Skip to content

perf(SelectPanel): memoize overlay props and callbacks #7621

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
hectahertz wants to merge 2 commits into
base: main
Choose a base branch
from
+43 −35
Open

perf(SelectPanel): memoize overlay props and callbacks #7621

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a16d2ec
perf(SelectPanel): memoize overlayProps, focusTrapSettings, and preve…
hectahertz Mar 4, 2026
82bab6d
chore: changeset
hectahertz Mar 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/perf-select-panel-memo-context.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@primer/react': patch
---

Memoize SelectPanel overlayProps, focusTrapSettings, and preventBubbling to reduce allocations on re-renders
73 changes: 38 additions & 35 deletions packages/react/src/SelectPanel/SelectPanel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import {SearchIcon, TriangleDownIcon, XIcon, type IconProps} from '@primer/octicons-react'
import React, {useCallback, useEffect, useMemo, useRef, useState, type KeyboardEventHandler, type JSX} from 'react'
import React, {useCallback, useEffect, useMemo, useRef, useState, type JSX} from 'react'
import type {AnchoredOverlayProps} from '../AnchoredOverlay'
import {AnchoredOverlay} from '../AnchoredOverlay'
import type {AnchoredOverlayWrapperAnchorProps} from '../AnchoredOverlay/AnchoredOverlay'
Expand Down Expand Up @@ -146,6 +146,8 @@ const focusZoneSettings: Partial<FocusZoneHookSettings> = {
disabled: true,
}

const closeButtonProps = {'aria-label': 'Cancel and close'}

const areItemsEqual = (itemA: ItemInput, itemB: ItemInput) => {
// prefer checking equivality by item.id
if (typeof itemA.id !== 'undefined') return itemA.id === itemB.id
Expand Down Expand Up @@ -698,9 +700,7 @@ function Panel({
}
}, [open, resetSort])

const focusTrapSettings = {
initialFocusRef: inputRef || undefined,
}
const focusTrapSettings = useMemo(() => ({initialFocusRef: inputRef || undefined}), [inputRef])

const extendedTextInputProps: Partial<TextInputProps> = useMemo(() => {
return {
Expand Down Expand Up @@ -797,12 +797,11 @@ function Panel({
'anchored',
)

const preventBubbling =
(customOnKeyDown: KeyboardEventHandler<HTMLDivElement> | undefined) =>
const preventBubbling = useCallback(
(event: React.KeyboardEvent<HTMLDivElement>) => {
// skip if a TextInput has focus
customOnKeyDown?.(event)
overlayProps?.onKeyDown?.(event as unknown as React.KeyboardEvent<HTMLDivElement>)

// skip if a TextInput has focus
const activeElement = document.activeElement as HTMLElement
if (activeElement.tagName === 'INPUT' || activeElement.tagName === 'TEXTAREA') return

Expand All @@ -817,7 +816,35 @@ function Panel({

// if this is a typeahead event, don't propagate outside of menu
event.stopPropagation()
}
},
[overlayProps],
)
Comment on lines 800 to +821
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.

preventBubbling closes over the entire overlayProps object and lists it in the dependency array. Since only overlayProps.onKeyDown is used, this will recreate the callback whenever any other overlay prop changes (defeating the memoization goal). Consider extracting const overlayOnKeyDown = overlayProps?.onKeyDown and depending on that instead (and call that inside the handler).

This issue also appears on line 801 of the same file.

Copilot uses AI. Check for mistakes.

const mergedOverlayProps = useMemo(
() => ({
role: 'dialog' as const,
'aria-labelledby': titleId,
'aria-describedby': subtitle ? subtitleId : undefined,
...overlayProps,
...(variant === 'modal'
? {
top: '50vh' as const,
left: '50vw' as const,
anchorSide: undefined,
}
: {}),
style: {
transform: variant === 'modal' ? 'translate(-50%, -50%)' : undefined,
...(isKeyboardVisible
? {
maxHeight: availablePanelHeight !== undefined ? `${availablePanelHeight}px` : 'auto',
}
: {}),
} as React.CSSProperties,
onKeyDown: preventBubbling,
}),
[titleId, subtitle, subtitleId, overlayProps, variant, isKeyboardVisible, availablePanelHeight, preventBubbling],
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.

mergedOverlayProps only uses subtitle for a truthiness check to decide whether to set aria-describedby. Depending on the full subtitle value can cause recomputation when callers pass a new ReactElement each render even though the boolean presence hasn’t changed. Consider using Boolean(subtitle) (or subtitle != null) in the deps instead of subtitle to keep the memo effective.

Suggested change
[titleId, subtitle, subtitleId, overlayProps, variant, isKeyboardVisible, availablePanelHeight, preventBubbling],
[titleId, Boolean(subtitle), subtitleId, overlayProps, variant, isKeyboardVisible, availablePanelHeight, preventBubbling],

Copilot uses AI. Check for mistakes.
)

return (
<>
Expand All @@ -828,31 +855,7 @@ function Panel({
open={open}
onOpen={onOpen}
onClose={onClose}
overlayProps={{
role: 'dialog',
'aria-labelledby': titleId,
'aria-describedby': subtitle ? subtitleId : undefined,
...overlayProps,
...(variant === 'modal'
? {
/* override AnchoredOverlay position */
top: '50vh',
left: '50vw',
anchorSide: undefined,
}
: {}),
style: {
/* override AnchoredOverlay position */
transform: variant === 'modal' ? 'translate(-50%, -50%)' : undefined,
// set maxHeight based on calculated availablePanelHeight when keyboard is visible
...(isKeyboardVisible
? {
maxHeight: availablePanelHeight !== undefined ? `${availablePanelHeight}px` : 'auto',
}
: {}),
} as React.CSSProperties,
onKeyDown: preventBubbling(overlayProps?.onKeyDown),
}}
overlayProps={mergedOverlayProps}
focusTrapSettings={focusTrapSettings}
focusZoneSettings={focusZoneSettings}
height={height}
Expand All @@ -862,7 +865,7 @@ function Panel({
pinPosition={!height}
className={classes.Overlay}
displayCloseButton={showXCloseIcon}
closeButtonProps={{'aria-label': 'Cancel and close'}}
closeButtonProps={closeButtonProps}
>
<div className={classes.Wrapper} data-variant={variant}>
<div className={classes.Header} data-variant={currentResponsiveVariant}>
Expand Down
Loading