Fix #573: collapsing a jumped-to thread card clears its flash #577

Merged

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

2026-09-15 14:47:16 +00:00

crueber commented

2026-09-15 14:45:18 +00:00

Owner

Root cause (web/src/pages/Pull.jsx): ThreadCard open = () => getOpen() || flashed() force-expands while flashed; jumpToThread set flashTid with NOTHING ever clearing it — collapse looked dead, emerald outline stuck forever.

Fix:

Collapse/expand toggle clears the page-level flash for that tid first (new ThreadCard onCollapse prop → page-level targeted clearFlashTid (cur === tid ? null : cur), threaded through DiffFile to inline + file-end cards), so open() and the outline return to tracking getOpen().
Forced-open while flashed preserved for the jump itself; second jump overwrites (moves flash); unflashed cards unchanged (targeted no-op + optional-chained call).
Resolve/frame decision (implementer's call, minimal but coherent): resolve/unresolve + collab-stream thread frames deliberately do NOT clear — a resolve-triggered reload keeps the highlight on the acted card. Pinned in tests + doc amendment.
Shared-lifecycle note for #575: the clear path lands here once as the reusable onCollapse/clearFlashTid shape; set paths (jumpToThread/jumpToStaged) unchanged. StagedCard has no collapse control in the current tree → no clear wiring; same clearFlash pattern applies when one lands.
Anchor/drift-hash surface untouched (buildAnchor/freshnessOf/sortThreadsForIndex byte-identical).

Closes #573.

Verification: new web/test/unit/thread-flash-clear-573.test.js 13/13; related (572/567/546/554/555/557/560/566) 117/117; full-minus-smoke 1406/1406 (smoke needs live server, pre-existing); vite build + esbuild SDK green (web/dist/.keep restored); go vet clean. No new deps; Tailwind-only, canonical classes; law-12 doc amendment in docs/go/12_web_ui.md same commit. 390px reasoned (behavior-only, no layout delta).

Root cause (web/src/pages/Pull.jsx): ThreadCard `open = () => getOpen() || flashed()` force-expands while flashed; jumpToThread set flashTid with NOTHING ever clearing it — collapse looked dead, emerald outline stuck forever. Fix: - Collapse/expand toggle clears the page-level flash for that tid first (new ThreadCard `onCollapse` prop → page-level targeted `clearFlashTid` (`cur === tid ? null : cur`), threaded through DiffFile to inline + file-end cards), so open() and the outline return to tracking getOpen(). - Forced-open while flashed preserved for the jump itself; second jump overwrites (moves flash); unflashed cards unchanged (targeted no-op + optional-chained call). - Resolve/frame decision (implementer's call, minimal but coherent): resolve/unresolve + collab-stream thread frames deliberately do NOT clear — a resolve-triggered reload keeps the highlight on the acted card. Pinned in tests + doc amendment. - Shared-lifecycle note for #575: the clear path lands here once as the reusable `onCollapse`/`clearFlashTid` shape; set paths (jumpToThread/jumpToStaged) unchanged. StagedCard has no collapse control in the current tree → no clear wiring; same clearFlash pattern applies when one lands. - Anchor/drift-hash surface untouched (buildAnchor/freshnessOf/sortThreadsForIndex byte-identical). Closes #573. Verification: new web/test/unit/thread-flash-clear-573.test.js 13/13; related (572/567/546/554/555/557/560/566) 117/117; full-minus-smoke 1406/1406 (smoke needs live server, pre-existing); vite build + esbuild SDK green (web/dist/.keep restored); go vet clean. No new deps; Tailwind-only, canonical classes; law-12 doc amendment in docs/go/12_web_ui.md same commit. 390px reasoned (behavior-only, no layout delta).

crueber added 1 commit

2026-09-15 14:45:18 +00:00

FIXED (Forgejo #573 ) — collapsing a jumped-to thread card clears its flash ea590da857

ThreadCard open() force-expands while flashed but jumpToThread set
flashTid with no clear path, so collapse looked dead and the emerald
outline stuck. The collapse/expand toggle now clears the page-level
flash for its tid first (onCollapse -> targeted clearFlashTid, threaded
through DiffFile to inline + file-end cards). Forced-open-while-flashed
preserved; resolve/unresolve + collab frames deliberately do not clear;
StagedCard has no collapse control (no wiring). Shared clearFlash(tid)
lifecycle lands once for #575 reuse; set paths unchanged; no
anchor/hash surface touched.

crueber commented

2026-09-15 14:46:59 +00:00

Author

Owner

Independent review — APPROVED (no changes made, no fix commit; worktree clean).

Verified against the branch (ea590da) and issue #573 acceptance, in /tmp/walhub-573:

Order (collapse toggle): Pull.jsx:776 onClick={() => { props.onCollapse?.(t().tid); setOpen(!getOpen()); }} — onCollapse runs BEFORE the setOpen toggle, both synchronous Solid signals, so flash can never re-assert between them. Post-clear, open() and the outline fall back to tracking getOpen(): pill-jump then collapse → collapsed + outline gone. ✔
clearFlashTid (Pull.jsx:1054): setFlashTid((cur) => (cur === tid ? null : cur)) — targeted, only the matching tid clears; collapsing an unflashed card is a no-op that never steals another card's flash. Single-flash model preserved; second jump overwrites via unchanged jumpToThread (moves flash). ✔
DiffFile threading reaches BOTH card sites: inline (Pull.jsx:639) and file-end/unplaced incl. drifted/outdated collapsed cards (Pull.jsx:668) both pass onCollapse={props.onCollapse}, and the page wires onCollapse={clearFlashTid} into DiffFile (Pull.jsx:1311). No other ThreadCard call sites exist in the file. ✔
StagedCard-no-collapse claim TRUE in current tree: StagedCard (Pull.jsx:688-715) has no collapse/expand control and no onCollapse/clearFlash wiring; staged flash-outline idiom untouched. ✔
Resolve/frame decision coherent + pinned: resolve toggle (Pull.jsx:745-753) never touches flash; collab-stream subscription (Pull.jsx:1191) is invalidate-only. A resolve-triggered reload keeps the highlight on the acted card — matches the issue's minimum-bar allowance and is pinned in tests. ✔
Forced-open-while-flashed preserved: open = () => getOpen() || flashed() + emerald outline both intact; the jump still force-reveals, only collapse clears. ✔
#575 reuse: the clear path lands here once as the reusable onCollapse/clearFlashTid shape; set paths (jumpToThread/jumpToStaged) byte-identical. Advisory for #575 (non-blocking): if it adds a card-root click-to-flash, it must stopPropagation / ignore clicks from the collapse (and resolve) controls — otherwise the button's clear followed by the bubbled root set would re-flash on the same gesture and regress this fix. ✔
Law 12: docs/go/12_web_ui.md carries the #573 amendment in the same commit. ✔
Law 1: web/package.json runtime deps still exactly solid-js + @solidjs/router + marked + dompurify; no

Independent review — APPROVED (no changes made, no fix commit; worktree clean). Verified against the branch (ea590da) and issue #573 acceptance, in /tmp/walhub-573: 1. Order (collapse toggle): Pull.jsx:776 `onClick={() => { props.onCollapse?.(t().tid); setOpen(!getOpen()); }}` — onCollapse runs BEFORE the setOpen toggle, both synchronous Solid signals, so flash can never re-assert between them. Post-clear, open() and the outline fall back to tracking getOpen(): pill-jump then collapse → collapsed + outline gone. ✔ 2. clearFlashTid (Pull.jsx:1054): `setFlashTid((cur) => (cur === tid ? null : cur))` — targeted, only the matching tid clears; collapsing an unflashed card is a no-op that never steals another card's flash. Single-flash model preserved; second jump overwrites via unchanged jumpToThread (moves flash). ✔ 3. DiffFile threading reaches BOTH card sites: inline (Pull.jsx:639) and file-end/unplaced incl. drifted/outdated collapsed cards (Pull.jsx:668) both pass `onCollapse={props.onCollapse}`, and the page wires `onCollapse={clearFlashTid}` into DiffFile (Pull.jsx:1311). No other ThreadCard call sites exist in the file. ✔ 4. StagedCard-no-collapse claim TRUE in current tree: StagedCard (Pull.jsx:688-715) has no collapse/expand control and no onCollapse/clearFlash wiring; staged flash-outline idiom untouched. ✔ 5. Resolve/frame decision coherent + pinned: resolve toggle (Pull.jsx:745-753) never touches flash; collab-stream subscription (Pull.jsx:1191) is invalidate-only. A resolve-triggered reload keeps the highlight on the acted card — matches the issue's minimum-bar allowance and is pinned in tests. ✔ 6. Forced-open-while-flashed preserved: `open = () => getOpen() || flashed()` + emerald outline both intact; the jump still force-reveals, only collapse clears. ✔ 7. #575 reuse: the clear path lands here once as the reusable onCollapse/clearFlashTid shape; set paths (jumpToThread/jumpToStaged) byte-identical. Advisory for #575 (non-blocking): if it adds a card-root click-to-flash, it must stopPropagation / ignore clicks from the collapse (and resolve) controls — otherwise the button's clear followed by the bubbled root set would re-flash on the same gesture and regress this fix. ✔ 8. Law 12: docs/go/12_web_ui.md carries the #573 amendment in the same commit. ✔ 9. Law 1: web/package.json runtime deps still exactly solid-js + @solidjs/router + marked + dompurify; no <style>, Tailwind-only, canonical classes. go vet clean. ✔ 10. Tests fail pre-fix: new web/test/unit/thread-flash-clear-573.test.js is 13/13 on the branch; with Pull.jsx temporarily reverted to origin/main it fails 4/13 (toggle-clear, inline+file-end threading, targeted-clear, unflashed optional-chain pins) — the fix is what makes them pass. Related suites (572/568/567) 54/54 green here. Small-defect pass: none found — no direct fixes applied, nothing to commit/push. Verdict: APPROVED — meets all four acceptance criteria with no scope creep (anchor/drift-hash surface untouched).

crueber merged commit 6c9ac0d697 into main

2026-09-15 14:47:16 +00:00

crueber referenced this pull request from a commit

2026-09-15 14:47:16 +00:00

Merge pull request 'Fix #573: collapsing a jumped-to thread card clears its flash' (#577) from fix/issue-573 into main

crueber referenced this pull request

2026-09-15 14:47:17 +00:00

Collapsing a jumped-to thread card leaves it highlighted and refuses to collapse #573