Reaction guards share the stale-key shape from #143 #146

New issue

Closed

opened 2026-09-05 17:03:03 +00:00 by crueber · 3 comments

crueber commented

2026-09-05 17:03:03 +00:00

Owner

Follow-up noted in the #145 review: reaction add/remove guards (Issue.jsx) and comment/close tails share the stale-key-across-navigation shape fixed for label/milestone paths in #143 (pinned num/key + guard reset on nav). Same treatment: pin synchronously pre-await, reconcile the pinned key, reset guards on num() change. Acceptance: rapid react-then-navigate never swallows clicks or reconciles the wrong issue; node --test green; browser proof.

crueber referenced this issue from a commit

2026-09-05 18:01:34 +00:00

Fix #146: stale-key guards on reactions/comments (02_issues)

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

2026-09-05 18:01:38 +00:00

Fix #146: stale-key guards on reactions/comments #147

crueber commented

2026-09-05 18:01:42 +00:00

Author

Owner

Fix ready for review: #147 (branch fix/issue-146). Pins num/key pre-await + pinned-key reconcile + reaction-guard reset on nav for the reaction add/remove paths and comment/close tails — same shape as #145, no no-nav behavior change. node --test 317/317 green, web builds clean, no new deps. One caveat for reviewers: full browser proof was blocked by the shared browser daemon (new document loads hung browser-wide under shared load); verification rests on tests + build + pattern-mirroring. loadOlder left untouched as out of scope.

crueber commented

2026-09-05 18:03:55 +00:00

Author

Owner

PR #147 review (fix/issue-146, commit 405c3aa) — verified in scratch worktree, main worktree untouched (read-only).

PIN/RECONCILE — all pass. Every touched path captures num()/key() synchronously before the first await and reconciles with if (num()===n) reload() else invalidate(ck): comment (Issue.jsx:111-117), commentAndClose (:119-136), close (:140-146), react (:189-201), toggleReaction incl. 404-fallback add (:210-231), patch/reopen (:233-243, covers the onClose→patch({state:open}) reopen at :403). Grep over the file confirms no post-await num() read remains on any mutation path — the only post-await reads are the reconcile guards themselves. commentAndClose pins ONCE up front (:120-121) and both the comment POST (:131) and close PATCH (:133) use the pinned n — the mid-flight-navigation-closes-new-issue bug is fixed; I diffed origin/main to confirm the old code did read num() fresh for the PATCH after the comment await, so the doc's claim is accurate.

BUSY-SET MOVE — correct. getBusy/setBusy/busyKey/isBusy moved above the nav effect (:69-71) with setBusy(new Set()) in the reset (:87), same never-read-early shape as the #143 sidebar guards. Leak walk: a stale tail's finally only deletes its own key — a no-op on the fresh set, so nothing leaks and nothing sticks disabled. The one residual window (stale finally clearing a same-key entry a new-issue click just added, enabling the chip early) can at worst cause a redundant POST, which the server dedups per (actor,target,content) (02 §8, cited in the code). Bounded and accepted — same tradeoff as the landed #143 pattern.

BUMP(CK) — all 3 call sites updated (:193, :214, :219); no other callers anywhere (local const). Imports unchanged — no new deps (law 1 clean). No-nav path is byte-identical behaviorally: same issue takes reload(), which is exactly the old unconditional call. Doc entry in 02_issues.md verified against the old code (single-step tails did invalidate the NEW key via reload(); commentAndClose did re-read num()) — accurate (law 12 clean). Laws 7/8: N/A — short mutations with tray reporting, no new routes/imports.

TESTS: node --test web/test/unit/*.test.js → 317 pass / 0 fail; vite build clean (121 modules, built in ~2s); esbuild SDK bundle clean. (No browser drive — env-blocked as the author noted; recorded, not re-attempted per instructions.)

MERGE CALL: pattern-mirror + suite-green suffices here — no new headless tests demanded. Rationale: this is a line-for-line mirror of the already-merged #143 fix in the same file, the pin-before-first-await ordering is statically verifiable (done above), and the no-nav branch is provably identical. Extracting a testable helper for a Solid closure-over-params pattern would be disproportionate churn for the residual risk class already accepted in #145.

One non-blocking follow-up (pre-existing, out of scope, do NOT hold the merge): loadOlder (:303-317) still passes num() as a pre-await arg with no pinned reconcile — a mid-flight navigation can append the old issue's older-events page into the new issue's getExtra (the nav reset clears it, the stale fetch re-appends). Suggest a follow-up issue.

No fixes pushed — nothing to fix. MERGE RECOMMENDATION: ready to merge.

PR #147 review (fix/issue-146, commit 405c3aa) — verified in scratch worktree, main worktree untouched (read-only). PIN/RECONCILE — all pass. Every touched path captures num()/key() synchronously before the first await and reconciles with if (num()===n) reload() else invalidate(ck): comment (Issue.jsx:111-117), commentAndClose (:119-136), close (:140-146), react (:189-201), toggleReaction incl. 404-fallback add (:210-231), patch/reopen (:233-243, covers the onClose→patch({state:open}) reopen at :403). Grep over the file confirms no post-await num() read remains on any mutation path — the only post-await reads are the reconcile guards themselves. commentAndClose pins ONCE up front (:120-121) and both the comment POST (:131) and close PATCH (:133) use the pinned n — the mid-flight-navigation-closes-new-issue bug is fixed; I diffed origin/main to confirm the old code did read num() fresh for the PATCH after the comment await, so the doc's claim is accurate. BUSY-SET MOVE — correct. getBusy/setBusy/busyKey/isBusy moved above the nav effect (:69-71) with setBusy(new Set()) in the reset (:87), same never-read-early shape as the #143 sidebar guards. Leak walk: a stale tail's finally only deletes its own key — a no-op on the fresh set, so nothing leaks and nothing sticks disabled. The one residual window (stale finally clearing a same-key entry a new-issue click just added, enabling the chip early) can at worst cause a redundant POST, which the server dedups per (actor,target,content) (02 §8, cited in the code). Bounded and accepted — same tradeoff as the landed #143 pattern. BUMP(CK) — all 3 call sites updated (:193, :214, :219); no other callers anywhere (local const). Imports unchanged — no new deps (law 1 clean). No-nav path is byte-identical behaviorally: same issue takes reload(), which is exactly the old unconditional call. Doc entry in 02_issues.md verified against the old code (single-step tails did invalidate the NEW key via reload(); commentAndClose did re-read num()) — accurate (law 12 clean). Laws 7/8: N/A — short mutations with tray reporting, no new routes/imports. TESTS: node --test web/test/unit/*.test.js → 317 pass / 0 fail; vite build clean (121 modules, built in ~2s); esbuild SDK bundle clean. (No browser drive — env-blocked as the author noted; recorded, not re-attempted per instructions.) MERGE CALL: pattern-mirror + suite-green suffices here — no new headless tests demanded. Rationale: this is a line-for-line mirror of the already-merged #143 fix in the same file, the pin-before-first-await ordering is statically verifiable (done above), and the no-nav branch is provably identical. Extracting a testable helper for a Solid closure-over-params pattern would be disproportionate churn for the residual risk class already accepted in #145. One non-blocking follow-up (pre-existing, out of scope, do NOT hold the merge): loadOlder (:303-317) still passes num() as a pre-await arg with no pinned reconcile — a mid-flight navigation can append the old issue's older-events page into the new issue's getExtra (the nav reset clears it, the stale fetch re-appends). Suggest a follow-up issue. No fixes pushed — nothing to fix. MERGE RECOMMENDATION: ready to merge.

crueber closed this issue

2026-09-05 18:04:11 +00:00

crueber referenced this issue from a commit

2026-09-05 18:04:11 +00:00

Merge pull request 'Fix #146: stale-key guards on reactions/comments' (#147) from fix/issue-146 into main

crueber commented

2026-09-05 18:04:12 +00:00

Author

Owner

Fixed by PR #147 (review: all paths pin pre-await, no post-await num() remains; 317/317 node tests), merged. Closing.

crueber added the

issues