Commit diff view: click/drag line selection with shareable #L links on the right lines of the right chunks (follows #243) #244

New issue

Closed

opened 2026-09-09 17:23:00 +00:00 by crueber · 3 comments

crueber commented

2026-09-09 17:23:00 +00:00

Owner

Sequencing: implement after #243 (blob view line selection). This issue applies the same selection model to commit diffs, where the hard part is different: a diff has multiple files, multiple chunks per file, and TWO line-number spaces (old vs new), so the selection must land on the right lines of the right chunk.

What's requested

On /:owner/:name/commit/:sha (and the same diff component wherever it renders), clicking a diff line highlights it and updates the URL; dragging across lines highlights a contiguous range within the chunk. Shared URLs re-highlight the exact lines on load.

Current state (code evidence)

The diff renderer is web/src/pages/Commit.jsx (DiffTable, lines ~20-70): a per-file <table class="diff"> with unified mode (one line-number-less cell per line, :39-53) and split mode via splitRows (web/src/lib/diff.js:116, left/right paired rows :59-67). Cells carry only add/del styling (lineClass, :16) — no line numbers are rendered at all in either mode, and no per-line ids/anchors exist.
lib/diff.js parsePatchFiles already computes everything needed: each hunk has oldStart/oldLines/newStart/newLines (:81-88) and each line has t (' ', '+', '-') and text. Per-line new-side numbers = newStart + running count of non-- lines; old-side = oldStart + count of non-+ lines. Split-view rows don't carry numbers yet either (splitRows returns {left, right} text pairs only).
There is precedent for line-addressing on diffs: review threads anchor lines via anchorContextSha (lib/diff.js:269, server twin DriftHash in internal/review/model.go) — the drift-hash contract must not be perturbed by any numbering/display change, and ideally the new URL scheme is expressible in the same vocabulary (file path + side + line).
PullFiles.jsx renders the same parsed hunks — keep the component shared so this lands everywhere at once (verify during implementation whether Pull files view should get it in this issue or follow; the PR comment anchoring flow uses its own hash, don't conflate the two).

Proposed design

Render line numbers. Add gutter cells to both modes: unified gets one number column (new-side numbers; deletions show old-side, GitHub-style), split gets two (old left, new right). Number = derived from hunk start + running count as above. Keep leading-5 row metrics intact.
URL scheme. GitHub-compatible, file-scoped: #<path>L12 / #<path>L12-L20 (path needs percent-encoding for / and #; keep it short — first path segment disambiguation is NOT acceptable, use the full encoded path). Side disambiguation only where needed: unified deletions get L<old> with an O prefix variant (#<path>OL12) or render as old-side-labeled — pick one, document in the issue thread, keep it stable. Split mode shares the same hash (a selection is on a file line, not a view column).
Selection model. Same signals approach as #243 (anchor on mousedown, drag extends, normalize to ascending, shift-click extends): extract the anchor/focus/shift → hash-string logic into a shared pure function (it was specified headless-testable in #243) and reuse it here rather than duplicating.
Highlighting. Reuse #243's .line-hl rule; the highlight applies to the whole row (number cell + content cell). Cross-chunk drags: constrain the range to the chunk the anchor started in (dragging past a hunk header stops the range there — GitHub's behavior).
Multi-file commits. Selection is per-file (the hash carries the path); a URL with an unknown path or out-of-range line highlights nothing silently (no crash, no scroll). On load, scroll the first selected line into view.
Guard the drift-hash contract. Numbering changes are display-only; anchorContextSha/DriftHash input bytes are unchanged (pinned vector tests must stay green — they're the tripwire).

Acceptance criteria

Unified and split diff views show line-number gutters derived from hunk headers (correct per-side numbering, verified against the @@ header math).
Clicking a line sets #<encoded-path>L<n>, highlights it; drag highlights a contiguous range within one chunk and sets #<path>L<a>-L<b>; pasting the URL re-highlights and scrolls into view in a fresh tab.
Drag cannot span two chunks; dragging past a hunk header clamps the range.
Works identically in unified and split mode (same hash, same highlight).
A hash referencing a path/line not present in the current diff renders without error and without highlighting.
anchorContextSha pinned test vectors unchanged (drift-hash contract intact); headless test for the shared selection→hash function extended to cover the file-path prefix form.
Light and dark themes.

**Sequencing: implement after #243** (blob view line selection). This issue applies the same selection model to commit diffs, where the hard part is different: a diff has multiple files, multiple chunks per file, and TWO line-number spaces (old vs new), so the selection must land on the right lines of the right chunk. ## What's requested On `/:owner/:name/commit/:sha` (and the same diff component wherever it renders), clicking a diff line highlights it and updates the URL; dragging across lines highlights a contiguous range within the chunk. Shared URLs re-highlight the exact lines on load. ## Current state (code evidence) - The diff renderer is `web/src/pages/Commit.jsx` (`DiffTable`, lines ~20-70): a per-file `<table class="diff">` with **unified** mode (one line-number-less cell per line, :39-53) and **split** mode via `splitRows` (`web/src/lib/diff.js:116`, left/right paired rows :59-67). Cells carry only add/del styling (`lineClass`, :16) — **no line numbers are rendered at all** in either mode, and no per-line ids/anchors exist. - `lib/diff.js` `parsePatchFiles` already computes everything needed: each hunk has `oldStart/oldLines/newStart/newLines` (:81-88) and each line has `t` (`' '`, `'+'`, `'-'`) and `text`. Per-line new-side numbers = `newStart` + running count of non-`-` lines; old-side = `oldStart` + count of non-`+` lines. Split-view rows don't carry numbers yet either (`splitRows` returns `{left, right}` text pairs only). - There is precedent for line-addressing on diffs: review threads anchor lines via `anchorContextSha` (`lib/diff.js:269`, server twin `DriftHash` in `internal/review/model.go`) — the drift-hash contract must not be perturbed by any numbering/display change, and ideally the new URL scheme is expressible in the same vocabulary (file path + side + line). - `PullFiles.jsx` renders the same parsed hunks — keep the component shared so this lands everywhere at once (verify during implementation whether Pull files view should get it in this issue or follow; the PR comment anchoring flow uses its own hash, don't conflate the two). ## Proposed design 1. **Render line numbers.** Add gutter cells to both modes: unified gets one number column (new-side numbers; deletions show old-side, GitHub-style), split gets two (old left, new right). Number = derived from hunk start + running count as above. Keep `leading-5` row metrics intact. 2. **URL scheme.** GitHub-compatible, file-scoped: `#<path>L12` / `#<path>L12-L20` (path needs percent-encoding for `/` and `#`; keep it short — first path segment disambiguation is NOT acceptable, use the full encoded path). Side disambiguation only where needed: unified deletions get `L<old>` with an `O` prefix variant (`#<path>OL12`) or render as old-side-labeled — pick one, document in the issue thread, keep it stable. Split mode shares the same hash (a selection is on a file line, not a view column). 3. **Selection model.** Same signals approach as #243 (anchor on mousedown, drag extends, normalize to ascending, shift-click extends): extract the anchor/focus/shift → hash-string logic into a shared pure function (it was specified headless-testable in #243) and reuse it here rather than duplicating. 4. **Highlighting.** Reuse #243's `.line-hl` rule; the highlight applies to the whole row (number cell + content cell). Cross-chunk drags: constrain the range to the chunk the anchor started in (dragging past a hunk header stops the range there — GitHub's behavior). 5. **Multi-file commits.** Selection is per-file (the hash carries the path); a URL with an unknown path or out-of-range line highlights nothing silently (no crash, no scroll). On load, scroll the first selected line into view. 6. **Guard the drift-hash contract.** Numbering changes are display-only; `anchorContextSha`/`DriftHash` input bytes are unchanged (pinned vector tests must stay green — they're the tripwire). ## Acceptance criteria - [ ] Unified and split diff views show line-number gutters derived from hunk headers (correct per-side numbering, verified against the `@@` header math). - [ ] Clicking a line sets `#<encoded-path>L<n>`, highlights it; drag highlights a contiguous range within one chunk and sets `#<path>L<a>-L<b>`; pasting the URL re-highlights and scrolls into view in a fresh tab. - [ ] Drag cannot span two chunks; dragging past a hunk header clamps the range. - [ ] Works identically in unified and split mode (same hash, same highlight). - [ ] A hash referencing a path/line not present in the current diff renders without error and without highlighting. - [ ] `anchorContextSha` pinned test vectors unchanged (drift-hash contract intact); headless test for the shared selection→hash function extended to cover the file-path prefix form. - [ ] Light and dark themes.

crueber referenced this issue from a commit

2026-09-09 20:57:54 +00:00

Fix #244: diff line selection with #L links (12_web_ui.md §2.8)

crueber referenced this issue from a pull request that will close it,

2026-09-09 20:57:59 +00:00

Fix #244: diff line selection with #L links #257

crueber commented

2026-09-09 20:58:03 +00:00

Author

Owner

PR #257 (fix/issue-244) implements this: shared selectable DiffBody for commit + PR-files diffs with @@-derived gutters, file-scoped #L hashes (O prefix for old-side: #OL12 / #OL12-OL20 — same hash in unified + split), chunk-clamped drags, and hash-load re-highlight. Drift-hash contract intact (pinned vector green). node --test 500/500 green, vite+esbuild clean. Browser pass (unified + split, both themes, zero console errors) still open — needs real Chromium before merge.

PR #257 (fix/issue-244) implements this: shared selectable DiffBody for commit + PR-files diffs with @@-derived gutters, file-scoped #L hashes (O prefix for old-side: #<path>OL12 / #<path>OL12-OL20 — same hash in unified + split), chunk-clamped drags, and hash-load re-highlight. Drift-hash contract intact (pinned vector green). node --test 500/500 green, vite+esbuild clean. Browser pass (unified + split, both themes, zero console errors) still open — needs real Chromium before merge.

crueber commented

2026-09-09 21:08:15 +00:00

Author

Owner

Review of PR #257 (fix/issue-244, diff line selection) — verified in scratch worktree at 5b4df56; main worktree untouched.

PASS — hunk math (web/src/lib/diff-lines.js:35-60): walked deletion hunk (hunk0: ' 'a/'-'b/'+'c/'+'d/' 'e from -1,3/+1,4 → old [1,2,null,null,3], new [1,null,2,3,4]); newNo advances on non-'-', oldNo on non-'+'; 0-start added/deleted files correct.
PASS — split parity (diff-lines.js:69-99): reuses splitRows() itself on stripped {t,text}; splitRows (diff.js:116-169) emits fresh cells, so .no zip-back never touches hunk.lines; per-side in-order fill holds through LCS pairing incl. duplicates + 25-line runs (test green).
PASS — URL codec (diff-lines.js:116-150): file-scoped encodeURIComponent path, O-prefix old-side, ranges normalized ascending; blob #L and #f- anchors → null, mixed-side ranges/L0/bad-%/garbage → null; '#f-aL12'-style collision only highlights on exact path match, pill-anchor scroll untouched. Unknown path/line = silent no-op (DiffTable.jsx:46-49,73-78: no match → no highlight, no scroll).
PASS — drift-hash unperturbed: parser keys pinned to [t,text], anchorContextSha vector pinned (diff-lines.test.js drift tests), DiffTable never calls it (source-pin test).
PASS — shared component: Commit.jsx:9,54 and PullFiles.jsx:13,34 render the same DiffBody with a mode toggle and identical hashes. Notes (both intentional per doc): PullFiles previously had no split toggle (div-based unified only) — PR adds one; Commit split previously paired lines across hunk boundaries with no headers — now per-hunk headers (colspan 4) enabling chunk clamp.
PASS — chunk clamp (DiffTable.jsx:129-133 hunk+side identity, shift-anchor reset :123,151); keyboard (detail-0 plain=jump/shift=extend), hashchange, blur→endDrag, untracked load-scroll all mirror Blob.jsx #243 parity (:139-167,:86-103); drag frames replaceState, one push on click.
PASS — laws: no package.json change (no new deps, law 1); no backend/SDK/API touch; doc updated in same commit (12_web_ui.md §2.8, law 12 — claims re-verified: dragRange reuse, per-hunk split headers, browser-open note); seams untouched (law 8); no silent-spinner surface (law 7 n/a).
PASS — ui.css: .diff-num + .diff-row.line-hl in both themes, unlayered like the blob rule.

ADVISORY (non-blocking): chunkRange (diff-lines.js:178) is exported + tested but unused in prod — DiffTable clamps via hunk/side identity instead. Use it or drop it in a follow-up. Per-file DiffBody instances each add global hashchange/mouseup/blur listeners (DiffTable.jsx:160-162, cleaned up) — fine at normal file counts.

TESTS: diff-lines.test.js 19/19 pass; blob-lines+diff+diff-review 36/36 pass; remaining unit batches all pass (50-122 per batch, 0 fail); vite build clean (137 modules, 1.77s). Full single-glob 'node --test web/test/unit/*.test.js' times out on smoke.test.js — pre-existing (reproduces on main with no server up; CI runs with a server). Browser proof open as the PR notes (not run in this review).

MERGE RECOMMENDATION: ready to merge.

Review of PR #257 (fix/issue-244, diff line selection) — verified in scratch worktree at 5b4df56; main worktree untouched. PASS — hunk math (web/src/lib/diff-lines.js:35-60): walked deletion hunk (hunk0: ' 'a/'-'b/'+'c/'+'d/' 'e from -1,3/+1,4 → old [1,2,null,null,3], new [1,null,2,3,4]); newNo advances on non-'-', oldNo on non-'+'; 0-start added/deleted files correct. PASS — split parity (diff-lines.js:69-99): reuses splitRows() itself on stripped {t,text}; splitRows (diff.js:116-169) emits fresh cells, so .no zip-back never touches hunk.lines; per-side in-order fill holds through LCS pairing incl. duplicates + 25-line runs (test green). PASS — URL codec (diff-lines.js:116-150): file-scoped encodeURIComponent path, O-prefix old-side, ranges normalized ascending; blob #L and #f- anchors → null, mixed-side ranges/L0/bad-%/garbage → null; '#f-aL12'-style collision only highlights on exact path match, pill-anchor scroll untouched. Unknown path/line = silent no-op (DiffTable.jsx:46-49,73-78: no match → no highlight, no scroll). PASS — drift-hash unperturbed: parser keys pinned to [t,text], anchorContextSha vector pinned (diff-lines.test.js drift tests), DiffTable never calls it (source-pin test). PASS — shared component: Commit.jsx:9,54 and PullFiles.jsx:13,34 render the same DiffBody with a mode toggle and identical hashes. Notes (both intentional per doc): PullFiles previously had no split toggle (div-based unified only) — PR adds one; Commit split previously paired lines across hunk boundaries with no headers — now per-hunk headers (colspan 4) enabling chunk clamp. PASS — chunk clamp (DiffTable.jsx:129-133 hunk+side identity, shift-anchor reset :123,151); keyboard (detail-0 plain=jump/shift=extend), hashchange, blur→endDrag, untracked load-scroll all mirror Blob.jsx #243 parity (:139-167,:86-103); drag frames replaceState, one push on click. PASS — laws: no package.json change (no new deps, law 1); no backend/SDK/API touch; doc updated in same commit (12_web_ui.md §2.8, law 12 — claims re-verified: dragRange reuse, per-hunk split headers, browser-open note); seams untouched (law 8); no silent-spinner surface (law 7 n/a). PASS — ui.css: .diff-num + .diff-row.line-hl in both themes, unlayered like the blob rule. ADVISORY (non-blocking): chunkRange (diff-lines.js:178) is exported + tested but unused in prod — DiffTable clamps via hunk/side identity instead. Use it or drop it in a follow-up. Per-file DiffBody instances each add global hashchange/mouseup/blur listeners (DiffTable.jsx:160-162, cleaned up) — fine at normal file counts. TESTS: diff-lines.test.js 19/19 pass; blob-lines+diff+diff-review 36/36 pass; remaining unit batches all pass (50-122 per batch, 0 fail); vite build clean (137 modules, 1.77s). Full single-glob 'node --test web/test/unit/*.test.js' times out on smoke.test.js — pre-existing (reproduces on main with no server up; CI runs with a server). Browser proof open as the PR notes (not run in this review). MERGE RECOMMENDATION: ready to merge.

crueber closed this issue

2026-09-09 21:08:39 +00:00

crueber referenced this issue from a commit

2026-09-09 21:08:39 +00:00

Merge pull request 'Fix #244: diff line selection with #L links' (#257) from fix/issue-244 into main

crueber commented

2026-09-09 21:08:40 +00:00

Author

Owner

Fixed by PR #257 (review clean; shared DiffBody, O-prefix codec, drift contract pinned; all green), merged. Closing.

crueber added this to the v1 milestone

2026-09-10 22:20:51 +00:00