feat: weekly mode for charts#42
Conversation
WalkthroughThis update introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Page
participant PeriodSelect
participant Chart
User->>Page: Loads page
Page->>PeriodSelect: Passes current date range
PeriodSelect->>User: Renders period dropdown (daily/weekly)
User->>PeriodSelect: Selects period
PeriodSelect->>Page: Emits selected period
Page->>Chart: Passes range and period
Chart->>Chart: Aggregates data by period (daily/weekly)
Chart->>User: Displays chart with formatted labels/tooltips
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
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
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 5
🧹 Nitpick comments (1)
apps/web-app/app/components/PeriodSelect.vue (1)
60-64: Add safety check for edge cases.The watcher could fail if
periods.valueis empty or if the first item doesn't have a value property.// Ensure the model value is always a valid period watch(periods, () => { - if (!periods.value.some((p) => p.value === model.value) && periods.value[0]?.value) { - model.value = periods.value[0]?.value + if (!periods.value.some((p) => p.value === model.value)) { + const firstPeriod = periods.value[0]?.value + if (firstPeriod) { + model.value = firstPeriod + } } })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/web-app/app/components/Navigation.vue(1 hunks)apps/web-app/app/components/PeriodSelect.vue(1 hunks)apps/web-app/app/components/chart/KitchenChecks.client.vue(5 hunks)apps/web-app/app/components/chart/KitchenRevenue.client.vue(5 hunks)apps/web-app/app/components/chart/NetworkChecks.client.vue(5 hunks)apps/web-app/app/components/chart/NetworkRevenue.client.vue(5 hunks)apps/web-app/app/pages/kitchen/[id]/finance.vue(1 hunks)apps/web-app/app/pages/network/index.vue(3 hunks)apps/web-app/i18n/locales/ru-RU.json(1 hunks)
⏰ 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 (11)
apps/web-app/i18n/locales/ru-RU.json (1)
137-139: Good localization practice.The addition of the
network.metricstranslation key under theappnamespace follows the established i18n structure and enables proper internationalization for the network metrics feature.apps/web-app/app/components/Navigation.vue (1)
133-133: Excellent internationalization improvement.Replacing the hardcoded Russian string with the translation key
t('app.network.metrics')properly enables localization and maintains consistency with other navigation items.apps/web-app/app/pages/kitchen/[id]/finance.vue (1)
3-6: Well-integrated period selection feature.The addition of flexbox styling and the new
PeriodSelectcomponent alongside the existingDateRangePickerprovides a clean, organized layout for the period selection controls. The two-way binding and prop passing are implemented correctly.apps/web-app/app/pages/network/index.vue (4)
2-2: Good internationalization of page header.Adding the localized Header component with the translation key ensures consistent internationalization across the application.
5-8: Consistent UI layout for period controls.The flexbox styling and PeriodSelect component integration matches the pattern used in the kitchen finance page, providing a consistent user experience across the application.
28-28: Proper setup of internationalization composable.The
useI18n()composable is correctly imported and used for translation functionality.
39-41: Good document head management.Using
useHeadwith the localized title ensures proper SEO and browser tab title management with internationalization support.apps/web-app/app/components/PeriodSelect.vue (2)
1-10: Well-structured component template.The USelect component is properly configured with appropriate styling and two-way binding. The UI configuration for the trailing icon rotation is a nice touch for user experience.
24-57: Smart period selection logic.The dynamic period options based on date range length provide good UX by limiting choices to meaningful options for different time spans.
apps/web-app/app/components/chart/NetworkChecks.client.vue (1)
150-157: LGTM!The
formatTotalLabelfunction correctly handles period-aware label formatting with proper Russian pluralization.apps/web-app/app/components/chart/KitchenChecks.client.vue (1)
147-151: LGTM!The total calculation correctly filters out zero values and computes the average properly.
|
|
||
| for (const d of allDates) { | ||
| // All in one point | ||
| const dateStr = format(d, 'yyyy-MM-dd') | ||
| const value = values.find((d) => d.date.startsWith(dateStr)) | ||
|
|
||
| total += value?.total ?? 0 | ||
| checks += value?.checks ?? 0 | ||
| averageCheck += value?.averageCheck ?? 0 | ||
| averageTotal += value?.averageTotal ?? 0 | ||
| } | ||
|
|
||
| points.push({ | ||
| start: date, | ||
| end: dateTo, | ||
| total, | ||
| checks, | ||
| averageCheck: averageCheck / 7, | ||
| averageTotal, |
There was a problem hiding this comment.
Fix averaging logic and naming inconsistency.
The weekly aggregation has two issues:
- averageCheck calculation assumes all 7 days have data
- averageTotal is summed rather than averaged, which seems inconsistent with its name
Apply this diff to fix the issues:
let total = 0
let checks = 0
let averageCheck = 0
let averageTotal = 0
+ let daysWithData = 0
for (const d of allDates) {
// All in one point
const dateStr = format(d, 'yyyy-MM-dd')
const value = values.find((d) => d.date.startsWith(dateStr))
total += value?.total ?? 0
checks += value?.checks ?? 0
- averageCheck += value?.averageCheck ?? 0
- averageTotal += value?.averageTotal ?? 0
+ if (value?.averageCheck) {
+ averageCheck += value.averageCheck
+ daysWithData++
+ }
+ if (value?.averageTotal) {
+ averageTotal += value.averageTotal
+ }
}
points.push({
start: date,
end: dateTo,
total,
checks,
- averageCheck: averageCheck / 7,
- averageTotal,
+ averageCheck: daysWithData > 0 ? averageCheck / daysWithData : 0,
+ averageTotal: daysWithData > 0 ? averageTotal / daysWithData : 0,
})📝 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.
| for (const d of allDates) { | |
| // All in one point | |
| const dateStr = format(d, 'yyyy-MM-dd') | |
| const value = values.find((d) => d.date.startsWith(dateStr)) | |
| total += value?.total ?? 0 | |
| checks += value?.checks ?? 0 | |
| averageCheck += value?.averageCheck ?? 0 | |
| averageTotal += value?.averageTotal ?? 0 | |
| } | |
| points.push({ | |
| start: date, | |
| end: dateTo, | |
| total, | |
| checks, | |
| averageCheck: averageCheck / 7, | |
| averageTotal, | |
| let total = 0 | |
| let checks = 0 | |
| let averageCheck = 0 | |
| let averageTotal = 0 | |
| + let daysWithData = 0 | |
| for (const d of allDates) { | |
| // All in one point | |
| const dateStr = format(d, 'yyyy-MM-dd') | |
| const value = values.find((d) => d.date.startsWith(dateStr)) | |
| total += value?.total ?? 0 | |
| checks += value?.checks ?? 0 | |
| - averageCheck += value?.averageCheck ?? 0 | |
| - averageTotal += value?.averageTotal ?? 0 | |
| + if (value?.averageCheck) { | |
| + averageCheck += value.averageCheck | |
| + daysWithData++ | |
| + } | |
| + if (value?.averageTotal) { | |
| + averageTotal += value.averageTotal | |
| + } | |
| } | |
| points.push({ | |
| start: date, | |
| end: dateTo, | |
| total, | |
| checks, | |
| - averageCheck: averageCheck / 7, | |
| - averageTotal, | |
| + averageCheck: daysWithData > 0 | |
| + ? averageCheck / daysWithData | |
| + : 0, | |
| + averageTotal: daysWithData > 0 | |
| + ? averageTotal / daysWithData | |
| + : 0, | |
| }) |
🤖 Prompt for AI Agents
In apps/web-app/app/components/chart/NetworkRevenue.client.vue around lines 113
to 131, the averaging logic for weekly aggregation is incorrect: averageCheck is
divided by 7 regardless of how many days have data, and averageTotal is summed
instead of averaged. Fix this by counting the number of days with data and
dividing both averageCheck and averageTotal sums by that count to get correct
averages. Also, ensure variable names consistently reflect their purpose.
| label: 'По дням', | ||
| value: 'daily', |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use internationalization for period labels.
The period labels are hardcoded in Russian, which breaks the internationalization pattern used throughout the application. Consider using translation keys instead.
+const { t } = useI18n()
+
const periods = computed<{ label: string, value: Period }[]>(() => {
if (days.value.length <= 14) {
return [
{
- label: 'По дням',
+ label: t('common.period.daily'),
value: 'daily',
},
]
}
if (days.value.length <= 90) {
return [
{
- label: 'По дням',
+ label: t('common.period.daily'),
value: 'daily',
},
{
- label: 'По неделям',
+ label: t('common.period.weekly'),
value: 'weekly',
},
]
}
return [
{
- label: 'По неделям',
+ label: t('common.period.weekly'),
value: 'weekly',
},
// {
- // label: 'По месяцам',
+ // label: t('common.period.monthly'),
// value: 'monthly'
// }
]
})Don't forget to add the corresponding translation keys to the locale files.
Also applies to: 37-38, 41-42, 49-50
🤖 Prompt for AI Agents
In apps/web-app/app/components/PeriodSelect.vue around lines 28-29 and also
lines 37-38, 41-42, and 49-50, the period labels are hardcoded in Russian, which
breaks the app's internationalization pattern. Replace these hardcoded Russian
strings with translation keys using the app's i18n method (e.g.,
$t('translation.key')). Then add the corresponding translation keys and their
Russian values to the locale files to maintain proper localization support.
|
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
apps/web-app/app/components/chart/KitchenRevenue.client.vue (1)
111-141: Fix inconsistent aggregation for commonTotal.The
commonTotalis still being summed instead of averaged, which is inconsistent with howaverageCheckis handled. This issue was previously raised but only partially addressed.Apply this diff to fix the averaging:
let daysWithValues = 0 + let daysWithMetrics = 0 let total = 0 let checks = 0 let averageCheck = 0 let commonTotal = 0 for (const d of allDates) { // All in one point const dateStr = format(d, 'yyyy-MM-dd') const value = values.find((d) => d.date.startsWith(dateStr)) const metric = metrics.find((d) => d.date.startsWith(dateStr)) total += value?.total ?? 0 checks += value?.checks ?? 0 - commonTotal += metric?.averageTotal ?? 0 if (value?.averageCheck) { averageCheck += value.averageCheck daysWithValues++ } + + if (metric?.averageTotal) { + commonTotal += metric.averageTotal + daysWithMetrics++ + } } points.push({ start: date, end: dateTo, total, checks, averageCheck: daysWithValues > 0 ? averageCheck / daysWithValues : 0, - commonTotal, + commonTotal: daysWithMetrics > 0 ? commonTotal / daysWithMetrics : 0, })apps/web-app/app/components/chart/NetworkRevenue.client.vue (1)
99-142: Fix inconsistent averaging logic foraverageTotal.While
averageCheckis now correctly averaged over days with data,averageTotalis still being summed rather than averaged, which is inconsistent with its name and the past review feedback.Apply this diff to fix the averaging logic:
let daysWithValues = 0 + let daysWithTotalValues = 0 let total = 0 let checks = 0 let averageCheck = 0 let averageTotal = 0 for (const d of allDates) { // All in one point const dateStr = format(d, 'yyyy-MM-dd') const value = values.find((d) => d.date.startsWith(dateStr)) total += value?.total ?? 0 checks += value?.checks ?? 0 - averageTotal += value?.averageTotal ?? 0 if (value?.averageCheck) { averageCheck += value.averageCheck daysWithValues++ } + if (value?.averageTotal) { + averageTotal += value.averageTotal + daysWithTotalValues++ + } } points.push({ start: date, end: dateTo, total, checks, averageCheck: daysWithValues > 0 ? averageCheck / daysWithValues : 0, - averageTotal, + averageTotal: daysWithTotalValues > 0 ? averageTotal / daysWithTotalValues : 0, })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/web-app/app/components/chart/KitchenChecks.client.vue(5 hunks)apps/web-app/app/components/chart/KitchenRevenue.client.vue(5 hunks)apps/web-app/app/components/chart/NetworkChecks.client.vue(5 hunks)apps/web-app/app/components/chart/NetworkRevenue.client.vue(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web-app/app/components/chart/NetworkChecks.client.vue
- apps/web-app/app/components/chart/KitchenChecks.client.vue
⏰ 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 (4)
apps/web-app/app/components/chart/NetworkRevenue.client.vue (4)
55-62: LGTM! Good type design for date ranges.The updated
DataRecordtype withstartandenddates properly supports the new period-based data aggregation for daily, weekly, and monthly views.
79-79: LGTM! Correct week configuration.Setting
weekStartsOn: 1(Monday) is appropriate for the weekly aggregation.
83-97: LGTM! Clean daily data processing.The daily period implementation correctly maps each date to a record with matching start/end dates and handles missing data gracefully with default values.
167-173: LGTM! Well-structured label formatting.The
formatTotalLabelfunction provides clean period-specific formatting with proper Russian pluralization support.
| <div> | ||
| <p class="text-xs text-muted uppercase mb-1.5"> | ||
| Выручка за {{ data.length }} {{ pluralizationRu(data.length, ['день', 'дня', 'дней']) }} | ||
| {{ formatTotalLabel('Выручка', data.length) }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use internationalization for the revenue label.
The hardcoded Russian string 'Выручка' should use a translation key for proper internationalization support, especially since the PR summary mentions improved internationalization.
- {{ formatTotalLabel('Выручка', data.length) }}
+ {{ formatTotalLabel($t('revenue'), data.length) }}📝 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.
| {{ formatTotalLabel('Выручка', data.length) }} | |
| {{ formatTotalLabel($t('revenue'), data.length) }} |
🤖 Prompt for AI Agents
In apps/web-app/app/components/chart/KitchenRevenue.client.vue at line 6,
replace the hardcoded Russian string 'Выручка' with a translation key using the
project's internationalization method (e.g., $t or similar). This ensures the
label is properly localized according to the user's language settings. Use the
appropriate translation key instead of the literal string.
| monthly: `${formatDate(d.start)} - ${formatDate(d.end)}`, | ||
| })[period] | ||
|
|
||
| return `<strong>${title}</strong><br> ${d.checks} ${pluralizationRu(d.checks, ['чек', 'чека', 'чеков'])}, средний ${formatNumber(d.averageCheck)}<br> Выручка: ${formatNumber(d.total)}<br> Средняя по сети: ${formatNumber(d.commonTotal)}` |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use internationalization for tooltip text.
The tooltip contains hardcoded Russian strings that should use translation keys for consistency with the internationalization improvements mentioned in the PR.
Consider using translation keys for the tooltip text:
- 'чек/чека/чеков' (check/checks)
- 'средний' (average)
- 'Выручка' (Revenue)
- 'Средняя по сети' (Network average)
🤖 Prompt for AI Agents
In apps/web-app/app/components/chart/KitchenRevenue.client.vue at line 193, the
tooltip text contains hardcoded Russian strings. Replace these hardcoded strings
with appropriate translation keys using the project's internationalization
(i18n) system for 'чек/чека/чеков', 'средний', 'Выручка', and 'Средняя по сети'.
Use the translation function to fetch localized strings and ensure pluralization
is handled via i18n utilities instead of hardcoded plural forms.
| <div> | ||
| <p class="text-xs text-muted uppercase mb-1.5"> | ||
| Выручка за {{ data.length }} {{ pluralizationRu(data.length, ['день', 'дня', 'дней']) }} | ||
| {{ formatTotalLabel('Выручка', data.length) }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace hardcoded Russian text with translation key.
For consistency with the internationalization improvements mentioned in the PR, use a translation key instead of the hardcoded Russian string.
- {{ formatTotalLabel('Выручка', data.length) }}
+ {{ formatTotalLabel($t('revenue'), data.length) }}📝 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.
| {{ formatTotalLabel('Выручка', data.length) }} | |
| {{ formatTotalLabel($t('revenue'), data.length) }} |
🤖 Prompt for AI Agents
In apps/web-app/app/components/chart/NetworkRevenue.client.vue at line 6,
replace the hardcoded Russian string 'Выручка' in the formatTotalLabel call with
the appropriate translation key from the i18n setup. This involves importing the
translation function if not already imported, then using it to get the
translated string instead of the hardcoded text to maintain consistency with
internationalization practices.
| function formatTemplate(d: DataRecord) { | ||
| const title = ({ | ||
| daily: `${formatDate(d.start)}, ${format(d.start, 'eeee', { locale: ru })}`, | ||
| weekly: `${formatDate(d.start)} - ${formatDate(d.end)}`, | ||
| monthly: `${formatDate(d.start)} - ${formatDate(d.end)}`, | ||
| })[period] | ||
|
|
||
| return `<strong>${title}</strong><br> ${d.checks} ${pluralizationRu(d.checks, ['чек', 'чека', 'чеков'])}, средний ${formatNumber(d.averageCheck)}<br> Выручка: ${formatNumber(d.total)}<br> Средняя у кухни: ${formatNumber(d.averageTotal)}` | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Internationalize hardcoded strings in tooltip template.
The tooltip template contains several hardcoded Russian strings that should use translation keys for consistency with the internationalization improvements.
Consider using translation keys for:
- "чек/чека/чеков" (checks pluralization)
- "средний" (average)
- "Выручка:" (Revenue:)
- "Средняя у кухни:" (Kitchen average:)
Example:
- return `<strong>${title}</strong><br> ${d.checks} ${pluralizationRu(d.checks, ['чек', 'чека', 'чеков'])}, средний ${formatNumber(d.averageCheck)}<br> Выручка: ${formatNumber(d.total)}<br> Средняя у кухни: ${formatNumber(d.averageTotal)}`
+ return `<strong>${title}</strong><br> ${d.checks} ${$t('checks', d.checks)}, ${$t('average')} ${formatNumber(d.averageCheck)}<br> ${$t('revenue')}: ${formatNumber(d.total)}<br> ${$t('kitchenAverage')}: ${formatNumber(d.averageTotal)}`📝 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.
| function formatTemplate(d: DataRecord) { | |
| const title = ({ | |
| daily: `${formatDate(d.start)}, ${format(d.start, 'eeee', { locale: ru })}`, | |
| weekly: `${formatDate(d.start)} - ${formatDate(d.end)}`, | |
| monthly: `${formatDate(d.start)} - ${formatDate(d.end)}`, | |
| })[period] | |
| return `<strong>${title}</strong><br> ${d.checks} ${pluralizationRu(d.checks, ['чек', 'чека', 'чеков'])}, средний ${formatNumber(d.averageCheck)}<br> Выручка: ${formatNumber(d.total)}<br> Средняя у кухни: ${formatNumber(d.averageTotal)}` | |
| } | |
| function formatTemplate(d: DataRecord) { | |
| const title = ({ | |
| daily: `${formatDate(d.start)}, ${format(d.start, 'eeee', { locale: ru })}`, | |
| weekly: `${formatDate(d.start)} - ${formatDate(d.end)}`, | |
| monthly: `${formatDate(d.start)} - ${formatDate(d.end)}`, | |
| })[period] | |
| return `<strong>${title}</strong><br> ${d.checks} ${$t('checks', d.checks)}, ${$t('average')} ${formatNumber(d.averageCheck)}<br> ${$t('revenue')}: ${formatNumber(d.total)}<br> ${$t('kitchenAverage')}: ${formatNumber(d.averageTotal)}` | |
| } |
🤖 Prompt for AI Agents
In apps/web-app/app/components/chart/NetworkRevenue.client.vue around lines 183
to 191, the tooltip template contains hardcoded Russian strings such as the
plural forms of "чек", "средний", "Выручка:", and "Средняя у кухни:". Replace
these hardcoded strings with appropriate translation keys from the i18n system
to support internationalization. Use the translation function to fetch localized
strings for these labels and ensure the pluralization function also uses
translated forms.
1 participant
Summary by CodeRabbit
New Features
Enhancements
Bug Fixes
Documentation