feat: in-room DMs UI + riverctl (#243 Phase 2/3) + share-invite-via-DM picker (#252 partial)#244
Conversation
Phase 2/3 of #230. Phase 1 (#240) shipped the DM wire format and the ChatRoomStateV1 fields; this PR adds the two end-user surfaces. common/ - ecies: seal_dm_for_recipient / unseal_dm_from_sender for arbitrary per-message bodies. Fresh ephemeral X25519 keypair + random nonce per call (forward-secret) — explicitly separate from the deterministic encrypt_secret_for_member helper to avoid generalising AES-GCM key reuse on attacker-controlled plaintext. - direct_messages: compose_direct_message / open_direct_message / advance_recipient_purges helpers so the UI and riverctl produce byte-identical wire bytes from one code path. - 6 new ECIES tests + 7 new helper tests through full ComposableState. UI (Phase 2) - New components/direct_messages module with DM thread + inbox modals. - "Send Direct Message" button in the member info modal. - "Direct Messages" button (with unread badge) in the members panel. - Thread modal decrypts inbound DMs on display, composes outbound DMs via the shared compose_direct_message helper, and offers a "Purge thread" action that bumps the recipient's purge envelope. - In-memory DM_LAST_SEEN drives the unread count. riverctl (Phase 3) - New \`riverctl dm send|list|purge\` subcommands, reusing the shared helpers so wire bytes match the UI. - Added send_state_delta() on ApiClient for pre-built deltas. - Enabled river-core "ecies","ecies-randomized" features on cli. - 3 unit tests for hex/index parsing. [AI-assisted - Claude] Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Findings consolidated from 4 internal reviewer agents + Codex CLI. P1 (Codex) — feature-gate `open_direct_message` - `open_direct_message` referenced `crate::ecies::unseal_dm_from_sender` unconditionally, but `ecies` is `#[cfg(feature = "ecies")]`. The room contract crate uses `river-core` without that feature, so building the room contract WASM after the prior commit failed with "could not find `ecies` in the crate root". Gating the helper on `ecies` restores the room-contract build. Verified via `cargo check -p room-contract --target wasm32-unknown-unknown`. High (skeptical) — per-pair cap silent loss - Contract `apply_delta` silently drops messages over `MAX_DM_MESSAGES_PER_PAIR` (100). Before this commit the CLI printed "DM sent" and the UI cleared the composer for messages that landed nowhere. Added `pair_message_count` helper in `common/src/room_state/direct_messages.rs` and a pre-flight check in both `cli/src/commands/dm.rs::execute_send` and `ui/src/components/direct_messages/dm_thread_modal.rs::send`. Surfaces a user-visible error instead of silent loss. Covered by new test `pair_message_count_only_counts_the_ordered_pair`. High (skeptical) — "Purge thread" stale token list - Token list was captured from the memo at render time and moved into the click closure, so any DM arriving between render and click survived the purge. Re-read tokens inside the click closure against fresh ROOMS state. Important (code-first) — `dm purge <index>` UX trap - `dm list --with X --limit N` indices did not match `dm purge <idx>` (which indexed the unfiltered/unlimited inbox). Easy to silently purge the wrong message. Dropped the index form; `dm purge` is now hex-token only. `dm list` already prints the token under each inbound DM, so the workflow is "look up in dm list, copy hex, pass to purge". Help text + Purge command doc explain the change. Also added an "already purged" sanity check so purging a tombstoned token errors rather than silently bumping the version. Should-fix - (big-picture) `count_total_unread_messages` in `ui/src/components/app/document_title.rs` now sums inbound-DM unread alongside room-message unread, so the tab-title badge tracks DMs the same way the inbox panel does. - (code-first / skeptical) `DmInboxModal` sort key switched from a formatted `"%Y-%m-%d %H:%M"` string to the underlying `u64` timestamp; empty threads now sort last regardless of nickname, and same-minute ties order deterministically. - (code-first) Trimmed dead `self_id == owner_id && peer == owner_id` disjunct — already covered by `self_id == peer`. - (big-picture) Updated `AGENTS.md` to reflect that Phase 2+3 shipped in #244 and to point at the new module / helpers. Deferred (filed mentally as follow-ups) - (skeptical) `ApiClient::send_state_delta` recv misattribution: pre- existing pattern across all `api.rs` callers; out of scope here. - (skeptical) Send-to-newly-banned-recipient TOCTOU: would require a fetch-after-send round trip; documented limitation, not a regression. - (skeptical) `advance_recipient_purges` with empty `new_tokens`: low risk, not exploitable; the new "already purged" check in `execute_purge` prevents the CLI from triggering it. [AI-assisted - Claude] Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Multi-model review pass complete (4 internal reviewer agents + Codex CLI). Findings addressed in 67ca488: Blocking — fixed
Should-fix — fixed
Deferred (with reasoning)
Tests + builds + clippy on `river-core`/`riverctl` and `cargo check -p room-contract --target wasm32-unknown-unknown` + UI release build all green. [AI-assisted - Claude] |
`unix_now()` in `dm_thread_modal.rs::send` called `SystemTime::now()`,
which panics at runtime on `wasm32-unknown-unknown` with "time not
implemented on this platform" because the rustc std stub for Wasm time
is unreachable. The unit tests pass because they run on native; the
problem only surfaces inside the published UI WASM.
Caught by manual end-to-end test against a published test contract:
sending a DM from the UI composer panicked the WASM thread, crashing
the page. Switched to `crate::util::get_current_system_time()`, which
already provides a `wasm_bindgen` `Date.now()` shim on Wasm and falls
through to `SystemTime::now()` on native — same helper the existing
room-message send path uses.
Verified by re-publishing the test contract and exercising the full
DM round trip:
- Alice (browser UI) creates room "DM Test Room"
- Alice invites Bob; Bob accepts via `riverctl invite accept`
- Bob sends DM via `riverctl dm send` — Alice's UI shows "Direct
Messages 1" badge, opens inbox, opens thread, plaintext "hello
alice, this is bob via riverctl" decrypts on display
- Alice replies "hi bob, alice here from the browser UI" via UI
composer — Bob's `riverctl dm list` shows the decrypted reply
- Bob purges Alice's DM via `riverctl dm purge <hex_token>` — Bob's
next `dm list` no longer shows it
- Alice clicks UI "Purge thread" — Alice's thread modal goes empty
("No messages yet. Say hello!"), Bob's DM tombstoned in her view
So the round-trip is wire-format compatible across the
`river-ui` (Wasm) and `riverctl` (native) consumers in both
directions, as designed.
[AI-assisted - Claude]
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Manual end-to-end verification via Playwright caught a runtime-only WASM bug — fixed in 88d5820. Test setup: Published the branch under a separate test web-container keypair (contract `yeWSLGJC2BLuuCJ1uhRTM673evsRVjrcB9jCiZPbp89`, generated via the existing `cargo make generate-test-keys` / `publish-river-test` flow) so production River (`raAqMhMG7KUpXBU2SxgCQ3Vh4PYjttxdSWd9ftV7RLv`) was unaffected. Bug found: `dm_thread_modal.rs::unix_now` was using `SystemTime::now()`, which panics at runtime on `wasm32-unknown-unknown` ("time not implemented on this platform"). Unit tests pass because they run on native. Now routed through the existing `crate::util::get_current_system_time` helper that wraps `Date.now()` via `wasm_bindgen`. End-to-end DM round trip verified after the fix:
Wire-format symmetry between `river-ui` (Wasm) and `riverctl` (native) confirmed in both directions, as designed. Both consumers go through the shared `compose_direct_message` / `open_direct_message` / `advance_recipient_purges` helpers in `river-core`. Screenshots saved locally; CI is re-running. [AI-assisted - Claude] |
DM bodies were rendered as raw text, so a pasted invite URL (the most useful thing you can DM someone today — see follow-up #252) wasn't clickable. Route the inbound DM body through the same `conversation::message_to_html` path room messages use: - bare URLs become anchors with target="_blank" rel="noopener noreferrer" - markdown `[label](url)` is preserved - URLs inside code spans/blocks are NOT linkified - Freenet web-contract URLs get host-stripped to same-origin paths when the UI is being served from behind a gateway, so an invite URL shared via DM lands on the recipient's gateway, not the sender's - newlines are preserved via the existing hard-break transform Made `conversation::message_to_html` `pub(crate)` so the DM bubble can reuse it. The existing linkify tests in `conversation::tests` (bare URL linkified, code-span URL NOT linkified, markdown link preserved, Freenet URL label shortened, etc.) cover the exact function the DM bubble now calls — no extra test surface needed. Sets up the lightweight half of #252 (cross-room invite-by-DM): the heavier "Share an invite via DM…" UX is still a separate PR. [AI-assisted - Claude] Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Follow-ups from review discussion:
[AI-assisted - Claude] |
Implements #252 ("share invite via DM") and folds in the findings from the multi-model review pass on commits 67ca488 / 88d5820 / e662ea5. # Feature: Share an invite via DM (#252) Click any co-member's name in a room → member-info modal now has a "Share an invite via DM…" button below "Send Direct Message". It opens a small picker listing the *other* rooms the local user is in. Pick one → River generates an invite for that room and drops a pre-composed DM into the current room's thread modal with the invite URL, ready to review and send. The DM body goes through the linkify pass added in e662ea5, so the URL is a clickable anchor on the recipient's side. Wiring: - `INVITE_VIA_DM_PICKER: GlobalSignal<Option<(VerifyingKey, MemberId)>>` - `DM_DRAFT: GlobalSignal<Option<(VerifyingKey, MemberId, String)>>` drained once when `DmThreadModalBody` mounts for the matching pair. - Factored `invite_member_modal::get_invitation_base_url()` to `pub(crate)` so the picker produces byte-identical URLs. # Review fixes ## Codex (P2) — ambiguous-prefix DM recipient `resolve_recipient_vk` in `cli/src/commands/dm.rs` was picking the FIRST prefix match silently, which could route a private DM to the wrong recipient on accidental or malicious prefix collisions. Now collects all candidates and errors if not exactly one matches (listing the ambiguous ids). Added `resolve_recipient_vk_rejects_ambiguous_prefix` test pinning the new behaviour. ## Skeptical (High) — sticky DM unread on reload `DM_LAST_SEEN` is intentionally in-memory only. Without seeding, every reload re-flagged every prior inbound DM as unread until each thread was opened — particularly loud in the tab title. Added `seed_dm_last_seen_from_rooms()` called from `App()` via a once-per-render `use_effect` (function is idempotent + monotonic). On startup it sets `DM_LAST_SEEN[(room, peer)]` to the max inbound timestamp already in state, so unread only counts DMs that arrive afterwards. ## Skeptical (Medium) — markdown-mangled placeholders `"<sent: ciphertext only>"` and `"<unable to decrypt: ...>"` were going through `message_to_html` (introduced in e662ea5), which interpreted the leading `<scheme:` as a markdown autolink and emitted a degraded anchor. Tagged each rendered DM with a `BodyKind` (`Plaintext` or `Placeholder`) and routed placeholders through a plain muted text node, skipping markdown entirely. ## Skeptical (Medium) — cap-check race in deferred apply The UI's per-pair pre-flight cap check ran synchronously but the actual `apply_delta` ran inside `defer()`. A peer-side inbound landing between the two could have pushed the count to the cap before our delta landed; the contract would then silently drop it. Added an in-defer cap recheck under the `ROOMS.with_mut` write-lock, with an `ApplyOutcome` enum routing the four cases (`Applied`, `RoomGone`, `CapHit`, `DeltaFailed`) to user-facing errors. ## Skeptical (Low) — `try_read` on DM_LAST_SEEN `count_unread_dms` was using `.read()` instead of `.try_read()` on a signal that is mutated via `defer()` callbacks. Pattern violates the re-entrant-borrow rule documented in AGENTS.md. Now `try_read`s and returns `0` if borrowed. ## Code-first — multi-paragraph DM rendering DM bubbles dropped `whitespace-pre-wrap` in favour of the markdown hard-break transform but didn't add the `prose prose-sm` wrapper that gives multi-paragraph room messages their inter-paragraph spacing. Added the same wrapper. ## Testing — `disallowed_methods` lint for `SystemTime::now` The 88d5820 incident (panicking UI WASM because `SystemTime::now()` isn't implemented on `wasm32-unknown-unknown`) had no structural guard. Added a workspace `clippy.toml` that bans `std::time::SystemTime::now` with a `reason` pointing at the wasm-safe helper. Clippy isn't in CI today (per `.github/workflows/clippy.yml.disabled`) but a local `cargo clippy --target wasm32-unknown-unknown` will catch a future offender immediately. ## Code-first / big-picture — chores - Dropped the unused `dm-body` CSS class. - `.gitignore` now ignores `dm-*.png` (manual e2e screenshots) and the two existing ones were removed. [AI-assisted - Claude] Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Pass 2 review consolidated + #252 implementation. Implemented #252 (Share invite via DM): member-info modal now has a "Share an invite via DM…" button below "Send Direct Message". Opens a picker listing other rooms the local user is in; pick one → generates an invite for that room → pre-fills a DM in the current room with the invite URL → user reviews + sends. Linkified via e662ea5. Factored Review findings addressed:
Deferred:
[AI-assisted - Claude] |
GitHub didn't create workflow runs for 968e3ac despite the push being delivered and visible via the API. This empty commit forces a fresh push event so build/check-delegate-migration/check-cli-wasm fire. [AI-assisted - Claude] Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Multi-model 3rd review pass (4 internal + Codex) on commit 968e3ac. # Blocking — fixed ## Codex + Skeptical — broken DM unread seed `seed_dm_last_seen_from_rooms` in 968e3ac was a regression: the `use_effect` calling it didn't subscribe to ROOMS, so it ran once on first render against an EMPTY map (ROOMS hydrates asynchronously from the delegate) and never re-ran. The advertised "unread-on-reload fix" was a no-op. Naively fixing it (subscribe to ROOMS) introduces the opposite bug Codex flagged independently: every subsequent ROOMS update — including an inbound DM arriving — would re-seed, marking that very DM as already-seen. The unread badge would NEVER fire. Fix: one-shot pattern. - `seed_dm_last_seen_if_needed` (renamed) gated on `DM_LAST_SEEN_SEEDED: GlobalSignal<bool>`. Returns early if already seeded. Also early-returns if `ROOMS.map.is_empty()` so a pre- hydration call doesn't latch the flag. - `App()`'s `use_effect` now explicitly subscribes via `let _hydration_marker = ROOMS.try_read().map(|r| r.map.len())…`, so the effect re-fires when the delegate populates ROOMS. The seed runs exactly once, the first time ROOMS is non-empty; subsequent ROOMS updates are no-ops because the flag is latched. - Factored the pure computation into `pub(crate) compute_dm_last_seen(&Rooms) -> HashMap` so it's unit- testable without touching signals. Added 3 unit tests covering: empty rooms, inbound-vs-outbound filter, max-per-peer. # Medium — fixed ## Code-first + Skeptical — DM_DRAFT clobbered typed composer The drain `use_effect` previously did `draft.set(body)`. If the user opened the picker over an already-open thread with typed text, the invite body silently overwrote what they had typed. Now appends with blank-line separator if the existing draft is non-empty. ## Skeptical — picker self-DM guard `open_invite_via_dm_picker` now refuses to open when `peer == self`, even though all current call sites gate on that — defense in depth so a future shortcut doesn't strand a user with a self-DM draft they can't send. ## Skeptical — `resolve_recipient_vk` spurious ambiguous error When the room owner is also enrolled as an explicit `AuthorizedMember`, the previous fix counted both the member entry AND the owner branch as distinct matches for the same `VerifyingKey`, surfacing a bogus "ambiguous" error. Now dedupes by destination `VerifyingKey` before counting. ## Codex — workspace clippy.toml broke cargo make clippy The workspace-level `disallowed_methods` ban on `SystemTime::now` fired against legitimate native/test call sites in riverctl, contracts, and even the UI's own `#[cfg(not(target_arch = "wasm32"))]` arms. `cargo make clippy` runs with `-D warnings`, so every legit call became an error. Removed the lint and instead beefed up the docstring on `crate::util::get_current_system_time` (the canonical wasm-safe replacement) so future contributors see why it exists. The 88d5820 incident note now lives at the API surface. # Documentation - AGENTS.md "In-Room Direct Messages" gains a "Phase 4 (#244 follow-up, issue #252 partial)" bullet covering the picker, INVITE_VIA_DM_PICKER, DM_DRAFT, the one-shot seed, the BodyKind split, and the pub(crate) get_invitation_base_url contract. - Notes #252 is only partially implemented (member-info entry point only; Invite-Member-modal "Send to a co-member" tab + cross-room member-filter deferred). [AI-assisted - Claude] Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Pass 3 review consolidated (4 internal + Codex): Critical (fixed in 2c69ff1):
Medium (fixed):
Docs:
[AI-assisted - Claude] |
GitHub webhook delivery has been flaky on this PR — second time the build/check workflows didn't fire on a push despite the SHA showing via the API. [AI-assisted - Claude] Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Conflict resolved in .gitignore (both sides added unrelated entries — kept both: the manual e2e screenshot ignore from #244 and the Syncthing in-flight tmp file ignore from main). Auto-merged: common/src/ecies.rs (decryption helpers added on both sides — our DM-envelope unseal vs main's bytes-friendly variant for the existing secret-distribution path), ui/src/components/app.rs, members.rs, and a handful of room-state / chat-delegate files. Verified after merge: - cargo check -p river-ui --target wasm32-unknown-unknown ✓ - cargo check -p room-contract --target wasm32-unknown-unknown ✓ - cargo test -p river-core --features ecies-randomized ✓ - cargo test -p riverctl --lib commands::dm ✓ - cargo test -p river-ui --bins (direct_messages) ✓ [AI-assisted - Claude] Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PR #244 added `riverctl dm send | list | purge`. Bump the version so the new commands ship to crates.io. The room contract WASM is unchanged in #244, so this isn't a forced bump for wire-format compatibility — just to make the new commands available to end users via `cargo install riverctl`. [AI-assisted - Claude] Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
river-core gained `ecies` / `ecies-randomized` features in #244 (DM envelope helpers); the published `river-core 0.1.6` on crates.io predates them, so `cargo publish -p riverctl 0.1.56` fails to resolve dependencies until river-core 0.1.7 is published with the new features. river-ui also rides the workspace version but is not published to crates.io — bumping it is cosmetic. [AI-assisted - Claude] Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Problem
#240 / Phase 1 shipped the in-room DM wire format and contract validation:
ChatRoomStateV1now carriesDirectMessagesV1with sender-signedAuthorizedDirectMessage+ recipient-signedAuthorizedRecipientPurges,validated by the room contract WASM. But neither the UI nor
riverctlknewhow to compose, decrypt, or purge those messages —
grep -rn "DirectMessage" ui/src/ cli/src/returned nothing before this PR.This PR closes #243 by adding both consumer surfaces in one go so they stay
in wire-format lockstep.
Approach
river-coreNew helpers shared by both consumers so wire bytes are byte-identical
regardless of which client posted them:
ecies::seal_dm_for_recipient/unseal_dm_from_sender— per-messagerandomized ECIES envelope. Fresh ephemeral X25519 keypair + random nonce
per call. Wire format is
[ephemeral_pub: 32 | nonce: 12 | aes_gcm: variable].Deliberately separate from the deterministic
encrypt_secret_for_memberhelper — that one is safe only because the plaintext IS a 32-byte
high-entropy secret; generalising it to attacker-controlled message
bodies would leak
plaintext_A XOR plaintext_Bon key reuse.direct_messages::compose_direct_message— encrypt + sign + cap-checkin one call.
direct_messages::open_direct_message— recipient-side decrypt thatworks without the
ecies-randomizedfeature (chat-delegate friendly).direct_messages::advance_recipient_purges— version-bump-and-unionenvelope builder used for both single-message and thread-wide purges.
6 new ECIES tests + 7 new end-to-end-helper tests covering: round-trip
through the full
ComposableState, refusal of self-DMs, future-skewrejection, body-cap enforcement, version monotonicity, recipient/key
mismatch, and a send → purge → re-send dance that confirms the tombstone
gate drops a re-delivered message.
UI (Phase 2)
components/direct_messagesmodule with two modals:DmThreadModal: per-pair thread, decrypt-on-display, composer withbody-size guard, "Purge thread" button.
DmInboxModal: lists every open DM thread the local user has in thecurrent room, unread-first then by recency.
modal for that pair.
inbox modal.
DM_LAST_SEENper-(room, peer)drives the unread badge; thethread modal advances it on render of inbound messages.
riverctl (Phase 3)
riverctl dm send <owner_vk> <recipient> "<msg>"riverctl dm list <owner_vk> [--with <peer>] [--limit N] [--since-minutes M]riverctl dm purge <owner_vk> <hex_token_or_index>All three go through the shared
river-corehelpers, so the bytes thecontract WASM sees are identical to what the UI emits.
dm purgeacceptseither a 32-character hex
PurgeTokenor a 1-based index into the mostrecent
dm listoutput, matching the issue's spec.Added a small
ApiClient::send_state_delta()helper so the new commandsdon't have to duplicate the contract-key + serialize + recv dance.
Crate features
cli/Cargo.tomlnow enableseciesandecies-randomizedonriver-core.The UI already enabled them.
Testing
cargo test -p river-core --features ecies-randomized: 254 pass (51 DMintegration tests, 14 ECIES tests, and 189 others — no regressions).
cargo test -p riverctl --lib: 8 pass (3 new DM tests).cargo build -p river-ui --target wasm32-unknown-unknown --release:succeeds.
cargo clippy -p river-core -p riverctl --all-targets --features ecies-randomized -- -D warnings:clean.
Test plan (next steps once merged into a deployment)
dm listshows decrypted body on the receiver and ciphertext-only onthe sender.
through the synchronizer and shows up on a second browser session
logged in as the recipient.
and survives a refresh.
Out of scope
Per the issue:
Closes #243
[AI-assisted - Claude]