crueber/walhub

Watch

Fork 0

Lock commenting (comments, replies, reactions, inline threads) on closed issues and merged/closed PRs #594

New issue

Closed

opened 2026-09-15 21:05:09 +00:00 by crueber · 1 comment

crueber commented

2026-09-15 21:05:09 +00:00

Owner

User rulings (binding)

Issues can be reopened — reopening must restore commenting. The lock keys on current thread state (not a one-way latch); reopen is already a standing acceptance criterion below, kept prominent.
Merged PRs are FINAL. No reopen path exists or should exist for a merged PR, and the comment lock on a merged PR is permanent. Today the server only refuses closing a merged PR (409 at internal/pulls/service.go:908); a state:"open" patch on a merged PR currently SUCCEEDS server-side (only the UI hides the button — pull-state.js:219 showReopen:false). This ticket must close that gap: server-side refusal of reopen-on-merged, and no reopen affordance anywhere for merged PRs (client already correct — bind it with a test).
Closed-but-unmerged PRs may still be reopened, per existing behavior: UpdatePR accepts state: open|closed for author or triage role (internal/pulls/service.go:896-901), and the client shows Reopen via pullCloseVisibility (web/src/lib/pull-state.js:213-224). Reopening a plain-closed PR restores commenting, same as issues.

What's requested

Lock commenting on finished conversations: when an issue is closed, or a PR is merged or closed, the thread becomes read-only — no new comments, no replies, no reactions, no inline review threads. Reopening (or unmerging the closed state) restores commenting.

Both layers are in scope:

Server-side gate (authoritative): the write endpoints refuse with a clear 409-class error when the target thread is closed (issue) or merged/closed (PR).
Client affordances: the composers and reaction controls on Issue.jsx, Pull.jsx, and PullFiles.jsx render as disabled-with-reason ("This conversation is closed" / "Merged — commenting is locked") instead of accepting input that only fails on submit.

Reopen restores everything: state flips back to open and the gate opens with it (the gate should key on current thread state, not a one-way latch).

Evidence (static read of current tree, no local repro)

Every write path authenticates and checks read access, then mutates without ever consulting thread state:

Issue comments: internal/issues/service.go AddComment (~:199) — requireAuthenticated + requireRead only; the CAS mutator appends unconditionally regardless of t.State.
Issue reactions: internal/issues/service.go AddReaction (~~:609) / RemoveReaction (~~:678) — validate actor, target event, and emoji set (ReactionContents), never state.
PR conversation comments: internal/pulls/service.go AddComment (~:992) — same shape; no check of merged or closed state.
Inline review threads: internal/review/threads.go OpenThread (~~:63), AddThreadComment (~~:257) — auth + read only; threads can be opened and replied to on a merged PR today.
Client: web/src/pages/Issue.jsx and web/src/pages/Pull.jsx render the composer unconditionally — the state chip (chip-closed / chip-merged via pullBadgeView in web/src/lib/pull-state.js) is display-only; Pull.jsx:1441 gates some UI on thread()?.state === "open" || pr()?.merged but not the composers.

Net effect: closing an issue or merging a PR doesn't stop the conversation, which surprises users and lets activity events (commented, reaction_changed) mutate the thread after it's finished.

Architecture notes

Gate placement: a small shared predicate per package (threadLocked(th) bool) checked at the top of AddComment, AddReaction, RemoveReaction (issues), AddComment (pulls), and OpenThread/AddThreadComment (review). Keep it in the service layer, not the HTTP handler, so all three route twins are covered by construction.
Error shape: a typed error alongside ErrInvalid/ErrNotFound in internal/issues/errors.go (mirrored for pulls/review), surfaced as 409 with a human-readable message the SPA can toast.
EventClosedByPR / state transitions already exist in internal/issues/model.go; pull-state.js already computes merged vs plain-closed for display — reuse canModifyPullState-style client helpers (web/src/lib/pull-state.js) for the affordance gating rather than new logic.
Existing precedent for disabled-with-reason affordances exists in the settings/review gating work (#586 family) — follow that pattern.
Reactions on a locked thread: decide whether a user can still REMOVE their own pre-lock reaction (recommend yes — removal is a personal undo, not conversation activity; flag as a decision point if the cheaper blanket-block is chosen).
Maintainers/admins: decide whether write-capable roles keep an override. Recommend no override in v1 (simplest contract); note it as a named decision point.

Acceptance criteria

POST comment on a closed issue → 409 with a human-readable reason; same for reactions (add + remove-decision) and reopened-not (state open → passes).
POST comment / inline thread open / thread reply on a merged PR → 409; plain-closed PR → 409.
Reopen restores: reopening the issue or a closed-but-unmerged PR re-enables all four write paths; history, reactions, and threads unchanged.
Merged PRs are terminal: server refuses state:"open" on a merged PR (409); regression test asserting reopen-on-merged fails server-side; client renders no reopen control for merged PRs (test pullCloseVisibility merged → {showClose:false, showReopen:false} stays).
Comment lock on a merged PR is permanent (no state transition unlocks it).
Issue.jsx, Pull.jsx, PullFiles.jsx composers render disabled with reason text on locked threads; no input that silently fails on submit.
Reaction controls on locked threads are disabled (or removal-only per the decision above) client-side too.
Stream/cache: the 409 is client-handleable (no raw TypeError toast); composer state reacts to issue/pull stream frames so a reopen unlocks without a reload.
Unit tests cover the gate per package (issues, pulls, review) including the reopen-restores case.

## User rulings (binding) 1. **Issues can be reopened — reopening must restore commenting.** The lock keys on current thread state (not a one-way latch); reopen is already a standing acceptance criterion below, kept prominent. 2. **Merged PRs are FINAL.** No reopen path exists or should exist for a merged PR, and the comment lock on a merged PR is permanent. Today the server only refuses *closing* a merged PR (409 at `internal/pulls/service.go:908`); a `state:"open"` patch on a merged PR currently SUCCEEDS server-side (only the UI hides the button — `pull-state.js:219` `showReopen:false`). This ticket must close that gap: server-side refusal of reopen-on-merged, and no reopen affordance anywhere for merged PRs (client already correct — bind it with a test). 3. **Closed-but-unmerged PRs may still be reopened**, per existing behavior: `UpdatePR` accepts `state: open|closed` for author or triage role (`internal/pulls/service.go:896-901`), and the client shows Reopen via `pullCloseVisibility` (`web/src/lib/pull-state.js:213-224`). Reopening a plain-closed PR restores commenting, same as issues. ## What's requested Lock commenting on finished conversations: when an issue is closed, or a PR is merged or closed, the thread becomes read-only — no new comments, no replies, no reactions, no inline review threads. Reopening (or unmerging the closed state) restores commenting. Both layers are in scope: 1. **Server-side gate** (authoritative): the write endpoints refuse with a clear 409-class error when the target thread is closed (issue) or merged/closed (PR). 2. **Client affordances**: the composers and reaction controls on Issue.jsx, Pull.jsx, and PullFiles.jsx render as disabled-with-reason ("This conversation is closed" / "Merged — commenting is locked") instead of accepting input that only fails on submit. Reopen restores everything: state flips back to open and the gate opens with it (the gate should key on current thread state, not a one-way latch). ## Evidence (static read of current tree, no local repro) Every write path authenticates and checks read access, then mutates without ever consulting thread state: - **Issue comments**: `internal/issues/service.go` `AddComment` (~:199) — `requireAuthenticated` + `requireRead` only; the CAS mutator appends unconditionally regardless of `t.State`. - **Issue reactions**: `internal/issues/service.go` `AddReaction` (~:609) / `RemoveReaction` (~:678) — validate actor, target event, and emoji set (`ReactionContents`), never state. - **PR conversation comments**: `internal/pulls/service.go` `AddComment` (~:992) — same shape; no check of `merged` or closed state. - **Inline review threads**: `internal/review/threads.go` `OpenThread` (~:63), `AddThreadComment` (~:257) — auth + read only; threads can be opened and replied to on a merged PR today. - **Client**: `web/src/pages/Issue.jsx` and `web/src/pages/Pull.jsx` render the composer unconditionally — the state chip (`chip-closed` / `chip-merged` via `pullBadgeView` in `web/src/lib/pull-state.js`) is display-only; `Pull.jsx:1441` gates some UI on `thread()?.state === "open" || pr()?.merged` but not the composers. Net effect: closing an issue or merging a PR doesn't stop the conversation, which surprises users and lets activity events (`commented`, `reaction_changed`) mutate the thread after it's finished. ## Architecture notes - Gate placement: a small shared predicate per package (`threadLocked(th) bool`) checked at the top of `AddComment`, `AddReaction`, `RemoveReaction` (issues), `AddComment` (pulls), and `OpenThread`/`AddThreadComment` (review). Keep it in the service layer, not the HTTP handler, so all three route twins are covered by construction. - Error shape: a typed error alongside `ErrInvalid`/`ErrNotFound` in `internal/issues/errors.go` (mirrored for pulls/review), surfaced as 409 with a human-readable message the SPA can toast. - `EventClosedByPR` / state transitions already exist in `internal/issues/model.go`; `pull-state.js` already computes merged vs plain-closed for display — reuse `canModifyPullState`-style client helpers (`web/src/lib/pull-state.js`) for the affordance gating rather than new logic. - Existing precedent for disabled-with-reason affordances exists in the settings/review gating work (#586 family) — follow that pattern. - Reactions on a locked thread: decide whether a user can still REMOVE their own pre-lock reaction (recommend yes — removal is a personal undo, not conversation activity; flag as a decision point if the cheaper blanket-block is chosen). - Maintainers/admins: decide whether write-capable roles keep an override. Recommend no override in v1 (simplest contract); note it as a named decision point. ## Acceptance criteria - [ ] POST comment on a closed issue → 409 with a human-readable reason; same for reactions (add + remove-decision) and reopened-not (state open → passes). - [ ] POST comment / inline thread open / thread reply on a merged PR → 409; plain-closed PR → 409. - [ ] **Reopen restores**: reopening the issue or a closed-but-unmerged PR re-enables all four write paths; history, reactions, and threads unchanged. - [ ] **Merged PRs are terminal**: server refuses `state:"open"` on a merged PR (409); regression test asserting reopen-on-merged fails server-side; client renders no reopen control for merged PRs (test `pullCloseVisibility` merged → `{showClose:false, showReopen:false}` stays). - [ ] Comment lock on a merged PR is permanent (no state transition unlocks it). - [ ] Issue.jsx, Pull.jsx, PullFiles.jsx composers render disabled with reason text on locked threads; no input that silently fails on submit. - [ ] Reaction controls on locked threads are disabled (or removal-only per the decision above) client-side too. - [ ] Stream/cache: the 409 is client-handleable (no raw TypeError toast); composer state reacts to `issue`/`pull` stream frames so a reopen unlocks without a reload. - [ ] Unit tests cover the gate per package (issues, pulls, review) including the reopen-restores case.

crueber added the

enhancement

issues

pull requests

labels

2026-09-15 21:05:19 +00:00

crueber added this to the v1 milestone

2026-09-15 21:05:19 +00:00

crueber added

bug

and removed

enhancement

labels

2026-09-15 21:05:54 +00:00

crueber referenced this issue from a commit

2026-09-15 21:28:56 +00:00

Lock commenting on closed issues and merged/closed PRs (Forgejo #594)

crueber referenced this issue

2026-09-15 21:29:00 +00:00

Terminal PRs (merged/closed) show a merged/closed status in the sidebar instead of the Review Summary, and hide the moot Mergeability section #602

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

2026-09-15 21:29:01 +00:00

Lock commenting on closed issues and merged/closed PRs (Fix #594) #603

crueber referenced this issue

2026-09-15 21:31:35 +00:00

Lock commenting on closed issues and merged/closed PRs (Fix #594) #603

crueber closed this issue

2026-09-15 21:31:57 +00:00

crueber commented

2026-09-15 21:31:58 +00:00

Author

Owner

Fixed by #603 (merged): service-layer threadLocked gates (issues/pulls/review → 409) incl. SubmitReview bypass-closure; reaction-remove stays open as personal undo; reopen-on-merged refused 409 + no client control; disabled-with-reason UI on all three pages keyed on live fetches (stream unlock, double-guarded submits); reopen restores everything. Verified: cover ≥95% all three packages, -race green, 1495 web unit green (smoke excluded, pre-existing), vite/esbuild green, independent review APPROVE.

crueber referenced this issue from a commit

2026-09-15 21:31:58 +00:00

Merge pull request 'Lock commenting on closed issues and merged/closed PRs (Fix #594)' (#603) from fix/issue-594 into main

crueber referenced this issue

2026-09-15 22:05:07 +00:00

Block review requests on merged/closed PRs (Fix #599) #607

crueber referenced this issue

2026-09-15 22:06:57 +00:00

Block review requests on merged/closed PRs (Fix #599) #607

crueber referenced this issue

2026-09-15 22:30:49 +00:00

Terminal PR sidebar: Status replaces Review summary, Mergeability hidden (Forgejo #602) #609