diff --git a/README.md b/README.md index e984a15..307d193 100644 --- a/README.md +++ b/README.md @@ -306,6 +306,48 @@ To skip undo tracking: project.update({ lastViewed: Date.now() }, { undoable: false }) ``` +#### Navigation-aware undo/redo + +When an undo action is tagged with a `path`, undo/redo can return the user to +the route where the change occurred before reverting it. Wire your router's +`navigate` via `onNavigate` on `FirestateProvider`: + +```tsx +import { useNavigate } from 'react-router-dom' + +function App() { + const navigate = useNavigate() + + return ( + navigate(path)} + > + {children} + + ) +} +``` + +When creating the store manually, pass `onNavigate` to `createStore`: + +```ts +const store = createStore({ + firestore: db, + onNavigate: (path) => router.push(path), +}) +``` + +Actions record a path via the `path` field on `UndoAction`: + +```tsx +undoManager.push({ + undo: () => restoreValue(), + redo: () => applyValue(), + path: '/projects/123', // navigate here on undo/redo +}) +``` + ### Lazy Collections For large applications, you may not want to subscribe to every collection immediately: @@ -592,6 +634,7 @@ Main provider component. autosave={1000} // Optional: default debounce (ms) minLoadTime={0} // Optional: minimum loading time (ms) maxUndoLength={20} // Optional: max undo stack size + onNavigate={(path) => navigate(path)} // Optional: router navigate for path-aware undo/redo onError={(error, context) => { // Optional: custom error handler console.error(context.path, error) diff --git a/src/collection.ts b/src/collection.ts index 5582d3d..dcf3ae8 100644 --- a/src/collection.ts +++ b/src/collection.ts @@ -220,8 +220,12 @@ export const createCollectionSubscription = ( return } - const currentData = getMergedData() - const newLocalState = deepClone(currentData) + // Use raw localState as the mutation base so serverTimestamp() sentinels + // in localState survive into newLocalState. getMergedData() substitutes + // display-override Timestamps at sentinel paths, which would erase the + // sentinel from state.localState on the next update() call. + const rawBase = state.localState ?? state.syncState ?? {} + const newLocalState = deepClone(rawBase) applyDiffMutable(newLocalState, diff as Record) // Ensure each document has its id @@ -238,10 +242,10 @@ export const createCollectionSubscription = ( if (undoOptions?.undoable !== false && onPushUndo) { const undoDiff = computeDiff( newLocalState as FirestoreObject, - currentData as FirestoreObject + rawBase as FirestoreObject ) const redoDiff = computeDiff( - currentData as FirestoreObject, + rawBase as FirestoreObject, newLocalState as FirestoreObject ) onPushUndo( diff --git a/src/document.ts b/src/document.ts index 1515b22..03e2ad1 100644 --- a/src/document.ts +++ b/src/document.ts @@ -244,7 +244,12 @@ export const createDocumentSubscription = ( return } - const newLocalState = deepClone(currentData) + // Use raw localState as the mutation base so serverTimestamp() sentinels + // in localState survive into newLocalState. getMergedData() substitutes + // display-override Timestamps at sentinel paths, which would erase the + // sentinel from state.localState on the next update() call. + const rawBase = (state.localState ?? state.syncState) as TData + const newLocalState = deepClone(rawBase) applyDiffMutable(newLocalState, diff as Record) // Push undo eagerly against the pre-mutation state. Cmd+Z within the @@ -254,10 +259,10 @@ export const createDocumentSubscription = ( if (undoOptions?.undoable !== false && onPushUndo) { const undoDiff = computeDiff( newLocalState as FirestoreObject, - currentData as FirestoreObject + rawBase as FirestoreObject ) const redoDiff = computeDiff( - currentData as FirestoreObject, + rawBase as FirestoreObject, newLocalState as FirestoreObject ) onPushUndo( @@ -303,7 +308,10 @@ export const createDocumentSubscription = ( undoOptions ) } else { - const dataToRestore = deepClone(currentData) + // Snapshot raw localState so the restore payload contains + // serverTimestamp() sentinels, not the frozen Timestamps that + // getMergedData() substitutes for display purposes. + const dataToRestore = deepClone((state.localState ?? state.syncState) as TData) onPushUndo( () => setData(dataToRestore, { undoable: false }), () => setData(dataForRedo, { undoable: false }), @@ -329,7 +337,8 @@ export const createDocumentSubscription = ( // Push undo against the pre-delete data (which includes any pending // local edits at this moment). if (undoOptions?.undoable !== false && onPushUndo) { - const dataToRestore = deepClone(currentData) + // Snapshot raw localState — same reason as in setData above. + const dataToRestore = deepClone((state.localState ?? state.syncState) as TData) onPushUndo( () => setData(dataToRestore, { undoable: false }), () => deleteDocument({ undoable: false }), diff --git a/src/firestate.integration.test.ts b/src/firestate.integration.test.ts index a4983ae..8c12c8e 100644 --- a/src/firestate.integration.test.ts +++ b/src/firestate.integration.test.ts @@ -254,4 +254,75 @@ describe('Document subscription: serverTimestamp display overrides', () => { // And the displayed value is NOT the sentinel that will ship. expect(displayed).not.toBe(sentinel) }) + + it('a chained update() after a serverTimestamp update keeps the sentinel in localState', () => { + // Regression: updateState used getMergedData() as the mutation base. + // getMergedData() substitutes the display-override Timestamp at the + // sentinel path; a second update() would clone that Timestamp into + // newLocalState, silently erasing the sentinel. If the sentinel is + // erased, reconcileDisplayOverrides drops the override and + // getMergedData() falls back to the syncState Timestamp(1000). + 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() }) + // Chain a second update that doesn't touch updatedAt. + sub.getHandle().update({ title: 'second' }) + + // Sentinel must still be in localState. Proxy: the display override is + // still active, so updatedAt is NOT the syncState Timestamp(1000). If the + // sentinel had been erased, the override would drop and Timestamp(1000) would show. + const displayed = sub.getState().data!.updatedAt + expect(displayed).toBeInstanceOf(Timestamp) + expect(displayed).not.toEqual(Timestamp.fromMillis(1000)) + expect(sub.getState().data!.title).toBe('second') + }) + + it('setData undo restore payload contains the sentinel, not a frozen client Timestamp', async () => { + // Regression: setData snapshotted deepClone(getMergedData()) for the + // undo restore payload. getMergedData() substitutes frozen Timestamps + // for sentinels; undo would then call setData() with a client Timestamp, + // re-introducing the C1 regression through the undo path. + const definition = buildDocumentDefinition( + doc({ path: 'tasks/{taskId}', schema }) + ) + const sub = createDocumentSubscription({ + store, + definition, + docId: 't1', + collectionPath: 'tasks', + onPushUndo: (undoFn, redoFn, opts) => + store.undoManager.push({ undo: undoFn, redo: redoFn, groupId: opts?.undoGroupId }), + }) + sub.load() + fireSnapshot({ title: 'first', updatedAt: Timestamp.fromMillis(1000) }) + + sub.getHandle().update({ updatedAt: serverTimestamp() }) + // Display override active — shows a Timestamp that is not the syncState value. + expect(sub.getState().data!.updatedAt).not.toEqual(Timestamp.fromMillis(1000)) + + // set() captures the undo restore snapshot of the pre-set state (with sentinel). + sub.getHandle().set({ title: 'replaced', updatedAt: Timestamp.fromMillis(9999) }) + expect(sub.getState().data!.title).toBe('replaced') + + // Undo restores the pre-set state. Sentinel is restored into localState, + // triggering a fresh display-override capture. + await store.undoManager.undo() + + expect(sub.getState().data!.title).toBe('first') + // updatedAt must NOT be Timestamp(9999) — that would mean the undo payload + // held the display-override Timestamp rather than the sentinel. + expect(sub.getState().data!.updatedAt).not.toEqual(Timestamp.fromMillis(9999)) + // It IS a Timestamp (display override fired for the restored sentinel). + expect(sub.getState().data!.updatedAt).toBeInstanceOf(Timestamp) + }) }) diff --git a/src/provider.tsx b/src/provider.tsx index 35ba162..595922e 100644 --- a/src/provider.tsx +++ b/src/provider.tsx @@ -21,6 +21,26 @@ export interface FirestateProviderProps { minLoadTime?: number; /** Maximum undo stack length, default 20 */ maxUndoLength?: number; + /** + * Called before undo/redo when the action carries a `path`. Wire your + * router's `navigate` here to return users to where a change occurred + * before reverting it. + * + * @example + * ```tsx + * import { useNavigate } from 'react-router-dom' + * + * function App() { + * const navigate = useNavigate() + * return ( + * navigate(path)}> + * {children} + * + * ) + * } + * ``` + */ + onNavigate?: (path: string) => void; /** Custom error handler */ onError?: (error: Error, context: ErrorContext) => void; /** React children */ @@ -55,12 +75,14 @@ export const FirestateProvider: React.FC = ({ minLoadTime = 0, maxUndoLength = 20, onError, + onNavigate, children, }) => { - // onError is intentionally excluded from the deps so that an inline - // callback (new reference per render) does not re-create the store and - // drop every existing subscription. The store exposes setOnError so the - // latest handler can be applied without store re-creation. + // onError and onNavigate are intentionally excluded from the deps so that + // inline callbacks (new reference per render) do not re-create the store and + // drop every existing subscription. The store exposes setOnError / + // setOnNavigate so the latest handlers can be applied without store + // re-creation. const store = useMemo( () => createStore({ @@ -69,6 +91,7 @@ export const FirestateProvider: React.FC = ({ minLoadTime, maxUndoLength, onError, + onNavigate, }), [firestore, autosave, minLoadTime, maxUndoLength] ); @@ -77,6 +100,10 @@ export const FirestateProvider: React.FC = ({ store.setOnError(onError); }, [store, onError]); + useEffect(() => { + store.setOnNavigate(onNavigate); + }, [store, onNavigate]); + return ( {children} diff --git a/src/store.test.ts b/src/store.test.ts index 67517a4..a67f661 100644 --- a/src/store.test.ts +++ b/src/store.test.ts @@ -210,6 +210,66 @@ describe('createStore', () => { }) }) + describe('onNavigate integration', () => { + it('calls onNavigate before undo when action has a path', async () => { + const onNavigate = vi.fn() + const store = createStore({ firestore: mockFirestore, onNavigate }) + + store.undoManager.push({ undo: vi.fn(), redo: vi.fn(), path: '/route/123' }) + await store.undoManager.undo() + + expect(onNavigate).toHaveBeenCalledWith('/route/123') + }) + + it('calls onNavigate before redo when action has a path', async () => { + const onNavigate = vi.fn() + const store = createStore({ firestore: mockFirestore, onNavigate }) + + store.undoManager.push({ undo: vi.fn(), redo: vi.fn(), path: '/route/123' }) + await store.undoManager.undo() + await store.undoManager.redo() + + expect(onNavigate).toHaveBeenCalledTimes(2) + expect(onNavigate).toHaveBeenLastCalledWith('/route/123') + }) + + it('does not call onNavigate when action has no path', async () => { + const onNavigate = vi.fn() + const store = createStore({ firestore: mockFirestore, onNavigate }) + + store.undoManager.push({ undo: vi.fn(), redo: vi.fn() }) + await store.undoManager.undo() + + expect(onNavigate).not.toHaveBeenCalled() + }) + + it('setOnNavigate replaces the handler without recreating the store', async () => { + const first = vi.fn() + const second = vi.fn() + const store = createStore({ firestore: mockFirestore, onNavigate: first }) + + store.setOnNavigate(second) + + store.undoManager.push({ undo: vi.fn(), redo: vi.fn(), path: '/route/456' }) + await store.undoManager.undo() + + expect(first).not.toHaveBeenCalled() + expect(second).toHaveBeenCalledWith('/route/456') + }) + + it('setOnNavigate with undefined disables navigation', async () => { + const onNavigate = vi.fn() + const store = createStore({ firestore: mockFirestore, onNavigate }) + + store.setOnNavigate(undefined) + + store.undoManager.push({ undo: vi.fn(), redo: vi.fn(), path: '/route/789' }) + await store.undoManager.undo() + + expect(onNavigate).not.toHaveBeenCalled() + }) + }) + describe('undo manager integration', () => { it('provides access to undo manager', () => { const store = createStore({ diff --git a/src/store.ts b/src/store.ts index f16fce6..6c658a4 100644 --- a/src/store.ts +++ b/src/store.ts @@ -22,6 +22,12 @@ export interface FirestateStore { * callback that changes reference on every render. */ setOnError: (handler?: (error: Error, context: ErrorContext) => void) => void + /** + * Replace the navigation handler at runtime. Used by FirestateProvider to + * keep the store identity stable when consumers pass an inline `onNavigate` + * callback that changes reference on every render. + */ + setOnNavigate: (handler?: (path: string) => void) => void /** Subscribe to sync state changes */ subscribeToSyncState: (fn: Subscriber) => Unsubscribe /** Report a document/collection sync state change */ @@ -62,11 +68,15 @@ export const createStore = (config: FirestateConfig): FirestateStore => { maxUndoLength = 20, } = config - // Mutable so the provider can update it without re-creating the store. + // Mutable so the provider can update them without re-creating the store. let onError = config.onError + let onNavigate = config.onNavigate const undoManager = createUndoManager({ maxLength: maxUndoLength, + // Stable wrapper — delegates to the mutable onNavigate ref so the + // undo manager doesn't need to be recreated when the callback changes. + onNavigate: (path) => onNavigate?.(path), }) // Track sync state of all documents/collections @@ -106,6 +116,10 @@ export const createStore = (config: FirestateConfig): FirestateStore => { onError = handler }, + setOnNavigate: (handler) => { + onNavigate = handler + }, + subscribeToSyncState: (fn) => { syncSubscribers.add(fn) return () => syncSubscribers.delete(fn) diff --git a/src/types.ts b/src/types.ts index c5eb860..ef4e45b 100644 --- a/src/types.ts +++ b/src/types.ts @@ -266,8 +266,12 @@ export interface FirestateConfig { minLoadTime?: number; /** Maximum undo stack length, default 20 */ maxUndoLength?: number; - /** Enable navigation-aware undo/redo */ - enableNavigation?: boolean; + /** + * Callback invoked before undo/redo when the action carries a `path`. + * Wire your router's `navigate` here so undo/redo returns the user to + * where a change occurred before reverting it. + */ + onNavigate?: (path: string) => void; /** Custom error handler */ onError?: (error: Error, context: ErrorContext) => void; }