fix(storage): for differences to main pool sizes#2657
Open
SimonFair wants to merge 4 commits into
Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughNormalize effective Btrfs filesystem size, compute pool-aware cache usage and a localized pool usage string in the backend, append that string to per-pool JSON, and render/update a dedicated pool usage span in the DashStats frontend. ChangesPool Usage Dashboard Feature
Sequence DiagramsequenceDiagram
participant DeviceList as device_list (effective_fs_size)
participant Backend as update_2 (backend)
participant Payload as JSON payload
participant Frontend as DashStats (frontend)
participant WS as WebSocket client
DeviceList->>Backend: provide effective_fs_size() and normalized fsType
Backend->>Backend: device_usage() computes pool-aware usage (Btrfs-adjusted)
Backend->>Backend: pool_usage() formats localized "Used X of Y (Z%)"
Backend->>Payload: append pool usage string into pool array (index 6)
Payload->>Frontend: initial render includes pool array with usage string
WS->>Frontend: realtime pool update with pool array (may include index 6)
Frontend->>Frontend: set `#pool_usage{n}` to payload.pool[n][6] when present
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
🔧 PR Test Plugin AvailableA test plugin has been generated for this PR that includes the modified files. Version: 📥 Installation Instructions:Install via Unraid Web UI:
Alternative: Direct Download
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@emhttp/plugins/dynamix/DashStats.page`:
- Around line 1194-1195: The code incorrectly checks $pool == "flash" when
deciding to use boot vs cache arrays, causing wrong text and undefined-index
notices; change the branch to detect boot pools (e.g., in_array($pool,
$bootpools) or use the disk type variable used elsewhere) instead of the literal
"flash" so the block uses $boot_used/$boot_size/$boot_rate for actual boot pools
and $cache_used/$cache_size/$cache_rate for others; update the conditional
around the echo that references $pool to use that boot-pool check (and ensure
the variable name you use matches the existing symbol holding boot pool
membership).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a2c46460-6230-4cae-a162-44622d176045
📒 Files selected for processing (2)
emhttp/plugins/dynamix/DashStats.pageemhttp/plugins/dynamix/nchan/update_2
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
emhttp/plugins/dynamix/DashStats.page (1)
1197-1198:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse pool type here, not the literal
flashname.Line 1176 merges both
$poolsand$bootpools, so this loop processes all pool types. Any boot pool with a name other than "flash" (e.g., "efi", "boot", "usb") will fall into the else branch and attempt to access$cache_used[$pool],$cache_size[$pool], and$cache_rate[$pool]. However, boot pool data is stored in the$boot_*arrays (populated at lines 94-103), not the$cache_*arrays. This causes undefined-index notices and displays incorrect usage information until the WebSocket refresh corrects it. Check the disk type instead of hardcoding the pool name.🔧 Proposed fix
+ <? if ($pool == "flash") echo sprintf(_("Used: %s of %s (%s %%)"),my_scale($boot_used[$pool]*1024,$unit)." $unit",my_scale($boot_size[$pool]*1024,$unit,-1,-1)." $unit",$boot_rate[$pool]); - else echo sprintf(_("Used: %s of %s (%s %%)"),my_scale($cache_used[$pool]*1024,$unit)." $unit",my_scale($cache_size[$pool]*1024,$unit,-1,-1)." $unit",$cache_rate[$pool]) + if ($poolType === 'boot') { + echo sprintf(_("Used: %s of %s (%s %%)"),my_scale($boot_used[$pool]*1024,$unit)." $unit",my_scale($boot_size[$pool]*1024,$unit,-1,-1)." $unit",$boot_rate[$pool]); + } else { + echo sprintf(_("Used: %s of %s (%s %%)"),my_scale($cache_used[$pool]*1024,$unit)." $unit",my_scale($cache_size[$pool]*1024,$unit,-1,-1)." $unit",$cache_rate[$pool]); + } + ?>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@emhttp/plugins/dynamix/DashStats.page` around lines 1197 - 1198, The code incorrectly checks if ($pool == "flash") to decide whether to use $boot_* vs $cache_* arrays; instead detect whether this pool has boot data (e.g., use isset($boot_used[$pool]) or another pool-type flag) and branch accordingly so boot pools use $boot_used/$boot_size/$boot_rate and non-boot pools use $cache_used/$cache_size/$cache_rate; update the echo logic around $pool to reference the appropriate arrays based on that check rather than the literal "flash".
🧹 Nitpick comments (1)
emhttp/plugins/dynamix/nchan/device_list (1)
668-674: 💤 Low valueConsider adding inline documentation for Btrfs special handling.
The helper function correctly implements Btrfs size normalization, but would benefit from a brief comment explaining why Btrfs requires
used + freeinstead of the reportedfsSize(Btrfs reports raw device size, but actual capacity depends on RAID level and metadata overhead).📝 Suggested documentation
+/** + * Get effective filesystem size, with Btrfs normalization. + * For Btrfs, returns used+free instead of reported fsSize because + * Btrfs reports raw device size, but actual capacity depends on + * RAID level and metadata overhead. + */ function effective_fs_size(&$disk) { $size = _var($disk,'fsSize',0); $used = _var($disk,'fsUsed',null); $free = _var($disk,'fsFree',null); $fsType = str_replace('luks:','',_var($disk,'fsType','')); return ($fsType=='btrfs' && is_numeric($used) && is_numeric($free)) ? $used+$free : $size; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@emhttp/plugins/dynamix/nchan/device_list` around lines 668 - 674, Add a short inline comment above or inside the effective_fs_size function explaining the Btrfs special-case: when $fsType == 'btrfs' use $used + $free because Btrfs reports raw device size (including RAID/multi-device and metadata overhead) in fsSize, so actual usable capacity must be computed from reported used + free; reference the function name effective_fs_size and the $disk keys 'fsSize','fsUsed','fsFree','fsType' in the comment for clarity.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@emhttp/plugins/dynamix/nchan/device_list`:
- Around line 248-250: The three functions get_pool_boot_metrics,
get_boot_pool_metrics, and get_pool_data_metrics duplicate Btrfs size
normalization logic; replace the inline logic that computes $size when
$fsType=='btrfs' (the code that strips 'luks:' and sums $used+$free when
numeric) with a call to the existing helper effective_fs_size() so those
functions pass the same inputs to effective_fs_size() and use its returned size,
removing the duplicated conditional and str_replace logic and ensuring
consistent behavior across all three functions.
---
Duplicate comments:
In `@emhttp/plugins/dynamix/DashStats.page`:
- Around line 1197-1198: The code incorrectly checks if ($pool == "flash") to
decide whether to use $boot_* vs $cache_* arrays; instead detect whether this
pool has boot data (e.g., use isset($boot_used[$pool]) or another pool-type
flag) and branch accordingly so boot pools use $boot_used/$boot_size/$boot_rate
and non-boot pools use $cache_used/$cache_size/$cache_rate; update the echo
logic around $pool to reference the appropriate arrays based on that check
rather than the literal "flash".
---
Nitpick comments:
In `@emhttp/plugins/dynamix/nchan/device_list`:
- Around line 668-674: Add a short inline comment above or inside the
effective_fs_size function explaining the Btrfs special-case: when $fsType ==
'btrfs' use $used + $free because Btrfs reports raw device size (including
RAID/multi-device and metadata overhead) in fsSize, so actual usable capacity
must be computed from reported used + free; reference the function name
effective_fs_size and the $disk keys 'fsSize','fsUsed','fsFree','fsType' in the
comment for clarity.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1be4f721-30bb-4146-9d39-d877c52d8a97
📒 Files selected for processing (3)
emhttp/plugins/dynamix/DashStats.pageemhttp/plugins/dynamix/nchan/device_listemhttp/plugins/dynamix/nchan/update_2
🚧 Files skipped from review as they are similar to previous changes (1)
- emhttp/plugins/dynamix/nchan/update_2
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
emhttp/plugins/dynamix/nchan/device_list (1)
271-282:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical: condition checks overwritten
$sizevariable.Identical issue to
get_pool_boot_metricsabove. Line 281 overwrites$sizewitheffective_fs_size()result (always numeric), then line 282 checks$size !== null(alwaystrue).🐛 Proposed fix
foreach ($bootDisks as $disk) { if (prefix(_var($disk,'name','')) !== $bootPoolName) continue; - $size = _var($disk,'fsSize',null); + $origSize = _var($disk,'fsSize',null); $used = _var($disk,'fsUsed',null); $free = _var($disk,'fsFree',null); - $fsDisk = [ - 'fsSize' => $size ?? 0, - 'fsUsed' => $used ?? 0, - 'fsFree' => $free ?? 0, - 'fsType' => _var($disk,'fsType',''), - ]; - $size = effective_fs_size($fsDisk); + $size = effective_fs_size($disk); - if ($size !== null || $used !== null || $free !== null) { + if ($origSize !== null || $used !== null || $free !== null) { return [ 'fsSize' => $size ?? 0, 'fsUsed' => $used ?? 0, 'fsFree' => $free ?? 0, ]; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@emhttp/plugins/dynamix/nchan/device_list` around lines 271 - 282, The condition check is broken because you overwrite $size with effective_fs_size($fsDisk) before testing for null; instead, keep the original raw $size/$used/$free and store the computed effective size in a new variable (e.g., $effectiveSize) so the subsequent if uses the original null checks (or checks against $effectiveSize explicitly if intended). Update the block around effective_fs_size(...) to assign to a new symbol rather than reusing $size and adjust the if(...) to reference the correct variables (retain references to effective_fs_size, $fsDisk, $size, $used, $free).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@emhttp/plugins/dynamix/nchan/device_list`:
- Around line 248-256: The condition that decides whether to return an empty
array is being evaluated against the value overwritten by effective_fs_size(),
because $size is reassigned; capture the original filesystem size before calling
effective_fs_size (e.g., store $size into $origSize or $rawSize), then call
effective_fs_size($fsDisk) into $sizeOnly (or reuse $size) and use the preserved
original ($origSize/$rawSize) in the conditional (the check that currently reads
$size !== null || $used !== null || $free !== null) so the condition reflects
the original input rather than the computed effective size.
---
Outside diff comments:
In `@emhttp/plugins/dynamix/nchan/device_list`:
- Around line 271-282: The condition check is broken because you overwrite $size
with effective_fs_size($fsDisk) before testing for null; instead, keep the
original raw $size/$used/$free and store the computed effective size in a new
variable (e.g., $effectiveSize) so the subsequent if uses the original null
checks (or checks against $effectiveSize explicitly if intended). Update the
block around effective_fs_size(...) to assign to a new symbol rather than
reusing $size and adjust the if(...) to reference the correct variables (retain
references to effective_fs_size, $fsDisk, $size, $used, $free).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a4f391eb-edae-4f6b-97f4-055f5b1dde7b
📒 Files selected for processing (1)
emhttp/plugins/dynamix/nchan/device_list
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
Bug Fixes
New Features