fix: unique addPage ids + collision-free scopeId hashing
M-3: PageContext.addPage minted ids from bare `page_${Date.now()}` --
two adds inside the same millisecond collided on id, so a subsequent
rename/delete/save silently acted on both pages at once. Added a
module-scoped monotonic counter combined with the timestamp
(nextPageId(), exported for direct unit testing) and used it
everywhere an addPage-style id is minted (addPage, replaceAllPages).
M-4: scopeId() lowercased + stripped non-alphanumeric characters from
the node id into a slug, so two node ids differing only by
case/punctuation (e.g. "AbC" vs "abc", or "a-b" vs "ab") collapsed
onto the same scope -- defeating the whole point of scoping ids per
node (M-1/Menu/Tabs/ColumnLayout/Gallery/etc. all rely on it). Now
hashes the raw node id via the existing djb2 stableHash() instead of
slugifying it: still deterministic (same id -> same scope) and a valid
CSS ident, but collision-resistant across case/punctuation. This
changes the exact scope strings Menu/Tabs/ColumnLayout/Gallery/etc.
emit -- expected and fine, since none of their tests pinned an exact
scope value (all already asserted structure/uniqueness).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -2,7 +2,7 @@ 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';
|
||||
import { PageProvider, usePages, uniqueSlug } from './PageContext';
|
||||
import { PageProvider, usePages, uniqueSlug, nextPageId } from './PageContext';
|
||||
|
||||
/* PageContext only needs `useEditor` from @craftjs/core (for query.serialize /
|
||||
actions.deserialize during page switches) — mock just that so PageProvider
|
||||
@@ -15,11 +15,10 @@ vi.mock('@craftjs/core', () => ({
|
||||
}),
|
||||
}));
|
||||
|
||||
/* addPage mints ids from `Date.now()`. Two adds inside the same test can land
|
||||
in the same millisecond and collide on id, which is an existing, unrelated
|
||||
bug (id collision, not slug collision) — out of scope here but it makes
|
||||
these tests flaky since a colliding id defeats the "other pages" slug
|
||||
lookup. Force distinct ids so the slug-dedupe assertions below are stable. */
|
||||
/* addPage mints ids via nextPageId() (timestamp + monotonic counter, M-3),
|
||||
so same-millisecond calls no longer collide on id. Date.now() is still
|
||||
pinned/advanced here for determinism across the slug-dedupe assertions
|
||||
below, independent of wall-clock timing. */
|
||||
let dateNowSpy: ReturnType<typeof vi.spyOn>;
|
||||
beforeEach(() => {
|
||||
let counter = 1_700_000_000_000;
|
||||
@@ -48,6 +47,23 @@ function unmount() {
|
||||
container.remove();
|
||||
}
|
||||
|
||||
describe('nextPageId (M-3: no same-millisecond id collision)', () => {
|
||||
test('two calls yield distinct ids even when Date.now() is pinned to a constant', () => {
|
||||
const spy = vi.spyOn(Date, 'now').mockReturnValue(1_700_000_000_000);
|
||||
try {
|
||||
const id1 = nextPageId();
|
||||
const id2 = nextPageId();
|
||||
expect(id1).not.toBe(id2);
|
||||
} finally {
|
||||
spy.mockRestore();
|
||||
}
|
||||
});
|
||||
|
||||
test('ids are prefixed with "page_"', () => {
|
||||
expect(nextPageId()).toMatch(/^page_/);
|
||||
});
|
||||
});
|
||||
|
||||
describe('uniqueSlug', () => {
|
||||
test('returns base unchanged when no collision', () => {
|
||||
expect(uniqueSlug('about', ['index', 'contact'])).toBe('about');
|
||||
|
||||
@@ -43,6 +43,20 @@ interface PageContextValue {
|
||||
const HEADER_ID = '__header__';
|
||||
const FOOTER_ID = '__footer__';
|
||||
|
||||
// M-3: `page_${Date.now()}` alone collides when two pages are minted inside
|
||||
// the same millisecond (addPage called twice in quick succession, or two AI
|
||||
// replaceAllPages entries) -- then rename/delete/save operate on both pages
|
||||
// at once since they share an id. A module-scoped monotonic counter,
|
||||
// combined with the timestamp, guarantees uniqueness regardless of how many
|
||||
// ids are minted within the same millisecond. This is state/id-minting code
|
||||
// (not toHtml/export), so Date.now() here is fine -- see task-minors-brief.md.
|
||||
let pageIdCounter = 0;
|
||||
|
||||
/** Mints a unique page id: timestamp (base36) + a monotonic per-process counter (base36). */
|
||||
export function nextPageId(): string {
|
||||
return 'page_' + Date.now().toString(36) + '_' + (++pageIdCounter).toString(36);
|
||||
}
|
||||
|
||||
const EMPTY_CANVAS =
|
||||
'{"ROOT":{"type":{"resolvedName":"Container"},"isCanvas":true,"props":{"style":{"minHeight":"100vh","backgroundColor":"#ffffff"},"tag":"div"},"displayName":"Container","custom":{},"hidden":false,"nodes":[],"linkedNodes":{}}}';
|
||||
|
||||
@@ -338,7 +352,7 @@ export const PageProvider: React.FC<{ children: ReactNode }> = ({ children }) =>
|
||||
const addPage = useCallback(
|
||||
(name: string, slug: string) => {
|
||||
const requestedSlug = slug || slugify(name);
|
||||
const id = `page_${Date.now()}`;
|
||||
const id = nextPageId();
|
||||
|
||||
// Save current page first
|
||||
saveCurrentState();
|
||||
@@ -452,7 +466,7 @@ export const PageProvider: React.FC<{ children: ReactNode }> = ({ children }) =>
|
||||
const slug = i === 0 ? 'index' : uniqueSlug(slugify(p.name), seenSlugs);
|
||||
seenSlugs.push(slug);
|
||||
return {
|
||||
id: i === 0 ? 'home' : `page_${Date.now()}_${i}`,
|
||||
id: i === 0 ? 'home' : nextPageId(),
|
||||
name: p.name,
|
||||
slug,
|
||||
craftState: treeToCraftState(p.tree),
|
||||
|
||||
Reference in New Issue
Block a user