feat: date range picker for charts#29
Conversation
WalkthroughA new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DateRangePicker
participant ChartComponent
User->>DateRangePicker: Open popover / select date range
DateRangePicker->>DateRangePicker: Update selected range (predefined or manual)
DateRangePicker-->>User: Display updated range
DateRangePicker-->>ChartComponent: Emit new date range
ChartComponent->>ChartComponent: Fetch and display data for selected range
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15–20 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/web-app/app/components/DateRangePicker.vue (1)
1-54: Template structure looks good with minor internationalization concern.The template is well-structured with proper conditional rendering and responsive design. However, consider making the placeholder text "Выберите даты" (line 20) configurable or use an internationalization system for better localization support.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/web-app/app/components/DateRangePicker.vue(1 hunks)apps/web-app/app/components/chart/KitchenChecks.client.vue(3 hunks)apps/web-app/app/components/chart/KitchenRevenue.client.vue(4 hunks)apps/web-app/app/pages/kitchen/[id]/finance.vue(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
apps/web-app/app/pages/kitchen/[id]/finance.vue (1)
apps/web-app/shared/types/index.ts (1)
Range(50-53)
apps/web-app/app/components/DateRangePicker.vue (1)
apps/web-app/shared/types/index.ts (1)
Range(50-53)
⏰ 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). (1)
- GitHub Check: build
🔇 Additional comments (19)
apps/web-app/app/components/DateRangePicker.vue (9)
1-54: LGTM! Well-structured template with good UX.The template properly handles conditional rendering for date display, provides clear visual feedback, and follows good practices with truncation and accessibility considerations.
56-63: LGTM! Proper imports and locale setup.The imports are appropriate, and the Russian locale setup for DateFormatter aligns well with the UI text.
66-73: LGTM! Well-defined predefined ranges.The predefined ranges cover common use cases and the (n - 1) calculation properly creates inclusive date ranges.
75-94: Review the default date fallback in calendarRange setter.The
toCalendarDatefunction is correct, but the setter incalendarRangehas a potential issue: whennewValue.startornewValue.endis null, it defaults tonew Date(). This could create unexpected behavior during partial range selection.Consider handling null values more explicitly:
set: (newValue: { start: CalendarDate | null, end: CalendarDate | null }) => { selected.value = { - start: newValue.start ? newValue.start.toDate(getLocalTimeZone()) : new Date(), - end: newValue.end ? newValue.end.toDate(getLocalTimeZone()) : new Date(), + start: newValue.start ? newValue.start.toDate(getLocalTimeZone()) : selected.value.start, + end: newValue.end ? newValue.end.toDate(getLocalTimeZone()) : selected.value.end, } },
96-134: LGTM! Solid range selection logic.The range comparison and selection logic is well-implemented. The code properly handles date arithmetic and comparisons using the @internationalized/date library, and shows good forward compatibility by supporting months/years options.
56-73: LGTM! Clean imports and setup.The imports, DateFormatter setup, and predefined ranges are well-implemented. The "days - 1" pattern in the ranges array correctly implements "last N days" to include today as part of the range.
75-81: LGTM! Correct date conversion.The function properly handles the month index conversion (JavaScript's 0-indexed months to CalendarDate's 1-indexed months).
96-116: LGTM! Correct range comparison logic.The function properly validates whether the current selection matches a predefined range, with consistent logic that matches the
selectRangefunction.
118-134: LGTM! Clean range selection implementation.The function correctly calculates and sets date ranges, with logic that's consistent with the
isRangeSelectedfunction.apps/web-app/app/pages/kitchen/[id]/finance.vue (2)
3-5: LGTM! Clean integration of DateRangePicker.The component is properly integrated with correct v-model binding.
29-33: LGTM! Improved default range and date consistency.The change to 14 days provides a more focused initial view, and using
const todayensures consistent date references throughout the initialization.apps/web-app/app/components/chart/KitchenChecks.client.vue (3)
18-18: LGTM! Consistent height alignment across chart components.The height reduction aligns with similar changes in other chart components for visual consistency.
104-108: LGTM! Improved calculation with proper zero checks.The updated calculation logic properly handles edge cases and prevents division by zero, making the code more robust.
128-128: LGTM! Grammatical correction in Russian text.The change from "Среднее" to "Средний" correctly matches the grammatical gender of "чек" in Russian.
apps/web-app/app/components/chart/KitchenRevenue.client.vue (5)
18-18: LGTM! Consistent height with other chart components.Matches the height change in KitchenChecks for visual consistency.
59-59: LGTM! Proper type extension.The DataRecord type is correctly extended with the averageCheck field.
66-66: LGTM! Props type updated consistently.The values prop type correctly includes the new averageCheck field.
90-90: LGTM! Consistent data mapping pattern.The averageCheck field is properly mapped following the same pattern as other fields.
126-126: LGTM! Enhanced tooltip with average check information.The tooltip now provides more comprehensive information by including the average check value alongside the existing data.
| const calendarRange = computed({ | ||
| get: () => ({ | ||
| start: selected.value.start ? toCalendarDate(selected.value.start) : undefined, | ||
| end: selected.value.end ? toCalendarDate(selected.value.end) : undefined, | ||
| }), | ||
| set: (newValue: { start: CalendarDate | null, end: CalendarDate | null }) => { | ||
| selected.value = { | ||
| start: newValue.start ? newValue.start.toDate(getLocalTimeZone()) : new Date(), | ||
| end: newValue.end ? newValue.end.toDate(getLocalTimeZone()) : new Date(), | ||
| } | ||
| }, | ||
| }) |
There was a problem hiding this comment.
Fix null value handling in calendarRange setter.
The setter logic has an issue: when newValue.start or newValue.end is null, it defaults to new Date(). This means clearing a date selection unexpectedly sets it to the current date instead of keeping it unselected.
Consider this fix to handle null values properly:
set: (newValue: { start: CalendarDate | null, end: CalendarDate | null }) => {
selected.value = {
- start: newValue.start ? newValue.start.toDate(getLocalTimeZone()) : new Date(),
- end: newValue.end ? newValue.end.toDate(getLocalTimeZone()) : new Date(),
+ start: newValue.start ? newValue.start.toDate(getLocalTimeZone()) : null as any,
+ end: newValue.end ? newValue.end.toDate(getLocalTimeZone()) : null as any,
}
},Or update the Range interface to allow null values if that's the intended behavior.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const calendarRange = computed({ | |
| get: () => ({ | |
| start: selected.value.start ? toCalendarDate(selected.value.start) : undefined, | |
| end: selected.value.end ? toCalendarDate(selected.value.end) : undefined, | |
| }), | |
| set: (newValue: { start: CalendarDate | null, end: CalendarDate | null }) => { | |
| selected.value = { | |
| start: newValue.start ? newValue.start.toDate(getLocalTimeZone()) : new Date(), | |
| end: newValue.end ? newValue.end.toDate(getLocalTimeZone()) : new Date(), | |
| } | |
| }, | |
| }) | |
| const calendarRange = computed({ | |
| get: () => ({ | |
| start: selected.value.start ? toCalendarDate(selected.value.start) : undefined, | |
| end: selected.value.end ? toCalendarDate(selected.value.end) : undefined, | |
| }), | |
| set: (newValue: { start: CalendarDate | null, end: CalendarDate | null }) => { | |
| selected.value = { | |
| start: newValue.start ? newValue.start.toDate(getLocalTimeZone()) : null as any, | |
| end: newValue.end ? newValue.end.toDate(getLocalTimeZone()) : null as any, | |
| } | |
| }, | |
| }) |
🤖 Prompt for AI Agents
In apps/web-app/app/components/DateRangePicker.vue around lines 83 to 94, the
setter for calendarRange incorrectly assigns new Date() when newValue.start or
newValue.end is null, causing cleared dates to reset to the current date. To fix
this, update the setter to assign undefined or null instead of new Date() when
the values are null, preserving the cleared state properly without defaulting to
the current date.
1 participant
Summary by CodeRabbit
New Features
Enhancements
Style