From cdd695a4ff85eaa1c8c4fef148316518dab2ae25 Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Fri, 30 Oct 2026 17:48:03 +0000 Subject: [PATCH 1/9] feat(search-query-builder): Remove default values for number based filters New numeric, duration, size, percentage and currency filters (including aggregates) now start with an empty value instead of a placeholder such as 100 or 10ms, so the search isn't filtered by a value the user didn't pick. The grammar accepts an empty value for number based filters, e.g. `span.duration:>`, and the frontend parser marks those filters as FILTER_MUST_HAVE_VALUE, which the builder already tolerates while the value menu is open. Dashboard global filters keep their example values. Co-Authored-By: Nicholas Deschenes --- .../searchQueryBuilder/index.spec.tsx | 66 ++++++++----------- .../searchQueryBuilder/tokens/utils.tsx | 42 ++++++++++-- .../components/searchSyntax/parser.spec.tsx | 27 ++++++++ static/app/components/searchSyntax/parser.tsx | 37 +++++++++-- .../dashboards/globalFilter/addFilter.tsx | 4 +- .../views/dashboards/globalFilter/utils.tsx | 4 +- .../explore/conversations/overview.spec.tsx | 2 +- .../app/views/explore/spans/spansTab.spec.tsx | 2 +- .../packages/search-syntax/src/grammar.pegjs | 36 ++++++++-- 9 files changed, 161 insertions(+), 59 deletions(-) diff --git a/static/app/components/searchQueryBuilder/index.spec.tsx b/static/app/components/searchQueryBuilder/index.spec.tsx index e3aef972af97..ce94815c92e4 100644 --- a/static/app/components/searchQueryBuilder/index.spec.tsx +++ b/static/app/components/searchQueryBuilder/index.spec.tsx @@ -1723,11 +1723,11 @@ describe('SearchQueryBuilder', () => { }); it.each([ - ['>', 'timesSeen:>100'], - ['<', 'timesSeen:<100'], - ['=', 'timesSeen:=100'], + ['>', 'timesSeen:>'], + ['<', 'timesSeen:<'], + ['=', 'timesSeen:='], ])( - 'adds default value for numeric filter when typing %s', + 'creates an empty numeric filter when typing %s', async (operator, expectedQuery) => { render(, { organization: numberOperatorConversionOrganization, @@ -1749,7 +1749,7 @@ describe('SearchQueryBuilder', () => { expect(getLastInput()).toHaveValue('timesSeen>'); }); - it('adds default value for numeric tag filters from filter key metadata', async () => { + it('creates empty numeric tag filters from filter key metadata', async () => { render(, { organization: numberOperatorConversionOrganization, }); @@ -1759,7 +1759,7 @@ describe('SearchQueryBuilder', () => { await userEvent.keyboard('{Escape}'); expect( - await screen.findByRole('row', {name: 'tags[bar,number]:>100'}) + await screen.findByRole('row', {name: 'tags[bar,number]:>'}) ).toBeInTheDocument(); }); @@ -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,12 +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:>'})).toBeInTheDocument(); + expect(screen.getByRole('combobox', {name: 'Edit filter value'})).toHaveValue(''); + + await userEvent.keyboard('100{Enter}'); expect( await screen.findByRole('row', {name: 'timesSeen:>100'}) ).toBeInTheDocument(); @@ -5324,15 +5327,12 @@ 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(); }); it('pre-fills input with current value when editing', async () => { @@ -5475,15 +5475,12 @@ 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(); }); it('pre-fills input with current value when editing', async () => { @@ -5624,13 +5621,12 @@ 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(); }); it('pre-fills input with current value when editing', async () => { @@ -5746,13 +5742,12 @@ 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(); }); it('pre-fills input with current value when editing', async () => { @@ -6303,14 +6298,14 @@ describe('SearchQueryBuilder', () => { filterKeySections: [], }; - it('can add an aggregate filter with default values', async () => { + it('can add an aggregate filter with default parameters', async () => { render(); await userEvent.click(getLastInput()); await userEvent.click(screen.getByRole('option', {name: 'count_if(...)'})); expect( await screen.findByRole('row', { - name: 'count_if(transaction.duration,greater,300ms):>100', + name: 'count_if(transaction.duration,greater,300ms):>', }) ).toBeInTheDocument(); }); @@ -6401,7 +6396,7 @@ describe('SearchQueryBuilder', () => { ).toBeInTheDocument(); }); - it('automatically changes the filter value if the type changes after editing parameters', async () => { + it('clears the filter value if the type changes after editing parameters', async () => { render( { await userEvent.clear(input); await userEvent.keyboard('timesSeen{enter}'); - // After selecting timesSeen, the value should change to a number expect( await screen.findByRole('row', { - name: 'p95(timesSeen):>100', + name: 'p95(timesSeen):>', }) ).toBeInTheDocument(); }); @@ -6464,7 +6458,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 +6468,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(); }); @@ -6507,7 +6501,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'); @@ -7232,9 +7226,7 @@ describe('SearchQueryBuilder', () => { expect( screen.getByRole('button', {name: 'Edit key for filter: tags[bar,number]'}) ).toHaveTextContent('bar'); - expect( - screen.getByRole('row', {name: 'tags[bar,number]:>100'}) - ).toBeInTheDocument(); + expect(screen.getByRole('row', {name: 'tags[bar,number]:>'})).toBeInTheDocument(); }); it('does not replace string key with suggestion when typing comparison operator', async () => { @@ -9306,7 +9298,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/tokens/utils.tsx b/static/app/components/searchQueryBuilder/tokens/utils.tsx index 19e7e0b1738d..18fe9bc318fd 100644 --- a/static/app/components/searchQueryBuilder/tokens/utils.tsx +++ b/static/app/components/searchQueryBuilder/tokens/utils.tsx @@ -177,26 +177,52 @@ export function resolveFilterKey({ return trimmedKey; } +/** + * Number based filters start without a value so the user picks one, rather than + * filtering by a placeholder they didn't choose. + */ export function getDefaultValueForValueType(valueType: FieldValueType | null): string { switch (valueType) { case FieldValueType.BOOLEAN: return 'true'; case FieldValueType.INTEGER: case FieldValueType.NUMBER: - return '100'; case FieldValueType.CURRENCY: - return '10'; + case FieldValueType.DURATION: + case FieldValueType.SIZE: + case FieldValueType.PERCENTAGE: + return ''; case FieldValueType.DATE: return '-24h'; + case FieldValueType.STRING: + default: + return '""'; + } +} + +/** + * Example values for number based filters, for callers that need a complete + * filter rather than one with an empty value. + */ +function getNumericExampleValue(fieldDefinition: FieldDefinition | null): string | null { + if (!fieldDefinition || defined(fieldDefinition.defaultValue)) { + return null; + } + + switch (fieldDefinition.valueType) { + case FieldValueType.INTEGER: + case FieldValueType.NUMBER: + return '100'; + case FieldValueType.CURRENCY: + return '10'; case FieldValueType.DURATION: return '10ms'; case FieldValueType.SIZE: return '10bytes'; case FieldValueType.PERCENTAGE: return '0.5'; - case FieldValueType.STRING: default: - return '""'; + return null; } } @@ -250,12 +276,14 @@ function getInitialValueType(fieldDefinition: FieldDefinition | null) { export function getInitialFilterText( key: string, fieldDefinition: FieldDefinition | null, - operator: TermOperator = TermOperator.GREATER_THAN + operator: TermOperator = TermOperator.GREATER_THAN, + {withNumericExampleValue = false}: {withNumericExampleValue?: boolean} = {} ) { - const defaultValue = getDefaultFilterValue({fieldDefinition}); - const keyText = getInitialFilterKeyText(key, fieldDefinition); const valueType = getInitialValueType(fieldDefinition); + const defaultValue = + (withNumericExampleValue ? getNumericExampleValue(fieldDefinition) : null) ?? + getDefaultFilterValue({fieldDefinition}); // Array attributes filter by membership. `addArrayMembershipOperator` places // the `[*]` operator (inside the bracket for the tag form, trailing for the diff --git a/static/app/components/searchSyntax/parser.spec.tsx b/static/app/components/searchSyntax/parser.spec.tsx index dcf686dbe30a..97789333fadf 100644 --- a/static/app/components/searchSyntax/parser.spec.tsx +++ b/static/app/components/searchSyntax/parser.spec.tsx @@ -261,6 +261,33 @@ describe('searchSyntax/parser', () => { expect(filterOf('avg(span.size):>1kb')).toBeUndefined(); }); + it.each([ + ['num:>', FilterType.NUMERIC, TermOperator.GREATER_THAN], + ['!num:<=', FilterType.NUMERIC, TermOperator.LESS_THAN_EQUAL], + ['num:', FilterType.NUMERIC, TermOperator.DEFAULT], + ['span.duration:>', FilterType.DURATION, TermOperator.GREATER_THAN], + ['span.size:<', FilterType.SIZE, TermOperator.LESS_THAN], + ['count():>', FilterType.AGGREGATE_NUMERIC, TermOperator.GREATER_THAN], + [ + 'p95(span.duration):>=', + FilterType.AGGREGATE_DURATION, + TermOperator.GREATER_THAN_EQUAL, + ], + ])('parses %s as a filter that must have a value', (query, filterType, operator) => { + const filter = parseSearch(query, { + numericKeys: new Set(['num']), + durationKeys: new Set(['span.duration']), + sizeKeys: new Set(['span.size']), + })?.find((token): token is TokenResult => token.type === Token.FILTER); + + expect(filter?.filter).toBe(filterType); + expect(filter?.operator).toBe(operator); + expect(filter?.value.text).toBe(''); + expect(filter?.invalid).toEqual( + expect.objectContaining({type: InvalidReason.FILTER_MUST_HAVE_VALUE}) + ); + }); + it('returns the matched text for keys, values, and aggregate params', () => { const result = parseSearch( 'tags[foo:bar]:baz browser.name-1:"Fire \\"fox\\"" device.family: count_if(user.email,equals,"I, got \\"quotes\\""):>100 firstSeen:+7d' diff --git a/static/app/components/searchSyntax/parser.tsx b/static/app/components/searchSyntax/parser.tsx index 4c4ca55228d1..1ac534dbb10c 100644 --- a/static/app/components/searchSyntax/parser.tsx +++ b/static/app/components/searchSyntax/parser.tsx @@ -331,6 +331,25 @@ type FilterMap = { type TextFilter = FilterMap[FilterType.TEXT]; type InFilter = FilterMap[FilterType.TEXT_IN] | FilterMap[FilterType.NUMERIC_IN]; + +/** + * Filter types whose value can be left empty after a comparison operator, + * such as `span.duration:>`. + */ +const COMPARISON_FILTER_TYPES = new Set([ + FilterType.NUMERIC, + FilterType.DURATION, + FilterType.SIZE, + FilterType.AGGREGATE_NUMERIC, + FilterType.AGGREGATE_DURATION, + FilterType.AGGREGATE_SIZE, + FilterType.AGGREGATE_PERCENTAGE, +]); +type ComparisonFilterValue = FilterMap[ + | FilterType.NUMERIC + | FilterType.DURATION + | FilterType.SIZE + | FilterType.AGGREGATE_PERCENTAGE]['value']; type AggregateFilterType = | FilterMap[FilterType.AGGREGATE_DATE] | FilterMap[FilterType.AGGREGATE_DURATION] @@ -678,7 +697,7 @@ export class TokenConverter { type: Token.VALUE_DURATION as const, value, - parsed: this.config.parse ? parseDuration(value, unit) : undefined, + parsed: this.config.parse && value !== '' ? parseDuration(value, unit) : undefined, unit, }); @@ -711,7 +730,7 @@ export class TokenConverter { value, // units are case insensitive, normalize them in their parsed representation // so that we dont have to compare all possible permutations. - parsed: this.config.parse ? parseSize(value, unit) : undefined, + parsed: this.config.parse && value !== '' ? parseSize(value, unit) : undefined, unit, }); @@ -719,7 +738,7 @@ export class TokenConverter { ...this.defaultTokenFields, type: Token.VALUE_PERCENTAGE as const, value, - parsed: this.config.parse ? parsePercentage(value) : undefined, + parsed: this.config.parse && value !== '' ? parsePercentage(value) : undefined, }); tokenValueBoolean = (value: string) => ({ @@ -735,7 +754,7 @@ export class TokenConverter { type: Token.VALUE_NUMBER as const, value, unit, - parsed: this.config.parse ? parseNumber(value, unit) : undefined, + parsed: this.config.parse && value !== '' ? parseNumber(value, unit) : undefined, }; }; @@ -974,6 +993,16 @@ export class TokenConverter { return this.checkInvalidInFilter(value as InFilter['value']); } + if ( + COMPARISON_FILTER_TYPES.has(filter) && + (value as ComparisonFilterValue).value === '' + ) { + return { + type: InvalidReason.FILTER_MUST_HAVE_VALUE, + reason: this.config.invalidMessages[InvalidReason.FILTER_MUST_HAVE_VALUE], + }; + } + if ('name' in key) { return this.checkInvalidAggregateKey(key); } diff --git a/static/app/views/dashboards/globalFilter/addFilter.tsx b/static/app/views/dashboards/globalFilter/addFilter.tsx index 7b034069dcdd..d8a8f58be9dd 100644 --- a/static/app/views/dashboards/globalFilter/addFilter.tsx +++ b/static/app/views/dashboards/globalFilter/addFilter.tsx @@ -183,7 +183,9 @@ export function AddFilter({ if (valueType && !IGNORE_DEFAULT_VALUES.includes(valueType)) { defaultFilterValue = getInitialFilterText( selectedFilterKey.key, - fieldDefinition + fieldDefinition, + undefined, + {withNumericExampleValue: true} ); } diff --git a/static/app/views/dashboards/globalFilter/utils.tsx b/static/app/views/dashboards/globalFilter/utils.tsx index 5271a7a3fb45..88cd2e1d3ca5 100644 --- a/static/app/views/dashboards/globalFilter/utils.tsx +++ b/static/app/views/dashboards/globalFilter/utils.tsx @@ -118,7 +118,9 @@ export function getFilterToken( const {tag, value} = globalFilter; let filterValue = value; if (value === '') { - filterValue = getInitialFilterText(tag.key, fieldDefinition); + filterValue = getInitialFilterText(tag.key, fieldDefinition, undefined, { + withNumericExampleValue: true, + }); } const filterTokens = parseFilterValue(filterValue, globalFilter); return filterTokens[0] ?? null; 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(); }); diff --git a/static/app/views/explore/spans/spansTab.spec.tsx b/static/app/views/explore/spans/spansTab.spec.tsx index b489c5947d23..8d578523ac03 100644 --- a/static/app/views/explore/spans/spansTab.spec.tsx +++ b/static/app/views/explore/spans/spansTab.spec.tsx @@ -649,7 +649,7 @@ describe('SpansTabContent', () => { const input = screen.getByRole('combobox'); await userEvent.click(input); - await userEvent.type(input, 'span.duration:>10ms{enter}'); + await userEvent.type(input, 'span.duration:10ms{enter}'); // re-open the combobox await userEvent.click(input); diff --git a/static/packages/search-syntax/src/grammar.pegjs b/static/packages/search-syntax/src/grammar.pegjs index 2ffab55dd862..f444bb898b92 100644 --- a/static/packages/search-syntax/src/grammar.pegjs +++ b/static/packages/search-syntax/src/grammar.pegjs @@ -106,7 +106,7 @@ rel_date_filter // filter for durations duration_filter - = negation:negation? key:search_key sep op:operator? value:duration_format &{ + = negation:negation? key:search_key sep op:operator? value:(duration_format / empty_duration_value) &{ return tc.predicateFilter(FilterType.DURATION, key) } { return tc.tokenFilter(FilterType.DURATION, key, value, op, !!negation); @@ -114,7 +114,7 @@ duration_filter // filter for file size size_filter - = negation:negation? key:search_key sep op:operator? value:size_format &{ + = negation:negation? key:search_key sep op:operator? value:(size_format / empty_size_value) &{ return tc.predicateFilter(FilterType.SIZE, key) } { return tc.tokenFilter(FilterType.SIZE, key, value, op, !!negation); @@ -138,7 +138,7 @@ numeric_in_filter // numeric comparison filter numeric_filter - = negation:negation? key:search_key sep op:operator? value:numeric_value &{ + = negation:negation? key:search_key sep op:operator? value:(numeric_value / empty_numeric_value) &{ return tc.predicateFilter(FilterType.NUMERIC, key) } { return tc.tokenFilter(FilterType.NUMERIC, key, value, op, !!negation); @@ -146,7 +146,7 @@ numeric_filter // aggregate duration filter aggregate_duration_filter - = negation:negation? key:aggregate_key sep op:operator? value:duration_format &{ + = negation:negation? key:aggregate_key sep op:operator? value:(duration_format / empty_duration_value) &{ return tc.predicateFilter(FilterType.AGGREGATE_DURATION, key) } { return tc.tokenFilter(FilterType.AGGREGATE_DURATION, key, value, op, !!negation); @@ -154,7 +154,7 @@ aggregate_duration_filter // aggregate file size filter aggregate_size_filter - = negation:negation? key:aggregate_key sep op:operator? value:size_format &{ + = negation:negation? key:aggregate_key sep op:operator? value:(size_format / empty_size_value) &{ return tc.predicateFilter(FilterType.AGGREGATE_SIZE, key) } { return tc.tokenFilter(FilterType.AGGREGATE_SIZE, key, value, op, !!negation); @@ -162,7 +162,7 @@ aggregate_size_filter // aggregate percentage filter aggregate_percentage_filter - = negation:negation? key:aggregate_key sep op:operator? value:percentage_format &{ + = negation:negation? key:aggregate_key sep op:operator? value:(percentage_format / empty_percentage_value) &{ return tc.predicateFilter(FilterType.AGGREGATE_PERCENTAGE, key) } { return tc.tokenFilter(FilterType.AGGREGATE_PERCENTAGE, key, value, op, !!negation); @@ -170,7 +170,7 @@ aggregate_percentage_filter // aggregate numeric filter aggregate_numeric_filter - = negation:negation? key:aggregate_key sep op:operator? value:numeric_value &{ + = negation:negation? key:aggregate_key sep op:operator? value:(numeric_value / empty_numeric_value) &{ return tc.predicateFilter(FilterType.AGGREGATE_NUMERIC, key) } { return tc.tokenFilter(FilterType.AGGREGATE_NUMERIC, key, value, op, !!negation); @@ -508,6 +508,28 @@ percentage_format return tc.tokenValuePercentage(value); } +// Number based filters may have an empty value, e.g. `span.duration:>`, which +// is how the search query builder represents a filter that has no value yet. +empty_numeric_value + = &end_value { + return tc.tokenValueNumber('', null); + } + +empty_duration_value + = &end_value { + return tc.tokenValueDuration('', null); + } + +empty_size_value + = &end_value { + return tc.tokenValueSize('', null); + } + +empty_percentage_value + = &end_value { + return tc.tokenValuePercentage(''); + } + // Units for special values number_unit = "k"i/"m"i/"b"i From efe2be346375d824190deee746335031bd2685c3 Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Fri, 30 Oct 2026 18:41:55 +0000 Subject: [PATCH 2/9] ref(search-query-builder): Keep empty number filters out of the query Rework the change so the search grammars stay as they are. A new number-based filter now starts without a value, but its placeholder value only lives in the builder's draft query. It is never committed, so onChange, onSearch and the API never see it. Entering a value commits the filter as usual, and leaving the filter without a value removes it. This reverts the grammar, parser and dashboard changes from the previous commit. Co-Authored-By: Nicholas Deschenes --- .../components/searchQueryBuilder/context.tsx | 7 + .../hooks/useQueryBuilderState.spec.tsx | 3 + .../hooks/useQueryBuilderState.tsx | 218 +++++++++++++++++- .../searchQueryBuilder/hooks/useUndoStack.tsx | 19 +- .../searchQueryBuilder/index.spec.tsx | 158 +++++++++++-- .../tokens/filter/filter.tsx | 95 +++++++- .../tokens/filter/filterOperator.tsx | 3 +- .../tokens/filter/valueCombobox.tsx | 57 ++++- .../searchQueryBuilder/tokens/utils.tsx | 42 +--- .../components/searchSyntax/parser.spec.tsx | 27 --- static/app/components/searchSyntax/parser.tsx | 37 +-- .../dashboards/globalFilter/addFilter.tsx | 4 +- .../views/dashboards/globalFilter/utils.tsx | 4 +- .../app/views/explore/spans/spansTab.spec.tsx | 2 +- .../packages/search-syntax/src/grammar.pegjs | 36 +-- 15 files changed, 533 insertions(+), 179 deletions(-) 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..b6a3a4057dc7 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,15 @@ 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 never + * committed: it is replaced when the user enters a value, and the filter is + * removed if they leave it without one. + */ + pendingFilterKey: string | null; + /** * The current query value. * This is the basic source of truth for what is currently being displayed. @@ -77,6 +90,19 @@ type UpdateQueryAction = { type ResetFocusOverrideAction = {type: 'RESET_FOCUS_OVERRIDE'}; +type DiscardPendingFilterAction = { + /** + * Only discard the filter if it is still the pending one. + */ + itemKey: string; + /** + * Move focus to where the filter was. Leave this off when the user has already + * moved focus somewhere else. + */ + refocus: boolean; + type: 'DISCARD_PENDING_FILTER'; +}; + type DeleteTokenAction = { token: ParseResultToken; type: 'DELETE_TOKEN'; @@ -249,6 +275,7 @@ export type QueryBuilderActions = | CommitQueryAction | UpdateQueryAction | ResetFocusOverrideAction + | DiscardPendingFilterAction | DeleteTokenAction | DeleteTokensAction | DeleteToCursorAction @@ -1119,6 +1146,163 @@ 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 === 'DISCARD_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,29 @@ export function useQueryBuilderState({ ...state, focusOverride: null, }; + case 'DISCARD_PENDING_FILTER': { + if (action.itemKey !== state.pendingFilterKey) { + return state; + } + const pendingFilter = findPendingFilter( + state.query, + state.pendingFilterKey, + parseQuery + ); + if (!pendingFilter) { + return state; + } + const newState = replaceTokensWithText(state, { + tokens: [pendingFilter], + text: '', + parseQuery, + shouldCommitQuery: true, + }); + return { + ...newState, + focusOverride: action.refocus ? newState.focusOverride : null, + }; + } case 'DELETE_TOKEN': { return { ...replaceTokensWithText(state, { @@ -1320,6 +1528,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 ce94815c92e4..656636f0bb63 100644 --- a/static/app/components/searchQueryBuilder/index.spec.tsx +++ b/static/app/components/searchQueryBuilder/index.spec.tsx @@ -1727,16 +1727,19 @@ describe('SearchQueryBuilder', () => { ['<', 'timesSeen:<'], ['=', 'timesSeen:='], ])( - 'creates an empty numeric filter when typing %s', - async (operator, expectedQuery) => { + 'creates a numeric filter without a value when typing %s', + async (operator, expectedFilter) => { render(, { organization: numberOperatorConversionOrganization, }); await userEvent.click(getLastInput()); - await userEvent.keyboard(`timesSeen${operator}{Escape}`); + await userEvent.keyboard(`timesSeen${operator}`); - expect(await screen.findByRole('row', {name: expectedQuery})).toBeInTheDocument(); + expect( + await screen.findByRole('row', {name: expectedFilter}) + ).toBeInTheDocument(); + expect(screen.getByRole('combobox', {name: 'Edit filter value'})).toHaveValue(''); } ); @@ -1749,14 +1752,13 @@ describe('SearchQueryBuilder', () => { expect(getLastInput()).toHaveValue('timesSeen>'); }); - it('creates empty numeric tag filters from filter key metadata', async () => { + it('creates numeric tag filters from filter key metadata', async () => { render(, { organization: numberOperatorConversionOrganization, }); await userEvent.click(getLastInput()); await userEvent.paste('tags[bar,number]>'); - await userEvent.keyboard('{Escape}'); expect( await screen.findByRole('row', {name: 'tags[bar,number]:>'}) @@ -5230,12 +5232,9 @@ describe('SearchQueryBuilder', () => { await userEvent.keyboard('time{ArrowDown}{Enter}'); expect(await screen.findByRole('row', {name: 'timesSeen:>'})).toBeInTheDocument(); - expect(screen.getByRole('combobox', {name: 'Edit filter value'})).toHaveValue(''); - - await userEvent.keyboard('100{Enter}'); - expect( - await screen.findByRole('row', {name: 'timesSeen:>100'}) - ).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 () => { @@ -5333,6 +5332,117 @@ describe('SearchQueryBuilder', () => { await userEvent.click(screen.getByRole('option', {name: 'duration'})); 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}'], + ])('removes the filter when %s is pressed without a value', async (_, key) => { + const mockOnChange = jest.fn(); + render(); + await addDurationFilter(); + + await userEvent.keyboard(key); + + await waitFor(() => { + expect( + screen.queryByRole('row', {name: 'duration:>'}) + ).not.toBeInTheDocument(); + }); + expect(getLastInput()).toHaveFocus(); + expect(mockOnChange).not.toHaveBeenCalled(); + }); + + it('does not bring back the placeholder 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('removes the filter when focus leaves it without a value', async () => { + const mockOnChange = jest.fn(); + render(); + await addDurationFilter(); + + await userEvent.click(document.body); + + await waitFor(() => { + expect( + screen.queryByRole('row', {name: 'duration:>'}) + ).not.toBeInTheDocument(); + }); + expect(mockOnChange).not.toHaveBeenCalled(); + }); }); it('pre-fills input with current value when editing', async () => { @@ -5481,6 +5591,7 @@ describe('SearchQueryBuilder', () => { await userEvent.click(screen.getByRole('option', {name: 'size'})); 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 () => { @@ -5627,6 +5738,7 @@ describe('SearchQueryBuilder', () => { await userEvent.click(screen.getByRole('option', {name: 'rate'})); 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 () => { @@ -5748,6 +5860,7 @@ describe('SearchQueryBuilder', () => { await userEvent.click(screen.getByRole('option', {name: 'cost'})); 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 () => { @@ -6298,7 +6411,7 @@ describe('SearchQueryBuilder', () => { filterKeySections: [], }; - it('can add an aggregate filter with default parameters', async () => { + it('can add an aggregate filter with default values', async () => { render(); await userEvent.click(getLastInput()); await userEvent.click(screen.getByRole('option', {name: 'count_if(...)'})); @@ -6396,7 +6509,7 @@ describe('SearchQueryBuilder', () => { ).toBeInTheDocument(); }); - it('clears the filter value if the type changes after editing parameters', async () => { + it('automatically changes the filter value if the type changes after editing parameters', async () => { render( { await userEvent.clear(input); await userEvent.keyboard('timesSeen{enter}'); + // After selecting timesSeen, the value should change to a number expect( await screen.findByRole('row', { - name: 'p95(timesSeen):>', + name: 'p95(timesSeen):>100', }) ).toBeInTheDocument(); }); @@ -6478,9 +6592,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 @@ -7208,7 +7320,7 @@ describe('SearchQueryBuilder', () => { render(); await userEvent.click(getLastInput()); - await userEvent.keyboard('bar:{Escape}'); + await userEvent.keyboard('bar:100{Enter}'); expect( screen.getByRole('button', {name: 'Edit key for filter: tags[bar,number]'}) @@ -7221,12 +7333,14 @@ describe('SearchQueryBuilder', () => { }); await userEvent.click(getLastInput()); - await userEvent.keyboard('bar>{Escape}'); + await userEvent.keyboard('bar>100{Enter}'); expect( screen.getByRole('button', {name: 'Edit key for filter: tags[bar,number]'}) ).toHaveTextContent('bar'); - expect(screen.getByRole('row', {name: 'tags[bar,number]:>'})).toBeInTheDocument(); + expect( + screen.getByRole('row', {name: 'tags[bar,number]:>100'}) + ).toBeInTheDocument(); }); it('does not replace string key with suggestion when typing comparison operator', async () => { @@ -7266,7 +7380,7 @@ describe('SearchQueryBuilder', () => { screen.getByRole('button', {name: 'Edit key for filter: browser.name'}) ); await userEvent.clear(screen.getByRole('combobox', {name: 'Edit filter key'})); - await userEvent.keyboard('bar{Enter}{Escape}'); + await userEvent.keyboard('bar{Enter}100{Enter}'); expect( screen.getByRole('button', {name: 'Edit key for filter: tags[bar,number]'}) diff --git a/static/app/components/searchQueryBuilder/tokens/filter/filter.tsx b/static/app/components/searchQueryBuilder/tokens/filter/filter.tsx index 3141e4051d5a..04e033fe0d4a 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(); @@ -94,6 +102,20 @@ function FilterValue({token, state, item, filterRef, onActiveChange}: FilterValu token={token} wrapperRef={ref} editingCommittedValue={editSource === 'click'} + isPendingValue={isPending} + onClose={() => { + setEditSource(false); + onActiveChange(false); + }} + onDiscard={() => { + setEditSource(false); + onActiveChange(false); + dispatch({ + type: 'DISCARD_PENDING_FILTER', + itemKey: item.key.toString(), + refocus: true, + }); + }} onDelete={() => { filterRef.current?.focus(); state.selectionManager.setFocusedKey(item.key); @@ -129,10 +151,11 @@ function FilterValue({token, state, item, filterRef, onActiveChange}: FilterValu }} disabled={disabled} showAllValues={showAllFilterValues} + isEmpty={isPending} {...filterButtonProps} > - + {isPending ? null : } ); } @@ -160,10 +183,51 @@ 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; + + // A pending filter has no value, so drop it once the user is done with it. + // Wait for focus to settle first: the operator menu moves focus out of the + // filter and back. + const discardIfAbandoned = useCallback(() => { + setTimeout(() => { + if ( + !ref.current || + filterMenuOpenRef.current || + ref.current.contains(document.activeElement) + ) { + return; + } + dispatch({ + type: 'DISCARD_PENDING_FILTER', + itemKey: item.key.toString(), + refocus: false, + }); + }); + }, [dispatch, item.key]); + + useEffect(() => { + if (isPending && !filterMenuOpen) { + discardIfAbandoned(); + } + }, [discardIfAbandoned, filterMenuOpen, isPending]); + + const {focusWithinProps} = useFocusWithin({ + onBlurWithin: () => { + if (isPending) { + discardIfAbandoned(); + } + }, + }); const {invalidFilterKeys, invalidFilterKeyMessage, filterKeyAliases} = useSearchQueryBuilderConfig(); const {showAllFilterValues} = useSearchQueryBuilderLayout(); @@ -184,9 +248,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 +305,14 @@ export function SearchQueryBuilderFilter({item, state, token}: SearchQueryTokenP item={item} token={token} state={state} - onActiveChange={setFilterMenuOpen} + onActiveChange={handleFilterMenuOpenChange} /> ) : ( )} @@ -249,7 +322,7 @@ export function SearchQueryBuilderFilter({item, state, token}: SearchQueryTokenP token={token} state={state} item={item} - onOpenChange={setFilterMenuOpen} + onOpenChange={handleFilterMenuOpenChange} /> @@ -258,7 +331,8 @@ export function SearchQueryBuilderFilter({item, state, token}: SearchQueryTokenP state={state} item={item} filterRef={ref} - onActiveChange={setFilterMenuOpen} + isPending={isPending} + onActiveChange={handleFilterMenuOpenChange} /> @@ -274,8 +348,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/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..1d6228eb3fc5 100644 --- a/static/app/components/searchQueryBuilder/tokens/filter/valueCombobox.tsx +++ b/static/app/components/searchQueryBuilder/tokens/filter/valueCombobox.tsx @@ -96,8 +96,22 @@ 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 and leaving it empty does not keep the placeholder. + */ + isPendingValue: boolean; + /** + * Closes the editor without committing, when a pending value is left empty + * because focus moved away. + */ + onClose: () => void; onCommit: () => void; onDelete: () => void; + /** + * Removes the pending filter, when its value is submitted or dismissed empty. + */ + onDiscard: () => void; token: TokenResult; wrapperRef: React.RefObject; }; @@ -681,8 +695,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 +718,13 @@ export function getInitialInputValue( export function SearchQueryBuilderValueCombobox({ token, + onClose, onDelete, onCommit, + onDiscard, wrapperRef, editingCommittedValue, + isPendingValue, }: SearchQueryValueBuilderProps) { const ref = useRef(null); const inputRef = useRef(null); @@ -744,7 +765,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 @@ -1244,7 +1270,16 @@ export function SearchQueryBuilderValueCombobox({ }; const handleInputValueConfirmed = useCallback( - (value: string) => { + (value: string, event?: React.FocusEvent) => { + if (isPendingValue && !value.trim()) { + if (event) { + onClose(); + } else { + onDiscard(); + } + return; + } + if (canSelectMultipleValues) { if (value.trim()) { addTypedValue(value); @@ -1262,7 +1297,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,8 +1334,11 @@ export function SearchQueryBuilderValueCombobox({ dispatch, editingCommittedValue, fieldDefinition, + isPendingValue, isRegexValue, + onClose, onCommit, + onDiscard, token, updateFilterValue, ] @@ -1443,7 +1486,11 @@ export function SearchQueryBuilderValueCombobox({ onExit={() => { setEditingChip(null); setInputValue(''); - onCommit(); + if (isPendingValue) { + onDiscard(); + } else { + onCommit(); + } }} inputValue={inputValue} filterValue={filterValue} diff --git a/static/app/components/searchQueryBuilder/tokens/utils.tsx b/static/app/components/searchQueryBuilder/tokens/utils.tsx index 18fe9bc318fd..19e7e0b1738d 100644 --- a/static/app/components/searchQueryBuilder/tokens/utils.tsx +++ b/static/app/components/searchQueryBuilder/tokens/utils.tsx @@ -177,52 +177,26 @@ export function resolveFilterKey({ return trimmedKey; } -/** - * Number based filters start without a value so the user picks one, rather than - * filtering by a placeholder they didn't choose. - */ export function getDefaultValueForValueType(valueType: FieldValueType | null): string { switch (valueType) { case FieldValueType.BOOLEAN: return 'true'; case FieldValueType.INTEGER: - case FieldValueType.NUMBER: - case FieldValueType.CURRENCY: - case FieldValueType.DURATION: - case FieldValueType.SIZE: - case FieldValueType.PERCENTAGE: - return ''; - case FieldValueType.DATE: - return '-24h'; - case FieldValueType.STRING: - default: - return '""'; - } -} - -/** - * Example values for number based filters, for callers that need a complete - * filter rather than one with an empty value. - */ -function getNumericExampleValue(fieldDefinition: FieldDefinition | null): string | null { - if (!fieldDefinition || defined(fieldDefinition.defaultValue)) { - return null; - } - - switch (fieldDefinition.valueType) { - case FieldValueType.INTEGER: case FieldValueType.NUMBER: return '100'; case FieldValueType.CURRENCY: return '10'; + case FieldValueType.DATE: + return '-24h'; case FieldValueType.DURATION: return '10ms'; case FieldValueType.SIZE: return '10bytes'; case FieldValueType.PERCENTAGE: return '0.5'; + case FieldValueType.STRING: default: - return null; + return '""'; } } @@ -276,14 +250,12 @@ function getInitialValueType(fieldDefinition: FieldDefinition | null) { export function getInitialFilterText( key: string, fieldDefinition: FieldDefinition | null, - operator: TermOperator = TermOperator.GREATER_THAN, - {withNumericExampleValue = false}: {withNumericExampleValue?: boolean} = {} + operator: TermOperator = TermOperator.GREATER_THAN ) { + const defaultValue = getDefaultFilterValue({fieldDefinition}); + const keyText = getInitialFilterKeyText(key, fieldDefinition); const valueType = getInitialValueType(fieldDefinition); - const defaultValue = - (withNumericExampleValue ? getNumericExampleValue(fieldDefinition) : null) ?? - getDefaultFilterValue({fieldDefinition}); // Array attributes filter by membership. `addArrayMembershipOperator` places // the `[*]` operator (inside the bracket for the tag form, trailing for the diff --git a/static/app/components/searchSyntax/parser.spec.tsx b/static/app/components/searchSyntax/parser.spec.tsx index 97789333fadf..dcf686dbe30a 100644 --- a/static/app/components/searchSyntax/parser.spec.tsx +++ b/static/app/components/searchSyntax/parser.spec.tsx @@ -261,33 +261,6 @@ describe('searchSyntax/parser', () => { expect(filterOf('avg(span.size):>1kb')).toBeUndefined(); }); - it.each([ - ['num:>', FilterType.NUMERIC, TermOperator.GREATER_THAN], - ['!num:<=', FilterType.NUMERIC, TermOperator.LESS_THAN_EQUAL], - ['num:', FilterType.NUMERIC, TermOperator.DEFAULT], - ['span.duration:>', FilterType.DURATION, TermOperator.GREATER_THAN], - ['span.size:<', FilterType.SIZE, TermOperator.LESS_THAN], - ['count():>', FilterType.AGGREGATE_NUMERIC, TermOperator.GREATER_THAN], - [ - 'p95(span.duration):>=', - FilterType.AGGREGATE_DURATION, - TermOperator.GREATER_THAN_EQUAL, - ], - ])('parses %s as a filter that must have a value', (query, filterType, operator) => { - const filter = parseSearch(query, { - numericKeys: new Set(['num']), - durationKeys: new Set(['span.duration']), - sizeKeys: new Set(['span.size']), - })?.find((token): token is TokenResult => token.type === Token.FILTER); - - expect(filter?.filter).toBe(filterType); - expect(filter?.operator).toBe(operator); - expect(filter?.value.text).toBe(''); - expect(filter?.invalid).toEqual( - expect.objectContaining({type: InvalidReason.FILTER_MUST_HAVE_VALUE}) - ); - }); - it('returns the matched text for keys, values, and aggregate params', () => { const result = parseSearch( 'tags[foo:bar]:baz browser.name-1:"Fire \\"fox\\"" device.family: count_if(user.email,equals,"I, got \\"quotes\\""):>100 firstSeen:+7d' diff --git a/static/app/components/searchSyntax/parser.tsx b/static/app/components/searchSyntax/parser.tsx index 1ac534dbb10c..4c4ca55228d1 100644 --- a/static/app/components/searchSyntax/parser.tsx +++ b/static/app/components/searchSyntax/parser.tsx @@ -331,25 +331,6 @@ type FilterMap = { type TextFilter = FilterMap[FilterType.TEXT]; type InFilter = FilterMap[FilterType.TEXT_IN] | FilterMap[FilterType.NUMERIC_IN]; - -/** - * Filter types whose value can be left empty after a comparison operator, - * such as `span.duration:>`. - */ -const COMPARISON_FILTER_TYPES = new Set([ - FilterType.NUMERIC, - FilterType.DURATION, - FilterType.SIZE, - FilterType.AGGREGATE_NUMERIC, - FilterType.AGGREGATE_DURATION, - FilterType.AGGREGATE_SIZE, - FilterType.AGGREGATE_PERCENTAGE, -]); -type ComparisonFilterValue = FilterMap[ - | FilterType.NUMERIC - | FilterType.DURATION - | FilterType.SIZE - | FilterType.AGGREGATE_PERCENTAGE]['value']; type AggregateFilterType = | FilterMap[FilterType.AGGREGATE_DATE] | FilterMap[FilterType.AGGREGATE_DURATION] @@ -697,7 +678,7 @@ export class TokenConverter { type: Token.VALUE_DURATION as const, value, - parsed: this.config.parse && value !== '' ? parseDuration(value, unit) : undefined, + parsed: this.config.parse ? parseDuration(value, unit) : undefined, unit, }); @@ -730,7 +711,7 @@ export class TokenConverter { value, // units are case insensitive, normalize them in their parsed representation // so that we dont have to compare all possible permutations. - parsed: this.config.parse && value !== '' ? parseSize(value, unit) : undefined, + parsed: this.config.parse ? parseSize(value, unit) : undefined, unit, }); @@ -738,7 +719,7 @@ export class TokenConverter { ...this.defaultTokenFields, type: Token.VALUE_PERCENTAGE as const, value, - parsed: this.config.parse && value !== '' ? parsePercentage(value) : undefined, + parsed: this.config.parse ? parsePercentage(value) : undefined, }); tokenValueBoolean = (value: string) => ({ @@ -754,7 +735,7 @@ export class TokenConverter { type: Token.VALUE_NUMBER as const, value, unit, - parsed: this.config.parse && value !== '' ? parseNumber(value, unit) : undefined, + parsed: this.config.parse ? parseNumber(value, unit) : undefined, }; }; @@ -993,16 +974,6 @@ export class TokenConverter { return this.checkInvalidInFilter(value as InFilter['value']); } - if ( - COMPARISON_FILTER_TYPES.has(filter) && - (value as ComparisonFilterValue).value === '' - ) { - return { - type: InvalidReason.FILTER_MUST_HAVE_VALUE, - reason: this.config.invalidMessages[InvalidReason.FILTER_MUST_HAVE_VALUE], - }; - } - if ('name' in key) { return this.checkInvalidAggregateKey(key); } diff --git a/static/app/views/dashboards/globalFilter/addFilter.tsx b/static/app/views/dashboards/globalFilter/addFilter.tsx index d8a8f58be9dd..7b034069dcdd 100644 --- a/static/app/views/dashboards/globalFilter/addFilter.tsx +++ b/static/app/views/dashboards/globalFilter/addFilter.tsx @@ -183,9 +183,7 @@ export function AddFilter({ if (valueType && !IGNORE_DEFAULT_VALUES.includes(valueType)) { defaultFilterValue = getInitialFilterText( selectedFilterKey.key, - fieldDefinition, - undefined, - {withNumericExampleValue: true} + fieldDefinition ); } diff --git a/static/app/views/dashboards/globalFilter/utils.tsx b/static/app/views/dashboards/globalFilter/utils.tsx index 88cd2e1d3ca5..5271a7a3fb45 100644 --- a/static/app/views/dashboards/globalFilter/utils.tsx +++ b/static/app/views/dashboards/globalFilter/utils.tsx @@ -118,9 +118,7 @@ export function getFilterToken( const {tag, value} = globalFilter; let filterValue = value; if (value === '') { - filterValue = getInitialFilterText(tag.key, fieldDefinition, undefined, { - withNumericExampleValue: true, - }); + filterValue = getInitialFilterText(tag.key, fieldDefinition); } const filterTokens = parseFilterValue(filterValue, globalFilter); return filterTokens[0] ?? null; diff --git a/static/app/views/explore/spans/spansTab.spec.tsx b/static/app/views/explore/spans/spansTab.spec.tsx index 8d578523ac03..b489c5947d23 100644 --- a/static/app/views/explore/spans/spansTab.spec.tsx +++ b/static/app/views/explore/spans/spansTab.spec.tsx @@ -649,7 +649,7 @@ describe('SpansTabContent', () => { const input = screen.getByRole('combobox'); await userEvent.click(input); - await userEvent.type(input, 'span.duration:10ms{enter}'); + await userEvent.type(input, 'span.duration:>10ms{enter}'); // re-open the combobox await userEvent.click(input); diff --git a/static/packages/search-syntax/src/grammar.pegjs b/static/packages/search-syntax/src/grammar.pegjs index f444bb898b92..2ffab55dd862 100644 --- a/static/packages/search-syntax/src/grammar.pegjs +++ b/static/packages/search-syntax/src/grammar.pegjs @@ -106,7 +106,7 @@ rel_date_filter // filter for durations duration_filter - = negation:negation? key:search_key sep op:operator? value:(duration_format / empty_duration_value) &{ + = negation:negation? key:search_key sep op:operator? value:duration_format &{ return tc.predicateFilter(FilterType.DURATION, key) } { return tc.tokenFilter(FilterType.DURATION, key, value, op, !!negation); @@ -114,7 +114,7 @@ duration_filter // filter for file size size_filter - = negation:negation? key:search_key sep op:operator? value:(size_format / empty_size_value) &{ + = negation:negation? key:search_key sep op:operator? value:size_format &{ return tc.predicateFilter(FilterType.SIZE, key) } { return tc.tokenFilter(FilterType.SIZE, key, value, op, !!negation); @@ -138,7 +138,7 @@ numeric_in_filter // numeric comparison filter numeric_filter - = negation:negation? key:search_key sep op:operator? value:(numeric_value / empty_numeric_value) &{ + = negation:negation? key:search_key sep op:operator? value:numeric_value &{ return tc.predicateFilter(FilterType.NUMERIC, key) } { return tc.tokenFilter(FilterType.NUMERIC, key, value, op, !!negation); @@ -146,7 +146,7 @@ numeric_filter // aggregate duration filter aggregate_duration_filter - = negation:negation? key:aggregate_key sep op:operator? value:(duration_format / empty_duration_value) &{ + = negation:negation? key:aggregate_key sep op:operator? value:duration_format &{ return tc.predicateFilter(FilterType.AGGREGATE_DURATION, key) } { return tc.tokenFilter(FilterType.AGGREGATE_DURATION, key, value, op, !!negation); @@ -154,7 +154,7 @@ aggregate_duration_filter // aggregate file size filter aggregate_size_filter - = negation:negation? key:aggregate_key sep op:operator? value:(size_format / empty_size_value) &{ + = negation:negation? key:aggregate_key sep op:operator? value:size_format &{ return tc.predicateFilter(FilterType.AGGREGATE_SIZE, key) } { return tc.tokenFilter(FilterType.AGGREGATE_SIZE, key, value, op, !!negation); @@ -162,7 +162,7 @@ aggregate_size_filter // aggregate percentage filter aggregate_percentage_filter - = negation:negation? key:aggregate_key sep op:operator? value:(percentage_format / empty_percentage_value) &{ + = negation:negation? key:aggregate_key sep op:operator? value:percentage_format &{ return tc.predicateFilter(FilterType.AGGREGATE_PERCENTAGE, key) } { return tc.tokenFilter(FilterType.AGGREGATE_PERCENTAGE, key, value, op, !!negation); @@ -170,7 +170,7 @@ aggregate_percentage_filter // aggregate numeric filter aggregate_numeric_filter - = negation:negation? key:aggregate_key sep op:operator? value:(numeric_value / empty_numeric_value) &{ + = negation:negation? key:aggregate_key sep op:operator? value:numeric_value &{ return tc.predicateFilter(FilterType.AGGREGATE_NUMERIC, key) } { return tc.tokenFilter(FilterType.AGGREGATE_NUMERIC, key, value, op, !!negation); @@ -508,28 +508,6 @@ percentage_format return tc.tokenValuePercentage(value); } -// Number based filters may have an empty value, e.g. `span.duration:>`, which -// is how the search query builder represents a filter that has no value yet. -empty_numeric_value - = &end_value { - return tc.tokenValueNumber('', null); - } - -empty_duration_value - = &end_value { - return tc.tokenValueDuration('', null); - } - -empty_size_value - = &end_value { - return tc.tokenValueSize('', null); - } - -empty_percentage_value - = &end_value { - return tc.tokenValuePercentage(''); - } - // Units for special values number_unit = "k"i/"m"i/"b"i From 15a61dee95e312bcddd2340b0102b923be05a1ea Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Fri, 30 Oct 2026 18:51:53 +0000 Subject: [PATCH 3/9] ref(search-query-builder): Keep the placeholder when leaving an empty number filter Leaving a new number filter without a value now commits it with its example value instead of removing it. Co-Authored-By: Nicholas Deschenes --- .../hooks/useQueryBuilderState.tsx | 43 +++-------- .../searchQueryBuilder/index.spec.tsx | 71 ++++++++++--------- .../tokens/filter/filter.tsx | 34 ++++----- .../tokens/filter/valueCombobox.tsx | 18 ++--- 4 files changed, 62 insertions(+), 104 deletions(-) diff --git a/static/app/components/searchQueryBuilder/hooks/useQueryBuilderState.tsx b/static/app/components/searchQueryBuilder/hooks/useQueryBuilderState.tsx index b6a3a4057dc7..a390e4d2d58f 100644 --- a/static/app/components/searchQueryBuilder/hooks/useQueryBuilderState.tsx +++ b/static/app/components/searchQueryBuilder/hooks/useQueryBuilderState.tsx @@ -61,9 +61,8 @@ type QueryBuilderState = { /** * 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 never - * committed: it is replaced when the user enters a value, and the filter is - * removed if they leave it without one. + * 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; @@ -90,17 +89,12 @@ type UpdateQueryAction = { type ResetFocusOverrideAction = {type: 'RESET_FOCUS_OVERRIDE'}; -type DiscardPendingFilterAction = { +type CommitPendingFilterAction = { /** - * Only discard the filter if it is still the pending one. + * Only commit the filter if it is still the pending one. */ itemKey: string; - /** - * Move focus to where the filter was. Leave this off when the user has already - * moved focus somewhere else. - */ - refocus: boolean; - type: 'DISCARD_PENDING_FILTER'; + type: 'COMMIT_PENDING_FILTER'; }; type DeleteTokenAction = { @@ -275,7 +269,7 @@ export type QueryBuilderActions = | CommitQueryAction | UpdateQueryAction | ResetFocusOverrideAction - | DiscardPendingFilterAction + | CommitPendingFilterAction | DeleteTokenAction | DeleteTokensAction | DeleteToCursorAction @@ -1206,8 +1200,7 @@ function getNextPendingFilterKey( if ( action.type === 'CLEAR' || action.type === 'UPDATE_QUERY' || - (action.type === 'DISCARD_PENDING_FILTER' && - action.itemKey === state.pendingFilterKey) + (action.type === 'COMMIT_PENDING_FILTER' && action.itemKey === state.pendingFilterKey) ) { return null; } @@ -1392,29 +1385,11 @@ export function useQueryBuilderState({ ...state, focusOverride: null, }; - case 'DISCARD_PENDING_FILTER': { + case 'COMMIT_PENDING_FILTER': if (action.itemKey !== state.pendingFilterKey) { return state; } - const pendingFilter = findPendingFilter( - state.query, - state.pendingFilterKey, - parseQuery - ); - if (!pendingFilter) { - return state; - } - const newState = replaceTokensWithText(state, { - tokens: [pendingFilter], - text: '', - parseQuery, - shouldCommitQuery: true, - }); - return { - ...newState, - focusOverride: action.refocus ? newState.focusOverride : null, - }; - } + return {...state, committedQuery: state.query}; case 'DELETE_TOKEN': { return { ...replaceTokensWithText(state, { diff --git a/static/app/components/searchQueryBuilder/index.spec.tsx b/static/app/components/searchQueryBuilder/index.spec.tsx index 656636f0bb63..f1666c6b22b5 100644 --- a/static/app/components/searchQueryBuilder/index.spec.tsx +++ b/static/app/components/searchQueryBuilder/index.spec.tsx @@ -1723,23 +1723,20 @@ describe('SearchQueryBuilder', () => { }); it.each([ - ['>', 'timesSeen:>'], - ['<', 'timesSeen:<'], - ['=', 'timesSeen:='], + ['>', 'timesSeen:>100'], + ['<', 'timesSeen:<100'], + ['=', 'timesSeen:=100'], ])( - 'creates a numeric filter without a value when typing %s', - async (operator, expectedFilter) => { + 'adds default value for numeric filter when typing %s', + async (operator, expectedQuery) => { render(, { organization: numberOperatorConversionOrganization, }); await userEvent.click(getLastInput()); - await userEvent.keyboard(`timesSeen${operator}`); + await userEvent.keyboard(`timesSeen${operator}{Escape}`); - expect( - await screen.findByRole('row', {name: expectedFilter}) - ).toBeInTheDocument(); - expect(screen.getByRole('combobox', {name: 'Edit filter value'})).toHaveValue(''); + expect(await screen.findByRole('row', {name: expectedQuery})).toBeInTheDocument(); } ); @@ -1752,16 +1749,17 @@ describe('SearchQueryBuilder', () => { expect(getLastInput()).toHaveValue('timesSeen>'); }); - it('creates numeric tag filters from filter key metadata', async () => { + it('adds default value for numeric tag filters from filter key metadata', async () => { render(, { organization: numberOperatorConversionOrganization, }); await userEvent.click(getLastInput()); await userEvent.paste('tags[bar,number]>'); + await userEvent.keyboard('{Escape}'); expect( - await screen.findByRole('row', {name: 'tags[bar,number]:>'}) + await screen.findByRole('row', {name: 'tags[bar,number]:>100'}) ).toBeInTheDocument(); }); @@ -5395,23 +5393,27 @@ describe('SearchQueryBuilder', () => { it.each([ ['escape', '{Escape}'], ['enter', '{Enter}'], - ])('removes the filter when %s is pressed without a value', async (_, key) => { - const mockOnChange = jest.fn(); - render(); - await addDurationFilter(); + ])( + 'keeps the placeholder value when %s is pressed without a value', + async (_, key) => { + const mockOnChange = jest.fn(); + render(); + await addDurationFilter(); - await userEvent.keyboard(key); + await userEvent.keyboard(key); - await waitFor(() => { expect( - screen.queryByRole('row', {name: 'duration:>'}) - ).not.toBeInTheDocument(); - }); - expect(getLastInput()).toHaveFocus(); - expect(mockOnChange).not.toHaveBeenCalled(); - }); + await screen.findByRole('row', {name: 'duration:>10ms'}) + ).toBeInTheDocument(); + expect(mockOnChange).toHaveBeenCalledTimes(1); + expect(mockOnChange).toHaveBeenCalledWith( + 'duration:>10ms', + expect.anything() + ); + } + ); - it('does not bring back the placeholder value on undo', async () => { + it('skips the filter without a value on undo', async () => { render(); await addDurationFilter(); await userEvent.keyboard('50ms{Enter}'); @@ -5429,19 +5431,18 @@ describe('SearchQueryBuilder', () => { expect(screen.queryByRole('row', {name: /^duration:/})).not.toBeInTheDocument(); }); - it('removes the filter when focus leaves it without a value', async () => { + it('keeps the placeholder value when focus leaves the filter', async () => { const mockOnChange = jest.fn(); render(); await addDurationFilter(); await userEvent.click(document.body); - await waitFor(() => { - expect( - screen.queryByRole('row', {name: 'duration:>'}) - ).not.toBeInTheDocument(); - }); - expect(mockOnChange).not.toHaveBeenCalled(); + expect( + await screen.findByRole('row', {name: 'duration:>10ms'}) + ).toBeInTheDocument(); + expect(mockOnChange).toHaveBeenCalledTimes(1); + expect(mockOnChange).toHaveBeenCalledWith('duration:>10ms', expect.anything()); }); }); @@ -7320,7 +7321,7 @@ describe('SearchQueryBuilder', () => { render(); await userEvent.click(getLastInput()); - await userEvent.keyboard('bar:100{Enter}'); + await userEvent.keyboard('bar:{Escape}'); expect( screen.getByRole('button', {name: 'Edit key for filter: tags[bar,number]'}) @@ -7333,7 +7334,7 @@ describe('SearchQueryBuilder', () => { }); await userEvent.click(getLastInput()); - await userEvent.keyboard('bar>100{Enter}'); + await userEvent.keyboard('bar>{Escape}'); expect( screen.getByRole('button', {name: 'Edit key for filter: tags[bar,number]'}) @@ -7380,7 +7381,7 @@ describe('SearchQueryBuilder', () => { screen.getByRole('button', {name: 'Edit key for filter: browser.name'}) ); await userEvent.clear(screen.getByRole('combobox', {name: 'Edit filter key'})); - await userEvent.keyboard('bar{Enter}100{Enter}'); + await userEvent.keyboard('bar{Enter}{Escape}'); expect( screen.getByRole('button', {name: 'Edit key for filter: tags[bar,number]'}) diff --git a/static/app/components/searchQueryBuilder/tokens/filter/filter.tsx b/static/app/components/searchQueryBuilder/tokens/filter/filter.tsx index 04e033fe0d4a..73d3d40cb243 100644 --- a/static/app/components/searchQueryBuilder/tokens/filter/filter.tsx +++ b/static/app/components/searchQueryBuilder/tokens/filter/filter.tsx @@ -107,15 +107,6 @@ function FilterValue({ setEditSource(false); onActiveChange(false); }} - onDiscard={() => { - setEditSource(false); - onActiveChange(false); - dispatch({ - type: 'DISCARD_PENDING_FILTER', - itemKey: item.key.toString(), - refocus: true, - }); - }} onDelete={() => { filterRef.current?.focus(); state.selectionManager.setFocusedKey(item.key); @@ -125,6 +116,11 @@ function FilterValue({ 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 ( @@ -195,10 +191,10 @@ export function SearchQueryBuilderFilter({item, state, token}: SearchQueryTokenP const {dispatch, pendingFilterKey} = useSearchQueryBuilderState(); const isPending = pendingFilterKey === item.key; - // A pending filter has no value, so drop it once the user is done with it. - // Wait for focus to settle first: the operator menu moves focus out of the - // filter and back. - const discardIfAbandoned = useCallback(() => { + // 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 || @@ -207,24 +203,20 @@ export function SearchQueryBuilderFilter({item, state, token}: SearchQueryTokenP ) { return; } - dispatch({ - type: 'DISCARD_PENDING_FILTER', - itemKey: item.key.toString(), - refocus: false, - }); + dispatch({type: 'COMMIT_PENDING_FILTER', itemKey: item.key.toString()}); }); }, [dispatch, item.key]); useEffect(() => { if (isPending && !filterMenuOpen) { - discardIfAbandoned(); + commitIfAbandoned(); } - }, [discardIfAbandoned, filterMenuOpen, isPending]); + }, [commitIfAbandoned, filterMenuOpen, isPending]); const {focusWithinProps} = useFocusWithin({ onBlurWithin: () => { if (isPending) { - discardIfAbandoned(); + commitIfAbandoned(); } }, }); diff --git a/static/app/components/searchQueryBuilder/tokens/filter/valueCombobox.tsx b/static/app/components/searchQueryBuilder/tokens/filter/valueCombobox.tsx index 1d6228eb3fc5..d3b1eabbf11b 100644 --- a/static/app/components/searchQueryBuilder/tokens/filter/valueCombobox.tsx +++ b/static/app/components/searchQueryBuilder/tokens/filter/valueCombobox.tsx @@ -98,20 +98,16 @@ type SearchQueryValueBuilderProps = { editingCommittedValue: boolean; /** * The filter is pending: its value is only a placeholder, so the input starts - * empty and leaving it empty does not keep the placeholder. + * empty. */ isPendingValue: boolean; /** * Closes the editor without committing, when a pending value is left empty - * because focus moved away. + * because focus moved away. The filter decides what to do once focus settles. */ onClose: () => void; onCommit: () => void; onDelete: () => void; - /** - * Removes the pending filter, when its value is submitted or dismissed empty. - */ - onDiscard: () => void; token: TokenResult; wrapperRef: React.RefObject; }; @@ -721,7 +717,6 @@ export function SearchQueryBuilderValueCombobox({ onClose, onDelete, onCommit, - onDiscard, wrapperRef, editingCommittedValue, isPendingValue, @@ -1275,7 +1270,7 @@ export function SearchQueryBuilderValueCombobox({ if (event) { onClose(); } else { - onDiscard(); + onCommit(); } return; } @@ -1338,7 +1333,6 @@ export function SearchQueryBuilderValueCombobox({ isRegexValue, onClose, onCommit, - onDiscard, token, updateFilterValue, ] @@ -1486,11 +1480,7 @@ export function SearchQueryBuilderValueCombobox({ onExit={() => { setEditingChip(null); setInputValue(''); - if (isPendingValue) { - onDiscard(); - } else { - onCommit(); - } + onCommit(); }} inputValue={inputValue} filterValue={filterValue} From c2b2a5032671645f4761c986b2dba31283b69ce4 Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Mon, 2 Nov 2026 12:03:37 +0000 Subject: [PATCH 4/9] fix(search-query-builder): Remove a pending filter when its remove button is pressed Pressing the remove button closes the empty filter's value editor first, and the click could miss the button. The filter was left behind without a value and got the placeholder value on blur. Co-Authored-By: Nicholas Deschenes --- .../searchQueryBuilder/index.spec.tsx | 41 +++++++++++++++++++ .../tokens/filter/filter.tsx | 18 +++++++- 2 files changed, 57 insertions(+), 2 deletions(-) diff --git a/static/app/components/searchQueryBuilder/index.spec.tsx b/static/app/components/searchQueryBuilder/index.spec.tsx index f1666c6b22b5..f857548dda0a 100644 --- a/static/app/components/searchQueryBuilder/index.spec.tsx +++ b/static/app/components/searchQueryBuilder/index.spec.tsx @@ -5444,6 +5444,47 @@ describe('SearchQueryBuilder', () => { 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('pre-fills input with current value when editing', async () => { diff --git a/static/app/components/searchQueryBuilder/tokens/filter/filter.tsx b/static/app/components/searchQueryBuilder/tokens/filter/filter.tsx index 73d3d40cb243..017d47e3b1cc 100644 --- a/static/app/components/searchQueryBuilder/tokens/filter/filter.tsx +++ b/static/app/components/searchQueryBuilder/tokens/filter/filter.tsx @@ -156,7 +156,12 @@ function FilterValue({ ); } -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}); @@ -164,6 +169,15 @@ function FilterDelete({token, state, item}: SearchQueryTokenProps) { return ( { + // 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(); + dispatch({type: 'DELETE_TOKEN', token}); + } + }} onClick={() => { dispatch({type: 'DELETE_TOKEN', token}); }} @@ -328,7 +342,7 @@ export function SearchQueryBuilderFilter({item, state, token}: SearchQueryTokenP /> - + From 23c39d440b3078317f318d8d2b9510b80f4c7ac7 Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Mon, 2 Nov 2026 12:58:05 +0000 Subject: [PATCH 5/9] fix(search-query-builder): Clear the query without committing a pending filter Pressing the clear button moved focus out of an empty number filter first, which committed its placeholder value and ran a search before the query was cleared. Co-Authored-By: Nicholas Deschenes --- .../searchQueryBuilder/index.spec.tsx | 38 +++++++++++++++++++ .../components/searchQueryBuilder/index.tsx | 11 +++++- 2 files changed, 48 insertions(+), 1 deletion(-) diff --git a/static/app/components/searchQueryBuilder/index.spec.tsx b/static/app/components/searchQueryBuilder/index.spec.tsx index f857548dda0a..ee20b5937303 100644 --- a/static/app/components/searchQueryBuilder/index.spec.tsx +++ b/static/app/components/searchQueryBuilder/index.spec.tsx @@ -5485,6 +5485,44 @@ describe('SearchQueryBuilder', () => { 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('pre-fills input with current value when editing', async () => { 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()} /> )} From a28ee53b22719c921ef9024ff92f3baecc924298 Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Mon, 2 Nov 2026 16:47:16 +0000 Subject: [PATCH 6/9] fix(search-query-builder): Fix review issues with pending number filters - Removing a pending filter on press no longer also removes the next filter, which reuses the remove button through positional item keys. - Changing a pending filter's operator to "is" no longer keeps its placeholder value as a selected value. - Rejecting an invalid value on blur marks the value editor closed, so the pending filter gets its placeholder value instead of staying empty. - Searching from the free text input uses the committed query while a filter is pending. Co-Authored-By: Nicholas Deschenes --- .../searchQueryBuilder/index.spec.tsx | 75 +++++++++++++++++++ .../tokens/filter/filter.tsx | 16 +++- .../tokens/filter/valueCombobox.tsx | 5 +- .../searchQueryBuilder/tokens/freeText.tsx | 6 +- 4 files changed, 97 insertions(+), 5 deletions(-) diff --git a/static/app/components/searchQueryBuilder/index.spec.tsx b/static/app/components/searchQueryBuilder/index.spec.tsx index ee20b5937303..fba22bfc5463 100644 --- a/static/app/components/searchQueryBuilder/index.spec.tsx +++ b/static/app/components/searchQueryBuilder/index.spec.tsx @@ -5523,6 +5523,81 @@ describe('SearchQueryBuilder', () => { ); } ); + + 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(); + }); + + it('commits the placeholder value after an invalid value is rejected', async () => { + const mockOnChange = 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()); + }); }); it('pre-fills input with current value when editing', async () => { diff --git a/static/app/components/searchQueryBuilder/tokens/filter/filter.tsx b/static/app/components/searchQueryBuilder/tokens/filter/filter.tsx index 017d47e3b1cc..7c6aa503166d 100644 --- a/static/app/components/searchQueryBuilder/tokens/filter/filter.tsx +++ b/static/app/components/searchQueryBuilder/tokens/filter/filter.tsx @@ -88,7 +88,10 @@ function FilterValue({ ) { 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); }, }); @@ -165,20 +168,31 @@ function FilterDelete({ 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 ( { + 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={() => { + 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} diff --git a/static/app/components/searchQueryBuilder/tokens/filter/valueCombobox.tsx b/static/app/components/searchQueryBuilder/tokens/filter/valueCombobox.tsx index d3b1eabbf11b..416b9d4bd371 100644 --- a/static/app/components/searchQueryBuilder/tokens/filter/valueCombobox.tsx +++ b/static/app/components/searchQueryBuilder/tokens/filter/valueCombobox.tsx @@ -788,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 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; } From 6e2e9e321c4e54545205881a8030ca5b844eb1e7 Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Mon, 2 Nov 2026 16:50:48 +0000 Subject: [PATCH 7/9] test(search-query-builder): Check search after a rejected pending value After an invalid value is rejected and the pending filter commits its placeholder, searching from the input uses the value the filter shows. Co-Authored-By: Nicholas Deschenes --- .../searchQueryBuilder/index.spec.tsx | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/static/app/components/searchQueryBuilder/index.spec.tsx b/static/app/components/searchQueryBuilder/index.spec.tsx index fba22bfc5463..18221a28ea90 100644 --- a/static/app/components/searchQueryBuilder/index.spec.tsx +++ b/static/app/components/searchQueryBuilder/index.spec.tsx @@ -5586,7 +5586,14 @@ describe('SearchQueryBuilder', () => { it('commits the placeholder value after an invalid value is rejected', async () => { const mockOnChange = jest.fn(); - render(); + const mockOnSearch = jest.fn(); + render( + + ); await addDurationFilter(); await userEvent.keyboard('abc'); @@ -5597,6 +5604,14 @@ describe('SearchQueryBuilder', () => { ).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() + ); }); }); From 16dc0a5e493e6a21d47971340961db934eb1b131 Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Mon, 2 Nov 2026 18:55:33 +0000 Subject: [PATCH 8/9] fix(search-query-builder): Keep a pending filter pending when its key changes Changing the key of a number filter without a value to another key of the same type committed the query and closed the key editor without returning focus to the filter. Committing removed the pending filter from the query and could leave a dangling Boolean operator (`OR device:foo`), and the focus loss then committed the placeholder value. While the filter is pending, the key change no longer commits the query and returns focus to the value editor, like an operator change does. Co-Authored-By: Nicholas Deschenes --- .../hooks/useQueryBuilderState.tsx | 5 +- .../searchQueryBuilder/index.spec.tsx | 92 +++++++++++++++++++ .../tokens/filter/filterKeyCombobox.tsx | 20 +++- 3 files changed, 114 insertions(+), 3 deletions(-) diff --git a/static/app/components/searchQueryBuilder/hooks/useQueryBuilderState.tsx b/static/app/components/searchQueryBuilder/hooks/useQueryBuilderState.tsx index a390e4d2d58f..4c1f3bae41a9 100644 --- a/static/app/components/searchQueryBuilder/hooks/useQueryBuilderState.tsx +++ b/static/app/components/searchQueryBuilder/hooks/useQueryBuilderState.tsx @@ -211,6 +211,8 @@ type UpdateFilterKeyAction = { key: string; token: TokenResult; type: 'UPDATE_FILTER_KEY'; + focusOverride?: FocusOverride; + shouldCommitQuery?: boolean; }; type UpdateFilterOpAction = { @@ -949,7 +951,8 @@ function updateFilterKey( return { ...state, query: newQuery, - committedQuery: newQuery, + committedQuery: (action.shouldCommitQuery ?? true) ? newQuery : state.committedQuery, + focusOverride: action.focusOverride ?? state.focusOverride, }; } diff --git a/static/app/components/searchQueryBuilder/index.spec.tsx b/static/app/components/searchQueryBuilder/index.spec.tsx index 18221a28ea90..b103e6ffafc4 100644 --- a/static/app/components/searchQueryBuilder/index.spec.tsx +++ b/static/app/components/searchQueryBuilder/index.spec.tsx @@ -5584,6 +5584,98 @@ describe('SearchQueryBuilder', () => { 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('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(); diff --git a/static/app/components/searchQueryBuilder/tokens/filter/filterKeyCombobox.tsx b/static/app/components/searchQueryBuilder/tokens/filter/filterKeyCombobox.tsx index 144e37ca22a7..274900707647 100644 --- a/static/app/components/searchQueryBuilder/tokens/filter/filterKeyCombobox.tsx +++ b/static/app/components/searchQueryBuilder/tokens/filter/filterKeyCombobox.tsx @@ -42,7 +42,8 @@ export function FilterKeyCombobox({token, onCommit, item}: KeyComboboxProps) { }); const {filterKeys, getFieldDefinition, getSuggestedFilterKey} = useSearchQueryBuilderConfig(); - const {dispatch} = useSearchQueryBuilderState(); + const {dispatch, pendingFilterKey} = useSearchQueryBuilderState(); + const isPending = pendingFilterKey === item.key; const currentFilterValueType = getFilterValueType( token, @@ -73,6 +74,13 @@ export function FilterKeyCombobox({token, onCommit, item}: KeyComboboxProps) { type: 'UPDATE_FILTER_KEY', token, key: keyName, + // A filter without a value stays pending, so go back to its value + ...(isPending + ? { + focusOverride: {itemKey: item.key.toString(), part: 'value'}, + shouldCommitQuery: false, + } + : {}), }); onCommit(); return; @@ -90,7 +98,15 @@ export function FilterKeyCombobox({token, onCommit, item}: KeyComboboxProps) { onCommit(); }, - [currentFilterValueType, dispatch, getFieldDefinition, item.key, onCommit, token] + [ + currentFilterValueType, + dispatch, + getFieldDefinition, + isPending, + item.key, + onCommit, + token, + ] ); const onOptionSelected = useCallback( From b059dbda2aac35458560e84f6235bb2adb0e432f Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Mon, 2 Nov 2026 19:16:55 +0000 Subject: [PATCH 9/9] fix(search-query-builder): Return to a pending filter's value when the key editor closes Selecting the same key, pressing Escape, or committing an empty key input closed the key editor of a number filter without a value and moved focus out of the filter, which committed its placeholder value. Every key editor close except blur now sends focus back to the pending filter's value through a new SET_FOCUS_OVERRIDE action, which also replaces the focus override added to UPDATE_FILTER_KEY. Co-Authored-By: Nicholas Deschenes --- .../hooks/useQueryBuilderState.tsx | 13 ++++- .../searchQueryBuilder/index.spec.tsx | 49 +++++++++++++++++++ .../tokens/filter/filterKeyCombobox.tsx | 34 ++++++++----- 3 files changed, 81 insertions(+), 15 deletions(-) diff --git a/static/app/components/searchQueryBuilder/hooks/useQueryBuilderState.tsx b/static/app/components/searchQueryBuilder/hooks/useQueryBuilderState.tsx index 4c1f3bae41a9..cc0dc47a9930 100644 --- a/static/app/components/searchQueryBuilder/hooks/useQueryBuilderState.tsx +++ b/static/app/components/searchQueryBuilder/hooks/useQueryBuilderState.tsx @@ -89,6 +89,11 @@ 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. @@ -211,7 +216,6 @@ type UpdateFilterKeyAction = { key: string; token: TokenResult; type: 'UPDATE_FILTER_KEY'; - focusOverride?: FocusOverride; shouldCommitQuery?: boolean; }; @@ -271,6 +275,7 @@ export type QueryBuilderActions = | CommitQueryAction | UpdateQueryAction | ResetFocusOverrideAction + | SetFocusOverrideAction | CommitPendingFilterAction | DeleteTokenAction | DeleteTokensAction @@ -952,7 +957,6 @@ function updateFilterKey( ...state, query: newQuery, committedQuery: (action.shouldCommitQuery ?? true) ? newQuery : state.committedQuery, - focusOverride: action.focusOverride ?? state.focusOverride, }; } @@ -1388,6 +1392,11 @@ 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; diff --git a/static/app/components/searchQueryBuilder/index.spec.tsx b/static/app/components/searchQueryBuilder/index.spec.tsx index b103e6ffafc4..fa519376e8d4 100644 --- a/static/app/components/searchQueryBuilder/index.spec.tsx +++ b/static/app/components/searchQueryBuilder/index.spec.tsx @@ -5646,6 +5646,55 @@ describe('SearchQueryBuilder', () => { 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(); diff --git a/static/app/components/searchQueryBuilder/tokens/filter/filterKeyCombobox.tsx b/static/app/components/searchQueryBuilder/tokens/filter/filterKeyCombobox.tsx index 274900707647..c1c8e3382919 100644 --- a/static/app/components/searchQueryBuilder/tokens/filter/filterKeyCombobox.tsx +++ b/static/app/components/searchQueryBuilder/tokens/filter/filterKeyCombobox.tsx @@ -45,6 +45,18 @@ export function FilterKeyCombobox({token, onCommit, item}: KeyComboboxProps) { 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, getFieldDefinition(getKeyName(token.key)) @@ -56,7 +68,7 @@ export function FilterKeyCombobox({token, onCommit, item}: KeyComboboxProps) { const newFilterValueType = getFilterValueType(token, newFieldDef); if (keyName === getKeyName(token.key)) { - onCommit(); + closeEditor(); return; } @@ -74,15 +86,10 @@ export function FilterKeyCombobox({token, onCommit, item}: KeyComboboxProps) { type: 'UPDATE_FILTER_KEY', token, key: keyName, - // A filter without a value stays pending, so go back to its value - ...(isPending - ? { - focusOverride: {itemKey: item.key.toString(), part: 'value'}, - shouldCommitQuery: false, - } - : {}), + // A filter without a value stays pending + shouldCommitQuery: !isPending, }); - onCommit(); + closeEditor(); return; } @@ -99,6 +106,7 @@ export function FilterKeyCombobox({token, onCommit, item}: KeyComboboxProps) { onCommit(); }, [ + closeEditor, currentFilterValueType, dispatch, getFieldDefinition, @@ -121,7 +129,7 @@ export function FilterKeyCombobox({token, onCommit, item}: KeyComboboxProps) { const trimmedKeyName = keyName.trim(); if (!trimmedKeyName) { - onCommit(); + closeEditor(); return; } @@ -134,7 +142,7 @@ export function FilterKeyCombobox({token, onCommit, item}: KeyComboboxProps) { }) ); }, - [filterKeys, getSuggestedFilterKey, handleSelectKey, onCommit, sortedFilterKeys] + [closeEditor, filterKeys, getSuggestedFilterKey, handleSelectKey, sortedFilterKeys] ); const onCustomValueBlurred = useCallback(() => { @@ -142,8 +150,8 @@ export function FilterKeyCombobox({token, onCommit, item}: KeyComboboxProps) { }, [onCommit]); const onExit = useCallback(() => { - onCommit(); - }, [onCommit]); + closeEditor(); + }, [closeEditor]); return (