diff --git a/static/app/components/searchQueryBuilder/context.tsx b/static/app/components/searchQueryBuilder/context.tsx index ecc2ab65564c..7b2da85e2683 100644 --- a/static/app/components/searchQueryBuilder/context.tsx +++ b/static/app/components/searchQueryBuilder/context.tsx @@ -50,6 +50,11 @@ interface SearchQueryBuilderStateContextData { handleSearch: (query: string) => void; parseQuery: (query: string) => ParseResult | null; parsedQuery: ParseResult | null; + /** + * Item key of a number-based filter that has no value yet. Its value in + * `query` is a placeholder and should be shown as empty. + */ + pendingFilterKey: string | null; query: string; } @@ -410,6 +415,7 @@ export function SearchQueryBuilderProvider({ handleSearch, parseQuery, parsedQuery, + pendingFilterKey: state.pendingFilterKey, query: state.query, }; }, [ @@ -420,6 +426,7 @@ export function SearchQueryBuilderProvider({ parsedQuery, state.committedQuery, state.focusOverride, + state.pendingFilterKey, state.query, ]); diff --git a/static/app/components/searchQueryBuilder/hooks/useQueryBuilderState.spec.tsx b/static/app/components/searchQueryBuilder/hooks/useQueryBuilderState.spec.tsx index 07f3636b44ff..100410e9387a 100644 --- a/static/app/components/searchQueryBuilder/hooks/useQueryBuilderState.spec.tsx +++ b/static/app/components/searchQueryBuilder/hooks/useQueryBuilderState.spec.tsx @@ -292,6 +292,7 @@ describe('multiSelectTokenValue', () => { committedQuery: query, focusOverride: null, clearAskSeerFeedback: false, + pendingFilterKey: null, }; return multiSelectTokenValue(state, {type: 'TOGGLE_FILTER_VALUE', token, value}); } @@ -396,6 +397,7 @@ describe('typed filter keys containing colons', () => { committedQuery: query, focusOverride: null, clearAskSeerFeedback: false, + pendingFilterKey: null, }; expect( @@ -563,6 +565,7 @@ describe('syntax-bearing filter keys', () => { committedQuery: query, focusOverride: null, clearAskSeerFeedback: false, + pendingFilterKey: null, }; expect( diff --git a/static/app/components/searchQueryBuilder/hooks/useQueryBuilderState.tsx b/static/app/components/searchQueryBuilder/hooks/useQueryBuilderState.tsx index a9d15e735374..cc0dc47a9930 100644 --- a/static/app/components/searchQueryBuilder/hooks/useQueryBuilderState.tsx +++ b/static/app/components/searchQueryBuilder/hooks/useQueryBuilderState.tsx @@ -14,7 +14,11 @@ import { type FieldDefinitionGetter, type FocusOverride, } from 'sentry/components/searchQueryBuilder/types'; -import {isDateToken, makeTokenKey} from 'sentry/components/searchQueryBuilder/utils'; +import { + isDateToken, + isNumericFilterToken, + makeTokenKey, +} from 'sentry/components/searchQueryBuilder/utils'; import { FilterType, TermOperator, @@ -54,6 +58,14 @@ type QueryBuilderState = { */ focusOverride: FocusOverride | null; + /** + * Item key of a number-based filter that was just added and has no value yet. + * The search grammars have no way to write an empty number filter, so `query` + * holds the filter with a placeholder value. The placeholder is only + * committed if the user leaves the filter without entering a value. + */ + pendingFilterKey: string | null; + /** * The current query value. * This is the basic source of truth for what is currently being displayed. @@ -77,6 +89,19 @@ type UpdateQueryAction = { type ResetFocusOverrideAction = {type: 'RESET_FOCUS_OVERRIDE'}; +type SetFocusOverrideAction = { + focusOverride: FocusOverride; + type: 'SET_FOCUS_OVERRIDE'; +}; + +type CommitPendingFilterAction = { + /** + * Only commit the filter if it is still the pending one. + */ + itemKey: string; + type: 'COMMIT_PENDING_FILTER'; +}; + type DeleteTokenAction = { token: ParseResultToken; type: 'DELETE_TOKEN'; @@ -191,6 +216,7 @@ type UpdateFilterKeyAction = { key: string; token: TokenResult; type: 'UPDATE_FILTER_KEY'; + shouldCommitQuery?: boolean; }; type UpdateFilterOpAction = { @@ -249,6 +275,8 @@ export type QueryBuilderActions = | CommitQueryAction | UpdateQueryAction | ResetFocusOverrideAction + | SetFocusOverrideAction + | CommitPendingFilterAction | DeleteTokenAction | DeleteTokensAction | DeleteToCursorAction @@ -928,7 +956,7 @@ function updateFilterKey( return { ...state, query: newQuery, - committedQuery: newQuery, + committedQuery: (action.shouldCommitQuery ?? true) ? newQuery : state.committedQuery, }; } @@ -1119,6 +1147,162 @@ function updateLogicOperator( }; } +function findPendingFilter( + query: string, + pendingFilterKey: string | null, + parseQuery: (query: string) => ParseResult | null +): TokenResult | null { + if (!pendingFilterKey) { + return null; + } + + const parsedQuery = parseQuery(query); + const token = parsedQuery?.find( + tk => makeTokenKey(tk, parsedQuery) === pendingFilterKey + ); + + return token?.type === Token.FILTER && isNumericFilterToken(token) ? token : null; +} + +/** + * Actions that add a filter with getInitialFilterText, which gives number + * filters a placeholder value. + */ +function isFilterCreationAction( + action: QueryBuilderActions +): action is + | UpdateFreeTextActionOnSelect + | UpdateFreeTextActionOnColon + | UpdateFreeTextActionOnFunction + | ReplaceTokensWithTextOnSelectAction { + return ( + action.type === 'UPDATE_FREE_TEXT_ON_SELECT' || + action.type === 'UPDATE_FREE_TEXT_ON_COLON' || + action.type === 'UPDATE_FREE_TEXT_ON_FUNCTION' || + action.type === 'REPLACE_TOKENS_WITH_TEXT_ON_SELECT' + ); +} + +function getActionTokens(action: QueryBuilderActions): ParseResultToken[] { + if ('tokens' in action) { + return action.tokens; + } + if ('token' in action) { + return [action.token]; + } + return []; +} + +/** + * Works out which filter, if any, is still pending after `action`. Item keys + * are positional, so when the action changed the query around the pending + * filter, find it again by the text before or after it. + */ +function getNextPendingFilterKey( + state: QueryBuilderState, + newState: QueryBuilderState, + action: QueryBuilderActions, + parseQuery: (query: string) => ParseResult | null +): string | null { + if ( + action.type === 'CLEAR' || + action.type === 'UPDATE_QUERY' || + (action.type === 'COMMIT_PENDING_FILTER' && action.itemKey === state.pendingFilterKey) + ) { + return null; + } + + if (isFilterCreationAction(action)) { + const itemKey = action.focusOverride?.itemKey; + if ( + itemKey?.startsWith(`${Token.FILTER}:`) && + findPendingFilter(newState.query, itemKey, parseQuery) + ) { + return itemKey; + } + } + + const pendingFilter = findPendingFilter( + state.query, + state.pendingFilterKey, + parseQuery + ); + if (!pendingFilter) { + return null; + } + + const {start, end} = pendingFilter.location; + const targetsPendingFilter = getActionTokens(action).some( + tk => tk.type === Token.FILTER && tk.location.start.offset === start.offset + ); + + if (targetsPendingFilter) { + switch (action.type) { + case 'UPDATE_TOKEN_VALUE': + case 'TOGGLE_FILTER_VALUE': + case 'DELETE_TOKEN': + case 'DELETE_TOKENS': + return null; + default: + // The pending filter itself was edited (key, operator or arguments), so + // no filters moved and its item key is unchanged. + return findPendingFilter(newState.query, state.pendingFilterKey, parseQuery) + ? state.pendingFilterKey + : null; + } + } + + let offset: number | null = null; + if (newState.query.startsWith(state.query.slice(0, end.offset))) { + offset = start.offset; + } else if (newState.query.endsWith(state.query.slice(start.offset))) { + offset = newState.query.length - (state.query.length - start.offset); + } + + const parsedQuery = parseQuery(newState.query); + const token = parsedQuery?.find( + tk => tk.type === Token.FILTER && tk.location.start.offset === offset + ); + + return token?.type === Token.FILTER && isNumericFilterToken(token) + ? makeTokenKey(token, parsedQuery) + : null; +} + +/** + * Keeps the pending filter's placeholder value out of `committedQuery`. + */ +function updatePendingFilter( + state: QueryBuilderState, + newState: QueryBuilderState, + action: QueryBuilderActions, + parseQuery: (query: string) => ParseResult | null +): QueryBuilderState { + if (!state.pendingFilterKey && !isFilterCreationAction(action)) { + return newState; + } + + const pendingFilterKey = getNextPendingFilterKey(state, newState, action, parseQuery); + const pendingFilter = findPendingFilter(newState.query, pendingFilterKey, parseQuery); + + if (!pendingFilter) { + return newState.pendingFilterKey === null + ? newState + : {...newState, pendingFilterKey: null}; + } + + const isCommitting = + action.type === 'COMMIT_QUERY' || newState.committedQuery !== state.committedQuery; + + return { + ...newState, + pendingFilterKey, + committedQuery: isCommitting + ? removeQueryTokensFromQuery(newState.query, [pendingFilter]) + : state.committedQuery, + }; +} + export function useQueryBuilderState({ disabled, displayAskSeerFeedback, @@ -1143,9 +1327,10 @@ export function useQueryBuilderState({ committedQuery: initialQuery, focusOverride: null, clearAskSeerFeedback: false, + pendingFilterKey: null, }; - const reducer: Reducer = useCallback( + const baseReducer: Reducer = useCallback( (state, action): QueryBuilderState => { if (disabled && !(action.type === 'UPDATE_QUERY' && action.ignoreDisabled)) { return state; @@ -1207,6 +1392,16 @@ export function useQueryBuilderState({ ...state, focusOverride: null, }; + case 'SET_FOCUS_OVERRIDE': + return { + ...state, + focusOverride: action.focusOverride, + }; + case 'COMMIT_PENDING_FILTER': + if (action.itemKey !== state.pendingFilterKey) { + return state; + } + return {...state, committedQuery: state.query}; case 'DELETE_TOKEN': { return { ...replaceTokensWithText(state, { @@ -1320,6 +1515,12 @@ export function useQueryBuilderState({ ] ); + const reducer: Reducer = useCallback( + (state, action) => + updatePendingFilter(state, baseReducer(state, action), action, parseQuery), + [baseReducer, parseQuery] + ); + const [state, dispatch] = useReducer(reducer, initialState); useEffect(() => { diff --git a/static/app/components/searchQueryBuilder/hooks/useUndoStack.tsx b/static/app/components/searchQueryBuilder/hooks/useUndoStack.tsx index c13c143ce625..e06491f07b61 100644 --- a/static/app/components/searchQueryBuilder/hooks/useUndoStack.tsx +++ b/static/app/components/searchQueryBuilder/hooks/useUndoStack.tsx @@ -77,17 +77,20 @@ function updateUndoStack({ * Hook that manages the undo stack for the search query builder. */ export function useUndoStack(state: ListState) { - const {query, focusOverride, dispatch} = useSearchQueryBuilderState(); + const {query, focusOverride, dispatch, pendingFilterKey} = useSearchQueryBuilderState(); const undoStackRef = useRef([]); const trimmedQuery = query.trim(); - updateUndoStack({ - // oxlint-disable-next-line react/refs - undoStack: undoStackRef.current, - query: trimmedQuery, - focusOverride, - state, - }); + // A pending filter's placeholder value must not come back as a real value + if (!pendingFilterKey) { + updateUndoStack({ + // oxlint-disable-next-line react/refs + undoStack: undoStackRef.current, + query: trimmedQuery, + focusOverride, + state, + }); + } const undo = useCallback(() => { const previousItem = getPreviousUndoItem(undoStackRef.current, trimmedQuery); diff --git a/static/app/components/searchQueryBuilder/index.spec.tsx b/static/app/components/searchQueryBuilder/index.spec.tsx index e3aef972af97..fa519376e8d4 100644 --- a/static/app/components/searchQueryBuilder/index.spec.tsx +++ b/static/app/components/searchQueryBuilder/index.spec.tsx @@ -2037,7 +2037,7 @@ describe('SearchQueryBuilder', () => { it.each([ ['!has:', '!has:""'], - ['!timesSeen:', '!timesSeen:100'], + ['!timesSeen:', '!timesSeen:'], ])('uses the filter key definition when typing %s', async (typed, filter) => { render(); await userEvent.click(getLastInput()); @@ -5224,15 +5224,15 @@ describe('SearchQueryBuilder', () => { }); describe('numeric', () => { - it('new numeric filters start with a value', async () => { + it('new numeric filters start with greater than operator and no value', async () => { render(); await userEvent.click(getLastInput()); await userEvent.keyboard('time{ArrowDown}{Enter}'); - // Should start with the > operator and a value of 100 - expect( - await screen.findByRole('row', {name: 'timesSeen:>100'}) - ).toBeInTheDocument(); + expect(await screen.findByRole('row', {name: 'timesSeen:>'})).toBeInTheDocument(); + const input = screen.getByRole('combobox', {name: 'Edit filter value'}); + expect(input).toHaveValue(''); + expect(input).toHaveAttribute('placeholder', '100'); }); it('pre-fills input with current value when editing', async () => { @@ -5324,15 +5324,436 @@ describe('SearchQueryBuilder', () => { fieldDefinitionGetter, }; - it('new duration filters start with greater than operator and default value', async () => { + it('new duration filters start with greater than operator and no value', async () => { render(); await userEvent.click(getLastInput()); await userEvent.click(screen.getByRole('option', {name: 'duration'})); - // Should start with the > operator and a value of 10ms - expect( - await screen.findByRole('row', {name: 'duration:>10ms'}) - ).toBeInTheDocument(); + expect(await screen.findByRole('row', {name: 'duration:>'})).toBeInTheDocument(); + expect(screen.getByRole('combobox', {name: 'Edit filter value'})).toHaveValue(''); + }); + + describe('new filter without a value', () => { + async function addDurationFilter() { + await userEvent.click(getLastInput()); + await userEvent.click(screen.getByRole('option', {name: 'duration'})); + await screen.findByRole('row', {name: 'duration:>'}); + } + + it('does not change the query until a value is entered', async () => { + const mockOnChange = jest.fn(); + render( + + ); + await addDurationFilter(); + expect(mockOnChange).not.toHaveBeenCalled(); + + await userEvent.keyboard('50ms{Enter}'); + + expect( + await screen.findByRole('row', {name: 'duration:>50ms'}) + ).toBeInTheDocument(); + expect(mockOnChange).toHaveBeenCalledTimes(1); + expect(mockOnChange).toHaveBeenCalledWith( + 'browser.name:firefox duration:>50ms', + expect.anything() + ); + }); + + it('keeps the filter while the operator is changed', async () => { + const mockOnChange = jest.fn(); + render(); + await addDurationFilter(); + + // The open value menu hides the rest of the filter from assistive tech + await userEvent.click( + screen.getByRole('button', { + name: 'Edit operator for filter: duration', + hidden: true, + }) + ); + await userEvent.click(await screen.findByRole('option', {name: '<'})); + + expect( + await screen.findByRole('row', {name: 'duration:<'}) + ).toBeInTheDocument(); + expect(screen.getByRole('combobox', {name: 'Edit filter value'})).toHaveFocus(); + expect(mockOnChange).not.toHaveBeenCalled(); + + await userEvent.keyboard('5ms{Enter}'); + + expect(mockOnChange).toHaveBeenCalledTimes(1); + expect(mockOnChange).toHaveBeenCalledWith('duration:<5ms', expect.anything()); + }); + + it.each([ + ['escape', '{Escape}'], + ['enter', '{Enter}'], + ])( + 'keeps the placeholder value when %s is pressed without a value', + async (_, key) => { + const mockOnChange = jest.fn(); + render(); + await addDurationFilter(); + + await userEvent.keyboard(key); + + expect( + await screen.findByRole('row', {name: 'duration:>10ms'}) + ).toBeInTheDocument(); + expect(mockOnChange).toHaveBeenCalledTimes(1); + expect(mockOnChange).toHaveBeenCalledWith( + 'duration:>10ms', + expect.anything() + ); + } + ); + + it('skips the filter without a value on undo', async () => { + render(); + await addDurationFilter(); + await userEvent.keyboard('50ms{Enter}'); + expect( + await screen.findByRole('row', {name: 'duration:>50ms'}) + ).toBeInTheDocument(); + + await userEvent.keyboard('{Control>}z{/Control}'); + + await waitFor(() => { + expect( + screen.queryByRole('row', {name: 'duration:>50ms'}) + ).not.toBeInTheDocument(); + }); + expect(screen.queryByRole('row', {name: /^duration:/})).not.toBeInTheDocument(); + }); + + it('keeps the placeholder value when focus leaves the filter', async () => { + const mockOnChange = jest.fn(); + render(); + await addDurationFilter(); + + await userEvent.click(document.body); + + expect( + await screen.findByRole('row', {name: 'duration:>10ms'}) + ).toBeInTheDocument(); + expect(mockOnChange).toHaveBeenCalledTimes(1); + expect(mockOnChange).toHaveBeenCalledWith('duration:>10ms', expect.anything()); + }); + + it.each([ + ['selected', addDurationFilter], + [ + 'typed', + async () => { + await userEvent.click(getLastInput()); + await userEvent.keyboard('duration:>'); + await screen.findByRole('row', {name: 'duration:>'}); + }, + ], + ])( + 'removes a %s filter when its remove button is pressed', + async (_, addFilter) => { + const mockOnChange = jest.fn(); + const user = userEvent.setup(); + render(); + await addFilter(); + + // Once the value editor closes, the click can miss the button, so + // release the pointer somewhere else. + await user.pointer([ + { + keys: '[MouseLeft>]', + target: screen.getByRole('button', { + name: 'Remove filter: duration', + hidden: true, + }), + }, + {keys: '[/MouseLeft]', target: document.body}, + ]); + + await waitFor(() => { + expect( + screen.queryByRole('row', {name: /^duration:/}) + ).not.toBeInTheDocument(); + }); + expect(getLastInput()).toHaveFocus(); + expect(mockOnChange).not.toHaveBeenCalled(); + } + ); + + it.each([ + ['', []], + ['browser.name:firefox', ['']], + ])( + 'clears the query "%s" without committing the placeholder value', + async (initialQuery, expectedChanges) => { + const mockOnChange = jest.fn(); + const user = userEvent.setup(); + render( + + ); + await addDurationFilter(); + + const clearButton = screen.getByRole('button', { + name: 'Clear search query', + hidden: true, + }); + // Hold the press long enough for focus to settle outside the filter + await user.pointer({keys: '[MouseLeft>]', target: clearButton}); + await act(() => new Promise(resolve => setTimeout(resolve, 10))); + await user.pointer({keys: '[/MouseLeft]', target: clearButton}); + + await waitFor(() => { + expect( + screen.queryByRole('row', {name: /^duration:/}) + ).not.toBeInTheDocument(); + }); + expect(getLastInput()).toHaveFocus(); + expect(mockOnChange.mock.calls.map(([query]) => query)).toEqual( + expectedChanges + ); + } + ); + + it('does not keep the placeholder value when the operator is changed to "is"', async () => { + const mockOnChange = jest.fn(); + render( + ({ + valueType: FieldValueType.NUMBER, + kind: FieldKind.FIELD, + })} + onChange={mockOnChange} + /> + ); + await userEvent.click(getLastInput()); + await userEvent.click(screen.getByRole('option', {name: 'count'})); + await screen.findByRole('row', {name: 'count:>'}); + + await userEvent.click( + screen.getByRole('button', { + name: 'Edit operator for filter: count', + hidden: true, + }) + ); + await userEvent.click(await screen.findByRole('option', {name: 'is'})); + await userEvent.keyboard('5{Enter}'); + + expect(await screen.findByRole('row', {name: 'count:5'})).toBeInTheDocument(); + expect(mockOnChange).toHaveBeenLastCalledWith('count:5', expect.anything()); + }); + + it('only removes the filter without a value when it is before another filter', async () => { + const mockOnChange = jest.fn(); + render( + + ); + await userEvent.click( + screen.getAllByRole('combobox', {name: 'Add a search term'})[0]! + ); + await userEvent.click(screen.getByRole('option', {name: 'duration'})); + await screen.findByRole('row', {name: 'duration:>'}); + + await userEvent.click( + screen.getByRole('button', {name: 'Remove filter: duration', hidden: true}) + ); + + await waitFor(() => { + expect( + screen.queryByRole('row', {name: /^duration:/}) + ).not.toBeInTheDocument(); + }); + expect( + screen.getByRole('row', {name: 'browser.name:firefox'}) + ).toBeInTheDocument(); + expect(mockOnChange).not.toHaveBeenCalled(); + }); + + describe('when changing the key', () => { + const keyChangeProps: SearchQueryBuilderProps = { + ...defaultProps, + filterKeys: { + 'browser.name': {key: 'browser.name', name: 'browser.name'}, + device: {key: 'device', name: 'device'}, + countOne: {key: 'countOne', name: 'countOne'}, + countTwo: {key: 'countTwo', name: 'countTwo'}, + }, + filterKeySections: [], + fieldDefinitionGetter: key => + key.startsWith('count') + ? {valueType: FieldValueType.NUMBER, kind: FieldKind.FIELD} + : {valueType: FieldValueType.STRING, kind: FieldKind.TAG}, + }; + + async function changeKey(from: string, to: string) { + await userEvent.click( + screen.getByRole('button', { + name: `Edit key for filter: ${from}`, + hidden: true, + }) + ); + await userEvent.clear( + screen.getByRole('combobox', {name: 'Edit filter key'}) + ); + await userEvent.click(screen.getByRole('option', {name: to})); + } + + it('keeps the filter without a value when its key is changed', async () => { + const mockOnChange = jest.fn(); + const mockOnSearch = jest.fn(); + render( + + ); + await userEvent.click(getLastInput()); + await userEvent.click(screen.getByRole('option', {name: 'countOne'})); + await screen.findByRole('row', {name: 'countOne:>'}); + + await changeKey('countOne', 'countTwo'); + + expect( + await screen.findByRole('row', {name: 'countTwo:>'}) + ).toBeInTheDocument(); + await waitFor(() => { + expect( + screen.getByRole('combobox', {name: 'Edit filter value'}) + ).toHaveFocus(); + }); + expect(mockOnChange).not.toHaveBeenCalled(); + expect(mockOnSearch).not.toHaveBeenCalled(); + + await userEvent.keyboard('5{Enter}'); + + expect(mockOnChange).toHaveBeenCalledTimes(1); + expect(mockOnChange).toHaveBeenCalledWith('countTwo:>5', expect.anything()); + }); + + it.each([ + [ + 'the same key is selected', + () => userEvent.click(screen.getByRole('option', {name: 'countOne'})), + ], + ['escape is pressed', () => userEvent.keyboard('{Escape}')], + ])( + 'keeps the filter without a value when the key editor closes after %s', + async (_, closeKeyEditor) => { + const mockOnChange = jest.fn(); + const mockOnSearch = jest.fn(); + render( + + ); + await userEvent.click(getLastInput()); + await userEvent.click(screen.getByRole('option', {name: 'countOne'})); + await screen.findByRole('row', {name: 'countOne:>'}); + + await userEvent.click( + screen.getByRole('button', { + name: 'Edit key for filter: countOne', + hidden: true, + }) + ); + await userEvent.clear( + screen.getByRole('combobox', {name: 'Edit filter key'}) + ); + await closeKeyEditor(); + + await waitFor(() => { + expect( + screen.getByRole('combobox', {name: 'Edit filter value'}) + ).toHaveFocus(); + }); + expect(screen.getByRole('row', {name: 'countOne:>'})).toBeInTheDocument(); + expect(mockOnChange).not.toHaveBeenCalled(); + expect(mockOnSearch).not.toHaveBeenCalled(); + + await userEvent.keyboard('5{Enter}'); + + expect(mockOnChange).toHaveBeenCalledTimes(1); + expect(mockOnChange).toHaveBeenCalledWith('countOne:>5', expect.anything()); + } + ); + + it('keeps the previous query while the new key has no value', async () => { + const mockOnChange = jest.fn(); + const mockOnSearch = jest.fn(); + render( + + ); + + await changeKey('browser.name', 'countOne'); + await screen.findByRole('row', {name: 'countOne:>'}); + await changeKey('countOne', 'countTwo'); + await screen.findByRole('row', {name: 'countTwo:>'}); + + expect(mockOnChange).not.toHaveBeenCalled(); + expect(mockOnSearch).not.toHaveBeenCalled(); + + await userEvent.keyboard('5{Enter}'); + + expect(mockOnChange).toHaveBeenCalledTimes(1); + expect(mockOnChange).toHaveBeenCalledWith( + 'countTwo:>5 OR device:foo', + expect.anything() + ); + }); + }); + + it('commits the placeholder value after an invalid value is rejected', async () => { + const mockOnChange = jest.fn(); + const mockOnSearch = jest.fn(); + render( + + ); + await addDurationFilter(); + + await userEvent.keyboard('abc'); + await userEvent.click(document.body); + + expect( + await screen.findByRole('row', {name: 'duration:>10ms'}) + ).toBeInTheDocument(); + expect(mockOnChange).toHaveBeenCalledTimes(1); + expect(mockOnChange).toHaveBeenCalledWith('duration:>10ms', expect.anything()); + + // The search uses the value the filter shows + await userEvent.click(getLastInput()); + await userEvent.keyboard('{Enter}'); + expect(mockOnSearch).toHaveBeenLastCalledWith( + 'duration:>10ms', + expect.anything() + ); + }); }); it('pre-fills input with current value when editing', async () => { @@ -5475,15 +5896,13 @@ describe('SearchQueryBuilder', () => { fieldDefinitionGetter, }; - it('new size filters start with greater than operator and default value', async () => { + it('new size filters start with greater than operator and no value', async () => { render(); await userEvent.click(getLastInput()); await userEvent.click(screen.getByRole('option', {name: 'size'})); - // Should start with the > operator and a value of 10bytes - expect( - await screen.findByRole('row', {name: 'size:>10bytes'}) - ).toBeInTheDocument(); + expect(await screen.findByRole('row', {name: 'size:>'})).toBeInTheDocument(); + expect(screen.getByRole('combobox', {name: 'Edit filter value'})).toHaveValue(''); }); it('pre-fills input with current value when editing', async () => { @@ -5624,13 +6043,13 @@ describe('SearchQueryBuilder', () => { fieldDefinitionGetter, }; - it('new percentage filters start with greater than operator and default value', async () => { + it('new percentage filters start with greater than operator and no value', async () => { render(); await userEvent.click(getLastInput()); await userEvent.click(screen.getByRole('option', {name: 'rate'})); - // Should start with the > operator and a value of 50% - expect(await screen.findByRole('row', {name: 'rate:>0.5'})).toBeInTheDocument(); + expect(await screen.findByRole('row', {name: 'rate:>'})).toBeInTheDocument(); + expect(screen.getByRole('combobox', {name: 'Edit filter value'})).toHaveValue(''); }); it('pre-fills input with current value when editing', async () => { @@ -5746,13 +6165,13 @@ describe('SearchQueryBuilder', () => { fieldDefinitionGetter, }; - it('new currency filters start with greater than operator and default value', async () => { + it('new currency filters start with greater than operator and no value', async () => { render(); await userEvent.click(getLastInput()); await userEvent.click(screen.getByRole('option', {name: 'cost'})); - // Should start with the > operator and a value of 10 - expect(await screen.findByRole('row', {name: 'cost:>10'})).toBeInTheDocument(); + expect(await screen.findByRole('row', {name: 'cost:>'})).toBeInTheDocument(); + expect(screen.getByRole('combobox', {name: 'Edit filter value'})).toHaveValue(''); }); it('pre-fills input with current value when editing', async () => { @@ -6310,7 +6729,7 @@ describe('SearchQueryBuilder', () => { expect( await screen.findByRole('row', { - name: 'count_if(transaction.duration,greater,300ms):>100', + name: 'count_if(transaction.duration,greater,300ms):>', }) ).toBeInTheDocument(); }); @@ -6464,7 +6883,7 @@ describe('SearchQueryBuilder', () => { await userEvent.click(getLastInput()); await userEvent.keyboard('count'); await userEvent.click(screen.getByRole('option', {name: 'count()'})); - expect(screen.getByLabelText('count():>100')).toBeInTheDocument(); + expect(screen.getByLabelText('count():>')).toBeInTheDocument(); expect(await screen.findByLabelText('Edit filter value')).toHaveFocus(); }); @@ -6474,7 +6893,7 @@ describe('SearchQueryBuilder', () => { await userEvent.click(getLastInput()); await userEvent.keyboard('count('); - expect(screen.getByLabelText('count():>100')).toBeInTheDocument(); + expect(screen.getByLabelText('count():>')).toBeInTheDocument(); expect(await screen.findByLabelText('Edit filter value')).toHaveFocus(); }); @@ -6484,9 +6903,7 @@ describe('SearchQueryBuilder', () => { await userEvent.click(getLastInput()); await userEvent.keyboard('p95('); - expect( - screen.getByLabelText('p95(transaction.duration):>300ms') - ).toBeInTheDocument(); + expect(screen.getByLabelText('p95(transaction.duration):>')).toBeInTheDocument(); expect(screen.getByLabelText('Edit function parameters')).toHaveFocus(); // XXX(malwilley): SearchQueryBuilderInput updates state in the render @@ -6507,7 +6924,7 @@ describe('SearchQueryBuilder', () => { await userEvent.click(getLastInput()); await userEvent.keyboard('count_if('); expect( - screen.getByLabelText('count_if(transaction.duration,greater,300ms):>100') + screen.getByLabelText('count_if(transaction.duration,greater,300ms):>') ).toBeInTheDocument(); expect(screen.getByLabelText('Edit function parameters')).toHaveFocus(); await userEvent.keyboard('transaction'); @@ -9306,7 +9723,7 @@ describe('SearchQueryBuilder', () => { ); expect( - await screen.findByRole('row', {name: 'app.vitals.start.warm.value:>100'}) + await screen.findByRole('row', {name: 'app.vitals.start.warm.value:>'}) ).toBeInTheDocument(); }); diff --git a/static/app/components/searchQueryBuilder/index.tsx b/static/app/components/searchQueryBuilder/index.tsx index 2ca524343594..342594b0223a 100644 --- a/static/app/components/searchQueryBuilder/index.tsx +++ b/static/app/components/searchQueryBuilder/index.tsx @@ -285,7 +285,7 @@ function ActionButtons({ ref?: React.Ref; trailingItems?: React.ReactNode; }) { - const {clearSearchQuery, query} = useSearchQueryBuilderState(); + const {clearSearchQuery, pendingFilterKey, query} = useSearchQueryBuilderState(); const {disabled, caseInsensitive, onCaseInsensitiveClick} = useSearchQueryBuilderConfig(); @@ -320,6 +320,15 @@ function ActionButtons({ size="zero" icon={} variant="transparent" + onPointerDown={e => { + // Clear on press while a filter has no value. Otherwise focus leaves + // the filter first, which commits its placeholder value and runs a + // search before the query is cleared. + if (pendingFilterKey && e.button === 0) { + e.preventDefault(); + clearSearchQuery(); + } + }} onClick={() => clearSearchQuery()} /> )} diff --git a/static/app/components/searchQueryBuilder/tokens/filter/filter.tsx b/static/app/components/searchQueryBuilder/tokens/filter/filter.tsx index 3141e4051d5a..7c6aa503166d 100644 --- a/static/app/components/searchQueryBuilder/tokens/filter/filter.tsx +++ b/static/app/components/searchQueryBuilder/tokens/filter/filter.tsx @@ -1,4 +1,4 @@ -import {useLayoutEffect, useRef, useState} from 'react'; +import {useCallback, useEffect, useLayoutEffect, useRef, useState} from 'react'; import styled from '@emotion/styled'; import {useFocusWithin} from '@react-aria/interactions'; import {mergeProps} from '@react-aria/utils'; @@ -47,10 +47,18 @@ interface SearchQueryTokenProps { interface FilterValueProps extends SearchQueryTokenProps { filterRef: React.RefObject; + isPending: boolean; onActiveChange: (active: boolean) => void; } -function FilterValue({token, state, item, filterRef, onActiveChange}: FilterValueProps) { +function FilterValue({ + token, + state, + item, + filterRef, + isPending, + onActiveChange, +}: FilterValueProps) { const ref = useRef(null); const {dispatch, focusOverride} = useSearchQueryBuilderState(); const {disabled} = useSearchQueryBuilderConfig(); @@ -80,7 +88,10 @@ function FilterValue({token, state, item, filterRef, onActiveChange}: FilterValu ) { return; } + // A rejected value leaves the editor without closing it, so mark it closed + // here too. Otherwise a pending filter is never treated as left. setEditSource(false); + onActiveChange(false); }, }); @@ -94,6 +105,11 @@ function FilterValue({token, state, item, filterRef, onActiveChange}: FilterValu token={token} wrapperRef={ref} editingCommittedValue={editSource === 'click'} + isPendingValue={isPending} + onClose={() => { + setEditSource(false); + onActiveChange(false); + }} onDelete={() => { filterRef.current?.focus(); state.selectionManager.setFocusedKey(item.key); @@ -103,6 +119,11 @@ function FilterValue({token, state, item, filterRef, onActiveChange}: FilterValu onCommit={() => { setEditSource(false); onActiveChange(false); + // Without a value, keep the placeholder value. This is a no-op if a + // value was just entered, since that already ended the pending state. + if (isPending) { + dispatch({type: 'COMMIT_PENDING_FILTER', itemKey: item.key.toString()}); + } dispatch({type: 'COMMIT_QUERY'}); // Committing on blur must not move focus back into a dismissed panel. if ( @@ -129,23 +150,49 @@ function FilterValue({token, state, item, filterRef, onActiveChange}: FilterValu }} disabled={disabled} showAllValues={showAllFilterValues} + isEmpty={isPending} {...filterButtonProps} > - + {isPending ? null : } ); } -function FilterDelete({token, state, item}: SearchQueryTokenProps) { +function FilterDelete({ + token, + state, + item, + isPending, +}: SearchQueryTokenProps & {isPending: boolean}) { const {dispatch} = useSearchQueryBuilderState(); const {disabled} = useSearchQueryBuilderConfig(); const filterButtonProps = useFilterButtonProps({state, item}); + // Item keys are positional, so after a press removes the filter this button is + // reused for the next filter, and the click from that press must not remove it. + const removedOnPressRef = useRef(false); return ( { + onPointerDown={e => { + removedOnPressRef.current = false; + // Remove a pending filter on press. The press closes its value editor and + // re-renders the filter, and the click that follows can miss this button, + // leaving the filter behind without a value. + if (isPending && e.button === 0) { + e.preventDefault(); + removedOnPressRef.current = true; + dispatch({type: 'DELETE_TOKEN', token}); + } + }} + onClick={e => { + const removedOnPress = removedOnPressRef.current; + removedOnPressRef.current = false; + // Keyboard clicks (detail 0) have no press, so they always remove + if (removedOnPress && e.detail > 0) { + return; + } dispatch({type: 'DELETE_TOKEN', token}); }} disabled={disabled} @@ -160,10 +207,47 @@ function FilterDelete({token, state, item}: SearchQueryTokenProps) { export function SearchQueryBuilderFilter({item, state, token}: SearchQueryTokenProps) { const ref = useRef(null); const [filterMenuOpen, setFilterMenuOpen] = useState(false); + // Read when focus has settled, which can be before the state update renders. + const filterMenuOpenRef = useRef(false); + const handleFilterMenuOpenChange = useCallback((open: boolean) => { + filterMenuOpenRef.current = open; + setFilterMenuOpen(open); + }, []); const isFocused = item.key === state.selectionManager.focusedKey; - const {dispatch} = useSearchQueryBuilderState(); + const {dispatch, pendingFilterKey} = useSearchQueryBuilderState(); + const isPending = pendingFilterKey === item.key; + + // Once the user leaves a pending filter without a value, keep its placeholder + // value. Wait for focus to settle first: the operator menu moves focus out of + // the filter and back. + const commitIfAbandoned = useCallback(() => { + setTimeout(() => { + if ( + !ref.current || + filterMenuOpenRef.current || + ref.current.contains(document.activeElement) + ) { + return; + } + dispatch({type: 'COMMIT_PENDING_FILTER', itemKey: item.key.toString()}); + }); + }, [dispatch, item.key]); + + useEffect(() => { + if (isPending && !filterMenuOpen) { + commitIfAbandoned(); + } + }, [commitIfAbandoned, filterMenuOpen, isPending]); + + const {focusWithinProps} = useFocusWithin({ + onBlurWithin: () => { + if (isPending) { + commitIfAbandoned(); + } + }, + }); const {invalidFilterKeys, invalidFilterKeyMessage, filterKeyAliases} = useSearchQueryBuilderConfig(); const {showAllFilterValues} = useSearchQueryBuilderLayout(); @@ -184,9 +268,18 @@ export function SearchQueryBuilderFilter({item, state, token}: SearchQueryTokenP }; // oxlint-disable-next-line react/refs - const modifiedRowProps = mergeProps(rowProps, { + const modifiedRowProps = mergeProps(rowProps, focusWithinProps, { tabIndex: isFocused ? 0 : -1, onKeyDown, + // The pending filter's value is a placeholder, so leave it out of the label + ...(isPending + ? { + 'aria-label': token.text.slice( + 0, + token.value.location.start.offset - token.location.start.offset + ), + } + : {}), }); const hasTokenInvalid = @@ -232,14 +325,14 @@ export function SearchQueryBuilderFilter({item, state, token}: SearchQueryTokenP item={item} token={token} state={state} - onActiveChange={setFilterMenuOpen} + onActiveChange={handleFilterMenuOpenChange} /> ) : ( )} @@ -249,7 +342,7 @@ export function SearchQueryBuilderFilter({item, state, token}: SearchQueryTokenP token={token} state={state} item={item} - onOpenChange={setFilterMenuOpen} + onOpenChange={handleFilterMenuOpenChange} /> @@ -258,11 +351,12 @@ export function SearchQueryBuilderFilter({item, state, token}: SearchQueryTokenP state={state} item={item} filterRef={ref} - onActiveChange={setFilterMenuOpen} + isPending={isPending} + onActiveChange={handleFilterMenuOpenChange} /> - + @@ -274,8 +368,9 @@ const FilterValueGridCell = styled(BaseGridCell)` min-width: 0; `; -const ValueButton = styled(UnstyledButton)<{showAllValues: boolean}>` +const ValueButton = styled(UnstyledButton)<{isEmpty: boolean; showAllValues: boolean}>` padding: 0 ${p => p.theme.space['2xs']}; + min-width: ${p => (p.isEmpty ? p.theme.space.xl : 0)}; color: ${p => p.theme.tokens.content.accent}; border-left: 1px solid transparent; border-right: 1px solid transparent; diff --git a/static/app/components/searchQueryBuilder/tokens/filter/filterKeyCombobox.tsx b/static/app/components/searchQueryBuilder/tokens/filter/filterKeyCombobox.tsx index 144e37ca22a7..c1c8e3382919 100644 --- a/static/app/components/searchQueryBuilder/tokens/filter/filterKeyCombobox.tsx +++ b/static/app/components/searchQueryBuilder/tokens/filter/filterKeyCombobox.tsx @@ -42,7 +42,20 @@ export function FilterKeyCombobox({token, onCommit, item}: KeyComboboxProps) { }); const {filterKeys, getFieldDefinition, getSuggestedFilterKey} = useSearchQueryBuilderConfig(); - const {dispatch} = useSearchQueryBuilderState(); + const {dispatch, pendingFilterKey} = useSearchQueryBuilderState(); + const isPending = pendingFilterKey === item.key; + + // Closing the key editor moves focus out of the filter, which would give a + // pending filter its placeholder value, so go back to its value instead. + const closeEditor = useCallback(() => { + if (isPending) { + dispatch({ + type: 'SET_FOCUS_OVERRIDE', + focusOverride: {itemKey: item.key.toString(), part: 'value'}, + }); + } + onCommit(); + }, [dispatch, isPending, item.key, onCommit]); const currentFilterValueType = getFilterValueType( token, @@ -55,7 +68,7 @@ export function FilterKeyCombobox({token, onCommit, item}: KeyComboboxProps) { const newFilterValueType = getFilterValueType(token, newFieldDef); if (keyName === getKeyName(token.key)) { - onCommit(); + closeEditor(); return; } @@ -73,8 +86,10 @@ export function FilterKeyCombobox({token, onCommit, item}: KeyComboboxProps) { type: 'UPDATE_FILTER_KEY', token, key: keyName, + // A filter without a value stays pending + shouldCommitQuery: !isPending, }); - onCommit(); + closeEditor(); return; } @@ -90,7 +105,16 @@ export function FilterKeyCombobox({token, onCommit, item}: KeyComboboxProps) { onCommit(); }, - [currentFilterValueType, dispatch, getFieldDefinition, item.key, onCommit, token] + [ + closeEditor, + currentFilterValueType, + dispatch, + getFieldDefinition, + isPending, + item.key, + onCommit, + token, + ] ); const onOptionSelected = useCallback( @@ -105,7 +129,7 @@ export function FilterKeyCombobox({token, onCommit, item}: KeyComboboxProps) { const trimmedKeyName = keyName.trim(); if (!trimmedKeyName) { - onCommit(); + closeEditor(); return; } @@ -118,7 +142,7 @@ export function FilterKeyCombobox({token, onCommit, item}: KeyComboboxProps) { }) ); }, - [filterKeys, getSuggestedFilterKey, handleSelectKey, onCommit, sortedFilterKeys] + [closeEditor, filterKeys, getSuggestedFilterKey, handleSelectKey, sortedFilterKeys] ); const onCustomValueBlurred = useCallback(() => { @@ -126,8 +150,8 @@ export function FilterKeyCombobox({token, onCommit, item}: KeyComboboxProps) { }, [onCommit]); const onExit = useCallback(() => { - onCommit(); - }, [onCommit]); + closeEditor(); + }, [closeEditor]); return ( diff --git a/static/app/components/searchQueryBuilder/tokens/filter/filterOperator.tsx b/static/app/components/searchQueryBuilder/tokens/filter/filterOperator.tsx index e648be7e7f0e..cdecc7417c48 100644 --- a/static/app/components/searchQueryBuilder/tokens/filter/filterOperator.tsx +++ b/static/app/components/searchQueryBuilder/tokens/filter/filterOperator.tsx @@ -39,7 +39,7 @@ const MENU_OFFSET: [number, number] = [0, 12]; export function FilterOperator({state, item, token, onOpenChange}: FilterOperatorProps) { const organization = useOrganization(); - const {dispatch, query, focusOverride} = useSearchQueryBuilderState(); + const {dispatch, query, focusOverride, pendingFilterKey} = useSearchQueryBuilderState(); const { allowRegexOperators, searchSource, @@ -134,6 +134,7 @@ export function FilterOperator({state, item, token, onOpenChange}: FilterOperato const focusValue = initialOpSettingRef.current || + pendingFilterKey === item.key || (isRegexOperator(option.value) && !isRegexOperator(token.operator)); dispatch({ diff --git a/static/app/components/searchQueryBuilder/tokens/filter/valueCombobox.tsx b/static/app/components/searchQueryBuilder/tokens/filter/valueCombobox.tsx index d4cdad29f401..416b9d4bd371 100644 --- a/static/app/components/searchQueryBuilder/tokens/filter/valueCombobox.tsx +++ b/static/app/components/searchQueryBuilder/tokens/filter/valueCombobox.tsx @@ -96,6 +96,16 @@ import {useOrganization} from 'sentry/utils/useOrganization'; type SearchQueryValueBuilderProps = { editingCommittedValue: boolean; + /** + * The filter is pending: its value is only a placeholder, so the input starts + * empty. + */ + isPendingValue: boolean; + /** + * Closes the editor without committing, when a pending value is left empty + * because focus moved away. The filter decides what to do once focus settles. + */ + onClose: () => void; onCommit: () => void; onDelete: () => void; token: TokenResult; @@ -681,8 +691,12 @@ function ValueComboboxCustomMenu( export function getInitialInputValue( token: TokenResult, canSelectMultipleValues: boolean, - editingCommittedValue?: boolean + editingCommittedValue?: boolean, + isPendingValue?: boolean ) { + if (isPendingValue) { + return ''; + } if (isDateToken(token)) { return token.value.type === Token.VALUE_ISO_8601_DATE ? token.value.text : ''; } @@ -700,10 +714,12 @@ export function getInitialInputValue( export function SearchQueryBuilderValueCombobox({ token, + onClose, onDelete, onCommit, wrapperRef, editingCommittedValue, + isPendingValue, }: SearchQueryValueBuilderProps) { const ref = useRef(null); const inputRef = useRef(null); @@ -744,7 +760,12 @@ export function SearchQueryBuilderValueCombobox({ const [inputValue, setInputValue] = useState(() => canSelectMultipleValues ? '' - : getInitialInputValue(token, canSelectMultipleValues, editingCommittedValue) + : getInitialInputValue( + token, + canSelectMultipleValues, + editingCommittedValue, + isPendingValue + ) ); // Tracks where the input sits within the chip row. `value` is the lifted chip's // text (so it can be restored on Escape and reinserted where it was rather than @@ -767,12 +788,13 @@ export function SearchQueryBuilderValueCombobox({ const filterValue = isRegexValue ? inputValue : unescapeAsteriskSearchValue(inputValue); + // A pending filter's value is only a placeholder, so it has nothing selected const selectedValues = useMemo( () => - canSelectMultipleValues + canSelectMultipleValues && !isPendingValue ? getSelectedValuesFromText(getMultiSelectInputValue(token)) : [], - [canSelectMultipleValues, token] + [canSelectMultipleValues, isPendingValue, token] ); // A chip being edited is lifted into the input but left in the token until the @@ -1244,7 +1266,16 @@ export function SearchQueryBuilderValueCombobox({ }; const handleInputValueConfirmed = useCallback( - (value: string) => { + (value: string, event?: React.FocusEvent) => { + if (isPendingValue && !value.trim()) { + if (event) { + onClose(); + } else { + onCommit(); + } + return; + } + if (canSelectMultipleValues) { if (value.trim()) { addTypedValue(value); @@ -1262,7 +1293,12 @@ export function SearchQueryBuilderValueCombobox({ const isUnchanged = value === - getInitialInputValue(token, canSelectMultipleValues, editingCommittedValue); + getInitialInputValue( + token, + canSelectMultipleValues, + editingCommittedValue, + isPendingValue + ); // If there's no user input and the token has no value, set a default one if (!value && !token.value.text && !isRegexValue) { @@ -1294,7 +1330,9 @@ export function SearchQueryBuilderValueCombobox({ dispatch, editingCommittedValue, fieldDefinition, + isPendingValue, isRegexValue, + onClose, onCommit, token, updateFilterValue, diff --git a/static/app/components/searchQueryBuilder/tokens/freeText.tsx b/static/app/components/searchQueryBuilder/tokens/freeText.tsx index dc9b50660352..bfeeff25167e 100644 --- a/static/app/components/searchQueryBuilder/tokens/freeText.tsx +++ b/static/app/components/searchQueryBuilder/tokens/freeText.tsx @@ -384,7 +384,8 @@ function SearchQueryBuilderInputInternal({ const filterValue = getWordAtCursorPosition(inputValue, selectionIndex); - const {query, dispatch, handleSearch} = useSearchQueryBuilderState(); + const {query, committedQuery, dispatch, handleSearch, pendingFilterKey} = + useSearchQueryBuilderState(); const { filterKeys, getFieldDefinition, @@ -754,7 +755,8 @@ function SearchQueryBuilderInputInternal({ // if we haven't changed anything, just search if (value.trim() === trimmedTokenValue) { - handleSearch(query); + // The draft query holds a pending filter's placeholder value + handleSearch(pendingFilterKey ? committedQuery : query); return; } diff --git a/static/app/views/explore/conversations/overview.spec.tsx b/static/app/views/explore/conversations/overview.spec.tsx index 0d766259f3ce..7eaa23371423 100644 --- a/static/app/views/explore/conversations/overview.spec.tsx +++ b/static/app/views/explore/conversations/overview.spec.tsx @@ -710,7 +710,7 @@ describe('ConversationsOverviewPage', () => { ); expect( await screen.findByRole('row', { - name: 'conversation.generationDuration:>10ms', + name: 'conversation.generationDuration:>', }) ).toBeInTheDocument(); });