perf(UnderlineNav): eliminate cascading render on mount by mattcosta7 · Pull Request #7875 · primer/react

mattcosta7 · 2026-05-22T22:36:56Z

Closes #

Summary

Re-architects UnderlineNav to eliminate the cascading render that profiles
showed on mount. The previous implementation reported every item's measured
width back up to the parent through context setter callbacks, so an N-item
nav produced O(N²) item renders during initial mount plus a render-phase
setState for the external-aria-current swap. After this PR, mount produces
at most 3 component-level renders total, irrespective of N, and per-item
work is O(1) at every step.

All behavior is preserved. 28 tests pass — 19 pre-existing + 9 new that pin
the architecture, structural, and performance contracts.

Note

This PR is independent of the dev-only-effect dual-build work in #7874.
The two __DEV__ invariants in UnderlineNav continue to use the inline
if (__DEV__) { useEffect(...) } + eslint-disable react-hooks/rules-of-hooks
pattern here; #7874 will migrate them once it lands.

What was wrong

A profile of UnderlineNav showed a long cascading render chain on mount.
Tracing it back, the cause was the measurement plumbing.

Each UnderlineNav.Item used to dispatch two parent setStates in a
mount-time useLayoutEffect:

// UnderlineNavItem
useLayoutEffect(() => {
  const domRect = ref.current.getBoundingClientRect()
  // …
  setChildrenWidth({text, width: domRect.width})           // -> parent setState
  setNoIconChildrenWidth({text, width: domRect.width - i}) // -> parent setState
}, [ref, setChildrenWidth, setNoIconChildrenWidth])

Compounded with these problems:

The context value was a fresh object literal every render
({setChildrenWidth, setNoIconChildrenWidth, loadingCounters, iconsVisible}),
so every parent re-render forced every item to re-render.
The aria-current swap-from-menu logic ran during render via a
setState inside menuItems.map(...). React 18 tolerates this by
discarding the in-progress render, but it added an extra commit every
time the menu changed.
Widths were stored as Array<{text, width}> and looked up by item
text. Lookups were O(N) .find() scans, and items whose children
weren't a plain string silently looked up as 0, breaking the swap math.
Item prop refresh was O(N²): responsiveProps.items.map(item => validChildren.find(c => c.key === item.key)) ran inside two .map()
loops every render.
Refs were read during render to compute the overflow menu's anchor —
unsafe under concurrent rendering and produced a perceptible reflow on
the first open.

How this PR fixes it

Eleven focused commits — each one independently shippable, each one keeping
all tests green — that re-architect the measurement and overflow pipeline.

Architectural

`perf(UnderlineNav): measure items once from the parent`

Inverts measurement entirely. UnderlineNavItem is now a leaf — it tags its
own <li> with data-underlinenav-item="true" and is done. UnderlineNav
itself walks all rendered items in a single useIsomorphicLayoutEffect via
that selector, reads their geometry, and stores the results in refs (not
state, because only computeOverflow reads them and they shouldn't
re-render).

The old context callbacks (setChildrenWidth,
setNoIconChildrenWidth) are gone from UnderlineNavContext.

`perf(UnderlineNav): consolidate overflow state into a single object`

items, menuItems, iconsVisible, measured (and later childrenKey)
collapse into a single useState object so every overflow update —
measurement, swap, resize, children-change reset — is exactly one
commit, never three.

`perf(UnderlineNav): memoize Item and stabilize context value`

Wraps UnderlineNavItem in React.memo and memoizes UnderlineNavContext's
value with useMemo([loadingCounters, iconsVisible]). Re-renders of
UnderlineNav no longer cascade through items when their props haven't
changed.

`perf(UnderlineNav): lift aria-current swap out of render phase`

Moves the external-aria-current promotion (when a consumer changes
aria-current to a menu item, pull it into the visible list) out of
render-phase setState and into a useIsomorphicLayoutEffect. Render is
pure again; the effect commits only when a swap is actually needed.

Structural

`perf(UnderlineNav): collapse nested updates into one effect, one commit`

Makes overflowEffect pure (renames to computeOverflow) and extracts a
pure computeSwap helper. Merges the measurement and aria-current effects
into a single combined layout effect that produces at most one
setOverflowState per pass. trackedChildrenKey folds into
overflowState as childrenKey, so the derived-state reset on children
change is one setState instead of two.

`perf(UnderlineNav): key width and child lookups by React element key`

Replaces Array<{text, width}> + .find() with Map<elementKey, number>.
Fixes the silent-zero bug for items with non-string children (e.g.
Label, <><Icon /> Label</>), and brings prop-refresh from
O(N²) down to O(N) by building a validChildrenByKey Map once per render.

`perf(UnderlineNav): move overlay anchoring into a layout effect`

Stops reading containerRef.current / listRef.current during render. The
menu anchor's left coordinate is now computed in a layout effect with an
idempotent setter (no feedback loop). Concurrent-rendering safe.

Correctness

`fix(UnderlineNav): use safe keys for children signature and menu items`

Two latent bugs:

childrenKey joined item keys with | — collisions if a consumer used
| in their key={...}. Switched to U+0000 (NUL), which React rejects
inside keys outright.
ActionList.LinkItem's key={menuItemChildren} coerced non-string
children to '[object Object]' and broke reconciliation for menus of
such items. Now uses String(menuItem.key) — guaranteed-unique inside
the children set.

`perf(UnderlineNav): use the key Map in the aria-current swap pass`

One leftover O(N²) .find() in the swap pass's prop refresh; converted to
the same validChildrenByKey.get(...) path.

`style(UnderlineNav): bound the measurement loop by validChildren.length`

Loop tidying for @typescript-eslint/no-unnecessary-condition.

Tests

`test(UnderlineNav): cover architecture, structure, and perf changes`

Adds three test groups that pin the work above and would catch a regression:

Architecture: every item is tagged with [data-underlinenav-item]; a
standalone <UnderlineNav.Item> mounts without a parent (no upstream
measurement-callback dependency); the wrapper is marked
data-overflow-measured="true" synchronously after the layout effect.
Structure: the lifted aria-current swap effect promotes a menu item
into the visible list when a consumer moves aria-current onto it,
including for items whose children are not a plain string (the swap-
math-with-text-key regression).
Performance: each item-body renders at most twice during initial
mount with stable children; per-item render rate does not grow with N;
UnderlineNav itself renders at most 3 times total during initial mount.

Plus a changeset.

Profiles

Render counts during initial mount with stable children (verified by the
tests above):

	Before	After
Total parent renders	≥ 3 + one per resize tick	≤ 3
Per-item renders	grows with N (cascade)	constant 1–2
`setState` calls per overflow change	3 (split state) → 4 with swap	1 (consolidated)
Render-phase `setState`s	1 (aria-current swap)	0 (lifted to effect)
`.find()` calls inside `.map()`	2 per render (O(N²))	0 (Map lookups)
Refs read during render	yes (`containerRef`, `listRef`)	no

Changelog

New

Internal data-underlinenav-item attribute on every <li> rendered by
UnderlineNav.Item. Enables parent-side measurement.

Changed

UnderlineNav re-architected: parent-driven measurement, single
consolidated overflow state, React.memo on UnderlineNav.Item, single
combined layout effect, key-Map width and child lookups, layout-effect
overlay anchoring, safe children-signature delimiter and menu item key.
Behavior is identical.

Removed

Internal setChildrenWidth / setNoIconChildrenWidth from
UnderlineNavContext (not exported; were upstream-callback plumbing).
ChildSize / ChildWidthArray from
packages/react/src/UnderlineNav/types.ts; replaced with ChildWidthMap = Map<string, number>. Not exported from @primer/react.

Rollout strategy

Patch release
Minor release
Major release; if selected, include a written rollout or migration plan
None; if selected, include a brief description as to why

No public API changes. Pure internal refactor + correctness fixes for
already-supported but mis-handled cases (non-string children, key-with-pipe).

Testing & Reviewing

# Run UnderlineNav tests only:
cd packages/react
npx vitest run --project '@primer/react (chromium)' src/UnderlineNav/UnderlineNav.test.tsx
# 28 tests pass

Reviewer focus:

The combined layout effect's two-pass structure (measurement +
aria-current swap) and why it has no deps array. The comment block at
that effect captures the reasoning — does it survive future readers?
computeOverflow / computeSwap are now pure. Worth checking the
call-site setState payloads are correctly built (especially the
childrenKey: prev.childrenKey carry-over).
The data-underlinenav-item selector + positional pairing in the
measurement loop. Is the assumption "the N-th DOM item corresponds to
validChildren[N]" robust? It holds because measurement runs only
when all items are in the list (the children-key derived reset
guarantees this), but it's worth confirming.

Merge checklist

Added/updated tests
Added/updated documentation (changeset)
Added/updated previews (Storybook)
Changes are SSR compatible
Tested in Chrome
Tested in Firefox
Tested in Safari
Tested in Edge
(GitHub staff only) Integration tests pass at github/github-ui (Learn more about how to run integration tests)

Previously every UnderlineNav.Item dispatched two parent setState calls in a mount-time useLayoutEffect via context callbacks. With N items that produced multiple parent state updates during initial mount and a context value that was recreated on every commit, causing every item to re-render on every parent re-render. Invert measurement: drop the two setChildrenWidth/setNoIconChildrenWidth callbacks from UnderlineNavContext, remove the per-item measurement effect, and let UnderlineNav itself measure every rendered item in one layout effect via a [data-underlinenav-item] selector. Width arrays now live in refs (they are only read by overflowEffect and never need to trigger a re-render). Children-list changes are detected by joined-key signature and trigger a single measurement pass with all items rendered in the list. The test fixture container is widened to 2000px because measurement now applies overflow synchronously during the first commit (previously it waited for an async ResizeObserver tick); narrower fixtures would push items into the menu before render() returned.

Previously updateListAndMenu called three separate setStates (setResponsiveProps, setIconsVisible, setIsOverflowMeasured). Even with React 18 auto-batching this added intermediate-state risk and made the component logic harder to reason about. Combine items/menuItems/iconsVisible/measured into one useState object so the post-measurement update is a single, atomic commit. Children-list changes also reset to the measurement state in one setOverflowState call.

The Provider's value was a fresh object literal on every render, which forced every UnderlineNavItem context consumer to re-render whenever UnderlineNav re-rendered (e.g. when overflow state changed), regardless of whether the item's own props changed. - Wrap UnderlineNavItem with React.memo so re-renders of UnderlineNav don't re-render every item when their props are referentially stable. - Memoize UnderlineNavContext's value object with useMemo, keyed on loadingCounters and iconsVisible. Together these eliminate the gratuitous N-item re-renders on every parent state change.

The external-aria-current promotion logic (when a consumer marks a menu item as the current page, pull it into the visible list) ran inside the menuItems.map() JSX during render, calling updateListAndMenu — i.e. setState during render. React 18 tolerates this by discarding the in-progress render and re-running, but it added an extra render every time the menu changed. Move the check into a useIsomorphicLayoutEffect keyed on the menuItems array; it only commits an additional render when a swap is actually needed.

Adds three test groups: - Architecture: every Item is tagged with [data-underlinenav-item] for parent-side measurement; Item renders standalone without a parent (no upstream measurement callback dependency); the wrapper is marked data-overflow-measured="true" synchronously after the layout effect. - Structure: the lifted aria-current swap effect promotes a menu item into the visible list when the consumer moves aria-current onto it. - Performance: each item-body renders at most twice during initial mount, and the per-item render count does not grow with the number of items (catches regressions to the prior child-callback pattern). Includes the patch changeset.

Previously the parent did: - one setOverflowState from the measurement layout effect - one setOverflowState from the aria-current swap layout effect - two setStates from the children-key derived reset (trackedChildrenKey plus overflowState) - one setOverflowState from the ResizeObserver callback plus three setStates inside overflowEffect's old multi-arg callback, sharing the same call chain. That's up to four back-to-back commits on mount and on every overflow change. Re-architect the writers so every overflow change is exactly one commit: - Make the layout algorithm pure: rename overflowEffect to computeOverflow (returns {items, menuItems, iconsVisible} instead of calling a callback) and extract a pure computeSwap that returns the post-swap arrays. The setOverflowState call now lives at the call site, not inside the helpers. - Merge the measurement effect and the aria-current swap effect into one layout effect. It runs the measurement pass (when not yet measured), then the swap pass (when a menu item has aria-current), then commits at most one setOverflowState built from both passes. - Fold trackedChildrenKey into overflowState as a childrenKey field, so the derived-state reset on children change is one setState instead of two. - Drop the now-unused updateListAndMenu callback adapter. Also adds a test that asserts UnderlineNav itself renders at most 3 times during initial mount with stable children (initial render + the children-key derived reset + at most one combined measurement/swap commit). Anything that regresses to nested updates will trip it.

Two related fixes: 1. Width tracking was an Array<{text, width}> with O(N) linear scans (Array.prototype.find) keyed by item text. That silently returned 0 for items whose children weren't a plain string (e.g. Label, <><Icon /> Label</>), making the swap math degenerate and pulling the wrong items into the menu. Switch to Map<elementKey, width> keyed by String(child.key). Lookups are now O(1) and survive non-string item children. The measurement pass pairs DOM elements with validChildren positionally (the only time it runs is when all items are in the list, in render order). 2. The prop-refresh used Array.prototype.find inside .map on every render to look up the current React element by key — O(N^2) per render, executed twice (listItems + menuItems). Build a Map<key, element> once and look up in O(1). Renames ChildWidthArray -> ChildWidthMap and getItemsWidth(text) -> getItemNoIconWidth(item). Adds a regression test that promotes a menu item whose children are Name via external aria-current — catches a future revert to text-based lookup.

The overflow menu's left coordinate was computed inline during render by reading containerRef.current and listRef.current, then merged with baseMenuInlineStyles based on listRef.current?.clientWidth. Reading mutable refs during render is unsafe under concurrent rendering: the refs are null on the first render (no commit yet) and may carry stale data on subsequent renders, producing a perceptible reflow on the first open of the menu. Move the anchoring into a useIsomorphicLayoutEffect that: - Reads containerRef / listRef from a post-commit context. - Sets state only when the anchor value actually changes (idempotent setter pattern avoids feedback loops on this layout-driven state). - Re-runs when isWidgetOpen, isOverflowMeasured, menuItems.length, or menuAnchorLeft change. The render now consumes a precomputed menuStyle CSSProperties object based on menuAnchorLeft, with no ref reads in render. Adds a test that opens the More menu in a narrow container and asserts the overlay is displayed with display: block — verifies the new layout effect doesn't crash on first open and applies its style.

The DOM-to-element pairing in the measurement effect used a guard clause (if (!child) continue) that TypeScript inferred as unreachable (@typescript-eslint/no-unnecessary-condition). Replace it with an explicit upper bound (Math.min) so the loop body is statically known to operate on valid indices.

Two small key-correctness fixes: 1. childrenKey delimiter was '|' (pipe). If a consumer used a pipe in their UnderlineNav.Item key (e.g. key={`${repo}|${tab}`}) the joined signature could collide across distinct children lists, and the measurement-reset detection would silently skip remeasuring. Switch the delimiter to U+0000 (NUL), which React explicitly rejects inside element keys — guaranteeing no collision. 2. The overflow menu's ActionList.LinkItem used key={menuItemChildren}. React keys must be string/number; when an item's children isn't a plain string (a React element, fragment, array, etc.) the value is coerced to '[object Object]' / a joined string, making every such item share the same key and breaking reconciliation. Use the React element's own key (assigned by React.Children.toArray) instead, which is always a unique string within the children set.

Leftover from the Map-based lookup refactor. Pass 2 of the combined layout effect was still doing validChildren.find(c => c.key === ...) inside a .map() — O(N^2) per commit. Switch to the same validChildrenByKey Map already built once at the top of the render function.

changeset-bot · 2026-05-22T22:37:01Z

🦋 Changeset detected

Latest commit: badd8a4

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@primer/react	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions · 2026-05-22T22:37:29Z

⚠️ Action required

👋 Hi, this pull request contains changes to the source code that github/github-ui depends on. If you are GitHub staff, test these changes with github/github-ui using the integration workflow. Check the integration testing docs for step-by-step instructions. Or, apply the integration-tests: skipped manually label to skip these checks.

To publish a canary release for integration testing, apply the Canary Release label to this PR.

mattcosta7 added 11 commits May 22, 2026 22:31

github-actions Bot added the integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm label May 22, 2026

mattcosta7 deployed to github-pages May 22, 2026 22:47 — with GitHub Actions View deployment

github-actions Bot deployed to storybook-preview-7875 May 22, 2026 22:47 View deployment

mattcosta7 marked this pull request as ready for review May 23, 2026 01:55

mattcosta7 requested a review from a team as a code owner May 23, 2026 01:55

mattcosta7 requested review from TylerJDev and Copilot and removed request for Copilot May 23, 2026 01:55

mattcosta7 self-assigned this May 23, 2026

mattcosta7 added the Canary Release Apply this label when you want CI to create a canary release of the current PR label May 23, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf(UnderlineNav): eliminate cascading render on mount#7875

perf(UnderlineNav): eliminate cascading render on mount#7875
mattcosta7 wants to merge 11 commits into
mainfrom
perf/underlinenav-render-cascade

mattcosta7 commented May 22, 2026

Uh oh!

changeset-bot Bot commented May 22, 2026

Uh oh!

github-actions Bot commented May 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

mattcosta7 commented May 22, 2026

Summary

What was wrong

How this PR fixes it

Architectural

Structural

Correctness

Tests

Profiles

Changelog

New

Changed

Removed

Rollout strategy

Testing & Reviewing

Merge checklist

Uh oh!

changeset-bot Bot commented May 22, 2026

🦋 Changeset detected

Uh oh!

github-actions Bot commented May 22, 2026

⚠️ Action required

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant