From 8d036698293866f91e4077b197a26328a69c0a2d Mon Sep 17 00:00:00 2001 From: TkDodo Date: Thu, 20 Nov 2025 14:00:51 +0100 Subject: [PATCH 01/18] ref: implement onClear by firing an onChange event with `undefined` and fix types to make onChange emit `| undefined` when `clearable` is set --- .../components/assigneeSelectorDropdown.tsx | 2 +- .../app/components/charts/optionSelector.tsx | 72 +++++++++++++------ .../core/compactSelect/compactSelect.tsx | 41 +++++++---- .../core/compactSelect/composite.tsx | 46 ++++-------- .../components/core/compactSelect/control.tsx | 37 +--------- .../components/core/compactSelect/list.tsx | 36 +++++++++- .../forms/fields/choiceMapperField.tsx | 4 +- .../components/timeRangeSelector/index.tsx | 6 +- .../performance/transactionSummary/filter.tsx | 2 +- .../transactionSummary/spanCategoryFilter.tsx | 5 +- .../transactionEvents/content.tsx | 4 +- .../transactionEvents/index.tsx | 2 +- .../transactionOverview/content.tsx | 2 +- .../gsApp/views/subscriptionPage/usageLog.tsx | 13 ++-- 14 files changed, 143 insertions(+), 129 deletions(-) diff --git a/static/app/components/assigneeSelectorDropdown.tsx b/static/app/components/assigneeSelectorDropdown.tsx index 656313f8e31ec4..c3c324ac1e28bc 100644 --- a/static/app/components/assigneeSelectorDropdown.tsx +++ b/static/app/components/assigneeSelectorDropdown.tsx @@ -311,7 +311,7 @@ export default function AssigneeSelectorDropdown({ })); }; - const handleSelect = (selectedOption: SelectOption | null) => { + const handleSelect = (selectedOption: SelectOption | undefined) => { // selectedOption is falsey when the option selected is already selected, or when the clear button is clicked if (!selectedOption) { if (onClear && group.assignedTo) { diff --git a/static/app/components/charts/optionSelector.tsx b/static/app/components/charts/optionSelector.tsx index c7a2c7e8e41aa2..e8152a4f50d097 100644 --- a/static/app/components/charts/optionSelector.tsx +++ b/static/app/components/charts/optionSelector.tsx @@ -1,6 +1,8 @@ import {Fragment, useMemo} from 'react'; import styled from '@emotion/styled'; +import type {DistributiveOmit} from '@sentry/scraps/types'; + import {FeatureBadge} from 'sentry/components/core/badge'; import type { MultipleSelectProps, @@ -17,28 +19,39 @@ type BaseProps = { featureType?: 'alpha' | 'beta' | 'new'; }; -interface SingleProps - extends Omit< - SingleSelectProps, - 'onChange' | 'defaultValue' | 'multiple' | 'title' | 'value' - >, - BaseProps { - onChange: (value: string) => void; - selected: string; - multiple?: false; -} +type SingleUnClearableProps = DistributiveOmit< + SingleSelectProps, + 'onChange' | 'multiple' | 'title' | 'value' +> & + BaseProps & { + onChange: (value: string) => void; + selected: string; + clearable?: false; + multiple?: false; + }; -interface MultipleProps - extends Omit< - MultipleSelectProps, - 'onChange' | 'defaultValue' | 'multiple' | 'title' | 'value' - >, - BaseProps { - multiple: true; - onChange: (value: string[]) => void; - selected: string[]; - defaultValue?: string[]; -} +type SingleClearableProps = DistributiveOmit< + SingleSelectProps, + 'onChange' | 'multiple' | 'title' | 'value' +> & + BaseProps & { + clearable: true; + onChange: (value: string | undefined) => void; + selected: string; + multiple?: false; + }; + +type SingleProps = SingleClearableProps | SingleUnClearableProps; + +type MultipleProps = DistributiveOmit< + MultipleSelectProps, + 'onChange' | 'multiple' | 'title' | 'value' +> & + BaseProps & { + multiple: true; + onChange: (value: string[]) => void; + selected: string[]; + }; function OptionSelector({ options, @@ -48,6 +61,7 @@ function OptionSelector({ featureType, multiple, closeOnSelect, + clearable, ...rest }: SingleProps | MultipleProps) { const mappedOptions = useMemo(() => { @@ -65,6 +79,7 @@ function OptionSelector({ if (multiple) { return { multiple, + clearable, value: selected, onChange: (sel: Array>) => { onChange?.(sel.map(o => o.value)); @@ -73,13 +88,24 @@ function OptionSelector({ }; } + if (clearable) { + return { + multiple, + clearable, + value: selected, + onChange: (opt: SelectOption | undefined) => onChange?.(opt?.value), + closeOnSelect, + }; + } + return { multiple, + clearable, value: selected, - onChange: (opt: any) => onChange?.(opt.value), + onChange: (opt: SelectOption) => onChange?.(opt.value), closeOnSelect, }; - }, [multiple, selected, onChange, closeOnSelect]); + }, [clearable, multiple, selected, onChange, closeOnSelect]); function isOptionDisabled(option: SelectOptionWithKey) { return Boolean( diff --git a/static/app/components/core/compactSelect/compactSelect.tsx b/static/app/components/core/compactSelect/compactSelect.tsx index 93e167f8aedae0..8554903cbef1a6 100644 --- a/static/app/components/core/compactSelect/compactSelect.tsx +++ b/static/app/components/core/compactSelect/compactSelect.tsx @@ -1,10 +1,11 @@ import {useId, useMemo} from 'react'; import {Item, Section} from '@react-stately/collections'; +import type {DistributiveOmit} from '@sentry/scraps/types'; + import {t} from 'sentry/locale'; -import type {ControlProps} from './control'; -import {Control} from './control'; +import {Control, type ControlProps} from './control'; import type {MultipleListProps, SingleListProps} from './list'; import {List} from './list'; import {EmptyMessage} from './styles'; @@ -19,23 +20,22 @@ import {getItemsWithKeys} from './utils'; export type {SelectOption, SelectOptionOrSection, SelectSection, SelectKey}; -interface BaseSelectProps extends ControlProps { +interface BaseSelectProps + extends Omit { options: Array>; } -export interface SingleSelectProps - extends BaseSelectProps, - Omit< - SingleListProps, - 'children' | 'items' | 'grid' | 'compositeIndex' | 'label' - > {} +export type SingleSelectProps = BaseSelectProps & + DistributiveOmit< + SingleListProps, + 'children' | 'items' | 'grid' | 'compositeIndex' | 'label' + >; -export interface MultipleSelectProps - extends BaseSelectProps, - Omit< - MultipleListProps, - 'children' | 'items' | 'grid' | 'compositeIndex' | 'label' - > {} +export type MultipleSelectProps = BaseSelectProps & + DistributiveOmit< + MultipleListProps, + 'children' | 'items' | 'grid' | 'compositeIndex' | 'label' + >; export type SelectProps = | SingleSelectProps @@ -63,6 +63,7 @@ export function CompactSelect({ onChange, onSectionToggle, multiple, + clearable, disallowEmptySelection, isOptionDisabled, sizeLimit, @@ -111,6 +112,16 @@ export function CompactSelect({ disabled={controlDisabled} grid={grid} size={size} + clearable={clearable} + onClear={() => { + if (clearable) { + if (multiple) { + onChange([]); + } else { + onChange(undefined); + } + } + }} > { * renders as a `ul` with its own list state) whose selection values don't interfere * with one another. */ -interface SingleCompositeSelectRegion - extends BaseCompositeSelectRegion, - Omit< +type SingleCompositeSelectRegion = + BaseCompositeSelectRegion & + DistributiveOmit< SingleListProps, - 'children' | 'items' | 'grid' | 'compositeIndex' | 'size' | 'limitOptions' - > {} + 'children' | 'items' | 'grid' | 'compositeIndex' | 'size' + >; /** * A multiple-selection (multiple options can be selected at the same time) "region" @@ -39,12 +41,12 @@ interface SingleCompositeSelectRegion * list (each renders as a `ul` with its own list state) whose selection values don't * interfere with one another. */ -interface MultipleCompositeSelectRegion - extends BaseCompositeSelectRegion, - Omit< +type MultipleCompositeSelectRegion = + BaseCompositeSelectRegion & + DistributiveOmit< MultipleListProps, - 'children' | 'items' | 'grid' | 'compositeIndex' | 'size' | 'limitOptions' - > {} + 'children' | 'items' | 'grid' | 'compositeIndex' | 'size' + >; /** * A "region" inside a composite select. Each "region" is a separated, self-contained @@ -132,42 +134,18 @@ type RegionProps = CompositeSelectRegion & { function Region({ options, - value, - onChange, - multiple, disallowEmptySelection, isOptionDisabled, - closeOnSelect, size, compositeIndex, label, ...props }: RegionProps) { - // Combine list props into an object with two clearly separated types, one where - // `multiple` is true and the other where it's not. Necessary to avoid TS errors. - const listProps = useMemo(() => { - if (multiple) { - return { - multiple, - value, - closeOnSelect, - onChange, - }; - } - return { - multiple, - value, - closeOnSelect, - onChange, - }; - }, [multiple, value, onChange, closeOnSelect]); - const itemsWithKey = useMemo(() => getItemsWithKeys(options), [options]); return ( void) { interface SelectContextValue { overlayIsOpen: boolean; - /** - * Function to be called once when a list is initialized, to register its state in - * SelectContext. In composite selectors, where there can be multiple lists, the - * `index` parameter is the list's index number (the order in which it appears). In - * non-composite selectors, where there's only one list, that list's index is 0. - */ - registerListState: (index: number, listState: ListState) => void; /** * Function to be called when a list's selection state changes. We need a complete * list of all selected options to label the trigger button. The `index` parameter - * indentifies the list, in the same way as in `registerListState`. + * identifies the list. */ saveSelectedOptions: ( index: number, @@ -78,7 +70,6 @@ interface SelectContextValue { } export const SelectContext = createContext({ - registerListState: () => {}, saveSelectedOptions: () => {}, overlayIsOpen: false, search: '', @@ -256,10 +247,6 @@ export function Control({ ...wrapperProps }: ControlProps) { const wrapperRef = useRef(null); - // Set up list states (in composite selects, each region has its own state, that way - // selection values are contained within each region). - const [listStates, setListStates] = useState>>([]); - /** * Search/filter value, used to filter out the list of displayed elements */ @@ -297,14 +284,6 @@ export function Control({ }, }); - /** - * Clears selection values across all list states - */ - const clearSelection = () => { - listStates.forEach(listState => listState.selectionManager.clearSelection()); - onClear?.(); - }; - // Manage overlay position const { isOpen: overlayIsOpen, @@ -485,19 +464,7 @@ export function Control({ ); const contextValue = useMemo(() => { - const registerListState: SelectContextValue['registerListState'] = ( - index, - listState - ) => { - setListStates(current => [ - ...current.slice(0, index), - listState, - ...current.slice(index + 1), - ]); - }; - return { - registerListState, saveSelectedOptions, overlayState, overlayIsOpen, @@ -549,7 +516,7 @@ export function Control({ ? menuHeaderTrailingItems({closeOverlay: overlayState.close}) : menuHeaderTrailingItems} {clearable && showClearButton && ( - + {t('Clear')} )} diff --git a/static/app/components/core/compactSelect/list.tsx b/static/app/components/core/compactSelect/list.tsx index b667faee6b9ea6..c1ee714abeff38 100644 --- a/static/app/components/core/compactSelect/list.tsx +++ b/static/app/components/core/compactSelect/list.tsx @@ -99,9 +99,35 @@ interface BaseListProps sizeLimitMessage?: string; } -export interface SingleListProps extends BaseListProps { +/** + * A single-selection (only one option can be selected at a time) list that allows + * clearing the selection. + * `value` can be `undefined` to represent no selection. + */ +export interface SingleClearableListProps + extends BaseListProps { + clearable: true; + onChange: (selectedOption: SelectOption | undefined) => void; + value: Value | undefined; + /** + * Whether to close the menu. Accepts either a boolean value or a callback function + * that receives the newly selected option and returns whether to close the menu. + */ + closeOnSelect?: + | boolean + | ((selectedOption: SelectOption | undefined) => boolean); + multiple?: false; +} + +export type SingleListProps = + | SingleClearableListProps + | SingleUnclearableListProps; + +export interface SingleUnclearableListProps + extends BaseListProps { onChange: (selectedOption: SelectOption) => void; value: Value | undefined; + clearable?: false; /** * Whether to close the menu. Accepts either a boolean value or a callback function * that receives the newly selected option and returns whether to close the menu. @@ -114,6 +140,11 @@ export interface MultipleListProps extends BaseListProp multiple: true; onChange: (selectedOptions: Array>) => void; value: Value[] | undefined; + /** + * set to a regular boolean here because the empty type can be represented as an empty array + */ + clearable?: boolean; + /** * Whether to close the menu. Accepts either a boolean value or a callback function * that receives the newly selected options and returns whether to close the menu. @@ -144,7 +175,7 @@ function List({ closeOnSelect, ...props }: SingleListProps | MultipleListProps) { - const {overlayState, registerListState, saveSelectedOptions, search, overlayIsOpen} = + const {overlayState, saveSelectedOptions, search, overlayIsOpen} = useContext(SelectContext); const hiddenOptions = useMemo( @@ -234,7 +265,6 @@ function List({ // Register the initialized list state once on mount useLayoutEffect(() => { - registerListState(compositeIndex, listState); saveSelectedOptions( compositeIndex, getSelectedOptions(items, listState.selectionManager.selectedKeys) diff --git a/static/app/components/forms/fields/choiceMapperField.tsx b/static/app/components/forms/fields/choiceMapperField.tsx index 367fc622694e1f..9759a342876fcf 100644 --- a/static/app/components/forms/fields/choiceMapperField.tsx +++ b/static/app/components/forms/fields/choiceMapperField.tsx @@ -1,6 +1,8 @@ import {Component, Fragment} from 'react'; import styled from '@emotion/styled'; +import type {DistributiveOmit} from '@sentry/scraps/types'; + import {Button} from 'sentry/components/core/button'; import { CompactSelect, @@ -48,7 +50,7 @@ export interface ChoiceMapperProps extends DefaultProps { /** * Props forwarded to the add mapping dropdown. */ - addDropdown: Omit, 'options'> & { + addDropdown: DistributiveOmit, 'options' | 'clearable'> & { items: Array>; noResultsMessage?: string; }; diff --git a/static/app/components/timeRangeSelector/index.tsx b/static/app/components/timeRangeSelector/index.tsx index 0f6ca8ba54e033..66e5027a5f6b86 100644 --- a/static/app/components/timeRangeSelector/index.tsx +++ b/static/app/components/timeRangeSelector/index.tsx @@ -65,7 +65,7 @@ export interface TimeRangeSelectorProps | 'options' | 'hideOptions' | 'value' - | 'defaultValue' + | 'clearable' | 'onChange' | 'onInteractOutside' | 'closeOnSelect' @@ -263,8 +263,8 @@ export function TimeRangeSelector({ ); }, [showRelative, onChange, internalValue, hasChanges]); - const handleChange = useCallback['onChange']>>( - option => { + const handleChange = useCallback( + (option: SelectOption) => { // The absolute option was selected -> open absolute selector if (option.value === ABSOLUTE_OPTION_VALUE) { setInternalValue(current => { diff --git a/static/app/views/performance/transactionSummary/filter.tsx b/static/app/views/performance/transactionSummary/filter.tsx index 4a217d3f59288a..68a8dc0d95258a 100644 --- a/static/app/views/performance/transactionSummary/filter.tsx +++ b/static/app/views/performance/transactionSummary/filter.tsx @@ -45,7 +45,7 @@ const OPTIONS: SpanOperationBreakdownFilter[] = [ type Props = { currentFilter: SpanOperationBreakdownFilter; - onChangeFilter: (newFilter: SpanOperationBreakdownFilter) => void; + onChangeFilter: (newFilter: SpanOperationBreakdownFilter | undefined) => void; organization: OrganizationSummary; }; diff --git a/static/app/views/performance/transactionSummary/spanCategoryFilter.tsx b/static/app/views/performance/transactionSummary/spanCategoryFilter.tsx index 563d007c6dcb23..eb90af256ce443 100644 --- a/static/app/views/performance/transactionSummary/spanCategoryFilter.tsx +++ b/static/app/views/performance/transactionSummary/spanCategoryFilter.tsx @@ -69,8 +69,8 @@ export function SpanCategoryFilter({serviceEntrySpanName}: Props) { })) ); - const onChange = (selectedOption: SelectOption | null) => { - setSelectedCategory(selectedOption?.value ?? undefined); + const onChange = (selectedOption: SelectOption | undefined) => { + setSelectedCategory(selectedOption?.value); navigate({ ...location, @@ -94,7 +94,6 @@ export function SpanCategoryFilter({serviceEntrySpanName}: Props) { clearable disallowEmptySelection={false} menuTitle={t('Filter by category')} - onClear={() => setSelectedCategory(undefined)} options={categoryOptions} value={selectedCategory ?? undefined} onChange={onChange} diff --git a/static/app/views/performance/transactionSummary/transactionEvents/content.tsx b/static/app/views/performance/transactionSummary/transactionEvents/content.tsx index 4786b1a4d241b3..afa5ed7bdfee1f 100644 --- a/static/app/views/performance/transactionSummary/transactionEvents/content.tsx +++ b/static/app/views/performance/transactionSummary/transactionEvents/content.tsx @@ -48,7 +48,9 @@ type Props = { eventsDisplayFilterName: EventsDisplayFilterName; location: Location; onChangeEventsDisplayFilter: (eventsDisplayFilterName: EventsDisplayFilterName) => void; - onChangeSpanOperationBreakdownFilter: (newFilter: SpanOperationBreakdownFilter) => void; + onChangeSpanOperationBreakdownFilter: ( + newFilter: SpanOperationBreakdownFilter | undefined + ) => void; organization: Organization; projectId: string; projects: Project[]; diff --git a/static/app/views/performance/transactionSummary/transactionEvents/index.tsx b/static/app/views/performance/transactionSummary/transactionEvents/index.tsx index 2c263f7d5310b9..ece01081aeefc0 100644 --- a/static/app/views/performance/transactionSummary/transactionEvents/index.tsx +++ b/static/app/views/performance/transactionSummary/transactionEvents/index.tsx @@ -58,7 +58,7 @@ function TransactionEvents() { }; const onChangeSpanOperationBreakdownFilter = ( - newFilter: SpanOperationBreakdownFilter + newFilter: SpanOperationBreakdownFilter | undefined ) => { trackAnalytics('performance_views.transactionEvents.ops_filter_dropdown.selection', { organization, diff --git a/static/app/views/performance/transactionSummary/transactionOverview/content.tsx b/static/app/views/performance/transactionSummary/transactionOverview/content.tsx index 182c11a143d441..a275816202aa28 100644 --- a/static/app/views/performance/transactionSummary/transactionOverview/content.tsx +++ b/static/app/views/performance/transactionSummary/transactionOverview/content.tsx @@ -83,7 +83,7 @@ type Props = { eventView: EventView; isLoading: boolean; location: Location; - onChangeFilter: (newFilter: SpanOperationBreakdownFilter) => void; + onChangeFilter: (newFilter: SpanOperationBreakdownFilter | undefined) => void; organization: Organization; projectId: string; projects: Project[]; diff --git a/static/gsApp/views/subscriptionPage/usageLog.tsx b/static/gsApp/views/subscriptionPage/usageLog.tsx index 6f2ee0f4c45d32..d4739329d92736 100644 --- a/static/gsApp/views/subscriptionPage/usageLog.tsx +++ b/static/gsApp/views/subscriptionPage/usageLog.tsx @@ -122,17 +122,17 @@ function UsageLog({location, subscription}: Props) { [auditLogs?.eventNames] ); - const handleEventFilter = (value: string | null) => { - if (value === null) { - // Clear filters + const handleEventFilter = (value: string | undefined) => { + if (typeof value === 'string') { navigate({ pathname: location.pathname, - query: {...location.query, event: undefined, cursor: undefined}, + query: {...location.query, event: value, cursor: undefined}, }); } else { + // Clear filters navigate({ pathname: location.pathname, - query: {...location.query, event: value, cursor: undefined}, + query: {...location.query, event: undefined, cursor: undefined}, }); } @@ -166,9 +166,8 @@ function UsageLog({location, subscription}: Props) { menuTitle={t('Subscription Actions')} options={eventNameOptions} value={selectedEventName} - onClear={() => handleEventFilter(null)} onChange={option => { - handleEventFilter(option.value); + handleEventFilter(option?.value); }} triggerProps={{ size: 'sm', From 8a636b62c32fc286b3e1ba1dd8be4154e3f1d3f5 Mon Sep 17 00:00:00 2001 From: TkDodo Date: Thu, 20 Nov 2025 14:48:15 +0100 Subject: [PATCH 02/18] ref: set disallowEmptySelection implicitly depending on clearable --- static/app/components/assigneeSelectorDropdown.tsx | 1 - .../core/compactSelect/compactSelect.stories.tsx | 1 + .../components/core/compactSelect/compactSelect.tsx | 6 +++--- .../app/components/core/compactSelect/composite.tsx | 2 -- static/app/components/core/compactSelect/list.tsx | 11 ++++++----- .../components/forms/metric/templateSection.tsx | 1 - .../mobile/screenload/components/affectSelector.tsx | 1 - .../views/performance/transactionSummary/filter.tsx | 5 ++--- .../transactionSummary/spanCategoryFilter.tsx | 1 - .../transactionSummary/transactionEvents/index.tsx | 4 +++- .../transactionSummary/transactionOverview/index.tsx | 6 +++--- .../settingsBreadcrumb/breadcrumbDropdown.tsx | 3 ++- 12 files changed, 20 insertions(+), 22 deletions(-) diff --git a/static/app/components/assigneeSelectorDropdown.tsx b/static/app/components/assigneeSelectorDropdown.tsx index c3c324ac1e28bc..b04a1338b617a5 100644 --- a/static/app/components/assigneeSelectorDropdown.tsx +++ b/static/app/components/assigneeSelectorDropdown.tsx @@ -565,7 +565,6 @@ export default function AssigneeSelectorDropdown({ className={className} menuWidth={275} position="bottom-end" - disallowEmptySelection={false} onClick={e => e.stopPropagation()} value={ group.assignedTo diff --git a/static/app/components/core/compactSelect/compactSelect.stories.tsx b/static/app/components/core/compactSelect/compactSelect.stories.tsx index 7ad5aeeec20518..c4574cb19b1c94 100644 --- a/static/app/components/core/compactSelect/compactSelect.stories.tsx +++ b/static/app/components/core/compactSelect/compactSelect.stories.tsx @@ -303,6 +303,7 @@ export default Storybook.story('CompactSelect', story => { { setValue(newValue.value); }} diff --git a/static/app/components/core/compactSelect/compactSelect.tsx b/static/app/components/core/compactSelect/compactSelect.tsx index 8554903cbef1a6..b6473cc2718426 100644 --- a/static/app/components/core/compactSelect/compactSelect.tsx +++ b/static/app/components/core/compactSelect/compactSelect.tsx @@ -64,7 +64,6 @@ export function CompactSelect({ onSectionToggle, multiple, clearable, - disallowEmptySelection, isOptionDisabled, sizeLimit, sizeLimitMessage, @@ -85,6 +84,7 @@ export function CompactSelect({ const listProps = useMemo(() => { if (multiple) { return { + clearable, multiple, value, onChange, @@ -93,13 +93,14 @@ export function CompactSelect({ }; } return { + clearable, multiple, value, onChange, closeOnSelect, grid, }; - }, [multiple, value, onChange, closeOnSelect, grid]); + }, [multiple, clearable, value, onChange, closeOnSelect, grid]); const itemsWithKey = useMemo(() => getItemsWithKeys(options), [options]); @@ -127,7 +128,6 @@ export function CompactSelect({ {...listProps} items={itemsWithKey} onSectionToggle={onSectionToggle} - disallowEmptySelection={disallowEmptySelection} isOptionDisabled={isOptionDisabled} size={size} sizeLimit={sizeLimit} diff --git a/static/app/components/core/compactSelect/composite.tsx b/static/app/components/core/compactSelect/composite.tsx index 17b1f65781be23..104dee6fca15f2 100644 --- a/static/app/components/core/compactSelect/composite.tsx +++ b/static/app/components/core/compactSelect/composite.tsx @@ -134,7 +134,6 @@ type RegionProps = CompositeSelectRegion & { function Region({ options, - disallowEmptySelection, isOptionDisabled, size, compositeIndex, @@ -147,7 +146,6 @@ function Region({ ()); interface BaseListProps - extends ListProps, + extends Omit, 'disallowEmptySelection'>, Omit< AriaListBoxOptions, + | 'disallowEmptySelection' | 'disabledKeys' | 'selectedKeys' | 'defaultSelectedKeys' @@ -48,6 +49,7 @@ interface BaseListProps >, Omit< AriaGridListOptions, + | 'disallowEmptySelection' | 'disabledKeys' | 'selectedKeys' | 'defaultSelectedKeys' @@ -165,7 +167,7 @@ function List({ onChange, grid, multiple, - disallowEmptySelection, + clearable, isOptionDisabled, shouldFocusWrap = true, shouldFocusOnHover = true, @@ -198,7 +200,6 @@ function List({ disabledKeys, // react-aria turns all keys into strings selectedKeys: value?.map(getEscapedKey), - disallowEmptySelection, allowDuplicateSelectionEvents: true, onSelectionChange: selection => { const selectedOptions = getSelectedOptions(items, selection); @@ -223,7 +224,7 @@ function List({ disabledKeys, // react-aria turns all keys into strings selectedKeys: defined(value) ? [getEscapedKey(value)] : undefined, - disallowEmptySelection: disallowEmptySelection ?? true, + disallowEmptySelection: !clearable, allowDuplicateSelectionEvents: true, onSelectionChange: selection => { const selectedOption = getSelectedOptions(items, selection)[0]!; @@ -250,7 +251,7 @@ function List({ isOptionDisabled, hiddenOptions, multiple, - disallowEmptySelection, + clearable, compositeIndex, saveSelectedOptions, closeOnSelect, diff --git a/static/app/views/detectors/components/forms/metric/templateSection.tsx b/static/app/views/detectors/components/forms/metric/templateSection.tsx index 621ef553b9f194..b4afff15a072c6 100644 --- a/static/app/views/detectors/components/forms/metric/templateSection.tsx +++ b/static/app/views/detectors/components/forms/metric/templateSection.tsx @@ -120,7 +120,6 @@ export function TemplateSection() { { return ( { + const onChangeFilter = (newFilter: SpanOperationBreakdownFilter | undefined) => { trackAnalytics('performance_views.filter_dropdown.selection', { organization, action: newFilter as string, @@ -151,7 +151,7 @@ function OTelOverviewContentWrapper() { const nextQuery: Location['query'] = { ...removeHistogramQueryStrings(location, [ZOOM_START, ZOOM_END]), - ...filterToLocationQuery(newFilter), + ...(newFilter ? filterToLocationQuery(newFilter) : undefined), }; if (newFilter === SpanOperationBreakdownFilter.NONE) { @@ -249,7 +249,7 @@ function OverviewContentWrapper() { const spanOperationBreakdownFilter = decodeFilterFromLocation(location); - const onChangeFilter = (newFilter: SpanOperationBreakdownFilter) => { + const onChangeFilter = (newFilter: SpanOperationBreakdownFilter | undefined) => { trackAnalytics('performance_views.filter_dropdown.selection', { organization, action: newFilter as string, diff --git a/static/app/views/settings/components/settingsBreadcrumb/breadcrumbDropdown.tsx b/static/app/views/settings/components/settingsBreadcrumb/breadcrumbDropdown.tsx index ebd6a5b3bb887e..98d6d321259749 100644 --- a/static/app/views/settings/components/settingsBreadcrumb/breadcrumbDropdown.tsx +++ b/static/app/views/settings/components/settingsBreadcrumb/breadcrumbDropdown.tsx @@ -12,7 +12,8 @@ import Crumb from './crumb'; import Divider from './divider'; import type {RouteWithName} from './types'; -interface BreadcrumbDropdownProps extends Omit, 'onChange'> { +interface BreadcrumbDropdownProps + extends Omit, 'onChange' | 'clearable'> { name: React.ReactNode; onCrumbSelect: (value: string) => void; route: RouteWithName; From 6ae272429c0413c7f769dd5c82f944c9da042c7e Mon Sep 17 00:00:00 2001 From: TkDodo Date: Thu, 20 Nov 2025 14:49:19 +0100 Subject: [PATCH 03/18] typescript needs more help --- .../components/core/compactSelect/compactSelect.tsx | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/static/app/components/core/compactSelect/compactSelect.tsx b/static/app/components/core/compactSelect/compactSelect.tsx index b6473cc2718426..c772a5f541d70c 100644 --- a/static/app/components/core/compactSelect/compactSelect.tsx +++ b/static/app/components/core/compactSelect/compactSelect.tsx @@ -81,6 +81,7 @@ export function CompactSelect({ // Combine list props into an object with two clearly separated types, one where // `multiple` is true and the other where it's not. Necessary to avoid TS errors. + // also multiple:false must be split into clearable true/false to satisfy TS const listProps = useMemo(() => { if (multiple) { return { @@ -92,6 +93,18 @@ export function CompactSelect({ grid, }; } + + if (clearable) { + return { + clearable, + multiple, + value, + onChange, + closeOnSelect, + grid, + }; + } + return { clearable, multiple, From 6ceba45c36ec1a162939f95501925e6032f8df4c Mon Sep 17 00:00:00 2001 From: TkDodo Date: Thu, 20 Nov 2025 15:28:02 +0100 Subject: [PATCH 04/18] fix: have clear button re-mount the internal list to clear listSelection --- .../components/core/compactSelect/compactSelect.tsx | 6 +++++- static/app/components/core/compactSelect/control.tsx | 10 +++++++++- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/static/app/components/core/compactSelect/compactSelect.tsx b/static/app/components/core/compactSelect/compactSelect.tsx index c772a5f541d70c..0760350e69e63a 100644 --- a/static/app/components/core/compactSelect/compactSelect.tsx +++ b/static/app/components/core/compactSelect/compactSelect.tsx @@ -1,4 +1,4 @@ -import {useId, useMemo} from 'react'; +import {useId, useMemo, useState} from 'react'; import {Item, Section} from '@react-stately/collections'; import type {DistributiveOmit} from '@sentry/scraps/types'; @@ -119,6 +119,8 @@ export function CompactSelect({ const controlDisabled = disabled ?? options?.length === 0; + const [listKey, setListKey] = useState(0); + return ( ({ } else { onChange(undefined); } + setListKey(key => key + 1); } }} > { + setSelectedOptions([]); + onClear?.(); + }; + // Manage overlay position const { isOpen: overlayIsOpen, @@ -516,7 +524,7 @@ export function Control({ ? menuHeaderTrailingItems({closeOverlay: overlayState.close}) : menuHeaderTrailingItems} {clearable && showClearButton && ( - + {t('Clear')} )} From 09f0a44b5d93e03d1d3f67f84602fb14d36c3013 Mon Sep 17 00:00:00 2001 From: TkDodo Date: Thu, 20 Nov 2025 15:48:55 +0100 Subject: [PATCH 05/18] fix: fix clearing values by not passing `undefined` to `selectedItems` --- .../app/components/core/compactSelect/compactSelect.tsx | 6 +----- static/app/components/core/compactSelect/list.tsx | 9 ++++----- 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/static/app/components/core/compactSelect/compactSelect.tsx b/static/app/components/core/compactSelect/compactSelect.tsx index 0760350e69e63a..c772a5f541d70c 100644 --- a/static/app/components/core/compactSelect/compactSelect.tsx +++ b/static/app/components/core/compactSelect/compactSelect.tsx @@ -1,4 +1,4 @@ -import {useId, useMemo, useState} from 'react'; +import {useId, useMemo} from 'react'; import {Item, Section} from '@react-stately/collections'; import type {DistributiveOmit} from '@sentry/scraps/types'; @@ -119,8 +119,6 @@ export function CompactSelect({ const controlDisabled = disabled ?? options?.length === 0; - const [listKey, setListKey] = useState(0); - return ( ({ } else { onChange(undefined); } - setListKey(key => key + 1); } }} > ({ selectionMode: 'multiple' as const, disabledKeys, // react-aria turns all keys into strings - selectedKeys: value?.map(getEscapedKey), + selectedKeys: value?.map(getEscapedKey) ?? [], allowDuplicateSelectionEvents: true, onSelectionChange: selection => { const selectedOptions = getSelectedOptions(items, selection); @@ -223,21 +223,20 @@ function List({ selectionMode: 'single' as const, disabledKeys, // react-aria turns all keys into strings - selectedKeys: defined(value) ? [getEscapedKey(value)] : undefined, + selectedKeys: defined(value) ? [getEscapedKey(value)] : [], disallowEmptySelection: !clearable, allowDuplicateSelectionEvents: true, onSelectionChange: selection => { const selectedOption = getSelectedOptions(items, selection)[0]!; // Save selected options in SelectContext, to update the trigger label - saveSelectedOptions(compositeIndex, selectedOption ?? null); - onChange?.(selectedOption ?? null); + onChange?.(selectedOption); // Close menu if closeOnSelect is true or undefined (by default single-selection // menus will close on selection) if ( !defined(closeOnSelect) || (typeof closeOnSelect === 'function' - ? closeOnSelect(selectedOption ?? null) + ? closeOnSelect(selectedOption) : closeOnSelect) ) { overlayState?.close(); From 1e098f5220ae9d23d0572b31da723dc45c7d4f84 Mon Sep 17 00:00:00 2001 From: TkDodo Date: Thu, 20 Nov 2025 16:15:47 +0100 Subject: [PATCH 06/18] test: type-tests for compactSelect onChange param --- .../core/compactSelect/compactSelect.spec.tsx | 96 ++++++++++++++++++- 1 file changed, 95 insertions(+), 1 deletion(-) diff --git a/static/app/components/core/compactSelect/compactSelect.spec.tsx b/static/app/components/core/compactSelect/compactSelect.spec.tsx index 4ad0b031c45d7e..650e46ce3db39b 100644 --- a/static/app/components/core/compactSelect/compactSelect.spec.tsx +++ b/static/app/components/core/compactSelect/compactSelect.spec.tsx @@ -1,10 +1,104 @@ import {Fragment} from 'react'; +import {expectTypeOf} from 'expect-type'; import {render, screen, userEvent, waitFor} from 'sentry-test/reactTestingLibrary'; -import {CompactSelect} from './'; +import {CompactSelect, type SelectOption} from './'; describe('CompactSelect', () => { + describe('types', () => { + it('should enforce correct types for onChange for SingleSelect', () => { + void ( + { + expectTypeOf(option).toEqualTypeOf>(); + }} + closeOnSelect={option => { + expectTypeOf(option).toEqualTypeOf>(); + return true; + }} + options={[ + {value: 'opt_one', label: 'Option One'}, + {value: 'opt_two', label: 'Option Two'}, + ]} + /> + ); + }); + + it('should add undefined to onChange when clearable for SingleSelect', () => { + void ( + { + expectTypeOf(option).toEqualTypeOf< + SelectOption<'opt_one' | 'opt_two'> | undefined + >(); + }} + closeOnSelect={option => { + expectTypeOf(option).toEqualTypeOf< + SelectOption<'opt_one' | 'opt_two'> | undefined + >(); + return true; + }} + options={[ + {value: 'opt_one', label: 'Option One'}, + {value: 'opt_two', label: 'Option Two'}, + ]} + /> + ); + }); + + it('should always use arrays for MultiSelect', () => { + const values: Array<'opt_one' | 'opt_two'> = ['opt_one']; + void ( + { + expectTypeOf(option).toEqualTypeOf< + Array> + >(); + }} + closeOnSelect={option => { + expectTypeOf(option).toEqualTypeOf< + Array> + >(); + return true; + }} + options={[ + {value: 'opt_one', label: 'Option One'}, + {value: 'opt_two', label: 'Option Two'}, + ]} + /> + ); + + void ( + { + expectTypeOf(option).toEqualTypeOf< + Array> + >(); + }} + closeOnSelect={option => { + expectTypeOf(option).toEqualTypeOf< + Array> + >(); + return true; + }} + options={[ + {value: 'opt_one', label: 'Option One'}, + {value: 'opt_two', label: 'Option Two'}, + ]} + /> + ); + }); + }); + it('renders', async () => { render( Date: Thu, 20 Nov 2025 16:16:36 +0100 Subject: [PATCH 07/18] =?UTF-8?q?=E2=9C=82=EF=B8=8F?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- static/app/components/core/compactSelect/list.tsx | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/static/app/components/core/compactSelect/list.tsx b/static/app/components/core/compactSelect/list.tsx index c260285e761832..8048e8e2865021 100644 --- a/static/app/components/core/compactSelect/list.tsx +++ b/static/app/components/core/compactSelect/list.tsx @@ -106,8 +106,7 @@ interface BaseListProps * clearing the selection. * `value` can be `undefined` to represent no selection. */ -export interface SingleClearableListProps - extends BaseListProps { +interface SingleClearableListProps extends BaseListProps { clearable: true; onChange: (selectedOption: SelectOption | undefined) => void; value: Value | undefined; @@ -125,7 +124,7 @@ export type SingleListProps = | SingleClearableListProps | SingleUnclearableListProps; -export interface SingleUnclearableListProps +interface SingleUnclearableListProps extends BaseListProps { onChange: (selectedOption: SelectOption) => void; value: Value | undefined; From 5adce71135340b5822d26ba3298af3b68c949ee6 Mon Sep 17 00:00:00 2001 From: TkDodo Date: Thu, 20 Nov 2025 16:17:03 +0100 Subject: [PATCH 08/18] fix stories --- .../components/core/compactSelect/compactSelect.stories.tsx | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/static/app/components/core/compactSelect/compactSelect.stories.tsx b/static/app/components/core/compactSelect/compactSelect.stories.tsx index c4574cb19b1c94..1ee062e0914a39 100644 --- a/static/app/components/core/compactSelect/compactSelect.stories.tsx +++ b/static/app/components/core/compactSelect/compactSelect.stories.tsx @@ -284,7 +284,7 @@ export default Storybook.story('CompactSelect', story => { }); story('Simple', () => { - const [value, setValue] = useState(''); + const [value, setValue] = useState(); const options = [ {value: '', label: 'All'}, {value: '2', label: '2XX', details: 'Optional'}, @@ -303,7 +303,6 @@ export default Storybook.story('CompactSelect', story => { { setValue(newValue.value); }} From 22ee87eab120f98d26282bf9561b8e34feb00331 Mon Sep 17 00:00:00 2001 From: TkDodo Date: Thu, 20 Nov 2025 16:36:17 +0100 Subject: [PATCH 09/18] test: fix databaseSystemSelector.spec.tsx since both navigate and localStorage are mocked, and the internal state of the component was removed, we no longer see a real update reflected in the test; this works fine in the product --- .../insights/database/components/databaseSystemSelector.spec.tsx | 1 - 1 file changed, 1 deletion(-) diff --git a/static/app/views/insights/database/components/databaseSystemSelector.spec.tsx b/static/app/views/insights/database/components/databaseSystemSelector.spec.tsx index c94d0896dfad36..64acea1f0be98a 100644 --- a/static/app/views/insights/database/components/databaseSystemSelector.spec.tsx +++ b/static/app/views/insights/database/components/databaseSystemSelector.spec.tsx @@ -219,7 +219,6 @@ describe('DatabaseSystemSelector', () => { expect(dropdownOptionLabels[1]).toHaveTextContent('MongoDB'); await userEvent.click(dropdownOptionLabels[0]!); - expect(dropdownSelector).toHaveTextContent('SystemPostgreSQL'); expect(mockSetState).toHaveBeenCalledWith('postgresql'); expect(mockNavigate).toHaveBeenCalledWith({ action: 'POP', From 7e0a8267d5d5dfc98c073a4287b39046e2d21d8e Mon Sep 17 00:00:00 2001 From: TkDodo Date: Thu, 20 Nov 2025 16:41:03 +0100 Subject: [PATCH 10/18] test: fix environmentSelector.spec.tsx if you mock `onChange`, the label doesn't magically update anymore --- .../components/workflowEngine/form/environmentSelector.spec.tsx | 2 -- 1 file changed, 2 deletions(-) diff --git a/static/app/components/workflowEngine/form/environmentSelector.spec.tsx b/static/app/components/workflowEngine/form/environmentSelector.spec.tsx index bb2be65be0e8af..a0ffd76f46d271 100644 --- a/static/app/components/workflowEngine/form/environmentSelector.spec.tsx +++ b/static/app/components/workflowEngine/form/environmentSelector.spec.tsx @@ -53,8 +53,6 @@ describe('EnvironmentSelector', () => { // Select "prod" await userEvent.click(screen.getByRole('option', {name: 'prod'})); - // Trigger label is updated - expect(screen.getByRole('button', {name: 'prod'})).toBeInTheDocument(); expect(mockOnChange).toHaveBeenCalledWith('prod'); }); }); From a02aa0fdb1701b2efec17e0960dbeea8412c17f2 Mon Sep 17 00:00:00 2001 From: TkDodo Date: Thu, 20 Nov 2025 17:04:45 +0100 Subject: [PATCH 11/18] test: add state to compactSelect tests --- .../core/compactSelect/compactSelect.spec.tsx | 325 +++++++++++------- 1 file changed, 200 insertions(+), 125 deletions(-) diff --git a/static/app/components/core/compactSelect/compactSelect.spec.tsx b/static/app/components/core/compactSelect/compactSelect.spec.tsx index 650e46ce3db39b..8d88e008af2dfa 100644 --- a/static/app/components/core/compactSelect/compactSelect.spec.tsx +++ b/static/app/components/core/compactSelect/compactSelect.spec.tsx @@ -1,4 +1,4 @@ -import {Fragment} from 'react'; +import {Fragment, useState} from 'react'; import {expectTypeOf} from 'expect-type'; import {render, screen, userEvent, waitFor} from 'sentry-test/reactTestingLibrary'; @@ -210,16 +210,25 @@ describe('CompactSelect', () => { describe('ListBox', () => { it('updates trigger label on selection', async () => { const mock = jest.fn(); - render( - - ); + + function Component() { + const [state, setState] = useState(); + return ( + { + mock(selection); + setState(selection.value); + }} + /> + ); + } + + render(); // click on the trigger button await userEvent.click(screen.getByRole('button')); @@ -233,17 +242,26 @@ describe('CompactSelect', () => { it('can select multiple options', async () => { const mock = jest.fn(); - render( - - ); + + function Component() { + const [state, setState] = useState([]); + return ( + { + mock(selection); + setState(selection.map(opt => opt.value)); + }} + value={state} + /> + ); + } + + render(); // click on the trigger button await userEvent.click(screen.getByRole('button')); @@ -261,17 +279,26 @@ describe('CompactSelect', () => { it('can select options with values containing quotes', async () => { const mock = jest.fn(); - render( - - ); + + function Component() { + const [state, setState] = useState([]); + return ( + { + mock(selection); + setState(selection.map(opt => opt.value)); + }} + value={state} + /> + ); + } + + render(); // click on the trigger button await userEvent.click(screen.getByRole('button')); @@ -424,34 +451,44 @@ describe('CompactSelect', () => { it('can toggle sections', async () => { const mock = jest.fn(); - render( - - ); + + function Component() { + const [state, setState] = useState([]); + + return ( + { + mock(selection); + setState(selection.map(opt => opt.value)); + }} + options={[ + { + key: 'section-1', + label: 'Section 1', + showToggleAllButton: true, + options: [ + {value: 'opt_one', label: 'Option One'}, + {value: 'opt_two', label: 'Option Two'}, + ], + }, + { + key: 'section-2', + label: 'Section 2', + showToggleAllButton: true, + options: [ + {value: 'opt_three', label: 'Option Three'}, + {value: 'opt_four', label: 'Option Four'}, + ], + }, + ]} + /> + ); + } + + render(); // click on the trigger button await userEvent.click(screen.getByRole('button', {expanded: false})); @@ -562,17 +599,27 @@ describe('CompactSelect', () => { describe('GridList', () => { it('updates trigger label on selection', async () => { const mock = jest.fn(); - render( - - ); + + function Component() { + const [state, setState] = useState(); + + return ( + { + mock(selection); + setState(selection.value); + }} + /> + ); + } + + render(); // click on the trigger button await userEvent.click(screen.getByRole('button')); @@ -586,18 +633,27 @@ describe('CompactSelect', () => { it('can select multiple options', async () => { const mock = jest.fn(); - render( - - ); + + function Component() { + const [state, setState] = useState([]); + return ( + { + mock(selection); + setState(selection.map(opt => opt.value)); + }} + value={state} + /> + ); + } + + render(); // click on the trigger button await userEvent.click(screen.getByRole('button')); @@ -615,18 +671,27 @@ describe('CompactSelect', () => { it('can select options with values containing quotes', async () => { const mock = jest.fn(); - render( - - ); + + function Component() { + const [state, setState] = useState([]); + return ( + { + mock(selection); + setState(selection.map(opt => opt.value)); + }} + value={state} + /> + ); + } + + render(); // click on the trigger button await userEvent.click(screen.getByRole('button')); @@ -733,35 +798,45 @@ describe('CompactSelect', () => { it('can toggle sections', async () => { const mock = jest.fn(); - render( - - ); + + function Component() { + const [state, setState] = useState([]); + + return ( + { + mock(selection); + setState(selection.map(opt => opt.value)); + }} + options={[ + { + key: 'section-1', + label: 'Section 1', + showToggleAllButton: true, + options: [ + {value: 'opt_one', label: 'Option One'}, + {value: 'opt_two', label: 'Option Two'}, + ], + }, + { + key: 'section-2', + label: 'Section 2', + showToggleAllButton: true, + options: [ + {value: 'opt_three', label: 'Option Three'}, + {value: 'opt_four', label: 'Option Four'}, + ], + }, + ]} + /> + ); + } + + render(); // click on the trigger button await userEvent.click(screen.getByRole('button', {expanded: false})); From bde40d9c8a8a29bd574d3381ce397c5816a600cf Mon Sep 17 00:00:00 2001 From: TkDodo Date: Thu, 20 Nov 2025 18:13:13 +0100 Subject: [PATCH 12/18] fix: missing call to saveSelectedOptions --- static/app/components/core/compactSelect/list.tsx | 1 + .../components/workflowEngine/form/environmentSelector.spec.tsx | 2 ++ .../database/components/databaseSystemSelector.spec.tsx | 1 + 3 files changed, 4 insertions(+) diff --git a/static/app/components/core/compactSelect/list.tsx b/static/app/components/core/compactSelect/list.tsx index 8048e8e2865021..7a0a7691206094 100644 --- a/static/app/components/core/compactSelect/list.tsx +++ b/static/app/components/core/compactSelect/list.tsx @@ -228,6 +228,7 @@ function List({ onSelectionChange: selection => { const selectedOption = getSelectedOptions(items, selection)[0]!; // Save selected options in SelectContext, to update the trigger label + saveSelectedOptions(compositeIndex, selectedOption); onChange?.(selectedOption); // Close menu if closeOnSelect is true or undefined (by default single-selection diff --git a/static/app/components/workflowEngine/form/environmentSelector.spec.tsx b/static/app/components/workflowEngine/form/environmentSelector.spec.tsx index a0ffd76f46d271..bb2be65be0e8af 100644 --- a/static/app/components/workflowEngine/form/environmentSelector.spec.tsx +++ b/static/app/components/workflowEngine/form/environmentSelector.spec.tsx @@ -53,6 +53,8 @@ describe('EnvironmentSelector', () => { // Select "prod" await userEvent.click(screen.getByRole('option', {name: 'prod'})); + // Trigger label is updated + expect(screen.getByRole('button', {name: 'prod'})).toBeInTheDocument(); expect(mockOnChange).toHaveBeenCalledWith('prod'); }); }); diff --git a/static/app/views/insights/database/components/databaseSystemSelector.spec.tsx b/static/app/views/insights/database/components/databaseSystemSelector.spec.tsx index 64acea1f0be98a..c94d0896dfad36 100644 --- a/static/app/views/insights/database/components/databaseSystemSelector.spec.tsx +++ b/static/app/views/insights/database/components/databaseSystemSelector.spec.tsx @@ -219,6 +219,7 @@ describe('DatabaseSystemSelector', () => { expect(dropdownOptionLabels[1]).toHaveTextContent('MongoDB'); await userEvent.click(dropdownOptionLabels[0]!); + expect(dropdownSelector).toHaveTextContent('SystemPostgreSQL'); expect(mockSetState).toHaveBeenCalledWith('postgresql'); expect(mockNavigate).toHaveBeenCalledWith({ action: 'POP', From 19bbd42bb3f76f253e6fc177b85dfe669e1a90e5 Mon Sep 17 00:00:00 2001 From: TkDodo Date: Thu, 20 Nov 2025 18:16:15 +0100 Subject: [PATCH 13/18] fix: restore condition --- .../transactionSummary/transactionOverview/index.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/static/app/views/performance/transactionSummary/transactionOverview/index.tsx b/static/app/views/performance/transactionSummary/transactionOverview/index.tsx index 057e9dfff3ccc3..0ea8b17e8e8338 100644 --- a/static/app/views/performance/transactionSummary/transactionOverview/index.tsx +++ b/static/app/views/performance/transactionSummary/transactionOverview/index.tsx @@ -151,7 +151,7 @@ function OTelOverviewContentWrapper() { const nextQuery: Location['query'] = { ...removeHistogramQueryStrings(location, [ZOOM_START, ZOOM_END]), - ...(newFilter ? filterToLocationQuery(newFilter) : undefined), + ...filterToLocationQuery(newFilter), }; if (newFilter === SpanOperationBreakdownFilter.NONE) { From 566704fc440e5a4430e54a51e49e96b3359be91e Mon Sep 17 00:00:00 2001 From: TkDodo Date: Thu, 20 Nov 2025 18:28:23 +0100 Subject: [PATCH 14/18] test: add state management to composite.spec.tsx --- .../core/compactSelect/composite.spec.tsx | 119 +++++++++++------- 1 file changed, 72 insertions(+), 47 deletions(-) diff --git a/static/app/components/core/compactSelect/composite.spec.tsx b/static/app/components/core/compactSelect/composite.spec.tsx index 20a67debd506e8..6d9298956c271b 100644 --- a/static/app/components/core/compactSelect/composite.spec.tsx +++ b/static/app/components/core/compactSelect/composite.spec.tsx @@ -1,8 +1,10 @@ +import {useState} from 'react'; + import {render, screen, userEvent, waitFor} from 'sentry-test/reactTestingLibrary'; import {CompositeSelect} from './composite'; -describe('CompactSelect', () => { +describe('CompositeSelect', () => { it('renders', async () => { render( { it('has separate, async self-contained select regions', async () => { const region1Mock = jest.fn(); const region2Mock = jest.fn(); - render( - }> - - - - ); + + function Component() { + const [region1, setRegion1] = useState(); + const [region2, setRegion2] = useState([]); + return ( + }> + { + region1Mock(selection); + setRegion1(selection.value); + }} + value={region1} + options={[ + {value: 'choice_one', label: 'Choice One'}, + {value: 'choice_two', label: 'Choice Two'}, + ]} + /> + { + region2Mock(selection); + setRegion2(selection.map(s => s.value)); + }} + value={region2} + options={[ + {value: 'choice_three', label: 'Choice Three'}, + {value: 'choice_four', label: 'Choice Four'}, + ]} + /> + + ); + } + + render(); // click on the trigger button await userEvent.click(screen.getByRole('button')); @@ -260,29 +275,39 @@ describe('CompactSelect', () => { }); it('works with grid lists', async () => { - render( - }> - {}} - options={[ - {value: 'choice_one', label: 'Choice One'}, - {value: 'choice_two', label: 'Choice Two'}, - ]} - /> - - - ); + function Component() { + const [region1, setRegion1] = useState('choice_one'); + const [region2, setRegion2] = useState([]); + return ( + }> + { + setRegion1(selection.value); + }} + value={region1} + options={[ + {value: 'choice_one', label: 'Choice One'}, + {value: 'choice_two', label: 'Choice Two'}, + ]} + /> + { + setRegion2(selection.map(s => s.value)); + }} + value={region2} + options={[ + {value: 'choice_three', label: 'Choice Three'}, + {value: 'choice_four', label: 'Choice Four'}, + ]} + /> + + ); + } + + render(); // click on the trigger button await userEvent.click(screen.getByRole('button')); From 3b7a44ce4cb3178aa52676092484309119acf82a Mon Sep 17 00:00:00 2001 From: TkDodo Date: Fri, 21 Nov 2025 09:06:27 +0100 Subject: [PATCH 15/18] fix: comments --- static/app/components/core/compactSelect/list.tsx | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/static/app/components/core/compactSelect/list.tsx b/static/app/components/core/compactSelect/list.tsx index 7a0a7691206094..a8641e17bedf1a 100644 --- a/static/app/components/core/compactSelect/list.tsx +++ b/static/app/components/core/compactSelect/list.tsx @@ -107,6 +107,9 @@ interface BaseListProps * `value` can be `undefined` to represent no selection. */ interface SingleClearableListProps extends BaseListProps { + /** + * If true, there will be a "Clear" button in the menu header. + */ clearable: true; onChange: (selectedOption: SelectOption | undefined) => void; value: Value | undefined; @@ -141,10 +144,7 @@ export interface MultipleListProps extends BaseListProp multiple: true; onChange: (selectedOptions: Array>) => void; value: Value[] | undefined; - /** - * set to a regular boolean here because the empty type can be represented as an empty array - */ - clearable?: boolean; + clearable?: boolean; // set to a regular boolean here because the empty type can be represented as an empty array /** * Whether to close the menu. Accepts either a boolean value or a callback function @@ -222,6 +222,8 @@ function List({ selectionMode: 'single' as const, disabledKeys, // react-aria turns all keys into strings + // we're setting selectedKeys to an empty array when value is undefined, because + // undefined makes react-aria treat it as uncontrolled selectedKeys: defined(value) ? [getEscapedKey(value)] : [], disallowEmptySelection: !clearable, allowDuplicateSelectionEvents: true, From 79352c22985939556046543cd30159fcb5c2e021 Mon Sep 17 00:00:00 2001 From: TkDodo Date: Fri, 21 Nov 2025 13:02:07 +0100 Subject: [PATCH 16/18] fix: menu should close when `clear` is clicked --- .../core/compactSelect/compactSelect.spec.tsx | 26 +++++++++++++++++++ .../core/compactSelect/compactSelect.tsx | 8 +++++- .../components/core/compactSelect/control.tsx | 8 ++++++ .../components/core/compactSelect/list.tsx | 17 +++++------- .../components/core/compactSelect/utils.tsx | 21 +++++++++++++++ 5 files changed, 69 insertions(+), 11 deletions(-) diff --git a/static/app/components/core/compactSelect/compactSelect.spec.tsx b/static/app/components/core/compactSelect/compactSelect.spec.tsx index 8d88e008af2dfa..53f14e64fa20cc 100644 --- a/static/app/components/core/compactSelect/compactSelect.spec.tsx +++ b/static/app/components/core/compactSelect/compactSelect.spec.tsx @@ -207,6 +207,32 @@ describe('CompactSelect', () => { expect(await screen.findByRole('option', {name: 'Option Three'})).toBeInTheDocument(); }); + it('closes menu when clear is clicked', async () => { + render( + + ); + + // open the menu + await userEvent.click(screen.getByRole('button', {name: 'Option One'})); + expect(screen.getByRole('option', {name: 'Option One'})).toBeInTheDocument(); + + // click the clear button + await userEvent.click(screen.getByRole('button', {name: 'Clear'})); + + // menu is closed + await waitFor(() => { + expect(screen.queryByRole('option', {name: 'Option One'})).not.toBeInTheDocument(); + }); + }); + describe('ListBox', () => { it('updates trigger label on selection', async () => { const mock = jest.fn(); diff --git a/static/app/components/core/compactSelect/compactSelect.tsx b/static/app/components/core/compactSelect/compactSelect.tsx index c772a5f541d70c..1a78bcdabc990e 100644 --- a/static/app/components/core/compactSelect/compactSelect.tsx +++ b/static/app/components/core/compactSelect/compactSelect.tsx @@ -16,7 +16,7 @@ import type { SelectOptionOrSectionWithKey, SelectSection, } from './types'; -import {getItemsWithKeys} from './utils'; +import {getItemsWithKeys, shouldCloseOnSelect} from './utils'; export type {SelectOption, SelectOptionOrSection, SelectSection, SelectKey}; @@ -127,6 +127,12 @@ export function CompactSelect({ grid={grid} size={size} clearable={clearable} + shouldCloseOnSelect={({selectedOptions}) => + shouldCloseOnSelect({ + ...listProps, + selectedOptions, + }) + } onClear={() => { if (clearable) { if (multiple) { diff --git a/static/app/components/core/compactSelect/control.tsx b/static/app/components/core/compactSelect/control.tsx index 05c5cfa8ac96ff..eaf4de8fe507ad 100644 --- a/static/app/components/core/compactSelect/control.tsx +++ b/static/app/components/core/compactSelect/control.tsx @@ -185,6 +185,9 @@ export interface ControlProps * menu items. */ searchable?: boolean; + shouldCloseOnSelect?: (props: { + selectedOptions: Array>; + }) => boolean; size?: FormSize; /** * Optional replacement for the default trigger button. Note that the replacement must @@ -197,6 +200,7 @@ export interface ControlProps }, isOpen: boolean ) => React.ReactNode; + /** * Props to be passed to the default trigger button. */ @@ -217,6 +221,7 @@ export function Control({ onInteractOutside, shouldCloseOnInteractOutside, shouldCloseOnBlur, + shouldCloseOnSelect, preventOverflowOptions, flipOptions, disabled, @@ -289,6 +294,9 @@ export function Control({ */ const clearSelection = () => { setSelectedOptions([]); + if (shouldCloseOnSelect?.({selectedOptions: []})) { + overlayState?.close(); + } onClear?.(); }; diff --git a/static/app/components/core/compactSelect/list.tsx b/static/app/components/core/compactSelect/list.tsx index a8641e17bedf1a..7e21328130e5d5 100644 --- a/static/app/components/core/compactSelect/list.tsx +++ b/static/app/components/core/compactSelect/list.tsx @@ -31,6 +31,7 @@ import { getHiddenOptions, getSelectedOptions, HiddenSectionToggle, + shouldCloseOnSelect, } from './utils'; export const SelectFilterContext = createContext(new Set()); @@ -206,12 +207,7 @@ function List({ saveSelectedOptions(compositeIndex, selectedOptions); onChange?.(selectedOptions); - // Close menu if closeOnSelect is true - if ( - typeof closeOnSelect === 'function' - ? closeOnSelect(selectedOptions) - : closeOnSelect - ) { + if (shouldCloseOnSelect({multiple, closeOnSelect, selectedOptions})) { overlayState?.close(); } }, @@ -236,10 +232,11 @@ function List({ // Close menu if closeOnSelect is true or undefined (by default single-selection // menus will close on selection) if ( - !defined(closeOnSelect) || - (typeof closeOnSelect === 'function' - ? closeOnSelect(selectedOption) - : closeOnSelect) + shouldCloseOnSelect({ + multiple, + closeOnSelect, + selectedOptions: [selectedOption], + }) ) { overlayState?.close(); } diff --git a/static/app/components/core/compactSelect/utils.tsx b/static/app/components/core/compactSelect/utils.tsx index 74cae18802985f..2b33a8902963a3 100644 --- a/static/app/components/core/compactSelect/utils.tsx +++ b/static/app/components/core/compactSelect/utils.tsx @@ -6,7 +6,9 @@ import type {ListState} from '@react-stately/list'; import type {Node, Selection} from '@react-types/shared'; import {t} from 'sentry/locale'; +import {defined} from 'sentry/utils'; +import type {SelectProps} from './compactSelect'; import {SectionToggleButton} from './styles'; import type { SelectKey, @@ -327,3 +329,22 @@ export function itemIsSectionWithKey( ): item is SelectSectionWithKey { return 'options' in item; } + +export function shouldCloseOnSelect({ + multiple, + closeOnSelect, + selectedOptions, +}: Pick, 'multiple' | 'closeOnSelect'> & { + selectedOptions: Array>; +}) { + if (typeof closeOnSelect === 'function') { + // type assertions are necessary here because we don't have the discriminated union anymore + return closeOnSelect((multiple ? selectedOptions : selectedOptions[0]) as never); + } + if (defined(closeOnSelect)) { + return closeOnSelect; + } + // By default, single-selection lists close on select, while multiple-selection + // lists stay open + return !multiple; +} From 202ed6c10ed16e30846e7783378fda25e9d99888 Mon Sep 17 00:00:00 2001 From: TkDodo Date: Fri, 21 Nov 2025 13:08:20 +0100 Subject: [PATCH 17/18] ref: better way to close overlay onClear --- .../components/core/compactSelect/compactSelect.tsx | 11 ++++------- static/app/components/core/compactSelect/control.tsx | 11 ++--------- 2 files changed, 6 insertions(+), 16 deletions(-) diff --git a/static/app/components/core/compactSelect/compactSelect.tsx b/static/app/components/core/compactSelect/compactSelect.tsx index 1a78bcdabc990e..ffdbaa4899503d 100644 --- a/static/app/components/core/compactSelect/compactSelect.tsx +++ b/static/app/components/core/compactSelect/compactSelect.tsx @@ -127,19 +127,16 @@ export function CompactSelect({ grid={grid} size={size} clearable={clearable} - shouldCloseOnSelect={({selectedOptions}) => - shouldCloseOnSelect({ - ...listProps, - selectedOptions, - }) - } - onClear={() => { + onClear={({overlayState}) => { if (clearable) { if (multiple) { onChange([]); } else { onChange(undefined); } + if (shouldCloseOnSelect?.({...listProps, selectedOptions: []})) { + overlayState?.close(); + } } }} > diff --git a/static/app/components/core/compactSelect/control.tsx b/static/app/components/core/compactSelect/control.tsx index eaf4de8fe507ad..d0af9ad9de041e 100644 --- a/static/app/components/core/compactSelect/control.tsx +++ b/static/app/components/core/compactSelect/control.tsx @@ -166,7 +166,7 @@ export interface ControlProps * Called when the clear button is clicked (applicable only when `clearable` is * true). */ - onClear?: () => void; + onClear?: (props: {overlayState: OverlayTriggerState}) => void; /** * Called when the menu is opened or closed. */ @@ -185,9 +185,6 @@ export interface ControlProps * menu items. */ searchable?: boolean; - shouldCloseOnSelect?: (props: { - selectedOptions: Array>; - }) => boolean; size?: FormSize; /** * Optional replacement for the default trigger button. Note that the replacement must @@ -221,7 +218,6 @@ export function Control({ onInteractOutside, shouldCloseOnInteractOutside, shouldCloseOnBlur, - shouldCloseOnSelect, preventOverflowOptions, flipOptions, disabled, @@ -294,10 +290,7 @@ export function Control({ */ const clearSelection = () => { setSelectedOptions([]); - if (shouldCloseOnSelect?.({selectedOptions: []})) { - overlayState?.close(); - } - onClear?.(); + onClear?.({overlayState}); }; // Manage overlay position From cf7e9e297a61d00e4bd37832b6b9e2c11cfa7359 Mon Sep 17 00:00:00 2001 From: TkDodo Date: Fri, 21 Nov 2025 13:44:47 +0100 Subject: [PATCH 18/18] fix: remove unnecessary check --- .../detectors/components/forms/metric/templateSection.tsx | 3 --- 1 file changed, 3 deletions(-) diff --git a/static/app/views/detectors/components/forms/metric/templateSection.tsx b/static/app/views/detectors/components/forms/metric/templateSection.tsx index b4afff15a072c6..4652d60d76b0a6 100644 --- a/static/app/views/detectors/components/forms/metric/templateSection.tsx +++ b/static/app/views/detectors/components/forms/metric/templateSection.tsx @@ -133,9 +133,6 @@ export function TemplateSection() { ); }} onChange={option => { - if (!option) { - return; - } const key = option.value; const meta = templateMetaByKey[key as MetricAlertType];