Skip to content

fix(ui): gate legacy delegate migration on current delegate response#254

Merged
sanity merged 2 commits into
mainfrom
worktree-fix-253-delegate-migration
May 16, 2026
Merged

fix(ui): gate legacy delegate migration on current delegate response#254
sanity merged 2 commits into
mainfrom
worktree-fix-253-delegate-migration

Conversation

@sanity
Copy link
Copy Markdown
Contributor

@sanity sanity commented May 16, 2026

Problem

A long-running River user reports newly-created rooms silently disappearing after page refresh (#253). Root cause: a race between the current delegate's load-rooms response and the legacy migration probes.

`set_up_chat_delegate()` was firing both the current delegate's load-rooms request and the legacy migration probes concurrently at startup. Each legacy response that returned data triggered `save_rooms_to_delegate()`, writing whatever was currently in `ROOMS` to the current delegate's storage.

When a legacy response arrives before the current delegate's response, `ROOMS` still holds an empty or partial state, so the legacy-triggered save overwrites the current delegate with stale data. On next refresh, the current delegate returns the overwritten state and recently-created rooms are gone.

The user reports this happening repeatedly across refreshes, which is consistent with: the localStorage "migration done" flag only gets set after a successful save, so if save races and writes incomplete state, the flag may still be unset, and the migration re-runs each session.

Approach

Gate legacy migration on the current delegate's response so it only fires when there is nothing to clobber:

  • `set_up_chat_delegate()` no longer fires migration probes.
  • When the current delegate returns rooms_data successfully, mark legacy migration done across sessions — current is the source of truth.
  • When the current delegate returns no rooms_data, then fire the legacy migration probes. With current empty, no race possible.

Trade-off: a user with data in both current and legacy (e.g. an interrupted migration in the past) will not get the legacy data merged. But the alternative — the current behavior — actively destroys newer data, which is much worse. The legacy data path was always best-effort recovery from old WASM versions; the new state is what the user has been actively building.

Testing

  • `cargo check -p river-ui --target wasm32-unknown-unknown --features no-sync` ✅
  • `cargo test -p river-core --test migration_test` ✅ (4/4)
  • `cargo fmt --check` ✅
  • `cargo clippy` produces no new warnings (4 pre-existing warnings unchanged)

A deterministic unit test for this race is impractical: it requires WASM, WebSocket, Dioxus signals, and a specific async ordering. The fix is structural — legacy probes are gated behind a single response — so the proof is by inspection of the changed control flow.

Closes #253

[AI-assisted - Claude]

Previously, set_up_chat_delegate() fired both the current delegate's
load-rooms request and the legacy migration probes concurrently at
startup. Each legacy response that returned data triggered a
save_rooms_to_delegate() that wrote whatever was currently in ROOMS
to the *current* delegate's storage.

This is racy: when a legacy delegate's response arrives before the
current delegate's response, ROOMS still holds an empty or partial
state, and the legacy-triggered save overwrites the current delegate's
storage with that stale state. The next page refresh then reads the
overwritten storage and the user sees rooms they had recently created
silently disappear (#253).

Fix: gate legacy migration on the current delegate's response.

- set_up_chat_delegate() no longer fires the migration probes.
- When the current delegate returns rooms_data successfully, mark
  legacy migration done across sessions — current is the source of
  truth and there is nothing to recover from legacy.
- When the current delegate returns no rooms_data, *then* fire the
  legacy migration probes. With current empty, there is no state for
  a legacy response to clobber.

This eliminates the cross-delegate save race entirely and also stops
the migration path from firing on every reconnect for users whose
current delegate already has data.

Closes #253

[AI-assisted - Claude]
Address PR #254 review findings:

- **Codex (P2)**: An empty `Some(serialized empty Rooms)` previously
  marked legacy migration done, stranding users whose only rooms exist
  under a legacy delegate. `save_rooms_to_delegate` writes a placeholder
  even before any room exists, so a present-but-empty payload is
  indistinguishable from "no rooms yet" and should fall through to
  legacy migration. Migration is now marked done only when the current
  delegate has at least one room OR at least one tombstone.

- **Testing review**: Extract `decide_legacy_migration_action()` as a
  pure helper so the gating logic is unit-testable without WASM or
  WebSocket plumbing. Add 5 unit tests covering each (value_present,
  has_rooms, has_tombstones) combination.

- **Skeptical (M2)**: Tighten `fire_legacy_migration_request` from
  `pub` to `pub(crate)` — it's only called from the response handler
  in this crate, and narrower visibility makes the "must only be
  called when current is empty" invariant easier to enforce.

- **Big-picture**: Update `.claude/rules/river-publish.md` to describe
  the gated migration flow. The previous doc claimed migration fired
  unconditionally at startup, which is no longer true.

H1 cursor-restore concern from the skeptical review (legacy snapshot
can overwrite the user's CURRENT_ROOM selection) is pre-existing and
filed as #255.

[AI-assisted - Claude]
@sanity
Copy link
Copy Markdown
Contributor Author

sanity commented May 16, 2026

Review responses (5 reviewers: 4 internal + Codex):

Addressed in 3cdacf2:

  • Codex P2: Empty Some(serialized empty Rooms) no longer suppresses migration. Now distinguishes "has rooms or tombstones" (authoritative — mark done) from "value present but empty" (placeholder — fire migration). Users with rooms only in legacy delegates are no longer stranded.
  • Testing review: Extracted decide_legacy_migration_action() as a pure helper with 5 unit tests covering each (value_present, has_rooms, has_tombstones) case. Locks the gating polarity against future refactors.
  • Skeptical M2: fire_legacy_migration_request tightened from pub to pub(crate).
  • Big-picture: .claude/rules/river-publish.md updated to reflect the gated flow.

Acknowledged, not changed:

All review verdicts: ready to merge with the fixes above.

[AI-assisted - Claude]

@sanity sanity merged commit 1a92c87 into main May 16, 2026
5 checks passed
@sanity sanity deleted the worktree-fix-253-delegate-migration branch May 16, 2026 20:50
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.

bug: old delegates overwriting the current active delegate

1 participant