diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 96e90c5cfa62..66d6e225029a 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -1540,6 +1540,24 @@ impl Session { } } + /// Returns the input if there was no task running to inject into + pub async fn inject_response_items( + &self, + input: Vec, + ) -> Result<(), Vec> { + let mut active = self.active_turn.lock().await; + match active.as_mut() { + Some(at) => { + let mut ts = at.turn_state.lock().await; + for item in input { + ts.push_pending_input(item); + } + Ok(()) + } + None => Err(input), + } + } + pub async fn get_pending_input(&self) -> Vec { let mut active = self.active_turn.lock().await; match active.as_mut() { diff --git a/codex-rs/core/src/event_mapping.rs b/codex-rs/core/src/event_mapping.rs index 6ab6291a4bb6..fe592236c116 100644 --- a/codex-rs/core/src/event_mapping.rs +++ b/codex-rs/core/src/event_mapping.rs @@ -9,6 +9,10 @@ use codex_protocol::models::ReasoningItemContent; use codex_protocol::models::ReasoningItemReasoningSummary; use codex_protocol::models::ResponseItem; use codex_protocol::models::WebSearchAction; +use codex_protocol::models::is_image_close_tag_text; +use codex_protocol::models::is_image_open_tag_text; +use codex_protocol::models::is_local_image_close_tag_text; +use codex_protocol::models::is_local_image_open_tag_text; use codex_protocol::user_input::UserInput; use tracing::warn; use uuid::Uuid; @@ -32,9 +36,17 @@ fn parse_user_message(message: &[ContentItem]) -> Option { let mut content: Vec = Vec::new(); - for content_item in message.iter() { + for (idx, content_item) in message.iter().enumerate() { match content_item { ContentItem::InputText { text } => { + if (is_local_image_open_tag_text(text) || is_image_open_tag_text(text)) + && (matches!(message.get(idx + 1), Some(ContentItem::InputImage { .. }))) + || (idx > 0 + && (is_local_image_close_tag_text(text) || is_image_close_tag_text(text)) + && matches!(message.get(idx - 1), Some(ContentItem::InputImage { .. }))) + { + continue; + } if is_session_prefix(text) || is_user_shell_command_text(text) { return None; } @@ -177,6 +189,80 @@ mod tests { } } + #[test] + fn skips_local_image_label_text() { + let image_url = "data:image/png;base64,abc".to_string(); + let label = codex_protocol::models::local_image_open_tag_text(1); + let user_text = "Please review this image.".to_string(); + + let item = ResponseItem::Message { + id: None, + role: "user".to_string(), + content: vec![ + ContentItem::InputText { text: label }, + ContentItem::InputImage { + image_url: image_url.clone(), + }, + ContentItem::InputText { + text: "".to_string(), + }, + ContentItem::InputText { + text: user_text.clone(), + }, + ], + }; + + let turn_item = parse_turn_item(&item).expect("expected user message turn item"); + + match turn_item { + TurnItem::UserMessage(user) => { + let expected_content = vec![ + UserInput::Image { image_url }, + UserInput::Text { text: user_text }, + ]; + assert_eq!(user.content, expected_content); + } + other => panic!("expected TurnItem::UserMessage, got {other:?}"), + } + } + + #[test] + fn skips_unnamed_image_label_text() { + let image_url = "data:image/png;base64,abc".to_string(); + let label = codex_protocol::models::image_open_tag_text(); + let user_text = "Please review this image.".to_string(); + + let item = ResponseItem::Message { + id: None, + role: "user".to_string(), + content: vec![ + ContentItem::InputText { text: label }, + ContentItem::InputImage { + image_url: image_url.clone(), + }, + ContentItem::InputText { + text: codex_protocol::models::image_close_tag_text(), + }, + ContentItem::InputText { + text: user_text.clone(), + }, + ], + }; + + let turn_item = parse_turn_item(&item).expect("expected user message turn item"); + + match turn_item { + TurnItem::UserMessage(user) => { + let expected_content = vec![ + UserInput::Image { image_url }, + UserInput::Text { text: user_text }, + ]; + assert_eq!(user.content, expected_content); + } + other => panic!("expected TurnItem::UserMessage, got {other:?}"), + } + } + #[test] fn skips_user_instructions_and_env() { let items = vec![ diff --git a/codex-rs/core/src/tools/handlers/view_image.rs b/codex-rs/core/src/tools/handlers/view_image.rs index 3c0670e3e047..87dd7207b1c1 100644 --- a/codex-rs/core/src/tools/handlers/view_image.rs +++ b/codex-rs/core/src/tools/handlers/view_image.rs @@ -11,7 +11,9 @@ use crate::tools::context::ToolPayload; use crate::tools::handlers::parse_arguments; use crate::tools::registry::ToolHandler; use crate::tools::registry::ToolKind; -use codex_protocol::user_input::UserInput; +use codex_protocol::models::ContentItem; +use codex_protocol::models::ResponseInputItem; +use codex_protocol::models::local_image_content_items_with_label_number; pub struct ViewImageHandler; @@ -63,8 +65,15 @@ impl ToolHandler for ViewImageHandler { } let event_path = abs_path.clone(); + let content: Vec = + local_image_content_items_with_label_number(&abs_path, None); + let input = ResponseInputItem::Message { + role: "user".to_string(), + content, + }; + session - .inject_input(vec![UserInput::LocalImage { path: abs_path }]) + .inject_response_items(vec![input]) .await .map_err(|_| { FunctionCallError::RespondToModel( diff --git a/codex-rs/core/src/tools/spec.rs b/codex-rs/core/src/tools/spec.rs index 846025d58e25..af293dc09c02 100644 --- a/codex-rs/core/src/tools/spec.rs +++ b/codex-rs/core/src/tools/spec.rs @@ -6,6 +6,7 @@ use crate::tools::handlers::PLAN_TOOL; use crate::tools::handlers::apply_patch::create_apply_patch_freeform_tool; use crate::tools::handlers::apply_patch::create_apply_patch_json_tool; use crate::tools::registry::ToolRegistryBuilder; +use codex_protocol::models::VIEW_IMAGE_TOOL_NAME; use codex_protocol::openai_models::ApplyPatchToolType; use codex_protocol::openai_models::ConfigShellToolType; use codex_protocol::openai_models::ModelInfo; @@ -403,10 +404,9 @@ fn create_view_image_tool() -> ToolSpec { ); ToolSpec::Function(ResponsesApiTool { - name: "view_image".to_string(), - description: - "Attach a local image (by filesystem path) to the thread context for this turn." - .to_string(), + name: VIEW_IMAGE_TOOL_NAME.to_string(), + description: "View a local image from the filesystem (only use if given a full filepath by the user, and the image isn't already attached to the thread context within tags)." + .to_string(), strict: false, parameters: JsonSchema::Object { properties, diff --git a/codex-rs/core/tests/suite/view_image.rs b/codex-rs/core/tests/suite/view_image.rs index 6c0f6dcc80c0..4815f9cc58e8 100644 --- a/codex-rs/core/tests/suite/view_image.rs +++ b/codex-rs/core/tests/suite/view_image.rs @@ -208,6 +208,20 @@ async fn view_image_tool_attaches_local_image() -> anyhow::Result<()> { let image_message = find_image_message(&body).expect("pending input image message not included in request"); + let content_items = image_message + .get("content") + .and_then(Value::as_array) + .expect("image message has content array"); + assert_eq!( + content_items.len(), + 1, + "view_image should inject only the image content item (no tag/label text)" + ); + assert_eq!( + content_items[0].get("type").and_then(Value::as_str), + Some("input_image"), + "view_image should inject only an input_image content item" + ); let image_url = image_message .get("content") .and_then(Value::as_array) diff --git a/codex-rs/protocol/src/models.rs b/codex-rs/protocol/src/models.rs index 69a682f2df41..bec2a35f5a4d 100644 --- a/codex-rs/protocol/src/models.rs +++ b/codex-rs/protocol/src/models.rs @@ -180,6 +180,48 @@ fn local_image_error_placeholder( } } +pub const VIEW_IMAGE_TOOL_NAME: &str = "view_image"; + +const IMAGE_OPEN_TAG: &str = ""; +const IMAGE_CLOSE_TAG: &str = ""; +const LOCAL_IMAGE_OPEN_TAG_PREFIX: &str = ""; +const LOCAL_IMAGE_CLOSE_TAG: &str = IMAGE_CLOSE_TAG; + +pub fn image_open_tag_text() -> String { + IMAGE_OPEN_TAG.to_string() +} + +pub fn image_close_tag_text() -> String { + IMAGE_CLOSE_TAG.to_string() +} + +pub fn local_image_label_text(label_number: usize) -> String { + format!("[Image #{label_number}]") +} + +pub fn local_image_open_tag_text(label_number: usize) -> String { + let label = local_image_label_text(label_number); + format!("{LOCAL_IMAGE_OPEN_TAG_PREFIX}{label}{LOCAL_IMAGE_OPEN_TAG_SUFFIX}") +} + +pub fn is_local_image_open_tag_text(text: &str) -> bool { + text.strip_prefix(LOCAL_IMAGE_OPEN_TAG_PREFIX) + .is_some_and(|rest| rest.ends_with(LOCAL_IMAGE_OPEN_TAG_SUFFIX)) +} + +pub fn is_local_image_close_tag_text(text: &str) -> bool { + is_image_close_tag_text(text) +} + +pub fn is_image_open_tag_text(text: &str) -> bool { + text == IMAGE_OPEN_TAG +} + +pub fn is_image_close_tag_text(text: &str) -> bool { + text == IMAGE_CLOSE_TAG +} + fn invalid_image_error_placeholder( path: &std::path::Path, error: impl std::fmt::Display, @@ -203,6 +245,53 @@ fn unsupported_image_error_placeholder(path: &std::path::Path, mime: &str) -> Co } } +pub fn local_image_content_items_with_label_number( + path: &std::path::Path, + label_number: Option, +) -> Vec { + match load_and_resize_to_fit(path) { + Ok(image) => { + let mut items = Vec::with_capacity(3); + if let Some(label_number) = label_number { + items.push(ContentItem::InputText { + text: local_image_open_tag_text(label_number), + }); + } + items.push(ContentItem::InputImage { + image_url: image.into_data_url(), + }); + if label_number.is_some() { + items.push(ContentItem::InputText { + text: LOCAL_IMAGE_CLOSE_TAG.to_string(), + }); + } + items + } + Err(err) => { + if matches!(&err, ImageProcessingError::Read { .. }) { + vec![local_image_error_placeholder(path, &err)] + } else if err.is_invalid_image() { + vec![invalid_image_error_placeholder(path, &err)] + } else { + let Some(mime_guess) = mime_guess::from_path(path).first() else { + return vec![local_image_error_placeholder( + path, + "unsupported MIME type (unknown)", + )]; + }; + let mime = mime_guess.essence_str().to_owned(); + if !mime.starts_with("image/") { + return vec![local_image_error_placeholder( + path, + format!("unsupported MIME type `{mime}`"), + )]; + } + vec![unsupported_image_error_placeholder(path, &mime)] + } + } + } +} + impl From for ResponseItem { fn from(item: ResponseInputItem) -> Self { match item { @@ -296,41 +385,27 @@ 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 .into_iter() - .filter_map(|c| match c { - UserInput::Text { text } => Some(ContentItem::InputText { text }), - UserInput::Image { image_url } => Some(ContentItem::InputImage { image_url }), - UserInput::LocalImage { path } => match load_and_resize_to_fit(&path) { - Ok(image) => Some(ContentItem::InputImage { - image_url: image.into_data_url(), - }), - Err(err) => { - if matches!(&err, ImageProcessingError::Read { .. }) { - Some(local_image_error_placeholder(&path, &err)) - } else if err.is_invalid_image() { - Some(invalid_image_error_placeholder(&path, &err)) - } else { - let Some(mime_guess) = mime_guess::from_path(&path).first() else { - return Some(local_image_error_placeholder( - &path, - "unsupported MIME type (unknown)", - )); - }; - let mime = mime_guess.essence_str().to_owned(); - if !mime.starts_with("image/") { - return Some(local_image_error_placeholder( - &path, - format!("unsupported MIME type `{mime}`"), - )); - } - Some(unsupported_image_error_placeholder(&path, &mime)) - } - } - }, - UserInput::Skill { .. } => None, // Skill bodies are injected later in core + .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::LocalImage { path } => { + image_index += 1; + local_image_content_items_with_label_number(&path, Some(image_index)) + } + UserInput::Skill { .. } => Vec::new(), // Skill bodies are injected later in core }) .collect::>(), } @@ -770,6 +845,33 @@ mod tests { Ok(()) } + #[test] + fn wraps_image_user_input_with_tags() -> Result<()> { + let image_url = "data:image/png;base64,abc".to_string(); + + let item = ResponseInputItem::from(vec![UserInput::Image { + image_url: image_url.clone(), + }]); + + match item { + ResponseInputItem::Message { content, .. } => { + let expected = vec![ + ContentItem::InputText { + text: image_open_tag_text(), + }, + ContentItem::InputImage { image_url }, + ContentItem::InputText { + text: image_close_tag_text(), + }, + ]; + assert_eq!(content, expected); + } + 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/bottom_pane/chat_composer.rs b/codex-rs/tui/src/bottom_pane/chat_composer.rs index d3c93caa1566..1bed23ac371d 100644 --- a/codex-rs/tui/src/bottom_pane/chat_composer.rs +++ b/codex-rs/tui/src/bottom_pane/chat_composer.rs @@ -46,6 +46,7 @@ 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; @@ -59,7 +60,7 @@ use codex_core::skills::model::SkillMetadata; use codex_file_search::FileMatch; use std::cell::RefCell; use std::collections::HashMap; -use std::path::Path; +use std::collections::HashSet; use std::path::PathBuf; use std::time::Duration; use std::time::Instant; @@ -267,10 +268,12 @@ 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((w, h)) => { + Ok((width, height)) => { tracing::info!("OK: {pasted}"); - let format_label = pasted_image_format(&path_buf).label(); - self.attach_image(path_buf, w, h, format_label); + 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); true } Err(err) => { @@ -401,13 +404,9 @@ impl ChatComposer { } /// Attempt to start a burst by retro-capturing recent chars before the cursor. - pub fn attach_image(&mut self, path: PathBuf, width: u32, height: u32, _format_label: &str) { - 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}"); - let placeholder = self.next_image_placeholder(&base_placeholder); + pub fn attach_image(&mut self, path: PathBuf) { + let image_number = 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. self.textarea.insert_element(&placeholder); @@ -470,22 +469,6 @@ 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; - } - } - pub(crate) fn insert_str(&mut self, text: &str) { self.textarea.insert_str(text); self.sync_popups(); @@ -811,47 +794,43 @@ impl ChatComposer { if is_image { // Determine dimensions; if that fails fall back to normal path insertion. let path_buf = PathBuf::from(&sel_path); - 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; - - self.textarea.replace_range(start_idx..end_idx, ""); - self.textarea.set_cursor(start_idx); - - 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); + 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..]; + + // 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.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); + } } } else { // Non-image: inserting file path. @@ -1449,20 +1428,29 @@ impl ChatComposer { } } - // 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() + // 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() { return (InputResult::None, true); } - // Normal input handling + // 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()) + }; + self.textarea.input(input); - let text_after = self.textarea.text(); + + if let Some(elements_before) = elements_before { + self.reconcile_deleted_elements(elements_before); + } // Update paste-burst heuristic for plain Char (no Ctrl/Alt) events. let crossterm::event::KeyEvent { @@ -1484,176 +1472,69 @@ 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()); - } + (InputResult::None, true) + } - 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; + fn try_remove_image_element_at_cursor_start(&mut self) -> bool { + if self.attached_images.is_empty() { + return false; } - (InputResult::None, true) + 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 } - /// 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()); - - // 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; - } - let start = p - ph.len(); - if text.get(start..p) != Some(ph.as_str()) { - continue; - } + fn reconcile_deleted_elements(&mut self, elements_before: Vec) { + let elements_after: HashSet = + self.textarea.element_payloads().into_iter().collect(); - // 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; - } - } + 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); - // Remove the occ_before-th attached image that shares this placeholder label. - out = if let Some((remove_idx, _)) = self + if let Some(idx) = self .attached_images .iter() - .enumerate() - .filter(|(_, img2)| img2.placeholder == *ph) - .nth(occ_before) + .position(|img| img.placeholder == removed) { - 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())); - } + self.attached_images.remove(idx); + removed_any_image = true; } - 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; + if removed_any_image { + self.relabel_attached_images_and_update_placeholders(); } + } - // 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 + 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 { + continue; } - }) { - self.textarea.replace_range(p..p + placeholder.len(), ""); - self.pending_pastes.retain(|(ph, _)| ph != &placeholder); - return true; - } - false + self.attached_images[idx].placeholder = expected.clone(); + let _renamed = self.textarea.replace_element_payload(¤t, &expected); + } } fn handle_shortcut_overlay_key(&mut self, key_event: &KeyEvent) -> bool { @@ -3498,12 +3379,12 @@ mod tests { false, ); let path = PathBuf::from("/tmp/image1.png"); - composer.attach_image(path.clone(), 32, 16, "PNG"); + composer.attach_image(path.clone()); 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, "[image1.png 32x16] hi"), + InputResult::Submitted(text) => assert_eq!(text, "[Image #1] hi"), _ => panic!("expected Submitted"), } let imgs = composer.take_recent_submission_images(); @@ -3522,11 +3403,11 @@ mod tests { false, ); let path = PathBuf::from("/tmp/image2.png"); - composer.attach_image(path.clone(), 10, 5, "PNG"); + composer.attach_image(path.clone()); let (result, _) = composer.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE)); match result { - InputResult::Submitted(text) => assert_eq!(text, "[image2.png 10x5]"), + InputResult::Submitted(text) => assert_eq!(text, "[Image #1]"), _ => panic!("expected Submitted"), } let imgs = composer.take_recent_submission_images(); @@ -3547,21 +3428,15 @@ mod tests { false, ); let path = PathBuf::from("/tmp/image_dup.png"); - composer.attach_image(path.clone(), 10, 5, "PNG"); + composer.attach_image(path.clone()); composer.handle_paste(" ".into()); - composer.attach_image(path, 10, 5, "PNG"); + composer.attach_image(path); let text = composer.textarea.text().to_string(); - 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]" - ); + 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]"); } #[test] @@ -3576,7 +3451,7 @@ mod tests { false, ); let path = PathBuf::from("/tmp/image3.png"); - composer.attach_image(path.clone(), 20, 10, "PNG"); + composer.attach_image(path.clone()); let placeholder = composer.attached_images[0].placeholder.clone(); // Case 1: backspace at end @@ -3587,7 +3462,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, 20, 10, "PNG"); + composer.attach_image(path); 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) { @@ -3619,7 +3494,7 @@ mod tests { // Insert an image placeholder at the start let path = PathBuf::from("/tmp/image_multibyte.png"); - composer.attach_image(path, 10, 5, "PNG"); + composer.attach_image(path); // Add multibyte text after the placeholder composer.textarea.insert_str("日本語"); @@ -3628,16 +3503,11 @@ 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_multibyte.png 10x5]") - ); + assert!(composer.textarea.text().starts_with("[Image #1]")); } #[test] - fn deleting_one_of_duplicate_image_placeholders_removes_matching_entry() { + fn deleting_one_of_duplicate_image_placeholders_removes_one_entry() { let (tx, _rx) = unbounded_channel::(); let sender = AppEventSender::new(tx); let mut composer = ChatComposer::new( @@ -3651,10 +3521,10 @@ mod tests { let path1 = PathBuf::from("/tmp/image_dup1.png"); let path2 = PathBuf::from("/tmp/image_dup2.png"); - composer.attach_image(path1, 10, 5, "PNG"); + composer.attach_image(path1); // separate placeholders with a space for clarity composer.handle_paste(" ".into()); - composer.attach_image(path2.clone(), 10, 5, "PNG"); + composer.attach_image(path2.clone()); let placeholder1 = composer.attached_images[0].placeholder.clone(); let placeholder2 = composer.attached_images[1].placeholder.clone(); @@ -3668,25 +3538,66 @@ mod tests { let new_text = composer.textarea.text().to_string(); assert_eq!( - 0, + 1, new_text.matches(&placeholder1).count(), - "first placeholder removed" + "one placeholder remains after deletion" ); assert_eq!( - 1, + 0, new_text.matches(&placeholder2).count(), - "second placeholder remains" + "second placeholder was relabeled" + ); + assert_eq!( + 1, + new_text.matches("[Image #1]").count(), + "remaining placeholder relabeled to #1" ); assert_eq!( vec![AttachedImage { path: path2, - placeholder: "[image_dup2.png 10x5]".to_string() + placeholder: "[Image #1]".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"); @@ -3707,12 +3618,7 @@ mod tests { let needs_redraw = composer.handle_paste(tmp_path.to_string_lossy().to_string()); assert!(needs_redraw); - assert!( - composer - .textarea - .text() - .starts_with("[codex_tui_test_paste_image.png 3x2] ") - ); + assert!(composer.textarea.text().starts_with("[Image #1] ")); let imgs = composer.take_recent_submission_images(); assert_eq!(imgs, vec![tmp_path]); diff --git a/codex-rs/tui/src/bottom_pane/mod.rs b/codex-rs/tui/src/bottom_pane/mod.rs index fe626537ac43..97fa4f9d6580 100644 --- a/codex-rs/tui/src/bottom_pane/mod.rs +++ b/codex-rs/tui/src/bottom_pane/mod.rs @@ -541,16 +541,9 @@ impl BottomPane { self.request_redraw(); } - pub(crate) fn attach_image( - &mut self, - path: PathBuf, - width: u32, - height: u32, - format_label: &str, - ) { + pub(crate) fn attach_image(&mut self, path: PathBuf) { if self.view_stack.is_empty() { - self.composer - .attach_image(path, width, height, format_label); + self.composer.attach_image(path); self.request_redraw(); } } diff --git a/codex-rs/tui/src/bottom_pane/textarea.rs b/codex-rs/tui/src/bottom_pane/textarea.rs index 4fc673a11deb..f2ed40758dab 100644 --- a/codex-rs/tui/src/bottom_pane/textarea.rs +++ b/codex-rs/tui/src/bottom_pane/textarea.rs @@ -715,6 +715,88 @@ impl TextArea { // ===== Text elements support ===== + pub fn element_payloads(&self) -> Vec { + self.elements + .iter() + .filter_map(|e| self.text.get(e.range.clone()).map(str::to_string)) + .collect() + } + + pub fn element_payload_starting_at(&self, pos: usize) -> Option { + let pos = pos.min(self.text.len()); + let elem = self.elements.iter().find(|e| e.range.start == pos)?; + self.text.get(elem.range.clone()).map(str::to_string) + } + + /// Renames a single text element in-place, keeping it atomic. + /// + /// This is intended for cases where the element payload is an identifier (e.g. a placeholder) + /// that must be updated without converting the element back into normal text. + pub fn replace_element_payload(&mut self, old: &str, new: &str) -> bool { + let Some(idx) = self + .elements + .iter() + .position(|e| self.text.get(e.range.clone()) == Some(old)) + else { + return false; + }; + + let range = self.elements[idx].range.clone(); + let start = range.start; + let end = range.end; + if start > end || end > self.text.len() { + return false; + } + + let removed_len = end - start; + let inserted_len = new.len(); + let diff = inserted_len as isize - removed_len as isize; + + self.text.replace_range(range, new); + self.wrap_cache.replace(None); + self.preferred_col = None; + + // Update the modified element's range. + self.elements[idx].range = start..(start + inserted_len); + + // Shift element ranges that occur after the replaced element. + if diff != 0 { + for (j, e) in self.elements.iter_mut().enumerate() { + if j == idx { + continue; + } + if e.range.end <= start { + continue; + } + if e.range.start >= end { + e.range.start = ((e.range.start as isize) + diff) as usize; + e.range.end = ((e.range.end as isize) + diff) as usize; + continue; + } + + // Elements should not partially overlap each other; degrade gracefully by + // snapping anything intersecting the replaced range to the new bounds. + e.range.start = start.min(e.range.start); + e.range.end = (start + inserted_len).max(e.range.end.saturating_add_signed(diff)); + } + } + + // Update the cursor position to account for the edit. + self.cursor_pos = if self.cursor_pos < start { + self.cursor_pos + } else if self.cursor_pos <= end { + start + inserted_len + } else { + ((self.cursor_pos as isize) + diff) as usize + }; + self.cursor_pos = self.clamp_pos_to_nearest_boundary(self.cursor_pos); + + // Keep element ordering deterministic. + self.elements.sort_by_key(|e| e.range.start); + + true + } + pub fn insert_element(&mut self, text: &str) { let start = self.clamp_pos_for_insertion(self.cursor_pos); self.insert_str_at(start, text); diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index fa0be84a7451..ee64872efa7c 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -1590,12 +1590,13 @@ impl ChatWidget { { match paste_image_to_temp_png() { Ok((path, info)) => { - self.attach_image( - path, + tracing::debug!( + "pasted image size={}x{} format={}", info.width, info.height, - info.encoded_format.label(), + info.encoded_format.label() ); + self.attach_image(path); } Err(err) => { tracing::warn!("failed to paste image: {err}"); @@ -1648,18 +1649,9 @@ impl ChatWidget { } } - pub(crate) fn attach_image( - &mut self, - path: PathBuf, - width: u32, - height: u32, - format_label: &str, - ) { - tracing::info!( - "attach_image path={path:?} width={width} height={height} format={format_label}", - ); - self.bottom_pane - .attach_image(path, width, height, format_label); + pub(crate) fn attach_image(&mut self, path: PathBuf) { + tracing::info!("attach_image path={path:?}"); + self.bottom_pane.attach_image(path); self.request_redraw(); } @@ -1964,14 +1956,14 @@ impl ChatWidget { return; } - if !text.is_empty() { - items.push(UserInput::Text { text: text.clone() }); - } - for path in image_paths { items.push(UserInput::LocalImage { path }); } + if !text.is_empty() { + items.push(UserInput::Text { text: text.clone() }); + } + if let Some(skills) = self.bottom_pane.skills() { let skill_mentions = find_skill_mentions(&text, skills); for skill in skill_mentions { diff --git a/codex-rs/tui/src/chatwidget/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index edfb4e1d411c..42af29759b76 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -1107,8 +1107,8 @@ 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"), 24, 42, "png"); - let placeholder = "[preview.png 24x42]"; + .attach_image(PathBuf::from("/tmp/preview.png")); + let placeholder = "[Image #1]"; assert!( chat.bottom_pane.composer_text().ends_with(placeholder), "expected placeholder {placeholder:?} in composer text" diff --git a/codex-rs/tui2/src/bottom_pane/chat_composer.rs b/codex-rs/tui2/src/bottom_pane/chat_composer.rs index 6198d0a57a28..e1d157f54210 100644 --- a/codex-rs/tui2/src/bottom_pane/chat_composer.rs +++ b/codex-rs/tui2/src/bottom_pane/chat_composer.rs @@ -49,6 +49,7 @@ 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; @@ -62,7 +63,7 @@ use codex_core::skills::model::SkillMetadata; use codex_file_search::FileMatch; use std::cell::RefCell; use std::collections::HashMap; -use std::path::Path; +use std::collections::HashSet; use std::path::PathBuf; use std::time::Duration; use std::time::Instant; @@ -280,10 +281,12 @@ 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((w, h)) => { + Ok((width, height)) => { tracing::info!("OK: {pasted}"); - let format_label = pasted_image_format(&path_buf).label(); - self.attach_image(path_buf, w, h, format_label); + 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); true } Err(err) => { @@ -335,12 +338,9 @@ impl ChatComposer { } /// Attempt to start a burst by retro-capturing recent chars before the cursor. - pub fn attach_image(&mut self, path: PathBuf, width: u32, height: u32, _format_label: &str) { - let file_label = path - .file_name() - .map(|name| name.to_string_lossy().into_owned()) - .unwrap_or_else(|| "image".to_string()); - let placeholder = format!("[{file_label} {width}x{height}]"); + pub fn attach_image(&mut self, path: PathBuf) { + let image_number = 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. self.textarea.insert_element(&placeholder); @@ -728,47 +728,43 @@ impl ChatComposer { if is_image { // Determine dimensions; if that fails fall back to normal path insertion. let path_buf = PathBuf::from(&sel_path); - 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; - - self.textarea.replace_range(start_idx..end_idx, ""); - self.textarea.set_cursor(start_idx); - - 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); + 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..]; + + // 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.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); + } } } else { // Non-image: inserting file path. @@ -1366,20 +1362,28 @@ impl ChatComposer { } } - // 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() + // 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() { return (InputResult::None, true); } - // Normal input handling + // 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()) + }; + self.textarea.input(input); - let text_after = self.textarea.text(); + + if let Some(elements_before) = elements_before { + self.reconcile_deleted_elements(elements_before); + } // Update paste-burst heuristic for plain Char (no Ctrl/Alt) events. let crossterm::event::KeyEvent { @@ -1401,176 +1405,69 @@ 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()); - } + (InputResult::None, true) + } - 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; + fn try_remove_image_element_at_cursor_start(&mut self) -> bool { + if self.attached_images.is_empty() { + return false; } - (InputResult::None, true) + 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 } - /// 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()); - - // 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; - } - let start = p - ph.len(); - if text.get(start..p) != Some(ph.as_str()) { - continue; - } + fn reconcile_deleted_elements(&mut self, elements_before: Vec) { + let elements_after: HashSet = + self.textarea.element_payloads().into_iter().collect(); - // 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; - } - } + 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); - // Remove the occ_before-th attached image that shares this placeholder label. - out = if let Some((remove_idx, _)) = self + if let Some(idx) = self .attached_images .iter() - .enumerate() - .filter(|(_, img2)| img2.placeholder == *ph) - .nth(occ_before) + .position(|img| img.placeholder == removed) { - 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())); - } + self.attached_images.remove(idx); + removed_any_image = true; } - 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; + if removed_any_image { + self.relabel_attached_images_and_update_placeholders(); } + } - // 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 + 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 { + continue; } - }) { - self.textarea.replace_range(p..p + placeholder.len(), ""); - self.pending_pastes.retain(|(ph, _)| ph != &placeholder); - return true; - } - false + self.attached_images[idx].placeholder = expected.clone(); + let _renamed = self.textarea.replace_element_payload(¤t, &expected); + } } fn handle_shortcut_overlay_key(&mut self, key_event: &KeyEvent) -> bool { @@ -3419,12 +3316,12 @@ mod tests { false, ); let path = PathBuf::from("/tmp/image1.png"); - composer.attach_image(path.clone(), 32, 16, "PNG"); + composer.attach_image(path.clone()); 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, "[image1.png 32x16] hi"), + InputResult::Submitted(text) => assert_eq!(text, "[Image #1] hi"), _ => panic!("expected Submitted"), } let imgs = composer.take_recent_submission_images(); @@ -3443,11 +3340,11 @@ mod tests { false, ); let path = PathBuf::from("/tmp/image2.png"); - composer.attach_image(path.clone(), 10, 5, "PNG"); + composer.attach_image(path.clone()); let (result, _) = composer.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE)); match result { - InputResult::Submitted(text) => assert_eq!(text, "[image2.png 10x5]"), + InputResult::Submitted(text) => assert_eq!(text, "[Image #1]"), _ => panic!("expected Submitted"), } let imgs = composer.take_recent_submission_images(); @@ -3468,7 +3365,7 @@ mod tests { false, ); let path = PathBuf::from("/tmp/image3.png"); - composer.attach_image(path.clone(), 20, 10, "PNG"); + composer.attach_image(path.clone()); let placeholder = composer.attached_images[0].placeholder.clone(); // Case 1: backspace at end @@ -3479,7 +3376,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, 20, 10, "PNG"); + composer.attach_image(path); 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) { @@ -3511,7 +3408,7 @@ mod tests { // Insert an image placeholder at the start let path = PathBuf::from("/tmp/image_multibyte.png"); - composer.attach_image(path, 10, 5, "PNG"); + composer.attach_image(path); // Add multibyte text after the placeholder composer.textarea.insert_str("日本語"); @@ -3520,12 +3417,7 @@ 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_multibyte.png 10x5]") - ); + assert!(composer.textarea.text().starts_with("[Image #1]")); } #[test] @@ -3543,10 +3435,10 @@ mod tests { let path1 = PathBuf::from("/tmp/image_dup1.png"); let path2 = PathBuf::from("/tmp/image_dup2.png"); - composer.attach_image(path1, 10, 5, "PNG"); + composer.attach_image(path1); // separate placeholders with a space for clarity composer.handle_paste(" ".into()); - composer.attach_image(path2.clone(), 10, 5, "PNG"); + composer.attach_image(path2.clone()); let placeholder1 = composer.attached_images[0].placeholder.clone(); let placeholder2 = composer.attached_images[1].placeholder.clone(); @@ -3561,24 +3453,60 @@ mod tests { let new_text = composer.textarea.text().to_string(); assert_eq!( 0, - new_text.matches(&placeholder1).count(), - "first placeholder removed" + new_text.matches(&placeholder2).count(), + "second placeholder was relabeled" ); assert_eq!( 1, - new_text.matches(&placeholder2).count(), - "second placeholder remains" + new_text.matches("[Image #1]").count(), + "remaining placeholder relabeled to #1" ); assert_eq!( vec![AttachedImage { path: path2, - placeholder: "[image_dup2.png 10x5]".to_string() + placeholder: "[Image #1]".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"); @@ -3599,12 +3527,7 @@ mod tests { let needs_redraw = composer.handle_paste(tmp_path.to_string_lossy().to_string()); assert!(needs_redraw); - assert!( - composer - .textarea - .text() - .starts_with("[codex_tui_test_paste_image.png 3x2] ") - ); + assert!(composer.textarea.text().starts_with("[Image #1] ")); let imgs = composer.take_recent_submission_images(); assert_eq!(imgs, vec![tmp_path]); diff --git a/codex-rs/tui2/src/bottom_pane/mod.rs b/codex-rs/tui2/src/bottom_pane/mod.rs index 4b6caf0d1aa8..b5ef2ee069f8 100644 --- a/codex-rs/tui2/src/bottom_pane/mod.rs +++ b/codex-rs/tui2/src/bottom_pane/mod.rs @@ -528,16 +528,9 @@ impl BottomPane { self.request_redraw(); } - pub(crate) fn attach_image( - &mut self, - path: PathBuf, - width: u32, - height: u32, - format_label: &str, - ) { + pub(crate) fn attach_image(&mut self, path: PathBuf) { if self.view_stack.is_empty() { - self.composer - .attach_image(path, width, height, format_label); + self.composer.attach_image(path); self.request_redraw(); } } diff --git a/codex-rs/tui2/src/bottom_pane/textarea.rs b/codex-rs/tui2/src/bottom_pane/textarea.rs index 4fc673a11deb..f2ed40758dab 100644 --- a/codex-rs/tui2/src/bottom_pane/textarea.rs +++ b/codex-rs/tui2/src/bottom_pane/textarea.rs @@ -715,6 +715,88 @@ impl TextArea { // ===== Text elements support ===== + pub fn element_payloads(&self) -> Vec { + self.elements + .iter() + .filter_map(|e| self.text.get(e.range.clone()).map(str::to_string)) + .collect() + } + + pub fn element_payload_starting_at(&self, pos: usize) -> Option { + let pos = pos.min(self.text.len()); + let elem = self.elements.iter().find(|e| e.range.start == pos)?; + self.text.get(elem.range.clone()).map(str::to_string) + } + + /// Renames a single text element in-place, keeping it atomic. + /// + /// This is intended for cases where the element payload is an identifier (e.g. a placeholder) + /// that must be updated without converting the element back into normal text. + pub fn replace_element_payload(&mut self, old: &str, new: &str) -> bool { + let Some(idx) = self + .elements + .iter() + .position(|e| self.text.get(e.range.clone()) == Some(old)) + else { + return false; + }; + + let range = self.elements[idx].range.clone(); + let start = range.start; + let end = range.end; + if start > end || end > self.text.len() { + return false; + } + + let removed_len = end - start; + let inserted_len = new.len(); + let diff = inserted_len as isize - removed_len as isize; + + self.text.replace_range(range, new); + self.wrap_cache.replace(None); + self.preferred_col = None; + + // Update the modified element's range. + self.elements[idx].range = start..(start + inserted_len); + + // Shift element ranges that occur after the replaced element. + if diff != 0 { + for (j, e) in self.elements.iter_mut().enumerate() { + if j == idx { + continue; + } + if e.range.end <= start { + continue; + } + if e.range.start >= end { + e.range.start = ((e.range.start as isize) + diff) as usize; + e.range.end = ((e.range.end as isize) + diff) as usize; + continue; + } + + // Elements should not partially overlap each other; degrade gracefully by + // snapping anything intersecting the replaced range to the new bounds. + e.range.start = start.min(e.range.start); + e.range.end = (start + inserted_len).max(e.range.end.saturating_add_signed(diff)); + } + } + + // Update the cursor position to account for the edit. + self.cursor_pos = if self.cursor_pos < start { + self.cursor_pos + } else if self.cursor_pos <= end { + start + inserted_len + } else { + ((self.cursor_pos as isize) + diff) as usize + }; + self.cursor_pos = self.clamp_pos_to_nearest_boundary(self.cursor_pos); + + // Keep element ordering deterministic. + self.elements.sort_by_key(|e| e.range.start); + + true + } + pub fn insert_element(&mut self, text: &str) { let start = self.clamp_pos_for_insertion(self.cursor_pos); self.insert_str_at(start, text); diff --git a/codex-rs/tui2/src/chatwidget.rs b/codex-rs/tui2/src/chatwidget.rs index 4ee3aa5f1f43..92a51f9a930a 100644 --- a/codex-rs/tui2/src/chatwidget.rs +++ b/codex-rs/tui2/src/chatwidget.rs @@ -1448,12 +1448,13 @@ impl ChatWidget { { match paste_image_to_temp_png() { Ok((path, info)) => { - self.attach_image( - path, + tracing::debug!( + "pasted image size={}x{} format={}", info.width, info.height, - info.encoded_format.label(), + info.encoded_format.label() ); + self.attach_image(path); } Err(err) => { tracing::warn!("failed to paste image: {err}"); @@ -1506,18 +1507,9 @@ impl ChatWidget { } } - pub(crate) fn attach_image( - &mut self, - path: PathBuf, - width: u32, - height: u32, - format_label: &str, - ) { - tracing::info!( - "attach_image path={path:?} width={width} height={height} format={format_label}", - ); - self.bottom_pane - .attach_image(path, width, height, format_label); + pub(crate) fn attach_image(&mut self, path: PathBuf) { + tracing::info!("attach_image path={path:?}"); + self.bottom_pane.attach_image(path); self.request_redraw(); } @@ -1769,14 +1761,14 @@ impl ChatWidget { return; } - if !text.is_empty() { - items.push(UserInput::Text { text: text.clone() }); - } - for path in image_paths { items.push(UserInput::LocalImage { path }); } + if !text.is_empty() { + items.push(UserInput::Text { text: text.clone() }); + } + if let Some(skills) = self.bottom_pane.skills() { let skill_mentions = find_skill_mentions(&text, skills); for skill in skill_mentions { diff --git a/codex-rs/tui2/src/chatwidget/tests.rs b/codex-rs/tui2/src/chatwidget/tests.rs index 09f5073e75e2..e287344ccc89 100644 --- a/codex-rs/tui2/src/chatwidget/tests.rs +++ b/codex-rs/tui2/src/chatwidget/tests.rs @@ -1056,8 +1056,8 @@ 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"), 24, 42, "png"); - let placeholder = "[preview.png 24x42]"; + .attach_image(PathBuf::from("/tmp/preview.png")); + let placeholder = "[Image #1]"; assert!( chat.bottom_pane.composer_text().ends_with(placeholder), "expected placeholder {placeholder:?} in composer text"