ENG-3398: Fix column reorder drop target in Datamap report modal#7905
ENG-3398: Fix column reorder drop target in Datamap report modal#7905gilluminate merged 3 commits intomainfrom
Conversation
Split the single ref used by react-dnd so the drop target covers the whole row (not just the 20px drag handle). Hover and drop now register anywhere on the row, eliminating the visual "revert" users saw when releasing slightly off the handle. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review: Fix column reorder drop target in Datamap report modal
Verdict: Good to merge with one minor optional cleanup noted inline.
What the PR does
Splits the single ref (which was simultaneously drag source and drop target) into two separate refs:
rowRef— attached to the full<Flex>row, used as the drop target and drag preview anchordragRef— attached to the handle icon<div>only, used as the drag source
This correctly fixes the root cause: the HTML5 DnD backend was registering drops only within the 20px handle icon bounding box instead of anywhere on the row. The split-ref pattern is the standard react-dnd solution for this class of bug.
What's correct
drop(rowRef)+drag(dragRef)+preview(rowRef)is the right call order;previewis correctly moved from JSX-level to inside the hook body and removed from the hook's return value.data-handler-id={handlerId}stays on the<Flex>(the drop target element), which is required for react-dnd's DOM correlation.- The comment explaining the two refs (
// rowRef is the drop target...) explains why, which is exactly what makes this maintainable. - Blast radius is limited to this component and its single consumer (
DraggableColumnList.tsx).
Suggestions
Inline (line 59): The ?. optional chain on rowRef.current is redundant after the null guard — see inline comment for the one-character fix. Pre-existing, but touched directly by this PR.
Follow-up (no action needed for merge):
- There are no test files for this component. A drag-and-drop integration test (e.g., Playwright or
@testing-library/user-event) would prevent this class of regression from silently reappearing. - The two inline
styleprops (opacity,cursor) are pre-existing candidates for Tailwind utility classes (opacity-20,cursor-grab/cursor-grabbing), but that's a separate cleanup.
🔬 Codegraph: connected (46363 nodes)
💡 Write /code-review in a comment to re-run this review.
| } | ||
|
|
||
| const hoverBoundingRect = ref.current?.getBoundingClientRect(); | ||
| const hoverBoundingRect = rowRef.current?.getBoundingClientRect(); |
There was a problem hiding this comment.
clients/admin-ui/src/features/common/table/column-settings/DraggableColumnListItem.tsx:59
The optional chaining (?.) here is redundant — rowRef.current is already guaranteed non-null by the if (!rowRef.current) { return; } guard on the line above this hunk. As written, hoverBoundingRect infers as DOMRect | undefined, even though it can never actually be undefined at runtime. It's a one-character fix:
const hoverBoundingRect = rowRef.current.getBoundingClientRect();This is pre-existing, but the PR renames ref → rowRef on this exact line, so it's the natural moment to clean it up.
|
lucanovera
left a comment
lucanovera
left a comment
There was a problem hiding this comment.
Confirmed the original behavior on nightly and compared it to the fix in this branch. Nice improvement, the reorder is way easier with the new larger target. Approved 💯
2 participants
Ticket ENG-3398
Description Of Changes
In the Datamap report's "Edit columns" modal, dragging a column to a new position appeared to revert before succeeding when the drop released slightly off the drag-handle icon. Root cause:
drag(drop(ref))inDraggableColumnListItemapplied both the drag source and drop target to the samerefon the 20px drag-handle<div>. Hover and drop only fired inside the handle's bounding box, so any release a few pixels to the side fell outside the drop target and the HTML5 backend cancelled the drop, snapping the native drag ghost back to its origin.Fix: split the single ref into two:
rowRef— drop target + drag preview on the full row<Flex>(hover fires anywhere on the row)dragRef— drag source on the handle icon (preserves the grab-handle UX)With the drop target now covering the whole row, drops register at the cursor wherever the user releases, the native ghost lands cleanly, and the reorder completes without the visual revert.
Code Changes
clients/admin-ui/src/features/common/table/column-settings/DraggableColumnListItem.tsx: splitrefintorowRef(drop target +preview) anddragRef(drag source); update hover handler to measure againstrowRef.Steps to Confirm
Pre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works