Conversation
Two connected elements with the same key overwrite each other in the collection's keyMap while their siblings still link to that key, so the sibling chain either drops items silently or loops forever when walked backwards. Track which element owns each key in the collection Document and throw in development when a different connected element claims it. The GridList sections fixture reused the animal ids for the ice cream section and only rendered because the duplicates were dropped.
Removed and hidden elements release their key in removeNode before any node is added, so the owner check only needs to compare elements; the isConnected and isHidden clauses were redundant. Cover the hidden subtree case: an item can move between a hidden and a visible Activity with the same key, and revealing a hidden duplicate next to a visible one throws. Skip the owner bookkeeping in production, where the check is off.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #8033
Related: #8886
Summary
Intent. Make duplicate keys in a collection fail loudly during development instead of corrupting the collection. This is the check discussed in #8033 ("check if the key already exists in the keyMap and throw if it does") and #8886 ("bailing out and throwing a warning").
Why the earlier attempt broke tests.
BaseCollection.addNodeis the update-or-insert path: every dirty element is re-added on each update, so a naive "key already inkeyMap" check fires on ordinary re-renders. What actually corrupts the collection is two different connected elements claiming the same key. With['a', 'a', 'b']onmain,getKeyBefore('a')returns'a'(a self-loop, which is the infinite iteration and out-of-memory crash in #8033); with['a', 'x', 'a'], iterating the collection yields only['a']whilesizereports 2 (the silent drop).How.
Documentnow records whichElementNodeowns each key when it adds the node to the collection. InaddNode, if a different element already owns the key, it throwsDuplicate key "…" found in collection. Every item in a collection must have a unique key.The owner entry is released inremoveNodeand cleared inresetAfterSSR. BecauseupdateCollectionprocesses removed and hidden elements before it adds anything, a key can move to a new element when the old one is removed in the same render, and hidden (Activity) subtrees do not conflict with visible ones; the second commit drops theisConnected/isHiddenclauses the first commit carried, after confirming with mutation runs that every test stays green without them, and adds a test that pins the hidden-subtree behavior. The check runs only whenprocess.env.NODE_ENV !== 'production'; production behavior is unchanged. The error surfaces fromgetCollection()during render, so an error boundary catches it like the existingCannot change the id of an itemerror.Fixture fix.
TestGridListSectionsinGridList.test.jsreusedcat,dogandkangaroofor the ice cream section. It only rendered two groups because the duplicates were dropped; the ids are now unique.✅ Pull Request Checklist:
Notes on the checklist: no storybook change, since this is a development-time error with no visual state; no docs change, since the unique ids requirement is already documented and this enforces it. AI disclosure: this PR was drafted with Claude Code, following AGENTS.md and
docs/contributing/; I reviewed and own every change.📝 Test Instructions:
Unit tests added:
packages/react-aria/test/collections/CollectionBuilder.test.js: adjacent and separated duplicate keys throw; a new element may reuse the key of an element removed in the same render.packages/react-aria-components/test/Table.test.js: theTablefrom Table: Duplicate item id causes infinite collection and out of memory crash #8033 with twoRows sharingid="1"throws.packages/react-aria-components/test/ListBox.test.js(React 19 only): an item can move between a hidden and a visibleActivitysubtree with the same key, and revealing a hidden duplicate next to a visible item throws.Manual check: render any RAC collection (
ListBox,GridList,Table,Menu,Tree) in development with two items sharing anid. Before: the second item is dropped, or keyboard navigation loops. After: an error names the duplicate key. Rendering with unique ids, re-keying items across renders, hiding a subtree withActivity, and SSR hydration are unchanged.Ran locally:
yarn test(8285 passed; the 3 remaining failures areDatePicker/LabeledValuetimezone tests that fail identically on a cleanmaincheckout on this machine),yarn test:ssr(74 passed),yarn test:browser(400 passed; the FirefoxChatspatial navigation failure reproduces on cleanmain),yarn lint(format, types, oxlint, package lint, constraints all clean).Tested with mouse and keyboard in jsdom; no screen reader, RTL, or high-contrast surface is involved.
🧢 Your Project:
PMDB (a React Aria Components / shadcn aria application). We hit the silent-drop form of this when grouped table rows shared an id.