diff --git a/.changeset/locked-blocks-are-actually-locked.md b/.changeset/locked-blocks-are-actually-locked.md new file mode 100644 index 0000000000..bda24f780b --- /dev/null +++ b/.changeset/locked-blocks-are-actually-locked.md @@ -0,0 +1,34 @@ +--- +"nextly": patch +"create-nextly-app": patch +"@nextlyhq/admin": patch +"@nextlyhq/admin-css": patch +"@nextlyhq/blocks-engine": patch +"@nextlyhq/blocks-react": patch +"@nextlyhq/ui": patch +"@nextlyhq/adapter-drizzle": patch +"@nextlyhq/adapter-postgres": patch +"@nextlyhq/adapter-mysql": patch +"@nextlyhq/adapter-sqlite": patch +"@nextlyhq/storage-s3": patch +"@nextlyhq/storage-uploadthing": patch +"@nextlyhq/storage-vercel-blob": patch +"@nextlyhq/plugin-form-builder": patch +"@nextlyhq/plugin-page-builder": patch +"@nextlyhq/plugin-seo": patch +"@nextlyhq/plugin-sdk": patch +"@nextlyhq/eslint-config": patch +"@nextlyhq/eslint-plugin": patch +"@nextlyhq/prettier-config": patch +"@nextlyhq/telemetry": patch +"@nextlyhq/tsconfig": patch +"@nextlyhq/builder": patch +"@nextlyhq/module-specifiers": patch +--- + +A locked block in the page builder now resists being moved or deleted, and says why. Until now the +flag was honoured only when dragging, so a block the layers panel showed as locked could still be +moved with the keyboard or deleted outright. Deleting a container that holds a locked block is +refused too, since removing the container would destroy the block inside it, and the refusal names +which block is locked. Moving a container is still allowed, because its locked children keep their +place inside it. diff --git a/packages/builder/src/canvas-drag.tsx b/packages/builder/src/canvas-drag.tsx index f9ee306da0..9d126830cd 100644 --- a/packages/builder/src/canvas-drag.tsx +++ b/packages/builder/src/canvas-drag.tsx @@ -60,6 +60,7 @@ import type { EditorState } from "./editor-state"; import type { Point, Rect } from "./geometry"; import { canvasContentPoint, canvasContentRect } from "./geometry-dom"; import type { SlotSource } from "./inserter"; +import { lockBlockingMove } from "./locking"; import { nextTargetSwitchState, NO_TARGET, @@ -220,7 +221,13 @@ export function useCanvasDrag({ // A locked block is one the author has asked the editor not to move. The // press is left alone rather than swallowed, so it still selects. - if (node.locked === true) return; + // + // Asked through the shared rule rather than by reading the flag here: the + // keyboard moves and delete ask the same question, and delete's answer + // differs — it checks the whole subtree, because removing a container + // destroys what is inside it. Three inline reads of `node.locked` would + // agree until one of them gained that distinction. + if (lockBlockingMove(current.document, nodeId) !== undefined) return; const rects = snapshotRects(root); gesture.current = { diff --git a/packages/builder/src/index.ts b/packages/builder/src/index.ts index bbdc650960..6b3ca202d3 100644 --- a/packages/builder/src/index.ts +++ b/packages/builder/src/index.ts @@ -232,3 +232,14 @@ export { type LayerSearch, } from "./layers"; export { blockLabel } from "./inserter"; + +/** + * @experimental Whether the editor may move or delete a block the author has + * locked. + * + * From this entry because it is plain functions over a document. A host or an + * agent deciding whether an action is permitted has to ask the SAME question + * the editor asks — a second reading of `node.locked` would miss that deleting + * a container is refused by a lock anywhere inside it, while moving one is not. + */ +export { isLocked, lockBlockingDelete, lockBlockingMove } from "./locking"; diff --git a/packages/builder/src/keyboard-actions.test.tsx b/packages/builder/src/keyboard-actions.test.tsx index e053c427be..e5c39bc725 100644 --- a/packages/builder/src/keyboard-actions.test.tsx +++ b/packages/builder/src/keyboard-actions.test.tsx @@ -117,6 +117,66 @@ describe("useBlockKeyboardActions", () => { expect(op.to).toEqual({ index: 1 }); }); + it("refuses to move a locked block, and says why", () => { + const editor = editorSpy( + documentOf([ + { id: "a", type: "acme/text", version: 1, props: {}, locked: true }, + { id: "b", type: "acme/text", version: 1, props: {} }, + ] as BlockDocument["nodes"]), + "a" + ); + mount(editor); + + press("ArrowDown", { altKey: true }); + + expect(editor.apply).not.toHaveBeenCalled(); + expect(screen.getByRole("status").textContent ?? "").toMatch( + /is locked\. unlock it to move it/i + ); + }); + + it("ALLOWS moving a container that holds a locked block", () => { + /* + * The direction that is easy to get wrong by being cautious, and the reason + * move and delete ask different questions. + * + * Moving the container leaves the locked child in the same slot at the same + * index with the same neighbours, so nothing the lock protects has changed. + * Refusing here would let one locked caption freeze the entire section + * around it, which an author would experience as the editor breaking rather + * than as a lock working. + */ + const editor = editorSpy( + documentOf([ + { + id: "wrap", + type: "acme/box", + version: 1, + props: {}, + slots: { + children: [ + { + id: "kid", + type: "acme/text", + version: 1, + props: {}, + locked: true, + }, + ], + }, + }, + { id: "after", type: "acme/text", version: 1, props: {} }, + ] as BlockDocument["nodes"]), + "wrap" + ); + mount(editor); + + press("ArrowDown", { altKey: true }); + + expect(editor.apply).toHaveBeenCalledTimes(1); + expect(editor.apply.mock.calls[0][0].kind).toBe("move"); + }); + it("moves the selected block up", () => { const editor = editorSpy(pair(), "b"); mount(editor); @@ -393,6 +453,92 @@ describe("useBlockKeyboardActions", () => { expect(said).toContain("Undo"); }); + it("refuses to delete a locked block, and says why", () => { + /* + * A lock is a POLICY refusal, so it is announced where a structural one is + * not. Nothing on the page explains why the key did nothing, the remedy is + * one the author can act on, and a keyboard user has no lock badge to look + * at — which is the whole reason silence would be wrong here. + */ + const editor = editorSpy( + documentOf([ + { id: "a", type: "acme/text", version: 1, props: {}, locked: true }, + { id: "b", type: "acme/text", version: 1, props: {} }, + ] as BlockDocument["nodes"]), + "a" + ); + mount(editor); + + press("Delete"); + + expect(editor.apply).not.toHaveBeenCalled(); + expect(screen.getByRole("status").textContent ?? "").toMatch( + /is locked\. unlock it to delete it/i + ); + }); + + it("refuses to delete a container holding a locked block, and names it", () => { + // THE case. Deleting the container destroys the locked child, which is the + // one outcome the flag exists to prevent — and it happens through an action + // aimed at something else, with the descendant count as the only clue. + const editor = editorSpy( + documentOf([ + { + id: "wrap", + type: "acme/box", + version: 1, + props: {}, + slots: { + children: [ + { + id: "kid", + type: "acme/text", + version: 1, + props: {}, + locked: true, + }, + ], + }, + }, + ] as BlockDocument["nodes"]), + "wrap" + ); + mount(editor); + + press("Delete"); + + expect(editor.apply).not.toHaveBeenCalled(); + expect(screen.getByRole("status").textContent ?? "").toMatch( + /contains .*which is locked/i + ); + }); + + it("still deletes a container whose children are all unlocked", () => { + // The control for both refusals above. Without it, a delete that refused + // everything would satisfy them and nothing would notice. + const editor = editorSpy( + documentOf([ + { + id: "wrap", + type: "acme/box", + version: 1, + props: {}, + slots: { + children: [{ id: "kid", type: "acme/text", version: 1, props: {} }], + }, + }, + ] as BlockDocument["nodes"]), + "wrap" + ); + mount(editor); + + press("Delete"); + + expect(editor.apply).toHaveBeenCalledWith( + expect.objectContaining({ kind: "remove", id: "wrap" }) + ); + }); + it("says how many blocks a container takes with it", () => { // A collapsed section looks exactly like an empty one, so the count is the // only thing telling an author what they just lost. diff --git a/packages/builder/src/keyboard-actions.tsx b/packages/builder/src/keyboard-actions.tsx index 9ead6b76f2..9ff9db6487 100644 --- a/packages/builder/src/keyboard-actions.tsx +++ b/packages/builder/src/keyboard-actions.tsx @@ -31,6 +31,7 @@ import { type MoveDirection, type MoveEffect, } from "./keyboard-move"; +import { lockBlockingDelete, lockBlockingMove } from "./locking"; /** * The bindings, and why these keys. @@ -183,6 +184,31 @@ export function useBlockKeyboardActions({ // said, because there is no event to report. if (deletion === null) return; + /* + * A lock is a POLICY refusal, so it is announced where a structural one is + * not. + * + * The moves below stay silent when a block simply has nowhere to go — that + * is a fact about the document an author can see, and saying it on every + * press at the end of a list is noise. A lock is the opposite: nothing about + * the page explains why the key did nothing, the remedy is one the author + * can act on, and a keyboard user has no badge to look at. + * + * The subtree is checked, not just the node. Deleting a container destroys + * what is inside it, so an author who locked a caption and then removed its + * section would lose the thing they locked through an action aimed at + * something else. + */ + const blocked = lockBlockingDelete(editorNow.document, deletion.id); + if (blocked !== undefined) { + announce( + blocked.id === deletion.id + ? `${blockLabel(blocked.type)} is locked. Unlock it to delete it.` + : `This block contains ${blockLabel(blocked.type)}, which is locked. Unlock it to delete.` + ); + return; + } + const applied = editorNow.apply({ kind: "remove", id: deletion.id, @@ -217,6 +243,18 @@ export function useBlockKeyboardActions({ const selectedId = editorNow.selectedId; if (selectedId === null) return; + // Only the node itself, never its subtree: moving a container leaves + // a locked child in the same slot at the same index, so the lock is + // not violated and refusing would let one locked caption freeze the + // whole section around it. + const lockedNode = lockBlockingMove(editorNow.document, selectedId); + if (lockedNode !== undefined) { + announce( + `${blockLabel(lockedNode.type)} is locked. Unlock it to move it.` + ); + return; + } + const move = keyboardMovePosition( editorNow.document.nodes, selectedId, diff --git a/packages/builder/src/locking.test.ts b/packages/builder/src/locking.test.ts new file mode 100644 index 0000000000..db1763b75b --- /dev/null +++ b/packages/builder/src/locking.test.ts @@ -0,0 +1,120 @@ +/** + * Whether the editor may move or delete a block the author has locked. + * + * The fixture that separates a correct rule from a plausible one is a container + * holding a locked CHILD: a rule that checks only the node passes every + * single-block case and loses the locked child the moment someone deletes what + * it sits in. + * + * @module locking.test + */ +import { describe, expect, it } from "vitest"; + +import type { BlockDocument, BlockNode } from "@nextlyhq/blocks-engine"; + +import { isLocked, lockBlockingDelete, lockBlockingMove } from "./locking"; + +function node( + id: string, + extra: Partial = {}, + children?: BlockNode[] +): BlockNode { + return { + id, + type: "acme/block", + version: 1, + props: {}, + ...(children ? { slots: { children } } : {}), + ...extra, + } as BlockNode; +} + +function documentOf(nodes: BlockNode[]): BlockDocument { + return { formatVersion: 1, kind: "page", nodes } as BlockDocument; +} + +/** A section holding a locked caption, plus an unlocked sibling. */ +function withLockedChild(): BlockDocument { + return documentOf([ + node("section", {}, [node("caption", { locked: true })]), + node("loose"), + ]); +} + +describe("isLocked", () => { + it("reads the flag, and treats absent as unlocked", () => { + // `locked` is optional, so most nodes never carry it. Reading a missing + // field as locked would freeze an entire document written before the flag + // existed. + expect(isLocked(node("a", { locked: true }))).toBe(true); + expect(isLocked(node("b", { locked: false }))).toBe(false); + expect(isLocked(node("c"))).toBe(false); + }); +}); + +describe("lockBlockingMove", () => { + it("refuses to move the locked node itself", () => { + expect(lockBlockingMove(withLockedChild(), "caption")?.id).toBe("caption"); + }); + + it("ALLOWS moving a container that holds a locked child", () => { + // The separating case, and the direction that is easy to get wrong by being + // cautious. Moving the section leaves the caption in the same slot at the + // same index with the same neighbours, so nothing the lock protects has + // changed — and refusing would let one locked caption freeze the whole + // section around it. + expect(lockBlockingMove(withLockedChild(), "section")).toBeUndefined(); + }); + + it("allows an ordinary node, and reports nothing for an id the document lost", () => { + // The control: without it, "returns undefined" would be satisfied by a + // function that always does. + expect(lockBlockingMove(withLockedChild(), "loose")).toBeUndefined(); + expect(lockBlockingMove(withLockedChild(), "gone")).toBeUndefined(); + }); +}); + +describe("lockBlockingDelete", () => { + it("refuses to delete the locked node itself", () => { + expect(lockBlockingDelete(withLockedChild(), "caption")?.id).toBe( + "caption" + ); + }); + + it("REFUSES to delete a container that holds a locked child", () => { + // THE case. Deleting the section destroys the caption, which is the one + // outcome the flag exists to prevent — and it would happen through an + // action aimed at something else. + expect(lockBlockingDelete(withLockedChild(), "section")?.id).toBe( + "caption" + ); + }); + + it("finds a lock nested several levels down", () => { + const document = documentOf([ + node("outer", {}, [node("middle", {}, [node("deep", { locked: true })])]), + ]); + + expect(lockBlockingDelete(document, "outer")?.id).toBe("deep"); + }); + + it("names the OUTERMOST lock when a subtree holds more than one", () => { + // The refusal has to name something an author can act on, and the deepest + // match is the least likely to be the one they meant. + const document = documentOf([ + node("outer", {}, [ + node("middle", { locked: true }, [node("deep", { locked: true })]), + ]), + ]); + + expect(lockBlockingDelete(document, "outer")?.id).toBe("middle"); + }); + + it("allows deleting a container whose children are all unlocked", () => { + // The control for every refusal above: a rule that refused everything would + // satisfy them all. + const document = documentOf([node("outer", {}, [node("child")])]); + + expect(lockBlockingDelete(document, "outer")).toBeUndefined(); + }); +}); diff --git a/packages/builder/src/locking.ts b/packages/builder/src/locking.ts new file mode 100644 index 0000000000..17105373cf --- /dev/null +++ b/packages/builder/src/locking.ts @@ -0,0 +1,90 @@ +/** + * Whether the editor may move or delete a block the author has locked. + * + * `BlockNode.locked` is described by the engine as an author-facing policy + * flag: *"while true, the editor command layer must not let the author move or + * delete this node"*, and — the half that decides where this module lives — + * *"system transforms (migrations, overlays, restore) still operate on locked + * nodes, and the pure tree primitives do not read it."* + * + * So the check belongs to the COMMAND layer and nowhere below it. Putting it in + * `applyOp` would read correctly and be wrong: a restore, a migration or an + * agent replaying history would start refusing ops on locked nodes, and a lock + * an author set to protect their own editing would begin blocking the system's + * own writes. + * + * **One predicate, because the command layer is several places.** Delete, the + * keyboard moves and the canvas drag each have to ask, and three copies of "is + * this locked" agree until one of them gains the subtree rule below. + * + * ## Moving checks the node; deleting checks the subtree + * + * They are different questions and the difference is not a nicety. + * + * Moving a container that holds a locked child leaves that child exactly where + * it was relative to its parent — same slot, same index, same neighbours — so + * the lock is not violated and refusing would make a locked caption freeze the + * whole section around it. + * + * Deleting that container DESTROYS the child. An author who locked a block and + * then removed its container would lose the thing they locked, which is the one + * outcome the flag exists to prevent — and it would happen through an action + * aimed at something else, with the count in the announcement as the only clue. + * + * @module locking + */ + +import { + findNode, + type BlockDocument, + type BlockNode, +} from "@nextlyhq/blocks-engine"; + +/** Whether this node itself carries the author's lock. */ +export function isLocked(node: BlockNode): boolean { + return node.locked === true; +} + +/** + * The locked node that stops this one being MOVED, or `undefined`. + * + * Only the node itself, for the reason in the module docblock. + */ +export function lockBlockingMove( + document: BlockDocument, + id: string +): BlockNode | undefined { + const node = findNode(document.nodes, id); + if (node === undefined) return undefined; + return isLocked(node) ? node : undefined; +} + +/** The first locked node in a subtree, walking outermost first. */ +function firstLockedIn(node: BlockNode): BlockNode | undefined { + if (isLocked(node)) return node; + for (const children of Object.values(node.slots ?? {})) { + for (const child of children) { + const found = firstLockedIn(child); + if (found !== undefined) return found; + } + } + return undefined; +} + +/** + * The locked node that stops this one being DELETED, or `undefined`. + * + * The node or anything inside it. Returns the OUTERMOST locked node rather than + * merely reporting that one exists, because the refusal has to name something + * an author can act on — "this section holds a locked block" is followed by + * going and finding it, and the deepest match is the least likely to be the one + * they meant. + */ +export function lockBlockingDelete( + document: BlockDocument, + id: string +): BlockNode | undefined { + const node = findNode(document.nodes, id); + if (node === undefined) return undefined; + return firstLockedIn(node); +}