feat(browser): save a configured layer and re-add it from My Data - #1564
Merged
Conversation
Add a Layer Library so a fully configured layer can be saved once and re-added to any later project in one click, closing the gap the existing libraries left: styles, services, views, and connections all persist across projects, but the layer itself did not. - `packages/core/src/layer-library.ts` follows `style-library.ts`: it captures a layer's source spec, type, full style (labels included), opacity, metadata, joins, virtual fields, and attribute form, plans how to re-add one, normalizes untrusted input, and reads/writes the shareable JSON bundle. Project-specific placement (id, groupId, beforeId) is deliberately not captured. - Entries store the **source specification, not the data**, so the library stays small and an entry always reflects its source's current contents. A layer whose features exist only in memory or in a local file embeds them behind a 5 MB per-entry cap; over the cap, a local file degrades to a path-only entry that only a filesystem-capable host can re-add (badged "desktop" in the tree). - Control-painted layers (Add Vector Layer, deck.gl rasters, 3D Tiles, LiDAR, Planetary Computer) are drawn by their plugin's restore pass, not by the store sync, so their features go in `metadata.embeddedGeoJSON` (the same field the project Embed/Share flow writes) and a re-add runs that pass via `restore-library-layer.ts`. Without it the layer would list in the Layers panel and draw nothing. - Entry points: Layers panel → layer actions → **Save to My Data**, and a **My Data** root node in the Browser panel above Services, sharing the tree's keyboard navigation. Rows rename in place and delete; the section header imports and exports the library as a JSON bundle. - The library is an app-level store slice persisted to IndexedDB (`layer-library-store.ts` + `useLayerLibraryPersistence`), outside the project lifecycle and outside undo history, with the display order stored alongside each record since `getAll()` returns key order. - `LayerType` is now derived from a new runtime `LAYER_TYPES` array so an imported bundle's layer type can be validated without the two drifting. Fixes #1520
|
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:
📝 WalkthroughWalkthroughAdds a persistent Layer Library for saving configured layers, restoring them in projects, managing entries through the Browser panel, and sharing them through JSON import/export. The implementation includes IndexedDB persistence, source and embedded-feature handling, restoration passes, and validation tests. ChangesLayer Library contracts and serialization
Layer Library state and IndexedDB persistence
Layer saving and restoration integration
My Data browser workflow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant LayerPanel
participant AppState
participant IndexedDB
participant BrowserPanel
participant BrowserTreeNode
LayerPanel->>AppState: saveLayerLibraryEntry
AppState->>IndexedDB: persist layer library
BrowserTreeNode->>BrowserPanel: activate library-layer
BrowserPanel->>AppState: lookup and plan library entry
BrowserPanel->>AppState: add and restore layer
Suggested reviewers: 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
🔍 Cloudflare PR preview
|
Contributor
🔍 GitHub Pages PR preview
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
|
All three inline comments posted. Now the final summary. Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
- Gate "Save to My Data" on whether the layer can actually be re-rendered, not just on having something to re-add from. `canSaveLayerToLibrary` now takes a `canRestoreControlPainted` predicate and refuses a control-painted layer without it, and the desktop app supplies `canRestoreLibraryLayer` (backed by the same restore-pass table). Previously any layer with a re-fetchable URL or a local path was offered, so control-painted kinds with no restore route (`cog-url`, `zarr-url`, `flatgeobuf-url`, `splatting-url`, `earth-engine-raster`, `duckdb-query`, …) saved fine and then re-added unrendered — the exact failure `restore-library-layer.ts` exists to avoid. - Give that gate a single source of truth: the `metadata.customLayerType` check layer-sync's dispatch branches on moves into core as `controlRendersLayer`, and layer-sync's `isExternalCustomLayer` delegates to it. Adding a kind to `RESTORE_BY_SOURCE_KIND` now also makes it saveable, so the two cannot drift. - Guard the Browser tree's rename editor with a ref instead of state. Enter and Escape both clear the parent's `renamingId`, unmounting the editor and firing a trailing blur handled by the pre-keypress closure — a `useState` flag is discarded with the unmounting component, so Escape committed the abandoned draft and Enter committed twice. Mirrors `handledRef` / `suppressBlurCommitRef` in LayerPanel, which exist for this same reason. - Hold the busy flag across a library add and await the restore pass, so the row spins through a slow re-ingest and a second click mid-restore cannot add the layer twice. Every other `activate` branch already did this.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/components/panels/BrowserTreeNode.tsx`:
- Line 130: Update the rename editor container in BrowserTreeNode to use the
logical paddingInlineStart inset instead of physical paddingLeft, preserving its
current indentation in LTR and aligning it correctly in RTL. Also update the
corresponding existing row indentation styles in the same component where needed
for consistent logical-direction behavior.
In `@apps/geolibre-desktop/src/components/panels/LayerPanel.tsx`:
- Around line 1037-1076: Update handleSaveToLibrary to re-read the latest layer
from the store after the await to materialize features, then pass that refreshed
layer to captureLayerLibraryEntry while preserving the materialized features
from the original layer. Follow the existing post-await refresh pattern used by
handleImportStyle, handleSaveEditsToSource, and handleRefreshLayer.
In `@apps/geolibre-desktop/src/lib/restore-library-layer.ts`:
- Around line 40-46: Add dedicated tests for canRestoreLibraryLayer and
restoreLibraryLayer in restore-library-layer.ts. Cover a RESTORE_BY_SOURCE_KIND
control-painted source kind, an external-native control-painted layer outside
the map, and an external-native GeoLibre-rendered layer, asserting each follows
the expected restore gating and dispatch behavior.
In `@packages/core/src/layer-library.ts`:
- Around line 395-411: Update normalizeLayerLibraryEntries() to validate members
before cloning or retaining candidate.joins, candidate.virtualFields, and
candidate.attributeForm. Preserve only joins containing id, joinLayerId,
targetField, and joinField; virtual fields containing id, name, and expression;
and attribute-form configs whose fields array has a field key on every member.
Keep the existing structural gates while dropping malformed definitions.
🪄 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: 5bed4205-81c0-4217-acb9-0a38e86ee319
📒 Files selected for processing (21)
apps/geolibre-desktop/src/App.tsxapps/geolibre-desktop/src/components/panels/BrowserPanel.tsxapps/geolibre-desktop/src/components/panels/BrowserTreeNode.tsxapps/geolibre-desktop/src/components/panels/LayerPanel.tsxapps/geolibre-desktop/src/hooks/useBrowserTree.tsapps/geolibre-desktop/src/hooks/useLayerLibraryPersistence.tsapps/geolibre-desktop/src/i18n/locales/en.jsonapps/geolibre-desktop/src/lib/browser-tree.tsapps/geolibre-desktop/src/lib/layer-library-store.tsapps/geolibre-desktop/src/lib/restore-library-layer.tsdocs/features.mdpackages/core/src/external-native-paint.tspackages/core/src/index.tspackages/core/src/layer-library.tspackages/core/src/store.tspackages/core/src/types.tspackages/map/src/layer-sync.tspackages/plugins/src/index.tstests/browser-tree.test.tstests/layer-library-store.test.tstests/layer-library.test.ts
Contributor
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
- Require `metadata.localFileReloadable` before treating `sourcePath` as a re-readable path. A browser-picked Add Vector Layer file has no path but `createVectorStoreLayer` still records the bare file *name* there, so an oversized one degraded into a `needsLocalFile` entry holding a filename no host could open — a false "Saved" for a permanently broken entry. It now reports "too large" instead, and a saved entry no longer carries the useless name at all. - Validate a persisted `sourcePath` (absolute, no `..` traversal) on capture, on import, and in `planLayerLibraryAdd`. A bundle is shareable, so a crafted `needsLocalFile` entry could otherwise aim `onAddFilePath` at any file on the importing user's disk — the analogous project-restore paths already re-validate client-side before calling into Tauri. `isAbsoluteLocalPath` moves into core as `isAbsoluteFilesystemPath` so both sides share one rule. - Carry the entry's style, opacity, joins, virtual fields, and attribute form on the `local-file` plan and apply them to the layer the file import produces. That import builds a default-styled layer, so a degraded entry previously came back unstyled, contradicting "save a fully configured layer". Joins and virtual fields go through their own store actions so the derived columns are materialized rather than stored inert. - Drop join, virtual-field, and attribute-form members that lack the fields their engine needs, instead of accepting them on shape alone: a join with no `joinLayerId`/`joinField` resolves to nothing and would sit in the Joins UI as a permanently broken definition. A block with no surviving member is omitted. - Capture from the current layer after awaiting the vector-control materialize, so a concurrent style/opacity/join edit is not lost to the pre-await closure (mirrors handleImportStyle / handleSaveEditsToSource). - Switch the Browser tree's inline indents to `paddingInlineStart` so they sit on the reading-direction side in RTL locales, per the project's logical- property convention. Applied to every row in the component, not just the new rename editor, so the editor still lines up with the row it replaces.
Contributor
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
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 (2)
apps/geolibre-desktop/src/components/panels/BrowserPanel.tsx (1)
624-627: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRestore tree focus after keyboard rename completion.
Enter/Escape clears
renamingIdand unmounts the focused input, leaving focus out of the tree. Restore focus to the renamed row after keyboard commit/cancel; preserve normal click-away focus by passing a keyboard-finish flag fromRenameRow.Also applies to: 742-745
🤖 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/panels/BrowserPanel.tsx` around lines 624 - 627, Update commitRename and the corresponding cancel path to accept a keyboard-finish flag from RenameRow, and when the rename ends via Enter or Escape, restore focus to the renamed tree row after clearing renamingId; leave click-away completion unchanged so it preserves normal focus behavior.packages/core/src/layer-library.ts (1)
328-346: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueConsider re-validating
entry.sourcePathin the default "layer" plan too.
layerLibraryEntryNeedsLocalFilere-validates the path withisReReadablePathbefore routing to the"local-file"plan, but the fallback"layer"plan at Line 339 spreadsentry.sourcePath(and the clonedentry.metadata, which can carrylocalFileReloadable: true) with no equivalent check. In the two real entry-creation paths (captureLayerLibraryEntryvialayerLocalPath, andnormalizeLayerLibraryEntriesvia the sameisReReadablePathgate)sourcePathis always pre-validated before reaching here, so this isn't reachable today — but it's a one-line gap in the "validate everywhere a persisted path flows toward a read" invariant this file otherwise enforces consistently (seeisReReadablePath's own doc comment: "Applied to captured and imported entries").♻️ Optional hardening
- ...(entry.sourcePath ? { sourcePath: entry.sourcePath } : {}), + ...(entry.sourcePath && isReReadablePath(entry.sourcePath) ? { sourcePath: entry.sourcePath } : {}),🤖 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/core/src/layer-library.ts` around lines 328 - 346, Re-validate entry.sourcePath in the default layer-plan construction before including it in the returned layer, using the existing isReReadablePath check. Preserve sourcePath only when it passes validation, and ensure metadata cannot retain localFileReloadable: true for an invalid path.
🤖 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/components/panels/BrowserPanel.tsx`:
- Around line 490-510: The file-import flow should use the created layer ID
returned by onAddFilePath instead of diffing global layer IDs. Update the shell
callback contract and its implementation to return the imported layer’s ID, then
in the import handling block use that ID directly with updateLayer,
setLayerJoins, and setLayerVirtualFields while preserving the existing error
handling.
---
Outside diff comments:
In `@apps/geolibre-desktop/src/components/panels/BrowserPanel.tsx`:
- Around line 624-627: Update commitRename and the corresponding cancel path to
accept a keyboard-finish flag from RenameRow, and when the rename ends via Enter
or Escape, restore focus to the renamed tree row after clearing renamingId;
leave click-away completion unchanged so it preserves normal focus behavior.
In `@packages/core/src/layer-library.ts`:
- Around line 328-346: Re-validate entry.sourcePath in the default layer-plan
construction before including it in the returned layer, using the existing
isReReadablePath check. Preserve sourcePath only when it passes validation, and
ensure metadata cannot retain localFileReloadable: true for an invalid 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: 052953c4-ec76-4728-a3c1-c0b468497b75
📒 Files selected for processing (7)
apps/geolibre-desktop/src/components/panels/BrowserPanel.tsxapps/geolibre-desktop/src/components/panels/BrowserTreeNode.tsxapps/geolibre-desktop/src/components/panels/LayerPanel.tsxapps/geolibre-desktop/src/lib/tauri-io.tspackages/core/src/layer-library.tspackages/core/src/paths.tstests/layer-library.test.ts
- Recognize a raster/COG's local path so a locally-opened one can be saved. The raster control records the absolute path in `metadata.localFilePath` (what `restoreRasterLayers` re-reads) and keeps only the basename in `sourcePath`, and it never sets `localFileReloadable` — so requiring that flag alone made every locally-opened COG unsaveable, despite `RESTORE_BY_SOURCE_KIND` already wiring its restore pass. `layerLocalPath` now honors both conventions, each re-validated. - Redact `source.requestHeaders` from the exported bundle. An authenticated 3D Tiles layer's bearer token is deliberately persisted into project files, but Export is pitched as one-click team sharing, which is a far more direct exposure path. The token stays in the local IndexedDB entry so re-adding on this machine still works, and only the bundle drops it — the recipient re-enters their own credentials. - Strip the transient metadata keys in `normalizeLayerLibraryEntries` too, not only on capture. That path runs on every startup load, so a stale `resolvedUrl` on an older record or an imported bundle would otherwise keep replaying a dev-server proxy rewrite instead of being cleaned up. - Identify the layer a `local-file` re-add produced by matching the path it was imported from, rather than requiring exactly one new store id. One file can legitimately yield several layers (a KML adds placemarks plus ground overlays and models) and an unrelated concurrent add would previously make the diff ambiguous and silently skip the config patch. Falls back to a lone new layer when no layer records a path, and still patches nothing rather than the wrong layer.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/components/panels/BrowserPanel.tsx`:
- Around line 509-519: Update the layer restoration flow around onAddFilePath
and the added-layer lookup so configuration is never applied to an arbitrary
same-path output. Extend the import/library contract to preserve a per-output
discriminator or return all created layer descriptors, use it to identify the
saved layer uniquely, and surface an error when no unique target can be
determined; do not fall back to the first matching output.
In `@packages/core/src/layer-library.ts`:
- Around line 491-495: Update the metadata normalization in the layer
restoration flow to validate metadata.localFilePath against the already
validated sourcePath. Remove an invalid or conflicting localFilePath, or
overwrite it with sourcePath, so raster restoration cannot read from an
untrusted path; preserve the existing TRANSIENT_METADATA_KEYS cleanup.
🪄 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: 8a6d4f55-9346-4c32-9ccf-a609c5043d6d
📒 Files selected for processing (3)
apps/geolibre-desktop/src/components/panels/BrowserPanel.tsxpackages/core/src/layer-library.tstests/layer-library.test.ts
Contributor
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
- Validate `metadata.localFilePath` during normalization. Teaching `layerLocalPath` to read that field (previous commit) gave an entry a second path, and normalization was copying it verbatim — so a shared bundle could keep a clean-looking `sourcePath` while pointing `restoreRasterLayers` at any file on the importing user's disk. It now gets the same absolute-path, no-traversal check as `sourcePath`, and is dropped when it fails. - Count that path in the "anything to re-add from" gate. A local raster's source has no URL and it has no features, so without this the entry the previous commit made saveable would have been discarded on the next load. - Require a unique path match before reapplying a `local-file` entry's saved configuration, and say so inline when there isn't one. One file can produce siblings that share a path (a KML's placemarks plus its ground overlays), and patching whichever appeared first would style the wrong one; leaving the user to notice their styling silently did not return was the other half of the problem.
- Redact credentials embedded in source URLs on export, not just
`source.requestHeaders`. Plenty of tile and web services authenticate in the
URL — an XYZ template with `?api_key=`, a Google `key=`, a Mapbox
`access_token=`, an S3 presigned link, an Azure SAS token — so the previous
redaction still shipped a key in a shared bundle. `source.url`,
`source.tiles`, and `metadata.originalUrl` now have known credential
parameters stripped, everything else preserved (including the `{z}/{x}/{y}`
placeholders, which is why the query is split by hand rather than through
URLSearchParams). The in-memory entry keeps its credential so re-adding
locally still works.
- Key the restore dispatch off constants the plugins export instead of
hand-typed literals. `PLANETARY_COMPUTER_SOURCE_KIND`,
`THREE_D_TILES_SOURCE_KIND`, and `LIDAR_SOURCE_KIND` are now defined next to
the `sourceKind` each plugin writes and used at both the write site and its
own read guard, so a rename cannot silently stop the table matching — the
failure mode this repo's "mirror" convention exists to prevent, and a quiet
one here ("Save to My Data" just stops appearing, or an entry re-adds
unrendered).
Contributor
|
All 8 inline comments posted successfully. Now the final summary. Code reviewBugs
Security
Quality
Accessibility
CLAUDE.md adherence Areas checked with no issues: the |
Core (`layer-library.ts`): - Rewind a resolved XYZ endpoint to `metadata.originalUrl` on capture, the way `prepareLayerForSave` does for project files. Stripping `metadata.resolvedUrl` alone left the session-specific endpoint sitting in `source.tiles`/`source.url`, so an entry would replay a stale host forever instead of re-resolving. - Enforce the per-entry size cap and a new `MAX_LAYER_LIBRARY_ENTRIES` count cap during normalization, not only on capture. A bundle is produced elsewhere, so import could otherwise push unbounded embedded features (and unbounded entries) into IndexedDB — the outcome the cap exists to prevent. - Measure that cap in UTF-8 bytes via TextEncoder rather than `JSON.stringify().length`. CJK/Arabic/Cyrillic text is one UTF-16 unit but 2-3 bytes per character, so the length check passed entries at up to 3x the cap. - Redact credentials in a URL fragment (`#access_token=…`, the same `name=value` shape) and `user:pass@` userinfo, not only query parameters. A plain `#anchor` fragment is preserved. App: - Correct `restoreLibraryLayer`'s contract: four of the five restore passes are `(app) => void` that fire their real work into a floating async IIFE, so awaiting them spans only the dispatch. The doc comment claimed "dispatched and settled" and the busy-flag comment implied full coverage; both now say what actually happens. - Restore focus to the tree row after a rename commits or cancels. The focused input unmounts, which dropped a keyboard user's focus to <body> and lost their place in the tree. - Guard `handleSaveToLibrary` against re-entrancy, so a repeat click during the vector-control materialize cannot write two entries under two generated ids. - Report when a `local-file` re-add adds no layers at all, which previously fell between both branches and no-oped silently. Stop shadowing the outer `entry` in the layer-diff closures.
Contributor
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
- Tell the user when a re-added layer's saved joins cannot resolve. A `LayerJoin` points at another layer by its **project-local id**, so a join travels only as far as the project it was captured in — and `applyLayerJoins` drops an unresolved one quietly, which is right at runtime but silent for a feature whose whole point is re-adding elsewhere. New `unresolvedLayerLibraryJoins` is checked before the add (so the layer's own fresh id cannot count as a match) and the panel reports the count. Kept capturing joins rather than dropping them: they do resolve when the join source is present — re-adding into the project the entry came from, or into one restored from the same saved project — and the issue asks for joins explicitly. The failure is now explained instead of invisible. - Split the over-cap failure in two. `too-large` was reported whether or not any features were embedded, so a layer with a perfectly good source that simply has a huge rule-based style was told "this layer's features are its only copy and are too large" — pointing at the wrong thing entirely. Now `features-too-large` and `config-too-large`, with their own messages. - Enforce `MAX_LAYER_LIBRARY_ENTRIES` on every store write, not only when normalizing untrusted input. Repeated saves or importing several bundles over time could otherwise grow the library past the cap its own doc comment promises. Mirrors `writeBrowserFavorites`, which slices to `MAX_FAVORITES` on each write; a new entry goes to the front, so at the cap the oldest falls off.
Contributor
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
Overall this is an unusually well-tested and carefully-commented PR (31+ new tests, explicit handling of untrusted/imported input, path-traversal and credential-redaction awareness). The two items above are the only things I'd want the author to look at before merge; everything else held up under review. |
- Put newly imported entries at the front of My Data. Import appended, which
contradicted the most-recently-first ordering `saveLayerLibraryEntry`
establishes and buried freshly shared layers under the whole existing library.
An id already present stays an in-place update rather than a reorder.
- Sweep nested source objects on export, not only top-level strings and arrays.
A layer type that groups its endpoint and auth as `{ store: { url, headers } }`
could otherwise ship a token one level deeper — the same "a per-field list has
to be extended for each new one" risk this file already calls out, which is the
argument for recursing rather than listing. Nested `requestHeaders`-style keys
are dropped like the top level's.
GeoJSON payloads are deliberately excluded from the sweep: `source.data` can
hold an inline FeatureCollection, and descending into it would rewrite any
feature property that looks like a URL with a `key=` parameter — corrupting the
user's data in the name of protecting it. Recursion is depth-capped.
- Add `password`, `pwd`, `client_secret`, and `clientsecret` to the redacted
query parameters.
Comment on lines
+272
to
+293
| if (input.libraryLayers) { | ||
| sections.push({ | ||
| id: "section:my-data", | ||
| kind: "section", | ||
| label: labels.myData ?? "My Data", | ||
| addable: false, | ||
| libraryImportExport: true, | ||
| count: input.libraryLayers.length, | ||
| children: input.libraryLayers.map( | ||
| (entry): BrowserNode => ({ | ||
| id: `library-layer:${entry.id}`, | ||
| kind: "library-layer", | ||
| label: entry.name, | ||
| addable: true, | ||
| libraryLayerId: entry.id, | ||
| renamable: true, | ||
| deletable: true, | ||
| ...(entry.needsLocalFile ? { needsLocalFile: true } : {}), | ||
| }), | ||
| ), | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
Minor UX nit (low confidence): filterBrowserTree (below, pre-existing) prunes a whole section when neither its own label nor any child matches the search query — which will also hide this section's Import (⬆) action whenever a search matches none of the saved layer names. Unlike Databases/Files, whose "always-on" ＋ actions share the same pre-existing gap, My Data's Import is arguably more likely to be wanted while searching (e.g. importing a shared bundle to find something not yet in the library). Not new logic, but this PR is what gives that gap a persistently-useful action to hide.
Member
Author
There was a problem hiding this comment.
Not changing this one, with reasoning rather than a dismissal. The pruning is filterBrowserTree pre-existing behavior and it is shared by every section, including the Databases and Files ＋ actions you note have the same gap. Making My Data an exception would leave the tree inconsistent; fixing it properly means changing how filterBrowserTree treats sections with always-on actions, which affects three sections that are not part of this feature and has its own UX question (should an empty-matching section render as a bare header while searching?).
I agree this PR is what makes the gap worth noticing, so it is a fair thing to raise — it just looks like a separate change to me rather than something to fold in at the end of this one. Happy to open a follow-up issue for it if that is useful; leaving this thread open for a maintainer to weigh in.
Contributor
|
All four inline comments posted successfully. Here's the final summary. Code reviewBugs
Security
Performance
Quality
CLAUDE.md
Overall the core data-layer module ( |
- Stop `canRestoreLibraryLayer` depending on a coincidence for Planetary Computer layers. Those set `externalNativeLayer`/`sourceKind` but never `customLayerType`, so `controlRendersLayer` returns false and the gate passed them through the "the map sync rebuilds it" branch without consulting the restore table — correct today only because the kind happens to be in that table. Listed explicitly as control-created, so the gate now depends on the restore entry and removing one stops offering the kind rather than silently saving entries that re-add unrendered. - Restore focus after deleting a My Data entry, to the section header (the nearest surviving row). The trash button held focus and its removal dropped focus to <body> — the same gap `endRename` already handles for rename. - Stop `endRename`'s deferred focus restore from stealing focus from a second rename editor. Clicking another row's pencil commits the first by blur, and that row's `autoFocus` input has already mounted by the time the frame runs, so the restore is now skipped when an editor is open (checked through a ref, since state is stale inside the deferred callback). - Correct `unresolvedLayerLibraryJoins`'s docstring: it returns join-target layer ids, not names — `LayerJoin` has no display name.
Comment on lines
+771
to
+804
| function redactEntryForExport(entry: LayerLibraryEntry): LayerLibraryEntry { | ||
| const source = { ...entry.source }; | ||
| let changed = false; | ||
| for (const key of EXPORT_REDACTED_SOURCE_KEYS) { | ||
| if (key in source) { | ||
| delete source[key]; | ||
| changed = true; | ||
| } | ||
| } | ||
| // Sweep every URL-shaped value in `source` rather than naming the fields that | ||
| // hold one. Different layer types put their URL in different places — `url`, | ||
| // `data` (a GeoJSON source pointing at a URL, which | ||
| // `hasRestorableLayerSource` already treats as re-fetchable), `tiles[]` — and | ||
| // a per-field list has to be extended for each new one, with a silent | ||
| // credential leak as the cost of forgetting. | ||
| for (const [key, value] of Object.entries(source)) { | ||
| const redacted = redactSourceValue(value); | ||
| if (redacted !== value) { | ||
| source[key] = redacted; | ||
| changed = true; | ||
| } | ||
| } | ||
| // `metadata.originalUrl` is the template a tile layer restores from, so it | ||
| // carries the same credential as `source.tiles`. | ||
| let metadata = entry.metadata; | ||
| if (typeof metadata.originalUrl === "string") { | ||
| const redacted = redactUrlCredentials(metadata.originalUrl); | ||
| if (redacted !== metadata.originalUrl) { | ||
| metadata = { ...metadata, originalUrl: redacted }; | ||
| changed = true; | ||
| } | ||
| } | ||
| return changed ? { ...entry, source, metadata } : entry; | ||
| } |
Contributor
There was a problem hiding this comment.
Security/privacy: local filesystem paths aren't redacted on export.
redactEntryForExport carefully strips credentials from source (headers, URL query params, userinfo, fragment tokens) and from metadata.originalUrl, but it never touches entry.sourcePath or entry.metadata.localFilePath. Both are captured verbatim for any layer read from a local file (see layerLocalPath/captureLayerLibraryEntry around line 319), including layers whose features are embedded under the cap (not just needsLocalFile degraded entries).
Since Export is pitched in the PR description as a one-click "share with your team" action, exported bundles will carry the exporting user's absolute local path — which on most OSes embeds the OS username (e.g. /Users/alice/Documents/..., C:\Users\alice\...) — with no redaction, even though the PR goes to considerable lengths to avoid leaking credentials the same way.
This may be intentional for the needsLocalFile case (recipients on a shared drive at the same path), but it applies indiscriminately to every locally-sourced entry, including ones where the path is unnecessary because the data is already embedded. Worth confirming this is deliberate, or stripping/warning about the path on export similar to the credential handling above.
Confidence: medium.
Comment on lines
+547
to
+555
| if (target) { | ||
| const { joins, virtualFields, ...rest } = plan.config; | ||
| useAppStore.getState().updateLayer(target.id, rest); | ||
| // Joins and virtual fields must go through their own actions so the | ||
| // derived columns are materialized rather than stored inert. | ||
| if (joins) useAppStore.getState().setLayerJoins(target.id, joins); | ||
| if (virtualFields) { | ||
| useAppStore.getState().setLayerVirtualFields(target.id, virtualFields); | ||
| } |
Contributor
There was a problem hiding this comment.
Bug: unresolved joins aren't reported on the local-file re-add path.
For the "layer" plan (line 500-508 above), unresolvedLayerLibraryJoins is checked before the add and browser.libraryLayerJoinsUnresolved is shown if any join's target layer isn't present in this project. This "local-file" branch applies joins via setLayerJoins (line 552) without ever calling unresolvedLayerLibraryJoins or surfacing that error — so re-adding a degraded (oversized) local-file entry with joins to a layer missing from the target project silently drops those joins (per the join engine's documented silent-drop behavior) with no equivalent warning to the user, unlike the parallel code path just above it.
Confidence: medium.
| // features at all (a rule-based style with hundreds of rules, a large | ||
| // attribute form), and telling that user their "features are the only copy" | ||
| // would send them looking in the wrong place. | ||
| return { ok: false, reason: embed ? "features-too-large" : "config-too-large" }; |
Contributor
There was a problem hiding this comment.
Minor: failure reason can be mislabeled after the path-only fallback also exceeds the cap.
If embed && localPath is true but the path-only entry built at line 333-338 is still over MAX_LAYER_LIBRARY_ENTRY_BYTES after geojson/embeddedGeoJSON are stripped (e.g. a huge style with a modestly-sized local file), execution falls through to this line, which reports "features-too-large" purely because embed was truthy earlier — even though at this point the entry no longer embeds any features and the actual overage is coming from the style/joins/attribute-form bulk. The user-facing message (saveToLibraryTooLarge) would then point them at the wrong cause. Consider re-deriving the reason from whether pathOnly (minus config) is itself over cap, or falling back to "config-too-large" when the non-feature fields alone exceed the limit.
Confidence: low (narrow edge case: requires both a large embedded/local-file feature set and an oversized style/joins/attribute-form).
Contributor
|
All three inline comments posted. Final summary below. Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
This was referenced Jul 31, 2026
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.
Fixes #1520
Adds a Layer Library: save a fully configured layer once, re-add it to any later project in one click. This closes the gap the existing libraries left — styles (Style Manager), services, views (bookmarks), and connections/favorites all persist across projects, but the layer itself did not, so re-adding a COG, a PostGIS table, or a remote GeoParquet meant walking the Add Data dialog again and restyling from scratch.
What it stores
packages/core/src/layer-library.tsfollows the shape ofstyle-library.tsand captures the source specification, not the data: source spec, layer type, fullLayerStyle(labels included), opacity, metadata, joins, virtual fields, and the attribute form. The library stays small and an entry always reflects its source's current contents. Project-specific placement (id,groupId,beforeId) is deliberately not captured; the transient XYZ proxy rewrite (metadata.resolvedUrl) is stripped.Edge cases from the issue
DESKTOPin the tree, with a clear message in the browser build instead of a silent failure. An in-memory layer over the cap is refused with a specific message rather than saved broken.metadata.embeddedGeoJSON— the same field the project Embed/Share flow writes and the plugin's restore pass reads — and a re-add runs that pass (apps/geolibre-desktop/src/lib/restore-library-layer.ts). Without this the re-added layer lists in the Layers panel and draws nothing; the current data is read from the control viamaterializeEmbeddableVectorLayers, so a tiles-mode layer works too.Entry points
Persistence
An app-level store slice persisted to IndexedDB (
layer-library-store.ts+useLayerLibraryPersistence), mirroring the Style Manager: outside the project lifecycle, never serialized into.geolibre.json, untouched by newProject/loadProject, and excluded from undo history.getAll()returns key order, so each record carries its display index and the load restores the most-recently-saved-first order.LayerTypeis now derived from a new runtimeLAYER_TYPESarray, so an imported bundle's layer type is validated against the real union with no chance of the list and the type drifting.Verification
Driven in a real browser with Playwright against
us_cities.geojsonfrom a local dataset directory, in both dark and light themes:0.70and fill#e11d48, saved it to My Data (confirmation shown, entry written to IndexedDB withopacity: 0.7,fillColor: #e11d48, and 109 features undermetadata.embeddedGeoJSON).geojson · 109 ft).opacity: 0.55; adding it produced a layer indistinguishable from the same service added through the app's own Services path.Export opens the browser's native save picker, which the headless run cannot complete — it goes through the same shared
saveTextFileWithFallbackhelper as the Style Manager export, andserializeLayerLibraryis covered by unit tests.Tests: 31 new cases in
tests/layer-library.test.ts(capture rules, the size-cap degradation ladder, the add plan, untrusted-input normalization, bundle round-trip), 8 intests/browser-tree.test.tsfor the My Data section, and 4 intests/layer-library-store.test.tsfor the stored-order restore. Full frontend suite (4448 passing), the coverage gate,npm run build, andpre-commitall pass.Summary by CodeRabbit