Add typed embed SDK and viewer preset - #1615
Open
giswqs wants to merge 18 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded a publishable ChangesEmbed SDK and viewer experience
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant HostPage
participant GeoLibreEmbedClient
participant useEmbedApi
participant LayerStore
participant MapCanvas
HostPage->>GeoLibreEmbedClient: connect to iframe
GeoLibreEmbedClient->>useEmbedApi: send versioned command
useEmbedApi->>LayerStore: update or list layers
useEmbedApi->>MapCanvas: read viewport or export image
LayerStore-->>useEmbedApi: return command result
MapCanvas-->>useEmbedApi: return viewport or image
useEmbedApi-->>GeoLibreEmbedClient: send acknowledgement with result
GeoLibreEmbedClient-->>HostPage: resolve request or dispatch event
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks 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 |
Contributor
🔍 GitHub Pages PR preview
|
Contributor
🔍 Cloudflare PR preview
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
- Preserve listen-only v1 hosts and bind acknowledgements to request versions. - Reject unsupported runtime layer types before adding them. - Use the shared supported-version list during parsing.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a publishable, dependency-free @geolibre/embed SDK for GeoLibre’s iframe postMessage API, bumps the embed protocol to v2 (while aiming to keep v1 compatibility), and adds a new read-only layout=viewer preset for embedded viewing experiences.
Changes:
- Add
@geolibre/embedworkspace package exporting typed commands/events and aconnect()client. - Extend the app-side embed protocol parsing/handling to v2 (layer visibility/listing, filters, viewport query, runtime layer add, PNG export) and update docs/tests accordingly.
- Add a
viewerlayout preset (read-only chrome, visibility-only legend) and supporting UI wiring.
Reviewed changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/layout-options.test.ts | Adds coverage for the new layout=viewer preset option mapping. |
| tests/embed-api.test.ts | Extends protocol parsing tests to cover v2 commands and version handling. |
| packages/map/src/layer-sync.ts | Applies the new transient embedFilter to MapLibre feature filters during sync. |
| packages/embed/tsconfig.json | Adds TS config for the new @geolibre/embed package. |
| packages/embed/tsconfig.build.json | Adds build TS config to emit JS + .d.ts into dist/. |
| packages/embed/src/index.ts | Implements the typed SDK surface (connect, commands, event typing). |
| packages/embed/package.json | Defines the publishable @geolibre/embed package metadata and build scripts. |
| packages/core/src/types.ts | Adds embedFilter?: unknown[] to GeoLibreLayer for embed-driven filtering. |
| packages/core/src/project.ts | Strips embedFilter on save so it remains transient/non-persistent. |
| package-lock.json | Wires @geolibre/embed into the workspace lockfile. |
| docs/user-guide/embedding.md | Documents layout=viewer, protocol v2, and recommends the new SDK usage. |
| apps/geolibre-desktop/src/lib/embed-api.ts | Updates protocol constants/types and parsing to accept v1 + v2 envelopes and new commands. |
| apps/geolibre-desktop/src/hooks/useLayoutOptions.ts | Introduces viewer layout option behavior (read-only chrome flags). |
| apps/geolibre-desktop/src/hooks/useEmbedApi.ts | Implements v2 command execution, ack.result, export image, runtime add layer, and v1/v2 event versioning. |
| apps/geolibre-desktop/src/components/panels/ViewerLayerPanel.tsx | Adds a minimal, read-only visibility legend for viewer mode. |
| apps/geolibre-desktop/src/components/layout/TopToolbar.tsx | Hides authoring/management menus when viewer is enabled. |
| apps/geolibre-desktop/src/components/layout/DesktopShell.tsx | Wires viewer mode into shell layout (panel suppression + viewer legend). |
| apps/geolibre-desktop/package.json | Ensures the embed SDK is built before desktop dev/build steps. |
| .github/workflows/publish-embed.yml | Adds a release-triggered workflow to publish @geolibre/embed to npm. |
Suppressed comments (1)
apps/geolibre-desktop/src/hooks/useEmbedApi.ts:374
- After emitting both v2 and v1
ready,hostVersionis set back to 2. A v1 host that only listens for events (and never sends a command) will then receive all subsequent events (projectLoaded,viewChanged, etc.) as v2 and ignore them, breaking existing v1 integrations.
return;
}
// Within the throttle window: schedule the trailing edge so the host still
// gets the final position of a drag.
if (trailingTimer !== null) return;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Honor addLayer insertion targets and expose keyboard focus in viewer layers. - Align nullable viewport bounds with runtime events. - Cover client origin checks, correlation, disconnects, and timeouts.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/geolibre-desktop/src/components/layout/TopToolbar.tsx (1)
1500-1740: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftGate all viewer mutation paths.
Viewer mode hides menus and panels, but it does not restrict the underlying actions. The full command list still reaches
useGlobalShortcutsandCommandPalette. The Controls menu also remains available. A viewer can open the command palette or use shortcuts to create, load, save, process, or add project data. The shell also keeps drag-and-drop import handlers active.
apps/geolibre-desktop/src/components/layout/TopToolbar.tsx#L1500-L1740: create a viewer allowlist for commands, shortcuts, and Controls actions. Keep only supported viewer actions such as navigation, basemap changes, place search, identify, and visibility controls.apps/geolibre-desktop/src/components/layout/DesktopShell.tsx#L1955-L1963: disable file drag-and-drop imports whenlayoutOptions.vieweris true.🤖 Prompt for 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. In `@apps/geolibre-desktop/src/components/layout/TopToolbar.tsx` around lines 1500 - 1740, Gate all viewer mutation paths: in apps/geolibre-desktop/src/components/layout/TopToolbar.tsx lines 1500-1740, define and apply a viewer allowlist for the command list, useGlobalShortcuts, CommandPalette, and ControlsMenu so viewers retain only navigation, basemap, place search, identify, and visibility actions; apps/geolibre-desktop/src/components/layout/DesktopShell.tsx lines 1955-1963 must disable drag-and-drop file imports when layoutOptions.viewer is true.
🤖 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 @.github/workflows/publish-embed.yml:
- Line 15: Update the actions/checkout@v6 step in the publish workflow to set
persist-credentials to false, ensuring checkout does not leave Git credentials
available to subsequent npm ci install scripts while preserving the existing npm
publish authentication.
- Around line 7-9: Move id-token: write from the workflow-level permissions
block into the publish job’s permissions block, while keeping contents: read at
the workflow level. Ensure only the publish job requests OIDC token access.
In `@apps/geolibre-desktop/src/components/layout/DesktopShell.tsx`:
- Around line 692-698: Move the geo-editor deactivation logic from the
standalone viewer effect into the project-restoration flow after
restoreProjectState completes, and retain the layoutOptions.viewer guard there.
Ensure the restoration path includes viewer mode so restored plugin state cannot
activate maplibre-gl-geo-editor in the read-only preset.
In `@apps/geolibre-desktop/src/components/panels/ViewerLayerPanel.tsx`:
- Around line 19-35: Add a visible focus-within ring or outline to the label
wrapping each checkbox in the layer control, while preserving the existing
styling and keyboard behavior. Update the label’s className in the layer
rendering block so focus on the sr-only input makes the active control visually
identifiable.
In `@apps/geolibre-desktop/src/lib/embed-api.ts`:
- Line 20: Update the version validation in parseEmbedRequest to check
membership in SUPPORTED_EMBED_API_VERSIONS rather than comparing against
hardcoded 1 and EMBED_API_VERSION values. Preserve rejection of unsupported
versions while keeping the check automatically aligned with future additions to
the supported-version constant.
- Around line 350-368: Extend the validation in the addLayer case to verify
optional spec fields before the unchecked AddLayerSpec cast: visible must be
boolean when present, opacity must be a finite number when present, style and
metadata must be records when present, and beforeId must be a string when
present. Also restrict type to the supported GeoLibre layer type values, while
preserving acceptance of omitted optional fields and the existing failure
response.
In `@docs/user-guide/embedding.md`:
- Around line 196-199: Update the message handler around the event.origin and
message.source checks to also require event.source === frame.contentWindow
before processing the payload, ensuring only messages from the intended iframe
are accepted.
- Around line 93-94: Update the connect call in the embedding example to narrow
document.querySelector("iframe") to a non-null HTMLIFrameElement before passing
it to connect, preserving the existing origin configuration and ensuring the
TypeScript example type-checks.
In `@packages/embed/src/index.ts`:
- Around line 110-117: Update send() to apply a per-request timeout after
registering each request in pending. When the timeout expires, remove that
requestId from pending and reject its promise with an appropriate timeout error;
ensure successful acknowledgements and disconnect cleanup cancel or otherwise
prevent the timeout from acting afterward.
In `@packages/map/src/layer-sync.ts`:
- Around line 179-181: Update the layer synchronization flow around the
embedFilter handling to apply filters before cluster inputs are created: use a
pre-filtered feature collection for the clustered GeoJSON/source path, and
filter the points supplied to Supercluster.load() on the tiled path. Keep
render-layer filtering for display, but ensure filtered features never enter
either clustering input.
In `@tests/embed-api.test.ts`:
- Around line 129-184: Add failure-path assertions to the “parseEmbedRequest: v2
commands” tests for setLayerVisibility, setFilter, and addLayer. Verify parsing
rejects a non-boolean visible value, an expression that is neither an array nor
null, and an addLayer spec with a missing or incorrectly typed optional field,
matching the validation behavior in parseEmbedRequest.
---
Outside diff comments:
In `@apps/geolibre-desktop/src/components/layout/TopToolbar.tsx`:
- Around line 1500-1740: Gate all viewer mutation paths: in
apps/geolibre-desktop/src/components/layout/TopToolbar.tsx lines 1500-1740,
define and apply a viewer allowlist for the command list, useGlobalShortcuts,
CommandPalette, and ControlsMenu so viewers retain only navigation, basemap,
place search, identify, and visibility actions;
apps/geolibre-desktop/src/components/layout/DesktopShell.tsx lines 1955-1963
must disable drag-and-drop file imports when layoutOptions.viewer is true.
🪄 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: 74f9539a-97be-4cf6-9782-fbbbbedbdebe
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (18)
.github/workflows/publish-embed.ymlapps/geolibre-desktop/package.jsonapps/geolibre-desktop/src/components/layout/DesktopShell.tsxapps/geolibre-desktop/src/components/layout/TopToolbar.tsxapps/geolibre-desktop/src/components/panels/ViewerLayerPanel.tsxapps/geolibre-desktop/src/hooks/useEmbedApi.tsapps/geolibre-desktop/src/hooks/useLayoutOptions.tsapps/geolibre-desktop/src/lib/embed-api.tsdocs/user-guide/embedding.mdpackages/core/src/project.tspackages/core/src/types.tspackages/embed/package.jsonpackages/embed/src/index.tspackages/embed/tsconfig.build.jsonpackages/embed/tsconfig.jsonpackages/map/src/layer-sync.tstests/embed-api.test.tstests/layout-options.test.ts
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 `@tests/embed-client.test.ts`:
- Around line 42-52: Extend the test around connect and the ready event to add
negative cases with a different event.source and a different data.source before
the valid event. Assert after each case that connect remains unsettled, then
keep the exact-origin event and final receive("ready", {}) as the successful
resolution path.
🪄 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: f7eb1866-e648-4880-bc5d-a68bf0900ec8
📒 Files selected for processing (5)
apps/geolibre-desktop/src/components/panels/ViewerLayerPanel.tsxapps/geolibre-desktop/src/hooks/useEmbedApi.tsapps/geolibre-desktop/src/lib/embed-api.tspackages/embed/src/index.tstests/embed-client.test.ts
Contributor
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
- Harden release permissions and skip already-published SDK versions. - Validate optional layer fields and add command response timeouts. - Keep viewer layers deterministic and SDK source hot-reloadable. - Expand protocol filtering and timeout tests.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/geolibre-desktop/src/components/layout/TopToolbar.tsx (1)
1500-1740: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winBlock authoring commands in viewer mode.
These guards hide menu triggers only.
commandsstill containsproject.*,add.*,proc.*, and plugin commands.useGlobalShortcutsregisters them, and Help can open the mountedCommandPalette. A viewer user can therefore open authoring flows through keyboard shortcuts or the palette.Use an explicit viewer-safe command allowlist for both
useGlobalShortcutsandCommandPalette. As per PR objectives,layout=viewermust hide authoring UI.🤖 Prompt for 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. In `@apps/geolibre-desktop/src/components/layout/TopToolbar.tsx` around lines 1500 - 1740, Viewer mode currently hides menu triggers but still exposes authoring commands through useGlobalShortcuts and CommandPalette. Add and apply an explicit viewer-safe command allowlist to both command consumers, excluding project, add-data, processing, and plugin authoring commands while preserving view, help, and other viewer-safe actions; ensure layout=viewer cannot invoke authoring flows via shortcuts or the palette.
♻️ Duplicate comments (1)
packages/map/src/layer-sync.ts (1)
179-181:⚠️ Potential issue | 🟠 MajorApply
embedFilterbefore building clustered data.
withFeatureFilterschanges only render-layer filters. The clustered GeoJSON source still receives the unfilteredlayer.geojson!, so MapLibre builds clusters from features thatembedFilterlater hides. Cluster bubbles andpoint_countcan therefore include excluded features. Pre-filter the clustered GeoJSON data and the tiled/Supercluster input, while retaining the render filter for unclustered features.#!/bin/bash set -euo pipefail rg -n -C 10 \ 'embedFilter|cluster: true|setData\(layer\.geojson|supercluster|point_count' \ packages/map/src🤖 Prompt for 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. In `@packages/map/src/layer-sync.ts` around lines 179 - 181, Apply layer.embedFilter to the feature collection before constructing clustered GeoJSON or passing data to Supercluster, so excluded features cannot affect clusters or point_count. Update the clustering/data-preparation flow near the existing embedFilter handling, while retaining the render-layer filter used for unclustered features and leaving non-clustered behavior unchanged.
🤖 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 `@apps/geolibre-desktop/src/hooks/useLayoutOptions.ts`:
- Around line 73-78: The layer panel visibility logic in useLayoutOptions must
force the panel visible in viewer mode unless panelsHidden is true, overriding
persisted layerPanelVisible settings; update tests/layout-options.test.ts lines
91-102 to cover viewer mode with layerPanelVisible false and assert the panel
remains enabled.
---
Outside diff comments:
In `@apps/geolibre-desktop/src/components/layout/TopToolbar.tsx`:
- Around line 1500-1740: Viewer mode currently hides menu triggers but still
exposes authoring commands through useGlobalShortcuts and CommandPalette. Add
and apply an explicit viewer-safe command allowlist to both command consumers,
excluding project, add-data, processing, and plugin authoring commands while
preserving view, help, and other viewer-safe actions; ensure layout=viewer
cannot invoke authoring flows via shortcuts or the palette.
---
Duplicate comments:
In `@packages/map/src/layer-sync.ts`:
- Around line 179-181: Apply layer.embedFilter to the feature collection before
constructing clustered GeoJSON or passing data to Supercluster, so excluded
features cannot affect clusters or point_count. Update the
clustering/data-preparation flow near the existing embedFilter handling, while
retaining the render-layer filter used for unclustered features and leaving
non-clustered behavior 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: 6d2fab38-080f-4662-a6b4-aa5824ba7bb4
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
.github/workflows/publish-embed.ymlapps/geolibre-desktop/package.jsonapps/geolibre-desktop/src/components/layout/DesktopShell.tsxapps/geolibre-desktop/src/components/layout/TopToolbar.tsxapps/geolibre-desktop/src/components/panels/ViewerLayerPanel.tsxapps/geolibre-desktop/src/hooks/useEmbedApi.tsapps/geolibre-desktop/src/hooks/useLayoutOptions.tsapps/geolibre-desktop/src/lib/embed-api.tsdocs/user-guide/embedding.mdpackages/core/src/project.tspackages/core/src/types.tspackages/embed/package.jsonpackages/embed/src/index.tspackages/embed/tsconfig.build.jsonpackages/embed/tsconfig.jsonpackages/map/src/layer-sync.tstests/embed-api.test.tstests/embed-client.test.tstests/layout-options.test.ts
Contributor
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
The |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/user-guide/embedding.md (1)
172-179: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winBroaden the documented
addLayersource requirements.
addLayercurrently accepts a non-emptysource.url, non-emptysource.dataas a URL string, non-emptysource.tiles[], ormetadata.originalUrlfor any layer type, plus inline object GeoJSON forgeojson/deckgl-viz. The current text omitssource.dataandmetadata.originalUrl, so host integrations can reject valid re-fetchable source shapes.🤖 Prompt for 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. In `@docs/user-guide/embedding.md` around lines 172 - 179, Update the addLayer source requirements in the documentation to include non-empty source.data URL strings and metadata.originalUrl for all layer types, while retaining the existing source.url, non-empty source.tiles, and inline GeoJSON rules for geojson/deckgl-viz layers.
🤖 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.
Outside diff comments:
In `@docs/user-guide/embedding.md`:
- Around line 172-179: Update the addLayer source requirements in the
documentation to include non-empty source.data URL strings and
metadata.originalUrl for all layer types, while retaining the existing
source.url, non-empty source.tiles, and inline GeoJSON rules for
geojson/deckgl-viz layers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 19cca261-e2fe-4877-b62e-a60bc067ac7b
📒 Files selected for processing (6)
apps/geolibre-desktop/src/components/layout/DesktopShell.tsxapps/geolibre-desktop/src/components/layout/TopToolbar.tsxapps/geolibre-desktop/src/hooks/useGlobalShortcuts.tsapps/geolibre-desktop/src/lib/embed-api.tsdocs/user-guide/embedding.mdtests/embed-api.test.ts
Contributor
|
Already covered by the inline comment posted above. That completes the review. Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
- The viewer preset's plugin guard now covers the Annotations plugin as well as the geo-editor. Annotations' activate() adds a full drawing toolbar via addMapControl, which lives on the map rather than in the React chrome the preset hides, so a project that saved it active handed a read-only embed a drawing toolbar on restore. - The two ids live in a new packages/plugins/src/viewer-plugins.ts as VIEWER_BLOCKED_PLUGIN_IDS, next to the plugins they name, so whoever adds the next authoring plugin is already in that package. Kept as a blocklist, not an allowlist: the viewer's job is to show the project as saved, so the display plugins a project relies on (layer control, basemaps, time slider, components) must keep working. - Exported ANNOTATIONS_PLUGIN_ID alongside the plugin, mirroring GEO_EDITOR_PLUGIN_ID, and added tests pinning the list to the ids the plugins actually register under — a rename would otherwise silently stop blocking them.
Contributor
|
All three inline comments posted. Now the final summary. Code reviewBugs
Security
Performance
Quality
CLAUDE.md
I traced the full protocol v1→v2 compatibility path, the |
- setFilter now stores the expression the style spec compiled, not the array that arrived. validateMapExpression sees a JSON round-trip, which rewrites `undefined`/`NaN`/`Infinity` — all of which survive a structured clone — as `null`, so a host sending `["==", ["get","x"], undefined]` was acked ok on a sanitized copy while layer-sync applied the unchecked original. - The viewer plugin guard re-runs after handleUrlParameters settles. It activates plugins asynchronously, so it can land after the synchronous pass following restoreProjectState. No blocked plugin registers a URL handler today, but covering every activation path is the point of the guard. - Mention embedFilter in applyExternalNativeFeatureFilters' docstring and its `@param layer`, matching the other two updated in this branch.
Contributor
Code reviewI reviewed the PR across the new I traced the plugin-manager activation/deactivation ordering ( Bugs: None found. Security: None found. The Performance: None found. Quality:
CLAUDE.md: No violations found — i18n via |
- Extract the embed API's envelope-targeting and version-pinning decisions out of the useEmbedApi closure into pure helpers in lib/embed-api.ts — embedEventTargets, embedEventVersions, embedRequestVersion — and cover them in tests/embed-api.test.ts: the broadcast-to-every-origin-until-the-host- speaks case, the scope-to-one-origin-after case, the wildcard collapse, the dual v2+v1 broadcast that keeps a listen-only v1 host working, the pin to the first request's version, and an ack answering in its own version whatever the host was pinned to. Previously only manual verification covered this branch.
Contributor
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
- Document the synchronous-activation invariant VIEWER_BLOCKED_PLUGIN_IDS depends on. PluginManager marks a plugin active before an async activate() resolves (watchAsyncActivation), and the viewer guard deactivates in that same tick — so an async blocked plugin would be torn down before its control existed and then mount anyway. Adding such a plugin means teaching the guard to await the pending activation, not just adding the id. A test asserts neither listed plugin declares activate as async. - Add a regression test for the transient per-feature filters being stripped on save. A leaked embedFilter would bake one host's runtime setFilter call into the shared .geolibre.json, where nothing in the UI would explain the filtered map to the next person who opened it.
Contributor
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
The embedding guide described the v2 protocol but not the client that is now the recommended way to speak it, and the rest of the docs still described the pre-v2 world. - embedding.md: a "The typed client" section with the install, `connect` options, the method table and what each resolves with, the rejection and timeout semantics, and when to call `disconnect()`. - sharing-embedding.md: `layout=viewer` in the parameter list with a note on when to reach for it over `maponly`, and a step that drives the embed from the host page with the client. - features.md: the viewer preset, the commands v2 added (layer toggle and listing, filters, viewport, runtime layer adds, PNG export), and the published client. - index.md: a `layout=viewer` example beside the `maponly` one. - architecture.md and CLAUDE.md: `@geolibre/embed` in the packages list, noting it is the one package published to npm.
- Add GeoAgent to VIEWER_BLOCKED_PLUGIN_IDS. Its control mounts via addMapControl and geoagent-layer-sync.ts calls addLayer/updateLayer/ removeLayer on the store, so a project saved with it active handed a layout=viewer embed an AI agent that can add and remove layers. My earlier sweep cleared it by grepping `app.` usage inside maplibre-geoagent.ts, which misses store writes made from a companion module — the docstring now says to follow what the control can do, not which file calls a mutator. - GeoAgent mounts behind a dynamic import, so the guard now awaits PluginManager.pendingActivation(id) before deactivating; deactivating during the pending window tore down nothing and let the mount land after. pendingActivation is a new public accessor over the existing private activationResults map. - Move the three blocked plugin ids into packages/plugins/src/plugin-ids.ts and have each plugin import its own id from there. maplibre-geoagent.ts pulls in maplibre-gl-earth-engine, which touches `window` at module load, so reading the id from the plugin module made viewer-plugins.ts (and its tests) browser-only. Same binding either side, so the ids cannot drift. SQL Workspace (maplibre-duckdb) was also raised but does not apply: it is not a registered plugin, so it cannot appear in projectPlugins.activePluginIds or be activated by restoreProjectState. It opens from the Add Data menu and the command palette, both already gated in viewer mode.
Comment on lines
+402
to
+414
| function hasRenderableEmbedSource(spec: Record<string, unknown>): boolean { | ||
| const source = isRecord(spec.source) ? spec.source : {}; | ||
| if ( | ||
| hasRestorableLayerSource({ source, metadata: isRecord(spec.metadata) ? spec.metadata : {} }) | ||
| ) { | ||
| return true; | ||
| } | ||
| if (typeof spec.type !== "string" || !INLINE_FEATURE_LAYER_TYPES.includes(spec.type)) { | ||
| return false; | ||
| } | ||
| // Inline GeoJSON is an object, on the spec's `geojson` or on `source.data`; | ||
| // the `hasRestorableLayerSource` check above only counts a string there. | ||
| return isRecord(spec.geojson) || isRecord(source.data); |
Contributor
There was a problem hiding this comment.
Bug: hasRenderableEmbedSource (via hasRestorableLayerSource) treats any layer type with a generic source.url/data/tiles/metadata.originalUrl as renderable, but layer-sync.ts's syncLayer only actually draws geojson, raster/wms/wmts/xyz, vector-tiles, mbtiles, video, image, or a genuine "external native" layer — the last category (pmtiles, cog, zarr, 3d-tiles, arcgis, lidar, gaussian-splat, deckgl-viz) is only dispatched when getExternalNativeLayerIds(layer) finds registration metadata (e.g. metadata.sourceKind === "pmtiles-url" + metadata.externalNativeLayer === true, checked by isPMTilesExternalLayer) that only each type's own control sets up via registerExternalNativeLayer — a bare addLayer/buildEmbedLayer call never populates it.
Concretely: tests/embed-api.test.ts ("accepts inline features on the spec or on the source for a data-backed type") asserts that addLayerSpec({ type: "pmtiles", source: { url: "pmtiles://https://x/y.pmtiles" } }) and addLayerSpec({ type: "cog", source: { url: "https://x/y.tif" } }) are accepted by parseEmbedRequest. Both will be pushed into the store and appear in listLayers(), but syncLayer has no case for pmtiles/cog outside the external-native path, and this spec never sets the metadata that path requires — so nothing is drawn on the map. This is exactly the "acked success, renders nothing" failure this function's own doc comment says it exists to prevent; it just doesn't cover control-painted types.
Confidence: high (traced through layer-sync.ts's syncLayer/isExternalNativeLayer/isPMTilesExternalLayer).
| * - `deckgl-viz`: `createDeckVizStoreLayer` keeps its features in `geojson` | ||
| * with a source holding only `{type, data: rows}`. | ||
| */ | ||
| const INLINE_FEATURE_LAYER_TYPES = ["geojson", "deckgl-viz"]; |
Contributor
There was a problem hiding this comment.
Bug: video layers can never be added through the addLayer embed command, even with a fully valid spec. syncVideoLayer in layer-sync.ts reads a video layer's media reference from source.urls (a plural array) plus source.coordinates — never source.url. hasRestorableLayerSource (used by hasRenderableEmbedSource two lines below) only checks source.url (singular)/data/tiles/metadata.originalUrl, and video isn't in INLINE_FEATURE_LAYER_TYPES either. So hasRenderableEmbedSource always returns false for type: "video", and any such spec is rejected with addLayer: a "video" layer needs a source with a url or tiles — regardless of how well-formed source.urls/source.coordinates are.
"video" is a valid LAYER_TYPES entry and passes the type check earlier in parseEmbedRequest, so this looks like an oversight rather than an intentional restriction (unlike the "control-painted" types, which are excluded on purpose per the comment above).
Confidence: medium-high.
Comment on lines
+188
to
+189
| const requireLayer = (layerId: string) => | ||
| requireEmbedLayer(useAppStore.getState().layers, layerId); |
Contributor
There was a problem hiding this comment.
Quality (minor): requireLayer is introduced here and used for setLayerVisibility/setFilter, but applyHighlight (above, unchanged) still does its own inline state.layers.find(...) + throw new Error(\No layer with id "${target.layerId}"`)instead of calling this helper — even thoughrequireEmbedLayer's doc comment in embed-api.tssays it's "shared bysetLayerVisibility, setFilter, and highlightFeature." Not a functional bug (the thrown message is identical either way), but worth switching applyHighlightto userequireLayer(target.layerId)` too so the three paths can't drift apart later.
Confidence: low-medium.
Contributor
|
All three inline comments posted successfully. Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
Drop NODE_AUTH_TOKEN/secrets.NPM_TOKEN in favour of the OIDC id-token, matching how opengeos/geolibre-rust publishes geolibre-wasm. The job already requested `id-token: write` and passed `--provenance`, so the missing pieces were the npm CLI version (Trusted Publishing needs >= 11.5.1, pinned rather than `npm@latest`, which briefly shipped a build with no bundled `sigstore`) and dropping the token env. npm has no PyPI-style pending publisher, so the trusted-publisher connection cannot be made until the package exists: 2.4.0 goes up from a maintainer's machine once, then the version gate here skips it and every later release publishes through this workflow. Also give the package the metadata npm renders on its page: a README with install, usage, the API tables, and the allowlist requirement, plus license, author, repository (with `directory`), homepage, bugs, and keywords. `npm pack --dry-run` confirms the README ships in the tarball.
GeoAgent joined the geometry editor and Annotations in the viewer's blocked list, so "map-drawing plugins" no longer describes the set.
Comment on lines
+222
to
+227
| case "addLayer": { | ||
| const state = useAppStore.getState(); | ||
| const layer = buildEmbedLayer(command.spec, state.layers); | ||
| state.addLayer(layer, command.spec.beforeId); | ||
| return layer.id; | ||
| } |
Contributor
There was a problem hiding this comment.
Bug (medium-high confidence): addLayer accepts specs for "control-painted" layer kinds (COG/raster, 3D Tiles, ArcGIS, LiDAR, Add-Vector-Layer-native, Overture, web-service, …) and acks ok: true with the new id, but it only calls state.addLayer(...) — it never runs the plugin restore pass those kinds need to actually appear on the map.
Those layer kinds are only rendered by their owning control (see raster-layer-sync.ts, maplibre-3d-tiles.ts, arcgis-i3s-tiles.ts, etc., which all stamp metadata.customLayerType + metadata.externalNativeLayer: true when the control creates the layer, and layer-sync.ts's applyExternalNativePaintBridge only forwards opacity/visibility to a bridge that must already be registered). parseEmbedRequest's hasRenderableEmbedSource only checks that a URL/tiles/inline-geojson is present — it doesn't gate on whether the type needs a control to render it, and the test at tests/embed-api.test.ts (addLayerSpec({ type: "cog", source: { url: "https://x/y.tif" } })) confirms a bare COG spec is accepted as valid.
This is the exact problem restoreLibraryLayer (apps/geolibre-desktop/src/lib/restore-library-layer.ts) was written to solve for the Layer Library's runtime "re-add" flow — re-adding a control-painted layer from My Data calls the matching RESTORE_BY_SOURCE_KIND pass so it actually renders. addLayer here doesn't do the equivalent, so a host that builds a spec with those metadata flags (e.g. by round-tripping an exported project layer) gets a silently inert layer: listLayers() reports it as visible: true, but nothing is painted.
Worth calling restoreLibraryLayer(layer, app) (or the matching RESTORE_BY_SOURCE_KIND pass) after state.addLayer(...) when the built layer is a control-painted kind, mirroring the Layer Library flow.
Contributor
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
@geolibre/embedclient with typed commands and eventslayout=viewerpreset with navigation controls and a visibility-only layer legendVerification
npm run buildnode --import tsx --test tests/embed-api.test.ts tests/layout-options.test.tspre-commit run --files <changed files>ready,listLayers, and correlatedack/resultmessagesFixes #1525
Summary by CodeRabbit