feat(dashboard): add second drive stat; add network speed card#15
Merged
Merged
Conversation
replace the combined network speed summary with dedicated upload and download values in the home system stats cards. improve stat rendering for responsive layouts by hiding network on mobile, stacking network directions on desktop, and refining text-like value styling with directional icons. also improve secondary disk selection by avoiding duplicate matches to the primary disk and using the disk key in the secondary disk description.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe changes extend the system stats dashboard to display secondary disk usage and network speed metrics (upload/download). The SystemStatValue component gains support for upload and download types with directional icons. The SystemStats component conditionally renders multiple values, and the data store adds new fields for these metrics alongside supporting logic in the provider. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 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. ✨ 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 |
Deploying godoxy-webui with
|
| Latest commit: |
0566188
|
| Status: | 🚫 Build failed. |
![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
🧹 Nitpick comments (3)
src/components/home/SystemStats.tsx (2)
68-72: Type safety:secondaryKeycould be undefined.
stat.secondaryKeyis typed as optional (FieldPath<...> | undefined), but it's passed directly tovalueKeywhich expects a defined value. While the currentstatsPropsdata ensures these keys exist for the relevant stats, TypeScript cannot verify this through thestat.keychecks.Consider adding a runtime guard or using a non-null assertion if you're confident the data is correct.
🛡️ Proposed defensive check
- ) : stat.key === 'networkSpeedUpload' ? ( + ) : stat.key === 'networkSpeedUpload' && stat.secondaryKey ? ( <div className="flex flex-col gap-0.5"> <SystemStatValue valueKey={stat.key} type="upload" /> <SystemStatValue valueKey={stat.secondaryKey} type="download" /> </div>Or for the disk section (lines 58-74), wrap with
stat.secondaryKey &&:- {stat.key === 'rootPartitionUsage' && hasSecondaryDisk ? ( + {stat.key === 'rootPartitionUsage' && hasSecondaryDisk && stat.secondaryKey ? (Also applies to: 77-78
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/home/SystemStats.tsx` around lines 68 - 72, The SystemStatValue is receiving stat.secondaryKey/secondaryDescriptionKey which are typed optional; add a runtime guard or assert non-null so a possibly undefined value isn't passed: for the disk-related rendering blocks around the SystemStatValue usage, either wrap the component render with a conditional check (e.g., stat.secondaryKey && stat.secondaryDescriptionKey && <SystemStatValue ...>) or, if you are certain the data always has these fields for those stats, use a non-null assertion when passing them (valueKey={stat.secondaryKey!} descriptionKey={stat.secondaryDescriptionKey!}); ensure you update both occurrences where SystemStatValue is used (the disk section at the shown block and the other occurrence around lines 77-78).
119-121: Null displayValue renders empty span fornetworkSpeedmode.When
mobileValueModeis'networkSpeed',displayValueisnull, resulting in an empty span. This works becausehideOnMobile: trueis set for the Network stat, but the coupling is implicit. Consider documenting this dependency or handling the null case explicitly.Also applies to: 131-131
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/home/SystemStats.tsx` around lines 119 - 121, The component (SystemStats / the logic around stat.mobileValueMode and displayValue) relies implicitly on networkSpeed producing a null displayValue and hideOnMobile being true for the Network stat, which causes an empty <span>; make this explicit by handling the null case: when stat.mobileValueMode === 'networkSpeed' or displayValue is null/undefined, return a clear fallback (e.g., render nothing, a descriptive placeholder, or conditionally skip rendering the <span>) or add a comment documenting the dependency and ensure the Network stat object (where hideOnMobile is set) is validated so the UI won't render an empty span; update both occurrences (the check at stat.mobileValueMode === 'networkSpeed' and the similar logic around line 131) to follow this explicit handling.src/components/home/SystemStatValue.tsx (1)
50-67: Dead code:type = 'text'assignments have no effect.The assignments
type = 'text'on lines 51 and 60 are ineffective because:
- The callback returns JSX immediately after each assignment
- The
isTextLikecheck on line 70 already explicitly handles'upload'and'download'typesThese assignments can be safely removed.
🧹 Proposed cleanup
if (type === 'upload') { - type = 'text' return ( <> <IconArrowUp className="size-4 text-green-500" />{' '} {formatBytes(Number(value), { precision: 0, unit: '/s' })} </> ) } if (type === 'download') { - type = 'text' return ( <> <IconArrowDown className="size-4 text-red-500" />{' '} {formatBytes(Number(value), { precision: 0, unit: '/s' })} </> ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/home/SystemStatValue.tsx` around lines 50 - 67, In SystemStatValue.tsx inside the render logic for the SystemStatValue component, remove the ineffective assignments `type = 'text'` that appear before the early returns in the 'upload' and 'download' branches (the blocks that return JSX with IconArrowUp/IconArrowDown and formatBytes). The fix: delete the `type = 'text'` lines since the function returns immediately and `isTextLike` already handles those types; leave the return JSX (IconArrowUp, IconArrowDown, formatBytes, and value) unchanged so behavior is preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/home/SystemStatsProvider.tsx`:
- Around line 60-72: The isSameDisk function currently treats disks as identical
if path matches or if fstype, total and used match; remove the volatile
disk.used comparison and compare only stable identifiers: keep the path equality
check first, then fall back to comparing stable properties such as fstype and
total (and any stable id/label/uuid on DiskUsageStat if available) — update the
function (isSameDisk and any call sites if necessary) to omit disk.used from the
equality branch so transient usage values don't cause false positives.
- Around line 16-19: The multiplication by 100 is misplaced due to operator
precedence: update the rootPartitionUsage and secondaryPartitionUsage
assignments to apply Math.round to the disk usage value (or remove the * 100
entirely if getDiskUsage / getSecondaryDiskUsage already return 0–100).
Specifically, change the expressions that use getDiskUsage(data.disks, '/') and
getSecondaryDiskUsage(data.disks, '/') so that either you remove the trailing "*
100" and use Math.round(getDiskUsage(...) ?? 0) and
Math.round(getSecondaryDiskUsage(...) ?? 0), or wrap the call in parentheses
before multiplying (Math.round((getDiskUsage(...) ?? 0) * 100)) if the functions
return 0–1; adjust both rootPartitionUsage and secondaryPartitionUsage
accordingly.
---
Nitpick comments:
In `@src/components/home/SystemStats.tsx`:
- Around line 68-72: The SystemStatValue is receiving
stat.secondaryKey/secondaryDescriptionKey which are typed optional; add a
runtime guard or assert non-null so a possibly undefined value isn't passed: for
the disk-related rendering blocks around the SystemStatValue usage, either wrap
the component render with a conditional check (e.g., stat.secondaryKey &&
stat.secondaryDescriptionKey && <SystemStatValue ...>) or, if you are certain
the data always has these fields for those stats, use a non-null assertion when
passing them (valueKey={stat.secondaryKey!}
descriptionKey={stat.secondaryDescriptionKey!}); ensure you update both
occurrences where SystemStatValue is used (the disk section at the shown block
and the other occurrence around lines 77-78).
- Around line 119-121: The component (SystemStats / the logic around
stat.mobileValueMode and displayValue) relies implicitly on networkSpeed
producing a null displayValue and hideOnMobile being true for the Network stat,
which causes an empty <span>; make this explicit by handling the null case: when
stat.mobileValueMode === 'networkSpeed' or displayValue is null/undefined,
return a clear fallback (e.g., render nothing, a descriptive placeholder, or
conditionally skip rendering the <span>) or add a comment documenting the
dependency and ensure the Network stat object (where hideOnMobile is set) is
validated so the UI won't render an empty span; update both occurrences (the
check at stat.mobileValueMode === 'networkSpeed' and the similar logic around
line 131) to follow this explicit handling.
In `@src/components/home/SystemStatValue.tsx`:
- Around line 50-67: In SystemStatValue.tsx inside the render logic for the
SystemStatValue component, remove the ineffective assignments `type = 'text'`
that appear before the early returns in the 'upload' and 'download' branches
(the blocks that return JSX with IconArrowUp/IconArrowDown and formatBytes). The
fix: delete the `type = 'text'` lines since the function returns immediately and
`isTextLike` already handles those types; leave the return JSX (IconArrowUp,
IconArrowDown, formatBytes, and value) unchanged so behavior is preserved.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/components/home/SystemStatValue.tsxsrc/components/home/SystemStats.tsxsrc/components/home/SystemStatsProvider.tsxsrc/components/home/store.ts
Comment on lines
16
to
+19
| rootPartitionUsage: Math.round(getDiskUsage(data.disks, '/') ?? 0 * 100), | ||
| rootPartitionUsageDesc: getDiskUsageDesc(data.disks, '/'), | ||
| secondaryPartitionUsage: Math.round(getSecondaryDiskUsage(data.disks, '/') ?? 0 * 100), | ||
| secondaryPartitionUsageDesc: getSecondaryDiskUsageDesc(data.disks, '/'), |
There was a problem hiding this comment.
Operator precedence bug: * 100 is never applied.
Due to operator precedence, getDiskUsage(data.disks, '/') ?? 0 * 100 evaluates as getDiskUsage(...) ?? (0 * 100) which equals getDiskUsage(...) ?? 0. The multiplication is only applied to the fallback 0, not to the disk usage value.
If used_percent from DiskUsageStat is already 0-100, remove * 100. If it's 0-1, add parentheses.
🐛 Proposed fix (assuming used_percent is already 0-100)
- rootPartitionUsage: Math.round(getDiskUsage(data.disks, '/') ?? 0 * 100),
+ rootPartitionUsage: Math.round(getDiskUsage(data.disks, '/') ?? 0),
rootPartitionUsageDesc: getDiskUsageDesc(data.disks, '/'),
- secondaryPartitionUsage: Math.round(getSecondaryDiskUsage(data.disks, '/') ?? 0 * 100),
+ secondaryPartitionUsage: Math.round(getSecondaryDiskUsage(data.disks, '/') ?? 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.
Suggested change
| rootPartitionUsage: Math.round(getDiskUsage(data.disks, '/') ?? 0 * 100), | |
| rootPartitionUsageDesc: getDiskUsageDesc(data.disks, '/'), | |
| secondaryPartitionUsage: Math.round(getSecondaryDiskUsage(data.disks, '/') ?? 0 * 100), | |
| secondaryPartitionUsageDesc: getSecondaryDiskUsageDesc(data.disks, '/'), | |
| rootPartitionUsage: Math.round(getDiskUsage(data.disks, '/') ?? 0), | |
| rootPartitionUsageDesc: getDiskUsageDesc(data.disks, '/'), | |
| secondaryPartitionUsage: Math.round(getSecondaryDiskUsage(data.disks, '/') ?? 0), | |
| secondaryPartitionUsageDesc: getSecondaryDiskUsageDesc(data.disks, '/'), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/home/SystemStatsProvider.tsx` around lines 16 - 19, The
multiplication by 100 is misplaced due to operator precedence: update the
rootPartitionUsage and secondaryPartitionUsage assignments to apply Math.round
to the disk usage value (or remove the * 100 entirely if getDiskUsage /
getSecondaryDiskUsage already return 0–100). Specifically, change the
expressions that use getDiskUsage(data.disks, '/') and
getSecondaryDiskUsage(data.disks, '/') so that either you remove the trailing "*
100" and use Math.round(getDiskUsage(...) ?? 0) and
Math.round(getSecondaryDiskUsage(...) ?? 0), or wrap the call in parentheses
before multiplying (Math.round((getDiskUsage(...) ?? 0) * 100)) if the functions
return 0–1; adjust both rootPartitionUsage and secondaryPartitionUsage
accordingly.
Comment on lines
+60
to
+72
| function isSameDisk(disk: DiskUsageStat, otherDisk: DiskUsageStat) { | ||
| if (disk.path === otherDisk.path) { | ||
| return true | ||
| } | ||
| if ( | ||
| disk.fstype === otherDisk.fstype && | ||
| disk.total === otherDisk.total && | ||
| disk.used === otherDisk.used | ||
| ) { | ||
| return true | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
Fragile comparison: disk.used changes frequently.
Comparing disk.used is unreliable because disk usage is dynamic. Two different disks could momentarily have identical used values, causing false positives. Consider comparing only stable properties.
🛡️ Proposed fix using stable properties only
function isSameDisk(disk: DiskUsageStat, otherDisk: DiskUsageStat) {
if (disk.path === otherDisk.path) {
return true
}
- if (
- disk.fstype === otherDisk.fstype &&
- disk.total === otherDisk.total &&
- disk.used === otherDisk.used
- ) {
+ // Same filesystem type and total size likely indicates same physical disk
+ // (e.g., different mount points on the same partition)
+ if (disk.fstype === otherDisk.fstype && disk.total === otherDisk.total) {
return true
}
return false
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/home/SystemStatsProvider.tsx` around lines 60 - 72, The
isSameDisk function currently treats disks as identical if path matches or if
fstype, total and used match; remove the volatile disk.used comparison and
compare only stable identifiers: keep the path equality check first, then fall
back to comparing stable properties such as fstype and total (and any stable
id/label/uuid on DiskUsageStat if available) — update the function (isSameDisk
and any call sites if necessary) to omit disk.used from the equality branch so
transient usage values don't cause false positives.
yusing
deleted the
session/agent_d3afa5f0-1f17-41c8-bc77-79541d305293
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit