Conversation
|
Here's a visual recap of what changed: Open the full interactive recap |
![builder-io-integration[bot]](https://avatars.githubusercontent.com/in/353721?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Builder reviewed your changes and found 5 potential issues 🟡
Review Details
Incremental Code Review Summary
The latest PR updates correctly address the two previously reported drop-target regressions: ancestry is now used instead of rectangle area, and equal-sized nested containers are covered by tests; the containing-block measurement fix also remains present. However, the new implementation introduces additional edge-case regressions in Design geometry and hit testing.
New findings
- 🟡 Duplicate authored IDs can make unrelated containers appear ancestor-related during drop-target selection.
- 🟡 Overlapping siblings are still resolved by DOM order, which ignores explicit z-index stacking.
- 🟡 Frame Selection wrappers do not persist measured flow origins, so Frame Selection → Ungroup can release in-flow children at incorrect coordinates.
- 🟡 Relative-wrapper ungrouping omits the wrapper’s normal-flow origin and can shift children when the wrapper follows preceding content.
- 🟡 Measuring against
offsetParentdoes not account for scrolling static ancestors between the selected node and containing block.
The new unit tests cover equal-sized nesting, DOM-order overlaps, static wrappers, and basic wrap/unwrap paths, but do not cover duplicate IDs, z-index stacking, Frame Selection ungrouping, relative wrappers with a nonzero flow origin, or scrolling static intermediaries.
🧪 Browser testing: Attempted a full 16-case Design run; all cases were unverified because MCP Chrome browser-control tools were unavailable to the executors.
| descendant: ParsedScreenPrimitive, | ||
| primitives: ParsedScreenPrimitive[], | ||
| ): boolean { | ||
| let parentId = descendant.parentNodeId ?? descendant.parentProjectionNodeId; |
There was a problem hiding this comment.
🟡 Prefer projection ancestry when authored IDs are duplicated
parentNodeId is the authored data-agent-native-node-id, and it is preferred over parentProjectionNodeId. Legacy/imported screens can contain duplicate authored IDs, so a child under one container can make an unrelated container with the same authored ID look like its ancestor. Prefer the unique projection parent identity when available, and add a duplicate-ID ancestry regression test.
Additional Info
Found by 1 review agent; distinct from the previously fixed area-based ancestry issue.
| !isPrimitiveAncestor(bestPrimitive, primitive, primitives) && | ||
| !isPrimitiveAncestor(primitive, bestPrimitive, primitives) | ||
| ) { | ||
| // Parsed order follows DOM paint order, so the later overlapping |
There was a problem hiding this comment.
🟡 Respect z-index when resolving overlapping siblings
The sibling fallback always selects the later parsed sibling, but DOM order is not CSS paint order when positioned containers have explicit z-index. An earlier sibling with a higher z-index can be visually on top while this code selects the later, obscured container. Compare stacking order before falling back to DOM order, and add a z-index overlap test.
Additional Info
Found independently by 2 review agents.
| ? ` ${MEASURED_FLOW_GROUP_ATTR}="true"` | ||
| : ""; | ||
| const wrapperOpen = `<div data-agent-native-node-id="${escapeHtmlAttribute(wrapperNodeId)}" data-agent-native-layer-name="${escapeHtmlAttribute(wrapperLayerName)}" data-agent-native-group-wrapper="true" data-agent-native-preserve-styles="true"${wrapperKindAttr}${measuredFlowAttr}${wrapperStyleAttr}>`; | ||
| const measuredFlowOriginAttr = hasMeasuredGroupRuntime |
There was a problem hiding this comment.
🟡 Persist measured origins for Frame Selection wrappers
runFrameSelection calls wrapNodes with wrapperKind: "frame", but hasMeasuredGroupRuntime is false for frame wrappers, so the measured flow origin attributes are not persisted. The children are still converted to absolute positions relative to the measured wrapper; ungrouping then has no origin to add back and can release them at local coordinates instead of their original containing-block positions. Persist the origin for measured frame wrappers and add a Frame Selection wrap/unwrap regression test.
Additional Info
Found by 1 review agent; verified against frame-selection.ts:389-394 and the measuredFlowGeometry path.
| ); | ||
| const shouldRebase = | ||
| wrapperStyle.position === "absolute" && | ||
| (wrapperStyle.position === "absolute" || |
There was a problem hiding this comment.
🟡 Preserve normal-flow origin when ungrouping relative wrappers
Treating position:relative like an absolute wrapper and rebasing only by its left/top inset omits the wrapper's normal-flow position. A relative wrapper after a preceding sibling renders its absolute child at static-flow-origin plus inset, but unwrap rewrites only the inset and drops the static origin. Measure or persist the complete rendered wrapper origin before rebasing relative wrappers, or keep the wrapper's positioning semantics during release.
Additional Info
Found by 1 review agent; distinct from the fixed static-wrapper regression.
| iframeWindow: Window, | ||
| ): { left: number; top: number; width: number; height: number } | null { | ||
| const parent = element.parentElement; | ||
| const parent = element.offsetParent ?? element.parentElement; |
There was a problem hiding this comment.
🟡 Include scrolling static ancestors in measured coordinates
Using offsetParent fixes the containing-block choice, but a static overflow/scroll ancestor between the selected element and that containing block is not included in the calculation. Its scroll displacement remains in the child client rect while only offsetParent.scrollLeft/scrollTop is added, so the stored coordinates can be off by the intermediary scroll amount and wrap/unwrap can visibly jump. Accumulate scroll offsets through the ancestor chain and add a scrolling-static-intermediary regression test.
Additional Info
Found by 1 review agent; independently investigated against the offsetParent implementation.
There was a problem hiding this comment.
Builder reviewed your changes and found 2 potential issues 🟡
Review Details
Incremental Code Review Summary
The latest commit resolves the previously reported issues around projection ancestry, direct z-index ordering, Frame Selection measured origins, relative-wrapper flow origins, and scrolling static ancestors. It also adds focused regression tests for each of those paths. However, the new scalar z-index comparison still does not model CSS stacking semantics completely.
New findings
- 🟡 Numeric z-index is applied even when CSS ignores it on static, non-flex/grid items, so an inert
z-indexcan incorrectly override DOM paint order. - 🟡 Comparing raw descendant z-index values across separate stacking contexts can select a visually obscured target; the ancestor stacking context must determine ordering before local descendant z-index.
The targeted unit coverage is strong for the direct positioned-sibling case and the previously reported fixes, but it does not cover inert z-index or nested stacking-context ordering.
🧪 Browser testing: Attempted a full 17-case Design run; all cases were unverified because MCP Chrome browser-control tools were unavailable to the executors.
| const bestZIndex = bestPrimitive.zIndex ?? 0; | ||
| if ((primitive.zIndex ?? 0) < bestZIndex) continue; |
There was a problem hiding this comment.
🟡 Ignore z-index on elements where CSS does not apply it
z-index is parsed from every inline style and compared here, but CSS ignores z-index on ordinary static blocks (unless they are flex/grid items). A static earlier sibling with z-index:10 can therefore beat a later painted sibling even though its z-index has no effect. Only use the value for elements that participate in stacking, and add a static-sibling regression test.
Additional Info
Found independently by 2 review agents; distinct from the direct positioned-sibling z-index fix.
| const bestZIndex = bestPrimitive.zIndex ?? 0; | ||
| if ((primitive.zIndex ?? 0) < bestZIndex) continue; |
There was a problem hiding this comment.
🟡 Compare stacking-context order before descendant z-index
The scalar comparison uses each candidate's local z-index without its stacking-context ancestry. A descendant at z-index:999 inside an ancestor stacking context below an overlapping sibling can incorrectly beat a target in the higher stacking context, even though the browser paints the entire lower context underneath. Compare the nearest competing stacking-context ancestors (or derive a paint-order key) before comparing local descendant z-index values, and add a nested-context regression test.
Additional Info
Found independently by 2 review agents; distinct from the direct z-index ordering case.
There was a problem hiding this comment.
Builder reviewed your changes and found 1 potential issue 🟡
Review Details
Incremental Code Review Summary
The latest commit substantially improves stacking-order handling: it now ignores z-index on ordinary static items, records stacking-context z-index paths, preserves projection ancestry, and adds regression tests for direct z-index, inert z-index, and ancestor-context ordering. The previously reported comments remain unresolved and were not reposted.
One new edge case remains in the comparator: when two sibling stacking contexts have the same z-index, a descendant’s local z-index is incorrectly used as a tie-breaker. CSS instead preserves the order of the sibling stacking contexts; a high-z descendant cannot promote its entire ancestor context above a later equal-z sibling context.
🧪 Browser testing: Attempted a full 16-case Design run; all cases were unverified because MCP Chrome browser-control tools were unavailable to the executors.
| for ( | ||
| let index = 0; | ||
| index < Math.min(leftContexts.length, rightContexts.length); | ||
| index += 1 | ||
| ) { | ||
| if (leftContexts[index] !== rightContexts[index]) { | ||
| return (leftContexts[index] ?? 0) - (rightContexts[index] ?? 0); | ||
| } | ||
| } | ||
| return (left.zIndex ?? 0) - (right.zIndex ?? 0); |
There was a problem hiding this comment.
🟡 Preserve DOM order for equal-z sibling stacking contexts
When two candidates share the same ancestor stacking-context prefix, this fallback compares the leaf primitive's zIndex. That lets a high-z descendant in an earlier z-index:1 sibling context beat a later sibling context also at z-index:1, even though the later context is painted above the entire earlier context. Compare the first diverging sibling context and preserve DOM order when its z-indices are equal; add a regression for equal-z sibling contexts with a high-z descendant.
Additional Info
Found by 1 review agent; distinct from the prior stacking-context and direct z-index comments.
1 participant
Fixes two Design pointer regressions from the physical matrix:
Focused proof:
Browser E2E was not run in this worktree; the focused bridge/runtime matrix remains the CI proof surface.,