From a11d95fa5c8a0d54bc8017cee8a6d717479e9c40 Mon Sep 17 00:00:00 2001 From: Jordan Simonovski Date: Wed, 5 Aug 2026 16:53:16 +1000 Subject: [PATCH 1/3] refactor(app): split the two largest chart files into directories MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit HDXMultiSeriesTimeChart.tsx 1503 -> 720 (+ 8 files) DBTimeChart.tsx 1022 -> 469 (+ 6 files) Each becomes a directory with a barrel, so every import path and every jest.mock('@/...') keeps resolving and the change is invisible to consumers. The seams follow what the code already separated rather than cutting by length: - searchUrl.ts takes buildSearchUrl out of a useCallback as a pure function, which makes its branching testable for the first time — which value column a series key resolves to, and whether that column's aggregation is attributable to individual events at all. A non-attributable aggregation must not produce a value filter, or drill-down returns rows that never contributed to the clicked point. Nine new tests cover it; that is the only new test surface here. - useChartScales holds the axis domains and the annotation elements. Pure derivation from props, no state, no recharts tree. - The tooltip, legend, recharts shape shims, layout constants, cross-chart pin registry and pure data helpers each move to a file named after what they are. MemoChart stays at 720 because what is left is one recharts element whose children must remain siblings, plus interaction state that genuinely shares a click-suppression flag with the brush-zoom. Splitting that tree would trade a real risk of dropping a render branch for a smaller number. Two deliberate changes beyond pure movement, both small: - xAxisDomain is typed as the [number, number] tuple it already returned rather than the wider AxisDomain, which removes two unsafe type assertions at its consumers. - dismissPinned and buildSearchUrl became block-bodied and a thin delegate respectively. Same behaviour. Verified by diffing every non-import line of the two originals against the new directories: the only lines that do not appear are import fragments that were redistributed and the two changes above. make ci-lint, make ci-unit (5205 tests) and the dashboard E2E suite (81 passed) all pass. E2E matters most here — it is the only check that catches a runtime-only breakage from moving code. First of the pieces split out of #2536. No feature code: this lands ahead of the exemplars overlay so that PR is a feature diff rather than a feature plus a 1,500-line move. --- .../DBTimeChart/__tests__/searchUrl.test.ts | 179 +++++ .../HDXMultiSeriesTimeChart/ChartLegend.tsx | 174 +++++ .../ChartTooltipContent.tsx | 162 ++++ .../MemoChart.tsx} | 703 +----------------- .../HDXMultiSeriesTimeChart/TooltipItem.tsx | 46 ++ .../__tests__/HDXMultiSeriesTimeChart.test.ts | 0 .../src/HDXMultiSeriesTimeChart/chartData.ts | 116 +++ .../chartPrimitives.tsx | 75 ++ .../src/HDXMultiSeriesTimeChart/constants.ts | 19 + .../app/src/HDXMultiSeriesTimeChart/index.ts | 15 + .../HDXMultiSeriesTimeChart/useChartScales.ts | 144 ++++ .../DBTimeChart/ChartTooltipOverlay.tsx | 152 ++++ .../{ => DBTimeChart}/DBTimeChart.tsx | 461 +----------- .../__tests__/DBTimeChart.test.tsx | 4 +- .../DBTimeChart/__tests__/searchUrl.test.ts | 179 +++++ .../components/DBTimeChart/crossChartPin.ts | 34 + .../app/src/components/DBTimeChart/index.ts | 7 + .../src/components/DBTimeChart/searchUrl.ts | 177 +++++ .../DBTimeChart/useChartToolbarItems.tsx | 145 ++++ 19 files changed, 1693 insertions(+), 1099 deletions(-) create mode 100644 packages/app/packages/app/src/components/DBTimeChart/__tests__/searchUrl.test.ts create mode 100644 packages/app/src/HDXMultiSeriesTimeChart/ChartLegend.tsx create mode 100644 packages/app/src/HDXMultiSeriesTimeChart/ChartTooltipContent.tsx rename packages/app/src/{HDXMultiSeriesTimeChart.tsx => HDXMultiSeriesTimeChart/MemoChart.tsx} (53%) create mode 100644 packages/app/src/HDXMultiSeriesTimeChart/TooltipItem.tsx rename packages/app/src/{ => HDXMultiSeriesTimeChart}/__tests__/HDXMultiSeriesTimeChart.test.ts (100%) create mode 100644 packages/app/src/HDXMultiSeriesTimeChart/chartData.ts create mode 100644 packages/app/src/HDXMultiSeriesTimeChart/chartPrimitives.tsx create mode 100644 packages/app/src/HDXMultiSeriesTimeChart/constants.ts create mode 100644 packages/app/src/HDXMultiSeriesTimeChart/index.ts create mode 100644 packages/app/src/HDXMultiSeriesTimeChart/useChartScales.ts create mode 100644 packages/app/src/components/DBTimeChart/ChartTooltipOverlay.tsx rename packages/app/src/components/{ => DBTimeChart}/DBTimeChart.tsx (50%) rename packages/app/src/components/{ => DBTimeChart}/__tests__/DBTimeChart.test.tsx (98%) create mode 100644 packages/app/src/components/DBTimeChart/__tests__/searchUrl.test.ts create mode 100644 packages/app/src/components/DBTimeChart/crossChartPin.ts create mode 100644 packages/app/src/components/DBTimeChart/index.ts create mode 100644 packages/app/src/components/DBTimeChart/searchUrl.ts create mode 100644 packages/app/src/components/DBTimeChart/useChartToolbarItems.tsx diff --git a/packages/app/packages/app/src/components/DBTimeChart/__tests__/searchUrl.test.ts b/packages/app/packages/app/src/components/DBTimeChart/__tests__/searchUrl.test.ts new file mode 100644 index 0000000000..57f2e17e62 --- /dev/null +++ b/packages/app/packages/app/src/components/DBTimeChart/__tests__/searchUrl.test.ts @@ -0,0 +1,179 @@ +import { + type ChartConfigWithDateRange, + SourceKind, + TSource, +} from '@hyperdx/common-utils/dist/types'; + +import { buildSeriesSearchUrl } from '@/components/DBTimeChart/searchUrl'; + +// The URL string itself is ChartUtils' concern and already covered there. What +// matters here is the branching this function does before delegating — which was +// previously locked inside a useCallback and untestable. +type SearchUrlArgs = { + dateRange: [Date, Date]; + groupFilters: { column: string; value: string }[]; + valueRangeFilter?: { expression: string; value: number }; +}; + +const mockBuildEventsSearchUrl = jest.fn( + () => '/search?mocked=1', +); +jest.mock('@/ChartUtils', () => ({ + ...jest.requireActual('@/ChartUtils'), + buildEventsSearchUrl: (arg: SearchUrlArgs) => mockBuildEventsSearchUrl(arg), +})); + +/** Args the function handed to buildEventsSearchUrl on its first call. */ +const delegatedArgs = () => mockBuildEventsSearchUrl.mock.calls[0][0]; + +const source = { + id: 'src-1', + kind: SourceKind.Log, + name: 'logs', + connection: 'conn-1', + from: { databaseName: 'default', tableName: 'otel_logs' }, + timestampValueExpression: 'Timestamp', +} as unknown as TSource; + +const clicked = new Date('2026-01-01T00:00:00Z'); + +const args = { + clickedActiveLabelDate: clicked as Date | undefined, + source: source as TSource | undefined, + granularity: '1 minute', + groupColumns: [] as string[], + valueColumns: undefined as string[] | undefined, + isSingleValueColumn: true as boolean | undefined, +}; + +const configWith = ( + select: { aggFn: string; valueExpression: string }[], +): ChartConfigWithDateRange => + ({ + connection: 'conn-1', + from: { databaseName: 'default', tableName: 'otel_logs' }, + timestampValueExpression: 'Timestamp', + where: '', + select, + dateRange: [clicked, clicked], + }) as ChartConfigWithDateRange; + +const rawSqlConfig = { + connection: 'conn-1', + configType: 'sql', + sqlTemplate: 'SELECT 1', + dateRange: [clicked, clicked], +} satisfies Partial as ChartConfigWithDateRange; + +beforeEach(() => jest.clearAllMocks()); + +describe('buildSeriesSearchUrl', () => { + describe('returns null when drill-down cannot be supported', () => { + it('with no clicked date', () => { + expect( + buildSeriesSearchUrl({ + ...args, + clickedActiveLabelDate: undefined, + config: configWith([{ aggFn: 'avg', valueExpression: 'Duration' }]), + }), + ).toBeNull(); + expect(mockBuildEventsSearchUrl).not.toHaveBeenCalled(); + }); + + it('with no resolved source', () => { + expect( + buildSeriesSearchUrl({ + ...args, + source: undefined, + config: configWith([{ aggFn: 'avg', valueExpression: 'Duration' }]), + }), + ).toBeNull(); + }); + + it('for a raw SQL chart', () => { + // Raw SQL doesn't resolve to a single source, so there is nothing to search. + expect( + buildSeriesSearchUrl({ + ...args, + config: rawSqlConfig, + }), + ).toBeNull(); + }); + }); + + it('ranges from the clicked bucket to one granularity later', () => { + buildSeriesSearchUrl({ + ...args, + config: configWith([{ aggFn: 'avg', valueExpression: 'Duration' }]), + }); + const { dateRange } = delegatedArgs(); + expect(dateRange[0]).toEqual(clicked); + expect(dateRange[1].getTime() - clicked.getTime()).toBe(60_000); + }); + + describe('value-range filter', () => { + it('is added for an attributable aggregation', () => { + buildSeriesSearchUrl({ + ...args, + seriesValue: 250, + config: configWith([{ aggFn: 'max', valueExpression: 'Duration' }]), + }); + const { valueRangeFilter } = delegatedArgs(); + expect(valueRangeFilter).toEqual({ + expression: 'Duration', + value: 250, + }); + }); + + it('is omitted for a non-attributable aggregation', () => { + // A `count`/`sum` point is not attributable to any single event's value, so + // filtering on it would return rows that never contributed to the point. + buildSeriesSearchUrl({ + ...args, + seriesValue: 250, + config: configWith([{ aggFn: 'count', valueExpression: 'Duration' }]), + }); + const { valueRangeFilter } = delegatedArgs(); + expect(valueRangeFilter).toBeUndefined(); + }); + + it('is omitted when no series value was clicked', () => { + buildSeriesSearchUrl({ + ...args, + config: configWith([{ aggFn: 'max', valueExpression: 'Duration' }]), + }); + const { valueRangeFilter } = delegatedArgs(); + expect(valueRangeFilter).toBeUndefined(); + }); + + it('resolves the value column by series-key prefix on a multi-value chart', () => { + // With more than one value column the series key is prefixed with the + // column name, so the filter must follow that prefix to the right select + // item rather than defaulting to select[0]. + buildSeriesSearchUrl({ + ...args, + seriesKey: 'p95', + seriesValue: 900, + isSingleValueColumn: false, + valueColumns: ['count', 'p95'], + config: configWith([ + { aggFn: 'count', valueExpression: 'Body' }, + { aggFn: 'p95', valueExpression: 'Duration' }, + ]), + }); + const { valueRangeFilter } = delegatedArgs(); + expect(valueRangeFilter).toEqual({ expression: 'Duration', value: 900 }); + }); + }); + + it('passes decoded group filters through', () => { + buildSeriesSearchUrl({ + ...args, + seriesKey: 'api', + groupColumns: ['ServiceName'], + config: configWith([{ aggFn: 'avg', valueExpression: 'Duration' }]), + }); + const { groupFilters } = delegatedArgs(); + expect(groupFilters).toEqual([{ column: 'ServiceName', value: 'api' }]); + }); +}); diff --git a/packages/app/src/HDXMultiSeriesTimeChart/ChartLegend.tsx b/packages/app/src/HDXMultiSeriesTimeChart/ChartLegend.tsx new file mode 100644 index 0000000000..b7a0e4fb6d --- /dev/null +++ b/packages/app/src/HDXMultiSeriesTimeChart/ChartLegend.tsx @@ -0,0 +1,174 @@ +import { memo, useMemo, useState } from 'react'; +import cx from 'classnames'; +import { Popover } from '@mantine/core'; + +import { type LineData } from '@/ChartUtils'; +import { truncateMiddle } from '@/utils'; + +import { hasSeriesSelection } from './chartData'; +import { MAX_LEGEND_ITEMS } from './constants'; + +import styles from '@styles/HDXLineChart.module.scss'; + +function ExpandableLegendItem({ + entry, + expanded, + isSelected, + isDisabled, + onToggle, +}: { + entry: any; + expanded?: boolean; + isSelected?: boolean; + isDisabled?: boolean; + onToggle?: (isShiftKey: boolean) => void; +}) { + const [_expanded, setExpanded] = useState(false); + const isExpanded = _expanded || expanded; + + return ( + { + if (onToggle) { + onToggle(e.shiftKey); + } else { + setExpanded(v => !v); + } + }} + title={ + isSelected + ? 'Click to show all (Shift+click to deselect)' + : 'Click to show only this (Shift+click for multi-select)' + } + > +
+ + + +
+ {isExpanded || isSelected + ? entry.value + : truncateMiddle(`${entry.value}`, 35)} + + ); +} + +export const LegendRenderer = memo<{ + payload?: { + dataKey: string; + value: string; + color: string; + }[]; + lineDataMap: { [key: string]: LineData }; + allLineData?: LineData[]; + selectedSeries?: Set; + onToggleSeries?: (seriesName: string, isShiftKey?: boolean) => void; +}>(props => { + const { payload, lineDataMap, allLineData, selectedSeries, onToggleSeries } = + props; + + const hasSelection = hasSeriesSelection(selectedSeries); + + // Use allLineData to ensure all series are always shown in legend + const allSeriesPayload = useMemo(() => { + if (allLineData?.length) { + return allLineData.map(ld => ({ + dataKey: ld.dataKey, + value: ld.displayName || ld.dataKey, + color: ld.color, + payload: { strokeDasharray: ld.isDashed ? '4 3' : '0' }, + })); + } + return payload ?? []; + }, [allLineData, payload]); + + const sortedLegendItems = useMemo(() => { + // Order items such that current and previous period lines are consecutive + const currentPeriodKeyIndex = new Map(); + allSeriesPayload.forEach((line, index) => { + const currentPeriodKey = + lineDataMap[line.dataKey]?.currentPeriodKey || ''; + if (!currentPeriodKeyIndex.has(currentPeriodKey)) { + currentPeriodKeyIndex.set(currentPeriodKey, index); + } + }); + + // Copy before sorting: when this comes from Recharts' legend payload it is + // kept in the Immer-backed store and frozen, so an in-place sort throws. + return [...allSeriesPayload].sort((a, b) => { + const keyA = lineDataMap[a.dataKey]?.currentPeriodKey ?? ''; + const keyB = lineDataMap[b.dataKey]?.currentPeriodKey ?? ''; + + const indexA = currentPeriodKeyIndex.get(keyA) ?? 0; + const indexB = currentPeriodKeyIndex.get(keyB) ?? 0; + + return indexB - indexA || a.dataKey.localeCompare(b.dataKey); + }); + }, [allSeriesPayload, lineDataMap]); + + const shownItems = sortedLegendItems.slice(0, MAX_LEGEND_ITEMS); + const restItems = sortedLegendItems.slice(MAX_LEGEND_ITEMS); + + return ( +
+ {shownItems.map((entry, index) => { + const isSelected = !!selectedSeries?.has(entry.value); + const isDisabled = hasSelection && !isSelected; + return ( + onToggleSeries?.(entry.value, isShiftKey)} + /> + ); + })} + {restItems.length ? ( + + +
+ +{restItems.length} more +
+ + +
+ {restItems.map((entry, index) => { + const isSelected = !!selectedSeries?.has(entry.value); + const isDisabled = hasSelection && !isSelected; + return ( + + onToggleSeries?.(entry.value, isShiftKey) + } + /> + ); + })} +
+ + + ) : null} +
+ ); +}); diff --git a/packages/app/src/HDXMultiSeriesTimeChart/ChartTooltipContent.tsx b/packages/app/src/HDXMultiSeriesTimeChart/ChartTooltipContent.tsx new file mode 100644 index 0000000000..58497eea43 --- /dev/null +++ b/packages/app/src/HDXMultiSeriesTimeChart/ChartTooltipContent.tsx @@ -0,0 +1,162 @@ +import { memo, useMemo } from 'react'; +import { withErrorBoundary } from 'react-error-boundary'; + +import { findNearestSeriesKey, type LineData } from '@/ChartUtils'; +import { + ChartTooltipContainer, + ChartTooltipHeader, + toViewportPoint, + useChartTooltipZIndex, +} from '@/components/charts/ChartTooltip'; +import type { NumberFormat } from '@/types'; + +import { + NEAREST_SERIES_MAX_DISTANCE_PX, + TOOLTIP_POINT_OFFSET_PX, +} from './constants'; +import { TooltipItem, type TooltipPayload } from './TooltipItem'; + +import styles from '@styles/HDXLineChart.module.scss'; + +type HDXLineChartTooltipProps = { + lineDataMap: { [keyName: string]: LineData }; + previousPeriodOffsetSeconds?: number; + numberFormat?: NumberFormat; + numberFormatByKey: Map; + /** Per-series active-point pixel Y, captured by the Area active dots. */ + activePointYByKeyRef: React.MutableRefObject>; + /** The chart's outer container; its viewport rect anchors this tooltip. */ + containerRef: React.MutableRefObject; +} & Record; + +/** + * The recharts `` content used for the HOVER tooltip (on the hovered + * chart and its synced followers). Clicking pins ChartSeriesTooltip instead. + * + * Because it's given `portal={document.body}`, recharts skips its own transform + * positioning, so this content self-anchors at the active point with + * `position: fixed` (container rect + `coordinate`) — matching the pinned + * tooltip's anchor, and escaping the chart's bounds so edges aren't clipped. + */ +export const HDXLineChartTooltip = withErrorBoundary( + memo((props: HDXLineChartTooltipProps) => { + const { + active, + payload, + label, + numberFormat, + numberFormatByKey, + lineDataMap, + previousPeriodOffsetSeconds, + activePointYByKeyRef, + containerRef, + } = props; + const typedPayload = payload as TooltipPayload[]; + + const tooltipZIndex = useChartTooltipZIndex(); + + const payloadByKey = useMemo( + () => new Map(typedPayload.map(p => [p.dataKey, p])), + [typedPayload], + ); + + if (active && payload && payload.length) { + // No onClose: hover renders the X hidden (kept for layout parity). + const header = ( + + ); + + // Bold the line nearest the cursor by comparing pointer Y to each series' + // active-dot Y. The dots write their positions earlier in this same render + // (Recharts draws graphical items before the tooltip), so it's current. + const pointerY: number | undefined = props.coordinate?.y; + // eslint-disable-next-line react-hooks/refs + const activePointYByKey = activePointYByKeyRef?.current ?? undefined; + const nearestSeriesKey = + typedPayload.length > 1 + ? findNearestSeriesKey( + activePointYByKey, + typedPayload.map(p => p.dataKey), + pointerY, + NEAREST_SERIES_MAX_DISTANCE_PX, + ) + : undefined; + + // Anchor at the active point (see the component docblock for why fixed). + const pointX = props.coordinate?.x; + const pointY = props.coordinate?.y; + // eslint-disable-next-line react-hooks/refs + const containerRect = containerRef?.current?.getBoundingClientRect(); + const anchor = + typeof pointX === 'number' && + typeof pointY === 'number' && + containerRect != null + ? toViewportPoint(containerRect, { x: pointX, y: pointY }) + : undefined; + const anchorStyle: React.CSSProperties = + anchor != null + ? { + position: 'fixed', + left: anchor.x, + top: anchor.y + TOOLTIP_POINT_OFFSET_PX, + transform: 'translateX(-50%)', + pointerEvents: 'none', + // z-index must live here: recharts leaves the portaled wrapper + // `position: static`, where z-index has no effect. + zIndex: tooltipZIndex, + } + : {}; + + return ( +
+ + {/* Copy before sorting: Recharts 3 freezes the payload, so an + in-place sort throws "this object has been frozen". */} + {[...payload] + .sort((a: TooltipPayload, b: TooltipPayload) => b.value - a.value) + .map((p: TooltipPayload) => { + const previousKey = lineDataMap[p.dataKey]?.previousPeriodKey; + const isPreviousPeriod = previousKey === p.dataKey; + const previousPayload = + !isPreviousPeriod && previousKey + ? payloadByKey.get(previousKey) + : undefined; + const valueColumnName = + lineDataMap[p.dataKey]?.valueColumnName ?? p.dataKey; + const numberFormatForKey = + numberFormatByKey.get(valueColumnName) ?? numberFormat; + + return ( + + ); + })} + +
+ ); + } + return null; + }), + { + onError: console.error, + fallback: ( +
+ An error occurred while rendering the tooltip. +
+ ), + }, +); diff --git a/packages/app/src/HDXMultiSeriesTimeChart.tsx b/packages/app/src/HDXMultiSeriesTimeChart/MemoChart.tsx similarity index 53% rename from packages/app/src/HDXMultiSeriesTimeChart.tsx rename to packages/app/src/HDXMultiSeriesTimeChart/MemoChart.tsx index 421e4f8351..edbf9787e6 100644 --- a/packages/app/src/HDXMultiSeriesTimeChart.tsx +++ b/packages/app/src/HDXMultiSeriesTimeChart/MemoChart.tsx @@ -7,15 +7,11 @@ import { useRef, useState, } from 'react'; -import cx from 'classnames'; -import { add, isSameSecond, sub } from 'date-fns'; -import { withErrorBoundary } from 'react-error-boundary'; import { Area, AreaChart, Bar, BarChart, - BarProps, CartesianGrid, Legend, ReferenceArea, @@ -25,594 +21,42 @@ import { XAxis, YAxis, } from 'recharts'; -import { AxisDomain } from 'recharts/types/util/types'; -import { convertGranularityToSeconds } from '@hyperdx/common-utils/dist/core/utils'; import { DisplayType } from '@hyperdx/common-utils/dist/types'; -import { Popover } from '@mantine/core'; +import { useChartSyncId } from '@/chartSync'; +import { findNearestSeriesKey, LineData } from '@/ChartUtils'; +import { ChartAnnotation } from '@/components/charts/chartAnnotations'; +import { ChartOverlayControls } from '@/components/charts/ChartOverlayControls'; +import { toViewportPoint } from '@/components/charts/ChartTooltip'; import type { NumberFormat } from '@/types'; -import { COLORS, formatNumber, truncateMiddle } from '@/utils'; +import { useFormatTime } from '@/useFormatTime'; +import { COLORS, formatNumber } from '@/utils'; import { - ChartAnnotation, - getAnnotationElements, -} from './components/charts/chartAnnotations'; -import { ChartOverlayControls } from './components/charts/ChartOverlayControls'; + type ActiveClickPayload, + buildActiveClickSeries, + getActiveLabel, + getSeriesDisplayName, + getVisibleLineData, + hasSeriesSelection, +} from './chartData'; +import { LegendRenderer } from './ChartLegend'; +import { CaptureActiveDot, StackedBarWithOverlap } from './chartPrimitives'; +import { HDXLineChartTooltip } from './ChartTooltipContent'; import { - ChartTooltipContainer, - ChartTooltipHeader, - ChartTooltipItem, - toViewportPoint, - useChartTooltipZIndex, -} from './components/charts/ChartTooltip'; -import { useChartSyncId } from './chartSync'; -import { - findNearestSeriesKey, - LineData, - MAX_TIME_CHART_SERIES, - toStartOfInterval, -} from './ChartUtils'; -import { useFormatTime } from './useFormatTime'; - -import styles from '@styles/HDXLineChart.module.scss'; - -const MAX_LEGEND_ITEMS = 4; - -// Vertical pixel distance within which a series' line counts as "near" the -// cursor for tooltip highlighting. Beyond this, no row is emphasized so the -// tooltip is not misleading when the pointer is in empty space. -const NEAREST_SERIES_MAX_DISTANCE_PX = 30; - -// Gap below the data point for the hover tooltip. Kept equal to the pinned -// tooltip's Popover `offset` so both land in the same spot. -const TOOLTIP_POINT_OFFSET_PX = 12; - -const Y_AXIS_WIDTH = 40; -const SINGLE_POINT_BAR_RIGHT_PADDING = 10; -const SINGLE_POINT_BAR_WIDTH_RATIO = 0.8; -// Top margin (px) reserved above the plot for annotation labels ("Alert"/"OK"), -// added only when a chart is showing annotations so other charts keep their -// tighter default headroom. -const ANNOTATION_LABEL_HEADROOM = 18; - -type TooltipPayload = { - dataKey: string; - name: string; - value: number; - color?: string; - stroke?: string; - strokeWidth?: number; - strokeDasharray?: string; - opacity?: number; -}; - -export const TooltipItem = memo( - ({ - p, - previous, - numberFormat, - highlighted, - dimmed, - }: { - p: TooltipPayload; - previous?: TooltipPayload; - numberFormat?: NumberFormat; - highlighted?: boolean; - dimmed?: boolean; - }) => { - return ( - - ); - }, -); - -type HDXLineChartTooltipProps = { - lineDataMap: { [keyName: string]: LineData }; - previousPeriodOffsetSeconds?: number; - numberFormat?: NumberFormat; - numberFormatByKey: Map; - /** Per-series active-point pixel Y, captured by the Area active dots. */ - activePointYByKeyRef: React.MutableRefObject>; - /** The chart's outer container; its viewport rect anchors this tooltip. */ - containerRef: React.MutableRefObject; -} & Record; - -/** - * The recharts `` content used for the HOVER tooltip (on the hovered - * chart and its synced followers). Clicking pins ChartSeriesTooltip instead. - * - * Because it's given `portal={document.body}`, recharts skips its own transform - * positioning, so this content self-anchors at the active point with - * `position: fixed` (container rect + `coordinate`) — matching the pinned - * tooltip's anchor, and escaping the chart's bounds so edges aren't clipped. - */ -const HDXLineChartTooltip = withErrorBoundary( - memo((props: HDXLineChartTooltipProps) => { - const { - active, - payload, - label, - numberFormat, - numberFormatByKey, - lineDataMap, - previousPeriodOffsetSeconds, - activePointYByKeyRef, - containerRef, - } = props; - const typedPayload = payload as TooltipPayload[]; - - const tooltipZIndex = useChartTooltipZIndex(); - - const payloadByKey = useMemo( - () => new Map(typedPayload.map(p => [p.dataKey, p])), - [typedPayload], - ); - - if (active && payload && payload.length) { - // No onClose: hover renders the X hidden (kept for layout parity). - const header = ( - - ); - - // Bold the line nearest the cursor by comparing pointer Y to each series' - // active-dot Y. The dots write their positions earlier in this same render - // (Recharts draws graphical items before the tooltip), so it's current. - const pointerY: number | undefined = props.coordinate?.y; - // eslint-disable-next-line react-hooks/refs - const activePointYByKey = activePointYByKeyRef?.current ?? undefined; - const nearestSeriesKey = - typedPayload.length > 1 - ? findNearestSeriesKey( - activePointYByKey, - typedPayload.map(p => p.dataKey), - pointerY, - NEAREST_SERIES_MAX_DISTANCE_PX, - ) - : undefined; - - // Anchor at the active point (see the component docblock for why fixed). - const pointX = props.coordinate?.x; - const pointY = props.coordinate?.y; - // eslint-disable-next-line react-hooks/refs - const containerRect = containerRef?.current?.getBoundingClientRect(); - const anchor = - typeof pointX === 'number' && - typeof pointY === 'number' && - containerRect != null - ? toViewportPoint(containerRect, { x: pointX, y: pointY }) - : undefined; - const anchorStyle: React.CSSProperties = - anchor != null - ? { - position: 'fixed', - left: anchor.x, - top: anchor.y + TOOLTIP_POINT_OFFSET_PX, - transform: 'translateX(-50%)', - pointerEvents: 'none', - // z-index must live here: recharts leaves the portaled wrapper - // `position: static`, where z-index has no effect. - zIndex: tooltipZIndex, - } - : {}; - - return ( -
- - {/* Copy before sorting: Recharts 3 freezes the payload, so an - in-place sort throws "this object has been frozen". */} - {[...payload] - .sort((a: TooltipPayload, b: TooltipPayload) => b.value - a.value) - .map((p: TooltipPayload) => { - const previousKey = lineDataMap[p.dataKey]?.previousPeriodKey; - const isPreviousPeriod = previousKey === p.dataKey; - const previousPayload = - !isPreviousPeriod && previousKey - ? payloadByKey.get(previousKey) - : undefined; - const valueColumnName = - lineDataMap[p.dataKey]?.valueColumnName ?? p.dataKey; - const numberFormatForKey = - numberFormatByKey.get(valueColumnName) ?? numberFormat; - - return ( - - ); - })} - -
- ); - } - return null; - }), - { - onError: console.error, - fallback: ( -
- An error occurred while rendering the tooltip. -
- ), - }, -); + ANNOTATION_LABEL_HEADROOM, + NEAREST_SERIES_MAX_DISTANCE_PX, + SINGLE_POINT_BAR_RIGHT_PADDING, + SINGLE_POINT_BAR_WIDTH_RATIO, + Y_AXIS_WIDTH, +} from './constants'; +import { useChartScales } from './useChartScales'; -function ExpandableLegendItem({ - entry, - expanded, - isSelected, - isDisabled, - onToggle, -}: { - entry: any; - expanded?: boolean; - isSelected?: boolean; - isDisabled?: boolean; - onToggle?: (isShiftKey: boolean) => void; -}) { - const [_expanded, setExpanded] = useState(false); - const isExpanded = _expanded || expanded; - - return ( - { - if (onToggle) { - onToggle(e.shiftKey); - } else { - setExpanded(v => !v); - } - }} - title={ - isSelected - ? 'Click to show all (Shift+click to deselect)' - : 'Click to show only this (Shift+click for multi-select)' - } - > -
- - - -
- {isExpanded || isSelected - ? entry.value - : truncateMiddle(`${entry.value}`, 35)} - - ); -} - -const LegendRenderer = memo<{ - payload?: { - dataKey: string; - value: string; - color: string; - }[]; - lineDataMap: { [key: string]: LineData }; - allLineData?: LineData[]; - selectedSeries?: Set; - onToggleSeries?: (seriesName: string, isShiftKey?: boolean) => void; -}>(props => { - const { payload, lineDataMap, allLineData, selectedSeries, onToggleSeries } = - props; - - const hasSelection = hasSeriesSelection(selectedSeries); - - // Use allLineData to ensure all series are always shown in legend - const allSeriesPayload = useMemo(() => { - if (allLineData?.length) { - return allLineData.map(ld => ({ - dataKey: ld.dataKey, - value: ld.displayName || ld.dataKey, - color: ld.color, - payload: { strokeDasharray: ld.isDashed ? '4 3' : '0' }, - })); - } - return payload ?? []; - }, [allLineData, payload]); - - const sortedLegendItems = useMemo(() => { - // Order items such that current and previous period lines are consecutive - const currentPeriodKeyIndex = new Map(); - allSeriesPayload.forEach((line, index) => { - const currentPeriodKey = - lineDataMap[line.dataKey]?.currentPeriodKey || ''; - if (!currentPeriodKeyIndex.has(currentPeriodKey)) { - currentPeriodKeyIndex.set(currentPeriodKey, index); - } - }); - - // Copy before sorting: when this comes from Recharts' legend payload it is - // kept in the Immer-backed store and frozen, so an in-place sort throws. - return [...allSeriesPayload].sort((a, b) => { - const keyA = lineDataMap[a.dataKey]?.currentPeriodKey ?? ''; - const keyB = lineDataMap[b.dataKey]?.currentPeriodKey ?? ''; - - const indexA = currentPeriodKeyIndex.get(keyA) ?? 0; - const indexB = currentPeriodKeyIndex.get(keyB) ?? 0; - - return indexB - indexA || a.dataKey.localeCompare(b.dataKey); - }); - }, [allSeriesPayload, lineDataMap]); - - const shownItems = sortedLegendItems.slice(0, MAX_LEGEND_ITEMS); - const restItems = sortedLegendItems.slice(MAX_LEGEND_ITEMS); - - return ( -
- {shownItems.map((entry, index) => { - const isSelected = !!selectedSeries?.has(entry.value); - const isDisabled = hasSelection && !isSelected; - return ( - onToggleSeries?.(entry.value, isShiftKey)} - /> - ); - })} - {restItems.length ? ( - - -
- +{restItems.length} more -
- - -
- {restItems.map((entry, index) => { - const isSelected = !!selectedSeries?.has(entry.value); - const isDisabled = hasSelection && !isSelected; - return ( - - onToggleSeries?.(entry.value, isShiftKey) - } - /> - ); - })} -
- - - ) : null} -
- ); -}); - -export const HARD_LINES_LIMIT = MAX_TIME_CHART_SERIES; - -// Debounce (ms) for the chart's ResponsiveContainer resize observer. Without // it the observer fires on every frame, and a resize → re-render → resize // cycle can keep the chart (and the form controls around it in the tile // editor) from ever settling. const RESPONSIVE_CONTAINER_DEBOUNCE_MS = 50; -/** One series entry in a tooltip's per-bucket payload (hover or click-frozen). */ -export type ActiveClickSeries = { - value?: number; - dataKey?: string; - name?: string; - /** Series color, matching the legend swatch. */ - color?: string; - /** Previous-period value at the same bucket, for the percent-change chip. */ - previousValue?: number; - /** Whether this series is a dashed previous-period line. */ - isPreviousPeriod?: boolean; - /** Result column the values came from, for per-column number formatting. */ - valueColumnName?: string; -}; - -/** - * State for the pinned (click-locked) tooltip. Produced by MemoChart's onClick - * and rendered by DBTimeChart via ChartSeriesTooltip. (Hover uses recharts' own - * ; recharts' is also kept for its synced cursor.) - */ -export type ActiveClickPayload = { - /** Active point in viewport coords; the Popover anchor. */ - viewportX: number; - viewportY: number; - activeLabel: string; - activePayload?: ActiveClickSeries[]; -}; - -/** Series label shown in the legend, tooltip, and line `name`. */ -const getSeriesDisplayName = (ld: LineData) => ld.displayName || ld.dataKey; - -/** Normalize a chart event's active label (number | string) to a string. */ -const getActiveLabel = (state?: { - activeLabel?: string | number; -}): string | undefined => - state?.activeLabel != null ? String(state.activeLabel) : undefined; - -/** - * Build the per-series payload for a click-frozen tooltip from the data row at - * the clicked bucket. Only the visible series (legend selection + - * HARD_LINES_LIMIT) with a numeric value at that bucket are included, so the - * drill-down popover mirrors exactly what is drawn. Exported for unit testing. - */ -export function buildActiveClickSeries( - visibleLineData: LineData[], - activeRow: Record | undefined, -): ActiveClickSeries[] { - if (activeRow == null) return []; - return visibleLineData.flatMap(ld => { - const value = activeRow[ld.dataKey]; - if (typeof value !== 'number') return []; - const isPreviousPeriod = ld.previousPeriodKey === ld.dataKey; - // Pair each current-period series with its previous-period value for the - // percent-change chip. Only current-period rows carry a comparison. - const previousRaw = - !isPreviousPeriod && ld.previousPeriodKey - ? activeRow[ld.previousPeriodKey] - : undefined; - return [ - { - dataKey: ld.dataKey, - name: getSeriesDisplayName(ld), - value, - color: ld.color, - isPreviousPeriod, - valueColumnName: ld.valueColumnName, - previousValue: - typeof previousRaw === 'number' ? previousRaw : undefined, - }, - ]; - }); -} - -/** - * The series actually drawn on the chart. Without a selection, the first - * HARD_LINES_LIMIT of lineData. With a selection (legend isolate, checkbox - * filter, or table search), the selection is applied FIRST and then capped, so - * an explicitly chosen series always draws even if it ranks beyond the limit. - * Applying the cap first would slice out a chosen low-ranked series, leaving an - * empty chart while its stats still show in the legend table. The rendered - * lines and the drill-down click payload both derive from this same set so they - * never diverge. Exported for unit testing. - */ -/** - * Whether a series selection is active. The single source of truth for the - * "isolate to these series" predicate that gates line visibility, the y-axis - * domain, legend dimming, and the "Show All Series" control — so those can't - * drift out of sync. - */ -function hasSeriesSelection( - selectedSeriesNames: Set | undefined, -): selectedSeriesNames is Set { - return !!selectedSeriesNames && selectedSeriesNames.size > 0; -} - -export function getVisibleLineData( - lineData: LineData[], - selectedSeriesNames: Set | undefined, -): LineData[] { - const hasSelection = hasSeriesSelection(selectedSeriesNames); - if (hasSelection) { - return lineData - .filter(ld => selectedSeriesNames.has(getSeriesDisplayName(ld))) - .slice(0, HARD_LINES_LIMIT); - } - return lineData.slice(0, HARD_LINES_LIMIT); -} - -const StackedBarWithOverlap = (props: BarProps) => { - const { x, y, width, fill } = props; - // `height` may arrive as a string, so coerce it to a number before the - // arithmetic below. - const height = - typeof props.height === 'number' ? props.height : Number(props.height ?? 0); - // Add a tiny bit to the height to create overlap. Otherwise there's a gap - return ( - 0 ? height + 0.5 : 0} - fill={fill} - /> - ); -}; - -type CaptureActiveDotProps = { - /** - * Called with each series' active-point pixel Y. This is a stable callback - * (not the ref itself) so Recharts, which stores this element's props in its - * Immer-backed store and freezes them, never freezes the underlying Map — - * the write happens on the ref captured in the callback's closure instead. - */ - onCapture: (dataKey: string, cy: number) => void; - cx?: number; - cy?: number; - dataKey?: string | number; - r?: number; - fill?: string; - stroke?: string; - strokeWidth?: number; -}; - -/** - * Active dot for an Area series. Records the active point's pixel Y (`cy`) - * via `onCapture`, keyed by dataKey, then draws the same dot Recharts - * renders by default. Recharts clones this element with the active-point - * props (cx, cy, dataKey, r, fill, stroke, strokeWidth) during the render - * that precedes the tooltip, so the capture is current when the tooltip reads - * it to find the series nearest the cursor. - */ -function CaptureActiveDot({ - onCapture, - cx, - cy, - dataKey, - r, - fill, - stroke, - strokeWidth, -}: CaptureActiveDotProps) { - if (dataKey != null && typeof cy === 'number' && Number.isFinite(cy)) { - // Written synchronously during render so the tooltip, which Recharts - // renders after the graphical items in the same commit, reads the - // current frame's positions rather than the previous frame's. - onCapture(String(dataKey), cy); - } - if (typeof cx !== 'number' || typeof cy !== 'number') { - return null; - } - return ( - - ); -} - /** * Compute the unique set of hexes referenced by `` defs * inside MemoChart. Exported so a unit test can pin the dedup-and-union @@ -801,64 +245,18 @@ export const MemoChart = memo(function MemoChart({ captureActivePointY, ]); - const yAxisDomain: AxisDomain = useMemo(() => { - const hasSelection = hasSeriesSelection(selectedSeriesNames); - - // Fitting the y-axis lower bound to the data only applies to line charts. - // Bar charts are always anchored at zero so the bar lengths stay - // proportional to their values. - const shouldFitYAxis = - fitYAxisToData && displayType !== DisplayType.StackedBar; - - // The data min/max is only needed to either zoom into a selection or to - // fit the lower bound to the data. When neither applies, let Recharts - // auto-calculate the upper bound while pinning the lower bound to zero. - if (!hasSelection && !shouldFitYAxis) { - return [0, 'auto']; - } - - // Calculate domain based on visible series (all series when there's no - // explicit selection). - let minValue = Infinity; - let maxValue = -Infinity; - - graphResults.forEach(dataPoint => { - lineData.forEach(ld => { - const seriesName = ld.displayName || ld.dataKey; - // Only consider visible series - if (!hasSelection || selectedSeriesNames.has(seriesName)) { - const value = dataPoint[ld.dataKey]; - if (typeof value === 'number' && !isNaN(value)) { - minValue = Math.min(minValue, value); - maxValue = Math.max(maxValue, value); - } - } - }); - }); - - // If we found valid values, return them with some padding - if (minValue !== Infinity && maxValue !== -Infinity) { - const padding = (maxValue - minValue) * 0.05; // 5% padding - // When fitting to data, allow the lower bound to follow the data - // minimum; otherwise keep it pinned at zero. The 5% padding must not - // drag the axis below zero unless the data itself is negative, so - // clamp at zero whenever the minimum is non-negative. - const lowerBound = - shouldFitYAxis && minValue < 0 - ? minValue - padding - : Math.max(0, minValue - padding); - const upperBound = maxValue + padding; - return [lowerBound, upperBound]; - } - - return ['auto', 'auto']; - }, [ + // Axis domains and annotation elements — see useChartScales. + const { yAxisDomain, xAxisDomain, annotationElements } = useChartScales({ + annotations, + dateRange, + granularity, + dateRangeEndInclusive, + displayType, + fitYAxisToData, graphResults, lineData, selectedSeriesNames, - fitYAxisToData, - displayType, - ]); + }); const [containerWidth, setContainerWidth] = useState(0); @@ -1014,41 +412,6 @@ export const MemoChart = memo(function MemoChart({ return map; }, [lineData]); - const xAxisDomain: AxisDomain = useMemo(() => { - let startTime = toStartOfInterval(dateRange[0], granularity); - let endTime = toStartOfInterval(dateRange[1], granularity); - const endTimeIsBoundaryAligned = isSameSecond(dateRange[1], endTime); - if (endTimeIsBoundaryAligned && !dateRangeEndInclusive) { - endTime = sub(endTime, { - seconds: convertGranularityToSeconds(granularity), - }); - } - - // For bar charts, extend the domain in both directions by half a granularity unit - // so that the full bar width is within the bounds of the chart - if (displayType === DisplayType.StackedBar) { - const halfGranularitySeconds = - convertGranularityToSeconds(granularity) / 2; - startTime = sub(startTime, { seconds: halfGranularitySeconds }); - endTime = add(endTime, { seconds: halfGranularitySeconds }); - } - - return [startTime.getTime() / 1000, endTime.getTime() / 1000]; - }, [dateRange, granularity, dateRangeEndInclusive, displayType]); - - // Alert/event markers as dashed lines, clamped to the chart's x-axis domain so - // an edge marker (e.g. an alert already firing at window open) stays visible - // instead of being dropped. Labels float in the reserved top headroom. - const annotationElements = useMemo(() => { - if (!annotations?.length) { - return null; - } - // xAxisDomain is a [min, max] tuple at runtime (declared as AxisDomain). - return getAnnotationElements(annotations, { - domain: xAxisDomain as [number, number], - }); - }, [annotations, xAxisDomain]); - return (
{ + return ( + + ); + }, +); diff --git a/packages/app/src/__tests__/HDXMultiSeriesTimeChart.test.ts b/packages/app/src/HDXMultiSeriesTimeChart/__tests__/HDXMultiSeriesTimeChart.test.ts similarity index 100% rename from packages/app/src/__tests__/HDXMultiSeriesTimeChart.test.ts rename to packages/app/src/HDXMultiSeriesTimeChart/__tests__/HDXMultiSeriesTimeChart.test.ts diff --git a/packages/app/src/HDXMultiSeriesTimeChart/chartData.ts b/packages/app/src/HDXMultiSeriesTimeChart/chartData.ts new file mode 100644 index 0000000000..4bf17e582f --- /dev/null +++ b/packages/app/src/HDXMultiSeriesTimeChart/chartData.ts @@ -0,0 +1,116 @@ +import { type LineData, MAX_TIME_CHART_SERIES } from '@/ChartUtils'; + +export const HARD_LINES_LIMIT = MAX_TIME_CHART_SERIES; + +// Debounce (ms) for the chart's ResponsiveContainer resize observer. Without +// it the observer fires on every frame, and a resize → re-render → resize +// cycle can keep the chart (and the form controls around it in the tile + +/** One series entry in a tooltip's per-bucket payload (hover or click-frozen). */ +export type ActiveClickSeries = { + value?: number; + dataKey?: string; + name?: string; + /** Series color, matching the legend swatch. */ + color?: string; + /** Previous-period value at the same bucket, for the percent-change chip. */ + previousValue?: number; + /** Whether this series is a dashed previous-period line. */ + isPreviousPeriod?: boolean; + /** Result column the values came from, for per-column number formatting. */ + valueColumnName?: string; +}; + +/** + * State for the pinned (click-locked) tooltip. Produced by MemoChart's onClick + * and rendered by DBTimeChart via ChartSeriesTooltip. (Hover uses recharts' own + * ; recharts' is also kept for its synced cursor.) + */ +export type ActiveClickPayload = { + /** Active point in viewport coords; the Popover anchor. */ + viewportX: number; + viewportY: number; + activeLabel: string; + activePayload?: ActiveClickSeries[]; +}; + +/** Series label shown in the legend, tooltip, and line `name`. */ +export const getSeriesDisplayName = (ld: LineData) => + ld.displayName || ld.dataKey; + +/** Normalize a chart event's active label (number | string) to a string. */ +export const getActiveLabel = (state?: { + activeLabel?: string | number; +}): string | undefined => + state?.activeLabel != null ? String(state.activeLabel) : undefined; + +/** + * Build the per-series payload for a click-frozen tooltip from the data row at + * the clicked bucket. Only the visible series (legend selection + + * HARD_LINES_LIMIT) with a numeric value at that bucket are included, so the + * drill-down popover mirrors exactly what is drawn. Exported for unit testing. + */ +export function buildActiveClickSeries( + visibleLineData: LineData[], + activeRow: Record | undefined, +): ActiveClickSeries[] { + if (activeRow == null) return []; + return visibleLineData.flatMap(ld => { + const value = activeRow[ld.dataKey]; + if (typeof value !== 'number') return []; + const isPreviousPeriod = ld.previousPeriodKey === ld.dataKey; + // Pair each current-period series with its previous-period value for the + // percent-change chip. Only current-period rows carry a comparison. + const previousRaw = + !isPreviousPeriod && ld.previousPeriodKey + ? activeRow[ld.previousPeriodKey] + : undefined; + return [ + { + dataKey: ld.dataKey, + name: getSeriesDisplayName(ld), + value, + color: ld.color, + isPreviousPeriod, + valueColumnName: ld.valueColumnName, + previousValue: + typeof previousRaw === 'number' ? previousRaw : undefined, + }, + ]; + }); +} + +/** + * Whether a series selection is active. The single source of truth for the + * "isolate to these series" predicate that gates line visibility, the y-axis + * domain, legend dimming, and the "Show All Series" control — so those can't + * drift out of sync. + */ +export function hasSeriesSelection( + selectedSeriesNames: Set | undefined, +): selectedSeriesNames is Set { + return !!selectedSeriesNames && selectedSeriesNames.size > 0; +} + +/** + * The series actually drawn on the chart. Without a selection, the first + * HARD_LINES_LIMIT of lineData. With a selection (legend isolate, checkbox + * filter, or table search), the selection is applied FIRST and then capped, so + * an explicitly chosen series always draws even if it ranks beyond the limit. + * Applying the cap first would slice out a chosen low-ranked series, leaving an + * empty chart while its stats still show in the legend table. The rendered + * lines and the drill-down click payload both derive from this same set so they + * never diverge. Exported for unit testing. + */ +export function getVisibleLineData( + lineData: LineData[], + selectedSeriesNames: Set | undefined, +): LineData[] { + const hasSelection = hasSeriesSelection(selectedSeriesNames); + if (hasSelection) { + return lineData + .filter(ld => selectedSeriesNames.has(getSeriesDisplayName(ld))) + .slice(0, HARD_LINES_LIMIT); + } + return lineData.slice(0, HARD_LINES_LIMIT); +} diff --git a/packages/app/src/HDXMultiSeriesTimeChart/chartPrimitives.tsx b/packages/app/src/HDXMultiSeriesTimeChart/chartPrimitives.tsx new file mode 100644 index 0000000000..1e49260d93 --- /dev/null +++ b/packages/app/src/HDXMultiSeriesTimeChart/chartPrimitives.tsx @@ -0,0 +1,75 @@ +import type { BarProps } from 'recharts'; + +export const StackedBarWithOverlap = (props: BarProps) => { + const { x, y, width, fill } = props; + // `height` may arrive as a string, so coerce it to a number before the + // arithmetic below. + const height = + typeof props.height === 'number' ? props.height : Number(props.height ?? 0); + // Add a tiny bit to the height to create overlap. Otherwise there's a gap + return ( + 0 ? height + 0.5 : 0} + fill={fill} + /> + ); +}; + +type CaptureActiveDotProps = { + /** + * Called with each series' active-point pixel Y. This is a stable callback + * (not the ref itself) so Recharts, which stores this element's props in its + * Immer-backed store and freezes them, never freezes the underlying Map — + * the write happens on the ref captured in the callback's closure instead. + */ + onCapture: (dataKey: string, cy: number) => void; + cx?: number; + cy?: number; + dataKey?: string | number; + r?: number; + fill?: string; + stroke?: string; + strokeWidth?: number; +}; + +/** + * Active dot for an Area series. Records the active point's pixel Y (`cy`) + * via `onCapture`, keyed by dataKey, then draws the same dot Recharts + * renders by default. Recharts clones this element with the active-point + * props (cx, cy, dataKey, r, fill, stroke, strokeWidth) during the render + * that precedes the tooltip, so the capture is current when the tooltip reads + * it to find the series nearest the cursor. + */ +export function CaptureActiveDot({ + onCapture, + cx, + cy, + dataKey, + r, + fill, + stroke, + strokeWidth, +}: CaptureActiveDotProps) { + if (dataKey != null && typeof cy === 'number' && Number.isFinite(cy)) { + // Written synchronously during render so the tooltip, which Recharts + // renders after the graphical items in the same commit, reads the + // current frame's positions rather than the previous frame's. + onCapture(String(dataKey), cy); + } + if (typeof cx !== 'number' || typeof cy !== 'number') { + return null; + } + return ( + + ); +} diff --git a/packages/app/src/HDXMultiSeriesTimeChart/constants.ts b/packages/app/src/HDXMultiSeriesTimeChart/constants.ts new file mode 100644 index 0000000000..68a22908af --- /dev/null +++ b/packages/app/src/HDXMultiSeriesTimeChart/constants.ts @@ -0,0 +1,19 @@ +/** Layout and measurement constants shared across the chart's parts. */ +export const MAX_LEGEND_ITEMS = 4; + +// Vertical pixel distance within which a series' line counts as "near" the +// cursor for tooltip highlighting. Beyond this, no row is emphasized so the +// tooltip is not misleading when the pointer is in empty space. +export const NEAREST_SERIES_MAX_DISTANCE_PX = 30; + +// Gap below the data point for the hover tooltip. Kept equal to the pinned +// tooltip's Popover `offset` so both land in the same spot. +export const TOOLTIP_POINT_OFFSET_PX = 12; + +export const Y_AXIS_WIDTH = 40; +export const SINGLE_POINT_BAR_RIGHT_PADDING = 10; +export const SINGLE_POINT_BAR_WIDTH_RATIO = 0.8; +// Top margin (px) reserved above the plot for annotation labels ("Alert"/"OK"), +// added only when a chart is showing annotations so other charts keep their +// tighter default headroom. +export const ANNOTATION_LABEL_HEADROOM = 18; diff --git a/packages/app/src/HDXMultiSeriesTimeChart/index.ts b/packages/app/src/HDXMultiSeriesTimeChart/index.ts new file mode 100644 index 0000000000..f6dea6a6c5 --- /dev/null +++ b/packages/app/src/HDXMultiSeriesTimeChart/index.ts @@ -0,0 +1,15 @@ +/** + * Public surface of the multi-series time chart. Split out of a single + * 1500-line module; consumers (and the tests that + * `jest.mock('@/HDXMultiSeriesTimeChart')`) import from here, so the internal + * file layout stays free to change. + */ +export { + type ActiveClickPayload, + type ActiveClickSeries, + buildActiveClickSeries, + getVisibleLineData, + HARD_LINES_LIMIT, +} from './chartData'; +export { collectMemoChartGradientHexes, MemoChart } from './MemoChart'; +export { TooltipItem } from './TooltipItem'; diff --git a/packages/app/src/HDXMultiSeriesTimeChart/useChartScales.ts b/packages/app/src/HDXMultiSeriesTimeChart/useChartScales.ts new file mode 100644 index 0000000000..1a76b324c0 --- /dev/null +++ b/packages/app/src/HDXMultiSeriesTimeChart/useChartScales.ts @@ -0,0 +1,144 @@ +import { useMemo } from 'react'; +import { add, isSameSecond, sub } from 'date-fns'; +import { AxisDomain } from 'recharts/types/util/types'; +import { convertGranularityToSeconds } from '@hyperdx/common-utils/dist/core/utils'; +import { DisplayType } from '@hyperdx/common-utils/dist/types'; + +import { type LineData, toStartOfInterval } from '@/ChartUtils'; +import { + ChartAnnotation, + getAnnotationElements, +} from '@/components/charts/chartAnnotations'; + +import { hasSeriesSelection } from './chartData'; + +type UseChartScalesArgs = { + annotations: ChartAnnotation[] | undefined; + dateRange: readonly [Date, Date]; + granularity: string; + dateRangeEndInclusive: boolean; + displayType: DisplayType; + fitYAxisToData: boolean | undefined; + graphResults: Record[]; + lineData: LineData[]; + selectedSeriesNames: Set | undefined; +}; + +/** + * Derive the chart's axis domains and the annotation elements that hang off the + * x-domain. + * + * Extracted from MemoChart because it is pure derivation from props — no state, + * no event handlers, no recharts tree — and the y-domain rules (fit-to-data, + * legend selection, zero-anchored bars) are easier to follow on their own. + */ +export function useChartScales({ + annotations, + dateRange, + granularity, + dateRangeEndInclusive, + displayType, + fitYAxisToData, + graphResults, + lineData, + selectedSeriesNames, +}: UseChartScalesArgs) { + const yAxisDomain: AxisDomain = useMemo(() => { + const hasSelection = hasSeriesSelection(selectedSeriesNames); + + // Fitting the y-axis lower bound to the data only applies to line charts. + // Bar charts are always anchored at zero so the bar lengths stay + // proportional to their values. + const shouldFitYAxis = + fitYAxisToData && displayType !== DisplayType.StackedBar; + + // The domain follows the visible series only. With no selection and no fit, + // let Recharts auto-scale, which pins the lower bound to 0. + if (!hasSelection && !shouldFitYAxis) { + return [0, 'auto']; + } + + // Calculate domain based on visible series (all series when there's no + // explicit selection). + let minValue = Infinity; + let maxValue = -Infinity; + + graphResults.forEach(dataPoint => { + lineData.forEach(ld => { + const seriesName = ld.displayName || ld.dataKey; + // Only consider visible series + if (!hasSelection || selectedSeriesNames.has(seriesName)) { + const value = dataPoint[ld.dataKey]; + if (typeof value === 'number' && !isNaN(value)) { + minValue = Math.min(minValue, value); + maxValue = Math.max(maxValue, value); + } + } + }); + }); + + // If we found valid values, return them with some padding + if (minValue !== Infinity && maxValue !== -Infinity) { + const padding = (maxValue - minValue) * 0.05; // 5% padding + // When fitting to data, allow the lower bound to follow the data + // minimum; otherwise keep it pinned at zero. The 5% padding must not + // drag the axis below zero unless the data itself is negative, so + // clamp at zero whenever the minimum is non-negative. + const lowerBound = + shouldFitYAxis && minValue < 0 + ? minValue - padding + : Math.max(0, minValue - padding); + const upperBound = maxValue + padding; + return [lowerBound, upperBound]; + } + + return ['auto', 'auto']; + }, [ + graphResults, + lineData, + selectedSeriesNames, + fitYAxisToData, + displayType, + ]); + + // Typed as the tuple it actually returns rather than the wider AxisDomain, so + // the annotation elements below can read [min, max] without asserting. Still + // assignable to XAxis's `domain`. + const xAxisDomain: [number, number] = useMemo(() => { + let startTime = toStartOfInterval(dateRange[0], granularity); + let endTime = toStartOfInterval(dateRange[1], granularity); + const endTimeIsBoundaryAligned = isSameSecond(dateRange[1], endTime); + if (endTimeIsBoundaryAligned && !dateRangeEndInclusive) { + endTime = sub(endTime, { + seconds: convertGranularityToSeconds(granularity), + }); + } + + // For bar charts, extend the domain in both directions by half a granularity unit + // so that the full bar width is within the bounds of the chart + if (displayType === DisplayType.StackedBar) { + const halfGranularitySeconds = + convertGranularityToSeconds(granularity) / 2; + startTime = sub(startTime, { seconds: halfGranularitySeconds }); + endTime = add(endTime, { seconds: halfGranularitySeconds }); + } + + return [startTime.getTime() / 1000, endTime.getTime() / 1000]; + }, [dateRange, granularity, dateRangeEndInclusive, displayType]); + + // Alert/event markers as dashed lines, clamped to the chart's x-axis domain so + // an edge marker (e.g. an alert already firing at window open) stays visible + // instead of being dropped. Labels float in the reserved top headroom. + const annotationElements = useMemo(() => { + if (!annotations?.length) { + return null; + } + return getAnnotationElements(annotations, { domain: xAxisDomain }); + }, [annotations, xAxisDomain]); + + return { + yAxisDomain, + xAxisDomain, + annotationElements, + }; +} diff --git a/packages/app/src/components/DBTimeChart/ChartTooltipOverlay.tsx b/packages/app/src/components/DBTimeChart/ChartTooltipOverlay.tsx new file mode 100644 index 0000000000..bfa1d82dfb --- /dev/null +++ b/packages/app/src/components/DBTimeChart/ChartTooltipOverlay.tsx @@ -0,0 +1,152 @@ +import { useEffect, useRef } from 'react'; +import { NumberFormat } from '@hyperdx/common-utils/dist/types'; +import { Popover, Portal } from '@mantine/core'; + +import { ChartSeriesTooltip } from '@/components/charts/ChartSeriesTooltip'; +import { useChartTooltipZIndex } from '@/components/charts/ChartTooltip'; +import type { ActiveClickPayload } from '@/HDXMultiSeriesTimeChart'; + +// The interactive PINNED tooltip, rendered over the chart in a body-portaled +// Mantine Popover anchored at the clicked point. Hover uses the recharts tooltip +// in MemoChart instead; this is only for the click-locked state. +export function ChartTooltipOverlay({ + payload, + buildSearchUrl, + onDismiss, + onFocusSeries, + onShowAllSeries, + fallbackNumberFormat, + numberFormatByKey, + previousPeriodOffsetSeconds, +}: { + payload: ActiveClickPayload | undefined; + buildSearchUrl: (key?: string, value?: number) => string | null; + onDismiss: () => void; + /** Focus a series by its raw series key (dataKey) and display name. */ + onFocusSeries: (payload: { dataKey?: string; name: string }) => void; + /** Clear an active series focus; undefined when nothing is focused. */ + onShowAllSeries?: () => void; + fallbackNumberFormat?: NumberFormat; + /** Per-value-column formats, keyed by result column name. */ + numberFormatByKey: Map; + previousPeriodOffsetSeconds?: number; +}) { + const isOpen = + payload != null && + payload.activePayload != null && + payload.activePayload.length > 0; + + const popoverZIndex = useChartTooltipZIndex({ pinned: true }); + + const dropdownRef = useRef(null); + + // The pinned tooltip anchors at `position: fixed` viewport coords captured + // once at click time. When a surrounding scroll container scrolls, the chart + // moves but the fixed tooltip stays glued to the viewport, detaching from its + // data point (Mantine's closeOnClickOutside/closeOnEscape don't fire on + // scroll). Dismiss on scroll instead so it never floats away — but ignore + // scrolls originating inside the tooltip's own scrollable series list, or a + // long tooltip couldn't be scrolled without instantly closing. + useEffect(() => { + if (!isOpen) return; + const handleScroll = (e: Event) => { + const target = e.target as Node | null; + if (target != null && dropdownRef.current?.contains(target)) { + return; + } + onDismiss(); + }; + window.addEventListener('scroll', handleScroll, { + capture: true, + passive: true, + }); + return () => { + window.removeEventListener('scroll', handleScroll, { capture: true }); + }; + }, [isOpen, onDismiss]); + + // Dismiss on outside click. Mantine's closeOnClickOutside misses it because + // the chart's recharts onClick calls stopPropagation (see + // HDXMultiSeriesTimeChart handleClick); a capture-phase listener sees the + // click regardless, ignoring clicks inside the tooltip's own dropdown. + useEffect(() => { + if (!isOpen) return; + const handleMouseDown = (e: MouseEvent) => { + const target = e.target; + if (target instanceof Node && dropdownRef.current?.contains(target)) { + return; + } + onDismiss(); + }; + document.addEventListener('mousedown', handleMouseDown, true); + return () => { + document.removeEventListener('mousedown', handleMouseDown, true); + }; + }, [isOpen, onDismiss]); + + if (!isOpen) { + return null; + } + + return ( + // Portal to body so the `position: fixed` anchor resolves against the + // viewport: dashboard tiles use CSS transforms, and a transformed ancestor + // would otherwise make `fixed` resolve against it and throw the tooltip off. + + { + if (!opened) { + onDismiss(); + } + }} + closeOnClickOutside + closeOnEscape + trapFocus={false} + withinPortal + position="bottom" + offset={12} + middlewares={{ flip: true, shift: true }} + returnFocus={false} + zIndex={popoverZIndex} + > + + {/* 1x1 anchor at the clicked data point. */} +
+ + + + + + + ); +} diff --git a/packages/app/src/components/DBTimeChart.tsx b/packages/app/src/components/DBTimeChart/DBTimeChart.tsx similarity index 50% rename from packages/app/src/components/DBTimeChart.tsx rename to packages/app/src/components/DBTimeChart/DBTimeChart.tsx index 8d1b99ccb4..ec70bc7772 100644 --- a/packages/app/src/components/DBTimeChart.tsx +++ b/packages/app/src/components/DBTimeChart/DBTimeChart.tsx @@ -1,35 +1,15 @@ -import React, { - memo, - useCallback, - useEffect, - useId, - useMemo, - useRef, - useState, -} from 'react'; -import { add, differenceInSeconds } from 'date-fns'; -import { - convertGranularityToSeconds, - getAlignedDateRange, -} from '@hyperdx/common-utils/dist/core/utils'; -import { - isBuilderChartConfig, - isPromqlChartConfig, - isRawSqlChartConfig, -} from '@hyperdx/common-utils/dist/guards'; +import React, { memo, useCallback, useEffect, useMemo, useState } from 'react'; +import { differenceInSeconds } from 'date-fns'; +import { getAlignedDateRange } from '@hyperdx/common-utils/dist/core/utils'; +import { isBuilderChartConfig } from '@hyperdx/common-utils/dist/guards'; import { BuilderChartConfigWithDateRange, ChartConfigWithDateRange, DisplayType, } from '@hyperdx/common-utils/dist/types'; -import { Popover, Portal } from '@mantine/core'; -import { IconChartBar, IconChartLine } from '@tabler/icons-react'; import api from '@/api'; import { - AGG_FNS, - buildEventsSearchUrl, - ChartKeyJoiner, convertToTimeChartConfig, formatResponseForTimeChart, getPreviousDateRange, @@ -37,240 +17,22 @@ import { useTimeChartSettings, } from '@/ChartUtils'; import { ChartAnnotation } from '@/components/charts/chartAnnotations'; -import { ChartSeriesTooltip } from '@/components/charts/ChartSeriesTooltip'; -import { useChartTooltipZIndex } from '@/components/charts/ChartTooltip'; +import ChartContainer from '@/components/charts/ChartContainer'; +import ChartErrorState, { + ChartErrorStateVariant, +} from '@/components/charts/ChartErrorState'; +import { ChartTooltipOverlay } from '@/components/DBTimeChart/ChartTooltipOverlay'; +import { useCrossChartPinDismiss } from '@/components/DBTimeChart/crossChartPin'; +import { + buildSeriesSearchUrl, + decodeSeriesGroupFilters, + type SeriesGroupFilter, +} from '@/components/DBTimeChart/searchUrl'; +import { useChartToolbarItems } from '@/components/DBTimeChart/useChartToolbarItems'; import { type ActiveClickPayload, MemoChart } from '@/HDXMultiSeriesTimeChart'; import { useQueriedChartConfig } from '@/hooks/useChartConfig'; import { useMVOptimizationExplanation } from '@/hooks/useMVOptimizationExplanation'; import { useChartNumberFormats, useSource } from '@/source'; -import type { NumberFormat } from '@/types'; - -import ChartContainer from './charts/ChartContainer'; -import ChartErrorState, { - ChartErrorStateVariant, -} from './charts/ChartErrorState'; -import DateRangeIndicator from './charts/DateRangeIndicator'; -import DisplaySwitcher from './charts/DisplaySwitcher'; -import MVOptimizationIndicator from './MaterializedViews/MVOptimizationIndicator'; - -/** A single group column / value pair decoded from a chart series key. */ -export type SeriesGroupFilter = { column: string; value: string }; - -// Only one pinned tooltip at a time across all charts. Module-level (not -// context) because charts can be scattered with no common provider, and their -// onClick stopPropagation hides cross-chart clicks from Mantine's click-outside. -const pinnedTooltipRegistry = new Map void>(); - -function broadcastTooltipPinned(activeId: string) { - pinnedTooltipRegistry.forEach((dismiss, id) => { - if (id !== activeId) { - dismiss(); - } - }); -} - -// Registers this chart's dismiss handler and returns a callback to close every -// other chart's pinned tooltip (call it when pinning this one). -function useCrossChartPinDismiss(onDismiss: () => void): () => void { - const id = useId(); - // Keep the latest onDismiss without re-subscribing each render. - const onDismissRef = useRef(onDismiss); - useEffect(() => { - onDismissRef.current = onDismiss; - }, [onDismiss]); - - useEffect(() => { - pinnedTooltipRegistry.set(id, () => onDismissRef.current()); - return () => { - pinnedTooltipRegistry.delete(id); - }; - }, [id]); - - return useCallback(() => broadcastTooltipPinned(id), [id]); -} - -// Decode a Recharts series key (e.g. "count · error · api") into the -// underlying group-column filters. This is the same decode `buildSearchUrl` -// uses to build a drill-down URL, extracted so the focus callback can hand the -// caller structured filters (rather than a display string) to apply to a -// sibling results list. -export function decodeSeriesGroupFilters({ - seriesKey, - groupColumns, - isSingleValueColumn, -}: { - seriesKey: string | undefined; - groupColumns: string[]; - isSingleValueColumn: boolean | undefined; -}): SeriesGroupFilter[] { - const seriesKeys = seriesKey?.split(ChartKeyJoiner); - const groupFilters: SeriesGroupFilter[] = []; - - if (seriesKeys?.length && groupColumns?.length) { - // When the series has multiple value columns, the key is prefixed with the - // value column name (e.g. "count · error"), so the group values start at - // index 1. (The "no group columns" case the original inline code also - // guarded is impossible here — this block only runs when groupColumns is - // non-empty.) - const startsWithValueColumn = !(isSingleValueColumn ?? true); - const groupValues = startsWithValueColumn - ? seriesKeys.slice(1) - : seriesKeys; - - groupValues.forEach((value, index) => { - if (groupColumns[index] != null) { - groupFilters.push({ column: groupColumns[index], value }); - } - }); - } - - return groupFilters; -} - -// The interactive PINNED tooltip, rendered over the chart in a body-portaled -// Mantine Popover anchored at the clicked point. Hover uses the recharts tooltip -// in MemoChart instead; this is only for the click-locked state. -function ChartTooltipOverlay({ - payload, - buildSearchUrl, - onDismiss, - onFocusSeries, - onShowAllSeries, - fallbackNumberFormat, - numberFormatByKey, - previousPeriodOffsetSeconds, -}: { - payload: ActiveClickPayload | undefined; - buildSearchUrl: (key?: string, value?: number) => string | null; - onDismiss: () => void; - /** Focus a series by its raw series key (dataKey) and display name. */ - onFocusSeries: (payload: { dataKey?: string; name: string }) => void; - /** Clear an active series focus; undefined when nothing is focused. */ - onShowAllSeries?: () => void; - fallbackNumberFormat?: NumberFormat; - /** Per-value-column formats, keyed by result column name. */ - numberFormatByKey: Map; - previousPeriodOffsetSeconds?: number; -}) { - const isOpen = - payload != null && - payload.activePayload != null && - payload.activePayload.length > 0; - - const popoverZIndex = useChartTooltipZIndex({ pinned: true }); - - const dropdownRef = useRef(null); - - // The pinned tooltip anchors at `position: fixed` viewport coords captured - // once at click time. When a surrounding scroll container scrolls, the chart - // moves but the fixed tooltip stays glued to the viewport, detaching from its - // data point (Mantine's closeOnClickOutside/closeOnEscape don't fire on - // scroll). Dismiss on scroll instead so it never floats away — but ignore - // scrolls originating inside the tooltip's own scrollable series list, or a - // long tooltip couldn't be scrolled without instantly closing. - useEffect(() => { - if (!isOpen) return; - const handleScroll = (e: Event) => { - const target = e.target as Node | null; - if (target != null && dropdownRef.current?.contains(target)) { - return; - } - onDismiss(); - }; - window.addEventListener('scroll', handleScroll, { - capture: true, - passive: true, - }); - return () => { - window.removeEventListener('scroll', handleScroll, { capture: true }); - }; - }, [isOpen, onDismiss]); - - // Dismiss on outside click. Mantine's closeOnClickOutside misses it because - // the chart's recharts onClick calls stopPropagation (see - // HDXMultiSeriesTimeChart handleClick); a capture-phase listener sees the - // click regardless, ignoring clicks inside the tooltip's own dropdown. - useEffect(() => { - if (!isOpen) return; - const handleMouseDown = (e: MouseEvent) => { - const target = e.target; - if (target instanceof Node && dropdownRef.current?.contains(target)) { - return; - } - onDismiss(); - }; - document.addEventListener('mousedown', handleMouseDown, true); - return () => { - document.removeEventListener('mousedown', handleMouseDown, true); - }; - }, [isOpen, onDismiss]); - - if (!isOpen) { - return null; - } - - return ( - // Portal to body so the `position: fixed` anchor resolves against the - // viewport: dashboard tiles use CSS transforms, and a transformed ancestor - // would otherwise make `fixed` resolve against it and throw the tooltip off. - - { - if (!opened) { - onDismiss(); - } - }} - closeOnClickOutside - closeOnEscape - trapFocus={false} - withinPortal - position="bottom" - offset={12} - middlewares={{ flip: true, shift: true }} - returnFocus={false} - zIndex={popoverZIndex} - > - - {/* 1x1 anchor at the clicked data point. */} -
- - - - - - - ); -} type DBTimeChartComponentProps = { config: ChartConfigWithDateRange; @@ -553,7 +315,9 @@ function DBTimeChartComponent({ ActiveClickPayload | undefined >(undefined); - const dismissPinned = useCallback(() => setActiveClickPayload(undefined), []); + const dismissPinned = useCallback(() => { + setActiveClickPayload(undefined); + }, []); const notifyTooltipPinned = useCrossChartPinDismiss(dismissPinned); // Pin the tooltip on click. Not gated on `source`: source-less charts still @@ -580,104 +344,18 @@ function DBTimeChartComponent({ }, [activeClickPayload]); const buildSearchUrl = useCallback( - (seriesKey?: string, seriesValue?: number) => { - // Raw SQL charts are not supported for drill-down as we don't know the source which is being used. - if ( - clickedActiveLabelDate == null || - source == null || - isRawSqlChartConfig(config) || - isPromqlChartConfig(config) - ) { - return null; - } - - // Parse the series key to extract group values - const seriesKeys = seriesKey?.split(ChartKeyJoiner); - const groupFilters = decodeSeriesGroupFilters({ + (seriesKey?: string, seriesValue?: number) => + buildSeriesSearchUrl({ seriesKey, - groupColumns, - isSingleValueColumn, - }); - - // Build value range filter for Y-axis if provided - let valueRangeFilter: - | { - expression: string; - value: number; - } - | undefined; - - if ( - seriesValue && - Array.isArray(config.select) && - config.select.length > 0 - ) { - // Determine which value column to filter on - let valueExpression: string | undefined; - - if ((isSingleValueColumn ?? true) && config.select.length === 1) { - const firstSelect = config.select[0]; - const aggFn = - typeof firstSelect === 'string' ? undefined : firstSelect.aggFn; - // Only add value range filter if the aggregation is attributable - const isAttributable = - AGG_FNS.find(fn => fn.value === aggFn)?.isAttributable !== false; - - if (isAttributable) { - valueExpression = - typeof firstSelect === 'string' - ? firstSelect - : firstSelect.valueExpression; - } - } else if (seriesKeys?.length && (valueColumns?.length ?? 0) > 0) { - const firstPart = seriesKeys[0]; - const valueColumnIndex = valueColumns?.findIndex( - col => col === firstPart, - ); - - if ( - valueColumnIndex != null && - valueColumnIndex >= 0 && - valueColumnIndex < config.select.length - ) { - const selectItem = config.select[valueColumnIndex]; - const aggFn = - typeof selectItem === 'string' ? undefined : selectItem.aggFn; - // Only add value range filter if the aggregation is attributable - const isAttributable = - AGG_FNS.find(fn => fn.value === aggFn)?.isAttributable !== false; - - if (isAttributable) { - valueExpression = - typeof selectItem === 'string' - ? selectItem - : selectItem.valueExpression; - } - } - } - - if (valueExpression) { - valueRangeFilter = { - expression: valueExpression, - value: seriesValue, - }; - } - } - - // Calculate time range from clicked date and granularity - const from = clickedActiveLabelDate; - const to = add(clickedActiveLabelDate, { - seconds: convertGranularityToSeconds(granularity), - }); - - return buildEventsSearchUrl({ + seriesValue, + clickedActiveLabelDate, source, config, - dateRange: [from, to], - groupFilters, - valueRangeFilter, - }); - }, + granularity, + groupColumns, + valueColumns, + isSingleValueColumn, + }), [ clickedActiveLabelDate, config, @@ -712,91 +390,20 @@ function DBTimeChartComponent({ [onFocusSeries, groupColumns, isSingleValueColumn, handleToggleSeries], ); - const toolbarItemsMemo = useMemo(() => { - const allToolbarItems = []; - - if (toolbarPrefix && toolbarPrefix.length > 0) { - allToolbarItems.push(...toolbarPrefix); - } - - if (source && showMVOptimizationIndicator && builderQueriedConfig) { - allToolbarItems.push( - , - ); - } - - const mvDateRange = mvOptimizationData?.optimizedConfig?.dateRange; - const isAlignedToChartGranularity = - queriedConfig.alignDateRangeToGranularity !== false; - - if ( - showDateRangeIndicator && - (mvDateRange || isAlignedToChartGranularity) - ) { - const mvGranularity = isAlignedToChartGranularity - ? undefined - : mvOptimizationData?.explanations.find(e => e.success)?.mvConfig - .minGranularity; - - allToolbarItems.push( - , - ); - } - - if (showDisplaySwitcher) { - allToolbarItems.push( - , - }, - { - value: DisplayType.StackedBar, - label: config.compareToPreviousPeriod - ? 'Bar Chart Unavailable When Comparing to Previous Period' - : 'Display as Bar Chart', - icon: , - disabled: config.compareToPreviousPeriod, - }, - ]} - />, - ); - } - - if (toolbarSuffix && toolbarSuffix.length > 0) { - allToolbarItems.push(...toolbarSuffix); - } - - return allToolbarItems; - }, [ + const toolbarItemsMemo = useChartToolbarItems({ builderQueriedConfig, config, displayType, handleSetDisplayType, + mvOptimizationData, + queriedConfig, + showDateRangeIndicator, showDisplaySwitcher, + showMVOptimizationIndicator, source, toolbarPrefix, toolbarSuffix, - showMVOptimizationIndicator, - showDateRangeIndicator, - mvOptimizationData, - queriedConfig, - ]); + }); return ( diff --git a/packages/app/src/components/__tests__/DBTimeChart.test.tsx b/packages/app/src/components/DBTimeChart/__tests__/DBTimeChart.test.tsx similarity index 98% rename from packages/app/src/components/__tests__/DBTimeChart.test.tsx rename to packages/app/src/components/DBTimeChart/__tests__/DBTimeChart.test.tsx index 1e86e1e60d..055849c387 100644 --- a/packages/app/src/components/__tests__/DBTimeChart.test.tsx +++ b/packages/app/src/components/DBTimeChart/__tests__/DBTimeChart.test.tsx @@ -39,11 +39,11 @@ jest.mock('@/source', () => ({ .mockReturnValue({ formatByColumn: new Map(), chartFormat: undefined }), })); -jest.mock('../MaterializedViews/MVOptimizationIndicator', () => +jest.mock('@/components/MaterializedViews/MVOptimizationIndicator', () => jest.fn(() => null), ); -jest.mock('../charts/DateRangeIndicator', () => jest.fn(() => null)); +jest.mock('@/components/charts/DateRangeIndicator', () => jest.fn(() => null)); describe('DBTimeChart', () => { const mockUseQueriedChartConfig = useQueriedChartConfig as jest.Mock; diff --git a/packages/app/src/components/DBTimeChart/__tests__/searchUrl.test.ts b/packages/app/src/components/DBTimeChart/__tests__/searchUrl.test.ts new file mode 100644 index 0000000000..57f2e17e62 --- /dev/null +++ b/packages/app/src/components/DBTimeChart/__tests__/searchUrl.test.ts @@ -0,0 +1,179 @@ +import { + type ChartConfigWithDateRange, + SourceKind, + TSource, +} from '@hyperdx/common-utils/dist/types'; + +import { buildSeriesSearchUrl } from '@/components/DBTimeChart/searchUrl'; + +// The URL string itself is ChartUtils' concern and already covered there. What +// matters here is the branching this function does before delegating — which was +// previously locked inside a useCallback and untestable. +type SearchUrlArgs = { + dateRange: [Date, Date]; + groupFilters: { column: string; value: string }[]; + valueRangeFilter?: { expression: string; value: number }; +}; + +const mockBuildEventsSearchUrl = jest.fn( + () => '/search?mocked=1', +); +jest.mock('@/ChartUtils', () => ({ + ...jest.requireActual('@/ChartUtils'), + buildEventsSearchUrl: (arg: SearchUrlArgs) => mockBuildEventsSearchUrl(arg), +})); + +/** Args the function handed to buildEventsSearchUrl on its first call. */ +const delegatedArgs = () => mockBuildEventsSearchUrl.mock.calls[0][0]; + +const source = { + id: 'src-1', + kind: SourceKind.Log, + name: 'logs', + connection: 'conn-1', + from: { databaseName: 'default', tableName: 'otel_logs' }, + timestampValueExpression: 'Timestamp', +} as unknown as TSource; + +const clicked = new Date('2026-01-01T00:00:00Z'); + +const args = { + clickedActiveLabelDate: clicked as Date | undefined, + source: source as TSource | undefined, + granularity: '1 minute', + groupColumns: [] as string[], + valueColumns: undefined as string[] | undefined, + isSingleValueColumn: true as boolean | undefined, +}; + +const configWith = ( + select: { aggFn: string; valueExpression: string }[], +): ChartConfigWithDateRange => + ({ + connection: 'conn-1', + from: { databaseName: 'default', tableName: 'otel_logs' }, + timestampValueExpression: 'Timestamp', + where: '', + select, + dateRange: [clicked, clicked], + }) as ChartConfigWithDateRange; + +const rawSqlConfig = { + connection: 'conn-1', + configType: 'sql', + sqlTemplate: 'SELECT 1', + dateRange: [clicked, clicked], +} satisfies Partial as ChartConfigWithDateRange; + +beforeEach(() => jest.clearAllMocks()); + +describe('buildSeriesSearchUrl', () => { + describe('returns null when drill-down cannot be supported', () => { + it('with no clicked date', () => { + expect( + buildSeriesSearchUrl({ + ...args, + clickedActiveLabelDate: undefined, + config: configWith([{ aggFn: 'avg', valueExpression: 'Duration' }]), + }), + ).toBeNull(); + expect(mockBuildEventsSearchUrl).not.toHaveBeenCalled(); + }); + + it('with no resolved source', () => { + expect( + buildSeriesSearchUrl({ + ...args, + source: undefined, + config: configWith([{ aggFn: 'avg', valueExpression: 'Duration' }]), + }), + ).toBeNull(); + }); + + it('for a raw SQL chart', () => { + // Raw SQL doesn't resolve to a single source, so there is nothing to search. + expect( + buildSeriesSearchUrl({ + ...args, + config: rawSqlConfig, + }), + ).toBeNull(); + }); + }); + + it('ranges from the clicked bucket to one granularity later', () => { + buildSeriesSearchUrl({ + ...args, + config: configWith([{ aggFn: 'avg', valueExpression: 'Duration' }]), + }); + const { dateRange } = delegatedArgs(); + expect(dateRange[0]).toEqual(clicked); + expect(dateRange[1].getTime() - clicked.getTime()).toBe(60_000); + }); + + describe('value-range filter', () => { + it('is added for an attributable aggregation', () => { + buildSeriesSearchUrl({ + ...args, + seriesValue: 250, + config: configWith([{ aggFn: 'max', valueExpression: 'Duration' }]), + }); + const { valueRangeFilter } = delegatedArgs(); + expect(valueRangeFilter).toEqual({ + expression: 'Duration', + value: 250, + }); + }); + + it('is omitted for a non-attributable aggregation', () => { + // A `count`/`sum` point is not attributable to any single event's value, so + // filtering on it would return rows that never contributed to the point. + buildSeriesSearchUrl({ + ...args, + seriesValue: 250, + config: configWith([{ aggFn: 'count', valueExpression: 'Duration' }]), + }); + const { valueRangeFilter } = delegatedArgs(); + expect(valueRangeFilter).toBeUndefined(); + }); + + it('is omitted when no series value was clicked', () => { + buildSeriesSearchUrl({ + ...args, + config: configWith([{ aggFn: 'max', valueExpression: 'Duration' }]), + }); + const { valueRangeFilter } = delegatedArgs(); + expect(valueRangeFilter).toBeUndefined(); + }); + + it('resolves the value column by series-key prefix on a multi-value chart', () => { + // With more than one value column the series key is prefixed with the + // column name, so the filter must follow that prefix to the right select + // item rather than defaulting to select[0]. + buildSeriesSearchUrl({ + ...args, + seriesKey: 'p95', + seriesValue: 900, + isSingleValueColumn: false, + valueColumns: ['count', 'p95'], + config: configWith([ + { aggFn: 'count', valueExpression: 'Body' }, + { aggFn: 'p95', valueExpression: 'Duration' }, + ]), + }); + const { valueRangeFilter } = delegatedArgs(); + expect(valueRangeFilter).toEqual({ expression: 'Duration', value: 900 }); + }); + }); + + it('passes decoded group filters through', () => { + buildSeriesSearchUrl({ + ...args, + seriesKey: 'api', + groupColumns: ['ServiceName'], + config: configWith([{ aggFn: 'avg', valueExpression: 'Duration' }]), + }); + const { groupFilters } = delegatedArgs(); + expect(groupFilters).toEqual([{ column: 'ServiceName', value: 'api' }]); + }); +}); diff --git a/packages/app/src/components/DBTimeChart/crossChartPin.ts b/packages/app/src/components/DBTimeChart/crossChartPin.ts new file mode 100644 index 0000000000..1fda2b8a08 --- /dev/null +++ b/packages/app/src/components/DBTimeChart/crossChartPin.ts @@ -0,0 +1,34 @@ +import { useCallback, useEffect, useId, useRef } from 'react'; + +// Only one pinned tooltip at a time across all charts. Module-level (not +// context) because charts can be scattered with no common provider, and their +// onClick stopPropagation hides cross-chart clicks from Mantine's click-outside. +const pinnedTooltipRegistry = new Map void>(); + +function broadcastTooltipPinned(activeId: string) { + pinnedTooltipRegistry.forEach((dismiss, id) => { + if (id !== activeId) { + dismiss(); + } + }); +} + +// Registers this chart's dismiss handler and returns a callback to close every +// other chart's pinned tooltip (call it when pinning this one). +export function useCrossChartPinDismiss(onDismiss: () => void): () => void { + const id = useId(); + // Keep the latest onDismiss without re-subscribing each render. + const onDismissRef = useRef(onDismiss); + useEffect(() => { + onDismissRef.current = onDismiss; + }, [onDismiss]); + + useEffect(() => { + pinnedTooltipRegistry.set(id, () => onDismissRef.current()); + return () => { + pinnedTooltipRegistry.delete(id); + }; + }, [id]); + + return useCallback(() => broadcastTooltipPinned(id), [id]); +} diff --git a/packages/app/src/components/DBTimeChart/index.ts b/packages/app/src/components/DBTimeChart/index.ts new file mode 100644 index 0000000000..80c60236a9 --- /dev/null +++ b/packages/app/src/components/DBTimeChart/index.ts @@ -0,0 +1,7 @@ +/** + * Public surface of the time-chart tile. Split out of a single 1000-line module; + * consumers (and the test files that `jest.mock('@/components/DBTimeChart')`) + * import from here, so the internal file layout stays free to change. + */ +export { DBTimeChart } from './DBTimeChart'; +export { decodeSeriesGroupFilters, type SeriesGroupFilter } from './searchUrl'; diff --git a/packages/app/src/components/DBTimeChart/searchUrl.ts b/packages/app/src/components/DBTimeChart/searchUrl.ts new file mode 100644 index 0000000000..65e65cce5d --- /dev/null +++ b/packages/app/src/components/DBTimeChart/searchUrl.ts @@ -0,0 +1,177 @@ +import { add } from 'date-fns'; +import { convertGranularityToSeconds } from '@hyperdx/common-utils/dist/core/utils'; +import { + isPromqlChartConfig, + isRawSqlChartConfig, +} from '@hyperdx/common-utils/dist/guards'; +import { + ChartConfigWithDateRange, + TSource, +} from '@hyperdx/common-utils/dist/types'; + +import { AGG_FNS, buildEventsSearchUrl, ChartKeyJoiner } from '@/ChartUtils'; + +export type SeriesGroupFilter = { column: string; value: string }; + +// Decode a Recharts series key (e.g. "count · error · api") into the +// underlying group-column filters. This is the same decode buildSeriesSearchUrl +// uses, extracted so the focus callback can hand the caller structured filters +// (rather than a display string) to apply to a sibling results list. +export function decodeSeriesGroupFilters({ + seriesKey, + groupColumns, + isSingleValueColumn, +}: { + seriesKey: string | undefined; + groupColumns: string[]; + isSingleValueColumn: boolean | undefined; +}): SeriesGroupFilter[] { + const seriesKeys = seriesKey?.split(ChartKeyJoiner); + const groupFilters: SeriesGroupFilter[] = []; + + if (seriesKeys?.length && groupColumns?.length) { + // When the series has multiple value columns, the key is prefixed with the + // value column name (e.g. "count · error"), so the group values start at + // index 1. (The "no group columns" case the original inline code also + // guarded is impossible here — this block only runs when groupColumns is + // non-empty.) + const startsWithValueColumn = !(isSingleValueColumn ?? true); + const groupValues = startsWithValueColumn + ? seriesKeys.slice(1) + : seriesKeys; + + groupValues.forEach((value, index) => { + if (groupColumns[index] != null) { + groupFilters.push({ column: groupColumns[index], value }); + } + }); + } + + return groupFilters; +} + +/** + * Build the drill-down search URL for a clicked point, or null when the chart + * cannot support it: raw SQL and PromQL charts don't resolve to a single source, + * and a click with no resolved date has no range to filter on. + * + * Pure, and extracted from a useCallback in the chart so the branching here can + * be read and tested without a render — in particular which value column a + * series key maps to, and whether that column's aggregation is attributable to + * individual events at all (a non-attributable agg must not produce a value + * filter, or the drill-down returns rows that never contributed to the point). + */ +export function buildSeriesSearchUrl({ + seriesKey, + seriesValue, + clickedActiveLabelDate, + source, + config, + granularity, + groupColumns, + valueColumns, + isSingleValueColumn, +}: { + seriesKey?: string; + seriesValue?: number; + clickedActiveLabelDate: Date | undefined; + source: TSource | undefined; + config: ChartConfigWithDateRange; + granularity: string; + groupColumns: string[]; + valueColumns: string[] | undefined; + isSingleValueColumn: boolean | undefined; +}): string | null { + // Raw SQL charts are not supported for drill-down as we don't know the source which is being used. + if ( + clickedActiveLabelDate == null || + source == null || + isRawSqlChartConfig(config) || + isPromqlChartConfig(config) + ) { + return null; + } + + // Parse the series key to extract group values + const seriesKeys = seriesKey?.split(ChartKeyJoiner); + const groupFilters = decodeSeriesGroupFilters({ + seriesKey, + groupColumns, + isSingleValueColumn, + }); + + // Build value range filter for Y-axis if provided + let valueRangeFilter: + | { + expression: string; + value: number; + } + | undefined; + + if (seriesValue && Array.isArray(config.select) && config.select.length > 0) { + // Determine which value column to filter on + let valueExpression: string | undefined; + + if ((isSingleValueColumn ?? true) && config.select.length === 1) { + const firstSelect = config.select[0]; + const aggFn = + typeof firstSelect === 'string' ? undefined : firstSelect.aggFn; + // Only add value range filter if the aggregation is attributable + const isAttributable = + AGG_FNS.find(fn => fn.value === aggFn)?.isAttributable !== false; + + if (isAttributable) { + valueExpression = + typeof firstSelect === 'string' + ? firstSelect + : firstSelect.valueExpression; + } + } else if (seriesKeys?.length && (valueColumns?.length ?? 0) > 0) { + const firstPart = seriesKeys[0]; + const valueColumnIndex = valueColumns?.findIndex( + col => col === firstPart, + ); + + if ( + valueColumnIndex != null && + valueColumnIndex >= 0 && + valueColumnIndex < config.select.length + ) { + const selectItem = config.select[valueColumnIndex]; + const aggFn = + typeof selectItem === 'string' ? undefined : selectItem.aggFn; + // Only add value range filter if the aggregation is attributable + const isAttributable = + AGG_FNS.find(fn => fn.value === aggFn)?.isAttributable !== false; + + if (isAttributable) { + valueExpression = + typeof selectItem === 'string' + ? selectItem + : selectItem.valueExpression; + } + } + } + + if (valueExpression) { + valueRangeFilter = { + expression: valueExpression, + value: seriesValue, + }; + } + } + + // Calculate time range from clicked date and granularity + const from = clickedActiveLabelDate; + const to = add(clickedActiveLabelDate, { + seconds: convertGranularityToSeconds(granularity), + }); + + return buildEventsSearchUrl({ + source, + config, + dateRange: [from, to], + groupFilters, + valueRangeFilter, + }); +} diff --git a/packages/app/src/components/DBTimeChart/useChartToolbarItems.tsx b/packages/app/src/components/DBTimeChart/useChartToolbarItems.tsx new file mode 100644 index 0000000000..a4fba76c94 --- /dev/null +++ b/packages/app/src/components/DBTimeChart/useChartToolbarItems.tsx @@ -0,0 +1,145 @@ +import React, { useMemo } from 'react'; +import { + type BuilderChartConfigWithDateRange, + type ChartConfigWithDateRange, + DisplayType, + type TSource, +} from '@hyperdx/common-utils/dist/types'; +import { IconChartBar, IconChartLine } from '@tabler/icons-react'; + +import DateRangeIndicator from '@/components/charts/DateRangeIndicator'; +import DisplaySwitcher from '@/components/charts/DisplaySwitcher'; +import MVOptimizationIndicator from '@/components/MaterializedViews/MVOptimizationIndicator'; + +type UseChartToolbarItemsArgs = { + builderQueriedConfig: BuilderChartConfigWithDateRange | undefined; + config: ChartConfigWithDateRange; + displayType: DisplayType | undefined; + handleSetDisplayType: (displayType: DisplayType) => void; + // Shape comes from useMVOptimizationExplanation; only these fields are read here. + mvOptimizationData: + | { + optimizedConfig?: { dateRange?: [Date, Date] }; + explanations: { + success: boolean; + mvConfig: { minGranularity?: string }; + }[]; + } + | undefined; + queriedConfig: ChartConfigWithDateRange; + showDateRangeIndicator: boolean; + showDisplaySwitcher: boolean; + showMVOptimizationIndicator: boolean; + source: TSource | undefined; + toolbarPrefix: React.ReactNode[] | undefined; + toolbarSuffix: React.ReactNode[] | undefined; +}; + +/** + * Assemble the chart's toolbar: caller-supplied prefix/suffix items plus the + * indicators the chart owns (materialized-view optimization, effective date + * range) and the display-type switcher. + * + * Extracted from DBTimeChart because it is a long, purely presentational list + * build with no bearing on the chart's data or interaction state. + */ +export function useChartToolbarItems({ + builderQueriedConfig, + config, + displayType, + handleSetDisplayType, + mvOptimizationData, + queriedConfig, + showDateRangeIndicator, + showDisplaySwitcher, + showMVOptimizationIndicator, + source, + toolbarPrefix, + toolbarSuffix, +}: UseChartToolbarItemsArgs) { + return useMemo(() => { + const allToolbarItems = []; + + if (toolbarPrefix && toolbarPrefix.length > 0) { + allToolbarItems.push(...toolbarPrefix); + } + + if (source && showMVOptimizationIndicator && builderQueriedConfig) { + allToolbarItems.push( + , + ); + } + + const mvDateRange = mvOptimizationData?.optimizedConfig?.dateRange; + const isAlignedToChartGranularity = + queriedConfig.alignDateRangeToGranularity !== false; + + if ( + showDateRangeIndicator && + (mvDateRange || isAlignedToChartGranularity) + ) { + const mvGranularity = isAlignedToChartGranularity + ? undefined + : mvOptimizationData?.explanations.find(e => e.success)?.mvConfig + .minGranularity; + + allToolbarItems.push( + , + ); + } + + if (showDisplaySwitcher) { + allToolbarItems.push( + , + }, + { + value: DisplayType.StackedBar, + label: config.compareToPreviousPeriod + ? 'Bar Chart Unavailable When Comparing to Previous Period' + : 'Display as Bar Chart', + icon: , + disabled: config.compareToPreviousPeriod, + }, + ]} + />, + ); + } + + if (toolbarSuffix && toolbarSuffix.length > 0) { + allToolbarItems.push(...toolbarSuffix); + } + + return allToolbarItems; + }, [ + builderQueriedConfig, + config, + displayType, + handleSetDisplayType, + showDisplaySwitcher, + source, + toolbarPrefix, + toolbarSuffix, + showMVOptimizationIndicator, + showDateRangeIndicator, + mvOptimizationData, + queriedConfig, + ]); +} From 5aac18d2c899d72899bfce7005c070c844a7dd9b Mon Sep 17 00:00:00 2001 From: Jordan Simonovski Date: Wed, 5 Aug 2026 22:51:41 +1000 Subject: [PATCH 2/3] fix(app): drill down on a clicked zero, and cover the extracted seams MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses review findings on the chart-file split. The one behaviour change: clicking a point whose value is exactly 0 dropped the value filter and searched every event in the bucket. The chart deliberately keeps zeroes rather than treating them as absent, so a truthiness guard was the wrong test. Deleted packages/app/packages/app/src/components/DBTimeChart/__tests__/ searchUrl.test.ts — a duplicate of the real test, created by a command that ran from the wrong directory. Jest never ran it, so it was free to drift while still costing tsc and eslint time. Tests for the two pure seams the split created and left uncovered: useChartScales (axis domains, the fit-to-data zero clamp, selection filtering, half-bucket bar padding, annotation clamping) and crossChartPin (the module-level registry that coordinates one pinned tooltip across charts). Also: restored a comment about the resize debounce that the move had cut in half across two files; ChartTooltipContent no longer maps over an undefined payload before its own guard, which used to latch the error boundary for the life of the instance; the pinned tooltip shares the hover tooltip's offset constant rather than repeating the literal; MemoChart's graphResults is typed as the hook already narrows it to; useChartToolbarItems derives its mvOptimizationData type from the hook instead of hand-copying the shape; buildSeriesSearchUrl is exported from the barrel it belongs to; and DBTimeChart.tsx imports its own siblings relatively, matching the other new directory. --- .changeset/timechart-drilldown-zero-value.md | 10 + .../DBTimeChart/__tests__/searchUrl.test.ts | 179 ------------------ .../ChartTooltipContent.tsx | 10 +- .../src/HDXMultiSeriesTimeChart/MemoChart.tsx | 5 +- .../__tests__/useChartScales.test.ts | 165 ++++++++++++++++ .../src/HDXMultiSeriesTimeChart/chartData.ts | 4 - .../app/src/HDXMultiSeriesTimeChart/index.ts | 1 + .../DBTimeChart/ChartTooltipOverlay.tsx | 9 +- .../components/DBTimeChart/DBTimeChart.tsx | 17 +- .../__tests__/crossChartPin.test.tsx | 69 +++++++ .../DBTimeChart/__tests__/searchUrl.test.ts | 36 +++- .../app/src/components/DBTimeChart/index.ts | 6 +- .../src/components/DBTimeChart/searchUrl.ts | 9 +- .../DBTimeChart/useChartToolbarItems.tsx | 14 +- 14 files changed, 326 insertions(+), 208 deletions(-) create mode 100644 .changeset/timechart-drilldown-zero-value.md delete mode 100644 packages/app/packages/app/src/components/DBTimeChart/__tests__/searchUrl.test.ts create mode 100644 packages/app/src/HDXMultiSeriesTimeChart/__tests__/useChartScales.test.ts create mode 100644 packages/app/src/components/DBTimeChart/__tests__/crossChartPin.test.tsx diff --git a/.changeset/timechart-drilldown-zero-value.md b/.changeset/timechart-drilldown-zero-value.md new file mode 100644 index 0000000000..0e24af0eb0 --- /dev/null +++ b/.changeset/timechart-drilldown-zero-value.md @@ -0,0 +1,10 @@ +--- +'@hyperdx/app': patch +--- + +fix: keep the drill-down value filter when the clicked point is zero + +Clicking a time-chart point with a value of exactly 0 dropped the value filter +and searched every event in the bucket instead of the matching ones. Zero is a +real point — the chart deliberately keeps it rather than treating it as absent — +so the guard now tests for a missing value rather than a falsy one. diff --git a/packages/app/packages/app/src/components/DBTimeChart/__tests__/searchUrl.test.ts b/packages/app/packages/app/src/components/DBTimeChart/__tests__/searchUrl.test.ts deleted file mode 100644 index 57f2e17e62..0000000000 --- a/packages/app/packages/app/src/components/DBTimeChart/__tests__/searchUrl.test.ts +++ /dev/null @@ -1,179 +0,0 @@ -import { - type ChartConfigWithDateRange, - SourceKind, - TSource, -} from '@hyperdx/common-utils/dist/types'; - -import { buildSeriesSearchUrl } from '@/components/DBTimeChart/searchUrl'; - -// The URL string itself is ChartUtils' concern and already covered there. What -// matters here is the branching this function does before delegating — which was -// previously locked inside a useCallback and untestable. -type SearchUrlArgs = { - dateRange: [Date, Date]; - groupFilters: { column: string; value: string }[]; - valueRangeFilter?: { expression: string; value: number }; -}; - -const mockBuildEventsSearchUrl = jest.fn( - () => '/search?mocked=1', -); -jest.mock('@/ChartUtils', () => ({ - ...jest.requireActual('@/ChartUtils'), - buildEventsSearchUrl: (arg: SearchUrlArgs) => mockBuildEventsSearchUrl(arg), -})); - -/** Args the function handed to buildEventsSearchUrl on its first call. */ -const delegatedArgs = () => mockBuildEventsSearchUrl.mock.calls[0][0]; - -const source = { - id: 'src-1', - kind: SourceKind.Log, - name: 'logs', - connection: 'conn-1', - from: { databaseName: 'default', tableName: 'otel_logs' }, - timestampValueExpression: 'Timestamp', -} as unknown as TSource; - -const clicked = new Date('2026-01-01T00:00:00Z'); - -const args = { - clickedActiveLabelDate: clicked as Date | undefined, - source: source as TSource | undefined, - granularity: '1 minute', - groupColumns: [] as string[], - valueColumns: undefined as string[] | undefined, - isSingleValueColumn: true as boolean | undefined, -}; - -const configWith = ( - select: { aggFn: string; valueExpression: string }[], -): ChartConfigWithDateRange => - ({ - connection: 'conn-1', - from: { databaseName: 'default', tableName: 'otel_logs' }, - timestampValueExpression: 'Timestamp', - where: '', - select, - dateRange: [clicked, clicked], - }) as ChartConfigWithDateRange; - -const rawSqlConfig = { - connection: 'conn-1', - configType: 'sql', - sqlTemplate: 'SELECT 1', - dateRange: [clicked, clicked], -} satisfies Partial as ChartConfigWithDateRange; - -beforeEach(() => jest.clearAllMocks()); - -describe('buildSeriesSearchUrl', () => { - describe('returns null when drill-down cannot be supported', () => { - it('with no clicked date', () => { - expect( - buildSeriesSearchUrl({ - ...args, - clickedActiveLabelDate: undefined, - config: configWith([{ aggFn: 'avg', valueExpression: 'Duration' }]), - }), - ).toBeNull(); - expect(mockBuildEventsSearchUrl).not.toHaveBeenCalled(); - }); - - it('with no resolved source', () => { - expect( - buildSeriesSearchUrl({ - ...args, - source: undefined, - config: configWith([{ aggFn: 'avg', valueExpression: 'Duration' }]), - }), - ).toBeNull(); - }); - - it('for a raw SQL chart', () => { - // Raw SQL doesn't resolve to a single source, so there is nothing to search. - expect( - buildSeriesSearchUrl({ - ...args, - config: rawSqlConfig, - }), - ).toBeNull(); - }); - }); - - it('ranges from the clicked bucket to one granularity later', () => { - buildSeriesSearchUrl({ - ...args, - config: configWith([{ aggFn: 'avg', valueExpression: 'Duration' }]), - }); - const { dateRange } = delegatedArgs(); - expect(dateRange[0]).toEqual(clicked); - expect(dateRange[1].getTime() - clicked.getTime()).toBe(60_000); - }); - - describe('value-range filter', () => { - it('is added for an attributable aggregation', () => { - buildSeriesSearchUrl({ - ...args, - seriesValue: 250, - config: configWith([{ aggFn: 'max', valueExpression: 'Duration' }]), - }); - const { valueRangeFilter } = delegatedArgs(); - expect(valueRangeFilter).toEqual({ - expression: 'Duration', - value: 250, - }); - }); - - it('is omitted for a non-attributable aggregation', () => { - // A `count`/`sum` point is not attributable to any single event's value, so - // filtering on it would return rows that never contributed to the point. - buildSeriesSearchUrl({ - ...args, - seriesValue: 250, - config: configWith([{ aggFn: 'count', valueExpression: 'Duration' }]), - }); - const { valueRangeFilter } = delegatedArgs(); - expect(valueRangeFilter).toBeUndefined(); - }); - - it('is omitted when no series value was clicked', () => { - buildSeriesSearchUrl({ - ...args, - config: configWith([{ aggFn: 'max', valueExpression: 'Duration' }]), - }); - const { valueRangeFilter } = delegatedArgs(); - expect(valueRangeFilter).toBeUndefined(); - }); - - it('resolves the value column by series-key prefix on a multi-value chart', () => { - // With more than one value column the series key is prefixed with the - // column name, so the filter must follow that prefix to the right select - // item rather than defaulting to select[0]. - buildSeriesSearchUrl({ - ...args, - seriesKey: 'p95', - seriesValue: 900, - isSingleValueColumn: false, - valueColumns: ['count', 'p95'], - config: configWith([ - { aggFn: 'count', valueExpression: 'Body' }, - { aggFn: 'p95', valueExpression: 'Duration' }, - ]), - }); - const { valueRangeFilter } = delegatedArgs(); - expect(valueRangeFilter).toEqual({ expression: 'Duration', value: 900 }); - }); - }); - - it('passes decoded group filters through', () => { - buildSeriesSearchUrl({ - ...args, - seriesKey: 'api', - groupColumns: ['ServiceName'], - config: configWith([{ aggFn: 'avg', valueExpression: 'Duration' }]), - }); - const { groupFilters } = delegatedArgs(); - expect(groupFilters).toEqual([{ column: 'ServiceName', value: 'api' }]); - }); -}); diff --git a/packages/app/src/HDXMultiSeriesTimeChart/ChartTooltipContent.tsx b/packages/app/src/HDXMultiSeriesTimeChart/ChartTooltipContent.tsx index 58497eea43..bc1ef76449 100644 --- a/packages/app/src/HDXMultiSeriesTimeChart/ChartTooltipContent.tsx +++ b/packages/app/src/HDXMultiSeriesTimeChart/ChartTooltipContent.tsx @@ -29,6 +29,9 @@ type HDXLineChartTooltipProps = { containerRef: React.MutableRefObject; } & Record; +/** Stable stand-in for a missing payload, so the memo below keeps its identity. */ +const EMPTY_PAYLOAD: TooltipPayload[] = []; + /** * The recharts `` content used for the HOVER tooltip (on the hovered * chart and its synced followers). Clicking pins ChartSeriesTooltip instead. @@ -51,7 +54,12 @@ export const HDXLineChartTooltip = withErrorBoundary( activePointYByKeyRef, containerRef, } = props; - const typedPayload = payload as TooltipPayload[]; + // recharts calls this with no payload when nothing is hovered, and the memo + // below runs before the `active && payload` guard. Mapping over undefined + // there throws inside the render, which withErrorBoundary latches for the + // rest of this instance's life — the chart never recovers. A shared constant + // rather than `?? []` so the memo's dependency stays referentially stable. + const typedPayload = (payload ?? EMPTY_PAYLOAD) as TooltipPayload[]; const tooltipZIndex = useChartTooltipZIndex(); diff --git a/packages/app/src/HDXMultiSeriesTimeChart/MemoChart.tsx b/packages/app/src/HDXMultiSeriesTimeChart/MemoChart.tsx index edbf9787e6..4fd8a77376 100644 --- a/packages/app/src/HDXMultiSeriesTimeChart/MemoChart.tsx +++ b/packages/app/src/HDXMultiSeriesTimeChart/MemoChart.tsx @@ -52,6 +52,7 @@ import { } from './constants'; import { useChartScales } from './useChartScales'; +// Debounce (ms) for the chart's ResponsiveContainer resize observer. Without // it the observer fires on every frame, and a resize → re-render → resize // cycle can keep the chart (and the form controls around it in the tile // editor) from ever settling. @@ -108,7 +109,9 @@ export const MemoChart = memo(function MemoChart({ dateRangeEndInclusive = true, fitYAxisToData = false, }: { - graphResults: any[]; + // Matches what useChartScales narrows to, so the hook's stricter type is + // actually checked at this boundary rather than satisfied by `any`. + graphResults: Record[]; setIsClickActive: (v: ActiveClickPayload | undefined) => void; isClickActive: ActiveClickPayload | undefined; dateRange: [Date, Date] | Readonly<[Date, Date]>; diff --git a/packages/app/src/HDXMultiSeriesTimeChart/__tests__/useChartScales.test.ts b/packages/app/src/HDXMultiSeriesTimeChart/__tests__/useChartScales.test.ts new file mode 100644 index 0000000000..a4b262a1be --- /dev/null +++ b/packages/app/src/HDXMultiSeriesTimeChart/__tests__/useChartScales.test.ts @@ -0,0 +1,165 @@ +import { DisplayType } from '@hyperdx/common-utils/dist/types'; +import { renderHook } from '@testing-library/react'; + +import { ChartAnnotation } from '@/components/charts/chartAnnotations'; +import { useChartScales } from '@/HDXMultiSeriesTimeChart/useChartScales'; + +/** + * The pure seam this refactor created: axis domains and annotation elements + * derived from props, with no state and no recharts tree. The branching that + * used to be buried in MemoChart — fit-to-data, legend selection, zero-anchored + * bars, the half-bucket bar padding — is what these cases pin. + */ +const lineData = [ + { dataKey: 'a', displayName: 'A' }, + { dataKey: 'b', displayName: 'B' }, +] as any; + +const baseArgs = { + annotations: undefined as ChartAnnotation[] | undefined, + dateRange: [ + new Date('2026-01-01T00:00:00Z'), + new Date('2026-01-01T01:00:00Z'), + ] as readonly [Date, Date], + granularity: '1 minute', + dateRangeEndInclusive: true, + displayType: DisplayType.Line, + fitYAxisToData: false, + graphResults: [ + { a: 10, b: 20 }, + { a: 30, b: 40 }, + ] as Record[], + lineData, + selectedSeriesNames: undefined as Set | undefined, +}; + +const scales = (overrides: Partial = {}) => + renderHook(() => useChartScales({ ...baseArgs, ...overrides })).result + .current; + +describe('useChartScales y-domain', () => { + it('lets recharts auto-scale from zero with no selection and no fit', () => { + expect(scales().yAxisDomain).toEqual([0, 'auto']); + }); + + it('fits the lower bound to the data minimum, less padding', () => { + // min 10, max 40 -> 5% padding is 1.5. + const [lower, upper] = scales({ fitYAxisToData: true }).yAxisDomain as [ + number, + number, + ]; + expect(lower).toBe(8.5); + expect(upper).toBe(41.5); + }); + + it('does not let the padding drag the axis below zero', () => { + // A chart of durations whose axis starts at -4ms reads as broken, so the + // clamp only lifts when the data itself never goes negative. + const [lower] = scales({ + fitYAxisToData: true, + graphResults: [{ a: 1, b: 100 }], + }).yAxisDomain as [number, number]; + expect(lower).toBe(0); + }); + + it('follows the data minimum when fitting and the data is negative', () => { + // min -50, max 25 -> 5% of the 75 range is 3.75, applied to both ends. + expect( + scales({ + fitYAxisToData: true, + graphResults: [ + { a: -50, b: -10 }, + { a: 0, b: 25 }, + ], + }).yAxisDomain, + ).toEqual([-53.75, 28.75]); + }); + + it('ignores deselected series when computing the range', () => { + // A alone spans 10..30, so padding is 1 and the domain is [9, 31]. With B + // included it would be [8.5, 41.5] — asserting the exact pair catches a + // wrong padding factor too, which a `toBeLessThan(40)` bound would not. + expect(scales({ selectedSeriesNames: new Set(['A']) }).yAxisDomain).toEqual( + [9, 31], + ); + }); + + it('bars stay anchored at zero even when fitting is requested', () => { + // Otherwise bar lengths stop being proportional to their values. + expect( + scales({ + fitYAxisToData: true, + displayType: DisplayType.StackedBar, + graphResults: [{ a: 100, b: 110 }], + }).yAxisDomain, + ).toEqual([0, 'auto']); + }); + + it('falls back to auto when no numeric values are present', () => { + expect( + scales({ + selectedSeriesNames: new Set(['A']), + graphResults: [{ a: null, b: 'x' }] as Record[], + }).yAxisDomain, + ).toEqual(['auto', 'auto']); + }); +}); + +describe('useChartScales x-domain', () => { + it('spans the requested range in seconds', () => { + const [start, end] = scales().xAxisDomain; + expect(end - start).toBe(3600); + }); + + it('drops the final bucket when the end is exclusive and boundary-aligned', () => { + // An exclusive end that lands exactly on a bucket boundary would otherwise + // render an extra empty bucket at the right edge. + const [, end] = scales({ dateRangeEndInclusive: false }).xAxisDomain; + const [, inclusiveEnd] = scales().xAxisDomain; + expect(inclusiveEnd - end).toBe(60); + }); + + it('pads both edges by half a bucket for bars so the full width fits', () => { + const [start, end] = scales({ + displayType: DisplayType.StackedBar, + }).xAxisDomain; + const [plainStart, plainEnd] = scales().xAxisDomain; + expect(plainStart - start).toBe(30); + expect(end - plainEnd).toBe(30); + }); +}); + +describe('useChartScales annotations', () => { + // `x` is the only thing worth asserting here: the element count is the same + // whatever time the annotation carries, so a test that only checks for a + // non-null result passes even when the time is read from the wrong field and + // every marker lands on NaN. + const markerX = (annotations: ChartAnnotation[]) => + (scales({ annotations }).annotationElements ?? []).map( + el => (el.props as { x: number }).x, + ); + + it('renders nothing when there are none', () => { + expect(scales().annotationElements).toBeNull(); + expect(scales({ annotations: [] }).annotationElements).toBeNull(); + }); + + it('places a marker at its own time, in the same unix seconds as the domain', () => { + const time = new Date('2026-01-01T00:30:00Z'); + expect(markerX([{ time, label: 'deploy' }])).toEqual([ + time.getTime() / 1000, + ]); + }); + + it('snaps a marker outside the window to the nearest edge', () => { + // An alert already firing when the window opens has a timestamp before the + // range; recharts drops such a marker outright, so it is clamped instead. + const [start, end] = scales().xAxisDomain; + expect( + markerX([ + { time: new Date('2025-12-25T00:00:00Z') }, + { time: new Date('2026-06-01T00:00:00Z') }, + ]), + ).toEqual([start, end]); + }); +}); diff --git a/packages/app/src/HDXMultiSeriesTimeChart/chartData.ts b/packages/app/src/HDXMultiSeriesTimeChart/chartData.ts index 4bf17e582f..f2c439c259 100644 --- a/packages/app/src/HDXMultiSeriesTimeChart/chartData.ts +++ b/packages/app/src/HDXMultiSeriesTimeChart/chartData.ts @@ -2,10 +2,6 @@ import { type LineData, MAX_TIME_CHART_SERIES } from '@/ChartUtils'; export const HARD_LINES_LIMIT = MAX_TIME_CHART_SERIES; -// Debounce (ms) for the chart's ResponsiveContainer resize observer. Without -// it the observer fires on every frame, and a resize → re-render → resize -// cycle can keep the chart (and the form controls around it in the tile - /** One series entry in a tooltip's per-bucket payload (hover or click-frozen). */ export type ActiveClickSeries = { value?: number; diff --git a/packages/app/src/HDXMultiSeriesTimeChart/index.ts b/packages/app/src/HDXMultiSeriesTimeChart/index.ts index f6dea6a6c5..d36c501c42 100644 --- a/packages/app/src/HDXMultiSeriesTimeChart/index.ts +++ b/packages/app/src/HDXMultiSeriesTimeChart/index.ts @@ -11,5 +11,6 @@ export { getVisibleLineData, HARD_LINES_LIMIT, } from './chartData'; +export { TOOLTIP_POINT_OFFSET_PX } from './constants'; export { collectMemoChartGradientHexes, MemoChart } from './MemoChart'; export { TooltipItem } from './TooltipItem'; diff --git a/packages/app/src/components/DBTimeChart/ChartTooltipOverlay.tsx b/packages/app/src/components/DBTimeChart/ChartTooltipOverlay.tsx index bfa1d82dfb..c48162269a 100644 --- a/packages/app/src/components/DBTimeChart/ChartTooltipOverlay.tsx +++ b/packages/app/src/components/DBTimeChart/ChartTooltipOverlay.tsx @@ -4,7 +4,10 @@ import { Popover, Portal } from '@mantine/core'; import { ChartSeriesTooltip } from '@/components/charts/ChartSeriesTooltip'; import { useChartTooltipZIndex } from '@/components/charts/ChartTooltip'; -import type { ActiveClickPayload } from '@/HDXMultiSeriesTimeChart'; +import { + type ActiveClickPayload, + TOOLTIP_POINT_OFFSET_PX, +} from '@/HDXMultiSeriesTimeChart'; // The interactive PINNED tooltip, rendered over the chart in a body-portaled // Mantine Popover anchored at the clicked point. Hover uses the recharts tooltip @@ -105,7 +108,9 @@ export function ChartTooltipOverlay({ trapFocus={false} withinPortal position="bottom" - offset={12} + // Same gap the hover tooltip uses, so the two states don't sit at + // different distances from the point. + offset={TOOLTIP_POINT_OFFSET_PX} middlewares={{ flip: true, shift: true }} returnFocus={false} zIndex={popoverZIndex} diff --git a/packages/app/src/components/DBTimeChart/DBTimeChart.tsx b/packages/app/src/components/DBTimeChart/DBTimeChart.tsx index ec70bc7772..63b6fef370 100644 --- a/packages/app/src/components/DBTimeChart/DBTimeChart.tsx +++ b/packages/app/src/components/DBTimeChart/DBTimeChart.tsx @@ -21,19 +21,20 @@ import ChartContainer from '@/components/charts/ChartContainer'; import ChartErrorState, { ChartErrorStateVariant, } from '@/components/charts/ChartErrorState'; -import { ChartTooltipOverlay } from '@/components/DBTimeChart/ChartTooltipOverlay'; -import { useCrossChartPinDismiss } from '@/components/DBTimeChart/crossChartPin'; -import { - buildSeriesSearchUrl, - decodeSeriesGroupFilters, - type SeriesGroupFilter, -} from '@/components/DBTimeChart/searchUrl'; -import { useChartToolbarItems } from '@/components/DBTimeChart/useChartToolbarItems'; import { type ActiveClickPayload, MemoChart } from '@/HDXMultiSeriesTimeChart'; import { useQueriedChartConfig } from '@/hooks/useChartConfig'; import { useMVOptimizationExplanation } from '@/hooks/useMVOptimizationExplanation'; import { useChartNumberFormats, useSource } from '@/source'; +import { ChartTooltipOverlay } from './ChartTooltipOverlay'; +import { useCrossChartPinDismiss } from './crossChartPin'; +import { + buildSeriesSearchUrl, + decodeSeriesGroupFilters, + type SeriesGroupFilter, +} from './searchUrl'; +import { useChartToolbarItems } from './useChartToolbarItems'; + type DBTimeChartComponentProps = { config: ChartConfigWithDateRange; disableQueryChunking?: boolean; diff --git a/packages/app/src/components/DBTimeChart/__tests__/crossChartPin.test.tsx b/packages/app/src/components/DBTimeChart/__tests__/crossChartPin.test.tsx new file mode 100644 index 0000000000..03273a7396 --- /dev/null +++ b/packages/app/src/components/DBTimeChart/__tests__/crossChartPin.test.tsx @@ -0,0 +1,69 @@ +import { renderHook } from '@testing-library/react'; + +import { useCrossChartPinDismiss } from '@/components/DBTimeChart/crossChartPin'; + +/** + * The registry backing this hook is module-level and survives renders, unmounts + * and every test in this file's worker — deliberately, because charts can be + * scattered with no common provider. That makes leaks between consumers a real + * failure mode rather than a theoretical one, so the cases below cover both + * directions: who gets dismissed, and who stops being reachable. + */ +describe('useCrossChartPinDismiss', () => { + it('dismisses the other chart but not the one doing the pinning', () => { + const dismissA = jest.fn(); + const dismissB = jest.fn(); + + const a = renderHook(() => useCrossChartPinDismiss(dismissA)); + renderHook(() => useCrossChartPinDismiss(dismissB)); + + a.result.current(); + + expect(dismissB).toHaveBeenCalledTimes(1); + expect(dismissA).not.toHaveBeenCalled(); + }); + + it('stops calling a consumer once it unmounts', () => { + const dismissA = jest.fn(); + const dismissB = jest.fn(); + + const a = renderHook(() => useCrossChartPinDismiss(dismissA)); + const b = renderHook(() => useCrossChartPinDismiss(dismissB)); + + b.unmount(); + a.result.current(); + + // A stale entry here would call into an unmounted component's setState on + // every pin, for the life of the page. + expect(dismissB).not.toHaveBeenCalled(); + }); + + it('calls the latest callback, not the one from the first render', () => { + const first = jest.fn(); + const second = jest.fn(); + + const a = renderHook(() => useCrossChartPinDismiss(jest.fn())); + const b = renderHook(({ cb }) => useCrossChartPinDismiss(cb), { + initialProps: { cb: first }, + }); + + b.rerender({ cb: second }); + a.result.current(); + + expect(second).toHaveBeenCalledTimes(1); + expect(first).not.toHaveBeenCalled(); + }); + + it('dismisses every other consumer, not just one', () => { + const dismissA = jest.fn(); + const others = [jest.fn(), jest.fn(), jest.fn()]; + + const a = renderHook(() => useCrossChartPinDismiss(dismissA)); + others.forEach(cb => renderHook(() => useCrossChartPinDismiss(cb))); + + a.result.current(); + + others.forEach(cb => expect(cb).toHaveBeenCalledTimes(1)); + expect(dismissA).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/app/src/components/DBTimeChart/__tests__/searchUrl.test.ts b/packages/app/src/components/DBTimeChart/__tests__/searchUrl.test.ts index 57f2e17e62..04a9355f13 100644 --- a/packages/app/src/components/DBTimeChart/__tests__/searchUrl.test.ts +++ b/packages/app/src/components/DBTimeChart/__tests__/searchUrl.test.ts @@ -4,7 +4,7 @@ import { TSource, } from '@hyperdx/common-utils/dist/types'; -import { buildSeriesSearchUrl } from '@/components/DBTimeChart/searchUrl'; +import { buildSeriesSearchUrl } from '@/components/DBTimeChart'; // The URL string itself is ChartUtils' concern and already covered there. What // matters here is the branching this function does before delegating — which was @@ -65,6 +65,13 @@ const rawSqlConfig = { dateRange: [clicked, clicked], } satisfies Partial as ChartConfigWithDateRange; +const promqlConfig = { + connection: 'conn-1', + configType: 'promql', + promqlExpression: 'up', + dateRange: [clicked, clicked], +} satisfies Partial as ChartConfigWithDateRange; + beforeEach(() => jest.clearAllMocks()); describe('buildSeriesSearchUrl', () => { @@ -99,6 +106,17 @@ describe('buildSeriesSearchUrl', () => { }), ).toBeNull(); }); + + it('for a PromQL chart', () => { + // Same reason as raw SQL, and covered separately because the two share one + // condition — without this, dropping the isPromqlChartConfig arm stays green. + expect( + buildSeriesSearchUrl({ + ...args, + config: promqlConfig, + }), + ).toBeNull(); + }); }); it('ranges from the clicked bucket to one granularity later', () => { @@ -146,6 +164,22 @@ describe('buildSeriesSearchUrl', () => { expect(valueRangeFilter).toBeUndefined(); }); + it('is added for a clicked value of zero', () => { + // Zero is a real point — buildActiveClickSeries keeps it rather than + // dropping it — so a truthiness guard here would silently widen the + // drill-down to every event in the bucket. + buildSeriesSearchUrl({ + ...args, + seriesValue: 0, + config: configWith([{ aggFn: 'max', valueExpression: 'Duration' }]), + }); + const { valueRangeFilter } = delegatedArgs(); + expect(valueRangeFilter).toEqual({ + expression: 'Duration', + value: 0, + }); + }); + it('resolves the value column by series-key prefix on a multi-value chart', () => { // With more than one value column the series key is prefixed with the // column name, so the filter must follow that prefix to the right select diff --git a/packages/app/src/components/DBTimeChart/index.ts b/packages/app/src/components/DBTimeChart/index.ts index 80c60236a9..45a76286ed 100644 --- a/packages/app/src/components/DBTimeChart/index.ts +++ b/packages/app/src/components/DBTimeChart/index.ts @@ -4,4 +4,8 @@ * import from here, so the internal file layout stays free to change. */ export { DBTimeChart } from './DBTimeChart'; -export { decodeSeriesGroupFilters, type SeriesGroupFilter } from './searchUrl'; +export { + buildSeriesSearchUrl, + decodeSeriesGroupFilters, + type SeriesGroupFilter, +} from './searchUrl'; diff --git a/packages/app/src/components/DBTimeChart/searchUrl.ts b/packages/app/src/components/DBTimeChart/searchUrl.ts index 65e65cce5d..8badb11e6e 100644 --- a/packages/app/src/components/DBTimeChart/searchUrl.ts +++ b/packages/app/src/components/DBTimeChart/searchUrl.ts @@ -108,7 +108,14 @@ export function buildSeriesSearchUrl({ } | undefined; - if (seriesValue && Array.isArray(config.select) && config.select.length > 0) { + // `!= null`, not truthiness: a clicked value of exactly 0 is a real point — + // buildActiveClickSeries preserves zeroes — and skipping the filter for it + // drills down to every event in the bucket instead of the matching ones. + if ( + seriesValue != null && + Array.isArray(config.select) && + config.select.length > 0 + ) { // Determine which value column to filter on let valueExpression: string | undefined; diff --git a/packages/app/src/components/DBTimeChart/useChartToolbarItems.tsx b/packages/app/src/components/DBTimeChart/useChartToolbarItems.tsx index a4fba76c94..f17580645c 100644 --- a/packages/app/src/components/DBTimeChart/useChartToolbarItems.tsx +++ b/packages/app/src/components/DBTimeChart/useChartToolbarItems.tsx @@ -10,22 +10,16 @@ import { IconChartBar, IconChartLine } from '@tabler/icons-react'; import DateRangeIndicator from '@/components/charts/DateRangeIndicator'; import DisplaySwitcher from '@/components/charts/DisplaySwitcher'; import MVOptimizationIndicator from '@/components/MaterializedViews/MVOptimizationIndicator'; +import { useMVOptimizationExplanation } from '@/hooks/useMVOptimizationExplanation'; type UseChartToolbarItemsArgs = { builderQueriedConfig: BuilderChartConfigWithDateRange | undefined; config: ChartConfigWithDateRange; displayType: DisplayType | undefined; handleSetDisplayType: (displayType: DisplayType) => void; - // Shape comes from useMVOptimizationExplanation; only these fields are read here. - mvOptimizationData: - | { - optimizedConfig?: { dateRange?: [Date, Date] }; - explanations: { - success: boolean; - mvConfig: { minGranularity?: string }; - }[]; - } - | undefined; + // Derived from the hook rather than hand-copied, so a change to its shape is + // a type error here instead of a field that quietly stops being read. + mvOptimizationData: ReturnType['data']; queriedConfig: ChartConfigWithDateRange; showDateRangeIndicator: boolean; showDisplaySwitcher: boolean; From b7799efe793d993625e006f370a8ecaa7b5e8991 Mon Sep 17 00:00:00 2001 From: Jordan Simonovski Date: Thu, 6 Aug 2026 08:01:37 +1000 Subject: [PATCH 3/3] fix(app): type the useChartScales test fixtures instead of asserting Main ratcheted the app warning ceiling from 740 to 663, and the four assertions this test file used to sidestep typing put the branch over it. Typed the LineData fixture properly, asserted the whole y-domain rather than destructuring a cast, and narrowed the annotation elements with isValidElement so a rename of `x` would be a type error rather than a silently passing test. --- .../__tests__/useChartScales.test.ts | 45 +++++++++++-------- 1 file changed, 26 insertions(+), 19 deletions(-) diff --git a/packages/app/src/HDXMultiSeriesTimeChart/__tests__/useChartScales.test.ts b/packages/app/src/HDXMultiSeriesTimeChart/__tests__/useChartScales.test.ts index a4b262a1be..6e976a133f 100644 --- a/packages/app/src/HDXMultiSeriesTimeChart/__tests__/useChartScales.test.ts +++ b/packages/app/src/HDXMultiSeriesTimeChart/__tests__/useChartScales.test.ts @@ -1,6 +1,8 @@ +import { isValidElement } from 'react'; import { DisplayType } from '@hyperdx/common-utils/dist/types'; import { renderHook } from '@testing-library/react'; +import { type LineData } from '@/ChartUtils'; import { ChartAnnotation } from '@/components/charts/chartAnnotations'; import { useChartScales } from '@/HDXMultiSeriesTimeChart/useChartScales'; @@ -10,10 +12,16 @@ import { useChartScales } from '@/HDXMultiSeriesTimeChart/useChartScales'; * used to be buried in MemoChart — fit-to-data, legend selection, zero-anchored * bars, the half-bucket bar padding — is what these cases pin. */ -const lineData = [ - { dataKey: 'a', displayName: 'A' }, - { dataKey: 'b', displayName: 'B' }, -] as any; +const series = (dataKey: string, displayName: string): LineData => ({ + dataKey, + displayName, + currentPeriodKey: dataKey, + previousPeriodKey: `${dataKey}-prev`, + valueColumnName: dataKey, + color: '#000000', +}); + +const lineData: LineData[] = [series('a', 'A'), series('b', 'B')]; const baseArgs = { annotations: undefined as ChartAnnotation[] | undefined, @@ -44,22 +52,19 @@ describe('useChartScales y-domain', () => { it('fits the lower bound to the data minimum, less padding', () => { // min 10, max 40 -> 5% padding is 1.5. - const [lower, upper] = scales({ fitYAxisToData: true }).yAxisDomain as [ - number, - number, - ]; - expect(lower).toBe(8.5); - expect(upper).toBe(41.5); + expect(scales({ fitYAxisToData: true }).yAxisDomain).toEqual([8.5, 41.5]); }); it('does not let the padding drag the axis below zero', () => { - // A chart of durations whose axis starts at -4ms reads as broken, so the - // clamp only lifts when the data itself never goes negative. - const [lower] = scales({ - fitYAxisToData: true, - graphResults: [{ a: 1, b: 100 }], - }).yAxisDomain as [number, number]; - expect(lower).toBe(0); + // min 1, max 100 -> padding 4.95 would put the floor at -3.95, and a chart of + // durations whose axis starts below zero reads as broken. The clamp only + // lifts when the data itself goes negative. + expect( + scales({ + fitYAxisToData: true, + graphResults: [{ a: 1, b: 100 }], + }).yAxisDomain, + ).toEqual([0, 104.95]); }); it('follows the data minimum when fitting and the data is negative', () => { @@ -135,8 +140,10 @@ describe('useChartScales annotations', () => { // non-null result passes even when the time is read from the wrong field and // every marker lands on NaN. const markerX = (annotations: ChartAnnotation[]) => - (scales({ annotations }).annotationElements ?? []).map( - el => (el.props as { x: number }).x, + (scales({ annotations }).annotationElements ?? []).map(el => + // A guard rather than a cast: the elements come back as ReactElement with + // unknown props, and asserting the shape would hide a rename of `x`. + isValidElement<{ x: number }>(el) ? el.props.x : undefined, ); it('renders nothing when there are none', () => {