Repository navigation
feat: open saved searches with a saved default time range - #3593
Asheesh7298 wants to merge 3 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
7 finding(s): 🔴 0 critical · 🟠 0 major · 🔵 7 minor 7 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: 13 files, ✅ No critical issues found. The null-clearing path is sound: after 🟡 P2 -- recommended
🔵 P3 nitpicks (5)
Reviewers (5 returned): kieran-typescript, api-contract, testing, maintainability, previous-comments. Correctness, adversarial, frontend-races, and project-standards surfaces were additionally verified directly by the orchestrator (null-clearing path, history-replace writes, effect-loop safety, e2e-helper existence, changeset presence) with no P0/P1 found. The previous-comments reviewer confirmed all prior feedback is addressed — notably the maintainer's UX note, which the latest commit resolved by replacing the option control with descriptive text plus a tooltip. Testing gaps:
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. |
| label={ | ||
| isLiveTailDuration(currentRange.value * 1000) | ||
| ? `Show live tail for the past ${formatRelativeDuration(currentRange.value)}` | ||
| : `Show the past ${formatRelativeDuration(currentRange.value)}` |
There was a problem hiding this comment.
UX Feedback: would it not make sense to just say 'Saving Saved Search with the past {relativeTime} as default search' or something along those lines? I feel like we should not give an option for this, but add a tooltip to explain how to customize
There was a problem hiding this comment.
Thanks for the suggestion,
Done in 3a4b8b1:
Removed the "When opened" choice. Saving now stores the selected range automatically.
The modal shows what the search will open with, e.g. "Opens with the past 1 day" or "Opens in live tail for the past 1 hour", with a tooltip explaining how to change it.
Ranges that end in the past aren't saved, and on Update they keep the stored range instead of clearing it.
Here's how it looks now. Happy to adjust the wording if you'd prefer something different.
Per review: drop the "When opened" choice from the save modal. Saving stores the selected range when it reads as "the past N" (a live tail interval or a window ending now). The modal shows what the search will open with, and a tooltip explains how to change it.
| [start, end]: [Date, Date], | ||
| nowMs: number, | ||
| ): boolean { | ||
| const tolerance = Math.max( |
There was a problem hiding this comment.
🔵 minor — Fixed windows that ended hours or days ago get saved as "the past N", which the tooltip says can't happen
Allowing 10% of the duration lets a 30-day absolute range that ended 2 days ago (tolerance is 3 days) be stored as {relative, 2592000}, and a 7-day range that ended 16h ago passes too. The tooltip in SaveTimeRangeField.tsx says "Ranges that end in the past are not saved". Cap the slack at a fixed small amount, or decide from what the picker shows rather than from timestamps (next finding).
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
| const range = toDefaultTimeRange(current.intervalMs); | ||
| return { value: range, opensWith: range }; | ||
| } | ||
| if (isRangeEndingNow(current.range, nowMs)) { |
There was a problem hiding this comment.
🔵 minor — Dashboards and saved searches decide "is this relative" in two different ways
Dashboards get the relative range from the picker text with timeRangeInputToSeconds(displayedTimeInputValue, timeZone) (DBDashboardPage.tsx:2176, helper in components/TimePicker/utils.ts:166). This PR guesses from the window's end time with a tolerance instead. Reuse the existing picker-text approach, or move one shared rule into TimePicker/utils.ts, so the same picker choice saves the same way in both features.
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
| '@hyperdx/common-utils': minor | ||
| --- | ||
|
|
||
| feat: saved searches can store a default time range (e.g. past 1 day) to open with instead of live tail, chosen in the save modal under "When opened" |
There was a problem hiding this comment.
🔵 minor — Changeset mentions a "When opened" control that no longer exists
The last commit, "save the selected time range without asking", removed the chooser. The modal now only shows read-only "Opens with…" text. Reword the release note to say the selected range is saved automatically.
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
| return formatDuration(intervalToDuration({ start: 0, end: seconds * 1000 })); | ||
| } | ||
|
|
||
| export type CurrentTimeRange = |
There was a problem hiding this comment.
🔵 minor — CurrentTimeRange is the same union as SavedSearchLanding in the same file
Both are {kind:'live'; intervalMs} | {kind:'window'; range}. Keep one exported type, e.g. type CurrentTimeRange = SavedSearchLanding. Also have the live landing return its range too, so DBSearchPage.tsx:1336 doesn't need to call parseRelativeTimeQuery a second time.
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
| } | ||
|
|
||
| /** "1 day", "1 hour 30 minutes". Anchored at epoch 0 so DST never shifts it. */ | ||
| export function formatRelativeDuration(seconds: number): string { |
There was a problem hiding this comment.
🔵 minor — formatRelativeDuration copies the existing getRelativeInterval formatting
timeQuery.ts:420 (getRelativeInterval) already formats a span with formatDuration(intervalToDuration(...)). Export one shared helper (anchored at epoch 0) and use it in both places.
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
| savedSearch.id === savedSearchId && // Make sure we've loaded the correct saved search | ||
| isSearchConfigEmpty // Only populate if URL doesn't have explicit config | ||
| ) { | ||
| const nowMs = performance.timeOrigin + performance.now(); |
There was a problem hiding this comment.
🔵 minor — performance.timeOrigin + performance.now() is used to get around the Date.now() lint rule
Here and in openSaveSearchModal, both inside an effect/callback, which the rule's message says is fine. Use Date.now() with // eslint-disable-next-line no-restricted-syntax, as parseRelativeTimeQuery in timeQuery.ts already does, so the intent stays obvious.
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
| return ( | ||
| <Group gap={6} data-testid="save-search-time-range"> | ||
| <Text size="xs">{describeOpensWith(opensWith)}</Text> | ||
| <Tooltip label={TOOLTIP} multiline w={280} withArrow> |
There was a problem hiding this comment.
🔵 minor — Keyboard users can't open the tooltip that explains how the range is saved
The Tooltip wraps a bare IconInfoCircle SVG, which can't take focus, so the explanation only shows on mouse hover. Wrap the icon in a focusable element such as <ActionIcon variant="subtle" aria-label=…>, or show the text inline under the label.
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
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.
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./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._.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).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)