From 45d6c74682d37032543713c577ad70c33075feb0 Mon Sep 17 00:00:00 2001 From: Jeremy Rose <172423086+nornagon-openai@users.noreply.github.com> Date: Fri, 15 Aug 2025 12:32:45 -0400 Subject: [PATCH] tui: align diff display by always showing sign char and keeping fixed gutter (#2353) diff lines without a sign char were misaligned. --- AGENTS.md | 16 ++++++++++++++ codex-rs/tui/src/diff_render.rs | 21 ++++++++----------- ...er__tests__update_details_with_rename.snap | 5 +++-- ...f_render__tests__wrap_behavior_insert.snap | 3 ++- 4 files changed, 30 insertions(+), 15 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index e5520d32..264b1e04 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -11,3 +11,19 @@ In the codex-rs folder where the rust code lives: Before finalizing a change to `codex-rs`, run `just fmt` (in `codex-rs` directory) to format the code and `just fix` (in `codex-rs` directory) to fix any linter issues in the code. Additionally, run the tests: 1. Run the test for the specific project that was changed. For example, if changes were made in `codex-rs/tui`, run `cargo test -p codex-tui`. 2. Once those pass, if any changes were made in common, core, or protocol, run the complete test suite with `cargo test --all-features`. + +## Snapshot tests + +This repo uses snapshot tests (via `insta`), especially in `codex-rs/tui`, to validate rendered output. When UI or text output changes intentionally, update the snapshots as follows: + +- Run tests to generate any updated snapshots: + - `cargo test -p codex-tui` +- Check what’s pending: + - `cargo insta pending-snapshots -p codex-tui` +- Review changes by reading the generated `*.snap.new` files directly in the repo, or preview a specific file: + - `cargo insta show -p codex-tui path/to/file.snap.new` +- Only if you intend to accept all new snapshots in this crate, run: + - `cargo insta accept -p codex-tui` + +If you don’t have the tool: +- `cargo install cargo-insta` diff --git a/codex-rs/tui/src/diff_render.rs b/codex-rs/tui/src/diff_render.rs index 152a8e28..24efefef 100644 --- a/codex-rs/tui/src/diff_render.rs +++ b/codex-rs/tui/src/diff_render.rs @@ -269,9 +269,9 @@ fn push_wrapped_diff_line( let mut remaining_text: &str = text; // Reserve a fixed number of spaces after the line number so that content starts - // at a consistent column. The sign ("+"/"-") is rendered as part of the content - // and colored with the same foreground as the edited text, not as a separate - // dimmed column. + // at a consistent column. Content includes a 1-character diff sign prefix + // ("+"/"-" for inserts/deletes, or a space for context lines) so alignment + // stays consistent across all diff lines. let gap_after_ln = SPACES_AFTER_LINE_NUMBER.saturating_sub(ln_str.len()); let first_prefix_cols = indent.len() + ln_str.len() + gap_after_ln; let cont_prefix_cols = indent.len() + ln_str.len() + gap_after_ln; @@ -306,14 +306,10 @@ fn push_wrapped_diff_line( spans.push(RtSpan::raw(indent)); spans.push(RtSpan::styled(ln_str.clone(), style_dim())); spans.push(RtSpan::raw(" ".repeat(gap_after_ln))); - - // Prefix the content with the sign if it is an insertion or deletion, and color - // the sign and content with the same foreground as the edited text. - let display_chunk = match sign_opt { - Some(sign_char) => format!("{sign_char}{chunk}"), - None => chunk.to_string(), - }; - + // Always include a sign character at the start of the displayed chunk + // ('+' for insert, '-' for delete, ' ' for context) so gutters align. + let sign_char = sign_opt.unwrap_or(' '); + let display_chunk = format!("{sign_char}{chunk}"); let content_span = match line_style { Some(style) => RtSpan::styled(display_chunk, style), None => RtSpan::raw(display_chunk), @@ -326,8 +322,9 @@ fn push_wrapped_diff_line( lines.push(line); first = false; } else { + // Continuation lines keep a space for the sign column so content aligns let hang_prefix = format!( - "{indent}{}{}", + "{indent}{}{} ", " ".repeat(ln_str.len()), " ".repeat(gap_after_ln) ); diff --git a/codex-rs/tui/src/snapshots/codex_tui__diff_render__tests__update_details_with_rename.snap b/codex-rs/tui/src/snapshots/codex_tui__diff_render__tests__update_details_with_rename.snap index d49ce8ad..76151409 100644 --- a/codex-rs/tui/src/snapshots/codex_tui__diff_render__tests__update_details_with_rename.snap +++ b/codex-rs/tui/src/snapshots/codex_tui__diff_render__tests__update_details_with_rename.snap @@ -1,13 +1,14 @@ --- source: tui/src/diff_render.rs +assertion_line: 380 expression: terminal.backend() --- "proposed patch to 1 file (+1 -1) " " └ src/lib.rs → src/lib_new.rs (+1 -1) " -" 1 line one " +" 1 line one " " 2 -line two " " 2 +line two changed " -" 3 line three " +" 3 line three " " " " " " " diff --git a/codex-rs/tui/src/snapshots/codex_tui__diff_render__tests__wrap_behavior_insert.snap b/codex-rs/tui/src/snapshots/codex_tui__diff_render__tests__wrap_behavior_insert.snap index 641552d5..14ff468e 100644 --- a/codex-rs/tui/src/snapshots/codex_tui__diff_render__tests__wrap_behavior_insert.snap +++ b/codex-rs/tui/src/snapshots/codex_tui__diff_render__tests__wrap_behavior_insert.snap @@ -1,9 +1,10 @@ --- source: tui/src/diff_render.rs +assertion_line: 380 expression: terminal.backend() --- " 1 +this is a very long line that should wrap across multiple terminal col " -" umns and continue " +" umns and continue " " " " " " "