From 3d8b5f37364c3dd5b69b4705be7d315f02a44ed1 Mon Sep 17 00:00:00 2001 From: Aaron Knudtson <87577305+knudtty@users.noreply.github.com> Date: Fri, 13 Nov 2026 16:17:03 -0500 Subject: [PATCH 1/3] feat: ad hoc filter finishing touches - Add IN (SQL) and =| (PromQL) operators; conditions on the same key are OR'd into one IN (...) or =~"a|b" condition. - Ad hoc filters always broadcast (isBroadcastEnabled: z.literal(true)), and their variable name is a unique identifier only, no longer declared as a $variable. - Warn when another ad hoc filter already applies to the same sources. - Collapse the filter input to three conditions behind a "+N more" toggle. --- .changeset/adhoc-filter-finishing-touches.md | 15 +++ .../api/__tests__/dashboard.int.test.ts | 4 +- .../__tests__/dashboards.int.test.ts | 1 + packages/app/src/DashboardFilters.tsx | 8 +- .../__tests__/DBDashboardImportPage.test.tsx | 2 + .../src/__tests__/DashboardFilters.test.ts | 5 +- .../AdhocDashboardFilter.tsx | 99 ++++++++++------ .../__tests__/AdhocDashboardFilter.test.tsx | 34 ++++++ .../__tests__/useAdhocFilterOptions.test.tsx | 1 + .../AdhocFilterEditForm.tsx | 90 ++++++++------ .../DashboardFiltersList.tsx | 5 +- .../VariableNameInput.tsx | 8 +- .../__tests__/AdhocFilterEditForm.test.tsx | 108 +++++++++++++++++ .../__tests__/filterFormState.test.ts | 11 +- .../DashboardFiltersModal/filterFormState.ts | 7 +- .../__tests__/FilterConditionEditor.test.tsx | 8 +- .../components/FilterPill/filterOperators.ts | 16 ++- .../__tests__/useDashboardFilters.test.tsx | 18 +-- .../dashboard-template-import.spec.ts | 6 + .../__tests__/dashboardFilterValues.test.ts | 7 +- .../src/__tests__/filters.test.ts | 63 +++++++++- .../common-utils/src/__tests__/types.test.ts | 10 ++ .../common-utils/src/__tests__/utils.test.ts | 4 + packages/common-utils/src/filters.ts | 112 +++++++++++++----- packages/common-utils/src/types.ts | 26 +++- 25 files changed, 516 insertions(+), 152 deletions(-) create mode 100644 .changeset/adhoc-filter-finishing-touches.md create mode 100644 packages/app/src/components/DashboardFiltersModal/__tests__/AdhocFilterEditForm.test.tsx diff --git a/.changeset/adhoc-filter-finishing-touches.md b/.changeset/adhoc-filter-finishing-touches.md new file mode 100644 index 0000000000..75ff3f0948 --- /dev/null +++ b/.changeset/adhoc-filter-finishing-touches.md @@ -0,0 +1,15 @@ +--- +'@hyperdx/common-utils': patch +'@hyperdx/app': patch +'@hyperdx/api': patch +--- + +feat: Ad hoc dashboard filter finishing touches + +- Add an `IN` operator for SQL and a `=|` operator for PromQL. Conditions on the + same key are OR'd into one `IN (...)` or `=~"a|b"` condition. +- Ad hoc filters always broadcast (`isBroadcastEnabled: true`), and their + variable name is only a unique identifier. It is no longer declared as a + `$variable`. +- Warn when another ad hoc filter already applies to the same sources. +- Collapse the filter input to three conditions behind a "+N more" toggle. diff --git a/packages/api/src/routers/api/__tests__/dashboard.int.test.ts b/packages/api/src/routers/api/__tests__/dashboard.int.test.ts index 2abf2baf64..bdd09283c4 100644 --- a/packages/api/src/routers/api/__tests__/dashboard.int.test.ts +++ b/packages/api/src/routers/api/__tests__/dashboard.int.test.ts @@ -1875,6 +1875,7 @@ describe('dashboard router', () => { const makeAdhocFilter = (overrides = {}) => ({ id: new Types.ObjectId().toString(), type: 'ADHOC' as const, + isBroadcastEnabled: true as const, name: 'Ad hoc', sourceType: 'sql' as const, sources: [logSourceId], @@ -1900,7 +1901,6 @@ describe('dashboard router', () => { const filter = makeAdhocFilter({ sources: [logSourceId, traceSourceId], appliesToSourceIds: [traceSourceId], - isBroadcastEnabled: false, }); const created = await agent @@ -1972,6 +1972,8 @@ describe('dashboard router', () => { ['an invalid variable name', { variableName: '1adhoc' }], ['not variable-enabled', { isVariableEnabled: false }], ['variables unset', { isVariableEnabled: undefined }], + ['broadcast disabled', { isBroadcastEnabled: false }], + ['broadcast unset', { isBroadcastEnabled: undefined }], ['a minimum selection', { minSelections: 1 }], ['a maximum selection', { maxSelections: 1 }], ['a global requirement', { isGlobalRequirement: true }], diff --git a/packages/api/src/routers/external-api/__tests__/dashboards.int.test.ts b/packages/api/src/routers/external-api/__tests__/dashboards.int.test.ts index 25d3f0b9c7..93fea0e4d5 100644 --- a/packages/api/src/routers/external-api/__tests__/dashboards.int.test.ts +++ b/packages/api/src/routers/external-api/__tests__/dashboards.int.test.ts @@ -8443,6 +8443,7 @@ describe('External API v2 Dashboards - new format', () => { const adhocFilter = () => ({ id: new ObjectId().toString(), type: 'ADHOC' as const, + isBroadcastEnabled: true as const, name: 'Ad hoc', sourceType: 'sql' as const, sources: [traceSource._id.toString()], diff --git a/packages/app/src/DashboardFilters.tsx b/packages/app/src/DashboardFilters.tsx index 5c15cdf9a5..56b7246232 100644 --- a/packages/app/src/DashboardFilters.tsx +++ b/packages/app/src/DashboardFilters.tsx @@ -73,9 +73,11 @@ export const getFilterEffect = (filter: DashboardFilter): FilterEffect => { ); } - const variableName = isFilterVariableEnabled(filter) - ? getFilterVariableName(filter) - : undefined; + // An ad hoc filter's variable name can't be referenced. + const variableName = + isFilterVariableEnabled(filter) && !isAdhocFilter(filter) + ? getFilterVariableName(filter) + : undefined; if (variableName) { parts.push( `${parts.length > 0 ? 'a' : 'A'}vailable as variable ($${variableName})`, diff --git a/packages/app/src/__tests__/DBDashboardImportPage.test.tsx b/packages/app/src/__tests__/DBDashboardImportPage.test.tsx index fee5937fd7..bf46e9e0e9 100644 --- a/packages/app/src/__tests__/DBDashboardImportPage.test.tsx +++ b/packages/app/src/__tests__/DBDashboardImportPage.test.tsx @@ -562,6 +562,7 @@ const adhocFilterTemplate = DashboardTemplateSchema.parse({ { id: 'filter-adhoc', type: 'ADHOC', + isBroadcastEnabled: true, name: 'Conditions', sourceType: 'sql', sources: ['Logs', 'Traces', MISSING], @@ -593,6 +594,7 @@ describe('Dashboard import - ad hoc filter sources', () => { const payload = mockMutateAsync.mock.calls[0][0]; expect(payload.filters[0]).toMatchObject({ type: 'ADHOC', + isBroadcastEnabled: true, sources: ['src-logs', 'src-traces'], appliesToSourceIds: ['src-traces'], }); diff --git a/packages/app/src/__tests__/DashboardFilters.test.ts b/packages/app/src/__tests__/DashboardFilters.test.ts index 6113b57271..e0111dfe2c 100644 --- a/packages/app/src/__tests__/DashboardFilters.test.ts +++ b/packages/app/src/__tests__/DashboardFilters.test.ts @@ -115,11 +115,12 @@ describe('getFilterEffect', () => { ).toEqual('Available as variable ($svc)'); }); - it('counts the sources an ad hoc filter broadcasts to', () => { + it('counts the sources an ad hoc filter broadcasts to, without a variable', () => { expect( getFilterEffect({ id: 'adhoc1', type: 'ADHOC', + isBroadcastEnabled: true, name: 'Conditions', sourceType: 'sql', sources: ['logs', 'traces'], @@ -128,7 +129,7 @@ describe('getFilterEffect', () => { }), ).toEqual({ hasEffect: true, - tooltip: 'Filters 2 sources, available as variable ($conds)', + tooltip: 'Filters 2 sources', }); }); }); diff --git a/packages/app/src/components/AdhocDashboardFilter/AdhocDashboardFilter.tsx b/packages/app/src/components/AdhocDashboardFilter/AdhocDashboardFilter.tsx index 8686035ab9..008860ab93 100644 --- a/packages/app/src/components/AdhocDashboardFilter/AdhocDashboardFilter.tsx +++ b/packages/app/src/components/AdhocDashboardFilter/AdhocDashboardFilter.tsx @@ -16,6 +16,7 @@ import { import { getVariableReferences } from '@hyperdx/common-utils/dist/variables'; import { ActionIcon, + Button, CloseButton, Group, Popover, @@ -39,6 +40,9 @@ import { notifyAdhocFilterFull, } from './notifications'; +// Series with many labels (common in PromQL) would otherwise grow the input tall. +const MAX_COLLAPSED_CONDITIONS = 3; + type AdhocDashboardFilterProps = { filter: AdhocDashboardFilterType; conditions: AdhocFilterCondition[]; @@ -62,7 +66,12 @@ export function AdhocDashboardFilter({ unsatisfiedRequiredFilters, }: AdhocDashboardFilterProps) { const [isAdding, setIsAdding] = useState(false); + const [isExpanded, setIsExpanded] = useState(false); const isAtLimit = conditions.length >= ADHOC_FILTER_MAX_CONDITIONS; + const hiddenCount = isExpanded + ? 0 + : Math.max(0, conditions.length - MAX_COLLAPSED_CONDITIONS); + const languageName = filter.sourceType === 'promql' ? 'PromQL' : 'SQL'; // The `where` narrowing the suggestions, and what it is still waiting on. // Blocked only by the required filters it references: a global requirement @@ -153,45 +162,57 @@ export function AdhocDashboardFilter({ borderRadius: 'var(--mantine-radius-sm)', }} > - {conditions.map((condition, index) => { - const operator = getFilterOperatorDisplay( - filter.sourceType, - condition.operator, - ); - return ( - remove(index)} - // Editing would look up suggestions; removing still works. - renderPopover={ - isDisabled - ? undefined - : close => ( - { - if (!isStorable(next)) return; - replace(index, next); - close(); - }} - /> - ) - } - data-testid="adhoc-condition-pill" - /> - ); - })} + {conditions + .slice(0, conditions.length - hiddenCount) + .map((condition, index) => { + const operator = getFilterOperatorDisplay( + filter.sourceType, + condition.operator, + ); + return ( + remove(index)} + // Editing would look up suggestions; removing still works. + renderPopover={ + isDisabled + ? undefined + : close => ( + { + if (!isStorable(next)) return; + replace(index, next); + close(); + }} + /> + ) + } + data-testid="adhoc-condition-pill" + /> + ); + })} + {conditions.length > MAX_COLLAPSED_CONDITIONS && ( + + )} {conditions.length === 0 && ( ({ const filter: AdhocDashboardFilterType = { id: 'adhoc1', type: 'ADHOC', + isBroadcastEnabled: true, name: 'Conditions', sourceType: 'sql', sources: ['logs'], @@ -81,6 +82,39 @@ describe('AdhocDashboardFilter', () => { expect(pills[1]).toHaveTextContent('Level regex ^err'); }); + it('collapses conditions past the first three until expanded', async () => { + renderFilter( + Array.from({ length: 5 }, (_, i) => ({ ...existing, value: `${i}` })), + ); + const toggle = screen.getByTestId('adhoc-filter-toggle-more-Conditions'); + + expect(screen.getAllByTestId('adhoc-condition-pill')).toHaveLength(3); + expect(toggle).toHaveTextContent('+2 more'); + + await userEvent.click(toggle); + expect(screen.getAllByTestId('adhoc-condition-pill')).toHaveLength(5); + expect(toggle).toHaveTextContent('Show less'); + + await userEvent.click(toggle); + expect(screen.getAllByTestId('adhoc-condition-pill')).toHaveLength(3); + }); + + it('shows no toggle for three conditions or fewer', () => { + renderFilter([existing, existing, existing]); + expect( + screen.queryByTestId('adhoc-filter-toggle-more-Conditions'), + ).not.toBeInTheDocument(); + }); + + it('marks conditions a SQL filter cannot apply as invalid', () => { + renderFilter([{ key: 'job', operator: '=|', value: 'api' }]); + + expect(screen.getByTestId('adhoc-condition-pill')).toHaveAttribute( + 'data-invalid', + 'true', + ); + }); + it('marks conditions a PromQL filter cannot apply as invalid', () => { renderFilter( [ diff --git a/packages/app/src/components/AdhocDashboardFilter/__tests__/useAdhocFilterOptions.test.tsx b/packages/app/src/components/AdhocDashboardFilter/__tests__/useAdhocFilterOptions.test.tsx index e0880af24f..755d36e428 100644 --- a/packages/app/src/components/AdhocDashboardFilter/__tests__/useAdhocFilterOptions.test.tsx +++ b/packages/app/src/components/AdhocDashboardFilter/__tests__/useAdhocFilterOptions.test.tsx @@ -181,6 +181,7 @@ describe('useAdhocFilterOptions', () => { ): AdhocDashboardFilter => ({ id: 'f', type: 'ADHOC', + isBroadcastEnabled: true, name: 'Conditions', sourceType, sources, diff --git a/packages/app/src/components/DashboardFiltersModal/AdhocFilterEditForm.tsx b/packages/app/src/components/DashboardFiltersModal/AdhocFilterEditForm.tsx index aa54074502..8a1c88cffd 100644 --- a/packages/app/src/components/DashboardFiltersModal/AdhocFilterEditForm.tsx +++ b/packages/app/src/components/DashboardFiltersModal/AdhocFilterEditForm.tsx @@ -2,15 +2,19 @@ import { useMemo } from 'react'; import { Controller, UseFormSetValue, useWatch } from 'react-hook-form'; import { Separator } from '@clickhouse/click-ui'; import { tcFromSource } from '@hyperdx/common-utils/dist/core/metadata'; -import { getAdhocFilterSourceKinds } from '@hyperdx/common-utils/dist/filters'; +import { + getAdhocFilterAppliesToSourceIds, + getAdhocFilterSourceKinds, + isAdhocFilter, +} from '@hyperdx/common-utils/dist/filters'; import { AdhocFilterSourceType, DashboardFilter, SourceKind, } from '@hyperdx/common-utils/dist/types'; -import { Box, SegmentedControl } from '@mantine/core'; +import { Alert, SegmentedControl } from '@mantine/core'; +import { IconAlertTriangle } from '@tabler/icons-react'; -import { CheckBoxControlled } from '@/components/InputControlled'; import SearchWhereInput from '@/components/SearchInput/SearchWhereInput'; import { SourceMultiSelectControlled } from '@/components/SourceMultiSelect'; import { IS_PROMQL_ENABLED } from '@/config'; @@ -40,10 +44,26 @@ export const AdhocFilterEditForm = ({ otherFilters, }: AdhocFilterEditFormProps) => { const { data: allSources } = useSources(); - const [sourceType, isBroadcastEnabled, sourceIds] = useWatch({ + const [sourceType, sourceIds, appliesToSourceIds] = useWatch({ control, - name: ['sourceType', 'isBroadcastEnabled', 'sources'], + name: ['sourceType', 'sources', 'appliesToSourceIds'], }); + // Click-to-filter updates every ad hoc filter reaching the clicked tile. + const overlappingFilters = useMemo(() => { + const targets = getAdhocFilterAppliesToSourceIds({ + sources: sourceIds ?? [], + appliesToSourceIds: appliesToSourceIds?.filter(id => !!id), + }); + return otherFilters + .filter(isAdhocFilter) + .filter( + other => + other.sourceType === sourceType && + getAdhocFilterAppliesToSourceIds(other).some(id => + targets.includes(id), + ), + ); + }, [otherFilters, sourceType, sourceIds, appliesToSourceIds]); const allowedSourceKinds = getAdhocFilterSourceKinds(sourceType); const tableConnections = useMemo( () => @@ -138,39 +158,41 @@ export const AdhocFilterEditForm = ({ )} - - - {isBroadcastEnabled && ( - - - - - + > + + + {overlappingFilters.length > 0 && ( + } + data-testid="adhoc-filter-overlap-warning" + > + Another ad hoc filter ( + {overlappingFilters.map(f => f.name).join(', ')}) already applies to + these sources. Clicking a value in a tile will update all of them. + )} - + ); }; diff --git a/packages/app/src/components/DashboardFiltersModal/DashboardFiltersList.tsx b/packages/app/src/components/DashboardFiltersModal/DashboardFiltersList.tsx index 22e99756ed..a9215a4bb9 100644 --- a/packages/app/src/components/DashboardFiltersModal/DashboardFiltersList.tsx +++ b/packages/app/src/components/DashboardFiltersModal/DashboardFiltersList.tsx @@ -1,6 +1,7 @@ import { getFilterBroadcastTarget, getFilterVariableName, + isAdhocFilter, isFilterGlobalRequirement, isFilterRequired, isFilterVariableEnabled, @@ -162,7 +163,9 @@ export const DashboardFiltersList = ({ } const variableName = - showVariableOptions && isFilterVariableEnabled(filter) + showVariableOptions && + isFilterVariableEnabled(filter) && + !isAdhocFilter(filter) ? getFilterVariableName(filter) : undefined; return ( diff --git a/packages/app/src/components/DashboardFiltersModal/VariableNameInput.tsx b/packages/app/src/components/DashboardFiltersModal/VariableNameInput.tsx index 72f8dbdf93..74b5db9424 100644 --- a/packages/app/src/components/DashboardFiltersModal/VariableNameInput.tsx +++ b/packages/app/src/components/DashboardFiltersModal/VariableNameInput.tsx @@ -12,11 +12,14 @@ import { FilterFormControl } from './filterFormState'; interface VariableNameInputProps { control: FilterFormControl; otherFilters: DashboardFilter[]; + /** Replaces the tooltip, for filters whose name isn't a referenceable variable. */ + description?: string; } export const VariableNameInput = ({ control, otherFilters, + description, }: VariableNameInputProps) => { const { errors } = useFormState({ control }); const filterName = useWatch({ control, name: 'name' }); @@ -25,10 +28,13 @@ export const VariableNameInput = ({ return ( ({ useSources: () => ({ data: [] }) })); +// The real inputs pull CodeMirror and source metadata into jsdom. +jest.mock('@/components/SearchInput/SearchWhereInput', () => ({ + __esModule: true, + default: () => null, + getStoredLanguage: () => undefined, +})); +jest.mock('@/components/SourceMultiSelect', () => ({ + SourceMultiSelectControlled: ({ name }: { name: string }) => ( +
+ ), +})); + +const adhoc = ( + overrides: Partial = {}, +): AdhocDashboardFilter => ({ + id: 'a1', + type: 'ADHOC', + name: 'Conditions', + sourceType: 'sql', + sources: ['logs'], + isBroadcastEnabled: true, + isVariableEnabled: true, + ...overrides, +}); + +function Harness({ + filter, + otherFilters, +}: { + filter: AdhocDashboardFilter; + otherFilters: DashboardFilter[]; +}) { + const { control, setValue } = useForm({ + defaultValues: toFormValues(filter), + }); + return ( + + ); +} + +const renderForm = ( + filter: AdhocDashboardFilter, + otherFilters: DashboardFilter[] = [], +) => renderWithMantine(); + +const warning = () => screen.queryByTestId('adhoc-filter-overlap-warning'); + +describe('AdhocFilterEditForm', () => { + it('always shows the broadcast scope, with no broadcast toggle', () => { + renderForm(adhoc()); + + expect( + screen.getByTestId('multi-select-appliesToSourceIds'), + ).toBeInTheDocument(); + expect( + screen.queryByTestId('filter-broadcast-checkbox'), + ).not.toBeInTheDocument(); + }); + + it('says the name is not a referenceable variable', () => { + renderForm(adhoc()); + + expect( + screen.getByText(/can't be referenced as variables/), + ).toBeInTheDocument(); + }); + + it('warns about another ad hoc filter reaching the same source', () => { + renderForm(adhoc(), [ + adhoc({ id: 'a2', name: 'Other', sources: ['traces'] }), + adhoc({ id: 'a3', name: 'Logs too', appliesToSourceIds: ['logs'] }), + ]); + + expect(warning()).toHaveTextContent('Another ad hoc filter (Logs too)'); + }); + + it('compares broadcast scopes rather than suggestion sources', () => { + renderForm(adhoc({ appliesToSourceIds: ['traces'] }), [ + adhoc({ id: 'a2', name: 'Other' }), + ]); + + expect(warning()).not.toBeInTheDocument(); + }); + + it('ignores ad hoc filters of the other source type', () => { + renderForm(adhoc(), [adhoc({ id: 'a2', sourceType: 'promql' })]); + + expect(warning()).not.toBeInTheDocument(); + }); +}); diff --git a/packages/app/src/components/DashboardFiltersModal/__tests__/filterFormState.test.ts b/packages/app/src/components/DashboardFiltersModal/__tests__/filterFormState.test.ts index d0e3131fbe..4d11730335 100644 --- a/packages/app/src/components/DashboardFiltersModal/__tests__/filterFormState.test.ts +++ b/packages/app/src/components/DashboardFiltersModal/__tests__/filterFormState.test.ts @@ -89,7 +89,7 @@ describe('toFormValues', () => { sourceType: 'promql', sources: ['prom'], appliesToSourceIds: ['prom-2'], - isBroadcastEnabled: false, + isBroadcastEnabled: true, isVariableEnabled: true, variableName: 'conds', }); @@ -100,7 +100,7 @@ describe('toFormValues', () => { sources: ['prom'], appliesToSourceIds: ['prom-2'], variableName: 'conds', - isBroadcastEnabled: false, + isBroadcastEnabled: true, }); }); @@ -117,6 +117,7 @@ describe('toFormValues', () => { sources: ['logs'], where: 'Namespace:$namespace', whereLanguage: 'lucene', + isBroadcastEnabled: true, isVariableEnabled: true, }); @@ -294,7 +295,7 @@ describe('toSavedFilter', () => { expect(saved).not.toHaveProperty('whereLanguage'); }); - it('drops the ad hoc broadcast scope when broadcast is off', () => { + it('always broadcasts an ad hoc filter, even when switched from a non-broadcasting type', () => { const saved = toSavedFilter( formValues({ type: 'ADHOC', @@ -306,8 +307,8 @@ describe('toSavedFilter', () => { ); expect(saved).toMatchObject({ - isBroadcastEnabled: false, - appliesToSourceIds: undefined, + isBroadcastEnabled: true, + appliesToSourceIds: ['logs'], }); }); diff --git a/packages/app/src/components/DashboardFiltersModal/filterFormState.ts b/packages/app/src/components/DashboardFiltersModal/filterFormState.ts index 7e450282eb..7b681fe2f5 100644 --- a/packages/app/src/components/DashboardFiltersModal/filterFormState.ts +++ b/packages/app/src/components/DashboardFiltersModal/filterFormState.ts @@ -149,7 +149,6 @@ export const toSavedFilter = (values: FilterFormValues): DashboardFilter => { // Selection limits don't apply to conditions, so they aren't stored. if (values.type === 'ADHOC') { - const isBroadcastEnabled = isFilterBroadcastEnabled(values); return DashboardFilterSchema.parse({ id: values.id, type: values.type, @@ -158,10 +157,8 @@ export const toSavedFilter = (values: FilterFormValues): DashboardFilter => { sources: values.sources, // Only SQL suggestions can be narrowed; the input is hidden otherwise ...(values.sourceType === 'sql' ? whereFields : {}), - isBroadcastEnabled, - // Hidden, and so unvalidated, while broadcast is off - appliesToSourceIds: - isBroadcastEnabled && appliesTo?.length ? appliesTo : undefined, + isBroadcastEnabled: true, + appliesToSourceIds: appliesTo?.length ? appliesTo : undefined, isVariableEnabled: true, variableName: getFilterVariableName(values), }); diff --git a/packages/app/src/components/FilterPill/__tests__/FilterConditionEditor.test.tsx b/packages/app/src/components/FilterPill/__tests__/FilterConditionEditor.test.tsx index 19e9740bbf..a1df5781c0 100644 --- a/packages/app/src/components/FilterPill/__tests__/FilterConditionEditor.test.tsx +++ b/packages/app/src/components/FilterPill/__tests__/FilterConditionEditor.test.tsx @@ -186,7 +186,13 @@ describe('FilterConditionEditor', () => { renderEditor({ operators: PROMQL_FILTER_OPERATORS }); await userEvent.click(screen.getByRole('combobox', { name: 'Operator' })); const options = await screen.findAllByRole('option', { hidden: true }); - expect(options.map(o => o.textContent)).toEqual(['=', '!=', '=~', '!~']); + expect(options.map(o => o.textContent)).toEqual([ + '=', + '!=', + '=~', + '!~', + '=|', + ]); }); it('disables Apply until a key is entered', async () => { diff --git a/packages/app/src/components/FilterPill/filterOperators.ts b/packages/app/src/components/FilterPill/filterOperators.ts index ece901cbf7..1caa35b089 100644 --- a/packages/app/src/components/FilterPill/filterOperators.ts +++ b/packages/app/src/components/FilterPill/filterOperators.ts @@ -1,6 +1,7 @@ import { AdhocFilterOperator, PROMQL_ADHOC_FILTER_OPERATORS, + SQL_ADHOC_FILTER_OPERATORS, } from '@hyperdx/common-utils/dist/types'; import type { FilterOperatorOption } from './FilterConditionEditor'; @@ -16,19 +17,30 @@ const REGEX_VALUE = { }; const LIKE_VALUE = { valuePlaceholder: 'Pattern, e.g. %error%' }; -export const SQL_FILTER_OPERATORS: FilterOperatorOption[] = [ +// One value per condition; the renderer ORs conditions sharing a key. +const ONE_OF = { + label: 'in', + valuePlaceholder: "OR'd with this key's other values", +}; + +const FILTER_OPERATORS: FilterOperatorOption[] = [ { value: '=', label: '=' }, { value: '!=', label: '!=', negated: true }, { value: '=~', label: 'regex', ...REGEX_VALUE }, { value: '!~', label: 'not regex', negated: true, ...REGEX_VALUE }, { value: 'LIKE', label: 'like', ...LIKE_VALUE }, { value: 'NOT LIKE', label: 'not like', negated: true, ...LIKE_VALUE }, + { value: 'IN', ...ONE_OF }, + { value: '=|', ...ONE_OF }, ]; +export const SQL_FILTER_OPERATORS: FilterOperatorOption[] = + FILTER_OPERATORS.filter(op => SQL_ADHOC_FILTER_OPERATORS.includes(op.value)); + // Derived so the editor never offers an operator the PromQL broadcast skips. // PromQL users know the operator syntax, so it doubles as the label. export const PROMQL_FILTER_OPERATORS: FilterOperatorOption[] = - SQL_FILTER_OPERATORS.filter(op => + FILTER_OPERATORS.filter(op => PROMQL_ADHOC_FILTER_OPERATORS.includes(op.value), ).map(op => ({ ...op, label: op.value })); diff --git a/packages/app/src/hooks/__tests__/useDashboardFilters.test.tsx b/packages/app/src/hooks/__tests__/useDashboardFilters.test.tsx index 41f9223420..7e605b78a1 100644 --- a/packages/app/src/hooks/__tests__/useDashboardFilters.test.tsx +++ b/packages/app/src/hooks/__tests__/useDashboardFilters.test.tsx @@ -995,6 +995,7 @@ describe('useDashboardFilters', () => { const adhocFilter: AdhocDashboardFilter = { id: 'adhoc1', type: 'ADHOC', + isBroadcastEnabled: true, name: 'Conditions', sourceType: 'sql', sources: ['logs'], @@ -1222,15 +1223,6 @@ describe('useDashboardFilters', () => { ).toEqual([conditionSql]); }); - it('applies nothing when broadcast is disabled', () => { - mockState = [{ type: 'adhoc', name: 'conds', conditions: [condition] }]; - const filters = [{ ...adhocFilter, isBroadcastEnabled: false }]; - - const { result } = renderHook(() => useDashboardFilters(filters)); - - expect(result.current.getFilterQueriesForSource('logs')).toEqual([]); - }); - describe('PromQL', () => { const promqlFilter: AdhocDashboardFilter = { ...adhocFilter, @@ -1274,14 +1266,6 @@ describe('useDashboardFilters', () => { ).toEqual([jobMatcher]); }); - it('applies nothing when broadcast is disabled', () => { - const filters = [{ ...promqlFilter, isBroadcastEnabled: false }]; - - const { result } = renderHook(() => useDashboardFilters(filters)); - - expect(result.current.getFilterQueriesForSource('prom')).toEqual([]); - }); - it('combines conditions across filters, deduplicated', () => { mockState = [ { diff --git a/packages/app/tests/e2e/features/dashboard-template-import.spec.ts b/packages/app/tests/e2e/features/dashboard-template-import.spec.ts index 6abeb69e86..5ee3f20415 100644 --- a/packages/app/tests/e2e/features/dashboard-template-import.spec.ts +++ b/packages/app/tests/e2e/features/dashboard-template-import.spec.ts @@ -134,6 +134,7 @@ function makeAdhocFilterTemplate({ { id: 'filter-1', type: 'ADHOC', + isBroadcastEnabled: true, name: 'Conditions', sourceType: 'sql', sources, @@ -748,6 +749,7 @@ test.describe('Dashboard Template Import', { tag: ['@dashboard'] }, () => { expect(dashboard.filters).toHaveLength(1); expect(dashboard.filters[0]).toMatchObject({ type: 'ADHOC', + isBroadcastEnabled: true, sources: [logsSourceId, tracesSourceId], appliesToSourceIds: [tracesSourceId], }); @@ -841,6 +843,7 @@ test.describe('Dashboard Template Import', { tag: ['@dashboard'] }, () => { ); expect(dashboard.filters[0]).toMatchObject({ type: 'ADHOC', + isBroadcastEnabled: true, sources: [logsSourceId], appliesToSourceIds: [tracesSourceId], }); @@ -897,6 +900,7 @@ test.describe('Dashboard Template Import', { tag: ['@dashboard'] }, () => { ); expect(dashboard.filters[0]).toMatchObject({ type: 'ADHOC', + isBroadcastEnabled: true, sources: [tracesSourceId], }); }); @@ -990,6 +994,7 @@ test.describe('Dashboard Template Import', { tag: ['@dashboard'] }, () => { ); expect(dashboard.filters[0]).toMatchObject({ type: 'ADHOC', + isBroadcastEnabled: true, sources: [tracesSourceId, logsSourceId, sessionsSourceId], }); }); @@ -1047,6 +1052,7 @@ test.describe('Dashboard Template Import', { tag: ['@dashboard'] }, () => { ); expect(dashboard.filters[0]).toMatchObject({ type: 'ADHOC', + isBroadcastEnabled: true, sources: [logsSourceId], appliesToSourceIds: [logsSourceId], }); diff --git a/packages/common-utils/src/__tests__/dashboardFilterValues.test.ts b/packages/common-utils/src/__tests__/dashboardFilterValues.test.ts index 0a3a177dd1..300e037754 100644 --- a/packages/common-utils/src/__tests__/dashboardFilterValues.test.ts +++ b/packages/common-utils/src/__tests__/dashboardFilterValues.test.ts @@ -907,6 +907,7 @@ describe('dashboardFilterValues', () => { ): AdhocDashboardFilter => ({ id: 'a1', type: 'ADHOC', + isBroadcastEnabled: true, name: 'Conditions', sourceType: 'sql', sources: ['logs'], @@ -929,12 +930,6 @@ describe('dashboardFilterValues', () => { expect(ids(filters, builderTile, 'logs')).toEqual(['a1', 'a2']); }); - it('skips filters that do not broadcast', () => { - expect( - ids([adhoc({ isBroadcastEnabled: false })], builderTile, 'logs'), - ).toEqual([]); - }); - it('matches the filter language to the tile', () => { const filters = [ adhoc(), diff --git a/packages/common-utils/src/__tests__/filters.test.ts b/packages/common-utils/src/__tests__/filters.test.ts index 0f26d14929..3627004a9e 100644 --- a/packages/common-utils/src/__tests__/filters.test.ts +++ b/packages/common-utils/src/__tests__/filters.test.ts @@ -802,6 +802,7 @@ describe('filters', () => { { id: 'conds', type: 'ADHOC', + isBroadcastEnabled: true, name: 'Conditions', sourceType: 'sql', sources: ['logs'], @@ -826,6 +827,7 @@ describe('filters', () => { { id: 'conds', type: 'ADHOC', + isBroadcastEnabled: true, name: 'Conditions', sourceType: 'promql', sources: ['metrics'], @@ -1680,14 +1682,12 @@ describe('filters', () => { name: 'Conditions', sourceType: 'sql', sources: ['logs', 'traces'], + isBroadcastEnabled: true, isVariableEnabled: true, }; expect(getFilterBroadcastTarget(adhoc)).toEqual({ appliesToSourceIds: ['logs', 'traces'], }); - expect( - getFilterBroadcastTarget({ ...adhoc, isBroadcastEnabled: false }), - ).toBeUndefined(); }); it('returns undefined when broadcasting is off', () => { @@ -1825,10 +1825,37 @@ describe('filters', () => { ['!~', "NOT match(toString(ServiceName), 'api')"], ['LIKE', "toString(ServiceName) LIKE 'api'"], ['NOT LIKE', "toString(ServiceName) NOT LIKE 'api'"], + ['IN', "toString(ServiceName) IN ('api')"], ])('renders %s', (operator, condition) => { expect(conditionFor(operator)).toEqual([{ type: 'sql', condition }]); }); + it('ORs IN conditions sharing a key, at the first one', () => { + expect( + adhocConditionsToFilters([ + { key: 'SeverityText', operator: '=', value: 'error' }, + { key: 'ServiceName', operator: 'IN', value: 'api' }, + { key: 'Env', operator: 'IN', value: 'prod' }, + { key: 'ServiceName', operator: 'IN', value: "it's" }, + { key: 'ServiceName', operator: '!=', value: 'web' }, + ]).map(filter => filter.condition), + ).toEqual([ + "toString(SeverityText) = 'error'", + "toString(ServiceName) IN ('api', 'it''s')", + "toString(Env) IN ('prod')", + "toString(ServiceName) != 'web'", + ]); + }); + + it('skips =| rather than throwing', () => { + expect( + adhocConditionsToFilters([ + { key: 'ServiceName', operator: '=|', value: 'api' }, + { key: 'Env', operator: '=', value: 'prod' }, + ]), + ).toEqual([{ type: 'sql', condition: "toString(Env) = 'prod'" }]); + }); + it('passes LIKE patterns through as written', () => { expect(conditionFor('LIKE', '%err_r%')).toEqual([ { type: 'sql', condition: "toString(ServiceName) LIKE '%err_r%'" }, @@ -2004,11 +2031,22 @@ describe('filters', () => { ['!=', 'service!="api"'], ['=~', 'service=~"api"'], ['!~', 'service!~"api"'], + ['=|', 'service=~"api"'], ])('renders %s', (operator, matcher) => { expect(matcherFor(operator)).toEqual([matcher]); }); - it.each(['LIKE', 'NOT LIKE'])( + it('ORs =| conditions sharing a label into one escaped regex', () => { + expect( + adhocConditionsToPromqlFilters([ + { key: 'service', operator: '=|', value: 'api' }, + { key: 'job', operator: '=|', value: 'node' }, + { key: 'service', operator: '=|', value: 'a.b\\c' }, + ]).map(filter => filter.condition), + ).toEqual(['service=~"(api|a\\\\.b\\\\\\\\c)"', 'job=~"node"']); + }); + + it.each(['LIKE', 'NOT LIKE', 'IN'])( 'skips %s rather than throwing', operator => { expect( @@ -2059,6 +2097,10 @@ describe('filters', () => { ['promql', 'NOT LIKE', false], ['promql', '=~', true], ['promql', '!=', true], + ['sql', 'IN', true], + ['sql', '=|', false], + ['promql', '=|', true], + ['promql', 'IN', false], ])('%s supports %s: %s', (sourceType, operator, expected) => { expect( isAdhocConditionSupported(sourceType, { @@ -2225,6 +2267,19 @@ describe('filters', () => { ).toEqual([]); }); + it('skips ad hoc filters, whose name is not referenceable', () => { + const adhoc: AdhocDashboardFilter = { + id: 'a1', + type: 'ADHOC', + name: 'Conditions', + sourceType: 'sql', + sources: ['logs'], + isBroadcastEnabled: true, + isVariableEnabled: true, + }; + expect(getDashboardVariableFilters([adhoc])).toEqual([]); + }); + it('skips a filter whose display name derives nothing usable', () => { expect( getDashboardVariableFilters([ diff --git a/packages/common-utils/src/__tests__/types.test.ts b/packages/common-utils/src/__tests__/types.test.ts index 1640578a2e..e7172940db 100644 --- a/packages/common-utils/src/__tests__/types.test.ts +++ b/packages/common-utils/src/__tests__/types.test.ts @@ -716,6 +716,7 @@ describe('DashboardFilterSchema ad hoc variant', () => { const adhocFilter = { id: 'f1', type: 'ADHOC' as const, + isBroadcastEnabled: true as const, name: 'Conditions', sourceType: 'sql' as const, sources: ['source-1'], @@ -740,6 +741,15 @@ describe('DashboardFilterSchema ad hoc variant', () => { }).success, ).toBe(false); }); + + it.each([false, undefined])('rejects isBroadcastEnabled: %s', value => { + expect( + DashboardFilterSchema.safeParse({ + ...adhocFilter, + isBroadcastEnabled: value, + }).success, + ).toBe(false); + }); }); // Preset dashboards are broadcast-only, so the preset schema extends the diff --git a/packages/common-utils/src/__tests__/utils.test.ts b/packages/common-utils/src/__tests__/utils.test.ts index 327585152b..22ec34b1e4 100644 --- a/packages/common-utils/src/__tests__/utils.test.ts +++ b/packages/common-utils/src/__tests__/utils.test.ts @@ -1390,6 +1390,7 @@ describe('utils', () => { { id: 'filter-scoped', type: 'ADHOC', + isBroadcastEnabled: true, name: 'Scoped', sourceType: 'sql', sources: ['source1', 'source2'], @@ -1400,6 +1401,7 @@ describe('utils', () => { { id: 'filter-unresolved', type: 'ADHOC', + isBroadcastEnabled: true, name: 'Unresolved', sourceType: 'sql', sources: ['source1'], @@ -1416,6 +1418,7 @@ describe('utils', () => { { id: 'filter-scoped', type: 'ADHOC', + isBroadcastEnabled: true, name: 'Scoped', sourceType: 'sql', sources: ['Logs', 'Traces'], @@ -1426,6 +1429,7 @@ describe('utils', () => { { id: 'filter-unresolved', type: 'ADHOC', + isBroadcastEnabled: true, name: 'Unresolved', sourceType: 'sql', sources: ['Logs'], diff --git a/packages/common-utils/src/filters.ts b/packages/common-utils/src/filters.ts index 3412e16d8c..e752d52111 100644 --- a/packages/common-utils/src/filters.ts +++ b/packages/common-utils/src/filters.ts @@ -23,6 +23,7 @@ import { } from '@/types'; import { escapePromqlStringValue, + formatVariableValues, getVariableReferences, substituteVariables, } from '@/variables'; @@ -966,15 +967,55 @@ export function isAdhocFilterValue( export const isAdhocComparableDataType = (type: string): boolean => !/\bDateTime/.test(type); +/** A condition with the values of every condition it was merged with. */ +type GroupedAdhocCondition = { + condition: AdhocFilterCondition; + values: string[]; +}; + +/** + * Merge the conditions using `operator` that share a key into the first of + * them, so their values can be OR'd. Other conditions keep one value each. + */ +const groupAdhocConditionsByKey = ( + conditions: AdhocFilterCondition[], + operator: AdhocFilterOperator, +): GroupedAdhocCondition[] => { + const valuesByKey = new Map(); + const grouped: GroupedAdhocCondition[] = []; + for (const condition of conditions) { + const existing = + condition.operator === operator + ? valuesByKey.get(condition.key) + : undefined; + if (existing) { + existing.push(condition.value); + continue; + } + const values = [condition.value]; + if (condition.operator === operator) valuesByKey.set(condition.key, values); + grouped.push({ condition, values }); + } + return grouped; +}; + +const unsupportedOperatorError = ( + operator: AdhocFilterOperator, + sourceType: AdhocFilterSourceType, +) => + new Error( + `Ad hoc filter operator '${operator}' is not supported for ${sourceType === 'sql' ? 'SQL' : 'PromQL'}`, + ); + const renderAdhocConditionSql = ({ - key, - operator, - value, -}: AdhocFilterCondition): string => { + condition: { key, operator, value }, + values, +}: GroupedAdhocCondition): string => { // Stringified like broadcast filter selections, so Map/JSON/Dynamic values // compare as strings. const target = `toString(${key})`; - const literal = `'${escapeSqlString(value)}'`; + const toLiteral = (v: string) => `'${escapeSqlString(v)}'`; + const literal = toLiteral(value); switch (operator) { case '=': case '!=': @@ -985,6 +1026,10 @@ const renderAdhocConditionSql = ({ return `match(${target}, ${literal})`; case '!~': return `NOT match(${target}, ${literal})`; + case 'IN': + return `${target} IN (${values.map(toLiteral).join(', ')})`; + case '=|': + throw unsupportedOperatorError(operator, 'sql'); default: operator satisfies never; throw new Error(`Unknown ad hoc filter operator '${operator}'`); @@ -992,16 +1037,21 @@ const renderAdhocConditionSql = ({ }; /** - * Render the conditions of a SQL ad hoc filter as filters, one per condition. - * Keys are SQL expressions and are used verbatim. + * Render the conditions of a SQL ad hoc filter as filters, one per condition, + * except that `IN` conditions sharing a key become one. Keys are SQL + * expressions and are used verbatim. Conditions SQL can't express are skipped; + * callers report them using `isAdhocConditionSupported`. */ export function adhocConditionsToFilters( conditions: AdhocFilterCondition[], ): ConditionFilter[] { - return conditions.map( - (condition): ConditionFilter => ({ + const supported = conditions.filter(condition => + isAdhocConditionSupported('sql', condition), + ); + return groupAdhocConditionsByKey(supported, 'IN').map( + (grouped): ConditionFilter => ({ type: 'sql', - condition: renderAdhocConditionSql(condition), + condition: renderAdhocConditionSql(grouped), }), ); } @@ -1062,10 +1112,9 @@ export function mergeAdhocConditions( const LEGACY_LABEL_NAME = /^[a-zA-Z_][a-zA-Z0-9_]*$/; const renderAdhocMatcherPromql = ({ - key, - operator, - value, -}: AdhocFilterCondition): string => { + condition: { key, operator, value }, + values, +}: GroupedAdhocCondition): string => { // Label names outside the legacy charset must be quoted (Prometheus 3). const label = LEGACY_LABEL_NAME.test(key) ? key @@ -1076,11 +1125,14 @@ const renderAdhocMatcherPromql = ({ case '=~': case '!~': return `${label}${operator}"${escapePromqlStringValue(value)}"`; + case '=|': { + const pattern = formatVariableValues({ values }, 'regex'); + return `${label}=~"${escapePromqlStringValue(pattern)}"`; + } case 'LIKE': case 'NOT LIKE': - throw new Error( - `Ad hoc filter operator '${operator}' is not supported for PromQL`, - ); + case 'IN': + throw unsupportedOperatorError(operator, 'promql'); default: operator satisfies never; throw new Error(`Unknown ad hoc filter operator '${operator}'`); @@ -1089,7 +1141,8 @@ const renderAdhocMatcherPromql = ({ /** * Whether an ad hoc filter of `sourceType` can apply `condition`. A filter - * switched from SQL to PromQL keeps its conditions, and PromQL has no LIKE. + * switched between SQL and PromQL keeps its conditions, and PromQL has no LIKE + * or IN, nor SQL a `=|`. */ export function isAdhocConditionSupported( sourceType: AdhocFilterSourceType, @@ -1108,20 +1161,22 @@ export function isAdhocConditionSupported( /** * Render the conditions of a PromQL ad hoc filter as label matchers, one per - * condition. Conditions PromQL can't express are skipped; callers report them + * condition, except that `=|` conditions sharing a label become one regex + * matcher. Conditions PromQL can't express are skipped; callers report them * using `isAdhocConditionSupported`. */ export function adhocConditionsToPromqlFilters( conditions: AdhocFilterCondition[], ): PromqlConditionFilter[] { - return conditions - .filter(condition => isAdhocConditionSupported('promql', condition)) - .map( - (condition): PromqlConditionFilter => ({ - type: 'promql', - condition: renderAdhocMatcherPromql(condition), - }), - ); + const supported = conditions.filter(condition => + isAdhocConditionSupported('promql', condition), + ); + return groupAdhocConditionsByKey(supported, '=|').map( + (grouped): PromqlConditionFilter => ({ + type: 'promql', + condition: renderAdhocMatcherPromql(grouped), + }), + ); } function isPromqlFilter( @@ -1211,6 +1266,7 @@ export type DashboardVariableDeclaration = Pick< /** Minimal projection of fields necessary to extract the variables a dashboard declares. */ export type FilterForVariableDeclaration = { + type?: DashboardFilterKind; name: string; expression?: string; variableName?: string; @@ -1230,6 +1286,8 @@ export function getDashboardVariableFilters< for (const filter of filters ?? []) { if (!isFilterVariableEnabled(filter)) continue; + // Their variable name only keys their conditions; it isn't referenceable. + if (filter.type === 'ADHOC') continue; // There shouldn't be any duplicate names, but if there are then the first one wins. const name = getFilterVariableName(filter); diff --git a/packages/common-utils/src/types.ts b/packages/common-utils/src/types.ts index dba9c7854e..7c6844d519 100644 --- a/packages/common-utils/src/types.ts +++ b/packages/common-utils/src/types.ts @@ -1335,18 +1335,32 @@ export const AdhocFilterOperatorSchema = z.enum([ '!~', 'LIKE', 'NOT LIKE', + 'IN', + '=|', ]); export type AdhocFilterOperator = z.infer; -export const SQL_ADHOC_FILTER_OPERATORS: readonly AdhocFilterOperator[] = - AdhocFilterOperatorSchema.options; +/** + * `IN` (SQL) and `=|` (PromQL) conditions sharing a key are OR'd together when + * rendered; every other condition is AND'd. + */ +export const SQL_ADHOC_FILTER_OPERATORS: readonly AdhocFilterOperator[] = [ + '=', + '!=', + '=~', + '!~', + 'LIKE', + 'NOT LIKE', + 'IN', +]; export const PROMQL_ADHOC_FILTER_OPERATORS: readonly AdhocFilterOperator[] = [ '=', '!=', '=~', '!~', + '=|', ]; /** One `key operator value` condition picked in an ad hoc filter. */ @@ -2434,8 +2448,12 @@ export const AdhocDashboardFilterSchema = dashboardFilterBaseSchema * empty means the filter's `sources`. */ appliesToSourceIds: z.array(z.string().min(1)).optional(), - /** Undefined / missing means ENABLED, as for QUERY_EXPRESSION filters. */ - isBroadcastEnabled: z.boolean().optional(), + // Ad hoc conditions can't be referenced as variables, so broadcast is how they apply. + isBroadcastEnabled: z.literal(true), + /** + * Keeps `variableName` unique among the dashboard's variable names, which + * share the `filters=` param. Not referenceable as `$variableName`. + */ isVariableEnabled: z.literal(true), }) .strict(); From 39e085d8e91455e0fb5432b31d3da6677c82310e Mon Sep 17 00:00:00 2001 From: Aaron Knudtson <87577305+knudtty@users.noreply.github.com> Date: Fri, 13 Nov 2026 16:44:44 -0500 Subject: [PATCH 2/3] fix(app): label the ad hoc filter name field "Unique name" --- .../components/DashboardFiltersModal/AdhocFilterEditForm.tsx | 3 ++- .../components/DashboardFiltersModal/VariableNameInput.tsx | 5 ++++- .../__tests__/AdhocFilterEditForm.test.tsx | 2 ++ 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/packages/app/src/components/DashboardFiltersModal/AdhocFilterEditForm.tsx b/packages/app/src/components/DashboardFiltersModal/AdhocFilterEditForm.tsx index 8a1c88cffd..d6d6d561de 100644 --- a/packages/app/src/components/DashboardFiltersModal/AdhocFilterEditForm.tsx +++ b/packages/app/src/components/DashboardFiltersModal/AdhocFilterEditForm.tsx @@ -191,7 +191,8 @@ export const AdhocFilterEditForm = ({ ); diff --git a/packages/app/src/components/DashboardFiltersModal/VariableNameInput.tsx b/packages/app/src/components/DashboardFiltersModal/VariableNameInput.tsx index 74b5db9424..aaa72b263b 100644 --- a/packages/app/src/components/DashboardFiltersModal/VariableNameInput.tsx +++ b/packages/app/src/components/DashboardFiltersModal/VariableNameInput.tsx @@ -12,6 +12,8 @@ import { FilterFormControl } from './filterFormState'; interface VariableNameInputProps { control: FilterFormControl; otherFilters: DashboardFilter[]; + /** For filters whose name isn't a referenceable variable. */ + label?: string; /** Replaces the tooltip, for filters whose name isn't a referenceable variable. */ description?: string; } @@ -19,6 +21,7 @@ interface VariableNameInputProps { export const VariableNameInput = ({ control, otherFilters, + label = 'Variable name', description, }: VariableNameInputProps) => { const { errors } = useFormState({ control }); @@ -27,7 +30,7 @@ export const VariableNameInput = ({ return ( { it('says the name is not a referenceable variable', () => { renderForm(adhoc()); + expect(screen.getByText('Unique name')).toBeInTheDocument(); + expect(screen.queryByText('Variable name')).not.toBeInTheDocument(); expect( screen.getByText(/can't be referenced as variables/), ).toBeInTheDocument(); From 61e898ab04cffc2c251faac1b30b27d1fbfca2d5 Mon Sep 17 00:00:00 2001 From: Aaron Knudtson <87577305+knudtty@users.noreply.github.com> Date: Fri, 13 Nov 2026 16:56:50 -0500 Subject: [PATCH 3/3] refactor: share isFilterReferenceableVariable; fix ad hoc e2e specs One predicate decides whether a filter's name is a referenceable variable, used by getDashboardVariableFilters, getFilterEffect, and the filters list. Updates the ad hoc filter e2e specs for the removed broadcast checkbox and the list no longer showing an ad hoc filter's name as a variable. --- packages/app/src/DashboardFilters.tsx | 10 ++--- .../DashboardFiltersList.tsx | 7 +--- .../features/dashboard-adhoc-filters.spec.ts | 37 ++++++------------- .../src/__tests__/filters.test.ts | 17 +++++++++ packages/common-utils/src/filters.ts | 16 ++++++-- 5 files changed, 48 insertions(+), 39 deletions(-) diff --git a/packages/app/src/DashboardFilters.tsx b/packages/app/src/DashboardFilters.tsx index 56b7246232..eb78462468 100644 --- a/packages/app/src/DashboardFilters.tsx +++ b/packages/app/src/DashboardFilters.tsx @@ -6,8 +6,8 @@ import { getPendingFilterValuesVariables, isAdhocFilter, isFilterGlobalRequirement, + isFilterReferenceableVariable, isFilterRequired, - isFilterVariableEnabled, isQueryExpressionFilter, isStaticListFilter, } from '@hyperdx/common-utils/dist/filters'; @@ -73,11 +73,9 @@ export const getFilterEffect = (filter: DashboardFilter): FilterEffect => { ); } - // An ad hoc filter's variable name can't be referenced. - const variableName = - isFilterVariableEnabled(filter) && !isAdhocFilter(filter) - ? getFilterVariableName(filter) - : undefined; + const variableName = isFilterReferenceableVariable(filter) + ? getFilterVariableName(filter) + : undefined; if (variableName) { parts.push( `${parts.length > 0 ? 'a' : 'A'}vailable as variable ($${variableName})`, diff --git a/packages/app/src/components/DashboardFiltersModal/DashboardFiltersList.tsx b/packages/app/src/components/DashboardFiltersModal/DashboardFiltersList.tsx index a9215a4bb9..940e9d350b 100644 --- a/packages/app/src/components/DashboardFiltersModal/DashboardFiltersList.tsx +++ b/packages/app/src/components/DashboardFiltersModal/DashboardFiltersList.tsx @@ -1,10 +1,9 @@ import { getFilterBroadcastTarget, getFilterVariableName, - isAdhocFilter, isFilterGlobalRequirement, + isFilterReferenceableVariable, isFilterRequired, - isFilterVariableEnabled, } from '@hyperdx/common-utils/dist/filters'; import { DashboardFilter } from '@hyperdx/common-utils/dist/types'; import { @@ -163,9 +162,7 @@ export const DashboardFiltersList = ({ } const variableName = - showVariableOptions && - isFilterVariableEnabled(filter) && - !isAdhocFilter(filter) + showVariableOptions && isFilterReferenceableVariable(filter) ? getFilterVariableName(filter) : undefined; return ( diff --git a/packages/app/tests/e2e/features/dashboard-adhoc-filters.spec.ts b/packages/app/tests/e2e/features/dashboard-adhoc-filters.spec.ts index c2412e95ef..9f032a0b06 100644 --- a/packages/app/tests/e2e/features/dashboard-adhoc-filters.spec.ts +++ b/packages/app/tests/e2e/features/dashboard-adhoc-filters.spec.ts @@ -59,11 +59,13 @@ test.describe( dashboardPage.getAdhocFilterSourceTypeControl(), ).toBeVisible(); await expect(dashboardPage.getAdhocFilterSourcesInput()).toBeVisible(); - await expect(dashboardPage.broadcastFilterCheckbox).toBeChecked(); + // Always broadcasts, so the scope is shown without a checkbox. + await expect(dashboardPage.broadcastFilterCheckbox).toHaveCount(0); await expect(dashboardPage.appliesToSourceSelector).toBeVisible(); - // Always a variable, so the name is asked for rather than unlocked by - // a checkbox. await expect(dashboardPage.variableNameInput).toBeVisible(); + await expect( + form.getByText('Unique name', { exact: true }), + ).toBeVisible(); await expect(form.getByTestId('source-selector')).toHaveCount(0); await expect(form.getByText('Filter expression')).toHaveCount(0); @@ -114,7 +116,8 @@ test.describe( await expect(item).toContainText( `${DEFAULT_LOGS_SOURCE_NAME}, ${DEFAULT_TRACES_SOURCE_NAME}`, ); - await expect(item).toContainText('($conds)'); + // The name isn't a variable, so the list doesn't present it as one. + await expect(item).not.toContainText('$conds'); await expect( dashboardPage.getFilterBroadcastTarget('Conditions'), ).toHaveText( @@ -127,9 +130,8 @@ test.describe( await page.reload(); await dashboardPage.waitForLoaded(); await dashboardPage.openEditFiltersModal(); - await expect( - dashboardPage.getFilterItemByName('Conditions'), - ).toContainText('($conds)'); + await dashboardPage.openEditFilterForm('Conditions'); + await expect(dashboardPage.variableNameInput).toHaveValue('conds'); }); }); @@ -169,34 +171,19 @@ test.describe( await dashboardPage.variableNameInput.fill('where_clause'); await page.getByTestId('save-filter-button').click(); - await expect( - dashboardPage.getFilterItemByName('Conditions'), - ).toContainText('($where_clause)'); await expect( dashboardPage.getFilterBroadcastTarget('Conditions'), ).toHaveText(DEFAULT_TRACES_SOURCE_NAME); }); - await test.step('Turning broadcast off drops the broadcast attribute', async () => { - await dashboardPage.openEditFilterForm('Conditions'); - await dashboardPage.broadcastFilterCheckbox.uncheck(); - await expect(dashboardPage.appliesToSourceSelector).toHaveCount(0); - await page.getByTestId('save-filter-button').click(); - - await expect( - dashboardPage.getFilterItemByName('Conditions'), - ).toBeVisible(); - await expect( - dashboardPage.getFilterBroadcastTarget('Conditions'), - ).toHaveCount(0); - }); - await test.step('The edits survive a reload', async () => { await page.reload(); await dashboardPage.waitForLoaded(); await dashboardPage.openEditFiltersModal(); + await expect( + dashboardPage.getFilterBroadcastTarget('Conditions'), + ).toHaveText(DEFAULT_TRACES_SOURCE_NAME); await dashboardPage.openEditFilterForm('Conditions'); - await expect(dashboardPage.broadcastFilterCheckbox).not.toBeChecked(); await expect(dashboardPage.variableNameInput).toHaveValue( 'where_clause', ); diff --git a/packages/common-utils/src/__tests__/filters.test.ts b/packages/common-utils/src/__tests__/filters.test.ts index 3627004a9e..62af6a8154 100644 --- a/packages/common-utils/src/__tests__/filters.test.ts +++ b/packages/common-utils/src/__tests__/filters.test.ts @@ -20,6 +20,7 @@ import { isAdhocConditionSupported, isFilterBroadcastEnabled, isFilterGlobalRequirement, + isFilterReferenceableVariable, isFilterRequired, isFilterVariableEnabled, isQueryExpressionFilter, @@ -1740,6 +1741,22 @@ describe('filters', () => { }); }); + describe('isFilterReferenceableVariable', () => { + it.each([ + ['QUERY_EXPRESSION', true, true], + ['STATIC_LIST', true, true], + ['QUERY_EXPRESSION', false, false], + ['ADHOC', true, false], + ] as const)( + '%s with variables %s: %s', + (type, isVariableEnabled, expected) => { + expect(isFilterReferenceableVariable({ type, isVariableEnabled })).toBe( + expected, + ); + }, + ); + }); + describe('isFilterRequired', () => { it('treats a missing or zero minimum as not required', () => { expect(isFilterRequired({})).toBe(false); diff --git a/packages/common-utils/src/filters.ts b/packages/common-utils/src/filters.ts index e752d52111..2af8d3c8a8 100644 --- a/packages/common-utils/src/filters.ts +++ b/packages/common-utils/src/filters.ts @@ -865,6 +865,18 @@ export function isFilterVariableEnabled(filter: { return filter.isVariableEnabled === true; } +/** + * Whether tiles can reference a filter's selection as `$variableName`. An ad + * hoc filter is variable-enabled only so its name stays unique; the name just + * keys its conditions. + */ +export function isFilterReferenceableVariable(filter: { + type?: DashboardFilterKind; + isVariableEnabled?: boolean; +}): boolean { + return isFilterVariableEnabled(filter) && filter.type !== 'ADHOC'; +} + /** * Whether a filter must have at least one value selected before the tiles it * covers will load. @@ -1285,9 +1297,7 @@ export function getDashboardVariableFilters< const takenNames = new Set(); for (const filter of filters ?? []) { - if (!isFilterVariableEnabled(filter)) continue; - // Their variable name only keys their conditions; it isn't referenceable. - if (filter.type === 'ADHOC') continue; + if (!isFilterReferenceableVariable(filter)) continue; // There shouldn't be any duplicate names, but if there are then the first one wins. const name = getFilterVariableName(filter);