diff --git a/client/src/components/Journey/JourneyMap.test.tsx b/client/src/components/Journey/JourneyMap.test.tsx index d73e8fd52..6e704a377 100644 --- a/client/src/components/Journey/JourneyMap.test.tsx +++ b/client/src/components/Journey/JourneyMap.test.tsx @@ -37,7 +37,7 @@ vi.mock('leaflet', () => { return { default: { map: vi.fn(() => mockMap), - tileLayer: vi.fn(() => ({ addTo: vi.fn() })), + tileLayer: vi.fn(() => ({ addTo: vi.fn(), setUrl: vi.fn() })), marker: vi.fn(() => mockMarker), polyline: vi.fn(() => { const line: any = { addTo: vi.fn(() => line), bindTooltip: vi.fn(() => line) }; return line }), divIcon: vi.fn(() => ({})), @@ -45,7 +45,7 @@ vi.mock('leaflet', () => { layerGroup: vi.fn(() => ({ addLayer: vi.fn(), addTo: vi.fn(), remove: vi.fn() })), }, map: vi.fn(() => mockMap), - tileLayer: vi.fn(() => ({ addTo: vi.fn() })), + tileLayer: vi.fn(() => ({ addTo: vi.fn(), setUrl: vi.fn() })), marker: vi.fn(() => mockMarker), polyline: vi.fn(() => { const line: any = { addTo: vi.fn(() => line), bindTooltip: vi.fn(() => line) }; return line }), divIcon: vi.fn(() => ({})), @@ -388,6 +388,22 @@ describe('JourneyMap', () => { expect(vi.mocked(L.tileLayer).mock.calls[0][0]).toBe('https://tiles.test/{z}/{x}/{y}.png'); }); + it('FE-COMP-JOURNEYMAP-041: a CARTO key arriving after mount retiles instead of rebuilding the map (#2097)', () => { + render(); + const tiles = vi.mocked(L.tileLayer).mock.results[0].value as { setUrl: ReturnType }; + const mapsBefore = vi.mocked(L.map).mock.calls.length; + + act(() => { + seedStore(useSettingsStore, { settings: buildSettings({ carto_api_key: 'k1' }) }); + }); + + expect(tiles.setUrl).toHaveBeenCalledWith(expect.stringContaining('key=k1')); + // The markers and tracks the map already carries are drawn by the effect that + // builds it, so a rebuild here would tear them down mid-flight. + expect(vi.mocked(L.map).mock.calls.length).toBe(mapsBefore); + expect(mockedMap().remove).not.toHaveBeenCalled(); + }); + it('FE-COMP-JOURNEYMAP-028: the activeMarkerId prop flies to that marker after the settle delay', () => { vi.useFakeTimers(); try { diff --git a/client/src/components/Journey/JourneyMap.tsx b/client/src/components/Journey/JourneyMap.tsx index 0147a0ed5..5539e98f1 100644 --- a/client/src/components/Journey/JourneyMap.tsx +++ b/client/src/components/Journey/JourneyMap.tsx @@ -154,6 +154,13 @@ function JourneyMap( const mapTileUrl = useSettingsStore(s => s.settings.map_tile_url) const storedCartoKey = useCartoApiKey() const cartoKey = cartoApiKey || storedCartoKey + const tileUrl = resolveTileUrl(mapTileUrl, dark ? CARTO_DARK : CARTO_VOYAGER, cartoKey) + // Read through a ref by the map effect, retiled in place by its own effect below: + // the CARTO key reaches the store after the first render, and rebuilding the map + // for that raced with the markers and layers already on it (#2097). + const tileUrlRef = useRef(tileUrl) + tileUrlRef.current = tileUrl + const tileLayerRef = useRef(null) const containerRef = useRef(null) const mapRef = useRef(null) const markersRef = useRef>(new Map()) @@ -232,7 +239,7 @@ function JourneyMap( }) mapRef.current = map - L.tileLayer(resolveTileUrl(mapTileUrl, dark ? CARTO_DARK : CARTO_VOYAGER, cartoKey), { + const tiles = L.tileLayer(tileUrlRef.current, { maxZoom: 18, attribution: '© OpenStreetMap', referrerPolicy: 'strict-origin-when-cross-origin', @@ -242,7 +249,9 @@ function JourneyMap( // updates and keep a larger ring of off-screen tiles ready. updateWhenIdle: false, keepBuffer: 4, - } as any).addTo(map) + } as any) + tiles.addTo(map) + tileLayerRef.current = tiles const items = buildMarkerItems(entries) itemsRef.current = items @@ -337,9 +346,16 @@ function JourneyMap( return () => { map.remove() mapRef.current = null + tileLayerRef.current = null markersRef.current.clear() } - }, [entries, stableTrail, stableTracks, dark, mapTileUrl, cartoKey, fullScreen, paddingBottom]) + }, [entries, stableTrail, stableTracks, dark, fullScreen, paddingBottom]) + + // Retile in place rather than through the effect above, which would drop every + // marker and track it just drew. + useEffect(() => { + tileLayerRef.current?.setUrl(tileUrl) + }, [tileUrl]) // Photo layer (#1614). Its own effect on purpose: photos arriving must not tear // down and rebuild the map the way the entry effect does. Redrawn on zoom and @@ -382,7 +398,7 @@ function JourneyMap( photoLayerRef.current?.remove() photoLayerRef.current = null } - }, [photos, entries, stableTrail, stableTracks, dark, mapTileUrl, cartoKey, fullScreen, paddingBottom]) + }, [photos, entries, stableTrail, stableTracks, dark, fullScreen, paddingBottom]) // react to activeMarkerId prop changes — runs after map is built useEffect(() => { diff --git a/client/src/pages/AtlasPage.test.tsx b/client/src/pages/AtlasPage.test.tsx index dcd6c5bf8..9c96e73be 100644 --- a/client/src/pages/AtlasPage.test.tsx +++ b/client/src/pages/AtlasPage.test.tsx @@ -71,7 +71,7 @@ vi.mock('leaflet', () => { const L = { map: vi.fn(() => mockMap), - tileLayer: vi.fn(() => ({ addTo: vi.fn().mockReturnThis() })), + tileLayer: vi.fn(() => ({ addTo: vi.fn().mockReturnThis(), setUrl: vi.fn() })), // Call onEachFeature and style callbacks for each feature so those paths are covered geoJSON: vi.fn((data: any, options: any) => { if (options?.onEachFeature && data?.features) { diff --git a/client/src/pages/atlas/useAtlas.test.tsx b/client/src/pages/atlas/useAtlas.test.tsx index 43bd31c69..bc1c7bf3e 100644 --- a/client/src/pages/atlas/useAtlas.test.tsx +++ b/client/src/pages/atlas/useAtlas.test.tsx @@ -105,7 +105,7 @@ vi.mock('leaflet', () => { const L = { map: vi.fn(() => { lf.mapsCreated += 1; return map; }), - tileLayer: vi.fn(() => ({ addTo: vi.fn() })), + tileLayer: vi.fn(() => ({ addTo: vi.fn(), setUrl: vi.fn() })), control: { zoom: vi.fn(() => ({ addTo: vi.fn() })) }, canvas: vi.fn(() => ({})), svg: vi.fn(() => ({})), @@ -708,6 +708,34 @@ describe('useAtlas', () => { expect(lf.mapsRemoved).toBeGreaterThan(0); }); + it('FE-HOOK-ATLAS-043: a CARTO key arriving after mount retiles instead of rebuilding the map (#2097)', async () => { + // Sliced from here: L.tileLayer's mock results carry over from earlier tests. + const madeBefore = vi.mocked(L.tileLayer).mock.results.length; + await mountAtlas({ geo: geoCountries }); + await waitFor(() => expect(lf.geoJson.length).toBeGreaterThan(0)); + + const tiles = vi.mocked(L.tileLayer).mock.results + .slice(madeBefore) + .map((r) => r.value as { setUrl: ReturnType }); + expect(tiles.length).toBeGreaterThan(0); + const layersBefore = lf.geoJson.length; + + act(() => { + seedStore(useSettingsStore, { settings: buildSettings({ dark_mode: false, carto_api_key: 'k1' }) }); + }); + + // The key reaches the tiles that are already on the map... + await waitFor(() => { + for (const t of tiles) expect(t.setUrl).toHaveBeenCalledWith(expect.stringContaining('key=k1')); + }); + // ...and nothing else is torn down: rebuilding the map dropped the country layer + // (only redrawn when its own data changes) and left Leaflet redrawing a canvas + // renderer whose map had gone. + expect(lf.mapsCreated).toBe(1); + expect(lf.mapsRemoved).toBe(0); + expect(lf.geoJson.length).toBe(layersBefore); + }); + it('FE-HOOK-ATLAS-027: a plugin layer naming no country in the map draws nothing', async () => { await mountAtlas({ geo: geoCountries, diff --git a/client/src/pages/atlas/useAtlas.ts b/client/src/pages/atlas/useAtlas.ts index bf870c533..34d1521ad 100644 --- a/client/src/pages/atlas/useAtlas.ts +++ b/client/src/pages/atlas/useAtlas.ts @@ -84,6 +84,12 @@ export function useAtlas() { // Label-free tiles on purpose (the country fills carry the names here), so the // user's own template is deliberately not read, only their CARTO key. const tileUrl = useTileUrl(dark ? CARTO_DARK_NOLABELS : CARTO_LIGHT_NOLABELS, true) + // The template is read through a ref inside the map effect so a template change — + // the CARTO key arriving after the first render is the usual one — retiles the + // layers below instead of tearing the whole map down and building it again (#2097). + const tileUrlRef = useRef(tileUrl) + tileUrlRef.current = tileUrl + const tileLayersRef = useRef([]) const mapRef = useRef(null) const mapInstance = useRef(null) const geoLayerRef = useRef(null) @@ -367,7 +373,7 @@ export function useAtlas() { L.control.zoom({ position: 'bottomright' }).addTo(map) - L.tileLayer(tileUrl, { + const baseTiles = L.tileLayer(tileUrlRef.current, { maxZoom: 10, keepBuffer: 25, updateWhenZooming: true, @@ -376,17 +382,20 @@ export function useAtlas() { zoomOffset: 0, crossOrigin: true, referrerPolicy: 'strict-origin-when-cross-origin', - } as any).addTo(map) + } as any) + baseTiles.addTo(map) // Preload adjacent zoom level tiles - L.tileLayer(tileUrl, { + const preloadTiles = L.tileLayer(tileUrlRef.current, { maxZoom: 10, keepBuffer: 10, opacity: 0, tileSize: 256, crossOrigin: true, referrerPolicy: 'strict-origin-when-cross-origin', - }).addTo(map) + }) + preloadTiles.addTo(map) + tileLayersRef.current = [baseTiles, preloadTiles] // Custom pane for region layer — above overlay (z-index 400) map.createPane('regionPane') @@ -443,8 +452,17 @@ export function useAtlas() { // appending a second container to the pane. regionLayerRef.current = null renderedRegionSigRef.current = '' + tileLayersRef.current = [] } - }, [dark, loading, tileUrl]) + }, [dark, loading]) + + // Retile in place. A rebuild would drop every layer the effects below hold a ref + // to — the country layer is only re-rendered when its own data changes, so the map + // would come back bare — and Leaflet keeps redrawing the torn-down canvas renderer + // for a frame after the map goes (#2097). + useEffect(() => { + for (const layer of tileLayersRef.current) layer.setUrl(tileUrl) + }, [tileUrl]) // Render GeoJSON countries useEffect(() => {