Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/live-tail-time-picker-presets.md
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+).
17 changes: 17 additions & 0 deletions packages/app/src/components/TimePicker/TimePicker.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -22,3 +22,20 @@ export const Default = () => {
/>
);
};

export const LiveTail = () => {
const [value, setValue] = React.useState('Live Tail');

return (
<TimePicker

Copy link
Copy Markdown
Contributor

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 showLive or isLiveMode, so it has no 'Live Tail' option and no live styling

Pass showLive and isLiveMode (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 the item[0] !== LIVE_TAIL_TIME_QUERY exemption.

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

inputValue={value}
setInputValue={setValue}
onSearch={() => {}}
onRelativeSearch={_rangeMs => {
// no-op: story only exercises the picker UI
}}
showLive
isLiveMode
/>
);
};
45 changes: 33 additions & 12 deletions packages/app/src/components/TimePicker/TimePicker.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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: {
Expand Down Expand Up @@ -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}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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 disabled means these buttons stay in the tab order, and screen readers treat them as active buttons. Add aria-disabled={isDisabledForLiveTail || undefined} next to data-disabled so assistive tech knows the control is inert. The click guard on line 385 already stops the action.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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 disabled with data-disabled puts these buttons back in the tab order with no disabled semantics. Screen readers announce them as active, and Enter does nothing silently. Add aria-disabled={isDisabledForLiveTail || undefined} next to data-disabled.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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 disabled with data-disabled keeps each preset focusable and announced as an active button. Clicking or pressing Enter just does nothing because of the if (isDisabledForLiveTail) return guard. Add aria-disabled={isDisabledForLiveTail || undefined} next to data-disabled so screen readers announce the state that sighted users get from the styling and tooltip.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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 disabled to data-disabled drops the native disabled semantics, so screen readers announce Last 2d–30d as active buttons and Enter/click silently does nothing. Add aria-disabled={isDisabledForLiveTail || undefined} next to data-disabled.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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 disabled for data-disabled keeps the button in the tab order and in the accessibility tree as an active button. Only the onClick guard stops it from doing anything. Add aria-disabled={isDisabledForLiveTail || undefined} next to data-disabled so screen readers and keyboard users are told the preset is unavailable, and so tests can assert it.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 minor — Presets disabled only via data-disabled still look enabled to keyboard users, screen readers and tests

Add aria-disabled={isDisabledForLiveTail || undefined} next to data-disabled. Today the 12h+ presets can still be focused with Tab and are announced as enabled buttons, and pressing Enter silently does nothing because of the onClick guard. Playwright's toBeDisabled() can't detect this state either. aria-disabled keeps pointer events working for the Tooltip and exposes the real state.

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 {
Expand All @@ -396,8 +402,23 @@ const TimePickerComponent = ({
>
{item[0]}
</Button>
),
)}
);
if (isDisabledForLiveTail) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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 data-disabled keeps pointer-events active; line 407 says [data-disabled] sets pointer-events:none. The first is right: Mantine's Button styles for [data-disabled] only change cursor and colour. Mantine's documented pattern for a disabled button with a tooltip puts data-disabled on the Button and passes the Button straight to <Tooltip>. Because the tooltip is attached to a non-focusable <div>, a keyboard user who tabs onto the Button never sees 'Not available for live tail'. Remove the wrapper div and its comment, pass the Button directly as the Tooltip child, and move cursor: not-allowed onto the Button (or rely on Mantine's disabled style).

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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 data-disabled keeps pointer events active so the Tooltip can fire. Lines 406-409 say [data-disabled] sets pointer-events:none and that a wrapper div catches hover. But the <Tooltip> wraps {button} directly, with no div. If the second comment were true the tooltip could never open. Delete the 406-409 comment. If pointer events really are blocked, add the wrapper it describes.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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>
Expand Down
4 changes: 2 additions & 2 deletions packages/app/src/components/TimePicker/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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],

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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

relative-time.spec.ts still only exercises intervals up to 1h, and nothing checks that 2d+ presets can't be clicked in Live Tail mode. Add 'Last 3 hours' (10800000) and 'Last 1 days' (86400000) to the interval list in should select different relative time intervals. Also add a case asserting that clicking 'Last 2 days' with the switch on leaves liveInterval unchanged.

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')],

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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 relativeSupport: true, that the switch is renamed to 'Live Tail', and that styling is manual. The diff only enables 3h and 6h, uses the label 'Live tail ranges', and relies on Mantine's [data-disabled] styles. Anyone following the description's test plan will see 12h and 1d disabled and think it's a bug. The changeset is the accurate one.

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')],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,9 @@ test.describe('Relative Time Picker', { tag: '@relative-time' }, () => {
});

test('should show Live Tail option in relative time mode', async () => {
const liveTailButton = searchPage.page
.locator('text=Live Tail')
.locator('..');
const liveTailButton = searchPage.timePicker.popover.getByRole('button', {
name: 'Live Tail',
});
await expect(liveTailButton).toBeVisible();
});
});
Expand Down
Loading