diff --git a/.changeset/underline-nav-cascade-reduction.md b/.changeset/underline-nav-cascade-reduction.md new file mode 100644 index 00000000000..700a5ecd2f0 --- /dev/null +++ b/.changeset/underline-nav-cascade-reduction.md @@ -0,0 +1,21 @@ +--- +'@primer/react': patch +--- + +Reduce render cascades in `UnderlineNav` and `UnderlinePanels` (experimental): + +- `UnderlineNav.Item` no longer runs a per-item layout effect that dispatches + `setChildrenWidth` + `setNoIconChildrenWidth` to the parent. The parent + `UnderlineNav` now measures all items in a single layout effect and stores + widths in refs (not state). For N items, that's one batched setState + instead of 2N. The hook's `updateListAndMenu` now compares items/menuItems + by key before committing, so the `ResizeObserver`'s initial fire (which + reports the same dimensions the parent layout effect just measured) bails + out instead of producing an extra render. +- `UnderlinePanels` now derives `tabs` and `tabPanels` inline via `useMemo` + instead of holding them in state and syncing through a post-paint + `useEffect`. The `listWidth` state is gone — the layout effect and resize + observer compute icon visibility by measuring both list and wrapper + together at decision time. + +No public API changes. diff --git a/packages/react/src/UnderlineNav/UnderlineNav.test.tsx b/packages/react/src/UnderlineNav/UnderlineNav.test.tsx index 6039ec2ad8a..887518c5a48 100644 --- a/packages/react/src/UnderlineNav/UnderlineNav.test.tsx +++ b/packages/react/src/UnderlineNav/UnderlineNav.test.tsx @@ -1,5 +1,5 @@ import {describe, expect, it, vi} from 'vitest' -import type React from 'react' +import React from 'react' import {render, screen} from '@testing-library/react' import userEvent from '@testing-library/user-event' import { @@ -238,6 +238,30 @@ describe('UnderlineNav', () => { const textSpan = item.querySelector('[data-component="text"]') expect(textSpan).toHaveAttribute('data-content', 'Simple Text') }) + + it('does not cascade renders on mount (parent-driven measurement)', async () => { + // Each `UnderlineNav.Item` previously ran its own layout effect that + // dispatched two setState calls to the parent — producing one batched + // re-commit on mount in addition to the ResizeObserver's initial fire. + // The parent-driven measurement collapses that to a single setState, and + // the equality-guarded `updateListAndMenu` causes the ResizeObserver's + // first entry to bail out instead of committing identical responsiveProps. + // + // Asserting an exact upper bound here pins the improvement: with the old + // code the counter saw >=3 commits on mount; with the new code it sees + // at most 2 (initial + 1 measurement-driven update). + let commitCount = 0 + render( + commitCount++}> + + , + ) + + // Allow the ResizeObserver's initial entry to flush. + await new Promise(resolve => setTimeout(resolve, 0)) + + expect(commitCount).toBeLessThanOrEqual(2) + }) }) describe('Keyboard Navigation', () => { diff --git a/packages/react/src/UnderlineNav/UnderlineNav.tsx b/packages/react/src/UnderlineNav/UnderlineNav.tsx index 14c52ee9683..fc875257425 100644 --- a/packages/react/src/UnderlineNav/UnderlineNav.tsx +++ b/packages/react/src/UnderlineNav/UnderlineNav.tsx @@ -1,9 +1,9 @@ import type {RefObject} from 'react' -import React, {useRef, forwardRef, useCallback, useState, useEffect} from 'react' +import React, {useRef, forwardRef, useCallback, useState, useEffect, useMemo} from 'react' import {UnderlineNavContext} from './UnderlineNavContext' import type {ResizeObserverEntry} from '../hooks/useResizeObserver' import {useResizeObserver} from '../hooks/useResizeObserver' -import type {ChildWidthArray, ResponsiveProps, ChildSize} from './types' +import type {ChildWidthArray, ResponsiveProps} from './types' import VisuallyHidden from '../_VisuallyHidden' import {dividerStyles, menuItemStyles, baseMenuMinWidth} from './styles' import {UnderlineItemList, UnderlineWrapper, LoadingCounter, GAP} from '../internal/components/UnderlineTabbedInterface' @@ -17,6 +17,7 @@ import CounterLabel from '../CounterLabel' import {invariant} from '../utils/invariant' import classes from './UnderlineNav.module.css' import {getAnchoredPosition} from '@primer/behaviors' +import useIsomorphicLayoutEffect from '../utils/useIsomorphicLayoutEffect' export type UnderlineNavProps = { children: React.ReactNode @@ -165,12 +166,17 @@ export const UnderlineNav = forwardRef( const [isWidgetOpen, setIsWidgetOpen] = useState(false) const [iconsVisible, setIconsVisible] = useState(true) - const [childWidthArray, setChildWidthArray] = useState([]) - const [noIconChildWidthArray, setNoIconChildWidthArray] = useState([]) + // Item widths are stored in refs (not state) because they're internal to + // the overflow computation — never read during JSX render. Keeping them + // out of state means measuring children no longer triggers a re-render. + const childWidthArrayRef = useRef([]) + const noIconChildWidthArrayRef = useRef([]) // Track whether the initial overflow calculation is complete to prevent CLS const [isOverflowMeasured, setIsOverflowMeasured] = useState(false) - const validChildren = getValidChildren(children) + // Memoize so a re-render with unchanged children doesn't invalidate the + // measurement layout effect's deps. + const validChildren = useMemo(() => getValidChildren(children), [children]) // Responsive props object manages which items are in the list and which items are in the menu. const [responsiveProps, setResponsiveProps] = useState({ @@ -204,7 +210,7 @@ export const UnderlineNav = forwardRef( } function getItemsWidth(itemText: string): number { - return noIconChildWidthArray.find(item => item.text === itemText)?.width ?? 0 + return noIconChildWidthArrayRef.current.find(item => item.text === itemText)?.width ?? 0 } const swapMenuItemWithListItem = ( @@ -250,7 +256,18 @@ export const UnderlineNav = forwardRef( const updateListAndMenu = useCallback( (props: ResponsiveProps, displayIcons: boolean, overflowMeasured: boolean) => { - setResponsiveProps(props) + // Equality guards: the ResizeObserver fires an initial entry on + // observe() with the same dimensions the parent layout effect + // already measured. Without these guards that initial fire + // commits an extra render with identical responsiveProps. + setResponsiveProps(prev => { + const sameItems = + prev.items.length === props.items.length && prev.items.every((item, i) => item.key === props.items[i].key) + const sameMenu = + prev.menuItems.length === props.menuItems.length && + prev.menuItems.every((item, i) => item.key === props.menuItems[i].key) + return sameItems && sameMenu ? prev : props + }) setIconsVisible(displayIcons) if (overflowMeasured) { @@ -259,19 +276,6 @@ export const UnderlineNav = forwardRef( }, [], ) - const setChildrenWidth = useCallback((size: ChildSize) => { - setChildWidthArray(arr => { - const newArr = [...arr, size] - return newArr - }) - }, []) - - const setNoIconChildrenWidth = useCallback((size: ChildSize) => { - setNoIconChildWidthArray(arr => { - const newArr = [...arr, size] - return newArr - }) - }, []) const closeOverlay = React.useCallback(() => { setIsWidgetOpen(false) @@ -301,6 +305,47 @@ export const UnderlineNav = forwardRef( useOnOutsideClick({onClickOutside: closeOverlay, containerRef, ignoreClickRefs: [moreMenuBtnRef]}) + // Measure all items in one parent-driven layout effect instead of having + // each UnderlineNavItem dispatch its own setState. Two wins: + // 1. N children → 1 setState (was 2N), so far fewer batched updates queued. + // 2. The measurements feed `overflowEffect` synchronously here, so the + // ResizeObserver's initial fire becomes a no-op (equality-guarded). + useIsomorphicLayoutEffect(() => { + if (!listRef.current || !navRef.current) return + + const itemEls = listRef.current.querySelectorAll('[data-underline-nav-item]') + if (itemEls.length === 0) return + + const childWidths: ChildWidthArray = [] + const noIconChildWidths: ChildWidthArray = [] + + for (const li of itemEls) { + const inner = li.firstElementChild as HTMLElement | null + if (!inner) continue + const rect = inner.getBoundingClientRect() + const textEl = inner.querySelector('[data-component="text"]') + const iconEl = inner.querySelector('[data-component="icon"]') + if (!textEl) continue + const text = textEl.textContent + const iconWidthWithMargin = iconEl + ? iconEl.getBoundingClientRect().width + + Number(getComputedStyle(iconEl).marginRight.slice(0, -2)) + + Number(getComputedStyle(iconEl).marginLeft.slice(0, -2)) + : 0 + childWidths.push({text, width: rect.width}) + noIconChildWidths.push({text, width: rect.width - iconWidthWithMargin}) + } + + childWidthArrayRef.current = childWidths + noIconChildWidthArrayRef.current = noIconChildWidths + + const navWidth = navRef.current.getBoundingClientRect().width + const moreMenuWidth = moreMenuRef.current?.getBoundingClientRect().width ?? 0 + if (navWidth !== 0) { + overflowEffect(navWidth, moreMenuWidth, validChildren, childWidths, noIconChildWidths, updateListAndMenu) + } + }, [validChildren, updateListAndMenu]) + useResizeObserver((resizeObserverEntries: ResizeObserverEntry[]) => { const navWidth = resizeObserverEntries[0].contentRect.width const moreMenuWidth = moreMenuRef.current?.getBoundingClientRect().width ?? 0 @@ -309,8 +354,8 @@ export const UnderlineNav = forwardRef( navWidth, moreMenuWidth, validChildren, - childWidthArray, - noIconChildWidthArray, + childWidthArrayRef.current, + noIconChildWidthArrayRef.current, updateListAndMenu, ) }, navRef as RefObject) @@ -333,8 +378,6 @@ export const UnderlineNav = forwardRef( return ( - setNoIconChildrenWidth: React.Dispatch<{text: string; width: number}> loadingCounters: boolean iconsVisible: boolean }>({ - setChildrenWidth: () => null, - setNoIconChildrenWidth: () => null, loadingCounters: false, iconsVisible: true, }) diff --git a/packages/react/src/UnderlineNav/UnderlineNavItem.tsx b/packages/react/src/UnderlineNav/UnderlineNavItem.tsx index 3341f677e11..c33808aeffe 100644 --- a/packages/react/src/UnderlineNav/UnderlineNavItem.tsx +++ b/packages/react/src/UnderlineNav/UnderlineNavItem.tsx @@ -1,9 +1,8 @@ -import type {MutableRefObject, RefObject} from 'react' -import React, {forwardRef, useRef, useContext} from 'react' +import type {RefObject} from 'react' +import React, {forwardRef, useContext} from 'react' import type {IconProps} from '@primer/octicons-react' import type {ForwardRefComponent as PolymorphicForwardRefComponent} from '../utils/polymorphic' import {UnderlineNavContext} from './UnderlineNavContext' -import useLayoutEffect from '../utils/useIsomorphicLayoutEffect' import {UnderlineItem} from '../internal/components/UnderlineTabbedInterface' import classes from './UnderlineNavItem.module.css' @@ -74,33 +73,8 @@ export const UnderlineNavItem = forwardRef( }, forwardedRef, ) => { - const backupRef = useRef(null) - const ref = (forwardedRef ?? backupRef) as RefObject - const {setChildrenWidth, setNoIconChildrenWidth, loadingCounters, iconsVisible} = useContext(UnderlineNavContext) - - useLayoutEffect(() => { - if (ref.current) { - const domRect = (ref as MutableRefObject).current.getBoundingClientRect() - - const icon = Array.from((ref as MutableRefObject).current.children).find( - child => child.getAttribute('data-component') === 'icon', - ) - - const content = Array.from((ref as MutableRefObject).current.children).find( - child => child.getAttribute('data-component') === 'text', - ) as HTMLElement - const text = content.textContent as string - - const iconWidthWithMargin = icon - ? icon.getBoundingClientRect().width + - Number(getComputedStyle(icon).marginRight.slice(0, -2)) + - Number(getComputedStyle(icon).marginLeft.slice(0, -2)) - : 0 - - setChildrenWidth({text, width: domRect.width}) - setNoIconChildrenWidth({text, width: domRect.width - iconWidthWithMargin}) - } - }, [ref, setChildrenWidth, setNoIconChildrenWidth]) + const ref = forwardedRef as RefObject | undefined + const {loadingCounters, iconsVisible} = useContext(UnderlineNavContext) const keyDownHandler = React.useCallback( (event: React.KeyboardEvent) => { @@ -120,7 +94,10 @@ export const UnderlineNavItem = forwardRef( ) return ( -
  • + // `data-underline-nav-item` lets the parent UnderlineNav measure all + // items in a single layout effect instead of each item dispatching its + // own setState. See UnderlineNav.tsx. +
  • { ) }).toThrow('Only one tab can be selected at a time.') }) + + it('does not cascade renders on mount (tabs derived inline, no list-width state)', async () => { + // Previously the component held `tabs`/`tabPanels` in state and synced + // them via a post-paint `useEffect`, producing one extra render every + // time children or `iconsVisible` changed. It also held the list width + // in state, with the initial measurement triggering a separate + // layout-effect commit. Both are now derived inline (useMemo + folded + // into the iconsVisible decision), so mount produces at most one + // measurement-driven update on top of the initial commit. + let commitCount = 0 + render( + commitCount++}> + + , + ) + + // Allow ResizeObserver's initial entry to flush. + await new Promise(resolve => setTimeout(resolve, 0)) + + expect(commitCount).toBeLessThanOrEqual(2) + }) }) diff --git a/packages/react/src/experimental/UnderlinePanels/UnderlinePanels.tsx b/packages/react/src/experimental/UnderlinePanels/UnderlinePanels.tsx index aa23618937f..770e4bbaa14 100644 --- a/packages/react/src/experimental/UnderlinePanels/UnderlinePanels.tsx +++ b/packages/react/src/experimental/UnderlinePanels/UnderlinePanels.tsx @@ -4,9 +4,10 @@ import React, { cloneElement, useState, useRef, + useMemo, + useCallback, type FC, type PropsWithChildren, - useEffect, type ElementType, } from 'react' import {TabContainerElement} from '@github/tab-container-element' @@ -20,7 +21,7 @@ import { } from '../../internal/components/UnderlineTabbedInterface' import {useId} from '../../hooks' import {invariant} from '../../utils/invariant' -import {useResizeObserver, type ResizeObserverEntry} from '../../hooks/useResizeObserver' +import {useResizeObserver} from '../../hooks/useResizeObserver' import useIsomorphicLayoutEffect from '../../utils/useIsomorphicLayoutEffect' import classes from './UnderlinePanels.module.css' import {clsx} from 'clsx' @@ -96,13 +97,11 @@ const UnderlinePanels: FCWithSlotMarker = ({ // called in the exact same order in every component render const parentId = useId(props.id) - const [tabs, setTabs] = useState([]) - const [tabPanels, setTabPanels] = useState([]) - - // Make sure we have fresh prop data whenever the tabs or panels are updated (keep aria-selected current) - useEffect(() => { - // Loop through the chidren, if it's a tab, then add id="{id}-tab-{index}" - // If it's a panel, then add aria-labelledby="{id}-tab-{index}" + // Derive tabs / tabPanels during render via useMemo instead of syncing + // to state through a post-paint useEffect. The previous pattern committed + // an extra render every time children or `iconsVisible` changed; this + // version produces the correct values on the very first render. + const {tabs, tabPanels} = useMemo(() => { let tabIndex = 0 let panelIndex = 0 @@ -118,42 +117,38 @@ const UnderlinePanels: FCWithSlotMarker = ({ return child }) - const newTabs = Children.toArray(childrenWithProps).filter(child => { + const nextTabs = Children.toArray(childrenWithProps).filter(child => { return isValidElement(child) && (child.type === Tab || isSlot(child, Tab)) }) - const newTabPanels = Children.toArray(childrenWithProps).filter( + const nextTabPanels = Children.toArray(childrenWithProps).filter( child => isValidElement(child) && (child.type === Panel || isSlot(child, Panel)), ) - // eslint-disable-next-line react-hooks/set-state-in-effect - setTabs(newTabs) - setTabPanels(newTabPanels) + return {tabs: nextTabs, tabPanels: nextTabPanels} }, [children, parentId, loadingCounters, iconsVisible]) - const tabsHaveIcons = tabs.some(tab => React.isValidElement(tab) && tab.props.icon) + const tabsHaveIcons = useMemo(() => tabs.some(tab => React.isValidElement(tab) && tab.props.icon), [tabs]) + + // Single source of truth for the icon-visibility decision, used by both the + // initial layout-effect pass and the ResizeObserver. Measuring list and + // wrapper together at decision time removes the need to hold list width in + // state. + const recomputeIconsVisible = useCallback(() => { + if (!tabsHaveIcons || !listRef.current || !wrapperRef.current) return + const wrapperWidth = wrapperRef.current.getBoundingClientRect().width + const listWidth = listRef.current.getBoundingClientRect().width + const next = wrapperWidth > listWidth + setIconsVisible(prev => (prev === next ? prev : next)) + }, [tabsHaveIcons]) - // this is a workaround to get the list's width on the first render - const [listWidth, setListWidth] = useState(0) useIsomorphicLayoutEffect(() => { - if (!tabsHaveIcons) { - return - } - - setListWidth(listRef.current?.getBoundingClientRect().width ?? 0) - }, [tabsHaveIcons]) + recomputeIconsVisible() + }, [recomputeIconsVisible]) - // when the wrapper resizes, check if the icons should be visible - // by comparing the wrapper width to the list width useResizeObserver( - (resizeObserverEntries: ResizeObserverEntry[]) => { - if (!tabsHaveIcons) { - return - } - - const wrapperWidth = resizeObserverEntries[0].contentRect.width - - setIconsVisible(wrapperWidth > listWidth) + () => { + recomputeIconsVisible() }, wrapperRef, [],