fix(shell): recover a drop overlay that outlived its drag - #1683
Conversation
The drag-and-drop overlay is one boolean fed from two independent places. The webview drag handlers balance it with a depth counter, but on desktop Tauri's native onDragDropEvent sets it with no counter at all, since the OS reports enter/leave/drop directly. Either feed can strand it: a native "leave" the OS never delivers (what a modal native file dialog opening mid-drag produces on WebKitGTK, which is how the reporter hit it via the style import dialog), or an unbalanced webview enter/leave pair that leaves the depth counter above zero. Both leave the overlay covering the map with nothing left to clear it, which is why neither Escape nor a resize helped and only dragging another file across the window dismissed it. Rather than chase every way the OS can swallow a drag event, recover on two signals that cannot occur while a real drag is in progress: a key press and a pointer button. HTML5 drags suppress mouse events for their duration and native drags hold a pointer grab, so neither can fire mid-drag and dismiss the overlay out from under a drag the user is actually performing. - Escape clears the overlay, which is what the reporter reached for first. - A pointer press clears it too, so it heals on the next interaction without the user knowing to press anything. - Both reset the depth counter, so the two feeds cannot disagree afterward. - Add a data-testid to the overlay and e2e cover for both recovery paths plus a guard that a balanced drag still shows and hides it. Fixes #1664
|
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 (2)
📝 WalkthroughWalkthroughDesktopShell now recovers stranded file-drop overlays after Escape or pointer interaction. Playwright tests cover stale overlay dismissal, balanced drag lifecycles, and GeoJSON layer imports. ChangesFile-drop overlay recovery
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 💡 1📝 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 |
There was a problem hiding this comment.
Pull request overview
Fixes GeoLibre#1664 by adding recovery paths to ensure the file drag-and-drop overlay cannot remain stuck over the map after a drag sequence is interrupted (notably when native drag events don’t deliver a matching leave), and by adding E2E regression coverage.
Changes:
- Add an “escape hatch” effect in
DesktopShellto clear the stranded drop overlay onEscapeand on the next pointer press, resetting bothisDraggingFilesanddragDepthRef. - Add a stable
data-testidto the overlay to make its presence/assertions testable. - Introduce Playwright E2E tests that intentionally strand the overlay via unbalanced drag events and assert both recovery behaviors plus the normal balanced enter/leave path.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| apps/geolibre-desktop/src/components/layout/DesktopShell.tsx | Adds global recovery listeners to clear a stranded drag overlay; adds data-testid for test targeting. |
| e2e/drop-overlay.spec.ts | Adds regression tests covering “stuck overlay” recovery via Escape and pointer press, plus a balanced-drag guard. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
🔍 Cloudflare PR preview
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@apps/geolibre-desktop/src/components/layout/DesktopShell.tsx`:
- Around line 1839-1844: Update the onKeyDown handler to call clear() for every
keydown event rather than only when event.key is "Escape". Add a regression test
that dispatches a non-Escape key and verifies the overlay state is cleared.
In `@e2e/drop-overlay.spec.ts`:
- Around line 58-65: Add a drop-handler test alongside “a balanced drag still
shows and hides the overlay” that uses the existing file-drag helper to drop
supported GeoJSON, then waits for and asserts the resulting imported layer or
successful drop status. Keep the existing overlay visibility 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: f2623747-456a-4c19-aa29-e546f5f7659c
📒 Files selected for processing (2)
apps/geolibre-desktop/src/components/layout/DesktopShell.tsxe2e/drop-overlay.spec.ts
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
Overall this is a well-reasoned, minimal, and well-tested fix; the two points above are edge-case hedges on the redundancy of the recovery paths, not blocking issues. |
🔍 GitHub Pages PR preview
|
- Listen for keydown and pointerdown in the capture phase rather than the bubble phase. Several controls stop propagation before these events reach window (startLayerPanelResize in this same file is one, and a focused Radix dialog handles its own Escape), which silently defeated the recovery for exactly the interaction a user is most likely to try first: clicking a panel control while the overlay is stuck. Covered by a new e2e test that presses the layer panel resize handle, which fails on the bubble phase. - Add an e2e test that a real GeoJSON drop still imports a layer and leaves the overlay hidden, so the recovery listeners cannot swallow the drop the overlay exists to invite. This was verified by hand before but not encoded.
Code reviewI reviewed the state-management fix in Bugs: None found. The recovery effect is correctly scoped ( Security: None found. No injection, unsafe input handling, or secrets involved; the change is purely client-side UI state recovery. Performance: None found. The listeners are only mounted while Quality: Low confidence — the claim that "neither [keydown nor pointerdown] can fire mid-drag" is solid for pointer events (OS/HTML5 drag holds the pointer grab) but slightly weaker for keydown: Escape reliably cancels drags in mainstream Chromium/Firefox/macOS/Windows, but if some platform/browser combination doesn't cancel the underlying drag on Escape, this would prematurely hide the overlay while a real drag continues. This is cosmetic only (drop handling doesn't depend on CLAUDE.md: No violations. Also verified: |
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 `@e2e/drop-overlay.spec.ts`:
- Around line 58-69: Replace the synthetic pointerdown dispatch in the
resize-handle test with a complete real pointer gesture, using the locator’s
built-in click action or a page.mouse down/up sequence. Ensure the gesture
supplies an active pointer ID for DesktopShell.startLayerPanelResize and reaches
pointerup so resize state is cleaned up while still verifying the overlay is
dismissed.
🪄 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: 021b57d6-116a-4920-9774-9abccdc4797d
📒 Files selected for processing (2)
apps/geolibre-desktop/src/components/layout/DesktopShell.tsxe2e/drop-overlay.spec.ts
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
Overall this is a well-reasoned, well-tested fix with good regression coverage; only one low-confidence note was raised for consideration. |
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
Overall this is a well-scoped, well-tested fix with clear reasoning and good e2e coverage of both the recovery paths and the non-regression cases (balanced drag, real drop). Only one minor documentation nit was raised. |
- e2e/drop-overlay.spec.ts: drive the resize-handle test with a real
`click()` instead of `dispatchEvent("pointerdown")`. A synthetic pointer
event carries no active pointer id, so `startLayerPanelResize`'s
`setPointerCapture` could throw before the capture-phase recovery was
exercised, and the gesture never reached `pointerup` to tear the resize
down. Verified the test still fails when the listeners are moved back to
the bubble phase, so it remains a real regression test.
- DesktopShell.tsx: scope the "cannot occur during a real drag" claim to
the pointer listener only. HTML5 drag-and-drop suppresses mouse events
and a native drag holds an OS pointer grab, but neither spec promises
keyboard events are withheld, and the inline comment on `onKeyDown`
already assumed a keypress can arrive mid-drag. Escape is now described
for what it is: a conventional request to cancel the stranded overlay or
the drag itself.
| // silently defeat the recovery for exactly the interaction the user is most | ||
| // likely to try first. Capturing on window runs before any of them. | ||
| window.addEventListener("keydown", onKeyDown, true); | ||
| window.addEventListener("pointerdown", clear, true); |
There was a problem hiding this comment.
Minor completeness note (low confidence): recovery only fires on Escape or a pointer press. A keyboard-only user navigating with Tab (no pointer, and not thinking to hit Escape) has no way to dismiss a stranded overlay. Given the reporter's workflow this is a big improvement either way, but it might be worth also clearing on focus-in/Tab if keyboard-only recovery matters here.
There was a problem hiding this comment.
Leaving this one as-is, and flagging it for a human call rather than resolving.
Two reasons:
-
A keyboard-only user is not actually blocked. The overlay is
pointer-events-nonewith no focusable children (DesktopShell.tsx:2616-2626), so it never traps focus or intercepts input — Tab navigation and every control keep working normally underneath it. The stranded overlay is a stale visual affordance, not a modal. And Escape, which does clear it, is the conventional dismiss key for exactly this kind of overlay. -
Clearing on focus-in/Tab would reintroduce the desync the previous review round just flagged. As noted in fix(shell): recover a drop overlay that outlived its drag #1683 (comment), keyboard and focus events are not spec-suppressed during a drag. Escape is safe despite that because pressing Escape mid-drag is itself the standard gesture to cancel the drag, so clearing the overlay is the correct outcome either way. Tab and focus changes have no such property: they do not cancel a drag, so clearing on them could hide the overlay while a genuine drag is still live.
Happy to add it if the accessibility angle is worth the tradeoff, but it looked like a net regression rather than a win.
Code reviewBugs: None found with meaningful confidence. The recovery logic correctly reasons about why Security: None — no new input handling, no untrusted data paths. Performance: None significant. The effect re-subscribes its two Quality: Low-confidence completeness note posted inline — recovery is keyed off CLAUDE.md: No violations — e2e spec correctly lives under The fix is well-reasoned, the new e2e tests ( |
Fixes #1664
The bug
After importing a style file onto a vector layer through the Layer panel's import dialog, the drag-and-drop overlay ("Drop vector or raster files here to add layers") sat over the map and never went away. Escape did nothing, resizing did nothing, and the only thing that dismissed it was dragging an unrelated file across the window.
Root cause
isDraggingFilesinDesktopShellis a single boolean fed from two independent places:dragDepthRefonDragDropEvent, which sets it with no counter at all, since the OS reports enter/leave/drop directlyEither feed can strand it. A native
leavethe OS never delivers leaves the flag set with nothing left to clear it, which is what a modal native file dialog opening mid-drag produces on WebKitGTK, and is how the reporter hit it through the style import dialog on Debian 12. An unbalanced webview enter/leave pair has the same effect by leavingdragDepthRefabove zero. That explains every symptom in the report, including why dragging another file across the window was the only workaround: it supplies the missing leave.The fix
Rather than chase every way the OS can swallow a drag event, recover on two signals that cannot occur while a real drag is in progress: a key press and a pointer button. HTML5 drags suppress mouse events for their duration, and native drags hold a pointer grab, so neither can fire mid-drag and dismiss the overlay out from under a drag the user is actually performing.
dragDepthRef, so the two feeds cannot disagree afterward.Verification
Driven in the real app with Playwright against
us_cities.geojson:Confirmed the overlay renders and clears correctly in both dark and light themes.
New e2e cover in
e2e/drop-overlay.spec.ts: the two recovery tests fail onmainand pass here; the balanced-drag guard passes either way. Also rannpm run buildandpre-commiton the changed files.Note on reproduction
The stranded-overlay defect and all four reported symptoms reproduce in a browser. The specific trigger the reporter described (the native GTK file chooser swallowing a drag
leave) is desktop-only and I could not drive it directly, since Playwright cannot attach to the Tauri window. The fix is deliberately trigger-agnostic for that reason: it guarantees the overlay can always be dismissed regardless of which side loses the event.Summary by CodeRabbit
Bug Fixes
Tests