From 7d25bf181ca7cd31d3cf915170091d7bdaa0eb63 Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Wed, 7 Oct 2026 12:51:48 -0700 Subject: [PATCH] fix(api): time out hung save/publish/load; show PHP error text - save, publish and load run under a 30s AbortController timeout (raced, so a stalled body read can't hang either); save/publish resolve to { success:false, error:'timeout', message } and load rejects with a readable Error, so a hung request no longer blocks the save queue - readApiResult falls back to obj.error when obj.message is absent, so PHP texts like 'Another save of this site is still in progress...' are shown instead of 'server returned HTTP 503' Refs #4 Refs #47 Co-Authored-By: Claude Opus 5.5 --- craft/src/hooks/useWhpApi.timeout.test.tsx | 167 +++++++++++++++++++++ craft/src/hooks/useWhpApi.ts | 167 ++++++++++++++------- 2 files changed, 279 insertions(+), 55 deletions(-) create mode 100644 craft/src/hooks/useWhpApi.timeout.test.tsx diff --git a/craft/src/hooks/useWhpApi.timeout.test.tsx b/craft/src/hooks/useWhpApi.timeout.test.tsx new file mode 100644 index 0000000..563640b --- /dev/null +++ b/craft/src/hooks/useWhpApi.timeout.test.tsx @@ -0,0 +1,167 @@ +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'; + +/** + * Integration follow-ups #4/#47 (items 11 + 12): + * - a save / publish / load request that never completes is abandoned after + * API_TIMEOUT_MS with a readable error, so it cannot block the save queue; + * - a failed response whose JSON only has `error` (PHP's readable text) + * shows that text instead of "server returned HTTP 503". + */ +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) : {}) }; + }, +})); + +import { LoadGate } from '../editor/LoadGate'; +import { EditorConfigProvider } from '../state/EditorConfigContext'; +import { SaveGuardProvider, useSaveGuard } from '../state/SaveGuardContext'; +import { PageProvider } from '../state/PageContext'; +import { SiteDesignProvider } from '../state/SiteDesignContext'; +import { useWhpApi, API_TIMEOUT_MS } from './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 }); +const never = () => new Promise(() => { /* the server never answers */ }); + +let container: HTMLDivElement; +let root: Root; +let fetchMock: ReturnType; +let api: ReturnType; +let guard: ReturnType; +const Probe: React.FC = () => { api = useWhpApi(); guard = useSaveGuard(); return null; }; +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}`)); + +async function mount(withLoadGate = true) { + container = document.createElement('div'); + document.body.appendChild(container); + act(() => { + root = createRoot(container); + root.render( + + + {withLoadGate && } + + + , + ); + }); + await flush(); +} + +beforeEach(() => { + vi.useFakeTimers(); + fetchMock = vi.fn(); + vi.stubGlobal('fetch', fetchMock); + vi.spyOn(console, 'error').mockImplementation(() => {}); +}); +afterEach(() => { + act(() => { root.unmount(); }); + container.remove(); + vi.useRealTimers(); + vi.unstubAllGlobals(); + vi.restoreAllMocks(); +}); + +describe('request timeout (item 11)', () => { + test('a hung save resolves with a readable timeout error and releases the save queue', async () => { + let saveN = 0; + fetchMock.mockImplementation((url: string) => { + if (String(url).includes('action=load')) return Promise.resolve(jsonResp({ success: true, project: null, rev: 'r1' })); + saveN += 1; + return saveN === 1 ? never() : Promise.resolve(jsonResp({ success: true, rev: 'r2' })); + }); + await mount(); + act(() => { guard.markDirty(); }); + + let first: any = 'pending'; + let second: any = 'pending'; + await act(async () => { void api.save().then((r) => { first = r; }); }); + await act(async () => { void api.save().then((r) => { second = r; }); }); + await flush(); + expect(calls('save')).toHaveLength(1); // the second waits behind the hung one + const signal: AbortSignal = calls('save')[0][1].signal; + expect(signal.aborted).toBe(false); + + await act(async () => { vi.advanceTimersByTime(API_TIMEOUT_MS - 1); }); + await flush(); + expect(first).toBe('pending'); + + await act(async () => { vi.advanceTimersByTime(1); }); + await flush(); + expect(signal.aborted).toBe(true); + expect(first).toMatchObject({ success: false, error: 'timeout' }); + expect(first.message).toMatch(/Save failed: the server did not respond within 30 seconds/); + // Queue released: the waiting save went out and succeeded. + expect(calls('save')).toHaveLength(2); + expect(second).toMatchObject({ success: true, rev: 'r2' }); + }); + + test('a hung publish times out with a readable error', async () => { + fetchMock.mockImplementation((url: string) => { + const u = String(url); + if (u.includes('action=load')) return Promise.resolve(jsonResp({ success: true, project: null, rev: 'r1' })); + if (u.includes('action=save')) return Promise.resolve(jsonResp({ success: true, rev: 'r2' })); + return never(); + }); + await mount(); + let res: any = 'pending'; + await act(async () => { void api.publish().then((r) => { res = r; }); }); + await flush(); + expect(calls('publish')).toHaveLength(1); + await act(async () => { vi.advanceTimersByTime(API_TIMEOUT_MS); }); + await flush(); + expect(res).toMatchObject({ success: false, error: 'timeout' }); + expect(res.message).toMatch(/^Publish failed: the server did not respond/); + }); + + test('a hung load rejects with a readable error', async () => { + fetchMock.mockImplementation(() => never()); + await mount(false); + let err: any = null; + await act(async () => { void api.load().catch((e) => { err = e; }); }); + await act(async () => { vi.advanceTimersByTime(API_TIMEOUT_MS); }); + await flush(); + expect(err).toBeInstanceOf(Error); + expect(err.message).toMatch(/^Load failed: the server did not respond within 30 seconds/); + }); +}); + +describe('readApiResult error text (item 12)', () => { + test('a 503 whose JSON has only `error` shows that text', async () => { + const text = 'Another save of this site is still in progress. Please try again in a moment.'; + fetchMock.mockImplementation((url: string) => { + if (String(url).includes('action=load')) return Promise.resolve(jsonResp({ success: true, project: null, rev: 'r1' })); + return Promise.resolve(jsonResp({ success: false, error: text }, 503)); + }); + await mount(); + let res: any = null; + await act(async () => { res = await api.save(); }); + expect(res).toMatchObject({ success: false, message: text }); + }); + + test('`message` still wins when present; neither -> the HTTP status text', async () => { + let body: any = { success: false, error: 'code', message: 'Readable' }; + fetchMock.mockImplementation((url: string) => { + if (String(url).includes('action=load')) return Promise.resolve(jsonResp({ success: true, project: null, rev: 'r1' })); + return Promise.resolve(jsonResp(body, 500)); + }); + await mount(); + let res: any = null; + await act(async () => { res = await api.save(); }); + expect(res.message).toBe('Readable'); + body = { success: false }; + await act(async () => { res = await api.save(); }); + expect(res.message).toBe('Save failed: server returned HTTP 500'); + }); +}); diff --git a/craft/src/hooks/useWhpApi.ts b/craft/src/hooks/useWhpApi.ts index 3994032..75071e3 100644 --- a/craft/src/hooks/useWhpApi.ts +++ b/craft/src/hooks/useWhpApi.ts @@ -263,6 +263,38 @@ export interface ApiResult { [key: string]: unknown; } +/** How long save / publish / load may take before the request is abandoned. */ +export const API_TIMEOUT_MS = 30_000; + +/** A request that got no complete answer within API_TIMEOUT_MS. */ +export class ApiTimeoutError extends Error {} + +/** + * Runs `fn` (a fetch plus reading its body) with an AbortSignal that fires + * after `ms`. Racing the timeout as well means a stalled body read can't hang + * the caller either: without this a request that never completes holds the + * save queue forever. + */ +async function withTimeout(action: string, fn: (signal: AbortSignal) => Promise, ms = API_TIMEOUT_MS): Promise { + const ctrl = new AbortController(); + let timer: ReturnType | undefined; + const timeout = new Promise((_, reject) => { + timer = setTimeout(() => { + ctrl.abort(); + reject(new ApiTimeoutError( + `${action} failed: the server did not respond within ${Math.round(ms / 1000)} seconds. Check your connection and try again.`, + )); + }, ms); + }); + try { + return await Promise.race([fn(ctrl.signal), timeout]); + } finally { + clearTimeout(timer); + } +} + +const timeoutResult = (e: ApiTimeoutError): ApiResult => ({ success: false, error: 'timeout', message: e.message }); + /** * Reads a save/publish response without ever throwing on the body: a non-2xx * status or a non-JSON body (a 502 HTML page, a PHP fatal) becomes a @@ -278,7 +310,12 @@ async function readApiResult(resp: Response, action: 'Save' | 'Publish'): Promis const obj = body && typeof body === 'object' ? (body as ApiResult) : null; if (resp.ok && obj) return obj; if (obj) { - return { ...obj, success: false, message: obj.message || `${action} failed: server returned HTTP ${resp.status}` }; + // PHP handlers often put their readable text in `error` only ("Another + // save of this site is still in progress..."): prefer it over the status. + const text = (typeof obj.message === 'string' && obj.message.trim()) + || (typeof obj.error === 'string' && obj.error.trim()) + || `${action} failed: server returned HTTP ${resp.status}`; + return { ...obj, success: false, message: text }; } return { success: false, @@ -357,40 +394,49 @@ export function useWhpApi() { extras: projectExtras.all(), }); - const resp = await fetch(`${whpConfig.apiUrl}?action=save`, { - method: 'POST', - headers: { - 'Content-Type': 'application/json', - 'X-CSRF-Token': whpConfig.csrfToken, - }, - body: JSON.stringify({ - ...payload, - // Present on every save so the server can detect a newer copy; left - // out only when load never reported a rev (an older backend). - ...(baseRev !== undefined ? { base_rev: baseRev } : {}), - ...(forceEmpty ? { force_empty: true } : {}), - ...(forceOverwrite ? { force_overwrite: true } : {}), - }), - }); + let result: ApiResult; + try { + result = await withTimeout('Save', async (signal) => { + const resp = await fetch(`${whpConfig.apiUrl}?action=save`, { + signal, + method: 'POST', + headers: { + 'Content-Type': 'application/json', + 'X-CSRF-Token': whpConfig.csrfToken, + }, + body: JSON.stringify({ + ...payload, + // Present on every save so the server can detect a newer copy; left + // out only when load never reported a rev (an older backend). + ...(baseRev !== undefined ? { base_rev: baseRev } : {}), + ...(forceEmpty ? { force_empty: true } : {}), + ...(forceOverwrite ? { force_overwrite: true } : {}), + }), + }); - // 409 empty_overwrite_refused is an expected outcome the UI handles, so - // return it as a structured result instead of throwing. - if (resp.status === 409) { - const body = await resp.json().catch(() => null); - if (body?.error === 'empty_overwrite_refused') { - return { - success: false, - error: 'empty_overwrite_refused', - message: body.message || 'This would replace your saved site with an empty page.', - }; - } - // Opens the conflict modal (here, so the save inside publish raises it too). - const message = body?.message || 'This site was changed in another tab or device since you opened it.'; - setConflict(message); - return { ...(body ?? {}), success: false, error: 'conflict', message }; + // 409 empty_overwrite_refused is an expected outcome the UI handles, so + // return it as a structured result instead of throwing. + if (resp.status === 409) { + const body = await resp.json().catch(() => null); + if (body?.error === 'empty_overwrite_refused') { + return { + success: false, + error: 'empty_overwrite_refused', + message: body.message || 'This would replace your saved site with an empty page.', + }; + } + // Opens the conflict modal (here, so the save inside publish raises it too). + const message = body?.message || 'This site was changed in another tab or device since you opened it.'; + setConflict(message); + return { ...(body ?? {}), success: false, error: 'conflict', message }; + } + + return readApiResult(resp, 'Save'); + }); + } catch (e) { + if (e instanceof ApiTimeoutError) return timeoutResult(e); + throw e; } - - const result = await readApiResult(resp, 'Save'); // The one-shot force_empty is spent only once a save actually went through. if (result.success) { if (forceEmpty) forceEmptyNextSave.current = false; @@ -466,33 +512,44 @@ export function useWhpApi() { } // Then publish from staging to live - const resp = await fetch( - `${whpConfig.apiUrl}?action=publish&site_id=${whpConfig.siteId}`, - { - method: 'POST', - headers: { - 'Content-Type': 'application/json', - 'X-CSRF-Token': whpConfig.csrfToken, - }, - body: JSON.stringify({ site_id: whpConfig.siteId }), - }, - ); - return readApiResult(resp, 'Publish'); + try { + return await withTimeout('Publish', async (signal) => { + const resp = await fetch( + `${whpConfig.apiUrl}?action=publish&site_id=${whpConfig.siteId}`, + { + signal, + method: 'POST', + headers: { + 'Content-Type': 'application/json', + 'X-CSRF-Token': whpConfig.csrfToken, + }, + body: JSON.stringify({ site_id: whpConfig.siteId }), + }, + ); + return readApiResult(resp, 'Publish'); + }); + } catch (e) { + if (e instanceof ApiTimeoutError) return timeoutResult(e); + throw e; + } }, [isWHP, whpConfig, loadState, save]); const load = useCallback(async () => { if (!isWHP || !whpConfig) return null; - const resp = await fetch( - `${whpConfig.apiUrl}?action=load&site_id=${whpConfig.siteId}`, - ); - if (!resp.ok) throw new Error(`Load failed: HTTP ${resp.status}`); - let data; - try { - data = await resp.json(); - } catch { - throw new Error('Load failed: response was not valid JSON'); - } + // A timeout rejects with ApiTimeoutError (an Error with a readable message). + const data = await withTimeout('Load', async (signal) => { + const resp = await fetch( + `${whpConfig.apiUrl}?action=load&site_id=${whpConfig.siteId}`, + { signal }, + ); + if (!resp.ok) throw new Error(`Load failed: HTTP ${resp.status}`); + try { + return await resp.json(); + } catch { + throw new Error('Load failed: response was not valid JSON'); + } + }); if (!data || data.success !== true) { throw new Error(`Load failed: ${data?.message || data?.error || 'server reported failure'}`); }