diff --git a/.changeset/a-class-id-is-a-key-not-a-property.md b/.changeset/a-class-id-is-a-key-not-a-property.md new file mode 100644 index 0000000000..f78f0b5c9f --- /dev/null +++ b/.changeset/a-class-id-is-a-key-not-a-property.md @@ -0,0 +1,39 @@ +--- +"@nextlyhq/adapter-drizzle": patch +"@nextlyhq/adapter-mysql": patch +"@nextlyhq/adapter-postgres": patch +"@nextlyhq/adapter-sqlite": patch +"@nextlyhq/admin": patch +"@nextlyhq/admin-css": patch +"@nextlyhq/blocks-engine": patch +"@nextlyhq/blocks-react": patch +"@nextlyhq/builder": patch +"create-nextly-app": patch +"@nextlyhq/eslint-config": patch +"@nextlyhq/eslint-plugin": patch +"@nextlyhq/module-specifiers": patch +"nextly": patch +"@nextlyhq/plugin-form-builder": patch +"@nextlyhq/plugin-mcp": patch +"@nextlyhq/plugin-page-builder": patch +"@nextlyhq/plugin-sdk": patch +"@nextlyhq/plugin-seo": patch +"@nextlyhq/prettier-config": patch +"@nextlyhq/storage-s3": patch +"@nextlyhq/storage-uploadthing": patch +"@nextlyhq/storage-vercel-blob": patch +"@nextlyhq/telemetry": patch +"@nextlyhq/tsconfig": patch +"@nextlyhq/ui": patch +--- + +A class whose id was `__proto__` could not be renamed again after a refused +save. The class manager records which rename of each class is the live one, and +it kept that record on a plain object keyed by class id, where `__proto__` reads +back an inherited object and a write to it stores nothing. The refused rename +never released its pending name, so retrying the same name was taken as no +change at all. + +The record is now a `Map`, and the pending names are read as the record's own +entries in both the editor and the class manager panel, so a class behaves the +same whatever id it carries. diff --git a/packages/builder/src/class-manager-panel.test.tsx b/packages/builder/src/class-manager-panel.test.tsx index f625927cae..892a5423c7 100644 --- a/packages/builder/src/class-manager-panel.test.tsx +++ b/packages/builder/src/class-manager-panel.test.tsx @@ -266,6 +266,34 @@ describe("renaming in place", () => { expect(onRename).not.toHaveBeenCalled(); }); + it("treats a name typed away and back as no rename, whatever the class id", () => { + /* + * A class id is stored data, and a pending-name record indexed by it can + * answer an inherited property instead of an entry. For `__proto__` that + * answer is an object, so a name typed away and back was compared against + * it rather than against the class's own name, and sent as a rename. + */ + const { onRename } = draw({ + library: [cls("__proto__", "odd", 0)], + pendingSlugs: {}, + }); + fireEvent.change(nameField("odd"), { target: { value: "od" } }); + fireEvent.change(nameField("odd"), { target: { value: "odd" } }); + fireEvent.keyDown(nameField("odd"), { key: "Enter" }); + expect(onRename).not.toHaveBeenCalled(); + }); + + it("draws the list when a host passes null for the pending names", () => { + // The prop's type excludes null, but a host written in JavaScript can pass + // it, and an own-key check on null throws while the rows are being drawn. + const { onRename } = draw({ + pendingSlugs: null as unknown as Record, + }); + fireEvent.change(nameField("hero"), { target: { value: "banner" } }); + fireEvent.keyDown(nameField("hero"), { key: "Enter" }); + expect(onRename).toHaveBeenCalledWith("id-hero", "banner"); + }); + it("treats a name typed away and back as no rename at all", () => { /* * Two things at once, and both matter. Its own slug is not a COLLISION, so diff --git a/packages/builder/src/class-manager-panel.tsx b/packages/builder/src/class-manager-panel.tsx index 256042ced6..e1842e769c 100644 --- a/packages/builder/src/class-manager-panel.tsx +++ b/packages/builder/src/class-manager-panel.tsx @@ -704,6 +704,28 @@ function usablePageSize(pageSize: number | undefined): number { : DEFAULT_PAGE_SIZE; } +/** + * The name a class is heading for, read as the record's OWN entry. + * + * A class id is stored data, so this record can be asked about any string. An + * index answers an inherited property for some of them, `__proto__` among them, + * and the row would then compare an edit against that object rather than a + * name, sending a name typed away and back as a rename. + * + * `null` is taken as no record, as an omitted one is. The prop's type excludes + * it, but a host written in JavaScript can still pass it, and `Object.hasOwn` + * throws on it where an optional index would have answered nothing. + */ +function pendingSlugFor( + pendingSlugs: Readonly> | null | undefined, + classId: string +): string | undefined { + if (pendingSlugs === undefined || pendingSlugs === null) return undefined; + return Object.hasOwn(pendingSlugs, classId) + ? pendingSlugs[classId] + : undefined; +} + function ClassList({ rows, searching, @@ -793,7 +815,7 @@ function ClassList({
  • { expect(last).toContain('"card"'); }); + it("lets a class whose id is __proto__ retry a rename its save refused", async () => { + /* + * The rename identity is keyed by class id, and a class id is stored data. + * On a plain record `__proto__` read back an inherited object and wrote + * through the prototype setter, so the refused attempt never released its + * pending name, and the retry, judged against that name, was dropped as no + * change at all. + */ + storedRead = { + data: { + classes: [{ id: "__proto__", slug: "card", orderIndex: 0, styles: {} }], + }, + isPending: false, + error: null, + }; + saveResult = new Error("The site style is locked."); + openEditor(); + + await act(async () => { + fireEvent.change(screen.getByLabelText("Name of card"), { + target: { value: "panel" }, + }); + fireEvent.blur(screen.getByLabelText("Name of card")); + }); + expect(await screen.findByText("The site style is locked.")).toBeTruthy(); + const refused = saved.length; + expect(refused).toBeGreaterThan(0); + + saveResult = { success: true }; + await act(async () => { + fireEvent.change(screen.getByLabelText("Name of card"), { + target: { value: "panel" }, + }); + fireEvent.blur(screen.getByLabelText("Name of card")); + }); + + // Written again, rather than swallowed as a no-op against a stuck name. + await vi.waitFor(() => expect(saved.length).toBeGreaterThan(refused)); + expect(JSON.stringify(saved[saved.length - 1])).toContain('"panel"'); + }); + it("says a failed read failed, rather than loading forever", () => { // A read that FAILED will not finish. A panel still saying "loading" // describes a state the site is not in, and the author waits for something diff --git a/packages/plugin-page-builder/src/admin/BlocksField.tsx b/packages/plugin-page-builder/src/admin/BlocksField.tsx index e95a7764da..aaa85f0bc5 100644 --- a/packages/plugin-page-builder/src/admin/BlocksField.tsx +++ b/packages/plugin-page-builder/src/admin/BlocksField.tsx @@ -1172,11 +1172,15 @@ function useClassSurface( * A ref rather than state because both readers need the value SYNCHRONOUSLY — * the panel reads it in the same event that started the attempt, and a state * update is not visible until the render after. + * + * A `Map` rather than a record, because a class id is stored data: every string + * a class can carry has to be a key here, and on a plain object some strings, + * `__proto__` among them, name an inherited accessor instead of an entry. */ - const renameAttempts = useRef>({}); + const renameAttempts = useRef(new Map()); const beginRename = useCallback((classId: string, slug: string): number => { - const mine = (renameAttempts.current[classId] ?? 0) + 1; - renameAttempts.current[classId] = mine; + const mine = (renameAttempts.current.get(classId) ?? 0) + 1; + renameAttempts.current.set(classId, mine); setPendingSlugs(current => current[classId] === slug ? current : { ...current, [classId]: slug } ); @@ -1191,16 +1195,19 @@ function useClassSurface( * reads as a no-op while the queued rename goes on to persist a different * name. */ - if (renameAttempts.current[classId] !== mine) return; + if (renameAttempts.current.get(classId) !== mine) return; setPendingSlugs(current => { - if (!(classId in current)) return current; + // The record's OWN entry, for the reason the counters are a `Map`: `in` + // also answers for inherited names, and removing one that is not there + // would rebuild the record for nothing. + if (!Object.hasOwn(current, classId)) return current; const { [classId]: _gone, ...rest } = current; return rest; }); }, []); /** The live attempt for a class, for a caller deciding whether it is current. */ const currentRenameAttempt = useCallback( - (classId: string): number => renameAttempts.current[classId] ?? 0, + (classId: string): number => renameAttempts.current.get(classId) ?? 0, [] ); return {