Repository navigation
fix(maps): honour the legacy dark tile URL, document raw-URL limits (#390) - #399
Conversation
…390) Follow-ups to #366. #366 restored the legacy LIGHT tile-URL override in front of the registry's implicit fallback; dark kept being masked by the registry default exactly the way light was. - map-tile-providers.js: getSelectedDarkId() mirrors getSelectedLightId() so an implicit fallback stays distinguishable from an explicit choice, and getTileSpec() resolves the legacy branch per theme. Precedence for both themes is now browser choice > configured provider default > legacy URL > built-in fallback. A raw URL keeps id:null, so it borrows no vendor credit, reference overlay or invert filter. - MC_applyTileFilter() resolves through getTileSpec() too: an operator's own dark tileset must not be CSS-inverted on top of already being dark. - roles.js: record MC_DARK_TILE_URL_OVERRIDE from tiles.dark / map.tiles.darkUrl, mirroring the light override. - config.example.json: document the four legacy raw-URL keys, their precedence, the generic credit they get, and their zoom ceiling. Configuring a provider by id (darkDefault/lightDefault) is unchanged and still takes priority, so installs that configure providers explicitly resolve through the registry exactly as before. Relates to #390.
Rapport — CS-Macmini PR#399 #390 — head eb44231Status: All three issue points addressed — point 1 fixed in code, points 2 and 3 documented with the configurable variants proposed under "Left for the owner"; every point has a test red before and green after, each with at least one mutant shown red; all CI jobs green on the first run. Evidence tags: [T] test/tool run, [A] architecture/code reasoning, [K] known/accepted. Rechecked master first: Per requirement
Why points 2 and 3 were not implemented as configBoth are phrased as "consider" and both would add new operator-facing settings, so per the brief they are written up under "Left for the owner" in the PR body rather than built. [A]
Both proposals name a concrete config shape in the PR body so the owner can decide. Tests
The single Visual check, both themesChromium against the local server,
Screenshots confirmed the dark page renders the operator's own tiles un-inverted, and that the default dark map still gets its invert filter. CI per job (run on head
|
| Job | Result |
|---|---|
| Go Build & Test | success [T] |
| Playwright E2E Tests | success [T] |
| Build & Publish Docker Image | success [T] |
| Release Artifacts | skipped (expected, non-release PR) |
| Deploy Staging | skipped (expected) |
| Publish Badges & Summary | skipped (expected) |
Green on the first run — no rerun needed, and the known-unstable test from #271 was not hit. [T]
Hygiene
cmd/serveruntouched: no Go file is in the diff at all, so no newmap[string]interface{}outside tests. [T]- No hardcoded colours. The only new literal in the resolver is an attribution string, hoisted into a shared
LEGACY_ATTRIBUTIONconstant so the code and the documentation cannot drift apart. [A] sh scripts/check-xss-sinks.sh --diff origin/master→ exit 0. [T]npx eslint public/*.js→ 0 errors (89 pre-existing warnings, none in the touched files' new lines). [T]- Fork guards unchanged: 9 in
deploy.yml, 1 inrelease-fast-path.yml; no workflow file is in the diff. [T] - No new config keys, no new dependencies. Title carries no closing keyword; the body opens with "Relates to Follow-ups to #366: dark legacy tile override, legacy attribution, zoom cap #390". PR is a draft. [T]
Compatibility note
The default provider for existing installs is unchanged. An install that selects its providers by id (darkDefault / lightDefault, which is how our deployments are configured) makes getSelectedDarkId() / getSelectedLightId() non-null and therefore never enters the legacy branch — the registry resolves exactly as before, with the proper vendor credit. [A] The behaviour change is confined to installs that set a raw URL and no provider id, which is precisely the case the issue reports as broken. MC_getDarkTileProvider() still returns a registry id in every case, so the layer control and the customizer picker are unaffected — pinned by its own test case. [T]
Residuals
- The legacy branch still prefers
cfg.tilesovercfg.map.tileswhen both config shapes are set at once, for dark as for light. Unchanged from theTILE_DARK/TILE_LIGHTlogic directly above it, so not a regression; not separately tested. [K][A] map.jsandlive.jsattach their tile layer with a hard-codedmaxZoom: 19and ignorespec.maxZoomentirely, so the legacy zoom ceiling only reaches the customizer preview maps in practice. Pre-existing and outside this issue; worth a separate look if the owner takes up the configurable-cap proposal. [A]- The layer-control picker lists registry providers only, so a legacy URL is reachable through "Auto" but is not itself a selectable entry. Pre-existing and matches the light behaviour shipped in fix(maps): preserve legacy light tile URL overrides (#364) #366. [K][A]
Review — CS-Minimax PR#399 — head eb44231Dom: APPROVE with nits Independent read-only review against Evidence tags: [T] test/tool run, [A] code reasoning, [K] known/accepted. Findings
Neither nit blocks: point 1 is a one-clause wording fix, point 2 is pre-existing logic that was relocated verbatim. No behaviour regresses. Finding 1 — detail
The P3 test pins the note against
Suggested fix: add a clause to the note (e.g. the resolved spec declares a ceiling of 18, while The hard-coded 19 itself is pre-existing and outside this issue: neither Answers to the issue pointsPoint 1 — dark legacy override is still masked. Fixed, and the fix reaches the real maps: both
Point 2 — attribution for legacy raw URLs. Honestly taken as the issue's own "or document it" branch, with the override written up under "Left for the owner". The stated reason is real: the registry's own attribution strings contain live Point 3 — zoom cap. Likewise moved to "Left for the owner" with a sound reason (raising the ceiling unconditionally would ask a z18-only provider for z19 and render blank tiles). Documented, test red before. Accepted, subject to finding 1 above. No scope creep: the diff adds no config key that any code reads ( Compatibility — the default provider is unchangedBrowser check against a local Go server,
Both config shapes behave identically, the legacy dark URL never leaks into light, the light legacy path shipped in #366 still works, and TestsEverything below run by me on the merged tree.
The one E2E prepared as in CI: freshen, the Kpa-clawbot#1486/Kpa-clawbot#1791 seed SQL, Red before / green after. Taking the new tests onto master's source: My own mutantsFive, applied to head and reverted from a scratch snapshot each time (never via
Hygiene
Not verified
|
Relates to #390.
Follow-ups to #366 (#364, merged): the dark legacy tile override, attribution for legacy raw URLs, and the zoom cap.
Plan
ce29f5dc), then mirror the fix(maps): preserve legacy light tile URL overrides (#364) #366 precedence for dark in the shared resolver.1 — Dark legacy override is no longer masked (fixed)
map-tile-providers.jsgetsgetSelectedDarkId(), the mirror ofgetSelectedLightId(), so an implicit fallback stays distinguishable from a choice the user or operator actually made.getActiveId()is nowgetSelectedDarkId() || DEFAULT_ID— a pure refactor of the same resolution chain, soMC_getDarkTileProvider()keeps returning a registry id and the layer control is unaffected.getTileSpec()resolves the legacy branch per theme. Precedence, for both themes:darkDefault/lightDefault,MC_setServerDefault*).roles.jsrecordsMC_DARK_TILE_URL_OVERRIDEfromtiles.dark/map.tiles.darkUrl, mirroring the light override. Like light, a raw URL resolves withid: null, so it borrows no vendor credit, no reference overlay and no invert filter.One extra fix the dark branch forces:
MC_applyTileFilter()resolved the filter straight out ofREGISTRY[getActiveId()], which would have inverted the operator's own dark tileset on top of it already being dark — a light basemap inside a dark page. It now resolves throughgetTileSpec()like every other reader. Light-mode and no-legacy behaviour is identical (the fallback chain is the same object).2 — Attribution for legacy raw URLs (documented)
Taken as the issue's "or document it" branch. An attribution override would be new config, so it is left for the owner (below).
config.example.jsonnow carriesmap.tiles._comment_legacy, documenting all four legacy keys, the precedence above, the generic© OpenStreetMap contributorscredit a raw URL gets, that it is never CSS-inverted, and thatdarkDefault/lightDefaultwith a registered style id is the way to get a correct vendor credit and licence link.A test asserts the documented credit is the string the resolver really returns, for both themes.
3 — Zoom cap (documented; configurable cap left for the owner)
Documented in the same note, and a test pins the documented ceiling to
MC_getTileSpec(theme).maxZoomfor a legacy URL in both themes, so the doc cannot drift from the code (the same guard #332 M9 uses for the default provider's zoom).Left for the owner
Both are "consider" points that would add new config, so they are proposals only — nothing in this PR implements them.
map.tiles.darkAttribution/lightAttribution(plus top-leveltiles.darkAttribution/lightAttributionfor symmetry), used verbatim as the legacy spec'sattributionwhen present, falling back to the generic credit. Small in code, but it is a new operator-facing setting and the value is rendered into the Leaflet attribution control, so it needs an escaping decision (the registry's own attributions contain real<a>markup) before it is safe to add.map.tiles.darkMaxZoom/lightMaxZoom, replacing the inheritedosm-*ceiling of 18 in the legacy spec. Raising the cap unconditionally is not safe: a provider that stops at 18 would then be asked for z19 and render blank tiles with no on-screen explanation, so the ceiling has to come from the operator rather than be guessed.Verification
test-issue-390-legacy-tile-followups.jsP1 (21 cases)roles.jsstops readingmap.tiles.darkUrl→ 6 reddarkDefault/ server-default cases!selectedguard dropped → 6 red;getSelectedDarkId()returns the fallback id → 11 redMC_applyTileFilter()back to theREGISTRYlookup → 2 redtest-issue-332-no-carto-tiles-e2e.js#390 steps (10)Suites run locally:
test-issue-390-legacy-tile-followups.js24/24;test-issue-332-tile-provider-fallback.js71/71;test-issue-1420-tile-providers.js39/39;test-frontend-helpers.js712/712;sh test-all.sh236/237 files;cmd/servergo test ./...ok 40.8s;test-issue-332-no-carto-tiles-e2e.js31/31 against a local Go server withe2e-fixture.dbprepared as in CI.The one
test-all.shfailure istest-backup-sqlite-safety.js(9 cases, all "missing tool setsid") — a macOS-only environment gap on this machine, unrelated to this change and present on plain master; the file is untouched here and the cases pass in CI.Both themes were also checked visually in Chromium against the local server with a legacy dark URL configured: dark + legacy renders the operator's tiles unfiltered with the generic credit, dark + default keeps the
osm-darkinvert filter, and light is unchanged either way.Scope and hygiene
cmd/serveruntouched (no Go files in the diff, so no newmap[string]interface{}outside tests). No hardcoded colours — the one new literal is a URL-free attribution string hoisted into a shared constant.scripts/check-xss-sinks.sh --diff origin/masterexits 0.npx eslint public/*.js0 errors (89 pre-existing warnings). Fork guards unchanged: 9 indeploy.yml, 1 inrelease-fast-path.yml. No new config keys, dependencies or workflow changes. The default provider for existing installs is unchanged, and a config that selects providers by id never enters the legacy branch.