fix(print-layout): improve atlas feature framing - #1783
Conversation
|
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 (4)
📝 WalkthroughWalkthroughAtlas printing now supports polygon-feature masks, print-frame viewport fitting, and optional table filtering by the active atlas feature. The dialog clears mask state during lifecycle changes, guards capture and export, and adds English and French controls. ChangesAtlas print flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant PrintLayoutDialog
participant AtlasMask
participant MapLibreMap
participant AtlasExport
PrintLayoutDialog->>AtlasMask: show selected atlas feature mask
AtlasMask->>MapLibreMap: create or update inverted fill source
PrintLayoutDialog->>MapLibreMap: fit cropped print-frame bounds
PrintLayoutDialog->>AtlasExport: export filtered atlas page with map-fit 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
ESLint install failed: dependency version conflict. Check your lock file or package.json. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@apps/geolibre-desktop/src/components/layout/PrintLayoutDialog.tsx`:
- Around line 906-913: Update the mask-retention condition in the useEffect
within PrintLayoutDialog so it also requires an active atlas page and mask
availability before returning. When no active atlas page remains, or the mask
cannot be used, clear the map through clearAtlasFeatureMask; preserve the
existing behavior while open with atlas and mask enabled and an active page.
🪄 Autofix
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: dc39b33d-60f2-4ae4-91f0-b7fe71cab986
📒 Files selected for processing (10)
apps/geolibre-desktop/src/components/layout/PrintLayoutDialog.tsxapps/geolibre-desktop/src/i18n/locales/en.jsonapps/geolibre-desktop/src/i18n/locales/fr.jsonapps/geolibre-desktop/src/lib/print-atlas-mask.tsapps/geolibre-desktop/src/lib/print-atlas.tsapps/geolibre-desktop/src/lib/print-data-blocks.tsapps/geolibre-desktop/src/lib/print-layout.tstests/print-atlas.test.tstests/print-data-blocks.test.tstests/print-layout.test.ts
🔍 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. |
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 (3)
apps/geolibre-desktop/src/components/layout/PrintLayoutDialog.tsx (3)
826-836: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winLimit the mask option to the current polygon page.
atlasMaskAvailableis true when any feature in the layer is polygonal. Atlas pages can still contain point or line features.showAtlasFeatureMaskclears the mask for those features, but the UI still shows the toggle. A previous polygon mask can also remain visible until the delayed page drive runs.Base availability on the current page’s source feature, or restrict mask pages to Polygon and MultiPolygon features.
Also applies to: 2266-2279
🤖 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 `@apps/geolibre-desktop/src/components/layout/PrintLayoutDialog.tsx` around lines 826 - 836, Update atlasMaskAvailable and the related showAtlasFeatureMask flow in PrintLayoutDialog so mask availability is determined by the current atlas page’s source feature, not any polygon feature in the whole layer. Restrict the toggle and mask state to Polygon or MultiPolygon pages, clearing any existing mask immediately when the current page is non-polygonal.
1703-1722: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPass the atlas capture fit through
optionsForPage.
captureAtlasPagecallssetMapFit(...)instead of returning the effective atlas fit, sohandleAtlasExportreceives the previousmapFitviaoptions. Atlas pages therefore export with the wrong crop; if the capture usedcontain, graticule labels can be trimmed.🤖 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 `@apps/geolibre-desktop/src/components/layout/PrintLayoutDialog.tsx` around lines 1703 - 1722, The page options construction around optionsForPage must use the effective fit produced by captureAtlasPage rather than the stale options.mapFit value. Update captureAtlasPage to return or expose the fit applied by setMapFit, then pass that atlas fit through handleAtlasExport into optionsForPage so atlas exports preserve the actual contain/cover crop used during capture.
1231-1242: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCancel or block atlas work when the dialog closes.
Close does not stop
captureAtlasPage, so laterawait waitForAtlasSettle(map)can callshowAtlasFeatureMask(map, coverageFeature)ormap.fitBounds(...)after the dialog has closed. Abort the atlas export at close or block close untilatlasBusy/exportingfinishes, and guard MapLibre mutations with the cancellation state.🤖 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 `@apps/geolibre-desktop/src/components/layout/PrintLayoutDialog.tsx` around lines 1231 - 1242, The captureAtlasPage flow must not mutate the map after the dialog closes. Add cancellation handling tied to dialog close, abort or await active atlas export work, and check that cancellation state before showAtlasFeatureMask, clearAtlasFeatureMask, map.fitBounds, and after waitForAtlasSettle; preserve normal atlas export behavior while open.
🤖 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 `@apps/geolibre-desktop/src/components/layout/PrintLayoutDialog.tsx`:
- Around line 826-836: Update atlasMaskAvailable and the related
showAtlasFeatureMask flow in PrintLayoutDialog so mask availability is
determined by the current atlas page’s source feature, not any polygon feature
in the whole layer. Restrict the toggle and mask state to Polygon or
MultiPolygon pages, clearing any existing mask immediately when the current page
is non-polygonal.
- Around line 1703-1722: The page options construction around optionsForPage
must use the effective fit produced by captureAtlasPage rather than the stale
options.mapFit value. Update captureAtlasPage to return or expose the fit
applied by setMapFit, then pass that atlas fit through handleAtlasExport into
optionsForPage so atlas exports preserve the actual contain/cover crop used
during capture.
- Around line 1231-1242: The captureAtlasPage flow must not mutate the map after
the dialog closes. Add cancellation handling tied to dialog close, abort or
await active atlas export work, and check that cancellation state before
showAtlasFeatureMask, clearAtlasFeatureMask, map.fitBounds, and after
waitForAtlasSettle; preserve normal atlas export behavior while open.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0e82c33e-a7eb-4fac-aab5-90bfc56f4523
📒 Files selected for processing (1)
apps/geolibre-desktop/src/components/layout/PrintLayoutDialog.tsx
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| ? rowsForBlock(tableFeatureInfos, tableAllRows, tableFilterToPage, displayFilterBounds) | ||
| ? tableFilterToAtlasFeature && tableUsesAtlasLayer && currentAtlasPage | ||
| ? rowForAtlasFeature(tableAllRows, currentAtlasPage.sourceIndex) | ||
| : rowsForBlock(tableFeatureInfos, tableAllRows, tableFilterToPage, displayFilterBounds) |
There was a problem hiding this comment.
Edge case: when tableFilterToAtlasFeature is checked but currentAtlasPage is momentarily null (e.g. the atlas filter currently yields 0 pages, or a newly selected page hasn't finished driving), this silently falls back to rowsForBlock(...) (page-extent/full-layer rows) instead of showing an empty table. That contradicts the hint text ("This takes priority over the page extent filter") and could surprise a user who expects "only the current feature" to mean "none" rather than "whatever the page-extent filter would show."
Same pattern applies to the export path at line ~1709 (tableFilterToAtlasFeature && tableUsesAtlasLayer ? rowForAtlasFeature(...) : rowsForBlock(...)), though there pages[i] is always defined so it's less reachable in practice.
Confidence: medium — this is a real behavioral gap, though it only surfaces in the narrow window where the atlas is enabled with a matching table layer but has no current page.
| clearAtlasFeatureMask(map); | ||
| return false; | ||
| } | ||
| const collection: FeatureCollection<Polygon | MultiPolygon> = { |
There was a problem hiding this comment.
Minor efficiency nit: buildInvertedMask (in @geolibre/map) memoizes on the identity of the collection object it's given, but showAtlasFeatureMask constructs a brand-new FeatureCollection literal on every call. That means the memoization never hits and turf.mask() re-runs on every invocation — once per debounced page-drive and once per page during a full atlas export/print. For a dense polygon coverage layer this adds avoidable recompute on every export page. Not a correctness issue, just worth knowing the cache is effectively bypassed here.
Confidence: low — likely a negligible cost for typical polygon complexity, but could matter for large/complex coverage geometries exported across many pages.
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
The core fix ( |
| }); | ||
| } | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Test coverage gap (low-medium confidence). This new module has no accompanying test file. The codebase already has a precedent for testing this exact kind of MapLibre-mutating code with a fake Map stub — see tests/print-extent.test.ts, which was added specifically because print-extent.ts "had no coverage before" and is DOM/map-driven the same way this file is. clearAtlasFeatureMask/showAtlasFeatureMask are pure enough (a few getLayer/addLayer/removeSource calls plus the Polygon/MultiPolygon type guard) that a similar lightweight fake-map test would be cheap to add and would catch regressions in the guard logic or layer/source lifecycle.
| const collection: FeatureCollection<Polygon | MultiPolygon> = { | ||
| type: "FeatureCollection", | ||
| features: [feature as Feature<Polygon | MultiPolygon>], | ||
| }; | ||
| const mask = buildInvertedMask(collection); |
There was a problem hiding this comment.
Minor performance nit (low confidence). buildInvertedMask memoizes on the FeatureCollection object identity via a WeakMap (packages/map/src/derived-geometry.ts), but a brand-new collection object is constructed here on every call, so the cache never hits and @turf/mask's polygon-clipping union is recomputed from scratch each time — including every time captureAtlasPage re-applies the mask for the same page (e.g. after the fixed-scale zoom correction triggers a second settle+capture). Since it's one polygon per call this is likely cheap in practice, but for a large "features" atlas with many pages and the mask enabled it adds needless repeated work. Not blocking, just flagging since the memoization exists specifically for this kind of repeated-call scenario.
| const atlasMaskAvailable = Boolean( | ||
| atlasCoverage === "features" && | ||
| (currentAtlasFeature?.geometry?.type === "Polygon" || | ||
| currentAtlasFeature?.geometry?.type === "MultiPolygon"), | ||
| ); |
There was a problem hiding this comment.
Minor UX nit (low confidence). atlasMaskAvailable only looks at the current page's feature geometry. For a "features" coverage layer that mixes polygon and non-polygon geometries, the "Mask area outside current feature" toggle (and its state) will appear/disappear as the user steps through atlas pages — a page on a non-polygon feature hides the control even though atlasMaskEnabled stays true underneath and silently resumes on the next polygon page. That's probably fine functionally (the mask is inherently per-feature), but the checkbox popping in and out while paging may read as a glitch rather than an intentional per-page state. Consider basing availability on whether any page in the atlas has polygon geometry, if that's not already the intent.
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
| const canvas = map.getCanvas(); | ||
| const viewportWidth = canvas.clientWidth || canvas.width; | ||
| const viewportHeight = canvas.clientHeight || canvas.height; |
There was a problem hiding this comment.
canvas.clientWidth/clientHeight are CSS pixels, but the || canvas.width / || canvas.height fallback (used when the client size reads 0, e.g. a not-yet-laid-out or hidden canvas) returns device pixels (canvas.width = dispW * dpr per line ~1608). atlasViewportFrame's result feeds both fitBounds's padding and the map.unproject([x, y]) calls a few lines below (line ~1307), both of which expect CSS-pixel screen coordinates. If the fallback path is ever hit on a HiDPI display, the computed padding/crop rectangle will be off by the device pixel ratio, skewing the atlas fit and the frameBounds used to filter data blocks.
Confidence: low-to-medium — this only triggers when clientWidth/clientHeight are 0 while an atlas capture runs, which should be rare since the map must be visible to drive the camera, but the fallback silently produces wrong units rather than failing loudly.
| paint: { | ||
| "fill-color": "#ffffff", |
There was a problem hiding this comment.
The mask fill is a hardcoded #ffffff at 0.7 opacity. On a dark basemap/theme this reads as a bright white overlay rather than an adaptive "de-emphasize" treatment — worth confirming this is the desired look for the dark-theme case the PR description says was tested, since GeoLibre otherwise threads light/dark awareness through most UI-facing rendering.
Confidence: low — this may well be an intentional, theme-independent print-composition choice (print output is typically a light background regardless of app theme).
|
Both inline comments posted successfully. Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
| if (source) source.setData(mask); | ||
| else map.addSource(SOURCE_ID, { type: "geojson", data: mask }); | ||
| if (!map.getLayer(FILL_LAYER_ID)) { | ||
| map.addLayer({ |
There was a problem hiding this comment.
addLayer here has no beforeId, so the mask fill always lands at the very top of the style, above the graticule label layer (GRATICULE_LABEL_LAYER_ID). captureAtlasPage explicitly switches to mapFit: "contain" specifically so the graticule's edge coordinate labels aren't cropped — but if a user also enables "Mask area outside current feature" on a page where the polygon doesn't fill the frame, the 70%-opacity white mask will sit on top of those edge labels (which are typically drawn right at/near the edges, i.e. in the masked "outside" region) and wash them out. Consider passing a beforeId (e.g. the graticule label layer, when present) so the mask renders under labels that should stay legible.
Confidence: medium — this is a plausible visual regression for the graticule + polygon-mask combination, but I haven't run the app to confirm the labels actually fall in the masked region.
| "filterToAtlasFeature": "Seulement l’entité courante de l’atlas", | ||
| "filterToAtlasFeatureHint": "Disponible lorsque la table utilise la couche de couverture de l’atlas. Ce filtre est prioritaire sur l’emprise de la page." |
There was a problem hiding this comment.
Minor style nit: these two new strings use a curly apostrophe (’) in "l’entité"/"l’atlas", while the rest of this file consistently uses a straight apostrophe ('), e.g. filterToPageHint right above uses "S'applique", "dataChart.noNumericFields" uses "n'a aucun", etc. Same for atlas.maskOutside/maskOutsideHint below. Worth normalizing for consistency with the rest of the catalog.
Confidence: low — purely cosmetic, doesn't affect functionality.
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
Overall the core fix (fitting the atlas camera to the actual cover-cropped print frame, and deriving |
Summary
Testing
npm run test:frontendnpm run buildpre-commit run --all-filesAddresses #1778
Summary by CodeRabbit
New Features
Bug Fixes