add a ton of e2e tests for design, ux improvements and fixes - #2690
add a ton of e2e tests for design, ux improvements and fixes#2690sidmohanty11 wants to merge 5 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
|
Here's a visual recap of what changed: Open the full interactive recap |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
![builder-io-integration[bot]](https://avatars.githubusercontent.com/in/353721?s=60&v=4){kind=small}
There was a problem hiding this comment.
Builder reviewed your changes and found 5 potential issues 🟡
Review Details
Code Review Summary
This large incremental update adds broad Playwright coverage for the Design editor and adjusts canvas insertion, selection chrome, auto-layout, breakpoint handling, drag/drop persistence, and inspector geometry. The overall direction is sound: the tests encode useful direct-manipulation invariants, frame-vs-rectangle semantics are made explicit, and the new tracing/debug hooks are bounded and disabled outside development. I also verified the design dev server is running cleanly for browser verification.
Risk: Standard (shared editor behavior and stateful UI logic, with no auth/payment/schema changes).
Key findings
- 🔴 Functional: nested frame insertion uses local offsets as document coordinates, so nested primitives can be adopted by the wrong frame or rendered at the wrong position.
- 🟡 Medium: SVG primitives bypass the new frame-host insertion path.
- 🟡 Medium: inspector fallback can measure a matching node from the wrong screen iframe.
- 🟡 Medium: constraints are hidden for absolutely positioned children of flex parents.
- 🟡 Medium: host-driven layer selection does not reposition the combined multi-selection bounds overlay.
🧪 Browser testing: Will run after this review (PR touches UI code)
| } | ||
|
|
||
| doc.body.appendChild(element); | ||
| const host = deepestFrameContaining(doc.body, left, top, width, height); |
There was a problem hiding this comment.
🟡 Resolve nested frame bounds in document coordinates
deepestFrameContaining compares each frame's inline left/top directly with document-space primitive coordinates, but nested frame offsets are relative to their positioned parent. Once a frame is nested, primitives can miss the deepest containing frame or be rebased against the wrong origin. Accumulate positioned ancestor offsets (and add nested-frame coverage) before containment and local coordinate conversion.
Additional Info
Reported by 2 of 4 reviewers; one reviewer classified it high.
| } | ||
|
|
||
| doc.body.appendChild(element); | ||
| const host = deepestFrameContaining(doc.body, left, top, width, height); |
There was a problem hiding this comment.
🟡 Nest SVG primitives inside containing frames
The frame-host lookup runs only in the div-based branch. path, line, arrow, polygon, and star return after appending directly to doc.body, so drawing those tools inside a frame produces a sibling while text/rectangle/frame use nesting. Move host resolution and coordinate rebasing into the shared insertion path, and cover an SVG primitive in a frame.
Additional Info
Reported by 1 of 4 reviewers; confirmed by the branch returns before the host lookup.
| if (typeof document === "undefined") return info; | ||
| const selector = info.runtimeSelector ?? info.selector; | ||
| if (!selector) return info; | ||
| for (const frame of Array.from( |
There was a problem hiding this comment.
🟡 Measure the selected screen instead of the first matching iframe
The geometry fallback scans every preview iframe and returns the first matching selector. Selectors and stamped node IDs are scoped per screen, so identical markup on another screen can supply the wrong X/Y/W/H for the selected layer. Restrict the lookup to the selected target's owning screen/iframe (or pass its file ID through this helper).
Additional Info
Reported by 2 of 4 reviewers.
| </Tooltip> | ||
| {/* Figma: constraints cannot apply to a child of an auto layout | ||
| frame — the parent's layout owns the position. */} | ||
| {element.isFlexChild ? null : ( |
There was a problem hiding this comment.
🟡 Keep constraints available for absolutely positioned flex children
The new isFlexChild gate is based only on the parent's flex display, so an absolutely positioned or fixed descendant is treated as a flex item and loses the Constraints control. Absolute-positioned descendants are not in the flex layout and still need anchoring/resize constraints; exclude them from this suppression.
Additional Info
Reported by 1 of 4 reviewers; clear regression for absolute children.
| passiveSelectionOverlays.push(overlay); | ||
| positionOverlay(overlay, el); | ||
| }); | ||
| // Selection changes do not go through refreshOverlays, so the combined |
There was a problem hiding this comment.
🟡 Reposition multi-selection bounds after host selection changes
When a Layers-panel select-element message changes selectedEl, the handler updates the single-element overlay but does not call positionMultiSelectionBounds(). With passive selections still present, the combined bounds box remains at the previous selection's geometry. Recompute the combined bounds whenever host-driven selection changes (or clear passive selections consistently).
Additional Info
Reported by 1 of 4 reviewers; the existing comment here identifies the missing refresh path.
1 participant
No description provided.