fix(input): parse x10 mouse reports and re-assert host sgr - #2311
fix(input): parse x10 mouse reports and re-assert host sgr#2311zhang17-24 wants to merge 1 commit into
Conversation
When the host terminal drops to DEFAULT mouse encoding (xterm.js reports X10, ESC[M), herdr typed each report into the focused pane as text. Parse X10 reports as mouse events and, on detecting them, force the host back to SGR (?1006h) so the encoding drop cannot persist. refs herdrdev#1537
📝 WalkthroughWalkthroughThe raw input layer now parses X10 mouse reports, buffers incomplete sequences, and discards expired tails. The Unix client path detects X10 reports and focus gains, then reasserts SGR mouse capture with a 500 ms debounce. ChangesX10 mouse input and capture recovery
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant UnixInputPath
participant RawInputFramer
participant HostTerminal
UnixInputPath->>RawInputFramer: receive X10 report or focus-gain input
RawInputFramer-->>UnixInputPath: report event or focus-gain event
UnixInputPath->>HostTerminal: clear host mouse reporting
UnixInputPath->>HostTerminal: enable SGR mouse capture
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
kangal-bot
added
the
ai-review
Greptile SummaryThe PR adds Unix-side parsing and framing for legacy X10 mouse reports, then debounces host-terminal SGR mouse-mode reassertion when X10 or focus-gain input is detected.
Confidence Score: 4/5The mouse-capture state bypass should be fixed before merging because delayed or stray X10 input can re-enable capture after it was disabled. The X10 branch invokes the unconditional host SGR reassertion without checking mouse_capture_active, while the focus-gain branch performs that check and the reassertion does not update the cached state. Files Needing Attention: src/client/mod.rs
|
| Filename | Overview |
|---|---|
| src/raw_input.rs | Adds X10 report recognition, semantic parsing, incomplete-sequence framing, detection, and focused unit coverage. |
| src/client/input.rs | Applies the longer mouse-sequence idle timeout while an incomplete X10 report is buffered. |
| src/client/mod.rs | Adds debounced SGR reassertion, but the X10 trigger can re-enable host capture while the client’s capture state is disabled. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[Host stdin bytes] --> B[RawInputByteFramer]
B --> C{Complete X10 report?}
C -->|Yes| D[Parse MouseEvent]
C -->|Partial| E[Wait with mouse timeout]
D --> F[Client loop detects X10]
F --> G[Debounce]
G --> H[Clear host mouse modes]
H --> I[Enable SGR mouse capture]
D --> J[Forward raw bytes to server]
Reviews (1): Last reviewed commit: "fix(input): parse x10 mouse reports and ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if crate::raw_input::contains_x10_mouse_report(&data) | ||
| || (host_capture_active | ||
| && events | ||
| .iter() | ||
| .any(|event| matches!(event, RawInputEvent::OuterFocusGained))) |
There was a problem hiding this comment.
When an X10 report remains queued after mouse capture is disabled, this condition calls the unconditional SGR reassertion without checking state.mouse_capture_active, re-enabling host mouse reporting while the cached state remains false and causing subsequent mouse input to be captured unexpectedly.
Knowledge Base Used: Client attach and raw input pipeline
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cb162512-ab5a-46f0-aafc-27e2a9d2d13e
📒 Files selected for processing (3)
src/client/input.rssrc/client/mod.rssrc/raw_input.rs
| use std::sync::atomic::{AtomicBool, AtomicU64, Ordering}; | ||
| use std::sync::{Arc, Mutex, OnceLock}; | ||
| use std::time::Duration; | ||
| use std::time::{Duration, Instant}; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --hidden \
-g 'Cargo.toml' -g 'Justfile' -g 'justfile' -g '*.yml' -g '*.yaml' \
'(deny|forbid)\(warnings\)|-D[[:space:]]*warnings|--deny[[:space:]]+warnings|cargo (check|clippy|test).*--target' .Repository: herdrdev/herdr
Length of output: 345
Compile-gate the Unix-only reassertion code.
If RawInputEvent, the debounce constant, helpers at lines 570–592, and loop state at line 1428 are Unix-only, add #[cfg(unix)] to each. The Windows just recipe runs Clippy with -D warnings, so unused items fail validation.
Source: Coding guidelines
| let host_capture_active = state.mouse_capture_active; | ||
| if crate::raw_input::contains_x10_mouse_report(&data) | ||
| || (host_capture_active | ||
| && events | ||
| .iter() | ||
| .any(|event| matches!(event, RawInputEvent::OuterFocusGained))) | ||
| { | ||
| // X10 (host fell to DEFAULT encoding) or focus regained | ||
| // with capture active (terminal recreated): force SGR back | ||
| // on. Debounced. Focus-gain re-assert is gated on capture | ||
| // being active so it cannot enable capture the user turned off. | ||
| reassert_host_sgr_mouse_capture(&mut last_sgr_reassert) | ||
| .map_err(ClientError::ConnectionFailed)?; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Gate X10 recovery on active mouse capture.
contains_x10_mouse_report(&data) is outside the host_capture_active condition. A matching stdin chunk calls EnableMouseCapture even when state.mouse_capture_active is false. This overrides the user's disabled mouse-capture setting.
Proposed fix
- if crate::raw_input::contains_x10_mouse_report(&data)
- || (host_capture_active
- && events
- .iter()
- .any(|event| matches!(event, RawInputEvent::OuterFocusGained)))
+ if host_capture_active
+ && (crate::raw_input::contains_x10_mouse_report(&data)
+ || events
+ .iter()
+ .any(|event| matches!(event, RawInputEvent::OuterFocusGained)))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let host_capture_active = state.mouse_capture_active; | |
| if crate::raw_input::contains_x10_mouse_report(&data) | |
| || (host_capture_active | |
| && events | |
| .iter() | |
| .any(|event| matches!(event, RawInputEvent::OuterFocusGained))) | |
| { | |
| // X10 (host fell to DEFAULT encoding) or focus regained | |
| // with capture active (terminal recreated): force SGR back | |
| // on. Debounced. Focus-gain re-assert is gated on capture | |
| // being active so it cannot enable capture the user turned off. | |
| reassert_host_sgr_mouse_capture(&mut last_sgr_reassert) | |
| .map_err(ClientError::ConnectionFailed)?; | |
| let host_capture_active = state.mouse_capture_active; | |
| if host_capture_active | |
| && (crate::raw_input::contains_x10_mouse_report(&data) | |
| || events | |
| .iter() | |
| .any(|event| matches!(event, RawInputEvent::OuterFocusGained))) | |
| { | |
| // X10 (host fell to DEFAULT encoding) or focus regained | |
| // with capture active (terminal recreated): force SGR back | |
| // on. Debounced. Focus-gain re-assert is gated on capture | |
| // being active so it cannot enable capture the user turned off. | |
| reassert_host_sgr_mouse_capture(&mut last_sgr_reassert) | |
| .map_err(ClientError::ConnectionFailed)?; |
| if starts_with_incomplete_x10_mouse_sequence(&self.buffer) { | ||
| tracing::debug!( | ||
| len = self.buffer.len(), | ||
| "discarding incomplete X10 mouse tail after input timeout" | ||
| ); | ||
| self.buffer.clear(); | ||
| return chunks; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Discard the remaining X10 bytes after a timeout.
Line 350 clears the partial report but does not retain how many coordinate bytes remain. If ESC[M C times out and '$ arrives later, the framer forwards those coordinate bytes to the pane as text.
Track the remaining 6 - buffer.len() bytes in discard state. Consume exactly that tail before parsing subsequent input. Add a regression test that appends normal input after the delayed tail and verifies that only the normal input is forwarded.
|
superseeded by #2312 |
3 participants
Summary
ESC[M) mouse reports on Unix so they are treated as mouse input instead of being typed into the focused pane as garbage.?1006h), debounced, so the encoding drop cannot persist.Reproduction
ESC[M), e.g. VS Code integrated terminal after the mouse encoding drops to DEFAULT (SGR off:printf '\033[?1006l' > $(tty)from the host, or after a terminal reset).CN1CQ2CS3.... After: reports parse as mouse events.Test plan
Mouse(no garbage) and?1006his re-asserted automatically.refs #1537