Add drag-and-drop reordering for schema form arrays#29
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 adds drag-and-drop reordering functionality to the schema form component. Three Changes
Sequence DiagramsequenceDiagram
actor User
participant UI as Schema Form<br/>Array List
participant DnD as DndContext<br/>SortableContext
participant Item as Array Item<br/>Component
participant State as Local State<br/>(localKeyedValues)
User->>Item: Grabs drag handle (GripVertical)
Item->>DnD: Drag initiated (useSortable)
Item->>Item: Apply drag styling<br/>(transform, opacity)
User->>DnD: Drops on new position
DnD->>UI: handleDragEnd triggered<br/>(active, over)
UI->>State: Swap items in array<br/>(localKeyedValues)
State->>UI: State updated
UI->>UI: onValuesChange called<br/>(propagate changes)
UI->>Item: Re-render with new order
Item->>User: List reordered visually
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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: handleDragEnd is calling moveItem with arguments reversed
which causes items to be moved in the wrong direction; locate the handleDragEnd
function handling DragEndEvent and when both oldIndex and newIndex are valid,
call moveItem with the source index first and destination second (i.e., pass
oldIndex then newIndex) so the signature moveItem(from: number, to: number) is
respected; ensure you reference localKeyedValues, active.id and over.id to
compute oldIndex/newIndex and update the single call to moveItem accordingly.
🧹 Nitpick comments (3)
ui-v2/src/components/schemas/schema-form-input-array-list.tsx (2)
45-48: Remove unusedprefixItemsCountvariable.This variable is computed but never used in the component.
♻️ Proposed fix
- const prefixItemsCount = isArray(property.prefixItems) - ? property.prefixItems.length - : 0; -
147-148: Misleading comment:sortableKeysincludes all items, not just non-prefix items.The comment states "non-prefix items" but
sortableKeyscontains keys for all items. The drag restriction for prefix items is correctly handled by thedisabledprop inuseSortablewithinSchemaFormInputArrayItem, so functionality is correct—but the comment should be updated or removed to avoid confusion.♻️ Proposed fix
- // Get the keys of items that can be dragged (non-prefix items) + // Keys for SortableContext; prefix items have drag disabled via useSortable const sortableKeys = localKeyedValues.map((item) => item.key);ui-v2/src/components/schemas/schema-form-input-array-item.tsx (1)
83-92: Consider adding a visible focus indicator for keyboard accessibility.The
focus:outline-noneclass removes the focus indicator, which can make keyboard navigation difficult for users relying on visual focus cues. Consider adding a focus-visible style instead.♻️ Proposed fix
<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.snapshot.test.tsx (1)
ui-v2/src/components/schemas/stories/utilities.tsx (1)
TestSchemaForm(8-50)
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)
⏰ 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 (3)
- GitHub Check: Test (1)
- GitHub Check: Test (2)
- GitHub Check: Pre-commit checks
- GitHub Check: E2E Tests
🔇 Additional comments (6)
ui-v2/src/components/schemas/schema-form.snapshot.test.tsx (1)
488-565: LGTM! Well-structured snapshot tests for array drag-and-drop functionality.The tests appropriately cover key scenarios:
- Base array rendering
- Arrays with values (verifying drag handles appear)
- Arrays with prefixItems (verifying drag handles don't appear on prefix items)
- Arrays with enum items
Good adherence to the file's note about adding new tests at the end to avoid order-dependent
useIdissues.ui-v2/src/components/schemas/schema-form-input-array-list.tsx (2)
49-54: LGTM! Sensor configuration follows@dnd-kitbest practices.Proper setup of both
PointerSensorandKeyboardSensorwithsortableKeyboardCoordinatesfor accessibility.
156-182: LGTM! DndContext and SortableContext integration is correct.The drag-and-drop context properly wraps the sortable items, and the
itemKeyprop is correctly passed to enable sortable identification.ui-v2/src/components/schemas/schema-form-input-array-item.tsx (2)
45-58: LGTM! CorrectuseSortableintegration with proper drag state styling.The hook is properly configured with
disabled: !canMovefor prefix items, and the style computation correctly handles transform, transition, and opacity feedback during drag operations.
77-95: LGTM! Well-structured drag handle with appropriate accessibility attributes.The implementation correctly:
- Uses
setNodeReffor sortable DOM binding- Applies dynamic styles for drag feedback
- Renders a placeholder spacer when dragging is disabled (prefix items)
- Includes
aria-labelon the drag handleui-v2/package.json (1)
32-34: No action needed. The@dnd-kitpackages are properly versioned and compatible.The specified versions are designed to work together:
@dnd-kit/sortable@10.0.0requires@dnd-kit/core@^6.3.0, and the package.json correctly specifies@dnd-kit/core@^6.3.1. Similarly,@dnd-kit/utilities@3.2.2is compatible with core v6. All three packages meet their peer dependencies and work with React 19.2.3.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: moveItem arguments are swapped, causing incorrect reorder direction.
The moveItem function signature is moveItem(from: number, to: number) (Line 117), but handleDragEnd passes arguments in reverse order: moveItem(newIndex, oldIndex). This will move the item in the wrong direction—moving FROM the drop position TO the original position instead of the intended behavior.
🐛 Proposed fix
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, handleDragEnd is calling moveItem with arguments reversed which
causes items to be moved in the wrong direction; locate the handleDragEnd
function handling DragEndEvent and when both oldIndex and newIndex are valid,
call moveItem with the source index first and destination second (i.e., pass
oldIndex then newIndex) so the signature moveItem(from: number, to: number) is
respected; ensure you reference localKeyedValues, active.id and over.id to
compute oldIndex/newIndex and update the single call to moveItem accordingly.
1 participant
Benchmark PR from agentic-review-benchmarks#3
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.