From dce348b947fa9fd5b3caa22a6bfd5dc6032ca86a Mon Sep 17 00:00:00 2001 From: Maurice Date: Sat, 29 Aug 2026 15:34:18 +0200 Subject: [PATCH] fix(map): give the Leaflet map its own zoom ceiling, so a vector basemap cannot take the planner down Opening a trip on 4.1.0 could land on the error boundary with "Map has no maxZoom specified", and so could the map settings tab, which left no screen from which to pick a different basemap. Leaflet answers getMaxZoom() from the map options first and otherwise from a layer that brought one, and only a GridLayer ever contributes via its beforeAdd hook. A vector basemap is a GL canvas on a plain L.Layer, so it contributes nothing, and MapContainer set no ceiling of its own. MarkerClusterGroup.onAdd refuses an infinite one by throwing, before any clustering, so an empty trip hit it too. That path opened when the default basemap became a MapLibre style. It reaches further than an operator choosing OpenFreeMap: resolveTileUrl sends a keyless CARTO template to the app default, which is now a vector style, so every instance still carrying the old CARTO basemap fell into it on upgrade. The ceiling belongs on the map rather than on the base layer, because it has to hold whichever of the three branches renders. 19 matches what the raster and satellite layers already carried, so nothing changes for the maps that worked. SharedTripPage gets the same treatment: it has no cluster to throw, but it draws the same vector basemap and its fitBounds asks for a ceiling. mapZoomCeiling.test.ts pins Leaflet's own semantics against the real library rather than a mock, since a mocked react-leaflet would have kept passing through all of this. --- client/src/components/Map/MapView.test.tsx | 16 +++- client/src/components/Map/MapView.tsx | 8 +- .../src/components/Map/mapZoomCeiling.test.ts | 84 +++++++++++++++++++ client/src/constants/mapDefaults.ts | 12 +++ client/src/pages/SharedTripPage.tsx | 5 +- 5 files changed, 120 insertions(+), 5 deletions(-) create mode 100644 client/src/components/Map/mapZoomCeiling.test.ts diff --git a/client/src/components/Map/MapView.test.tsx b/client/src/components/Map/MapView.test.tsx index 4d92f63f0..00911a2f1 100644 --- a/client/src/components/Map/MapView.test.tsx +++ b/client/src/components/Map/MapView.test.tsx @@ -5,6 +5,7 @@ import { act, fireEvent, waitFor } from '@testing-library/react' import userEvent from '@testing-library/user-event' import { resetAllStores } from '../../../tests/helpers/store' import { buildPlace, buildReservation } from '../../../tests/helpers/factories' +import { MAP_MAX_ZOOM } from '../../constants/mapDefaults' import { useAuthStore } from '../../store/authStore' import { CATEGORY_ICON_MAP } from '../shared/categoryIcons' import * as photoService from '../../services/photoService' @@ -48,9 +49,10 @@ vi.mock('../../hooks/useGeolocation', () => ({ const thumbCallbacks = vi.hoisted(() => new Map void>()) vi.mock('react-leaflet', () => ({ - // center/zoom are surfaced so tests can assert the camera the map is built with. - MapContainer: ({ children, center, zoom }: any) => ( -
{children}
+ // center/zoom are surfaced so tests can assert the camera the map is built + // with; maxZoom because a cluster refuses to attach to a map without one. + MapContainer: ({ children, center, zoom, maxZoom }: any) => ( +
{children}
), TileLayer: () =>
, Marker: ({ children, eventHandlers, position, icon, zIndexOffset }: any) => ( @@ -245,6 +247,14 @@ describe('MapView', () => { expect(screen.getByTestId('cluster-group')).toBeTruthy() }) + it('FE-COMP-MAPVIEW-010b: the map carries a zoom ceiling of its own, whatever the basemap is', () => { + // Without it, a vector basemap leaves the map with none and the cluster + // above throws on attach, taking the planner to the error boundary. The + // ceiling has to sit on the map because only a GridLayer can contribute one. + render() + expect(screen.getByTestId('map-container').getAttribute('data-maxzoom')).toBe(String(MAP_MAX_ZOOM)) + }) + it('FE-COMP-MAPVIEW-011: renders the route polyline; travel times are no longer drawn on the map', () => { const route = [[[48.0, 2.0], [49.0, 3.0]]] as unknown as [number, number][][] render() diff --git a/client/src/components/Map/MapView.tsx b/client/src/components/Map/MapView.tsx index fe9737e3a..c119af4a7 100644 --- a/client/src/components/Map/MapView.tsx +++ b/client/src/components/Map/MapView.tsx @@ -19,7 +19,7 @@ import { escapeHtml } from '@trek/shared' import type { Day, Reservation, RouteVia } from '../../types' import { POI_CATEGORY_BY_KEY, type Poi } from './poiCategories' import { resolveTrackColor, hasManualTrackColor } from './trackColors' -import { OFM_POSITRON, DEFAULT_MAP_CENTER, DEFAULT_MAP_ZOOM, SATELLITE_TILE_URL, SATELLITE_TILE_ATTRIBUTION, SATELLITE_TILE_MAXZOOM, attributionForTile } from '../../constants/mapDefaults' +import { OFM_POSITRON, DEFAULT_MAP_CENTER, DEFAULT_MAP_ZOOM, MAP_MAX_ZOOM, SATELLITE_TILE_URL, SATELLITE_TILE_ATTRIBUTION, SATELLITE_TILE_MAXZOOM, attributionForTile } from '../../constants/mapDefaults' import { resolveBasemap } from '../../utils/tileUrl' import VectorBasemap from './VectorBasemap' import { useSettingsStore } from '../../store/settingsStore' @@ -832,6 +832,12 @@ export const MapView = memo(function MapView({ center={initialView.center} zoom={initialView.zoom} zoomControl={false} + // On the map itself, not left to the base layer. Leaflet reads its zoom + // ceiling from the map options or, failing that, from a GridLayer that + // brought one; a vector basemap is neither, so a map drawn by + // VectorBasemap had no ceiling at all. MarkerClusterGroup.onAdd throws + // outright on an infinite one, which took the whole planner down. + maxZoom={MAP_MAX_ZOOM} className="w-full h-full bg-[#e5e7eb]" > {/* The basemap is a vector style by default and a raster template when the diff --git a/client/src/components/Map/mapZoomCeiling.test.ts b/client/src/components/Map/mapZoomCeiling.test.ts new file mode 100644 index 000000000..74664a0cf --- /dev/null +++ b/client/src/components/Map/mapZoomCeiling.test.ts @@ -0,0 +1,84 @@ +/** + * The zoom ceiling a Leaflet map needs before a marker cluster will attach. + * + * This is a test about Leaflet's own semantics rather than about our components, + * because that semantic is what a basemap change quietly broke: switching the + * default basemap to a vector style replaced the TileLayer with a GL canvas, the + * ceiling went with it, and MarkerClusterGroup.onAdd threw on the way in. The + * planner and the map settings preview both went to the error boundary, which + * left no screen from which to pick a different basemap. + * + * Nothing is mocked here on purpose. A mocked react-leaflet would have kept + * passing throughout. + */ +import { describe, it, expect, afterEach } from 'vitest' +import L from 'leaflet' +import 'leaflet.markercluster' +import { MAP_MAX_ZOOM } from '../../constants/mapDefaults' + +/** + * A layer that is not a GridLayer, which is what a vector basemap amounts to: + * maplibre-gl-leaflet hands back an L.Layer holding a canvas, so it has no + * beforeAdd and never reaches Leaflet's _addZoomLimit. + */ +const GlCanvasLayer = L.Layer.extend({ onAdd: () => undefined, onRemove: () => undefined }) +function glCanvasLayer(): L.Layer { + return new GlCanvasLayer() as L.Layer +} + +/** + * `leaflet.markercluster` ships no types and we do not depend on + * `@types/leaflet.markercluster`, so the one factory this file needs is named + * here rather than pulling a package in for a single call. + */ +const markerClusterGroup = (L as unknown as { markerClusterGroup: () => L.Layer }).markerClusterGroup + +const containers: HTMLElement[] = [] + +function mapOn(options: L.MapOptions): L.Map { + const el = document.createElement('div') + Object.defineProperty(el, 'clientWidth', { value: 800 }) + Object.defineProperty(el, 'clientHeight', { value: 600 }) + document.body.appendChild(el) + containers.push(el) + return L.map(el, { center: [48.85, 2.35], zoom: 12, ...options }) +} + +afterEach(() => { + for (const el of containers.splice(0)) el.remove() +}) + +describe('the zoom ceiling of a Leaflet map', () => { + it('MAPZOOM-001: a vector basemap contributes none, so the map has none', () => { + const map = mapOn({}) + glCanvasLayer().addTo(map) + // Only a GridLayer feeds Leaflet's _layersMaxZoom, via its beforeAdd hook. + expect(map.getMaxZoom()).toBe(Infinity) + }) + + it('MAPZOOM-002: a raster basemap contributes one, which is why raster maps never broke', () => { + const map = mapOn({}) + L.tileLayer('https://example.test/{z}/{x}/{y}.png', { maxZoom: 19 }).addTo(map) + expect(map.getMaxZoom()).toBe(19) + }) + + it('MAPZOOM-003: a cluster refuses to attach to a map with no ceiling', () => { + const map = mapOn({}) + glCanvasLayer().addTo(map) + // The throw is in onAdd, before any clustering, so an empty trip hits it too. + expect(() => markerClusterGroup().addTo(map)).toThrow(/maxZoom/) + }) + + it('MAPZOOM-004: the ceiling on the map itself carries every basemap kind', () => { + const map = mapOn({ maxZoom: MAP_MAX_ZOOM }) + glCanvasLayer().addTo(map) + expect(map.getMaxZoom()).toBe(MAP_MAX_ZOOM) + expect(() => markerClusterGroup().addTo(map)).not.toThrow() + }) + + it('MAPZOOM-005: it does not lower the raster and satellite layers, which already sat at 19', () => { + const map = mapOn({ maxZoom: MAP_MAX_ZOOM }) + L.tileLayer('https://example.test/{z}/{x}/{y}.png', { maxZoom: 19 }).addTo(map) + expect(map.getMaxZoom()).toBe(19) + }) +}) diff --git a/client/src/constants/mapDefaults.ts b/client/src/constants/mapDefaults.ts index fb0a64ac7..d97731181 100644 --- a/client/src/constants/mapDefaults.ts +++ b/client/src/constants/mapDefaults.ts @@ -3,6 +3,18 @@ export const DEFAULT_MAP_LNG = 0 export const DEFAULT_MAP_ZOOM = 2 export const DEFAULT_MAP_CENTER: [number, number] = [DEFAULT_MAP_LAT, DEFAULT_MAP_LNG] +/** + * Zoom ceiling for a Leaflet map, set on the map rather than on its base layer. + * + * Leaflet answers `getMaxZoom()` from the map options first and only then from a + * layer that carried one, and only a GridLayer ever contributes: a vector + * basemap is a GL canvas, so a map drawn by one has no ceiling from anywhere. + * `MarkerClusterGroup.onAdd` refuses an infinite ceiling by throwing, which is + * how a basemap choice could take down the whole planner. Matches the raster and + * satellite layers so nothing changes for the maps that already had one. + */ +export const MAP_MAX_ZOOM = 19 + // Tokenless satellite base layer (ESRI World Imagery) — works without an API key. export const SATELLITE_TILE_URL = 'https://server.arcgisonline.com/ArcGIS/rest/services/World_Imagery/MapServer/tile/{z}/{y}/{x}' diff --git a/client/src/pages/SharedTripPage.tsx b/client/src/pages/SharedTripPage.tsx index 88fc4b5ab..912fb22ed 100644 --- a/client/src/pages/SharedTripPage.tsx +++ b/client/src/pages/SharedTripPage.tsx @@ -19,7 +19,7 @@ import { createElement, useEffect, useRef } from 'react'; import { renderIconMarkup } from '../utils/iconMarkup'; import { MapContainer, Marker, Polyline, TileLayer, Tooltip, useMap } from 'react-leaflet'; import { getCategoryIcon } from '../components/shared/categoryIcons'; -import { OFM_POSITRON, DEFAULT_MAP_CENTER, DEFAULT_MAP_ZOOM, attributionForTile } from '../constants/mapDefaults'; +import { OFM_POSITRON, DEFAULT_MAP_CENTER, DEFAULT_MAP_ZOOM, MAP_MAX_ZOOM, attributionForTile } from '../constants/mapDefaults'; import VectorBasemap from '../components/Map/VectorBasemap'; import { SUPPORTED_LANGUAGES, useTranslation } from '../i18n'; import { useSettingsStore } from '../store/settingsStore'; @@ -489,6 +489,9 @@ export default function SharedTripPage() { center={initialView.center} zoom={initialView.zoom} zoomControl={false} + // Same reason as the planner map: a vector basemap contributes + // no zoom ceiling, and fitBounds below asks for one. + maxZoom={MAP_MAX_ZOOM} style={{ width: '100%', height: '100%' }} > {basemap.kind === 'vector' ? (