From 1a66e5704334d7631369496694cbcd3e262e16c9 Mon Sep 17 00:00:00 2001 From: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Date: Wed, 28 Oct 2026 15:07:28 +0000 Subject: [PATCH 1/5] [HSF-476] fix: batch snapshot notifications to stop React #185 on write bursts Co-Authored-By: Andrew Krippner --- AGENTS.md | 7 + docs/architecture.md | 4 + src/__tests__/snapshot-batching.test.ts | 176 ++++++++++++++++++++++++ src/core/collection.ts | 29 +++- src/core/document.ts | 31 ++++- src/core/snapshot-batcher.ts | 41 ++++++ 6 files changed, 279 insertions(+), 9 deletions(-) create mode 100644 src/__tests__/snapshot-batching.test.ts create mode 100644 src/core/snapshot-batcher.ts diff --git a/AGENTS.md b/AGENTS.md index 56f5d5f..634f915 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -193,6 +193,12 @@ Preserve these unless the task explicitly changes them. - Unmounting the last subscriber clears the shared autosave timer and unregisters its sync state. Pending debounced edits are not automatically flushed on `stop()`. +- Listener snapshots update state (`getState()`/`getHandle()`) at once, but + the subscriber fan-out is batched per task (`src/core/snapshot-batcher.ts`): + the first snapshot of a task notifies synchronously, and later snapshots in + the same task fold into one notify on the next task. A burst of server + writes otherwise drives one synchronous React commit per snapshot and throws + React #185 (maximum update depth). Local writes still notify synchronously. - Undo actions are client-local. Grouped undo actions undo newest to oldest and redo oldest to newest. - Firestore updates use flattened diffs for `updateDoc`; full document @@ -230,6 +236,7 @@ Add or update focused tests near the behavior being changed: - Diff behavior: `src/utils/diff.test.ts`. - Structural sharing and dev-mode freezing (identity across edits, snapshots, and rebases): `src/__tests__/structural-sharing.test.ts`. +- Snapshot burst batching: `src/__tests__/snapshot-batching.test.ts`. - Undo behavior: `src/utils/undo.test.ts`. - Store/global sync behavior: `src/core/store.test.ts`. - React hook surface — selectors, shared subscriptions, `queryConstraints` diff --git a/docs/architecture.md b/docs/architecture.md index de091e7..1d4251f 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -258,6 +258,10 @@ Important details: - Subscription state and handles are both cached until state changes (`getState()` and `getHandle()` return identity-stable references, rebuilt only on `notify()`), so React sees stable snapshots between commits. +- Listener snapshots publish state at once but batch the subscriber fan-out per + task: the first snapshot of a task notifies, and later snapshots in the same + task fold into one notify on the next task. This keeps a burst of server + writes from driving 50+ synchronous React commits (React #185). ## Document Subscriptions diff --git a/src/__tests__/snapshot-batching.test.ts b/src/__tests__/snapshot-batching.test.ts new file mode 100644 index 0000000..911e5db --- /dev/null +++ b/src/__tests__/snapshot-batching.test.ts @@ -0,0 +1,176 @@ +/** + * Snapshot batching: a burst of listener snapshots in one task updates state + * at once but notifies subscribers at most twice (the first snapshot, then one + * trailing notify on the next task). Without it, each snapshot drove one + * synchronous React commit, and 50+ of them in a row threw React #185 + * ("Maximum update depth exceeded"). + */ +import { vi, describe, it, expect, beforeEach } from 'vitest' + +vi.mock('firebase/firestore', async () => { + const actual = + await vi.importActual( + 'firebase/firestore' + ) + const { buildFirestoreMock } = await import('./test-harness') + return buildFirestoreMock(actual as unknown as Record) +}) + +import { createHarness, type Harness } from './test-harness' +import { createCollectionSubscription } from '../core/collection' +import { createDocumentSubscription } from '../core/document' +import { defineCollection, defineDocument } from '../registry/schema' +import { createStore, type FirestateStore } from '../core/store' + +interface Space { + id?: string + processed?: boolean +} + +interface Project { + revision?: number +} + +const spaces = defineCollection({ path: 'spaces' }) +const projectDef = defineDocument({ collection: 'projects', id: 'p1' }) + +const nextTask = () => new Promise((resolve) => setTimeout(resolve, 0)) + +const BURST_SIZE = 100 + +const spacesProcessedUpTo = (processedCount: number) => + Object.fromEntries( + Array.from({ length: BURST_SIZE }, (_, index) => [ + `space${index}`, + { processed: index < processedCount }, + ]) + ) + +describe('snapshot batching', () => { + let store: FirestateStore + let h: Harness + + beforeEach(() => { + vi.clearAllMocks() + store = createStore({ firestore: {} as never, autosave: 0 }) + h = createHarness() + }) + + const makeColl = async () => { + const sub = createCollectionSubscription({ + store, + definition: spaces, + collectionPath: 'spaces', + }) + sub.load() + h.fireCollectionSnapshot(spacesProcessedUpTo(0)) + await nextTask() + return sub + } + + const makeDoc = async () => { + const sub = createDocumentSubscription({ + store, + definition: projectDef, + docId: 'p1', + collectionPath: 'projects', + }) + sub.load() + h.fireDocSnapshot({ revision: 0 }) + await nextTask() + return sub + } + + describe('collection', () => { + it('notifies a single snapshot synchronously', async () => { + const sub = await makeColl() + const subscriber = vi.fn() + sub.subscribe(subscriber) + + h.fireCollectionSnapshot(spacesProcessedUpTo(1)) + + expect(subscriber).toHaveBeenCalledTimes(1) + await nextTask() + expect(subscriber).toHaveBeenCalledTimes(1) + }) + + it('folds a burst of snapshots into one trailing notify', async () => { + const sub = await makeColl() + const subscriber = vi.fn() + sub.subscribe(subscriber) + + for (let count = 1; count <= BURST_SIZE; count++) { + h.fireCollectionSnapshot(spacesProcessedUpTo(count)) + } + + expect(subscriber).toHaveBeenCalledTimes(1) + expect(sub.getState().data.space99?.processed).toBe(true) + + await nextTask() + expect(subscriber).toHaveBeenCalledTimes(2) + expect(subscriber).toHaveBeenLastCalledWith(sub.getState()) + }) + + it('folds a burst delivered through a microtask chain', async () => { + const sub = await makeColl() + const subscriber = vi.fn() + sub.subscribe(subscriber) + + for (let count = 1; count <= BURST_SIZE; count++) { + await Promise.resolve() + h.fireCollectionSnapshot(spacesProcessedUpTo(count)) + } + await nextTask() + + expect(subscriber).toHaveBeenCalledTimes(2) + expect(sub.getState().data.space99?.processed).toBe(true) + }) + + it('drops the trailing notify after stop()', async () => { + const sub = await makeColl() + const subscriber = vi.fn() + sub.subscribe(subscriber) + + h.fireCollectionSnapshot(spacesProcessedUpTo(1)) + h.fireCollectionSnapshot(spacesProcessedUpTo(2)) + sub.stop() + await nextTask() + + expect(subscriber).toHaveBeenCalledTimes(1) + }) + + it('keeps local updates synchronous during a burst', async () => { + const sub = await makeColl() + const subscriber = vi.fn() + sub.subscribe(subscriber) + + h.fireCollectionSnapshot(spacesProcessedUpTo(1)) + h.fireCollectionSnapshot(spacesProcessedUpTo(2)) + sub.getHandle().update({ space50: { processed: true } }) + + expect(subscriber).toHaveBeenCalledTimes(2) + expect(subscriber).toHaveBeenLastCalledWith(sub.getState()) + expect(sub.getState().data.space1?.processed).toBe(true) + expect(sub.getState().data.space50?.processed).toBe(true) + }) + }) + + describe('document', () => { + it('folds a burst of snapshots into one trailing notify', async () => { + const sub = await makeDoc() + const subscriber = vi.fn() + sub.subscribe(subscriber) + + for (let revision = 1; revision <= BURST_SIZE; revision++) { + h.fireDocSnapshot({ revision }) + } + + expect(subscriber).toHaveBeenCalledTimes(1) + expect(sub.getState().data?.revision).toBe(BURST_SIZE) + + await nextTask() + expect(subscriber).toHaveBeenCalledTimes(2) + expect(subscriber).toHaveBeenLastCalledWith(sub.getState()) + }) + }) +}) diff --git a/src/core/collection.ts b/src/core/collection.ts index a062027..b379375 100644 --- a/src/core/collection.ts +++ b/src/core/collection.ts @@ -22,6 +22,7 @@ import type { UpdateOptions, } from '../types' import type { FirestateStore } from './store' +import { createSnapshotBatcher } from './snapshot-batcher' import { applyDiff, applyOverridesAtPaths, @@ -257,7 +258,9 @@ export const createCollectionSubscription = ( prev.isActive !== next.isActive || observableStateChanged(prev, next) - const notify = () => { + // Makes the current state visible to getState()/getHandle(). Returns + // whether anything observable changed. + const publishState = (): boolean => { // State is structurally shared between syncState, localState, and // every published snapshot, so it must never be mutated in place. freezeDeepInDevelopment(state.syncState) @@ -272,14 +275,31 @@ export const createCollectionSubscription = ( // Snapshot-side no-op collapse: nothing observable changed → publish // nothing, keeping the cached handle identity stable. if (lastPublished !== null && !publicStateChanged(lastPublished, publicState)) { - return + return false } lastPublished = publicState cachedHandle = null // Reuse the published state as the cached snapshot (see document.ts). cachedState = publicState - subscribers.forEach((fn) => fn(publicState)) store.reportSyncState(syncKey, publicState.isSynced) + return true + } + + const notifySubscribers = () => { + const publicState = getState() + subscribers.forEach((fn) => fn(publicState)) + } + + const notify = () => { + if (publishState()) notifySubscribers() + } + + // Snapshots update state at once but batch the subscriber fan-out, so a + // burst of snapshots in one task re-renders consumers at most twice. + const snapshotBatcher = createSnapshotBatcher(notifySubscribers) + + const notifySnapshot = () => { + if (publishState()) snapshotBatcher.publishSnapshot() } // Pre-snapshot mutations are unsafe because computing a partial-update @@ -663,7 +683,7 @@ export const createCollectionSubscription = ( scheduleAutosave() } - notify() + notifySnapshot() } const handleError = (error: Error) => { @@ -752,6 +772,7 @@ export const createCollectionSubscription = ( clearTimeout(minLoadTimeout) minLoadTimeout = null } + snapshotBatcher.cancel() // Drop this subscription's entry from the global sync-state map so // an unmounted hook does not leave useIsSynced stuck at false. store.unregisterSyncState(syncKey) diff --git a/src/core/document.ts b/src/core/document.ts index 40fbf61..be20b00 100644 --- a/src/core/document.ts +++ b/src/core/document.ts @@ -20,6 +20,7 @@ import type { UpdateOptions, } from '../types' import type { FirestateStore } from './store' +import { createSnapshotBatcher } from './snapshot-batcher' import { applyDiff, applyOverridesAtPaths, @@ -240,7 +241,9 @@ export const createDocumentSubscription = ( const publicStateChanged = observableStateChanged - const notify = () => { + // Makes the current state visible to getState()/getHandle(). Returns + // whether anything observable changed. + const publishState = (): boolean => { // State is structurally shared between syncState, localState, and // every published snapshot, so it must never be mutated in place. freezeDeepInDevelopment(state.syncState) @@ -260,15 +263,32 @@ export const createDocumentSubscription = ( // changed → publish nothing. Keeps the cached handle identity stable // so useSyncExternalStore does not re-render. if (lastPublished !== null && !publicStateChanged(lastPublished, publicState)) { - return + return false } lastPublished = publicState cachedHandle = null // Reuse the just-built state as the cached snapshot so getState() and // the published value share one identity-stable reference. cachedState = publicState - subscribers.forEach((fn) => fn(publicState)) store.reportSyncState(syncKey, publicState.isSynced) + return true + } + + const notifySubscribers = () => { + const publicState = getState() + subscribers.forEach((fn) => fn(publicState)) + } + + const notify = () => { + if (publishState()) notifySubscribers() + } + + // Snapshots update state at once but batch the subscriber fan-out, so a + // burst of snapshots in one task re-renders consumers at most twice. + const snapshotBatcher = createSnapshotBatcher(notifySubscribers) + + const notifySnapshot = () => { + if (publishState()) snapshotBatcher.publishSnapshot() } const updateState = ( @@ -623,7 +643,7 @@ export const createDocumentSubscription = ( scheduleAutosave() } - notify() + notifySnapshot() } // A document that does not exist is not an error condition — consumers @@ -658,7 +678,7 @@ export const createDocumentSubscription = ( state.isLoading = false } loaded = true - notify() + notifySnapshot() } const handleError = (error: Error) => { @@ -731,6 +751,7 @@ export const createDocumentSubscription = ( clearTimeout(minLoadTimeout) minLoadTimeout = null } + snapshotBatcher.cancel() // Drop this subscription's entry from the global sync-state map so // an unmounted hook does not leave useIsSynced stuck at false. store.unregisterSyncState(syncKey) diff --git a/src/core/snapshot-batcher.ts b/src/core/snapshot-batcher.ts new file mode 100644 index 0000000..55c6d08 --- /dev/null +++ b/src/core/snapshot-batcher.ts @@ -0,0 +1,41 @@ +/** + * Firestore can deliver many listener snapshots in one task: its queue runs as + * a promise chain, so a burst of server writes (e.g. a Cloud Function that + * updates 100+ documents one by one) arrives back to back. Publishing each one + * makes React run one synchronous `useSyncExternalStore` commit per snapshot. + * When a consumer's effect schedules more work after each commit, React counts + * the commits as nested updates and throws "Maximum update depth exceeded" + * (#185) after 50 of them. + * + * The batcher publishes the first snapshot of a task right away, so a single + * snapshot stays synchronous. It folds every later snapshot of the same task + * into one publish on the next task. + */ +export const createSnapshotBatcher = (publish: () => void) => { + let windowTimeout: ReturnType | null = null + let hasDeferredSnapshot = false + + const publishSnapshot = () => { + if (windowTimeout !== null) { + hasDeferredSnapshot = true + return + } + publish() + windowTimeout = setTimeout(() => { + windowTimeout = null + if (!hasDeferredSnapshot) return + hasDeferredSnapshot = false + publish() + }, 0) + } + + const cancel = () => { + if (windowTimeout !== null) { + clearTimeout(windowTimeout) + windowTimeout = null + } + hasDeferredSnapshot = false + } + + return { publishSnapshot, cancel } +} From a8c34af1e429d3714e634a010a612bf3630dda4a Mon Sep 17 00:00:00 2001 From: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Date: Wed, 28 Oct 2026 15:10:14 +0000 Subject: [PATCH 2/5] fix: skip the trailing snapshot notify after a local notify Co-Authored-By: Andrew Krippner --- src/__tests__/snapshot-batching.test.ts | 21 +++++++++++++++++++++ src/core/collection.ts | 4 +++- src/core/document.ts | 4 +++- src/core/snapshot-batcher.ts | 8 +++++++- 4 files changed, 34 insertions(+), 3 deletions(-) diff --git a/src/__tests__/snapshot-batching.test.ts b/src/__tests__/snapshot-batching.test.ts index 911e5db..ec8b2b4 100644 --- a/src/__tests__/snapshot-batching.test.ts +++ b/src/__tests__/snapshot-batching.test.ts @@ -126,6 +126,24 @@ describe('snapshot batching', () => { expect(sub.getState().data.space99?.processed).toBe(true) }) + it('folds a backlog of snapshots queued as separate tasks', async () => { + const sub = await makeColl() + const subscriber = vi.fn() + sub.subscribe(subscriber) + + for (let count = 1; count <= BURST_SIZE; count++) { + setTimeout(() => { + h.fireCollectionSnapshot(spacesProcessedUpTo(count)) + }, 0) + } + await nextTask() + await nextTask() + + expect(subscriber).toHaveBeenCalledTimes(2) + expect(subscriber).toHaveBeenLastCalledWith(sub.getState()) + expect(sub.getState().data.space99?.processed).toBe(true) + }) + it('drops the trailing notify after stop()', async () => { const sub = await makeColl() const subscriber = vi.fn() @@ -152,6 +170,9 @@ describe('snapshot batching', () => { expect(subscriber).toHaveBeenLastCalledWith(sub.getState()) expect(sub.getState().data.space1?.processed).toBe(true) expect(sub.getState().data.space50?.processed).toBe(true) + + await nextTask() + expect(subscriber).toHaveBeenCalledTimes(2) }) }) diff --git a/src/core/collection.ts b/src/core/collection.ts index b379375..2efd96f 100644 --- a/src/core/collection.ts +++ b/src/core/collection.ts @@ -291,7 +291,9 @@ export const createCollectionSubscription = ( } const notify = () => { - if (publishState()) notifySubscribers() + if (!publishState()) return + snapshotBatcher.discardDeferred() + notifySubscribers() } // Snapshots update state at once but batch the subscriber fan-out, so a diff --git a/src/core/document.ts b/src/core/document.ts index be20b00..f461dac 100644 --- a/src/core/document.ts +++ b/src/core/document.ts @@ -280,7 +280,9 @@ export const createDocumentSubscription = ( } const notify = () => { - if (publishState()) notifySubscribers() + if (!publishState()) return + snapshotBatcher.discardDeferred() + notifySubscribers() } // Snapshots update state at once but batch the subscriber fan-out, so a diff --git a/src/core/snapshot-batcher.ts b/src/core/snapshot-batcher.ts index 55c6d08..a416225 100644 --- a/src/core/snapshot-batcher.ts +++ b/src/core/snapshot-batcher.ts @@ -37,5 +37,11 @@ export const createSnapshotBatcher = (publish: () => void) => { hasDeferredSnapshot = false } - return { publishSnapshot, cancel } + // A synchronous notify already delivered the latest state, so a deferred + // snapshot has nothing new to publish. + const discardDeferred = () => { + hasDeferredSnapshot = false + } + + return { publishSnapshot, discardDeferred, cancel } } From 1c36ccd5577bbbfe3046d3678f0a02be8e57244b Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 28 Oct 2026 21:52:21 +0000 Subject: [PATCH 3/5] fix: share the snapshot batching window across a store's subscriptions The batcher was per subscription, so a burst that lands as one snapshot on each of many document subscriptions still notified every one synchronously and still threw React #185. One batcher per store now folds them into one trailing notify. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Qwc3zMrUoB5ykW8WFppENr --- AGENTS.md | 4 +- docs/architecture.md | 3 +- src/__tests__/snapshot-batching.test.ts | 77 +++++++++++++++++++++++++ src/core/collection.ts | 15 ++--- src/core/document.ts | 15 ++--- src/core/snapshot-batcher.ts | 52 ++++++++++------- 6 files changed, 130 insertions(+), 36 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 634f915..f4bc5d5 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -196,7 +196,9 @@ Preserve these unless the task explicitly changes them. - Listener snapshots update state (`getState()`/`getHandle()`) at once, but the subscriber fan-out is batched per task (`src/core/snapshot-batcher.ts`): the first snapshot of a task notifies synchronously, and later snapshots in - the same task fold into one notify on the next task. A burst of server + the same task fold into one notify on the next task. The batching window is + shared by every subscription on a store, so one snapshot each across many + document subscriptions folds the same way. A burst of server writes otherwise drives one synchronous React commit per snapshot and throws React #185 (maximum update depth). Local writes still notify synchronously. - Undo actions are client-local. Grouped undo actions undo newest to oldest and diff --git a/docs/architecture.md b/docs/architecture.md index 1d4251f..d559fa0 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -260,7 +260,8 @@ Important details: so React sees stable snapshots between commits. - Listener snapshots publish state at once but batch the subscriber fan-out per task: the first snapshot of a task notifies, and later snapshots in the same - task fold into one notify on the next task. This keeps a burst of server + task fold into one notify on the next task. The window is shared per store, + across subscriptions. This keeps a burst of server writes from driving 50+ synchronous React commits (React #185). ## Document Subscriptions diff --git a/src/__tests__/snapshot-batching.test.ts b/src/__tests__/snapshot-batching.test.ts index ec8b2b4..ebdef14 100644 --- a/src/__tests__/snapshot-batching.test.ts +++ b/src/__tests__/snapshot-batching.test.ts @@ -194,4 +194,81 @@ describe('snapshot batching', () => { expect(subscriber).toHaveBeenLastCalledWith(sub.getState()) }) }) + + describe('across subscriptions', () => { + const docSnap = (data: object) => ({ + exists: () => true, + data: () => data, + metadata: { fromCache: false, hasPendingWrites: false }, + }) + + // One snapshot per document subscription commits as often as a burst + // on one subscription, so the batching window is shared per store. + it('folds one snapshot each across many document subscriptions', async () => { + const subs = Array.from({ length: BURST_SIZE }, (_, index) => + createDocumentSubscription({ + store, + definition: projectDef, + docId: `p${index}`, + collectionPath: 'projects', + }) + ) + subs.forEach((sub) => sub.load()) + const listeners = h.listeners() + listeners.forEach((l) => l.onNext(docSnap({ revision: 0 }))) + await nextTask() + + const subscribers = subs.map((sub) => { + const subscriber = vi.fn() + sub.subscribe(subscriber) + return subscriber + }) + const totalCalls = () => + subscribers.reduce((n, fn) => n + fn.mock.calls.length, 0) + + for (const listener of listeners) { + await Promise.resolve() + listener.onNext(docSnap({ revision: 1 })) + } + + expect(totalCalls()).toBe(1) + expect( + subs.every((sub) => sub.getState().data?.revision === 1) + ).toBe(true) + + await nextTask() + subscribers.forEach((subscriber, index) => { + expect(subscriber).toHaveBeenCalledTimes(1) + expect(subscriber).toHaveBeenLastCalledWith( + subs[index]!.getState() + ) + }) + }) + + it('drops a stopped subscription from the shared trailing notify', async () => { + const a = await makeDoc() + const b = createDocumentSubscription({ + store, + definition: projectDef, + docId: 'p2', + collectionPath: 'projects', + }) + b.load() + h.fireDocSnapshot({ revision: 0 }) + await nextTask() + + const aSubscriber = vi.fn() + const bSubscriber = vi.fn() + a.subscribe(aSubscriber) + b.subscribe(bSubscriber) + + h.listeners()[0]!.onNext(docSnap({ revision: 1 })) + h.fireDocSnapshot({ revision: 1 }) + b.stop() + await nextTask() + + expect(aSubscriber).toHaveBeenCalledTimes(1) + expect(bSubscriber).not.toHaveBeenCalled() + }) + }) }) diff --git a/src/core/collection.ts b/src/core/collection.ts index 2efd96f..ec052c9 100644 --- a/src/core/collection.ts +++ b/src/core/collection.ts @@ -22,7 +22,7 @@ import type { UpdateOptions, } from '../types' import type { FirestateStore } from './store' -import { createSnapshotBatcher } from './snapshot-batcher' +import { getSnapshotBatcher } from './snapshot-batcher' import { applyDiff, applyOverridesAtPaths, @@ -292,16 +292,17 @@ export const createCollectionSubscription = ( const notify = () => { if (!publishState()) return - snapshotBatcher.discardDeferred() + snapshotBatcher.discardDeferred(notifySubscribers) notifySubscribers() } - // Snapshots update state at once but batch the subscriber fan-out, so a - // burst of snapshots in one task re-renders consumers at most twice. - const snapshotBatcher = createSnapshotBatcher(notifySubscribers) + // Snapshots update state at once but batch the subscriber fan-out across + // the store, so a burst of snapshots in one task re-renders consumers at + // most twice. + const snapshotBatcher = getSnapshotBatcher(store) const notifySnapshot = () => { - if (publishState()) snapshotBatcher.publishSnapshot() + if (publishState()) snapshotBatcher.publishSnapshot(notifySubscribers) } // Pre-snapshot mutations are unsafe because computing a partial-update @@ -774,7 +775,7 @@ export const createCollectionSubscription = ( clearTimeout(minLoadTimeout) minLoadTimeout = null } - snapshotBatcher.cancel() + snapshotBatcher.discardDeferred(notifySubscribers) // Drop this subscription's entry from the global sync-state map so // an unmounted hook does not leave useIsSynced stuck at false. store.unregisterSyncState(syncKey) diff --git a/src/core/document.ts b/src/core/document.ts index f461dac..9ccec5a 100644 --- a/src/core/document.ts +++ b/src/core/document.ts @@ -20,7 +20,7 @@ import type { UpdateOptions, } from '../types' import type { FirestateStore } from './store' -import { createSnapshotBatcher } from './snapshot-batcher' +import { getSnapshotBatcher } from './snapshot-batcher' import { applyDiff, applyOverridesAtPaths, @@ -281,16 +281,17 @@ export const createDocumentSubscription = ( const notify = () => { if (!publishState()) return - snapshotBatcher.discardDeferred() + snapshotBatcher.discardDeferred(notifySubscribers) notifySubscribers() } - // Snapshots update state at once but batch the subscriber fan-out, so a - // burst of snapshots in one task re-renders consumers at most twice. - const snapshotBatcher = createSnapshotBatcher(notifySubscribers) + // Snapshots update state at once but batch the subscriber fan-out across + // the store, so a burst of snapshots in one task re-renders consumers at + // most twice. + const snapshotBatcher = getSnapshotBatcher(store) const notifySnapshot = () => { - if (publishState()) snapshotBatcher.publishSnapshot() + if (publishState()) snapshotBatcher.publishSnapshot(notifySubscribers) } const updateState = ( @@ -753,7 +754,7 @@ export const createDocumentSubscription = ( clearTimeout(minLoadTimeout) minLoadTimeout = null } - snapshotBatcher.cancel() + snapshotBatcher.discardDeferred(notifySubscribers) // Drop this subscription's entry from the global sync-state map so // an unmounted hook does not leave useIsSynced stuck at false. store.unregisterSyncState(syncKey) diff --git a/src/core/snapshot-batcher.ts b/src/core/snapshot-batcher.ts index a416225..f103036 100644 --- a/src/core/snapshot-batcher.ts +++ b/src/core/snapshot-batcher.ts @@ -1,3 +1,5 @@ +import type { FirestateStore } from './store' + /** * Firestore can deliver many listener snapshots in one task: its queue runs as * a promise chain, so a burst of server writes (e.g. a Cloud Function that @@ -9,39 +11,49 @@ * * The batcher publishes the first snapshot of a task right away, so a single * snapshot stays synchronous. It folds every later snapshot of the same task - * into one publish on the next task. + * into one publish per subscription on the next task. One batcher is shared by + * every subscription on a store, because a burst spread across many document + * subscriptions (one snapshot each) commits just as often as a burst on one. */ -export const createSnapshotBatcher = (publish: () => void) => { +export const createSnapshotBatcher = () => { let windowTimeout: ReturnType | null = null - let hasDeferredSnapshot = false + const deferred = new Set<() => void>() - const publishSnapshot = () => { + const publishSnapshot = (publish: () => void) => { if (windowTimeout !== null) { - hasDeferredSnapshot = true + deferred.add(publish) return } publish() windowTimeout = setTimeout(() => { windowTimeout = null - if (!hasDeferredSnapshot) return - hasDeferredSnapshot = false - publish() + const pending = [...deferred] + deferred.clear() + // Publishing in one synchronous loop lets React batch them into a + // single commit. + pending.forEach((fn) => fn()) }, 0) } - const cancel = () => { - if (windowTimeout !== null) { - clearTimeout(windowTimeout) - windowTimeout = null - } - hasDeferredSnapshot = false + // Called when a synchronous notify already delivered the latest state, or + // when the subscription stops, so its deferred snapshot has nothing to do. + const discardDeferred = (publish: () => void) => { + deferred.delete(publish) } - // A synchronous notify already delivered the latest state, so a deferred - // snapshot has nothing new to publish. - const discardDeferred = () => { - hasDeferredSnapshot = false - } + return { publishSnapshot, discardDeferred } +} + +export type SnapshotBatcher = ReturnType - return { publishSnapshot, discardDeferred, cancel } +const batchers = new WeakMap() + +/** The snapshot batcher shared by every subscription on `store`. */ +export const getSnapshotBatcher = (store: FirestateStore): SnapshotBatcher => { + let batcher = batchers.get(store) + if (!batcher) { + batcher = createSnapshotBatcher() + batchers.set(store, batcher) + } + return batcher } From 3376b4481db7150ce0847da201726a0e601778ca Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 28 Oct 2026 21:58:59 +0000 Subject: [PATCH 4/5] fix: keep notifying other subscriptions when one subscriber throws The shared trailing notify ran every deferred subscription in one loop, so a throw from one subscriber (React #185 is thrown from inside the callback) skipped the rest and left their consumers stale. Each one now runs, and the first error is rethrown afterwards. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Qwc3zMrUoB5ykW8WFppENr --- src/__tests__/snapshot-batching.test.ts | 49 +++++++++++++++++++++++++ src/core/snapshot-batcher.ts | 13 ++++++- 2 files changed, 60 insertions(+), 2 deletions(-) diff --git a/src/__tests__/snapshot-batching.test.ts b/src/__tests__/snapshot-batching.test.ts index ebdef14..6167493 100644 --- a/src/__tests__/snapshot-batching.test.ts +++ b/src/__tests__/snapshot-batching.test.ts @@ -245,6 +245,55 @@ describe('snapshot batching', () => { }) }) + // React throws #185 from inside the subscriber callback, so one + // subscription's throw must not strand the rest of the shared flush. + it('notifies every subscription when one subscriber throws', async () => { + const a = await makeDoc() + const b = createDocumentSubscription({ + store, + definition: projectDef, + docId: 'p2', + collectionPath: 'projects', + }) + b.load() + h.fireDocSnapshot({ revision: 0 }) + await nextTask() + + // Open the window with a third subscription so a and b both defer. + const c = createDocumentSubscription({ + store, + definition: projectDef, + docId: 'p3', + collectionPath: 'projects', + }) + c.load() + h.fireDocSnapshot({ revision: 0 }) + + const thrown = new Error('subscriber failed') + a.subscribe(() => { + throw thrown + }) + const bSubscriber = vi.fn() + b.subscribe(bSubscriber) + + h.listeners()[0]!.onNext(docSnap({ revision: 1 })) + h.listeners()[1]!.onNext(docSnap({ revision: 1 })) + + const uncaught: unknown[] = [] + const onUncaught = (error: unknown) => uncaught.push(error) + process.on('uncaughtException', onUncaught) + try { + await nextTask() + await nextTask() + } finally { + process.off('uncaughtException', onUncaught) + } + + expect(bSubscriber).toHaveBeenCalledTimes(1) + expect(bSubscriber).toHaveBeenLastCalledWith(b.getState()) + expect(uncaught).toEqual([thrown]) + }) + it('drops a stopped subscription from the shared trailing notify', async () => { const a = await makeDoc() const b = createDocumentSubscription({ diff --git a/src/core/snapshot-batcher.ts b/src/core/snapshot-batcher.ts index f103036..e9b1f1f 100644 --- a/src/core/snapshot-batcher.ts +++ b/src/core/snapshot-batcher.ts @@ -30,8 +30,17 @@ export const createSnapshotBatcher = () => { const pending = [...deferred] deferred.clear() // Publishing in one synchronous loop lets React batch them into a - // single commit. - pending.forEach((fn) => fn()) + // single commit. A throw (e.g. React #185 from inside a subscriber) + // must not strand the other subscriptions, so it is rethrown last. + let firstError: { error: unknown } | null = null + for (const fn of pending) { + try { + fn() + } catch (error) { + firstError ??= { error } + } + } + if (firstError) throw firstError.error }, 0) } From 7a58a0a6cd67df1bc41ced2965c5171d96e5e083 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 28 Oct 2026 22:45:05 +0000 Subject: [PATCH 5/5] fix: open the batching window before the first notify If the first snapshot's notify threw (React #185 is thrown from inside the subscriber), the window never opened and every later snapshot in the burst notified synchronously, the exact pattern the batcher exists to stop. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Qwc3zMrUoB5ykW8WFppENr --- src/__tests__/snapshot-batching.test.ts | 30 +++++++++++++++++++++++++ src/core/snapshot-batcher.ts | 4 +++- 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/src/__tests__/snapshot-batching.test.ts b/src/__tests__/snapshot-batching.test.ts index 6167493..dfcd829 100644 --- a/src/__tests__/snapshot-batching.test.ts +++ b/src/__tests__/snapshot-batching.test.ts @@ -144,6 +144,36 @@ describe('snapshot batching', () => { expect(sub.getState().data.space99?.processed).toBe(true) }) + // React #185 is thrown from inside the first synchronous notify; the + // rest of the burst must still be batched, not notified one by one. + it('keeps batching when the first notify throws', async () => { + const sub = await makeColl() + const thrown = new Error('subscriber failed') + const subscriber = vi.fn(() => { + throw thrown + }) + sub.subscribe(subscriber) + + for (let count = 1; count <= BURST_SIZE; count++) { + try { + h.fireCollectionSnapshot(spacesProcessedUpTo(count)) + } catch (error) { + expect(error).toBe(thrown) + } + } + expect(subscriber).toHaveBeenCalledTimes(1) + + const onUncaught = () => {} + process.on('uncaughtException', onUncaught) + try { + await nextTask() + await nextTask() + } finally { + process.off('uncaughtException', onUncaught) + } + expect(subscriber).toHaveBeenCalledTimes(2) + }) + it('drops the trailing notify after stop()', async () => { const sub = await makeColl() const subscriber = vi.fn() diff --git a/src/core/snapshot-batcher.ts b/src/core/snapshot-batcher.ts index e9b1f1f..905fab9 100644 --- a/src/core/snapshot-batcher.ts +++ b/src/core/snapshot-batcher.ts @@ -24,7 +24,8 @@ export const createSnapshotBatcher = () => { deferred.add(publish) return } - publish() + // Open the window before publishing: if publish throws (React #185), + // the rest of the burst must still be batched. windowTimeout = setTimeout(() => { windowTimeout = null const pending = [...deferred] @@ -42,6 +43,7 @@ export const createSnapshotBatcher = () => { } if (firstError) throw firstError.error }, 0) + publish() } // Called when a synchronous notify already delivered the latest state, or