fix: throw on duplicate keys in collections (#10605)

* fix: throw on duplicate keys in collections

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.

* fix: simplify the duplicate key check and cover Activity boundaries

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.

Author: greglum

packages/react-aria-components/test/GridList.test.js

@@ -87,15 +87,15 @@ let TestGridListSections = ({listBoxProps, itemProps}) => (
       </GridListItem>
     </GridListSection>
     <GridListSection aria-label="Favorite Ice Cream">
-      <GridListItem {...itemProps} id="cat" textValue="Vanilla">
+      <GridListItem {...itemProps} id="vanilla" textValue="Vanilla">
         <Checkbox slot="selection" />
         Vanilla
       </GridListItem>
-      <GridListItem {...itemProps} id="dog" textValue="Chocolate">
+      <GridListItem {...itemProps} id="chocolate" textValue="Chocolate">
         <Checkbox slot="selection" />
         Chocolate
       </GridListItem>
-      <GridListItem {...itemProps} id="kangaroo" textValue="Strawberry">
+      <GridListItem {...itemProps} id="strawberry" textValue="Strawberry">
         <Checkbox slot="selection" />
         Strawberry
       </GridListItem>

packages/react-aria-components/test/ListBox.test.js

@@ -2444,6 +2444,47 @@ describe('ListBox', () => {
       rerender(<App mode="visible" pathname="/" ready />);
       expect(queryAllByRole('option')).toHaveLength(5);
     });
+
+    it('should only treat visible items as duplicate keys across Activity boundaries', () => {
+      function App({first}) {
+        return (
+          <ListBox aria-label="Test">
+            <React.Activity mode={first ? 'visible' : 'hidden'}>
+              <ListBoxItem id="a">First A</ListBoxItem>
+            </React.Activity>
+            <React.Activity mode={first ? 'hidden' : 'visible'}>
+              <ListBoxItem id="a">Second A</ListBoxItem>
+            </React.Activity>
+            <ListBoxItem id="b">B</ListBoxItem>
+          </ListBox>
+        );
+      }
+
+      let {getAllByRole, rerender, unmount} = render(<App first />);
+      expect(getAllByRole('option').map(o => o.textContent)).toEqual(['First A', 'B']);
+      rerender(<App first={false} />);
+      expect(getAllByRole('option').map(o => o.textContent)).toEqual(['Second A', 'B']);
+      rerender(<App first />);
+      expect(getAllByRole('option').map(o => o.textContent)).toEqual(['First A', 'B']);
+      unmount();
+
+      function Reveal({mode}) {
+        return (
+          <ListBox aria-label="Test">
+            <React.Activity mode={mode}>
+              <ListBoxItem id="a">Hidden A</ListBoxItem>
+            </React.Activity>
+            <ListBoxItem id="a">Visible A</ListBoxItem>
+          </ListBox>
+        );
+      }
+
+      let revealed = render(<Reveal mode="hidden" />);
+      expect(revealed.getAllByRole('option').map(o => o.textContent)).toEqual(['Visible A']);
+      expect(() => revealed.rerender(<Reveal mode="visible" />)).toThrow(
+        'Duplicate key "a" found in collection.'
+      );
+    });
   }
 });
 

packages/react-aria-components/test/Table.test.js

@@ -2426,6 +2426,37 @@ describe('Table', () => {
         'No id detected for the Row element. The Row element requires a id to be provided to it when the cells are rendered dynamically.'
       );
     });
+
+    it('should throw an error if two rows share an id', () => {
+      function DuplicateRowIds() {
+        return (
+          <Table aria-label="Files">
+            <TableHeader>
+              <Column isRowHeader>Name</Column>
+              <Column>Type</Column>
+            </TableHeader>
+            <TableBody>
+              <Row id="1">
+                <Cell>Games</Cell>
+                <Cell>File folder</Cell>
+              </Row>
+              <Row id="1">
+                <Cell>Program Files</Cell>
+                <Cell>File folder</Cell>
+              </Row>
+              <Row id="2">
+                <Cell>bootmgr</Cell>
+                <Cell>System file</Cell>
+              </Row>
+            </TableBody>
+          </Table>
+        );
+      }
+
+      expect(() => render(<DuplicateRowIds />)).toThrow(
+        'Duplicate key "1" found in collection. Every item in a collection must have a unique key.'
+      );
+    });
   });
 
   describe('load more spinner', () => {

packages/react-aria/src/collections/Document.ts

@@ -13,7 +13,7 @@
 import {BaseCollection, CollectionNode, Mutable} from './BaseCollection';
 import {CollectionNodeClass} from './CollectionBuilder';
 import {CSSProperties, ForwardedRef, ReactElement, ReactNode} from 'react';
-import {Node} from '@react-types/shared';
+import {Key, Node} from '@react-types/shared';
 
 // This Collection implementation is perhaps a little unusual. It works by rendering the React tree into a
 // Portal to a fake DOM implementation. This gives us efficient access to the tree of rendered objects, and
@@ -432,6 +432,7 @@ export class Document<T, C extends BaseCollection<T> = BaseCollection<T>> extend
   isSSR = false;
   nodeId = 0;
   nodesByProps: WeakMap<object, ElementNode<T>> = new WeakMap<object, ElementNode<T>>();
+  private keyOwners: Map<Key, ElementNode<T>> = new Map();
   private collection: C;
   private nextCollection: C | null = null;
   private subscriptions: Set<() => void> = new Set();
@@ -470,6 +471,21 @@ export class Document<T, C extends BaseCollection<T> = BaseCollection<T>> extend
       return;
     }
 
+    if (process.env.NODE_ENV !== 'production') {
+      // Two elements with the same key would corrupt the linked list of sibling keys, which
+      // silently drops items or makes key traversal loop forever. Removed and hidden elements
+      // release their key in removeNode before any node is added, so a different owner here
+      // is a live duplicate.
+      let key = element.node.key;
+      let owner = this.keyOwners.get(key);
+      if (owner && owner !== element) {
+        throw new Error(
+          `Duplicate key "${String(key)}" found in collection. Every item in a collection must have a unique key.`
+        );
+      }
+      this.keyOwners.set(key, element);
+    }
+
     let collection = this.getMutableCollection();
     if (!collection.getItem(element.node.key)) {
       for (let child of element) {
@@ -488,6 +504,9 @@ export class Document<T, C extends BaseCollection<T> = BaseCollection<T>> extend
     if (node.node) {
       let collection = this.getMutableCollection();
       collection.removeNode(node.node.key);
+      if (process.env.NODE_ENV !== 'production' && this.keyOwners.get(node.node.key) === node) {
+        this.keyOwners.delete(node.node.key);
+      }
     }
   }
 
@@ -592,6 +611,7 @@ export class Document<T, C extends BaseCollection<T> = BaseCollection<T>> extend
       this.firstChild = null;
       this.lastChild = null;
       this.nodeId = 0;
+      this.keyOwners.clear();
     }
   }
 }

packages/react-aria/test/collections/CollectionBuilder.test.js

@@ -40,6 +40,22 @@ const renderItems = (items, spyCollection) => (
   </CollectionBuilder>
 );
 
+const renderItemsWithIds = (items, spyCollection) => (
+  <CollectionBuilder
+    content={
+      <Collection>
+        {items.map(item => (
+          <Item key={item.key} id={item.id} />
+        ))}
+      </Collection>
+    }>
+    {collection => {
+      spyCollection.current = collection;
+      return null;
+    }}
+  </CollectionBuilder>
+);
+
 const renderItemsOld = (items, spyCollection) => (
   <CollectionBuilder
     content={
@@ -74,6 +90,56 @@ describe('CollectionBuilder', () => {
     expect(spyCollection.current.lastKey).toBe(null);
   });
 
+  it('should throw when two items share a key', () => {
+    let spyCollection = {};
+    let adjacent = [
+      {key: 1, id: 'a'},
+      {key: 2, id: 'a'},
+      {key: 3, id: 'b'}
+    ];
+    expect(() => render(renderItemsWithIds(adjacent, spyCollection))).toThrow(
+      'Duplicate key "a" found in collection. Every item in a collection must have a unique key.'
+    );
+
+    let separated = [
+      {key: 1, id: 'a'},
+      {key: 2, id: 'x'},
+      {key: 3, id: 'a'}
+    ];
+    expect(() => render(renderItemsWithIds(separated, spyCollection))).toThrow(
+      'Duplicate key "a" found in collection.'
+    );
+  });
+
+  it('should allow a new item to reuse the key of an item removed in the same render', () => {
+    let spyCollection = {};
+    const {rerender} = render(
+      renderItemsWithIds(
+        [
+          {key: 1, id: 'a'},
+          {key: 2, id: 'b'}
+        ],
+        spyCollection
+      )
+    );
+    expect([...spyCollection.current.getKeys()]).toEqual(['a', 'b']);
+
+    rerender(
+      renderItemsWithIds(
+        [
+          {key: 1, id: 'a'},
+          {key: 3, id: 'c'},
+          {key: 4, id: 'b'}
+        ],
+        spyCollection
+      )
+    );
+    expect([...spyCollection.current.getKeys()]).toEqual(['a', 'c', 'b']);
+
+    rerender(renderItemsWithIds([{key: 5, id: 'a'}], spyCollection));
+    expect([...spyCollection.current.getKeys()]).toEqual(['a']);
+  });
+
   it('should still support using strings for the collection node class in createLeafComponent/createBranchComponent', () => {
     let spyCollection = {};
     render(renderItemsOld(['a'], spyCollection));