From 732cb7a2c32a8dcf2ec04a4bcafcf19dbebf6bcd Mon Sep 17 00:00:00 2001 From: Felipe Coury Date: Wed, 29 Apr 2026 17:01:19 -0300 Subject: [PATCH] feat(tui): add readable table fallback --- codex-rs/tui/src/markdown_render.rs | 2 +- codex-rs/tui/src/markdown_render/table.rs | 435 ++++++++++++++++++---- codex-rs/tui/src/markdown_render_tests.rs | 140 +++++++ 3 files changed, 513 insertions(+), 64 deletions(-) diff --git a/codex-rs/tui/src/markdown_render.rs b/codex-rs/tui/src/markdown_render.rs index 3a6164d9e6..98c1547f13 100644 --- a/codex-rs/tui/src/markdown_render.rs +++ b/codex-rs/tui/src/markdown_render.rs @@ -406,7 +406,7 @@ where return; }; let lines = render_table_lines(&table.rows, self.wrap_width); - self.text.lines.extend(lines.into_iter().map(Line::from)); + self.text.lines.extend(lines); self.needs_newline = true; } diff --git a/codex-rs/tui/src/markdown_render/table.rs b/codex-rs/tui/src/markdown_render/table.rs index a62600cf66..8d37ebdd55 100644 --- a/codex-rs/tui/src/markdown_render/table.rs +++ b/codex-rs/tui/src/markdown_render/table.rs @@ -1,5 +1,8 @@ use std::borrow::Cow; +use ratatui::style::Stylize; +use ratatui::text::Line; +use ratatui::text::Span; use unicode_width::UnicodeWidthStr; #[derive(Debug, Default)] @@ -68,13 +71,22 @@ impl TableState { } } -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -enum TableLayoutMode { - Normal, - Compact, +#[derive(Debug)] +struct TableLayoutCandidate { + column_widths: Vec, + padding: usize, + hard_wrap: bool, } -pub(super) fn render_table_lines(rows: &[Vec], width: Option) -> Vec { +#[derive(Debug)] +struct TableMetrics { + average_body_row_height: f64, + max_body_row_height: usize, + hard_wraps_url_or_code: bool, + hard_wrap_count: usize, +} + +pub(super) fn render_table_lines(rows: &[Vec], width: Option) -> Vec> { if rows.is_empty() { return Vec::new(); } @@ -88,11 +100,16 @@ pub(super) fn render_table_lines(rows: &[Vec], width: Option) -> let normalized_rows = normalize_table_rows(rows, column_count); let widths = desired_column_widths(&normalized_rows, column_count); - let layout = choose_table_layout(&widths, available_width, column_count); - match layout { - Some((mode, column_widths, padding)) => { - render_box_table(&normalized_rows, &column_widths, padding, mode) - } + match choose_table_layout(&normalized_rows, &widths, available_width, column_count) { + Some(candidate) => render_box_table( + &normalized_rows, + &candidate.column_widths, + candidate.padding, + candidate.hard_wrap, + ) + .into_iter() + .map(Line::from) + .collect(), None => render_vertical_table(&normalized_rows, available_width), } } @@ -230,21 +247,32 @@ fn desired_column_widths(rows: &[Vec], column_count: usize) -> Vec], desired_widths: &[usize], available_width: usize, column_count: usize, -) -> Option<(TableLayoutMode, Vec, usize)> { - if available_width < 20 { +) -> Option { + if available_width < 32 || width_shape_requires_vertical(column_count, available_width) { return None; } - if let Some(widths) = allocate_table_widths( + let normal = allocate_table_widths( desired_widths, available_width, column_count, /*padding*/ 1, - ) { - return Some((TableLayoutMode::Normal, widths, 1)); + ) + .and_then(|widths| { + build_table_candidate( + rows, + desired_widths, + widths, + available_width, + /*padding*/ 1, + ) + }); + if normal.is_some() { + return normal; } allocate_table_widths( @@ -253,7 +281,56 @@ fn choose_table_layout( column_count, /*padding*/ 0, ) - .map(|widths| (TableLayoutMode::Compact, widths, 0)) + .and_then(|widths| { + build_table_candidate( + rows, + desired_widths, + widths, + available_width, + /*padding*/ 0, + ) + }) +} + +fn width_shape_requires_vertical(column_count: usize, available_width: usize) -> bool { + (column_count >= 5 && available_width < 72) || (column_count >= 6 && available_width < 96) +} + +fn build_table_candidate( + rows: &[Vec], + desired_widths: &[usize], + column_widths: Vec, + available_width: usize, + padding: usize, +) -> Option { + let metrics = table_metrics(rows, &column_widths, /*hard_wrap*/ false); + let needs_hard_wrap = metrics.hard_wrap_count > 0 || metrics.max_body_row_height > 12; + let (hard_wrap, metrics) = if needs_hard_wrap { + ( + true, + table_metrics(rows, &column_widths, /*hard_wrap*/ true), + ) + } else { + (false, metrics) + }; + + if should_render_vertical( + rows, + desired_widths, + &column_widths, + available_width, + padding, + hard_wrap, + &metrics, + ) { + return None; + } + + Some(TableLayoutCandidate { + column_widths, + padding, + hard_wrap, + }) } fn allocate_table_widths( @@ -293,29 +370,6 @@ fn allocate_table_widths( } fn render_box_table( - rows: &[Vec], - column_widths: &[usize], - padding: usize, - _mode: TableLayoutMode, -) -> Vec { - let mut rendered = - render_box_table_with_wrap(rows, column_widths, padding, /*hard_wrap*/ false); - let available_width = table_total_width(column_widths, padding); - let needs_hard_wrap = rendered.iter().any(|line| line.width() > available_width) - || any_row_too_tall(rows, column_widths, padding, /*hard_wrap*/ false); - - if needs_hard_wrap { - rendered = - render_box_table_with_wrap(rows, column_widths, padding, /*hard_wrap*/ true); - if any_row_too_tall(rows, column_widths, padding, /*hard_wrap*/ true) { - return render_vertical_table(rows, available_width); - } - } - - rendered -} - -fn render_box_table_with_wrap( rows: &[Vec], column_widths: &[usize], padding: usize, @@ -408,20 +462,94 @@ fn wrap_table_cell(cell: &str, width: usize, hard_wrap: bool) -> Vec { } } -fn any_row_too_tall( - rows: &[Vec], - column_widths: &[usize], - _padding: usize, - hard_wrap: bool, -) -> bool { - rows.iter().skip(1).any(|row| { - row.iter() +fn table_metrics(rows: &[Vec], column_widths: &[usize], hard_wrap: bool) -> TableMetrics { + let mut row_heights = Vec::with_capacity(rows.len()); + let mut hard_wraps_url_or_code = false; + let mut hard_wrap_count = 0usize; + + for row in rows { + let row_height = row + .iter() .zip(column_widths) - .map(|(cell, width)| wrap_table_cell(cell, *width, hard_wrap).len()) + .map(|(cell, width)| { + if cell_needs_hard_wrap(cell, *width) { + hard_wrap_count += 1; + hard_wraps_url_or_code |= is_url_or_code_like(cell); + } + wrap_table_cell(cell, *width, hard_wrap).len() + }) .max() - .unwrap_or(1) - > 20 - }) + .unwrap_or(1); + row_heights.push(row_height); + } + + let body_heights = row_heights.iter().skip(1).copied().collect::>(); + let body_row_count = body_heights.len(); + let max_body_row_height = body_heights.iter().copied().max().unwrap_or(0); + let average_body_row_height = if body_row_count == 0 { + 0.0 + } else { + body_heights.iter().sum::() as f64 / body_row_count as f64 + }; + + TableMetrics { + average_body_row_height, + max_body_row_height, + hard_wraps_url_or_code, + hard_wrap_count, + } +} + +fn cell_needs_hard_wrap(cell: &str, width: usize) -> bool { + cell.split('\n') + .flat_map(str::split_whitespace) + .any(|token| token.width() > width) +} + +fn should_render_vertical( + rows: &[Vec], + desired_widths: &[usize], + column_widths: &[usize], + available_width: usize, + padding: usize, + hard_wrap: bool, + metrics: &TableMetrics, +) -> bool { + let column_count = column_widths.len(); + let body_rows = rows.len().saturating_sub(1); + if metrics.max_body_row_height > 12 + || (body_rows >= 10 && metrics.average_body_row_height > 2.5) + || (body_rows >= 24 && metrics.average_body_row_height > 1.75) + { + return true; + } + + if hard_wrap && ((body_rows > 5 && metrics.hard_wraps_url_or_code) || body_rows >= 10) { + return true; + } + + if column_count >= 4 && (body_rows >= 8 || column_count >= 5) { + let starved_columns = column_widths.iter().filter(|width| **width <= 3).count(); + if starved_columns > 1 { + return true; + } + + for (index, width) in column_widths.iter().enumerate().take(column_count) { + if is_index_column(rows, index) { + continue; + } + if is_content_heavy_column(rows, index) && *width < 12 { + return true; + } + } + } + + let slack = available_width.saturating_sub(table_total_width(column_widths, padding)); + has_width_risk_chars(rows) + && column_count >= 3 + && available_width <= 48 + && slack < 2 + && desired_widths.iter().sum::() + column_count + 1 >= available_width } fn table_total_width(column_widths: &[usize], padding: usize) -> usize { @@ -431,26 +559,207 @@ fn table_total_width(column_widths: &[usize], padding: usize) -> usize { + padding * 2 * column_widths.len() } -fn render_vertical_table(rows: &[Vec], available_width: usize) -> Vec { +fn is_index_column(rows: &[Vec], index: usize) -> bool { + let header = rows + .first() + .and_then(|row| row.get(index)) + .map(|header| normalized_header(header)) + .unwrap_or_default(); + if matches!(header.as_str(), "#" | "id" | "idx" | "index" | "row") { + return true; + } + + rows.iter() + .skip(1) + .filter_map(|row| row.get(index)) + .filter(|cell| !cell.trim().is_empty()) + .all(|cell| cell.trim().chars().all(|ch| ch.is_ascii_digit())) +} + +fn is_content_heavy_column(rows: &[Vec], index: usize) -> bool { + let header = rows + .first() + .and_then(|row| row.get(index)) + .map(|header| normalized_header(header)) + .unwrap_or_default(); + if [ + "link", + "url", + "code", + "sample", + "content", + "description", + "summary", + "expectation", + "notes", + ] + .iter() + .any(|needle| header.contains(needle)) + { + return true; + } + + rows.iter() + .skip(1) + .filter_map(|row| row.get(index)) + .any(|cell| is_url_or_code_like(cell) || cell.width() > 24) +} + +fn is_url_or_code_like(cell: &str) -> bool { + cell.contains("://") + || cell.contains("::") + || cell.contains("=>") + || cell.contains("->") + || cell.contains('`') + || cell.contains('{') + || cell.contains('}') + || cell.contains('(') + || cell.contains(')') +} + +fn has_width_risk_chars(rows: &[Vec]) -> bool { + rows.iter().flatten().any(|cell| { + cell.chars().any(|ch| { + matches!(ch, '\u{fe0f}' | '\u{200d}') || ('\u{1f300}'..='\u{1faff}').contains(&ch) + }) + }) +} + +fn normalized_header(header: &str) -> String { + header.trim().to_ascii_lowercase() +} + +fn render_vertical_table(rows: &[Vec], available_width: usize) -> Vec> { let Some((headers, body_rows)) = rows.split_first() else { return Vec::new(); }; - let wrap_width = available_width.max(1); + let included_columns = included_vertical_columns(headers, body_rows); + if included_columns.is_empty() { + return Vec::new(); + } + + let max_header_width = included_columns + .iter() + .map(|index| vertical_label(headers, *index).width()) + .max() + .unwrap_or(4); + let label_width = max_header_width.min(20).min(available_width / 3).max(4); + let value_width = available_width.saturating_sub(label_width + 2).max(1); let mut out = Vec::new(); + for (row_index, row) in body_rows.iter().enumerate() { if row_index > 0 { - out.push(String::new()); + out.push(Line::default()); } - out.push(format!("Row {}", row_index + 1)); - for (header, cell) in headers.iter().zip(row) { - let label = if header.is_empty() { "Column" } else { header }; - let line = format!("{label}: {cell}"); - out.extend( - textwrap::wrap(&line, textwrap::Options::new(wrap_width)) - .into_iter() - .map(std::borrow::Cow::into_owned), - ); + out.push( + Line::from(format_vertical_row_title(headers, row, row_index)) + .dim() + .bold(), + ); + for index in &included_columns { + let label = truncate_to_width(&vertical_label(headers, *index), label_width); + let cell = row.get(*index).map(String::as_str).unwrap_or("").trim(); + let value = if cell.is_empty() { "—" } else { cell }; + let wrapped = textwrap::wrap( + value, + textwrap::Options::new(value_width) + .break_words(false) + .word_separator(textwrap::WordSeparator::AsciiSpace) + .wrap_algorithm(textwrap::WrapAlgorithm::FirstFit), + ) + .into_iter() + .map(std::borrow::Cow::into_owned) + .collect::>(); + let wrapped = if wrapped.is_empty() { + vec![String::new()] + } else { + wrapped + }; + + for (line_index, value_line) in wrapped.iter().enumerate() { + if line_index == 0 { + let prefix = format!("{label:>label_width$}: "); + let value_span = if cell.is_empty() { + Span::from(value_line.clone()).dim() + } else { + Span::from(value_line.clone()) + }; + out.push(Line::from(vec![prefix.dim(), value_span])); + } else { + out.push(Line::from(vec![ + " ".repeat(label_width + 2).into(), + value_line.clone().into(), + ])); + } + } } } out } + +fn included_vertical_columns(headers: &[String], body_rows: &[Vec]) -> Vec { + let first_column_titles_rows = headers + .first() + .map(|header| normalized_header(header)) + .is_some_and(|header| matches!(header.as_str(), "#" | "id" | "idx" | "index" | "row")) + && headers.len() > 1; + + (0..headers.len()) + .filter(|index| !(first_column_titles_rows && *index == 0)) + .filter(|index| { + !headers[*index].trim().is_empty() + || body_rows + .iter() + .any(|row| row.get(*index).is_some_and(|cell| !cell.trim().is_empty())) + }) + .collect() +} + +fn vertical_label(headers: &[String], index: usize) -> String { + headers + .get(index) + .map(|header| header.trim()) + .filter(|header| !header.is_empty()) + .unwrap_or("Column") + .to_string() +} + +fn format_vertical_row_title(headers: &[String], row: &[String], row_index: usize) -> String { + let first_header = headers.first().map(|header| normalized_header(header)); + let first_cell = row.first().map(|cell| cell.trim()).unwrap_or(""); + if first_header + .as_deref() + .is_some_and(|header| matches!(header, "#" | "id" | "idx" | "index" | "row")) + && !first_cell.is_empty() + { + format!("Row {first_cell}") + } else { + format!("Row {}", row_index + 1) + } +} + +fn truncate_to_width(input: &str, max_width: usize) -> String { + if input.width() <= max_width { + return input.to_string(); + } + if max_width == 0 { + return String::new(); + } + if max_width == 1 { + return "…".to_string(); + } + + let mut out = String::new(); + let target = max_width - 1; + let mut width = 0usize; + for ch in input.chars() { + let ch_width = ch.to_string().width(); + if width + ch_width > target { + break; + } + out.push(ch); + width += ch_width; + } + out.push('…'); + out +} diff --git a/codex-rs/tui/src/markdown_render_tests.rs b/codex-rs/tui/src/markdown_render_tests.rs index 805f5aa81f..b544a882fb 100644 --- a/codex-rs/tui/src/markdown_render_tests.rs +++ b/codex-rs/tui/src/markdown_render_tests.rs @@ -32,6 +32,40 @@ fn table_fixture() -> &'static str { "| Area | Result |\n| --- | --- |\n| Streaming resize | This cell contains enough prose to wrap differently across widths. |\n| Scrollback preservation | SENTINEL_TABLE_VALUE_WITH_LONG_UNBREAKABLE_TOKEN |\n" } +fn dense_large_table_fixture() -> String { + let mut markdown = String::from("| # | Token | Style | Link | Code Sample |\n| --- | --- | --- | --- | --- |\n"); + for row in 1..=34 { + let token = match row { + 1 => "Alpha".to_string(), + 2 => "Beta".to_string(), + 3 => "Gamma".to_string(), + 24 => "Omega".to_string(), + _ => format!("Row {row}"), + }; + let style = match row { + 1 => "bold", + 2 => "italic", + 3 => "both", + 21 => "a \\| b", + 31 => "Mixed code", + 33 => "Escaped \\*", + 34 => "Done", + _ => "Normal", + }; + let code = match row { + 1 => "alpha()", + 2 => "beta + 1", + 3 => "Vec::::new()", + 21 => "split_fn", + _ => "row", + }; + markdown.push_str(&format!( + "| {row} | {token} | {style} | row (https://example.com/{row}) | {code}_{row} |\n" + )); + } + markdown +} + #[test] fn table_resize_lifecycle_renderer_uses_thin_borders_and_fits_widths() { for width in [36, 64, 96] { @@ -94,6 +128,112 @@ fn table_resize_lifecycle_renderer_uses_vertical_fallback_only_at_tiny_width() { ); } +#[test] +fn table_readability_fallback_keeps_dense_large_table_boxed_when_wide() { + let rendered = render_markdown_text_with_width_and_cwd( + &dense_large_table_fixture(), + Some(140), + /*cwd*/ None, + ); + let lines = plain_lines(&rendered); + + assert!( + lines.iter().any(|line| line.contains('┌')), + "wide large table should remain boxed: {lines:?}" + ); + assert!( + lines.iter().all(|line| !line.starts_with("Row 31")), + "wide large table should not use vertical fallback: {lines:?}" + ); +} + +#[test] +fn table_readability_fallback_uses_vertical_for_dense_large_table_when_narrow() { + let rendered = render_markdown_text_with_width_and_cwd( + &dense_large_table_fixture(), + Some(64), + /*cwd*/ None, + ); + let lines = plain_lines(&rendered); + + assert!( + lines.iter().all(|line| !line.contains('┌')), + "narrow large table should not render as boxed table: {lines:?}" + ); + assert!( + lines.iter().any(|line| line == "Row 31"), + "narrow large table should render row records: {lines:?}" + ); + assert!( + lines.iter().any(|line| line.contains(" Token: Row 31")) + && lines.iter().any(|line| line.contains(" Link: row")), + "vertical fallback should align labels in a gutter: {lines:?}" + ); +} + +#[test] +fn table_readability_fallback_wraps_vertical_values_under_value_column() { + let markdown = "| # | Color Sample |\n| --- | --- |\n| 31 | The color sample is the same as the others, but with a touch of white too. |\n"; + let rendered = render_markdown_text_with_width_and_cwd(markdown, Some(30), /*cwd*/ None); + let lines = plain_lines(&rendered); + + assert_eq!(lines[0], "Row 31"); + assert!( + lines + .iter() + .any(|line| line.starts_with("Color Sam…: The color")), + "first vertical value line should include the label: {lines:?}" + ); + assert!( + lines + .iter() + .skip(1) + .any(|line| line.starts_with(" ")), + "wrapped vertical value should align under the value column: {lines:?}" + ); +} + +#[test] +fn table_readability_fallback_keeps_small_status_table_boxed_when_narrow() { + let markdown = "| ID | Status | Owner | Summary |\n| --- | --- | --- | --- |\n| 1 | ✅ Done | Ana | Added markdown_table parsing |\n| 2 | 🟡 Review | Ben | Checks links like RFC (https://example.com/rfc) |\n| 3 | 🔴 Blocked | ci-bot | Fails on foo \\| bar escaping |\n| 4 | ⚪ Planned | Dana | Needs snapshot coverage |\n"; + let rendered = render_markdown_text_with_width_and_cwd(markdown, Some(64), /*cwd*/ None); + let lines = plain_lines(&rendered); + + assert!( + lines.iter().any(|line| line.contains('┌')), + "small status table should remain boxed at narrow widths: {lines:?}" + ); +} + +#[test] +fn table_readability_fallback_keeps_tiny_table_boxed() { + let markdown = "| Tiny | Table |\n| --- | --- |\n| A | B |\n| C | D |\n"; + let rendered = render_markdown_text_with_width_and_cwd(markdown, Some(40), /*cwd*/ None); + let lines = plain_lines(&rendered); + + assert!( + lines.iter().any(|line| line.contains('┌')), + "tiny table should remain boxed when there is enough width: {lines:?}" + ); +} + +#[test] +fn table_readability_fallback_uses_vertical_for_emoji_near_fit() { + let markdown = "| Feature | Markdown Coverage | Sample Output |\n| --- | --- | --- |\n| Emoji | 😎 ✅ 🧩 | Visual glyphs |\n"; + let rendered = render_markdown_text_with_width_and_cwd(markdown, Some(41), /*cwd*/ None); + let lines = plain_lines(&rendered); + + assert!( + lines.iter().all(|line| !line.contains('┌')), + "emoji-heavy near-fit table should fall back to vertical layout: {lines:?}" + ); + assert!( + lines.iter().any(|line| line == "Row 1") + && lines.iter().any(|line| line.contains("Markdown Cov…:")), + "emoji-heavy fallback should render row records: {lines:?}" + ); +} + #[test] fn table_inline_links_and_html_breaks_stay_inside_table() { let markdown = "| A | B |\n|---|---|\n| [link](https://example.com) | [CLI docs](https://example.com/cli) |\n| one
two | three
four |\n";