From add648df825563bca7611a005365858270c0041f Mon Sep 17 00:00:00 2001 From: Charley Cunningham Date: Tue, 27 Jan 2026 18:39:59 -0800 Subject: [PATCH] Restore image attachments/text elements when recalling input history (Up/Down) (#9628) **Summary** - Up/Down input history now restores image attachments and text elements for local entries. - Composer history stores rich local entries (text + text elements + local image paths) while persistent history remains text-only. - Added tests to verify history recall rehydrates image placeholders and attachments in both `tui` and `tui2`. **Changes** - `tui/src/bottom_pane/chat_composer_history.rs`: store `HistoryEntry` (text + elements + image paths) for local history; adapt navigation + tests. - `tui2/src/bottom_pane/chat_composer_history.rs`: same as above. - `tui/src/bottom_pane/chat_composer.rs`: record rich history entries and restore them on Up/Down; update Ctrl+C history and tests. - `tui2/src/bottom_pane/chat_composer.rs`: same as above. --- codex-rs/tui/src/bottom_pane/chat_composer.rs | 86 +++++++++++-- .../src/bottom_pane/chat_composer_history.rs | 114 +++++++++++++----- codex-rs/tui/src/chatwidget/tests.rs | 5 +- docs/tui-chat-composer.md | 13 ++ 4 files changed, 171 insertions(+), 47 deletions(-) diff --git a/codex-rs/tui/src/bottom_pane/chat_composer.rs b/codex-rs/tui/src/bottom_pane/chat_composer.rs index af45054792..e59f820821 100644 --- a/codex-rs/tui/src/bottom_pane/chat_composer.rs +++ b/codex-rs/tui/src/bottom_pane/chat_composer.rs @@ -15,6 +15,16 @@ //! [`ChatComposer::handle_key_event_without_popup`]. After every handled key, we call //! [`ChatComposer::sync_popups`] so UI state follows the latest buffer/cursor. //! +//! # History Navigation (↑/↓) +//! +//! The Up/Down history path is managed by [`ChatComposerHistory`]. It merges: +//! +//! - Persistent cross-session history (text-only; no element ranges or attachments). +//! - Local in-session history (full text + text elements + local image paths). +//! +//! When recalling a local entry, the composer rehydrates text elements and image attachments. +//! When recalling a persistent entry, only the text is restored. +//! //! # Submission and Prompt Expansion //! //! On submit/queue paths, the composer: @@ -89,6 +99,7 @@ use ratatui::widgets::StatefulWidgetRef; use ratatui::widgets::WidgetRef; use super::chat_composer_history::ChatComposerHistory; +use super::chat_composer_history::HistoryEntry; use super::command_popup::CommandItem; use super::command_popup::CommandPopup; use super::command_popup::CommandPopupFlags; @@ -460,11 +471,12 @@ impl ChatComposer { offset: usize, entry: Option, ) -> bool { - let Some(text) = self.history.on_entry_response(log_id, offset, entry) else { + let Some(entry) = self.history.on_entry_response(log_id, offset, entry) else { return false; }; - // Composer history (↑/↓) stores plain text only; no UI element ranges/attachments to restore here. - self.set_text_content(text, Vec::new(), Vec::new()); + // 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(entry.text, entry.text_elements, entry.local_image_paths); true } @@ -707,9 +719,19 @@ impl ChatComposer { return None; } let previous = self.current_text(); + let text_elements = self.textarea.text_elements(); + let local_image_paths = self + .attached_images + .iter() + .map(|img| img.path.clone()) + .collect(); self.set_text_content(String::new(), Vec::new(), Vec::new()); self.history.reset_navigation(); - self.history.record_local_submission(&previous); + self.history.record_local_submission(HistoryEntry { + text: previous.clone(), + text_elements, + local_image_paths, + }); Some(previous) } @@ -1792,8 +1814,17 @@ impl ChatComposer { if text.is_empty() && self.attached_images.is_empty() { return None; } - if !text.is_empty() { - self.history.record_local_submission(&text); + if !text.is_empty() || !self.attached_images.is_empty() { + let local_image_paths = self + .attached_images + .iter() + .map(|img| img.path.clone()) + .collect(); + self.history.record_local_submission(HistoryEntry { + text: text.clone(), + text_elements: text_elements.clone(), + local_image_paths, + }); } self.pending_pastes.clear(); Some((text, text_elements)) @@ -1989,15 +2020,19 @@ impl ChatComposer { .history .should_handle_navigation(self.textarea.text(), self.textarea.cursor()) { - let replace_text = match key_event.code { + let replace_entry = match key_event.code { KeyCode::Up => self.history.navigate_up(&self.app_event_tx), KeyCode::Down => self.history.navigate_down(&self.app_event_tx), KeyCode::Char('p') => self.history.navigate_up(&self.app_event_tx), KeyCode::Char('n') => self.history.navigate_down(&self.app_event_tx), _ => unreachable!(), }; - if let Some(text) = replace_text { - self.set_text_content(text, Vec::new(), Vec::new()); + if let Some(entry) = replace_entry { + self.set_text_content( + entry.text, + entry.text_elements, + entry.local_image_paths, + ); return (InputResult::None, true); } } @@ -3268,7 +3303,7 @@ mod tests { assert_eq!( composer.history.navigate_up(&composer.app_event_tx), - Some("draft text".to_string()) + Some(HistoryEntry::from_text("draft text".to_string())) ); } @@ -4721,6 +4756,37 @@ mod tests { assert_eq!(vec![path], imgs); } + #[test] + fn history_navigation_restores_image_attachments() { + 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, + ); + composer.set_steer_enabled(true); + let path = PathBuf::from("/tmp/image1.png"); + composer.attach_image(path.clone()); + + let (result, _needs_redraw) = + composer.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE)); + assert!(matches!(result, InputResult::Submitted { .. })); + + composer.set_text_content(String::new(), Vec::new(), Vec::new()); + + let _ = composer.handle_key_event(KeyEvent::new(KeyCode::Up, KeyModifiers::NONE)); + + let text = composer.current_text(); + assert_eq!(text, "[Image #1]"); + let text_elements = composer.text_elements(); + assert_eq!(text_elements.len(), 1); + assert_eq!(text_elements[0].placeholder(&text), Some("[Image #1]")); + assert_eq!(composer.local_image_paths(), vec![path]); + } + #[test] fn set_text_content_reattaches_images_without_placeholder_metadata() { 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 991283a566..da9f46ae4b 100644 --- a/codex-rs/tui/src/bottom_pane/chat_composer_history.rs +++ b/codex-rs/tui/src/bottom_pane/chat_composer_history.rs @@ -1,8 +1,35 @@ use std::collections::HashMap; +use std::path::PathBuf; use crate::app_event::AppEvent; use crate::app_event_sender::AppEventSender; use codex_core::protocol::Op; +use codex_protocol::user_input::TextElement; + +#[derive(Debug, Clone, PartialEq)] +pub(crate) struct HistoryEntry { + pub(crate) text: String, + pub(crate) text_elements: Vec, + pub(crate) local_image_paths: Vec, +} + +impl HistoryEntry { + fn empty() -> Self { + Self { + text: String::new(), + text_elements: Vec::new(), + local_image_paths: Vec::new(), + } + } + + pub(crate) fn from_text(text: String) -> Self { + Self { + text, + text_elements: Vec::new(), + local_image_paths: Vec::new(), + } + } +} /// State machine that manages shell-style history navigation (Up/Down) inside /// the chat composer. This struct is intentionally decoupled from the @@ -15,10 +42,10 @@ pub(crate) struct ChatComposerHistory { history_entry_count: usize, /// Messages submitted by the user *during this UI session* (newest at END). - local_history: Vec, + local_history: Vec, /// Cache of persistent history entries fetched on-demand. - fetched_history: HashMap, + fetched_history: HashMap, /// Current cursor within the combined (persistent + local) history. `None` /// indicates the user is *not* currently browsing history. @@ -54,8 +81,8 @@ 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, text: &str) { - if text.is_empty() { + pub fn record_local_submission(&mut self, entry: HistoryEntry) { + if entry.text.is_empty() && entry.local_image_paths.is_empty() { return; } @@ -63,11 +90,11 @@ impl ChatComposerHistory { self.last_history_text = None; // Avoid inserting a duplicate if identical to the previous entry. - if self.local_history.last().is_some_and(|prev| prev == text) { + if self.local_history.last().is_some_and(|prev| prev == &entry) { return; } - self.local_history.push(text.to_string()); + self.local_history.push(entry); } /// Reset navigation tracking so the next Up key resumes from the latest entry. @@ -99,7 +126,7 @@ impl ChatComposerHistory { /// Handle . Returns true when the key was consumed and the caller /// should request a redraw. - pub fn navigate_up(&mut self, app_event_tx: &AppEventSender) -> Option { + pub fn navigate_up(&mut self, app_event_tx: &AppEventSender) -> Option { let total_entries = self.history_entry_count + self.local_history.len(); if total_entries == 0 { return None; @@ -116,7 +143,7 @@ impl ChatComposerHistory { } /// Handle . - pub fn navigate_down(&mut self, app_event_tx: &AppEventSender) -> Option { + pub fn navigate_down(&mut self, app_event_tx: &AppEventSender) -> Option { let total_entries = self.history_entry_count + self.local_history.len(); if total_entries == 0 { return None; @@ -137,7 +164,7 @@ impl ChatComposerHistory { // Past newest – clear and exit browsing mode. self.history_cursor = None; self.last_history_text = None; - Some(String::new()) + Some(HistoryEntry::empty()) } } } @@ -148,16 +175,17 @@ impl ChatComposerHistory { log_id: u64, offset: usize, entry: Option, - ) -> Option { + ) -> Option { if self.history_log_id != Some(log_id) { return None; } let text = entry?; - self.fetched_history.insert(offset, text.clone()); + let entry = HistoryEntry::from_text(text); + self.fetched_history.insert(offset, entry.clone()); if self.history_cursor == Some(offset as isize) { - self.last_history_text = Some(text.clone()); - return Some(text); + self.last_history_text = Some(entry.text.clone()); + return Some(entry); } None } @@ -170,19 +198,20 @@ impl ChatComposerHistory { &mut self, global_idx: usize, app_event_tx: &AppEventSender, - ) -> Option { + ) -> Option { if global_idx >= self.history_entry_count { // Local entry. - if let Some(text) = self + if let Some(entry) = self .local_history .get(global_idx - self.history_entry_count) + .cloned() { - self.last_history_text = Some(text.clone()); - return Some(text.clone()); + self.last_history_text = Some(entry.text.clone()); + return Some(entry); } - } else if let Some(text) = self.fetched_history.get(&global_idx) { - self.last_history_text = Some(text.clone()); - return Some(text.clone()); + } else if let Some(entry) = self.fetched_history.get(&global_idx).cloned() { + self.last_history_text = Some(entry.text.clone()); + return Some(entry); } else if let Some(log_id) = self.history_log_id { let op = Op::GetHistoryEntryRequest { offset: global_idx, @@ -206,22 +235,28 @@ mod tests { let mut history = ChatComposerHistory::new(); // Empty submissions are ignored. - history.record_local_submission(""); + history.record_local_submission(HistoryEntry::from_text(String::new())); assert_eq!(history.local_history.len(), 0); // First entry is recorded. - history.record_local_submission("hello"); + history.record_local_submission(HistoryEntry::from_text("hello".to_string())); assert_eq!(history.local_history.len(), 1); - assert_eq!(history.local_history.last().unwrap(), "hello"); + assert_eq!( + history.local_history.last().unwrap(), + &HistoryEntry::from_text("hello".to_string()) + ); // Identical consecutive entry is skipped. - history.record_local_submission("hello"); + history.record_local_submission(HistoryEntry::from_text("hello".to_string())); assert_eq!(history.local_history.len(), 1); // Different entry is recorded. - history.record_local_submission("world"); + history.record_local_submission(HistoryEntry::from_text("world".to_string())); assert_eq!(history.local_history.len(), 2); - assert_eq!(history.local_history.last().unwrap(), "world"); + assert_eq!( + history.local_history.last().unwrap(), + &HistoryEntry::from_text("world".to_string()) + ); } #[test] @@ -252,7 +287,7 @@ mod tests { // Inject the async response. assert_eq!( - Some("latest".into()), + Some(HistoryEntry::from_text("latest".to_string())), history.on_entry_response(1, 2, Some("latest".into())) ); @@ -273,7 +308,7 @@ mod tests { ); assert_eq!( - Some("older".into()), + Some(HistoryEntry::from_text("older".to_string())), history.on_entry_response(1, 1, Some("older".into())) ); } @@ -285,16 +320,29 @@ mod tests { let mut history = ChatComposerHistory::new(); history.set_metadata(1, 3); - history.fetched_history.insert(1, "command2".into()); - history.fetched_history.insert(2, "command3".into()); + history + .fetched_history + .insert(1, HistoryEntry::from_text("command2".to_string())); + history + .fetched_history + .insert(2, HistoryEntry::from_text("command3".to_string())); - assert_eq!(Some("command3".into()), history.navigate_up(&tx)); - assert_eq!(Some("command2".into()), history.navigate_up(&tx)); + assert_eq!( + Some(HistoryEntry::from_text("command3".to_string())), + history.navigate_up(&tx) + ); + assert_eq!( + Some(HistoryEntry::from_text("command2".to_string())), + history.navigate_up(&tx) + ); history.reset_navigation(); assert!(history.history_cursor.is_none()); assert!(history.last_history_text.is_none()); - assert_eq!(Some("command3".into()), history.navigate_up(&tx)); + assert_eq!( + Some(HistoryEntry::from_text("command3".to_string())), + history.navigate_up(&tx) + ); } } diff --git a/codex-rs/tui/src/chatwidget/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index 76d5600010..64060aece8 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -1749,10 +1749,7 @@ async fn ctrl_c_cleared_prompt_is_recoverable_via_history() { assert!(!chat.bottom_pane.quit_shortcut_hint_visible()); let images = chat.bottom_pane.take_recent_submission_images(); - assert!( - images.is_empty(), - "attachments are not preserved in history recall" - ); + assert_eq!(vec![PathBuf::from("/tmp/preview.png")], images); } #[tokio::test] diff --git a/docs/tui-chat-composer.md b/docs/tui-chat-composer.md index baab4ce0ed..01b5334d94 100644 --- a/docs/tui-chat-composer.md +++ b/docs/tui-chat-composer.md @@ -49,6 +49,19 @@ The solution is to detect paste-like _bursts_ and buffer them into a single expl - After handling the key, `sync_popups()` runs so popup visibility/filters stay consistent with the latest text + cursor. +### History navigation (↑/↓) + +Up/Down recall is handled by `ChatComposerHistory` and merges two sources: + +- **Persistent history** (cross-session, fetched from `~/.codex/history.jsonl`): text-only. It + does **not** carry text element ranges or local image attachments, so recalling one of these + entries only restores the text. +- **Local history** (current session): stores the full submission payload, including text + elements and local image paths. Recalling a local entry rehydrates placeholders and attachments. + +This distinction keeps the on-disk history backward compatible and avoids persisting attachments, +while still providing a richer recall experience for in-session edits. + ## Config gating for reuse `ChatComposer` now supports feature gating via `ChatComposerConfig`