feat: relative date ranges can be saved for dashboards - #3073
Conversation
🦋 Changeset detectedLatest commit: f557179 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🟡 Tier 3 — StandardIntroduces new logic, modifies core functionality, or touches areas with non-trivial risk. Why this tier:
Review process: Full human review — logic, architecture, edge cases. Stats
|
E2E Test Results✅ All tests passed • 342 passed • 1 skipped • 1445s
Tests ran across 4 shards in parallel. |
Greptile SummaryThe PR adds persisted relative time-range defaults to dashboards while preserving explicit URL time ranges as the higher-priority view state.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| packages/app/src/DBDashboardPage.tsx | Integrates saved relative ranges into dashboard save, removal, loading, URL precedence, and dashboard-switch initialization. |
| packages/app/src/components/TimePicker/utils.ts | Adds a focused helper that parses a time input and returns its duration in seconds. |
| packages/common-utils/src/types.ts | Adds a discriminated, nullable saved date-range schema shared across dashboard consumers. |
| packages/api/src/models/dashboard.ts | Extends dashboard persistence to retain the validated saved date-range payload. |
| packages/app/tests/e2e/features/dashboard.spec.ts | Updates save-success assertions to match the expanded persisted-default behavior. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[Load dashboard] --> B{URL has from and to?}
B -->|Yes| C[Use explicit URL range]
B -->|No| D{Saved date range exists?}
D -->|Yes| E[Derive saved relative range]
D -->|No| F[Use Past 1h]
C --> G[Render dashboard queries]
E --> G
F --> G
H[Save query and filters] --> I[Persist relative range duration]
I --> A
Reviews (7): Last reviewed commit: "fix: add dashboard guard to properly han..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| ? filterValueEntries | ||
| : []; | ||
|
|
||
| const currentRelativeDateRange = |
There was a problem hiding this comment.
🔵 minor — handleSaveQuery saves the pre-submit time range, unlike the query it saves from post-submit form values
onSubmit() on line 2065 applies displayedTimeInputValue (which TimePicker.tsx:299 updates on every keystroke, without searching), but searchedTimeRange only refreshes after the from/to round-trip, so typing a new range into the picker and then clicking the menu item persists the old duration while the dashboard shows the new one. Derive the duration from the value being submitted — parseTimeRangeInput(displayedTimeInputValue) is already imported at line 149 — rather than from searchedTimeRange.
|
|
||
| // Initialize dashboard with the saved date range | ||
| if (dashboard.savedRelativeDateRange) { | ||
| onTimeRangeSelect( |
There was a problem hiding this comment.
🔵 minor — Applying the saved range on load pushes a history entry, so Back re-lands on the dashboard
onTimeRangeSelect writes from/to through useQueryStates(..., { history: 'push' }) (packages/app/src/timeQuery.ts:442-447), while the saved-query init path uses useQueryState defaults (replace) — so arriving from the dashboards list now costs two Back presses, and the first one drops the range to Past 1h. Apply the saved range without a push (e.g. feed it as initialTimeRange to useNewTimeQuery, which also makes URL from/to win naturally).
PR Review7 finding(s): 🔴 0 critical · 🟠 2 major · 🔵 5 minor 6 posted as inline comment(s) on the changed lines. 1 listed below. Findings outside the changed lines
Severity is the reviewer's own estimate and is used for ordering, not filtering. |
Deep ReviewRelative dashboard date ranges are now persisted as a discriminated-union ✅ No critical issues found. 🟡 P2 — recommended
🔵 P3 nitpicks (4)
Reviewers (5): correctness, kieran-typescript, julik-frontend-races, testing, previous-comments. Testing gaps:
|
| const [start, end] = parseRelativeTimeQuery( | ||
| dashboard.savedRelativeDateRange * 1000, | ||
| ); | ||
| onTimeRangeSelect(start, end); |
There was a problem hiding this comment.
🔵 minor — Restoring the saved range pushes a browser history entry on every dashboard open
onTimeRangeSelect writes through useQueryStates(timeRangeQueryStateMap, { history: 'push' }) (timeQuery.ts:442-447), so applying the saved range during initialization adds a history entry the user never created: pressing Back after opening the dashboard returns to the same dashboard without from/to (which resets the range to Past 1h) instead of going to the previous page. Apply the restored range with history: 'replace' semantics — e.g. an option on onTimeRangeSelect, matching how the other restored defaults (setWhere, setFilterValueEntries) use nuqs' default replace behaviour.
| savedQuery: z.string().nullable().optional(), | ||
| savedQueryLanguage: SearchConditionLanguageSchema.nullable().optional(), | ||
| savedFilterValues: z.array(DashboardFilterValueSchema).optional(), | ||
| savedRelativeDateRange: z.number().nullish(), |
There was a problem hiding this comment.
🔵 minor — Schema admits zero and negative durations
savedRelativeDateRange: z.number().nullish() accepts any finite number, and the PATCH body schema (packages/api/src/routers/api/dashboards.ts:179) is the only validation on the way to Mongo. PATCH /api/dashboards/:id {"savedRelativeDateRange": -3600} stores it, and on load parseRelativeTimeQuery(-3600000) yields start > end, so every tile queries an inverted range. Use z.number().positive().nullish().
| } | ||
|
|
||
| // Initialize dashboard with the saved date range | ||
| if (!hasDateRangeInUrl) { |
There was a problem hiding this comment.
🔵 minor — No test covers the save → restore round-trip for the date range
The save path (duration derivation, line 2081) and the restore path (line 2022) are both untested: dashboard.spec.ts:1245 and dashboard-filter-value-format.spec.ts:354 already drive saveQueryAndFiltersAsDefault() and assert the query/filter defaults restore on reload, but nothing asserts the time range. Extend one of those specs to select a known window, save defaults, reopen the dashboard with no params, and assert the resulting from/to span matches the saved duration — that would also have caught the hidden "Remove" menu item above.
| ): number | null { | ||
| const [start, end] = parseTimeRangeInput(str, isUTC); | ||
| if (start == null || end == null) return null; | ||
| return (end.getTime() - start.getTime()) / 1000; |
There was a problem hiding this comment.
🔵 minor — Persisted duration is fractional seconds, breaking the existing millisecond-integer convention
(end - start) / 1000 is unrounded, and in real (non-faked) time parseTimeRangeInput builds start and the end fallback from two separate new Date() calls (utils.ts:10 and utils.ts:63), so "Past 1h" persists as e.g. 3600.002 rather than 3600 — the unit test only gets 3600 because it freezes the clock. Every other relative range in the app is a whole-millisecond integer (RELATIVE_TIME_OPTIONS, LIVE_TAIL_DURATION_MS, and the persisted liveInterval query state at DBSearchPage.tsx:1594-1597), which is also what getRelativeTimeOptionLabel keys off. Store rounded milliseconds and drop the * 1000 at DBDashboardPage.tsx:2029, and tighten the schema to z.number().int().nonnegative().nullish() — this is a persisted format, so old values stay valid forever.
| const [start, end] = parseRelativeTimeQuery( | ||
| dashboard.savedRelativeDateRange * 1000, | ||
| ); | ||
| onTimeRangeSelect(start, end); |
There was a problem hiding this comment.
🔵 minor — Applying the saved range on load pushes a history entry, trapping the back button
onTimeRangeSelect writes through setTimeRangeQuery, which is configured history: 'push' (timeQuery.ts:442-447). Opening a dashboard that has a saved range therefore adds a ?from=…&to=… entry on mount: pressing Back from the dashboard list returns to the same dashboard with the range silently reset to Past 1h (the from == null && to == null branch at timeQuery.ts:480), and the user must press Back twice to leave. Apply the initial range with replace semantics — e.g. add a replace option to onTimeRangeSelect/setTimeRangeQuery — since this write is not user-initiated navigation.
| describe('timeRangeInputToSeconds', () => { | ||
| beforeEach(() => { | ||
| jest.useFakeTimers(); | ||
| jest.setSystemTime(new Date('2025-01-15T22:00:00')); |
There was a problem hiding this comment.
🔵 minor — New tests cover only the helper, not the feature the PR is about
The added tests exercise timeRangeInputToSeconds and the Zod field, but nothing covers the two pieces of real logic: the initialization branch at DBDashboardPage.tsx:2026-2033 (saved range applied only when from/to are absent from the URL) and the save branch at 2084-2094. A regression that stopped applying the saved range, or that let a URL range be overwritten by it, would still pass. The existing e2e dashboard.spec.ts:1245 ("URL query params overriding saved query") is the natural place to add the parallel date-range case; the isUTC: true path of the helper is also untested.
| savedFilterValues?: DashboardFilterValue[]; | ||
| savedDateRange?: | ||
| | { type: 'relative'; value: number } | ||
| | { type: 'historical'; value: [Date, Date] } |
There was a problem hiding this comment.
🔵 minor — App Dashboard.savedDateRange disagrees with DashboardSchema on the historical variant
The app type declares {type:'historical'; value:[Date, Date]} while the wire schema declares value: z.array(z.number()).length(2) (packages/common-utils/src/types.ts:2063-2066), and JSON from the API can never carry Dates — so const [start, end] = dateRange.value at packages/app/src/DBDashboardPage.tsx:1969 is typed Date but receives numbers for any dashboard written through the API or MCP. Derive the app field from the shared schema (z.infer<typeof DashboardSchema>['savedDateRange']) instead of restating it, per the "one source of truth" rule in the conventions, and document what the two numbers mean (epoch ms).
| 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', |
There was a problem hiding this comment.
🔵 minor — Unparseable time input leaves a stale saved range while the toast claims it was saved
if (currentRelativeDateRange) skips the write when the input doesn't parse (e.g. Live Tail, or a half-typed value), but the notification still reports "…and relative time range have been saved with the dashboard", and any previously saved range silently survives. Clear draft.savedDateRange = null in the else branch, or word the toast off the value actually written.
476ff37 to
3cabd3c
Compare
| const [start, end] = | ||
| savedDateRange.type === 'relative' | ||
| ? parseRelativeTimeQuery(savedDateRange.value * 1000) | ||
| : savedDateRange.value.map(v => new Date(v)); |
There was a problem hiding this comment.
🔵 minor — historical values are read as milliseconds while relative is seconds
savedDateRange.value * 1000 treats the relative value as seconds, but savedDateRange.value.map(v => new Date(v)) treats the historical pair as epoch millis — the new schema test uses [1700000000, 1700003600], i.e. epoch seconds, which this branch would render as Jan 1970. Pin the unit in DashboardSchema (a comment plus a consistent * 1000) before the format is persisted and impossible to change.
| return dateRangeToString([start, end], isUTC); | ||
| }, [savedDateRange, hasUrlRange, isUTC]); | ||
|
|
||
| if (!dashboardProps || !router.isReady) return <Loader size="lg" />; |
There was a problem hiding this comment.
🔵 minor — !dashboardProps can never be true, so the guard doesn't guard the fetch
useDashboard always returns an object literal, so only !router.isReady is doing anything: the child still mounts while the remote dashboard is loading, with savedDateRange undefined and defaultTimeInput 'Past 1h', and the saved range only lands later via the initialTimeRange effect. Gate on the data instead — e.g. if (!router.isReady || (!dashboardProps.isLocalDashboard && dashboardProps.isFetching)).
| dashboard?.savedQuery, | ||
| dashboard?.savedQueryLanguage, | ||
| dashboard?.savedFilterValues, | ||
| dashboard?.savedDateRange, |
There was a problem hiding this comment.
🔵 minor — savedDateRange and onTimeRangeSelect added as deps of an effect that uses neither
The initialization effect body (lines 1994–2034) never reads dashboard.savedDateRange or calls onTimeRangeSelect; the two new deps only make the effect look like it applies the saved range when the range is actually applied through defaultTimeInput. Drop both deps, or move the range application into this effect where the initializedDashboardRef / URL-precedence logic already lives.
| ? filterValueEntries | ||
| : []; | ||
|
|
||
| const currentRelativeDateRange = timeRangeInputToSeconds( |
There was a problem hiding this comment.
🔵 minor — Unparseable input keeps the old saved range while the toast claims it was saved
If displayedTimeInputValue doesn't parse (mid-edit text, Live Tail), currentRelativeDateRange is null, the if is skipped so a previously saved range silently survives, yet the notification still says the relative time range was saved. Either clear draft.savedDateRange = null in the null case or keep the range out of the toast copy when nothing was written.
| ]); | ||
| }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
🔵 minor — New tests cover only the pure helper, not the feature
timeRangeInputToSeconds and the Zod shape are tested, but nothing exercises the actual behaviour this PR adds — handleSaveQuery writing savedDateRange, DBDashboardPageGuarded deriving defaultTimeInput from it, or URL from/to taking precedence (the branch that produces the [null, null] range above). Add a test around the guard's derivation, and extend the saved-defaults e2e in packages/app/tests/e2e/features/dashboard.spec.ts:1245 to reload and assert the restored time range.
3cabd3c to
f557179
Compare
| const [start, end] = | ||
| savedDateRange.type === 'relative' | ||
| ? parseRelativeTimeQuery(savedDateRange.value * 1000) | ||
| : savedDateRange.value.map(v => new Date(v)); |
There was a problem hiding this comment.
🔵 minor — Resolved dates are round-tripped through a year-less display string
The guard resolves savedDateRange to real Dates, throws them away by formatting with dateRangeToString (format 'normal' = MMM d HH:mm:ss, no year — see TIME_TOKENS in packages/common-utils/src/core/utils.ts:590), and the child re-derives them with chrono at line 1958. For the historical branch handled right here — writable today via PATCH /api/dashboards/:id, whose body is DashboardSchema.partial() (packages/api/src/routers/api/dashboards.ts:179) — a range from a previous year comes back parsed into the current year, so the dashboard loads the wrong window. The as [Date, Date] cast at line 1958 also hides the [null, null] that parseTimeQuery returns when the string doesn't parse, which would flow into searchedTimeRange. Pass the resolved [Date, Date] down next to the display string instead of re-parsing it.
| savedQuery: z.string().nullable().optional(), | ||
| savedQueryLanguage: SearchConditionLanguageSchema.nullable().optional(), | ||
| savedFilterValues: z.array(DashboardFilterValueSchema).optional(), | ||
| savedDateRange: z |
There was a problem hiding this comment.
🔵 minor — savedDateRange.value units are unspecified and inconsistent between variants
relative is written in seconds (DBDashboardPage.tsx:2079) and read back as value * 1000, while the only reader of historical does new Date(v), i.e. epoch milliseconds — even though the new test uses second-scale epochs (1700000000). Every other relative duration in the app is milliseconds (RELATIVE_TIME_OPTIONS, LIVE_TAIL_DURATION_MS, the liveInterval param in DBSearchPage.tsx:1594), and getRelativeTimeOptionLabel — the helper the line 3450 TODO needs — takes ms. The seconds value is also fractional ((end - start) / 1000 over two wall-clock reads, so ~3600.002), which will never match a label. Store ms as an integer in both variants and document the unit in the schema; this format is persisted, so old values stay valid forever.
| dashboard?.savedQuery, | ||
| dashboard?.savedQueryLanguage, | ||
| dashboard?.savedFilterValues, | ||
| dashboard?.savedDateRange, |
There was a problem hiding this comment.
🔵 minor — Dead dependencies added to the dashboard-initialization effect
dashboard?.savedDateRange and onTimeRangeSelect were added to the dependency list, but nothing in the effect body (lines 1994-2034) reads either — the saved range is applied through the defaultTimeInput prop instead, and the effect early-returns via initializedDashboardRef. Drop both deps (or move the range application into this effect, which is what they suggest was intended).
| return dateRangeToString([start, end], isUTC); | ||
| }, [savedDateRange, isUTC]); | ||
|
|
||
| if (!dashboardProps || !router.isReady) return <Loader size="lg" />; |
There was a problem hiding this comment.
🔵 minor — !dashboardProps can never be true
useDashboard returns a fresh object literal on every render (packages/app/src/dashboard.ts:274), so this half of the guard is unreachable. Reduce it to if (!router.isReady), or gate on something meaningful such as dashboardProps.isFetching.
| // 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', |
There was a problem hiding this comment.
🔵 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 timeRangeInputToSeconds and the Zod shape — nothing asserts that a saved range comes back on load, which is the whole feature, and nothing pins the relative-vs-absolute distinction the save path depends on. should save and restore query and filter values (line 1151) already saves defaults, navigates away, and returns with URL params cleared; add an assertion on the TimePicker input there.
1 participant
Summary
Allows date ranges to be saved for dashboards. If a user clicks "Save Query & Filters as default", the time range will be used when loading the dashboard in the future.
Screenshots or video
export-1788457467064.mp4
References