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

Merged

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

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

crueber commented

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

Owner

Implements Forgejo #594 per the binding user rulings.

Server gates (service layer, all route twins covered):

issues AddComment/AddReaction refuse 409 (ErrLocked) on closed issues; removal stays allowed (personal undo, named decision); no override in v1 (named decision).
pulls AddComment refuses 409 on merged/closed PRs; UpdatePR state:open on a merged PR now refused 409 (ErrConflict, closing the server-side reopen gap — only closing was refused before).
review OpenThread/AddThreadComment/SubmitReview refuse 409 on merged/closed PRs (SubmitReview gated deliberately: it would otherwise bypass the lock outright).
All gates ride already-loaded docs (zero new store reads, law 6); no new locks (law 3); no schema change (law 5, proto append-only).

Client:

pullCommentLock (pull-state.js) / issueCommentLock (issue-events.js) helpers; composers + thread replies render disabled-with-reason; add-reaction menu disabled (chips stay removal-only); no reopen control for merged PRs (already correct, now test-pinned); 409s toast verbatim via the tray path; reopen unlocks via stream with no reload.

Tests: table-driven Go tests per package (locked→409+message, open→passes, reopen-restores, merged-terminal incl. reopen refusal, reaction add/remove split); web unit (helpers, merged no-reopen pin, 409 toast path). Coverage ≥95% holds (issues/pulls/review at 96.0/96.2/96.0%). Web full-minus-smoke 1495/1495; vite+esbuild builds clean; web/dist/.keep restored.

Docs: Decisions + behavior notes in 02_issues.md, 03_pull_requests.md, 04_code_review.md in the same commit.

Known limitation: no live-browser drive in this environment (no CDP/Chromium); UI changes are disabled-states on existing controls with canonical Tailwind classes only (no layout changes) — suggest a mobile+desktop console check at review.

Implements Forgejo #594 per the binding user rulings. Server gates (service layer, all route twins covered): - issues AddComment/AddReaction refuse 409 (ErrLocked) on closed issues; removal stays allowed (personal undo, named decision); no override in v1 (named decision). - pulls AddComment refuses 409 on merged/closed PRs; UpdatePR state:open on a merged PR now refused 409 (ErrConflict, closing the server-side reopen gap — only closing was refused before). - review OpenThread/AddThreadComment/SubmitReview refuse 409 on merged/closed PRs (SubmitReview gated deliberately: it would otherwise bypass the lock outright). - All gates ride already-loaded docs (zero new store reads, law 6); no new locks (law 3); no schema change (law 5, proto append-only). Client: - pullCommentLock (pull-state.js) / issueCommentLock (issue-events.js) helpers; composers + thread replies render disabled-with-reason; add-reaction menu disabled (chips stay removal-only); no reopen control for merged PRs (already correct, now test-pinned); 409s toast verbatim via the tray path; reopen unlocks via stream with no reload. Tests: table-driven Go tests per package (locked→409+message, open→passes, reopen-restores, merged-terminal incl. reopen refusal, reaction add/remove split); web unit (helpers, merged no-reopen pin, 409 toast path). Coverage ≥95% holds (issues/pulls/review at 96.0/96.2/96.0%). Web full-minus-smoke 1495/1495; vite+esbuild builds clean; web/dist/.keep restored. Docs: Decisions + behavior notes in 02_issues.md, 03_pull_requests.md, 04_code_review.md in the same commit. Known limitation: no live-browser drive in this environment (no CDP/Chromium); UI changes are disabled-states on existing controls with canonical Tailwind classes only (no layout changes) — suggest a mobile+desktop console check at review.

crueber added 1 commit

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

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

Server: service-layer threadLocked gates (zero new store reads) in
internal/issues (AddComment, AddReaction), internal/pulls (AddComment,
UpdatePR refuses reopen-on-merged 409), internal/review (OpenThread,
AddThreadComment, SubmitReview); typed ErrLocked -> 409 per package.
Reaction removal stays allowed (personal undo); no override in v1.

Client: pullCommentLock/issueCommentLock helpers; composers and thread
replies render disabled-with-reason; add-reaction menu disabled
(chips stay removal-only); no reopen control for merged PRs (pinned);
409s toast verbatim; reopen unlocks via stream with no reload.

Docs: decisions + behavior notes in 02_issues, 03_pull_requests,
04_code_review.

crueber commented

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

Author

Owner

APPROVE — independent review of fix/issue-594 (0ef124a) vs #594.

Verified against the 8 scrutiny points (code-read + tests run in /tmp/walhub-594):

EXTRA SubmitReview gate — CORRECT and consistent. review/service.go:368 gates on threadLocked(h,side) = h.State!=open OR side.Merged, same predicate as OpenThread/AddThreadComment. Closed-but-unmerged is locked (ticket wants locked), reopen (state→open, merged=false) restores submit too — covered by TestThreadWritesLockedOnClosedPR reopen block. No legit flow broken: open PRs unaffected; closed/merged submits now 409 like every other conversation write. Justification in 04_code_review.md (bypass otherwise trivial) accepted.
Race safety — pulls hoisting SAFE. AddComment hoists loadPR ahead of appendEvent, but the gate runs INSIDE the CAS mutator on the fresh thread doc, so a merge landing between the two reads still trips the state arm (merge stamps StateClosed in the same header CAS). Same-read-count claim holds: the post-CAS loadPR it replaces was result-discarded (old lines ~1023-1025). Issues AddComment likewise gates inside the mutator; AddReaction checks the already-loaded header (pre-existing read); review prHeadOf header+sidecar reused (no new reads). Law 6 holds. Residual note (non-blocking): review OpenThread/AddThreadComment/SubmitReview check-then-act outside their write CASes, so a merge landing in that micro-window could slip one thread through — narrow, no extra-read fix available in-budget; pulls/issues do not share this hole.
Law 3 / law 8 / 409 / toast — clean. No new locks/mutexes in the diff; no internal/server or internal/api imports (only server/auth + store, as before). ErrLocked→409 via statusFor in all three packages (issues/errors.go, pulls/model.go, review/model.go), and all three http.go route through writePlain(statusFor(err), err.Error()) — 409, not 500. Messages are plain human text with the number only ('issue #N is closed', 'pull request #N'), no internals leaked; web 409-toast test pins verbatim tray delivery.
Reaction removal allowed — consistent, no bypass. issues RemoveReaction deliberately ungated; chips stay live while the + menu disables. Abuse check: removal requires owning the reaction (reactionState scan, else 404) and rides a CAS that only decrements the summary + appends a remove event; it does bump Version/UpdatedAt, but that is the pre-existing personal-undo write, not new-content injection. Accepted as decided.
Resolve/unresolve ungated — agree, curation. setResolved touches only the thread header (Resolved/By/At, Version/UpdatedAt), emits thread_resolved/unresolved notify+stream classes and refreshSummary — no commented/reaction_changed conversation event, so the lock spirit holds.
Client — stream unlock verified structurally, merged pin holds, double-guarded submits. Issue.jsx keys disabled off live thread() with useCollabStream(['issue','issue_event']) invalidating the key; Pull.jsx keys pullCommentLock(thread(),pr()) with useCollabStream(['pull','review','thread','check'], num-filter) — reopen flips the helper to unlocked with no reload. pullCloseVisibility merged→{false,false} first-line, test-pinned for all roles; closed-unmerged keeps Reopen. Every locked composer is double-guarded: CommentComposer (locked() early-return + disabled textarea/submit), ThreadCard reply (commentLocked return + disabled input/button), FinishReview (return + disabled submit), PullFiles submitThread (return + CommentComposer disabled).
Pin updates strictly stronger — yes. composer-cancel-row-587 and review-restyle-545 now assert the full literal including 'disabled:cursor-not-allowed disabled:opacity-50' (milestone-picker idiom); primary idiom intact, requirement strengthened, not weakened.
Coverage/docs/deps — hold. go test -cover: issues 96.0 / pulls 96.1 / review 96.0 (≥95). New tests fail pre-fix by inspection (no gate existed: AddComment/AddReaction/OpenThread/SubmitReview never consulted state; reopen-on-merged succeeded server-side). Docs amended in-commit per law 12 (02_issues, 03_pull_requests, 04_code_review with named decisions). No go.mod/web dependency changes.

No fix commits made — no blocking defects found. One suggestion for follow-up (not requested as rework): consider re-checking threadLocked inside SubmitReview's reserve loop for the stamp-in-flight window if a cheap header-state re-read is ever acceptable. Live-browser mobile+desktop console check still outstanding per PR description (no CDP here either).

Verdict: APPROVE.

APPROVE — independent review of fix/issue-594 (0ef124a) vs #594. Verified against the 8 scrutiny points (code-read + tests run in /tmp/walhub-594): 1. EXTRA SubmitReview gate — CORRECT and consistent. review/service.go:368 gates on threadLocked(h,side) = h.State!=open OR side.Merged, same predicate as OpenThread/AddThreadComment. Closed-but-unmerged is locked (ticket wants locked), reopen (state→open, merged=false) restores submit too — covered by TestThreadWritesLockedOnClosedPR reopen block. No legit flow broken: open PRs unaffected; closed/merged submits now 409 like every other conversation write. Justification in 04_code_review.md (bypass otherwise trivial) accepted. 2. Race safety — pulls hoisting SAFE. AddComment hoists loadPR ahead of appendEvent, but the gate runs INSIDE the CAS mutator on the fresh thread doc, so a merge landing between the two reads still trips the state arm (merge stamps StateClosed in the same header CAS). Same-read-count claim holds: the post-CAS loadPR it replaces was result-discarded (old lines ~1023-1025). Issues AddComment likewise gates inside the mutator; AddReaction checks the already-loaded header (pre-existing read); review prHeadOf header+sidecar reused (no new reads). Law 6 holds. Residual note (non-blocking): review OpenThread/AddThreadComment/SubmitReview check-then-act outside their write CASes, so a merge landing in that micro-window could slip one thread through — narrow, no extra-read fix available in-budget; pulls/issues do not share this hole. 3. Law 3 / law 8 / 409 / toast — clean. No new locks/mutexes in the diff; no internal/server or internal/api imports (only server/auth + store, as before). ErrLocked→409 via statusFor in all three packages (issues/errors.go, pulls/model.go, review/model.go), and all three http.go route through writePlain(statusFor(err), err.Error()) — 409, not 500. Messages are plain human text with the number only ('issue #N is closed', 'pull request #N'), no internals leaked; web 409-toast test pins verbatim tray delivery. 4. Reaction removal allowed — consistent, no bypass. issues RemoveReaction deliberately ungated; chips stay live while the + menu disables. Abuse check: removal requires owning the reaction (reactionState scan, else 404) and rides a CAS that only decrements the summary + appends a remove event; it does bump Version/UpdatedAt, but that is the pre-existing personal-undo write, not new-content injection. Accepted as decided. 5. Resolve/unresolve ungated — agree, curation. setResolved touches only the thread header (Resolved/By/At, Version/UpdatedAt), emits thread_resolved/unresolved notify+stream classes and refreshSummary — no commented/reaction_changed conversation event, so the lock spirit holds. 6. Client — stream unlock verified structurally, merged pin holds, double-guarded submits. Issue.jsx keys disabled off live thread() with useCollabStream(['issue','issue_event']) invalidating the key; Pull.jsx keys pullCommentLock(thread(),pr()) with useCollabStream(['pull','review','thread','check'], num-filter) — reopen flips the helper to unlocked with no reload. pullCloseVisibility merged→{false,false} first-line, test-pinned for all roles; closed-unmerged keeps Reopen. Every locked composer is double-guarded: CommentComposer (locked() early-return + disabled textarea/submit), ThreadCard reply (commentLocked return + disabled input/button), FinishReview (return + disabled submit), PullFiles submitThread (return + CommentComposer disabled). 7. Pin updates strictly stronger — yes. composer-cancel-row-587 and review-restyle-545 now assert the full literal including 'disabled:cursor-not-allowed disabled:opacity-50' (milestone-picker idiom); primary idiom intact, requirement strengthened, not weakened. 8. Coverage/docs/deps — hold. go test -cover: issues 96.0 / pulls 96.1 / review 96.0 (≥95). New tests fail pre-fix by inspection (no gate existed: AddComment/AddReaction/OpenThread/SubmitReview never consulted state; reopen-on-merged succeeded server-side). Docs amended in-commit per law 12 (02_issues, 03_pull_requests, 04_code_review with named decisions). No go.mod/web dependency changes. No fix commits made — no blocking defects found. One suggestion for follow-up (not requested as rework): consider re-checking threadLocked inside SubmitReview's reserve loop for the stamp-in-flight window if a cheap header-state re-read is ever acceptable. Live-browser mobile+desktop console check still outstanding per PR description (no CDP here either). Verdict: APPROVE.

crueber merged commit 58b43e705b into main

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

crueber referenced this pull request

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

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

crueber referenced this pull request 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