Repository navigation
feat(builder): a click inside a component selects the instance - #1724
Conversation
A component is inlined at render, so every element of the definition's tree
carries an id re-minted during composition — one the page's stored document does
not contain. `nodeIdFromEvent` returned that id and `onSelect` was handed an
address it cannot select, edit or delete. The drag path bailed instead, because
`findNode` could not resolve it either, so dragging inside a component already
did nothing by accident.
The walk now asks the element it lands on whether the resolver marked it, and
answers with the host instance when it did.
ASKING THE ELEMENT rather than its ancestors is the design, not a detail.
`owner.closest("[data-nx-instance]")` is the obvious implementation and it is
wrong: an instance's slot content belongs to the page and is deliberately
unmarked, but it renders NESTED INSIDE the definition's marked box, so a walk
upwards finds that box and hands back the component for a node the author can
and should select directly — which is exactly the content a marketer opened the
editor to change. The marker is per-node rather than a wrapper element precisely
so the question can be answered without walking, and `getAttribute` on the
element is what answers it. Break-verified against the `closest()` form, which
fails the slot-content case.
An empty marker reads as page-owned. The renderer never emits one — it treats an
empty id as no provenance — but an address of "" is not selectable, and reading
it as page-owned leaves the element addressable by its own id rather than by
nothing.
`nodeElement` answers the same addresses, because the two are one mapping read
in two directions. An instance id names a node that renders no element of its
own, so a caller measuring the selection is pointed at the first element the
definition contributed — the outermost, in document order. A node's own element
still wins where both exist; they cannot collide today, and stating the
precedence keeps it from being iteration order.
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Warning Review limit reachedNext included review available in 41 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d0e89d68dd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // canvas rendered within another rendered page would otherwise resolve a | ||
| // click to the outer page's node. | ||
| if (owner.closest(`.${CANVAS_ROOT_CLASS}`) === null) return null; | ||
| return addressOf(owner); |
There was a problem hiding this comment.
When a definition-owned element is right-clicked or touch-long-pressed, this translation is bypassed: both the canvas context handler and BlockContextMenu.noteTarget call contextMenuTargetOf, which still returns the re-minted NODE_ID_ATTRIBUTE value from canvas.tsx:164-184. Since that ID is absent from the stored document, applySelection retains the previous selection and the menu can execute destructive actions such as Delete against that previous block rather than the component under the pointer. Derive the context-menu target from the same instance-aware address used here.
AGENTS.md reference: AGENTS.md:L288-L291
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed, reproduced and fixed at eacbc7134. This is the sharpest of the three because the consequence is destructive.
You are right on the path: contextMenuTargetOf returned the raw NODE_ID_ATTRIBUTE, applySelection ignores an id the stored document does not contain, so the PREVIOUS selection stood — and the menu's verbs, Delete among them, acted on a block nobody was pointing at.
The finding is really about a partial migration, and the fix is the general one rather than this call site. I translated the click path and the geometry lookup and left three other readers of the same element-to-node mapping on the raw attribute: this one, the selection markers, and the drag rectangles. Each is its own defect and they have one cause.
So nodeAddressOf is exported and all three ask it. Patching contextMenuTargetOf alone would have left the same shape waiting in the other two.
One rule fell out of doing all three at once, and it is the part worth reviewing. An address sits on several elements for two reasons that need OPPOSITE answers:
- a node a block renders more than once puts one id on SIBLING elements, and every copy is a rendering of that node — an existing test pins that marking one outlines a single row of ten;
- a component instance puts one address on every element its definition contributed, NESTED, and those are renderings of different definition nodes that merely share a host.
My first attempt used "first in document order" and broke the first case; the suite caught it. isOutermostForAddress asks about ENCLOSURE instead, which separates them exactly. The host marker is contiguous over a subtree, so the nearest enclosing one carrying the same value is the whole test.
Break-verified: restoring the raw attribute here fails the new context-menu case while the page-owned control stays green.
| if (hosted === null && element.getAttribute(INSTANCE_ATTRIBUTE) === id) { | ||
| hosted = element; |
There was a problem hiding this comment.
When the new hit test selects an instance ID, useCanvasMarkers still compares the selection only with each element's re-minted NODE_ID_ATTRIBUTE at canvas.tsx:879-903. Consequently no definition-owned element receives data-nx-selected, so the component has no selection outline and forced interaction states—which are derived from the primary selection marker—are not shown. The marker walk needs to derive its target from the same instance-aware address rather than maintaining the old node-ID-only mapping.
AGENTS.md reference: AGENTS.md:L288-L291
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed at eacbc7134, as part of putting every reader of the element-to-node mapping on one rule — this was one of three left on the raw attribute.
The marker walk compared each element's NODE_ID_ATTRIBUTE against the selection, so a definition-owned element never matched an instance id: no data-nx-selected, therefore no outline, and no forced interaction state, since those derive from the same marker.
What I got wrong on the first attempt is worth reporting. Marking every element that carries the address seemed obviously right, so I deduplicated to one per address by taking the first in document order. That broke an existing test: a block that renders its child twice puts one id on two SIBLING elements, and BOTH must be marked — marking one outlines a single row of ten while previewing the state on another.
So "one element per address" and "every rendering of the node" are both real requirements, and they are not in conflict once the right question is asked. It is ENCLOSURE:
- sibling copies of one node — neither encloses the other, both outermost, both marked;
- an instance's inlined elements — nested under the definition's box, so only the box is marked.
isOutermostForAddress asks exactly that, and the host marker being contiguous over a subtree means the nearest enclosing one carrying the same value is the whole test — no walk to the root.
The same helper keys the drag rectangles, so the outline and the geometry describe one box rather than two elements chosen by different rules.
| // that as no provenance — but an address of "" is not selectable, so reading | ||
| // it as page-owned is the answer that leaves the element addressable by its | ||
| // own id. | ||
| if (instance !== null && instance !== "") return instance; |
There was a problem hiding this comment.
When a resolved component is dragged, useCanvasDrag now receives the host instance ID from this helper and gets past its previous findNode bailout, but snapshotRects in canvas-drag.tsx:387-408 still keys every rectangle only by the re-minted data-nx-node value. collectRegions subsequently requests rectangles by stored instance ID, so on a page made of component instances every child is treated as unrendered and targetsInRegion collapses the root to a single index-0 midpoint; releasing an activated drag can therefore move the component to the start regardless of where it was dropped. Derive the rectangle keys from the same instance-aware address before enabling this drag path.
AGENTS.md reference: AGENTS.md:L288-L291
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed at eacbc7134. This is the one I most needed pointed out, because this branch is what made it reachable — it is a regression I introduced rather than a defect I merely failed to fix.
Your chain is exact. snapshotRects keyed every rectangle by the raw data-nx-node, collectRegions asks by the address, so on a page of component instances every child measured as unrendered; targetsInRegion then collapsed the root to a single index-0 midpoint and releasing an activated drag moved the component to the start whatever was under the pointer.
It could not happen before this branch. The drag bailed earlier, because findNode could not resolve the re-minted id — the PR description even noted that dragging inside a component "already did nothing, by accident rather than by design". Getting the hit test past that bailout is precisely what exposed the rect indexing, so the fix that enabled the path owed this one and did not pay it.
Keys come from nodeAddressOf now, with the same outermost rule the selection markers use, so the drag geometry and the outline describe one box.
The outermost rule is doing real work here rather than being tidy. The previous keying was last-wins by iteration, so an instance would have measured whichever element its definition contributed LAST — an inner leaf. Taking the outermost is what makes the rectangle the component's own. Sibling copies of a node a block rendered twice are each outermost, so that case keeps the last-wins behaviour it had.
I have not added a drag-level regression test for the collapse itself, and I would rather say so than imply the coverage is wider than it is: the new tests pin the addressing and the enclosure rule, which is the cause, but not targetsInRegion's behaviour end to end. If you would rather see that pinned before this lands, say so and I will build the fixture.
@nextlyhq/adapter-drizzle
@nextlyhq/adapter-mysql
@nextlyhq/adapter-postgres
@nextlyhq/adapter-sqlite
@nextlyhq/admin
@nextlyhq/admin-css
@nextlyhq/blocks-engine
@nextlyhq/blocks-react
@nextlyhq/builder
create-nextly-app
@nextlyhq/eslint-plugin
nextly
@nextlyhq/plugin-form-builder
@nextlyhq/plugin-page-builder
@nextlyhq/plugin-sdk
@nextlyhq/plugin-seo
@nextlyhq/storage-s3
@nextlyhq/storage-uploadthing
@nextlyhq/storage-vercel-blob
@nextlyhq/ui
commit: |
The first version translated the click path and the geometry lookup and left three other readers on the raw attribute. Each is a way a definition-owned element addresses nothing. The CONTEXT MENU returned the re-minted id, which `applySelection` ignores because the stored document does not contain it — so a right-click inside a component left the PREVIOUS selection in place and the menu's verbs acted on a block nobody pointed at. Delete among them. The SELECTION MARKERS compared each element against the raw attribute, so a selected component received no marker at all: no outline, and no forced interaction state, both of which derive from it. The DRAG RECTANGLES were keyed by the raw attribute while `collectRegions` asks by the address, so every child of a component measured as unrendered and `targetsInRegion` collapsed the root to one index-0 midpoint — releasing a drag moved the component to the start whatever was under the pointer. Reachable only because this branch got the drag past its old `findNode` bailout, so making that path work is what exposed it. `nodeAddressOf` is exported for them, and one more rule came out of it. An address can sit on several elements for two reasons that need OPPOSITE answers. A node a block renders more than once puts one id on SIBLING elements, and every copy is a rendering of that node — marking one outlines a single row of ten, which an existing test pins and which the first attempt here broke. A component instance puts one address on every element its definition contributed, NESTED, and those are renderings of different definition nodes that merely share a host — marking all of them outlines everything inside the component, and measuring all of them keys the geometry to whichever was visited last. Enclosure separates them, so `isOutermostForAddress` asks exactly that. The host marker is contiguous over a subtree, so the nearest enclosing one carrying the same value is the whole test.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eacbc71340
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (id !== null && isOutermostForAddress(element, id)) { | ||
| measured.set(id, canvasContentRect(element, root)); |
There was a problem hiding this comment.
When a component definition has multiple top-level nodes, every root is a sibling and therefore passes isOutermostForAddress; this assignment then overwrites the same instance ID for each root and retains only the last root's rectangle. Multi-root definitions are explicitly supported by resolveComponentInstances, so collectRegions will order drops against only that final element rather than the component's full rendered extent, allowing a drop near an earlier root to resolve to the wrong before/after index. Aggregate the outermost rectangles belonging to an instance instead of using last-write-wins.
Useful? React with 👍 / 👎.
| // The ADDRESS, not the raw attribute. A definition-owned element | ||
| // carries a re-minted id the selection never holds, so comparing the | ||
| // attribute leaves a selected component with no outline at all. | ||
| const id = nodeAddressOf(element); |
There was a problem hiding this comment.
When a definition-owned component is locked or becomes the active drag source, this translated instance address flows into lockedIds and draggingId, but the immediately following drag-marker walk at lines 972-982 recomputes the raw NODE_ID_ATTRIBUTE. Because that re-minted ID never equals the stored instance ID, locked components still advertise the grab cursor and dragged components never receive the dimming marker. Use nodeAddressOf (with the same outermost policy) for those markers as well.
AGENTS.md reference: AGENTS.md:L288-L291
Useful? React with 👍 / 👎.
| // The node's OWN element wins where both exist. They cannot today — an | ||
| // instance is replaced rather than rendered — but the precedence states which | ||
| // answer is the node's rather than leaving it to iteration order. | ||
| return own ?? hosted; |
There was a problem hiding this comment.
When an outer canvas contains a block that renders another canvas, IDs are unique only within each document, as ownedNodeElement below already notes. If the inner document has an ordinary node whose ID equals the selected outer component instance ID, this unscoped scan populates own with that foreign element and own ?? hosted chooses it over the correct instance rendering regardless of document order. BlockToolbar and SpacingOverlay consequently measure and position themselves against the inner canvas; restrict both matches with the existing canvas-ownership predicate.
Useful? React with 👍 / 👎.
| * EXPORTED because it is the one rule, and every reader of the element-to-node | ||
| * mapping has to use it. Slice one of this work updated the click path and the | ||
| * geometry lookup and left three other readers on the raw attribute — the | ||
| * context menu, the selection markers and the drag rectangles — which is how a | ||
| * right-click came to act on the previously selected block. |
There was a problem hiding this comment.
Rewrite this documentation to state only the current element-to-node invariant: describing what “Slice one of this work” updated and which readers it previously left behind records task and review history in a production-code comment, contrary to the repository's explicit requirement that comments describe code only and never reference tasks, plans, conversations, or review findings. The similar historical explanation in snapshotRects should be removed at the same time.
AGENTS.md reference: AGENTS.md:L277-L279
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50a765e214
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // The ADDRESS, not the raw attribute. A definition-owned element | ||
| // carries a re-minted id the selection never holds, so comparing the | ||
| // attribute leaves a selected component with no outline at all. | ||
| const id = nodeAddressOf(element); |
There was a problem hiding this comment.
When an outer canvas selects a component whose instance ID is also used by a component in a nested canvas, touched includes the nested canvas elements and this translated address makes the inner component appear selected. Unlike the drag-marker pass below, this pass never calls ownedByCanvas, so the outer effect can add a primary outline and forced interaction state to a block belonging to another document. Filter by canvas ownership before comparing addresses.
AGENTS.md reference: AGENTS.md:L288-L291
Useful? React with 👍 / 👎.
| const host = element.parentElement?.closest(`[${INSTANCE_ATTRIBUTE}]`); | ||
| if (host === null || host === undefined) return true; | ||
| return host.getAttribute(INSTANCE_ATTRIBUTE) !== address; |
There was a problem hiding this comment.
When a canvas is rendered inside a definition-owned element and its document reuses the enclosing component's instance ID—which is valid because IDs are document-scoped—this unbounded closest escapes the inner canvas and finds the outer document's marker. The inner component is then classified as non-outermost, so it receives no selection marker and snapshotRects omits its rectangle, degrading drag placement. Stop the ancestor search at the component's owning canvas.
AGENTS.md reference: AGENTS.md:L288-L291
Useful? React with 👍 / 👎.
| // canvas rendered within another rendered page would otherwise resolve a | ||
| // click to the outer page's node. | ||
| if (owner.closest(`.${CANVAS_ROOT_CLASS}`) === null) return null; | ||
| return nodeAddressOf(owner); |
There was a problem hiding this comment.
When this hit test selects a definition-owned element, it now returns the stored instance ID, but useCanvasReading still reaches markedElementOf, which compares that ID only with raw NODE_ID_ATTRIBUTE values. Because an inlined instance has no element carrying its own ID there, the style inspector permanently reads both its rendered tag and orientation as unknown, losing tag-dependent provenance and the correctly oriented logical-side editor. Route that reverse lookup through the same instance-aware element mapping.
AGENTS.md reference: AGENTS.md:L288-L291
Useful? React with 👍 / 👎.
… the one rule Three ways a nested canvas — the shape component edit mode produces, a canvas rendered inside a definition-owned element — could be read wrongly by the canvas around it. Ids are scoped per document, so the inner document may hold an instance with the same address as one selected outside, and each defect is that address crossing the boundary. The selection pass marked every element the outer container could query, including the inner canvas's, and unlike the drag pass it never asked which canvas an element belonged to. It could outline a block in a document the selection is not in, and on a later pass clear a mark the inner canvas had set. It now leaves foreign canvases alone, as the drag pass already did. The outermost lookup searched ancestors without bound, so from inside the inner canvas it found the OUTER document's instance marker, classified the inner component as nested inside itself, and left it with no outline and no drag rectangle. The search now stops at the canvas that owns the element. And the style inspector's reverse lookup — which element a selected node is drawn as — read the node-id attribute directly, so for an instance, which renders no element carrying its own id, it found nothing: tag and orientation read as unknown for as long as a component was selected. That lookup was a sixth reader of the element-to-node mapping beside the five that already go through `nodeElement`; it is now an adapter over that one rule rather than a second search. Each case break-verified by restoring the wrong implementation.
|
All three reproduced and fixed in c19dd26, each break-verified by restoring the wrong implementation. Selection pass over foreign canvases — confirmed, and it was worse than marking: on a later pass it would also clear a mark the inner canvas had set. It now skips elements another canvas owns, exactly as the drag pass already did. The test nests a canvas root inside a block and selects an address only the inner document holds. Unbounded outermost lookup — confirmed. The ancestor search now stops at the canvas that owns the element; a host found beyond it belongs to another document and cannot make this component nested.
The branch was also merged onto main past #1761, so its integration legs run against the repaired golden test. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c19dd26802
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // canvas rendered within another rendered page would otherwise resolve a | ||
| // click to the outer page's node. | ||
| if (owner.closest(`.${CANVAS_ROOT_CLASS}`) === null) return null; | ||
| return nodeAddressOf(owner); |
There was a problem hiding this comment.
When one canvas is rendered inside another, this check only proves that the owner belongs to some canvas. A click on an inner component bubbles to the outer canvas, and because IDs are document-scoped, the inner instance may validly share an ID with an outer node; this new translation then returns that stored instance ID to both handlers, so the outer onSelect can select its unrelated node and subsequent keyboard or toolbar actions target the wrong document. Pass the current canvas root into this lookup and reject owners whose nearest canvas differs, as contextMenuTargetOf already does.
AGENTS.md reference: AGENTS.md:L294-L297
Useful? React with 👍 / 👎.
1 participant
T-6a slice 2, and the consumer #1694 shipped the marker for.
A component is inlined at render: the instance node is replaced by the tree its definition describes, and every element of that tree carries an id re-minted during composition — one the page's stored document does not contain.
So
nodeIdFromEventreturned that id andonSelectwas handed an address it cannot select, edit or delete. The drag path bailed instead, becausefindNodecould not resolve it either — dragging inside a component already did nothing, by accident rather than by design.The walk now asks the element it lands on whether the resolver marked it, and answers with the host instance when it did.
Asking the ELEMENT, not its ancestors
This is the design rather than a detail, and it is the part worth reviewing closely.
owner.closest("[data-nx-instance]")is the obvious implementation and it is wrong. An instance's slot content belongs to the page and is deliberately unmarked — but it renders nested inside the definition's marked box. A walk upwards finds that box and hands back the component for a node the author can and should select directly, which is exactly the content someone opened the editor to change.The marker is per-node rather than a wrapper element precisely so the question can be answered without walking.
Break-verified against that exact form. Implementing
closest()fails the slot-content case:The inverse reader answers the same addresses
nodeElementandnodeIdFromEventare one mapping read in two directions; separated, they become two answers to what a node id means in the DOM.An instance id names a node that renders no element of its own, so chrome measuring the selection is pointed at the first element the definition contributed — the outermost, in document order. Without it, selecting inside a component would draw its outline nowhere. Break-verified: removing the instance lookup fails that case.
A node's own element still wins where both exist. They cannot collide today — an instance is replaced rather than rendered — and stating the precedence keeps it from being iteration order.
An empty marker reads as page-owned
The renderer never emits one; it treats an empty id as no provenance. But an address of
""is not selectable, and reading it as page-owned leaves the element addressable by its own id rather than by nothing.Verified
packages/builder: 3079 tests pass, lint 0, check-types 0fallow audit --base origin/main: pass, no introduced complexity or dead codechangeset status: valid