From 627d4b48ca7251b692f134d4c612d234bda044e9 Mon Sep 17 00:00:00 2001 From: Malachi Willey Date: Tue, 21 May 2024 08:22:35 -0700 Subject: [PATCH 1/3] feat(query-builder): Add basic support for selecting multiple values --- .../components/searchQueryBuilder/filter.tsx | 2 +- .../searchQueryBuilder/index.spec.tsx | 76 ++++-- .../useQueryBuilderState.tsx | 85 ++++++- .../searchQueryBuilder/valueCombobox.tsx | 239 +++++++++++++++--- 4 files changed, 339 insertions(+), 63 deletions(-) diff --git a/static/app/components/searchQueryBuilder/filter.tsx b/static/app/components/searchQueryBuilder/filter.tsx index 64bc418c32f00c..05e5750b9f3642 100644 --- a/static/app/components/searchQueryBuilder/filter.tsx +++ b/static/app/components/searchQueryBuilder/filter.tsx @@ -138,7 +138,7 @@ function FilterValue({token, state, item}: SearchQueryTokenProps) { { + onCommit={() => { setIsEditing(false); if (state.collection.getKeyAfter(item.key)) { state.selectionManager.setFocusedKey( diff --git a/static/app/components/searchQueryBuilder/index.spec.tsx b/static/app/components/searchQueryBuilder/index.spec.tsx index 9247355d830fc1..4935f9bee8b790 100644 --- a/static/app/components/searchQueryBuilder/index.spec.tsx +++ b/static/app/components/searchQueryBuilder/index.spec.tsx @@ -7,7 +7,12 @@ import type {TagCollection} from 'sentry/types/group'; import {FieldKey, FieldKind} from 'sentry/utils/fields'; const MOCK_SUPPORTED_KEYS: TagCollection = { - [FieldKey.AGE]: {key: FieldKey.AGE, name: 'Age', kind: FieldKind.FIELD}, + [FieldKey.AGE]: { + key: FieldKey.AGE, + name: 'Age', + kind: FieldKind.FIELD, + predefined: true, + }, [FieldKey.ASSIGNED]: { key: FieldKey.ASSIGNED, name: 'Assigned To', @@ -95,55 +100,88 @@ describe('SearchQueryBuilder', function () { ).toBeInTheDocument(); }); - it('can modify the value by clicking into it', async function () { - render( - - ); + it('can modify the value by clicking into it (single-select)', async function () { + // `age` is a duration filter which only accepts single values + render(); - // Should display as "firefox" to start + // Should display as "-1d" to start expect( within( - screen.getByRole('button', {name: 'Edit value for filter: browser.name'}) - ).getByText('firefox') + screen.getByRole('button', {name: 'Edit value for filter: age'}) + ).getByText('-1d') ).toBeInTheDocument(); await userEvent.click( - screen.getByRole('button', {name: 'Edit value for filter: browser.name'}) + screen.getByRole('button', {name: 'Edit value for filter: age'}) ); // Should have placeholder text of previous value expect(screen.getByRole('combobox', {name: 'Edit filter value'})).toHaveAttribute( 'placeholder', - 'firefox' + '-1d' ); await userEvent.click(screen.getByRole('combobox', {name: 'Edit filter value'})); - // Clicking the "Chrome option should update the value" - await userEvent.click(screen.getByRole('option', {name: 'Chrome'})); - expect(screen.getByRole('row', {name: 'browser.name:Chrome'})).toBeInTheDocument(); + // Clicking the "+14d" option should update the value + await userEvent.click(screen.getByRole('option', {name: '+14d'})); + expect(screen.getByRole('row', {name: 'age:+14d'})).toBeInTheDocument(); expect( within( - screen.getByRole('button', {name: 'Edit value for filter: browser.name'}) - ).getByText('Chrome') + screen.getByRole('button', {name: 'Edit value for filter: age'}) + ).getByText('+14d') ).toBeInTheDocument(); }); - it('escapes values with spaces and reserved characters', async function () { + it('can modify the value by clicking into it (multi-select)', async function () { + // `browser.name` is a string filter which accepts multiple values render( ); + // Should display as "firefox" to start + expect( + within( + screen.getByRole('button', {name: 'Edit value for filter: browser.name'}) + ).getByText('firefox') + ).toBeInTheDocument(); + await userEvent.click( screen.getByRole('button', {name: 'Edit value for filter: browser.name'}) ); - await userEvent.keyboard('some" value{enter}'); + // Previous value should be rendered before the input + expect( + within(screen.getByRole('row', {name: 'browser.name:firefox'})).getByText( + 'firefox,' + ) + ).toBeInTheDocument(); + await userEvent.click(screen.getByRole('combobox', {name: 'Edit filter value'})); + + // Clicking the "Chrome option should add it to the list and commit changes + await userEvent.click(screen.getByRole('option', {name: 'Chrome'})); + expect( + screen.getByRole('row', {name: 'browser.name:[firefox,Chrome]'}) + ).toBeInTheDocument(); + expect( + within( + screen.getByRole('button', {name: 'Edit value for filter: browser.name'}) + ).getByText('[firefox,Chrome]') + ).toBeInTheDocument(); + }); + + it('escapes values with spaces and reserved characters', async function () { + render(); + await userEvent.click(screen.getByRole('combobox', {name: 'Add a search term'})); + await userEvent.type( + screen.getByRole('combobox', {name: 'Add a search term'}), + 'assigned:some" value{enter}' + ); // Value should be surrounded by quotes and escaped expect( - screen.getByRole('row', {name: 'browser.name:"some\\" value"'}) + screen.getByRole('row', {name: 'assigned:"some\\" value"'}) ).toBeInTheDocument(); // Display text should be display the original value expect( within( - screen.getByRole('button', {name: 'Edit value for filter: browser.name'}) + screen.getByRole('button', {name: 'Edit value for filter: assigned'}) ).getByText('some" value') ).toBeInTheDocument(); }); diff --git a/static/app/components/searchQueryBuilder/useQueryBuilderState.tsx b/static/app/components/searchQueryBuilder/useQueryBuilderState.tsx index a570801d384877..0ff4aa6294e814 100644 --- a/static/app/components/searchQueryBuilder/useQueryBuilderState.tsx +++ b/static/app/components/searchQueryBuilder/useQueryBuilderState.tsx @@ -3,7 +3,7 @@ import {type Reducer, useCallback, useReducer} from 'react'; import { type ParseResultToken, TermOperator, - type Token, + Token, type TokenResult, } from 'sentry/components/searchSyntax/parser'; import {stringifyToken} from 'sentry/components/searchSyntax/utils'; @@ -35,11 +35,24 @@ type UpdateTokenValueAction = { value: string; }; +type MultiSelectFilterValueAction = { + token: TokenResult; + type: 'TOGGLE_FILTER_VALUE'; + value: string; +}; + +type DeleteLastMultiSelectFilterValueAction = { + token: TokenResult; + type: 'DELETE_LAST_MULTI_SELECT_FILTER_VALUE'; +}; + export type QueryBuilderActions = | DeleteTokenAction | UpdateFreeTextAction | UpdateFilterOpAction - | UpdateTokenValueAction; + | UpdateTokenValueAction + | MultiSelectFilterValueAction + | DeleteLastMultiSelectFilterValueAction; function removeQueryToken(query: string, token: TokenResult): string { return ( @@ -101,6 +114,70 @@ function updateFreeText( }; } +function updateFilterMultipleValues( + state: QueryBuilderState, + token: TokenResult, + values: string[] +) { + const uniqNonEmptyValues = Array.from( + new Set(values.filter(value => value.length > 0)) + ); + if (uniqNonEmptyValues.length === 0) { + return {...state, query: replaceQueryToken(state.query, token.value, '')}; + } + + const newValue = + uniqNonEmptyValues.length > 1 + ? `[${uniqNonEmptyValues.join(',')}]` + : uniqNonEmptyValues[0]; + + return {...state, query: replaceQueryToken(state.query, token.value, newValue)}; +} + +function mutliSelectTokenValue( + state: QueryBuilderState, + action: MultiSelectFilterValueAction +) { + const tokenValue = action.token.value; + + switch (tokenValue.type) { + case Token.VALUE_TEXT_LIST: + case Token.VALUE_NUMBER_LIST: + const values = tokenValue.items.map(item => item.value?.text ?? ''); + const containsValue = values.includes(action.value); + const newValues = containsValue + ? values.filter(value => value !== action.value) + : [...values, action.value]; + + return updateFilterMultipleValues(state, action.token, newValues); + default: + if (tokenValue.text === action.value) { + return updateFilterMultipleValues(state, action.token, ['']); + } + return updateFilterMultipleValues(state, action.token, [ + tokenValue.text, + action.value, + ]); + } +} + +function deleteLastMutliSelectTokenValue( + state: QueryBuilderState, + action: DeleteLastMultiSelectFilterValueAction +) { + const tokenValue = action.token.value; + + switch (tokenValue.type) { + case Token.VALUE_TEXT_LIST: + case Token.VALUE_NUMBER_LIST: + const newValues = tokenValue.items.slice(0, -1).map(item => item.value?.text ?? ''); + + return updateFilterMultipleValues(state, action.token, newValues); + default: + return updateFilterMultipleValues(state, action.token, ['']); + } +} + export function useQueryBuilderState({initialQuery}: {initialQuery: string}) { const initialState: QueryBuilderState = {query: initialQuery}; @@ -121,6 +198,10 @@ export function useQueryBuilderState({initialQuery}: {initialQuery: string}) { return { query: replaceQueryToken(state.query, action.token, action.value), }; + case 'TOGGLE_FILTER_VALUE': + return mutliSelectTokenValue(state, action); + case 'DELETE_LAST_MULTI_SELECT_FILTER_VALUE': + return deleteLastMutliSelectTokenValue(state, action); default: return state; } diff --git a/static/app/components/searchQueryBuilder/valueCombobox.tsx b/static/app/components/searchQueryBuilder/valueCombobox.tsx index 6fa0a6ca89089a..2945641ec09872 100644 --- a/static/app/components/searchQueryBuilder/valueCombobox.tsx +++ b/static/app/components/searchQueryBuilder/valueCombobox.tsx @@ -1,22 +1,27 @@ import {useCallback, useMemo, useState} from 'react'; +import styled from '@emotion/styled'; import {Item, Section} from '@react-stately/collections'; +import orderBy from 'lodash/orderBy'; +import Checkbox from 'sentry/components/checkbox'; import {getItemsWithKeys} from 'sentry/components/compactSelect/utils'; import {SearchQueryBuilderCombobox} from 'sentry/components/searchQueryBuilder/combobox'; import {useSearchQueryBuilder} from 'sentry/components/searchQueryBuilder/context'; import { escapeTagValue, formatFilterValue, + unescapeTagValue, } from 'sentry/components/searchQueryBuilder/utils'; -import type {Token, TokenResult} from 'sentry/components/searchSyntax/parser'; +import {Token, type TokenResult} from 'sentry/components/searchSyntax/parser'; import type {SearchGroup} from 'sentry/components/smartSearchBar/types'; import {t} from 'sentry/locale'; +import {space} from 'sentry/styles/space'; import type {Tag} from 'sentry/types'; import {FieldValueType, getFieldDefinition} from 'sentry/utils/fields'; import {type QueryKey, useQuery} from 'sentry/utils/queryClient'; type SearchQueryValueBuilderProps = { - onChange: () => void; + onCommit: () => void; token: TokenResult; }; @@ -53,15 +58,89 @@ function getPredefinedValues({key}: {key?: Tag}): string[] { } } +function keySupportsMultipleValues(key: Tag): boolean { + const fieldDef = getFieldDefinition(key.key); + + switch (fieldDef?.valueType) { + case FieldValueType.STRING: + case FieldValueType.NUMBER: + case FieldValueType.INTEGER: + return true; + default: + return false; + } +} + +function getOtherSelectedValues(token: TokenResult): string[] { + switch (token.value.type) { + case Token.VALUE_TEXT: + if (!token.value.value) { + return []; + } + return [unescapeTagValue(token.value.value)]; + case Token.VALUE_NUMBER_LIST: + return token.value.items.map(item => item.value?.text ?? ''); + case Token.VALUE_TEXT_LIST: + return token.value.items.map(item => unescapeTagValue(item.value?.value ?? '')); + default: + return []; + } +} + +function ItemCheckbox({ + token, + isFocused, + selected, + disabled, + value, +}: { + disabled: boolean; + isFocused: boolean; + selected: boolean; + token: TokenResult; + value: string; +}) { + const {dispatch} = useSearchQueryBuilder(); + + return ( + e.stopPropagation()}> + + { + dispatch({ + type: 'TOGGLE_FILTER_VALUE', + token: token, + value: escapeTagValue(value), + }); + }} + aria-label={t('Select %s', value)} + tabIndex={-1} + /> + + + ); +} + export function SearchQueryBuilderValueCombobox({ token, - onChange, + onCommit, }: SearchQueryValueBuilderProps) { const [inputValue, setInputValue] = useState(''); const {getTagValues, keys, dispatch} = useSearchQueryBuilder(); const key = keys[token.key.text]; const shouldFetchValues = key && !key.predefined; + const canSelectMultipleValues = keySupportsMultipleValues(key); + const selectedValues = useMemo( + () => + canSelectMultipleValues + ? orderBy(getOtherSelectedValues(token), 'value', 'asc') + : [], + [canSelectMultipleValues, token] + ); // TODO(malwilley): Display error states const {data} = useQuery({ @@ -71,53 +150,131 @@ export function SearchQueryBuilderValueCombobox({ enabled: shouldFetchValues, }); + const createItem = useCallback( + (value: string, selected = false) => { + return { + label: value, + value: value, + textValue: value, + hideCheck: true, + selectionMode: canSelectMultipleValues ? 'multiple' : 'single', + trailingItems: ({isFocused, disabled}) => { + if (!canSelectMultipleValues) { + return null; + } + + return ( + + ); + }, + }; + }, + [canSelectMultipleValues, token] + ); + const items = useMemo(() => { const values = (shouldFetchValues ? data : getPredefinedValues({key})) ?? []; - return getItemsWithKeys( - values.map(value => { - return { - label: value, - value: value, - textValue: value, - hideCheck: true, - }; - }) - ); - }, [data, key, shouldFetchValues]); + return getItemsWithKeys([ + ...selectedValues.map(value => createItem(value, true)), + ...values + .filter(value => !selectedValues.includes(value)) + .map(value => createItem(value)), + ]); + }, [createItem, data, key, selectedValues, shouldFetchValues]); const handleSelectValue = useCallback( (value: string) => { - dispatch({ - type: 'UPDATE_TOKEN_VALUE', - token: token.value, - value: escapeTagValue(value), - }); - onChange(); + if (canSelectMultipleValues) { + dispatch({ + type: 'TOGGLE_FILTER_VALUE', + token: token, + value: escapeTagValue(value), + }); + + // If toggling off a value, keep focus inside the value + if (!selectedValues.includes(value)) { + onCommit(); + } + } else { + dispatch({ + type: 'UPDATE_TOKEN_VALUE', + token: token.value, + value: escapeTagValue(value), + }); + onCommit(); + } + }, + [canSelectMultipleValues, dispatch, onCommit, selectedValues, token] + ); + + const onKeyDown = useCallback( + (e: React.KeyboardEvent) => { + // If at the start of the input and backspace is pressed, delete the last selected value + if ( + e.key === 'Backspace' && + e.currentTarget.selectionStart === 0 && + e.currentTarget.selectionEnd === 0 + ) { + dispatch({type: 'DELETE_LAST_MULTI_SELECT_FILTER_VALUE', token}); + } }, - [dispatch, onChange, token.value] + [dispatch, token] ); return ( - // TODO(malwilley): Support for multiple values - setInputValue(e.target.value)} - autoFocus - > -
- {items.map(item => ( - - {item.label} - - ))} -
- + + {selectedValues.map(value => ( + {value}, + ))} + setInputValue(e.target.value)} + onKeyDown={onKeyDown} + autoFocus + > +
+ {items.map(item => ( + + {item.label} + + ))} +
+ + ); } + +const ValueEditing = styled('div')` + display: flex; + height: 100%; + align-items: center; + gap: ${space(0.25)}; +`; + +const TrailingWrap = styled('div')` + display: grid; + grid-auto-flow: column; + align-items: center; + gap: ${space(1)}; +`; + +const CheckWrap = styled('div')<{visible: boolean}>` + display: flex; + justify-content: center; + align-items: center; + opacity: ${p => (p.visible ? 1 : 0)}; + padding: ${space(0.25)} 0 ${space(0.25)} ${space(0.25)}; +`; From 8b7bb097ec2701e90336ab674c580d226519132e Mon Sep 17 00:00:00 2001 From: Malachi Willey Date: Thu, 23 May 2024 11:58:08 -0700 Subject: [PATCH 2/3] Fix typo --- .../components/searchQueryBuilder/useQueryBuilderState.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/static/app/components/searchQueryBuilder/useQueryBuilderState.tsx b/static/app/components/searchQueryBuilder/useQueryBuilderState.tsx index 0ff4aa6294e814..6cd00cbb4a3b6c 100644 --- a/static/app/components/searchQueryBuilder/useQueryBuilderState.tsx +++ b/static/app/components/searchQueryBuilder/useQueryBuilderState.tsx @@ -161,7 +161,7 @@ function mutliSelectTokenValue( } } -function deleteLastMutliSelectTokenValue( +function deleteLastMultiSelectTokenValue( state: QueryBuilderState, action: DeleteLastMultiSelectFilterValueAction ) { @@ -201,7 +201,7 @@ export function useQueryBuilderState({initialQuery}: {initialQuery: string}) { case 'TOGGLE_FILTER_VALUE': return mutliSelectTokenValue(state, action); case 'DELETE_LAST_MULTI_SELECT_FILTER_VALUE': - return deleteLastMutliSelectTokenValue(state, action); + return deleteLastMultiSelectTokenValue(state, action); default: return state; } From 01ba07e9da2fbd47a9b0b989eaab6bc1368d80b8 Mon Sep 17 00:00:00 2001 From: Malachi Willey Date: Tue, 28 May 2024 09:19:23 -0700 Subject: [PATCH 3/3] Fix another typo --- .../components/searchQueryBuilder/useQueryBuilderState.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/static/app/components/searchQueryBuilder/useQueryBuilderState.tsx b/static/app/components/searchQueryBuilder/useQueryBuilderState.tsx index 6cd00cbb4a3b6c..288e667f32e56f 100644 --- a/static/app/components/searchQueryBuilder/useQueryBuilderState.tsx +++ b/static/app/components/searchQueryBuilder/useQueryBuilderState.tsx @@ -134,7 +134,7 @@ function updateFilterMultipleValues( return {...state, query: replaceQueryToken(state.query, token.value, newValue)}; } -function mutliSelectTokenValue( +function multiSelectTokenValue( state: QueryBuilderState, action: MultiSelectFilterValueAction ) { @@ -199,7 +199,7 @@ export function useQueryBuilderState({initialQuery}: {initialQuery: string}) { query: replaceQueryToken(state.query, action.token, action.value), }; case 'TOGGLE_FILTER_VALUE': - return mutliSelectTokenValue(state, action); + return multiSelectTokenValue(state, action); case 'DELETE_LAST_MULTI_SELECT_FILTER_VALUE': return deleteLastMultiSelectTokenValue(state, action); default: