From 68abb418b18f9a0be6fc8ede05a54730c67dc785 Mon Sep 17 00:00:00 2001 From: Jeremy Weinstein Date: Tue, 12 May 2026 20:44:56 -0700 Subject: [PATCH] feat: avoid no-op draft writes --- .../root-cms/ui/hooks/useDraftDoc.test.ts | 50 +++++++++++++++++++ packages/root-cms/ui/hooks/useDraftDoc.tsx | 46 ++++++++++++++++- 2 files changed, 95 insertions(+), 1 deletion(-) create mode 100644 packages/root-cms/ui/hooks/useDraftDoc.test.ts diff --git a/packages/root-cms/ui/hooks/useDraftDoc.test.ts b/packages/root-cms/ui/hooks/useDraftDoc.test.ts new file mode 100644 index 000000000..e34bab5da --- /dev/null +++ b/packages/root-cms/ui/hooks/useDraftDoc.test.ts @@ -0,0 +1,50 @@ +import {describe, expect, it} from 'vitest'; +import {testDraftUpdateChangesValue} from './useDraftDoc.js'; + +describe('testDraftUpdateChangesValue', () => { + it('returns false for deeply equal values', () => { + expect( + testDraftUpdateChangesValue( + {items: [{title: 'One'}, {title: 'Two'}]}, + {items: [{title: 'One'}, {title: 'Two'}]} + ) + ).toBe(false); + }); + + it('returns true when values differ', () => { + expect(testDraftUpdateChangesValue('Published title', 'Draft title')).toBe( + true + ); + }); + + it('treats same-reference objects as changed', () => { + const value = {title: 'Maybe mutated in place'}; + + expect(testDraftUpdateChangesValue(value, value)).toBe(true); + }); + + it('returns false when deleting an absent value', () => { + expect(testDraftUpdateChangesValue(undefined, undefined)).toBe(false); + }); + + it('returns true when deleting a present null value', () => { + expect(testDraftUpdateChangesValue(null, undefined)).toBe(true); + }); + + it('can treat null as a delete operation for batch updates', () => { + expect( + testDraftUpdateChangesValue(undefined, null, {deleteNull: true}) + ).toBe(false); + expect(testDraftUpdateChangesValue(null, null, {deleteNull: true})).toBe( + true + ); + }); + + it('allows changed metadata array updates', () => { + expect(testDraftUpdateChangesValue(['en'], ['en', 'es'])).toBe(true); + }); + + it('skips unchanged metadata array updates', () => { + expect(testDraftUpdateChangesValue(['en', 'es'], ['en', 'es'])).toBe(false); + }); +}); diff --git a/packages/root-cms/ui/hooks/useDraftDoc.tsx b/packages/root-cms/ui/hooks/useDraftDoc.tsx index 060a71372..dc37fc4fe 100644 --- a/packages/root-cms/ui/hooks/useDraftDoc.tsx +++ b/packages/root-cms/ui/hooks/useDraftDoc.tsx @@ -22,6 +22,7 @@ import { UnsubscribeCallback, } from '../utils/json-trie-store.js'; import {errorMessage} from '../utils/notifications.js'; +import {deepEqual} from '../utils/objects.js'; import {TIME_UNITS} from '../utils/time.js'; const SAVE_DELAY = 3 * TIME_UNITS.second; @@ -200,6 +201,10 @@ export class DraftDocController extends EventListener { if (this.readOnly) { return; } + if (!testDraftUpdateChangesValue(this.store.get(key), value)) { + // Avoid metadata-only writes that make published docs look like drafts. + return; + } if (value === undefined) { // Firestore doesn't support `undefined`, so use deleteField() instead. this.pendingUpdates.set(key, deleteField()); @@ -220,8 +225,16 @@ export class DraftDocController extends EventListener { if (this.readOnly) { return; } + const changedUpdates: Record = {}; for (const key in updates) { const val = updates[key]; + if ( + !testDraftUpdateChangesValue(this.store.get(key), val, { + deleteNull: true, + }) + ) { + continue; + } if (val === null || val === undefined) { // Firestore doesn't support `undefined`, so use deleteField() instead. // NOTE(stevenle): this doesn't currently handle nested `undefined` @@ -230,8 +243,13 @@ export class DraftDocController extends EventListener { } else { this.pendingUpdates.set(key, val); } + changedUpdates[key] = val; + } + if (Object.keys(changedUpdates).length === 0) { + // Avoid metadata-only writes when every batch update is a no-op. + return; } - this.store.update(updates); + this.store.update(changedUpdates); this.setSaveState(SaveState.UPDATES_PENDING); if (this.autoSave) { this.queueChanges(); @@ -245,6 +263,10 @@ export class DraftDocController extends EventListener { if (this.readOnly) { return; } + if (!testDraftUpdateChangesValue(this.store.get(key), undefined)) { + // Avoid metadata-only writes when deleting a value that is already absent. + return; + } this.pendingUpdates.set(key, deleteField()); this.store.set(key, undefined); this.setSaveState(SaveState.UPDATES_PENDING); @@ -386,6 +408,28 @@ export class DraftDocController extends EventListener { } } +/** + * Returns whether a requested draft update would change the current value. + * This prevents no-op field edits from being saved as metadata-only writes. + */ +export function testDraftUpdateChangesValue( + currentValue: any, + newValue: any, + options?: {deleteNull?: boolean} +): boolean { + if (newValue === undefined || (options?.deleteNull && newValue === null)) { + return currentValue !== undefined; + } + if (currentValue === newValue && isMutableDraftValue(currentValue)) { + return true; + } + return !deepEqual(currentValue, newValue); +} + +function isMutableDraftValue(value: any): boolean { + return typeof value === 'object' && value !== null; +} + function applyUpdates(data: any, updates: any) { for (const key in updates) { const val = updates[key];