Repository navigation
feat(builder): honour a block's lock in every command, not only in drag - #1016
Merged
Merged
Conversation
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.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Collaborator
Author
|
@codex please review this PR |
|
Warning Review limit reached
Next review available in: 44 minutes Limit details: You’ve used all 1 included review currently available under your plan. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Greptile SummaryThe PR centralizes lock-policy queries and applies them to canvas drag and keyboard move/delete commands, including subtree-aware deletion and accessible refusal announcements.
Confidence Score: 4/5The container-move contract must be reconciled with applyOp before merging, because unlocked containers with locked descendants still cannot be moved. The command layer intentionally allows an unlocked container to move, but the existing operation layer rejects the same move after finding its locked descendant, leaving keyboard and drag behavior inconsistent with the feature’s stated contract. Files Needing Attention: packages/builder/src/locking.ts, packages/builder/src/ops.ts, packages/builder/src/keyboard-actions.test.tsx
|
| Filename | Overview |
|---|---|
| packages/builder/src/locking.ts | Adds centralized node-only move and subtree-aware delete predicates, but the move semantics conflict with applyOp’s existing subtree-wide enforcement. |
| packages/builder/src/keyboard-actions.tsx | Announces lock refusals for keyboard move/delete, although the advertised container-move path is still rejected downstream. |
| packages/builder/src/canvas-drag.tsx | Replaces the inline drag lock read with the shared move predicate, inheriting the downstream semantic mismatch. |
| packages/builder/src/index.ts | Safely exports the new plain locking helpers from the existing server-safe root entry. |
| packages/builder/src/locking.test.ts | Tests the intended move/delete asymmetry at the predicate level but does not cover integration with applyOp. |
| packages/builder/src/keyboard-actions.test.tsx | Uses an apply spy that accepts container moves, so it misses the real store’s subtree-wide rejection. |
| .changeset/locked-blocks-are-actually-locked.md | Provides the repository-required all-package patch changeset and describes the intended lock behavior. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Move unlocked container with locked child] --> B[lockBlockingMove checks container only]
B -->|Allowed| C[Keyboard or drag calls editor.apply]
C --> D[applyOp lockedWithin scans subtree]
D -->|Locked child found| E[Move rejected]
E --> F[Keyboard silently does not move / drag cannot commit]
Prompt To Fix All With AI
### Issue 1
packages/builder/src/locking.ts:57-59
**Subtree lock still blocks moves**
When an unlocked container has a locked descendant, `lockBlockingMove` permits the move, but the existing subtree-wide check in `applyOp` rejects it, causing keyboard movement to become a silent no-op and preventing drag from committing the move.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "feat(builder): honour a block's lock in ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+57
to
+59
| const node = findNode(document.nodes, id); | ||
| if (node === undefined) return undefined; | ||
| return isLocked(node) ? node : undefined; |
There was a problem hiding this comment.
When an unlocked container has a locked descendant, lockBlockingMove permits the move, but the existing subtree-wide check in applyOp rejects it, causing keyboard movement to become a silent no-op and preventing drag from committing the move.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/builder/src/locking.ts
Line: 57-59
Comment:
**Subtree lock still blocks moves**
When an unlocked container has a locked descendant, `lockBlockingMove` permits the move, but the existing subtree-wide check in `applyOp` rejects it, causing keyboard movement to become a silent no-op and preventing drag from committing the move.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Contributor
Fallow audit reportFound 40 findings. Details
Generated by fallow. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A lock the editor showed and did not keep. The engine defines
lockedas "the editor command layer must not let the author move or delete this node". Only the canvas drag read it. #1013 shipped a Locked badge in the layers panel yesterday — so the editor advertised a lock whilealt+Arrowmoved the block andDeleteremoved it.That inconsistency is mine, from #1013, and it is worse than not showing the badge. This is the fix.
Move and delete do not ask the same question
Moving checks the node alone. A container holding a locked child leaves that child in the same slot, at the same index, with the same neighbours — nothing the lock protects has changed. Refusing would let one locked caption freeze the whole section around it, which an author experiences as the editor breaking rather than as a lock working.
Deleting checks the subtree. Removing a container destroys what is inside it. 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, and the deepest match is the least likely to be the one they meant.
One predicate, because the command layer is three places and three inline reads of
node.lockedagree right up until one of them gains that distinction. The canvas drag now asks it too, rather than reading the flag itself.The check stays out of
applyOp, deliberatelyThe engine is explicit: "system transforms (migrations, overlays, restore) still operate on locked nodes, and the pure tree primitives do not read it." Enforcing there would read correctly and be wrong — an author's editing lock would start refusing the system's own writes.
Refusals are announced; the existing silent ones stay silent
That is the existing reasoning applied, not overturned. The move handler already says refusals stay quiet because "'this block is already first' on every press at the end of a list is noise an author cannot act on, and the block not moving is itself the answer." A lock is the opposite on every clause: nothing on the page explains it, the remedy is one the author can act on, and a keyboard user has no badge to look at.
Structural refusals silent, policy refusals announced.
Verified end to end, with a genuinely locked block
Nothing in the UI sets
lockedyet, so the document was seeded through the API the way an import or an agent would.Unlocked editing re-verified in the same browser and unaffected: click selects,
alt+Arrowreorders and announces "Block moved",Deleteremoves and announces the undo path.Tests
620 in
@nextlyhq/builder(was 615), 61 inplugin-page-builder, 30/30 tasks. Four stub-verifications, including both directions of the asymmetry — making delete node-only fails 4 tests, making move subtree-wide fails the two that assert a container may still move.Not in this PR
No lock toggle. Nothing can set
lockedfrom the UI yet, and the natural home is a question worth its own decision: a button inside arole="treeitem"row complicates the APG keyboard model thatTreeViewowns, and the inspector is currently built aroundprops/PropSchemarather than node-level fields. Enforcement is complete on its own — it makes the badge honest for every document an import, an agent or the API can already produce — and the toggle is a UI decision rather than a correctness one.One unexplained observation, recorded rather than smoothed over
In one probe run,
alt+Downon a container selected via the breadcrumb neither moved it nor announced anything. A focused probe afterwards could not reproduce it: keyboard moves fire correctly after both a canvas click and a crumb click, in both selection paths. I have no mechanism for the single observation and am not claiming one.