crueber/walhub

Watch

Fork 0

Diff rows: added/removed lines need light green/light red full-row backgrounds on all diff surfaces, both themes #544

New issue

Closed

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

crueber commented

2026-09-14 22:41:18 +00:00

Owner

Added/removed lines in every diff view should carry a full-row light green / light red background (not just tinted text), consistently in both the light and dark themes.

What's requested

Every diff line whose type is + renders with a light green row background; every - line with a light red row background — the background spans the full table row (line-number gutter cells included), not just the code cell.
Applies on ALL diff surfaces, in BOTH themes (light + dark; the .dark class on <html> toggles themes — web/src/ui.css:1-13).

Evidence (current state, tree at `1f37977`)

The color rules partially exist but coverage is incomplete:

Content-cell-only coloring on the shared DiffBody. web/src/ui.css:295-296 already defines .diff-add { @apply bg-emerald-50 text-emerald-900 dark:bg-emerald-950/60 dark:text-emerald-200; } and the matching .diff-del red pair. lineClass() (web/src/components/DiffTable.jsx:37) applies these classes — but only to the code <td> (DiffTable.jsx:225, 249, 253). The .diff-num gutter cells (ui.css:302-304, bg-zinc-100 / dark:bg-zinc-900) stay grey on colored rows, so the row reads as half-striped. Same in split mode.
The main PR page's diff has NO coloring at all. DiffFile in web/src/pages/Pull.jsx (~line 310 onward, row render near line 386) renders each line as its own div.group.flex row with old/new number spans and the line text — it never applies lineClass()/.diff-add/.diff-del, so added/removed lines are indistinguishable from context except by the leading +/- character. This is the primary PR conversation surface.
Other consumers of the shared DiffBody (commit detail web/src/pages/Commit.jsx, PR Files tab web/src/pages/PullFiles.jsx) inherit gap 1 only.

Note: web/css/repo.css still carries legacy .diff/.diffstat rules (.diffstat .add/.del text colors); that sheet is separate from the Tailwind v4 web/src/ui.css that ships the dark-variant system — the fix should live in ui.css per the D-WEB-6 convention (no tailwind.config, class-based .dark variant).

Architecture notes

Line type is already computed per line as "+"/"-" by the parser in web/src/lib/diff.js and exposed via lineClass(t) — no wire/parser changes needed.
Cleanest shape: move the background class to the row element (the <tr class="diff-row"> in DiffTable already exists for the line-hl selection mechanic, ui.css:321-323 shows row-level styling precedent) or apply lineClass to every cell in the row; mirror the same decision in Pull.jsx's div-based rows (a wrapper class on the row div).
Mind the cascade: .diff-row.line-hl > td (amber selection highlight, ui.css:321-323) and .diff-num's background must not silently win over the row background — the file documents an unlayered-override precedent for exactly this class of conflict.
Static analysis only; no runtime verification was performed for this ticket.

Acceptance criteria

On the commit detail diff, a + line's entire row (gutter + code cells, unified AND split mode) has a light green background in light theme and a legible dark green tint in dark theme; - lines likewise red.
Same on the PR Files tab (PullFiles.jsx).
Same on the main PR page diff (Pull.jsx DiffFile) — added/removed rows are visually green/red there too.
Line-number gutters participate in the row color (no grey stripe through colored rows).
Line-selection highlight (line-hl) still visually wins over the add/del background when both apply.
Diff line-comment anchoring is untouched: anchorContextSha input bytes and pinned vectors stay byte-identical (web/src/lib/diff.js ↔ internal/review/model.go); unit tests for the hash vectors stay green.
No hardcoded color literals in JSX — colors live in ui.css utility composition (both themes covered by dark: variants).

Added/removed lines in every diff view should carry a full-row light green / light red background (not just tinted text), consistently in both the light and dark themes. ## What's requested - Every diff line whose type is `+` renders with a light green **row** background; every `-` line with a light red **row** background — the background spans the full table row (line-number gutter cells included), not just the code cell. - Applies on ALL diff surfaces, in BOTH themes (light + dark; the `.dark` class on `<html>` toggles themes — `web/src/ui.css:1-13`). ## Evidence (current state, tree at 1f37977) The color rules partially exist but coverage is incomplete: 1. **Content-cell-only coloring on the shared DiffBody.** `web/src/ui.css:295-296` already defines `.diff-add { @apply bg-emerald-50 text-emerald-900 dark:bg-emerald-950/60 dark:text-emerald-200; }` and the matching `.diff-del` red pair. `lineClass()` (`web/src/components/DiffTable.jsx:37`) applies these classes — but only to the code `<td>` (`DiffTable.jsx:225, 249, 253`). The `.diff-num` gutter cells (`ui.css:302-304`, `bg-zinc-100` / `dark:bg-zinc-900`) stay grey on colored rows, so the row reads as half-striped. Same in split mode. 2. **The main PR page's diff has NO coloring at all.** `DiffFile` in `web/src/pages/Pull.jsx` (~line 310 onward, row render near line 386) renders each line as its own `div.group.flex` row with old/new number spans and the line text — it never applies `lineClass()`/`.diff-add`/`.diff-del`, so added/removed lines are indistinguishable from context except by the leading `+`/`-` character. This is the primary PR conversation surface. 3. Other consumers of the shared `DiffBody` (commit detail `web/src/pages/Commit.jsx`, PR Files tab `web/src/pages/PullFiles.jsx`) inherit gap 1 only. Note: `web/css/repo.css` still carries legacy `.diff`/`.diffstat` rules (`.diffstat .add/.del` text colors); that sheet is separate from the Tailwind v4 `web/src/ui.css` that ships the dark-variant system — the fix should live in `ui.css` per the D-WEB-6 convention (no tailwind.config, class-based `.dark` variant). ## Architecture notes - Line type is already computed per line as `"+"`/`"-"` by the parser in `web/src/lib/diff.js` and exposed via `lineClass(t)` — no wire/parser changes needed. - Cleanest shape: move the background class to the row element (the `<tr class="diff-row">` in DiffTable already exists for the `line-hl` selection mechanic, `ui.css:321-323` shows row-level styling precedent) or apply `lineClass` to every cell in the row; mirror the same decision in Pull.jsx's div-based rows (a wrapper class on the row div). - Mind the cascade: `.diff-row.line-hl > td` (amber selection highlight, `ui.css:321-323`) and `.diff-num`'s background must not silently win over the row background — the file documents an unlayered-override precedent for exactly this class of conflict. - Static analysis only; no runtime verification was performed for this ticket. ## Acceptance criteria - [ ] On the commit detail diff, a `+` line's entire row (gutter + code cells, unified AND split mode) has a light green background in light theme and a legible dark green tint in dark theme; `-` lines likewise red. - [ ] Same on the PR Files tab (`PullFiles.jsx`). - [ ] Same on the main PR page diff (`Pull.jsx` `DiffFile`) — added/removed rows are visually green/red there too. - [ ] Line-number gutters participate in the row color (no grey stripe through colored rows). - [ ] Line-selection highlight (`line-hl`) still visually wins over the add/del background when both apply. - [ ] Diff line-comment anchoring is untouched: `anchorContextSha` input bytes and pinned vectors stay byte-identical (`web/src/lib/diff.js` ↔ `internal/review/model.go`); unit tests for the hash vectors stay green. - [ ] No hardcoded color literals in JSX — colors live in `ui.css` utility composition (both themes covered by `dark:` variants).

crueber added the

enhancement

pull requests

labels

2026-09-14 22:41:26 +00:00

crueber added this to the v1 milestone

2026-09-14 22:41:26 +00:00

crueber added the

bug

label

2026-09-14 22:42:13 +00:00

crueber referenced this issue from a commit

2026-09-14 22:53:13 +00:00

Fix #544: diff add/del full-row backgrounds incl. gutters, all surfaces, both themes

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

2026-09-14 22:53:16 +00:00

Fix #544: diff row backgrounds #548

crueber commented

2026-09-14 22:53:19 +00:00

Author

Owner

Fixed by PR #548 (#548) — branch fix/issue-544. Styling only: per-cell lineClass() (unified full-row, split per-side), compound gutter tiebreaks in ui.css, DiffFile reuses the shared helper, line-hl still wins, diff.js untouched. Tests 1250 total / 1249 pass (1 pre-existing live-server smoke failure, verified head-to-head); vite+esbuild green. Browser proof open (no browser available — noted in the decision entry).

Fixed by PR #548 (https://git.packden.us/crueber/walhub/pulls/548) — branch fix/issue-544. Styling only: per-cell lineClass() (unified full-row, split per-side), compound gutter tiebreaks in ui.css, DiffFile reuses the shared helper, line-hl still wins, diff.js untouched. Tests 1250 total / 1249 pass (1 pre-existing live-server smoke failure, verified head-to-head); vite+esbuild green. Browser proof open (no browser available — noted in the decision entry).

crueber commented

2026-09-14 23:09:25 +00:00

Author

Owner

REVIEW PR #548 (fix/issue-544, ab61128) — all 9 checks pass, no fixes needed.

(1) Unified rows: DiffTable.jsx:229 gutter is now diff-num ${lineClass(l.t)} + code cell keeps lineClass — full-row background incl. gutter. PASS.
(2) Split rows: DiffTable.jsx:253/257 gutters follow their own side (row.left ? lineClass(row.left.t) : ''); no row-level single-color class (would miscolor pairs) — paired change rows render red-left/green-right. PASS.
(3) Pull.jsx DiffFile: Pull.jsx:19 imports shared lineClass from DiffTable (no forked mapping); Pull.jsx:382 applies it to the row div — spans carry no bg so gutters inherit the row background. Commit.jsx/PullFiles.jsx inherit via DiffBody. PASS.
(4) Specificity: compound .diff-num.diff-add/.diff-del (0,2,0, ui.css:314-315, inside @layer components) beats plain .diff-num (0,1,0, ui.css:302) at any order — verified in compiled bundle. .diff-row.line-hl rules (ui.css:331-333) sit outside @layer (ends ui.css:316), so unlayered selection still wins. PASS.
(5) Dark variants: both compound rules carry the same dark: tokens (emerald-950/60 + emerald-200 / red-950/60 + red-200) — present in compiled CSS as :where(.dark, .dark *) rules. PASS.
(6) Legacy web/css/repo.css untouched (not in diff). (7) web/src/lib/diff.js untouched; anchorContextSha intact, diff-review hash vectors green. PASS.
(8) Guideline: docs/style-guideline.md §8 gains the .diff-add/.diff-del bullet citing tokens, per-cell decision, gutter tiebreak, unlayered precedent, F2 — composes canonical idioms, no new pattern; law-12 decision logged in 12_web_ui.md same change. PASS.
(9) No package-manifest changes — no new deps. Docs counts accurate.

VERIFY (scratch worktree /tmp/pr548, removed afterward; main worktree left clean): node --test web/test/unit/*.test.js → 1249 pass / 1 fail, the single failure being the pre-existing live-server smoke subtest (smoke.test.js /setup 403, needs a live Go server; unrelated to this styling-only change). diff-row-544 + diff-lines + diff-review: 34/34 green. vite build green; compound rules + dark variants + unlayered line-hl confirmed in dist bundle. No browser (styling-only change; compiled-CSS reasoning per instructions — noted explicitly).

Non-blocking nit: DiffFile gutter spans keep text-zinc-400 on colored rows while DiffTable gutters get emerald/red text via the compound rules — background requirement is met everywhere; text-color parity could follow later.

MERGE RECOMMENDATION: ready to merge.

REVIEW PR #548 (fix/issue-544, ab61128) — all 9 checks pass, no fixes needed. (1) Unified rows: DiffTable.jsx:229 gutter is now `diff-num ${lineClass(l.t)}` + code cell keeps lineClass — full-row background incl. gutter. PASS. (2) Split rows: DiffTable.jsx:253/257 gutters follow their own side (`row.left ? lineClass(row.left.t) : ''`); no row-level single-color class (would miscolor pairs) — paired change rows render red-left/green-right. PASS. (3) Pull.jsx DiffFile: Pull.jsx:19 imports shared lineClass from DiffTable (no forked mapping); Pull.jsx:382 applies it to the row div — spans carry no bg so gutters inherit the row background. Commit.jsx/PullFiles.jsx inherit via DiffBody. PASS. (4) Specificity: compound .diff-num.diff-add/.diff-del (0,2,0, ui.css:314-315, inside @layer components) beats plain .diff-num (0,1,0, ui.css:302) at any order — verified in compiled bundle. .diff-row.line-hl rules (ui.css:331-333) sit outside @layer (ends ui.css:316), so unlayered selection still wins. PASS. (5) Dark variants: both compound rules carry the same dark: tokens (emerald-950/60 + emerald-200 / red-950/60 + red-200) — present in compiled CSS as :where(.dark, .dark *) rules. PASS. (6) Legacy web/css/repo.css untouched (not in diff). (7) web/src/lib/diff.js untouched; anchorContextSha intact, diff-review hash vectors green. PASS. (8) Guideline: docs/style-guideline.md §8 gains the .diff-add/.diff-del bullet citing tokens, per-cell decision, gutter tiebreak, unlayered precedent, F2 — composes canonical idioms, no new pattern; law-12 decision logged in 12_web_ui.md same change. PASS. (9) No package-manifest changes — no new deps. Docs counts accurate. VERIFY (scratch worktree /tmp/pr548, removed afterward; main worktree left clean): node --test web/test/unit/*.test.js → 1249 pass / 1 fail, the single failure being the pre-existing live-server smoke subtest (smoke.test.js /setup 403, needs a live Go server; unrelated to this styling-only change). diff-row-544 + diff-lines + diff-review: 34/34 green. vite build green; compound rules + dark variants + unlayered line-hl confirmed in dist bundle. No browser (styling-only change; compiled-CSS reasoning per instructions — noted explicitly). Non-blocking nit: DiffFile gutter spans keep text-zinc-400 on colored rows while DiffTable gutters get emerald/red text via the compound rules — background requirement is met everywhere; text-color parity could follow later. MERGE RECOMMENDATION: ready to merge.

crueber closed this issue

2026-09-14 23:14:44 +00:00

crueber commented

2026-09-14 23:14:44 +00:00

Author

Owner

Fixed by PR #548 (review clean; rendered-verified with headless Chromium screenshots — full-row green incl. gutter, both states; my first screenshot pass tested a stale scratch binary and has been redone correctly), merged. Closing.

crueber referenced this issue from a commit

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

Merge pull request 'Fix #544: diff row backgrounds' (#548) from fix/issue-544 into main