From e7ced622a5b1a71c856b9cfa4958b9a44e9fc1e6 Mon Sep 17 00:00:00 2001 From: giswqs Date: Wed, 29 Jul 2026 23:19:38 -0400 Subject: [PATCH 01/13] feat(browser): save a configured layer and re-add it from My Data MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- apps/geolibre-desktop/src/App.tsx | 2 + .../src/components/panels/BrowserPanel.tsx | 136 ++++- .../src/components/panels/BrowserTreeNode.tsx | 258 ++++++++-- .../src/components/panels/LayerPanel.tsx | 68 +++ .../src/hooks/useBrowserTree.ts | 30 +- .../src/hooks/useLayerLibraryPersistence.ts | 71 +++ .../geolibre-desktop/src/i18n/locales/en.json | 17 +- apps/geolibre-desktop/src/lib/browser-tree.ts | 65 ++- .../src/lib/layer-library-store.ts | 129 +++++ .../src/lib/restore-library-layer.ts | 63 +++ docs/features.md | 1 + packages/core/src/index.ts | 1 + packages/core/src/layer-library.ts | 473 ++++++++++++++++++ packages/core/src/store.ts | 52 ++ packages/core/src/types.ts | 101 +++- packages/plugins/src/index.ts | 10 +- tests/browser-tree.test.ts | 92 ++++ tests/layer-library-store.test.ts | 58 +++ tests/layer-library.test.ts | 473 ++++++++++++++++++ 19 files changed, 2014 insertions(+), 86 deletions(-) create mode 100644 apps/geolibre-desktop/src/hooks/useLayerLibraryPersistence.ts create mode 100644 apps/geolibre-desktop/src/lib/layer-library-store.ts create mode 100644 apps/geolibre-desktop/src/lib/restore-library-layer.ts create mode 100644 packages/core/src/layer-library.ts create mode 100644 tests/layer-library-store.test.ts create mode 100644 tests/layer-library.test.ts diff --git a/apps/geolibre-desktop/src/App.tsx b/apps/geolibre-desktop/src/App.tsx index 256e11d66..2844560b7 100644 --- a/apps/geolibre-desktop/src/App.tsx +++ b/apps/geolibre-desktop/src/App.tsx @@ -8,6 +8,7 @@ import { useLayoutOptions } from "./hooks/useLayoutOptions"; import { useProjectUrlLoader } from "./hooks/useProjectUrlLoader"; import { useBeforeUnloadGuard } from "./hooks/useBeforeUnloadGuard"; import { useRecentProjectsPersistence } from "./hooks/useRecentProjectsPersistence"; +import { useLayerLibraryPersistence } from "./hooks/useLayerLibraryPersistence"; import { useStyleLibraryPersistence } from "./hooks/useStyleLibraryPersistence"; import { useTemplateLibraryPersistence } from "./hooks/useTemplateLibraryPersistence"; import { useRuntimeEnvironmentVariables } from "./hooks/useRuntimeEnvironmentVariables"; @@ -33,6 +34,7 @@ export default function App() { useThemeScheme(); useRecentProjectsPersistence(); useStyleLibraryPersistence(); + useLayerLibraryPersistence(); useTemplateLibraryPersistence(); useRuntimeEnvironmentVariables(); useUndoRedoShortcuts(); diff --git a/apps/geolibre-desktop/src/components/panels/BrowserPanel.tsx b/apps/geolibre-desktop/src/components/panels/BrowserPanel.tsx index 039d89bfe..33f5c87c9 100644 --- a/apps/geolibre-desktop/src/components/panels/BrowserPanel.tsx +++ b/apps/geolibre-desktop/src/components/panels/BrowserPanel.tsx @@ -1,4 +1,10 @@ -import { useAppStore } from "@geolibre/core"; +import { + createLayerLibraryEntryId, + parseLayerLibrary, + planLayerLibraryAdd, + serializeLayerLibrary, + useAppStore, +} from "@geolibre/core"; import type { MapController } from "@geolibre/map"; import { fetchPostgisStatus, listPostgisTables } from "@geolibre/processing"; import { Input, ScrollArea } from "@geolibre/ui"; @@ -6,9 +12,18 @@ import { Search } from "lucide-react"; import { useCallback, useMemo, useRef, useState, type KeyboardEvent, type RefObject } from "react"; import { useTranslation } from "react-i18next"; import { startGeoLibreSidecar } from "../../lib/sidecar"; -import { isLoadableFilePath, isTauri, listDirectory, pickLocalDirectory } from "../../lib/tauri-io"; +import { + isLoadableFilePath, + isTauri, + listDirectory, + openLocalDataFileWithFallback, + pickLocalDirectory, + saveTextFileWithFallback, +} from "../../lib/tauri-io"; import { pinFolder, unpinFolder } from "../../lib/browser-folders"; import { addFavorite, isFavoritableKind, removeFavorite } from "../../lib/browser-favorites"; +import { createAppAPI } from "../../hooks/usePlugins"; +import { restoreLibraryLayer } from "../../lib/restore-library-layer"; import { useBrowserTree } from "../../hooks/useBrowserTree"; import { augmentConnections, @@ -48,6 +63,7 @@ interface BrowserPanelProps { /** The section nodes are expanded by default so their contents are visible. */ const DEFAULT_EXPANDED = new Set([ "section:favorites", + "section:my-data", "section:services", "section:recent", "section:databases", @@ -83,12 +99,16 @@ export function BrowserPanel({ }: BrowserPanelProps) { const { t } = useTranslation(); const addLayer = useAppStore((s) => s.addLayer); - const { tree, serviceById, favoriteIds } = useBrowserTree(); + const renameLayerLibraryEntry = useAppStore((s) => s.renameLayerLibraryEntry); + const deleteLayerLibraryEntry = useAppStore((s) => s.deleteLayerLibraryEntry); + const { tree, serviceById, favoriteIds, libraryLayerById } = useBrowserTree(); const [query, setQuery] = useState(""); const [expanded, setExpanded] = useState>(() => new Set(DEFAULT_EXPANDED)); const [busyId, setBusyId] = useState(null); const [error, setError] = useState(null); + // Id of the My Data row whose name is being edited in place, or null. + const [renamingId, setRenamingId] = useState(null); // Ref mirror of busyId for the re-entrancy guard: two clicks dispatched // back-to-back (before React commits the state update and the button's // disabled prop) would both read a stale `busyId === null`, so the guard @@ -434,6 +454,37 @@ export function BrowserPanel({ table: node.tableName, }, }); + } else if (node.kind === "library-layer" && node.libraryLayerId) { + const entry = libraryLayerById(node.libraryLayerId); + if (!entry) { + // The tree renders straight off the store slice, so an entry can only + // be missing if it was deleted between render and click. + setError(t("browser.libraryLayerMissing")); + return; + } + const plan = planLayerLibraryAdd(entry, { id: createLayerLibraryEntryId() }); + if (plan.kind === "layer") { + // Re-add exactly like a project load does: put the layer record in the + // store so MapController.syncLayers builds its map output, then run the + // owning plugin's restore pass when the layer is control-painted (that + // pass, not the store sync, is what draws those layers). + addLayer(plan.layer); + restoreLibraryLayer(plan.layer, createAppAPI(mapControllerRef)); + return; + } + // Entries whose features were too large to embed carry only a file path, + // so re-adding needs the desktop host's filesystem read. + if (!onAddFilePath) { + setError(t("browser.libraryLayerNeedsDesktop")); + return; + } + beginBusy(node.id); + try { + const addError = await onAddFilePath(plan.path); + if (addError) setError(addError); + } finally { + endBusy(); + } } else if (node.kind === "file" && node.path && onAddFilePath) { // Add the clicked file as a layer via the shell's dispatcher (which owns // the vector/raster/MBTiles store add-paths); it resolves to an error @@ -540,11 +591,81 @@ export function BrowserPanel({ }); }; + // My Data rename/delete. The store slice drives the tree, so both changes are + // reflected (and persisted to IndexedDB) with no refresh event to fire. + const commitRename = (node: BrowserNode, name: string) => { + setRenamingId(null); + if (node.libraryLayerId) renameLayerLibraryEntry(node.libraryLayerId, name); + }; + + const deleteLibraryLayer = (node: BrowserNode) => { + setError(null); + if (!node.libraryLayerId) return; + // Leave no rename editor open on a row that no longer exists. + if (renamingId === node.id) setRenamingId(null); + deleteLayerLibraryEntry(node.libraryLayerId); + }; + + // Import/export the whole library as a JSON bundle, matching how the Style + // Manager shares its presets. Ids collide on purpose so re-importing an + // exported bundle updates entries instead of duplicating them. + const importLibrary = async () => { + setError(null); + try { + const picked = await openLocalDataFileWithFallback({ + filters: [{ name: t("browser.libraryFilterName"), extensions: ["json"] }], + accept: ".json,application/json", + readText: true, + }); + if (!picked || picked.text === undefined) return; + const entries = parseLayerLibrary(picked.text); + // Read the library fresh from the store: the file picker above can block + // while another surface saves a layer. + const next = [...useAppStore.getState().layerLibrary]; + for (const entry of entries) { + const index = next.findIndex((e) => e.id === entry.id); + if (index >= 0) next[index] = entry; + else next.push(entry); + } + useAppStore.getState().setLayerLibrary(next); + } catch (err) { + // parseLayerLibrary's messages name the specific problem (bad JSON, + // unsupported version, no usable entries), so surface them directly. + setError(err instanceof Error ? err.message : t("browser.libraryImportFailed")); + } + }; + + const exportLibrary = async () => { + setError(null); + const entries = useAppStore.getState().layerLibrary; + if (entries.length === 0) return; + try { + await saveTextFileWithFallback(serializeLayerLibrary(entries), { + defaultName: "geolibre-layers.json", + filters: [{ name: t("browser.libraryFilterName"), extensions: ["json"] }], + browserTypes: [ + { + description: t("browser.libraryFilterName"), + accept: { "application/json": [".json"] }, + }, + ], + mimeType: "application/json", + }); + } catch (err) { + console.error("Failed to export the layer library", err); + setError(t("browser.libraryExportFailed")); + } + }; + // A section counts as content if it has children *or* an always-on ＋ action // (Databases' "New connection" and Files' "Add folder" show even with zero // entries, so a first-run user isn't stuck on the empty-state message). const hasContent = filtered.some( - (section) => section.children?.length || section.newConnectionKind || section.addFolderAction, + (section) => + section.children?.length || + section.newConnectionKind || + section.addFolderAction || + section.libraryImportExport, ); return ( @@ -590,6 +711,13 @@ export function BrowserPanel({ onRemoveFolder={removeFolder} favoriteIds={favoriteIds} onToggleFavorite={toggleFavorite} + renamingId={renamingId} + onBeginRename={setRenamingId} + onCommitRename={commitRename} + onCancelRename={() => setRenamingId(null)} + onDeleteLibraryLayer={deleteLibraryLayer} + onImportLibrary={() => void importLibrary()} + onExportLibrary={() => void exportLibrary()} /> ))} diff --git a/apps/geolibre-desktop/src/components/panels/BrowserTreeNode.tsx b/apps/geolibre-desktop/src/components/panels/BrowserTreeNode.tsx index ff4dfaa57..b0413613c 100644 --- a/apps/geolibre-desktop/src/components/panels/BrowserTreeNode.tsx +++ b/apps/geolibre-desktop/src/components/panels/BrowserTreeNode.tsx @@ -1,20 +1,26 @@ -import { cn } from "@geolibre/ui"; +import { cn, Input } from "@geolibre/ui"; import { ChevronDown, ChevronRight, Clock, Database, + Download, File, Folder, FolderOpen, Globe2, + Layers, Loader2, + Pencil, Plus, Star, Table, + Trash2, + Upload, X, type LucideIcon, } from "lucide-react"; +import { useState, type KeyboardEvent } from "react"; import { useTranslation } from "react-i18next"; import type { AddDataKind } from "../layout/AddDataDialog"; import { isFavoritableKind } from "../../lib/browser-favorites"; @@ -46,6 +52,20 @@ interface BrowserTreeNodeProps { favoriteIds: ReadonlySet; /** Toggle a favoritable node's favorite state (its ☆/★). */ onToggleFavorite: (node: BrowserNode) => void; + /** Id of the node currently being renamed in place, or null. */ + renamingId: string | null; + /** Start renaming a renamable node (its pencil). */ + onBeginRename: (id: string) => void; + /** Commit a rename; a blank or unchanged name is a no-op for the caller. */ + onCommitRename: (node: BrowserNode, name: string) => void; + /** Abandon the in-progress rename (Escape). */ + onCancelRename: () => void; + /** Delete a saved Layer Library entry (its trash icon). */ + onDeleteLibraryLayer: (node: BrowserNode) => void; + /** Import a Layer Library JSON bundle (the My Data section's ⬆). */ + onImportLibrary: () => void; + /** Export the Layer Library as a JSON bundle (the My Data section's ⬇). */ + onExportLibrary: () => void; } /** The leading icon for a node, chosen by kind (and expanded state for groups). */ @@ -61,11 +81,63 @@ function nodeIcon(node: BrowserNode, isExpanded: boolean): LucideIcon { return Table; case "file": return File; + case "library-layer": + return Layers; default: return isExpanded ? FolderOpen : Folder; } } +/** + * The in-place rename editor a `library-layer` row swaps in for its treeitem + * button. Enter commits, Escape abandons, and blur commits too so clicking away + * keeps the typed name rather than silently discarding it. + */ +function RenameRow({ + node, + paddingLeft, + onCommit, + onCancel, +}: { + node: BrowserNode; + paddingLeft: number; + onCommit: (name: string) => void; + onCancel: () => void; +}) { + const { t } = useTranslation(); + const [value, setValue] = useState(node.label); + // Escape must not also fire the blur commit that follows the unmount. + const [cancelled, setCancelled] = useState(false); + const onKeyDown = (event: KeyboardEvent) => { + if (event.key === "Enter") { + event.preventDefault(); + onCommit(value); + } else if (event.key === "Escape") { + event.preventDefault(); + setCancelled(true); + onCancel(); + } + }; + return ( +
+ setValue(event.target.value)} + onKeyDown={onKeyDown} + onBlur={() => { + if (!cancelled) onCommit(value); + }} + /> +
+ ); +} + /** * One row in the Browser tree, rendered recursively. Group nodes * (section/category) toggle their children; leaf nodes (service/recent-project) @@ -85,6 +157,13 @@ export function BrowserTreeNode({ onRemoveFolder, favoriteIds, onToggleFavorite, + renamingId, + onBeginRename, + onCommitRename, + onCancelRename, + onDeleteLibraryLayer, + onImportLibrary, + onExportLibrary, }: BrowserTreeNodeProps) { const { t } = useTranslation(); // A status row (loading / error) is non-interactive text, not a tree control. @@ -141,64 +220,132 @@ export function BrowserTreeNode({ // pin/unpin them to the Favorites section. const favoritable = isFavoritableKind(node.kind); const favorited = favoritable && favoriteIds.has(node.id); + const isRenaming = renamingId === node.id; return ( // role="none": the treeitem role lives on the inner button, so the
  • // must not add a listitem role to the tree/group.
  • - + + )} + {isBusy ? ( + + ) : ( + + )} + {node.label} + {node.builtin ? ( + + {t("browser.builtinBadge")} + + ) : null} + {node.needsLocalFile ? ( + // A saved layer whose features were too large to embed: only a host + // that can read its file path can re-add it. + + {t("browser.desktopOnlyBadge")} + + ) : null} + {typeof node.count === "number" && node.count > 0 ? ( + {node.count} + ) : null} + + )} + {node.renamable && !isRenaming ? ( + + ) : null} + {node.deletable && !isRenaming ? ( + + ) : null} + {node.libraryImportExport ? ( + <> + + + + ) : null} {favoritable ? (
    diff --git a/apps/geolibre-desktop/src/components/panels/LayerPanel.tsx b/apps/geolibre-desktop/src/components/panels/LayerPanel.tsx index def3f69b4..1a4e63afd 100644 --- a/apps/geolibre-desktop/src/components/panels/LayerPanel.tsx +++ b/apps/geolibre-desktop/src/components/panels/LayerPanel.tsx @@ -170,6 +170,7 @@ import { reloadLocalFileLayer, setLayerWatchConfig, } from "../../lib/local-file-watch"; +import { canRestoreLibraryLayer } from "../../lib/restore-library-layer"; import { getSqlQueryLayerConfig, isSqlQueryLayer, @@ -2599,9 +2600,13 @@ export function LayerPanel({ // layers. Shares the Style Manager's gate so the two can't drift. const canImportStyle = isStyleLibraryTargetLayer(layer.type); // Saving the whole layer (source + style + labels + filters + joins) - // to the Layer Library needs something re-addable to point at - // (issue #1520); a layer with no source and no features is excluded. - const canSaveToLibrary = canSaveLayerToLibrary(layer); + // to the Layer Library needs something re-addable to point at AND a + // way to render it again (issue #1520), so a layer with no source and + // no features is excluded — and so is a control-painted layer whose + // kind has no restore route, which would otherwise re-add blank. + const canSaveToLibrary = canSaveLayerToLibrary(layer, { + canRestoreControlPainted: canRestoreLibraryLayer, + }); // Copy/paste symbology (issue #1339). Vector-styled layers and // deck.gl rasters each copy their own style family; a paste only // lands when the clipboard entry shares the target's family. diff --git a/apps/geolibre-desktop/src/lib/restore-library-layer.ts b/apps/geolibre-desktop/src/lib/restore-library-layer.ts index 51291775f..41932b4fa 100644 --- a/apps/geolibre-desktop/src/lib/restore-library-layer.ts +++ b/apps/geolibre-desktop/src/lib/restore-library-layer.ts @@ -12,7 +12,11 @@ // is exactly what the project-load path relies on, so invoking one for a single // new layer is safe. -import { isExternalNativeLayerRecord, type GeoLibreLayer } from "@geolibre/core"; +import { + controlRendersLayer, + isExternalNativeLayerRecord, + type GeoLibreLayer, +} from "@geolibre/core"; import { RASTER_SOURCE_KIND, restoreLidarLayers, @@ -41,23 +45,54 @@ const RESTORE_BY_SOURCE_KIND: Record void | Pro "lidar-url": restoreLidarLayers, }; +/** The restore pass that renders `layer`, or undefined when it needs none. */ +function restorePassFor( + layer: GeoLibreLayer, +): ((app: GeoLibreAppAPI) => void | Promise) | undefined { + const sourceKind = layer.metadata.sourceKind; + return typeof sourceKind === "string" ? RESTORE_BY_SOURCE_KIND[sourceKind] : undefined; +} + /** - * Run the plugin restore pass that renders `layer`, if it is control-painted. - * A no-op for a layer GeoLibre renders itself. + * Whether re-adding `layer` from the Layer Library will actually render it. The + * map's own sync rebuilds most external-native layers straight from the store + * record — PMTiles, Esri Wayback, basemap-control and web-service rasters, the + * generic raster-tile path, and the GeoJSON fall-through. Only a custom-render + * layer ({@link controlRendersLayer}) needs its control to recreate the map + * output, and for those we need a restore pass for that kind. + * + * This is the capability `canSaveLayerToLibrary` gates "Save to My Data" on, so + * a layer that would re-add blank is never offered in the first place. Adding a + * kind to {@link RESTORE_BY_SOURCE_KIND} therefore also makes it saveable — the + * two cannot drift apart. + * + * @param layer - The layer being considered for the library. + * @returns Whether the layer can be re-added and rendered. + */ +export function canRestoreLibraryLayer(layer: GeoLibreLayer): boolean { + if (!isExternalNativeLayerRecord(layer)) return true; + return !controlRendersLayer(layer) || restorePassFor(layer) !== undefined; +} + +/** + * Run the plugin restore pass that renders `layer`, if it is control-painted and + * needs one. A no-op for a layer the map sync rebuilds from the record. * * @param layer - The layer just added from the Layer Library. * @param app - The plugin app API (from `createAppAPI`). - * @returns Whether a restore pass was dispatched. + * @returns Resolves once the pass has been dispatched and settled (immediately + * when none was needed). The passes log their own per-layer failures. */ -export function restoreLibraryLayer(layer: GeoLibreLayer, app: GeoLibreAppAPI): boolean { - if (!isExternalNativeLayerRecord(layer)) return false; - const sourceKind = layer.metadata.sourceKind; - const restore = typeof sourceKind === "string" ? RESTORE_BY_SOURCE_KIND[sourceKind] : undefined; - if (!restore) return false; - // The passes are fire-and-forget (they log their own failures); await nothing - // here so a slow re-ingest never blocks the panel's click handler. - void Promise.resolve(restore(app)).catch((error: unknown) => { +export async function restoreLibraryLayer( + layer: GeoLibreLayer, + app: GeoLibreAppAPI, +): Promise { + if (!isExternalNativeLayerRecord(layer)) return; + const restore = restorePassFor(layer); + if (!restore) return; + try { + await restore(app); + } catch (error) { console.error("[GeoLibre] Failed to restore a layer added from My Data", error); - }); - return true; + } } diff --git a/packages/core/src/external-native-paint.ts b/packages/core/src/external-native-paint.ts index f1c7cdd45..783b98e11 100644 --- a/packages/core/src/external-native-paint.ts +++ b/packages/core/src/external-native-paint.ts @@ -42,6 +42,25 @@ export type ExternalNativePaintMode = "geolibre" | "plugin"; * True when the plugin that registered this layer paints it itself, so GeoLibre * must not offer (or apply) MapLibre paint properties for it. */ +/** + * Whether a layer is drawn by a **custom render layer** its control created — a + * deck.gl overlay, 3D Tiles, or Add Vector Layer's own MapLibre layers — rather + * than by GeoLibre's layer sync. `metadata.customLayerType` is what marks one, + * and it is the flag `syncExternalNativeLayer` branches on: for these layers the + * sync only *moves and filters* native layers that already exist, so nothing + * appears on the map unless the owning control (re)creates them. + * + * Lives here, in core, so the map's dispatch and the consumers that need to + * reason about it — the Layer Library's "can this be re-added and rendered?" + * gate (issue #1520) — share one definition and cannot drift. + * + * @param layer - A store layer (metadata only). + * @returns True when only the owning control can create the layer's map output. + */ +export function controlRendersLayer(layer: { metadata: Record }): boolean { + return typeof layer.metadata.customLayerType === "string"; +} + export function pluginOwnsPaint(layer: { metadata: Record }): boolean { return layer.metadata.paintMode === "plugin"; } diff --git a/packages/core/src/layer-library.ts b/packages/core/src/layer-library.ts index 9aae9ce45..0a2876ee2 100644 --- a/packages/core/src/layer-library.ts +++ b/packages/core/src/layer-library.ts @@ -79,25 +79,39 @@ function featureCount(geojson: FeatureCollection | undefined): number { /** * Whether "Save to library" applies to a layer at all — the cheap predicate the - * layer actions menu gates on. A layer qualifies when it has a re-fetchable - * source, a local file path, or features to embed; the only layers excluded are - * those with none of the three, which nothing could re-add. + * layer actions menu gates on. Two things have to hold: + * + * 1. There is something to re-add *from*: a re-fetchable source, a local file + * path, or features to embed. + * 2. The host can actually render it again. For a control-painted layer + * ({@link isExternalNativeLayerRecord}) that means either the map's own sync + * rebuilds it from the record or the owning plugin's restore pass can be + * re-run, which only the host knows — so the caller supplies + * `canRestoreControlPainted`. Without it, control-painted layers are refused + * rather than saved into an entry that would re-add unrendered. * * A layer that qualifies here can still fail to save because its features are - * too large to embed ({@link MAX_LAYER_LIBRARY_ENTRY_BYTES}); that outcome - * needs the full capture, so it is reported by - * {@link captureLayerLibraryEntry} rather than hidden behind a disabled menu - * item. + * too large to embed ({@link MAX_LAYER_LIBRARY_ENTRY_BYTES}); that outcome needs + * the full capture, so it is reported by {@link captureLayerLibraryEntry} rather + * than hidden behind a disabled menu item. * * @param layer - The layer to test. + * @param options - `canRestoreControlPainted` answers whether the host can + * re-render a control-painted layer (the desktop app passes its + * `canRestoreLibraryLayer`). * @returns Whether the layer can be offered to the library. */ -export function canSaveLayerToLibrary(layer: GeoLibreLayer): boolean { - return ( +export function canSaveLayerToLibrary( + layer: GeoLibreLayer, + options: { canRestoreControlPainted?: (layer: GeoLibreLayer) => boolean } = {}, +): boolean { + const hasSomethingToReAddFrom = hasRestorableLayerSource(layer) || layerLocalPath(layer) !== undefined || - featureCount(layer.geojson) > 0 - ); + featureCount(layer.geojson) > 0; + if (!hasSomethingToReAddFrom) return false; + if (!isExternalNativeLayerRecord(layer)) return true; + return options.canRestoreControlPainted?.(layer) === true; } /** Why a layer could not be captured into the library. */ diff --git a/packages/map/src/layer-sync.ts b/packages/map/src/layer-sync.ts index 9918e1492..da42d73b7 100644 --- a/packages/map/src/layer-sync.ts +++ b/packages/map/src/layer-sync.ts @@ -1,4 +1,5 @@ import { + controlRendersLayer, DEFAULT_LAYER_STYLE, type GeoLibreLayer, type ExternalNativePaintBridge, @@ -674,8 +675,12 @@ function isPMTilesExternalLayer(layer: GeoLibreLayer): boolean { ); } +// Delegates to core's `controlRendersLayer` so the flag this dispatch branches +// on has exactly one definition: the Layer Library's "can this be re-added and +// rendered?" gate reads the same predicate (issue #1520), and a change to what +// marks a custom-render layer cannot leave the two disagreeing. function isExternalCustomLayer(layer: GeoLibreLayer): boolean { - return typeof layer.metadata.customLayerType === "string"; + return controlRendersLayer(layer); } // Opt-in for control-managed layers (`customLayerType`, the ordering-only path) diff --git a/tests/layer-library.test.ts b/tests/layer-library.test.ts index 652096492..38f90957f 100644 --- a/tests/layer-library.test.ts +++ b/tests/layer-library.test.ts @@ -4,6 +4,7 @@ import type { FeatureCollection } from "geojson"; import { captureLayerLibraryEntry, canSaveLayerToLibrary, + controlRendersLayer, createLayerLibraryEntryId, DEFAULT_LAYER_STYLE, hasRestorableLayerSource, @@ -101,6 +102,75 @@ describe("canSaveLayerToLibrary", () => { false, ); }); + + it("refuses a control-painted layer the host cannot re-render", () => { + // Saving one anyway would produce an entry that re-adds into the Layers + // panel and draws nothing, so it must not be offered at all. + const controlPainted = layer({ + source: { url: "https://example.com/a.tif" }, + metadata: { externalNativeLayer: true, sourceKind: "some-deckgl-control" }, + }); + assert.equal(canSaveLayerToLibrary(controlPainted), false); + assert.equal( + canSaveLayerToLibrary(controlPainted, { canRestoreControlPainted: () => false }), + false, + ); + assert.equal( + canSaveLayerToLibrary(controlPainted, { canRestoreControlPainted: () => true }), + true, + ); + }); + + it("agrees with controlRendersLayer about which layers need the predicate", () => { + // `controlRendersLayer` is the flag layer-sync's dispatch branches on, and + // the desktop app's `canRestoreLibraryLayer` reads the same predicate: an + // external-native layer WITHOUT `customLayerType` is rebuilt from the record + // by the map sync, so it needs no restore pass to be saveable. + const rebuiltBySync = layer({ + type: "pmtiles", + source: { url: "https://example.com/a.pmtiles" }, + metadata: { externalNativeLayer: true, sourceKind: "pmtiles-url" }, + }); + assert.equal(controlRendersLayer(rebuiltBySync), false); + assert.equal( + canSaveLayerToLibrary(rebuiltBySync, { + canRestoreControlPainted: (candidate) => !controlRendersLayer(candidate), + }), + true, + ); + const needsItsControl = layer({ + type: "cog", + source: { url: "https://example.com/a.tif" }, + metadata: { + externalNativeLayer: true, + // The real shape a Vantor/STAC COG layer carries. + customLayerType: "raster", + sourceKind: "cog-url", + }, + }); + assert.equal(controlRendersLayer(needsItsControl), true); + assert.equal( + canSaveLayerToLibrary(needsItsControl, { + canRestoreControlPainted: (candidate) => !controlRendersLayer(candidate), + }), + false, + ); + }); + + it("does not consult the host predicate for a layer GeoLibre renders itself", () => { + let asked = false; + const plain = layer({ source: { url: "https://example.com/a.fgb" } }); + assert.equal( + canSaveLayerToLibrary(plain, { + canRestoreControlPainted: () => { + asked = true; + return false; + }, + }), + true, + ); + assert.equal(asked, false); + }); }); describe("captureLayerLibraryEntry", () => { From dd616ac321b6b6c88cabf0f1daaa5f9f3990b395 Mon Sep 17 00:00:00 2001 From: giswqs Date: Thu, 30 Jul 2026 00:03:52 -0400 Subject: [PATCH 03/13] Address round-2 review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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. --- .../src/components/panels/BrowserPanel.tsx | 23 ++- .../src/components/panels/BrowserTreeNode.tsx | 18 +- .../src/components/panels/LayerPanel.tsx | 6 +- apps/geolibre-desktop/src/lib/tauri-io.ts | 18 +- packages/core/src/layer-library.ts | 177 +++++++++++++++--- packages/core/src/paths.ts | 21 +++ tests/layer-library.test.ts | 151 ++++++++++++++- 7 files changed, 358 insertions(+), 56 deletions(-) diff --git a/apps/geolibre-desktop/src/components/panels/BrowserPanel.tsx b/apps/geolibre-desktop/src/components/panels/BrowserPanel.tsx index a3557f709..c26a55d3b 100644 --- a/apps/geolibre-desktop/src/components/panels/BrowserPanel.tsx +++ b/apps/geolibre-desktop/src/components/panels/BrowserPanel.tsx @@ -487,8 +487,29 @@ export function BrowserPanel({ } beginBusy(node.id); try { + // The file import builds a fresh, default-styled layer, so the entry's + // saved configuration is applied to it afterwards — otherwise a degraded + // entry would silently come back unstyled. The import does not report + // which layer it created, so it is identified by diffing the ids around + // the call (a concurrent add elsewhere would leave more than one new id, + // in which case nothing is patched rather than the wrong layer). + const idsBefore = new Set(useAppStore.getState().layers.map((entry) => entry.id)); const addError = await onAddFilePath(plan.path); - if (addError) setError(addError); + if (addError) { + setError(addError); + } else { + const added = useAppStore.getState().layers.filter((entry) => !idsBefore.has(entry.id)); + if (added.length === 1) { + const { joins, virtualFields, ...rest } = plan.config; + useAppStore.getState().updateLayer(added[0].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(added[0].id, joins); + if (virtualFields) { + useAppStore.getState().setLayerVirtualFields(added[0].id, virtualFields); + } + } + } } finally { endBusy(); } diff --git a/apps/geolibre-desktop/src/components/panels/BrowserTreeNode.tsx b/apps/geolibre-desktop/src/components/panels/BrowserTreeNode.tsx index 6c17205ac..a8273daae 100644 --- a/apps/geolibre-desktop/src/components/panels/BrowserTreeNode.tsx +++ b/apps/geolibre-desktop/src/components/panels/BrowserTreeNode.tsx @@ -28,7 +28,7 @@ import type { BrowserNode } from "../../lib/browser-tree"; interface BrowserTreeNodeProps { node: BrowserNode; - /** Nesting depth, for the row's left indent. */ + /** Nesting depth, for the row's inline-start indent. */ depth: number; /** Ids of the currently expanded group nodes. */ expanded: ReadonlySet; @@ -95,12 +95,12 @@ function nodeIcon(node: BrowserNode, isExpanded: boolean): LucideIcon { */ function RenameRow({ node, - paddingLeft, + indentStart, onCommit, onCancel, }: { node: BrowserNode; - paddingLeft: number; + indentStart: number; onCommit: (name: string) => void; onCancel: () => void; }) { @@ -127,7 +127,7 @@ function RenameRow({ } }; return ( -
    +

    onCommitRename(node, name)} onCancel={onCancelRename} /> @@ -253,7 +253,7 @@ export function BrowserTreeNode({ "disabled:pointer-events-none disabled:opacity-50", node.kind === "section" && "font-semibold", )} - style={{ paddingLeft }} + style={{ paddingInlineStart: indentStart }} role="treeitem" aria-level={depth + 1} aria-expanded={isGroup ? isExpanded : undefined} @@ -441,7 +441,7 @@ export function BrowserTreeNode({ // is opened) shows a hint instead of a bare gap.

    {t("browser.emptyGroup")}

    diff --git a/apps/geolibre-desktop/src/components/panels/LayerPanel.tsx b/apps/geolibre-desktop/src/components/panels/LayerPanel.tsx index 1a4e63afd..d7984ad2e 100644 --- a/apps/geolibre-desktop/src/components/panels/LayerPanel.tsx +++ b/apps/geolibre-desktop/src/components/panels/LayerPanel.tsx @@ -1050,7 +1050,11 @@ export function LayerPanel({ const features = isEmbeddableLocalVectorLayer(layer) ? (await materializeEmbeddableVectorLayers([layer])).get(layer.id) : undefined; - const result = captureLayerLibraryEntry(layer, { + // The materialize await can outlive a concurrent style/opacity/join edit, + // so capture from the current layer rather than the closure's snapshot + // (mirrors handleImportStyle / handleSaveEditsToSource). + const latest = useAppStore.getState().layers.find((l) => l.id === layer.id) ?? layer; + const result = captureLayerLibraryEntry(latest, { id: createLayerLibraryEntryId(), addedAt: new Date().toISOString(), ...(features ? { features } : {}), diff --git a/apps/geolibre-desktop/src/lib/tauri-io.ts b/apps/geolibre-desktop/src/lib/tauri-io.ts index 194390d0a..0901e134d 100644 --- a/apps/geolibre-desktop/src/lib/tauri-io.ts +++ b/apps/geolibre-desktop/src/lib/tauri-io.ts @@ -1,4 +1,9 @@ -import { hasPathTraversal, parseProject, type GeoLibreProject } from "@geolibre/core"; +import { + hasPathTraversal, + isAbsoluteFilesystemPath, + parseProject, + type GeoLibreProject, +} from "@geolibre/core"; import { convertFileSrc, invoke } from "@tauri-apps/api/core"; import { open, save } from "@tauri-apps/plugin-dialog"; import { @@ -1671,13 +1676,10 @@ async function tryLoadPickedNativeVectorPath( } export function isAbsoluteLocalPath(path: string): boolean { - // Match the raw path (not a trimmed copy): a whitespace-padded value would - // pass a trimmed check but reach `readFile` unchanged and fail there, so - // reject it up front instead. Accept POSIX paths and Windows drive-letter - // paths only. UNC paths (\\server\share) are deliberately rejected: reading - // one can make Windows auto-authenticate against a remote host (NTLM hash - // capture), and a remote share is not a supported local data source. - return path.startsWith("/") || /^[a-z]:[\\/]/i.test(path); + // Delegates to core so record normalization (which cannot import this module) + // validates a persisted path by exactly the same rule; see + // `isAbsoluteFilesystemPath` for why UNC paths are rejected. + return isAbsoluteFilesystemPath(path); } async function loadTauriVectorFile( diff --git a/packages/core/src/layer-library.ts b/packages/core/src/layer-library.ts index 0a2876ee2..953953198 100644 --- a/packages/core/src/layer-library.ts +++ b/packages/core/src/layer-library.ts @@ -14,11 +14,16 @@ import type { FeatureCollection } from "geojson"; import { DEFAULT_LAYER_STYLE, LAYER_TYPES, + type AttributeFormConfig, type GeoLibreLayer, + type LayerJoin, type LayerLibraryEntry, + type LayerStyle, type LayerType, + type LayerVirtualField, } from "./types"; import { sanitizeLayerStylePatch } from "./style-library"; +import { hasPathTraversal, isAbsoluteFilesystemPath } from "./paths"; /** `type` discriminator of an exported Layer Library bundle file. */ export const LAYER_LIBRARY_BUNDLE_TYPE = "geolibre-layer-library"; @@ -68,9 +73,36 @@ export function hasRestorableLayerSource( return nonEmptyString((layer.metadata ?? {}).originalUrl); } -/** The absolute local path a layer was read from, or undefined. */ -function layerLocalPath(layer: Pick): string | undefined { - return nonEmptyString(layer.sourcePath) ? layer.sourcePath : undefined; +/** + * The absolute local path a layer's features can be **re-read** from, or + * undefined. + * + * A non-empty `sourcePath` is not enough on its own: a file picked in the + * browser has no path, and `createVectorStoreLayer` still records the bare file + * *name* there for display. `metadata.localFileReloadable` is the flag the rest + * of the codebase uses to mark a genuinely re-readable absolute path + * (`prepareLayerForSave`, `needsLocalFileReload`, `restoreVectorLayers`), so + * require it — otherwise a degraded path-only entry would store a bare filename + * no host could ever open. The path is re-validated because a hand-edited + * project can set both fields to anything. + */ +function layerLocalPath(layer: Pick): string | undefined { + if ((layer.metadata ?? {}).localFileReloadable !== true) return undefined; + return nonEmptyString(layer.sourcePath) && isReReadablePath(layer.sourcePath) + ? layer.sourcePath + : undefined; +} + +/** + * Whether a persisted path is safe to hand to a host file read: absolute, with + * no `..` traversal. Applied to captured *and* imported entries, because a + * shared bundle is untrusted input — a crafted `needsLocalFile` entry could + * otherwise point at any file on the importing user's disk, and clicking it in + * My Data would read that file. Mirrors the client-side re-validation the + * project-restore paths already do before calling into Tauri. + */ +function isReReadablePath(path: string): boolean { + return isAbsoluteFilesystemPath(path) && !hasPathTraversal(path); } function featureCount(geojson: FeatureCollection | undefined): number { @@ -248,16 +280,32 @@ export function captureLayerLibraryEntry( return { ok: false, reason: "too-large" }; } +/** + * The presentation state a re-added layer takes from its entry. For the + * `local-file` plan the host imports the file first (producing a default-styled + * layer) and then applies this patch, so a degraded entry still comes back + * "fully configured" rather than losing the styling the entry preserved. + */ +export interface LayerLibraryConfigPatch { + name: string; + style: LayerStyle; + opacity: number; + joins?: LayerJoin[]; + virtualFields?: LayerVirtualField[]; + attributeForm?: AttributeFormConfig; +} + /** How an entry should be re-added to the current project. */ export type LayerLibraryAddPlan = /** Add this layer record to the store; the map sync renders it. */ | { kind: "layer"; layer: GeoLibreLayer } /** - * The entry's features live only in a local file the host must re-read - * (its data was too large to embed), so the caller runs its local-file add - * path for `path`. Only reachable on a host with filesystem access. + * The entry's features live only in a local file the host must re-read (its + * data was too large to embed), so the caller runs its local-file add path for + * `path` and then applies `config` to the layer that import produced. Only + * reachable on a host with filesystem access. */ - | { kind: "local-file"; path: string; name: string }; + | { kind: "local-file"; path: string; config: LayerLibraryConfigPatch }; /** * Plan how to re-add a library entry to the current project. @@ -270,8 +318,13 @@ export function planLayerLibraryAdd( entry: LayerLibraryEntry, options: { id: string }, ): LayerLibraryAddPlan { - if (entry.needsLocalFile && nonEmptyString(entry.sourcePath)) { - return { kind: "local-file", path: entry.sourcePath, name: entry.name }; + if (layerLibraryEntryNeedsLocalFile(entry)) { + return { + kind: "local-file", + // Non-null by the guard above, which also re-validates the path. + path: entry.sourcePath as string, + config: layerLibraryConfigPatch(entry), + }; } return { kind: "layer", @@ -294,6 +347,24 @@ export function planLayerLibraryAdd( }; } +/** + * The presentation state to apply to a layer the host imported from an entry's + * file, so the `local-file` plan preserves everything the entry captured. + * + * @param entry - The entry being re-added. + * @returns The patch to merge onto the imported layer. + */ +export function layerLibraryConfigPatch(entry: LayerLibraryEntry): LayerLibraryConfigPatch { + return { + name: entry.name, + style: structuredClone(entry.style), + opacity: entry.opacity, + ...(entry.joins ? { joins: structuredClone(entry.joins) } : {}), + ...(entry.virtualFields ? { virtualFields: structuredClone(entry.virtualFields) } : {}), + ...(entry.attributeForm ? { attributeForm: structuredClone(entry.attributeForm) } : {}), + }; +} + /** * Whether re-adding an entry needs a host that can read local files — used to * badge the entry in the Browser panel and to explain the failure in the @@ -303,7 +374,11 @@ export function planLayerLibraryAdd( * @returns True when only a filesystem-capable host can add it. */ export function layerLibraryEntryNeedsLocalFile(entry: LayerLibraryEntry): boolean { - return entry.needsLocalFile === true && nonEmptyString(entry.sourcePath); + return ( + entry.needsLocalFile === true && + nonEmptyString(entry.sourcePath) && + isReReadablePath(entry.sourcePath) + ); } /** A plain JSON object (not an array, not null), or undefined. */ @@ -313,6 +388,50 @@ function plainObject(value: unknown): Record | undefined { : undefined; } +/** Whether every listed key on an object holds a non-empty string. */ +function hasStringKeys(object: Record, keys: readonly string[]): boolean { + return keys.every((key) => nonEmptyString(object[key])); +} + +/** + * Keep only the {@link LayerJoin} members a hand-edited or imported bundle got + * right. A join missing any of its required fields resolves to nothing and would + * sit in the Joins UI as a permanently-broken definition, so drop it rather than + * the whole entry. + */ +function validJoins(value: unknown): LayerJoin[] | undefined { + const items = objectArray(value)?.filter((join) => + hasStringKeys(join, ["id", "joinLayerId", "targetField", "joinField"]), + ); + return items && items.length > 0 ? (structuredClone(items) as unknown as LayerJoin[]) : undefined; +} + +/** Keep only {@link LayerVirtualField} members that carry an id, name, and expression. */ +function validVirtualFields(value: unknown): LayerVirtualField[] | undefined { + const items = objectArray(value)?.filter((field) => + hasStringKeys(field, ["id", "name", "expression"]), + ); + return items && items.length > 0 + ? (structuredClone(items) as unknown as LayerVirtualField[]) + : undefined; +} + +/** + * Keep an {@link AttributeFormConfig} only when every field entry names the + * feature property it configures; a field with no `field` key would apply to + * nothing. + */ +function validAttributeForm(value: unknown): AttributeFormConfig | undefined { + const config = plainObject(value); + if (!config || !Array.isArray(config.fields)) return undefined; + const fields = config.fields.filter( + (field) => + plainObject(field) !== undefined && nonEmptyString((field as { field?: unknown }).field), + ); + if (fields.length === 0) return undefined; + return structuredClone({ ...config, fields }) as unknown as AttributeFormConfig; +} + /** An array of plain JSON objects, or undefined when nothing usable is present. */ function objectArray(value: unknown): Record[] | undefined { if (!Array.isArray(value)) return undefined; @@ -360,7 +479,14 @@ export function normalizeLayerLibraryEntries(value: unknown): LayerLibraryEntry[ if (!id || !name || !layerType || seen.has(id)) continue; const source = plainObject(candidate.source) ?? {}; const metadata = plainObject(candidate.metadata) ?? {}; - const sourcePath = nonEmptyString(candidate.sourcePath) ? candidate.sourcePath : undefined; + // A bundle is shareable, so its `sourcePath` is untrusted: keep it only when + // it is an absolute, traversal-free path a host could legitimately re-read. + // Without this a crafted entry could point `onAddFilePath` at any file on the + // importing user's disk. + const sourcePath = + nonEmptyString(candidate.sourcePath) && isReReadablePath(candidate.sourcePath) + ? candidate.sourcePath + : undefined; const geojson = featureCollection(candidate.geojson); // Same "is there anything to re-add from" gate as the capture path, so a // hand-edited bundle cannot introduce an entry that always fails. A @@ -392,23 +518,18 @@ export function normalizeLayerLibraryEntries(value: unknown): LayerLibraryEntry[ opacity, metadata: structuredClone(metadata), ...(sourcePath ? { sourcePath } : {}), - ...(objectArray(candidate.joins) - ? { joins: structuredClone(candidate.joins) as LayerLibraryEntry["joins"] } - : {}), - ...(objectArray(candidate.virtualFields) - ? { - virtualFields: structuredClone( - candidate.virtualFields, - ) as LayerLibraryEntry["virtualFields"], - } - : {}), - ...(plainObject(candidate.attributeForm) - ? { - attributeForm: structuredClone( - candidate.attributeForm, - ) as LayerLibraryEntry["attributeForm"], - } - : {}), + ...(() => { + // Each block is kept only when its members carry the fields their engine + // needs; a malformed member is dropped rather than the whole entry. + const joins = validJoins(candidate.joins); + const virtualFields = validVirtualFields(candidate.virtualFields); + const attributeForm = validAttributeForm(candidate.attributeForm); + return { + ...(joins ? { joins } : {}), + ...(virtualFields ? { virtualFields } : {}), + ...(attributeForm ? { attributeForm } : {}), + }; + })(), ...(geojson ? { geojson: structuredClone(geojson) } : {}), // `needsLocalFile` only means anything with a path to read. ...(candidate.needsLocalFile === true && sourcePath ? { needsLocalFile: true } : {}), diff --git a/packages/core/src/paths.ts b/packages/core/src/paths.ts index abd41fdad..68d8227e4 100644 --- a/packages/core/src/paths.ts +++ b/packages/core/src/paths.ts @@ -12,3 +12,24 @@ const PATH_TRAVERSAL = /(?:^|[/\\])\.\.(?:[/\\]|$)/; export function hasPathTraversal(path: string): boolean { return PATH_TRAVERSAL.test(path); } + +/** + * Whether a path is an absolute local filesystem path a host could read. + * + * Matches the raw path (not a trimmed copy): a whitespace-padded value would + * pass a trimmed check but reach the file read unchanged and fail there, so it + * is rejected up front. Accepts POSIX paths and Windows drive-letter paths only. + * UNC paths (`\\server\share`) are deliberately rejected: reading one can make + * Windows auto-authenticate against a remote host (NTLM hash capture), and a + * remote share is not a supported local data source. + * + * Lives in core so validation of a persisted path is available wherever a + * record is normalized — the desktop app's `isAbsoluteLocalPath` re-exports the + * same rule for its Tauri call sites. + * + * @param path - The path to check. + * @returns True when the path is an absolute POSIX or Windows path. + */ +export function isAbsoluteFilesystemPath(path: string): boolean { + return path.startsWith("/") || /^[a-z]:[\\/]/i.test(path); +} diff --git a/tests/layer-library.test.ts b/tests/layer-library.test.ts index 38f90957f..785288164 100644 --- a/tests/layer-library.test.ts +++ b/tests/layer-library.test.ts @@ -89,12 +89,37 @@ describe("hasRestorableLayerSource", () => { }); describe("canSaveLayerToLibrary", () => { - it("accepts a layer with a source, a local path, or features", () => { + it("accepts a layer with a source, a re-readable local path, or features", () => { assert.equal(canSaveLayerToLibrary(layer({ source: { url: "https://x/a.fgb" } })), true); - assert.equal(canSaveLayerToLibrary(layer({ sourcePath: "/data/cities.geojson" })), true); + assert.equal( + canSaveLayerToLibrary( + layer({ sourcePath: "/data/cities.geojson", metadata: { localFileReloadable: true } }), + ), + true, + ); assert.equal(canSaveLayerToLibrary(layer({ geojson: POINTS })), true); }); + it("does not treat a browser-picked file's bare name as a re-readable path", () => { + // A file picked in the browser has no path, but `createVectorStoreLayer` + // still records the bare name in `sourcePath` for display, and omits + // `localFileReloadable` — the flag that marks a genuinely re-readable path. + assert.equal(canSaveLayerToLibrary(layer({ sourcePath: "us_cities.geojson" })), false); + // Nor a relative or traversing path a hand-edited project could smuggle in. + assert.equal( + canSaveLayerToLibrary( + layer({ sourcePath: "data/cities.geojson", metadata: { localFileReloadable: true } }), + ), + false, + ); + assert.equal( + canSaveLayerToLibrary( + layer({ sourcePath: "/data/../../etc/passwd", metadata: { localFileReloadable: true } }), + ), + false, + ); + }); + it("rejects a layer with nothing to re-add from", () => { assert.equal(canSaveLayerToLibrary(layer()), false); assert.equal( @@ -184,14 +209,14 @@ describe("captureLayerLibraryEntry", () => { joins: [ { id: "join-1", - sourceLayerId: "layer-2", + joinLayerId: "layer-2", targetField: "id", - sourceField: "id", + joinField: "id", fields: ["pop"], }, ], virtualFields: [{ id: "vf-1", name: "double", expression: "1 + 1" }], - attributeForm: { fields: { name: { field: "name", widget: "text" } } }, + attributeForm: { fields: [{ field: "name", widget: "text" }] }, }), CAPTURE_OPTIONS, ); @@ -308,6 +333,7 @@ describe("captureLayerLibraryEntry", () => { const result = captureLayerLibraryEntry( layer({ sourcePath: "/data/huge.geojson", + metadata: { localFileReloadable: true }, geojson: bulkyFeatures(MAX_LAYER_LIBRARY_ENTRY_BYTES + 1_000), }), CAPTURE_OPTIONS, @@ -327,6 +353,19 @@ describe("captureLayerLibraryEntry", () => { ); assert.deepEqual(result, { ok: false, reason: "too-large" }); }); + + it("refuses an oversized browser-picked file, whose bare name cannot be re-read", () => { + // There is no path to degrade to, so this must fail rather than save an + // entry that could never be re-added. + const result = captureLayerLibraryEntry( + layer({ + sourcePath: "huge.geojson", + geojson: bulkyFeatures(MAX_LAYER_LIBRARY_ENTRY_BYTES + 1_000), + }), + CAPTURE_OPTIONS, + ); + assert.deepEqual(result, { ok: false, reason: "too-large" }); + }); }); describe("planLayerLibraryAdd", () => { @@ -359,7 +398,9 @@ describe("planLayerLibraryAdd", () => { assert.deepEqual(plan.layer.geojson, POINTS); }); - it("asks the host to re-read the file for a path-only entry", () => { + it("asks the host to re-read the file for a path-only entry, carrying its config", () => { + // The host's file import builds a default-styled layer, so the entry's saved + // configuration has to travel with the plan or the re-add loses it. const plan = planLayerLibraryAdd( { id: "entry-1", @@ -367,15 +408,47 @@ describe("planLayerLibraryAdd", () => { addedAt: "", layerType: "geojson", source: {}, - style: DEFAULT_LAYER_STYLE, - opacity: 1, + style: { ...DEFAULT_LAYER_STYLE, fillColor: "#e11d48" }, + opacity: 0.4, metadata: {}, sourcePath: "/data/huge.geojson", needsLocalFile: true, + joins: [ + { id: "j1", joinLayerId: "layer-2", targetField: "id", joinField: "id", fields: ["pop"] }, + ], + virtualFields: [{ id: "vf1", name: "double", expression: "1 + 1" }], }, { id: "layer-new" }, ); - assert.deepEqual(plan, { kind: "local-file", path: "/data/huge.geojson", name: "Huge" }); + assert.equal(plan.kind, "local-file"); + if (plan.kind !== "local-file") return; + assert.equal(plan.path, "/data/huge.geojson"); + assert.equal(plan.config.name, "Huge"); + assert.equal(plan.config.opacity, 0.4); + assert.equal(plan.config.style.fillColor, "#e11d48"); + assert.equal(plan.config.joins?.length, 1); + assert.equal(plan.config.virtualFields?.length, 1); + }); + + it("adds a path-only entry as a layer when its path is not re-readable", () => { + // A hand-edited bundle can claim needsLocalFile with a relative path; the + // plan must not hand that to the host's file read. + const plan = planLayerLibraryAdd( + { + id: "entry-1", + name: "Sketchy", + addedAt: "", + layerType: "geojson", + source: { data: "https://example.com/a.geojson" }, + style: { ...DEFAULT_LAYER_STYLE }, + opacity: 1, + metadata: {}, + sourcePath: "../../etc/passwd", + needsLocalFile: true, + }, + { id: "layer-new" }, + ); + assert.equal(plan.kind, "layer"); }); it("does not alias the entry, so re-adding twice yields independent layers", () => { @@ -469,6 +542,23 @@ describe("normalizeLayerLibraryEntries", () => { assert.equal(entry.needsLocalFile, undefined); }); + it("drops a sourcePath an importing host must not be told to read", () => { + // A bundle is shareable, so a crafted entry could otherwise aim the host's + // file read at any path on the importing user's disk. + for (const path of ["cities.geojson", "data/cities.geojson", "/data/../../etc/shadow"]) { + const [entry] = normalizeLayerLibraryEntries([ + { ...valid, sourcePath: path, needsLocalFile: true }, + ]); + assert.equal(entry.sourcePath, undefined, path); + assert.equal(entry.needsLocalFile, undefined, path); + } + const [kept] = normalizeLayerLibraryEntries([ + { ...valid, sourcePath: "/data/cities.geojson", needsLocalFile: true }, + ]); + assert.equal(kept.sourcePath, "/data/cities.geojson"); + assert.equal(kept.needsLocalFile, true); + }); + it("drops malformed optional blocks rather than the whole entry", () => { const [entry] = normalizeLayerLibraryEntries([ { ...valid, joins: "many", virtualFields: [], attributeForm: [], geojson: { type: "Point" } }, @@ -478,6 +568,49 @@ describe("normalizeLayerLibraryEntries", () => { assert.equal(entry.attributeForm, undefined); assert.equal(entry.geojson, undefined); }); + + it("drops join, virtual-field, and form members missing the keys their engine needs", () => { + const [entry] = normalizeLayerLibraryEntries([ + { + ...valid, + joins: [ + { id: "j1", joinLayerId: "l2", targetField: "id", joinField: "id" }, + { id: "j2", joinLayerId: "l3" }, + ], + virtualFields: [ + { id: "vf1", name: "ok", expression: "1" }, + { id: "vf2", name: "no expression" }, + ], + attributeForm: { fields: [{ field: "name", widget: "text" }, { widget: "text" }] }, + }, + ]); + assert.deepEqual( + entry.joins?.map((join) => join.id), + ["j1"], + ); + assert.deepEqual( + entry.virtualFields?.map((field) => field.id), + ["vf1"], + ); + assert.deepEqual( + entry.attributeForm?.fields.map((field) => field.field), + ["name"], + ); + }); + + it("drops a block entirely when no member survives", () => { + const [entry] = normalizeLayerLibraryEntries([ + { + ...valid, + joins: [{ id: "j1" }], + virtualFields: [{ name: "nameless" }], + attributeForm: { fields: [{ widget: "text" }] }, + }, + ]); + assert.equal(entry.joins, undefined); + assert.equal(entry.virtualFields, undefined); + assert.equal(entry.attributeForm, undefined); + }); }); describe("serializeLayerLibrary / parseLayerLibrary", () => { From 0c516cbf503b2cfd43a4b31ede3a19e81053898d Mon Sep 17 00:00:00 2001 From: giswqs Date: Thu, 30 Jul 2026 00:22:08 -0400 Subject: [PATCH 04/13] Address round-3 review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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. --- .../src/components/panels/BrowserPanel.tsx | 34 +++++++--- packages/core/src/layer-library.ts | 64 ++++++++++++++---- tests/layer-library.test.ts | 65 +++++++++++++++++++ 3 files changed, 142 insertions(+), 21 deletions(-) diff --git a/apps/geolibre-desktop/src/components/panels/BrowserPanel.tsx b/apps/geolibre-desktop/src/components/panels/BrowserPanel.tsx index c26a55d3b..db1035a4a 100644 --- a/apps/geolibre-desktop/src/components/panels/BrowserPanel.tsx +++ b/apps/geolibre-desktop/src/components/panels/BrowserPanel.tsx @@ -4,6 +4,7 @@ import { planLayerLibraryAdd, serializeLayerLibrary, useAppStore, + type GeoLibreLayer, } from "@geolibre/core"; import type { MapController } from "@geolibre/map"; import { fetchPostgisStatus, listPostgisTables } from "@geolibre/processing"; @@ -70,6 +71,16 @@ const DEFAULT_EXPANDED = new Set([ "section:files", ]); +/** + * Whether a layer records `path` as the file it was read from. The vector import + * puts the absolute path in `sourcePath`; the raster control puts it in + * `metadata.localFilePath` and keeps only the basename in `sourcePath`. Used to + * pick the layer a library re-add's file import just produced. + */ +function layerCameFromPath(layer: GeoLibreLayer, path: string): boolean { + return layer.sourcePath === path || layer.metadata.localFilePath === path; +} + /** Collects every group node id in a tree (used to expand-all while searching). */ function collectGroupIds(nodes: readonly BrowserNode[], into: Set): void { for (const node of nodes) { @@ -489,24 +500,31 @@ export function BrowserPanel({ try { // The file import builds a fresh, default-styled layer, so the entry's // saved configuration is applied to it afterwards — otherwise a degraded - // entry would silently come back unstyled. The import does not report - // which layer it created, so it is identified by diffing the ids around - // the call (a concurrent add elsewhere would leave more than one new id, - // in which case nothing is patched rather than the wrong layer). + // entry would silently come back unstyled. `onAddFilePath` reports only + // success or an error message (one file can yield several layers — a KML + // adds placemarks plus ground overlays and models), so the target is + // found among the newly-added ids by matching the path this add used. + // That stays correct if an unrelated layer lands concurrently, and + // patches nothing rather than the wrong layer if none matches. const idsBefore = new Set(useAppStore.getState().layers.map((entry) => entry.id)); const addError = await onAddFilePath(plan.path); if (addError) { setError(addError); } else { const added = useAppStore.getState().layers.filter((entry) => !idsBefore.has(entry.id)); - if (added.length === 1) { + const target = + added.find((entry) => layerCameFromPath(entry, plan.path)) ?? + // Nothing recorded the path (a format that keeps neither): fall back + // to a lone new layer, which is unambiguous. + (added.length === 1 ? added[0] : undefined); + if (target) { const { joins, virtualFields, ...rest } = plan.config; - useAppStore.getState().updateLayer(added[0].id, rest); + 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(added[0].id, joins); + if (joins) useAppStore.getState().setLayerJoins(target.id, joins); if (virtualFields) { - useAppStore.getState().setLayerVirtualFields(added[0].id, virtualFields); + useAppStore.getState().setLayerVirtualFields(target.id, virtualFields); } } } diff --git a/packages/core/src/layer-library.ts b/packages/core/src/layer-library.ts index 953953198..39d37cbde 100644 --- a/packages/core/src/layer-library.ts +++ b/packages/core/src/layer-library.ts @@ -74,20 +74,30 @@ export function hasRestorableLayerSource( } /** - * The absolute local path a layer's features can be **re-read** from, or - * undefined. + * The absolute local path a layer's data can be **re-read** from, or undefined. * * A non-empty `sourcePath` is not enough on its own: a file picked in the - * browser has no path, and `createVectorStoreLayer` still records the bare file - * *name* there for display. `metadata.localFileReloadable` is the flag the rest - * of the codebase uses to mark a genuinely re-readable absolute path - * (`prepareLayerForSave`, `needsLocalFileReload`, `restoreVectorLayers`), so - * require it — otherwise a degraded path-only entry would store a bare filename - * no host could ever open. The path is re-validated because a hand-edited - * project can set both fields to anything. + * browser has no path, and both the vector and raster controls still record the + * bare file *name* there for display. Each control marks a genuinely re-readable + * absolute path differently, so both conventions are honored: + * + * - Vector / plain local GeoJSON: `metadata.localFileReloadable` alongside an + * absolute `sourcePath` (`prepareLayerForSave`, `needsLocalFileReload`, + * `restoreVectorLayers`). + * - Raster / COG: `metadata.localFilePath` holds the absolute path outright, + * and it is what `restoreRasterLayers` re-reads on restore. Without this + * branch a locally-opened COG could not be saved at all, even though its + * restore pass is fully able to replay it. + * + * Every candidate is re-validated, because a hand-edited project or an imported + * bundle can set these fields to anything. */ function layerLocalPath(layer: Pick): string | undefined { - if ((layer.metadata ?? {}).localFileReloadable !== true) return undefined; + const metadata = layer.metadata ?? {}; + if (nonEmptyString(metadata.localFilePath) && isReReadablePath(metadata.localFilePath)) { + return metadata.localFilePath; + } + if (metadata.localFileReloadable !== true) return undefined; return nonEmptyString(layer.sourcePath) && isReReadablePath(layer.sourcePath) ? layer.sourcePath : undefined; @@ -478,7 +488,11 @@ export function normalizeLayerLibraryEntries(value: unknown): LayerLibraryEntry[ : undefined; if (!id || !name || !layerType || seen.has(id)) continue; const source = plainObject(candidate.source) ?? {}; - const metadata = plainObject(candidate.metadata) ?? {}; + const metadata = { ...(plainObject(candidate.metadata) ?? {}) }; + // Strip the same per-session keys the capture path drops, so a stale one on + // an older record or an imported bundle is cleaned up on load instead of + // replaying a dev-server proxy rewrite indefinitely. + for (const key of TRANSIENT_METADATA_KEYS) delete metadata[key]; // A bundle is shareable, so its `sourcePath` is untrusted: keep it only when // it is an absolute, traversal-free path a host could legitimately re-read. // Without this a crafted entry could point `onAddFilePath` at any file on the @@ -538,9 +552,33 @@ export function normalizeLayerLibraryEntries(value: unknown): LayerLibraryEntry[ return entries; } +/** + * `source` fields that can hold a per-user credential and are therefore removed + * on export. `requestHeaders` carries the bearer token / API key an + * authenticated 3D Tiles layer was added with (see + * `persistedThreeDTilesRequestHeaders`, which keeps non-Google auth headers when + * persisting into a project file). + * + * Kept in the local IndexedDB entry so re-adding on this machine still works, + * and dropped only from the bundle: Export is pitched as a one-click "share with + * your team" action, which makes it a far more direct exposure path than handing + * someone a whole project file. A recipient re-enters their own credentials, + * which is the correct outcome. + */ +const EXPORT_REDACTED_SOURCE_KEYS = ["requestHeaders"] as const; + +/** Copy of an entry with per-user credentials removed from its source. */ +function redactEntryForExport(entry: LayerLibraryEntry): LayerLibraryEntry { + if (!EXPORT_REDACTED_SOURCE_KEYS.some((key) => key in entry.source)) return entry; + const source = { ...entry.source }; + for (const key of EXPORT_REDACTED_SOURCE_KEYS) delete source[key]; + return { ...entry, source }; +} + /** * Serialize Layer Library entries into the shareable bundle JSON written by the - * Export action and read back by {@link parseLayerLibrary}. + * Export action and read back by {@link parseLayerLibrary}. Per-user credentials + * are redacted first (see {@link EXPORT_REDACTED_SOURCE_KEYS}). * * @param entries - The entries to export. * @returns Pretty-printed bundle JSON. @@ -550,7 +588,7 @@ export function serializeLayerLibrary(entries: LayerLibraryEntry[]): string { { type: LAYER_LIBRARY_BUNDLE_TYPE, version: LAYER_LIBRARY_BUNDLE_VERSION, - entries, + entries: entries.map(redactEntryForExport), }, null, 2, diff --git a/tests/layer-library.test.ts b/tests/layer-library.test.ts index 785288164..eb40ab736 100644 --- a/tests/layer-library.test.ts +++ b/tests/layer-library.test.ts @@ -100,6 +100,25 @@ describe("canSaveLayerToLibrary", () => { assert.equal(canSaveLayerToLibrary(layer({ geojson: POINTS })), true); }); + it("accepts a locally-opened raster, whose absolute path lives in metadata", () => { + // The raster control records the absolute path in `metadata.localFilePath` + // (what restoreRasterLayers re-reads) and keeps only the basename in + // `sourcePath`, so requiring `localFileReloadable` alone would make a + // locally-opened COG unsaveable. + const cog = layer({ + type: "cog", + source: { type: "raster" }, + sourcePath: "dem.tif", + metadata: { + externalNativeLayer: true, + customLayerType: "raster", + sourceKind: "maplibre-gl-raster", + localFilePath: "/data/dem.tif", + }, + }); + assert.equal(canSaveLayerToLibrary(cog, { canRestoreControlPainted: () => true }), true); + }); + it("does not treat a browser-picked file's bare name as a re-readable path", () => { // A file picked in the browser has no path, but `createVectorStoreLayer` // still records the bare name in `sourcePath` for display, and omits @@ -635,6 +654,52 @@ describe("serializeLayerLibrary / parseLayerLibrary", () => { assert.deepEqual(parseLayerLibrary(json), entries); }); + it("redacts per-user credentials from the exported bundle only", () => { + // Export is a share action, so an authenticated 3D Tiles layer's bearer + // token must not travel with it — while the local entry keeps it so + // re-adding on this machine still works. + const [withToken] = normalizeLayerLibraryEntries([ + { + id: "e-auth", + name: "Authenticated tileset", + addedAt: "", + layerType: "3d-tiles", + source: { + url: "https://example.com/tileset.json", + requestHeaders: { Authorization: "Bearer super-secret" }, + }, + opacity: 1, + metadata: {}, + }, + ]); + assert.deepEqual(withToken.source.requestHeaders, { Authorization: "Bearer super-secret" }); + const exported = serializeLayerLibrary([withToken]); + assert.equal(exported.includes("super-secret"), false); + const [reimported] = parseLayerLibrary(exported); + assert.equal("requestHeaders" in reimported.source, false); + // The rest of the source survives, so the recipient only re-enters the token. + assert.equal(reimported.source.url, "https://example.com/tileset.json"); + }); + + it("strips a stale per-session metadata key when reading entries back", () => { + // useLayerLibraryPersistence normalizes on every startup load, so an older + // record (or a hand-edited bundle) must not keep replaying a dev-server + // proxy rewrite. + const [entry] = normalizeLayerLibraryEntries([ + { + id: "e-stale", + name: "XYZ", + addedAt: "", + layerType: "xyz", + source: { tiles: ["https://tiles.example.com/{z}/{x}/{y}.png"] }, + opacity: 1, + metadata: { originalUrl: "https://tiles.example.com/{z}/{x}/{y}.png", resolvedUrl: "/p/x" }, + }, + ]); + assert.equal("resolvedUrl" in entry.metadata, false); + assert.equal(entry.metadata.originalUrl, "https://tiles.example.com/{z}/{x}/{y}.png"); + }); + it("accepts a hand-authored bare array", () => { assert.deepEqual(parseLayerLibrary(JSON.stringify(entries)), entries); }); From f4ce0bf00b934ded5e26c35429e029673e9502d2 Mon Sep 17 00:00:00 2001 From: giswqs Date: Thu, 30 Jul 2026 00:30:28 -0400 Subject: [PATCH 05/13] Address round-4 review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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. --- .../src/components/panels/BrowserPanel.tsx | 19 +++++++-- .../geolibre-desktop/src/i18n/locales/en.json | 3 +- packages/core/src/layer-library.ts | 18 ++++++-- tests/layer-library.test.ts | 41 +++++++++++++++++++ 4 files changed, 73 insertions(+), 8 deletions(-) diff --git a/apps/geolibre-desktop/src/components/panels/BrowserPanel.tsx b/apps/geolibre-desktop/src/components/panels/BrowserPanel.tsx index db1035a4a..468bdb365 100644 --- a/apps/geolibre-desktop/src/components/panels/BrowserPanel.tsx +++ b/apps/geolibre-desktop/src/components/panels/BrowserPanel.tsx @@ -512,11 +512,17 @@ export function BrowserPanel({ setError(addError); } else { const added = useAppStore.getState().layers.filter((entry) => !idsBefore.has(entry.id)); + const matches = added.filter((entry) => layerCameFromPath(entry, plan.path)); const target = - added.find((entry) => layerCameFromPath(entry, plan.path)) ?? - // Nothing recorded the path (a format that keeps neither): fall back - // to a lone new layer, which is unambiguous. - (added.length === 1 ? added[0] : undefined); + matches.length === 1 + ? matches[0] + : // Nothing recorded the path (a format that keeps neither): a lone + // new layer is still unambiguous. Several matches are not — one + // file can yield siblings that share a path (a KML's placemarks + // plus its ground overlays) — so patch none of them. + matches.length === 0 && added.length === 1 + ? added[0] + : undefined; if (target) { const { joins, virtualFields, ...rest } = plan.config; useAppStore.getState().updateLayer(target.id, rest); @@ -526,6 +532,11 @@ export function BrowserPanel({ if (virtualFields) { useAppStore.getState().setLayerVirtualFields(target.id, virtualFields); } + } else if (added.length > 0) { + // The file came back but we cannot tell which layer the saved + // configuration belongs to. Say so, rather than leaving the user to + // notice their styling silently did not return. + setError(t("browser.libraryLayerConfigNotApplied", { name: plan.config.name })); } } } finally { diff --git a/apps/geolibre-desktop/src/i18n/locales/en.json b/apps/geolibre-desktop/src/i18n/locales/en.json index 717682ffb..ade17502e 100644 --- a/apps/geolibre-desktop/src/i18n/locales/en.json +++ b/apps/geolibre-desktop/src/i18n/locales/en.json @@ -35,7 +35,8 @@ "libraryImportFailed": "Could not read this layer library file.", "libraryExportFailed": "Could not export the layer library.", "libraryLayerMissing": "That saved layer no longer exists.", - "libraryLayerNeedsDesktop": "This saved layer reads a file from disk, so it needs GeoLibre Desktop. Add the file again to save a new entry." + "libraryLayerNeedsDesktop": "This saved layer reads a file from disk, so it needs GeoLibre Desktop. Add the file again to save a new entry.", + "libraryLayerConfigNotApplied": "Added the file for “{{name}}”, but its saved styling could not be matched to a single layer, so it was not reapplied." }, "about": { "trigger": "About", diff --git a/packages/core/src/layer-library.ts b/packages/core/src/layer-library.ts index 39d37cbde..a01a48a0f 100644 --- a/packages/core/src/layer-library.ts +++ b/packages/core/src/layer-library.ts @@ -493,6 +493,16 @@ export function normalizeLayerLibraryEntries(value: unknown): LayerLibraryEntry[ // an older record or an imported bundle is cleaned up on load instead of // replaying a dev-server proxy rewrite indefinitely. for (const key of TRANSIENT_METADATA_KEYS) delete metadata[key]; + // `metadata.localFilePath` is the *second* path in an entry, and the one + // `restoreRasterLayers` re-reads — so it needs the same validation as + // `sourcePath` below, not a free pass because it happens to live in + // metadata. Without this a shared bundle could keep a valid-looking + // `sourcePath` while pointing the raster restore at any file on the + // importing user's disk. + if ("localFilePath" in metadata) { + const path = metadata.localFilePath; + if (!nonEmptyString(path) || !isReReadablePath(path)) delete metadata.localFilePath; + } // A bundle is shareable, so its `sourcePath` is untrusted: keep it only when // it is an absolute, traversal-free path a host could legitimately re-read. // Without this a crafted entry could point `onAddFilePath` at any file on the @@ -503,12 +513,14 @@ export function normalizeLayerLibraryEntries(value: unknown): LayerLibraryEntry[ : undefined; const geojson = featureCollection(candidate.geojson); // Same "is there anything to re-add from" gate as the capture path, so a - // hand-edited bundle cannot introduce an entry that always fails. A - // control-painted entry carries its features in `metadata.embeddedGeoJSON` - // instead of `geojson`, so that counts too. + // hand-edited bundle cannot introduce an entry that always fails. Both path + // conventions count (a local raster's only path is `metadata.localFilePath`, + // already validated above), and so does a control-painted entry's + // `metadata.embeddedGeoJSON`, which holds its features instead of `geojson`. if ( !hasRestorableLayerSource({ source, metadata }) && !sourcePath && + !nonEmptyString(metadata.localFilePath) && featureCount(geojson) === 0 && featureCount(featureCollection(metadata.embeddedGeoJSON)) === 0 ) { diff --git a/tests/layer-library.test.ts b/tests/layer-library.test.ts index eb40ab736..8afa46aa1 100644 --- a/tests/layer-library.test.ts +++ b/tests/layer-library.test.ts @@ -578,6 +578,47 @@ describe("normalizeLayerLibraryEntries", () => { assert.equal(kept.needsLocalFile, true); }); + it("validates metadata.localFilePath, the second path a raster entry carries", () => { + // `restoreRasterLayers` re-reads this field, so a bundle must not be able to + // keep a clean-looking `sourcePath` while redirecting that read elsewhere. + const rasterEntry = { + ...valid, + layerType: "cog", + source: { type: "raster" }, + sourcePath: "/safe/file.tif", + }; + for (const path of ["/data/../../etc/shadow", "file.tif", "", 42]) { + const [entry] = normalizeLayerLibraryEntries([ + { ...rasterEntry, metadata: { localFilePath: path } }, + ]); + assert.equal(entry.metadata.localFilePath, undefined, String(path)); + // The validated sourcePath is unaffected by dropping the bad metadata path. + assert.equal(entry.sourcePath, "/safe/file.tif"); + } + const [kept] = normalizeLayerLibraryEntries([ + { ...rasterEntry, metadata: { localFilePath: "/data/dem.tif" } }, + ]); + assert.equal(kept.metadata.localFilePath, "/data/dem.tif"); + }); + + it("keeps a local raster entry whose only path is metadata.localFilePath", () => { + // Its source has no URL and it has no features, so the "anything to re-add + // from" gate has to count that path or the entry would vanish on load. + const entries = normalizeLayerLibraryEntries([ + { + ...valid, + layerType: "cog", + source: { type: "raster" }, + sourcePath: "dem.tif", + metadata: { sourceKind: "maplibre-gl-raster", localFilePath: "/data/dem.tif" }, + }, + ]); + assert.equal(entries.length, 1); + assert.equal(entries[0].metadata.localFilePath, "/data/dem.tif"); + // The basename is not a re-readable path, so it is not kept as one. + assert.equal(entries[0].sourcePath, undefined); + }); + it("drops malformed optional blocks rather than the whole entry", () => { const [entry] = normalizeLayerLibraryEntries([ { ...valid, joins: "many", virtualFields: [], attributeForm: [], geojson: { type: "Point" } }, From 8134c4953eddb26e4e7699889fdb044448e8c8b5 Mon Sep 17 00:00:00 2001 From: giswqs Date: Thu, 30 Jul 2026 00:37:09 -0400 Subject: [PATCH 06/13] Address round-5 review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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). --- .../src/lib/restore-library-layer.ts | 23 +++-- packages/core/src/layer-library.ts | 98 ++++++++++++++++++- packages/plugins/src/index.ts | 3 + .../plugins/src/plugins/maplibre-3d-tiles.ts | 11 ++- .../src/plugins/maplibre-components.ts | 11 ++- .../plugins/maplibre-planetary-computer.ts | 11 ++- tests/layer-library.test.ts | 53 ++++++++++ 7 files changed, 192 insertions(+), 18 deletions(-) diff --git a/apps/geolibre-desktop/src/lib/restore-library-layer.ts b/apps/geolibre-desktop/src/lib/restore-library-layer.ts index 41932b4fa..13117bf90 100644 --- a/apps/geolibre-desktop/src/lib/restore-library-layer.ts +++ b/apps/geolibre-desktop/src/lib/restore-library-layer.ts @@ -18,31 +18,36 @@ import { type GeoLibreLayer, } from "@geolibre/core"; import { + LIDAR_SOURCE_KIND, + PLANETARY_COMPUTER_SOURCE_KIND, RASTER_SOURCE_KIND, restoreLidarLayers, restorePlanetaryComputerLayers, restoreRasterLayers, restoreThreeDTilesLayers, restoreVectorLayers, + THREE_D_TILES_SOURCE_KIND, VECTOR_SOURCE_KIND, type GeoLibreAppAPI, } from "@geolibre/plugins"; /** * `metadata.sourceKind` values of the control-painted layer kinds whose restore - * pass we can re-run, mapped to that pass. The string keys mirror the values - * each plugin writes onto its store layers; the two that export a constant use - * it. A layer whose kind is absent here is left to the store sync, which is - * correct for every GeoLibre-rendered layer — and for a control-painted kind not - * listed, it means the layer re-adds unrendered, which is why "Save to My Data" - * is offered only for the kinds this map (or a plain store add) covers. + * pass we can re-run, mapped to that pass. Every key is the constant its own + * plugin writes onto the store layer — never a copy of the literal — because the + * failure mode of a drifted key is silent: "Save to My Data" simply stops + * appearing for that layer type, and an entry saved earlier re-adds unrendered. + * + * A layer whose kind is absent here is left to the store sync, which is correct + * for every GeoLibre-rendered layer; for a control-painted kind, being absent is + * what makes `canRestoreLibraryLayer` refuse to offer it in the first place. */ const RESTORE_BY_SOURCE_KIND: Record void | Promise> = { [VECTOR_SOURCE_KIND]: restoreVectorLayers, [RASTER_SOURCE_KIND]: restoreRasterLayers, - "planetary-computer-raster": restorePlanetaryComputerLayers, - "3d-tiles-url": restoreThreeDTilesLayers, - "lidar-url": restoreLidarLayers, + [PLANETARY_COMPUTER_SOURCE_KIND]: restorePlanetaryComputerLayers, + [THREE_D_TILES_SOURCE_KIND]: restoreThreeDTilesLayers, + [LIDAR_SOURCE_KIND]: restoreLidarLayers, }; /** The restore pass that renders `layer`, or undefined when it needs none. */ diff --git a/packages/core/src/layer-library.ts b/packages/core/src/layer-library.ts index a01a48a0f..7b6ba293d 100644 --- a/packages/core/src/layer-library.ts +++ b/packages/core/src/layer-library.ts @@ -579,12 +579,104 @@ export function normalizeLayerLibraryEntries(value: unknown): LayerLibraryEntry[ */ const EXPORT_REDACTED_SOURCE_KEYS = ["requestHeaders"] as const; +/** + * Query-parameter names removed from a source URL on export. Plenty of tile and + * web services authenticate in the URL rather than a header — an XYZ template + * with `?api_key=`, a Google `key=`, a Mapbox `access_token=`, an S3 presigned + * link (`X-Amz-*`), an Azure SAS token (`sig`/`se`/`sp`/…) — so redacting only + * `requestHeaders` would still ship a credential in a shared bundle. + * + * Matched case-insensitively, plus any `x-amz-*` parameter. Erring toward + * redaction is deliberate for a share action: a stripped parameter leaves the + * recipient re-entering their own key, while a missed one leaks yours. Every + * other part of the URL is preserved, so the entry stays recognizable and + * re-usable once the recipient supplies their credential. + */ +const EXPORT_REDACTED_URL_PARAMS = new Set([ + "access_token", + "accesstoken", + "api_key", + "apikey", + "key", + "token", + "subscription-key", + "subscriptionkey", + "signature", + "sig", + "se", + "sp", + "sv", + "sr", + "st", + "skoid", +]); + +/** True when a query-parameter name is treated as a credential on export. */ +function isRedactedUrlParam(name: string): boolean { + const lower = name.toLowerCase(); + return EXPORT_REDACTED_URL_PARAMS.has(lower) || lower.startsWith("x-amz-"); +} + +/** + * Strip credential-bearing query parameters from a URL, leaving everything else + * intact. A value that is not a parseable absolute URL (an XYZ template with + * `{z}/{x}/{y}` placeholders parses fine; a relative path or a `pmtiles://` + * form may not) is returned unchanged rather than mangled. + */ +function redactUrlCredentials(value: string): string { + const separator = value.indexOf("?"); + if (separator === -1) return value; + const [base, rawQuery] = [value.slice(0, separator), value.slice(separator + 1)]; + // Split by hand rather than through URL/URLSearchParams: those re-encode the + // `{z}/{x}/{y}` placeholders a tile template depends on. + const [query, fragment] = rawQuery.split("#", 2); + const kept = query + .split("&") + .filter((pair) => pair !== "" && !isRedactedUrlParam(pair.split("=", 1)[0])); + const rebuiltQuery = kept.length > 0 ? `?${kept.join("&")}` : ""; + return `${base}${rebuiltQuery}${fragment === undefined ? "" : `#${fragment}`}`; +} + /** Copy of an entry with per-user credentials removed from its source. */ function redactEntryForExport(entry: LayerLibraryEntry): LayerLibraryEntry { - if (!EXPORT_REDACTED_SOURCE_KEYS.some((key) => key in entry.source)) return entry; const source = { ...entry.source }; - for (const key of EXPORT_REDACTED_SOURCE_KEYS) delete source[key]; - return { ...entry, source }; + let changed = false; + for (const key of EXPORT_REDACTED_SOURCE_KEYS) { + if (key in source) { + delete source[key]; + changed = true; + } + } + if (typeof source.url === "string") { + const redacted = redactUrlCredentials(source.url); + if (redacted !== source.url) { + source.url = redacted; + changed = true; + } + } + const originalTiles: unknown[] | undefined = Array.isArray(source.tiles) + ? source.tiles + : undefined; + if (originalTiles) { + const tiles = originalTiles.map((tile) => + typeof tile === "string" ? redactUrlCredentials(tile) : tile, + ); + if (tiles.some((tile, index) => tile !== originalTiles[index])) { + source.tiles = tiles; + 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; } /** diff --git a/packages/plugins/src/index.ts b/packages/plugins/src/index.ts index 6f07252ec..ae3e42151 100644 --- a/packages/plugins/src/index.ts +++ b/packages/plugins/src/index.ts @@ -92,6 +92,7 @@ export { openHtmlPanel, openLegendPanel, openLegendPanelWithItems, + LIDAR_SOURCE_KIND, openLidarLayerPanel, restoreLidarLayers, openMeasurePanel, @@ -170,6 +171,7 @@ export { export { closePlanetaryComputerPanel, openPlanetaryComputerPanel, + PLANETARY_COMPUTER_SOURCE_KIND, restorePlanetaryComputerLayers, } from "./plugins/maplibre-planetary-computer"; export { @@ -183,6 +185,7 @@ export { closeThreeDTilesLayerPanel, openThreeDTilesLayerPanel, restoreThreeDTilesLayers, + THREE_D_TILES_SOURCE_KIND, } from "./plugins/maplibre-3d-tiles"; export { addRasterToMap, diff --git a/packages/plugins/src/plugins/maplibre-3d-tiles.ts b/packages/plugins/src/plugins/maplibre-3d-tiles.ts index 5a5656da4..44f34a486 100644 --- a/packages/plugins/src/plugins/maplibre-3d-tiles.ts +++ b/packages/plugins/src/plugins/maplibre-3d-tiles.ts @@ -35,6 +35,13 @@ import { THREE_D_TILES_DECK_LOAD_OPTIONS, } from "./arcgis-i3s-tiles"; +/** + * `metadata.sourceKind` marking the 3D Tiles layers this plugin adds. Exported so the Layer Library's restore + * dispatch keys off the same value this plugin writes rather than a hand-typed + * copy (issue #1520). + */ +export const THREE_D_TILES_SOURCE_KIND = "3d-tiles-url"; + const threeDTilesControlPosition: GeoLibreMapControlPosition = "top-left"; const THREE_D_TILES_LAYER_ID = "geolibre-3d-tiles"; // Keep in sync with the three.js version maplibre-gl-3d-tiles is built @@ -517,7 +524,7 @@ function createThreeDTilesStoreLayer( nativeLayerIds: [tileset.layerId], panelCollapsed, sourceId: tileset.id, - sourceKind: "3d-tiles-url", + sourceKind: THREE_D_TILES_SOURCE_KIND, status: tileset.status, }, sourcePath: tileset.tilesetUrl, @@ -575,7 +582,7 @@ function resetThreeDTilesControl(control: ThreeDTilesControl | null): void { function isThreeDTilesControlLayer(layer: GeoLibreLayer): boolean { return ( layer.type === "3d-tiles" && - layer.metadata.sourceKind === "3d-tiles-url" && + layer.metadata.sourceKind === THREE_D_TILES_SOURCE_KIND && layer.metadata.externalNativeLayer === true && !isGooglePhotorealisticTilesetLayerUrl(layer) ); diff --git a/packages/plugins/src/plugins/maplibre-components.ts b/packages/plugins/src/plugins/maplibre-components.ts index ec86ff1d4..f542f6d42 100644 --- a/packages/plugins/src/plugins/maplibre-components.ts +++ b/packages/plugins/src/plugins/maplibre-components.ts @@ -94,6 +94,13 @@ import { type ZarrDirectoryReader, } from "./zarr-directory-store"; +/** + * `metadata.sourceKind` marking the LiDAR point-cloud layers this plugin adds. Exported so the Layer Library's + * restore dispatch keys off the same value this plugin writes rather than a + * hand-typed copy (issue #1520). + */ +export const LIDAR_SOURCE_KIND = "lidar-url"; + type ControlGridConstructor = (typeof import("maplibre-gl-components"))["ControlGrid"]; type AddVectorControlConstructor = (typeof import("maplibre-gl-components"))["AddVectorControl"]; type BookmarkControlConstructor = (typeof import("maplibre-gl-components"))["BookmarkControl"]; @@ -5433,7 +5440,7 @@ function createLidarStoreLayer(pointCloud: PointCloudInfo): GeoLibreLayer { identifiable: false, pointCount: pointCloud.pointCount, sourceId: pointCloud.id, - sourceKind: "lidar-url", + sourceKind: LIDAR_SOURCE_KIND, wkt: pointCloud.wkt, }, sourcePath: pointCloud.source, @@ -5522,7 +5529,7 @@ function isStacSearchControlLayer(layer: GeoLibreLayer): boolean { function isLidarControlLayer(layer: GeoLibreLayer): boolean { return ( layer.type === "lidar" && - layer.metadata.sourceKind === "lidar-url" && + layer.metadata.sourceKind === LIDAR_SOURCE_KIND && layer.metadata.externalNativeLayer === true ); } diff --git a/packages/plugins/src/plugins/maplibre-planetary-computer.ts b/packages/plugins/src/plugins/maplibre-planetary-computer.ts index 8b992df74..60a97ca8a 100644 --- a/packages/plugins/src/plugins/maplibre-planetary-computer.ts +++ b/packages/plugins/src/plugins/maplibre-planetary-computer.ts @@ -12,6 +12,13 @@ import type { } from "maplibre-gl-planetary-computer"; import type { GeoLibreAppAPI, GeoLibreMapControlPosition } from "../types"; +/** + * `metadata.sourceKind` marking the Planetary Computer raster layers this plugin adds. Exported so the + * Layer Library's restore dispatch keys off the same value this plugin writes + * rather than a hand-typed copy (issue #1520). + */ +export const PLANETARY_COMPUTER_SOURCE_KIND = "planetary-computer-raster"; + type PlanetaryComputerControlConstructor = (typeof import("maplibre-gl-planetary-computer"))["PlanetaryComputerControl"]; type STACClientConstructor = (typeof import("maplibre-gl-planetary-computer"))["STACClient"]; @@ -248,7 +255,7 @@ function createPlanetaryComputerStoreLayer(activeLayer: ActiveLayer): GeoLibreLa renderParams: activeLayer.renderParams, sourceId: activeLayer.sourceId, sourceIds: [activeLayer.sourceId], - sourceKind: "planetary-computer-raster", + sourceKind: PLANETARY_COMPUTER_SOURCE_KIND, stacCollectionId: collectionId, stacItemId: activeLayer.item?.id, tileType: "raster", @@ -699,7 +706,7 @@ function showPlanetaryComputerControl(control: PlanetaryComputerControl | null): function isPlanetaryComputerLayer(layer: GeoLibreLayer): boolean { return ( layer.type === "raster" && - layer.metadata.sourceKind === "planetary-computer-raster" && + layer.metadata.sourceKind === PLANETARY_COMPUTER_SOURCE_KIND && layer.metadata.externalNativeLayer === true ); } diff --git a/tests/layer-library.test.ts b/tests/layer-library.test.ts index 8afa46aa1..9bea57891 100644 --- a/tests/layer-library.test.ts +++ b/tests/layer-library.test.ts @@ -722,6 +722,59 @@ describe("serializeLayerLibrary / parseLayerLibrary", () => { assert.equal(reimported.source.url, "https://example.com/tileset.json"); }); + it("redacts credentials embedded in source URLs and tile templates", () => { + // Plenty of services authenticate in the URL rather than a header, so + // redacting only requestHeaders would still ship a key in a shared bundle. + const [entry] = normalizeLayerLibraryEntries([ + { + id: "e-key", + name: "Keyed tiles", + addedAt: "", + layerType: "xyz", + source: { + url: "https://tiles.example.com/service?api_key=SECRET1&style=dark", + tiles: ["https://tiles.example.com/{z}/{x}/{y}.png?access_token=SECRET2&tileSize=512"], + }, + opacity: 1, + metadata: { originalUrl: "https://tiles.example.com/{z}/{x}/{y}.png?key=SECRET3" }, + }, + ]); + const exported = serializeLayerLibrary([entry]); + for (const secret of ["SECRET1", "SECRET2", "SECRET3"]) { + assert.equal(exported.includes(secret), false, secret); + } + const [reimported] = parseLayerLibrary(exported); + // Everything that is not a credential survives, including the tile + // placeholders, so the entry stays usable once a key is supplied again. + assert.equal(reimported.source.url, "https://tiles.example.com/service?style=dark"); + assert.deepEqual(reimported.source.tiles, [ + "https://tiles.example.com/{z}/{x}/{y}.png?tileSize=512", + ]); + assert.equal(reimported.metadata.originalUrl, "https://tiles.example.com/{z}/{x}/{y}.png"); + // The in-memory entry is untouched, so re-adding locally still works. + assert.equal(entry.source.url, "https://tiles.example.com/service?api_key=SECRET1&style=dark"); + }); + + it("leaves a credential-free URL byte-identical", () => { + const [entry] = normalizeLayerLibraryEntries([ + { + id: "e-plain", + name: "Plain tiles", + addedAt: "", + layerType: "wms", + source: { + url: "https://wms.example.com/service?SERVICE=WMS&LAYERS=a,b&FORMAT=image/png", + tiles: ["https://t.example.com/{z}/{x}/{y}.png"], + }, + opacity: 1, + metadata: {}, + }, + ]); + const [reimported] = parseLayerLibrary(serializeLayerLibrary([entry])); + assert.equal(reimported.source.url, entry.source.url); + assert.deepEqual(reimported.source.tiles, entry.source.tiles); + }); + it("strips a stale per-session metadata key when reading entries back", () => { // useLayerLibraryPersistence normalizes on every startup load, so an older // record (or a hand-edited bundle) must not keep replaying a dev-server From 195a62863614abc496a3b610404ef8e136329ca9 Mon Sep 17 00:00:00 2001 From: giswqs Date: Thu, 30 Jul 2026 00:42:28 -0400 Subject: [PATCH 07/13] Address round-6 review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Let the saveability gate see features that live only in a control. A control-painted layer's features are not in the store record — a tiles-mode Add Vector Layer layer has no `layer.geojson` at all — yet the save handler already reads them out of the control via `materializeEmbeddableVectorLayers`. The gate was therefore able to hide a layer the capture path could embed. `canSaveLayerToLibrary` now takes `hasMaterializableFeatures` (the app passes `isEmbeddableLocalVectorLayer`, the same predicate the handler uses), so gate and capture agree instead of depending on whether something happened to populate `layer.geojson`. New tests cover the true no-path case at both the gate and the capture, which the previous fixtures did not. - Fold the non-empty check into `isReReadablePath` so every path-like field goes through one predicate. Splitting the two halves is what let `sourcePath` and `metadata.localFilePath` drift apart in the first place; all four call sites now share it. --- .../src/components/panels/LayerPanel.tsx | 5 ++ packages/core/src/layer-library.ts | 64 ++++++++++--------- tests/layer-library.test.ts | 56 ++++++++++++++++ 3 files changed, 95 insertions(+), 30 deletions(-) diff --git a/apps/geolibre-desktop/src/components/panels/LayerPanel.tsx b/apps/geolibre-desktop/src/components/panels/LayerPanel.tsx index d7984ad2e..0af34f22f 100644 --- a/apps/geolibre-desktop/src/components/panels/LayerPanel.tsx +++ b/apps/geolibre-desktop/src/components/panels/LayerPanel.tsx @@ -2608,8 +2608,13 @@ export function LayerPanel({ // way to render it again (issue #1520), so a layer with no source and // no features is excluded — and so is a control-painted layer whose // kind has no restore route, which would otherwise re-add blank. + // `hasMaterializableFeatures` is the same predicate the save handler + // uses to read features out of the vector control, so the menu never + // hides a layer the capture path could in fact embed (a tiles-mode + // Add Vector Layer layer has no `layer.geojson` to look at). const canSaveToLibrary = canSaveLayerToLibrary(layer, { canRestoreControlPainted: canRestoreLibraryLayer, + hasMaterializableFeatures: isEmbeddableLocalVectorLayer, }); // Copy/paste symbology (issue #1339). Vector-styled layers and // deck.gl rasters each copy their own style family; a paste only diff --git a/packages/core/src/layer-library.ts b/packages/core/src/layer-library.ts index 7b6ba293d..8d4a8f0c3 100644 --- a/packages/core/src/layer-library.ts +++ b/packages/core/src/layer-library.ts @@ -94,25 +94,27 @@ export function hasRestorableLayerSource( */ function layerLocalPath(layer: Pick): string | undefined { const metadata = layer.metadata ?? {}; - if (nonEmptyString(metadata.localFilePath) && isReReadablePath(metadata.localFilePath)) { - return metadata.localFilePath; - } + if (isReReadablePath(metadata.localFilePath)) return metadata.localFilePath; if (metadata.localFileReloadable !== true) return undefined; - return nonEmptyString(layer.sourcePath) && isReReadablePath(layer.sourcePath) - ? layer.sourcePath - : undefined; + return isReReadablePath(layer.sourcePath) ? layer.sourcePath : undefined; } /** - * Whether a persisted path is safe to hand to a host file read: absolute, with - * no `..` traversal. Applied to captured *and* imported entries, because a - * shared bundle is untrusted input — a crafted `needsLocalFile` entry could - * otherwise point at any file on the importing user's disk, and clicking it in - * My Data would read that file. Mirrors the client-side re-validation the - * project-restore paths already do before calling into Tauri. + * Whether a value is a path safe to hand to a host file read: a non-empty + * string, absolute, with no `..` traversal. Takes `unknown` and does the + * non-empty check itself so every path-like field — `sourcePath`, + * `metadata.localFilePath`, and any added later — goes through one predicate; + * splitting the two halves is what let those two fields drift apart in the + * first place. + * + * Applied to captured *and* imported entries, because a shared bundle is + * untrusted input: a crafted `needsLocalFile` entry could otherwise point at any + * file on the importing user's disk, and clicking it in My Data would read that + * file. Mirrors the client-side re-validation the project-restore paths already + * do before calling into Tauri. */ -function isReReadablePath(path: string): boolean { - return isAbsoluteFilesystemPath(path) && !hasPathTraversal(path); +function isReReadablePath(value: unknown): value is string { + return nonEmptyString(value) && isAbsoluteFilesystemPath(value) && !hasPathTraversal(value); } function featureCount(geojson: FeatureCollection | undefined): number { @@ -124,7 +126,11 @@ function featureCount(geojson: FeatureCollection | undefined): number { * layer actions menu gates on. Two things have to hold: * * 1. There is something to re-add *from*: a re-fetchable source, a local file - * path, or features to embed. + * path, or features to embed. A control-painted layer's features live in its + * control rather than the store record — a tiles-mode Add Vector Layer layer + * has no `layer.geojson` at all — so the caller can answer for that case with + * `hasMaterializableFeatures`, keeping this gate consistent with the capture + * path, which reads those features through `options.features`. * 2. The host can actually render it again. For a control-painted layer * ({@link isExternalNativeLayerRecord}) that means either the map's own sync * rebuilds it from the record or the owning plugin's restore pass can be @@ -140,17 +146,23 @@ function featureCount(geojson: FeatureCollection | undefined): number { * @param layer - The layer to test. * @param options - `canRestoreControlPainted` answers whether the host can * re-render a control-painted layer (the desktop app passes its - * `canRestoreLibraryLayer`). + * `canRestoreLibraryLayer`); `hasMaterializableFeatures` answers whether it can + * read the layer's features out of its control (the app passes + * `isEmbeddableLocalVectorLayer`, the same predicate its save handler uses). * @returns Whether the layer can be offered to the library. */ export function canSaveLayerToLibrary( layer: GeoLibreLayer, - options: { canRestoreControlPainted?: (layer: GeoLibreLayer) => boolean } = {}, + options: { + canRestoreControlPainted?: (layer: GeoLibreLayer) => boolean; + hasMaterializableFeatures?: (layer: GeoLibreLayer) => boolean; + } = {}, ): boolean { const hasSomethingToReAddFrom = hasRestorableLayerSource(layer) || layerLocalPath(layer) !== undefined || - featureCount(layer.geojson) > 0; + featureCount(layer.geojson) > 0 || + options.hasMaterializableFeatures?.(layer) === true; if (!hasSomethingToReAddFrom) return false; if (!isExternalNativeLayerRecord(layer)) return true; return options.canRestoreControlPainted?.(layer) === true; @@ -384,11 +396,7 @@ export function layerLibraryConfigPatch(entry: LayerLibraryEntry): LayerLibraryC * @returns True when only a filesystem-capable host can add it. */ export function layerLibraryEntryNeedsLocalFile(entry: LayerLibraryEntry): boolean { - return ( - entry.needsLocalFile === true && - nonEmptyString(entry.sourcePath) && - isReReadablePath(entry.sourcePath) - ); + return entry.needsLocalFile === true && isReReadablePath(entry.sourcePath); } /** A plain JSON object (not an array, not null), or undefined. */ @@ -499,18 +507,14 @@ export function normalizeLayerLibraryEntries(value: unknown): LayerLibraryEntry[ // metadata. Without this a shared bundle could keep a valid-looking // `sourcePath` while pointing the raster restore at any file on the // importing user's disk. - if ("localFilePath" in metadata) { - const path = metadata.localFilePath; - if (!nonEmptyString(path) || !isReReadablePath(path)) delete metadata.localFilePath; + if ("localFilePath" in metadata && !isReReadablePath(metadata.localFilePath)) { + delete metadata.localFilePath; } // A bundle is shareable, so its `sourcePath` is untrusted: keep it only when // it is an absolute, traversal-free path a host could legitimately re-read. // Without this a crafted entry could point `onAddFilePath` at any file on the // importing user's disk. - const sourcePath = - nonEmptyString(candidate.sourcePath) && isReReadablePath(candidate.sourcePath) - ? candidate.sourcePath - : undefined; + const sourcePath = isReReadablePath(candidate.sourcePath) ? candidate.sourcePath : undefined; const geojson = featureCollection(candidate.geojson); // Same "is there anything to re-add from" gate as the capture path, so a // hand-edited bundle cannot introduce an entry that always fails. Both path diff --git a/tests/layer-library.test.ts b/tests/layer-library.test.ts index 9bea57891..64ba2d280 100644 --- a/tests/layer-library.test.ts +++ b/tests/layer-library.test.ts @@ -139,6 +139,38 @@ describe("canSaveLayerToLibrary", () => { ); }); + it("accepts a control-painted layer whose features live only in its control", () => { + // A tiles-mode Add Vector Layer layer has no `layer.geojson`, no re-fetchable + // source, and no path — but the save handler can read its features out of the + // control, so the gate must not hide what the capture path can embed. + const tilesMode = layer({ + type: "vector-tiles", + source: { type: "vector" }, + sourcePath: "cities.geojson", + metadata: { + externalNativeLayer: true, + customLayerType: "vector-fill", + sourceKind: "maplibre-gl-vector", + }, + }); + assert.equal(canSaveLayerToLibrary(tilesMode, { canRestoreControlPainted: () => true }), false); + assert.equal( + canSaveLayerToLibrary(tilesMode, { + canRestoreControlPainted: () => true, + hasMaterializableFeatures: () => true, + }), + true, + ); + // Still refused when it cannot be re-rendered, whatever the features say. + assert.equal( + canSaveLayerToLibrary(tilesMode, { + canRestoreControlPainted: () => false, + hasMaterializableFeatures: () => true, + }), + false, + ); + }); + it("rejects a layer with nothing to re-add from", () => { assert.equal(canSaveLayerToLibrary(layer()), false); assert.equal( @@ -338,6 +370,30 @@ describe("captureLayerLibraryEntry", () => { assert.equal("localFileReloadable" in result.entry.metadata, false); }); + it("captures a control-painted layer that has no path and no store features", () => { + // The true no-path case: a browser-picked or tiles-mode Add Vector Layer + // layer. Everything the entry needs comes from the caller's materialized + // features, so the capture must not depend on `sourcePath` or `layer.geojson`. + const result = captureLayerLibraryEntry( + layer({ + type: "vector-tiles", + source: { type: "vector" }, + sourcePath: "cities.geojson", + metadata: { + externalNativeLayer: true, + customLayerType: "vector-fill", + sourceKind: "maplibre-gl-vector", + }, + }), + { ...CAPTURE_OPTIONS, features: POINTS }, + ); + assert.equal(result.ok, true); + if (!result.ok) return; + assert.deepEqual(result.entry.metadata.embeddedGeoJSON, POINTS); + assert.equal(result.entry.sourcePath, undefined); + assert.equal(result.entry.needsLocalFile, undefined); + }); + it("prefers caller-supplied features over the layer's attribute-table copy", () => { const result = captureLayerLibraryEntry(layer({ geojson: POINTS }), { ...CAPTURE_OPTIONS, From a0967c810a2453f388981e7b8e0af4331114ae82 Mon Sep 17 00:00:00 2001 From: giswqs Date: Thu, 30 Jul 2026 00:44:54 -0400 Subject: [PATCH 08/13] Restore pluginOwnsPaint's docstring `controlRendersLayer` was inserted between `pluginOwnsPaint`'s existing docstring and the function it documents, so the old comment read as if it described the new predicate and `pluginOwnsPaint` was left undocumented. Move the new function above that docstring and note how the two differ: one asks who creates the layer, the other who paints an existing one. --- packages/core/src/external-native-paint.ts | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/packages/core/src/external-native-paint.ts b/packages/core/src/external-native-paint.ts index 783b98e11..86dfba449 100644 --- a/packages/core/src/external-native-paint.ts +++ b/packages/core/src/external-native-paint.ts @@ -38,10 +38,6 @@ export interface ExternalNativePaintBridge { /** Serializable paint-ownership marker stored in `GeoLibreLayer.metadata`. */ export type ExternalNativePaintMode = "geolibre" | "plugin"; -/** - * True when the plugin that registered this layer paints it itself, so GeoLibre - * must not offer (or apply) MapLibre paint properties for it. - */ /** * Whether a layer is drawn by a **custom render layer** its control created — a * deck.gl overlay, 3D Tiles, or Add Vector Layer's own MapLibre layers — rather @@ -54,6 +50,9 @@ export type ExternalNativePaintMode = "geolibre" | "plugin"; * reason about it — the Layer Library's "can this be re-added and rendered?" * gate (issue #1520) — share one definition and cannot drift. * + * Distinct from {@link pluginOwnsPaint}: that asks who applies *paint* to an + * existing layer, this asks who creates the layer at all. + * * @param layer - A store layer (metadata only). * @returns True when only the owning control can create the layer's map output. */ @@ -61,6 +60,10 @@ export function controlRendersLayer(layer: { metadata: Record } return typeof layer.metadata.customLayerType === "string"; } +/** + * True when the plugin that registered this layer paints it itself, so GeoLibre + * must not offer (or apply) MapLibre paint properties for it. + */ export function pluginOwnsPaint(layer: { metadata: Record }): boolean { return layer.metadata.paintMode === "plugin"; } From d859f8884263feb67621116fd68a4af6d449c224 Mon Sep 17 00:00:00 2001 From: giswqs Date: Thu, 30 Jul 2026 00:48:15 -0400 Subject: [PATCH 09/13] Redact credentials from every URL-shaped source field on export MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `source.data` holds the URL for a URL-backed GeoJSON layer — a form `hasRestorableLayerSource` already treats as re-fetchable — but export only swept `url`, `tiles`, and `metadata.originalUrl`, so such an entry leaked its token into a shared bundle. Fixed by sweeping every string (and every string in an array) under `source` rather than naming the fields that hold a URL. A per-field list has to be extended for each new layer type, with a silent credential leak as the cost of forgetting — the third time in this PR that a second field of the same kind was missed after the first was handled. Non-string values are returned untouched, so an inline FeatureCollection in `source.data` is unaffected. --- packages/core/src/layer-library.ts | 42 +++++++++++++++---------- tests/layer-library.test.ts | 49 ++++++++++++++++++++++++++++++ 2 files changed, 75 insertions(+), 16 deletions(-) diff --git a/packages/core/src/layer-library.ts b/packages/core/src/layer-library.ts index 8d4a8f0c3..38d89a263 100644 --- a/packages/core/src/layer-library.ts +++ b/packages/core/src/layer-library.ts @@ -651,22 +651,16 @@ function redactEntryForExport(entry: LayerLibraryEntry): LayerLibraryEntry { changed = true; } } - if (typeof source.url === "string") { - const redacted = redactUrlCredentials(source.url); - if (redacted !== source.url) { - source.url = redacted; - changed = true; - } - } - const originalTiles: unknown[] | undefined = Array.isArray(source.tiles) - ? source.tiles - : undefined; - if (originalTiles) { - const tiles = originalTiles.map((tile) => - typeof tile === "string" ? redactUrlCredentials(tile) : tile, - ); - if (tiles.some((tile, index) => tile !== originalTiles[index])) { - source.tiles = tiles; + // 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; } } @@ -683,6 +677,22 @@ function redactEntryForExport(entry: LayerLibraryEntry): LayerLibraryEntry { return changed ? { ...entry, source, metadata } : entry; } +/** + * Redact a single `source` value: a string is treated as a possible URL, an + * array as a list of them (tile templates), and anything else — an inline + * FeatureCollection, a number, a nested object — is returned as-is. Returns the + * original reference when nothing changed, so callers can detect that by + * identity. + */ +function redactSourceValue(value: unknown): unknown { + if (typeof value === "string") return redactUrlCredentials(value); + if (!Array.isArray(value)) return value; + const redacted = value.map((item) => + typeof item === "string" ? redactUrlCredentials(item) : item, + ); + return redacted.some((item, index) => item !== value[index]) ? redacted : value; +} + /** * Serialize Layer Library entries into the shareable bundle JSON written by the * Export action and read back by {@link parseLayerLibrary}. Per-user credentials diff --git a/tests/layer-library.test.ts b/tests/layer-library.test.ts index 64ba2d280..f610463c4 100644 --- a/tests/layer-library.test.ts +++ b/tests/layer-library.test.ts @@ -811,6 +811,55 @@ describe("serializeLayerLibrary / parseLayerLibrary", () => { assert.equal(entry.source.url, "https://tiles.example.com/service?api_key=SECRET1&style=dark"); }); + it("redacts every URL-shaped source field, not a named list of them", () => { + // A URL-backed GeoJSON layer keeps its URL in `source.data`, which + // `hasRestorableLayerSource` already treats as re-fetchable — so it has to be + // swept too, along with any field a future layer type puts a URL in. + const [entry] = normalizeLayerLibraryEntries([ + { + id: "e-data", + name: "Remote GeoJSON", + addedAt: "", + layerType: "geojson", + source: { + type: "geojson", + data: "https://api.example.com/features.geojson?token=SECRET_DATA", + someFutureUrl: "https://api.example.com/x?api_key=SECRET_FUTURE", + maxzoom: 14, + }, + opacity: 1, + metadata: {}, + }, + ]); + const exported = serializeLayerLibrary([entry]); + assert.equal(exported.includes("SECRET_DATA"), false); + assert.equal(exported.includes("SECRET_FUTURE"), false); + const [reimported] = parseLayerLibrary(exported); + assert.equal(reimported.source.data, "https://api.example.com/features.geojson"); + assert.equal(reimported.source.someFutureUrl, "https://api.example.com/x"); + // Non-string values are untouched. + assert.equal(reimported.source.maxzoom, 14); + assert.equal(reimported.source.type, "geojson"); + }); + + it("leaves an inline GeoJSON source untouched on export", () => { + // `source.data` can also be the features themselves; the sweep must not + // mangle a FeatureCollection while looking for URLs. + const [entry] = normalizeLayerLibraryEntries([ + { + id: "e-inline", + name: "Drawn features", + addedAt: "", + layerType: "geojson", + source: { type: "geojson", data: POINTS }, + opacity: 1, + metadata: { embeddedGeoJSON: POINTS }, + }, + ]); + const [reimported] = parseLayerLibrary(serializeLayerLibrary([entry])); + assert.deepEqual(reimported.source.data, POINTS); + }); + it("leaves a credential-free URL byte-identical", () => { const [entry] = normalizeLayerLibraryEntries([ { From 7734a2bc14f9b9f25fed5b5c6445e90c6cb4f0a4 Mon Sep 17 00:00:00 2001 From: giswqs Date: Thu, 30 Jul 2026 01:10:18 -0400 Subject: [PATCH 10/13] Address round-9 review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 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. --- .../src/components/panels/BrowserPanel.tsx | 34 +++-- .../src/components/panels/BrowserTreeNode.tsx | 6 +- .../src/components/panels/LayerPanel.tsx | 67 +++++---- .../src/lib/restore-library-layer.ts | 12 +- packages/core/src/layer-library.ts | 135 ++++++++++++++---- tests/layer-library.test.ts | 134 +++++++++++++++++ 6 files changed, 318 insertions(+), 70 deletions(-) diff --git a/apps/geolibre-desktop/src/components/panels/BrowserPanel.tsx b/apps/geolibre-desktop/src/components/panels/BrowserPanel.tsx index 468bdb365..46d6adbc3 100644 --- a/apps/geolibre-desktop/src/components/panels/BrowserPanel.tsx +++ b/apps/geolibre-desktop/src/components/panels/BrowserPanel.tsx @@ -479,8 +479,10 @@ export function BrowserPanel({ // store so MapController.syncLayers builds its map output, then run the // owning plugin's restore pass when the layer is control-painted (that // pass, not the store sync, is what draws those layers). Held under the - // busy flag so the row spins through a slow re-ingest and a second click - // mid-restore cannot add the layer twice. + // busy flag so a second click during the add cannot duplicate the layer. + // Note the flag only covers dispatching the restore, not the re-ingest + // that follows: see `restoreLibraryLayer` on why four of the five passes + // cannot be awaited to completion. beginBusy(node.id); try { addLayer(plan.layer); @@ -506,13 +508,13 @@ export function BrowserPanel({ // found among the newly-added ids by matching the path this add used. // That stays correct if an unrelated layer lands concurrently, and // patches nothing rather than the wrong layer if none matches. - const idsBefore = new Set(useAppStore.getState().layers.map((entry) => entry.id)); + const idsBefore = new Set(useAppStore.getState().layers.map((l) => l.id)); const addError = await onAddFilePath(plan.path); if (addError) { setError(addError); } else { - const added = useAppStore.getState().layers.filter((entry) => !idsBefore.has(entry.id)); - const matches = added.filter((entry) => layerCameFromPath(entry, plan.path)); + const added = useAppStore.getState().layers.filter((l) => !idsBefore.has(l.id)); + const matches = added.filter((l) => layerCameFromPath(l, plan.path)); const target = matches.length === 1 ? matches[0] @@ -532,10 +534,11 @@ export function BrowserPanel({ if (virtualFields) { useAppStore.getState().setLayerVirtualFields(target.id, virtualFields); } - } else if (added.length > 0) { - // The file came back but we cannot tell which layer the saved - // configuration belongs to. Say so, rather than leaving the user to - // notice their styling silently did not return. + } else { + // Either the import produced several indistinguishable layers, or it + // reported success while adding none. Both leave the entry's saved + // configuration unapplied, so say so rather than letting the user + // discover their styling silently did not return. setError(t("browser.libraryLayerConfigNotApplied", { name: plan.config.name })); } } @@ -650,8 +653,17 @@ export function BrowserPanel({ // My Data rename/delete. The store slice drives the tree, so both changes are // reflected (and persisted to IndexedDB) with no refresh event to fire. - const commitRename = (node: BrowserNode, name: string) => { + // Closing the editor unmounts the focused input, which would drop a + // keyboard-only user's focus to and lose their place in the tree, so + // hand focus back to the row. Deferred a frame because the row's button does + // not exist again until this state change has rendered. + const endRename = (rowId: string) => { setRenamingId(null); + requestAnimationFrame(() => focusRow(rowId)); + }; + + const commitRename = (node: BrowserNode, name: string) => { + endRename(node.id); if (node.libraryLayerId) renameLayerLibraryEntry(node.libraryLayerId, name); }; @@ -771,7 +783,7 @@ export function BrowserPanel({ renamingId={renamingId} onBeginRename={setRenamingId} onCommitRename={commitRename} - onCancelRename={() => setRenamingId(null)} + onCancelRename={endRename} onDeleteLibraryLayer={deleteLibraryLayer} onImportLibrary={() => void importLibrary()} onExportLibrary={() => void exportLibrary()} diff --git a/apps/geolibre-desktop/src/components/panels/BrowserTreeNode.tsx b/apps/geolibre-desktop/src/components/panels/BrowserTreeNode.tsx index a8273daae..0663a0c88 100644 --- a/apps/geolibre-desktop/src/components/panels/BrowserTreeNode.tsx +++ b/apps/geolibre-desktop/src/components/panels/BrowserTreeNode.tsx @@ -58,8 +58,8 @@ interface BrowserTreeNodeProps { onBeginRename: (id: string) => void; /** Commit a rename; a blank or unchanged name is a no-op for the caller. */ onCommitRename: (node: BrowserNode, name: string) => void; - /** Abandon the in-progress rename (Escape). */ - onCancelRename: () => void; + /** Abandon the in-progress rename (Escape), by row id. */ + onCancelRename: (id: string) => void; /** Delete a saved Layer Library entry (its trash icon). */ onDeleteLibraryLayer: (node: BrowserNode) => void; /** Import a Layer Library JSON bundle (the My Data section's ⬆). */ @@ -241,7 +241,7 @@ export function BrowserTreeNode({ node={node} indentStart={indentStart} onCommit={(name) => onCommitRename(node, name)} - onCancel={onCancelRename} + onCancel={() => onCancelRename(node.id)} /> ) : (