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 <aibrahim@openai.com>
This commit is contained in:
@@ -259,7 +259,7 @@ impl ChatComposer {
|
|||||||
/// Replace the entire composer content with `text` and reset cursor.
|
/// Replace the entire composer content with `text` and reset cursor.
|
||||||
pub(crate) fn set_text_content(&mut self, text: String) {
|
pub(crate) fn set_text_content(&mut self, text: String) {
|
||||||
self.textarea.set_text(&text);
|
self.textarea.set_text(&text);
|
||||||
self.textarea.set_cursor(usize::MAX);
|
self.textarea.set_cursor(0);
|
||||||
self.sync_command_popup();
|
self.sync_command_popup();
|
||||||
self.sync_file_search_popup();
|
self.sync_file_search_popup();
|
||||||
}
|
}
|
||||||
@@ -407,6 +407,33 @@ impl ChatComposer {
|
|||||||
input => self.handle_input_basic(input),
|
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.
|
/// Handle key events when file search popup is visible.
|
||||||
fn handle_key_event_with_file_popup(&mut self, key_event: KeyEvent) -> (InputResult, bool) {
|
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.
|
// using the flat text and byte-offset cursor API.
|
||||||
let cursor_offset = self.textarea.cursor();
|
let cursor_offset = self.textarea.cursor();
|
||||||
let text = self.textarea.text();
|
let text = self.textarea.text();
|
||||||
let before_cursor = &text[..cursor_offset];
|
// Clamp to a valid char boundary to avoid panics when slicing.
|
||||||
let after_cursor = &text[cursor_offset..];
|
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.
|
// Determine token boundaries in the full text.
|
||||||
let start_idx = before_cursor
|
let start_idx = before_cursor
|
||||||
@@ -476,7 +505,7 @@ impl ChatComposer {
|
|||||||
.find(|(_, c)| c.is_whitespace())
|
.find(|(_, c)| c.is_whitespace())
|
||||||
.map(|(idx, _)| idx)
|
.map(|(idx, _)| idx)
|
||||||
.unwrap_or(after_cursor.len());
|
.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.replace_range(start_idx..end_idx, "");
|
||||||
self.textarea.set_cursor(start_idx);
|
self.textarea.set_cursor(start_idx);
|
||||||
@@ -624,9 +653,11 @@ impl ChatComposer {
|
|||||||
fn insert_selected_path(&mut self, path: &str) {
|
fn insert_selected_path(&mut self, path: &str) {
|
||||||
let cursor_offset = self.textarea.cursor();
|
let cursor_offset = self.textarea.cursor();
|
||||||
let text = self.textarea.text();
|
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 before_cursor = &text[..safe_cursor];
|
||||||
let after_cursor = &text[cursor_offset..];
|
let after_cursor = &text[safe_cursor..];
|
||||||
|
|
||||||
// Determine token boundaries.
|
// Determine token boundaries.
|
||||||
let start_idx = before_cursor
|
let start_idx = before_cursor
|
||||||
@@ -640,7 +671,7 @@ impl ChatComposer {
|
|||||||
.find(|(_, c)| c.is_whitespace())
|
.find(|(_, c)| c.is_whitespace())
|
||||||
.map(|(idx, _)| idx)
|
.map(|(idx, _)| idx)
|
||||||
.unwrap_or(after_cursor.len());
|
.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.
|
// Replace the slice `[start_idx, end_idx)` with the chosen path and a trailing space.
|
||||||
let mut new_text =
|
let mut new_text =
|
||||||
@@ -808,6 +839,13 @@ impl ChatComposer {
|
|||||||
let has_ctrl_or_alt =
|
let has_ctrl_or_alt =
|
||||||
modifiers.contains(KeyModifiers::CONTROL) || modifiers.contains(KeyModifiers::ALT);
|
modifiers.contains(KeyModifiers::CONTROL) || modifiers.contains(KeyModifiers::ALT);
|
||||||
if !has_ctrl_or_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.
|
// Update burst heuristics.
|
||||||
match self.last_plain_char_time {
|
match self.last_plain_char_time {
|
||||||
Some(prev) if now.duration_since(prev) <= PASTE_BURST_CHAR_INTERVAL => {
|
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.
|
/// Attempts to remove an image or paste placeholder if the cursor is at the end of one.
|
||||||
/// Returns true if a placeholder was removed.
|
/// Returns true if a placeholder was removed.
|
||||||
fn try_remove_any_placeholder_at_cursor(&mut self) -> bool {
|
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 text = self.textarea.text();
|
||||||
|
let p = Self::clamp_to_char_boundary(text, self.textarea.cursor());
|
||||||
|
|
||||||
// Try image placeholders first
|
// Try image placeholders first
|
||||||
let mut out: Option<(usize, String)> = None;
|
let mut out: Option<(usize, String)> = None;
|
||||||
@@ -955,7 +994,7 @@ impl ChatComposer {
|
|||||||
continue;
|
continue;
|
||||||
}
|
}
|
||||||
let start = p - ph.len();
|
let start = p - ph.len();
|
||||||
if text[start..p] != *ph {
|
if text.get(start..p) != Some(ph.as_str()) {
|
||||||
continue;
|
continue;
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -963,7 +1002,11 @@ impl ChatComposer {
|
|||||||
let mut occ_before = 0usize;
|
let mut occ_before = 0usize;
|
||||||
let mut search_pos = 0usize;
|
let mut search_pos = 0usize;
|
||||||
while search_pos < start {
|
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;
|
occ_before += 1;
|
||||||
search_pos += found + ph.len();
|
search_pos += found + ph.len();
|
||||||
} else {
|
} else {
|
||||||
@@ -999,7 +1042,7 @@ impl ChatComposer {
|
|||||||
if p + ph.len() > text.len() {
|
if p + ph.len() > text.len() {
|
||||||
continue;
|
continue;
|
||||||
}
|
}
|
||||||
if &text[p..p + ph.len()] != ph {
|
if text.get(p..p + ph.len()) != Some(ph.as_str()) {
|
||||||
continue;
|
continue;
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -1007,7 +1050,11 @@ impl ChatComposer {
|
|||||||
let mut occ_before = 0usize;
|
let mut occ_before = 0usize;
|
||||||
let mut search_pos = 0usize;
|
let mut search_pos = 0usize;
|
||||||
while search_pos < p {
|
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;
|
occ_before += 1;
|
||||||
search_pos += found + ph.len();
|
search_pos += found + ph.len();
|
||||||
} else {
|
} else {
|
||||||
@@ -1042,7 +1089,7 @@ impl ChatComposer {
|
|||||||
return None;
|
return None;
|
||||||
}
|
}
|
||||||
let start = p - ph.len();
|
let start = p - ph.len();
|
||||||
if text[start..p] == *ph {
|
if text.get(start..p) == Some(ph.as_str()) {
|
||||||
Some(ph.clone())
|
Some(ph.clone())
|
||||||
} else {
|
} else {
|
||||||
None
|
None
|
||||||
@@ -1058,7 +1105,7 @@ impl ChatComposer {
|
|||||||
if p + ph.len() > text.len() {
|
if p + ph.len() > text.len() {
|
||||||
return None;
|
return None;
|
||||||
}
|
}
|
||||||
if &text[p..p + ph.len()] == ph {
|
if text.get(p..p + ph.len()) == Some(ph.as_str()) {
|
||||||
Some(ph.clone())
|
Some(ph.clone())
|
||||||
} else {
|
} else {
|
||||||
None
|
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::<AppEvent>();
|
||||||
|
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]
|
#[test]
|
||||||
fn deleting_one_of_duplicate_image_placeholders_removes_matching_entry() {
|
fn deleting_one_of_duplicate_image_placeholders_removes_matching_entry() {
|
||||||
let (tx, _rx) = unbounded_channel::<AppEvent>();
|
let (tx, _rx) = unbounded_channel::<AppEvent>();
|
||||||
|
|||||||
Reference in New Issue
Block a user