fix(dm): share-invite-via-DM picker no longer hangs at "generating" (Bug #2)#268
Merged
Conversation
…Bug #2) DmThreadModalBody's use_effect that drains DM_DRAFT deferred the signal clear via crate::util::defer(...). The effect subscribes to DM_DRAFT AND to the local draft signal (via draft.read()), then calls draft.set() inside the effect. Because Dioxus re-renders happen on the next microtask but defer() schedules via setTimeout(0) (a macrotask), the re-fired effect saw the still-Some DM_DRAFT, appended body to the merged draft again, and looped forever — locking up the tab until the user force-closed it. Matches Ivvor's repro: pick a target room → modal goes to "Generating…" (the picker's deferred state-flip lands first, opening the DM thread modal which then hangs on its first effect run). The deferred-clear pattern was already documented as forbidden in AGENTS.md: "Never defer signal clears in use_effect" — same rule that the invite-click interceptor at app.rs:124 calls out explicitly. This PR aligns DM_DRAFT with that pattern: - Clear DM_DRAFT synchronously BEFORE draft.set(). - Read draft via .peek() (not .read()) so the effect never subscribes to its own writes — defence in depth. - Extract the merge logic to a pure helper `merge_invite_into_draft` with 5 unit tests, including a regression pin (`merge_invite_into_draft_is_not_self_stable_without_external_clear`) that asserts the merge GROWS on re-application — the synchronous DM_DRAFT clear is the only thing preventing #267's growth-then-hang pattern. [AI-assisted - Claude] Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
8 tasks
sanity
added a commit
that referenced
this pull request
May 17, 2026
…on (Bug #3 PR A) (#270) * fix(room-contract): accept private messages at any known secret version (Bug #3 PR A) Problem ------- Ivvor (2026-05-17 Matrix): "Two invitees processed invite and room opened, but no messages shown. Only room owner can send messages, but they don't show up, even as encrypted, to other members. Owner is on secret #3. The other members are on secret #0." Root cause ---------- `MessagesV1::apply_delta` enforced two checks that, together, paralysed the room as soon as the owner's local `current_version` got ahead of any invitee's: 1. `secret_version == current_version` (strict). Any private message encrypted at v_new arriving while the invitee was still at v_old caused `apply_delta` to return `Err`, the composable macro short-circuited via `?`, and the ENTIRE `ChatRoomStateV1Delta` was dropped — including the message itself, membership updates, and any in-band secrets-delta in the same payload. 2. `has_complete_distribution(members)` had to be true for the current version. A single member missing a blob froze the entire room for messages with no recovery path. Compounding this, `RoomSecretsV1::apply_delta` mutated `self.versions` before running later checks. If any later check failed, the half-mutated state survived and broke CRDT convergence: re-applying the same failing delta would now succeed because the version was already there, but `current_version` / `encrypted_secrets` / pruning never ran. Approach -------- Three changes, all in `common/`: 1. `MessagesV1::apply_delta`: accept any `secret_version` that has a corresponding signed record in `parent_state.secrets.versions`. Author safety is still enforced by `MessagesV1::verify`'s member-or-owner signature check, and post-apply ban-sweep in `ChatRoomStateV1::post_apply_cleanup` purges DMs from banned authors. A message at a fabricated version (no signed record) is still rejected (defense in depth). 2. Remove the `has_complete_distribution` gate from `MessagesV1::apply_delta`. A single member's missing blob no longer freezes the room. 3. Make `RoomSecretsV1::apply_delta` transactional: stage all changes on a `working` clone of `self`, then commit (`*self = working`) only if every check passes. The composable `apply_delta` is now all-or-nothing for the secrets sub-state. Migration --------- Adds legacy_delegates.toml entry V20 capturing the pre-change chat delegate key (code_hash = 05a96a22b12f548d51a9d785c374790f6dcb22986e0d98e9f0dc023ffbaa4ca0, delegate_key = 49b626c43a0b0bf9a8506a5266c690afdebc1dc6ce8a7d6d9397b8b1f1b21a4d) so users' room data migrates to the new key on next load. Testing ------- Four new regression tests in `common/tests/private_room_test.rs`: - `message_at_older_or_newer_known_secret_version_is_accepted`: invitee at `current_version=0` with v0+v1 signed records accepts a message at v1 (Ivvor's repro). Defense-in-depth: message at v99 (no signed record) is still rejected. - `single_member_missing_blob_does_not_freeze_room`: Alice/Bob members, Bob missing v1 blob — Alice's v1 message is still accepted (no full-distribution gate). - `secrets_apply_delta_is_transactional_on_failure`: bad delta leaves `RoomSecretsV1` byte-identical to pre-call state. - `delta_with_rotation_plus_message_at_new_version_applies_atomically`: combined rotation+message delta applies cleanly (regression guard for the happy path). I verified each test fails on `1d150125` (PR #268 merge) without the fix and passes with it. All 251 river-core tests and the full `cargo make test` suite pass. This is part 1 of 2 for Bug #3. PR B will follow with the UI back-fill path so previously-rejected messages re-sync after the contract validation is relaxed, plus the #110 prune-cleanup fix. [AI-assisted - Claude] Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * docs(room-contract): document Bug #3 PR A trade-off for Codex review Codex review (2026-05-17) flagged that the relaxation in PR A allows a member with a stale client to send a message at an older `secret_version` after the room has rotated, and members previously holding that older secret (e.g. banned members) could decrypt it. Add a comment block explaining why we accept this trade-off: - Banned members already hold the plaintext of every message sent during the old version's tenure, so the marginal exposure is small and bounded by how quickly senders catch up. - The alternative (`secret_version == current_version`) is the pre-fix rule that produced Bug #3 in the first place — receivers whose state lagged the sender's `current_version` dropped every message, including legitimate ones. - Confidentiality of post-rotation messages is properly enforced at the SENDER, not the contract. PR B will add the UI back-fill so stragglers rotate forward promptly. Comment-only change — no WASM hash change. [AI-assisted - Claude] Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
sanity
added a commit
that referenced
this pull request
May 17, 2026
…publish Publishes the merged tip of main containing all 5 bug fixes: - #263 (#255): legacy delegate cursor overwrite - #268 (#268, Bug #2): share-invite-via-DM picker hang - #269 (#269, Bug #1): DM to inactive-but-invited members - #270 (Bug #3 PR A): room-contract accepts msgs at any known secret version - #272 (Bug #3 PR B): UI rotate back-fill + PUT join_event + #110 prune exemption - #273 (#273, Bug #5): WebSocket indicator on no-rooms screen river-core 0.1.8 published to crates.io (workspace version bump). riverctl 0.1.58 published to crates.io alongside the UI publish (addresses Bug #4 — riverctl now matches the deployed room contract WASM). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Ivvor on Matrix (2026-05-17): "Share Invite" option to member doesn't work. It allows the target room to be selected but then goes to "Generating" and the browser tab locks up, eventually requiring it to be closed. Tested on both members with and without an existing DM channel active. Same failure on Chrome and Firefox.
Root cause:
DmThreadModalBody'suse_effectthat drainsDM_DRAFTdeferred the signal clear viacrate::util::defer(...). The effect subscribes toDM_DRAFTAND (viadraft.read()) to the localdraftsignal, then callsdraft.set(merged)inside the effect. Because Dioxus re-renders happen on the next microtask butdefer()schedules viasetTimeout(0)(a macrotask), the re-fired effect saw the still-SomeDM_DRAFT, appendedbodyto the merged draft again, and looped forever — burning CPU until the user closed the tab.This matches Ivvor's repro exactly: pick a target room → picker's deferred state-flip lands first, opening the DM thread modal → that modal mounts → first
use_effectrun starts the infinite loop → tab hangs at the "Generating…" state shown by the still-open picker behind it.The deferred-clear pattern was already documented as forbidden in AGENTS.md "Dioxus WASM Signal Safety Rules" → "Never defer signal clears in use_effect". The same rule was called out explicitly in
app.rs:124for the invite-click interceptor. We just forgot to apply it here.Approach
Align
DM_DRAFTwith the rule:DM_DRAFTsynchronously BEFOREdraft.set()— kills the re-fire loop at its source. Same shape as the invite-click interceptor inapp.rs.draftvia.peek()(not.read()) so the effect never subscribes to its own writes — defence in depth.merge_invite_into_draftso the regression can be pinned without touching Dioxus signal subscription wiring.Considered alternatives:
.peek()change, keepingdefer(): insufficient — the effect still subscribes toDM_DRAFT, and the defer-clear still loses the synchronous-clear race against Dioxus's microtask re-render.The fix is the minimal change that satisfies the project rule.
Testing
New unit tests in
dm_thread_modal::tests:merge_invite_into_draft_replaces_empty— empty draft → body becomes draftmerge_invite_into_draft_replaces_whitespace_only— whitespace-only also counts as emptymerge_invite_into_draft_appends_after_user_text— user text preserved, body appended after blank linemerge_invite_into_draft_trims_trailing_whitespace_before_appending— no double-newlines from trailing whitespacemerge_invite_into_draft_is_not_self_stable_without_external_clear— regression pin for bug(dm): same-second inbound DM stays hidden after hide cutoff #267: asserts the merge GROWS on re-application, documenting that the synchronousDM_DRAFTclear is the only thing preventing the hang.Full workspace tests pass (
cargo make test): 129 UI tests + 24 chat-delegate tests + room-contract / web-container / common tests.cargo check -p river-ui --target wasm32-unknown-unknown --features no-syncclean.Clippy: no new warnings (5 pre-existing warnings in unrelated files:
app.rsunused import,room_synchronizer.rscomplex-type,conversation.rsclone-on-copy — all onmain).Test plan
main: pick a target room → tab hangs (parent agent's playwright-MCP verification, see report)[AI-assisted - Claude]