From fc40b0394eb79bb15d0e575df489eb41c746be71 Mon Sep 17 00:00:00 2001 From: Felipe Coury Date: Fri, 1 May 2026 15:50:17 -0300 Subject: [PATCH] fix(tui): keep narrow markdown tables boxed when readable --- codex-rs/tui/src/markdown_render/table.rs | 155 +++++++++++++++++++--- codex-rs/tui/src/markdown_render_tests.rs | 109 ++++++++++----- 2 files changed, 214 insertions(+), 50 deletions(-) diff --git a/codex-rs/tui/src/markdown_render/table.rs b/codex-rs/tui/src/markdown_render/table.rs index dfe2d771aa..84857c182e 100644 --- a/codex-rs/tui/src/markdown_render/table.rs +++ b/codex-rs/tui/src/markdown_render/table.rs @@ -23,6 +23,14 @@ struct TableMetrics { hard_wrap_count: usize, } +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +enum TableFallbackPolicy { + LastResort, + Balanced, +} + +const DEFAULT_TABLE_FALLBACK_POLICY: TableFallbackPolicy = TableFallbackPolicy::LastResort; + pub(super) fn render_table_lines( rows: &[Vec], width: Option, @@ -40,7 +48,13 @@ pub(super) fn render_table_lines( let normalized_rows = normalize_table_rows(rows, column_count); let widths = desired_column_widths(&normalized_rows, column_count); - match choose_table_layout(&normalized_rows, &widths, available_width, column_count) { + match choose_table_layout( + &normalized_rows, + &widths, + available_width, + column_count, + current_table_fallback_policy(), + ) { Some(candidate) => render_box_table( &normalized_rows, &candidate.column_widths, @@ -51,6 +65,16 @@ pub(super) fn render_table_lines( } } +fn current_table_fallback_policy() -> TableFallbackPolicy { + match std::env::var("CODEX_TUI_TABLE_FALLBACK_POLICY") + .ok() + .as_deref() + { + Some("balanced") => TableFallbackPolicy::Balanced, + _ => DEFAULT_TABLE_FALLBACK_POLICY, + } +} + pub(super) fn normalize_table_boundaries(input: &str) -> Cow<'_, str> { if !input.contains('|') { return Cow::Borrowed(input); @@ -188,12 +212,16 @@ fn choose_table_layout( desired_widths: &[usize], available_width: usize, column_count: usize, + policy: TableFallbackPolicy, ) -> Option { - if available_width < 32 || width_shape_requires_vertical(column_count, available_width) { + if policy == TableFallbackPolicy::Balanced + && (available_width < 32 || width_shape_requires_vertical(column_count, available_width)) + { return None; } let normal = allocate_table_widths( + rows, desired_widths, available_width, column_count, @@ -206,6 +234,7 @@ fn choose_table_layout( widths, available_width, /*padding*/ 1, + policy, ) }); if normal.is_some() { @@ -213,6 +242,7 @@ fn choose_table_layout( } allocate_table_widths( + rows, desired_widths, available_width, column_count, @@ -225,6 +255,7 @@ fn choose_table_layout( widths, available_width, /*padding*/ 0, + policy, ) }) } @@ -239,6 +270,7 @@ fn build_table_candidate( column_widths: Vec, available_width: usize, padding: usize, + policy: TableFallbackPolicy, ) -> 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; @@ -251,15 +283,25 @@ fn build_table_candidate( (false, metrics) }; - if should_render_vertical( - rows, - desired_widths, - &column_widths, - available_width, - padding, - hard_wrap, - &metrics, - ) { + let should_fallback = match policy { + TableFallbackPolicy::LastResort => should_render_vertical_last_resort( + rows, + &column_widths, + available_width, + padding, + &metrics, + ), + TableFallbackPolicy::Balanced => should_render_vertical_balanced( + rows, + desired_widths, + &column_widths, + available_width, + padding, + hard_wrap, + &metrics, + ), + }; + if should_fallback { return None; } @@ -271,6 +313,7 @@ fn build_table_candidate( } fn allocate_table_widths( + rows: &[Vec], desired_widths: &[usize], available_width: usize, column_count: usize, @@ -286,15 +329,54 @@ fn allocate_table_widths( let mut widths = vec![3; column_count]; let mut remaining = available_content_width - min_total; - while remaining > 0 { + + let basic_targets = desired_widths + .iter() + .enumerate() + .map(|(index, desired)| { + let header = rows + .first() + .and_then(|row| row.get(index)) + .map(normalized_header) + .unwrap_or_default(); + let is_icon_column = matches!(header.as_str(), "icon" | "emoji"); + let compact_target = if is_index_column(rows, index) || is_icon_column { + 4 + } else { + 6 + }; + (*desired).min(compact_target).max(3) + }) + .collect::>(); + grow_columns_to_targets(&mut widths, &mut remaining, &basic_targets); + + let content_targets = desired_widths + .iter() + .enumerate() + .map(|(index, desired)| { + if is_content_heavy_column(rows, index) { + (*desired).min(24).max(widths[index]) + } else { + widths[index] + } + }) + .collect::>(); + grow_columns_to_targets(&mut widths, &mut remaining, &content_targets); + grow_columns_to_targets(&mut widths, &mut remaining, desired_widths); + + Some(widths) +} + +fn grow_columns_to_targets(widths: &mut [usize], remaining: &mut usize, targets: &[usize]) { + while *remaining > 0 { let mut changed = false; - for index in 0..column_count { - if remaining == 0 { + for index in 0..widths.len() { + if *remaining == 0 { break; } - if widths[index] < desired_widths[index] { + if widths[index] < targets[index] { widths[index] += 1; - remaining -= 1; + *remaining -= 1; changed = true; } } @@ -302,8 +384,6 @@ fn allocate_table_widths( break; } } - - Some(widths) } fn render_box_table( @@ -480,7 +560,44 @@ fn cell_needs_hard_wrap(cell: &TableCell, width: usize) -> bool { .any(|token| token.width() > width) } -fn should_render_vertical( +fn should_render_vertical_last_resort( + rows: &[Vec], + column_widths: &[usize], + available_width: usize, + padding: usize, + metrics: &TableMetrics, +) -> bool { + let column_count = column_widths.len(); + let body_rows = rows.len().saturating_sub(1); + if metrics.max_body_row_height > 24 + || (body_rows >= 10 && metrics.average_body_row_height > 6.0) + || (body_rows >= 24 && metrics.average_body_row_height > 4.0) + { + return true; + } + + let non_index_columns = (0..column_count) + .filter(|index| !is_index_column(rows, *index)) + .count(); + let starved_non_index_columns = column_widths + .iter() + .enumerate() + .filter(|(index, width)| !is_index_column(rows, *index) && **width <= 3) + .count(); + if column_count >= 4 + && body_rows >= 12 + && non_index_columns > 0 + && starved_non_index_columns == non_index_columns + { + return true; + } + + has_width_risk_chars(rows) + && available_width <= 24 + && table_total_width(column_widths, padding) >= available_width +} + +fn should_render_vertical_balanced( rows: &[Vec], desired_widths: &[usize], column_widths: &[usize], diff --git a/codex-rs/tui/src/markdown_render_tests.rs b/codex-rs/tui/src/markdown_render_tests.rs index 69acd6254c..90ec0af594 100644 --- a/codex-rs/tui/src/markdown_render_tests.rs +++ b/codex-rs/tui/src/markdown_render_tests.rs @@ -172,55 +172,55 @@ fn table_readability_fallback_keeps_dense_large_table_boxed_when_wide() { } #[test] -fn table_readability_fallback_uses_vertical_for_dense_large_table_when_narrow() { +fn table_readability_fallback_keeps_dense_large_table_boxed_when_narrow() { let rendered = render_markdown_text_with_width_and_cwd( &dense_large_table_fixture(), - Some(64), + /*width*/ Some(64), /*cwd*/ None, ); let lines = plain_lines(&rendered); assert!( - lines.iter().any(|line| line.contains("│ Token │ Row 31")) + lines.iter().any(|line| line.starts_with("┌")) && lines .iter() - .any(|line| line.contains("│ Link │ row (https://example.com/31)")), - "vertical fallback should render aligned key/value records: {lines:?}" + .any(|line| line.contains("│ 31") && line.contains("Row 31")), + "last-resort fallback should keep narrow dense tables boxed with wrapping: {lines:?}" ); assert!( - lines.iter().any(|line| line.contains('┼')), - "vertical fallback should separate source rows: {lines:?}" - ); - assert!( - lines.iter().all(|line| !line.starts_with("Row ")), - "vertical fallback should not render row headers: {lines:?}" + lines + .iter() + .all(|line| !line.contains("│ Token │ Row 31")), + "narrow dense table should not use key/value fallback: {lines:?}" ); assert_table_lines_leave_wrap_safety_column(&lines, /*width*/ 64); } #[test] -fn table_readability_fallback_wraps_vertical_values_under_value_column() { +fn table_readability_fallback_wraps_boxed_values_before_fallback() { 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 rendered = + render_markdown_text_with_width_and_cwd(markdown, /*width*/ Some(30), /*cwd*/ None); let lines = plain_lines(&rendered); - assert_eq!(lines[0], "┌──────────────┬────────────┐"); assert!( lines .iter() - .any(|line| line.starts_with("│ Color Sample │ The color")), - "first vertical value line should include the boxed label: {lines:?}" + .any(|line| line.contains("│ 31") && line.contains("The color sample")), + "first boxed value line should include the row value: {lines:?}" ); assert!( lines .iter() .skip(1) - .any(|line| line.starts_with("│ │ sample")), - "wrapped vertical value should align under the value column: {lines:?}" + .any(|line| line.contains("│ │") && line.contains("the same as the")), + "wrapped boxed value should align under its column: {lines:?}" ); assert!( - lines.iter().all(|line| !line.starts_with("Row ")), - "vertical fallback should not render row headers: {lines:?}" + lines + .iter() + .all(|line| !line.contains("│ Color Sample │ The color")), + "wrappable two-column table should not use key/value fallback: {lines:?}" ); assert_table_lines_leave_wrap_safety_column(&lines, /*width*/ 30); } @@ -228,7 +228,8 @@ fn table_readability_fallback_wraps_vertical_values_under_value_column() { #[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 rendered = + render_markdown_text_with_width_and_cwd(markdown, /*width*/ Some(64), /*cwd*/ None); let lines = plain_lines(&rendered); assert!( @@ -238,10 +239,33 @@ fn table_readability_fallback_keeps_small_status_table_boxed_when_narrow() { assert_table_lines_leave_wrap_safety_column(&lines, /*width*/ 64); } +#[test] +fn table_readability_fallback_keeps_video_small_table_boxed() { + let markdown = "| Emoji | Title | Content | State | Notes |\n| --- | --- | --- | --- | --- |\n| 🚀 | Alpha | **bold** text | done | launch |\n| 🎯 | Beta | *italic* text | pending | target |\n| 💡 | Gamma | `inline code` | review | snippet |\n| 🔗 | Delta | [delta](https://example.com/delta) | done | link cell |\n| ✨ | Epsilon | ~~strike~~ | done | cleanup |\n"; + let rendered = + render_markdown_text_with_width_and_cwd(markdown, /*width*/ Some(64), /*cwd*/ None); + let lines = plain_lines(&rendered); + + assert!( + lines + .iter() + .any(|line| line.starts_with("│ 🚀") && line.contains("Alpha")), + "video small table should render as a boxed table at 64 columns: {lines:?}" + ); + assert!( + lines + .iter() + .all(|line| !line.contains("│ Emoji │ 🚀")), + "video small table should not use key/value fallback at 64 columns: {lines:?}" + ); + assert_table_lines_leave_wrap_safety_column(&lines, /*width*/ 64); +} + #[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 rendered = + render_markdown_text_with_width_and_cwd(markdown, /*width*/ Some(40), /*cwd*/ None); let lines = plain_lines(&rendered); assert!( @@ -252,28 +276,52 @@ fn table_readability_fallback_keeps_tiny_table_boxed() { } #[test] -fn table_readability_fallback_uses_vertical_for_emoji_near_fit() { +fn table_readability_fallback_keeps_emoji_near_fit_boxed() { 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 rendered = + render_markdown_text_with_width_and_cwd(markdown, /*width*/ Some(41), /*cwd*/ None); let lines = plain_lines(&rendered); assert!( lines .iter() - .any(|line| line.contains("│ Markdown Coverage │ 😎 ✅ 🧩")), - "emoji-heavy near-fit table should fall back to key/value layout: {lines:?}" + .any(|line| line.contains("│ Emoji") && line.contains("😎 ✅ 🧩")), + "emoji-heavy near-fit table should remain boxed: {lines:?}" ); assert!( - lines.iter().all(|line| !line.starts_with("Row ")), - "emoji-heavy fallback should not render row headers: {lines:?}" + lines + .iter() + .all(|line| !line.contains("│ Markdown Coverage │ 😎 ✅ 🧩")), + "emoji-heavy near-fit table should not use key/value fallback: {lines:?}" ); assert_table_lines_leave_wrap_safety_column(&lines, /*width*/ 41); } +#[test] +fn table_readability_fallback_uses_vertical_when_boxed_is_impossible() { + let markdown = "| A | B | C | D | E |\n| --- | --- | --- | --- | --- |\n| one | two | three | four | five |\n| six | seven | eight | nine | ten |\n"; + let rendered = + render_markdown_text_with_width_and_cwd(markdown, /*width*/ Some(20), /*cwd*/ None); + let lines = plain_lines(&rendered); + + assert!( + lines + .iter() + .any(|line| line.contains("A │ one")), + "impossibly narrow table should use key/value fallback: {lines:?}" + ); + assert!( + lines.iter().any(|line| line.contains('┼')), + "vertical fallback should separate source rows: {lines:?}" + ); + assert_table_lines_leave_wrap_safety_column(&lines, /*width*/ 20); +} + #[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"; - let rendered = render_markdown_text_with_width_and_cwd(markdown, Some(72), /*cwd*/ None); + let rendered = + render_markdown_text_with_width_and_cwd(markdown, /*width*/ Some(72), /*cwd*/ None); let lines = plain_lines(&rendered); assert!( @@ -324,8 +372,7 @@ fn table_preserves_inline_styles_in_boxed_layout() { #[test] fn table_preserves_inline_styles_in_vertical_layout() { - let markdown = - "| Feature | Sample | Notes |\n| --- | --- | --- |\n| Code | `cargo test` | **done** |\n"; + let markdown = "| # | Feature | Sample | Notes | Extra | A | B | C |\n| --- | --- | --- | --- | --- | --- | --- | --- |\n| 1 | Code | `cargo test` | **done** | x | y | z | q |\n"; let rendered = render_markdown_text_with_width_and_cwd(markdown, /*width*/ Some(30), /*cwd*/ None); let lines = plain_lines(&rendered);