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

crueber commented

2026-09-15 11:31:30 +00:00

Owner

What's requested

Review and decision statuses on the PR page render as raw SCREAMING_SNAKE wire values instead of human-readable labels. The API's wire contract (internal/review/model.go:91, :163 — state is APPROVED|CHANGES_REQUESTED|COMMENTED; decision is REVIEW_REQUIRED etc.) is correct and must stay unchanged — this is a display-label mapping at the render sites only.

Add ONE shared display-label helper (e.g. reviewVerdictLabel(state) in web/src/lib/pull-state.js, the existing home of PR display helpers, dependency-free and headless-testable per the pull-state.js convention) mapping:

APPROVED → Approved
CHANGES_REQUESTED → Changes requested
COMMENTED → Commented
REVIEW_REQUIRED → Review required
DISMISSED → Dismissed (rollup-only value, internal/review/model.go:62)
unknown/missing → pass through as-is (or a neutral Unknown), implementer's call, noted in the PR

Apply it at every user-facing site; wire values (select option values, reviewVerdictChip state comparisons, Show guards, title attributes) stay byte-identical.

Evidence (static read, web/src/pages/Pull.jsx)

Line 107 — summary-bar decision badge renders the raw value:
{summary()?.decision ?? "REVIEW_REQUIRED"} → users see REVIEW_REQUIRED as the page's headline status.
Line 117 — reviewer chips: {who} · {r.state} → alice · APPROVED, bob · CHANGES_REQUESTED.
Line 163 — review card chips: the non-dismissed branch renders rv.state raw (APPROVED/CHANGES_REQUESTED/COMMENTED).

Not bugs (already human text, leave alone): the FinishReview select option labels (Pull.jsx:699-701, "approve" etc. — the values are wire strings and must stay), MergeBox.jsx text ("changes requested"), and the chip-class mapping reviewVerdictChip (Pull.jsx:82, keyed on wire values by design).

Architecture notes

web/src/lib/pull-state.js is the established home for exactly this class of pure PR display helpers (pullBadgeView, pullListChip) and is covered by node --test; the new helper belongs there next to them, with a unit test.
Chip color mapping and label mapping are separate concerns: reviewVerdictChip (Pull.jsx:82) stays keyed on wire values — only the rendered text changes.
The title tooltip on reviewer chips (Pull.jsx:116) may keep the wire value if that's useful for debugging, implementer's call — but the visible text gets the label.

Acceptance criteria

One shared helper (pull-state.js or a sibling lib module) maps every verdict/decision wire value to a human label; no per-callsite ternaries.
Summary-bar decision badge (Pull.jsx:107) shows "Review required" / "Approved" / etc., never REVIEW_REQUIRED.
Reviewer chips (Pull.jsx:117) show alice · Approved style labels.
Review card chips (Pull.jsx:163) show labels for rv.state.
Wire values unchanged: select option values, chip-class mapping, state comparisons, and API payloads are untouched.
node --test covers the new helper (all five known values + unknown passthrough).
No other page renders raw review/decision wire values (sweep: grep -rn "r.state\|\.decision\|rv.state" web/src).

## What's requested Review and decision statuses on the PR page render as raw SCREAMING_SNAKE wire values instead of human-readable labels. The API's wire contract (internal/review/model.go:91, :163 — `state` is `APPROVED|CHANGES_REQUESTED|COMMENTED`; `decision` is `REVIEW_REQUIRED` etc.) is correct and must stay unchanged — this is a display-label mapping at the render sites only. Add ONE shared display-label helper (e.g. `reviewVerdictLabel(state)` in `web/src/lib/pull-state.js`, the existing home of PR display helpers, dependency-free and headless-testable per the pull-state.js convention) mapping: - `APPROVED` → `Approved` - `CHANGES_REQUESTED` → `Changes requested` - `COMMENTED` → `Commented` - `REVIEW_REQUIRED` → `Review required` - `DISMISSED` → `Dismissed` (rollup-only value, internal/review/model.go:62) - unknown/missing → pass through as-is (or a neutral `Unknown`), implementer's call, noted in the PR Apply it at every user-facing site; wire values (select option values, `reviewVerdictChip` state comparisons, `Show` guards, title attributes) stay byte-identical. ## Evidence (static read, web/src/pages/Pull.jsx) - **Line 107** — summary-bar decision badge renders the raw value: `{summary()?.decision ?? "REVIEW_REQUIRED"}` → users see `REVIEW_REQUIRED` as the page's headline status. - **Line 117** — reviewer chips: `{who} · {r.state}` → `alice · APPROVED`, `bob · CHANGES_REQUESTED`. - **Line 163** — review card chips: the non-dismissed branch renders `rv.state` raw (`APPROVED`/`CHANGES_REQUESTED`/`COMMENTED`). Not bugs (already human text, leave alone): the FinishReview select option labels (Pull.jsx:699-701, "approve" etc. — the *values* are wire strings and must stay), MergeBox.jsx text ("changes requested"), and the chip-class mapping `reviewVerdictChip` (Pull.jsx:82, keyed on wire values by design). ## Architecture notes - `web/src/lib/pull-state.js` is the established home for exactly this class of pure PR display helpers (pullBadgeView, pullListChip) and is covered by `node --test`; the new helper belongs there next to them, with a unit test. - Chip color mapping and label mapping are separate concerns: `reviewVerdictChip` (Pull.jsx:82) stays keyed on wire values — only the rendered text changes. - The `title` tooltip on reviewer chips (Pull.jsx:116) may keep the wire value if that's useful for debugging, implementer's call — but the visible text gets the label. ## Acceptance criteria - [ ] One shared helper (pull-state.js or a sibling lib module) maps every verdict/decision wire value to a human label; no per-callsite ternaries. - [ ] Summary-bar decision badge (Pull.jsx:107) shows "Review required" / "Approved" / etc., never `REVIEW_REQUIRED`. - [ ] Reviewer chips (Pull.jsx:117) show `alice · Approved` style labels. - [ ] Review card chips (Pull.jsx:163) show labels for `rv.state`. - [ ] Wire values unchanged: select option values, chip-class mapping, state comparisons, and API payloads are untouched. - [ ] `node --test` covers the new helper (all five known values + unknown passthrough). - [ ] No other page renders raw review/decision wire values (sweep: `grep -rn "r.state\|\.decision\|rv.state" web/src`).

crueber added the

bug

pull requests

labels

2026-09-15 11:31:37 +00:00

crueber added this to the v1 milestone

2026-09-15 11:31:37 +00:00

crueber added the

issues

label

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

crueber referenced this issue from a commit

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

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

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

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

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

crueber referenced this issue

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

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

crueber closed this issue

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

crueber referenced this issue 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 commented

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

Author

Owner

Fixed by #563 (merged): shared reviewVerdictLabel helper in pull-state.js applied at all three PR-page sites (summary badge, reviewer chips, review-card chips). Wire contract untouched. Verified: full suite green apart from pre-existing live-server smoke failure, vite+esbuild green, independent review APPROVE.

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 18:15:12 +00:00

Mergeability wording: "Able to be Merged" in green; simplified soft-red phrases for every blocked state #588

crueber referenced this issue

2026-09-15 19:17:04 +00:00

Mergeability wording: human phrases with green/soft-red tones (Forgejo #588) #591