feat: add Overture Maps plugin under the Plugins menu#166
Conversation
Add maplibre-gl-overture-maps as a new Overture Maps plugin, listed after Esri Wayback in the Plugins menu and defaulting to the top-left control position. The panel opens on activation and remembers its release, visibility, and opacity across reposition and project reload. Theme the control, panel, and inspect popup from the app light/dark toggle by remapping the upstream --ovt-* tokens to the app theme tokens, overriding the package's prefers-color-scheme defaults. Depends on maplibre-gl-overture-maps 0.2.0, which re-applies its sources and layers after a basemap style change.
|
Caution Review failedPull request was closed or merged during review No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds maplibreOvertureMapsPlugin (control lifecycle, position, persistence, and bidirectional sync with Layers store), re-exports and registers the plugin in the desktop app, adds the overture stylesheet and dependency, maps Overture CSS tokens to the app theme, and exposes an app API method exportTextFile. ChangesOverture Maps Plugin Integration
sequenceDiagram
participant PluginsIndex as packages/plugins/src/index.ts
participant Desktop as apps/geolibre-desktop
participant PluginManager as PluginManager
participant OverturePlugin as maplibreOvertureMapsPlugin
participant OvertureControl as OvertureMapsControl
participant LayersStore as Layers Store
PluginsIndex->>Desktop: export plugin
Desktop->>PluginManager: registerAll([...maplibreOvertureMapsPlugin])
PluginManager->>OverturePlugin: activate(app)
OverturePlugin->>OvertureControl: attach control
OvertureControl->>OverturePlugin: statechange (visibility/opacity)
OverturePlugin->>LayersStore: reconcileStore (mirror layers)
LayersStore->>OverturePlugin: user edits -> reverseSync
Desktop->>OverturePlugin: createAppAPI.exportTextFile(filename, content)
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
Netlify preview: https://pr-166--opengeos.netlify.app |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Code reviewReviewed Bugs
PerformanceNothing notable. SecurityNothing notable. The plugin wraps a third-party MapLibre control with no direct DOM manipulation, URL construction, or user-supplied input paths in the new code. Quality
CLAUDE.mdNo Overall the implementation is well-structured and consistent with existing plugin patterns. The state-ordering issue in |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/plugins/src/plugins/maplibre-overture-maps.ts`:
- Around line 26-38: The getOvertureControlOptions function uses inconsistent
null-checks: change the release branch from a truthy check to the same non-null
check pattern used for collapsed and panelWidth so empty-string or
falsy-but-valid values are preserved; specifically, update the conditional that
references pendingState?.release in getOvertureControlOptions to use
pendingState?.release != null and spread { release: pendingState.release } when
present, keeping the rest of the return (OVERTURE_OPTIONS and position:
overturePosition) unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 32c96382-9473-4a9d-96ea-4c16c9edc0c4
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
apps/geolibre-desktop/src/hooks/usePlugins.tsapps/geolibre-desktop/src/index.cssapps/geolibre-desktop/src/main.tsxpackages/plugins/package.jsonpackages/plugins/src/index.tspackages/plugins/src/plugins/maplibre-overture-maps.ts
Mirror each visible Overture source layer (e.g. building, building_part) into the GeoLibre layer store as a single external-native custom layer so it appears in the Layers panel, combining its fill/line/circle native layers into one entry that matches the Overture Maps control structure. Sync is bidirectional: control changes update the panel, and panel visibility/opacity edits drive the control. Entries persist across visibility toggles, and the layers and plugin state round-trip through saved projects. Default to showing only the buildings theme on activation.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/plugins/src/plugins/maplibre-overture-maps.ts`:
- Around line 185-192: attachStoreSync currently calls useAppStore.subscribe(()
=> handleStoreChange(control)) causing handleStoreChange to run on every store
mutation; change this to subscribe only to layer changes (e.g., use a
selector-based subscription if supported) or update handleStoreChange to
early-return by shallow-comparing the incoming store.layers to a cached
previousLayers before doing work (keep reference to previousLayers inside the
module), and ensure you still respect the existing syncing guard; update
storeUnsubscribe accordingly and reference attachStoreSync,
useAppStore.subscribe, handleStoreChange, and storeUnsubscribe when making the
change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 46b65152-21bf-42e7-a4dc-669de4bd02c9
📒 Files selected for processing (1)
packages/plugins/src/plugins/maplibre-overture-maps.ts
Code reviewReviewed the new Bugs
Performance
Quality
SecurityNothing to flag. The plugin does not handle user-supplied strings in ways that could cause injection; all data flows through the upstream CLAUDE.mdNo CLAUDE.md file found in the repository; no guidelines to check against. Overall: the implementation is well-structured and the bidirectional sync logic (including the three-way |
- activate: attach store sync inside the control-creation guard and clean up on the addMapControl failure path (Claude) to avoid double-attach/leaks - setMapControlPosition: snapshot pending state before removing the control so a failed re-add keeps the latest state (Claude) - createOvertureControl: rely on setState for restoration instead of also spreading collapsed/panelWidth/release into the constructor (Claude); this also removes the inconsistent release null-check (CodeRabbit) - isOvertureMapsState: require a distinctive OvertureMapsState field so an unrelated stored object is not forwarded to setState (Claude) - store subscription: ignore updates that do not change the layers array (CodeRabbit) - reverseSync: forward opacity to the control even while a source layer is hidden so the value persists (Claude) - shouldUpdateStoreLayer: compare source/metadata with a key-order-insensitive stringify to avoid spurious updates for project-deserialized layers (Claude)
| if (!overtureControl) { | ||
| overtureControl = createOvertureControl(); | ||
| attachStoreSync(overtureControl); | ||
| } | ||
| const added = app.addMapControl(overtureControl, overturePosition); | ||
| if (!added) { | ||
| detachStoreSync(); | ||
| overtureControl = null; | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Bug (medium): store sync is attached before addMapControl succeeds
attachStoreSync is called before app.addMapControl, so if addMapControl returns false, the detachStoreSync() call on the failure path removes any Overture layers that handleStoreChange (invoked inside attachStoreSync) just added to the store — including project-restored layers from applyProjectState. This leaves the store in a worse state than when activate was entered.
The maplibre-gl-enviroatlas and maplibre-gl-national-map plugins attach the store sync only after addMapControl succeeds, which avoids this window. (The maplibre-gl-esri-wayback plugin uses the same ordering as this PR, so this is a pre-existing inconsistency, but it's still worth fixing here.)
| if (!overtureControl) { | |
| overtureControl = createOvertureControl(); | |
| attachStoreSync(overtureControl); | |
| } | |
| const added = app.addMapControl(overtureControl, overturePosition); | |
| if (!added) { | |
| detachStoreSync(); | |
| overtureControl = null; | |
| return false; | |
| } | |
| if (!overtureControl) { | |
| overtureControl = createOvertureControl(); | |
| } | |
| const added = app.addMapControl(overtureControl, overturePosition); | |
| if (!added) { | |
| overtureControl = null; | |
| return false; | |
| } | |
| attachStoreSync(overtureControl); |
| ): value is Partial<OvertureMapsState> { | ||
| if (!value || typeof value !== "object" || Array.isArray(value)) { | ||
| return false; | ||
| } | ||
| // Require at least one distinctive OvertureMapsState field so an unrelated | ||
| // object stored under this plugin's key is not forwarded to setState. | ||
| return ( | ||
| "themes" in value || "release" in value || "collapsed" in value | ||
| ); | ||
| } |
There was a problem hiding this comment.
Quality (low-medium): type guard is too permissive
The union check passes for any object that has any of the three keys. The field "collapsed" is common in serialised UI state, and "release" could appear in version objects. A stale or mismatched save entry under this plugin's project key would silently pass validation and get forwarded to setState, potentially corrupting the control's state.
Conjuncting the checks (or requiring at least the most discriminating field) makes the guard more robust:
| ): value is Partial<OvertureMapsState> { | |
| if (!value || typeof value !== "object" || Array.isArray(value)) { | |
| return false; | |
| } | |
| // Require at least one distinctive OvertureMapsState field so an unrelated | |
| // object stored under this plugin's key is not forwarded to setState. | |
| return ( | |
| "themes" in value || "release" in value || "collapsed" in value | |
| ); | |
| } | |
| return ( | |
| "themes" in value && ("release" in value || "collapsed" in value) | |
| ); |
| if (!storeLayer) { | ||
| // The entry was removed from the Layers panel: hide the source layer. | ||
| // Source layers we never mirrored (no last value) are left untouched. | ||
| if (lastControlValues.has(key) && layerState.visible) { | ||
| control.setLayerVisible(unit.theme, unit.sourceLayer, false); | ||
| } | ||
| lastControlValues.delete(key); | ||
| continue; |
There was a problem hiding this comment.
Quality (low): key deleted by reverseSync is unconditionally re-added by the reconcileStore that immediately follows
After the key is deleted here, reconcileStore (called next in handleStoreChange) reads visible: false from the control and existing = null from the store, so the !visible && !existing branch fires and re-inserts the key with {visible: false}. On every subsequent unrelated store mutation, reverseSync deletes the key again and reconcileStore re-adds it, resulting in repeated churn.
The intent of the deletion is to record "this unit was never actively mirrored", but since reconcileStore always re-records whatever the control currently reports, the semantics are never preserved for more than one cycle.
One fix is to skip the delete when the control already reports visible: false for the unit (nothing actually changed that needed recording). This keeps the existing entry and stops the cycle:
| if (!storeLayer) { | |
| // The entry was removed from the Layers panel: hide the source layer. | |
| // Source layers we never mirrored (no last value) are left untouched. | |
| if (lastControlValues.has(key) && layerState.visible) { | |
| control.setLayerVisible(unit.theme, unit.sourceLayer, false); | |
| } | |
| lastControlValues.delete(key); | |
| continue; | |
| if (!storeLayer) { | |
| // The entry was removed from the Layers panel: hide the source layer. | |
| // Source layers we never mirrored (no last value) are left untouched. | |
| if (lastControlValues.has(key) && layerState.visible) { | |
| control.setLayerVisible(unit.theme, unit.sourceLayer, false); | |
| lastControlValues.delete(key); | |
| } | |
| continue; | |
| } |
| classes outrank the upstream `.overture-control.ovt-theme-*` and | ||
| prefers-color-scheme blocks regardless of stylesheet order. */ | ||
| .overture-control.overture-control.overture-control, | ||
| .overture-control-panel.overture-control-panel.overture-control-panel, |
There was a problem hiding this comment.
Quality (low): triple-class specificity trick is fragile
Repeating the same class three times (.overture-control.overture-control.overture-control) is a well-known hack to boost specificity without !important. The comment explains the motivation, which is good. However, if the upstream package ever restructures its specificity (e.g. adds an extra class or switches to :where()/layers), this rule will silently win or lose unexpectedly, and the triple-repeat is not immediately obvious to readers.
A slightly more future-proof alternative is a :is() no-op (which is also a specificity bump trick) or just one extra app-level ancestor selector — but the real fix is upstream. For now, leaving a note that this must be re-evaluated on each maplibre-gl-overture-maps version bump would help maintainers.
Code reviewReviewed Bugs
Quality
CSS
Security / Performance / CLAUDE.mdNothing to flag. The |
The control's export button downloads GeoJSON via an anchor element, which does nothing in the Tauri webview (no download manager), so the button appeared broken in the desktop app while working on the web. - add exportTextFile to the plugin app API, implemented with the host's saveTextFileWithFallback (native save dialog under Tauri, download on web) - wire the Overture control's new onExport handler (maplibre-gl-overture-maps 0.3.0) to exportTextFile so saving goes through the host - bump maplibre-gl-overture-maps to ^0.3.0
| ...(app.exportTextFile | ||
| ? { | ||
| onExport: (filename, data) => | ||
| app.exportTextFile?.(filename, JSON.stringify(data)), |
There was a problem hiding this comment.
Redundant optional chain: this code path is only reached when app.exportTextFile is truthy (the surrounding conditional spread guarantees it), so the ?. is a no-op. Use a plain call instead.
| app.exportTextFile?.(filename, JSON.stringify(data)), | |
| app.exportTextFile(filename, JSON.stringify(data)), |
| function attachStoreSync(control: OvertureMapsControl): void { | ||
| controlEventHandler = () => handleControlEvent(control); | ||
| control.on("statechange", controlEventHandler); | ||
| // Only react to layer changes; the store also updates on unrelated mutations | ||
| // (basemap, UI state) that cannot affect the Overture mirror. | ||
| storeUnsubscribe = useAppStore.subscribe((state, prev) => { | ||
| if (state.layers === prev.layers) return; | ||
| handleStoreChange(control); | ||
| }); | ||
| // Mirror the control's current source layers and adopt any layers restored | ||
| // from a project, pushing their state back into the control. | ||
| handleStoreChange(control); |
There was a problem hiding this comment.
Potential lost state on first activation with a pre-loaded project.
attachStoreSync finishes by calling handleStoreChange(control), which calls reverseSync and in turn control.setLayerVisible / control.setLayerOpacity. At this point in activate (line 91 precedes line 93), the control hasn't been added to the map yet via app.addMapControl.
If OvertureMapsControl.setLayerVisible / setLayerOpacity only buffer state changes when the control has a live map reference (i.e. after onAdd), those calls silently do nothing. Any project-restored visibility/opacity that differs from the control's initial state would then be overwritten by reconcileStore reading back the (unchanged) initial state.
The ordering in activate is:
91: attachStoreSync(overtureControl); // ← handleStoreChange fires here
93: app.addMapControl(overtureControl, …); // ← onAdd fires here
Consider calling handleStoreChange after addMapControl succeeds, or defer the initial sync via a statechange event once the control has been added and has settled. Compare with how syncCurrentWaybackLayer / syncPersistentWaybackLayers are called inside the setTimeout in maplibre-esri-wayback.ts — after the control is on the map.
| exportTextFile: (filename: string, content: string) => { | ||
| void saveTextFileWithFallback(content, { | ||
| defaultName: filename, | ||
| filters: [{ name: "GeoJSON", extensions: ["geojson", "json"] }], | ||
| browserTypes: [ | ||
| { | ||
| description: "GeoJSON", | ||
| accept: { "application/geo+json": [".geojson", ".json"] }, | ||
| }, | ||
| ], | ||
| mimeType: "application/geo+json", | ||
| }).catch((error) => { | ||
| console.error(`Could not export ${filename}.`, error); | ||
| }); | ||
| }, |
There was a problem hiding this comment.
The exportTextFile API in GeoLibreAppAPI is intentionally generic (filename: string, content: string), but this implementation hardcodes GeoJSON MIME type and file-extension filters for every call regardless of the actual content type. That works fine today because the only caller is the Overture Maps export, but if a future plugin uses this API to export a non-GeoJSON format (e.g. GPX, CSV), the save dialog will show the wrong type hint.
Consider threading the MIME type through the API:
exportTextFile?: (filename: string, content: string, mimeType?: string) => void;and deriving the filters from mimeType in this implementation. Alternatively, document that the API is currently GeoJSON-only.
| if (shouldUpdateStoreLayer(existing, nextLayer)) { | ||
| store.updateLayer(id, { | ||
| name: nextLayer.name, | ||
| type: nextLayer.type, | ||
| source: nextLayer.source, | ||
| sourcePath: nextLayer.sourcePath, | ||
| metadata: nextLayer.metadata, | ||
| }); | ||
| } | ||
| // Push opacity/visibility only when the control changed them, so a value | ||
| // set through the Layers panel is not reverted by an unrelated event. | ||
| if (last && opacity !== last.opacity && opacity !== existing.opacity) { | ||
| store.updateLayer(id, { opacity }); | ||
| } | ||
| if (last && visible !== last.visible && visible !== existing.visible) { | ||
| store.updateLayer(id, { visible }); | ||
| } | ||
| } | ||
| lastControlValues.set(unitKey(unit), { visible, opacity }); |
There was a problem hiding this comment.
Nits (grouped):
-
Up to three separate
store.updateLayercalls per layer per sync pass. Metadata, opacity, and visibility are each dispatched independently. React should batch these in practice, but a single composite update would be cleaner and guarantee one re-render:const patch: Partial<GeoLibreLayer> = { name: nextLayer.name, type: nextLayer.type, source: nextLayer.source, sourcePath: nextLayer.sourcePath, metadata: nextLayer.metadata, }; if (last && opacity !== last.opacity && opacity !== existing.opacity) patch.opacity = opacity; if (last && visible !== last.visible && visible !== existing.visible) patch.visible = visible; store.updateLayer(id, patch);
-
existingis stale after the firstupdateLayercall. The opacity/visibility delta check on lines 283–288 readsexisting.opacity/existing.visiblefrom the snapshot taken before the metadata update. This is intentional (it must compare against the user-set value, not the just-written one), but worth a brief comment so the next reader doesn't "fix" it. -
Plugin
versionfield is"0.2.0"but the installed package is0.3.0(lock file). If this field tracks the wrapper version, that's fine — but the PR description mentions^0.2.0whilepackage.jsonactually pins^0.3.0, so the description appears outdated.
Code reviewOverall the implementation is solid and follows the existing plugin patterns well. The bidirectional store sync design is thoughtful, the CSS specificity workaround is correctly documented, and the Tauri export fallback is a clean abstraction. Four findings below, one medium-confidence bug and three quality items. Bugs
Quality
Security / PerformanceNothing worth raising. The |
1 participant
Summary
Adds maplibre-gl-overture-maps as a new Overture Maps plugin. It visualizes the six Overture Maps PMTiles themes (addresses, base, buildings, divisions, places, transportation) with per-theme visibility, opacity, styling, and feature inspection.
Theming
The upstream control themes itself from
prefers-color-scheme(the OS theme), so a light OS would keep the panel light even in app dark mode. This remaps the upstream--ovt-*tokens to the app theme tokens (scoped, high-specificity, with a.darkoverride andcolor-scheme), keying the control, panel, and inspect popup to the app's light/dark toggle. This follows the same pattern already used for the FEMA / NASA / EnviroAtlas / National Map panels.Dependency
Depends on
maplibre-gl-overture-maps@^0.2.0. That release adds astyle.loadhandler so the control re-applies its sources and layers after a basemap change (map.setStyle()otherwise wipes them). See opengeos/maplibre-gl-overture-maps#4.Testing
npm run build(tsc + vite) andpre-commit run --all-filespass.npm run test:frontend(113 tests) passes.Summary by CodeRabbit
New Features
Behavior
Style