crueber/walhub

Watch

Fork 0

Restyle the PR review UI (Reviews section + Finish review form) to match system styling #545

New issue

Closed

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

crueber commented

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

Owner

Restyle the PR review UI (Reviews section + Finish review form) to match system styling

What's requested

Bring the two remaining hand-rolled blocks of the PR page — the Reviews section and the Finish review form — onto the app's established styling idioms: the canonical form structure from #479, the primary-button idiom, chip verdicts, and the sectioned-card pattern the rest of the PR page already uses. Styling/markup only; no behavior change.

Canonical references in the current tree

Form structure: #479 pattern (ReleaseNew.jsx / IssueNew.jsx) — label with grid gap-1, a text-sm font-medium span label, the .input class on every control, help text scoped to its field via id + aria-describedby.
Primary button idiom: web/src/ui.css line 73 — .btn.primary (emerald), i.e. class="btn primary".
Chip verdicts: the .chip family in web/src/ui.css lines 108–119 (.chip, .chip-open, .chip-closed, .chip-merged, .chip-draft) — the status-chip language used by issue/pull rows and test pr-structure-531.test.js.
Sectioned cards: the PR page's other sections (Mergeability, Reviewers, Checks, Merge) are sibling .card panels each with a single .card-header (Forgejo #521 idiom).

Divergences found (evidence, all in `web/src/pages/Pull.jsx`)

Undefined card-list class — ReviewsList wraps its review cards in <ul class="card-list"> (Pull.jsx:131; also Checks.jsx:224). card-list has zero CSS rules in web/src/ui.css (the shipped stylesheet), so it renders as a bare ul. The #531 test already bans card-list from the pulls list (pr-structure-531.test.js line 102) — extend that standard here.
Card-in-card nesting — ReviewsList is itself a .card with a card-header (Pull.jsx:129–130) whose items are <li class="card"> (Pull.jsx:134): a bordered card inside a bordered card, diverging from the sibling sectioned-card panels on the same page. Restructure so reviews render as cards at the section level (or as an unstyled list inside one panel — implementer's call, but the double chrome must go).
btn btn-primary is not the primary-button idiom — the Finish review submit button uses class="btn btn-primary" (Pull.jsx:552). Only .btn.primary is defined in ui.css; btn-primary matches no rule, so the "primary" action currently renders as a default secondary button. Use class="btn primary" (the #479 canonical: one primary btn primary px-3 py-1 with busy label swap — the busy swap is already present at Pull.jsx:553).
label.field is an undefined class — FinishReview's two fields use <label class="field"> (Pull.jsx:538, 542; same pattern in PullNew.jsx and MergeBox.jsx). .field has no CSS rule in ui.css, so the label composition drifts from the #479 canonical label.grid.gap-1 + span.text-sm.font-medium + control.
Form controls not using .input — the Body textarea and Verdict select (Pull.jsx:540, 544) carry no .input class and render as browser-default controls, unlike every restyled form surface. Note w-full on textarea is also missing, so the two controls don't even fill the card consistently.
Verdict badges use pills, not the chip idiom — decisionBadge (Pull.jsx:68) returns .pill classes with hardcoded bg-emerald-100/bg-red-100 overrides, used for the summary-bar decision (Pull.jsx:90) and each review's state (Pull.jsx:138). Restyle verdicts onto the .chip family (rounded 4px uppercase 10px), with the state mapping spelled out: APPROVED → emerald chip, CHANGES_REQUESTED → red chip, COMMENTED → neutral .chip, dismissed → neutral/muted chip (implementer may add chip variants composing with .chip in ui.css rather than inline bg overrides — that keeps future verdicts from re-forking the palette).
Help line not field-scoped — "reviewing <head sha>" (Pull.jsx:550) is a bare paragraph; wire it via id + aria-describedby per the #479 pattern or drop it.

Acceptance criteria

No card-list (or any other undefined component class) remains in Pull.jsx's review surface; the card-in-card nesting in ReviewsList is resolved to flat sectioned cards matching the Mergeability/Reviewers/Checks panels.
Each review card reads: author, chip verdict, timestamp in the .card-meta language; stale marker and dismiss affordance unchanged in behavior.
Finish review form follows the #479 structure: label + text-sm font-medium span + .input control for Body and Verdict; the reviewing-sha line is field-scoped via aria-describedby or removed.
Submit button is the defined primary idiom (btn primary); cancel stays a plain btn. Verified in rendered output, not just source (see below).
Verdicts everywhere on the PR page (summary bar + review cards) render as chips from one shared mapping; no per-callsite hardcoded bg colors.
No behavior change: submit/dismiss/request flows, verdict values, thread staging, and navigation stay as-is; no wire/API change.
Existing unit tests stay green, including pr-structure-531.test.js.
Render verification mandate (required before close): the implementer confirms the RENDERED result, not just the source — headless DOM assertions on the review cards' structure and the Finish review form's control classes, and/or screenshot review at a desktop and a 390px width, before marking this done. CSS that looks right in source is exactly what shipped wrong once here (undefined card-list/field/btn-primary classes are invisible in source reading).

Architecture notes

The shipped stylesheet is web/src/ui.css (Tailwind v4, CSS-first); web/css/*.css is dead/unbundled — any new shared classes (chip verdict variants) belong in ui.css @layer components.
card-list also appears in Checks.jsx:224; fix it here only if trivial, otherwise note it for a follow-up so the class doesn't get re-legitimized.

# Restyle the PR review UI (Reviews section + Finish review form) to match system styling ## What's requested Bring the two remaining hand-rolled blocks of the PR page — the **Reviews section** and the **Finish review form** — onto the app's established styling idioms: the canonical form structure from #479, the primary-button idiom, chip verdicts, and the sectioned-card pattern the rest of the PR page already uses. Styling/markup only; no behavior change. ## Canonical references in the current tree - Form structure: #479 pattern (ReleaseNew.jsx / IssueNew.jsx) — `label` with `grid gap-1`, a `text-sm font-medium` span label, the `.input` class on every control, help text scoped to its field via `id` + `aria-describedby`. - Primary button idiom: `web/src/ui.css` line 73 — `.btn.primary` (emerald), i.e. `class="btn primary"`. - Chip verdicts: the `.chip` family in `web/src/ui.css` lines 108–119 (`.chip`, `.chip-open`, `.chip-closed`, `.chip-merged`, `.chip-draft`) — the status-chip language used by issue/pull rows and test `pr-structure-531.test.js`. - Sectioned cards: the PR page's other sections (Mergeability, Reviewers, Checks, Merge) are sibling `.card` panels each with a single `.card-header` (Forgejo #521 idiom). ## Divergences found (evidence, all in `web/src/pages/Pull.jsx`) - **Undefined `card-list` class** — ReviewsList wraps its review cards in `<ul class="card-list">` (Pull.jsx:131; also Checks.jsx:224). `card-list` has **zero CSS rules** in `web/src/ui.css` (the shipped stylesheet), so it renders as a bare ul. The #531 test already bans `card-list` from the pulls list (`pr-structure-531.test.js` line 102) — extend that standard here. - **Card-in-card nesting** — ReviewsList is itself a `.card` with a `card-header` (Pull.jsx:129–130) whose items are `<li class="card">` (Pull.jsx:134): a bordered card inside a bordered card, diverging from the sibling sectioned-card panels on the same page. Restructure so reviews render as cards at the section level (or as an unstyled list inside one panel — implementer's call, but the double chrome must go). - **`btn btn-primary` is not the primary-button idiom** — the Finish review submit button uses `class="btn btn-primary"` (Pull.jsx:552). Only `.btn.primary` is defined in ui.css; `btn-primary` matches no rule, so the "primary" action currently renders as a default secondary button. Use `class="btn primary"` (the #479 canonical: one primary `btn primary px-3 py-1` with busy label swap — the busy swap is already present at Pull.jsx:553). - **`label.field` is an undefined class** — FinishReview's two fields use `<label class="field">` (Pull.jsx:538, 542; same pattern in PullNew.jsx and MergeBox.jsx). `.field` has no CSS rule in ui.css, so the label composition drifts from the #479 canonical `label.grid.gap-1` + `span.text-sm.font-medium` + control. - **Form controls not using `.input`** — the Body textarea and Verdict select (Pull.jsx:540, 544) carry no `.input` class and render as browser-default controls, unlike every restyled form surface. Note `w-full` on textarea is also missing, so the two controls don't even fill the card consistently. - **Verdict badges use pills, not the chip idiom** — `decisionBadge` (Pull.jsx:68) returns `.pill` classes with hardcoded `bg-emerald-100`/`bg-red-100` overrides, used for the summary-bar decision (Pull.jsx:90) and each review's state (Pull.jsx:138). Restyle verdicts onto the `.chip` family (rounded 4px uppercase 10px), with the state mapping spelled out: APPROVED → emerald chip, CHANGES_REQUESTED → red chip, COMMENTED → neutral `.chip`, dismissed → neutral/muted chip (implementer may add chip variants composing with `.chip` in ui.css rather than inline bg overrides — that keeps future verdicts from re-forking the palette). - **Help line not field-scoped** — "reviewing <head sha>" (Pull.jsx:550) is a bare paragraph; wire it via `id` + `aria-describedby` per the #479 pattern or drop it. ## Acceptance criteria - [ ] No `card-list` (or any other undefined component class) remains in Pull.jsx's review surface; the card-in-card nesting in ReviewsList is resolved to flat sectioned cards matching the Mergeability/Reviewers/Checks panels. - [ ] Each review card reads: author, chip verdict, timestamp in the `.card-meta` language; stale marker and dismiss affordance unchanged in behavior. - [ ] Finish review form follows the #479 structure: `label` + `text-sm font-medium` span + `.input` control for Body and Verdict; the reviewing-sha line is field-scoped via `aria-describedby` or removed. - [ ] Submit button is the defined primary idiom (`btn primary`); cancel stays a plain `btn`. Verified in rendered output, not just source (see below). - [ ] Verdicts everywhere on the PR page (summary bar + review cards) render as chips from one shared mapping; no per-callsite hardcoded bg colors. - [ ] No behavior change: submit/dismiss/request flows, verdict values, thread staging, and navigation stay as-is; no wire/API change. - [ ] Existing unit tests stay green, including `pr-structure-531.test.js`. - [ ] **Render verification mandate (required before close):** the implementer confirms the RENDERED result, not just the source — headless DOM assertions on the review cards' structure and the Finish review form's control classes, and/or screenshot review at a desktop and a 390px width, before marking this done. CSS that looks right in source is exactly what shipped wrong once here (undefined `card-list`/`field`/`btn-primary` classes are invisible in source reading). ## Architecture notes - The shipped stylesheet is `web/src/ui.css` (Tailwind v4, CSS-first); `web/css/*.css` is dead/unbundled — any new shared classes (chip verdict variants) belong in ui.css `@layer components`. - `card-list` also appears in Checks.jsx:224; fix it here only if trivial, otherwise note it for a follow-up so the class doesn't get re-legitimized.

crueber added the

issues

label

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

crueber added this to the v1 milestone

2026-09-14 22:43:05 +00:00

crueber added the

pull requests

label

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

crueber referenced this issue from a commit

2026-09-14 23:40:01 +00:00

Fix #545: PR review UI onto system styling idioms (chips, #479 form, flat reviews)

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

2026-09-14 23:40:04 +00:00

Fix #545: review UI restyle #549

crueber commented

2026-09-14 23:40:07 +00:00

Author

Owner

Fix ready for review: #549 (branch fix/issue-545). Styling/markup only — flat unstyled reviews list in the one Reviews panel, one reviewVerdictChip mapping (+ new zinc chip-neutral), #479 Finish-review form, btn primary; Checks.jsx wrapper class rides along. Tests 1260/1259 (1 pre-existing live-server smoke fail), vite+esbuild green. Browser proof open per workspace rules.

Fix ready for review: https://git.packden.us/crueber/walhub/pulls/549 (branch fix/issue-545). Styling/markup only — flat unstyled reviews list in the one Reviews panel, one reviewVerdictChip mapping (+ new zinc chip-neutral), #479 Finish-review form, btn primary; Checks.jsx wrapper class rides along. Tests 1260/1259 (1 pre-existing live-server smoke fail), vite+esbuild green. Browser proof open per workspace rules.

crueber commented

2026-09-14 23:47:01 +00:00

Author

Owner

Review of PR #549 (fix/issue-545) — verified in scratch worktree /tmp/pr549 (removed afterward).

All 10 checks pass; no fixes needed:

card-in-card gone: ReviewsList stays ONE .card + card-header (the #521/#531 conversation-column idiom) holding a flat divide-y list; per-review .card chrome deleted (Pull.jsx:147-149). The issue allowed either resolution; keeping the titled panel is the right call (section-level cards would have dropped the Reviews title the guideline names as a card-header consumer).
card-list fully extinct: zero hits in web/src (Pull.jsx wrapper + Checks.jsx:224 ride-along both gone). No other undefined classes in the touched surface: label.field and btn-primary gone from Pull.jsx; decisionBadge/pill verdict def gone. Remaining .pill uses (Pull.jsx:268 reviewer-requested names, :463/:468 thread outdated/resolved badges) are the legitimate filter-chip/badge idiom per guideline §2, not verdicts.
btn primary: submit is class="btn primary px-3 py-1", cancel stays plain btn, busy swap kept (Pull.jsx:574-577).
#479 shape: 2× label.grid.gap-1 + text-sm font-medium span + .input w-full (ids finish-review-body/verdict), help id finish-review-head-help with aria-describedby on both controls, .muted helper (Pull.jsx:561-572).
Verdicts: ONE reviewVerdictChip (APPROVED→chip-open, CHANGES_REQUESTED→chip-closed, COMMENTED→chip-neutral, default→chip-draft) serving summary-bar + reviewer + review-card sites; dismissed/requested inline chip-neutral. chip-neutral composes with .chip (same inline-flex/rounded/10px/uppercase), zinc both themes (ui.css:123-125) — warranted, no neutral existed. No hardcoded bg-* verdict colors left.
Behavior identical: submit/dismiss/staging/navigation byte-identical (pinned by no-behavior-change test); diff is classes/markup/mapping only.
Guideline extended correctly: §6 gains chip-neutral + reviewVerdictChip consumer map and a Decisions entry, same change (law 11 satisfied — shared impl + entry together).
#531 pins faithful: card-list ban extended to Pull.jsx+Checks.jsx; card-meta badge-slot pin retargeted decisionBadge→reviewVerdictChip; nothing else in 531 touched.
Follow-ups correctly out of scope: label.field/btn-primary leftovers in PullNew.jsx/MergeBox.jsx/Settings.jsx noted in the 12_web_ui.md entry, not re-legitimized.
No backend change (diff is web/ + docs/ only), no new deps (no package manifest change), docs accurate (12_web_ui.md #545 entry matches the diff).

Tests (scratch worktree, node_modules symlinked from main): node --test web/test/unit/*.test.js → 1259 pass / 1 fail; the 1 failure is the pre-existing live-server smoke subtest (/setup 403) — reproduced on pristine main too, zero PR-caused. vite build exit 0. No browser drive (per task rules; reasoned at desktop + 390px from the shared row/panel/chip classes — noted explicitly).

MERGE RECOMMENDATION: ready to merge.

Review of PR #549 (fix/issue-545) — verified in scratch worktree /tmp/pr549 (removed afterward). All 10 checks pass; no fixes needed: 1. card-in-card gone: ReviewsList stays ONE .card + card-header (the #521/#531 conversation-column idiom) holding a flat divide-y list; per-review .card chrome deleted (Pull.jsx:147-149). The issue allowed either resolution; keeping the titled panel is the right call (section-level cards would have dropped the Reviews title the guideline names as a card-header consumer). 2. card-list fully extinct: zero hits in web/src (Pull.jsx wrapper + Checks.jsx:224 ride-along both gone). No other undefined classes in the touched surface: label.field and btn-primary gone from Pull.jsx; decisionBadge/pill verdict def gone. Remaining .pill uses (Pull.jsx:268 reviewer-requested names, :463/:468 thread outdated/resolved badges) are the legitimate filter-chip/badge idiom per guideline §2, not verdicts. 3. btn primary: submit is class="btn primary px-3 py-1", cancel stays plain btn, busy swap kept (Pull.jsx:574-577). 4. #479 shape: 2× label.grid.gap-1 + text-sm font-medium span + .input w-full (ids finish-review-body/verdict), help id finish-review-head-help with aria-describedby on both controls, .muted helper (Pull.jsx:561-572). 5. Verdicts: ONE reviewVerdictChip (APPROVED→chip-open, CHANGES_REQUESTED→chip-closed, COMMENTED→chip-neutral, default→chip-draft) serving summary-bar + reviewer + review-card sites; dismissed/requested inline chip-neutral. chip-neutral composes with .chip (same inline-flex/rounded/10px/uppercase), zinc both themes (ui.css:123-125) — warranted, no neutral existed. No hardcoded bg-* verdict colors left. 6. Behavior identical: submit/dismiss/staging/navigation byte-identical (pinned by no-behavior-change test); diff is classes/markup/mapping only. 7. Guideline extended correctly: §6 gains chip-neutral + reviewVerdictChip consumer map and a Decisions entry, same change (law 11 satisfied — shared impl + entry together). 8. #531 pins faithful: card-list ban extended to Pull.jsx+Checks.jsx; card-meta badge-slot pin retargeted decisionBadge→reviewVerdictChip; nothing else in 531 touched. 9. Follow-ups correctly out of scope: label.field/btn-primary leftovers in PullNew.jsx/MergeBox.jsx/Settings.jsx noted in the 12_web_ui.md entry, not re-legitimized. 10. No backend change (diff is web/ + docs/ only), no new deps (no package manifest change), docs accurate (12_web_ui.md #545 entry matches the diff). Tests (scratch worktree, node_modules symlinked from main): node --test web/test/unit/*.test.js → 1259 pass / 1 fail; the 1 failure is the pre-existing live-server smoke subtest (/setup 403) — reproduced on pristine main too, zero PR-caused. vite build exit 0. No browser drive (per task rules; reasoned at desktop + 390px from the shared row/panel/chip classes — noted explicitly). MERGE RECOMMENDATION: ready to merge.

crueber closed this issue

2026-09-14 23:47:22 +00:00

crueber referenced this issue from a commit

2026-09-14 23:47:22 +00:00

Merge pull request 'Fix #545: review UI restyle' (#549) from fix/issue-545 into main

crueber commented

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

Author

Owner

Fixed by PR #549 (review clean — all 10 checks pass, guideline extended, behavior identical), merged. Closing.

crueber referenced this issue

2026-09-15 01:13:04 +00:00