fix: preserve Firestore opaque values through the diff pipeline

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<T extends FirestoreObject> {
     error: Error | undefined
     waitingForUpdate: boolean
     inflightLocalState: Record<string, T> | undefined
+    /**
+     * Frozen display values for `serverTimestamp()` sentinels currently
+     * sitting in `localState`. Keyed by dotted path (e.g.
+     * `"<docId>.updatedAt"`). See document.ts for the full contract.
+     */
+    displayOverrides: Map<string, unknown>
 }
 
 /**
@@ -139,6 +153,7 @@ export const createCollectionSubscription = <TData extends FirestoreObject>(
         error: undefined,
         waitingForUpdate: false,
         inflightLocalState: undefined,
+        displayOverrides: new Map(),
     }
 
     const subscribers = new Set<Subscriber<CollectionState<TData>>>()
@@ -156,8 +171,10 @@ export const createCollectionSubscription = <TData extends FirestoreObject>(
     // subscriptions to the same path don't share (or clobber) one entry.
     const syncKey = `col:${collectionPath}#${++syncKeyCounter}`
 
-    const getMergedData = (): Record<string, TData> =>
-        state.localState ?? state.syncState ?? {}
+    const getMergedData = (): Record<string, TData> => {
+        const base = state.localState ?? state.syncState ?? {}
+        return applyOverridesAtPaths(base, state.displayOverrides)
+    }
 
     const getPublicState = (): CollectionState<TData> => ({
         data: getMergedData(),
@@ -168,6 +185,12 @@ export const createCollectionSubscription = <TData extends FirestoreObject>(
     })
 
     const notify = () => {
+        // Reconcile display overrides against the current localState
+        // before publishing — see document.ts for the full contract.
+        reconcileDisplayOverrides(
+            state.localState as Record<string, unknown> | undefined,
+            state.displayOverrides
+        )
         cachedHandle = null
         const publicState = getPublicState()
         subscribers.forEach((fn) => fn(publicState))

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<string, unknown> = {}
+            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<string, unknown>
+
+            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<string, unknown>
+
+            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<string, unknown>
+
+            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<string, unknown>
+
+            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<string, unknown>
+
+            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<string, unknown>
+
+            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<string, unknown>()
+                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<string, unknown>()
+                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<string, unknown>()
+                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<string, unknown>()
+                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<string, unknown>([['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<string, unknown>([['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<string, unknown>([['updatedAt', ts]])
+
+                applyOverridesAtPaths(original, overrides)
+
+                // The original still holds the sentinel — overrides are
+                // a render-time concern only.
+                expect(original.updatedAt).not.toBe(ts)
+            })
+        })
+    })
 })