Skip to content

Make zoom icons conditional on drawing mode #455

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
ngoiyaeric wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Make zoom icons conditional on drawing mode #455

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
334e550
feat: make zoom icons conditional on drawing mode
google-labs-jules[bot] Jan 27, 2026
ff33a46
fix: prevent map recreation on mode switch
google-labs-jules[bot] Jan 28, 2026
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
133 changes: 56 additions & 77 deletions 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 navControlRef = useRef<mapboxgl.NavigationControl | null>(null)
const rotationFrameRef = useRef<number | null>(null)
const polygonLabelsRef = useRef<{ [id: string]: mapboxgl.Marker }>({})
const lineLabelsRef = useRef<{ [id: string]: mapboxgl.Marker }>({})
Expand All @@ -34,6 +35,11 @@ export const Mapbox: React.FC<{ position?: { latitude: number; longitude: number
const { mapData, setMapData } = useMapData(); // Consume the new context, get setMapData
const { setIsMapLoaded } = useMapLoading(); // Get setIsMapLoaded from context
const previousMapTypeRef = useRef<MapToggleEnum | null>(null)
const mapTypeRef = useRef<MapToggleEnum>(mapType)

useEffect(() => {
mapTypeRef.current = mapType
}, [mapType])

// Refs for long-press functionality
const longPressTimerRef = useRef<NodeJS.Timeout | null>(null);
Expand Down Expand Up @@ -221,7 +227,7 @@ export const Mapbox: React.FC<{ position?: { latitude: number; longitude: number
})
})
setTimeout(() => {
if (mapType === MapToggleEnum.RealTimeMode) {
if (mapTypeRef.current === MapToggleEnum.RealTimeMode) {
startRotation()
}
isUpdatingPositionRef.current = false
Expand All @@ -231,7 +237,7 @@ export const Mapbox: React.FC<{ position?: { latitude: number; longitude: number
isUpdatingPositionRef.current = false
}
}
}, [mapType, startRotation, stopRotation])
}, [startRotation, stopRotation])

// Set up drawing tools
const setupDrawingTools = useCallback(() => {
Expand Down Expand Up @@ -295,7 +301,7 @@ export const Mapbox: React.FC<{ position?: { latitude: number; longitude: number
geolocationWatchIdRef.current = null
}

if (mapType !== MapToggleEnum.RealTimeMode) return
if (mapTypeRef.current !== MapToggleEnum.RealTimeMode) return

if (!navigator.geolocation) {
toast('Geolocation is not supported by your browser')
Expand All @@ -312,7 +318,7 @@ export const Mapbox: React.FC<{ position?: { latitude: number; longitude: number
}

geolocationWatchIdRef.current = navigator.geolocation.watchPosition(success, error)
}, [mapType, updateMapPosition])
}, [updateMapPosition])

// Capture map center changes
const captureMapCenter = useCallback(() => {
Expand Down Expand Up @@ -367,7 +373,23 @@ export const Mapbox: React.FC<{ position?: { latitude: number; longitude: number
if (mapType === MapToggleEnum.DrawingMode) {
// If switching to drawing mode, setup drawing tools
setupDrawingTools()

// Setup zoom controls if not already present
if (!navControlRef.current) {
navControlRef.current = new mapboxgl.NavigationControl();
map.current.addControl(navControlRef.current, 'top-left');
}
} else {
// Remove zoom controls if present
if (navControlRef.current) {
try {
map.current.removeControl(navControlRef.current);
navControlRef.current = null;
} catch (e) {
console.log('Error removing navigation control:', e);
}
}

// If switching from drawing mode, remove drawing tools but save features
if (drawRef.current) {
// Save current drawings before removing control
Expand Down Expand Up @@ -436,10 +458,6 @@ export const Mapbox: React.FC<{ position?: { latitude: number; longitude: number
preserveDrawingBuffer: true
})

if (window.innerWidth > 768) {
map.current.addControl(new mapboxgl.NavigationControl(), 'top-left')
}

// Register event listeners
map.current.on('moveend', captureMapCenter)
map.current.on('mousedown', handleUserInteraction)
Expand Down Expand Up @@ -472,13 +490,17 @@ export const Mapbox: React.FC<{ position?: { latitude: number; longitude: number
},
})

// Initialize drawing tools based on initial mode
if (mapType === MapToggleEnum.DrawingMode) {
// Initial setup for the current mode
if (mapTypeRef.current === MapToggleEnum.DrawingMode) {
setupDrawingTools()
if (!navControlRef.current) {
navControlRef.current = new mapboxgl.NavigationControl();
map.current.addControl(navControlRef.current, 'top-left');
}
}
Comment on lines +493 to 500
Copy link
Contributor

@coderabbitai coderabbitai bot Jan 28, 2026

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial

Consider adding a defensive null check for map.current.

While the early return at line 470 typically ensures map.current exists, adding a guard before addControl would be consistent with the defensive try-catch patterns used elsewhere in this file.

♻️ Suggested improvement 
         // Initial setup for the current mode
         if (mapTypeRef.current === MapToggleEnum.DrawingMode) {
           setupDrawingTools()
-          if (!navControlRef.current) {
+          if (!navControlRef.current && map.current) {
             navControlRef.current = new mapboxgl.NavigationControl();
             map.current.addControl(navControlRef.current, 'top-left');
           }
         }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Initial setup for the current mode
if (mapTypeRef.current === MapToggleEnum.DrawingMode) {
setupDrawingTools()
if (!navControlRef.current) {
navControlRef.current = new mapboxgl.NavigationControl();
map.current.addControl(navControlRef.current, 'top-left');
}
}
// Initial setup for the current mode
if (mapTypeRef.current === MapToggleEnum.DrawingMode) {
setupDrawingTools()
if (!navControlRef.current && map.current) {
navControlRef.current = new mapboxgl.NavigationControl();
map.current.addControl(navControlRef.current, 'top-left');
}
}
🤖 Prompt for AI Agents 
In `@components/map/mapbox-map.tsx` around lines 493 - 500, Add a defensive
null-check for map.current before calling map.current.addControl inside the
MapToggleEnum.DrawingMode branch: ensure when mapTypeRef.current ===
MapToggleEnum.DrawingMode and after setupDrawingTools() you only call
map.current.addControl(...) if map.current is truthy (and handle/log the
unexpected null case); update the block that assigns navControlRef.current and
calls addControl to guard on map.current to match other defensive patterns in
this file.


// If not in real-time mode, start rotation
if (mapType !== MapToggleEnum.RealTimeMode) {
if (mapTypeRef.current !== MapToggleEnum.RealTimeMode) {
startRotation()
}

Expand All @@ -489,6 +511,10 @@ export const Mapbox: React.FC<{ position?: { latitude: number; longitude: number
}

return () => {
if (longPressTimerRef.current) {
clearTimeout(longPressTimerRef.current);
}

if (map.current) {
map.current.off('moveend', captureMapCenter)

Expand All @@ -498,10 +524,20 @@ export const Mapbox: React.FC<{ position?: { latitude: number; longitude: number
map.current.off('draw.delete', updateMeasurementLabels)
map.current.off('draw.update', updateMeasurementLabels)
map.current.removeControl(drawRef.current)
drawRef.current = null
} catch (e) {
console.log('Draw control already removed')
}
}

if (navControlRef.current) {
try {
map.current.removeControl(navControlRef.current)
navControlRef.current = null
} catch (e) {
console.log('Navigation control already removed')
}
}

// Clean up any existing labels
Object.values(polygonLabelsRef.current).forEach(marker => marker.remove())
Expand All @@ -520,16 +556,16 @@ export const Mapbox: React.FC<{ position?: { latitude: number; longitude: number
}
}
}, [
handleUserInteraction,
startRotation,
stopRotation,
mapType,
updateMeasurementLabels,
setupGeolocationWatcher,
captureMapCenter,
setupDrawingTools,
captureMapCenter,
handleUserInteraction,
setIsMapLoaded,
setMap
setMap,
setupDrawingTools,
setupGeolocationWatcher,
startRotation,
stopRotation,
updateMeasurementLabels
// Removed position, mapData.cameraState and mapType to avoid map recreation
])
Comment on lines 558 to 569
Copy link
Contributor

@coderabbitai coderabbitai bot Jan 28, 2026

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial

Intentional dependency omissions are documented - consider adding eslint disable comment.

The comment explains why position, mapData.cameraState, and mapType are omitted to prevent map recreation. This is a valid architectural decision for this component.

If the react-hooks/exhaustive-deps eslint rule flags this, consider adding an explicit disable comment with explanation:

♻️ Suggested improvement 
   }, [
     captureMapCenter,
     handleUserInteraction,
     setIsMapLoaded,
     setMap,
     setupDrawingTools,
     setupGeolocationWatcher,
     startRotation,
     stopRotation,
     updateMeasurementLabels
-    // Removed position, mapData.cameraState and mapType to avoid map recreation
+    // eslint-disable-next-line react-hooks/exhaustive-deps -- Intentionally omit position, mapData.cameraState, and mapType to prevent map recreation on mode switches
   ])
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
}, [
handleUserInteraction,
startRotation,
stopRotation,
mapType,
updateMeasurementLabels,
setupGeolocationWatcher,
captureMapCenter,
setupDrawingTools,
captureMapCenter,
handleUserInteraction,
setIsMapLoaded,
setMap
setMap,
setupDrawingTools,
setupGeolocationWatcher,
startRotation,
stopRotation,
updateMeasurementLabels
// Removed position, mapData.cameraState and mapType to avoid map recreation
])
}, [
captureMapCenter,
handleUserInteraction,
setIsMapLoaded,
setMap,
setupDrawingTools,
setupGeolocationWatcher,
startRotation,
stopRotation,
updateMeasurementLabels
// eslint-disable-next-line react-hooks/exhaustive-deps -- Intentionally omit position, mapData.cameraState, and mapType to prevent map recreation on mode switches
])
🤖 Prompt for AI Agents 
In `@components/map/mapbox-map.tsx` around lines 558 - 569, The dependency array
intentionally omits position, mapData.cameraState, and mapType to avoid
recreating the map; add an eslint disable comment to document this and suppress
react-hooks/exhaustive-deps warnings (e.g., place a // eslint-disable-next-line
react-hooks/exhaustive-deps with a short rationale) immediately above the
useEffect that depends on captureMapCenter, handleUserInteraction,
setIsMapLoaded, setMap, setupDrawingTools, setupGeolocationWatcher,
startRotation, stopRotation, and updateMeasurementLabels so reviewers and
linters understand the intentional omission.


// Handle position updates from props
Expand All @@ -547,11 +583,6 @@ export const Mapbox: React.FC<{ position?: { latitude: number; longitude: number
updateMapPosition(lat, lng);
}
}
// TODO: Handle mapData.mapFeature for drawing routes, polygons, etc. in a future step.
// For example:
// if (mapData.mapFeature && mapData.mapFeature.route_geometry && typeof drawRoute === 'function') {
// drawRoute(mapData.mapFeature.route_geometry); // Implement drawRoute function if needed
// }
}, [mapData.targetPosition, mapData.mapFeature, updateMapPosition]);

// Long-press handlers
Expand Down Expand Up @@ -579,58 +610,6 @@ export const Mapbox: React.FC<{ position?: { latitude: number; longitude: number
}
}, []);

// Cleanup for the main useEffect
useEffect(() => {
// ... existing useEffect logic ...
return () => {
// ... existing cleanup logic ...
if (longPressTimerRef.current) { // Cleanup timer on component unmount
clearTimeout(longPressTimerRef.current);
longPressTimerRef.current = null;
}
// ... existing cleanup logic for map and geolocation ...
if (map.current) {
map.current.off('moveend', captureMapCenter)

if (drawRef.current) {
try {
map.current.off('draw.create', updateMeasurementLabels)
map.current.off('draw.delete', updateMeasurementLabels)
map.current.off('draw.update', updateMeasurementLabels)
map.current.removeControl(drawRef.current)
} catch (e) {
console.log('Draw control already removed')
}
}

Object.values(polygonLabelsRef.current).forEach(marker => marker.remove())
Object.values(lineLabelsRef.current).forEach(marker => marker.remove())

stopRotation()
setIsMapLoaded(false)
setMap(null)
map.current.remove()
map.current = null
}

if (geolocationWatchIdRef.current !== null) {
navigator.geolocation.clearWatch(geolocationWatchIdRef.current)
geolocationWatchIdRef.current = null
}
};
}, [
handleUserInteraction,
startRotation,
stopRotation,
mapType, // mapType is already here, good.
updateMeasurementLabels,
setupGeolocationWatcher,
captureMapCenter,
setupDrawingTools,
setIsMapLoaded,
setMap
]);


return (
<div className="relative h-full w-full">
Expand Down
4 changes: 4 additions & 0 deletions tests/map.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,10 @@ test.describe('Map functionality', () => {
return;
}

// Switch to drawing mode to make zoom controls visible
await page.click('[data-testid="map-toggle"]');
await page.click('[data-testid="map-mode-draw"]');

const hasMap = await page.evaluate(() => Boolean((window as any).map));
if (!hasMap) test.skip(true, 'Map instance not available on window for E2E');

Expand Down