-
Notifications
You must be signed in to change notification settings - Fork 0
feat(ui): add IntervalSelector component #314
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughA new interval selector component system is introduced, featuring a Select-based control that enables users to choose from predefined past intervals (1h, 6h, 12h, 1d, 2d, 1w, 1mo) or select custom ranges via an integrated calendar picker, with state management and utility functions for interval matching and option discovery. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant IntervalSelector
participant IntervalSelectorContent
participant Calendar
User->>IntervalSelector: Click preset option
IntervalSelector->>IntervalSelector: Update value & state
IntervalSelector->>User: Call onIntervalChange callback
User->>IntervalSelectorContent: Click calendar button
IntervalSelectorContent->>IntervalSelectorContent: Set calendarOpen = true
IntervalSelectorContent->>Calendar: Render calendar in range mode
User->>Calendar: Select date range
Calendar->>IntervalSelectorContent: Range selection (from, to)
IntervalSelectorContent->>IntervalSelectorContent: Validate range
IntervalSelectorContent->>User: Call onCalendarSelect callback
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Pull request overview
This PR introduces a new IntervalSelector component for selecting time intervals. The component supports both predefined time intervals (like "Past 1 hour", "Past 6 hours") and custom date ranges via a calendar picker.
Changes:
- New IntervalSelector component with utility functions, main component, content subcomponent, and Storybook story
- Exports added to main component index
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| interval-selector.utils.ts | Defines interval options and utility functions for finding and matching intervals |
| interval-selector.tsx | Main component with state management for select and calendar open states |
| interval-selector-content.tsx | Renders either calendar or list of preset intervals based on state |
| interval-selector.stories.tsx | Storybook story demonstrating component usage |
| interval-selector/index.ts | Exports component and utilities |
| components/index.ts | Adds IntervalSelector to main exports |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
libs/react/ui/src/components/interval-selector/interval-selector.tsx
Outdated
Show resolved
Hide resolved
libs/react/ui/src/components/interval-selector/interval-selector.tsx
Outdated
Show resolved
Hide resolved
libs/react/ui/src/components/interval-selector/interval-selector-content.tsx
Outdated
Show resolved
Hide resolved
libs/react/ui/src/components/interval-selector/interval-selector-content.tsx
Outdated
Show resolved
Hide resolved
libs/react/ui/src/components/interval-selector/interval-selector.utils.ts
Outdated
Show resolved
Hide resolved
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.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@libs/react/ui/src/components/interval-selector/interval-selector.tsx`:
- Around line 47-52: The calendar selection handler handleCalendarSelect doesn't
close the dropdown after a full range is chosen; update it so that when
range?.from and range?.to are present you call onIntervalChange({start:
range.from, end: range.to}), onValueChange?.('custom') and then call
setSelectOpen(false) (same behavior as handleOptionSelect) to ensure the
dropdown is dismissed immediately after selection.
🧹 Nitpick comments (2)
libs/react/ui/src/components/interval-selector/interval-selector.stories.tsx (1)
36-42: Consider removing unusedargssincerenderoverrides them.The
argsobject at lines 37-40 is not used because therenderfunction returns<ControlledIntervalSelector />which manages its own state. These args appear in Storybook's Controls panel but don't affect the rendered component.♻️ Suggested simplification
export const Default: Story = { - args: { - interval: intervalToNowFromDuration({hours: 1}), - onIntervalChange: () => undefined, - }, render: () => <ControlledIntervalSelector />, };libs/react/ui/src/components/interval-selector/interval-selector-content.tsx (1)
50-60: Unnecessary truthiness check for required prop.The
intervalprop is required (not optional) inIntervalSelectorContentProps, so theinterval ? ... : undefinedcheck is redundant.♻️ Suggested simplification
<Calendar mode="range" - selected={ - interval - ? ({ - from: interval.start, - to: interval.end, - } as DayPickerDateRange) - : undefined - } + selected={{ + from: interval.start, + to: interval.end, + } as DayPickerDateRange} onSelect={handleCalendarSelect}
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
libs/react/ui/src/components/index.tslibs/react/ui/src/components/interval-selector/index.tslibs/react/ui/src/components/interval-selector/interval-selector-content.tsxlibs/react/ui/src/components/interval-selector/interval-selector.stories.tsxlibs/react/ui/src/components/interval-selector/interval-selector.tsxlibs/react/ui/src/components/interval-selector/interval-selector.utils.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*
⚙️ CodeRabbit configuration file
We handle errors at the edge of our applications in most cases. Do not recommend to add error handling around every single function. We prefer them to bubble up and be handled at upper layers.
Files:
libs/react/ui/src/components/interval-selector/index.tslibs/react/ui/src/components/interval-selector/interval-selector-content.tsxlibs/react/ui/src/components/interval-selector/interval-selector.tsxlibs/react/ui/src/components/index.tslibs/react/ui/src/components/interval-selector/interval-selector.utils.tslibs/react/ui/src/components/interval-selector/interval-selector.stories.tsx
🧬 Code graph analysis (4)
libs/react/ui/src/components/interval-selector/interval-selector-content.tsx (3)
libs/react/ui/src/components/select/select.tsx (3)
SelectContent(212-212)SelectItem(214-214)SelectSeparator(215-215)libs/react/ui/src/components/calendar/calendar.tsx (1)
Calendar(8-90)libs/react/ui/src/components/interval-selector/interval-selector.utils.ts (1)
PAST_INTERVALS(14-22)
libs/react/ui/src/components/interval-selector/interval-selector.tsx (4)
libs/react/ui/src/components/interval-selector/interval-selector.utils.ts (1)
findOption(24-26)libs/react/ui/src/utils/date.ts (1)
intervalToNowFromDuration(34-51)libs/react/ui/src/components/select/select.tsx (3)
Select(208-208)SelectTrigger(211-211)SelectValue(210-210)libs/react/ui/src/components/interval-selector/interval-selector-content.tsx (1)
IntervalSelectorContent(20-97)
libs/react/ui/src/components/interval-selector/interval-selector.utils.ts (1)
libs/react/ui/src/utils/date.ts (1)
intervalToNowFromDuration(34-51)
libs/react/ui/src/components/interval-selector/interval-selector.stories.tsx (2)
libs/react/ui/src/utils/date.ts (1)
intervalToNowFromDuration(34-51)libs/react/ui/src/components/interval-selector/interval-selector.utils.ts (1)
findOptionValueForInterval(28-54)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Agent
- GitHub Check: Continuous integration
🔇 Additional comments (12)
libs/react/ui/src/components/interval-selector/interval-selector.utils.ts (3)
1-12: LGTM!The type definitions are well-structured.
IntervalOptionTypeclearly distinguishes between predefined past intervals and custom ranges, andIntervalOptioncaptures all necessary properties including the optionaldurationfor past intervals.
14-26: LGTM!The
PAST_INTERVALSconstant provides a comprehensive set of commonly used time intervals, andfindOptionis a clean lookup implementation.
28-54: This concern is theoretical and not applicable to the actual data flow.While
NormalizedIntervaltype definition does allowstartandendas numbers, the function only receives intervals fromintervalToNowFromDuration(), which always returns Date objects. All code paths in the codebase produce Date objects, never raw timestamps. Adding defensive type checks here contradicts the project's error handling approach (errors should bubble up at application edges, not be handled throughout).Likely an incorrect or invalid review comment.
libs/react/ui/src/components/index.ts (1)
26-26: LGTM!The export is correctly placed in alphabetical order, following the existing barrel export pattern.
libs/react/ui/src/components/interval-selector/index.ts (1)
1-2: LGTM!Clean barrel export that exposes both the component and its utilities.
libs/react/ui/src/components/interval-selector/interval-selector.stories.tsx (1)
19-34: LGTM!The
ControlledIntervalSelectordemonstrates a clean pattern for managing the bidirectional state betweenintervalandvalue, correctly falling back to'custom'when the interval doesn't match a predefined option.libs/react/ui/src/components/interval-selector/interval-selector-content.tsx (2)
32-36: Consider UX for partial range selection.
handleCalendarSelectsilently ignores selections until bothfromandtoare defined. This is correct for ensuring a complete range, but the user receives no visual feedback when clicking a single date. This is a common pattern for range pickers though.
73-96: LGTM!The dropdown list view cleanly renders predefined intervals with keyboard shortcuts and provides a clear button to open the calendar picker.
libs/react/ui/src/components/interval-selector/interval-selector.tsx (4)
1-19: LGTM!Clean imports and a well-structured interface. The separation of required
interval/onIntervalChange(for the actual date range) from optionalvalue/onValueChange(for tracking selected option) provides good flexibility for consumers.
30-33: LGTM!Good separation of local UI state (
selectOpen,calendarOpen) from the controlled interval data. DerivingintervalDisplayfrom theintervalprop ensures the display always reflects the actual data state.
35-45: LGTM!The handler correctly:
- Notifies the parent of the value change
- Derives the new interval from the option's duration
- Closes the dropdown
The guard on
option?.durationis defensive, though in practicefindOptionshould always succeed since the Select options come fromPAST_INTERVALS.
54-78: LGTM!The JSX structure is well-organized:
- The
onOpenChangeguard on line 61 correctly prevents the Select from closing while the calendar is active, which is needed to keep the calendar overlay visible.- Props are properly wired to
IntervalSelectorContent.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
libs/react/ui/src/components/interval-selector/interval-selector.tsx
Outdated
Show resolved
Hide resolved
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
…e recent interval detection
|
It looks like the implementation does not exactly match the expected outcome, |
…estions components for enhanced interval selection
… detection and parsing
…nterval for absolute interval from Calendar
… and maintainability (#334) * refactor(interval-selector): restructure interval selector hooks for improved modularity and maintainability * refactor(interval-selector): ensure selectedLabel is cleared consistently and improve navigation handling * refactor(interval-selector): remove selection prop from IntervalSelectorCalendar and streamline date range handling * refactor(interval-selector): modularize utility functions and enhance calendar interval handling * Ensure correct behaviour of calendar selector * Move tests to correct location * Do not make calendarInterval a function * Refactor to have a manageable state * Fix type * Ensure shortcut is linked with current focus state * minor fixes * Approximate interval to closest duration shortcut --------- Co-authored-by: Noe Charmet <noe.charmet@shipfox.io>
…suggestions components
… for accurate date range handling
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.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
libs/react/ui/src/components/interval-selector/hooks/use-interval-selector.ts
Outdated
Show resolved
Hide resolved
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.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
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.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.
Note
Introduces a new
IntervalSelectorcomponent suite and replaces dashboard "time period" with a sharedIntervalSelectionstate.components/interval-selector(input, suggestions, calendar, hooks, types, stories) with keyboard navigation, popover container support, and invalid-input shake animation (index.css)selection(replacestimePeriod),PageToolbarembedsIntervalSelector,AnalyticsPage/JobsPagesimplified to use context-driven intervalcontainer; components index exportsinterval-selector@radix-ui/react-dismissable-layerdependency, vitest split configs (storybook/unit) with setup cleanup; changeset for minor releaseWritten by Cursor Bugbot for commit 103793b. This will update automatically on new commits. Configure here.