fix(terminal): clear stuck xterm selection drag#917
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a new selection guard module, wires it into renderer pool event handling, and adds Vitest coverage for the guard helpers and slot-clearing behavior. ChangesStuck Terminal Selection Guard
Estimated code review effort: 2 (Simple) | ~12 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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
♻️ Duplicate comments (1)
src/modules/terminal/lib/rendererPool.ts (1)
100-141: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTrack the initiating slot, not just a boolean, before clearing.
terminalSelectionMouseDownonly records that a drag started in some.xterm, not which pooled slot. When the stuck-drag clear fires (mousemove-release, blur, or hidden),clearLiveTerminalSelections(slots)at line 107 clears every slot with a non-nullcurrentLeafId— up to 5 bound terminals — including ones with no relation to the drag that got stuck. It's scoped correctly to the buggy edge case (normal mouseup already resets the flag before blur/hide can fire), but the actual clear is broader than the bug: rare, but it can silently wipe a user's unrelated selection sitting in a different bound tab.If the mousedown target can be resolved back to a specific slot/leaf id (e.g. via a
data-leaf-idon the.xtermcontainer), consider tracking that id instead of a boolean and filteringclearLiveTerminalSelectionsto just that slot.🤖 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 `@src/modules/terminal/lib/rendererPool.ts` around lines 100 - 141, The stuck-selection guard in bindStuckSelectionGuard is clearing all pooled terminal slots because it only tracks a boolean drag state, not the specific slot that started the selection. Update the mousedown path to resolve the originating slot or leaf id from the event target (for example via the terminal container), store that identifier instead of just terminalSelectionMouseDown, and then have clearTrackedSelection filter clearLiveTerminalSelections so only the matching slot is reset on mousemove-release, blur, or visibilitychange.
🧹 Nitpick comments (1)
src/modules/terminal/lib/stuckSelectionGuard.test.ts (1)
8-13: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winLock the
closestreceiver contract.This mock only checks the selector, so it would still pass if
isTerminalSelectionStartstopped binding the original target before callingclosest. Add a receiver assertion or use a real element spy so the.call(target, ".xterm")contract stays covered.🤖 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 `@src/modules/terminal/lib/stuckSelectionGuard.test.ts` around lines 8 - 13, The current `target()` mock in `stuckSelectionGuard.test.ts` only validates the selector and does not verify the `closest` receiver, so it can miss regressions in `isTerminalSelectionStart`. Update the test around `target()` and the `closest` spy to assert the method is invoked with the original target as `this` (for example via a receiver assertion or a real element spy), so the `.call(target, ".xterm")` binding contract remains covered.
🤖 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 `@src/modules/terminal/lib/stuckSelectionGuard.ts`:
- Around line 28-35: The current stuck-selection cleanup is too broad because
clearLiveTerminalSelections clears every slot with a non-null currentLeafId
instead of only the terminal that started the drag. Update
bindStuckSelectionGuard in rendererPool.ts to track the originating leaf/slot
for the active drag, then pass that specific target into
clearLiveTerminalSelections so only that terminal’s selection is cleared. Keep
the fix localized around clearLiveTerminalSelections and
bindStuckSelectionGuard, and avoid iterating over all bound slots for cleanup.
---
Duplicate comments:
In `@src/modules/terminal/lib/rendererPool.ts`:
- Around line 100-141: The stuck-selection guard in bindStuckSelectionGuard is
clearing all pooled terminal slots because it only tracks a boolean drag state,
not the specific slot that started the selection. Update the mousedown path to
resolve the originating slot or leaf id from the event target (for example via
the terminal container), store that identifier instead of just
terminalSelectionMouseDown, and then have clearTrackedSelection filter
clearLiveTerminalSelections so only the matching slot is reset on
mousemove-release, blur, or visibilitychange.
---
Nitpick comments:
In `@src/modules/terminal/lib/stuckSelectionGuard.test.ts`:
- Around line 8-13: The current `target()` mock in `stuckSelectionGuard.test.ts`
only validates the selector and does not verify the `closest` receiver, so it
can miss regressions in `isTerminalSelectionStart`. Update the test around
`target()` and the `closest` spy to assert the method is invoked with the
original target as `this` (for example via a receiver assertion or a real
element spy), so the `.call(target, ".xterm")` binding contract remains covered.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b327ef4d-1367-4ef1-8432-d6a7d1525bdd
📒 Files selected for processing (3)
src/modules/terminal/lib/rendererPool.tssrc/modules/terminal/lib/stuckSelectionGuard.test.tssrc/modules/terminal/lib/stuckSelectionGuard.ts
1 participant
What
Adds a small guard around the terminal renderer pool to clear xterm selections when a terminal selection drag loses its matching mouseup event.
Why
If xterm receives a selection mousedown and later misses the corresponding mouseup, subsequent mouse movement can keep extending the terminal selection even when the primary button is no longer pressed. In the app this appears as large text ranges becoming selected while simply moving the mouse.
How
.xterm.mousemove.buttons === 0, clear selections on live terminal slots.Testing
pnpm exec vitest run src/modules/terminal/lib/stuckSelectionGuard.test.tspnpm exec tsc --noEmitcleanpnpm testgit diff --checkpnpm lintexits 0; existing Biome warnings remain outside this diffScreenshots / GIFs
Not applicable; no visible UI change.
Notes reviewer
This intentionally keeps the fix scoped to xterm selection state. It does not change composer behavior, markdown rendering, or terminal input handling.
Summary by CodeRabbit
Bug Fixes
Tests