diff --git a/craft/src/state/PageContext.pure-updaters.test.tsx b/craft/src/state/PageContext.pure-updaters.test.tsx new file mode 100644 index 0000000..0694cee --- /dev/null +++ b/craft/src/state/PageContext.pure-updaters.test.tsx @@ -0,0 +1,147 @@ +import { describe, test, expect, vi } from 'vitest'; +import React from 'react'; +import { createRoot, Root } from 'react-dom/client'; +import { act } from 'react-dom/test-utils'; +import { PageProvider, usePages } from './PageContext'; + +/** + * D6 regression coverage: `switchPage` and `deletePage` used to run their + * side effects (actions.deserialize via loadState, setActivePageId, + * activePageIdRef mutation) INSIDE a setPages/setHeaderPage/setFooterPage + * functional updater. React (in , dev builds) invokes + * updater functions passed to setState twice to help surface exactly this + * kind of impurity — so the old code deserialized twice per page switch. + * These tests mount under StrictMode and assert the side effect fires once. + */ +const deserializeMock = vi.fn(); +vi.mock('@craftjs/core', () => ({ + useEditor: () => ({ + query: { serialize: () => '{}' }, + actions: { deserialize: deserializeMock }, + }), +})); + +let container: HTMLDivElement; +let root: Root; + +function render(ui: React.ReactElement) { + container = document.createElement('div'); + document.body.appendChild(container); + act(() => { + root = createRoot(container); + root.render(ui); + }); +} + +function unmount() { + act(() => { + root.unmount(); + }); + container.remove(); +} + +/** `loadState` defers `actions.deserialize` via a real setTimeout(fn, 0). */ +async function flushTimers() { + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, 10)); + }); +} + +describe('PageContext pure updaters (D6)', () => { + test('switchPage deserializes exactly once under StrictMode double-invocation', async () => { + deserializeMock.mockClear(); + let ctx: ReturnType | null = null; + const Consumer: React.FC = () => { + ctx = usePages(); + return null; + }; + + render( + + + + + , + ); + + act(() => { + ctx!.addPage('About', 'about'); + }); + await flushTimers(); + deserializeMock.mockClear(); + + act(() => { + ctx!.switchPage('home'); + }); + await flushTimers(); + + expect(deserializeMock).toHaveBeenCalledTimes(1); + expect(ctx!.activePageId).toBe('home'); + + unmount(); + }); + + test('deletePage: deserialize fires exactly once when deleting the active page', async () => { + deserializeMock.mockClear(); + let ctx: ReturnType | null = null; + const Consumer: React.FC = () => { + ctx = usePages(); + return null; + }; + + render( + + + + + , + ); + + act(() => { + ctx!.addPage('About', 'about'); + }); + await flushTimers(); + + const aboutId = ctx!.pages[1].id; + act(() => { + ctx!.switchPage(aboutId); + }); + await flushTimers(); + deserializeMock.mockClear(); + + act(() => { + ctx!.deletePage(aboutId); + }); + await flushTimers(); + + expect(deserializeMock).toHaveBeenCalledTimes(1); + expect(ctx!.pages.length).toBe(1); + expect(ctx!.activePageId).toBe('home'); + + unmount(); + }); + + test("deletePage: can't delete the last remaining page", async () => { + let ctx: ReturnType | null = null; + const Consumer: React.FC = () => { + ctx = usePages(); + return null; + }; + + render( + + + + + , + ); + + act(() => { + ctx!.deletePage(ctx!.pages[0].id); + }); + + expect(ctx!.pages.length).toBe(1); + + unmount(); + }); +}); diff --git a/craft/src/state/PageContext.tsx b/craft/src/state/PageContext.tsx index 55b2220..2393b77 100644 --- a/craft/src/state/PageContext.tsx +++ b/craft/src/state/PageContext.tsx @@ -195,6 +195,17 @@ export const PageProvider: React.FC<{ children: ReactNode }> = ({ children }) => const activePageIdRef = useRef(activePageId); activePageIdRef.current = activePageId; + // Mirror the latest state in refs so event handlers (switchPage, + // deletePage) can synchronously read "what's current" without smuggling a + // side effect into a setState updater function to peek at `prev` — updater + // functions must stay pure since React (StrictMode) may invoke them twice. + const pagesRef = useRef(pages); + pagesRef.current = pages; + const headerPageRef = useRef(headerPage); + headerPageRef.current = headerPage; + const footerPageRef = useRef(footerPage); + footerPageRef.current = footerPage; + const isEditingHeader = activePageId === HEADER_ID; const isEditingFooter = activePageId === FOOTER_ID; @@ -252,27 +263,19 @@ export const PageProvider: React.FC<{ children: ReactNode }> = ({ children }) => ); } - // Load target page state. - // For header/footer we need the latest saved state. Since setState above is async, - // read from the ref-like state getter. For header/footer, we read the current - // state value and fall back to what we just saved if the target is the same slot. + // Load target page state. `pageId` can never equal `currentId` here + // (guarded by the early return above), so the target's stored state + // was untouched by the setState calls just above — the refs (kept in + // sync with state on every render) are safe to read synchronously + // without waiting for a re-render, and without running the load as a + // side effect inside a setState updater. if (pageId === HEADER_ID) { - // Use functional state read to get the latest value - setHeaderPage((prev) => { - loadState(prev.craftState, EMPTY_HEADER); - return prev; - }); + loadState(headerPageRef.current.craftState, EMPTY_HEADER); } else if (pageId === FOOTER_ID) { - setFooterPage((prev) => { - loadState(prev.craftState, EMPTY_FOOTER); - return prev; - }); + loadState(footerPageRef.current.craftState, EMPTY_FOOTER); } else { - setPages((prev) => { - const target = prev.find((p) => p.id === pageId); - loadState(target?.craftState || null, EMPTY_CANVAS); - return prev; - }); + const target = pagesRef.current.find((p) => p.id === pageId); + loadState(target?.craftState || null, EMPTY_CANVAS); } setActivePageId(pageId); @@ -322,21 +325,22 @@ export const PageProvider: React.FC<{ children: ReactNode }> = ({ children }) => // Can't delete header/footer if (pageId === HEADER_ID || pageId === FOOTER_ID) return; - setPages((prev) => { - if (prev.length <= 1) return prev; + const prev = pagesRef.current; + if (prev.length <= 1) return; - const filtered = prev.filter((p) => p.id !== pageId); + const filtered = prev.filter((p) => p.id !== pageId); + setPages(filtered); - // If deleting the active page, switch to the first remaining - if (pageId === activePageIdRef.current) { - const nextPage = filtered[0]; - setActivePageId(nextPage.id); - activePageIdRef.current = nextPage.id; - loadState(nextPage.craftState, EMPTY_CANVAS); - } - - return filtered; - }); + // If deleting the active page, switch to the first remaining. This + // runs after the state update is computed (not inside the setPages + // updater) so the side effects (deserialize, ref/state mutation) fire + // exactly once regardless of how many times React invokes updaters. + if (pageId === activePageIdRef.current) { + const nextPage = filtered[0]; + setActivePageId(nextPage.id); + activePageIdRef.current = nextPage.id; + loadState(nextPage.craftState, EMPTY_CANVAS); + } }, [loadState], );