Skip to content

Conversation

@brandon-pereira
Copy link
Member

@brandon-pereira brandon-pereira commented Oct 27, 2025

Adds "Relative Time" switch to TimePicker component (if relative time is supported by parent). When enabled, searches will work similar to Live Tail but be relative to the option selected.

Screenshot 2025-10-27 at 2 05 25 PM

Some notes:

  1. When relative is enabled, I disabled very large time ranges to prioritize performance.
  2. If you select "Last 15 mins" then reload, the Input will save "Live Tail" because these are the same option, this should be an edge case.
  3. In the future, we might want to make "Relative Time" the default, but I didn't want to immediately do that. We could probably improve the UX further (cc @elizabetdev).
  4. Moves a lot of the "Live Tail" logic out of various spots and centralizes it in a unified spot to support other values

Fixes HDX-2653

@changeset-bot
Copy link

changeset-bot bot commented Oct 27, 2025

🦋 Changeset detected

Latest commit: 71e24da

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@hyperdx/app Patch
@hyperdx/api Patch

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

@vercel
Copy link

vercel bot commented Oct 27, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
hyperdx-v2-oss-app Ready Ready Preview Comment Oct 29, 2025 3:11pm

@claude
Copy link

claude bot commented Oct 27, 2025

Code Review

Critical Issues

✅ No critical issues found.

Minor Observations

  • ⚠️ Type safety: DURATIONS record uses any type in utils.ts:114 → Consider typing as Record<string, Duration>
  • ⚠️ Optional chaining: handleRelativeSearch callback in TimePicker.tsx uses optional chaining but is always defined when switch is rendered → Consider guarding the switch render instead
  • ⚠️ URL state management: liveInterval defaults to LIVE_TAIL_DURATION_MS but could be more explicit about when it's set → Current implementation is acceptable but could benefit from comment

Positive Notes

  • ✅ Good test coverage with comprehensive e2e tests
  • ✅ Proper state management with URL query params
  • ✅ Clean separation of concerns (utils, components, page logic)
  • ✅ Follows Mantine UI patterns consistently
  • ✅ Backward compatibility maintained (Live Tail still works as before)

Overall: Well-implemented feature with good architecture. The minor items above are suggestions, not blockers.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 27, 2025

E2E Test Results

All tests passed • 39 passed • 3 skipped • 298s

Status Count
✅ Passed 39
❌ Failed 0
⚠️ Flaky 0
⏭️ Skipped 3

View full report →

@pulpdrew
Copy link
Contributor

pulpdrew commented Oct 28, 2025

When I select something like "Last 3 hours" in non-relative time, then click "Resume Live Tail," live tailing is resumed for the last 15 minutes, but the time picker text box still shows the time range corresponding to the last 3 hours.

See the mismatch between the histogram data (15 minutes) and the time picker (3 hours):

Screenshot 2025-10-28 at 8 37 14 AM

Also in this scenario, despite the fact that HyperDX is live tailing, the Live Tail button is not selected (it is if I try the same on main).

Screenshot 2025-10-28 at 8 38 06 AM

@pulpdrew
Copy link
Contributor

I'm seeing a bunch of console errors about duplicate keys

Screenshot 2025-10-28 at 8 42 32 AM

@pulpdrew
Copy link
Contributor

Would it make sense to toggle "Relative Time" off if the user selects an absolute time range from the "Time Range" or "Around a time" panels? Because once they do that, the time is no longer relative, right?

Also a related side-note, to me the Relative Time toggle being where it is seems slightly confusing because it doesn't seem to have any impact on the "Time Range" or "Around a time" panels. It only really seems to impact the "Last N " buttons, but it is visually separated from them.

@pulpdrew
Copy link
Contributor

One minor thing I notice is that the relative time switch state is not persisted when I share the URL, despite the relative live tailing behavior being persisted

Screen.Recording.2025-10-28.at.8.57.33.AM.mov

@brandon-pereira
Copy link
Member Author

brandon-pereira commented Oct 28, 2025

@pulpdrew

  1. Good catch, I refactored the code and missed a gate on that flow. I have fixed this, it should have fixed both scenarios
  2. Fixed the key issue
  3. I agree, I made it so when Relative Time is enabled, it disables the form entirely… not great, I'd love @elizabetdev's thoughts when she's back. I also swapped the checkbox and time toggles to help streamline it.
  4. I decided to persist the relative time toggle only when the selected range isn’t the default (15 min). That keeps backward compatibility for first-time loads, but still makes the shared URL behave as expected when a custom Live Tail range is used.

Thanks for a thorough QA, I appreciate the feedback!

Edit: One of my tests is flaky - I need to investigate more... Feel free to do a second pass of the logic though 🙏

pulpdrew
pulpdrew previously approved these changes Oct 29, 2025
Copy link
Contributor

@pulpdrew pulpdrew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Happy to re-review after any test fixes

@brandon-pereira
Copy link
Member Author

@pulpdrew I believe I actually was able to fix the test yesterday - but I got a merge conflict I had to resolve which marked the review as stale. Mind re-approving? Nothing has changed.

@kodiakhq kodiakhq bot merged commit de0b4fc into main Oct 29, 2025
10 checks passed
@kodiakhq kodiakhq bot deleted the brandon/relative-live branch October 29, 2025 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants