Skip to content

tui: expand image placeholders to full paths for models (keep basenames for users) #8737

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mikita-oai wants to merge 4 commits into
base: main
Choose a base branch
from
Open

tui: expand image placeholders to full paths for models (keep basenames for users) #8737

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
77c23a6
tui: include full image paths in attachment placeholders
mikita-oai Jan 5, 2026
7486201
tui: separate user-visible vs model-visible image placeholders
mikita-oai Jan 6, 2026
c5925b6
rollback to original UX: only basenames in user-visible placeholder
mikita-oai Jan 6, 2026
9e1cfd1
tui: fix clippy after image placeholder refactor
mikita-oai Jan 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
132 changes: 94 additions & 38 deletions codex-rs/tui/src/bottom_pane/chat_composer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,8 @@ pub enum InputResult {

#[derive(Clone, Debug, PartialEq)]
struct AttachedImage {
placeholder: String,
display_placeholder: String,
model_placeholder: String,
path: PathBuf,
}

Expand Down Expand Up @@ -291,7 +292,11 @@ impl ChatComposer {

// Count placeholder occurrences in the new text.
let mut placeholder_counts: HashMap<String, usize> = HashMap::new();
for placeholder in self.attached_images.iter().map(|img| &img.placeholder) {
for placeholder in self
.attached_images
.iter()
.map(|img| &img.display_placeholder)
{
if placeholder_counts.contains_key(placeholder) {
continue;
}
Expand All @@ -304,7 +309,7 @@ impl ChatComposer {
// Keep attachments only while we have matching occurrences left.
let mut kept_images = Vec::new();
for img in self.attached_images.drain(..) {
if let Some(count) = placeholder_counts.get_mut(&img.placeholder)
if let Some(count) = placeholder_counts.get_mut(&img.display_placeholder)
&& *count > 0
{
*count -= 1;
Expand All @@ -317,7 +322,9 @@ impl ChatComposer {
self.textarea.set_text("");
let mut remaining: HashMap<&str, usize> = HashMap::new();
for img in &self.attached_images {
*remaining.entry(img.placeholder.as_str()).or_insert(0) += 1;
*remaining
.entry(img.display_placeholder.as_str())
.or_insert(0) += 1;
}

let mut occurrences: Vec<(usize, &str)> = Vec::new();
Expand Down Expand Up @@ -396,24 +403,43 @@ 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);
let display_label = Self::display_label_for_image_placeholder(&path);
let full_label = path.display().to_string();

let display_base = format!("{display_label} {width}x{height}");
let model_base = format!("{full_label} {width}x{height}");

let display_placeholder = self.next_image_placeholder(&display_base);
let model_placeholder = Self::model_placeholder_from_display_placeholder(
&display_placeholder,
&display_base,
&model_base,
);
// Insert as an element to match large paste placeholder behavior:
// styled distinctly and treated atomically for cursor/mutations.
self.textarea.insert_element(&placeholder);
self.attached_images
.push(AttachedImage { placeholder, path });
self.textarea.insert_element(&display_placeholder);
self.attached_images.push(AttachedImage {
display_placeholder,
model_placeholder,
path,
});
}

pub fn take_recent_submission_images(&mut self) -> Vec<PathBuf> {
let images = std::mem::take(&mut self.attached_images);
images.into_iter().map(|img| img.path).collect()
}

pub fn expand_attached_image_placeholders_for_model(&self, display_text: &str) -> String {
let mut text = display_text.to_string();
for img in &self.attached_images {
if text.contains(&img.display_placeholder) {
text = text.replace(&img.display_placeholder, &img.model_placeholder);
}
}
text
}

pub(crate) fn flush_paste_burst_if_due(&mut self) -> bool {
self.handle_paste_burst_flush(Instant::now())
}
Expand Down Expand Up @@ -480,6 +506,35 @@ impl ChatComposer {
}
}

fn display_label_for_image_placeholder(path: &Path) -> String {
// Keep the UI placeholder compact: show only the basename (pre-existing behavior).
// The model can still receive the full path via model_placeholder expansion.
path.file_name()
.and_then(|name| name.to_str())
.map(str::to_string)
.unwrap_or_else(|| "image".to_string())
}

fn model_placeholder_from_display_placeholder(
display_placeholder: &str,
display_base: &str,
model_base: &str,
) -> String {
let Some(inner) = display_placeholder
.strip_prefix('[')
.and_then(|s| s.strip_suffix(']'))
else {
return format!("[{model_base}]");
};

if let Some(tail) = inner.strip_prefix(display_base) {
format!("[{model_base}{tail}]")
} else {
// Fallback: preserve the placeholder bracket shape even if we can't recover the suffix.
format!("[{model_base}]")
}
}

pub(crate) fn insert_str(&mut self, text: &str) {
self.textarea.insert_str(text);
self.sync_popups();
Expand Down Expand Up @@ -1438,15 +1493,15 @@ impl ChatComposer {
let mut needed: HashMap<String, usize> = HashMap::new();
for img in &self.attached_images {
needed
.entry(img.placeholder.clone())
.or_insert_with(|| text_after.matches(&img.placeholder).count());
.entry(img.display_placeholder.clone())
.or_insert_with(|| text_after.matches(&img.display_placeholder).count());
}

let mut used: HashMap<String, usize> = HashMap::new();
let mut kept: Vec<AttachedImage> = 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);
let total_needed = *needed.get(&img.display_placeholder).unwrap_or(&0);
let used_count = used.entry(img.display_placeholder.clone()).or_insert(0);
if *used_count < total_needed {
kept.push(img);
*used_count += 1;
Expand All @@ -1470,7 +1525,7 @@ impl ChatComposer {
// 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;
let ph = &img.display_placeholder;
if p < ph.len() {
continue;
}
Expand Down Expand Up @@ -1500,7 +1555,7 @@ impl ChatComposer {
.attached_images
.iter()
.enumerate()
.filter(|(_, img2)| img2.placeholder == *ph)
.filter(|(_, img2)| img2.display_placeholder == *ph)
.nth(occ_before)
{
Some((remove_idx, ph.clone()))
Expand All @@ -1519,7 +1574,7 @@ impl ChatComposer {
// let result = 'out: {
let out: Option<(usize, String)> = 'out: {
for (i, img) in self.attached_images.iter().enumerate() {
let ph = &img.placeholder;
let ph = &img.display_placeholder;
if p + ph.len() > text.len() {
continue;
}
Expand Down Expand Up @@ -1547,7 +1602,7 @@ impl ChatComposer {
.attached_images
.iter()
.enumerate()
.filter(|(_, img2)| img2.placeholder == *ph)
.filter(|(_, img2)| img2.display_placeholder == *ph)
.nth(occ_before)
{
break 'out Some((remove_idx, ph.clone()));
Expand Down Expand Up @@ -3296,11 +3351,11 @@ mod tests {
assert!(text.contains("[image_dup.png 10x5]"));
assert!(text.contains("[image_dup.png 10x5 #2]"));
assert_eq!(
composer.attached_images[0].placeholder,
composer.attached_images[0].display_placeholder,
"[image_dup.png 10x5]"
);
assert_eq!(
composer.attached_images[1].placeholder,
composer.attached_images[1].display_placeholder,
"[image_dup.png 10x5 #2]"
);
}
Expand All @@ -3318,7 +3373,7 @@ mod tests {
);
let path = PathBuf::from("/tmp/image3.png");
composer.attach_image(path.clone(), 20, 10, "PNG");
let placeholder = composer.attached_images[0].placeholder.clone();
let placeholder = composer.attached_images[0].display_placeholder.clone();

// Case 1: backspace at end
composer.textarea.move_cursor_to_end_of_line(false);
Expand All @@ -3329,7 +3384,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");
let placeholder2 = composer.attached_images[0].placeholder.clone();
let placeholder2 = composer.attached_images[0].display_placeholder.clone();
// Move cursor to roughly middle of placeholder
if let Some(start_pos) = composer.textarea.text().find(&placeholder2) {
let mid_pos = start_pos + (placeholder2.len() / 2);
Expand Down Expand Up @@ -3397,8 +3452,8 @@ mod tests {
composer.handle_paste(" ".into());
composer.attach_image(path2.clone(), 10, 5, "PNG");

let placeholder1 = composer.attached_images[0].placeholder.clone();
let placeholder2 = composer.attached_images[1].placeholder.clone();
let placeholder1 = composer.attached_images[0].display_placeholder.clone();
let placeholder2 = composer.attached_images[1].display_placeholder.clone();
let text = composer.textarea.text().to_string();
let start1 = text.find(&placeholder1).expect("first placeholder present");
let end1 = start1 + placeholder1.len();
Expand All @@ -3421,7 +3476,8 @@ mod tests {
assert_eq!(
vec![AttachedImage {
path: path2,
placeholder: "[image_dup2.png 10x5]".to_string()
display_placeholder: "[image_dup2.png 10x5]".to_string(),
model_placeholder: "[/tmp/image_dup2.png 10x5]".to_string()
}],
composer.attached_images,
"one image mapping remains"
Expand All @@ -3448,12 +3504,9 @@ 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] ")
);
let display_label = ChatComposer::display_label_for_image_placeholder(&tmp_path);
let expected_prefix = format!("[{display_label} 3x2] ");
assert!(composer.textarea.text().starts_with(&expected_prefix));

let imgs = composer.take_recent_submission_images();
assert_eq!(imgs, vec![tmp_path]);
Expand Down Expand Up @@ -4170,7 +4223,8 @@ mod tests {
let placeholder = "[image 10x10]".to_string();
composer.textarea.insert_element(&placeholder);
composer.attached_images.push(AttachedImage {
placeholder: placeholder.clone(),
display_placeholder: placeholder.clone(),
model_placeholder: placeholder.clone(),
path: PathBuf::from("img.png"),
});
composer
Expand All @@ -4185,7 +4239,7 @@ mod tests {
);
assert!(composer.pending_pastes.is_empty());
assert_eq!(composer.attached_images.len(), 1);
assert_eq!(composer.attached_images[0].placeholder, placeholder);
assert_eq!(composer.attached_images[0].display_placeholder, placeholder);
assert_eq!(composer.textarea.cursor(), composer.current_text().len());
}

Expand All @@ -4204,7 +4258,8 @@ mod tests {
let placeholder = "[image 10x10]".to_string();
composer.textarea.insert_element(&placeholder);
composer.attached_images.push(AttachedImage {
placeholder: placeholder.clone(),
display_placeholder: placeholder.clone(),
model_placeholder: placeholder.clone(),
path: PathBuf::from("img.png"),
});

Expand Down Expand Up @@ -4254,7 +4309,8 @@ mod tests {
let placeholder = "[image 10x10]".to_string();
composer.textarea.insert_element(&placeholder);
composer.attached_images.push(AttachedImage {
placeholder: placeholder.clone(),
display_placeholder: placeholder.clone(),
model_placeholder: placeholder.clone(),
path: PathBuf::from("img.png"),
});

Expand Down
8 changes: 8 additions & 0 deletions codex-rs/tui/src/bottom_pane/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -549,6 +549,14 @@ impl BottomPane {
self.composer.take_recent_submission_images()
}

pub(crate) fn expand_attached_image_placeholders_for_model(
&self,
display_text: &str,
) -> String {
self.composer
.expand_attached_image_placeholders_for_model(display_text)
}

fn as_renderable(&'_ self) -> RenderableItem<'_> {
if let Some(view) = self.active_view() {
RenderableItem::Borrowed(view)
Expand Down
Loading
Loading