fix(builder): make switchPage/deletePage state updaters pure (D6)
switchPage and deletePage ran side effects (loadState -> actions.deserialize, setActivePageId, activePageIdRef mutation) INSIDE setPages/setHeaderPage/ setFooterPage updater callbacks -- using the functional-updater form purely to peek at the latest `prev` value. React (in StrictMode dev builds) double-invokes updater functions passed to setState to catch exactly this kind of impurity, so every page switch deserialized the target's craft state twice. Add pagesRef/headerPageRef/footerPageRef mirroring the latest state on every render (same pattern already used for activePageIdRef), so switchPage/deletePage can read "what's current" synchronously in the event-handler body and run their side effects there -- after the state update is computed, not inside the updater. deletePage now passes a plain next-value to setPages instead of a function updater. Switching/deleting behavior (correct page loads, can't delete the last page) is unchanged. Verified the added test fails against the pre-fix code (deserialize called 2x) and passes against the fix (1x). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -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 <React.StrictMode>, 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<typeof usePages> | null = null;
|
||||
const Consumer: React.FC = () => {
|
||||
ctx = usePages();
|
||||
return null;
|
||||
};
|
||||
|
||||
render(
|
||||
<React.StrictMode>
|
||||
<PageProvider>
|
||||
<Consumer />
|
||||
</PageProvider>
|
||||
</React.StrictMode>,
|
||||
);
|
||||
|
||||
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<typeof usePages> | null = null;
|
||||
const Consumer: React.FC = () => {
|
||||
ctx = usePages();
|
||||
return null;
|
||||
};
|
||||
|
||||
render(
|
||||
<React.StrictMode>
|
||||
<PageProvider>
|
||||
<Consumer />
|
||||
</PageProvider>
|
||||
</React.StrictMode>,
|
||||
);
|
||||
|
||||
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<typeof usePages> | null = null;
|
||||
const Consumer: React.FC = () => {
|
||||
ctx = usePages();
|
||||
return null;
|
||||
};
|
||||
|
||||
render(
|
||||
<React.StrictMode>
|
||||
<PageProvider>
|
||||
<Consumer />
|
||||
</PageProvider>
|
||||
</React.StrictMode>,
|
||||
);
|
||||
|
||||
act(() => {
|
||||
ctx!.deletePage(ctx!.pages[0].id);
|
||||
});
|
||||
|
||||
expect(ctx!.pages.length).toBe(1);
|
||||
|
||||
unmount();
|
||||
});
|
||||
});
|
||||
@@ -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],
|
||||
);
|
||||
|
||||
Reference in New Issue
Block a user