From 2556ac453226b0967c8a364352a4fecadcb63dda Mon Sep 17 00:00:00 2001 From: Matthew Costabile Date: Sat, 23 May 2026 12:25:18 +0000 Subject: [PATCH 1/5] perf(UnderlinePanels): derive tabs and panels in render, not from state MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The component used to mirror its children into two pieces of state via a useEffect: const [tabs, setTabs] = useState([]) const [tabPanels, setTabPanels] = useState([]) useEffect(() => { /* clone children → setTabs / setTabPanels */ }, deps) That is React's documented anti-pattern for syncing props to state. On mount it produced one empty-tablist render followed by a second render with the real tabs after the effect ran, and every consumer rerender went through the same two-commit cycle. Replace with a useMemo that derives the cloned tabs and panels in render. Behaviour is identical (same id assignment, same prop overrides, same children walk), but consumers no longer see an empty-tablist frame on mount and the component renders once per prop/state change instead of twice. --- .../UnderlinePanels/UnderlinePanels.tsx | 33 ++++++++----------- 1 file changed, 14 insertions(+), 19 deletions(-) diff --git a/packages/react/src/experimental/UnderlinePanels/UnderlinePanels.tsx b/packages/react/src/experimental/UnderlinePanels/UnderlinePanels.tsx index aa23618937f..75eec8ed930 100644 --- a/packages/react/src/experimental/UnderlinePanels/UnderlinePanels.tsx +++ b/packages/react/src/experimental/UnderlinePanels/UnderlinePanels.tsx @@ -6,7 +6,7 @@ import React, { useRef, type FC, type PropsWithChildren, - useEffect, + useMemo, type ElementType, } from 'react' import {TabContainerElement} from '@github/tab-container-element' @@ -96,13 +96,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}" + const [tabs, tabPanels] = useMemo(() => { + // Walk children, clone each Tab with a generated id + pass-through flags, + // and each Panel with a matching aria-labelledby. Derive in render so we + // never ship a "before-the-effect-ran" empty-tablist frame and so that + // re-renders of UnderlinePanels don't churn through an extra commit cycle. let tabIndex = 0 let panelIndex = 0 @@ -118,17 +116,14 @@ const UnderlinePanels: FCWithSlotMarker = ({ return child }) - const newTabs = Children.toArray(childrenWithProps).filter(child => { - return isValidElement(child) && (child.type === Tab || isSlot(child, Tab)) - }) - - const newTabPanels = 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) + const tabs: React.ReactNode[] = [] + const tabPanels: React.ReactNode[] = [] + for (const child of Children.toArray(childrenWithProps)) { + if (!isValidElement(child)) continue + if (child.type === Tab || isSlot(child, Tab)) tabs.push(child) + else if (child.type === Panel || isSlot(child, Panel)) tabPanels.push(child) + } + return [tabs, tabPanels] }, [children, parentId, loadingCounters, iconsVisible]) const tabsHaveIcons = tabs.some(tab => React.isValidElement(tab) && tab.props.icon) From b8e01f64cb0e0f7313a82c79456e85f75bb23c2d Mon Sep 17 00:00:00 2001 From: Matthew Costabile Date: Sat, 23 May 2026 12:26:47 +0000 Subject: [PATCH 2/5] perf(UnderlinePanels): keep list width in a ref, not state MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit listWidth was stored in useState even though no render path reads it — only the resize-observer callback does, to compare against the wrapper width and decide whether the icons fit. Storing in state forced an extra commit each time the value was set (mount via the layout effect, plus every list-width change). Move to a useRef. The layout effect writes ref.current; the resize observer reads ref.current. setIconsVisible is the only setState the resize path needs, and it only fires when the visibility decision actually flips. --- .../experimental/UnderlinePanels/UnderlinePanels.tsx | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/packages/react/src/experimental/UnderlinePanels/UnderlinePanels.tsx b/packages/react/src/experimental/UnderlinePanels/UnderlinePanels.tsx index 75eec8ed930..6c924672818 100644 --- a/packages/react/src/experimental/UnderlinePanels/UnderlinePanels.tsx +++ b/packages/react/src/experimental/UnderlinePanels/UnderlinePanels.tsx @@ -128,14 +128,17 @@ const UnderlinePanels: FCWithSlotMarker = ({ const tabsHaveIcons = tabs.some(tab => React.isValidElement(tab) && tab.props.icon) - // this is a workaround to get the list's width on the first render - const [listWidth, setListWidth] = useState(0) + // The list's natural width (icons + labels). Used only inside the resize + // observer to decide whether to show or hide the icons — never read in + // render — so it lives in a ref to avoid an extra commit on mount and on + // every list resize. + const listWidthRef = useRef(0) useIsomorphicLayoutEffect(() => { if (!tabsHaveIcons) { return } - setListWidth(listRef.current?.getBoundingClientRect().width ?? 0) + listWidthRef.current = listRef.current?.getBoundingClientRect().width ?? 0 }, [tabsHaveIcons]) // when the wrapper resizes, check if the icons should be visible @@ -148,7 +151,7 @@ const UnderlinePanels: FCWithSlotMarker = ({ const wrapperWidth = resizeObserverEntries[0].contentRect.width - setIconsVisible(wrapperWidth > listWidth) + setIconsVisible(wrapperWidth > listWidthRef.current) }, wrapperRef, [], From 9c33899a9bd9dd4a18dc17beb3e177ced99cce76 Mon Sep 17 00:00:00 2001 From: Matthew Costabile Date: Sat, 23 May 2026 12:30:56 +0000 Subject: [PATCH 3/5] perf(UnderlinePanels): pass iconsVisible / loadingCounters via context iconsVisible (resize-driven) and loadingCounters were baked into each Tab element by cloneElement. Both deps had to live in the memo's deps array, so every iconsVisible flip re-cloned every Tab element and forced reconciliation of the entire tablist subtree. Move both flags to a new UnderlinePanelsContext. The cloneElement call now only assigns the generated id, and the memo deps shrink to [children, parentId]. Tabs read the flags from context, so they still react to iconsVisible toggles, but unrelated UnderlinePanels re-renders no longer cause Tab elements to lose referential stability. React.memo on the Tab implementation completes the picture: with referentially stable Tab elements + React.memo, an iconsVisible-driven re-render of UnderlinePanels updates only the context-consuming part of each Tab, not the surrounding subtree. --- .../UnderlinePanels/UnderlinePanels.tsx | 80 ++++++++++++++----- 1 file changed, 59 insertions(+), 21 deletions(-) diff --git a/packages/react/src/experimental/UnderlinePanels/UnderlinePanels.tsx b/packages/react/src/experimental/UnderlinePanels/UnderlinePanels.tsx index 6c924672818..1ddf649c88e 100644 --- a/packages/react/src/experimental/UnderlinePanels/UnderlinePanels.tsx +++ b/packages/react/src/experimental/UnderlinePanels/UnderlinePanels.tsx @@ -1,7 +1,9 @@ import React, { Children, + createContext, isValidElement, cloneElement, + useContext, useState, useRef, type FC, @@ -81,6 +83,20 @@ export type PanelProps = React.HTMLAttributes const TabContainerComponent = createComponent(TabContainerElement, 'tab-container') +// Carries flags that affect every Tab's rendering but that don't belong on the +// consumer-facing Tab API. Passing them via context (instead of cloneElement) +// keeps each Tab element's props referentially stable across UnderlinePanels +// re-renders, so React.memo(Tab) can skip work when an unrelated piece of +// state changes. +type UnderlinePanelsContextValue = { + iconsVisible: boolean + loadingCounters: boolean | undefined +} +const UnderlinePanelsContext = createContext({ + iconsVisible: true, + loadingCounters: undefined, +}) + const UnderlinePanels: FCWithSlotMarker = ({ 'aria-label': ariaLabel, 'aria-labelledby': ariaLabelledBy, @@ -97,16 +113,21 @@ const UnderlinePanels: FCWithSlotMarker = ({ const parentId = useId(props.id) const [tabs, tabPanels] = useMemo(() => { - // Walk children, clone each Tab with a generated id + pass-through flags, - // and each Panel with a matching aria-labelledby. Derive in render so we - // never ship a "before-the-effect-ran" empty-tablist frame and so that - // re-renders of UnderlinePanels don't churn through an extra commit cycle. + // Walk children, clone each Tab with a generated id, and each Panel with a + // matching aria-labelledby. Derive in render so we never ship a + // "before-the-effect-ran" empty-tablist frame and so that re-renders of + // UnderlinePanels don't churn through an extra commit cycle. + // + // iconsVisible / loadingCounters are NOT baked into the cloned Tab + // elements — they flow through UnderlinePanelsContext, so this memo's deps + // can stay tight ([children, parentId]) and Tab elements stay + // referentially stable across resize-driven iconsVisible toggles. let tabIndex = 0 let panelIndex = 0 const childrenWithProps = Children.map(children, child => { if (isValidElement>(child) && (child.type === Tab || isSlot(child, Tab))) { - return cloneElement(child, {id: `${parentId}-tab-${tabIndex++}`, loadingCounters, iconsVisible}) + return cloneElement(child, {id: `${parentId}-tab-${tabIndex++}`}) } if (isValidElement(child) && (child.type === Panel || isSlot(child, Panel))) { @@ -124,7 +145,12 @@ const UnderlinePanels: FCWithSlotMarker = ({ else if (child.type === Panel || isSlot(child, Panel)) tabPanels.push(child) } return [tabs, tabPanels] - }, [children, parentId, loadingCounters, iconsVisible]) + }, [children, parentId]) + + const contextValue = useMemo( + () => ({iconsVisible, loadingCounters}), + [iconsVisible, loadingCounters], + ) const tabsHaveIcons = tabs.some(tab => React.isValidElement(tab) && tab.props.icon) @@ -173,24 +199,27 @@ const UnderlinePanels: FCWithSlotMarker = ({ } return ( - - - - {tabs} - - - {tabPanels} - + + + + + {tabs} + + + {tabPanels} + + ) } -const Tab: FCWithSlotMarker = ({'aria-selected': ariaSelected, onSelect, ...props}) => { +const TabImpl: FCWithSlotMarker = ({'aria-selected': ariaSelected, onSelect, ...props}) => { + const {iconsVisible, loadingCounters} = useContext(UnderlinePanelsContext) const clickHandler = React.useCallback( (event: React.MouseEvent) => { if (!event.defaultPrevented && typeof onSelect === 'function') { @@ -217,11 +246,20 @@ const Tab: FCWithSlotMarker = ({'aria-selected': ariaSelected, onSelec type="button" onClick={clickHandler} onKeyDown={keyDownHandler} + iconsVisible={iconsVisible} + loadingCounters={loadingCounters} {...props} /> ) } +// Memoized so that UnderlinePanels re-rendering (e.g. when iconsVisible flips) +// only re-renders Tabs whose own props actually changed. iconsVisible and +// loadingCounters reach Tab via UnderlinePanelsContext, so Tabs still react +// to those changes through context propagation. +TabImpl.displayName = 'UnderlinePanels.Tab' +const Tab = React.memo(TabImpl) as unknown as FCWithSlotMarker + Tab.displayName = 'UnderlinePanels.Tab' const Panel: FCWithSlotMarker = ({children, ...rest}) => { From f8deeaa5f0689455d20fe25c40064f8bdda6016e Mon Sep 17 00:00:00 2001 From: Matthew Costabile Date: Sat, 23 May 2026 12:32:49 +0000 Subject: [PATCH 4/5] test(UnderlinePanels): pin no-empty-frame, bounded mount renders, memo stability MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three regression tests for the perf refactor in this branch: 1. Tabs render synchronously on mount — no empty-tablist frame between the initial commit and the (previously) useEffect-driven setState. 2. The host wrapping UnderlinePanels renders at most once on mount. Catches the props-in-state anti-pattern (mount → effect → setState → second render) if someone reintroduces it. 3. Re-rendering an ancestor for an unrelated reason does not re-render any Tab body. iconsVisible / loadingCounters flow via context, Tab itself is React.memo'd, and consumer-provided children references are stable — so memo should skip every Tab on an unrelated parent render. The test pokes the host three times and asserts zero extra Tab body renders. Includes a patch changeset. --- .changeset/underlinepanels-render-cascade.md | 11 +++ .../UnderlinePanels/UnderlinePanels.test.tsx | 90 +++++++++++++++++++ 2 files changed, 101 insertions(+) create mode 100644 .changeset/underlinepanels-render-cascade.md diff --git a/.changeset/underlinepanels-render-cascade.md b/.changeset/underlinepanels-render-cascade.md new file mode 100644 index 00000000000..40ee8984e13 --- /dev/null +++ b/.changeset/underlinepanels-render-cascade.md @@ -0,0 +1,11 @@ +--- +'@primer/react': patch +--- + +UnderlinePanels: Eliminate the empty-tablist frame on mount and the cascading +re-render when icons toggle. Tabs and panels are now derived in render +(previously stored in state synced via `useEffect`), the list width is kept +in a ref instead of state, and `iconsVisible` / `loadingCounters` flow to +each tab via context — combined with `React.memo(Tab)`, that makes +resize-driven icon toggles update only the part of each tab that depends on +the change, not the whole tablist subtree. Behavior is unchanged. diff --git a/packages/react/src/experimental/UnderlinePanels/UnderlinePanels.test.tsx b/packages/react/src/experimental/UnderlinePanels/UnderlinePanels.test.tsx index 099eeb0f1b5..0767a008ac7 100644 --- a/packages/react/src/experimental/UnderlinePanels/UnderlinePanels.test.tsx +++ b/packages/react/src/experimental/UnderlinePanels/UnderlinePanels.test.tsx @@ -1,5 +1,6 @@ // Most of the functionality is already tested in [@github/tab-container-element](https://github.com/github/tab-container-element) +import type React from 'react' import {render, screen} from '@testing-library/react' import {describe, it, afterEach, expect, vi} from 'vitest' import UnderlinePanels from './UnderlinePanels' @@ -146,3 +147,92 @@ describe('UnderlinePanels', () => { }).toThrow('Only one tab can be selected at a time.') }) }) + +describe('UnderlinePanels — render architecture', () => { + it('renders tabs synchronously on mount (no empty-tablist frame)', () => { + // Regression: tabs and panels used to be stored in state and populated by + // a useEffect, which produced one empty render before the real children + // appeared. Now they're derived in render — querying the tablist + // synchronously after render() must already see every tab. + const {container} = render() + const tabs = container.querySelectorAll('[role="tab"]') + expect(tabs).toHaveLength(3) + expect(tabs[0]).toHaveTextContent('Tab 1') + expect(tabs[2]).toHaveTextContent('Tab 3') + }) + + it('bounds parent renders on initial mount', () => { + // Performance pin: UnderlinePanels itself should render at most twice + // during initial mount — once for the initial commit and at most once + // more if the resize observer fires synchronously to set iconsVisible. + // If a future change reintroduces the props-mirrored-in-state pattern + // (mount → effect → setState → second render), this would trip. + let parentRenderCount = 0 + function CountedHost(props: {children: React.ReactNode}) { + parentRenderCount++ + return {props.children} + } + render( + + One + Two + Three + Panel One + Panel Two + Panel Three + , + ) + // The host wrapper renders once; UnderlinePanels itself is allowed up + // to two renders (initial + a potential post-resize iconsVisible toggle + // if the test environment fires ResizeObserver synchronously). Anything + // higher indicates a regression to the old state-from-effect pattern. + expect(parentRenderCount).toBeLessThanOrEqual(1) + }) + + it('does not re-render tabs when an unrelated re-render of the parent occurs', () => { + // Regression / perf pin: Tab is React.memo'd and iconsVisible / + // loadingCounters flow via context, so re-rendering an ancestor for an + // unrelated reason should NOT re-render the Tab function bodies. We + // measure that by counting renders inside the Tab's own children, which + // sit inside React.memo and only get called when the Tab actually + // renders. + const tabBodyRenderCount = vi.fn() + function TabBody({label}: {label: string}) { + tabBodyRenderCount(label) + return <>{label} + } + + // Stable element so re-rendering the host with the same `tree` reference + // flows through unchanged to UnderlinePanels' useMemo (deps: + // [children, parentId]) — that's exactly the path React.memo on Tab is + // supposed to protect. + const tree = ( + + + + + + + + Panel A + Panel B + + ) + + const {rerender} = render(
{tree}
) + const initialCalls = tabBodyRenderCount.mock.calls.length + expect(initialCalls).toBe(2) // one per Tab on mount + + // Re-render with the SAME tree reference but a different host attribute + // — simulates an unrelated parent re-render. Tab elements are + // referentially stable, memo should skip every Tab. + rerender(
{tree}
) + rerender(
{tree}
) + rerender(
{tree}
) + + // TabBody render count must not have grown. If memoization regresses + // (e.g. iconsVisible re-baked into Tab via cloneElement), each + // re-render would produce two extra TabBody renders. + expect(tabBodyRenderCount.mock.calls.length).toBe(initialCalls) + }) +}) From 61f3421c069dacfff0a8aa5591263fc53ee041da Mon Sep 17 00:00:00 2001 From: Matthew Costabile Date: Sat, 23 May 2026 15:14:34 +0000 Subject: [PATCH 5/5] perf(UnderlinePanels): fold tabsHaveIcons into the tabs/panels memo MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit tabsHaveIcons was computed in render-body via tabs.some(...). The walk is cheap (O(N), single-digit constant) but it ran on every render and lived outside the cache that already produces tabs / tabPanels. Moving the .some() inside that same useMemo means: - tabsHaveIcons is now cached on [children, parentId], aligned with the cache for the data it walks. - One less O(N) iteration per render on the cache-hit path. - The closures that depend on it (the listWidth layout effect and the resize-observer callback) see exactly the value the memo produced — no chance of skew between when tabs were partitioned and when tabsHaveIcons was computed. Pure refactor; behaviour identical. Existing tests cover it. --- .../src/experimental/UnderlinePanels/UnderlinePanels.tsx | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/packages/react/src/experimental/UnderlinePanels/UnderlinePanels.tsx b/packages/react/src/experimental/UnderlinePanels/UnderlinePanels.tsx index 1ddf649c88e..c08b608122f 100644 --- a/packages/react/src/experimental/UnderlinePanels/UnderlinePanels.tsx +++ b/packages/react/src/experimental/UnderlinePanels/UnderlinePanels.tsx @@ -112,7 +112,7 @@ const UnderlinePanels: FCWithSlotMarker = ({ // called in the exact same order in every component render const parentId = useId(props.id) - const [tabs, tabPanels] = useMemo(() => { + const [tabs, tabPanels, tabsHaveIcons] = useMemo(() => { // Walk children, clone each Tab with a generated id, and each Panel with a // matching aria-labelledby. Derive in render so we never ship a // "before-the-effect-ran" empty-tablist frame and so that re-renders of @@ -144,7 +144,10 @@ const UnderlinePanels: FCWithSlotMarker = ({ if (child.type === Tab || isSlot(child, Tab)) tabs.push(child) else if (child.type === Panel || isSlot(child, Panel)) tabPanels.push(child) } - return [tabs, tabPanels] + + const tabsHaveIcons = tabs.some(tab => React.isValidElement(tab) && tab.props.icon) + + return [tabs, tabPanels, tabsHaveIcons] as const }, [children, parentId]) const contextValue = useMemo( @@ -152,8 +155,6 @@ const UnderlinePanels: FCWithSlotMarker = ({ [iconsVisible, loadingCounters], ) - const tabsHaveIcons = tabs.some(tab => React.isValidElement(tab) && tab.props.icon) - // The list's natural width (icons + labels). Used only inside the resize // observer to decide whether to show or hide the icons — never read in // render — so it lives in a ref to avoid an extra commit on mount and on