PRs: plain Close / Reopen action with issue-style state badge (no merge/review/comment required)

crueber commented

2026-09-14 15:44:47 +00:00

Owner

What's requested

A plain Close (and Reopen) action on the pull request conversation page, for PRs that should not be merged and need no review or comment — with the PR's state displayed exactly the way the issue page displays state.

Evidence

The backend and SDK already fully support this; only the UI is missing:

internal/pulls/service.go (~L848–955): UpdatePR applies PUT field state (open|closed). Auth: author or triage (triage may close others'). Closing a merged PR is refused (409). Close/reopen append state_changed events and emit pull stream events (closed / reopened actions).
web/sdk/src/pulls.js L37–39: repo.pulls.update(num, fields) — PUT …/pulls/{num} (title/body/state; unknown keys 400). The client surface exists and is unused for state.
web/src/pages/Pull.jsx: the only PR lifecycle affordance is MergeBox. There is no Close button, no Reopen button, and the header does not render an open/closed state badge. A stale or unwanted PR can only be abandoned open, or merged to get rid of it.
The convention to mirror is the issue page (web/src/pages/Issue.jsx): header with state badge (title left, state badge right, per its header comment), a Close menu in the comment-composer area (including comment-and-close), and a Reopen control once closed. State-label helpers live in web/src/lib/issue-events.js (closedStateLabel, closePatch).

Architecture notes

No backend change: wire repo.pulls.update(num, { state: "closed" | "open" }) from the PR page. Same params, same permissions the server already enforces.
The thread GET (repo.pulls.get) carries the current state (thread.state / pr header) — use it as the state source of truth for badge and button visibility, same as the issue page does.
Reconcile: UpdatePR streams pull events (closed/reopened); Pull.jsx already subscribes to the collaboration stream and has the cross-page reconcile pattern (issue #319 / the #318 pattern, ~L585) — invalidation of coalesced keys on those frames should cover list cards + open_pulls badges; verify the stream accept filter runs before invalidation (see Pull.jsx L34–36, state_changed → "closed"/"reopened" timeline text already exists).
No cached-SWR field is added, so no ETag coverage change is needed.
UI state sourced from shared fetches: the state badge on the PR page must read the per-PR thread fetch, not any repo-level summary (repo summaries are ref/state-blind by design).

Acceptance criteria

Open PR shows a Close action (author or triage sees it; below-write viewers do not — the page already hides review/merge affordances below write, ~L571, follow that pattern for visibility)
Close sends PUT { state: "closed" }, adds a "closed" timeline entry, and flips the header state badge to closed without a full reload (stream reconcile)
Closed (unmerged) PR shows a Reopen action with the same permission rules; reopening flips the badge back
Merged PRs: no Close/Reopen control; server 409 on merged close stays a handled error (toast, no silent failure)
State badge text/style matches the issue page's state badge convention (title left, badge right)
Closing/reopening a PR updates the open_pulls count surfaces via the existing invalidation path (no stale count until next full reload)
No code change to internal/pulls — pure client wiring

## What's requested A plain **Close** (and **Reopen**) action on the pull request conversation page, for PRs that should not be merged and need no review or comment — with the PR's state displayed exactly the way the issue page displays state. ## Evidence The backend and SDK already fully support this; only the UI is missing: - `internal/pulls/service.go` (~L848–955): `UpdatePR` applies PUT field `state` (`open|closed`). Auth: **author or triage** (triage may close others'). Closing a merged PR is refused (409). Close/reopen append `state_changed` events and emit `pull` stream events (`closed` / `reopened` actions). - `web/sdk/src/pulls.js` L37–39: `repo.pulls.update(num, fields)` — `PUT …/pulls/{num}` (title/body/state; unknown keys 400). The client surface exists and is unused for state. - `web/src/pages/Pull.jsx`: the only PR lifecycle affordance is `MergeBox`. There is no Close button, no Reopen button, and the header does not render an open/closed state badge. A stale or unwanted PR can only be abandoned open, or merged to get rid of it. - The convention to mirror is the issue page (`web/src/pages/Issue.jsx`): header with **state badge** (title left, state badge right, per its header comment), a **Close menu** in the comment-composer area (including comment-and-close), and a **Reopen** control once closed. State-label helpers live in `web/src/lib/issue-events.js` (`closedStateLabel`, `closePatch`). ## Architecture notes - No backend change: wire `repo.pulls.update(num, { state: "closed" | "open" })` from the PR page. Same params, same permissions the server already enforces. - The thread GET (`repo.pulls.get`) carries the current state (`thread.state` / `pr` header) — use it as the state source of truth for badge and button visibility, same as the issue page does. - Reconcile: `UpdatePR` streams `pull` events (`closed`/`reopened`); Pull.jsx already subscribes to the collaboration stream and has the cross-page reconcile pattern (issue #319 / the #318 pattern, ~L585) — invalidation of coalesced keys on those frames should cover list cards + open_pulls badges; verify the stream `accept` filter runs before invalidation (see `Pull.jsx` L34–36, `state_changed` → "closed"/"reopened" timeline text already exists). - No cached-SWR field is added, so no ETag coverage change is needed. - UI state sourced from shared fetches: the state badge on the PR page must read the per-PR thread fetch, not any repo-level summary (repo summaries are ref/state-blind by design). ## Acceptance criteria - [ ] Open PR shows a Close action (author or triage sees it; below-write viewers do not — the page already hides review/merge affordances below write, ~L571, follow that pattern for visibility) - [ ] Close sends `PUT { state: "closed" }`, adds a "closed" timeline entry, and flips the header state badge to closed without a full reload (stream reconcile) - [ ] Closed (unmerged) PR shows a Reopen action with the same permission rules; reopening flips the badge back - [ ] Merged PRs: no Close/Reopen control; server 409 on merged close stays a handled error (toast, no silent failure) - [ ] State badge text/style matches the issue page's state badge convention (title left, badge right) - [ ] Closing/reopening a PR updates the open_pulls count surfaces via the existing invalidation path (no stale count until next full reload) - [ ] No code change to `internal/pulls` — pure client wiring

crueber added the

pull requests

label

2026-09-14 15:44:50 +00:00

crueber added this to the v1 milestone

2026-09-14 15:44:50 +00:00

crueber referenced this issue from a commit

2026-09-14 16:29:00 +00:00

Fix #517: PR Close/Reopen with issue-style state badge (pure client wiring)

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

2026-09-14 16:29:06 +00:00

Fix #517: PR Close/Reopen #523

crueber commented

2026-09-14 16:29:09 +00:00

Author

Owner

Fix ready for review: #523 (branch fix/issue-517) — pure client wiring, no backend change. Badge (title-left/badge-right, Merged wins via chip-merged), Close + Comment-and-Close on open PRs and Reopen on closed-unmerged for author-or-triage, no control on merged (409 → tray toast), #318-style reconcile via new invalidatePullLists + existing pull-frame stream path. Tests: 1159 pass / 0 fail (3 live-server smoke skipped), vite+esbuild green, no new deps.

crueber commented

2026-09-14 16:38:55 +00:00

Author

Owner

Review of PR #523 (fix/issue-517, commit e820570) — all 7 acceptance criteria verified, no fixes needed.

(1) Badge — PASS. Pull.jsx L716-723 mirrors Issue.jsx L434-442 exactly (flex flex-wrap items-start justify-between, title left / badge right). Open/Closed reuse chip-open/chip-closed; merged-wins via pr.merged is correct since merge stamps StateClosed too (merge.go outcome is write-once, thread.state alone can't distinguish). chip-merged (ui.css L114-115, purple) is warranted — new visual state, GitHub convention, no existing class fits.

(2) Visibility — PASS. pull-state.js L42-44 mirrors UpdatePR auth (service.go L875-876: author-or-triage) with the hierarchical P6 ladder (policy.go + perms.jsx LADDER identical: read<triage<write<maintain<admin, so write ⊇ triage). Below-triage non-authors get plain composer only (canComment at Pull.jsx L578 is role-non-null-gated, independent of closeVis). Author-with-read can close own PR — matches server.

(3) Close / Comment-and-Close / Reopen — PASS. Correct endpoint: pulls.update → PUT …/pulls/{num} {state} (sdk pulls.js L37-39). commentAndClosePR (Pull.jsx L678-690) is line-for-line the Issue.jsx L174-190 pattern (comment-first GitHub semantics, same split-failure note + plain-Close fallback). Merged → closeVis false/false, no control; raced merged-close 409 lands in tray via CommentComposer runClose/commentAndClose catch → reportError (CommentComposer.jsx L206-229), never silent. No reason chooser — correct, PR state is open|closed only.

(4) Stream reconcile — PASS. Mutation site reload()s own thread key (badge flip + closed timeline entry, no full reload; eventText state_changed→closed/reopened already at Pull.jsx L35-36) + invalidatePullLists. Other tabs follow closed/reopened pull frames on the existing repo stream; accept filter runs before invalidateCollab (collab.jsx L28-29), and the pull frame table already covers thread+lists+repo (collab.js L54-57). No new kinds, no ETag change.

(5) open_pulls — PASS. invalidatePullLists (data.js L361-364) clears pulls:{full}:* (matches Pulls.jsx L37 list-key shape) + repo:{full} (open_pulls numerator per tabs.js); reload() also clears repo:. Mirrors invalidateIssueLists (#318 precedent).

(6) Scope/hygiene — PASS. No .go diff, no package.json/pnpm diff (no new deps), docs Decision entry in 03_pull_requests.md (law 12, same commit). Badge reads live thread/pr fetch, never repo summary.

Verified in scratch worktree /tmp/pr523 (removed afterward): new pull-state-517.test.js 8/8 pass; full suite minus smoke 1159 pass / 0 fail (smoke.test.js hangs without a live server — expected, same 3-skipped shape as PR description); vite build green with 'Comment and Close' + 'chip-merged' confirmed in dist bundle. No browser drive (node tests + reasoning only — shared chrome-cdp daemon blocks loopback; header/composer rows reuse the issue page's flex-wrap pattern). Main worktree left clean (only pre-existing untracked .opencode/).

MERGE RECOMMENDATION: ready to merge.

Review of PR #523 (fix/issue-517, commit e820570) — all 7 acceptance criteria verified, no fixes needed. (1) Badge — PASS. Pull.jsx L716-723 mirrors Issue.jsx L434-442 exactly (flex flex-wrap items-start justify-between, title left / badge right). Open/Closed reuse chip-open/chip-closed; merged-wins via pr.merged is correct since merge stamps StateClosed too (merge.go outcome is write-once, thread.state alone can't distinguish). chip-merged (ui.css L114-115, purple) is warranted — new visual state, GitHub convention, no existing class fits. (2) Visibility — PASS. pull-state.js L42-44 mirrors UpdatePR auth (service.go L875-876: author-or-triage) with the hierarchical P6 ladder (policy.go + perms.jsx LADDER identical: read<triage<write<maintain<admin, so write ⊇ triage). Below-triage non-authors get plain composer only (canComment at Pull.jsx L578 is role-non-null-gated, independent of closeVis). Author-with-read can close own PR — matches server. (3) Close / Comment-and-Close / Reopen — PASS. Correct endpoint: pulls.update → PUT …/pulls/{num} {state} (sdk pulls.js L37-39). commentAndClosePR (Pull.jsx L678-690) is line-for-line the Issue.jsx L174-190 pattern (comment-first GitHub semantics, same split-failure note + plain-Close fallback). Merged → closeVis false/false, no control; raced merged-close 409 lands in tray via CommentComposer runClose/commentAndClose catch → reportError (CommentComposer.jsx L206-229), never silent. No reason chooser — correct, PR state is open|closed only. (4) Stream reconcile — PASS. Mutation site reload()s own thread key (badge flip + closed timeline entry, no full reload; eventText state_changed→closed/reopened already at Pull.jsx L35-36) + invalidatePullLists. Other tabs follow closed/reopened pull frames on the existing repo stream; accept filter runs before invalidateCollab (collab.jsx L28-29), and the pull frame table already covers thread+lists+repo (collab.js L54-57). No new kinds, no ETag change. (5) open_pulls — PASS. invalidatePullLists (data.js L361-364) clears pulls:{full}:* (matches Pulls.jsx L37 list-key shape) + repo:{full} (open_pulls numerator per tabs.js); reload() also clears repo:. Mirrors invalidateIssueLists (#318 precedent). (6) Scope/hygiene — PASS. No .go diff, no package.json/pnpm diff (no new deps), docs Decision entry in 03_pull_requests.md (law 12, same commit). Badge reads live thread/pr fetch, never repo summary. Verified in scratch worktree /tmp/pr523 (removed afterward): new pull-state-517.test.js 8/8 pass; full suite minus smoke 1159 pass / 0 fail (smoke.test.js hangs without a live server — expected, same 3-skipped shape as PR description); vite build green with 'Comment and Close' + 'chip-merged' confirmed in dist bundle. No browser drive (node tests + reasoning only — shared chrome-cdp daemon blocks loopback; header/composer rows reuse the issue page's flex-wrap pattern). Main worktree left clean (only pre-existing untracked .opencode/). MERGE RECOMMENDATION: ready to merge.

crueber closed this issue

2026-09-14 16:39:27 +00:00

crueber referenced this issue from a commit

2026-09-14 16:39:27 +00:00

Merge pull request 'Fix #517: PR Close/Reopen' (#523) from fix/issue-517 into main

crueber commented

2026-09-14 16:39:28 +00:00

Author

Owner

Fixed by PR #523 (review clean — all 6 areas pass, issue convention mirrored, reconcile verified), merged. Closing.

crueber referenced this issue from a commit

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