crueber/walhub

Watch

Fork 0

Select line(s) in a PR diff to create inline comment threads, with a jump-to-comments index at the conversation top #546

New issue

Closed

opened 2026-09-14 22:45:00 +00:00 by crueber · 3 comments

crueber commented

2026-09-14 22:45:00 +00:00

Owner

What's requested

On a PR, let a reviewer select line(s) in the diff and create an inline comment thread anchored exactly there, plus a jump-to-comments index at the top of the conversation page listing every anchored thread (path:line, resolved state) that scrolls to the thread when clicked.

Current state (static read, no local repro)

Much of the machinery exists; three gaps remain:

Files tab: selection without commenting. web/src/components/DiffTable.jsx implements full drag/shift line selection over the diff (issue #244) but exposes no comment affordance — the only comment entry point is the conversation page's inline diff.
Conversation page: single-line, single-shot commenting. web/src/pages/Pull.jsx stageLine (~line 311) builds a count: 1 anchor per line and collects the body via window.prompt, staged into the finish-review modal. There is no multi-line selection → range anchor path, and the composer should be the real CommentComposer (components/CommentComposer.jsx), not a browser prompt.
No jump-to-comments index. The conversation page shows a summary count (threads_unresolved, ~line 94) and renders ThreadCards inline via placement()/threadsAt() (~lines 328–392), but there is no index of anchored threads at the top of the conversation to jump to them; on a large diff, inline threads are only findable by scrolling.

Architecture notes

The server anchor model already supports ranges: internal/review/model.go (ThreadAnchor with old_start/old_lines/new_start/new_lines, validation at ~line 282 requires new_start/new_lines >= 1 for NEW-side). No backend change needed for range anchors — only the client ever builds count: 1 today.
lib/diff.js anchorContextSha is the ONLY §4 hash implementation and has pinned vectors; any range selection must feed it inputs byte-identically (drift-hash twin with DriftHash in internal/review/model.go). If range anchors change hash inputs, that is a pinned-contract change and needs an explicit decision.
lib/diff-lines.js (issue #244) already provides headless per-line numbering and the file-scoped hash codec — reuse it for selection → anchor conversion rather than re-deriving numbering in the component.
Thread placement/drift handling already works: anchors that no longer locate render collapsed at file end, never relocated (threadFreshness, placement() in Pull.jsx). The index must reflect the same placement truth (mark drifted/outdated entries rather than linking to a line that no longer exists).
Existing convention: single-line anchors carry side: "NEW" for context/additions, side: "OLD" for deletions (Pull.jsx stageLine). Range selection in DiffTable is already clamped to one chunk + one side (DiffTable drag logic), which maps cleanly onto the anchor model.

Acceptance criteria

DiffTable (Files tab) exposes a "comment on selection" affordance when lines are selected, opening the shared CommentComposer (no window.prompt anywhere on this path)
Multi-line selection creates a range anchor (new_lines/old_lines > 1 as appropriate); single-line keeps the current shape
Commenting from the Files tab lands the thread in the same conversation/threads list (no new wire surface; reuses pulls.threads.* SDK client)
Conversation page shows a jump-to-comments index at the top: one entry per anchored thread (path:line or range, resolved/outdated state), each scrolling to and highlighting the inline thread card
Index entries for drifted/outdated anchors are marked and do not attempt to scroll to a nonexistent line
anchorContextSha inputs and pinned vector tests stay byte-identical, or the change is flagged as a pinned-contract decision for review
Unresolved-first ordering and existing resolve/unresolve behavior unchanged

## What's requested On a PR, let a reviewer select line(s) in the diff and create an inline comment thread anchored exactly there, plus a jump-to-comments index at the top of the conversation page listing every anchored thread (path:line, resolved state) that scrolls to the thread when clicked. ## Current state (static read, no local repro) Much of the machinery exists; three gaps remain: 1. **Files tab: selection without commenting.** `web/src/components/DiffTable.jsx` implements full drag/shift line selection over the diff (issue #244) but exposes no comment affordance — the only comment entry point is the conversation page's inline diff. 2. **Conversation page: single-line, single-shot commenting.** `web/src/pages/Pull.jsx` `stageLine` (~line 311) builds a `count: 1` anchor per line and collects the body via `window.prompt`, staged into the finish-review modal. There is no multi-line selection → range anchor path, and the composer should be the real `CommentComposer` (components/CommentComposer.jsx), not a browser prompt. 3. **No jump-to-comments index.** The conversation page shows a summary count (`threads_unresolved`, ~line 94) and renders `ThreadCard`s inline via `placement()`/`threadsAt()` (~lines 328–392), but there is no index of anchored threads at the top of the conversation to jump to them; on a large diff, inline threads are only findable by scrolling. ## Architecture notes - The server anchor model already supports ranges: `internal/review/model.go` (`ThreadAnchor` with `old_start/old_lines/new_start/new_lines`, validation at ~line 282 requires `new_start/new_lines >= 1` for NEW-side). No backend change needed for range anchors — only the client ever builds `count: 1` today. - `lib/diff.js` `anchorContextSha` is the ONLY §4 hash implementation and has pinned vectors; any range selection must feed it inputs byte-identically (drift-hash twin with `DriftHash` in `internal/review/model.go`). If range anchors change hash inputs, that is a pinned-contract change and needs an explicit decision. - `lib/diff-lines.js` (issue #244) already provides headless per-line numbering and the file-scoped hash codec — reuse it for selection → anchor conversion rather than re-deriving numbering in the component. - Thread placement/drift handling already works: anchors that no longer locate render collapsed at file end, never relocated (`threadFreshness`, `placement()` in Pull.jsx). The index must reflect the same placement truth (mark drifted/outdated entries rather than linking to a line that no longer exists). - Existing convention: single-line anchors carry `side: "NEW"` for context/additions, `side: "OLD"` for deletions (Pull.jsx `stageLine`). Range selection in DiffTable is already clamped to one chunk + one side (DiffTable drag logic), which maps cleanly onto the anchor model. ## Acceptance criteria - [ ] DiffTable (Files tab) exposes a "comment on selection" affordance when lines are selected, opening the shared `CommentComposer` (no `window.prompt` anywhere on this path) - [ ] Multi-line selection creates a range anchor (`new_lines`/`old_lines` > 1 as appropriate); single-line keeps the current shape - [ ] Commenting from the Files tab lands the thread in the same conversation/threads list (no new wire surface; reuses `pulls.threads.*` SDK client) - [ ] Conversation page shows a jump-to-comments index at the top: one entry per anchored thread (path:line or range, resolved/outdated state), each scrolling to and highlighting the inline thread card - [ ] Index entries for drifted/outdated anchors are marked and do not attempt to scroll to a nonexistent line - [ ] `anchorContextSha` inputs and pinned vector tests stay byte-identical, or the change is flagged as a pinned-contract decision for review - [ ] Unresolved-first ordering and existing resolve/unresolve behavior unchanged

crueber added the

enhancement

pull requests

labels

2026-09-14 22:45:37 +00:00

crueber added this to the v1 milestone

2026-09-14 22:45:37 +00:00

crueber added the

issues

label

2026-09-14 22:46:00 +00:00

crueber referenced this issue from a commit

2026-09-15 00:26:06 +00:00

Fix #546: diff line selection creates inline threads + jump-to-comments index

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

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

Fix #546: diff line comments #551

crueber commented

2026-09-15 00:26:12 +00:00

Author

Owner

Fixed by PR #551 (#551, branch fix/issue-546): diff line selection creates inline threads + jump-to-comments index. Client-only, no backend change.

Fixed by PR #551 (https://git.packden.us/crueber/walhub/pulls/551, branch fix/issue-546): diff line selection creates inline threads + jump-to-comments index. Client-only, no backend change.

crueber commented

2026-09-15 00:53:14 +00:00

Author

Owner

Review of PR #551 (fix/issue-546) — verified in scratch worktree /tmp/pr551

Verdict: ready to merge. All 7 acceptance criteria hold; no fixes pushed (nothing broken found).

(1) Anchor conversion — correct. buildAnchor (review-anchor.js:53) preserves the zero-pair convention (NEW zeroes old_, OLD zeroes new_, lines===1 single-line — identical to old stageLine shape) and clamps via matchAnnotated (null on empty → "pick again" path, no throw). Byte-identity verified by reading both numberings: numberedLines (Pull.jsx:292) and annotateHunkLines (diff-lines.js:35) use identical counter logic over the same hunk.lines order, so annotated index == old row.idx; single-line spans hash {start: idx, count: 1} exactly as before. Pinned Go-twin vector test untouched and passing; freshnessOf reduces to the old check for count:1. No drift-hash change — correctly NOT flagged as a pinned-contract change.

(2) DiffTable affordance — correct. Opt-in (onCommentSelect + canComment, DiffTable.jsx:284); Commit.jsx passes neither, so the commit page is unaffected. Consumer-owned CommentComposer, zero window.prompt, zero direct anchorContextSha( calls in the component (comment-mentions only) — both pinned by tests.

(3) PullFiles staging — correct. pulls.threads.create(num, {anchor, body}, {noPopupAuth:true}) matches the SDK signature (reviews.js:60). Invalidates threads:{full}:{num} — byte-identical key shape to the conversation's threadsKey (Pull.jsx:703), so the new thread appears on navigation. #502 gate role() !== null && !anon() matches Pull.jsx:746 exactly. Non-blocking note: the review summary (threads_unresolved) is TTL-staled after a Files-tab create until refetch — standard app-wide staleness, not a defect.

(4) Conversation composer — correct. window.prompt("Comment on…") gone (remaining prompt at Pull.jsx:141 is the pre-existing dismissal-reason path, out of scope). Draft CommentComposer → onStage → pending → FinishReview (Pull.jsx:814/965/997) with anchorLabel range rendering. Shift+click gated on same file+side+hunkIdx (the DiffTable clamp convention).

(5) ThreadIndex — correct. path:line / path:start-end via shared anchorLabel, resolved/outdated suffixes, unresolved-first via sortThreadsForIndex (same order as inline cards). Jump scrolls to thread-<tid> with emerald flash + expand (flashed() forces open()); drifted entries target their collapsed file-end card (same id), file-gone entries render · gone with no target. No dead-line links.

(6) Placement truth — single source. Both cards (threadFreshness → delegates to freshnessOf) and index use the same helper; anchors never relocate.

(7) Guideline — compliant. Only canonical idioms (.card/.card-header/.pill/.btn/.muted/.err-line, shared CommentComposer); ui.css untouched (no new CSS, correctly no guideline extension — law 11 carried by the two doc amendments). outline-emerald-500 flash is a Tailwind token (cf. tab-badge precedent), not a literal; flex-wrap index/affordance rows degrade gracefully at ~390px by construction.

(8) No backend, no deps. File list is docs + web/ only (the one grep go hit is docs/go/12_web_ui.md); no .go, no package.json/go.mod change. Law-12 decisions appended in both docs in the same change.

Tests: node --test web/test/unit/*.test.js → 1277 pass / 1 fail; the 1 failure is the live-server smoke subtest ("built SPA shell is served at / and /setup", /setup 403 from the standing instance) — unrelated to this client-only change (a main-worktree head-to-head run was attempted but the smoke test hangs without a live server, itself confirming environment-dependence). New review-anchor-546.test.js: 18/18 pass. vite build green (only the standard chunk-size warning).

No browser drive per task instructions (node tests + reasoning; layout reasoned from shared card/pill/btn classes at desktop + ~390px).

## Review of PR #551 (fix/issue-546) — verified in scratch worktree /tmp/pr551 **Verdict: ready to merge.** All 7 acceptance criteria hold; no fixes pushed (nothing broken found). **(1) Anchor conversion — correct.** `buildAnchor` (review-anchor.js:53) preserves the zero-pair convention (NEW zeroes old_*, OLD zeroes new_*, lines===1 single-line — identical to old `stageLine` shape) and clamps via `matchAnnotated` (null on empty → "pick again" path, no throw). Byte-identity verified by reading both numberings: `numberedLines` (Pull.jsx:292) and `annotateHunkLines` (diff-lines.js:35) use identical counter logic over the same `hunk.lines` order, so annotated index == old `row.idx`; single-line spans hash `{start: idx, count: 1}` exactly as before. Pinned Go-twin vector test untouched and passing; `freshnessOf` reduces to the old check for count:1. No drift-hash change — correctly NOT flagged as a pinned-contract change. **(2) DiffTable affordance — correct.** Opt-in (`onCommentSelect` + `canComment`, DiffTable.jsx:284); Commit.jsx passes neither, so the commit page is unaffected. Consumer-owned `CommentComposer`, zero `window.prompt`, zero direct `anchorContextSha(` calls in the component (comment-mentions only) — both pinned by tests. **(3) PullFiles staging — correct.** `pulls.threads.create(num, {anchor, body}, {noPopupAuth:true})` matches the SDK signature (reviews.js:60). Invalidates `threads:{full}:{num}` — byte-identical key shape to the conversation's `threadsKey` (Pull.jsx:703), so the new thread appears on navigation. #502 gate `role() !== null && !anon()` matches Pull.jsx:746 exactly. Non-blocking note: the review *summary* (`threads_unresolved`) is TTL-staled after a Files-tab create until refetch — standard app-wide staleness, not a defect. **(4) Conversation composer — correct.** `window.prompt("Comment on…")` gone (remaining prompt at Pull.jsx:141 is the pre-existing dismissal-reason path, out of scope). Draft `CommentComposer` → `onStage` → `pending` → FinishReview (Pull.jsx:814/965/997) with `anchorLabel` range rendering. Shift+click gated on same file+side+hunkIdx (the DiffTable clamp convention). **(5) ThreadIndex — correct.** `path:line` / `path:start-end` via shared `anchorLabel`, resolved/outdated suffixes, unresolved-first via `sortThreadsForIndex` (same order as inline cards). Jump scrolls to `thread-<tid>` with emerald flash + expand (`flashed()` forces `open()`); drifted entries target their collapsed file-end card (same id), file-gone entries render `· gone` with no target. No dead-line links. **(6) Placement truth — single source.** Both cards (`threadFreshness` → delegates to `freshnessOf`) and index use the same helper; anchors never relocate. **(7) Guideline — compliant.** Only canonical idioms (`.card/.card-header/.pill/.btn/.muted/.err-line`, shared `CommentComposer`); `ui.css` untouched (no new CSS, correctly no guideline extension — law 11 carried by the two doc amendments). `outline-emerald-500` flash is a Tailwind token (cf. `tab-badge` precedent), not a literal; `flex-wrap` index/affordance rows degrade gracefully at ~390px by construction. **(8) No backend, no deps.** File list is docs + `web/` only (the one `grep go` hit is `docs/go/12_web_ui.md`); no `.go`, no `package.json`/`go.mod` change. Law-12 decisions appended in both docs in the same change. **Tests:** `node --test web/test/unit/*.test.js` → 1277 pass / 1 fail; the 1 failure is the live-server smoke subtest ("built SPA shell is served at / and /setup", /setup 403 from the standing instance) — unrelated to this client-only change (a main-worktree head-to-head run was attempted but the smoke test hangs without a live server, itself confirming environment-dependence). New `review-anchor-546.test.js`: 18/18 pass. `vite build` green (only the standard chunk-size warning). No browser drive per task instructions (node tests + reasoning; layout reasoned from shared card/pill/btn classes at desktop + ~390px).

crueber closed this issue

2026-09-15 00:54:25 +00:00

crueber referenced this issue from a commit

2026-09-15 00:54:25 +00:00

Merge pull request 'Fix #546: diff line comments' (#551) from fix/issue-546 into main

crueber commented

2026-09-15 00:54:26 +00:00

Author

Owner

Fixed by PR #551 (review clean — all 7 criteria pass, hash parity + placement truth verified), merged. Closing.

crueber referenced this issue

2026-09-15 02:12:56 +00:00