feat(builder): drag blocks on the canvas - #1007
Merged
Merged
Conversation
Adds the drop rules, the pointer gesture and the indicator that draws it. Region first, then a line. Three earlier designs scored every candidate position against the pointer and were refuted, because scoring is comparative and no measure of one position can encode "the pointer is inside THIS container": containment is a statement about the candidates being compared against. Resolving which region owns the pointer BEFORE ranking anything makes every value in a comparison the same unit by construction. An insertion point is a line rather than a rectangle's middle. Ranking by distance to the line puts the switch boundary at each child's centre whatever the children's sizes are, which is what a rule measuring to a zone's middle gets wrong once adjacent blocks differ in height. Nothing reads a block's height as a threshold. `core/spacer` takes its height from an author-set prop with no lower bound and `core/divider` renders one pixel tall, so any minimum size excludes some authored block and makes it impossible to drop beside. Steadiness comes from pointer travel instead, which `target-switch` already decides and which had no consumer until now. The canvas root gains `min-height: 100%`: it was exactly as tall as its content, so the end of the page had no pixels to aim at and a block could not be dragged there at all. The DOM reads move into `geometry-dom`, beside the frame inset, so rectangles still enter this package through one door; the ownership guard allows that second path and now exempts test files, which must assign a rectangle because jsdom lays nothing out.
|
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: 31 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 (14)
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 adds pointer-based block reordering and container drops to the builder canvas, using region-first target resolution and insertion-line indicators.
Confidence Score: 4/5The same-list forward-drag indexing defect should be fixed before merging because blocks can land after a different sibling than the displayed insertion line indicates. Drop targets are generated from the original sibling list, while the tree mover removes the dragged node before interpreting the destination index, shifting every forward destination by one. Files Needing Attention: packages/builder/src/drop-targets.ts, packages/builder/src/canvas-drag.tsx
|
| Filename | Overview |
|---|---|
| packages/builder/src/canvas-drag.tsx | Implements gesture lifecycle and commits resolved targets, but forwards target indexes unchanged to post-removal move semantics. |
| packages/builder/src/drop-targets.ts | Adds region-first drop resolution and line ranking; same-list forward targets retain pre-removal indexes and therefore land late. |
| packages/builder/src/geometry-dom.ts | Adds consistent viewport-to-canvas coordinate conversion for rectangles and pointer positions. |
| packages/plugin-page-builder/src/admin/BlocksField.tsx | Wires the drag hook, shared registry sources, handlers, and indicator into the production builder canvas. |
| packages/builder/src/inserter.ts | Centralizes block-name nesting checks and registry slot discovery for palette and drag consumers. |
Sequence Diagram
sequenceDiagram
participant User
participant Canvas
participant Drag as useCanvasDrag
participant Resolver as resolveDrop
participant Editor
User->>Canvas: pointer down on block
Canvas->>Drag: snapshot document regions and rectangles
User->>Canvas: pointer move
Canvas->>Drag: current pointer coordinates
Drag->>Resolver: regions, nesting rules, pointer
Resolver-->>Drag: insertion target and line
Drag-->>Canvas: render DropIndicator
User->>Canvas: pointer up
Canvas->>Drag: commit target
Drag->>Editor: apply move operation
Prompt To Fix All With AI
### Issue 1
packages/builder/src/drop-targets.ts:349-357
**Forward drops use shifted indexes**
When a block is dragged forward within its current root or slot, `targetsInRegion` computes the target index from the list that still contains the moving block, but `moveNode` interprets that index after removing it. The block therefore lands one position after the displayed insertion line—for example, dropping A between C and D in `[A,B,C,D]` produces `[B,C,D,A]`.
---
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): drag blocks on the canvas" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+349
to
+357
| const positionAt = (index: number): OpPosition => | ||
| region.parentId === undefined || region.slot === undefined | ||
| ? { index } | ||
| : { parentId: region.parentId, slot: region.slot, index }; | ||
|
|
||
| const make = (index: number, line: number): DropTarget => ({ | ||
| id: `${region.id}#${String(index)}`, | ||
| regionId: region.id, | ||
| at: positionAt(index), |
There was a problem hiding this comment.
When a block is dragged forward within its current root or slot, targetsInRegion computes the target index from the list that still contains the moving block, but moveNode interprets that index after removing it. The block therefore lands one position after the displayed insertion line—for example, dropping A between C and D in [A,B,C,D] produces [B,C,D,A].
Knowledge Base Used: Page Builder plugin
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/builder/src/drop-targets.ts
Line: 349-357
Comment:
**Forward drops use shifted indexes**
When a block is dragged forward within its current root or slot, `targetsInRegion` computes the target index from the list that still contains the moving block, but `moveNode` interprets that index after removing it. The block therefore lands one position after the displayed insertion line—for example, dropping A between C and D in `[A,B,C,D]` produces `[B,C,D,A]`.
**Knowledge Base Used:** [Page Builder plugin](https://app.greptile.com/nextly/-/custom-context/knowledge-base/nextlyhq/nextly/-/docs/plugin-page-builder.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Contributor
Fallow audit reportFound 41 findings. Dependencies (40)
Health (1)
Generated by fallow. |
github-actions
Bot
added
type: docs
This was referenced Aug 18, 2026
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.
Blocks can now be dragged on the page-builder canvas. Press a block, move, and a line shows where it will land; release and it goes there.
The design, and why it is not the one that was tried before
Three earlier designs for this were built and refuted by measurement (#810 and #829, both closed). Each scored every candidate position against the pointer and took the best score. Scoring is comparative, so once two positions in different containers can tie, their scores have to mean the same thing — and no measure of a single position can encode "the pointer is inside THIS container", because containment is a statement about the candidates you are being compared against. That is an arity problem, not an arithmetic one.
drop-targets.tsstops comparing across containers at all:Every value in a comparison is then the same unit by construction.
An insertion point is a LINE, not a rectangle's middle. Ranking by distance to the line puts the switch boundary exactly at each child's centre, whatever the children's sizes are — which is what a rule measuring to a drop zone's middle gets wrong as soon as adjacent blocks differ in height.
drop-targets.test.tsasserts that on a 300px block beside a 20px one; equal heights pass either way.Nothing reads a block's height as a threshold.
core/spacertakes its height from an author-set prop with no lower bound andcore/dividerrenders 1px (measured on the canvas, not inferred), so any minimum size makes some authored block impossible to drop beside. Steadiness comes from pointer travel instead —target-switch.ts, which was merged, tested, and had no consumer until now.Accessibility
WCAG 2.2 SC 2.5.7 requires any drag-operated function to be achievable without dragging, and SC 2.1.1 requires keyboard operability. Both were already satisfied before this change: click to select,
alt+Arrowto move, each move announced. So drag is an enhancement over an existing baseline, and it adds no second live region —keyboard-actionsowns the one there is.Verified in a real browser, not only in tests
Every run included an auth control, because a 401 renders a healthy-looking admin whose field controls never load.
H2,P,HR→P,HR,H2, twice, identicalOne fix came out of that and could not have come from anywhere else: the canvas root was exactly as tall as its content, so the end of the page had no pixels to aim at and a block could not be dragged there at all.
min-height: 100%.Known limitation, stated rather than left to be found
An empty container renders ~2px tall, so a pointer cannot realistically aim into one. Insert-into-empty-container (#1001) is unaffected — it works from the selection, not the pointer. Dragging into a container that already has content works, and is verified above. Making empty containers an aimable drop area needs either a renderer marker for "this node declares slots" or a drag-time reflow, and both are decisions worth their own change.
Also here
blockAllowedAt— the nesting rule by block NAME, so the palette and the drag ask one question rather than two.entryAllowedAtdelegates to it.registrySlotSourcemoves intoinserter.ts; the palette and the canvas now read the same slot source, so a container the palette will fill is one a drag can aim at.geometry-dom.ts, beside the frame inset, so rectangles still enter this package through one door. The ownership guard allows that second path explicitly and now exempts test files — which must assign a rectangle, because jsdom lays nothing out.target-switchand the drop rules are exported from the root entry. Neither was exported from any entry before, sotarget-switchnever reacheddistat all.Tests
573 in
@nextlyhq/builder(was 549), 61 inplugin-page-builder. Every new mechanism was stub-verified — broken one at a time, confirmed the intended test failed, restored, restore confirmed by diff. That is 6 breaks for the drop rules, 7 for the gesture, 3 for the coordinate readers, plus a positive control proving the loosened ownership guard still catches a product-file read.