diff --git a/craft/src/editor/SaveSerialise.test.tsx b/craft/src/editor/SaveSerialise.test.tsx new file mode 100644 index 0000000..79ad740 --- /dev/null +++ b/craft/src/editor/SaveSerialise.test.tsx @@ -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) => { + 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; +let guard: ReturnType; +let design: ReturnType; +let api: ReturnType; + +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( + + + + + + + + {}} showGuides onToggleGuides={() => {}} /> + + + + , + ); + }); + 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; + 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; + 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: `` }); }); + await act(async () => { vi.advanceTimersByTime(10_000); }); + } + await flush(); + expect(calls('save').length).toBeGreaterThanOrEqual(2); + }); +}); diff --git a/craft/src/editor/SaveState.test.tsx b/craft/src/editor/SaveState.test.tsx index f18232e..e8d0eb4 100644 --- a/craft/src/editor/SaveState.test.tsx +++ b/craft/src/editor/SaveState.test.tsx @@ -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); diff --git a/craft/src/hooks/useWhpApi.ts b/craft/src/hooks/useWhpApi.ts index 9b4f6f4..97cd5be 100644 --- a/craft/src/hooks/useWhpApi.ts +++ b/craft/src/hooks/useWhpApi.ts @@ -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(); - /** 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 => { - 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 => { + 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 => { + 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 | null; + const run = (async (): Promise => { + 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 => { if (!isWHP || !whpConfig) return null; diff --git a/craft/src/panels/topbar/SaveStatusPill.tsx b/craft/src/panels/topbar/SaveStatusPill.tsx index 67d8704..9fe0c27 100644 --- a/craft/src/panels/topbar/SaveStatusPill.tsx +++ b/craft/src/panels/topbar/SaveStatusPill.tsx @@ -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 = { unsaved: 'Unsaved changes', @@ -8,6 +9,7 @@ const LABELS: Record = { saved: 'Saved', published: 'Published', failed: 'Save failed', + new: 'Not saved yet', }; const ICONS: Record = { @@ -16,6 +18,7 @@ const ICONS: Record = { 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. */ diff --git a/craft/src/panels/topbar/TopBar.tsx b/craft/src/panels/topbar/TopBar.tsx index ff5be8e..e080cb4 100644 --- a/craft/src/panels/topbar/TopBar.tsx +++ b/craft/src/panels/topbar/TopBar.tsx @@ -38,6 +38,7 @@ export const TopBar: React.FC = ({ 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 = ({ 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 = ({ 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 = ({ 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 = ({ device, onDeviceChange, showGuid