-
Notifications
You must be signed in to change notification settings - Fork 6
feat: add total used traffic above the chart #3
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
base: main
Are you sure you want to change the base?
Conversation
WalkthroughThe traffic-chart component now displays total period traffic in the CardHeader with computed memoized calculation and human-readable byte formatting. RTL layout support is added based on language setting, and the title row is restructured to show the title on the left half and formatted total traffic on the right half when data exists. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/traffic-chart.tsx(3 hunks)
🔇 Additional comments (1)
src/components/traffic-chart.tsx (1)
137-141: LGTM!The memoized calculation correctly sums the total traffic across all data points.
| // Format bytes to human-readable | ||
| const formatBytes = (bytes: number) => { | ||
| if (bytes === 0) return '0 B' | ||
| const k = 1024 | ||
| const sizes = ['B', 'KB', 'MB', 'GB', 'TB'] | ||
| const i = Math.floor(Math.log(bytes) / Math.log(k)) | ||
| return `${(bytes / Math.pow(k, i)).toFixed(2)} ${sizes[i]}` | ||
| } |
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.
🛠️ Refactor suggestion | 🟠 Major
Extract the duplicate formatBytes function.
The same formatBytes function is defined twice: here (lines 143-150) and in CustomTrafficTooltip (lines 58-64). Extract it to module scope or a shared utility to eliminate duplication.
Apply this diff to extract the function to module scope:
+// Format bytes to human-readable
+const formatBytes = (bytes: number) => {
+ if (bytes === 0) return '0 B'
+ const k = 1024
+ const sizes = ['B', 'KB', 'MB', 'GB', 'TB']
+ const i = Math.floor(Math.log(bytes) / Math.log(k))
+ return `${(bytes / Math.pow(k, i)).toFixed(2)} ${sizes[i]}`
+}
+
function CustomTrafficTooltip({ active, payload }: TooltipProps<number, string>) {
const { t, i18n } = useTranslation()
if (!active || !payload || !payload.length) return null
const data = payload[0].payload as FormattedDataPoint
- // Format bytes to human-readable
- const formatBytes = (bytes: number) => {
- if (bytes === 0) return '0 B'
- const k = 1024
- const sizes = ['B', 'KB', 'MB', 'GB', 'TB']
- const i = Math.floor(Math.log(bytes) / Math.log(k))
- return `${(bytes / Math.pow(k, i)).toFixed(2)} ${sizes[i]}`
- }
-
// Format date using dateUtilsAnd remove the duplicate inside TrafficChart:
const totalPeriodTraffic = React.useMemo(() => {
if (!data || data.length === 0) return 0
return data.reduce((sum, point) => sum + point.total_traffic, 0)
}, [data])
- // Format bytes to human-readable
- const formatBytes = (bytes: number) => {
- if (bytes === 0) return '0 B'
- const k = 1024
- const sizes = ['B', 'KB', 'MB', 'GB', 'TB']
- const i = Math.floor(Math.log(bytes) / Math.log(k))
- return `${(bytes / Math.pow(k, i)).toFixed(2)} ${sizes[i]}`
- }
-
const timeRangeOptions = [Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/components/traffic-chart.tsx around lines 143-150 (and duplicate in
CustomTrafficTooltip at lines 58-64), the formatBytes function is duplicated;
extract a single formatBytes implementation to module scope or a shared utility
(e.g., export a named formatBytes from the top of this file or from
src/utils/format.ts), remove the duplicate inside TrafficChart and remove the
duplicate in CustomTrafficTooltip, and update both callers to import/reference
the shared function with correct TypeScript typing (bytes: number) so only one
implementation remains and both components use it.
| <CardHeader className="flex flex-1 flex-col gap-4 space-y-0 border-b pb-4"> | ||
| <div className={`flex w-full items-center flex-row justify-between gap-2 `}> | ||
| <CardTitle className="text-base sm:text-lg w-1/2"> | ||
| {t('usage.title')} | ||
| </CardTitle> | ||
| <div className="w-1/2"> | ||
| {hasChartPoints && ( | ||
| <div className="flex flex-1"> | ||
| <span className={`text-lg flex-1 ${isRTL ? "text-left": "text-right"} sm:text-xl font-bold text-foreground f`}> | ||
| {formatBytes(totalPeriodTraffic)} | ||
| </span> | ||
| </div> | ||
| )} | ||
| </div> | ||
| </div> |
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.
Fix the typo and improve RTL layout handling.
Several issues in the header layout:
-
Line 172 has a typo: The className string ends with
"f\"` which creates an invalid CSS class. -
RTL layout incomplete: The container lacks a
dirattribute, so in RTL languages the visual order won't flip—title and traffic will remain in the same positions. Consider addingdir={isRTL ? 'rtl' : 'ltr'}to the container on line 165. -
Overly nested structure: Lines 169-172 have redundant nesting with
flex-1used inside a fixedw-1/2container.
Apply this diff to fix the typo and improve the layout:
- <CardHeader className="flex flex-1 flex-col gap-4 space-y-0 border-b pb-4">
- <div className={`flex w-full items-center flex-row justify-between gap-2 `}>
+ <CardHeader className="flex flex-col gap-4 space-y-0 border-b pb-4">
+ <div className="flex w-full items-center justify-between gap-2" dir={isRTL ? 'rtl' : 'ltr'}>
<CardTitle className="text-base sm:text-lg w-1/2">
{t('usage.title')}
</CardTitle>
- <div className="w-1/2">
- {hasChartPoints && (
- <div className="flex flex-1">
- <span className={`text-lg flex-1 ${isRTL ? "text-left": "text-right"} sm:text-xl font-bold text-foreground f`}>
- {formatBytes(totalPeriodTraffic)}
- </span>
- </div>
- )}
- </div>
+ {hasChartPoints && (
+ <div className="w-1/2 text-right sm:text-xl">
+ <span className="text-lg sm:text-xl font-bold text-foreground">
+ {formatBytes(totalPeriodTraffic)}
+ </span>
+ </div>
+ )}
</div>Note: Removed redundant flex-1 from CardHeader (line 164), removed duplicate flex and explicit flex-row (line 165), simplified the traffic display structure, and added dir attribute for proper RTL support.
📝 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.
| <CardHeader className="flex flex-1 flex-col gap-4 space-y-0 border-b pb-4"> | |
| <div className={`flex w-full items-center flex-row justify-between gap-2 `}> | |
| <CardTitle className="text-base sm:text-lg w-1/2"> | |
| {t('usage.title')} | |
| </CardTitle> | |
| <div className="w-1/2"> | |
| {hasChartPoints && ( | |
| <div className="flex flex-1"> | |
| <span className={`text-lg flex-1 ${isRTL ? "text-left": "text-right"} sm:text-xl font-bold text-foreground f`}> | |
| {formatBytes(totalPeriodTraffic)} | |
| </span> | |
| </div> | |
| )} | |
| </div> | |
| </div> | |
| <CardHeader className="flex flex-col gap-4 space-y-0 border-b pb-4"> | |
| <div className="flex w-full items-center justify-between gap-2" dir={isRTL ? 'rtl' : 'ltr'}> | |
| <CardTitle className="text-base sm:text-lg w-1/2"> | |
| {t('usage.title')} | |
| </CardTitle> | |
| {hasChartPoints && ( | |
| <div className="w-1/2 text-right sm:text-xl"> | |
| <span className="text-lg sm:text-xl font-bold text-foreground"> | |
| {formatBytes(totalPeriodTraffic)} | |
| </span> | |
| </div> | |
| )} | |
| </div> |
🤖 Prompt for AI Agents
In src/components/traffic-chart.tsx around lines 164 to 178, fix the header
layout: remove the redundant "flex-1" from the CardHeader className, add
dir={isRTL ? 'rtl' : 'ltr'} to the top-level container div to enable RTL
flipping, remove the extra nested flex wrappers and duplicate flex/flex-row
classes so the container is a single flex row with justify-between, and delete
the stray trailing "f" character in the span className; then simplify the
traffic display to a single span (no extra wrapper) with the appropriate text
alignment computed from isRTL (e.g., text-left when RTL, text-right when LTR)
and ensure widths (w-1/2) are applied only where needed.
Summary by CodeRabbit
Release Notes
New Features
Style