fix: new drag-n-drop-list component using dnd-kit lib - #279
Conversation
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
|
Warning Review limit reached
Next review available in: 48 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a new ChangesDragNDropList component addition
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant SortableItem
participant DndContext
participant DragNDropList
User->>SortableItem: pointer/keyboard drag start
SortableItem->>DndContext: drag events (active, over)
DndContext->>DragNDropList: onDragEnd(active, over)
DragNDropList->>DragNDropList: compute old/new index, arrayMove, update updateOrderKey
DragNDropList->>User: onReorder(reorderedItems)
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://cdn.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/components/mui/drag-n-drop-list.js (1)
71-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider an activation constraint on
PointerSensor.Without
activationConstraint(e.g.{ distance: 8 }), any interactive controls rendered byrenderItem(buttons, checkboxes, links) may have their click behavior swallowed by drag activation on the first pointer move.♻️ Suggested tweak
- useSensor(PointerSensor), + useSensor(PointerSensor, { activationConstraint: { distance: 8 } }),🤖 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 `@src/components/mui/drag-n-drop-list.js` around lines 71 - 74, Add an activation constraint to the PointerSensor used in useSensors so simple clicks on interactive content inside renderItem are not interpreted as drag starts. Update the sensor setup in drag-n-drop-list.js to configure PointerSensor with a small distance threshold (for example, 8px) while leaving KeyboardSensor and sortableKeyboardCoordinates unchanged.src/components/mui/__tests__/drag-n-drop-list.test.js (1)
131-179: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd regression coverage for falsy
idKeyvalues (e.g.id: 0).All fixtures here use truthy ids. Once the
getItemIdtruthy-check bug indrag-n-drop-list.js(Lines 61-62) is fixed, add a case with an item whose id is0to prevent regression.🤖 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 `@src/components/mui/__tests__/drag-n-drop-list.test.js` around lines 131 - 179, Add regression coverage in DragAndDropList id resolution tests for a falsy idKey value such as 0. Extend the existing id resolution block in drag-n-drop-list.test.js with a case that renders DragAndDropList using an item whose id (or custom idKey) is 0 and verifies getItemId still returns that id rather than falling back to new-{index}. Make sure the assertion exercises the onReorder path so the bug in getItemId is covered.
🤖 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 `@src/components/mui/drag-n-drop-list.js`:
- Around line 61-62: `getItemId` in `DragNDropList` is using a truthy check that
treats valid ids like 0 or empty string as missing, which can desync
`SortableContext` ids from `handleDragEnd` lookups and cause `arrayMove` to
reorder the wrong item. Update the id resolution in `getItemId` to check for
null/undefined instead of truthiness, and make sure `handleDragEnd` only calls
`arrayMove` when both `active.id` and `over.id` resolve to valid indexes in the
current list.
---
Nitpick comments:
In `@src/components/mui/__tests__/drag-n-drop-list.test.js`:
- Around line 131-179: Add regression coverage in DragAndDropList id resolution
tests for a falsy idKey value such as 0. Extend the existing id resolution block
in drag-n-drop-list.test.js with a case that renders DragAndDropList using an
item whose id (or custom idKey) is 0 and verifies getItemId still returns that
id rather than falling back to new-{index}. Make sure the assertion exercises
the onReorder path so the bug in getItemId is covered.
In `@src/components/mui/drag-n-drop-list.js`:
- Around line 71-74: Add an activation constraint to the PointerSensor used in
useSensors so simple clicks on interactive content inside renderItem are not
interpreted as drag starts. Update the sensor setup in drag-n-drop-list.js to
configure PointerSensor with a small distance threshold (for example, 8px) while
leaving KeyboardSensor and sortableKeyboardCoordinates unchanged.
🪄 Autofix (Beta)
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
Run ID: d7d4e316-de35-4f98-994e-301385a42329
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
package.jsonsrc/components/index.jssrc/components/mui/__tests__/drag-n-drop-list.test.jssrc/components/mui/drag-n-drop-list.jswebpack.common.js
…atched id Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR introduces a new MUI-friendly drag-and-drop list component built on dnd-kit, and wires it into the build so it can be consumed alongside existing MUI components.
Changes:
- Added
src/components/mui/drag-n-drop-list.jsimplementing a sortable list with pointer + keyboard sensors and automatic order-field updates. - Added Jest coverage for rendering, reordering behavior, id handling, and custom
updateOrderKey. - Added
@dnd-kit/*dependencies and registered the new component entry in Webpack.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| yarn.lock | Locks newly added @dnd-kit/* transitive dependencies. |
| webpack.common.js | Adds a build entry for components/mui/drag-n-drop-list. |
| src/components/mui/drag-n-drop-list.js | New dnd-kit-based sortable list component. |
| src/components/mui/tests/drag-n-drop-list.test.js | Unit tests covering reorder + id/order behaviors. |
| src/components/index.js | Adds a commented-out export note for the new component (consistent with other “3rd party deps” exports). |
| package.json | Adds @dnd-kit/* to devDependencies + peerDependencies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
santipalenque
left a comment
santipalenque
left a comment
There was a problem hiding this comment.
other than this, LGTM
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/components/mui/DragNDropList/sortable-item.js (1)
22-47: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider
React.memofor row-level performance.Each
SortableItemre-renders whenever the parentDragNDropListre-renders (e.g., on every drag-move tick if the parent re-renders items), even for rows unaffected by the current drag. Memoizing (with a comparator that accounts forrenderItemidentity) can reduce wasted renders in larger lists.🤖 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 `@src/components/mui/DragNDropList/sortable-item.js` around lines 22 - 47, Wrap SortableItem with React.memo to prevent unnecessary row re-renders during DragNDropList updates, especially on drag-move ticks. Update the SortableItem component so it only re-renders when its relevant props change, and make sure the comparator accounts for renderItem identity (and any other props that affect output) to avoid stale rendering. Keep the existing useSortable, Box, and renderItem behavior unchanged while improving row-level performance.
🤖 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 `@src/components/mui/DragNDropList/sortable-item.js`:
- Around line 22-46: In SortableItem, the dragged row only updates transform and
transition, so it can still render beneath neighboring items during a drag.
Update the Box styling in SortableItem to bump stacking order when isDragging is
true by applying a higher zIndex alongside the existing background change,
keeping the logic localized to the useSortable result and the Box wrapper.
---
Nitpick comments:
In `@src/components/mui/DragNDropList/sortable-item.js`:
- Around line 22-47: Wrap SortableItem with React.memo to prevent unnecessary
row re-renders during DragNDropList updates, especially on drag-move ticks.
Update the SortableItem component so it only re-renders when its relevant props
change, and make sure the comparator accounts for renderItem identity (and any
other props that affect output) to avoid stale rendering. Keep the existing
useSortable, Box, and renderItem behavior unchanged while improving row-level
performance.
🪄 Autofix (Beta)
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
Run ID: 0a58eba3-7faf-4105-8fb2-f83e4b269b52
📒 Files selected for processing (5)
src/components/index.jssrc/components/mui/DragNDropList/index.jssrc/components/mui/DragNDropList/sortable-item.jssrc/components/mui/__tests__/drag-n-drop-list.test.jswebpack.common.js
✅ Files skipped from review due to trivial changes (1)
- src/components/index.js
🚧 Files skipped from review as they are similar to previous changes (2)
- webpack.common.js
- src/components/mui/tests/drag-n-drop-list.test.js
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
ref: https://github.com/fntechgit/sponsor-services/pull/57
from https://github.com/fntechgit/sponsor-services/pull/57#discussion_r3505871793
Signed-off-by: Tomás Castillo tcastilloboireau@gmail.com
Summary by CodeRabbit
New Features
Tests