From a795f961be68383738e56ac396742b3e8451696a Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Fri, 30 Oct 2026 22:20:00 +0000 Subject: [PATCH] Make viewed image paths clickable in the TUI (#58446) ## What changed - Add terminal file hyperlinks to viewed image filenames and transcript paths, including older history loaded through pagination. - Enable links only for local workspaces outside SSH sessions. Require a local regular file with a recognized image extension on both the original and resolved paths, and reject Windows reparse points. - Preserve links when summaries are truncated or transcript paths wrap. ## Testing Add coverage for local versus remote workspace links and for hyperlink destinations and column ranges in compact and expanded image rows. GitOrigin-RevId: a7dff255c280ad4e609644e6f5ef5e130d9a55fb --- codex-rs/tui/src/app/history_pagination.rs | 3 ++ codex-rs/tui/src/app/history_ui.rs | 19 +++++++- codex-rs/tui/src/app/history_ui_tests.rs | 36 ++++++++++++++ codex-rs/tui/src/history_cell/patches.rs | 47 +++++++++++++++++-- .../tui/src/history_cell/patches_tests.rs | 23 +++++++-- codex-rs/tui/src/terminal_hyperlinks.rs | 35 +++++++++++++- 6 files changed, 151 insertions(+), 12 deletions(-) diff --git a/codex-rs/tui/src/app/history_pagination.rs b/codex-rs/tui/src/app/history_pagination.rs index 6ba490ca89..74f8b7edd1 100644 --- a/codex-rs/tui/src/app/history_pagination.rs +++ b/codex-rs/tui/src/app/history_pagination.rs @@ -244,6 +244,9 @@ impl App { cells.push(Arc::new(completion)); } } + for cell in cells.iter_mut().filter_map(Arc::get_mut) { + self.link_viewed_image(cell); + } cells } diff --git a/codex-rs/tui/src/app/history_ui.rs b/codex-rs/tui/src/app/history_ui.rs index b7c5f08c78..ac38f1110f 100644 --- a/codex-rs/tui/src/app/history_ui.rs +++ b/codex-rs/tui/src/app/history_ui.rs @@ -22,7 +22,24 @@ pub(super) struct ThreadUsageStatusHistory { } impl App { - pub(super) fn insert_history_cell(&mut self, tui: &mut tui::Tui, cell: Box) { + pub(super) fn link_viewed_image(&self, cell: &mut dyn HistoryCell) { + if let Some(image) = cell + .as_any_mut() + .downcast_mut::() + && !self.app_server_target.uses_remote_workspace() + && crate::projectless::has_only_local_environments(&self.environment_manager) + && !crate::clipboard_copy::is_ssh_session() + { + image.link_local_image(); + } + } + + pub(super) fn insert_history_cell( + &mut self, + tui: &mut tui::Tui, + mut cell: Box, + ) { + self.link_viewed_image(cell.as_mut()); // Global deprecations can be delivered again by hidden threads. Scope deduplication to // retained history so clearing or rebuilding a transcript can show the notice again. if let Some(notice) = cell diff --git a/codex-rs/tui/src/app/history_ui_tests.rs b/codex-rs/tui/src/app/history_ui_tests.rs index 252eb046b4..46bbd9eb47 100644 --- a/codex-rs/tui/src/app/history_ui_tests.rs +++ b/codex-rs/tui/src/app/history_ui_tests.rs @@ -33,6 +33,42 @@ fn render_cell(cell: &impl HistoryCell) -> String { .join("\n") } +#[tokio::test] +async fn viewed_image_links_follow_workspace_locality() -> Result<()> { + let dir = tempfile::tempdir()?; + let path = dir.path().join("frame.png"); + image::RgbaImage::new(1, 1).save(&path)?; + let endpoint = crate::resolve_remote_addr("ws://127.0.0.1:4500")?; + let (mut app, _, _) = make_test_app_with_channels().await; + let mut tui = crate::tui::test_support::make_test_tui()?; + let local_links = !crate::clipboard_copy::is_ssh_session(); + for (target, linked) in [ + (AppServerTarget::Embedded, local_links), + ( + AppServerTarget::LocalDaemon { + endpoint: endpoint.clone(), + allow_embedded_fallback: false, + }, + local_links, + ), + (AppServerTarget::Remote { endpoint }, false), + ] { + app.app_server_target = target; + let cell = history_cell::new_view_image_tool_call( + "view".into(), + codex_utils_path_uri::LegacyAppPathString::from_path(&path), + ); + app.insert_history_cell(&mut tui, Box::new(cell)); + let rows = app + .transcript_cells + .last() + .unwrap() + .compact_hyperlink_lines(/*width*/ 80); + assert_eq!(!rows[0].hyperlinks.is_empty(), linked); + } + Ok(()) +} + #[tokio::test] async fn owned_clear_resets_navigation_and_retains_one_fresh_header() -> Result<()> { let (mut app, _rx, _op_rx) = make_test_app_with_channels().await; diff --git a/codex-rs/tui/src/history_cell/patches.rs b/codex-rs/tui/src/history_cell/patches.rs index 49191948f0..c0258c1a24 100644 --- a/codex-rs/tui/src/history_cell/patches.rs +++ b/codex-rs/tui/src/history_cell/patches.rs @@ -2,6 +2,8 @@ use super::*; use crate::line_truncation::truncate_line_with_ellipsis_if_overflow; +use crate::terminal_hyperlinks::TerminalHyperlink; +use crate::terminal_hyperlinks::adaptive_wrap_hyperlink_lines; use codex_ansi_escape::ansi_escape; use codex_utils_path_uri::LegacyAppPathString; @@ -162,6 +164,17 @@ pub(crate) struct ViewImageHistoryCell { activity_id: String, filename: String, path_label: String, + link: Option, +} + +impl ViewImageHistoryCell { + pub(crate) fn link_local_image(&mut self) { + let start = "Viewed image ".len(); + let end = start + crate::width::display_width(&self.path_label); + self.link = self + .image_path() + .and_then(|path| TerminalHyperlink::viewed_image(start..end, path.as_path())); + } } impl HistoryCell for ViewImageHistoryCell { @@ -186,13 +199,36 @@ impl HistoryCell for ViewImageHistoryCell { )] } + fn display_hyperlink_lines(&self, width: u16) -> Vec { + let mut lines = plain_hyperlink_lines(self.display_lines(width)); + if let ([line], Some(link)) = (lines.as_mut_slice(), &self.link) { + let start = crate::width::display_width("• Viewed image "); + if start < line.width() { + line.hyperlinks.push(link.with_columns(start..line.width())); + } + } + lines + } + fn transcript_lines(&self, width: u16) -> Vec> { - PrefixedWrappedHistoryCell::new( - Line::from(vec!["Viewed image ".bold(), self.path_label.clone().dim()]), - vec!["• ".dim()], - " ", + visible_lines(self.transcript_hyperlink_lines(width)) + } + + fn transcript_hyperlink_lines(&self, width: u16) -> Vec { + if width == 0 { + return Vec::new(); + } + let mut line = HyperlinkLine::new(Line::from(vec![ + "Viewed image ".bold(), + self.path_label.clone().dim(), + ])); + line.hyperlinks.extend(self.link.iter().cloned()); + adaptive_wrap_hyperlink_lines( + &[line], + RtOptions::new(usize::from(width)) + .initial_indent("• ".dim().into()) + .subsequent_indent(" ".into()), ) - .display_lines(width) } fn raw_lines(&self) -> Vec> { @@ -212,6 +248,7 @@ pub(crate) fn new_view_image_tool_call( activity_id: format!("image-view:{id}"), filename, path_label: path.into_string(), + link: None, } } diff --git a/codex-rs/tui/src/history_cell/patches_tests.rs b/codex-rs/tui/src/history_cell/patches_tests.rs index 9950b4350d..0ea5b8c04d 100644 --- a/codex-rs/tui/src/history_cell/patches_tests.rs +++ b/codex-rs/tui/src/history_cell/patches_tests.rs @@ -71,11 +71,24 @@ fn viewed_image_retains_original_path_in_details() { #[test] fn viewed_image_narrow_summary() { - let cell = new_view_image_tool_call( - "view".to_string(), - LegacyAppPathString::from_string("/workspace/very-long-screenshot-name.png"), - ); - insta::assert_snapshot!(cell.display_lines(/*width*/ 32)[0].to_string()); + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("very-long-screenshot #5.png"); + image::RgbaImage::new(1, 1).save(&path).unwrap(); + let url = Url::from_file_path(path.canonicalize().unwrap()).unwrap(); + let mut cell = new_view_image_tool_call("view".into(), LegacyAppPathString::from_path(&path)); + cell.link_local_image(); + let compact = cell.compact_hyperlink_lines(/*width*/ 32); + let expanded = cell.expanded_hyperlink_lines(u16::MAX); + for row in [&compact[0], &expanded[0]] { + let [link] = row.hyperlinks.as_slice() else { + panic!("expected one image link") + }; + assert_eq!( + (link.columns.clone(), link.terminal_destination()), + (15..row.width(), Some(url.to_string())) + ); + } + insta::assert_snapshot!(compact[0].line.to_string()); } #[test] diff --git a/codex-rs/tui/src/terminal_hyperlinks.rs b/codex-rs/tui/src/terminal_hyperlinks.rs index cc2b53c718..ff9c9188a4 100644 --- a/codex-rs/tui/src/terminal_hyperlinks.rs +++ b/codex-rs/tui/src/terminal_hyperlinks.rs @@ -50,7 +50,7 @@ pub(crate) struct TerminalHyperlink { #[derive(Clone, Copy, Debug, Eq, PartialEq)] enum DestinationKind { Web, - /// A generated visualization or a capability-validated spoken workspace artifact. + /// A generated visualization, validated workspace artifact, or locally viewed image. TrustedFile, } @@ -113,6 +113,39 @@ impl TerminalHyperlink { self.destination_kind = DestinationKind::TrustedFile; } + /// A viewed image can live outside the workspace. Only link a local regular file whose + /// resolved extension opens as an image (a symlink named screenshot.png may target a script). + pub(crate) fn viewed_image(columns: Range, path: &Path) -> Option { + let url = Url::from_file_path(path).ok()?; + if url.host().is_some() || image::ImageFormat::from_path(path).is_err() { + return None; + } + // A Windows junction can lead to a UNC share. Check parents before following any of them. + #[cfg(windows)] + for ancestor in path.ancestors().collect::>().into_iter().rev() { + use std::os::windows::fs::MetadataExt; + use windows_sys::Win32::Storage::FileSystem::FILE_ATTRIBUTE_REPARSE_POINT; + if ancestor.symlink_metadata().ok()?.file_attributes() & FILE_ATTRIBUTE_REPARSE_POINT + != 0 + { + return None; + } + } + let target = path.canonicalize().ok()?; + let url = Url::from_file_path(&target).ok()?; + if url.host().is_some() + || image::ImageFormat::from_path(&target).is_err() + || !target.is_file() + { + return None; + } + Some(Self { + columns, + destination: url.into(), + destination_kind: DestinationKind::TrustedFile, + }) + } + pub(crate) fn trusted_workspace_file( columns: Range, file: TrustedWorkspaceFile,