Follow up #8001 by deriving Autocomplete and LabelGroup state during render

.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.

packages/react/src/Autocomplete/Autocomplete.test.tsx

@@ -289,50 +289,61 @@ 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(
         <LabelledAutocomplete
           menuProps={{
             items: mockItems,
             selectedItemIds: [],
             sortOnCloseFn: sortOnCloseFnMock,
+            onOpenChange: onOpenChangeMock,
             ['aria-labelledby']: 'autocompleteLabel',
           }}
         />,
       )
-      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())
     })
 
     it("calls onOpenChange with the menu's open state", async () => {
-      const user = userEvent.setup()
       const onOpenChangeMock = vi.fn()
-      const {container} = render(
+      const {getByLabelText} = render(
         <LabelledAutocomplete
           menuProps={{
-            items: mockItems,
+            items: [],
             selectedItemIds: [],
             onOpenChange: onOpenChangeMock,
             ['aria-labelledby']: 'autocompleteLabel',
           }}
         />,
       )
-      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 () => {

packages/react/src/Autocomplete/AutocompleteMenu.tsx

@@ -324,26 +324,29 @@ function AutocompleteMenu<T extends AutocompleteItemProps>(props: AutocompleteMe
     }
   }, [highlightedItem, deferredInputValue, selectedItemIds, setAutocompleteSuggestion])
 
-  useEffect(() => {
+  // 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])
 
-    // 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 (!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

packages/react/src/LabelGroup/LabelGroup.test.tsx

@@ -112,7 +112,7 @@ describe('LabelGroup', () => {
   })
 
   it('should truncate labels to a specified number', () => {
-    const {getByText} = render(
+    const {getByText, queryByText, rerender} = render(
       <ThemeAndStyleContainer>
         <LabelGroup visibleChildCount={3}>
           <Label>One</Label>
@@ -126,6 +126,21 @@ describe('LabelGroup', () => {
     const expandButton = getByText('+2')
 
     expect(expandButton).toBeDefined()
+
+    rerender(
+      <ThemeAndStyleContainer>
+        <LabelGroup visibleChildCount={4}>
+          <Label>One</Label>
+          <Label>Two</Label>
+          <Label>Three</Label>
+          <Label>Four</Label>
+          <Label>Five</Label>
+        </LabelGroup>
+      </ThemeAndStyleContainer>,
+    )
+
+    expect(queryByText('+2')).toBeNull()
+    expect(getByText('+1')).toBeDefined()
   })
 
   it('should expand all tokens into an overlay when overflowStyle="overlay"', async () => {

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<string, boolean> = {}
+
+  for (let index = 0; index < childCount; index++) {
+    updatedEntries[index.toString()] = index < truncateAfter
+  }
+
+  return updatedEntries
+}
+
+const visibilityMapsMatch = (left: Record<string, boolean>, right: Record<string, boolean>) => {
+  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<HTMLButtonElement>
   collapseInlineExpandedChildren: () => void
@@ -131,6 +151,7 @@ const LabelGroup: React.FC<React.PropsWithChildren<LabelGroupProps>> = ({
     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<React.PropsWithChildren<LabelGroupProps>> = ({
   )
 
   // 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<string, boolean> = {}
-    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,13 +218,26 @@ const LabelGroup: React.FC<React.PropsWithChildren<LabelGroupProps>> = ({
     setIsOverflowShown(true)
   }, [setVisibilityMap, setIsOverflowShown])
 
+  const numericVisibilityMap = React.useMemo(
+    () =>
+      visibleChildCount && typeof visibleChildCount === 'number' && !isOverflowShown
+        ? getVisibilityMapAfterIndex(childArray.length, visibleChildCount)
+        : undefined,
+    [childArray.length, isOverflowShown, visibleChildCount],
+  )
+
+  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
     if (!visibleChildCount || isOverflowShown) {
       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 +273,7 @@ const LabelGroup: React.FC<React.PropsWithChildren<LabelGroupProps>> = ({
 
       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 +325,7 @@ const LabelGroup: React.FC<React.PropsWithChildren<LabelGroupProps>> = ({
       className={clsx(className, classes.Container)}
       data-component="LabelGroup"
     >
-      {React.Children.map(children, (child, index) => (
+      {childArray.map((child, index) => (
         <ItemWrapperComponent
           // data-index is used as an identifier we can use in the IntersectionObserver
           data-index={index}
@@ -325,7 +346,7 @@ const LabelGroup: React.FC<React.PropsWithChildren<LabelGroupProps>> = ({
             hiddenItemIds={hiddenItemIds}
             isOverflowShown={isOverflowShown}
             showAllTokensInline={showAllTokensInline}
-            totalLength={React.Children.toArray(children).length}
+            totalLength={childArray.length}
           />
         ) : (
           <OverlayToggle
@@ -336,7 +357,7 @@ const LabelGroup: React.FC<React.PropsWithChildren<LabelGroupProps>> = ({
             openOverflowOverlay={openOverflowOverlay}
             overlayPaddingPx={overlayPaddingPx}
             overlayWidth={overlayWidth}
-            totalLength={React.Children.toArray(children).length}
+            totalLength={childArray.length}
           >
             {children}
           </OverlayToggle>