Improve drag and drop for granular changes follow ons - #1783
Conversation
FedericoBonel
changed the title
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds configurable ChangesDrag-and-drop behavior
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant DragDropContext
participant useLinePlaceholder
participant DropZone
participant runDropAnimation
participant ZoneStore
User->>DragDropContext: move dragged component
DragDropContext->>useLinePlaceholder: update pointer position
useLinePlaceholder->>ZoneStore: calculate and store insertion index
ZoneStore->>DropZone: render line or ghost preview
User->>DragDropContext: release component
DragDropContext->>ZoneStore: commit move
DragDropContext->>runDropAnimation: animate committed drop
runDropAnimation->>DropZone: animate feedback to rendered item
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (13)
packages/core/components/DragDropContext/index.tsx (1)
494-503: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winThrottle
updateLinePlaceholderto one call per animation frame.
onDragMovefires on every pointer move.updateLinePlaceholderthen reads the zone rect and callsgetNearestGapIndex, which measures the zone children. This forces a synchronous layout read proportional to the number of items in the zone, on every pointer event. In a large slot this runs on the drag hot path.The scroll path in
use-line-placeholder.tsalready coalesces withrequestAnimationFrame. Apply the same coalescing to the move path so both entry points share one throttle. The hook is the better place for the change, because it owns therafhandle and the cleanup.♻️ Sketch: move the rAF coalescing into the hook and reuse it for both entry points
+ const scheduledUpdate = useRef<number | null>(null); + + const scheduleUpdate = useCallback( + (manager: DragDropManager) => { + if (scheduledUpdate.current !== null) return; + + scheduledUpdate.current = requestAnimationFrame(() => { + scheduledUpdate.current = null; + update(manager); + }); + }, + [update] + );Return
scheduleUpdatealongsideupdate, use it in thescrollhandler, cancel it instopScrollTracking, and call it fromonDragMove:onDragMove={(event, manager) => { // Keep the line in the gap nearest the pointer as it moves within // the target zone: collisions only re-fire when the target // changes, which can leave the line in a stale gap - updateLinePlaceholder(manager); + scheduleLinePlaceholderUpdate(manager);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/components/DragDropContext/index.tsx` around lines 494 - 503, Move the requestAnimationFrame coalescing into the line-placeholder hook: expose a scheduleUpdate operation alongside update, have the scroll handler use it, and cancel any pending frame in stopScrollTracking. In onDragMove, replace the direct updateLinePlaceholder call with the shared scheduler so pointer-move and scroll entry points perform at most one update per animation frame.packages/core/components/DraggableComponent/styles.css (1)
23-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBoth stylesheets depend on Selectors Level 4
:not()with a complex argument. Each rule passes[data-puck-line-drag] *, a descendant-combinator selector, to:not(). Level 3:not()accepts only a simple selector, so a browser without Level 4 support treats the whole selector as invalid and drops the rule. The failure is not limited to line drags: the solid placeholder styling disappears for every drag. Confirm the package browser support matrix, or replace the negation with an affirmative guard attribute set on the placeholder container.
packages/core/components/DraggableComponent/styles.css#L23-L26: verify or replace the:not([data-puck-line-drag] *)guard on the placeholder rule, and apply the same change to the descendant and pseudo-element rules at Lines 38-40.packages/core/components/Sortable/styles.css#L4-L8: apply the same guard change to both the hidden-content rule and the faded-container rule.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/components/DraggableComponent/styles.css` around lines 23 - 26, Replace the Level 4 complex :not() guards with a browser-compatible affirmative guard attribute, or verify that the package browser matrix explicitly supports them. Update the placeholder rule and related descendant/pseudo-element rules in DraggableComponent/styles.css (23-26 and 38-40), plus both hidden-content and faded-container rules in Sortable/styles.css (4-8), preserving the existing line-drag behavior.packages/core/lib/dnd/frame-pointer.ts (1)
3-13: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix the double space in the JSDoc.
Line 4 contains "a position from". Remove the extra space.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/lib/dnd/frame-pointer.ts` around lines 3 - 13, Update the JSDoc description for the frame-pointer position mapping to remove the duplicate space between “position” and “from”.packages/core/lib/__tests__/math.spec.ts (1)
14-14: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the describe name with the exported function.
The block is named
distanceToSegment, but the export isgetDistanceToSegment.♻️ Proposed change
-describe("distanceToSegment", () => { +describe("getDistanceToSegment", () => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/lib/__tests__/math.spec.ts` at line 14, Rename the describe block for the distance-to-segment tests from distanceToSegment to getDistanceToSegment so it matches the exported function name.packages/core/lib/dnd/__tests__/resolve-flow.spec.ts (2)
130-137: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove
toArgsabove thedescribeblock.
toArgsis aconstarrow function declared at Line 130, but the tests reference it from Line 32 onward. The tests still pass becauseitcallbacks run after module evaluation completes, so no temporal dead zone error occurs. Defining the helper next tozoneWithat the top of the file removes the forward reference and matches the existing layout.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/lib/dnd/__tests__/resolve-flow.spec.ts` around lines 130 - 137, Move the toArgs helper declaration above the describe block, placing it next to zoneWith at the top of the test file. Keep its signature and implementation unchanged so the existing tests continue using the same helper without a forward reference.
107-127: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the
inline-gridassertion to the multi-column case.The case is named "treats column auto-flow grids as horizontal". The first assertion matches that name. The second assertion at Lines 118-126 sets
gridAutoFlow: "row"withgridTemplateColumns: "1fr 1fr", so it exercises the multi-column path forinline-grid, not column auto-flow. Move it into the "treats a multi-column grid as horizontal" case at Line 91.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/lib/dnd/__tests__/resolve-flow.spec.ts` around lines 107 - 127, Move the inline-grid assertion with gridTemplateColumns "1fr 1fr" and gridAutoFlow "row" from the "treats column auto-flow grids as horizontal" test into the existing "treats a multi-column grid as horizontal" test, leaving the column auto-flow test focused only on its matching assertion.packages/core/lib/math.ts (1)
8-41: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the axis-aligned constraint.
getDistanceToSegmentclampsxandyindependently. This computes the distance to the segment's bounding box, not to the segment itself. For axis-aligned segments (x1 === x2ory1 === y2) the two are identical, and all current callers innearest-gap.tspass axis-aligned segments. For a diagonal segment the result is too small. State this constraint in the JSDoc so a future caller does not pass a diagonal segment.📝 Proposed doc change
/** - * Gets the distance from a point to the closest point of a line segment in a 2D plane using pythagoras' theorem. + * Gets the distance from a point to the closest point of an axis-aligned line + * segment in a 2D plane using pythagoras' theorem. + * + * The x and y coordinates are clamped independently, so the result is the + * distance to the segment's bounding box. Only pass axis-aligned segments + * (`x1 === x2` or `y1 === y2`); a diagonal segment returns a smaller distance + * than the true point-to-segment distance.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/lib/math.ts` around lines 8 - 41, Update the JSDoc for getDistanceToSegment to explicitly state that it supports only axis-aligned segments, where x1 === x2 or y1 === y2, because its independent coordinate clamping is not accurate for diagonal segments. Keep the implementation unchanged.packages/core/lib/dnd/__tests__/nearest-gap.spec.ts (1)
137-190: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for the wrapped-row branch.
The horizontal suite covers a single non-wrapping row. It does not reach the wrap-detection branch in
nearest-gap.tsat Lines 96-105, which pushes two candidates that share one index when a row wraps. That branch carries the most conditional logic in the module and it is currently untested.Add a case with two rows, where the second row's first item starts to the left of the previous item's right edge.
💚 Proposed test to add
+ it("exposes both ends of a wrapped row as the same index", () => { + const contentIds = ["a", "b", "c"]; + const zone = document.createElement("div"); + + // a and b fill the first row; c wraps onto a second row. + const specs = [ + { id: "a", rect: { left: 0, right: 100, top: 0, bottom: 100 } }, + { id: "b", rect: { left: 100, right: 200, top: 0, bottom: 100 } }, + { id: "c", rect: { left: 0, right: 100, top: 100, bottom: 200 } }, + ]; + + specs.forEach(({ id, rect: r }) => { + const el = document.createElement("div"); + + el.setAttribute("data-puck-component", id); + el.getBoundingClientRect = () => + ({ ...r, width: r.right - r.left, height: r.bottom - r.top, x: r.left, y: r.top, toJSON: () => {} } as DOMRect); + + zone.appendChild(el); + }); + + document.body.appendChild(zone); + + // Near the end of the first row resolves to the same index as the start + // of the second row. + expect(getNearestGapIndex(zone, { x: 199, y: 50 }, contentIds)).toBe(2); + expect(getNearestGapIndex(zone, { x: 1, y: 150 }, contentIds)).toBe(2); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/lib/dnd/__tests__/nearest-gap.spec.ts` around lines 137 - 190, Add a wrapped-row test within the “getNearestGapIndex in horizontal flows” suite, using a two-row zone where the first item of the second row starts left of the previous row’s last item right edge. Assert the expected gap index at a point in the wrapped layout so the wrap-detection branch in getNearestGapIndex is exercised.packages/core/components/DropZone/LinePlaceholder.tsx (1)
98-112: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider the case where neither neighbour is rendered.
getItemreturnsundefinedwhen the element is not in the DOM, which happens in a virtualized zone for any index outside the rendered window. If bothprevandnextare unrendered, control reaches the "Empty zone" branch at Line 104 and draws the caret at the zone's leading padding, far from the pointer.
nearest-gap.tshandles the virtualized case explicitly at its Lines 91-95, so it normally returns an index adjacent to a rendered item, and at least one neighbour resolves. A scroll during a drag can still produce the mismatch briefly.Consider distinguishing "the zone has no rendered items" from "this index has no rendered neighbour", and keeping the last known caret position in the second case.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/components/DropZone/LinePlaceholder.tsx` around lines 98 - 112, Update the caret-position logic in LinePlaceholder so the empty-zone padding fallback is used only when the zone has no rendered items; when both prev and next are unavailable for an index in a populated virtualized zone, retain the last known caret position instead of repositioning to leading padding. Use the existing rendered-item state and caret-position symbols rather than treating missing neighbors as an empty zone.packages/core/lib/dnd/drop-animation.ts (1)
207-223: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuarantee cleanup if
animatefails.
cleanupremoves bothcopyandhideStyle. It only runs through the.finishedpromise chain. Ifcopy.animatethrows, or if the returned animation has nofinishedproperty,cleanupnever runs.hideStylethen stays intargetDoc.head, and its[data-puck-overlay] { opacity: 0 !important }rule hides every Puck overlay until reload.Wrap the animation call so
cleanupalways runs.🛡️ Proposed fix
- copy - .animate( - { - translate: [ - "0px 0px 0", - `${final.left - rect.left}px ${final.top - rect.top}px 0`, - ], - width: [`${rect.width}px`, `${final.width}px`], - height: [`${rect.height}px`, `${final.height}px`], - }, - { ...COMMIT_ANIMATION, fill: "forwards" } - ) - .finished.catch(() => undefined) - .then(cleanup); + try { + const animation = copy.animate( + { + translate: [ + "0px 0px 0", + `${final.left - rect.left}px ${final.top - rect.top}px 0`, + ], + width: [`${rect.width}px`, `${final.width}px`], + height: [`${rect.height}px`, `${final.height}px`], + }, + { ...COMMIT_ANIMATION, fill: "forwards" } + ); + + Promise.resolve(animation?.finished) + .catch(() => undefined) + .then(cleanup); + } catch { + cleanup(); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/lib/dnd/drop-animation.ts` around lines 207 - 223, Update the animation flow around copy.animate so cleanup always executes, including when animate throws synchronously or returns an object without a usable finished promise. Preserve the existing animation options and completion handling, while ensuring both copy and hideStyle are removed through the existing cleanup function.packages/core/lib/dnd/__tests__/drop-animation.spec.ts (1)
48-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the reduced-motion and failed-commit paths.
The suite covers only the successful glide. Two branches in
runTargetDropAnimationcarry cleanup risk and are untested:
prefersReducedMotionreturns true, so the function returns before it appendscopyorhideStyle.waitForCommitreports no commit, socleanupmust remove bothcopyand the injectedhideStyleelement.The second case matters because a leaked
hideStylekeeps[data-puck-overlay] { opacity: 0 !important }applied.Do you want me to generate these test cases?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/lib/dnd/__tests__/drop-animation.spec.ts` around lines 48 - 83, Add tests alongside the existing runTargetDropAnimation coverage for the prefersReducedMotion early-return path, asserting no copy or hideStyle is appended, and for a failed waitForCommit path, asserting cleanup removes both the copy and injected hideStyle element. Reuse the existing animation, DOM, and commit-test helpers while preserving the successful glide test.packages/core/lib/dnd/commit-animation.ts (1)
91-105: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider returning a cancel handle and using the target document's
requestAnimationFrame.
waitForCommituses the ambientrequestAnimationFramewhile it queriesdoc, which is often the iframe document. Usedoc.defaultView?.requestAnimationFrameso the polling and the queried DOM share one window.The loop also has no cancel path. It is bounded to 10 frames, so it does not leak, but the callback can still run after the drag context unmounts. Returning a cancel function lets callers stop it.
♻️ Proposed refactor
const initialIds = new Set(initialExpectedOrder); let attempts = 0; + let frame = 0; + let cancelled = false; + const raf = doc.defaultView?.requestAnimationFrame ?? requestAnimationFrame; + const caf = doc.defaultView?.cancelAnimationFrame ?? cancelAnimationFrame; const tick = () => { + if (cancelled) return;attempts++; - requestAnimationFrame(tick); + frame = raf(tick); return; } callback(committed); }; - requestAnimationFrame(tick); + frame = raf(tick); + + return () => { + cancelled = true; + caf(frame); + }; };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/lib/dnd/commit-animation.ts` around lines 91 - 105, Update waitForCommit to schedule polling through doc.defaultView?.requestAnimationFrame, with an appropriate fallback when no window is available, so animation timing matches the queried document. Return a cancel function that prevents pending polling and suppresses callback execution after cancellation, while preserving the existing frame limit and commit result behavior.packages/core/lib/dnd/__tests__/flip-commit.spec.ts (1)
3-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated and diverged commit-animation test scaffolding. Both suites redefine the same
getFramemock,recthelper,makeItemhelper, andbeforeEachsetup. The frame-flush logic diverged between them: one swaps the array before invoking, the other callsforEachdirectly and therefore skips callbacks thatwaitForCommitqueues during a retry. One shared helper module fixes both sites.
packages/core/lib/dnd/__tests__/flip-commit.spec.ts#L3-L53: move thegetFramemock,rect,makeItem, thebeforeEachsetup, andflushAnimationFrameinto a shared helper module, and makeflushAnimationFrameloop untilanimationFramesis empty so retries are flushed.packages/core/lib/dnd/__tests__/drop-animation.spec.ts#L65-L74: import that shared helper and replaceanimationFrames.forEach((callback) => callback(0))with the sharedflushAnimationFramecall.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/lib/dnd/__tests__/flip-commit.spec.ts` around lines 3 - 53, In packages/core/lib/dnd/__tests__/flip-commit.spec.ts#L3-L53, extract the shared getFrame mock, rect, makeItem, beforeEach setup, and flushAnimationFrame into a helper module, making flushAnimationFrame repeatedly drain animationFrames so callbacks queued during retries are also processed. In packages/core/lib/dnd/__tests__/drop-animation.spec.ts#L65-L74, import the shared helper and replace the local direct animationFrames.forEach flush with flushAnimationFrame; remove the duplicated scaffolding there as applicable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/components/DragDropContext/index.tsx`:
- Around line 611-620: Update the line-placeholder handling in the drag-and-drop
commit path so a null result from getLinePlaceholderTargetIndex disables
linePlaceholder instead of retaining the fallback targetIndex. Preserve the
resolved gap index when available, and ensure same-zone moves do not receive the
later line-placeholder decrement without a valid gap index.
In `@packages/core/components/DropZone/LinePlaceholder.tsx`:
- Around line 114-156: Adjust the zone-relative coordinate calculations in both
branches of the DropZone caret positioning logic to subtract the corresponding
zone border width from the left/top offsets derived from zoneRect. Reuse the
already computed zoneStyle border widths, updating the vertical branch alongside
the horizontal branch while preserving existing clamping and scroll handling.
In `@packages/core/lib/__tests__/math.spec.ts`:
- Around line 27-32: Update the test case for getDistanceToSegment named “clamps
beyond the segment endpoints” so the point lies beyond one segment endpoint on
the primary x-axis, exercising endpoint clamping rather than only perpendicular
distance. Keep the expected distance consistent with the revised point and
correct the directional comment to match the coordinate values.
In `@packages/core/lib/dnd/commit-animation.ts`:
- Around line 50-64: Update the selector construction used by getZoneSelector
and getComponentSelector in the tick flow to escape interpolated values even
when CSS.escape is unavailable. Preserve the existing quoted-value behavior
while adding fallback escaping for quotes, control characters, and backslashes
so querySelector remains valid and matches the intended zone and component
identifiers.
In `@packages/core/lib/dnd/frame-pointer.ts`:
- Around line 24-30: Update the frame-pointer calculation around the scale
variable to return the original position unchanged whenever scale is not a
positive finite number. Keep the existing coordinate transformation for valid
positive finite scales.
---
Nitpick comments:
In `@packages/core/components/DragDropContext/index.tsx`:
- Around line 494-503: Move the requestAnimationFrame coalescing into the
line-placeholder hook: expose a scheduleUpdate operation alongside update, have
the scroll handler use it, and cancel any pending frame in stopScrollTracking.
In onDragMove, replace the direct updateLinePlaceholder call with the shared
scheduler so pointer-move and scroll entry points perform at most one update per
animation frame.
In `@packages/core/components/DraggableComponent/styles.css`:
- Around line 23-26: Replace the Level 4 complex :not() guards with a
browser-compatible affirmative guard attribute, or verify that the package
browser matrix explicitly supports them. Update the placeholder rule and related
descendant/pseudo-element rules in DraggableComponent/styles.css (23-26 and
38-40), plus both hidden-content and faded-container rules in
Sortable/styles.css (4-8), preserving the existing line-drag behavior.
In `@packages/core/components/DropZone/LinePlaceholder.tsx`:
- Around line 98-112: Update the caret-position logic in LinePlaceholder so the
empty-zone padding fallback is used only when the zone has no rendered items;
when both prev and next are unavailable for an index in a populated virtualized
zone, retain the last known caret position instead of repositioning to leading
padding. Use the existing rendered-item state and caret-position symbols rather
than treating missing neighbors as an empty zone.
In `@packages/core/lib/__tests__/math.spec.ts`:
- Line 14: Rename the describe block for the distance-to-segment tests from
distanceToSegment to getDistanceToSegment so it matches the exported function
name.
In `@packages/core/lib/dnd/__tests__/drop-animation.spec.ts`:
- Around line 48-83: Add tests alongside the existing runTargetDropAnimation
coverage for the prefersReducedMotion early-return path, asserting no copy or
hideStyle is appended, and for a failed waitForCommit path, asserting cleanup
removes both the copy and injected hideStyle element. Reuse the existing
animation, DOM, and commit-test helpers while preserving the successful glide
test.
In `@packages/core/lib/dnd/__tests__/flip-commit.spec.ts`:
- Around line 3-53: In
packages/core/lib/dnd/__tests__/flip-commit.spec.ts#L3-L53, extract the shared
getFrame mock, rect, makeItem, beforeEach setup, and flushAnimationFrame into a
helper module, making flushAnimationFrame repeatedly drain animationFrames so
callbacks queued during retries are also processed. In
packages/core/lib/dnd/__tests__/drop-animation.spec.ts#L65-L74, import the
shared helper and replace the local direct animationFrames.forEach flush with
flushAnimationFrame; remove the duplicated scaffolding there as applicable.
In `@packages/core/lib/dnd/__tests__/nearest-gap.spec.ts`:
- Around line 137-190: Add a wrapped-row test within the “getNearestGapIndex in
horizontal flows” suite, using a two-row zone where the first item of the second
row starts left of the previous row’s last item right edge. Assert the expected
gap index at a point in the wrapped layout so the wrap-detection branch in
getNearestGapIndex is exercised.
In `@packages/core/lib/dnd/__tests__/resolve-flow.spec.ts`:
- Around line 130-137: Move the toArgs helper declaration above the describe
block, placing it next to zoneWith at the top of the test file. Keep its
signature and implementation unchanged so the existing tests continue using the
same helper without a forward reference.
- Around line 107-127: Move the inline-grid assertion with gridTemplateColumns
"1fr 1fr" and gridAutoFlow "row" from the "treats column auto-flow grids as
horizontal" test into the existing "treats a multi-column grid as horizontal"
test, leaving the column auto-flow test focused only on its matching assertion.
In `@packages/core/lib/dnd/commit-animation.ts`:
- Around line 91-105: Update waitForCommit to schedule polling through
doc.defaultView?.requestAnimationFrame, with an appropriate fallback when no
window is available, so animation timing matches the queried document. Return a
cancel function that prevents pending polling and suppresses callback execution
after cancellation, while preserving the existing frame limit and commit result
behavior.
In `@packages/core/lib/dnd/drop-animation.ts`:
- Around line 207-223: Update the animation flow around copy.animate so cleanup
always executes, including when animate throws synchronously or returns an
object without a usable finished promise. Preserve the existing animation
options and completion handling, while ensuring both copy and hideStyle are
removed through the existing cleanup function.
In `@packages/core/lib/dnd/frame-pointer.ts`:
- Around line 3-13: Update the JSDoc description for the frame-pointer position
mapping to remove the duplicate space between “position” and “from”.
In `@packages/core/lib/math.ts`:
- Around line 8-41: Update the JSDoc for getDistanceToSegment to explicitly
state that it supports only axis-aligned segments, where x1 === x2 or y1 === y2,
because its independent coordinate clamping is not accurate for diagonal
segments. Keep the implementation unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fe905825-bbf4-410e-a3ba-cf6caab5484d
📒 Files selected for processing (34)
apps/demo/app/[...puckPath]/client.tsxapps/docs/pages/docs/api-reference/components/puck.mdxpackages/core/components/DragDropContext/index.tsxpackages/core/components/DragDropContext/use-drop-animation.tspackages/core/components/DragDropContext/use-line-placeholder.tspackages/core/components/DraggableComponent/index.tsxpackages/core/components/DraggableComponent/styles.csspackages/core/components/Drawer/index.tsxpackages/core/components/DropZone/LinePlaceholder.tsxpackages/core/components/DropZone/VirtualizedDropZone.tsxpackages/core/components/DropZone/context.tsxpackages/core/components/DropZone/index.tsxpackages/core/components/DropZone/lib/use-content-with-preview.tspackages/core/components/DropZone/styles.module.csspackages/core/components/Puck/components/Layout/index.tsxpackages/core/components/Sortable/styles.csspackages/core/lib/__tests__/get-zone-content-ids.spec.tspackages/core/lib/__tests__/math.spec.tspackages/core/lib/dnd/__tests__/drop-animation.spec.tspackages/core/lib/dnd/__tests__/flip-commit.spec.tspackages/core/lib/dnd/__tests__/nearest-gap.spec.tspackages/core/lib/dnd/__tests__/resolve-dnd-mode.spec.tspackages/core/lib/dnd/__tests__/resolve-flow.spec.tspackages/core/lib/dnd/commit-animation.tspackages/core/lib/dnd/drop-animation.tspackages/core/lib/dnd/flip-commit.tspackages/core/lib/dnd/frame-pointer.tspackages/core/lib/dnd/nearest-gap.tspackages/core/lib/dnd/resolve-dnd-mode.tspackages/core/lib/dnd/resolve-flow.tspackages/core/lib/dom-selectors.tspackages/core/lib/get-zone-content-ids.tspackages/core/lib/math.tspackages/core/types/API/index.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/components/DropZone/LinePlaceholder.tsx (1)
114-154: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCorrect empty-zone coordinates after the border conversion.
For an empty bordered zone,
mainuseszoneRectplus padding but omits the border width. Lines 125, 132, 147, and 154 then subtract that border width. The line placeholder is offset toward the leading edge by the border width.Add the applicable border width when constructing fallback viewport coordinates. Handle trailing borders for reversed flow.
Proposed fix
const borderLeft = px(zoneStyle.borderLeftWidth); const borderTop = px(zoneStyle.borderTopWidth); + const borderRight = px(zoneStyle.borderRightWidth); + const borderBottom = px(zoneStyle.borderBottomWidth); // Empty zone: start after the zone's leading padding. main = horizontal ? reversed - ? zoneRect.right - px(zoneStyle.paddingRight) - : zoneRect.left + px(zoneStyle.paddingLeft) + ? zoneRect.right - borderRight - px(zoneStyle.paddingRight) + : zoneRect.left + borderLeft + px(zoneStyle.paddingLeft) : reversed - ? zoneRect.bottom - px(zoneStyle.paddingBottom) - : zoneRect.top + px(zoneStyle.paddingTop); + ? zoneRect.bottom - borderBottom - px(zoneStyle.paddingBottom) + : zoneRect.top + borderTop + px(zoneStyle.paddingTop); if (horizontal) { setStyle({ top: - (closest?.rect.top ?? zoneRect.top + px(zoneStyle.paddingTop)) - + (closest?.rect.top ?? + zoneRect.top + borderTop + px(zoneStyle.paddingTop)) - zoneRect.top + zoneEl.scrollTop - borderTop, ... } else { setStyle({ left: - (closest?.rect.left ?? zoneRect.left + px(zoneStyle.paddingLeft)) - + (closest?.rect.left ?? + zoneRect.left + borderLeft + px(zoneStyle.paddingLeft)) - zoneRect.left + zoneEl.scrollLeft - borderLeft,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/components/DropZone/LinePlaceholder.tsx` around lines 114 - 154, Update the empty-zone fallback coordinates in the LinePlaceholder positioning logic so they include the applicable leading border width before the existing border subtraction, keeping the fallback aligned with the content box. Account for reversed flow by using the trailing border width where appropriate, in both horizontal and vertical branches; leave coordinates derived from closest.rect unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/core/components/DropZone/LinePlaceholder.tsx`:
- Around line 114-154: Update the empty-zone fallback coordinates in the
LinePlaceholder positioning logic so they include the applicable leading border
width before the existing border subtraction, keeping the fallback aligned with
the content box. Account for reversed flow by using the trailing border width
where appropriate, in both horizontal and vertical branches; leave coordinates
derived from closest.rect unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 46200930-896d-439f-b1e0-93cc0320982e
📒 Files selected for processing (1)
packages/core/components/DropZone/LinePlaceholder.tsx
4e01c60 to
7244e38
Compare
8ff832c to
0a460b2
Compare
0a460b2 to
9b78301
Compare
chrisvxd
left a comment
chrisvxd
left a comment
There was a problem hiding this comment.
Unfortunately I found one more issue.
435a2b2 to
3f0cf14
Compare
3f0cf14 to
ff2878d
Compare
2 participants
This PR builds on top of #1735 and addresses a few bugs, the PR comments, and re-organizes the architecture while keeping the same behavior. It also rebases from main and resolves the conflicts we had.
---- Original PR description:
Closes #1358, closes #1286
Description
This PR changes the default drag-and-drop behavior when dragging between slots to use a static placeholder with a line preview. When dragging within the same slot, the existing behavior remains.
This is configurable via the
dnd.behaviorAPI, which can beauto(the default, changing based on reparenting),fluid(the original) orstatic.How to test
?dndBehavior="fluid"on the demo URL to restore the legacy behavior?dndBehavior="static"on the demo URL to enable static behavior for everythingIn code, set the
dnd.behaviorproperty to change behavior:Remaining considerations
behavior, so if you have any suggestions, I'm all ears.Summary by CodeRabbit