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

Merged

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

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

crueber commented

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

Owner

Follow-up noted in the #145 review: reaction add/remove tails (react, toggleReaction incl. the remove-404 fallback add) and comment/close tails (comment, commentAndClose, close, reopen patch) in web/src/pages/Issue.jsx shared the stale-key-across-navigation shape fixed for label/milestone paths in #143. Same treatment: pin num()/key() synchronously pre-await, run the mutation + optimistic paint against the pinned key, reconcile with reload() when still on the issue else invalidate() the pinned key, and reset the per-(seq,content) reaction busy set in the num() navigation effect. No behavior change on the no-nav path (same issue still takes the reload() branch). Appends the decision to docs/features/02_issues.md in the same change.

Worth noting: the two-step commentAndClose was worse than stale-reconcile — its close PATCH evaluated num() after the comment POST resolved, so a mid-flight navigation closed the NEW issue instead; pinning once up front fixes that too.

Verification: node --test web/test/unit/*.test.js 317/317 green; make web (vite+esbuild) builds clean. No new deps. Browser proof attempted in headless Chrome over CDP against a scratch server (forced mid-flight navigation via delayed reactions POST + client-side route change, both themes, console-error capture): the shared browser daemon stopped loading new documents mid-session (evaluates fine, new document loads hang browser-wide under shared load; one earlier tab did render the fixed page with 1 reaction row). Full browser proof therefore not completed — node tests + build + pattern-mirroring carry the verification. Deliberately out of scope: loadOlder has a similar tail shape but was not named in #146.

Follow-up noted in the #145 review: reaction add/remove tails (`react`, `toggleReaction` incl. the remove-404 fallback add) and comment/close tails (`comment`, `commentAndClose`, `close`, reopen `patch`) in `web/src/pages/Issue.jsx` shared the stale-key-across-navigation shape fixed for label/milestone paths in #143. Same treatment: pin `num()`/`key()` synchronously pre-await, run the mutation + optimistic paint against the pinned key, reconcile with `reload()` when still on the issue else `invalidate()` the pinned key, and reset the per-(`seq`,`content`) reaction busy set in the `num()` navigation effect. No behavior change on the no-nav path (same issue still takes the `reload()` branch). Appends the decision to `docs/features/02_issues.md` in the same change. Worth noting: the two-step `commentAndClose` was worse than stale-reconcile — its close PATCH evaluated `num()` after the comment POST resolved, so a mid-flight navigation closed the NEW issue instead; pinning once up front fixes that too. Verification: `node --test web/test/unit/*.test.js` 317/317 green; `make web` (vite+esbuild) builds clean. No new deps. Browser proof attempted in headless Chrome over CDP against a scratch server (forced mid-flight navigation via delayed reactions POST + client-side route change, both themes, console-error capture): the shared browser daemon stopped loading new documents mid-session (evaluates fine, new document loads hang browser-wide under shared load; one earlier tab did render the fixed page with 1 reaction row). Full browser proof therefore not completed — node tests + build + pattern-mirroring carry the verification. Deliberately out of scope: `loadOlder` has a similar tail shape but was not named in #146.

crueber added 1 commit

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

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

Reaction add/remove tails (react, toggleReaction incl. the remove-404
fallback add) and the comment/close tails (comment, commentAndClose,
close, reopen patch) shared the stale-key-across-navigation shape fixed
for label/milestone paths in #143: same-route navigation reuses the one
thread component, so async tails reconciled (reload) the NEW issue's key
and stranded the mutated issue on its optimistic guess; the two-step
commentAndClose even evaluated num() for its close PATCH after the
comment POST resolved (wrong-issue close on mid-flight navigation); and
the per-(seq,content) reaction busy set leaked across issues (in-flight
busy on the old issue silently swallowed clicks on the new one).

Same treatment as #143: pin num()/key() synchronously pre-await, run the
mutation + optimistic paint against the pinned key, reconcile with
reload() when still on the issue else invalidate() the pinned key, and
reset the reaction busy set in the num() navigation effect. No behavior
change when the view does not move (same-issue takes the reload branch).

Appends the decision to 02_issues.md in the same change.

crueber referenced this pull request

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

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

crueber referenced this pull request

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

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

crueber merged commit c2ae865d85 into main

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

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