From 6b9bc4b0723d826d0c5beb5ababc16f4f2f4da10 Mon Sep 17 00:00:00 2001 From: Roland Date: Sun, 2 Aug 2026 13:25:13 -0600 Subject: [PATCH] fix: the map viewer was blank in night and amber MapViewer handed protomaps-themes-base the WROLPi theme name as its basemap "flavor". That worked while the only themes were `light` and `dark`, and broke the day night and amber arrived: protomaps looks the flavor up in a record and dereferences the result without checking, so an unknown name is not a fallback but TypeError: Cannot read properties of undefined (reading 'background') at nolabels_layers (base_layers.ts:18) thrown before the first tile. MapLibre never constructed, so /map rendered nothing in two of the four themes. `mapFlavor()` in themes/names.ts maps a theme to one of the two flavors we can actually serve. Only `light` and `dark`: the flavor is interpolated into `/map-assets/sprites/` too, and those are the two sprite sets we ship, so protomaps' `black` and `grayscale` would load a style and then find no icons. Night and amber take `dark` and let the media filter tint the canvas -- a filter shifts hue, so handing them `light` would give a red or amber slab, which is the opposite of the point. Two layers of test, because they cover different halves: - map-flavor.test.js checks every theme against the REAL protomaps (calling layers() and requiring it not to throw) and against the sprites directory on disk, rather than a list of valid names copied into the test, which is what would have passed here in the first place. It fails when a fifth theme is added without a mapping. - map-themes.cy.js drives the actual page in all four themes and waits for the MapLibre canvas. The unit test cannot see whether MapViewer USES the mapping, which is the half that was broken. Verified against the planted regression: night and amber fail with no canvas, light and dark still pass. Not a bug: the 404 on /blobs/map-overview.pmtiles in the same trace. That is a HEAD probe for an optional planet overview, gated on `resp.ok`, and the blob is absent in a dev checkout. The console line is unavoidable. 1093 jest / 61 suites, clean build. Co-Authored-By: Claude Opus 5 (1M context) --- app/cypress/e2e/map-themes.cy.js | 44 +++++++++++++++++++++++ app/src/components/MapViewer.js | 5 +-- app/src/themes/map-flavor.test.js | 58 +++++++++++++++++++++++++++++++ app/src/themes/names.ts | 22 ++++++++++++ 4 files changed, 127 insertions(+), 2 deletions(-) create mode 100644 app/cypress/e2e/map-themes.cy.js create mode 100644 app/src/themes/map-flavor.test.js diff --git a/app/cypress/e2e/map-themes.cy.js b/app/cypress/e2e/map-themes.cy.js new file mode 100644 index 00000000..ddccf46f --- /dev/null +++ b/app/cypress/e2e/map-themes.cy.js @@ -0,0 +1,44 @@ +/* + * The map basemap has a "flavor" of its own, and MapViewer used to hand protomaps the WROLPi + * theme name. That worked while the only themes were `light` and `dark`, and broke when + * night and amber arrived: protomaps looks the flavor up in a record and dereferences the + * result without checking, so an unknown name throws + * `Cannot read properties of undefined (reading 'background')` before the first tile and the + * viewer renders nothing at all. + * + * Driven through the real page in every theme, because the unit test can only check that the + * mapping returns something valid -- it cannot see whether MapViewer USES it, which is the + * half that was broken. + */ +const THEMES = ['light', 'dark', 'night', 'amber']; + +describe('the map viewer builds a basemap in every theme', () => { + THEMES.forEach((theme) => { + it(`initialises MapLibre in ${theme}`, () => { + const errors = []; + cy.visit('/map', { + onBeforeLoad(win) { + // Set the theme before the app boots, so the viewer builds its style once + // with the theme under test rather than rebuilding after a switch. + win.localStorage.setItem('color-scheme', theme); + // MapViewer reports a failed init through console.error and then renders + // nothing, so the console is where the defect is visible. + const original = win.console.error; + win.console.error = (...args) => { + errors.push(args.map(String).join(' ')); + original(...args); + }; + }, + }); + + // The canvas only exists once MapLibre constructed a style successfully. + cy.get('canvas.maplibregl-canvas', {timeout: 40000}).should('exist'); + + cy.then(() => { + const failures = errors.filter(message => + /MapLibre failed to initialize|reading 'background'/.test(message)); + expect(failures.join('\n')).to.equal(''); + }); + }); + }); +}); diff --git a/app/src/components/MapViewer.js b/app/src/components/MapViewer.js index 9a4765c3..f7ce1970 100644 --- a/app/src/components/MapViewer.js +++ b/app/src/components/MapViewer.js @@ -9,6 +9,7 @@ import {addMapPin, deleteMapPin, getMapFiles, getMapPins, searchMap, setMapDefau import {MAP_VIEWER_URI} from "./Vars"; import {Button, Checkbox, Header, Icon, Panel, TextInput, Toggle} from "./ui"; import {SettingsContext, ThemeContext} from "../contexts/contexts"; +import {mapFlavor} from "../themes/names"; // Terrain DEM file prefix for hillshade and contours. const TERRAIN_PREFIX = "terrain-"; @@ -611,7 +612,7 @@ export default function MapViewer() { try { map = new maplibregl.Map({ container: mapContainer.current, - style: buildStyle(mapSources, theme, hasTerrain, scaleUnit), + style: buildStyle(mapSources, mapFlavor(theme), hasTerrain, scaleUnit), center: [initialLon, initialLat], zoom: initialZoom, attributionControl: true, @@ -757,7 +758,7 @@ export default function MapViewer() { const center = map.getCenter(); const zoom = map.getZoom(); - map.setStyle(buildStyle(activeFilesRef.current, theme, hasTerrainRef.current, scaleUnit)); + map.setStyle(buildStyle(activeFilesRef.current, mapFlavor(theme), hasTerrainRef.current, scaleUnit)); map.once("style.load", () => { map.setCenter(center); map.setZoom(zoom); diff --git a/app/src/themes/map-flavor.test.js b/app/src/themes/map-flavor.test.js new file mode 100644 index 00000000..23cc7454 --- /dev/null +++ b/app/src/themes/map-flavor.test.js @@ -0,0 +1,58 @@ +import fs from 'fs'; +import path from 'path'; +import layers from 'protomaps-themes-base'; +import {isDarkTheme, mapFlavor, themeNames} from './names'; + +/* + * The map viewer draws its basemap with protomaps-themes-base, which takes a "flavor". + * + * MapViewer used to hand it the WROLPi theme name directly. That worked while there were + * two themes called `light` and `dark`, and broke the day night and amber arrived: protomaps + * looks the flavor up in a record, finds nothing, and dereferences it anyway -- + * `TypeError: Cannot read properties of undefined (reading 'background')`, thrown before a + * single tile is drawn, so /map was blank in two of the four themes. + * + * The flavor is also interpolated into a sprite URL, so it has to be a name we ship sprites + * for, which is a smaller set again. Both halves are checked against the real thing -- + * protomaps itself, and the sprites directory on disk -- rather than against a list repeated + * here, because a list repeated here is exactly what would have passed in the first place. + */ + +const SPRITES = path.join(__dirname, '../../../modules/map/static/sprites'); + +/** The sprite sets actually shipped, from disk. */ +const spriteFlavors = () => [...new Set(fs.readdirSync(SPRITES) + .map(file => file.replace(/(@2x)?\.(json|png)$/, '')))].sort(); + +describe('the map basemap flavor', () => { + it('ships sprites for at least two flavors', () => { + // The premise for every case below. If the directory were empty or unreadable, + // `toContain` would fail for the wrong reason and read as a mapping bug. + expect(spriteFlavors().length).toBeGreaterThan(1); + }); + + it.each(themeNames)('maps %s to a flavor protomaps understands', (theme) => { + const flavor = mapFlavor(theme); + + // protomaps itself is the authority, not a list copied into this file. It throws on + // an unknown flavor rather than returning nothing, which is the whole bug. + const built = layers('src', flavor); + expect(Array.isArray(built)).toBe(true); + expect(built.length).toBeGreaterThan(0); + }); + + it.each(themeNames)('maps %s to a flavor we ship sprites for', (theme) => { + // The flavor is interpolated into `/map-assets/sprites/`, so a name protomaps + // accepts is not enough -- `black` and `grayscale` are real flavors with no sprites. + expect(spriteFlavors()).toContain(mapFlavor(theme)); + }); + + it.each(themeNames)('keeps %s on a basemap of its own brightness', (theme) => { + /* + * A dark theme must not get the light basemap. Night and amber tint the canvas with + * an SVG filter, but that only shifts hue -- a white slab filtered to red is a red + * slab, and the point of night mode is not to put one of those on screen. + */ + expect(mapFlavor(theme) === 'light').toBe(!isDarkTheme(theme)); + }); +}); diff --git a/app/src/themes/names.ts b/app/src/themes/names.ts index 7c06e157..32f0a537 100644 --- a/app/src/themes/names.ts +++ b/app/src/themes/names.ts @@ -42,6 +42,28 @@ export const isMonochromeTheme = (theme: ThemeName): boolean => export const isDarkTheme = (theme: ThemeName): boolean => darkThemes.includes(theme); +/* + * The basemap flavor for a theme. + * + * The map viewer draws with protomaps-themes-base, which takes a flavor of its own and is + * NOT a place to pass a WROLPi theme name. It looks the flavor up in a record and then + * dereferences the result without checking, so an unknown name is not a fallback but a + * `TypeError: Cannot read properties of undefined (reading 'background')` thrown before the + * first tile -- which is what /map did in night and amber once those themes existed. + * + * Only `light` and `dark` are available: the flavor is also interpolated into + * `/map-assets/sprites/`, and those are the two sprite sets we ship. protomaps' + * `black` and `grayscale` would load a style and then fail to find any icons. + * + * Night and amber take `dark` and let the media filter tint the canvas from there. Handing + * them `light` would not work even with the filter on: a filter shifts hue, so a white + * basemap becomes a red one, which is the opposite of what a night-vision theme is for. + */ +export type MapFlavor = 'light' | 'dark'; + +export const mapFlavor = (theme: ThemeName): MapFlavor => + isDarkTheme(theme) ? 'dark' : 'light'; + export const isThemeName = (value: unknown): value is ThemeName => themeNames.includes(value as ThemeName);