Repository navigation
feat: relative date ranges can be saved for dashboards #3073
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
d021ac1
1014344
e557c33
83f263e
b2210e6
c693601
f557179
7ab165f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| --- | ||
| "@hyperdx/common-utils": patch | ||
| "@hyperdx/api": patch | ||
| "@hyperdx/app": patch | ||
| --- | ||
|
|
||
| feat: relative date ranges for dashboards can now be saved |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -78,6 +78,7 @@ import { | |
| Group, | ||
| Indicator, | ||
| List, | ||
| Loader, | ||
| Menu, | ||
| Modal, | ||
| Paper, | ||
|
|
@@ -147,7 +148,10 @@ import { PageHeader } from '@/components/PageHeader'; | |
| import { PageLayout } from '@/components/PageLayout'; | ||
| import { SqlVariablesProvider } from '@/components/SQLEditor/variableCompletions'; | ||
| import { TimePicker } from '@/components/TimePicker'; | ||
| import { parseTimeRangeInput } from '@/components/TimePicker/utils'; | ||
| import { | ||
| parseTimeRangeInput, | ||
| timeRangeInputToSeconds, | ||
| } from '@/components/TimePicker/utils'; | ||
| import { | ||
| Dashboard, | ||
| type Tile, | ||
|
|
@@ -211,6 +215,7 @@ import { | |
| } from './source'; | ||
| import { | ||
| dateRangeToString, | ||
| parseRelativeTimeQuery, | ||
| useDefaultTimeRange, | ||
| useNewTimeQuery, | ||
| } from './timeQuery'; | ||
|
|
@@ -1855,15 +1860,14 @@ function DashboardContainerRow({ | |
|
|
||
| const DEFAULT_INTERVAL = 'Past 1h'; | ||
|
|
||
| function DBDashboardPage({ presetConfig }: { presetConfig?: Dashboard }) { | ||
| const defaultTimeRange = useDefaultTimeRange(DEFAULT_INTERVAL); | ||
| const brandName = useBrandDisplayName(); | ||
| const confirm = useConfirm(); | ||
|
|
||
| const router = useRouter(); | ||
| const dashboardId = router.query.dashboardId as string | undefined; | ||
| const { enterKioskMode, exitKioskMode, isKioskMode } = | ||
| useDashboardKioskMode(); | ||
| function DBDashboardPage({ | ||
| dashboardProps, | ||
| defaultTimeInput = DEFAULT_INTERVAL, | ||
| }: { | ||
| dashboardProps: ReturnType<typeof useDashboard>; | ||
| defaultTimeInput?: string; | ||
| }) { | ||
| const defaultTimeRange = useDefaultTimeRange(defaultTimeInput); | ||
|
|
||
| const { | ||
| dashboard, | ||
|
|
@@ -1872,10 +1876,17 @@ function DBDashboardPage({ presetConfig }: { presetConfig?: Dashboard }) { | |
| isLocalDashboard, | ||
| isFetching: isFetchingDashboard, | ||
| isSetting: isSavingDashboard, | ||
| } = useDashboard({ | ||
| dashboardId: dashboardId as string | undefined, | ||
| presetConfig, | ||
| }); | ||
| } = dashboardProps; | ||
| const brandName = useBrandDisplayName(); | ||
| const confirm = useConfirm(); | ||
| const { | ||
| userPreferences: { isUTC }, | ||
| } = useUserPreferences(); | ||
|
|
||
| const router = useRouter(); | ||
| const dashboardId = router.query.dashboardId as string | undefined; | ||
| const { enterKioskMode, exitKioskMode, isKioskMode } = | ||
| useDashboardKioskMode(); | ||
|
|
||
| const { data: sources } = useSources(); | ||
| const { data: connections } = useConnections(); | ||
|
|
@@ -2033,10 +2044,10 @@ function DBDashboardPage({ presetConfig }: { presetConfig?: Dashboard }) { | |
| }, [router.isReady, watchedGranularity, granularity, setGranularity]); | ||
|
|
||
| const [displayedTimeInputValue, setDisplayedTimeInputValue] = | ||
| useState(DEFAULT_INTERVAL); | ||
| useState(defaultTimeInput); | ||
|
|
||
| const { searchedTimeRange, onSearch, onTimeRangeSelect } = useNewTimeQuery({ | ||
| initialDisplayValue: DEFAULT_INTERVAL, | ||
| initialDisplayValue: defaultTimeInput, | ||
| initialTimeRange: defaultTimeRange, | ||
| setDisplayedTimeInputValue, | ||
| }); | ||
|
|
@@ -2113,6 +2124,7 @@ function DBDashboardPage({ presetConfig }: { presetConfig?: Dashboard }) { | |
| dashboard?.savedQuery, | ||
| dashboard?.savedQueryLanguage, | ||
| dashboard?.savedFilterValues, | ||
| dashboard?.savedDateRange, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 minor — The initialization effect body (lines 1994–2034) never reads
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 minor — Dead dependencies added to the dashboard-initialization effect
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 minor — The initialization effect at line 2081 never touches |
||
| isLocalDashboard, | ||
| isFetchingDashboard, | ||
| router.isReady, | ||
|
|
@@ -2121,6 +2133,7 @@ function DBDashboardPage({ presetConfig }: { presetConfig?: Dashboard }) { | |
| setWhere, | ||
| setWhereLanguage, | ||
| setFilterValueEntries, | ||
| onTimeRangeSelect, | ||
| ]); | ||
|
|
||
| // Sync changes to the URL params into the form | ||
|
|
@@ -2150,18 +2163,30 @@ function DBDashboardPage({ presetConfig }: { presetConfig?: Dashboard }) { | |
| ? filterValueEntries | ||
| : []; | ||
|
|
||
| const currentRelativeDateRange = timeRangeInputToSeconds( | ||
|
knudtty marked this conversation as resolved.
Resolved
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 minor — Unparseable input keeps the old saved range while the toast claims it was saved If |
||
| displayedTimeInputValue, | ||
| isUTC, | ||
| ); | ||
|
|
||
| setDashboard( | ||
| produce(dashboard, draft => { | ||
| draft.savedQuery = currentWhere; | ||
| draft.savedQueryLanguage = currentWhereLanguage; | ||
| draft.savedFilterValues = currentFilterValues; | ||
| // Only supporting relative date range saving ATM | ||
| if (currentRelativeDateRange) { | ||
|
knudtty marked this conversation as resolved.
Resolved
knudtty marked this conversation as resolved.
Resolved
knudtty marked this conversation as resolved.
Resolved
knudtty marked this conversation as resolved.
Resolved
|
||
| draft.savedDateRange = { | ||
| type: 'relative', | ||
| value: currentRelativeDateRange, | ||
| }; | ||
| } | ||
|
knudtty marked this conversation as resolved.
Resolved
|
||
| }), | ||
| () => { | ||
| notifications.show({ | ||
| color: 'green', | ||
| title: 'Query saved and executed', | ||
| message: | ||
| 'Filter query and dropdown values have been saved with the dashboard', | ||
| 'Filter query, dropdown values, and relative time range have been saved with the dashboard', | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 minor — Unparseable time input leaves a stale saved range while the toast claims it was saved
knudtty marked this conversation as resolved.
Resolved
|
||
| autoClose: 3000, | ||
| }); | ||
| }, | ||
|
|
@@ -2173,6 +2198,8 @@ function DBDashboardPage({ presetConfig }: { presetConfig?: Dashboard }) { | |
| getValues, | ||
| filterValueEntries, | ||
| onSubmit, | ||
| isUTC, | ||
| displayedTimeInputValue, | ||
| ]); | ||
| const handleRemoveSavedQuery = useCallback(() => { | ||
|
knudtty marked this conversation as resolved.
Resolved
|
||
| if (!dashboard || isLocalDashboard) return; | ||
|
|
@@ -2182,6 +2209,7 @@ function DBDashboardPage({ presetConfig }: { presetConfig?: Dashboard }) { | |
| draft.savedQuery = null; | ||
| draft.savedQueryLanguage = null; | ||
| draft.savedFilterValues = []; | ||
| draft.savedDateRange = null; | ||
| }), | ||
| () => { | ||
| notifications.show({ | ||
|
|
@@ -3484,7 +3512,46 @@ function DBDashboardPage({ presetConfig }: { presetConfig?: Dashboard }) { | |
| ); | ||
| } | ||
|
|
||
| const DBDashboardPageDynamic = dynamic(async () => DBDashboardPage, { | ||
| function DBDashboardPageGuarded({ | ||
| presetConfig, | ||
| }: { | ||
| presetConfig?: Dashboard; | ||
| }) { | ||
| const router = useRouter(); | ||
| const dashboardId = router.query.dashboardId as string | undefined; | ||
| const dashboardProps = useDashboard({ | ||
| dashboardId: dashboardId as string | undefined, | ||
| presetConfig, | ||
| }); | ||
| const { | ||
| userPreferences: { isUTC }, | ||
| } = useUserPreferences(); | ||
|
|
||
| const savedDateRange = dashboardProps.dashboard?.savedDateRange; | ||
| // Keyed on the saved range, not the render: a relative range re-stringifies | ||
| // to a new value every render, and useNewTimeQuery resets the input on change. | ||
| // Valid URL from/to still win: useNewTimeQuery overwrites the input from them. | ||
| const defaultTimeInput = useMemo(() => { | ||
| if (!savedDateRange) return undefined; | ||
| const [start, end] = | ||
| savedDateRange.type === 'relative' | ||
| ? parseRelativeTimeQuery(savedDateRange.value * 1000) | ||
| : savedDateRange.value.map(v => new Date(v)); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 minor —
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 minor — Resolved dates are round-tripped through a year-less display string The guard resolves
knudtty marked this conversation as resolved.
Resolved
|
||
| // TODO: show relative ranges as "Past Xh" via getRelativeInterval | ||
| return dateRangeToString([start, end], isUTC); | ||
|
knudtty marked this conversation as resolved.
Resolved
|
||
| }, [savedDateRange, isUTC]); | ||
|
|
||
| if (!dashboardProps || !router.isReady) return <Loader size="lg" />; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 minor —
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 minor —
|
||
|
|
||
| return ( | ||
| <DBDashboardPage | ||
| dashboardProps={dashboardProps} | ||
| defaultTimeInput={defaultTimeInput} | ||
| /> | ||
| ); | ||
| } | ||
|
|
||
| const DBDashboardPageDynamic = dynamic(async () => DBDashboardPageGuarded, { | ||
| ssr: false, | ||
| }); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,8 @@ | ||
| import { dateParser, parseTimeRangeInput } from '@/components/TimePicker/utils'; | ||
| import { | ||
| dateParser, | ||
| parseTimeRangeInput, | ||
| timeRangeInputToSeconds, | ||
| } from '@/components/TimePicker/utils'; | ||
|
|
||
| describe('dateParser', () => { | ||
| let mockDate: Date; | ||
|
|
@@ -154,3 +158,30 @@ describe('parseTimeRangeInput', () => { | |
| ]); | ||
| }); | ||
| }); | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 minor — New tests cover only the pure helper, not the feature
|
||
| describe('timeRangeInputToSeconds', () => { | ||
| beforeEach(() => { | ||
| jest.useFakeTimers(); | ||
| jest.setSystemTime(new Date('2025-01-15T22:00:00')); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 minor — New tests cover only the helper, not the feature the PR is about The added tests exercise |
||
| }); | ||
|
|
||
| afterEach(() => { | ||
| jest.useRealTimers(); | ||
| }); | ||
|
|
||
| it('returns the relative range length in seconds', () => { | ||
| expect(timeRangeInputToSeconds('Past 1h', false)).toBe(3600); | ||
| expect(timeRangeInputToSeconds('Past 15m', false)).toBe(900); | ||
| }); | ||
|
|
||
| it('returns the span of an absolute range', () => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 minor — New tests never exercise the feature, and one pins the absolute-as-relative bug as correct The only new coverage is |
||
| expect(timeRangeInputToSeconds('2024-01-15 to 2024-01-16', false)).toBe( | ||
| 86400, | ||
| ); | ||
| }); | ||
|
|
||
| it('returns null for unparseable input', () => { | ||
| expect(timeRangeInputToSeconds('', false)).toBeNull(); | ||
| expect(timeRangeInputToSeconds('not a date', false)).toBeNull(); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -72,6 +72,16 @@ export function parseTimeRangeInput( | |
| } | ||
| } | ||
|
|
||
| // Duration of a time range input in seconds, or null if it doesn't parse. | ||
| export function timeRangeInputToSeconds( | ||
| str: string, | ||
| isUTC: boolean, | ||
| ): number | null { | ||
| const [start, end] = parseTimeRangeInput(str, isUTC); | ||
| if (start == null || end == null) return null; | ||
| return (end.getTime() - start.getTime()) / 1000; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 minor — Persisted duration is fractional seconds, breaking the existing millisecond-integer convention
|
||
| } | ||
|
|
||
| export const LIVE_TAIL_TIME_QUERY = 'Live Tail' as const; | ||
| export const LIVE_TAIL_DURATION_MS = ms('15m'); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -276,7 +276,7 @@ | |
| await dashboardPage.chartEditor.setChartName('My Unsaved Chart'); | ||
|
|
||
| await dashboardPage.page.keyboard.press('Escape'); | ||
| await expect(dashboardPage.unsavedChangesConfirmModal).toBeAttached({ | ||
|
Check failure on line 279 in packages/app/tests/e2e/features/dashboard.spec.ts
|
||
| timeout: 5000, | ||
| }); | ||
|
|
||
|
|
@@ -1201,7 +1201,7 @@ | |
|
|
||
| // Wait for success notification | ||
| const notification = dashboardPage.page.locator( | ||
| 'text=/Filter query and dropdown values/i', | ||
| 'text=/Filter query, dropdown values, and relative time range/i', | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 minor — No test covers the save→reload round trip this PR adds The three e2e edits only update the notification regex, and the new unit tests cover |
||
| ); | ||
| await expect(notification).toBeVisible({ timeout: 5000 }); | ||
| }); | ||
|
|
@@ -1266,7 +1266,7 @@ | |
| // Wait for the save success notification rather than a blind sleep, so | ||
| // we only read the URL once the save has actually landed. | ||
| const notification = dashboardPage.page.locator( | ||
| 'text=/Filter query and dropdown values/i', | ||
| 'text=/Filter query, dropdown values, and relative time range/i', | ||
| ); | ||
| await expect(notification).toBeVisible({ timeout: 5000 }); | ||
|
|
||
|
|
@@ -2649,7 +2649,7 @@ | |
|
|
||
| // Wait for success notification | ||
| const notification = dashboardPage.page.locator( | ||
| 'text=/Filter query and dropdown values/i', | ||
| 'text=/Filter query, dropdown values, and relative time range/i', | ||
| ); | ||
| await expect(notification).toBeVisible({ timeout: 5000 }); | ||
| }); | ||
|
|
@@ -2670,7 +2670,7 @@ | |
|
|
||
| // Wait for success notification | ||
| const notification = dashboardPage.page.locator( | ||
| 'text=/Filter query and dropdown values/i', | ||
| 'text=/Filter query, dropdown values, and relative time range/i', | ||
| ); | ||
| await expect(notification).toBeVisible({ timeout: 5000 }); | ||
| }); | ||
|
|
@@ -2965,7 +2965,7 @@ | |
| columnIndex: number, | ||
| substring: string, | ||
| ) => { | ||
| await expect | ||
|
Check failure on line 2968 in packages/app/tests/e2e/features/dashboard.spec.ts
|
||
| .poll( | ||
| async () => { | ||
| const cells = await dashboardPage.getTileTableCellTexts( | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.