From bd65c4db87886c21895ee0d25f6389f899217e56 Mon Sep 17 00:00:00 2001 From: mattsu <35655889+mattsu2020@users.noreply.github.com> Date: Wed, 27 Aug 2025 10:31:49 +0900 Subject: [PATCH] Fix crash when backspacing placeholders adjacent to multibyte text (#2674) Prevented panics when deleting placeholders near multibyte characters by clamping the cursor to a valid boundary and using get-based slicing Added a regression test to ensure backspacing after multibyte text leaves placeholders intact without crashing --------- Co-authored-by: Ahmed Ibrahim --- codex-rs/tui/src/bottom_pane/chat_composer.rs | 100 +++++++++++++++--- 1 file changed, 86 insertions(+), 14 deletions(-) diff --git a/codex-rs/tui/src/bottom_pane/chat_composer.rs b/codex-rs/tui/src/bottom_pane/chat_composer.rs index 08ebcefc..26b9a571 100644 --- a/codex-rs/tui/src/bottom_pane/chat_composer.rs +++ b/codex-rs/tui/src/bottom_pane/chat_composer.rs @@ -259,7 +259,7 @@ impl ChatComposer { /// Replace the entire composer content with `text` and reset cursor. pub(crate) fn set_text_content(&mut self, text: String) { self.textarea.set_text(&text); - self.textarea.set_cursor(usize::MAX); + self.textarea.set_cursor(0); self.sync_command_popup(); self.sync_file_search_popup(); } @@ -407,6 +407,33 @@ impl ChatComposer { input => self.handle_input_basic(input), } } + #[inline] + fn clamp_to_char_boundary(text: &str, pos: usize) -> usize { + let mut p = pos.min(text.len()); + if p < text.len() && !text.is_char_boundary(p) { + p = text + .char_indices() + .map(|(i, _)| i) + .take_while(|&i| i <= p) + .last() + .unwrap_or(0); + } + p + } + + #[inline] + fn handle_non_ascii_char(&mut self, input: KeyEvent) -> (InputResult, bool) { + if !self.paste_burst_buffer.is_empty() || self.in_paste_burst_mode { + let pasted = std::mem::take(&mut self.paste_burst_buffer); + self.in_paste_burst_mode = false; + self.handle_paste(pasted); + } + self.textarea.input(input); + let text_after = self.textarea.text(); + self.pending_pastes + .retain(|(placeholder, _)| text_after.contains(placeholder)); + (InputResult::None, true) + } /// Handle key events when file search popup is visible. fn handle_key_event_with_file_popup(&mut self, key_event: KeyEvent) -> (InputResult, bool) { @@ -462,8 +489,10 @@ impl ChatComposer { // using the flat text and byte-offset cursor API. let cursor_offset = self.textarea.cursor(); let text = self.textarea.text(); - let before_cursor = &text[..cursor_offset]; - let after_cursor = &text[cursor_offset..]; + // Clamp to a valid char boundary to avoid panics when slicing. + let safe_cursor = Self::clamp_to_char_boundary(text, cursor_offset); + let before_cursor = &text[..safe_cursor]; + let after_cursor = &text[safe_cursor..]; // Determine token boundaries in the full text. let start_idx = before_cursor @@ -476,7 +505,7 @@ impl ChatComposer { .find(|(_, c)| c.is_whitespace()) .map(|(idx, _)| idx) .unwrap_or(after_cursor.len()); - let end_idx = cursor_offset + end_rel_idx; + let end_idx = safe_cursor + end_rel_idx; self.textarea.replace_range(start_idx..end_idx, ""); self.textarea.set_cursor(start_idx); @@ -624,9 +653,11 @@ impl ChatComposer { fn insert_selected_path(&mut self, path: &str) { let cursor_offset = self.textarea.cursor(); let text = self.textarea.text(); + // Clamp to a valid char boundary to avoid panics when slicing. + let safe_cursor = Self::clamp_to_char_boundary(text, cursor_offset); - let before_cursor = &text[..cursor_offset]; - let after_cursor = &text[cursor_offset..]; + let before_cursor = &text[..safe_cursor]; + let after_cursor = &text[safe_cursor..]; // Determine token boundaries. let start_idx = before_cursor @@ -640,7 +671,7 @@ impl ChatComposer { .find(|(_, c)| c.is_whitespace()) .map(|(idx, _)| idx) .unwrap_or(after_cursor.len()); - let end_idx = cursor_offset + end_rel_idx; + let end_idx = safe_cursor + end_rel_idx; // Replace the slice `[start_idx, end_idx)` with the chosen path and a trailing space. let mut new_text = @@ -808,6 +839,13 @@ impl ChatComposer { let has_ctrl_or_alt = modifiers.contains(KeyModifiers::CONTROL) || modifiers.contains(KeyModifiers::ALT); if !has_ctrl_or_alt { + // Non-ASCII characters (e.g., from IMEs) can arrive in quick bursts and be + // misclassified by our non-bracketed paste heuristic. To avoid leaving + // residual buffered content or misdetecting a paste, flush any burst buffer + // and insert non-ASCII characters directly. + if !ch.is_ascii() { + return self.handle_non_ascii_char(input); + } // Update burst heuristics. match self.last_plain_char_time { Some(prev) if now.duration_since(prev) <= PASTE_BURST_CHAR_INTERVAL => { @@ -942,8 +980,9 @@ impl ChatComposer { /// Attempts to remove an image or paste placeholder if the cursor is at the end of one. /// Returns true if a placeholder was removed. fn try_remove_any_placeholder_at_cursor(&mut self) -> bool { - let p = self.textarea.cursor(); + // Clamp the cursor to a valid char boundary to avoid panics when slicing. let text = self.textarea.text(); + let p = Self::clamp_to_char_boundary(text, self.textarea.cursor()); // Try image placeholders first let mut out: Option<(usize, String)> = None; @@ -955,7 +994,7 @@ impl ChatComposer { continue; } let start = p - ph.len(); - if text[start..p] != *ph { + if text.get(start..p) != Some(ph.as_str()) { continue; } @@ -963,7 +1002,11 @@ impl ChatComposer { let mut occ_before = 0usize; let mut search_pos = 0usize; while search_pos < start { - if let Some(found) = text[search_pos..start].find(ph) { + let segment = match text.get(search_pos..start) { + Some(s) => s, + None => break, + }; + if let Some(found) = segment.find(ph) { occ_before += 1; search_pos += found + ph.len(); } else { @@ -999,7 +1042,7 @@ impl ChatComposer { if p + ph.len() > text.len() { continue; } - if &text[p..p + ph.len()] != ph { + if text.get(p..p + ph.len()) != Some(ph.as_str()) { continue; } @@ -1007,7 +1050,11 @@ impl ChatComposer { let mut occ_before = 0usize; let mut search_pos = 0usize; while search_pos < p { - if let Some(found) = text[search_pos..p].find(ph) { + let segment = match text.get(search_pos..p) { + Some(s) => s, + None => break 'out None, + }; + if let Some(found) = segment.find(ph) { occ_before += 1; search_pos += found + ph.len(); } else { @@ -1042,7 +1089,7 @@ impl ChatComposer { return None; } let start = p - ph.len(); - if text[start..p] == *ph { + if text.get(start..p) == Some(ph.as_str()) { Some(ph.clone()) } else { None @@ -1058,7 +1105,7 @@ impl ChatComposer { if p + ph.len() > text.len() { return None; } - if &text[p..p + ph.len()] == ph { + if text.get(p..p + ph.len()) == Some(ph.as_str()) { Some(ph.clone()) } else { None @@ -1907,6 +1954,31 @@ mod tests { } } + #[test] + fn backspace_with_multibyte_text_before_placeholder_does_not_panic() { + use crossterm::event::KeyCode; + use crossterm::event::KeyEvent; + use crossterm::event::KeyModifiers; + + let (tx, _rx) = unbounded_channel::(); + let sender = AppEventSender::new(tx); + let mut composer = + ChatComposer::new(true, sender, false, "Ask Codex to do anything".to_string()); + + // Insert an image placeholder at the start + let path = PathBuf::from("/tmp/image_multibyte.png"); + composer.attach_image(path, 10, 5, "PNG"); + // Add multibyte text after the placeholder + composer.textarea.insert_str("日本語"); + + // Cursor is at end; pressing backspace should delete the last character + // without panicking and leave the placeholder intact. + composer.handle_key_event(KeyEvent::new(KeyCode::Backspace, KeyModifiers::NONE)); + + assert_eq!(composer.attached_images.len(), 1); + assert!(composer.textarea.text().starts_with("[image 10x5 PNG]")); + } + #[test] fn deleting_one_of_duplicate_image_placeholders_removes_matching_entry() { let (tx, _rx) = unbounded_channel::();