fix(#26): preserve un-synced localState across subscription rebuild#27
Open
TrystonPerry wants to merge 2 commits into
Open
fix(#26): preserve un-synced localState across subscription rebuild#27TrystonPerry wants to merge 2 commits into
TrystonPerry wants to merge 2 commits into
Conversation
readOnly was in the useMemo deps for both useDocument and useCollection, so any runtime toggle (e.g. auth/role state loading) tore down the subscription and discarded pending local edits in the autosave window. Fix: - Move readOnly out of the subscription memo deps for both hooks. A stable ref-backed getter (getReadOnly) is passed to the subscription factories instead, evaluated at mutation time — the same pattern used for `undoable`/`onPushUndo`. Toggling readOnly now changes the write guard in place without rebuilding the subscription. - Add getReadOnly option to DocumentOptions and CollectionOptions (the static readOnly option continues to work for direct API callers). - For genuine rebuilds (path/query change), attempt a fire-and-forget sync() in the hook cleanup before stop() so pending edits reach Firestore rather than being silently dropped. - Add a stopped flag to both subscription factories so an async sync() completing after teardown cannot re-register a stale sync key in the global store and corrupt useIsSynced. Closes #26 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EJWMe1UYgBbCjjM618GxKT
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a2ecfbca1
ℹ️ 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".
…teardown stop() now sets stopped=true to prevent a fire-and-forget sync() from re-registering a stale sync key. But the retryOnError path calls stop() followed immediately by load() (or startListener()), which re-activates the subscription — leaving stopped=true forever and causing notify() to skip reportSyncState permanently after a retry. Reset stopped=false at the start of load() (document.ts) and startListener() (collection.ts) so sync-state reporting resumes after a successful retry, and useIsSynced correctly reflects pending edits. Fixes review finding on PR #27 (Codex P1). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EJWMe1UYgBbCjjM618GxKT
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
2 participants
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.
Summary
readOnlyfrom theuseMemodependency array in bothuseDocumentanduseCollection.readOnlyis now a write-time guard applied via a stable ref-backed getter (getReadOnly), using the same pattern already used forundoable/onPushUndo. TogglingreadOnlyat runtime no longer tears down the subscription or discards pending local state.getReadOnly?: () => boolean | undefinedtoDocumentOptionsandCollectionOptions(backward-compatible — the staticreadOnlyoption still works for direct API callers).sync()call in both hook cleanup functions, so genuine subscription rebuilds (path/query changes) attempt to flush pending edits before teardown rather than silently dropping them.stoppedflag to both subscription factories to prevent an asyncsync()completing after teardown from re-registering a stale sync key and corruptinguseIsSynced.Changes
src/document.tsgetReadOnlyoption;resolveReadOnly()replacesisReadOnlyconst;stoppedflag guardsreportSyncStateinnotify()and is set instop()src/collection.tsdocument.tssrc/hooks.tsreadOnlyRef+ stablegetReadOnlycallback in both hooks;readOnlyremoved from memo deps; fire-and-forgetsync()added to subscription cleanupsrc/firestate.integration.test.tsgetReadOnlytoggle preserves document/collection localState; sync-before-stop flushes document/collection writes; stale sync key is not re-registered after stopHow to verify
useCollection({ ..., readOnly: derivedFromAuthState })wherereadOnlyflipsfalse → true → falseduring startup.readOnlyis brieflytrue, make an optimistic edit.readOnlyflips back.Repro for path-change flush: edit a document, immediately navigate (changing the path params) before autosave fires — the edit now reaches Firestore.
Follow-ups / risks
localStateto the new subscription, but that requires a subscription handshake API.Closes #26
Generated by Claude Code