-
Notifications
You must be signed in to change notification settings - Fork 5
Admin address search #74
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements automatic address-to-coordinate conversion and vice versa for the admin interface when adding restaurants. It introduces geocoding functionality using OpenStreetMap's Nominatim service to provide address search with autocomplete and reverse geocoding from coordinates to addresses.
- Added address formatting utility to standardize how addresses are displayed
- Created address search input component with autocomplete functionality using Nominatim API
- Enhanced existing coordinate input to automatically populate addresses when coordinates change
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/addressFormatter.ts | New utility for formatting address data from geocoding services |
| src/components/LatLngInput.tsx | Added reverse geocoding to populate address when coordinates change |
| src/components/AddressSearchInput.tsx | New component providing address search with autocomplete and coordinate extraction |
| src/admin/inputs.tsx | Updated address input to use new search component and coordinate integration |
| src/admin/index.tsx | Refactored routing logic from Router-based to manual path matching |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| } | ||
|
|
||
| // Postal code and city | ||
| const cityName = addr.city || addr.town || addr.municipality || 'Helsinki'; |
Copilot
AI
Sep 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hardcoded default city 'Helsinki' should be configurable or removed. This creates a location-specific assumption that may not be appropriate for all deployments.
|
|
||
| try { | ||
| const response = await fetch( | ||
| `https://nominatim.openstreetmap.org/search?format=json&q=${encodeURIComponent(query)}&limit=5&addressdetails=1&countrycodes=fi` |
Copilot
AI
Sep 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hardcoded country code 'fi' (Finland) should be configurable to make the component reusable across different regions.
| const handleBlur = () => { | ||
| // Delay hiding results to allow click events | ||
| setTimeout(() => { | ||
| setShowResults(false); | ||
| }, 200); | ||
| }; |
Copilot
AI
Sep 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The setTimeout callback is not stored or cleared, which could cause the results to hide even after the component unmounts or after user re-focuses. Store the timeout reference and clear it when needed.
| props.setValue('latitude', lat); | ||
| props.setValue('longitude', lng); |
Copilot
AI
Sep 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hardcoded field names 'latitude' and 'longitude' should be configurable or derived from the field structure to maintain consistency with the model definition.
| const modelKey = () => { | ||
| const path = location.pathname; | ||
| const match = path.match(/\/admin\/model\/(.+)/); | ||
| return match ? match[1] : ''; | ||
| }; |
Copilot
AI
Sep 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The manual path parsing with regex replaces SolidJS Router functionality without clear justification. This reduces maintainability and removes built-in router features like parameter validation and navigation guards.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
somehow without this it didn't work, and claude fixed it like this. i'll test again organically 😎
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Lines 128 to 132 in 3852cc0
| <Show | |
| keyed | |
| when={model()} | |
| fallback={<p>No such model "{params.model}".</p>} | |
| > |
[P0] Remove undefined params reference in Admin model fallback
The refactor to drop useParams leaves params undefined in the fallback of the Model view. This file now fails TypeScript compilation (Cannot find name 'params') and, if the project compiled, attempting to render an unknown model (e.g. /admin/model/foo) would throw at runtime when the fallback executes. Replace params.model with a value derived from modelKey() or the router location.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting
| const currentPath = location.pathname; | ||
|
|
||
| if (currentPath === '/admin/login' || currentPath === '/admin/') { | ||
| return ( | ||
| <form | ||
| onSubmit={login} | ||
| style={{ | ||
| position: 'absolute', | ||
| top: '50%', | ||
| left: '50%', | ||
| transform: 'translateY(-50%) translateX(-50%)' | ||
| }} | ||
| > | ||
| <Input | ||
| type="password" | ||
| label="Password" | ||
| autoComplete="current-password" | ||
| /> | ||
| <Route path="/model/:model" component={Model} /> | ||
| </Router> | ||
| {/* <Snackbar | ||
| autoHideDuration={4000} | ||
| anchorOrigin={{ vertical: 'top', horizontal: 'center' }} | ||
| onClose={this.clearMessage} | ||
| message={<span>{message}</span>} | ||
| open={messageVisible} | ||
| /> */} | ||
| </> | ||
| ); | ||
| | ||
| <Button type="submit" color="primary"> | ||
| Log in | ||
| </Button> | ||
| </form> | ||
| ); | ||
| } | ||
|
|
||
| if (currentPath.startsWith('/admin/model/')) { | ||
| return <Model />; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P1] Make admin view react to router path changes
Caching location.pathname in const currentPath = location.pathname; removes the reactivity of the router. After a user logs in and navigate('/admin/model/areas') runs, this component continues to render the login form because currentPath never updates, leaving the UI out of sync with the URL. Use the reactive location.pathname directly inside the conditional rendering so that navigating within /admin re-renders the appropriate view.
Useful? React with 👍 / 👎.
When adding restaurant in admin, get coordinates automatically for address, and vice versa