Skip to content

fix(ui): keep the user's chosen nickname when joining a private room#297

Merged
sanity merged 3 commits into
mainfrom
fix/private-room-invite-nickname-override
May 21, 2026
Merged

fix(ui): keep the user's chosen nickname when joining a private room#297
sanity merged 3 commits into
mainfrom
fix/private-room-invite-nickname-override

Conversation

@sanity
Copy link
Copy Markdown
Contributor

@sanity sanity commented May 21, 2026

Problem

A user reported that the new auto-generated default nicknames (PR #294's 90s-hacker handles) ignore a nickname the user typed, specifically when joining off an invite delivered via DM.

Root cause

It is not actually DM-specific — it is private-room-specific (DM is just the natural channel for private-room invites).

When you accept an invite to a private room, the room secret is often not yet available to encrypt your nickname. The GET-response invite path therefore deliberately defers building your member_info — publishing a plaintext nickname into a private room would be a privacy leak — and leaves it to the build_member_info_heal self-heal path. But the nickname you chose was recorded only in PENDING_INVITES, which is discarded once the join completes. By the time the heal runs to remediate the "Unknown member" stranding, it has no record of your choice, so it falls back to generate_default_nickname() and publishes a generated handle — silently overriding the nickname you typed.

Fix

Persist the chosen nickname in a new RoomData.self_nickname field. The member-info rebuild paths resolve the nickname in priority order: the published self_member_info, then self_nickname (the join-time / last-edited choice), then a generated default handle.

The same symptom was reachable through three code paths — all are now fixed:

  1. build_member_info_heal (join-time self-heal) — uses the priority order above.
  2. build_rejoin_delta (inactivity re-add) — previously rebuilt a stranded member's member_info with a hardcoded plaintext "Member", which also leaked plaintext into private rooms. It now resolves the nickname the same way, seals per privacy mode, and defers member_info (members-only delta) for a private room with no secret rather than leaking. Its reuse branch also applies the same Private-sealed guard build_member_info_heal uses, so a stale Public-sealed entry (left by a public→private reconfiguration) is never republished as plaintext.
  3. Nickname edits (NicknameField) — editing your nickname now updates the cached self_member_info and self_nickname (via RoomData::record_self_nickname_edit), so an edit can't be reverted by a later heal/rejoin firing before the next sync.

The invite-accept path sets the credentials through a new RoomData::record_invite_credentials helper, so a caller cannot set self_member_info and forget self_nickname — the omission that caused the original bug.

Compatibility

  • The new field uses #[serde(default)] — the same backward-compatible pattern as the other added RoomData fields. Old persisted blobs deserialize with self_nickname = None.
  • No contract or delegate WASM changeRoomData is the UI's local per-room state, stored by the chat delegate as an opaque blob. No contract/delegate key change, no migration, no data-loss risk.
  • self_nickname is stored in plaintext; this adds no exposure — the persisted RoomData already carries self_sk, from which every room secret (hence every sealed nickname) is derivable. The rebuild paths always seal it before publishing into a private room.

Testing

14 new/extended unit tests in room_data.rs covering: the heal and rejoin nickname-priority ordering (self_member_info > self_nickname > default), private-room sealing, deferral when no secret is available, the Private-sealed-guard on the rejoin reuse branch, record_invite_credentials, and record_self_nickname_edit. Full river-ui suite green (220 passed); cargo fmt and clippy clean on the changed code.

Follow-up

#298 — identity-export-before-heal can still lose the chosen nickname on re-import (IdentityExport carries no plaintext nickname). Narrow window, pre-existing, out of scope here.

[AI-assisted - Claude]

🤖 Generated with Claude Code

sanity and others added 2 commits May 21, 2026 10:38
When you join a private room via invite, the room secret is often not
yet available to encrypt your nickname, so the GET-response invite path
deliberately defers building your `member_info` (publishing a plaintext
nickname into a private room would leak it). The nickname you chose was
recorded only in `PENDING_INVITES`, which is discarded once the join
completes.

By the time `build_member_info_heal` runs to remediate the resulting
"Unknown member" stranding, it had no record of your choice, so it fell
back to `generate_default_nickname()` and published a generated handle
— silently overriding the nickname you typed. DM-delivered invites are
the natural channel for private rooms, which is where users hit this.

Persist the chosen nickname in a new `RoomData.self_nickname` field
(plaintext — `RoomData` is trusted local state and already holds
`self_sk` and decrypted secrets). `build_member_info_heal` now resolves
the nickname in priority order: the published `self_member_info`, then
`self_nickname` (the join-time choice), then a generated default.

The field uses `#[serde(default)]`, the same backward-compatible pattern
as the other added `RoomData` fields — old persisted blobs deserialize
with `self_nickname = None`. No contract or delegate WASM changes, so no
key change and no migration.

Adds three regression tests covering the public-room heal, the
private-room sealed heal, and the no-nickname-on-record default path.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Round-2 review found two more paths to the same symptom and gaps:

- M1 (skeptical): the nickname-edit modal did not update `self_nickname`,
  so editing your nickname during the deferred-seal window could be
  reverted by a later self-heal. `NicknameField::save_changes` now keeps
  `self_nickname` in step with the edit.

- build_rejoin_delta (big-picture): the inactivity-rejoin path rebuilt a
  stranded member's `member_info` with a hardcoded plaintext "Member",
  ignoring the chosen nickname AND leaking plaintext into private rooms.
  It now resolves the nickname `self_member_info` > `self_nickname` >
  generated default (same order as the heal), seals it correctly per
  privacy mode, and defers member_info (returns members-only) for a
  private room with no secret rather than leaking plaintext.

- Testing: the invite-accept producer path is now a testable
  `RoomData::record_invite_credentials` method that sets the three
  `self_*` credential fields together, so a caller cannot set
  `self_member_info` and forget `self_nickname`.

- Doc (M2): corrected the `self_nickname` privacy rationale — the
  persisted blob already carries `self_sk`, from which every room secret
  (hence every sealed nickname) is derivable, so plaintext storage of the
  nickname adds no exposure.

Adds 7 regression tests: heal/rejoin nickname-priority ordering, private
rejoin sealing, private rejoin deferral without a secret, and
`record_invite_credentials`. Full river-ui suite green (216 passed).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- S1 (skeptical): `build_rejoin_delta`'s reuse branch cloned
  `self_member_info` unconditionally, with none of the Private-sealed
  guard `build_member_info_heal` applies. A room reconfigured public →
  private can hold a stale Public-sealed self entry; rejoining would
  republish it as plaintext into the private room. The reuse branch now
  applies the same guard — a Public-sealed entry in a private room is
  not reused; the entry is rebuilt and Private-sealed instead.

- Codex P2 / testing: the nickname-edit path updated `self_nickname` but
  not the higher-priority `self_member_info`, so a heal/rejoin firing
  before the next sync could still republish the pre-edit nickname. The
  edit-path logic is now a testable `RoomData::record_self_nickname_edit`
  method that updates BOTH cached fields (gated on the edit being the
  local user's own), replacing the inline `nickname_field.rs` write.

- Doc: note in `capture_self_membership_data` that `self_nickname` is
  deliberately not refreshed there (it is the lower-priority fallback,
  kept current at its own write sites).

Adds 4 regression tests: rejoin ignores a Public-sealed stored entry in
a private room, rejoin reuses a Private-sealed one verbatim, and
`record_self_nickname_edit` updates self / ignores other members.

Filed #298 for the residual identity-export-before-heal nickname gap.

Full river-ui suite green (220 passed).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor Author

@sanity sanity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comprehensive PR Review: #297

Summary

  • PR Title: fix(ui): keep the user's chosen nickname when joining a private room
  • Type: fix
  • CI Status: migration checks + CLA green; build / ui-playwright-tests pending at review time (must be green on HEAD before merge)
  • Linked Issues: none closed; follow-ups #298, #299 filed
  • Review tier: Full (touches persistence/serialization + invite/auth/member-info flow)
  • Reviewers run: code-first, testing, skeptical, big-picture, + Codex (external). Three review rounds — this is the round-3 (final) synthesis on HEAD 9dc1701b.

Code-First Analysis

Independent understanding: A new RoomData.self_nickname field persists the nickname the user chose for a room. Three rebuild paths (build_member_info_heal, build_rejoin_delta, the nickname-edit path) resolve the nickname in a consistent priority order — self_member_info > self_nickname > generated default.
Stated intent: Stop the auto-generated default handle (#294) from silently overriding a user-typed nickname when joining a private room.
Alignment: Matches. The root cause is correctly diagnosed as private-room-specific (the member_info seal is deferred when the room secret hasn't arrived; the chosen nickname lived only in the discarded PENDING_INVITES).
Gaps: None material. The rewritten PR description is accurate against HEAD 9dc1701b.


Testing Assessment

Coverage Level: adequate

Test Type Status Notes
Unit 14 new/extended tests; full river-ui suite 220 passed
Integration ⚠️ handle_get_response is async/global-signal-bound — not unit-tested; pre-existing, the producer logic was extracted into the testable record_invite_credentials
Simulation N/A
E2E N/A timing-dependent bug, not deterministically reproducible in Playwright

Regression Test: present and valid — heal/rejoin nickname-priority ordering, private-room sealing, secret-absent deferral, the Private-sealed guard on both reuse branches, and both helper methods are all directly pinned.
Missing Tests: none blocking.


Skeptical Findings

Risk Level: low

Concern Severity Location Details
Rejoin reuse branch republished a stale Public-sealed entry as plaintext into a private room High Fixed room_data.rs:600-606 Round-2 finding S1 — fixed: the reuse branch now applies the same Private-sealed guard as build_member_info_heal
Nickname edit didn't update self_member_info Medium Fixed nickname_field.rs Round-2 (Codex P2) — fixed via record_self_nickname_edit, which updates both cached fields
record_self_nickname_edit may cache a Public-sealed entry in a private room (no local secret) Consider room_data.rs:528-539 Inert — every reader (build_rejoin_delta, build_member_info_heal guards, capture_self_membership_data) filters or overwrites it
(Some(members), None) rejoin re-adds a member without member_info Consider (reconciled) room_data.rs:629-631 Codex P2 — see Recommendations; by design

Big Picture Assessment

Goal Alignment: yes — the bug is fixed across all three reachable paths (join-heal, inactivity-rejoin, nickname-edit).
Anti-Patterns Detected: none. No ignored/weakened tests, no swallowed errors. Round 2 additionally removed a pre-existing plaintext-"Member"-into-private-room leak in build_rejoin_delta.
Removed Code Concerns: none — removed lines are exactly the old buggy logic being replaced.
Scope Assessment: focused. The non-core file changes are mechanical self_nickname: None additions to RoomData struct literals, required by the new field.


Documentation

  • Code docs: complete — self_nickname, record_invite_credentials, record_self_nickname_edit, and the capture_self_membership_data note all accurately explain the non-obvious priority/staleness reasoning.
  • Architecture docs: n/a.
  • User docs: n/a.
  • No contract/delegate WASM change — confirmed: diff is entirely under ui/src/; check-room-contract-migration and check-delegate-migration both pass. #[serde(default)] keeps old persisted RoomData blobs compatible.

Recommendations

Must Fix (Blocking)

None.

Should Fix (Important)

None outstanding. The round-2 Should-Fix findings (S1 rejoin privacy guard; testable edit-path method; self_member_info updated on edit) are all addressed and tested.

Consider (Suggestions)

  1. Codex P2 — (Some(members), None) rejoin return — reconciled as by-design, not actioned. Codex flagged that re-adding a member without member_info makes them render "Unknown" until the self-heal runs. This is intentional and consistent with the existing join path: build_state_for_put already injects a member without member_info for a private room whose secret isn't available (pinned by build_state_for_put_omits_member_info_when_none). The "Unknown" state is precisely the transient build_member_info_heal exists to remediate — idempotent and self-resolving. Codex's alternative (defer/reject the rejoin) would make the callers' preflight reject the user's message/DM send outright — strictly worse UX than a brief auto-healed "Unknown". The only way to carry member_info here is to publish a plaintext nickname into the private room — the exact leak this PR removes. Both round-2 and round-3 skeptical passes independently verified (Some, None) as the correct trade-off.
  2. A one-line doc note on build_rejoin_delta that it reads privacy/secret from self (unlike build_member_info_heal, which reads from network state) would help future maintainers. Optional.
  3. Pre-existing issues surfaced and filed as follow-ups, out of scope here: #298 (identity export-before-heal loses the chosen nickname) and #299 (nickname edit in a private room with no local secret publishes plaintext).

Verdict

State: Ready to Merge — pending CI green on the current HEAD.
HEAD SHA reviewed: 9dc1701b

Three review rounds: round 1 surfaced Consider-level items; round 2 found two new real issues (S1 rejoin privacy guard, the edit path not updating self_member_info); round 3 fixed both and this final pass is clean — no Must-Fix or Should-Fix code findings from any reviewer. The one external-model finding (Codex P2) is reconciled above as intentional and design-consistent. Merge once build and ui-playwright-tests pass on 9dc1701b.

[AI-assisted - Claude]

@sanity sanity merged commit d435747 into main May 21, 2026
7 checks passed
sanity added a commit that referenced this pull request May 21, 2026
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
sanity added a commit that referenced this pull request May 24, 2026
`common/src/ecies.rs:144` started failing to compile when building
chat-delegate (which enables `river-core/ecies`) on 2026-05-24:

    error[E0599]: no method named `as_bytes` found for struct
    `GenericArray<u8, UInt<...>>` in the current scope

A transitive dep enabled blake3 1.8.2's `digest` / `traits-preview`
feature, which adds an `impl digest::Digest for blake3::Hasher`.
With `use sha2::Digest` already in scope at the top of this file,
`hasher.finalize()` started resolving to the trait method
(returning `GenericArray<u8, U64>`) instead of the inherent
(returning `blake3::Hash`).

Force the inherent via UFCS — byte-identical compiled output to
all prior builds, no behavior change. Last green main was 1c3643e
on 2026-05-23; this restores buildability on top of the May-24
commits (#297, #301, #303) that caused the feature flip.

Pushed direct-to-main per Ian's explicit per-incident authorization
(main is wedged for everyone; the river-publish workflow is blocked
on this).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.

1 participant