From 572e736f1fffed60bad50bb7823606cf843d8133 Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Wed, 7 Oct 2026 10:32:15 -0700 Subject: [PATCH] fix(site-builder): Undo can no longer cross the load or a page switch Craft records actions.deserialize as an undoable step and nothing cleared history, so Undo right after load returned the canvas to the blank pre-load Frame (autosave would then save it), and Undo after a page switch put the previous page's content into the current slot. - clear history after the load-time deserialize and after every page/ header/footer switch (in-place AI replacement of the same slot keeps it) - continuousSetProp opens a burst only when its first call actually changed something, so it never merges into an entry it did not create - the empty-save banner's Undo is disabled when there is nothing to undo Real-editor tests: load, page/header switch, no-op-first burst, banner. Co-Authored-By: Claude Opus 5.5 --- craft/src/editor/LoadGate.conflict.test.tsx | 8 +- craft/src/editor/LoadGate.save-guard.test.tsx | 8 +- craft/src/editor/SaveState.test.tsx | 2 +- craft/src/hooks/useWhpApi.load.test.tsx | 2 +- .../useWhpApi.orphan-repair-wiring.test.tsx | 2 +- craft/src/hooks/useWhpApi.ts | 4 + craft/src/panels/right/styles/shared.tsx | 16 +- craft/src/panels/topbar/EmptySaveBanner.tsx | 6 +- craft/src/panels/topbar/TopBar.tsx | 1 + .../PageContext.orphan-repair-wiring.test.tsx | 2 +- .../state/PageContext.pure-updaters.test.tsx | 2 +- craft/src/state/PageContext.tsx | 15 +- .../history-baseline.integration.test.tsx | 175 ++++++++++++++++++ .../topbar-history.integration.test.tsx | 84 +++++++++ 14 files changed, 310 insertions(+), 17 deletions(-) create mode 100644 craft/src/test-utils/integration/history-baseline.integration.test.tsx create mode 100644 craft/src/test-utils/integration/topbar-history.integration.test.tsx diff --git a/craft/src/editor/LoadGate.conflict.test.tsx b/craft/src/editor/LoadGate.conflict.test.tsx index 405edfb..1b0a445 100644 --- a/craft/src/editor/LoadGate.conflict.test.tsx +++ b/craft/src/editor/LoadGate.conflict.test.tsx @@ -10,14 +10,16 @@ import { act } from 'react-dom/test-utils'; * `@craftjs/core`'s useEditor and sibling chrome are stubbed. */ const undoMock = vi.fn(); +// The banner's Undo is disabled when Craft reports nothing to undo; tests that click it set this. +const mockCanUndo = { value: false }; const deserializeMock = vi.fn(); vi.mock('@craftjs/core', () => ({ useEditor: (collector?: (state: unknown, query: unknown) => Record) => { const query = { serialize: () => '{}', - history: { canUndo: () => false, canRedo: () => false }, + history: { canUndo: () => mockCanUndo.value, canRedo: () => false }, }; - const actions = { history: { undo: undoMock, redo: vi.fn() }, deserialize: deserializeMock }; + const actions = { history: { undo: undoMock, redo: vi.fn(), clear: vi.fn() }, deserialize: deserializeMock }; const collected = collector ? collector({}, query) : {}; return { actions, query, ...collected }; }, @@ -82,6 +84,7 @@ function mount() { } beforeEach(() => { + mockCanUndo.value = false; vi.useFakeTimers(); fetchMock = vi.fn(); reloadMock.mockReset(); @@ -240,6 +243,7 @@ describe('revision and overwrite edge cases', () => { }); test('Keep mine -> empty refusal -> Undo disarms force_overwrite: the next autosave is conflict-checked', async () => { + mockCanUndo.value = true; loadedThen('r1', [ conflict409, jsonResp({ success: false, error: 'empty_overwrite_refused', message: 'e' }, 409), diff --git a/craft/src/editor/LoadGate.save-guard.test.tsx b/craft/src/editor/LoadGate.save-guard.test.tsx index 4c6d67a..576ec6f 100644 --- a/craft/src/editor/LoadGate.save-guard.test.tsx +++ b/craft/src/editor/LoadGate.save-guard.test.tsx @@ -10,14 +10,16 @@ import { act } from 'react-dom/test-utils'; * timers; only `@craftjs/core`'s useEditor and sibling chrome are stubbed. */ const undoMock = vi.fn(); +// The banner's Undo is disabled when Craft reports nothing to undo; tests that click it set this. +const mockCanUndo = { value: false }; const deserializeMock = vi.fn(); vi.mock('@craftjs/core', () => ({ useEditor: (collector?: (state: unknown, query: unknown) => Record) => { const query = { serialize: () => '{}', - history: { canUndo: () => false, canRedo: () => false }, + history: { canUndo: () => mockCanUndo.value, canRedo: () => false }, }; - const actions = { history: { undo: undoMock, redo: vi.fn() }, deserialize: deserializeMock }; + const actions = { history: { undo: undoMock, redo: vi.fn(), clear: vi.fn() }, deserialize: deserializeMock }; const collected = collector ? collector({}, query) : {}; return { actions, query, ...collected }; }, @@ -81,6 +83,7 @@ function mount() { } beforeEach(() => { + mockCanUndo.value = false; vi.useFakeTimers(); fetchMock = vi.fn(); vi.stubGlobal('fetch', fetchMock); @@ -245,6 +248,7 @@ describe('empty-overwrite guard', () => { }); test('Undo on the banner runs Craft history undo and dismisses it', async () => { + mockCanUndo.value = true; loadedThen([jsonResp({ success: false, error: 'empty_overwrite_refused', message: 'r' }, 409)]); mount(); await flush(); diff --git a/craft/src/editor/SaveState.test.tsx b/craft/src/editor/SaveState.test.tsx index 2707418..f18232e 100644 --- a/craft/src/editor/SaveState.test.tsx +++ b/craft/src/editor/SaveState.test.tsx @@ -17,7 +17,7 @@ vi.mock('@craftjs/core', () => ({ serialize: () => '{}', history: { canUndo: () => false, canRedo: () => false }, }; - const actions = { history: { undo: vi.fn(), redo: vi.fn() }, deserialize: vi.fn() }; + const actions = { history: { undo: vi.fn(), redo: vi.fn(), clear: vi.fn() }, deserialize: vi.fn() }; const collected = collector ? collector({}, query) : {}; return { actions, query, ...collected }; }, diff --git a/craft/src/hooks/useWhpApi.load.test.tsx b/craft/src/hooks/useWhpApi.load.test.tsx index e4c587a..5ac64fb 100644 --- a/craft/src/hooks/useWhpApi.load.test.tsx +++ b/craft/src/hooks/useWhpApi.load.test.tsx @@ -21,7 +21,7 @@ const deserializeMock = vi.fn(); vi.mock('@craftjs/core', () => ({ useEditor: () => ({ query: { serialize: () => '{}' }, - actions: { deserialize: deserializeMock }, + actions: { deserialize: deserializeMock, history: { clear: () => {} } }, }), })); diff --git a/craft/src/hooks/useWhpApi.orphan-repair-wiring.test.tsx b/craft/src/hooks/useWhpApi.orphan-repair-wiring.test.tsx index 5570b32..af929e1 100644 --- a/craft/src/hooks/useWhpApi.orphan-repair-wiring.test.tsx +++ b/craft/src/hooks/useWhpApi.orphan-repair-wiring.test.tsx @@ -31,7 +31,7 @@ const deserializeMock = vi.fn(); vi.mock('@craftjs/core', () => ({ useEditor: () => ({ query: { serialize: () => '{}' }, - actions: { deserialize: deserializeMock }, + actions: { deserialize: deserializeMock, history: { clear: () => {} } }, }), })); diff --git a/craft/src/hooks/useWhpApi.ts b/craft/src/hooks/useWhpApi.ts index 9d27e64..9b4f6f4 100644 --- a/craft/src/hooks/useWhpApi.ts +++ b/craft/src/hooks/useWhpApi.ts @@ -514,6 +514,10 @@ export function useWhpApi() { ); } actions.deserialize(state); + // Craft records deserialize as an undoable step; left in, Undo + // right after load returns the canvas to the pre-load blank Frame + // (and autosave would then save that). History starts here. + actions.history.clear(); } catch (e) { // Never fall back to a blank canvas: autosave would then replace // the customer's real (if unreadable) page with it. Fail the load. diff --git a/craft/src/panels/right/styles/shared.tsx b/craft/src/panels/right/styles/shared.tsx index e8e034f..2ea2fec 100644 --- a/craft/src/panels/right/styles/shared.tsx +++ b/craft/src/panels/right/styles/shared.tsx @@ -344,8 +344,20 @@ export function continuousSetProp(actions: any, nodeId: string, controlKey: stri const continuing = openBurst?.key === key; if (!continuing) closeContinuousBurst(); else clearTimeout(openBurst!.timer); - (continuing ? actions.history.merge() : actions).setProp(nodeId, updater); - openBurst = { key, timer: setTimeout(closeContinuousBurst, CONTINUOUS_EDIT_IDLE_MS) }; + // Craft records no history entry for a change that changes nothing. A burst + // must only be opened by a call that recorded its OWN entry: otherwise the + // next change would merge() into whatever entry came before (another edit, + // or the page load -- and Undo would then blank the canvas). + let changed = false; + const tracked = (p: any) => { + const before = JSON.stringify(p); + updater(p); + changed = JSON.stringify(p) !== before; + }; + (continuing ? actions.history.merge() : actions).setProp(nodeId, tracked); + if (continuing || changed) { + openBurst = { key, timer: setTimeout(closeContinuousBurst, CONTINUOUS_EDIT_IDLE_MS) }; + } } /** Plain setProp for a discrete edit: always its own undo step. */ diff --git a/craft/src/panels/topbar/EmptySaveBanner.tsx b/craft/src/panels/topbar/EmptySaveBanner.tsx index b87a973..b4ae520 100644 --- a/craft/src/panels/topbar/EmptySaveBanner.tsx +++ b/craft/src/panels/topbar/EmptySaveBanner.tsx @@ -3,15 +3,17 @@ import React from 'react'; export interface EmptySaveBannerProps { onSaveAnyway: () => void; onUndo: () => void; + /** Nothing to undo (e.g. the empty page came straight from load): Undo is disabled, not a dead button. */ + undoDisabled?: boolean; } /** Persistent banner shown when the server refused a save that would replace * the saved site with an empty page. Autosave stays paused while it's up. */ -export const EmptySaveBanner: React.FC = ({ onSaveAnyway, onUndo }) => ( +export const EmptySaveBanner: React.FC = ({ onSaveAnyway, onUndo, undoDisabled }) => (