Skip to content

Commit a258fcf

Browse files
alex-fedotyevalex-fedotyev
authored
fix(dashboards): match the number-tile background sparkline to the displayed value (#2501)
Follow-up to #2489. The background trend sparkline that #2489 added to number tiles can plot the wrong data when a tile carries a leftover `groupBy`. ## Summary The big number on a number tile is a single aggregate: `convertToNumberChartConfig` strips `groupBy` from its query. The background sparkline builds its own query from the tile config but kept `groupBy`, so a tile that still carried one queried multiple series and the renderer plotted only the first. The faint trend behind the value then belonged to a single group while the value itself aggregated every group. A residual `groupBy` is reachable in normal use: switching a grouped Line chart to the Number display type only hides the group-by input, it does not clear the value, so a saved Number tile can still carry it. The fix extracts the sparkline's query derivation into a small pure helper, `buildSparklineTimeConfig`, and drops `groupBy` there, mirroring the value query's strip. `granularity` is still kept (defaulting to `auto`) so the trend stays bucketed. There is no visual or layout change: the sparkline renders exactly as before, it just plots the same single series as the value it sits behind. ## Changes - Extract `buildSparklineTimeConfig(config)` from the inline `useMemo` in `NumberTileBackgroundChart` and strip `groupBy` alongside the existing display-only fields. - Add a render test that asserts the issued query has no `groupBy`, plus unit tests for the helper. - Patch changeset. ## Test plan - [x] `nx run @hyperdx/app:ci:lint` (lint + tsc) - [x] `nx run @hyperdx/app:ci:unit` (2069 passed, 0 failures) - [x] New render test mounts `NumberTileBackgroundChart` with a tile that has a residual `groupBy` and asserts the config passed to `useQueriedChartConfig` drops it, exercising the real `buildSparklineTimeConfig` then `convertToTimeChartConfig` path rather than the helper in isolation. [ui-check: allow] Data-correctness fix: it changes which series the sparkline queries, not how anything is drawn. There is no visual, layout, color, or state surface to capture, and the new render test proves the issued query is correct. [viewport: allow] No layout change.
1 parent 34a8559 commit a258fcf

3 files changed

Lines changed: 142 additions & 23 deletions

File tree

‎.changeset/number-tile-sparkline-groupby.md‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
---
2+
"@hyperdx/app": patch
3+
---
4+
5+
fix(dashboards): match the number-tile background sparkline to the displayed value
6+
7+
The big number on a number tile is a single aggregate (its query drops `groupBy`), but the background sparkline kept any `groupBy` the tile carried over from a prior Line display type. It then plotted only the first group's trend behind a value that aggregates every group. The sparkline now drops `groupBy` as well, so its trend reflects the same single series as the value it sits behind.

‎packages/app/src/components/NumberTileBackgroundChart.tsx‎

Lines changed: 39 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -70,35 +70,52 @@ export function sparklinePointsFromGraphResults(
7070
return points;
7171
}
7272

73+
/**
74+
* Derive the sparkline's time-series query config from a number tile's config.
75+
*
76+
* The big number strips both `granularity` and `groupBy`
77+
* (`convertToNumberChartConfig`), collapsing the query to a single aggregate.
78+
* The sparkline must plot that same single series, so it strips `groupBy` too;
79+
* otherwise a tile carrying a residual `groupBy` (left over from a prior Line
80+
* display type) would query multiple series, and the renderer plots only the
81+
* first, which would not match the value. `granularity` is kept (auto when
82+
* unset) to recover the temporal trend behind the value.
83+
*
84+
* Number-tile display-only fields are dropped as well: they flow into the
85+
* query key (via `convertToTimeChartConfig`), so leaving them in would refetch
86+
* identical time-series data on every purely visual edit (sparkline type, tile
87+
* color, color rules, number format). Exported for unit testing.
88+
*/
89+
export function buildSparklineTimeConfig(
90+
config: ChartConfigWithDateRange,
91+
): ChartConfigWithDateRange {
92+
const {
93+
backgroundChart: _backgroundChart,
94+
color: _color,
95+
colorRules: _colorRules,
96+
numberFormat: _numberFormat,
97+
...rest
98+
} = config;
99+
const timeConfig: ChartConfigWithDateRange = {
100+
...rest,
101+
displayType: DisplayType.Line,
102+
granularity: config.granularity ?? 'auto',
103+
};
104+
// `groupBy` exists only on builder configs, so drop it under the guard.
105+
if (isBuilderChartConfig(timeConfig)) {
106+
delete timeConfig.groupBy;
107+
}
108+
return timeConfig;
109+
}
110+
73111
function NumberTileBackgroundChartInner({
74112
config,
75113
backgroundChart,
76114
}: {
77115
config: ChartConfigWithDateRange;
78116
backgroundChart: BackgroundChart;
79117
}) {
80-
// Number tiles strip granularity / group-by (`convertToNumberChartConfig`),
81-
// collapsing the query to a single value. Request a line series at the
82-
// tile's granularity (auto when unset) to recover the temporal trend.
83-
//
84-
// Drop number-tile display-only fields first: they flow into the query key
85-
// (via `convertToTimeChartConfig`), so leaving them in would refetch
86-
// identical time-series data on every purely visual edit (sparkline type,
87-
// tile color, number format).
88-
const timeConfig = useMemo<ChartConfigWithDateRange>(() => {
89-
const {
90-
backgroundChart: _backgroundChart,
91-
color: _color,
92-
colorRules: _colorRules,
93-
numberFormat: _numberFormat,
94-
...rest
95-
} = config;
96-
return {
97-
...rest,
98-
displayType: DisplayType.Line,
99-
granularity: config.granularity ?? 'auto',
100-
};
101-
}, [config]);
118+
const timeConfig = useMemo(() => buildSparklineTimeConfig(config), [config]);
102119

103120
const { dateRange, granularity, fillNulls } =
104121
useTimeChartSettings(timeConfig);

‎packages/app/src/components/__tests__/NumberTileBackgroundChart.test.tsx‎

Lines changed: 96 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,20 @@
1-
import { sparklinePointsFromGraphResults } from '@/components/NumberTileBackgroundChart';
1+
import React from 'react';
2+
import { DisplayType } from '@hyperdx/common-utils/dist/types';
3+
4+
import NumberTileBackgroundChart, {
5+
buildSparklineTimeConfig,
6+
sparklinePointsFromGraphResults,
7+
} from '@/components/NumberTileBackgroundChart';
8+
import { useQueriedChartConfig } from '@/hooks/useChartConfig';
9+
import { useSource } from '@/source';
10+
11+
jest.mock('@/hooks/useChartConfig', () => ({
12+
useQueriedChartConfig: jest.fn(),
13+
}));
14+
15+
jest.mock('@/source', () => ({
16+
useSource: jest.fn(),
17+
}));
218

319
describe('sparklinePointsFromGraphResults', () => {
420
const ts = '__hdx_time_bucket';
@@ -55,3 +71,82 @@ describe('sparklinePointsFromGraphResults', () => {
5571
]);
5672
});
5773
});
74+
75+
describe('buildSparklineTimeConfig', () => {
76+
const baseConfig = {
77+
dateRange: [
78+
new Date('2024-01-01T00:00:00Z'),
79+
new Date('2024-01-01T01:00:00Z'),
80+
] as [Date, Date],
81+
from: { databaseName: 'test', tableName: 'test' },
82+
timestampValueExpression: 'timestamp',
83+
connection: 'test-connection',
84+
select: '',
85+
where: '',
86+
};
87+
88+
it('drops groupBy and the display-only fields, keeps granularity, and forces a Line display type', () => {
89+
const result = buildSparklineTimeConfig({
90+
...baseConfig,
91+
granularity: '5 minute',
92+
groupBy: 'ServiceName',
93+
backgroundChart: { type: 'area' as const },
94+
color: 'chart-success' as const,
95+
colorRules: [
96+
{ operator: 'gte' as const, value: 1, color: 'chart-error' as const },
97+
],
98+
numberFormat: { output: 'percent' as const, mantissa: 2 },
99+
});
100+
101+
expect(result).not.toHaveProperty('groupBy');
102+
expect(result).not.toHaveProperty('backgroundChart');
103+
expect(result).not.toHaveProperty('color');
104+
expect(result).not.toHaveProperty('colorRules');
105+
expect(result).not.toHaveProperty('numberFormat');
106+
expect(result.granularity).toBe('5 minute');
107+
expect(result.displayType).toBe(DisplayType.Line);
108+
});
109+
110+
it("defaults granularity to 'auto' when the tile has none", () => {
111+
const result = buildSparklineTimeConfig(baseConfig);
112+
expect(result.granularity).toBe('auto');
113+
expect(result.displayType).toBe(DisplayType.Line);
114+
});
115+
});
116+
117+
describe('NumberTileBackgroundChart', () => {
118+
const mockUseQueriedChartConfig = useQueriedChartConfig as jest.Mock;
119+
120+
beforeEach(() => {
121+
jest.clearAllMocks();
122+
mockUseQueriedChartConfig.mockReturnValue({ data: undefined });
123+
(useSource as jest.Mock).mockReturnValue({ data: null });
124+
});
125+
126+
it('strips groupBy from the issued query so the sparkline matches the single displayed aggregate', () => {
127+
const config = {
128+
dateRange: [
129+
new Date('2024-01-01T00:00:00Z'),
130+
new Date('2024-01-01T01:00:00Z'),
131+
] as [Date, Date],
132+
from: { databaseName: 'test', tableName: 'test' },
133+
timestampValueExpression: 'timestamp',
134+
connection: 'test-connection',
135+
select: '',
136+
where: '',
137+
groupBy: 'ServiceName',
138+
backgroundChart: { type: 'area' as const },
139+
};
140+
141+
renderWithMantine(
142+
<NumberTileBackgroundChart
143+
config={config}
144+
backgroundChart={{ type: 'area' }}
145+
/>,
146+
);
147+
148+
expect(mockUseQueriedChartConfig).toHaveBeenCalled();
149+
const queriedConfig = mockUseQueriedChartConfig.mock.calls[0][0];
150+
expect(queriedConfig).not.toHaveProperty('groupBy');
151+
});
152+
});

0 commit comments

Comments
 (0)