From 62e157be955cda2c343367c4ed8fbc3c4c2a6719 Mon Sep 17 00:00:00 2001 From: Yang Zhang Date: Sun, 13 Sep 2026 17:29:29 -0700 Subject: [PATCH 1/6] fix(workflow): keep a newer local edit over a save's response Every save's response is fed back as the workflow's metadata: the canvas autosave, the menu's own save (rename, description, revert) and the Form View switch's save. Saves go out one at a time, so a response can land after a newer local edit and put the old name or description back. Rename while an autosave is out and the title flips back until the rename's own save answers; rename while the Form View switch's save is out and the rename is lost, since the switch left as soon as its own save had completed and the page load aborted the rename's. WorkflowPersistService, the one place every save goes through, now relays each response with the page's current name and description in place of the ones the save was sent with (left alone when another workflow is open by then; kept for a workflow the save has just created, which still carries the default id locally), and exposes whenSavesDrained(), which the Form View switch waits for before leaving, so a save queued behind its own lands before the page unloads. Closes #8536. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01FVvP3ttj22f9LB4p9u2anY --- .../workflow-persist.service.spec.ts | 88 +++++++++++++++++++ .../workflow-persist.service.ts | 61 +++++++++++-- .../component/menu/menu.component.spec.ts | 20 +++++ .../component/menu/menu.component.ts | 27 ++++-- 4 files changed, 185 insertions(+), 11 deletions(-) diff --git a/frontend/src/app/common/service/workflow-persist/workflow-persist.service.spec.ts b/frontend/src/app/common/service/workflow-persist/workflow-persist.service.spec.ts index a35c9ec106e..8a42df86384 100644 --- a/frontend/src/app/common/service/workflow-persist/workflow-persist.service.spec.ts +++ b/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 = { diff --git a/frontend/src/app/common/service/workflow-persist/workflow-persist.service.ts b/frontend/src/app/common/service/workflow-persist/workflow-persist.service.ts index 66d267a673d..7a23c8971de 100644 --- a/frontend/src/app/common/service/workflow-persist/workflow-persist.service.ts +++ b/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,75 @@ 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; result: Subject }>(); + /** Saves asked for and not yet answered (or failed); see whenSavesDrained. */ + private pendingSaves = 0; + private readonly savesDrained = new Subject(); + 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 its view on completion (the Form View switch): its own save + * completing is not enough. A save queued behind it (a rename's, a description's) is still sent + * -- this service outlives the view -- but it answers to the component that asked for it, and a + * component the route has destroyed shows no error and feeds back no response. + */ + public whenSavesDrained(): Observable { + 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 +172,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(1); + this.pendingSaves += 1; this.persistQueue.next({ send, result }); return result.asObservable(); } diff --git a/frontend/src/app/workspace/component/menu/menu.component.spec.ts b/frontend/src/app/workspace/component/menu/menu.component.spec.ts index e9ab4f7e905..627f6b154a0 100644 --- a/frontend/src/app/workspace/component/menu/menu.component.spec.ts +++ b/frontend/src/app/workspace/component/menu/menu.component.spec.ts @@ -241,6 +241,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, so that save's error + // or response still reaches this component rather than one the route has destroyed. + 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(); + 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); diff --git a/frontend/src/app/workspace/component/menu/menu.component.ts b/frontend/src/app/workspace/component/menu/menu.component.ts index e320fc484e2..1395de2f429 100644 --- a/frontend/src/app/workspace/component/menu/menu.component.ts +++ b/frontend/src/app/workspace/component/menu/menu.component.ts @@ -734,11 +734,16 @@ export class MenuComponent implements OnInit, OnDestroy { // than carrying changes that were never stored into a view that has no reason to say so. 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 route): workflowChanged marks it, and the drain below saves once more before handing - // over, so the switch does not leave that edit to an autosave that would fire under the other view. + // it and completes after it. A graph edit made while our save is out (the page stays editable + // until the route): workflowChanged marks it, and saveThenOpenFormView saves once more before + // handing over, so the switch does not leave that edit to an autosave that would fire under the + // other view. 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 route would not abort it, but its + // outcome answers to this component -- the error shown, the response fed back -- and this + // component is gone once the route lands. So the hand-over leaves only once the service's save + // queue has drained. this.handingOverToFormView = true; this.isSaving = true; this.saveThenOpenFormView(wid); @@ -772,8 +777,18 @@ 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) answers to this component: its error is shown here, its + // response fed back here, and neither reaches a component the route has destroyed. Leave + // once it has answered; a failure of its own is reported by its caller and does not hold + // the hand-over. + this.workflowPersistService + .whenSavesDrained() + .pipe(untilDestroyed(this)) + .subscribe(() => { + this.isSaving = false; + this.openFormViewPage(target); + }); }, }); } From 81ee927322b18b24a8c8beb85ad64a1220023253 Mon Sep 17 00:00:00 2001 From: Yang Zhang Date: Thu, 17 Sep 2026 13:09:02 -0700 Subject: [PATCH 2/6] fix(workflow): drop the Form View's own copy of the local-name rule Review follow-up on #8540: with WorkflowPersistService relaying every response with the page's current name and description, the form's persist handler was enforcing the same rule a second time. By the time it ran, the response already carried the current name, so both branches of its guard produced the same value. Keeping both meant two statements of one rule, and the caller-side one was the narrower of the two: name only, keyed off the snapshot the save was sent with. It never covered description, so a reader trusting it would have concluded the description was protected too, and a later change to withLocalEdits could have reintroduced exactly the bug this PR fixes. The spec that pinned the caller-side guard goes with it. It drove the persist service through a double, so it never exercised withLocalEdits at all and only described behaviour the component no longer owns. The rule itself stays covered where it now lives, in workflow-persist.service.spec, and the form's remaining obligation -- feeding the response back so "Saved at ..." advances -- is still covered by "feeds the persist response back into the workflow metadata". Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FVvP3ttj22f9LB4p9u2anY --- .../workflow-form.component.spec.ts | 26 ------------------- .../workflow-form/workflow-form.component.ts | 11 +++----- 2 files changed, 4 insertions(+), 33 deletions(-) diff --git a/frontend/src/app/workspace/component/workflow-form/workflow-form.component.spec.ts b/frontend/src/app/workspace/component/workflow-form/workflow-form.component.spec.ts index 752816414f5..382e0854fae 100644 --- a/frontend/src/app/workspace/component/workflow-form/workflow-form.component.spec.ts +++ b/frontend/src/app/workspace/component/workflow-form/workflow-form.component.spec.ts @@ -630,32 +630,6 @@ describe("WorkflowFormComponent", () => { vi.useRealTimers(); }); - it("does not let an older save's response undo a rename made while it was in flight", () => { - // Save A carries the old name. The author renames to B (B's own save is queued behind A). When - // A returns, its echoed name must not be written back over B, or an autosave in that window - // would carry the old name and the rename would be lost. The server-owned timestamp is kept. - vi.useFakeTimers(); - enableSave(); - build(formViewWorkflow).ngOnInit(); - workflowPersistService.persistWorkflow.mockClear(); - const saveA$ = new Subject(); - workflowPersistService.persistWorkflow.mockReturnValueOnce(saveA$); - h.workflowChangedStream.next(undefined); - vi.runAllTimers(); - expect(workflowPersistService.persistWorkflow).toHaveBeenCalledTimes(1); - - // The rename lands in the shared metadata while A is still out. - workflowActionService.getWorkflowMetadata = () => ({ name: "B", lastModifiedTime: 1 }); - saveA$.next({ ...formViewWorkflow, wid: 7, name: "scGPT", lastModifiedTime: 42 } as any); - saveA$.complete(); - - expect(workflowActionService.setWorkflowMetadata).toHaveBeenCalledTimes(1); - const fedBack = workflowActionService.setWorkflowMetadata.mock.calls[0][0]; - expect(fedBack.name).toBe("B"); - expect(fedBack.lastModifiedTime).toBe(42); - vi.useRealTimers(); - }); - it("hands over only once a save queued behind the switch's has completed too", () => { // The page stays interactive while the switch's save is in flight, so an edit made then gets its // own autosave queued behind it. Navigating on the switch's save alone would abort that newer diff --git a/frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts b/frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts index 9b06c05273e..b9332e222fc 100644 --- a/frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts +++ b/frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts @@ -1972,13 +1972,10 @@ export class WorkflowFormComponent implements OnInit, OnDestroy { if (this.destroyed) { return; } - // The response reflects the snapshot that was sent. A rename made since must not be undone - // by it (its own save is already queued behind this one); what this feedback is for is the - // server-owned part, the timestamp above all, and the normalised name when nothing changed. - const current = this.workflowActionService.getWorkflowMetadata(); - this.workflowActionService.setWorkflowMetadata( - current.name !== preserved.name ? { ...updatedWorkflow, name: current.name } : updatedWorkflow - ); + // Fed back as it arrives: WorkflowPersistService already relays every response with the + // page's current name and description, so an edit made while this save was out is not + // undone here. What is left to apply is the server-owned part, the timestamp above all. + this.workflowActionService.setWorkflowMetadata(updatedWorkflow); }, // A save that fails silently is the worst thing this page can do: the author walks // away believing the form they just built is stored. From e6ca835bf832a9e9bbdf20665db35e083539436c Mon Sep 17 00:00:00 2001 From: Yang Zhang Date: Wed, 23 Sep 2026 17:07:59 -0700 Subject: [PATCH 3/6] fix(workflow): check for edits after the hand-over's drain, not before it Review follow-up on #8540. The Form View hand-over saved, checked whether an edit had landed while that save was out, and only then waited for the save queue to drain. The wait is a second window the page stays editable in, and an edit made in it slipped past the check: the hand-over routed with the edit still unsaved, leaving it to an autosave whose owner the route had just destroyed. The check now sits in the drain's callback, after the wait. whenSavesDrained emits at once when nothing is queued, so a single check covers both windows, and an edit found there saves once more and drains again before routing. The spec adds the case: an edit reported between the switch's save completing and the drain emitting is saved a second time, and the hand-over leaves only after that save's own drain. With the check back before the drain the test fails (persistWorkflow called once, not twice). Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01FVvP3ttj22f9LB4p9u2anY --- .../component/menu/menu.component.spec.ts | 31 +++++++++++++++++++ .../component/menu/menu.component.ts | 14 +++++---- 2 files changed, 39 insertions(+), 6 deletions(-) diff --git a/frontend/src/app/workspace/component/menu/menu.component.spec.ts b/frontend/src/app/workspace/component/menu/menu.component.spec.ts index 627f6b154a0..4717b1f502e 100644 --- a/frontend/src/app/workspace/component/menu/menu.component.spec.ts +++ b/frontend/src/app/workspace/component/menu/menu.component.spec.ts @@ -24,6 +24,7 @@ import { HttpClientTestingModule } from "@angular/common/http/testing"; import { RouterTestingModule } from "@angular/router/testing"; import { NzModalService, NzModalModule, NzModalRef } from "ng-zorro-antd/modal"; import { BehaviorSubject, of, Subject, throwError } from "rxjs"; +import { take } from "rxjs/operators"; import { WorkflowResultExportService } from "../../service/workflow-result-export/workflow-result-export.service"; import { MenuComponent } from "./menu.component"; @@ -261,6 +262,36 @@ describe("MenuComponent", () => { expect(component.isSaving).toBe(false); }); + it("saves once more when an edit lands while the hand-over waits for the queue to drain", () => { + // Waiting for a queued save is a second window the page stays editable in, after the one the + // switch's own save opened; an edit made in it is stored before leaving, like one made in the first. + const edits = new Subject(); + vi.spyOn(component["workflowActionService"], "workflowChanged").mockReturnValue(edits.asObservable()); + component.ngOnInit(); + component.writeAccess = true; + vi.spyOn(component["workflowActionService"], "getWorkflowMetadata").mockReturnValue({ wid: 7 } as any); + vi.spyOn(component["workflowActionService"], "setWorkflowMetadata").mockImplementation(() => {}); + const persistSpy = vi + .spyOn(workflowPersistService, "persistWorkflow") + .mockReturnValue(of({ wid: 7, name: "saved" } as any)); + const drained$ = new Subject(); + // One emission per call, as the service's whenSavesDrained gives. + vi.spyOn(workflowPersistService, "whenSavesDrained").mockImplementation(() => drained$.pipe(take(1))); + const navigate = vi.spyOn(component as any, "openFormViewPage").mockImplementation(() => {}); + + component.onClickOpenFormView(); + expect(persistSpy).toHaveBeenCalledTimes(1); + edits.next(undefined); // an edit while a queued save is still being waited for + drained$.next(); + + expect(persistSpy).toHaveBeenCalledTimes(2); // saved once more instead of leaving + expect(navigate).not.toHaveBeenCalled(); + drained$.next(); // nothing queued behind the second save + expect(navigate).toHaveBeenCalledTimes(1); + 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); diff --git a/frontend/src/app/workspace/component/menu/menu.component.ts b/frontend/src/app/workspace/component/menu/menu.component.ts index 1395de2f429..b852707d3cb 100644 --- a/frontend/src/app/workspace/component/menu/menu.component.ts +++ b/frontend/src/app/workspace/component/menu/menu.component.ts @@ -771,12 +771,6 @@ export class MenuComponent implements OnInit, OnDestroy { this.notificationService.error("Could not save. Your latest changes are not stored yet."); }, complete: () => { - if (this.editedSinceSwitchSnapshot) { - // An edit landed while the save was out; store it here rather than leave it to an - // autosave that would fire under the other view. - this.saveThenOpenFormView(target); - return; - } // A save queued behind ours (a rename's, a description's: those save through the menu // itself, not the autosave) answers to this component: its error is shown here, its // response fed back here, and neither reaches a component the route has destroyed. Leave @@ -786,6 +780,14 @@ export class MenuComponent implements OnInit, OnDestroy { .whenSavesDrained() .pipe(untilDestroyed(this)) .subscribe(() => { + // The page stayed editable while our save was out and while the queue drained. An + // edit landed in either window is stored here rather than left to an autosave that + // would fire under the other view -- checked after the drain, so the two windows + // are one. + if (this.editedSinceSwitchSnapshot) { + this.saveThenOpenFormView(target); + return; + } this.isSaving = false; this.openFormViewPage(target); }); From 0db02c52dd0dd90da09bc2744c7bd846e283213d Mon Sep 17 00:00:00 2001 From: Yang Zhang Date: Wed, 23 Sep 2026 17:28:33 -0700 Subject: [PATCH 4/6] test(workflow): pin whenSavesDrained from the hand-over's side The whenSavesDrained specs all subscribed before any save had answered, and none asked what a caller gets after its answer. The Form View hand-over asks from inside its own save's complete callback, which the queue relays before it counts that save as done; and its subscription outlives a refused navigation. Two cases pin that: asking from the complete callback still waits for a save queued behind and is answered once that one has come back; and a call is answered once, so a drain caused by some later save does not run the hand-over's callback again and route without a click. Dropping the take(1) turned no test red before the second case. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01FVvP3ttj22f9LB4p9u2anY --- .../workflow-persist.service.spec.ts | 38 +++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/frontend/src/app/common/service/workflow-persist/workflow-persist.service.spec.ts b/frontend/src/app/common/service/workflow-persist/workflow-persist.service.spec.ts index 8a42df86384..ddec32f913c 100644 --- a/frontend/src/app/common/service/workflow-persist/workflow-persist.service.spec.ts +++ b/frontend/src/app/common/service/workflow-persist/workflow-persist.service.spec.ts @@ -335,6 +335,44 @@ describe("WorkflowPersistService", () => { expect(emitted).toBe(true); }); + it("answers a caller asking from its own save's complete callback once the save queued behind has too", () => { + // How the Form View hand-over uses it: its save completes, it asks there, and a rename's save + // queued behind must have answered before it is told the queue is drained. + let emitted = false; + service.persistWorkflow(wf("switch")).subscribe({ + complete: () => service.whenSavesDrained().subscribe(() => (emitted = true)), + }); + service.persistWorkflow(wf("rename")).subscribe(); + + httpTestingController + .expectOne(`${API}/${WORKFLOW_PERSIST_URL}`) + .flush({ wid: 9, name: "switch", content: "{}" }); + expect(emitted).toBe(false); // asked, and the rename's save is still out + + httpTestingController + .expectOne(`${API}/${WORKFLOW_PERSIST_URL}`) + .flush({ wid: 9, name: "rename", content: "{}" }); + expect(emitted).toBe(true); + }); + + it("answers a call once: a later drain does not reach a caller answered already", () => { + // The hand-over's subscription outlives a refused navigation; a drain caused by some later + // save must not run its callback again and route without a click. + let emissions = 0; + service.persistWorkflow(wf("first")).subscribe(); + service.whenSavesDrained().subscribe(() => emissions++); + httpTestingController + .expectOne(`${API}/${WORKFLOW_PERSIST_URL}`) + .flush({ wid: 9, name: "first", content: "{}" }); + expect(emissions).toBe(1); + + service.persistWorkflow(wf("second")).subscribe(); + httpTestingController + .expectOne(`${API}/${WORKFLOW_PERSIST_URL}`) + .flush({ wid: 9, name: "second", content: "{}" }); + expect(emissions).toBe(1); + }); + 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; From 4ac6dbe7f6d79855251064432b6358a631ba5749 Mon Sep 17 00:00:00 2001 From: Yang Zhang Date: Fri, 25 Sep 2026 23:36:03 -0700 Subject: [PATCH 5/6] fix(workflow): tell a cleared page from a just-created workflow by the id the save was sent with Review follow-up on #8540. withLocalEdits merged the page's name and description into a response whenever the page held the default id, meaning to catch a workflow the save had just created. A page cleared while a save was out holds the default id too (clearWorkflow puts the default metadata back), and its response came out carrying "Untitled Workflow" and no description. Latent, since every caller is component-scoped and gone by then, but nothing said a caller had to be. The response is now the page's only when the page still holds the id the save went out with: the open workflow's, or the default id of a workflow this very save created. A cleared page holds the default id while its save went out with the real one, so its response is left alone, as is one for a workflow no longer open. The default-id branch goes away. Spec: a save sent with a real id, answered after the page went back to the default, keeps its own name and description; red on the old check. Co-Authored-By: Claude Fable 5.1 --- .../workflow-persist.service.spec.ts | 15 +++++++++ .../workflow-persist.service.ts | 31 +++++++++++-------- 2 files changed, 33 insertions(+), 13 deletions(-) diff --git a/frontend/src/app/common/service/workflow-persist/workflow-persist.service.spec.ts b/frontend/src/app/common/service/workflow-persist/workflow-persist.service.spec.ts index ddec32f913c..7773458b5f1 100644 --- a/frontend/src/app/common/service/workflow-persist/workflow-persist.service.spec.ts +++ b/frontend/src/app/common/service/workflow-persist/workflow-persist.service.spec.ts @@ -307,6 +307,21 @@ describe("WorkflowPersistService", () => { expect(result?.name).toBe("old"); }); + + it("leaves the response alone when the page was cleared while its save was out", () => { + // clearWorkflow puts the default metadata back, so the page holds the default id like a + // just-created workflow does; the save went out with the real id, which tells them apart. + currentMetadata = { wid: 0, name: "Untitled Workflow", 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", description: "d1", content: "{}" }); + + expect(result?.name).toBe("old"); + expect(result?.description).toBe("d1"); + }); }); describe("whenSavesDrained", () => { diff --git a/frontend/src/app/common/service/workflow-persist/workflow-persist.service.ts b/frontend/src/app/common/service/workflow-persist/workflow-persist.service.ts index 7a23c8971de..fa61b112008 100644 --- a/frontend/src/app/common/service/workflow-persist/workflow-persist.service.ts +++ b/frontend/src/app/common/service/workflow-persist/workflow-persist.service.ts @@ -26,10 +26,7 @@ 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 { 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"; @@ -78,7 +75,11 @@ export class WorkflowPersistService { * 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; result: Subject }>(); + private readonly persistQueue = new Subject<{ + send: Observable; + result: Subject; + sentWid: number | undefined; + }>(); /** Saves asked for and not yet answered (or failed); see whenSavesDrained. */ private pendingSaves = 0; @@ -93,9 +94,9 @@ export class WorkflowPersistService { ) { this.persistQueue .pipe( - concatMap(({ send, result }) => + concatMap(({ send, result, sentWid }) => send.pipe( - map(updated => this.withLocalEdits(updated)), + map(updated => this.withLocalEdits(updated, sentWid)), tap({ next: updated => result.next(updated), error: (err: unknown) => result.error(err), @@ -129,13 +130,17 @@ export class WorkflowPersistService { /** * 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. + * save was sent with, and an edit made since would be undone by feeding them back. + * + * Only for a response that is the page's, which is one whose save went out with the id the page + * still holds: the open workflow's, or the default id of a workflow this very save created and + * the page still holds under it. Left alone otherwise: another workflow is open by now, or the + * page was cleared meanwhile (clearWorkflow puts the default id back, but this save went out with + * the real one). Nothing local belongs to those. */ - private withLocalEdits(response: Workflow): Workflow { + private withLocalEdits(response: Workflow, sentWid: number | undefined): Workflow { const current = this.injector.get(WorkflowActionService).getWorkflowMetadata(); - if (current.wid !== response.wid && current.wid !== DEFAULT_WORKFLOW.wid) { + if (current.wid !== sentWid) { return response; } return { ...response, name: current.name, description: current.description }; @@ -173,7 +178,7 @@ export class WorkflowPersistService { // save that was quick, or a synchronous test double) still receives it. const result = new ReplaySubject(1); this.pendingSaves += 1; - this.persistQueue.next({ send, result }); + this.persistQueue.next({ send, result, sentWid: workflow.wid }); return result.asObservable(); } From aff3b835a23854b5bf9f209ec6b7a55bfb1f942e Mon Sep 17 00:00:00 2001 From: Yang Zhang Date: Fri, 25 Sep 2026 23:36:03 -0700 Subject: [PATCH 6/6] fix(workflow): bound the hand-over's wait for the save queue Review follow-up on #8540. Waiting for the queue to drain made the Form View switch depend on requests it did not make: a save queued behind its own that never answers -- neither completes nor fails, which the queue does count -- left pendingSaves above zero for good, and with it the spinner and a dead button; the ways out were a reload or another route. The wait is now bounded at HANDOVER_DRAIN_TIMEOUT_MS (10 s). Past the bound the hand-over leaves as it did before the wait existed, the request going on in the service with nobody to answer to, and an edit landed meanwhile is not saved again, since that save would queue behind the request that never answers. A request that never answers ahead of the switch's own save stalls that save the same way, as it did before this PR: the queue is serial, and a timeout on the persist request itself is the general fix, not taken here. Spec: a drain that never emits, the clock advanced to the bound, the hand-over leaves and persistWorkflow was called once. Co-Authored-By: Claude Fable 5.1 --- .../component/menu/menu.component.spec.ts | 35 ++++++++++++++++++- .../component/menu/menu.component.ts | 26 +++++++++++--- 2 files changed, 55 insertions(+), 6 deletions(-) diff --git a/frontend/src/app/workspace/component/menu/menu.component.spec.ts b/frontend/src/app/workspace/component/menu/menu.component.spec.ts index 4717b1f502e..6b1b61dfb21 100644 --- a/frontend/src/app/workspace/component/menu/menu.component.spec.ts +++ b/frontend/src/app/workspace/component/menu/menu.component.spec.ts @@ -27,7 +27,7 @@ import { BehaviorSubject, of, Subject, throwError } from "rxjs"; import { take } from "rxjs/operators"; import { WorkflowResultExportService } from "../../service/workflow-result-export/workflow-result-export.service"; -import { MenuComponent } from "./menu.component"; +import { HANDOVER_DRAIN_TIMEOUT_MS, MenuComponent } from "./menu.component"; import { WorkflowWebsocketService } from "../../service/workflow-websocket/workflow-websocket.service"; import type { ExecutionDurationUpdateEvent } from "../../types/workflow-websocket.interface"; import { OperatorMetadataService } from "../../service/operator-metadata/operator-metadata.service"; @@ -292,6 +292,39 @@ describe("MenuComponent", () => { expect(component.isSaving).toBe(false); }); + it("leaves after a bound when a save queued behind its own never answers", () => { + // The wait is on a request this component did not make; one that never answers must not hold + // the spinner and the button for good. Past the bound the hand-over leaves as it did before + // the wait existed, and an edit landed meanwhile is not saved again: that save would queue + // behind the request that never answers. + vi.useFakeTimers(); + try { + const edits = new Subject(); + vi.spyOn(component["workflowActionService"], "workflowChanged").mockReturnValue(edits.asObservable()); + component.ngOnInit(); + component.writeAccess = true; + vi.spyOn(component["workflowActionService"], "getWorkflowMetadata").mockReturnValue({ wid: 7 } as any); + vi.spyOn(component["workflowActionService"], "setWorkflowMetadata").mockImplementation(() => {}); + const persistSpy = vi + .spyOn(workflowPersistService, "persistWorkflow") + .mockReturnValue(of({ wid: 7, name: "saved" } as any)); + vi.spyOn(workflowPersistService, "whenSavesDrained").mockReturnValue(new Subject().asObservable()); + const navigate = vi.spyOn(component as any, "openFormViewPage").mockImplementation(() => {}); + + component.onClickOpenFormView(); + edits.next(undefined); // an edit while the queue is waited for + vi.advanceTimersByTime(HANDOVER_DRAIN_TIMEOUT_MS - 1); + expect(navigate).not.toHaveBeenCalled(); + vi.advanceTimersByTime(1); + + expect(navigate).toHaveBeenCalledWith(7); + expect(persistSpy).toHaveBeenCalledTimes(1); // not saved again behind a request that never answers + expect(component.isSaving).toBe(false); + } finally { + vi.useRealTimers(); + } + }); + 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); diff --git a/frontend/src/app/workspace/component/menu/menu.component.ts b/frontend/src/app/workspace/component/menu/menu.component.ts index b852707d3cb..b0b415f1ec6 100644 --- a/frontend/src/app/workspace/component/menu/menu.component.ts +++ b/frontend/src/app/workspace/component/menu/menu.component.ts @@ -32,7 +32,7 @@ import { HeatmapView } from "../../service/heatmap/heatmap-scoring"; import { loadPersistedHeatmapView, savePersistedHeatmapView } from "../../service/heatmap/heatmap-overlay-persistence"; import { WorkflowWebsocketService } from "../../service/workflow-websocket/workflow-websocket.service"; import { WorkflowResultExportService } from "../../service/workflow-result-export/workflow-result-export.service"; -import { catchError, debounceTime, tap } from "rxjs/operators"; +import { catchError, debounceTime, tap, timeout } from "rxjs/operators"; import { UntilDestroy, untilDestroyed } from "@ngneat/until-destroy"; import { WorkflowUtilService } from "../../service/workflow-graph/util/workflow-util.service"; import { WorkflowVersionService } from "../../../dashboard/service/user/workflow-version/workflow-version.service"; @@ -89,6 +89,15 @@ import { JupyterPanelService } from "../../service/jupyter-panel/jupyter-panel.s * @author Henry Chen * */ +/** + * How long the Form View hand-over waits for the save queue to drain after its own save has + * completed (see saveThenOpenFormView). Long enough for a slow save queued behind it to land, + * short enough that a request which never answers does not read as a hang. + */ +export const HANDOVER_DRAIN_TIMEOUT_MS = 10_000; +/** What the bounded wait yields past the bound, in place of the drain. */ +const DRAIN_TIMED_OUT = "drain timed out" as const; + @UntilDestroy() @Component({ selector: "texera-menu", @@ -776,15 +785,22 @@ export class MenuComponent implements OnInit, OnDestroy { // response fed back here, and neither reaches a component the route has destroyed. Leave // once it has answered; a failure of its own is reported by its caller and does not hold // the hand-over. + // + // Bounded, because this is a wait on requests this component did not make: a save queued + // behind ours that never answers -- neither completes nor fails, which the queue does + // count -- would otherwise hold the spinner and the button for good. Past the bound the + // hand-over leaves as it did before this wait existed, the request going on in the + // service with nobody left to answer to. this.workflowPersistService .whenSavesDrained() - .pipe(untilDestroyed(this)) - .subscribe(() => { + .pipe(timeout({ first: HANDOVER_DRAIN_TIMEOUT_MS, with: () => of(DRAIN_TIMED_OUT) }), untilDestroyed(this)) + .subscribe(outcome => { // The page stayed editable while our save was out and while the queue drained. An // edit landed in either window is stored here rather than left to an autosave that // would fire under the other view -- checked after the drain, so the two windows - // are one. - if (this.editedSinceSwitchSnapshot) { + // are one. Not past the bound: a save then would queue behind the request that never + // answers, and never complete. + if (outcome !== DRAIN_TIMED_OUT && this.editedSinceSwitchSnapshot) { this.saveThenOpenFormView(target); return; }