Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions static/app/components/searchQueryBuilder/index.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2320,6 +2320,38 @@ describe('SearchQueryBuilder', () => {
});
});

it('sorts multi-selected values after re-opening the combobox', async () => {
const user = userEvent.setup();
render(<SearchQueryBuilder {...defaultProps} initialQuery='browser.name:""' />);

await userEvent.click(
screen.getByRole('button', {name: 'Edit value for filter: browser.name'})
);

const checkboxOptions = await screen.findAllByRole('checkbox');
expect(checkboxOptions).toHaveLength(4);
expect(checkboxOptions[0]).toHaveAccessibleName('Toggle Chrome');
expect(checkboxOptions[1]).toHaveAccessibleName('Toggle Firefox');

await user.keyboard('{Control>}');
await user.click(checkboxOptions[1]!);

expect(checkboxOptions[1]).toHaveAccessibleName('Toggle Firefox');
expect(checkboxOptions[1]).toBeChecked();

await user.keyboard('{Escape}');
await userEvent.click(
screen.getByRole('button', {name: 'Edit value for filter: browser.name'})
);

const updatedCheckboxOptions = await screen.findAllByRole('checkbox');
expect(updatedCheckboxOptions).toHaveLength(4);
expect(updatedCheckboxOptions[0]).toHaveAccessibleName('Toggle Firefox');
expect(updatedCheckboxOptions[0]).toBeChecked();
expect(updatedCheckboxOptions[1]).toHaveAccessibleName('Toggle Chrome');
expect(updatedCheckboxOptions[1]).not.toBeChecked();
});

it('collapses many selected options', async () => {
render(
<SearchQueryBuilder
Expand Down
18 changes: 13 additions & 5 deletions static/app/components/searchQueryBuilder/tokens/combobox.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -95,10 +95,11 @@ type SearchQueryBuilderComboboxProps<T extends SelectOptionOrSectionWithKey<stri
extra: {state: ComboBoxState<T>}
) => void;
onKeyUp?: (e: KeyboardEvent) => void;
onOpenChange?: (newOpenState: boolean) => void;
onOpenChange?: (newOpenState: boolean, state?: any) => void;
onPaste?: (e: React.ClipboardEvent<HTMLInputElement>) => void;
openOnFocus?: boolean;
placeholder?: string;
preserveFocusOnInputChange?: boolean;
ref?: React.Ref<HTMLInputElement>;
/**
* Function to determine whether the menu should close when interacting with
Expand Down Expand Up @@ -365,6 +366,7 @@ export function SearchQueryBuilderCombobox<
onClick,
customMenu,
isOpen: incomingIsOpen,
preserveFocusOnInputChange = false,
['data-test-id']: dataTestId,
ref,
}: SearchQueryBuilderComboboxProps<T>) {
Expand Down Expand Up @@ -462,10 +464,15 @@ export function SearchQueryBuilderCombobox<

const previousInputValue = usePrevious(inputValue);
useEffect(() => {
if (inputValue !== previousInputValue) {
if (!preserveFocusOnInputChange && inputValue !== previousInputValue) {
state.selectionManager.setFocusedKey(null);
}
}, [inputValue, previousInputValue, state.selectionManager]);
}, [
inputValue,
previousInputValue,
state.selectionManager,
preserveFocusOnInputChange,
]);

const totalOptions = items.reduce(
(acc, item) => acc + (itemIsSection(item) ? item.options.length : 1),
Expand All @@ -482,9 +489,10 @@ export function SearchQueryBuilderCombobox<
isOpen: incomingIsOpen,
});

// Always notify parent of state so they can maintain up-to-date references
useEffect(() => {
onOpenChange?.(isOpen);
}, [onOpenChange, isOpen]);
onOpenChange?.(isOpen, state);
}, [onOpenChange, isOpen, state]);

const {
overlayProps,
Expand Down
135 changes: 116 additions & 19 deletions static/app/components/searchQueryBuilder/tokens/filter/valueCombobox.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -144,10 +144,9 @@ function getSelectedValuesFromText(

// Check if this value is selected by looking at the character after the value in
// the text. If there's a comma after the value, it means this value is selected.
// We need to check the text content to ensure that we account for any quotes the
// user may have added.
const valueText = item.value?.text ?? '';
const selected = text.charAt(text.indexOf(valueText) + valueText.length) === ',';
// Use the actual token location from the parser instead of indexOf to avoid
// matching substrings (e.g., "artifact_bundle.assemble" inside "artifact_bundle.assemble.find_projects")
const selected = text.charAt(item.value?.location.end.offset ?? 0) === ',';
Comment on lines +147 to +149

Choose a reason for hiding this comment

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

[BestPractice]

Potential substring matching issue in selection detection:

// Use the actual token location from the parser instead of indexOf to avoid
// matching substrings (e.g., "artifact_bundle.assemble" inside "artifact_bundle.assemble.find_projects")
const selected = text.charAt(item.value?.location.end.offset ?? 0) === ',';

This fix is good, but there's still a potential issue: if item.value?.location.end.offset is 0 (falsy), the fallback ?? 0 will use index 0, which may not be correct for the actual token position. Consider using a more explicit check:

const endOffset = item.value?.location?.end?.offset;
if (endOffset === undefined) {
  // Handle case where location info is missing
  continue; // or some other fallback logic
}
const selected = text.charAt(endOffset) === ',';
Context for Agents
[**BestPractice**]

Potential substring matching issue in selection detection:

```typescript
// Use the actual token location from the parser instead of indexOf to avoid
// matching substrings (e.g., "artifact_bundle.assemble" inside "artifact_bundle.assemble.find_projects")
const selected = text.charAt(item.value?.location.end.offset ?? 0) === ',';
```

This fix is good, but there's still a potential issue: if `item.value?.location.end.offset` is 0 (falsy), the fallback `?? 0` will use index 0, which may not be correct for the actual token position. Consider using a more explicit check:

```typescript
const endOffset = item.value?.location?.end?.offset;
if (endOffset === undefined) {
  // Handle case where location info is missing
  continue; // or some other fallback logic
}
const selected = text.charAt(endOffset) === ',';
```

File: static/app/components/searchQueryBuilder/tokens/filter/valueCombobox.tsx
Line: 149


return {value, selected};
});
Expand Down Expand Up @@ -323,10 +322,12 @@ function useFilterSuggestions({
token,
filterValue,
selectedValues,
initialSelectedValues,
ctrlKeyPressed,
}: {
ctrlKeyPressed: boolean;
filterValue: string;
initialSelectedValues: Array<{selected: boolean; value: string}>;
selectedValues: Array<{selected: boolean; value: string}>;
token: TokenResult<Token.FILTER>;
}) {
Expand Down Expand Up @@ -381,8 +382,13 @@ function useFilterSuggestions({
enabled: shouldFetchValues,
});

// Create a ref to hold current selected values for the checkbox to access
// without causing item recreation
const selectedValuesRef = useRef(selectedValues);
selectedValuesRef.current = selectedValues;

const createItem = useCallback(
(suggestion: SuggestionItem, selected = false) => {
(suggestion: SuggestionItem) => {
return {
label: suggestion.label ?? suggestion.value,
value: suggestion.value,
Expand All @@ -395,6 +401,11 @@ function useFilterSuggestions({
return null;
}

// Look up selected state dynamically from ref to avoid recreating items
const selected = selectedValuesRef.current.some(
v => v.value === suggestion.value && v.selected
);

return (
<ItemCheckbox
isFocused={isFocused}
Expand Down Expand Up @@ -434,41 +445,71 @@ function useFilterSuggestions({
}, [data, predefinedValues, shouldFetchValues, key?.key]);

// Grouped sections for rendering purposes
// Use initialSelectedValues for ordering (to avoid re-ordering during selection)
// Selected state is looked up dynamically in the checkbox render to avoid recreating items
const suggestionSectionItems = useMemo<SuggestionSectionItem[]>(() => {
const allSuggestionValues = suggestionGroups
.flatMap(group => group.suggestions)
.map(s => s.value);

// Build list with initialSelectedValues at top, followed by all other suggestions
const initialValueSet = new Set(initialSelectedValues.map(v => v.value));

const itemsWithoutSection = suggestionGroups
.filter(group => group.sectionText === '')
.flatMap(group => group.suggestions)
.filter(suggestion => !selectedValues.some(v => v.value === suggestion.value));
.filter(suggestion => !initialValueSet.has(suggestion.value));
const sections = suggestionGroups.filter(group => group.sectionText !== '');

// Add the current filterValue as an option if it's not empty and not already in the list
const trimmedFilterValue = filterValue.trim();
const shouldShowFilterValue =
canSelectMultipleValues &&
trimmedFilterValue.length > 0 &&
!allSuggestionValues.includes(trimmedFilterValue) &&
!initialValueSet.has(trimmedFilterValue);

return [
{
sectionText: '',
items: getItemsWithKeys([
...selectedValues.map(value => {
// Show initial selected values at top (these were added via typing+comma)
...initialSelectedValues.map(value => {
const matchingSuggestion = suggestionGroups
.flatMap(group => group.suggestions)
.find(suggestion => suggestion.value === value.value);

if (matchingSuggestion) {
return createItem(matchingSuggestion, value.selected);
return createItem(matchingSuggestion);
}

return createItem({value: value.value}, value.selected);
return createItem({value: value.value});
}),
// Show currently typed value if it's new
...(shouldShowFilterValue ? [createItem({value: filterValue.trim()})] : []),
// Then all other suggestions in their original order
...itemsWithoutSection.map(suggestion => createItem(suggestion)),
]),
},
...sections.map(group => ({
sectionText: group.sectionText,
items: getItemsWithKeys(
group.suggestions
.filter(suggestion => !selectedValues.some(v => v.value === suggestion.value))
.filter(suggestion => !initialValueSet.has(suggestion.value))
.map(suggestion => createItem(suggestion))
),
})),
];
}, [createItem, selectedValues, suggestionGroups]);
// selectedValues is needed to trigger re-renders when checkboxes are toggled
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [
createItem,
suggestionGroups,
canSelectMultipleValues,
filterValue,
selectedValues,
initialSelectedValues,
]);

// Flat list used for state management
const items = useMemo(() => {
Expand All @@ -479,6 +520,7 @@ function useFilterSuggestions({
items,
suggestionSectionItems,
isFetching: isFetching || isDebouncing,
suggestionGroups,
};
}

Expand Down Expand Up @@ -546,6 +588,7 @@ export function SearchQueryBuilderValueCombobox({
}: SearchQueryValueBuilderProps) {
const ref = useRef<HTMLDivElement>(null);
const inputRef = useRef<HTMLInputElement>(null);
const comboboxStateRef = useRef<any>(null);
const organization = useOrganization();
const {
getFieldDefinition,
Expand Down Expand Up @@ -613,12 +656,50 @@ export function SearchQueryBuilderValueCombobox({
}
}, []);

const {items, suggestionSectionItems, isFetching} = useFilterSuggestions({
token,
filterValue,
selectedValues: selectedValuesUnescaped,
ctrlKeyPressed,
});
// Track the initial selected values when the combobox is mounted to preserve list order during selection
const initialSelectedValues = useRef(selectedValuesUnescaped);
const prevInputValue = useRef(inputValue);

const {items, suggestionSectionItems, isFetching, suggestionGroups} =
useFilterSuggestions({
token,
filterValue,
selectedValues: selectedValuesUnescaped,
initialSelectedValues: initialSelectedValues.current,
ctrlKeyPressed,
});

// Update initialSelectedValues only for custom typed values (not existing suggestions)
useEffect(() => {
if (!canSelectMultipleValues) {
return;
}

// Only process if inputValue actually changed
if (prevInputValue.current === inputValue) {
return;
}
prevInputValue.current = inputValue;

// Get all values that exist in suggestions
const allSuggestionValues = new Set(
suggestionGroups.flatMap(group => group.suggestions).map(s => s.value)
);

// Find new selected values that are NOT in suggestions (i.e., custom typed values)
const currentInitialSet = new Set(initialSelectedValues.current.map(v => v.value));
const newCustomValues = selectedValuesUnescaped.filter(
v =>
v.selected && !currentInitialSet.has(v.value) && !allSuggestionValues.has(v.value)
);

if (newCustomValues.length > 0) {
initialSelectedValues.current = [
...initialSelectedValues.current,
...newCustomValues,
];
}
}, [inputValue, canSelectMultipleValues, selectedValuesUnescaped, suggestionGroups]);

const analyticsData = useMemo(
() => ({
Expand Down Expand Up @@ -933,14 +1014,30 @@ export function SearchQueryBuilderValueCombobox({
token={token}
inputLabel={t('Edit filter value')}
onInputChange={e => setInputValue(e.target.value)}
onKeyDown={onKeyDown}
onKeyDown={(e, {state}) => {
comboboxStateRef.current = state;
onKeyDown(e);
}}
onKeyUp={updateSelectionIndex}
onClick={updateSelectionIndex}
onClick={() => {
updateSelectionIndex();
// Also capture state on click to ensure it's available for mouse selections
if (inputRef.current) {
inputRef.current.focus();
}
}}
onOpenChange={(_isOpen, state) => {
// Capture the combobox state so we can restore focus after selections
if (state) {
comboboxStateRef.current = state;
}
}}
autoFocus
maxOptions={50}
openOnFocus
customMenu={customMenu}
shouldCloseOnInteractOutside={shouldCloseOnInteractOutside}
preserveFocusOnInputChange={canSelectMultipleValues}
>
{suggestionSectionItems.map(section => (
<Section key={section.sectionText} title={section.sectionText}>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
import {Fragment, useCallback} from 'react';
import {Fragment, useCallback, useEffect, useLayoutEffect, useRef} from 'react';
import {createPortal} from 'react-dom';
import styled from '@emotion/styled';
import {getItemId} from '@react-aria/listbox';
import {isMac} from '@react-aria/utils';
import type {Key} from '@react-types/shared';

import {ListBox} from 'sentry/components/core/compactSelect/listBox';
import type {SelectOptionOrSectionWithKey} from 'sentry/components/core/compactSelect/types';
Expand Down Expand Up @@ -118,6 +120,46 @@ export function ValueListBox<T extends SelectOptionOrSectionWithKey<string>>({
token,
wrapperRef,
}: ValueListBoxProps<T>) {
// Track and restore focused option if react-aria clears it during multi-select updates
const lastFocusedKeyRef = useRef<Key | null>(null);

const centerKeyInView = useCallback(
(key: Key | null) => {
if (key === null) return;
const container = listBoxRef.current;
if (!container) return;
const elId = getItemId(state as any, key as any);
if (!elId) return;
const el = document.getElementById(elId);
if (!el) return;

const containerRect = container.getBoundingClientRect();
const elRect = el.getBoundingClientRect();
const offsetTopWithinContainer =
container.scrollTop + (elRect.top - containerRect.top);
const targetScrollTop =
offsetTopWithinContainer - container.clientHeight / 2 + elRect.height / 2;

container.scrollTop = Math.max(0, targetScrollTop);
},
[listBoxRef, state]
);

useEffect(() => {
lastFocusedKeyRef.current = state.selectionManager.focusedKey;
}, [state.selectionManager.focusedKey]);

useLayoutEffect(() => {
if (!isOpen) return;
if (
lastFocusedKeyRef.current !== null &&
state.selectionManager.focusedKey === null
) {
const keyToRestore = lastFocusedKeyRef.current;
state.selectionManager.setFocusedKey(keyToRestore);
centerKeyInView(keyToRestore);
}
}, [isOpen, state, centerKeyInView]);
const totalOptions = items.reduce(
(acc, item) => acc + (itemIsSection(item) ? item.options.length : 1),
0
Expand Down