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..28761c9f9db 100644 --- a/packages/react/src/UnderlineNav/UnderlineNav.tsx +++ b/packages/react/src/UnderlineNav/UnderlineNav.tsx @@ -175,17 +175,36 @@ export const UnderlineNav = forwardRef( menuItems: [], }) + // 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('|') + + // `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 = 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 @@ -294,19 +313,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}