Add drag-and-drop reordering for schema form arrays

[tomerqodo]

Benchmark PR from agentic-review-benchmarks#3

[claude]

🔴 handleDragEnd calls moveItem(newIndex, oldIndex) but moveItem(from, to) expects the source index first and destination second. This means dragging an item moves the target to the source position instead of the other way around. Fix: swap to moveItem(oldIndex, newIndex).

Extended reasoning...

What the bug is

The moveItem function on line 117 has the signature moveItem(from: number, to: number) and implements reordering by removing the item at index from and inserting it at index to via newValues.splice(to, 0, newValues.splice(from, 1)[0]).

In handleDragEnd (line 142), the call is moveItem(newIndex, oldIndex), which passes from=newIndex (the drop target position) and to=oldIndex (the dragged item's original position). This is backwards.

Confirmation from existing callers

The correct argument order is confirmed by the existing moveUp and moveDown callbacks on line 172-173:

• moveUp={() => moveItem(index, index - 1)} — moves the item FROM its current index TO one position earlier
• moveDown={() => moveItem(index, index + 1)} — moves the item FROM its current index TO one position later

Both correctly pass the current position as from and the destination as to.

Step-by-step proof

Consider an array [A, B, C, D] where the user drags item A (index 0) to the position of item C (index 2):

1. active.id resolves to A's key, so oldIndex = 0
2. over.id resolves to C's key, so newIndex = 2
3. The code calls moveItem(2, 0) — meaning from=2, to=0
4. splice(2, 1) removes C → array becomes [A, B, D]
5. splice(0, 0, C) inserts C at position 0 → array becomes [C, A, B, D]

The user intended to move A to index 2, expecting [B, C, A, D], but got [C, A, B, D] — the opposite operation.

Impact

Every drag-and-drop reorder operation in schema form arrays will produce the wrong result. The item at the drop target moves to the drag source instead of the dragged item moving to the drop target. This completely breaks the drag-and-drop feature being added in this PR.

Fix

Change line 142 from moveItem(newIndex, oldIndex) to moveItem(oldIndex, newIndex).

[claude]

🟡 sortableKeys (line 148) includes ALL items despite the comment saying "non-prefix items" — prefixItemsCount is computed on lines 45-47 but never used. This means prefix items are registered with SortableContext, allowing verticalListSortingStrategy to compute transforms for them during drags and closestCenter to resolve them as drop targets, which can cause visual glitches and potentially allow items to be reordered into prefix positions. Fix: localKeyedValues.filter((_, i) => i >= prefixItemsCount).map((item) => item.key).

Extended reasoning...

What the bug is

On line 147, the comment reads // Get the keys of items that can be dragged (non-prefix items), but line 148 does localKeyedValues.map((item) => item.key) which includes ALL items — prefix items and non-prefix items alike. The variable prefixItemsCount is computed on lines 45-47 but is never referenced anywhere else in the file, which is strong evidence that the developer intended to use it for filtering but forgot to implement the filter.

How it manifests

When a schema has prefixItems (fixed-position items at the start of an array), all items including prefix items are passed to <SortableContext items={sortableKeys}>. While useSortable({ disabled: !canMove }) in SchemaFormInputArrayItem prevents prefix items from being picked up for dragging, having them in the SortableContext items array means:

1. The verticalListSortingStrategy computes sort transforms for ALL items including prefix items, which can cause prefix items to visually shift/animate during drag operations even though they should remain fixed in place.
2. The closestCenter collision detection algorithm considers prefix item positions when determining drop targets. Since disabled on useSortable primarily affects the draggable/activator behavior, the droppable aspect may still participate in collision resolution depending on the dnd-kit version.

Step-by-step proof

Consider a schema with 2 prefix items and 2 regular items, giving us localKeyedValues = [{key: "a", ...}, {key: "b", ...}, {key: "c", ...}, {key: "d", ...}] where "a" and "b" are prefix items.

1. prefixItemsCount is computed as 2 (line 45-47) but never used.
2. sortableKeys becomes ["a", "b", "c", "d"] — all four keys (line 148).
3. SortableContext receives all four keys as its items prop.
4. When the user drags item "c", verticalListSortingStrategy calculates position transforms for all four items, potentially causing prefix items "a" and "b" to animate/shift.
5. If the user drops "c" near prefix item "a", handleDragEnd fires with over.id = "a", and moveItem would reorder the array, displacing the prefix item from its fixed position.

The correct behavior is that sortableKeys should only contain ["c", "d"] so that prefix items are excluded from sort calculations entirely.

Why existing code does not prevent it

While useSortable({ disabled: true }) on prefix items prevents them from being dragged, it does not remove them from the SortableContext items list. The sorting strategy still tracks all registered items for position calculations. The disabled flag is a partial mitigation — it prevents picking up prefix items but does not fully isolate them from the sort system.

Impact

The practical impact is mitigated by disabled: true preventing prefix items from being dragged. However, prefix items may still visually shift during drag operations, and in certain drag scenarios, items could potentially be dropped at prefix positions. The dead prefixItemsCount variable is also a code quality issue.

Fix

Replace line 148 with:

const sortableKeys = localKeyedValues.filter((_, i) => i >= prefixItemsCount).map((item) => item.key);

This uses the already-computed prefixItemsCount to exclude prefix items from the sortable context, matching the intent described in the comment.

Note on duplicate concern

One verifier flagged this as a duplicate of bug_003 (unused prefixItemsCount). While they share the same root cause, this bug report specifically addresses the incorrect sortableKeys computation and its behavioral impact on drag-and-drop, which is the actionable code defect. The unused variable is evidence of the incomplete implementation rather than a separate bug.