Cascade elimination nav panels#7865
Closed
mattcosta7 wants to merge 3 commits into
Closed
Conversation
🦋 Changeset detectedLatest commit: 59423af The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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
Bot
added
the
integration-tests: recommended
Contributor
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aims to reduce mount-time render cascades in UnderlineNav and experimental UnderlinePanels by restructuring DOM measurement and derived state to avoid extra commit cycles, and pins the improvements with profiler-based tests.
Changes:
UnderlineNav: replaces per-item layout-effect measurement with a single parent-driven layout effect, stores measured widths in refs, and adds key-based equality guards to avoid ResizeObserver’s initial “no-op” commit.UnderlinePanels: derivestabs/tabPanelsinline viauseMemo, removes derived-state-via-effect, and consolidates icon-visibility measurement into a shared callback used by layout effect + ResizeObserver.- Adds profiler tests asserting
<= 2commits on mount, plus a patch changeset.
Show a summary per file
| File | Description |
|---|---|
| packages/react/src/UnderlineNav/UnderlineNavItem.tsx | Removes per-item measurement effect; adds data-underline-nav-item marker for parent measurement. |
| packages/react/src/UnderlineNav/UnderlineNavContext.tsx | Narrows context shape by removing width-dispatch setters. |
| packages/react/src/UnderlineNav/UnderlineNav.tsx | Implements parent-driven measurement; moves widths to refs; adds responsiveProps equality guard; memoizes valid children. |
| packages/react/src/UnderlineNav/UnderlineNav.test.tsx | Adds profiler test to cap mount commit count. |
| packages/react/src/experimental/UnderlinePanels/UnderlinePanels.tsx | Replaces effect-synced state with useMemo derivation; removes listWidth state; adds shared recompute callback. |
| packages/react/src/experimental/UnderlinePanels/UnderlinePanels.test.tsx | Adds profiler test to cap mount commit count. |
| .changeset/underline-nav-cascade-reduction.md | Adds patch changeset documenting cascade reductions. |
Copilot's findings
- Files reviewed: 7/7 changed files
- Comments generated: 3
Comment on lines
+326
to
+336
| const textEl = inner.querySelector('[data-component="text"]') | ||
| const iconEl = inner.querySelector<HTMLElement>('[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}) |
Comment on lines
+133
to
+143
| // 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]) |
Comment on lines
74
to
+77
| forwardedRef, | ||
| ) => { | ||
| const backupRef = useRef<HTMLElement>(null) | ||
| const ref = (forwardedRef ?? backupRef) as RefObject<HTMLAnchorElement> | ||
| const {setChildrenWidth, setNoIconChildrenWidth, loadingCounters, iconsVisible} = useContext(UnderlineNavContext) | ||
|
|
||
| useLayoutEffect(() => { | ||
| if (ref.current) { | ||
| const domRect = (ref as MutableRefObject<HTMLElement>).current.getBoundingClientRect() | ||
|
|
||
| const icon = Array.from((ref as MutableRefObject<HTMLElement>).current.children).find( | ||
| child => child.getAttribute('data-component') === 'icon', | ||
| ) | ||
|
|
||
| const content = Array.from((ref as MutableRefObject<HTMLElement>).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<HTMLAnchorElement> | undefined | ||
| const {loadingCounters, iconsVisible} = useContext(UnderlineNavContext) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #
This PR removes two render cascades on mount: the per-item measurement loop in
UnderlineNavand the derived-state-via-effect pattern inUnderlinePanels. Both components measure DOM after first paint to decide what to render — those measurements need to be applied before a second paint to avoid layout shift, but the way the work was structured was producing extra commits beyond the unavoidable one.This branch is independent of #7864 and rebases cleanly onto
main.Changelog
Changed
1.
UnderlineNav— collapse N×2 child setStates into a single parent-driven measurementFiles:
UnderlineNav.tsx,UnderlineNavItem.tsx,UnderlineNavContext.tsxBefore: every
UnderlineNav.Itemran its ownuseLayoutEffectthat measuredgetBoundingClientRect()+ icon dimensions and dispatchedsetChildrenWidth+setNoIconChildrenWidthto the parent. For N items that meant 2N setStates queued during the commit phase, all batched by React 18 into one re-commit. ThenuseResizeObserver's initial entry fired and calledoverflowEffect→updateListAndMenu, which always calledsetResponsivePropswith a freshly-constructed object — committing another render even when the data hadn't changed.Net result on mount: 1 mount commit + 1 nested-update from the child-measurement batch + 1 from the observer's initial fire = ≥3 commits before paint stabilized.
After:
UnderlineNavItem. Each item now just adds adata-underline-nav-itemattribute on its<li>wrapper so the parent can find it.UnderlineNavruns a single parent-drivenuseIsomorphicLayoutEffectthat queries every[data-underline-nav-item]once, measures inner widths and icon dimensions in one pass, stores the results in refs (not state — they're never read during render), and callsoverflowEffectsynchronously with the just-measured values.updateListAndMenunow compares incomingitems/menuItemsto the current state bykey. The observer's initial fire reports the same dimensions the parent layout effect already measured, soupdateListAndMenureturns the previous state object and React bails — no extra render.UnderlineNavContextno longer needs to publishsetChildrenWidth/setNoIconChildrenWidthsinceUnderlineNavItemdoesn't dispatch widths anymore.Net result on mount: 1 mount commit + 1 measurement-driven commit = ≤2 commits, regardless of how many items the nav contains.
The math inside
overflowEffectis byte-for-byte identical tomain; only the data flow into it changed.2.
UnderlinePanels— derive tabs inline, fold list-width measurement into the icon-visibility decisionFiles:
UnderlinePanels.tsxBefore:
tabsandtabPanelswere held inuseStateand synced fromchildrenvia a post-paintuseEffect. Initial render hadtabs = [], committed empty markup, then the effect firedsetTabs(newTabs)+setTabPanels(newTabPanels)and re-rendered with real data. Same extra commit fired on every change tochildren/loadingCounters/iconsVisible.listWidthwas a separateuseStatewhose only purpose was to feed theiconsVisiblecomparison inside theResizeObservercallback. The initial measurement ran in its ownuseIsomorphicLayoutEffectthat committed an extra setState before the observer had even fired.After:
tabsandtabPanelsare derived viauseMemofromchildren/parentId/loadingCounters/iconsVisible. Initial render now produces the correct markup directly — no post-paint sync.tabsHaveIconsis memoized as well.listWidthstate is gone. A newrecomputeIconsVisiblecallback measures both list and wrapper at decision time and is the single source of truth used by both the layout effect and the resize observer. The callback'ssetIconsVisible(prev => prev === next ? prev : next)bails when the value hasn't changed.Net result on mount: 1 commit + at most 1 measurement-driven commit when icons need to be hidden = ≤2 commits. Children / state changes elsewhere no longer produce an extra commit from the dropped
useEffect.Identity stability of
tabsandtabPanelsimproves as a side effect —useMemoreturns the same array reference when inputs are unchanged, whereas the previous effect created a new array on every run.Removed
No public API changes.
UnderlineNavContextis module-internal; its narrowing is invisible to consumers.Rollout strategy
No public API surface changes.
UnderlineNav.Item's ref forwarding behaviour is unchanged, the wrapper<li>gains adata-underline-nav-itemattribute (additive), andUnderlinePanels's rendered markup is identical tomain.Testing & Reviewing
Existing test suites should continue to pass — both files only restructure the data flow into the existing overflow algorithm (
UnderlineNav) and the existing tab/panel rendering (UnderlinePanels). Reviewer high-signal spots:UnderlineNav.test.tsx"Keyboard Navigation" — tab order depends onresponsiveProps.itemsbeing correct after measurement. If the new parent measurement misses an item, this fails.UnderlineNav.test.tsxAVT/VRT runs — visually confirm the more-menu still produces the right items on resize and that swapping a menu item into the visible list works.UnderlinePanels.test.tsxids / aria-labelledby — these depend ontabs/tabPanelsbeing derived correctly during render. TheuseMemomove should actually improve timing here (was post-paint, now during render).New profiler tests added:
UnderlineNav.test.tsx— asserts<=2commits when mounting a 7-item nav.UnderlinePanels.test.tsx— asserts<=2commits when mounting a 3-tab panel.Both use
React.Profilerdirectly (no shared harness; this PR is self-contained).Manual verification I'd recommend before approving:
Components/UnderlineNav → Defaultonmain, then on this branch. Confirm the commit count drops.Behavioral risk callouts
li.firstElementChildto get the link element. EveryUnderlineNav.Itemrenders<li><UnderlineItem /></li>, so internal usage is fine. External consumers that wrapUnderlineNav.Itemin something else are unaffected because they wrap around the item, not inside its<li>.tabs/tabPanelsidentity is now stable across renders. If anything downstream ofUnderlinePanels(e.g. effects in slot consumers) usedtabsas a referential-equality dep, those effects now fire less often. This is almost certainly a strict improvement, but worth eyeballing.updateListAndMenuskips a setState when bothitems[i].key === prev.items[i].keyfor alliand the lengths match. If two items have the same key but different content (a key collision that React would already warn about), the equality guard would bail when it shouldn't. The existinggetValidChildrendoesn't deduplicate keys, so a key collision is a pre-existing bug that this code doesn't worsen.Merge checklist
github/github-ui