Skip to content

Commit 141b496

Browse files
authored
fix: Correctly disable previous period query (#1528)
Closes HDX-3118 # Summary This PR ensures that "previous period" queries are not run when `compareToPreviousPeriod` is undefined. Previously, these queries were running unnecessarily, increasing the burden on ClickHouse.
1 parent 5062d80 commit 141b496

File tree

3 files changed

+133
-1
lines changed

3 files changed

+133
-1
lines changed

.changeset/olive-suns-repair.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@hyperdx/app": patch
3+
---
4+
5+
fix: Correctly disable previous period query

packages/app/src/components/DBTimeChart.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -285,7 +285,7 @@ function DBTimeChartComponent({
285285
useQueriedChartConfig(previousPeriodChartConfig, {
286286
placeholderData: (prev: any) => prev,
287287
queryKey: [queryKeyPrefix, previousPeriodChartConfig, 'chunked'],
288-
enabled: enabled && config.compareToPreviousPeriod,
288+
enabled: !!(enabled && config.compareToPreviousPeriod),
289289
enableQueryChunking: true,
290290
});
291291

Lines changed: 127 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,127 @@
1+
import React from 'react';
2+
3+
import api from '@/api';
4+
import { useQueriedChartConfig } from '@/hooks/useChartConfig';
5+
import { useSource } from '@/source';
6+
7+
import { DBTimeChart } from '../DBTimeChart';
8+
9+
// Mock dependencies
10+
jest.mock('@/hooks/useChartConfig', () => ({
11+
useQueriedChartConfig: jest.fn(),
12+
}));
13+
14+
jest.mock('@/api', () => ({
15+
__esModule: true,
16+
default: {
17+
useMe: jest.fn(),
18+
},
19+
}));
20+
21+
jest.mock('@/source', () => ({
22+
useSource: jest.fn(),
23+
}));
24+
25+
describe('DBTimeChart', () => {
26+
const mockUseQueriedChartConfig = useQueriedChartConfig as jest.Mock;
27+
const mockUseMe = api.useMe as jest.Mock;
28+
const mockUseSource = useSource as jest.Mock;
29+
30+
const baseTestConfig = {
31+
dateRange: [new Date('2024-01-01'), new Date('2024-01-02')] as [Date, Date],
32+
from: { databaseName: 'test', tableName: 'test' },
33+
timestampValueExpression: 'timestamp',
34+
connection: 'test-connection',
35+
select: 'value',
36+
where: '',
37+
};
38+
39+
beforeEach(() => {
40+
jest.clearAllMocks();
41+
42+
mockUseMe.mockReturnValue({
43+
data: { team: { parallelizeWhenPossible: false } },
44+
isLoading: false,
45+
});
46+
47+
mockUseSource.mockReturnValue({
48+
data: undefined,
49+
isLoading: false,
50+
});
51+
52+
mockUseQueriedChartConfig.mockReturnValue({
53+
data: {
54+
data: [{ timestamp: 1704067200, value: 100 }],
55+
meta: [],
56+
rows: 1,
57+
isComplete: true,
58+
},
59+
isLoading: false,
60+
isError: false,
61+
isSuccess: true,
62+
isPlaceholderData: false,
63+
});
64+
});
65+
66+
it('passes enabled: false to useQueriedChartConfig for previous period when compareToPreviousPeriod is undefined', () => {
67+
const config = {
68+
...baseTestConfig,
69+
compareToPreviousPeriod: undefined,
70+
};
71+
72+
renderWithMantine(<DBTimeChart config={config} />);
73+
74+
// Get the second call (previous period query)
75+
const [, secondCallOptions] = mockUseQueriedChartConfig.mock.calls[1];
76+
77+
// Verify that enabled is false for the previous period query
78+
expect(secondCallOptions.enabled).toBe(false);
79+
});
80+
81+
it('passes enabled: true to useQueriedChartConfig for previous period when compareToPreviousPeriod is true', () => {
82+
const config = {
83+
...baseTestConfig,
84+
compareToPreviousPeriod: true,
85+
};
86+
87+
renderWithMantine(<DBTimeChart config={config} />);
88+
89+
// Get the second call (previous period query)
90+
const [, secondCallOptions] = mockUseQueriedChartConfig.mock.calls[1];
91+
92+
// Verify that enabled is true for the previous period query
93+
expect(secondCallOptions.enabled).toBe(true);
94+
});
95+
96+
it('passes enabled: false to useQueriedChartConfig for previous period when compareToPreviousPeriod is false', () => {
97+
const config = {
98+
...baseTestConfig,
99+
compareToPreviousPeriod: false,
100+
};
101+
102+
renderWithMantine(<DBTimeChart config={config} />);
103+
104+
// Get the second call (previous period query)
105+
const [, secondCallOptions] = mockUseQueriedChartConfig.mock.calls[1];
106+
107+
// Verify that enabled is false for the previous period query
108+
expect(secondCallOptions.enabled).toBe(false);
109+
});
110+
111+
it('respects the enabled prop when determining if previous period query should run', () => {
112+
const config = {
113+
...baseTestConfig,
114+
compareToPreviousPeriod: true,
115+
};
116+
117+
// Render with enabled=false
118+
renderWithMantine(<DBTimeChart config={config} enabled={false} />);
119+
120+
// Get the second call (previous period query)
121+
const [, secondCallOptions] = mockUseQueriedChartConfig.mock.calls[1];
122+
123+
// Verify that enabled is false even when compareToPreviousPeriod is true
124+
// because the enabled prop is false
125+
expect(secondCallOptions.enabled).toBe(false);
126+
});
127+
});

0 commit comments

Comments
 (0)