From c1fcd71c1c78d0b70bb9b804abb75b9f2883e606 Mon Sep 17 00:00:00 2001 From: jubnl Date: Fri, 1 May 2026 01:26:26 +0200 Subject: [PATCH] fix(map): prevent auto zoom-out when opening/closing place inspector (issue #921) Both Leaflet and Mapbox GL renderers now gate fitBounds strictly on fitKey increments from the parent. Selecting or dismissing a place inspector changes paddingOpts (via hasInspector) but no longer triggers a re-fit that zoomed the map out to the full trip extent when no day was selected. Also removes the zoom-12 visibility gate on Leaflet route info pills so they render at all zoom levels when a route is active. --- client/src/components/Map/MapView.test.tsx | 50 +++++- client/src/components/Map/MapView.tsx | 15 +- client/src/components/Map/MapViewGL.test.tsx | 164 +++++++++++++++++++ client/src/components/Map/MapViewGL.tsx | 11 +- 4 files changed, 211 insertions(+), 29 deletions(-) create mode 100644 client/src/components/Map/MapViewGL.test.tsx diff --git a/client/src/components/Map/MapView.test.tsx b/client/src/components/Map/MapView.test.tsx index 7a6a4c16..902a8f32 100644 --- a/client/src/components/Map/MapView.test.tsx +++ b/client/src/components/Map/MapView.test.tsx @@ -7,6 +7,16 @@ import { resetAllStores } from '../../../tests/helpers/store' import { buildPlace } from '../../../tests/helpers/factories' import * as photoService from '../../services/photoService' +const mapMock = vi.hoisted(() => ({ + panTo: vi.fn(), + setView: vi.fn(), + fitBounds: vi.fn(), + getZoom: vi.fn().mockReturnValue(10), + on: vi.fn(), + off: vi.fn(), + panBy: vi.fn(), +})) + vi.mock('react-leaflet', () => ({ MapContainer: ({ children }: any) =>
{children}
, TileLayer: () =>
, @@ -27,15 +37,7 @@ vi.mock('react-leaflet', () => ({ Polyline: ({ positions }: any) =>
, CircleMarker: () =>
, Circle: () =>
, - useMap: () => ({ - panTo: vi.fn(), - setView: vi.fn(), - fitBounds: vi.fn(), - getZoom: () => 10, - on: vi.fn(), - off: vi.fn(), - panBy: vi.fn(), - }), + useMap: () => mapMock, useMapEvents: () => ({}), })) @@ -79,6 +81,7 @@ function buildMapPlace(overrides: Record = {}) { } afterEach(() => { + vi.clearAllMocks() resetAllStores() }) @@ -216,4 +219,33 @@ describe('MapView', () => { render() expect(screen.getByTestId('marker')).toBeTruthy() }) + + it('FE-COMP-MAPVIEW-018: changing selectedPlaceId/hasInspector does not refit bounds (issue #921)', () => { + const places = [ + buildMapPlace({ id: 1, lat: 48.8584, lng: 2.2945 }), + buildMapPlace({ id: 2, lat: 48.86, lng: 2.337 }), + ] + const { rerender } = render() + const initialCount = mapMock.fitBounds.mock.calls.length + + // Toggle selectedPlaceId on — mimics opening place inspector (hasInspector flips, + // paddingOpts memo creates new object). fitBounds must NOT fire again. + rerender() + expect(mapMock.fitBounds).toHaveBeenCalledTimes(initialCount) + + // Toggle selectedPlaceId off — mimics closing inspector via X button. + rerender() + expect(mapMock.fitBounds).toHaveBeenCalledTimes(initialCount) + }) + + it('FE-COMP-MAPVIEW-019: bumping fitKey triggers a new fitBounds call', () => { + const places = [ + buildMapPlace({ id: 1, lat: 48.8584, lng: 2.2945 }), + ] + const { rerender } = render() + const afterFirst = mapMock.fitBounds.mock.calls.length + + rerender() + expect(mapMock.fitBounds.mock.calls.length).toBeGreaterThan(afterFirst) + }) }) diff --git a/client/src/components/Map/MapView.tsx b/client/src/components/Map/MapView.tsx index 5af4bea3..a4a9831d 100644 --- a/client/src/components/Map/MapView.tsx +++ b/client/src/components/Map/MapView.tsx @@ -186,7 +186,7 @@ function BoundsController({ places, fitKey, paddingOpts, hasDayDetail }: BoundsC } } } catch {} - }, [fitKey, places, paddingOpts, map, hasDayDetail]) + }, [fitKey]) // eslint-disable-line react-hooks/exhaustive-deps return null } @@ -233,18 +233,7 @@ interface RouteLabelProps { } function RouteLabel({ midpoint, walkingText, drivingText }: RouteLabelProps) { - const map = useMap() - const [visible, setVisible] = useState(map ? map.getZoom() >= 12 : false) - - useEffect(() => { - if (!map) return - const check = () => setVisible(map.getZoom() >= 12) - check() - map.on('zoomend', check) - return () => map.off('zoomend', check) - }, [map]) - - if (!visible || !midpoint) return null + if (!midpoint) return null const icon = L.divIcon({ className: 'route-info-pill', diff --git a/client/src/components/Map/MapViewGL.test.tsx b/client/src/components/Map/MapViewGL.test.tsx new file mode 100644 index 00000000..5a305a70 --- /dev/null +++ b/client/src/components/Map/MapViewGL.test.tsx @@ -0,0 +1,164 @@ +import React from 'react' +import { describe, it, expect, vi, afterEach, beforeEach } from 'vitest' +import { render } from '../../../tests/helpers/render' +import { act } from '@testing-library/react' +import { resetAllStores } from '../../../tests/helpers/store' +import { buildPlace } from '../../../tests/helpers/factories' +import { useSettingsStore } from '../../store/settingsStore' + +// Stable fake map so fitBounds call counts survive re-renders. +const glMap = vi.hoisted(() => ({ + on: vi.fn(), + off: vi.fn(), + once: vi.fn(), + loaded: vi.fn().mockReturnValue(true), + fitBounds: vi.fn(), + flyTo: vi.fn(), + jumpTo: vi.fn(), + getZoom: vi.fn().mockReturnValue(10), + addControl: vi.fn(), + removeControl: vi.fn(), + remove: vi.fn(), + addSource: vi.fn(), + getSource: vi.fn().mockReturnValue(null), + addLayer: vi.fn(), + setLayoutProperty: vi.fn(), + getStyle: vi.fn().mockReturnValue({ layers: [] }), + isStyleLoaded: vi.fn().mockReturnValue(true), + getCanvasContainer: vi.fn(() => document.createElement('div')), +})) + +vi.mock('mapbox-gl', () => ({ + default: { + accessToken: '', + Map: vi.fn(() => glMap), + Marker: vi.fn(() => ({ + setLngLat: vi.fn().mockReturnThis(), + addTo: vi.fn().mockReturnThis(), + remove: vi.fn(), + getElement: vi.fn(() => document.createElement('div')), + })), + LngLatBounds: vi.fn(() => ({ extend: vi.fn().mockReturnThis() })), + NavigationControl: vi.fn(), + }, +})) +vi.mock('mapbox-gl/dist/mapbox-gl.css', () => ({})) + +vi.mock('./mapboxSetup', () => ({ + isStandardFamily: vi.fn(() => false), + supportsCustom3d: vi.fn(() => false), + wantsTerrain: vi.fn(() => false), + addCustom3dBuildings: vi.fn(), + addTerrainAndSky: vi.fn(), +})) + +vi.mock('./locationMarkerMapbox', () => ({ + attachLocationMarker: vi.fn(() => ({ update: vi.fn() })), +})) + +vi.mock('./reservationsMapbox', () => ({ + ReservationMapboxOverlay: vi.fn().mockImplementation(() => ({ update: vi.fn() })), +})) + +vi.mock('../../hooks/useGeolocation', () => ({ + useGeolocation: vi.fn(() => ({ + position: null, + mode: 'off', + error: null, + cycleMode: vi.fn(), + setMode: vi.fn(), + })), +})) + +vi.mock('../../services/photoService', () => ({ + getCached: vi.fn(() => null), + isLoading: vi.fn(() => false), + fetchPhoto: vi.fn(), + onThumbReady: vi.fn(() => () => {}), + getAllThumbs: vi.fn(() => ({})), +})) + +import { MapViewGL } from './MapViewGL' + +function buildMapPlace(overrides: Record = {}) { + return { + ...buildPlace(), + category_name: null, + category_color: null, + category_icon: null, + ...overrides, + } as any +} + +beforeEach(() => { + useSettingsStore.setState({ + settings: { + ...useSettingsStore.getState().settings, + map_provider: 'mapbox-gl', + mapbox_access_token: 'pk.test_token', + mapbox_style: 'mapbox://styles/mapbox/streets-v12', + mapbox_3d_enabled: false, + }, + } as any) +}) + +afterEach(() => { + vi.clearAllMocks() + resetAllStores() +}) + +describe('MapViewGL', () => { + it('FE-COMP-MAPVIEWGL-001: opening place inspector does not refit bounds (issue #921)', async () => { + const places = [ + buildMapPlace({ id: 1, lat: 48.8584, lng: 2.2945 }), + buildMapPlace({ id: 2, lat: 48.86, lng: 2.337 }), + ] + + const { rerender } = render( + , + ) + await act(async () => {}) + const after_initial = glMap.fitBounds.mock.calls.length + + // Selecting a place flips hasInspector → paddingOpts memo changes. + // fitBounds must NOT fire again (this was the bug). + rerender( + , + ) + await act(async () => {}) + expect(glMap.fitBounds).toHaveBeenCalledTimes(after_initial) + }) + + it('FE-COMP-MAPVIEWGL-002: closing inspector does not refit bounds (issue #921)', async () => { + const places = [ + buildMapPlace({ id: 1, lat: 48.8584, lng: 2.2945 }), + ] + + const { rerender } = render( + , + ) + await act(async () => {}) + const after_initial = glMap.fitBounds.mock.calls.length + + // Closing inspector (X button) clears selectedPlaceId → hasInspector=false → new paddingOpts. + rerender( + , + ) + await act(async () => {}) + expect(glMap.fitBounds).toHaveBeenCalledTimes(after_initial) + }) + + it('FE-COMP-MAPVIEWGL-003: bumping fitKey triggers a new fitBounds call', async () => { + const places = [ + buildMapPlace({ id: 1, lat: 48.8584, lng: 2.2945 }), + ] + + const { rerender } = render() + await act(async () => {}) + const after_first = glMap.fitBounds.mock.calls.length + + rerender() + await act(async () => {}) + expect(glMap.fitBounds.mock.calls.length).toBeGreaterThan(after_first) + }) +}) diff --git a/client/src/components/Map/MapViewGL.tsx b/client/src/components/Map/MapViewGL.tsx index 557e2647..9be41e17 100644 --- a/client/src/components/Map/MapViewGL.tsx +++ b/client/src/components/Map/MapViewGL.tsx @@ -507,13 +507,10 @@ export function MapViewGL({ return { top, right: rightWidth + 40, bottom, left: leftWidth + 40 } }, [leftWidth, rightWidth, hasInspector, hasDayDetail]) - // Also fit when the places collection changes so the initial render - // zooms to the trip instead of the default center. - const placeBoundsKey = useMemo( - () => places.filter(p => p.lat && p.lng).map(p => `${p.id}:${p.lat}:${p.lng}`).join('|'), - [places] - ) + const prevFitKey = useRef(-1) useEffect(() => { + if (fitKey === prevFitKey.current) return + prevFitKey.current = fitKey const map = mapRef.current if (!map) return const target = dayPlaces.length > 0 ? dayPlaces : places @@ -533,7 +530,7 @@ export function MapViewGL({ } if (map.loaded()) run() else map.once('load', run) - }, [fitKey, placeBoundsKey, paddingOpts, mapbox3d]) // eslint-disable-line react-hooks/exhaustive-deps + }, [fitKey]) // eslint-disable-line react-hooks/exhaustive-deps // flyTo selected place useEffect(() => {