fix: keep project load, collab sync, and layer cleanup consistent - #1575
Merged
giswqs merged 9 commits intoJul 31, 2026
Merged
Conversation
Normalize the primary mapView on parse/apply, broadcast collaboration snapshots when map grid/models/widgets/style library change, scrub storymap chapter layer refs on remove, and clear paint-bridge cache when a layer leaves the map.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis change centralizes collaboration project-change detection, normalizes persisted map views, removes dangling Story Map and secondary-pane layer references, refreshes join-derived columns, and clears stale map paint-bridge state. Tests cover each behavior. ChangesProject state consistency
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
Ayush7614
force-pushed
the
fix/project-sync-and-load-integrity
branch
from
July 30, 2026 16:29
bbc63b6 to
266cfdb
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@packages/core/src/store.ts`:
- Around line 1384-1386: Update removeLayerGroup, specifically the
removeChildren path that filters s.layers, to collect all deleted layer IDs and
pass them through the shared Story Map reference scrub/removal helper used by
scrubStorymapLayerRefs instead of only removing the group layer. Preserve
existing group-deletion behavior while ensuring onChapterEnter/onChapterExit
references to every removed child are cleared, and add a regression test
covering grouped deletion.
In `@tests/collaboration-project-changed.test.ts`:
- Around line 11-35: Expand the tests around projectChanged to cover every
broadcast-relevant field compared by buildProjectSnapshot, including processing
history, widgets, style library, metadata, layers, and plugins, plus a
collaboration/UI-only update that must remain false. Add focused assertions for
each change and preserve the existing map-grid, model, and camera cases; ensure
the added coverage meets the stated frontend coverage thresholds.
In `@tests/core-project.test.ts`:
- Around line 1289-1294: Update the mapView assertions in the core project test
to verify exact normalization outputs: assert bearing equals 270 and assert the
fallback longitude equals its expected exact value, rather than only checking
finiteness or a bearing range. Keep the existing zoom, pitch, and latitude
assertions unchanged.
🪄 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: 7b557703-ee16-46fc-b54b-330d37ccc23f
📒 Files selected for processing (8)
apps/geolibre-desktop/src/hooks/useCollaboration.tsapps/geolibre-desktop/src/lib/project-broadcast-changed.tspackages/core/src/project.tspackages/core/src/store.tspackages/map/src/layer-sync.tstests/collaboration-project-changed.test.tstests/core-project.test.tstests/storymap-layer-scrub.test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/core/src/store.ts (1)
1384-1386: 🗄️ Data Integrity & Integration | 🟠 MajorScrub Story Map references when deleting groups.
This fixes direct
removeLayercalls, butremoveLayerGroup(..., { removeChildren: true })still removes child layers via filtering without calling this helper. Group deletion can therefore leaveonChapterEnter/onChapterExitentries referencing deleted layer IDs. Reuse a shared multi-ID scrub path for all removed children.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/store.ts` around lines 1384 - 1386, Update removeLayerGroup’s removeChildren path to collect all child layer IDs and scrub Story Map references through a shared multi-ID helper, rather than only filtering layers. Reuse that helper from removeLayer for single-ID deletion so both direct layer removal and group deletion remove matching onChapterEnter/onChapterExit entries.
🤖 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 `@packages/map/src/layer-sync.ts`:
- Around line 3505-3507: Add a regression test covering the paint-bridge cache
invalidation around appliedBridgeState.delete: apply bridge state, remove the
map layer, recreate it with the same layerId and a new bridge, then verify
opacity and visibility are applied again.
In `@tests/core-project.test.ts`:
- Around line 1275-1295: The existing test only validates normalization through
parseProject; add a regression test that invokes applyProjectToStore directly
with invalid mapView center, zoom, bearing, and pitch values, then assert the
store’s resulting camera values are finite and clamped to the expected bounds.
---
Duplicate comments:
In `@packages/core/src/store.ts`:
- Around line 1384-1386: Update removeLayerGroup’s removeChildren path to
collect all child layer IDs and scrub Story Map references through a shared
multi-ID helper, rather than only filtering layers. Reuse that helper from
removeLayer for single-ID deletion so both direct layer removal and group
deletion remove matching onChapterEnter/onChapterExit entries.
🪄 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: 95ba8d98-2a21-4a3e-9ae4-bae87c5b24a7
📒 Files selected for processing (8)
apps/geolibre-desktop/src/hooks/useCollaboration.tsapps/geolibre-desktop/src/lib/project-broadcast-changed.tspackages/core/src/project.tspackages/core/src/store.tspackages/map/src/layer-sync.tstests/collaboration-project-changed.test.tstests/core-project.test.tstests/storymap-layer-scrub.test.ts
Member
|
/claude-review |
Contributor
🔍 Cloudflare PR preview
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
Code reviewBugs: None found. Traced all four fixes against their call sites and existing tests:
Security: None found — no new untrusted input handling, injection surface, or secret exposure in this diff. Performance: None found — the new comparisons and array filters are all O(n) over already-small collections (chapters, layers), consistent with the existing code's style. Quality:
CLAUDE.md: No violations — no new external map/tile hosts, no touched |
Contributor
🔍 GitHub Pages PR preview
|
Scrub storymap refs when deleting a group with its children, drop the unused projectChanged re-export, and tighten mapView/collaboration/ group-deletion regression coverage.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/core/src/store.ts`:
- Around line 1658-1660: Update the group-child deletion path around
scrubStorymapLayerRefs to reuse or extract a bulk layer-removal helper
equivalent to removeLayer. Ensure it cleans secondary-pane visibility overrides
and invokes cascadeLayerJoinRefresh for every removedIds member before
completing deletion. Add coverage verifying stale joined attributes and
secondary-pane overrides are removed for deleted child layers.
🪄 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: e106fc74-c801-4610-8665-402069df3048
📒 Files selected for processing (5)
apps/geolibre-desktop/src/hooks/useCollaboration.tspackages/core/src/store.tstests/collaboration-project-changed.test.tstests/core-project.test.tstests/storymap-layer-scrub.test.ts
💤 Files with no reviewable changes (1)
- apps/geolibre-desktop/src/hooks/useCollaboration.ts
Cascade join refresh and scrub secondary-pane visibility the same way removeLayer does, so group delete cannot leave stale joins or overrides.
Ayush7614
force-pushed
the
fix/project-sync-and-load-integrity
branch
from
July 30, 2026 20:39
f4932b0 to
9e78d74
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/collaboration-project-changed.test.ts`:
- Around line 6-18: Extract the duplicate geojsonLayer helper into a shared
test-fixtures module and update collaboration-project-changed.test.ts,
core-project.test.ts, and storymap-layer-scrub.test.ts to import and reuse it.
Preserve the existing GeoLibreLayer construction and helper call behavior across
all three test files.
- Around line 40-98: Split the combined test around projectChanged into separate
it() blocks for processing history, widgets, style library, metadata, layers,
and plugins. Keep each scenario’s newProject setup, mutation, and assertion
intact so one failing field does not prevent the remaining coverage from
running.
🪄 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: b1cb0b1f-8e90-4840-862e-c17fd7f7d3cf
📒 Files selected for processing (6)
apps/geolibre-desktop/src/hooks/useCollaboration.tspackages/core/src/store.tstests/collaboration-project-changed.test.tstests/core-project.test.tstests/layer-joins.test.tstests/storymap-layer-scrub.test.ts
💤 Files with no reviewable changes (1)
- apps/geolibre-desktop/src/hooks/useCollaboration.ts
Lock the removeLayerFromMap appliedBridgeState delete so a reused layer id with the same bridge still gets a fresh opacity apply.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/plugin-owned-paint.test.ts`:
- Around line 211-232: Extend the “re-applies after removeLayerFromMap when the
same layer id is reused” test to include a setVisibility callback and visibility
assertions alongside the existing opacity checks. Register and verify visibility
before removal, clear the recorded calls, then reuse the layer ID and assert
visibility is applied again, covering the shared cache behavior for both
operations.
🪄 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: c27b64f3-71f8-4fa1-a64e-5b6f467f00f9
📒 Files selected for processing (1)
tests/plugin-owned-paint.test.ts
Resolve the removeLayerGroup conflict with the nested layer groups work (opengeos#1593): keep main's recursive descendant-group collection and parent re-linking, and layer this branch's cleanup on top so join refresh, secondary-pane visibility scrubbing, and storymap ref scrubbing run over every layer in the removed group *subtree*, not just its direct children.
- tests/plugin-owned-paint.test.ts: the layer-id-reuse regression test now registers a `setVisibility` callback and asserts visibility alongside opacity, since `removeLayerFromMap` clears the cache both setters share (CodeRabbit). - tests/helpers/layer-fixtures.ts: extract the `geojsonLayer` skeleton that was duplicated in three suites into one shared fixture, so the required `GeoLibreLayer` fields cannot drift apart (CodeRabbit). - tests/core-project.test.ts, tests/storymap-layer-scrub.test.ts, tests/collaboration-project-changed.test.ts: use the shared fixture. - tests/collaboration-project-changed.test.ts: split the six-scenario broadcast-field test into one `it()` per field, so a failure in the first no longer hides regressions in the rest (CodeRabbit).
giswqs
approved these changes
Jul 31, 2026
This was referenced Aug 1, 2026
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
mapViewon parse/apply the same way secondary panes already do, so hand-edited cameras cannot land out of MapLibre range.projectChangedwithbuildProjectSnapshotso map grid, models, widgets, processing history, and style-library edits broadcast on their own.removeLayerFromMap.Test plan
node --import tsx --test tests/core-project.test.ts tests/collaboration-project-changed.test.ts tests/storymap-layer-scrub.test.ts(53 passing)Summary by CodeRabbit