From e8b98feba6513f31330ca33ffdb059a01059613d Mon Sep 17 00:00:00 2001 From: Chriss4123 Date: Sat, 10 Jan 2026 23:26:34 +0200 Subject: [PATCH] fix(tui): rehydrate drafts and normalize image placeholders --- codex-rs/tui/src/bottom_pane/chat_composer.rs | 174 ++++++++++++++++-- .../src/bottom_pane/chat_composer_history.rs | 78 +++++--- docs/tui-chat-composer.md | 45 +++++ 3 files changed, 255 insertions(+), 42 deletions(-) diff --git a/codex-rs/tui/src/bottom_pane/chat_composer.rs b/codex-rs/tui/src/bottom_pane/chat_composer.rs index 5ce5ebafb4d..60fea9b0888 100644 --- a/codex-rs/tui/src/bottom_pane/chat_composer.rs +++ b/codex-rs/tui/src/bottom_pane/chat_composer.rs @@ -540,12 +540,7 @@ impl ChatComposer { }; // Persistent ↑/↓ history is text-only (backwards-compatible and avoids persisting // attachments), but local in-session ↑/↓ history can rehydrate elements and image paths. - self.set_text_content_with_mention_bindings( - entry.text, - entry.text_elements, - entry.local_image_paths, - entry.mention_bindings, - ); + self.apply_history_entry(entry); true } @@ -837,6 +832,7 @@ impl ChatComposer { .iter() .map(|img| img.path.clone()) .collect(); + let pending_pastes = std::mem::take(&mut self.pending_pastes); let mention_bindings = self.snapshot_mention_bindings(); self.set_text_content(String::new(), Vec::new(), Vec::new()); self.history.reset_navigation(); @@ -845,6 +841,7 @@ impl ChatComposer { text_elements, local_image_paths, mention_bindings, + pending_pastes, }); Some(previous) } @@ -854,6 +851,23 @@ impl ChatComposer { self.textarea.text().to_string() } + fn apply_history_entry(&mut self, entry: HistoryEntry) { + let HistoryEntry { + text, + text_elements, + local_image_paths, + mention_bindings, + pending_pastes, + } = entry; + self.set_text_content_with_mention_bindings( + text, + text_elements, + local_image_paths, + mention_bindings, + ); + self.set_pending_pastes(pending_pastes); + } + pub(crate) fn text_elements(&self) -> Vec { self.textarea.text_elements() } @@ -2065,6 +2079,7 @@ impl ChatComposer { text_elements: text_elements.clone(), local_image_paths, mention_bindings: original_mention_bindings, + pending_pastes: Vec::new(), }); } self.pending_pastes.clear(); @@ -2352,12 +2367,7 @@ impl ChatComposer { _ => unreachable!(), }; if let Some(entry) = replace_entry { - self.set_text_content_with_mention_bindings( - entry.text, - entry.text_elements, - entry.local_image_paths, - entry.mention_bindings, - ); + self.apply_history_entry(entry); return (InputResult::None, true); } } @@ -4057,10 +4067,120 @@ mod tests { assert_eq!( composer.history.navigate_up(&composer.app_event_tx), - Some(HistoryEntry::from_text("draft text".to_string())) + Some(HistoryEntry::new("draft text".to_string())) ); } + #[test] + fn clear_for_ctrl_c_preserves_pending_paste_history_entry() { + 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(), + false, + ); + + let large = "x".repeat(LARGE_PASTE_CHAR_THRESHOLD + 5); + composer.handle_paste(large.clone()); + let char_count = large.chars().count(); + let placeholder = format!("[Pasted Content {char_count} chars]"); + assert_eq!(composer.textarea.text(), placeholder); + assert_eq!( + composer.pending_pastes, + vec![(placeholder.clone(), large.clone())] + ); + + composer.clear_for_ctrl_c(); + assert!(composer.is_empty()); + + let history_entry = composer + .history + .navigate_up(&composer.app_event_tx) + .expect("expected history entry"); + let text_elements = vec![TextElement::new( + (0..placeholder.len()).into(), + Some(placeholder.clone()), + )]; + assert_eq!( + history_entry, + HistoryEntry::with_pending( + placeholder.clone(), + text_elements, + Vec::new(), + vec![(placeholder.clone(), large.clone())] + ) + ); + + composer.apply_history_entry(history_entry); + assert_eq!(composer.textarea.text(), placeholder); + assert_eq!(composer.pending_pastes, vec![(placeholder.clone(), large)]); + assert_eq!(composer.textarea.element_payloads(), vec![placeholder]); + + composer.set_steer_enabled(true); + let (result, _needs_redraw) = + composer.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE)); + match result { + InputResult::Submitted { + text, + text_elements, + } => { + assert_eq!(text, "x".repeat(LARGE_PASTE_CHAR_THRESHOLD + 5)); + assert!(text_elements.is_empty()); + } + _ => panic!("expected Submitted"), + } + } + + #[test] + fn clear_for_ctrl_c_preserves_image_draft_state() { + 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(), + false, + ); + + let path = PathBuf::from("example.png"); + composer.attach_image(path.clone()); + let placeholder = local_image_label_text(1); + + composer.clear_for_ctrl_c(); + assert!(composer.is_empty()); + + let history_entry = composer + .history + .navigate_up(&composer.app_event_tx) + .expect("expected history entry"); + let text_elements = vec![TextElement::new( + (0..placeholder.len()).into(), + Some(placeholder.clone()), + )]; + assert_eq!( + history_entry, + HistoryEntry::with_pending( + placeholder.clone(), + text_elements, + vec![path.clone()], + Vec::new() + ) + ); + + composer.apply_history_entry(history_entry); + assert_eq!(composer.textarea.text(), placeholder); + assert_eq!(composer.local_image_paths(), vec![path]); + assert_eq!(composer.textarea.element_payloads(), vec![placeholder]); + } + /// Behavior: `?` toggles the shortcut overlay only when the composer is otherwise empty. After /// any typing has occurred, `?` should be inserted as a literal character. #[test] @@ -7481,6 +7601,34 @@ mod tests { assert!(composer.attached_images.is_empty()); } + #[test] + fn apply_external_edit_renumbers_image_placeholders() { + 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(), + false, + ); + + let first_path = PathBuf::from("img1.png"); + let second_path = PathBuf::from("img2.png"); + composer.attach_image(first_path); + composer.attach_image(second_path.clone()); + + let placeholder2 = local_image_label_text(2); + composer.apply_external_edit(format!("Keep {placeholder2}")); + + let placeholder1 = local_image_label_text(1); + assert_eq!(composer.current_text(), format!("Keep {placeholder1}")); + assert_eq!(composer.attached_images.len(), 1); + assert_eq!(composer.attached_images[0].placeholder, placeholder1); + assert_eq!(composer.local_image_paths(), vec![second_path]); + assert_eq!(composer.textarea.element_payloads(), vec![placeholder1]); + } + #[test] fn current_text_with_pending_expands_placeholders() { let (tx, _rx) = unbounded_channel::(); diff --git a/codex-rs/tui/src/bottom_pane/chat_composer_history.rs b/codex-rs/tui/src/bottom_pane/chat_composer_history.rs index 6d9322cd194..d080b79369c 100644 --- a/codex-rs/tui/src/bottom_pane/chat_composer_history.rs +++ b/codex-rs/tui/src/bottom_pane/chat_composer_history.rs @@ -8,25 +8,23 @@ use crate::mention_codec::decode_history_mentions; use codex_core::protocol::Op; use codex_protocol::user_input::TextElement; +/// A composer history entry that can rehydrate draft state. #[derive(Debug, Clone, PartialEq)] pub(crate) struct HistoryEntry { + /// Raw text stored in history (may include placeholder strings). pub(crate) text: String, + /// Text element ranges for placeholders inside `text`. pub(crate) text_elements: Vec, + /// Local image paths captured alongside `text_elements`. pub(crate) local_image_paths: Vec, + /// Mention bindings for tool/app/skill references inside `text`. pub(crate) mention_bindings: Vec, + /// Placeholder-to-payload pairs used to restore large paste content. + pub(crate) pending_pastes: Vec<(String, String)>, } impl HistoryEntry { - fn empty() -> Self { - Self { - text: String::new(), - text_elements: Vec::new(), - local_image_paths: Vec::new(), - mention_bindings: Vec::new(), - } - } - - pub(crate) fn from_text(text: String) -> Self { + pub(crate) fn new(text: String) -> Self { let decoded = decode_history_mentions(&text); Self { text: decoded.text, @@ -40,6 +38,23 @@ impl HistoryEntry { path: mention.path, }) .collect(), + pending_pastes: Vec::new(), + } + } + + #[cfg(test)] + pub(crate) fn with_pending( + text: String, + text_elements: Vec, + local_image_paths: Vec, + pending_pastes: Vec<(String, String)>, + ) -> Self { + Self { + text, + text_elements, + local_image_paths, + mention_bindings: Vec::new(), + pending_pastes, } } } @@ -55,9 +70,10 @@ pub(crate) struct ChatComposerHistory { history_entry_count: usize, /// Messages submitted by the user *during this UI session* (newest at END). + /// Local entries retain full draft state (text elements, image paths, pending pastes). local_history: Vec, - /// Cache of persistent history entries fetched on-demand. + /// Cache of persistent history entries fetched on-demand (text-only). fetched_history: HashMap, /// Current cursor within the combined (persistent + local) history. `None` @@ -95,10 +111,14 @@ impl ChatComposerHistory { /// Record a message submitted by the user in the current session so it can /// be recalled later. pub fn record_local_submission(&mut self, entry: HistoryEntry) { - if entry.text.is_empty() && entry.local_image_paths.is_empty() { + if entry.text.is_empty() + && entry.text_elements.is_empty() + && entry.local_image_paths.is_empty() + && entry.mention_bindings.is_empty() + && entry.pending_pastes.is_empty() + { return; } - self.history_cursor = None; self.last_history_text = None; @@ -177,7 +197,7 @@ impl ChatComposerHistory { // Past newest – clear and exit browsing mode. self.history_cursor = None; self.last_history_text = None; - Some(HistoryEntry::empty()) + Some(HistoryEntry::new(String::new())) } } } @@ -192,8 +212,7 @@ impl ChatComposerHistory { if self.history_log_id != Some(log_id) { return None; } - let text = entry?; - let entry = HistoryEntry::from_text(text); + let entry = HistoryEntry::new(entry?); self.fetched_history.insert(offset, entry.clone()); if self.history_cursor == Some(offset as isize) { @@ -241,6 +260,7 @@ mod tests { use super::*; use crate::app_event::AppEvent; use codex_core::protocol::Op; + use pretty_assertions::assert_eq; use tokio::sync::mpsc::unbounded_channel; #[test] @@ -248,27 +268,27 @@ mod tests { let mut history = ChatComposerHistory::new(); // Empty submissions are ignored. - history.record_local_submission(HistoryEntry::from_text(String::new())); + history.record_local_submission(HistoryEntry::new(String::new())); assert_eq!(history.local_history.len(), 0); // First entry is recorded. - history.record_local_submission(HistoryEntry::from_text("hello".to_string())); + history.record_local_submission(HistoryEntry::new("hello".to_string())); assert_eq!(history.local_history.len(), 1); assert_eq!( history.local_history.last().unwrap(), - &HistoryEntry::from_text("hello".to_string()) + &HistoryEntry::new("hello".to_string()) ); // Identical consecutive entry is skipped. - history.record_local_submission(HistoryEntry::from_text("hello".to_string())); + history.record_local_submission(HistoryEntry::new("hello".to_string())); assert_eq!(history.local_history.len(), 1); // Different entry is recorded. - history.record_local_submission(HistoryEntry::from_text("world".to_string())); + history.record_local_submission(HistoryEntry::new("world".to_string())); assert_eq!(history.local_history.len(), 2); assert_eq!( history.local_history.last().unwrap(), - &HistoryEntry::from_text("world".to_string()) + &HistoryEntry::new("world".to_string()) ); } @@ -300,7 +320,7 @@ mod tests { // Inject the async response. assert_eq!( - Some(HistoryEntry::from_text("latest".to_string())), + Some(HistoryEntry::new("latest".to_string())), history.on_entry_response(1, 2, Some("latest".into())) ); @@ -321,7 +341,7 @@ mod tests { ); assert_eq!( - Some(HistoryEntry::from_text("older".to_string())), + Some(HistoryEntry::new("older".to_string())), history.on_entry_response(1, 1, Some("older".into())) ); } @@ -335,17 +355,17 @@ mod tests { history.set_metadata(1, 3); history .fetched_history - .insert(1, HistoryEntry::from_text("command2".to_string())); + .insert(1, HistoryEntry::new("command2".to_string())); history .fetched_history - .insert(2, HistoryEntry::from_text("command3".to_string())); + .insert(2, HistoryEntry::new("command3".to_string())); assert_eq!( - Some(HistoryEntry::from_text("command3".to_string())), + Some(HistoryEntry::new("command3".to_string())), history.navigate_up(&tx) ); assert_eq!( - Some(HistoryEntry::from_text("command2".to_string())), + Some(HistoryEntry::new("command2".to_string())), history.navigate_up(&tx) ); @@ -354,7 +374,7 @@ mod tests { assert!(history.last_history_text.is_none()); assert_eq!( - Some(HistoryEntry::from_text("command3".to_string())), + Some(HistoryEntry::new("command3".to_string())), history.navigate_up(&tx) ); } diff --git a/docs/tui-chat-composer.md b/docs/tui-chat-composer.md index b927e2db448..89de775e41f 100644 --- a/docs/tui-chat-composer.md +++ b/docs/tui-chat-composer.md @@ -123,6 +123,51 @@ positional args, Enter auto-submits without calling `prepare_submission_text`. T - Prunes attachments based on expanded placeholders. - Clears pending pastes after a successful auto-submit. +## History navigation (Up/Down) and backtrack prefill + +`ChatComposerHistory` merges two kinds of history: + +- **Persistent history** (cross-session, fetched from core on demand): text-only. +- **Local history** (this UI session): full draft state. + +Local history entries capture: + +- raw text (including placeholders), +- `TextElement` ranges for placeholders, +- local image paths, +- pending large-paste payloads (for drafts). + +Persistent history entries only restore text. They intentionally do **not** rehydrate attachments +or pending paste payloads. + +### Draft recovery (Ctrl+C) + +Ctrl+C clears the composer but stashes the full draft state (text elements, image paths, and +pending paste payloads) into local history. Pressing Up immediately restores that draft, including +image placeholders and large-paste placeholders with their payloads. + +### Submitted message recall + +After a successful submission, the local history entry stores the submitted text and any element +ranges and local image paths. Pending paste payloads are cleared during submission, so large-paste +placeholders are expanded into their full text before being recorded. This means: + +- Up/Down recall of a submitted message restores image placeholders and their local paths. +- Large-paste placeholders are not expected in recalled submitted history; the text is the + expanded paste content. + +### Backtrack prefill + +Backtrack selections read `UserHistoryCell` data from the transcript. The composer prefill now +reuses the selected message’s text elements and local image paths, so image placeholders and +attachments rehydrate when rolling back to a prior user message. + +### External editor edits + +When the composer content is replaced from an external editor, the composer rebuilds text elements +and keeps only attachments whose placeholders still appear in the new text. Image placeholders are +then normalized to `[Image #1]..[Image #N]` to keep attachment mapping consistent after edits. + ## Paste burst: concepts and assumptions The burst detector is intentionally conservative: it only processes “plain” character input