From b4ac6e84f921fe2d0101d3a96dac86277a3afdea Mon Sep 17 00:00:00 2001 From: Drew Davis Date: Wed, 22 Apr 2026 07:41:10 -0400 Subject: [PATCH 1/8] refactor: Move filtersToQuery to common-utils --- packages/app/src/DashboardFilters.tsx | 2 +- .../app/src/__tests__/searchFilters.test.ts | 120 +---------------- .../src/components/DBSearchPageFilters.tsx | 2 +- .../DBSearchPageFilters/NestedFilterGroup.tsx | 3 +- .../usePresetDashboardFilters.test.tsx | 2 +- .../app/src/hooks/useDashboardFilters.tsx | 6 +- packages/app/src/searchFilters.tsx | 57 +-------- .../src/__tests__/filters.test.ts | 121 ++++++++++++++++++ packages/common-utils/src/filters.ts | 54 ++++++++ 9 files changed, 189 insertions(+), 178 deletions(-) create mode 100644 packages/common-utils/src/__tests__/filters.test.ts create mode 100644 packages/common-utils/src/filters.ts diff --git a/packages/app/src/DashboardFilters.tsx b/packages/app/src/DashboardFilters.tsx index 331984c36f..38cc9d0d72 100644 --- a/packages/app/src/DashboardFilters.tsx +++ b/packages/app/src/DashboardFilters.tsx @@ -1,9 +1,9 @@ +import { FilterState } from '@hyperdx/common-utils/dist/filters'; import { DashboardFilter } from '@hyperdx/common-utils/dist/types'; import { Group, MultiSelect } from '@mantine/core'; import { IconRefresh } from '@tabler/icons-react'; import { useDashboardFilterValues } from './hooks/useDashboardFilterValues'; -import { FilterState } from './searchFilters'; interface DashboardFilterSelectProps { filter: DashboardFilter; diff --git a/packages/app/src/__tests__/searchFilters.test.ts b/packages/app/src/__tests__/searchFilters.test.ts index 5a001e0a70..e797ccf736 100644 --- a/packages/app/src/__tests__/searchFilters.test.ts +++ b/packages/app/src/__tests__/searchFilters.test.ts @@ -1,9 +1,9 @@ import { enableMapSet } from 'immer'; +import { filtersToQuery } from '@hyperdx/common-utils/dist/filters'; import { act, renderHook } from '@testing-library/react'; import { areFiltersEqual, - filtersToQuery, parseQuery, useSearchPageFilterState, } from '../searchFilters'; @@ -11,124 +11,6 @@ import { enableMapSet(); describe('searchFilters', () => { - describe('filtersToQuery', () => { - it('should return empty string when no filters', () => { - const filters = {}; - expect(filtersToQuery(filters)).toEqual([]); - }); - - it('should return query for one filter', () => { - const filters = { - a: { included: new Set(['b']), excluded: new Set() }, - }; - expect(filtersToQuery(filters)).toEqual([ - { type: 'sql', condition: "a IN ('b')" }, - ]); - }); - - it('should return query for multiple filters', () => { - const filters = { - a: { included: new Set(['b']), excluded: new Set() }, - c: { - included: new Set(['d', 'x']), - excluded: new Set(), - }, - }; - expect(filtersToQuery(filters)).toEqual([ - { type: 'sql', condition: "a IN ('b')" }, - { type: 'sql', condition: "c IN ('d', 'x')" }, - ]); - }); - - it('should handle excluded values', () => { - const filters = { - a: { - included: new Set(['b']), - excluded: new Set(['c']), - }, - }; - expect(filtersToQuery(filters)).toEqual([ - { type: 'sql', condition: "a IN ('b')" }, - { type: 'sql', condition: "a NOT IN ('c')" }, - ]); - }); - - it('should wrap keys with toString() when specified', () => { - const filters = { - 'json.key': { - included: new Set(['value']), - excluded: new Set(['other value']), - }, - }; - expect(filtersToQuery(filters, { stringifyKeys: true })).toEqual([ - { type: 'sql', condition: "toString(json.key) IN ('value')" }, - { type: 'sql', condition: "toString(json.key) NOT IN ('other value')" }, - ]); - }); - - it('should should handle boolean filter values', () => { - const filters = { - isRootSpan: { - included: new Set([true]), - excluded: new Set([]), - }, - another_column: { - included: new Set([]), - excluded: new Set([true, false]), - }, - }; - expect(filtersToQuery(filters)).toEqual([ - { type: 'sql', condition: 'isRootSpan IN (true)' }, - { type: 'sql', condition: 'another_column NOT IN (true, false)' }, - ]); - }); - - it('should escape single quotes in filter values', () => { - const filters = { - message: { - included: new Set(["my 'filter' key"]), - excluded: new Set(), - }, - }; - expect(filtersToQuery(filters)).toEqual([ - { - type: 'sql', - condition: "message IN ('my ''filter'' key')", - }, - ]); - }); - - it('should escape single quotes in excluded filter values', () => { - const filters = { - message: { - included: new Set(), - excluded: new Set(["it's a test"]), - }, - }; - expect(filtersToQuery(filters)).toEqual([ - { - type: 'sql', - condition: "message NOT IN ('it''s a test')", - }, - ]); - }); - - it('should escape single quotes with stringifyKeys', () => { - const filters = { - 'json.key': { - included: new Set(["value with 'quotes'"]), - excluded: new Set(), - }, - }; - expect(filtersToQuery(filters, { stringifyKeys: true })).toEqual([ - { - type: 'sql', - condition: "toString(json.key) IN ('value with ''quotes''')", - }, - ]); - }); - }); - describe('parseQuery', () => { it('empty query', () => { const result = parseQuery([]); diff --git a/packages/app/src/components/DBSearchPageFilters.tsx b/packages/app/src/components/DBSearchPageFilters.tsx index 49f0645598..2b5a541537 100644 --- a/packages/app/src/components/DBSearchPageFilters.tsx +++ b/packages/app/src/components/DBSearchPageFilters.tsx @@ -4,6 +4,7 @@ import { TableMetadata, tcFromSource, } from '@hyperdx/common-utils/dist/core/metadata'; +import { FilterState } from '@hyperdx/common-utils/dist/filters'; import { BuilderChartConfigWithDateRange, SourceKind, @@ -60,7 +61,6 @@ import { useMetadataWithSettings } from '@/hooks/useMetadata'; import useResizable from '@/hooks/useResizable'; import { usePinnedFiltersApi } from '@/pinnedFilters'; import { - type FilterState, FilterStateHook, IS_ROOT_SPAN_COLUMN_NAME, usePinnedFilters, diff --git a/packages/app/src/components/DBSearchPageFilters/NestedFilterGroup.tsx b/packages/app/src/components/DBSearchPageFilters/NestedFilterGroup.tsx index 027aeee10e..8a3106b5cc 100644 --- a/packages/app/src/components/DBSearchPageFilters/NestedFilterGroup.tsx +++ b/packages/app/src/components/DBSearchPageFilters/NestedFilterGroup.tsx @@ -1,9 +1,8 @@ import { useMemo, useRef, useState } from 'react'; +import { FilterState } from '@hyperdx/common-utils/dist/filters'; import { Accordion, Group, Text, Tooltip, UnstyledButton } from '@mantine/core'; import { useVirtualizer } from '@tanstack/react-virtual'; -import { FilterState } from '@/searchFilters'; - import { FilterGroup } from '../DBSearchPageFilters'; import classes from '../../../styles/SearchPage.module.scss'; diff --git a/packages/app/src/hooks/__tests__/usePresetDashboardFilters.test.tsx b/packages/app/src/hooks/__tests__/usePresetDashboardFilters.test.tsx index 4c9ccbce58..85d27fe04b 100644 --- a/packages/app/src/hooks/__tests__/usePresetDashboardFilters.test.tsx +++ b/packages/app/src/hooks/__tests__/usePresetDashboardFilters.test.tsx @@ -1,3 +1,4 @@ +import { FilterState } from '@hyperdx/common-utils/dist/filters'; import { DashboardFilter, Filter, @@ -7,7 +8,6 @@ import { import { act, renderHook } from '@testing-library/react'; import api from '@/api'; -import { FilterState } from '@/searchFilters'; import useDashboardFilters from '../useDashboardFilters'; import usePresetDashboardFilters from '../usePresetDashboardFilters'; diff --git a/packages/app/src/hooks/useDashboardFilters.tsx b/packages/app/src/hooks/useDashboardFilters.tsx index 7cdcf7f0ee..4270016fe5 100644 --- a/packages/app/src/hooks/useDashboardFilters.tsx +++ b/packages/app/src/hooks/useDashboardFilters.tsx @@ -1,8 +1,12 @@ import { useCallback, useMemo } from 'react'; import { useQueryState } from 'nuqs'; +import { + FilterState, + filtersToQuery, +} from '@hyperdx/common-utils/dist/filters'; import { DashboardFilter, Filter } from '@hyperdx/common-utils/dist/types'; -import { FilterState, filtersToQuery, parseQuery } from '@/searchFilters'; +import { parseQuery } from '@/searchFilters'; import { parseAsJsonEncoded } from '@/utils/queryParsers'; const filterQueriesParser = parseAsJsonEncoded(); diff --git a/packages/app/src/searchFilters.tsx b/packages/app/src/searchFilters.tsx index 4deb6e07f7..3f9db3cc1a 100644 --- a/packages/app/src/searchFilters.tsx +++ b/packages/app/src/searchFilters.tsx @@ -1,5 +1,9 @@ import { useCallback, useEffect, useMemo, useRef, useState } from 'react'; import produce from 'immer'; +import { + type FilterState, + filtersToQuery, +} from '@hyperdx/common-utils/dist/filters'; import type { Filter } from '@hyperdx/common-utils/dist/types'; import { usePinnedFiltersApi, useUpdatePinnedFilters } from './pinnedFilters'; @@ -7,59 +11,6 @@ import { useLocalStorage } from './utils'; export const IS_ROOT_SPAN_COLUMN_NAME = 'isRootSpan'; -export type FilterState = { - [key: string]: { - included: Set; - excluded: Set; - range?: { min: number; max: number }; // For BETWEEN conditions - }; -}; - -export const filtersToQuery = ( - filters: FilterState, - { stringifyKeys = false }: { stringifyKeys?: boolean } = {}, -): Filter[] => { - return Object.entries(filters) - .filter( - ([_, values]) => - values.included.size > 0 || - values.excluded.size > 0 || - values.range != null, - ) - .flatMap(([key, values]) => { - const conditions = []; - const actualKey = stringifyKeys ? `toString(${key})` : key; - - if (values.included.size > 0) { - conditions.push({ - type: 'sql' as const, - condition: `${actualKey} IN (${Array.from(values.included) - .map(v => - typeof v === 'string' ? `'${v.replace(/'/g, "''")}'` : v, - ) - .join(', ')})`, - }); - } - if (values.excluded.size > 0) { - conditions.push({ - type: 'sql' as const, - condition: `${actualKey} NOT IN (${Array.from(values.excluded) - .map(v => - typeof v === 'string' ? `'${v.replace(/'/g, "''")}'` : v, - ) - .join(', ')})`, - }); - } - if (values.range != null) { - conditions.push({ - type: 'sql' as const, - condition: `${actualKey} BETWEEN ${values.range.min} AND ${values.range.max}`, - }); - } - return conditions; - }); -}; - export const areFiltersEqual = (a: FilterState, b: FilterState) => { const aKeys = Object.keys(a); const bKeys = Object.keys(b); diff --git a/packages/common-utils/src/__tests__/filters.test.ts b/packages/common-utils/src/__tests__/filters.test.ts new file mode 100644 index 0000000000..743ca2fbff --- /dev/null +++ b/packages/common-utils/src/__tests__/filters.test.ts @@ -0,0 +1,121 @@ +import { filtersToQuery } from '@/filters'; + +describe('searchFilters', () => { + describe('filtersToQuery', () => { + it('should return empty string when no filters', () => { + const filters = {}; + expect(filtersToQuery(filters)).toEqual([]); + }); + + it('should return query for one filter', () => { + const filters = { + a: { included: new Set(['b']), excluded: new Set() }, + }; + expect(filtersToQuery(filters)).toEqual([ + { type: 'sql', condition: "a IN ('b')" }, + ]); + }); + + it('should return query for multiple filters', () => { + const filters = { + a: { included: new Set(['b']), excluded: new Set() }, + c: { + included: new Set(['d', 'x']), + excluded: new Set(), + }, + }; + expect(filtersToQuery(filters)).toEqual([ + { type: 'sql', condition: "a IN ('b')" }, + { type: 'sql', condition: "c IN ('d', 'x')" }, + ]); + }); + + it('should handle excluded values', () => { + const filters = { + a: { + included: new Set(['b']), + excluded: new Set(['c']), + }, + }; + expect(filtersToQuery(filters)).toEqual([ + { type: 'sql', condition: "a IN ('b')" }, + { type: 'sql', condition: "a NOT IN ('c')" }, + ]); + }); + + it('should wrap keys with toString() when specified', () => { + const filters = { + 'json.key': { + included: new Set(['value']), + excluded: new Set(['other value']), + }, + }; + expect(filtersToQuery(filters, { stringifyKeys: true })).toEqual([ + { type: 'sql', condition: "toString(json.key) IN ('value')" }, + { type: 'sql', condition: "toString(json.key) NOT IN ('other value')" }, + ]); + }); + + it('should should handle boolean filter values', () => { + const filters = { + isRootSpan: { + included: new Set([true]), + excluded: new Set([]), + }, + another_column: { + included: new Set([]), + excluded: new Set([true, false]), + }, + }; + expect(filtersToQuery(filters)).toEqual([ + { type: 'sql', condition: 'isRootSpan IN (true)' }, + { type: 'sql', condition: 'another_column NOT IN (true, false)' }, + ]); + }); + + it('should escape single quotes in filter values', () => { + const filters = { + message: { + included: new Set(["my 'filter' key"]), + excluded: new Set(), + }, + }; + expect(filtersToQuery(filters)).toEqual([ + { + type: 'sql', + condition: "message IN ('my ''filter'' key')", + }, + ]); + }); + + it('should escape single quotes in excluded filter values', () => { + const filters = { + message: { + included: new Set(), + excluded: new Set(["it's a test"]), + }, + }; + expect(filtersToQuery(filters)).toEqual([ + { + type: 'sql', + condition: "message NOT IN ('it''s a test')", + }, + ]); + }); + + it('should escape single quotes with stringifyKeys', () => { + const filters = { + 'json.key': { + included: new Set(["value with 'quotes'"]), + excluded: new Set(), + }, + }; + expect(filtersToQuery(filters, { stringifyKeys: true })).toEqual([ + { + type: 'sql', + condition: "toString(json.key) IN ('value with ''quotes''')", + }, + ]); + }); + }); +}); diff --git a/packages/common-utils/src/filters.ts b/packages/common-utils/src/filters.ts new file mode 100644 index 0000000000..29ca8665cc --- /dev/null +++ b/packages/common-utils/src/filters.ts @@ -0,0 +1,54 @@ +import { Filter } from '@/types'; + +export type FilterState = { + [key: string]: { + included: Set; + excluded: Set; + range?: { min: number; max: number }; // For BETWEEN conditions + }; +}; + +export const filtersToQuery = ( + filters: FilterState, + { stringifyKeys = false }: { stringifyKeys?: boolean } = {}, +): Filter[] => { + return Object.entries(filters) + .filter( + ([_, values]) => + values.included.size > 0 || + values.excluded.size > 0 || + values.range != null, + ) + .flatMap(([key, values]) => { + const conditions: Filter[] = []; + const actualKey = stringifyKeys ? `toString(${key})` : key; + + if (values.included.size > 0) { + conditions.push({ + type: 'sql' as const, + condition: `${actualKey} IN (${Array.from(values.included) + .map(v => + typeof v === 'string' ? `'${v.replace(/'/g, "''")}'` : v, + ) + .join(', ')})`, + }); + } + if (values.excluded.size > 0) { + conditions.push({ + type: 'sql' as const, + condition: `${actualKey} NOT IN (${Array.from(values.excluded) + .map(v => + typeof v === 'string' ? `'${v.replace(/'/g, "''")}'` : v, + ) + .join(', ')})`, + }); + } + if (values.range != null) { + conditions.push({ + type: 'sql' as const, + condition: `${actualKey} BETWEEN ${values.range.min} AND ${values.range.max}`, + }); + } + return conditions; + }); +}; From f24c74067e08863ff8c7fdf837a883111063fa0e Mon Sep 17 00:00:00 2001 From: Drew Davis Date: Wed, 22 Apr 2026 08:26:53 -0400 Subject: [PATCH 2/8] feat: Add filter templating to custom dashboard on-click --- .changeset/shaggy-experts-collect.md | 6 + packages/app/src/DBDashboardPage.tsx | 44 ++- .../OnClickForm/FilterTemplateList.tsx | 82 +++++ .../OnClickForm/OnClickDrawer.tsx | 88 ++++- .../OnClickTargetInputControlled.tsx | 14 +- .../usePresetDashboardFilters.test.tsx | 1 + .../app/src/hooks/useDashboardFilters.tsx | 51 ++- .../e2e/components/ChartEditorComponent.ts | 43 +++ .../features/dashboard-table-linking.spec.ts | 227 ++++++++++++ .../tests/e2e/page-objects/DashboardPage.ts | 19 + .../src/core/__tests__/linkUrlBuilder.test.ts | 346 ++++++++++++++++++ .../common-utils/src/core/linkUrlBuilder.ts | 64 +++- packages/common-utils/src/types.ts | 9 + 13 files changed, 959 insertions(+), 35 deletions(-) create mode 100644 .changeset/shaggy-experts-collect.md create mode 100644 packages/app/src/components/DBEditTimeChartForm/OnClickForm/FilterTemplateList.tsx diff --git a/.changeset/shaggy-experts-collect.md b/.changeset/shaggy-experts-collect.md new file mode 100644 index 0000000000..123e15f84a --- /dev/null +++ b/.changeset/shaggy-experts-collect.md @@ -0,0 +1,6 @@ +--- +"@hyperdx/common-utils": patch +"@hyperdx/app": patch +--- + +feat: Add filter templating to custom dashboard on-click diff --git a/packages/app/src/DBDashboardPage.tsx b/packages/app/src/DBDashboardPage.tsx index 4370020943..272c7b3aa7 100644 --- a/packages/app/src/DBDashboardPage.tsx +++ b/packages/app/src/DBDashboardPage.tsx @@ -47,6 +47,7 @@ import { } from '@hyperdx/common-utils/dist/types'; import { ActionIcon, + Alert, Anchor, Box, Breadcrumbs, @@ -64,6 +65,7 @@ import { import { useHotkeys } from '@mantine/hooks'; import { notifications } from '@mantine/notifications'; import { + IconAlertTriangle, IconArrowsMaximize, IconBell, IconChartBar, @@ -1088,8 +1090,29 @@ function DBDashboardPage({ presetConfig }: { presetConfig?: Dashboard }) { const [showFiltersModal, setShowFiltersModal] = useState(false); const filters = dashboard?.filters ?? []; - const { filterValues, setFilterValue, filterQueries, setFilterQueries } = - useDashboardFilters(filters); + const { + filterValues, + setFilterValue, + filterQueries, + setFilterQueries, + ignoredFilterExpressions, + } = useDashboardFilters(filters); + + // Warn when the URL has filter values that don't correspond to any declared + // dashboard filter — they'd otherwise be silently dropped, and users who + // arrive via a shared link, bookmark, or onClick action might not notice. + // Only consider URL filters ignored once the dashboard has finished loading + // so we don't flash the banner before `dashboard.filters` is available. + const dashboardReady = + !!dashboard?.id && + router.isReady && + (isLocalDashboard || !isFetchingDashboard); + const [dismissedIgnoredFilters, setDismissedIgnoredFilters] = + useState(false); + const shouldShowIgnoredFiltersBanner = + dashboardReady && + ignoredFilterExpressions.length > 0 && + !dismissedIgnoredFilters; const handleSaveFilter = (filter: DashboardFilter) => { if (!dashboard) return; @@ -2137,6 +2160,23 @@ function DBDashboardPage({ presetConfig }: { presetConfig?: Dashboard }) { Run + {shouldShowIgnoredFiltersBanner && ( + } + title="Some filters could not be applied" + data-testid="ignored-url-filters-banner" + withCloseButton + closeButtonLabel="Dismiss" + onClose={() => setDismissedIgnoredFilters(true)} + > + No dashboard filter(s) found for{' '} + {ignoredFilterExpressions.length === 1 ? 'expression' : 'expressions'}{' '} + in the URL: {ignoredFilterExpressions.join(', ')}. Add a filter with a + matching expression to apply these filters. + + )} + Filters + + Enter an expression (e.g. a column name) and a template for its value. + + + {filters.map((filter, i) => ( + + + + remove(i)} + mt={3} + data-testid="onclick-filter-remove-button" + > + + + + ))} + + + + ); +} diff --git a/packages/app/src/components/DBEditTimeChartForm/OnClickForm/OnClickDrawer.tsx b/packages/app/src/components/DBEditTimeChartForm/OnClickForm/OnClickDrawer.tsx index c92f5af652..5fe005c7b6 100644 --- a/packages/app/src/components/DBEditTimeChartForm/OnClickForm/OnClickDrawer.tsx +++ b/packages/app/src/components/DBEditTimeChartForm/OnClickForm/OnClickDrawer.tsx @@ -1,8 +1,18 @@ import { useCallback, useEffect, useMemo } from 'react'; -import { Controller, useForm, useWatch } from 'react-hook-form'; +import { + Controller, + useForm, + UseFormGetValues, + UseFormSetValue, + useWatch, +} from 'react-hook-form'; import { zodResolver } from '@hookform/resolvers/zod'; import { validateOnClickTemplate } from '@hyperdx/common-utils/dist/core/linkUrlBuilder'; -import { isSearchableSource, OnClick } from '@hyperdx/common-utils/dist/types'; +import { + isSearchableSource, + OnClick, + OnClickTarget, +} from '@hyperdx/common-utils/dist/types'; import { Box, Button, @@ -20,6 +30,7 @@ import SearchWhereInput from '@/components/SearchInput/SearchWhereInput'; import { useDashboards } from '@/dashboard'; import { useSources } from '@/source'; +import { FilterTemplateList } from './FilterTemplateList'; import { OnClickTargetInputControlled } from './OnClickTargetInputControlled'; import { DrawerControl, @@ -53,6 +64,8 @@ function SearchOnClickFields({ control }: { control: DrawerControl }) { objectType="source" /> + + ; + getValues: UseFormGetValues; +}) { const { data: dashboards } = useDashboards(); const dashboardOptions = useMemo(() => { return dashboards?.map(dashboard => ({ @@ -81,6 +102,31 @@ function DashboardOnClickFields({ control }: { control: DrawerControl }) { })); }, [dashboards]); + // When the target dashboard changes, create empty filter templates + // for each of the target dashboard's existing filters + // (if the current templates are all empty). + const handleTargetChange = useCallback( + (target: OnClickTarget) => { + if (target.mode !== 'id') return; + const selected = dashboards?.find(d => d.id === target.id); + const dashboardFilters = selected?.filters ?? []; + + const currentFilters = getValues('onClick.filters') ?? []; + const allTemplatesEmpty = currentFilters.every(f => f.template === ''); + if (!allTemplatesEmpty) return; + + setValue( + 'onClick.filters', + dashboardFilters.map(f => ({ + kind: 'expressionTemplate' as const, + expression: f.expression, + template: '', + })), + ); + }, + [dashboards, setValue, getValues], + ); + return ( @@ -91,8 +137,11 @@ function DashboardOnClickFields({ control }: { control: DrawerControl }) { control={control} options={dashboardOptions} objectType="dashboard" + onTargetChange={handleTargetChange} /> + + ; + getValues: UseFormGetValues; +}) { const onClick = useWatch({ control, name: 'onClick' }); if (onClick?.type === 'search') { return ; } else if (onClick?.type === 'dashboard') { - return ; + return ( + + ); } return ( @@ -147,10 +210,11 @@ export default function OnClickDrawer({ [value], ); - const { control, handleSubmit, reset, setValue } = useForm({ - defaultValues: appliedDefaults, - resolver: zodResolver(DrawerSchema), - }); + const { control, handleSubmit, reset, setValue, getValues } = + useForm({ + defaultValues: appliedDefaults, + resolver: zodResolver(DrawerSchema), + }); // Whenever the drawer is (re)opened with a fresh value from the parent, // sync the local form to that value. Reopening after a cancel should not @@ -239,7 +303,11 @@ export default function OnClickDrawer({ )} /> - + diff --git a/packages/app/src/components/DBEditTimeChartForm/OnClickForm/OnClickTargetInputControlled.tsx b/packages/app/src/components/DBEditTimeChartForm/OnClickForm/OnClickTargetInputControlled.tsx index 94c5e25de8..d516c442b1 100644 --- a/packages/app/src/components/DBEditTimeChartForm/OnClickForm/OnClickTargetInputControlled.tsx +++ b/packages/app/src/components/DBEditTimeChartForm/OnClickForm/OnClickTargetInputControlled.tsx @@ -1,5 +1,6 @@ import { useMemo } from 'react'; import { Controller } from 'react-hook-form'; +import { OnClickTarget } from '@hyperdx/common-utils/dist/types'; import { Select } from '@mantine/core'; import { TextInputControlled } from '@/components/InputControlled'; @@ -13,10 +14,12 @@ export function OnClickTargetInputControlled({ control, options, objectType, + onTargetChange, }: { control: DrawerControl; options: { label: string; value: string }[] | undefined; objectType: 'source' | 'dashboard'; + onTargetChange?: (target: OnClickTarget) => void; }) { const optionsWithTemplate = useMemo(() => { return [ @@ -76,11 +79,12 @@ export function OnClickTargetInputControlled({ : undefined } onChange={value => { - if (value === TEMPLATE_SELECT_VALUE) { - field.onChange({ mode: 'template', template: '' }); - } else { - field.onChange({ mode: 'id', id: value ?? '' }); - } + const newTarget: OnClickTarget = + value === TEMPLATE_SELECT_VALUE + ? { mode: 'template', template: '' } + : { mode: 'id', id: value ?? '' }; + field.onChange(newTarget); + onTargetChange?.(newTarget); }} /> {field.value?.mode === 'template' && ( diff --git a/packages/app/src/hooks/__tests__/usePresetDashboardFilters.test.tsx b/packages/app/src/hooks/__tests__/usePresetDashboardFilters.test.tsx index 85d27fe04b..377cb45f16 100644 --- a/packages/app/src/hooks/__tests__/usePresetDashboardFilters.test.tsx +++ b/packages/app/src/hooks/__tests__/usePresetDashboardFilters.test.tsx @@ -109,6 +109,7 @@ describe('usePresetDashboardFilters', () => { setFilterValue: mockSetFilterValue, filterQueries: mockFilterQueries, setFilterQueries: jest.fn(), + ignoredFilterExpressions: [], }); }); diff --git a/packages/app/src/hooks/useDashboardFilters.tsx b/packages/app/src/hooks/useDashboardFilters.tsx index 4270016fe5..e1b03c6c40 100644 --- a/packages/app/src/hooks/useDashboardFilters.tsx +++ b/packages/app/src/hooks/useDashboardFilters.tsx @@ -39,33 +39,50 @@ const useDashboardFilters = (filters: DashboardFilter[]) => { [setFilterQueries], ); - const { valuesForExistingFilters, queriesForExistingFilters } = - useMemo(() => { - const { filters: parsedFilters } = parseQuery(filterQueries ?? []); - const valuesForExistingFilters: FilterState = {}; + const { + valuesForExistingFilters, + queriesForExistingFilters, + ignoredExpressions, + } = useMemo(() => { + const { filters: parsedFilters } = parseQuery(filterQueries ?? []); + const valuesForExistingFilters: FilterState = {}; + const knownExpressions = new Set(filters.map(f => f.expression)); + const ignored: string[] = []; - for (const { expression } of filters) { - if (expression in parsedFilters) { - valuesForExistingFilters[expression] = parsedFilters[expression]; - } + for (const { expression } of filters) { + if (expression in parsedFilters) { + valuesForExistingFilters[expression] = parsedFilters[expression]; + } + } + for (const key of Object.keys(parsedFilters)) { + if (!knownExpressions.has(key)) { + ignored.push(key); } + } - return { + return { + valuesForExistingFilters, + queriesForExistingFilters: filtersToQuery( valuesForExistingFilters, - queriesForExistingFilters: filtersToQuery( - valuesForExistingFilters, - // Wrap keys in `toString()` to support JSON/Dynamic-type columns. - // All keys can be stringified, since filter select values are stringified as well. - { stringifyKeys: true }, - ), - }; - }, [filterQueries, filters]); + // Wrap keys in `toString()` to support JSON/Dynamic-type columns. + // All keys can be stringified, since filter select values are stringified as well. + { stringifyKeys: true }, + ), + ignoredExpressions: ignored, + }; + }, [filterQueries, filters]); return { filterValues: valuesForExistingFilters, filterQueries: queriesForExistingFilters, setFilterValue, setFilterQueries, + /** + * Expressions parsed from the URL `filters=` param that don't correspond + * to any of this dashboard's declared filters — i.e., values that would + * be silently dropped. Callers can surface a warning. + */ + ignoredFilterExpressions: ignoredExpressions, }; }; diff --git a/packages/app/tests/e2e/components/ChartEditorComponent.ts b/packages/app/tests/e2e/components/ChartEditorComponent.ts index cd9124e170..27a65dfc2f 100644 --- a/packages/app/tests/e2e/components/ChartEditorComponent.ts +++ b/packages/app/tests/e2e/components/ChartEditorComponent.ts @@ -385,6 +385,49 @@ export class ChartEditorComponent { await this.rowClickDrawer.waitFor({ state: 'hidden', timeout: 5000 }); } + /** + * Add a row of filter templates to the Row Click drawer by clicking + * "Add filter" and filling the expression and template inputs for the + * newly-added row (placed at position `index`). + */ + async addOnClickFilterTemplate( + index: number, + expression: string, + template: string, + ) { + await this.rowClickDrawer + .getByRole('button', { name: 'Add filter' }) + .click(); + await this.rowClickDrawer + .getByTestId('onclick-filter-expression-input') + .nth(index) + .fill(expression); + await this.rowClickDrawer + .getByTestId('onclick-filter-template-input') + .nth(index) + .fill(template); + } + + /** + * Read the current value of the expression input for the filter at + * position `index` within the Row Click drawer. + */ + onClickFilterExpressionInput(index: number) { + return this.rowClickDrawer + .getByTestId('onclick-filter-expression-input') + .nth(index); + } + + /** + * Read the current value of the template input for the filter at + * position `index` within the Row Click drawer. + */ + onClickFilterTemplateInput(index: number) { + return this.rowClickDrawer + .getByTestId('onclick-filter-template-input') + .nth(index); + } + get rowClickDrawer() { return this.page.getByTestId('onclick-drawer'); } diff --git a/packages/app/tests/e2e/features/dashboard-table-linking.spec.ts b/packages/app/tests/e2e/features/dashboard-table-linking.spec.ts index 75c623753a..8bee9c3d12 100644 --- a/packages/app/tests/e2e/features/dashboard-table-linking.spec.ts +++ b/packages/app/tests/e2e/features/dashboard-table-linking.spec.ts @@ -513,5 +513,232 @@ test.describe( await expect(dashboardPage.getLinkErrorNotification()).toBeHidden(); }); }); + + test('Search mode: filter templates render into the /search URL filters param', async ({ + page, + }) => { + const ts = Date.now(); + const logSources = await getSources(page, 'log'); + const logsSource = logSources.find( + (s: { name: string }) => s.name === DEFAULT_LOGS_SOURCE_NAME, + ); + expect(logsSource).toBeDefined(); + const logsSourceId: string = logsSource.id; + + await test.step('Configure Search-mode row click with a filter template', async () => { + await addTableTile(`E2E Search Filter ${ts}`); + await dashboardPage.chartEditor.openRowClickDrawer(); + await dashboardPage.chartEditor.setRowClickMode('Search'); + await dashboardPage.chartEditor.fillRowClickTemplate( + DEFAULT_LOGS_SOURCE_NAME, + ); + await dashboardPage.chartEditor.addOnClickFilterTemplate( + 0, + 'ServiceName', + '{{ServiceName}}', + ); + await dashboardPage.chartEditor.applyRowClickDrawer(); + await dashboardPage.saveTile(); + }); + + await test.step('Set dashboard time range to Last 6 hours', async () => { + await dashboardPage.timePicker.selectRelativeTime('Last 6 hours'); + }); + + await dashboardPage.waitForTableTileRows(0); + // ServiceName is the second column in the rendered table (count is col 0). + const serviceName = await dashboardPage.getFirstTableRowValue(0, 1); + expect(serviceName.length).toBeGreaterThan(0); + + await test.step('Click first table row', async () => { + await dashboardPage.clickFirstTableRow(0); + }); + + await test.step('Verify /search URL has the rendered filter template in filters param', async () => { + await expect(page).toHaveURL(/\/search\?/, { timeout: 10000 }); + const url = new URL(page.url()); + expect(url.searchParams.get('source')).toBe(logsSourceId); + const filtersRaw = url.searchParams.get('filters'); + expect(filtersRaw).not.toBeNull(); + const filters = JSON.parse(filtersRaw!); + expect(filters).toEqual([ + { + type: 'sql', + condition: `ServiceName IN ('${serviceName}')`, + }, + ]); + }); + + await test.step('Verify search page reflects the selected source', async () => { + await expect(searchPage.currentSource).toHaveValue( + DEFAULT_LOGS_SOURCE_NAME, + { timeout: 10000 }, + ); + }); + }); + + test('Dashboard mode: filter templates render into the destination dashboard URL filters param', async ({ + page, + }) => { + const ts = Date.now(); + const targetDashboardName = `E2E Filter Target ${ts}`; + let targetDashboardId = ''; + + await test.step('Create the target dashboard with a declared filter matching ServiceName', async () => { + // beforeEach already created a dashboard; repurpose it as the target. + await dashboardPage.editDashboardName(targetDashboardName); + await dashboardPage.addTile(); + await dashboardPage.chartEditor.createBasicChart( + `Filter Target Tile ${ts}`, + ); + targetDashboardId = dashboardPage.getCurrentDashboardId(); + // Declare a dashboard filter for ServiceName so the ignored-filters + // banner does NOT appear after navigation. + await dashboardPage.openEditFiltersModal(); + await dashboardPage.addFilterToDashboard( + 'Service Filter', + DEFAULT_LOGS_SOURCE_NAME, + 'ServiceName', + ); + await dashboardPage.closeFiltersModal(); + }); + + await test.step('Create source dashboard with Dashboard-mode tile using a filter template', async () => { + await dashboardPage.goto(); + await dashboardPage.createNewDashboard(); + await addTableTile(`E2E Dashboard Filter ${ts}`); + await dashboardPage.chartEditor.openRowClickDrawer(); + await dashboardPage.chartEditor.setRowClickMode('Dashboard'); + // Selecting the target dashboard auto-populates a filter row per + // declared DashboardFilter (expression filled, template empty). + // Just fill the template of the auto-populated ServiceName row. + await dashboardPage.chartEditor.selectRowClickTarget( + targetDashboardName, + ); + await expect( + dashboardPage.chartEditor.onClickFilterExpressionInput(0), + ).toHaveValue('ServiceName'); + await dashboardPage.chartEditor + .onClickFilterTemplateInput(0) + .fill('{{ServiceName}}'); + await dashboardPage.chartEditor.applyRowClickDrawer(); + await dashboardPage.saveTile(); + }); + + await test.step('Set dashboard time range to Last 6 hours', async () => { + await dashboardPage.timePicker.selectRelativeTime('Last 6 hours'); + }); + + await dashboardPage.waitForTableTileRows(0); + const serviceName = await dashboardPage.getFirstTableRowValue(0, 1); + expect(serviceName.length).toBeGreaterThan(0); + + await test.step('Click first table row', async () => { + await dashboardPage.clickFirstTableRow(0); + }); + + await test.step('Verify navigated to target dashboard with rendered filter in URL', async () => { + await expect(page).toHaveURL( + new RegExp(`/dashboards/${targetDashboardId}`), + { timeout: 10000 }, + ); + const url = new URL(page.url()); + expect(url.pathname).toBe(`/dashboards/${targetDashboardId}`); + const filtersRaw = url.searchParams.get('filters'); + expect(filtersRaw).not.toBeNull(); + const filters = JSON.parse(filtersRaw!); + expect(filters).toEqual([ + { + type: 'sql', + condition: `ServiceName IN ('${serviceName}')`, + }, + ]); + }); + + await test.step("Verify target dashboard's heading is visible", async () => { + await expect( + dashboardPage.getDashboardHeading(targetDashboardName), + ).toBeVisible({ timeout: 10000 }); + }); + + await test.step('Verify no Link error notification appeared', async () => { + await expect(dashboardPage.getLinkErrorNotification()).toBeHidden(); + }); + + await test.step('Verify ignored-filters banner is hidden (filter was declared on target)', async () => { + await expect(dashboardPage.ignoredUrlFiltersBanner).toBeHidden(); + }); + }); + + test('Dashboard mode: ignored-filter warning banner is dismissable', async ({ + page, + }) => { + const ts = Date.now(); + const targetDashboardName = `E2E Orphan Filter Target ${ts}`; + let targetDashboardId = ''; + + await test.step('Create the target dashboard with NO declared filters', async () => { + // beforeEach already created a dashboard; repurpose it as the target. + await dashboardPage.editDashboardName(targetDashboardName); + await dashboardPage.addTile(); + await dashboardPage.chartEditor.createBasicChart( + `Orphan Filter Tile ${ts}`, + ); + targetDashboardId = dashboardPage.getCurrentDashboardId(); + }); + + await test.step('Create source dashboard with Dashboard-mode tile using an undeclared filter expression', async () => { + await dashboardPage.goto(); + await dashboardPage.createNewDashboard(); + await addTableTile(`E2E Orphan Filter Source ${ts}`); + await dashboardPage.chartEditor.openRowClickDrawer(); + await dashboardPage.chartEditor.setRowClickMode('Dashboard'); + await dashboardPage.chartEditor.selectRowClickTarget( + targetDashboardName, + ); + // OrphanExpression is not declared as a filter on the target dashboard, + // so the warning banner should appear after navigation. + await dashboardPage.chartEditor.addOnClickFilterTemplate( + 0, + 'OrphanExpression', + '{{ServiceName}}', + ); + await dashboardPage.chartEditor.applyRowClickDrawer(); + await dashboardPage.saveTile(); + }); + + await test.step('Set dashboard time range to Last 6 hours', async () => { + await dashboardPage.timePicker.selectRelativeTime('Last 6 hours'); + }); + + await dashboardPage.waitForTableTileRows(0); + + await test.step('Click first table row', async () => { + await dashboardPage.clickFirstTableRow(0); + }); + + await test.step('Verify navigated to target dashboard with filters param in URL', async () => { + await expect(page).toHaveURL( + new RegExp(`/dashboards/${targetDashboardId}`), + { timeout: 10000 }, + ); + const url = new URL(page.url()); + expect(url.searchParams.get('filters')).not.toBeNull(); + }); + + await test.step('Verify ignored-filters banner is visible and mentions the orphan expression', async () => { + await expect(dashboardPage.ignoredUrlFiltersBanner).toBeVisible({ + timeout: 10000, + }); + await expect(dashboardPage.ignoredUrlFiltersBanner).toContainText( + 'OrphanExpression', + ); + }); + + await test.step('Dismiss the banner and verify it disappears', async () => { + await dashboardPage.dismissIgnoredUrlFiltersBanner(); + await expect(dashboardPage.ignoredUrlFiltersBanner).toBeHidden(); + }); + }); }, ); diff --git a/packages/app/tests/e2e/page-objects/DashboardPage.ts b/packages/app/tests/e2e/page-objects/DashboardPage.ts index 5de4dd591f..8f8e6a45b0 100644 --- a/packages/app/tests/e2e/page-objects/DashboardPage.ts +++ b/packages/app/tests/e2e/page-objects/DashboardPage.ts @@ -664,6 +664,25 @@ export class DashboardPage { .filter({ hasText: 'Link error' }); } + /** + * Banner shown on the dashboard page when the URL's `filters=` param + * references expressions that don't correspond to any declared dashboard + * filter. Users can dismiss it with the close button. + */ + get ignoredUrlFiltersBanner() { + return this.page.getByTestId('ignored-url-filters-banner'); + } + + /** + * Dismiss the ignored-URL-filters banner by clicking its close button. + * Mantine's Alert renders the close button with `aria-label="Dismiss"`. + */ + async dismissIgnoredUrlFiltersBanner() { + await this.ignoredUrlFiltersBanner + .getByRole('button', { name: 'Dismiss' }) + .click(); + } + // Getters for assertions get createButton() { diff --git a/packages/common-utils/src/core/__tests__/linkUrlBuilder.test.ts b/packages/common-utils/src/core/__tests__/linkUrlBuilder.test.ts index fa265011fb..ce9c61a526 100644 --- a/packages/common-utils/src/core/__tests__/linkUrlBuilder.test.ts +++ b/packages/common-utils/src/core/__tests__/linkUrlBuilder.test.ts @@ -190,6 +190,179 @@ describe('renderOnClickSearch', () => { "Multiple sources named 'Duplicated' — source names must be unique to use them in a link", ); }); + + it('omits the filters param when no filter templates are provided', () => { + const onClick: OnClickSearch = { + type: 'search', + target: { mode: 'id', id: 'src_1' }, + whereLanguage: 'sql', + filters: [], + }; + const result = renderOnClickSearch({ + onClick, + row: {}, + sourceIds, + sourceIdsByName, + dateRange, + }); + expect(result.ok).toBe(true); + if (result.ok) { + const params = new URLSearchParams(result.url.split('?')[1]); + expect(params.has('filters')).toBe(false); + } + }); + + it('renders a single filter template as an IN clause', () => { + const onClick: OnClickSearch = { + type: 'search', + target: { mode: 'id', id: 'src_1' }, + whereLanguage: 'sql', + filters: [ + { + kind: 'expressionTemplate', + expression: 'ServiceName', + template: '{{ServiceName}}', + }, + ], + }; + const result = renderOnClickSearch({ + onClick, + row: { ServiceName: 'MyService' }, + sourceIds, + sourceIdsByName, + dateRange, + }); + expect(result.ok).toBe(true); + if (result.ok) { + const params = new URLSearchParams(result.url.split('?')[1]); + expect(JSON.parse(params.get('filters') ?? '')).toEqual([ + { type: 'sql', condition: "ServiceName IN ('MyService')" }, + ]); + } + }); + + it('merges filter templates sharing an expression into one IN clause', () => { + const onClick: OnClickSearch = { + type: 'search', + target: { mode: 'id', id: 'src_1' }, + whereLanguage: 'sql', + filters: [ + { + kind: 'expressionTemplate', + expression: 'ServiceName', + template: '{{Service1}}', + }, + { + kind: 'expressionTemplate', + expression: 'ServiceName', + template: '{{Service2}}', + }, + ], + }; + const result = renderOnClickSearch({ + onClick, + row: { Service1: 'A', Service2: 'B' }, + sourceIds, + sourceIdsByName, + dateRange, + }); + expect(result.ok).toBe(true); + if (result.ok) { + const params = new URLSearchParams(result.url.split('?')[1]); + expect(JSON.parse(params.get('filters') ?? '')).toEqual([ + { type: 'sql', condition: "ServiceName IN ('A', 'B')" }, + ]); + } + }); + + it('emits separate filters for distinct expressions', () => { + const onClick: OnClickSearch = { + type: 'search', + target: { mode: 'id', id: 'src_1' }, + whereLanguage: 'sql', + filters: [ + { + kind: 'expressionTemplate', + expression: 'ServiceName', + template: '{{Service}}', + }, + { + kind: 'expressionTemplate', + expression: 'SeverityText', + template: '{{Severity}}', + }, + ], + }; + const result = renderOnClickSearch({ + onClick, + row: { Service: 'MyService', Severity: 'error' }, + sourceIds, + sourceIdsByName, + dateRange, + }); + expect(result.ok).toBe(true); + if (result.ok) { + const params = new URLSearchParams(result.url.split('?')[1]); + expect(JSON.parse(params.get('filters') ?? '')).toEqual([ + { type: 'sql', condition: "ServiceName IN ('MyService')" }, + { type: 'sql', condition: "SeverityText IN ('error')" }, + ]); + } + }); + + it('escapes single quotes in rendered filter values', () => { + const onClick: OnClickSearch = { + type: 'search', + target: { mode: 'id', id: 'src_1' }, + whereLanguage: 'sql', + filters: [ + { + kind: 'expressionTemplate', + expression: 'ServiceName', + template: '{{ServiceName}}', + }, + ], + }; + const result = renderOnClickSearch({ + onClick, + row: { ServiceName: "O'Malley" }, + sourceIds, + sourceIdsByName, + dateRange, + }); + expect(result.ok).toBe(true); + if (result.ok) { + const params = new URLSearchParams(result.url.split('?')[1]); + expect(JSON.parse(params.get('filters') ?? '')).toEqual([ + { type: 'sql', condition: "ServiceName IN ('O''Malley')" }, + ]); + } + }); + + it('errors when a filter template references a missing column', () => { + const onClick: OnClickSearch = { + type: 'search', + target: { mode: 'id', id: 'src_1' }, + whereLanguage: 'sql', + filters: [ + { + kind: 'expressionTemplate', + expression: 'ServiceName', + template: '{{MissingColumn}}', + }, + ], + }; + const result = renderOnClickSearch({ + onClick, + row: {}, + sourceIds, + sourceIdsByName, + dateRange, + }); + expect(result.ok).toBe(false); + if (!result.ok) + expect(result.error).toBe("Row has no column 'MissingColumn'"); + }); }); describe('renderOnClickDashboard', () => { @@ -376,6 +549,179 @@ describe('renderOnClickDashboard', () => { "Multiple dashboards named 'Duplicated' — dashboard names must be unique to use them in a link", ); }); + + it('omits the filters param when no filter templates are provided', () => { + const onClick: OnClickDashboard = { + type: 'dashboard', + target: { mode: 'id', id: 'dash_1' }, + whereLanguage: 'sql', + filters: [], + }; + const result = renderOnClickDashboard({ + onClick, + row: {}, + dashboardIds, + dashboardIdsByName, + dateRange, + }); + expect(result.ok).toBe(true); + if (result.ok) { + const params = new URLSearchParams(result.url.split('?')[1]); + expect(params.has('filters')).toBe(false); + } + }); + + it('renders a single filter template as an IN clause', () => { + const onClick: OnClickDashboard = { + type: 'dashboard', + target: { mode: 'id', id: 'dash_1' }, + whereLanguage: 'sql', + filters: [ + { + kind: 'expressionTemplate', + expression: 'ServiceName', + template: '{{ServiceName}}', + }, + ], + }; + const result = renderOnClickDashboard({ + onClick, + row: { ServiceName: 'MyService' }, + dashboardIds, + dashboardIdsByName, + dateRange, + }); + expect(result.ok).toBe(true); + if (result.ok) { + const params = new URLSearchParams(result.url.split('?')[1]); + expect(JSON.parse(params.get('filters') ?? '')).toEqual([ + { type: 'sql', condition: "ServiceName IN ('MyService')" }, + ]); + } + }); + + it('merges filter templates sharing an expression into one IN clause', () => { + const onClick: OnClickDashboard = { + type: 'dashboard', + target: { mode: 'id', id: 'dash_1' }, + whereLanguage: 'sql', + filters: [ + { + kind: 'expressionTemplate', + expression: 'ServiceName', + template: '{{Service1}}', + }, + { + kind: 'expressionTemplate', + expression: 'ServiceName', + template: '{{Service2}}', + }, + ], + }; + const result = renderOnClickDashboard({ + onClick, + row: { Service1: 'A', Service2: 'B' }, + dashboardIds, + dashboardIdsByName, + dateRange, + }); + expect(result.ok).toBe(true); + if (result.ok) { + const params = new URLSearchParams(result.url.split('?')[1]); + expect(JSON.parse(params.get('filters') ?? '')).toEqual([ + { type: 'sql', condition: "ServiceName IN ('A', 'B')" }, + ]); + } + }); + + it('emits separate filters for distinct expressions', () => { + const onClick: OnClickDashboard = { + type: 'dashboard', + target: { mode: 'id', id: 'dash_1' }, + whereLanguage: 'sql', + filters: [ + { + kind: 'expressionTemplate', + expression: 'ServiceName', + template: '{{Service}}', + }, + { + kind: 'expressionTemplate', + expression: 'SeverityText', + template: '{{Severity}}', + }, + ], + }; + const result = renderOnClickDashboard({ + onClick, + row: { Service: 'MyService', Severity: 'error' }, + dashboardIds, + dashboardIdsByName, + dateRange, + }); + expect(result.ok).toBe(true); + if (result.ok) { + const params = new URLSearchParams(result.url.split('?')[1]); + expect(JSON.parse(params.get('filters') ?? '')).toEqual([ + { type: 'sql', condition: "ServiceName IN ('MyService')" }, + { type: 'sql', condition: "SeverityText IN ('error')" }, + ]); + } + }); + + it('escapes single quotes in rendered filter values', () => { + const onClick: OnClickDashboard = { + type: 'dashboard', + target: { mode: 'id', id: 'dash_1' }, + whereLanguage: 'sql', + filters: [ + { + kind: 'expressionTemplate', + expression: 'ServiceName', + template: '{{ServiceName}}', + }, + ], + }; + const result = renderOnClickDashboard({ + onClick, + row: { ServiceName: "O'Malley" }, + dashboardIds, + dashboardIdsByName, + dateRange, + }); + expect(result.ok).toBe(true); + if (result.ok) { + const params = new URLSearchParams(result.url.split('?')[1]); + expect(JSON.parse(params.get('filters') ?? '')).toEqual([ + { type: 'sql', condition: "ServiceName IN ('O''Malley')" }, + ]); + } + }); + + it('errors when a filter template references a missing column', () => { + const onClick: OnClickDashboard = { + type: 'dashboard', + target: { mode: 'id', id: 'dash_1' }, + whereLanguage: 'sql', + filters: [ + { + kind: 'expressionTemplate', + expression: 'ServiceName', + template: '{{MissingColumn}}', + }, + ], + }; + const result = renderOnClickDashboard({ + onClick, + row: {}, + dashboardIds, + dashboardIdsByName, + dateRange, + }); + expect(result.ok).toBe(false); + if (!result.ok) + expect(result.error).toBe("Row has no column 'MissingColumn'"); + }); }); describe('validateOnClickTemplate', () => { diff --git a/packages/common-utils/src/core/linkUrlBuilder.ts b/packages/common-utils/src/core/linkUrlBuilder.ts index f026d704f6..690375aa9f 100644 --- a/packages/common-utils/src/core/linkUrlBuilder.ts +++ b/packages/common-utils/src/core/linkUrlBuilder.ts @@ -1,4 +1,11 @@ -import type { OnClick, OnClickDashboard, OnClickSearch } from '../types'; +import { FilterState, filtersToQuery } from '../filters'; +import type { + Filter, + OnClick, + OnClickDashboard, + OnClickFilterTemplate, + OnClickSearch, +} from '../types'; import { LinkTemplateError, MissingTemplateVariableError, @@ -30,6 +37,37 @@ function renderOrError( } } +/** + * Render the filter entries into `{expression} IN (v1, v2, ...)` SQL + * conditions. Entries that share the same `filter` expression are merged + * into a single IN clause so the destination sees all requested values. + * Expressions appear in the URL in order of first occurrence; values within + * a group retain their input order. + */ +function renderFilterTemplates( + templates: OnClickFilterTemplate[] | undefined, + row: Record, +): { ok: true; filters: Filter[] } | { ok: false; error: string } { + if (!templates || templates.length === 0) return { ok: true, filters: [] }; + + const state: FilterState = {}; + for (const template of templates) { + const rendered = renderOrError(template.template, row); + if (!rendered.ok) return rendered; + + if (state[template.expression]) { + state[template.expression].included.add(rendered.value); + } else { + state[template.expression] = { + included: new Set([rendered.value]), + excluded: new Set(), + }; + } + } + + return { ok: true, filters: filtersToQuery(state) }; +} + /** * Render an OnClickSearch to a URL. */ @@ -100,6 +138,14 @@ export function renderOnClickSearch({ from: String(dateRange[0].getTime()), to: String(dateRange[1].getTime()), }); + + const filterRenderResult = renderFilterTemplates(onClick.filters, row); + if (!filterRenderResult.ok) return filterRenderResult; + + if (filterRenderResult.filters.length > 0) { + params.set('filters', JSON.stringify(filterRenderResult.filters)); + } + return { ok: true, url: `/search?${params.toString()}` }; } @@ -176,6 +222,13 @@ export function renderOnClickDashboard({ to: String(dateRange[1].getTime()), }); + const filterRenderResult = renderFilterTemplates(onClick.filters, row); + if (!filterRenderResult.ok) return filterRenderResult; + + if (filterRenderResult.filters.length > 0) { + params.set('filters', JSON.stringify(filterRenderResult.filters)); + } + return { ok: true, url: `/dashboards/${dashboardId}?${params.toString()}` }; } @@ -184,6 +237,15 @@ export function validateOnClickTemplate(onClick: OnClick) { if (onClick.target.mode === 'template') { validateTemplate(onClick.target.template); } + + if (onClick.filters) { + for (const filter of onClick.filters) { + if (filter.kind === 'expressionTemplate') { + validateTemplate(filter.template); + } + } + } + if (onClick.whereTemplate) { validateTemplate(onClick.whereTemplate); } diff --git a/packages/common-utils/src/types.ts b/packages/common-utils/src/types.ts index acab616ff9..b2646ac76d 100644 --- a/packages/common-utils/src/types.ts +++ b/packages/common-utils/src/types.ts @@ -667,6 +667,13 @@ export const NumberFormatSchema = z.object({ export type NumberFormat = z.infer; +export const OnClickFilterTemplateSchema = z.object({ + kind: z.literal('expressionTemplate'), + expression: z.string().min(1, 'Expression is required'), + template: z.string().min(1, 'Template is required'), +}); +export type OnClickFilterTemplate = z.infer; + const OnClickTargetSchema = z.discriminatedUnion('mode', [ z.object({ mode: z.literal('id'), id: z.string().min(1) }), z.object({ mode: z.literal('template'), template: z.string().min(1) }), @@ -678,6 +685,7 @@ const OnClickSearchSchema = z.object({ target: OnClickTargetSchema, whereTemplate: z.string().optional(), whereLanguage: SearchConditionLanguageSchema, + filters: z.array(OnClickFilterTemplateSchema).optional(), }); export type OnClickSearch = z.infer; @@ -686,6 +694,7 @@ export const OnClickDashboardSchema = z.object({ target: OnClickTargetSchema, whereTemplate: z.string().optional(), whereLanguage: SearchConditionLanguageSchema, + filters: z.array(OnClickFilterTemplateSchema).optional(), }); export type OnClickDashboard = z.infer; From 226d4ffb85a5fbae48ab8508fba3f018c99bcfb4 Mon Sep 17 00:00:00 2001 From: Drew Davis Date: Thu, 23 Apr 2026 16:40:12 -0400 Subject: [PATCH 3/8] fix: Prevent banner flash when navigating away from dashboard --- packages/app/src/DBDashboardPage.tsx | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/packages/app/src/DBDashboardPage.tsx b/packages/app/src/DBDashboardPage.tsx index 272c7b3aa7..5f9aa65fdc 100644 --- a/packages/app/src/DBDashboardPage.tsx +++ b/packages/app/src/DBDashboardPage.tsx @@ -1107,12 +1107,18 @@ function DBDashboardPage({ presetConfig }: { presetConfig?: Dashboard }) { !!dashboard?.id && router.isReady && (isLocalDashboard || !isFetchingDashboard); - const [dismissedIgnoredFilters, setDismissedIgnoredFilters] = + + // Latched on dashboard load only — not on every URL change — so the banner + // doesn't flash while navigating between dashboards. Known bug - when navigating + // to the current dashboard with new and invalid filters in the URL, the banner + // will not show up. + const [shouldShowIgnoredFiltersBanner, setShouldShowIgnoredFiltersBanner] = useState(false); - const shouldShowIgnoredFiltersBanner = - dashboardReady && - ignoredFilterExpressions.length > 0 && - !dismissedIgnoredFilters; + useEffect(() => { + if (!dashboardReady) return; + setShouldShowIgnoredFiltersBanner(ignoredFilterExpressions.length > 0); + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [dashboard?.id, dashboardReady]); const handleSaveFilter = (filter: DashboardFilter) => { if (!dashboard) return; @@ -2169,7 +2175,7 @@ function DBDashboardPage({ presetConfig }: { presetConfig?: Dashboard }) { data-testid="ignored-url-filters-banner" withCloseButton closeButtonLabel="Dismiss" - onClose={() => setDismissedIgnoredFilters(true)} + onClose={() => setShouldShowIgnoredFiltersBanner(false)} > No dashboard filter(s) found for{' '} {ignoredFilterExpressions.length === 1 ? 'expression' : 'expressions'}{' '} From d806f59ee2509ca967fae1ba53fa00f64be600ca Mon Sep 17 00:00:00 2001 From: Drew Davis Date: Mon, 27 Apr 2026 11:59:23 +0900 Subject: [PATCH 4/8] review: Address Claude feedback --- packages/app/src/DBDashboardPage.tsx | 17 +++++++++-------- .../OnClickForm/FilterTemplateList.tsx | 2 -- .../common-utils/src/__tests__/filters.test.ts | 2 +- 3 files changed, 10 insertions(+), 11 deletions(-) diff --git a/packages/app/src/DBDashboardPage.tsx b/packages/app/src/DBDashboardPage.tsx index 5f9aa65fdc..57033ff91a 100644 --- a/packages/app/src/DBDashboardPage.tsx +++ b/packages/app/src/DBDashboardPage.tsx @@ -1098,20 +1098,21 @@ function DBDashboardPage({ presetConfig }: { presetConfig?: Dashboard }) { ignoredFilterExpressions, } = useDashboardFilters(filters); - // Warn when the URL has filter values that don't correspond to any declared - // dashboard filter — they'd otherwise be silently dropped, and users who - // arrive via a shared link, bookmark, or onClick action might not notice. - // Only consider URL filters ignored once the dashboard has finished loading - // so we don't flash the banner before `dashboard.filters` is available. const dashboardReady = !!dashboard?.id && router.isReady && (isLocalDashboard || !isFetchingDashboard); + // Warn when the URL has filter values that don't correspond to any declared + // dashboard filter — they'd otherwise be silently dropped, and users who + // arrive via a shared link, bookmark, or onClick action might not notice. + // Only consider URL filters ignored once the dashboard has finished loading + // so we don't flash the banner before `dashboard.filters` is available. + // // Latched on dashboard load only — not on every URL change — so the banner - // doesn't flash while navigating between dashboards. Known bug - when navigating - // to the current dashboard with new and invalid filters in the URL, the banner - // will not show up. + // doesn't flash while navigating between dashboards due to nuqs state changing + // before the next router state. Known limitation - when navigating to the current + // dashboard with new and invalid filters in the URL, the banner will not show up. const [shouldShowIgnoredFiltersBanner, setShouldShowIgnoredFiltersBanner] = useState(false); useEffect(() => { diff --git a/packages/app/src/components/DBEditTimeChartForm/OnClickForm/FilterTemplateList.tsx b/packages/app/src/components/DBEditTimeChartForm/OnClickForm/FilterTemplateList.tsx index ab68d2553d..cb950c8f91 100644 --- a/packages/app/src/components/DBEditTimeChartForm/OnClickForm/FilterTemplateList.tsx +++ b/packages/app/src/components/DBEditTimeChartForm/OnClickForm/FilterTemplateList.tsx @@ -37,7 +37,6 @@ export function FilterTemplateList({ control }: { control: DrawerControl }) { control={control} name={`onClick.filters.${i}.expression` as const} placeholder="Expression" - value={filter.expression} style={{ flex: 1 }} data-testid="onclick-filter-expression-input" /> @@ -45,7 +44,6 @@ export function FilterTemplateList({ control }: { control: DrawerControl }) { control={control} name={`onClick.filters.${i}.template` as const} placeholder="Template (e.g. {{ServiceName}})" - value={filter.template} style={{ flex: 1 }} data-testid="onclick-filter-template-input" /> diff --git a/packages/common-utils/src/__tests__/filters.test.ts b/packages/common-utils/src/__tests__/filters.test.ts index 743ca2fbff..679780804e 100644 --- a/packages/common-utils/src/__tests__/filters.test.ts +++ b/packages/common-utils/src/__tests__/filters.test.ts @@ -1,6 +1,6 @@ import { filtersToQuery } from '@/filters'; -describe('searchFilters', () => { +describe('filters', () => { describe('filtersToQuery', () => { it('should return empty string when no filters', () => { const filters = {}; From 1012dc0253c8f7b1511c3e0d8931423cbdbebaf5 Mon Sep 17 00:00:00 2001 From: Drew Davis Date: Mon, 11 May 2026 11:18:01 -0400 Subject: [PATCH 5/8] review: Address P0/P1 review comments --- .../external-api/v2/utils/dashboards.ts | 2 +- packages/app/src/DBDashboardPage.tsx | 44 +++-- .../src/__tests__/filters.test.ts | 45 +++++ .../src/core/__tests__/linkUrlBuilder.test.ts | 170 ++++++++++++++++-- .../common-utils/src/core/linkUrlBuilder.ts | 10 +- packages/common-utils/src/filters.ts | 12 +- 6 files changed, 238 insertions(+), 45 deletions(-) diff --git a/packages/api/src/routers/external-api/v2/utils/dashboards.ts b/packages/api/src/routers/external-api/v2/utils/dashboards.ts index 504cf85cbe..0212f9b9d1 100644 --- a/packages/api/src/routers/external-api/v2/utils/dashboards.ts +++ b/packages/api/src/routers/external-api/v2/utils/dashboards.ts @@ -485,7 +485,7 @@ export function convertToInternalTileConfig( // Omit keys that are null/undefined, so that they're not saved as null in Mongo. // We know that the resulting object will conform to SavedChartConfig since we're just // removing null properties and anything that is null will just be undefined instead. - // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion + const strippedConfig = _.omitBy(internalConfig, _.isNil) as SavedChartConfig; return { diff --git a/packages/app/src/DBDashboardPage.tsx b/packages/app/src/DBDashboardPage.tsx index a0b5d07e5e..260fb1d26e 100644 --- a/packages/app/src/DBDashboardPage.tsx +++ b/packages/app/src/DBDashboardPage.tsx @@ -1271,9 +1271,14 @@ function DBDashboardPage({ presetConfig }: { presetConfig?: Dashboard }) { // dashboard with new and invalid filters in the URL, the banner will not show up. const [shouldShowIgnoredFiltersBanner, setShouldShowIgnoredFiltersBanner] = useState(false); + // dashboardReady will toggle when fetching dashboard due to a dashboard save - + // in this case we don't want to show the banner again. + const lastLoadedIdForBannerRef = useRef(undefined); useEffect(() => { - if (!dashboardReady) return; + if (!dashboardReady || lastLoadedIdForBannerRef.current === dashboard?.id) + return; setShouldShowIgnoredFiltersBanner(ignoredFilterExpressions.length > 0); + lastLoadedIdForBannerRef.current = dashboard?.id; // eslint-disable-next-line react-hooks/exhaustive-deps }, [dashboard?.id, dashboardReady]); @@ -2323,23 +2328,26 @@ function DBDashboardPage({ presetConfig }: { presetConfig?: Dashboard }) { Run - {shouldShowIgnoredFiltersBanner && ( - } - title="Some filters could not be applied" - data-testid="ignored-url-filters-banner" - withCloseButton - closeButtonLabel="Dismiss" - onClose={() => setShouldShowIgnoredFiltersBanner(false)} - > - No dashboard filter(s) found for{' '} - {ignoredFilterExpressions.length === 1 ? 'expression' : 'expressions'}{' '} - in the URL: {ignoredFilterExpressions.join(', ')}. Add a filter with a - matching expression to apply these filters. - - )} + {shouldShowIgnoredFiltersBanner && + ignoredFilterExpressions.length > 0 && ( + } + title="Some filters could not be applied" + data-testid="ignored-url-filters-banner" + withCloseButton + closeButtonLabel="Dismiss" + onClose={() => setShouldShowIgnoredFiltersBanner(false)} + > + No dashboard filter(s) found for{' '} + {ignoredFilterExpressions.length === 1 + ? 'expression' + : 'expressions'}{' '} + in the URL: {ignoredFilterExpressions.join(', ')}. Add a filter with + a matching expression to apply these filters. + + )} { }, ]); }); + + it('should escape backslashes in filter values', () => { + const filters = { + FilePath: { + included: new Set(['C:\\path\\to\\file']), + excluded: new Set(), + }, + }; + expect(filtersToQuery(filters)).toEqual([ + { + type: 'sql', + condition: "FilePath IN ('C:\\\\path\\\\to\\\\file')", + }, + ]); + }); + + it('should escape backslashes in excluded filter values', () => { + const filters = { + FilePath: { + included: new Set(), + excluded: new Set(['C:\\path\\to\\file']), + }, + }; + expect(filtersToQuery(filters)).toEqual([ + { + type: 'sql', + condition: "FilePath NOT IN ('C:\\\\path\\\\to\\\\file')", + }, + ]); + }); + + it('should escape backslashes before single quotes so quotes stay escaped', () => { + const filters = { + message: { + included: new Set(["a\\'b"]), + excluded: new Set(), + }, + }; + expect(filtersToQuery(filters)).toEqual([ + { + type: 'sql', + condition: "message IN ('a\\\\''b')", + }, + ]); + }); }); }); diff --git a/packages/common-utils/src/core/__tests__/linkUrlBuilder.test.ts b/packages/common-utils/src/core/__tests__/linkUrlBuilder.test.ts index ce9c61a526..d66725f5a6 100644 --- a/packages/common-utils/src/core/__tests__/linkUrlBuilder.test.ts +++ b/packages/common-utils/src/core/__tests__/linkUrlBuilder.test.ts @@ -235,9 +235,9 @@ describe('renderOnClickSearch', () => { expect(result.ok).toBe(true); if (result.ok) { const params = new URLSearchParams(result.url.split('?')[1]); - expect(JSON.parse(params.get('filters') ?? '')).toEqual([ - { type: 'sql', condition: "ServiceName IN ('MyService')" }, - ]); + expect( + JSON.parse(decodeURIComponent(params.get('filters') ?? '')), + ).toEqual([{ type: 'sql', condition: "ServiceName IN ('MyService')" }]); } }); @@ -269,9 +269,9 @@ describe('renderOnClickSearch', () => { expect(result.ok).toBe(true); if (result.ok) { const params = new URLSearchParams(result.url.split('?')[1]); - expect(JSON.parse(params.get('filters') ?? '')).toEqual([ - { type: 'sql', condition: "ServiceName IN ('A', 'B')" }, - ]); + expect( + JSON.parse(decodeURIComponent(params.get('filters') ?? '')), + ).toEqual([{ type: 'sql', condition: "ServiceName IN ('A', 'B')" }]); } }); @@ -303,7 +303,9 @@ describe('renderOnClickSearch', () => { expect(result.ok).toBe(true); if (result.ok) { const params = new URLSearchParams(result.url.split('?')[1]); - expect(JSON.parse(params.get('filters') ?? '')).toEqual([ + expect( + JSON.parse(decodeURIComponent(params.get('filters') ?? '')), + ).toEqual([ { type: 'sql', condition: "ServiceName IN ('MyService')" }, { type: 'sql', condition: "SeverityText IN ('error')" }, ]); @@ -333,8 +335,73 @@ describe('renderOnClickSearch', () => { expect(result.ok).toBe(true); if (result.ok) { const params = new URLSearchParams(result.url.split('?')[1]); - expect(JSON.parse(params.get('filters') ?? '')).toEqual([ - { type: 'sql', condition: "ServiceName IN ('O''Malley')" }, + expect( + JSON.parse(decodeURIComponent(params.get('filters') ?? '')), + ).toEqual([{ type: 'sql', condition: "ServiceName IN ('O''Malley')" }]); + } + }); + + it('escapes backslashes in rendered filter values', () => { + const onClick: OnClickSearch = { + type: 'search', + target: { mode: 'id', id: 'src_1' }, + whereLanguage: 'sql', + filters: [ + { + kind: 'expressionTemplate', + expression: 'FilePath', + template: '{{FilePath}}', + }, + ], + }; + const result = renderOnClickSearch({ + onClick, + row: { FilePath: 'C:\\path\\to\\file' }, + sourceIds, + sourceIdsByName, + dateRange, + }); + expect(result.ok).toBe(true); + if (result.ok) { + const params = new URLSearchParams(result.url.split('?')[1]); + expect( + JSON.parse(decodeURIComponent(params.get('filters') ?? '')), + ).toEqual([ + { type: 'sql', condition: "FilePath IN ('C:\\\\path\\\\to\\\\file')" }, + ]); + } + }); + + it('URL-encodes rendered filter values', () => { + const onClick: OnClickSearch = { + type: 'search', + target: { mode: 'id', id: 'src_1' }, + whereLanguage: 'sql', + filters: [ + { + kind: 'expressionTemplate', + expression: "SpanAttributes['url']", + template: '{{url}}', + }, + ], + }; + const result = renderOnClickSearch({ + onClick, + row: { url: '/users%2F42' }, + sourceIds, + sourceIdsByName, + dateRange, + }); + expect(result.ok).toBe(true); + if (result.ok) { + const params = new URLSearchParams(result.url.split('?')[1]); + expect( + JSON.parse(decodeURIComponent(params.get('filters') ?? '')), + ).toEqual([ + { + type: 'sql', + condition: "SpanAttributes['url'] IN ('/users%2F42')", + }, ]); } }); @@ -594,9 +661,9 @@ describe('renderOnClickDashboard', () => { expect(result.ok).toBe(true); if (result.ok) { const params = new URLSearchParams(result.url.split('?')[1]); - expect(JSON.parse(params.get('filters') ?? '')).toEqual([ - { type: 'sql', condition: "ServiceName IN ('MyService')" }, - ]); + expect( + JSON.parse(decodeURIComponent(params.get('filters') ?? '')), + ).toEqual([{ type: 'sql', condition: "ServiceName IN ('MyService')" }]); } }); @@ -628,9 +695,9 @@ describe('renderOnClickDashboard', () => { expect(result.ok).toBe(true); if (result.ok) { const params = new URLSearchParams(result.url.split('?')[1]); - expect(JSON.parse(params.get('filters') ?? '')).toEqual([ - { type: 'sql', condition: "ServiceName IN ('A', 'B')" }, - ]); + expect( + JSON.parse(decodeURIComponent(params.get('filters') ?? '')), + ).toEqual([{ type: 'sql', condition: "ServiceName IN ('A', 'B')" }]); } }); @@ -662,7 +729,9 @@ describe('renderOnClickDashboard', () => { expect(result.ok).toBe(true); if (result.ok) { const params = new URLSearchParams(result.url.split('?')[1]); - expect(JSON.parse(params.get('filters') ?? '')).toEqual([ + expect( + JSON.parse(decodeURIComponent(params.get('filters') ?? '')), + ).toEqual([ { type: 'sql', condition: "ServiceName IN ('MyService')" }, { type: 'sql', condition: "SeverityText IN ('error')" }, ]); @@ -692,8 +761,73 @@ describe('renderOnClickDashboard', () => { expect(result.ok).toBe(true); if (result.ok) { const params = new URLSearchParams(result.url.split('?')[1]); - expect(JSON.parse(params.get('filters') ?? '')).toEqual([ - { type: 'sql', condition: "ServiceName IN ('O''Malley')" }, + expect( + JSON.parse(decodeURIComponent(params.get('filters') ?? '')), + ).toEqual([{ type: 'sql', condition: "ServiceName IN ('O''Malley')" }]); + } + }); + + it('escapes backslashes in rendered filter values', () => { + const onClick: OnClickDashboard = { + type: 'dashboard', + target: { mode: 'id', id: 'dash_1' }, + whereLanguage: 'sql', + filters: [ + { + kind: 'expressionTemplate', + expression: 'FilePath', + template: '{{FilePath}}', + }, + ], + }; + const result = renderOnClickDashboard({ + onClick, + row: { FilePath: 'C:\\path\\to\\file' }, + dashboardIds, + dashboardIdsByName, + dateRange, + }); + expect(result.ok).toBe(true); + if (result.ok) { + const params = new URLSearchParams(result.url.split('?')[1]); + expect( + JSON.parse(decodeURIComponent(params.get('filters') ?? '')), + ).toEqual([ + { type: 'sql', condition: "FilePath IN ('C:\\\\path\\\\to\\\\file')" }, + ]); + } + }); + + it('URL-encodes rendered filter values', () => { + const onClick: OnClickDashboard = { + type: 'dashboard', + target: { mode: 'id', id: 'dash_1' }, + whereLanguage: 'sql', + filters: [ + { + kind: 'expressionTemplate', + expression: "SpanAttributes['url']", + template: '{{url}}', + }, + ], + }; + const result = renderOnClickDashboard({ + onClick, + row: { url: '/users%2F42' }, + dashboardIds, + dashboardIdsByName, + dateRange, + }); + expect(result.ok).toBe(true); + if (result.ok) { + const params = new URLSearchParams(result.url.split('?')[1]); + expect( + JSON.parse(decodeURIComponent(params.get('filters') ?? '')), + ).toEqual([ + { + type: 'sql', + condition: "SpanAttributes['url'] IN ('/users%2F42')", + }, ]); } }); diff --git a/packages/common-utils/src/core/linkUrlBuilder.ts b/packages/common-utils/src/core/linkUrlBuilder.ts index 690375aa9f..80d113e121 100644 --- a/packages/common-utils/src/core/linkUrlBuilder.ts +++ b/packages/common-utils/src/core/linkUrlBuilder.ts @@ -143,7 +143,10 @@ export function renderOnClickSearch({ if (!filterRenderResult.ok) return filterRenderResult; if (filterRenderResult.filters.length > 0) { - params.set('filters', JSON.stringify(filterRenderResult.filters)); + params.set( + 'filters', + encodeURIComponent(JSON.stringify(filterRenderResult.filters)), + ); } return { ok: true, url: `/search?${params.toString()}` }; @@ -226,7 +229,10 @@ export function renderOnClickDashboard({ if (!filterRenderResult.ok) return filterRenderResult; if (filterRenderResult.filters.length > 0) { - params.set('filters', JSON.stringify(filterRenderResult.filters)); + params.set( + 'filters', + encodeURIComponent(JSON.stringify(filterRenderResult.filters)), + ); } return { ok: true, url: `/dashboards/${dashboardId}?${params.toString()}` }; diff --git a/packages/common-utils/src/filters.ts b/packages/common-utils/src/filters.ts index 29ca8665cc..9a7ac150b1 100644 --- a/packages/common-utils/src/filters.ts +++ b/packages/common-utils/src/filters.ts @@ -8,6 +8,10 @@ export type FilterState = { }; }; +const escapeString = (s: string) => { + return s.replace(/\\/g, '\\\\').replace(/'/g, "''"); +}; + export const filtersToQuery = ( filters: FilterState, { stringifyKeys = false }: { stringifyKeys?: boolean } = {}, @@ -27,9 +31,7 @@ export const filtersToQuery = ( conditions.push({ type: 'sql' as const, condition: `${actualKey} IN (${Array.from(values.included) - .map(v => - typeof v === 'string' ? `'${v.replace(/'/g, "''")}'` : v, - ) + .map(v => (typeof v === 'string' ? `'${escapeString(v)}'` : v)) .join(', ')})`, }); } @@ -37,9 +39,7 @@ export const filtersToQuery = ( conditions.push({ type: 'sql' as const, condition: `${actualKey} NOT IN (${Array.from(values.excluded) - .map(v => - typeof v === 'string' ? `'${v.replace(/'/g, "''")}'` : v, - ) + .map(v => (typeof v === 'string' ? `'${escapeString(v)}'` : v)) .join(', ')})`, }); } From 412620c2583c9e2f48fb19bccec20fe482dc417b Mon Sep 17 00:00:00 2001 From: Drew Davis Date: Mon, 11 May 2026 11:35:20 -0400 Subject: [PATCH 6/8] test: Fix URL decoding in dashboard linking E2E tests --- .../app/tests/e2e/features/dashboard-table-linking.spec.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/app/tests/e2e/features/dashboard-table-linking.spec.ts b/packages/app/tests/e2e/features/dashboard-table-linking.spec.ts index 8bee9c3d12..09ba6b95b7 100644 --- a/packages/app/tests/e2e/features/dashboard-table-linking.spec.ts +++ b/packages/app/tests/e2e/features/dashboard-table-linking.spec.ts @@ -560,7 +560,7 @@ test.describe( expect(url.searchParams.get('source')).toBe(logsSourceId); const filtersRaw = url.searchParams.get('filters'); expect(filtersRaw).not.toBeNull(); - const filters = JSON.parse(filtersRaw!); + const filters = JSON.parse(decodeURIComponent(filtersRaw!)); expect(filters).toEqual([ { type: 'sql', @@ -646,7 +646,7 @@ test.describe( expect(url.pathname).toBe(`/dashboards/${targetDashboardId}`); const filtersRaw = url.searchParams.get('filters'); expect(filtersRaw).not.toBeNull(); - const filters = JSON.parse(filtersRaw!); + const filters = JSON.parse(decodeURIComponent(filtersRaw!)); expect(filters).toEqual([ { type: 'sql', From 8366d0bd514167b9199d50360f0ede394169b932 Mon Sep 17 00:00:00 2001 From: Drew Davis Date: Mon, 11 May 2026 11:46:19 -0400 Subject: [PATCH 7/8] review: More encoding fixes --- .../features/dashboard-table-linking.spec.ts | 8 +++--- .../src/core/__tests__/linkUrlBuilder.test.ts | 28 +++++++++++++------ .../common-utils/src/core/linkUrlBuilder.ts | 4 +-- 3 files changed, 26 insertions(+), 14 deletions(-) diff --git a/packages/app/tests/e2e/features/dashboard-table-linking.spec.ts b/packages/app/tests/e2e/features/dashboard-table-linking.spec.ts index 09ba6b95b7..084d233c36 100644 --- a/packages/app/tests/e2e/features/dashboard-table-linking.spec.ts +++ b/packages/app/tests/e2e/features/dashboard-table-linking.spec.ts @@ -81,7 +81,7 @@ test.describe( expect(url.searchParams.get('source')).toBe(logsSourceId); expect(url.searchParams.get('whereLanguage')).toBe('sql'); expect(url.searchParams.get('isLive')).toBe('false'); - expect(url.searchParams.get('where')).toBe( + expect(decodeURIComponent(url.searchParams.get('where') ?? '')).toBe( `ServiceName = '${serviceName}'`, ); const from = Number(url.searchParams.get('from')); @@ -280,7 +280,7 @@ test.describe( ); const url = new URL(page.url()); expect(url.pathname).toBe(`/dashboards/${targetDashboardId}`); - expect(url.searchParams.get('where')).toBe( + expect(decodeURIComponent(url.searchParams.get('where') ?? '')).toBe( `ServiceName = '${serviceName}'`, ); expect(url.searchParams.get('whereLanguage')).toBe('sql'); @@ -423,7 +423,7 @@ test.describe( expect(url.searchParams.get('source')).toBe(logsSourceId); expect(url.searchParams.get('whereLanguage')).toBe('sql'); expect(url.searchParams.get('isLive')).toBe('false'); - expect(url.searchParams.get('where')).toBe( + expect(decodeURIComponent(url.searchParams.get('where') ?? '')).toBe( `ServiceName = '${serviceName}'`, ); }); @@ -493,7 +493,7 @@ test.describe( ); const url = new URL(page.url()); expect(url.pathname).toBe(`/dashboards/${targetDashboardId}`); - expect(url.searchParams.get('where')).toBe( + expect(decodeURIComponent(url.searchParams.get('where') ?? '')).toBe( `ServiceName = '${serviceName}'`, ); expect(url.searchParams.get('whereLanguage')).toBe('sql'); diff --git a/packages/common-utils/src/core/__tests__/linkUrlBuilder.test.ts b/packages/common-utils/src/core/__tests__/linkUrlBuilder.test.ts index d66725f5a6..2dd7ec5fd5 100644 --- a/packages/common-utils/src/core/__tests__/linkUrlBuilder.test.ts +++ b/packages/common-utils/src/core/__tests__/linkUrlBuilder.test.ts @@ -90,8 +90,11 @@ describe('renderOnClickSearch', () => { }); expect(result.ok).toBe(true); if (result.ok) { - expect(result.url).toContain('where=ServiceName+%3D+MyService'); - expect(result.url).toContain('whereLanguage=sql'); + const params = new URLSearchParams(result.url.split('?')[1]); + expect(decodeURIComponent(params.get('where') ?? '')).toBe( + 'ServiceName = MyService', + ); + expect(params.get('whereLanguage')).toBe('sql'); } }); @@ -111,8 +114,11 @@ describe('renderOnClickSearch', () => { }); expect(result.ok).toBe(true); if (result.ok) { - expect(result.url).toContain('where=ServiceName%3AMyService'); - expect(result.url).toContain('whereLanguage=lucene'); + const params = new URLSearchParams(result.url.split('?')[1]); + expect(decodeURIComponent(params.get('where') ?? '')).toBe( + 'ServiceName:MyService', + ); + expect(params.get('whereLanguage')).toBe('lucene'); } }); @@ -516,8 +522,11 @@ describe('renderOnClickDashboard', () => { }); expect(result.ok).toBe(true); if (result.ok) { - expect(result.url).toContain('where=ServiceName+%3D+MyService'); - expect(result.url).toContain('whereLanguage=sql'); + const params = new URLSearchParams(result.url.split('?')[1]); + expect(decodeURIComponent(params.get('where') ?? '')).toBe( + 'ServiceName = MyService', + ); + expect(params.get('whereLanguage')).toBe('sql'); } }); @@ -537,8 +546,11 @@ describe('renderOnClickDashboard', () => { }); expect(result.ok).toBe(true); if (result.ok) { - expect(result.url).toContain('where=ServiceName%3AMyService'); - expect(result.url).toContain('whereLanguage=lucene'); + const params = new URLSearchParams(result.url.split('?')[1]); + expect(decodeURIComponent(params.get('where') ?? '')).toBe( + 'ServiceName:MyService', + ); + expect(params.get('whereLanguage')).toBe('lucene'); } }); diff --git a/packages/common-utils/src/core/linkUrlBuilder.ts b/packages/common-utils/src/core/linkUrlBuilder.ts index 80d113e121..62c16daef3 100644 --- a/packages/common-utils/src/core/linkUrlBuilder.ts +++ b/packages/common-utils/src/core/linkUrlBuilder.ts @@ -132,7 +132,7 @@ export function renderOnClickSearch({ const params = new URLSearchParams({ source: sourceId, - where, + where: encodeURIComponent(where), whereLanguage: onClick.whereLanguage ?? 'lucene', isLive: 'false', from: String(dateRange[0].getTime()), @@ -219,7 +219,7 @@ export function renderOnClickDashboard({ } const params = new URLSearchParams({ - where, + where: encodeURIComponent(where), whereLanguage: onClick.whereLanguage ?? 'lucene', from: String(dateRange[0].getTime()), to: String(dateRange[1].getTime()), From 1fc3ef1f19b5040cb652e75380d5afdf1e6ce6e0 Mon Sep 17 00:00:00 2001 From: Drew Davis Date: Mon, 11 May 2026 12:28:09 -0400 Subject: [PATCH 8/8] review: Address more review comments --- .../app/src/__tests__/searchFilters.test.ts | 38 +++++++++++ .../__tests__/useDashboardFilters.test.tsx | 64 +++++++++++++++++++ packages/app/src/searchFilters.tsx | 17 +++-- .../features/dashboard-table-linking.spec.ts | 11 ++++ .../tests/e2e/page-objects/DashboardPage.ts | 25 ++++++++ 5 files changed, 150 insertions(+), 5 deletions(-) diff --git a/packages/app/src/__tests__/searchFilters.test.ts b/packages/app/src/__tests__/searchFilters.test.ts index e797ccf736..f838f49710 100644 --- a/packages/app/src/__tests__/searchFilters.test.ts +++ b/packages/app/src/__tests__/searchFilters.test.ts @@ -590,6 +590,44 @@ describe('searchFilters', () => { }, }); }); + + it('round-trips values containing backslashes (Windows paths)', () => { + const originalFilters = { + FilePath: { + included: new Set(['C:\\path\\file']), + excluded: new Set(), + }, + }; + + const query = filtersToQuery(originalFilters); + const parsed = parseQuery(query); + + expect(parsed.filters).toEqual({ + FilePath: { + included: new Set(['C:\\path\\file']), + excluded: new Set(), + }, + }); + }); + + it('round-trips values containing both backslashes and single quotes', () => { + const originalFilters = { + message: { + included: new Set(["O\\'Malley"]), + excluded: new Set(), + }, + }; + + const query = filtersToQuery(originalFilters); + const parsed = parseQuery(query); + + expect(parsed.filters).toEqual({ + message: { + included: new Set(["O\\'Malley"]), + excluded: new Set(), + }, + }); + }); }); describe('useSearchPageFilterState', () => { diff --git a/packages/app/src/hooks/__tests__/useDashboardFilters.test.tsx b/packages/app/src/hooks/__tests__/useDashboardFilters.test.tsx index dcf3fc005b..baa8904b3b 100644 --- a/packages/app/src/hooks/__tests__/useDashboardFilters.test.tsx +++ b/packages/app/src/hooks/__tests__/useDashboardFilters.test.tsx @@ -208,4 +208,68 @@ describe('useDashboardFilters', () => { new Set(['api']), ); }); + + describe('ignoredFilterExpressions', () => { + it('is empty when no URL filters are set', () => { + const { result } = renderHook(() => useDashboardFilters(mockFilters)); + + expect(result.current.ignoredFilterExpressions).toEqual([]); + }); + + it('is empty when URL filters only reference declared expressions', () => { + mockState = [ + { type: 'sql', condition: "environment IN ('production')" }, + { type: 'sql', condition: "service.name IN ('api')" }, + ]; + + const { result } = renderHook(() => useDashboardFilters(mockFilters)); + + expect(result.current.ignoredFilterExpressions).toEqual([]); + }); + + it('lists a single ignored expression not declared by the dashboard', () => { + mockState = [ + { type: 'sql', condition: "environment IN ('production')" }, + { type: 'sql', condition: "team IN ('platform')" }, + ]; + + const { result } = renderHook(() => useDashboardFilters(mockFilters)); + + expect(result.current.ignoredFilterExpressions).toEqual(['team']); + // sanity: declared expression still wins through normal path + expect(result.current.filterValues.environment.included).toEqual( + new Set(['production']), + ); + }); + + it('lists multiple ignored expressions in URL-encounter order', () => { + mockState = [ + { type: 'sql', condition: "team IN ('platform')" }, + { type: 'sql', condition: "environment IN ('production')" }, + { type: 'sql', condition: "region IN ('us-east-1')" }, + { type: 'sql', condition: "owner IN ('drew')" }, + ]; + + const { result } = renderHook(() => useDashboardFilters(mockFilters)); + + expect(result.current.ignoredFilterExpressions).toEqual([ + 'team', + 'region', + 'owner', + ]); + expect(Object.keys(result.current.filterValues)).toEqual(['environment']); + }); + + it('does not flag declared expressions with no URL values as ignored', () => { + // URL is empty — every declared expression has no values, but none of + // them should be reported as ignored since they are valid dashboard + // filters that just happen to be unset. + mockState = null; + + const { result } = renderHook(() => useDashboardFilters(mockFilters)); + + expect(result.current.filterValues).toEqual({}); + expect(result.current.ignoredFilterExpressions).toEqual([]); + }); + }); }); diff --git a/packages/app/src/searchFilters.tsx b/packages/app/src/searchFilters.tsx index 78731e94ab..fbb6292857 100644 --- a/packages/app/src/searchFilters.tsx +++ b/packages/app/src/searchFilters.tsx @@ -51,20 +51,27 @@ const getBooleanOrUnquotedString = (value: string): string | boolean => { return trimmed.toLowerCase() === 'true'; } - // Remove surrounding quotes and un-escape '' → ' + // Remove surrounding quotes and reverse the escape sequences produced by + // filtersToQuery's escapeString. Order matters: collapse \\ → \ first so + // that the following '' → ' pass doesn't mistake content for an escape. if (trimmed.startsWith("'") && trimmed.endsWith("'")) { - return trimmed.slice(1, -1).replace(/''/g, "'"); + return trimmed.slice(1, -1).replace(/\\\\/g, '\\').replace(/''/g, "'"); } return trimmed; }; // Returns true when the single-quote at position `i` is a real string delimiter // rather than an escape sequence. Handles both ClickHouse/SQL '' escaping and -// backslash \' escaping. +// backslash \' escaping. An odd number of preceding backslashes means the +// quote is escaped via \'; an even number (including zero) means the +// backslashes are themselves escaped (\\) and the quote is a real boundary. function isQuoteBoundary(s: string, i: number): boolean { if (s[i] !== "'") return false; - if (i > 0 && s[i - 1] === '\\') return false; - return true; + let backslashes = 0; + for (let j = i - 1; j >= 0 && s[j] === '\\'; j--) { + backslashes++; + } + return backslashes % 2 === 0; } // If we're inside a quoted string and hit a quote, check whether the next diff --git a/packages/app/tests/e2e/features/dashboard-table-linking.spec.ts b/packages/app/tests/e2e/features/dashboard-table-linking.spec.ts index 084d233c36..88c4d43d0e 100644 --- a/packages/app/tests/e2e/features/dashboard-table-linking.spec.ts +++ b/packages/app/tests/e2e/features/dashboard-table-linking.spec.ts @@ -739,6 +739,17 @@ test.describe( await dashboardPage.dismissIgnoredUrlFiltersBanner(); await expect(dashboardPage.ignoredUrlFiltersBanner).toBeHidden(); }); + + await test.step('Save the target dashboard and verify the banner stays hidden', async () => { + // Renaming the dashboard PATCHes it and causes React Query to refetch, + // toggling isFetchingDashboard true→false. The lastLoadedIdForBannerRef + // invariant must prevent the dismissed banner from reappearing. + await dashboardPage.renameDashboard( + targetDashboardName, + `${targetDashboardName} Renamed`, + ); + await expect(dashboardPage.ignoredUrlFiltersBanner).toBeHidden(); + }); }); }, ); diff --git a/packages/app/tests/e2e/page-objects/DashboardPage.ts b/packages/app/tests/e2e/page-objects/DashboardPage.ts index 8f8e6a45b0..39bd4e2095 100644 --- a/packages/app/tests/e2e/page-objects/DashboardPage.ts +++ b/packages/app/tests/e2e/page-objects/DashboardPage.ts @@ -210,6 +210,31 @@ export class DashboardPage { await updatedHeading.waitFor({ state: 'visible', timeout: 10000 }); } + /** + * Rename a dashboard from a known current name to a new name. Use when the + * heading is no longer the "My Dashboard" default (e.g. already renamed + * earlier in the same test). + */ + async renameDashboard(currentName: string, newName: string) { + const currentHeading = this.page.getByRole('heading', { + name: currentName, + level: 3, + }); + await currentHeading.waitFor({ state: 'visible', timeout: 10000 }); + + await currentHeading.dblclick(); + + const nameInput = this.page.locator('input[placeholder="Name"]'); + await nameInput.fill(newName); + await this.page.keyboard.press('Enter'); + + const updatedHeading = this.page.getByRole('heading', { + name: newName, + level: 3, + }); + await updatedHeading.waitFor({ state: 'visible', timeout: 10000 }); + } + /** * Add a new tile to the dashboard */