perf(compact-select): render options lazily #103811

TkDodo · 2025-11-21T13:51:04Z

This PR generally aims to improve performance of the compactSelect component by not rendering all options eagerly. This is achieved with a simple conditional rendering, however, some pre-requisites are required to get that going.

Why did we render all those options even though the select was closed?

Good question! We did that because the compactSelect component internally holds a <List>, which managed its state internally. That state is (was) bubbled up to the parent (with some effects, inverting the parent-child data flow into child-parent), so that the parent could create the label of the select trigger. For that, it needed to render all children, otherwise the bubble-up didn’t work.

This just shows why child->parent dataflows are bad design. We need to render e.g. 1000 options just to get to the correct label of a button. It’s not react. Furthermore, the whole internal state of <List> is unnecessary, because we use the component in a fully controlled way exclusively. The parent has the state already! Another reminder that state duplication & syncing is the root of all evil.

So now, we’re using the parent state to calculate that and remove the internal state. That means we can also conditionally render options only when the select is opened. Getting there was painful:

First, we needed to make sure that we disallow uncontrolled usage of this component:

ref: do not allow uncontrolled compactSelect #103637
And make it required that we pass value and onChange:
ref(scraps): make value and onChange required on compactSelect #103654

Then, there’s CompositeSelect, which shares some internals with CompactSelect, however for those, the parent doesn’t pass the full state to CompositeSelect itself because it’s built up of Regions. The good news is that we never use the default trigger label, so we don’t need to calculate it based on the current selection. To make sure we don’t do this in the future, CompositeSelect now has a required trigger:

ref: compositeSelect gets a required trigger #103713

Lastly, there’s clearable. The Clear button appears outside of List, so it doesn’t have all the information necessary to perform a clear if we remove the reverse data flow. I changed how clear works by just triggering an onChange with an empty value. Also, types were a mess and that’s now fixed:

ref(compactSelect): improve clearing values #103720

With that, I could finally remove the internal useLayoutEffect that synced state from the List child with the Control parent.

Test Changes

Then, many tests failed. Note that I checked all failing tests in the product before I changed them, and they all work fine. A couple of thoughts on why the tests failed:

They sometimes relied on the internal state management and mocked onChange. Now, without onChange, there is no state change happening. I’ve changed some tests to do some basic useState around the component (like we do in the product, too).
They sometimes had tests that relied upon all option popups being rendered, so they were accessing the Nth search field. Now they are no longer in the DOM because of conditional rendering so all of those needed to update.

…losed

we no longer do trigger label updating

where auto focussing still works

as all selects need to be fully controlled

as it's no longer needed

codecov · 2025-11-21T13:56:05Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #103811      +/-   ##
===========================================
- Coverage   80.61%    80.61%   -0.01%     
===========================================
  Files        9284      9284              
  Lines      396288    396266      -22     
  Branches    25261     25261              
===========================================
- Hits       319463    319442      -21     
+ Misses      76365     76364       -1     
  Partials      460       460

TkDodo · 2025-11-21T17:47:56Z

static/app/views/insights/database/components/databaseSystemSelector.spec.tsx

    expect(dropdownOptionLabels[1]).toHaveTextContent('MongoDB');

    await userEvent.click(dropdownOptionLabels[0]!);
-    expect(dropdownSelector).toHaveTextContent('SystemPostgreSQL');


note: for this test, the useLocalStorage hook is mocked so we don’t really have state management that would make sure the text updates. We previously relied on the value updating automatically from within. Now, we need state.

TkDodo · 2025-11-21T17:51:03Z

static/app/views/dashboards/manage/dashboardTable.spec.tsx

-    await userEvent.click((await screen.findAllByTestId('edit-access-dropdown'))[0]!);
+    await userEvent.click(screen.getByText('All'));
    expect(screen.getAllByPlaceholderText('Search Teams')[0]).toBeInTheDocument();


this test wasn’t really opening anything, but because all the search fields were rendered invisibly, the toBeInTheDocument succeeded anyway 😅

TkDodo · 2025-11-21T17:53:36Z

static/app/views/explore/tables/columnEditorModal.spec.tsx

-    await userEvent.click(screen.getByRole('button', {name: 'Column project string'}));
+
+    const projectColumn = screen.getAllByTestId('editor-column')[1]!;
+
+    await userEvent.click(
+      within(projectColumn).getByRole('button', {name: 'Column project string'})
+    );


tbh, I’m not really sure why I had to change all these tests. I got a message that buttons with that name exist more than once, and from what I could tell, the whole row is a button with the same accessibility label. But I’m not sure what my PR changed to surface this or how this could’ve worked on master.

My best guess is that it worked coincidentally, I’m now just narrowing the scope of the selector to make the tests pass again. It’s hard to investigate if you don’t get to physically “see” what gets rendered though...

Did logging the buttons give any clues?

TkDodo · 2025-11-21T17:54:40Z

static/app/components/core/compactSelect/control.tsx

-   * A list of selected options across all select regions, to be used to generate the
-   * trigger label.
-   */
-  const [selectedOptions, setSelectedOptions] = useState<
-    Array<SelectOption<SelectKey> | Array<SelectOption<SelectKey>>>
-  >([]);
-  const saveSelectedOptions = useCallback<SelectContextValue['saveSelectedOptions']>(
-    (index, newSelectedOptions) => {
-      setSelectedOptions(current => [
-        ...current.slice(0, index),
-        newSelectedOptions,
-        ...current.slice(index + 1),
-      ]);
-    },
-    []
-  );


no more internal state that needs to be synced just to calculate a trigger label

TkDodo · 2025-11-21T17:55:25Z

static/app/components/core/compactSelect/control.tsx

-              )}
-            </FocusScope>
-          </StyledOverlay>
+          {overlayIsOpen && (


this is where the perf improvement comes from 😅

TkDodo · 2025-11-21T17:55:49Z

static/app/components/core/compactSelect/list.tsx

-  // Register the initialized list state once on mount
-  useLayoutEffect(() => {
-    saveSelectedOptions(
-      compositeIndex,
-      getSelectedOptions(items, listState.selectionManager.selectedKeys)
-    );
-    // eslint-disable-next-line react-hooks/exhaustive-deps
-  }, [listState.collection]);
-


static/app/components/core/compactSelect/control.tsx

JonasBa

Beautiful work 🙏 I only left a few minor comments that are non blocking

JonasBa · 2025-11-21T18:27:16Z

static/app/components/core/compactSelect/control.tsx


 export interface ControlProps
  extends Omit<
      React.BaseHTMLAttributes<HTMLDivElement>,


I have never heard of basehtmlattributes

JonasBa · 2025-11-21T18:28:52Z

static/app/components/core/compactSelect/compactSelect.tsx

            return (
              <Section key={item.key} title={item.label}>
-                {item.options.map(opt => (
+                {item.options?.map(opt => (


I would expect the check above to narrow it down to Array, is this an expected change?

I think that's expected as items support an explicit undefined now?

JonasBa · 2025-11-21T18:30:17Z

static/app/components/core/compactSelect/control.tsx

  ...wrapperProps
-}: ControlProps) {
+}: ControlProps & {
+  items?: Array<SelectOptionOrSection<SelectKey>>;


Was items always optional?

JonasBa · 2025-11-21T18:35:25Z

static/app/components/core/compactSelect/control.tsx

  menuBody,
  menuFooter,
  onOpenChange,
+  items = [],


This is not a big deal, but when values are not provided, these default initializers will cause react components to inefficiently rerender as each rerender will initialize to a fresh reference.

I think it's a bit of an anti pattern in react and should be avoided if possible (not necessarily for performance, but maybe just as a guide to try and keep the APIs more explicit)

JonasBa · 2025-11-21T18:36:18Z

static/app/components/core/compactSelect/control.tsx

-  /**
-   * Clears selection values
-   */
-  const clearSelection = () => {


JonasBa · 2025-11-21T18:38:11Z

static/app/views/explore/tables/columnEditorModal.spec.tsx

-    await userEvent.click(screen.getByRole('button', {name: 'Column project string'}));
+
+    const projectColumn = screen.getAllByTestId('editor-column')[1]!;
+
+    await userEvent.click(
+      within(projectColumn).getByRole('button', {name: 'Column project string'})
+    );


Did logging the buttons give any clues?

natemoo-re · 2025-11-21T18:54:40Z

static/app/components/core/compactSelect/compactSelect.tsx

            return (
              <Section key={item.key} title={item.label}>
-                {item.options.map(opt => (
+                {item.options?.map(opt => (


I think that's expected as items support an explicit undefined now?

natemoo-re · 2025-11-21T19:08:12Z

static/app/components/core/compactSelect/control.tsx

+    const selectedOptions = Array.isArray(value)
+      ? value
+      : value === undefined
+        ? []
+        : [value];
+    return selectedOptions.flat().length > 0;


Not sure why we need the array.flat() check here?

Suggested change

const selectedOptions = Array.isArray(value)

? value

: value === undefined

? []

: [value];

return selectedOptions.flat().length > 0;

if (value === undefined) return false;

if (Array.isArray(value)) return value.length > 0;

return true;

gggritso · 2025-11-21T21:08:42Z

Just wanted to chime in with some gratitude on this one, we have a lot of slow CompactSelect components in the Widget Builder, and this improvement saves on the order of 200-250ms when rendering the Widget Builder contents for complicated widgets 🙌🏻

TkDodo added 16 commits November 19, 2025 14:24

perf: do not render <List> inside CompactSelect when the overlay is c…

ecdc9b1

…losed

Merge branch 'master' into tkdodo/perf/compact-select

f6f4975

ref: defaultValue is no more

562b528

ref: make items and value optional for control

984999b

ref: move conditional rendering further down

65eca0f

Merge branch 'master' into tkdodo/perf/compact-select

215698d

ref: compositeSelect gets a required trigger

a3b60f7

ref: fix tests

8055756

we no longer do trigger label updating

Merge branch 'master' into tkdodo/ref/composite-select-required-trigger

13f258e

merge branch ref/composite-select-required-trigger

0860554

fix: move conditional rendering to the highest possible point

5c5c864

where auto focussing still works

fix: we need proper state management to make selection work now

31c671e

as all selects need to be fully controlled

merge branch master

91179cc

ref: remove compositeIndex

988d997

as it's no longer needed

fix: inline onClear and remove call to non-existing setSelectedOptions

b18d6e0

fix: hasSelection logic

93f5e34

github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Nov 21, 2025

vercel bot deployed to Preview November 21, 2025 13:52 View deployment

TkDodo added 2 commits November 21, 2025 16:48

test: adapt tests

5518059

Merge branch 'master' into tkdodo/perf/compact-select

c32c0b0

vercel bot deployed to Preview November 21, 2025 15:59 View deployment

TkDodo added 2 commits November 21, 2025 18:31

test: more test fixes

dc6b4d7

Merge branch 'master' into tkdodo/perf/compact-select

f83ffd5

vercel bot deployed to Preview November 21, 2025 17:52 View deployment

TkDodo commented Nov 21, 2025

View reviewed changes

TkDodo marked this pull request as ready for review November 21, 2025 18:02

TkDodo requested review from a team as code owners November 21, 2025 18:02

TkDodo requested a review from a team as a code owner November 21, 2025 18:02

cursor bot reviewed Nov 21, 2025

View reviewed changes

static/app/components/core/compactSelect/control.tsx Show resolved Hide resolved

TkDodo added 2 commits November 21, 2025 19:08

options?.flat

4b1bf62

Merge branch 'master' into tkdodo/perf/compact-select

5317c59

vercel bot deployed to Preview November 21, 2025 18:11 View deployment

JonasBa approved these changes Nov 21, 2025

View reviewed changes

natemoo-re approved these changes Nov 21, 2025

View reviewed changes

Uh oh!

perf(compact-select): render options lazily #103811

Are you sure you want to change the base?

perf(compact-select): render options lazily #103811

Conversation

TkDodo commented Nov 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Changes

Uh oh!

codecov bot commented Nov 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JonasBa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gggritso commented Nov 21, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

TkDodo commented Nov 21, 2025 •

edited

Loading

codecov bot commented Nov 21, 2025 •

edited

Loading