fix(desktop): scrub Print Layout blocks when their layer is deleted - #1995
Conversation
Follow-up to #1994, which landed before these two review findings could be folded into it. - Scrub the Print Layout data/atlas blocks in removeLayer and deleteLayerGroup, alongside the storymap/widget/comment/legend scrubbing already there. Deleting a layer a block referenced, then saving without reopening the composer, wrote a dangling layer id into the project file; it only self-healed on the next load. Adds scrubPrintLayoutForRemovedLayers, the delete-time counterpart of the load-time scrub, matching scrubLegendForRemovedLayers' shape. - Gate the "fixed scale needs a physical page" correction on `open`, the last effect still writing a persisted field ungated. It also runs on the mount that every project load triggers, so a hand-edited file pairing a pixel page with scale mode would be corrected, and the project marked dirty, before the composer had ever been opened. Refs #1992
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 8 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughThe change adds shared print layout cleanup for removed layers, integrates it with layer and group deletion persistence, and limits atlas extent-mode normalization to open print layout dialogs. ChangesPrint Layout Layer Cleanup
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The change removes deleted-layer references from saved Print Layout data and prevents project opening from marking valid projects dirty. No actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant Store
participant PrintLayoutCleanup
participant Persistence
Store->>PrintLayoutCleanup: remove layer or destructive group
PrintLayoutCleanup-->>Store: cleaned print layout configuration
Store->>Persistence: persist updated state
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
apps/geolibre-desktop/src/components/layout/PrintLayoutDialog.tsxtypescript-eslint does not support TS 7.0. Oops! Something went wrong! :( ESLint: 10.8.1 Error: typescript-eslint does not support TS 7.0. packages/core/src/print-layout-config.tsESLint skipped: the matched ESLint configuration already failed (config-incompatibility). packages/core/src/store.tsESLint skipped: the matched ESLint configuration already failed (config-incompatibility).
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 |
|
Everything checks out. The tests correctly verify the new function's behavior, including reference-equality no-op returns. No inline comments are warranted — I'll finalize with a clean summary. Code reviewReviewed the diff and cross-checked against the sibling scrub helpers ( Bugs: None found. Security: No concerns — no user input handling, injection surface, or secrets involved. (Confidence: high) Performance: No issues — scrub functions return the same object reference when nothing changes, avoiding unnecessary re-renders/writes, consistent with existing helpers. (Confidence: high) Quality: Clean refactor — extracting CLAUDE.md: No violations — this is a bug fix, not a new abstraction, and doesn't touch any of the mirrored-constant or catalog-generation conventions called out in CLAUDE.md. Overall: a small, well-scoped, well-tested fix that faithfully follows the codebase's established scrub-on-delete pattern. Nothing to flag. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
🔍 Cloudflare PR preview
|
🔍 GitHub Pages PR preview
Note GitHub Pages built this preview successfully, but its serving edge returned HTTP 403 when checked. The links may still be propagating. |
Follow-up to #1994. Its last review round raised two findings after the PR had already been merged, so they land here instead.
Dangling layer id in a saved project
removeLayeranddeleteLayerGroupalready scrubstorymap,widgets,commentsandlegendfor the layers being deleted, but not the newprintLayout. Deleting a layer that a Print Layout data or atlas block referenced, then saving without reopening the composer, wrote a block pointing at a layer the file no longer carries. It self-healed on the next project load (via the load-time scrub) or the next time the composer was opened, leaving a window where the saved file disagreed with its own layer set, unlike every sibling section.Adds
scrubPrintLayoutForRemovedLayers, the delete-time counterpart of the existing load-timescrubPrintLayoutForLayers, takingstring | Iterable<string>to matchscrubLegendForRemovedLayersand its siblings. Both entry points now share one block-clearing helper, so they cannot drift.Opening a project could mark it dirty
One effect writing a persisted field was still ungated on
open:The dialog is mounted for the whole session and remounts on every project load, so this ran before the composer had ever been opened. A project pairing a pixel page size with
atlasExtentMode: "scale"(only reachable by hand-editing today, since the live UI keeps the two in sync) would be corrected on load, which pushed the corrected config into the store and marked the project dirty purely from opening it. Gated onopen, like the three layer-defaulting effects in #1994. Nothing acts on the pairing while the composer is closed, and opening it runs the correction.I audited every
useEffectin the file that writes one of the persisted fields; this was the last ungated one, and all four are now gated.Verification
npm run buildandnpm run test:frontend(6348 pass) are green. New tests cover the delete-time scrub (single id, id set, and the no-op case) and the store path: deleting a layer a block references clears that block and leaves a block on a surviving layer alone.Refs #1992
Summary by CodeRabbit