From 090144e0eca3978b3ebf29bc376a48b3d37523c5 Mon Sep 17 00:00:00 2001 From: sayan-oai Date: Wed, 27 May 2026 15:11:29 -0700 Subject: [PATCH] [codex] Fix hyperlink-aware key-value table rendering (#24825) ## Why The key/value markdown table renderer added in #24636 still operates on `Line` values, while table cells and rendered table output now carry `HyperlinkLine`. That mismatch breaks `codex-tui` compilation on `main` and would risk losing semantic web-link annotations if corrected by flattening the values. ## What changed - Make key/value record rendering wrap and emit `HyperlinkLine` values consistently with the existing grid renderer. - Remap wrapped hyperlink ranges and shift them when value content is prefixed by record-mode indentation or labels. - Add focused coverage verifying key/value fallback output preserves web-link destinations. ## Verification - `just test -p codex-tui -E 'test(key_value_table_keeps_web_annotations) | test(/table_renders_(key_value_records_when_compact_fragmentation_is_systemic_snapshot|stacked_key_value_records_when_path_column_becomes_too_narrow_snapshot|records_when_multiple_prose_columns_are_starved_snapshot)/)'` --- codex-rs/tui/src/markdown_render.rs | 20 ++++++ .../src/markdown_render/table_key_value.rs | 69 +++++++++++++------ 2 files changed, 69 insertions(+), 20 deletions(-) diff --git a/codex-rs/tui/src/markdown_render.rs b/codex-rs/tui/src/markdown_render.rs index 8711428588..9847c6dc3a 100644 --- a/codex-rs/tui/src/markdown_render.rs +++ b/codex-rs/tui/src/markdown_render.rs @@ -2712,6 +2712,26 @@ mod tests { })); } + #[test] + fn key_value_table_keeps_web_annotations() { + let destination = "https://example.com/a/very/long/path"; + let markdown = format!( + "| c1 | c2 | c3 | c4 | c5 | c6 |\n| --- | --- | --- | --- | --- | --- |\n| {destination} | 2 | 3 | 4 | 5 | 6 |\n" + ); + let lines = render_markdown_lines_with_width_and_cwd( + &markdown, + /*width*/ Some(20), + /*cwd*/ None, + ); + let destinations = lines + .iter() + .flat_map(|line| line.hyperlinks.iter().map(|link| link.destination.as_str())) + .collect::>(); + + assert!(!destinations.is_empty()); + assert!(destinations.iter().all(|link| *link == destination)); + } + #[test] fn does_not_annotate_code_or_non_web_markdown_links() { let markdown = "`https://example.com/inline`\n\n```text\nhttps://example.com/block\n```\n\n[mail](mailto:test@example.com)\n\n[https://example.com/label](mailto:test@example.com)\n\n| Target |\n| --- |\n| [https://example.com/table-label](mailto:test@example.com) |"; diff --git a/codex-rs/tui/src/markdown_render/table_key_value.rs b/codex-rs/tui/src/markdown_render/table_key_value.rs index 9e506a0d45..e071f242bc 100644 --- a/codex-rs/tui/src/markdown_render/table_key_value.rs +++ b/codex-rs/tui/src/markdown_render/table_key_value.rs @@ -4,7 +4,10 @@ use super::TABLE_BODY_SEPARATOR_CHAR; use super::TableCell; use super::TableColumnKind; use super::TableColumnMetrics; +use crate::render::line_utils::line_to_static; use crate::render::line_utils::push_owned_lines; +use crate::terminal_hyperlinks::HyperlinkLine; +use crate::terminal_hyperlinks::remap_wrapped_line; use crate::wrapping::RtOptions; use crate::wrapping::word_wrap_line; use ratatui::style::Style; @@ -99,7 +102,7 @@ pub(super) fn render_records( available_width: Option, label_style: Style, separator_style: Style, -) -> Vec> { +) -> Vec { let label_width = headers .iter() .map(|header| header.plain_text().width()) @@ -135,10 +138,10 @@ pub(super) fn render_records( } if row_index + 1 < rows.len() { let width = available_width.unwrap_or_else(|| widest_line_width(&out)); - out.push(Line::from(Span::styled( + out.push(HyperlinkLine::new(Line::from(Span::styled( TABLE_BODY_SEPARATOR_CHAR.to_string().repeat(width), separator_style, - ))); + )))); } } @@ -146,7 +149,7 @@ pub(super) fn render_records( } fn render_aligned_field( - out: &mut Vec>, + out: &mut Vec, header: &TableCell, value: &TableCell, label_width: usize, @@ -170,13 +173,12 @@ fn render_aligned_field( } else { spans.push(Span::raw(" ".repeat(value_indent))); } - spans.extend(value_line.spans); - out.push(Line::from(spans)); + push_prefixed_value_line(out, spans, value_line); } } fn render_stacked_field( - out: &mut Vec>, + out: &mut Vec, header: &TableCell, value: &TableCell, available_width: Option, @@ -194,35 +196,60 @@ fn render_stacked_field( for label_line in wrapped_labels { let mut spans = vec![Span::raw(" ".repeat(FIELD_LEADING_PADDING))]; spans.extend(label_line.spans); - out.push(Line::from(spans)); + out.push(HyperlinkLine::new(Line::from(spans))); } let value_width = available_width .map(|width| width.saturating_sub(STACKED_VALUE_INDENT).max(1)) .unwrap_or_else(|| cell_width(value).max(1)); for value_line in wrap_cell(value, value_width) { - let mut spans = vec![Span::raw(" ".repeat(STACKED_VALUE_INDENT))]; - spans.extend(value_line.spans); - out.push(Line::from(spans)); + push_prefixed_value_line( + out, + vec![Span::raw(" ".repeat(STACKED_VALUE_INDENT))], + value_line, + ); } } -fn wrap_cell(cell: &TableCell, width: usize) -> Vec> { +fn push_prefixed_value_line( + out: &mut Vec, + mut prefix: Vec>, + mut value_line: HyperlinkLine, +) { + let shift = prefix + .iter() + .map(|span| span.content.width()) + .sum::(); + prefix.append(&mut value_line.line.spans); + let mut output_line = HyperlinkLine::new(Line::from(prefix)); + output_line + .hyperlinks + .extend(value_line.hyperlinks.into_iter().map(|mut link| { + link.columns = link.columns.start + shift..link.columns.end + shift; + link + })); + out.push(output_line); +} + +fn wrap_cell(cell: &TableCell, width: usize) -> Vec { if cell.lines.is_empty() { - return vec![Line::default()]; + return vec![HyperlinkLine::new(Line::default())]; } let mut wrapped = Vec::new(); for source_line in &cell.lines { - let rendered = word_wrap_line(source_line, RtOptions::new(width.max(1))); + let rendered = word_wrap_line(&source_line.line, RtOptions::new(width.max(1))) + .into_iter() + .map(|line| line_to_static(&line)) + .collect::>(); if rendered.is_empty() { - wrapped.push(Line::default()); + wrapped.push(HyperlinkLine::new(Line::default())); } else { - push_owned_lines(&rendered, &mut wrapped); + wrapped.extend(remap_wrapped_line(source_line, rendered)); } } if wrapped.is_empty() { - wrapped.push(Line::default()); + wrapped.push(HyperlinkLine::new(Line::default())); } wrapped } @@ -231,7 +258,8 @@ fn cell_width(cell: &TableCell) -> usize { cell.lines .iter() .map(|line| { - line.spans + line.line + .spans .iter() .map(|span| span.content.width()) .sum::() @@ -240,11 +268,12 @@ fn cell_width(cell: &TableCell) -> usize { .unwrap_or(0) } -fn widest_line_width(lines: &[Line<'_>]) -> usize { +fn widest_line_width(lines: &[HyperlinkLine]) -> usize { lines .iter() .map(|line| { - line.spans + line.line + .spans .iter() .map(|span| span.content.width()) .sum::()