From 37820249b7014d3cae792061d5d24580b1fb205f Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Tue, 18 Aug 2026 12:36:19 +0500 Subject: [PATCH] feat(builder): honour a block's lock in every command, not only in drag MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The engine defines `locked` as "the editor command layer must not let the author move or delete this node", and until now only the canvas drag read it. The layers panel shipped a Locked badge yesterday, so the editor was showing a lock it did not keep: the same block moved with alt+Arrow and deleted with Delete. The rule is asked in one place because the command layer is several, and because the two commands do NOT ask the same question. Moving checks the node alone. A container that holds a locked child leaves that child 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. Deleting checks the subtree. Removing a container destroys what is inside it, so an author who locked a caption and then deleted its section would lose the thing they locked, through an action aimed at something else, with the descendant count as the only clue. The refusal names the outermost locked node, because "this section holds a locked block" is followed by going to find it. Refusals are ANNOUNCED, where the existing structural ones stay silent. That distinction is the existing reasoning applied rather than overturned: "this block is already first" is noise an author cannot act on and the block not moving is itself the answer, while nothing on the page explains a lock, the remedy is one the author can act on, and a keyboard user has no badge to see. The check stays out of `applyOp`. The engine is explicit that the pure tree primitives do not read the flag and that system transforms — migrations, overlays, restore — still operate on locked nodes, so enforcing it there would make an author's editing lock start refusing the system's own writes. --- .../locked-blocks-are-actually-locked.md | 34 ++++ packages/builder/src/canvas-drag.tsx | 9 +- packages/builder/src/index.ts | 11 ++ .../builder/src/keyboard-actions.test.tsx | 146 ++++++++++++++++++ packages/builder/src/keyboard-actions.tsx | 38 +++++ packages/builder/src/locking.test.ts | 120 ++++++++++++++ packages/builder/src/locking.ts | 90 +++++++++++ 7 files changed, 447 insertions(+), 1 deletion(-) create mode 100644 .changeset/locked-blocks-are-actually-locked.md create mode 100644 packages/builder/src/locking.test.ts create mode 100644 packages/builder/src/locking.ts 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); +}