feat: network metrics page#41
Conversation
WalkthroughA new "Network Metrics" feature was introduced to the web application. This includes a navigation link to a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Navigation
participant NetworkPage
participant API
participant RevenueChart
participant ChecksChart
User->>Navigation: Click "Показатели сети"
Navigation->>NetworkPage: Route to /network
NetworkPage->>API: Fetch /api/network/metrics (with period, range)
API-->>NetworkPage: Return metrics data
NetworkPage->>RevenueChart: Pass period, range, values
NetworkPage->>ChecksChart: Pass period, range, values
RevenueChart-->>User: Render revenue chart
ChecksChart-->>User: Render checks chart
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
✨ 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. 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: 2
🔭 Outside diff range comments (1)
apps/web-app/app/components/chart/NetworkChecks.client.vue (1)
1-140: Consider extracting shared chart logic into a composable.Both NetworkRevenue and NetworkChecks components share significant amounts of code (data processing, date formatting, chart configuration). This creates maintenance overhead.
Consider creating a shared composable like
useNetworkChartto reduce code duplication:// composables/useNetworkChart.ts export function useNetworkChart(period: Ref<Period>, range: Ref<Range>, values: Ref<any[]>) { const data = ref([]) const cardRef = useTemplateRef<HTMLElement | null>('cardRef') const { width } = useElementSize(cardRef) const formatNumber = new Intl.NumberFormat('ru', { style: 'currency', currency: 'RUB', maximumFractionDigits: 0 }).format function formatDate(date: Date, period: Period): string { // shared date formatting logic } // shared data processing logic // shared chart configuration return { data, cardRef, width, formatNumber, formatDate, // other shared utilities } }
🧹 Nitpick comments (6)
apps/web-app/app/components/Navigation.vue (1)
132-137: Consider using i18n for consistency.The new navigation item follows the established pattern correctly. However, while other menu items use
t('app.menu.xxx')for internationalization, this item uses a hardcoded Russian label.Consider adding an i18n key for consistency:
- label: 'Показатели сети', + label: t('app.menu.network-metrics'),apps/web-app/app/components/chart/NetworkRevenue.client.vue (3)
75-94: Optimize the data processing logic.The watch function processes data on every change, but the date string matching logic could be more efficient and robust.
Consider these improvements:
watch([() => period, () => range, () => values], () => { const dates = ({ daily: eachDayOfInterval, weekly: eachWeekOfInterval, monthly: eachMonthOfInterval, } as Record<Period, typeof eachDayOfInterval>)[period](range) + // Create a map for O(1) lookups + const valuesMap = new Map(values.map(v => [v.date.substring(0, 10), v])) data.value = dates.map((date) => { const dateStr = format(date, 'yyyy-MM-dd') - const value = values.find((d) => d.date.startsWith(dateStr)) + const value = valuesMap.get(dateStr) return { date, total: value?.total ?? 0, checks: value?.checks ?? 0, averageCheck: value?.averageCheck ?? 0, averageTotal: value?.averageTotal ?? 0, } }) }, { immediate: true })
96-105: Simplify chart configuration functions.The color and line dash array functions are overly complex for their simple purpose.
Simplify these functions:
-const color = (_: DataRecord, i: number) => ['var(--ui-secondary)', 'var(--ui-secondary)'][i] -const lineDashArray = (_: DataRecord, i: number) => [i === 0 ? undefined : 3] +const color = () => 'var(--ui-secondary)' +const lineDashArray = (_: DataRecord, i: number) => i === 0 ? undefined : [3]
126-126: Consider breaking down the complex tooltip template.The tooltip template is quite long and complex, making it hard to maintain.
Consider extracting the tooltip logic:
+function formatTooltip(d: DataRecord): string { + const dateStr = `${formatDate(d.date)}, ${format(d.date, 'eeee', { locale: ru })}` + const checksStr = `${d.checks} ${pluralizationRu(d.checks, ['чек', 'чека', 'чеков'])}` + const avgCheckStr = `средний ${formatNumber(d.averageCheck)}` + const revenueStr = `Выручка: ${formatNumber(d.total)}` + const avgTotalStr = `Средняя у кухни: ${formatNumber(d.averageTotal)}` + + return `<strong>${dateStr}</strong><br> ${checksStr}, ${avgCheckStr}<br> ${revenueStr}<br> ${avgTotalStr}` +} -const template = (d: DataRecord) => `<strong>${formatDate(d.date)}, ${format(d.date, 'eeee', { locale: ru })}</strong><br> ${d.checks} ${pluralizationRu(d.checks, ['чек', 'чека', 'чеков'])}, средний ${formatNumber(d.averageCheck)}<br> Выручка: ${formatNumber(d.total)}<br> Средняя у кухни: ${formatNumber(d.averageTotal)}` +const template = formatTooltipapps/web-app/app/components/chart/NetworkChecks.client.vue (2)
73-90: Apply the same data processing optimization as NetworkRevenue.This component has the same inefficient data lookup pattern as the NetworkRevenue component.
Apply the same optimization using a Map for O(1) lookups:
watch([() => period, () => range, () => values], () => { const dates = ({ daily: eachDayOfInterval, weekly: eachWeekOfInterval, monthly: eachMonthOfInterval, } as Record<Period, typeof eachDayOfInterval>)[period](range) + const valuesMap = new Map(values.map(v => [v.date.substring(0, 10), v])) data.value = dates.map((date) => { const dateStr = format(date, 'yyyy-MM-dd') - const value = values.find((d) => d.date.startsWith(dateStr)) + const value = valuesMap.get(dateStr) return { date, checks: value?.checks ?? 0, averageCheck: value?.averageCheck ?? 0, } }) }, { immediate: true })
96-97: Remove unused array indexing in chart configuration.The color and lineDashArray functions reference array indices that aren't used since this component only has one data series.
Simplify these functions:
-const color = (_: DataRecord, i: number) => ['var(--ui-info)', 'var(--ui-info)'][i] -const lineDashArray = (_: DataRecord, i: number) => [i === 0 ? undefined : 3] +const color = () => 'var(--ui-info)' +const lineDashArray = () => undefined
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/web-app/app/components/Navigation.vue(1 hunks)apps/web-app/app/components/chart/NetworkChecks.client.vue(1 hunks)apps/web-app/app/components/chart/NetworkRevenue.client.vue(1 hunks)apps/web-app/app/pages/network/index.vue(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 (1)
apps/web-app/app/pages/network/index.vue (1)
28-31: Fix the date range calculation.The date range calculation has an off-by-one error. Subtracting
days: 30 - 1results in a 30-day range, but the logic suggests it should be exactly 30 days from today.Apply this fix for clarity:
const range = shallowRef<Range>({ - start: sub(today, { days: 30 - 1 }), + start: sub(today, { days: 29 }), end: today, })Or if you want exactly 30 days including today:
const range = shallowRef<Range>({ - start: sub(today, { days: 30 - 1 }), + start: sub(today, { days: 30 }), end: today, })Likely an incorrect or invalid review comment.
| import type { Period, Range } from '#shared/types' | ||
| import { sub } from 'date-fns' | ||
|
|
||
| const { data: metrics } = useFetch('/api/network/metrics') |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for the API fetch.
The useFetch call lacks error handling, which could result in a poor user experience if the API fails.
Consider adding error handling:
-const { data: metrics } = useFetch('/api/network/metrics')
+const { data: metrics, error } = useFetch('/api/network/metrics')Then handle the error state in the template or show a fallback UI.
📝 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 { data: metrics } = useFetch('/api/network/metrics') | |
| const { data: metrics, error } = useFetch('/api/network/metrics') |
🤖 Prompt for AI Agents
In apps/web-app/app/pages/network/index.vue at line 25, the useFetch call to
'/api/network/metrics' lacks error handling. Modify the code to capture any
errors returned by useFetch, typically by destructuring an error property
alongside data. Then, update the template to check for this error state and
display an appropriate fallback UI or error message to improve user experience
when the API call fails.
|
1 participant
Summary by CodeRabbit