From c0cf38ddd3dee7bc0a956caaccca7ce0622429ef Mon Sep 17 00:00:00 2001 From: Tushar Bahl Date: Mon, 5 Oct 2026 12:35:48 +0530 Subject: [PATCH 01/16] fix: Stale waterfall selection when opening another span from the same trace Opening one span, closing its side panel, and then opening a different span from the same trace leaves the waterfall selection and span details on the first span. The results list contains several spans that share one TraceId and SpanName but have different SpanId values. The side-panel header follows the newly opened row (rowWhere), while the waterfall highlight and Overview stay on the previously selected span (eventRowWhere). --- .../trace-span-follows-opened-result.md | 5 + .../components/DBSqlRowTableWithSidebar.tsx | 26 ++- packages/app/src/components/DBTracePanel.tsx | 26 +-- .../app/src/components/ExpandableRowTable.tsx | 14 +- .../DBSqlRowTableWithSidebar.test.tsx | 171 ++++++++++++++++++ packages/app/src/components/eventRowWhere.ts | 40 ++++ 6 files changed, 254 insertions(+), 28 deletions(-) create mode 100644 .changeset/trace-span-follows-opened-result.md create mode 100644 packages/app/src/components/__tests__/DBSqlRowTableWithSidebar.test.tsx create mode 100644 packages/app/src/components/eventRowWhere.ts diff --git a/.changeset/trace-span-follows-opened-result.md b/.changeset/trace-span-follows-opened-result.md new file mode 100644 index 0000000000..142399ba29 --- /dev/null +++ b/.changeset/trace-span-follows-opened-result.md @@ -0,0 +1,5 @@ +--- +'@hyperdx/app': patch +--- + +Fix the trace waterfall sticking on the previously opened span when a result is closed and another span of the same trace is opened. The span selected in the waterfall is cleared with the result, unless the result being opened is that span. diff --git a/packages/app/src/components/DBSqlRowTableWithSidebar.tsx b/packages/app/src/components/DBSqlRowTableWithSidebar.tsx index c683786711..bdc53a49ed 100644 --- a/packages/app/src/components/DBSqlRowTableWithSidebar.tsx +++ b/packages/app/src/components/DBSqlRowTableWithSidebar.tsx @@ -25,6 +25,10 @@ import DBRowSidePanel, { } from './DBRowSidePanel'; import { DBRowSidePanelErrorState } from './DBRowSidePanelErrorState'; import { DBRowTableVariant, DBSqlRowTable } from './DBRowTable'; +import { + eventRowWhereForOpenedRow, + eventRowWhereParser, +} from './eventRowWhere'; interface Props { sourceId: string; @@ -88,6 +92,10 @@ export default function DBSqlRowTableWithSideBar({ const { data: sourceData } = useSource({ id: sourceId }); const [rowId, setRowId] = useQueryState('rowWhere', parseAsStringEncoded); const [rowSource, setRowSource] = useQueryState('rowSource'); + const [, setEventRowWhere] = useQueryState( + 'eventRowWhere', + eventRowWhereParser, + ); const [aliasWith, setAliasWith] = useState([]); const onOpenSidebar = useCallback( @@ -96,14 +104,28 @@ export default function DBSqlRowTableWithSideBar({ setAliasWith(rowWhere.aliasWith); setRowSource(sourceId); onSidebarOpen?.(rowWhere.where); + // Same trace id used to leave the previous span selected, so the + // waterfall kept showing it after this result was closed and another + // span of that trace was opened. + setEventRowWhere(current => + eventRowWhereForOpenedRow(current, rowWhere.where), + ); }, - [setRowId, setAliasWith, setRowSource, sourceId, onSidebarOpen], + [ + setRowId, + setAliasWith, + setRowSource, + sourceId, + onSidebarOpen, + setEventRowWhere, + ], ); const onCloseSidebar = useCallback(() => { setRowId(null); setRowSource(null); - }, [setRowId, setRowSource]); + setEventRowWhere(null); + }, [setRowId, setRowSource, setEventRowWhere]); const renderRowDetails = useCallback( (r: { id: string; aliasWith?: WithClause[]; [key: string]: unknown }) => { if (!sourceData) { diff --git a/packages/app/src/components/DBTracePanel.tsx b/packages/app/src/components/DBTracePanel.tsx index 822eb208d4..008a7ecc66 100644 --- a/packages/app/src/components/DBTracePanel.tsx +++ b/packages/app/src/components/DBTracePanel.tsx @@ -10,14 +10,12 @@ import { useAtom } from 'jotai'; import { atomWithStorage } from 'jotai/utils'; import { useQueryState } from 'nuqs'; import { useForm, useWatch } from 'react-hook-form'; -import { z } from 'zod'; import { tcFromSource } from '@hyperdx/common-utils/dist/core/metadata'; import { isLogSource, isTraceSource, SourceKind, TSource, - WithClauseSchema, } from '@hyperdx/common-utils/dist/types'; import { ActionIcon, @@ -41,7 +39,6 @@ import useResizable from '@/hooks/useResizable'; import { WithClause } from '@/hooks/useRowWhere'; import { useSource, useUpdateSource } from '@/source'; import TabBar from '@/TabBar'; -import { parseAsJsonEncoded } from '@/utils/queryParsers'; import DBInfraPanel from './DBInfraPanel'; import { RowDataPanel, rowHasK8sContext, useRowData } from './DBRowDataPanel'; @@ -54,31 +51,10 @@ import SourceSchemaPreview, { isSourceSchemaPreviewEnabled, } from './SourceSchemaPreview'; import { SourceSelectControlled } from './SourceSelect'; +import { EventRowWhere, eventRowWhereParser } from './eventRowWhere'; import resizeStyles from '@/../styles/ResizablePanel.module.scss'; -type EventRowWhere = { - id: string; - type: string; - aliasWith: WithClause[]; - // The trace this span selection was made in. Used to gate a selection left in - // the URL from a previous trace so it can't render against a different one. - traceId?: string; -}; - -// Validate the persisted span selection so a stale / hand-edited `eventRowWhere` -// (valid JSON but wrong shape) resolves to null instead of feeding a malformed -// row id into the span detail query. -const eventRowWhereSchema = z.object({ - id: z.string(), - type: z.string(), - aliasWith: z.array(WithClauseSchema), - traceId: z.string().optional(), -}); -const eventRowWhereParser = parseAsJsonEncoded( - eventRowWhereSchema.parse, -); - enum SpanDetailTab { Overview = 'overview', Parsed = 'parsed', diff --git a/packages/app/src/components/ExpandableRowTable.tsx b/packages/app/src/components/ExpandableRowTable.tsx index 51f979035c..a674acb036 100644 --- a/packages/app/src/components/ExpandableRowTable.tsx +++ b/packages/app/src/components/ExpandableRowTable.tsx @@ -7,19 +7,31 @@ import { IconArrowsMaximize, IconChevronRight } from '@tabler/icons-react'; import { INTERNAL_ROW_FIELDS } from '@/hooks/useRowWhere'; import { parseAsStringEncoded } from '@/utils/queryParsers'; +import { + eventRowWhereForOpenedRow, + eventRowWhereParser, +} from './eventRowWhere'; + import styles from '@styles/LogTable.module.scss'; // Hook that provides a function to open the sidebar with specific row details const useSidebarOpener = () => { const [, setRowId] = useQueryState('rowWhere', parseAsStringEncoded); const [, setRowSource] = useQueryState('rowSource'); + const [, setEventRowWhere] = useQueryState( + 'eventRowWhere', + eventRowWhereParser, + ); return useCallback( (rowWhere: string, sourceId?: string) => { setRowId(rowWhere); setRowSource(sourceId ?? null); + setEventRowWhere(current => + eventRowWhereForOpenedRow(current, rowWhere), + ); }, - [setRowId, setRowSource], + [setRowId, setRowSource, setEventRowWhere], ); }; diff --git a/packages/app/src/components/__tests__/DBSqlRowTableWithSidebar.test.tsx b/packages/app/src/components/__tests__/DBSqlRowTableWithSidebar.test.tsx new file mode 100644 index 0000000000..1ee02e021c --- /dev/null +++ b/packages/app/src/components/__tests__/DBSqlRowTableWithSidebar.test.tsx @@ -0,0 +1,171 @@ +import { BuilderChartConfigWithDateRange } from '@hyperdx/common-utils/dist/types'; +import { fireEvent, screen } from '@testing-library/react'; + +import DBSqlRowTableWithSideBar from '@/components/DBSqlRowTableWithSidebar'; + +const FIRST_SPAN = "SpanId='8cea712d1d4df628'"; +const OTHER_SPAN = "SpanId='b2e2b0df5304a26a'"; + +type StoredSpan = { + id: string; + type: string; + aliasWith: unknown[]; + traceId: string; +}; + +const mockQuery: Record = {}; +const mockListeners = new Set<() => void>(); + +jest.mock('nuqs', () => { + const React = jest.requireActual('react'); + const current = (key: string) => + Object.prototype.hasOwnProperty.call(mockQuery, key) + ? mockQuery[key] + : null; + return { + useQueryState: (key: string) => { + const [, rerender] = React.useState(0); + React.useEffect(() => { + const listener = () => rerender((tick: number) => tick + 1); + mockListeners.add(listener); + return () => { + mockListeners.delete(listener); + }; + }, [rerender]); + + const setValue = React.useCallback( + (next: unknown) => { + const previous = current(key); + const resolved = typeof next === 'function' ? next(previous) : next; + if (Object.is(resolved, previous)) return; + mockQuery[key] = resolved; + mockListeners.forEach(listener => listener()); + }, + [key], + ); + + return [current(key), setValue]; + }, + }; +}); + +jest.mock('@/source', () => ({ + useSource: () => ({ + data: { id: 'trace-source', kind: 'trace' }, + isLoading: false, + }), +})); + +jest.mock('../DBRowTable', () => ({ + DBSqlRowTable: ({ + onRowDetailsClick, + }: { + onRowDetailsClick: (row: { where: string; aliasWith: never[] }) => void; + }) => ( + <> + + + + ), +})); + +jest.mock('../DBRowSidePanel', () => { + const React = jest.requireActual('react'); + return { + __esModule: true, + default: ({ + onClose, + }: { + onClose: () => void; + rowId?: string; + }) => ( + + ), + RowSidePanelContext: React.createContext({}), + }; +}); + +jest.mock('../DBRowDataPanel', () => ({ + useRowData: () => ({ isError: false, error: null }), + RowDataPanel: () => null, +})); + +jest.mock('../DBRowOverviewPanel', () => ({ + RowOverviewPanel: () => null, +})); + +jest.mock('../DBRowSidePanelErrorState', () => ({ + DBRowSidePanelErrorState: () => null, +})); + +const chartConfig = { + select: [], + where: '', + whereLanguage: 'sql', + dateRange: [new Date(0), new Date(1)], +} as BuilderChartConfigWithDateRange; + +function span(id: string): StoredSpan { + return { + id, + type: 'trace', + aliasWith: [], + traceId: 'trace-123', + }; +} + +function renderResults() { + renderWithMantine( + , + ); +} + +describe('DBSqlRowTableWithSideBar span selection', () => { + beforeEach(() => { + Object.keys(mockQuery).forEach(key => { + delete mockQuery[key]; + }); + mockListeners.clear(); + }); + + it('drops the span when one result is closed and another span of the same trace is opened', () => { + mockQuery.rowWhere = FIRST_SPAN; + mockQuery.eventRowWhere = span(FIRST_SPAN); + renderResults(); + + expect(mockQuery.eventRowWhere).toEqual(span(FIRST_SPAN)); + + fireEvent.click(screen.getByRole('button', { name: 'close result' })); + expect(mockQuery.eventRowWhere).toBeNull(); + + fireEvent.click(screen.getByRole('button', { name: 'open other span' })); + expect(mockQuery.eventRowWhere).toBeNull(); + expect(mockQuery.rowWhere).toBe(OTHER_SPAN); + }); + + it('keeps the span when the opened result is that span', () => { + mockQuery.eventRowWhere = span(FIRST_SPAN); + renderResults(); + + fireEvent.click(screen.getByRole('button', { name: 'open first span' })); + + expect(mockQuery.eventRowWhere).toEqual(span(FIRST_SPAN)); + expect(mockQuery.rowWhere).toBe(FIRST_SPAN); + }); +}); diff --git a/packages/app/src/components/eventRowWhere.ts b/packages/app/src/components/eventRowWhere.ts new file mode 100644 index 0000000000..82ceaa2ed3 --- /dev/null +++ b/packages/app/src/components/eventRowWhere.ts @@ -0,0 +1,40 @@ +import { z } from 'zod'; +import { WithClauseSchema } from '@hyperdx/common-utils/dist/types'; + +import { parseAsJsonEncoded } from '@/utils/queryParsers'; + +// Validate the persisted span selection so a stale / hand-edited `eventRowWhere` +// (valid JSON but wrong shape) resolves to null instead of feeding a malformed +// row id into the span detail query. +const eventRowWhereSchema = z.object({ + id: z.string(), + type: z.string(), + aliasWith: z.array(WithClauseSchema), + // The trace this span selection was made in. Used to gate a selection left in + // the URL from a previous trace so it can't render against a different one. + traceId: z.string().optional(), +}); + +export type EventRowWhere = z.infer; + +export const eventRowWhereParser = parseAsJsonEncoded( + eventRowWhereSchema.parse, +); + +// `rowWhere` is the search result; `eventRowWhere` is the span open in the +// waterfall. A new result of the same trace must not keep the previous span. +// The exception is the result that *is* that span — clearing it would drop the +// selection the click just made. +export function eventRowWhereForOpenedRow( + current: T | null, + openedRowWhere: string | null, +): T | null { + if ( + current != null && + openedRowWhere != null && + current.id === openedRowWhere + ) { + return current; + } + return null; +} From d29dc330ef37adcc56c9f40e9807d0a3ea63f88f Mon Sep 17 00:00:00 2001 From: Tushar Bahl Date: Mon, 5 Oct 2026 17:32:20 +0530 Subject: [PATCH 02/16] fix: comparing two different row-id formats --- .../trace-span-follows-opened-result.md | 2 +- .../components/DBSqlRowTableWithSidebar.tsx | 6 ++---- .../app/src/components/ExpandableRowTable.tsx | 6 +++--- .../DBSqlRowTableWithSidebar.test.tsx | 21 ++++++++++++++++--- packages/app/src/components/eventRowWhere.ts | 17 ++++++++------- 5 files changed, 34 insertions(+), 18 deletions(-) diff --git a/.changeset/trace-span-follows-opened-result.md b/.changeset/trace-span-follows-opened-result.md index 142399ba29..c8f9ebc447 100644 --- a/.changeset/trace-span-follows-opened-result.md +++ b/.changeset/trace-span-follows-opened-result.md @@ -2,4 +2,4 @@ '@hyperdx/app': patch --- -Fix the trace waterfall sticking on the previously opened span when a result is closed and another span of the same trace is opened. The span selected in the waterfall is cleared with the result, unless the result being opened is that span. +Fix the trace waterfall sticking on the previously opened span when a result is closed and another span of the same trace is opened. Opening a different result clears the span selected in the waterfall. Opening the result that is already open leaves that selection in place. diff --git a/packages/app/src/components/DBSqlRowTableWithSidebar.tsx b/packages/app/src/components/DBSqlRowTableWithSidebar.tsx index bdc53a49ed..161684b07b 100644 --- a/packages/app/src/components/DBSqlRowTableWithSidebar.tsx +++ b/packages/app/src/components/DBSqlRowTableWithSidebar.tsx @@ -104,14 +104,12 @@ export default function DBSqlRowTableWithSideBar({ setAliasWith(rowWhere.aliasWith); setRowSource(sourceId); onSidebarOpen?.(rowWhere.where); - // Same trace id used to leave the previous span selected, so the - // waterfall kept showing it after this result was closed and another - // span of that trace was opened. setEventRowWhere(current => - eventRowWhereForOpenedRow(current, rowWhere.where), + eventRowWhereForOpenedRow(current, rowId, rowWhere.where), ); }, [ + rowId, setRowId, setAliasWith, setRowSource, diff --git a/packages/app/src/components/ExpandableRowTable.tsx b/packages/app/src/components/ExpandableRowTable.tsx index a674acb036..4649cf5a7b 100644 --- a/packages/app/src/components/ExpandableRowTable.tsx +++ b/packages/app/src/components/ExpandableRowTable.tsx @@ -16,7 +16,7 @@ import styles from '@styles/LogTable.module.scss'; // Hook that provides a function to open the sidebar with specific row details const useSidebarOpener = () => { - const [, setRowId] = useQueryState('rowWhere', parseAsStringEncoded); + const [rowId, setRowId] = useQueryState('rowWhere', parseAsStringEncoded); const [, setRowSource] = useQueryState('rowSource'); const [, setEventRowWhere] = useQueryState( 'eventRowWhere', @@ -28,10 +28,10 @@ const useSidebarOpener = () => { setRowId(rowWhere); setRowSource(sourceId ?? null); setEventRowWhere(current => - eventRowWhereForOpenedRow(current, rowWhere), + eventRowWhereForOpenedRow(current, rowId, rowWhere), ); }, - [setRowId, setRowSource, setEventRowWhere], + [rowId, setRowId, setRowSource, setEventRowWhere], ); }; diff --git a/packages/app/src/components/__tests__/DBSqlRowTableWithSidebar.test.tsx b/packages/app/src/components/__tests__/DBSqlRowTableWithSidebar.test.tsx index 1ee02e021c..854c3c9292 100644 --- a/packages/app/src/components/__tests__/DBSqlRowTableWithSidebar.test.tsx +++ b/packages/app/src/components/__tests__/DBSqlRowTableWithSidebar.test.tsx @@ -5,6 +5,9 @@ import DBSqlRowTableWithSideBar from '@/components/DBSqlRowTableWithSidebar'; const FIRST_SPAN = "SpanId='8cea712d1d4df628'"; const OTHER_SPAN = "SpanId='b2e2b0df5304a26a'"; +// Waterfall ids omit SpanAttributes/SpanEvents and are not the search row's where. +const WATERFALL_SPAN = + "TraceId='trace-123' AND SpanId='b2e2b0df5304a26a' AND Timestamp='2024-01-01'"; type StoredSpan = { id: string; @@ -159,13 +162,25 @@ describe('DBSqlRowTableWithSideBar span selection', () => { expect(mockQuery.rowWhere).toBe(OTHER_SPAN); }); - it('keeps the span when the opened result is that span', () => { - mockQuery.eventRowWhere = span(FIRST_SPAN); + it('keeps the waterfall span when the same result is opened again', () => { + mockQuery.rowWhere = FIRST_SPAN; + mockQuery.eventRowWhere = span(WATERFALL_SPAN); renderResults(); fireEvent.click(screen.getByRole('button', { name: 'open first span' })); - expect(mockQuery.eventRowWhere).toEqual(span(FIRST_SPAN)); + expect(mockQuery.eventRowWhere).toEqual(span(WATERFALL_SPAN)); expect(mockQuery.rowWhere).toBe(FIRST_SPAN); }); + + it('drops the waterfall span when a different result is opened', () => { + mockQuery.rowWhere = FIRST_SPAN; + mockQuery.eventRowWhere = span(OTHER_SPAN); + renderResults(); + + fireEvent.click(screen.getByRole('button', { name: 'open other span' })); + + expect(mockQuery.eventRowWhere).toBeNull(); + expect(mockQuery.rowWhere).toBe(OTHER_SPAN); + }); }); diff --git a/packages/app/src/components/eventRowWhere.ts b/packages/app/src/components/eventRowWhere.ts index 82ceaa2ed3..8e7e231194 100644 --- a/packages/app/src/components/eventRowWhere.ts +++ b/packages/app/src/components/eventRowWhere.ts @@ -21,18 +21,21 @@ export const eventRowWhereParser = parseAsJsonEncoded( eventRowWhereSchema.parse, ); -// `rowWhere` is the search result; `eventRowWhere` is the span open in the -// waterfall. A new result of the same trace must not keep the previous span. -// The exception is the result that *is* that span — clearing it would drop the -// selection the click just made. -export function eventRowWhereForOpenedRow( +// `openRowWhere` is the search result already open. `openedRowWhere` is the +// result being opened. Both come from the search table. The waterfall's +// `eventRowWhere.id` is built from different columns, so comparing it to +// either string never matches and would always clear the span. +// Opening a different result of the same trace must drop the previous span. +// Re-opening the result already open must keep the span picked in the waterfall. +export function eventRowWhereForOpenedRow( current: T | null, + openRowWhere: string | null, openedRowWhere: string | null, ): T | null { if ( current != null && - openedRowWhere != null && - current.id === openedRowWhere + openRowWhere != null && + openRowWhere === openedRowWhere ) { return current; } From 00fae350e5d2a8f2ed8c57e1270ff8e50e6d26ac Mon Sep 17 00:00:00 2001 From: Tushar Bahl Date: Mon, 5 Oct 2026 17:36:29 +0530 Subject: [PATCH 03/16] fix in keep check Same span loses selection. That comment is on the old current.id === openedRowWhere check. The waterfall where string and the search-row where string are built from different columns, so that comparison never matched. The keep check now compares the two search-row where strings, and the test uses a waterfall where string that is not the search row's. --- .changeset/trace-span-follows-opened-result.md | 2 +- .../src/components/DBSqlRowTableWithSidebar.tsx | 3 +-- .../__tests__/DBSqlRowTableWithSidebar.test.tsx | 14 +++++++++++++- 3 files changed, 15 insertions(+), 4 deletions(-) diff --git a/.changeset/trace-span-follows-opened-result.md b/.changeset/trace-span-follows-opened-result.md index c8f9ebc447..29cae14b64 100644 --- a/.changeset/trace-span-follows-opened-result.md +++ b/.changeset/trace-span-follows-opened-result.md @@ -2,4 +2,4 @@ '@hyperdx/app': patch --- -Fix the trace waterfall sticking on the previously opened span when a result is closed and another span of the same trace is opened. Opening a different result clears the span selected in the waterfall. Opening the result that is already open leaves that selection in place. +Fix the trace waterfall sticking on the previously opened span when a result is closed and another span of the same trace is opened. Opening a different result clears that span. Closing the result, or opening the result that is already open, leaves the span selected in the waterfall. diff --git a/packages/app/src/components/DBSqlRowTableWithSidebar.tsx b/packages/app/src/components/DBSqlRowTableWithSidebar.tsx index 161684b07b..f36e73eabd 100644 --- a/packages/app/src/components/DBSqlRowTableWithSidebar.tsx +++ b/packages/app/src/components/DBSqlRowTableWithSidebar.tsx @@ -122,8 +122,7 @@ export default function DBSqlRowTableWithSideBar({ const onCloseSidebar = useCallback(() => { setRowId(null); setRowSource(null); - setEventRowWhere(null); - }, [setRowId, setRowSource, setEventRowWhere]); + }, [setRowId, setRowSource]); const renderRowDetails = useCallback( (r: { id: string; aliasWith?: WithClause[]; [key: string]: unknown }) => { if (!sourceData) { diff --git a/packages/app/src/components/__tests__/DBSqlRowTableWithSidebar.test.tsx b/packages/app/src/components/__tests__/DBSqlRowTableWithSidebar.test.tsx index 854c3c9292..530e7285f8 100644 --- a/packages/app/src/components/__tests__/DBSqlRowTableWithSidebar.test.tsx +++ b/packages/app/src/components/__tests__/DBSqlRowTableWithSidebar.test.tsx @@ -155,13 +155,25 @@ describe('DBSqlRowTableWithSideBar span selection', () => { expect(mockQuery.eventRowWhere).toEqual(span(FIRST_SPAN)); fireEvent.click(screen.getByRole('button', { name: 'close result' })); - expect(mockQuery.eventRowWhere).toBeNull(); + expect(mockQuery.eventRowWhere).toEqual(span(FIRST_SPAN)); + expect(mockQuery.rowWhere).toBeNull(); fireEvent.click(screen.getByRole('button', { name: 'open other span' })); expect(mockQuery.eventRowWhere).toBeNull(); expect(mockQuery.rowWhere).toBe(OTHER_SPAN); }); + it('keeps the waterfall span when the result is closed', () => { + mockQuery.rowWhere = FIRST_SPAN; + mockQuery.eventRowWhere = span(WATERFALL_SPAN); + renderResults(); + + fireEvent.click(screen.getByRole('button', { name: 'close result' })); + + expect(mockQuery.rowWhere).toBeNull(); + expect(mockQuery.eventRowWhere).toEqual(span(WATERFALL_SPAN)); + }); + it('keeps the waterfall span when the same result is opened again', () => { mockQuery.rowWhere = FIRST_SPAN; mockQuery.eventRowWhere = span(WATERFALL_SPAN); From 30a6b973f3f6308343131f6c24c7697580eab399 Mon Sep 17 00:00:00 2001 From: Tushar Bahl Date: Fri, 9 Oct 2026 09:37:41 +0530 Subject: [PATCH 04/16] fix: changes in drawer span selection Closing a result, or opening a different one while the drawer stays open, now clears the previous waterfall span instead of leaving it selected. --- .../trace-span-follows-opened-result.md | 2 +- .../app/src/components/DBRowSidePanel.tsx | 16 +- .../components/DBSqlRowTableWithSidebar.tsx | 21 +- .../src/components/DBTraceWaterfallChart.tsx | 57 +---- .../app/src/components/ExpandableRowTable.tsx | 16 +- .../DBSqlRowTableWithSidebar.test.tsx | 198 ------------------ packages/app/src/components/eventRowWhere.ts | 21 -- .../__tests__/useRowHighlightHint.test.tsx | 177 ++++++++++++++++ packages/app/src/hooks/useRowHighlightHint.ts | 81 +++++++ .../e2e/components/SidePanelComponent.ts | 29 +++ .../tests/e2e/components/TableComponent.ts | 4 + .../e2e/features/traces-workflow.spec.ts | 89 ++++++++ 12 files changed, 407 insertions(+), 304 deletions(-) delete mode 100644 packages/app/src/components/__tests__/DBSqlRowTableWithSidebar.test.tsx create mode 100644 packages/app/src/hooks/__tests__/useRowHighlightHint.test.tsx create mode 100644 packages/app/src/hooks/useRowHighlightHint.ts diff --git a/.changeset/trace-span-follows-opened-result.md b/.changeset/trace-span-follows-opened-result.md index 29cae14b64..202929364d 100644 --- a/.changeset/trace-span-follows-opened-result.md +++ b/.changeset/trace-span-follows-opened-result.md @@ -2,4 +2,4 @@ '@hyperdx/app': patch --- -Fix the trace waterfall sticking on the previously opened span when a result is closed and another span of the same trace is opened. Opening a different result clears that span. Closing the result, or opening the result that is already open, leaves the span selected in the waterfall. +Fix the trace waterfall sticking on the previously opened span when another span of the same trace is opened. Closing the result clears the selected span. Opening a different result while the drawer stays open selects that result's span. diff --git a/packages/app/src/components/DBRowSidePanel.tsx b/packages/app/src/components/DBRowSidePanel.tsx index 17f3412748..81374577ed 100644 --- a/packages/app/src/components/DBRowSidePanel.tsx +++ b/packages/app/src/components/DBRowSidePanel.tsx @@ -9,6 +9,7 @@ import { } from 'react'; import { add } from 'date-fns'; import { isString } from 'lodash'; +import { useQueryState } from 'nuqs'; import { ErrorBoundary, FallbackProps } from 'react-error-boundary'; import { useHotkeys } from 'react-hotkeys-hook'; import SqlString from 'sqlstring'; @@ -82,6 +83,7 @@ import SidePanelBreadcrumbs, { BreadcrumbItem } from './SidePanelBreadcrumbs'; import { SpanLinkData } from './SpanLinksSubpanel'; import TraceLogsPanel from './TraceLogsPanel'; import { ViewTraceCalloutButton } from './ViewTraceCalloutButton'; +import { eventRowWhereParser } from './eventRowWhere'; import styles from '@/../styles/LogSidePanel.module.scss'; @@ -1324,6 +1326,10 @@ export default function DBRowSidePanelErrorBoundary({ }, [isFullWidth, setSize]); const { clear: clearTraceWaterfallSearchState } = useWaterfallSearchState({}); + const [, setEventRowWhere] = useQueryState( + 'eventRowWhere', + eventRowWhereParser, + ); const sidePanelStack = useSidePanelStack({ initialRowId: rowId }); @@ -1333,8 +1339,16 @@ export default function DBRowSidePanelErrorBoundary({ // Clear waterfall search state on close, so that filters don't // persist when reopening another trace. clearTraceWaterfallSearchState(); + // The selected span is in the URL. Leaving it there makes the next open, + // and any shared row link, show the previous span. + setEventRowWhere(null); onClose(); - }, [sidePanelStack, onClose, clearTraceWaterfallSearchState]); + }, [ + sidePanelStack, + onClose, + clearTraceWaterfallSearchState, + setEventRowWhere, + ]); useCloseOnClickOutside({ // Only close on outside click at the root level. When the user has diff --git a/packages/app/src/components/DBSqlRowTableWithSidebar.tsx b/packages/app/src/components/DBSqlRowTableWithSidebar.tsx index f36e73eabd..c683786711 100644 --- a/packages/app/src/components/DBSqlRowTableWithSidebar.tsx +++ b/packages/app/src/components/DBSqlRowTableWithSidebar.tsx @@ -25,10 +25,6 @@ import DBRowSidePanel, { } from './DBRowSidePanel'; import { DBRowSidePanelErrorState } from './DBRowSidePanelErrorState'; import { DBRowTableVariant, DBSqlRowTable } from './DBRowTable'; -import { - eventRowWhereForOpenedRow, - eventRowWhereParser, -} from './eventRowWhere'; interface Props { sourceId: string; @@ -92,10 +88,6 @@ export default function DBSqlRowTableWithSideBar({ const { data: sourceData } = useSource({ id: sourceId }); const [rowId, setRowId] = useQueryState('rowWhere', parseAsStringEncoded); const [rowSource, setRowSource] = useQueryState('rowSource'); - const [, setEventRowWhere] = useQueryState( - 'eventRowWhere', - eventRowWhereParser, - ); const [aliasWith, setAliasWith] = useState([]); const onOpenSidebar = useCallback( @@ -104,19 +96,8 @@ export default function DBSqlRowTableWithSideBar({ setAliasWith(rowWhere.aliasWith); setRowSource(sourceId); onSidebarOpen?.(rowWhere.where); - setEventRowWhere(current => - eventRowWhereForOpenedRow(current, rowId, rowWhere.where), - ); }, - [ - rowId, - setRowId, - setAliasWith, - setRowSource, - sourceId, - onSidebarOpen, - setEventRowWhere, - ], + [setRowId, setAliasWith, setRowSource, sourceId, onSidebarOpen], ); const onCloseSidebar = useCallback(() => { diff --git a/packages/app/src/components/DBTraceWaterfallChart.tsx b/packages/app/src/components/DBTraceWaterfallChart.tsx index 35e116764f..2a1917df7c 100644 --- a/packages/app/src/components/DBTraceWaterfallChart.tsx +++ b/packages/app/src/components/DBTraceWaterfallChart.tsx @@ -1,9 +1,7 @@ import { ReactNode, useCallback, - useEffect, useMemo, - useRef, useState, } from 'react'; import _, { omit } from 'lodash'; @@ -58,6 +56,7 @@ import { TRACE_WATERFALL_ROW_LIMIT, } from '@/components/traceWaterfallLimits'; import useOffsetPaginatedQuery from '@/hooks/useOffsetPaginatedQuery'; +import { useRowHighlightHint } from '@/hooks/useRowHighlightHint'; import useRowWhere, { WithClause } from '@/hooks/useRowWhere'; import useWaterfallSearchState from '@/hooks/useWaterfallSearchState'; import { @@ -909,53 +908,13 @@ export function DBTraceWaterfallChartContainer({ logRowsMeta, ]); - // Auto-select the originating span once when the panel opens — but only if - // nothing is already selected. Two cases must NOT trigger a (re-)select: - // - the user explicitly cleared the selection (closing the span detail), and - // - a selection was restored from the URL on load (deep link / reload). - // We mark the hint applied as soon as a selection exists or we apply it - // ourselves, so it never fires twice for the same hint; it re-fires only when - // the hint genuinely changes (different originating span / trace). - const appliedHighlightHintRef = useRef(null); - - useEffect(() => { - appliedHighlightHintRef.current = null; - }, [traceId]); - - useEffect(() => { - if (!initialRowHighlightHint || !onClick) { - return; - } - - const hintKey = `${initialRowHighlightHint.timestamp}|${initialRowHighlightHint.spanId}|${initialRowHighlightHint.body}`; - if (appliedHighlightHintRef.current === hintKey) { - return; - } - - // A selection already exists (restored from the URL, or user-chosen). Honor - // it and record the hint so we never override it — now or later. - if (highlightedRowWhere != null) { - appliedHighlightHintRef.current = hintKey; - return; - } - - const initialRowHighlightIndex = rows.findIndex(row => { - return ( - row.Timestamp === initialRowHighlightHint.timestamp && - row.SpanId === initialRowHighlightHint.spanId && - row.Body === initialRowHighlightHint.body - ); - }); - - if (initialRowHighlightIndex !== -1) { - appliedHighlightHintRef.current = hintKey; - onClick({ - id: rows[initialRowHighlightIndex].id, - type: rows[initialRowHighlightIndex].type ?? '', - aliasWith: rows[initialRowHighlightIndex].aliasWith, - }); - } - }, [initialRowHighlightHint, rows, onClick, highlightedRowWhere]); + useRowHighlightHint({ + traceId, + initialRowHighlightHint, + highlightedRowWhere, + rows, + onClick, + }); // 3 Edge-cases // 1. No spans, just logs (ex. sampling) diff --git a/packages/app/src/components/ExpandableRowTable.tsx b/packages/app/src/components/ExpandableRowTable.tsx index 4649cf5a7b..51f979035c 100644 --- a/packages/app/src/components/ExpandableRowTable.tsx +++ b/packages/app/src/components/ExpandableRowTable.tsx @@ -7,31 +7,19 @@ import { IconArrowsMaximize, IconChevronRight } from '@tabler/icons-react'; import { INTERNAL_ROW_FIELDS } from '@/hooks/useRowWhere'; import { parseAsStringEncoded } from '@/utils/queryParsers'; -import { - eventRowWhereForOpenedRow, - eventRowWhereParser, -} from './eventRowWhere'; - import styles from '@styles/LogTable.module.scss'; // Hook that provides a function to open the sidebar with specific row details const useSidebarOpener = () => { - const [rowId, setRowId] = useQueryState('rowWhere', parseAsStringEncoded); + const [, setRowId] = useQueryState('rowWhere', parseAsStringEncoded); const [, setRowSource] = useQueryState('rowSource'); - const [, setEventRowWhere] = useQueryState( - 'eventRowWhere', - eventRowWhereParser, - ); return useCallback( (rowWhere: string, sourceId?: string) => { setRowId(rowWhere); setRowSource(sourceId ?? null); - setEventRowWhere(current => - eventRowWhereForOpenedRow(current, rowId, rowWhere), - ); }, - [rowId, setRowId, setRowSource, setEventRowWhere], + [setRowId, setRowSource], ); }; diff --git a/packages/app/src/components/__tests__/DBSqlRowTableWithSidebar.test.tsx b/packages/app/src/components/__tests__/DBSqlRowTableWithSidebar.test.tsx deleted file mode 100644 index 530e7285f8..0000000000 --- a/packages/app/src/components/__tests__/DBSqlRowTableWithSidebar.test.tsx +++ /dev/null @@ -1,198 +0,0 @@ -import { BuilderChartConfigWithDateRange } from '@hyperdx/common-utils/dist/types'; -import { fireEvent, screen } from '@testing-library/react'; - -import DBSqlRowTableWithSideBar from '@/components/DBSqlRowTableWithSidebar'; - -const FIRST_SPAN = "SpanId='8cea712d1d4df628'"; -const OTHER_SPAN = "SpanId='b2e2b0df5304a26a'"; -// Waterfall ids omit SpanAttributes/SpanEvents and are not the search row's where. -const WATERFALL_SPAN = - "TraceId='trace-123' AND SpanId='b2e2b0df5304a26a' AND Timestamp='2024-01-01'"; - -type StoredSpan = { - id: string; - type: string; - aliasWith: unknown[]; - traceId: string; -}; - -const mockQuery: Record = {}; -const mockListeners = new Set<() => void>(); - -jest.mock('nuqs', () => { - const React = jest.requireActual('react'); - const current = (key: string) => - Object.prototype.hasOwnProperty.call(mockQuery, key) - ? mockQuery[key] - : null; - return { - useQueryState: (key: string) => { - const [, rerender] = React.useState(0); - React.useEffect(() => { - const listener = () => rerender((tick: number) => tick + 1); - mockListeners.add(listener); - return () => { - mockListeners.delete(listener); - }; - }, [rerender]); - - const setValue = React.useCallback( - (next: unknown) => { - const previous = current(key); - const resolved = typeof next === 'function' ? next(previous) : next; - if (Object.is(resolved, previous)) return; - mockQuery[key] = resolved; - mockListeners.forEach(listener => listener()); - }, - [key], - ); - - return [current(key), setValue]; - }, - }; -}); - -jest.mock('@/source', () => ({ - useSource: () => ({ - data: { id: 'trace-source', kind: 'trace' }, - isLoading: false, - }), -})); - -jest.mock('../DBRowTable', () => ({ - DBSqlRowTable: ({ - onRowDetailsClick, - }: { - onRowDetailsClick: (row: { where: string; aliasWith: never[] }) => void; - }) => ( - <> - - - - ), -})); - -jest.mock('../DBRowSidePanel', () => { - const React = jest.requireActual('react'); - return { - __esModule: true, - default: ({ - onClose, - }: { - onClose: () => void; - rowId?: string; - }) => ( - - ), - RowSidePanelContext: React.createContext({}), - }; -}); - -jest.mock('../DBRowDataPanel', () => ({ - useRowData: () => ({ isError: false, error: null }), - RowDataPanel: () => null, -})); - -jest.mock('../DBRowOverviewPanel', () => ({ - RowOverviewPanel: () => null, -})); - -jest.mock('../DBRowSidePanelErrorState', () => ({ - DBRowSidePanelErrorState: () => null, -})); - -const chartConfig = { - select: [], - where: '', - whereLanguage: 'sql', - dateRange: [new Date(0), new Date(1)], -} as BuilderChartConfigWithDateRange; - -function span(id: string): StoredSpan { - return { - id, - type: 'trace', - aliasWith: [], - traceId: 'trace-123', - }; -} - -function renderResults() { - renderWithMantine( - , - ); -} - -describe('DBSqlRowTableWithSideBar span selection', () => { - beforeEach(() => { - Object.keys(mockQuery).forEach(key => { - delete mockQuery[key]; - }); - mockListeners.clear(); - }); - - it('drops the span when one result is closed and another span of the same trace is opened', () => { - mockQuery.rowWhere = FIRST_SPAN; - mockQuery.eventRowWhere = span(FIRST_SPAN); - renderResults(); - - expect(mockQuery.eventRowWhere).toEqual(span(FIRST_SPAN)); - - fireEvent.click(screen.getByRole('button', { name: 'close result' })); - expect(mockQuery.eventRowWhere).toEqual(span(FIRST_SPAN)); - expect(mockQuery.rowWhere).toBeNull(); - - fireEvent.click(screen.getByRole('button', { name: 'open other span' })); - expect(mockQuery.eventRowWhere).toBeNull(); - expect(mockQuery.rowWhere).toBe(OTHER_SPAN); - }); - - it('keeps the waterfall span when the result is closed', () => { - mockQuery.rowWhere = FIRST_SPAN; - mockQuery.eventRowWhere = span(WATERFALL_SPAN); - renderResults(); - - fireEvent.click(screen.getByRole('button', { name: 'close result' })); - - expect(mockQuery.rowWhere).toBeNull(); - expect(mockQuery.eventRowWhere).toEqual(span(WATERFALL_SPAN)); - }); - - it('keeps the waterfall span when the same result is opened again', () => { - mockQuery.rowWhere = FIRST_SPAN; - mockQuery.eventRowWhere = span(WATERFALL_SPAN); - renderResults(); - - fireEvent.click(screen.getByRole('button', { name: 'open first span' })); - - expect(mockQuery.eventRowWhere).toEqual(span(WATERFALL_SPAN)); - expect(mockQuery.rowWhere).toBe(FIRST_SPAN); - }); - - it('drops the waterfall span when a different result is opened', () => { - mockQuery.rowWhere = FIRST_SPAN; - mockQuery.eventRowWhere = span(OTHER_SPAN); - renderResults(); - - fireEvent.click(screen.getByRole('button', { name: 'open other span' })); - - expect(mockQuery.eventRowWhere).toBeNull(); - expect(mockQuery.rowWhere).toBe(OTHER_SPAN); - }); -}); diff --git a/packages/app/src/components/eventRowWhere.ts b/packages/app/src/components/eventRowWhere.ts index 8e7e231194..dfe6d1e592 100644 --- a/packages/app/src/components/eventRowWhere.ts +++ b/packages/app/src/components/eventRowWhere.ts @@ -20,24 +20,3 @@ export type EventRowWhere = z.infer; export const eventRowWhereParser = parseAsJsonEncoded( eventRowWhereSchema.parse, ); - -// `openRowWhere` is the search result already open. `openedRowWhere` is the -// result being opened. Both come from the search table. The waterfall's -// `eventRowWhere.id` is built from different columns, so comparing it to -// either string never matches and would always clear the span. -// Opening a different result of the same trace must drop the previous span. -// Re-opening the result already open must keep the span picked in the waterfall. -export function eventRowWhereForOpenedRow( - current: T | null, - openRowWhere: string | null, - openedRowWhere: string | null, -): T | null { - if ( - current != null && - openRowWhere != null && - openRowWhere === openedRowWhere - ) { - return current; - } - return null; -} diff --git a/packages/app/src/hooks/__tests__/useRowHighlightHint.test.tsx b/packages/app/src/hooks/__tests__/useRowHighlightHint.test.tsx new file mode 100644 index 0000000000..9481750a1e --- /dev/null +++ b/packages/app/src/hooks/__tests__/useRowHighlightHint.test.tsx @@ -0,0 +1,177 @@ +import { renderHook } from '@testing-library/react'; + +import { + RowHighlightHint, + useRowHighlightHint, +} from '@/hooks/useRowHighlightHint'; + +const hintA: RowHighlightHint = { + timestamp: 't1', + spanId: 'span-a', + body: 'a', +}; +const hintB: RowHighlightHint = { + timestamp: 't2', + spanId: 'span-b', + body: 'b', +}; + +function row(id: string, hint: RowHighlightHint) { + return { + id, + type: 'trace', + aliasWith: [], + Timestamp: hint.timestamp, + SpanId: hint.spanId, + Body: hint.body, + }; +} + +const rows = [row('where-a', hintA), row('where-b', hintB)]; + +function renderHint( + overrides: Partial[0]> = {}, +) { + const onClick = jest.fn(); + const view = renderHook( + (props: Parameters[0]) => + useRowHighlightHint(props), + { + initialProps: { + traceId: 'trace-1', + initialRowHighlightHint: hintA, + highlightedRowWhere: null as string | null, + rows, + onClick, + ...overrides, + }, + }, + ); + return { ...view, onClick }; +} + +describe('useRowHighlightHint', () => { + it('yields to a selection that is already present on the first hint', () => { + const { onClick, rerender } = renderHint({ + highlightedRowWhere: 'where-existing', + }); + + expect(onClick).not.toHaveBeenCalled(); + + rerender({ + traceId: 'trace-1', + initialRowHighlightHint: hintA, + highlightedRowWhere: 'where-existing', + rows, + onClick, + }); + + expect(onClick).not.toHaveBeenCalled(); + }); + + it('selects the matching row when nothing is selected', () => { + const { onClick } = renderHint(); + + expect(onClick).toHaveBeenCalledWith({ + id: 'where-a', + type: 'trace', + aliasWith: [], + }); + }); + + it('replaces the selection when a later hint arrives', () => { + const { onClick, rerender } = renderHint(); + + rerender({ + traceId: 'trace-1', + initialRowHighlightHint: hintB, + highlightedRowWhere: 'where-a', + rows, + onClick, + }); + + expect(onClick).toHaveBeenLastCalledWith({ + id: 'where-b', + type: 'trace', + aliasWith: [], + }); + }); + + it('does not select again for the same hint', () => { + const { onClick, rerender } = renderHint(); + + rerender({ + traceId: 'trace-1', + initialRowHighlightHint: hintA, + highlightedRowWhere: 'where-a', + rows, + onClick, + }); + + expect(onClick).toHaveBeenCalledTimes(1); + }); + + it('yields again after the trace changes, then follows the next hint', () => { + const { onClick, rerender } = renderHint({ + highlightedRowWhere: 'where-existing', + }); + + rerender({ + traceId: 'trace-2', + initialRowHighlightHint: hintB, + highlightedRowWhere: 'where-existing', + rows, + onClick, + }); + + expect(onClick).not.toHaveBeenCalled(); + + rerender({ + traceId: 'trace-2', + initialRowHighlightHint: hintA, + highlightedRowWhere: 'where-existing', + rows, + onClick, + }); + + expect(onClick).toHaveBeenCalledWith({ + id: 'where-a', + type: 'trace', + aliasWith: [], + }); + }); + + it('waits until the hinted row is loaded', () => { + const { onClick, rerender } = renderHint({ rows: [] }); + + expect(onClick).not.toHaveBeenCalled(); + + rerender({ + traceId: 'trace-1', + initialRowHighlightHint: hintA, + highlightedRowWhere: null, + rows, + onClick, + }); + + expect(onClick).toHaveBeenCalledWith({ + id: 'where-a', + type: 'trace', + aliasWith: [], + }); + }); + + it('does not restore a selection the user cleared while the hint is unchanged', () => { + const { onClick, rerender } = renderHint(); + + rerender({ + traceId: 'trace-1', + initialRowHighlightHint: hintA, + highlightedRowWhere: null, + rows, + onClick, + }); + + expect(onClick).toHaveBeenCalledTimes(1); + }); +}); diff --git a/packages/app/src/hooks/useRowHighlightHint.ts b/packages/app/src/hooks/useRowHighlightHint.ts new file mode 100644 index 0000000000..238d6fc84a --- /dev/null +++ b/packages/app/src/hooks/useRowHighlightHint.ts @@ -0,0 +1,81 @@ +import { useEffect, useRef } from 'react'; + +import { WithClause } from '@/hooks/useRowWhere'; + +export type RowHighlightHint = { + timestamp: string; + spanId: string; + body: string; +}; + +type HighlightRow = { + id: string; + type?: string; + aliasWith: WithClause[]; + Timestamp?: unknown; + SpanId?: unknown; + Body?: unknown; +}; + +// Only the first hint after mount or a trace change yields to an existing +// selection (restored from the URL, or picked by the user). A later hint +// change means a different row was opened, so it replaces the selection. +export function useRowHighlightHint({ + traceId, + initialRowHighlightHint, + highlightedRowWhere, + rows, + onClick, +}: { + traceId: string; + initialRowHighlightHint?: RowHighlightHint; + highlightedRowWhere?: string | null; + rows: HighlightRow[]; + onClick?: (rowWhere: { + id: string; + type: string; + aliasWith: WithClause[]; + }) => void; +}) { + const appliedHighlightHintRef = useRef(null); + + useEffect(() => { + appliedHighlightHintRef.current = null; + }, [traceId]); + + useEffect(() => { + if (!initialRowHighlightHint || !onClick) { + return; + } + + const hintKey = `${initialRowHighlightHint.timestamp}|${initialRowHighlightHint.spanId}|${initialRowHighlightHint.body}`; + if (appliedHighlightHintRef.current === hintKey) { + return; + } + + if ( + appliedHighlightHintRef.current == null && + highlightedRowWhere != null + ) { + appliedHighlightHintRef.current = hintKey; + return; + } + + const initialRowHighlightIndex = rows.findIndex(row => { + return ( + row.Timestamp === initialRowHighlightHint.timestamp && + row.SpanId === initialRowHighlightHint.spanId && + row.Body === initialRowHighlightHint.body + ); + }); + + if (initialRowHighlightIndex !== -1) { + appliedHighlightHintRef.current = hintKey; + onClick({ + id: rows[initialRowHighlightIndex].id, + type: rows[initialRowHighlightIndex].type ?? '', + aliasWith: rows[initialRowHighlightIndex].aliasWith, + }); + } + }, [initialRowHighlightHint, rows, onClick, highlightedRowWhere]); +} diff --git a/packages/app/tests/e2e/components/SidePanelComponent.ts b/packages/app/tests/e2e/components/SidePanelComponent.ts index d36ca993bc..a947be46cb 100644 --- a/packages/app/tests/e2e/components/SidePanelComponent.ts +++ b/packages/app/tests/e2e/components/SidePanelComponent.ts @@ -218,6 +218,35 @@ export class SidePanelComponent { await this.getWaterfallSpan(name).first().click({ timeout: 10_000 }); } + /** + * The waterfall label currently marked selected. The highlighted class sits + * on the span's own `role="button"` row. + */ + getHighlightedWaterfallSpan(name: string) { + return this.panelContainer + .locator('[role="button"][class*="traceTimelineLabelHighlighted"]') + .filter({ hasText: name }); + } + + /** + * The span detail drawer beside the waterfall. Trace sources don't render an + * Overview tab of their own, so `tab-overview` plus the close control only + * exist together on this drawer. The drawer is the innermost container that + * holds both — ancestors also match, and the tab bar itself does not hold + * the overview body. + */ + get spanDetailsPanel() { + const closeSpanDetails = this.page.getByRole('button', { + name: 'Close span details', + }); + return this.panelContainer + .locator('div') + .filter({ has: closeSpanDetails }) + .filter({ has: this.page.getByTestId('tab-overview') }) + .filter({ has: this.page.locator('.flex-grow-1') }) + .last(); + } + /** * Rows in the span detail's "Span Links" section. Each row's open action * (`span-link-open-trace`) shows the linked span's name once the target diff --git a/packages/app/tests/e2e/components/TableComponent.ts b/packages/app/tests/e2e/components/TableComponent.ts index 2259bcfc2b..f0a24b3028 100644 --- a/packages/app/tests/e2e/components/TableComponent.ts +++ b/packages/app/tests/e2e/components/TableComponent.ts @@ -67,6 +67,10 @@ export class TableComponent { await this.firstRow.click(); } + async clickRowContaining(text: string) { + await this.getRows().filter({ hasText: text }).first().click(); + } + /** * Open a row in the side panel via its hover button. The button only exists * once the `expand` row-click preference is set — without it a plain diff --git a/packages/app/tests/e2e/features/traces-workflow.spec.ts b/packages/app/tests/e2e/features/traces-workflow.spec.ts index 6e4261170e..ec8b9d39b9 100644 --- a/packages/app/tests/e2e/features/traces-workflow.spec.ts +++ b/packages/app/tests/e2e/features/traces-workflow.spec.ts @@ -155,3 +155,92 @@ test.describe('Advanced Search Workflow - Traces', { tag: '@traces' }, () => { }); }); }); + +// trace-0 is four spans in one trace. Opening a result and then another must +// follow the span that was just opened, including a click inside the waterfall +// that is already on screen. +test.describe( + 'Trace waterfall span selection', + { tag: ['@traces', '@full-stack'] }, + () => { + let searchPage: SearchPage; + + test.beforeEach(async ({ page }) => { + searchPage = new SearchPage(page); + await searchPage.goto(); + await searchPage.selectSource(DEFAULT_TRACES_SOURCE_NAME); + await searchPage.timePicker.selectRelativeTime('Last 1 days'); + await searchPage.switchToLuceneMode(); + }); + + async function openSpanInWaterfall(spanId: string, spanName: string) { + await searchPage.performSearch(`SpanId:"${spanId}"`); + await expect(searchPage.table.firstRow).toBeVisible(); + await searchPage.table.clickFirstRow(); + await expect(searchPage.sidePanel.container).toBeVisible(); + await expect(searchPage.sidePanel.getTab('trace')).toBeVisible({ + timeout: 15_000, + }); + await searchPage.sidePanel.clickTab('trace'); + await expect( + searchPage.sidePanel.getWaterfallSpan(spanName).first(), + ).toBeVisible({ timeout: 15_000 }); + } + + test('Opening a span, closing it, opening another span from the same trace', async () => { + await openSpanInWaterfall('span-0-0', 'GET /api/logs'); + await expect( + searchPage.sidePanel.getHighlightedWaterfallSpan('GET /api/logs'), + ).toBeVisible({ timeout: 15_000 }); + await expect(searchPage.sidePanel.spanDetailsPanel).toContainText( + 'GET /api/logs', + { timeout: 15_000 }, + ); + + await searchPage.sidePanel.close(); + await expect(searchPage.sidePanel.container).toBeHidden(); + + await openSpanInWaterfall('span-0-1', 'POST /api/traces'); + await expect( + searchPage.sidePanel.getHighlightedWaterfallSpan('POST /api/traces'), + ).toBeVisible({ timeout: 15_000 }); + await expect( + searchPage.sidePanel.getHighlightedWaterfallSpan('GET /api/logs'), + ).toBeHidden(); + await expect(searchPage.sidePanel.spanDetailsPanel).toContainText( + 'POST /api/traces', + { timeout: 15_000 }, + ); + await expect(searchPage.sidePanel.spanDetailsPanel).not.toContainText( + 'GET /api/logs', + ); + }); + + test('Opening a span, then selecting another span from the same trace without closing the side panel', async () => { + await searchPage.performSearch('TraceId:"trace-0"'); + await searchPage.table.clickRowContaining('GET /api/logs'); + await expect(searchPage.sidePanel.container).toBeVisible(); + await expect(searchPage.sidePanel.getTab('trace')).toBeVisible({ + timeout: 15_000, + }); + await searchPage.sidePanel.clickTab('trace'); + await expect( + searchPage.sidePanel.getHighlightedWaterfallSpan('GET /api/logs'), + ).toBeVisible({ timeout: 15_000 }); + + await searchPage.table.clickRowContaining('POST /api/traces'); + + await expect(searchPage.sidePanel.container).toBeVisible(); + await expect( + searchPage.sidePanel.getHighlightedWaterfallSpan('POST /api/traces'), + ).toBeVisible({ timeout: 15_000 }); + await expect( + searchPage.sidePanel.getHighlightedWaterfallSpan('GET /api/logs'), + ).toBeHidden(); + await expect(searchPage.sidePanel.spanDetailsPanel).toContainText( + 'POST /api/traces', + { timeout: 15_000 }, + ); + }); + }, +); From 7c5d48fa9276de7eee3648203f266927e4c78e0c Mon Sep 17 00:00:00 2001 From: Tushar Bahl Date: Fri, 9 Oct 2026 10:25:25 +0530 Subject: [PATCH 05/16] fix: changes in trace view mount Earlier: The trace view unmounts while a new row loads and when you leave the Trace tab. When it mounts again, the highlight hook treats the previous span as a restored selection and keeps it. Now: Opening a different result now clears that span in DBRowSidePanel before the trace view comes back. The new row's hint then selects it. The first open still keeps a span that arrived in the URL, and switching tabs without changing the result still keeps the span you picked in the waterfall. Also a remount test covers cases: an existing selection is kept, and a cleared selection follows the new hint. --- .../trace-span-follows-opened-result.md | 2 +- .../app/src/components/DBRowSidePanel.tsx | 13 +++++++++ .../__tests__/useRowHighlightHint.test.tsx | 29 +++++++++++++++++++ packages/app/src/hooks/useRowHighlightHint.ts | 4 ++- 4 files changed, 46 insertions(+), 2 deletions(-) diff --git a/.changeset/trace-span-follows-opened-result.md b/.changeset/trace-span-follows-opened-result.md index 202929364d..f1a2f0ca15 100644 --- a/.changeset/trace-span-follows-opened-result.md +++ b/.changeset/trace-span-follows-opened-result.md @@ -2,4 +2,4 @@ '@hyperdx/app': patch --- -Fix the trace waterfall sticking on the previously opened span when another span of the same trace is opened. Closing the result clears the selected span. Opening a different result while the drawer stays open selects that result's span. +Fix the trace waterfall sticking on the previously opened span when another span of the same trace is opened. Closing the result clears the selected span. Opening a different result while the drawer stays open selects that result's span, including after the trace view remounts for a load or a tab change. diff --git a/packages/app/src/components/DBRowSidePanel.tsx b/packages/app/src/components/DBRowSidePanel.tsx index 81374577ed..e51f977cd7 100644 --- a/packages/app/src/components/DBRowSidePanel.tsx +++ b/packages/app/src/components/DBRowSidePanel.tsx @@ -1331,6 +1331,19 @@ export default function DBRowSidePanelErrorBoundary({ eventRowWhereParser, ); + // The trace view unmounts while a new row loads and when the user leaves the + // Trace tab. A fresh mount treats the previous span as a restored selection + // and keeps it. Drop that span when the opened row changes; the first mount + // keeps a selection that arrived with the URL. + const openedRowIdRef = useRef(rowId); + useEffect(() => { + if (openedRowIdRef.current === rowId) { + return; + } + openedRowIdRef.current = rowId; + setEventRowWhere(null); + }, [rowId, setEventRowWhere]); + const sidePanelStack = useSidePanelStack({ initialRowId: rowId }); const _onClose = useCallback(() => { diff --git a/packages/app/src/hooks/__tests__/useRowHighlightHint.test.tsx b/packages/app/src/hooks/__tests__/useRowHighlightHint.test.tsx index 9481750a1e..c9e13ee35d 100644 --- a/packages/app/src/hooks/__tests__/useRowHighlightHint.test.tsx +++ b/packages/app/src/hooks/__tests__/useRowHighlightHint.test.tsx @@ -161,6 +161,35 @@ describe('useRowHighlightHint', () => { }); }); + it('keeps an existing selection when remounted with a different hint', () => { + const first = renderHint({ highlightedRowWhere: 'where-a' }); + expect(first.onClick).not.toHaveBeenCalled(); + first.unmount(); + + const remounted = renderHint({ + initialRowHighlightHint: hintB, + highlightedRowWhere: 'where-a', + }); + + expect(remounted.onClick).not.toHaveBeenCalled(); + }); + + it('selects the new hint when remounted after the selection was cleared', () => { + const first = renderHint({ highlightedRowWhere: 'where-a' }); + first.unmount(); + + const remounted = renderHint({ + initialRowHighlightHint: hintB, + highlightedRowWhere: null, + }); + + expect(remounted.onClick).toHaveBeenCalledWith({ + id: 'where-b', + type: 'trace', + aliasWith: [], + }); + }); + it('does not restore a selection the user cleared while the hint is unchanged', () => { const { onClick, rerender } = renderHint(); diff --git a/packages/app/src/hooks/useRowHighlightHint.ts b/packages/app/src/hooks/useRowHighlightHint.ts index 238d6fc84a..3a10311636 100644 --- a/packages/app/src/hooks/useRowHighlightHint.ts +++ b/packages/app/src/hooks/useRowHighlightHint.ts @@ -19,7 +19,9 @@ type HighlightRow = { // Only the first hint after mount or a trace change yields to an existing // selection (restored from the URL, or picked by the user). A later hint -// change means a different row was opened, so it replaces the selection. +// change while this hook stays mounted replaces the selection. A remount +// cannot tell a new row from that restored selection, so the side panel +// clears the span when its opened row changes. export function useRowHighlightHint({ traceId, initialRowHighlightHint, From 5c7646cff774c8a8182d3924f7dc533824363803 Mon Sep 17 00:00:00 2001 From: Tushar Bahl Date: Fri, 9 Oct 2026 12:57:59 +0530 Subject: [PATCH 06/16] fix: stale span selection is fixed in both drawers The reset now lives on the inner row panel, which the search drawer and the session replay drawer both render. Changing the opened row clears the previous span in a layout effect, before the waterfall applies the new hint. The first open still keeps a span that came in on the URL. Closing the panel, or stepping back from an event to the session, clears it too. RowHighlightHint is now the only definition of that hint shape. DBTracePanel and DBTraceWaterfallChart both use it. --- .../trace-span-follows-opened-result.md | 2 +- .../app/src/components/DBRowSidePanel.tsx | 35 +-- packages/app/src/components/DBTracePanel.tsx | 9 +- .../src/components/DBTraceWaterfallChart.tsx | 18 +- .../DBRowSidePanel.spanSelection.test.tsx | 227 ++++++++++++++++++ .../useOpenedRowSpanSelection.test.tsx | 150 ++++++++++++ .../src/hooks/useOpenedRowSpanSelection.ts | 32 +++ 7 files changed, 437 insertions(+), 36 deletions(-) create mode 100644 packages/app/src/components/__tests__/DBRowSidePanel.spanSelection.test.tsx create mode 100644 packages/app/src/hooks/__tests__/useOpenedRowSpanSelection.test.tsx create mode 100644 packages/app/src/hooks/useOpenedRowSpanSelection.ts diff --git a/.changeset/trace-span-follows-opened-result.md b/.changeset/trace-span-follows-opened-result.md index f1a2f0ca15..0e6eb514c1 100644 --- a/.changeset/trace-span-follows-opened-result.md +++ b/.changeset/trace-span-follows-opened-result.md @@ -2,4 +2,4 @@ '@hyperdx/app': patch --- -Fix the trace waterfall sticking on the previously opened span when another span of the same trace is opened. Closing the result clears the selected span. Opening a different result while the drawer stays open selects that result's span, including after the trace view remounts for a load or a tab change. +Fix the trace waterfall sticking on the previously opened span when another span of the same trace is opened, including from a session replay. Closing the result clears the selected span. Opening a different result selects that result's span, including when the waterfall stays mounted on cached data. diff --git a/packages/app/src/components/DBRowSidePanel.tsx b/packages/app/src/components/DBRowSidePanel.tsx index e51f977cd7..cbeebeee17 100644 --- a/packages/app/src/components/DBRowSidePanel.tsx +++ b/packages/app/src/components/DBRowSidePanel.tsx @@ -38,6 +38,7 @@ import { import { IconCopy, IconKeyboard, IconShare, IconX } from '@tabler/icons-react'; import { useCloseOnClickOutside } from '@/hooks/useCloseOnClickOutside'; +import { useOpenedRowSpanSelection } from '@/hooks/useOpenedRowSpanSelection'; import useResizable from '@/hooks/useResizable'; import { WithClause } from '@/hooks/useRowWhere'; import useSidePanelStack, { @@ -78,12 +79,12 @@ import { DrawerFullWidthToggle, INITIAL_DRAWER_WIDTH_PERCENT, } from './DrawerUtils'; +import { eventRowWhereParser } from './eventRowWhere'; import LogLevel from './LogLevel'; import SidePanelBreadcrumbs, { BreadcrumbItem } from './SidePanelBreadcrumbs'; import { SpanLinkData } from './SpanLinksSubpanel'; import TraceLogsPanel from './TraceLogsPanel'; import { ViewTraceCalloutButton } from './ViewTraceCalloutButton'; -import { eventRowWhereParser } from './eventRowWhere'; import styles from '@/../styles/LogSidePanel.module.scss'; @@ -404,18 +405,25 @@ export const DBRowSidePanelInner = ({ [pushSource], ); + const clearSpanSelection = useOpenedRowSpanSelection(initialRowId); + const closePanel = useCallback(() => { + clearSpanSelection(); + onClose(); + }, [clearSpanSelection, onClose]); + const handlePanelBack = useCallback(() => { // Pop one level (nav → source), restoring the tab active before that level // was entered. When the trail is empty, leave the panel: hand off to an // embedding parent (session) or close. if (popOne() === 'none') { + clearSpanSelection(); if (onNavigateToParent) { onNavigateToParent(); } else { onClose(); } } - }, [popOne, onNavigateToParent, onClose]); + }, [popOne, onNavigateToParent, onClose, clearSpanSelection]); useHotkeys(['esc'], handlePanelBack); @@ -848,13 +856,19 @@ export const DBRowSidePanelInner = ({ ), - [allBreadcrumbs, handlePanelBack, onClose, isFullWidth, onToggleFullWidth], + [ + allBreadcrumbs, + handlePanelBack, + closePanel, + isFullWidth, + onToggleFullWidth, + ], ); if (isRowLoading || isResolvingSource) { @@ -1331,19 +1345,6 @@ export default function DBRowSidePanelErrorBoundary({ eventRowWhereParser, ); - // The trace view unmounts while a new row loads and when the user leaves the - // Trace tab. A fresh mount treats the previous span as a restored selection - // and keeps it. Drop that span when the opened row changes; the first mount - // keeps a selection that arrived with the URL. - const openedRowIdRef = useRef(rowId); - useEffect(() => { - if (openedRowIdRef.current === rowId) { - return; - } - openedRowIdRef.current = rowId; - setEventRowWhere(null); - }, [rowId, setEventRowWhere]); - const sidePanelStack = useSidePanelStack({ initialRowId: rowId }); const _onClose = useCallback(() => { diff --git a/packages/app/src/components/DBTracePanel.tsx b/packages/app/src/components/DBTracePanel.tsx index 008a7ecc66..f29346d023 100644 --- a/packages/app/src/components/DBTracePanel.tsx +++ b/packages/app/src/components/DBTracePanel.tsx @@ -36,6 +36,7 @@ import { import { DBTraceWaterfallChartContainer } from '@/components/DBTraceWaterfallChart'; import { SQLInlineEditorControlled } from '@/components/SQLEditor/SQLInlineEditor'; import useResizable from '@/hooks/useResizable'; +import { RowHighlightHint } from '@/hooks/useRowHighlightHint'; import { WithClause } from '@/hooks/useRowWhere'; import { useSource, useUpdateSource } from '@/source'; import TabBar from '@/TabBar'; @@ -47,11 +48,11 @@ import { deriveRowSidePanelContextForSource, RowSidePanelContext, } from './DBRowSidePanel'; +import { eventRowWhereParser } from './eventRowWhere'; import SourceSchemaPreview, { isSourceSchemaPreviewEnabled, } from './SourceSchemaPreview'; import { SourceSelectControlled } from './SourceSelect'; -import { EventRowWhere, eventRowWhereParser } from './eventRowWhere'; import resizeStyles from '@/../styles/ResizablePanel.module.scss'; @@ -231,11 +232,7 @@ export default function DBTracePanel({ // Passed in from side panel to try to identify which // span in the chart to highlight first without constructing // a full row where clause - initialRowHighlightHint?: { - timestamp: string; - spanId: string; - body: string; - }; + initialRowHighlightHint?: RowHighlightHint; emptyState?: ReactNode; 'data-testid'?: string; }) { diff --git a/packages/app/src/components/DBTraceWaterfallChart.tsx b/packages/app/src/components/DBTraceWaterfallChart.tsx index 2a1917df7c..22d6b6f396 100644 --- a/packages/app/src/components/DBTraceWaterfallChart.tsx +++ b/packages/app/src/components/DBTraceWaterfallChart.tsx @@ -1,9 +1,4 @@ -import { - ReactNode, - useCallback, - useMemo, - useState, -} from 'react'; +import { ReactNode, useCallback, useMemo, useState } from 'react'; import _, { omit } from 'lodash'; import { useForm } from 'react-hook-form'; import SqlString from 'sqlstring'; @@ -56,7 +51,10 @@ import { TRACE_WATERFALL_ROW_LIMIT, } from '@/components/traceWaterfallLimits'; import useOffsetPaginatedQuery from '@/hooks/useOffsetPaginatedQuery'; -import { useRowHighlightHint } from '@/hooks/useRowHighlightHint'; +import { + RowHighlightHint, + useRowHighlightHint, +} from '@/hooks/useRowHighlightHint'; import useRowWhere, { WithClause } from '@/hooks/useRowWhere'; import useWaterfallSearchState from '@/hooks/useWaterfallSearchState'; import { @@ -642,11 +640,7 @@ export function DBTraceWaterfallChartContainer({ aliasWith: WithClause[]; }) => void; highlightedRowWhere?: string | null; - initialRowHighlightHint?: { - timestamp: string; - spanId: string; - body: string; - }; + initialRowHighlightHint?: RowHighlightHint; emptyState?: ReactNode; /** Extra controls rendered in the waterfall controls bar (e.g. the correlated logs source selector). */ controlsExtra?: ReactNode; diff --git a/packages/app/src/components/__tests__/DBRowSidePanel.spanSelection.test.tsx b/packages/app/src/components/__tests__/DBRowSidePanel.spanSelection.test.tsx new file mode 100644 index 0000000000..d63a0e0964 --- /dev/null +++ b/packages/app/src/components/__tests__/DBRowSidePanel.spanSelection.test.tsx @@ -0,0 +1,227 @@ +import { TSource } from '@hyperdx/common-utils/dist/types'; +import { MantineProvider } from '@mantine/core'; +import { fireEvent, render, screen } from '@testing-library/react'; + +// Controlled stand-in for nuqs. Values are the parsed shapes the panel reads. +// Prefixed with `mock` so the hoisted jest.mock factory can close over them. +const mockQueryStore: Record = {}; +const mockSetters: Record = {}; + +function seedParam(key: string, value: unknown) { + mockQueryStore[key] = value; +} +function setterFor(key: string) { + if (!mockSetters[key]) mockSetters[key] = jest.fn(); + return mockSetters[key]; +} +function resetQueryState() { + Object.keys(mockQueryStore).forEach(k => delete mockQueryStore[k]); + Object.keys(mockSetters).forEach(k => delete mockSetters[k]); +} + +jest.mock('nuqs', () => { + const actual = jest.requireActual('nuqs'); + return { + ...actual, + useQueryState: (key: string, parser?: { defaultValue?: unknown }) => { + const hasValue = Object.prototype.hasOwnProperty.call( + mockQueryStore, + key, + ); + const fallback = + parser && 'defaultValue' in parser ? parser.defaultValue : null; + const value = hasValue ? mockQueryStore[key] : (fallback ?? null); + const setters = mockSetters; + if (!setters[key]) setters[key] = jest.fn(); + return [value, setters[key]]; + }, + }; +}); + +jest.mock('../DBRowDataPanel', () => ({ + __esModule: true, + useRowData: () => ({ + data: undefined, + isLoading: false, + isSuccess: false, + isError: false, + error: null, + }), + ROW_DATA_ALIASES: { + DURATION_MS: '__hdx_duration', + SPAN_KIND: '__hdx_span_kind', + SERVICE_NAME: '__hdx_service_name', + SEVERITY_TEXT: '__hdx_severity_text', + }, + rowHasK8sContext: () => false, + RowDataPanel: () => null, +})); + +jest.mock('@/source', () => ({ + __esModule: true, + getEventBody: () => '__hdx_body', + useSource: () => ({ data: undefined, isLoading: false, isSuccess: false }), +})); + +jest.mock('../DBSessionPanel', () => ({ + __esModule: true, + useSessionId: () => ({ rumSessionId: undefined, rumServiceName: undefined }), + DBSessionPanel: () => null, +})); + +jest.mock('@/utils/highlightedAttributes', () => ({ + __esModule: true, + getHighlightedAttributesFromData: () => [], +})); + +jest.mock('../DBTracePanel', () => ({ __esModule: true, default: () => null })); +jest.mock('../ContextSidePanel', () => ({ + __esModule: true, + default: () => null, +})); +jest.mock('../DBInfraPanel', () => ({ __esModule: true, default: () => null })); +jest.mock('../DBRowOverviewPanel', () => ({ + __esModule: true, + RowOverviewPanel: () => null, +})); +jest.mock('../DBRowSidePanelErrorState', () => ({ + __esModule: true, + DBRowSidePanelErrorState: () => null, +})); +jest.mock('../DBRowSidePanelHeader', () => ({ + __esModule: true, + default: () => null, +})); +jest.mock('../LogLevel', () => ({ __esModule: true, default: () => null })); +jest.mock('../ServiceMap/ServiceMapSidePanel', () => ({ + __esModule: true, + default: () => null, +})); +jest.mock('../TimelineChart/utils', () => ({ + __esModule: true, + renderMs: () => '', +})); +jest.mock('../DrawerUtils', () => ({ + __esModule: true, + DrawerFullWidthToggle: () => null, + INITIAL_DRAWER_WIDTH_PERCENT: 50, +})); +jest.mock('@/LogSidePanelElements', () => ({ + __esModule: true, + KeyboardShortcutsModal: () => null, +})); +jest.mock('@/TabBar', () => ({ __esModule: true, default: () => null })); +jest.mock('@/useFormatTime', () => ({ + __esModule: true, + FormatTime: () => null, +})); + +import DBRowSidePanelErrorBoundary, { + DBRowSidePanelInner, +} from '@/components/DBRowSidePanel'; +import useSidePanelStack from '@/hooks/useSidePanelStack'; + +// eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion +const ROOT_SOURCE = { + id: 'log-src', + kind: 'log', + traceSourceId: 'trace-src', +} as TSource; + +const urlSpan = { + id: 'span-where', + type: 'trace', + aliasWith: [], + traceId: 'trace-1', +}; + +function renderBoundary(rowId: string) { + return render( + + + , + ); +} + +function InnerHarness({ + rowId, + onClose, + onNavigateToParent, +}: { + rowId: string; + onClose: () => void; + onNavigateToParent?: () => void; +}) { + const sidePanelStack = useSidePanelStack({ initialRowId: rowId }); + return ( + + ); +} + +describe('DBRowSidePanelErrorBoundary span selection', () => { + beforeEach(() => { + resetQueryState(); + seedParam('eventRowWhere', urlSpan); + }); + + it('keeps an eventRowWhere that arrived with the URL on the first render', () => { + renderBoundary('row-a'); + + expect(setterFor('eventRowWhere')).not.toHaveBeenCalled(); + }); + + it('clears the span when the opened row changes', () => { + const view = renderBoundary('row-a'); + + view.rerender( + + + , + ); + + expect(setterFor('eventRowWhere')).toHaveBeenCalledWith(null); + }); + + it('clears the span when the drawer closes', () => { + renderBoundary('row-a'); + + fireEvent.click(screen.getByLabelText('Close')); + + expect(setterFor('eventRowWhere')).toHaveBeenCalledWith(null); + }); + + it('clears the span when leaving an embedded event for its parent', () => { + const onNavigateToParent = jest.fn(); + render( + + + , + ); + + fireEvent.click(screen.getByLabelText('Back')); + + expect(setterFor('eventRowWhere')).toHaveBeenCalledWith(null); + expect(onNavigateToParent).toHaveBeenCalled(); + }); +}); diff --git a/packages/app/src/hooks/__tests__/useOpenedRowSpanSelection.test.tsx b/packages/app/src/hooks/__tests__/useOpenedRowSpanSelection.test.tsx new file mode 100644 index 0000000000..8d4f7abcf4 --- /dev/null +++ b/packages/app/src/hooks/__tests__/useOpenedRowSpanSelection.test.tsx @@ -0,0 +1,150 @@ +import { useQueryState } from 'nuqs'; +import { render } from '@testing-library/react'; + +import { eventRowWhereParser } from '@/components/eventRowWhere'; +import { useOpenedRowSpanSelection } from '@/hooks/useOpenedRowSpanSelection'; +import { + RowHighlightHint, + useRowHighlightHint, +} from '@/hooks/useRowHighlightHint'; + +type StoredSpan = { + id: string; + type: string; + aliasWith: unknown[]; + traceId: string; +}; + +// Mirrors nuqs: each useQueryState keeps its own state and hears updates +// through a subscription registered in useInsertionEffect, which runs before +// useLayoutEffect. Prefixed with `mock` so the hoisted jest.mock factory can +// close over it. +const mockSpanStore: { + value: unknown; + sets: unknown[]; + listeners: Set<(value: unknown) => void>; +} = { + value: null, + sets: [], + listeners: new Set(), +}; + +jest.mock('nuqs', () => { + const React = jest.requireActual('react'); + const actual = jest.requireActual('nuqs'); + return { + ...actual, + useQueryState: (key: string) => { + const [value, setValue] = React.useState( + key === 'eventRowWhere' ? mockSpanStore.value : null, + ); + React.useInsertionEffect(() => { + if (key !== 'eventRowWhere') { + return undefined; + } + const listener = (next: unknown) => setValue(next); + mockSpanStore.listeners.add(listener); + return () => { + mockSpanStore.listeners.delete(listener); + }; + }, [key]); + const set = React.useCallback( + (next: unknown) => { + if (key !== 'eventRowWhere') { + return; + } + mockSpanStore.value = next; + mockSpanStore.sets.push(next); + mockSpanStore.listeners.forEach(listener => listener(next)); + }, + [key], + ); + return [value, set]; + }, + }; +}); + +const hintA: RowHighlightHint = { + timestamp: 't1', + spanId: 'span-a', + body: 'a', +}; +const hintB: RowHighlightHint = { + timestamp: 't2', + spanId: 'span-b', + body: 'b', +}; + +function row(id: string, hint: RowHighlightHint) { + return { + id, + type: 'trace', + aliasWith: [], + Timestamp: hint.timestamp, + SpanId: hint.spanId, + Body: hint.body, + }; +} + +const rows = [row('where-a', hintA), row('where-b', hintB)]; + +const spanA: StoredSpan = { + id: 'where-a', + type: 'trace', + aliasWith: [], + traceId: 'trace-1', +}; + +function Waterfall({ hint }: { hint: RowHighlightHint }) { + const [eventRowWhere, setEventRowWhere] = useQueryState( + 'eventRowWhere', + eventRowWhereParser, + ); + const highlightedRowWhere = + eventRowWhere != null && eventRowWhere.traceId === 'trace-1' + ? eventRowWhere.id + : null; + + useRowHighlightHint({ + traceId: 'trace-1', + initialRowHighlightHint: hint, + highlightedRowWhere, + rows, + onClick: where => setEventRowWhere({ ...where, traceId: 'trace-1' }), + }); + return null; +} + +function Panel({ rowId, hint }: { rowId: string; hint: RowHighlightHint }) { + useOpenedRowSpanSelection(rowId); + return ; +} + +describe('useOpenedRowSpanSelection', () => { + beforeEach(() => { + mockSpanStore.value = spanA; + mockSpanStore.sets = []; + mockSpanStore.listeners = new Set(); + }); + + it('keeps a URL selection on the first mount', () => { + render(); + + expect(mockSpanStore.sets).toEqual([]); + expect(mockSpanStore.value).toEqual(spanA); + }); + + it('clears the previous span before a cached waterfall applies the next hint', () => { + const view = render(); + + view.rerender(); + + expect(mockSpanStore.sets[0]).toBeNull(); + expect(mockSpanStore.value).toEqual({ + id: 'where-b', + type: 'trace', + aliasWith: [], + traceId: 'trace-1', + }); + }); +}); diff --git a/packages/app/src/hooks/useOpenedRowSpanSelection.ts b/packages/app/src/hooks/useOpenedRowSpanSelection.ts new file mode 100644 index 0000000000..3038dd4cd6 --- /dev/null +++ b/packages/app/src/hooks/useOpenedRowSpanSelection.ts @@ -0,0 +1,32 @@ +import { useCallback, useLayoutEffect, useRef } from 'react'; +import { useQueryState } from 'nuqs'; + +import { eventRowWhereParser } from '@/components/eventRowWhere'; + +// The waterfall treats the first hint after mount as a restored selection and +// keeps it. Drop the previous span when the opened row changes so the next +// view follows the new row. The first mount keeps a selection that arrived +// with the URL. +// +// useLayoutEffect, not useEffect. Child effects run first, so a parent +// useEffect clears the span only after the waterfall has applied the new +// hint and marked it done. The next render then skips that hint. +export function useOpenedRowSpanSelection(rowId: string | undefined) { + const [, setEventRowWhere] = useQueryState( + 'eventRowWhere', + eventRowWhereParser, + ); + const openedRowIdRef = useRef(rowId); + + useLayoutEffect(() => { + if (openedRowIdRef.current === rowId) { + return; + } + openedRowIdRef.current = rowId; + setEventRowWhere(null); + }, [rowId, setEventRowWhere]); + + return useCallback(() => { + setEventRowWhere(null); + }, [setEventRowWhere]); +} From a0ce39bdf3be2ca524a99bdaf5580b1b147b2e05 Mon Sep 17 00:00:00 2001 From: Tushar Bahl Date: Mon, 12 Oct 2026 08:43:17 +0530 Subject: [PATCH 07/16] fix: clearing span when session name cliked Clicking the session name in the breadcrumb now clears the selected span, so the next event in that trace opens on its own span instead of the previous one. Closing the search drawer clears the span once. Opening another event from the session, or closing the session, clears it too. Switching away from the trace tab, opening another row of the same trace, and coming back selects that new row. A span that arrived on the URL is still kept on the first open. --- packages/app/src/SessionSidePanel.tsx | 9 ++++- .../app/src/components/DBRowSidePanel.tsx | 38 +++++++++++-------- .../DBRowSidePanel.spanSelection.test.tsx | 24 ++++++++++++ .../useOpenedRowSpanSelection.test.tsx | 25 +++++++++++- .../src/hooks/useOpenedRowSpanSelection.ts | 18 ++++++--- 5 files changed, 89 insertions(+), 25 deletions(-) diff --git a/packages/app/src/SessionSidePanel.tsx b/packages/app/src/SessionSidePanel.tsx index a5ff7414ae..943c947feb 100644 --- a/packages/app/src/SessionSidePanel.tsx +++ b/packages/app/src/SessionSidePanel.tsx @@ -38,6 +38,7 @@ import SidePanelBreadcrumbs, { BreadcrumbItem, } from '@/components/SidePanelBreadcrumbs'; import { useCloseOnClickOutside } from '@/hooks/useCloseOnClickOutside'; +import { useClearSpanSelection } from '@/hooks/useOpenedRowSpanSelection'; import useResizable from '@/hooks/useResizable'; import { WithClause } from '@/hooks/useRowWhere'; import { @@ -129,6 +130,8 @@ export default function SessionSidePanel({ initialRowId: selectedEvent?.rowId, }); + const clearSpanSelection = useClearSpanSelection(); + const handleBackToSession = useCallback(() => { setSelectedEvent(null); sidePanelStack.clearTrail(); @@ -136,18 +139,20 @@ export default function SessionSidePanel({ const handleEventNavigate = useCallback( (rowId: string, aliasWith: WithClause[]) => { + clearSpanSelection(); sidePanelStack.clearTrail(); setSelectedEvent({ rowId, aliasWith, sessionId }); }, - [setSelectedEvent, sidePanelStack, sessionId], + [clearSpanSelection, setSelectedEvent, sidePanelStack, sessionId], ); // X / Esc-at-root closes the whole panel and clears the session-panel params. const handleClose = useCallback(() => { + clearSpanSelection(); setSelectedEvent(null); sidePanelStack.clearTrail(); onClose(); - }, [setSelectedEvent, sidePanelStack, onClose]); + }, [clearSpanSelection, setSelectedEvent, sidePanelStack, onClose]); useHotkeys(['esc'], handleClose, { enabled: !selectedEvent }); diff --git a/packages/app/src/components/DBRowSidePanel.tsx b/packages/app/src/components/DBRowSidePanel.tsx index cbeebeee17..c3c211144c 100644 --- a/packages/app/src/components/DBRowSidePanel.tsx +++ b/packages/app/src/components/DBRowSidePanel.tsx @@ -406,18 +406,15 @@ export const DBRowSidePanelInner = ({ ); const clearSpanSelection = useOpenedRowSpanSelection(initialRowId); - const closePanel = useCallback(() => { - clearSpanSelection(); - onClose(); - }, [clearSpanSelection, onClose]); const handlePanelBack = useCallback(() => { // Pop one level (nav → source), restoring the tab active before that level // was entered. When the trail is empty, leave the panel: hand off to an - // embedding parent (session) or close. + // embedding parent (session) or close. The search drawer clears the span + // in its own close handler; a parent (the session) does not. if (popOne() === 'none') { - clearSpanSelection(); if (onNavigateToParent) { + clearSpanSelection(); onNavigateToParent(); } else { onClose(); @@ -765,7 +762,23 @@ export const DBRowSidePanelInner = ({ const items: BreadcrumbItem[] = []; if (parentBreadcrumbs) { - items.push(...parentBreadcrumbs); + // A parent crumb (the session name) leaves this row without going + // through Back or Close. Clear the span on that click too. + items.push( + ...parentBreadcrumbs.map(item => { + const onClick = item.onClick; + if (onClick == null) { + return item; + } + return { + ...item, + onClick: () => { + clearSpanSelection(); + onClick(); + }, + }; + }), + ); } const crumbSourceStack = sourceStack; @@ -827,6 +840,7 @@ export const DBRowSidePanelInner = ({ source.kind, handleBreadcrumbNavigation, parentBreadcrumbs, + clearSpanSelection, ]); const availableTabs = useMemo(() => { @@ -856,19 +870,13 @@ export const DBRowSidePanelInner = ({ ), - [ - allBreadcrumbs, - handlePanelBack, - closePanel, - isFullWidth, - onToggleFullWidth, - ], + [allBreadcrumbs, handlePanelBack, onClose, isFullWidth, onToggleFullWidth], ); if (isRowLoading || isResolvingSource) { diff --git a/packages/app/src/components/__tests__/DBRowSidePanel.spanSelection.test.tsx b/packages/app/src/components/__tests__/DBRowSidePanel.spanSelection.test.tsx index d63a0e0964..ca194c347f 100644 --- a/packages/app/src/components/__tests__/DBRowSidePanel.spanSelection.test.tsx +++ b/packages/app/src/components/__tests__/DBRowSidePanel.spanSelection.test.tsx @@ -152,10 +152,12 @@ function InnerHarness({ rowId, onClose, onNavigateToParent, + parentBreadcrumbs, }: { rowId: string; onClose: () => void; onNavigateToParent?: () => void; + parentBreadcrumbs?: { label: string; onClick?: () => void }[]; }) { const sidePanelStack = useSidePanelStack({ initialRowId: rowId }); return ( @@ -165,6 +167,7 @@ function InnerHarness({ aliasWith={[]} onClose={onClose} onNavigateToParent={onNavigateToParent} + parentBreadcrumbs={parentBreadcrumbs} sidePanelStack={sidePanelStack} /> ); @@ -204,6 +207,7 @@ describe('DBRowSidePanelErrorBoundary span selection', () => { fireEvent.click(screen.getByLabelText('Close')); + expect(setterFor('eventRowWhere')).toHaveBeenCalledTimes(1); expect(setterFor('eventRowWhere')).toHaveBeenCalledWith(null); }); @@ -221,7 +225,27 @@ describe('DBRowSidePanelErrorBoundary span selection', () => { fireEvent.click(screen.getByLabelText('Back')); + expect(setterFor('eventRowWhere')).toHaveBeenCalledTimes(1); expect(setterFor('eventRowWhere')).toHaveBeenCalledWith(null); expect(onNavigateToParent).toHaveBeenCalled(); }); + + it('clears the span when the session breadcrumb is clicked', () => { + const onSessionClick = jest.fn(); + render( + + + , + ); + + fireEvent.click(screen.getByTestId('side-panel-breadcrumb-0')); + + expect(setterFor('eventRowWhere')).toHaveBeenCalledTimes(1); + expect(setterFor('eventRowWhere')).toHaveBeenCalledWith(null); + expect(onSessionClick).toHaveBeenCalled(); + }); }); diff --git a/packages/app/src/hooks/__tests__/useOpenedRowSpanSelection.test.tsx b/packages/app/src/hooks/__tests__/useOpenedRowSpanSelection.test.tsx index 8d4f7abcf4..765f5c0c71 100644 --- a/packages/app/src/hooks/__tests__/useOpenedRowSpanSelection.test.tsx +++ b/packages/app/src/hooks/__tests__/useOpenedRowSpanSelection.test.tsx @@ -115,9 +115,17 @@ function Waterfall({ hint }: { hint: RowHighlightHint }) { return null; } -function Panel({ rowId, hint }: { rowId: string; hint: RowHighlightHint }) { +function Panel({ + rowId, + hint, + showTrace = true, +}: { + rowId: string; + hint: RowHighlightHint; + showTrace?: boolean; +}) { useOpenedRowSpanSelection(rowId); - return ; + return showTrace ? : null; } describe('useOpenedRowSpanSelection', () => { @@ -147,4 +155,17 @@ describe('useOpenedRowSpanSelection', () => { traceId: 'trace-1', }); }); + + it('selects the new row when the trace view remounts after the row changes', () => { + mockSpanStore.value = null; + const view = render(); + expect(mockSpanStore.value).toMatchObject({ id: 'where-a' }); + + view.rerender(); + view.rerender(); + expect(mockSpanStore.value).toBeNull(); + + view.rerender(); + expect(mockSpanStore.value).toMatchObject({ id: 'where-b' }); + }); }); diff --git a/packages/app/src/hooks/useOpenedRowSpanSelection.ts b/packages/app/src/hooks/useOpenedRowSpanSelection.ts index 3038dd4cd6..adc2d7ab5d 100644 --- a/packages/app/src/hooks/useOpenedRowSpanSelection.ts +++ b/packages/app/src/hooks/useOpenedRowSpanSelection.ts @@ -11,11 +11,19 @@ import { eventRowWhereParser } from '@/components/eventRowWhere'; // useLayoutEffect, not useEffect. Child effects run first, so a parent // useEffect clears the span only after the waterfall has applied the new // hint and marked it done. The next render then skips that hint. -export function useOpenedRowSpanSelection(rowId: string | undefined) { +export function useClearSpanSelection() { const [, setEventRowWhere] = useQueryState( 'eventRowWhere', eventRowWhereParser, ); + + return useCallback(() => { + setEventRowWhere(null); + }, [setEventRowWhere]); +} + +export function useOpenedRowSpanSelection(rowId: string | undefined) { + const clearSpanSelection = useClearSpanSelection(); const openedRowIdRef = useRef(rowId); useLayoutEffect(() => { @@ -23,10 +31,8 @@ export function useOpenedRowSpanSelection(rowId: string | undefined) { return; } openedRowIdRef.current = rowId; - setEventRowWhere(null); - }, [rowId, setEventRowWhere]); + clearSpanSelection(); + }, [rowId, clearSpanSelection]); - return useCallback(() => { - setEventRowWhere(null); - }, [setEventRowWhere]); + return clearSpanSelection; } From 4602d8868e59afd21e72f486bc1053dbfa45dae8 Mon Sep 17 00:00:00 2001 From: Tushar Bahl Date: Wed, 14 Oct 2026 17:05:27 +0530 Subject: [PATCH 08/16] fix: lint issue --- .../DBRowSidePanel.spanSelection.test.tsx | 1 - scripts/ci/__tests__/ratchet.test.mjs | 26 ++++++++++++++----- 2 files changed, 20 insertions(+), 7 deletions(-) diff --git a/packages/app/src/components/__tests__/DBRowSidePanel.spanSelection.test.tsx b/packages/app/src/components/__tests__/DBRowSidePanel.spanSelection.test.tsx index ca194c347f..fbb48512d1 100644 --- a/packages/app/src/components/__tests__/DBRowSidePanel.spanSelection.test.tsx +++ b/packages/app/src/components/__tests__/DBRowSidePanel.spanSelection.test.tsx @@ -121,7 +121,6 @@ import DBRowSidePanelErrorBoundary, { } from '@/components/DBRowSidePanel'; import useSidePanelStack from '@/hooks/useSidePanelStack'; -// eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion const ROOT_SOURCE = { id: 'log-src', kind: 'log', diff --git a/scripts/ci/__tests__/ratchet.test.mjs b/scripts/ci/__tests__/ratchet.test.mjs index f84900e2f4..f0c193fbbf 100644 --- a/scripts/ci/__tests__/ratchet.test.mjs +++ b/scripts/ci/__tests__/ratchet.test.mjs @@ -22,7 +22,10 @@ function fixture(packages, workspaces = ['packages/*']) { for (const [pkg, files] of Object.entries(packages)) { const dir = path.join(root, 'packages', pkg); mkdirSync(dir, { recursive: true }); - writeFileSync(path.join(dir, 'package.json'), JSON.stringify({ name: pkg })); + writeFileSync( + path.join(dir, 'package.json'), + JSON.stringify({ name: pkg }), + ); for (const [rel, source] of Object.entries(files)) { const file = path.join(dir, rel); mkdirSync(path.dirname(file), { recursive: true }); @@ -96,8 +99,12 @@ test('above baseline fails', () => { test('an advisory pattern above baseline warns instead of failing', () => { const { failed, messages } = compare( - { cli: { 'as-any': 0, 'ts-ignore': 0, 'eslint-disable': 0, '@source': 38 } }, - { cli: { 'as-any': 0, 'ts-ignore': 0, 'eslint-disable': 0, '@source': 37 } }, + { + cli: { 'as-any': 0, 'ts-ignore': 0, 'eslint-disable': 0, '@source': 38 }, + }, + { + cli: { 'as-any': 0, 'ts-ignore': 0, 'eslint-disable': 0, '@source': 37 }, + }, ); assert.equal(failed, false); assert.match(messages.join('\n'), /! cli\/@source: 38 > baseline 37/); @@ -105,8 +112,12 @@ test('an advisory pattern above baseline warns instead of failing', () => { test('an advisory rise does not mask a gated pattern rising too', () => { const { failed, messages } = compare( - { cli: { 'as-any': 1, 'ts-ignore': 0, 'eslint-disable': 0, '@source': 38 } }, - { cli: { 'as-any': 0, 'ts-ignore': 0, 'eslint-disable': 0, '@source': 37 } }, + { + cli: { 'as-any': 1, 'ts-ignore': 0, 'eslint-disable': 0, '@source': 38 }, + }, + { + cli: { 'as-any': 0, 'ts-ignore': 0, 'eslint-disable': 0, '@source': 37 }, + }, ); assert.equal(failed, true); assert.match(messages.join('\n'), /x cli\/as-any: 1 > baseline 0/); @@ -143,7 +154,10 @@ test('a baselined package that vanished is a hard error, not a free pass', () => test('the committed baseline is not already breached by the real repo', () => { const repoRoot = path.resolve(import.meta.dirname, '../../..'); const baseline = JSON.parse( - readFileSync(path.join(repoRoot, 'scripts/ci/ratchet-baseline.json'), 'utf8'), + readFileSync( + path.join(repoRoot, 'scripts/ci/ratchet-baseline.json'), + 'utf8', + ), ); // Only asserts not-above-baseline: below-baseline is a deliberate warn. assert.equal(compare(collectCounts(repoRoot), baseline).failed, false); From cb5dde5c4e21122c2846d1d9c616ad7a8cd03f6f Mon Sep 17 00:00:00 2001 From: Tushar Bahl Date: Wed, 14 Oct 2026 17:30:37 +0530 Subject: [PATCH 09/16] revert changes of ratchet test file. --- scripts/ci/__tests__/ratchet.test.mjs | 26 ++++++-------------------- 1 file changed, 6 insertions(+), 20 deletions(-) diff --git a/scripts/ci/__tests__/ratchet.test.mjs b/scripts/ci/__tests__/ratchet.test.mjs index f0c193fbbf..f84900e2f4 100644 --- a/scripts/ci/__tests__/ratchet.test.mjs +++ b/scripts/ci/__tests__/ratchet.test.mjs @@ -22,10 +22,7 @@ function fixture(packages, workspaces = ['packages/*']) { for (const [pkg, files] of Object.entries(packages)) { const dir = path.join(root, 'packages', pkg); mkdirSync(dir, { recursive: true }); - writeFileSync( - path.join(dir, 'package.json'), - JSON.stringify({ name: pkg }), - ); + writeFileSync(path.join(dir, 'package.json'), JSON.stringify({ name: pkg })); for (const [rel, source] of Object.entries(files)) { const file = path.join(dir, rel); mkdirSync(path.dirname(file), { recursive: true }); @@ -99,12 +96,8 @@ test('above baseline fails', () => { test('an advisory pattern above baseline warns instead of failing', () => { const { failed, messages } = compare( - { - cli: { 'as-any': 0, 'ts-ignore': 0, 'eslint-disable': 0, '@source': 38 }, - }, - { - cli: { 'as-any': 0, 'ts-ignore': 0, 'eslint-disable': 0, '@source': 37 }, - }, + { cli: { 'as-any': 0, 'ts-ignore': 0, 'eslint-disable': 0, '@source': 38 } }, + { cli: { 'as-any': 0, 'ts-ignore': 0, 'eslint-disable': 0, '@source': 37 } }, ); assert.equal(failed, false); assert.match(messages.join('\n'), /! cli\/@source: 38 > baseline 37/); @@ -112,12 +105,8 @@ test('an advisory pattern above baseline warns instead of failing', () => { test('an advisory rise does not mask a gated pattern rising too', () => { const { failed, messages } = compare( - { - cli: { 'as-any': 1, 'ts-ignore': 0, 'eslint-disable': 0, '@source': 38 }, - }, - { - cli: { 'as-any': 0, 'ts-ignore': 0, 'eslint-disable': 0, '@source': 37 }, - }, + { cli: { 'as-any': 1, 'ts-ignore': 0, 'eslint-disable': 0, '@source': 38 } }, + { cli: { 'as-any': 0, 'ts-ignore': 0, 'eslint-disable': 0, '@source': 37 } }, ); assert.equal(failed, true); assert.match(messages.join('\n'), /x cli\/as-any: 1 > baseline 0/); @@ -154,10 +143,7 @@ test('a baselined package that vanished is a hard error, not a free pass', () => test('the committed baseline is not already breached by the real repo', () => { const repoRoot = path.resolve(import.meta.dirname, '../../..'); const baseline = JSON.parse( - readFileSync( - path.join(repoRoot, 'scripts/ci/ratchet-baseline.json'), - 'utf8', - ), + readFileSync(path.join(repoRoot, 'scripts/ci/ratchet-baseline.json'), 'utf8'), ); // Only asserts not-above-baseline: below-baseline is a deliberate warn. assert.equal(compare(collectCounts(repoRoot), baseline).failed, false); From 85d723f37261a80c8f4e762d0aa1ed8644b746f5 Mon Sep 17 00:00:00 2001 From: Drew Davis Date: Thu, 15 Oct 2026 11:28:56 -0400 Subject: [PATCH 10/16] chore: Fix lint warning --- .../__tests__/DBRowSidePanel.spanSelection.test.tsx | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/packages/app/src/components/__tests__/DBRowSidePanel.spanSelection.test.tsx b/packages/app/src/components/__tests__/DBRowSidePanel.spanSelection.test.tsx index fbb48512d1..fe72300553 100644 --- a/packages/app/src/components/__tests__/DBRowSidePanel.spanSelection.test.tsx +++ b/packages/app/src/components/__tests__/DBRowSidePanel.spanSelection.test.tsx @@ -1,4 +1,4 @@ -import { TSource } from '@hyperdx/common-utils/dist/types'; +import { SourceKind, TLogSource } from '@hyperdx/common-utils/dist/types'; import { MantineProvider } from '@mantine/core'; import { fireEvent, render, screen } from '@testing-library/react'; @@ -121,11 +121,16 @@ import DBRowSidePanelErrorBoundary, { } from '@/components/DBRowSidePanel'; import useSidePanelStack from '@/hooks/useSidePanelStack'; -const ROOT_SOURCE = { +const ROOT_SOURCE: TLogSource = { id: 'log-src', - kind: 'log', + kind: SourceKind.Log, + name: 'Logs', + connection: 'conn', + from: { databaseName: 'default', tableName: 'otel_logs' }, + timestampValueExpression: 'Timestamp', + defaultTableSelectExpression: 'Timestamp, Body', traceSourceId: 'trace-src', -} as TSource; +}; const urlSpan = { id: 'span-where', From 2a7f725e7d0714ca898e86441443411e6cdf2e3d Mon Sep 17 00:00:00 2001 From: Drew Davis Date: Thu, 15 Oct 2026 13:43:14 -0400 Subject: [PATCH 11/16] feat: Add UI for configuring adhoc dashboard filters (#3341) --- .changeset/adhoc-filter-ui.md | 6 + packages/app/.env.development | 1 + packages/app/playwright.config.ts | 4 +- packages/app/src/DBDashboardPage.tsx | 2 +- packages/app/src/__tests__/hdxDebug.test.ts | 1 + .../AdhocFilterEditForm.tsx | 146 +++++++++ .../DashboardFilterEditForm.tsx | 19 +- .../DashboardFiltersList.tsx | 33 +- .../__tests__/filterFormState.test.ts | 80 +++++ .../DashboardFiltersModal/filterFormState.ts | 33 +- packages/app/src/config.ts | 2 + packages/app/src/hdxDebug.ts | 2 + .../features/dashboard-adhoc-filters.spec.ts | 310 ++++++++++++++++++ .../tests/e2e/page-objects/DashboardPage.ts | 77 ++++- .../src/__tests__/filters.test.ts | 24 ++ packages/common-utils/src/filters.ts | 13 + 16 files changed, 729 insertions(+), 24 deletions(-) create mode 100644 .changeset/adhoc-filter-ui.md create mode 100644 packages/app/src/components/DashboardFiltersModal/AdhocFilterEditForm.tsx create mode 100644 packages/app/tests/e2e/features/dashboard-adhoc-filters.spec.ts diff --git a/.changeset/adhoc-filter-ui.md b/.changeset/adhoc-filter-ui.md new file mode 100644 index 0000000000..0a464f980a --- /dev/null +++ b/.changeset/adhoc-filter-ui.md @@ -0,0 +1,6 @@ +--- +'@hyperdx/common-utils': patch +'@hyperdx/app': patch +--- + +feat: Add UI for configuring adhoc dashboard filters diff --git a/packages/app/.env.development b/packages/app/.env.development index af4a79ce0f..81ff15ce3b 100644 --- a/packages/app/.env.development +++ b/packages/app/.env.development @@ -13,4 +13,5 @@ NEXT_PUBLIC_OTEL_EXPORTER_OTLP_ENDPOINT="http://localhost:${HDX_DEV_OTEL_HTTP_PO # NEXT_PUBLIC_OTEL_EXPORTER_OTLP_METRICS_ENDPOINT= # NEXT_PUBLIC_OTEL_EXPORTER_OTLP_LOGS_ENDPOINT= NEXT_PUBLIC_ENABLE_PROMQL=true +NEXT_PUBLIC_ENABLE_ADHOC_FILTERS=true NEXT_PUBLIC_ENABLE_ALERT_DETAILS=true diff --git a/packages/app/playwright.config.ts b/packages/app/playwright.config.ts index ca3adee7f4..a0cd862caf 100644 --- a/packages/app/playwright.config.ts +++ b/packages/app/playwright.config.ts @@ -105,8 +105,8 @@ export default defineConfig({ { // Full UI: Alerts + Dashboards. Not local mode; Alerts enabled; command: USE_DEV - ? `SERVER_URL=http://localhost:${API_PORT} PORT=${APP_PORT} NEXT_DIST_DIR=.next-e2e NEXT_PUBLIC_ENABLE_ALERT_DETAILS=true NEXT_PUBLIC_ENABLE_PROMQL=true next dev --webpack` - : `SERVER_URL=http://localhost:${API_PORT} PORT=${APP_PORT} NEXT_DIST_DIR=.next-e2e NEXT_PUBLIC_ENABLE_ALERT_DETAILS=true NEXT_PUBLIC_ENABLE_PROMQL=true yarn build && SERVER_URL=http://localhost:${API_PORT} PORT=${APP_PORT} NEXT_DIST_DIR=.next-e2e NEXT_PUBLIC_ENABLE_ALERT_DETAILS=true NEXT_PUBLIC_ENABLE_PROMQL=true yarn start`, + ? `SERVER_URL=http://localhost:${API_PORT} PORT=${APP_PORT} NEXT_DIST_DIR=.next-e2e NEXT_PUBLIC_ENABLE_ALERT_DETAILS=true NEXT_PUBLIC_ENABLE_PROMQL=true NEXT_PUBLIC_ENABLE_ADHOC_FILTERS=true next dev --webpack` + : `SERVER_URL=http://localhost:${API_PORT} PORT=${APP_PORT} NEXT_DIST_DIR=.next-e2e NEXT_PUBLIC_ENABLE_ALERT_DETAILS=true NEXT_PUBLIC_ENABLE_PROMQL=true NEXT_PUBLIC_ENABLE_ADHOC_FILTERS=true yarn build && SERVER_URL=http://localhost:${API_PORT} PORT=${APP_PORT} NEXT_DIST_DIR=.next-e2e NEXT_PUBLIC_ENABLE_ALERT_DETAILS=true NEXT_PUBLIC_ENABLE_PROMQL=true NEXT_PUBLIC_ENABLE_ADHOC_FILTERS=true yarn start`, port: parseInt(APP_PORT, 10), reuseExistingServer: !process.env.CI, timeout: APP_SERVER_STARTUP_TIMEOUT_MS, diff --git a/packages/app/src/DBDashboardPage.tsx b/packages/app/src/DBDashboardPage.tsx index 29bd584a28..b7e102324b 100644 --- a/packages/app/src/DBDashboardPage.tsx +++ b/packages/app/src/DBDashboardPage.tsx @@ -3524,7 +3524,7 @@ function DBDashboardPage({ setShowFiltersModal(false)} - filters={filters} + filters={dashboard?.filters ?? []} onSaveFilter={handleSaveFilter} onRemoveFilter={handleRemoveFilter} isLoading={isSavingDashboard || isFetchingDashboard} diff --git a/packages/app/src/__tests__/hdxDebug.test.ts b/packages/app/src/__tests__/hdxDebug.test.ts index 3c6760ec02..40bd113d08 100644 --- a/packages/app/src/__tests__/hdxDebug.test.ts +++ b/packages/app/src/__tests__/hdxDebug.test.ts @@ -4,6 +4,7 @@ const baseConfig = { APP_VERSION: '9.9.9', BASE_PATH: '', + IS_ADHOC_FILTERS_ENABLED: false, IS_ALERT_DETAILS_ENABLED: false, IS_CLICKHOUSE_BUILD: false, IS_LLM_COST_ENABLED: false, diff --git a/packages/app/src/components/DashboardFiltersModal/AdhocFilterEditForm.tsx b/packages/app/src/components/DashboardFiltersModal/AdhocFilterEditForm.tsx new file mode 100644 index 0000000000..83cffdf038 --- /dev/null +++ b/packages/app/src/components/DashboardFiltersModal/AdhocFilterEditForm.tsx @@ -0,0 +1,146 @@ +import { Controller, UseFormSetValue, useWatch } from 'react-hook-form'; +import { getAdhocFilterSourceKinds } from '@hyperdx/common-utils/dist/filters'; +import { + AdhocFilterSourceType, + DashboardFilter, + SourceKind, +} from '@hyperdx/common-utils/dist/types'; +import { Box, Divider, SegmentedControl } from '@mantine/core'; + +import { CheckBoxControlled } from '@/components/InputControlled'; +import { SourceMultiSelectControlled } from '@/components/SourceMultiSelect'; +import { IS_PROMQL_ENABLED } from '@/config'; +import { useSources } from '@/source'; + +import { CustomInputWrapper } from './CustomInputWrapper'; +import { FilterFormControl, FilterFormValues } from './filterFormState'; +import { VariableNameInput } from './VariableNameInput'; + +const SOURCE_TYPE_OPTIONS: { value: AdhocFilterSourceType; label: string }[] = [ + { value: 'sql', label: 'ClickHouse' }, + { value: 'promql', label: 'Prometheus' }, +]; + +interface AdhocFilterEditFormProps { + control: FilterFormControl; + setValue: UseFormSetValue; + /** Filters other than the one being edited, used to keep variable names unique. */ + otherFilters: DashboardFilter[]; +} + +/** Form describing an ad hoc dashboard filter */ +export const AdhocFilterEditForm = ({ + control, + setValue, + otherFilters, +}: AdhocFilterEditFormProps) => { + const { data: allSources } = useSources(); + const [sourceType, isBroadcastEnabled] = useWatch({ + control, + name: ['sourceType', 'isBroadcastEnabled'], + }); + const allowedSourceKinds = getAdhocFilterSourceKinds(sourceType); + // Kept visible for a saved PromQL filter so its source type can be changed + const showSourceType = + sourceType === 'promql' || + (IS_PROMQL_ENABLED && + !!allSources?.some(s => s.kind === SourceKind.Promql)); + + // The multi-selects hide sources of other kinds, which the form can still + // hold after switching from another filter type. + const validateSourceKinds = (ids: string[] | undefined) => { + const hasOtherKind = ids?.some(id => { + const source = allSources?.find(s => s.id === id); + return source && !allowedSourceKinds.includes(source.kind); + }); + return ( + !hasOtherKind || + (sourceType === 'promql' + ? 'Select only PromQL sources' + : 'Select only log, trace, or session sources') + ); + }; + + return ( + <> + {showSourceType && ( + + ( + { + onChange(next); + // Sources of the previous type can't be kept + setValue('sources', [], { shouldDirty: true }); + setValue('appliesToSourceIds', [], { shouldDirty: true }); + }} + data={SOURCE_TYPE_OPTIONS} + data-testid="adhoc-filter-source-type" + /> + )} + /> + + )} + + + !value?.length + ? 'Select at least one source' + : validateSourceKinds(value), + }} + /> + + + + + {isBroadcastEnabled && ( + + + + + + )} + + + + ); +}; diff --git a/packages/app/src/components/DashboardFiltersModal/DashboardFilterEditForm.tsx b/packages/app/src/components/DashboardFiltersModal/DashboardFilterEditForm.tsx index 6e2d194412..2a0dd96e23 100644 --- a/packages/app/src/components/DashboardFiltersModal/DashboardFilterEditForm.tsx +++ b/packages/app/src/components/DashboardFiltersModal/DashboardFilterEditForm.tsx @@ -26,10 +26,11 @@ import { ErrorBoundary } from '@/components/Error/ErrorBoundary'; import { CheckBoxControlled } from '@/components/InputControlled'; import SelectControlled from '@/components/SelectControlled'; import { SqlVariablesProvider } from '@/components/SQLEditor/variableCompletions'; -import { IS_PROMQL_ENABLED } from '@/config'; +import { IS_ADHOC_FILTERS_ENABLED, IS_PROMQL_ENABLED } from '@/config'; import { useConfirm } from '@/useConfirm'; import { useZIndex } from '@/zIndex'; +import { AdhocFilterEditForm } from './AdhocFilterEditForm'; import { MODAL_SIZE } from './constants'; import { CustomInputWrapper } from './CustomInputWrapper'; import { @@ -55,6 +56,14 @@ const FILTER_TYPE_OPTIONS = [ }, ] : []), + ...(IS_ADHOC_FILTERS_ENABLED + ? [ + { + value: DashboardFilterType.enum.ADHOC, + label: 'Ad hoc keys and values', + }, + ] + : []), ]; interface DashboardFilterEditFormProps { @@ -232,6 +241,12 @@ export const DashboardFilterEditForm = ({ control={control} otherFilters={otherFilters} /> + ) : formFilterType === 'ADHOC' ? ( + ) : formFilterType === 'PROMETHEUS_LABEL' ? ( - {showRequiredFilterOptions && ( + {showRequiredFilterOptions && formFilterType !== 'ADHOC' && ( void; } +function getSourceNames( + ids: string[], + sources?: { id: string; name: string }[], +) { + return ids + .map(id => sources?.find(s => s.id === id)?.name) + .filter((name): name is string => !!name) + .join(', '); +} + /** Where the dropdown's values come from: the queried source, or the authored list. */ function getValuesAttribute( filter: DashboardFilter, @@ -74,9 +88,12 @@ function getValuesAttribute( label: `${count} custom option${count === 1 ? '' : 's'}`, }; } - // Not shown in the dashboard UI yet case 'ADHOC': - return { icon: , label: '' }; + return { + icon: , + tooltip: 'Sources the condition keys and values come from', + label: getSourceNames(filter.sources, sources), + }; default: filter satisfies never; // exhaustive check return { icon: , label: '' }; @@ -87,6 +104,12 @@ function getBroadcastTargetDisplay( filter: DashboardFilter, sources?: { id: string; name: string }[], ) { + // An ad hoc filter broadcasts to its own sources unless narrowed further + if (isAdhocFilter(filter)) { + if (!isFilterBroadcastEnabled(filter)) return undefined; + return getSourceNames(getAdhocFilterAppliesToSourceIds(filter), sources); + } + const broadcastTarget = getFilterBroadcastTarget(filter); if (!broadcastTarget) return undefined; @@ -94,11 +117,7 @@ function getBroadcastTargetDisplay( return 'All sources'; } - const appliedSourceNames = broadcastTarget.appliesToSourceIds - .map(id => sources?.find(s => s.id === id)?.name) - .filter((name): name is string => !!name); - - return appliedSourceNames.join(', '); + return getSourceNames(broadcastTarget.appliesToSourceIds, sources); } export const DashboardFiltersList = ({ diff --git a/packages/app/src/components/DashboardFiltersModal/__tests__/filterFormState.test.ts b/packages/app/src/components/DashboardFiltersModal/__tests__/filterFormState.test.ts index 8b8d789d35..a19d565601 100644 --- a/packages/app/src/components/DashboardFiltersModal/__tests__/filterFormState.test.ts +++ b/packages/app/src/components/DashboardFiltersModal/__tests__/filterFormState.test.ts @@ -80,6 +80,33 @@ describe('toFormValues', () => { isVariableEnabled: true, }); }); + + it('seeds the ad hoc fields from a stored ad hoc filter', () => { + const values = toFormValues({ + id: 'a', + type: 'ADHOC', + name: 'Conditions', + sourceType: 'promql', + sources: ['prom'], + appliesToSourceIds: ['prom-2'], + isBroadcastEnabled: false, + isVariableEnabled: true, + variableName: 'conds', + }); + + expect(values).toMatchObject({ + type: 'ADHOC', + sourceType: 'promql', + sources: ['prom'], + appliesToSourceIds: ['prom-2'], + variableName: 'conds', + isBroadcastEnabled: false, + }); + }); + + it('defaults a new filter to SQL ad hoc sources', () => { + expect(toFormValues()).toMatchObject({ sourceType: 'sql', sources: [] }); + }); }); describe('toSavedFilter', () => { @@ -186,6 +213,59 @@ describe('toSavedFilter', () => { ).toThrow(); }); + it('keeps only the ad hoc fields on an ad hoc filter', () => { + const saved = toSavedFilter( + formValues({ + type: 'ADHOC', + name: 'Conditions', + sourceType: 'sql', + sources: ['logs', 'traces'], + appliesToSourceIds: ['logs'], + // Left behind by the other editors before the type was switched. + expression: 'Env', + source: 'logs', + options: ['prod'], + isRequired: true, + isSingleSelect: true, + }), + ); + + expect(saved).toEqual({ + id: 'a', + type: 'ADHOC', + name: 'Conditions', + sourceType: 'sql', + sources: ['logs', 'traces'], + appliesToSourceIds: ['logs'], + isBroadcastEnabled: true, + isVariableEnabled: true, + variableName: 'Conditions', + }); + }); + + it('drops the ad hoc broadcast scope when broadcast is off', () => { + const saved = toSavedFilter( + formValues({ + type: 'ADHOC', + name: 'Conditions', + sources: ['logs'], + appliesToSourceIds: ['logs'], + isBroadcastEnabled: false, + }), + ); + + expect(saved).toMatchObject({ + isBroadcastEnabled: false, + appliesToSourceIds: undefined, + }); + }); + + it('rejects an ad hoc filter without sources', () => { + expect(() => + toSavedFilter(formValues({ type: 'ADHOC', name: 'Conditions' })), + ).toThrow(); + }); + it('rejects values the schema cannot accept', () => { expect(() => toSavedFilter(formValues({ type: 'STATIC_LIST', options: [] })), diff --git a/packages/app/src/components/DashboardFiltersModal/filterFormState.ts b/packages/app/src/components/DashboardFiltersModal/filterFormState.ts index 2dac17acd0..538e317bc5 100644 --- a/packages/app/src/components/DashboardFiltersModal/filterFormState.ts +++ b/packages/app/src/components/DashboardFiltersModal/filterFormState.ts @@ -7,6 +7,7 @@ import { isFilterVariableEnabled, } from '@hyperdx/common-utils/dist/filters'; import { + AdhocDashboardFilter, DashboardFilter, DashboardFilterSchema, PromqlLabelDashboardFilter, @@ -51,7 +52,8 @@ export type FilterFormValues = { | 'minSelections' | 'isGlobalRequirement' | 'maxSelections' - >; + > & + Pick; export type FilterFormControl = Control; @@ -62,6 +64,7 @@ export const toFormValues = ( const queried = filter?.type === 'QUERY_EXPRESSION' ? filter : undefined; const staticList = filter?.type === 'STATIC_LIST' ? filter : undefined; const promqlLabel = filter?.type === 'PROMETHEUS_LABEL' ? filter : undefined; + const adhoc = filter?.type === 'ADHOC' ? filter : undefined; return { id: filter?.id ?? crypto.randomUUID(), @@ -80,7 +83,8 @@ export const toFormValues = ( sourceMetricType: queried?.sourceMetricType, where: queried?.where ?? '', whereLanguage: queried?.whereLanguage ?? getStoredLanguage() ?? 'sql', - appliesToSourceIds: queried?.appliesToSourceIds ?? [], + appliesToSourceIds: + queried?.appliesToSourceIds ?? adhoc?.appliesToSourceIds ?? [], // STATIC_LIST fields options: staticList?.options ?? [], @@ -88,6 +92,10 @@ export const toFormValues = ( // PROMETHEUS_LABEL fields label: promqlLabel?.label ?? '', match: promqlLabel?.match ?? '', + + // ADHOC fields + sourceType: adhoc?.sourceType ?? 'sql', + sources: adhoc?.sources ?? [], }; }; @@ -128,8 +136,27 @@ export const toSavedFilter = (values: FilterFormValues): DashboardFilter => { }); } - const trimmedWhere = values.where?.trim() ?? ''; const appliesTo = values.appliesToSourceIds?.filter(id => !!id?.length); + + // 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, + name: values.name, + sourceType: values.sourceType, + sources: values.sources, + isBroadcastEnabled, + // Hidden, and so unvalidated, while broadcast is off + appliesToSourceIds: + isBroadcastEnabled && appliesTo?.length ? appliesTo : undefined, + isVariableEnabled: true, + variableName: getFilterVariableName(values), + }); + } + + const trimmedWhere = values.where?.trim() ?? ''; const isVariableEnabled = isFilterVariableEnabled(values); return DashboardFilterSchema.parse({ diff --git a/packages/app/src/config.ts b/packages/app/src/config.ts index c416d1e660..c0ce75adf6 100644 --- a/packages/app/src/config.ts +++ b/packages/app/src/config.ts @@ -82,6 +82,8 @@ export const IS_METRICS_ENABLED = true; export const IS_MTVIEWS_ENABLED = false; export const IS_SESSIONS_ENABLED = true; export const IS_PROMQL_ENABLED = env('NEXT_PUBLIC_ENABLE_PROMQL') === 'true'; +export const IS_ADHOC_FILTERS_ENABLED = + env('NEXT_PUBLIC_ENABLE_ADHOC_FILTERS') === 'true'; // Suffix for the tab title/sidebar pill. Shown in full in the page title; // only truncated visually (via CSS ellipsis) in the sidebar, where space is // constrained. Internal whitespace is collapsed so it displays consistently diff --git a/packages/app/src/hdxDebug.ts b/packages/app/src/hdxDebug.ts index 1f78812761..c5d5981e7d 100644 --- a/packages/app/src/hdxDebug.ts +++ b/packages/app/src/hdxDebug.ts @@ -5,6 +5,7 @@ import { APP_VERSION, BASE_PATH, INSTANCE_LABEL, + IS_ADHOC_FILTERS_ENABLED, IS_ALERT_DETAILS_ENABLED, IS_CLICKHOUSE_BUILD, IS_DEV, @@ -85,6 +86,7 @@ const staticFeatures: Record = { promql: IS_PROMQL_ENABLED, llmCost: IS_LLM_COST_ENABLED, alertDetails: IS_ALERT_DETAILS_ENABLED, + adhocFilters: IS_ADHOC_FILTERS_ENABLED, instanceLabel: Boolean(INSTANCE_LABEL), }; diff --git a/packages/app/tests/e2e/features/dashboard-adhoc-filters.spec.ts b/packages/app/tests/e2e/features/dashboard-adhoc-filters.spec.ts new file mode 100644 index 0000000000..e64ed257cc --- /dev/null +++ b/packages/app/tests/e2e/features/dashboard-adhoc-filters.spec.ts @@ -0,0 +1,310 @@ +/** + * An `ADHOC` dashboard filter's conditions (`key operator value`) are picked + * from the keys of its sources. For now the filters modal only lists, creates, + * and edits them; they are not rendered or applied on the dashboard. + */ +import { DashboardPage } from '../page-objects/DashboardPage'; +import { expect, test } from '../utils/base-test'; +import { + DEFAULT_LOGS_SOURCE_NAME, + DEFAULT_METRICS_SOURCE_NAME, + DEFAULT_TRACES_SOURCE_NAME, + PROMQL_SOURCE_NAME, +} from '../utils/constants'; + +test.describe( + 'Ad hoc dashboard filters', + { tag: ['@dashboard', '@full-stack'] }, + () => { + let dashboardPage: DashboardPage; + + test.beforeEach(async ({ page }) => { + dashboardPage = new DashboardPage(page); + await dashboardPage.goto(); + }); + + test('authors a SQL filter over several sources', async ({ page }) => { + await test.step('Open the add-filter form and pick the ad hoc type', async () => { + await dashboardPage.createNewDashboard(); + await dashboardPage.openEditFiltersModal(); + await dashboardPage.openAddFilterForm(); + await dashboardPage.getFilterNameInput().fill('Conditions'); + await dashboardPage.selectFilterType('Ad hoc keys and values'); + await expect(dashboardPage.getFilterNameInput()).toHaveValue( + 'Conditions', + ); + }); + + await test.step('Only the fields an ad hoc filter has are rendered', async () => { + const form = dashboardPage.getFilterForm(); + await expect( + dashboardPage.getAdhocFilterSourceTypeControl(), + ).toBeVisible(); + await expect(dashboardPage.getAdhocFilterSourcesInput()).toBeVisible(); + await expect(dashboardPage.broadcastFilterCheckbox).toBeChecked(); + 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.getByTestId('source-selector')).toHaveCount(0); + await expect(form.locator('div.cm-editor')).toHaveCount(0); + await expect( + form.getByTestId('filter-variable-enabled-checkbox'), + ).toHaveCount(0); + await expect(dashboardPage.requiredFilterCheckbox).toHaveCount(0); + await expect(dashboardPage.singleSelectCheckbox).toHaveCount(0); + }); + + await test.step('Saving without a source is rejected', async () => { + await page.getByTestId('save-filter-button').click(); + await expect( + dashboardPage.getFilterForm().getByText('Select at least one source'), + ).toBeVisible(); + }); + + await test.step('Only SQL sources without a metric type are offered', async () => { + await dashboardPage.getAdhocFilterSourcesInput().click(); + await expect( + dashboardPage.getFilterOption(DEFAULT_LOGS_SOURCE_NAME), + ).toBeVisible(); + await expect( + dashboardPage.getFilterOption(DEFAULT_TRACES_SOURCE_NAME), + ).toBeVisible(); + await expect( + dashboardPage.getFilterOption(DEFAULT_METRICS_SOURCE_NAME), + ).toHaveCount(0); + await expect( + dashboardPage.getFilterOption(PROMQL_SOURCE_NAME), + ).toHaveCount(0); + await page.keyboard.press('Escape'); + }); + + await test.step('The saved filter is summarized by its sources', async () => { + await dashboardPage.selectSourcesInMultiSelect( + dashboardPage.getAdhocFilterSourcesInput(), + [DEFAULT_LOGS_SOURCE_NAME, DEFAULT_TRACES_SOURCE_NAME], + ); + await dashboardPage.variableNameInput.fill('conds'); + await page.getByTestId('save-filter-button').click(); + + const item = dashboardPage.getFilterItemByName('Conditions'); + await expect(item).toBeVisible(); + await expect(item).toContainText( + `${DEFAULT_LOGS_SOURCE_NAME}, ${DEFAULT_TRACES_SOURCE_NAME}`, + ); + await expect(item).toContainText('($conds)'); + await expect( + dashboardPage.getFilterBroadcastTarget('Conditions'), + ).toHaveText( + `${DEFAULT_LOGS_SOURCE_NAME}, ${DEFAULT_TRACES_SOURCE_NAME}`, + ); + }); + + await test.step('The filter survives a reload', async () => { + await dashboardPage.closeFiltersModal(); + await page.reload(); + await dashboardPage.waitForLoaded(); + await dashboardPage.openEditFiltersModal(); + await expect( + dashboardPage.getFilterItemByName('Conditions'), + ).toContainText('($conds)'); + }); + }); + + test('edits the broadcast scope and variable name of a saved filter', async ({ + page, + }) => { + await test.step('Create a SQL ad hoc filter', async () => { + await dashboardPage.createNewDashboard(); + await dashboardPage.openEditFiltersModal(); + await dashboardPage.addAdhocFilterToDashboard( + 'Conditions', + [DEFAULT_LOGS_SOURCE_NAME, DEFAULT_TRACES_SOURCE_NAME], + { variableName: 'conds' }, + ); + }); + + await test.step('The edit form loads the saved values', async () => { + await dashboardPage.openEditFilterForm('Conditions'); + const form = dashboardPage.getFilterForm(); + await expect(dashboardPage.getFilterTypePicker()).toHaveValue( + 'Ad hoc keys and values', + ); + await expect( + form.getByText(DEFAULT_LOGS_SOURCE_NAME, { exact: true }), + ).toBeVisible(); + await expect( + form.getByText(DEFAULT_TRACES_SOURCE_NAME, { exact: true }), + ).toBeVisible(); + await expect(dashboardPage.variableNameInput).toHaveValue('conds'); + }); + + await test.step('Narrow the broadcast to one source', async () => { + await dashboardPage.selectSourcesInMultiSelect( + dashboardPage.appliesToSourceSelector, + [DEFAULT_TRACES_SOURCE_NAME], + ); + 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 dashboardPage.openEditFilterForm('Conditions'); + await expect(dashboardPage.broadcastFilterCheckbox).not.toBeChecked(); + await expect(dashboardPage.variableNameInput).toHaveValue( + 'where_clause', + ); + }); + }); + + test('authors a PromQL filter', async ({ page }) => { + await test.step('Switching source type clears the picked sources', async () => { + await dashboardPage.createNewDashboard(); + await dashboardPage.openEditFiltersModal(); + await dashboardPage.openAddFilterForm(); + await dashboardPage.getFilterNameInput().fill('Labels'); + await dashboardPage.selectFilterType('Ad hoc keys and values'); + await dashboardPage.selectSourcesInMultiSelect( + dashboardPage.getAdhocFilterSourcesInput(), + [DEFAULT_LOGS_SOURCE_NAME], + ); + const form = dashboardPage.getFilterForm(); + await expect( + form.getByText(DEFAULT_LOGS_SOURCE_NAME, { exact: true }), + ).toBeVisible(); + + await dashboardPage.selectAdhocFilterSourceType('Prometheus'); + await expect( + form.getByText(DEFAULT_LOGS_SOURCE_NAME, { exact: true }), + ).toHaveCount(0); + }); + + await test.step('Only PromQL sources are offered', async () => { + await dashboardPage.getAdhocFilterSourcesInput().click(); + await expect( + dashboardPage.getFilterOption(PROMQL_SOURCE_NAME), + ).toBeVisible(); + await expect( + dashboardPage.getFilterOption(DEFAULT_LOGS_SOURCE_NAME), + ).toHaveCount(0); + await dashboardPage.getFilterOption(PROMQL_SOURCE_NAME).click(); + await page.keyboard.press('Escape'); + }); + + await test.step('The saved filter keeps its source type', async () => { + await page.getByTestId('save-filter-button').click(); + await expect(dashboardPage.getFilterItemByName('Labels')).toContainText( + PROMQL_SOURCE_NAME, + ); + + await dashboardPage.openEditFilterForm('Labels'); + await expect( + dashboardPage + .getAdhocFilterSourceTypeControl() + .getByRole('radio', { name: 'Prometheus' }), + ).toBeChecked(); + }); + }); + + test('rejects broadcast sources carried over from another filter type', async ({ + page, + }) => { + await dashboardPage.createNewDashboard(); + await dashboardPage.openEditFiltersModal(); + await dashboardPage.openAddFilterForm(); + await dashboardPage.getFilterNameInput().fill('Conditions'); + await dashboardPage.selectSourcesInMultiSelect( + dashboardPage.appliesToSourceSelector, + [DEFAULT_METRICS_SOURCE_NAME], + ); + + await dashboardPage.selectFilterType('Ad hoc keys and values'); + await dashboardPage.selectSourcesInMultiSelect( + dashboardPage.getAdhocFilterSourcesInput(), + [DEFAULT_LOGS_SOURCE_NAME], + ); + await page.getByTestId('save-filter-button').click(); + + await expect( + dashboardPage + .getFilterForm() + .getByText('Select only log, trace, or session sources'), + ).toBeVisible(); + }); + + test("lives alongside the dashboard's other filters", async ({ page }) => { + await test.step('Create an ad hoc and a static filter', async () => { + await dashboardPage.createNewDashboard(); + await dashboardPage.openEditFiltersModal(); + await dashboardPage.addAdhocFilterToDashboard( + 'Conditions', + [DEFAULT_LOGS_SOURCE_NAME], + { variableName: 'conds' }, + ); + await dashboardPage.addStaticListFilterToDashboard('Environment', [ + 'prod', + ]); + }); + + await test.step('Other filters cannot reuse its variable name', async () => { + await dashboardPage.openEditFilterForm('Environment'); + await dashboardPage.variableNameInput.fill('conds'); + await page.getByTestId('save-filter-button').click(); + await expect( + dashboardPage + .getFilterForm() + .getByText( + 'This variable name is used by another filter on this dashboard (Conditions)', + ), + ).toBeVisible(); + await page.getByRole('button', { name: 'Cancel' }).click(); + }); + + await test.step('Only the other filter is rendered on the dashboard', async () => { + await dashboardPage.closeFiltersModal(); + await expect( + dashboardPage.getFilterSelectByName('Environment'), + ).toBeVisible(); + await expect( + dashboardPage.getFilterSelectByName('Conditions'), + ).toHaveCount(0); + }); + + await test.step('Deleting it leaves the other filter in place', async () => { + await dashboardPage.openEditFiltersModal(); + await dashboardPage.deleteFilterFromDashboard('Conditions'); + await expect( + dashboardPage.getFilterItemByName('Conditions'), + ).toHaveCount(0); + await expect( + dashboardPage.getFilterItemByName('Environment'), + ).toBeVisible(); + }); + }); + }, +); diff --git a/packages/app/tests/e2e/page-objects/DashboardPage.ts b/packages/app/tests/e2e/page-objects/DashboardPage.ts index b72a03b77c..974646c7d1 100644 --- a/packages/app/tests/e2e/page-objects/DashboardPage.ts +++ b/packages/app/tests/e2e/page-objects/DashboardPage.ts @@ -965,14 +965,10 @@ export class DashboardPage { await this.setFilterRequirement(variableOptions); if (appliesToSourceNames && appliesToSourceNames.length > 0) { - for (const appliesName of appliesToSourceNames) { - await this.appliesToSourceSelector.click(); - await this.page - .getByRole('option', { name: appliesName, exact: true }) - .click(); - } - // Close the dropdown so the save button is clickable. - await this.page.keyboard.press('Escape'); + await this.selectSourcesInMultiSelect( + this.appliesToSourceSelector, + appliesToSourceNames, + ); } const saveFilterButton = this.page.getByTestId('save-filter-button'); @@ -1150,6 +1146,11 @@ export class DashboardPage { return this.page.getByTestId(`dashboard-filter-item-${name}`); } + /** The filters list's summary of which sources a filter broadcasts to. */ + getFilterBroadcastTarget(name: string) { + return this.page.getByTestId(`dashboard-filter-applies-to-${name}`); + } + getFilterSelectByName(name: string) { return this.page.getByTestId(`dashboard-filter-select-${name}`); } @@ -1364,7 +1365,11 @@ export class DashboardPage { /** Switch the add-filter form between the available value types. */ async selectFilterType( - label: 'Queried values' | 'Static values' | 'PromQL label values', + label: + | 'Queried values' + | 'Static values' + | 'PromQL label values' + | 'Ad hoc keys and values', ) { await this.getFilterTypePicker().click(); await this.getFilterOption(label).click(); @@ -1529,6 +1534,60 @@ export class DashboardPage { }); } + /** The ad hoc filter form's ClickHouse / Prometheus source type control. */ + getAdhocFilterSourceTypeControl(): Locator { + return this.getFilterForm().getByTestId('adhoc-filter-source-type'); + } + + /** The ad hoc filter form's data sources multi-select. */ + getAdhocFilterSourcesInput(): Locator { + return this.getFilterForm().getByTestId('adhoc-filter-sources'); + } + + /** Switch the ad hoc filter form between ClickHouse and Prometheus sources. */ + async selectAdhocFilterSourceType(label: 'ClickHouse' | 'Prometheus') { + await this.getAdhocFilterSourceTypeControl() + .getByText(label, { exact: true }) + .click(); + } + + /** Add `sourceNames` to a source multi-select, then close its dropdown. */ + async selectSourcesInMultiSelect(input: Locator, sourceNames: string[]) { + for (const sourceName of sourceNames) { + await input.click(); + await this.getFilterOption(sourceName).click(); + } + // Close the dropdown so the save button is clickable. + await this.page.keyboard.press('Escape'); + } + + /** + * Add an ad hoc filter over `sourceNames` and save it. Assumes the Edit + * Filters modal is already open; leaves it open on the filters list, having + * waited for the new filter to land there. + */ + async addAdhocFilterToDashboard( + name: string, + sourceNames: string[], + options?: { variableName?: string }, + ) { + await this.addFiltersButton.click(); + await this.selectFilterType('Ad hoc keys and values'); + await this.getFilterNameInput().fill(name); + await this.selectSourcesInMultiSelect( + this.getAdhocFilterSourcesInput(), + sourceNames, + ); + if (options?.variableName !== undefined) { + await this.variableNameInput.fill(options.variableName); + } + await this.page.getByTestId('save-filter-button').click(); + await this.getFilterItemByName(name).waitFor({ + state: 'visible', + timeout: 10000, + }); + } + /** * The options offered by the dashboard filter dropdown that is currently * open, in render order. Scoped to visible nodes: a just-closed dropdown's diff --git a/packages/common-utils/src/__tests__/filters.test.ts b/packages/common-utils/src/__tests__/filters.test.ts index 3a68bc1a97..aff7b0326d 100644 --- a/packages/common-utils/src/__tests__/filters.test.ts +++ b/packages/common-utils/src/__tests__/filters.test.ts @@ -4,6 +4,7 @@ import { FilterState, filterStateToPredicate, filtersToQuery, + getAdhocFilterAppliesToSourceIds, getDashboardVariableDeclarations, getDashboardVariableFilters, getFilterBroadcastTarget, @@ -1718,6 +1719,29 @@ describe('filters', () => { }); }); + describe('getAdhocFilterAppliesToSourceIds', () => { + it('falls back to the filter sources when no scope is set', () => { + expect(getAdhocFilterAppliesToSourceIds({ sources: ['logs'] })).toEqual([ + 'logs', + ]); + expect( + getAdhocFilterAppliesToSourceIds({ + sources: ['logs'], + appliesToSourceIds: [], + }), + ).toEqual(['logs']); + }); + + it('returns the scoped sources when set', () => { + expect( + getAdhocFilterAppliesToSourceIds({ + sources: ['logs', 'traces'], + appliesToSourceIds: ['traces'], + }), + ).toEqual(['traces']); + }); + }); + describe('doesFilterApplyToSource', () => { const filter = ( overrides: Partial = {}, diff --git a/packages/common-utils/src/filters.ts b/packages/common-utils/src/filters.ts index 33cdf604b6..0ad3dc1ae6 100644 --- a/packages/common-utils/src/filters.ts +++ b/packages/common-utils/src/filters.ts @@ -3,6 +3,7 @@ import * as SQLParser from 'node-sql-parser'; import { escapeSqlString, replaceJsonExpressions } from '@/core/utils'; import { parse } from '@/queryParser'; import { + AdhocDashboardFilter, AdhocFilterSourceType, AdhocFilterValue, ChartVariable, @@ -923,6 +924,18 @@ export function isAdhocFilter( return filter.type === 'ADHOC'; } +/** + * The sources an ADHOC filter broadcasts its conditions to: its + * `appliesToSourceIds`, or its own `sources` when that is unset or empty. + */ +export function getAdhocFilterAppliesToSourceIds( + filter: Pick, +): string[] { + return filter.appliesToSourceIds?.length + ? filter.appliesToSourceIds + : filter.sources; +} + /** Type guard for the saved conditions of an ADHOC filter. */ export function isAdhocFilterValue( value: DashboardFilterValue, From 0d800d6056e81f7aad0678e30423a411f527cd2e Mon Sep 17 00:00:00 2001 From: Drew Davis Date: Thu, 15 Oct 2026 15:21:23 -0400 Subject: [PATCH 12/16] feat: Render adhoc dashboard filter inputs (#3343) --- .changeset/adhoc-filter-inputs.md | 6 + packages/app/src/DBDashboardPage.tsx | 13 +- packages/app/src/DashboardFilters.tsx | 78 ++-- .../src/__tests__/DashboardFilters.test.ts | 17 + .../AdhocConditionEditor.tsx | 56 +++ .../AdhocDashboardFilter.tsx | 185 ++++++++++ .../__tests__/AdhocDashboardFilter.test.tsx | 151 ++++++++ .../__tests__/useAdhocFilterOptions.test.tsx | 225 ++++++++++++ .../useAdhocFilterOptions.ts | 345 ++++++++++++++++++ .../components/DBSearchPageFilters/hooks.ts | 8 +- .../src/components/DashboardFilterLabel.tsx | 61 ++++ .../DashboardFiltersList.tsx | 9 - .../FilterPill/FilterAutocomplete.tsx | 2 +- .../FilterConditionEditor.stories.tsx | 13 + .../FilterPill/FilterConditionEditor.tsx | 8 + .../FilterPill/FilterPill.stories.tsx | 7 +- .../__tests__/FilterConditionEditor.test.tsx | 14 + .../components/FilterPill/filterOperators.ts | 9 + .../__tests__/useDashboardFilters.test.tsx | 153 ++++++++ .../usePresetDashboardFilters.test.tsx | 2 + .../app/src/hooks/useDashboardFilters.tsx | 75 +++- packages/app/src/hooks/useMetadata.tsx | 6 + .../src/hooks/usePromqlLabelFilterValues.tsx | 20 +- packages/app/src/hooks/usePromqlMetadata.ts | 40 +- .../features/dashboard-adhoc-filters.spec.ts | 214 ++++++++++- .../tests/e2e/page-objects/DashboardPage.ts | 88 ++++- packages/app/tests/e2e/utils/filters-param.ts | 12 +- .../__tests__/dashboardFilterValues.test.ts | 57 ++- .../src/__tests__/filters.test.ts | 23 +- .../src/clickhouse/__tests__/index.test.ts | 23 ++ packages/common-utils/src/clickhouse/index.ts | 15 + .../common-utils/src/dashboardFilterValues.ts | 39 +- packages/common-utils/src/filters.ts | 16 +- 33 files changed, 1852 insertions(+), 138 deletions(-) create mode 100644 .changeset/adhoc-filter-inputs.md create mode 100644 packages/app/src/components/AdhocDashboardFilter/AdhocConditionEditor.tsx create mode 100644 packages/app/src/components/AdhocDashboardFilter/AdhocDashboardFilter.tsx create mode 100644 packages/app/src/components/AdhocDashboardFilter/__tests__/AdhocDashboardFilter.test.tsx create mode 100644 packages/app/src/components/AdhocDashboardFilter/__tests__/useAdhocFilterOptions.test.tsx create mode 100644 packages/app/src/components/AdhocDashboardFilter/useAdhocFilterOptions.ts create mode 100644 packages/app/src/components/DashboardFilterLabel.tsx diff --git a/.changeset/adhoc-filter-inputs.md b/.changeset/adhoc-filter-inputs.md new file mode 100644 index 0000000000..48162bfae0 --- /dev/null +++ b/.changeset/adhoc-filter-inputs.md @@ -0,0 +1,6 @@ +--- +'@hyperdx/common-utils': patch +'@hyperdx/app': patch +--- + +feat: Render adhoc dashboard filter inputs diff --git a/packages/app/src/DBDashboardPage.tsx b/packages/app/src/DBDashboardPage.tsx index b7e102324b..cc6c0c873b 100644 --- a/packages/app/src/DBDashboardPage.tsx +++ b/packages/app/src/DBDashboardPage.tsx @@ -35,7 +35,6 @@ import { isTimeSeriesDisplayType, } from '@hyperdx/common-utils/dist/core/utils'; import { getBlockingRequiredFilterNames } from '@hyperdx/common-utils/dist/dashboardFilterValues'; -import { isAdhocFilter } from '@hyperdx/common-utils/dist/filters'; import { displayTypeRequiresSource, isBuilderChartConfig, @@ -1979,14 +1978,12 @@ function DBDashboardPage({ const [showFiltersModal, setShowFiltersModal] = useState(false); - // Ad hoc filters aren't rendered or applied yet - const filters = useMemo( - () => (dashboard?.filters ?? []).filter(filter => !isAdhocFilter(filter)), - [dashboard?.filters], - ); + const filters = useMemo(() => dashboard?.filters ?? [], [dashboard?.filters]); const { selectionByFilterId, setFilterValue, + adhocConditionsByFilterId, + setAdhocConditions, setFilterValueEntries, filterValueEntries, ignoredFilterExpressions, @@ -3351,6 +3348,8 @@ function DBDashboardPage({ filters={filters} selectionByFilterId={selectionByFilterId} onSetFilterValue={setFilterValue} + adhocConditionsByFilterId={adhocConditionsByFilterId} + onSetAdhocConditions={setAdhocConditions} dateRange={searchedTimeRange} variables={variables} /> @@ -3524,7 +3523,7 @@ function DBDashboardPage({ setShowFiltersModal(false)} - filters={dashboard?.filters ?? []} + filters={filters} onSaveFilter={handleSaveFilter} onRemoveFilter={handleRemoveFilter} isLoading={isSavingDashboard || isFetchingDashboard} diff --git a/packages/app/src/DashboardFilters.tsx b/packages/app/src/DashboardFilters.tsx index 9915d1154c..cf5f926230 100644 --- a/packages/app/src/DashboardFilters.tsx +++ b/packages/app/src/DashboardFilters.tsx @@ -4,6 +4,7 @@ import { getFilterBroadcastTarget, getFilterVariableName, getPendingFilterValuesVariables, + isAdhocFilter, isFilterGlobalRequirement, isFilterRequired, isFilterVariableEnabled, @@ -11,13 +12,20 @@ import { isStaticListFilter, } from '@hyperdx/common-utils/dist/filters'; import { + AdhocFilterCondition, ChartVariable, DashboardFilter, } from '@hyperdx/common-utils/dist/types'; import { isFilterSingleSelect } from '@hyperdx/common-utils/dist/variables'; -import { Group, Stack, Text, Tooltip } from '@mantine/core'; -import { IconAlertTriangle, IconHelp, IconRefresh } from '@tabler/icons-react'; +import { Group, Stack, Text } from '@mantine/core'; +import { IconRefresh } from '@tabler/icons-react'; +import { AdhocDashboardFilter } from './components/AdhocDashboardFilter/AdhocDashboardFilter'; +import { + DashboardFilterLabel, + FilterCaution, + FilterEffect, +} from './components/DashboardFilterLabel'; import { FilterLinkToggle } from './components/FilterLinkToggle'; import { VirtualMultiSelect } from './components/VirtualMultiSelect/VirtualMultiSelect'; import { useDashboardFilterValues } from './hooks/useDashboardFilterValues'; @@ -65,9 +73,7 @@ export const getRequiredFilterTooltip = (filter: DashboardFilter): string => * Describe what a filter does with the value you pick: broadcast it as a * condition, expose it as a variable, both, or neither. */ -export const getFilterEffect = ( - filter: DashboardFilter, -): { hasEffect: boolean; tooltip: string } => { +export const getFilterEffect = (filter: DashboardFilter): FilterEffect => { const parts: string[] = []; const broadcast = getFilterBroadcastTarget(filter); @@ -99,25 +105,6 @@ export const getFilterEffect = ( return { hasEffect: true, tooltip: parts.join(', ') }; }; -/** One of the caution icons a filter's label row can carry, with its tooltip. */ -const FilterCaution = ({ - label, - testId, - variant = 'warning', -}: { - label: string; - testId: string; - variant?: 'warning' | 'danger'; -}) => ( - - - -); - const DashboardFilterSelect = ({ filter, onChange, @@ -134,25 +121,7 @@ const DashboardFilterSelect = ({ return ( - - - {filter.name} - - - {effect.hasEffect ? ( - - ) : ( - - )} - + {isMissingRequiredValue && ( )} - +
; onSetFilterValue: (filterId: string, values: string[]) => void; + adhocConditionsByFilterId?: ReadonlyMap; + onSetAdhocConditions?: ( + filterId: string, + conditions: AdhocFilterCondition[], + ) => void; dateRange: [Date, Date]; /** * The dashboard's variables and their current selections. Defined only when @@ -217,6 +191,8 @@ const DashboardFilters = ({ dateRange, selectionByFilterId, onSetFilterValue, + adhocConditionsByFilterId, + onSetAdhocConditions, variables, }: DashboardFilterProps) => { // "Link" mode (opt-in, off by default): each dropdown's values are narrowed by @@ -248,6 +224,20 @@ const DashboardFilters = ({ return ( {filters.map(filter => { + if (isAdhocFilter(filter)) { + return ( + + onSetAdhocConditions?.(filter.id, conditions) + } + dateRange={dateRange} + effect={getFilterEffect(filter)} + /> + ); + } const queriedFilterValues = filterValuesById?.get(filter.id); const included = selectionByFilterId.get(filter.id)?.included; const selectedValues = included diff --git a/packages/app/src/__tests__/DashboardFilters.test.ts b/packages/app/src/__tests__/DashboardFilters.test.ts index d063d63a9c..5c5df455ab 100644 --- a/packages/app/src/__tests__/DashboardFilters.test.ts +++ b/packages/app/src/__tests__/DashboardFilters.test.ts @@ -117,6 +117,23 @@ describe('getFilterEffect', () => { }).tooltip, ).toEqual('Available as variable ($svc)'); }); + + it('counts the sources an ad hoc filter broadcasts to', () => { + expect( + getFilterEffect({ + id: 'adhoc1', + type: 'ADHOC', + name: 'Conditions', + sourceType: 'sql', + sources: ['logs', 'traces'], + isVariableEnabled: true, + variableName: 'conds', + }), + ).toEqual({ + hasEffect: true, + tooltip: 'Filters 2 sources, available as variable ($conds)', + }); + }); }); describe('getPendingVariablesTooltip', () => { diff --git a/packages/app/src/components/AdhocDashboardFilter/AdhocConditionEditor.tsx b/packages/app/src/components/AdhocDashboardFilter/AdhocConditionEditor.tsx new file mode 100644 index 0000000000..51135a5549 --- /dev/null +++ b/packages/app/src/components/AdhocDashboardFilter/AdhocConditionEditor.tsx @@ -0,0 +1,56 @@ +import { useState } from 'react'; +import { + AdhocDashboardFilter, + AdhocFilterCondition, +} from '@hyperdx/common-utils/dist/types'; + +import { + FilterConditionEditor, + FilterOperator, + getFilterOperators, +} from '@/components/FilterPill'; + +import { useAdhocFilterOptions } from './useAdhocFilterOptions'; + +type AdhocConditionEditorProps = { + filter: AdhocDashboardFilter; + dateRange: [Date, Date]; + /** The condition being edited; unset when adding one. */ + initial?: AdhocFilterCondition; + onSubmit: (condition: AdhocFilterCondition) => void; +}; + +/** + * Pick a condition from the filter's keys and values. Mounted only while its + * popover is open, so keys load on open and values once a key is picked. + */ +export function AdhocConditionEditor({ + filter, + dateRange, + initial, + onSubmit, +}: AdhocConditionEditorProps) { + const [valueKey, setValueKey] = useState(initial?.key ?? ''); + const { keys, values } = useAdhocFilterOptions(filter, valueKey, dateRange); + const isPromql = filter.sourceType === 'promql'; + + return ( + + operators={getFilterOperators(filter.sourceType)} + initial={initial} + keyOptions={keys.data} + isLoadingKeys={keys.isLoading} + isKeysError={keys.isError} + valueOptions={values.data} + isLoadingValues={values.isLoading} + isValuesError={values.isError} + onKeyChange={setValueKey} + keyLabel={isPromql ? 'Label' : 'Key'} + keyPlaceholder={ + isPromql ? 'Select a label' : 'Select a column or map key' + } + onSubmit={onSubmit} + data-testid={`adhoc-condition-editor-${filter.name}`} + /> + ); +} diff --git a/packages/app/src/components/AdhocDashboardFilter/AdhocDashboardFilter.tsx b/packages/app/src/components/AdhocDashboardFilter/AdhocDashboardFilter.tsx new file mode 100644 index 0000000000..a6c2bae9ba --- /dev/null +++ b/packages/app/src/components/AdhocDashboardFilter/AdhocDashboardFilter.tsx @@ -0,0 +1,185 @@ +import { useState } from 'react'; +import { + ADHOC_FILTER_MAX_CONDITIONS, + AdhocDashboardFilter as AdhocDashboardFilterType, + AdhocFilterCondition, + AdhocFilterConditionSchema, +} from '@hyperdx/common-utils/dist/types'; +import { + ActionIcon, + CloseButton, + Group, + Popover, + Stack, + Text, + Tooltip, +} from '@mantine/core'; +import { notifications } from '@mantine/notifications'; +import { IconPlus } from '@tabler/icons-react'; + +import { + DashboardFilterLabel, + FilterEffect, +} from '@/components/DashboardFilterLabel'; +import { FilterPill, getFilterOperatorDisplay } from '@/components/FilterPill'; + +import { AdhocConditionEditor } from './AdhocConditionEditor'; + +type AdhocDashboardFilterProps = { + filter: AdhocDashboardFilterType; + conditions: AdhocFilterCondition[]; + onChange: (conditions: AdhocFilterCondition[]) => void; + dateRange: [Date, Date]; + effect: FilterEffect; +}; + +/** A dashboard filter bar control holding one ad hoc filter's conditions. */ +export function AdhocDashboardFilter({ + filter, + conditions, + onChange, + dateRange, + effect, +}: AdhocDashboardFilterProps) { + const [isAdding, setIsAdding] = useState(false); + const isAtLimit = conditions.length >= ADHOC_FILTER_MAX_CONDITIONS; + + const toggleAdding = () => { + if (isAtLimit) { + notifications.show({ + id: `adhoc-filter-full-${filter.id}`, + color: 'yellow', + title: `${filter.name} is full`, + message: `A filter holds up to ${ADHOC_FILTER_MAX_CONDITIONS} conditions. Remove one to add another.`, + }); + return; + } + setIsAdding(!isAdding); + }; + // The URL parser drops an invalid entry whole, so one bad condition would + // lose the filter's others on reload. Keep the editor open to fix it instead. + const isStorable = (condition: AdhocFilterCondition) => { + if (AdhocFilterConditionSchema.safeParse(condition).success) return true; + const maxLength = AdhocFilterConditionSchema.shape.value.maxLength; + notifications.show({ + id: `adhoc-filter-too-long-${filter.id}`, + color: 'yellow', + title: 'Condition is too long', + message: `Keys and values can be up to ${maxLength?.toLocaleString()} characters.`, + }); + return false; + }; + const replace = (index: number, condition: AdhocFilterCondition) => + onChange( + conditions.map((existing, i) => (i === index ? condition : existing)), + ); + const remove = (index: number) => + onChange(conditions.filter((_existing, i) => i !== index)); + + return ( + + + + {conditions.map((condition, index) => { + const operator = getFilterOperatorDisplay( + filter.sourceType, + condition.operator, + ); + return ( + remove(index)} + renderPopover={close => ( + { + if (!isStorable(next)) return; + replace(index, next); + close(); + }} + /> + )} + data-testid="adhoc-condition-pill" + /> + ); + })} + {conditions.length === 0 && ( + setIsAdding(true)} + style={{ cursor: 'pointer', flex: 1 }} + > + {filter.name} + + )} + + {conditions.length > 0 && ( + onChange([])} + data-testid={`adhoc-filter-clear-${filter.name}`} + /> + )} + + + + + + + + + + { + if (!isStorable(condition)) return; + onChange([...conditions, condition]); + setIsAdding(false); + }} + /> + + + + + + ); +} diff --git a/packages/app/src/components/AdhocDashboardFilter/__tests__/AdhocDashboardFilter.test.tsx b/packages/app/src/components/AdhocDashboardFilter/__tests__/AdhocDashboardFilter.test.tsx new file mode 100644 index 0000000000..bae4b1d5f0 --- /dev/null +++ b/packages/app/src/components/AdhocDashboardFilter/__tests__/AdhocDashboardFilter.test.tsx @@ -0,0 +1,151 @@ +import { + ADHOC_FILTER_MAX_CONDITIONS, + AdhocDashboardFilter as AdhocDashboardFilterType, + AdhocFilterCondition, +} from '@hyperdx/common-utils/dist/types'; +import { notifications } from '@mantine/notifications'; +import { screen } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; + +import { AdhocDashboardFilter } from '@/components/AdhocDashboardFilter/AdhocDashboardFilter'; +import { useAdhocFilterOptions } from '@/components/AdhocDashboardFilter/useAdhocFilterOptions'; + +jest.mock('@mantine/notifications', () => ({ + notifications: { show: jest.fn() }, +})); + +jest.mock('@/components/AdhocDashboardFilter/useAdhocFilterOptions', () => ({ + useAdhocFilterOptions: jest.fn(), +})); + +const filter: AdhocDashboardFilterType = { + id: 'adhoc1', + type: 'ADHOC', + name: 'Conditions', + sourceType: 'sql', + sources: ['logs'], + isVariableEnabled: true, + variableName: 'conds', +}; + +const dateRange: [Date, Date] = [new Date(0), new Date(1000)]; + +const existing: AdhocFilterCondition = { + key: 'ServiceName', + operator: '!=', + value: 'api', +}; + +const editorId = 'adhoc-condition-editor-Conditions'; + +const addButton = () => screen.getByTestId('adhoc-filter-add-Conditions'); + +const renderFilter = (conditions: AdhocFilterCondition[]) => { + const onChange = jest.fn(); + renderWithMantine( + , + ); + return onChange; +}; + +describe('AdhocDashboardFilter', () => { + beforeEach(() => { + jest.mocked(useAdhocFilterOptions).mockReturnValue({ + keys: { + data: ['ServiceName', 'SeverityText'], + isLoading: false, + isError: false, + }, + values: { data: ['api', 'web'], isLoading: false, isError: false }, + }); + }); + + it('renders a pill per condition', () => { + renderFilter([existing, { key: 'Level', operator: '=~', value: '^err' }]); + + const pills = screen.getAllByTestId('adhoc-condition-pill'); + expect(pills[0]).toHaveTextContent('ServiceName != api'); + expect(pills[1]).toHaveTextContent('Level regex ^err'); + }); + + it('removes a condition', async () => { + const onChange = renderFilter([ + existing, + { key: 'Level', operator: '=', value: 'error' }, + ]); + + await userEvent.click( + screen.getAllByRole('button', { name: 'Remove filter' })[0], + ); + + expect(onChange).toHaveBeenCalledWith([ + { key: 'Level', operator: '=', value: 'error' }, + ]); + }); + + it('stops adding conditions at the limit', async () => { + renderFilter( + Array.from({ length: ADHOC_FILTER_MAX_CONDITIONS }, (_, i) => ({ + ...existing, + value: `${i}`, + })), + ); + + expect(addButton()).toHaveAttribute('aria-disabled', 'true'); + await userEvent.click(addButton()); + expect(screen.queryByTestId(`${editorId}-key`)).not.toBeInTheDocument(); + expect(notifications.show).toHaveBeenCalledWith( + expect.objectContaining({ title: 'Conditions is full' }), + ); + }); + + it('edits a condition from its pill', async () => { + const onChange = renderFilter([ + existing, + { key: 'Level', operator: '=', value: 'error' }, + ]); + + await userEvent.click(screen.getAllByTestId('adhoc-condition-pill')[0]); + const valueInput = await screen.findByTestId(`${editorId}-value`); + expect(valueInput).toHaveValue('api'); + await userEvent.clear(valueInput); + await userEvent.type(valueInput, 'web'); + await userEvent.click(screen.getByTestId(`${editorId}-apply`)); + + expect(onChange).toHaveBeenCalledWith([ + { key: 'ServiceName', operator: '!=', value: 'web' }, + { key: 'Level', operator: '=', value: 'error' }, + ]); + }); + + it('warns instead of applying a value too long for the URL', async () => { + const onChange = renderFilter([existing]); + + await userEvent.click(screen.getAllByTestId('adhoc-condition-pill')[0]); + const valueInput = await screen.findByTestId(`${editorId}-value`); + await userEvent.clear(valueInput); + await userEvent.click(valueInput); + await userEvent.paste('x'.repeat(10001)); + await userEvent.click(screen.getByTestId(`${editorId}-apply`)); + + expect(onChange).not.toHaveBeenCalled(); + expect(notifications.show).toHaveBeenCalledWith( + expect.objectContaining({ title: 'Condition is too long' }), + ); + expect(valueInput).toBeInTheDocument(); + }); + + it('clears every condition', async () => { + const onChange = renderFilter([existing]); + + await userEvent.click(screen.getByTestId('adhoc-filter-clear-Conditions')); + + expect(onChange).toHaveBeenCalledWith([]); + }); +}); diff --git a/packages/app/src/components/AdhocDashboardFilter/__tests__/useAdhocFilterOptions.test.tsx b/packages/app/src/components/AdhocDashboardFilter/__tests__/useAdhocFilterOptions.test.tsx new file mode 100644 index 0000000000..11dd946059 --- /dev/null +++ b/packages/app/src/components/AdhocDashboardFilter/__tests__/useAdhocFilterOptions.test.tsx @@ -0,0 +1,225 @@ +import { ReactNode } from 'react'; +import { ColumnMeta, JSDataType } from '@hyperdx/common-utils/dist/clickhouse'; +import { + AdhocDashboardFilter, + SourceKind, + TSource, +} from '@hyperdx/common-utils/dist/types'; +import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; +import { renderHook, waitFor } from '@testing-library/react'; + +import { + getSourcesWithKey, + getSqlSourceColumnKeys, + getSqlSourceKeys, + getValueSuggestions, + useAdhocFilterOptions, +} from '@/components/AdhocDashboardFilter/useAdhocFilterOptions'; +import { + useMultipleAllFields, + useMultipleGetKeyValues, +} from '@/hooks/useMetadata'; +import { + fetchPromqlLabelNames, + fetchPromqlLabelValues, +} from '@/hooks/usePromqlMetadata'; + +let mockSources: Partial[] = []; + +jest.mock('@/source', () => ({ + useSources: () => ({ data: mockSources }), +})); +jest.mock('@/queryAttribution', () => ({ + useMetadataQueryAttribution: () => undefined, +})); +jest.mock('@/hooks/useMetadata', () => ({ + useMultipleAllFields: jest.fn(), + useMultipleGetKeyValues: jest.fn(() => ({ + data: undefined, + isFetching: false, + isError: false, + })), +})); +jest.mock('@/hooks/usePromqlMetadata', () => ({ + ...jest.requireActual('@/hooks/usePromqlMetadata'), + fetchPromqlLabelNames: jest.fn(), + fetchPromqlLabelValues: jest.fn(), +})); + +const column = (name: string, type: string): ColumnMeta => ({ + name, + type, + codec_expression: '', + comment: '', + default_expression: '', + default_type: '', + ttl_expression: '', +}); + +describe('getSqlSourceKeys', () => { + it('offers scalar columns and the keys inside map and JSON columns, not the whole columns', () => { + expect( + getSqlSourceKeys( + [ + { path: ['ServiceName'], type: 'String', jsType: JSDataType.String }, + { path: ['LogAttributes'], type: 'Map', jsType: JSDataType.Map }, + { path: ['LogAttributes', '1'], type: 'String', jsType: null }, + { path: ['Body'], type: 'JSON', jsType: JSDataType.JSON }, + { path: ['Body', 'user.id'], type: 'String', jsType: null }, + ], + [ + column('ServiceName', 'LowCardinality(String)'), + column('LogAttributes', 'Map(LowCardinality(String), String)'), + column('Body', 'JSON'), + ], + ), + ).toEqual(['ServiceName', "LogAttributes['1']", 'Body.`user`.`id`']); + }); +}); + +describe('getSqlSourceColumnKeys', () => { + it('offers only the scalar columns', () => { + expect( + getSqlSourceColumnKeys([ + column('ServiceName', 'LowCardinality(String)'), + column('LogAttributes', 'Map(LowCardinality(String), String)'), + ]), + ).toEqual(['ServiceName']); + }); +}); + +describe('getValueSuggestions', () => { + it('deduplicates scalar values and leaves out map and JSON values', () => { + const mapValue = JSON.parse('{"service.name":"api"}'); + expect( + getValueSuggestions([ + { key: 'ResourceAttributes', value: [mapValue, 'b', 'a'] }, + { key: 'ResourceAttributes', value: ['a', JSON.parse('null')] }, + ]), + ).toEqual(['a', 'b']); + }); +}); + +describe('getSourcesWithKey', () => { + const sources = [{ id: 'logs' }, { id: 'traces' }]; + + it('looks up a key no source is known to have in every source', () => { + expect( + getSourcesWithKey(sources, new Map([['logs', ['ServiceName']]]), 'typed'), + ).toEqual(sources); + }); +}); + +describe('useAdhocFilterOptions', () => { + const DATE_RANGE: [Date, Date] = [ + new Date('2026-01-01T00:00:00Z'), + new Date('2026-01-01T01:00:00Z'), + ]; + const source = (id: string, kind: SourceKind): Partial => ({ + id, + kind, + connection: 'conn', + from: { databaseName: 'default', tableName: id }, + timestampValueExpression: 'Timestamp', + }); + const filter = ( + sourceType: AdhocDashboardFilter['sourceType'], + sources: string[], + ): AdhocDashboardFilter => ({ + id: 'f', + type: 'ADHOC', + name: 'Conditions', + sourceType, + sources, + isVariableEnabled: true, + }); + + /** `useMultipleAllFields` with the given columns per source, by index. */ + const mockAllFields = ( + columnsBySource: ColumnMeta[][], + { isLoading = false } = {}, + ) => + jest.mocked(useMultipleAllFields).mockReturnValue({ + fieldsByTableConnection: new Map(), + columnsByTableConnection: new Map(columnsBySource.entries()), + isFieldMetadataDisabled: true, + isLoading, + isError: false, + } as unknown as ReturnType); + + const renderOptions = ( + adhocFilter: AdhocDashboardFilter, + key = 'SeverityText', + ) => { + const queryClient = new QueryClient(); + const wrapper = ({ children }: { children: ReactNode }) => ( + {children} + ); + return renderHook( + () => useAdhocFilterOptions(adhocFilter, key, DATE_RANGE), + { wrapper }, + ); + }; + + beforeEach(() => { + mockAllFields([]); + jest.mocked(fetchPromqlLabelNames).mockResolvedValue([]); + jest.mocked(fetchPromqlLabelValues).mockResolvedValue([]); + }); + + const lastSqlLookup = () => + jest.mocked(useMultipleGetKeyValues).mock.calls.at(-1)!; + + it('waits for keys, then looks up values only in the sources that have the key', () => { + mockSources = [ + source('logs', SourceKind.Log), + source('traces', SourceKind.Trace), + ]; + mockAllFields([], { isLoading: true }); + const { result, rerender } = renderOptions( + filter('sql', ['logs', 'traces']), + ); + + expect(lastSqlLookup()[0].keys).toEqual([]); + expect(result.current.values.isLoading).toBe(true); + + mockAllFields([ + [column('SeverityText', 'LowCardinality(String)')], + [column('SpanName', 'LowCardinality(String)')], + ]); + rerender(); + + expect(result.current.keys.data).toEqual(['SeverityText', 'SpanName']); + const [{ chartConfigs, keys }] = lastSqlLookup(); + expect(keys).toEqual(['SeverityText']); + expect(chartConfigs).toEqual([expect.objectContaining({ source: 'logs' })]); + }); + + it('quotes a map column name that is not a bare identifier', () => { + mockSources = [source('logs', SourceKind.Log)]; + mockAllFields([[column('log-attributes', 'Map(String, String)')]]); + renderOptions(filter('sql', ['logs']), "log-attributes['service']"); + + expect(lastSqlLookup()[0].keys).toEqual(["`log-attributes`['service']"]); + }); + + it('leaves the SQL lookup disabled for a PromQL filter', async () => { + mockSources = [source('prom', SourceKind.Promql)]; + jest + .mocked(fetchPromqlLabelNames) + .mockResolvedValue(['__name__', 'SeverityText']); + const { result } = renderOptions(filter('promql', ['prom'])); + + await waitFor(() => + expect(result.current.keys.data).toEqual(['SeverityText']), + ); + expect(lastSqlLookup()[1]).toEqual( + expect.objectContaining({ enabled: false }), + ); + await waitFor(() => + expect(fetchPromqlLabelValues).toHaveBeenCalledWith( + expect.objectContaining({ label: 'SeverityText', sourceId: 'prom' }), + ), + ); + }); +}); diff --git a/packages/app/src/components/AdhocDashboardFilter/useAdhocFilterOptions.ts b/packages/app/src/components/AdhocDashboardFilter/useAdhocFilterOptions.ts new file mode 100644 index 0000000000..e54151862d --- /dev/null +++ b/packages/app/src/components/AdhocDashboardFilter/useAdhocFilterOptions.ts @@ -0,0 +1,345 @@ +import { useCallback, useMemo } from 'react'; +import { + ColumnMeta, + convertCHDataTypeToJSType, + filterColumnMetaByType, + isJSDataTypeScalar, + JSDataType, + QueryAttribution, +} from '@hyperdx/common-utils/dist/clickhouse'; +import { Field, tcFromSource } from '@hyperdx/common-utils/dist/core/metadata'; +import { getAdhocFilterSourceKinds } from '@hyperdx/common-utils/dist/filters'; +import { + AdhocDashboardFilter, + TSource, +} from '@hyperdx/common-utils/dist/types'; +import { + keepPreviousData, + useQueries, + UseQueryOptions, + UseQueryResult, +} from '@tanstack/react-query'; + +import { toQuotedClickHouseKeyExpression } from '@/components/DBSearchPageFilters/utils'; +import { + Facet, + SINGLE_KEY_VALUES_LIMIT, + useMultipleAllFields, + useMultipleGetKeyValues, +} from '@/hooks/useMetadata'; +import { + fetchPromqlLabelNames, + fetchPromqlLabelValues, + toPromqlSeconds, +} from '@/hooks/usePromqlMetadata'; +import { useMetadataQueryAttribution } from '@/queryAttribution'; +import { pickSourceConfigFields } from '@/ServicesDashboardPage/helpers'; +import { useSources } from '@/source'; +import { mergePath } from '@/utils'; + +const STALE_TIME_MS = 1000 * 60 * 5; +const NO_SOURCES: TSource[] = []; +const NO_COLUMNS: Set = new Set(); + +const unionSorted = (lists: (string[] | undefined)[]) => + Array.from(new Set(lists.flatMap(list => list ?? []))).sort(); + +/** + * A SQL source's filterable keys, in the search sidebar's key format: scalar + * columns, and the keys inside its map and JSON columns. + */ +export function getSqlSourceKeys(fields: Field[], columns: ColumnMeta[]) { + const columnNames = (types: JSDataType[]) => + filterColumnMetaByType(columns, types)?.map(column => column.name) ?? []; + const jsonColumns = columnNames([JSDataType.JSON]); + const mapColumns = columnNames([JSDataType.Map]); + return fields + .filter(field => field.path.length > 1 || isJSDataTypeScalar(field.jsType)) + .map(field => mergePath(field.path, jsonColumns, mapColumns)); +} + +/** A SQL source's scalar columns, the only keys known without field metadata. */ +export function getSqlSourceColumnKeys(columns: ColumnMeta[]) { + return columns + .filter(column => + isJSDataTypeScalar(convertCHDataTypeToJSType(column.type)), + ) + .map(column => column.name); +} + +/** + * Suggestions from looked-up values. A key typed by hand can name a whole map + * or JSON column, whose values are objects rather than suggestions. + */ +export function getValueSuggestions(facets: Facet[] | undefined) { + return unionSorted( + (facets ?? []).map(({ value }) => + value + .filter((v: unknown) => v != null && typeof v !== 'object') + .map(String), + ), + ); +} + +/** + * The sources to look up `key`'s values in: those known to have it, so a source + * without the column doesn't fail its query, or every source when none are + * known to (a key typed by hand). + */ +export function getSourcesWithKey( + sources: T[], + keysBySourceId: ReadonlyMap, + key: string, +) { + const withKey = sources.filter(source => + keysBySourceId.get(source.id)?.includes(key), + ); + return withKey.length > 0 ? withKey : sources; +} + +/** The filter's sources, limited to the kinds its source type reads. */ +function useAdhocFilterSources(filter: AdhocDashboardFilter) { + const { data: allSources } = useSources(); + return useMemo(() => { + const kinds = getAdhocFilterSourceKinds(filter.sourceType); + return (allSources ?? []).filter( + source => + filter.sources.includes(source.id) && kinds.includes(source.kind), + ); + }, [allSources, filter.sources, filter.sourceType]); +} + +/** Suggestions for one input of the condition editor. */ +type AdhocFilterSuggestions = { + data: string[]; + isLoading: boolean; + isError: boolean; +}; + +type AdhocFilterKeys = AdhocFilterSuggestions & { + keysBySourceId: ReadonlyMap; + /** Top-level column names across SQL sources, for quoting keys in SQL. */ + columnNames: Set; +}; + +function useSqlSourceKeys( + sources: TSource[], + dateRange: [Date, Date], +): AdhocFilterKeys { + const tableConnections = useMemo(() => sources.map(tcFromSource), [sources]); + const { + fieldsByTableConnection, + columnsByTableConnection, + isFieldMetadataDisabled, + isLoading, + isError, + } = useMultipleAllFields(tableConnections, { dateRange }); + + return useMemo(() => { + const keysBySourceId = new Map(); + const columnNames = new Set(); + sources.forEach((source, index) => { + const columns = columnsByTableConnection.get(index); + if (!columns) return; + columns.forEach(column => columnNames.add(column.name)); + keysBySourceId.set( + source.id, + isFieldMetadataDisabled + ? getSqlSourceColumnKeys(columns) + : getSqlSourceKeys(fieldsByTableConnection.get(index) ?? [], columns), + ); + }); + return { + data: unionSorted([...keysBySourceId.values()]), + keysBySourceId, + columnNames, + isLoading, + isError, + }; + }, [ + sources, + fieldsByTableConnection, + columnsByTableConnection, + isFieldMetadataDisabled, + isLoading, + isError, + ]); +} + +const promqlLookupParams = ( + source: TSource, + { start, end }: { start: number; end: number }, + attribution: QueryAttribution, +) => ({ + sourceId: source.id, + start, + end, + attribution, +}); + +function usePromqlSourceKeys( + sources: TSource[], + dateRange: [Date, Date], +): AdhocFilterKeys { + const attribution = useMetadataQueryAttribution(); + const seconds = toPromqlSeconds(dateRange); + + const combineKeys = useCallback( + (results: UseQueryResult[]) => { + const keysBySourceId = new Map(); + sources.forEach((source, index) => { + const keys = results[index]?.data; + if (keys) keysBySourceId.set(source.id, keys); + }); + return { + data: unionSorted([...keysBySourceId.values()]), + keysBySourceId, + columnNames: NO_COLUMNS, + isLoading: results.some(result => result.isLoading), + isError: results.some(result => result.isError), + }; + }, + [sources], + ); + + return useQueries({ + queries: sources.map( + (source): UseQueryOptions => ({ + queryKey: [ + 'adhoc-filter-promql-keys', + source.id, + seconds.start, + seconds.end, + ], + queryFn: async () => { + const labels = await fetchPromqlLabelNames( + promqlLookupParams(source, seconds, attribution), + ); + return labels.filter(label => label !== '__name__'); + }, + staleTime: STALE_TIME_MS, + placeholderData: keepPreviousData, + }), + ), + combine: combineKeys, + }); +} + +function useSqlSourceValues( + sources: TSource[], + key: string, + dateRange: [Date, Date], + columnNames: Set, +): AdhocFilterSuggestions { + const chartConfigs = useMemo( + () => + sources.map(source => ({ + ...pickSourceConfigFields(source), + dateRange, + source: source.id, + select: '', + where: '', + whereLanguage: 'sql' as const, + })), + [sources, dateRange], + ); + // Quoted like the search sidebar does, so a column named e.g. + // `log-attributes` isn't read as a subtraction. + const keys = useMemo( + () => (key ? [toQuotedClickHouseKeyExpression(key, columnNames)] : []), + [key, columnNames], + ); + const { data, isFetching, isError } = useMultipleGetKeyValues( + { chartConfigs, keys, limit: SINGLE_KEY_VALUES_LIMIT }, + // The previous key's values aren't suggestions for this one. + { placeholderData: undefined, retry: false, enabled: sources.length > 0 }, + ); + + const values = useMemo(() => getValueSuggestions(data), [data]); + return { data: values, isLoading: !!key && isFetching, isError }; +} + +const combineValues = ( + results: UseQueryResult[], +): AdhocFilterSuggestions => ({ + data: unionSorted(results.map(result => result.data)), + isLoading: results.some(result => result.isFetching), + isError: results.some(result => result.isError), +}); + +function usePromqlSourceValues( + sources: TSource[], + key: string, + dateRange: [Date, Date], +): AdhocFilterSuggestions { + const attribution = useMetadataQueryAttribution(); + const seconds = toPromqlSeconds(dateRange); + + return useQueries({ + queries: sources.map( + (source): UseQueryOptions => ({ + queryKey: [ + 'adhoc-filter-promql-values', + source.id, + key, + seconds.start, + seconds.end, + ], + queryFn: () => + fetchPromqlLabelValues({ + label: key, + ...promqlLookupParams(source, seconds, attribution), + }), + enabled: !!key, + staleTime: STALE_TIME_MS, + retry: false, + }), + ), + combine: combineValues, + }); +} + +/** + * Suggestions for an ad hoc filter's condition editor, deduplicated across its + * sources. Keys are the columns and map / JSON keys of its SQL sources, in the + * search sidebar's key format, or the label names of its PromQL sources. + * Values are those observed for `key`. + */ +export function useAdhocFilterOptions( + filter: AdhocDashboardFilter, + key: string, + dateRange: [Date, Date], +): { keys: AdhocFilterSuggestions; values: AdhocFilterSuggestions } { + const sources = useAdhocFilterSources(filter); + const isPromql = filter.sourceType === 'promql'; + const sqlKeys = useSqlSourceKeys(isPromql ? NO_SOURCES : sources, dateRange); + const promqlKeys = usePromqlSourceKeys( + isPromql ? sources : NO_SOURCES, + dateRange, + ); + const keys = isPromql ? promqlKeys : sqlKeys; + + // Until keys load, every source would be queried, then the sources known to + // have `key` queried again. + const lookupKey = keys.isLoading ? '' : key; + const sourcesWithKey = useMemo( + () => getSourcesWithKey(sources, keys.keysBySourceId, lookupKey), + [sources, keys.keysBySourceId, lookupKey], + ); + const sqlValues = useSqlSourceValues( + isPromql ? NO_SOURCES : sourcesWithKey, + lookupKey, + dateRange, + keys.columnNames, + ); + const promqlValues = usePromqlSourceValues( + isPromql ? sourcesWithKey : NO_SOURCES, + lookupKey, + dateRange, + ); + const values = isPromql ? promqlValues : sqlValues; + return { + keys, + values: key && keys.isLoading ? { ...values, isLoading: true } : values, + }; +} diff --git a/packages/app/src/components/DBSearchPageFilters/hooks.ts b/packages/app/src/components/DBSearchPageFilters/hooks.ts index be73a26336..3c6b0d657c 100644 --- a/packages/app/src/components/DBSearchPageFilters/hooks.ts +++ b/packages/app/src/components/DBSearchPageFilters/hooks.ts @@ -16,6 +16,7 @@ import { import { Facet, + SINGLE_KEY_VALUES_LIMIT, useAllFields, useColumns, useDateTimeColumns, @@ -32,9 +33,6 @@ import { toQuotedClickHouseKeyExpression } from './utils'; const INITIAL_LOAD_LIMIT = 20; -/* The maximum number of values per filter to load when "Load More" is clicked */ -const LOAD_MORE_LOAD_LIMIT = 10000; - /** * Decide which table key-value discovery reads from. * @@ -230,7 +228,7 @@ function useFacets({ ), }, keys: [sqlKey], - limit: LOAD_MORE_LOAD_LIMIT, + limit: SINGLE_KEY_VALUES_LIMIT, disableRowLimit: true, source, }); @@ -255,7 +253,7 @@ function useFacets({ connectionId: tableConnection.connectionId, metadataMVs: tableConnection.metadataMVs, keyExpressions: [sqlKey], - maxValuesPerKey: LOAD_MORE_LOAD_LIMIT, + maxValuesPerKey: SINGLE_KEY_VALUES_LIMIT, dateRange, timestampValueExpression: source?.timestampValueExpression ?? diff --git a/packages/app/src/components/DashboardFilterLabel.tsx b/packages/app/src/components/DashboardFilterLabel.tsx new file mode 100644 index 0000000000..37f8927fbc --- /dev/null +++ b/packages/app/src/components/DashboardFilterLabel.tsx @@ -0,0 +1,61 @@ +import { ReactNode } from 'react'; +import { Group, Text, Tooltip } from '@mantine/core'; +import { IconAlertTriangle, IconHelp } from '@tabler/icons-react'; + +/** What a filter does with its value, from `getFilterEffect`. */ +export type FilterEffect = { hasEffect: boolean; tooltip: string }; + +/** One of the caution icons a filter's label row can carry, with its tooltip. */ +export const FilterCaution = ({ + label, + testId, + variant = 'warning', +}: { + label: string; + testId: string; + variant?: 'warning' | 'danger'; +}) => ( + + + +); + +/** + * The row above a dashboard filter's input: its name, what it does, and any + * cautions passed as children. + */ +export const DashboardFilterLabel = ({ + name, + effect, + children, +}: { + name: string; + effect: FilterEffect; + children?: ReactNode; +}) => ( + + + {name} + + + {effect.hasEffect ? ( + + ) : ( + + )} + + {children} + +); diff --git a/packages/app/src/components/DashboardFiltersModal/DashboardFiltersList.tsx b/packages/app/src/components/DashboardFiltersModal/DashboardFiltersList.tsx index 5f92373c35..22e99756ed 100644 --- a/packages/app/src/components/DashboardFiltersModal/DashboardFiltersList.tsx +++ b/packages/app/src/components/DashboardFiltersModal/DashboardFiltersList.tsx @@ -1,9 +1,6 @@ import { - getAdhocFilterAppliesToSourceIds, getFilterBroadcastTarget, getFilterVariableName, - isAdhocFilter, - isFilterBroadcastEnabled, isFilterGlobalRequirement, isFilterRequired, isFilterVariableEnabled, @@ -104,12 +101,6 @@ function getBroadcastTargetDisplay( filter: DashboardFilter, sources?: { id: string; name: string }[], ) { - // An ad hoc filter broadcasts to its own sources unless narrowed further - if (isAdhocFilter(filter)) { - if (!isFilterBroadcastEnabled(filter)) return undefined; - return getSourceNames(getAdhocFilterAppliesToSourceIds(filter), sources); - } - const broadcastTarget = getFilterBroadcastTarget(filter); if (!broadcastTarget) return undefined; diff --git a/packages/app/src/components/FilterPill/FilterAutocomplete.tsx b/packages/app/src/components/FilterPill/FilterAutocomplete.tsx index 60e3a1c499..4a68d8aeb0 100644 --- a/packages/app/src/components/FilterPill/FilterAutocomplete.tsx +++ b/packages/app/src/components/FilterPill/FilterAutocomplete.tsx @@ -26,7 +26,7 @@ export type FilterAutocompleteProps = { 'aria-label'?: string; 'data-testid'?: string; ref?: Ref; -} & Pick; +} & Pick; /** Suggestion input for filter keys and values that also accepts free text. */ export function FilterAutocomplete({ diff --git a/packages/app/src/components/FilterPill/FilterConditionEditor.stories.tsx b/packages/app/src/components/FilterPill/FilterConditionEditor.stories.tsx index 7db25cea43..c0ff2ad772 100644 --- a/packages/app/src/components/FilterPill/FilterConditionEditor.stories.tsx +++ b/packages/app/src/components/FilterPill/FilterConditionEditor.stories.tsx @@ -112,3 +112,16 @@ export const LoadingValues = () => ( export const NoSuggestions = () => ( ); + +export const KeysError = () => ( + +); + +export const ValuesError = () => ( + +); diff --git a/packages/app/src/components/FilterPill/FilterConditionEditor.tsx b/packages/app/src/components/FilterPill/FilterConditionEditor.tsx index e5486e544a..fbee6097c6 100644 --- a/packages/app/src/components/FilterPill/FilterConditionEditor.tsx +++ b/packages/app/src/components/FilterPill/FilterConditionEditor.tsx @@ -26,9 +26,11 @@ export type FilterConditionEditorProps = { initial?: FilterCondition; keyOptions: string[]; isLoadingKeys?: boolean; + isKeysError?: boolean; /** Suggestions for the key last reported by `onKeyChange`. */ valueOptions: string[]; isLoadingValues?: boolean; + isValuesError?: boolean; /** The trimmed key, once typing pauses, so the caller can fetch its values. */ onKeyChange?: (key: string) => void; keyLabel?: string; @@ -38,6 +40,7 @@ export type FilterConditionEditorProps = { }; const KEY_DEBOUNCE_MS = 300; +const SUGGESTIONS_ERROR = "Couldn't load suggestions"; /** Pick a key, an operator, and a value. Free text is accepted for both inputs. */ export function FilterConditionEditor({ @@ -45,8 +48,10 @@ export function FilterConditionEditor({ initial, keyOptions, isLoadingKeys, + isKeysError, valueOptions, isLoadingValues, + isValuesError, onKeyChange, keyLabel = 'Key', keyPlaceholder = 'Select a key', @@ -69,6 +74,7 @@ export function FilterConditionEditor({ // Values shown while the key is still settling belong to the previous key. const isValueLoading = !!trimmedKey && (!!isLoadingValues || debouncedKey !== trimmedKey); + const isValuesErrorShown = !!trimmedKey && !isValueLoading && !!isValuesError; const selectedOperator = operators.find(op => op.value === operator); const canSubmit = !!trimmedKey && operator != null; @@ -84,6 +90,7 @@ export function FilterConditionEditor({ placeholder={keyPlaceholder} options={keyOptions} isLoading={isLoadingKeys} + error={!isLoadingKeys && isKeysError ? SUGGESTIONS_ERROR : undefined} value={key} onChange={setKey} // Enter moves on to the value instead of applying an empty-value filter. @@ -105,6 +112,7 @@ export function FilterConditionEditor({ placeholder={selectedOperator?.valuePlaceholder ?? 'Select a value'} options={valueOptions} isLoading={isValueLoading} + error={isValuesErrorShown ? SUGGESTIONS_ERROR : undefined} value={value} onChange={setValue} onSubmit={submit} diff --git a/packages/app/src/components/FilterPill/FilterPill.stories.tsx b/packages/app/src/components/FilterPill/FilterPill.stories.tsx index 0f16ab08ab..b872d92105 100644 --- a/packages/app/src/components/FilterPill/FilterPill.stories.tsx +++ b/packages/app/src/components/FilterPill/FilterPill.stories.tsx @@ -15,6 +15,7 @@ import { import { FilterLanguage, FilterOperator, + getFilterOperatorDisplay, getFilterOperators, } from './filterOperators'; import { FilterPill } from './FilterPill'; @@ -149,7 +150,7 @@ function ConditionEditorDemo() { language === 'promql' ? MOCK_PROMQL_VALUES : MOCK_SQL_VALUES; const values = useMockValues(mockValues, key); const operators = getFilterOperators(language); - const operator = operators.find(op => op.value === condition.operator); + const operator = getFilterOperatorDisplay(language, condition.operator); return ( @@ -171,9 +172,9 @@ function ConditionEditorDemo() { /> {}} renderPopover={close => ( { ).toBeInTheDocument(); }); + it('shows an error on the key input when keys fail to load', () => { + renderEditor({ isKeysError: true }); + expect(screen.getByText("Couldn't load suggestions")).toBeInTheDocument(); + }); + + it('shows a value error only once the key settles', async () => { + renderEditor({ isValuesError: true }); + expect(screen.queryByText("Couldn't load suggestions")).toBeNull(); + await userEvent.type(keyInput(), 'ServiceName'); + expect( + await screen.findByText("Couldn't load suggestions"), + ).toBeInTheDocument(); + }); + it('submits free-form text on Enter in the value input', async () => { const onSubmit = renderEditor({ isLoadingValues: true }); // `[[` types a literal `[`. diff --git a/packages/app/src/components/FilterPill/filterOperators.ts b/packages/app/src/components/FilterPill/filterOperators.ts index e766e247bd..62ba56ff5d 100644 --- a/packages/app/src/components/FilterPill/filterOperators.ts +++ b/packages/app/src/components/FilterPill/filterOperators.ts @@ -32,3 +32,12 @@ export function getFilterOperators( ): FilterOperatorOption[] { return language === 'promql' ? PROMQL_FILTER_OPERATORS : SQL_FILTER_OPERATORS; } + +/** How a pill shows `operator`: its label, and whether it excludes values. */ +export function getFilterOperatorDisplay( + language: FilterLanguage, + operator: string, +): { label: string; isExcluded: boolean } { + const option = getFilterOperators(language).find(op => op.value === operator); + return { label: option?.label ?? operator, isExcluded: !!option?.negated }; +} diff --git a/packages/app/src/hooks/__tests__/useDashboardFilters.test.tsx b/packages/app/src/hooks/__tests__/useDashboardFilters.test.tsx index b3f9fffc39..1ea51eb2fc 100644 --- a/packages/app/src/hooks/__tests__/useDashboardFilters.test.tsx +++ b/packages/app/src/hooks/__tests__/useDashboardFilters.test.tsx @@ -1,4 +1,7 @@ import { + ADHOC_FILTER_MAX_CONDITIONS, + AdhocDashboardFilter, + AdhocFilterCondition, DashboardFilter, DashboardFilterValue, Filter, @@ -988,6 +991,156 @@ describe('useDashboardFilters', () => { }); }); + describe('ad hoc filters', () => { + const adhocFilter: AdhocDashboardFilter = { + id: 'adhoc1', + type: 'ADHOC', + name: 'Conditions', + sourceType: 'sql', + sources: ['logs'], + isVariableEnabled: true, + variableName: 'conds', + }; + const condition: AdhocFilterCondition = { + key: 'ServiceName', + operator: '!=', + value: 'api', + }; + + it('reads conditions from an ad hoc entry with its variable name', () => { + mockState = [{ type: 'adhoc', name: 'conds', conditions: [condition] }]; + + const { result } = renderHook(() => useDashboardFilters([adhocFilter])); + + expect(result.current.adhocConditionsByFilterId.get('adhoc1')).toEqual([ + condition, + ]); + expect(result.current.ignoredVariableNames).toEqual([]); + }); + + it('writes and clears conditions without touching other entries', () => { + mockState = [{ type: 'sql', condition: "environment IN ('prod')" }]; + const filters = [mockFilters[0], adhocFilter]; + const { result } = renderHook(() => useDashboardFilters(filters)); + + act(() => { + result.current.setAdhocConditions('adhoc1', [condition]); + }); + expect(mockState).toEqual([ + { type: 'sql', condition: "environment IN ('prod')" }, + { type: 'adhoc', name: 'conds', conditions: [condition] }, + ]); + + act(() => { + result.current.setAdhocConditions('adhoc1', []); + }); + expect(mockState).toEqual([ + { type: 'sql', condition: "environment IN ('prod')" }, + ]); + }); + + it('drops repeated conditions on read and write', () => { + mockState = [ + { type: 'adhoc', name: 'conds', conditions: [condition, condition] }, + ]; + const { result } = renderHook(() => useDashboardFilters([adhocFilter])); + + expect(result.current.adhocConditionsByFilterId.get('adhoc1')).toEqual([ + condition, + ]); + + act(() => { + result.current.setAdhocConditions('adhoc1', [condition, condition]); + }); + expect(mockState).toEqual([ + { type: 'adhoc', name: 'conds', conditions: [condition] }, + ]); + }); + + it('ignores a write of more conditions than the URL entry can hold', () => { + mockState = [{ type: 'adhoc', name: 'conds', conditions: [condition] }]; + const { result } = renderHook(() => useDashboardFilters([adhocFilter])); + const conditionsUpTo = (count: number) => + Array.from({ length: count }, (_, i) => ({ + ...condition, + value: `${i}`, + })); + + act(() => { + result.current.setAdhocConditions( + 'adhoc1', + conditionsUpTo(ADHOC_FILTER_MAX_CONDITIONS + 1), + ); + }); + expect(mockState).toEqual([ + { type: 'adhoc', name: 'conds', conditions: [condition] }, + ]); + + act(() => { + result.current.setAdhocConditions( + 'adhoc1', + conditionsUpTo(ADHOC_FILTER_MAX_CONDITIONS), + ); + }); + expect(mockState).toEqual([ + { + type: 'adhoc', + name: 'conds', + conditions: conditionsUpTo(ADHOC_FILTER_MAX_CONDITIONS), + }, + ]); + }); + + it('ignores a write of a condition too long for the URL entry', () => { + mockState = [{ type: 'adhoc', name: 'conds', conditions: [condition] }]; + const { result } = renderHook(() => useDashboardFilters([adhocFilter])); + + act(() => { + result.current.setAdhocConditions('adhoc1', [ + condition, + { ...condition, value: 'x'.repeat(10001) }, + ]); + }); + expect(mockState).toEqual([ + { type: 'adhoc', name: 'conds', conditions: [condition] }, + ]); + }); + + it('keeps conditions across a write to another filter', () => { + mockState = [{ type: 'adhoc', name: 'conds', conditions: [condition] }]; + const filters = [mockFilters[0], adhocFilter]; + const { result } = renderHook(() => useDashboardFilters(filters)); + + act(() => { + result.current.setFilterValue('filter1', ['prod']); + }); + + expect(mockState).toEqual([ + { type: 'sql', condition: "environment IN ('prod')" }, + { type: 'adhoc', name: 'conds', conditions: [condition] }, + ]); + }); + + it('exposes no variable, selection, or broadcast query', () => { + mockState = [{ type: 'adhoc', name: 'conds', conditions: [condition] }]; + + const { result } = renderHook(() => useDashboardFilters([adhocFilter])); + + expect(result.current.variables).toEqual([]); + expect(result.current.selectionByFilterId.size).toBe(0); + expect(result.current.broadcastedFilters).toEqual([]); + }); + + it('lists an ad hoc entry naming no declared filter as ignored', () => { + mockState = [{ type: 'adhoc', name: 'renamed', conditions: [condition] }]; + + const { result } = renderHook(() => useDashboardFilters([adhocFilter])); + + expect(result.current.adhocConditionsByFilterId.size).toBe(0); + expect(result.current.ignoredVariableNames).toEqual(['renamed']); + }); + }); + describe('unsatisfiedRequiredFilters', () => { const required = ( overrides: Partial = {}, diff --git a/packages/app/src/hooks/__tests__/usePresetDashboardFilters.test.tsx b/packages/app/src/hooks/__tests__/usePresetDashboardFilters.test.tsx index 2ccd430ffd..f3770007cc 100644 --- a/packages/app/src/hooks/__tests__/usePresetDashboardFilters.test.tsx +++ b/packages/app/src/hooks/__tests__/usePresetDashboardFilters.test.tsx @@ -111,6 +111,8 @@ describe('usePresetDashboardFilters', () => { jest.mocked(useDashboardFilters).mockReturnValue({ selectionByFilterId: mockSelectionByFilterId, setFilterValue: mockSetFilterValue, + adhocConditionsByFilterId: new Map(), + setAdhocConditions: jest.fn(), broadcastedFilters: mockFilterQueries, setFilterValueEntries: jest.fn(), filterValueEntries: null, diff --git a/packages/app/src/hooks/useDashboardFilters.tsx b/packages/app/src/hooks/useDashboardFilters.tsx index e103ab81e3..547a7de988 100644 --- a/packages/app/src/hooks/useDashboardFilters.tsx +++ b/packages/app/src/hooks/useDashboardFilters.tsx @@ -1,4 +1,5 @@ import { useCallback, useMemo } from 'react'; +import { partition, uniqBy } from 'lodash'; import { useQueryState } from 'nuqs'; import { FilterSelection, @@ -8,6 +9,7 @@ import { ParsedDashboardFilterValues, resolveFilterSelection, serializeDashboardFilterValues, + VariableEntryValue, } from '@hyperdx/common-utils/dist/dashboardFilterValues'; import { doesFilterApplyToSource, @@ -15,10 +17,14 @@ import { filtersToQuery, getDashboardVariableFilters, getFilterExpression, + getFilterVariableName, + isAdhocFilter, isFilterBroadcastEnabled, isQueryExpressionFilter, } from '@hyperdx/common-utils/dist/filters'; import { + AdhocFilterCondition, + AdhocFilterValueSchema, ChartVariable, DashboardFilter, Filter, @@ -39,6 +45,10 @@ const hasSelection = (selection: FilterSelection): boolean => const isRepresentableAsVariable = (selection: FilterSelection): boolean => selection.excluded.size === 0 && selection.range == null; +/** Conditions are identified by content (e.g. pill keys), so drop repeats. */ +const uniqConditions = (conditions: AdhocFilterCondition[]) => + uniqBy(conditions, condition => JSON.stringify(condition)); + /** * Rebuild the whole state array from the selections every declared filter * currently resolves to. Anything that belongs to no declared filter is @@ -49,11 +59,12 @@ const rebuildEntries = ( parsed: ParsedDashboardFilterValues, ) => { const byExpression: FilterState = {}; - const byVariable = new Map(); + const byVariable = new Map(); const declaredExpressions = new Set(); const declaredVariableNames = new Set(); for (const filter of filters) { + if (isAdhocFilter(filter)) continue; const expression = getFilterExpression(filter); if (expression) declaredExpressions.add(expression); @@ -67,7 +78,10 @@ const rebuildEntries = ( if (!isRepresentableAsVariable(selection) && expression) { byExpression[expression] = selection; } else if (selection.included.size > 0) { - byVariable.set(key.name, Array.from(selection.included).map(String)); + byVariable.set(key.name, { + type: 'variable', + values: Array.from(selection.included).map(String), + }); } } @@ -83,9 +97,10 @@ const rebuildEntries = ( } } - // Pass through any entries that don't correspond to a declared filter. - for (const [name, values] of parsed.byVariable) { - if (!declaredVariableNames.has(name)) byVariable.set(name, values); + // Pass through any entries that don't correspond to a declared filter. Ad hoc + // filters are skipped above, so their conditions pass through here too. + for (const [name, value] of parsed.byVariable) { + if (!declaredVariableNames.has(name)) byVariable.set(name, value); } for (const [expression, selection] of Object.entries(parsed.byExpression)) { if (!declaredExpressions.has(expression)) { @@ -120,7 +135,7 @@ const useDashboardFilters = (filters: DashboardFilter[]) => { const key = filterSelectionKey(target); if (key.kind === 'variable') { // Set even when empty to avoid falling back to a legacy expression-keyed value - parsed.byVariable.set(key.name, values); + parsed.byVariable.set(key.name, { type: 'variable', values }); } else if (values.length === 0) { delete parsed.byExpression[key.expression]; } else { @@ -136,28 +151,62 @@ const useDashboardFilters = (filters: DashboardFilter[]) => { [setFilterValueEntries, filters], ); + const setAdhocConditions = useCallback( + (filterId: string, conditions: AdhocFilterCondition[]) => { + setFilterValueEntries(prev => { + const target = filters.find(f => f.id === filterId); + const name = target && getFilterVariableName(target); + // The URL parser drops an invalid entry whole, losing every condition + const unique = uniqConditions(conditions); + const isStorable = + AdhocFilterValueSchema.shape.conditions.safeParse(unique).success; + if (!name || !isStorable) return prev; + + const parsed = parseDashboardFilterValues(prev ?? []); + parsed.byVariable.set(name, { type: 'adhoc', conditions: unique }); + return rebuildEntries(filters, parsed); + }); + }, + [setFilterValueEntries, filters], + ); + const { selectionByFilterId, + adhocConditionsByFilterId, ignoredExpressions, ignoredVariableNames, variables, unsatisfiedRequiredFilters, } = useMemo<{ selectionByFilterId: ReadonlyMap; + adhocConditionsByFilterId: ReadonlyMap; ignoredExpressions: string[]; ignoredVariableNames: string[]; variables: ChartVariable[]; unsatisfiedRequiredFilters: DashboardFilter[]; }>(() => { const parsed = parseDashboardFilterValues(filterValueEntries ?? []); + // Ad hoc filters aren't exposed as variables yet + const [adhocFilters, nonAdhocFilters] = partition(filters, isAdhocFilter); // Keyed by filter ID: two definitions may share one expression const selectionByFilterId = new Map(); - for (const filter of filters) { + for (const filter of nonAdhocFilters) { const selection = resolveFilterSelection(filter, parsed); if (selection) selectionByFilterId.set(filter.id, selection); } + const adhocConditionsByFilterId = new Map(); + for (const filter of adhocFilters) { + const value = parsed.byVariable.get(getFilterVariableName(filter) ?? ''); + if (value?.type === 'adhoc') { + adhocConditionsByFilterId.set( + filter.id, + uniqConditions(value.conditions), + ); + } + } + // Find state that doesn't correspond to any declared filter, so the caller can surface a warning. const knownExpressions = new Set( filters.filter(isQueryExpressionFilter).map(f => f.expression), @@ -166,8 +215,11 @@ const useDashboardFilters = (filters: DashboardFilter[]) => { expression => !knownExpressions.has(expression), ); - const variableFilters = getDashboardVariableFilters(filters); - const declaredVariableNames = new Set(variableFilters.map(v => v.name)); + const variableFilters = getDashboardVariableFilters(nonAdhocFilters); + const declaredVariableNames = new Set([ + ...variableFilters.map(v => v.name), + ...adhocFilters.flatMap(filter => getFilterVariableName(filter) ?? []), + ]); const ignoredVariableNames = Array.from(parsed.byVariable.keys()).filter( name => !declaredVariableNames.has(name), ); @@ -191,6 +243,7 @@ const useDashboardFilters = (filters: DashboardFilter[]) => { return { selectionByFilterId, + adhocConditionsByFilterId, variables, ignoredExpressions, ignoredVariableNames, @@ -260,6 +313,10 @@ const useDashboardFilters = (filters: DashboardFilter[]) => { broadcastedFilters, /** Set the selected values for a filter, by its ID */ setFilterValue, + /** Each ad hoc filter's current conditions, keyed by `filter.id`. */ + adhocConditionsByFilterId, + /** Replace the conditions of an ad hoc filter, by its ID */ + setAdhocConditions, /** Set the raw filter value state */ setFilterValueEntries, /** The raw persisted entries, as they appear in the URL param. */ diff --git a/packages/app/src/hooks/useMetadata.tsx b/packages/app/src/hooks/useMetadata.tsx index fd265c1af0..c55ac4cc8c 100644 --- a/packages/app/src/hooks/useMetadata.tsx +++ b/packages/app/src/hooks/useMetadata.tsx @@ -39,6 +39,12 @@ import { toArray } from '@/utils'; export type Facet = { key: string; value: string[] }; +/** + * The most values to load when one key's values are looked up in full, e.g. + * the search sidebar's "Load more". + */ +export const SINGLE_KEY_VALUES_LIMIT = 10000; + // Hook to get metadata with proper settings applied export function useMetadataWithSettings() { const [metadata, setMetadata] = useState(getMetadata()); diff --git a/packages/app/src/hooks/usePromqlLabelFilterValues.tsx b/packages/app/src/hooks/usePromqlLabelFilterValues.tsx index ed66bd5384..b1cfdeeed2 100644 --- a/packages/app/src/hooks/usePromqlLabelFilterValues.tsx +++ b/packages/app/src/hooks/usePromqlLabelFilterValues.tsx @@ -14,7 +14,10 @@ import { UseQueryResult, } from '@tanstack/react-query'; -import { prometheusApi } from '@/api'; +import { + fetchPromqlLabelValues, + toPromqlSeconds, +} from '@/hooks/usePromqlMetadata'; import { useMetadataQueryAttribution } from '@/queryAttribution'; import { useSources } from '@/source'; import { mapKeyBy } from '@/utils'; @@ -42,9 +45,7 @@ export function usePromqlLabelFilterValues({ const { data: sources, isLoading: isLoadingSources } = useSources(); const sourcesById = useMemo(() => mapKeyBy(sources ?? [], 'id'), [sources]); - // Round the date range, since the API accepts whole seconds - const startSec = Math.floor(dateRange[0].getTime() / 1000); - const endSec = Math.ceil(dateRange[1].getTime() / 1000); + const { start: startSec, end: endSec } = toPromqlSeconds(dateRange); // A filter's selector may reference the dashboard's variables. Expand them // here so react-query keys on the resolved selector rather than the template. @@ -113,20 +114,15 @@ export function usePromqlLabelFilterValues({ return cached[0]?.data; }, staleTime: 1000 * 60 * 5, - queryFn: async (): Promise => { - const resp = await prometheusApi.labelValues({ + queryFn: () => + fetchPromqlLabelValues({ label: call.label, sourceId: call.sourceId, start: startSec, end: endSec, match: call.match, attribution, - }); - if (resp.status === 'error') { - throw new Error(resp.error ?? 'Label values query failed'); - } - return resp.data ?? []; - }, + }), }; }), }); diff --git a/packages/app/src/hooks/usePromqlMetadata.ts b/packages/app/src/hooks/usePromqlMetadata.ts index b8102ac745..045054ace0 100644 --- a/packages/app/src/hooks/usePromqlMetadata.ts +++ b/packages/app/src/hooks/usePromqlMetadata.ts @@ -1,20 +1,53 @@ +import { QueryAttribution } from '@hyperdx/common-utils/dist/clickhouse'; import { useQuery } from '@tanstack/react-query'; import { prometheusApi } from '@/api'; import { useMetadataQueryAttribution } from '@/queryAttribution'; +type PromqlLabelLookup = { + sourceId: string; + start?: number; + end?: number; + attribution?: QueryAttribution; +}; + +/** The label lookup APIs accept whole seconds. */ +export const toPromqlSeconds = (dateRange: [Date, Date]) => ({ + start: Math.floor(dateRange[0].getTime() / 1000), + end: Math.ceil(dateRange[1].getTime() / 1000), +}); + +export async function fetchPromqlLabelNames( + params: PromqlLabelLookup, +): Promise { + const resp = await prometheusApi.labels(params); + if (resp.status === 'error') { + throw new Error(resp.error ?? 'Label names query failed'); + } + return resp.data ?? []; +} + +export async function fetchPromqlLabelValues( + params: PromqlLabelLookup & { label: string; match?: string }, +): Promise { + const resp = await prometheusApi.labelValues(params); + if (resp.status === 'error') { + throw new Error(resp.error ?? 'Label values query failed'); + } + return resp.data ?? []; +} + export function usePromqlMetricNames(sourceId: string | undefined) { const attribution = useMetadataQueryAttribution(); return useQuery({ queryKey: ['promql-metric-names', sourceId], queryFn: async () => { if (!sourceId) return []; - const resp = await prometheusApi.labelValues({ + return fetchPromqlLabelValues({ label: '__name__', sourceId, attribution, }); - return resp.data ?? []; }, enabled: !!sourceId, staleTime: 60_000, @@ -28,8 +61,7 @@ export function usePromqlLabelNames(sourceId: string | undefined) { queryKey: ['promql-label-names', sourceId], queryFn: async () => { if (!sourceId) return []; - const resp = await prometheusApi.labels({ sourceId, attribution }); - return resp.data ?? []; + return fetchPromqlLabelNames({ sourceId, attribution }); }, enabled: !!sourceId, staleTime: 60_000, 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 e64ed257cc..4c2bfa9172 100644 --- a/packages/app/tests/e2e/features/dashboard-adhoc-filters.spec.ts +++ b/packages/app/tests/e2e/features/dashboard-adhoc-filters.spec.ts @@ -1,9 +1,11 @@ /** * An `ADHOC` dashboard filter's conditions (`key operator value`) are picked - * from the keys of its sources. For now the filters modal only lists, creates, - * and edits them; they are not rendered or applied on the dashboard. + * from the keys of its sources. The filters modal creates and edits them, and + * the dashboard renders their conditions and keeps them in the URL; they are not + * applied to tiles yet. */ import { DashboardPage } from '../page-objects/DashboardPage'; +import { SERVICES } from '../seed-clickhouse'; import { expect, test } from '../utils/base-test'; import { DEFAULT_LOGS_SOURCE_NAME, @@ -11,6 +13,7 @@ import { DEFAULT_TRACES_SOURCE_NAME, PROMQL_SOURCE_NAME, } from '../utils/constants'; +import { expectFiltersParam, filtersParam } from '../utils/filters-param'; test.describe( 'Ad hoc dashboard filters', @@ -23,6 +26,16 @@ test.describe( await dashboardPage.goto(); }); + /** Create a dashboard holding one ad hoc filter, and close the modal. */ + const createDashboardWithAdhocFilter = async ( + ...args: Parameters + ) => { + await dashboardPage.createNewDashboard(); + await dashboardPage.openEditFiltersModal(); + await dashboardPage.addAdhocFilterToDashboard(...args); + await dashboardPage.closeFiltersModal(); + }; + test('authors a SQL filter over several sources', async ({ page }) => { await test.step('Open the add-filter form and pick the ad hoc type', async () => { await dashboardPage.createNewDashboard(); @@ -257,6 +270,200 @@ test.describe( ).toBeVisible(); }); + test('picks conditions from the keys and values of one SQL source', async ({ + page, + }) => { + const filterName = 'Conditions'; + const pills = dashboardPage.getAdhocConditionPills(filterName); + + await test.step('Create the filter', async () => { + await createDashboardWithAdhocFilter( + filterName, + [DEFAULT_LOGS_SOURCE_NAME], + { variableName: 'conds' }, + ); + await expect(dashboardPage.getAdhocFilter(filterName)).toBeVisible(); + }); + + await test.step('Keys are suggested, values only once a key is picked', async () => { + const editor = dashboardPage.getAdhocConditionEditor(filterName); + const valueInput = dashboardPage.getAdhocConditionInput( + filterName, + 'value', + ); + const serviceNameOption = dashboardPage.getAdhocConditionOption( + filterName, + 'ServiceName', + ); + const accountingOption = dashboardPage.getAdhocConditionOption( + filterName, + 'accounting', + ); + + await dashboardPage.openAddAdhocCondition(filterName); + await valueInput.click(); + await expect(editor.getByRole('option')).toHaveCount(0); + + await dashboardPage.getAdhocConditionInput(filterName, 'key').click(); + await expect(serviceNameOption).toBeVisible({ timeout: 30000 }); + await serviceNameOption.click(); + + await valueInput.click(); + await expect(accountingOption).toBeVisible({ timeout: 30000 }); + await accountingOption.click(); + await valueInput.press('Enter'); + await expect(editor).toBeHidden(); + }); + + await test.step('The condition is shown and kept in the URL', async () => { + await expect(pills).toHaveCount(1); + await expect(pills).toHaveText('ServiceName = accounting'); + await expectFiltersParam(page, [ + { + type: 'adhoc', + name: 'conds', + conditions: [ + { key: 'ServiceName', operator: '=', value: 'accounting' }, + ], + }, + ]); + }); + + await test.step('Editing the pill changes its operator', async () => { + await dashboardPage.editAdhocCondition( + filterName, + 'ServiceName = accounting', + { operatorLabel: '!=' }, + ); + await expect(pills).toHaveText('ServiceName != accounting'); + await expectFiltersParam(page, [ + { + type: 'adhoc', + name: 'conds', + conditions: [ + { key: 'ServiceName', operator: '!=', value: 'accounting' }, + ], + }, + ]); + }); + + await test.step('The condition survives a reload', async () => { + await page.reload(); + await dashboardPage.waitForLoaded(); + await expect(pills).toHaveText('ServiceName != accounting'); + }); + + await test.step('Removing the pill clears it from the URL', async () => { + await pills.getByRole('button', { name: 'Remove filter' }).click(); + await expect(pills).toHaveCount(0); + await expect.poll(() => filtersParam(page) ?? []).toEqual([]); + }); + }); + + test('offers deduplicated keys and values across several SQL sources', async ({ + page, + }) => { + const filterName = 'Both'; + const pills = dashboardPage.getAdhocConditionPills(filterName); + + await test.step('Create a filter over logs and traces', async () => { + await createDashboardWithAdhocFilter( + filterName, + [DEFAULT_LOGS_SOURCE_NAME, DEFAULT_TRACES_SOURCE_NAME], + { variableName: 'both' }, + ); + }); + + const keyInput = dashboardPage.getAdhocConditionInput(filterName, 'key'); + + await test.step('A key both sources have is listed once', async () => { + await dashboardPage.openAddAdhocCondition(filterName); + // Typing narrows the list, which renders at most 200 suggestions. + await keyInput.fill('ServiceName'); + await expect( + dashboardPage.getAdhocConditionOption(filterName, 'ServiceName'), + ).toHaveCount(1, { timeout: 30000 }); + }); + + await test.step('A key only one source has is offered too', async () => { + await keyInput.fill('SpanName'); + await expect( + dashboardPage.getAdhocConditionOption(filterName, 'SpanName'), + ).toHaveCount(1); + }); + + await test.step("The key's values are listed once each", async () => { + await keyInput.fill('ServiceName'); + await dashboardPage.getAdhocConditionInput(filterName, 'value').click(); + await expect( + dashboardPage.getAdhocConditionOption(filterName, 'accounting'), + ).toHaveCount(1, { timeout: 30000 }); + const options = await dashboardPage.getOpenFilterDropdownOptions(); + expect(new Set(options).size).toBe(options.length); + expect(options).toEqual(expect.arrayContaining([...SERVICES])); + await dashboardPage.fillAdhocConditionEditor(filterName, { + value: 'accounting', + }); + }); + + await test.step('A key only one source has gets its values', async () => { + await dashboardPage.openAddAdhocCondition(filterName); + await keyInput.fill('SeverityText'); + await dashboardPage.getAdhocConditionInput(filterName, 'value').click(); + await expect( + dashboardPage.getAdhocConditionOption(filterName, 'error'), + ).toBeVisible({ timeout: 30000 }); + await dashboardPage.fillAdhocConditionEditor(filterName, { + value: 'error', + }); + }); + + await test.step('Both conditions survive a reload', async () => { + await expect(pills).toHaveText([ + 'ServiceName = accounting', + 'SeverityText = error', + ]); + await page.reload(); + await dashboardPage.waitForLoaded(); + await expect(pills).toHaveText([ + 'ServiceName = accounting', + 'SeverityText = error', + ]); + }); + }); + + test('picks conditions from the labels of a PromQL source', async () => { + const filterName = 'Labels'; + + await createDashboardWithAdhocFilter(filterName, [PROMQL_SOURCE_NAME], { + sourceType: 'Prometheus', + }); + + await dashboardPage.openAddAdhocCondition(filterName); + await dashboardPage.getAdhocConditionInput(filterName, 'key').click(); + await expect( + dashboardPage.getAdhocConditionOption(filterName, 'service'), + ).toBeVisible({ timeout: 30000 }); + await expect( + dashboardPage.getAdhocConditionOption(filterName, '__name__'), + ).toHaveCount(0); + + await dashboardPage + .getAdhocConditionInput(filterName, 'key') + .fill('service'); + await dashboardPage.getAdhocConditionInput(filterName, 'value').click(); + await expect( + dashboardPage.getAdhocConditionOption(filterName, 'accounting'), + ).toBeVisible({ timeout: 30000 }); + await dashboardPage.fillAdhocConditionEditor(filterName, { + operatorLabel: '=~', + value: 'acc.*', + }); + await expect(dashboardPage.getAdhocConditionPills(filterName)).toHaveText( + 'service =~ acc.*', + ); + }); + test("lives alongside the dashboard's other filters", async ({ page }) => { await test.step('Create an ad hoc and a static filter', async () => { await dashboardPage.createNewDashboard(); @@ -285,11 +492,12 @@ test.describe( await page.getByRole('button', { name: 'Cancel' }).click(); }); - await test.step('Only the other filter is rendered on the dashboard', async () => { + await test.step('Both filters are rendered on the dashboard', async () => { await dashboardPage.closeFiltersModal(); await expect( dashboardPage.getFilterSelectByName('Environment'), ).toBeVisible(); + await expect(dashboardPage.getAdhocFilter('Conditions')).toBeVisible(); await expect( dashboardPage.getFilterSelectByName('Conditions'), ).toHaveCount(0); diff --git a/packages/app/tests/e2e/page-objects/DashboardPage.ts b/packages/app/tests/e2e/page-objects/DashboardPage.ts index 974646c7d1..fcfcdaf9f3 100644 --- a/packages/app/tests/e2e/page-objects/DashboardPage.ts +++ b/packages/app/tests/e2e/page-objects/DashboardPage.ts @@ -1569,11 +1569,18 @@ export class DashboardPage { async addAdhocFilterToDashboard( name: string, sourceNames: string[], - options?: { variableName?: string }, + options?: { + variableName?: string; + sourceType?: 'ClickHouse' | 'Prometheus'; + }, ) { await this.addFiltersButton.click(); await this.selectFilterType('Ad hoc keys and values'); await this.getFilterNameInput().fill(name); + // Before the sources, since switching the type clears them. + if (options?.sourceType) { + await this.selectAdhocFilterSourceType(options.sourceType); + } await this.selectSourcesInMultiSelect( this.getAdhocFilterSourcesInput(), sourceNames, @@ -1588,6 +1595,85 @@ export class DashboardPage { }); } + /** The filter bar control of an ad hoc filter. */ + getAdhocFilter(filterName: string): Locator { + return this.page.getByTestId(`adhoc-filter-${filterName}`); + } + + /** The condition pills of an ad hoc filter in the filter bar. */ + getAdhocConditionPills(filterName: string): Locator { + return this.getAdhocFilter(filterName).getByTestId('adhoc-condition-pill'); + } + + /** The open condition editor of an ad hoc filter. */ + getAdhocConditionEditor(filterName: string): Locator { + return this.page.getByTestId(`adhoc-condition-editor-${filterName}`); + } + + /** The key, operator, or value input of an ad hoc filter's open editor. */ + getAdhocConditionInput( + filterName: string, + input: 'key' | 'operator' | 'value', + ): Locator { + return this.page.getByTestId( + `adhoc-condition-editor-${filterName}-${input}`, + ); + } + + /** A suggestion, matched exactly, in an ad hoc filter's open editor. */ + getAdhocConditionOption(filterName: string, name: string): Locator { + return this.getAdhocConditionEditor(filterName).getByRole('option', { + name, + exact: true, + }); + } + + /** Open the editor that adds a condition to an ad hoc filter. */ + async openAddAdhocCondition(filterName: string) { + await this.page.getByTestId(`adhoc-filter-add-${filterName}`).click(); + await this.getAdhocConditionEditor(filterName).waitFor({ + state: 'visible', + }); + } + + /** Click a condition's pill and change any part of it in the editor. */ + async editAdhocCondition( + filterName: string, + pillText: string, + changes: { key?: string; operatorLabel?: string; value?: string }, + ) { + await this.getAdhocConditionPills(filterName) + .filter({ hasText: pillText }) + .click(); + await this.fillAdhocConditionEditor(filterName, changes); + } + + /** Fill the open editor's inputs, then apply it. */ + async fillAdhocConditionEditor( + filterName: string, + { + key, + operatorLabel, + value, + }: { key?: string; operatorLabel?: string; value?: string }, + ) { + if (key !== undefined) { + await this.getAdhocConditionInput(filterName, 'key').fill(key); + } + if (operatorLabel !== undefined) { + await this.getAdhocConditionInput(filterName, 'operator').click(); + await this.getAdhocConditionOption(filterName, operatorLabel).click(); + } + const valueInput = this.getAdhocConditionInput(filterName, 'value'); + if (value !== undefined) { + await valueInput.fill(value); + } + // Enter applies. Escape would close the whole popover, and the value + // suggestions can cover the Apply button. + await valueInput.press('Enter'); + await this.getAdhocConditionEditor(filterName).waitFor({ state: 'hidden' }); + } + /** * The options offered by the dashboard filter dropdown that is currently * open, in render order. Scoped to visible nodes: a just-closed dropdown's diff --git a/packages/app/tests/e2e/utils/filters-param.ts b/packages/app/tests/e2e/utils/filters-param.ts index 17a26c7851..a131150672 100644 --- a/packages/app/tests/e2e/utils/filters-param.ts +++ b/packages/app/tests/e2e/utils/filters-param.ts @@ -1,13 +1,19 @@ /** * The `filters=` URL param carries dashboard filter selections in one of two - * shapes: legacy, keyed by SQL expression, or variable-keyed by variable name. - * These helpers are the one definition of that contract for the E2E suite. + * shapes: legacy, keyed by SQL expression, or variable-keyed by variable name + * (selected values, or an ad hoc filter's conditions). These helpers are the + * one definition of that contract for the E2E suite. */ import { expect, type Page } from '@playwright/test'; export type FilterEntry = | { type: 'sql'; condition: string } - | { type: 'variable'; name: string; values: string[] }; + | { type: 'variable'; name: string; values: string[] } + | { + type: 'adhoc'; + name: string; + conditions: { key: string; operator: string; value: string }[]; + }; /** The raw, still-encoded `filters=` param. `null` when the param is absent. */ export const rawFiltersParam = (page: Page): string | null => diff --git a/packages/common-utils/src/__tests__/dashboardFilterValues.test.ts b/packages/common-utils/src/__tests__/dashboardFilterValues.test.ts index 69652d0bea..669c190fa4 100644 --- a/packages/common-utils/src/__tests__/dashboardFilterValues.test.ts +++ b/packages/common-utils/src/__tests__/dashboardFilterValues.test.ts @@ -7,9 +7,11 @@ import { parseDashboardFilterValues, resolveFilterSelection, serializeDashboardFilterValues, + VariableEntryValue, } from '@/dashboardFilterValues'; import { FilterState, filtersToQuery } from '@/filters'; import type { + AdhocFilterCondition, ChartConfigWithOptDateRange, DashboardFilter, DashboardFilterValue, @@ -56,6 +58,17 @@ const promqlFilter = ( ...overrides, }); +const condition: AdhocFilterCondition = { + key: 'ServiceName', + operator: '=', + value: 'api', +}; + +const variableValue = (...values: string[]): VariableEntryValue => ({ + type: 'variable', + values, +}); + const included = (...values: (string | boolean)[]) => ({ included: new Set(values), excluded: new Set(), @@ -79,7 +92,9 @@ describe('dashboardFilterValues', () => { ]); expect(parsed.byExpression).toEqual({ Env: included('prod') }); - expect(Array.from(parsed.byVariable)).toEqual([['svc', ['accounting']]]); + expect(Array.from(parsed.byVariable)).toEqual([ + ['svc', variableValue('accounting')], + ]); expect(parsed.passthrough).toEqual([]); }); @@ -91,8 +106,8 @@ describe('dashboardFilterValues', () => { expect(parsed.byExpression).toEqual({}); expect(Array.from(parsed.byVariable)).toEqual([ - ['svc', ['a', 'b']], - ['env', []], + ['svc', variableValue('a', 'b')], + ['env', variableValue()], ]); }); @@ -114,7 +129,9 @@ describe('dashboardFilterValues', () => { { type: 'variable', name: 'svc', values: ['second'] }, ]); - expect(Array.from(parsed.byVariable)).toEqual([['svc', ['first']]]); + expect(Array.from(parsed.byVariable)).toEqual([ + ['svc', variableValue('first')], + ]); }); it('routes non-sql entries to passthrough', () => { @@ -151,9 +168,14 @@ describe('dashboardFilterValues', () => { { type: 'variable', name: 'env', values: ['prod'] }, ]); + expect(Array.from(parsed.byVariable)).toEqual([ + ['adhoc', { type: 'adhoc', conditions: adhoc.conditions }], + ['env', variableValue('prod')], + ]); + expect(parsed.passthrough).toEqual([]); expect(serializeDashboardFilterValues(parsed)).toEqual([ - { type: 'variable', name: 'env', values: ['prod'] }, adhoc, + { type: 'variable', name: 'env', values: ['prod'] }, ]); }); @@ -189,8 +211,8 @@ describe('dashboardFilterValues', () => { serializeDashboardFilterValues({ byExpression: { Env: included('prod'), Region: included('us') }, byVariable: new Map([ - ['svc', ['accounting']], - ['team', ['platform']], + ['svc', variableValue('accounting')], + ['team', variableValue('platform')], ]), passthrough: [passthrough], }), @@ -210,9 +232,10 @@ describe('dashboardFilterValues', () => { Env: included(), Region: included('us'), }, - byVariable: new Map([ - ['svc', []], - ['team', ['platform']], + byVariable: new Map([ + ['svc', variableValue()], + ['team', variableValue('platform')], + ['conds', { type: 'adhoc', conditions: [] }], ]), }), ).toEqual([ @@ -264,7 +287,7 @@ describe('dashboardFilterValues', () => { 'preserves %s exactly on the variable path', (_label, value) => { const entries = serializeDashboardFilterValues({ - byVariable: new Map([['svc', [value]]]), + byVariable: new Map([['svc', variableValue(value)]]), }); expect(entries).toEqual([ { type: 'variable', name: 'svc', values: [value] }, @@ -275,7 +298,7 @@ describe('dashboardFilterValues', () => { const parsed = parseDashboardFilterValues( JSON.parse(JSON.stringify(entries)), ); - expect(parsed.byVariable.get('svc')).toEqual([value]); + expect(parsed.byVariable.get('svc')).toEqual(variableValue(value)); expect(parsed.byExpression).toEqual({}); expect(parsed.passthrough).toEqual([]); }, @@ -319,7 +342,7 @@ describe('dashboardFilterValues', () => { ]); expect( serializeDashboardFilterValues({ - byVariable: new Map([['svc', ['true']]]), + byVariable: new Map([['svc', variableValue('true')]]), }), ).toEqual([{ type: 'variable', name: 'svc', values: ['true'] }]); }); @@ -430,6 +453,14 @@ describe('dashboardFilterValues', () => { ); }); + it('does not read an ad hoc entry as a selection', () => { + const parsed = parseDashboardFilterValues([ + { type: 'adhoc', name: 'svc', conditions: [condition] }, + ]); + + expect(resolveFilterSelection(variableFilter, parsed)).toBeUndefined(); + }); + it('falls back to the expression entry for back-compat', () => { const parsed = parseDashboardFilterValues([ { type: 'sql', condition: "ServiceName IN ('legacy')" }, diff --git a/packages/common-utils/src/__tests__/filters.test.ts b/packages/common-utils/src/__tests__/filters.test.ts index aff7b0326d..050834af73 100644 --- a/packages/common-utils/src/__tests__/filters.test.ts +++ b/packages/common-utils/src/__tests__/filters.test.ts @@ -28,6 +28,7 @@ import { validateVariableName, } from '@/filters'; import type { + AdhocDashboardFilter, ChartVariable, DashboardFilter, Filter, @@ -1611,14 +1612,30 @@ describe('filters', () => { ...overrides, }); - it('reports the expression and scope for a broadcasting filter', () => { + it('reports the scope for a broadcasting filter', () => { expect(getFilterBroadcastTarget(filter({}))).toEqual({ - expression: 'ServiceName', appliesToSourceIds: undefined, }); expect( getFilterBroadcastTarget(filter({ appliesToSourceIds: ['logs'] })), - ).toEqual({ expression: 'ServiceName', appliesToSourceIds: ['logs'] }); + ).toEqual({ appliesToSourceIds: ['logs'] }); + }); + + it('scopes an ad hoc filter to the sources it applies to', () => { + const adhoc: AdhocDashboardFilter = { + id: 'f4', + type: 'ADHOC', + name: 'Conditions', + sourceType: 'sql', + sources: ['logs', 'traces'], + isVariableEnabled: true, + }; + expect(getFilterBroadcastTarget(adhoc)).toEqual({ + appliesToSourceIds: ['logs', 'traces'], + }); + expect( + getFilterBroadcastTarget({ ...adhoc, isBroadcastEnabled: false }), + ).toBeUndefined(); }); it('returns undefined when broadcasting is off', () => { diff --git a/packages/common-utils/src/clickhouse/__tests__/index.test.ts b/packages/common-utils/src/clickhouse/__tests__/index.test.ts index 69e70a306d..50b67a3dbe 100644 --- a/packages/common-utils/src/clickhouse/__tests__/index.test.ts +++ b/packages/common-utils/src/clickhouse/__tests__/index.test.ts @@ -3,6 +3,7 @@ import { convertCHDataTypeToJSType, extractColumnReferencesFromKey, isAccessDeniedError, + isJSDataTypeScalar, isMissingColumnError, JSDataType, } from '@/clickhouse'; @@ -182,6 +183,28 @@ describe('convertCHDataTypeToJSType', () => { }); }); +describe('isJSDataTypeScalar', () => { + it.each([ + JSDataType.String, + JSDataType.Number, + JSDataType.Bool, + JSDataType.Date, + JSDataType.Dynamic, + null, + ])('treats %s as scalar', dataType => { + expect(isJSDataTypeScalar(dataType)).toBe(true); + }); + + it.each([ + JSDataType.Map, + JSDataType.JSON, + JSDataType.Array, + JSDataType.Tuple, + ])('treats %s as non-scalar', dataType => { + expect(isJSDataTypeScalar(dataType)).toBe(false); + }); +}); + // logQuery is protected on the base class; this subclass exposes it so the // tests can drive it without reaching through the client's public query path. class TestClickhouseClient extends ClickhouseClient { diff --git a/packages/common-utils/src/clickhouse/index.ts b/packages/common-utils/src/clickhouse/index.ts index 0c6dcde01a..2a9f3d2054 100644 --- a/packages/common-utils/src/clickhouse/index.ts +++ b/packages/common-utils/src/clickhouse/index.ts @@ -170,6 +170,21 @@ export const isJSDataTypeJSONStringifiable = ( ); }; +/** Types that hold structure rather than a filterable value. */ +const NON_SCALAR_JS_DATA_TYPES = [ + JSDataType.Map, + JSDataType.JSON, + JSDataType.Array, + JSDataType.Tuple, +]; + +/** + * Unlike !isJSDataTypeJSONStringifiable, Dynamic (JSON subfields) and + * unrecognized (null) types count as scalar. + */ +export const isJSDataTypeScalar = (dataType: JSDataType | null) => + dataType == null || !NON_SCALAR_JS_DATA_TYPES.includes(dataType); + export const extractInnerCHArrayJSType = ( dataType: string, ): JSDataType | null => { diff --git a/packages/common-utils/src/dashboardFilterValues.ts b/packages/common-utils/src/dashboardFilterValues.ts index f060de0ee3..33b5f6180f 100644 --- a/packages/common-utils/src/dashboardFilterValues.ts +++ b/packages/common-utils/src/dashboardFilterValues.ts @@ -5,6 +5,7 @@ import { getDashboardVariableFilters, getFilterExpression, getFilterVariableName, + isAdhocFilterValue, isFilterGlobalRequirement, isFilterRequired, isFilterVariableEnabled, @@ -12,6 +13,7 @@ import { } from '@/filters'; import { isMissingFiltersMacro } from '@/macros'; import { + AdhocFilterValue, ChartConfigWithOptDateRange, DashboardFilter, DashboardFilterValue, @@ -27,6 +29,7 @@ import { isFilterSingleSelect } from '@/variables'; * * - legacy, keyed by SQL expression: `{ type: 'sql', condition: "E IN ('a')" }` * - variable-keyed: `{ type: 'variable', name: 'svc', values: ['a'] }` + * `{ type: 'adhoc', name: 'conds', conditions: [...] }` * * Expression keying cannot represent two filters that share an `expression`, * and has nowhere to put a selection for a filter with no expression at all. @@ -39,11 +42,16 @@ import { isFilterSingleSelect } from '@/variables'; /** One filter's selection: what a `FilterState` holds per key. */ export type FilterSelection = FilterState[string]; +/** A variable-keyed entry, without the name it is keyed by. */ +export type VariableEntryValue = + | Omit + | Omit; + export type ParsedDashboardFilterValues = { /** Selections addressed by SQL expression, parsed via `parseQuery`. */ byExpression: FilterState; - /** Selections addressed by dashboard variable name. */ - byVariable: Map; + /** Selected values and ad hoc conditions addressed by dashboard variable name. */ + byVariable: Map; /** * Non-`sql` entries (`lucene`, `sql_ast`), carried verbatim so a write doesn't * destroy them. @@ -59,15 +67,16 @@ const isVariableEntry = ( export function parseDashboardFilterValues( entries: DashboardFilterValue[] | undefined, ): ParsedDashboardFilterValues { - const byVariable = new Map(); + const byVariable = new Map(); const passthrough: DashboardFilterValue[] = []; const sqlEntries: Filter[] = []; for (const entry of entries ?? []) { - if (isVariableEntry(entry)) { + if (isVariableEntry(entry) || isAdhocFilterValue(entry)) { // There shouldn't be duplicate names, but if there are the first wins. if (!byVariable.has(entry.name)) { - byVariable.set(entry.name, entry.values); + const { name, ...value } = entry; + byVariable.set(name, value); } continue; } @@ -99,7 +108,7 @@ export function parseDashboardFilterValues( */ export function serializeDashboardFilterValues(input: { byExpression?: FilterState; - byVariable?: ReadonlyMap; + byVariable?: ReadonlyMap; passthrough?: DashboardFilterValue[]; }): DashboardFilterValue[] { const entries: DashboardFilterValue[] = [ @@ -107,9 +116,13 @@ export function serializeDashboardFilterValues(input: { ...filtersToQuery(input.byExpression ?? {}, { stringifyKeys: false }), ]; - for (const [name, values] of input.byVariable ?? []) { - if (values.length === 0) continue; // Empty selections are omitted. - entries.push({ type: 'variable', name, values }); + for (const [name, value] of input.byVariable ?? []) { + // Empty selections are omitted. + const isEmpty = + value.type === 'adhoc' + ? value.conditions.length === 0 + : value.values.length === 0; + if (!isEmpty) entries.push({ name, ...value }); } entries.push(...(input.passthrough ?? [])); @@ -149,7 +162,7 @@ export function filterSelectionKey( export function resolveFilterSelection( filter: DashboardFilter, parsed: Pick & { - byVariable: ReadonlyMap; + byVariable: ReadonlyMap; }, ): FilterSelection | undefined { const selection = resolveUncappedFilterSelection(filter, parsed); @@ -165,15 +178,15 @@ export function resolveFilterSelection( function resolveUncappedFilterSelection( filter: DashboardFilter, parsed: Pick & { - byVariable: ReadonlyMap; + byVariable: ReadonlyMap; }, ): FilterSelection | undefined { const key = filterSelectionKey(filter); if (key.kind === 'variable') { const values = parsed.byVariable.get(key.name); - if (values) { + if (values?.type === 'variable') { return { - included: new Set(values), + included: new Set(values.values), excluded: new Set(), }; } diff --git a/packages/common-utils/src/filters.ts b/packages/common-utils/src/filters.ts index 0ad3dc1ae6..9fa6b71c35 100644 --- a/packages/common-utils/src/filters.ts +++ b/packages/common-utils/src/filters.ts @@ -950,16 +950,16 @@ export function getFilterExpression( return isQueryExpressionFilter(filter) ? filter.expression : undefined; } -/** What the filter broadcasts, or undefined when it cannot broadcast. */ +/** Where the filter broadcasts, or undefined when it cannot broadcast. */ export function getFilterBroadcastTarget( filter: DashboardFilter, -): { expression: string; appliesToSourceIds?: string[] } | undefined { - if (!isQueryExpressionFilter(filter) || !isFilterBroadcastEnabled(filter)) - return undefined; - return { - expression: filter.expression, - appliesToSourceIds: filter.appliesToSourceIds, - }; +): { appliesToSourceIds?: string[] } | undefined { + if (!isFilterBroadcastEnabled(filter)) return undefined; + if (isAdhocFilter(filter)) { + return { appliesToSourceIds: getAdhocFilterAppliesToSourceIds(filter) }; + } + if (!isQueryExpressionFilter(filter)) return undefined; + return { appliesToSourceIds: filter.appliesToSourceIds }; } /** From 8b8ae5a090062d1f42101f69735adebb72bb280e Mon Sep 17 00:00:00 2001 From: Jordan Simonovski Date: Fri, 16 Oct 2026 10:37:30 +1100 Subject: [PATCH 13/16] fix(app): keep dashboard tile menu on screen with many groups (#3360) --- .changeset/tidy-tile-menu-groups.md | 5 + packages/app/src/DBDashboardPage.tsx | 179 +++--------------- .../dashboard/MoveToGroupSubmenu.tsx | 87 +++++++++ .../__tests__/MoveToGroupSubmenu.test.tsx | 154 +++++++++++++++ .../features/dashboard-tile-move-menu.spec.ts | 107 +++++++++++ .../tests/e2e/page-objects/DashboardPage.ts | 27 +++ 6 files changed, 404 insertions(+), 155 deletions(-) create mode 100644 .changeset/tidy-tile-menu-groups.md create mode 100644 packages/app/src/components/dashboard/MoveToGroupSubmenu.tsx create mode 100644 packages/app/src/components/dashboard/__tests__/MoveToGroupSubmenu.test.tsx create mode 100644 packages/app/tests/e2e/features/dashboard-tile-move-menu.spec.ts diff --git a/.changeset/tidy-tile-menu-groups.md b/.changeset/tidy-tile-menu-groups.md new file mode 100644 index 0000000000..38e33c387d --- /dev/null +++ b/.changeset/tidy-tile-menu-groups.md @@ -0,0 +1,5 @@ +--- +'@hyperdx/app': patch +--- + +fix: Keep the dashboard tile menu on screen when a dashboard has many groups, by moving the group list into a scrollable "Move to group" submenu diff --git a/packages/app/src/DBDashboardPage.tsx b/packages/app/src/DBDashboardPage.tsx index cc6c0c873b..ada2e7e680 100644 --- a/packages/app/src/DBDashboardPage.tsx +++ b/packages/app/src/DBDashboardPage.tsx @@ -99,7 +99,6 @@ import { IconChevronsDown, IconChevronsUp, IconCopy, - IconCornerDownRight, IconDeviceFloppy, IconDotsVertical, IconDownload, @@ -125,6 +124,11 @@ import { import { IsolatedChartSyncProvider } from '@/chartSync'; import { mergeAnnotations } from '@/components/charts/chartAnnotations'; import { ContactSupportText } from '@/components/ContactSupportText'; +import { + buildMoveTargets, + MoveTarget, + MoveToGroupSubmenu, +} from '@/components/dashboard/MoveToGroupSubmenu'; import SnapGridLayout from '@/components/dashboard/SnapGridLayout'; import DashboardContainer from '@/components/DashboardContainer'; import { @@ -376,14 +380,6 @@ function HeatmapTile({ ); } -type MoveTarget = { - containerId: string; - tabId?: string; - label: string; - // For tabs: all tabs in order with the target tab ID - allTabs?: { id: string; title: string }[]; -}; - const tileToLayoutItem = (chart: Tile): RGL.Layout => ({ i: chart.id, x: chart.x, @@ -990,64 +986,12 @@ const Tile = ({ Edit {canMoveToGroup && ( - <> - - Move to Group - {chart.containerId && ( - } - onClick={() => onMoveToGroup(undefined)} - > - (Ungrouped) - - )} - {moveTargets - .filter( - t => - !( - t.containerId === chart.containerId && - t.tabId === chart.tabId - ), - ) - .map(t => ( - } - onClick={() => onMoveToGroup(t.containerId, t.tabId)} - > - {t.allTabs ? ( - - {t.allTabs.map((tab, i) => ( - - {i > 0 && ( - - {' | '} - - )} - - {tab.title} - - - ))} - - ) : ( - t.label - )} - - ))} - + )} { if (readOnly) return null; @@ -1137,64 +1081,12 @@ const Tile = ({ Edit {canMoveToGroup && ( - <> - - Move to Group - {chart.containerId && ( - } - onClick={() => onMoveToGroup(undefined)} - > - (Ungrouped) - - )} - {moveTargets - .filter( - t => - !( - t.containerId === chart.containerId && - t.tabId === chart.tabId - ), - ) - .map(t => ( - } - onClick={() => onMoveToGroup(t.containerId, t.tabId)} - > - {t.allTabs ? ( - - {t.allTabs.map((tab, i) => ( - - {i > 0 && ( - - {' | '} - - )} - - {tab.title} - - - ))} - - ) : ( - t.label - )} - - ))} - + )} (() => { - const targets: MoveTarget[] = []; - for (const c of containers) { - const cTabs = c.tabs ?? []; - if (cTabs.length >= 2) { - for (const tab of cTabs) { - targets.push({ - containerId: c.id, - tabId: tab.id, - label: tab.title, - allTabs: cTabs.map(t => ({ id: t.id, title: t.title })), - }); - } - } else if (cTabs.length === 1) { - // 1-tab group: show just the group name, target the single tab - targets.push({ - containerId: c.id, - tabId: cTabs[0].id, - label: cTabs[0].title, - }); - } else { - targets.push({ containerId: c.id, label: c.title }); - } - } - return targets; - }, [containers]); + const moveTargetContainers = useMemo( + () => buildMoveTargets(containers), + [containers], + ); const hasContainers = containers.length > 0; const allTiles = useMemo(() => dashboard?.tiles ?? [], [dashboard?.tiles]); diff --git a/packages/app/src/components/dashboard/MoveToGroupSubmenu.tsx b/packages/app/src/components/dashboard/MoveToGroupSubmenu.tsx new file mode 100644 index 0000000000..a8681739d0 --- /dev/null +++ b/packages/app/src/components/dashboard/MoveToGroupSubmenu.tsx @@ -0,0 +1,87 @@ +import { Fragment } from 'react'; +import { DashboardContainer } from '@hyperdx/common-utils/dist/types'; +import { Menu } from '@mantine/core'; +import { IconCornerDownRight, IconFolderSymlink } from '@tabler/icons-react'; + +export type MoveTarget = { + containerId: string; + tabId?: string; + label: string; + // Heading for the tabs of a multi-tab group + group?: string; +}; + +export function buildMoveTargets( + containers: DashboardContainer[], +): MoveTarget[] { + return containers.flatMap(c => { + const tabs = c.tabs ?? []; + if (tabs.length === 0) return [{ containerId: c.id, label: c.title }]; + if (tabs.length === 1) + return [{ containerId: c.id, tabId: tabs[0].id, label: tabs[0].title }]; + // A multi-tab group is named by its first tab, matching its header + return tabs.map(tab => ({ + containerId: c.id, + tabId: tab.id, + label: tab.title, + group: tabs[0].title, + })); + }); +} + +// A submenu keeps the tile menu short however many groups the dashboard has. +export function MoveToGroupSubmenu({ + moveTargets, + containerId, + tabId, + onMoveToGroup, +}: { + moveTargets: MoveTarget[]; + containerId?: string; + tabId?: string; + onMoveToGroup: (containerId: string | undefined, tabId?: string) => void; +}) { + const targets = moveTargets.filter( + t => !(t.containerId === containerId && t.tabId === tabId), + ); + + return ( + + + }> + Move to group + + + + {containerId && ( + } + onClick={() => onMoveToGroup(undefined)} + > + (Ungrouped) + + )} + {containerId && targets.length > 0 && } + {targets.map((t, i) => { + const prev = targets[i - 1]; + // Sections key on container, since default group names repeat + const newContainer = prev?.containerId !== t.containerId; + return ( + + {prev && newContainer && (prev.group || t.group) && ( + + )} + {t.group && newContainer && {t.group}} + } + onClick={() => onMoveToGroup(t.containerId, t.tabId)} + > + {t.label} + + + ); + })} + + + ); +} diff --git a/packages/app/src/components/dashboard/__tests__/MoveToGroupSubmenu.test.tsx b/packages/app/src/components/dashboard/__tests__/MoveToGroupSubmenu.test.tsx new file mode 100644 index 0000000000..c0fbc9ca22 --- /dev/null +++ b/packages/app/src/components/dashboard/__tests__/MoveToGroupSubmenu.test.tsx @@ -0,0 +1,154 @@ +import { Menu } from '@mantine/core'; +import { screen, within } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; + +import { + buildMoveTargets, + MoveTarget, + MoveToGroupSubmenu, +} from '@/components/dashboard/MoveToGroupSubmenu'; + +const containers = [ + { + id: 'tabbed', + title: 'Ignored', + collapsed: false, + tabs: [ + { id: 't1', title: 'Services' }, + { id: 't2', title: 'Queues' }, + ], + }, + { + id: 'single', + title: 'Ignored', + collapsed: false, + tabs: [{ id: 's1', title: 'Single tab' }], + }, + { id: 'plain', title: 'Plain group', collapsed: false }, +]; + +describe('buildMoveTargets', () => { + it('lists each tab of a multi-tab group under its first tab, and one target per other group', () => { + expect(buildMoveTargets(containers)).toEqual([ + { + containerId: 'tabbed', + tabId: 't1', + label: 'Services', + group: 'Services', + }, + { + containerId: 'tabbed', + tabId: 't2', + label: 'Queues', + group: 'Services', + }, + { containerId: 'single', tabId: 's1', label: 'Single tab' }, + { containerId: 'plain', label: 'Plain group' }, + ]); + }); +}); + +describe('MoveToGroupSubmenu', () => { + async function openSubmenu( + props: { containerId?: string; tabId?: string } = {}, + moveTargets: MoveTarget[] = buildMoveTargets(containers), + ) { + const onMoveToGroup = jest.fn(); + // jsdom has no layout, so Popover would hide the dropdown as detached + const { unmount } = renderWithMantine( + + + + + + + + , + ); + await userEvent.hover(screen.getByText('Move to group')); + const submenu = (await screen.findAllByRole('menu')).at(-1)!; + return { submenu: within(submenu), onMoveToGroup, unmount }; + } + + it('shows one heading per multi-tab group and hides the current location', async () => { + const { submenu } = await openSubmenu({ + containerId: 'tabbed', + tabId: 't2', + }); + + expect(submenu.getAllByText('Services')).toHaveLength(2); // heading + tab + expect(submenu.queryByText('Queues')).not.toBeInTheDocument(); + expect(submenu.getByText('Single tab')).toBeInTheDocument(); + expect(submenu.getByText('(Ungrouped)')).toBeInTheDocument(); + }); + + it('offers no "(Ungrouped)" target for an ungrouped tile', async () => { + const { submenu } = await openSubmenu(); + expect(submenu.queryByText('(Ungrouped)')).not.toBeInTheDocument(); + }); + + it('moves to the chosen tab', async () => { + const { submenu, onMoveToGroup } = await openSubmenu(); + await userEvent.click(submenu.getByText('Queues')); + expect(onMoveToGroup).toHaveBeenCalledWith('tabbed', 't2'); + }); + + it('moves out of its group via "(Ungrouped)"', async () => { + const { submenu, onMoveToGroup } = await openSubmenu({ + containerId: 'plain', + }); + expect(submenu.queryByText('Plain group')).not.toBeInTheDocument(); + await userEvent.click(submenu.getByText('(Ungrouped)')); + expect(onMoveToGroup).toHaveBeenCalledWith(undefined); + }); + + it('gives each multi-tab group its own heading when names repeat', async () => { + const tabs = (id: string) => [ + { id: `${id}-1`, title: 'New Group' }, + { id: `${id}-2`, title: 'Tab 2' }, + ]; + const { submenu } = await openSubmenu( + {}, + buildMoveTargets([ + { id: 'a', title: 'New Group', collapsed: false, tabs: tabs('a') }, + { id: 'b', title: 'New Group', collapsed: false, tabs: tabs('b') }, + ]), + ); + expect(submenu.getAllByText('New Group')).toHaveLength(4); + expect( + submenu.getAllByRole('menuitem', { name: 'New Group', hidden: true }), + ).toHaveLength(2); + }); + + it('separates tabbed sections and "(Ungrouped)" from the groups around them', async () => { + const separatorsBefore = async ( + props: { containerId?: string }, + targets?: MoveTarget[], + ) => { + const { submenu, unmount } = await openSubmenu(props, targets); + const labels = submenu + .queryAllByRole('separator', { hidden: true }) + .map(s => s.nextElementSibling?.textContent); + unmount(); + return labels; + }; + + // Plain groups following each other share no divider + expect(await separatorsBefore({})).toEqual(['Single tab']); + expect(await separatorsBefore({ containerId: 'plain' })).toEqual([ + 'Services', + 'Single tab', + ]); + const [tabbed, single, plain] = containers; + expect( + await separatorsBefore({}, buildMoveTargets([plain, tabbed])), + ).toEqual(['Services']); + expect( + await separatorsBefore({}, buildMoveTargets([single, plain])), + ).toEqual([]); + }); +}); diff --git a/packages/app/tests/e2e/features/dashboard-tile-move-menu.spec.ts b/packages/app/tests/e2e/features/dashboard-tile-move-menu.spec.ts new file mode 100644 index 0000000000..c3cac8f8a9 --- /dev/null +++ b/packages/app/tests/e2e/features/dashboard-tile-move-menu.spec.ts @@ -0,0 +1,107 @@ +import { Page } from '@playwright/test'; + +import { DashboardPage } from '../page-objects/DashboardPage'; +import { getApiUrl } from '../utils/api-helpers'; +import { expect, test } from '../utils/base-test'; + +test.use({ viewport: { width: 1280, height: 720 } }); + +const TAB_NAMES = [ + 'Services', + 'Bff', + 'Queues', + 'App', + 'Orders', + 'Guest', + 'Ops', +]; +const LAST_TARGET = 'Plain group 11'; + +const markdownTile = (id: string, x: number, w: number) => ({ + id, + x, + y: 0, + w, + h: 4, + containerId: 'plain-12', + config: { + name: id, + source: 'Logs', + displayType: 'markdown', + select: [], + where: '', + whereLanguage: 'lucene', + markdown: id, + }, +}); + +// Enough groups that the move list is far taller than the viewport, with +// the tiles under test in the last group at the bottom of the page. +async function seedDashboard(page: Page): Promise { + const containers = [ + ...Array.from({ length: 6 }, (_, g) => ({ + id: `tabbed-${g}`, + title: `Tabbed ${g + 1}`, + collapsed: false, + tabs: TAB_NAMES.map((name, t) => ({ + id: `tabbed-${g}-${t}`, + title: `${name} ${g + 1}`, + })), + })), + ...Array.from({ length: 12 }, (_, g) => ({ + id: `plain-${g + 1}`, + title: `Plain group ${g + 1}`, + collapsed: false, + })), + ]; + const res = await page.request.post(`${getApiUrl()}/dashboards`, { + data: { + name: `E2E tile move menu ${Date.now()}`, + containers, + tiles: [ + markdownTile('wide-tile', 0, 12), + markdownTile('narrow-tile', 12, 3), + ], + tags: [], + }, + }); + expect(res.ok()).toBeTruthy(); + const body = await res.json(); + return body.id ?? body._id; +} + +test.describe( + 'Dashboard tile move menu', + { tag: ['@dashboard', '@full-stack'] }, + () => { + let dashboardPage: DashboardPage; + + test.beforeEach(async ({ page }) => { + dashboardPage = new DashboardPage(page); + await dashboardPage.gotoDashboard(await seedDashboard(page)); + }); + + async function expectMoveTargetsReachable() { + const submenu = await dashboardPage.openMoveToGroupSubmenu(); + // Fully on screen, so the last target is reached by the submenu's own scroll + await expect(submenu).toBeInViewport({ ratio: 1 }); + const last = dashboardPage.getTileMenuItem(LAST_TARGET); + await last.scrollIntoViewIfNeeded(); + await expect(last).toBeInViewport(); + } + + test('keeps Edit on screen for a tile at the bottom of the page', async () => { + await dashboardPage.openTileActionsMenuById('wide-tile'); + await expect(dashboardPage.getTileMenuItem('Edit')).toBeInViewport(); + await expectMoveTargetsReachable(); + }); + + test('keeps Edit on screen in the narrow-tile menu', async () => { + await dashboardPage.openTileActionsMenuById('narrow-tile', { + narrow: true, + }); + await expect(dashboardPage.getTileMenuItem('Edit')).toBeInViewport(); + await expectMoveTargetsReachable(); + }); + }, +); diff --git a/packages/app/tests/e2e/page-objects/DashboardPage.ts b/packages/app/tests/e2e/page-objects/DashboardPage.ts index fcfcdaf9f3..b47de94465 100644 --- a/packages/app/tests/e2e/page-objects/DashboardPage.ts +++ b/packages/app/tests/e2e/page-objects/DashboardPage.ts @@ -657,6 +657,33 @@ export class DashboardPage { .click(); } + /** + * Open the actions menu of the tile with this id. Narrow tiles swap the + * toolbar for a single collapsed menu button. + */ + async openTileActionsMenuById(tileId: string, { narrow = false } = {}) { + const tile = this.page.getByTestId(`dashboard-tile-${tileId}`); + await tile.scrollIntoViewIfNeeded(); + await tile.hover(); + const trigger = narrow + ? tile.locator('button').filter({ visible: true }) + : this.page.getByTestId(`tile-actions-button-${tileId}`); + await trigger.click(); + } + + getTileMenuItem(name: string): Locator { + return this.page.getByRole('menuitem', { name, exact: true }); + } + + /** + * Open the "Move to group" submenu of an open tile menu. + */ + async openMoveToGroupSubmenu(): Promise { + const item = this.getTileMenuItem('Move to group'); + await item.hover(); + return this.page.locator(`#${await item.getAttribute('aria-controls')}`); + } + /** * Edit a tile */ From 34918804c4290c7c1b5d7b4371633d6b4b85c30e Mon Sep 17 00:00:00 2001 From: Tushar Bahl Date: Fri, 16 Oct 2026 16:43:11 +0530 Subject: [PATCH 14/16] fix: spans details assertion to saw span body. --- packages/app/tests/e2e/components/SidePanelComponent.ts | 8 ++++---- packages/app/tests/e2e/components/TableComponent.ts | 6 +++++- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/packages/app/tests/e2e/components/SidePanelComponent.ts b/packages/app/tests/e2e/components/SidePanelComponent.ts index a947be46cb..a82acc67e8 100644 --- a/packages/app/tests/e2e/components/SidePanelComponent.ts +++ b/packages/app/tests/e2e/components/SidePanelComponent.ts @@ -231,9 +231,9 @@ export class SidePanelComponent { /** * The span detail drawer beside the waterfall. Trace sources don't render an * Overview tab of their own, so `tab-overview` plus the close control only - * exist together on this drawer. The drawer is the innermost container that - * holds both — ancestors also match, and the tab bar itself does not hold - * the overview body. + * exist together on this drawer. The tab bar's spacer is also `flex-grow-1`, + * so that class alone matches the tab chrome and misses the overview body. + * The overview root is `flex-grow-1 overflow-auto`. */ get spanDetailsPanel() { const closeSpanDetails = this.page.getByRole('button', { @@ -243,7 +243,7 @@ export class SidePanelComponent { .locator('div') .filter({ has: closeSpanDetails }) .filter({ has: this.page.getByTestId('tab-overview') }) - .filter({ has: this.page.locator('.flex-grow-1') }) + .filter({ has: this.page.locator('.flex-grow-1.overflow-auto') }) .last(); } diff --git a/packages/app/tests/e2e/components/TableComponent.ts b/packages/app/tests/e2e/components/TableComponent.ts index 41bde80b1f..6509b30c0d 100644 --- a/packages/app/tests/e2e/components/TableComponent.ts +++ b/packages/app/tests/e2e/components/TableComponent.ts @@ -68,7 +68,11 @@ export class TableComponent { } async clickRowContaining(text: string) { - await this.getRows().filter({ hasText: text }).first().click(); + const row = this.getRows().filter({ hasText: text }).first(); + // The open side panel covers the right side of the results table, so a + // click on the row's center lands on the drawer. The left edge of the + // first cell stays in the uncovered strip. + await row.locator('td').first().click({ position: { x: 4, y: 8 } }); } /** From 22833cebae8b31f34e9df7b677822218ba018201 Mon Sep 17 00:00:00 2001 From: Tushar Bahl Date: Wed, 21 Oct 2026 09:06:51 +0530 Subject: [PATCH 15/16] fix: lint error and e2e test fix. --- .../app/src/components/DBRowSidePanel.tsx | 1 + .../tests/e2e/components/TableComponent.ts | 38 +++++++++++++++++-- packages/app/tests/e2e/docker-compose.yml | 8 ++-- scripts/test-e2e.sh | 17 +++++++++ 4 files changed, 57 insertions(+), 7 deletions(-) diff --git a/packages/app/src/components/DBRowSidePanel.tsx b/packages/app/src/components/DBRowSidePanel.tsx index d84b5cb4af..30e4b718d8 100644 --- a/packages/app/src/components/DBRowSidePanel.tsx +++ b/packages/app/src/components/DBRowSidePanel.tsx @@ -206,6 +206,7 @@ function SidePanelHeaderActions({ size="sm" onClick={onClose} aria-label="Close" + data-testid="side-panel-close" > diff --git a/packages/app/tests/e2e/components/TableComponent.ts b/packages/app/tests/e2e/components/TableComponent.ts index 6509b30c0d..51b2d53451 100644 --- a/packages/app/tests/e2e/components/TableComponent.ts +++ b/packages/app/tests/e2e/components/TableComponent.ts @@ -69,10 +69,40 @@ export class TableComponent { async clickRowContaining(text: string) { const row = this.getRows().filter({ hasText: text }).first(); - // The open side panel covers the right side of the results table, so a - // click on the row's center lands on the drawer. The left edge of the - // first cell stays in the uncovered strip. - await row.locator('td').first().click({ position: { x: 4, y: 8 } }); + // The leading cell is the selection checkbox. Its click toggles selection + // and does not open the row. The content button does. An open side panel + // covers the right side of the table, so aim at the button's left edge. + // On a short viewport with the nav expanded that edge is covered too; the + // click still has to reach this button, since it is what switches the row. + const button = row.getByRole('button', { + name: /^(View details for log entry|Expand log row|Collapse log row)$/, + }); + if (await this.isCoveredBySidePanel(button)) { + // A pointer click at these coordinates lands on the drawer, which + // covers the row. The button is what switches the open row. + await button.evaluate(el => { + if (el instanceof HTMLElement) { + el.click(); + } + }); + return; + } + await button.click({ position: { x: 4, y: 8 } }); + } + + private async isCoveredBySidePanel(target: Locator): Promise { + const panel = this.page.getByTestId('row-side-panel'); + if ((await panel.count()) === 0 || !(await panel.isVisible())) { + return false; + } + const [targetBox, panelBox] = await Promise.all([ + target.boundingBox(), + panel.boundingBox(), + ]); + if (targetBox == null || panelBox == null) { + return false; + } + return panelBox.x <= targetBox.x + 8; } /** diff --git a/packages/app/tests/e2e/docker-compose.yml b/packages/app/tests/e2e/docker-compose.yml index b8152ccc6c..947a861ff9 100644 --- a/packages/app/tests/e2e/docker-compose.yml +++ b/packages/app/tests/e2e/docker-compose.yml @@ -22,9 +22,11 @@ services: environment: CLICKHOUSE_DEFAULT_ACCESS_MANAGEMENT: 1 volumes: - - ../../../../docker/clickhouse/local/config.xml:/etc/clickhouse-server/config.xml - - ../../../../docker/clickhouse/local/users.xml:/etc/clickhouse-server/users.xml - - ../../../../docker/clickhouse/local/init-db-e2e.sh:/docker-entrypoint-initdb.d/init-db.sh + # HDX_CLICKHOUSE_CONFIG_DIR is set when Docker cannot mount the checkout + # (macOS blocks the VM from Desktop, Documents, and Downloads). + - ${HDX_CLICKHOUSE_CONFIG_DIR:-../../../../docker/clickhouse/local}/config.xml:/etc/clickhouse-server/config.xml + - ${HDX_CLICKHOUSE_CONFIG_DIR:-../../../../docker/clickhouse/local}/users.xml:/etc/clickhouse-server/users.xml + - ${HDX_CLICKHOUSE_CONFIG_DIR:-../../../../docker/clickhouse/local}/init-db-e2e.sh:/docker-entrypoint-initdb.d/init-db.sh networks: - internal deploy: diff --git a/scripts/test-e2e.sh b/scripts/test-e2e.sh index 7b6e060d66..522e7aeaba 100755 --- a/scripts/test-e2e.sh +++ b/scripts/test-e2e.sh @@ -196,7 +196,24 @@ setup_cleanup_trap() { fi } +# macOS privacy blocks Docker's VM (com.docker.virtualization) from Desktop, +# Documents, and Downloads even when Docker.app has Full Disk Access. A file +# elsewhere in the home directory still mounts. Stage the bind-mounted +# ClickHouse files there so the container can start. +stage_clickhouse_config_if_needed() { + local src="$REPO_ROOT/docker/clickhouse/local" + if docker run --rm -v "$src/config.xml:/mnt/probe:ro" alpine true >/dev/null 2>&1; then + return 0 + fi + local dest="${HOME}/.cache/hyperdx/e2e-clickhouse" + mkdir -p "$dest" + cp "$src/config.xml" "$src/users.xml" "$src/init-db-e2e.sh" "$dest/" + export HDX_CLICKHOUSE_CONFIG_DIR="$dest" + echo "Docker cannot mount this checkout. Staged ClickHouse config at $dest" +} + setup_clickhouse() { + stage_clickhouse_config_if_needed echo "Starting ClickHouse..." docker compose -p "$E2E_PROJECT" -f "$DOCKER_COMPOSE_FILE" up -d ch-server From 011d4c35796468e92d4665c32318f807fc08ee5f Mon Sep 17 00:00:00 2001 From: Drew Davis Date: Thu, 22 Oct 2026 07:43:49 -0400 Subject: [PATCH 16/16] chore: Revert changes to unrelated files --- packages/app/tests/e2e/docker-compose.yml | 8 +++----- scripts/test-e2e.sh | 17 ----------------- 2 files changed, 3 insertions(+), 22 deletions(-) diff --git a/packages/app/tests/e2e/docker-compose.yml b/packages/app/tests/e2e/docker-compose.yml index 947a861ff9..b8152ccc6c 100644 --- a/packages/app/tests/e2e/docker-compose.yml +++ b/packages/app/tests/e2e/docker-compose.yml @@ -22,11 +22,9 @@ services: environment: CLICKHOUSE_DEFAULT_ACCESS_MANAGEMENT: 1 volumes: - # HDX_CLICKHOUSE_CONFIG_DIR is set when Docker cannot mount the checkout - # (macOS blocks the VM from Desktop, Documents, and Downloads). - - ${HDX_CLICKHOUSE_CONFIG_DIR:-../../../../docker/clickhouse/local}/config.xml:/etc/clickhouse-server/config.xml - - ${HDX_CLICKHOUSE_CONFIG_DIR:-../../../../docker/clickhouse/local}/users.xml:/etc/clickhouse-server/users.xml - - ${HDX_CLICKHOUSE_CONFIG_DIR:-../../../../docker/clickhouse/local}/init-db-e2e.sh:/docker-entrypoint-initdb.d/init-db.sh + - ../../../../docker/clickhouse/local/config.xml:/etc/clickhouse-server/config.xml + - ../../../../docker/clickhouse/local/users.xml:/etc/clickhouse-server/users.xml + - ../../../../docker/clickhouse/local/init-db-e2e.sh:/docker-entrypoint-initdb.d/init-db.sh networks: - internal deploy: diff --git a/scripts/test-e2e.sh b/scripts/test-e2e.sh index 522e7aeaba..7b6e060d66 100755 --- a/scripts/test-e2e.sh +++ b/scripts/test-e2e.sh @@ -196,24 +196,7 @@ setup_cleanup_trap() { fi } -# macOS privacy blocks Docker's VM (com.docker.virtualization) from Desktop, -# Documents, and Downloads even when Docker.app has Full Disk Access. A file -# elsewhere in the home directory still mounts. Stage the bind-mounted -# ClickHouse files there so the container can start. -stage_clickhouse_config_if_needed() { - local src="$REPO_ROOT/docker/clickhouse/local" - if docker run --rm -v "$src/config.xml:/mnt/probe:ro" alpine true >/dev/null 2>&1; then - return 0 - fi - local dest="${HOME}/.cache/hyperdx/e2e-clickhouse" - mkdir -p "$dest" - cp "$src/config.xml" "$src/users.xml" "$src/init-db-e2e.sh" "$dest/" - export HDX_CLICKHOUSE_CONFIG_DIR="$dest" - echo "Docker cannot mount this checkout. Staged ClickHouse config at $dest" -} - setup_clickhouse() { - stage_clickhouse_config_if_needed echo "Starting ClickHouse..." docker compose -p "$E2E_PROJECT" -f "$DOCKER_COMPOSE_FILE" up -d ch-server