diff --git a/craft/src/state/PageContext.pages-productivity.test.tsx b/craft/src/state/PageContext.pages-productivity.test.tsx index 1be4d70..b45e804 100644 --- a/craft/src/state/PageContext.pages-productivity.test.tsx +++ b/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 | null = null; + const Consumer: React.FC = () => { + ctx = usePages(); + return null; + }; + render( + + + , + ); + + 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 | null = null; + const Consumer: React.FC = () => { + ctx = usePages(); + return null; + }; + render( + + + , + ); + + 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 | null = null; const Consumer: React.FC = () => { diff --git a/craft/src/state/PageContext.tsx b/craft/src/state/PageContext.tsx index 7c60152..2a52ada 100644 --- a/craft/src/state/PageContext.tsx +++ b/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 - // for whoever ends up at index 0) plus every non-landing page's existing - // slug except the demoted page's (it currently holds 'index' and is about - // to be given a new one). Computed upfront, over the WHOLE array, so the - // demoted page's new slug is checked against every other page regardless - // of array order -- checking only "slugs seen so far" while walking the - // array would miss a collision against a page that appears LATER in the - // list than the demoted one. - const fixedSlugs: string[] = ['index']; + // Slugs that must not be collided into: 'index' (reserved for whoever + // ends up at index 0) plus every non-landing page's existing slug except + // any demoted page's (it currently holds 'index' and is about to be given + // a new one). Computed upfront, over the WHOLE array, so a demoted page's + // new slug is checked against every other page regardless of array order + // -- checking only "slugs seen so far" while walking the array would miss + // a collision against a page that appears LATER in the list than the + // demoted one. Mutated (pushed to) as pages are demoted below so that two + // 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. - return { ...page, slug: uniqueSlug(slugify(page.name), fixedSlugs) }; + // Demoted landing page (or a stray extra 'index' page -- see doc + // 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();