perf(ActionList): memoize context values, menuItemProps, aria attributes#7554
perf(ActionList): memoize context values, menuItemProps, aria attributes#7554hectahertz wants to merge 6 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: 79d7a58 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 |
|
👋 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 |
github-actions
bot
added
the
integration-tests: recommended
hectahertz
left a comment
hectahertz
left a comment
There was a problem hiding this comment.
Walkthrough of the conceptual changes in this PR, grouped by optimization type.
|
|
||
| const slotsConfig = {...baseSlots, description: Description} | ||
|
|
||
| // Pre-allocated array for selectableRoles check, avoids per-render allocation |
There was a problem hiding this comment.
Hoist static objects to module scope
baseSlots, slotsConfig, selectableRoles, and listRoleTypes are constant across all renders and all Item instances. Previously they were recreated inside the component body on every render for every item. Moving them to module scope eliminates N object allocations per render (where N = number of items in the list).
| } | ||
|
|
||
| const [partialSlots, childrenWithoutSlots] = useSlots(props.children, {...baseSlots, description: Description}) | ||
| const [partialSlots, childrenWithoutSlots] = useSlots(props.children, slotsConfig) |
There was a problem hiding this comment.
Pass stable slot config reference
Previously: useSlots(props.children, {...baseSlots, description: Description}) created a new object via spread on every render. Now it receives the stable module-level slotsConfig.
| (event: React.KeyboardEvent<HTMLElement>) => { | ||
| if (disabled || inactive || loading) return | ||
| if ([' ', 'Enter'].includes(event.key)) { | ||
| if (event.key === ' ' || event.key === 'Enter') { |
There was a problem hiding this comment.
Avoid array allocation on keypress
Replaced [' ', 'Enter'].includes(event.key) with direct === comparison. The array was allocated on every keypress event handler invocation.
| if (showInactiveIndicator) { | ||
| focusable = true | ||
| } | ||
| const focusable = showInactiveIndicator ? true : undefined |
There was a problem hiding this comment.
Convert mutable let to const ternary
The old let focusable + if block was flagged by the React Compiler as a mutable dependency that couldn't be preserved in memoization. Converting to a const ternary fixes React Compiler compatibility and is more concise.
| role: itemRole, | ||
| id: itemId, | ||
| } | ||
| const hasTrailingVisualSlot = Boolean(slots.trailingVisual) |
There was a problem hiding this comment.
Memoize aria attribute strings
Previously, aria-labelledby was built with template literals that always produced a new string (including trailing spaces from empty slots). aria-describedby used .filter(String).join(' ').trim() which allocates intermediate arrays on every render.
Now both are computed in useMemo with explicit conditionals. Boolean deps (hasTrailingVisualSlot, hasDescriptionSlot) are derived above to keep the dep array stable and avoid depending on slot object references.
|
|
||
| const ariaDescribedBy = React.useMemo(() => { | ||
| const parts: string[] = [] | ||
| if (hasDescriptionSlot && descriptionVariant === 'block') parts.push(blockDescriptionId) |
There was a problem hiding this comment.
Memoize menuItemProps object
This object is created per item and passed to either the <li> (via containerProps) or the <ItemWrapper> (via wrapperProps). Previously rebuilt on every render even when no values changed. Now memoized with all relevant deps.
Note: this has a large dep array (14 deps) because the object touches many item-level values. The memoization still pays off because in typical usage most deps are stable across re-renders (e.g. clickHandler, keyPressHandler, itemRole, itemId rarely change).
| @@ -238,18 +263,21 @@ const UnwrappedItem = <As extends React.ElementType = 'li'>( | |||
| ref: forwardedRef, | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Memoize ItemContext.Provider value
Previously, the inline object passed to ItemContext.Provider was recreated on every render, causing all context consumers (Description, LeadingVisual, TrailingVisual, Selection) to re-render even when their relevant values hadn't changed.
Now wrapped in useMemo with 7 deps. In a 100-item list, this prevents up to ~400 unnecessary child component re-renders per parent state change.
| listRole === 'menu' || container === 'SelectPanel' || container === 'FilteredActionList' ? 'wrap' : undefined, | ||
| }) | ||
|
|
||
| const listContextValue = React.useMemo( |
There was a problem hiding this comment.
Memoize ListContext.Provider value
Same pattern as ItemContext. The inline object was recreated on every ActionList render, causing all ActionList.Item children to re-render due to context referential inequality, even when variant, selectionVariant, showDividers, role, and headingId hadn't changed.
hectahertz
changed the title
hectahertz
force-pushed
the
hectahertz/perf-action-list-memoization
branch
from
844a7bd to
3eb5a4a
Compare
There was a problem hiding this comment.
Pull request overview
This PR optimizes ActionList performance by memoizing context values and computed props to prevent unnecessary re-renders. The changes address a performance issue where every parent re-render created 8 new objects per ActionList.Item, including new context values that prevented React from bailing out of child renders.
Changes:
- Memoized React context provider values (
ListContextandItemContext) usinguseMemoto prevent unnecessary child re-renders - Memoized computed props (
menuItemProps,ariaLabelledBy,ariaDescribedBy) to avoid recreating objects on every render - Hoisted static configuration objects (
baseSlots,slotsConfig,selectableRoles,listRoleTypes) to module scope to avoid per-render allocation - Replaced array allocation in keypress check with direct string comparison for micro-optimization
- Converted mutable
let focusableto immutableconstternary for React Compiler compatibility
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
packages/react/src/ActionList/List.tsx |
Memoized ListContext.Provider value to stabilize context and prevent unnecessary consumer re-renders |
packages/react/src/ActionList/Item.tsx |
Hoisted static configs to module scope; memoized ItemContext.Provider value, menuItemProps, and ARIA attribute computations; optimized keypress handler and focusable variable |
.changeset/perf-action-list-memoization.md |
Added changeset documenting the performance improvements as a patch release |
1 participant
Closes #
Every parent re-render created 8 new objects per ActionList.Item, including new context values that prevented React from bailing out of child renders. This stabilizes context values and computed props.
Changes:
ListContext.ProviderandItemContext.Providervalues withuseMemomenuItemPropsobject (rebuilt per item per render)aria-labelledbyandaria-describedbycomputationbaseSlots,slotsConfig,selectableRoles,listRoleTypesto module scope (were recreated per item per render)lettoconstternary (React Compiler compatibility)Metrics
Object allocation reduction (microbenchmark, Chrome):
Changelog
New
N/A
Changed
Removed
N/A
Rollout strategy
Testing & Reviewing
StressTests/Components/ActionList)Merge checklist