diff --git a/codex-rs/protocol/src/models.rs b/codex-rs/protocol/src/models.rs index 137b12916b..488dbd7247 100644 --- a/codex-rs/protocol/src/models.rs +++ b/codex-rs/protocol/src/models.rs @@ -187,18 +187,18 @@ fn local_image_label_suffix() -> String { } pub fn local_image_label_text(path: &std::path::Path) -> String { - format!("Image {}{}", path.display(), local_image_label_suffix()) + local_image_label_text_with_number(path, None) } pub fn is_local_image_label_text(text: &str) -> bool { let suffix = local_image_label_suffix(); let trimmed = text.trim(); - trimmed.starts_with("Image ") && trimmed.ends_with(&suffix) + trimmed.starts_with("[Image ") && trimmed.ends_with(&suffix) } -fn local_image_label(path: &std::path::Path) -> ContentItem { +fn local_image_label(path: &std::path::Path, label_number: Option) -> ContentItem { ContentItem::InputText { - text: local_image_label_text(path), + text: local_image_label_text_with_number(path, label_number), } } @@ -226,11 +226,29 @@ fn unsupported_image_error_placeholder(path: &std::path::Path, mime: &str) -> Co } pub fn local_image_content_items(path: &std::path::Path, include_label: bool) -> Vec { + local_image_content_items_with_label_number(path, include_label, None) +} + +fn local_image_label_text_with_number( + path: &std::path::Path, + label_number: Option, +) -> String { + match label_number { + Some(label_number) => format!("[Image #{label_number}]{}", local_image_label_suffix()), + None => format!("[Image {}]{}", path.display(), local_image_label_suffix()), + } +} + +fn local_image_content_items_with_label_number( + path: &std::path::Path, + include_label: bool, + label_number: Option, +) -> Vec { match load_and_resize_to_fit(path) { Ok(image) => { let mut items = Vec::with_capacity(2); if include_label { - items.push(local_image_label(path)); + items.push(local_image_label(path, label_number)); } items.push(ContentItem::InputImage { image_url: image.into_data_url(), @@ -262,6 +280,13 @@ pub fn local_image_content_items(path: &std::path::Path, include_label: bool) -> } } +fn is_default_clipboard_image_path(path: &std::path::Path) -> bool { + path.file_name() + .and_then(|name| name.to_str()) + .map(|name| name.starts_with("codex-clipboard-")) + .unwrap_or(false) +} + impl From for ResponseItem { fn from(item: ResponseInputItem) -> Self { match item { @@ -355,6 +380,7 @@ pub enum ReasoningItemContent { impl From> for ResponseInputItem { fn from(items: Vec) -> Self { + let mut image_index = 0; Self::Message { role: "user".to_string(), content: items @@ -362,7 +388,15 @@ impl From> for ResponseInputItem { .flat_map(|c| match c { UserInput::Text { text } => vec![ContentItem::InputText { text }], UserInput::Image { image_url } => vec![ContentItem::InputImage { image_url }], - UserInput::LocalImage { path } => local_image_content_items(&path, true), + UserInput::LocalImage { path } => { + image_index += 1; + let label_number = if is_default_clipboard_image_path(&path) { + Some(image_index) + } else { + None + }; + local_image_content_items_with_label_number(&path, true, label_number) + } UserInput::Skill { .. } => Vec::new(), // Skill bodies are injected later in core }) .collect::>(), diff --git a/codex-rs/tui/src/bottom_pane/chat_composer.rs b/codex-rs/tui/src/bottom_pane/chat_composer.rs index 2e7e2241b9..4ad59dc2f1 100644 --- a/codex-rs/tui/src/bottom_pane/chat_composer.rs +++ b/codex-rs/tui/src/bottom_pane/chat_composer.rs @@ -46,7 +46,6 @@ use crate::style::user_message_style; use codex_common::fuzzy_match::fuzzy_match; use codex_protocol::custom_prompts::CustomPrompt; use codex_protocol::custom_prompts::PROMPTS_CMD_PREFIX; -use codex_protocol::models::local_image_label_text; use crate::app_event::AppEvent; use crate::app_event_sender::AppEventSender; @@ -60,18 +59,11 @@ use codex_core::skills::model::SkillMetadata; use codex_file_search::FileMatch; use std::cell::RefCell; use std::collections::HashMap; -use std::collections::HashSet; +use std::path::Path; use std::path::PathBuf; use std::time::Duration; use std::time::Instant; -fn windows_degraded_sandbox_active() -> bool { - cfg!(target_os = "windows") - && codex_core::windows_sandbox::ELEVATED_SANDBOX_NUX_ENABLED - && codex_core::get_platform_sandbox().is_some() - && !codex_core::is_windows_elevated_sandbox_enabled() -} - /// If the pasted content exceeds this number of characters, replace it with a /// placeholder in the UI. const LARGE_PASTE_CHAR_THRESHOLD: usize = 1000; @@ -275,12 +267,10 @@ impl ChatComposer { // normalize_pasted_path already handles Windows → WSL path conversion, // so we can directly try to read the image dimensions. match image::image_dimensions(&path_buf) { - Ok((width, height)) => { + Ok((w, h)) => { tracing::info!("OK: {pasted}"); - tracing::debug!("image dimensions={}x{}", width, height); - let format = pasted_image_format(&path_buf); - tracing::debug!("attached image format={}", format.label()); - self.attach_image(path_buf); + let format_label = pasted_image_format(&path_buf).label(); + self.attach_image(path_buf, w, h, format_label); true } Err(err) => { @@ -411,9 +401,19 @@ impl ChatComposer { } /// Attempt to start a burst by retro-capturing recent chars before the cursor. - pub fn attach_image(&mut self, path: PathBuf) { + pub fn attach_image(&mut self, path: PathBuf, width: u32, height: u32, _format_label: &str) { let image_number = self.attached_images.len() + 1; - let placeholder = local_image_label_text(image_number); + let placeholder = if Self::is_default_clipboard_filename(&path) { + let base_placeholder = format!("Image #{image_number}"); + self.next_image_placeholder(&base_placeholder) + } else { + let file_label = path + .file_name() + .map(|name| name.to_string_lossy().into_owned()) + .unwrap_or_else(|| "image".to_string()); + let base_placeholder = format!("{file_label} {width}x{height}"); + self.next_image_placeholder(&base_placeholder) + }; // Insert as an element to match large paste placeholder behavior: // styled distinctly and treated atomically for cursor/mutations. self.textarea.insert_element(&placeholder); @@ -476,6 +476,29 @@ impl ChatComposer { } } + fn next_image_placeholder(&mut self, base: &str) -> String { + let text = self.textarea.text(); + let mut suffix = 1; + loop { + let placeholder = if suffix == 1 { + format!("[{base}]") + } else { + format!("[{base} #{suffix}]") + }; + if !text.contains(&placeholder) { + return placeholder; + } + suffix += 1; + } + } + + fn is_default_clipboard_filename(path: &Path) -> bool { + path.file_name() + .and_then(|name| name.to_str()) + .map(|name| name.starts_with("codex-clipboard-")) + .unwrap_or(false) + } + pub(crate) fn insert_str(&mut self, text: &str) { self.textarea.insert_str(text); self.sync_popups(); @@ -801,43 +824,47 @@ impl ChatComposer { if is_image { // Determine dimensions; if that fails fall back to normal path insertion. let path_buf = PathBuf::from(&sel_path); - match image::image_dimensions(&path_buf) { - Ok((width, height)) => { - tracing::debug!("selected image dimensions={}x{}", width, height); - // Remove the current @token (mirror logic from insert_selected_path without inserting text) - // using the flat text and byte-offset cursor API. - let cursor_offset = self.textarea.cursor(); - let text = self.textarea.text(); - // Clamp to a valid char boundary to avoid panics when slicing. - let safe_cursor = Self::clamp_to_char_boundary(text, cursor_offset); - let before_cursor = &text[..safe_cursor]; - let after_cursor = &text[safe_cursor..]; + if let Ok((w, h)) = image::image_dimensions(&path_buf) { + // Remove the current @token (mirror logic from insert_selected_path without inserting text) + // using the flat text and byte-offset cursor API. + let cursor_offset = self.textarea.cursor(); + let text = self.textarea.text(); + // Clamp to a valid char boundary to avoid panics when slicing. + let safe_cursor = Self::clamp_to_char_boundary(text, cursor_offset); + let before_cursor = &text[..safe_cursor]; + let after_cursor = &text[safe_cursor..]; - // Determine token boundaries in the full text. - let start_idx = before_cursor - .char_indices() - .rfind(|(_, c)| c.is_whitespace()) - .map(|(idx, c)| idx + c.len_utf8()) - .unwrap_or(0); - let end_rel_idx = after_cursor - .char_indices() - .find(|(_, c)| c.is_whitespace()) - .map(|(idx, _)| idx) - .unwrap_or(after_cursor.len()); - let end_idx = safe_cursor + end_rel_idx; + // Determine token boundaries in the full text. + let start_idx = before_cursor + .char_indices() + .rfind(|(_, c)| c.is_whitespace()) + .map(|(idx, c)| idx + c.len_utf8()) + .unwrap_or(0); + let end_rel_idx = after_cursor + .char_indices() + .find(|(_, c)| c.is_whitespace()) + .map(|(idx, _)| idx) + .unwrap_or(after_cursor.len()); + let end_idx = safe_cursor + end_rel_idx; - self.textarea.replace_range(start_idx..end_idx, ""); - self.textarea.set_cursor(start_idx); + self.textarea.replace_range(start_idx..end_idx, ""); + self.textarea.set_cursor(start_idx); - self.attach_image(path_buf); - // Add a trailing space to keep typing fluid. - self.textarea.insert_str(" "); - } - Err(err) => { - tracing::trace!("image dimensions lookup failed: {err}"); - // Fallback to plain path insertion if metadata read fails. - self.insert_selected_path(&sel_path); - } + let format_label = match Path::new(&sel_path) + .extension() + .and_then(|e| e.to_str()) + .map(str::to_ascii_lowercase) + { + Some(ext) if ext == "png" => "PNG", + Some(ext) if ext == "jpg" || ext == "jpeg" => "JPEG", + _ => "IMG", + }; + self.attach_image(path_buf, w, h, format_label); + // Add a trailing space to keep typing fluid. + self.textarea.insert_str(" "); + } else { + // Fallback to plain path insertion if metadata read fails. + self.insert_selected_path(&sel_path); } } else { // Non-image: inserting file path. @@ -1215,10 +1242,6 @@ impl ChatComposer { && rest.is_empty() && let Some((_n, cmd)) = built_in_slash_commands() .into_iter() - .filter(|(_, cmd)| { - windows_degraded_sandbox_active() - || *cmd != SlashCommand::ElevateSandbox - }) .find(|(n, _)| *n == name) { self.textarea.set_text(""); @@ -1286,10 +1309,6 @@ impl ChatComposer { if !treat_as_plain_text { let is_builtin = built_in_slash_commands() .into_iter() - .filter(|(_, cmd)| { - windows_degraded_sandbox_active() - || *cmd != SlashCommand::ElevateSandbox - }) .any(|(command_name, _)| command_name == name); let prompt_prefix = format!("{PROMPTS_CMD_PREFIX}:"); let is_known_prompt = name @@ -1443,29 +1462,20 @@ impl ChatComposer { } } - // Backspace at the start of an image placeholder should delete that placeholder (rather - // than deleting content before it). Do this without scanning the full text by consulting - // the textarea's element list. - if matches!(input.code, KeyCode::Backspace) - && self.try_remove_image_element_at_cursor_start() + // For non-char inputs (or after flushing), handle normally. + // Special handling for backspace on placeholders + if let KeyEvent { + code: KeyCode::Backspace, + .. + } = input + && self.try_remove_any_placeholder_at_cursor() { return (InputResult::None, true); } - // 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() { - None - } else { - Some(self.textarea.element_payloads()) - }; - + // Normal input handling self.textarea.input(input); - - if let Some(elements_before) = elements_before { - self.reconcile_deleted_elements(elements_before); - } + let text_after = self.textarea.text(); // Update paste-burst heuristic for plain Char (no Ctrl/Alt) events. let crossterm::event::KeyEvent { @@ -1487,69 +1497,176 @@ impl ChatComposer { } } + // Check if any placeholders were removed and remove their corresponding pending pastes + self.pending_pastes + .retain(|(placeholder, _)| text_after.contains(placeholder)); + + // Keep attached images in proportion to how many matching placeholders exist in the text. + // This handles duplicate placeholders that share the same visible label. + if !self.attached_images.is_empty() { + let mut needed: HashMap = HashMap::new(); + for img in &self.attached_images { + needed + .entry(img.placeholder.clone()) + .or_insert_with(|| text_after.matches(&img.placeholder).count()); + } + + let mut used: HashMap = HashMap::new(); + let mut kept: Vec = Vec::with_capacity(self.attached_images.len()); + for img in self.attached_images.drain(..) { + let total_needed = *needed.get(&img.placeholder).unwrap_or(&0); + let used_count = used.entry(img.placeholder.clone()).or_insert(0); + if *used_count < total_needed { + kept.push(img); + *used_count += 1; + } + } + self.attached_images = kept; + } + (InputResult::None, true) } - fn try_remove_image_element_at_cursor_start(&mut self) -> bool { - if self.attached_images.is_empty() { - return false; - } + /// Attempts to remove an image or paste placeholder if the cursor is at the end of one. + /// Returns true if a placeholder was removed. + fn try_remove_any_placeholder_at_cursor(&mut self) -> bool { + // Clamp the cursor to a valid char boundary to avoid panics when slicing. + let text = self.textarea.text(); + let p = Self::clamp_to_char_boundary(text, self.textarea.cursor()); - let p = self.textarea.cursor(); - let Some(payload) = self.textarea.element_payload_starting_at(p) else { - return false; - }; - let Some(idx) = self - .attached_images - .iter() - .position(|img| img.placeholder == payload) - else { - return false; - }; - - self.textarea.replace_range(p..p + payload.len(), ""); - self.attached_images.remove(idx); - self.relabel_attached_images_and_update_placeholders(); - true - } - - fn reconcile_deleted_elements(&mut self, elements_before: Vec) { - let elements_after: HashSet = - self.textarea.element_payloads().into_iter().collect(); - - let mut removed_any_image = false; - for removed in elements_before - .into_iter() - .filter(|payload| !elements_after.contains(payload)) - { - self.pending_pastes.retain(|(ph, _)| ph != &removed); - - if let Some(idx) = self - .attached_images - .iter() - .position(|img| img.placeholder == removed) - { - self.attached_images.remove(idx); - removed_any_image = true; + // Try image placeholders first + let mut out: Option<(usize, String)> = None; + // Detect if the cursor is at the end of any image placeholder. + // If duplicates exist, remove the specific occurrence's mapping. + for (i, img) in self.attached_images.iter().enumerate() { + let ph = &img.placeholder; + if p < ph.len() { + continue; } - } - - if removed_any_image { - self.relabel_attached_images_and_update_placeholders(); - } - } - - 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 current = self.attached_images[idx].placeholder.clone(); - if current == expected { + let start = p - ph.len(); + if text.get(start..p) != Some(ph.as_str()) { continue; } - self.attached_images[idx].placeholder = expected.clone(); - let _renamed = self.textarea.replace_element_payload(¤t, &expected); + // Count the number of occurrences of `ph` before `start`. + let mut occ_before = 0usize; + let mut search_pos = 0usize; + while search_pos < start { + let segment = match text.get(search_pos..start) { + Some(s) => s, + None => break, + }; + if let Some(found) = segment.find(ph) { + occ_before += 1; + search_pos += found + ph.len(); + } else { + break; + } + } + + // Remove the occ_before-th attached image that shares this placeholder label. + out = if let Some((remove_idx, _)) = self + .attached_images + .iter() + .enumerate() + .filter(|(_, img2)| img2.placeholder == *ph) + .nth(occ_before) + { + Some((remove_idx, ph.clone())) + } else { + Some((i, ph.clone())) + }; + break; } + if let Some((idx, placeholder)) = out { + self.textarea.replace_range(p - placeholder.len()..p, ""); + self.attached_images.remove(idx); + return true; + } + + // Also handle when the cursor is at the START of an image placeholder. + // let result = 'out: { + let out: Option<(usize, String)> = 'out: { + for (i, img) in self.attached_images.iter().enumerate() { + let ph = &img.placeholder; + if p + ph.len() > text.len() { + continue; + } + if text.get(p..p + ph.len()) != Some(ph.as_str()) { + continue; + } + + // Count occurrences of `ph` before `p`. + let mut occ_before = 0usize; + let mut search_pos = 0usize; + while search_pos < p { + let segment = match text.get(search_pos..p) { + Some(s) => s, + None => break 'out None, + }; + if let Some(found) = segment.find(ph) { + occ_before += 1; + search_pos += found + ph.len(); + } else { + break 'out None; + } + } + + if let Some((remove_idx, _)) = self + .attached_images + .iter() + .enumerate() + .filter(|(_, img2)| img2.placeholder == *ph) + .nth(occ_before) + { + break 'out Some((remove_idx, ph.clone())); + } else { + break 'out Some((i, ph.clone())); + } + } + None + }; + + if let Some((idx, placeholder)) = out { + self.textarea.replace_range(p..p + placeholder.len(), ""); + self.attached_images.remove(idx); + return true; + } + + // Then try pasted-content placeholders + if let Some(placeholder) = self.pending_pastes.iter().find_map(|(ph, _)| { + if p < ph.len() { + return None; + } + let start = p - ph.len(); + if text.get(start..p) == Some(ph.as_str()) { + Some(ph.clone()) + } else { + None + } + }) { + self.textarea.replace_range(p - placeholder.len()..p, ""); + self.pending_pastes.retain(|(ph, _)| ph != &placeholder); + return true; + } + + // Also handle when the cursor is at the START of a pasted-content placeholder. + if let Some(placeholder) = self.pending_pastes.iter().find_map(|(ph, _)| { + if p + ph.len() > text.len() { + return None; + } + if text.get(p..p + ph.len()) == Some(ph.as_str()) { + Some(ph.clone()) + } else { + None + } + }) { + self.textarea.replace_range(p..p + placeholder.len(), ""); + self.pending_pastes.retain(|(ph, _)| ph != &placeholder); + return true; + } + + false } fn handle_shortcut_overlay_key(&mut self, key_event: &KeyEvent) -> bool { @@ -1611,6 +1728,7 @@ impl ChatComposer { self.active_popup = ActivePopup::None; return; } + let skill_token = self.current_skill_token(); let allow_command_popup = file_token.is_none() && skill_token.is_none(); @@ -1680,9 +1798,6 @@ impl ChatComposer { let builtin_match = built_in_slash_commands() .into_iter() - .filter(|(_, cmd)| { - windows_degraded_sandbox_active() || *cmd != SlashCommand::ElevateSandbox - }) .any(|(cmd_name, _)| fuzzy_match(cmd_name, name).is_some()); if builtin_match { @@ -2952,6 +3067,44 @@ mod tests { assert!(composer.textarea.is_empty(), "composer should be cleared"); } + #[test] + fn slash_review_with_args_dispatches_command_with_args() { + use crossterm::event::KeyCode; + use crossterm::event::KeyEvent; + use crossterm::event::KeyModifiers; + + let (tx, _rx) = unbounded_channel::(); + let sender = AppEventSender::new(tx); + let mut composer = ChatComposer::new( + true, + sender, + false, + "Ask Codex to do anything".to_string(), + false, + ); + + type_chars_humanlike(&mut composer, &['/', 'r', 'e', 'v', 'i', 'e', 'w', ' ']); + type_chars_humanlike(&mut composer, &['f', 'i', 'x', ' ', 't', 'h', 'i', 's']); + + let (result, _needs_redraw) = + composer.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE)); + + match result { + InputResult::CommandWithArgs(cmd, args) => { + assert_eq!(cmd, SlashCommand::Review); + assert_eq!(args, "fix this"); + } + InputResult::Command(cmd) => { + panic!("expected args for '/review', got bare command: {cmd:?}") + } + InputResult::Submitted(text) => { + panic!("expected command dispatch, got literal submit: {text}") + } + InputResult::None => panic!("expected CommandWithArgs result for '/review'"), + } + assert!(composer.textarea.is_empty(), "composer should be cleared"); + } + #[test] fn extract_args_supports_quoted_paths_single_arg() { let args = extract_positional_args_for_prompt_line( @@ -3358,12 +3511,12 @@ mod tests { false, ); let path = PathBuf::from("/tmp/image1.png"); - composer.attach_image(path.clone()); + composer.attach_image(path.clone(), 32, 16, "PNG"); composer.handle_paste(" hi".into()); let (result, _) = composer.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE)); match result { - InputResult::Submitted(text) => assert_eq!(text, "[Image #1] hi"), + InputResult::Submitted(text) => assert_eq!(text, "[image1.png 32x16] hi"), _ => panic!("expected Submitted"), } let imgs = composer.take_recent_submission_images(); @@ -3382,11 +3535,11 @@ mod tests { false, ); let path = PathBuf::from("/tmp/image2.png"); - composer.attach_image(path.clone()); + composer.attach_image(path.clone(), 10, 5, "PNG"); let (result, _) = composer.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE)); match result { - InputResult::Submitted(text) => assert_eq!(text, "[Image #1]"), + InputResult::Submitted(text) => assert_eq!(text, "[image2.png 10x5]"), _ => panic!("expected Submitted"), } let imgs = composer.take_recent_submission_images(); @@ -3407,15 +3560,21 @@ mod tests { false, ); let path = PathBuf::from("/tmp/image_dup.png"); - composer.attach_image(path.clone()); + composer.attach_image(path.clone(), 10, 5, "PNG"); composer.handle_paste(" ".into()); - composer.attach_image(path); + composer.attach_image(path, 10, 5, "PNG"); let text = composer.textarea.text().to_string(); - assert!(text.contains("[Image #1]")); - assert!(text.contains("[Image #2]")); - assert_eq!(composer.attached_images[0].placeholder, "[Image #1]"); - assert_eq!(composer.attached_images[1].placeholder, "[Image #2]"); + assert!(text.contains("[image_dup.png 10x5]")); + assert!(text.contains("[image_dup.png 10x5 #2]")); + assert_eq!( + composer.attached_images[0].placeholder, + "[image_dup.png 10x5]" + ); + assert_eq!( + composer.attached_images[1].placeholder, + "[image_dup.png 10x5 #2]" + ); } #[test] @@ -3430,7 +3589,7 @@ mod tests { false, ); let path = PathBuf::from("/tmp/image3.png"); - composer.attach_image(path.clone()); + composer.attach_image(path.clone(), 20, 10, "PNG"); let placeholder = composer.attached_images[0].placeholder.clone(); // Case 1: backspace at end @@ -3441,7 +3600,7 @@ mod tests { // Re-add and test backspace in middle: should break the placeholder string // and drop the image mapping (same as text placeholder behavior). - composer.attach_image(path); + composer.attach_image(path, 20, 10, "PNG"); let placeholder2 = composer.attached_images[0].placeholder.clone(); // Move cursor to roughly middle of placeholder if let Some(start_pos) = composer.textarea.text().find(&placeholder2) { @@ -3473,7 +3632,7 @@ mod tests { // Insert an image placeholder at the start let path = PathBuf::from("/tmp/image_multibyte.png"); - composer.attach_image(path); + composer.attach_image(path, 10, 5, "PNG"); // Add multibyte text after the placeholder composer.textarea.insert_str("日本語"); @@ -3482,11 +3641,16 @@ mod tests { composer.handle_key_event(KeyEvent::new(KeyCode::Backspace, KeyModifiers::NONE)); assert_eq!(composer.attached_images.len(), 1); - assert!(composer.textarea.text().starts_with("[Image #1]")); + assert!( + composer + .textarea + .text() + .starts_with("[image_multibyte.png 10x5]") + ); } #[test] - fn deleting_one_of_duplicate_image_placeholders_removes_one_entry() { + fn deleting_one_of_duplicate_image_placeholders_removes_matching_entry() { let (tx, _rx) = unbounded_channel::(); let sender = AppEventSender::new(tx); let mut composer = ChatComposer::new( @@ -3500,10 +3664,10 @@ mod tests { let path1 = PathBuf::from("/tmp/image_dup1.png"); let path2 = PathBuf::from("/tmp/image_dup2.png"); - composer.attach_image(path1); + composer.attach_image(path1, 10, 5, "PNG"); // separate placeholders with a space for clarity composer.handle_paste(" ".into()); - composer.attach_image(path2.clone()); + composer.attach_image(path2.clone(), 10, 5, "PNG"); let placeholder1 = composer.attached_images[0].placeholder.clone(); let placeholder2 = composer.attached_images[1].placeholder.clone(); @@ -3516,67 +3680,26 @@ mod tests { composer.handle_key_event(KeyEvent::new(KeyCode::Backspace, KeyModifiers::NONE)); let new_text = composer.textarea.text().to_string(); - assert_eq!( - 1, - new_text.matches(&placeholder1).count(), - "one placeholder remains after deletion" - ); assert_eq!( 0, - new_text.matches(&placeholder2).count(), - "second placeholder was relabeled" + new_text.matches(&placeholder1).count(), + "first placeholder removed" ); assert_eq!( 1, - new_text.matches("[Image #1]").count(), - "remaining placeholder relabeled to #1" + new_text.matches(&placeholder2).count(), + "second placeholder remains" ); assert_eq!( vec![AttachedImage { path: path2, - placeholder: "[Image #1]".to_string() + placeholder: "[image_dup2.png 10x5]".to_string() }], composer.attached_images, "one image mapping remains" ); } - #[test] - fn deleting_first_text_element_renumbers_following_text_element() { - use crossterm::event::KeyCode; - use crossterm::event::KeyEvent; - use crossterm::event::KeyModifiers; - - let (tx, _rx) = unbounded_channel::(); - let sender = AppEventSender::new(tx); - let mut composer = ChatComposer::new( - true, - sender, - false, - "Ask Codex to do anything".to_string(), - false, - ); - - let path1 = PathBuf::from("/tmp/image_first.png"); - let path2 = PathBuf::from("/tmp/image_second.png"); - - // Insert two adjacent atomic elements. - composer.attach_image(path1); - composer.attach_image(path2.clone()); - assert_eq!(composer.textarea.text(), "[Image #1][Image #2]"); - assert_eq!(composer.attached_images.len(), 2); - - // Delete the first element using normal textarea editing (Delete at cursor start). - composer.textarea.set_cursor(0); - composer.handle_key_event(KeyEvent::new(KeyCode::Delete, KeyModifiers::NONE)); - - // Remaining image should be renumbered and the textarea element updated. - assert_eq!(composer.attached_images.len(), 1); - assert_eq!(composer.attached_images[0].path, path2); - assert_eq!(composer.attached_images[0].placeholder, "[Image #1]"); - assert_eq!(composer.textarea.text(), "[Image #1]"); - } - #[test] fn pasting_filepath_attaches_image() { let tmp = tempdir().expect("create TempDir"); @@ -3597,7 +3720,12 @@ mod tests { let needs_redraw = composer.handle_paste(tmp_path.to_string_lossy().to_string()); assert!(needs_redraw); - assert!(composer.textarea.text().starts_with("[Image #1] ")); + assert!( + composer + .textarea + .text() + .starts_with("[codex_tui_test_paste_image.png 3x2] ") + ); let imgs = composer.take_recent_submission_images(); assert_eq!(imgs, vec![tmp_path]); @@ -4278,6 +4406,59 @@ mod tests { assert_eq!(result, InputResult::None); } + #[test] + fn history_navigation_takes_priority_over_popups() { + use codex_protocol::protocol::SkillScope; + use crossterm::event::KeyCode; + use crossterm::event::KeyEvent; + use crossterm::event::KeyModifiers; + use tokio::sync::mpsc::unbounded_channel; + + 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_skill_mentions(Some(vec![SkillMetadata { + name: "codex-cli-release-notes".to_string(), + description: "example".to_string(), + short_description: None, + path: PathBuf::from("skills/codex-cli-release-notes/SKILL.md"), + scope: SkillScope::Repo, + }])); + + // Seed local history; the newest entry triggers the skills popup. + composer.history.record_local_submission("older"); + composer + .history + .record_local_submission("$codex-cli-release-notes"); + + // First Up recalls "$...", but we should not open the skills popup while browsing history. + let (result, _redraw) = + composer.handle_key_event(KeyEvent::new(KeyCode::Up, KeyModifiers::NONE)); + assert_eq!(result, InputResult::None); + assert_eq!(composer.textarea.text(), "$codex-cli-release-notes"); + assert!( + matches!(composer.active_popup, ActivePopup::None), + "expected no skills popup while browsing history" + ); + + // Second Up should navigate history again (no popup should interfere). + let (result, _redraw) = + composer.handle_key_event(KeyEvent::new(KeyCode::Up, KeyModifiers::NONE)); + assert_eq!(result, InputResult::None); + assert_eq!(composer.textarea.text(), "older"); + assert!( + matches!(composer.active_popup, ActivePopup::None), + "expected popup to be dismissed after history navigation" + ); + } + #[test] fn slash_popup_activated_for_bare_slash_and_valid_prefixes() { // use crossterm::event::{KeyCode, KeyEvent, KeyModifiers}; @@ -4435,7 +4616,6 @@ mod tests { ); assert_eq!(composer.attached_images.len(), 1); } - #[test] fn input_disabled_ignores_keypresses_and_hides_cursor() { use crossterm::event::KeyCode; diff --git a/codex-rs/tui/src/bottom_pane/mod.rs b/codex-rs/tui/src/bottom_pane/mod.rs index f795ea64f1..8579597865 100644 --- a/codex-rs/tui/src/bottom_pane/mod.rs +++ b/codex-rs/tui/src/bottom_pane/mod.rs @@ -542,9 +542,16 @@ impl BottomPane { self.request_redraw(); } - pub(crate) fn attach_image(&mut self, path: PathBuf) { + pub(crate) fn attach_image( + &mut self, + path: PathBuf, + width: u32, + height: u32, + format_label: &str, + ) { if self.view_stack.is_empty() { - self.composer.attach_image(path); + self.composer + .attach_image(path, width, height, format_label); self.request_redraw(); } } diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index e3fd7a891e..1a3e886e2f 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -1599,7 +1599,7 @@ impl ChatWidget { info.height, info.encoded_format.label() ); - self.attach_image(path); + self.attach_image(path, info.width, info.height, info.encoded_format.label()); } Err(err) => { tracing::warn!("failed to paste image: {err}"); @@ -1652,9 +1652,16 @@ impl ChatWidget { } } - pub(crate) fn attach_image(&mut self, path: PathBuf) { + pub(crate) fn attach_image( + &mut self, + path: PathBuf, + width: u32, + height: u32, + format_label: &str, + ) { tracing::info!("attach_image path={path:?}"); - self.bottom_pane.attach_image(path); + self.bottom_pane + .attach_image(path, width, height, format_label); self.request_redraw(); } diff --git a/codex-rs/tui/src/chatwidget/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index 5d1ac36638..9b4870422d 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -1114,7 +1114,7 @@ async fn ctrl_c_cleared_prompt_is_recoverable_via_history() { chat.bottom_pane.insert_str("draft message "); chat.bottom_pane - .attach_image(PathBuf::from("/tmp/preview.png")); + .attach_image(PathBuf::from("/tmp/preview.png"), 10, 10, "PNG"); let placeholder = "[Image #1]"; assert!( chat.bottom_pane.composer_text().ends_with(placeholder), diff --git a/codex-rs/tui2/src/bottom_pane/chat_composer.rs b/codex-rs/tui2/src/bottom_pane/chat_composer.rs index 118dc8cd3d..bfd24482d2 100644 --- a/codex-rs/tui2/src/bottom_pane/chat_composer.rs +++ b/codex-rs/tui2/src/bottom_pane/chat_composer.rs @@ -49,7 +49,6 @@ use crate::style::user_message_style; use codex_common::fuzzy_match::fuzzy_match; use codex_protocol::custom_prompts::CustomPrompt; use codex_protocol::custom_prompts::PROMPTS_CMD_PREFIX; -use codex_protocol::models::local_image_label_text; use crate::app_event::AppEvent; use crate::app_event_sender::AppEventSender; @@ -63,18 +62,11 @@ use codex_core::skills::model::SkillMetadata; use codex_file_search::FileMatch; use std::cell::RefCell; use std::collections::HashMap; -use std::collections::HashSet; +use std::path::Path; use std::path::PathBuf; use std::time::Duration; use std::time::Instant; -fn windows_degraded_sandbox_active() -> bool { - cfg!(target_os = "windows") - && codex_core::windows_sandbox::ELEVATED_SANDBOX_NUX_ENABLED - && codex_core::get_platform_sandbox().is_some() - && !codex_core::is_windows_elevated_sandbox_enabled() -} - /// If the pasted content exceeds this number of characters, replace it with a /// placeholder in the UI. const LARGE_PASTE_CHAR_THRESHOLD: usize = 1000; @@ -288,12 +280,10 @@ impl ChatComposer { // normalize_pasted_path already handles Windows → WSL path conversion, // so we can directly try to read the image dimensions. match image::image_dimensions(&path_buf) { - Ok((width, height)) => { + Ok((w, h)) => { tracing::info!("OK: {pasted}"); - tracing::debug!("image dimensions={}x{}", width, height); - let format = pasted_image_format(&path_buf); - tracing::debug!("attached image format={}", format.label()); - self.attach_image(path_buf); + let format_label = pasted_image_format(&path_buf).label(); + self.attach_image(path_buf, w, h, format_label); true } Err(err) => { @@ -345,9 +335,17 @@ impl ChatComposer { } /// Attempt to start a burst by retro-capturing recent chars before the cursor. - pub fn attach_image(&mut self, path: PathBuf) { + pub fn attach_image(&mut self, path: PathBuf, width: u32, height: u32, _format_label: &str) { let image_number = self.attached_images.len() + 1; - let placeholder = local_image_label_text(image_number); + let placeholder = if Self::is_default_clipboard_filename(&path) { + format!("[Image #{image_number}]") + } else { + let file_label = path + .file_name() + .map(|name| name.to_string_lossy().into_owned()) + .unwrap_or_else(|| "image".to_string()); + format!("[{file_label} {width}x{height}]") + }; // Insert as an element to match large paste placeholder behavior: // styled distinctly and treated atomically for cursor/mutations. self.textarea.insert_element(&placeholder); @@ -410,6 +408,13 @@ impl ChatComposer { } } + fn is_default_clipboard_filename(path: &Path) -> bool { + path.file_name() + .and_then(|name| name.to_str()) + .map(|name| name.starts_with("codex-clipboard-")) + .unwrap_or(false) + } + pub(crate) fn insert_str(&mut self, text: &str) { self.textarea.insert_str(text); self.sync_popups(); @@ -735,43 +740,47 @@ impl ChatComposer { if is_image { // Determine dimensions; if that fails fall back to normal path insertion. let path_buf = PathBuf::from(&sel_path); - match image::image_dimensions(&path_buf) { - Ok((width, height)) => { - tracing::debug!("selected image dimensions={}x{}", width, height); - // Remove the current @token (mirror logic from insert_selected_path without inserting text) - // using the flat text and byte-offset cursor API. - let cursor_offset = self.textarea.cursor(); - let text = self.textarea.text(); - // Clamp to a valid char boundary to avoid panics when slicing. - let safe_cursor = Self::clamp_to_char_boundary(text, cursor_offset); - let before_cursor = &text[..safe_cursor]; - let after_cursor = &text[safe_cursor..]; + if let Ok((w, h)) = image::image_dimensions(&path_buf) { + // Remove the current @token (mirror logic from insert_selected_path without inserting text) + // using the flat text and byte-offset cursor API. + let cursor_offset = self.textarea.cursor(); + let text = self.textarea.text(); + // Clamp to a valid char boundary to avoid panics when slicing. + let safe_cursor = Self::clamp_to_char_boundary(text, cursor_offset); + let before_cursor = &text[..safe_cursor]; + let after_cursor = &text[safe_cursor..]; - // Determine token boundaries in the full text. - let start_idx = before_cursor - .char_indices() - .rfind(|(_, c)| c.is_whitespace()) - .map(|(idx, c)| idx + c.len_utf8()) - .unwrap_or(0); - let end_rel_idx = after_cursor - .char_indices() - .find(|(_, c)| c.is_whitespace()) - .map(|(idx, _)| idx) - .unwrap_or(after_cursor.len()); - let end_idx = safe_cursor + end_rel_idx; + // Determine token boundaries in the full text. + let start_idx = before_cursor + .char_indices() + .rfind(|(_, c)| c.is_whitespace()) + .map(|(idx, c)| idx + c.len_utf8()) + .unwrap_or(0); + let end_rel_idx = after_cursor + .char_indices() + .find(|(_, c)| c.is_whitespace()) + .map(|(idx, _)| idx) + .unwrap_or(after_cursor.len()); + let end_idx = safe_cursor + end_rel_idx; - self.textarea.replace_range(start_idx..end_idx, ""); - self.textarea.set_cursor(start_idx); + self.textarea.replace_range(start_idx..end_idx, ""); + self.textarea.set_cursor(start_idx); - self.attach_image(path_buf); - // Add a trailing space to keep typing fluid. - self.textarea.insert_str(" "); - } - Err(err) => { - tracing::trace!("image dimensions lookup failed: {err}"); - // Fallback to plain path insertion if metadata read fails. - self.insert_selected_path(&sel_path); - } + let format_label = match Path::new(&sel_path) + .extension() + .and_then(|e| e.to_str()) + .map(str::to_ascii_lowercase) + { + Some(ext) if ext == "png" => "PNG", + Some(ext) if ext == "jpg" || ext == "jpeg" => "JPEG", + _ => "IMG", + }; + self.attach_image(path_buf, w, h, format_label); + // Add a trailing space to keep typing fluid. + self.textarea.insert_str(" "); + } else { + // Fallback to plain path insertion if metadata read fails. + self.insert_selected_path(&sel_path); } } else { // Non-image: inserting file path. @@ -1149,10 +1158,6 @@ impl ChatComposer { && rest.is_empty() && let Some((_n, cmd)) = built_in_slash_commands() .into_iter() - .filter(|(_, cmd)| { - windows_degraded_sandbox_active() - || *cmd != SlashCommand::ElevateSandbox - }) .find(|(n, _)| *n == name) { self.textarea.set_text(""); @@ -1220,10 +1225,6 @@ impl ChatComposer { if !treat_as_plain_text { let is_builtin = built_in_slash_commands() .into_iter() - .filter(|(_, cmd)| { - windows_degraded_sandbox_active() - || *cmd != SlashCommand::ElevateSandbox - }) .any(|(command_name, _)| command_name == name); let prompt_prefix = format!("{PROMPTS_CMD_PREFIX}:"); let is_known_prompt = name @@ -1377,28 +1378,20 @@ impl ChatComposer { } } - // Backspace at the start of an image placeholder should delete that placeholder (rather - // than deleting content before it). Do this without scanning the full text by consulting - // the textarea's element list. - if matches!(input.code, KeyCode::Backspace) - && self.try_remove_image_element_at_cursor_start() + // For non-char inputs (or after flushing), handle normally. + // Special handling for backspace on placeholders + if let KeyEvent { + code: KeyCode::Backspace, + .. + } = input + && self.try_remove_any_placeholder_at_cursor() { return (InputResult::None, true); } - // 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() { - None - } else { - Some(self.textarea.element_payloads()) - }; - + // Normal input handling self.textarea.input(input); - - if let Some(elements_before) = elements_before { - self.reconcile_deleted_elements(elements_before); - } + let text_after = self.textarea.text(); // Update paste-burst heuristic for plain Char (no Ctrl/Alt) events. let crossterm::event::KeyEvent { @@ -1420,69 +1413,176 @@ impl ChatComposer { } } + // Check if any placeholders were removed and remove their corresponding pending pastes + self.pending_pastes + .retain(|(placeholder, _)| text_after.contains(placeholder)); + + // Keep attached images in proportion to how many matching placeholders exist in the text. + // This handles duplicate placeholders that share the same visible label. + if !self.attached_images.is_empty() { + let mut needed: HashMap = HashMap::new(); + for img in &self.attached_images { + needed + .entry(img.placeholder.clone()) + .or_insert_with(|| text_after.matches(&img.placeholder).count()); + } + + let mut used: HashMap = HashMap::new(); + let mut kept: Vec = Vec::with_capacity(self.attached_images.len()); + for img in self.attached_images.drain(..) { + let total_needed = *needed.get(&img.placeholder).unwrap_or(&0); + let used_count = used.entry(img.placeholder.clone()).or_insert(0); + if *used_count < total_needed { + kept.push(img); + *used_count += 1; + } + } + self.attached_images = kept; + } + (InputResult::None, true) } - fn try_remove_image_element_at_cursor_start(&mut self) -> bool { - if self.attached_images.is_empty() { - return false; - } + /// Attempts to remove an image or paste placeholder if the cursor is at the end of one. + /// Returns true if a placeholder was removed. + fn try_remove_any_placeholder_at_cursor(&mut self) -> bool { + // Clamp the cursor to a valid char boundary to avoid panics when slicing. + let text = self.textarea.text(); + let p = Self::clamp_to_char_boundary(text, self.textarea.cursor()); - let p = self.textarea.cursor(); - let Some(payload) = self.textarea.element_payload_starting_at(p) else { - return false; - }; - let Some(idx) = self - .attached_images - .iter() - .position(|img| img.placeholder == payload) - else { - return false; - }; - - self.textarea.replace_range(p..p + payload.len(), ""); - self.attached_images.remove(idx); - self.relabel_attached_images_and_update_placeholders(); - true - } - - fn reconcile_deleted_elements(&mut self, elements_before: Vec) { - let elements_after: HashSet = - self.textarea.element_payloads().into_iter().collect(); - - let mut removed_any_image = false; - for removed in elements_before - .into_iter() - .filter(|payload| !elements_after.contains(payload)) - { - self.pending_pastes.retain(|(ph, _)| ph != &removed); - - if let Some(idx) = self - .attached_images - .iter() - .position(|img| img.placeholder == removed) - { - self.attached_images.remove(idx); - removed_any_image = true; + // Try image placeholders first + let mut out: Option<(usize, String)> = None; + // Detect if the cursor is at the end of any image placeholder. + // If duplicates exist, remove the specific occurrence's mapping. + for (i, img) in self.attached_images.iter().enumerate() { + let ph = &img.placeholder; + if p < ph.len() { + continue; } - } - - if removed_any_image { - self.relabel_attached_images_and_update_placeholders(); - } - } - - 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 current = self.attached_images[idx].placeholder.clone(); - if current == expected { + let start = p - ph.len(); + if text.get(start..p) != Some(ph.as_str()) { continue; } - self.attached_images[idx].placeholder = expected.clone(); - let _renamed = self.textarea.replace_element_payload(¤t, &expected); + // Count the number of occurrences of `ph` before `start`. + let mut occ_before = 0usize; + let mut search_pos = 0usize; + while search_pos < start { + let segment = match text.get(search_pos..start) { + Some(s) => s, + None => break, + }; + if let Some(found) = segment.find(ph) { + occ_before += 1; + search_pos += found + ph.len(); + } else { + break; + } + } + + // Remove the occ_before-th attached image that shares this placeholder label. + out = if let Some((remove_idx, _)) = self + .attached_images + .iter() + .enumerate() + .filter(|(_, img2)| img2.placeholder == *ph) + .nth(occ_before) + { + Some((remove_idx, ph.clone())) + } else { + Some((i, ph.clone())) + }; + break; } + if let Some((idx, placeholder)) = out { + self.textarea.replace_range(p - placeholder.len()..p, ""); + self.attached_images.remove(idx); + return true; + } + + // Also handle when the cursor is at the START of an image placeholder. + // let result = 'out: { + let out: Option<(usize, String)> = 'out: { + for (i, img) in self.attached_images.iter().enumerate() { + let ph = &img.placeholder; + if p + ph.len() > text.len() { + continue; + } + if text.get(p..p + ph.len()) != Some(ph.as_str()) { + continue; + } + + // Count occurrences of `ph` before `p`. + let mut occ_before = 0usize; + let mut search_pos = 0usize; + while search_pos < p { + let segment = match text.get(search_pos..p) { + Some(s) => s, + None => break 'out None, + }; + if let Some(found) = segment.find(ph) { + occ_before += 1; + search_pos += found + ph.len(); + } else { + break 'out None; + } + } + + if let Some((remove_idx, _)) = self + .attached_images + .iter() + .enumerate() + .filter(|(_, img2)| img2.placeholder == *ph) + .nth(occ_before) + { + break 'out Some((remove_idx, ph.clone())); + } else { + break 'out Some((i, ph.clone())); + } + } + None + }; + + if let Some((idx, placeholder)) = out { + self.textarea.replace_range(p..p + placeholder.len(), ""); + self.attached_images.remove(idx); + return true; + } + + // Then try pasted-content placeholders + if let Some(placeholder) = self.pending_pastes.iter().find_map(|(ph, _)| { + if p < ph.len() { + return None; + } + let start = p - ph.len(); + if text.get(start..p) == Some(ph.as_str()) { + Some(ph.clone()) + } else { + None + } + }) { + self.textarea.replace_range(p - placeholder.len()..p, ""); + self.pending_pastes.retain(|(ph, _)| ph != &placeholder); + return true; + } + + // Also handle when the cursor is at the START of a pasted-content placeholder. + if let Some(placeholder) = self.pending_pastes.iter().find_map(|(ph, _)| { + if p + ph.len() > text.len() { + return None; + } + if text.get(p..p + ph.len()) == Some(ph.as_str()) { + Some(ph.clone()) + } else { + None + } + }) { + self.textarea.replace_range(p..p + placeholder.len(), ""); + self.pending_pastes.retain(|(ph, _)| ph != &placeholder); + return true; + } + + false } fn handle_shortcut_overlay_key(&mut self, key_event: &KeyEvent) -> bool { @@ -1571,15 +1671,6 @@ impl ChatComposer { fn sync_popups(&mut self) { let file_token = Self::current_at_token(&self.textarea); - let browsing_history = self - .history - .should_handle_navigation(self.textarea.text(), self.textarea.cursor()); - // When browsing input history (shell-style Up/Down recall), skip all popup - // synchronization so nothing steals focus from continued history navigation. - if browsing_history { - self.active_popup = ActivePopup::None; - return; - } let skill_token = self.current_skill_token(); let allow_command_popup = file_token.is_none() && skill_token.is_none(); @@ -1649,9 +1740,6 @@ impl ChatComposer { let builtin_match = built_in_slash_commands() .into_iter() - .filter(|(_, cmd)| { - windows_degraded_sandbox_active() || *cmd != SlashCommand::ElevateSandbox - }) .any(|(cmd_name, _)| fuzzy_match(cmd_name, name).is_some()); if builtin_match { @@ -2899,6 +2987,44 @@ mod tests { assert!(composer.textarea.is_empty(), "composer should be cleared"); } + #[test] + fn slash_review_with_args_dispatches_command_with_args() { + use crossterm::event::KeyCode; + use crossterm::event::KeyEvent; + use crossterm::event::KeyModifiers; + + let (tx, _rx) = unbounded_channel::(); + let sender = AppEventSender::new(tx); + let mut composer = ChatComposer::new( + true, + sender, + false, + "Ask Codex to do anything".to_string(), + false, + ); + + type_chars_humanlike(&mut composer, &['/', 'r', 'e', 'v', 'i', 'e', 'w', ' ']); + type_chars_humanlike(&mut composer, &['f', 'i', 'x', ' ', 't', 'h', 'i', 's']); + + let (result, _needs_redraw) = + composer.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE)); + + match result { + InputResult::CommandWithArgs(cmd, args) => { + assert_eq!(cmd, SlashCommand::Review); + assert_eq!(args, "fix this"); + } + InputResult::Command(cmd) => { + panic!("expected args for '/review', got bare command: {cmd:?}") + } + InputResult::Submitted(text) => { + panic!("expected command dispatch, got literal submit: {text}") + } + InputResult::None => panic!("expected CommandWithArgs result for '/review'"), + } + assert!(composer.textarea.is_empty(), "composer should be cleared"); + } + #[test] fn extract_args_supports_quoted_paths_single_arg() { let args = extract_positional_args_for_prompt_line( @@ -3305,12 +3431,12 @@ mod tests { false, ); let path = PathBuf::from("/tmp/image1.png"); - composer.attach_image(path.clone()); + composer.attach_image(path.clone(), 32, 16, "PNG"); composer.handle_paste(" hi".into()); let (result, _) = composer.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE)); match result { - InputResult::Submitted(text) => assert_eq!(text, "[Image #1] hi"), + InputResult::Submitted(text) => assert_eq!(text, "[image1.png 32x16] hi"), _ => panic!("expected Submitted"), } let imgs = composer.take_recent_submission_images(); @@ -3329,11 +3455,11 @@ mod tests { false, ); let path = PathBuf::from("/tmp/image2.png"); - composer.attach_image(path.clone()); + composer.attach_image(path.clone(), 10, 5, "PNG"); let (result, _) = composer.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE)); match result { - InputResult::Submitted(text) => assert_eq!(text, "[Image #1]"), + InputResult::Submitted(text) => assert_eq!(text, "[image2.png 10x5]"), _ => panic!("expected Submitted"), } let imgs = composer.take_recent_submission_images(); @@ -3354,7 +3480,7 @@ mod tests { false, ); let path = PathBuf::from("/tmp/image3.png"); - composer.attach_image(path.clone()); + composer.attach_image(path.clone(), 20, 10, "PNG"); let placeholder = composer.attached_images[0].placeholder.clone(); // Case 1: backspace at end @@ -3365,7 +3491,7 @@ mod tests { // Re-add and test backspace in middle: should break the placeholder string // and drop the image mapping (same as text placeholder behavior). - composer.attach_image(path); + composer.attach_image(path, 20, 10, "PNG"); let placeholder2 = composer.attached_images[0].placeholder.clone(); // Move cursor to roughly middle of placeholder if let Some(start_pos) = composer.textarea.text().find(&placeholder2) { @@ -3397,7 +3523,7 @@ mod tests { // Insert an image placeholder at the start let path = PathBuf::from("/tmp/image_multibyte.png"); - composer.attach_image(path); + composer.attach_image(path, 10, 5, "PNG"); // Add multibyte text after the placeholder composer.textarea.insert_str("日本語"); @@ -3406,7 +3532,12 @@ mod tests { composer.handle_key_event(KeyEvent::new(KeyCode::Backspace, KeyModifiers::NONE)); assert_eq!(composer.attached_images.len(), 1); - assert!(composer.textarea.text().starts_with("[Image #1]")); + assert!( + composer + .textarea + .text() + .starts_with("[image_multibyte.png 10x5]") + ); } #[test] @@ -3424,10 +3555,10 @@ mod tests { let path1 = PathBuf::from("/tmp/image_dup1.png"); let path2 = PathBuf::from("/tmp/image_dup2.png"); - composer.attach_image(path1); + composer.attach_image(path1, 10, 5, "PNG"); // separate placeholders with a space for clarity composer.handle_paste(" ".into()); - composer.attach_image(path2.clone()); + composer.attach_image(path2.clone(), 10, 5, "PNG"); let placeholder1 = composer.attached_images[0].placeholder.clone(); let placeholder2 = composer.attached_images[1].placeholder.clone(); @@ -3442,60 +3573,24 @@ mod tests { let new_text = composer.textarea.text().to_string(); assert_eq!( 0, - new_text.matches(&placeholder2).count(), - "second placeholder was relabeled" + new_text.matches(&placeholder1).count(), + "first placeholder removed" ); assert_eq!( 1, - new_text.matches("[Image #1]").count(), - "remaining placeholder relabeled to #1" + new_text.matches(&placeholder2).count(), + "second placeholder remains" ); assert_eq!( vec![AttachedImage { path: path2, - placeholder: "[Image #1]".to_string() + placeholder: "[image_dup2.png 10x5]".to_string() }], composer.attached_images, "one image mapping remains" ); } - #[test] - fn deleting_first_text_element_renumbers_following_text_element() { - use crossterm::event::KeyCode; - use crossterm::event::KeyEvent; - use crossterm::event::KeyModifiers; - - let (tx, _rx) = unbounded_channel::(); - let sender = AppEventSender::new(tx); - let mut composer = ChatComposer::new( - true, - sender, - false, - "Ask Codex to do anything".to_string(), - false, - ); - - let path1 = PathBuf::from("/tmp/image_first.png"); - let path2 = PathBuf::from("/tmp/image_second.png"); - - // Insert two adjacent atomic elements. - composer.attach_image(path1); - composer.attach_image(path2.clone()); - assert_eq!(composer.textarea.text(), "[Image #1][Image #2]"); - assert_eq!(composer.attached_images.len(), 2); - - // Delete the first element using normal textarea editing (Delete at cursor start). - composer.textarea.set_cursor(0); - composer.handle_key_event(KeyEvent::new(KeyCode::Delete, KeyModifiers::NONE)); - - // Remaining image should be renumbered and the textarea element updated. - assert_eq!(composer.attached_images.len(), 1); - assert_eq!(composer.attached_images[0].path, path2); - assert_eq!(composer.attached_images[0].placeholder, "[Image #1]"); - assert_eq!(composer.textarea.text(), "[Image #1]"); - } - #[test] fn pasting_filepath_attaches_image() { let tmp = tempdir().expect("create TempDir"); @@ -3516,7 +3611,12 @@ mod tests { let needs_redraw = composer.handle_paste(tmp_path.to_string_lossy().to_string()); assert!(needs_redraw); - assert!(composer.textarea.text().starts_with("[Image #1] ")); + assert!( + composer + .textarea + .text() + .starts_with("[codex_tui_test_paste_image.png 3x2] ") + ); let imgs = composer.take_recent_submission_images(); assert_eq!(imgs, vec![tmp_path]); @@ -4215,7 +4315,6 @@ mod tests { "'/zzz' should not activate slash popup because it is not a prefix of any built-in command" ); } - #[test] fn input_disabled_ignores_keypresses_and_hides_cursor() { use crossterm::event::KeyCode; diff --git a/codex-rs/tui2/src/bottom_pane/mod.rs b/codex-rs/tui2/src/bottom_pane/mod.rs index 887fb3e64d..8258d6b50d 100644 --- a/codex-rs/tui2/src/bottom_pane/mod.rs +++ b/codex-rs/tui2/src/bottom_pane/mod.rs @@ -529,9 +529,16 @@ impl BottomPane { self.request_redraw(); } - pub(crate) fn attach_image(&mut self, path: PathBuf) { + pub(crate) fn attach_image( + &mut self, + path: PathBuf, + width: u32, + height: u32, + format_label: &str, + ) { if self.view_stack.is_empty() { - self.composer.attach_image(path); + self.composer + .attach_image(path, width, height, format_label); self.request_redraw(); } } diff --git a/codex-rs/tui2/src/chatwidget.rs b/codex-rs/tui2/src/chatwidget.rs index f5af1bfe67..ce27581872 100644 --- a/codex-rs/tui2/src/chatwidget.rs +++ b/codex-rs/tui2/src/chatwidget.rs @@ -1458,7 +1458,7 @@ impl ChatWidget { info.height, info.encoded_format.label() ); - self.attach_image(path); + self.attach_image(path, info.width, info.height, info.encoded_format.label()); } Err(err) => { tracing::warn!("failed to paste image: {err}"); @@ -1511,9 +1511,16 @@ impl ChatWidget { } } - pub(crate) fn attach_image(&mut self, path: PathBuf) { + pub(crate) fn attach_image( + &mut self, + path: PathBuf, + width: u32, + height: u32, + format_label: &str, + ) { tracing::info!("attach_image path={path:?}"); - self.bottom_pane.attach_image(path); + self.bottom_pane + .attach_image(path, width, height, format_label); self.request_redraw(); } diff --git a/codex-rs/tui2/src/chatwidget/tests.rs b/codex-rs/tui2/src/chatwidget/tests.rs index 9b7a825e0e..e12e64e72d 100644 --- a/codex-rs/tui2/src/chatwidget/tests.rs +++ b/codex-rs/tui2/src/chatwidget/tests.rs @@ -1065,7 +1065,7 @@ async fn ctrl_c_cleared_prompt_is_recoverable_via_history() { chat.bottom_pane.insert_str("draft message "); chat.bottom_pane - .attach_image(PathBuf::from("/tmp/preview.png")); + .attach_image(PathBuf::from("/tmp/preview.png"), 10, 10, "PNG"); let placeholder = "[Image #1]"; assert!( chat.bottom_pane.composer_text().ends_with(placeholder),