Diff lines need tap/click-to-comment affordance (selection-only misses tap, painful on mobile)

crueber commented

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

Owner

Follow-up to #546. The DiffTable comment affordance (DiffTable.jsx:284) requires an active drag/shift SELECTION (getSel()) — a tap (mobile) or plain click (desktop) selects nothing, so no affordance ever appears. On mobile, drag-selecting code text is painful; on desktop, click-to-comment is the expected idiom.

Fix: per-line tap/click affordance on diff rows (e.g. tapping a line stages a single-line comment draft / opens the composer anchored there), working with touch (tap, not drag) and mouse (click and/or hover affordance), on both the Files tab and conversation inline diffs. Must not fight text selection (tap vs drag disambiguation) and must respect the existing anchor model (side NEW/OLD, chunk clamping) and #502 gate. Rendered-verified before closing.

Follow-up to #546. The DiffTable comment affordance (DiffTable.jsx:284) requires an active drag/shift SELECTION (getSel()) — a tap (mobile) or plain click (desktop) selects nothing, so no affordance ever appears. On mobile, drag-selecting code text is painful; on desktop, click-to-comment is the expected idiom. Fix: per-line tap/click affordance on diff rows (e.g. tapping a line stages a single-line comment draft / opens the composer anchored there), working with touch (tap, not drag) and mouse (click and/or hover affordance), on both the Files tab and conversation inline diffs. Must not fight text selection (tap vs drag disambiguation) and must respect the existing anchor model (side NEW/OLD, chunk clamping) and #502 gate. Rendered-verified before closing.

crueber referenced this issue from a commit

2026-09-15 03:24:36 +00:00

Fix #555: per-line tap-to-comment affordance on diff rows (docs/go/12_web_ui.md amendment + style-guideline §8)

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

2026-09-15 03:24:38 +00:00

Fix #555: tap-to-comment #558

crueber commented

2026-09-15 03:24:45 +00:00

Author

Owner

Fix open: PR #558 (branch fix/issue-555) — per-line tap/click affordance on both surfaces, staged through the existing #546 anchor model, #502-gated, no backend change. Tests 1302/1301 (only pre-existing smoke fails); vite build green. Not merging per instructions.

crueber commented

2026-09-15 03:33:31 +00:00

Author

Owner

Review of PR #558 (fix/issue-555, 7b53734) — verified in scratch worktree /tmp/pr558 (since removed), node_modules symlinked from main, main worktree untouched (still clean).

All 7 review axes pass; no fixes needed, nothing pushed.

(1) Tap target — PASS. DiffTable.jsx lineTap() is a discrete

Review of PR #558 (fix/issue-555, 7b53734) — verified in scratch worktree /tmp/pr558 (since removed), node_modules symlinked from main, main worktree untouched (still clean). All 7 review axes pass; no fixes needed, nothing pushed. (1) Tap target — PASS. DiffTable.jsx lineTap() is a discrete <button> per code line, not a row handler: unified rows tap their own side+number via unifiedSide/unifiedNo (diff-lines.js:102-110, deletions->old else new), split left taps OLD (row.left?.no), right taps NEW (row.right?.no). Gate includes no != null, so gutter-only/empty cells get no button. (2) Anchor shape — PASS. Tap feeds onCommentSelect({path, side, start: no, end: no}) — byte-identical to the #546 single-line shape; PullFiles.jsx resolves + hashes through shared lib/review-anchor.js (findHunkForSelection + selectionToAnchor, REUSED never forked). Chunk clamping inherent (one line names its own hunk; test pins hunk 0 vs 1). Conversation DiffFile keeps stageLine->buildAnchor incl. Shift+click ranges, untouched. (3) Tap-vs-drag — PASS, reasoning sound. No onTouch* anywhere in web/src (verified by grep — zero matches), exactly one onMouseDown in DiffTable.jsx:186 (gutter drag anchor); code cells carry no handlers. A press starting on the button never starts a drag, so no movement threshold is needed; a custom touchstart would double-stage against the synthesized click. Text selection unfought. (4) Conversation DiffFile — PASS. + was hidden group-hover:inline only; now also pointer-coarse:inline (touch) + focus-visible:inline (keyboard, a11y floor). Gated by Show when={props.canComment !== false} with canComment={canComment()} plumbed at Pull.jsx:1009 call site; anon (role null or anonymous) -> false -> hidden. Matches Files-tab #502 idiom; loading state fail-closed (hidden until role loads). link class removal verified correct: .link has NO definition anywhere in web/ (grep for the rule finds nothing), so the old class was a no-op; explicit emerald F2 tokens both themes. Other link usages left alone (out of scope, not re-legitimized). (5) #502 gate + staging/invalidation — PASS. PullFiles submitThread -> pulls.threads.create + invalidate(threadsKey) byte-identical; drag/shift range flow + comment-on-selection bar untouched (pinned by test). (6) 390px — PASS by reasoning (no browser per review constraints — stated explicitly). Inline ml-2 button at end of code cell, no new columns, overflow-x-auto wrappers unchanged on both surfaces. (7) Invariants — PASS. No Go/backend change (diff outside web+docs is empty); package.json runtime deps pinned exactly by test (solid-js + @solidjs/router + marked + dompurify); docs/go/12_web_ui.md #555 amendment + style-guideline §8 idiom entry in same change (law 11/12); guideline §10 write-gate cross-reference verified (§10 names the #502 gate). Verification results: new tap-comment-555.test.js 13/13 pass; full suite minus live-server smoke 1299/1299 pass; vite build green (2.24s) with pointer-coarse:inline confirmed present in the built CSS bundle. smoke.test.js fails identically with/without this PR — it needs a live Go server (something answers on :8080 here with 403 on /setup; untouched per no-live-instance rule). No backend change, so environmental, not PR-caused. MERGE RECOMMENDATION: ready to merge. Browser proof remains open (shared-daemon loopback guard, same as stated in the PR body).

crueber closed this issue

2026-09-15 03:54:13 +00:00

crueber referenced this issue from a commit

2026-09-15 03:54:13 +00:00

Merge pull request 'Fix #555: tap-to-comment' (#558) from fix/issue-555 into main

crueber commented

2026-09-15 03:54:13 +00:00

Author

Owner

Fixed by PR #558 (review clean; rendered-verified end-to-end with headless Chromium — tap buttons, hover reveal, staged composer with anchor; selection bar also confirmed), merged. Closing.

crueber referenced this issue

2026-09-15 11:28:47 +00:00

Refine inline review composer: gutter + buttons, click-anywhere-on-line composer below the line, unlimited simultaneous composers #560

crueber referenced this issue

2026-09-15 11:38:19 +00:00

Fix #560: refine inline review composer (gutter triggers, row-click, multi-drafts) #562

crueber referenced this issue

2026-09-15 13:07:45 +00:00

Staged inline review comments vanish from the diff — only the staged count increments; they must render in-thread at their line #567

crueber referenced this issue

2026-09-15 21:53:13 +00:00

Diff gutter rework: sign-first rows, hover affordance (Fix #598) #606