From d156404b840bd930593a9b77bda6625cb8cc44c7 Mon Sep 17 00:00:00 2001 From: Maurice Date: Mon, 17 Aug 2026 14:21:11 +0200 Subject: [PATCH] perf(atlas): bound the region layer to what is actually on screen Interaction degraded the longer the map was used past zoom 5 and a reload fixed it, which the report traced accurately: admin-1 geometry was fetched for every country whose bounds crossed the viewport, the cache was never evicted, and the whole accumulated set was torn down and rebuilt as one layer on every addition. Panning past Canada or Russia added their geometry to every later rebuild for the rest of the session. Only the countries in view are rendered now, a signature over that set skips the rebuild entirely while panning inside the same countries, and the cache holds a dozen countries, never evicting one that is on screen or still loading. One cause was not in the report and is probably the bigger one: a fresh renderer was constructed on every rebuild. Leaflet registers a renderer as its own map layer and removing the GeoJSON layer does not remove it, so the pane collected an orphaned element per rebuild that kept updating on every pan for the rest of the session, and at the country layer each orphan is a canvas of four times the viewport area. Both renderers are created once per map now. The region layer stays on SVG on purpose. Leaflet's stylesheet only makes SVG paths mouse-transparent, so a canvas container would swallow the clicks meant for the country layer below zoom 5. Closes #1950 --- client/src/pages/atlas/atlasModel.test.ts | 28 +++ client/src/pages/atlas/atlasModel.ts | 27 +++ client/src/pages/atlas/useAtlas.test.tsx | 231 +++++++++++++++++++++- client/src/pages/atlas/useAtlas.ts | 153 ++++++++++++-- 4 files changed, 412 insertions(+), 27 deletions(-) diff --git a/client/src/pages/atlas/atlasModel.test.ts b/client/src/pages/atlas/atlasModel.test.ts index e7c1c927a..4e02c96af 100644 --- a/client/src/pages/atlas/atlasModel.test.ts +++ b/client/src/pages/atlas/atlasModel.test.ts @@ -6,10 +6,12 @@ import { isBucketDuplicateError, isCountryVisible, normalizeRegionName, + regionCacheEvictions, withCountryMarkedVisited, wishlistA3Codes, countryColor, COUNTRY_COLORS, + REGION_CACHE_MAX, type AtlasData, type BucketItem, } from './atlasModel'; @@ -260,6 +262,32 @@ describe('findBucketDuplicate (#1898)', () => { }); }); +// The region cache used to grow for the whole session (#1950). The eviction order is +// pure arithmetic, so it is decided here rather than inside the Leaflet effect. +describe('regionCacheEvictions (#1950)', () => { + it('drops nothing while the cache is within its cap', () => { + expect(regionCacheEvictions(['FR', 'IT'], new Set(), REGION_CACHE_MAX)).toEqual([]); + expect(regionCacheEvictions(['FR', 'IT'], new Set(), 2)).toEqual([]); + }); + + it('drops the least recently viewed countries first, and only as many as needed', () => { + expect(regionCacheEvictions(['FR', 'IT', 'ES', 'PT'], new Set(), 2)).toEqual(['FR', 'IT']); + expect(regionCacheEvictions(['FR', 'IT', 'ES', 'PT'], new Set(), 3)).toEqual(['FR']); + }); + + it('never drops a country that has to stay', () => { + // FR is the oldest entry but still on screen, so the next-oldest ones go instead. + expect(regionCacheEvictions(['FR', 'IT', 'ES', 'PT'], new Set(['FR']), 2)).toEqual(['IT', 'ES']); + }); + + it('stays over the cap rather than dropping what is in view', () => { + // Three countries on screen with room for two: evicting one would only make the + // next pan fetch it straight back. + expect(regionCacheEvictions(['FR', 'IT', 'ES'], new Set(['FR', 'IT', 'ES']), 2)).toEqual([]); + expect(regionCacheEvictions(['FR', 'IT', 'ES', 'PT'], new Set(['IT', 'ES', 'PT']), 2)).toEqual(['FR']); + }); +}); + describe('isBucketDuplicateError (#1898)', () => { it('recognises the 409 and nothing else', () => { expect(isBucketDuplicateError({ response: { status: 409 } })).toBe(true); diff --git a/client/src/pages/atlas/atlasModel.ts b/client/src/pages/atlas/atlasModel.ts index 7bd5a1d5e..a806b29c6 100644 --- a/client/src/pages/atlas/atlasModel.ts +++ b/client/src/pages/atlas/atlasModel.ts @@ -184,6 +184,33 @@ export function countryColor(a3: string): string { return COUNTRY_COLORS[hash % COUNTRY_COLORS.length] } +// How many countries' worth of admin-1 geometry the region layer keeps around. +// Panning back to a country you just left has to stay free, but a session that +// wanders across a continent used to hold on to every country it ever touched, +// a few hundred polygons each (#1950). A dozen covers the countries around any +// one view, and /regions/geo is served with a day of cache headroom, so the +// countries that do fall out come back without hitting the database. +export const REGION_CACHE_MAX = 12 + +/** + * Which cached countries to drop, given the cache order (least recently viewed + * first) and the codes that have to stay. Never returns a code from `keep`: when + * the countries in view alone exceed the cap it returns a shorter list instead of + * dropping geometry that is on screen, which is what stops panning across a big + * country turning into fetch / evict / refetch. + */ +export function regionCacheEvictions(order: string[], keep: Set, max: number): string[] { + const drop: string[] = [] + let remaining = order.length + for (const code of order) { + if (remaining <= max) break + if (keep.has(code)) continue + drop.push(code) + remaining -= 1 + } + return drop +} + // Convert country code to flag emoji export function countryCodeToFlag(code: string): string { if (!code || code.length !== 2) return '' diff --git a/client/src/pages/atlas/useAtlas.test.tsx b/client/src/pages/atlas/useAtlas.test.tsx index a575a0dd1..43bd31c69 100644 --- a/client/src/pages/atlas/useAtlas.test.tsx +++ b/client/src/pages/atlas/useAtlas.test.tsx @@ -6,6 +6,7 @@ import { server } from '../../../tests/helpers/msw/server'; import { resetAllStores, seedStore } from '../../../tests/helpers/store'; import { buildSettings } from '../../../tests/helpers/factories'; import { useSettingsStore } from '../../store/settingsStore'; +import L from 'leaflet'; import { A2_TO_A3 } from './atlasModel'; import { useAtlas } from './useAtlas'; @@ -16,6 +17,8 @@ interface MockGeoJson { options: Record; entries: { feature: Record; layer: MockLayer }[]; styles: Record[]; + /** Whether this layer is on the map right now: built is not the same as shown. */ + attached: boolean; } interface MockLayer { @@ -32,6 +35,9 @@ const lf = vi.hoisted(() => ({ panes: {} as Record }>, zoom: 3, intersects: true, + // The country codes the viewport is over, when a test needs to be selective about it. + // null falls back to the blanket `intersects` answer. + intersectsOnly: null as string[] | null, hasLayer: true, boundsThrows: false, mapsCreated: 0, @@ -44,6 +50,7 @@ const lf = vi.hoisted(() => ({ this.panes = { overlayPane: { style: {} } }; this.zoom = 3; this.intersects = true; + this.intersectsOnly = null; this.hasLayer = true; this.boundsThrows = false; this.mapsCreated = 0; @@ -53,7 +60,10 @@ const lf = vi.hoisted(() => ({ })); vi.mock('leaflet', () => { - const makeLayer = () => { + // The bounds a country layer reports carry its own code, so the map's bounds can + // answer `intersects` per country instead of all-or-nothing. + const makeLayer = (feature?: { properties?: Record }) => { + const code = typeof feature?.properties?.ISO_A2 === 'string' ? feature.properties.ISO_A2 : null; const handlers: Record void> = {}; const layer = { handlers, @@ -62,7 +72,7 @@ vi.mock('leaflet', () => { setStyle: vi.fn(), getBounds: vi.fn(() => { if (lf.boundsThrows) throw new Error('no bounds'); - return { isValid: () => true }; + return { isValid: () => true, code }; }), }; return layer; @@ -75,10 +85,16 @@ vi.mock('leaflet', () => { remove: vi.fn(() => { lf.mapsRemoved += 1; }), fitBounds: vi.fn(), addLayer: vi.fn(), - removeLayer: vi.fn(), + removeLayer: vi.fn((layer?: { record?: { attached: boolean } }) => { + if (layer?.record) layer.record.attached = false; + }), getZoom: vi.fn(() => lf.zoom), getCenter: vi.fn(() => ({ lat: 25, lng: 0 })), - getBounds: vi.fn(() => ({ intersects: () => lf.intersects })), + getBounds: vi.fn(() => ({ + intersects: (other?: { code?: string | null }) => ( + lf.intersectsOnly ? !!other?.code && lf.intersectsOnly.includes(other.code) : lf.intersects + ), + })), hasLayer: vi.fn(() => lf.hasLayer), createPane: vi.fn((name: string) => { lf.panes[name] = { style: {} }; }), getPane: vi.fn((name: string) => lf.panes[name]), @@ -100,22 +116,23 @@ vi.mock('leaflet', () => { }), layerGroup: vi.fn(() => ({ addTo: vi.fn(() => ({})) })), geoJSON: vi.fn((data: { features?: Record[] }, options: Record) => { - const record = { data, options, entries: [] as unknown[], styles: [] as unknown[] }; + const record = { data, options, entries: [] as unknown[], styles: [] as unknown[], attached: false }; const style = options?.style as ((f: unknown) => unknown) | undefined; const onEachFeature = options?.onEachFeature as ((f: unknown, l: unknown) => void) | undefined; for (const feature of data?.features ?? []) { if (style) record.styles.push(style(feature)); if (onEachFeature) { - const layer = makeLayer(); + const layer = makeLayer(feature as { properties?: Record }); onEachFeature(feature, layer); record.entries.push({ feature, layer }); } } lf.geoJson.push(record); const result = { - addTo: vi.fn(() => result), - removeFrom: vi.fn(), - remove: vi.fn(), + record, + addTo: vi.fn(() => { record.attached = true; return result; }), + removeFrom: vi.fn(() => { record.attached = false; }), + remove: vi.fn(() => { record.attached = false; }), clearLayers: vi.fn(), resetStyle: vi.fn(), }; @@ -196,6 +213,20 @@ function countryLayers(): MockGeoJson[] { return (lf.geoJson as MockGeoJson[]).filter((g) => g.options.pane === undefined); } +/** Every region layer ever built, oldest first: one record per rebuild. */ +function regionLayers(): MockGeoJson[] { + return (lf.geoJson as MockGeoJson[]).filter((g) => g.options.pane === 'regionPane'); +} + +function newestRegionLayer(): MockGeoJson | undefined { + const layers = regionLayers(); + return layers[layers.length - 1]; +} + +function regionCodesOf(layer: MockGeoJson | undefined): string[] { + return (layer?.data.features ?? []).map((f) => (f.properties as Record).iso_3166_2); +} + async function mountAtlas(over: Partial> = {}, props: { withPanel?: boolean } = {}) { useAtlasHandlers(over); const view = render(); @@ -808,6 +839,188 @@ describe('useAtlas', () => { expect((lf.geoJson as MockGeoJson[]).some((g) => g.options.pane === 'regionPane')).toBe(false); }); + + // ── The region layer used to keep every country it had ever seen and rebuild all of + // it, with a fresh renderer each time (#1950) ──────────────────────────────────── + const twoRegions = { + type: 'FeatureCollection', + features: [ + feature({ iso_a2: 'fr', iso_3166_2: 'FR-BRE', name: 'Bretagne', name_en: 'Brittany', admin: 'France' }), + feature({ iso_a2: 'it', iso_3166_2: 'IT-62', name: 'Lazio', name_en: 'Latium', admin: 'Italy' }), + ], + }; + + /** Zoom in far enough for regions, over the given countries. */ + const zoomTo = async (...codes: string[]) => { + lf.zoom = 6; + lf.intersectsOnly = codes; + await act(async () => { lf.mapHandlers.zoomend?.forEach((cb) => cb()); }); + }; + + const panTo = async (...codes: string[]) => { + lf.intersectsOnly = codes; + await act(async () => { lf.mapHandlers.moveend?.forEach((cb) => cb()); }); + }; + + it('FE-HOOK-ATLAS-038: a map builds one canvas and one svg renderer for its whole life (#1950)', async () => { + // Leaflet keeps a renderer as a layer of its own and leaves its container in the + // pane when the GeoJSON layer goes, so one per redraw piles up orphaned canvases + // that keep updating on every pan. + const canvasBefore = vi.mocked(L.canvas).mock.calls.length; + const svgBefore = vi.mocked(L.svg).mock.calls.length; + await mountAtlas({ geo: geoCountries, regionGeo: twoRegions }); + await waitFor(() => expect(lf.mapHandlers.zoomend?.length).toBeGreaterThan(0)); + + await zoomTo('FR'); + await waitFor(() => expect(regionLayers().length).toBe(1)); + // A second country lands, then the planned toggle repaints both layers. + await panTo('FR', 'IT'); + await waitFor(() => expect(regionLayers().length).toBe(2)); + act(() => atlas.togglePlanned()); + await waitFor(() => expect(regionLayers().length).toBe(3)); + + expect(lf.mapsCreated).toBe(1); + expect(vi.mocked(L.canvas).mock.calls.length).toBe(canvasBefore + 1); + expect(vi.mocked(L.svg).mock.calls.length).toBe(svgBefore + 1); + }); + + it('FE-HOOK-ATLAS-039: only the countries in view contribute features (#1950)', async () => { + await mountAtlas({ geo: geoCountries, regionGeo: twoRegions }); + await waitFor(() => expect(lf.mapHandlers.zoomend?.length).toBeGreaterThan(0)); + + await zoomTo('FR', 'IT'); + await waitFor(() => expect(regionCodesOf(newestRegionLayer())).toEqual(['FR-BRE', 'IT-62'])); + + // Panning off Italy leaves it in the cache but takes it out of the layer. + await panTo('FR'); + expect(regionCodesOf(newestRegionLayer())).toEqual(['FR-BRE']); + + // Coming back draws it again straight from the cache. + await panTo('FR', 'IT'); + expect(regionCodesOf(newestRegionLayer())).toEqual(['FR-BRE', 'IT-62']); + }); + + it('FE-HOOK-ATLAS-040: panning inside the same countries rebuilds nothing (#1950)', async () => { + await mountAtlas({ geo: geoCountries, regionGeo: twoRegions }); + await waitFor(() => expect(lf.mapHandlers.zoomend?.length).toBeGreaterThan(0)); + + await zoomTo('FR', 'IT'); + await waitFor(() => expect(regionLayers().length).toBe(1)); + + await panTo('FR', 'IT'); + expect(regionLayers().length).toBe(1); + + // Losing France from the view is a change, so that one does redraw. + await panTo('IT'); + expect(regionLayers().length).toBe(2); + expect(regionCodesOf(newestRegionLayer())).toEqual(['IT-62']); + }); + + it('FE-HOOK-ATLAS-041: the cache stops at a dozen countries and reloads what fell out (#1950)', async () => { + // Fourteen countries seen one at a time: the oldest two are gone by the end, so + // coming back to the first one has to ask the server again. + const seen = ['FR', 'IT', 'ES', 'PT', 'NL', 'BE', 'AT', 'CH', 'PL', 'CZ', 'SE', 'NO', 'DK', 'FI']; + const geo = { + type: 'FeatureCollection', + // Deliberately the other way round from the visiting order: with both the same, + // dropping the country from the front of the dataset and dropping the one seen + // longest ago pick the same victim, and nothing below tells them apart. + features: [...seen].reverse().map((a2) => feature({ ISO_A2: a2, ADM0_A3: A2_TO_A3[a2], ISO_A3: A2_TO_A3[a2], NAME: a2, ADMIN: a2 })), + }; + const regionGeo = { + type: 'FeatureCollection', + features: seen.map((a2) => feature({ iso_a2: a2.toLowerCase(), iso_3166_2: `${a2}-01`, name: `${a2} region`, admin: a2 })), + }; + await mountAtlas({ geo, regionGeo }); + await waitFor(() => expect(atlas.atlas_country_options.length).toBe(seen.length)); + + const requested: string[] = []; + server.use(http.get('/api/addons/atlas/regions/geo', ({ request }) => { + requested.push(new URL(request.url).searchParams.get('countries') ?? ''); + return HttpResponse.json(regionGeo); + })); + + lf.zoom = 6; + for (let i = 0; i < seen.length; i++) { + await panTo(seen[i]); + await waitFor(() => expect(requested).toHaveLength(i + 1)); + } + expect(requested).toEqual(seen); + + await panTo('FR'); + await waitFor(() => expect(requested).toHaveLength(seen.length + 1)); + expect(requested[requested.length - 1]).toBe('FR'); + + // The country the view sat on longest is still cached, so it costs nothing. + await panTo('FI'); + expect(regionCodesOf(newestRegionLayer())).toEqual(['FI-01']); + expect(requested).toHaveLength(seen.length + 1); + + // And the two the view left just before that are still there as well. They sit at + // the front of the dataset, which is the whole difference between dropping by + // position and dropping by how long ago the country was last on screen. + await panTo('DK'); + await panTo('NO'); + expect(requested).toHaveLength(seen.length + 1); + }); + + it('FE-HOOK-ATLAS-042: a theme change and the planned toggle still restyle the regions (#1950)', async () => { + await mountAtlas({ geo: geoCountries, regionGeo: twoRegions }); + await waitFor(() => expect(lf.mapHandlers.zoomend?.length).toBeGreaterThan(0)); + + await zoomTo('FR'); + await waitFor(() => expect(regionLayers().length).toBe(1)); + + const unvisitedFill = (layer: MockGeoJson) => + (layer.options.style as (f: unknown) => { fillColor: string })(twoRegions.features[0]).fillColor; + expect(unvisitedFill(newestRegionLayer()!)).toBe('#000000'); + + act(() => atlas.togglePlanned()); + await waitFor(() => expect(regionLayers().length).toBe(2)); + + // Dark mode rebuilds the map itself, so the layer has to come back dark-styled + // even though the countries in view never changed. + act(() => { seedStore(useSettingsStore, { settings: buildSettings({ dark_mode: true }) }); }); + await waitFor(() => expect(regionLayers().length).toBe(3)); + expect(unvisitedFill(newestRegionLayer()!)).toBe('#ffffff'); + }); + + it('FE-HOOK-ATLAS-043: flying to a country from the search draws its regions (#1950)', async () => { + await mountAtlas({ geo: geoCountries, regionGeo: twoRegions }); + await waitFor(() => expect(atlas.atlas_country_options.length).toBe(3)); + + // fitBounds puts the view on Italy alone; the zoomend that follows must not cull it. + await act(async () => { atlas.select_country_from_search('IT'); }); + await zoomTo('IT'); + + await waitFor(() => expect(regionCodesOf(newestRegionLayer())).toEqual(['IT-62'])); + }); + + it('FE-HOOK-ATLAS-044: zooming out to 5 keeps the drawn regions on the map (#1950)', async () => { + await mountAtlas({ geo: geoCountries, regionGeo: twoRegions }); + await waitFor(() => expect(lf.mapHandlers.zoomend?.length).toBeGreaterThan(0)); + + // Both countries land in the cache, then the view narrows to France alone. + await zoomTo('FR', 'IT'); + await waitFor(() => expect(regionLayers().length).toBe(1)); + await panTo('FR'); + await waitFor(() => expect(regionCodesOf(newestRegionLayer())).toEqual(['FR-BRE'])); + const shown = newestRegionLayer()!; + expect(shown.attached).toBe(true); + + // One step out. Regions still show at zoom 5 and the country layer stops taking + // clicks there, so the region layer is the only way to mark anything. The wider view + // pulls Italy back in, which is exactly the change that asks for a rebuild, and the + // map only accepts a rebuilt layer from zoom 6, so it must not happen here. + lf.zoom = 5; + lf.intersectsOnly = ['FR', 'IT']; + await act(async () => { lf.mapHandlers.zoomend?.forEach((cb) => cb()); }); + await act(async () => { lf.mapHandlers.moveend?.forEach((cb) => cb()); }); + + expect(lf.panes.overlayPane.style.pointerEvents).toBe('none'); + expect(shown.attached).toBe(true); + expect(newestRegionLayer()).toBe(shown); + }); }); describe('planned countries (#1048)', () => { diff --git a/client/src/pages/atlas/useAtlas.ts b/client/src/pages/atlas/useAtlas.ts index 140f386a3..d67ac283b 100644 --- a/client/src/pages/atlas/useAtlas.ts +++ b/client/src/pages/atlas/useAtlas.ts @@ -5,7 +5,7 @@ import { useSettingsStore } from '../../store/settingsStore' import apiClient, { mapsApi, pluginsApi, type PluginAtlasLayer } from '../../api/client' import L from 'leaflet' import type { GeoJsonFeatureCollection } from '../../types' -import { A2_TO_A3, countryStatus, findBucketDuplicate, isBucketDuplicateError, isCountryVisible, normalizeRegionName, withCountryMarkedVisited, wishlistA3Codes, countryColor, type AtlasData, type AtlasPlaceHit, type CountryDetail, type BucketItem } from './atlasModel' +import { A2_TO_A3, countryStatus, findBucketDuplicate, isBucketDuplicateError, isCountryVisible, normalizeRegionName, regionCacheEvictions, withCountryMarkedVisited, wishlistA3Codes, countryColor, REGION_CACHE_MAX, type AtlasData, type AtlasPlaceHit, type CountryDetail, type BucketItem } from './atlasModel' import { continentForCountry, escapeHtml, type VisitStatus } from '@trek/shared' import { useToast } from '../../components/shared/Toast' import { getApiErrorMessage } from '../../types' @@ -82,6 +82,12 @@ export function useAtlas() { const mapRef = useRef(null) const mapInstance = useRef(null) const geoLayerRef = useRef(null) + // One renderer per map, not one per redraw: Leaflet registers a renderer as a layer + // of its own and leaves its container in the pane when the GeoJSON layer is removed, + // so building a fresh one on every rebuild left an orphaned canvas/svg behind that + // kept redrawing itself on every pan for the rest of the session (#1950). + const countryRendererRef = useRef(null) + const regionRendererRef = useRef(null) const glareRef = useRef(null) const borderGlareRef = useRef(null) const panelRef = useRef(null) @@ -117,6 +123,16 @@ export function useAtlas() { const pluginLayerRef = useRef(null) const regionLayerRef = useRef(null) const regionGeoCache = useRef>({}) + // Cached countries, least recently in view first, capped at REGION_CACHE_MAX. + const regionCacheOrder = useRef([]) + // Countries whose /regions/geo answer is still on the way. They must not be asked + // for twice while a pan fires moveend over and over, and must not be evicted before + // their own response lands. + const pendingRegionCodes = useRef>(new Set()) + // Which countries the drawn layer was built from, so panning inside the same set of + // countries costs nothing. + const renderedRegionSigRef = useRef('') + const rebuildRegionLayerRef = useRef<(force?: boolean) => void>(() => {}) const [showRegions, setShowRegions] = useState(false) // Countries you only plan to visit stay off the map until asked for — the atlas is a // record of where you have been, not of where you booked a flight to (#1048). @@ -242,6 +258,52 @@ export function useAtlas() { .catch(() => setPluginLayers([])) }, []) + /** The view the map currently shows, or null while there is no map to ask. */ + const viewportBounds = (): L.LatLngBounds | null => { + try { + return mapInstance.current?.getBounds() ?? null + } catch { + return null + } + } + + /** Does this country's outline touch the given view? Fail open: a country we hold no + * outline for, or one whose bounds throw, counts as in view, because dropping it + * would blank regions the user can see. */ + const countryInView = (code: string, bounds: L.LatLngBounds | null): boolean => { + if (!bounds) return true + const layer = country_layer_by_a2_ref.current[code] + if (!layer) return true + try { + return bounds.intersects(layer.getBounds()) + } catch { + return true + } + } + + const touchRegionCode = (code: string): void => { + const order = regionCacheOrder.current + const at = order.indexOf(code) + if (at !== -1) order.splice(at, 1) + order.push(code) + } + + /** Drop the countries nobody is looking at once the cache outgrows its cap. A country + * still in view, or one still waiting for its response, is never dropped, or panning + * across a wide country would fetch it, evict it and fetch it again. */ + const evictRegionCache = (): void => { + const bounds = viewportBounds() + const keep = new Set(pendingRegionCodes.current) + for (const code of regionCacheOrder.current) { + if (!keep.has(code) && countryInView(code, bounds)) keep.add(code) + } + for (const code of regionCacheEvictions(regionCacheOrder.current, keep, REGION_CACHE_MAX)) { + delete regionGeoCache.current[code] + const at = regionCacheOrder.current.indexOf(code) + if (at !== -1) regionCacheOrder.current.splice(at, 1) + } + } + // Load admin-1 GeoJSON for countries visible in the current viewport const loadRegionsForViewportRef = useRef<() => void>(() => {}) const loadRegionsForViewport = (): void => { @@ -249,12 +311,21 @@ export function useAtlas() { const bounds = mapInstance.current.getBounds() const toLoad: string[] = [] for (const [code, layer] of Object.entries(country_layer_by_a2_ref.current)) { - if (regionGeoCache.current[code]) continue + if (regionGeoCache.current[code]) { + // Recency means recency of being on screen. Touching every cached country wrote + // the order back into GeoJSON feature order on every moveend, so the eviction + // picked its victim by position in the dataset instead of by what the view had + // just left, and panning back to that country paid for a refetch. + if (countryInView(code, bounds)) touchRegionCode(code) + continue + } + if (pendingRegionCodes.current.has(code)) continue try { if (bounds.intersects((layer as any).getBounds())) toLoad.push(code) } catch {} } if (!toLoad.length) return + for (const code of toLoad) pendingRegionCodes.current.add(code) apiClient.get(`/addons/atlas/regions/geo?countries=${toLoad.join(',')}`) .then(geoRes => { const geo = geoRes.data @@ -262,11 +333,12 @@ export function useAtlas() { let added = false for (const c of toLoad) { const features = geo.features.filter((f: any) => f.properties?.iso_a2?.toUpperCase() === c) - if (features.length > 0) { regionGeoCache.current[c] = { type: 'FeatureCollection', features }; added = true } + if (features.length > 0) { regionGeoCache.current[c] = { type: 'FeatureCollection', features }; touchRegionCode(c); added = true } } - if (added) setRegionGeoLoaded(v => v + 1) + if (added) { evictRegionCache(); setRegionGeoLoaded(v => v + 1) } }) .catch(() => {}) + .finally(() => { for (const code of toLoad) pendingRegionCodes.current.delete(code) }) } loadRegionsForViewportRef.current = loadRegionsForViewport @@ -320,6 +392,10 @@ export function useAtlas() { map.getPane('regionPane')!.style.zIndex = '401' mapInstance.current = map + // Both renderers live as long as the map does (see the note on the refs); map.remove() + // disposes them along with every other layer it holds. + countryRendererRef.current = L.canvas({ padding: 0.5, tolerance: 5 }) + regionRendererRef.current = L.svg({ pane: 'regionPane' }) // Zoom-based region switching map.on('zoomend', () => { @@ -337,6 +413,7 @@ export function useAtlas() { regionLayerRef.current.addTo(map) } loadRegionsForViewportRef.current() + rebuildRegionLayerRef.current() } else { // Physically remove region layer so its SVG paths can't intercept events if (regionTooltipRef.current) regionTooltipRef.current.style.display = 'none' @@ -348,15 +425,29 @@ export function useAtlas() { }) map.on('moveend', () => { - if (map.getZoom() >= 6) loadRegionsForViewportRef.current() + if (map.getZoom() < 6) return + loadRegionsForViewportRef.current() + // A pan that stays over the same countries changes nothing, and the rebuild + // notices that for itself, cheaper than working it out here. + rebuildRegionLayerRef.current() }) - return () => { map.remove(); mapInstance.current = null } + return () => { + map.remove() + mapInstance.current = null + countryRendererRef.current = null + regionRendererRef.current = null + // The layer belongs to the map that just went away. Without this the next map's + // zoomend would re-attach it, and Leaflet would revive the old renderer by + // appending a second container to the pane. + regionLayerRef.current = null + renderedRegionSigRef.current = '' + } }, [dark, loading]) // Render GeoJSON countries useEffect(() => { - if (!mapInstance.current || !geoData || !data) return + if (!mapInstance.current || !geoData || !data || !countryRendererRef.current) return const visitedA3 = new Set(visibleCountries.map(c => A2_TO_A3[c.code]).filter(Boolean)) const plannedA3 = new Set(visibleCountries.filter(c => countryStatus(c) !== 'visited').map(c => A2_TO_A3[c.code]).filter(Boolean)) @@ -378,7 +469,7 @@ export function useAtlas() { const colorForCode = countryColor const wishlistPatternCache = new Map() - const canvasRenderer = L.canvas({ padding: 0.5, tolerance: 5 }) + const canvasRenderer = countryRendererRef.current geoLayerRef.current = L.geoJSON(geoData, { renderer: canvasRenderer, @@ -541,17 +632,35 @@ export function useAtlas() { // layers fetched before that would otherwise never get drawn. }, [geoData, pluginLayers, dark, loading]) - // Render sub-national region layer (zoom >= 5) - useEffect(() => { - if (!mapInstance.current) return + // Render sub-national region layer (zoom >= 5). `force` is for the changes that alter + // how the regions look (theme, visits, the planned toggle); the map's own zoom/pan + // handlers pass nothing and get a rebuild only when the countries in view changed. + const rebuildRegionLayer = (force = false): void => { + if (!mapInstance.current || !regionRendererRef.current) return + // Below zoom 6 a rebuild can only do harm: it drops the layer that is on the map and + // the add at the end of this function starts at 6, so the regions would go and stay + // gone. At zoom 5 they are the only clickable layer left (the country layer is dimmed + // and pointer-events off), so the layer built further in has to survive the zoom out. + if (!force && mapInstance.current.getZoom() < 6) return + const regionRenderer = regionRendererRef.current + + // Draw only the countries that are actually on screen. The cache deliberately + // outlives the viewport so panning back is free, but merging all of it into one + // layer meant a continent's worth of admin-1 polygons stayed in the DOM, and every + // newly loaded country tore the whole thing down and built it again (#1950). + const bounds = viewportBounds() + const inViewCodes = Object.keys(regionGeoCache.current).filter(code => countryInView(code, bounds)).sort() + const sig = inViewCodes.join('|') + if (!force && sig === renderedRegionSigRef.current) return // Remove existing region layer if (regionLayerRef.current) { mapInstance.current.removeLayer(regionLayerRef.current) regionLayerRef.current = null } + renderedRegionSigRef.current = sig - if (Object.keys(regionGeoCache.current).length === 0) return + if (inViewCodes.length === 0) return // Build set of visited region codes and per-country name sets. Regions follow their // country's status, so zooming into a planned country can't reveal "visited" regions. @@ -598,10 +707,11 @@ export function useAtlas() { const isVisitedFeature = (f: any) => matchesRegions(f, visitedRegionCodes, visitedRegionNamesByCountry) const isPlannedFeature = (f: any) => matchesRegions(f, plannedRegionCodes, plannedRegionNamesByCountry) - // Include ALL region features — visited ones get colored fill, unvisited get outline only + // Include every region feature of the countries in view: visited ones get colored + // fill, unvisited get outline only (clicking one is how a region gets marked) const allFeatures: any[] = [] - for (const geo of Object.values(regionGeoCache.current)) { - for (const f of geo.features) { + for (const code of inViewCodes) { + for (const f of regionGeoCache.current[code].features) { allFeatures.push(f) } } @@ -614,10 +724,8 @@ export function useAtlas() { const mergedGeo = { type: 'FeatureCollection', features: allFeatures } - const svgRenderer = L.svg({ pane: 'regionPane' }) - regionLayerRef.current = L.geoJSON(mergedGeo as any, { - renderer: svgRenderer, + renderer: regionRenderer, interactive: true, pane: 'regionPane', style: (feature) => { @@ -706,8 +814,17 @@ export function useAtlas() { if (mapInstance.current.getZoom() >= 6) { regionLayerRef.current.addTo(mapInstance.current) } + } + // Reassigned every render so the map handlers always call a closure that sees the + // current visits, theme and toggle state, same pattern as loadRegionsForViewport. + rebuildRegionLayerRef.current = rebuildRegionLayer + + useEffect(() => { + // Anything in the deps changes how the regions look rather than which are on screen, + // so it has to redraw even when the countries in view are the same ones. // visitedCountries belongs here: the region colours are derived from it, and without // the dep this effect kept painting regions from a stale country list. + rebuildRegionLayerRef.current(true) }, [regionGeoLoaded, visitedRegions, dark, t, visitedCountries, showPlanned]) const handleMarkCountry = (code: string, name: string): void => {