Feat: Add custom user location marker to map #369
Conversation
This commit introduces a custom marker for the user's live location on the map. - A new `user-marker.tsx` component has been created to render the custom marker. - The `mapbox-map.tsx` component has been updated to use this new marker, creating it when the user's position is first available and updating its position as it changes. - The user-provided image for the marker has been added to the `public` directory.
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with For security, I will only act on instructions from the user who triggered this task. New to Jules? Learn more at jules.google/docs. |
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the Comment |
|
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
This commit introduces a custom marker for the user's live location on the map. - A new `user-marker.tsx` component has been created to render the custom marker. - The `mapbox-map.tsx` component has been updated to use this new marker, creating it when the user's position is first available and updating its position as it changes. - The user-provided image for the marker has been added to the `public` directory. This commit also addresses a build error by updating the deprecated `ReactDOM.render` to `createRoot` and a build warning by using `next/image` for image optimization.
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
|||||||||||||||||||||||||
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||||
![charliecreates[bot]](https://avatars.githubusercontent.com/in/1140780?s=60&v=4){kind=small}
There was a problem hiding this comment.
The main concern is lifecycle management of the React root created via createRoot for the custom user marker: it is never stored or unmounted, which risks memory leaks over time. Both cleanup paths remove the Mapbox marker but do not yet unmount the associated React component tree, so React resources are not fully released. The UserMarker component is functionally correct, though its current inline, fixed-size styling could be made more reusable and maintainable with configurable sizing and extracted styles. Addressing the root lifecycle will significantly improve the robustness of this feature.
Additional notes (2)
-
Maintainability |
components/map/mapbox-map.tsx:207-255
TheupdateMapPositionlogic now both changes the map view and manages the lifecycle of theuserMarkerRef, which mixes two separate concerns into one callback and adds complexity. This makes the function harder to understand and more error-prone if marker behavior needs to evolve (e.g., different marker per map mode, marker visibility toggles, etc.). Consider extracting the marker creation/update into a small helper (e.g.,updateUserMarker(latitude, longitude)) that only deals withuserMarkerRefandmap.current, and havingupdateMapPositioncall that helper before/afterflyTo. That would keep responsibilities clearer and simplify future changes to marker behavior without risking regressions in camera movement. -
Maintainability |
components/map/user-marker.tsx:8-25
The inline style approach is simple, but the hard-coded50pxcontainer and20pxinner image size means the marker scale is fixed and not easily reusable or themeable. It also makes it slightly harder to adjust the marker size in response to device pixel ratio or map zoom if you ever need that.
While not strictly incorrect, encapsulating styles in a CSS module or tailwind classes (if used elsewhere in the project) would improve maintainability and consistency with the rest of the codebase.
Summary of changes
Summary of Changes
- Added a new
UserMarkerReact component (components/map/user-marker.tsx) that renders a styled circular container with a user-location image usingnext/image. - Updated
Mapboxmap component (components/map/mapbox-map.tsx) to:- Import and use
createRootfromreact-dom/clientand the newUserMarkercomponent. - Maintain a
userMarkerRefto track a MapboxMarkerrepresenting the user's location. - Extend
updateMapPositionto create the marker (via a React root) on first use and update its position on subsequent updates while still animating the map withflyTo. - Ensure the custom user marker is removed and reference cleared in cleanup paths.
- Import and use
- Added the
public/user-location-marker.pngasset used byUserMarker.
| const markerElement = document.createElement('div') | ||
| const root = createRoot(markerElement) | ||
| root.render(<UserMarker />) | ||
| userMarkerRef.current = new mapboxgl.Marker({ | ||
| element: markerElement | ||
| }) | ||
| .setLngLat([longitude, latitude]) | ||
| .addTo(map.current) | ||
| } | ||
|
|
||
| await new Promise<void>(resolve => { | ||
| map.current?.flyTo({ | ||
| center: [longitude, latitude], | ||
| zoom: 12, | ||
| essential: true, | ||
| speed: 0.5, | ||
| curve: 1 | ||
| }) | ||
| map.current?.once('moveend', () => { | ||
| resolve() | ||
| }) | ||
| }) | ||
|
|
||
| setTimeout(() => { | ||
| if (mapType === MapToggleEnum.RealTimeMode) { | ||
| startRotation() | ||
| } | ||
| isUpdatingPositionRef.current = false | ||
| }, 500) | ||
| } catch (error) { | ||
| console.error('Error updating map position:', error) | ||
| isUpdatingPositionRef.current = false | ||
| }, 500) | ||
| } catch (error) { | ||
| console.error('Error updating map position:', error) | ||
| isUpdatingPositionRef.current = false | ||
| } |
There was a problem hiding this comment.
createRoot is correctly used to render UserMarker into a detached DOM node, but the React root instance is not retained or unmounted. This leaves React tree lifecycle unmanaged and risks memory leaks when the map is destroyed or if the marker is ever replaced, because root.unmount() is never called.
Also, the userMarkerRef only tracks the Mapbox Marker, not the React root, so you cannot currently cleanly unmount the React component during cleanup. This is particularly important in long-lived sessions with route changes or map reinitializations.
Suggestion
Consider storing the React root in a ref alongside the marker and explicitly unmounting it during cleanup when the marker is removed. For example:
const userMarkerRef = useRef<{
marker: mapboxgl.Marker | null
root: Root | null
}>({ marker: null, root: null })Then when creating the marker:
const markerElement = document.createElement('div')
const root = createRoot(markerElement)
root.render(<UserMarker />)
userMarkerRef.current = {
marker: new mapboxgl.Marker({ element: markerElement })
.setLngLat([longitude, latitude])
.addTo(map.current),
root,
}And in your cleanup (both in the main useEffect return and the map-style change cleanup), ensure you unmount:
if (userMarkerRef.current?.marker) {
userMarkerRef.current.marker.remove()
}
if (userMarkerRef.current?.root) {
userMarkerRef.current.root.unmount()
}
userMarkerRef.current = { marker: null, root: null }This keeps React and Mapbox lifecycles aligned and avoids orphaned React roots. Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
User description
This change adds a custom user location marker to the map, as requested by the user. It includes a new
UserMarkercomponent and updates theMapboxcomponent to use it.PR created automatically by Jules for task 4752322746026312812 started by @ngoiyaeric
PR Type
Enhancement
Description
Add custom user location marker component with styled container
Integrate marker into map with dynamic creation and position updates
Replace deprecated ReactDOM.render with createRoot for React 18 compatibility
Implement proper marker cleanup in component unmount lifecycle
Diagram Walkthrough
File Walkthrough
mapbox-map.tsx
Integrate custom user marker with React 18 supportcomponents/map/mapbox-map.tsx
createRootfrom react-dom/client for React 18 compatibilityuserMarkerRefto track custom user location marker instanceupdateMapPositioncallback
user-marker.tsx
Create custom user location marker componentcomponents/map/user-marker.tsx
icon