fix(pages): duplicatePage must save outgoing active canvas before teardown (avoid dropping live edits)

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

craft/src/state/PageContext.pages-productivity.test.tsx

@@ -240,6 +240,44 @@ describe('PageContext.movePage', () => {
 
     unmount();
   });
+
+  test('moving the current landing page down demotes it and promotes its neighbor', () => {
+    let ctx: ReturnType<typeof usePages> | null = null;
+    const Consumer: React.FC = () => {
+      ctx = usePages();
+      return null;
+    };
+    render(
+      <PageProvider>
+        <Consumer />
+      </PageProvider>,
+    );
+
+    act(() => ctx!.addPage('About', 'about'));
+    // pages: [Home(index0, slug index), About]
+    const homeId = ctx!.pages[0].id;
+    const aboutId = ctx!.pages[1].id;
+
+    act(() => ctx!.movePage(homeId, 'down'));
+    // pages: [About, Home]
+
+    expect(ctx!.pages.map((p) => p.id)).toEqual([aboutId, homeId]);
+
+    // Order changed and the landing invariant re-established: index 0
+    // (now About) gets slug 'index'; the moved page (now at index 1, Home)
+    // gets a real, non-'index' unique slug.
+    expect(ctx!.pages[0].id).toBe(aboutId);
+    expect(ctx!.pages[0].slug).toBe('index');
+    const demotedHome = ctx!.pages.find((p) => p.id === homeId)!;
+    expect(demotedHome.slug).not.toBe('index');
+    expect(demotedHome.slug).toBe('home');
+
+    // Exactly one 'index' slug.
+    const indexPages = ctx!.pages.filter((p) => p.slug === 'index');
+    expect(indexPages).toHaveLength(1);
+
+    unmount();
+  });
 });
 
 describe('PageContext.setLandingPage', () => {
@@ -404,6 +442,50 @@ describe('PageContext.duplicatePage', () => {
     unmount();
   });
 
+  test('duplicating a non-active page does NOT drop the outgoing active page\'s live unsaved edits (regression lock)', async () => {
+    // Regression test for the Critical bug: duplicatePage(pageId) used to
+    // call saveCurrentState() ONLY when pageId === the active page, yet
+    // ALWAYS ended by tearing down the canvas via loadState() + switching
+    // activePageId to the copy. If the duplicated page was NOT the active
+    // one, the active page's live canvas edits were never serialized into
+    // its slot before that teardown -- silently discarded. This asserts the
+    // outgoing active page ('About') keeps its live-serialized craftState
+    // after duplicating a DIFFERENT page ('Home').
+    let ctx: ReturnType<typeof usePages> | null = null;
+    const Consumer: React.FC = () => {
+      ctx = usePages();
+      return null;
+    };
+    render(
+      <PageProvider>
+        <Consumer />
+      </PageProvider>,
+    );
+
+    act(() => ctx!.addPage('About', 'about'));
+    await flushTimers();
+    // pages: [Home, About]; About is active (addPage switches to it).
+    const homeId = ctx!.pages[0].id;
+    const aboutId = ctx!.pages[1].id;
+    expect(ctx!.activePageId).toBe(aboutId);
+
+    // Simulate the user having made live, unsaved edits to About (the
+    // active page) that have not yet been serialized into pages[] state.
+    const liveAboutEdit = '{"ROOT":{"live":"about-edit-not-yet-saved"}}';
+    serializeReturn = liveAboutEdit;
+
+    // Duplicate a DIFFERENT page (Home), not the active one (About).
+    act(() => ctx!.duplicatePage(homeId));
+    await flushTimers();
+
+    // The outgoing active page's live edits must have been persisted into
+    // its own slot before the canvas was torn down and switched away.
+    const aboutAfter = ctx!.pages.find((p) => p.id === aboutId)!;
+    expect(aboutAfter.craftState).toBe(liveAboutEdit);
+
+    unmount();
+  });
+
   test('switches the canvas to the new copy (deserialize called with the copy craftState)', async () => {
     let ctx: ReturnType<typeof usePages> | null = null;
     const Consumer: React.FC = () => {

craft/src/state/PageContext.tsx

@@ -268,24 +268,28 @@ export function uniqueSlug(base: string, existingSlugs: string[]): string {
  * shared "fix the invariant up after reordering" step, and directly
  * unit-testable without mounting `PageProvider`.
  *
- * Assumes at most one page enters with slug `'index'` (true for any array
+ * Normally at most one page enters with slug `'index'` (true for any array
  * that already satisfied the invariant before the reorder that produced this
- * input) -- exactly the case both callers hand it.
+ * input) -- exactly the case both callers hand it. Defensively, though, a
+ * STRAY second page with slug `'index'` at index > 0 (e.g. from legacy
+ * loaded data that predates this invariant) is also demoted rather than left
+ * as a duplicate -- see the running `usedSlugs` accumulation below.
  */
 export function applyLandingInvariant(pages: PageData[]): PageData[] {
   if (pages.length === 0) return pages;
 
-  // Slugs that are FIXED and must not be collided into: 'index' (reserved
+  // Slugs that must not be collided into: 'index' (reserved for whoever
-  // for whoever ends up at index 0) plus every non-landing page's existing
+  // ends up at index 0) plus every non-landing page's existing slug except
-  // slug except the demoted page's (it currently holds 'index' and is about
+  // any demoted page's (it currently holds 'index' and is about to be given
-  // to be given a new one). Computed upfront, over the WHOLE array, so the
+  // a new one). Computed upfront, over the WHOLE array, so a demoted page's
-  // demoted page's new slug is checked against every other page regardless
+  // new slug is checked against every other page regardless of array order
-  // of array order -- checking only "slugs seen so far" while walking the
+  // -- checking only "slugs seen so far" while walking the array would miss
-  // array would miss a collision against a page that appears LATER in the
+  // a collision against a page that appears LATER in the list than the
-  // list than the demoted one.
+  // demoted one. Mutated (pushed to) as pages are demoted below so that two
-  const fixedSlugs: string[] = ['index'];
+  // demoted pages in the same pass can't collide with EACH OTHER either.
+  const usedSlugs: string[] = ['index'];
   for (let i = 1; i < pages.length; i++) {
-    if (pages[i].slug !== 'index') fixedSlugs.push(pages[i].slug);
+    if (pages[i].slug !== 'index') usedSlugs.push(pages[i].slug);
   }
 
   return pages.map((page, i) => {
@@ -293,8 +297,11 @@ export function applyLandingInvariant(pages: PageData[]): PageData[] {
       return page.slug === 'index' ? page : { ...page, slug: 'index' };
     }
     if (page.slug === 'index') {
-      // Demoted landing page -- give it a real, unique slug of its own.
+      // Demoted landing page (or a stray extra 'index' page -- see doc
-      return { ...page, slug: uniqueSlug(slugify(page.name), fixedSlugs) };
+      // comment above) -- give it a real, unique slug of its own.
+      const newSlug = uniqueSlug(slugify(page.name), usedSlugs);
+      usedSlugs.push(newSlug);
+      return { ...page, slug: newSlug };
     }
     return page;
   });
@@ -487,21 +494,25 @@ export const PageProvider: React.FC<{ children: ReactNode }> = ({ children }) =>
    */
   const duplicatePage = useCallback(
     (pageId: string) => {
+      // Always persist whatever is on the live canvas back into its page
+      // slot BEFORE any teardown below (same as addPage/switchPage/deletePage
+      // do unconditionally). Without this, duplicating a page OTHER than the
+      // active one would tear down and switch the canvas via loadState()
+      // further down without ever serializing the outgoing active page's
+      // live edits into its slot -- silently discarding them.
+      saveCurrentState();
+
       const isActive = pageId === activePageIdRef.current;
-
-      // If the source is the active page, persist its current on-canvas
-      // state back into `pages` first (same as switchPage/addPage do)
-      // so the ORIGINAL page isn't left with a stale craftState after
-      // this. `query.serialize()` below reads the live canvas directly
-      // rather than waiting on this (React state update timing aside,
-      // it's simplest to just ask Craft.js for the truth).
-      if (isActive) {
-        saveCurrentState();
-      }
-
       const source = pagesRef.current.find((p) => p.id === pageId);
       if (!source) return;
 
+      // If the source IS the active page, saveCurrentState() above just
+      // wrote the live canvas into `source.craftState`'s slot -- but
+      // `pagesRef.current` (captured above) may still be the pre-update
+      // snapshot depending on render timing, so ask Craft.js directly for
+      // the same value rather than re-reading the ref. If the source is a
+      // NON-active page, its stored craftState is untouched by saving the
+      // (different) active page above, so use it as-is.
       const sourceCraftState = isActive ? query.serialize() : source.craftState;
       const otherSlugs = pagesRef.current.map((p) => p.slug);
       const copyId = nextPageId();