fix(workflow): keep a newer local edit over a save's response

frontend/src/app/common/service/workflow-persist/workflow-persist.service.spec.ts

@@ -46,6 +46,7 @@ import { DashboardWorkflow } from "../../../dashboard/type/dashboard-workflow.in
 import { DefaultView } from "../../../dashboard/type/workflow-metadata.interface";
 import { SearchFilterParameters, toQueryStrings } from "../../../dashboard/type/search-filter-parameters";
 import { NotificationService } from "../notification/notification.service";
+import { WorkflowActionService } from "../../../workspace/service/workflow-graph/model/workflow-action.service";
 import { last } from "rxjs/operators";
 
 describe("WorkflowPersistService", () => {
@@ -70,9 +71,15 @@ describe("WorkflowPersistService", () => {
     '{"linkID":"link-c94e24a6-2c77-40cf-ba22-1a7ffba64b7d","source":{"operatorID":' +
     '"MySQLSource-operator-1ee619b1-8884-4564-a136-29ef77dfcc50","portID":"output-0"},"target":' +
     '{"operatorID":"Limit-operator-a11370eb-940a-4f10-8b36-8b413b2396c9","portID":"input-0"}}],"breakpoints":{}}';
+  // What the page currently holds as the open workflow's metadata (read at response time to keep
+  // the user's name/description edits). Another workflow by default, so a response is relayed as
+  // is; the tests about local edits point it at the saved workflow.
+  let currentMetadata: { wid: number | undefined; name: string; description: string | undefined };
   beforeEach(() => {
+    currentMetadata = { wid: 999, name: "another workflow", description: undefined };
     TestBed.configureTestingModule({
       imports: [HttpClientTestingModule],
+      providers: [{ provide: WorkflowActionService, useValue: { getWorkflowMetadata: () => currentMetadata } }],
     });
     service = TestBed.inject(WorkflowPersistService);
     httpTestingController = TestBed.inject(HttpTestingController);
@@ -260,6 +267,87 @@ describe("WorkflowPersistService", () => {
       expect(secondName).toBe("second");
     });
 
+    describe("a response versus an edit made since the save was sent", () => {
+      const wf = (name: string) => ({ wid: 9, name, description: "d1", content: validContent }) as unknown as Workflow;
+
+      it("relays the response with the page's current name and description, not the ones it was saved with", () => {
+        currentMetadata = { wid: 9, name: "renamed meanwhile", description: "described meanwhile" };
+        let result: Workflow | undefined;
+        service.persistWorkflow(wf("old")).subscribe(w => (result = w));
+
+        httpTestingController
+          .expectOne(`${API}/${WORKFLOW_PERSIST_URL}`)
+          .flush({ wid: 9, name: "old", description: "d1", lastModifiedTime: 777, content: "{}" });
+
+        // Feeding this back as the metadata keeps the rename; the server-owned fields still arrive.
+        expect(result?.name).toBe("renamed meanwhile");
+        expect(result?.description).toBe("described meanwhile");
+        expect(result?.lastModifiedTime).toBe(777);
+      });
+
+      it("keeps the local name for a workflow the save has just created (the page still holds the default id)", () => {
+        currentMetadata = { wid: 0, name: "named before the first save answered", description: undefined };
+        let result: Workflow | undefined;
+        service.persistWorkflow({ ...wf("Untitled workflow"), wid: 0 } as Workflow).subscribe(w => (result = w));
+
+        httpTestingController
+          .expectOne(`${API}/${WORKFLOW_PERSIST_URL}`)
+          .flush({ wid: 42, name: "Untitled workflow", content: "{}" });
+
+        expect(result?.wid).toBe(42);
+        expect(result?.name).toBe("named before the first save answered");
+      });
+
+      it("leaves the response alone when another workflow is open by the time it answers", () => {
+        currentMetadata = { wid: 10, name: "the other one", description: undefined };
+        let result: Workflow | undefined;
+        service.persistWorkflow(wf("old")).subscribe(w => (result = w));
+
+        httpTestingController.expectOne(`${API}/${WORKFLOW_PERSIST_URL}`).flush({ wid: 9, name: "old", content: "{}" });
+
+        expect(result?.name).toBe("old");
+      });
+    });
+
+    describe("whenSavesDrained", () => {
+      const wf = (name: string) => ({ wid: 9, name, description: "", content: validContent }) as unknown as Workflow;
+
+      it("emits at once when no save is pending", () => {
+        let emitted = false;
+        service.whenSavesDrained().subscribe(() => (emitted = true));
+        expect(emitted).toBe(true);
+      });
+
+      it("emits only once the last queued save has answered, not when the first has", () => {
+        service.persistWorkflow(wf("first")).subscribe();
+        service.persistWorkflow(wf("second")).subscribe();
+        let emitted = false;
+        service.whenSavesDrained().subscribe(() => (emitted = true));
+
+        httpTestingController
+          .expectOne(`${API}/${WORKFLOW_PERSIST_URL}`)
+          .flush({ wid: 9, name: "first", content: "{}" });
+        expect(emitted).toBe(false); // the second is still out
+
+        httpTestingController
+          .expectOne(`${API}/${WORKFLOW_PERSIST_URL}`)
+          .flush({ wid: 9, name: "second", content: "{}" });
+        expect(emitted).toBe(true);
+      });
+
+      it("counts a failed save as done, so a failure does not hold the drain forever", () => {
+        service.persistWorkflow(wf("first")).subscribe({ error: () => {} });
+        let emitted = false;
+        service.whenSavesDrained().subscribe(() => (emitted = true));
+
+        httpTestingController
+          .expectOne(`${API}/${WORKFLOW_PERSIST_URL}`)
+          .flush("boom", { status: 500, statusText: "Server Error" });
+
+        expect(emitted).toBe(true);
+      });
+    });
+
     it("persistWorkflow notifies the user when the workflow is broken but still POSTs", () => {
       const errorSpy = vi.spyOn(notificationService, "error").mockImplementation(() => {});
       const workflow = {

frontend/src/app/common/service/workflow-persist/workflow-persist.service.ts

@@ -18,14 +18,18 @@
  */
 
 import { HttpClient, HttpParams } from "@angular/common/http";
-import { Injectable } from "@angular/core";
-import { EMPTY, Observable, ReplaySubject, Subject, throwError } from "rxjs";
-import { catchError, concatMap, filter, map, tap } from "rxjs/operators";
+import { Injectable, Injector } from "@angular/core";
+import { EMPTY, Observable, of, ReplaySubject, Subject, throwError } from "rxjs";
+import { catchError, concatMap, filter, finalize, map, take, tap } from "rxjs/operators";
 import { AppSettings } from "../../app-setting";
 import { Workflow, WorkflowContent } from "../../type/workflow";
 import { DashboardWorkflow } from "../../../dashboard/type/dashboard-workflow.interface";
 import { DefaultView } from "../../../dashboard/type/workflow-metadata.interface";
 import { WorkflowUtilService } from "../../../workspace/service/workflow-graph/util/workflow-util.service";
+import {
+  DEFAULT_WORKFLOW,
+  WorkflowActionService,
+} from "../../../workspace/service/workflow-graph/model/workflow-action.service";
 import { NotificationService } from "../notification/notification.service";
 import { SearchFilterParameters, toQueryStrings } from "../../../dashboard/type/search-filter-parameters";
 import { User } from "../../type/user";
@@ -68,29 +72,74 @@ export class WorkflowPersistService {
    * before it has landed too. Each request snapshots its payload when asked for; it is sent when its
    * turn comes, and its outcome is relayed to that caller alone. A failed save fails its own caller
    * and does not hold up the next.
+   *
+   * A response is relayed with the page's current name and description in place of its own (see
+   * withLocalEdits): those are the two fields a user edits, and a response answers the save it was
+   * sent for, which may be older than an edit made since. Callers feed the response back as the
+   * workflow's metadata; without this, a rename made while a save was out came back undone.
    */
   private readonly persistQueue = new Subject<{ send: Observable<Workflow>; result: Subject<Workflow> }>();
 
+  /** Saves asked for and not yet answered (or failed); see whenSavesDrained. */
+  private pendingSaves = 0;
+  private readonly savesDrained = new Subject<void>();
+
   constructor(
     private http: HttpClient,
-    private notificationService: NotificationService
+    private notificationService: NotificationService,
+    // Looked up lazily, at response time: the persist service is also used by the dashboard, where
+    // no workflow is open and constructing the (graph-owning) action service would be a side effect.
+    private injector: Injector
   ) {
     this.persistQueue
       .pipe(
         concatMap(({ send, result }) =>
           send.pipe(
+            map(updated => this.withLocalEdits(updated)),
             tap({
               next: updated => result.next(updated),
               error: (err: unknown) => result.error(err),
               complete: () => result.complete(),
             }),
-            catchError(() => EMPTY)
+            catchError(() => EMPTY),
+            finalize(() => this.saveDone())
           )
         )
       )
       .subscribe();
   }
 
+  /**
+   * Emits once every save asked for so far has been answered or has failed; at once when none is
+   * pending. For a caller that leaves the page on completion (the view switches): its own save
+   * completing is not enough, a save queued behind it (a rename's, a description's) would still be
+   * aborted by the page load.
+   */
+  public whenSavesDrained(): Observable<void> {
+    return this.pendingSaves === 0 ? of(undefined) : this.savesDrained.pipe(take(1));
+  }
+
+  private saveDone(): void {
+    this.pendingSaves -= 1;
+    if (this.pendingSaves === 0) {
+      this.savesDrained.next();
+    }
+  }
+
+  /**
+   * The response with the page's current name and description: a response carries the values the
+   * save was sent with, and an edit made since would be undone by feeding them back. Left alone when
+   * another workflow is open by now (nothing local belongs to this response); a workflow just created
+   * still carries the default id locally, and its rename made meanwhile is kept too.
+   */
+  private withLocalEdits(response: Workflow): Workflow {
+    const current = this.injector.get(WorkflowActionService).getWorkflowMetadata();
+    if (current.wid !== response.wid && current.wid !== DEFAULT_WORKFLOW.wid) {
+      return response;
+    }
+    return { ...response, name: current.name, description: current.description };
+  }
+
   /**
    * persists a workflow to backend database and returns its updated information (e.g., new wid).
    * The request is queued behind any save still in flight (see persistQueue); the returned
@@ -122,6 +171,7 @@ export class WorkflowPersistService {
     // Replayed, so a caller that subscribes after the queue has already relayed the outcome (a
     // save that was quick, or a synchronous test double) still receives it.
     const result = new ReplaySubject<Workflow>(1);
+    this.pendingSaves += 1;
     this.persistQueue.next({ send, result });
     return result.asObservable();
   }

frontend/src/app/workspace/component/menu/menu.component.spec.ts

@@ -203,6 +203,26 @@ describe("MenuComponent", () => {
     expect(component.isSaving).toBe(false);
   });
 
+  it("leaves only once every queued save has landed, not just its own", () => {
+    // A rename made while the switch's save is out saves through the menu itself and is queued
+    // behind the switch's save; the hand-over waits for the queue to drain, or the page load
+    // would abort that save.
+    component.writeAccess = true;
+    vi.spyOn(component["workflowActionService"], "getWorkflowMetadata").mockReturnValue({ wid: 7 } as any);
+    vi.spyOn(workflowPersistService, "persistWorkflow").mockReturnValue(of({ wid: 7, name: "saved" } as any));
+    vi.spyOn(component["workflowActionService"], "setWorkflowMetadata").mockImplementation(() => {});
+    const drained$ = new Subject<void>();
+    vi.spyOn(workflowPersistService, "whenSavesDrained").mockReturnValue(drained$.asObservable());
+    const navigate = vi.spyOn(component as any, "openFormViewPage").mockImplementation(() => {});
+
+    component.onClickOpenFormView();
+
+    expect(navigate).not.toHaveBeenCalled(); // its own save is done, another is still queued
+    drained$.next();
+    expect(navigate).toHaveBeenCalledWith(7);
+    expect(component.isSaving).toBe(false);
+  });
+
   it("ignores a second click while the hand-over is already in progress", () => {
     component.writeAccess = true;
     vi.spyOn(component["workflowActionService"], "getWorkflowMetadata").mockReturnValue({ wid: 7 } as any);

frontend/src/app/workspace/component/menu/menu.component.ts

@@ -665,11 +665,14 @@ export class MenuComponent implements OnInit, OnDestroy {
     // save that fails keeps the user here with the error shown, rather than leaving with changes
     // that were never stored. The form's own switch (openRegularCanvas) does the same.
     //
-    // Two more things the hand-over must not lose. An autosave already in flight when the switch
+    // Three more things the hand-over must not lose. An autosave already in flight when the switch
     // is clicked: WorkflowPersistService sends saves one at a time and in order, so ours lands after
-    // it and completes after it. And an edit made while our save is out (the page stays editable
-    // until the load): workflowChanged marks it, and the drain below saves once more before handing
-    // over rather than letting the full-page load abort that edit's own debounced autosave.
+    // it and completes after it. A graph edit made while our save is out (the page stays editable
+    // until the load): workflowChanged marks it, and saveThenOpenFormView saves once more before
+    // handing over rather than letting the full-page load abort that edit's own debounced autosave.
+    // And a save queued behind ours (a rename's or a description's, which save through the menu
+    // itself and do not go through workflowChanged): the hand-over leaves only once the service's
+    // save queue has drained.
     this.handingOverToFormView = true;
     this.isSaving = true;
     this.saveThenOpenFormView(wid);
@@ -702,8 +705,15 @@ export class MenuComponent implements OnInit, OnDestroy {
             this.saveThenOpenFormView(target);
             return;
           }
-          this.isSaving = false;
-          this.openFormViewPage(target);
+          // A save queued behind ours (a rename's, a description's: those save through the menu
+          // itself, not the autosave) must land too, or the page load aborts it.
+          this.workflowPersistService
+            .whenSavesDrained()
+            .pipe(untilDestroyed(this))
+            .subscribe(() => {
+              this.isSaving = false;
+              this.openFormViewPage(target);
+            });
         },
       });
   }