NAS-141483: Address review — result-derived successMessage, trim restated comments

Claude-Session: https://claude.ai/code/session_01SUZQL1eT6RcmdRR59eYxpY

Author: AlexKarpov98

src/app/modules/forms/ix-forms/components/ix-form/ix-form.component.spec.ts

@@ -1247,8 +1247,9 @@ describe('IxFormComponent', () => {
         // This block asserts the delay itself, so restore the real duration that
         // `ixFormTestingProviders()` zeroes for every other spec.
         { provide: ixFormMinSubmitFeedbackMs, useValue: defaultMinSubmitFeedbackMs },
-        // Force the `<tn-side-panel>` host: no SlideInRef (the harness would otherwise auto-mock
-        // one, taking the un-delayed legacy path). `null` is what `inject(…, {optional:true})` sees.
+        // Force the `<tn-side-panel>` host: `null` is exactly what `inject(SlideInRef,
+        // {optional: true})` sees when nothing provides one, and stating it here keeps the block
+        // independent of whatever the surrounding suite happens to provide.
         { provide: SlideInRef, useValue: null },
         mockAuth(),
       ],
@@ -1362,5 +1363,36 @@ describe('IxFormComponent', () => {
       expect(warn).not.toHaveBeenCalled();
       warn.mockRestore();
     });
+
+    it('builds the snackbar from the request result when successMessage is a function', () => {
+      submitHandlerSpy.mockReturnValue({
+        request$: of({ id: 1, name: 'saved-record' }),
+        successMessage: (result: { name: string }) => `Saved «${result.name}».` as TranslatedString,
+      });
+
+      const sidePanelSpectator = createSidePanelComponent({
+        providers: [{ provide: ixFormMinSubmitFeedbackMs, useValue: 0 }],
+      });
+      sidePanelSpectator.component.ixForm().submit();
+
+      expect(sidePanelSpectator.inject(SnackbarService).success).toHaveBeenCalledWith('Saved «saved-record».');
+    });
+
+    it('stays silent without warning when a successMessage function returns null', () => {
+      // A function returning `null` decided, for this result, that no confirmation is wanted (the
+      // dataset form does exactly that when the save navigates on to the ACL editor). That's an
+      // explicit choice, unlike a statically `null` message, so it must not dev-warn.
+      const warn = jest.spyOn(console, 'warn').mockImplementation();
+      submitHandlerSpy.mockReturnValue({ request$: of({ id: 1 }), successMessage: () => null });
+
+      const sidePanelSpectator = createSidePanelComponent({
+        providers: [{ provide: ixFormMinSubmitFeedbackMs, useValue: 0 }],
+      });
+      sidePanelSpectator.component.ixForm().submit();
+
+      expect(sidePanelSpectator.inject(SnackbarService).success).not.toHaveBeenCalled();
+      expect(warn).not.toHaveBeenCalledWith(expect.stringContaining('null successMessage'));
+      warn.mockRestore();
+    });
   });
 });

src/app/modules/forms/ix-forms/components/ix-form/ix-form.component.ts

@@ -82,12 +82,16 @@ interface SubmitResultBase<R, TResult> {
   request$: Observable<TResult>;
 
   /**
-   * Success snackbar text. Required, but nullable: pass `null` — visibly, at the callsite — for a
-   * form that raises its own snackbar (or deliberately raises none) under
-   * `[suppressSuccessSnackbar]`. A `null` without that input set is a silent save, and warns in
-   * dev mode.
+   * Success snackbar text — a string, or a function of the request result for the common case of a
+   * confirmation that names the saved record (which a static string can't reach).
+   *
+   * Required, but nullable: pass `null` — visibly, at the callsite — for a form that raises its own
+   * snackbar (or deliberately raises none) under `[suppressSuccessSnackbar]`. A `null` without that
+   * input set is a silent save, and warns in dev mode. The function form may also return `null`,
+   * for a save whose confirmation depends on the outcome (e.g. none when the success path navigates
+   * away); that's an explicit per-result decision, so it never warns.
    */
-  successMessage: TranslatedString | null;
+  successMessage: TranslatedString | ((result: TResult) => TranslatedString | null) | null;
 
   /** Runs after success, before close (store/navigation fire pre-animation). */
   onSuccess?: (result: TResult) => void;
@@ -416,9 +420,12 @@ export class IxFormComponent<
         handledSuccess = true;
         this.hadSuccessfulSubmit = true;
         if (!this.suppressSuccessSnackbar()) {
-          if (successMessage) {
-            this.snackbar.success(successMessage);
-          } else if (isDevMode()) {
+          const message = typeof successMessage === 'function' ? successMessage(result) : successMessage;
+          if (message) {
+            this.snackbar.success(message);
+          } else if (successMessage === null && isDevMode()) {
+            // Only a statically `null` successMessage warns — a function that returned `null` chose
+            // silence for this particular result, which is a supported outcome.
             console.warn(
               '[ix-form] submitHandler returned a null successMessage and suppressSuccessSnackbar is not '
               + 'set, so this save gives the user no confirmation. Provide a successMessage, or set '

src/app/pages/datasets/components/dataset-capacity-management-card/dataset-capacity-settings/dataset-capacity-settings.component.ts

@@ -41,7 +41,6 @@ export class DatasetCapacitySettingsComponent extends IxFormHostForm implements
   private validators = inject(IxValidatorsService);
   private destroyRef = inject(DestroyRef);
 
-  /** Read by the `<tn-side-panel>` host to role-gate its footer Save. */
   readonly requiredRoles = [Role.DatasetWrite];
   protected readonly InputType = InputType;
 

src/app/pages/datasets/components/dataset-form/dataset-form.component.html

@@ -3,7 +3,6 @@
   [formGroup]="form"
   [externalLoading]="isLoading()"
   [extraDisabled]="!areSubFormsValid()"
-  [suppressSuccessSnackbar]="true"
   [submitHandler]="handleSubmit"
   (closed)="onFormClosed($event)"
 >

src/app/pages/datasets/components/dataset-form/dataset-form.component.spec.ts

@@ -334,8 +334,8 @@ describe('DatasetFormComponent', () => {
   });
 
   describe('success messages', () => {
-    // The form saves with a `null` successMessage under `[suppressSuccessSnackbar]` (its message
-    // needs the saved record), so these snackbars are the ONLY confirmation a save produces.
+    // These snackbars are the ONLY confirmation a save produces, and they are built from the saved
+    // record by `successMessage`'s function form.
     // Declining the ACL prompt keeps us off the navigate-away branch, which is deliberately silent.
     const declineAclPrompt = [mockProvider(DialogService, { confirm: jest.fn(() => of(false)) })];
 

src/app/pages/datasets/components/dataset-form/dataset-form.component.ts

@@ -23,7 +23,6 @@ import {
   FormSubmitEvent, IxFormComponent, SubmitResult,
 } from 'app/modules/forms/ix-forms/components/ix-form/ix-form.component';
 import { SidePanelFooterAction } from 'app/modules/slide-ins/form-side-panel/form-side-panel-container.component';
-import { SnackbarService } from 'app/modules/snackbar/services/snackbar.service';
 import { ApiService } from 'app/modules/websocket/api.service';
 import {
   EncryptionSectionComponent,
@@ -69,7 +68,6 @@ export class DatasetFormComponent extends IxFormHostForm<Dataset | null> impleme
   private datasetFormService = inject(DatasetFormService);
   private router = inject(Router);
   private errorHandler = inject(ErrorHandlerService);
-  private snackbar = inject(SnackbarService);
   private translate = inject(TranslateService);
   private store$ = inject<Store<AppState>>(Store);
 
@@ -83,7 +81,6 @@ export class DatasetFormComponent extends IxFormHostForm<Dataset | null> impleme
   private quotasSection = viewChild(QuotasSectionComponent);
   private otherOptionsSection = viewChild(OtherOptionsSectionComponent);
 
-  /** Read by the `<tn-side-panel>` host to role-gate its footer Save. */
   readonly requiredRoles = [Role.DatasetWrite];
 
   protected readonly isNameAndOptionsValid = signal(true);
@@ -293,9 +290,19 @@ export class DatasetFormComponent extends IxFormHostForm<Dataset | null> impleme
 
     return {
       request$: this.saveDataset(request$),
-      // The message depends on the saved record, so `onSaved` raises it instead — the form runs
-      // under `[suppressSuccessSnackbar]`, which is what makes this `null` deliberate.
-      successMessage: null,
+      // Owned by the form (not its openers) so every entry point confirms identically, and phrased
+      // as plain "created"/"updated" to match the zvol form — a message that has to hold for every
+      // opener can't assume the user is being taken to the new record.
+      successMessage: ([savedDataset, shouldGoToAclEditor]) => {
+        // Accepting the ACL prompt navigates to the ACL editor; a toast about the dataset would
+        // land on a page the user has already left.
+        if (shouldGoToAclEditor) {
+          return null;
+        }
+        return this.isNew()
+          ? this.translate.instant('Dataset «{name}» created.', { name: getDatasetLabel(savedDataset) })
+          : this.translate.instant('Dataset «{name}» updated.', { name: getDatasetLabel(savedDataset) });
+      },
       onSuccess: ([savedDataset, shouldGoToAclEditor]) => this.onSaved(savedDataset, shouldGoToAclEditor),
       closeWith: ([savedDataset]) => savedDataset,
       onError: (error: unknown) => {
@@ -334,17 +341,6 @@ export class DatasetFormComponent extends IxFormHostForm<Dataset | null> impleme
     }
 
     this.aclEditorPath = shouldGoToAclEditor ? savedDataset.mountpoint : undefined;
-
-    if (!shouldGoToAclEditor) {
-      // Phrased as plain "created", matching the zvol form: now that the forms own their messages
-      // they must hold for every opener, and not all of them navigate to the new record (the
-      // explorer's Create Zvol just fills the field in place).
-      this.snackbar.success(
-        this.isNew()
-          ? this.translate.instant('Dataset «{name}» created.', { name: getDatasetLabel(savedDataset) })
-          : this.translate.instant('Dataset «{name}» updated.', { name: getDatasetLabel(savedDataset) }),
-      );
-    }
   }
 
   /**

src/app/pages/datasets/components/zvol-form/zvol-form.component.spec.ts

@@ -157,8 +157,9 @@ describe('ZvolFormComponent', () => {
     ],
     providers: [
       mockApi([
-        mockCall('pool.dataset.create', { id: 'parentId/new zvol' } as Dataset),
-        mockCall('pool.dataset.update', { id: 'zvolId' } as Dataset),
+        // `name` matters: the success snackbars are built from the saved record, not the payload.
+        mockCall('pool.dataset.create', { id: 'parentId/new zvol', name: 'parentId/new zvol' } as Dataset),
+        mockCall('pool.dataset.update', { id: 'zvolId', name: 'zvolId' } as Dataset),
         mockCall('pool.dataset.recommended_zvol_blocksize', '16K' as DatasetRecordSize),
         mockCall('pool.dataset.query', (params) => {
           if ((params[0][0] as QueryFilter<Dataset>)[2] === 'parentId') {

src/app/pages/datasets/components/zvol-form/zvol-form.component.ts

@@ -100,7 +100,6 @@ export class ZvolFormComponent extends IxFormHostForm<Dataset> implements OnInit
 
   private destroyRef = inject(DestroyRef);
 
-  /** Read by the `<tn-side-panel>` host to role-gate its footer Save. */
   readonly requiredRoles = [Role.DatasetWrite];
 
   /** Edit/create parameters supplied by the `<tn-side-panel>` host. */
@@ -266,9 +265,10 @@ export class ZvolFormComponent extends IxFormHostForm<Dataset> implements OnInit
     return {
       request$: this.api.call('pool.dataset.create', [data as DatasetCreate]),
       // Owned by the form so every entry point (details panel, details card, explorer) confirms
-      // identically — the openers deliberately raise no snackbar of their own.
-      successMessage: this.translate.instant('Zvol «{name}» created.', {
-        name: getDatasetLabel({ name: data.name ?? '' }),
+      // identically — the openers deliberately raise no snackbar of their own. Named from the
+      // created record rather than the payload, so the toast can't drift from what was saved.
+      successMessage: (created) => this.translate.instant('Zvol «{name}» created.', {
+        name: getDatasetLabel(created),
       }),
       // The opener needs the record to switch to the new zvol.
       closeWith: (result) => result,
@@ -286,12 +286,11 @@ export class ZvolFormComponent extends IxFormHostForm<Dataset> implements OnInit
           return this.api.call('pool.dataset.update', [this.parentOrZvolId(), payload]);
         }),
       ),
-      // See `buildCreateResult` — the message is the form's, not the opener's. On edit
-      // `parentOrZvolId()` IS the zvol's own id (create passes the parent), and it names the zvol
-      // correctly only because the Name control is disabled in edit mode — a rename would have to
-      // read the submitted value instead.
-      successMessage: this.translate.instant('Zvol «{name}» updated.', {
-        name: getDatasetLabel({ name: this.parentOrZvolId() }),
+      // See `buildCreateResult` — the message is the form's, not the opener's, and it names the
+      // updated record rather than `parentOrZvolId()`, so it stays correct if Name ever becomes
+      // editable on edit.
+      successMessage: (updated) => this.translate.instant('Zvol «{name}» updated.', {
+        name: getDatasetLabel(updated),
       }),
       closeWith: (result) => result,
       onError: (error: unknown) => {

src/app/pages/datasets/modules/snapshots/snapshot-add-form/snapshot-add-form.component.ts

@@ -65,7 +65,6 @@ export class SnapshotAddFormComponent extends IxFormHostForm implements OnInit {
   private storageService = inject(StorageService);
   private destroyRef = inject(DestroyRef);
 
-  /** Read by the `<tn-side-panel>` host to role-gate its footer Save. */
   readonly requiredRoles = [Role.SnapshotWrite];
 
   /** Initial options load. Drives the panel's progress bar and busy overlay. */