From 533376392ec49d2b1570a9897a84b70db8871c02 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 17 Jun 2026 12:30:28 +0000 Subject: [PATCH 1/4] Initial plan From 7b943151e8b61d6bd97b86e4cd8d2e1a391fd6e8 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 17 Jun 2026 12:43:22 +0000 Subject: [PATCH 2/4] refactor: derive follow-up state during render Co-authored-by: mattcosta7 <8616962+mattcosta7@users.noreply.github.com> --- .../src/Autocomplete/Autocomplete.test.tsx | 19 ++++-- .../src/Autocomplete/AutocompleteMenu.tsx | 28 ++++---- .../react/src/LabelGroup/LabelGroup.test.tsx | 17 ++++- packages/react/src/LabelGroup/LabelGroup.tsx | 64 ++++++++++++------- 4 files changed, 84 insertions(+), 44 deletions(-) diff --git a/packages/react/src/Autocomplete/Autocomplete.test.tsx b/packages/react/src/Autocomplete/Autocomplete.test.tsx index 054a9b52e0a..ae93bb6f807 100644 --- a/packages/react/src/Autocomplete/Autocomplete.test.tsx +++ b/packages/react/src/Autocomplete/Autocomplete.test.tsx @@ -317,22 +317,29 @@ describe('Autocomplete', () => { }) it("calls onOpenChange with the menu's open state", async () => { - const user = userEvent.setup() const onOpenChangeMock = vi.fn() - const {container} = render( + const {getByLabelText} = render( , ) - const inputNode = container.querySelector('#autocompleteInput') + const inputNode = getByLabelText(AUTOCOMPLETE_LABEL) - inputNode && (await user.type(inputNode, 'ze')) - expect(onOpenChangeMock).toHaveBeenCalled() + expect(onOpenChangeMock).toHaveBeenNthCalledWith(1, false) + + fireEvent.click(inputNode) + fireEvent.keyDown(inputNode, {key: 'ArrowDown'}) + await waitFor(() => expect(onOpenChangeMock).toHaveBeenLastCalledWith(true)) + + // eslint-disable-next-line github/no-blur + fireEvent.blur(inputNode) + + await waitFor(() => expect(onOpenChangeMock).toHaveBeenLastCalledWith(false)) }) it('calls onSelectedChange with the data for the selected items', async () => { diff --git a/packages/react/src/Autocomplete/AutocompleteMenu.tsx b/packages/react/src/Autocomplete/AutocompleteMenu.tsx index b87472c3149..357a4722561 100644 --- a/packages/react/src/Autocomplete/AutocompleteMenu.tsx +++ b/packages/react/src/Autocomplete/AutocompleteMenu.tsx @@ -169,6 +169,7 @@ function AutocompleteMenu(props: AutocompleteMe const allItemsToRenderRef = useRef([]) const [highlightedItem, setHighlightedItem] = useState() const [sortedItemIds, setSortedItemIds] = useState>(items.map(({id: itemId}) => itemId)) + const [prevShowMenu, setPrevShowMenu] = useState(showMenu) const generatedUniqueId = useId(id) const selectableItems = useMemo( @@ -324,26 +325,25 @@ function AutocompleteMenu(props: AutocompleteMe } }, [highlightedItem, deferredInputValue, selectedItemIds, setAutocompleteSuggestion]) - useEffect(() => { - const itemIdSortResult = [...sortedItemIds].sort( - sortOnCloseFn ? sortOnCloseFn : getDefaultSortFn(itemId => isItemSelected(itemId, selectedItemIds)), - ) - const sortResultMatchesState = - itemIdSortResult.length === sortedItemIds.length && - itemIdSortResult.every((element, index) => element === sortedItemIds[index]) + const itemIdSortResult = [...sortedItemIds].sort( + sortOnCloseFn ? sortOnCloseFn : getDefaultSortFn(itemId => isItemSelected(itemId, selectedItemIds)), + ) + const sortResultMatchesState = + itemIdSortResult.length === sortedItemIds.length && + itemIdSortResult.every((element, index) => element === sortedItemIds[index]) - // eslint-disable-next-line react-you-might-not-need-an-effect/no-event-handler - if (showMenu === false && !sortResultMatchesState) { - // Re-sort only when the menu closes. This effect also fires `onOpenChange` below, so it - // stays an effect. (Follow-up: the sort could be derived on the open→closed transition - // while keeping `onOpenChange` in an effect.) - // eslint-disable-next-line react-hooks/set-state-in-effect, react-you-might-not-need-an-effect/no-derived-state + if (prevShowMenu !== showMenu) { + setPrevShowMenu(showMenu) + + if (prevShowMenu && showMenu === false && !sortResultMatchesState) { setSortedItemIds(itemIdSortResult) } + } + useEffect(() => { // eslint-disable-next-line react-you-might-not-need-an-effect/no-pass-data-to-parent onOpenChange && onOpenChange(Boolean(showMenu)) - }, [showMenu, onOpenChange, selectedItemIds, sortOnCloseFn, sortedItemIds]) + }, [showMenu, onOpenChange]) useEffect(() => { // eslint-disable-next-line react-you-might-not-need-an-effect/no-event-handler diff --git a/packages/react/src/LabelGroup/LabelGroup.test.tsx b/packages/react/src/LabelGroup/LabelGroup.test.tsx index 2e141fc6d78..f22715d3654 100644 --- a/packages/react/src/LabelGroup/LabelGroup.test.tsx +++ b/packages/react/src/LabelGroup/LabelGroup.test.tsx @@ -111,7 +111,7 @@ describe('LabelGroup', () => { }) it('should truncate labels to a specified number', () => { - const {getByText} = render( + const {getByText, queryByText, rerender} = render( @@ -125,6 +125,21 @@ describe('LabelGroup', () => { const expandButton = getByText('+2') expect(expandButton).toBeDefined() + + rerender( + + + + + + + + + , + ) + + expect(queryByText('+2')).toBeNull() + expect(getByText('+1')).toBeDefined() }) it('should expand all tokens into an overlay when overflowStyle="overlay"', async () => { diff --git a/packages/react/src/LabelGroup/LabelGroup.tsx b/packages/react/src/LabelGroup/LabelGroup.tsx index fa91230cb56..f40667806f7 100644 --- a/packages/react/src/LabelGroup/LabelGroup.tsx +++ b/packages/react/src/LabelGroup/LabelGroup.tsx @@ -23,6 +23,26 @@ const getOverlayWidth = ( overlayPaddingPx: number, ) => overlayPaddingPx + buttonClientRect.right - (containerRef.current?.getBoundingClientRect().left || 0) +const getVisibilityMapAfterIndex = (childCount: number, truncateAfter: number) => { + const updatedEntries: Record = {} + + for (let index = 0; index < childCount; index++) { + updatedEntries[index.toString()] = index < truncateAfter + } + + return updatedEntries +} + +const visibilityMapsMatch = (left: Record, right: Record) => { + const leftKeys = Object.keys(left) + const rightKeys = Object.keys(right) + + return ( + leftKeys.length === rightKeys.length && + leftKeys.every(key => Object.prototype.hasOwnProperty.call(right, key) && left[key] === right[key]) + ) +} + const InlineToggle: React.FC<{ collapseButtonRef: React.RefObject collapseInlineExpandedChildren: () => void @@ -131,6 +151,7 @@ const LabelGroup: React.FC> = ({ bottom: 0, toJSON: () => undefined, }) + const childArray = React.Children.toArray(children) const overlayPaddingPx = 8 // var(--base-size-8), hardcoded to do some math const hiddenItemIds = Object.keys(visibilityMap).filter(key => !visibilityMap[key]) @@ -163,18 +184,12 @@ const LabelGroup: React.FC> = ({ ) // Sets the visibility map to hide children after the given index. - const hideChildrenAfterIndex = React.useCallback((truncateAfter: number) => { - const containerChildren = containerRef.current?.children || [] - const updatedEntries: Record = {} - for (const child of containerChildren) { - const targetId = child.getAttribute('data-index') - if (targetId) { - updatedEntries[targetId] = parseInt(targetId, 10) < truncateAfter - } - } - - setVisibilityMap(updatedEntries) - }, []) + const hideChildrenAfterIndex = React.useCallback( + (truncateAfter: number) => { + setVisibilityMap(getVisibilityMapAfterIndex(childArray.length, truncateAfter)) + }, + [childArray.length], + ) const openOverflowOverlay = React.useCallback(() => setIsOverflowShown(true), [setIsOverflowShown]) @@ -203,6 +218,15 @@ const LabelGroup: React.FC> = ({ setIsOverflowShown(true) }, [setVisibilityMap, setIsOverflowShown]) + const numericVisibilityMap = + typeof visibleChildCount === 'number' && !isOverflowShown + ? getVisibilityMapAfterIndex(childArray.length, visibleChildCount) + : undefined + + if (numericVisibilityMap && !visibilityMapsMatch(visibilityMap, numericVisibilityMap)) { + setVisibilityMap(numericVisibilityMap) + } + React.useEffect(() => { // If we're not truncating, we don't need to run this useEffect. // eslint-disable-next-line react-you-might-not-need-an-effect/no-event-handler @@ -210,6 +234,7 @@ const LabelGroup: React.FC> = ({ return } + // eslint-disable-next-line react-you-might-not-need-an-effect/no-event-handler if (visibleChildCount === 'auto') { // Instantiates the IntersectionObserver to track when children fit in the container. const observer = new IntersectionObserver( @@ -245,14 +270,7 @@ const LabelGroup: React.FC> = ({ return () => observer.disconnect() } - // We're not auto truncating, so we need to hide children after the given `visibleChildCount`. - // `hideChildrenAfterIndex` reads the rendered children from the DOM, so it must run in an - // effect. (Follow-up: this branch could derive the visibility map from child indices instead.) - else { - // eslint-disable-next-line react-hooks/set-state-in-effect - hideChildrenAfterIndex(visibleChildCount) - } - }, [buttonClientRect, visibleChildCount, hideChildrenAfterIndex, isOverflowShown]) + }, [buttonClientRect, visibleChildCount, isOverflowShown]) // Updates the index of the first hidden child. // We need to keep track of this so we can focus the first hidden child when the overflow is shown inline. @@ -304,7 +322,7 @@ const LabelGroup: React.FC> = ({ className={clsx(className, classes.Container)} data-component="LabelGroup" > - {React.Children.map(children, (child, index) => ( + {childArray.map((child, index) => ( > = ({ hiddenItemIds={hiddenItemIds} isOverflowShown={isOverflowShown} showAllTokensInline={showAllTokensInline} - totalLength={React.Children.toArray(children).length} + totalLength={childArray.length} /> ) : ( > = ({ openOverflowOverlay={openOverflowOverlay} overlayPaddingPx={overlayPaddingPx} overlayWidth={overlayWidth} - totalLength={React.Children.toArray(children).length} + totalLength={childArray.length} > {children} From cfb76f4f9b81553ac4bd68824f0f6e9f3141b1f1 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 17 Jun 2026 12:51:03 +0000 Subject: [PATCH 3/4] refactor: finalize render-time state derivation Co-authored-by: mattcosta7 <8616962+mattcosta7@users.noreply.github.com> --- packages/react/src/LabelGroup/LabelGroup.tsx | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/packages/react/src/LabelGroup/LabelGroup.tsx b/packages/react/src/LabelGroup/LabelGroup.tsx index f40667806f7..15017e1facd 100644 --- a/packages/react/src/LabelGroup/LabelGroup.tsx +++ b/packages/react/src/LabelGroup/LabelGroup.tsx @@ -218,10 +218,13 @@ const LabelGroup: React.FC> = ({ setIsOverflowShown(true) }, [setVisibilityMap, setIsOverflowShown]) - const numericVisibilityMap = - typeof visibleChildCount === 'number' && !isOverflowShown - ? getVisibilityMapAfterIndex(childArray.length, visibleChildCount) - : undefined + const numericVisibilityMap = React.useMemo( + () => + typeof visibleChildCount === 'number' && !isOverflowShown + ? getVisibilityMapAfterIndex(childArray.length, visibleChildCount) + : undefined, + [childArray.length, isOverflowShown, visibleChildCount], + ) if (numericVisibilityMap && !visibilityMapsMatch(visibilityMap, numericVisibilityMap)) { setVisibilityMap(numericVisibilityMap) From 5d24be483f2fdfa305970907984d04522e2aa13c Mon Sep 17 00:00:00 2001 From: Matthew Costabile Date: Tue, 23 Jun 2026 15:23:16 +0000 Subject: [PATCH 4/4] fix(Autocomplete): derive close-time sort safely despite menu remount; address review feedback - AutocompleteMenu remounts on open/close (Overlay vs VisuallyHidden parent), so the prevShowMenu render transition never fired and the re-sort was dead code. Derive the sorted order during render while the menu is closed instead. - LabelGroup: align numeric visibility guard with existing truthiness checks so visibleChildCount={0} no longer causes redundant render-phase updates. - Add changeset for the Autocomplete/LabelGroup render-time derivation. --- ...autocomplete-labelgroup-state-in-render.md | 5 ++++ .../src/Autocomplete/Autocomplete.test.tsx | 26 ++++++++++-------- .../src/Autocomplete/AutocompleteMenu.tsx | 27 ++++++++++--------- packages/react/src/LabelGroup/LabelGroup.tsx | 2 +- 4 files changed, 36 insertions(+), 24 deletions(-) create mode 100644 .changeset/derive-autocomplete-labelgroup-state-in-render.md diff --git a/.changeset/derive-autocomplete-labelgroup-state-in-render.md b/.changeset/derive-autocomplete-labelgroup-state-in-render.md new file mode 100644 index 00000000000..67cfa116ac1 --- /dev/null +++ b/.changeset/derive-autocomplete-labelgroup-state-in-render.md @@ -0,0 +1,5 @@ +--- +'@primer/react': patch +--- + +Derive `AutocompleteMenu` close-time re-sorting and `LabelGroup` numeric truncation during render instead of in effects, removing redundant render passes. No public API or visual changes. diff --git a/packages/react/src/Autocomplete/Autocomplete.test.tsx b/packages/react/src/Autocomplete/Autocomplete.test.tsx index ae93bb6f807..7caf9d195a8 100644 --- a/packages/react/src/Autocomplete/Autocomplete.test.tsx +++ b/packages/react/src/Autocomplete/Autocomplete.test.tsx @@ -289,30 +289,34 @@ describe('Autocomplete', () => { }) it('calls a custom sort function when the menu closes', async () => { - const user = userEvent.setup() const sortOnCloseFnMock = vi.fn() + const onOpenChangeMock = vi.fn() const {container} = render( , ) - const inputNode = container.querySelector('#autocompleteInput') + const inputNode = container.querySelector('#autocompleteInput') as HTMLInputElement - // `sortOnCloseFnMock` will be called in a `.sort()` on render to check if the - // current sort order matches the result of `sortOnCloseFnMock` - expect(sortOnCloseFnMock).toHaveBeenCalledTimes(mockItems.length - 1) - if (inputNode) { - await user.type(inputNode, 'ze') - // eslint-disable-next-line github/no-blur - fireEvent.blur(inputNode) - } + // The menu starts closed, so the items are sorted during render. + expect(sortOnCloseFnMock).toHaveBeenCalled() - // wait a tick for blur to finish + // Opening the menu does not run the close-time sort. + sortOnCloseFnMock.mockClear() + fireEvent.click(inputNode) + fireEvent.keyDown(inputNode, {key: 'ArrowDown'}) + await waitFor(() => expect(onOpenChangeMock).toHaveBeenLastCalledWith(true)) + expect(sortOnCloseFnMock).not.toHaveBeenCalled() + + // Closing the menu re-runs the sort. + // eslint-disable-next-line github/no-blur + fireEvent.blur(inputNode) await waitFor(() => expect(sortOnCloseFnMock).toHaveBeenCalled()) }) diff --git a/packages/react/src/Autocomplete/AutocompleteMenu.tsx b/packages/react/src/Autocomplete/AutocompleteMenu.tsx index 357a4722561..672be5835cc 100644 --- a/packages/react/src/Autocomplete/AutocompleteMenu.tsx +++ b/packages/react/src/Autocomplete/AutocompleteMenu.tsx @@ -169,7 +169,6 @@ function AutocompleteMenu(props: AutocompleteMe const allItemsToRenderRef = useRef([]) const [highlightedItem, setHighlightedItem] = useState() const [sortedItemIds, setSortedItemIds] = useState>(items.map(({id: itemId}) => itemId)) - const [prevShowMenu, setPrevShowMenu] = useState(showMenu) const generatedUniqueId = useId(id) const selectableItems = useMemo( @@ -325,17 +324,21 @@ function AutocompleteMenu(props: AutocompleteMe } }, [highlightedItem, deferredInputValue, selectedItemIds, setAutocompleteSuggestion]) - const itemIdSortResult = [...sortedItemIds].sort( - sortOnCloseFn ? sortOnCloseFn : getDefaultSortFn(itemId => isItemSelected(itemId, selectedItemIds)), - ) - const sortResultMatchesState = - itemIdSortResult.length === sortedItemIds.length && - itemIdSortResult.every((element, index) => element === sortedItemIds[index]) - - if (prevShowMenu !== showMenu) { - setPrevShowMenu(showMenu) - - if (prevShowMenu && showMenu === false && !sortResultMatchesState) { + // Re-sort the items during render while the menu is closed so the order is ready the next time + // it opens. `AutocompleteOverlay` mounts this component under a different parent when the menu + // opens vs. closes, so it remounts on each open/close and a render-time previous-value transition + // can't be observed here — guarding on the closed state is what replaces the old close-time effect. + // The comparator therefore runs only while the menu is closed, never on the frequent renders that + // happen while it is open. + if (showMenu === false) { + const itemIdSortResult = [...sortedItemIds].sort( + sortOnCloseFn ? sortOnCloseFn : getDefaultSortFn(itemId => isItemSelected(itemId, selectedItemIds)), + ) + const sortResultMatchesState = + itemIdSortResult.length === sortedItemIds.length && + itemIdSortResult.every((element, index) => element === sortedItemIds[index]) + + if (!sortResultMatchesState) { setSortedItemIds(itemIdSortResult) } } diff --git a/packages/react/src/LabelGroup/LabelGroup.tsx b/packages/react/src/LabelGroup/LabelGroup.tsx index 15017e1facd..b3fe7a5b133 100644 --- a/packages/react/src/LabelGroup/LabelGroup.tsx +++ b/packages/react/src/LabelGroup/LabelGroup.tsx @@ -220,7 +220,7 @@ const LabelGroup: React.FC> = ({ const numericVisibilityMap = React.useMemo( () => - typeof visibleChildCount === 'number' && !isOverflowShown + visibleChildCount && typeof visibleChildCount === 'number' && !isOverflowShown ? getVisibilityMapAfterIndex(childArray.length, visibleChildCount) : undefined, [childArray.length, isOverflowShown, visibleChildCount],