Skip to content

fix(diff): seed newLineNumber from the hunk's to-file start - #239

Open
codymikol wants to merge 3 commits into
mainfrom
agent/issue-238
Open

fix(diff): seed newLineNumber from the hunk's to-file start#239
codymikol wants to merge 3 commits into
mainfrom
agent/issue-238

Conversation

@codymikol

Copy link
Copy Markdown
Owner

Summary

Root-causes and fixes the "always falls back to highlightLine" bug: Hunk.make seeded both the original- and new-line-number incrementers from header.fromFileLineNumbersStart, so newLineNumber was only correct when a hunk's from/to starts happened to match (typically only the first hunk in a file). Any later hunk got every Unchanged/Added line mis-numbered, which made FullFileLineHighlighter.highlight's line-content check fail and silently fall back to per-line highlighting.

  • src/main/kotlin/com/codymikol/data/diff/Hunk.kt — seed newLineNumberIncrementer from header.toFileLineNumbersStart instead.
  • src/test/kotlin/com/codymikol/data/diff/HunkSpec.kt — new unit coverage for Hunk.make's per-line numbering (previously untested; only HunkHeaderSpec covered header parsing), including the bare single-line header form (@@ -20 +25 @@).
  • src/test/kotlin/com/codymikol/highlighting/FullFileLineHighlighterSpec.kt — new integration case with a real two-hunk diff whose from/to starts diverge, proving highlight() succeeds (and produces a real span) on the second hunk's line instead of returning null.
  • src/main/kotlin/com/codymikol/components/commit/diff/Diff.kt — comment only, explaining why the highlightLine fallback call is kept.

Closes #238

Note for reviewer — one open question

The issue text says the highlightLine fallback in Diff.kt:180 "should be removed" and Diff.kt "should always be using highlightLineFromFullFile". I did not do that, and want a maintainer to confirm before it happens:

FullFileLineHighlighter.highlight legitimately returns null in cases that aren't bugs — a stash entry (getFullContent can't produce one), or an implausibly large file (the MAX_FULL_FILE_CHARS guard) — both of which have existing passing tests asserting null. The old highlightLine is what currently renders any highlighting at all for those cases (it only needs the single line's text, not full-file content). Removing it outright would silently regress the Stash view and any 2MB+ file to zero syntax highlighting, not fix a bug.

The issue's own attached root-cause research comment (marked "Verdict: recommend") diagnoses the actual defect as the Hunk.kt numbering bug fixed here, and prescribes that fix specifically — it does not recommend deleting the fallback. My read is the "should be removed" line in the issue body was written before that root-cause was known, on the assumption the fallback itself was the bug. Two independent review passes on this branch flagged the same tension and both concluded it needs explicit human sign-off rather than a unilateral call either way — surfacing it here rather than picking a side.

Test plan

  • CI (./gradlew build) — this sandbox has no local JDK/nix toolchain available (nix devShell requires temurin-bin-21, not present in this container's read-only, pre-baked Nix store, and no daemon/build support is available to fetch it), so tests could not be executed locally; verified by manual trace of Hunk.make against the new HunkSpec cases instead.

bot added 3 commits July 22, 2026 10:18
Hunk.make had no direct unit test; only HunkHeaderSpec exercised
header parsing, so a wrong per-line numbering went unnoticed. Covers
both the two-value and bare single-line header forms.
Hunk.make seeded both the original- and new-line-number incrementers
from header.fromFileLineNumbersStart, so newLineNumber was correct
only when a hunk's from/to starts happened to match. Any hunk where
they diverge (any hunk after the first, or one following an uneven
add/remove count) got every Unchanged/Added line numbered wrong,
which made FullFileLineHighlighter.highlight's line-content check
fail and silently fall back to per-line highlighting.

The per-line fallback in Diff.kt itself stays: it's also the correct
behavior for content FullFileLineHighlighter legitimately can't place
(stash entries, oversized files), not just a symptom of this bug.
Adds an integration case with two hunks whose from/to line-number
starts diverge, asserting highlight() succeeds on the second hunk's
line - and produces a real highlighted span, not just a non-null
result - instead of returning null and forcing the per-line fallback.
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.

bug: Diff.kt falling back to highlightLine rather than highlightLineFromFullFile

1 participant