feat(processing): make the Whitebox toolbox a floating panel - #1226
Conversation
Replace the Radix modal with a non-modal, draggable floating window so the map stays interactive while a tool is open. This matters for the COG/WMS/XYZ subset tools, whose "Use map extent" affordance needs the user to pan/zoom the map without dismissing the dialog. Mirrors the RecordVideoDialog pattern: a fixed, draggable panel with a grip title bar, close button, and Escape-to-close; no backdrop overlay.
✅ Deploy Preview for geolibre-app ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Warning Review limit reached
Next review available in: 17 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
ChangesProcessing panel and extent workflow
Whitebox availability and menu visibility
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ProcessingDialog
participant PrintExtent
User->>ProcessingDialog: Activate Draw on map
ProcessingDialog->>PrintExtent: Start drawPrintExtent
PrintExtent-->>ProcessingDialog: Return selected bounds
ProcessingDialog->>ProcessingDialog: Validate and apply bbox extent
User->>ProcessingDialog: Toggle drawing or close panel
ProcessingDialog->>PrintExtent: Abort active extent drawing
Possibly related PRs
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 |
⚡ Cloudflare Pages preview
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| const onDragMove = (event: React.PointerEvent) => { | ||
| if (!dragOffset.current) return; | ||
| const width = panelRef.current?.offsetWidth ?? 0; | ||
| const height = panelRef.current?.offsetHeight ?? 0; | ||
| // Keep the panel within the viewport so it can't be dragged off-screen. | ||
| const x = Math.max( | ||
| 0, | ||
| Math.min(event.clientX - dragOffset.current.x, window.innerWidth - width), | ||
| ); | ||
| const y = Math.max( | ||
| 0, | ||
| Math.min( | ||
| event.clientY - dragOffset.current.y, | ||
| window.innerHeight - height, | ||
| ), | ||
| ); | ||
| setPos({ x, y }); | ||
| }; |
There was a problem hiding this comment.
Performance: drag now re-renders the full (unvirtualized) tool list on every pointer-move frame
onDragMove calls setPos(...) on every pointermove event while dragging. pos is state on the top-level ProcessingDialog component, so each move re-renders the whole component tree — including the tool list at line ~1367 (filteredTools.map(...)), which is a plain, non-virtualized map that can hold up to the "~700 Whitebox tools" mentioned elsewhere in this file (line 721). Before this PR there was no drag interaction, so this expensive subtree only re-rendered on meaningful actions (typing in search, selecting a tool); now it re-renders at pointer-move frequency for the entire duration of every drag.
In practice this is likely fine at typical list sizes/reconciliation speed, but with the full catalog loaded it could show visible jank while dragging. Consider isolating the list (e.g. React.memo on the row/list, or moving pos into a ref + directly mutating the panel's inline style via useLayoutEffect/direct DOM write instead of triggering a React re-render per frame) if this turns out to be perceptible.
Confidence: medium — a plausible perf regression under real-world list sizes, but not verified with profiling, and the same pattern (pos as top-level state) is already used in RecordVideoDialog/RecordTourDialog, just without a large list behind it.
There was a problem hiding this comment.
Leaving as-is for now (open for your call). This mirrors the accepted pos-as-state pattern in RecordVideoDialog/RecordTourDialog; the difference here is the ~700-item unvirtualized tool list behind it. It is unverified (no profiling shows jank), and the clean fixes (memoizing/extracting the list, or moving pos to a ref + direct style writes) are a non-trivial refactor I would rather not do speculatively. Happy to optimize if profiling with the full catalog shows perceptible drag jank.
There was a problem hiding this comment.
Still deferring (leaving open for a maintainer call). Unchanged reasoning: this mirrors the accepted pos-as-state pattern in RecordVideoDialog/RecordTourDialog, and the perf hit is unverified (no profiling shows jank). The clean fixes (memoizing/extracting the list or ref + direct DOM writes) are a non-trivial refactor I would rather not do speculatively for a floating-panel drag. Happy to optimize if profiling with the full ~700-tool catalog shows perceptible jank.
Code reviewI reviewed the diff ( Bugs: None found. The drag/pointer-capture logic, viewport clamping, Escape-to-close, and the Security: None found. No new user input handling, no injected HTML, no secrets. Performance:
Quality:
CLAUDE.md: No violations beyond the i18n nit above (grouped under Quality). The new floating-panel pattern correctly reuses the |
|
(typo fix in my review comment above: "whitespid" should read "whitespace" — no change to the findings.) |
![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/processing/ProcessingDialog.tsx`:
- Around line 1225-1230: Update the title-bar drag handle around onDragStart,
onDragMove, and onDragEnd to be keyboard accessible: make it focusable and
handle arrow-key movement, or add explicit move/reset controls. Ensure keyboard
movement repositions the panel consistently with pointer dragging and preserves
access to the map beneath.
- Around line 1209-1216: Update the ProcessingDialog component’s open/close
lifecycle around the panelRef element to move focus to the panel or its first
control when the dialog opens, then restore focus to the previously active
element when it closes. Preserve the existing conditional rendering and ensure
the focus behavior handles repeated open/close cycles without targeting an
unmounted element.
- Line 1215: Update the new Whitebox panel title, its aria-label, and the
runtime fallback messages in the processing dialog to use the existing
react-i18next `t()` translation function instead of hard-coded strings. Add the
corresponding translation keys and preserve the current displayed text as their
default values.
- Around line 477-494: The saved panel position is not re-clamped when the
viewport changes. In the ProcessingDialog open lifecycle, add a resize handler
that reads the current panel dimensions, clamps the existing pos coordinates to
the viewport bounds, and updates state; register it while open and remove it on
cleanup so reopening after a resize also restores an on-screen position.
🪄 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: 596ca57c-7a4d-4141-a994-181948c606f0
📒 Files selected for processing (1)
apps/geolibre-desktop/src/components/processing/ProcessingDialog.tsx
- Route the new panel title, aria-label, and runtime fallback strings through t() (new i18n keys) so they are translatable and the title/aria-label share one source instead of two hardcoded copies. - Move focus into the panel on open (deferred two frames past the Radix menu's own focus restore) and best-effort restore it on close, replacing the focus management lost when the modal was removed. - Re-clamp a dragged position on window resize so the panel can't be left off-screen after the viewport shrinks. - Re-indent the content block one level shallower now that it is no longer wrapped in Dialog/DialogContent.
Code reviewReviewed the conversion of the Whitebox Processing dialog from a Radix modal to a draggable, non-modal floating panel ( Bugs
Security
Performance
Quality
CLAUDE.md
Also verified: no rules-of-hooks violation from the late |
Add a bottom-right resize grip that grows the panel from its pinned top-left, clamped to a usable minimum (560x400) and the viewport. Size is re-clamped on window resize alongside position. Also cap the default height to leave room for the top offset and a bottom margin so the whole panel, including the grip, stays on screen at small viewport heights.
Code reviewReviewed the conversion of the Whitebox Processing dialog from a Radix modal to a draggable, resizable, non-modal floating panel ( Bugs
Security
Performance
Quality
CLAUDE.md
|
Alongside "Use map extent", the COG/WMS/XYZ subset tools now offer "Draw on map": rubber-band a box on the map to fill the bbox (and bbox_crs 4326). This is only workable now that the panel is non-modal. Reuses the print-extent draw helper (pan/zoom suspended during the drag, box preview, Escape/blur handling); the drawn box is cleared afterward. Escape during a draw cancels the draw instead of closing the panel, and closing the panel or toggling the button aborts an in-flight draw. The fill/validation is shared with "Use map extent".
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/processing/ProcessingDialog.tsx`:
- Around line 1044-1085: Update the bbox context handlers around
handleUseMapExtent, handleDrawBbox, and tool selection so any active draw is
aborted before applying the map extent or switching tools. Ensure the pending
draw cannot later call applyBboxExtent or update the newly selected tool’s
values, while preserving normal drawing behavior when the context remains
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: 18e6357e-497a-4b25-b31d-6651be81d207
📒 Files selected for processing (2)
apps/geolibre-desktop/src/components/processing/ProcessingDialog.tsxapps/geolibre-desktop/src/i18n/locales/en.json
Code reviewReviewed the conversion of the Whitebox Processing dialog from a Radix modal to a draggable/resizable non-modal floating panel, plus the new "Draw on map" bbox tool for the COG/WMS/XYZ subset parameters. Compared the new drag/resize code against the existing Bugs
Security
Performance
Quality
CLAUDE.md
Everything else, the drag/pin/clamp logic, the shared |
- Abort an in-flight "Draw on map" when the selected tool changes or "Use map extent" is clicked, so a late-resolving box can't fill the wrong tool's bbox. - Scope the panel's Escape-to-close to when focus is inside the panel, so Escaping an unrelated map/panel interaction doesn't also close it. - Cap the resize minimum to the room available to the viewport edge (and clamp the re-clamp effect's minimum to the viewport), so a panel pinned near an edge or a small viewport shrinks to fit instead of being pushed off-screen. - Clamp size/position once on open (not only on resize), fixing a stale off-screen position when the window shrank while the panel was closed. - Split toolsAvailable into _one/_other plural forms per the i18next convention.
The Whitebox toolbox and its WASI/GeoLibre tool catalog run entirely in the browser via WebAssembly (the dialog defaults to the WASM runner off-desktop), so unlike the sidecar-backed tools they don't need a Python process. Drop the mobile gate on the Whitebox menu item and its tool categories so they're available on Android/iOS; Conversion, Raster tools, and AI Segmentation stay hidden on mobile since they still require the sidecar.
Code reviewReviewed the full diff ( Bugs
Security
Performance
Quality
CLAUDE.md
|
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)
apps/geolibre-desktop/src/components/processing/ProcessingDialog.tsx (1)
600-639: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
posre-clamp reads stale panel dimensions in the same pass that resizes it.
setSize(...)(Lines 603-615) queues the viewport-capped width/height, but the very next lines readpanel.offsetWidth/offsetHeight(Lines 625, 629) to clamppos— since React defers the DOM commit until after this synchronous callback returns, those reads still reflect the pre-clamp layout. If the panel was oversized before this run,posgets clamped against the old (larger) dimensions instead of the new ones just computed, so the position can end up more/less clamped than the just-resized panel actually needs. It self-corrects on the next resize event, but is inconsistent for this pass.♻️ Proposed fix: derive the clamped width/height instead of reading the DOM
const clampToViewport = () => { - setSize((current) => - current - ? { - w: clamp( - current.w, - Math.min(PANEL_MIN_W, window.innerWidth), - window.innerWidth, - ), - h: clamp( - current.h, - Math.min(PANEL_MIN_H, window.innerHeight), - window.innerHeight, - ), - } - : null, - ); - const panel = panelRef.current; - if (!panel) return; + const panel = panelRef.current; + let nextW = panel?.offsetWidth; + let nextH = panel?.offsetHeight; + setSize((current) => { + if (!current) return null; + nextW = clamp( + current.w, + Math.min(PANEL_MIN_W, window.innerWidth), + window.innerWidth, + ); + nextH = clamp( + current.h, + Math.min(PANEL_MIN_H, window.innerHeight), + window.innerHeight, + ); + return { w: nextW, h: nextH }; + }); + if (!panel || nextW === undefined || nextH === undefined) return; setPos((current) => current ? { - x: Math.max( - 0, - Math.min(current.x, window.innerWidth - panel.offsetWidth), - ), - y: Math.max( - 0, - Math.min(current.y, window.innerHeight - panel.offsetHeight), - ), + x: Math.max(0, Math.min(current.x, window.innerWidth - nextW)), + y: Math.max(0, Math.min(current.y, window.innerHeight - nextH)), } : null, ); };🤖 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/processing/ProcessingDialog.tsx` around lines 600 - 639, Update the clampToViewport callback in the open viewport effect to derive the viewport-capped width and height once, then use those dimensions when clamping pos instead of reading panelRef.current.offsetWidth/offsetHeight. Preserve the existing setSize behavior and ensure both size and position are calculated from the same clamped dimensions during a single pass.
🤖 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/processing/ProcessingDialog.tsx`:
- Around line 600-639: Update the clampToViewport callback in the open viewport
effect to derive the viewport-capped width and height once, then use those
dimensions when clamping pos instead of reading
panelRef.current.offsetWidth/offsetHeight. Preserve the existing setSize
behavior and ensure both size and position are calculated from the same clamped
dimensions during a single pass.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 73ca22b1-65c1-46c6-a8c3-1f7bc4f1ee7c
📒 Files selected for processing (2)
apps/geolibre-desktop/src/components/processing/ProcessingDialog.tsxapps/geolibre-desktop/src/i18n/locales/en.json
- Coalesce the window-resize clamp with requestAnimationFrame so a live OS window-resize drag re-renders the tool browser once per frame instead of on every resize event, matching the rAF pattern used by the pointer handlers. - Label the panel via aria-labelledby pointing at the h2 title's id, so the accessible name shares the single visible text node instead of a second t() lookup that could drift from the title.
| if (!rect) return; | ||
| // Pin the top-left so the panel grows toward the grip instead of staying | ||
| // centered (the default placement uses a translate to center it). | ||
| setPos({ x: rect.left, y: rect.top }); |
There was a problem hiding this comment.
Bug (medium confidence): onResizeStart fires on every pointerdown on the grip, including a plain click with no subsequent drag, and unconditionally calls setPos({ x: rect.left, y: rect.top }). That permanently switches the panel from the responsive, CSS-centered placement (left-1/2 top-16 -translate-x-1/2) to an explicitly pinned position — even though nothing was actually resized (onResizeEnd only clears resizeStart.current, never pos).
Net effect: merely touching the resize grip (e.g. an accidental click) un-centers the panel for the rest of the session, and afterwards a browser-window resize no longer re-centers it (only re-clamps it to stay on-screen via the viewport effect).
Consider only committing to an explicit pos once real movement is observed (e.g. set it lazily on the first onResizeMove, or track a small drag-distance threshold) rather than unconditionally on pointerdown.
| }} | ||
| /> | ||
| ) : ( | ||
| error && ( | ||
| <p className="flex items-center gap-2 text-sm text-destructive"> | ||
| <AlertCircle className="h-4 w-4" /> | ||
| {error} | ||
| </p> | ||
| ) | ||
| )} | ||
| {job && ( |
There was a problem hiding this comment.
Accessibility (medium confidence): the resize grip is pointer-only (onPointerDown/onPointerMove/onPointerUp on a role="presentation" div) with no keyboard equivalent — a keyboard-only user has no way to enlarge the panel beyond its default/last-dragged size. This is called out honestly in the comment above, but it's still a real WCAG 2.1.1 (keyboard operable) gap worth tracking, e.g. a follow-up that lets arrow keys resize the panel when the grip (or the panel itself) has focus.
| > | ||
| <span className="block truncate font-medium"> | ||
| {tool.locked ? "[Locked] " : ""} | ||
| {toolLabel(tool)} |
There was a problem hiding this comment.
Quality / CLAUDE.md i18n convention (low-medium confidence): this PR wraps the neighboring runtime-status strings in the same panel with t() (processing.whitebox.checkingRuntime, .toolsAvailable, .runtimeUnavailable, .toolbox), but these sibling strings in the same touched block are still hardcoded English and bypass t():
- line 1615:
"No tool selected" - line 1664:
"This tool is locked." - line 1673:
"This tool has no parameters."
They're pre-existing (just re-indented here), so this is a minor nit rather than a regression, but since the PR is already touching/reformatting this exact code and per CLAUDE.md "Use t() for new user-facing strings" for the surrounding additions, it'd be a small, low-risk win to fold these into the same i18n pass while the file is already open.
Code reviewReviewed the diff for feat(processing): make the Whitebox toolbox a floating panel (ProcessingMenu.tsx, ProcessingDialog.tsx, en.json, is-mobile.ts). This converts the Whitebox dialog from a Radix modal to a draggable/resizable, non-modal floating panel, and adds Draw on map as a second way to fill a bbox parameter alongside Use map extent. Bugs
Security
Performance
Quality
CLAUDE.md adherence
Overall this is a well-considered, carefully-commented change that mirrors an existing pattern (RecordVideoDialog) and adds thoughtful extras (viewport re-clamping, escape-to-close scoped to focus-within, abort-safe draw handling) beyond what it mirrors. The findings above are edge cases and polish items, not blockers. |
| // The Whitebox toolbox (and its WASI/GeoLibre tool catalog) runs entirely in | ||
| // the browser via WebAssembly, so unlike the sidecar-backed tools it stays | ||
| // available on mobile. | ||
| const showWhitebox = show("processing.whitebox"); |
There was a problem hiding this comment.
Bug (medium-high confidence): Making the Whitebox toolbox reachable on mobile exposes a case ProcessingDialog.tsx doesn't handle: its desktop flag is just isTauri() (ProcessingDialog.tsx:446), which is also true inside a Tauri Android/iOS webview — per is-mobile.ts's own comment, "the Tauri Android webview reports an Android UA."
Every other sidecar-backed dialog (Conversion, Raster, Segmentation) avoids this because it stays behind !mobile && show(...) here, so it's never opened on mobile at all. Whitebox is now the one exception, but ProcessingDialog's runLocal still defaults to !desktop (i.e. false, sidecar mode) on a mobile Tauri build, and its "Start server" button is gated only on desktop, not !mobile — so a mobile Tauri user opening this dialog lands in sidecar mode by default with a "Start server" button that calls startGeoLibreSidecar(), which cannot succeed on Android/iOS. They'd have to discover and manually check "Run locally" to get a working WASM tool.
Worth defaulting runLocal/gating the server buttons off desktop && !mobile instead of desktop alone.
| </button> | ||
| </div> | ||
|
|
||
| <div className="grid min-h-0 flex-1 grid-cols-[minmax(260px,320px)_minmax(0,1fr)] gap-4 overflow-hidden p-5"> |
There was a problem hiding this comment.
Quality/UX (medium confidence): This two-column layout (grid-cols-[minmax(260px,320px)_minmax(0,1fr)], overflow-hidden) is unchanged from the modal version and was never responsive for narrow viewports — but that was fine while Whitebox stayed hidden on mobile (ProcessingMenu.tsx's old !mobile && show(...) gate). Now that ProcessingMenu.tsx shows Whitebox on mobile (including a plain mobile browser, not just Tauri), the panel's own width can be as little as 95vw, and the tool-list column alone needs 260–320px minimum — on a ~360–400px-wide phone that leaves little to no room for the parameter form on the right, with overflow-hidden clipping rather than scrolling it into view.
Since this PR is what makes the panel reachable on phone-width screens for the first time, it may be worth a narrow-viewport fallback (e.g. stack the columns, or drop to a single-column list at small widths) rather than relying on the desktop-oriented two-column grid unconditionally.
| const onDragMove = (event: React.PointerEvent) => { | ||
| if (!dragOffset.current) return; | ||
| const width = panelRef.current?.offsetWidth ?? 0; | ||
| const height = panelRef.current?.offsetHeight ?? 0; | ||
| // Keep the panel within the viewport so it can't be dragged off-screen. | ||
| const x = Math.max( | ||
| 0, | ||
| Math.min(event.clientX - dragOffset.current.x, window.innerWidth - width), | ||
| ); | ||
| const y = Math.max( | ||
| 0, | ||
| Math.min( | ||
| event.clientY - dragOffset.current.y, | ||
| window.innerHeight - height, | ||
| ), | ||
| ); | ||
| setPos({ x, y }); | ||
| }; | ||
|
|
||
| const onDragEnd = (event: React.PointerEvent) => { | ||
| dragOffset.current = null; | ||
| event.currentTarget.releasePointerCapture?.(event.pointerId); | ||
| }; | ||
|
|
||
| const onResizeStart = (event: React.PointerEvent) => { | ||
| // Don't also start a header drag; the grip lives outside the header but stop | ||
| // propagation defensively. | ||
| event.stopPropagation(); | ||
| const rect = panelRef.current?.getBoundingClientRect(); | ||
| if (!rect) return; | ||
| // Pin the top-left so the panel grows toward the grip instead of staying | ||
| // centered (the default placement uses a translate to center it). | ||
| setPos({ x: rect.left, y: rect.top }); | ||
| resizeStart.current = { | ||
| x: event.clientX, | ||
| y: event.clientY, | ||
| w: rect.width, | ||
| h: rect.height, | ||
| left: rect.left, | ||
| top: rect.top, | ||
| }; | ||
| event.currentTarget.setPointerCapture(event.pointerId); | ||
| }; | ||
|
|
||
| const onResizeMove = (event: React.PointerEvent) => { |
There was a problem hiding this comment.
Performance (medium confidence): onDragMove/onResizeMove call setPos/setSize directly on every raw pointermove event, with no per-frame coalescing — unlike drawPrintExtent's queueMove/rAF-batching pattern used elsewhere in this very PR (lib/print-extent.ts). Each call re-renders the whole panel, including the (potentially ~700-row) tool list and the parameter form.
This mirrors RecordVideoDialog's existing drag handler, so it's a pre-existing pattern in the codebase, but RecordVideoDialog's tree is much lighter than this dialog's — dragging/resizing the Whitebox panel while a large tool list/param form is mounted is more likely to visibly jank on slower machines. Consider coalescing to one setPos/setSize per animation frame, the way drawPrintExtent already does for the rubber-band preview.
| const bounds = mapControllerRef.current?.readView().bbox; | ||
| if (!bounds) { | ||
| setError(t("processing.whitebox.mapExtentUnavailable")); | ||
| return; |
There was a problem hiding this comment.
Quality (low-medium confidence): applyBboxExtent is now shared between "Use map extent" and the new "Draw on map" (below), but the error copy it shows on an invalid/unavailable box — mapExtentInvalid: "The current map view can't be used as a bounding box…" and mapExtentUnavailable: "The map view is not available yet…" — still only makes sense for the map-extent path. A drawn box that happens to fail this same validation (e.g. spans the antimeridian, per drawPrintExtent's documented limitation) would show a message about "the current map view" even though the user just hand-drew a box, which reads as confusing/wrong for that caller.
Might be worth caller-agnostic copy (or a second pair of strings for the draw path) now that this validation is shared between the two entry points.
Code reviewReviewed the conversion of the Whitebox Processing dialog from a Radix modal to a draggable, resizable non-modal floating panel, plus the new "Draw on map" bbox affordance and the mobile-visibility change in Bugs
Performance
Quality
Security
CLAUDE.md adherence
|
The "Draw on map" preview reused drawPrintExtent's MapLibre fill/line box, which is occluded by the interleaved deck.gl raster overlay (WebGPU) — the same reason RasterSubsetPanel draws its box as an SVG. Add an optional onPreview callback and a drawBox toggle to drawPrintExtent so the caller can render its own preview; the subset dialog now draws the box as a viewport-space SVG portaled to <body> at a higher z-index, so it stays visible above the raster. Print Layout is unchanged (drawBox defaults on, onPreview unset).
| // The Whitebox toolbox (and its WASI/GeoLibre tool catalog) runs entirely in | ||
| // the browser via WebAssembly, so unlike the sidecar-backed tools it stays | ||
| // available on mobile. | ||
| const showWhitebox = show("processing.whitebox"); |
There was a problem hiding this comment.
Bug (medium confidence): Removing the !mobile gate here means the Whitebox toolbox is now shown on Tauri Android/iOS too, but ProcessingDialog.tsx still defaults runLocal to useState(!desktop), and desktop = isTauri() is true on Tauri mobile builds (per the isMobile() doc comment: "the Tauri Android webview reports an "Android" UA"). So on Tauri Android/iOS, runLocal defaults to false (sidecar mode) — but the sidecar can never run on mobile, so runtimeAvailable stays false and the Run button (disabled={... (!runLocal && runtimeAvailable !== true)}) stays disabled until the user manually checks "Run locally". The toolbox becomes visible-but-non-functional by default on that platform, which cuts against this PR's stated rationale ("unlike the sidecar-backed tools it stays available on mobile").
Suggest defaulting runLocal to something like !desktop || isMobile() so Tauri-mobile also opens straight into WASM mode.
| const handleUseMapExtent = () => { | ||
| // Clear any stale banner (e.g. a prior "map not ready") so a later success | ||
| // doesn't leave it lingering, mirroring RasterSubsetPanel.handleUseView. | ||
| // Validate a WGS84 box and write it into the `bbox`/`bbox_crs` fields. Shared |
There was a problem hiding this comment.
Quality (low confidence): This now stacks two comment blocks that explain the same thing. The lines just above (1054-1057) are the original comment written for handleUseMapExtent (references GeoLibre#1213) but now sit above the renamed/shared applyBboxExtent, immediately followed by this new block covering the same validation logic from a slightly different angle. The actual handleUseMapExtent a few lines below (now a 3-line wrapper) is left with no comment of its own. Consider merging these into one comment on applyBboxExtent and dropping the stale block, or moving the GH#1213 context down to handleUseMapExtent.
Code reviewReviewed the diff ( Bugs
Security
Performance
Quality
CLAUDE.md adherence
No security, correctness-blocking, or CLAUDE.md-violation issues beyond what is listed above; the two items above (mobile runLocal default, stale comment) are the only points worth a second look before merge. |
Replace the Radix modals with draggable, non-modal floating windows (mirroring RecordVideoDialog's pattern, as the Whitebox toolbox did in #1226) so the map and attribute table stay interactive while selecting: run a selection, pan to inspect the highlight, refine, and re-run without reopening. Both panels can be open at once. Since the camera can now move while the panel is open, Select by Expression re-snapshots ["zoom"] and the @map_* variables at each run instead of at open (the modal Expression Builder keeps its open-time snapshot for stable prop identities). The redundant footer Close button is dropped in favor of the title-bar X.
* feat(selection): Select by Expression and Select by Location (#1314) Interactive feature selection, QGIS style: two new dialogs turn expression matches or spatial-predicate matches into the live selection (highlighted on the map and in the attribute table), with four combine modes (new/add/remove/ intersect) plus selection actions (zoom to, invert, clear, export as layer) in the Edit menu and the vector layer context menu. - core: selection helpers (featureSelectionId, applySelectionMode, invertSelection) and matchFeaturesByExpression (compile-once batch boolean evaluation with per-feature runtime-error counting) - processing: extract matchFeaturesByLocation from the Select by location tool and share it with the new dialog; export MAX_CLIENT_PAIRS and the predicate types - app: SelectByExpressionDialog (reuses the shared Expression Builder), SelectByLocationDialog, selection actions lib, Edit menu + layer context menu entries with ui-profile toggles, i18n strings - StylePanel: extract getAttributePropertyNames/standardExpressionVariables into lib/expression-inputs so all expression entry points share them - tests: tests/feature-selection.test.ts (21 cases) Verified end-to-end with Playwright against real data: 9/1000 US cities match population > 1M; cities vs states intersects/disjoint = 992/8 (matches shapely ground truth); modes, invert, clear, export all correct. * Address review feedback - Gate "Select by Location..." on two selectable layers in both the Edit menu and the layer context menu, so it can't open straight into the "needs two vector layers" dead end (Claude review) - Move applyMatchedSelection into lib/selection-actions.ts and add four store-integration tests covering the layer-id guard and the selectLayer-before-selectFeatures ordering (Claude review) - Compute the MAX_CLIENT_PAIRS check from geometry-bearing features only, matching the Select by location processing tool's accounting (Claude review) - Deduplicate remove/intersect results in applySelectionMode to honor the documented dedup guarantee, with tests (CodeRabbit) - Reset the selection dialogs' open state and preselected layer ids in newProject/loadProject so a stale dialog can't survive a project switch (CodeRabbit) - Add a directional "contains" predicate test (CodeRabbit) * Address Claude review feedback - Force mode "new" (and disable add/remove/intersect in the mode dropdown) when the dialog's target layer does not hold the live selection, so remove/intersect can no longer silently produce an always-empty selection across layers; document the cross-layer fallback accurately in applyMatchedSelection's docstring * feat(selection): render Select by Expression/Location as floating panels Replace the Radix modals with draggable, non-modal floating windows (mirroring RecordVideoDialog's pattern, as the Whitebox toolbox did in #1226) so the map and attribute table stay interactive while selecting: run a selection, pan to inspect the highlight, refine, and re-run without reopening. Both panels can be open at once. Since the camera can now move while the panel is open, Select by Expression re-snapshots ["zoom"] and the @map_* variables at each run instead of at open (the modal Expression Builder keeps its open-time snapshot for stable prop identities). The redundant footer Close button is dropped in favor of the title-bar X. * feat(selection): anchor the floating panels to the map canvas top-left Mount the selection panels inside the map area (next to the plugin FloatingPanels overlay) and switch the shell from viewport-fixed to an absolute inset-0 pass-through layer, so the panels open in the map canvas's top-left corner and drag-clamp to the map area instead of the whole window — they can no longer cover the sidebars.
Summary
Test plan
tsc -bandpre-commit run --files(eslint + npm build) passSummary by CodeRabbit
New Features
Bug Fixes
Documentation