NAS-139093 / 26.04 / feat: add auto-expand to jobs page and improve details on cell state button

[mattwyatt-ix]

Changes:
the error dialog we pop up when clicking on a failed job's state button didn't provide any information beyond the error message we got from middleware. specifically, in the case for a failed rsync task, the message we displayed from middleware directed the user to Check logs for further information, but our popup didn't show any indication of how to get there. this PR improves that experience a little bit.

• add a URL parameter and handling to the jobs list page. this allows us to navigate to the jobs page and have it immediately expand one of the rows if we want to highlight something.
• add logic to the job state button we use in ix-tables to show a Download Logs button and a View Details button when those details are available.
• when already on the jobs page, the View Details button will not appear so as not to confuse users that may believe it would take them elsewhere.
• fix a slightly unrelated bug where we were mutating an argument inside a function, which was causing the rsync tasks card not to show all the tasks.

Testing:

1. create an rsync task (or any other task) and have it fail.
2. click on the big red FAILED button on the job's table row and observe the two new buttons.
3. observe that its logs are downloadable and that the View Details button takes you to the jobs page and automatically expands the proper job's row.

Screenshots and screen recordings:

the new error popup

Screencast_20260105_094426.webm

Downstream

Affects	Reasoning
Documentation
Testing

[claude]

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

---

Code Review Complete! ✅

Great work on this UX improvement! 🎉 The implementation is well-structured, thoroughly tested, and provides a much better user experience for accessing job details and logs.

Positive Aspects

The PR demonstrates excellent attention to detail with comprehensive test coverage and thoughtful UX considerations like hiding the "View Details" button when already on the jobs page. The bug fix in the rsync component is a good catch!

Issues Found

1. 🔴 Critical: Spread operator mutation in transformRsyncTasks still present

Location: src/app/pages/data-protection/rsync-task/rsync-task-card/rsync-task-card.component.ts:233

While you fixed the mutation in updateRowStateAndJob (line 265), the original issue still exists in transformRsyncTasks:

private transformRsyncTasks(rsyncTasks: RsyncTaskUi[]): RsyncTaskUi[] {
  return rsyncTasks.map((rsyncTask: RsyncTaskUi) => {
    const task: RsyncTaskUi = { ...rsyncTask };  // ⚠️ Shallow copy only
    if (task.job === null) {
      task.state = { state: task.locked ? TaskState.Locked : TaskState.Pending };
    } else {
      task.state = { state: task.job.state };  // ✅ This is fine
      this.store$.select(selectJob(task.job.id)).pipe(filter(Boolean), untilDestroyed(this))
        .subscribe((job: Job) => {
          task.state = { state: job.state };  // ⚠️ Mutating the shallow copied object
          task.job = job;  // ⚠️ This mutates the original rsyncTask.job reference!
          this.jobStates.set(job.id, job.state);
        });
    }
    return task;
  });
}

The issue: Even though you're spreading rsyncTask, the job property is still a reference to the original object. When you do task.job = job, you're replacing a reference that might be shared. More importantly, the subscription is mutating task which affects the already-returned array.

Why this matters: This pattern can lead to unexpected side effects, especially with Angular's change detection and NgRx state management. The mutation happens asynchronously in the subscription, affecting data that's already been processed.

Suggested fix: Store job updates separately or ensure complete immutability:

private transformRsyncTasks(rsyncTasks: RsyncTaskUi[]): RsyncTaskUi[] {
  return rsyncTasks.map((rsyncTask: RsyncTaskUi) => {
    const task: RsyncTaskUi = { 
      ...rsyncTask,
      job: rsyncTask.job ? { ...rsyncTask.job } : null  // Deep copy the job
    };
    
    if (task.job === null) {
      task.state = { state: task.locked ? TaskState.Locked : TaskState.Pending };
    } else {
      task.state = { state: task.job.state };
      // Consider updating rsyncTasks array immutably instead of mutating task
      this.store$.select(selectJob(task.job.id)).pipe(
        filter(Boolean), 
        untilDestroyed(this)
      ).subscribe((job: Job) => {
        const updatedTask = { ...task, state: { state: job.state }, job };
        const index = this.rsyncTasks.findIndex(t => t.id === rsyncTask.id);
        if (index !== -1) {
          this.rsyncTasks = [
            ...this.rsyncTasks.slice(0, index),
            updatedTask,
            ...this.rsyncTasks.slice(index + 1)
          ];
          this.dataProvider.setRows(this.rsyncTasks);
        }
        this.jobStates.set(job.id, job.state);
      });
    }
    return task;
  });
}

2. 🟡 Medium: Missing actions parameter in error dialog call

Location: src/app/modules/ix-table/components/ix-table-body/cells/ix-cell-state-button/ix-cell-state-button.component.ts:179

The test at line 179 in the spec file expects actions to be undefined when on the jobs page, but you're not passing it at all. While this works, it's inconsistent with how you handle it at lines 171-177.

Current code:

this.dialogService.error({
  title: state.state,
  message,
  // since these two may be undefined and are optional, we can just include them in the call outright
  logs,
  actions,
});

Issue: When actions is undefined (line 177 sets it conditionally), you're still passing actions: undefined. This is technically correct but inconsistent with the comment.

Suggested improvement: Either explicitly set actions: undefined when not needed, or use conditional spreading:

this.dialogService.error({
  title: state.state,
  message,
  ...(logs && { logs }),
  ...(actions && { actions }),
});

3. 🟡 Medium: Potential memory leak with effect in IxCellStateButtonComponent

Location: src/app/modules/ix-table/components/ix-table-body/cells/ix-cell-state-button/ix-cell-state-button.component.ts:62-64

The component uses both effect() and ngOnInit() to call setupRow(), which could lead to unnecessary work:

private readonly rowUpdateEffect = effect(() => {
  this.setupRow();
});

ngOnInit(): void {
  this.setupRow();
}

Issue: The effect will trigger on every signal change, and ngOnInit also calls it. This means setupRow() is called twice on initialization. Additionally, setupRow() creates a new observable subscription (jobUpdates$) each time without cleaning up the previous one.

Suggested fix: Either use only the effect OR only ngOnInit, and ensure proper cleanup:

// Option 1: Use effect only
private readonly rowUpdateEffect = effect(() => {
  this.setupRow();
}, { allowSignalWrites: false });

// Remove ngOnInit call

// Option 2: Add cleanup if keeping both
private jobSubscription?: Subscription;

private setupRow(): void {
  // ... existing code ...
  
  this.jobSubscription?.unsubscribe(); // Clean up previous subscription
  this.jobUpdates$ = this.store$.select(selectJob(jobId)).pipe(
    filter((job) => !!job),
    tap((job) => {
      this.job.set(job);
      this.state.set(job.state);
    }),
    untilDestroyed(this),
  ) as Observable<Job<ApiJobResponse<ApiJobMethod>>>;
}

4. 🔵 Minor: Test implementation detail exposure

Location: src/app/modules/ix-table/components/ix-table-body/cells/ix-cell-state-button/ix-cell-state-button.component.spec.ts:105-106

The test directly accesses and modifies the component's getWarnings function:

spectator.component.getWarnings = (row) => row.warnings;

While this works, it's testing implementation details rather than behavior. Consider testing through the public API by passing getWarnings during component setup.

5. 🔵 Minor: Inconsistent error dialog title in error-parser.service.ts

Location: src/app/services/errors/error-parser.service.ts:193-198

In parseJobError, you're adding actions to show "View Details" and "Download Logs", but these are always added, even when logs is undefined:

return {
  title: job.state,
  message: job.error || job.exception || this.translate.instant('Unknown error'),
  stackTrace: job.logs_excerpt || job.exception,
  // display a `View Details` button and `Download Logs` button on any failed job dialogs
  actions: [{
    label: this.translate.instant('View Details'),
    route: '/jobs',
    params: { jobId: job.id },
  }],
  logs: job.logs_excerpt ? job : undefined,
};

Issue: This differs from the implementation in ix-cell-state-button.component.ts:154 where you only add actions when logs_excerpt exists. The error parser should follow the same pattern for consistency.

Suggested fix:

return {
  title: job.state,
  message: job.error || job.exception || this.translate.instant('Unknown error'),
  stackTrace: job.logs_excerpt || job.exception,
  ...(job.logs_excerpt && {
    actions: [{
      label: this.translate.instant('View Details'),
      route: '/jobs',
      params: { jobId: job.id },
    }],
    logs: job,
  }),
};

Summary

Excellent feature implementation with one critical issue that needs addressing. The mutation bug in transformRsyncTasks is the most important fix needed. The other issues are medium/minor but would improve code quality and prevent potential future bugs. 🚀

---

[bugclerk]

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

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.72%. Comparing base (297a642) to head (5720adb).
⚠️ Report is 4 commits behind head on master.
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #13059   +/-   ##
=======================================
  Coverage   86.71%   86.72%           
=======================================
  Files        1847     1847           
  Lines       69415    69451   +36     
  Branches     8585     8599   +14     
=======================================
+ Hits        60193    60230   +37     
+ Misses       9222     9221    -1     

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