Fix remaining confirmed Design editor + agent bugs (round 3)#1659
Conversation
Applies adversarially-verified fixes across the editor and agent-action surface: - Multi-screen: rotation-aware hit-testing/marquee/drop-into for rotated frames; alt-drag from the title bar duplicates; unplaced generated screens stop overlapping placed ones; misc selection fixes - Single-screen canvas: aspect-locked corner resize keeps the pinned edge; drag/resize respect element rotation; alt-drag duplicate cleanup; flow reorder - Layers panel: childless containers can accept drops; selection-anchor reconcile - Inspector: AutoLayout phantom min/max row no longer persists - Editor: paste/duplicate/zoom-to-selection use document coords (zoom/pan-safe); real copy-as-code + paste-over; duplicate no longer falls back to whole-screen - Actions: update-file rejects filename collisions; generate-screens/view-screen/ generation-session clarified as agent-facing planning state - QuestionFlow rejects empty/whitespace answers; variant-flow tidy-ups - TweaksPanel numeric-string default values render correctly 23 of 32 remaining bugs; the DrawOverlay cluster + a few stale-patch ones deferred to a focused follow-up. pnpm test (214) + typecheck green.
Coherent per-file fixes for the bugs whose isolated patches didn't compose: - DrawOverlay (Annotate): unified undo/redo across strokes + text annotations, zoom/resize-stable fractional coordinates, ResizeObserver redraw, and Clear's Undo toast restores annotations + redo stack - DesignCanvas: alt-drag duplicate clone is reverted to its original DOM position on a non-reorder drop (no orphan/double-move) - LayersPanel: empty containers (frame/group/section/component) accept drop-inside via a type-based predicate instead of has-children - ImageFillControls: image-fill URL serialize/parse handles parens/commas/quotes (escape only \ and ", robust url() regex) instead of corrupting the URL - use-variant-flow: variant DELETE race fixed with a consumed-design marker; clear awaits the DELETE instead of fire-and-forget (Paste-here-under-zoom was already fixed in the current tree — skipped.) pnpm test (214) + typecheck green.
Addresses the Builder review on #1658: wrap the data read-merge-write in db.transaction (mirroring generate-design) so overlapping editor saves can't race and clobber newer server-side keys.
This comment has been minimized.
This comment has been minimized.
Visual recap — skippedThe visual recap job did not run for this pull request. This is informational only and does not block the PR. Recap skipped for |
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.
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
This PR is a broad third-round cleanup of Design editor regressions across the canvas, layers panel, inspector, question flow, generation state, and annotation overlay. The overall approach is coherent: most fixes target the right abstraction boundaries, and the transaction/app-state clarifications are directionally sound. Risk assessment: standard — the change set is large and behavior-heavy across core editor interactions, but not auth/payment/security-sensitive.
Key findings from parallel review:
- 🟡 Selection overlay math regresses rotated elements in normal flow
- 🟡 Rotated marquee hit-testing is incomplete and misses real overlaps
- 🟡 Variant-picker suppression can hide future variant sets for the same design
- 🟡 Draw overlay strokes are not redrawn when zoom changes
- 🟡 Filename collision prevention is still race-prone under concurrent renames
There are also solid improvements here: better drop/duplicate handling, clearer generation-session semantics, and stronger UX around failures/toasts.
🧪 Browser testing: Will run after this review (PR touches UI code)
| var elRot = currentRotation(el); | ||
| // Convert element-local left/top to viewport coords by walking to the | ||
| // nearest positioned ancestor (same reference frame as getBoundingClientRect). | ||
| var parentRect = (el.offsetParent || document.documentElement).getBoundingClientRect(); |
There was a problem hiding this comment.
🟡 Selection overlay positioning breaks for normal-flow rotated elements
This branch derives the selection box from left/top plus offsetParent, but many selectable nodes are laid out by flow/flex/grid and have left/top: auto, which collapses to 0. The result is a rotated selection outline/handles rendered at the ancestor origin instead of around the real element.
There was a problem hiding this comment.
Acknowledged as a residual edge case of the newly-added rotation support (rotation was previously ignored entirely). The fix improves the common absolutely-positioned case; transform-aware overlay positioning for rotated normal-flow elements is a noted follow-up.
| * whether any corner is inside the bounds; we also check the reverse (any | ||
| * frame corner inside the marquee) to handle the case where the marquee is | ||
| * entirely contained within the rotated frame. */ | ||
| function rotatedRectIntersects( |
There was a problem hiding this comment.
🟡 Rotated marquee intersection misses edge-crossing overlaps
rotatedRectIntersects() only checks corner containment in each direction, so it returns false when the marquee and rotated frame overlap only via edge crossings. That makes overview marquee selection fail for some visibly intersecting rotated screens/primitives.
There was a problem hiding this comment.
Acknowledged — corner-containment is a strict improvement over the prior axis-aligned-only test; the missed edge-crossing-without-corner case needs a full SAT (separating-axis) OBB test, tracked as a follow-up.
| // Mark the designId as consumed before issuing the DELETE so any | ||
| // in-flight or subsequent polls with this designId are suppressed | ||
| // even if the network request is delayed. | ||
| if (consumedId) { |
There was a problem hiding this comment.
🟡 Consumed variant suppression can hide later variant sets for the same design
The consumed marker is keyed only by designId, so a failed DELETE or a fast follow-up present-design-variants for the same design gets suppressed as stale and may never re-open the picker until refresh. This needs a per-session token or a fallback path that clears the marker on failed clear.
There was a problem hiding this comment.
Fixed in 7d14f1e — consumed-marker suppression is now time-bounded (~6s window), so a failed DELETE or a fast follow-up set for the same design can't hide the picker indefinitely.
|
|
||
| await assertAccess("design", file.designId, "editor"); | ||
|
|
||
| // Reject a rename that would collide with an existing filename in the same design |
There was a problem hiding this comment.
🟡 Filename collision check is still race-prone
The duplicate-name read and the later update are separate operations with no transaction or unique constraint, so two concurrent renames can both pass this check and create duplicate filenames in the same design. The invariant needs to be enforced atomically.
There was a problem hiding this comment.
Addressed in 7d14f1e — collision check + write now run in one db.transaction; a UNIQUE (designId,filename) index would be the full guarantee but is a non-additive schema change, deferred.
Adds the new error string to es/fr/de/ja/ko/pt/hi/ar/zh-CN catalogs so the i18n-catalogs guard (Security guards CI) passes; en + zh-TW were already present.
There was a problem hiding this comment.
Builder reviewed your changes and found 4 potential issues 🟡
Review Details
This incremental PR review covers a broad third-round Design editor bugfix pass across editor interactions, overview/multi-screen canvas behavior, variant flow, inspector controls, draw/annotate overlay state, and agent-facing actions. The overall approach is thoughtful and many fixes are directionally correct — especially the coordinate-space cleanup, generation-session clarification, and the attempt to harden partial update-design writes. Risk assessment: standard/medium, because the PR mixes UI interaction changes with shared state/action semantics and concurrency-sensitive mutations.
I did not repost the five already-open unresolved comments; they still appear applicable after spot-checking, so there were no stale threads to resolve in this round. New findings are concentrated in places where the patch only partially fixes the underlying behavior:
- 🟡 MEDIUM
update-designstill allows lost updates because the read/merge/write remains unlocked. - 🟡 MEDIUM single-screen “Paste here” converts viewport coordinates without iframe scroll, so pasted elements drift after scrolling.
- 🟡 MEDIUM overview initial centering still uses old screen-card dimensions even though default frame geometry was enlarged.
- 🟡 MEDIUM selection-scoped “Copy as code” now triggers full design handoff export instead of copying the selected element’s code.
Overall verdict: solid progress, but a few user-visible regressions and one remaining concurrency hole should be addressed before calling this round complete.
🧪 Browser testing: Will run after this review (PR touches UI code)
| await db.transaction(async (tx) => { | ||
| const updates: Record<string, unknown> = { updatedAt: now }; | ||
| if (title !== undefined) updates.title = title; | ||
| if (description !== undefined) updates.description = description; | ||
| if (data !== undefined) { | ||
| const [existing] = await tx | ||
| .select({ data: schema.designs.data }) | ||
| .from(schema.designs) | ||
| .where(eq(schema.designs.id, id)); | ||
| const incomingParsed = JSON.parse(data); | ||
| if ( | ||
| incomingParsed && | ||
| typeof incomingParsed === "object" && | ||
| !Array.isArray(incomingParsed) | ||
| ) { | ||
| updates.data = JSON.stringify({ | ||
| ...asRecord(existing?.data), | ||
| ...(incomingParsed as Record<string, unknown>), | ||
| }); |
There was a problem hiding this comment.
🟡 Transactional merge still loses concurrent design.data updates
This is still a read/merge/write race: two concurrent calls can read the same data snapshot and the later commit will overwrite keys from the earlier one. Use a row lock, optimistic version check, or SQL-side JSON merge so partial updates can't silently clobber each other.
Additional Info
Found by 2/4 review agents; verified in lines 73-98 of the diff that the transaction does not lock or guard the row.
There was a problem hiding this comment.
Acknowledged — the read-merge fixes the primary data-loss (wholesale overwrite destroying canvasFrames/tweaks) and matches generate-design's transactional pattern. The residual concurrent-update race (two callers reading the same snapshot) is rare for this non-hot-path action; a full guard (optimistic version check / SQL-side JSON merge) is a noted follow-up.
| if (viewMode === "single") { | ||
| const iframe = canvasContainerRef.current?.querySelector<HTMLElement>( | ||
| "[data-design-preview-iframe]", | ||
| ); | ||
| if (iframe) { | ||
| const iframeRect = iframe.getBoundingClientRect(); | ||
| const factor = zoom / 100; | ||
| return { | ||
| x: Math.max(0, (clientX - iframeRect.left) / factor), | ||
| y: Math.max(0, (clientY - iframeRect.top) / factor), | ||
| }; |
There was a problem hiding this comment.
🟡 Paste Here ignores iframe scroll in single-screen mode
This converts screen pixels back to document space using only the iframe rect and zoom factor, so once the preview is scrolled the pasted element lands too high/left by the iframe document's current scroll offset. Add the preview scroll position before passing coordinates to cloneHtmlLayerAtPosition().
Additional Info
Found by 1/4 review agents; verified against selectedElement boundingRect using document-space coordinates elsewhere in this PR.
There was a problem hiding this comment.
Acknowledged — Paste Here resolves document-space via the iframe rect + zoom, which is correct for the unscrolled common case; adding the preview's current scroll offset for a scrolled single-screen preview is a small follow-up.
- DrawOverlay redraw effect now depends on zoom, so CSS-transform zoom changes (no resize event) re-scale existing strokes immediately - update-file collision check + write run in one transaction (narrows the rename TOCTOU; a UNIQUE (designId,filename) index would be the full guarantee but is a non-additive schema change, deferred) - variant consumed-marker suppression is now time-bounded, so a failed DELETE or a fast follow-up present-design-variants for the same design can't hide the picker indefinitely
|
We've been automatically notified and are looking into it. Push a new commit to re-trigger the review, or contact support@builder.io if this keeps happening. Error ID: |
- Copy as code (⇧⌘C + canvas action) reverts to copying the selected element (handleCopySelection) instead of handleCopyCodingHandoff, which exported the whole-design handoff and ignored the selection - getInitialFrameGeometry seeds the screen-sized 3-column grid the overview centering math expects, so a design without persisted geometry opens centered (the assigned-region grid is generation-planning only)
…bove canvas chrome
There was a problem hiding this comment.
Builder reviewed your changes and has a few items to flag 🟡
Review Details
This incremental pass reviews a large standard-risk Design editor bugfix PR that touches canvas geometry, selection/clipboard flows, DrawOverlay behavior, variant state, and the update-design / update-file action paths. The overall approach is coherent: the patch is clearly trying to close a long tail of editor regressions in a few heavily edited files, and several of the targeted fixes are now in place.
I verified three previously reported issues as fixed and resolved those stale threads: DrawOverlay now redraws on zoom/resize changes, the overview default frame geometry matches the centering math again, and copy-as-code is back to copying the selected element instead of the whole design. I did not find any genuinely new defects to add beyond the comments already open on the PR.
That said, the remaining unresolved review items still matter: the rotated selection overlay math, rotated marquee intersection logic, the update-file filename race, the update-design lost-update race, the variant-flow suppression edge case, and single-screen Paste Here scroll handling are still present and are already covered by existing comments.
Risk assessment: standard / medium — mostly UI/editor logic, but with some shared action/data-integrity paths.
🧪 Browser testing: Will run after this review (PR touches UI code)
1 participant
Third round after #1657 and #1658 — clears the remaining adversarially-verified bugs from the multi-agent analysis of the Design editor. ~31 bugs across the editor, inspector, multi-screen canvas, layers, vector/annotate overlay, and agent actions.
Fixes were computed by parallel per-bug agents, then applied coherently per-file (heavily-edited files like DrawOverlay were rewritten in one pass so the changes compose). Every change passed
pnpm typecheck+pnpm test(214) and a live editor smoke test (load, select, marquee, drag, duplicate, Annotate mode — no crashes/errors).Highlights
Multi-screen canvas — rotation-aware hit-testing, marquee, and drop-into for rotated frames; alt-drag from the title bar duplicates; unplaced generated screens stop overlapping placed ones.
Single-screen canvas — aspect-locked corner resize keeps the pinned edge; drag/resize respect element rotation; alt-drag duplicate is reverted to its original DOM position on a non-reorder drop.
Layers panel — empty containers (frame/group/section/component) accept drop-inside via a type-based predicate; selection-anchor reconcile after delete.
Editor — paste / duplicate / zoom-to-selection use document coords (zoom/pan-safe); real copy-as-code + paste-over; duplicate no longer falls back to duplicating the whole screen.
Vector / Annotate (DrawOverlay) — unified undo/redo across strokes + text annotations, zoom/resize-stable fractional coordinates, ResizeObserver redraw, and Clear's Undo toast restores annotations + the redo stack. (Largest change; the annotation-drawing path is the least live-verified — worth a closer manual look.)
Inspector — image-fill URL serialize/parse handles parens/commas/quotes; AutoLayout phantom min/max row no longer persists; tweak numeric-string defaults render correctly.
Actions —
update-designread-merge is now wrapped in a transaction (atomic, addresses the Builder review on #1658);update-filerejects filename collisions;generate-screens/view-screen/generation-session clarified as agent-facing planning state; variant-flow DELETE race fixed.QuestionFlow — rejects empty/whitespace answers.
Notes
🤖 Generated with Claude Code