Skip to content
Merged
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
201 changes: 189 additions & 12 deletions codex-rs/tui/src/bottom_pane/chat_composer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -683,6 +683,42 @@ impl ChatComposer {
if self.paste_burst.try_append_char_if_active(ch, now) {
return (InputResult::None, true);
}
// Non-ASCII input often comes from IMEs and can arrive in quick bursts.
// We do not want to hold the first char (flicker suppression) on this path, but we
// still want to detect paste-like bursts. Before applying any non-ASCII input, flush
// any existing burst buffer (including a pending first char from the ASCII path) so
// we don't carry that transient state forward.
if let Some(pasted) = self.paste_burst.flush_before_modified_input() {
self.handle_paste(pasted);
}
if let Some(decision) = self.paste_burst.on_plain_char_no_hold(now) {
match decision {
CharDecision::BufferAppend => {
self.paste_burst.append_char_to_buffer(ch, now);
return (InputResult::None, true);
}
CharDecision::BeginBuffer { retro_chars } => {
let cur = self.textarea.cursor();
let txt = self.textarea.text();
let safe_cur = Self::clamp_to_char_boundary(txt, cur);
let before = &txt[..safe_cur];
// If decision is to buffer, seed the paste burst buffer with the grabbed chars + new.
// Otherwise, fall through to normal insertion below.
if let Some(grab) =
self.paste_burst
.decide_begin_buffer(now, before, retro_chars as usize)
{
if !grab.grabbed.is_empty() {
self.textarea.replace_range(grab.start_byte..safe_cur, "");
}
// seed the paste burst buffer with everything (grabbed + new)
self.paste_burst.append_char_to_buffer(ch, now);
return (InputResult::None, true);
}
}
_ => unreachable!("on_plain_char_no_hold returned unexpected variant"),
}
}
}
if let Some(pasted) = self.paste_burst.flush_before_modified_input() {
self.handle_paste(pasted);
Expand Down Expand Up @@ -1346,9 +1382,8 @@ impl ChatComposer {
{
let has_ctrl_or_alt = has_ctrl_or_alt(modifiers);
if !has_ctrl_or_alt {
// Non-ASCII characters (e.g., from IMEs) can arrive in quick bursts and be
// misclassified by paste heuristics. Flush any active burst buffer and insert
// non-ASCII characters directly.
// Non-ASCII characters (e.g., from IMEs) can arrive in quick bursts, so avoid
// holding the first char while still allowing burst detection for paste input.
if !ch.is_ascii() {
return self.handle_non_ascii_char(input);
}
Expand All @@ -1370,7 +1405,6 @@ impl ChatComposer {
if !grab.grabbed.is_empty() {
self.textarea.replace_range(grab.start_byte..safe_cur, "");
}
self.paste_burst.begin_with_retro_grabbed(grab.grabbed, now);
self.paste_burst.append_char_to_buffer(ch, now);
return (InputResult::None, true);
}
Expand Down Expand Up @@ -2271,8 +2305,7 @@ mod tests {
composer.handle_key_event(KeyEvent::new(KeyCode::Char('?'), KeyModifiers::NONE));
assert_eq!(result, InputResult::None);
assert!(needs_redraw, "typing should still mark the view dirty");
std::thread::sleep(ChatComposer::recommended_paste_flush_delay());
let _ = composer.flush_paste_burst_if_due();
let _ = flush_after_paste_burst(&mut composer);
assert_eq!(composer.textarea.text(), "h?");
assert_eq!(composer.footer_mode, FooterMode::ShortcutSummary);
assert_eq!(composer.footer_mode(), FooterMode::ContextOnly);
Expand All @@ -2294,14 +2327,18 @@ mod tests {
false,
);

// Force an active paste burst so this test doesn't depend on tight timing.
composer
.paste_burst
.begin_with_retro_grabbed(String::new(), Instant::now());

for ch in ['h', 'i', '?', 't', 'h', 'e', 'r', 'e'] {
let _ = composer.handle_key_event(KeyEvent::new(KeyCode::Char(ch), KeyModifiers::NONE));
}
assert!(composer.is_in_paste_burst());
assert_eq!(composer.textarea.text(), "");

std::thread::sleep(ChatComposer::recommended_paste_flush_delay());
let _ = composer.flush_paste_burst_if_due();
let _ = flush_after_paste_burst(&mut composer);

assert_eq!(composer.textarea.text(), "hi?there");
assert_ne!(composer.footer_mode, FooterMode::ShortcutOverlay);
Expand Down Expand Up @@ -2510,6 +2547,116 @@ mod tests {
}
}

#[test]
fn non_ascii_char_inserts_immediately_without_burst_state() {
use crossterm::event::KeyCode;
use crossterm::event::KeyEvent;
use crossterm::event::KeyModifiers;

let (tx, _rx) = unbounded_channel::<AppEvent>();
let sender = AppEventSender::new(tx);
let mut composer = ChatComposer::new(
true,
sender,
false,
"Ask Codex to do anything".to_string(),
false,
);

let _ = composer.handle_key_event(KeyEvent::new(KeyCode::Char('あ'), KeyModifiers::NONE));

assert_eq!(composer.textarea.text(), "あ");
assert!(!composer.is_in_paste_burst());
}

// test a variety of non-ascii char sequences to ensure we are handling them correctly
#[test]
fn non_ascii_burst_handles_newline() {
let test_cases = [
// triggers on windows
"天地玄黄 宇宙洪荒
日月盈昃 辰宿列张
寒来暑往 秋收冬藏

你好世界 编码测试
汉字处理 UTF-8
终端显示 正确无误

风吹竹林 月照大江
白云千载 青山依旧
程序员 与 Unicode 同行",
// Simulate pasting "你 好\nhi" with an ideographic space to trigger pastey heuristics.
"你 好\nhi",
];

for test_case in test_cases {
use crossterm::event::KeyCode;
use crossterm::event::KeyEvent;
use crossterm::event::KeyModifiers;

let (tx, _rx) = unbounded_channel::<AppEvent>();
let sender = AppEventSender::new(tx);
let mut composer = ChatComposer::new(
true,
sender,
false,
"Ask Codex to do anything".to_string(),
false,
);

for c in test_case.chars() {
let _ =
composer.handle_key_event(KeyEvent::new(KeyCode::Char(c), KeyModifiers::NONE));
}

assert!(
composer.textarea.text().is_empty(),
"non-empty textarea before flush: {test_case}",
);
let _ = flush_after_paste_burst(&mut composer);
assert_eq!(composer.textarea.text(), test_case);
}
}

#[test]
fn ascii_burst_treats_enter_as_newline() {
use crossterm::event::KeyCode;
use crossterm::event::KeyEvent;
use crossterm::event::KeyModifiers;

let (tx, _rx) = unbounded_channel::<AppEvent>();
let sender = AppEventSender::new(tx);
let mut composer = ChatComposer::new(
true,
sender,
false,
"Ask Codex to do anything".to_string(),
false,
);

// Force an active burst so this test doesn't depend on tight timing.
composer
.paste_burst
.begin_with_retro_grabbed(String::new(), Instant::now());

let _ = composer.handle_key_event(KeyEvent::new(KeyCode::Char('h'), KeyModifiers::NONE));
let _ = composer.handle_key_event(KeyEvent::new(KeyCode::Char('i'), KeyModifiers::NONE));

let (result, _) =
composer.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE));
assert!(
matches!(result, InputResult::None),
"Enter during a burst should insert newline, not submit"
);

for ch in ['t', 'h', 'e', 'r', 'e'] {
let _ = composer.handle_key_event(KeyEvent::new(KeyCode::Char(ch), KeyModifiers::NONE));
}

let _ = flush_after_paste_burst(&mut composer);
assert_eq!(composer.textarea.text(), "hi\nthere");
}

#[test]
fn handle_paste_small_inserts_text() {
use crossterm::event::KeyCode;
Expand Down Expand Up @@ -2800,6 +2947,11 @@ mod tests {
}
}

fn flush_after_paste_burst(composer: &mut ChatComposer) -> bool {
std::thread::sleep(PasteBurst::recommended_active_flush_delay());
composer.flush_paste_burst_if_due()
}

// Test helper: simulate human typing with a brief delay and flush the paste-burst buffer
fn type_chars_humanlike(composer: &mut ChatComposer, chars: &[char]) {
use crossterm::event::KeyCode;
Expand Down Expand Up @@ -3987,6 +4139,33 @@ mod tests {
assert_eq!(InputResult::Submitted(expected), result);
}

#[test]
fn pending_first_ascii_char_flushes_as_typed() {
use crossterm::event::KeyCode;
use crossterm::event::KeyEvent;
use crossterm::event::KeyModifiers;

let (tx, _rx) = unbounded_channel::<AppEvent>();
let sender = AppEventSender::new(tx);
let mut composer = ChatComposer::new(
true,
sender,
false,
"Ask Codex to do anything".to_string(),
false,
);

let _ = composer.handle_key_event(KeyEvent::new(KeyCode::Char('h'), KeyModifiers::NONE));
assert!(composer.is_in_paste_burst());
assert!(composer.textarea.text().is_empty());

std::thread::sleep(ChatComposer::recommended_paste_flush_delay());
let flushed = composer.flush_paste_burst_if_due();
assert!(flushed, "expected pending first char to flush");
assert_eq!(composer.textarea.text(), "h");
assert!(!composer.is_in_paste_burst());
}

#[test]
fn burst_paste_fast_small_buffers_and_flushes_on_stop() {
use crossterm::event::KeyCode;
Expand Down Expand Up @@ -4021,8 +4200,7 @@ mod tests {
composer.textarea.text().is_empty(),
"text should remain empty until flush"
);
std::thread::sleep(ChatComposer::recommended_paste_flush_delay());
let flushed = composer.flush_paste_burst_if_due();
let flushed = flush_after_paste_burst(&mut composer);
assert!(flushed, "expected buffered text to flush after stop");
assert_eq!(composer.textarea.text(), "a".repeat(count));
assert!(
Expand Down Expand Up @@ -4055,8 +4233,7 @@ mod tests {

// Nothing should appear until we stop and flush
assert!(composer.textarea.text().is_empty());
std::thread::sleep(ChatComposer::recommended_paste_flush_delay());
let flushed = composer.flush_paste_burst_if_due();
let flushed = flush_after_paste_burst(&mut composer);
assert!(flushed, "expected flush after stopping fast input");

let expected_placeholder = format!("[Pasted Content {count} chars]");
Expand Down
61 changes: 52 additions & 9 deletions codex-rs/tui/src/bottom_pane/paste_burst.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,12 @@ use std::time::Instant;
const PASTE_BURST_MIN_CHARS: u16 = 3;
const PASTE_BURST_CHAR_INTERVAL: Duration = Duration::from_millis(8);
const PASTE_ENTER_SUPPRESS_WINDOW: Duration = Duration::from_millis(120);
// Slower paste burts have been observed in windows environments, but ideally
// we want to keep this low
#[cfg(not(windows))]
const PASTE_BURST_ACTIVE_IDLE_TIMEOUT: Duration = Duration::from_millis(8);
#[cfg(windows)]
const PASTE_BURST_ACTIVE_IDLE_TIMEOUT: Duration = Duration::from_millis(60);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would be good to capture the reason for the differences in these intervals - are they based on testing / docs / ? Do we have more context on why we might increase or decrease? Are they likely to cause differences in operating systems / remote systems / multiplexers?

Maybe just something here that captures why we chose is enough and we can come back to the deeper questions.

Copy link
Contributor

Choose a reason for hiding this comment

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

In Windows, if we use an 8ms interval, copying long non-ASCII text might cause the Pasted Content Block to be split into multiple chunks, indicating that the paste burst was temporarily exited midway through the paste process. Sometimes, it might even cause line breaks to be recognized as commits. I'm not sure if there's a better way.

The following images show the results caused by two different delays.

8ms:
8ms

60ms:
60ms

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks - mostly I want a short summary in the code, so we know why / how to tweak it if needed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks @occurrent! I added a comment here and made this platform-specific


#[derive(Default)]
pub(crate) struct PasteBurst {
Expand Down Expand Up @@ -52,16 +58,14 @@ impl PasteBurst {
PASTE_BURST_CHAR_INTERVAL + Duration::from_millis(1)
}

#[cfg(test)]
pub(crate) fn recommended_active_flush_delay() -> Duration {
PASTE_BURST_ACTIVE_IDLE_TIMEOUT + Duration::from_millis(1)
}

/// Entry point: decide how to treat a plain char with current timing.
pub fn on_plain_char(&mut self, ch: char, now: Instant) -> CharDecision {
match self.last_plain_char_time {
Some(prev) if now.duration_since(prev) <= PASTE_BURST_CHAR_INTERVAL => {
self.consecutive_plain_char_burst =
self.consecutive_plain_char_burst.saturating_add(1)
}
_ => self.consecutive_plain_char_burst = 1,
}
self.last_plain_char_time = Some(now);
self.note_plain_char(now);

if self.active {
self.burst_window_until = Some(now + PASTE_ENTER_SUPPRESS_WINDOW);
Expand Down Expand Up @@ -92,6 +96,40 @@ impl PasteBurst {
CharDecision::RetainFirstChar
}

/// Like on_plain_char(), but never holds the first char.
///
/// Used for non-ASCII input paths (e.g., IMEs) where holding a character can
/// feel like dropped input, while still allowing burst-based paste detection.
///
/// Note: This method will only ever return BufferAppend or BeginBuffer.
pub fn on_plain_char_no_hold(&mut self, now: Instant) -> Option<CharDecision> {
self.note_plain_char(now);

if self.active {
self.burst_window_until = Some(now + PASTE_ENTER_SUPPRESS_WINDOW);
return Some(CharDecision::BufferAppend);
}

if self.consecutive_plain_char_burst >= PASTE_BURST_MIN_CHARS {
return Some(CharDecision::BeginBuffer {
retro_chars: self.consecutive_plain_char_burst.saturating_sub(1),
});
}

None
}

fn note_plain_char(&mut self, now: Instant) {
match self.last_plain_char_time {
Some(prev) if now.duration_since(prev) <= PASTE_BURST_CHAR_INTERVAL => {
self.consecutive_plain_char_burst =
self.consecutive_plain_char_burst.saturating_add(1)
}
_ => self.consecutive_plain_char_burst = 1,
}
self.last_plain_char_time = Some(now);
}

/// Flush the buffered burst if the inter-key timeout has elapsed.
///
/// Returns Some(String) when either:
Expand All @@ -102,9 +140,14 @@ impl PasteBurst {
///
/// Returns None if the timeout has not elapsed or there is nothing to flush.
pub fn flush_if_due(&mut self, now: Instant) -> FlushResult {
let timeout = if self.is_active_internal() {
PASTE_BURST_ACTIVE_IDLE_TIMEOUT
} else {
PASTE_BURST_CHAR_INTERVAL
};
let timed_out = self
.last_plain_char_time
.is_some_and(|t| now.duration_since(t) > PASTE_BURST_CHAR_INTERVAL);
.is_some_and(|t| now.duration_since(t) > timeout);
if timed_out && self.is_active_internal() {
self.active = false;
let out = std::mem::take(&mut self.buffer);
Expand Down
Loading
Loading