Skip to content

fix(ui): defer event-handler global-signal writes; retain traces for flakes - #545

Merged
sanity merged 3 commits into
mainfrom
fix-538-539
Jul 29, 2026
Merged

fix(ui): defer event-handler global-signal writes; retain traces for flakes#545
sanity merged 3 commits into
mainfrom
fix-538-539

Conversation

@sanity

@sanity sanity commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Problem

Two follow-ups from #536.

#539 — signal-safety violation. Seven event handlers across four files
mutated a GlobalSignal directly inside onclick:

file handlers
room_list.rs open the create-room modal
create_room_modal.rs backdrop, Cancel
edit_room_modal.rs backdrop, close
receive_invitation_modal.rs Retry, Dismiss

.claude/rules/dioxus-signal-safety.md names this exact shape as wrong: a
signal write's Drop fires subscriber notifications synchronously, and Firefox
mobile runs them during Drop, so a memo doing try_read() on the same signal
can hit a re-entrant RefCell already borrowed panic. defer also supplies
the Dioxus runtime + root scope a bare handler lacks. notification_modal.rs
already did this correctly — this makes the rest match it.

#538 — a flake that destroyed its own evidence. retries: 2 means an
intermittent failure is reported as "flaky" and the job still goes green, and
the artifact upload was gated on if: failure(). So a flake left nothing
behind but the test's name.

Approach

#539 — wrap all seven in crate::util::defer. The Dismiss handler needed
care: its two statements are order-dependent and
dismiss_invitation_persistently itself writes a signal, so both go inside the
defer (code after a defer() runs BEFORE the deferred closure), and it needs a
clone because an onclick is FnMut and the invitation is not Copy.

Guarded by a source scan, not per-site pins: the deferred and undeferred
shapes are indistinguishable from the outside — the modal closes either way —
and pinning seven sites would not stop the eighth.

#538 — I could not reproduce it, and the hypothesis I filed it with is
wrong.
I am deliberately not shipping a speculative fix.

I filed it guessing "timeout under parallel load". Measuring the phases on a
machine at load average 86 (where it was originally seen):

phase time budget
first boot → .app-root ~1.0s 30s
→ modal visible ~40ms 15s
reload boot → .app-root ~0.8s 30s
→ modal visible ~28ms 15s

That is a ~500x margin on the assertion that actually failed, so a timeout
does not explain it. Nor is it reproducible: 58 runs all passed — 6
standalone, 12 through Playwright at 6 workers, and 40 across concurrent
browser contexts with generous timeouts and page-error capture, which would
have caught a genuine "modal never appears" race.

Bumping the timeout would have been the obvious move and would have been
cargo-culting a fix onto a disproven theory. What this PR fixes instead is the
reason the flake was undiagnosable: keep a trace on-first-retry, and upload
test-results/ even when the job goes green.

#538 therefore stays open — this makes the next occurrence produce a real
artifact rather than another guess.

Testing

  • cargo test -p river-ui --bins781 passed (780 + the new scan).
  • Full Playwright suite — 682 passed, 23 skipped, 0 flaky, across all five
    projects. Deferring modal open/close changes their timing, so this was the
    real risk; the create-room, edit-room, receive-invitation, notification and
    rooms-loading specs all pass.
  • cargo fmt clean. No delegate/contract WASM rebuilt or committed.

Both parser branches of the new scan are mutation-tested. Un-deferring a
braced handler makes it fail with edit_room_modal.rs:381; rewriting one as an
expression-bodied handler (onclick: move |_| MODAL.write().show = true,)
makes it fail with room_list.rs:395. The first draft of the scan had a false
positive — for a brace-less handler it grabbed the next unrelated { block —
which the mutation test caught and which is now fixed and commented.

The trace change is verified too: a deliberately failing probe produced
test-results/<test>-retry1/trace.zip.

Closes #539

[AI-assisted - Claude]

sanity and others added 3 commits July 29, 2026 12:51
Closes #539.

Seven event handlers across four files mutated a `GlobalSignal` directly
inside `onclick`, which `.claude/rules/dioxus-signal-safety.md` names
explicitly as the wrong shape:

  room_list.rs                    open the create-room modal
  create_room_modal.rs            backdrop + Cancel
  edit_room_modal.rs              backdrop + close
  receive_invitation_modal.rs     Retry + Dismiss

A signal write's Drop fires subscriber notifications synchronously, and
Firefox mobile runs them during Drop, so a memo doing `try_read()` on the same
signal can hit a re-entrant `RefCell already borrowed` panic; `defer` also
supplies the Dioxus runtime + root scope a bare handler lacks.
`notification_modal.rs` already did this correctly and is the shape followed
here.

The Dismiss handler needed care: its two statements are order-dependent and
`dismiss_invitation_persistently` itself writes a signal, so BOTH go inside
the defer (code after a `defer()` runs BEFORE the deferred closure). It also
needs a clone, because an `onclick` is `FnMut` and the invitation is not
`Copy`.

Guarded by a source scan rather than per-site pins, since the deferred and
undeferred shapes are indistinguishable from the outside — the modal closes
either way — and pinning seven sites would not stop the eighth. Both of its
parser branches are mutation-tested: un-deferring a braced handler and
rewriting one as an expression-bodied handler each make it fail with exactly
the offending file:line.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QaMA3cqXTBiTNmPYcMeZsi
Re #538.

`retries: 2` means an intermittent failure is reported as "flaky" and the run
still goes green, and the artifact upload was gated on `if: failure()` — so a
flake left nothing behind but the test's name. That is how #538 ended up with
a plausible-but-wrong hypothesis attached to it.

Investigating #538 disproved that hypothesis. It was filed as a timeout under
parallel load; measuring the phases (on a machine at load average 86, which is
where it was originally seen) gives:

  first boot ~1.0s, first modal ~40ms, reload boot ~0.8s, reload modal ~28ms

against a 15s modal budget and a 30s app-root budget — a margin of roughly
500x on the assertion that failed. It is also not reproducible: 58 runs (6
standalone, 12 via Playwright at 6 workers, 40 across concurrent contexts with
generous timeouts and error capture) all passed.

So the cause remains unknown, and no speculative change is made to the test.
What IS fixed is that the next occurrence will be diagnosable: keep a trace
`on-first-retry` and upload test-results even when the job goes green. Both
verified — a deliberately failing probe produced trace.zip under
test-results/<test>-retry1/.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QaMA3cqXTBiTNmPYcMeZsi
Review pass on the scan found a hole: it only inspected INLINE closures, so a
handler passed by name (`onclick: handle_close,`) was invisible to it. There
are 20+ such handlers across the UI, which is a big enough gap to drive the
next violation through.

For each name used as an event handler, find its `let <name> = move |…| { … }`
binding in the same file and scan that body too. No current violations — every
named handler that writes a global signal already defers — so this adds no
fixes, only coverage.

Mutation-tested like the other two branches: removing the `defer` from
`dm_thread_modal.rs`'s `close` handler makes it fail with
`dm_thread_modal.rs:411 (handler `close`)`. Offenders are deduped, since a
named handler used at several call sites is one defect, not N.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QaMA3cqXTBiTNmPYcMeZsi
@sanity

sanity commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

Review (Full tier)

Tier: Full — the diff touches .github/workflows/build.yml, which
~/.claude/rules/multi-model-review.md puts on the always-Full list
(CI configuration), and it changes signal-handling semantics across four UI
files.

Lenses run sequentially in-session (no pr-review skill in this session,
subagents not requested): code-first, testing, skeptical,
big-picture, plus the change-specific lens, defer-ordering semantics.
External non-Claude reviewers not invoked — opt-in per the rule.

Finding, fixed in 9658c2f

The new source scan had a hole (testing lens). It only inspected INLINE
closures, so a handler passed by name — onclick: handle_close, — was
invisible to it. There are 20+ such handlers in the UI. A guard that misses the
most common alternative spelling is not a guard.

Extended to resolve each named handler to its let <name> = move |…| { … }
binding and scan that body. Checked all of them: no current violations, so
this adds coverage rather than fixes. Mutation-tested — removing the defer
from dm_thread_modal.rs's close makes it fail with
dm_thread_modal.rs:411 (handler `close`).

An earlier draft also had a false positive: for a brace-less handler
(onclick: move |_| show_confirm.set(true),) it took "the next {", swallowing
an unrelated block. Caught by mutation testing, fixed by parsing the closure
properly, and both body shapes now have their own mutation test.

Checked, no change needed

  • Defer ordering. defer() runs its closure on a later tick, so code after
    it runs FIRST. Only one of the seven sites has more than one statement — the
    Dismiss handler — and both of its statements go inside the defer, preserving
    order. It also needs a clone(), since an onclick is FnMut and the
    invitation is not Copy. The other six are single writes with no captures.
  • dismiss_invitation_persistently writes a signal too (invitation.set(None)).
    Deferring the whole handler fixes that write as well, rather than leaving it
    as an undeferred write one call deeper.
  • Local use_signal handles are deliberately NOT flagged. They have no
    external subscribers, and the existing copy buttons set them directly in
    onclick. Flagging them would fight the codebase for no safety gain.
  • Deferring the create-room modal's OPEN (not just closes) — show = true
    is idempotent, so a double click is harmless, and the create-room specs pass.
  • if: always() on the artifact upload — pairs with
    if-no-files-found: ignore, so a clean run is a no-op rather than a warning.

On #538 specifically

I filed #538 myself with a timeout hypothesis. That hypothesis is wrong, and
this PR says so rather than shipping a fix built on it. Measured margin is ~500x
on the assertion that failed, and 58 runs could not reproduce it. Bumping the
timeout would have looked like a fix and fixed nothing. #538 stays open with the
measurements recorded; the trace change means the next occurrence produces an
artifact instead of another guess.

Verification

  • cargo test -p river-ui --bins — 781 passed.
  • Full Playwright suite — 682 passed, 23 skipped, 0 flaky, all five projects.
    Deferring modal open/close changes their timing, so this was the real risk.
  • All three scan branches (braced, expression-bodied, named) mutation-tested;
    each fails with the exact offending file:line.
  • Source confirmed free of mutation residue before commit.

[AI-assisted - Claude]

@sanity
sanity marked this pull request as ready for review July 29, 2026 17:56
@sanity
sanity merged commit 37dc0e2 into main Jul 29, 2026
6 checks passed
sanity added a commit that referenced this pull request Jul 29, 2026
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QaMA3cqXTBiTNmPYcMeZsi
@sanity
sanity deleted the fix-538-539 branch July 29, 2026 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

edit_room_modal closes via an undeferred GlobalSignal write in onclick (dioxus-signal-safety violation)

1 participant