Skip to content

feat: Show Mapbox NavigationControl in DrawingMode and RealTimeMode #373

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: Show Mapbox NavigationControl in DrawingMode and RealTimeMode #373

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 20 additions & 1 deletion components/map/mapbox-map.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ export const Mapbox: React.FC<{ position?: { latitude: number; longitude: number
const map = useRef<mapboxgl.Map | null>(null)
const { setMap } = useMap()
const drawRef = useRef<MapboxDraw | null>(null)
const navigationControlRef = useRef<mapboxgl.NavigationControl | null>(null)
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 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 +20

Length 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.

const rotationFrameRef = useRef<number | null>(null)
const polygonLabelsRef = useRef<{ [id: string]: mapboxgl.Marker }>({})
const lineLabelsRef = useRef<{ [id: string]: mapboxgl.Marker }>({})
Expand Down Expand Up @@ -352,6 +353,24 @@ export const Mapbox: React.FC<{ position?: { latitude: number; longitude: number
// Handle geolocation setup based on mode
setupGeolocationWatcher()

// 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
}
}
Comment on lines +356 to +372
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 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) return

You also removed the unconditional NavigationControl creation from the map init (around line 431). Combined, this means:

  • On first mount, mapType effect runs before the map is initialized → early return, so no control is added.
  • The effect won’t re-run until mapType changes, so if the initial mapType is RealTimeMode or DrawingMode (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=tsx

Length of output: 27

🏁 Script executed:

#!/bin/bash
# Search for map initialization and 'load' event handlers
rg -nC5 "map\.current\.on\s*\(\s*['\"]load" --type=tsx

Length 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:

  1. The mapType effect (lines 341–428) checks if (!map.current || !initializedRef.current) return before doing anything
  2. On first render, this effect runs before the map finishes loading, so it bails out early
  3. The effect won't run again unless mapType changes (it's in the dependency array)
  4. 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.


// Handle draw controls based on mode
if (mapType === MapToggleEnum.DrawingMode) {
// If switching to drawing mode, setup drawing tools
Expand Down Expand Up @@ -409,7 +428,7 @@ export const Mapbox: React.FC<{ position?: { latitude: number; longitude: number
preserveDrawingBuffer: true
})

map.current.addControl(new mapboxgl.NavigationControl(), 'top-left')


// Register event listeners
map.current.on('moveend', captureMapCenter)
Expand Down