diff --git a/AGENTS.md b/AGENTS.md index f4bc5d5..2789427 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -121,10 +121,17 @@ Preserve these unless the task explicitly changes them. (`update`/`set`/`delete`/`add`/`remove`/`load`/`sync`) and `ref` — status fields are absent unless the selector folds them in, so a status flip the selector ignores (e.g. `isSynced` churning on a save) cannot re-render it. - Writers/`ref` are read live from the subscription (the snapshot is the state; - the handle is read separately), not from the memoized selection, so a rebuilt - subscription always surfaces its own methods even when the selected slice is - value-equal. + Writers/`ref` are read from the subscription's handle (the snapshot is the + state; the handle is read separately), not from the memoized selection. +- Every method on a hook's handle (`update`/`set`/`delete`/`add`/`remove`/ + `load`/`sync`) looks up the facade's *current* subscription when it is + called, not when the hook renders, and keeps one identity per resolved + `shared` facade (safe in `useCallback`/`useEffect` deps). The facade can switch + subscriptions after render — `acquire()` adopts a sibling's committed entry, + or rebuilds a stopped one after refCount hit 0 — and a value-equal slice never + re-runs the handle memo. Methods copied at render time therefore wrote into an + abandoned subscription, which dropped the write for want of a snapshot + (HSF-1642). Never copy methods off `getHandle()` into the returned handle. - A hook called WITHOUT a selector returns the **sync-agnostic default handle**: `{ data, isLoaded, error, ...writers, ref }` for a document, plus `isActive` for a collection. It deliberately drops `isSynced` (and `isLoading`, folded diff --git a/README.md b/README.md index 460ffb4..f123297 100644 --- a/README.md +++ b/README.md @@ -756,6 +756,12 @@ const id = add({ name: 'New Space', area: 500, floor: 1 }) remove('oldSpaceId') ``` +Handle methods (`update`, `set`, `delete`, `add`, `remove`, `load`, `sync`) keep +their identity while the hook's resource (definition, path, and query) stays the +same, so they are safe in `useCallback` and `useEffect` dependency lists. Every +call reaches the resource's live subscription, whatever order the hooks on that +resource mounted in. + #### Selecting a slice (`selector` + `isEqual`) By default a component re-renders when the data, the load state (`isLoaded`), or diff --git a/docs/architecture.md b/docs/architecture.md index d559fa0..6f18662 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -240,12 +240,19 @@ Important details: or the standalone `useDocumentSyncStatus`/… helpers) are thin readers over `useDocument`/`useCollection` with a fixed read-only selector, so they resolve the same shared entry — one listener — and read the same optimistic state. -- Either way the projection deliberately excludes methods and `ref`; those are - read *live* from the current subscription's `getHandle()` at render time (a - separate read from the state snapshot). Otherwise a subscription rebuild whose - projection happened to be value-equal would be collapsed by `isEqual`, and the - hook would keep handing back the previous subscription's methods (e.g. `load()` - against torn-down constraints). +- Either way the projection deliberately excludes methods and `ref`. Otherwise a + subscription rebuild whose projection happened to be value-equal would be + collapsed by `isEqual`, and the hook would keep handing back the previous + subscription's methods (e.g. `load()` against torn-down constraints). `ref` is + read from `getHandle()` when the handle is built. Methods are wrappers built + once per `shared` facade that call `getHandle()[method]` when invoked, so they + always reach the facade's *current* subscription. This matters because the + facade can switch subscriptions after render without the projection changing: + hooks that render together each build a provisional entry, and `acquire()` + makes every hook after the first adopt the first one's entry. Likewise, a + re-acquire after refCount hit 0 rebuilds the entry's stopped subscription. + Methods copied at render time would keep calling the abandoned subscription, + which has no snapshot and drops the write. - Disabled hooks do not resolve params or create subscriptions. - Toggling `undoable` should not rebuild Firestore listeners. - `queryConstraints` are keyed by semantic query identity, not array diff --git a/package.json b/package.json index 32db265..d40a1d7 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@hvakr/firestate", - "version": "0.2.1", + "version": "0.2.2", "description": "Firestore state management for React with real-time sync, undo/redo, optimistic updates, and Zod schema validation", "author": "HVAKR", "license": "MIT", diff --git a/src/react/hooks.test.ts b/src/react/hooks.test.ts index fc2c496..f066d84 100644 --- a/src/react/hooks.test.ts +++ b/src/react/hooks.test.ts @@ -43,12 +43,27 @@ import { type Firestore, type QueryConstraint, } from 'firebase/firestore' -import { useCollection, FirestateContext } from './hooks' -import { defineCollection } from '../registry/schema' +import { + useCollection, + useCollectionLoadingStatus, + useDocument, + useDocumentLoadingStatus, + FirestateContext, +} from './hooks' +import { defineCollection, defineDocument } from '../registry/schema' import { createFirestate, col } from '../registry/firestate' import { z } from 'zod' import { createStore, type FirestateStore } from '../core/store' -import type { CollectionHandle, FirestoreObject, LoadingStatus } from '../types' +import type { + CollectionHandle, + CollectionState, + DocumentHandle, + DocumentState, + FirestoreObject, + LoadingStatus, + SelectedCollectionHandle, + SelectedDocumentHandle, +} from '../types' interface Station extends FirestoreObject { name: string @@ -681,3 +696,372 @@ describe('createFirestate .select shares one collection listener (real queries)' expect(loading).toEqual({ isLoading: true, isLoaded: false }) }) }) + +describe('hook methods resolve the live subscription at call time', () => { + // Regression for HSF-1642 (flowcircuits/hvakr#8248). Hooks on one resource + // that render together each build a provisional subscription; on commit, + // every hook after the first adopts the first one's entry. The returned + // handle used to copy its methods off the provisional subscription at render + // time and re-copy them only when its selected slice changed. A hook whose + // slice is value-equal across the first snapshot (an empty collection's + // `{}`, a missing document's `undefined`, a writer-only selector) therefore + // kept calling the abandoned subscription, which has no snapshot and drops + // every write. Only onSnapshot is mocked; queries and refs are real. + interface Zone extends FirestoreObject { + name: string + } + const zones = defineCollection({ path: 'zones', lazy: true }) + const eagerZones = defineCollection({ path: 'eagerZones' }) + const zone = defineDocument({ collection: 'zones', id: 'z1' }) + + type ColSelector = (s: CollectionState) => unknown + type DocSelector = (s: DocumentState) => unknown + const allData: ColSelector = (s) => s.data + const allDocData: DocSelector = (s) => s.data + // A hook that only writes (a "Delete" button): its slice never changes. + const writerOnly = (): null => null + + let store: FirestateStore + const renderers: ReactTestRenderer[] = [] + let listeners: Array<{ + deliver: (snapshot: unknown) => void + unsubscribe: ReturnType + }> + // The latest value each probe rendered. + let status: LoadingStatus + let writer: SelectedCollectionHandle + let reader: CollectionHandle + let docWriter: SelectedDocumentHandle + let docReader: DocumentHandle + + const onSnapshotMock = onSnapshot as unknown as ReturnType + + beforeEach(() => { + vi.clearAllMocks() + vi.useFakeTimers() + store = createStore({ firestore, autosave: 0 }) + listeners = [] + onSnapshotMock.mockImplementation( + (_ref: unknown, onNext: (snapshot: unknown) => void) => { + const unsubscribe = vi.fn() + listeners.push({ deliver: onNext, unsubscribe }) + return unsubscribe + } + ) + }) + + afterEach(() => { + act(() => { + renderers.splice(0).forEach((r) => r.unmount()) + }) + vi.useRealTimers() + }) + + // Status hook FIRST, writer second: the order that broke Create Zone. + const StatusThenWriter = (p: { + definition: typeof zones + selector: ColSelector + readOnly?: boolean + }) => { + status = useCollectionLoadingStatus({ definition: p.definition }) + writer = useCollection({ + definition: p.definition, + selector: p.selector, + readOnly: p.readOnly, + }) + return null + } + + const WriterThenStatus = (p: { + definition: typeof zones + selector: ColSelector + }) => { + writer = useCollection({ + definition: p.definition, + selector: p.selector, + }) + status = useCollectionLoadingStatus({ definition: p.definition }) + return null + } + + const Reader = (p: { definition: typeof zones }) => { + reader = useCollection({ definition: p.definition }) + return null + } + + const DocStatusThenWriter = (p: { selector: DocSelector }) => { + status = useDocumentLoadingStatus({ definition: zone }) + docWriter = useDocument({ definition: zone, selector: p.selector }) + return null + } + + const DocReader = () => { + docReader = useDocument({ definition: zone }) + return null + } + + const wrap = (node: ReactElement): ReactElement => + createElement(FirestateContext.Provider, { value: store }, node) + + const mount = (node: ReactElement): ReactTestRenderer => { + let renderer!: ReactTestRenderer + act(() => { + renderer = create(wrap(node)) + }) + renderers.push(renderer) + return renderer + } + + /** Deliver a snapshot on the newest listener and settle minLoadTime. */ + const deliver = (snapshot: unknown): void => { + act(() => { + listeners[listeners.length - 1]!.deliver(snapshot) + vi.runAllTimers() + }) + } + + const colSnap = (docs: Record) => ({ + docs: Object.entries(docs).map(([id, data]) => ({ + id, + data: () => data, + })), + }) + + const docSnap = (data: Zone | null) => ({ + exists: () => data !== null, + data: () => data ?? undefined, + metadata: { fromCache: false, hasPendingWrites: false }, + }) + + /** + * Mount the writer tree, then a reader in its own renderer. The reader + * renders after the writer tree commits, so it resolves the committed entry + * directly; its `load()` is never stale, which pins any failure on the + * writer. + */ + const loadCollection = ( + writerTree: ReactElement, + docs: Record + ): void => { + mount(writerTree) + mount(createElement(Reader, { definition: zones })) + act(() => reader.load()) + expect(listeners).toHaveLength(1) + deliver(colSnap(docs)) + expect(reader.isLoaded).toBe(true) + expect(status.isLoaded).toBe(true) + } + + const loadDocument = (selector: DocSelector, data: Zone | null): void => { + mount(createElement(DocStatusThenWriter, { selector })) + mount(createElement(DocReader)) + expect(listeners).toHaveLength(1) + deliver(docSnap(data)) + expect(docReader.isLoaded).toBe(true) + expect(status.isLoaded).toBe(true) + } + + it('collection, empty: update() from a writer mounted after a status hook reaches the live subscription', () => { + loadCollection( + createElement(StatusThenWriter, { + definition: zones, + selector: allData, + }), + {} + ) + + act(() => writer.update({ first: { name: 'x' } })) + expect(reader.data.first?.name).toBe('x') + }) + + it('collection, empty: add() from a writer mounted after a status hook reaches the live subscription', () => { + loadCollection( + createElement(StatusThenWriter, { + definition: zones, + selector: allData, + }), + {} + ) + + act(() => { + writer.add('first', { name: 'x' }) + }) + expect(reader.data.first?.name).toBe('x') + }) + + it('collection: remove() from a writer-only hook mounted after a status hook reaches the live subscription', () => { + // remove() needs an existing doc, and a slice that changes on the first + // snapshot re-copied the methods. A writer-only slice never changes. + loadCollection( + createElement(StatusThenWriter, { + definition: zones, + selector: writerOnly, + }), + { first: { name: 'x' } } + ) + + act(() => writer.remove('first')) + expect(reader.data.first).toBeUndefined() + }) + + it('document, missing: set() from a writer mounted after a status hook reaches the live subscription', () => { + // A missing doc's data is `undefined` before and after the snapshot. + loadDocument(allDocData, null) + + act(() => docWriter.set({ name: 'x' })) + expect(docReader.data).toEqual({ name: 'x' }) + }) + + it('document: update() from a writer-only hook mounted after a status hook reaches the live subscription', () => { + loadDocument(writerOnly, { name: 'a' }) + + act(() => docWriter.update({ name: 'b' })) + expect(docReader.data).toEqual({ name: 'b' }) + }) + + it('read-only: load() reaches the live subscription and writers stay blocked', () => { + mount( + createElement(StatusThenWriter, { + definition: zones, + selector: writerOnly, + readOnly: true, + }) + ) + expect(listeners).toHaveLength(0) + + // load() passes through the read-only facade to the ONE shared + // subscription, the same one the status hook reads. + act(() => writer.load()) + expect(listeners).toHaveLength(1) + expect(status).toEqual({ isLoading: true, isLoaded: false }) + deliver(colSnap({ first: { name: 'x' } })) + expect(status).toEqual({ isLoading: false, isLoaded: true }) + + mount(createElement(Reader, { definition: zones })) + act(() => { + writer.update({ first: { name: 'nope' } }) + writer.add('second', { name: 'nope' }) + writer.remove('first') + }) + expect(Object.keys(reader.data)).toEqual(['first']) + expect(reader.data.first?.name).toBe('x') + expect(store.isSynced).toBe(true) + }) + + it('replacement: a method held across refCount 0 and re-acquire calls the rebuilt subscription', () => { + // Re-keying the screen unmounts every old hook and mounts new ones in + // ONE commit. The new hooks render first and resolve the still-live + // entry; then the old hooks release (refCount 0 stops the entry and + // evicts it) and the new ones re-acquire it, which rebuilds `ent.sub`. + // The new handles were built at render time, from the stopped sub. + // (StrictMode's effect double-run and offscreen take the same path.) + // Behavior chosen: the held method keeps its identity AND reaches the + // rebuilt subscription, because identity is tied to the facade + // (`shared`), which survives the rebuild. + const Screen = () => { + writer = useCollection({ + definition: eagerZones, + selector: allData, + }) + reader = useCollection({ definition: eagerZones }) + return null + } + const r = mount(createElement(Screen, { key: 'a' })) + deliver(colSnap({})) + expect(reader.isLoaded).toBe(true) + + act(() => r.update(wrap(createElement(Screen, { key: 'b' })))) + // The first subscription stopped at refCount 0; the second is the one + // rebuilt on re-acquire. + expect(listeners).toHaveLength(2) + expect(listeners[0]!.unsubscribe).toHaveBeenCalledTimes(1) + const held = writer.update + + deliver(colSnap({})) + expect(reader.isLoaded).toBe(true) + + act(() => held({ first: { name: 'x' } })) + expect(reader.data.first?.name).toBe('x') + expect(writer.update).toBe(held) + }) + + it('keeps method identity across rerenders while the resource is unchanged', () => { + // Before the fix, the first snapshot re-ran the merge memo and swapped + // every method from the provisional to the adopted subscription: a new + // identity mid-life that re-fired effects depending on it. + let full!: CollectionHandle + const StatusThenDefaultWriter = (p: { definition: typeof zones }) => { + status = useCollectionLoadingStatus({ definition: p.definition }) + full = useCollection({ definition: p.definition }) + return null + } + const r = mount( + createElement(StatusThenDefaultWriter, { definition: eagerZones }) + ) + const held = { + update: full.update, + add: full.add, + remove: full.remove, + load: full.load, + sync: full.sync, + } + + // Rerenders: a snapshot, a local edit, and a parent rerender. + deliver(colSnap({ first: { name: 'x' } })) + act(() => full.update({ first: { name: 'y' } })) + act(() => + r.update( + wrap( + createElement(StatusThenDefaultWriter, { + definition: eagerZones, + }) + ) + ) + ) + + expect(full.update).toBe(held.update) + expect(full.add).toBe(held.add) + expect(full.remove).toBe(held.remove) + expect(full.load).toBe(held.load) + expect(full.sync).toBe(held.sync) + expect(full.data.first?.name).toBe('y') + + // A different resource resolves a different `shared`: new methods. + act(() => + r.update( + wrap( + createElement(StatusThenDefaultWriter, { + definition: zones, + }) + ) + ) + ) + expect(full.update).not.toBe(held.update) + }) + + it('control: writer mounted before the status hook', () => { + loadCollection( + createElement(WriterThenStatus, { + definition: zones, + selector: allData, + }), + {} + ) + + act(() => writer.update({ first: { name: 'x' } })) + expect(reader.data.first?.name).toBe('x') + }) + + it('control: collection with data in its first snapshot', () => { + loadCollection( + createElement(StatusThenWriter, { + definition: zones, + selector: allData, + }), + { first: { name: 'a' } } + ) + + act(() => writer.update({ first: { name: 'b' } })) + expect(reader.data.first?.name).toBe('b') + }) +}) diff --git a/src/react/hooks.ts b/src/react/hooks.ts index b48e3c2..2ac9a3e 100644 --- a/src/react/hooks.ts +++ b/src/react/hooks.ts @@ -256,6 +256,19 @@ const defaultDataEqual = valuesEqualForNoOp as ( b: TSelected ) => boolean +/** + * A stand-in for `getHandle()[key]` that looks the method up when it is + * CALLED, not when the hook renders. The facade behind `getHandle` can switch + * subscriptions after render: `acquire()` adopts a sibling's committed entry, or + * rebuilds the entry's stopped subscription. A copied method would still point + * at the abandoned subscription, which has no snapshot and drops every write. + * Built once per `getHandle` (that is, per `shared`), so identity is stable for + * `useCallback`/`useEffect` deps. + */ +const callLive = (getHandle: () => H, key: K): H[K] => + ((...args: unknown[]) => + (getHandle()[key] as (...a: unknown[]) => unknown)(...args)) as H[K] + /** * Context for providing the Firestate store */ @@ -574,9 +587,22 @@ export function useDocument( equal ) - // Re-wrap into a handle. Methods and `ref` are read *live* via getHandle() - // (keyed on `shared`) so a rebuild always hands back the new subscription's - // methods even when the projection was value-equal (see DefaultSelection). + // Methods resolve the facade's CURRENT subscription on each call (see + // callLive), not the one it had at render time. One set per `shared`, so + // identity holds across rerenders. + const methods = useMemo( + () => ({ + update: callLive(getHandle, 'update'), + set: callLive(getHandle, 'set'), + delete: callLive(getHandle, 'delete'), + sync: callLive(getHandle, 'sync'), + }), + [getHandle] + ) + + // Re-wrap into a handle. `ref` is read via getHandle() (keyed on `shared`) + // so a rebuild hands back the new subscription's ref even when the + // projection was value-equal (see DefaultSelection). // // Keyed on selector *presence*, never its identity: an inline selector is a // fresh function each render yet yields a value-equal `selection`, so the @@ -584,32 +610,32 @@ export function useDocument( // the handle's shape — selected vs full — depends on the selector. const hasSelector = selector != null return useMemo(() => { - const handle = getHandle() + const { ref } = getHandle() if (hasSelector) { // Pure selected handle: `data` is the slice; status is intentionally // absent (select it to react to it). Writers act on the full doc. return { data: selection as TSelected, - update: handle.update, - set: handle.set, - delete: handle.delete, - sync: handle.sync, - ref: handle.ref, + update: methods.update, + set: methods.set, + delete: methods.delete, + sync: methods.sync, + ref, } } // Default handle: sync-agnostic — data + isLoaded + error (no isSynced). const s = selection as DefaultSelection return { data: s.data, - update: handle.update, - set: handle.set, - delete: handle.delete, + update: methods.update, + set: methods.set, + delete: methods.delete, isLoaded: s.isLoaded, - sync: handle.sync, + sync: methods.sync, error: s.error, - ref: handle.ref, + ref, } - }, [selection, getHandle, hasSelector]) + }, [selection, getHandle, hasSelector, methods]) } /** @@ -928,40 +954,52 @@ export function useCollection( equal ) + // See useDocument: methods resolve the current subscription per call. + const methods = useMemo( + () => ({ + update: callLive(getHandle, 'update'), + add: callLive(getHandle, 'add'), + remove: callLive(getHandle, 'remove'), + load: callLive(getHandle, 'load'), + sync: callLive(getHandle, 'sync'), + }), + [getHandle] + ) + // See useDocument: keyed on selector *presence*, not identity, so an inline - // selector still yields a stable handle. Methods/`ref` read live from - // getHandle; only the handle's shape depends on the selector. + // selector still yields a stable handle. Only the handle's shape depends on + // the selector. const hasSelector = selector != null return useMemo(() => { - const handle = getHandle() + const { ref } = getHandle() if (hasSelector) { // Pure selected handle: status is absent (select it to react to it); // writers and `load` act on the full collection. return { data: selection as TSelected, - update: handle.update, - add: handle.add, - remove: handle.remove, - load: handle.load, - sync: handle.sync, - ref: handle.ref, + update: methods.update, + add: methods.add, + remove: methods.remove, + load: methods.load, + sync: methods.sync, + ref, } } // Default handle: sync-agnostic — data + isLoaded + isActive + error. const s = selection as DefaultSelection> return { data: s.data, - update: handle.update, - add: handle.add, - remove: handle.remove, + update: methods.update, + add: methods.add, + remove: methods.remove, isLoaded: s.isLoaded, isActive: s.isActive ?? false, - load: handle.load, - sync: handle.sync, + load: methods.load, + sync: methods.sync, error: s.error, - ref: handle.ref, + ref, } - }, [selection, getHandle, hasSelector]) + }, [selection, getHandle, hasSelector, methods]) } // ---------------------------------------------------------------------------