feat: Show Mapbox NavigationControl in DrawingMode and RealTimeMode #373
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Map as Mapbox Map
participant Control as NavigationControl
User->>Map: Change mapType to DrawingMode
Map->>Map: Check navigationControlRef
alt ref is null
Map->>Control: Create new NavigationControl
Control-->>Map: Instance created
Map->>Map: Add control to map
Map->>Map: Store in navigationControlRef
end
User->>Map: Change mapType to FreeMode
Map->>Map: Check navigationControlRef
alt ref exists
Map->>Control: Remove from map
Map->>Map: Clear navigationControlRef
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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 new NavigationControl handling correctly scopes the control to DrawingMode and RealTimeMode, but its dependence on isMapTypeChanged makes it somewhat fragile and tightly coupled to that flag. Removing the initial unconditional NavigationControl addition is correct, yet you now rely on later mode-change logic to attach the control, which might cause a brief mismatch between the initial mode and visible controls. Extracting the control management into a dedicated effect/helper that always derives visibility from mapType and map initialization would improve robustness and clarity.
Additional notes (1)
- Maintainability |
components/map/mapbox-map.tsx:353-373
The logic for adding/removingNavigationControlbased onmapTypeis correct, but it's tightly coupled toisMapTypeChanged. IfmapTypewere ever changed withoutisMapTypeChangedbeing true (or this flag were miscomputed), the control state could get out of sync with the mode. That makes the behavior somewhat brittle and harder to extend.
A more robust pattern is to have a small dedicated effect or helper that always reconciles the control visibility against the current mapType and map.current whenever either changes, instead of only when a particular flag is set.
Summary of changes
- Introduced a
navigationControlRef(useRef<mapboxgl.NavigationControl | null>) to keep track of a singleNavigationControlinstance on the map. - Removed the unconditional
NavigationControladdition during initial map creation. - Added logic on map mode change to:
- Add the navigation control when
mapTypeisDrawingModeorRealTimeMode. - Remove and clear the navigation control when switching to
FreeMode.
- Add the navigation control when
- Ensured the control is only instantiated once and reused across mode switches.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
components/map/mapbox-map.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
components/map/mapbox-map.tsx (1)
components/map-toggle.tsx (1)
MapToggle(14-39)
| const map = useRef<mapboxgl.Map | null>(null) | ||
| const { setMap } = useMap() | ||
| const drawRef = useRef<MapboxDraw | null>(null) | ||
| const navigationControlRef = useRef<mapboxgl.NavigationControl | null>(null) |
There was a problem hiding this comment.
🧩 Analysis chain
Reset navigationControlRef on unmount to avoid stale control instance across remounts
navigationControlRef is only updated inside the map-type effect; it’s never cleared in the unmount/teardown paths. If the component unmounts while in DrawingMode or RealTimeMode, the ref will still point at a NavigationControl instance on the next mount. Because the add logic is guarded with if (!navigationControlRef.current), this can prevent a control from ever being (re)attached to the new map instance.
Add explicit cleanup so the ref is nulled and (optionally) the control is removed before map.current.remove():
- if (map.current) {
- map.current.off('moveend', captureMapCenter)
+ if (map.current) {
+ if (navigationControlRef.current) {
+ try {
+ map.current.removeControl(navigationControlRef.current)
+ } catch (e) {
+ console.log('Navigation control already removed')
+ }
+ navigationControlRef.current = null
+ }
+ map.current.off('moveend', captureMapCenter)
…
- map.current.remove()
- map.current = null
+ map.current.remove()
+ map.current = null
}You’ll want equivalent handling in the duplicate cleanup effect at lines 578-615, or (better) dedupe those cleanups and have a single teardown path that also clears navigationControlRef.
Also applies to: 481-511, 578-615
I'll verify the concerns in this review by examining the actual code structure.
🏁 Script executed:
#!/bin/bash
# Extract and examine navigationControlRef usage and related effects
cat components/map/mapbox-map.tsx | head -n 650 | tail -n +20Length of output: 21799
Ensure navigationControlRef is cleared in all cleanup paths to prevent stale control references on remount
The two cleanup effects (lines 481–511 and 578–615) do not reset navigationControlRef, allowing a stale reference to persist across component remounts. If the component unmounts while in DrawingMode or RealTimeMode, the ref will retain the old NavigationControl instance. On remount, the guard if (!navigationControlRef.current) will be false, preventing a fresh control from being added to the new map.
Add cleanup before map.current.remove() in both effects:
if (navigationControlRef.current) {
try {
map.current.removeControl(navigationControlRef.current)
} catch (e) {
console.log('Navigation control already removed')
}
navigationControlRef.current = null
}Consolidate the two cleanup effects into a single teardown path if possible to avoid maintaining duplicate cleanup logic.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
components/map/mapbox-map.tsx (line 23 and cleanup areas around lines 481–511
and 578–615): the navigationControlRef declared at line 23 is not being cleared
in the component cleanup paths, which can leave a stale NavigationControl on
unmount and prevent a fresh control from being added on remount; update both
cleanup branches to null out navigationControlRef.current before calling
map.current.remove() (or wherever the map is torn down), and refactor the two
teardown effects into a single consolidated cleanup effect if possible so the
ref-reset logic is maintained in one place to avoid duplication.
| // Add navigation control for DrawingMode or RealTimeMode | ||
| if (mapType === MapToggleEnum.DrawingMode || mapType === MapToggleEnum.RealTimeMode) { | ||
| if (!navigationControlRef.current) { | ||
| navigationControlRef.current = new mapboxgl.NavigationControl() | ||
| if (map.current) { | ||
| map.current.addControl(navigationControlRef.current, 'top-left') | ||
| } | ||
| } | ||
| } else { | ||
| // Remove navigation control in FreeMode | ||
| if (navigationControlRef.current) { | ||
| if (map.current) { | ||
| map.current.removeControl(navigationControlRef.current) | ||
| } | ||
| navigationControlRef.current = null | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
NavigationControl is not added on initial mount for Drawing/RealTime modes
The useEffect handling map-type changes only runs after render and currently bails out when the map isn’t initialized:
if (!map.current || !initializedRef.current) returnYou also removed the unconditional NavigationControl creation from the map init (around line 431). Combined, this means:
- On first mount,
mapTypeeffect runs before the map is initialized → earlyreturn, so no control is added. - The effect won’t re-run until
mapTypechanges, so if the initialmapTypeisRealTimeModeorDrawingMode(which is likely), the user won’t see zoom controls until they toggle modes at least once.
To ensure the control shows up immediately in those modes, initialize it in the map load handler and keep this effect strictly for mode transitions:
map.current.on('load', () => {
if (!map.current) return
setMap(map.current)
…
if (mapType === MapToggleEnum.DrawingMode) {
setupDrawingTools()
}
+
+ // Ensure NavigationControl is present on initial load for Drawing/RealTime modes
+ if (mapType === MapToggleEnum.DrawingMode || mapType === MapToggleEnum.RealTimeMode) {
+ if (!navigationControlRef.current) {
+ navigationControlRef.current = new mapboxgl.NavigationControl()
+ map.current.addControl(navigationControlRef.current, 'top-left')
+ }
+ }
…
})The existing useEffect block at lines 356-372 can then reliably handle adding/removing the control when mapType changes (including removing it in FreeMode).
Also applies to: 431-431
I'll help verify this review comment by examining the actual code structure. Let me generate shell scripts to investigate the implementation.
🏁 Script executed:
#!/bin/bash
# Find and display the mapbox-map.tsx file structure
fd -t f "mapbox-map.tsx" --exec cat {} \;Length of output: 22680
🏁 Script executed:
#!/bin/bash
# Check specific line ranges mentioned in the review
# Lines 356-372 for the useEffect and context around line 431
fd -t f "mapbox-map.tsx" --exec sed -n '340,450p' {} \;Length of output: 4046
🏁 Script executed:
#!/bin/bash
# Search for NavigationControl usage and initialization
rg -nC5 "NavigationControl" --type=tsxLength of output: 27
🏁 Script executed:
#!/bin/bash
# Search for map initialization and 'load' event handlers
rg -nC5 "map\.current\.on\s*\(\s*['\"]load" --type=tsxLength of output: 27
Add NavigationControl initialization to map load handler
Your review is correct. The NavigationControl won't appear on initial mount when mapType is DrawingMode or RealTimeMode because:
- The
mapTypeeffect (lines 341–428) checksif (!map.current || !initializedRef.current) returnbefore doing anything - On first render, this effect runs before the map finishes loading, so it bails out early
- The effect won't run again unless
mapTypechanges (it's in the dependency array) - The load handler (lines 412–431) never initializes NavigationControl
To fix this, initialize NavigationControl in the map load handler when the initial mapType is DrawingMode or RealTimeMode. Add the logic immediately after setupDrawingTools() at line ~427:
// Ensure NavigationControl is present on initial load for Drawing/RealTime modes
if (mapType === MapToggleEnum.DrawingMode || mapType === MapToggleEnum.RealTimeMode) {
if (!navigationControlRef.current) {
navigationControlRef.current = new mapboxgl.NavigationControl()
map.current.addControl(navigationControlRef.current, 'top-left')
}
}The existing mapType effect can then reliably handle adding/removing the control on subsequent mode transitions.
🤖 Prompt for AI Agents
components/map/mapbox-map.tsx around lines 356 to 372: NavigationControl is only
added by the mapType effect which bails out before the map finishes loading, so
the control never appears on initial mount when mapType is DrawingMode or
RealTimeMode; to fix, add the same initialization into the map load handler
(immediately after setupDrawingTools()): check if mapType is DrawingMode or
RealTimeMode, and if navigationControlRef.current is null create a new
mapboxgl.NavigationControl(), add it to map.current at 'top-left', and assign it
to navigationControlRef.current so subsequent effects can manage it; keep the
guard to avoid duplicate creation.
|
@coderabbitai the zoom icons are not showing even when using the mapping tools |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 22680 @ngoiyaeric I found the issue! The NavigationControl is only being added when the map mode changes, but not during the initial map load. If you start in DrawingMode or RealTimeMode, the control never gets added. Here's the fix - you need to add the NavigationControl initialization in the map.current.on('load', () => {
if (!map.current) return
setMap(map.current) // Set map instance in context
// Add terrain and sky
map.current.addSource('mapbox-dem', {
type: 'raster-dem',
url: 'mapbox://mapbox.mapbox-terrain-dem-v1',
tileSize: 512,
maxzoom: 14,
})
map.current.setTerrain({ source: 'mapbox-dem', exaggeration: 1.5 })
map.current.addLayer({
id: 'sky',
type: 'sky',
paint: {
'sky-type': 'atmosphere',
'sky-atmosphere-sun': [0.0, 0.0],
'sky-atmosphere-sun-intensity': 15,
},
})
// Add navigation control for initial DrawingMode or RealTimeMode
if (mapType === MapToggleEnum.DrawingMode || mapType === MapToggleEnum.RealTimeMode) {
navigationControlRef.current = new mapboxgl.NavigationControl()
map.current.addControl(navigationControlRef.current, 'top-left')
}
// Initialize drawing tools based on initial mode
if (mapType === MapToggleEnum.DrawingMode) {
setupDrawingTools()
}
// If not in real-time mode, start rotation
if (mapType !== MapToggleEnum.RealTimeMode) {
startRotation()
}
initializedRef.current = true
setIsMapLoaded(true) // Set map loaded state to true
setupGeolocationWatcher()
})The key addition is these lines: // Add navigation control for initial DrawingMode or RealTimeMode
if (mapType === MapToggleEnum.DrawingMode || mapType === MapToggleEnum.RealTimeMode) {
navigationControlRef.current = new mapboxgl.NavigationControl()
map.current.addControl(navigationControlRef.current, 'top-left')
}This ensures the NavigationControl is added during the initial map load if you're starting in DrawingMode or RealTimeMode, and then the existing effect at line 389 will handle adding/removing it when switching between modes. |
User description
This PR implements the requested change to show the Mapbox NavigationControl (zoom icons) in both
DrawingModeandRealTimeMode, ensuring users have precise map control in both scenarios. The control is hidden inFreeMode.PR Type
Enhancement
Description
Implement dynamic NavigationControl visibility based on map mode
Show zoom controls in DrawingMode and RealTimeMode
Hide zoom controls in FreeMode
Manage NavigationControl lifecycle with proper ref tracking
Diagram Walkthrough
flowchart LR mapType{"Map Mode Changed?"} drawing["DrawingMode or RealTimeMode"] free["FreeMode"] addControl["Add NavigationControl"] removeControl["Remove NavigationControl"] mapType -->|Yes| drawing mapType -->|Yes| free drawing --> addControl free --> removeControlFile Walkthrough
mapbox-map.tsx
Dynamic NavigationControl management by map modecomponents/map/mapbox-map.tsx
navigationControlRefto track NavigationControl instanceand RealTimeMode
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.