Repository navigation
fix(time-picker): enable live tail for 3h-1d, rename switch, add disa… #3219
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
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/app': patch | ||
| --- | ||
|
|
||
| fix: expand live tail time picker presets to 6h and clarify switch label | ||
|
|
||
| Enable 3h and 6h presets in live tail mode (previously capped at 1h in the picker, though longer ranges already worked via URL). Ranges above 6h remain disabled because the refresh tick (10s default) re-queries the full window, and 12–24h scans would be excessive. Rename the toggle switch from "Relative Time" to "Live tail ranges" to clarify that it controls which preset intervals are available, not whether live tail is active. Add a "Not available for Live Tail" tooltip to disabled presets (12h+). |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -330,7 +330,7 @@ const TimePickerComponent = ({ | |
| size="xs" | ||
| checked={isRelative} | ||
| onChange={e => setIsRelative(e.currentTarget.checked)} | ||
| label="Relative Time" | ||
| label="Live tail ranges" | ||
| labelPosition="right" | ||
| styles={{ | ||
| label: { | ||
|
|
@@ -367,18 +367,24 @@ const TimePickerComponent = ({ | |
| <Card w={180} p={0}> | ||
| <ScrollArea h={300} scrollbarSize={5}> | ||
| <Stack gap={0} p="xs"> | ||
| {relativeTimeOptions.map((item, index) => | ||
| item === 'divider' ? ( | ||
| <Divider key={index} my={4} /> | ||
| ) : ( | ||
| {relativeTimeOptions.map((item, index) => { | ||
| if (item === 'divider') { | ||
| return <Divider key={index} my={4} />; | ||
| } | ||
| const isDisabledForLiveTail = | ||
| isRelative && !item[2] && item[0] !== LIVE_TAIL_TIME_QUERY; | ||
| const button = ( | ||
| <Button | ||
| key={item[0]} | ||
| disabled={ | ||
| isRelative && | ||
| !item[2] && | ||
| item[0] !== LIVE_TAIL_TIME_QUERY | ||
| } | ||
| // Use data-disabled instead of the `disabled` prop so | ||
| // pointer-events stay active and the Tooltip can fire. | ||
| // Mantine's [data-disabled] styles handle appearance. | ||
| // aria-disabled exposes the state to screen readers, | ||
| // keyboard users, and test assertions (toBeDisabled). | ||
| data-disabled={isDisabledForLiveTail || undefined} | ||
|
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 — Presets that look disabled are still focusable and aren't announced as disabled Dropping the native 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
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 — Blocked presets are focusable and read as enabled by assistive tech Replacing native 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
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 — Disabled presets are still exposed as enabled buttons to assistive tech and keyboard users Replacing 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
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 — Disabled presets are exposed as enabled buttons to assistive tech Switching from 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
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 — Presets that are unavailable in live tail are only styled as disabled; assistive tech still reports them as active buttons Swapping 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
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 — Presets disabled only via Add 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 |
||
| aria-disabled={isDisabledForLiveTail || undefined} | ||
| onClick={() => { | ||
| if (isDisabledForLiveTail) return; | ||
| if (isRelative || item[0] === LIVE_TAIL_TIME_QUERY) { | ||
| handleRelativeSearch?.(item[0], item[1]); | ||
| } else { | ||
|
|
@@ -396,8 +402,23 @@ const TimePickerComponent = ({ | |
| > | ||
| {item[0]} | ||
| </Button> | ||
| ), | ||
| )} | ||
| ); | ||
| if (isDisabledForLiveTail) { | ||
|
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 extra wrapper div is based on a wrong comment and hides the tooltip from keyboard users The two comments contradict each other. Line 379 says 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
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 — Comment describes a wrapper div that doesn't exist and contradicts the comment above it Lines 379-383 say 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 ( | ||
| <Tooltip | ||
|
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 — Disabled-preset tooltip only opens on hover, so keyboard users who focus a disabled option never see why The buttons use aria-disabled, so they stay in the tab order, but Mantine Tooltip defaults to events={{ hover: true, focus: false }}. Pass events={{ hover: true, focus: true, touch: false }} so focusing a disabled preset also shows 'Not available for Live Tail'. This is what the code comment claims keyboard users get. 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 |
||
| key={item[0]} | ||
| label="Not available for Live Tail" | ||
| position="right" | ||
| withArrow | ||
| openDelay={0} | ||
| zIndex={getDefaultZIndex('popover') + 3} // above the Popover (+2) | ||
| > | ||
| {button} | ||
| </Tooltip> | ||
| ); | ||
| } | ||
| return button; | ||
| })} | ||
| </Stack> | ||
| </ScrollArea> | ||
| </Card> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -99,8 +99,8 @@ export const RELATIVE_TIME_OPTIONS: ( | |
| ['Last 45 minutes', ms('45m'), true], | ||
| 'divider', | ||
| ['Last 1 hour', ms('1h'), true], | ||
| ['Last 3 hours', ms('3h')], | ||
| ['Last 6 hours', ms('6h')], | ||
| ['Last 3 hours', ms('3h'), true], | ||
|
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 — Newly enabled 3h–1d live ranges and the disabled-with-tooltip behaviour have no test coverage
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 |
||
| ['Last 6 hours', ms('6h'), true], | ||
| ['Last 12 hours', ms('12h')], | ||
|
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 PR description says more presets are enabled than the diff actually enables Update the PR title and description, or the code, so they agree. The description and title ('enable live tail for 3h-1d') say 12h and 1d get 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 |
||
| 'divider', | ||
| ['Last 1 days', ms('1d')], | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔵 minor — LiveTail story doesn't pass
showLiveorisLiveMode, so it has no 'Live Tail' option and no live stylingPass
showLiveandisLiveMode(as DBSearchPage does) and use a real initial value such as 'Live Tail'. That way the story matches the live tail picker it is named for, including theitem[0] !== LIVE_TAIL_TIME_QUERYexemption.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