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

Merged

crueber merged 2 commits from fix/issue-598 into main

2026-09-15 21:57:33 +00:00

crueber commented

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

Owner

Left gutter '+' removed entirely (DiffFile w-6 always-visible column; DiffTable lineTap hover-plus at the code-cell end).
+/-/space sign column FIRST on every row, ahead of line numbers, themed with the row (add green / del red via lineClass, context muted). Unified DiffTable rows gain a visible sign (were color-only). Split rows pair one sign per side (red-left/green-right kept); hunk colspans grow (2->3 / 4->6).
Row hover-highlight (existing ui.css .diff-row.line-hl amber + emerald tokens, both themes, unlayered) is the interactivity affordance; DiffFile row divs gain diff-row + cursor-pointer.
ml-14 composer/thread indent re-derived to ml-8 (removed w-6 = 1.5rem = 6 units; slot follows by 6 — relative alignment preserved) on composer + StagedCard + ThreadCard.

SCOPE DECISION: DiffTable single-line staging relocates into the new leftmost sign cell (button when gated, plain sign otherwise — same #546 single-line shape, same #502 gate). A DiffFile-style row-click was rejected for DiffTable: a click also fires after text-selection drags, regressing the #555 code-text-stays-handler-free decision. #244 drag/shift gutter-number selection untouched; sign column select-none.

PRESERVED: anchor construction + hash inputs byte-identical (diff.js untouched, diff-review vectors green); DiffFile onRowClick isolation; #502 anon gate on all entry points; range staging paths; Escape dismissal now refocuses the staging row (tabindex=-1, never in tab order).

Rules: law 1 (no new deps); Tailwind-only + reused line-hl tokens, no new colors/CSS class; FIXED (Forgejo #598) amendment in docs/go/12_web_ui.md + style-guideline row-anatomy update (law 12).

Verification: new web/test/unit/diff-gutter-598.test.js (25 tests) + #598-scoped updates to six stale pins; full-minus-smoke 1534/1534 green; vite build + esbuild SDK green (dist/.keep restored); go vet clean. Browser proof open per shared-daemon guard (PUT /json/new returns empty — no drivable target; hover rules verified live in the shipped bundle CSS; headless DOM row-structure + 390px pins in-test).

Implements Forgejo issue #598 — diff gutter rework on both renderers, all three surfaces (conversation DiffFile in web/src/pages/Pull.jsx, Files-tab/commit DiffTable in web/src/components/DiffTable.jsx). 1. Left gutter '+' removed entirely (DiffFile w-6 always-visible column; DiffTable lineTap hover-plus at the code-cell end). 2. +/-/space sign column FIRST on every row, ahead of line numbers, themed with the row (add green / del red via lineClass, context muted). Unified DiffTable rows gain a visible sign (were color-only). Split rows pair one sign per side (red-left/green-right kept); hunk colspans grow (2->3 / 4->6). 3. Row hover-highlight (existing ui.css .diff-row.line-hl amber + emerald tokens, both themes, unlayered) is the interactivity affordance; DiffFile row divs gain diff-row + cursor-pointer. 4. ml-14 composer/thread indent re-derived to ml-8 (removed w-6 = 1.5rem = 6 units; slot follows by 6 — relative alignment preserved) on composer + StagedCard + ThreadCard. SCOPE DECISION: DiffTable single-line staging relocates into the new leftmost sign cell (button when gated, plain sign otherwise — same #546 single-line shape, same #502 gate). A DiffFile-style row-click was rejected for DiffTable: a <tr> click also fires after text-selection drags, regressing the #555 code-text-stays-handler-free decision. #244 drag/shift gutter-number selection untouched; sign column select-none. PRESERVED: anchor construction + hash inputs byte-identical (diff.js untouched, diff-review vectors green); DiffFile onRowClick isolation; #502 anon gate on all entry points; range staging paths; Escape dismissal now refocuses the staging row (tabindex=-1, never in tab order). Rules: law 1 (no new deps); Tailwind-only + reused line-hl tokens, no new colors/CSS class; FIXED (Forgejo #598) amendment in docs/go/12_web_ui.md + style-guideline row-anatomy update (law 12). Verification: new web/test/unit/diff-gutter-598.test.js (25 tests) + #598-scoped updates to six stale pins; full-minus-smoke 1534/1534 green; vite build + esbuild SDK green (dist/.keep restored); go vet clean. Browser proof open per shared-daemon guard (PUT /json/new returns empty — no drivable target; hover rules verified live in the shipped bundle CSS; headless DOM row-structure + 390px pins in-test).

crueber added 1 commit

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

Diff gutter rework: sign-first rows, hover affordance, no per-line plus (Fix #598 ) 617aaaec82

Implements docs/go/12_web_ui.md FIXED (Forgejo #598) + style-guideline
row-anatomy update in the same change (law 12).

Both renderers lose their per-line '+' triggers (DiffFile w-6 gutter
column; DiffTable lineTap hover-plus). Every row leads with a +/-/space
sign column ahead of the line numbers, themed with the row; unified
DiffTable rows gain a visible sign. Row hover (reused line-hl amber +
emerald tokens, both themes) is the interactivity affordance. Pull.jsx
inline-card slot re-derived ml-14 -> ml-8, following the removed w-6
gutter. Scope decision: DiffTable single-line staging relocates into
the sign cell (row-click rejected - a <tr> click also fires after
text-selection drags); #244 selection, anchors/hashes, #502 gates,
range paths preserved.

crueber referenced this pull request from a commit

2026-09-15 21:56:29 +00:00

Gate DiffFile row cursor-pointer on canComment/commentLocked (review #606: anon/locked rows promise no interactivity)

crueber added 1 commit

2026-09-15 21:56:29 +00:00

Gate DiffFile row cursor-pointer on canComment/commentLocked (review #606 : anon/locked rows promise no interactivity) 4ce914f11d

crueber commented

2026-09-15 21:56:57 +00:00

Author

Owner

Independent review of #606 (617aaae + review fixup 4ce914f) against #598 — verdict: APPROVE.

All nine acceptance bullets hold. Per-point findings:

NEW ui.css .diff-row:hover rules — JUSTIFIED shared pattern, keep as-is. Reuses exactly the selection tokens (#fef3c7 / #10b981 / rgba(245,158,11,0.16), pinned by the hover-token test — no new color), deliberately unlayered for the same cascade reason as the .line-hl/blob precedents (a Tailwind hover: utility is layered and would lose to the diff-add/del cell backgrounds), covers both variants (> td for the DiffTable tables, div.diff-row for the DiffFile div rows), and the guideline § row-anatomy section is rewritten in the same change. Group-hover composition could not express this — correct call.
DiffTable sign-cell-as-tap-target — correct. #502 anon gets plain sign text (Show gate onCommentSelect && canComment !== false && no != null, fallback={signChar(t)}), never a button. Drag-selection (#244) unaffected: sign cell is select-none, carries no mousedown/mouseover (exactly one onMouseDown in the file — the gutter-drag anchor), no touch handlers, no <tr onClick>. Split pairing correct (left sign stages OLD, right sign stages NEW; no row-level single color). Colspans audited — all three in the file are consistent (binary split ? 6 : 3, unified 3, split 6); no broken layout.
DiffFile — gutter-plus deletion complete (no w-6 markup left, only comment mentions; the one remaining stopPropagation is the Escape-keydown handler, not a trigger). triggerRefs correctly re-wired from the deleted gutter button to the row div; Escape refocus lands on the staging row via tabindex="-1" (verified -1, never tab order; focus() never re-stages). ml-14→ml-8 math checks: removed w-6 = 1.5rem = 6 spacing units, slot follows 14−6=8 on composer + StagedCard + ThreadCard alike (relative alignment preserved). ONE DEFECT FOUND + FIXED (see below): cursor-pointer was unconditional, so anon/locked rows promised interactivity they don't have.
Hash vectors green, anchors byte-identical — lib/diff.js and internal/review/model.go untouched by the diff; diff-review.test.js vectors pass unmodified.
Six stale-pin updates are justified redesign updates, not weakenings. Anchor shapes, #502 gate, no-touch/no-handler contract, and range flow are all re-pinned at their relocated homes (555's surface sections become relocation pins with shapes kept; 560's trigger section becomes sign-first/row-click pins; 567/574 slot pins move ml-14→ml-8 with the shared-slot assertion intact; 574's keyboard-ring pin relocates to the sign button with identical classes; 544 gains the affordance classes; diff-lines colspans become 3/6). Verified: DiffFile row-click predates this change (#560); DiffTable keeps code text handler-free (no <tr onClick>, mousedown count stays 1).
Law 12 satisfied (FIXED amendment in docs/go/12_web_ui.md + guideline rewrite, same change); law 1 satisfied (diff.js untouched, runtime deps still exactly the four pinned); new 25-test file fails pre-fix by inspection (main still has lineTap, the w-6 gutter, zero :hover rules) and passes post-fix; full unit suite minus the environmental smoke test is 1536/1536 green, go vet clean (make fmt errors on its empty file list — pre-existing, no Go files touched).

Review fixup committed + pushed on this branch (4ce914f): DiffFile row cursor-pointer is now gated on canComment !== false && !commentLocked, with the three pins that asserted the unconditional class updated to assert the gated form (diff-gutter-598, inline-composer-560, diff-row-544). Hover tint stays ungated (global CSS; weak affordance, and DiffTable rows remain selectable for share-links even when ungated).

Browser proof remains open per the shared-daemon guard, same as declared — headless DOM row-structure + 390px pins in-test cover the anatomy.

Independent review of #606 (617aaae + review fixup 4ce914f) against #598 — verdict: APPROVE. All nine acceptance bullets hold. Per-point findings: 1. NEW `ui.css` `.diff-row:hover` rules — JUSTIFIED shared pattern, keep as-is. Reuses exactly the selection tokens (`#fef3c7` / `#10b981` / `rgba(245,158,11,0.16)`, pinned by the hover-token test — no new color), deliberately unlayered for the same cascade reason as the `.line-hl`/blob precedents (a Tailwind `hover:` utility is layered and would lose to the `diff-add/del` cell backgrounds), covers both variants (`> td` for the DiffTable tables, `div.diff-row` for the DiffFile div rows), and the guideline § row-anatomy section is rewritten in the same change. Group-hover composition could not express this — correct call. 2. DiffTable sign-cell-as-tap-target — correct. `#502` anon gets plain sign text (`Show` gate `onCommentSelect && canComment !== false && no != null`, `fallback={signChar(t)}`), never a button. Drag-selection (#244) unaffected: sign cell is `select-none`, carries no `mousedown`/`mouseover` (exactly one `onMouseDown` in the file — the gutter-drag anchor), no touch handlers, no `<tr onClick>`. Split pairing correct (left sign stages OLD, right sign stages NEW; no row-level single color). Colspans audited — all three in the file are consistent (binary `split ? 6 : 3`, unified `3`, split `6`); no broken layout. 3. DiffFile — gutter-plus deletion complete (no `w-6` markup left, only comment mentions; the one remaining `stopPropagation` is the Escape-keydown handler, not a trigger). `triggerRefs` correctly re-wired from the deleted gutter button to the row div; Escape refocus lands on the staging row via `tabindex="-1"` (verified `-1`, never tab order; `focus()` never re-stages). ml-14→ml-8 math checks: removed w-6 = 1.5rem = 6 spacing units, slot follows 14−6=8 on composer + StagedCard + ThreadCard alike (relative alignment preserved). ONE DEFECT FOUND + FIXED (see below): `cursor-pointer` was unconditional, so anon/locked rows promised interactivity they don't have. 4. Hash vectors green, anchors byte-identical — `lib/diff.js` and `internal/review/model.go` untouched by the diff; `diff-review.test.js` vectors pass unmodified. 5. Six stale-pin updates are justified redesign updates, not weakenings. Anchor shapes, `#502` gate, no-touch/no-handler contract, and range flow are all re-pinned at their relocated homes (555's surface sections become relocation pins with shapes kept; 560's trigger section becomes sign-first/row-click pins; 567/574 slot pins move ml-14→ml-8 with the shared-slot assertion intact; 574's keyboard-ring pin relocates to the sign button with identical classes; 544 gains the affordance classes; diff-lines colspans become 3/6). Verified: DiffFile row-click predates this change (#560); DiffTable keeps code text handler-free (no `<tr onClick>`, mousedown count stays 1). 6. Law 12 satisfied (FIXED amendment in `docs/go/12_web_ui.md` + guideline rewrite, same change); law 1 satisfied (`diff.js` untouched, runtime deps still exactly the four pinned); new 25-test file fails pre-fix by inspection (main still has `lineTap`, the `w-6` gutter, zero `:hover` rules) and passes post-fix; full unit suite minus the environmental smoke test is 1536/1536 green, `go vet` clean (`make fmt` errors on its empty file list — pre-existing, no Go files touched). Review fixup committed + pushed on this branch (4ce914f): DiffFile row `cursor-pointer` is now gated on `canComment !== false && !commentLocked`, with the three pins that asserted the unconditional class updated to assert the gated form (`diff-gutter-598`, `inline-composer-560`, `diff-row-544`). Hover tint stays ungated (global CSS; weak affordance, and DiffTable rows remain selectable for share-links even when ungated). Browser proof remains open per the shared-daemon guard, same as declared — headless DOM row-structure + 390px pins in-test cover the anatomy.

crueber merged commit fc940bad70 into main

2026-09-15 21:57:33 +00:00

crueber referenced this pull request from a commit

2026-09-15 21:57:34 +00:00

Merge pull request 'Diff gutter rework: sign-first rows, hover affordance (Fix #598)' (#606) from fix/issue-598 into main

crueber referenced this pull request

2026-09-15 21:57:35 +00:00

Diff gutter: drop the hover-plus, lead with the +/- sign column, make row hover the affordance #598