feat: figma dnd parity in design#2148
Conversation
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.
|
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}
There was a problem hiding this comment.
Builder reviewed your changes and found 5 potential issues 🟡
Review Details
Code Review Summary
PR #2148 adds a gated Figma-parity drag experience to the design editor: lifted cursor-following elements, stabilized flow targets, packed-flex sibling reflow, a nesting size guard, Ctrl/Cmd free placement, and an undo checkpoint marker. The overall architecture is sound: the pure drag-reflow reference module is well covered by unit tests, the generated bridge mirrors the source bridge, and teardown paths attempt to restore inline styles. This is a standard-risk UI/state-management change.
Key findings
- 🟡 MEDIUM: The live-reflow hysteresis timestamp is not reset when the committed target/container is reset, allowing a stale dwell time to accept a new slot immediately.
- 🟡 MEDIUM: Incoming agent checkpoints can still coalesce backward into a preceding user edit, losing the checkpoint marker and undo boundary.
- 🟡 MEDIUM: Group-drag members that are both lifted and reflowed can retain their lift transform because cleanup restores lift before reflow.
- 🟡 MEDIUM: Live mode retains a target from the last move even when Ctrl changes at release, so modifier-sensitive drop semantics can be stale.
- 🟡 MEDIUM: The size guard only checks
insidetargets, bypassing populated-container drops resolved asbefore/after.
🧪 Browser testing: Skipped — dev server is healthy, but browser automation tooling was unavailable; all planned UI cases were recorded as environment-blocked.
| if ( | ||
| !container || | ||
| (reorderEl as HTMLElement).parentElement !== container | ||
| ) { | ||
| reorderCommittedTarget = null; | ||
| reorderCommittedSlot = null; |
There was a problem hiding this comment.
🟡 Reset the hysteresis dwell timestamp with the committed target
When the container/target context is reset, reorderCommittedTarget and reorderCommittedSlot are cleared but reorderCommittedAt remains from the previous target. The next slot change can therefore satisfy the 60ms dwell check immediately, bypassing hysteresis after leaving and re-entering a container. Reset reorderCommittedAt to 0 in each path that clears the committed target state.
| if ( | ||
| last && | ||
| last.fileId === change.fileId && | ||
| last.after === change.before && | ||
| !last.isCheckpoint |
There was a problem hiding this comment.
🟡 Preserve an incoming agent checkpoint as an undo boundary
The merge condition only checks !last.isCheckpoint; an incoming checkpoint can still merge into a preceding user edit when last.after === change.before, losing isCheckpoint. Require the incoming change to be non-checkpoint as well, so an agent-authored replacement always remains a distinct undo entry.
| clearReorderLift(); | ||
| clearReorderReflow(); |
There was a problem hiding this comment.
🟡 Restore reflowed group members before restoring their lift
A group member can be present in both reorderLiftedMembers and reflowSiblings. Cleanup currently clears the lift first, then restores the reflow snapshot captured after the lift was applied, which can leave the lift transform stuck on that member after drop. Restore reflow first and lift last, and apply the same ordering in the generated bridge.
| // With live reflow on, commit the SAME stabilized target the guide/gap | ||
| // showed during the move (so it drops exactly where it previewed); | ||
| // otherwise resolve fresh from the release point (legacy behavior). | ||
| if (!liveReflowEnabled) { |
There was a problem hiding this comment.
🟡 Re-evaluate modifier-sensitive drop mode at release
With live reflow enabled, pointer-up intentionally keeps currentTarget from the last move. Because Ctrl changes flowMoveTargetForPoint semantics for auto-layout drops, pressing or releasing Ctrl after the last move can leave the final drop mode stale and perform a flow reorder instead of the requested absolute drop. Re-resolve at release when modifier state changes, or refresh the stabilized target on modifier transitions.
| function applyReorderSizeGuard(target, ev) { | ||
| if (!liveReflowEnabled || !target || target.placement !== "inside") { |
There was a problem hiding this comment.
🟡 Apply the size guard to child-anchored flow targets
The size guard returns early unless target.placement === "inside", but drops over children in a populated container resolve as before or after while still targeting that parent container. Large elements can therefore be inserted into an undersized container through its children. Apply the guard to every flow-insert target whose resolved container differs from the dragged element's parent, while retaining the Ctrl/Cmd override.
There was a problem hiding this comment.
Builder reviewed your changes — no new findings
Review Details
Incremental Code Review Summary
I reviewed the latest PR #2148 head and compared the updated checkpoint propagation against the prior review. The latest changes are limited to marking external content-history checkpoints in DesignEditor.tsx; the generated bridge and drag/reflow implementation remain synchronized. The five previously reported issues are still present and remain open, so they were intentionally not reposted or resolved.
Two independent incremental code-review passes found no genuinely new regressions. The checkpoint propagation itself is wired consistently with the existing history API, and the existing unit/reflow test coverage remains intact. This remains a standard-risk UI/state-management change.
Findings
No new actionable findings in this incremental review. Existing open comments remain applicable:
- 🟡 Stale hysteresis timestamp reset.
- 🟡 Incoming checkpoint coalescing into a preceding user edit.
- 🟡 Group lift/reflow cleanup ordering.
- 🟡 Modifier-sensitive target staleness at release.
- 🟡 Size-guard bypass for child-anchored targets.
🧪 Browser testing: Attempted full visual verification. The dev server was healthy, but all 17 grounded browser test cases were blocked because browser automation tools were unavailable in executor sessions.
sidmohanty11
changed the title
2 participants
No description provided.