fix(lidar): re-stream COPC point clouds when a saved project is reopened#877
Conversation
A lidar (COPC) layer restores into the store as inert metadata: the point cloud is streamed by the LiDAR control, not the store, so reopening a saved project showed the layer in the Layers panel but rendered nothing. Add restoreLidarLayers, called from the same project-restore effect as the raster/vector/3D-tiles restorers, which streams each unloaded lidar-url layer via the LiDAR control (mounted hidden, Mercator forced for the deck.gl overlay as the other deck controls already do). Because loadPointCloud assigns a fresh id, the load handler reattaches the loaded cloud to the saved layer in place, preserving its visibility, opacity, style, name, and position rather than adding a duplicate. Fixes #870
✅ Deploy Preview for geolibre-app ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughDesktopShell now calls ChangesLiDAR project restore flow
Sequence Diagram(s)sequenceDiagram
participant DesktopShell
participant "restoreLidarLayers" as RestoreLidarLayers
participant "openStandaloneLidarControl" as OpenStandaloneLidarControl
participant LiDARControl
participant "load event handler" as LoadEventHandler
participant "app store" as AppStore
DesktopShell->>RestoreLidarLayers: call with appAPI after project restore
RestoreLidarLayers->>OpenStandaloneLidarControl: open with reveal=false
RestoreLidarLayers->>LiDARControl: loadPointCloud(url)
LiDARControl->>LoadEventHandler: emit load event
LoadEventHandler->>AppStore: replace restored layer and apply visibility/opacity
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
⚡ Cloudflare Pages preview
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Code reviewBugs
Quality
What I checked and found clean
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/geolibre-desktop/src/components/layout/DesktopShell.tsx`:
- Line 821: The `DesktopShell` restore flow is swallowing a possible rejection
from `restoreLidarLayers(appAPI)`, so this `void` call can become an unhandled
promise rejection. Update the restore logic around `restoreLidarLayers` to
handle its rejection explicitly, either by awaiting it within the surrounding
async flow or by attaching a catch handler that logs and reports the failure.
Make sure the fix keeps the existing per-layer `loadPointCloud(...).catch(...)`
behavior intact while adding top-level failure handling for
`restoreLidarLayers`.
In `@packages/plugins/src/plugins/maplibre-components.ts`:
- Around line 3460-3473: The restore flow in maplibre-components is using
store.removeLayer() and store.addLayer(), which incorrectly flips isDirty and
records undo history during LiDAR placeholder replacement. Update the
restore-time replacement logic around the restored/restored.id block to use a
non-dirty swap path that replaces the layer in the store without changing the
current dirty/history state, preserving clean projects after the point cloud
loads.
- Around line 753-760: `PendingLidarRestore` and the LiDAR restore path are
dropping folder membership because `groupId` is not carried through, so reopened
layers get re-added ungrouped. Add `groupId` to `PendingLidarRestore`, thread it
through the restore logic that rebuilds the replacement layer, and ensure the
layer add/update call preserves the original group assignment in the relevant
LiDAR restore helpers.
- Line 761: The LiDAR restore tracking in maplibre-components is deduplicating
by URL only, so multiple saved layers that share the same COPC source cannot
both be restored. Update the pending restore state around pendingLidarRestores,
isLidarRestorePending(), and the restore handling paths to queue entries per URL
and distinguish them by layerId, then consume exactly one pending entry for each
load event so each placeholder layer can restore independently even when the URL
matches.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3796474d-b55f-46ff-9020-097b2dfbc44d
📒 Files selected for processing (3)
apps/geolibre-desktop/src/components/layout/DesktopShell.tsxpackages/plugins/src/index.tspackages/plugins/src/plugins/maplibre-components.ts
- 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).
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/plugins/src/plugins/maplibre-components.ts (1)
3486-3493: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winResolve
beforeLayerIdthrough restored placeholder IDs.
beforeLayerIdstores the saved next layer’s placeholder ID, but restored LiDAR layers are re-added with freshloadPointCloudIDs. If that next layer finishes first, its placeholder ID no longer exists, so this falls back tonulland appends the current layer, losing saved layer order. TracksavedPlaceholderId -> loadedLayerIdduring restore, or preserve the saved ID when re-adding, before resolvingbeforeLayerId.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/plugins/src/plugins/maplibre-components.ts` around lines 3486 - 3493, `beforeLayerId` resolution in the restore flow is using the saved placeholder ID directly, but restored LiDAR layers get new IDs from `loadPointCloud`, so ordering can be lost when the next layer restores first. Update the restore logic around `store.addLayer(restored, beforeLayerId)` to resolve `restore.beforeLayerId` through a `savedPlaceholderId -> loadedLayerId` mapping, or preserve the original placeholder ID when re-adding restored layers, then use that resolved ID when checking `useAppStore.getState().layers` and inserting the layer.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/plugins/src/plugins/maplibre-components.ts`:
- Around line 3486-3493: `beforeLayerId` resolution in the restore flow is using
the saved placeholder ID directly, but restored LiDAR layers get new IDs from
`loadPointCloud`, so ordering can be lost when the next layer restores first.
Update the restore logic around `store.addLayer(restored, beforeLayerId)` to
resolve `restore.beforeLayerId` through a `savedPlaceholderId -> loadedLayerId`
mapping, or preserve the original placeholder ID when re-adding restored layers,
then use that resolved ID when checking `useAppStore.getState().layers` and
inserting the layer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: aca6a58b-a8f1-4e1a-8387-8fc546eff6da
📒 Files selected for processing (2)
apps/geolibre-desktop/src/components/layout/DesktopShell.tsxpackages/plugins/src/plugins/maplibre-components.ts
| } | ||
| const restored: GeoLibreLayer = { | ||
| ...layer, | ||
| name: restore.name || layer.name, |
There was a problem hiding this comment.
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:
| 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.
| 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); |
There was a problem hiding this comment.
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:
| 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); |
| }); | ||
| } | ||
| } finally { | ||
| lidarRestoreInFlight = false; |
There was a problem hiding this comment.
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.
| if (!restored.visible) { | ||
| lidarLayerAdapter?.setVisibility(restored.id, false); | ||
| } | ||
| if (restored.opacity !== 1) { | ||
| lidarLayerAdapter?.setOpacity(restored.id, restored.opacity); |
There was a problem hiding this comment.
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.
Code reviewReviewed the diff (186 additions, 3 deletions across Bugs
Quality / Maintainability
Security / PerformanceNothing new introduced. The only external input touched ( CLAUDE.mdNo violations found. New user-facing state (layer name, visibility, opacity) is not translatable copy, so no Overall: The fix correctly addresses the reported bug. The placeholder-resurrection edge case is the one finding worth considering before merge; the rest are low-severity nits. |
1 participant
Problem
Reopening a saved project that contains a LiDAR (COPC) layer shows the layer in the Layers panel but renders nothing on the map. Reported in #870.
Root cause: a
lidar-urllayer is restored into the store as inert metadata. The point cloud is streamed by the LiDAR control (loadPointCloud), not by the store layer sync, and there was no code path that re-streamed a restored layer. GeoLibre never calledloadPointCloudoutside the LiDAR panel's own UI, so a reopened project's point cloud was never fetched. (Verified in the real app: the layer is present, but no COPC request is issued and nothing renders, even for a CORS-friendly source.)Fix
Add
restoreLidarLayers, invoked from the same project-restore effect inDesktopShellthat already re-runsrestoreRasterLayers/restoreVectorLayers/restoreThreeDTilesLayers(gated onprojectGeneration/mapReadyGeneration):lidar-urllayers that are in the store but not yet loaded into the control, mounts the LiDAR control hidden (reveal: false, so the panel does not pop open), forces the Mercator projection the deck.gl point-cloud overlay needs (matching the USGS LiDAR plugin and the other deck.gl controls), and streams each one.loadPointCloudassigns a fresh id, the load handler reattaches the loaded cloud to the saved layer in place, preserving its visibility, opacity, style, name, and position instead of adding a duplicate.Verification
Drove the real app with Playwright using a saved project that references the Autzen sample COPC:
npm run build,npm run test:frontend(1612 pass), eslint, and pre-commit all green.Fixes #870
Summary by CodeRabbit
New Features
Bug Fixes