fix(site-builder): serialise all saves; honest pill for conflicts and new sites

A manual Save during Publish sent a second save with the same base_rev; the
loser 409'd against its own tab (and could abort the publish).

- useWhpApi: every save runs through one in-flight promise (SaveGuard
  saveInFlight). A save requested while one is out waits, then sends only
  if still dirty, with the rev the first returned; payload is read from the
  editor at send time. Forced (empty/overwrite) saves always send.
- Save buttons are disabled while publishing.
- Pill: a conflict reads 'Unsaved changes' (modal open) from Save and from
  Publish alike; a never-saved site (load rev null) reads 'Not saved yet'.
- Autosave interval reads runSave through a ref, so editing can't keep
  restarting it and starve autosave.
- Fix stale 'Throws like exportBodyHtml' comment.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
shadowdaoandClaude Opus 5.5 committed 2026-10-07 10:35:11 -07:00
1 parent 572e736f1f
commit 97ccbee461
7 files changed
+335 -20

No files matched your search

+237
View File
@@ -0,0 +1,237 @@
import { describe, test, expect, vi, beforeEach, afterEach } from 'vitest';
import React from 'react';
import { createRoot, Root } from 'react-dom/client';
import { act } from 'react-dom/test-utils';
/**
* One save at a time. Every save (Save button, autosave, the save inside
* Publish) goes through a single in-flight promise in useWhpApi, so a second
* save can never race the first with the same base_rev and 409 against its
* own tab. Also: the pill for conflicts and brand-new sites, and an autosave
* timer that continuous editing cannot starve.
*
* The fake server checks base_rev when it PROCESSES a save (in arrival
* order), like the real one: a save based on a stale rev gets a 409.
*/
vi.mock('@craftjs/core', () => ({
useEditor: (collector?: (state: unknown, query: unknown) => Record<string, unknown>) => {
const query = { serialize: () => '{}', history: { canUndo: () => false, canRedo: () => false } };
const actions = { history: { undo: vi.fn(), redo: vi.fn(), clear: vi.fn() }, deserialize: vi.fn() };
return { actions, query, ...(collector ? collector({}, query) : {}) };
},
}));
vi.mock('../panels/topbar/TemplateModal', () => ({ TemplateModal: () => null }));
vi.mock('../panels/topbar/HeadCodeModal', () => ({ HeadCodeModal: () => null }));
vi.mock('../panels/sitesmith/SitesmithButton', () => ({ SitesmithButton: () => null }));
import { LoadGate } from './LoadGate';
import { DirtyTracker } from './DirtyTracker';
import { TopBar } from '../panels/topbar/TopBar';
import { EditorConfigProvider } from '../state/EditorConfigContext';
import { SaveGuardProvider, useSaveGuard } from '../state/SaveGuardContext';
import { PageProvider } from '../state/PageContext';
import { SiteDesignProvider, useSiteDesign } from '../state/SiteDesignContext';
import { useWhpApi } from '../hooks/useWhpApi';
import { WhpConfig } from '../types';
const whpConfig: WhpConfig = {
user: 'u', apiUrl: '/api', csrfToken: 't', siteId: 1,
siteDomain: 'example.com', siteName: 'Example', backUrl: '/', isRoot: false,
};
const jsonResp = (body: unknown, status = 200) => ({ ok: status >= 200 && status < 300, status, json: async () => body });
let container: HTMLDivElement;
let root: Root;
let fetchMock: ReturnType<typeof vi.fn>;
let guard: ReturnType<typeof useSaveGuard>;
let design: ReturnType<typeof useSiteDesign>;
let api: ReturnType<typeof useWhpApi>;
const Probe: React.FC = () => {
guard = useSaveGuard();
design = useSiteDesign();
api = useWhpApi();
return null;
};
const q = (sel: string) => container.querySelector(sel) as HTMLButtonElement | null;
const pill = () => q('[data-testid="save-status-pill"]')!.getAttribute('data-status');
const flush = () => act(async () => { for (let i = 0; i < 10; i++) await Promise.resolve(); });
const calls = (action: string) => fetchMock.mock.calls.filter(([u]) => String(u).includes(`action=${action}`));
const saveBody = (i: number) => JSON.parse(calls('save')[i][1].body);
/** Fake server. Saves wait in `pending` until served, then are checked against the server's rev. */
let serverRev: string | null;
let revN: number;
let pending: { body: any; resolve: (r: unknown) => void }[];
let statuses: number[];
let holdSaves: boolean;
function serveSave(p: { body: any; resolve: (r: unknown) => void }) {
if (p.body.base_rev !== serverRev && !p.body.force_overwrite) {
statuses.push(409);
p.resolve(jsonResp({ success: false, error: 'conflict', message: 'other tab' }, 409));
return;
}
revN += 1;
serverRev = `r${revN}`;
statuses.push(200);
p.resolve(jsonResp({ success: true, rev: serverRev }));
}
async function serveNext() {
await act(async () => { serveSave(pending.shift()!); });
await flush();
}
function server(loadRev: string | null) {
serverRev = loadRev;
revN = 1;
pending = [];
statuses = [];
fetchMock.mockImplementation((url: string, init?: { body?: string }) => {
const u = String(url);
if (u.includes('action=load')) return Promise.resolve(jsonResp({ success: true, project: null, rev: loadRev }));
if (u.includes('action=publish')) return Promise.resolve(jsonResp({ success: true }));
const body = JSON.parse(init!.body!);
return new Promise((resolve) => {
const p = { body, resolve };
if (holdSaves) pending.push(p); else serveSave(p);
});
});
}
async function mount() {
container = document.createElement('div');
document.body.appendChild(container);
act(() => {
root = createRoot(container);
root.render(
<EditorConfigProvider config={whpConfig}>
<SaveGuardProvider>
<SiteDesignProvider>
<PageProvider>
<LoadGate />
<DirtyTracker />
<Probe />
<TopBar device="desktop" onDeviceChange={() => {}} showGuides onToggleGuides={() => {}} />
</PageProvider>
</SiteDesignProvider>
</SaveGuardProvider>
</EditorConfigProvider>,
);
});
await flush();
}
beforeEach(() => {
vi.useFakeTimers();
fetchMock = vi.fn();
holdSaves = false;
vi.stubGlobal('fetch', fetchMock);
vi.spyOn(console, 'error').mockImplementation(() => {});
});
afterEach(() => {
act(() => { root.unmount(); });
container.remove();
vi.useRealTimers();
vi.unstubAllGlobals();
vi.restoreAllMocks();
});
const edit = () => act(() => { guard.markDirty(); });
const clickPublish = async () => { await act(async () => { q('button[aria-label="Publish to live site"]')!.click(); }); await flush(); };
describe('saves are serialised', () => {
test('a save requested during an in-flight publish waits, then saves with the rev the first save returned', async () => {
server('r1');
await mount();
holdSaves = true;
edit();
await clickPublish();
expect(calls('save')).toHaveLength(1); // the publish's own save, still in flight
edit(); // more typing while it is out...
let second!: Promise<unknown>;
await act(async () => { second = api.save(); }); // ...and a save request (any path)
await flush();
expect(calls('save')).toHaveLength(1); // queued, not sent alongside
await serveNext(); // first save lands: r2
expect(calls('save')).toHaveLength(2);
expect(saveBody(0).base_rev).toBe('r1');
expect(saveBody(1).base_rev).toBe('r2');
await serveNext();
await act(async () => { await second; });
expect(statuses).toEqual([200, 200]);
expect(q('[data-testid="conflict-modal"]')).toBeNull();
expect(calls('publish')).toHaveLength(1);
});
test('a queued save is skipped when the save it waited for left nothing unsaved', async () => {
server('r1');
await mount();
holdSaves = true;
edit();
await clickPublish();
let second!: Promise<any>;
await act(async () => { second = api.save(); });
await serveNext();
while (pending.length) await serveNext(); // never hang: serve anything else that went out
let result: any;
await act(async () => { result = await second; });
expect(calls('save')).toHaveLength(1);
expect(result.success).toBe(true);
});
test('the Save button is disabled while a publish is running', async () => {
server('r1');
await mount();
holdSaves = true;
edit();
await clickPublish();
expect(q('button[aria-label="Save Draft"]')!.disabled).toBe(true);
await serveNext();
expect(q('button[aria-label="Save Draft"]')!.disabled).toBe(false);
});
});
describe('status pill', () => {
test('a conflict shows "Unsaved changes" whether it came from Save or from Publish', async () => {
server('r1');
await mount();
serverRev = 'r-other'; // another tab saved
edit();
await act(async () => { q('button[aria-label="Save Draft"]')!.click(); });
await flush();
expect(q('[data-testid="conflict-modal"]')).not.toBeNull();
const fromSave = pill();
act(() => { guard.setConflict(null); });
await clickPublish();
expect(q('[data-testid="conflict-modal"]')).not.toBeNull();
expect(fromSave).toBe('unsaved');
expect(pill()).toBe('unsaved');
});
test('a brand-new site that was never saved says "Not saved yet", not "Saved"', async () => {
server(null);
await mount();
expect(pill()).toBe('new');
expect(q('[data-testid="save-status-pill"]')!.textContent).toContain('Not saved yet');
edit();
await act(async () => { q('button[aria-label="Save Draft"]')!.click(); });
await flush();
expect(pill()).toBe('saved');
});
});
describe('autosave timer', () => {
test('continuous design edits do not keep restarting the 30s autosave interval', async () => {
server('r1');
await mount();
for (let i = 1; i <= 7; i++) {
act(() => { design.updateDesign({ headCode: `<!-- ${i} -->` }); });
await act(async () => { vi.advanceTimersByTime(10_000); });
}
await flush();
expect(calls('save').length).toBeGreaterThanOrEqual(2);
});
});
+2 -1
View File
@@ -133,7 +133,8 @@ describe('dirty lifecycle and status pill', () => {
respondWith({ save: [jsonResp({ success: false, error: 'x', message: 'nope' }, 500), jsonResp({ success: true })] });
await mount();
expect(guard.dirty).toBe(false);
expect(pill()).toBe('saved');
// rev: null -- the server has no copy yet, so not "Saved".
expect(pill()).toBe('new');
edit();
expect(guard.dirty).toBe(true);
+51 -8
View File
@@ -1,4 +1,4 @@
import { useCallback } from 'react';
import { useCallback, useRef } from 'react';
import { useEditor } from '@craftjs/core';
import { useEditorConfig } from '../state/EditorConfigContext';
import { usePages } from '../state/PageContext';
@@ -74,7 +74,8 @@ export function buildSavePayload(input: BuildSavePayloadInput) {
// Stored states touched by this build; the cache is pruned to this set.
const usedStates = new Set<string>();
/** Export a stored (not live) state, reusing the memo. Throws like exportBodyHtml. */
/** Export a stored (not live) state, reusing the memo. Never throws for a bad state:
* exportBodyHtml swallows its own errors and returns empty html (see below). */
const exportStored = (state: string): ExportResult => {
usedStates.add(state);
const hit = exportCache.get(state);
@@ -301,14 +302,18 @@ export function useWhpApi() {
const {
loadState, forceEmptyNextSave, beginSave, endSave,
getRev, setRev, setConflict, forceOverwriteNextSave,
saveInFlight, isDirty,
} = useSaveGuard();
const save = useCallback(async (opts?: SaveOptions): Promise<ApiResult | null> => {
if (!isWHP || !whpConfig) return null;
// Never save before a successful load: the canvas may be blank only
// because the load failed, and saving it would destroy the real site.
if (loadState !== 'loaded') return null;
// A save queued behind another runs later: it must build its payload from
// the editor as it is THEN, not from the render that queued it.
const latest = useRef({ pages, headerPage, footerPage, activePageId, isEditingHeader, isEditingFooter, design });
latest.current = { pages, headerPage, footerPage, activePageId, isEditingHeader, isEditingFooter, design };
/** One save request, built from the current editor state. Only ever called through `save`. */
const sendSave = useCallback(async (opts?: SaveOptions): Promise<ApiResult | null> => {
if (!whpConfig) return null;
const { pages, headerPage, footerPage, activePageId, isEditingHeader, isEditingFooter, design } = latest.current;
// The ticket pins "what changed since" to this moment: edits made while
// the request is in flight keep the editor dirty after it succeeds.
const ticket = beginSave();
@@ -382,7 +387,45 @@ export function useWhpApi() {
} finally {
endSave(ticket, succeeded);
}
}, [isWHP, whpConfig, loadState, forceEmptyNextSave, forceOverwriteNextSave, getRev, setRev, setConflict, beginSave, endSave, query, pages, activePageId, headerPage, footerPage, isEditingHeader, isEditingFooter, design]);
}, [whpConfig, forceEmptyNextSave, forceOverwriteNextSave, getRev, setRev, setConflict, beginSave, endSave, query]);
/**
* Save, one at a time. Every save (Save button, autosave, Keep mine, the
* save inside publish) runs through `saveInFlight`: a save requested while
* another is out waits for it, then sends only if there is still something
* to save, with the rev that save returned. Two concurrent saves would
* otherwise carry the same base_rev and the second would 409 against its
* own tab.
*
* After waiting, the earlier result is reused (nothing sent) when it went
* through and nothing changed since, or when it hit a conflict (the modal
* owns that; resending would only 409 again). A forceEmpty / forceOverwrite
* save (passed in or armed one-shot) always sends.
*/
const save = useCallback((opts?: SaveOptions): Promise<ApiResult | null> => {
if (!isWHP || !whpConfig) return Promise.resolve(null);
// Never save before a successful load: the canvas may be blank only
// because the load failed, and saving it would destroy the real site.
if (loadState !== 'loaded') return Promise.resolve(null);
const prev = saveInFlight.current as Promise<ApiResult | null> | null;
const run = (async (): Promise<ApiResult | null> => {
if (prev) {
const prior = await prev.catch(() => null);
const forced = !!opts?.forceEmpty || !!opts?.forceOverwrite
|| forceEmptyNextSave.current || forceOverwriteNextSave.current;
if (prior && !forced) {
if (prior.success && !isDirty()) return prior;
if (prior.error === 'conflict') return prior;
}
}
return sendSave(opts);
})();
saveInFlight.current = run;
const release = () => { if (saveInFlight.current === run) saveInFlight.current = null; };
run.then(release, release);
return run;
}, [isWHP, whpConfig, loadState, saveInFlight, isDirty, sendSave, forceEmptyNextSave, forceOverwriteNextSave]);
const publish = useCallback(async (): Promise<ApiResult | null> => {
if (!isWHP || !whpConfig) return null;
+4 -1
View File
@@ -1,6 +1,7 @@
import React from 'react';
export type SaveStatus = 'unsaved' | 'saving' | 'saved' | 'published' | 'failed';
/** 'new': the server has no copy yet (brand-new site, never saved). */
export type SaveStatus = 'unsaved' | 'saving' | 'saved' | 'published' | 'failed' | 'new';
const LABELS: Record<SaveStatus, string> = {
unsaved: 'Unsaved changes',
@@ -8,6 +9,7 @@ const LABELS: Record<SaveStatus, string> = {
saved: 'Saved',
published: 'Published',
failed: 'Save failed',
new: 'Not saved yet',
};
const ICONS: Record<SaveStatus, string> = {
@@ -16,6 +18,7 @@ const ICONS: Record<SaveStatus, string> = {
saved: 'fa-check',
published: 'fa-check-circle',
failed: 'fa-exclamation-triangle',
new: 'fa-circle-o',
};
/** Persistent save state. It changes only when the state does; nothing resets it on a timer. */
+22 -8
View File
@@ -38,6 +38,7 @@ export const TopBar: React.FC<TopBarProps> = ({ device, onDeviceChange, showGuid
const {
loadState, emptyRefusal, setEmptyRefusal, dirty, isDirty, getVersion,
conflict, setConflict, armForceOverwriteNextSave, disarmForceOverwriteNextSave, reloadLatest,
neverSaved,
} = useSaveGuard();
const canSave = loadState === 'loaded';
const { headerPage, footerPage, pages, activePageId } = usePages();
@@ -68,10 +69,17 @@ export const TopBar: React.FC<TopBarProps> = ({ device, onDeviceChange, showGuid
if (dirty) setPublished(false);
}, [dirty]);
// A conflict reads "Unsaved changes" (with the conflict modal open) whether
// it came from Save, autosave or the save inside Publish: the edits are
// intact locally, just not on the server. "Not saved yet": the server has
// no copy of this site and nothing has been saved since load (an older
// backend that reports no rev falls back to "Saved").
const status: SaveStatus = saving || publishing ? 'saving'
: conflict ? 'unsaved'
: saveFailed ? 'failed'
: dirty ? 'unsaved'
: published ? 'published'
: neverSaved ? 'new'
: 'saved';
const messageOf = (e: unknown, what: string) =>
@@ -112,18 +120,24 @@ export const TopBar: React.FC<TopBarProps> = ({ device, onDeviceChange, showGuid
}
}, [save, canSave, isDirty, setEmptyRefusal, setConflict]);
// Read at tick time: runSave's identity changes with editor state, and
// restarting the interval on every change would starve autosave during
// continuous editing.
const runSaveRef = useRef(runSave);
runSaveRef.current = runSave;
// Auto-save every 30 seconds
useEffect(() => {
// Paused until the site has loaded, and while the empty-save banner is up.
if (!isWHP || !canSave || emptyRefusal || conflict) return;
// A running publish does its own save; a second one would race it.
// Read through refs/isDirty() at tick time, never a stale closure.
// A running publish does its own save; a second one would only queue
// behind it. Read through refs/isDirty() at tick time, never a stale closure.
// Also paused while the conflict modal is up: it is the authority, the
// effect guard above only avoids a pointless timer.
const autosaveDue = () => isDirty() && !publishingRef.current && !conflictRef.current;
const interval = setInterval(() => { if (autosaveDue()) void runSave(); }, 30000);
const interval = setInterval(() => { if (autosaveDue()) void runSaveRef.current(); }, 30000);
return () => clearInterval(interval);
}, [isWHP, canSave, emptyRefusal, conflict, runSave, isDirty]);
}, [isWHP, canSave, emptyRefusal, conflict, isDirty]);
const handleSave = runSave;
@@ -137,8 +151,8 @@ export const TopBar: React.FC<TopBarProps> = ({ device, onDeviceChange, showGuid
try {
const result = await publish();
if (result?.error === 'conflict') {
// Nothing was published: the conflict modal (opened by save()) takes over.
setSaveFailed(true);
// Nothing was published: the conflict modal (opened by save()) takes
// over, and the pill reads "Unsaved changes" exactly as for a Save.
} else if (result?.error === 'empty_overwrite_refused') {
setEmptyRefusal(result.message ?? null);
} else if (result?.success) {
@@ -330,7 +344,7 @@ export const TopBar: React.FC<TopBarProps> = ({ device, onDeviceChange, showGuid
<button
className="topbar-btn icon-only primary"
onClick={() => { void handleSave(); }}
disabled={saving || !canSave}
disabled={saving || publishing || !canSave}
aria-label="Save Draft"
data-tooltip="Save"
>
@@ -473,7 +487,7 @@ export const TopBar: React.FC<TopBarProps> = ({ device, onDeviceChange, showGuid
<button
className="topbar-btn primary"
onClick={() => { void handleSave(); }}
disabled={saving || !canSave}
disabled={saving || publishing || !canSave}
title="Save Draft"
aria-label="Save Draft"
>
+18 -2
View File
@@ -46,6 +46,12 @@ interface SaveGuardContextValue {
disarmForceOverwriteNextSave: () => void;
/** "Reload latest": skip the unsaved-changes prompt, then reload the whole editor. */
reloadLatest: () => void;
/** The one save request in flight (shared by every useWhpApi instance), or null.
* useWhpApi chains each new save behind it so two saves never race on one base_rev. */
saveInFlight: React.MutableRefObject<Promise<unknown> | null>;
/** True while the server has no copy of this site (load reported rev null) and no save
* has gone through since. Drives the "Not saved yet" pill. */
neverSaved: boolean;
}
/** Identifies the editor state a save was built from. */
@@ -78,6 +84,8 @@ const defaultValue: SaveGuardContextValue = {
armForceOverwriteNextSave: () => {},
disarmForceOverwriteNextSave: () => {},
reloadLatest: () => {},
saveInFlight: { current: null },
neverSaved: false,
};
const SaveGuardContext = createContext<SaveGuardContextValue>(defaultValue);
@@ -92,8 +100,13 @@ export const SaveGuardProvider: React.FC<{ children: ReactNode; /** Injectable f
const [conflict, setConflict] = useState<string | null>(null);
const forceOverwriteNextSave = useRef(false);
const revRef = useRef<string | null | undefined>(undefined);
const [neverSaved, setNeverSaved] = useState(false);
const saveInFlight = useRef<Promise<unknown> | null>(null);
const getRev = useCallback(() => revRef.current, []);
const setRev = useCallback((rev: string | null) => { revRef.current = rev; }, []);
const setRev = useCallback((rev: string | null) => {
revRef.current = rev;
setNeverSaved(rev === null);
}, []);
// Dirty tracking. `versionRef` counts changes so a save that raced with an
// edit cannot mark that edit clean.
@@ -127,6 +140,7 @@ export const SaveGuardProvider: React.FC<{ children: ReactNode; /** Injectable f
}, []);
const endSave = useCallback((ticket: SaveTicket, succeeded: boolean) => {
setSavesInFlight((n) => Math.max(0, n - 1));
if (succeeded) setNeverSaved(false);
if (succeeded && ticket.version === versionRef.current) {
dirtyRef.current = false;
setDirty(false);
@@ -175,7 +189,9 @@ export const SaveGuardProvider: React.FC<{ children: ReactNode; /** Injectable f
armForceOverwriteNextSave: () => { forceOverwriteNextSave.current = true; },
disarmForceOverwriteNextSave: () => { forceOverwriteNextSave.current = false; },
reloadLatest,
}), [loadState, emptyRefusal, dirty, saving, isDirty, getVersion, markDirty, beginSave, endSave, suppressBeforeUnload, getRev, setRev, conflict, reloadLatest]);
saveInFlight,
neverSaved,
}), [loadState, emptyRefusal, dirty, saving, isDirty, getVersion, markDirty, beginSave, endSave, suppressBeforeUnload, getRev, setRev, conflict, reloadLatest, neverSaved]);
return <SaveGuardContext.Provider value={value}>{children}</SaveGuardContext.Provider>;
};
+1
View File
@@ -1992,6 +1992,7 @@ body {
.save-pill.saved { background: rgba(16, 185, 129, 0.12); color: #10b981; border-color: rgba(16, 185, 129, 0.25); }
.save-pill.published { background: rgba(16, 185, 129, 0.2); color: #34d399; border-color: rgba(16, 185, 129, 0.4); }
.save-pill.failed { background: rgba(239, 68, 68, 0.15); color: #f87171; border-color: rgba(239, 68, 68, 0.4); }
.save-pill.new { background: rgba(161, 161, 170, 0.12); color: #a1a1aa; border-color: rgba(161, 161, 170, 0.3); }
.topbar-mobile .save-pill { padding: 3px 6px; font-size: 10px; }
/* Error toast: fixed, stays until dismissed */