diff --git a/codex-rs/protocol/src/models.rs b/codex-rs/protocol/src/models.rs index b5ce0f2b2..2f0a024fd 100644 --- a/codex-rs/protocol/src/models.rs +++ b/codex-rs/protocol/src/models.rs @@ -720,15 +720,18 @@ impl From> for ResponseInputItem { .into_iter() .flat_map(|c| match c { UserInput::Text { text, .. } => vec![ContentItem::InputText { text }], - UserInput::Image { image_url } => vec![ - ContentItem::InputText { - text: image_open_tag_text(), - }, - ContentItem::InputImage { image_url }, - ContentItem::InputText { - text: image_close_tag_text(), - }, - ], + UserInput::Image { image_url } => { + image_index += 1; + vec![ + ContentItem::InputText { + text: image_open_tag_text(), + }, + ContentItem::InputImage { image_url }, + ContentItem::InputText { + text: image_close_tag_text(), + }, + ] + } UserInput::LocalImage { path } => { image_index += 1; local_image_content_items_with_label_number(&path, Some(image_index)) @@ -1609,6 +1612,61 @@ mod tests { Ok(()) } + #[test] + fn mixed_remote_and_local_images_share_label_sequence() -> Result<()> { + let image_url = "data:image/png;base64,abc".to_string(); + let dir = tempdir()?; + let local_path = dir.path().join("local.png"); + let png_bytes = include_bytes!( + "../../core/src/skills/assets/samples/skill-creator/assets/skill-creator.png" + ); + std::fs::write(&local_path, png_bytes.as_slice())?; + + let item = ResponseInputItem::from(vec![ + UserInput::Image { + image_url: image_url.clone(), + }, + UserInput::LocalImage { path: local_path }, + ]); + + match item { + ResponseInputItem::Message { content, .. } => { + assert_eq!( + content.first(), + Some(&ContentItem::InputText { + text: image_open_tag_text(), + }) + ); + assert_eq!(content.get(1), Some(&ContentItem::InputImage { image_url })); + assert_eq!( + content.get(2), + Some(&ContentItem::InputText { + text: image_close_tag_text(), + }) + ); + assert_eq!( + content.get(3), + Some(&ContentItem::InputText { + text: local_image_open_tag_text(2), + }) + ); + assert!(matches!( + content.get(4), + Some(ContentItem::InputImage { .. }) + )); + assert_eq!( + content.get(5), + Some(&ContentItem::InputText { + text: image_close_tag_text(), + }) + ); + } + other => panic!("expected message response but got {other:?}"), + } + + Ok(()) + } + #[test] fn local_image_read_error_adds_placeholder() -> Result<()> { let dir = tempdir()?; diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index 599b2cf91..7bd105695 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -2862,6 +2862,7 @@ impl App { #[cfg(test)] mod tests { use super::*; + use crate::app_backtrack::BacktrackSelection; use crate::app_backtrack::BacktrackState; use crate::app_backtrack::user_count; use crate::chatwidget::tests::make_chatwidget_manual_with_sender; @@ -2884,6 +2885,8 @@ mod tests { use codex_otel::OtelManager; use codex_protocol::ThreadId; use codex_protocol::user_input::TextElement; + use codex_protocol::user_input::UserInput; + use crossterm::event::KeyModifiers; use insta::assert_snapshot; use pretty_assertions::assert_eq; use ratatui::prelude::Line; @@ -3427,12 +3430,14 @@ mod tests { let user_cell = |text: &str, text_elements: Vec, - local_image_paths: Vec| + local_image_paths: Vec, + remote_image_urls: Vec| -> Arc { Arc::new(UserHistoryCell { message: text.to_string(), text_elements, local_image_paths, + remote_image_urls, }) as Arc }; let agent_cell = |text: &str| -> Arc { @@ -3478,17 +3483,18 @@ mod tests { // and an edited turn appended after a session header boundary. app.transcript_cells = vec![ make_header(true), - user_cell("first question", Vec::new(), Vec::new()), + user_cell("first question", Vec::new(), Vec::new(), Vec::new()), agent_cell("answer first"), - user_cell("follow-up", Vec::new(), Vec::new()), + user_cell("follow-up", Vec::new(), Vec::new(), Vec::new()), agent_cell("answer follow-up"), make_header(false), - user_cell("first question", Vec::new(), Vec::new()), + user_cell("first question", Vec::new(), Vec::new(), Vec::new()), agent_cell("answer first"), user_cell( &edited_text, edited_text_elements.clone(), edited_local_image_paths.clone(), + vec!["https://example.com/backtrack.png".to_string()], ), agent_cell("answer edited"), ]; @@ -3527,8 +3533,16 @@ mod tests { assert_eq!(selection.prefill, edited_text); assert_eq!(selection.text_elements, edited_text_elements); assert_eq!(selection.local_image_paths, edited_local_image_paths); + assert_eq!( + selection.remote_image_urls, + vec!["https://example.com/backtrack.png".to_string()] + ); app.apply_backtrack_rollback(selection); + assert_eq!( + app.chat_widget.remote_image_urls(), + vec!["https://example.com/backtrack.png".to_string()] + ); let mut rollback_turns = None; while let Ok(op) = op_rx.try_recv() { @@ -3540,6 +3554,104 @@ mod tests { assert_eq!(rollback_turns, Some(1)); } + #[tokio::test] + async fn backtrack_remote_image_only_selection_clears_existing_composer_draft() { + let (mut app, _app_event_rx, mut op_rx) = make_test_app_with_channels().await; + + app.transcript_cells = vec![Arc::new(UserHistoryCell { + message: "original".to_string(), + text_elements: Vec::new(), + local_image_paths: Vec::new(), + remote_image_urls: Vec::new(), + }) as Arc]; + app.chat_widget + .set_composer_text("stale draft".to_string(), Vec::new(), Vec::new()); + + let remote_image_url = "https://example.com/remote-only.png".to_string(); + app.apply_backtrack_rollback(BacktrackSelection { + nth_user_message: 0, + prefill: String::new(), + text_elements: Vec::new(), + local_image_paths: Vec::new(), + remote_image_urls: vec![remote_image_url.clone()], + }); + + assert_eq!(app.chat_widget.composer_text_with_pending(), ""); + assert_eq!(app.chat_widget.remote_image_urls(), vec![remote_image_url]); + + let mut rollback_turns = None; + while let Ok(op) = op_rx.try_recv() { + if let Op::ThreadRollback { num_turns } = op { + rollback_turns = Some(num_turns); + } + } + assert_eq!(rollback_turns, Some(1)); + } + + #[tokio::test] + async fn backtrack_resubmit_preserves_data_image_urls_in_user_turn() { + let (mut app, _app_event_rx, mut op_rx) = make_test_app_with_channels().await; + + let thread_id = ThreadId::new(); + app.chat_widget.handle_codex_event(Event { + id: String::new(), + msg: EventMsg::SessionConfigured(SessionConfiguredEvent { + session_id: thread_id, + forked_from_id: None, + thread_name: None, + model: "gpt-test".to_string(), + model_provider_id: "test-provider".to_string(), + approval_policy: AskForApproval::Never, + sandbox_policy: SandboxPolicy::new_read_only_policy(), + cwd: PathBuf::from("/home/user/project"), + reasoning_effort: None, + history_log_id: 0, + history_entry_count: 0, + initial_messages: None, + network_proxy: None, + rollout_path: Some(PathBuf::new()), + }), + }); + + let data_image_url = "data:image/png;base64,abc123".to_string(); + app.transcript_cells = vec![Arc::new(UserHistoryCell { + message: "please inspect this".to_string(), + text_elements: Vec::new(), + local_image_paths: Vec::new(), + remote_image_urls: vec![data_image_url.clone()], + }) as Arc]; + + app.apply_backtrack_rollback(BacktrackSelection { + nth_user_message: 0, + prefill: "please inspect this".to_string(), + text_elements: Vec::new(), + local_image_paths: Vec::new(), + remote_image_urls: vec![data_image_url.clone()], + }); + + app.chat_widget + .handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE)); + + let mut saw_rollback = false; + let mut submitted_items: Option> = None; + while let Ok(op) = op_rx.try_recv() { + match op { + Op::ThreadRollback { .. } => saw_rollback = true, + Op::UserTurn { items, .. } => submitted_items = Some(items), + _ => {} + } + } + + assert!(saw_rollback); + let items = submitted_items.expect("expected user turn after backtrack resubmit"); + assert!(items.iter().any(|item| { + matches!( + item, + UserInput::Image { image_url } if image_url == &data_image_url + ) + })); + } + #[tokio::test] async fn replayed_initial_messages_apply_rollback_in_queue_order() { let (mut app, mut app_event_rx, _op_rx) = make_test_app_with_channels().await; @@ -3702,6 +3814,7 @@ mod tests { message: "first".to_string(), text_elements: Vec::new(), local_image_paths: Vec::new(), + remote_image_urls: Vec::new(), }) as Arc, Arc::new(AgentMessageCell::new( vec![Line::from("after first")], @@ -3711,6 +3824,7 @@ mod tests { message: "second".to_string(), text_elements: Vec::new(), local_image_paths: Vec::new(), + remote_image_urls: Vec::new(), }) as Arc, Arc::new(AgentMessageCell::new( vec![Line::from("after second")], diff --git a/codex-rs/tui/src/app_backtrack.rs b/codex-rs/tui/src/app_backtrack.rs index 22b437ec5..b9b5be2b5 100644 --- a/codex-rs/tui/src/app_backtrack.rs +++ b/codex-rs/tui/src/app_backtrack.rs @@ -85,6 +85,8 @@ pub(crate) struct BacktrackSelection { pub(crate) text_elements: Vec, /// Local image paths associated with the selected user message. pub(crate) local_image_paths: Vec, + /// Remote image URLs associated with the selected user message. + pub(crate) remote_image_urls: Vec, } /// An in-flight rollback requested from core. @@ -207,12 +209,19 @@ impl App { let prefill = selection.prefill.clone(); let text_elements = selection.text_elements.clone(); let local_image_paths = selection.local_image_paths.clone(); + let remote_image_urls = selection.remote_image_urls.clone(); + let has_remote_image_urls = !remote_image_urls.is_empty(); self.backtrack.pending_rollback = Some(PendingBacktrackRollback { selection, thread_id: self.chat_widget.thread_id(), }); self.chat_widget.submit_op(Op::ThreadRollback { num_turns }); - if !prefill.is_empty() || !text_elements.is_empty() || !local_image_paths.is_empty() { + self.chat_widget.set_remote_image_urls(remote_image_urls); + if !prefill.is_empty() + || !text_elements.is_empty() + || !local_image_paths.is_empty() + || has_remote_image_urls + { self.chat_widget .set_composer_text(prefill, text_elements, local_image_paths); } @@ -523,7 +532,7 @@ impl App { return None; } - let (prefill, text_elements, local_image_paths) = + let (prefill, text_elements, local_image_paths, remote_image_urls) = nth_user_position(&self.transcript_cells, nth_user_message) .and_then(|idx| self.transcript_cells.get(idx)) .and_then(|cell| cell.as_any().downcast_ref::()) @@ -532,15 +541,17 @@ impl App { cell.message.clone(), cell.text_elements.clone(), cell.local_image_paths.clone(), + cell.remote_image_urls.clone(), ) }) - .unwrap_or_else(|| (String::new(), Vec::new(), Vec::new())); + .unwrap_or_else(|| (String::new(), Vec::new(), Vec::new(), Vec::new())); Some(BacktrackSelection { nth_user_message, prefill, text_elements, local_image_paths, + remote_image_urls, }) } @@ -659,6 +670,7 @@ mod tests { message: "first user".to_string(), text_elements: Vec::new(), local_image_paths: Vec::new(), + remote_image_urls: Vec::new(), }) as Arc, Arc::new(AgentMessageCell::new(vec![Line::from("assistant")], true)) as Arc, @@ -677,6 +689,7 @@ mod tests { message: "first".to_string(), text_elements: Vec::new(), local_image_paths: Vec::new(), + remote_image_urls: Vec::new(), }) as Arc, Arc::new(AgentMessageCell::new(vec![Line::from("after")], false)) as Arc, @@ -707,6 +720,7 @@ mod tests { message: "first".to_string(), text_elements: Vec::new(), local_image_paths: Vec::new(), + remote_image_urls: Vec::new(), }) as Arc, Arc::new(AgentMessageCell::new(vec![Line::from("between")], false)) as Arc, @@ -714,6 +728,7 @@ mod tests { message: "second".to_string(), text_elements: Vec::new(), local_image_paths: Vec::new(), + remote_image_urls: Vec::new(), }) as Arc, Arc::new(AgentMessageCell::new(vec![Line::from("tail")], false)) as Arc, @@ -759,6 +774,7 @@ mod tests { message: "first".to_string(), text_elements: Vec::new(), local_image_paths: Vec::new(), + remote_image_urls: Vec::new(), }) as Arc, Arc::new(AgentMessageCell::new( vec![Line::from("after first")], @@ -768,6 +784,7 @@ mod tests { message: "second".to_string(), text_elements: Vec::new(), local_image_paths: Vec::new(), + remote_image_urls: Vec::new(), }) as Arc, Arc::new(AgentMessageCell::new( vec![Line::from("after second")], @@ -795,6 +812,7 @@ mod tests { message: "first".to_string(), text_elements: Vec::new(), local_image_paths: Vec::new(), + remote_image_urls: Vec::new(), }) as Arc, Arc::new(AgentMessageCell::new(vec![Line::from("after")], false)) as Arc, diff --git a/codex-rs/tui/src/bottom_pane/chat_composer.rs b/codex-rs/tui/src/bottom_pane/chat_composer.rs index 5ef643f77..36142cbae 100644 --- a/codex-rs/tui/src/bottom_pane/chat_composer.rs +++ b/codex-rs/tui/src/bottom_pane/chat_composer.rs @@ -21,9 +21,10 @@ //! 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). +//! - Local in-session history (full text + text elements + local/remote image attachments). //! -//! When recalling a local entry, the composer rehydrates text elements and image attachments. +//! When recalling a local entry, the composer rehydrates text elements and both attachment kinds +//! (local image paths + remote image URLs). //! When recalling a persistent entry, only the text is restored. //! Recalled entries move the cursor to end-of-line so repeated Up/Down presses keep shell-like //! history traversal semantics instead of dropping to column 0. @@ -39,13 +40,33 @@ //! - Expands pending paste placeholders so element ranges align with the final text. //! - Trims whitespace and rebases text elements accordingly. //! - Expands `/prompts:` custom prompts (named or numeric args), preserving text elements. -//! - Prunes attached images so only placeholders that survive expansion are sent. +//! - Prunes local attached images so only placeholders that survive expansion are sent. +//! - Preserves remote image URLs as separate attachments even when text is empty. //! //! The numeric auto-submit path used by the slash popup performs the same pending-paste expansion //! and attachment pruning, and clears pending paste state on success. //! Slash commands with arguments (like `/plan` and `/review`) reuse the same preparation path so //! pasted content and text elements are preserved when extracting args. //! +//! # Remote Image Rows (Up/Down/Delete) +//! +//! Remote image URLs are rendered as non-editable `[Image #N]` rows above the textarea (inside the +//! same composer block). These rows represent image attachments rehydrated from app-server/backtrack +//! history; TUI users can remove them, but cannot type into that row region. +//! +//! Keyboard behavior: +//! +//! - `Up` at textarea cursor `0` enters remote-row selection at the last remote image. +//! - `Up`/`Down` move selection between remote rows. +//! - `Down` on the last row clears selection and returns control to the textarea. +//! - `Delete`/`Backspace` remove the selected remote image row. +//! +//! Placeholder numbering is unified across remote and local images: +//! +//! - Remote rows occupy `[Image #1]..[Image #M]`. +//! - Local placeholders are offset after that range (`[Image #M+1]..`). +//! - Deleting a remote row relabels local placeholders to keep numbering contiguous. +//! //! # Non-bracketed Paste Bursts //! //! On some terminals (especially on Windows), pastes arrive as a rapid sequence of @@ -107,6 +128,7 @@ use ratatui::style::Stylize; use ratatui::text::Line; use ratatui::text::Span; use ratatui::widgets::Block; +use ratatui::widgets::Paragraph; use ratatui::widgets::StatefulWidgetRef; use ratatui::widgets::WidgetRef; @@ -277,7 +299,8 @@ pub(crate) struct ChatComposer { pending_pastes: Vec<(String, String)>, large_paste_counters: HashMap, has_focus: bool, - /// Invariant: attached images are labeled `[Image #1]..[Image #N]` in vec order. + /// Invariant: attached images are labeled in vec order as + /// `[Image #M+1]..[Image #N]`, where `M` is the number of remote images. attached_images: Vec, placeholder_text: String, is_task_running: bool, @@ -291,6 +314,10 @@ pub(crate) struct ChatComposer { custom_prompts: Vec, footer_mode: FooterMode, footer_hint_override: Option>, + remote_image_urls: Vec, + /// Tracks keyboard selection for the remote-image rows so Up/Down + Delete/Backspace + /// can highlight and remove remote attachments from the composer UI. + selected_remote_image_index: Option, footer_flash: Option, context_window_percent: Option, context_window_used_tokens: Option, @@ -390,6 +417,8 @@ impl ChatComposer { custom_prompts: Vec::new(), footer_mode: FooterMode::ComposerEmpty, footer_hint_override: None, + remote_image_urls: Vec::new(), + selected_remote_image_index: None, footer_flash: None, context_window_percent: None, context_window_used_tokens: None, @@ -492,7 +521,7 @@ impl ChatComposer { pub fn set_windows_degraded_sandbox_active(&mut self, enabled: bool) { self.windows_degraded_sandbox_active = enabled; } - fn layout_areas(&self, area: Rect) -> [Rect; 3] { + fn layout_areas(&self, area: Rect) -> [Rect; 4] { let footer_props = self.footer_props(); let footer_hint_height = self .custom_footer_height() @@ -511,8 +540,24 @@ impl ChatComposer { }; let [composer_rect, popup_rect] = Layout::vertical([Constraint::Min(3), popup_constraint]).areas(area); - let textarea_rect = composer_rect.inset(Insets::tlbr(1, LIVE_PREFIX_COLS, 1, 1)); - [composer_rect, textarea_rect, popup_rect] + let mut textarea_rect = composer_rect.inset(Insets::tlbr(1, LIVE_PREFIX_COLS, 1, 1)); + let remote_images_height = self + .remote_images_lines(textarea_rect.width) + .len() + .try_into() + .unwrap_or(u16::MAX) + .min(textarea_rect.height.saturating_sub(1)); + let remote_images_separator = u16::from(remote_images_height > 0); + let consumed = remote_images_height.saturating_add(remote_images_separator); + let remote_images_rect = Rect { + x: textarea_rect.x, + y: textarea_rect.y, + width: textarea_rect.width, + height: remote_images_height, + }; + textarea_rect.y = textarea_rect.y.saturating_add(consumed); + textarea_rect.height = textarea_rect.height.saturating_sub(consumed); + [composer_rect, remote_images_rect, textarea_rect, popup_rect] } fn footer_spacing(footer_hint_height: u16) -> u16 { @@ -523,9 +568,11 @@ impl ChatComposer { } } - /// Returns true if the composer currently contains no user input. + /// Returns true if the composer currently contains no user-entered input. pub(crate) fn is_empty(&self) -> bool { self.textarea.is_empty() + && self.attached_images.is_empty() + && self.remote_image_urls.is_empty() } /// Record the history metadata advertised by `SessionConfiguredEvent` so @@ -649,8 +696,8 @@ impl ChatComposer { /// Replace the composer content with text from an external editor. /// Clears pending paste placeholders and keeps only attachments whose /// placeholder labels still appear in the new text. Image placeholders - /// are renumbered to `[Image #1]..[Image #N]`. Cursor is placed at the end - /// after rebuilding elements. + /// are renumbered to `[Image #M+1]..[Image #N]` (where `M` is the number of + /// remote images). Cursor is placed at the end after rebuilding elements. pub(crate) fn apply_external_edit(&mut self, text: String) { self.pending_pastes.clear(); @@ -712,7 +759,8 @@ impl ChatComposer { self.textarea.insert_str(&text[idx..]); } - // Keep image placeholders normalized to [Image #1].. in attachment order. + // Keep local image placeholders normalized in attachment order after the + // remote-image prefix. self.relabel_attached_images_and_update_placeholders(); self.textarea.set_cursor(self.textarea.text().len()); self.sync_popups(); @@ -746,6 +794,25 @@ impl ChatComposer { self.footer_hint_override = items; } + pub(crate) fn set_remote_image_urls(&mut self, urls: Vec) { + self.remote_image_urls = urls; + self.selected_remote_image_index = None; + self.relabel_attached_images_and_update_placeholders(); + self.sync_popups(); + } + + pub(crate) fn remote_image_urls(&self) -> Vec { + self.remote_image_urls.clone() + } + + pub(crate) fn take_remote_image_urls(&mut self) -> Vec { + let urls = std::mem::take(&mut self.remote_image_urls); + self.selected_remote_image_index = None; + self.relabel_attached_images_and_update_placeholders(); + self.sync_popups(); + urls + } + #[cfg(test)] pub(crate) fn show_footer_flash(&mut self, line: Line<'static>, duration: Duration) { let expires_at = Instant::now() @@ -807,20 +874,15 @@ impl ChatComposer { self.textarea.set_text_with_elements(&text, &text_elements); - let image_placeholders: HashSet = text_elements - .iter() - .filter_map(|elem| elem.placeholder(&text).map(str::to_string)) - .collect(); for (idx, path) in local_image_paths.into_iter().enumerate() { - let placeholder = local_image_label_text(idx + 1); - if image_placeholders.contains(&placeholder) { - self.attached_images - .push(AttachedImage { placeholder, path }); - } + let placeholder = local_image_label_text(self.remote_image_urls.len() + idx + 1); + self.attached_images + .push(AttachedImage { placeholder, path }); } self.bind_mentions_from_snapshot(mention_bindings); - + self.relabel_attached_images_and_update_placeholders(); + self.selected_remote_image_index = None; self.textarea.set_cursor(0); self.sync_popups(); } @@ -848,13 +910,17 @@ impl ChatComposer { .map(|img| img.path.clone()) .collect(); let pending_pastes = std::mem::take(&mut self.pending_pastes); + let remote_image_urls = self.remote_image_urls.clone(); let mention_bindings = self.snapshot_mention_bindings(); self.set_text_content(String::new(), Vec::new(), Vec::new()); + self.remote_image_urls.clear(); + self.selected_remote_image_index = None; self.history.reset_navigation(); self.history.record_local_submission(HistoryEntry { text: previous.clone(), text_elements, local_image_paths, + remote_image_urls, mention_bindings, pending_pastes, }); @@ -878,9 +944,11 @@ impl ChatComposer { text, text_elements, local_image_paths, + remote_image_urls, mention_bindings, pending_pastes, } = entry; + self.set_remote_image_urls(remote_image_urls); self.set_text_content_with_mention_bindings( text, text_elements, @@ -935,7 +1003,7 @@ impl ChatComposer { /// Insert an attachment placeholder and track it for the next submission. pub fn attach_image(&mut self, path: PathBuf) { - let image_number = self.attached_images.len() + 1; + let image_number = self.remote_image_urls.len() + self.attached_images.len() + 1; let placeholder = local_image_label_text(image_number); // Insert as an element to match large paste placeholder behavior: // styled distinctly and treated atomically for cursor/mutations. @@ -2091,11 +2159,15 @@ impl ChatComposer { // Custom prompt expansion can remove or rewrite image placeholders, so prune any // attachments that no longer have a corresponding placeholder in the expanded text. self.prune_attached_images_for_submission(&text, &text_elements); - if text.is_empty() && self.attached_images.is_empty() { + if text.is_empty() && self.attached_images.is_empty() && self.remote_image_urls.is_empty() { return None; } self.recent_submission_mention_bindings = original_mention_bindings.clone(); - if record_history && (!text.is_empty() || !self.attached_images.is_empty()) { + if record_history + && (!text.is_empty() + || !self.attached_images.is_empty() + || !self.remote_image_urls.is_empty()) + { let local_image_paths = self .attached_images .iter() @@ -2105,6 +2177,7 @@ impl ChatComposer { text: text.clone(), text_elements: text_elements.clone(), local_image_paths, + remote_image_urls: self.remote_image_urls.clone(), mention_bindings: original_mention_bindings, pending_pastes: Vec::new(), }); @@ -2345,8 +2418,95 @@ impl ChatComposer { .collect() } + fn remote_images_lines(&self, _width: u16) -> Vec> { + self.remote_image_urls + .iter() + .enumerate() + .map(|(idx, _)| { + let label = local_image_label_text(idx + 1); + if self.selected_remote_image_index == Some(idx) { + label.cyan().reversed().into() + } else { + label.cyan().into() + } + }) + .collect() + } + + fn clear_remote_image_selection(&mut self) { + self.selected_remote_image_index = None; + } + + fn remove_selected_remote_image(&mut self, selected_index: usize) { + if selected_index >= self.remote_image_urls.len() { + self.clear_remote_image_selection(); + return; + } + self.remote_image_urls.remove(selected_index); + self.selected_remote_image_index = if self.remote_image_urls.is_empty() { + None + } else { + Some(selected_index.min(self.remote_image_urls.len() - 1)) + }; + self.relabel_attached_images_and_update_placeholders(); + self.sync_popups(); + } + + fn handle_remote_image_selection_key( + &mut self, + key_event: &KeyEvent, + ) -> Option<(InputResult, bool)> { + if self.remote_image_urls.is_empty() + || key_event.modifiers != KeyModifiers::NONE + || key_event.kind != KeyEventKind::Press + { + return None; + } + + match key_event.code { + KeyCode::Up => { + if let Some(selected) = self.selected_remote_image_index { + self.selected_remote_image_index = Some(selected.saturating_sub(1)); + Some((InputResult::None, true)) + } else if self.textarea.cursor() == 0 { + self.selected_remote_image_index = Some(self.remote_image_urls.len() - 1); + Some((InputResult::None, true)) + } else { + None + } + } + KeyCode::Down => { + if let Some(selected) = self.selected_remote_image_index { + if selected + 1 < self.remote_image_urls.len() { + self.selected_remote_image_index = Some(selected + 1); + } else { + self.clear_remote_image_selection(); + } + Some((InputResult::None, true)) + } else { + None + } + } + KeyCode::Delete | KeyCode::Backspace => { + if let Some(selected) = self.selected_remote_image_index { + self.remove_selected_remote_image(selected); + Some((InputResult::None, true)) + } else { + None + } + } + _ => None, + } + } + /// Handle key event when no popup is visible. fn handle_key_event_without_popup(&mut self, key_event: KeyEvent) -> (InputResult, bool) { + if let Some((result, redraw)) = self.handle_remote_image_selection_key(&key_event) { + return (result, redraw); + } + if self.selected_remote_image_index.is_some() { + self.clear_remote_image_selection(); + } if self.handle_shortcut_overlay_key(&key_event) { return (InputResult::None, true); } @@ -2569,7 +2729,10 @@ impl ChatComposer { // For non-char inputs (or after flushing), handle normally. // Track element removals so we can drop any corresponding placeholders without scanning // the full text. (Placeholders are atomic elements; when deleted, the element disappears.) - let elements_before = if self.pending_pastes.is_empty() && self.attached_images.is_empty() { + let elements_before = if self.pending_pastes.is_empty() + && self.attached_images.is_empty() + && self.remote_image_urls.is_empty() + { None } else { Some(self.textarea.element_payloads()) @@ -2632,7 +2795,7 @@ impl ChatComposer { fn relabel_attached_images_and_update_placeholders(&mut self) { for idx in 0..self.attached_images.len() { - let expected = local_image_label_text(idx + 1); + let expected = local_image_label_text(self.remote_image_urls.len() + idx + 1); let current = self.attached_images[idx].placeholder.clone(); if current == expected { continue; @@ -3259,11 +3422,11 @@ fn find_next_mention_token_range(text: &str, token: &str, from: usize) -> Option impl Renderable for ChatComposer { fn cursor_pos(&self, area: Rect) -> Option<(u16, u16)> { - if !self.input_enabled { + if !self.input_enabled || self.selected_remote_image_index.is_some() { return None; } - let [_, textarea_rect, _] = self.layout_areas(area); + let [_, _, textarea_rect, _] = self.layout_areas(area); let state = *self.textarea_state.borrow(); self.textarea.cursor_pos_with_state(textarea_rect, state) } @@ -3276,8 +3439,16 @@ impl Renderable for ChatComposer { let footer_spacing = Self::footer_spacing(footer_hint_height); let footer_total_height = footer_hint_height + footer_spacing; const COLS_WITH_MARGIN: u16 = LIVE_PREFIX_COLS + 1; - self.textarea - .desired_height(width.saturating_sub(COLS_WITH_MARGIN)) + let inner_width = width.saturating_sub(COLS_WITH_MARGIN); + let remote_images_height: u16 = self + .remote_images_lines(inner_width) + .len() + .try_into() + .unwrap_or(u16::MAX); + let remote_images_separator = u16::from(remote_images_height > 0); + self.textarea.desired_height(inner_width) + + remote_images_height + + remote_images_separator + 2 + match &self.active_popup { ActivePopup::None => footer_total_height, @@ -3294,7 +3465,8 @@ impl Renderable for ChatComposer { impl ChatComposer { pub(crate) fn render_with_mask(&self, area: Rect, buf: &mut Buffer, mask_char: Option) { - let [composer_rect, textarea_rect, popup_rect] = self.layout_areas(area); + let [composer_rect, remote_images_rect, textarea_rect, popup_rect] = + self.layout_areas(area); match &self.active_popup { ActivePopup::Command(popup) => { popup.render_ref(popup_rect, buf); @@ -3515,6 +3687,11 @@ impl ChatComposer { } let style = user_message_style(); Block::default().style(style).render_ref(composer_rect, buf); + if !remote_images_rect.is_empty() { + Paragraph::new(self.remote_images_lines(remote_images_rect.width)) + .style(style) + .render_ref(remote_images_rect, buf); + } if !textarea_rect.is_empty() { let prompt = if self.input_enabled { "›".bold() @@ -4230,6 +4407,85 @@ mod tests { assert_eq!(composer.textarea.element_payloads(), vec![placeholder]); } + #[test] + fn clear_for_ctrl_c_preserves_remote_offset_image_labels() { + 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 remote_image_url = "https://example.com/one.png".to_string(); + composer.set_remote_image_urls(vec![remote_image_url.clone()]); + let text = "[Image #2] draft".to_string(); + let text_elements = vec![TextElement::new( + (0.."[Image #2]".len()).into(), + Some("[Image #2]".to_string()), + )]; + let local_image_path = PathBuf::from("/tmp/local-draft.png"); + composer.set_text_content(text, text_elements, vec![local_image_path.clone()]); + let expected_text = composer.current_text(); + let expected_elements = composer.text_elements(); + assert_eq!(expected_text, "[Image #2] draft"); + assert_eq!( + expected_elements[0].placeholder(&expected_text), + Some("[Image #2]") + ); + + assert_eq!(composer.clear_for_ctrl_c(), Some(expected_text.clone())); + + assert_eq!( + composer.history.navigate_up(&composer.app_event_tx), + Some(HistoryEntry::with_pending_and_remote( + expected_text, + expected_elements, + vec![local_image_path], + Vec::new(), + vec![remote_image_url], + )) + ); + } + + #[test] + fn apply_history_entry_preserves_local_placeholders_after_remote_prefix() { + 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 remote_image_url = "https://example.com/one.png".to_string(); + let local_image_path = PathBuf::from("/tmp/local-draft.png"); + composer.apply_history_entry(HistoryEntry::with_pending_and_remote( + "[Image #2] draft".to_string(), + vec![TextElement::new( + (0.."[Image #2]".len()).into(), + Some("[Image #2]".to_string()), + )], + vec![local_image_path.clone()], + Vec::new(), + vec![remote_image_url.clone()], + )); + + let restored_text = composer.current_text(); + assert_eq!(restored_text, "[Image #2] draft"); + let restored_elements = composer.text_elements(); + assert_eq!(restored_elements.len(), 1); + assert_eq!( + restored_elements[0].placeholder(&restored_text), + Some("[Image #2]") + ); + assert_eq!(composer.local_image_paths(), vec![local_image_path]); + assert_eq!(composer.remote_image_urls(), vec![remote_image_url]); + } + /// 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] @@ -5101,6 +5357,43 @@ mod tests { }); } + #[test] + fn remote_image_rows_snapshots() { + use crossterm::event::KeyCode; + use crossterm::event::KeyEvent; + use crossterm::event::KeyModifiers; + + snapshot_composer_state("remote_image_rows", false, |composer| { + composer.set_remote_image_urls(vec![ + "https://example.com/one.png".to_string(), + "https://example.com/two.png".to_string(), + ]); + composer.set_text_content("describe these".to_string(), Vec::new(), Vec::new()); + }); + + snapshot_composer_state("remote_image_rows_selected", false, |composer| { + composer.set_remote_image_urls(vec![ + "https://example.com/one.png".to_string(), + "https://example.com/two.png".to_string(), + ]); + composer.set_text_content("describe these".to_string(), Vec::new(), Vec::new()); + composer.textarea.set_cursor(0); + let _ = composer.handle_key_event(KeyEvent::new(KeyCode::Up, KeyModifiers::NONE)); + }); + + snapshot_composer_state("remote_image_rows_after_delete_first", false, |composer| { + composer.set_remote_image_urls(vec![ + "https://example.com/one.png".to_string(), + "https://example.com/two.png".to_string(), + ]); + composer.set_text_content("describe these".to_string(), Vec::new(), Vec::new()); + composer.textarea.set_cursor(0); + let _ = composer.handle_key_event(KeyEvent::new(KeyCode::Up, KeyModifiers::NONE)); + let _ = composer.handle_key_event(KeyEvent::new(KeyCode::Up, KeyModifiers::NONE)); + let _ = composer.handle_key_event(KeyEvent::new(KeyCode::Delete, KeyModifiers::NONE)); + }); + } + #[test] fn slash_popup_model_first_for_mo_ui() { use ratatui::Terminal; @@ -5993,7 +6286,7 @@ mod tests { } #[test] - fn history_navigation_restores_image_attachments() { + fn history_navigation_restores_remote_and_local_image_attachments() { let (tx, _rx) = unbounded_channel::(); let sender = AppEventSender::new(tx); let mut composer = ChatComposer::new( @@ -6004,6 +6297,8 @@ mod tests { false, ); composer.set_steer_enabled(true); + let remote_image_url = "https://example.com/remote.png".to_string(); + composer.set_remote_image_urls(vec![remote_image_url.clone()]); let path = PathBuf::from("/tmp/image1.png"); composer.attach_image(path.clone()); @@ -6011,17 +6306,50 @@ mod tests { composer.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE)); assert!(matches!(result, InputResult::Submitted { .. })); + let _ = composer.take_remote_image_urls(); 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]"); + assert_eq!(text, "[Image #2]"); let text_elements = composer.text_elements(); assert_eq!(text_elements.len(), 1); - assert_eq!(text_elements[0].placeholder(&text), Some("[Image #1]")); + assert_eq!(text_elements[0].placeholder(&text), Some("[Image #2]")); assert_eq!(composer.local_image_paths(), vec![path]); - assert_eq!(composer.textarea.cursor(), composer.textarea.text().len()); + assert_eq!(composer.remote_image_urls(), vec![remote_image_url]); + } + + #[test] + fn history_navigation_restores_remote_only_submissions() { + 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 remote_image_urls = vec![ + "https://example.com/one.png".to_string(), + "https://example.com/two.png".to_string(), + ]; + composer.set_remote_image_urls(remote_image_urls.clone()); + + let (submitted_text, submitted_elements) = composer + .prepare_submission_text(true) + .expect("remote-only submission should be prepared"); + assert_eq!(submitted_text, ""); + assert!(submitted_elements.is_empty()); + + let _ = composer.take_remote_image_urls(); + composer.set_text_content(String::new(), Vec::new(), Vec::new()); + + let _ = composer.handle_key_event(KeyEvent::new(KeyCode::Up, KeyModifiers::NONE)); + assert_eq!(composer.current_text(), ""); + assert!(composer.text_elements().is_empty()); + assert_eq!(composer.remote_image_urls(), remote_image_urls); } #[test] @@ -7903,6 +8231,141 @@ mod tests { assert_eq!(composer.attached_images.len(), 1); } + #[test] + fn remote_images_do_not_modify_textarea_text_or_elements() { + 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_remote_image_urls(vec![ + "https://example.com/one.png".to_string(), + "https://example.com/two.png".to_string(), + ]); + + assert_eq!(composer.current_text(), ""); + assert_eq!(composer.text_elements(), Vec::::new()); + } + + #[test] + fn attach_image_after_remote_prefix_uses_offset_label() { + 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_remote_image_urls(vec![ + "https://example.com/one.png".to_string(), + "https://example.com/two.png".to_string(), + ]); + composer.attach_image(PathBuf::from("/tmp/local.png")); + + assert_eq!(composer.attached_images[0].placeholder, "[Image #3]"); + assert_eq!(composer.current_text(), "[Image #3]"); + } + + #[test] + fn prepare_submission_keeps_remote_offset_local_placeholder_numbering() { + 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_remote_image_urls(vec!["https://example.com/one.png".to_string()]); + let base_text = "[Image #2] hello".to_string(); + let base_elements = vec![TextElement::new( + (0.."[Image #2]".len()).into(), + Some("[Image #2]".to_string()), + )]; + composer.set_text_content( + base_text, + base_elements, + vec![PathBuf::from("/tmp/local.png")], + ); + + let (submitted_text, submitted_elements) = composer + .prepare_submission_text(true) + .expect("remote+local submission should be generated"); + assert_eq!(submitted_text, "[Image #2] hello"); + assert_eq!( + submitted_elements, + vec![TextElement::new( + (0.."[Image #2]".len()).into(), + Some("[Image #2]".to_string()) + )] + ); + } + + #[test] + fn prepare_submission_with_only_remote_images_returns_empty_text() { + 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_remote_image_urls(vec!["https://example.com/one.png".to_string()]); + let (submitted_text, submitted_elements) = composer + .prepare_submission_text(true) + .expect("remote-only submission should be generated"); + assert_eq!(submitted_text, ""); + assert!(submitted_elements.is_empty()); + } + + #[test] + fn delete_selected_remote_image_relabels_local_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, + ); + + composer.set_remote_image_urls(vec![ + "https://example.com/one.png".to_string(), + "https://example.com/two.png".to_string(), + ]); + composer.attach_image(PathBuf::from("/tmp/local.png")); + composer.textarea.set_cursor(0); + + let _ = composer.handle_key_event(KeyEvent::new(KeyCode::Up, KeyModifiers::NONE)); + let _ = composer.handle_key_event(KeyEvent::new(KeyCode::Delete, KeyModifiers::NONE)); + assert_eq!( + composer.remote_image_urls(), + vec!["https://example.com/one.png".to_string()] + ); + assert_eq!(composer.current_text(), "[Image #2]"); + assert_eq!(composer.attached_images[0].placeholder, "[Image #2]"); + + let _ = composer.handle_key_event(KeyEvent::new(KeyCode::Up, KeyModifiers::NONE)); + let _ = composer.handle_key_event(KeyEvent::new(KeyCode::Delete, KeyModifiers::NONE)); + assert_eq!(composer.remote_image_urls(), Vec::::new()); + assert_eq!(composer.current_text(), "[Image #1]"); + assert_eq!(composer.attached_images[0].placeholder, "[Image #1]"); + } + #[test] fn input_disabled_ignores_keypresses_and_hides_cursor() { use crossterm::event::KeyCode; 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 e1c1d32ca..604641d20 100644 --- a/codex-rs/tui/src/bottom_pane/chat_composer_history.rs +++ b/codex-rs/tui/src/bottom_pane/chat_composer_history.rs @@ -17,6 +17,8 @@ pub(crate) struct HistoryEntry { pub(crate) text_elements: Vec, /// Local image paths captured alongside `text_elements`. pub(crate) local_image_paths: Vec, + /// Remote image URLs restored with this draft. + pub(crate) remote_image_urls: 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. @@ -30,6 +32,7 @@ impl HistoryEntry { text: decoded.text, text_elements: Vec::new(), local_image_paths: Vec::new(), + remote_image_urls: Vec::new(), mention_bindings: decoded .mentions .into_iter() @@ -53,6 +56,25 @@ impl HistoryEntry { text, text_elements, local_image_paths, + remote_image_urls: Vec::new(), + mention_bindings: Vec::new(), + pending_pastes, + } + } + + #[cfg(test)] + pub(crate) fn with_pending_and_remote( + text: String, + text_elements: Vec, + local_image_paths: Vec, + pending_pastes: Vec<(String, String)>, + remote_image_urls: Vec, + ) -> Self { + Self { + text, + text_elements, + local_image_paths, + remote_image_urls, mention_bindings: Vec::new(), pending_pastes, } @@ -70,7 +92,7 @@ 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 entries retain full draft state (text elements, image paths, pending pastes, remote image URLs). local_history: Vec, /// Cache of persistent history entries fetched on-demand (text-only). @@ -115,6 +137,7 @@ impl ChatComposerHistory { if entry.text.is_empty() && entry.text_elements.is_empty() && entry.local_image_paths.is_empty() + && entry.remote_image_urls.is_empty() && entry.mention_bindings.is_empty() && entry.pending_pastes.is_empty() { diff --git a/codex-rs/tui/src/bottom_pane/mod.rs b/codex-rs/tui/src/bottom_pane/mod.rs index 5ad79df69..4e4dabe5c 100644 --- a/codex-rs/tui/src/bottom_pane/mod.rs +++ b/codex-rs/tui/src/bottom_pane/mod.rs @@ -250,6 +250,7 @@ impl BottomPane { /// Clear pending attachments and mention bindings e.g. when a slash command doesn't submit text. pub(crate) fn drain_pending_submission_state(&mut self) { let _ = self.take_recent_submission_images_with_placeholders(); + let _ = self.take_remote_image_urls(); let _ = self.take_recent_submission_mention_bindings(); let _ = self.take_mention_bindings(); } @@ -520,6 +521,21 @@ impl BottomPane { self.request_redraw(); } + pub(crate) fn set_remote_image_urls(&mut self, urls: Vec) { + self.composer.set_remote_image_urls(urls); + self.request_redraw(); + } + + pub(crate) fn remote_image_urls(&self) -> Vec { + self.composer.remote_image_urls() + } + + pub(crate) fn take_remote_image_urls(&mut self) -> Vec { + let urls = self.composer.take_remote_image_urls(); + self.request_redraw(); + urls + } + /// Update the status indicator header (defaults to "Working") and details below it. /// /// Passing `None` clears any existing details. No-ops if the status indicator is not active. @@ -1315,6 +1331,58 @@ mod tests { ); } + #[test] + fn remote_images_render_above_composer_text() { + let (tx_raw, _rx) = unbounded_channel::(); + let tx = AppEventSender::new(tx_raw); + let mut pane = BottomPane::new(BottomPaneParams { + app_event_tx: tx, + frame_requester: FrameRequester::test_dummy(), + has_input_focus: true, + enhanced_keys_supported: false, + placeholder_text: "Ask Codex to do anything".to_string(), + disable_paste_burst: false, + animations_enabled: true, + skills: Some(Vec::new()), + }); + + pane.set_remote_image_urls(vec![ + "https://example.com/one.png".to_string(), + "data:image/png;base64,aGVsbG8=".to_string(), + ]); + + assert_eq!(pane.composer_text(), ""); + let width = 48; + let height = pane.desired_height(width); + let area = Rect::new(0, 0, width, height); + let snapshot = render_snapshot(&pane, area); + assert!(snapshot.contains("[Image #1]")); + assert!(snapshot.contains("[Image #2]")); + } + + #[test] + fn drain_pending_submission_state_clears_remote_image_urls() { + let (tx_raw, _rx) = unbounded_channel::(); + let tx = AppEventSender::new(tx_raw); + let mut pane = BottomPane::new(BottomPaneParams { + app_event_tx: tx, + frame_requester: FrameRequester::test_dummy(), + has_input_focus: true, + enhanced_keys_supported: false, + placeholder_text: "Ask Codex to do anything".to_string(), + disable_paste_burst: false, + animations_enabled: true, + skills: Some(Vec::new()), + }); + + pane.set_remote_image_urls(vec!["https://example.com/one.png".to_string()]); + assert_eq!(pane.remote_image_urls().len(), 1); + + pane.drain_pending_submission_state(); + + assert!(pane.remote_image_urls().is_empty()); + } + #[test] fn esc_with_skill_popup_does_not_interrupt_task() { let (tx_raw, mut rx) = unbounded_channel::(); diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__remote_image_rows.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__remote_image_rows.snap new file mode 100644 index 000000000..cb00c404b --- /dev/null +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__remote_image_rows.snap @@ -0,0 +1,13 @@ +--- +source: tui/src/bottom_pane/chat_composer.rs +expression: terminal.backend() +--- +" " +" [Image #1] " +" [Image #2] " +" " +"› describe these " +" " +" " +" " +" 100% context left " diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__remote_image_rows_after_delete_first.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__remote_image_rows_after_delete_first.snap new file mode 100644 index 000000000..fd3cf1f6c --- /dev/null +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__remote_image_rows_after_delete_first.snap @@ -0,0 +1,13 @@ +--- +source: tui/src/bottom_pane/chat_composer.rs +expression: terminal.backend() +--- +" " +" [Image #1] " +" " +"› describe these " +" " +" " +" " +" " +" 100% context left " diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__remote_image_rows_selected.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__remote_image_rows_selected.snap new file mode 100644 index 000000000..cb00c404b --- /dev/null +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__remote_image_rows_selected.snap @@ -0,0 +1,13 @@ +--- +source: tui/src/bottom_pane/chat_composer.rs +expression: terminal.backend() +--- +" " +" [Image #1] " +" [Image #2] " +" " +"› describe these " +" " +" " +" " +" 100% context left " diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index e36147ae7..36251c448 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -650,6 +650,12 @@ pub(crate) struct ActiveCellTranscriptKey { pub(crate) struct UserMessage { text: String, local_images: Vec, + /// Remote image attachments represented as URLs (for example data URLs) + /// provided by app-server clients. + /// + /// Unlike `local_images`, these are not created by TUI image attach/paste + /// flows. The TUI can restore and remove them while editing/backtracking. + remote_image_urls: Vec, text_elements: Vec, mention_bindings: Vec, } @@ -659,6 +665,7 @@ impl From for UserMessage { Self { text, local_images: Vec::new(), + remote_image_urls: Vec::new(), // Plain text conversion has no UI element ranges. text_elements: Vec::new(), mention_bindings: Vec::new(), @@ -671,6 +678,7 @@ impl From<&str> for UserMessage { Self { text: text.to_string(), local_images: Vec::new(), + remote_image_urls: Vec::new(), // Plain text conversion has no UI element ranges. text_elements: Vec::new(), mention_bindings: Vec::new(), @@ -698,6 +706,7 @@ pub(crate) fn create_initial_user_message( Some(UserMessage { text, local_images, + remote_image_urls: Vec::new(), text_elements, mention_bindings: Vec::new(), }) @@ -713,6 +722,7 @@ fn remap_placeholders_for_message(message: UserMessage, next_label: &mut usize) text, text_elements, local_images, + remote_image_urls, mention_bindings, } = message; if local_images.is_empty() { @@ -720,6 +730,7 @@ fn remap_placeholders_for_message(message: UserMessage, next_label: &mut usize) text, text_elements, local_images, + remote_image_urls, mention_bindings, }; } @@ -774,6 +785,7 @@ fn remap_placeholders_for_message(message: UserMessage, next_label: &mut usize) UserMessage { text: rebuilt, local_images: remapped_images, + remote_image_urls, text_elements: rebuilt_elements, mention_bindings, } @@ -1722,11 +1734,15 @@ impl ChatWidget { text: self.bottom_pane.composer_text(), text_elements: self.bottom_pane.composer_text_elements(), local_images: self.bottom_pane.composer_local_images(), + remote_image_urls: self.bottom_pane.remote_image_urls(), mention_bindings: self.bottom_pane.composer_mention_bindings(), }; let mut to_merge: Vec = self.queued_user_messages.drain(..).collect(); - if !existing_message.text.is_empty() || !existing_message.local_images.is_empty() { + if !existing_message.text.is_empty() + || !existing_message.local_images.is_empty() + || !existing_message.remote_image_urls.is_empty() + { to_merge.push(existing_message); } @@ -1734,10 +1750,15 @@ impl ChatWidget { text: String::new(), text_elements: Vec::new(), local_images: Vec::new(), + remote_image_urls: Vec::new(), mention_bindings: Vec::new(), }; let mut combined_offset = 0usize; - let mut next_image_label = 1usize; + let total_remote_images = to_merge + .iter() + .map(|message| message.remote_image_urls.len()) + .sum::(); + let mut next_image_label = total_remote_images + 1; for (idx, message) in to_merge.into_iter().enumerate() { if idx > 0 { @@ -1756,6 +1777,7 @@ impl ChatWidget { elem })); combined.local_images.extend(message.local_images); + combined.remote_image_urls.extend(message.remote_image_urls); combined.mention_bindings.extend(message.mention_bindings); } @@ -1766,10 +1788,12 @@ impl ChatWidget { let UserMessage { text, local_images, + remote_image_urls, text_elements, mention_bindings, } = user_message; let local_image_paths = local_images.into_iter().map(|img| img.path).collect(); + self.set_remote_image_urls(remote_image_urls); self.bottom_pane.set_composer_text_with_mention_bindings( text, text_elements, @@ -3102,11 +3126,14 @@ impl ChatWidget { text, text_elements, } => { + let local_images = self + .bottom_pane + .take_recent_submission_images_with_placeholders(); + let remote_image_urls = self.take_remote_image_urls(); let user_message = UserMessage { text, - local_images: self - .bottom_pane - .take_recent_submission_images_with_placeholders(), + local_images, + remote_image_urls, text_elements, mention_bindings: self .bottom_pane @@ -3127,11 +3154,14 @@ impl ChatWidget { text, text_elements, } => { + let local_images = self + .bottom_pane + .take_recent_submission_images_with_placeholders(); + let remote_image_urls = self.take_remote_image_urls(); let user_message = UserMessage { text, - local_images: self - .bottom_pane - .take_recent_submission_images_with_placeholders(), + local_images, + remote_image_urls, text_elements, mention_bindings: self .bottom_pane @@ -3517,11 +3547,14 @@ impl ChatWidget { else { return; }; + let local_images = self + .bottom_pane + .take_recent_submission_images_with_placeholders(); + let remote_image_urls = self.take_remote_image_urls(); let user_message = UserMessage { text: prepared_args, - local_images: self - .bottom_pane - .take_recent_submission_images_with_placeholders(), + local_images, + remote_image_urls, text_elements: prepared_elements, mention_bindings: self.bottom_pane.take_recent_submission_mention_bindings(), }; @@ -3671,18 +3704,22 @@ impl ChatWidget { let UserMessage { text, local_images, + remote_image_urls, text_elements, mention_bindings, } = user_message; - if text.is_empty() && local_images.is_empty() { + if text.is_empty() && local_images.is_empty() && remote_image_urls.is_empty() { return; } - if !local_images.is_empty() && !self.current_model_supports_images() { + if (!local_images.is_empty() || !remote_image_urls.is_empty()) + && !self.current_model_supports_images() + { self.restore_blocked_image_submission( text, text_elements, local_images, mention_bindings, + remote_image_urls, ); return; } @@ -3707,6 +3744,12 @@ impl ChatWidget { return; } + for image_url in &remote_image_urls { + items.push(UserInput::Image { + image_url: image_url.clone(), + }); + } + for image in &local_images { items.push(UserInput::LocalImage { path: image.path.clone(), @@ -3846,13 +3889,21 @@ impl ChatWidget { }); } - // Only show the text portion in conversation history. + // Show replayable user content in conversation history. if !text.is_empty() { let local_image_paths = local_images.into_iter().map(|img| img.path).collect(); self.add_to_history(history_cell::new_user_prompt( text, text_elements, local_image_paths, + remote_image_urls, + )); + } else if !remote_image_urls.is_empty() { + self.add_to_history(history_cell::new_user_prompt( + String::new(), + Vec::new(), + Vec::new(), + remote_image_urls, )); } @@ -3872,9 +3923,11 @@ impl ChatWidget { text_elements: Vec, local_images: Vec, mention_bindings: Vec, + remote_image_urls: Vec, ) { // Preserve the user's composed payload so they can retry after changing models. let local_image_paths = local_images.iter().map(|img| img.path.clone()).collect(); + self.set_remote_image_urls(remote_image_urls); self.bottom_pane.set_composer_text_with_mention_bindings( text, text_elements, @@ -4153,11 +4206,16 @@ impl ChatWidget { } fn on_user_message_event(&mut self, event: UserMessageEvent) { - if !event.message.trim().is_empty() { + let remote_image_urls = event.images.unwrap_or_default(); + if !event.message.trim().is_empty() + || !event.text_elements.is_empty() + || !remote_image_urls.is_empty() + { self.add_to_history(history_cell::new_user_prompt( event.message, event.text_elements, event.local_images, + remote_image_urls, )); } @@ -6650,6 +6708,7 @@ impl ChatWidget { let user_message = UserMessage { text, local_images: Vec::new(), + remote_image_urls: Vec::new(), text_elements: Vec::new(), mention_bindings: Vec::new(), }; @@ -6682,6 +6741,19 @@ impl ChatWidget { .set_composer_text(text, text_elements, local_image_paths); } + pub(crate) fn set_remote_image_urls(&mut self, remote_image_urls: Vec) { + self.bottom_pane.set_remote_image_urls(remote_image_urls); + } + + fn take_remote_image_urls(&mut self) -> Vec { + self.bottom_pane.take_remote_image_urls() + } + + #[cfg(test)] + pub(crate) fn remote_image_urls(&self) -> Vec { + self.bottom_pane.remote_image_urls() + } + pub(crate) fn show_esc_backtrack_hint(&mut self) { self.bottom_pane.show_esc_backtrack_hint(); } diff --git a/codex-rs/tui/src/chatwidget/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index f0af50415..cd03b9899 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -250,16 +250,174 @@ async fn replayed_user_message_preserves_text_elements_and_local_images() { cell.message.clone(), cell.text_elements.clone(), cell.local_image_paths.clone(), + cell.remote_image_urls.clone(), )); break; } } - let (stored_message, stored_elements, stored_images) = + let (stored_message, stored_elements, stored_images, stored_remote_image_urls) = user_cell.expect("expected a replayed user history cell"); assert_eq!(stored_message, message); assert_eq!(stored_elements, text_elements); assert_eq!(stored_images, local_images); + assert!(stored_remote_image_urls.is_empty()); +} + +#[tokio::test] +async fn replayed_user_message_preserves_remote_image_urls() { + let (mut chat, mut rx, _ops) = make_chatwidget_manual(None).await; + + let message = "replayed with remote image".to_string(); + let remote_image_urls = vec!["https://example.com/image.png".to_string()]; + + let conversation_id = ThreadId::new(); + let rollout_file = NamedTempFile::new().unwrap(); + let configured = codex_core::protocol::SessionConfiguredEvent { + session_id: conversation_id, + forked_from_id: None, + thread_name: None, + model: "test-model".to_string(), + model_provider_id: "test-provider".to_string(), + approval_policy: AskForApproval::Never, + sandbox_policy: SandboxPolicy::new_read_only_policy(), + cwd: PathBuf::from("/home/user/project"), + reasoning_effort: Some(ReasoningEffortConfig::default()), + history_log_id: 0, + history_entry_count: 0, + initial_messages: Some(vec![EventMsg::UserMessage(UserMessageEvent { + message: message.clone(), + images: Some(remote_image_urls.clone()), + text_elements: Vec::new(), + local_images: Vec::new(), + })]), + network_proxy: None, + rollout_path: Some(rollout_file.path().to_path_buf()), + }; + + chat.handle_codex_event(Event { + id: "initial".into(), + msg: EventMsg::SessionConfigured(configured), + }); + + let mut user_cell = None; + while let Ok(ev) = rx.try_recv() { + if let AppEvent::InsertHistoryCell(cell) = ev + && let Some(cell) = cell.as_any().downcast_ref::() + { + user_cell = Some(( + cell.message.clone(), + cell.local_image_paths.clone(), + cell.remote_image_urls.clone(), + )); + break; + } + } + + let (stored_message, stored_local_images, stored_remote_image_urls) = + user_cell.expect("expected a replayed user history cell"); + assert_eq!(stored_message, message); + assert!(stored_local_images.is_empty()); + assert_eq!(stored_remote_image_urls, remote_image_urls); +} + +#[tokio::test] +async fn replayed_user_message_with_only_remote_images_renders_history_cell() { + let (mut chat, mut rx, _ops) = make_chatwidget_manual(None).await; + + let remote_image_urls = vec!["https://example.com/remote-only.png".to_string()]; + + let conversation_id = ThreadId::new(); + let rollout_file = NamedTempFile::new().unwrap(); + let configured = codex_core::protocol::SessionConfiguredEvent { + session_id: conversation_id, + forked_from_id: None, + thread_name: None, + model: "test-model".to_string(), + model_provider_id: "test-provider".to_string(), + approval_policy: AskForApproval::Never, + sandbox_policy: SandboxPolicy::new_read_only_policy(), + cwd: PathBuf::from("/home/user/project"), + reasoning_effort: Some(ReasoningEffortConfig::default()), + history_log_id: 0, + history_entry_count: 0, + initial_messages: Some(vec![EventMsg::UserMessage(UserMessageEvent { + message: String::new(), + images: Some(remote_image_urls.clone()), + text_elements: Vec::new(), + local_images: Vec::new(), + })]), + network_proxy: None, + rollout_path: Some(rollout_file.path().to_path_buf()), + }; + + chat.handle_codex_event(Event { + id: "initial".into(), + msg: EventMsg::SessionConfigured(configured), + }); + + let mut user_cell = None; + while let Ok(ev) = rx.try_recv() { + if let AppEvent::InsertHistoryCell(cell) = ev + && let Some(cell) = cell.as_any().downcast_ref::() + { + user_cell = Some((cell.message.clone(), cell.remote_image_urls.clone())); + break; + } + } + + let (stored_message, stored_remote_image_urls) = + user_cell.expect("expected a replayed remote-image-only user history cell"); + assert!(stored_message.is_empty()); + assert_eq!(stored_remote_image_urls, remote_image_urls); +} + +#[tokio::test] +async fn replayed_user_message_with_only_local_images_does_not_render_history_cell() { + let (mut chat, mut rx, _ops) = make_chatwidget_manual(None).await; + + let local_images = vec![PathBuf::from("/tmp/replay-local-only.png")]; + + let conversation_id = ThreadId::new(); + let rollout_file = NamedTempFile::new().unwrap(); + let configured = codex_core::protocol::SessionConfiguredEvent { + session_id: conversation_id, + forked_from_id: None, + thread_name: None, + model: "test-model".to_string(), + model_provider_id: "test-provider".to_string(), + approval_policy: AskForApproval::Never, + sandbox_policy: SandboxPolicy::new_read_only_policy(), + cwd: PathBuf::from("/home/user/project"), + reasoning_effort: Some(ReasoningEffortConfig::default()), + history_log_id: 0, + history_entry_count: 0, + initial_messages: Some(vec![EventMsg::UserMessage(UserMessageEvent { + message: String::new(), + images: None, + text_elements: Vec::new(), + local_images, + })]), + network_proxy: None, + rollout_path: Some(rollout_file.path().to_path_buf()), + }; + + chat.handle_codex_event(Event { + id: "initial".into(), + msg: EventMsg::SessionConfigured(configured), + }); + + let mut found_user_history_cell = false; + while let Ok(ev) = rx.try_recv() { + if let AppEvent::InsertHistoryCell(cell) = ev + && cell.as_any().downcast_ref::().is_some() + { + found_user_history_cell = true; + break; + } + } + + assert!(!found_user_history_cell); } #[tokio::test] @@ -394,16 +552,288 @@ async fn submission_preserves_text_elements_and_local_images() { cell.message.clone(), cell.text_elements.clone(), cell.local_image_paths.clone(), + cell.remote_image_urls.clone(), )); break; } } - let (stored_message, stored_elements, stored_images) = + let (stored_message, stored_elements, stored_images, stored_remote_image_urls) = user_cell.expect("expected submitted user history cell"); assert_eq!(stored_message, text); assert_eq!(stored_elements, text_elements); assert_eq!(stored_images, local_images); + assert!(stored_remote_image_urls.is_empty()); +} + +#[tokio::test] +async fn submission_with_remote_and_local_images_keeps_local_placeholder_numbering() { + let (mut chat, mut rx, mut op_rx) = make_chatwidget_manual(None).await; + + let conversation_id = ThreadId::new(); + let rollout_file = NamedTempFile::new().unwrap(); + let configured = codex_core::protocol::SessionConfiguredEvent { + session_id: conversation_id, + forked_from_id: None, + thread_name: None, + model: "test-model".to_string(), + model_provider_id: "test-provider".to_string(), + approval_policy: AskForApproval::Never, + sandbox_policy: SandboxPolicy::new_read_only_policy(), + cwd: PathBuf::from("/home/user/project"), + reasoning_effort: Some(ReasoningEffortConfig::default()), + history_log_id: 0, + history_entry_count: 0, + initial_messages: None, + network_proxy: None, + rollout_path: Some(rollout_file.path().to_path_buf()), + }; + chat.handle_codex_event(Event { + id: "initial".into(), + msg: EventMsg::SessionConfigured(configured), + }); + drain_insert_history(&mut rx); + + let remote_url = "https://example.com/remote.png".to_string(); + chat.set_remote_image_urls(vec![remote_url.clone()]); + + let placeholder = "[Image #2]"; + let text = format!("{placeholder} submit mixed"); + let text_elements = vec![TextElement::new( + (0..placeholder.len()).into(), + Some(placeholder.to_string()), + )]; + let local_images = vec![PathBuf::from("/tmp/submitted-mixed.png")]; + + chat.bottom_pane + .set_composer_text(text.clone(), text_elements.clone(), local_images.clone()); + assert_eq!(chat.bottom_pane.composer_text(), "[Image #2] submit mixed"); + chat.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE)); + + let items = match next_submit_op(&mut op_rx) { + Op::UserTurn { items, .. } => items, + other => panic!("expected Op::UserTurn, got {other:?}"), + }; + assert_eq!(items.len(), 3); + assert_eq!( + items[0], + UserInput::Image { + image_url: remote_url.clone(), + } + ); + assert_eq!( + items[1], + UserInput::LocalImage { + path: local_images[0].clone(), + } + ); + assert_eq!( + items[2], + UserInput::Text { + text: text.clone(), + text_elements: text_elements.clone(), + } + ); + assert_eq!(text_elements[0].placeholder(&text), Some("[Image #2]")); + + let mut user_cell = None; + while let Ok(ev) = rx.try_recv() { + if let AppEvent::InsertHistoryCell(cell) = ev + && let Some(cell) = cell.as_any().downcast_ref::() + { + user_cell = Some(( + cell.message.clone(), + cell.text_elements.clone(), + cell.local_image_paths.clone(), + cell.remote_image_urls.clone(), + )); + break; + } + } + + let (stored_message, stored_elements, stored_images, stored_remote_image_urls) = + user_cell.expect("expected submitted user history cell"); + assert_eq!(stored_message, text); + assert_eq!(stored_elements, text_elements); + assert_eq!(stored_images, local_images); + assert_eq!(stored_remote_image_urls, vec![remote_url]); +} + +#[tokio::test] +async fn enter_with_only_remote_images_submits_user_turn() { + let (mut chat, mut rx, mut op_rx) = make_chatwidget_manual(None).await; + + let conversation_id = ThreadId::new(); + let rollout_file = NamedTempFile::new().unwrap(); + let configured = codex_core::protocol::SessionConfiguredEvent { + session_id: conversation_id, + forked_from_id: None, + thread_name: None, + model: "test-model".to_string(), + model_provider_id: "test-provider".to_string(), + approval_policy: AskForApproval::Never, + sandbox_policy: SandboxPolicy::new_read_only_policy(), + cwd: PathBuf::from("/home/user/project"), + reasoning_effort: Some(ReasoningEffortConfig::default()), + history_log_id: 0, + history_entry_count: 0, + initial_messages: None, + network_proxy: None, + rollout_path: Some(rollout_file.path().to_path_buf()), + }; + chat.handle_codex_event(Event { + id: "initial".into(), + msg: EventMsg::SessionConfigured(configured), + }); + drain_insert_history(&mut rx); + + let remote_url = "https://example.com/remote-only.png".to_string(); + chat.set_remote_image_urls(vec![remote_url.clone()]); + assert_eq!(chat.bottom_pane.composer_text(), ""); + + chat.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE)); + + let items = match next_submit_op(&mut op_rx) { + Op::UserTurn { items, .. } => items, + other => panic!("expected Op::UserTurn, got {other:?}"), + }; + assert_eq!( + items, + vec![UserInput::Image { + image_url: remote_url.clone(), + }] + ); + assert!(chat.remote_image_urls().is_empty()); + + let mut user_cell = None; + while let Ok(ev) = rx.try_recv() { + if let AppEvent::InsertHistoryCell(cell) = ev + && let Some(cell) = cell.as_any().downcast_ref::() + { + user_cell = Some((cell.message.clone(), cell.remote_image_urls.clone())); + break; + } + } + + let (stored_message, stored_remote_image_urls) = + user_cell.expect("expected submitted user history cell"); + assert_eq!(stored_message, String::new()); + assert_eq!(stored_remote_image_urls, vec![remote_url]); +} + +#[tokio::test] +async fn shift_enter_with_only_remote_images_does_not_submit_user_turn() { + let (mut chat, mut rx, mut op_rx) = make_chatwidget_manual(None).await; + + let conversation_id = ThreadId::new(); + let rollout_file = NamedTempFile::new().unwrap(); + let configured = codex_core::protocol::SessionConfiguredEvent { + session_id: conversation_id, + forked_from_id: None, + thread_name: None, + model: "test-model".to_string(), + model_provider_id: "test-provider".to_string(), + approval_policy: AskForApproval::Never, + sandbox_policy: SandboxPolicy::new_read_only_policy(), + cwd: PathBuf::from("/home/user/project"), + reasoning_effort: Some(ReasoningEffortConfig::default()), + history_log_id: 0, + history_entry_count: 0, + initial_messages: None, + network_proxy: None, + rollout_path: Some(rollout_file.path().to_path_buf()), + }; + chat.handle_codex_event(Event { + id: "initial".into(), + msg: EventMsg::SessionConfigured(configured), + }); + drain_insert_history(&mut rx); + + let remote_url = "https://example.com/remote-only.png".to_string(); + chat.set_remote_image_urls(vec![remote_url.clone()]); + assert_eq!(chat.bottom_pane.composer_text(), ""); + + chat.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::SHIFT)); + + assert_no_submit_op(&mut op_rx); + assert_eq!(chat.remote_image_urls(), vec![remote_url]); +} + +#[tokio::test] +async fn enter_with_only_remote_images_does_not_submit_when_modal_is_active() { + let (mut chat, mut rx, mut op_rx) = make_chatwidget_manual(None).await; + + let conversation_id = ThreadId::new(); + let rollout_file = NamedTempFile::new().unwrap(); + let configured = codex_core::protocol::SessionConfiguredEvent { + session_id: conversation_id, + forked_from_id: None, + thread_name: None, + model: "test-model".to_string(), + model_provider_id: "test-provider".to_string(), + approval_policy: AskForApproval::Never, + sandbox_policy: SandboxPolicy::new_read_only_policy(), + cwd: PathBuf::from("/home/user/project"), + reasoning_effort: Some(ReasoningEffortConfig::default()), + history_log_id: 0, + history_entry_count: 0, + initial_messages: None, + network_proxy: None, + rollout_path: Some(rollout_file.path().to_path_buf()), + }; + chat.handle_codex_event(Event { + id: "initial".into(), + msg: EventMsg::SessionConfigured(configured), + }); + drain_insert_history(&mut rx); + + let remote_url = "https://example.com/remote-only.png".to_string(); + chat.set_remote_image_urls(vec![remote_url.clone()]); + + chat.open_review_popup(); + chat.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE)); + + assert_eq!(chat.remote_image_urls(), vec![remote_url]); + assert_no_submit_op(&mut op_rx); +} + +#[tokio::test] +async fn enter_with_only_remote_images_does_not_submit_when_input_disabled() { + let (mut chat, mut rx, mut op_rx) = make_chatwidget_manual(None).await; + + let conversation_id = ThreadId::new(); + let rollout_file = NamedTempFile::new().unwrap(); + let configured = codex_core::protocol::SessionConfiguredEvent { + session_id: conversation_id, + forked_from_id: None, + thread_name: None, + model: "test-model".to_string(), + model_provider_id: "test-provider".to_string(), + approval_policy: AskForApproval::Never, + sandbox_policy: SandboxPolicy::new_read_only_policy(), + cwd: PathBuf::from("/home/user/project"), + reasoning_effort: Some(ReasoningEffortConfig::default()), + history_log_id: 0, + history_entry_count: 0, + initial_messages: None, + network_proxy: None, + rollout_path: Some(rollout_file.path().to_path_buf()), + }; + chat.handle_codex_event(Event { + id: "initial".into(), + msg: EventMsg::SessionConfigured(configured), + }); + drain_insert_history(&mut rx); + + let remote_url = "https://example.com/remote-only.png".to_string(); + chat.set_remote_image_urls(vec![remote_url.clone()]); + chat.bottom_pane + .set_composer_input_enabled(false, Some("Input disabled for test.".to_string())); + + chat.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE)); + + assert_eq!(chat.remote_image_urls(), vec![remote_url]); + assert_no_submit_op(&mut op_rx); } #[tokio::test] @@ -508,6 +938,7 @@ async fn blocked_image_restore_preserves_mention_bindings() { text_elements, local_images.clone(), mention_bindings.clone(), + Vec::new(), ); let mention_start = text.find("$file").expect("mention token exists"); @@ -537,6 +968,94 @@ async fn blocked_image_restore_preserves_mention_bindings() { ); } +#[tokio::test] +async fn blocked_image_restore_with_remote_images_keeps_local_placeholder_mapping() { + let (mut chat, _rx, _op_rx) = make_chatwidget_manual(None).await; + + let first_placeholder = "[Image #2]"; + let second_placeholder = "[Image #3]"; + let text = format!("{first_placeholder} first\n{second_placeholder} second"); + let second_start = text.find(second_placeholder).expect("second placeholder"); + let text_elements = vec![ + TextElement::new( + (0..first_placeholder.len()).into(), + Some(first_placeholder.to_string()), + ), + TextElement::new( + (second_start..second_start + second_placeholder.len()).into(), + Some(second_placeholder.to_string()), + ), + ]; + let local_images = vec![ + LocalImageAttachment { + placeholder: first_placeholder.to_string(), + path: PathBuf::from("/tmp/blocked-first.png"), + }, + LocalImageAttachment { + placeholder: second_placeholder.to_string(), + path: PathBuf::from("/tmp/blocked-second.png"), + }, + ]; + let remote_image_urls = vec!["https://example.com/blocked-remote.png".to_string()]; + + chat.restore_blocked_image_submission( + text.clone(), + text_elements.clone(), + local_images.clone(), + Vec::new(), + remote_image_urls.clone(), + ); + + assert_eq!(chat.bottom_pane.composer_text(), text); + assert_eq!(chat.bottom_pane.composer_text_elements(), text_elements); + assert_eq!(chat.bottom_pane.composer_local_images(), local_images); + assert_eq!(chat.remote_image_urls(), remote_image_urls); +} + +#[tokio::test] +async fn queued_restore_with_remote_images_keeps_local_placeholder_mapping() { + let (mut chat, _rx, _op_rx) = make_chatwidget_manual(None).await; + + let first_placeholder = "[Image #2]"; + let second_placeholder = "[Image #3]"; + let text = format!("{first_placeholder} first\n{second_placeholder} second"); + let second_start = text.find(second_placeholder).expect("second placeholder"); + let text_elements = vec![ + TextElement::new( + (0..first_placeholder.len()).into(), + Some(first_placeholder.to_string()), + ), + TextElement::new( + (second_start..second_start + second_placeholder.len()).into(), + Some(second_placeholder.to_string()), + ), + ]; + let local_images = vec![ + LocalImageAttachment { + placeholder: first_placeholder.to_string(), + path: PathBuf::from("/tmp/queued-first.png"), + }, + LocalImageAttachment { + placeholder: second_placeholder.to_string(), + path: PathBuf::from("/tmp/queued-second.png"), + }, + ]; + let remote_image_urls = vec!["https://example.com/queued-remote.png".to_string()]; + + chat.restore_user_message_to_composer(UserMessage { + text: text.clone(), + local_images: local_images.clone(), + remote_image_urls: remote_image_urls.clone(), + text_elements: text_elements.clone(), + mention_bindings: Vec::new(), + }); + + assert_eq!(chat.bottom_pane.composer_text(), text); + assert_eq!(chat.bottom_pane.composer_text_elements(), text_elements); + assert_eq!(chat.bottom_pane.composer_local_images(), local_images); + assert_eq!(chat.remote_image_urls(), remote_image_urls); +} + #[tokio::test] async fn interrupted_turn_restores_queued_messages_with_images_and_elements() { let (mut chat, _rx, _op_rx) = make_chatwidget_manual(None).await; @@ -571,6 +1090,7 @@ async fn interrupted_turn_restores_queued_messages_with_images_and_elements() { placeholder: first_placeholder.to_string(), path: first_images[0].clone(), }], + remote_image_urls: Vec::new(), text_elements: first_elements, mention_bindings: Vec::new(), }); @@ -580,6 +1100,7 @@ async fn interrupted_turn_restores_queued_messages_with_images_and_elements() { placeholder: second_placeholder.to_string(), path: second_images[0].clone(), }], + remote_image_urls: Vec::new(), text_elements: second_elements, mention_bindings: Vec::new(), }); @@ -649,6 +1170,7 @@ async fn interrupted_turn_restore_keeps_active_mode_for_resubmission() { chat.queued_user_messages.push_back(UserMessage { text: "Implement the plan.".to_string(), local_images: Vec::new(), + remote_image_urls: Vec::new(), text_elements: Vec::new(), mention_bindings: Vec::new(), }); @@ -711,6 +1233,7 @@ async fn remap_placeholders_uses_attachment_labels() { text, text_elements: elements, local_images: attachments, + remote_image_urls: vec!["https://example.com/a.png".to_string()], mention_bindings: Vec::new(), }; let mut next_label = 3usize; @@ -743,6 +1266,10 @@ async fn remap_placeholders_uses_attachment_labels() { }, ] ); + assert_eq!( + remapped.remote_image_urls, + vec!["https://example.com/a.png".to_string()] + ); } #[tokio::test] @@ -772,6 +1299,7 @@ async fn remap_placeholders_uses_byte_ranges_when_placeholder_missing() { text, text_elements: elements, local_images: attachments, + remote_image_urls: Vec::new(), mention_bindings: Vec::new(), }; let mut next_label = 3usize; @@ -1150,6 +1678,15 @@ fn next_submit_op(op_rx: &mut tokio::sync::mpsc::UnboundedReceiver) -> Op { } } +fn assert_no_submit_op(op_rx: &mut tokio::sync::mpsc::UnboundedReceiver) { + while let Ok(op) = op_rx.try_recv() { + assert!( + !matches!(op, Op::UserTurn { .. }), + "unexpected submit op: {op:?}" + ); + } +} + fn set_chatgpt_auth(chat: &mut ChatWidget) { chat.auth_manager = codex_core::test_support::auth_manager_from_auth( CodexAuth::create_dummy_chatgpt_auth_for_testing(), diff --git a/codex-rs/tui/src/history_cell.rs b/codex-rs/tui/src/history_cell.rs index 360d481aa..878919694 100644 --- a/codex-rs/tui/src/history_cell.rs +++ b/codex-rs/tui/src/history_cell.rs @@ -49,6 +49,7 @@ use codex_protocol::account::PlanType; use codex_protocol::mcp::Resource; use codex_protocol::mcp::ResourceTemplate; use codex_protocol::models::WebSearchAction; +use codex_protocol::models::local_image_label_text; use codex_protocol::openai_models::ReasoningEffort as ReasoningEffortConfig; use codex_protocol::plan_tool::PlanItemArg; use codex_protocol::plan_tool::StepStatus; @@ -168,6 +169,7 @@ pub(crate) struct UserHistoryCell { pub text_elements: Vec, #[allow(dead_code)] pub local_image_paths: Vec, + pub remote_image_urls: Vec, } /// Build logical lines for a user message with styled text elements. @@ -236,10 +238,22 @@ fn build_user_message_lines_with_elements( raw_lines } +fn remote_image_display_line(style: Style, index: usize) -> Line<'static> { + Line::from(local_image_label_text(index)).style(style) +} + +fn trim_trailing_blank_lines(mut lines: Vec>) -> Vec> { + while lines + .last() + .is_some_and(|line| line.spans.iter().all(|span| span.content.trim().is_empty())) + { + lines.pop(); + } + lines +} + impl HistoryCell for UserHistoryCell { fn display_lines(&self, width: u16) -> Vec> { - let mut lines: Vec> = Vec::new(); - let wrap_width = width .saturating_sub( LIVE_PREFIX_COLS + 1, /* keep a one-column right margin for wrapping */ @@ -249,13 +263,35 @@ impl HistoryCell for UserHistoryCell { let style = user_message_style(); let element_style = style.fg(Color::Cyan); - let wrapped = if self.text_elements.is_empty() { - word_wrap_lines( - self.message.split('\n').map(|l| Line::from(l).style(style)), + let wrapped_remote_images = if self.remote_image_urls.is_empty() { + None + } else { + Some(word_wrap_lines( + self.remote_image_urls + .iter() + .enumerate() + .map(|(idx, _url)| { + remote_image_display_line(element_style, idx.saturating_add(1)) + }), + RtOptions::new(usize::from(wrap_width)) + .wrap_algorithm(textwrap::WrapAlgorithm::FirstFit), + )) + }; + + let wrapped_message = if self.message.is_empty() && self.text_elements.is_empty() { + None + } else if self.text_elements.is_empty() { + let message_without_trailing_newlines = self.message.trim_end_matches(['\r', '\n']); + let wrapped = word_wrap_lines( + message_without_trailing_newlines + .split('\n') + .map(|line| Line::from(line).style(style)), // Wrap algorithm matches textarea.rs. RtOptions::new(usize::from(wrap_width)) .wrap_algorithm(textwrap::WrapAlgorithm::FirstFit), - ) + ); + let wrapped = trim_trailing_blank_lines(wrapped); + (!wrapped.is_empty()).then_some(wrapped) } else { let raw_lines = build_user_message_lines_with_elements( &self.message, @@ -263,18 +299,57 @@ impl HistoryCell for UserHistoryCell { style, element_style, ); - word_wrap_lines( + let wrapped = word_wrap_lines( raw_lines, RtOptions::new(usize::from(wrap_width)) .wrap_algorithm(textwrap::WrapAlgorithm::FirstFit), - ) + ); + let wrapped = trim_trailing_blank_lines(wrapped); + (!wrapped.is_empty()).then_some(wrapped) }; - lines.push(Line::from("").style(style)); - lines.extend(prefix_lines(wrapped, "› ".bold().dim(), " ".into())); + if wrapped_remote_images.is_none() && wrapped_message.is_none() { + return Vec::new(); + } + + let mut lines: Vec> = vec![Line::from("").style(style)]; + + if let Some(wrapped_remote_images) = wrapped_remote_images { + lines.extend(prefix_lines( + wrapped_remote_images, + " ".into(), + " ".into(), + )); + if wrapped_message.is_some() { + lines.push(Line::from("").style(style)); + } + } + + if let Some(wrapped_message) = wrapped_message { + lines.extend(prefix_lines( + wrapped_message, + "› ".bold().dim(), + " ".into(), + )); + } + lines.push(Line::from("").style(style)); lines } + + fn desired_height(&self, width: u16) -> u16 { + self.display_lines(width) + .len() + .try_into() + .unwrap_or(u16::MAX) + } + + fn desired_transcript_height(&self, width: u16) -> u16 { + self.display_lines(width) + .len() + .try_into() + .unwrap_or(u16::MAX) + } } #[derive(Debug)] @@ -1018,11 +1093,13 @@ pub(crate) fn new_user_prompt( message: String, text_elements: Vec, local_image_paths: Vec, + remote_image_urls: Vec, ) -> UserHistoryCell { UserHistoryCell { message, text_elements, local_image_paths, + remote_image_urls, } } @@ -3359,6 +3436,7 @@ mod tests { message: msg.to_string(), text_elements: Vec::new(), local_image_paths: Vec::new(), + remote_image_urls: Vec::new(), }; // Small width to force wrapping more clearly. Effective wrap width is width-2 due to the ▌ prefix and trailing space. @@ -3369,6 +3447,120 @@ mod tests { insta::assert_snapshot!(rendered); } + #[test] + fn user_history_cell_renders_remote_image_urls() { + let cell = UserHistoryCell { + message: "describe these".to_string(), + text_elements: Vec::new(), + local_image_paths: Vec::new(), + remote_image_urls: vec!["https://example.com/example.png".to_string()], + }; + + let rendered = render_lines(&cell.display_lines(80)).join("\n"); + + assert!(rendered.contains("[Image #1]")); + assert!(rendered.contains("describe these")); + insta::assert_snapshot!(rendered); + } + + #[test] + fn user_history_cell_summarizes_inline_data_urls() { + let cell = UserHistoryCell { + message: "describe inline image".to_string(), + text_elements: Vec::new(), + local_image_paths: Vec::new(), + remote_image_urls: vec!["data:image/png;base64,aGVsbG8=".to_string()], + }; + + let rendered = render_lines(&cell.display_lines(80)).join("\n"); + + assert!(rendered.contains("[Image #1]")); + assert!(rendered.contains("describe inline image")); + } + + #[test] + fn user_history_cell_numbers_multiple_remote_images() { + let cell = UserHistoryCell { + message: "describe both".to_string(), + text_elements: Vec::new(), + local_image_paths: Vec::new(), + remote_image_urls: vec![ + "https://example.com/one.png".to_string(), + "https://example.com/two.png".to_string(), + ], + }; + + let rendered = render_lines(&cell.display_lines(80)).join("\n"); + + assert!(rendered.contains("[Image #1]")); + assert!(rendered.contains("[Image #2]")); + insta::assert_snapshot!(rendered); + } + + #[test] + fn user_history_cell_height_matches_rendered_lines_with_remote_images() { + let cell = UserHistoryCell { + message: "line one\nline two".to_string(), + text_elements: Vec::new(), + local_image_paths: Vec::new(), + remote_image_urls: vec![ + "https://example.com/one.png".to_string(), + "https://example.com/two.png".to_string(), + ], + }; + + let width = 80; + let rendered_len: u16 = cell + .display_lines(width) + .len() + .try_into() + .unwrap_or(u16::MAX); + assert_eq!(cell.desired_height(width), rendered_len); + assert_eq!(cell.desired_transcript_height(width), rendered_len); + } + + #[test] + fn user_history_cell_trims_trailing_blank_message_lines() { + let cell = UserHistoryCell { + message: "line one\n\n \n\t \n".to_string(), + text_elements: Vec::new(), + local_image_paths: Vec::new(), + remote_image_urls: vec!["https://example.com/one.png".to_string()], + }; + + let rendered = render_lines(&cell.display_lines(80)); + let trailing_blank_count = rendered + .iter() + .rev() + .take_while(|line| line.trim().is_empty()) + .count(); + assert_eq!(trailing_blank_count, 1); + assert!(rendered.iter().any(|line| line.contains("line one"))); + } + + #[test] + fn user_history_cell_trims_trailing_blank_message_lines_with_text_elements() { + let message = "tokenized\n\n\n".to_string(); + let cell = UserHistoryCell { + message, + text_elements: vec![TextElement::new( + (0..8).into(), + Some("tokenized".to_string()), + )], + local_image_paths: Vec::new(), + remote_image_urls: vec!["https://example.com/one.png".to_string()], + }; + + let rendered = render_lines(&cell.display_lines(80)); + let trailing_blank_count = rendered + .iter() + .rev() + .take_while(|line| line.trim().is_empty()) + .count(); + assert_eq!(trailing_blank_count, 1); + assert!(rendered.iter().any(|line| line.contains("tokenized"))); + } + #[test] fn plan_update_with_note_and_wrapping_snapshot() { // Long explanation forces wrapping; include long step text to verify step wrapping and alignment. diff --git a/codex-rs/tui/src/snapshots/codex_tui__history_cell__tests__user_history_cell_numbers_multiple_remote_images.snap b/codex-rs/tui/src/snapshots/codex_tui__history_cell__tests__user_history_cell_numbers_multiple_remote_images.snap new file mode 100644 index 000000000..076df8fff --- /dev/null +++ b/codex-rs/tui/src/snapshots/codex_tui__history_cell__tests__user_history_cell_numbers_multiple_remote_images.snap @@ -0,0 +1,9 @@ +--- +source: tui/src/history_cell.rs +expression: rendered +--- + + [Image #1] + [Image #2] + +› describe both diff --git a/codex-rs/tui/src/snapshots/codex_tui__history_cell__tests__user_history_cell_renders_remote_image_urls.snap b/codex-rs/tui/src/snapshots/codex_tui__history_cell__tests__user_history_cell_renders_remote_image_urls.snap new file mode 100644 index 000000000..923ee953a --- /dev/null +++ b/codex-rs/tui/src/snapshots/codex_tui__history_cell__tests__user_history_cell_renders_remote_image_urls.snap @@ -0,0 +1,8 @@ +--- +source: tui/src/history_cell.rs +expression: rendered +--- + + [Image #1] + +› describe these diff --git a/docs/tui-chat-composer.md b/docs/tui-chat-composer.md index e4cf0c1b0..40e23df3e 100644 --- a/docs/tui-chat-composer.md +++ b/docs/tui-chat-composer.md @@ -56,10 +56,11 @@ The solution is to detect paste-like _bursts_ and buffer them into a single expl 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. + does **not** carry text element ranges or 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. + elements, local image paths, and remote image URLs. 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. @@ -127,6 +128,23 @@ 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. +## Remote image rows (selection/deletion flow) + +Remote image URLs are shown as `[Image #N]` rows above the textarea, inside the same composer box. +They are attachment rows, not editable textarea content. + +- TUI can remove these rows, but cannot type before/between them. +- Press `Up` at textarea cursor position `0` to select the last remote image row. +- While selected, `Up`/`Down` moves selection across remote image rows. +- Pressing `Down` on the last row exits remote-row selection and returns to textarea editing. +- `Delete` or `Backspace` removes the selected remote image row. + +Image numbering is unified: + +- Remote image rows always occupy `[Image #1]..[Image #M]`. +- Local attached image placeholders start after that offset (`[Image #M+1]..`). +- Removing remote rows relabels local placeholders so numbering stays contiguous. + ## History navigation (Up/Down) and backtrack prefill `ChatComposerHistory` merges two kinds of history: @@ -139,6 +157,7 @@ Local history entries capture: - raw text (including placeholders), - `TextElement` ranges for placeholders, - local image paths, +- remote image URLs, - pending large-paste payloads (for drafts). Persistent history entries only restore text. They intentionally do **not** rehydrate attachments @@ -150,17 +169,17 @@ line). This keeps multiline cursor movement intact while preserving shell-like h ### 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. +Ctrl+C clears the composer but stashes the full draft state (text elements, local image paths, +remote image URLs, 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: +After a successful submission, the local history entry stores the submitted text, element ranges, +local image paths, and remote image URLs. 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. +- Up/Down recall of a submitted message restores remote image rows plus local image placeholders. - Recalled entries place the cursor at end-of-line to match typical shell history editing. - Large-paste placeholders are not expected in recalled submitted history; the text is the expanded paste content. @@ -168,14 +187,15 @@ placeholders are expanded into their full text before being recorded. This means ### 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. +reuses the selected message’s text elements, local image paths, and remote image URLs, 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. +then normalized to `[Image #M]..[Image #N]`, where `M` starts after the number of remote image +rows, to keep attachment mapping consistent after edits. ## Paste burst: concepts and assumptions