Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions apps/geolibre-desktop/src/components/layout/DesktopShell.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
restoreReverseGeocode,
REVERSE_GEOCODE_PLUGIN_ID,
restoreEffects,
restoreLidarLayers,
restoreRasterLayers,
restoreThreeDTilesLayers,
restoreVectorLayers,
Expand Down Expand Up @@ -813,6 +814,13 @@
restoreThreeDTilesLayers(appAPI);
restoreRasterLayers(appAPI);
restoreVectorLayers(appAPI);
// Re-stream saved LiDAR (COPC) point clouds. A `lidar-url` layer restores
// into the store as inert metadata; the point cloud is loaded by the LiDAR
// control, not the store, so without this the layer shows in the panel but
// renders nothing.
void restoreLidarLayers(appAPI).catch((error: unknown) => {
console.warn("[lidar] failed to restore saved point clouds", error);
});
// Re-read drag-dropped / Add Data local-file GeoJSON layers from disk
// (their data was saved as a path, not embedded).
void restoreLocalFileLayers();
Expand Down Expand Up @@ -1160,7 +1168,7 @@
disposed = true;
unlisten?.();
};
}, [clearDropMessageLater,

Check warning on line 1171 in apps/geolibre-desktop/src/components/layout/DesktopShell.tsx

View workflow job for this annotation

GitHub Actions / Build and test

React Hook useEffect has a missing dependency: 't'. Either include it or remove the dependency array
finishDrop,
addDroppedRasters,
addDroppedPhotos,
Expand Down Expand Up @@ -1300,7 +1308,7 @@
clearDropMessageLater();
}
},
[clearDropMessageLater,

Check warning on line 1311 in apps/geolibre-desktop/src/components/layout/DesktopShell.tsx

View workflow job for this annotation

GitHub Actions / Build and test

React Hook useCallback has a missing dependency: 't'. Either include it or remove the dependency array
finishDrop,
addDroppedRasters,
addDroppedPhotos,
Expand Down
1 change: 1 addition & 0 deletions packages/plugins/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ export {
openHtmlPanel,
openLegendPanel,
openLidarLayerPanel,
restoreLidarLayers,
openMeasurePanel,
openMinimapPanel,
openPMTilesLayerPanel,
Expand Down
180 changes: 177 additions & 3 deletions packages/plugins/src/plugins/maplibre-components.ts
Original file line number Diff line number Diff line change
Expand Up @@ -741,6 +741,28 @@ let stacSearchStoreUnsubscribe: (() => void) | null = null;
let zarrStoreUnsubscribe: (() => void) | null = null;
let lidarStoreUnsubscribe: (() => void) | null = null;
let splattingStoreUnsubscribe: (() => void) | null = null;

// Re-streaming saved LiDAR layers on project open. The store only holds a
// `lidar-url` layer's metadata; the point cloud itself is loaded by the LiDAR
// control, not the store, so a reopened project shows the layer in the panel
// but renders nothing until we ask the control to stream it again (see
// restoreLidarLayers). Because loadPointCloud assigns a fresh id, each entry
// carries the saved layer's desired state so the load handler can reattach the
// loaded cloud to the saved layer instead of adding a duplicate. The map is
// keyed by source URL and holds a FIFO queue per URL, so two saved layers that
// point at the same COPC file both restore (one entry consumed per load event).
interface PendingLidarRestore {
layerId: string;
name: string;
visible: boolean;
opacity: number;
style: GeoLibreLayer["style"];
groupId: string | undefined;
beforeLayerId: string | null;
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
const pendingLidarRestores = new Map<string, PendingLidarRestore[]>();
let lidarRestoreInFlight = false;

let pluginActive = false;
let componentsControlRevision = 0;
let componentsConstructorsPromise: Promise<ComponentsConstructors> | null =
Expand Down Expand Up @@ -2433,13 +2455,20 @@ async function openStandaloneHtmlControl(
}

async function openStandaloneLidarControl(
app: GeoLibreAppAPI
app: GeoLibreAppAPI,
options: { reveal?: boolean } = {}
): Promise<boolean> {
// `reveal` shows and expands the panel (the default, for the Add LiDAR Layer
// menu action). Project restore mounts the control only to re-stream saved
// clouds, so it passes `reveal: false` to keep the panel out of the user's
// way; a freshly created control is hidden so it does not pop open on load.
const reveal = options.reveal ?? true;
const {
LidarControl: LidarControlClass,
LidarLayerAdapter: LidarLayerAdapterClass,
} = await getComponentsConstructors();

const created = !lidarControl;
lidarControl ??= createLidarControl(
LidarControlClass,
LidarLayerAdapterClass
Expand All @@ -2457,12 +2486,105 @@ async function openStandaloneLidarControl(
startLidarThemeSync();

setTimeout(() => {
showLidarControl(lidarControl);
lidarControl?.expand();
if (reveal) {
showLidarControl(lidarControl);
lidarControl?.expand();
} else if (created) {
hideLidarControl(lidarControl);
}
}, 0);
return true;
}

/**
* Read the source URL of a `lidar-url` layer, preferring the dedicated
* `sourcePath` and falling back to `source.url`.
*/
function lidarLayerUrl(layer: GeoLibreLayer): string | null {
if (typeof layer.sourcePath === "string" && layer.sourcePath) {
return layer.sourcePath;
}
const url = (layer.source as { url?: unknown }).url;
return typeof url === "string" && url ? url : null;
}

/** Whether a restore is already queued or in flight for this specific layer. */
function isLidarRestorePending(layer: GeoLibreLayer): boolean {
for (const queue of pendingLidarRestores.values()) {
if (queue.some((pending) => pending.layerId === layer.id)) return true;
}
return false;
}

/**
* Re-stream the point clouds for any restored `lidar-url` layers that are not
* yet loaded into the LiDAR control (e.g. after opening a saved project). The
* store only holds the layer metadata, so without this the layer appears in the
* Layers panel but renders nothing. The loaded cloud is reattached to the saved
* layer in {@link createLidarLoadHandler}, preserving its visibility, opacity,
* style, name, and position.
*/
export async function restoreLidarLayers(app: GeoLibreAppAPI): Promise<void> {
if (lidarRestoreInFlight) return;

const pending = useAppStore
.getState()
.layers.filter(
(layer) =>
isLidarControlLayer(layer) &&
!hasLidarPointCloud(layer.id) &&
!isLidarRestorePending(layer)
);
if (pending.length === 0) return;

lidarRestoreInFlight = true;
try {
const opened = await openStandaloneLidarControl(app, { reveal: false });
if (!opened || !lidarControl) return;
// The deck.gl point-cloud overlay only renders under the Mercator
// projection (the streaming loader's viewport math breaks under the default
// globe), matching the USGS LiDAR plugin and the other deck.gl controls.
ensureMercatorProjection(app.getMap?.());

for (const layer of pending) {
const url = lidarLayerUrl(layer);
if (!url) continue;
// Re-check against the live store: a layer may have been removed, already
// loaded, or queued while the control was loading asynchronously.
const current = useAppStore.getState().layers;
const index = current.findIndex((item) => item.id === layer.id);
if (index === -1) continue;
if (hasLidarPointCloud(layer.id) || isLidarRestorePending(layer)) continue;
Comment thread
giswqs marked this conversation as resolved.

const entry: PendingLidarRestore = {
layerId: layer.id,
name: layer.name,
visible: layer.visible,
opacity: layer.opacity,
style: layer.style,
groupId: layer.groupId,
beforeLayerId: current[index + 1]?.id ?? null,
};
const queue = pendingLidarRestores.get(url);
if (queue) queue.push(entry);
else pendingLidarRestores.set(url, [entry]);
lidarControl.loadPointCloud(url).catch((error: unknown) => {
// Drop only this layer's entry so a sibling restore for the same URL is
// not lost; clean up the map key once its queue empties.
const remaining = pendingLidarRestores.get(url);
if (remaining) {
const at = remaining.indexOf(entry);
if (at !== -1) remaining.splice(at, 1);
if (remaining.length === 0) pendingLidarRestores.delete(url);
}
console.warn("[lidar] failed to restore point cloud", url, error);
});
}
} finally {
lidarRestoreInFlight = false;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Low confidence – flag is cleared while loadPointCloud calls are still in flight

lidarRestoreInFlight is reset in finally after all loadPointCloud calls are fired, not after they complete. A concurrent call to restoreLidarLayers that arrives after the finally runs (but before any streams finish) will pass the guard at line 2528 and proceed; individual layers are protected from re-queuing by isLidarRestorePending, but the flag itself no longer blocks the setup path.

This is fine functionally (the isLidarRestorePending check is the real per-layer guard), but the name lidarRestoreInFlight implies it covers the full restore lifecycle. A name like lidarRestoreSetupInProgress would better communicate that this only serialises the async setup step.

}
}

async function openStandaloneSplattingControl(
app: GeoLibreAppAPI
): Promise<boolean> {
Expand Down Expand Up @@ -3297,6 +3419,10 @@ function setHtmlPanelVisible(visible: boolean): void {

function teardownLidarControl(app: GeoLibreAppAPI): void {
stopLidarThemeSync();
// Clear restore bookkeeping so a teardown mid-restore (project reload, map
// re-init) cannot strand the in-flight guard and block later restores.
pendingLidarRestores.clear();
Comment thread
giswqs marked this conversation as resolved.
lidarRestoreInFlight = false;
lidarStoreUnsubscribe?.();
lidarStoreUnsubscribe = null;
lidarLayerAdapter?.destroy();
Expand Down Expand Up @@ -3326,6 +3452,54 @@ function createLidarLoadHandler(): LidarControlEventHandler {

const store = useAppStore.getState();
const layer = createLidarStoreLayer(event.pointCloud);

// Project restore: this load was triggered to re-stream a saved layer (see
// restoreLidarLayers). loadPointCloud assigns a fresh id, so swap the inert
// placeholder (saved id) for the loaded layer in place, carrying over the
// saved visibility, opacity, style, name, and position.
const restoreKey =
typeof event.pointCloud.source === "string"
? event.pointCloud.source
: null;
const restoreQueue = restoreKey
? pendingLidarRestores.get(restoreKey)
: undefined;
const restore = restoreQueue?.shift();
if (restore && restoreKey) {
if (restoreQueue && restoreQueue.length === 0) {
pendingLidarRestores.delete(restoreKey);
}
const restored: GeoLibreLayer = {
...layer,
name: restore.name || layer.name,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit – falsy coercion swallows intentional empty names

restore.name || layer.name uses ||, so an empty string (a user who cleared the layer name to "") would silently fall through to the URL-derived name. Prefer a nullish check:

Suggested change
name: restore.name || layer.name,
name: restore.name !== "" ? restore.name : layer.name,

Or if the type already ensures name is never null | undefined, just restore.name || layer.name is safe—but then the intent should be documented. Confidence: low.

visible: restore.visible,
opacity: restore.opacity,
style: restore.style,
...(restore.groupId ? { groupId: restore.groupId } : {}),
};
if (
restore.layerId !== restored.id &&
store.layers.some((item) => item.id === restore.layerId)
) {
store.removeLayer(restore.layerId);
}
const beforeLayerId =
restore.beforeLayerId &&
useAppStore
.getState()
.layers.some((item) => item.id === restore.beforeLayerId)
? restore.beforeLayerId
: null;
store.addLayer(restored, beforeLayerId);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Comment on lines +3480 to +3493

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Medium confidence – placeholder removal is gated, but addLayer is unconditional

The guard at lines 3480-3484 skips removeLayer if the saved placeholder was already removed (e.g. user deleted the layer during streaming). However, store.addLayer(restored, …) on line 3493 runs regardless, so the layer is silently resurrected even if the user explicitly deleted it.

The check also mixes two state snapshots: store.layers (captured at line 3453, before removeLayer) for the existence test, and a fresh useAppStore.getState().layers (line 3488-3491) for the beforeLayerId validation. Grabbing a fresh snapshot for the existence check too would make this consistent and rule out a TOCTOU between capture and use.

Suggested fix – bail out if the placeholder is gone (indicating a user deletion), using a single fresh read:

Suggested change
if (
restore.layerId !== restored.id &&
store.layers.some((item) => item.id === restore.layerId)
) {
store.removeLayer(restore.layerId);
}
const beforeLayerId =
restore.beforeLayerId &&
useAppStore
.getState()
.layers.some((item) => item.id === restore.beforeLayerId)
? restore.beforeLayerId
: null;
store.addLayer(restored, beforeLayerId);
const currentLayers = useAppStore.getState().layers;
const placeholderIndex = currentLayers.findIndex(
(item) => item.id === restore.layerId
);
if (placeholderIndex === -1) {
// Placeholder was removed while the point cloud was streaming; skip.
return;
}
store.removeLayer(restore.layerId);
const afterRemoveLayers = useAppStore.getState().layers;
const beforeLayerId =
restore.beforeLayerId &&
afterRemoveLayers.some((item) => item.id === restore.beforeLayerId)
? restore.beforeLayerId
: null;
store.addLayer(restored, beforeLayerId);

if (!restored.visible) {
lidarLayerAdapter?.setVisibility(restored.id, false);
}
if (restored.opacity !== 1) {
lidarLayerAdapter?.setOpacity(restored.id, restored.opacity);
Comment on lines +3494 to +3498

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit – explicit adapter calls may be redundant with the store subscriber

createLidarControl sets up a store subscriber (via lidarStoreUnsubscribe) that already syncs visibility and opacity from the store to the adapter on every state change. Adding the layer via store.addLayer(restored, …) above triggers that subscriber, so these explicit setVisibility/setOpacity calls are likely no-ops that fire a millisecond after the subscriber already did the same work. If they're intentional (to guarantee synchronous application before the first render frame), a brief comment would help future readers understand why both paths exist.

}
return;
}

if (store.layers.some((item) => item.id === layer.id)) {
store.updateLayer(layer.id, {
metadata: layer.metadata,
Expand Down
Loading