NAS-138979 / 26.04 / feat: implement immediate updates for disk forms and list

[mattwyatt-ix]

Changes:
when saving the disk edit form or the bulk disk edit form, we call AsyncDataProvider.load which calls the API methods disk.query and disk.details. since these take a while to finish executing, we were seeing inconsistency if you went to edit a disk that you had just edited. this PR updates that information client-side in the interim while we wait for the API to give us back the real data.

there is a minor concern for a data race in the following scenario: if a user were to quickly make two edits to one disk, there would be a period of time where we may receive the first edit from the API, but not the second, and the user sees their latest edit revert. this would happen due to the API being slightly behind. i think this has an acceptably-low chance of happening and causing any confusion.

also, we now clear the contents of the SED password field when checking the box. if the box is checked, the password is actually cleared in the logic and on the backend, but users may get confused if it looks like the password is still in the box.

Testing:

1. on the disks page, edit a disk and save it. specifically, for the ticket, edit or clear the SED password.
2. quickly open that same disk's edit form back up and observe that the values are still populated correctly, as opposed to the prior behavior where it may not have updated yet.
3. do the same for the bulk-edit workflow.

note that the clear SED password checkbox visually clears it too.

Downstream

Affects	Reasoning
Documentation
Testing

[claude]

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

---

PR Review: Immediate Updates for Disk Forms and List

Great approach to solving the race condition! 🎯 The solution elegantly handles the slow API response by optimistically updating the UI with client-side data while waiting for the real API response.

✅ What Works Well

The implementation correctly addresses the core issue by returning the update payload from both form components and using it for immediate UI updates in the list component. The test coverage is solid and properly validates the new return types.

🔍 Issues to Address

1. Data Inconsistency Risk (disk-list.component.ts:280-298)

The current implementation updates this.disks but doesn't update this.unusedDisks, which could cause inconsistencies. Consider this scenario:

// User edits disk sda (currently in unusedDisks)
// diskUpdates$ fires and updates this.disks
this.disks = this.disks.map((disk) => {
  if (disk.identifier === diskUpdate.identifier) {
    return { ...disk, ...diskUpdate }; // ✅ Updated
  }
  return disk;
});

// But this.unusedDisks still has stale data!
// getPoolColumn() reads from this.unusedDisks (line 374)
// So the Pool column may show stale data

Solution: Either update both arrays or refactor to eliminate the dual source of truth:

this.diskUpdates$.pipe(
  takeUntilDestroyed(this.destroyRef),
).subscribe((diskUpdate) => {
  // Update disks array
  this.disks = this.disks.map((disk) => {
    if (disk.identifier === diskUpdate.identifier) {
      return { ...disk, ...diskUpdate };
    }
    return disk;
  });

  // Update unusedDisks if the disk exists there
  this.unusedDisks = this.unusedDisks.map((disk) => {
    if (disk.identifier === diskUpdate.identifier) {
      return { ...disk, ...diskUpdate };
    }
    return disk;
  });

  this.dataProvider.setRows(this.disks);
});

2. Missing Client-Side Pool Recalculation (disk-list.component.ts:373-379)

When you update a disk, the pool property is derived from getPoolColumn(), which depends on unusedDisks. Your optimistic update doesn't recalculate this:

// In diskUpdates$ handler, you do:
return { ...disk, ...diskUpdate }; // ❌ Doesn't recalculate pool

// Should be:
const updatedDisk = { ...disk, ...diskUpdate };
return {
  ...updatedDisk,
  pool: this.getPoolColumn(updatedDisk), // ✅ Recalculate derived data
};

3. Incomplete Bulk Edit Error Handling (disk-bulk-edit.component.ts:154-162)

When isSuccessful === false, you close with an empty response array, which means diskUpdates$ won't emit anything. This leaves the UI in a stale state - the user sees old data even though some disks may have been updated successfully.

// Current code on partial failure:
this.slideInRef.close({ response: [] }); // ❌ Loses successful updates

// Better approach - return the successful ones:
const successfulUpdates = req
  .filter((_, idx) => job.result[idx].error === null)
  .map((diskUpdate) => ({
    identifier: diskUpdate[0],
    ...diskUpdate[1],
  }));
this.slideInRef.close({ response: successfulUpdates });

4. Test Gap (disk-bulk-edit.component.spec.ts:123-125)

The bulk edit test verifies that slideInRef.close is called, but doesn't verify the content of the response like the single disk form test does:

// disk-form.component.spec.ts:112-119 ✅ Checks response content
expect(spectator.inject(SlideInRef).close).toHaveBeenCalledWith({
  response: [{
    identifier: '{serial}VB9fbb6dfe-9cf26570',
    advpowermgmt: '64',
    // ... validates full payload
  }],
});

// disk-bulk-edit.component.spec.ts:124 ❌ Only checks it was called
expect(spectator.inject(SlideInRef).close).toHaveBeenCalled();

Add assertions to verify the response payload structure.

📝 Minor Observations

• Good: The passwd field cleanup (markAsPristine(), setValue('')) in disk-form.component.ts:113-116 properly handles form state
• Good: Test coverage for the SED password clearing behavior is comprehensive
• Note: The comment at line 295 mentions "5-10s" - consider making this configurable or documenting where this latency comes from

Summary

Solid architectural approach with proper type safety and test coverage. Address the data consistency issues (particularly the unusedDisks and pool recalculation) to ensure the optimistic update fully mirrors the API response.

---

[bugclerk]

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

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.72%. Comparing base (1b4fd67) to head (171e5aa).
⚠️ Report is 10 commits behind head on master.
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #13050      +/-   ##
==========================================
+ Coverage   86.69%   86.72%   +0.02%     
==========================================
  Files        1846     1847       +1     
  Lines       69371    69429      +58     
  Branches     8574     8586      +12     
==========================================
+ Hits        60143    60211      +68     
+ Misses       9228     9218      -10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

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

[aervin]

works as described

[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.