Skip to content

Commit 216314e

Browse files
committed
Address Claude and CodeRabbit review feedback
- Reset lidarRestoreInFlight on teardownLidarControl so a teardown while a restore is awaiting the control cannot strand the guard and block later restores (Claude). - Support two saved LiDAR layers that reference the same COPC URL: key pendingLidarRestores by URL with a FIFO queue, match each layer by layerId, and consume one entry per load event so neither placeholder is left inert (Claude, CodeRabbit). - Preserve groupId so a LiDAR layer saved inside a folder restores into that folder instead of at the top level (CodeRabbit). - Catch restoreLidarLayers rejections at the DesktopShell call site so a failure is logged, not an unhandled rejection (CodeRabbit). - Use a non-null assertion instead of `as string` for the narrowed restoreKey (Claude nit).
1 parent a5f7a33 commit 216314e

2 files changed

Lines changed: 39 additions & 17 deletions

File tree

apps/geolibre-desktop/src/components/layout/DesktopShell.tsx

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -818,7 +818,9 @@ export function DesktopShell({
818818
// into the store as inert metadata; the point cloud is loaded by the LiDAR
819819
// control, not the store, so without this the layer shows in the panel but
820820
// renders nothing.
821-
void restoreLidarLayers(appAPI);
821+
void restoreLidarLayers(appAPI).catch((error: unknown) => {
822+
console.warn("[lidar] failed to restore saved point clouds", error);
823+
});
822824
// Re-read drag-dropped / Add Data local-file GeoJSON layers from disk
823825
// (their data was saved as a path, not embedded).
824826
void restoreLocalFileLayers();

packages/plugins/src/plugins/maplibre-components.ts

Lines changed: 36 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -746,19 +746,21 @@ let splattingStoreUnsubscribe: (() => void) | null = null;
746746
// `lidar-url` layer's metadata; the point cloud itself is loaded by the LiDAR
747747
// control, not the store, so a reopened project shows the layer in the panel
748748
// but renders nothing until we ask the control to stream it again (see
749-
// restoreLidarLayers). Because loadPointCloud assigns a fresh id, this map
750-
// carries the saved layer's desired state, keyed by source URL, so the load
751-
// handler can reattach the loaded cloud to the saved layer instead of adding a
752-
// duplicate.
749+
// restoreLidarLayers). Because loadPointCloud assigns a fresh id, each entry
750+
// carries the saved layer's desired state so the load handler can reattach the
751+
// loaded cloud to the saved layer instead of adding a duplicate. The map is
752+
// keyed by source URL and holds a FIFO queue per URL, so two saved layers that
753+
// point at the same COPC file both restore (one entry consumed per load event).
753754
interface PendingLidarRestore {
754755
layerId: string;
755756
name: string;
756757
visible: boolean;
757758
opacity: number;
758759
style: GeoLibreLayer["style"];
760+
groupId: string | undefined;
759761
beforeLayerId: string | null;
760762
}
761-
const pendingLidarRestores = new Map<string, PendingLidarRestore>();
763+
const pendingLidarRestores = new Map<string, PendingLidarRestore[]>();
762764
let lidarRestoreInFlight = false;
763765

764766
let pluginActive = false;
@@ -2506,12 +2508,10 @@ function lidarLayerUrl(layer: GeoLibreLayer): string | null {
25062508
return typeof url === "string" && url ? url : null;
25072509
}
25082510

2509-
/** Whether a restore is already queued or in flight for this layer. */
2511+
/** Whether a restore is already queued or in flight for this specific layer. */
25102512
function isLidarRestorePending(layer: GeoLibreLayer): boolean {
2511-
const url = lidarLayerUrl(layer);
2512-
if (url && pendingLidarRestores.has(url)) return true;
2513-
for (const pending of pendingLidarRestores.values()) {
2514-
if (pending.layerId === layer.id) return true;
2513+
for (const queue of pendingLidarRestores.values()) {
2514+
if (queue.some((pending) => pending.layerId === layer.id)) return true;
25152515
}
25162516
return false;
25172517
}
@@ -2556,16 +2556,27 @@ export async function restoreLidarLayers(app: GeoLibreAppAPI): Promise<void> {
25562556
if (index === -1) continue;
25572557
if (hasLidarPointCloud(layer.id) || isLidarRestorePending(layer)) continue;
25582558

2559-
pendingLidarRestores.set(url, {
2559+
const entry: PendingLidarRestore = {
25602560
layerId: layer.id,
25612561
name: layer.name,
25622562
visible: layer.visible,
25632563
opacity: layer.opacity,
25642564
style: layer.style,
2565+
groupId: layer.groupId,
25652566
beforeLayerId: current[index + 1]?.id ?? null,
2566-
});
2567+
};
2568+
const queue = pendingLidarRestores.get(url);
2569+
if (queue) queue.push(entry);
2570+
else pendingLidarRestores.set(url, [entry]);
25672571
lidarControl.loadPointCloud(url).catch((error: unknown) => {
2568-
pendingLidarRestores.delete(url);
2572+
// Drop only this layer's entry so a sibling restore for the same URL is
2573+
// not lost; clean up the map key once its queue empties.
2574+
const remaining = pendingLidarRestores.get(url);
2575+
if (remaining) {
2576+
const at = remaining.indexOf(entry);
2577+
if (at !== -1) remaining.splice(at, 1);
2578+
if (remaining.length === 0) pendingLidarRestores.delete(url);
2579+
}
25692580
console.warn("[lidar] failed to restore point cloud", url, error);
25702581
});
25712582
}
@@ -3408,7 +3419,10 @@ function setHtmlPanelVisible(visible: boolean): void {
34083419

34093420
function teardownLidarControl(app: GeoLibreAppAPI): void {
34103421
stopLidarThemeSync();
3422+
// Clear restore bookkeeping so a teardown mid-restore (project reload, map
3423+
// re-init) cannot strand the in-flight guard and block later restores.
34113424
pendingLidarRestores.clear();
3425+
lidarRestoreInFlight = false;
34123426
lidarStoreUnsubscribe?.();
34133427
lidarStoreUnsubscribe = null;
34143428
lidarLayerAdapter?.destroy();
@@ -3447,15 +3461,21 @@ function createLidarLoadHandler(): LidarControlEventHandler {
34473461
typeof event.pointCloud.source === "string"
34483462
? event.pointCloud.source
34493463
: null;
3450-
const restore = restoreKey ? pendingLidarRestores.get(restoreKey) : null;
3451-
if (restore) {
3452-
pendingLidarRestores.delete(restoreKey as string);
3464+
const restoreQueue = restoreKey
3465+
? pendingLidarRestores.get(restoreKey)
3466+
: undefined;
3467+
const restore = restoreQueue?.shift();
3468+
if (restore && restoreKey) {
3469+
if (restoreQueue && restoreQueue.length === 0) {
3470+
pendingLidarRestores.delete(restoreKey);
3471+
}
34533472
const restored: GeoLibreLayer = {
34543473
...layer,
34553474
name: restore.name || layer.name,
34563475
visible: restore.visible,
34573476
opacity: restore.opacity,
34583477
style: restore.style,
3478+
...(restore.groupId ? { groupId: restore.groupId } : {}),
34593479
};
34603480
if (
34613481
restore.layerId !== restored.id &&

0 commit comments

Comments
 (0)