From 011aa5165e7c75ab7c1893c94246f5b16fd9a871 Mon Sep 17 00:00:00 2001 From: "posthog[bot]" <206114724+posthog[bot]@users.noreply.github.com> Date: Sat, 8 Aug 2026 03:15:34 +0000 Subject: [PATCH] fix: attach resource context to reported listener errors and stop retrying permission-denied store.reportError now wraps every error in a FirestateError that carries type, path, operation, and the Firestore code on own fields, keeps the original on cause, and puts the path in the message. A consumer that forwards only the first argument to an error tracker keeps the context and gets a distinct fingerprint per resource. Context still travels as the second onError argument. The listener retry branch now treats a terminal Firestore code (permission-denied, unauthenticated) as terminal even when retryOnError is set: it reports the error, sets state.error, and clears isLoading instead of re-attaching the listener every retryInterval behind a spinner that never resolves. Only transient codes still retry. Generated-By: PostHog Code Task-Id: 22c2ca54-66e1-40e9-9799-2942e40ad4ac --- AGENTS.md | 10 ++ README.md | 33 +++++ src/__tests__/listener-error.test.ts | 212 +++++++++++++++++++++++++++ src/core/collection.ts | 32 ++-- src/core/document.ts | 32 ++-- src/core/errors.ts | 63 ++++++++ src/core/store.test.ts | 60 +++++++- src/core/store.ts | 43 +++--- src/index.ts | 3 + 9 files changed, 440 insertions(+), 48 deletions(-) create mode 100644 src/__tests__/listener-error.test.ts create mode 100644 src/core/errors.ts diff --git a/AGENTS.md b/AGENTS.md index 7b4baee..8d69029 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -101,6 +101,16 @@ Preserve these unless the task explicitly changes them. replace a document. - Collection `add`, `update`, and `remove` require the first snapshot. They bail before the initial snapshot to avoid clobbering unknown server fields. +- `store.reportError` wraps every error in a `FirestateError` before calling + `onError`. The wrapper carries `type`, `path`, `operation`, and (when present) + the Firestore `code` on own fields, puts the path in the message, and keeps + the original error on `cause`. This lets a consumer that forwards only the + first argument to a tracker keep the context and get a distinct fingerprint + per resource. Context still travels as the second `onError` argument. +- A listener error with a terminal Firestore code (`permission-denied`, + `unauthenticated`) is never retried, even with `retryOnError: true` — a retry + can never clear it. The handler reports it, sets `state.error`, and clears + `isLoading`. Only transient codes re-attach the listener on `retryInterval`. - `enabled: false` on hooks must not resolve paths or create subscriptions. It returns stable no-op handles. - `queryConstraints` are keyed by *semantic query identity*, not by array diff --git a/README.md b/README.md index c9a69c4..1650e79 100644 --- a/README.md +++ b/README.md @@ -1095,6 +1095,39 @@ subscription.load() subscription.stop() ``` +### Error Handling + +Firestate reports every error through `onError`. The first argument is always a +`FirestateError` that carries its own context, so a consumer that forwards only +the error to a tracker keeps a usable path and a distinct fingerprint per +resource: + +```typescript +import { FirestateError } from '@hvakr/firestate' + +const store = createStore({ + firestore: db, + onError: (error) => { + // error is a FirestateError with own fields: + // error.path → 'projects/123/tasks/t1' + // error.type → 'document' | 'collection' | 'undo' + // error.operation → 'read' | 'write' | 'undo' | 'redo' + // error.code → Firestore code, e.g. 'permission-denied' + // error.cause → the original FirebaseError + Sentry.captureException(error) + }, +}) +``` + +The `context` object still arrives as the second argument for consumers that +prefer it. + +`retryOnError: true` re-attaches a listener after a transient error (e.g. +`unavailable`). A non-transient code — `permission-denied` or +`unauthenticated` — is treated as terminal even so: Firestate reports it, sets +`state.error`, and clears the loading flag instead of retrying forever behind a +spinner. + ### Custom Undo Manager Create a standalone undo manager with navigation support: diff --git a/src/__tests__/listener-error.test.ts b/src/__tests__/listener-error.test.ts new file mode 100644 index 0000000..221e8ea --- /dev/null +++ b/src/__tests__/listener-error.test.ts @@ -0,0 +1,212 @@ +/** + * Listener error handling: what a document / collection subscription does when + * `onSnapshot` delivers an error. + * + * Two contracts are pinned here: + * + * - Reported errors carry context. `store.reportError` wraps the raw + * FirebaseError in a FirestateError whose message and own fields hold the + * resource path, so a consumer that forwards only the error to a tracker + * still gets a usable path and a distinct fingerprint per resource. + * - permission-denied is terminal. A retry can never clear it, so even with + * `retryOnError: true` the subscription reports it, sets `state.error`, and + * clears `isLoading` instead of re-attaching the listener forever. A + * transient code (e.g. `unavailable`) still retries. + */ +import { vi, describe, it, expect, beforeEach, afterEach } 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, firestoreError, 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' +import { FirestateError } from '../core/errors' +import type { ErrorContext } from '../types' + +interface Doc { + field1?: string +} + +interface Item { + id?: string + name?: string +} + +describe('listener error handling', () => { + let onError: ReturnType + let store: FirestateStore + let h: Harness + + beforeEach(() => { + vi.useFakeTimers() + vi.spyOn(console, 'error').mockImplementation(() => {}) + vi.spyOn(console, 'warn').mockImplementation(() => {}) + onError = vi.fn() + store = createStore({ firestore: {} as never, autosave: 0, onError }) + h = createHarness() + }) + + afterEach(() => { + vi.clearAllTimers() + vi.useRealTimers() + vi.restoreAllMocks() + }) + + const lastReported = () => { + const call = onError.mock.calls.at(-1) as + | [FirestateError, ErrorContext] + | undefined + return call?.[0] + } + + describe('reported error carries context', () => { + it('wraps a document listener error with its path', () => { + const def = defineDocument({ + collection: 'projects/p1/tasks', + id: 't1', + }) + const sub = createDocumentSubscription({ + store, + definition: def, + docId: 't1', + collectionPath: 'projects/p1/tasks', + }) + sub.load() + + h.fireListenerError(firestoreError('permission-denied')) + + const reported = lastReported() + expect(reported).toBeInstanceOf(FirestateError) + expect(reported?.type).toBe('document') + expect(reported?.path).toBe('projects/p1/tasks/t1') + expect(reported?.operation).toBe('read') + expect(reported?.code).toBe('permission-denied') + // The path is in the message, so a tracker fingerprints on it. + expect(reported?.message).toContain('projects/p1/tasks/t1') + sub.stop() + }) + + it('gives two resources distinct fingerprints', () => { + const mk = (path: string, id: string) => { + const sub = createDocumentSubscription({ + store, + definition: defineDocument({ collection: path, id }), + docId: id, + collectionPath: path, + }) + sub.load() + return sub + } + + const a = mk('projects/p1/tasks', 't1') + h.fireListenerError(firestoreError('permission-denied')) + const first = lastReported()?.message + + const b = mk('projects/p2/notes', 'n9') + h.fireListenerError(firestoreError('permission-denied')) + const second = lastReported()?.message + + expect(first).not.toBe(second) + a.stop() + b.stop() + }) + }) + + describe('permission-denied is terminal', () => { + it('reports and stops loading on a document even with retryOnError', () => { + const def = defineDocument({ + collection: 'docs', + id: 'd1', + retryOnError: true, + }) + const sub = createDocumentSubscription({ + store, + definition: def, + docId: 'd1', + collectionPath: 'docs', + }) + sub.load() + expect(sub.getState().isLoading).toBe(true) + + h.fireListenerError(firestoreError('permission-denied')) + + expect(onError).toHaveBeenCalledTimes(1) + expect(sub.getState().error).toBeInstanceOf(Error) + expect(sub.getState().isLoading).toBe(false) + + // No retry was scheduled: advancing past the interval attaches no + // new listener and reports nothing more. + const before = h.listeners().length + vi.advanceTimersByTime(10000) + expect(h.listeners().length).toBe(before) + expect(onError).toHaveBeenCalledTimes(1) + sub.stop() + }) + + it('reports and stops loading on a collection even with retryOnError', () => { + const def = defineCollection({ + path: 'items', + retryOnError: true, + }) + const sub = createCollectionSubscription({ + store, + definition: def, + collectionPath: 'items', + }) + sub.load() + expect(sub.getState().isLoading).toBe(true) + + h.fireListenerError(firestoreError('permission-denied')) + + expect(onError).toHaveBeenCalledTimes(1) + expect(lastReported()?.path).toBe('items') + expect(sub.getState().error).toBeInstanceOf(Error) + expect(sub.getState().isLoading).toBe(false) + + const before = h.listeners().length + vi.advanceTimersByTime(10000) + expect(h.listeners().length).toBe(before) + sub.stop() + }) + }) + + describe('transient error still retries', () => { + it('re-attaches the listener and does not report on a document', () => { + const def = defineDocument({ + collection: 'docs', + id: 'd1', + retryOnError: true, + retryInterval: 5000, + }) + const sub = createDocumentSubscription({ + store, + definition: def, + docId: 'd1', + collectionPath: 'docs', + }) + sub.load() + const before = h.listeners().length + + h.fireListenerError(firestoreError('unavailable')) + + // Transient: no report, no terminal state, retry scheduled. + expect(onError).not.toHaveBeenCalled() + expect(sub.getState().error).toBeUndefined() + expect(sub.getState().isLoading).toBe(true) + + vi.advanceTimersByTime(5000) + // A fresh listener was attached by the retry. + expect(h.listeners().length).toBe(before + 1) + sub.stop() + }) + }) +}) diff --git a/src/core/collection.ts b/src/core/collection.ts index 828d5ab..b44d9e1 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 { isTerminalListenerError } from './errors' import { applyDiff, applyDiffMutable, @@ -633,25 +634,30 @@ export const createCollectionSubscription = ( } const handleError = (error: Error) => { - if (retryOnError) { + // A terminal code (permission-denied, unauthenticated) never clears on + // retry. Re-attaching the listener every retryInterval would spin + // forever behind a loading spinner and never report, so treat it as + // terminal even when retryOnError is set and fall through to reporting. + if (retryOnError && !isTerminalListenerError(error)) { console.warn('Collection listener error, retrying:', error) retryTimeout = setTimeout(() => { stop() startListener() }, retryInterval) - } else { - state.error = error - // Don't leave consumers stuck on a loading spinner — the listener - // has reported a terminal error, so loading is done. - state.isLoading = false - loaded = true - store.reportError(error, { - type: 'collection', - path: collectionPath, - operation: 'read', - }) - notify() + return } + + state.error = error + // Don't leave consumers stuck on a loading spinner — the listener + // has reported a terminal error, so loading is done. + state.isLoading = false + loaded = true + store.reportError(error, { + type: 'collection', + path: collectionPath, + operation: 'read', + }) + notify() } const startListener = () => { diff --git a/src/core/document.ts b/src/core/document.ts index a96a723..a0a816e 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 { isTerminalListenerError } from './errors' import { applyDiff, applyDiffMutable, @@ -648,25 +649,30 @@ export const createDocumentSubscription = ( } const handleError = (error: Error) => { - if (retryOnError) { + // A terminal code (permission-denied, unauthenticated) never clears on + // retry. Re-attaching the listener every retryInterval would spin + // forever behind a loading spinner and never report, so treat it as + // terminal even when retryOnError is set and fall through to reporting. + if (retryOnError && !isTerminalListenerError(error)) { console.warn('Document listener error, retrying:', error) retryTimeout = setTimeout(() => { stop() load() }, retryInterval) - } else { - state.error = error - // Don't leave consumers stuck on a loading spinner — the listener - // has reported a terminal error, so loading is done. - state.isLoading = false - loaded = true - store.reportError(error, { - type: 'document', - path: `${collectionPath}/${documentId}`, - operation: 'read', - }) - notify() + return } + + state.error = error + // Don't leave consumers stuck on a loading spinner — the listener + // has reported a terminal error, so loading is done. + state.isLoading = false + loaded = true + store.reportError(error, { + type: 'document', + path: `${collectionPath}/${documentId}`, + operation: 'read', + }) + notify() } const load = () => { diff --git a/src/core/errors.ts b/src/core/errors.ts new file mode 100644 index 0000000..207640a --- /dev/null +++ b/src/core/errors.ts @@ -0,0 +1,63 @@ +import type { ErrorContext } from '../types' + +/** + * Error wrapper that carries its own {@link ErrorContext}. + * + * `store.reportError` builds one of these before it calls the consumer's + * `onError`. The context (resource `type`, `path`, and `operation`) used to + * travel only as a second argument, so a consumer that forwarded just the first + * argument to an error tracker dropped every useful field. Then every rules + * denial from every resource shared one fingerprint whose stack lived inside + * the minified `@firebase/firestore` bundle. + * + * A FirestateError puts the path into the message and onto own fields, so a + * consumer that forwards the error alone still gets a usable path and a + * distinct fingerprint per resource. The original error stays reachable through + * `cause`, and a Firestore error `code` (e.g. `permission-denied`) is copied + * onto `code` for consumers that branch on it. + */ +export class FirestateError extends Error { + /** Resource kind the error came from. */ + readonly type: ErrorContext['type'] + /** Firestore path of the document or collection. */ + readonly path: string + /** Operation that failed. */ + readonly operation: ErrorContext['operation'] + /** Firestore error code copied from the cause, when present. */ + readonly code?: string + + constructor(cause: Error, context: ErrorContext) { + super( + `Firestate ${context.type} ${context.operation} failed at ${context.path}: ${cause.message}`, + { cause } + ) + this.name = 'FirestateError' + this.type = context.type + this.path = context.path + this.operation = context.operation + const code = (cause as { code?: unknown }).code + if (typeof code === 'string') { + this.code = code + } + } +} + +/** + * Firestore error codes that a listener retry can never clear. Re-attaching the + * listener for one of these spins forever behind a loading spinner, so the + * subscription treats them as terminal even when `retryOnError` is set. + * + * `permission-denied` is a rules denial; `unauthenticated` means the request + * carried no valid credential. Neither becomes valid by waiting. + */ +const TERMINAL_FIRESTORE_CODES = new Set(['permission-denied', 'unauthenticated']) + +/** + * Report whether a listener error is terminal — a code that a retry will never + * clear. Unknown or transient codes (e.g. `unavailable`) return `false` so the + * retry path stays free to re-attach. + */ +export const isTerminalListenerError = (error: unknown): boolean => { + const code = (error as { code?: unknown } | null)?.code + return typeof code === 'string' && TERMINAL_FIRESTORE_CODES.has(code) +} diff --git a/src/core/store.test.ts b/src/core/store.test.ts index 1f19a83..47f6e7c 100644 --- a/src/core/store.test.ts +++ b/src/core/store.test.ts @@ -1,5 +1,6 @@ import { describe, it, expect, vi } from 'vitest' import { createStore } from './store' +import { FirestateError } from './errors' // Mock Firestore instance const mockFirestore = {} as any @@ -62,7 +63,59 @@ describe('createStore', () => { store.reportError(error, context) - expect(onError).toHaveBeenCalledWith(error, context) + // The reported error is wrapped so a consumer that forwards only + // the first argument still keeps the context. The original error + // stays reachable through `cause`, and context still travels as the + // second argument. + expect(onError).toHaveBeenCalledTimes(1) + const [reported, reportedContext] = onError.mock.calls[0]! + expect(reported).toBeInstanceOf(FirestateError) + expect(reported.type).toBe('document') + expect(reported.path).toBe('projects/123') + expect(reported.operation).toBe('read') + expect(reported.message).toContain('projects/123') + expect(reported.cause).toBe(error) + expect(reportedContext).toEqual(context) + }) + + it('copies a Firestore error code onto the wrapped error', () => { + const onError = vi.fn() + const store = createStore({ firestore: mockFirestore, onError }) + + const error = Object.assign(new Error('Missing permissions'), { + code: 'permission-denied', + }) + store.reportError(error, { + type: 'collection', + path: 'projects/123/tasks', + operation: 'read', + }) + + const [reported] = onError.mock.calls[0]! + expect(reported).toBeInstanceOf(FirestateError) + expect(reported.code).toBe('permission-denied') + expect(reported.path).toBe('projects/123/tasks') + }) + + it('does not double-wrap an already-wrapped error', () => { + const onError = vi.fn() + const store = createStore({ firestore: mockFirestore, onError }) + + const original = new Error('Test error') + const wrapped = new FirestateError(original, { + type: 'document', + path: 'projects/123', + operation: 'read', + }) + store.reportError(wrapped, { + type: 'document', + path: 'projects/123', + operation: 'read', + }) + + const [reported] = onError.mock.calls[0]! + expect(reported).toBe(wrapped) + expect(reported.cause).toBe(original) }) it('logs to console if no onError handler', () => { @@ -96,7 +149,10 @@ describe('createStore', () => { await expect(store.undoManager.undo()).rejects.toThrow( 'Undo failed' ) - expect(onError).toHaveBeenCalledWith(error, { + const [reported, reportedContext] = onError.mock.calls[0]! + expect(reported).toBeInstanceOf(FirestateError) + expect(reported.cause).toBe(error) + expect(reportedContext).toEqual({ type: 'undo', path: '/projects/123', operation: 'undo', diff --git a/src/core/store.ts b/src/core/store.ts index cda2dc1..1c9c5fe 100644 --- a/src/core/store.ts +++ b/src/core/store.ts @@ -7,6 +7,7 @@ import type { Unsubscribe, } from '../types' import { createUndoManager, type UndoManagerWithSubscribe } from '../utils/undo' +import { FirestateError } from './errors' /** * Firestate store that holds configuration and shared state @@ -99,6 +100,25 @@ export const createStore = (config: FirestateConfig): FirestateStore => { let onUndo = config.onUndo let onRedo = config.onRedo + // Wrap every reported error in a FirestateError so a consumer that forwards + // only the first argument to an error tracker keeps the path, type, and + // operation — and gets a distinct fingerprint per resource. The context + // still travels as the second argument for consumers that read it. + const dispatchError = (error: Error, context: ErrorContext) => { + const enriched = + error instanceof FirestateError + ? error + : new FirestateError(error, context) + if (onError) { + onError(enriched, context) + } else { + console.error( + `Firestate error in ${context.type} ${context.path} during ${context.operation}:`, + enriched + ) + } + } + const undoManager = createUndoManager({ maxLength: maxUndoLength, // Stable wrapper — delegates to the mutable onNavigate ref so the @@ -112,19 +132,11 @@ export const createStore = (config: FirestateConfig): FirestateStore => { // store's established onError channel rather than adding undo-specific // error callbacks to FirestateConfig. onError: (error, action, operation) => { - const context: ErrorContext = { + dispatchError(error, { type: 'undo', path: action.path ?? 'undo', operation, - } - if (onError) { - onError(error, context) - } else { - console.error( - `Firestate error in ${context.type} ${context.path} during ${context.operation}:`, - error - ) - } + }) }, }) @@ -150,16 +162,7 @@ export const createStore = (config: FirestateConfig): FirestateStore => { autosave, minLoadTime, - reportError: (error, context) => { - if (onError) { - onError(error, context) - } else { - console.error( - `Firestate error in ${context.type} ${context.path} during ${context.operation}:`, - error - ) - } - }, + reportError: dispatchError, setOnError: (handler) => { onError = handler diff --git a/src/index.ts b/src/index.ts index ffb5f1f..de39b43 100644 --- a/src/index.ts +++ b/src/index.ts @@ -89,6 +89,9 @@ export { shallow } from './utils/shallow' export { createStore } from './core/store' export type { FirestateStore, Store } from './core/store' +// Error reporting +export { FirestateError } from './core/errors' + // Undo manager export { createUndoManager } from './utils/undo' export type { UndoManagerConfig, UndoManagerWithSubscribe } from './utils/undo'