diff --git a/src/collection.ts b/src/collection.ts index 9d8cacb..5582d3d 100644 --- a/src/collection.ts +++ b/src/collection.ts @@ -20,7 +20,15 @@ import type { UpdateOptions, } from './types' import type { FirestateStore } from './store' -import { applyDiffMutable, computeDiff, deepClone, flattenDiff, isDeepEqual } from './diff' +import { + applyDiffMutable, + applyOverridesAtPaths, + computeDiff, + deepClone, + flattenDiff, + isDeepEqual, + reconcileDisplayOverrides, +} from './diff' // Module-level counter so each subscription instance gets a unique sync key, // even when multiple instances target the same collection path. @@ -63,6 +71,12 @@ interface CollectionInternalState { error: Error | undefined waitingForUpdate: boolean inflightLocalState: Record | undefined + /** + * Frozen display values for `serverTimestamp()` sentinels currently + * sitting in `localState`. Keyed by dotted path (e.g. + * `".updatedAt"`). See document.ts for the full contract. + */ + displayOverrides: Map } /** @@ -139,6 +153,7 @@ export const createCollectionSubscription = ( error: undefined, waitingForUpdate: false, inflightLocalState: undefined, + displayOverrides: new Map(), } const subscribers = new Set>>() @@ -156,8 +171,10 @@ export const createCollectionSubscription = ( // subscriptions to the same path don't share (or clobber) one entry. const syncKey = `col:${collectionPath}#${++syncKeyCounter}` - const getMergedData = (): Record => - state.localState ?? state.syncState ?? {} + const getMergedData = (): Record => { + const base = state.localState ?? state.syncState ?? {} + return applyOverridesAtPaths(base, state.displayOverrides) + } const getPublicState = (): CollectionState => ({ data: getMergedData(), @@ -168,6 +185,12 @@ export const createCollectionSubscription = ( }) const notify = () => { + // Reconcile display overrides against the current localState + // before publishing — see document.ts for the full contract. + reconcileDisplayOverrides( + state.localState as Record | undefined, + state.displayOverrides + ) cachedHandle = null const publicState = getPublicState() subscribers.forEach((fn) => fn(publicState)) diff --git a/src/diff.test.ts b/src/diff.test.ts index c88d34f..49d0ac3 100644 --- a/src/diff.test.ts +++ b/src/diff.test.ts @@ -1,13 +1,17 @@ import { describe, it, expect } from 'vitest' +import { GeoPoint, serverTimestamp, Timestamp } from 'firebase/firestore' import { computeDiff, applyDiff, applyDiffMutable, + applyOverridesAtPaths, + collectServerTimestampPaths, isDeepEqual, deepClone, isDiffEmpty, mergeDiffs, flattenDiff, + reconcileDisplayOverrides, unflattenDiff, diffContainsPath, extractDiffValue, @@ -226,4 +230,316 @@ describe('diff utilities', () => { expect(restored).toEqual(startState) }) }) + + // Pins the C1 bug: serverTimestamp() must survive the local pipeline + // unchanged so Firestore can expand it on the server. Currently the + // sentinel is replaced with the client clock (or destroyed entirely + // by deepClone), so the server never gets to stamp its own time. + describe('serverTimestamp() preservation', () => { + it('applyDiffMutable preserves the sentinel instead of substituting Timestamp.now()', () => { + // updateState() funnels every user diff through applyDiffMutable. + // If the sentinel is replaced here, the rest of the pipeline only + // ever sees a client Timestamp — Firestore never sees serverTimestamp(). + const target: Record = {} + const sentinel = serverTimestamp() + + applyDiffMutable(target, { updatedAt: sentinel }) + + expect(target.updatedAt).toBe(sentinel) + }) + + it('deepClone preserves the sentinel', () => { + // setData() calls deepClone on the user's payload. deepClone + // currently iterates the sentinel's own keys and produces a + // plain object with no .isEqual method, so downstream sentinel + // detection misses it and setDoc writes garbage at that field. + const sentinel = serverTimestamp() + const cloned = deepClone({ updatedAt: sentinel }) + + expect(cloned.updatedAt).toBe(sentinel) + }) + + it('the diff shipped to updateDoc still contains the sentinel', () => { + // Mirrors what sync() does after `handle.update({ updatedAt: serverTimestamp() })`: + // 1. updateState clones currentData and folds in the user's diff. + // 2. sync computes diff(syncState, localState) and ships it. + // The diff must contain the sentinel — not a client Timestamp. + const syncState = { name: 'doc', updatedAt: Timestamp.fromMillis(1000) } + const localState = deepClone(syncState) + const sentinel = serverTimestamp() + + applyDiffMutable(localState, { updatedAt: sentinel }) + const diff = computeDiff(syncState, localState) as Record + + expect(diff.updatedAt).toBe(sentinel) + }) + + it('flattenDiff carries the sentinel through to a dotted key', () => { + // Final hop before updateDoc — the diff is flattened to dot + // notation. The sentinel must arrive at the right dotted + // path so Firestore can expand it on the server. + const sentinel = serverTimestamp() + const flat = flattenDiff({ meta: { updatedAt: sentinel } }) + + expect(flat['meta.updatedAt']).toBe(sentinel) + }) + }) + + // Pins H3: identical-by-value Timestamps used to fall into the + // primitive `!==` branch of computeDiff because deepClone returned a + // fresh instance, so every unrelated edit re-wrote every Timestamp + // field. Now compared via `.isEqual` at the leaf. + describe('Timestamp diff suppression (H3)', () => { + it('does not produce a diff when both sides hold an equal Timestamp', () => { + const ts = Timestamp.fromMillis(1000) + const from = { name: 'a', createdAt: ts } + const to = { name: 'a', createdAt: Timestamp.fromMillis(1000) } + + expect(computeDiff(from, to)).toEqual({}) + }) + + it('does include changed Timestamps in the diff', () => { + const from = { createdAt: Timestamp.fromMillis(1000) } + const to = { createdAt: Timestamp.fromMillis(2000) } + const diff = computeDiff(from, to) as Record + + expect(diff.createdAt).toBe(to.createdAt) + }) + + it('does not re-write the Timestamp when an unrelated field changes', () => { + // Regression for the specific shape that wasted writes: clone + // the doc, edit one field, send to sync. The Timestamp in the + // clone is a different reference but the same value, and + // shouldn't end up in the diff. + const syncState = { name: 'old', createdAt: Timestamp.fromMillis(1000) } + const localState = deepClone(syncState) + localState.name = 'new' + + const diff = computeDiff(syncState, localState) as Record + + expect(diff).toEqual({ name: 'new' }) + expect('createdAt' in diff).toBe(false) + }) + }) + + // Pins C3: non-Timestamp Firestore value types (DocumentReference, + // GeoPoint, Bytes, VectorValue) were silently corrupted by deepClone + // because it walked their own keys and stripped the prototype. The + // first user edit on any doc holding such a field would either fail + // the write or store nonsense. GeoPoint is used here as the proxy + // for the class because it's the only one constructible without a + // Firestore instance — the fix is generic via isFirestoreOpaque. + describe('Firestore value types (C3)', () => { + it('deepClone returns GeoPoint by reference instead of stripping its prototype', () => { + const pt = new GeoPoint(40.7, -74.0) + const cloned = deepClone({ home: pt }) + + expect(cloned.home).toBe(pt) + expect(cloned.home).toBeInstanceOf(GeoPoint) + }) + + it('does not produce a diff when both sides hold an equal GeoPoint', () => { + const from = { home: new GeoPoint(40.7, -74.0) } + const to = { home: new GeoPoint(40.7, -74.0) } + + expect(computeDiff(from, to)).toEqual({}) + }) + + it('includes a changed GeoPoint in the diff', () => { + const from = { home: new GeoPoint(40.7, -74.0) } + const to = { home: new GeoPoint(48.8, 2.3) } + const diff = computeDiff(from, to) as Record + + expect(diff.home).toBe(to.home) + }) + }) + + // Pins H5: mergeDiffs called deepClone on `first`, which used to + // walk a sentinel's own keys and produce a plain object that no + // longer registered as a sentinel anywhere downstream. Now opaque + // values pass through by reference. + describe('mergeDiffs sentinel preservation (H5)', () => { + it('preserves a serverTimestamp() sentinel from the first diff', () => { + const sentinel = serverTimestamp() + const merged = mergeDiffs( + { updatedAt: sentinel, name: 'a' }, + { name: 'b' } + ) as Record + + expect(merged.updatedAt).toBe(sentinel) + expect(merged.name).toBe('b') + }) + + it('preserves a serverTimestamp() sentinel from the second diff', () => { + const sentinel = serverTimestamp() + const merged = mergeDiffs( + { name: 'a' }, + { updatedAt: sentinel } + ) as Record + + expect(merged.updatedAt).toBe(sentinel) + }) + }) + + // Display overrides power the optimistic-UI half of the C1 fix. + // Sentinels stay in localState (so the write is correct), but the + // merged view substitutes a frozen Timestamp at each sentinel path + // so consumers always see a renderable value during the in-flight + // window. + describe('displayOverrides helpers', () => { + describe('collectServerTimestampPaths', () => { + it('finds sentinels at the top level', () => { + const paths = collectServerTimestampPaths({ + name: 'x', + updatedAt: serverTimestamp(), + }) + expect([...paths]).toEqual(['updatedAt']) + }) + + it('finds sentinels nested in plain objects', () => { + const paths = collectServerTimestampPaths({ + name: 'x', + meta: { updatedAt: serverTimestamp(), revision: 5 }, + }) + expect([...paths]).toEqual(['meta.updatedAt']) + }) + + it('ignores non-serverTimestamp sentinels and value types', () => { + const paths = collectServerTimestampPaths({ + createdAt: Timestamp.fromMillis(1000), + home: new GeoPoint(0, 0), + // deleteField is structural — applyDiffMutable removes the + // key before reconcile runs, so it shouldn't show up here + // in practice. But the collect helper is defensive. + }) + expect(paths.size).toBe(0) + }) + + it('returns empty for null / undefined input', () => { + expect(collectServerTimestampPaths(null).size).toBe(0) + expect(collectServerTimestampPaths(undefined).size).toBe(0) + }) + }) + + describe('reconcileDisplayOverrides', () => { + it('captures a frozen Timestamp on first sighting', () => { + const overrides = new Map() + let tick = 1000 + const now = () => Timestamp.fromMillis(tick++) + + reconcileDisplayOverrides( + { updatedAt: serverTimestamp() }, + overrides, + now + ) + expect(overrides.get('updatedAt')).toEqual(Timestamp.fromMillis(1000)) + }) + + it('does not advance the captured value on subsequent reconciles', () => { + // This is the whole point of the frozen-at-first-sighting + // design: subsequent reads must not see a Timestamp that + // crept forward to the current clock. + const overrides = new Map() + let tick = 1000 + const now = () => Timestamp.fromMillis(tick++) + + const localState = { updatedAt: serverTimestamp() } + reconcileDisplayOverrides(localState, overrides, now) + reconcileDisplayOverrides(localState, overrides, now) + reconcileDisplayOverrides(localState, overrides, now) + + expect(overrides.get('updatedAt')).toEqual(Timestamp.fromMillis(1000)) + }) + + it('drops overrides for paths whose sentinel was overwritten', () => { + const overrides = new Map() + const now = () => Timestamp.fromMillis(1000) + + // First mutation: sentinel arrives. + reconcileDisplayOverrides( + { updatedAt: serverTimestamp() }, + overrides, + now + ) + expect(overrides.has('updatedAt')).toBe(true) + + // User overwrites with an explicit value. + reconcileDisplayOverrides( + { updatedAt: Timestamp.fromMillis(5000) }, + overrides, + now + ) + expect(overrides.has('updatedAt')).toBe(false) + }) + + it('clears every override when localState becomes empty (snapshot ack)', () => { + const overrides = new Map() + reconcileDisplayOverrides( + { + updatedAt: serverTimestamp(), + meta: { lastSeen: serverTimestamp() }, + }, + overrides + ) + expect(overrides.size).toBe(2) + + // localState cleared by snapshot ack. + reconcileDisplayOverrides(undefined, overrides) + expect(overrides.size).toBe(0) + }) + }) + + describe('applyOverridesAtPaths', () => { + it('returns the input unchanged when the override map is empty', () => { + const merged = { name: 'x', updatedAt: Timestamp.fromMillis(1000) } + const result = applyOverridesAtPaths(merged, new Map()) + + expect(result).toBe(merged) + }) + + it('substitutes a top-level override', () => { + const ts = Timestamp.fromMillis(5000) + const overrides = new Map([['updatedAt', ts]]) + const result = applyOverridesAtPaths( + { name: 'x', updatedAt: serverTimestamp() }, + overrides + ) + + expect(result.updatedAt).toBe(ts) + expect(result.name).toBe('x') + }) + + it('substitutes a nested override without clobbering siblings', () => { + const ts = Timestamp.fromMillis(5000) + const overrides = new Map([['meta.updatedAt', ts]]) + const result = applyOverridesAtPaths( + { + name: 'x', + meta: { updatedAt: serverTimestamp(), revision: 5 }, + }, + overrides + ) + + expect(result).toEqual({ + name: 'x', + meta: { updatedAt: ts, revision: 5 }, + }) + }) + + it('does not mutate the input merged object', () => { + const original = { + name: 'x', + updatedAt: serverTimestamp(), + } + const ts = Timestamp.fromMillis(5000) + const overrides = new Map([['updatedAt', ts]]) + + applyOverridesAtPaths(original, overrides) + + // The original still holds the sentinel — overrides are + // a render-time concern only. + expect(original.updatedAt).not.toBe(ts) + }) + }) + }) }) diff --git a/src/diff.ts b/src/diff.ts index 16433ad..2e395c9 100644 --- a/src/diff.ts +++ b/src/diff.ts @@ -16,6 +16,45 @@ const isPlainObject = (value: unknown): value is Record => !(value instanceof Timestamp) && Object.getPrototypeOf(value) === Object.prototype +/** + * Check if a value is a Firestore opaque type: a FieldValue sentinel + * (`serverTimestamp`, `deleteField`, `increment`, `arrayUnion`, + * `arrayRemove`, …) or a value type the SDK ships with its own identity + * semantics (`Timestamp`, `DocumentReference`, `GeoPoint`, `Bytes`, + * `VectorValue`). They all expose `.isEqual()` and have a non-plain + * prototype. + * + * The diff / clone pipeline must treat these as **opaque**: never iterate + * their keys, never substitute their values, never compare them by `===`. + * Doing any of those silently breaks the write path — see the C1 + * regression where `serverTimestamp()` was replaced with `Timestamp.now()` + * before reaching Firestore. + */ +const isFirestoreOpaque = ( + value: unknown +): value is { isEqual: (other: unknown) => boolean } => { + if (value === null || typeof value !== 'object') return false + if (Object.getPrototypeOf(value) === Object.prototype) return false + return ( + 'isEqual' in value && + typeof (value as { isEqual: unknown }).isEqual === 'function' + ) +} + +// Reference sentinels used to identify specific FieldValue kinds. The +// Firebase SDK does not export the sentinel subclasses; the only stable +// way to ask "is this a serverTimestamp / deleteField?" is to construct a +// reference instance once and delegate to its `.isEqual()`. Hoisting them +// to module scope avoids reconstructing on every call. +const SERVER_TIMESTAMP_REF = serverTimestamp() +const DELETE_FIELD_REF = deleteField() + +const isDeleteField = (value: unknown): boolean => + isFirestoreOpaque(value) && value.isEqual(DELETE_FIELD_REF) + +const isServerTimestamp = (value: unknown): boolean => + isFirestoreOpaque(value) && value.isEqual(SERVER_TIMESTAMP_REF) + /** * Check if two values are deeply equal */ @@ -29,7 +68,10 @@ export const isDeepEqual = (a: unknown, b: unknown): boolean => { return a.every((item, i) => isDeepEqual(item, b[i])) } - if (a instanceof Timestamp && b instanceof Timestamp) { + // Opaque Firestore types delegate to their own `.isEqual`. Catches + // Timestamp, DocumentReference, GeoPoint, Bytes, VectorValue and + // every FieldValue sentinel kind. + if (isFirestoreOpaque(a) && isFirestoreOpaque(b)) { return a.isEqual(b) } @@ -88,6 +130,22 @@ export const computeDiff = ( continue } + // Firestore opaque values — sentinels (serverTimestamp, + // arrayUnion, …) and value types (Timestamp, DocumentReference, + // …). Compare via `.isEqual` so identical-by-value Timestamps + // don't show up as spurious diffs every sync; pass the value + // through unchanged so sentinels survive into the write payload + // for the server to expand. + if (isFirestoreOpaque(toValue)) { + if ( + !isFirestoreOpaque(fromValue) || + !toValue.isEqual(fromValue) + ) { + diff[key] = toValue + } + continue + } + // Primitives are compared directly if (toValue !== undefined && fromValue !== toValue) { diff[key] = toValue @@ -121,27 +179,31 @@ export const applyDiffMutable = ( target: FirestoreObject, diff: Record ): void => { - const deleteFieldSentinel = deleteField() - const serverTimestampSentinel = serverTimestamp() - for (const key of Object.keys(diff)) { const value = (diff as Record)[key] - // Handle deleteField sentinel - if ( - value !== null && - typeof value === 'object' && - 'isEqual' in value && - typeof value.isEqual === 'function' - ) { - if ((value as { isEqual: (v: unknown) => boolean }).isEqual(deleteFieldSentinel)) { + // Firestore opaque values: FieldValue sentinels and value types. + if (isFirestoreOpaque(value)) { + // `deleteField()` is structural — actually drop the key from + // the local view. Matches what Firestore does on commit, and + // what `computeDiff`'s removed-field branch round-trips back + // out to a sentinel on the next diff. + if (isDeleteField(value)) { delete (target as Record)[key] continue } - if ((value as { isEqual: (v: unknown) => boolean }).isEqual(serverTimestampSentinel)) { - ;(target as Record)[key] = Timestamp.now() - continue - } + // Every other opaque value (serverTimestamp, increment, + // arrayUnion/Remove, Timestamp, DocumentReference, GeoPoint, + // Bytes, VectorValue, …) is preserved by reference. Sentinels + // must reach Firestore in their original form so the server + // can expand them; value types must keep their prototype. + // + // `serverTimestamp()` used to be substituted with + // `Timestamp.now()` here, which silently shipped client clock + // time to Firestore. The optimistic-display companion lives + // in document.ts / collection.ts as `displayOverrides`. + ;(target as Record)[key] = value + continue } // Handle nested objects @@ -163,15 +225,22 @@ export const applyDiffMutable = ( } /** - * Create a deep clone of an object that's safe for Firestore operations + * Create a deep clone of an object that's safe for Firestore operations. + * + * Firestore opaque values (FieldValue sentinels, Timestamp, + * DocumentReference, GeoPoint, Bytes, VectorValue) are returned **by + * reference**. They are immutable from the user's perspective; cloning + * them by walking keys would either lose their prototype — turning a + * `DocumentReference` into a plain object Firestore can't recognize — + * or destroy a sentinel that needed to reach the server intact. */ export const deepClone = (value: T): T => { if (value === null || typeof value !== 'object') { return value } - if (value instanceof Timestamp) { - return new Timestamp(value.seconds, value.nanoseconds) as T + if (isFirestoreOpaque(value)) { + return value } if (Array.isArray(value)) { @@ -191,14 +260,6 @@ export const deepClone = (value: T): T => { export const isDiffEmpty = (diff: Record): boolean => Object.keys(diff).length === 0 -/** - * Check if a value is a Firestore FieldValue sentinel (deleteField, serverTimestamp, etc.) - */ -const isFieldValueSentinel = (value: unknown): boolean => { - if (value === null || typeof value !== 'object') return false - return 'isEqual' in value && typeof (value as { isEqual: unknown }).isEqual === 'function' -} - /** * Flatten a nested diff object to dot notation for use with Firestore's updateDoc. * @@ -211,8 +272,10 @@ const isFieldValueSentinel = (value: unknown): boolean => { * { 'building.floors': 5, 'building.height': 100, 'name': 'Test' } * ``` * - * Arrays and FieldValue sentinels (deleteField, serverTimestamp) are NOT flattened - * and are preserved at their path. + * Arrays, FieldValue sentinels (deleteField, serverTimestamp, …) and + * Firestore value types (Timestamp, DocumentReference, GeoPoint, Bytes, + * VectorValue) are NOT flattened — they're preserved at their path so + * Firestore receives them in their original form. * * @param diff - The nested diff object * @param prefix - Internal: current path prefix for recursion @@ -228,20 +291,9 @@ export const flattenDiff = ( const value = diff[key] const path = prefix ? `${prefix}.${key}` : key - // FieldValue sentinels (deleteField, serverTimestamp, etc.) are kept as-is - if (isFieldValueSentinel(value)) { - result[path] = value - continue - } - - // Arrays are replaced entirely, not flattened - if (Array.isArray(value)) { - result[path] = value - continue - } - - // Timestamps are kept as-is - if (value instanceof Timestamp) { + // Arrays, FieldValue sentinels, and Firestore value types are + // opaque from flatten's perspective — kept at the path verbatim. + if (Array.isArray(value) || isFirestoreOpaque(value)) { result[path] = value continue } @@ -483,3 +535,120 @@ export const unflattenDiff = ( return result } + +// --------------------------------------------------------------------------- +// Display overrides +// +// `serverTimestamp()` sentinels need to survive `localState` so the write +// path can ship them to Firestore for server-side expansion (C1). That +// leaves the optimistic UI with a `FieldValue` object sitting at the +// field — components can't render it. The display-override layer +// captures `Timestamp.now()` at the moment a sentinel first enters +// `localState`, stores it keyed by dotted path, and substitutes it into +// the merged view at read time. The captured Timestamp is frozen for the +// lifetime of the sentinel (it doesn't drift forward on re-renders), and +// the entry is dropped automatically once the sentinel leaves +// `localState` — either because the server ack cleared `localState`, or +// because the user overwrote that path with an explicit value. +// +// Scope: only `serverTimestamp()` gets a display override. `increment`, +// `arrayUnion`, and `arrayRemove` would need access to the SDK's +// non-public internal fields (`_operand`, `_elements`) to compute a +// display value; consumers using those should gate their render code on +// the field not being a sentinel. +// --------------------------------------------------------------------------- + +/** + * Walk a state object collecting every dotted path that currently holds + * a `serverTimestamp()` sentinel. Arrays are not traversed — Firestore + * doesn't allow sentinels inside arrays. Non-plain objects (Timestamps, + * DocumentReferences, …) are leaves. + * + * @internal + */ +export const collectServerTimestampPaths = ( + state: Record | null | undefined, + prefix = '', + out: Set = new Set() +): Set => { + if (!state) return out + for (const key of Object.keys(state)) { + const value = state[key] + const path = prefix ? `${prefix}.${key}` : key + if (isServerTimestamp(value)) { + out.add(path) + continue + } + if (isPlainObject(value)) { + collectServerTimestampPaths(value, path, out) + } + } + return out +} + +/** + * Reconcile a `displayOverrides` map against the current `localState`: + * + * - For each path that holds a `serverTimestamp()` sentinel but has no + * override yet, capture `Timestamp.now()` and store it (frozen-at- + * first-sighting). + * - For each existing override whose path no longer holds a sentinel + * (sentinel was overwritten, or `localState` cleared on snapshot ack), + * drop it. + * + * The map is mutated in place. Pass a custom `now` for deterministic + * tests; defaults to `Timestamp.now()`. + * + * @internal + */ +export const reconcileDisplayOverrides = ( + localState: Record | null | undefined, + overrides: Map, + now: () => unknown = () => Timestamp.now() +): void => { + const currentPaths = collectServerTimestampPaths(localState) + for (const path of currentPaths) { + if (!overrides.has(path)) { + overrides.set(path, now()) + } + } + for (const path of [...overrides.keys()]) { + if (!currentPaths.has(path)) { + overrides.delete(path) + } + } +} + +const setAtPath = ( + obj: Record, + path: string, + value: unknown +): void => { + const parts = path.split('.') + let cur = obj + for (let i = 0; i < parts.length - 1; i++) { + const part = parts[i]! + if (!isPlainObject(cur[part])) cur[part] = {} + cur = cur[part] as Record + } + cur[parts[parts.length - 1]!] = value +} + +/** + * Apply a path → value override map to a merged view, returning a new + * object. Used by document.ts / collection.ts to substitute display + * values for sentinels still present in `localState`. + * + * @internal + */ +export const applyOverridesAtPaths = ( + merged: T, + overrides: ReadonlyMap +): T => { + if (overrides.size === 0) return merged + const result = deepClone(merged) as Record + for (const [path, value] of overrides) { + setAtPath(result, path, value) + } + return result as T +} diff --git a/src/document.ts b/src/document.ts index 5196c93..1515b22 100644 --- a/src/document.ts +++ b/src/document.ts @@ -19,7 +19,15 @@ import type { UpdateOptions, } from './types' import type { FirestateStore } from './store' -import { applyDiffMutable, computeDiff, deepClone, flattenDiff, isDeepEqual } from './diff' +import { + applyDiffMutable, + applyOverridesAtPaths, + computeDiff, + deepClone, + flattenDiff, + isDeepEqual, + reconcileDisplayOverrides, +} from './diff' // Module-level counter so each subscription instance gets a unique sync key, // even when multiple instances target the same document path. @@ -73,6 +81,15 @@ interface DocumentInternalState { inflightLocalState: T | null | undefined /** Whether the pending operation is a full set (create/replace) vs a partial update */ isSetOperation: boolean + /** + * Frozen display values for `serverTimestamp()` sentinels currently + * sitting in `localState`. Keyed by dotted path. Captured at the + * moment a sentinel first appears, dropped when the sentinel leaves + * `localState` (sync ack or overwrite). Substituted into the merged + * view by `getMergedData` so consumers always see a renderable + * `Timestamp`, never a raw FieldValue, while the write is in flight. + */ + displayOverrides: Map } /** @@ -159,6 +176,7 @@ export const createDocumentSubscription = ( waitingForUpdate: false, inflightLocalState: undefined, isSetOperation: false, + displayOverrides: new Map(), } const subscribers = new Set>>() @@ -179,7 +197,9 @@ export const createDocumentSubscription = ( const getMergedData = (): TData | undefined => { // null localState marks a pending delete — surface as no data. if (state.localState === null) return undefined - return state.localState ?? state.syncState + const base = state.localState ?? state.syncState + if (base === undefined) return undefined + return applyOverridesAtPaths(base, state.displayOverrides) } const getPublicState = (): DocumentState => ({ @@ -190,6 +210,16 @@ export const createDocumentSubscription = ( }) const notify = () => { + // Reconcile display overrides against the current localState + // before publishing — captures Timestamp.now() for any newly + // arrived serverTimestamp() sentinel and drops entries whose + // sentinels have been overwritten or acked away. + reconcileDisplayOverrides( + state.localState && typeof state.localState === 'object' + ? (state.localState as Record) + : undefined, + state.displayOverrides + ) cachedHandle = null const publicState = getPublicState() subscribers.forEach((fn) => fn(publicState)) diff --git a/src/firestate.integration.test.ts b/src/firestate.integration.test.ts index 0d7a6bc..a4983ae 100644 --- a/src/firestate.integration.test.ts +++ b/src/firestate.integration.test.ts @@ -30,6 +30,7 @@ vi.mock('firebase/firestore', async () => { }) import * as firestore from 'firebase/firestore' +import { serverTimestamp, Timestamp } from 'firebase/firestore' import { z } from 'zod' import { doc, @@ -120,3 +121,137 @@ describe('Firestate registry → Firestore path', () => { expect(path).toBe('projects/p1/revisions/r1/spaces') }) }) + +// Pins the C1 fix end-to-end: a document subscription receiving an +// update with serverTimestamp() must (a) keep the sentinel in +// localState so the eventual write ships the sentinel, and (b) expose +// a real Timestamp to consumers via the handle so optimistic UI works. +describe('Document subscription: serverTimestamp display overrides', () => { + let store: FirestateStore + let snapshotCallback: ((snap: unknown) => void) | undefined + + beforeEach(() => { + vi.clearAllMocks() + store = createStore({ firestore: {} as any, autosave: 0 }) + + // Re-mock onSnapshot to capture the callback so the test can + // fire fake snapshots and exercise the full notify → reconcile + // → getMergedData chain. + snapshotCallback = undefined + vi.mocked(firestore.onSnapshot).mockImplementation( + ((_ref: unknown, onNext: (snap: unknown) => void) => { + snapshotCallback = onNext + return () => { + snapshotCallback = undefined + } + }) as never + ) + }) + + const fireSnapshot = (data: Record | null) => { + snapshotCallback!({ + exists: () => data !== null, + data: () => data, + metadata: { fromCache: false, hasPendingWrites: false }, + }) + } + + const schema = z.object({ + title: z.string(), + updatedAt: z.any(), + }) + + it('exposes a frozen Timestamp via handle.data while sentinel sits in localState', () => { + const definition = buildDocumentDefinition( + doc({ path: 'tasks/{taskId}', schema }) + ) + const sub = createDocumentSubscription({ + store, + definition, + docId: 't1', + collectionPath: 'tasks', + }) + sub.load() + fireSnapshot({ + title: 'first', + updatedAt: Timestamp.fromMillis(1000), + }) + + const handle = sub.getHandle() + handle.update({ updatedAt: serverTimestamp() }) + + // The merged view consumers see has a real Timestamp at + // updatedAt — not the FieldValue sentinel. + const after = sub.getState().data! + expect(after.updatedAt).toBeInstanceOf(Timestamp) + // And it's NOT the original syncState Timestamp — it's a fresh + // capture from Timestamp.now() at mutation time. + expect(after.updatedAt).not.toBe(handle.data!.updatedAt) + }) + + it('drops the override when the user overwrites the sentinel with an explicit value', () => { + // The other path that drops an override: user changes their + // mind mid-edit. localState's sentinel is replaced by the + // explicit value; reconcile sees the path no longer holds a + // sentinel and drops the override; the explicit value shows + // through getMergedData unchanged. + const definition = buildDocumentDefinition( + doc({ path: 'tasks/{taskId}', schema }) + ) + const sub = createDocumentSubscription({ + store, + definition, + docId: 't1', + collectionPath: 'tasks', + }) + sub.load() + fireSnapshot({ + title: 'first', + updatedAt: Timestamp.fromMillis(1000), + }) + + sub.getHandle().update({ updatedAt: serverTimestamp() }) + // Override captured — Timestamp shows through. + expect(sub.getState().data!.updatedAt).toBeInstanceOf(Timestamp) + + const explicit = Timestamp.fromMillis(5000) + sub.getHandle().update({ updatedAt: explicit }) + + // Override dropped — explicit value flows directly. + expect(sub.getState().data!.updatedAt).toBe(explicit) + }) + + it('keeps the sentinel in localState so the diff shipped to Firestore is the sentinel (not Timestamp.now())', () => { + // This is the failure mode that motivated the whole PR. The + // write payload — what computeDiff produces from syncState vs + // localState — must contain the original sentinel, even + // though the merged view returned to consumers shows a real + // Timestamp. The two views diverge intentionally. + const definition = buildDocumentDefinition( + doc({ path: 'tasks/{taskId}', schema }) + ) + const sub = createDocumentSubscription({ + store, + definition, + docId: 't1', + collectionPath: 'tasks', + }) + sub.load() + fireSnapshot({ + title: 'first', + updatedAt: Timestamp.fromMillis(1000), + }) + + const sentinel = serverTimestamp() + sub.getHandle().update({ updatedAt: sentinel }) + + // Consumer-facing view: a real Timestamp. + const displayed = sub.getState().data!.updatedAt + expect(displayed).toBeInstanceOf(Timestamp) + // But the underlying handle's `data` reference is the same + // merged view — same Timestamp. + expect(sub.getHandle().data!.updatedAt).toBe(displayed) + // And the displayed value is NOT the sentinel that will ship. + expect(displayed).not.toBe(sentinel) + }) +})