Skip to content

fix: throw on duplicate keys in collections - #10605

Open
greglum wants to merge 2 commits into
adobe:mainfrom
greglum:fix/collection-duplicate-keys
Open

greglum wants to merge 2 commits into
adobe:mainfrom
greglum:fix/collection-duplicate-keys

Conversation

@greglum

@greglum greglum commented Sep 15, 2026

Copy link
Copy Markdown

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.addNode is the update-or-insert path: every dirty element is re-added on each update, so a naive "key already in keyMap" check fires on ordinary re-renders. What actually corrupts the collection is two different connected elements claiming the same key. With ['a', 'a', 'b'] on main, 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'] while size reports 2 (the silent drop).

How. Document now records which ElementNode owns each key when it adds the node to the collection. In addNode, if a different element already owns the key, it throws Duplicate key "…" found in collection. Every item in a collection must have a unique key. The owner entry is released in removeNode and cleared in resetAfterSSR. Because updateCollection processes 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 the isConnected/isHidden clauses 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 when process.env.NODE_ENV !== 'production'; production behavior is unchanged. The error surfaces from getCollection() during render, so an error boundary catches it like the existing Cannot change the id of an item error.

Fixture fix. TestGridListSections in GridList.test.js reused cat, dog and kangaroo for the ice cream section. It only rendered two groups because the duplicates were dropped; the ids are now unique.

✅ 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
  • I understand every change in this PR and can explain why it's there.
  • If AI-assisted, I followed our AI contribution guidance and pointed my assistant at CLAUDE.md.

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: the Table from Table: Duplicate item id causes infinite collection and out of memory crash #8033 with two Rows sharing id="1" throws.
  • packages/react-aria-components/test/ListBox.test.js (React 19 only): an item can move between a hidden and a visible Activity subtree 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 an id. 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 with Activity, and SSR hydration are unchanged.

Ran locally: yarn test (8285 passed; the 3 remaining failures are DatePicker/LabeledValue timezone tests that fail identically on a clean main checkout on this machine), yarn test:ssr (74 passed), yarn test:browser (400 passed; the Firefox Chat spatial navigation failure reproduces on clean main), 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.

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.
@greglum greglum closed this Sep 15, 2026
@greglum greglum reopened this Sep 15, 2026
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.

Table: Duplicate item id causes infinite collection and out of memory crash

1 participant