Repository navigation
feat: open saved searches with a saved default time range - #3593
Asheesh7298 wants to merge 2 commits into
Conversation
Saved searches can store a relative defaultTimeRange (e.g. past 1 day) that is applied when the search is opened without a time in the URL, instead of always landing in live tail. The save modal gets a "When opened" choice. Same shape as the relative case of dashboard savedDateRange; fixed windows are not accepted. The saved-search PATCH route now drops only undefined, so sending null clears the stored range. Refs ClickHouse#3567
|
@Asheesh7298 is attempting to deploy a commit to the HyperDX Team on Vercel. A member of the Team first needs to authorize it. |
github-actions
Bot
added
external
needs-vouch
|
Hi @Asheesh7298, thanks for the pull request! Before we review code from a first-time contributor, a maintainer vouches for them, and you're not on our list yet. There's nothing else to file and this PR stays open — a maintainer will look at it, usually within a day or two, and vouch for you right here if it's a genuine attempt at the problem. To make that quick, check the PR description has:
More detail in our contributing guide. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // Live updates pause while the save modal is open, so the window is stable. | ||
| const options = useMemo(() => { | ||
| const current: CurrentTimeRange = | ||
| isLive || openedAtMs == null |
There was a problem hiding this comment.
🟠 major — A relative interval in live mode (Last 1–6 hours etc.) can't be saved as the default time range
In relative mode, onTimePickerRelativeSearch (DBSearchPage.tsx:2246) sets isLive=true and liveInterval=rangeMs for every option with relativeSupport (1 minute to 6 hours, TimePicker/utils.ts:185-193). This hook treats any isLive as { kind: 'live' }, so the modal only offers "Show live tail" and says "To save a time range, pick one first", even though the user just picked one. When the saved search reopens, it drops the chosen 1h interval and falls back to 15m live tail. Fix: pass interval into the hook. When isLive && interval !== LIVE_TAIL_DURATION_MS, offer current as { type: 'relative', value: interval / 1000 }.
Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond
There was a problem hiding this comment.
Fixed in 437ab94. A live tail interval (1 minute to 6 hours) can now be saved, and those ranges reopen as live tail with that interval. Longer ranges still open as a window ending now. Added unit and e2e coverage.
| */ | ||
| export const RelativeDateRangeSchema = z.object({ | ||
| type: z.literal('relative'), | ||
| value: z.number(), |
There was a problem hiding this comment.
🔵 minor — RelativeDateRangeSchema accepts zero and negative durations, so the client has to guard against them
value: z.number() lets POST/PATCH store { type: 'relative', value: 0 } or -60. getSavedSearchLandingRange then silently ignores these values (savedSearchTimeRange.ts:29), and the unit tests pin that workaround. Make the schema z.number().positive() (this suits dashboards too) so invalid ranges are rejected with a 400. Then drop the <= 0 check on the client.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
There was a problem hiding this comment.
Fixed in 437ab94. defaultTimeRange now requires a positive duration and returns 400 otherwise, and the client guard is removed. I left the shared RelativeDateRangeSchema as is so dashboard validation doesn't change in this PR.
| if (!Number.isFinite(stored.value) || stored.value <= 0) return null; | ||
| // Whole seconds, like parseRelativeTimeQuery, so re-parsing the displayed | ||
| // text on the first submit lands on the same from/to. | ||
| const end = Math.floor(nowMs / 1000) * 1000; |
There was a problem hiding this comment.
🔵 minor — New helpers duplicate existing time helpers in timeQuery.ts
getSavedSearchLandingRange rebuilds parseRelativeTimeQuery (packages/app/src/timeQuery.ts:46): the end is truncated to the second and the duration is subtracted. The comment even names it. formatRelativeDuration (line 67) also repeats the formatDuration(intervalToDuration(...)) logic of getRelativeInterval (timeQuery.ts:417). Fix: give parseRelativeTimeQuery an optional nowMs argument and call it. Export one duration formatter from timeQuery.ts and use it in both places, instead of keeping a second copy (this follows the REQUIRED DRY rule).
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
There was a problem hiding this comment.
Partly fixed in 437ab94: opening a search now calls parseRelativeTimeQuery, which takes an optional now. I kept formatRelativeDuration separate on purpose. It anchors at epoch 0, so a saved "1 day" never prints as "23 hours" across a DST change, which getRelativeInterval can do because it uses the real start and end dates.
PR Review
2 finding(s): 🔴 0 critical · 🟠 0 major · 🔵 2 minor 2 posted as inline comment(s) on the changed lines. Severity is the reviewer's own estimate and is used for ordering, not filtering. No finding blocks merge automatically; a maintainer decides. |
Deep ReviewScope: PR #3593 — open saved searches with a saved default time range. 13 files, 2 commits, diffed against merge-base ✅ No critical issues found. The high-risk areas check out. The PATCH change from 🟡 P2 — recommended
🔵 P3 nitpicks (1)
Reviewers (8): correctness, testing, maintainability, project-standards, kieran-typescript, api-contract, julik-frontend-races, previous-comments. Testing gaps: The Note: the forced completion fired before 7 of the 8 dispatched reviewers returned structured findings; the findings above reflect the orchestrator's own full-diff analysis plus the completed previous-comments reviewer (which found nothing, as the prior comments are automated Vercel/first-contributor bot messages with no code feedback). Severities were graded down per the ship-blocker rubric. No finding blocks merge automatically. A maintainer will expect P0/P1 findings in code this PR changes to be fixed; P2/P3 are your call -- fix or reply. Never fix findings about surrounding code here; reply instead. Do not widen the PR. How to respond |
Address review feedback: - A live tail interval picked in the time picker (1 minute to 6 hours) can now be saved. Ranges live tail supports reopen as live tail with that interval; longer ones open as a window ending now. - defaultTimeRange rejects zero and negative durations. - Reuse parseRelativeTimeQuery (now takes an optional "now") instead of rebuilding it.
|
|
||
| /** "1 day", "1 hour 30 minutes". Anchored at epoch 0 so DST never shifts it. */ | ||
| export function formatRelativeDuration(seconds: number): string { | ||
| return formatDuration(intervalToDuration({ start: 0, end: seconds * 1000 })); |
There was a problem hiding this comment.
🔵 minor — formatRelativeDuration re-implements the duration formatting that getRelativeInterval in timeQuery.ts already does
getRelativeInterval (packages/app/src/timeQuery.ts:420) already does formatDuration(intervalToDuration(...)) to build the time picker's "Past …" label. This PR adds a second copy of that call in a new module, so the saved-search labels and the picker labels can drift apart. Extract one shared duration formatter in timeQuery.ts, use it from both getRelativeInterval and the save modal, and delete the copy, as the REQUIRED DRY convention asks.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
There was a problem hiding this comment.
Same point as the earlier thread: I kept this separate on purpose. It anchors at epoch 0 so a saved duration like "1 day" never prints as "23 hours" across a DST change, which getRelativeInterval can do because it formats real start/end dates. Making both share it would change the picker's existing label, so I'd rather leave that for a separate change.
|
|
||
| // A pick only counts for the opening it was made in. Until then the default | ||
| // applies, which can change once the stored saved search loads. | ||
| const [picked, setPicked] = useState<{ |
There was a problem hiding this comment.
🔵 minor — No test covers the hook's per-opening pick logic or the landing effect in DBSearchPage
Unit tests only cover the pure helpers. Nothing exercises the logic that drops a pick when openedAtMs changes, or that falls back to defaultChoice once stored loads. Nothing tests the DBSearchPage landing effect either, including whether it skips URLs that carry isLive/liveInterval. The e2e only covers the two create paths, so a regression where a reopened modal keeps the last pick (and Update overwrites the stored range) would still pass. Add a renderHook test for useSaveTimeRangeField (pick, close, reopen, expect default) and an update-flow case to DBSearchPage.saveSearch.test.tsx.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
There was a problem hiding this comment.
Fair point. The choice logic and the update resolver are unit tested, and the landing path is checked by the two e2e tests (window and live interval). I'm stopping bot-driven changes after two review rounds per AGENTS.md. Happy to add a renderHook test for the pick/reopen behaviour if a maintainer would like it in this PR.
|
@brandon-pereira this is ready for a look when you have time. It follows the plan from #3567 (defaultTimeRange, relative ranges only). Two automated review rounds are addressed: the live tail interval case is fixed, and the remaining minor notes are answered in their threads. |
Summary
Closes #3567.
Saved searches can now store a default time range (for example "past 1 day") and open with it instead of always landing in Live Tail. This follows the approach agreed on the issue, which is the same one #3073 used for dashboards.
Storage. A new optional
defaultTimeRangefield on saved searches, shaped{ type: 'relative', value: <seconds> }like the relative case of the dashboardsavedDateRange. That relative variant is now a namedRelativeDateRangeSchema, and both schemas use it. Saved searches accept only relative ranges: as agreed on the issue, fixed windows are out for now. Existing searches don't have the field and behave as before, so there's no migration.Opening. When the URL carries no time (nav, list and Spotlight links are a bare
/search/<id>), a stored range that live tail supports (1 minute to 6 hours) opens as live tail with that interval. Longer ranges open as[now − N, now]with live tail off, the same as dashboards. Links that already carry a time (isLiveorliveInterval, e.g. alert links) keep winning. The URL writes usereplace: a push would leave the bare URL in history, and Back would land on it and apply the range again.Saving. The save modal has a "When opened" choice:
Clearing. The PATCH route used
_.omitBy(…, _.isNil), which made it impossible to remove a stored range. It now drops onlyundefined. No other saved-search field acceptsnull, so validation still rejects null for those (there's a test for this).Not in this PR. These are follow-ups mentioned on the issue:
Screenshots or video
How to reproduce
Starting point:
yarn app:dev:local, onboarding → Connect to Demo Server.On
main:On this branch:
How to test on Vercel preview
Preview routes: /search, /search/list
Steps:
isLive=falseand the time picker shows a range of about one day ending now.References
savedDateRange)