Fix error for non slot devices.#11
Open
SimonFair wants to merge 2 commits into
Open
Conversation
Add animation for zfs array disk.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization 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)
📝 WalkthroughWalkthroughThe PHP ChangesPHP ZFS backend: array device lookup, filtering, and tests
JS storage group peer animation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@assets/45d/45drives-disks/assets/index.bbceb330.js`:
- Around line 23297-23303: The forEach loop that animates storage group members
currently includes the current disk in the animation. Within the group.forEach
loop where dsk is iterated, add an additional condition to skip the current disk
(cd) so that only peer members are animated. Check if dsk.name (or dsk) matches
the current disk identifier and return early if it does, before calling
p5.animateZpools. This ensures peer-only highlighting by excluding the current
disk from the animation loop.
In `@php/zfs_info.php`:
- Around line 415-419: The verify_zfs_device_format() function currently returns
an empty alert array unconditionally, which suppresses warnings for all cases
including genuinely malformed or unexpectedly mapped devices. Modify the return
statement to conditionally return an empty array only for valid non-bay devices
(boot, flash, standalone NVMe pools), while ensuring that genuinely malformed or
unexpectedly mapped members still populate and return the alert array with
appropriate diagnostic warnings. This requires adding conditional logic before
the return statement to distinguish between non-slot devices that should be
silenced and actual format violations that should still be reported.
In `@tests/run.php`:
- Around line 556-567: The putenv call that sets DRIVEMAP_ZFS_FIXTURE_DIR at the
beginning of the test block mutates the global process environment and leaves it
set after the block completes, which can leak into subsequent tests. Capture the
original value of DRIVEMAP_ZFS_FIXTURE_DIR (if it exists) before the putenv
call, then restore it back to its original state after all the assertions in
this test block have completed, using another putenv call or by unsetting the
variable if it didn't exist before.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 872a8adf-684a-4aab-9b28-2d216e18abc5
📒 Files selected for processing (13)
assets/45d/45drives-disks/assets/index.bbceb330.jsphp/zfs_info.phptests/fixtures/zfs_array/zfs_list.txttests/fixtures/zfs_array/zpool_iostat_disk1.txttests/fixtures/zfs_array/zpool_iostat_path_disk1.txttests/fixtures/zfs_array/zpool_iostat_path_zmirror.txttests/fixtures/zfs_array/zpool_iostat_zmirror.txttests/fixtures/zfs_array/zpool_list.txttests/fixtures/zfs_array/zpool_status_disk1.txttests/fixtures/zfs_array/zpool_status_path_disk1.txttests/fixtures/zfs_array/zpool_status_path_zmirror.txttests/fixtures/zfs_array/zpool_status_zmirror.txttests/run.php
Comment on lines
+23297
to
+23303
| group.forEach((dsk) => { | ||
| const dsk_idx = diskLocations2.findIndex((loc) => loc.BAY === dsk.name); | ||
| if (dsk_idx < 0 || !diskLocations2[dsk_idx]?.image) | ||
| return; | ||
| const loc = diskLocations2[dsk_idx]; | ||
| p5.animateZpools(loc.x, loc.y, loc.image.width, loc.image.height + y_offset, p5.zfsAnimationSteps, p5.zfsAnimationIndex, p5.zfsAnimationColors.storage_group.start, p5.zfsAnimationColors.storage_group.end, p5.zfsAnimationDir); | ||
| }); |
There was a problem hiding this comment.
Exclude the current disk from peer-group animation.
showStorageGroupPeers currently animates all group members, including cd. For peer-only highlighting, skip the current disk in this loop.
Suggested fix
group.forEach((dsk) => {
+ if (dsk.name === cd)
+ return;
const dsk_idx = diskLocations2.findIndex((loc) => loc.BAY === dsk.name);
if (dsk_idx < 0 || !diskLocations2[dsk_idx]?.image)
return;
const loc = diskLocations2[dsk_idx];
p5.animateZpools(loc.x, loc.y, loc.image.width, loc.image.height + y_offset, p5.zfsAnimationSteps, p5.zfsAnimationIndex, p5.zfsAnimationColors.storage_group.start, p5.zfsAnimationColors.storage_group.end, p5.zfsAnimationDir);
});📝 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
| group.forEach((dsk) => { | |
| const dsk_idx = diskLocations2.findIndex((loc) => loc.BAY === dsk.name); | |
| if (dsk_idx < 0 || !diskLocations2[dsk_idx]?.image) | |
| return; | |
| const loc = diskLocations2[dsk_idx]; | |
| p5.animateZpools(loc.x, loc.y, loc.image.width, loc.image.height + y_offset, p5.zfsAnimationSteps, p5.zfsAnimationIndex, p5.zfsAnimationColors.storage_group.start, p5.zfsAnimationColors.storage_group.end, p5.zfsAnimationDir); | |
| }); | |
| group.forEach((dsk) => { | |
| if (dsk.name === cd) | |
| return; | |
| const dsk_idx = diskLocations2.findIndex((loc) => loc.BAY === dsk.name); | |
| if (dsk_idx < 0 || !diskLocations2[dsk_idx]?.image) | |
| return; | |
| const loc = diskLocations2[dsk_idx]; | |
| p5.animateZpools(loc.x, loc.y, loc.image.width, loc.image.height + y_offset, p5.zfsAnimationSteps, p5.zfsAnimationIndex, p5.zfsAnimationColors.storage_group.start, p5.zfsAnimationColors.storage_group.end, p5.zfsAnimationDir); | |
| }); |
🤖 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 `@assets/45d/45drives-disks/assets/index.bbceb330.js` around lines 23297 -
23303, The forEach loop that animates storage group members currently includes
the current disk in the animation. Within the group.forEach loop where dsk is
iterated, add an additional condition to skip the current disk (cd) so that only
peer members are animated. Check if dsk.name (or dsk) matches the current disk
identifier and return early if it does, before calling p5.animateZpools. This
ensures peer-only highlighting by excluding the current disk from the animation
loop.
Comment on lines
+415
to
419
| // Non-bay devices such as boot, flash, and standalone NVMe pools are valid | ||
| // ZFS members, but there is no slot to annotate in the drivemap UI. | ||
| // Keep them in the pool list and skip user-facing warnings. | ||
| $alert = []; | ||
| if (!isset($status_obj[$pool_name])) { | ||
| return $alert; | ||
| } | ||
|
|
||
| $default_pattern = '/^\t (\d+-\d+)(?:-part[0-9])?\s+(\S+)\s+(\S+)\s+(\S+)\s+(\S+).*/m'; | ||
| $unsupported_pattern = '/^\t (\S+)(?:-part[0-9])?\s+(\S+)\s+(\S+)\s+(\S+)\s+(\S+).*/m'; | ||
|
|
||
| preg_match_all($default_pattern, $status_obj[$pool_name], $default_matches, PREG_SET_ORDER); | ||
| preg_match_all($unsupported_pattern, $status_obj[$pool_name], $unsupported_matches, PREG_SET_ORDER); | ||
|
|
||
| $default_disks = []; | ||
| foreach ($default_matches as $match) { | ||
| $default_disks[] = $match[1]; | ||
| } | ||
|
|
||
| $unsupported_disks = []; | ||
| foreach ($unsupported_matches as $match) { | ||
| $unsupported_disks[] = $match[1]; | ||
| } | ||
|
|
||
| if (count($unsupported_disks) > count($default_disks)) { | ||
| $filtered = array_values(array_diff($unsupported_disks, $default_disks)); | ||
| $filtered = array_values(array_filter($filtered, function ($name) { | ||
| return !preg_match('/^(\d+-\d+)(?:-part[0-9])/', $name); | ||
| })); | ||
| $filtered = array_values(array_filter($filtered, function ($name) use ($disk_lookup) { | ||
| return !preg_match('/^\d+-\d+$/', canonical_zfs_disk_name($name, $disk_lookup)); | ||
| })); | ||
|
|
||
| if (!$filtered) { | ||
| return $alert; | ||
| } | ||
|
|
||
| $alert[] = "ZFS status displayed by this module for zpool '$pool_name' may be incomplete.\n\n"; | ||
| $alert[] = "This module can only display zfs status information for devices that are created using a device alias.\n\n"; | ||
| $alert[] = "This can be done using the 45Drives cockpit-zfs-manager package:\nhttps://github.com/45Drives/cockpit-zfs-manager/releases/\n\n"; | ||
| if ($filtered) { | ||
| $alert[] = "The following zfs devices do not conform:\n"; | ||
| foreach ($filtered as $disk) { | ||
| $alert[] = "\t $disk\n"; | ||
| } | ||
| } | ||
| $alert[] = "\n"; | ||
| } | ||
|
|
||
| return $alert; |
There was a problem hiding this comment.
verify_zfs_device_format() now suppresses all warnings, not only non-slot ones.
Line 415-419 returns an empty array unconditionally, so genuinely malformed or unexpectedly mapped members also lose diagnostics. That widens behavior beyond the stated non-slot suppression and weakens the API’s warning signal.
🤖 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 `@php/zfs_info.php` around lines 415 - 419, The verify_zfs_device_format()
function currently returns an empty alert array unconditionally, which
suppresses warnings for all cases including genuinely malformed or unexpectedly
mapped devices. Modify the return statement to conditionally return an empty
array only for valid non-bay devices (boot, flash, standalone NVMe pools), while
ensuring that genuinely malformed or unexpectedly mapped members still populate
and return the alert array with appropriate diagnostic warnings. This requires
adding conditional logic before the return statement to distinguish between
non-slot devices that should be silenced and actual format violations that
should still be reported.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
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.
Add animation for zfs array disk.
Summary by CodeRabbit
New Features
Bug Fixes
Tests