Pre-existing: some local signal mutations in conversation.rs aren't wrapped in defer()

[sanity]

Context

While reviewing PR #403 (touch message actions), a reviewer noted a pre-existing inconsistency: .claude/rules/dioxus-signal-safety.md requires signal mutations in synchronous event handlers to be wrapped in crate::util::defer() (Firefox-mobile re-entrant RefCell already borrowed / empty-scope panics), but several local (use_signal) mutations in ui/src/components/conversation.rs set subscribed signals directly in onclick/oninput handlers without deferring.

Concrete example the reviewer found (NOT introduced by #403):

• The inline reaction-add ("+") button handler sets picker_show_above and open_emoji_picker directly (around ui/src/components/conversation.rs:3004). Both are subscribed in render (picker_show_above.read(), the inline-picker gate), so on Firefox mobile a subscriber notification can fire from the write's Drop.

Other same-shape sites exist (e.g. editing_message.set, edit_text.set, open_emoji_picker.set, replying_to.set), all pre-existing.

PR #403 wrapped every mutation it added (the kebab menu handlers, and the IntersectionObserver is_at_bottom set that #403 made reactive), so the new code complies. This issue tracks bringing the pre-existing local-signal call sites into line.

Scope

Audit ui/src/components/conversation.rs (and siblings) for signal mutations in synchronous handlers that aren't deferred, and wrap them per the rule. Verify no timing regressions (defer runs on setTimeout(0)). Not urgent — these have shipped without widespread reports — but they are latent crash paths of a known class.

[AI-assisted - Claude]

[sanity]

Related item found in the PR #403 review (round 5): open_emoji_picker is also a per-MessageGroupComponent use_signal, so two reaction pickers can be open at once across message groups (a later message's z-50 "+"/kebab paints above an earlier picker's z-40 backdrop). PR #403 hoisted open_action_menu to the parent Conversation to make it single-instance; open_emoji_picker should get the same treatment for consistency. Pre-existing (the inline "+" reaction button already exhibits it); out of scope for #403.

[AI-assisted - Claude]

[sanity]

Update: PR #403 (round 7) hoisted open_emoji_picker to Conversation, so the two-pickers-across-groups part is now fixed. This issue remains open for the other pre-existing non-deferred local signal mutations (e.g. editing_message, edit_text, replying_to, the inline "+" picker's picker_show_above/open_emoji_picker writes) that should also go through crate::util::defer().

[AI-assisted - Claude]

[sanity]

Issue Priority Assessment

Generated: 2026-07-24T12:44:51.758195905+00:00
Issue hash: 88432a5a

Value Assessment

Interesting — this confirms the issue is partially stale but mostly still live. Line 3248-3249 (the kebab-menu-triggered inline picker path, added by #403) is already wrapped in defer(), matching the issue's claim that #403 deferred its own new code. But the direct "+" button click path (line 3404-3405) and the editing_message/edit_text/replying_to sites are all still bare .set() calls inside synchronous onclick handlers, unwrapped, exactly as described.

Assessment

Relevance: Still fully relevant. Direct grep confirms editing_message.set() (lines 2600-2601, 2720, 2737, 2770, 2838, 2872, 3063-3064, 3263-3264), replying_to.set() (line 2198), and the inline "+" button's picker_show_above.set()/open_emoji_picker.set() (lines 3404-3405) all remain bare, undeferred mutations inside synchronous onclick/oninput handlers — none have been fixed since the issue was filed. The open_emoji_picker hoisting mentioned in the comments (single-instance across message groups) is a separate concern from deferral and doesn't touch this issue's core ask. One data point favors the issue's own framing: the kebab-menu path that #403 added (lines 3240-3249) is correctly deferred, confirming new code complies while old code doesn't — so the remaining scope is exactly the pre-existing sites the issue lists.

User impact: Low-to-moderate severity, narrow browser scope. This is a Firefox-mobile-specific re-entrant RefCell panic class (per dioxus-signal-safety.md), triggered when a signal write's Drop handler synchronously notifies a subscriber that tries to borrow the same signal. It requires a specific mobile browser plus timing coincidence to manifest as a crash, so it's not a routine failure — but when it hits, it's a hard WASM panic that likely breaks the whole message composer/edit/reaction flow for that user, forcing a reload. The issue itself notes "shipped without widespread reports," suggesting actual incident frequency is low so far.

Strategic alignment: Directly enforces an established, documented project safety rule (dioxus-signal-safety.md) that exists precisely because this exact panic class has bitten River before. Fixing it is mechanical, low-risk, and improves reliability on mobile — consistent with River's goal of being a usable cross-platform chat client, but it's maintenance/hardening work, not a feature or roadmap driver.

Blocking factor: Doesn't block other work. It's isolated to conversation.rs UI internals and doesn't gate any contract, delegate, or protocol change; other PRs can proceed independently.

Urgency: Low-to-moderate. Not user-reported yet, so no fire to put out, but it's a known latent crash class in a heavily-used, frequently-touched file (message editing/reactions/replies), and every new PR touching this file risks copying the same bad pattern (as #403 nearly did before its later review rounds caught it). Worth batching into a single cleanup PR sooner rather than later, but not an emergency.

Cost Assessment

Based on this, here's my cost assessment.

Relevance. Still fully actionable and the code matches the issue's description. conversation.rs (~3500+ lines) currently has a genuine mix: some sites already wrap mutations in crate::util::defer() (e.g. lines 1481, 1585, 1713, 1927, 3133, 3188, 3227, 3247, 3262, 3275), while a long tail of .set() calls on subscribed signals still fire synchronously from onclick/oninput handlers — pending_delete (2192, 2470, 2483, 2493), replying_to (2198), edit_trigger/editing_message/edit_text (2285, 2600-2602, 2720, 2737, 2770, 2838, 2872, 3063-3064), and the inline reaction picker's open_emoji_picker/picker_show_above (3380, 3395, 3404-3405, 3439). So the issue's premise holds and no rethink is needed — it's a mechanical audit-and-wrap job, not a design problem.

Scope and complexity. Single file (conversation.rs), no architectural changes — each fix is wrapping an existing .set() call (or a short sequence of them) in crate::util::defer(move || { ... }), following the pattern already used dozens of times in the same file. The only complexity is the ordering caveat called out in the project's own dioxus-signal-safety.md: any code immediately after a .set() that reads the just-set value must be checked, since defer() runs on setTimeout(0) and execution continues before the closure runs. A few sites chain multiple sets together (e.g. 2600-2602, 3404-3405) and need to move as a unit into one defer() closure rather than being wrapped individually, to avoid interleaving. Since this is a known, previously-applied pattern in the very same file, an implementer can largely copy the existing style.

External dependencies. None — no third-party services, approvals, or waiting on other people.

Risk. Low but not zero. The main regression risk is timing: deferring a signal set delays UI feedback by one tick (e.g. opening the edit box, showing the emoji picker), which is imperceptible normally but could subtly break tests or interactions that assume synchronous state right after a click. The issue explicitly calls out verifying "no timing regressions." Since these are UI-only local signals (not room/network state), a mistake would surface as a visual/interaction glitch, not data corruption.

Testing effort. No dedicated automated test coverage exists for these interaction sequences beyond the Playwright layout suite, which doesn't exercise click-to-edit/react-to-message flows in a way that would catch a defer-related timing bug. Verification is realistically manual: build with example data, click through the edit/reply/react/delete flows (including on a mobile-width viewport, ideally via Playwright MCP or the interactive dev-browser skill) to confirm no regression. Given the many call sites, this is a moderate, careful, mostly-manual QA pass rather than something CI verifies for you. Overall this is a small-to-moderate, low-risk, single-file cleanup task — a few hours of mechanical editing plus a manual click-through verification pass.

---

Generated by issue-prioritizer