From 0276f2ee551f2cedf03f54ddab7fbc63a7579e94 Mon Sep 17 00:00:00 2001 From: Benjamin Carlsson Date: Tue, 1 Sep 2026 19:39:17 +0000 Subject: [PATCH] Preserve descriptive labels on local file links (#42123) ## What changed - Render descriptive Markdown labels alongside canonical local file targets instead of discarding them. - Collapse labels that resolve to the same path, accounting for relative and absolute forms, `file://` URLs, percent encoding, case, separators, and location suffixes. - Preserve `~/` destinations for display and keep absolute paths unless they can be shortened relative to the session working directory. ## Testing - Add unit and snapshot coverage for descriptive and path-equivalent labels, invalid percent encoding, trailing separators, Unix and Windows paths, UNC paths, `file://` URLs, and table wrapping. GitOrigin-RevId: 636bf4485b2899d96c6273be3f3b7f38728c9013 --- codex-rs/tui/src/markdown_render.rs | 331 +++++------------- .../tui/src/markdown_render/local_links.rs | 299 ++++++++++++++++ .../src/markdown_render/local_links_tests.rs | 21 ++ codex-rs/tui/src/markdown_render_tests.rs | 147 +++++++- ...th_spellings_without_changing_display.snap | 27 ++ ...iling_separators_when_comparing_paths.snap | 11 + ...nk_keeps_descriptive_label_and_target.snap | 5 + ...d_relative_label_with_matching_suffix.snap | 5 + ..._labels_with_invalid_percent_encoding.snap | 7 + ...erves_tilde_and_absolute_destinations.snap | 19 + ...th_column_becomes_too_narrow_snapshot.snap | 24 +- 11 files changed, 614 insertions(+), 282 deletions(-) create mode 100644 codex-rs/tui/src/markdown_render/local_links.rs create mode 100644 codex-rs/tui/src/markdown_render/local_links_tests.rs create mode 100644 codex-rs/tui/src/snapshots/codex_tui__markdown_render__markdown_render_tests__file_link_compares_path_spellings_without_changing_display.snap create mode 100644 codex-rs/tui/src/snapshots/codex_tui__markdown_render__markdown_render_tests__file_link_ignores_trailing_separators_when_comparing_paths.snap create mode 100644 codex-rs/tui/src/snapshots/codex_tui__markdown_render__markdown_render_tests__file_link_keeps_descriptive_label_and_target.snap create mode 100644 codex-rs/tui/src/snapshots/codex_tui__markdown_render__markdown_render_tests__file_link_keeps_unrelated_relative_label_with_matching_suffix.snap create mode 100644 codex-rs/tui/src/snapshots/codex_tui__markdown_render__markdown_render_tests__file_link_preserves_labels_with_invalid_percent_encoding.snap create mode 100644 codex-rs/tui/src/snapshots/codex_tui__markdown_render__markdown_render_tests__file_link_preserves_tilde_and_absolute_destinations.snap diff --git a/codex-rs/tui/src/markdown_render.rs b/codex-rs/tui/src/markdown_render.rs index e08b722a5b..39e885da60 100644 --- a/codex-rs/tui/src/markdown_render.rs +++ b/codex-rs/tui/src/markdown_render.rs @@ -5,10 +5,7 @@ //! display. It is the final rendering stage used by higher-level helpers in //! `markdown.rs`. //! -//! This renderer intentionally treats local file links differently from normal web links. For -//! local paths, the displayed text comes from the destination, not the markdown label, so -//! transcripts show the real file target (including normalized location suffixes) and can shorten -//! absolute paths relative to a known working directory. +//! Local file-link parsing and display policy live in [`local_links`]. //! //! ## Table rendering pipeline //! @@ -54,8 +51,6 @@ use crate::width::display_width; use crate::wrapping::RtOptions; use crate::wrapping::adaptive_wrap_line; use crate::wrapping::word_wrap_line; -use codex_utils_string::normalize_markdown_hash_location_suffix; -use dirs::home_dir; use pulldown_cmark::Alignment; use pulldown_cmark::CodeBlockKind; use pulldown_cmark::CowStr; @@ -69,17 +64,18 @@ use ratatui::style::Style; use ratatui::text::Line; use ratatui::text::Span; use ratatui::text::Text; -use regex_lite::Regex; use std::ops::Range; use std::path::Path; use std::path::PathBuf; -use std::sync::LazyLock; -use url::Url; +mod local_links; mod streaming; mod table_key_value; mod web_links; +use local_links::is_local_path_like_link; +use local_links::render_local_link_target; +use local_links::should_render_local_link_label; pub(crate) use streaming::StreamingMarkdownRender; pub(crate) use streaming::render_streaming_markdown_lines_with_width_and_cwd; pub(crate) use web_links::hide_web_link_destination; @@ -354,30 +350,16 @@ struct LinkState { has_visible_label: bool, /// Pre-rendered display text for local file links. /// - /// When this is present, the markdown label is intentionally suppressed so the rendered - /// transcript always reflects the real target path. + /// When this is present, label spans are buffered until the link closes so path-like labels + /// can collapse to this canonical target without losing descriptive labels. local_target_display: Option, + local_label_spans: Vec>, } fn should_render_link_destination(dest_url: &str) -> bool { !is_local_path_like_link(dest_url) } -static COLON_LOCATION_SUFFIX_RE: LazyLock = - LazyLock::new( - || match Regex::new(r":\d+(?::\d+)?(?:[-–]\d+(?::\d+)?)?$") { - Ok(regex) => regex, - Err(error) => panic!("invalid location suffix regex: {error}"), - }, - ); - -// Covered by load_location_suffix_regexes. -static HASH_LOCATION_SUFFIX_RE: LazyLock = - LazyLock::new(|| match Regex::new(r"^L\d+(?:C\d+)?(?:-L\d+(?:C\d+)?)?$") { - Ok(regex) => regex, - Err(error) => panic!("invalid hash location regex: {error}"), - }); - /// Stateful pulldown-cmark event consumer that builds styled `ratatui` output. /// /// Tracks inline style nesting, indent/blockquote context, list numbering, @@ -646,7 +628,14 @@ where } fn text(&mut self, text: CowStr<'a>) { - if self.suppressing_local_link_label() { + if self.collecting_local_link_label() { + let style = self.inline_styles.last().copied().unwrap_or_default(); + for (index, line) in text.lines().enumerate() { + if index > 0 { + self.push_local_link_label_break(); + } + self.push_local_link_label_span(Span::styled(line.to_string(), style)); + } return; } self.line_ends_with_local_link_target = false; @@ -700,7 +689,8 @@ where } fn code(&mut self, code: CowStr<'a>) { - if self.suppressing_local_link_label() { + if self.collecting_local_link_label() { + self.push_local_link_label_span(Span::from(code.into_string()).style(self.styles.code)); return; } self.line_ends_with_local_link_target = false; @@ -718,7 +708,17 @@ where } fn html(&mut self, html: CowStr<'a>, inline: bool) { - if self.suppressing_local_link_label() { + if self.collecting_local_link_label() { + let style = self.inline_styles.last().copied().unwrap_or_default(); + for (index, line) in html.lines().enumerate() { + if index > 0 { + self.push_local_link_label_break(); + } + self.push_local_link_label_span(Span::styled(line.to_string(), style)); + } + if !inline { + self.push_local_link_label_break(); + } return; } self.line_ends_with_local_link_target = false; @@ -751,7 +751,8 @@ where } fn hard_break(&mut self) { - if self.suppressing_local_link_label() { + if self.collecting_local_link_label() { + self.push_local_link_label_break(); return; } self.line_ends_with_local_link_target = false; @@ -763,7 +764,8 @@ where } fn soft_break(&mut self) { - if self.suppressing_local_link_label() { + if self.collecting_local_link_label() { + self.push_local_link_label_break(); return; } if self.in_table_cell() { @@ -1823,6 +1825,7 @@ where } else { None }, + local_label_spans: Vec::new(), destination: dest_url, }); } @@ -1862,8 +1865,13 @@ where self.push_span(")".into()); } } else if let Some(local_target_display) = link.local_target_display { - // Local file links are rendered as code-like path text so the transcript shows the - // resolved target instead of arbitrary caller-provided label text. + let local_label_text = link + .local_label_spans + .iter() + .map(|span| span.content.as_ref()) + .collect::(); + let show_label = + should_render_local_link_label(&local_label_text, &link.destination); let style = self .inline_styles .last() @@ -1872,25 +1880,61 @@ where .patch(self.styles.code); let span = Span::styled(local_target_display, style); if self.in_table_cell() { + if show_label { + for label_span in link.local_label_spans { + self.push_span_to_table_cell(label_span); + } + self.push_span_to_table_cell(" (".into()); + } self.push_span_to_table_cell(span); + if show_label { + self.push_span_to_table_cell(")".into()); + } } else { if self.pending_marker_line { self.push_line(Line::default()); } + if show_label { + for label_span in link.local_label_spans { + self.push_span(label_span); + } + self.push_span(" (".into()); + } self.push_span(span); + if show_label { + self.push_span(")".into()); + } self.line_ends_with_local_link_target = true; } } } } - fn suppressing_local_link_label(&self) -> bool { + fn collecting_local_link_label(&self) -> bool { self.link .as_ref() .and_then(|link| link.local_target_display.as_ref()) .is_some() } + fn push_local_link_label_span(&mut self, span: Span<'static>) { + if let Some(link) = self.link.as_mut() { + link.local_label_spans.push(span); + } + } + + fn push_local_link_label_break(&mut self) { + let needs_space = self + .link + .as_ref() + .and_then(|link| link.local_label_spans.last()) + .and_then(|span| span.content.chars().last()) + .is_some_and(|character| !character.is_whitespace()); + if needs_space { + self.push_local_link_label_span(" ".into()); + } + } + fn flush_current_line(&mut self) { if let Some(mut line) = self.current_line_content.take() { let style = self.current_line_style; @@ -2080,225 +2124,6 @@ where } } -fn is_local_path_like_link(dest_url: &str) -> bool { - dest_url.starts_with("file://") - || dest_url.starts_with('/') - || dest_url.starts_with("~/") - || dest_url.starts_with("./") - || dest_url.starts_with("../") - || dest_url.starts_with("\\\\") - || matches!( - dest_url.as_bytes(), - [drive, b':', separator, ..] - if drive.is_ascii_alphabetic() && matches!(separator, b'/' | b'\\') - ) -} - -/// Parse a local link target into normalized path text plus an optional location suffix. -/// -/// This accepts the path shapes Codex emits today: `file://` URLs, absolute and relative paths, -/// `~/...`, Windows paths, and `#L..C..` or `:line:col` suffixes. -fn render_local_link_target(dest_url: &str, cwd: Option<&Path>) -> Option { - let (path_text, location_suffix) = parse_local_link_target(dest_url)?; - let mut rendered = display_local_link_path(&path_text, cwd); - if let Some(location_suffix) = location_suffix { - rendered.push_str(&location_suffix); - } - Some(rendered) -} - -/// Split a local-link destination into `(normalized_path_text, location_suffix)`. -/// -/// The returned path text never includes a trailing `#L..` or `:line[:col]` suffix. Path -/// normalization expands `~/...` when possible and rewrites path separators into display-stable -/// forward slashes. The suffix, when present, is returned separately in normalized markdown form. -/// -/// Returns `None` only when the destination looks like a `file://` URL but cannot be parsed into a -/// local path. Plain path-like inputs always return `Some(...)` even if they are relative. -fn parse_local_link_target(dest_url: &str) -> Option<(String, Option)> { - if dest_url.starts_with("file://") { - let url = Url::parse(dest_url).ok()?; - let path_text = file_url_to_local_path_text(&url)?; - let location_suffix = url - .fragment() - .and_then(normalize_hash_location_suffix_fragment); - return Some((path_text, location_suffix)); - } - - let mut path_text = dest_url; - let mut location_suffix = None; - // Prefer `#L..` style fragments when both forms are present so URLs like `path#L10` do not - // get misparsed as a plain path ending in `:10`. - if let Some((candidate_path, fragment)) = dest_url.rsplit_once('#') - && let Some(normalized) = normalize_hash_location_suffix_fragment(fragment) - { - path_text = candidate_path; - location_suffix = Some(normalized); - } - if location_suffix.is_none() - && let Some(suffix) = extract_colon_location_suffix(path_text) - { - let path_len = path_text.len().saturating_sub(suffix.len()); - path_text = &path_text[..path_len]; - location_suffix = Some(suffix); - } - - let decoded_path_text = - urlencoding::decode(path_text).unwrap_or(std::borrow::Cow::Borrowed(path_text)); - Some((expand_local_link_path(&decoded_path_text), location_suffix)) -} - -/// Normalize a hash fragment like `L12` or `L12C3-L14C9` into the display suffix we render. -/// -/// Returns `None` for fragments that are not location references. This deliberately ignores other -/// `#...` fragments so non-location hashes stay part of the path text. -fn normalize_hash_location_suffix_fragment(fragment: &str) -> Option { - HASH_LOCATION_SUFFIX_RE - .is_match(fragment) - .then(|| format!("#{fragment}")) - .and_then(|suffix| normalize_markdown_hash_location_suffix(&suffix)) -} - -/// Extract a trailing `:line`, `:line:col`, or range suffix from a plain path-like string. -/// -/// The suffix must occur at the end of the input; embedded colons elsewhere in the path are left -/// alone. This is what keeps Windows drive letters like `C:/...` from being misread as locations. -fn extract_colon_location_suffix(path_text: &str) -> Option { - COLON_LOCATION_SUFFIX_RE - .find(path_text) - .filter(|matched| matched.end() == path_text.len()) - .map(|matched| matched.as_str().to_string()) -} - -/// Expand home-relative paths and normalize separators for display. -/// -/// If `~/...` cannot be expanded because the home directory is unavailable, the original text still -/// goes through separator normalization and is returned as-is otherwise. -fn expand_local_link_path(path_text: &str) -> String { - // Expand `~/...` eagerly so home-relative links can participate in the same normalization and - // cwd-relative shortening path as absolute links. - if let Some(rest) = path_text.strip_prefix("~/") - && let Some(home) = home_dir() - { - return normalize_local_link_path_text(&home.join(rest).to_string_lossy()); - } - - normalize_local_link_path_text(path_text) -} - -/// Convert a `file://` URL into the normalized local-path text used for transcript rendering. -/// -/// This prefers `Url::to_file_path()` for standard file URLs. When that rejects Windows-oriented -/// encodings, we reconstruct a display path from the host/path parts so UNC paths and drive-letter -/// URLs still render sensibly. -fn file_url_to_local_path_text(url: &Url) -> Option { - if let Ok(path) = url.to_file_path() { - return Some(normalize_local_link_path_text(&path.to_string_lossy())); - } - - // Fall back to string reconstruction for cases `to_file_path()` rejects, especially UNC-style - // hosts and Windows drive paths encoded in URL form. - let mut path_text = url.path().to_string(); - if let Some(host) = url.host_str() - && !host.is_empty() - && host != "localhost" - { - path_text = format!("//{host}{path_text}"); - } else if matches!( - path_text.as_bytes(), - [b'/', drive, b':', b'/', ..] if drive.is_ascii_alphabetic() - ) { - path_text.remove(0); - } - - Some(normalize_local_link_path_text(&path_text)) -} - -/// Normalize local-path text into the transcript display form. -/// -/// Display normalization is intentionally lexical: it does not touch the filesystem, resolve -/// symlinks, or collapse `.` / `..`. It only converts separators to forward slashes and rewrites -/// UNC-style `\\\\server\\share` inputs into `//server/share` so later prefix checks operate on a -/// stable representation. -fn normalize_local_link_path_text(path_text: &str) -> String { - // Render all local link paths with forward slashes so display and prefix stripping are stable - // across mixed Windows and Unix-style inputs. - if let Some(rest) = path_text.strip_prefix("\\\\") { - format!("//{}", rest.replace('\\', "/").trim_start_matches('/')) - } else { - path_text.replace('\\', "/") - } -} - -fn is_absolute_local_link_path(path_text: &str) -> bool { - path_text.starts_with('/') - || path_text.starts_with("//") - || matches!( - path_text.as_bytes(), - [drive, b':', b'/', ..] if drive.is_ascii_alphabetic() - ) -} - -/// Remove trailing separators from a local path without destroying root semantics. -/// -/// Roots like `/`, `//`, and `C:/` stay intact so callers can still distinguish "the root itself" -/// from "a path under the root". -fn trim_trailing_local_path_separator(path_text: &str) -> &str { - if path_text == "/" || path_text == "//" { - return path_text; - } - if matches!(path_text.as_bytes(), [drive, b':', b'/'] if drive.is_ascii_alphabetic()) { - return path_text; - } - path_text.trim_end_matches('/') -} - -/// Strip `cwd_text` from the start of `path_text` when `path_text` is strictly underneath it. -/// -/// Returns the relative remainder without a leading slash. If the path equals the cwd exactly, this -/// returns `None` so callers can keep rendering the full path instead of collapsing it to an empty -/// string. -fn strip_local_path_prefix<'a>(path_text: &'a str, cwd_text: &str) -> Option<&'a str> { - let path_text = trim_trailing_local_path_separator(path_text); - let cwd_text = trim_trailing_local_path_separator(cwd_text); - if path_text == cwd_text { - return None; - } - - // Treat filesystem roots specially so `/tmp/x` under `/` becomes `tmp/x` instead of being - // left unchanged by the generic prefix-stripping branch. - if cwd_text == "/" || cwd_text == "//" { - return path_text.strip_prefix('/'); - } - - path_text - .strip_prefix(cwd_text) - .and_then(|rest| rest.strip_prefix('/')) -} - -/// Choose the visible path text for a local link after normalization. -/// -/// Relative paths stay relative. Absolute paths are shortened against `cwd` only when they are -/// lexically underneath it; otherwise the absolute path is preserved. This is display logic only, -/// not filesystem canonicalization. -fn display_local_link_path(path_text: &str, cwd: Option<&Path>) -> String { - let path_text = normalize_local_link_path_text(path_text); - if !is_absolute_local_link_path(&path_text) { - return path_text; - } - - if let Some(cwd) = cwd { - // Only shorten absolute paths that are under the provided session cwd; otherwise preserve - // the original absolute target for clarity. - let cwd_text = normalize_local_link_path_text(&cwd.to_string_lossy()); - if let Some(stripped) = strip_local_path_prefix(&path_text, &cwd_text) { - return stripped.to_string(); - } - } - - path_text -} - #[cfg(test)] mod markdown_render_tests { include!("markdown_render_tests.rs"); diff --git a/codex-rs/tui/src/markdown_render/local_links.rs b/codex-rs/tui/src/markdown_render/local_links.rs new file mode 100644 index 0000000000..1266ace096 --- /dev/null +++ b/codex-rs/tui/src/markdown_render/local_links.rs @@ -0,0 +1,299 @@ +//! Local file-link parsing, label comparison, and display for Markdown transcripts. +//! +//! Markdown rendering intentionally treats local file links differently from normal web links. For +//! local paths, transcripts always show the real file target (including normalized location +//! suffixes) and can shorten absolute paths relative to a known working directory. Descriptive +//! Markdown labels remain visible alongside that target, while path-like labels collapse to the +//! canonical target to avoid duplicate file references. +//! + +use codex_utils_string::normalize_markdown_hash_location_suffix; +use regex_lite::Regex; +use std::path::Path; +use std::sync::LazyLock; +use url::Url; + +static COLON_LOCATION_SUFFIX_RE: LazyLock = + LazyLock::new( + || match Regex::new(r":\d+(?::\d+)?(?:[-–]\d+(?::\d+)?)?$") { + Ok(regex) => regex, + Err(error) => panic!("invalid location suffix regex: {error}"), + }, + ); + +// Covered by load_location_suffix_regexes. +static HASH_LOCATION_SUFFIX_RE: LazyLock = + LazyLock::new(|| match Regex::new(r"^L\d+(?:C\d+)?(?:-L\d+(?:C\d+)?)?$") { + Ok(regex) => regex, + Err(error) => panic!("invalid hash location regex: {error}"), + }); + +pub(super) fn is_local_path_like_link(dest_url: &str) -> bool { + dest_url.starts_with("file://") + || dest_url.starts_with('/') + || dest_url.starts_with("~/") + || dest_url.starts_with("./") + || dest_url.starts_with("../") + || dest_url.starts_with("\\\\") + || matches!( + dest_url.as_bytes(), + [drive, b':', separator, ..] + if drive.is_ascii_alphabetic() && matches!(separator, b'/' | b'\\') + ) +} + +/// Decide whether a local-file link label adds meaning beyond its canonical target. +/// +/// Matching path-like labels collapse to the target; prose labels remain visible. +pub(super) fn should_render_local_link_label(label: &str, destination: &str) -> bool { + let label = label.trim(); + if label.is_empty() { + return false; + } + let Some(parsed_label) = comparable_local_link_path(label) else { + return true; + }; + let Some(target) = comparable_local_link_path(destination) else { + return true; + }; + let target_path = trim_trailing_local_path_separator(target.trim_start_matches("./")); + let has_boundary_suffix = |path: &str, suffix: &str| { + !suffix.is_empty() + && path + .strip_suffix(suffix) + .is_some_and(|prefix| prefix.is_empty() || prefix.ends_with('/')) + }; + // Labels can spell a filename literally or URL-encode it. Compare both without decoding + // the destination twice (for example, percent%2520.rs denotes percent%20.rs). + let literal_label = normalize_local_link_path_text(label).to_lowercase(); + ![literal_label, parsed_label].iter().any(|label| { + let label_path = trim_trailing_local_path_separator(label.trim_start_matches("./")); + has_boundary_suffix(target_path, label_path) + || (is_absolute_local_link_path(label_path) + && has_boundary_suffix(label_path, target_path)) + }) +} + +/// Normalize original Markdown strings for comparison only, never already-rendered path text. +/// Case and URL spelling are intentionally forgiving; the visible target retains its spelling. +fn comparable_local_link_path(text: &str) -> Option { + let text = if text + .get(..7) + .is_some_and(|scheme| scheme.eq_ignore_ascii_case("file://")) + { + std::borrow::Cow::Owned(format!("file://{}", &text[7..])) + } else { + std::borrow::Cow::Borrowed(text) + }; + let (mut path, _) = parse_local_link_target(&text)?; + if text.starts_with("file://") { + let url = Url::parse(&text).ok()?; + // The display parser's fallback preserves URL escapes on platforms that cannot convert + // this URL to a native path. Decode that fallback exactly once for comparison as well. + if url.to_file_path().is_err() { + path = urlencoding::decode(&path) + .unwrap_or(std::borrow::Cow::Borrowed(&path)) + .into_owned(); + } + // Unix URL conversion retains the slash before a Windows drive; ignore it here only. + if matches!(path.as_bytes(), [b'/', drive, b':', b'/', ..] if drive.is_ascii_alphabetic()) { + path.remove(0); + } + } + Some(normalize_local_link_path_text(&path).to_lowercase()) +} + +/// Parse a local link target into normalized path text plus an optional location suffix. +/// +/// This accepts the path shapes Codex emits today: `file://` URLs, absolute and relative paths, +/// `~/...`, Windows paths, and `#L..C..` or `:line:col` suffixes. +pub(super) fn render_local_link_target(dest_url: &str, cwd: Option<&Path>) -> Option { + let (path_text, location_suffix) = parse_local_link_target(dest_url)?; + let mut rendered = display_local_link_path(&path_text, cwd); + if let Some(location_suffix) = location_suffix { + rendered.push_str(&location_suffix); + } + Some(rendered) +} + +/// Split a local-link destination into `(normalized_path_text, location_suffix)`. +/// +/// The returned path text never includes a trailing `#L..` or `:line[:col]` suffix. Path +/// normalization preserves `~/...` and rewrites path separators into display-stable forward +/// slashes. The suffix, when present, is returned separately in normalized markdown form. +/// +/// Returns `None` only when the destination looks like a `file://` URL but cannot be parsed into a +/// local path. Plain path-like inputs always return `Some(...)` even if they are relative. +fn parse_local_link_target(dest_url: &str) -> Option<(String, Option)> { + if dest_url.starts_with("file://") { + let url = Url::parse(dest_url).ok()?; + let path_text = file_url_to_local_path_text(&url)?; + let location_suffix = url + .fragment() + .and_then(normalize_hash_location_suffix_fragment); + return Some((path_text, location_suffix)); + } + + let mut path_text = dest_url; + let mut location_suffix = None; + // Prefer `#L..` style fragments when both forms are present so URLs like `path#L10` do not + // get misparsed as a plain path ending in `:10`. + if let Some((candidate_path, fragment)) = dest_url.rsplit_once('#') + && let Some(normalized) = normalize_hash_location_suffix_fragment(fragment) + { + path_text = candidate_path; + location_suffix = Some(normalized); + } + if location_suffix.is_none() + && let Some(suffix) = extract_colon_location_suffix(path_text) + { + let path_len = path_text.len().saturating_sub(suffix.len()); + path_text = &path_text[..path_len]; + location_suffix = Some(suffix); + } + + let decoded_path_text = + urlencoding::decode(path_text).unwrap_or(std::borrow::Cow::Borrowed(path_text)); + Some(( + normalize_local_link_path_text(&decoded_path_text), + location_suffix, + )) +} + +/// Normalize a hash fragment like `L12` or `L12C3-L14C9` into the display suffix we render. +/// +/// Returns `None` for fragments that are not location references. This deliberately ignores other +/// `#...` fragments so non-location hashes stay part of the path text. +fn normalize_hash_location_suffix_fragment(fragment: &str) -> Option { + HASH_LOCATION_SUFFIX_RE + .is_match(fragment) + .then(|| format!("#{fragment}")) + .and_then(|suffix| normalize_markdown_hash_location_suffix(&suffix)) +} + +/// Extract a trailing `:line`, `:line:col`, or range suffix from a plain path-like string. +/// +/// The suffix must occur at the end of the input; embedded colons elsewhere in the path are left +/// alone. This is what keeps Windows drive letters like `C:/...` from being misread as locations. +fn extract_colon_location_suffix(path_text: &str) -> Option { + COLON_LOCATION_SUFFIX_RE + .find(path_text) + .filter(|matched| matched.end() == path_text.len()) + .map(|matched| matched.as_str().to_string()) +} + +/// Convert a `file://` URL into the normalized local-path text used for transcript rendering. +/// +/// This prefers `Url::to_file_path()` for standard file URLs. When that rejects Windows-oriented +/// encodings, we reconstruct a display path from the host/path parts so UNC paths and drive-letter +/// URLs still render sensibly. +fn file_url_to_local_path_text(url: &Url) -> Option { + if let Ok(path) = url.to_file_path() { + return Some(normalize_local_link_path_text(&path.to_string_lossy())); + } + + // Fall back to string reconstruction for cases `to_file_path()` rejects, especially UNC-style + // hosts and Windows drive paths encoded in URL form. + let mut path_text = url.path().to_string(); + if let Some(host) = url.host_str() + && !host.is_empty() + && host != "localhost" + { + path_text = format!("//{host}{path_text}"); + } else if matches!( + path_text.as_bytes(), + [b'/', drive, b':', b'/', ..] if drive.is_ascii_alphabetic() + ) { + path_text.remove(0); + } + + Some(normalize_local_link_path_text(&path_text)) +} + +/// Normalize local-path text into the transcript display form. +/// +/// Display normalization is intentionally lexical: it does not touch the filesystem, resolve +/// symlinks, or collapse `.` / `..`. It only converts separators to forward slashes and rewrites +/// UNC-style `\\\\server\\share` inputs into `//server/share` so later prefix checks operate on a +/// stable representation. +fn normalize_local_link_path_text(path_text: &str) -> String { + // Render all local link paths with forward slashes so display and prefix stripping are stable + // across mixed Windows and Unix-style inputs. + if let Some(rest) = path_text.strip_prefix("\\\\") { + format!("//{}", rest.replace('\\', "/").trim_start_matches('/')) + } else { + path_text.replace('\\', "/") + } +} + +fn is_absolute_local_link_path(path_text: &str) -> bool { + path_text.starts_with('/') + || path_text.starts_with("//") + || matches!( + path_text.as_bytes(), + [drive, b':', b'/', ..] if drive.is_ascii_alphabetic() + ) +} + +/// Remove trailing separators from a local path without destroying root semantics. +/// +/// Roots like `/`, `//`, and `C:/` stay intact so callers can still distinguish "the root itself" +/// from "a path under the root". +fn trim_trailing_local_path_separator(path_text: &str) -> &str { + if path_text == "/" || path_text == "//" { + return path_text; + } + if matches!(path_text.as_bytes(), [drive, b':', b'/'] if drive.is_ascii_alphabetic()) { + return path_text; + } + path_text.trim_end_matches('/') +} + +/// Strip `cwd_text` from the start of `path_text` when `path_text` is strictly underneath it. +/// +/// Returns the relative remainder without a leading slash. If the path equals the cwd exactly, this +/// returns `None` so callers can keep rendering the full path instead of collapsing it to an empty +/// string. +fn strip_local_path_prefix<'a>(path_text: &'a str, cwd_text: &str) -> Option<&'a str> { + let path_text = trim_trailing_local_path_separator(path_text); + let cwd_text = trim_trailing_local_path_separator(cwd_text); + if path_text == cwd_text { + return None; + } + + // Treat filesystem roots specially so `/tmp/x` under `/` becomes `tmp/x` instead of being + // left unchanged by the generic prefix-stripping branch. + if cwd_text == "/" || cwd_text == "//" { + return path_text.strip_prefix('/'); + } + + path_text + .strip_prefix(cwd_text) + .and_then(|rest| rest.strip_prefix('/')) +} + +/// Choose the visible path text for a local link after normalization. +/// +/// Relative paths (including `~/...`) stay relative. Absolute paths prefer cwd-relative display +/// and otherwise stay absolute: the frontend home may differ from the execution host's home. +/// This is display logic only, not filesystem canonicalization. +fn display_local_link_path(path_text: &str, cwd: Option<&Path>) -> String { + let path_text = normalize_local_link_path_text(path_text); + if !is_absolute_local_link_path(&path_text) { + return path_text; + } + + if let Some(cwd) = cwd { + // Only the session cwd is known to refer to the execution host. + let cwd_text = normalize_local_link_path_text(&cwd.to_string_lossy()); + if let Some(stripped) = strip_local_path_prefix(&path_text, &cwd_text) { + return stripped.to_string(); + } + } + + path_text +} + +#[cfg(test)] +#[path = "local_links_tests.rs"] +mod tests; diff --git a/codex-rs/tui/src/markdown_render/local_links_tests.rs b/codex-rs/tui/src/markdown_render/local_links_tests.rs new file mode 100644 index 0000000000..d54d982f27 --- /dev/null +++ b/codex-rs/tui/src/markdown_render/local_links_tests.rs @@ -0,0 +1,21 @@ +//! Tests for local-link parsing and root-preserving separator normalization. + +use super::COLON_LOCATION_SUFFIX_RE; +use super::HASH_LOCATION_SUFFIX_RE; +use super::trim_trailing_local_path_separator; +use pretty_assertions::assert_eq; + +#[test] +fn load_location_suffix_regexes() { + let _colon = &*COLON_LOCATION_SUFFIX_RE; + let _hash = &*HASH_LOCATION_SUFFIX_RE; +} + +#[test] +fn trailing_separator_trimming_preserves_local_roots() { + let paths = ["/", "//", "C:/", "dir/", "//server/share/", "C:/dir/"]; + assert_eq!( + paths.map(trim_trailing_local_path_separator), + ["/", "//", "C:/", "dir", "//server/share", "C:/dir"] + ); +} diff --git a/codex-rs/tui/src/markdown_render_tests.rs b/codex-rs/tui/src/markdown_render_tests.rs index 222094f95d..54daa081ff 100644 --- a/codex-rs/tui/src/markdown_render_tests.rs +++ b/codex-rs/tui/src/markdown_render_tests.rs @@ -6,8 +6,6 @@ use ratatui::text::Span; use ratatui::text::Text; use std::path::Path; -use crate::markdown_render::COLON_LOCATION_SUFFIX_RE; -use crate::markdown_render::HASH_LOCATION_SUFFIX_RE; use crate::markdown_render::render_markdown_lines_with_width_and_cwd; use crate::markdown_render::render_markdown_text; use crate::markdown_render::render_markdown_text_with_width; @@ -825,33 +823,142 @@ fn web_link_labels_have_a_visible_underline_snapshot() { } #[test] -fn load_location_suffix_regexes() { - let _colon = &*COLON_LOCATION_SUFFIX_RE; - let _hash = &*HASH_LOCATION_SUFFIX_RE; +fn file_link_hides_destination() { + let text = render_markdown_text_for_cwd( + "[/Users/example/code/codex/codex-rs/tui/src/My%20File.rs](/Users/example/code/codex/codex-rs/tui/src/My%20File.rs)", + Path::new("/Users/example/code/codex"), + ); + let expected = Text::from(Line::from_iter(["codex-rs/tui/src/My File.rs".cyan()])); + assert_eq!(text, expected); } #[test] -fn file_link_hides_destination() { +fn file_link_keeps_descriptive_label_and_target() { let text = render_markdown_text_for_cwd( - "[codex-rs/tui/src/markdown_render.rs](/Users/example/code/codex/codex-rs/tui/src/markdown_render.rs)", - Path::new("/Users/example/code/codex"), + "Your `codex` launcher [automatically adds those overrides](/home/dev-user/code/openai/project/dotslash-gen/bin/codex:1105), even though you did not specify any.", + Path::new("/home/dev-user/code/openai"), ); let expected = Text::from(Line::from_iter([ - "codex-rs/tui/src/markdown_render.rs".cyan() + "Your ".into(), + "codex".cyan(), + " launcher ".into(), + "automatically adds those overrides".into(), + " (".into(), + "project/dotslash-gen/bin/codex:1105".cyan(), + ")".into(), + ", even though you did not specify any.".into(), ])); assert_eq!(text, expected); + + let rendered = text + .lines + .iter() + .map(|line| { + line.spans + .iter() + .map(|span| span.content.as_ref()) + .collect::() + }) + .collect::>() + .join("\n"); + assert_snapshot!(rendered); +} + +#[test] +fn file_link_preserves_tilde_and_absolute_destinations() { + let markdown = "[~/notes](~/notes)\n\n\ + [/home/alice/notes](/home/alice/notes)\n\n\ + [/home/alice/notes](~/notes)\n\n\ + [~/notes](/home/alice/notes)\n\n\ + [my **notes**](~/notes#L12C3)\n\n\ + [~/project/src/lib.rs](~/project/src/lib.rs)\n\n\ + [/home/alice/project/src/lib.rs](/home/alice/project/src/lib.rs)\n\n\ + [~](/home/alice:12)"; + let text = render_markdown_text_for_cwd(markdown, Path::new("/home/alice/project")); + assert_snapshot!(plain_lines(&text).join("\n")); +} + +#[test] +fn file_link_compares_path_spellings_without_changing_display() { + let markdown = r"[file:///repo/src/lib.rs](file:///repo/src/lib.rs) + +[SRC\LIB.RS](/repo/src/lib.rs#L12) + +[./src/lib.rs](/repo/src/lib.rs) + +[file:///C:/Repo/Src/Lib.rs](C:/Repo/Src/Lib.rs) + +[FILE:///C:/Repo/Src/Lib.rs](C:/Repo/Src/Lib.rs) + +[file://server/share/My%20File.rs](//server/share/My%20File.rs) + +[//SERVER/SHARE/My File.rs](file://server/share/My%20File.rs) + +[file:///repo/My%20File.rs](/repo/My%20File.rs) + +[file:///repo/percent%2520.rs](/repo/percent%2520.rs) + +[percent%20.rs](/repo/percent%2520.rs) + +[open **My File.rs**](/repo/My%20File.rs) + +[other/src/lib.rs](/repo/src/lib.rs)"; + let text = render_markdown_text_for_cwd(markdown, Path::new("/repo")); + // UNC file-URL display currently preserves escapes on Unix but decodes them on Windows. + // Keep that existing display behavior separate from comparison normalization. + let rendered = plain_lines(&text) + .join("\n") + .replace("My%20File.rs", "My File.rs"); + assert_snapshot!(rendered); } #[test] fn file_link_decodes_percent_encoded_bare_path_destination() { let text = render_markdown_text_for_cwd( - "[report](/Users/example/code/codex/Example%20Folder/R%C3%A9sum%C3%A9/report.md)", + "[open Example Folder/Résumé/report.md](/Users/example/code/codex/Example%20Folder/R%C3%A9sum%C3%A9/report.md)", Path::new("/Users/example/code/codex"), ); - let expected = Text::from(Line::from_iter(["Example Folder/Résumé/report.md".cyan()])); + let expected = Text::from(Line::from_iter([ + "open Example Folder/Résumé/report.md".into(), + " (".into(), + "Example Folder/Résumé/report.md".cyan(), + ")".into(), + ])); assert_eq!(text, expected); } +#[test] +fn file_link_preserves_labels_with_invalid_percent_encoding() { + let text = render_markdown_text_for_cwd( + "[bad%FF label](/tmp/)\n\n[bad%FF label](/)", + Path::new("/repo"), + ); + assert_snapshot!(plain_lines(&text).join("\n")); +} + +#[test] +fn file_link_ignores_trailing_separators_when_comparing_paths() { + let text = render_markdown_text_for_cwd( + "[dir](./dir/)\n\n[dir/](./dir)\n\n[dir](/outside/dir/)\n\n[dir/](/outside/dir)", + Path::new("/repo"), + ); + assert_snapshot!(plain_lines(&text).join("\n")); +} + +#[test] +fn file_link_keeps_unrelated_relative_label_with_matching_suffix() { + let text = + render_markdown_text_for_cwd("[other/src/lib.rs](/repo/src/lib.rs)", Path::new("/repo")); + let expected = Text::from(Line::from_iter([ + "other/src/lib.rs".into(), + " (".into(), + "src/lib.rs".cyan(), + ")".into(), + ])); + assert_eq!(text, expected); + assert_snapshot!(plain_lines(&text).join("\n")); +} + #[test] fn file_link_appends_line_number_when_label_lacks_it() { let text = render_markdown_text_for_cwd( @@ -945,7 +1052,12 @@ fn multiline_file_link_label_after_styled_prefix_does_not_panic() { let expected = Text::from(Line::from_iter([ "bold".bold(), " plain ".into(), + "foo".into(), + " ".into(), + "bar".into(), + " (".into(), "codex-rs/tui/src/markdown_render.rs:74:3".cyan(), + ")".into(), ])); assert_eq!(text, expected); } @@ -1015,9 +1127,9 @@ fn unordered_list_local_file_link_stays_inline_with_following_text() { assert_eq!( rendered, vec![ - "- codex-rs/README.md:93: core is the agent/business logic, tui is the", - " terminal UI, exec is the headless automation surface, and cli is the", - " top-level multitool binary.", + "- binary (codex-rs/README.md:93): core is the agent/business logic, tui", + " is the terminal UI, exec is the headless automation surface, and cli", + " is the top-level multitool binary.", ] ); } @@ -1041,7 +1153,7 @@ fn unordered_list_local_file_link_soft_break_before_colon_stays_inline() { .collect::>(); assert_eq!( rendered, - vec!["- codex-rs/README.md:93: core is the agent/business logic.",] + vec!["- binary (codex-rs/README.md:93): core is the agent/business logic.",] ); } @@ -1065,8 +1177,9 @@ fn consecutive_unordered_list_local_file_links_do_not_detach_paths() { assert_eq!( rendered, vec![ - "- codex-rs/README.md:93: cli is the top-level multitool binary.", - "- codex-rs/core/README.md:1: codex-core owns the real runtime behavior.", + "- binary (codex-rs/README.md:93): cli is the top-level multitool binary.", + "- expectations (codex-rs/core/README.md:1): codex-core owns the real", + " runtime behavior.", ] ); } diff --git a/codex-rs/tui/src/snapshots/codex_tui__markdown_render__markdown_render_tests__file_link_compares_path_spellings_without_changing_display.snap b/codex-rs/tui/src/snapshots/codex_tui__markdown_render__markdown_render_tests__file_link_compares_path_spellings_without_changing_display.snap new file mode 100644 index 0000000000..0a4594d0e6 --- /dev/null +++ b/codex-rs/tui/src/snapshots/codex_tui__markdown_render__markdown_render_tests__file_link_compares_path_spellings_without_changing_display.snap @@ -0,0 +1,27 @@ +--- +source: tui/src/markdown_render_tests.rs +expression: rendered +--- +src/lib.rs + +src/lib.rs:12 + +src/lib.rs + +C:/Repo/Src/Lib.rs + +C:/Repo/Src/Lib.rs + +//server/share/My File.rs + +//server/share/My File.rs + +My File.rs + +percent%20.rs + +percent%20.rs + +open My File.rs (My File.rs) + +other/src/lib.rs (src/lib.rs) diff --git a/codex-rs/tui/src/snapshots/codex_tui__markdown_render__markdown_render_tests__file_link_ignores_trailing_separators_when_comparing_paths.snap b/codex-rs/tui/src/snapshots/codex_tui__markdown_render__markdown_render_tests__file_link_ignores_trailing_separators_when_comparing_paths.snap new file mode 100644 index 0000000000..bee4184ce2 --- /dev/null +++ b/codex-rs/tui/src/snapshots/codex_tui__markdown_render__markdown_render_tests__file_link_ignores_trailing_separators_when_comparing_paths.snap @@ -0,0 +1,11 @@ +--- +source: tui/src/markdown_render_tests.rs +expression: "plain_lines(&text).join(\"\\n\")" +--- +./dir/ + +./dir + +/outside/dir/ + +/outside/dir diff --git a/codex-rs/tui/src/snapshots/codex_tui__markdown_render__markdown_render_tests__file_link_keeps_descriptive_label_and_target.snap b/codex-rs/tui/src/snapshots/codex_tui__markdown_render__markdown_render_tests__file_link_keeps_descriptive_label_and_target.snap new file mode 100644 index 0000000000..b85cbe0197 --- /dev/null +++ b/codex-rs/tui/src/snapshots/codex_tui__markdown_render__markdown_render_tests__file_link_keeps_descriptive_label_and_target.snap @@ -0,0 +1,5 @@ +--- +source: tui/src/markdown_render_tests.rs +expression: rendered +--- +Your codex launcher automatically adds those overrides (project/dotslash-gen/bin/codex:1105), even though you did not specify any. diff --git a/codex-rs/tui/src/snapshots/codex_tui__markdown_render__markdown_render_tests__file_link_keeps_unrelated_relative_label_with_matching_suffix.snap b/codex-rs/tui/src/snapshots/codex_tui__markdown_render__markdown_render_tests__file_link_keeps_unrelated_relative_label_with_matching_suffix.snap new file mode 100644 index 0000000000..51d10da11b --- /dev/null +++ b/codex-rs/tui/src/snapshots/codex_tui__markdown_render__markdown_render_tests__file_link_keeps_unrelated_relative_label_with_matching_suffix.snap @@ -0,0 +1,5 @@ +--- +source: tui/src/markdown_render_tests.rs +expression: "plain_lines(&text).join(\"\\n\")" +--- +other/src/lib.rs (src/lib.rs) diff --git a/codex-rs/tui/src/snapshots/codex_tui__markdown_render__markdown_render_tests__file_link_preserves_labels_with_invalid_percent_encoding.snap b/codex-rs/tui/src/snapshots/codex_tui__markdown_render__markdown_render_tests__file_link_preserves_labels_with_invalid_percent_encoding.snap new file mode 100644 index 0000000000..55c635e1ab --- /dev/null +++ b/codex-rs/tui/src/snapshots/codex_tui__markdown_render__markdown_render_tests__file_link_preserves_labels_with_invalid_percent_encoding.snap @@ -0,0 +1,7 @@ +--- +source: tui/src/markdown_render_tests.rs +expression: "plain_lines(&text).join(\"\\n\")" +--- +bad%FF label (/tmp/) + +bad%FF label (/) diff --git a/codex-rs/tui/src/snapshots/codex_tui__markdown_render__markdown_render_tests__file_link_preserves_tilde_and_absolute_destinations.snap b/codex-rs/tui/src/snapshots/codex_tui__markdown_render__markdown_render_tests__file_link_preserves_tilde_and_absolute_destinations.snap new file mode 100644 index 0000000000..364b334df3 --- /dev/null +++ b/codex-rs/tui/src/snapshots/codex_tui__markdown_render__markdown_render_tests__file_link_preserves_tilde_and_absolute_destinations.snap @@ -0,0 +1,19 @@ +--- +source: tui/src/markdown_render_tests.rs +expression: "plain_lines(&text).join(\"\\n\")" +--- +~/notes + +/home/alice/notes + +/home/alice/notes (~/notes) + +~/notes (/home/alice/notes) + +my notes (~/notes:12:3) + +~/project/src/lib.rs + +src/lib.rs + +~ (/home/alice:12) diff --git a/codex-rs/tui/src/snapshots/codex_tui__markdown_render__markdown_render_tests__table_renders_stacked_key_value_records_when_path_column_becomes_too_narrow_snapshot.snap b/codex-rs/tui/src/snapshots/codex_tui__markdown_render__markdown_render_tests__table_renders_stacked_key_value_records_when_path_column_becomes_too_narrow_snapshot.snap index bcdecdc149..229efcbb21 100644 --- a/codex-rs/tui/src/snapshots/codex_tui__markdown_render__markdown_render_tests__table_renders_stacked_key_value_records_when_path_column_becomes_too_narrow_snapshot.snap +++ b/codex-rs/tui/src/snapshots/codex_tui__markdown_render__markdown_render_tests__table_renders_stacked_key_value_records_when_path_column_becomes_too_narrow_snapshot.snap @@ -3,10 +3,10 @@ source: tui/src/markdown_render_tests.rs expression: "plain_lines(&text).join(\"\\n\")" --- Session - /Users/felipe.coury/.codex/ - sessions/2026/05/25/rollout-2026-05- - 25T18-13-09-019e60fc-0518-7c21-9596- - 980fe97225ba.jsonl + 2026-05-25 current gallery (/Users/ + felipe.coury/.codex/sessions/2026/05/25/ + rollout-2026-05-25T18-13-09-019e60fc- + 0518-7c21-9596-980fe97225ba.jsonl) Why useful The large gallery from this thread: emojis, links, emphasis, code, @@ -16,10 +16,10 @@ expression: "plain_lines(&text).join(\"\\n\")" 7 ────────────────────────────────────────── Session - /Users/felipe.coury/.codex/ - sessions/2026/05/14/rollout-2026-05- - 14T12-57-18-019e2734-e500-7011-8278- - 975c94d06000.jsonl + 2026-05-14 renderer testing (/Users/ + felipe.coury/.codex/sessions/2026/05/14/ + rollout-2026-05-14T12-57-18-019e2734- + e500-7011-8278-975c94d06000.jsonl) Why useful Explicit "markdown tables for testing" session with several successive @@ -28,10 +28,10 @@ expression: "plain_lines(&text).join(\"\\n\")" 16 ────────────────────────────────────────── Session - /Users/felipe.coury/.codex/ - sessions/2026/05/14/rollout-2026-05- - 14T12-27-57-019e271a-064c-78c3-a5cd- - a6f20a0c1ad5.jsonl + 2026-05-14 five-table test (/Users/ + felipe.coury/.codex/sessions/2026/05/14/ + rollout-2026-05-14T12-27-57-019e271a- + 064c-78c3-a5cd-a6f20a0c1ad5.jsonl) Why useful Explicit request for five tables containing emojis, code, italics, and