From 334e5503b9ba0776c262954f7cf8661945ba49a6 Mon Sep 17 00:00:00 2001 From: "google-labs-jules[bot]" <161369871+google-labs-jules[bot]@users.noreply.github.com> Date: Tue, 27 Jan 2026 20:04:04 +0000 Subject: [PATCH 1/2] feat: make zoom icons conditional on drawing mode This change ensures that the Mapbox navigation controls (zoom in, zoom out, and compass) are only visible when the map is in 'Draw & Measure' mode, aligning their visibility with the drawing tools. - Added `navControlRef` to manage the `NavigationControl` instance. - Modified map initialization and `mapType` change handler to add/remove the control based on the current mode. - Updated E2E tests to switch to drawing mode before verifying zoom controls. - Improved cleanup logic to ensure controls are removed on unmount. Co-authored-by: ngoiyaeric <115367894+ngoiyaeric@users.noreply.github.com> --- components/map/mapbox-map.tsx | 40 +++++++++++++++++++++++++++++++++-- tests/map.spec.ts | 4 ++++ 2 files changed, 42 insertions(+), 2 deletions(-) diff --git a/components/map/mapbox-map.tsx b/components/map/mapbox-map.tsx index 3dd390cd..344a5d95 100644 --- a/components/map/mapbox-map.tsx +++ b/components/map/mapbox-map.tsx @@ -20,6 +20,7 @@ export const Mapbox: React.FC<{ position?: { latitude: number; longitude: number const map = useRef(null) const { setMap } = useMap() const drawRef = useRef(null) + const navControlRef = useRef(null) const rotationFrameRef = useRef(null) const polygonLabelsRef = useRef<{ [id: string]: mapboxgl.Marker }>({}) const lineLabelsRef = useRef<{ [id: string]: mapboxgl.Marker }>({}) @@ -367,7 +368,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 @@ -436,8 +453,9 @@ export const Mapbox: React.FC<{ position?: { latitude: number; longitude: number preserveDrawingBuffer: true }) - if (window.innerWidth > 768) { - map.current.addControl(new mapboxgl.NavigationControl(), 'top-left') + if (mapType === MapToggleEnum.DrawingMode) { + navControlRef.current = new mapboxgl.NavigationControl(); + map.current.addControl(navControlRef.current, 'top-left'); } // Register event listeners @@ -502,6 +520,15 @@ export const Mapbox: React.FC<{ position?: { latitude: number; longitude: number 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()) @@ -603,6 +630,15 @@ export const Mapbox: React.FC<{ position?: { latitude: number; longitude: number } } + if (navControlRef.current) { + try { + map.current.removeControl(navControlRef.current) + navControlRef.current = null + } catch (e) { + console.log('Navigation control already removed') + } + } + Object.values(polygonLabelsRef.current).forEach(marker => marker.remove()) Object.values(lineLabelsRef.current).forEach(marker => marker.remove()) diff --git a/tests/map.spec.ts b/tests/map.spec.ts index 5d6aa972..1047d3e1 100644 --- a/tests/map.spec.ts +++ b/tests/map.spec.ts @@ -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'); From ff33a46d31f9153a38d2de34c8a0b94b6d0403e9 Mon Sep 17 00:00:00 2001 From: "google-labs-jules[bot]" <161369871+google-labs-jules[bot]@users.noreply.github.com> Date: Wed, 28 Jan 2026 09:33:37 +0000 Subject: [PATCH 2/2] fix: prevent map recreation on mode switch This commit fixes a regression where the Mapbox instance was being destroyed and recreated on every mode switch. This was caused by including `mapType` in the dependency array of the initialization `useEffect`, whose cleanup function removed the map. - Removed `mapType` from initialization `useEffect` dependencies. - Added `mapTypeRef` to track the current mode without re-triggering map initialization. - Stabilized callbacks used in the initialization effect. - Removed redundant cleanup `useEffect` at the end of `mapbox-map.tsx`. - Ensured resolution search results (GeoJSON layers) persist through mode switches. Co-authored-by: ngoiyaeric <115367894+ngoiyaeric@users.noreply.github.com> --- components/map/mapbox-map.tsx | 117 +++++++++------------------------- 1 file changed, 30 insertions(+), 87 deletions(-) diff --git a/components/map/mapbox-map.tsx b/components/map/mapbox-map.tsx index 344a5d95..69b601b9 100644 --- a/components/map/mapbox-map.tsx +++ b/components/map/mapbox-map.tsx @@ -35,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(null) + const mapTypeRef = useRef(mapType) + + useEffect(() => { + mapTypeRef.current = mapType + }, [mapType]) // Refs for long-press functionality const longPressTimerRef = useRef(null); @@ -222,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 @@ -232,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(() => { @@ -296,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') @@ -313,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(() => { @@ -453,11 +458,6 @@ export const Mapbox: React.FC<{ position?: { latitude: number; longitude: number preserveDrawingBuffer: true }) - if (mapType === MapToggleEnum.DrawingMode) { - navControlRef.current = new mapboxgl.NavigationControl(); - map.current.addControl(navControlRef.current, 'top-left'); - } - // Register event listeners map.current.on('moveend', captureMapCenter) map.current.on('mousedown', handleUserInteraction) @@ -490,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'); + } } // If not in real-time mode, start rotation - if (mapType !== MapToggleEnum.RealTimeMode) { + if (mapTypeRef.current !== MapToggleEnum.RealTimeMode) { startRotation() } @@ -507,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) @@ -516,6 +524,7 @@ 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') } @@ -547,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 ]) // Handle position updates from props @@ -574,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 @@ -606,67 +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') - } - } - - if (navControlRef.current) { - try { - map.current.removeControl(navControlRef.current) - navControlRef.current = null - } catch (e) { - console.log('Navigation 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 (