From e7973824dc7606f0c5643967253951fae7204255 Mon Sep 17 00:00:00 2001 From: Aymeric Rabot Date: Tue, 4 Aug 2026 14:34:45 -0400 Subject: [PATCH] fix(editor): keep the autosave wipe guard armed after the scene loads MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `useAutoSave` refuses to persist a graph that drops from populated to a bare scaffold, on the assumption that it is an accidental full deletion. The baseline it compares against was seeded once when the hook mounted — which happens before the scene has loaded, so it sat at the scaffold count for the whole session and the guard could never fire. The one write it exists to stop is the one it let through: an autosave racing the initial load overwrites the stored scene with the scaffold. The loading branch of the store subscription already refreshed the snapshot, collections, materials and plugin refs; it just never refreshed the count. Rather than add a fourth assignment to a branch whose contract was implicit, the baseline now lives in `createStoredNodeCountTracker`, which distinguishes the two things that were being conflated: a graph read from storage becomes the new baseline, an edited graph does not. That also removes the duplicated guard between `executeSave` and `flushOnExit`, and makes the invariant testable without React — the same approach `floorplan-camera-sync.ts` takes for its closure state. Surfaced by @evolv3ai in #551, which fixed the symptom with a `nodeCount === 0` check in the standalone app's save route. This fixes it in the shared hook instead, so the hosted editor and npm consumers are covered too, and a blocked write can't be laundered through the 409 conflict path that `scene-loader.tsx` treats as success. Co-Authored-By: Claude Opus 5 --- .../editor/src/hooks/use-auto-save.test.ts | 51 +++++++++++++++++- packages/editor/src/hooks/use-auto-save.ts | 53 +++++++++++++++---- 2 files changed, 94 insertions(+), 10 deletions(-) diff --git a/packages/editor/src/hooks/use-auto-save.test.ts b/packages/editor/src/hooks/use-auto-save.test.ts index 9f40569fc6..593f6d1d56 100644 --- a/packages/editor/src/hooks/use-auto-save.test.ts +++ b/packages/editor/src/hooks/use-auto-save.test.ts @@ -1,5 +1,5 @@ import { describe, expect, test } from 'bun:test' -import { isSuspiciousNodeDrop } from './use-auto-save' +import { createStoredNodeCountTracker, isSuspiciousNodeDrop } from './use-auto-save' describe('isSuspiciousNodeDrop', () => { test('blocks populated scenes from being flushed as empty skeletons', () => { @@ -12,3 +12,52 @@ describe('isSuspiciousNodeDrop', () => { expect(isSuspiciousNodeDrop(4, 0)).toBe(false) }) }) + +describe('createStoredNodeCountTracker', () => { + test('adopts a loaded graph as the baseline the guard measures against', () => { + // The hook mounts before the scene loads, so the tracker starts at the bare + // scaffold and only learns the real size when the load lands. Without this, + // every session's first save compares a populated graph against ~0 and the + // guard is dead for exactly the write that can destroy the most work. + const tracker = createStoredNodeCountTracker(0) + tracker.trackLoadedGraph(42) + + expect(tracker.allowWrite(0)).toBe(false) + expect(tracker.count).toBe(42) + }) + + test('keeps blocking after a blocked write instead of adopting the empty graph', () => { + const tracker = createStoredNodeCountTracker(0) + tracker.trackLoadedGraph(42) + + expect(tracker.allowWrite(0)).toBe(false) + // A debounced retry must not be the thing that finally lets the wipe land. + expect(tracker.allowWrite(0)).toBe(false) + }) + + test('lets ordinary edits through and advances the baseline', () => { + const tracker = createStoredNodeCountTracker(0) + tracker.trackLoadedGraph(12) + + expect(tracker.allowWrite(13)).toBe(true) + expect(tracker.count).toBe(13) + expect(tracker.allowWrite(5)).toBe(true) + expect(tracker.count).toBe(5) + }) + + test('does not treat a deliberately empty new scene as suspicious', () => { + const tracker = createStoredNodeCountTracker(0) + tracker.trackLoadedGraph(4) + + expect(tracker.allowWrite(0)).toBe(true) + }) + + test('adopts a smaller loaded graph, so restoring an older version still saves', () => { + // Loading a 3-node version over a 40-node one is not a deletion. + const tracker = createStoredNodeCountTracker(0) + tracker.trackLoadedGraph(40) + tracker.trackLoadedGraph(3) + + expect(tracker.allowWrite(3)).toBe(true) + }) +}) diff --git a/packages/editor/src/hooks/use-auto-save.ts b/packages/editor/src/hooks/use-auto-save.ts index b6e094942e..8a7909ead1 100644 --- a/packages/editor/src/hooks/use-auto-save.ts +++ b/packages/editor/src/hooks/use-auto-save.ts @@ -11,6 +11,40 @@ export function isSuspiciousNodeDrop(previousNodeCount: number, currentNodeCount return previousNodeCount > STRUCTURAL_NODE_COUNT && currentNodeCount <= STRUCTURAL_NODE_COUNT } +/** + * Tracks the node count of the graph we believe is stored, which is what the + * accidental-wipe guard measures every write against. + * + * The distinction that matters: a graph that came from storage is authoritative + * and has to become the new baseline, while an edited or previewed graph must + * not. Seeding the baseline once at mount is not enough — the hook mounts + * before the scene has loaded, so it would sit at ~0 for the whole session and + * `isSuspiciousNodeDrop` could never fire. + */ +export function createStoredNodeCountTracker(initialNodeCount: number) { + let count = initialNodeCount + + return { + get count() { + return count + }, + /** A graph read from storage — it defines what "populated" means from here. */ + trackLoadedGraph(nodeCount: number) { + count = nodeCount + }, + /** + * `false` when the write would drop a populated scene to a bare scaffold, + * which is an accidental full deletion far more often than an intent. The + * caller reports the block; on `true` the write becomes the new baseline. + */ + allowWrite(nodeCount: number) { + if (isSuspiciousNodeDrop(count, nodeCount)) return false + count = nodeCount + return true + }, + } +} + export type SaveStatus = 'idle' | 'pending' | 'saving' | 'saved' | 'paused' | 'error' interface UseAutoSaveOptions { @@ -65,7 +99,9 @@ export function useAutoSave({ // Stable subscription to scene changes useEffect(() => { let lastNodesSnapshot = JSON.stringify(useScene.getState().nodes) - let lastNodeCount = Object.keys(useScene.getState().nodes).length + const storedNodeCount = createStoredNodeCountTracker( + Object.keys(useScene.getState().nodes).length, + ) // Collections + scene materials are document-level state that persists with // the graph but lives outside `nodes`. Track them by reference (zustand // hands out a new object on every mutation) so a material edit or a @@ -90,17 +126,15 @@ export function useAutoSave({ installedPlugins, } as SceneGraph - // Guard: refuse to autosave if the scene went from populated to nearly empty. - // This catches accidental full deletions before they're persisted. const currentNodeCount = Object.keys(nodes).length - if (isSuspiciousNodeDrop(lastNodeCount, currentNodeCount)) { + const previousNodeCount = storedNodeCount.count + if (!storedNodeCount.allowWrite(currentNodeCount)) { console.warn( - `[autosave] Blocked: scene dropped from ${lastNodeCount} to ${currentNodeCount} nodes. Likely accidental deletion.`, + `[autosave] Blocked: scene dropped from ${previousNodeCount} to ${currentNodeCount} nodes. Likely accidental deletion.`, ) setSaveStatus('error') return } - lastNodeCount = currentNodeCount isSavingRef.current = true pendingSaveRef.current = false @@ -135,6 +169,7 @@ export function useAutoSave({ const unsubscribe = useScene.subscribe((state) => { if (isLoadingSceneRef.current) { lastNodesSnapshot = JSON.stringify(state.nodes) + storedNodeCount.trackLoadedGraph(Object.keys(state.nodes).length) lastCollectionsRef = state.collections lastMaterialsRef = state.materials lastInstalledPluginsRef = state.installedPlugins @@ -188,16 +223,16 @@ export function useAutoSave({ if (!hasDirtyChangesRef.current) return const { nodes, rootNodeIds, collections, materials, installedPlugins } = useScene.getState() const currentNodeCount = Object.keys(nodes).length - if (isSuspiciousNodeDrop(lastNodeCount, currentNodeCount)) { + const previousNodeCount = storedNodeCount.count + if (!storedNodeCount.allowWrite(currentNodeCount)) { console.warn( - `[autosave] Blocked unload flush: scene dropped from ${lastNodeCount} to ${currentNodeCount} nodes. Likely accidental deletion.`, + `[autosave] Blocked unload flush: scene dropped from ${previousNodeCount} to ${currentNodeCount} nodes. Likely accidental deletion.`, ) setSaveStatus('error') return } hasDirtyChangesRef.current = false - lastNodeCount = currentNodeCount const sceneGraph = { nodes, rootNodeIds,