Skip to content

Conversation

@lixiaoyan
Copy link
Contributor

collection.getKeys() doesn’t guarantee the correct order.

Closes

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

🧢 Your Project:

`collection.getKeys()` doesn’t guarantee the correct order.
Copy link
Member

@reidbarber reidbarber left a comment

Choose a reason for hiding this comment

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

Looks like there is a failing test.

Can you say more about what issue this was causing?

@lixiaoyan
Copy link
Contributor Author

@reidbarber
I’m not entirely sure why the tests failed; a quick look didn't reveal the root cause. I’ll need to investigate further tomorrow. In the meantime, I’ve added a story to reproduce the issue. Below are the "before" and "after" recordings.

Before After
Screencast.From.2026-01-29.01-19-35.mp4
Screencast.From.2026-01-29.01-19-06.mp4

return itemNode?.type === 'item' ? itemNode : null;
}
).filter(node => node !== null);
const cachedItemNodes = Array.from(collection).filter(itemNode => itemNode.type === 'item');
Copy link
Contributor

Choose a reason for hiding this comment

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

This is probably the typo causing the test failure. This should be using cachedCollection.current.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that was a typo. But technically, it shouldn't impact single-item deletes because only the cachedCollection length is used to gate multi-item deletion. I've applied the fix, but the test is still failing.

@snowystinger
Copy link
Member

Thanks for the PR, I've opened a new one #9545 that fixes the test and adds a test instead of storybook so we are less likely to regress.

There was a little more complexity to the keys in a collection with section and the logic was also flawed moving focus by all the items removed instead of only the ones before. I think we were getting lucky earlier with this test passing, though it was also just a really hard to follow test that did a bunch of things that wouldn't happen in real life.

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.

4 participants