useTreeData: move() silently drops a node moved into its own subtree; moveBefore/moveAfter miss root-level nodes

[ByronDWall]

Provide a general summary of the issue here

useTreeData's move() has no guard against moving a node into its own subtree. moveBefore() and moveAfter() have a guard, but it skips root-level moved nodes. Both gaps silently destroy the moved node and all its descendants. Drag-and-drop is not affected; useDroppableCollectionState's getDropOperation correctly cancels these targets. This is purely an imperative-API problem.

Expected Behavior?

All three mutations should throw when the destination is inside the moved node's own subtree. moveBefore/moveAfter already throw "Cannot move an item to be a child of itself." for non-root moved nodes. move() should do the same, and the root-level gap in moveBefore/moveAfter should be closed.

Current Behavior

move(key, toParentKey, index) has no guard. It removes the node from the map via updateTree(items, key, () => null), then calls updateTree(newItems, toParentKey, ...) to re-insert it. The removal already deleted toParentKey from the map (it was inside the moved subtree), so the lookup in updateTree finds nothing (originalMap.get(key) returns undefined) and returns the tree without the node. Silent data loss, no error.

moveBefore(key, keys) / moveAfter(key, keys) have a guard with a boundary bug. Both delegate to moveItems(), which walks the destination's ancestor chain:

while (parent?.parentKey != null) {
  if (removeKeys.has(parent.key)) {
    throw new Error('Cannot move an item to be a child of itself.');
  }
  parent = nodeMap.get(parent.parentKey!) ?? null;
}

For a root-level node, parentKey is null, so the while condition is false on entry and the body never executes. removeKeys.has(parent.key) is never evaluated. Calling moveBefore('B', ['A']) where A is root-level and B is A's child silently drops A and everything under it. Non-root moved nodes are caught correctly.

Context

We maintain a tree component built on useTreeData and React Aria Components' Tree. Our hook exposes move/moveBefore/moveAfter as an imperative controller for programmatic edits (keyboard shortcuts, context menus, external integrations). During testing we discovered that tree.move(nodeKey, childKey, 0) silently destroyed the node and its entire subtree. We have added our own pre-call guard, but the fix belongs upstream since the gap affects anyone using useTreeData's imperative API directly.

Steps to Reproduce

import { useTreeData } from 'react-stately';

function Demo() {
  const tree = useTreeData({
    initialItems: [
      { id: 'A', children: [{ id: 'B', children: [] }] },
      { id: 'C', children: [] },
    ],
    getKey: (item) => item.id,
    getChildren: (item) => item.children,
  });

  return (
    <div>
      <pre>{JSON.stringify(tree.items.map(n => n.key))}</pre>

      {/* Bug 1: move() has no guard. A and B silently disappear. */}
      <button onClick={() => tree.move('A', 'B', 0)}>
        move A into B (expect error, get silent loss)
      </button>

      {/* Bug 2: moveBefore() root gap. A and B silently disappear. */}
      <button onClick={() => tree.moveBefore('B', ['A'])}>
        moveBefore B with A (expect error, get silent loss)
      </button>
    </div>
  );
}

Click either button. The display changes from ["A","C"] to ["C"]. No error in the console.

Version

3.49.0

Browsers

• Chrome
• Firefox
• Safari
• Microsoft Edge

Operating System

All (state-management bug, not browser-specific)

Possible Solution

For moveItems, check the final parent after the loop exits:

while (parent?.parentKey != null) {
  if (removeKeys.has(parent.key)) {
    throw new Error('Cannot move an item to be a child of itself.');
  }
  parent = nodeMap.get(parent.parentKey!) ?? null;
}
// Check the root-level node the loop stopped at
if (parent != null && removeKeys.has(parent.key)) {
  throw new Error('Cannot move an item to be a child of itself.');
}

For move, add an equivalent walk before the remove-and-insert:

move(key, toParentKey, index) {
  setItems(({ items, nodeMap }) => {
    let current = toParentKey;
    while (current != null) {
      if (current === key) {
        throw new Error('Cannot move an item to be a child of itself.');
      }
      current = nodeMap.get(current)?.parentKey ?? null;
    }
    // ... existing logic
  });
}

Your Company/Team

commercetools