Skip to content
30 changes: 21 additions & 9 deletions static/app/components/core/compactSelect/compactSelect.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ interface BaseSelectProps<Value extends SelectKey> extends Omit<
options: Array<SelectOptionOrSection<Value>>;
/**
* Number of options above which virtualization will be enabled.
* Note that virtualization is always disabled if there are sections in the options.
* @default 150
*/
virtualizeThreshold?: number;
Expand Down Expand Up @@ -144,7 +143,8 @@ export function CompactSelect<Value extends SelectKey>({
if (needsMeasuring) {
const longestOption = maxBy(options, option => {
if ('options' in option) {
return 0;
// Consider section header label length for width measurement
return typeof option.label === 'string' ? option.label.length : 0;
}
if (option.textValue) {
return option.textValue.length;
Expand All @@ -154,7 +154,20 @@ export function CompactSelect<Value extends SelectKey>({
}
return 0;
});
return longestOption ? getItemsWithKeys([longestOption]) : [];
if (!longestOption) {
return [];
}
// If the longest item is a section, pick its longest child option for measurement
if ('options' in longestOption && longestOption.options.length > 0) {
const longestChild = maxBy(longestOption.options, opt => {
if (opt.textValue) {
return opt.textValue.length;
}
return typeof opt.label === 'string' ? opt.label.length : 0;
});
return longestChild ? getItemsWithKeys([longestChild]) : [];
}
return getItemsWithKeys([longestOption]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Width measurement may pick wrong option across sections

Medium Severity

The maxBy call compares section header label lengths against option label/textValue lengths in a single pass. If a section header has the longest text, it "wins," and then only the longest child within that specific section is picked for width measurement. Options in other sections or at the top level that are actually wider get ignored, potentially resulting in a menu that's too narrow to display them.

Fix in Cursor Fix in Web

}
return [];
}, [needsMeasuring, options]);
Expand Down Expand Up @@ -243,12 +256,11 @@ function shouldVirtualize<Value extends SelectKey>(
items: Array<SelectOptionOrSection<Value>>,
virtualizeThreshold = 150
) {
const hasSections = items.some(item => 'options' in item);
if (hasSections) {
return false;
}

return items.length > virtualizeThreshold;
const totalCount = items.reduce(
(sum, item) => sum + ('options' in item ? item.options.length : 1),
0
);
return totalCount > virtualizeThreshold;
}

function trackVirtualizationMetrics<Value extends SelectKey>(
Expand Down
6 changes: 5 additions & 1 deletion static/app/components/core/compactSelect/control.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -558,7 +558,11 @@ export function Control({
{typeof menuBody === 'function'
? menuBody({closeOverlay: overlayState.close})
: menuBody}
{!hideOptions && <Stack minHeight="0">{children}</Stack>}
{!hideOptions && (
<Stack flex="1 1 0" minHeight="0">
{children}
</Stack>
)}
{menuFooter && (
<MenuFooter>
{typeof menuFooter === 'function'
Expand Down
109 changes: 77 additions & 32 deletions static/app/components/core/compactSelect/gridList/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ import {
type SelectKey,
type SelectSection,
} from '@sentry/scraps/compactSelect';
import {useVirtualizedItems} from '@sentry/scraps/compactSelect/useVirtualizedItems';
import {Container} from '@sentry/scraps/layout';

import {t} from 'sentry/locale';

Expand Down Expand Up @@ -58,11 +60,19 @@ interface GridListProps
section: SelectSection<SelectKey>,
type: 'select' | 'unselect'
) => void;
/**
* When false, hides section headers in the grid list.
*/
showSectionHeaders?: boolean;
size?: GridListOptionProps['size'];
/**
* Message to be displayed when some options are hidden due to `sizeLimit`.
*/
sizeLimitMessage?: string;
/**
* If true, virtualization will be enabled for the list.
*/
virtualized?: boolean;
}

/**
Expand All @@ -82,6 +92,8 @@ function GridList({
onSectionToggle,
sizeLimitMessage,
keyDownHandler,
virtualized,
showSectionHeaders = true,
...props
}: GridListProps) {
const ref = useRef<HTMLUListElement>(null);
Expand Down Expand Up @@ -114,42 +126,75 @@ function GridList({
[listState.collection, hiddenOptions]
);

const virtualizer = useVirtualizedItems({
listItems,
virtualized,
size,
hiddenOptions,
showSectionHeaders,
});

const listContent = virtualizer.items.map(row => {
const item = listItems[row.index]!;
if (item.type === 'section') {
return (
<GridListSection
{...virtualizer.itemProps(row.index)}
key={item.key}
node={item}
listState={listState}
onToggle={onSectionToggle}
size={size}
isFirst={row.index === 0}
/>
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GridListSection ignores showSectionHeaders prop causing height mismatch

Medium Severity

GridList accepts showSectionHeaders and passes it to useVirtualizedItems for height estimation, but never passes it to GridListSection. The section component always renders headers and separators regardless. This contrasts with ListBoxSection, which receives and respects showSectionHeaders. When showSectionHeaders is false, the virtualizer estimates zero height for headers but the DOM still renders them, causing a mismatch between estimated and actual section heights that can produce visual glitches (overlapping items or gaps) with virtualization enabled.

Additional Locations (1)

Fix in Cursor Fix in Web

}

return (
<GridListOption
{...virtualizer.itemProps(row.index)}
key={item.key}
node={item}
listState={listState}
size={size}
/>
);
});

const sizeLimitContent = !searchable && hiddenOptions.size > 0 && (
<SizeLimitMessage>
{sizeLimitMessage ?? t('Use search to find more options…')}
</SizeLimitMessage>
);

return (
<Fragment>
{listItems.length !== 0 && <ListSeparator role="separator" />}
{listItems.length !== 0 && label && <ListLabel id={labelId}>{label}</ListLabel>}
{overlayIsOpen && (
<ListWrap {...mergeProps(gridProps, props)} onKeyDown={onKeyDown} ref={ref}>
{listItems.map(item => {
if (item.type === 'section') {
return (
<GridListSection
key={item.key}
node={item}
listState={listState}
onToggle={onSectionToggle}
size={size}
/>
);
}

return (
<GridListOption
key={item.key}
node={item}
listState={listState}
size={size}
/>
);
})}

{!searchable && hiddenOptions.size > 0 && (
<SizeLimitMessage>
{sizeLimitMessage ?? t('Use search to find more options…')}
</SizeLimitMessage>
)}
</ListWrap>
)}
{overlayIsOpen &&
(virtualized ? (
<Container ref={virtualizer.scrollElementRef} height="100%" overflowY="auto">
<Container {...virtualizer.wrapperProps}>
<ListWrap
{...mergeProps(gridProps, props)}
style={{
...gridProps.style,
...virtualizer.listWrapStyle,
}}
onKeyDown={onKeyDown}
ref={ref}
>
{listContent}
{sizeLimitContent}
</ListWrap>
</Container>
</Container>
) : (
<ListWrap {...mergeProps(gridProps, props)} onKeyDown={onKeyDown} ref={ref}>
{listContent}
{sizeLimitContent}
</ListWrap>
))}
</Fragment>
);
}
Expand Down
15 changes: 12 additions & 3 deletions static/app/components/core/compactSelect/gridList/option.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import styled from '@emotion/styled';
import type {AriaGridListItemOptions} from '@react-aria/gridlist';
import {useGridListItem, useGridListSelectionCheckbox} from '@react-aria/gridlist';
import {useFocusWithin, useHover} from '@react-aria/interactions';
import {mergeProps} from '@react-aria/utils';
import {mergeProps, mergeRefs} from '@react-aria/utils';
import type {ListState} from '@react-stately/list';
import type {Node} from '@react-types/shared';

Expand All @@ -18,13 +18,21 @@ export interface GridListOptionProps extends AriaGridListItemOptions {
listState: ListState<any>;
node: Node<any>;
size: FormSize;
'data-index'?: number;
ref?: React.Ref<HTMLLIElement>;
}

/**
* A <li /> element with accessibile behaviors & attributes.
* https://react-spectrum.adobe.com/react-aria/useGridList.html
*/
export function GridListOption({node, listState, size}: GridListOptionProps) {
export function GridListOption({
node,
listState,
size,
ref: externalRef,
'data-index': dataIndex,
}: GridListOptionProps) {
const ref = useRef<HTMLLIElement>(null);
const {
label,
Expand Down Expand Up @@ -117,7 +125,8 @@ export function GridListOption({node, listState, size}: GridListOptionProps) {
return (
<StyledMenuListItem
{...rowPropsMemo}
ref={ref}
data-index={dataIndex}
ref={externalRef ? mergeRefs(ref, externalRef) : ref}
size={size}
label={label}
details={details}
Expand Down
88 changes: 52 additions & 36 deletions static/app/components/core/compactSelect/gridList/section.tsx
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
import {Fragment, useContext, useId, useMemo} from 'react';
import {useContext, useId, useMemo} from 'react';
import styled from '@emotion/styled';
import {useSeparator} from '@react-aria/separator';
import type {ListState} from '@react-stately/list';
import type {Node} from '@react-types/shared';

import {
SectionGroup,
SectionHeader,
SectionSeparator,
SectionTitle,
SectionToggle,
SectionWrap,
Expand All @@ -20,16 +20,27 @@ interface GridListSectionProps {
listState: ListState<any>;
node: Node<any>;
size: GridListOptionProps['size'];
'data-index'?: number;
isFirst?: boolean;
onToggle?: (section: SelectSection<SelectKey>, type: 'select' | 'unselect') => void;
ref?: React.Ref<HTMLLIElement>;
}

/**
* A <li /> element that functions as a grid list section (renders a nested <ul />
* inside). https://react-spectrum.adobe.com/react-aria/useGridList.html
*/
export function GridListSection({node, listState, onToggle, size}: GridListSectionProps) {
export function GridListSection({
node,
listState,
onToggle,
size,
ref,
'data-index': dataIndex,
isFirst = false,
}: GridListSectionProps) {
const titleId = useId();
const {separatorProps} = useSeparator({elementType: 'li'});
const {separatorProps} = useSeparator({elementType: 'div'});

const showToggleAllButton =
listState.selectionManager.selectionMode === 'multiple' &&
Expand All @@ -42,37 +53,42 @@ export function GridListSection({node, listState, onToggle, size}: GridListSecti
);

return (
<Fragment>
<SectionSeparator {...separatorProps} />
<SectionWrap
role="rowgroup"
{...(node['aria-label']
? {'aria-label': node['aria-label']}
: {'aria-labelledby': titleId})}
>
{(node.rendered || showToggleAllButton) && (
<SectionHeader>
{node.rendered && (
<SectionTitle id={titleId} aria-hidden>
{node.rendered}
</SectionTitle>
)}
{showToggleAllButton && (
<SectionToggle item={node} listState={listState} onToggle={onToggle} />
)}
</SectionHeader>
)}
<SectionGroup role="presentation">
{childNodes.map(child => (
<GridListOption
key={child.key}
node={child}
listState={listState}
size={size}
/>
))}
</SectionGroup>
</SectionWrap>
</Fragment>
<SectionWrap
role="rowgroup"
data-index={dataIndex}
ref={ref}
{...(node['aria-label']
? {'aria-label': node['aria-label']}
: {'aria-labelledby': titleId})}
>
{!isFirst && <SectionSeparatorInner {...separatorProps} />}
{(node.rendered || showToggleAllButton) && (
<SectionHeader>
{node.rendered && (
<SectionTitle id={titleId} aria-hidden>
{node.rendered}
</SectionTitle>
)}
{showToggleAllButton && (
<SectionToggle item={node} listState={listState} onToggle={onToggle} />
)}
</SectionHeader>
)}
<SectionGroup role="presentation">
{childNodes.map(child => (
<GridListOption
key={child.key}
node={child}
listState={listState}
size={size}
/>
))}
</SectionGroup>
</SectionWrap>
);
}

const SectionSeparatorInner = styled('div')`
border-top: solid 1px ${p => p.theme.tokens.border.secondary};
margin: ${p => p.theme.space.xs} ${p => p.theme.space.lg};
`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicate SectionSeparatorInner styled component across files

Low Severity

SectionSeparatorInner is identically defined in both gridList/section.tsx and listBox/section.tsx. There's already a shared SectionSeparator in styles.tsx (exported from the compactSelect package). This new component could be extracted to the shared styles module to avoid the duplication and risk of the two copies diverging.

Additional Locations (1)

Fix in Cursor Fix in Web

Loading
Loading