From 2f98696a9920ac371d3c3ed515ab0f03d16c7bea Mon Sep 17 00:00:00 2001 From: yishuiliunian Date: Mon, 6 Apr 2026 13:51:04 +0800 Subject: [PATCH 1/2] fix(tui): replace alternate scroll heuristic with MouseCapture for deterministic scroll detection (#88) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous approach used xterm alternate scroll mode (\x1b[?1007h) to convert mouse wheel events into Up/Down arrow key sequences, then relied on batch-counting heuristics (≥2 arrows per batch = scroll) to distinguish scroll from keyboard input. This was fundamentally unreliable across different terminal emulators and system load conditions. Switch to crossterm's EnableMouseCapture which provides actual MouseEvent::ScrollUp/ScrollDown events, making the distinction deterministic. Up/Down arrow keys now always route to history navigation. --- crates/loopal-tui/src/event.rs | 71 +++++++++++++------ crates/loopal-tui/src/terminal.rs | 40 +---------- crates/loopal-tui/src/tui_loop.rs | 33 ++------- crates/loopal-tui/tests/suite.rs | 4 +- .../loopal-tui/tests/suite/e2e_scroll_test.rs | 55 ++++++++++---- .../{scroll_burst_test.rs => scroll_test.rs} | 64 +++++++---------- 6 files changed, 127 insertions(+), 140 deletions(-) rename crates/loopal-tui/tests/suite/{scroll_burst_test.rs => scroll_test.rs} (64%) diff --git a/crates/loopal-tui/src/event.rs b/crates/loopal-tui/src/event.rs index 2b4b3ab3..48a56c36 100644 --- a/crates/loopal-tui/src/event.rs +++ b/crates/loopal-tui/src/event.rs @@ -1,4 +1,4 @@ -use crossterm::event::{self, Event as CrosstermEvent, KeyEvent}; +use crossterm::event::{self, Event as CrosstermEvent, KeyEvent, MouseEventKind}; use loopal_protocol::AgentEvent; use tokio::sync::mpsc; @@ -9,6 +9,10 @@ use crate::input::paste::PasteResult; pub enum AppEvent { /// Keyboard / terminal event Key(KeyEvent), + /// Mouse scroll up (wheel toward user → content scrolls toward top) + ScrollUp, + /// Mouse scroll down (wheel away from user → content scrolls toward bottom) + ScrollDown, /// Resize event Resize(u16, u16), /// Agent event from the runtime @@ -38,37 +42,58 @@ impl EventHandler { // blocks the agent-side `event_tx.send().await` — deadlock. let (tx, rx) = mpsc::channel(4096); - // Spawn crossterm event polling task + // Spawn crossterm event polling task. + // + // Reads ALL buffered events in a single `spawn_blocking` call so + // that rapid events (e.g. paste sequences) land in the channel + // together, improving batch processing in `tui_loop.rs`. let term_tx = tx.clone(); tokio::spawn(async move { loop { - // Poll crossterm with a 50ms timeout to yield periodically - match tokio::task::spawn_blocking(|| { - if event::poll(std::time::Duration::from_millis(50)).unwrap_or(false) { - event::read().ok() - } else { - None + let result = tokio::task::spawn_blocking(|| { + // Wait up to 50ms for the first event. + if !event::poll(std::time::Duration::from_millis(50)).unwrap_or(false) { + return Vec::new(); } - }) - .await - { - Ok(Some(CrosstermEvent::Key(key))) => { - if term_tx.send(AppEvent::Key(key)).await.is_err() { - break; - } + let mut events = Vec::new(); + if let Ok(ev) = event::read() { + events.push(ev); } - Ok(Some(CrosstermEvent::Resize(w, h))) => { - if term_tx.send(AppEvent::Resize(w, h)).await.is_err() { - break; + // Drain any additional buffered events without waiting. + while event::poll(std::time::Duration::ZERO).unwrap_or(false) { + match event::read() { + Ok(ev) => events.push(ev), + Err(_) => break, } } - Ok(Some(CrosstermEvent::Paste(text))) => { - let result = PasteResult::Text(text); - if term_tx.send(AppEvent::Paste(result)).await.is_err() { - break; + events + }) + .await; + + match result { + Ok(events) if events.is_empty() => continue, + Ok(events) => { + for ev in events { + let app_event = match ev { + CrosstermEvent::Key(key) => Some(AppEvent::Key(key)), + CrosstermEvent::Mouse(mouse) => match mouse.kind { + MouseEventKind::ScrollUp => Some(AppEvent::ScrollUp), + MouseEventKind::ScrollDown => Some(AppEvent::ScrollDown), + _ => None, + }, + CrosstermEvent::Resize(w, h) => Some(AppEvent::Resize(w, h)), + CrosstermEvent::Paste(text) => { + Some(AppEvent::Paste(PasteResult::Text(text))) + } + _ => None, + }; + if let Some(app_event) = app_event + && term_tx.send(app_event).await.is_err() + { + return; + } } } - Ok(_) => {} Err(_) => break, } } diff --git a/crates/loopal-tui/src/terminal.rs b/crates/loopal-tui/src/terminal.rs index 05a720b6..dc5333f3 100644 --- a/crates/loopal-tui/src/terminal.rs +++ b/crates/loopal-tui/src/terminal.rs @@ -1,45 +1,11 @@ -use std::fmt; use std::io; use crossterm::{ - Command, - event::{DisableBracketedPaste, EnableBracketedPaste}, + event::{DisableBracketedPaste, DisableMouseCapture, EnableBracketedPaste, EnableMouseCapture}, execute, terminal::{EnterAlternateScreen, LeaveAlternateScreen, disable_raw_mode, enable_raw_mode}, }; -/// Enable xterm alternate scroll mode (`\x1b[?1007h`). -/// -/// In alternate screen the terminal translates mouse wheel events into -/// Up/Down arrow key sequences. This preserves terminal-native text -/// selection (click + drag) while providing scroll wheel support. -struct EnableAlternateScroll; - -impl Command for EnableAlternateScroll { - fn write_ansi(&self, f: &mut impl fmt::Write) -> fmt::Result { - write!(f, "\x1b[?1007h") - } - - #[cfg(windows)] - fn execute_winapi(&self) -> io::Result<()> { - Ok(()) - } -} - -/// Disable xterm alternate scroll mode (`\x1b[?1007l`). -struct DisableAlternateScroll; - -impl Command for DisableAlternateScroll { - fn write_ansi(&self, f: &mut impl fmt::Write) -> fmt::Result { - write!(f, "\x1b[?1007l") - } - - #[cfg(windows)] - fn execute_winapi(&self) -> io::Result<()> { - Ok(()) - } -} - /// RAII guard that ensures raw mode and alternate screen are cleaned up on drop, /// even if the TUI panics or returns early via `?`. pub struct TerminalGuard; @@ -51,7 +17,7 @@ impl TerminalGuard { execute!( stdout, EnterAlternateScreen, - EnableAlternateScroll, + EnableMouseCapture, EnableBracketedPaste )?; Ok(Self) @@ -64,7 +30,7 @@ impl Drop for TerminalGuard { let _ = execute!( io::stdout(), DisableBracketedPaste, - DisableAlternateScroll, + DisableMouseCapture, LeaveAlternateScreen ); } diff --git a/crates/loopal-tui/src/tui_loop.rs b/crates/loopal-tui/src/tui_loop.rs index 769d8135..5ce267e3 100644 --- a/crates/loopal-tui/src/tui_loop.rs +++ b/crates/loopal-tui/src/tui_loop.rs @@ -4,7 +4,6 @@ use std::io; use std::path::PathBuf; use std::sync::Arc; -use crossterm::event::KeyCode; use ratatui::prelude::*; use loopal_protocol::{AgentEvent, AgentEventPayload}; @@ -61,36 +60,14 @@ where batch.push(event); } - // Pre-scan: count Up/Down arrow key events in this batch. - // Mouse wheel (via \x1b[?1007h]) generates ≥2 arrows per tick; - // a single keyboard press generates exactly 1. - let arrow_count = batch - .iter() - .filter(|e| { - matches!( - e, - AppEvent::Key(k) if matches!(k.code, KeyCode::Up | KeyCode::Down) - ) - }) - .count(); - let is_scroll_burst = arrow_count >= 2; - let mut should_quit = false; for event in batch { match event { - AppEvent::Key(key) - if is_scroll_burst && matches!(key.code, KeyCode::Up | KeyCode::Down) => - { - // Mouse-wheel burst → scroll content, bypass key handler. - match key.code { - KeyCode::Up => { - app.content_scroll.scroll_up(3); - } - KeyCode::Down => { - app.content_scroll.scroll_down(3); - } - _ => unreachable!(), - } + AppEvent::ScrollUp => { + app.content_scroll.scroll_up(3); + } + AppEvent::ScrollDown => { + app.content_scroll.scroll_down(3); } AppEvent::Key(key) => { should_quit = handle_key_action(app, key, &events).await; diff --git a/crates/loopal-tui/tests/suite.rs b/crates/loopal-tui/tests/suite.rs index 2e8d4682..288505b2 100644 --- a/crates/loopal-tui/tests/suite.rs +++ b/crates/loopal-tui/tests/suite.rs @@ -60,8 +60,8 @@ mod message_lines_test; mod panel_tab_test; #[path = "suite/render_guard_test.rs"] mod render_guard_test; -#[path = "suite/scroll_burst_test.rs"] -mod scroll_burst_test; +#[path = "suite/scroll_test.rs"] +mod scroll_test; #[path = "suite/scroll_compensation_test.rs"] mod scroll_compensation_test; #[path = "suite/skill_render_test.rs"] diff --git a/crates/loopal-tui/tests/suite/e2e_scroll_test.rs b/crates/loopal-tui/tests/suite/e2e_scroll_test.rs index 65cbc102..44fb8f5b 100644 --- a/crates/loopal-tui/tests/suite/e2e_scroll_test.rs +++ b/crates/loopal-tui/tests/suite/e2e_scroll_test.rs @@ -1,7 +1,10 @@ /// E2E integration tests for scroll behavior through the full TUI event loop. /// +/// Mouse scroll uses `AppEvent::ScrollUp/ScrollDown` (deterministic, no heuristic). +/// Keyboard Up/Down always navigates history via `handle_key_action`. +/// /// Regression tests for: -/// - Bug 1: Mouse wheel (≥2 arrow events in batch) scrolls content, not history +/// - Bug 1: Mouse scroll events scroll content, not history /// - Bug 2: Single keyboard arrow navigates history, not scroll /// - Bug 3: Scroll offset is compensated when content grows during streaming use std::sync::Arc; @@ -65,19 +68,18 @@ fn stream_event(text: &str) -> AppEvent { })) } -// --- Bug 1 regression: burst arrows → scroll, not history --- +// --- Bug 1 regression: scroll events → scroll content, not history --- #[tokio::test] -async fn test_burst_arrows_scroll_not_history() { +async fn test_scroll_events_scroll_not_history() { let (mut terminal, mut app, events, tx) = build_scroll_rig(); tokio::spawn(async move { tokio::time::sleep(Duration::from_millis(30)).await; - // Send 3 Up events in rapid succession (mouse wheel burst). - // They should all land in the same batch via try_next(). - let _ = tx.send(AppEvent::Key(up_key())).await; - let _ = tx.send(AppEvent::Key(up_key())).await; - let _ = tx.send(AppEvent::Key(up_key())).await; + // Mouse wheel fires ScrollUp events (deterministic, no batch detection). + let _ = tx.send(AppEvent::ScrollUp).await; + let _ = tx.send(AppEvent::ScrollUp).await; + let _ = tx.send(AppEvent::ScrollUp).await; tokio::time::sleep(Duration::from_millis(30)).await; let _ = tx.send(AppEvent::Key(ctrl_d())).await; }); @@ -90,17 +92,46 @@ async fn test_burst_arrows_scroll_not_history() { assert!( app.content_scroll.offset > 0, - "burst should scroll content, got offset={}", + "ScrollUp should scroll content, got offset={}", app.content_scroll.offset ); assert!( app.input.is_empty(), - "burst should NOT navigate history, got input={:?}", + "ScrollUp should NOT navigate history, got input={:?}", app.input ); } -// --- Bug 1 regression: single arrow → history, not scroll --- +#[tokio::test] +async fn test_scroll_down_decreases_offset() { + let (mut terminal, mut app, events, tx) = build_scroll_rig(); + + tokio::spawn(async move { + tokio::time::sleep(Duration::from_millis(30)).await; + // Scroll up first to create an offset + let _ = tx.send(AppEvent::ScrollUp).await; + let _ = tx.send(AppEvent::ScrollUp).await; + let _ = tx.send(AppEvent::ScrollUp).await; + let _ = tx.send(AppEvent::ScrollUp).await; + // Then scroll down + let _ = tx.send(AppEvent::ScrollDown).await; + tokio::time::sleep(Duration::from_millis(30)).await; + let _ = tx.send(AppEvent::Key(ctrl_d())).await; + }); + + let _ = tokio::time::timeout( + Duration::from_secs(3), + run_tui_loop(&mut terminal, events, &mut app), + ) + .await; + + assert_eq!( + app.content_scroll.offset, 9, + "4 ScrollUp (12) - 1 ScrollDown (3) = 9" + ); +} + +// --- Bug 2 regression: single arrow → history, not scroll --- #[tokio::test] async fn test_single_arrow_navigates_history() { @@ -130,7 +161,7 @@ async fn test_single_arrow_navigates_history() { ); } -// --- Bug 2 regression: scroll offset compensated during streaming --- +// --- Bug 3 regression: scroll offset compensated during streaming --- #[tokio::test] async fn test_scroll_offset_stable_during_streaming() { diff --git a/crates/loopal-tui/tests/suite/scroll_burst_test.rs b/crates/loopal-tui/tests/suite/scroll_test.rs similarity index 64% rename from crates/loopal-tui/tests/suite/scroll_burst_test.rs rename to crates/loopal-tui/tests/suite/scroll_test.rs index 21c78634..e165a7b0 100644 --- a/crates/loopal-tui/tests/suite/scroll_burst_test.rs +++ b/crates/loopal-tui/tests/suite/scroll_test.rs @@ -1,9 +1,8 @@ -/// Tests for batch-based scroll detection: mouse-wheel bursts vs keyboard arrows. +/// Tests for scroll behavior: mouse scroll events vs keyboard arrow keys. /// -/// Mouse wheel (via `\x1b[?1007h`) produces ≥2 arrow events per batch → scroll. -/// A single keyboard arrow press produces 1 event per batch → history navigation. -/// The batch detection logic lives in `tui_loop.rs`; these tests verify the -/// observable state after batch processing via direct App manipulation. +/// Mouse scroll uses dedicated `AppEvent::ScrollUp/Down` (via `EnableMouseCapture`). +/// Keyboard Up/Down always routes to history navigation through `handle_key`. +/// The two paths are completely independent — no heuristic detection needed. use crossterm::event::{KeyCode, KeyEvent, KeyModifiers}; use loopal_protocol::ControlCommand; @@ -33,69 +32,57 @@ fn key(code: KeyCode) -> KeyEvent { KeyEvent::new(code, KeyModifiers::NONE) } -/// Simulate mouse-wheel scroll burst: multiple arrow events applied as scroll. -fn apply_scroll_burst(app: &mut App, code: KeyCode, count: usize) { - for _ in 0..count { - match code { - KeyCode::Up => app.content_scroll.offset = app.content_scroll.offset.saturating_add(3), - KeyCode::Down => { - app.content_scroll.offset = app.content_scroll.offset.saturating_sub(3) - } - _ => {} - } - } -} - -// --- Mouse wheel burst detection (≥2 arrows → scroll) --- +// --- Mouse scroll (ContentScroll direct manipulation) --- #[test] -fn test_scroll_burst_up_increases_offset() { +fn test_scroll_up_increases_offset() { let mut app = make_app(); app.input_history.push("should not appear".into()); - // Simulate a burst of 3 Up events (mouse wheel) - apply_scroll_burst(&mut app, KeyCode::Up, 3); - assert_eq!( - app.content_scroll.offset, 9, - "3 Up scroll events should add 9" - ); + // Mouse scroll events directly modify content_scroll, never touching history. + app.content_scroll.scroll_up(3); + app.content_scroll.scroll_up(3); + app.content_scroll.scroll_up(3); + assert_eq!(app.content_scroll.offset, 9, "3 scroll_up(3) should add 9"); assert!(app.input.is_empty(), "scroll should NOT navigate history"); } #[test] -fn test_scroll_burst_down_decreases_offset() { +fn test_scroll_down_decreases_offset() { let mut app = make_app(); app.content_scroll.offset = 30; - apply_scroll_burst(&mut app, KeyCode::Down, 2); + app.content_scroll.scroll_down(3); + app.content_scroll.scroll_down(3); assert_eq!( app.content_scroll.offset, 24, - "2 Down scroll events should reduce by 6" + "2 scroll_down(3) should reduce by 6" ); } #[test] -fn test_scroll_burst_down_clamps_at_zero() { +fn test_scroll_down_clamps_at_zero() { let mut app = make_app(); app.content_scroll.offset = 2; - apply_scroll_burst(&mut app, KeyCode::Down, 3); + app.content_scroll.scroll_down(3); + app.content_scroll.scroll_down(3); + app.content_scroll.scroll_down(3); assert_eq!( app.content_scroll.offset, 0, "scroll_offset should not go negative" ); } -// --- Single arrow → keyboard → history --- +// --- Keyboard Up/Down → history --- #[test] -fn test_single_up_navigates_history() { +fn test_up_navigates_history() { let mut app = make_app(); app.input_history.push("previous".into()); - // Single Up event → keyboard → history navigation handle_key(&mut app, key(KeyCode::Up)); - assert_eq!(app.input, "previous", "single Up should browse history"); + assert_eq!(app.input, "previous", "Up should browse history"); } #[test] -fn test_single_down_navigates_history_forward() { +fn test_down_navigates_history_forward() { let mut app = make_app(); app.input_history.push("first".into()); app.input_history.push("second".into()); @@ -109,10 +96,11 @@ fn test_single_down_navigates_history_forward() { // --- Scroll does not affect history --- #[test] -fn test_scroll_burst_does_not_touch_history() { +fn test_scroll_does_not_touch_history() { let mut app = make_app(); app.input_history.push("preserved".into()); - apply_scroll_burst(&mut app, KeyCode::Up, 5); + app.content_scroll.scroll_up(3); + app.content_scroll.scroll_up(3); assert!(app.content_scroll.offset > 0); assert!(app.input.is_empty(), "scroll must not touch input"); assert!( From 8e04d0eb78d5865ce6a0469fac31cbb50ef34580 Mon Sep 17 00:00:00 2001 From: yishuiliunian Date: Mon, 6 Apr 2026 13:59:16 +0800 Subject: [PATCH 2/2] fix: rustfmt module ordering in suite.rs --- crates/loopal-tui/tests/suite.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/loopal-tui/tests/suite.rs b/crates/loopal-tui/tests/suite.rs index 288505b2..07bc63e3 100644 --- a/crates/loopal-tui/tests/suite.rs +++ b/crates/loopal-tui/tests/suite.rs @@ -60,10 +60,10 @@ mod message_lines_test; mod panel_tab_test; #[path = "suite/render_guard_test.rs"] mod render_guard_test; -#[path = "suite/scroll_test.rs"] -mod scroll_test; #[path = "suite/scroll_compensation_test.rs"] mod scroll_compensation_test; +#[path = "suite/scroll_test.rs"] +mod scroll_test; #[path = "suite/skill_render_test.rs"] mod skill_render_test; #[path = "suite/styled_wrap_test.rs"]