ref(scraps): compactSelect Triggers must be a specific SelectTrigger by TkDodo · Pull Request #105040 · getsentry/sentry

TkDodo · 2025-12-16T14:32:06Z

ref: https://linear.app/getsentry/issue/DE-668/streamline-compactselect-triggers

This PR disallows passing arbitrary components to trigger of compactSelect on type level. Everything that isn’t SelectTrigger.Button (or future SelectTriggers that we can add here) will produce a type error. This has a couple of advantages:

SelectTrigger.Button is just a regular DropdownButton, however, we can ensure that we read from the SelectContext and therefore have access to the size, disabled and open state of the compactSelect. This ensures consistency, as we can’t forget to pass those along. Forgetting to pass isOpen leads to the chevron not being inverted correctly. Passing size explicitly is brittle. All these things work out of the box when using triggerProps, but not when passing a custom trigger. Note: The final goal here is to get rid of triggerProps and only use the trigger prop.
If we add more allowed triggers (like an Input), it’s hard to get the typings right, mostly because of refs. It’s actually impossible without a type assertions (we’d need fragment refs, which are not out yet). This is now hidden inside SelectTrigger.Button

The implications are:

In places where we used a Button instead of a DropdownButton, we now get the chevron shown unless we pass showChevron={false}. I did that for all the places I touched, it’s especially important for buttons that only have an icon where a second chevron icon would be out of place.
In places where we didn’t use a Button as a trigger at all, we would now get an error. However, we were already tied to Button because of the ref typings, so there was only one place, which already has a ts-expect-error: The BreadcrumbDropdown:

sentry/static/app/views/settings/components/settingsBreadcrumb/breadcrumbDropdown.tsx

Lines 59 to 67 in 98b67ef

    
           trigger={triggerProps => ( 
        
             <MenuCrumb 
        
               crumbLabel={name || route.name} 
        
               menuHasHover={isHovered} 
        
               {...triggerProps} 
        
               // @ts-expect-error - TODO: Crumb component should be refactored to use a button element instead of a div 
        
               ref={triggerProps.ref} 
        
             /> 
        
           )}

I refactored this towards a button in:

ref(ui): make settings breadcrumb trigger a button #105031

We should merge that one first, and then I’ll fix the conflicts here.

There is one additional place I couldn’t fix because it uses a completely unstyled button, and t hat’s inside the searchQueryBuilder. I don’t know why there is only one element that uses compactSelect, as everything else seems to just use ListBox directly. I tried to replicate the current look with a transparent button but I couldn’t quite get it to look identical, so I added a ts-expect-error for that place.

Naming is up for debate: I didn’t want to attach it to CompactSelect.TriggerButton because we also use it for CompositeSelect.

TkDodo · 2025-12-16T14:32:54Z

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

+    );
+  },
+
+  Input(props: InputTriggerProps) {


this is mostly just a showcase, I can also delete it for now.

I deleted this for now and made the error message better by adding an invalid type to the union: 9234a5b

TkDodo · 2025-12-16T14:33:18Z

static/app/components/core/tabs/tabList.tsx

        trigger={triggerProps => (
          <OverflowMenuTrigger
            {...triggerProps}
-            size="sm"


size now gets inherited from CompactSelect, which gets the same size passed

TkDodo · 2025-12-16T14:34:28Z

static/app/components/organizations/environmentPageFilter/index.tsx

-            isOpen={isOpen}
-            size={selectProps.size}


both props are now applied automatically from SelectContext

TkDodo · 2025-12-16T14:35:38Z

static/app/components/replays/preferences/replayPreferenceDropdown.tsx

+      size="sm"
      disabled={isLoading}
      trigger={triggerProps => (
-        <Button
+        <SelectTrigger.Button
          {...triggerProps}
-          size="sm"
+          showChevron={false}


I intentionally moved size up to CompositeSelect because it’s not consistent to have a sm button with a md menu. I checked in the product and it looks good.

TkDodo · 2025-12-16T14:36:04Z

static/app/components/searchQueryBuilder/tokens/boolean.tsx

            options={LOGIC_OPERATOR_OPTIONS}
            trigger={triggerProps => {
              return (
+                // @ts-expect-error we don't allow arbitrary buttons as triggers


I can take a stab in a follow-up to try and get this to work with our buttons. Maybe a SelectTrigger.Unstyled would be a good idea too if we need this more often ?

TkDodo · 2025-12-16T14:39:24Z

static/app/components/dropdownButton.tsx

-export interface DropdownButtonProps
-  extends Omit<ButtonProps, 'type' | 'prefix' | 'onClick'> {
+export type DropdownButtonProps = DistributedOmit<
+  ButtonProps,
+  'type' | 'prefix' | 'onClick'
+> & {


Note: This is a classic type-level bug with Omit: ButtonProps is a discriminated union type, and Omit destroys that. So we had instances where we passed an icon, but no children and no aria-label, which the types tried to ensure.

DistributedOmit preserve the union type, but then again interfaces can’t extend union types, so I had to switch to types. This is the main reason why I think using type everywhere is better.

JonasBa · 2025-12-16T18:54:13Z

@TkDodo mind creating a design eng ticket for this, and mark it as done? I'm going to compile a changelog for beginning of next year, and I want to make sure we don't miss this

JonasBa

I think this PR captures really well the notion of composability and level of restriction that a design system should be opinionated about. Awesome work, as always!

JonasBa · 2025-12-16T18:49:41Z

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

+export type SelectTriggerProps = React.HTMLAttributes<TriggerEl> & {
+  ref?: React.Ref<TriggerEl>;
+};
+
+export type ButtonTriggerProps = React.ComponentPropsWithoutRef<typeof DropdownButton> & {
+  ref?: React.Ref<TriggerEl>;
+};
+
+type InputTriggerProps = React.ComponentPropsWithoutRef<typeof Input> & {
+  ref?: React.Ref<TriggerEl>;
+};


Can we export props or use an interface here? These are the kinds of types that can really slow down the type checker

can’t use an interface because ButtonProps is a union, which you can’t extend, and if I use DropdownButtonProps directly, I need to remove ref with DistributedOmit<DropdownButtonProps, 'ref'> which I don't think is better / faster.

JonasBa · 2025-12-16T18:52:26Z

static/app/components/dropdownMenu/index.tsx

   * component.
   */
-  triggerProps?: DropdownButtonProps;
+  triggerProps?: Partial<DropdownButtonProps>;


This might not seem like much, but it makes these components so much friendlier to work with

I mainly got some errors here because the union is now respected, some props like aria-label or children became required all of a sudden, which isn’t what we want here.

linear · 2025-12-17T08:31:02Z

DE-668 streamline compactSelect triggers

by adding a union with a type that contains the descriptive error message

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

TkDodo added 2 commits December 16, 2025 15:15

ref(scraps): compactSelect Triggers must be a specific SelectTrigger

d25a331

Merge branch 'master' into tkdodo/ref/dedicated-select-trigger

fdb6492

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

fix: add missing showShevron

ec428b7

TkDodo commented Dec 16, 2025

View reviewed changes

fix: ✂️ knip

2abfd65

TkDodo marked this pull request as ready for review December 16, 2025 14:43

TkDodo requested a review from a team December 16, 2025 14:43

TkDodo requested review from a team as code owners December 16, 2025 14:43

vercel bot deployed to Preview December 16, 2025 14:45 View deployment

TkDodo added 3 commits December 16, 2025 18:01

Merge branch 'master' into tkdodo/ref/dedicated-select-trigger

18cc7f3

fix: conflict after merge

404c8db

Merge branch 'master' into tkdodo/ref/dedicated-select-trigger

ad7ed07

vercel bot deployed to Preview December 16, 2025 17:12 View deployment

JonasBa approved these changes Dec 16, 2025

View reviewed changes

TkDodo added 2 commits December 17, 2025 09:43

ref: better error message

9234a5b

by adding a union with a type that contains the descriptive error message

Merge branch 'master' into tkdodo/ref/dedicated-select-trigger

0147a00

vercel bot deployed to Preview December 17, 2025 08:46 View deployment

docs

0a2170b

cursor bot reviewed Dec 17, 2025

View reviewed changes

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

vercel bot deployed to Preview December 17, 2025 09:07 View deployment

fix: forward disabled from select context

5b35523

ref: simplify props spreading

a280c03

vercel bot deployed to Preview December 17, 2025 09:17 View deployment

TkDodo merged commit b2da7c7 into master Dec 17, 2025
49 checks passed

TkDodo deleted the tkdodo/ref/dedicated-select-trigger branch December 17, 2025 09:30

github-actions bot locked and limited conversation to collaborators Jan 1, 2026

	trigger={triggerProps => (
	<MenuCrumb
	crumbLabel={name \|\| route.name}
	menuHasHover={isHovered}
	{...triggerProps}
	// @ts-expect-error - TODO: Crumb component should be refactored to use a button element instead of a div
	ref={triggerProps.ref}
	/>
	)}

Uh oh!

Conversation

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

Uh oh!

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!

JonasBa commented Dec 16, 2025

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!

linear bot commented Dec 17, 2025

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

TkDodo commented Dec 16, 2025 •

edited

Loading