This is an automated email from the ASF dual-hosted git repository. github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-8540-ba2667a2259b6f5d7442979e5111d885ab57a4a5 in repository https://gitbox.apache.org/repos/asf/texera.git
commit e0f49897dbb525dc7faa6eac5f293fdb02b8f7f9 Author: yangzhang75 <[email protected]> AuthorDate: Sun Sep 27 03:50:35 2026 +0000 fix(workflow): keep a newer local edit over a save's response (#8540) ### What changes were proposed in this PR? Closes #8536, the item deferred from #8456 on review. Every save's response is fed back as the workflow's metadata (the canvas autosave in `workspace.component.ts`, the menu's own save for a rename / description / revert, 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: the switch's own response puts the old name back, the hand-over leaves at once, and the canvas's save on its way out (`persistBeforeLeaving`) stores that old name, queued behind the rename's own save. (When this PR was opened the switch was a page load that aborted the rename's save outright; since #8581 made it a route the save is sent, and the rename is lost this way instead.) Handled once, in `WorkflowPersistService`, the one place every save goes through: - Each response is relayed with the page's current name and description in place of the ones the save was sent with. Those are the two fields a user edits; everything else in the response (id, timestamps, publish state, default view) is the server's and arrives as before. A response is left alone when another workflow is open by the time it answers, or when the page was cleared meanwhile; the local name is kept for a workflow the save has just created (the page still holds the default id), which the id the save was sent with tells apart from a cleared page, since that one also holds the default id. The action service is looked up lazily at response time, so the dashboard, which also uses this service (retrieve, create, duplicate), does not construct the graph-owning service as a side effect. The callers that feed a response back as metadata (the workspace autosave, the menu's own save, the Form View switch, the form's save) are unchanged by this part; the settings panel's save and the workspace's unload save never read the response. - `whenSavesDrained()` emits once every save asked for so far has answered or failed (at once when none is pending). The Form View switch waits for it before leaving, so a save queued behind its own (a rename's, a description's, which save through the menu itself and do not go through `workflowChanged`) has answered while the menu that asked for it is still there to show its error or feed back its response, the same reason the switch already waits for its own save. A queued save that fails reports its error through its own caller and does not hold the hand-over. The wait is bounded (`HANDOVER_DRAIN_TIMEOUT_MS`, 10 s): a queued save that never answers does not hold the switch either; past the bound it leaves as it did before the wait existed. Verified in a real browser against today's main and this branch side by side (two dev servers on one backend, every persist request held 1.5 s by a proxy in front of each). Before: a rename made while an earlier rename's save was out flipped the title back to the old name when that response landed (1.6 s) until the second answered (3.2 s); a rename made during the Form View switch's save ended with the old name shown in the Form View and stored (the switch left at 1.5 s, the canvas's save on its way out carried the reverted name). After: the title stays on the new name; the switch leaves once the rename's save has landed (3.1 s) and the Form View shows, and the server stores, the new name; an operator dragged while the hand-over waited for the queue was saved before the route, with its new position in the stored content. ### Any related issues, documentation, discussions? Closes #8536. Follow-up to #8456 (threads on `menu.component.ts:692` and `:231`); part of the Form View feature (parent issue #8011). ### How was this PR tested? Unit tests (vitest): the persist service relays a response with the page's current name and description and the server's other fields, keeps the local name for a just-created workflow, leaves a response alone when another workflow is open or when the page was cleared while the save was out; `whenSavesDrained` emits at once when idle, only after the last of two queued saves has answered, and after a failed save; the menu's switch leaves only once the queue has drained, and an edit made while it waits for the queue is saved before it leaves; the service answers a caller asking from its own save's complete callback only once the save queued behind has answered too, and answers a call once (a drain caused by a later save does not reach a caller answered already); the menu's switch leaves after the bound when a queued save never answers, without saving again behind it. Each new guard was deletion-checked (removing it turns the corresponding test red). eslint, prettier and the production (AOT) build pass; every changed line is statement and function covered. ### Was this PR authored or co-authored using generative AI tooling? Yes. Generated-by: Claude Code (Claude Fable 5.1, Anthropic). Co-authored with Claude, reviewed line by line by the author before submission. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01FVvP3ttj22f9LB4p9u2anY --------- Co-authored-by: Claude Fable 5.1 <[email protected]> --- .../workflow-persist.service.spec.ts | 141 +++++++++++++++++++++ .../workflow-persist/workflow-persist.service.ts | 72 +++++++++-- .../component/menu/menu.component.spec.ts | 86 ++++++++++++- .../app/workspace/component/menu/menu.component.ts | 59 +++++++-- .../workflow-form/workflow-form.component.spec.ts | 26 ---- .../workflow-form/workflow-form.component.ts | 11 +- 6 files changed, 340 insertions(+), 55 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 a35c9ec106..7773458b5f 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,140 @@ 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"); + }); + + 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", () => { + 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("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; + 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 66d267a673..fa61b11200 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,15 @@ */ 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 { 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 +69,83 @@ 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> }>(); + private readonly persistQueue = new Subject<{ + send: Observable<Workflow>; + result: Subject<Workflow>; + sentWid: number | undefined; + }>(); + + /** 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 }) => + concatMap(({ send, result, sentWid }) => send.pipe( + map(updated => this.withLocalEdits(updated, sentWid)), 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<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. + * + * 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, sentWid: number | undefined): Workflow { + const current = this.injector.get(WorkflowActionService).getWorkflowMetadata(); + if (current.wid !== sentWid) { + 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,7 +177,8 @@ 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.persistQueue.next({ send, result }); + this.pendingSaves += 1; + this.persistQueue.next({ send, result, sentWid: workflow.wid }); 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 83ceb5127e..085d61b1ae 100644 --- a/frontend/src/app/workspace/component/menu/menu.component.spec.ts +++ b/frontend/src/app/workspace/component/menu/menu.component.spec.ts @@ -24,9 +24,10 @@ 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"; +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"; @@ -241,6 +242,89 @@ 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<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("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<unknown>(); + 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<void>(); + // 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("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<unknown>(); + 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<void>().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 54f5bde599..f04afd8bfd 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"; @@ -90,6 +90,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", @@ -755,11 +764,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); @@ -787,14 +801,33 @@ 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; - } - 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. + // + // 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(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. 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; + } + this.isSaving = false; + this.openFormViewPage(target); + }); }, }); } 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 ca5694bdef..d276b87494 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<Workflow>(); - 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 4fbd753aad..d783c43b1c 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 @@ -1992,13 +1992,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.
