diff --git a/.changeset/stupid-monkeys-beg.md b/.changeset/stupid-monkeys-beg.md new file mode 100644 index 00000000000..685c96bab90 --- /dev/null +++ b/.changeset/stupid-monkeys-beg.md @@ -0,0 +1,5 @@ +--- +"@primer/react": patch +--- + +SelectPanel: Fix items not being selected when defined within scope (track selection by item.id) diff --git a/packages/react/src/SelectPanel/SelectPanel.examples.stories.tsx b/packages/react/src/SelectPanel/SelectPanel.examples.stories.tsx index 248432e0f9a..3e261c6bce3 100644 --- a/packages/react/src/SelectPanel/SelectPanel.examples.stories.tsx +++ b/packages/react/src/SelectPanel/SelectPanel.examples.stories.tsx @@ -309,3 +309,39 @@ export const CustomItemRenderer = () => { ) } + +export const ItemsInScope = () => { + // items are defined in the same scope as selection, so they could rerender and create new object references + // We use item.id to track selection + // Reported in: https://github.com/primer/react/issues/4315 + const items = [ + {text: 'enhancement', id: 1}, + {text: 'bug', id: 2}, + {text: 'good first issue', id: 3}, + {text: 'design', id: 4}, + {text: 'blocker', id: 5}, + {text: 'backend', id: 6}, + {text: 'frontend', id: 7}, + ] + + const [selected, setSelected] = React.useState([items[0], items[1]]) + const [open, setOpen] = useState(false) + const [filter, setFilter] = React.useState('') + const filteredItems = items.filter(item => item.text.toLowerCase().startsWith(filter.toLowerCase())) + + return ( + <> +

Items in component scope

+ + + ) +} diff --git a/packages/react/src/SelectPanel/SelectPanel.test.tsx b/packages/react/src/SelectPanel/SelectPanel.test.tsx index 936f3b544fc..a047507fb7e 100644 --- a/packages/react/src/SelectPanel/SelectPanel.test.tsx +++ b/packages/react/src/SelectPanel/SelectPanel.test.tsx @@ -335,6 +335,53 @@ for (const useModernActionList of [false, true]) { screen.getByRole('option', {name: 'item one'}).id, ) }) + + it('should select an item (by item.id) even when items are defined in the component', async () => { + const user = userEvent.setup() + + function Fixture() { + // items are defined in the same scope as selection, so they could rerender and create new object references + // We use item.id to track selection + const items: SelectPanelProps['items'] = [ + {id: 'one', text: 'item one'}, + {id: 'two', text: 'item two'}, + {id: 'three', text: 'item three'}, + ] + + const [open, setOpen] = React.useState(false) + const [selected, setSelected] = React.useState([]) + const [filter, setFilter] = React.useState('') + + return ( + + + + ) + } + + renderWithFlag(, useModernActionList) + + await user.click(screen.getByText('Select items')) + + await user.click(screen.getByText('item one')) + expect(screen.getByRole('option', {name: 'item one'})).toHaveAttribute('aria-selected', 'true') + + await user.click(screen.getByText('item two')) + expect(screen.getByRole('option', {name: 'item two'})).toHaveAttribute('aria-selected', 'true') + + await user.click(screen.getByRole('option', {name: 'item one'})) + expect(screen.getByRole('option', {name: 'item one'})).toHaveAttribute('aria-selected', 'false') + }) }) function FilterableSelectPanel() { diff --git a/packages/react/src/SelectPanel/SelectPanel.tsx b/packages/react/src/SelectPanel/SelectPanel.tsx index efae1b5a7e1..c862950deb3 100644 --- a/packages/react/src/SelectPanel/SelectPanel.tsx +++ b/packages/react/src/SelectPanel/SelectPanel.tsx @@ -61,6 +61,16 @@ const focusZoneSettings: Partial = { disabled: true, } +const areItemsEqual = (itemA: ItemInput, itemB: ItemInput) => { + // prefer checking equivality by item.id + if (typeof itemA.id !== 'undefined') return itemA.id === itemB.id + else return itemA === itemB +} + +const doesItemsIncludeItem = (items: ItemInput[], item: ItemInput) => { + return items.some(i => areItemsEqual(i, item)) +} + export function SelectPanel({ open, onOpenChange, @@ -129,7 +139,7 @@ export function SelectPanel({ const itemsToRender = useMemo(() => { return items.map(item => { - const isItemSelected = isMultiSelectVariant(selected) ? selected.includes(item) : selected === item + const isItemSelected = isMultiSelectVariant(selected) ? doesItemsIncludeItem(selected, item) : selected === item return { ...item, @@ -143,8 +153,10 @@ export function SelectPanel({ } if (isMultiSelectVariant(selected)) { - const otherSelectedItems = selected.filter(selectedItem => selectedItem !== item) - const newSelectedItems = selected.includes(item) ? otherSelectedItems : [...otherSelectedItems, item] + const otherSelectedItems = selected.filter(selectedItem => !areItemsEqual(selectedItem, item)) + const newSelectedItems = doesItemsIncludeItem(selected, item) + ? otherSelectedItems + : [...otherSelectedItems, item] const multiSelectOnChange = onSelectedChange as SelectPanelMultiSelection['onSelectedChange'] multiSelectOnChange(newSelectedItems)