From 0e19575a7a47d60024deff146c922843b23ba5c0 Mon Sep 17 00:00:00 2001 From: Hector Garcia Date: Sun, 15 Feb 2026 10:22:31 +0000 Subject: [PATCH 1/5] perf(ActionList): memoize context values, menuItemProps, aria attributes --- packages/react/src/ActionList/Item.tsx | 139 ++++++++++++++++--------- packages/react/src/ActionList/List.tsx | 21 ++-- 2 files changed, 101 insertions(+), 59 deletions(-) diff --git a/packages/react/src/ActionList/Item.tsx b/packages/react/src/ActionList/Item.tsx index 97fbe722e2b..e6625ca0cdf 100644 --- a/packages/react/src/ActionList/Item.tsx +++ b/packages/react/src/ActionList/Item.tsx @@ -70,6 +70,19 @@ const DivItemContainerNoBox = React.forwardRef( { variant = 'default', @@ -92,14 +105,7 @@ const UnwrappedItem = ( // eslint-disable-next-line @typescript-eslint/no-explicit-any forwardedRef: React.Ref, ): JSX.Element => { - const baseSlots = { - leadingVisual: LeadingVisual, - trailingVisual: TrailingVisual, - trailingAction: TrailingAction, - subItem: SubItem, - } - - const [partialSlots, childrenWithoutSlots] = useSlots(props.children, {...baseSlots, description: Description}) + const [partialSlots, childrenWithoutSlots] = useSlots(props.children, slotsConfig) const slots = {description: undefined, ...partialSlots} @@ -173,7 +179,6 @@ const UnwrappedItem = ( itemRole === 'menuitemcheckbox' || itemRole === 'tab' - const listRoleTypes = ['listbox', 'menu', 'list'] const listSemantics = (listRole && listRoleTypes.includes(listRole)) || inactive || listItemSemantics const buttonSemantics = !listSemantics && !_PrivateItemWrapper @@ -188,7 +193,7 @@ const UnwrappedItem = ( const keyPressHandler = React.useCallback( (event: React.KeyboardEvent) => { if (disabled || inactive || loading) return - if ([' ', 'Enter'].includes(event.key)) { + if (event.key === ' ' || event.key === 'Enter') { if (event.key === ' ') { event.preventDefault() // prevent scrolling on Space // immediately reset defaultPrevented once its job is done @@ -214,40 +219,61 @@ const UnwrappedItem = ( const ItemWrapper = _PrivateItemWrapper || DefaultItemWrapper - // only apply aria-selected and aria-checked to selectable items - const selectableRoles = ['menuitemradio', 'menuitemcheckbox', 'option'] const includeSelectionAttribute = itemSelectionAttribute && itemRole && selectableRoles.includes(itemRole) - let focusable - - if (showInactiveIndicator) { - focusable = true - } + const focusable = showInactiveIndicator ? true : undefined // Extract the variant prop value from the description slot component const descriptionVariant = slots.description?.props.variant ?? 'inline' - const menuItemProps = { - onClick: clickHandler, - onKeyPress: !buttonSemantics ? keyPressHandler : undefined, - 'aria-disabled': disabled ? true : undefined, - 'data-inactive': inactive ? true : undefined, - 'data-loading': loading && !inactive ? true : undefined, - tabIndex: focusable ? undefined : 0, - 'aria-labelledby': `${labelId} ${slots.trailingVisual ? trailingVisualId : ''}`, - 'aria-describedby': - [ - slots.description && descriptionVariant === 'block' ? blockDescriptionId : undefined, - slots.description && descriptionVariant === 'inline' ? inlineDescriptionId : undefined, - inactiveWarningId ?? undefined, - ] - .filter(String) - .join(' ') - .trim() || undefined, - ...(includeSelectionAttribute && {[itemSelectionAttribute]: selected}), - role: itemRole, - id: itemId, - } + const hasTrailingVisualSlot = Boolean(slots.trailingVisual) + const hasDescriptionSlot = Boolean(slots.description) + + const ariaLabelledBy = React.useMemo(() => { + const parts = [labelId] + if (hasTrailingVisualSlot) parts.push(trailingVisualId) + if (hasDescriptionSlot && descriptionVariant === 'inline') parts.push(inlineDescriptionId) + return parts.join(' ') + }, [labelId, hasTrailingVisualSlot, trailingVisualId, hasDescriptionSlot, descriptionVariant, inlineDescriptionId]) + + const ariaDescribedBy = React.useMemo(() => { + const parts: string[] = [] + if (hasDescriptionSlot && descriptionVariant === 'block') parts.push(blockDescriptionId) + if (inactiveWarningId) parts.push(inactiveWarningId) + return parts.length > 0 ? parts.join(' ') : undefined + }, [hasDescriptionSlot, descriptionVariant, blockDescriptionId, inactiveWarningId]) + + const menuItemProps = React.useMemo( + () => ({ + onClick: clickHandler, + onKeyPress: !buttonSemantics ? keyPressHandler : undefined, + 'aria-disabled': disabled ? true : undefined, + 'data-inactive': inactive ? true : undefined, + 'data-loading': loading && !inactive ? true : undefined, + tabIndex: focusable ? undefined : 0, + 'aria-labelledby': ariaLabelledBy, + 'aria-describedby': ariaDescribedBy, + ...(includeSelectionAttribute && {[itemSelectionAttribute]: selected}), + role: itemRole, + id: itemId, + }), + [ + clickHandler, + buttonSemantics, + keyPressHandler, + disabled, + inactive, + loading, + focusable, + ariaLabelledBy, + ariaDescribedBy, + includeSelectionAttribute, + itemSelectionAttribute, + selected, + itemRole, + itemId, + ], + ) const containerProps = _PrivateItemWrapper ? {role: itemRole ? 'none' : undefined, ...props} @@ -262,19 +288,32 @@ const UnwrappedItem = ( ref: forwardedRef, } + const itemContextValue = React.useMemo( + () => ({ + variant, + size, + disabled, + inactive: Boolean(inactiveText), + inlineDescriptionId, + blockDescriptionId, + trailingVisualId, + setTruncatedText: buttonSemantics ? setTruncatedText : undefined, + }), + [ + variant, + size, + disabled, + inactiveText, + inlineDescriptionId, + blockDescriptionId, + trailingVisualId, + buttonSemantics, + setTruncatedText, + ], + ) + return ( - +
  • ( listRole === 'menu' || container === 'SelectPanel' || container === 'FilteredActionList' ? 'wrap' : undefined, }) + const listContextValue = React.useMemo( + () => ({ + variant, + selectionVariant: selectionVariant || containerSelectionVariant, + showDividers, + role: listRole, + headingId, + }), + [variant, selectionVariant, containerSelectionVariant, showDividers, listRole, headingId], + ) + return ( - + {slots.heading} {/* @ts-expect-error ref needs a non nullable ref */} Date: Sun, 15 Feb 2026 10:24:12 +0000 Subject: [PATCH 2/5] changeset --- .changeset/perf-action-list-memoization.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/perf-action-list-memoization.md diff --git a/.changeset/perf-action-list-memoization.md b/.changeset/perf-action-list-memoization.md new file mode 100644 index 00000000000..bb97d9d8ce1 --- /dev/null +++ b/.changeset/perf-action-list-memoization.md @@ -0,0 +1,5 @@ +--- +'@primer/react': patch +--- + +perf(ActionList): memoize context values, menuItemProps, and aria attributes From db59430bbc7cccae612f4b0bf52396d94edfccbf Mon Sep 17 00:00:00 2001 From: Hector Garcia Date: Sun, 15 Feb 2026 11:24:24 +0000 Subject: [PATCH 3/5] ci: retry flaky test From 57c948caa8ea44292b88a12f02a74e691fe03227 Mon Sep 17 00:00:00 2001 From: Hector Garcia Date: Sun, 15 Feb 2026 11:36:16 +0000 Subject: [PATCH 4/5] test(ActionList): add parent re-render stress test --- .../ActionList.stress.dev.stories.tsx | 139 ++++++++++++++++++ 1 file changed, 139 insertions(+) diff --git a/packages/react/src/ActionList/ActionList.stress.dev.stories.tsx b/packages/react/src/ActionList/ActionList.stress.dev.stories.tsx index 352ce000368..c79d86f0110 100644 --- a/packages/react/src/ActionList/ActionList.stress.dev.stories.tsx +++ b/packages/react/src/ActionList/ActionList.stress.dev.stories.tsx @@ -3,6 +3,8 @@ import type {ComponentProps} from '../utils/types' import {StressTest} from '../utils/StressTest' import {TableIcon} from '@primer/octicons-react' import {ActionList} from '.' +import React, {useState, useEffect, useRef, useCallback, memo} from 'react' +import afterFrame from 'afterframe' export default { title: 'StressTests/Components/ActionList', @@ -48,3 +50,140 @@ export const SingleSelect = () => { /> ) } + +export const ParentRerender = () => { + return +} + +/** + * This benchmark isolates the effect of context value memoization. + * + * Setup: A parent component holds a counter that increments 100 times. + * The ActionList is rendered via a memoized child component (MemoizedList), + * so React only re-renders it if props or context change. + * + * - Without useMemo on ListContext/ItemContext: the context values are new + * objects every render, so React re-renders all context consumers (every + * Description, LeadingVisual, TrailingVisual, Selection in every Item). + * - With useMemo: context values are referentially stable, React bails out + * of re-rendering the consumers entirely. + */ + +const listItems = Array.from({length: 100}, (_, i) => ({ + name: `Project ${i + 1}`, + scope: `Scope ${i + 1}`, +})) + +// Memoized list component: only re-renders when props change or context forces it +const MemoizedList = memo(function MemoizedList() { + return ( + + {listItems.map((project, index) => ( + + + + + {project.name} + {project.scope} + + ))} + + ) +}) + +function ContextMemoizationBenchmark() { + const totalIterations = 100 + const [count, setCount] = useState(0) + const [running, setRunning] = useState(false) + const [results, setResults] = useState<{median: number; average: number; min: number; max: number} | null>(null) + const observerRef = useRef<{observer: PerformanceObserver; data: number[]} | null>(null) + + useEffect(() => { + const duration: number[] = [] + const obs = new PerformanceObserver(list => { + for (const entry of list.getEntries()) { + if (entry.entryType === 'measure' && entry.name === 'ctx-rerender') { + duration.push(entry.duration) + } + } + }) + obs.observe({entryTypes: ['measure']}) + observerRef.current = {data: duration, observer: obs} + return () => obs.disconnect() + }, []) + + const start = useCallback(() => { + if (observerRef.current) observerRef.current.data.length = 0 + setResults(null) + setRunning(true) + setCount(0) + + let i = 0 + const interval = setInterval(() => { + if (i < totalIterations - 1) { + performance.mark('ctx-start') + setCount(c => c + 1) + afterFrame(() => { + performance.mark('ctx-end') + performance.measure('ctx-rerender', 'ctx-start', 'ctx-end') + }) + i++ + } else { + clearInterval(interval) + setTimeout(() => { + const durations = observerRef.current?.data ?? [] + const sorted = [...durations].sort((a, b) => a - b) + setResults({ + median: sorted[Math.floor(sorted.length / 2)] ?? 0, + average: durations.reduce((a, b) => a + b, 0) / durations.length, + min: Math.min(...durations), + max: Math.max(...durations), + }) + setRunning(false) + }, 100) + } + }, 10) + }, []) + + return ( +
    +
    +
    + ActionList Context Memoization +

    + Parent re-renders 100x with a counter. ActionList is memoized, so only context changes trigger item + re-renders. Tests whether ListContext/ItemContext values are stable. +

    +
    + +
    + + {/* The counter forces this component to re-render, but MemoizedList should bail out if context is stable */} +
    Counter: {count}
    + + + {results && ( +
    + Median: {results.median.toFixed(2)}ms + {' | '}Average: {results.average.toFixed(2)}ms{' | '} + Min: {results.min.toFixed(2)}ms{' | '} + Max: {results.max.toFixed(2)}ms +
    + )} +
    + ) +} From 39a9219ca8503c4a8b136a2023bb364be0db7d83 Mon Sep 17 00:00:00 2001 From: Hector Garcia Date: Sun, 15 Feb 2026 11:44:41 +0000 Subject: [PATCH 5/5] revert: remove ParentRerender stress test --- .../ActionList.stress.dev.stories.tsx | 139 ------------------ 1 file changed, 139 deletions(-) diff --git a/packages/react/src/ActionList/ActionList.stress.dev.stories.tsx b/packages/react/src/ActionList/ActionList.stress.dev.stories.tsx index c79d86f0110..352ce000368 100644 --- a/packages/react/src/ActionList/ActionList.stress.dev.stories.tsx +++ b/packages/react/src/ActionList/ActionList.stress.dev.stories.tsx @@ -3,8 +3,6 @@ import type {ComponentProps} from '../utils/types' import {StressTest} from '../utils/StressTest' import {TableIcon} from '@primer/octicons-react' import {ActionList} from '.' -import React, {useState, useEffect, useRef, useCallback, memo} from 'react' -import afterFrame from 'afterframe' export default { title: 'StressTests/Components/ActionList', @@ -50,140 +48,3 @@ export const SingleSelect = () => { /> ) } - -export const ParentRerender = () => { - return -} - -/** - * This benchmark isolates the effect of context value memoization. - * - * Setup: A parent component holds a counter that increments 100 times. - * The ActionList is rendered via a memoized child component (MemoizedList), - * so React only re-renders it if props or context change. - * - * - Without useMemo on ListContext/ItemContext: the context values are new - * objects every render, so React re-renders all context consumers (every - * Description, LeadingVisual, TrailingVisual, Selection in every Item). - * - With useMemo: context values are referentially stable, React bails out - * of re-rendering the consumers entirely. - */ - -const listItems = Array.from({length: 100}, (_, i) => ({ - name: `Project ${i + 1}`, - scope: `Scope ${i + 1}`, -})) - -// Memoized list component: only re-renders when props change or context forces it -const MemoizedList = memo(function MemoizedList() { - return ( - - {listItems.map((project, index) => ( - - - - - {project.name} - {project.scope} - - ))} - - ) -}) - -function ContextMemoizationBenchmark() { - const totalIterations = 100 - const [count, setCount] = useState(0) - const [running, setRunning] = useState(false) - const [results, setResults] = useState<{median: number; average: number; min: number; max: number} | null>(null) - const observerRef = useRef<{observer: PerformanceObserver; data: number[]} | null>(null) - - useEffect(() => { - const duration: number[] = [] - const obs = new PerformanceObserver(list => { - for (const entry of list.getEntries()) { - if (entry.entryType === 'measure' && entry.name === 'ctx-rerender') { - duration.push(entry.duration) - } - } - }) - obs.observe({entryTypes: ['measure']}) - observerRef.current = {data: duration, observer: obs} - return () => obs.disconnect() - }, []) - - const start = useCallback(() => { - if (observerRef.current) observerRef.current.data.length = 0 - setResults(null) - setRunning(true) - setCount(0) - - let i = 0 - const interval = setInterval(() => { - if (i < totalIterations - 1) { - performance.mark('ctx-start') - setCount(c => c + 1) - afterFrame(() => { - performance.mark('ctx-end') - performance.measure('ctx-rerender', 'ctx-start', 'ctx-end') - }) - i++ - } else { - clearInterval(interval) - setTimeout(() => { - const durations = observerRef.current?.data ?? [] - const sorted = [...durations].sort((a, b) => a - b) - setResults({ - median: sorted[Math.floor(sorted.length / 2)] ?? 0, - average: durations.reduce((a, b) => a + b, 0) / durations.length, - min: Math.min(...durations), - max: Math.max(...durations), - }) - setRunning(false) - }, 100) - } - }, 10) - }, []) - - return ( -
    -
    -
    - ActionList Context Memoization -

    - Parent re-renders 100x with a counter. ActionList is memoized, so only context changes trigger item - re-renders. Tests whether ListContext/ItemContext values are stable. -

    -
    - -
    - - {/* The counter forces this component to re-render, but MemoizedList should bail out if context is stable */} -
    Counter: {count}
    - - - {results && ( -
    - Median: {results.median.toFixed(2)}ms - {' | '}Average: {results.average.toFixed(2)}ms{' | '} - Min: {results.min.toFixed(2)}ms{' | '} - Max: {results.max.toFixed(2)}ms -
    - )} -
    - ) -}