Skip to content

Fix useTreeData subtree move validation - #10560

Open
su-jin1425 wants to merge 4 commits into
adobe:mainfrom
su-jin1425:fix-usetreedata-move-bug
Open

Fix useTreeData subtree move validation#10560
su-jin1425 wants to merge 4 commits into
adobe:mainfrom
su-jin1425:fix-usetreedata-move-bug

Conversation

@su-jin1425

Copy link
Copy Markdown

Description

Closes #10539

Summary

Prevent useTreeData from silently dropping nodes when an item is moved into its own subtree.

This adds validation to the imperative move, moveBefore, and moveAfter APIs so that moves targeting a node's own descendants throw:

Cannot move an item to be a child of itself.

The validation also covers root-level nodes, which were previously missed by moveBefore and moveAfter.

Changes

  • Added ancestor validation to move().
  • Fixed root-level ancestor validation in moveBefore() and moveAfter().
  • Added isolated regression tests covering nested and root-level self-subtree moves for all three APIs.
  • No changes to drag-and-drop behavior.

Pull Request Checklist:

Test Instructions:

Run the useTreeData test suite:

yarn jest useTreeData

The regression tests verify that move, moveBefore, and moveAfter throw when attempting to move an item into its own subtree, including root-level ancestor cases.

Your Project:

None

@su-jin1425
su-jin1425 force-pushed the fix-usetreedata-move-bug branch from d305b81 to 6bc74bf Compare September 3, 2026 10:45
reidbarber
reidbarber previously approved these changes Sep 3, 2026
@su-jin1425
su-jin1425 force-pushed the fix-usetreedata-move-bug branch from 6bc74bf to 0153b79 Compare September 5, 2026 01:18
snowystinger
snowystinger previously approved these changes Sep 6, 2026
expect(e.toString()).toContain('Cannot move an item to be a child of itself.');
}

// In React 17, act() does not synchronously re-throw errors from state updaters.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, change it to this:

    let reactMajor = parseInt(React.version, 10);
    let expectMoveError = (result, action) => {
      if (reactMajor >= 18) {
        expect(() => act(action)).toThrow('Cannot move an item to be a child of itself.');
      } else {
        act(action);
        expect(result.error?.message).toContain('Cannot move an item to be a child of itself.');
      }
    };

and put the change I made to source back or explain why it doesn't work

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the cleaner test helper! I've updated the tests to use expectMoveError.

My previous commit reverting your while (parent != null) logic was just an accidental git staging mistake while debugging locally.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I saw Ci fail I suspected your change to the while loop in useTreeData.ts might have caused it.
To test this theory, I ran git checkout aef9f68 packages/react-stately/src/data/useTreeData.ts to see if it fixed the test failures locally. eventually figured out the actual issue was a yarn cache problem locally However, I forgot to un-stage my temporary checkout of useTreeData.ts. So when I committed the test fixes

Thank you for correcting me.

@su-jin1425
su-jin1425 force-pushed the fix-usetreedata-move-bug branch from e61d1e1 to 9cc97f0 Compare September 7, 2026 02:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

3 participants