From 7749e0da6eaf7baec66a106846e8159880cd434f Mon Sep 17 00:00:00 2001 From: Liu Liu Date: Tue, 23 Jun 2026 11:42:51 -0700 Subject: [PATCH 1/3] on child tabs change --- ...nderlinenav-rerender-on-children-change.md | 5 ++ .../UnderlineNav.features.stories.tsx | 27 ++++++ .../src/UnderlineNav/UnderlineNav.test.tsx | 46 +++++++++- .../react/src/UnderlineNav/UnderlineNav.tsx | 89 ++++++++++++------- 4 files changed, 136 insertions(+), 31 deletions(-) create mode 100644 .changeset/underlinenav-rerender-on-children-change.md diff --git a/.changeset/underlinenav-rerender-on-children-change.md b/.changeset/underlinenav-rerender-on-children-change.md new file mode 100644 index 00000000000..13e60e5757e --- /dev/null +++ b/.changeset/underlinenav-rerender-on-children-change.md @@ -0,0 +1,5 @@ +--- +'@primer/react': patch +--- + +Fix `UnderlineNav` not re-rendering when child tabs are added, removed, or reordered. diff --git a/packages/react/src/UnderlineNav/UnderlineNav.features.stories.tsx b/packages/react/src/UnderlineNav/UnderlineNav.features.stories.tsx index 6b35a2c739b..39c1749ba87 100644 --- a/packages/react/src/UnderlineNav/UnderlineNav.features.stories.tsx +++ b/packages/react/src/UnderlineNav/UnderlineNav.features.stories.tsx @@ -13,6 +13,7 @@ import { } from '@primer/octicons-react' import type {Meta} from '@storybook/react-vite' import {UnderlineNav} from './index' +import {Button} from '../Button' import {INITIAL_VIEWPORTS} from 'storybook/viewport' const meta = { @@ -154,3 +155,29 @@ export const VariantFlush = () => { ) } + +export const DynamicChildren = () => { + const [showItem, setShowItem] = React.useState(false) + + return ( +
+ + + + Code + + Pull requests + Actions + Projects + Wiki + {showItem && Another} + +
+ ) +} diff --git a/packages/react/src/UnderlineNav/UnderlineNav.test.tsx b/packages/react/src/UnderlineNav/UnderlineNav.test.tsx index 1ab761f8e34..0ea57f7aba6 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 { @@ -240,6 +240,50 @@ describe('UnderlineNav', () => { const textSpan = item.querySelector('[data-component="text"]') expect(textSpan).toHaveAttribute('data-content', 'Simple Text') }) + + // Regression test for https://github.com/github/primer/issues/6389 + it('re-renders when a conditionally rendered child tab is toggled', async () => { + const Example = () => { + const [showItem, setShowItem] = React.useState(false) + return ( +
+ + + + Code + + Pull requests + Actions + Projects + Wiki + {showItem && Another} + +
+ ) + } + + render() + const toggle = screen.getByRole('button', {name: 'show item'}) + const user = userEvent.setup() + + // Initially the conditional tab is not rendered + expect(screen.queryByText('Another')).not.toBeInTheDocument() + + // Toggle on: the new tab should appear in the DOM (either in the visible list + // or the overflow menu) without the nav being remounted. + await user.click(toggle) + expect(screen.getByText('Another')).toBeInTheDocument() + // Existing tabs are still present + expect(screen.getByText('Code')).toBeInTheDocument() + expect(screen.getByText('Pull requests')).toBeInTheDocument() + + // Toggle off: the tab should be removed + await user.click(screen.getByRole('button', {name: 'hide item'})) + expect(screen.queryByText('Another')).not.toBeInTheDocument() + expect(screen.getByText('Code')).toBeInTheDocument() + }) }) describe('Keyboard Navigation', () => { diff --git a/packages/react/src/UnderlineNav/UnderlineNav.tsx b/packages/react/src/UnderlineNav/UnderlineNav.tsx index 21774554c2f..13bdf0b5320 100644 --- a/packages/react/src/UnderlineNav/UnderlineNav.tsx +++ b/packages/react/src/UnderlineNav/UnderlineNav.tsx @@ -162,30 +162,57 @@ export const UnderlineNav = forwardRef( const [isWidgetOpen, setIsWidgetOpen] = useState(false) const [iconsVisible, setIconsVisible] = useState(true) - const [childWidthArray, setChildWidthArray] = useState([]) - const [noIconChildWidthArray, setNoIconChildWidthArray] = useState([]) + // Width measurements are stored keyed by item text so that adding/removing/reordering + // children does not produce stale or out-of-order entries (as the previous append-only + // arrays did). See https://github.com/github/primer/issues/6389. + const [childWidthMap, setChildWidthMap] = useState>({}) + const [noIconChildWidthMap, setNoIconChildWidthMap] = useState>({}) // Track whether the initial overflow calculation is complete to prevent CLS const [isOverflowMeasured, setIsOverflowMeasured] = useState(false) const validChildren = getValidChildren(children) + // Build ordered width arrays from the current children so order always matches + // `validChildren`, even after items are added, removed, or reordered. + const childWidthArray: ChildWidthArray = validChildren.map(child => { + const text = child.props.children as string + return {text, width: childWidthMap[text] ?? 0} + }) + const noIconChildWidthArray: ChildWidthArray = validChildren.map(child => { + const text = child.props.children as string + return {text, width: noIconChildWidthMap[text] ?? 0} + }) + // Responsive props object manages which items are in the list and which items are in the menu. const [responsiveProps, setResponsiveProps] = useState({ items: validChildren, menuItems: [], }) + // Detect when children added/removed/reordered relative to the last overflow calculation. + // When that happens, `responsiveProps` is stale, so we render `validChildren` directly + // (placing all items inline with no overflow menu) until the layout effect below recomputes. + const childrenSignature = validChildren.map(child => String(child.key ?? '')).join('|') + const responsiveSignature = [...responsiveProps.items, ...responsiveProps.menuItems] + .map(item => String(item.key ?? '')) + .join('|') + const childrenChanged = childrenSignature !== responsiveSignature + // Make sure to have the fresh props data for list items when children are changed (keeping aria-current up-to-date) - const listItems = responsiveProps.items.map(item => { - return validChildren.find(child => child.key === item.key) ?? item - }) + const listItems = childrenChanged + ? validChildren + : responsiveProps.items.map(item => { + return validChildren.find(child => child.key === item.key) ?? item + }) // Make sure to have the fresh props data for menu items when children are changed (keeping aria-current up-to-date) - const menuItems = responsiveProps.menuItems.map(menuItem => { - return validChildren.find(child => child.key === menuItem.key) ?? menuItem - }) + const menuItems = childrenChanged + ? [] + : responsiveProps.menuItems.map(menuItem => { + return validChildren.find(child => child.key === menuItem.key) ?? menuItem + }) // This is the case where the viewport is too narrow to show any list item with the more menu. In this case, we only show the dropdown - const onlyMenuVisible = responsiveProps.items.length === 0 + const onlyMenuVisible = !childrenChanged && responsiveProps.items.length === 0 useDevOnlyEffect(() => { // Address illegal state where there are multiple items that have `aria-current='page'` attribute @@ -253,17 +280,11 @@ export const UnderlineNav = forwardRef( [], ) const setChildrenWidth = useCallback((size: ChildSize) => { - setChildWidthArray(arr => { - const newArr = [...arr, size] - return newArr - }) + setChildWidthMap(prev => (prev[size.text] === size.width ? prev : {...prev, [size.text]: size.width})) }, []) const setNoIconChildrenWidth = useCallback((size: ChildSize) => { - setNoIconChildWidthArray(arr => { - const newArr = [...arr, size] - return newArr - }) + setNoIconChildWidthMap(prev => (prev[size.text] === size.width ? prev : {...prev, [size.text]: size.width})) }, []) const closeOverlay = React.useCallback(() => { @@ -294,19 +315,27 @@ export const UnderlineNav = forwardRef( useOnOutsideClick({onClickOutside: closeOverlay, containerRef, ignoreClickRefs: [moreMenuBtnRef]}) - useResizeObserver((resizeObserverEntries: ResizeObserverEntry[]) => { - const navWidth = resizeObserverEntries[0].contentRect.width - const moreMenuWidth = moreMenuRef.current?.getBoundingClientRect().width ?? 0 - navWidth !== 0 && - overflowEffect( - navWidth, - moreMenuWidth, - validChildren, - childWidthArray, - noIconChildWidthArray, - updateListAndMenu, - ) - }, navRef as RefObject) + // The resize observer fires when the nav element resizes, and re-observes (firing the + // callback again with current size) whenever its deps change. Passing `childrenSignature` + // ensures the overflow calculation re-runs when child tabs are added, removed, or + // reordered. See https://github.com/github/primer/issues/6389. + useResizeObserver( + (resizeObserverEntries: ResizeObserverEntry[]) => { + const navWidth = resizeObserverEntries[0].contentRect.width + const moreMenuWidth = moreMenuRef.current?.getBoundingClientRect().width ?? 0 + navWidth !== 0 && + overflowEffect( + navWidth, + moreMenuWidth, + validChildren, + childWidthArray, + noIconChildWidthArray, + updateListAndMenu, + ) + }, + navRef as RefObject, + [childrenSignature], + ) // Compute menuInlineStyles if needed let menuInlineStyles: React.CSSProperties = {...baseMenuInlineStyles} From 3902288716a21e4322004bb0582a5ec442563c21 Mon Sep 17 00:00:00 2001 From: Liu Liu Date: Tue, 23 Jun 2026 12:12:12 -0700 Subject: [PATCH 2/3] fix: only treat membership change as stale children, not internal swap reorders --- .../react/src/UnderlineNav/UnderlineNav.tsx | 20 ++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/packages/react/src/UnderlineNav/UnderlineNav.tsx b/packages/react/src/UnderlineNav/UnderlineNav.tsx index 13bdf0b5320..8285cb0ecc3 100644 --- a/packages/react/src/UnderlineNav/UnderlineNav.tsx +++ b/packages/react/src/UnderlineNav/UnderlineNav.tsx @@ -189,14 +189,20 @@ export const UnderlineNav = forwardRef( menuItems: [], }) - // Detect when children added/removed/reordered relative to the last overflow calculation. - // When that happens, `responsiveProps` is stale, so we render `validChildren` directly - // (placing all items inline with no overflow menu) until the layout effect below recomputes. + // The ordered signature drives `useResizeObserver` deps below so it re-fires whenever + // children are added, removed, or reordered. const childrenSignature = validChildren.map(child => String(child.key ?? '')).join('|') - const responsiveSignature = [...responsiveProps.items, ...responsiveProps.menuItems] - .map(item => String(item.key ?? '')) - .join('|') - const childrenChanged = childrenSignature !== responsiveSignature + + // `childrenChanged` only tracks membership (set of keys), not order. This is intentional + // because `swapMenuItemWithListItem` legitimately reorders `responsiveProps.items` (to keep + // the selected item visible), and we don't want that to be treated as a stale-children + // situation. We only need to fall back to rendering `validChildren` directly when a child + // was actually added or removed (so a new item isn't missed and a removed item isn't kept). + const responsiveKeys = new Set() + for (const item of responsiveProps.items) responsiveKeys.add(String(item.key ?? '')) + for (const item of responsiveProps.menuItems) responsiveKeys.add(String(item.key ?? '')) + const validKeys = validChildren.map(child => String(child.key ?? '')) + const childrenChanged = validKeys.length !== responsiveKeys.size || validKeys.some(key => !responsiveKeys.has(key)) // Make sure to have the fresh props data for list items when children are changed (keeping aria-current up-to-date) const listItems = childrenChanged From 6e635127ee36b2b7decd5701e1a7afc46898293f Mon Sep 17 00:00:00 2001 From: Liu Liu Date: Tue, 23 Jun 2026 13:39:30 -0700 Subject: [PATCH 3/3] fix: revert width tracking to positional arrays to avoid breaking non-string children --- .../react/src/UnderlineNav/UnderlineNav.tsx | 28 +++++++------------ 1 file changed, 10 insertions(+), 18 deletions(-) diff --git a/packages/react/src/UnderlineNav/UnderlineNav.tsx b/packages/react/src/UnderlineNav/UnderlineNav.tsx index 8285cb0ecc3..28761c9f9db 100644 --- a/packages/react/src/UnderlineNav/UnderlineNav.tsx +++ b/packages/react/src/UnderlineNav/UnderlineNav.tsx @@ -162,27 +162,13 @@ export const UnderlineNav = forwardRef( const [isWidgetOpen, setIsWidgetOpen] = useState(false) const [iconsVisible, setIconsVisible] = useState(true) - // Width measurements are stored keyed by item text so that adding/removing/reordering - // children does not produce stale or out-of-order entries (as the previous append-only - // arrays did). See https://github.com/github/primer/issues/6389. - const [childWidthMap, setChildWidthMap] = useState>({}) - const [noIconChildWidthMap, setNoIconChildWidthMap] = useState>({}) + const [childWidthArray, setChildWidthArray] = useState([]) + const [noIconChildWidthArray, setNoIconChildWidthArray] = useState([]) // Track whether the initial overflow calculation is complete to prevent CLS const [isOverflowMeasured, setIsOverflowMeasured] = useState(false) const validChildren = getValidChildren(children) - // Build ordered width arrays from the current children so order always matches - // `validChildren`, even after items are added, removed, or reordered. - const childWidthArray: ChildWidthArray = validChildren.map(child => { - const text = child.props.children as string - return {text, width: childWidthMap[text] ?? 0} - }) - const noIconChildWidthArray: ChildWidthArray = validChildren.map(child => { - const text = child.props.children as string - return {text, width: noIconChildWidthMap[text] ?? 0} - }) - // Responsive props object manages which items are in the list and which items are in the menu. const [responsiveProps, setResponsiveProps] = useState({ items: validChildren, @@ -286,11 +272,17 @@ export const UnderlineNav = forwardRef( [], ) const setChildrenWidth = useCallback((size: ChildSize) => { - setChildWidthMap(prev => (prev[size.text] === size.width ? prev : {...prev, [size.text]: size.width})) + setChildWidthArray(arr => { + const newArr = [...arr, size] + return newArr + }) }, []) const setNoIconChildrenWidth = useCallback((size: ChildSize) => { - setNoIconChildWidthMap(prev => (prev[size.text] === size.width ? prev : {...prev, [size.text]: size.width})) + setNoIconChildWidthArray(arr => { + const newArr = [...arr, size] + return newArr + }) }, []) const closeOverlay = React.useCallback(() => {