diff --git a/static/app/components/arithmeticBuilder/conditionalFilter/conditionalFilter.spec.ts b/static/app/components/arithmeticBuilder/conditionalFilter/conditionalFilter.spec.ts new file mode 100644 index 000000000000..eb500185ed51 --- /dev/null +++ b/static/app/components/arithmeticBuilder/conditionalFilter/conditionalFilter.spec.ts @@ -0,0 +1,308 @@ +import { + formatConditionalFilterClause, + getConditionalFilterEditContext, + replaceConditionalFilterClause, +} from 'sentry/components/arithmeticBuilder/conditionalFilter/conditionalFilter'; + +describe('getConditionalFilterEditContext', () => { + it('treats the full string as one clause when there are no boolean operators', () => { + expect(getConditionalFilterEditContext('organization.slug:sentry', 10)).toMatchObject( + { + phase: 'key', + editText: 'organizati', + replaceStart: 0, + replaceEnd: 24, + } + ); + }); + + it('starts a new key clause after a boolean operator', () => { + const value = 'organization.slug:sentry and '; + expect(getConditionalFilterEditContext(value, value.length)).toEqual({ + phase: 'key', + editText: '', + replaceStart: 29, + replaceEnd: 29, + }); + }); + + it('uses key mode for the active clause when typing a second filter key', () => { + const value = 'organization.slug:sentry and span.op'; + expect(getConditionalFilterEditContext(value, value.length)).toMatchObject({ + phase: 'key', + editText: 'span.op', + replaceStart: 29, + replaceEnd: 36, + }); + }); + + it('does not split on boolean operators inside quoted values', () => { + const value = 'span.description:"foo and bar" and span.op:'; + expect(getConditionalFilterEditContext(value, value.length)).toMatchObject({ + phase: 'value', + filterKey: 'span.op', + valueQuery: '', + replaceStart: 35, + replaceEnd: 43, + }); + }); + + it('limits the clause to the next operator when the cursor is on a boolean operator', () => { + const value = 'span.op:db and span.description:foo and span.status:ok'; + const andIndex = value.indexOf(' and '); + // Cursor on `and` is treated as the start of the following clause (key mode). + expect(getConditionalFilterEditContext(value, andIndex + 1)).toMatchObject({ + phase: 'key', + editText: '', + replaceStart: 15, + replaceEnd: 35, + }); + }); + + it('limits the clause to the next operator when the cursor is at the start of the next clause', () => { + const value = 'span.op:db and span.description:foo and span.status:ok'; + const secondClauseStart = value.indexOf('span.description'); + expect(getConditionalFilterEditContext(value, secondClauseStart)).toMatchObject({ + phase: 'key', + editText: '', + replaceStart: 15, + replaceEnd: 35, + }); + }); + + it('uses key mode before the first colon in a clause', () => { + expect( + getConditionalFilterEditContext('organization.slug:sentry and span.op', 36).phase + ).toBe('key'); + }); + + it('uses value mode after the colon while typing an unquoted value', () => { + expect( + getConditionalFilterEditContext('organization.slug:sentry and span.op:db', 39) + ).toMatchObject({ + phase: 'value', + filterKey: 'span.op', + valueQuery: 'db', + }); + }); + + it('uses key mode after a boolean operator', () => { + const value = 'organization.slug:sentry and '; + expect(getConditionalFilterEditContext(value, value.length).phase).toBe('key'); + }); + + it('uses key mode after a completed unquoted value and trailing space', () => { + const value = 'organization.slug:sentry '; + expect(getConditionalFilterEditContext(value, value.length)).toMatchObject({ + phase: 'key', + editText: '', + }); + }); + + it('stays in value mode for an unclosed quoted value', () => { + const value = 'organization.slug:"hello there'; + expect(getConditionalFilterEditContext(value, value.length)).toMatchObject({ + phase: 'value', + filterKey: 'organization.slug', + valueQuery: 'hello there', + }); + }); + + it('uses key mode after a closed quoted value', () => { + const value = 'organization.slug:"hello there"'; + expect(getConditionalFilterEditContext(value, value.length).phase).toBe('key'); + }); + + it('uses key mode when typing a key after a completed value', () => { + const value = 'organization.slug:sentry span'; + expect(getConditionalFilterEditContext(value, value.length)).toMatchObject({ + phase: 'key', + editText: 'span', + }); + }); + + it('reads the key query from the active clause', () => { + expect( + getConditionalFilterEditContext('organization.slug:sentry and span', 34) + ).toMatchObject({ + phase: 'key', + editText: 'span', + }); + }); + + it('reads the key query after a completed value', () => { + const value = 'organization.slug:sentry spa'; + expect(getConditionalFilterEditContext(value, value.length)).toMatchObject({ + phase: 'key', + editText: 'spa', + }); + }); + + it('returns an empty key query after a completed value and space', () => { + const value = 'organization.slug:sentry '; + expect(getConditionalFilterEditContext(value, value.length)).toMatchObject({ + phase: 'key', + editText: '', + }); + }); + + it('shows an empty key query right after an open parenthesis', () => { + expect(getConditionalFilterEditContext('(', 1)).toMatchObject({ + phase: 'key', + editText: '', + replaceStart: 1, + replaceEnd: 1, + }); + }); + + it('keeps key mode when typing after an open parenthesis', () => { + expect(getConditionalFilterEditContext('(span', 5)).toMatchObject({ + phase: 'key', + editText: 'span', + replaceStart: 1, + replaceEnd: 5, + }); + }); + + it('keeps value mode for a key typed after an open parenthesis', () => { + expect(getConditionalFilterEditContext('(span.op:', 9)).toMatchObject({ + phase: 'value', + filterKey: 'span.op', + editText: '', + replaceStart: 1, + replaceEnd: 9, + }); + }); + + it('ignores a trailing close parenthesis when reading the filter key', () => { + const value = '(span.op:db)'; + expect(getConditionalFilterEditContext(value, value.length - 1)).toMatchObject({ + phase: 'value', + filterKey: 'span.op', + editText: 'db', + valueQuery: 'db', + }); + }); + + it('keeps next-key replace range inside grouping parentheses', () => { + const value = '(span.op:db span)'; + expect(getConditionalFilterEditContext(value, value.length - 1)).toMatchObject({ + phase: 'key', + editText: 'span', + replaceStart: 12, + replaceEnd: value.length - 1, + }); + }); + + it('stays in value mode for a bracketed list with spaces', () => { + const value = 'span.op:[db, http]'; + expect(getConditionalFilterEditContext(value, value.length)).toMatchObject({ + phase: 'value', + filterKey: 'span.op', + editText: '[db, http]', + valueQuery: '[db, http]', + }); + }); + + it('stays in value mode while typing an unclosed bracketed list', () => { + const value = 'span.op:[db, htt'; + expect(getConditionalFilterEditContext(value, value.length)).toMatchObject({ + phase: 'value', + filterKey: 'span.op', + editText: '[db, htt', + valueQuery: '[db, htt', + }); + }); + + it('uses key mode after a completed bracketed list and trailing space', () => { + const value = 'span.op:[db, http] '; + expect(getConditionalFilterEditContext(value, value.length)).toMatchObject({ + phase: 'key', + editText: '', + }); + }); + + it('stays in value mode for quoted entries inside a bracketed list', () => { + const value = 'agent_name:["Agent Run","Assisted Query"]'; + expect(getConditionalFilterEditContext(value, value.length)).toMatchObject({ + phase: 'value', + filterKey: 'agent_name', + editText: '["Agent Run","Assisted Query"]', + valueQuery: '["Agent Run","Assisted Query"]', + }); + }); +}); + +describe('replaceConditionalFilterClause', () => { + it('replaces only the active clause when selecting a key suggestion', () => { + const value = 'organization.slug:sentry and '; + expect(replaceConditionalFilterClause(value, value.length, 'span.op:')).toEqual({ + newCursorIndex: 37, + newValue: 'organization.slug:sentry and span.op:', + }); + }); + + it('appends a key after a completed value instead of replacing it', () => { + const value = 'organization.slug:sentry '; + expect(replaceConditionalFilterClause(value, value.length, 'span.op:')).toEqual({ + newCursorIndex: 33, + newValue: 'organization.slug:sentry span.op:', + }); + }); + + it('preserves a leading parenthesis when selecting a key suggestion', () => { + expect(replaceConditionalFilterClause('(', 1, 'span.op:')).toEqual({ + newCursorIndex: 9, + newValue: '(span.op:', + }); + }); + + it('preserves grouping parentheses when selecting a value suggestion', () => { + expect(replaceConditionalFilterClause('(span.op:)', 9, 'span.op:db')).toEqual({ + newCursorIndex: 11, + newValue: '(span.op:db)', + }); + }); + + it('preserves a trailing parenthesis when selecting a next-key suggestion', () => { + const value = '(span.op:db span)'; + expect( + replaceConditionalFilterClause(value, value.length - 1, 'span.status:') + ).toEqual({ + newCursorIndex: 24, + newValue: '(span.op:db span.status:)', + }); + }); + + it('does not rewrite a bracketed list when selecting a key suggestion mid-list', () => { + const value = 'span.op:[db, http]'; + // Cursor on `http` inside the list must still replace the whole clause as a value + // edit path would; selecting a key suggestion should not split on the space. + expect( + replaceConditionalFilterClause(value, value.length - 2, 'span.status:') + ).toEqual({ + newCursorIndex: 12, + newValue: 'span.status:', + }); + }); +}); + +describe('formatConditionalFilterClause', () => { + it('quotes values that contain spaces', () => { + expect(formatConditionalFilterClause('organization.slug', 'hello there')).toBe( + 'organization.slug:"hello there"' + ); + }); + + it('leaves simple values unquoted', () => { + expect(formatConditionalFilterClause('organization.slug', 'sentry')).toBe( + 'organization.slug:sentry' + ); + }); + + it('preserves bracketed list syntax', () => { + expect(formatConditionalFilterClause('span.op', '[db, http]')).toBe( + 'span.op:[db, http]' + ); + }); +}); diff --git a/static/app/components/arithmeticBuilder/conditionalFilter/conditionalFilter.ts b/static/app/components/arithmeticBuilder/conditionalFilter/conditionalFilter.ts new file mode 100644 index 000000000000..e993c69bb641 --- /dev/null +++ b/static/app/components/arithmeticBuilder/conditionalFilter/conditionalFilter.ts @@ -0,0 +1,431 @@ +import {escapeDoubleQuotes} from 'sentry/utils'; + +/** + * EAP `_if` filter args are backtick-wrapped (`avg_if(\`span.op:db\`,…)`). Detect them + * by parameter name so the editor can show the raw query and re-wrap on commit. + */ +export function isSearchFilterParameter( + parameter: {kind?: string; name?: string} | null | undefined +): boolean { + return parameter?.kind === 'value' && parameter.name === 'filter'; +} + +/** + * Remove backticks so the filter can be safely wrapped in them. + */ +export function escapeConditionalFilter(filter: string): string { + return filter.replace(/`/g, '').trim(); +} + +/** + * Wrap a search filter for use as the first argument of an EAP `_if` aggregate. + * Empty input becomes empty backticks so the arithmetic tokenizer keeps a filter slot. + */ +export function ensureSearchFilterArgument(value: string): string { + const escaped = escapeConditionalFilter(value); + return `\`${escaped}\``; +} + +/** + * Strip outer backticks from an `_if` filter argument for display / editing. + */ +export function unwrapSearchFilterArgument(value: string): string { + const trimmed = value.trim(); + if (trimmed.length >= 2 && trimmed.startsWith('`') && trimmed.endsWith('`')) { + return trimmed.slice(1, -1); + } + return trimmed; +} + +const NEEDS_QUOTING_RE = /[\s(),\\"]/; +/** Search syntax for multi-value filters, e.g. `key:[value1, value2]`. */ +const BRACKETED_LIST_VALUE_RE = /^\[[^\]]*\]$/; + +/** + * Quote a tag value when it contains spaces or other special search characters. + */ +function formatConditionalFilterTagValue(value: string): string { + if (value === '') { + return '""'; + } + if ( + (value.startsWith('"') && value.endsWith('"') && value.length >= 2) || + BRACKETED_LIST_VALUE_RE.test(value) + ) { + return value; + } + if (NEEDS_QUOTING_RE.test(value)) { + return `"${escapeDoubleQuotes(value)}"`; + } + return value; +} + +export function formatConditionalFilterClause( + filterKey: string, + tagValue: string +): string { + return `${filterKey}:${formatConditionalFilterTagValue(tagValue)}`; +} + +type BooleanOperatorMatch = { + end: number; + start: number; +}; + +function isEscaped(value: string, index: number): boolean { + let backslashes = 0; + for (let i = index - 1; i >= 0 && value[i] === '\\'; i--) { + backslashes++; + } + return backslashes % 2 === 1; +} + +function hasUnclosedQuote(value: string): boolean { + let inQuotes = false; + for (let i = 0; i < value.length; i++) { + if (value[i] === '\\' && i + 1 < value.length) { + i++; + continue; + } + if (value[i] === '"' && !isEscaped(value, i)) { + inQuotes = !inQuotes; + } + } + return inQuotes; +} + +/** + * Length of a complete bracketed list starting at index 0, or null if unclosed / + * not a bracketed list. Quotes inside the list are respected so commas and `]` + * in `"Agent Run"` do not end the list early. + */ +function getClosedBracketListLength(value: string): number | null { + if (!value.startsWith('[')) { + return null; + } + + let inQuotes = false; + for (let i = 1; i < value.length; i++) { + const char = value[i]!; + if (char === '\\' && i + 1 < value.length) { + i++; + continue; + } + if (char === '"' && !isEscaped(value, i)) { + inQuotes = !inQuotes; + continue; + } + if (!inQuotes && char === ']') { + return i + 1; + } + } + return null; +} + +function hasUnclosedBracketList(value: string): boolean { + return value.startsWith('[') && getClosedBracketListLength(value) === null; +} + +function stripQuotesForValueSearch(value: string): string { + if (value.startsWith('"')) { + const withoutOpen = value.slice(1); + if (!hasUnclosedQuote(value) && withoutOpen.endsWith('"')) { + return withoutOpen.slice(0, -1); + } + return withoutOpen; + } + return value; +} + +function findBooleanOperators(value: string): BooleanOperatorMatch[] { + const matches: BooleanOperatorMatch[] = []; + let inQuotes = false; + let index = 0; + + while (index < value.length) { + const char = value[index]!; + + if (char === '\\' && index + 1 < value.length) { + index += 2; + continue; + } + + if (char === '"' && !isEscaped(value, index)) { + inQuotes = !inQuotes; + index++; + continue; + } + + if (!inQuotes) { + const rest = value.slice(index); + const match = rest.match(/^(\s+)(and|or)(\s+)/i); + if (match) { + matches.push({ + start: index, + end: index + match[0].length, + }); + index += match[0].length; + continue; + } + } + + index++; + } + + return matches; +} + +function getConditionalFilterClauseBounds( + value: string, + cursorIndex: number, + booleanOperators: BooleanOperatorMatch[] = findBooleanOperators(value) +): {clauseEnd: number; clauseStart: number} { + let cursor = Math.max(0, Math.min(cursorIndex, value.length)); + + // Cursor on a boolean operator counts as the start of the following clause. + for (const operator of booleanOperators) { + if (cursor >= operator.start && cursor <= operator.end) { + cursor = operator.end; + break; + } + } + + let clauseStart = 0; + let clauseEnd = value.length; + + for (const operator of booleanOperators) { + if (cursor >= operator.end) { + clauseStart = operator.end; + continue; + } + + if (cursor < operator.start) { + clauseEnd = operator.start; + break; + } + } + + return {clauseStart, clauseEnd}; +} + +function getConditionalFilterClauseAtCursor( + value: string, + cursorIndex: number +): { + clause: string; + clauseCursorIndex: number; + clauseEnd: number; + clauseStart: number; +} { + const {clauseStart, clauseEnd} = getConditionalFilterClauseBounds(value, cursorIndex); + const clause = value.slice(clauseStart, clauseEnd); + return { + clause, + clauseStart, + clauseEnd, + clauseCursorIndex: Math.max(0, Math.min(cursorIndex, value.length) - clauseStart), + }; +} + +export type ConditionalFilterEditContext = { + /** Text used to filter key suggestions or ComboBox input matching. */ + editText: string; + phase: 'key' | 'value'; + replaceEnd: number; + replaceStart: number; + filterKey?: string; + /** Tag-value API search string (quotes stripped). */ + valueQuery?: string; +}; + +/** + * Grouping `(` / `)` around a clause (e.g. `(span.op:db)`) must not be treated as part + * of the filter key or value, or autocomplete filters itself to nothing after `(`. + */ +function getClauseInnerBounds(clause: string): {innerEnd: number; innerStart: number} { + let innerStart = 0; + let innerEnd = clause.length; + + while (innerStart < innerEnd && clause[innerStart] === '(') { + innerStart++; + while (innerStart < innerEnd && clause[innerStart] === ' ') { + innerStart++; + } + } + + const innerSoFar = clause.slice(innerStart, innerEnd); + if (!hasUnclosedQuote(innerSoFar)) { + while (innerEnd > innerStart && clause[innerEnd - 1] === ')') { + innerEnd--; + while (innerEnd > innerStart && clause[innerEnd - 1] === ' ') { + innerEnd--; + } + } + } + + return {innerStart, innerEnd}; +} + +/** + * Decide whether the cursor is editing a filter key or value, and which substring + * a suggestion should replace. + * + * Value mode continues while the value is still open: + * - empty value after `:` + * - unquoted value with no trailing whitespace yet + * - quoted value with an unclosed `"` + * - bracketed multi-value list (`[a, b]`) until the matching `]` + * + * Once a value is complete (`key:value `, `key:"quoted"`, or `key:[a, b] `), subsequent + * text is a new key and key autocomplete is shown. + * + * Leading/trailing grouping parentheses are preserved outside the replace range so + * suggestions still work inside `(...)`. + */ +export function getConditionalFilterEditContext( + value: string, + cursorIndex: number +): ConditionalFilterEditContext { + const {clause, clauseStart, clauseCursorIndex} = getConditionalFilterClauseAtCursor( + value, + cursorIndex + ); + + const {innerStart, innerEnd} = getClauseInnerBounds(clause); + const inner = clause.slice(innerStart, innerEnd); + const innerCursorIndex = Math.max( + 0, + Math.min(clauseCursorIndex, clause.length) - innerStart + ); + const clampedInnerCursor = Math.max(0, Math.min(innerCursorIndex, inner.length)); + const absoluteInnerStart = clauseStart + innerStart; + // Keep trailing grouping `)` outside replacements; leading `(` stay before replaceStart. + const absoluteInnerEnd = clauseStart + innerEnd; + + const colonIndex = inner.indexOf(':'); + if (colonIndex === -1 || clampedInnerCursor <= colonIndex) { + return { + phase: 'key', + editText: inner.slice(0, clampedInnerCursor).trim(), + replaceStart: absoluteInnerStart, + replaceEnd: absoluteInnerEnd, + }; + } + + const filterKey = inner.slice(0, colonIndex).trim(); + const valuePart = inner.slice(colonIndex + 1); + const cursorInValue = Math.max(0, clampedInnerCursor - (colonIndex + 1)); + const beforeCursor = valuePart.slice(0, cursorInValue); + + // Unclosed quotes → keep editing the value (including spaces inside the quote). + if (hasUnclosedQuote(beforeCursor)) { + return { + phase: 'value', + editText: beforeCursor, + filterKey, + valueQuery: stripQuotesForValueSearch(beforeCursor), + replaceStart: absoluteInnerStart, + replaceEnd: absoluteInnerEnd, + }; + } + + // Closed quoted value: `"hello there"` or `"hello there" nextKey` + if (beforeCursor.startsWith('"')) { + const closedQuoteMatch = beforeCursor.match(/^"(?:[^"\\]|\\.)*"(\s*)(.*)$/); + if (closedQuoteMatch) { + const [, spaces = '', nextKey = ''] = closedQuoteMatch; + const quotedValue = beforeCursor.slice( + 0, + beforeCursor.length - spaces.length - nextKey.length + ); + const keyStartInInner = colonIndex + 1 + quotedValue.length + spaces.length; + return { + phase: 'key', + editText: nextKey, + replaceStart: absoluteInnerStart + keyStartInInner, + replaceEnd: absoluteInnerEnd, + }; + } + } + + // Bracketed multi-value lists (`key:[a, b]`) keep spaces inside the brackets. + // Unclosed `[` stays in value mode; a closed list only starts a new key after + // trailing whitespace (same as quoted values). + if (beforeCursor.startsWith('[')) { + if (hasUnclosedBracketList(beforeCursor)) { + return { + phase: 'value', + editText: beforeCursor, + filterKey, + valueQuery: beforeCursor, + replaceStart: absoluteInnerStart, + replaceEnd: absoluteInnerEnd, + }; + } + + const listLength = getClosedBracketListLength(beforeCursor); + if (listLength !== null) { + const afterList = beforeCursor.slice(listLength); + const trailingMatch = afterList.match(/^(\s*)(.*)$/); + if (trailingMatch) { + const [, spaces = '', nextKey = ''] = trailingMatch; + if (spaces.length > 0 || nextKey.length > 0) { + const keyStartInInner = colonIndex + 1 + listLength + spaces.length; + return { + phase: 'key', + editText: nextKey, + replaceStart: absoluteInnerStart + keyStartInInner, + replaceEnd: absoluteInnerEnd, + }; + } + } + + return { + phase: 'value', + editText: beforeCursor.slice(0, listLength), + filterKey, + valueQuery: beforeCursor.slice(0, listLength), + replaceStart: absoluteInnerStart, + replaceEnd: absoluteInnerEnd, + }; + } + } + + // Unquoted value: complete once whitespace follows a non-empty token. + const unquotedMatch = beforeCursor.match(/^(\S+)(\s+)(.*)$/); + if (unquotedMatch) { + const [, completedValue, spaces, nextKey = ''] = unquotedMatch; + const keyStartInInner = colonIndex + 1 + completedValue!.length + spaces!.length; + return { + phase: 'key', + editText: nextKey, + replaceStart: absoluteInnerStart + keyStartInInner, + replaceEnd: absoluteInnerEnd, + }; + } + + // Still typing an unquoted value (or empty value after `:`). + return { + phase: 'value', + editText: beforeCursor, + filterKey, + valueQuery: beforeCursor, + replaceStart: absoluteInnerStart, + replaceEnd: absoluteInnerEnd, + }; +} + +export function replaceConditionalFilterClause( + value: string, + cursorIndex: number, + newClause: string +): {newCursorIndex: number; newValue: string} { + const {replaceStart, replaceEnd} = getConditionalFilterEditContext(value, cursorIndex); + const newValue = value.slice(0, replaceStart) + newClause + value.slice(replaceEnd); + const newCursorIndex = replaceStart + newClause.length; + return {newValue, newCursorIndex}; +} + +export function isFilterKeySuggestion(value: string): boolean { + return value.endsWith(':') && value.indexOf(':') === value.length - 1; +} diff --git a/static/app/components/arithmeticBuilder/conditionalFilter/conditionalFilterAutocomplete.spec.tsx b/static/app/components/arithmeticBuilder/conditionalFilter/conditionalFilterAutocomplete.spec.tsx new file mode 100644 index 000000000000..efdeb6ecef1f --- /dev/null +++ b/static/app/components/arithmeticBuilder/conditionalFilter/conditionalFilterAutocomplete.spec.tsx @@ -0,0 +1,150 @@ +import {act, renderHookWithProviders, waitFor} from 'sentry-test/reactTestingLibrary'; + +import {useConditionalFilterAutocomplete} from 'sentry/components/arithmeticBuilder/conditionalFilter/conditionalFilterAutocomplete'; +import {DEFAULT_DEBOUNCE_DURATION} from 'sentry/constants'; +import {FieldKind} from 'sentry/utils/fields'; + +const functionArguments = [{name: 'span.op', kind: FieldKind.TAG, label: 'span.op'}]; + +describe('useConditionalFilterAutocomplete', () => { + it('does not fetch values with an empty filter key', async () => { + const getFilterTagValues = jest.fn().mockResolvedValue([{value: 'db'}]); + + const {rerender} = renderHookWithProviders( + ({filterValue, selectionIndex}: {filterValue: string; selectionIndex: number}) => + useConditionalFilterAutocomplete({ + enabled: true, + filterValue, + functionArguments, + getFilterTagValues, + selectionIndex, + }), + {initialProps: {filterValue: '', selectionIndex: 0}} + ); + + expect(getFilterTagValues).not.toHaveBeenCalled(); + + rerender({filterValue: 'span.op', selectionIndex: 6}); + expect(getFilterTagValues).not.toHaveBeenCalled(); + + rerender({filterValue: 'span.op:', selectionIndex: 8}); + + await waitFor(() => { + expect(getFilterTagValues).toHaveBeenCalledWith({ + tag: expect.objectContaining({key: 'span.op'}), + searchQuery: '', + }); + }); + expect(getFilterTagValues).not.toHaveBeenCalledWith( + expect.objectContaining({ + tag: expect.objectContaining({key: ''}), + }) + ); + }); + + it('shows key suggestions when the cursor is before the colon', () => { + const getFilterTagValues = jest.fn().mockResolvedValue([{value: 'db'}]); + + const {result} = renderHookWithProviders(() => + useConditionalFilterAutocomplete({ + enabled: true, + filterValue: 'span.op:db', + functionArguments, + getFilterTagValues, + selectionIndex: 3, + }) + ); + + expect(result.current.items.map(item => item.label)).toEqual(['span.op:']); + expect(getFilterTagValues).not.toHaveBeenCalled(); + }); + + it('shows key suggestions after an open parenthesis', () => { + const getFilterTagValues = jest.fn().mockResolvedValue([{value: 'db'}]); + + const {result} = renderHookWithProviders(() => + useConditionalFilterAutocomplete({ + enabled: true, + filterValue: '(', + functionArguments, + getFilterTagValues, + selectionIndex: 1, + }) + ); + + expect(result.current.items.map(item => item.label)).toEqual(['span.op:']); + expect(getFilterTagValues).not.toHaveBeenCalled(); + }); + + it('shows value suggestions when the cursor is after the colon', async () => { + const getFilterTagValues = jest.fn().mockResolvedValue([{value: 'db'}]); + + const {result} = renderHookWithProviders(() => + useConditionalFilterAutocomplete({ + enabled: true, + filterValue: 'span.op:db', + functionArguments, + getFilterTagValues, + selectionIndex: 10, + }) + ); + + await waitFor(() => { + expect(getFilterTagValues).toHaveBeenCalled(); + }); + + await waitFor(() => { + expect(result.current.items.map(item => item.label)).toEqual(['db']); + }); + }); + + it('does not show previous key values while debounce catches up to a new key', async () => { + jest.useFakeTimers(); + const getFilterTagValues = jest.fn(({tag}) => { + if (tag.key === 'span.op') { + return Promise.resolve([{value: 'db'}]); + } + if (tag.key === 'span.description') { + return Promise.resolve([{value: 'SELECT 1'}]); + } + return Promise.resolve([]); + }); + + try { + const {result, rerender} = renderHookWithProviders( + ({filterValue, selectionIndex}: {filterValue: string; selectionIndex: number}) => + useConditionalFilterAutocomplete({ + enabled: true, + filterValue, + functionArguments: [ + {name: 'span.op', kind: FieldKind.TAG, label: 'span.op'}, + { + name: 'span.description', + kind: FieldKind.TAG, + label: 'span.description', + }, + ], + getFilterTagValues, + selectionIndex, + }), + {initialProps: {filterValue: 'span.op:', selectionIndex: 8}} + ); + + await act(() => jest.advanceTimersByTimeAsync(DEFAULT_DEBOUNCE_DURATION)); + await waitFor(() => { + expect(result.current.items.map(item => item.label)).toEqual(['db']); + }); + + rerender({filterValue: 'span.description:', selectionIndex: 17}); + // Immediate render still has prior query data + new filterKey — must not relabel. + expect(result.current.items.map(item => item.label)).toEqual([]); + + await act(() => jest.advanceTimersByTimeAsync(DEFAULT_DEBOUNCE_DURATION)); + await waitFor(() => { + expect(result.current.items.map(item => item.label)).toEqual(['SELECT 1']); + }); + } finally { + jest.useRealTimers(); + } + }); +}); diff --git a/static/app/components/arithmeticBuilder/conditionalFilter/conditionalFilterAutocomplete.tsx b/static/app/components/arithmeticBuilder/conditionalFilter/conditionalFilterAutocomplete.tsx new file mode 100644 index 000000000000..c4e0a15f48d7 --- /dev/null +++ b/static/app/components/arithmeticBuilder/conditionalFilter/conditionalFilterAutocomplete.tsx @@ -0,0 +1,184 @@ +import {useMemo} from 'react'; +import {useDebouncedValue} from '@tanstack/react-pacer'; +import {useQuery} from '@tanstack/react-query'; + +import type {SelectOptionWithKey} from '@sentry/scraps/compactSelect'; + +import { + formatConditionalFilterClause, + getConditionalFilterEditContext, +} from 'sentry/components/arithmeticBuilder/conditionalFilter/conditionalFilter'; +import type {FunctionArgument} from 'sentry/components/arithmeticBuilder/types'; +import type {GetTagValues} from 'sentry/components/searchQueryBuilder'; +import {DEFAULT_DEBOUNCE_DURATION} from 'sentry/constants'; +import {FieldKind} from 'sentry/utils/fields'; + +function useFilterKeyItems( + attributes: FunctionArgument[] +): Array> { + return useMemo(() => { + return attributes.map(item => { + const key = item.name; + const filterKey = `${key}:`; + return { + key, + label: filterKey, + value: filterKey, + textValue: key, + hideCheck: true, + }; + }); + }, [attributes]); +} + +function useFilterValueItems({ + enabled, + filterKey, + valueQuery, + getFilterTagValues, + tagKind, +}: { + enabled: boolean; + filterKey: string; + valueQuery: string; + getFilterTagValues?: GetTagValues; + tagKind?: FieldKind; +}): Array> { + const tag = useMemo( + () => ({ + key: filterKey, + name: filterKey, + kind: tagKind, + }), + [filterKey, tagKind] + ); + + const queryKey = useMemo( + () => ['arithmetic-filter-tag-values', tag, valueQuery] as const, + [tag, valueQuery] + ); + const [debouncedQueryKey] = useDebouncedValue(queryKey, { + wait: DEFAULT_DEBOUNCE_DURATION, + }); + const debouncedFilterKey = debouncedQueryKey[1].key; + + const {data} = useQuery({ + queryKey: debouncedQueryKey, + queryFn: ctx => + getFilterTagValues!({ + tag: ctx.queryKey[1], + searchQuery: ctx.queryKey[2] ?? '', + }), + // Gate on the *debounced* key. `enabled` flips true as soon as the user types `:`, + // but the query key still holds the previous empty key for one debounce window — + // fetching then hits `/attributes//values/` and 404-retries. + // + // Do not use keepPreviousData: after switching keys in a compound filter, placeholder + // results from the prior key would be rewritten with the new key's clause. + enabled: enabled && Boolean(getFilterTagValues && debouncedFilterKey), + staleTime: 30_000, + retry: false, + }); + + return useMemo(() => { + // Hide results until debounce catches up to the key the cursor is editing. + if (!data?.length || debouncedFilterKey !== filterKey) { + return []; + } + + return data.map(item => { + const tagValue = typeof item === 'string' ? item : item.value; + return { + key: `filter-value:${filterKey}:${tagValue}`, + label: tagValue, + value: formatConditionalFilterClause(filterKey, tagValue), + textValue: tagValue, + hideCheck: true, + }; + }); + }, [data, debouncedFilterKey, filterKey]); +} + +export function useConditionalFilterAutocomplete({ + enabled, + filterValue, + functionArguments, + getFilterTagValues, + selectionIndex, +}: { + enabled: boolean; + filterValue: string; + functionArguments: FunctionArgument[]; + selectionIndex: number; + getFilterTagValues?: GetTagValues; +}) { + const editContext = useMemo( + () => (enabled ? getConditionalFilterEditContext(filterValue, selectionIndex) : null), + [enabled, filterValue, selectionIndex] + ); + + const editPhase = editContext?.phase ?? 'key'; + const parsedFilterInput = useMemo(() => { + if (editContext?.phase !== 'value' || !editContext.filterKey) { + return null; + } + return { + filterKey: editContext.filterKey, + valueQuery: editContext.valueQuery ?? '', + }; + }, [editContext]); + + const filterKeyItems = useFilterKeyItems(functionArguments); + const filterValueItems = useFilterValueItems({ + enabled: enabled && Boolean(parsedFilterInput && getFilterTagValues), + filterKey: parsedFilterInput?.filterKey ?? '', + valueQuery: parsedFilterInput?.valueQuery ?? '', + getFilterTagValues, + tagKind: functionArguments.find( + argument => argument.name === parsedFilterInput?.filterKey + )?.kind, + }); + + const comboBoxFilterValue = useMemo(() => { + if (!editContext) { + return ''; + } + if (editContext.phase === 'value' && parsedFilterInput && getFilterTagValues) { + return parsedFilterInput.valueQuery; + } + return editContext.phase === 'key' ? editContext.editText : ''; + }, [editContext, getFilterTagValues, parsedFilterInput]); + + const items = useMemo(() => { + if (!enabled) { + return []; + } + if (editPhase === 'value' && parsedFilterInput && getFilterTagValues) { + return filterValueItems; + } + const keyQuery = (editContext?.editText ?? '').trim().toLowerCase(); + if (!keyQuery) { + return filterKeyItems; + } + return filterKeyItems.filter( + item => + item.value.toLowerCase().includes(keyQuery) || + (item.textValue?.toLowerCase().includes(keyQuery) ?? false) + ); + }, [ + editContext?.editText, + editPhase, + enabled, + filterKeyItems, + filterValueItems, + getFilterTagValues, + parsedFilterInput, + ]); + + return { + comboBoxFilterValue, + editPhase, + items, + parsedFilterInput, + }; +} diff --git a/static/app/components/arithmeticBuilder/conditionalFilter/conditionalFilterInput.tsx b/static/app/components/arithmeticBuilder/conditionalFilter/conditionalFilterInput.tsx new file mode 100644 index 000000000000..1a92eed2f9fb --- /dev/null +++ b/static/app/components/arithmeticBuilder/conditionalFilter/conditionalFilterInput.tsx @@ -0,0 +1,274 @@ +import type {ChangeEvent, FocusEvent, KeyboardEvent, MouseEvent} from 'react'; +import {useCallback, useLayoutEffect, useRef, useState} from 'react'; +import {Item, Section} from '@react-stately/collections'; + +import type {SelectOptionWithKey} from '@sentry/scraps/compactSelect'; + +import { + ensureSearchFilterArgument, + isFilterKeySuggestion, + replaceConditionalFilterClause, + unwrapSearchFilterArgument, +} from 'sentry/components/arithmeticBuilder/conditionalFilter/conditionalFilter'; +import {useConditionalFilterAutocomplete} from 'sentry/components/arithmeticBuilder/conditionalFilter/conditionalFilterAutocomplete'; +import {useArithmeticBuilder} from 'sentry/components/arithmeticBuilder/context'; +import { + ArgumentGridCell, + ArgumentGridRow, + useFunctionArgumentInput, + type FunctionArgumentInputProps, +} from 'sentry/components/arithmeticBuilder/token/useFunctionArgumentInput'; +import {itemIsSection} from 'sentry/components/searchQueryBuilder/tokens/utils'; +import {ComboBox} from 'sentry/components/tokenizedInput/token/comboBox'; +import {t} from 'sentry/locale'; + +export function ConditionalFilterArgumentInput(props: FunctionArgumentInputProps) { + const {argument, argumentIndex, onArgumentsChange} = props; + const { + clearSkipBlurFlush, + commitFunctionToken, + dataTestId, + flushArgumentsIfLeavingGrid, + focusArgument, + gridCellProps, + gridCellRef, + inputRef, + isFocused, + onKeyDown, + onKeyDownCapture, + rowProps, + shouldCloseOnInteractOutside, + } = useFunctionArgumentInput(props); + + const {functionArguments: builderFunctionArguments, getFilterTagValues} = + useArithmeticBuilder(); + + const initialLabel = unwrapSearchFilterArgument(argument.label); + const [inputValue, setInputValue] = useState(''); + const [currentValue, setCurrentValue] = useState(initialLabel); + const [isCurrentlyEditing, setIsCurrentlyEditing] = useState(false); + const [selectionIndex, setSelectionIndex] = useState(0); + const displayValue = isCurrentlyEditing ? inputValue : currentValue; + // Apply after React commits the controlled value. A lone rAF + focus() races in + // Chrome and resets the caret before (or when) focus returns from the listbox. + const pendingCaretRef = useRef<{pos: number; value: string} | null>(null); + const clickSelectionRafRef = useRef(null); + + const {comboBoxFilterValue, editPhase, items} = useConditionalFilterAutocomplete({ + enabled: isCurrentlyEditing, + filterValue: inputValue, + functionArguments: builderFunctionArguments, + getFilterTagValues, + selectionIndex, + }); + + useLayoutEffect(() => { + const pendingCaret = pendingCaretRef.current; + if (pendingCaret === null) { + return; + } + const input = inputRef.current; + if (input?.value !== pendingCaret.value) { + return; + } + pendingCaretRef.current = null; + if (document.activeElement !== input) { + input.focus(); + } + input.setSelectionRange(pendingCaret.pos, pendingCaret.pos); + setSelectionIndex(pendingCaret.pos); + }, [inputRef, inputValue]); + + // Suggestion updates re-render the controlled input and can reset the DOM caret. + // Keep a collapsed caret aligned with selectionIndex so key↔value autocomplete stays + // correct. Never collapse an active text range (select-all + Backspace must stay in + // the argument, not delete the whole function). + useLayoutEffect(() => { + if (!isCurrentlyEditing) { + return; + } + const input = inputRef.current; + if (!input || document.activeElement !== input) { + return; + } + if (input.selectionStart !== input.selectionEnd) { + return; + } + if ( + input.selectionStart === selectionIndex && + input.selectionEnd === selectionIndex + ) { + return; + } + input.setSelectionRange(selectionIndex, selectionIndex); + }, [displayValue, inputRef, isCurrentlyEditing, items, selectionIndex]); + + const shouldFilterComboBoxResults = !(editPhase === 'value' && getFilterTagValues); + + const updateSelectionIndex = useCallback( + (input?: HTMLInputElement | null) => { + const target = input ?? inputRef.current; + setSelectionIndex(target?.selectionStart ?? 0); + }, + [inputRef] + ); + + const resetInputValue = useCallback(() => { + setInputValue(''); + updateSelectionIndex(); + }, [updateSelectionIndex]); + + const onClick = useCallback( + (evt: MouseEvent) => { + const input = evt.currentTarget; + if (clickSelectionRafRef.current !== null) { + window.cancelAnimationFrame(clickSelectionRafRef.current); + } + // Read the caret after the browser places it; cancel if keyUp/change updates first. + clickSelectionRafRef.current = window.requestAnimationFrame(() => { + clickSelectionRafRef.current = null; + updateSelectionIndex(input); + }); + }, + [updateSelectionIndex] + ); + + const onKeyUp = useCallback( + (evt: KeyboardEvent) => { + if (clickSelectionRafRef.current !== null) { + window.cancelAnimationFrame(clickSelectionRafRef.current); + clickSelectionRafRef.current = null; + } + updateSelectionIndex(evt.currentTarget); + }, + [updateSelectionIndex] + ); + + const onInputChange = useCallback((evt: ChangeEvent) => { + if (clickSelectionRafRef.current !== null) { + window.cancelAnimationFrame(clickSelectionRafRef.current); + clickSelectionRafRef.current = null; + } + setInputValue(evt.target.value); + setCurrentValue(evt.target.value); + setSelectionIndex(evt.target.selectionStart ?? 0); + }, []); + + const onInputEscape = useCallback(() => { + resetInputValue(); + setIsCurrentlyEditing(false); + }, [resetInputValue]); + + const onInputFocus = useCallback( + (evt: FocusEvent) => { + evt.stopPropagation(); + clearSkipBlurFlush(); + focusArgument(); + setIsCurrentlyEditing(true); + setInputValue(currentValue); + updateSelectionIndex(evt.currentTarget); + }, + [clearSkipBlurFlush, currentValue, focusArgument, updateSelectionIndex] + ); + + // Persist free-text filter edits on blur. Empty input becomes `` so clearing the filter + // updates argsRef (otherwise the prior value is kept and commitArgumentsIfChanged no-ops). + // Skip REPLACE_TOKEN while focus stays inside the arguments grid — that remounts the + // function and steals focus from the next arg. Pending edits flush via onArgumentsBlur. + const onInputBlur = useCallback( + (evt?: FocusEvent) => { + const value = ensureSearchFilterArgument(inputValue); + setCurrentValue(unwrapSearchFilterArgument(value)); + onArgumentsChange(argumentIndex, value); + resetInputValue(); + setIsCurrentlyEditing(false); + flushArgumentsIfLeavingGrid(evt); + }, + [ + argumentIndex, + flushArgumentsIfLeavingGrid, + inputValue, + onArgumentsChange, + resetInputValue, + ] + ); + + const onInputCommit = useCallback(() => { + // Filter args may intentionally be cleared; don't fall back to the prior label. + const value = ensureSearchFilterArgument(inputValue.trim()); + setCurrentValue(unwrapSearchFilterArgument(value)); + onArgumentsChange(argumentIndex, value); + commitFunctionToken(value); + resetInputValue(); + }, [ + argumentIndex, + commitFunctionToken, + inputValue, + onArgumentsChange, + resetInputValue, + ]); + + const onOptionSelected = useCallback( + (option: SelectOptionWithKey) => { + const {newValue, newCursorIndex} = replaceConditionalFilterClause( + inputValue, + selectionIndex, + option.value + ); + if (isFilterKeySuggestion(option.value)) { + pendingCaretRef.current = {pos: newCursorIndex, value: newValue}; + } + setCurrentValue(newValue); + setInputValue(newValue); + setIsCurrentlyEditing(true); + setSelectionIndex(newCursorIndex); + }, + [inputValue, selectionIndex] + ); + + return ( + + + isFilterKeySuggestion(option.value)} + shouldFilterResults={shouldFilterComboBoxResults} + tabIndex={isFocused ? 0 : -1} + shouldCloseOnInteractOutside={shouldCloseOnInteractOutside} + onClick={onClick} + onInputBlur={onInputBlur} + onInputChange={onInputChange} + onInputCommit={onInputCommit} + onInputEscape={onInputEscape} + onInputFocus={onInputFocus} + onKeyDown={onKeyDown} + onKeyDownCapture={onKeyDownCapture} + onInputKeyUp={onKeyUp} + onOptionSelected={onOptionSelected} + data-test-id={dataTestId} + > + {keyItem => + itemIsSection(keyItem) ? ( +
+ {keyItem.options.map(child => ( + + {child.label} + + ))} +
+ ) : ( + + {keyItem.label} + + ) + } +
+
+
+ ); +} diff --git a/static/app/components/arithmeticBuilder/conditionalFilter/index.ts b/static/app/components/arithmeticBuilder/conditionalFilter/index.ts new file mode 100644 index 000000000000..f1b6bb7b8fb7 --- /dev/null +++ b/static/app/components/arithmeticBuilder/conditionalFilter/index.ts @@ -0,0 +1,7 @@ +export { + ensureSearchFilterArgument, + escapeConditionalFilter, + isSearchFilterParameter, + unwrapSearchFilterArgument, +} from './conditionalFilter'; +export {ConditionalFilterArgumentInput} from './conditionalFilterInput'; diff --git a/static/app/components/arithmeticBuilder/context.tsx b/static/app/components/arithmeticBuilder/context.tsx index e915ca60f753..bf34b38cbff1 100644 --- a/static/app/components/arithmeticBuilder/context.tsx +++ b/static/app/components/arithmeticBuilder/context.tsx @@ -6,6 +6,7 @@ import type { FocusOverride, } from 'sentry/components/arithmeticBuilder/action'; import type {FunctionArgument} from 'sentry/components/arithmeticBuilder/types'; +import type {GetTagValues} from 'sentry/components/searchQueryBuilder'; import type {FieldDefinition} from 'sentry/utils/fields'; interface ArithmeticBuilderContextData { @@ -13,7 +14,19 @@ interface ArithmeticBuilderContextData { dispatch: Dispatch; focusOverride: FocusOverride | null; functionArguments: FunctionArgument[]; - getFieldDefinition: (key: string) => FieldDefinition | null; + getFieldDefinition: ( + key: string, + attributeTexts?: readonly string[] + ) => FieldDefinition | null; + /** + * When true, `_if` combinators use the EAP filter-first editor. Mirrors + * `explore-conditional-aggregates`. + */ + hasConditionalAggregates: boolean; + /** + * Fetches tag values for `_if` combinator filter arguments (e.g. after `span.op:`). + */ + getFilterTagValues?: GetTagValues; getSuggestedKey?: (key: string) => string | null; references?: Set; } @@ -24,6 +37,7 @@ export const ArithmeticBuilderContext = createContext null, + hasConditionalAggregates: false, }); export function useArithmeticBuilder() { diff --git a/static/app/components/arithmeticBuilder/index.spec.tsx b/static/app/components/arithmeticBuilder/index.spec.tsx index deb33d1d4ca4..6d987dba2c4a 100644 --- a/static/app/components/arithmeticBuilder/index.spec.tsx +++ b/static/app/components/arithmeticBuilder/index.spec.tsx @@ -240,26 +240,44 @@ describe('ArithmeticBuilder', () => { // Because we're deleting tokens from the start, we cannot get them // up front as they will change as we delete. We have to get the // element once we reach that position. - const tokens: Array<() => HTMLElement | null> = [ - firstFreeText, - () => screen.queryByRole('gridcell', {name: 'Delete left'}), - firstFreeText, - () => screen.queryByPlaceholderText('span.duration'), - firstFreeText, - () => screen.queryByRole('gridcell', {name: 'Delete +'}), - firstFreeText, - () => screen.queryByPlaceholderText('span.op'), - firstFreeText, - () => screen.queryByRole('gridcell', {name: 'Delete right'}), - firstFreeText, + const tokens: Array<{ + focus: () => HTMLElement | null; + gone: () => HTMLElement | null; + }> = [ + {focus: firstFreeText, gone: firstFreeText}, + { + focus: () => screen.queryByRole('gridcell', {name: 'Delete left'}), + gone: () => screen.queryByRole('gridcell', {name: 'Delete left'}), + }, + {focus: firstFreeText, gone: firstFreeText}, + { + focus: () => screen.queryByPlaceholderText('span.duration'), + gone: () => screen.queryByRole('row', {name: 'sum(span.duration)'}), + }, + {focus: firstFreeText, gone: firstFreeText}, + { + focus: () => screen.queryByRole('gridcell', {name: 'Delete +'}), + gone: () => screen.queryByRole('gridcell', {name: 'Delete +'}), + }, + {focus: firstFreeText, gone: firstFreeText}, + { + focus: () => screen.queryByPlaceholderText('span.op'), + gone: () => screen.queryByRole('row', {name: 'count_if(span.op,equals,db)'}), + }, + {focus: firstFreeText, gone: firstFreeText}, + { + focus: () => screen.queryByRole('gridcell', {name: 'Delete right'}), + gone: () => screen.queryByRole('gridcell', {name: 'Delete right'}), + }, + {focus: firstFreeText, gone: firstFreeText}, ]; let i = 0; - const focus = () => expect(tokens[i]!()).toHaveFocus(); - const focus0 = () => expect(tokens[0]!()).toHaveFocus(); - const deletion = () => expect(tokens[i]!()).not.toBeInTheDocument(); + const focus = () => expect(tokens[i]!.focus()).toHaveFocus(); + const focus0 = () => expect(tokens[0]!.focus()).toHaveFocus(); + const deletion = () => expect(tokens[i]!.gone()).not.toBeInTheDocument(); - await userEvent.click(tokens[i]!()!); + await userEvent.click(tokens[i]!.focus()!); await waitFor(focus); while (i < tokens.length - 1) { @@ -277,6 +295,23 @@ describe('ArithmeticBuilder', () => { expect(screen.getAllByRole('row')).toHaveLength(1); }); + it('deleting a function keeps the following function arguments', async () => { + const expression = 'sum(span.duration) + count_if(span.op,equals,db)'; + render(); + + const sumRow = await screen.findByRole('row', {name: 'sum(span.duration)'}); + await userEvent.click( + within(sumRow).getByRole('button', {name: 'Remove function sum(span.duration)'}) + ); + + expect( + screen.queryByRole('row', {name: 'sum(span.duration)'}) + ).not.toBeInTheDocument(); + expect( + screen.getByRole('row', {name: 'count_if(span.op,equals,db)'}) + ).toBeInTheDocument(); + }); + it('deleting a middle literal does not shift remaining literal values', async () => { const expression = 'A + 1 + 2 + 3'; render( diff --git a/static/app/components/arithmeticBuilder/index.tsx b/static/app/components/arithmeticBuilder/index.tsx index 1a0f70d7f8ac..e62dceeddfad 100644 --- a/static/app/components/arithmeticBuilder/index.tsx +++ b/static/app/components/arithmeticBuilder/index.tsx @@ -9,6 +9,7 @@ import {ArithmeticBuilderContext} from 'sentry/components/arithmeticBuilder/cont import type {Expression} from 'sentry/components/arithmeticBuilder/expression'; import {TokenGrid} from 'sentry/components/arithmeticBuilder/token/grid'; import type {FunctionArgument} from 'sentry/components/arithmeticBuilder/types'; +import type {GetTagValues} from 'sentry/components/searchQueryBuilder'; import type {FieldDefinition} from 'sentry/utils/fields'; import {FieldKind} from 'sentry/utils/fields'; import {PanelProvider} from 'sentry/utils/panelProvider'; @@ -17,10 +18,18 @@ interface ArithmeticBuilderProps { aggregations: string[]; expression: string; functionArguments: FunctionArgument[]; - getFieldDefinition: (key: string) => FieldDefinition | null; + getFieldDefinition: ( + key: string, + attributeTexts?: readonly string[] + ) => FieldDefinition | null; className?: string; 'data-test-id'?: string; disabled?: boolean; + /** + * Fetches tag values for `_if` combinator filter arguments in equations. + * Only used when `hasConditionalAggregates` is on. + */ + getFilterTagValues?: GetTagValues; /** * This is used when a user types in a search key and submits the token. * The submission happens when the user types a colon or presses enter. @@ -28,6 +37,11 @@ interface ArithmeticBuilderProps { * to a known column. */ getSuggestedKey?: (key: string) => string | null; + /** + * Enables the EAP filter-first `_if` argument editor. Should follow + * `explore-conditional-aggregates`. + */ + hasConditionalAggregates?: boolean; /** * When provided, the arithmetic builder will use the references to suggest * keys for the user instead of aggregations and function arguments. @@ -45,7 +59,9 @@ export function ArithmeticBuilder({ aggregations, functionArguments, getFieldDefinition, + getFilterTagValues, getSuggestedKey, + hasConditionalAggregates = false, className, disabled, references, @@ -73,7 +89,9 @@ export function ArithmeticBuilder({ }), functionArguments, getFieldDefinition, + getFilterTagValues: hasConditionalAggregates ? getFilterTagValues : undefined, getSuggestedKey, + hasConditionalAggregates, references, }; }, [ @@ -82,7 +100,9 @@ export function ArithmeticBuilder({ aggregations, functionArguments, getFieldDefinition, + getFilterTagValues, getSuggestedKey, + hasConditionalAggregates, references, ]); @@ -104,10 +124,12 @@ export function ArithmeticBuilder({ } const Wrapper = styled(Input.withComponent('div'))<{state: 'valid' | 'invalid'}>` - min-height: 38px; + min-height: ${p => p.theme.form.md.minHeight}; padding: 0; height: auto; width: 100%; + min-width: 0; + max-width: 100%; position: relative; font-size: ${p => p.theme.font.size.md}; cursor: text; diff --git a/static/app/components/arithmeticBuilder/token/freeText.tsx b/static/app/components/arithmeticBuilder/token/freeText.tsx index f6cb56498afa..410774bf66e1 100644 --- a/static/app/components/arithmeticBuilder/token/freeText.tsx +++ b/static/app/components/arithmeticBuilder/token/freeText.tsx @@ -62,6 +62,8 @@ export function ArithmeticTokenFreeText({ focusable: true, }); + const isCollapsed = !token.text.trim(); + return ( - + ) { evt.stopPropagation(); } -const GridCell = styled('div')` +const GridCell = styled('div', { + shouldForwardProp: prop => prop !== 'collapsed' && prop !== 'showPlaceholder', +})<{collapsed?: boolean; showPlaceholder?: boolean}>` position: relative; display: flex; align-items: stretch; @@ -646,8 +656,14 @@ const GridCell = styled('div')` width: 100%; input { - padding: 0 ${p => p.theme.space.xs}; - min-width: 9px; + /* Collapsed empty spacers stay zero-width/padding so they do not wrap. + * Mid-expression hit targets come from the Row collapsed mid-gap width in + * styles.tsx. Trailing empty fields with a placeholder keep horizontal + * inset so it lines up with the aggregate filter. */ + padding: 0 ${p => (p.collapsed && !p.showPlaceholder ? 0 : p.theme.space.xs)}; + min-width: ${p => (p.collapsed && !p.showPlaceholder ? 0 : '9px')}; width: 100%; + height: 100%; + min-height: 100%; } `; diff --git a/static/app/components/arithmeticBuilder/token/function.tsx b/static/app/components/arithmeticBuilder/token/function.tsx index 7da5083a3ff2..087878168fd9 100644 --- a/static/app/components/arithmeticBuilder/token/function.tsx +++ b/static/app/components/arithmeticBuilder/token/function.tsx @@ -1,35 +1,60 @@ import type {ChangeEvent, FocusEvent, RefObject} from 'react'; -import {useCallback, useMemo, useRef, useState} from 'react'; +import {useCallback, useEffect, useMemo, useRef, useState} from 'react'; import {css} from '@emotion/react'; import styled from '@emotion/styled'; import {type AriaGridListOptions} from '@react-aria/gridlist'; import {Item, Section} from '@react-stately/collections'; import {useListState, type ListState} from '@react-stately/list'; -import type {CollectionChildren, KeyboardEvent, Node} from '@react-types/shared'; +import type {CollectionChildren, Node} from '@react-types/shared'; import type {SelectOptionWithKey} from '@sentry/scraps/compactSelect'; import {Flex} from '@sentry/scraps/layout'; +import { + ConditionalFilterArgumentInput, + isSearchFilterParameter, + unwrapSearchFilterArgument, +} from 'sentry/components/arithmeticBuilder/conditionalFilter'; import {useArithmeticBuilder} from 'sentry/components/arithmeticBuilder/context'; import type { Token, TokenAttribute, TokenFunction, } from 'sentry/components/arithmeticBuilder/token'; -import {TokenKind} from 'sentry/components/arithmeticBuilder/token'; import {DeleteButton} from 'sentry/components/arithmeticBuilder/token/deleteButton'; -import {nextTokenKeyOfKind} from 'sentry/components/arithmeticBuilder/tokenizer'; +import { + ArgumentGridCell, + ArgumentGridRow, + useFunctionArgumentInput, + type FunctionArgumentInputProps, +} from 'sentry/components/arithmeticBuilder/token/useFunctionArgumentInput'; import type {FunctionArgument} from 'sentry/components/arithmeticBuilder/types'; import {itemIsSection} from 'sentry/components/searchQueryBuilder/tokens/utils'; import {useGridList} from 'sentry/components/tokenizedInput/grid/useGridList'; import {useGridListItem} from 'sentry/components/tokenizedInput/grid/useGridListItem'; -import {focusTarget} from 'sentry/components/tokenizedInput/grid/utils'; import {ComboBox} from 'sentry/components/tokenizedInput/token/comboBox'; import {InputBox} from 'sentry/components/tokenizedInput/token/inputBox'; import {t} from 'sentry/locale'; import {defined} from 'sentry/utils/defined'; import {FieldKind, FieldValueType, prettifyTagKey} from 'sentry/utils/fields'; +function resolveArgumentDisplayLabel( + parameterDefinition: + | {defaultLabel?: string; kind?: string; name?: string} + | null + | undefined, + fallbackLabel: string, + hasConditionalAggregates: boolean +): string { + if (parameterDefinition?.kind === 'column' && parameterDefinition.defaultLabel) { + return parameterDefinition.defaultLabel; + } + if (hasConditionalAggregates && isSearchFilterParameter(parameterDefinition)) { + return unwrapSearchFilterArgument(fallbackLabel); + } + return fallbackLabel; +} + interface ArithmeticTokenFunctionProps { item: Node; state: ListState; @@ -44,6 +69,7 @@ export function ArithmeticTokenFunction({ const functionArguments = token.attributes; const ref = useRef(null); + const skipArgumentFocusRef = useRef(false); const {rowProps, gridCellProps} = useGridListItem({ item, ref, @@ -51,6 +77,21 @@ export function ArithmeticTokenFunction({ focusable: defined(functionArguments) && functionArguments.length > 0, // if there are no arguments, it's not focusable }); + const onRowFocus = useCallback( + (evt: FocusEvent) => { + if (skipArgumentFocusRef.current) { + skipArgumentFocusRef.current = false; + return; + } + rowProps.onFocus?.(evt); + }, + [rowProps] + ); + + const onFunctionNameMouseDown = useCallback(() => { + skipArgumentFocusRef.current = true; + }, []); + const isFocused = item.key === state.selectionManager.focusedKey; const attrText = functionArguments.map(arg => arg.attribute).join(','); @@ -58,22 +99,35 @@ export function ArithmeticTokenFunction({ return ( - {token.function} - - + + {token.function} + + {/* Function tokens are keyed by position (`func:0`), so deleting an earlier + function reuses this component for the next one. Remount the arguments + grid when the token identity changes so we don't keep the previous + function's draft argument state. */} + + - + ); } -type Argument = {label: string; value: string}; +type Argument = FunctionArgumentInputProps['argument']; interface ArgumentsGridProps extends ArithmeticTokenFunctionProps { rowRef: RefObject; @@ -85,19 +139,21 @@ function ArgumentsGrid({ token: functionToken, rowRef, }: ArgumentsGridProps) { - const {getFieldDefinition} = useArithmeticBuilder(); + const {dispatch, getFieldDefinition, hasConditionalAggregates} = useArithmeticBuilder(); const resolveArgumentLabel = useCallback( (index: number, fallbackLabel: string) => { - const fieldDefinition = getFieldDefinition(functionToken.function)?.parameters?.[ - index - ]; - if (fieldDefinition?.kind === 'column') { - return fieldDefinition?.defaultLabel ?? fallbackLabel; - } - return fallbackLabel; + const fieldDefinition = getFieldDefinition( + functionToken.function, + functionToken.attributes.map(attr => attr.text) + )?.parameters?.[index]; + return resolveArgumentDisplayLabel( + fieldDefinition, + fallbackLabel, + hasConditionalAggregates + ); }, - [getFieldDefinition, functionToken] + [getFieldDefinition, functionToken, hasConditionalAggregates] ); const [args, setArguments] = useState( @@ -109,18 +165,44 @@ function ArgumentsGrid({ }) ); + const argsRef = useRef(args); + const functionTokenRef = useRef(functionToken); + + useEffect(() => { + argsRef.current = args; + }, [args]); + + useEffect(() => { + functionTokenRef.current = functionToken; + }, [functionToken]); + + const commitArgumentsIfChanged = useCallback(() => { + const nextArgs = argsRef.current.map(argument => argument.value).join(','); + const prevArgs = functionTokenRef.current.attributes + .map(attribute => attribute.text) + .join(','); + if (nextArgs === prevArgs) { + return; + } + dispatch({ + type: 'REPLACE_TOKEN', + token: functionTokenRef.current, + text: `${functionTokenRef.current.function}(${nextArgs})`, + }); + }, [dispatch]); + const updateArgumentAtIndex = (index: number, argument: string) => { - setArguments(prev => - prev.map((item, i) => - index === i - ? { - ...item, - value: argument, - label: resolveArgumentLabel(index, prettifyTagKey(argument)), - } - : item - ) + const next = argsRef.current.map((item, i) => + index === i + ? { + ...item, + value: argument, + label: resolveArgumentLabel(index, prettifyTagKey(argument)), + } + : item ); + argsRef.current = next; + setArguments(next); }; if (!args.length) { @@ -136,6 +218,7 @@ function ArgumentsGrid({ item={functionItem} state={functionListState} token={functionToken} + onArgumentsBlur={commitArgumentsIfChanged} onArgumentsChange={(index: number, argument: string) => updateArgumentAtIndex(index, argument) } @@ -149,6 +232,7 @@ interface GridListProps extends AriaGridListOptions, ArithmeticTokenFunctionProps { arguments: Argument[]; children: CollectionChildren; + onArgumentsBlur: () => void; onArgumentsChange: (index: number, argument: string) => void; rowRef: RefObject; } @@ -157,6 +241,7 @@ function ArgumentsGridList({ item: functionItem, state: functionListState, token: functionToken, + onArgumentsBlur, onArgumentsChange, arguments: functionArguments, rowRef, @@ -185,12 +270,15 @@ function ArgumentsGridList({ ref, }); + // Shrink to fit beside the function name; do not grow to the full chip width + // (that painted long filters over the name). return ( {index < functionToken.attributes.length - 1 && ','} @@ -229,93 +318,79 @@ function ArgumentsGridList({ ); } -interface InternalInputProps { - argument: Argument; - argumentIndex: number; - argumentItem: Node; - argumentRef: RefObject; - arguments: Argument[]; - argumentsListState: ListState; - functionItem: Node; - functionListState: ListState; - functionToken: TokenFunction; - onArgumentsChange: (index: number, argument: string) => void; - rowRef: RefObject; -} +function InternalInput(props: FunctionArgumentInputProps) { + const {getFieldDefinition, hasConditionalAggregates} = useArithmeticBuilder(); + const parameterDefinition = useMemo( + () => + getFieldDefinition( + props.functionToken.function, + props.functionToken.attributes.map(attr => attr.text) + )?.parameters?.[props.argumentIndex], + [getFieldDefinition, props.argumentIndex, props.functionToken] + ); -function InternalInput({ - argumentIndex, - functionToken, - functionItem, - functionListState, - argumentsListState, - argumentItem, - argument, - arguments: functionArguments, - onArgumentsChange, -}: InternalInputProps) { - const inputRef = useRef(null); - const gridCellRef = useRef(null); - const {rowProps, gridCellProps} = useGridListItem({ - item: argumentItem, - ref: gridCellRef, - state: argumentsListState, - focusable: true, - }); + if (hasConditionalAggregates && isSearchFilterParameter(parameterDefinition)) { + return ; + } - const isFocused = argumentItem.key === argumentsListState.selectionManager.focusedKey; - const hasNextArgument = argumentIndex < functionToken.attributes.length - 1; - const hasPrevArgument = argumentIndex > 0; + return ; +} + +function FunctionArgumentInput(props: FunctionArgumentInputProps) { + const {argument, argumentIndex, functionToken, onArgumentsChange} = props; + const { + clearSkipBlurFlush, + commitFunctionToken, + dataTestId, + flushArgumentsIfLeavingGrid, + focusArgument, + focusNextArgument, + gridCellProps, + gridCellRef, + hasNextArgument, + inputRef, + isFocused, + onKeyDown, + onKeyDownCapture, + rowProps, + shouldCloseOnInteractOutside, + } = useFunctionArgumentInput(props); const { - dispatch, functionArguments: builderFunctionArguments, getFieldDefinition, getSuggestedKey, + hasConditionalAggregates, } = useArithmeticBuilder(); const parameterDefinition = useMemo( - () => getFieldDefinition(functionToken.function)?.parameters?.[argumentIndex], + () => + getFieldDefinition( + functionToken.function, + functionToken.attributes.map(attr => attr.text) + )?.parameters?.[argumentIndex], [argumentIndex, getFieldDefinition, functionToken] ); const resolveDisplayLabel = useCallback( (fallback: string): string => - parameterDefinition?.kind === 'column' && parameterDefinition.defaultLabel - ? parameterDefinition.defaultLabel - : fallback, - [parameterDefinition] + resolveArgumentDisplayLabel( + parameterDefinition, + fallback, + hasConditionalAggregates + ), + [hasConditionalAggregates, parameterDefinition] ); const initialLabel = resolveDisplayLabel(argument.label); - const [inputValue, setInputValue] = useState(''); const [currentValue, setCurrentValue] = useState(initialLabel); const [isCurrentlyEditing, setIsCurrentlyEditing] = useState(false); - const [_selectionIndex, setSelectionIndex] = useState(0); // TODO - const [_isOpen, setIsOpen] = useState(false); // TODO - - const filterValue = inputValue.trim(); const displayValue = isCurrentlyEditing ? inputValue : currentValue; - const updateSelectionIndex = useCallback(() => { - setSelectionIndex(inputRef.current?.selectionStart ?? 0); - }, [setSelectionIndex]); - const resetInputValue = useCallback(() => { setInputValue(''); - updateSelectionIndex(); - }, [updateSelectionIndex]); - - const updateAttrsWith = useCallback( - (value: string) => { - const tokenArguments = functionArguments.map(arg => arg.value); - tokenArguments[argumentIndex] = value; - const argsStr = tokenArguments.join(','); - return argsStr; - }, - [argumentIndex, functionArguments] - ); + }, []); const attributesFilter = useMemo(() => { if (parameterDefinition?.kind === 'column') { @@ -348,6 +423,8 @@ function InternalInput({ const attributeItems = useAttributeItems(allowedAttributes); const items = useMemo(() => { + const filterValue = inputValue.trim(); + if (parameterDefinition?.kind === 'value' && parameterDefinition.options) { return parameterDefinition.options .filter( @@ -393,20 +470,16 @@ function InternalInput({ } return result; - }, [parameterDefinition, filterValue, attributeItems]); - - const shouldCloseOnInteractOutside = useCallback((el: Element) => { - return !gridCellRef.current?.contains(el); - }, []); + }, [attributeItems, inputValue, parameterDefinition]); - const onClick = useCallback(() => { - updateSelectionIndex(); - }, [updateSelectionIndex]); - - const onInputBlur = useCallback(() => { - resetInputValue(); - setIsCurrentlyEditing(false); - }, [resetInputValue]); + const onInputBlur = useCallback( + (evt?: FocusEvent) => { + resetInputValue(); + setIsCurrentlyEditing(false); + flushArgumentsIfLeavingGrid(evt); + }, + [flushArgumentsIfLeavingGrid, resetInputValue] + ); const resolveValue = useCallback( (raw: string): string => { @@ -423,44 +496,30 @@ function InternalInput({ [parameterDefinition] ); - const onTextInputBlur = useCallback(() => { - if (inputValue) { - onArgumentsChange(argumentIndex, inputValue); - dispatch({ - text: `${functionToken.function}(${updateAttrsWith(inputValue)})`, - type: 'REPLACE_TOKEN', - token: functionToken, - focusOverride: { - itemKey: nextTokenKeyOfKind( - functionListState, - functionToken, - TokenKind.FREE_TEXT - ), - }, - }); - } - resetInputValue(); - setIsCurrentlyEditing(false); - }, [ - argumentIndex, - dispatch, - functionListState, - functionToken, - inputValue, - onArgumentsChange, - resetInputValue, - updateAttrsWith, - ]); - - const onInputChange = useCallback( - (evt: ChangeEvent) => { - setInputValue(evt.target.value); - setCurrentValue(evt.target.value); - setSelectionIndex(evt.target.selectionStart ?? 0); + // Non-filter free-text values (e.g. apdex threshold) flush pending edits when leaving. + const onTextInputBlur = useCallback( + (evt: FocusEvent) => { + if (inputValue) { + onArgumentsChange(argumentIndex, inputValue); + } + resetInputValue(); + setIsCurrentlyEditing(false); + flushArgumentsIfLeavingGrid(evt); }, - [setInputValue] + [ + argumentIndex, + flushArgumentsIfLeavingGrid, + inputValue, + onArgumentsChange, + resetInputValue, + ] ); + const onInputChange = useCallback((evt: ChangeEvent) => { + setInputValue(evt.target.value); + setCurrentValue(evt.target.value); + }, []); + const onInputCommit = useCallback(() => { let value = inputValue.trim() || argument.label; @@ -472,34 +531,19 @@ function InternalInput({ setCurrentValue(resolveDisplayLabel(value)); onArgumentsChange(argumentIndex, value); - - dispatch({ - text: `${functionToken.function}(${updateAttrsWith(value)})`, - type: 'REPLACE_TOKEN', - token: functionToken, - focusOverride: { - itemKey: nextTokenKeyOfKind( - functionListState, - functionToken, - TokenKind.FREE_TEXT - ), - }, - }); + commitFunctionToken(value); resetInputValue(); }, [ - inputValue, argument.label, + argumentIndex, + commitFunctionToken, getSuggestedKey, + inputValue, + onArgumentsChange, parameterDefinition, + resetInputValue, resolveDisplayLabel, resolveValue, - onArgumentsChange, - argumentIndex, - dispatch, - functionToken, - updateAttrsWith, - functionListState, - resetInputValue, ]); const onInputEscape = useCallback(() => { @@ -512,148 +556,54 @@ function InternalInput({ // We're stopping propagation because `useGridListItem` in the parent component // always steals and sets focus to the first child and we don't want that happening. evt.stopPropagation(); + clearSkipBlurFlush(); // Explicitly focus target on this item because we're calling evt.stopPropagation(). // If this isn't called, the argument collection doesn't shift focus to current arg // causing bugs. Test for this behaviour can be found in // static/app/components/arithmeticBuilder/token/index.spec.tsx -t 'shifts focus between args correctly' - focusTarget(argumentsListState, argumentItem.key); + focusArgument(); setIsCurrentlyEditing(true); resetInputValue(); }, - [argumentItem.key, argumentsListState, resetInputValue] - ); - - const onKeyDownCapture = useCallback( - (evt: React.KeyboardEvent) => { - // At start and pressing left arrow, focus the previous full token - if ( - evt.currentTarget.selectionStart === 0 && - evt.currentTarget.selectionEnd === 0 && - evt.key === 'ArrowLeft' - ) { - if (hasPrevArgument) { - focusTarget( - argumentsListState, - argumentsListState.collection.getKeyBefore(argumentItem.key) - ); - } else { - focusTarget( - functionListState, - functionListState.collection.getKeyBefore(functionItem.key) - ); - } - return; - } - - // At end and pressing right arrow, focus the next full token - if ( - evt.currentTarget.selectionStart === evt.currentTarget.value.length && - evt.currentTarget.selectionEnd === evt.currentTarget.value.length && - evt.key === 'ArrowRight' - ) { - if (hasNextArgument) { - focusTarget( - argumentsListState, - argumentsListState.collection.getKeyAfter(argumentItem.key) - ); - } else { - focusTarget( - functionListState, - functionListState.collection.getKeyAfter(functionItem.key) - ); - } - return; - } - }, - [ - hasPrevArgument, - argumentsListState, - argumentItem.key, - functionListState, - functionItem.key, - hasNextArgument, - ] + [clearSkipBlurFlush, focusArgument, resetInputValue] ); - const onKeyDown = useCallback( - (evt: KeyboardEvent) => { - // TODO: handle meta keys - - // At start and pressing backspace, delete this token - if ( - evt.currentTarget.selectionStart === 0 && - evt.currentTarget.selectionEnd === 0 && - evt.key === 'Backspace' - ) { - const itemKey = functionListState.collection.getKeyBefore(functionItem.key); - dispatch({ - type: 'DELETE_TOKEN', - token: functionToken, - focusOverride: defined(itemKey) ? {itemKey} : undefined, - }); - } - - // At end and pressing delete, focus the next full token - if ( - evt.currentTarget.selectionStart === evt.currentTarget.value.length && - evt.currentTarget.selectionEnd === evt.currentTarget.value.length && - evt.key === 'Delete' - ) { - const itemKey = functionListState.collection.getKeyBefore(functionItem.key); - dispatch({ - type: 'DELETE_TOKEN', - token: functionToken, - focusOverride: defined(itemKey) ? {itemKey} : undefined, - }); - } + // Free-text value args should keep their current text on focus so the user can edit it. + // ComboBox clears on focus to type a new query. + const onTextInputFocus = useCallback( + (evt: FocusEvent) => { + evt.stopPropagation(); + clearSkipBlurFlush(); + focusArgument(); + setIsCurrentlyEditing(true); + setInputValue(currentValue); }, - [dispatch, functionToken, functionListState, functionItem] + [clearSkipBlurFlush, currentValue, focusArgument] ); const onOptionSelected = useCallback( (option: SelectOptionWithKey) => { setCurrentValue(resolveDisplayLabel(prettifyTagKey(option.value))); + onArgumentsChange(argumentIndex, option.value); if (hasNextArgument) { - focusTarget( - argumentsListState, - argumentsListState.collection.getKeyAfter(argumentItem.key) - ); - onArgumentsChange(argumentIndex, option.value); + focusNextArgument(); } else { - dispatch({ - text: `${functionToken.function}(${updateAttrsWith(option.value)})`, - type: 'REPLACE_TOKEN', - token: functionToken, - focusOverride: { - itemKey: nextTokenKeyOfKind( - functionListState, - functionToken, - TokenKind.FREE_TEXT - ), - }, - }); + commitFunctionToken(option.value); } resetInputValue(); }, [ + argumentIndex, + commitFunctionToken, + focusNextArgument, hasNextArgument, - resolveDisplayLabel, - resetInputValue, - argumentsListState, - argumentItem.key, onArgumentsChange, - argumentIndex, - dispatch, - functionToken, - updateAttrsWith, - functionListState, + resetInputValue, + resolveDisplayLabel, ] ); - const onPaste = useCallback((_evt: React.ClipboardEvent) => { - // TODO - }, []); - + // Free-text value args with no options (e.g. apdex threshold) use a plain input. if ( parameterDefinition?.kind === 'value' && (!defined(parameterDefinition.options) || !parameterDefinition.options.length) @@ -666,16 +616,14 @@ function InternalInput({ ref={inputRef} inputLabel={t('Add a value')} inputValue={displayValue} - onClick={onClick} onInputBlur={onTextInputBlur} onInputChange={onInputChange} onInputCommit={onInputCommit} onInputEscape={onInputEscape} - onInputFocus={onInputFocus} + onInputFocus={onTextInputFocus} onKeyDown={onKeyDown} onKeyDownCapture={onKeyDownCapture} /> - {argumentIndex < functionToken.attributes.length - 1 && ','} ); @@ -698,12 +646,9 @@ function InternalInput({ : t('Select an option') } inputValue={displayValue} - filterValue={filterValue} - tabIndex={ - argumentItem.key === argumentsListState.selectionManager.focusedKey ? 0 : -1 - } + filterValue={inputValue} + tabIndex={isFocused ? 0 : -1} shouldCloseOnInteractOutside={shouldCloseOnInteractOutside} - onClick={onClick} onInputBlur={onInputBlur} onInputChange={onInputChange} onInputCommit={onInputCommit} @@ -711,14 +656,8 @@ function InternalInput({ onInputFocus={onInputFocus} onKeyDown={onKeyDown} onKeyDownCapture={onKeyDownCapture} - onOpenChange={setIsOpen} onOptionSelected={onOptionSelected} - onPaste={onPaste} - data-test-id={ - functionListState.collection.getLastKey() === functionItem.key - ? 'arithmetic-builder-argument-input' - : undefined - } + data-test-id={dataTestId} > {keyItem => itemIsSection(keyItem) ? ( @@ -790,36 +729,21 @@ const FunctionWrapper = styled('div')<{state: 'invalid' | 'warning' | 'valid'}>` } `; -const ArgumentGridRow = styled('div')` - display: flex; - align-items: center; - position: relative; - height: 100%; - flex: 0 1 auto; - max-width: fit-content; -`; - -const ArgumentGridCell = styled('div')` - display: flex; - align-items: center; - height: 100%; - - > div input { - max-width: 130px !important; - min-width: 0 !important; - white-space: nowrap !important; - } -`; - const BaseGridCell = styled('div')` display: flex; align-items: center; position: relative; height: 100%; min-height: 22px; + min-width: 0; +`; + +/* Name + delete must not shrink so long arguments cannot cover them. */ +const FunctionChromeCell = styled(BaseGridCell)` + flex-shrink: 0; `; -const FunctionGridCell = styled(BaseGridCell)` +const FunctionNameCell = styled(FunctionChromeCell)` color: ${p => p.theme.colors.green500}; padding-left: ${p => p.theme.space.xs}; `; diff --git a/static/app/components/arithmeticBuilder/token/grid.tsx b/static/app/components/arithmeticBuilder/token/grid.tsx index b679d67c727c..e363dbe9ab39 100644 --- a/static/app/components/arithmeticBuilder/token/grid.tsx +++ b/static/app/components/arithmeticBuilder/token/grid.tsx @@ -1,4 +1,4 @@ -import {useLayoutEffect, useMemo, useRef} from 'react'; +import {useCallback, useLayoutEffect, useMemo, useRef, type PointerEvent} from 'react'; import styled from '@emotion/styled'; import type {AriaGridListOptions} from '@react-aria/gridlist'; import {Item} from '@react-stately/collections'; @@ -25,6 +25,8 @@ import {ArithmeticTokenParenthesis} from 'sentry/components/arithmeticBuilder/to import {ArithmeticBuilderTokenReference} from 'sentry/components/arithmeticBuilder/token/reference'; import {computeNextAllowedTokenKinds} from 'sentry/components/arithmeticBuilder/validator'; import {useGridList} from 'sentry/components/tokenizedInput/grid/useGridList'; +import {focusTarget} from 'sentry/components/tokenizedInput/grid/utils'; +import {shiftFocusToChild} from 'sentry/components/tokenizedInput/token/utils'; import {t} from 'sentry/locale'; import {defined} from 'sentry/utils/defined'; @@ -107,13 +109,46 @@ function GridList({showPlaceholder, ...props}: GridListProps) { useApplyFocusOverride(state); + const onGridPaddingPointerDown = useCallback( + (evt: PointerEvent) => { + if (evt.target !== evt.currentTarget) { + gridProps.onPointerDown?.(evt); + return; + } + + // Padding clicks would otherwise focus the grid itself, which has no caret. + evt.preventDefault(); + + const rect = evt.currentTarget.getBoundingClientRect(); + const closerToStart = evt.clientY < rect.top + rect.height / 2; + const key = closerToStart + ? state.collection.getFirstKey() + : state.collection.getLastKey(); + if (!key) { + return; + } + + focusTarget(state, key); + + const item = state.collection.getItem(key); + const rows = Array.from( + evt.currentTarget.querySelectorAll('[role="row"]') + ).filter(row => row.closest('[role="grid"]') === evt.currentTarget); + const row = closerToStart ? rows.at(0) : rows.at(-1); + if (row && item) { + shiftFocusToChild(row, item, state); + } + }, + [gridProps, state] + ); + const nextAllowedTokenKindsAtIndex = useMemo(() => { const tokens = Array.from(state.collection, item => item.value); return computeNextAllowedTokenKinds(tokens); }, [state.collection]); return ( - + {Array.from(state.collection, (item, i) => { const token = item.value; @@ -197,11 +232,23 @@ function GridList({showPlaceholder, ...props}: GridListProps) { } const TokenGridWrapper = styled('div')` - padding: ${p => p.theme.space.sm}; + box-sizing: border-box; + width: 100%; + min-width: 0; + max-width: 100%; + min-height: 100%; + /* Match SearchQueryBuilder so equation and aggregate filter rows share height. + * +1px accounts for the border; keep horizontal padding so empty-field clicks + * still land on this grid and route into an input. */ + padding-top: calc(${p => p.theme.space.xs} + 1px); + padding-bottom: calc(${p => p.theme.space.xs} + 1px); + padding-left: ${p => p.theme.space.sm}; + padding-right: ${p => p.theme.space.sm}; display: flex; - align-items: center; + align-items: stretch; row-gap: ${p => p.theme.space.xs}; flex-wrap: wrap; + cursor: text; &:focus { outline: none; diff --git a/static/app/components/arithmeticBuilder/token/index.spec.tsx b/static/app/components/arithmeticBuilder/token/index.spec.tsx index c77368bfaf72..347b3d41a2d0 100644 --- a/static/app/components/arithmeticBuilder/token/index.spec.tsx +++ b/static/app/components/arithmeticBuilder/token/index.spec.tsx @@ -2,12 +2,14 @@ import type {Dispatch} from 'react'; import {useCallback} from 'react'; import { + fireEvent, render, screen, userEvent, waitFor, within, } from 'sentry-test/reactTestingLibrary'; +import {getEmotionRules} from 'sentry-test/utils'; import type {ArithmeticBuilderAction} from 'sentry/components/arithmeticBuilder/action'; import {useArithmeticBuilderAction} from 'sentry/components/arithmeticBuilder/action'; @@ -18,7 +20,8 @@ import { TokenKind, } from 'sentry/components/arithmeticBuilder/token'; import {TokenGrid} from 'sentry/components/arithmeticBuilder/token/grid'; -import {FieldKind, getFieldDefinition} from 'sentry/utils/fields'; +import type {GetTagValues} from 'sentry/components/searchQueryBuilder'; +import {FieldKind, getExploreEquationFieldDefinition} from 'sentry/utils/fields'; const aggregations = ['avg', 'avg_if', 'sum', 'epm', 'count', 'count_unique', 'count_if']; @@ -29,14 +32,6 @@ const functionArguments = [ {name: 'span.description', kind: FieldKind.TAG}, ]; -const getSpanFieldDefinition = (key: string) => { - const argument = functionArguments.find( - functionArgument => functionArgument.name === key - ); - - return getFieldDefinition(key, 'span', argument?.kind); -}; - const getSuggestedKey = (key: string) => { switch (key) { case 'duration': @@ -52,10 +47,17 @@ const getSuggestedKey = (key: string) => { interface TokensProp { expression: string; dispatch?: Dispatch; + getFilterTagValues?: GetTagValues; + /** + * Mirrors `explore-conditional-aggregates`. Defaults to on so EAP filter-first + * coverage stays the default; Discover 3/4-arg `_if` tests pass `false`. + */ + hasConditionalAggregates?: boolean; references?: Set; } function Tokens(props: TokensProp) { + const hasConditionalAggregates = props.hasConditionalAggregates ?? true; const {state, dispatch} = useArithmeticBuilderAction({ initialExpression: props.expression, references: props.references, @@ -69,6 +71,22 @@ function Tokens(props: TokensProp) { [dispatch, props] ); + const getSpanFieldDefinition = useCallback( + (key: string, attributeTexts?: readonly string[]) => { + const argument = functionArguments.find( + functionArgument => functionArgument.name === key + ); + + return getExploreEquationFieldDefinition( + key, + argument?.kind, + hasConditionalAggregates, + attributeTexts + ); + }, + [hasConditionalAggregates] + ); + return ( @@ -95,6 +117,13 @@ function getLastInput() { } describe('token', () => { + it('focuses the last input when clicking empty space in the field', async () => { + render(); + + await userEvent.click(screen.getByRole('grid', {name: 'Enter an equation'})); + + expect(getLastInput()).toHaveFocus(); + }); describe('ArithmeticTokenFreeText', () => { it('renders default place holder', async () => { render(); @@ -161,7 +190,7 @@ describe('token', () => { }); it('fills in every argument when selecting avg_if', async () => { - render(); + render(); const input = screen.getByRole('combobox', {name: 'Add a term'}); @@ -176,6 +205,41 @@ describe('token', () => { ).toBeInTheDocument(); }); + it('fills in filter-first arguments when selecting avg_if with the feature', async () => { + render(); + + const input = screen.getByRole('combobox', {name: 'Add a term'}); + + await userEvent.click(input); + await userEvent.type(input, 'avg_if'); + await userEvent.click(screen.getByRole('option', {name: 'avg_if'})); + + expect( + await screen.findByRole('row', { + name: 'avg_if(``,span.duration)', + }) + ).toBeInTheDocument(); + }); + + it('does not render the EAP filter argument input when the feature is off', async () => { + render( + + ); + + expect( + await screen.findByRole('row', { + name: 'avg_if(`span.op:db`,span.duration)', + }) + ).toBeInTheDocument(); + + expect( + screen.queryByRole('combobox', {name: 'Add a filter'}) + ).not.toBeInTheDocument(); + }); + it('allows selecting function with no arguments using mouse', async () => { render(); @@ -854,10 +918,568 @@ describe('token', () => { await waitFor(() => { expect(thirdArg).toHaveFocus(); }); - await userEvent.keyboard('db'); + await userEvent.clear(thirdArg); + await userEvent.type(thirdArg, 'db'); expect(thirdArg).toHaveValue('db'); }); + it('keeps filter text when focusing an _if filter argument', async () => { + render(); + + expect( + await screen.findByRole('row', { + name: 'avg_if(`span.op:db`,span.duration)', + }) + ).toBeInTheDocument(); + + const filterArg = within( + screen.getByRole('grid', {name: 'Enter arguments'}) + ).getByRole('combobox', {name: 'Add a filter'}); + expect(filterArg).toHaveValue('span.op:db'); + + await userEvent.click(filterArg); + expect(filterArg).toHaveValue('span.op:db'); + }); + + it('autocompletes attribute keys in an _if filter argument', async () => { + render(); + + expect( + await screen.findByRole('row', { + name: 'avg_if(``,span.duration)', + }) + ).toBeInTheDocument(); + + const filterArg = within( + screen.getByRole('grid', {name: 'Enter arguments'}) + ).getByRole('combobox', {name: 'Add a filter'}); + + await userEvent.click(filterArg); + await userEvent.type(filterArg, 'span.op'); + expect(screen.getByRole('option', {name: 'span.op:'})).toBeInTheDocument(); + await userEvent.click(screen.getByRole('option', {name: 'span.op:'})); + + expect(filterArg).toHaveValue('span.op:'); + expect(filterArg).toHaveFocus(); + // Caret must sit after the colon so value suggestions kick in (Chrome used to + // reset this when focus returned from the listbox). + expect(filterArg).toHaveProperty('selectionStart', 'span.op:'.length); + expect(filterArg).toHaveProperty('selectionEnd', 'span.op:'.length); + }); + + it('switches to value suggestions after selecting a filter key', async () => { + const getFilterTagValues = jest.fn().mockResolvedValue([{value: 'db'}]); + + render( + + ); + + expect( + await screen.findByRole('row', { + name: 'avg_if(``,span.duration)', + }) + ).toBeInTheDocument(); + + const filterArg = within( + screen.getByRole('grid', {name: 'Enter arguments'}) + ).getByRole('combobox', {name: 'Add a filter'}); + + await userEvent.click(filterArg); + await userEvent.type(filterArg, 'span.op'); + await userEvent.click(screen.getByRole('option', {name: 'span.op:'})); + + expect(filterArg).toHaveValue('span.op:'); + await waitFor(() => { + expect(getFilterTagValues).toHaveBeenCalled(); + }); + expect(await screen.findByRole('option', {name: 'db'})).toBeInTheDocument(); + }); + + it('autocompletes tag values in an _if filter argument', async () => { + const getFilterTagValues = jest.fn(({tag, searchQuery}) => { + if (tag.key === 'span.op') { + return Promise.resolve( + [{value: 'db'}, {value: 'http'}].filter( + item => !searchQuery || item.value.includes(searchQuery) + ) + ); + } + return Promise.resolve([]); + }); + + render( + + ); + + expect( + await screen.findByRole('row', { + name: 'avg_if(``,span.duration)', + }) + ).toBeInTheDocument(); + + const filterArg = within( + screen.getByRole('grid', {name: 'Enter arguments'}) + ).getByRole('combobox', {name: 'Add a filter'}); + + await userEvent.click(filterArg); + await userEvent.type(filterArg, 'span.op:'); + + await waitFor(() => { + expect(getFilterTagValues).toHaveBeenCalled(); + }); + expect(await screen.findByRole('option', {name: 'db'})).toBeInTheDocument(); + await userEvent.click(screen.getByRole('option', {name: 'db'})); + + expect(filterArg).toHaveValue('span.op:db'); + expect(filterArg).toHaveFocus(); + await waitFor(() => { + expect(screen.queryByRole('option')).not.toBeInTheDocument(); + }); + }); + + it('continues value autocomplete inside an unclosed quoted value', async () => { + const getFilterTagValues = jest.fn(({searchQuery}) => { + return Promise.resolve( + [{value: 'hello world'}, {value: 'hello there'}].filter( + item => !searchQuery || item.value.includes(searchQuery) + ) + ); + }); + + render( + + ); + + expect( + await screen.findByRole('row', { + name: 'avg_if(``,span.duration)', + }) + ).toBeInTheDocument(); + + const filterArg = within( + screen.getByRole('grid', {name: 'Enter arguments'}) + ).getByRole('combobox', {name: 'Add a filter'}); + + await userEvent.click(filterArg); + await userEvent.type(filterArg, 'span.description:"hello '); + + await waitFor(() => { + expect(getFilterTagValues).toHaveBeenCalledWith( + expect.objectContaining({searchQuery: 'hello '}) + ); + }); + expect( + await screen.findByRole('option', {name: 'hello world'}) + ).toBeInTheDocument(); + await userEvent.click(screen.getByRole('option', {name: 'hello world'})); + expect(filterArg).toHaveValue('span.description:"hello world"'); + }); + + it('shows key autocomplete after a completed value and trailing space', async () => { + const getFilterTagValues = jest.fn().mockResolvedValue([{value: 'db'}]); + + render( + + ); + + expect( + await screen.findByRole('row', { + name: 'avg_if(``,span.duration)', + }) + ).toBeInTheDocument(); + + const filterArg = within( + screen.getByRole('grid', {name: 'Enter arguments'}) + ).getByRole('combobox', {name: 'Add a filter'}); + + await userEvent.click(filterArg); + await userEvent.type(filterArg, 'span.op:db '); + + expect( + await screen.findByRole('option', {name: 'span.description:'}) + ).toBeInTheDocument(); + expect(screen.queryByRole('option', {name: 'db'})).not.toBeInTheDocument(); + await userEvent.click(screen.getByRole('option', {name: 'span.description:'})); + expect(filterArg).toHaveValue('span.op:db span.description:'); + }); + + it('autocompletes keys after a boolean operator in a compound filter', async () => { + render(); + + expect( + await screen.findByRole('row', { + name: 'avg_if(``,span.duration)', + }) + ).toBeInTheDocument(); + + const filterArg = within( + screen.getByRole('grid', {name: 'Enter arguments'}) + ).getByRole('combobox', {name: 'Add a filter'}); + + await userEvent.click(filterArg); + await userEvent.type(filterArg, 'span.op:db and '); + + expect( + await screen.findByRole('option', {name: 'span.description:'}) + ).toBeInTheDocument(); + await userEvent.click(screen.getByRole('option', {name: 'span.description:'})); + + expect(filterArg).toHaveValue('span.op:db and span.description:'); + }); + + it('does not show a values dropdown when there are no matching values', async () => { + const getFilterTagValues = jest.fn().mockResolvedValue([]); + + render( + + ); + + expect( + await screen.findByRole('row', { + name: 'avg_if(``,span.duration)', + }) + ).toBeInTheDocument(); + + const filterArg = within( + screen.getByRole('grid', {name: 'Enter arguments'}) + ).getByRole('combobox', {name: 'Add a filter'}); + + await userEvent.click(filterArg); + await userEvent.type(filterArg, 'span.description'); + expect(screen.getByRole('option', {name: 'span.description:'})).toBeInTheDocument(); + + await userEvent.type(filterArg, ':'); + await waitFor(() => { + expect(getFilterTagValues).toHaveBeenCalled(); + expect(screen.queryByRole('option')).not.toBeInTheDocument(); + }); + expect(screen.queryByText('No options found')).not.toBeInTheDocument(); + }); + + it('shows filter key suggestions when editing an existing _if filter', async () => { + const getFilterTagValues = jest.fn().mockResolvedValue([{value: 'db'}]); + + render( + + ); + + expect( + await screen.findByRole('row', { + name: 'avg_if(`span.op:db`,span.duration)', + }) + ).toBeInTheDocument(); + + const filterArg = within( + screen.getByRole('grid', {name: 'Enter arguments'}) + ).getByRole('combobox', {name: 'Add a filter'}); + + await userEvent.click(filterArg); + expect(filterArg).toHaveFocus(); + // Move to the start of the key so autocomplete switches out of value mode. + (filterArg as HTMLInputElement).setSelectionRange(0, 0); + fireEvent.keyUp(filterArg, {key: 'Home', code: 'Home'}); + await waitFor(() => { + expect(screen.getByRole('option', {name: 'span.op:'})).toBeInTheDocument(); + }); + expect(screen.queryByRole('option', {name: 'db'})).not.toBeInTheDocument(); + }); + + it('shows filter value suggestions when the cursor is after the colon', async () => { + const getFilterTagValues = jest.fn().mockResolvedValue([{value: 'db'}]); + + render( + + ); + + expect( + await screen.findByRole('row', { + name: 'avg_if(`span.op:db`,span.duration)', + }) + ).toBeInTheDocument(); + + const filterArg = within( + screen.getByRole('grid', {name: 'Enter arguments'}) + ).getByRole('combobox', {name: 'Add a filter'}); + + await userEvent.click(filterArg); + (filterArg as HTMLInputElement).setSelectionRange(9, 9); + fireEvent.keyUp(filterArg, {key: 'ArrowLeft', code: 'ArrowLeft'}); + + await waitFor(() => { + expect(getFilterTagValues).toHaveBeenCalled(); + }); + expect(await screen.findByRole('option', {name: 'db'})).toBeInTheDocument(); + expect(screen.queryByRole('option', {name: 'span.op:'})).not.toBeInTheDocument(); + }); + + it('does not open filter autocomplete when clicking the function name', async () => { + const getFilterTagValues = jest.fn().mockResolvedValue([{value: 'db'}]); + + render( + + ); + + const functionRow = await screen.findByRole('row', { + name: 'avg_if(`span.op:db`,span.duration)', + }); + + await userEvent.click(within(functionRow).getByText('avg_if')); + + expect( + within(functionRow).getByRole('combobox', {name: 'Add a filter'}) + ).not.toHaveFocus(); + expect(screen.queryByRole('option', {name: 'span.op:'})).not.toBeInTheDocument(); + }); + + it('renders filter-first avg_if arguments and allows navigating between them', async () => { + render(); + + expect( + await screen.findByRole('row', { + name: 'avg_if(`span.op:db`,span.duration)', + }) + ).toBeInTheDocument(); + + const args = within( + screen.getByRole('grid', {name: 'Enter arguments'}) + ).queryAllByRole('gridcell'); + + expect(args).toHaveLength(2); + + const filterArg = within( + screen.getByRole('grid', {name: 'Enter arguments'}) + ).getByRole('combobox', {name: 'Add a filter'}); + // Filter args are shown without backticks; wrapping is applied on commit. + expect(filterArg).toHaveValue('span.op:db'); + + const columnArg = within( + screen.getByRole('grid', {name: 'Enter arguments'}) + ).getByRole('combobox', {name: 'Select an attribute'}); + expect(columnArg).toHaveAttribute('placeholder', 'span.duration'); + + await userEvent.click(columnArg); + await userEvent.type(columnArg, 'span.self_time'); + expect(screen.getByRole('option', {name: 'span.self_time'})).toBeInTheDocument(); + await userEvent.click(screen.getByRole('option', {name: 'span.self_time'})); + + await waitFor(() => { + expect(getLastInput()).toHaveFocus(); + }); + expect( + screen.queryByRole('option', {name: 'span.self_time'}) + ).not.toBeInTheDocument(); + }); + + it('commits _if filter on blur when leaving the equation', async () => { + const dispatch = jest.fn(); + render(); + + expect( + await screen.findByRole('row', { + name: 'avg_if(``,span.duration)', + }) + ).toBeInTheDocument(); + + const filterArg = within( + screen.getByRole('grid', {name: 'Enter arguments'}) + ).getByRole('combobox', {name: 'Add a filter'}); + + await userEvent.click(filterArg); + await userEvent.type(filterArg, 'span.op:db'); + await userEvent.click(getLastInput()); + + await waitFor(() => { + expect(dispatch).toHaveBeenCalledWith( + expect.objectContaining({ + type: 'REPLACE_TOKEN', + text: 'avg_if(`span.op:db`,span.duration)', + }) + ); + }); + }); + + it('clears _if filter on blur when the filter input is emptied', async () => { + const dispatch = jest.fn(); + render( + + ); + + expect( + await screen.findByRole('row', { + name: 'avg_if(`span.op:db`,span.duration)', + }) + ).toBeInTheDocument(); + + const filterArg = within( + screen.getByRole('grid', {name: 'Enter arguments'}) + ).getByRole('combobox', {name: 'Add a filter'}); + + await userEvent.click(filterArg); + await userEvent.clear(filterArg); + await userEvent.click(getLastInput()); + + await waitFor(() => { + expect(dispatch).toHaveBeenCalledWith( + expect.objectContaining({ + type: 'REPLACE_TOKEN', + text: 'avg_if(``,span.duration)', + }) + ); + }); + }); + + it('clears _if filter on Enter when the filter input is emptied', async () => { + const dispatch = jest.fn(); + render( + + ); + + expect( + await screen.findByRole('row', { + name: 'avg_if(`span.op:db`,span.duration)', + }) + ).toBeInTheDocument(); + + const filterArg = within( + screen.getByRole('grid', {name: 'Enter arguments'}) + ).getByRole('combobox', {name: 'Add a filter'}); + + await userEvent.click(filterArg); + await userEvent.clear(filterArg); + await userEvent.keyboard('{Enter}'); + + await waitFor(() => { + expect(dispatch).toHaveBeenCalledWith( + expect.objectContaining({ + type: 'REPLACE_TOKEN', + text: 'avg_if(``,span.duration)', + }) + ); + }); + }); + + it('does not delete the function when Backspace clears a selected filter', async () => { + const dispatch = jest.fn(); + render( + + ); + + const filterArg = within( + await screen.findByRole('grid', {name: 'Enter arguments'}) + ).getByRole('combobox', {name: 'Add a filter'}); + + await userEvent.click(filterArg); + await waitFor(() => { + expect(filterArg).toHaveValue('span.op:db'); + }); + const filterInput = filterArg as HTMLInputElement; + filterInput.setSelectionRange(0, filterInput.value.length); + await userEvent.keyboard('{Backspace}'); + + expect( + screen.getByRole('row', {name: 'avg_if(`span.op:db`,span.duration)'}) + ).toBeInTheDocument(); + expect(dispatch).not.toHaveBeenCalledWith( + expect.objectContaining({type: 'DELETE_TOKEN'}) + ); + }); + + it('does not rewrite the function when moving from filter to another argument', async () => { + const dispatch = jest.fn(); + render(); + + const argsGrid = await screen.findByRole('grid', {name: 'Enter arguments'}); + const filterArg = within(argsGrid).getByRole('combobox', {name: 'Add a filter'}); + const columnArg = within(argsGrid).getByRole('combobox', { + name: 'Select an attribute', + }); + + await userEvent.click(filterArg); + await userEvent.type(filterArg, 'span.op:db'); + await userEvent.click(columnArg); + + await waitFor(() => { + expect(columnArg).toHaveFocus(); + }); + expect(dispatch).not.toHaveBeenCalledWith( + expect.objectContaining({type: 'REPLACE_TOKEN'}) + ); + }); + + it('flushes pending filter edits when leaving the arguments grid', async () => { + const dispatch = jest.fn(); + render(); + + const argsGrid = await screen.findByRole('grid', {name: 'Enter arguments'}); + const filterArg = within(argsGrid).getByRole('combobox', {name: 'Add a filter'}); + const columnArg = within(argsGrid).getByRole('combobox', { + name: 'Select an attribute', + }); + + await userEvent.click(filterArg); + await userEvent.type(filterArg, 'span.op:db'); + await userEvent.click(columnArg); + await waitFor(() => { + expect(columnArg).toHaveFocus(); + }); + await userEvent.click(getLastInput()); + + await waitFor(() => { + expect(dispatch).toHaveBeenCalledWith( + expect.objectContaining({ + type: 'REPLACE_TOKEN', + text: 'avg_if(`span.op:db`,span.duration)', + }) + ); + }); + }); + + it('keeps Discover-style avg_if arguments editable when the feature is on', async () => { + render(); + + const argumentsGrid = await screen.findByRole('grid', {name: 'Enter arguments'}); + + expect( + within(argumentsGrid).queryByRole('combobox', {name: 'Add a filter'}) + ).not.toBeInTheDocument(); + + const [numberArg, stringArg] = within(argumentsGrid).getAllByRole('combobox', { + name: 'Select an attribute', + }); + expect(numberArg).toHaveValue('span.duration'); + expect(stringArg).toHaveValue('span.op'); + expect( + within(argumentsGrid).getByRole('combobox', {name: 'Select an option'}) + ).toBeInTheDocument(); + expect( + within(argumentsGrid).getByRole('textbox', {name: 'Add a value'}) + ).toHaveValue('queue.process'); + }); + it('suggests attributes for each argument of avg_if', async () => { render(); @@ -893,6 +1515,27 @@ describe('token', () => { expect(valueArg).toHaveValue('queue.process'); }); + + it('suggests column attributes for filter-first avg_if', async () => { + render(); + + const argumentsGrid = await screen.findByRole('grid', {name: 'Enter arguments'}); + + const filterArg = within(argumentsGrid).getByRole('combobox', { + name: 'Add a filter', + }); + const columnArg = within(argumentsGrid).getByRole('combobox', { + name: 'Select an attribute', + }); + + expect(filterArg).toHaveValue('span.op:db'); + + await userEvent.click(columnArg); + expect(screen.getAllByRole('option').map(option => option.textContent)).toEqual([ + 'span.duration', + 'span.self_time', + ]); + }); }); it('shifts focus between args correctly', async () => { @@ -947,6 +1590,41 @@ describe('token', () => { expect(screen.queryByRole('option', {name: 'span.op'})).not.toBeInTheDocument(); }); + it('shifts focus between filter-first avg_if args correctly', async () => { + render(); + + expect( + await screen.findByRole('row', { + name: 'avg_if(`span.op:db`,span.duration)', + }) + ).toBeInTheDocument(); + + const argsGrid = screen.getByRole('grid', {name: 'Enter arguments'}); + expect(within(argsGrid).queryAllByRole('gridcell')).toHaveLength(2); + + const filterArg = within(argsGrid).getByRole('combobox', { + name: 'Add a filter', + }); + const columnArg = within(argsGrid).getByRole('combobox', { + name: 'Select an attribute', + }); + + await userEvent.click(filterArg); + await waitFor(() => { + expect(filterArg).toHaveFocus(); + }); + + await userEvent.click(columnArg); + await waitFor(() => { + expect(columnArg).toHaveFocus(); + }); + + await userEvent.click(filterArg); + await waitFor(() => { + expect(filterArg).toHaveFocus(); + }); + }); + describe('ArithmeticTokenLiteral', () => { it.each(['1', '1.', '1.0', '+1', '+1.', '+1.0', '-1', '-1.', '-1.0'])( 'renders literal %s', @@ -1085,6 +1763,48 @@ describe('token', () => { }); describe('ArithmeticTokenOperator', () => { + it('keeps mid-expression empty free text clickable after deleting an operator', async () => { + render(); + + expect(screen.getByRole('gridcell', {name: 'Delete +'})).toBeInTheDocument(); + await userEvent.click(screen.getByRole('gridcell', {name: 'Delete +'})); + await waitFor(() => { + expect( + screen.queryByRole('gridcell', {name: 'Delete +'}) + ).not.toBeInTheDocument(); + }); + + // Free text: leading spacer, gap between functions, trailing field. + // Leading/mid empty spacers can be zero-width in jsdom's layout stub, so + // include hidden nodes when collecting them. + const freeTextInputs = screen.getAllByRole('combobox', { + name: 'Add a term', + hidden: true, + }); + expect(freeTextInputs).toHaveLength(3); + + const middleFreeTextRow = freeTextInputs[1]!.closest('[role="row"]'); + if (!(middleFreeTextRow instanceof HTMLElement)) { + throw new Error('Expected mid-expression free text row'); + } + const middleRules = getEmotionRules(middleFreeTextRow).join(' '); + // Collapsed mid gaps must keep a hit target (leading stays 0 to avoid wrap). + // getComputedStyle is stubbed in tests, so assert the emotion rules instead. + expect(middleRules).toMatch(/:not\(:first-child\):not\(:last-child\)/); + expect(middleRules).toMatch(/min-width:\s*9px/); + + // Click the mid gap (regression: a zero-width spacer made this impossible), + // then restore the operator through that free-text input. + await userEvent.click(freeTextInputs[1]!); + expect(freeTextInputs[1]).toHaveFocus(); + await userEvent.type(freeTextInputs[1]!, '+', {skipClick: true}); + await userEvent.keyboard('{Escape}'); + + expect( + await screen.findByRole('gridcell', {name: 'Delete +', hidden: true}) + ).toBeInTheDocument(); + }); + it('renders addition operator', async () => { const dispatch = jest.fn(); render(); diff --git a/static/app/components/arithmeticBuilder/token/styles.tsx b/static/app/components/arithmeticBuilder/token/styles.tsx index 0de75c4ddffb..a0434912cebd 100644 --- a/static/app/components/arithmeticBuilder/token/styles.tsx +++ b/static/app/components/arithmeticBuilder/token/styles.tsx @@ -1,7 +1,9 @@ import {css} from '@emotion/react'; import styled from '@emotion/styled'; -export const Row = styled('div')<{withBorder?: boolean}>` +export const Row = styled('div', { + shouldForwardProp: prop => prop !== 'collapsed' && prop !== 'withBorder', +})<{collapsed?: boolean; withBorder?: boolean}>` position: relative; display: flex; align-items: stretch; @@ -15,8 +17,33 @@ export const Row = styled('div')<{withBorder?: boolean}>` border-radius: ${p.theme.radius.md}; `} + /* Empty spacers must not consume a flex line. A wide _if function is + max-width 100%, so even a few pixels of leading free text wrap onto + their own row. Last-child still grows to fill leftover space. + Mid-expression empty gaps keep a small hit target so deleting an + operator does not leave an unclickable seam between tokens. */ + ${p => + p.collapsed && + css` + width: 0; + min-width: 0; + flex-grow: 0; + flex-shrink: 0; + flex-basis: 0; + overflow: visible; + + &:not(:first-child):not(:last-child) { + width: 9px; + min-width: 9px; + flex-basis: 9px; + } + `} + &:last-child { flex-grow: 1; + min-width: 0; + max-width: none; + flex-basis: 0; } &[aria-invalid='true'] { diff --git a/static/app/components/arithmeticBuilder/token/useFunctionArgumentInput.tsx b/static/app/components/arithmeticBuilder/token/useFunctionArgumentInput.tsx new file mode 100644 index 000000000000..b751e9c18f28 --- /dev/null +++ b/static/app/components/arithmeticBuilder/token/useFunctionArgumentInput.tsx @@ -0,0 +1,297 @@ +import type {FocusEvent, RefObject} from 'react'; +import {useCallback, useRef} from 'react'; +import styled from '@emotion/styled'; +import type {ListState} from '@react-stately/list'; +import type {KeyboardEvent, Node} from '@react-types/shared'; + +import {Flex, type FlexProps} from '@sentry/scraps/layout'; + +import {useArithmeticBuilder} from 'sentry/components/arithmeticBuilder/context'; +import { + TokenKind, + type Token, + type TokenAttribute, + type TokenFunction, +} from 'sentry/components/arithmeticBuilder/token'; +import {nextTokenKeyOfKind} from 'sentry/components/arithmeticBuilder/tokenizer'; +import {useGridListItem} from 'sentry/components/tokenizedInput/grid/useGridListItem'; +import {focusTarget} from 'sentry/components/tokenizedInput/grid/utils'; +import {UnstyledInput} from 'sentry/components/tokenizedInput/token/unstyledInput'; +import {defined} from 'sentry/utils/defined'; + +type FunctionArgumentValue = {label: string; value: string}; + +export interface FunctionArgumentInputProps { + argument: FunctionArgumentValue; + argumentIndex: number; + argumentItem: Node; + argumentRef: RefObject; + arguments: FunctionArgumentValue[]; + argumentsListState: ListState; + functionItem: Node; + functionListState: ListState; + functionToken: TokenFunction; + onArgumentsBlur: () => void; + onArgumentsChange: (index: number, argument: string) => void; + rowRef: RefObject; +} + +export function useFunctionArgumentInput({ + argumentIndex, + argumentItem, + argumentRef, + arguments: functionArguments, + argumentsListState, + functionItem, + functionListState, + functionToken, + onArgumentsBlur, +}: FunctionArgumentInputProps) { + const inputRef = useRef(null); + const gridCellRef = useRef(null); + const skipBlurFlushRef = useRef(false); + const {rowProps, gridCellProps} = useGridListItem({ + item: argumentItem, + ref: gridCellRef, + state: argumentsListState, + focusable: true, + }); + + const isFocused = argumentItem.key === argumentsListState.selectionManager.focusedKey; + const hasNextArgument = argumentIndex < functionToken.attributes.length - 1; + const hasPrevArgument = argumentIndex > 0; + const {dispatch} = useArithmeticBuilder(); + + const shouldCloseOnInteractOutside = useCallback((el: Element) => { + return !gridCellRef.current?.contains(el); + }, []); + + const updateAttrsWith = useCallback( + (value: string) => { + const tokenArguments = functionArguments.map(arg => arg.value); + tokenArguments[argumentIndex] = value; + return tokenArguments.join(','); + }, + [argumentIndex, functionArguments] + ); + + const clearSkipBlurFlush = useCallback(() => { + skipBlurFlushRef.current = false; + }, []); + + const flushArgumentsIfLeavingGrid = useCallback( + (evt?: FocusEvent) => { + // Stay skipped until the next focus: a later interact-outside `onInputBlur()` + // timeout would otherwise restore the token after DELETE/REPLACE. + if (skipBlurFlushRef.current) { + return; + } + + if (!evt) { + window.setTimeout(() => { + if (skipBlurFlushRef.current) { + return; + } + const stayingInArgs = Boolean( + document.activeElement && + argumentRef.current?.contains(document.activeElement) + ); + if (!stayingInArgs) { + onArgumentsBlur(); + } + }, 0); + return; + } + + const argsGrid = evt.currentTarget.closest('[role="grid"]'); + const related = evt.relatedTarget; + const stayingInArgs = Boolean( + argsGrid && related instanceof globalThis.Node && argsGrid.contains(related) + ); + if (!stayingInArgs) { + onArgumentsBlur(); + } + }, + [argumentRef, onArgumentsBlur] + ); + + const onKeyDownCapture = useCallback( + (evt: React.KeyboardEvent) => { + // At start and pressing left arrow, focus the previous full token + if ( + evt.currentTarget.selectionStart === 0 && + evt.currentTarget.selectionEnd === 0 && + evt.key === 'ArrowLeft' + ) { + if (hasPrevArgument) { + focusTarget( + argumentsListState, + argumentsListState.collection.getKeyBefore(argumentItem.key) + ); + } else { + focusTarget( + functionListState, + functionListState.collection.getKeyBefore(functionItem.key) + ); + } + return; + } + + // At end and pressing right arrow, focus the next full token + if ( + evt.currentTarget.selectionStart === evt.currentTarget.value.length && + evt.currentTarget.selectionEnd === evt.currentTarget.value.length && + evt.key === 'ArrowRight' + ) { + if (hasNextArgument) { + focusTarget( + argumentsListState, + argumentsListState.collection.getKeyAfter(argumentItem.key) + ); + } else { + focusTarget( + functionListState, + functionListState.collection.getKeyAfter(functionItem.key) + ); + } + } + }, + [ + hasPrevArgument, + argumentsListState, + argumentItem.key, + functionListState, + functionItem.key, + hasNextArgument, + ] + ); + + const onKeyDown = useCallback( + (evt: KeyboardEvent) => { + const selectionStart = evt.currentTarget.selectionStart ?? 0; + const selectionEnd = evt.currentTarget.selectionEnd ?? 0; + const valueLength = evt.currentTarget.value.length; + const isCollapsedAtStart = selectionStart === 0 && selectionEnd === 0; + const isCollapsedAtEnd = + selectionStart === valueLength && selectionEnd === valueLength; + + // Collapsed caret at start + Backspace deletes the function. A full + // selection must not — filter/value args keep their text on focus, so + // select-all then Backspace should edit the argument. + if (evt.key === 'Backspace' && isCollapsedAtStart) { + evt.preventDefault(); + skipBlurFlushRef.current = true; + const itemKey = functionListState.collection.getKeyBefore(functionItem.key); + dispatch({ + type: 'DELETE_TOKEN', + token: functionToken, + focusOverride: defined(itemKey) ? {itemKey} : undefined, + }); + return; + } + + // Collapsed caret at end + Delete deletes the function. + if (evt.key === 'Delete' && isCollapsedAtEnd) { + evt.preventDefault(); + skipBlurFlushRef.current = true; + const itemKey = functionListState.collection.getKeyBefore(functionItem.key); + dispatch({ + type: 'DELETE_TOKEN', + token: functionToken, + focusOverride: defined(itemKey) ? {itemKey} : undefined, + }); + } + }, + [dispatch, functionToken, functionListState, functionItem] + ); + + const dataTestId = + functionListState.collection.getLastKey() === functionItem.key + ? 'arithmetic-builder-argument-input' + : undefined; + + const focusArgument = useCallback(() => { + focusTarget(argumentsListState, argumentItem.key); + }, [argumentItem.key, argumentsListState]); + + const focusNextArgument = useCallback(() => { + focusTarget( + argumentsListState, + argumentsListState.collection.getKeyAfter(argumentItem.key) + ); + }, [argumentItem.key, argumentsListState]); + + const commitFunctionToken = useCallback( + (value: string) => { + skipBlurFlushRef.current = true; + dispatch({ + text: `${functionToken.function}(${updateAttrsWith(value)})`, + type: 'REPLACE_TOKEN', + token: functionToken, + focusOverride: { + itemKey: nextTokenKeyOfKind( + functionListState, + functionToken, + TokenKind.FREE_TEXT + ), + }, + }); + }, + [dispatch, functionListState, functionToken, updateAttrsWith] + ); + + return { + clearSkipBlurFlush, + commitFunctionToken, + dataTestId, + flushArgumentsIfLeavingGrid, + focusArgument, + focusNextArgument, + gridCellProps, + gridCellRef, + hasNextArgument, + inputRef, + isFocused, + onKeyDown, + onKeyDownCapture, + rowProps, + shouldCloseOnInteractOutside, + }; +} + +export function ArgumentGridRow(props: FlexProps) { + return ( + + ); +} + +const StyledArgumentGridCell = styled(Flex)` + min-width: 0; + max-width: 100%; + + ${UnstyledInput} { + max-width: 130px; + min-width: 0; + white-space: nowrap; + overflow: hidden; + text-overflow: ellipsis; + } + + /* Expanded bars: grow into the remaining args column (beside the function + name), not the full chip width. */ + [data-expanded='true'] & ${UnstyledInput} { + max-width: 100%; + } +`; + +export function ArgumentGridCell(props: FlexProps) { + return ; +} diff --git a/static/app/components/searchQueryBuilder/tokens/useSearchTokenCombobox.tsx b/static/app/components/searchQueryBuilder/tokens/useSearchTokenCombobox.tsx index 7be3d2474c59..81870c280e11 100644 --- a/static/app/components/searchQueryBuilder/tokens/useSearchTokenCombobox.tsx +++ b/static/app/components/searchQueryBuilder/tokens/useSearchTokenCombobox.tsx @@ -238,7 +238,10 @@ export function useSearchTokenCombobox( }), listBoxProps: mergeProps(menuProps, listBoxProps, { onAction: undefined, - autoFocus: state.focusStrategy || true, + // Only virtual-focus an option after ArrowUp/ArrowDown sets focusStrategy. + // Autofocusing the first item on open makes Enter select it instead of + // committing the typed value (and blocks expandable equation dismiss). + autoFocus: state.focusStrategy ?? false, shouldUseVirtualFocus: true, shouldSelectOnPressUp: true, shouldFocusOnHover: true, diff --git a/static/app/components/tokenizedInput/token/comboBox.spec.tsx b/static/app/components/tokenizedInput/token/comboBox.spec.tsx index e8040bfeb7a9..1a7dafb154a0 100644 --- a/static/app/components/tokenizedInput/token/comboBox.spec.tsx +++ b/static/app/components/tokenizedInput/token/comboBox.spec.tsx @@ -1,7 +1,7 @@ import type {ComponentProps} from 'react'; import {Item} from '@react-stately/collections'; -import {render, screen, userEvent} from 'sentry-test/reactTestingLibrary'; +import {render, screen, userEvent, waitFor} from 'sentry-test/reactTestingLibrary'; import {ComboBox} from 'sentry/components/tokenizedInput/token/comboBox'; @@ -65,6 +65,8 @@ describe('ComboBox', () => { await userEvent.click(screen.getByRole('combobox')); expect(onOpenChange).not.toHaveBeenCalledWith(true); + expect(screen.queryByRole('option')).not.toBeInTheDocument(); + expect(screen.queryByText('No options found')).not.toBeInTheDocument(); }); it('does not open the menu when every option is filtered out', async () => { @@ -87,4 +89,48 @@ describe('ComboBox', () => { expect(onOpenChange).not.toHaveBeenCalledWith(true); }); + + it('closes the menu after selecting when keepMenuOpenOnSelect is configured', async () => { + render( + ({ + key: item, + label: item, + value: item, + }))} + keepMenuOpenOnSelect={() => false} + /> + ); + + await userEvent.click(screen.getByRole('combobox')); + await userEvent.click(screen.getByRole('option', {name: 'foo'})); + + await waitFor(() => { + expect(screen.queryByRole('option')).not.toBeInTheDocument(); + }); + }); + + it('keeps the menu open when keepMenuOpenOnSelect returns true', async () => { + render( + ({ + key: item, + label: item, + value: item, + }))} + keepMenuOpenOnSelect={() => true} + /> + ); + + await userEvent.click(screen.getByRole('combobox')); + await userEvent.click(screen.getByRole('option', {name: 'foo'})); + + expect(screen.getByRole('option', {name: 'bar'})).toBeInTheDocument(); + }); }); diff --git a/static/app/components/tokenizedInput/token/comboBox.tsx b/static/app/components/tokenizedInput/token/comboBox.tsx index 71fc7b3a8dd5..e379b889116b 100644 --- a/static/app/components/tokenizedInput/token/comboBox.tsx +++ b/static/app/components/tokenizedInput/token/comboBox.tsx @@ -1,7 +1,9 @@ import type { ChangeEventHandler, ClipboardEvent, + FocusEvent, FocusEventHandler, + KeyboardEventHandler, MouseEventHandler, Ref, } from 'react'; @@ -22,11 +24,12 @@ import { itemIsSectionWithKey, ListBox, } from '@sentry/scraps/compactSelect'; -import {Input, useAutosizeInput} from '@sentry/scraps/input'; +import {useAutosizeInput} from '@sentry/scraps/input'; import {Flex} from '@sentry/scraps/layout'; import {Overlay} from 'sentry/components/overlay'; import {useSearchTokenCombobox} from 'sentry/components/searchQueryBuilder/tokens/useSearchTokenCombobox'; +import {UnstyledInput} from 'sentry/components/tokenizedInput/token/unstyledInput'; import {useOverlay} from 'sentry/utils/useOverlay'; interface ComboBoxProps { @@ -36,12 +39,23 @@ interface ComboBoxProps { inputValue: string; items: Array>; ['data-test-id']?: string; + /** + * Keep the suggestion menu open after selecting an option. Useful when the + * user still needs to pick a follow-up value (e.g. filter key → filter value). + */ + keepMenuOpenOnSelect?: boolean | ((option: SelectOptionWithKey) => boolean); onClick?: MouseEventHandler; - onInputBlur?: () => void; + onInputBlur?: (evt?: FocusEvent) => void; onInputChange?: ChangeEventHandler; onInputCommit?: (value: string) => void; onInputEscape?: () => void; onInputFocus?: FocusEventHandler; + /** + * Native keyup on the input. Callers that need the caret after arrow-key + * movement (e.g. equation filter autocomplete) should use this rather than + * onKeyDown, which fires before the browser updates selection. + */ + onInputKeyUp?: KeyboardEventHandler; onKeyDown?: (evt: KeyboardEvent) => void; onKeyDownCapture?: (evt: React.KeyboardEvent) => void; onOpenChange?: (newOpenState: boolean) => void; @@ -54,6 +68,10 @@ interface ComboBoxProps { * other elements. */ shouldCloseOnInteractOutside?: (interactedElement: Element) => boolean; + /** + * When false, all items from `items` are shown and filtering is left to the caller. + */ + shouldFilterResults?: boolean; tabIndex?: number; } @@ -110,18 +128,64 @@ export function ComboBox({ placeholder, tabIndex, ref, + keepMenuOpenOnSelect, + onInputKeyUp, + shouldFilterResults = true, }: ComboBoxProps) { const inputRef = useRef(null); const listBoxRef = useRef(null); const popoverRef = useRef(null); + const openMenuRef = useRef<(() => void) | null>(null); + const closeMenuRef = useRef<(() => void) | null>(null); + const suppressAutoOpenRef = useRef(false); const {hiddenOptions, disabledKeys} = useHiddenItems({ items, filterValue, maxOptions: 50, - shouldFilterResults: true, + shouldFilterResults, + }); + + const hasVisibleItems = items.some(item => { + if (itemIsSectionWithKey(item)) { + return item.options.some(option => !hiddenOptions.has(option.key)); + } + return !hiddenOptions.has(item.key); }); + const shouldKeepMenuOpenOnSelect = useCallback( + (option: SelectOptionWithKey) => { + if (typeof keepMenuOpenOnSelect === 'function') { + return keepMenuOpenOnSelect(option); + } + return keepMenuOpenOnSelect ?? false; + }, + [keepMenuOpenOnSelect] + ); + + const applyOptionSelection = useCallback( + (option: SelectOptionWithKey) => { + onOptionSelected?.(option); + if (shouldKeepMenuOpenOnSelect(option)) { + openMenuRef.current?.(); + return; + } + if (keepMenuOpenOnSelect === undefined) { + return; + } + // Selecting closes the menu and briefly suppresses auto-open so focus/click + // returning to the input does not immediately reopen it. + suppressAutoOpenRef.current = true; + requestAnimationFrame(() => { + closeMenuRef.current?.(); + requestAnimationFrame(() => { + suppressAutoOpenRef.current = false; + }); + }); + }, + [keepMenuOpenOnSelect, onOptionSelected, shouldKeepMenuOpenOnSelect] + ); + const handleValueChange = useCallback( (key: Key | null) => { if (!key) { @@ -132,18 +196,16 @@ export function ComboBox({ if (itemIsSectionWithKey(item)) { const option = item.options.find(child => child.key === key); if (option) { - onOptionSelected?.(option); - break; - } - } else { - if (item.key === key) { - onOptionSelected?.(item); + applyOptionSelection(option); break; } + } else if (item.key === key) { + applyOptionSelection(item); + break; } } }, - [items, onOptionSelected] + [applyOptionSelection, items] ); const comboBoxProps: Partial>> = @@ -166,10 +228,15 @@ export function ComboBox({ shouldCloseOnBlur: false, ...comboBoxProps, }); + openMenuRef.current = () => state.open(); + closeMenuRef.current = () => state.close(); const handleComboBoxFocus: FocusEventHandler = useCallback( evt => { onInputFocus?.(evt); + if (suppressAutoOpenRef.current) { + return; + } state.open(); }, [onInputFocus, state] @@ -180,19 +247,15 @@ export function ComboBox({ if (evt.relatedTarget && !shouldCloseOnInteractOutside?.(evt.relatedTarget)) { return; } - onInputBlur?.(); + onInputBlur?.(evt); state.close(); }, [onInputBlur, shouldCloseOnInteractOutside, state] ); - const totalOptions = items.reduce( - (acc, item) => acc + (itemIsSectionWithKey(item) ? item.options.length : 1), - 0 - ); - // Showing the overlay with nothing to select renders as an empty grey bar - const isOpen = state.isOpen && totalOptions > hiddenOptions.size; + const isOpen = state.isOpen && hasVisibleItems; + const isMenuVisible = isOpen; const handleComboBoxKeyDown = useCallback( (evt: KeyboardEvent) => { @@ -282,11 +345,22 @@ export function ComboBox({ evt.stopPropagation(); inputProps.onClick?.(evt); onClick?.(evt); + if (suppressAutoOpenRef.current) { + return; + } state.open(); }, [inputProps, state, onClick] ); + const handleInputKeyUp: KeyboardEventHandler = useCallback( + evt => { + inputProps.onKeyUp?.(evt); + onInputKeyUp?.(evt); + }, + [inputProps, onInputKeyUp] + ); + useUpdateOverlayPositionOnContentChange({ contentRef: popoverRef, updateOverlayPosition, @@ -299,6 +373,10 @@ export function ComboBox({ - + ); } - -const UnstyledInput = styled(Input)` - background: transparent; - border: none; - box-shadow: none; - flex-grow: 1; - padding: 0; - height: auto; - min-height: auto; - resize: none; - min-width: 1px; - border-radius: 0; - - &:focus { - outline: none; - border: none; - box-shadow: none; - } -`; diff --git a/static/app/components/tokenizedInput/token/unstyledInput.tsx b/static/app/components/tokenizedInput/token/unstyledInput.tsx new file mode 100644 index 000000000000..9ddbfdd6f6ef --- /dev/null +++ b/static/app/components/tokenizedInput/token/unstyledInput.tsx @@ -0,0 +1,22 @@ +import styled from '@emotion/styled'; + +import {Input} from '@sentry/scraps/input'; + +export const UnstyledInput = styled(Input)` + background: transparent; + border: none; + box-shadow: none; + flex-grow: 1; + padding: 0; + height: auto; + min-height: auto; + resize: none; + min-width: 1px; + border-radius: 0; + + &:focus { + outline: none; + border: none; + box-shadow: none; + } +`; diff --git a/static/app/utils/discover/fields.spec.tsx b/static/app/utils/discover/fields.spec.tsx index c6a7fb25eb62..ba774ad44b34 100644 --- a/static/app/utils/discover/fields.spec.tsx +++ b/static/app/utils/discover/fields.spec.tsx @@ -15,6 +15,7 @@ import { isMeasurement, measurementType, parseFunction, + prettifyParsedFunction, } from 'sentry/utils/discover/fields'; describe('parseFunction', () => { @@ -115,6 +116,11 @@ describe('parseFunction', () => { arguments: ['`span.description:"GET /foo, /bar"`', 'span.duration'], filter: 'span.description:"GET /foo, /bar"', }); + expect(parseFunction('avg_if(`tags[Limit,number]:>5`,span.duration)')).toEqual({ + name: 'avg_if', + arguments: ['`tags[Limit,number]:>5`', 'span.duration'], + filter: 'tags[Limit,number]:>5', + }); }); it('handles backtick wrapped search filters as the only argument', () => { @@ -482,3 +488,34 @@ describe('fieldAlignment()', () => { expect(fieldAlignment('title', undefined, meta)).toBe('left'); }); }); + +describe('prettifyParsedFunction', () => { + it('prettifies typed tag arguments', () => { + expect( + prettifyParsedFunction({ + name: 'avg', + arguments: ['tags[Limit,number]'], + }) + ).toBe('avg(Limit)'); + }); + + it('prettifies typed tag keys inside conditional filter arguments', () => { + expect( + prettifyParsedFunction({ + name: 'avg_if', + arguments: ['`tags[Limit,number]:>5`', 'span.duration'], + filter: 'tags[Limit,number]:>5', + }) + ).toBe('avg_if(`Limit:>5`,span.duration)'); + }); + + it('does not collapse a filter argument to only the typed tag name', () => { + expect( + prettifyParsedFunction({ + name: 'avg_if', + arguments: ['`tags[Limit,number]:>5`', 'tags[Limit,number]'], + filter: 'tags[Limit,number]:>5', + }) + ).toBe('avg_if(`Limit:>5`,Limit)'); + }); +}); diff --git a/static/app/utils/discover/fields.tsx b/static/app/utils/discover/fields.tsx index 29ab546e9b46..17975c8e9ee1 100644 --- a/static/app/utils/discover/fields.tsx +++ b/static/app/utils/discover/fields.tsx @@ -1812,6 +1812,22 @@ export function prettifyParsedFunction(func: ParsedFunction) { return `${func.name}(${prettifyTagKey(metricName ?? '')})`; } - const args = func.arguments.map(prettifyTagKey); + const args = func.arguments.map(prettifyFunctionArgument); return `${func.name}(${args.join(',')})`; } + +/** + * Prettify a function argument for display. Search-filter args (backtick-wrapped) + * must not go through {@link prettifyTagKey} directly — that regex is unanchored and + * would collapse `` `tags[Limit,number]:>5` `` to `Limit`. Instead, rewrite typed tag + * keys inside the filter while keeping the rest of the query intact. + */ +function prettifyFunctionArgument(arg: string): string { + if (isSearchFilterArgument(arg)) { + const filter = arg + .slice(1, -1) + .replace(/tags\[(\S*),(\S*)\]/g, match => prettifyTagKey(match)); + return `\`${filter}\``; + } + return prettifyTagKey(arg); +} diff --git a/static/app/utils/fields/exploreEquationAggregates.spec.tsx b/static/app/utils/fields/exploreEquationAggregates.spec.tsx new file mode 100644 index 000000000000..b68f18281273 --- /dev/null +++ b/static/app/utils/fields/exploreEquationAggregates.spec.tsx @@ -0,0 +1,122 @@ +import { + ALLOWED_EXPLORE_EQUATION_AGGREGATES, + ALLOWED_EXPLORE_EQUATION_CONDITIONAL_AGGREGATES, + AggregationKey, + EXPLORE_FILTERABLE_AGGREGATES, + getExploreEquationAggregates, + getExploreEquationFieldDefinition, + getFieldDefinition, +} from 'sentry/utils/fields'; + +describe('Explore equation conditional aggregates', () => { + it('keeps Discover avg_if and count_if on the ungated equation list', () => { + expect(ALLOWED_EXPLORE_EQUATION_AGGREGATES).toContain(AggregationKey.AVG_IF); + expect(ALLOWED_EXPLORE_EQUATION_AGGREGATES).toContain(AggregationKey.COUNT_IF); + expect(getExploreEquationAggregates(false)).toEqual( + ALLOWED_EXPLORE_EQUATION_AGGREGATES + ); + }); + + it('offers EAP _if aggregates only when the feature is on', () => { + expect(ALLOWED_EXPLORE_EQUATION_CONDITIONAL_AGGREGATES).toEqual( + EXPLORE_FILTERABLE_AGGREGATES.map(name => `${name}_if`) + ); + expect(getExploreEquationAggregates(true)).toEqual( + expect.arrayContaining(ALLOWED_EXPLORE_EQUATION_CONDITIONAL_AGGREGATES) + ); + for (const name of ALLOWED_EXPLORE_EQUATION_CONDITIONAL_AGGREGATES) { + if (name === 'avg_if' || name === 'count_if') { + continue; + } + expect(getExploreEquationAggregates(false)).not.toContain(name); + } + }); + + it('uses EAP filter-first avg_if when gated on', () => { + const ungated = getFieldDefinition('avg_if', 'span'); + expect(ungated?.parameters?.map(parameter => parameter.name)).toEqual([ + 'column', + 'condition_column', + 'condition', + 'value', + ]); + + const definition = getExploreEquationFieldDefinition('avg_if', undefined, true); + expect(definition?.parameters?.map(parameter => parameter.name)).toEqual([ + 'filter', + 'column', + ]); + expect(definition?.parameters?.[0]).toMatchObject({ + kind: 'value', + defaultValue: '``', + }); + expect(definition?.parameters?.[1]).toMatchObject({ + kind: 'column', + defaultValue: 'span.duration', + }); + }); + + it('keeps Discover avg_if when existing args are not backtick filters', () => { + const definition = getExploreEquationFieldDefinition('avg_if', undefined, true, [ + 'span.duration', + 'span.op', + 'equals', + 'db', + ]); + expect(definition?.parameters?.map(parameter => parameter.name)).toEqual([ + 'column', + 'condition_column', + 'condition', + 'value', + ]); + }); + + it('keeps EAP filter-first params for EAP-only _if without backticks', () => { + expect(getFieldDefinition('sum_if', 'span')).toBeNull(); + + const definition = getExploreEquationFieldDefinition('sum_if', undefined, true, [ + 'span.duration', + ]); + expect(definition?.parameters?.map(parameter => parameter.name)).toEqual([ + 'filter', + 'column', + ]); + }); + + it('keeps Discover count_if unless the feature is on', () => { + const ungated = getFieldDefinition('count_if', 'span'); + expect(ungated?.parameters?.map(parameter => parameter.name)).toEqual([ + 'column', + 'value', + 'value', + ]); + expect(ungated?.parameters?.some(parameter => 'options' in parameter)).toBe(true); + + const gated = getExploreEquationFieldDefinition('count_if', undefined, true); + expect(gated?.parameters?.map(parameter => parameter.name)).toEqual([ + 'filter', + 'column', + ]); + expect(gated?.parameters?.[0]).toMatchObject({ + kind: 'value', + defaultValue: '``', + }); + expect(gated?.parameters?.some(parameter => 'options' in parameter)).toBe(false); + }); + + it('does not use Discover-style condition operators on gated equation _if aggregates', () => { + for (const name of ALLOWED_EXPLORE_EQUATION_CONDITIONAL_AGGREGATES) { + const definition = getExploreEquationFieldDefinition(name, undefined, true); + expect(definition?.parameters?.[0]).toMatchObject({ + name: 'filter', + kind: 'value', + defaultValue: '``', + }); + expect( + definition?.parameters?.some( + parameter => 'options' in parameter && Boolean(parameter.options?.length) + ) + ).toBe(false); + } + }); +}); diff --git a/static/app/utils/fields/index.ts b/static/app/utils/fields/index.ts index e69628b3a9c4..4fb8b7e71d4a 100644 --- a/static/app/utils/fields/index.ts +++ b/static/app/utils/fields/index.ts @@ -1153,6 +1153,35 @@ export const ALLOWED_EXPLORE_VISUALIZE_AGGREGATES: AggregationKey[] = [ AggregationKey.OPPORTUNITY_SCORE, ]; +/** + * Span aggregates that EAP generates an `_if` combinator for. Used by Explore series + * filters and equation builders. See `SPAN_AGGREGATE_COMBINATORS` in + * `src/sentry/search/eap/spans/aggregates.py`. + */ +export const EXPLORE_FILTERABLE_AGGREGATES: AggregationKey[] = [ + AggregationKey.COUNT, + AggregationKey.COUNT_UNIQUE, + AggregationKey.SUM, + AggregationKey.AVG, + AggregationKey.MIN, + AggregationKey.MAX, + AggregationKey.P50, + AggregationKey.P75, + AggregationKey.P90, + AggregationKey.P95, + AggregationKey.P99, + AggregationKey.P100, +]; + +/** + * EAP conditional aggregates offered in the Explore equation builder + * (`avg_if(\`span.op:db\`,span.duration)`). The first argument is a backtick-wrapped + * search filter, followed by the base aggregate's parameters. Only included when + * `explore-conditional-aggregates` is enabled; see {@link getExploreEquationAggregates}. + */ +export const ALLOWED_EXPLORE_EQUATION_CONDITIONAL_AGGREGATES: string[] = + EXPLORE_FILTERABLE_AGGREGATES.map(name => `${name}_if`); + export const ALLOWED_EXPLORE_EQUATION_AGGREGATES: AggregationKey[] = [ ...ALLOWED_EXPLORE_VISUALIZE_AGGREGATES, AggregationKey.AVG_IF, @@ -1161,6 +1190,25 @@ export const ALLOWED_EXPLORE_EQUATION_AGGREGATES: AggregationKey[] = [ AggregationKey.USER_MISERY, ]; +/** + * Aggregates offered in the Explore equation builder. When + * `explore-conditional-aggregates` is on, Discover `avg_if` / `count_if` are replaced by + * the EAP `_if` combinators (`avg_if`, `count_if`, `sum_if`, …). + */ +export function getExploreEquationAggregates( + hasConditionalAggregates: boolean +): string[] { + if (!hasConditionalAggregates) { + return ALLOWED_EXPLORE_EQUATION_AGGREGATES; + } + return [ + ...ALLOWED_EXPLORE_VISUALIZE_AGGREGATES, + ...ALLOWED_EXPLORE_EQUATION_CONDITIONAL_AGGREGATES, + AggregationKey.APDEX, + AggregationKey.USER_MISERY, + ]; +} + const LOG_AGGREGATION_FIELDS: Record = { ...AGGREGATION_FIELDS, [AggregationKey.COUNT]: { @@ -1699,6 +1747,35 @@ export const NO_ARGUMENT_SPAN_AGGREGATES: AggregationKey[] = Object.entries( .filter(([_, field]) => field.parameters?.length === 0) .map(([key]) => key as AggregationKey); +/** + * Prepend the EAP `_if` search filter argument to a base span aggregate definition. + * Empty backticks are the default so the tokenizer keeps a filter slot until the user + * fills it in: `avg_if(``,span.duration)`. + */ +function withConditionalFilterParameter(definition: FieldDefinition): FieldDefinition { + return { + ...definition, + parameters: [ + { + name: 'filter', + kind: 'value', + dataType: FieldValueType.STRING, + defaultValue: '``', + required: true, + }, + ...(definition.parameters ?? []), + ], + }; +} + +const SPAN_CONDITIONAL_AGGREGATION_FIELDS: Record = + Object.fromEntries( + EXPLORE_FILTERABLE_AGGREGATES.map(name => [ + `${name}_if`, + withConditionalFilterParameter(SPAN_AGGREGATION_FIELDS[name]), + ]) + ); + export const MEASUREMENT_FIELDS: Record = { [WebVital.FP]: { desc: t('Web Vital First Paint'), @@ -3852,6 +3929,50 @@ export const getFieldDefinition = ( return _getFieldFromMappings(type, key, kind) ?? null; }; +/** + * Span field definitions for the Explore equation builder. When + * `explore-conditional-aggregates` is on, `_if` combinators use the EAP filter-first + * signature (`avg_if(\`span.op:db\`,span.duration)`), including `count_if`. + * + * Existing Discover-style calls (`avg_if(span.duration,span.op,equals,db)`) keep the + * Discover definition so editing them does not reinterpret the first column as a filter. + * EAP-only `_if`s without a Discover definition keep the filter-first signature. + */ +export function getExploreEquationFieldDefinition( + key: string, + kind?: FieldKind, + hasConditionalAggregates = false, + attributeTexts?: readonly string[] +): FieldDefinition | null { + if (hasConditionalAggregates) { + const conditionalDefinition = SPAN_CONDITIONAL_AGGREGATION_FIELDS[key]; + if (conditionalDefinition) { + if (usesDiscoverStyleConditionalAggregateArgs(attributeTexts)) { + // Only Discover-defined `_if`s (`avg_if`/`count_if`) should stay on the Discover + // arity. EAP-only combinators (`sum_if`, …) have no Discover definition — keep + // the filter-first signature even when the first arg is not backtick-wrapped yet. + return getFieldDefinition(key, 'span', kind) ?? conditionalDefinition; + } + return conditionalDefinition; + } + } + return getFieldDefinition(key, 'span', kind); +} + +/** + * Discover `_if` aggregates put a column first. EAP filter-first forms wrap the first + * argument in backticks (`\`span.op:db\`` or empty `` ` ` ``). + */ +function usesDiscoverStyleConditionalAggregateArgs( + attributeTexts: readonly string[] | undefined +): boolean { + if (!attributeTexts?.length) { + return false; + } + const first = attributeTexts[0]!.trim(); + return !(first.startsWith('`') && first.endsWith('`')); +} + export function isDeviceClass(key: any): boolean { return key === FieldKey.DEVICE_CLASS; } diff --git a/static/app/views/dashboards/widgetBuilder/components/exploreArithmeticBuilder.tsx b/static/app/views/dashboards/widgetBuilder/components/exploreArithmeticBuilder.tsx index 682451399b2c..4555b3ab51a6 100644 --- a/static/app/views/dashboards/widgetBuilder/components/exploreArithmeticBuilder.tsx +++ b/static/app/views/dashboards/widgetBuilder/components/exploreArithmeticBuilder.tsx @@ -1,16 +1,9 @@ -import {useCallback, useMemo} from 'react'; +import {useCallback} from 'react'; -import {ArithmeticBuilder} from 'sentry/components/arithmeticBuilder'; import type {Expression} from 'sentry/components/arithmeticBuilder/expression'; -import type {FunctionArgument} from 'sentry/components/arithmeticBuilder/types'; import {stripEquationPrefix} from 'sentry/utils/discover/fields'; -import { - ALLOWED_EXPLORE_EQUATION_AGGREGATES, - FieldKind, - getFieldDefinition, -} from 'sentry/utils/fields'; import {useWidgetBuilderTraceItemConfig} from 'sentry/views/dashboards/widgetBuilder/hooks/useWidgetBuilderTraceItemConfig'; -import {useExploreSuggestedAttribute} from 'sentry/views/explore/hooks/useExploreSuggestedAttribute'; +import {ExploreEquationArithmeticBuilder} from 'sentry/views/explore/components/exploreEquationArithmeticBuilder'; import {useTraceItemDatasetAttributes} from 'sentry/views/explore/hooks/useTraceItemAttributes'; type Props = { @@ -37,33 +30,6 @@ export function ExploreArithmeticBuilder({equation, onUpdate}: Props) { 'boolean' ); - const functionArguments: FunctionArgument[] = useMemo(() => { - return [ - ...Object.entries(numberTags).map(([key, tag]) => { - return { - kind: FieldKind.MEASUREMENT, - name: key, - label: tag.name, - }; - }), - ...Object.entries(stringTags).map(([key, tag]) => { - return { - kind: FieldKind.TAG, - name: key, - label: tag.name, - }; - }), - ]; - }, [numberTags, stringTags]); - - const getSpanFieldDefinition = useCallback( - (key: string) => { - const tag = numberTags[key] ?? stringTags[key]; - return getFieldDefinition(key, 'span', tag?.kind); - }, - [numberTags, stringTags] - ); - const handleExpressionChange = useCallback( (newExpression: Expression) => { onUpdate(stripEquationPrefix(newExpression.text)); @@ -71,20 +37,14 @@ export function ExploreArithmeticBuilder({equation, onUpdate}: Props) { [onUpdate] ); - const getSuggestedAttribute = useExploreSuggestedAttribute({ - numberAttributes: numberTags, - stringAttributes: stringTags, - booleanAttributes: booleanTags, - }); - return ( - ); } diff --git a/static/app/views/explore/components/exploreEquationArithmeticBuilder.tsx b/static/app/views/explore/components/exploreEquationArithmeticBuilder.tsx new file mode 100644 index 000000000000..32f305545a2c --- /dev/null +++ b/static/app/views/explore/components/exploreEquationArithmeticBuilder.tsx @@ -0,0 +1,57 @@ +import {ArithmeticBuilder} from 'sentry/components/arithmeticBuilder'; +import type {Expression} from 'sentry/components/arithmeticBuilder/expression'; +import type {TagCollection} from 'sentry/types/group'; +import {useExploreEquationBuilderConfig} from 'sentry/views/explore/hooks/useExploreEquationBuilderConfig'; +import type {TraceItemDataset} from 'sentry/views/explore/types'; + +interface ExploreEquationArithmeticBuilderProps { + booleanTags: TagCollection; + expression: string; + numberTags: TagCollection; + setExpression: (expression: Expression) => void; + stringTags: TagCollection; + traceItemType: TraceItemDataset; + ['data-test-id']?: string; +} + +/** + * Shared ArithmeticBuilder wiring for Explore equation editors (toolbar, column + * editor, dashboards widget builder). + */ +export function ExploreEquationArithmeticBuilder({ + expression, + setExpression, + traceItemType, + numberTags, + stringTags, + booleanTags, + 'data-test-id': dataTestId, +}: ExploreEquationArithmeticBuilderProps) { + const { + aggregations, + functionArguments, + getFieldDefinition, + getFilterTagValues, + getSuggestedKey, + hasConditionalAggregates, + } = useExploreEquationBuilderConfig({ + traceItemType, + numberTags, + stringTags, + booleanTags, + }); + + return ( + + ); +} diff --git a/static/app/views/explore/components/toolbar/toolbarVisualize/expandableFilterSearchBar.spec.tsx b/static/app/views/explore/components/toolbar/toolbarVisualize/expandableFilterSearchBar.spec.tsx index 75dccdc48967..50e1a19b789b 100644 --- a/static/app/views/explore/components/toolbar/toolbarVisualize/expandableFilterSearchBar.spec.tsx +++ b/static/app/views/explore/components/toolbar/toolbarVisualize/expandableFilterSearchBar.spec.tsx @@ -32,6 +32,24 @@ function SearchBarStub({ ); } +function EquationBuilderStub({ + children, + ...inputProps +}: { + children?: React.ReactNode; +} & React.ComponentProps<'input'>) { + return ( + +
+
+ +
+ {children} +
+
+ ); +} + function isExpanded(input: HTMLElement) { return Boolean(input.closest('[data-expanded="true"]')); } @@ -48,6 +66,20 @@ describe('ExpandableFilterSearchBar', () => { expect(input).toHaveProperty('selectionStart', 'span.op:db'.length); }); + it('expands the equation builder and puts the caret at the end of the query', async () => { + render(); + + const input = screen.getByTestId('arithmetic-builder-input'); + await userEvent.click(input); + + expect(isExpanded(input)).toBe(true); + expect(input).toHaveFocus(); + expect(input).toHaveProperty( + 'selectionStart', + 'avg_if(span.duration,span.op,db)'.length + ); + }); + it('collapses on Enter when no suggestion menu is open', async () => { render(); @@ -58,13 +90,73 @@ describe('ExpandableFilterSearchBar', () => { expect(isExpanded(input)).toBe(true); await userEvent.keyboard('{Enter}'); + await flushAnimationFrames(); + await waitFor(() => { + expect(isExpanded(input)).toBe(false); + }); + }); + + it('lets the focused input handle Enter before collapsing', async () => { + const onKeyDown = jest.fn(); + render(); + + const input = screen.getByTestId('query-builder-input'); + await userEvent.click(input); + await flushAnimationFrames(); + + await userEvent.keyboard('{Enter}'); + expect(onKeyDown).toHaveBeenCalledWith( + expect.objectContaining({key: 'Enter', defaultPrevented: false}) + ); + await flushAnimationFrames(); + await waitFor(() => { + expect(isExpanded(input)).toBe(false); + }); + }); + + it('collapses the equation builder on Enter when no suggestion is highlighted', async () => { + render( + +
    +
  • avg
  • +
+
+ ); + + const input = screen.getByTestId('arithmetic-builder-input'); + await userEvent.click(input); + await flushAnimationFrames(); + expect(isExpanded(input)).toBe(true); + + // Suggestions may be open without a highlight — Enter dismisses after commit. + await userEvent.keyboard('{Enter}'); + await flushAnimationFrames(); await waitFor(() => { expect(isExpanded(input)).toBe(false); }); }); - it('stays expanded on Enter while a suggestion menu is open', async () => { - render(); + it('stays expanded on Enter while a suggestion is highlighted', async () => { + render( + +
    +
  • + span.op +
  • +
+
+ ); const input = screen.getByTestId('query-builder-input'); await userEvent.click(input); @@ -76,7 +168,18 @@ describe('ExpandableFilterSearchBar', () => { }); it('stays expanded after blur while a suggestion menu is open', async () => { - render(); + render( + +
    +
  • span.op
  • +
+
+ ); const input = screen.getByTestId('query-builder-input'); await userEvent.click(input); @@ -87,7 +190,6 @@ describe('ExpandableFilterSearchBar', () => { await flushAnimationFrames(); expect(isExpanded(input)).toBe(true); }); - it('collapses after blur when no suggestion menu is open', async () => { render(