PROut needs merged flag so the pulls list can render the merged chip #530

New issue

Closed

opened 2026-09-14 18:25:56 +00:00 by crueber · 3 comments

crueber commented

2026-09-14 18:25:56 +00:00

Owner

Follow-up scoped out of #521 (PR #528 review findings). The conversation page renders the merged chip from the thread payload (pr.merged), but PROut (service.go:731-741) carries no merged field and ListPRs (:801-805) drops pr.Merged — so the pulls list (Pulls.jsx:124 chip-${state}) cannot render a merged chip without a wire change. Add merged to PROut (additive field, old clients ignore) and render chip-merged on the list.

crueber referenced this issue

2026-09-14 18:26:10 +00:00

Redesign PR conversation page to match the issue page's design language (header chip, markdown bodies, merged state) #521

crueber referenced this issue from a commit

2026-09-14 20:02:29 +00:00

Fix #530: PROut.merged flag (03 §8)

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

2026-09-14 20:02:32 +00:00

Fix #530: PROut.merged flag #534

crueber commented

2026-09-14 20:02:34 +00:00

Author

Owner

Fixed by #534 (#534): PROut gains additive always-present merged, populated from the existing per-row pr.json read (zero new trips); list renders chip-merged.

Fixed by #534 (https://git.packden.us/crueber/walhub/pulls/534): PROut gains additive always-present `merged`, populated from the existing per-row pr.json read (zero new trips); list renders chip-merged.

crueber commented

2026-09-14 20:52:23 +00:00

Author

Owner

Review of PR #534 (fix/issue-530, commit 8bb2b8b), verified in scratch worktree /tmp/pr534 (removed afterward). No browser (tests + reasoning only, per instructions). Main worktree untouched (still clean apart from pre-existing untracked .opencode/).

FINDINGS (all pass, no fixes needed):

Additive field — OK. internal/pulls/service.go:731-743: Merged bool json:"merged" with NO omitempty, matching the PROut/Draft always-present discipline. Additive JSON: old clients ignore unknown fields. No bucket/protobuf touch (PRDoc.Merged already exists at internal/pulls/model.go:103), so law 5 wire-compat holds; no golden-fixture impact.
Zero new round trips — OK. service.go:796 loads pr.json per row (pre-existing enrichment GET); :809 just reads pr.Merged off the already-loaded struct. No new loadPR/getJSON call; law 6 holds structurally.
List chip — OK. web/src/lib/pull-state.js pullListChip: merged wins, lowercase "merged" text on chip chip-merged (list convention; conversation header keeps capitalized badge). web/src/pages/Pulls.jsx:131-134 wires it. Absent-flag old payloads fall back to state (pinned in test). Nit (non-blocking): Pulls.jsx calls pullListChip(pr) twice per row (cls + text); harmless.
#521 pin update legitimate — OK. web/test/unit/pull-event-text-521.test.js replaces the stale no-signal pin with a #530 pin and documents the supersede in the header comment. Justified: the wire it pinned changed.
Coverage/deps/docs — OK. internal/pulls 96.1% statements (≥95% gate); gofmt clean; go vet clean; go build ./... ok; no go.mod/package.json changes (no new deps); docs/features/03_pull_requests.md §8 table + decision entry accurate.

TEST RESULTS (scratch worktree):

go test ./internal/pulls/... -race -count=1: PASS (2.4s)
coverage: 96.1% statements
node --test web/test/unit/*.test.js: 1216/1217 pass; the 1 fail is smoke.test.js 'built SPA shell is served at / and /setup' (403 vs 200 — needs live :8080, pre-existing, unrelated; matches PR description disclosure). Targeted pull-list-chip-530 + pull-event-text-521: 16/16 pass.
vite build: PASS (2.2s; chunk-size warning pre-existing). make web itself fails on mise/pnpm env, so vite was run directly via symlinked node_modules.

MERGE RECOMMENDATION: ready to merge.

Review of PR #534 (fix/issue-530, commit 8bb2b8b), verified in scratch worktree /tmp/pr534 (removed afterward). No browser (tests + reasoning only, per instructions). Main worktree untouched (still clean apart from pre-existing untracked .opencode/). FINDINGS (all pass, no fixes needed): 1. Additive field — OK. internal/pulls/service.go:731-743: Merged bool json:"merged" with NO omitempty, matching the PROut/Draft always-present discipline. Additive JSON: old clients ignore unknown fields. No bucket/protobuf touch (PRDoc.Merged already exists at internal/pulls/model.go:103), so law 5 wire-compat holds; no golden-fixture impact. 2. Zero new round trips — OK. service.go:796 loads pr.json per row (pre-existing enrichment GET); :809 just reads pr.Merged off the already-loaded struct. No new loadPR/getJSON call; law 6 holds structurally. 3. List chip — OK. web/src/lib/pull-state.js pullListChip: merged wins, lowercase "merged" text on chip chip-merged (list convention; conversation header keeps capitalized badge). web/src/pages/Pulls.jsx:131-134 wires it. Absent-flag old payloads fall back to state (pinned in test). Nit (non-blocking): Pulls.jsx calls pullListChip(pr) twice per row (cls + text); harmless. 4. #521 pin update legitimate — OK. web/test/unit/pull-event-text-521.test.js replaces the stale no-signal pin with a #530 pin and documents the supersede in the header comment. Justified: the wire it pinned changed. 5. Coverage/deps/docs — OK. internal/pulls 96.1% statements (≥95% gate); gofmt clean; go vet clean; go build ./... ok; no go.mod/package.json changes (no new deps); docs/features/03_pull_requests.md §8 table + decision entry accurate. TEST RESULTS (scratch worktree): - go test ./internal/pulls/... -race -count=1: PASS (2.4s) - coverage: 96.1% statements - node --test web/test/unit/*.test.js: 1216/1217 pass; the 1 fail is smoke.test.js 'built SPA shell is served at / and /setup' (403 vs 200 — needs live :8080, pre-existing, unrelated; matches PR description disclosure). Targeted pull-list-chip-530 + pull-event-text-521: 16/16 pass. - vite build: PASS (2.2s; chunk-size warning pre-existing). make web itself fails on mise/pnpm env, so vite was run directly via symlinked node_modules. MERGE RECOMMENDATION: ready to merge.

crueber closed this issue

2026-09-14 20:53:26 +00:00

crueber commented

2026-09-14 20:53:27 +00:00

Author

Owner

Fixed by PR #534 (review clean — additive field, zero new trips, list chip verified; review was delayed by an interruption, now complete), merged. Closing.

crueber referenced this issue from a commit

2026-09-14 20:53:27 +00:00

Merge pull request 'Fix #530: PROut.merged flag' (#534) from fix/issue-530 into main

crueber referenced this issue

2026-09-14 22:07:02 +00:00

Fix #531: PR structure rework #542

crueber referenced this issue

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

Rework PR page right column into one sectioned panel and PR list into flat divider rows (issues-surface idioms) — supersedes #521's structure #531