fix: restart the persistence process when usePersistSignals entries change - #163
fix: restart the persistence process when usePersistSignals entries change#163mbret wants to merge 1 commit into
Conversation
…hange Entries changes were piped through concatMap, but the inner persistSignals observable never completes (it keeps persisting signal updates), so a new entries list was queued forever: signals added after the initial hydration were never hydrated nor persisted. Switch to switchMap — mirroring the adapter-change path — so a new list restarts the process as documented. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0152ZzwVACMdMfk6jt7Ezzkn
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b94bb31c57
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| * queue new entries forever. Restart the process instead | ||
| * whenever a new list is emitted. | ||
| */ | ||
| switchMap((entries) => |
There was a problem hiding this comment.
Preserve pending updates when restarting persistence
When the entries reference changes within the 500 ms throttleTime window after an existing signal update, this switchMap unsubscribes the old persistence stream and discards its pending trailing write. The replacement persistSignals then rehydrates every existing entry from the adapter, so the stale stored value overwrites the newer in-memory value and is persisted again; for example, updating a and immediately changing [a] to [a, b] loses the update to a. Restarting should flush/preserve pending values or hydrate only newly added entries rather than rehydrating unchanged signals.
Useful? React with 👍 / 👎.
The bug
usePersistSignalsdocuments that "Passing a new list of entries will start over the process" — but entries passed after the initial hydration are silently ignored: they are never hydrated from storage and their updates are never persisted.How to observe: render
usePersistSignals({ entries: [a], adapter }), wait forisHydrated, then re-render withentries: [a, b]where storage already holds a value forb's key. Signalbkeeps its default value forever (stored value never hydrated), and updates tobare never written to the adapter. Only the original entryakeeps working.Root cause
In
usePersistSignals, entries emissions are piped throughconcatMap(persistSignals):concatMaponly subscribes to the next inner observable once the previous one completes — but the observable returned bypersistSignalsnever completes on its own: after hydrating, it keeps persisting signal updates indefinitely (persisted$is an infinite merge of the signals' streams). So every new entries list is queued behind an inner observable that never finishes, and is starved forever.The fix
Use
switchMapinstead, mirroring the adapter-change path a few lines above (which already usesswitchMap): a new entries list tears down the previous hydrate-and-persist process and starts a fresh one, as documented. The stale "once the current one is finished" phrasing in theentriesdoc comment is updated to match.Verification
New regression test
src/lib/state/react/usePersistSignals.test.tsx:[a], waits forisHydrated,[a, b]where storage holds7forb→ assertsbhydrates to7,bto9→ asserts the adapter receives the write.The test times out on
main(step 2 never happens) and passes with the fix. Full gates run locally:npm run check,npm run build, andnpm run test:ci(25 files / 140 tests) all pass. (useQuery$.test.tsx > should return consecutive resultstimed out once on a cleanmainbaseline under parallel load but passes in isolation and in the post-fix full run — pre-existing flake, unrelated.)Other findings (not addressed in this PR)
queryClient.clear()while a live-streamuseQuery$is mounted looked like it would leave a stale bridge cache entry, but TanStack'scache.remove()callsquery.destroy(), which aborts the fetch signal and the bridge'stakeUntil(fromEvent(signal, "abort"))tears the entry down correctly (verified empirically against a promise-based baseline).Generated by Claude Code