On the issue comment page, must be able to remove a reaction #36

New issue

Closed

opened 2026-09-04 16:44:17 +00:00 by crueber · 3 comments

crueber commented

2026-09-04 16:44:17 +00:00

Owner

If you react to a comment on the issue comment page, you should be able to remove that reaction, and you should only be able to add a given reaction one time.

crueber referenced this issue from a commit

2026-09-04 20:02:06 +00:00

Fix #36, #42: reaction toggle/remove, immediate show, fold event rows (02_issues.md UI)

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

2026-09-04 20:02:13 +00:00

Fix #36, #42: reaction toggle/remove, immediate show, no comment-row #53

crueber commented

2026-09-04 20:02:25 +00:00

Author

Owner

Fix open in #53 (scratch-verified, needs review — do not merge yet): #53 — summary chips toggle (DELETE own reaction, remove-404 falls back to add), one in-flight mutation per (seq,content) so double-clicks never double-fire.

Fix open in #53 (scratch-verified, needs review — do not merge yet): https://git.packden.us/crueber/walhub/pulls/53 — summary chips toggle (DELETE own reaction, remove-404 falls back to add), one in-flight mutation per (seq,content) so double-clicks never double-fire.

crueber commented

2026-09-04 20:05:53 +00:00

Author

Owner

PR #53 review (scratch worktree @ ae16709, main worktree untouched read-only):

VERIFIED

node --test web/test/unit/*.test.js: 227/227 pass (initial 2 failures were missing web/node_modules in the fresh worktree, not PR code; resolved by copying deps into scratch env only).
vite build + esbuild SDK bundle: both succeed, no new deps (runtime still solid-js + @solidjs/router; law 1 holds).

REVIEW vs brief, all sound:

Toggle/remove (web/src/pages/Issue.jsx:154-172): remove-first; 404 (err?.notFound || status===404, matches sdk ApiError getter web/sdk/src/errors.js:25) falls back to add; non-404 only reports, never re-adds; double failure reports. Optimistic guess tracks it (-1, then +2 on fallback). Correct.
Busy-key scope (Issue.jsx:106-122): one in-flight per (seq,content), shared by picker react() and chip toggleReaction(), buttons natively disabled with theme-agnostic disabled:cursor-wait/disabled:opacity-50. Double-click/Enter-repeat cannot double-fire; server §8 dedup covers sequential dupes (doc hedges 'sequential' correctly per 02 §8 best-effort note).
patchCached vs #41 guard (web/src/lib/data.js:211-219): seq++ retires pre-mutation in-flight before the signal commit, so the race resolves for the optimistic value; every path follows with invalidate() which reconciles (or rolls back on failure). No unguarded cache commits; fn-returns-new-object contract documented + enforced by adjustSummary's frozen-input test. reaction-cache.test.js covers paint/reconcile, stale-drop, rollback, prune.
Folding complete (Issue.jsx:79 + eventText): reaction_changed filtered before ThreadTimeline, no row path remains (default branch unreachable for that type); reactionChangedText kept, still tested, documented as diagnostics-only. grep confirms no other consumer.
a11y/dark-light: native buttons, aria-labels/titles/role=group untouched; only theme-agnostic disabled classes added.
Doc (docs/features/02_issues.md): matches code on all four claims; law 12 satisfied in same change. Laws 7/8: no new seams, no new waiting (fast mutations + tray, existing pattern).

No findings requiring changes. Recommendation: ready to merge.

PR #53 review (scratch worktree @ ae16709, main worktree untouched read-only): VERIFIED - node --test web/test/unit/*.test.js: 227/227 pass (initial 2 failures were missing web/node_modules in the fresh worktree, not PR code; resolved by copying deps into scratch env only). - vite build + esbuild SDK bundle: both succeed, no new deps (runtime still solid-js + @solidjs/router; law 1 holds). REVIEW vs brief, all sound: - Toggle/remove (web/src/pages/Issue.jsx:154-172): remove-first; 404 (err?.notFound || status===404, matches sdk ApiError getter web/sdk/src/errors.js:25) falls back to add; non-404 only reports, never re-adds; double failure reports. Optimistic guess tracks it (-1, then +2 on fallback). Correct. - Busy-key scope (Issue.jsx:106-122): one in-flight per (seq,content), shared by picker react() and chip toggleReaction(), buttons natively disabled with theme-agnostic disabled:cursor-wait/disabled:opacity-50. Double-click/Enter-repeat cannot double-fire; server §8 dedup covers sequential dupes (doc hedges 'sequential' correctly per 02 §8 best-effort note). - patchCached vs #41 guard (web/src/lib/data.js:211-219): seq++ retires pre-mutation in-flight before the signal commit, so the race resolves for the optimistic value; every path follows with invalidate() which reconciles (or rolls back on failure). No unguarded cache commits; fn-returns-new-object contract documented + enforced by adjustSummary's frozen-input test. reaction-cache.test.js covers paint/reconcile, stale-drop, rollback, prune. - Folding complete (Issue.jsx:79 + eventText): reaction_changed filtered before ThreadTimeline, no row path remains (default branch unreachable for that type); reactionChangedText kept, still tested, documented as diagnostics-only. grep confirms no other consumer. - a11y/dark-light: native buttons, aria-labels/titles/role=group untouched; only theme-agnostic disabled classes added. - Doc (docs/features/02_issues.md): matches code on all four claims; law 12 satisfied in same change. Laws 7/8: no new seams, no new waiting (fast mutations + tray, existing pattern). No findings requiring changes. Recommendation: ready to merge.

crueber referenced this issue

2026-09-04 20:05:54 +00:00

issue comment reactions - #31 isn't complete #42

crueber closed this issue

2026-09-04 20:06:08 +00:00

crueber commented

2026-09-04 20:06:09 +00:00

Author

Owner

Fixed by PR #53 (review clean; 227/227 node tests), merged. Closing.

crueber referenced this issue from a commit

2026-09-04 20:06:09 +00:00