From 9a2ecfbca16cece5a7c175407a2bbd9acd1b0ab1 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 18 Jun 2026 22:52:49 +0000 Subject: [PATCH 1/2] fix(#26): preserve un-synced localState across subscription rebuild MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01EJWMe1UYgBbCjjM618GxKT --- src/collection.ts | 32 ++++- src/document.ts | 32 ++++- src/firestate.integration.test.ts | 204 ++++++++++++++++++++++++++++++ src/hooks.ts | 65 +++++++--- 4 files changed, 301 insertions(+), 32 deletions(-) diff --git a/src/collection.ts b/src/collection.ts index eac9e5d..3a5ca58 100644 --- a/src/collection.ts +++ b/src/collection.ts @@ -73,6 +73,13 @@ export interface CollectionOptions { collectionPath?: string /** Override read-only setting */ readOnly?: boolean + /** + * Dynamic read-only getter. When provided, evaluated on each mutation + * call instead of the static `readOnly` option. The hook layer uses this + * so that toggling `readOnly` at runtime does not tear down the + * subscription or discard pending local state. + */ + getReadOnly?: () => boolean | undefined /** Additional query constraints */ queryConstraints?: QueryConstraint[] /** Callback for pushing undo actions */ @@ -137,7 +144,7 @@ export const createCollectionSubscription = ( /** Force sync now */ sync: () => Promise } => { - const { store, definition, collectionPath: resolvedPath, readOnly, queryConstraints: extraConstraints, onPushUndo } = options + const { store, definition, collectionPath: resolvedPath, readOnly, getReadOnly, queryConstraints: extraConstraints, onPushUndo } = options const { firestore, autosave: defaultAutosave, minLoadTime: defaultMinLoadTime } = store const { @@ -152,7 +159,12 @@ export const createCollectionSubscription = ( schema, } = definition - const isReadOnly = readOnly ?? definitionReadOnly ?? false + // When the hook layer supplies getReadOnly, evaluate it on each mutation + // call so that toggling readOnly at runtime takes effect without rebuilding + // the subscription. Direct API callers that pass the static `readOnly` + // option work as before. + const resolveReadOnly = () => + (getReadOnly ? getReadOnly() : readOnly) ?? definitionReadOnly ?? false // Prefer the caller-resolved path. Fall back to a string `definition.path` // for ergonomic direct use; if both are missing, the caller forgot to // resolve a function path. @@ -184,6 +196,9 @@ export const createCollectionSubscription = ( let retryTimeout: ReturnType | null = null let minLoadTimeElapsed = false let loaded = false + // Set to true by stop() so that an in-flight fire-and-forget sync() + // completing after teardown doesn't re-register a stale sync key. + let stopped = false // Cached handle — returns the same reference until notify() invalidates // it. Lets useSyncExternalStore consumers rely on handle identity. let cachedHandle: CollectionHandle | null = null @@ -215,7 +230,9 @@ export const createCollectionSubscription = ( cachedHandle = null const publicState = getPublicState() subscribers.forEach((fn) => fn(publicState)) - store.reportSyncState(syncKey, publicState.isSynced) + if (!stopped) { + store.reportSyncState(syncKey, publicState.isSynced) + } } // Pre-snapshot mutations are unsafe because computing a partial-update @@ -235,7 +252,7 @@ export const createCollectionSubscription = ( diff: WithFieldValue>>, undoOptions: UpdateOptions = {} ) => { - if (isReadOnly) return + if (resolveReadOnly()) return if (state.syncState === undefined) { warnNoSnapshot('update') return @@ -307,7 +324,7 @@ export const createCollectionSubscription = ( ? maybeUndoOptions : (dataOrOptions as UpdateOptions | undefined)) ?? {} - if (isReadOnly) return undefined + if (resolveReadOnly()) return undefined if (state.syncState === undefined) { // Bail rather than queueing: an explicit id that happens to exist // on the server would round-trip through computeDiff and clobber @@ -360,7 +377,7 @@ export const createCollectionSubscription = ( } const removeDocument = (id: string, undoOptions: UpdateOptions = {}) => { - if (isReadOnly) return + if (resolveReadOnly()) return if (state.syncState === undefined) { warnNoSnapshot('remove') return @@ -601,6 +618,9 @@ export const createCollectionSubscription = ( } const stop = () => { + // Prevent any in-flight fire-and-forget sync() from re-registering + // a stale sync key after teardown. + stopped = true if (retryTimeout) { clearTimeout(retryTimeout) retryTimeout = null diff --git a/src/document.ts b/src/document.ts index 03e2ad1..1ab61d9 100644 --- a/src/document.ts +++ b/src/document.ts @@ -54,6 +54,13 @@ export interface DocumentOptions { collectionPath?: string /** Override read-only setting */ readOnly?: boolean + /** + * Dynamic read-only getter. When provided, evaluated on each mutation + * call instead of the static `readOnly` option. The hook layer uses this + * so that toggling `readOnly` at runtime does not tear down the + * subscription or discard pending local state. + */ + getReadOnly?: () => boolean | undefined /** Callback for pushing undo actions */ onPushUndo?: ( undoAction: () => void, @@ -127,7 +134,7 @@ export const createDocumentSubscription = ( /** Force sync now */ sync: () => Promise } => { - const { store, definition, docId, collectionPath: resolvedCollectionPath, readOnly, onPushUndo } = options + const { store, definition, docId, collectionPath: resolvedCollectionPath, readOnly, getReadOnly, onPushUndo } = options const { firestore, autosave: defaultAutosave, minLoadTime: defaultMinLoadTime } = store const { @@ -141,7 +148,12 @@ export const createDocumentSubscription = ( schema, } = definition - const isReadOnly = readOnly ?? definitionReadOnly ?? false + // When the hook layer supplies getReadOnly, evaluate it on each mutation + // call so that toggling readOnly at runtime takes effect without rebuilding + // the subscription. Direct API callers that pass the static `readOnly` + // option work as before. + const resolveReadOnly = () => + (getReadOnly ? getReadOnly() : readOnly) ?? definitionReadOnly ?? false // Prefer the caller-resolved docId. Fall back to a string `definition.id` // for ergonomic direct use; if both are missing, the caller forgot to // resolve a function id and we surface that immediately. @@ -186,6 +198,9 @@ export const createDocumentSubscription = ( let minLoadTimeout: ReturnType | null = null let minLoadTimeElapsed = false let loaded = false + // Set to true by stop() so that an in-flight fire-and-forget sync() + // completing after teardown doesn't re-register a stale sync key. + let stopped = false // Cached handle — returns the same reference until notify() invalidates // it. Lets useSyncExternalStore consumers rely on handle identity. let cachedHandle: DocumentHandle | null = null @@ -223,14 +238,16 @@ export const createDocumentSubscription = ( cachedHandle = null const publicState = getPublicState() subscribers.forEach((fn) => fn(publicState)) - store.reportSyncState(syncKey, publicState.isSynced) + if (!stopped) { + store.reportSyncState(syncKey, publicState.isSynced) + } } const updateState = ( diff: WithFieldValue>, undoOptions: UpdateOptions = {} ) => { - if (isReadOnly) return + if (resolveReadOnly()) return const currentData = getMergedData() if (!currentData) { @@ -280,7 +297,7 @@ export const createDocumentSubscription = ( } const setData = (data: TData, undoOptions: UpdateOptions = {}) => { - if (isReadOnly) return + if (resolveReadOnly()) return // Use schema.parse as a validation guard — throw on bad input — but // discard the parsed result and store the caller's original object. @@ -328,7 +345,7 @@ export const createDocumentSubscription = ( } const deleteDocument = (undoOptions: UpdateOptions = {}) => { - if (isReadOnly) return + if (resolveReadOnly()) return const currentData = getMergedData() // Nothing to delete — bail rather than queueing a no-op deleteDoc. @@ -586,6 +603,9 @@ export const createDocumentSubscription = ( } const stop = () => { + // Prevent any in-flight fire-and-forget sync() from re-registering + // a stale sync key after teardown. + stopped = true if (unsubscribeListener) { unsubscribeListener() unsubscribeListener = null diff --git a/src/firestate.integration.test.ts b/src/firestate.integration.test.ts index 8c12c8e..42e7b7f 100644 --- a/src/firestate.integration.test.ts +++ b/src/firestate.integration.test.ts @@ -26,6 +26,15 @@ vi.mock('firebase/firestore', async () => { onSnapshot: vi.fn(() => () => { /* noop unsubscribe */ }), + setDoc: vi.fn(() => Promise.resolve()), + updateDoc: vi.fn(() => Promise.resolve()), + deleteDoc: vi.fn(() => Promise.resolve()), + writeBatch: vi.fn(() => ({ + set: vi.fn(), + update: vi.fn(), + delete: vi.fn(), + commit: vi.fn(() => Promise.resolve()), + })), } }) @@ -39,6 +48,7 @@ import { buildCollectionDefinition, } from './firestate' import { createDocumentSubscription } from './document' +import { createCollectionSubscription } from './collection' import { createStore, type FirestateStore } from './store' const revisionSchema = z.object({ title: z.string() }) @@ -326,3 +336,197 @@ describe('Document subscription: serverTimestamp display overrides', () => { expect(sub.getState().data!.updatedAt).toBeInstanceOf(Timestamp) }) }) + +// Pins the fix for issue #26: subscription rebuild on readOnly/query change +// must not silently discard pending un-synced localState. +describe('Issue #26: readOnly as live ref / flush on teardown', () => { + let store: FirestateStore + let snapshotCallback: ((snap: unknown) => void) | undefined + + beforeEach(() => { + vi.clearAllMocks() + // Use a large autosave so the debounce timer never fires during tests — + // we control sync() calls explicitly. + store = createStore({ firestore: {} as any, autosave: 999999 }) + + snapshotCallback = undefined + vi.mocked(firestore.onSnapshot).mockImplementation( + ((_ref: unknown, onNext: (snap: unknown) => void) => { + snapshotCallback = onNext + return () => { + snapshotCallback = undefined + } + }) as never + ) + }) + + const fireDocSnapshot = (data: Record | null) => { + snapshotCallback!({ + exists: () => data !== null, + data: () => data, + metadata: { fromCache: false, hasPendingWrites: false }, + }) + } + + const fireColSnapshot = (docs: Array<{ id: string; data: Record }>) => { + snapshotCallback!({ + docs: docs.map((d) => ({ id: d.id, data: () => d.data })), + }) + } + + const taskSchema = z.object({ title: z.string() }) + + // Part 1: readOnly toggle must not discard pending localState + + it('toggling getReadOnly on a document subscription does not discard pending localState', () => { + const definition = buildDocumentDefinition( + doc({ path: 'tasks/{taskId}', schema: taskSchema }) + ) + + let readOnly = false + const getReadOnly = () => readOnly + + const sub = createDocumentSubscription({ + store, + definition, + docId: 't1', + collectionPath: 'tasks', + getReadOnly, + }) + sub.load() + fireDocSnapshot({ title: 'original' }) + + sub.getHandle().update({ title: 'edited' }) + expect(sub.getState().isSynced).toBe(false) + expect(sub.getState().data?.title).toBe('edited') + + // Toggle readOnly — must not rebuild the subscription or clear localState + readOnly = true + + expect(sub.getState().isSynced).toBe(false) + expect(sub.getState().data?.title).toBe('edited') + + // Write guard is now active — further mutations must be blocked + sub.getHandle().update({ title: 'blocked' }) + expect(sub.getState().data?.title).toBe('edited') + + // Re-enabling writes must work on the same subscription + readOnly = false + sub.getHandle().update({ title: 'continued' }) + expect(sub.getState().data?.title).toBe('continued') + expect(sub.getState().isSynced).toBe(false) + }) + + it('toggling getReadOnly on a collection subscription does not discard pending localState', () => { + const definition = buildCollectionDefinition( + col({ path: 'tasks', schema: taskSchema }) + ) + + let readOnly = false + const getReadOnly = () => readOnly + + const sub = createCollectionSubscription({ + store, + definition, + collectionPath: 'tasks', + getReadOnly, + }) + sub.load() + fireColSnapshot([{ id: 't1', data: { title: 'original', id: 't1' } }]) + + sub.getHandle().update({ t1: { title: 'edited' } }) + expect(sub.getState().isSynced).toBe(false) + expect(sub.getState().data['t1']?.title).toBe('edited') + + // Toggle readOnly — must not discard the pending edit + readOnly = true + + expect(sub.getState().isSynced).toBe(false) + expect(sub.getState().data['t1']?.title).toBe('edited') + + // Re-enable and verify further edits work + readOnly = false + sub.getHandle().update({ t1: { title: 'continued' } }) + expect(sub.getState().data['t1']?.title).toBe('continued') + expect(sub.getState().isSynced).toBe(false) + }) + + // Part 2: fire-and-forget sync on teardown must attempt the Firestore write + + it('sync() before stop() on a document subscription flushes pending localState', async () => { + const definition = buildDocumentDefinition( + doc({ path: 'tasks/{taskId}', schema: taskSchema }) + ) + + const sub = createDocumentSubscription({ + store, + definition, + docId: 't1', + collectionPath: 'tasks', + }) + sub.load() + fireDocSnapshot({ title: 'original' }) + + sub.getHandle().update({ title: 'edited' }) + expect(sub.getState().isSynced).toBe(false) + + // Simulate the hook cleanup: fire-and-forget sync then stop + const syncPromise = sub.sync() + sub.stop() + await syncPromise + + // updateDoc must have been called for the partial update + expect(vi.mocked(firestore.updateDoc)).toHaveBeenCalled() + }) + + it('sync() before stop() on a collection subscription flushes pending localState', async () => { + const definition = buildCollectionDefinition( + col({ path: 'tasks', schema: taskSchema }) + ) + + const sub = createCollectionSubscription({ + store, + definition, + collectionPath: 'tasks', + }) + sub.load() + fireColSnapshot([{ id: 't1', data: { title: 'original', id: 't1' } }]) + + sub.getHandle().update({ t1: { title: 'edited' } }) + expect(sub.getState().isSynced).toBe(false) + + const syncPromise = sub.sync() + sub.stop() + await syncPromise + + expect(vi.mocked(firestore.writeBatch)).toHaveBeenCalled() + }) + + it('stop() after sync() does not leave a stale sync key in the global store', async () => { + const definition = buildDocumentDefinition( + doc({ path: 'tasks/{taskId}', schema: taskSchema }) + ) + + const sub = createDocumentSubscription({ + store, + definition, + docId: 't1', + collectionPath: 'tasks', + }) + sub.load() + fireDocSnapshot({ title: 'original' }) + + sub.getHandle().update({ title: 'edited' }) + // Global store sees this subscription as unsynced + expect(store.isSynced).toBe(false) + + const syncPromise = sub.sync() + sub.stop() + // stop() must immediately unregister the key so isSynced can flip back + expect(store.isSynced).toBe(true) + + // Completing the async sync must not re-add the stale key + await syncPromise + expect(store.isSynced).toBe(true) + }) +}) diff --git a/src/hooks.ts b/src/hooks.ts index ba135d6..02a8d5f 100644 --- a/src/hooks.ts +++ b/src/hooks.ts @@ -199,10 +199,10 @@ export interface UseDocumentOptions { * Hook to subscribe to a Firestore document with real-time updates. * * The subscription is keyed on the resolved document path (`definition` + - * computed id) and `readOnly`. When that key changes — typically because - * `params` produces a different id — the hook tears down the old Firestore - * listener and attaches a new one. Toggling `undoable` does not rebuild the - * subscription. + * computed id). When that changes — typically because `params` produces a + * different id — the hook tears down the old Firestore listener and attaches a + * new one. Toggling `readOnly` or `undoable` does not rebuild the subscription; + * `readOnly` is applied at mutation time via a live ref. * * Use `enabled: false` to suppress the subscription entirely (e.g., when * route params aren't ready yet). @@ -255,6 +255,14 @@ export const useDocument = ( const undoableRef = useRef(undoable); undoableRef.current = undoable; + // Hold readOnly in a ref for the same reason: toggling readOnly should + // change the write guard without rebuilding the subscription or discarding + // any pending local state. getReadOnly is stable (no deps) and the + // subscription calls it at mutation time. + const readOnlyRef = useRef(readOnly); + readOnlyRef.current = readOnly; + const getReadOnly = useCallback(() => readOnlyRef.current, []); + const onPushUndo = useCallback( (undoAction: () => void, redoAction: () => void, opts?: UpdateOptions) => { if (!undoableRef.current) return; @@ -290,11 +298,15 @@ export const useDocument = ( definition, docId, collectionPath, - readOnly, + getReadOnly, onPushUndo, }) : null, - [enabled, store, definition, docId, collectionPath, readOnly, onPushUndo] + // readOnly is intentionally excluded: it is applied at mutation time via + // getReadOnly (a stable ref-backed getter) so that toggling it does not + // rebuild the subscription or discard pending local state. + // eslint-disable-next-line react-hooks/exhaustive-deps + [enabled, store, definition, docId, collectionPath, onPushUndo] ); const subscribe = useCallback( @@ -304,6 +316,12 @@ export const useDocument = ( subscription.load(); return () => { unsub(); + // Fire-and-forget flush: if there are pending local edits and the + // subscription is being torn down due to a path change, attempt a + // final sync so the edits reach Firestore. The subscription's + // `stopped` flag prevents the async completion from re-registering + // a stale sync key after stop(). + void subscription.sync(); subscription.stop(); }; }, @@ -346,10 +364,11 @@ export interface UseCollectionOptions { /** * Hook to subscribe to a Firestore collection with real-time updates. * - * The subscription is keyed on the resolved collection path, `readOnly`, and - * the *semantic identity* of `queryConstraints`. When any of these change, the - * listener is torn down and re-attached with the new query. Toggling - * `undoable` does not rebuild the subscription. + * The subscription is keyed on the resolved collection path and the *semantic + * identity* of `queryConstraints`. When either changes, the listener is torn + * down and re-attached with the new query. Toggling `readOnly` or `undoable` + * does not rebuild the subscription; `readOnly` is applied at mutation time + * via a live ref. * * **You do not need to memoize `queryConstraints`.** `QueryConstraint` objects * are opaque, so Firestate compares the *built query* with Firestore's own @@ -424,6 +443,12 @@ export const useCollection = ( const undoableRef = useRef(undoable); undoableRef.current = undoable; + // Hold readOnly in a ref for the same reason as in useDocument: toggling it + // should not rebuild the subscription or discard pending local state. + const readOnlyRef = useRef(readOnly); + readOnlyRef.current = readOnly; + const getReadOnly = useCallback(() => readOnlyRef.current, []); + const onPushUndo = useCallback( (undoAction: () => void, redoAction: () => void, opts?: UpdateOptions) => { if (!undoableRef.current) return; @@ -489,20 +514,16 @@ export const useCollection = ( store, definition, collectionPath, - readOnly, + getReadOnly, queryConstraints: stableConstraints, onPushUndo, }) : null, - [ - enabled, - store, - definition, - collectionPath, - readOnly, - stableConstraints, - onPushUndo, - ] + // readOnly is intentionally excluded: it is applied at mutation time via + // getReadOnly (a stable ref-backed getter) so that toggling it does not + // rebuild the subscription or discard pending local state. + // eslint-disable-next-line react-hooks/exhaustive-deps + [enabled, store, definition, collectionPath, stableConstraints, onPushUndo] ); const isLazy = definition.lazy ?? false; @@ -516,6 +537,10 @@ export const useCollection = ( } return () => { unsub(); + // Fire-and-forget flush: if there are pending local edits and the + // subscription is being torn down due to a path/query change, attempt + // a final sync so the edits reach Firestore. + void subscription.sync(); subscription.stop(); }; }, From d33814ee7c9549ee4fe6faa47eecf20af409164a Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 18 Jun 2026 22:56:19 +0000 Subject: [PATCH 2/2] fix: reset stopped flag in load()/startListener() after retryOnError teardown MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01EJWMe1UYgBbCjjM618GxKT --- src/collection.ts | 4 ++++ src/document.ts | 4 ++++ src/firestate.integration.test.ts | 32 +++++++++++++++++++++++++++++++ 3 files changed, 40 insertions(+) diff --git a/src/collection.ts b/src/collection.ts index 3a5ca58..99f71df 100644 --- a/src/collection.ts +++ b/src/collection.ts @@ -573,6 +573,10 @@ export const createCollectionSubscription = ( const startListener = () => { if (unsubscribeListener) return + // Clear the stopped flag so notify() resumes reporting sync state. + // Needed for the retryOnError path: stop() sets stopped=true, then + // startListener() re-activates the subscription. + stopped = false loaded = false minLoadTimeElapsed = false diff --git a/src/document.ts b/src/document.ts index 1ab61d9..68c96f3 100644 --- a/src/document.ts +++ b/src/document.ts @@ -576,6 +576,10 @@ export const createDocumentSubscription = ( const load = () => { if (unsubscribeListener) return + // Clear the stopped flag so notify() resumes reporting sync state. + // Needed for the retryOnError path: stop() sets stopped=true, then + // load() re-activates the subscription. + stopped = false loaded = false minLoadTimeElapsed = false diff --git a/src/firestate.integration.test.ts b/src/firestate.integration.test.ts index 42e7b7f..85aabb8 100644 --- a/src/firestate.integration.test.ts +++ b/src/firestate.integration.test.ts @@ -529,4 +529,36 @@ describe('Issue #26: readOnly as live ref / flush on teardown', () => { await syncPromise expect(store.isSynced).toBe(true) }) + + // Part 3: stopped flag must be reset on retry so isSynced tracking resumes + + it('load() after stop() resets stopped so reportSyncState is called again', () => { + // Regression for the retryOnError path: stop() sets stopped=true, + // then load() re-activates. Without the reset, notify() would skip + // reportSyncState forever, leaving useIsSynced stuck after a retry. + const definition = buildDocumentDefinition( + doc({ path: 'tasks/{taskId}', schema: taskSchema }) + ) + + const sub = createDocumentSubscription({ + store, + definition, + docId: 't1', + collectionPath: 'tasks', + }) + sub.load() + fireDocSnapshot({ title: 'original' }) + + // Simulate retryOnError teardown + reload + sub.stop() + expect(store.isSynced).toBe(true) + + sub.load() + fireDocSnapshot({ title: 'original' }) + + // After reload an edit must flip isSynced back to false + sub.getHandle().update({ title: 'after-retry' }) + expect(store.isSynced).toBe(false) + expect(sub.getState().isSynced).toBe(false) + }) })