feat(tui): diff cards emphasise the changed words within a line - #5813
Conversation
A deleted run followed by an added run of the same length is a set of replaced lines. Each pair is now diffed at unicode word boundaries (similar's `from_unicode_words`, so punctuation stays out of the emphasis) and the words that differ are painted bold + reversed on the existing −rose / +green line; unchanged words keep the plain line style. Any other shape — unequal runs, or a pair sharing less than half its words — renders exactly as before, so a rewrite does not light up the whole row. Emphasis flags are carried across the wrapper's breaks, and the row text is unchanged. `similar` gains its `unicode` feature; unicode-segmentation was already in the tree (ratatui), so no new crate. syntect is left out: colouring hunk lines by file extension is not a small, obviously-safe addition to this renderer today. Evidence: `scripts/dev-test.sh tui diff_render` — test result: ok. 9 passed; 0 failed; 0 ignored. `scripts/dev-test.sh tui` — 11854 tests run: 11854 passed, 13 skipped (no golden changed). clippy -D warnings with CI's allow flags: clean. Signed-off-by: CodeWhale Bot <bot@codewhale.net>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_35bf9ae7-0c9f-4f9a-b309-c1e02772dae9) |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
Claude finished @Hmbown's task in 4m 54s —— View job Review: PR #5813 — diff cards emphasise the changed words within a line
Note on evidence: this sandbox doesn't have Bash/cargo execution permission for this job ( Correctness — 1 real bug found (inline comment on
|
There was a problem hiding this comment.
🟡 Changes recommended
The new run-buffering logic can allocate proportional to large hunks (even with small max_rows), undermining the function’s stated bounded-allocation behavior and risking memory spikes in the TUI.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR enhances the TUI diff-card renderer to emphasize intraline edits for 1:1 replaced lines, using similar’s word-level diffing to highlight only the changed word runs while preserving the existing per-line red/green styling and wrapping behavior.
Changes:
- Pair consecutive
-/+runs of equal length inrender_diff_body_boundedand compute per-line intraline emphasis viaTextDiff::from_unicode_words. - Extend line rendering to optionally emit multiple styled spans per wrapped chunk so emphasis survives wrapping without changing displayed text.
- Enable
similar’sunicodefeature incodewhale-tui(lockfile updated accordingly).
File summaries
| File | Description |
|---|---|
| crates/tui/src/tui/diff_render.rs | Implements run pairing + intraline emphasis, adds helpers for added/deleted detection/styles, and adds tests for emphasis behavior and wrapping. |
| crates/tui/Cargo.toml | Enables similar’s unicode feature needed for unicode word boundary diffing. |
| Cargo.lock | Updates resolved dependencies due to the new similar feature flag. |
Review details
- Files reviewed: 2/3 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if is_deleted(raw) { | ||
| // A deleted run followed by an added run of the same length is a | ||
| // set of replaced lines: emphasise the words that changed within | ||
| // each pair. Any other shape renders line by line as before. | ||
| let mut removed = vec![raw.trim_start_matches('-')]; | ||
| while let Some(next) = lines.next_if(|next| is_deleted(next)) { | ||
| removed.push(next.trim_start_matches('-')); | ||
| } | ||
| let mut added = Vec::new(); |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| fn emphasised_spans( | ||
| chunk: &str, | ||
| style: Style, | ||
| source: &[(char, bool)], | ||
| cursor: &mut usize, | ||
| ) -> Vec<Span<'static>> { | ||
| let emphasis = style.add_modifier(Modifier::BOLD | Modifier::REVERSED); | ||
| let mut spans = Vec::new(); | ||
| let mut run = String::new(); | ||
| let mut run_flag = false; | ||
| for ch in chunk.chars() { | ||
| while *cursor < source.len() | ||
| && !(source[*cursor].0 == ch | ||
| || (source[*cursor].0.is_whitespace() && ch.is_whitespace())) | ||
| { | ||
| *cursor += 1; | ||
| } | ||
| let flag = source.get(*cursor).map(|(_, flag)| *flag).unwrap_or(false); | ||
| *cursor = cursor.saturating_add(1).min(source.len()); | ||
| if flag != run_flag && !run.is_empty() { | ||
| let painted = if run_flag { emphasis } else { style }; | ||
| spans.push(Span::styled(std::mem::take(&mut run), painted)); | ||
| } | ||
| run_flag = flag; | ||
| run.push(ch); | ||
| } | ||
| if !run.is_empty() { | ||
| let painted = if run_flag { emphasis } else { style }; | ||
| spans.push(Span::styled(run, painted)); | ||
| } | ||
| spans | ||
| } |
There was a problem hiding this comment.
Correctness: emphasis desyncs (and silently disappears) on wrapped, indented lines.
wrap_text (L557+) resets current = lead.clone() at every wrap point (L591, L604), so it repeats the line's leading indentation on every continuation chunk. source (built at L500-508 from the pre-computed segments) contains that indentation only once, at the very start.
emphasised_spans's cursor (L465-472) only tolerates missing whitespace in source (it skips forward past extra source chars), not extra whitespace in chunk that has no counterpart left in source. When a continuation chunk's repeated lead is wider than the single separator space that wrapping dropped at the break (i.e. any indent ≥ 2 columns — the common case for real code), the extra lead characters skip the cursor past the real next character(s) in source without ever matching them. Once that happens cursor lands at source.len(), and every remaining char for every subsequent chunk of that logical line gets flag = false — emphasis silently vanishes for the rest of the line.
Concretely, using the PR's own example line but at a width that forces a wrap:
- let total = price * quantity;
+ let total = price * count;
at e.g. width = 20, the first wrapped chunk consumes the 4-space indent + let total = cleanly, but the second chunk re-adds (4 spaces) while only one source char (the dropped word-separator space) is available to match against — the cursor overruns into the following real characters and gets stuck at the end. The changed word (quantity/count) ends up unemphasised, or the diff panics-free but visibly wrong.
None of the three new tests exercise this: emphasis_survives_wrapping_without_changing_text deliberately uses text with no leading whitespace ("alpha beta gamma..."), so lead is empty and the repeated-indent path is never hit. Since diff cards render at the panel width (file_mutation.rs:167-189), not a fixed 80 columns, and most real replaced lines are indented Rust/etc. code, this will trigger routinely in normal use, not just in a contrived edge case.
Suggest either: (a) make wrap_text (or a variant used for emphasised lines) not repeat lead in a way that's invisible to the cursor — e.g. also advance the cursor implicitly past repeated-lead chars without requiring a source match, or (b) restructure so wrapping operates on the already-flagged segments directly (carry (char, bool) through wrap_text) rather than re-deriving alignment by heuristically re-matching plain wrapped text against the source afterward. (b) removes this whole class of desync risk.
![codewhale-agent[bot]](https://avatars.githubusercontent.com/u/101357273?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codewhale review
Adds intraline word emphasis to TUI diff rendering for 1:1 replaced line runs. The core change is well structured for the common case, but the word-similarity fallback, the chunk-to-source emphasis mapping, and a few edge-case tests need attention before merge.
Findings
- [WARNING] Intraline fallback ratio includes non-word tokens (
crates/tui/src/tui/diff_render.rs:419)
TextDiff::from_unicode_words(...).ratio()scores every token, including whitespace and punctuation. Lines with similar syntax but differing identifiers can stay aboveINTRALINE_MIN_RATIOand receive word emphasis where a wholesale rewrite was intended to be skipped. Consider filtering to word tokens only for the threshold or using a separate word-only diff ratio. - [WARNING] Emphasis alignment is heuristic and has no recovery path (
crates/tui/src/tui/diff_render.rs:455)
emphasised_spansmaps each wrapped chunk back to the originalsourceby greedy forward matching, assumingwrap_textonly drops or collapses whitespace. If that invariant changes, or a Unicode/rewrap edge case introduces a mismatch, the cursor silently drifts and subsequent words get the wrong emphasis (or none) with no panic or fallback. Add a debug assertion that non-whitespace characters are consumed in order and a fallback to whole-line styling on mismatch; add direct tests for wrapped changed-word placement. - [WARNING] Test coverage misses important replacement-shape cases (
crates/tui/src/tui/diff_render.rs)
The three new tests cover a single 1:1 pair, unequal/rewrite fallback, and a broad wrapping check. They do not cover multiple replacement pairs in one run, mixed Some/None pairs (one pair above threshold, one below), Unicode word-boundary/punctuation behavior, or exact emphasis locations after wrapping—the wrapping test concatenates all emphasised text and cannot detect emphasis on the wrong side of the line. - [INFO] No-newline markers can prevent pairing (
crates/tui/src/tui/diff_render.rs)
In unified diffs a\ No newline at end of fileline can appear between a deleted and added line.next_if(is_added)stops at that marker, so an otherwise 1:1 replaced pair is not emphasised for files without trailing newlines. The rendered text remains correct, but the new feature silently does not engage. Consider skipping these marker lines when forming runs.
Assessment
Low-risk presentation-only change with a solid common-case implementation and good basics tests. Address the threshold semantics for wholesale rewrites, make the emphasis-alignment path more defensive, and add edge-case tests before shipping.
Advisory review by Codewhale (codewhale review --pr 5813 --post, head 0a26418e12f0a9b3bd6ef25c80b34248f48cba0a). Line-specific findings are also posted as inline review comments; mechanical fixes arrive as committable suggestions you can apply from the Files tab. CODEOWNERS approval still governs merge.
| fn intraline_segments(old: &str, new: &str) -> Option<(Vec<Segment>, Vec<Segment>)> { | ||
| let diff = TextDiff::from_unicode_words(old, new); | ||
| if diff.ratio() < INTRALINE_MIN_RATIO { | ||
| return None; |
There was a problem hiding this comment.
[WARNING] Intraline fallback ratio includes non-word tokens
TextDiff::from_unicode_words(...).ratio() scores every token, including whitespace and punctuation. Lines with similar syntax but differing identifiers can stay above INTRALINE_MIN_RATIO and receive word emphasis where a wholesale rewrite was intended to be skipped. Consider filtering to word tokens only for the threshold or using a separate word-only diff ratio.
| /// across the wrap. `wrap_text` only drops or collapses whitespace, so every | ||
| /// chunk character is matched forward against the source characters. | ||
| fn emphasised_spans( | ||
| chunk: &str, |
There was a problem hiding this comment.
[WARNING] Emphasis alignment is heuristic and has no recovery path
emphasised_spans maps each wrapped chunk back to the original source by greedy forward matching, assuming wrap_text only drops or collapses whitespace. If that invariant changes, or a Unicode/rewrap edge case introduces a mismatch, the cursor silently drifts and subsequent words get the wrong emphasis (or none) with no panic or fallback. Add a debug assertion that non-whitespace characters are consumed in order and a fallback to whole-line styling on mismatch; add direct tests for wrapped changed-word placement.
… only Review findings on #5813: - Emphasis desynced on wrapped, indented lines: `wrap_text` re-inserts the indent lead on every continuation chunk and the greedy forward match drifted past it. Chunks now map each non-whitespace character back to the source by position (the invariant the wrapper keeps); whitespace joins an emphasised run only when both neighbours are in it. A `debug_assert!` pins the order, and any mismatch falls back to whole-line styling — never a wrong-word emphasis. - The similarity threshold counted whitespace and punctuation tokens, so lines agreeing only on syntax passed. The ratio now counts word tokens only (tokens bearing an alphanumeric character). - `\ No newline at end of file` markers are lifted out of a `-`/`+` run and painted after it, so files without a trailing newline still pair. - The buffered run is capped at `INTRALINE_MAX_RUN` (64) replaced pairs and the `render_diff_body_bounded` doc states the bound honestly: the retained preview, one wrapped source line, and the current run's borrowed slices plus word segments for at most 64 pairs. - Tests: multiple pairs in one run; a rewrite next to an edit in one run; syntax-only sharing stays plain; unicode word boundaries; the no-newline marker; an indented line whose change lands on the third wrapped chunk, asserting the exact text before the emphasis; and the wrapping test now asserts emphasis position per chunk. Evidence: `scripts/dev-test.sh tui diff_render` — test result: ok. 15 passed; 0 failed; 0 ignored. `scripts/dev-test.sh tui` — 11860 tests run: 11859 passed, 1 failed (chatgpt_oauth::tests::callback_is_accepted_on_either_loopback_family, untouched by this diff; passes alone: 1 passed; 0 failed — loopback port contention under six concurrent builds). clippy -D warnings with CI's allow flags: clean. Signed-off-by: CodeWhale Bot <bot@codewhale.net>
|
Review findings addressed in the follow-up commit:
Evidence: |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_2acd7a5d-cfed-4eaa-a1b2-3e3e1d28c93a) |
|
Claude encountered an error after 2m 59s —— View job Review: PR #5813 — diff cards emphasise the changed words within a line
Note on evidence: this sandbox has no Bash approval for Correctness — the previously reported wrapping bug is fixedI filed the original finding on this PR: One minor/non-blocking note: Other prior findings — also addressed
Provider/model/route safetyNot applicable — no provider, model, or routing code touched. Reuse / simplificationNo new issues. Tests15 tests now cover: single-pair emphasis, multiple pairs in one run, a rewrite mixed with an edit in the same run, syntax-only similarity (ratio fix), unequal runs, Unicode word boundaries, the no-newline marker, and — the one that mattered most — an indented line whose change lands on the third wrapped chunk. This closes every gap the earlier review rounds flagged. SecurityNo concerns — presentation-only, no new input-parsing or exec surface. AssessmentThe fix commit ( |
There was a problem hiding this comment.
Codewhale review
The PR adds intraline word emphasis for 1:1 replaced diff lines using similar's unicode word diff. The rendering logic is well covered by new tests and the fallback paths are generally safe, but the advertised allocation bound is not enforced during run collection, and no-newline marker handling has ordering and asymmetrical fallthrough issues.
Findings
- [WARNING] Run collection is unbounded despite INTRALINE_MAX_RUN (
crates/tui/src/tui/diff_render.rs:129)
The new buffering path consumes every consecutive deleted line intoremovedand every consecutive added line intoaddedbefore theremoved.len() <= INTRALINE_MAX_RUNcheck is evaluated. A large deletion-only or replacement hunk therefore allocates aVec<&str>proportional to the whole run, contradicting the bounded-allocation comment. The cap only disables emphasis after the vectors are already allocated. - [WARNING] No-newline markers outside a deletion run still render as context rows (
crates/tui/src/tui/diff_render.rs:96)
is_no_newline_markeris only consumed inside a-run. A pure insertion hunk such as+new line/\ No newline at end of filefalls through to the context branch, which renders the marker with line-number gutters and incrementsold_line/new_linefor it. That can display incorrect line numbers and is inconsistent with the header-style marker rendering used in the deletion path. - [INFO] No-newline markers are moved after the whole replaced run (
crates/tui/src/tui/diff_render.rs:181)
In the deletion-run path, markers are collected and rendered after all removed and added lines. For a replacement pair such as-old line, marker,+new line, marker, the rendered order becomes old, new, marker, marker. The marker no longer sits next to the line missing the trailing newline, so a single marker can appear to belong to the added line when it actually belongs to the deleted line. Tests assert marker count and text, not placement. - [INFO] Missing regression tests for INTRALINE_MAX_RUN and pure-insertion markers (
crates/tui/src/tui/diff_render.rs)
The new suite covers replaced-pair emphasis, rewrite fallback, unicode boundaries, and wrapping, but does not cover: (a) a replacement run longer thanINTRALINE_MAX_RUN, where pairing must be skipped and allocation bounded; or (b) a no-newline marker following a pure insertion. Both paths currently lack regression protection.
Suggestions
-
crates/tui/src/tui/diff_render.rs:96— Handle no-newline markers globally before the added/deleted branches so markers outside a deletion run do not fall through to the context branch and corrupt line numbering.if is_no_newline_marker(raw) { collector.extend(render_header_line(raw, width)); continue; } if is_added(raw) {
Assessment
The feature is low-risk and well tested for its core happy paths, but the run-collection memory bound must be enforced before merge, and no-newline marker handling should be made consistent so line numbering and marker placement remain faithful.
Advisory review by Codewhale (codewhale review --pr 5813 --post, head a708452d6c25ada3631e5207145d8feb41bedafd). Line-specific findings are also posted as inline review comments; mechanical fixes arrive as committable suggestions you can apply from the Files tab. CODEOWNERS approval still governs merge.
| } else if added.is_empty() | ||
| && let Some(next) = lines.next_if(|next| is_deleted(next)) | ||
| { | ||
| removed.push(next.trim_start_matches('-')); |
There was a problem hiding this comment.
[WARNING] Run collection is unbounded despite INTRALINE_MAX_RUN
The new buffering path consumes every consecutive deleted line into removed and every consecutive added line into added before the removed.len() <= INTRALINE_MAX_RUN check is evaluated. A large deletion-only or replacement hunk therefore allocates a Vec<&str> proportional to the whole run, contradicting the bounded-allocation comment. The cap only disables emphasis after the vectors are already allocated.
| } | ||
|
|
||
| if raw.starts_with('+') && !raw.starts_with("+++") { | ||
| if is_added(raw) { |
There was a problem hiding this comment.
[WARNING] No-newline markers outside a deletion run still render as context rows
is_no_newline_marker is only consumed inside a - run. A pure insertion hunk such as +new line / \ No newline at end of file falls through to the context branch, which renders the marker with line-number gutters and increments old_line/new_line for it. That can display incorrect line numbers and is inconsistent with the header-style marker rendering used in the deletion path.
| *line = line.saturating_add(1); | ||
| } | ||
| } | ||
| for marker in markers { |
There was a problem hiding this comment.
[INFO] No-newline markers are moved after the whole replaced run
In the deletion-run path, markers are collected and rendered after all removed and added lines. For a replacement pair such as -old line, marker, +new line, marker, the rendered order becomes old, new, marker, marker. The marker no longer sits next to the line missing the trailing newline, so a single marker can appear to belong to the added line when it actually belongs to the deleted line. Tests assert marker count and text, not placement.
| } | ||
|
|
||
| if raw.starts_with('+') && !raw.starts_with("+++") { | ||
| if is_added(raw) { |
There was a problem hiding this comment.
Handle no-newline markers globally before the added/deleted branches so markers outside a deletion run do not fall through to the context branch and corrupt line numbering.
| if is_added(raw) { | |
| if is_no_newline_marker(raw) { | |
| collector.extend(render_header_line(raw, width)); | |
| continue; | |
| } | |
| if is_added(raw) { |
Keep the unicode feature on 'similar' (intraline word segmentation) and take main's new ansi-to-tui dependency. Signed-off-by: CodeWhale Bot <bot@codewhale.net>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_2c3e0546-4adf-4928-b197-581e968ccad3) |
Signed-off-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-Authored-By: Hunter Bown <hmbown@gmail.com>
…to feat/diff-intraline-20260901
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| ChangeTag::Delete => { | ||
| changed = true; | ||
| total_words += usize::from(is_word); | ||
| push_segment(&mut old_segments, text, true); | ||
| } | ||
| ChangeTag::Insert => { | ||
| changed = true; | ||
| total_words += usize::from(is_word); | ||
| push_segment(&mut new_segments, text, true); |
There was a problem hiding this comment.
🟡 Punctuation receives changed-word emphasis
When similar lines differ only in punctuation, push_segment emphasizes those punctuation tokens. Diff cards highlight symbols instead of changed words.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
| let mut total_words = 0usize; | ||
| for change in diff.iter_all_changes() { | ||
| let text = change.value(); | ||
| let is_word = text.chars().any(char::is_alphanumeric); |
There was a problem hiding this comment.
🟡 Rewrite threshold miscounts adjacent words
When many adjacent words change, intraline_segments counts the entire changed run once. Wholesale rewrites can reach the 0.5 threshold and receive misleading emphasis.
Was this helpful? React with 👍 or 👎 to provide feedback.
| .map(move |ch| (ch, *flag)) | ||
| }) | ||
| .collect(); | ||
| let emphasis = style.add_modifier(Modifier::BOLD | Modifier::REVERSED); |
There was a problem hiding this comment.
🔍 Terminal appearance remains unverified
Span assertions cannot establish that reversed red and green text stays legible across supported terminals. Capture the changed cards at the prescribed terminal sizes.
Was this helpful? React with 👍 or 👎 to provide feedback.
Signed-off-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-Authored-By: Hunter Bown <hmbown@gmail.com>
| &mut new_line, | ||
| '-', | ||
| ); | ||
| continue 'line; |
There was a problem hiding this comment.
🟡 Oversized replacements gain false emphasis
When a deletion run exceeds 64 lines and its remaining tail matches the addition count, continue 'line restarts pairing mid-run. Unequal replacements receive misleading changed-word emphasis.
Prompt for agents
In crates/tui/src/tui/diff_render.rs, render_diff_body_bounded restarts the outer parser after overflowing the 64-line removed buffer. The unconsumed tail can then be interpreted as a fresh, equal-length replacement and receive intraline emphasis even though the complete replacement run exceeded the limit or had unequal lengths. Preserve the bounded buffering, but once either side exceeds INTRALINE_MAX_RUN, render the entire remainder of that contiguous deleted/added/marker run plainly without allowing pairing to restart inside it. Add a regression case such as 100 deleted lines followed by 35 added lines and assert that no row is emphasized.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Codewhale review
The PR adds intraline word emphasis for equal-length replaced runs in the TUI diff renderer. The line pairing and wrapping logic is well structured and the new tests cover the main paths, but there is a correctness bug where punctuation/whitespace changes are still emphasised despite the stated word-only intent, and a secondary user-visible change where no-newline markers are now rendered as header rows.
Findings
- [WARNING] Punctuation and whitespace changes are emphasised as word changes (
crates/tui/src/tui/diff_render.rs:532)
Inintraline_segments, Delete/Insert tokens are always pushed withtrueeven when they are not words (is_wordis false). A line pair whose words are mostly identical but where only punctuation or whitespace changes will pass the ratio check and highlight the punctuation/whitespace, contradicting the comment and PR claim that punctuation stays out of the emphasis. For example,let x = 1;vslet x = 1,would highlight;and,. A test for punctuation-only or whitespace-only changes is missing. - [WARNING] No-newline markers are now rendered as diff header rows (
crates/tui/src/tui/diff_render.rs:109)
Previously,\ No newline at end of filelines fell through the old loop and were not rendered. The new branch renders these markers withrender_header_line, adding visible header-styled rows for every diff containing such markers and consuming rows from the bounded preview budget. This is a user-visible output change outside the stated intraline-emphasis scope. If this visibility is intentional it should be reflected in the PR description; otherwise, markers should be peeled for pairing but not rendered as header rows.
Suggestions
-
crates/tui/src/tui/diff_render.rs:532— Useis_wordrather thantrueso only word tokens are emphasised, keeping punctuation and whitespace changes plain.push_segment(&mut old_segments, text, is_word); -
crates/tui/src/tui/diff_render.rs:537— Useis_wordrather thantruefor inserted tokens so only word tokens are emphasised.push_segment(&mut new_segments, text, is_word);
Assessment
The implementation is generally solid and well-tested, but the punctuation/whitespace emphasis bug should be fixed before merge because it produces incorrect highlighting in common edits. The no-newline marker rendering change should either be made intentional and documented, or adjusted to preserve the previous output.
Advisory review by Codewhale (codewhale review --pr 5813 --post, head b2ccdde16fd96569ea15bf6abfa9334204da17f0). Line-specific findings are also posted as inline review comments; mechanical fixes arrive as committable suggestions you can apply from the Files tab. CODEOWNERS approval still governs merge.
| changed = true; | ||
| total_words += usize::from(is_word); | ||
| push_segment(&mut old_segments, text, true); | ||
| } |
There was a problem hiding this comment.
[WARNING] Punctuation and whitespace changes are emphasised as word changes
In intraline_segments, Delete/Insert tokens are always pushed with true even when they are not words (is_word is false). A line pair whose words are mostly identical but where only punctuation or whitespace changes will pass the ratio check and highlight the punctuation/whitespace, contradicting the comment and PR claim that punctuation stays out of the emphasis. For example, let x = 1; vs let x = 1, would highlight ; and ,. A test for punctuation-only or whitespace-only changes is missing.
| Style::default() | ||
| .fg(palette::DIFF_ADDED) | ||
| .bg(palette::DIFF_ADDED_BG), | ||
| added_style(), |
There was a problem hiding this comment.
[WARNING] No-newline markers are now rendered as diff header rows
Previously, \ No newline at end of file lines fell through the old loop and were not rendered. The new branch renders these markers with render_header_line, adding visible header-styled rows for every diff containing such markers and consuming rows from the bounded preview budget. This is a user-visible output change outside the stated intraline-emphasis scope. If this visibility is intentional it should be reflected in the PR description; otherwise, markers should be peeled for pairing but not rendered as header rows.
| changed = true; | ||
| total_words += usize::from(is_word); | ||
| push_segment(&mut old_segments, text, true); | ||
| } |
There was a problem hiding this comment.
Use is_word rather than true so only word tokens are emphasised, keeping punctuation and whitespace changes plain.
| } | |
| push_segment(&mut old_segments, text, is_word); |
| changed = true; | ||
| total_words += usize::from(is_word); | ||
| push_segment(&mut new_segments, text, true); | ||
| } |
There was a problem hiding this comment.
Use is_word rather than true for inserted tokens so only word tokens are emphasised.
| } | |
| push_segment(&mut new_segments, text, is_word); |
No-Issue: internal 0.9.12 shell wave slice.
Rendering wave R5 (founder's list, rung 5 of the ladder: both crates already installed).
What changed for the user. In a diff card, when a line was replaced by another line, the words that actually changed are now emphasised (bold + reversed) on the red/green line, so a review card reads at a glance instead of forcing a word-by-word compare. Only 1:1 replaced runs get this; unequal runs and wholesale rewrites (less than half the words shared) render exactly as before.
Why.
diff_render.rspainted whole lines −rose/+green and never usedsimilar's inline ops even though the crate was already the diff engine; review cards were hard to scan.How.
render_diff_body_boundedbuffers a-run and the+run that follows it; equal-length runs are paired and each pair goes throughTextDiff::from_unicode_words(unicode word boundaries keep punctuation out of the emphasis). Emphasis flags are carried across the wrapper's breaks; line text is byte-identical to before.similargains itsunicodefeature —unicode-segmentationwas already in the tree, no new crate. syntect is not used here: colouring hunk lines by extension was not a small, obviously-safe addition to this renderer.Evidence.
scripts/dev-test.sh tui diff_render—test result: ok. 9 passed; 0 failed; 0 ignored(3 new tests: replaced-pair emphasis, unequal/rewrite fall-through, emphasis across wrapping).scripts/dev-test.sh tui—11854 tests run: 11854 passed, 13 skipped; no golden changed.cargo clippy -p codewhale-tui --all-targets -- -D warningswith CI's allow flags: clean.Refs: 0.9.12 runlog, rendering wave R5.
Note
Low Risk
TUI-only diff presentation with bounded allocation and fallbacks to unchanged line rendering; no security or persistence impact.
Overview
Diff cards now highlight only the words that changed when a deleted line is replaced by an added line of the same length, instead of painting the whole row red/green.
The renderer buffers equal-length
-/+runs, pairs lines, and usessimilar’s unicode word diff (TextDiff::from_unicode_words) to build emphasis segments. Changed tokens get bold + reversed styling; line text stays the same. Emphasis is skipped for unequal runs, runs longer than 64 lines, pairs with low shared-word ratio (treats them as rewrites), and lines that differ only in syntax.\ No newline at end of filemarkers are peeled off so pairing still works. Wrapping maps emphasis per character with a safe fallback to plain lines if mapping fails.similaris enabled with theunicodefeature incodewhale-tui(lockfile pulls inunicode-segmentation). New unit tests cover pairing, unicode words, wrapping, and newline markers.Reviewed by Cursor Bugbot for commit 812e2f5. Bugbot is set up for automated code reviews on this repo. Configure here.