Skip to content

fix(diff): scope whitespace entity conversion to changed tokens only - #18

Merged
jfyne merged 1 commit into
masterfrom
worktree-fix-diff-highlighting
Apr 13, 2026
Merged

fix(diff): scope whitespace entity conversion to changed tokens only#18
jfyne merged 1 commit into
masterfrom
worktree-fix-diff-highlighting

Conversation

@jfyne

@jfyne jfyne commented Apr 13, 2026

Copy link
Copy Markdown
Owner

Whats changed

Scopes the word-diff   conversion so that only changed-whitespace tokens inside intra-add/intra-del spans emit entities while equal tokens keep literal whitespace, and adds tests in internal/app/worddiff_test.go and internal/app/http_render_test.go covering the no-chroma render path and whitespace rendering.

Risk Profile

Signal Value Risk
Size 375 LOC (+369, -6) Medium
Files 3 files across 1 directory Low
Sensitive paths None matched Normal
Test coverage 2 test files / 1 production file Good
Type Bug-fix Elevated

Overall: HIGH — bug-fix Type and Medium Size are both at level 2, which escalates the Medium floor one level to High; good test coverage and no sensitive paths keep it from going higher.

Suggested Review Order

# File What it does Link
1 worddiff.go Switches equal-token rendering to html.EscapeString (literal whitespace) while keeping escapeWordToken (  for spaces/tabs) for wordDelete/wordInsert tokens inside intra-line spans. view
2 worddiff_test.go New table-driven tests for tokenizeWords, diffWordTokens, and renderIntraLineHTML, including whitespace-preservation cases where equal tokens keep literal tabs/spaces and only changed tokens emit  . view
3 http_render_test.go New end-to-end HTTP render tests exercising renderReviewHTML with RenderFile=false (no chroma), confirming theme class, comment form autofocus, and word-diff HTML presence in the rendered page. view

@jfyne
jfyne marked this pull request as ready for review April 13, 2026 10:11
Previously, the word-diff fix converted ALL whitespace (including unchanged
whitespace in equal tokens) to   entities. This caused indentation
misalignment when changed lines were compared to surrounding context lines,
since equal-token whitespace was rendered with   glyphs while context
lines used literal tabs/spaces under white-space:pre-wrap.

The fix:
- Equal tokens keep literal whitespace (spaces and tabs) so they align with
  context lines in the same diff
- Only changed-whitespace tokens inside intra-add/intra-del spans use  
  entities (to prevent browser collapse at the span boundary)
- Added comprehensive tests covering the no-chroma (RenderFile=false) case
  and whitespace rendering correctness
@jfyne
jfyne force-pushed the worktree-fix-diff-highlighting branch from 84b735e to c3aadbf Compare April 13, 2026 10:13
@jfyne
jfyne merged commit ed25f31 into master Apr 13, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant