Uh oh!
There was an error while loading. Please reload this page.
perf(SelectPanel): memoize overlay props and callbacks - #7621
Conversation
🦋 Changeset detectedLatest commit: d66524d 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 |
|
There was a problem hiding this comment.
Pull request overview
This PR refactors SelectPanel to reduce unnecessary allocations by memoizing several props/callbacks passed into AnchoredOverlay, helping stabilize references across renders.
Changes:
- Memoizes
focusTrapSettingsand consolidatedoverlayPropsinto amergedOverlayPropsuseMemo. - Refactors
preventBubblingfrom a curried function to a directuseCallback. - Hoists
closeButtonPropsto a module-level constant and adds a patch changeset.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/react/src/SelectPanel/SelectPanel.tsx | Memoizes overlay-related props/callbacks to reduce re-renders/allocations when passing props to AnchoredOverlay. |
| .changeset/perf-select-panel-memo-context.md | Adds a patch changeset describing the perf-focused memoization refactor. |
Comments suppressed due to low confidence (1)
packages/react/src/SelectPanel/SelectPanel.tsx:803
- The double cast in
overlayProps?.onKeyDown?.(event as unknown as React.KeyboardEvent<HTMLDivElement>)looks unnecessary becauseeventis already aReact.KeyboardEvent<HTMLDivElement>. If there’s a typing mismatch, it would be clearer to type/narrowoverlayProps.onKeyDown(or the extractedoverlayOnKeyDown) rather than casting the event tounknown.
(event: React.KeyboardEvent<HTMLDivElement>) => {
overlayProps?.onKeyDown?.(event as unknown as React.KeyboardEvent<HTMLDivElement>)
| @@ -817,7 +816,35 @@ function Panel({ | |||
| // if this is a typeahead event, don't propagate outside of menu | |||
| event.stopPropagation() | |||
| } | |||
| }, | |||
| [overlayProps], | |||
| ) | |||
There was a problem hiding this comment.
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.
| } as React.CSSProperties, | ||
| onKeyDown: preventBubbling, | ||
| }), | ||
| [titleId, subtitle, subtitleId, overlayProps, variant, isKeyboardVisible, availablePanelHeight, preventBubbling], |
There was a problem hiding this comment.
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.
| [titleId,subtitle,subtitleId,overlayProps,variant,isKeyboardVisible,availablePanelHeight,preventBubbling], | |
| [titleId,Boolean(subtitle),subtitleId,overlayProps,variant,isKeyboardVisible,availablePanelHeight,preventBubbling], |
jonrohan
left a comment
There was a problem hiding this comment.
Seems good, would be worth checking out integration tests also
82bab6d to
bd74d49Comparebd74d49 to
43c0462CompareUh oh!
There was an error while loading. Please reload this page.
👋 Hi from github/github-ui! Your integration PR is ready: https://github.com/github/github-ui/pull/15932 |
Closes #
SelectPanel was creating several new object/function references on every render that got passed as props to
AnchoredOverlay:overlayProps: complex nested object with style, role, aria attributes, created inline every renderfocusTrapSettings: new object every renderpreventBubbling: curried function creating a new closure every rendercloseButtonProps: inline object literal every renderMemoized all of these. Also refactored
preventBubblingfrom a curried function to a directuseCallback, passing the overlay'sonKeyDownthrough the dependency array instead of the closure.Changelog
New
N/A
Changed
overlayProps,focusTrapSettings, andpreventBubblingto stabilize references across renderscloseButtonPropsto a module-level constantRemoved
N/A
Rollout strategy
Testing & Reviewing
Pure memoization refactor, no behavioral changes. The
preventBubblingfunction was refactored from curried to direct callback, but the logic is identical.Merge checklist