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 => {