Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/client/input.rs
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,9 @@ fn idle_flush_timeout_ms(
host_mouse_capture_active: bool,
) -> i32 {
if host_mouse_capture_active
&& (framer.has_pending_lone_escape() || framer.has_pending_incomplete_sgr_mouse_sequence())
&& (framer.has_pending_lone_escape()
|| framer.has_pending_incomplete_sgr_mouse_sequence()
|| framer.has_pending_incomplete_x10_mouse_sequence())
{
crate::raw_input::MOUSE_ACTIVE_ESCAPE_SEQUENCE_FLUSH_TIMEOUT_MS
} else {
Expand Down
54 changes: 53 additions & 1 deletion src/client/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ use std::collections::HashSet;
use std::io::{self, BufRead, Write as _};
use std::sync::atomic::{AtomicBool, AtomicU64, Ordering};
use std::sync::{Arc, Mutex, OnceLock};
use std::time::Duration;
use std::time::{Duration, Instant};

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 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


use base64::Engine;
use crossterm::event::{
Expand All @@ -44,6 +44,7 @@ use crate::protocol::{
ClientMessage, NotifyKind, RenderEncoding, ServerMessage, MAX_FRAME_SIZE,
MAX_GRAPHICS_FRAME_SIZE, PROTOCOL_VERSION,
};
use crate::raw_input::RawInputEvent;
use crate::server::socket_paths::client_socket_path;

static RECEIVED_KITTY_GRAPHICS_IDS: OnceLock<Mutex<HashSet<u32>>> = OnceLock::new();
Expand Down Expand Up @@ -566,6 +567,29 @@ fn set_mouse_capture(enabled: bool) -> io::Result<()> {
}
}

const HOST_SGR_REASSERT_DEBOUNCE: Duration = Duration::from_millis(500);

/// Whether a host SGR re-assert is allowed now. Returns true at most once per
/// debounce window; each true consumes the state, so callers must write the
/// capture sequence when it returns true.
fn should_reassert_host_sgr(last_assert: &mut Option<Instant>) -> bool {
if last_assert.is_some_and(|at| at.elapsed() < HOST_SGR_REASSERT_DEBOUNCE) {
return false;
}
*last_assert = Some(Instant::now());
true
}

/// Force the host terminal back to SGR mouse encoding, bypassing the
/// host_mouse_capture_active caches that would otherwise suppress a re-send.
fn reassert_host_sgr_mouse_capture(last_assert: &mut Option<Instant>) -> std::io::Result<()> {
if !should_reassert_host_sgr(last_assert) {
return Ok(());
}
crate::terminal_modes::clear_host_mouse_reporting(&mut io::stdout())?;
execute!(io::stdout(), EnableMouseCapture)
}

fn restore_terminal_state(
reset_modify_other_keys: bool,
reset_host_color_scheme_reports: bool,
Expand Down Expand Up @@ -1401,6 +1425,7 @@ async fn run_client_loop(
let mut prefix_input_source = crate::platform::RealPrefixInputSource::default();

// Main event loop.
let mut last_sgr_reassert: Option<Instant> = None;
while !should_quit.load(Ordering::Acquire) {
let event = tokio::select! {
ev = event_rx.recv() => ev.unwrap_or(ClientLoopEvent::Timer),
Expand Down Expand Up @@ -1446,6 +1471,20 @@ async fn run_client_loop(
}
} else {
let events = crate::raw_input::parse_raw_input_bytes_sync(&data);
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)))
Comment on lines +1475 to +1479

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 X10 bypasses disabled capture state

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

{
// 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)?;
Comment on lines +1474 to +1486

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.

Suggested change
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 crate::raw_input::events_require_host_surface_redraw(
&events,
state.redraw_on_focus_gained,
Expand Down Expand Up @@ -2305,6 +2344,19 @@ mod tests {
assert!(resize_report_required(false, (120, 40, 9, 18), size));
}

#[test]
fn x10_reassert_debounce_suppresses_frequent_triggers() {
let mut last = None;
assert!(
should_reassert_host_sgr(&mut last),
"first X10 trigger re-asserts"
);
assert!(
!should_reassert_host_sgr(&mut last),
"immediate repeat within debounce is suppressed"
);
}

fn restore_env_var(key: &str, value: Option<OsString>) {
if let Some(value) = value {
std::env::set_var(key, value);
Expand Down
158 changes: 157 additions & 1 deletion src/raw_input.rs
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,10 @@ impl RawInputFramer {
self.byte_framer.has_pending_incomplete_sgr_mouse_sequence()
}

pub(crate) fn has_pending_incomplete_x10_mouse_sequence(&self) -> bool {
self.byte_framer.has_pending_incomplete_x10_mouse_sequence()
}

#[cfg(any(windows, test))]
pub(crate) fn has_pending_bracketed_paste(&self) -> bool {
self.byte_framer.has_pending_bracketed_paste()
Expand Down Expand Up @@ -281,6 +285,10 @@ impl RawInputByteFramer {
starts_with_incomplete_sgr_mouse_sequence(&self.buffer)
}

pub(crate) fn has_pending_incomplete_x10_mouse_sequence(&self) -> bool {
starts_with_incomplete_x10_mouse_sequence(&self.buffer)
}

#[cfg(any(windows, test))]
pub(crate) fn has_pending_bracketed_paste(&self) -> bool {
self.buffer.starts_with(BRACKETED_PASTE_START)
Expand Down Expand Up @@ -334,6 +342,15 @@ impl RawInputByteFramer {
return chunks;
}

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;
Comment on lines +345 to +351

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.

}

if starts_with_incomplete_sgr_mouse_sequence(&self.buffer) {
tracing::debug!(
bytes = ?self.buffer,
Expand Down Expand Up @@ -586,7 +603,9 @@ pub(crate) fn events_require_host_terminal_theme_query(events: &[RawInputEvent])
}

fn input_flush_timeout_ms(framer: &RawInputFramer) -> i32 {
if framer.has_pending_incomplete_sgr_mouse_sequence() {
if framer.has_pending_incomplete_sgr_mouse_sequence()
|| framer.has_pending_incomplete_x10_mouse_sequence()
{
MOUSE_ACTIVE_ESCAPE_SEQUENCE_FLUSH_TIMEOUT_MS
} else {
RAW_INPUT_IDLE_FLUSH_TIMEOUT_MS
Expand Down Expand Up @@ -783,6 +802,15 @@ fn extract_one_event(buffer: &[u8]) -> Option<(RawInputEvent, usize)> {
}

if buffer[0] == ESC {
// X10 ("normal") mouse reports: ESC [ M cb col row. Must be consumed
// before complete_escape_sequence_len, which would split ESC[M at 3 bytes.
if buffer.starts_with(b"\x1b[M") {
if let Some(mouse) = parse_x10_mouse(buffer) {
return Some((RawInputEvent::Mouse(mouse), 6));
}
// Incomplete X10 report: hold for the remaining coordinate bytes.
return None;
}
let seq_len = complete_escape_sequence_len(buffer)?;
let seq = std::str::from_utf8(&buffer[..seq_len]).ok()?;

Expand Down Expand Up @@ -1020,6 +1048,10 @@ fn starts_with_incomplete_sgr_mouse_sequence(buffer: &[u8]) -> bool {
.all(|byte| byte.is_ascii_digit() || *byte == b';')
}

fn starts_with_incomplete_x10_mouse_sequence(buffer: &[u8]) -> bool {
buffer.starts_with(b"\x1b[M") && buffer.len() < 6
}

fn starts_with_incomplete_orphaned_sgr_mouse_tail(buffer: &[u8]) -> bool {
if buffer.len() > MAX_ORPHANED_SGR_MOUSE_TAIL_BYTES {
return false;
Expand Down Expand Up @@ -1197,6 +1229,35 @@ fn parse_sgr_mouse(sequence: &str) -> Option<MouseEvent> {
})
}

/// Parse an X10 ("normal") mouse report: ESC [ M <cb+32> <col+32> <row+32>.
/// Every wire value is the raw one plus 32 (ASCII offset) and coordinates are
/// 1-based; the parsed event uses 0-based coordinates like the SGR path.
/// Returns None when the buffer is not a complete X10 report.
fn parse_x10_mouse(buffer: &[u8]) -> Option<MouseEvent> {
if !buffer.starts_with(b"\x1b[M") || buffer.len() < 6 {
return None;
}
let cb = buffer[3].checked_sub(32)?;
let column = u16::from(buffer[4].saturating_sub(32)).saturating_sub(1);
let row = u16::from(buffer[5].saturating_sub(32)).saturating_sub(1);
let (kind, modifiers) = parse_mouse_cb(cb)?;
Some(MouseEvent {
kind,
column,
row,
modifiers,
})
}

/// True when `data` contains a complete X10 ("normal") mouse report
/// (ESC [ M cb col row). These only arrive when the host terminal is in
/// DEFAULT mouse encoding, so they are the client's signal to re-assert SGR.
pub(crate) fn contains_x10_mouse_report(data: &[u8]) -> bool {
data.windows(6).any(|window| {
window.starts_with(b"\x1b[M") && window[3] >= 32 && window[4] >= 32 && window[5] >= 32
})
}

fn parse_mouse_cb(cb: u8) -> Option<(MouseEventKind, KeyModifiers)> {
let button_number = (cb & 0b0000_0011) | ((cb & 0b1100_0000) >> 4);
let dragging = cb & 0b0010_0000 == 0b0010_0000;
Expand Down Expand Up @@ -1246,6 +1307,14 @@ mod tests {
assert_eq!(key.modifiers, modifiers);
}

#[test]
fn detects_x10_report_in_raw_input() {
assert!(contains_x10_mouse_report(b"\x1b[MC'$"));
assert!(contains_x10_mouse_report(b"xx\x1b[MC!!"));
assert!(!contains_x10_mouse_report(b"\x1b[<35;8;5M"));
assert!(!contains_x10_mouse_report(b"plain text"));
}

fn decode_hex(hex: &str) -> Vec<u8> {
let hex = hex.trim();
assert_eq!(hex.len() % 2, 0, "hex string must have even length");
Expand Down Expand Up @@ -1395,6 +1464,69 @@ mod tests {
}
}

#[test]
fn parses_x10_mouse_report() {
// ESC [ M, button 35+32=67('C'), column 7+32=39('\''), row 4+32=36('$').
// No spaces between bytes: the raw report is exactly \x1b[MC'$.
let bytes = b"\x1b[MC'$";
let events = parse_raw_input_bytes_sync(bytes);
match &events[0] {
RawInputEvent::Mouse(m) => {
assert_eq!(m.kind, MouseEventKind::Moved);
// X10 wire values are 1-based; parsed events are 0-based like SGR.
assert_eq!(m.column, 6);
assert_eq!(m.row, 3);
}
other => panic!("expected Mouse, got {other:?}"),
}
assert_eq!(
events.len(),
1,
"a complete X10 report must parse as exactly one event"
);
}

#[test]
fn x10_coordinates_are_zero_based() {
// col 1, row 1 -> bytes 33 ('!'), so parsed as 0-based (0, 0)
let bytes = b"\x1b[MC!!";
match &parse_raw_input_bytes_sync(bytes)[0] {
RawInputEvent::Mouse(m) => {
assert_eq!(m.column, 0);
assert_eq!(m.row, 0);
}
other => panic!("expected Mouse, got {other:?}"),
}
}

#[test]
fn x10_scroll_wheel_events() {
// ScrollUp = 64+32 = 96 ('`'), ScrollDown = 65+32 = 97 ('a')
let up = b"\x1b[M`!!";
match &parse_raw_input_bytes_sync(up)[0] {
RawInputEvent::Mouse(m) => assert_eq!(m.kind, MouseEventKind::ScrollUp),
other => panic!("expected ScrollUp, got {other:?}"),
}
let down = b"\x1b[Ma!!";
match &parse_raw_input_bytes_sync(down)[0] {
RawInputEvent::Mouse(m) => assert_eq!(m.kind, MouseEventKind::ScrollDown),
other => panic!("expected ScrollDown, got {other:?}"),
}
}

#[test]
fn x10_button_down_maps_to_down() {
// Left button down = 0 + 32 = 32, and byte 32 IS a literal space, so the
// space here is the button byte (not a separator).
let bytes = b"\x1b[M !!"; // \x1b[M + 32(' ') + 33('!') + 33('!')
match &parse_raw_input_bytes_sync(bytes)[0] {
RawInputEvent::Mouse(m) => {
assert_eq!(m.kind, MouseEventKind::Down(MouseButton::Left));
}
other => panic!("expected Down(Left), got {other:?}"),
}
}

#[test]
fn parses_host_default_color_response_with_st() {
let (RawInputEvent::HostDefaultColor { kind, color }, consumed) =
Expand Down Expand Up @@ -2011,6 +2143,30 @@ mod tests {
assert_eq!(framer.push(b"M"), vec![b"M".to_vec()]);
}

#[test]
fn x10_incomplete_report_waits_for_coordinate_bytes() {
let mut framer = RawInputByteFramer::for_host_input();
// \x1b[M + only the button byte: not yet a complete report
let chunks = framer.push(b"\x1b[M\x43");
assert!(chunks.is_empty(), "partial X10 must not emit a chunk");
assert!(framer.has_pending_incomplete_x10_mouse_sequence());
assert!(framer.has_pending_input());
// remaining two coordinate bytes complete the report
let chunks = framer.push(b"\x27\x24");
assert_eq!(chunks, vec![b"\x1b[M\x43\x27\x24".to_vec()]);
assert!(!framer.has_pending_input());
}

#[test]
fn x10_truncated_report_is_discarded_on_flush() {
let mut framer = RawInputByteFramer::for_host_input();
framer.push(b"\x1b[M\x43\x27");
assert!(framer.has_pending_incomplete_x10_mouse_sequence());
let chunks = framer.flush_timeout();
assert!(chunks.is_empty(), "truncated X10 must not leak as text");
assert!(!framer.has_pending_input());
}

#[test]
fn sgr_mouse_tail_after_lone_escape_timeout_is_discarded() {
let mut framer = RawInputFramer::default();
Expand Down
Loading