fix issues with navlist static to interactive subnav state

[RSoeborg]

Changelog

This PR fixes a visual flicker in NavList where a parent item with SubNav could briefly appear in the wrong state when navigating between sibling sub-items.

A small additional change when touching this code, is that the "subNavRef" had the wrong type HTMLDivElement, I changed that to HTMLUListElement

This can be seen here (example happening on the docs site primer.style):

example.from.docs.mov

Notice how Foundations flicker when I change submenu.

Here is a minimal example (amplified)

amplified.example.mov

I updated the dev storybook with an amplified example, that makes it easier to see whats actually happening.

Example after fix

postfix.mov

This is how it looks after the fix.

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

Testing & Reviewing

Just replace these parts:

  const [isOpen, setIsOpen] = React.useState((defaultOpen || null) ?? hasCurrentItem)
  const subNavRef = React.useRef<HTMLUListElement>(null)
  const [containsCurrentItem, setContainsCurrentItem] = React.useState(hasCurrentItem)

With the "current" behaviour:

  const [isOpen, setIsOpen] = React.useState((defaultOpen || null) ?? false)
  const subNavRef = React.useRef<HTMLUListElement>(null)
  const [containsCurrentItem, setContainsCurrentItem] = React.useState(false)

and test in storybook: http://localhost:6006/?path=/story/components-navlist-dev--sub-nav-static-to-interactive-transition

Merge checklist

• Added/updated tests
• Added/updated documentation
• 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 (Learn more about how to run integration tests)

[changeset-bot]

🦋 Changeset detected

Latest commit: 92c7214

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

[Copilot]

Pull request overview

Fixes a NavList flicker where a parent item with a SubNav briefly renders in the wrong expanded/current state when transitioning from static (SSR markup) to interactive (hydrated) state.

Changes:

• Add a recursive pre-render check to detect whether a SubNav contains the current item and initialize the parent’s open state accordingly.
• Add an SSR-focused unit test to verify initial expanded rendering when a current SubNav item exists.
• Add a dev Storybook scenario that reproduces the static-to-interactive transition flicker.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
packages/react/src/NavList/NavList.tsx	Pre-computes “contains current item” before first render to avoid initial-state mismatch/flicker.
packages/react/src/NavList/NavList.test.tsx	Adds SSR test to validate initial expanded state when SubNav contains the current item.
packages/react/src/NavList/NavList.dev.stories.tsx	Adds an amplified Storybook repro by switching between static HTML and interactive render.

[Copilot]

Importing renderToStaticMarkup from react-dom/server in a Storybook story (browser bundle) can break Vite/Storybook builds because react-dom/server is Node-oriented. Prefer switching this import to react-dom/server.browser (if available in your React version), or restructure the story to avoid bundling the server renderer into the client (e.g., generate the static markup outside the browser bundle / via a build-time step).

Suggested change

	import {renderToStaticMarkup} from 'react-dom/server'
	import {renderToStaticMarkup} from 'react-dom/server.browser'

[RSoeborg]

We can remove the .dev.stories after a maintainer has tested the changes locally. It's just to showcase the bug

[Copilot]

Importing renderToStaticMarkup from react-dom/server in a Storybook story (browser bundle) can break Vite/Storybook builds because react-dom/server is Node-oriented. Prefer switching this import to react-dom/server.browser (if available in your React version), or restructure the story to avoid bundling the server renderer into the client (e.g., generate the static markup outside the browser bundle / via a build-time step).

[RSoeborg]

Same as above, we can remove the .dev.stories after a maintainer has tested the changes locally. It's just to showcase the bug

[hectahertz]

Thanks for the contribution @RSoeborg! The videos really helped, the flicker is immediately obvious and the before/after comparison makes the fix crystal clear. Great job explaining the issue. Let's fix a couple things and merge this :)

hasCurrentNavItem walks the React children tree

This recursively calls isValidElement, reads node.props['aria-current'], and iterates children with React.Children.toArray. I understand why, the existing querySelector approach runs in a layout effect which is too late to avoid the flash. But now we have two code paths doing the same check differently (React tree walk for initial render, DOM query in the effect), and they need to stay in sync.

Could we remove the querySelector path entirely and rely only on hasCurrentNavItem? If the React tree walk is correct at init time, it should also be correct on subsequent updates. That way there's one source of truth instead of two. If there's a reason the DOM query is still needed (e.g. children that don't have aria-current in props but do in the rendered DOM), let's document that.

Test assertion is too loose

expect(markup).toContain('aria-expanded="true"')

This passes if any element in the markup has aria-expanded="true", not specifically the parent item ("Item 2"). Parse the markup into a container and assert on the specific button:

const container = document.createElement('div')
container.innerHTML = renderToStaticMarkup(<NavListWithCurrentSubNav />)
const item2Button = container.querySelector('button[aria-expanded]')
expect(item2Button).toHaveAttribute('aria-expanded', 'true')
// or even better, query by accessible name to be explicit about which item

Remove the dev story

I've seen the issue and confirmed the fix, so let's remove the dev story before merge. The renderToStaticMarkup import from react-dom/server in a browser bundle would cause build issues anyway, and the test covers the behavior.

Minor: ref type change

The subNavRef type changed from HTMLDivElement to HTMLUListElement. This looks correct but is unrelated to the fix, worth calling out in the PR description.

[RSoeborg]

Could we remove the querySelector path entirely and rely only on hasCurrentNavItem? If the React tree walk is correct at init time, it should also be correct on subsequent updates. That way there's one source of truth instead of two. If there's a reason the DOM query is still needed (e.g. children that don't have aria-current in props but do in the rendered DOM), let's document that.

Good point, I changed the code to use hasCurrentNavItem( ... ) on the react node instead. It seems to behave exactly the same way as if we did a querySelector. Looking at the other code I can't come up with a reason why a child would have aria-current in the props but not in the rendered DOM, so I am fairly confident in just using hasCurrentNavItem instead like you pointed out (so we have a single source of truth).

When I did this I also chose to remove the if (subNavRef.current) { ... } condition. I don't know if that has any unintentional side-effects? But I can't see why that would be, just thought I should mention it in case you can see anything problematic with that.

Test assertion is too loose

This passes if any element in the markup has aria-expanded="true", not specifically the parent item ("Item 2"). Parse the markup into a container and assert on the specific button

Agreed. I updated the tests. One thing I also did, however; is this to suppress a warning specifically when we create the static markup with my console spy: Warning: useLayoutEffect does nothing on the server, because its effect cannot be encoded into the server renderer's output format. This will lead to a mismatch between the initial, non-hydrated UI and the intended UI.. I imagine this warning is quite annoying when running tests in the future, and it is not relevant to this specific scenario where we just want to test the initial SSR render. But let me know if its an anti-pattern. I could not come up with a better approach.