fix(record-tour): make the Record Map Tour close button work (#876)#878
Conversation
The close (X) button lives inside the dialog's drag-handle title bar, whose onPointerDown starts a drag and calls setPointerCapture. Pressing the X therefore captured the pointer on the title bar, redirecting the ensuing click away from the button so its onClick never fired and the panel could not be closed. Bail out of the drag when the pointer goes down on an interactive control (the close button), so no capture happens and the click reaches the button. Dragging from the grip/title still works. Fixes #876
✅ Deploy Preview for geolibre-app ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
⚡ Cloudflare Pages preview
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // The close button lives inside this drag handle. Starting a drag here | ||
| // captures the pointer, which would redirect the ensuing click away from | ||
| // the button and swallow it, so never begin a drag from an interactive | ||
| // control. |
There was a problem hiding this comment.
The first sentence describes DOM structure (the "what"), which per CLAUDE.md should be left out of comments. The pointer-capture explanation is the valuable "why". Consider trimming to one tight line:
| // The close button lives inside this drag handle. Starting a drag here | |
| // captures the pointer, which would redirect the ensuing click away from | |
| // the button and swallow it, so never begin a drag from an interactive | |
| // control. | |
| // setPointerCapture would redirect the subsequent click away from the button's onClick; bail out so the button works normally. |
There was a problem hiding this comment.
Done in 3186ad9 — trimmed the comment to lead with the pointer-capture 'why' and dropped the DOM-structure sentence.
| // captures the pointer, which would redirect the ensuing click away from | ||
| // the button and swallow it, so never begin a drag from an interactive | ||
| // control. | ||
| if ((event.target as HTMLElement).closest("button")) return; |
There was a problem hiding this comment.
Two minor nits to group:
-
TypeScript cast accuracy (low confidence):
event.targeton a pointer event that originates from the SVG<X>icon is anSVGElement, not anHTMLElement. Both have.closest()(inherited fromElement), so it works, butElementis the more accurate common ancestor type:(event.target as Element). -
Selector breadth (low confidence, forward-looking): The guard only matches
<button>elements by tag. If a link or[role="button"]were ever added to the drag handle the same pointer-capture bug would recur. A slightly broader selector is more defensive:
| if ((event.target as HTMLElement).closest("button")) return; | |
| if ((event.target as Element).closest("button, a, [role='button']")) return; |
There was a problem hiding this comment.
Both applied in 3186ad9: cast to Element (the pointerdown can originate on the SVG icon, not an HTMLElement) and broadened the selector to button, a, [role='button'] so the guard stays robust if another control is ever added to the drag handle.
|
Warning Review limit reached
More reviews will be available in 8 minutes and 55 seconds. Learn how PR review limits work. To continue reviewing without waiting, enable usage-based billing in the billing tab. ⌛ How to resolve this issue?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 credits. 🚦 How do rate 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 see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRecordTourDialog now ignores pointer-down events that begin on nested buttons inside the drag handle, allowing those buttons to receive clicks without starting a drag. ChangesRecordTourDialog drag handling
🎯 1 (Trivial) | ⏱️ ~2 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Code reviewThis is a small, targeted bug fix (5 lines). The root cause diagnosis in the PR description is correct — Bugs
Security
Performance
Quality
CLAUDE.md
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Cast event.target to Element (the pointerdown can originate on the SVG icon inside the button, not an HTMLElement) and broaden the guard to also skip links and role=button controls, so the same pointer-capture issue can't recur if another interactive control is added to the drag handle.
| // click away from the close button and swallow it, so never begin a drag | ||
| // from an interactive control. (event.target may be the SVG icon inside the | ||
| // button, so cast to the Element ancestor type rather than HTMLElement.) | ||
| if ((event.target as Element).closest("button, a, [role='button']")) return; |
There was a problem hiding this comment.
The selector covers the current drag-handle content (only the close <button> and its <X> SVG child are in there), so the fix is correct as written.
Minor forward-compatibility nit: if someone later adds an <input>, <select>, or <textarea> to the title bar, those wouldn't be caught and the drag would capture the pointer mid-edit. Broadening the guard costs nothing:
| if ((event.target as Element).closest("button, a, [role='button']")) return; | |
| if ((event.target as Element).closest("button, a, input, select, textarea, [role='button']")) return; |
Not a bug today — just worth knowing if the drag handle ever grows.
Code reviewReviewed the single changed file ( Bugs
Security
Performance
Quality
CLAUDE.md
Overall: The fix is correct and minimal. One low-confidence selector-completeness note posted as an inline suggestion; everything else looks good. |
1 participant
Summary
The close (X) button in the Record Map Tour panel did nothing, so users could not exit the recording window (#876).
Root cause
The close button lives inside the panel's drag-handle title bar. That title bar has an
onPointerDownhandler that begins a drag and callssetPointerCapture(pointerId). Pressing the X bubbled thepointerdownup to the title bar, which captured the pointer; with the pointer captured by the title bar, the subsequentclickwas dispatched to the title bardivinstead of the button, so the button'sonClicknever fired and the panel stayed open.Fix
Bail out of
onDragStartwhen the pointer goes down on an interactive control (the close button), so no pointer capture happens and the click reaches the button normally. Dragging the panel by the grip/title still works.Verification
Reproduced and verified in the real app with Playwright:
.click()).pre-commit run --files ...passes (eslint + full npm build).Fixes #876
Summary by CodeRabbit