Fix #561: PR page renders review/decision statuses as display labels #563

Merged

crueber merged 1 commit from fix/issue-561 into main

2026-09-15 11:44:32 +00:00

crueber commented

2026-09-15 11:41:52 +00:00

Owner

Summary

The PR page rendered review/decision statuses as raw wire values (APPROVED, CHANGES_REQUESTED, REVIEW_REQUIRED, ...). This change adds ONE shared display-label helper and applies it at the three user-facing render sites. Display-only: the API wire contract (internal/review/model.go) is unchanged.

Changes

web/src/lib/pull-state.js: new shared reviewVerdictLabel(state) (the existing home of pullBadgeView/pullListChip — dependency-free, headless-testable, no new deps per law 1):
- APPROVED→Approved, CHANGES_REQUESTED→Changes requested, COMMENTED→Commented, REVIEW_REQUIRED→Review required, DISMISSED→Dismissed (rollup-only, model.go:62)
- unknown values pass through as-is (debuggable, never blank); missing (null/undefined/"") reads Unknown
web/src/pages/Pull.jsx (3 sites): summary-bar decision badge (~L107), reviewer chips (~L117), review-card non-dismissed branch (~L163)
Wire values stay byte-identical: FinishReview select option values, reviewVerdictChip state comparisons (keyed on wire by design — only rendered text changes), Show guards, title attributes, API payloads, MergeBox text/comparisons
Sweep grep -rn "r.state | .decision | rv.state" web/src: no other page renders raw review/decision wire values
docs/go/12_web_ui.md: FIXED (Forgejo #561) amendment in Decisions (law 12); style-guideline by reference only (composition, no new pattern); Tailwind untouched

Tests

New web/test/unit/review-verdict-label-561.test.js (6 tests): all five known labels, unknown passthrough + missing→Unknown, single-definition pin, three render-site pins, no-raw-render pins, wire-intact pins
Targeted: 38/38 pass (new + pull-state-517 + pull-state + review-restyle-545 + reviews-card-554 + review-siblings-557)
Full suite minus smoke: 1328/1328 pass (smoke excluded — needs live server, pre-existing)
vite build green; esbuild SDK bundle green; go vet ./internal/... clean (no Go change); web/dist/.keep restored

## Summary The PR page rendered review/decision statuses as raw wire values (APPROVED, CHANGES_REQUESTED, REVIEW_REQUIRED, ...). This change adds ONE shared display-label helper and applies it at the three user-facing render sites. Display-only: the API wire contract (internal/review/model.go) is unchanged. ## Changes - **web/src/lib/pull-state.js**: new shared `reviewVerdictLabel(state)` (the existing home of pullBadgeView/pullListChip — dependency-free, headless-testable, no new deps per law 1): - APPROVED→Approved, CHANGES_REQUESTED→Changes requested, COMMENTED→Commented, REVIEW_REQUIRED→Review required, DISMISSED→Dismissed (rollup-only, model.go:62) - unknown values pass through as-is (debuggable, never blank); missing (null/undefined/"") reads Unknown - **web/src/pages/Pull.jsx** (3 sites): summary-bar decision badge (~L107), reviewer chips (~L117), review-card non-dismissed branch (~L163) - Wire values stay byte-identical: FinishReview select option values, reviewVerdictChip state comparisons (keyed on wire by design — only rendered text changes), Show guards, title attributes, API payloads, MergeBox text/comparisons - Sweep `grep -rn "r.state | .decision | rv.state" web/src`: no other page renders raw review/decision wire values - **docs/go/12_web_ui.md**: FIXED (Forgejo #561) amendment in Decisions (law 12); style-guideline by reference only (composition, no new pattern); Tailwind untouched ## Tests - New web/test/unit/review-verdict-label-561.test.js (6 tests): all five known labels, unknown passthrough + missing→Unknown, single-definition pin, three render-site pins, no-raw-render pins, wire-intact pins - Targeted: 38/38 pass (new + pull-state-517 + pull-state + review-restyle-545 + reviews-card-554 + review-siblings-557) - Full suite minus smoke: 1328/1328 pass (smoke excluded — needs live server, pre-existing) - `vite build` green; `esbuild` SDK bundle green; `go vet ./internal/...` clean (no Go change); web/dist/.keep restored

crueber added 1 commit

2026-09-15 11:41:52 +00:00

Fix #561 : render review/decision statuses as display labels via shared reviewVerdictLabel c7ecb533a7

The PR page rendered raw wire values (APPROVED, CHANGES_REQUESTED,
REVIEW_REQUIRED, ...) at three user-facing sites. Adds one shared
reviewVerdictLabel(state) in web/src/lib/pull-state.js (dependency-free,
headless-testable) mapping the five known wire values to human labels
(unknown passes through, missing reads Unknown) and applies it at the
summary-bar decision badge, reviewer chips, and review-card chips in
Pull.jsx. Wire contract untouched: option values, chip-class mapping,
Show guards, titles, payloads byte-identical. Docs: 12_web_ui.md
Decisions amended (law 12); style-guideline by reference only.

crueber commented

2026-09-15 11:44:20 +00:00

Author

Owner

Independent review — APPROVED (no changes requested)

Reviewed origin/fix/issue-561 (c7ecb53, 4 files +109/-4) against issue #561, verified independently in /tmp/walhub-561. No fix commits — the branch is clean as-is.

Acceptance criteria (all met)

One shared helper, no per-callsite ternaries — reviewVerdictLabel(state) in web/src/lib/pull-state.js, used at all 3 sites; no ternaries/mapping logic in Pull.jsx.
Summary badge shows labels — Pull.jsx:107 renders reviewVerdictLabel(summary()?.decision ?? "REVIEW_REQUIRED").
Reviewer chips show labels — Pull.jsx:117 renders {who} · {reviewVerdictLabel(r.state)}.
Review-card chips show labels — Pull.jsx:163 non-dismissed branch renders reviewVerdictLabel(rv.state); dismissed branch keeps its dismissed #N text (correct — already human text).
Wire values unchanged — verified via full diff: FinishReview option values (COMMENTED/APPROVED/CHANGES_REQUESTED), state: getVerdict() payload, stale Show guard still keyed on r.state === "APPROVED"", all three reviewVerdictChip(...)class mappings still keyed on wire values, reviewer-chiptitle` keeps wire value (explicitly implementer's call per the issue).
Helper unit-tested — review-verdict-label-561.test.js: all 5 known values + unknown passthrough + 3 missing shapes (null/undefined/"") — exceeds the 5+unknown bar.
Sweep — independently re-ran the grep over web/src: no other raw renders. reviewDecision={() => summary()?.decision} (Pull.jsx:1157) passes wire into MergeBox, which only compares (=== "CHANGES_REQUESTED") and renders already-human text ("changes requested") — not a raw render, correctly untouched. {who} · requested is human text. Nothing missed.

Checklist items

Helper placement/shape: correct home (next to pullBadgeView/pullListChip), pure, zero imports (dependency-free), JSDoc convention matches siblings.
Mapping correctness: APPROVED→Approved, CHANGES_REQUESTED→Changes requested, COMMENTED→Commented, REVIEW_REQUIRED→Review required, DISMISSED→Dismissed; unknown passes through via String(state) (debuggable, never blank); null/undefined/"" → "Unknown" (the issue's implementer's-call option, noted in the doc amendment).
Exactly 3 call sites, nothing else in Pull.jsx: confirmed — import line + 3 render substitutions; no wire drift.
Docs (law 12): amendment appended to docs/go/12_web_ui.md Decisions section in the same change.
No new deps (law 1): no package.json/manifest changes; helper and test use stdlib/built-ins only.

Verification run

New test file: 6/6 pass.
Full unit suite: 1330/1331 — the single failure is the pre-existing live-server smoke.test.js (needs a live server; no server/static changes in this PR, unrelated).
vite build green (chunk-size warning is pre-existing). Note: the build wiped the tracked web/dist/.keep (emptyOutDir); I restored it — worktree is clean, nothing committed.

Verdict: APPROVE — ready to merge.

## Independent review — APPROVED (no changes requested) Reviewed `origin/fix/issue-561` (c7ecb53, 4 files +109/-4) against issue #561, verified independently in `/tmp/walhub-561`. No fix commits — the branch is clean as-is. ### Acceptance criteria (all met) - [x] **One shared helper, no per-callsite ternaries** — `reviewVerdictLabel(state)` in `web/src/lib/pull-state.js`, used at all 3 sites; no ternaries/mapping logic in `Pull.jsx`. - [x] **Summary badge shows labels** — `Pull.jsx:107` renders `reviewVerdictLabel(summary()?.decision ?? "REVIEW_REQUIRED")`. - [x] **Reviewer chips show labels** — `Pull.jsx:117` renders `{who} · {reviewVerdictLabel(r.state)}`. - [x] **Review-card chips show labels** — `Pull.jsx:163` non-dismissed branch renders `reviewVerdictLabel(rv.state)`; dismissed branch keeps its `dismissed #N` text (correct — already human text). - [x] **Wire values unchanged** — verified via full diff: FinishReview option values (`COMMENTED`/`APPROVED`/`CHANGES_REQUESTED`), `state: getVerdict()` payload, stale `Show` guard still keyed on `r.state === "APPROVED"", all three `reviewVerdictChip(...)` class mappings still keyed on wire values, reviewer-chip `title` keeps wire value (explicitly implementer's call per the issue). - [x] **Helper unit-tested** — `review-verdict-label-561.test.js`: all 5 known values + unknown passthrough + 3 missing shapes (null/undefined/"") — exceeds the 5+unknown bar. - [x] **Sweep** — independently re-ran the grep over `web/src`: no other raw renders. `reviewDecision={() => summary()?.decision}` (Pull.jsx:1157) passes wire into `MergeBox`, which only *compares* (`=== "CHANGES_REQUESTED"`) and renders already-human text ("changes requested") — not a raw render, correctly untouched. `{who} · requested` is human text. Nothing missed. ### Checklist items - **Helper placement/shape**: correct home (next to `pullBadgeView`/`pullListChip`), pure, zero imports (dependency-free), JSDoc convention matches siblings. - **Mapping correctness**: APPROVED→Approved, CHANGES_REQUESTED→Changes requested, COMMENTED→Commented, REVIEW_REQUIRED→Review required, DISMISSED→Dismissed; unknown passes through via `String(state)` (debuggable, never blank); null/undefined/"" → "Unknown" (the issue's implementer's-call option, noted in the doc amendment). - **Exactly 3 call sites, nothing else in Pull.jsx**: confirmed — import line + 3 render substitutions; no wire drift. - **Docs (law 12)**: amendment appended to `docs/go/12_web_ui.md` Decisions section in the same change. - **No new deps (law 1)**: no package.json/manifest changes; helper and test use stdlib/built-ins only. ### Verification run - New test file: 6/6 pass. - Full unit suite: 1330/1331 — the single failure is the pre-existing live-server `smoke.test.js` (needs a live server; no server/static changes in this PR, unrelated). - `vite build` green (chunk-size warning is pre-existing). Note: the build wiped the tracked `web/dist/.keep` (emptyOutDir); I restored it — worktree is clean, nothing committed. Verdict: **APPROVE — ready to merge.**

crueber merged commit 9c9690e5ae into main

2026-09-15 11:44:32 +00:00

crueber referenced this pull request from a commit

2026-09-15 11:44:33 +00:00

Merge pull request 'Fix #561: PR page renders review/decision statuses as display labels' (#563) from fix/issue-561 into main

crueber referenced this pull request

2026-09-15 11:44:33 +00:00

PR page renders review/decision statuses as raw wire values (REVIEW_REQUIRED etc.) #561