Add drag-and-drop reordering for schema form arrays#16
Conversation
Co-Authored-By: alex.s@prefect.io <ajstreed1@gmail.com>
- Add snapshot tests for arrays with drag handles visible - Add snapshot tests for prefixItems without drag handles - Add behavioral tests for move up/down menu options - Add test verifying prefix items cannot be moved Co-Authored-By: alex.s@prefect.io <ajstreed1@gmail.com>
WalkthroughThis pull request introduces drag-and-drop functionality to schema form arrays. Three Changes
Sequence DiagramsequenceDiagram
actor User
participant DndContext as DndContext<br/>(Sensors)
participant SchemaFormInputArrayList as SchemaFormInputArrayList<br/>(SortableContext)
participant SchemaFormInputArrayItem as SchemaFormInputArrayItem<br/>(Sortable Item)
participant State as Array State
User->>SchemaFormInputArrayItem: Click + Hold GripVertical
SchemaFormInputArrayItem->>DndContext: Pointer/Keyboard Sensor triggered
DndContext->>DndContext: Track drag movement
User->>DndContext: Drag item over another item
DndContext->>DndContext: Detect drop target
User->>DndContext: Release mouse/pointer
DndContext->>SchemaFormInputArrayList: onDragEnd event
SchemaFormInputArrayList->>SchemaFormInputArrayList: handleDragEnd: compute indices
SchemaFormInputArrayList->>State: moveItem(oldIndex, newIndex)
State->>SchemaFormInputArrayList: Array reordered
SchemaFormInputArrayList->>SchemaFormInputArrayItem: Re-render with new order
SchemaFormInputArrayItem->>User: Updated UI with reordered items
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 1
🤖 Fix all issues with AI agents
In `@ui-v2/src/components/schemas/schema-form-input-array-list.tsx`:
- Around line 130-145: The drag handler in handleDragEnd is calling moveItem
with the indices reversed causing items to be moved the wrong direction; locate
handleDragEnd (using localKeyedValues, active.id and over.id) and change the
moveItem call to pass the source index first and destination second (i.e., call
moveItem(oldIndex, newIndex) instead of moveItem(newIndex, oldIndex)) so the
dragged item is moved from oldIndex to newIndex.
🧹 Nitpick comments (3)
ui-v2/src/components/schemas/schema-form-input-array-list.tsx (2)
45-47: Unused variable:prefixItemsCountis never referenced.This variable is calculated but not used anywhere in the component. The prefix item check is performed inline in
getCanMoveForIndex(lines 93-98).♻️ Remove unused variable
- const prefixItemsCount = isArray(property.prefixItems) - ? property.prefixItems.length - : 0; - const sensors = useSensors(
147-148: Misleading comment: all keys are included, not just non-prefix items.The comment states "keys of items that can be dragged (non-prefix items)" but the code includes all item keys. This is actually correct—
SortableContextneeds all items for position tracking, while individual items disable dragging viauseSortable({ disabled: !canMove }). Consider removing or correcting the comment.♻️ Fix or remove misleading comment
- // Get the keys of items that can be dragged (non-prefix items) + // All items must be in SortableContext; dragging is disabled per-item via useSortable const sortableKeys = localKeyedValues.map((item) => item.key);ui-v2/src/components/schemas/schema-form-input-array-item.tsx (1)
83-95: Consider adding a visible focus indicator for keyboard accessibility.The drag handle button uses
focus:outline-nonewhich removes the default focus ring. For keyboard users, this may make it difficult to see which element is focused. Consider adding a custom focus style.♻️ Add focus-visible style
<button type="button" - className="flex items-center justify-center cursor-grab text-muted-foreground hover:text-foreground focus:outline-none" + className="flex items-center justify-center cursor-grab text-muted-foreground hover:text-foreground focus:outline-none focus-visible:ring-2 focus-visible:ring-ring focus-visible:ring-offset-2" aria-label="Drag to reorder" {...attributes} {...listeners} >
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
ui-v2/package-lock.jsonis excluded by!**/package-lock.jsonui-v2/src/components/schemas/__snapshots__/schema-form.snapshot.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (4)
ui-v2/package.jsonui-v2/src/components/schemas/schema-form-input-array-item.tsxui-v2/src/components/schemas/schema-form-input-array-list.tsxui-v2/src/components/schemas/schema-form.snapshot.test.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
ui-v2/src/components/schemas/schema-form-input-array-list.tsx (2)
ui-v2/src/components/schemas/utilities/guards.ts (1)
isArray(21-23)ui-v2/src/components/schemas/schema-form-input-array-item.tsx (1)
SchemaFormInputArrayItem(29-121)
ui-v2/src/components/schemas/schema-form.snapshot.test.tsx (1)
ui-v2/src/components/schemas/stories/utilities.tsx (1)
TestSchemaForm(8-50)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Test (4)
- GitHub Check: Test (2)
- GitHub Check: Test (1)
- GitHub Check: Test (3)
- GitHub Check: E2E Tests (Auth Enabled)
- GitHub Check: Pre-commit checks
🔇 Additional comments (5)
ui-v2/src/components/schemas/schema-form.snapshot.test.tsx (1)
489-565: LGTM! Good test coverage for the new drag-and-drop feature.The new array test cases appropriately cover:
- Base rendering (empty array)
- Array with values (verifying drag handles are visible)
- Arrays with prefixItems (verifying drag handles are not shown on prefix items)
- Arrays with enum items
The tests are correctly placed at the end of the file, respecting the order-dependency constraint noted in the file's header comments.
ui-v2/src/components/schemas/schema-form-input-array-list.tsx (1)
156-182: LGTM! DnD context setup follows best practices.The sensor configuration with
PointerSensorandKeyboardSensorprovides good accessibility support. UsingclosestCentercollision detection andverticalListSortingStrategyis appropriate for this vertical list reordering use case.ui-v2/src/components/schemas/schema-form-input-array-item.tsx (2)
45-58: LGTM! Sortable hook integration is correct.The
useSortablehook is properly configured withitemKeyas the identifier and respects thecanMoveprop via thedisabledoption. The style computation withCSS.Transform.toStringand opacity feedback during drag follows@dnd-kitconventions.
77-82: LGTM! Clean DOM structure for sortable items.The grid layout correctly accommodates the drag handle column, and the
setNodeRefandstyleare properly applied to the container for drag-and-drop transforms.ui-v2/package.json (1)
32-34: @dnd-kit versions are compatible.The
@dnd-kitpackages use independent versioning.@dnd-kit/sortablev10.0.0 requires@dnd-kit/core^6.3.0, and the specified version@dnd-kit/core^6.3.1 satisfies this requirement.Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| function handleDragEnd(event: DragEndEvent) { | ||
| const { active, over } = event; | ||
|
|
||
| if (over && active.id !== over.id) { | ||
| const oldIndex = localKeyedValues.findIndex( | ||
| (item) => item.key === active.id, | ||
| ); | ||
| const newIndex = localKeyedValues.findIndex( | ||
| (item) => item.key === over.id, | ||
| ); | ||
|
|
||
| if (oldIndex !== -1 && newIndex !== -1) { | ||
| moveItem(newIndex, oldIndex); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical bug: moveItem arguments are swapped.
The moveItem(from, to) function expects the source index first, then the destination index. The current call moveItem(newIndex, oldIndex) moves the item from the drop position back to the original position, reversing the intended drag direction.
🐛 Proposed fix
function handleDragEnd(event: DragEndEvent) {
const { active, over } = event;
if (over && active.id !== over.id) {
const oldIndex = localKeyedValues.findIndex(
(item) => item.key === active.id,
);
const newIndex = localKeyedValues.findIndex(
(item) => item.key === over.id,
);
if (oldIndex !== -1 && newIndex !== -1) {
- moveItem(newIndex, oldIndex);
+ moveItem(oldIndex, newIndex);
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function handleDragEnd(event: DragEndEvent) { | |
| const { active, over } = event; | |
| if (over && active.id !== over.id) { | |
| const oldIndex = localKeyedValues.findIndex( | |
| (item) => item.key === active.id, | |
| ); | |
| const newIndex = localKeyedValues.findIndex( | |
| (item) => item.key === over.id, | |
| ); | |
| if (oldIndex !== -1 && newIndex !== -1) { | |
| moveItem(newIndex, oldIndex); | |
| } | |
| } | |
| } | |
| function handleDragEnd(event: DragEndEvent) { | |
| const { active, over } = event; | |
| if (over && active.id !== over.id) { | |
| const oldIndex = localKeyedValues.findIndex( | |
| (item) => item.key === active.id, | |
| ); | |
| const newIndex = localKeyedValues.findIndex( | |
| (item) => item.key === over.id, | |
| ); | |
| if (oldIndex !== -1 && newIndex !== -1) { | |
| moveItem(oldIndex, newIndex); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@ui-v2/src/components/schemas/schema-form-input-array-list.tsx` around lines
130 - 145, The drag handler in handleDragEnd is calling moveItem with the
indices reversed causing items to be moved the wrong direction; locate
handleDragEnd (using localKeyedValues, active.id and over.id) and change the
moveItem call to pass the source index first and destination second (i.e., call
moveItem(oldIndex, newIndex) instead of moveItem(newIndex, oldIndex)) so the
dragged item is moved from oldIndex to newIndex.
1 participant
Benchmark PR from qodo-benchmark#555
Summary by CodeRabbit
Release Notes
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.