feat(layers): show a tiled archive as a group of its source layers - #2065
feat(layers): show a tiled archive as a group of its source layers#2065clintonlunn wants to merge 2 commits into
Conversation
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🔍 Cloudflare PR preview
|
🔍 GitHub Pages PR preview
Note GitHub Pages built this preview successfully, but its serving edge returned HTTP 403 when checked. The links may still be propagating. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| nativeLayerIds: options.nativeLayerIds?.filter((id) => | ||
| id.includes(encodeVectorTileLayerPart(sourceLayer)), | ||
| ), |
There was a problem hiding this comment.
Bug: .includes() does substring matching, not exact-segment matching, so two source layers whose encoded names overlap (e.g. water / waterway, or road / railroad) collide. encodeVectorTileLayerPart("waterway") contains encodeVectorTileLayerPart("water") as a substring, so "grid-waterway-fill".includes("water") is true — the water split layer's metadata.nativeLayerIds ends up also containing waterway's native ids.
This isn't just cosmetic: syncExternalNativeLayer's fallback loop (layer-sync.ts ~line 629, reached after ensurePMTilesExternalLayer for a PMTiles vector layer) iterates every id in metadata.nativeLayerIds and applies this layer's visibility, feature filters, zoom range and z-order to whatever native MapLibre layer that id resolves to via map.getLayer. With the over-inclusive list, toggling/reordering the water layer would also mutate waterway's native layer.
Since pmtilesVectorLayerId (same file, used a few lines up and in hasPMTilesNativeSourceLayer) already builds the exact id for a given source layer + kind, matching against that exactly would avoid the collision:
| nativeLayerIds: options.nativeLayerIds?.filter((id) => | |
| id.includes(encodeVectorTileLayerPart(sourceLayer)), | |
| ), | |
| nativeLayerIds: options.nativeLayerIds?.filter((id) => | |
| ["fill", "line", "circle"].some( | |
| (kind) => id === pmtilesVectorLayerId(options.id, sourceLayer, kind), | |
| ), | |
| ), |
Confidence: medium-high — traced through getExternalNativeLayerIds → syncExternalNativeLayer's loop, but haven't run it live.
| true, | ||
| false, | ||
| ]), | ||
| paint: fillExtrusionPaint(layer.style, layer.opacity), |
There was a problem hiding this comment.
Bug (likely oversight): this still uses layer.style instead of the partStyle computed above (line 3087) for this sourceLayer. Every other paint call in this loop (fill at 3133, line at 3154, circle at 3175) was switched to partStyle, and the analogous fill-extrusion branch in syncMbtilesVectorLayer (line 3242) does use partStyle. As written, a vector-tiles-backed archive layer with extrusionEnabled won't pick up its assigned per-source-layer colour — it'll paint every source layer in the archive's own uniform layer.style colour instead.
| paint: fillExtrusionPaint(layer.style, layer.opacity), | |
| paint: fillExtrusionPaint(partStyle, layer.opacity), |
Confidence: medium-high — grep across the file shows this is the one Paint(layer.style, …) call left inside a partStyle-scoped loop; no test exercises extrusion + archive colouring together, which would have caught it.
| const known = layers.filter((layer) => store.layers.some((item) => item.id === layer.id)); | ||
| if (known.length > 0) { | ||
| for (const layer of known) { | ||
| store.updateLayer(layer.id, { | ||
| metadata: layer.metadata, | ||
| opacity: layer.opacity, | ||
| source: layer.source, | ||
| style: layer.style, | ||
| visible: layer.visible, | ||
| }); | ||
| } | ||
| return; | ||
| } |
There was a problem hiding this comment.
Minor edge case: this branches on "at least one of the computed layers is already known", not "all of them are". If a later layeradd event ever reports a different/larger set of source layers for the same event.layerId than an earlier event did (e.g. archive metadata finishes loading asynchronously after a partial first add), known would be a strict subset of layers. This branch would then only update the already-known subset and return — the newly-appeared source layers would never be added to the store, and no group would be created for them.
Not sure this is reachable given how the PMTiles control emits layeradd today, so flagging at lower confidence, but it's worth double-checking whether layerInfo's source-layer set can change across events for the same id, and if so, reconciling known vs layers (add the missing ones) rather than early-returning on partial overlap.
Confidence: low-medium — plausible from reading the store-sync logic, not verified against the control's actual event sequence.
| }); | ||
| return { | ||
| ...layer, | ||
| metadata: { ...layer.metadata, sourceId: options.id, archiveId: options.id }, |
There was a problem hiding this comment.
Minor: archiveId is set to the same value as sourceId right above it, and nothing in this PR (or elsewhere in the repo, by grep) reads metadata.archiveId. If it's not needed by other code, consider dropping it to avoid a redundant/dead field; if it's meant for something downstream (e.g. future grouping lookups), a short comment on why it's distinct from sourceId would help.
Confidence: low — quality nit, not a correctness issue.
Code reviewBugs
Quality
Security / Performance / CLAUDE.md
|
| // The colours a control gave an archive's source layers. | ||
| // | ||
| // A control assigns one per source layer and records them all, but a layer carries a single style, | ||
| // so every part drew in the first one's colour. This is for archives that are still one layer — the | ||
| // STAC panel's, an offline extract. One added through the PMTiles control is split into a layer per | ||
| // source layer and never reaches here. |
There was a problem hiding this comment.
This module's payoff for "archives that are still one layer" appears unreachable in practice. assignedSourceLayerColor/styleForSourceLayer only do anything when layer.metadata.sourceLayerColors is set, but the only place that ever sets it is pmtilesLayerOptions in packages/plugins/src/plugins/maplibre-components.ts (fed from the PMTiles control's layerInfo.sourceLayerColors) — and that path always goes through createPMTilesArchiveLayers, which splits any archive with 2+ source layers into separate layers rather than keeping it as one.
The two callers this comment names as the "still one layer" beneficiaries — addPMTilesAsset in packages/plugins/src/plugins/stac-layers.ts and the vector branch of BasemapExtractPanel.tsx (~line 707) — call createPMTilesStoreLayer directly and never pass sourceLayerColors. So a multi-source-layer archive added from the STAC panel or the offline basemap extract will still render every source layer in one flat colour, same as before this PR, despite the PR description ("Colours where an archive is still one layer") claiming otherwise.
Worth double-checking against a real STAC/offline-extract archive with several source layers — if I'm right, either those two call sites need to start populating sourceLayerColors, or the PR description/this comment should be corrected. (Confidence: medium-high, based on static analysis — I don't have a way to run the app here.)
| // Each layer is added or updated on its own, so a later event reporting a source layer the | ||
| // first did not still lands rather than being skipped as "this archive is already here". | ||
| const added: string[] = []; | ||
| for (const layer of layers) { | ||
| if (store.layers.some((item) => item.id === layer.id)) { | ||
| store.updateLayer(layer.id, { | ||
| metadata: layer.metadata, | ||
| opacity: layer.opacity, | ||
| source: layer.source, | ||
| style: layer.style, | ||
| visible: layer.visible, | ||
| }); | ||
| continue; | ||
| } | ||
| store.addLayer(layer); | ||
| added.push(layer.id); | ||
| } | ||
| // An archive of several source layers is a folder of them, named after the archive. | ||
| if (layers.length > 1 && added.length === layers.length) { | ||
| store.addLayerGroup( | ||
| layerInfo.name || layerNameFromUrl(layerInfo.url, event.layerId), | ||
| layers.map((layer) => layer.id), | ||
| ); | ||
| } |
There was a problem hiding this comment.
The comment on lines 4810-4811 anticipates a later layeradd event reporting a source layer the first event didn't (e.g. metadata that loads incrementally), and the per-layer add/update loop does handle that correctly for the store layer itself. But the group-folding step right below it doesn't: addLayerGroup (in packages/core/src/store.ts) always creates a brand-new group, so it's only called here when added.length === layers.length — i.e. only on the very first event where every split layer is new.
If a later event for the same archive id does add a genuinely new source layer (some already existed and were updated, added.length is between 0 and layers.length), that new layer is pushed into the store but this if is false, so it's never folded into the existing group — it lands as a bare top-level layer instead of inside the archive's folder. This path doesn't appear to be covered by layer-parts-every-path.test.ts or pmtiles-archive-layers.test.ts, both of which construct layers directly rather than driving createPMTilesLayerAddHandler through multiple events.
If the control genuinely never re-fires layeradd with a growing source-layer set for the same id, this is dead code and harmless; if it can (which the comment above suggests was a real concern), this is a real gap. Confidence: medium — I can't inspect the maplibre-gl-components control's emission behavior from here (not installed in this sandbox).
| return sourceLayers.map((sourceLayer) => { | ||
| const assigned = options.sourceLayerColors?.[sourceLayer]; | ||
| const layer = createPMTilesStoreLayer({ | ||
| ...options, | ||
| id: `${options.id}-${encodeVectorTileLayerPart(sourceLayer)}`, | ||
| name: sourceLayer, | ||
| sourceLayers: [sourceLayer], | ||
| nativeLayerIds: ownNativeLayerIds(options.nativeLayerIds, options.id, sourceLayer), | ||
| ...(assigned | ||
| ? { style: { ...options.style, fillColor: assigned, strokeColor: assigned } } | ||
| : {}), | ||
| }); | ||
| return { ...layer, metadata: { ...layer.metadata, sourceId: options.id } }; | ||
| }); |
There was a problem hiding this comment.
Minor: ...options is spread into each split child's createPMTilesStoreLayer call, so every child ends up carrying the entire archive's sourceLayerColors map in its own metadata.sourceLayerColors (e.g. the "water" child also stores the "waterway" colour), not just its own entry. It's harmless today — assignedSourceLayerColor only ever looks up the child's own single source layer — but it's a bit of unnecessary duplication per layer, and a future reader of a single layer's metadata could reasonably be surprised to find colours for other layers embedded in it. Not blocking.
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
Otherwise the core refactor (splitting a PMTiles archive into a layer group, refcounting the shared source in |
3547866 to
63f6ff2
Compare
| for (const layer of removedLayers) { | ||
| pmtilesControl?.removeLayer(layer.id); | ||
| for (const archiveId of pmtilesArchivesFullyRemoved(previous.layers, state.layers)) { | ||
| pmtilesControl?.removeLayer(archiveId); |
There was a problem hiding this comment.
Bug (medium-high confidence): pmtilesArchivesFullyRemoved isn't filtered by controlOwnedArchives, unlike its sibling pmtilesLayerIdsToRemove (which takes an owned set for exactly this reason — see the comment on controlOwnedArchives a few lines above and on isPMTilesControlLayer/pmtilesArchiveId below).
isPMTilesControlLayer now only requires metadata.controlArchiveId to be set, which survives a project save/reload and outlives the control instance that added it. controlOwnedArchives is cleared in teardownPMTilesControl (closing the panel) and rebuilt only from layeradd events on the new control instance.
Concrete sequence: add an archive via the control → close the panel (controlOwnedArchives.clear()) → reopen it (fresh control, controlOwnedArchives still empty for this archive since it was never re-reported via layeradd) → delete one of the archive's layers from the Layers panel. The store subscription still sees a layer whose shape passes isPMTilesControlLayer (it carries controlArchiveId), reports the archive as fully removed, and calls pmtilesControl.removeLayer(archiveId) on a control instance that never loaded that archive — the exact "clear-all takes an archive it never added" failure mode the ownership set was introduced to prevent, just on the store→control direction instead of control→store.
Consider gating the call on ownership too:
| pmtilesControl?.removeLayer(archiveId); | |
| pmtilesStoreUnsubscribe ??= useAppStore.subscribe((state, previous) => { | |
| for (const archiveId of pmtilesArchivesFullyRemoved(previous.layers, state.layers)) { | |
| if (!controlOwnedArchives.has(archiveId)) continue; | |
| pmtilesControl?.removeLayer(archiveId); | |
| } | |
| }); |
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
The rest of the change (native-layer-id partitioning for overlapping source-layer names, the source-refcounting in |
63f6ff2 to
f2ee9c0
Compare
| export function addPMTilesArchive(layers: readonly GeoLibreLayer[], name: string): string[] { | ||
| const store = useAppStore.getState(); | ||
| // Taken before the adds, which is what it should be: the ids within one archive are distinct, so | ||
| // nothing added here can read back as already known. | ||
| const known = new Set(store.layers.map((item) => item.id)); | ||
| const added: string[] = []; | ||
| for (const layer of layers) { | ||
| if (known.has(layer.id)) { | ||
| store.updateLayer(layer.id, { | ||
| metadata: layer.metadata, | ||
| opacity: layer.opacity, | ||
| source: layer.source, | ||
| style: layer.style, | ||
| visible: layer.visible, | ||
| }); | ||
| continue; | ||
| } | ||
| store.addLayer(layer); | ||
| added.push(layer.id); |
There was a problem hiding this comment.
known is a snapshot of the store taken once, before the loop, and is never updated as layers are pushed within this same call (comment on line 22-23 assumes "the ids within one archive are distinct, so nothing added here can read back as already known" — but that's not quite guaranteed).
encodeVectorTileLayerPart (packages/map/src/vector-tile-layer-ids.ts) is documented as not injective: a/b and a_2Fb both encode to a_2Fb. createPMTilesArchiveLayers builds each split layer's id as `${options.id}-${encodeVectorTileLayerPart(sourceLayer)}`, so an archive with two source layers whose names collide under that encoding produces two GeoLibreLayer objects sharing one id.
Since known doesn't grow during this loop, the second occurrence isn't recognized as "already added in this batch" — it falls to store.addLayer(layer) again. addLayer in packages/core/src/store.ts just appends (layers.push(...)) with no id-uniqueness check, so the store ends up with two distinct layer objects sharing the same id. That breaks the id-uniqueness invariant syncLayers/removeLayerFromMap (keyed Maps/Sets over layer id) and the Layers panel (id as React key) rely on elsewhere.
This is a pre-existing, narrow edge case in the encoder rather than something newly introduced by the encoding itself, but this PR is what first makes two distinct layers derive their id from it in the same batch — previously an archive was always a single layer, so there was nothing to collide with. Low likelihood (needs unusually-named vector-tile layers in the same archive) but worth a guard, e.g. updating known inside the loop:
| export function addPMTilesArchive(layers: readonly GeoLibreLayer[], name: string): string[] { | |
| const store = useAppStore.getState(); | |
| // Taken before the adds, which is what it should be: the ids within one archive are distinct, so | |
| // nothing added here can read back as already known. | |
| const known = new Set(store.layers.map((item) => item.id)); | |
| const added: string[] = []; | |
| for (const layer of layers) { | |
| if (known.has(layer.id)) { | |
| store.updateLayer(layer.id, { | |
| metadata: layer.metadata, | |
| opacity: layer.opacity, | |
| source: layer.source, | |
| style: layer.style, | |
| visible: layer.visible, | |
| }); | |
| continue; | |
| } | |
| store.addLayer(layer); | |
| added.push(layer.id); | |
| const added: string[] = []; | |
| for (const layer of layers) { | |
| if (known.has(layer.id)) { | |
| store.updateLayer(layer.id, { | |
| metadata: layer.metadata, | |
| opacity: layer.opacity, | |
| source: layer.source, | |
| style: layer.style, | |
| visible: layer.visible, | |
| }); | |
| continue; | |
| } | |
| known.add(layer.id); | |
| store.addLayer(layer); | |
| added.push(layer.id); | |
| } |
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
f2ee9c0 to
ad679ee
Compare
| export function createPMTilesArchiveLayers(options: PMTilesStoreLayerOptions): GeoLibreLayer[] { | ||
| const sourceLayers = [...options.sourceLayers]; | ||
| if (options.tileType === "raster" || sourceLayers.length < 2) { | ||
| return [createPMTilesStoreLayer(options)]; | ||
| } | ||
| return sourceLayers.map((sourceLayer) => { | ||
| const layer = createPMTilesStoreLayer({ | ||
| ...options, | ||
| id: `${options.id}-${encodeVectorTileLayerPart(sourceLayer)}`, | ||
| name: sourceLayer, | ||
| sourceLayers: [sourceLayer], | ||
| nativeLayerIds: ownNativeLayerIds(options.nativeLayerIds, options.id, sourceLayer), | ||
| }); | ||
| // Both fields, because readers are split: `getPMTilesSourceId` prefers the metadata, | ||
| // `loadedVectorTileFeatures` reads `source.sourceId` alone and swallows a bad id in a `catch`. | ||
| return { | ||
| ...layer, | ||
| source: { ...layer.source, sourceId: options.id }, | ||
| metadata: { ...layer.metadata, sourceId: options.id }, | ||
| }; | ||
| }); | ||
| } |
There was a problem hiding this comment.
Bug (medium confidence): createPMTilesArchiveLayers doesn't dedupe sourceLayers before mapping each one to a store layer whose id is `${options.id}-${encodeVectorTileLayerPart(sourceLayer)}`. Two entries produce the same store-layer id whenever:
- the archive's
vector_layersmetadata literally repeats a source-layer name (malformed/duplicate tileset metadata happens in the wild), or - two distinct names collide after encoding —
encodeVectorTileLayerPartis documented as "not injective" (a/banda_2Fbboth encode toa_2Fb).
addPMTilesArchive computes its known set once, before iterating, so a colliding second layer isn't recognized as a duplicate: it goes through store.addLayer(layer) again with the same id as the first, silently clobbering/duplicating a layer instead of erroring or merging.
Consider deduping (e.g. new Set(options.sourceLayers)) — or at least logging/dropping repeats — before mapping, since this is now a primary store key rather than just a MapLibre native-layer id suffix (where the pre-existing non-injectivity was lower-stakes).
| export function createPMTilesArchiveLayers(options: PMTilesStoreLayerOptions): GeoLibreLayer[] { | |
| const sourceLayers = [...options.sourceLayers]; | |
| if (options.tileType === "raster" || sourceLayers.length < 2) { | |
| return [createPMTilesStoreLayer(options)]; | |
| } | |
| return sourceLayers.map((sourceLayer) => { | |
| const layer = createPMTilesStoreLayer({ | |
| ...options, | |
| id: `${options.id}-${encodeVectorTileLayerPart(sourceLayer)}`, | |
| name: sourceLayer, | |
| sourceLayers: [sourceLayer], | |
| nativeLayerIds: ownNativeLayerIds(options.nativeLayerIds, options.id, sourceLayer), | |
| }); | |
| // Both fields, because readers are split: `getPMTilesSourceId` prefers the metadata, | |
| // `loadedVectorTileFeatures` reads `source.sourceId` alone and swallows a bad id in a `catch`. | |
| return { | |
| ...layer, | |
| source: { ...layer.source, sourceId: options.id }, | |
| metadata: { ...layer.metadata, sourceId: options.id }, | |
| }; | |
| }); | |
| } | |
| export function createPMTilesArchiveLayers(options: PMTilesStoreLayerOptions): GeoLibreLayer[] { | |
| const sourceLayers = [...new Set(options.sourceLayers)]; | |
| if (options.tileType === "raster" || sourceLayers.length < 2) { | |
| return [createPMTilesStoreLayer(options)]; | |
| } | |
| return sourceLayers.map((sourceLayer) => { | |
| const layer = createPMTilesStoreLayer({ | |
| ...options, | |
| id: `${options.id}-${encodeVectorTileLayerPart(sourceLayer)}`, | |
| name: sourceLayer, | |
| sourceLayers: [sourceLayer], | |
| nativeLayerIds: ownNativeLayerIds(options.nativeLayerIds, options.id, sourceLayer), | |
| }); | |
| // Both fields, because readers are split: `getPMTilesSourceId` prefers the metadata, | |
| // `loadedVectorTileFeatures` reads `source.sourceId` alone and swallows a bad id in a `catch`. | |
| return { | |
| ...layer, | |
| source: { ...layer.source, sourceId: options.id }, | |
| metadata: { ...layer.metadata, sourceId: options.id }, | |
| }; | |
| }); | |
| } |
| // An archive's source layers share one source, so it goes only once nothing draws from it. | ||
| const stillInUse = new Set( | ||
| (survivingLayers ?? []) | ||
| .filter((candidate) => candidate.id !== layerId) | ||
| .flatMap((candidate) => getExternalSourceIds(candidate)), | ||
| ); |
There was a problem hiding this comment.
Performance (low confidence, minor): removeLayerFromMap is called once per removed layer id from the loop in MapController.syncLayers, and each call rebuilds stillInUse from scratch by scanning the entire survivingLayers (next-state) array. For a sync pass that removes many layers at once (bulk delete, project switch tearing down a large layer list), that's O(removed × surviving) work redone every call instead of once. Given typical layer counts this is unlikely to matter, but if it ever does, computing the shared "sources still in use" set once in syncLayers and threading it through (instead of recomputing per call) would avoid the repeated scan.
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
ea56cb2 to
7f7d4c9
Compare
| const drawnFromSource = (src: string): boolean => | ||
| (map.getStyle()?.layers ?? []).some( | ||
| (styleLayer) => "source" in styleLayer && styleLayer.source === src, | ||
| ); | ||
| for (const src of [ | ||
| ...getExternalSourceIds(layer), | ||
| sourceId(layerId), | ||
| labelSourceId(layerId), | ||
| invertedSourceId(layerId), | ||
| generatorSourceId(layerId), | ||
| ]) { | ||
| if (src && map.getSource(src)) map.removeSource(src); | ||
| if (src && !stillInUse.has(src) && !drawnFromSource(src) && map.getSource(src)) { |
There was a problem hiding this comment.
drawnFromSource calls map.getStyle() — which serializes the whole style (sources, layers, sprite, glyphs) — and it's invoked fresh for every source candidate in the loop, not just the PMTiles archive-sharing case. Two compounding issues:
- It's evaluated before the cheap
map.getSource(src)existence check in the&&chain, so it runs even when the candidate source doesn't exist at all (the common case forlabelSourceId/invertedSourceId/generatorSourceIdon an ordinary layer). map.getStyle()is re-computed on every iteration of thefor (const src of [...])loop instead of once perremoveLayerFromMapcall.
Since this runs on every layer removal (not just PMTiles archives — stillInUse is empty whenever survivingLayers isn't passed or doesn't reference the source), this adds up to several full style serializations on every ordinary "delete a layer" action, which previously was just cheap Map/object lookups.
Suggested fix — hoist the style read out of the loop and check existence first:
| const drawnFromSource = (src: string): boolean => | |
| (map.getStyle()?.layers ?? []).some( | |
| (styleLayer) => "source" in styleLayer && styleLayer.source === src, | |
| ); | |
| for (const src of [ | |
| ...getExternalSourceIds(layer), | |
| sourceId(layerId), | |
| labelSourceId(layerId), | |
| invertedSourceId(layerId), | |
| generatorSourceId(layerId), | |
| ]) { | |
| if (src && map.getSource(src)) map.removeSource(src); | |
| if (src && !stillInUse.has(src) && !drawnFromSource(src) && map.getSource(src)) { | |
| const styleLayers = map.getStyle()?.layers ?? []; | |
| const drawnFromSource = (src: string): boolean => | |
| styleLayers.some((styleLayer) => "source" in styleLayer && styleLayer.source === src); | |
| for (const src of [ | |
| ...getExternalSourceIds(layer), | |
| sourceId(layerId), | |
| labelSourceId(layerId), | |
| invertedSourceId(layerId), | |
| generatorSourceId(layerId), | |
| ]) { | |
| if (src && map.getSource(src) && !stillInUse.has(src) && !drawnFromSource(src)) { | |
| map.removeSource(src); | |
| } | |
| } |
Confidence: medium-high — correctness is unaffected (the cheap sourceId(layerId) etc. are unique per removed layer, so drawnFromSource almost always resolves false for them anyway), this is purely about avoiding needless getStyle() calls on a hot path.
| @@ -1,6 +1,6 @@ | |||
| import { useAppStore } from "@geolibre/core"; | |||
| import { createPMTilesStoreLayer, readRemotePMTilesInfo } from "@geolibre/map/pmtiles-layer"; | |||
| import { createPMTilesArchiveLayers, readRemotePMTilesInfo } from "@geolibre/map/pmtiles-layer"; | |||
There was a problem hiding this comment.
Nit: the function's docstring (a few lines below, unchanged by this diff) still says "the layer shape still comes from {@link createPMTilesStoreLayer}", but addPMTilesAsset now goes through createPMTilesArchiveLayers, which can return several layers (one per source layer) rather than a single one. Worth updating the @link/wording so it doesn't undersell that an asset can now land as multiple layers in a folder.
Confidence: low (doc-only, no behavioral impact).
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
268e0d3 to
6fcf5a6
Compare
| const archiveId = layers[0]?.metadata.sourceId; | ||
| if (typeof archiveId === "string") { | ||
| for (const stale of store.layers) { | ||
| if (stale.metadata.sourceId === archiveId && !ids.has(stale.id)) store.removeLayer(stale.id); | ||
| } | ||
| } |
There was a problem hiding this comment.
This stale-layer cleanup matches purely on stale.metadata.sourceId === archiveId across every layer in the store, not just PMTiles/archive-owned ones — metadata.sourceId is a generic key several other layer kinds set too (see getExternalSourceIds/getLayerSourceIds in layer-sync.ts/map-controller.ts). If an unrelated layer's metadata.sourceId ever collided with a freshly generated archive id, it would be silently removeLayer'd here.
In practice ids are UUID/generator-derived so a real collision is very unlikely, but since this is the one place in the PR that removes layers based on a loosely-typed metadata match rather than an explicit "this is a PMTiles archive layer" check (contrast with isPMTilesControlLayer/pmtilesArchiveId in maplibre-components.ts), it might be worth scoping the stale.metadata.sourceId === archiveId match to layers that are also recognizably PMTiles archive layers (e.g. stale.type === "pmtiles"). Confidence: low — defensive-coding suggestion rather than an observed failure.
| @@ -5425,7 +5471,7 @@ export function pmtilesStoreLayer(id: string, layerInfo: PMTilesLayerInfo): GeoL | |||
| // The control created these MapLibre layers itself, so its ids stand rather than derived ones. | |||
| nativeLayerIds: layerInfo.layerIds, | |||
There was a problem hiding this comment.
Nit: this comment ("The control created these MapLibre layers itself, so its ids stand rather than derived ones") is only accurate for the single-layer path now. Once createPMTilesArchiveLayers splits an archive into per-source-layer layers, it unconditionally overrides nativeLayerIds: undefined and re-derives them from the naming scheme (see pmtiles-layer.ts), discarding layerInfo.layerIds for every split layer. Worth a one-line update so a future reader doesn't assume the control's own ids always survive.
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
8ad326e to
315875d
Compare
| if (typeof archiveId === "string") { | ||
| for (const stale of store.layers) { | ||
| // `metadata.sourceId` is a generic key other layer kinds set too, so the type is checked | ||
| // rather than trusting an id match to mean "a layer of this archive". | ||
| if (stale.type !== "pmtiles" || stale.metadata.sourceId !== archiveId) continue; | ||
| if (!ids.has(stale.id)) store.removeLayer(stale.id); | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug (moderate confidence): this "stale" pass removes layers left over from an old id scheme, but doesn't clean up a now-empty layer group the way createPMTilesLayerRemoveHandler in maplibre-components.ts does for a control-initiated removal.
Trace: archive first arrives with 2 source layers → split into 2 layers + a new folder (line 55-58 below). A later re-add for the same archive reports only 1 distinct id (e.g. the source-layer count shrinks, or tileType flips to raster so createPMTilesArchiveLayers forces the single-layer branch) → this loop removes both old split layers (stale.id not in the new ids) → the grouping block at line 49 is skipped entirely because layers.length is now 1 (layers.length > 1 is false) → the folder that held them is never removed and lingers empty in layerGroups (and gets persisted into the saved project).
Consider tracking the groups of removed stale layers here and pruning any that end up empty, mirroring the cleanup in createPMTilesLayerRemoveHandler:
| if (typeof archiveId === "string") { | |
| for (const stale of store.layers) { | |
| // `metadata.sourceId` is a generic key other layer kinds set too, so the type is checked | |
| // rather than trusting an id match to mean "a layer of this archive". | |
| if (stale.type !== "pmtiles" || stale.metadata.sourceId !== archiveId) continue; | |
| if (!ids.has(stale.id)) store.removeLayer(stale.id); | |
| } | |
| } | |
| const archiveId = layers[0]?.metadata.sourceId; | |
| if (typeof archiveId === "string") { | |
| const staleGroupIds = new Set<string>(); | |
| for (const stale of store.layers) { | |
| // `metadata.sourceId` is a generic key other layer kinds set too, so the type is checked | |
| // rather than trusting an id match to mean "a layer of this archive". | |
| if (stale.type !== "pmtiles" || stale.metadata.sourceId !== archiveId) continue; | |
| if (ids.has(stale.id)) continue; | |
| if (stale.groupId) staleGroupIds.add(stale.groupId); | |
| store.removeLayer(stale.id); | |
| } | |
| const afterStale = useAppStore.getState(); | |
| for (const groupId of staleGroupIds) { | |
| if (!afterStale.layers.some((layer) => layer.groupId === groupId)) { | |
| afterStale.removeLayerGroup(groupId); | |
| } | |
| } | |
| } |
| metadata: { | ||
| externalNativeLayer: true, | ||
| nativeLayerIds: [ | ||
| ...(options.nativeLayerIds ?? pmtilesNativeLayerIds(id, tileType, sourceLayers)), | ||
| ...(options.nativeLayerIds ?? pmtilesNativeLayerIds(sourceId, tileType, sourceLayers)), | ||
| ], | ||
| pickable: options.pickable ?? true, | ||
| sourceId: id, | ||
| sourceId, | ||
| sourceKind: "pmtiles-url", | ||
| ...(options.sourceLayerColors ? { sourceLayerColors: options.sourceLayerColors } : {}), | ||
| sourceLayers, | ||
| tileType, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Quality (medium confidence): this hunk drops the sourceLayerColors field that used to be written into metadata here (removed line: ...(options.sourceLayerColors ? { sourceLayerColors: options.sourceLayerColors } : {})), without replacing its purpose anywhere else.
The PR description says the "archive stays one layer" paths (STAC panel, offline basemap extract) "paint each [source layer] in the colour the archive assigned (assignedSourceLayerColor), which was already computed at add time and previously discarded after the first. A user restyling the layer takes it back," crediting a new test layer-parts-every-path.test.ts.
Neither seems to hold as shipped: fillColor above (line ~81-83) still only ever reads sourceLayerColors[sourceLayers[0]], unchanged from main — so a multi-source-layer archive that stays a single layer (basemap extract, or any STAC asset whose sourceLayers happen to collide/dedupe to one id) still has no way to color source layers past the first, same as before this PR. And layer-parts-every-path.test.ts doesn't exist anywhere in this repo/diff. Worth checking whether the PR body was written against an earlier version of the change — as written it may overstate what this PR actually fixes for per-source-layer coloring.
| export function createPMTilesArchiveLayers(options: PMTilesStoreLayerOptions): GeoLibreLayer[] { | ||
| // Keyed by the id each source layer would take, not by its name: `encodeVectorTileLayerPart` is | ||
| // not injective (`a/b` and `a_2Fb` both encode to `a_2Fb`), and an archive's metadata can repeat | ||
| // a name outright. Either way a second layer would carry the first one's id. | ||
| const parts = new Map<string, string>(); | ||
| for (const sourceLayer of options.sourceLayers) { | ||
| const id = `${options.id}-${encodeVectorTileLayerPart(sourceLayer)}`; | ||
| if (!parts.has(id)) parts.set(id, sourceLayer); | ||
| } | ||
| if (options.tileType === "raster" || parts.size < 2) { | ||
| // Deduped here too, so one layer never carries the same source layer twice. | ||
| return [ | ||
| createPMTilesStoreLayer({ ...options, sourceLayers: [...new Set(options.sourceLayers)] }), | ||
| ]; | ||
| } | ||
| return [...parts].map(([id, sourceLayer]) => | ||
| createPMTilesStoreLayer({ | ||
| ...options, | ||
| id, | ||
| name: sourceLayer, | ||
| sourceLayers: [sourceLayer], | ||
| // The archive's source, and so the archive's ids: a layer deriving its own would name ids | ||
| // nothing on the map answers to. | ||
| sourceId: options.id, | ||
| nativeLayerIds: undefined, | ||
| }), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Quality (low confidence, minor): when two source layer names collide under encodeVectorTileLayerPart (or are literal duplicates), the second one is silently dropped — its features never render under any layer, split or unsplit, and nothing surfaces this to the user (confirmed intentional by the pmtiles-archive-layers.test.ts case "the colliding second name is dropped, the first stands"). That's a reasonable fallback for an edge case, but a real archive with two source layers whose data quietly vanishes with no warning/log could be confusing to debug. Worth at least a one-line log/console warning when this happens, if not already planned as follow-up.
|
All three inline comments posted. Here is the final summary. Code reviewBugs
Security
Performance
Quality
CLAUDE.md
I focused on the new PMTiles-archive splitting/grouping logic, the shared-source refcounting in |
315875d to
8d0887d
Compare
| // The control no longer has it, so neither does the claim. | ||
| const stillListed = new Set(event.state.layers.map((layer) => layer.id)); | ||
| for (const archiveId of controlOwnedArchives) { | ||
| if (!stillListed.has(archiveId)) controlOwnedArchives.delete(archiveId); | ||
| } |
There was a problem hiding this comment.
This drops ownership for any archiveId currently in controlOwnedArchives that isn't in event.state.layers, not just archives touched by this particular removal. If the control ever emits a layerremove event whose state.layers snapshot is momentarily incomplete for an unrelated archive (e.g. a transient event fired while another archive is still being loaded/registered), this would silently forget ownership of a still-live, still-owned archive. A later re-add of that archive would then be treated as never having been the control's own (e.g. pmtilesArchivesFullyRemoved would stop reporting it, and a future clear-all would leave its layers behind).
Since this is about the external control's event-ordering guarantees (which aren't visible from this diff), I'm not fully certain this can happen in practice — flagging as a moderate-confidence concern rather than a confirmed bug.
| if (layers.length > 1 && added.length > 0) { | ||
| const state = useAppStore.getState(); | ||
| const existing = state.layers.find((item) => ids.has(item.id) && item.groupId)?.groupId; | ||
| if (existing) { | ||
| state.moveLayersToGroup(added, existing); | ||
| } else { | ||
| state.addLayerGroup( | ||
| name, | ||
| layers.map((layer) => layer.id), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Edge case: if a later read adds a genuinely new source layer (added.length > 0) while every pre-existing sibling of this archive has been manually moved out of its folder by the user (so none of ids currently carries a groupId), existing resolves to undefined and this falls into addLayerGroup(name, layers.map(l => l.id)) — which passes every id in the archive, including the ones the user deliberately ungrouped, silently re-folding them into a brand-new group. Minor/edge-case UX regression rather than a crash, and only reachable when at least one truly new layer arrives for an archive whose other layers were all manually ungrouped.
| const parts = new Map<string, string>(); | ||
| for (const sourceLayer of options.sourceLayers) { | ||
| const id = `${options.id}-${encodeVectorTileLayerPart(sourceLayer)}`; | ||
| const taken = parts.get(id); | ||
| if (taken === undefined) { | ||
| parts.set(id, sourceLayer); | ||
| } else if (taken !== sourceLayer) { | ||
| // Two different names, one id: the second's features would draw nowhere. Reported rather | ||
| // than dropped in silence — the Diagnostics panel is where a user can see it. | ||
| console.warn( | ||
| `PMTiles archive "${options.id}": source layer "${sourceLayer}" collides with "${taken}" and is not drawn.`, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Minor: the collision-detection loop (and its console.warn) runs before the options.tileType === "raster" check below, so a raster archive that happens to report a non-empty/colliding sourceLayers list (raster tiles never split, and the id math is meaningless for them) would still emit a "collides with... and is not drawn" warning even though nothing is actually dropped — the raster branch ignores sourceLayers for splitting purposes entirely. Low impact in practice since readPMTilesArchiveInfo/readRemotePMTilesInfo always return an empty sourceLayers for raster, but worth guarding (if (options.tileType !== "raster") { ...collision loop... }) so the diagnostic can't misfire for a hybrid/unusual archive.
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
Overall the shared-source refcounting logic ( |
8d0887d to
3606657
Compare
| if (stale.type !== "pmtiles" || stale.metadata.sourceId !== archiveId) continue; | ||
| if (ids.has(stale.id)) continue; | ||
| emptied.add(stale.groupId); | ||
| store.removeLayer(stale.id); |
There was a problem hiding this comment.
Possible reentrancy when an archive migrates id schemes while the control's subscriber is live.
This loop removes the old-scheme layer (e.g. asset-3) via store.removeLayer(stale.id) before the new split layers (asset-3-faults, asset-3-folds) are added a few lines below. When this runs through createPMTilesLayerAddHandler (maplibre-components.ts), the module-level subscriber set up in createPMTilesControl (maplibre-components.ts:3990) fires on this intermediate state transition too — at that point next contains neither the old nor the new layers for the archive, so pmtilesArchivesFullyRemoved reports the archive as fully gone even though it's mid-migration, and the subscriber calls pmtilesControl?.removeLayer(archiveId) back into the third-party control.
That re-entrant call happens while the control is still in the middle of reporting more source layers for that very archive (i.e. inside its own layeradd processing). Depending on what the control's removeLayer does (unregister the archive, abort in-flight requests, drop it from its own list), this could desync the control's panel from the store, or interfere with it continuing to report metadata for the archive — even though controlOwnedArchives.add(event.layerId) at the end of the add handler restores bookkeeping afterward.
This path isn't exercised by any test in this PR — the grouping/removal tests call addPMTilesArchive and the handlers directly, never through the real useAppStore.subscribe wired up in createPMTilesControl, so the interaction between this stale-layer removal and that subscriber isn't covered.
Confidence: medium (the concrete impact depends on the third-party control's removeLayer semantics, which aren't visible from this repo, but the reentrant call itself is real and reproducible from the store/subscriber code alone).
| // Only an external source can be shared — the derived ids below are this layer's alone — so the | ||
| // map is asked at most once, and only when a shareable source is actually up for removal. Walked | ||
| // layer by layer rather than read from `getStyle()`, which serializes the whole document. | ||
| const shareable = new Set(getExternalSourceIds(layer)); | ||
| let drawnSources: Set<string> | undefined; | ||
| const stillDrawn = (src: string): boolean => { | ||
| drawnSources ??= new Set( | ||
| map | ||
| .getLayersOrder() | ||
| .map((styleLayerId) => map.getLayer(styleLayerId)?.source) | ||
| .filter((source): source is string => typeof source === "string"), | ||
| ); | ||
| return drawnSources.has(src); | ||
| }; |
There was a problem hiding this comment.
Performance: this scan now runs for every removal of any layer with an external source, not just shared PMTiles archives.
shareable is populated from getExternalSourceIds(layer), which is non-empty for any layer kind that sets metadata.sourceId/sourceIds — MBTiles, vector-tile, Esri Wayback, 3D Tiles, etc. (many plugins set this). For all of those, stillDrawn now does an O(current style layers) walk (getLayersOrder() + getLayer() per id) on every single removal, even though most of these layer kinds don't actually share a source with siblings — only the new PMTiles-archive split case does.
Removing many such layers in one pass (e.g. deleting a project full of external-source layers, or dispose() in headless.ts) turns what was an O(1)-per-removal source cleanup into effectively O(n²) for n external-source layers, since each removal re-derives drawnSources from scratch (the drawnSources cache is scoped per removeLayerFromMap call, not shared across the batch).
Confidence: medium — likely fine in practice given typical layer counts, but worth confirming this doesn't show up as a regression on large projects/imports with many external layers.
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
Everything else — the refcounted source removal, archive splitting/dedup/collision handling, group lifecycle (create/join/prune), undo coalescing, and project save/reload — is thoroughly covered by the new test files and matches the implementation on inspection. |
3606657 to
e8290c3
Compare
| } else { | ||
| state.addLayerGroup( | ||
| name, | ||
| layers.map((layer) => layer.id), |
There was a problem hiding this comment.
Bug: when there's no existing group to fold into, this creates a new folder containing every layer in layers (old + new), not just added. The comment right above this (lines 62-64) explicitly describes the intended behavior — "the layer being added now starts a fresh one rather than being pulled back into the folder they emptied" — but the code re-groups everything.
Concretely: an archive is split into layers A and B and grouped. The user manually drags both A and B out of the folder (moveLayersToGroup([...], null)), so neither has a groupId. Later the PMTiles control reports a newly-discovered source layer C for the same archive (progressive metadata discovery is exactly what this module is built to handle). addPMTilesArchive runs again: A and B are known (→ updateLayer, which doesn't touch groupId, so they stay ungrouped), C is new (→ added = [C]). Since no layer among A/B/C currently has a groupId, existing is undefined, and the else branch creates a brand-new folder containing A, B, and C — silently reverting the user's explicit ungrouping of A and B.
Confidence: medium — this requires a specific sequence (split archive → user ungroups all siblings → a later progressive metadata read adds another source layer), but that sequence is exactly the scenario this module's other logic (the stale-shape replacement above) is designed around, and no test covers it.
| layers.map((layer) => layer.id), | |
| added, |
| } else if (taken !== sourceLayer) { | ||
| // Two different names, one id: the second's features would draw nowhere. Reported rather | ||
| // than dropped in silence — the Diagnostics panel is where a user can see it. | ||
| console.warn( |
There was a problem hiding this comment.
Minor/quality (low confidence): this warns on every call to createPMTilesArchiveLayers. The PMTiles control reports source layers progressively as metadata arrives (per the comments elsewhere in this PR about "a later read" adding source layers), so a genuine encoding collision in an archive will re-log this warning on every progressive layeradd event for that archive, not just once. Not incorrect, just potentially noisy in the console/Diagnostics panel for a long-loading archive.
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
Overall the refcounted-source removal logic ( |
e8290c3 to
3ac6ec9
Compare
| if (parts.size < 2) { | ||
| // Deduped here too, so one layer never carries the same source layer twice. | ||
| return [ | ||
| createPMTilesStoreLayer({ ...options, sourceLayers: [...new Set(options.sourceLayers)] }), | ||
| ]; |
There was a problem hiding this comment.
Bug (medium confidence): when an archive's source layers collide down to a single id (parts.size < 2) because two different names encode to the same id (not just an exact repeat), this fallback keeps both original names via [...new Set(options.sourceLayers)] instead of the collision-resolved parts map. That reproduces exactly the failure the console.warn above describes — a second, differently-named source layer sharing a derived native-layer id with the first — except here it's silently kept in source.sourceLayers and never actually rendered (the second ensureLayer call for the shared id is a no-op), whereas the split path a few lines down correctly drops the loser.
Example: sourceLayers: ["a/b", "a_2Fb"] — both collide to one id, so parts.size === 1, and this branch returns a single layer covering ["a/b", "a_2Fb"] even though only one of them will ever draw.
Using the already-deduped map instead would keep this path consistent with the split path:
| if (parts.size < 2) { | |
| // Deduped here too, so one layer never carries the same source layer twice. | |
| return [ | |
| createPMTilesStoreLayer({ ...options, sourceLayers: [...new Set(options.sourceLayers)] }), | |
| ]; | |
| if (parts.size < 2) { | |
| // Deduped here too, so one layer never carries the same source layer twice. | |
| return [createPMTilesStoreLayer({ ...options, sourceLayers: [...parts.values()] })]; | |
| } |
| @@ -1,5 +1,6 @@ | |||
| import assert from "node:assert/strict"; | |||
| import { describe, it } from "node:test"; | |||
| import type { GeoLibreLayer } from "../packages/core/src/types"; | |||
There was a problem hiding this comment.
Nit: GeoLibreLayer is imported but never used in this file — not caught by CI since the repo's ESLint config only enables the React Hooks rules (no no-unused-vars) and tsc isn't run with noUnusedLocals, but it's dead weight left over from editing.
| import type { GeoLibreLayer } from "../packages/core/src/types"; |
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
Overall the refactor (splitting a tiled archive into one layer per source layer, refcounting the shared MapLibre source on removal, and keeping the PMTiles control's ownership tracking correct across add/remove/reload) is carefully reasoned and well covered by the new tests, including the subtle "never momentarily layerless" and "shared source outlives siblings" cases. |
…control's layer ids
| // A sibling's folder, if any sibling is still in one: a user who dragged them all out has said | ||
| // this archive is not a folder any more. Where an id was reused, whatever was taken over counts | ||
| // as a sibling, so the two archives share a folder under whichever name got there first. | ||
| const existing = state.layers.find((item) => ids.has(item.id) && item.groupId)?.groupId; |
There was a problem hiding this comment.
Quality / edge case (low confidence): existing picks the group of the first layer found in state.layers that belongs to this archive's ids and has a groupId. If a user has manually dragged one sibling out of the archive's folder into a different group (leaving the rest in the original folder), a later re-add (e.g. reopening the panel, or a STAC/control refresh with a changed selection) will fold the newly-added layers into whichever group Array.prototype.find happens to hit first — not necessarily the folder most of the siblings are still in. This is a rare interaction (re-adding an archive whose siblings have been manually split across groups), so the impact is small, but the result is effectively non-deterministic from the user's perspective. Worth a comment noting the "first match wins" behavior is intentional, or picking the most common groupId among the survivors instead.
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
1 participant
Closes #2062
What
A vector PMTiles archive is added as a folder named after it, with one layer per source layer
inside. The Protomaps basemap arrives as
v4holdingwater,roads,buildings,earthand therest, each in the colour the control assigned it.
Nothing new in the panel: they are ordinary layers in an ordinary group, so visibility, opacity,
reordering, zoom-to, the Style panel and delete all work on them already.
A raster archive, or one with a single source layer, is added as one layer as before.
Why this shape
The first cut gave the layer a list of parts with their own toggles — a second, parallel way to
express visibility and styling. The maintainer's suggestion on #2062 was to use layer groups, which
already carry collapse, group visibility ANDed with each child, group opacity multiplied into each
child, and nesting. That deleted more code than it added and gave per-source-layer styling for free.
The part that needed care
The layers share one MapLibre source.
removeLayerFromMapremoved a layer's sources unconditionally,so deleting one source layer would have pulled the source out from under its siblings — with a delete
button now on every row, that is one click away. It now takes the surviving layers and keeps a source
while anything still draws from it.
Colours where an archive is still one layer
The STAC panel and the offline basemap extract build a single layer over all the source layers.
Those still paint each one in the colour the archive assigned (
assignedSourceLayerColor), whichwas already computed at add time and previously discarded after the first. A user restyling the
layer takes it back.
Tests
pmtiles-archive-layers.test.tscovers the expansion and the refcount, including that a sharedsource survives one sibling's removal and goes when the last one does.
layer-parts-every-path.test.tspins the assigned colours through the vector-tiles and MBTiles sync paths.
6770/6771 pass, tsc clean.