Skip to content

perf(ActionMenu): memoize context values#7619

Open
hectahertz wants to merge 2 commits intomainfrom
hectahertz/perf-action-menu-memo-context
Open

perf(ActionMenu): memoize context values#7619
hectahertz wants to merge 2 commits intomainfrom
hectahertz/perf-action-menu-memo-context

Conversation

@hectahertz
Copy link
Contributor

Closes #

Both MenuContext.Provider and ActionListContainerContext.Provider in ActionMenu were creating new object references on every render. This caused all context consumers (menu items, anchors) to re-render even when no values actually changed.

Wrapped both context values in useMemo and extracted the inline afterSelect callback into useCallback.

Changelog

New

N/A

Changed

  • Memoized MenuContext value in Menu component
  • Memoized ActionListContainerContext value in Overlay component
  • Extracted afterSelect into a stable useCallback

Removed

N/A

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

All 39 existing ActionMenu tests pass. This is a pure memoization change with no behavioral difference.

Merge checklist

Copilot AI review requested due to automatic review settings March 4, 2026 11:00
@hectahertz hectahertz requested a review from a team as a code owner March 4, 2026 11:00
@hectahertz hectahertz requested a review from siddharthkp March 4, 2026 11:00
@changeset-bot
Copy link

changeset-bot bot commented Mar 4, 2026

🦋 Changeset detected

Latest commit: af32e81

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 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 Mar 4, 2026
@github-actions
Copy link
Contributor

github-actions bot commented Mar 4, 2026

⚠️ 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. Or, apply the integration-tests: skipped manually label to skip these checks.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Improves @primer/react’s ActionMenu render performance by attempting to stabilize context provider values so context consumers don’t re-render due solely to new object/function identities.

Changes:

  • Memoized MenuContext.Provider value in Menu.
  • Memoized ActionListContainerContext.Provider value in Overlay.
  • Extracted afterSelect into a stable useCallback.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
packages/react/src/ActionMenu/ActionMenu.tsx Memoizes context values and stabilizes afterSelect to reduce unnecessary re-renders.
.changeset/perf-action-menu-memo-context.md Adds a patch changeset entry describing the perf improvement.

Comment on lines +169 to +179
const menuContextValue = useMemo(
() => ({
anchorRef,
renderAnchor,
anchorId,
open: combinedOpenState,
onOpen,
onClose,
isSubmenu,
}),
[anchorRef, renderAnchor, anchorId, combinedOpenState, onOpen, onClose, isSubmenu],
Copy link

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

menuContextValue will still change on every render because renderAnchor is reassigned to a newly-created inline function during the React.Children.map(...) pass. Since renderAnchor is in the useMemo deps, this memoization won’t prevent re-renders of MenuContext consumers in the common case. Consider memoizing renderAnchor (and/or the contents computation that sets it) so the function identity is stable when children/anchorRef haven’t changed, otherwise the useMemo here provides little/no benefit.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants