NAS-138173 / 26.04 / fix: show scrub progress correctly on dashboard

[mattwyatt-ix]

Changes:

• the in-progress check was checking if endTime AND isScanInProgress were true simultaneously, which is impossible, so the in-progress elements were never shown. fixed this by adding the correct check.

• also, the percentage was not displayed correctly, since the API reports the percentage by itself (a number like 58.25 as opposed to 0.5825) and Angular's percent formatting function expects a number between 0 and 1. fixed this by dividing the API value by 100

• prevent regressions in test file and refactor it too so we can better handle testing differences between having multiple pools in the component vs. having a single pool in the component.

Screenshots:

the whole component showing the in-progress icon

the tooltip shown when hovering over the in-progress icon

Testing:

1. start a scrub from the Storage page by clicking the Scrub Now button under the Storage Health card.
2. navigate to the dashboard (quickly, if your VM is small and the scrub will finish fast!) and observe that the scrub shows the arrow icon and correctly reports the progress.

[claude[bot]]

Claude finished @mattwyatt-ix's task —— View job

---

Code Review Complete ✅

Great work fixing the scrub progress display issues! 🎯 The core fixes are solid and the live update implementation is a nice enhancement.

Strengths

• Correct logic fix: The in-progress check now properly uses !endTime && isScanInProgress instead of the impossible endTime && isScanInProgress condition
• Percentage calculation fix: Dividing by 100 to convert API values (like 58.25) to the format Angular's PercentPipe expects (0-1 range)
• Live updates: The pool.scan subscription enables real-time scrub progress updates without page refresh
• Performance improvement: Changing the tracking expression from track poolInfo to track poolInfo.name prevents unnecessary re-renders
• Test coverage: Good test improvements with separate describe blocks for single/multiple pool configurations and live update testing

Issues Found

1. API Subscription Endpoint Inconsistency ⚠️

In widget-resources.service.ts:89, you're subscribing to pool.scan:

readonly scans$ = this.api.subscribe('pool.scan').pipe(

But in resilver-progress.component.ts:53 and resilvering-indicator.component.ts:36, the same subscription is still used. However, based on commit 7041901, it appears there was an API change (NAS-138409) that may have affected this endpoint.

Concern: The latest commit message says "this API event was changed: NAS-138409" but I can see the endpoint is now pool.scan across all files. If the API was recently changed from something like zfs.pool.scan to pool.scan, this looks correct. However, the intermediate commit a65aadf shows the endpoint as zfs.pool.scan in the comment, which creates confusion about the correct endpoint name.

2. Test Data Inconsistency ⚠️

In widget-storage.component.spec.ts:317-318, the test for live updates has an inconsistency:

scan: {
  ...poolWithOngoingScrub.scan,
  percentage: 71.234,  // ← Should be a value like 71.234, not 0.71234
},

The test correctly expects '71.23%' as output, which confirms the API provides percentage values (not fractions). The current test value of 71.234 is correct, but in the earlier commit a65aadf, this was 0.71234, which would have been inconsistent with how the API actually works.

3. Code Comments Need Cleanup

In widget-resources.service.ts:87-88, the comment references the wrong endpoint:

// since pool.query doesn't emit events for scan updates, we need to subscribe to
// the `zfs.pool.scan` endpoint to actually receive real-time scrub/resilver updates.
readonly scans$ = this.api.subscribe('pool.scan').pipe(

The comment says zfs.pool.scan but the code uses pool.scan. These should match.

Recommendations

1. Update the comment in widget-resources.service.ts:87-88 to reference pool.scan instead of zfs.pool.scan
2. Verify the test in widget-storage.component.spec.ts - ensure the percentage value 71.234 is correct for the API (it appears to be, based on other test data like 65.19)
3. Consider adding a test case for when a scrub completes (transitions from in-progress to finished) to verify the checkmark icon appears correctly - this was the issue reported by aervin in the review

Verification for aervin's concern

The reviewer mentioned the checkmark doesn't appear after scrub completion. Looking at the logic in widget-storage.component.ts:290-305:

if (!endTime && isScanInProgress) {
  // case: scan is in progress.
  icon = statusIcons.arrowCircleRight;
  level = StatusLevel.Safe;
  value = this.formatScanPercentage(scan);
} else if (endTime && !isScanInProgress) {
  // case: scan is finished.
  icon = isScanFinished ? statusIcons.checkCircle : statusIcons.error;
  level = isScanFinished ? StatusLevel.Safe : StatusLevel.Warn;
  value = this.formatDateTimePipe.transform(endTime);

The logic looks correct for showing the checkmark when complete. The issue aervin reported might have been fixed by the live update implementation that now properly uses the pool.scan subscription data. When a scrub completes, the subscription should emit an update with endTime set and isScanInProgress false, triggering the checkmark display.

Suggestion: Add a test case that simulates a scrub completing to verify this transition works correctly:

it('shows checkmark when scrub completes', () => {
  // Start with ongoing scrub
  let poolsInfo = spectator.component.poolsInfo();
  expect(poolsInfo[0].scan.icon).toBe('arrow_circle_right');
  
  // Simulate completion
  scans$.next({
    name: 'poolWithOngoingScrub',
    scan: {
      ...poolWithOngoingScrub.scan,
      state: PoolScanState.Finished,
      end_time: { $date: Date.now() },
      percentage: 100,
    },
  });
  
  spectator.detectChanges();
  poolsInfo = spectator.component.poolsInfo();
  expect(poolsInfo[0].scan.icon).toBe('check_circle');
});

---

Overall, this is a well-structured PR with good attention to testing and performance! The main issues are minor documentation inconsistencies and a missing test case for the completion scenario.

[bugclerk]

Jira URL: https://ixsystems.atlassian.net/browse/NAS-138173

[codecov[bot]]

Codecov Report

:x: Patch coverage is 75.00000% with 5 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 86.43%. Comparing base (1622a10) to head (7041901). :warning: Report is 40 commits behind head on master. :white_check_mark: All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...ges/dashboard/services/widget-resources.service.ts	20.00%	4 Missing :warning:
...r/resilver-progress/resilver-progress.component.ts	0.00%	1 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #12753      +/-   ##
==========================================
+ Coverage   86.28%   86.43%   +0.15%     
==========================================
  Files        1824     1825       +1     
  Lines       67584    67968     +384     
  Branches     8138     8228      +90     
==========================================
+ Hits        58313    58749     +436     
+ Misses       9271     9219      -52     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:

• :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[mattwyatt-ix]

time 04:00

[mattwyatt-ix]

@aervin glad you caught that! i resolved that live-update issue, and it ended up being a little more involved than i originally expected, but i also ended up fixing the storage component being redrawn in its entirety over-and-over.

[mattwyatt-ix]

had to make a few changes due to this middleware PR but we should be good now.

[bugclerk]

This PR has been merged and conversations have been locked. If you would like to discuss more about this issue please use our forums or raise a Jira ticket.