From 96383625581526e35310499c66a7e43929602015 Mon Sep 17 00:00:00 2001 From: Yang Zhang Date: Thu, 17 Sep 2026 14:11:02 -0700 Subject: [PATCH 01/13] perf(workflow): switch between a workflow's two views without reloading the page The operator canvas and the Form View are two views of one open workflow, but moving between them was a full browser page load. Everything that made the workflow live was thrown away on the way out -- the shared document and the co-editing room it holds, the computing unit connection, the execution state -- and rebuilt on the other side. The reader waited through an Angular bootstrap (blocked on /api/config), a re-parse of the whole bundle, a re-fetch of the operator metadata that a root singleton already had, a workflow fetch, a new co-editing room and a reconnect, only to be told again what the backend had already told the page it just left: that the run is still going. Both directions now route. The view leaving hands the session to the view arriving rather than tearing it down, and the view arriving attaches to what it was given rather than building its own. Two questions, each asked in one place: * `isLeavingWorkspace` -- is the navigation now in flight leaving this workflow, or moving between the two views it is open under? Both views ask it as they are destroyed, and both must answer it the same way, so the rule lives next to the URLs it is about. No navigation in flight is the browser unloading, which is leaving. * `WorkflowActionService.hasWorkflowOpen` -- is the shared document already in this workflow's room? Both views ask it before loading, and skip the fetch, the reset, the new shared model and the graph rebuild when it is. The canvas also lifts the lock the Form View put on the graph, since editing is what a canvas is for, and centres its new paper. Routing in-process was tried before and reverted, and the ghost coeditor of yourself that it left is removed here rather than avoided: the ghost was the second shared document the arriving view built for a workflow the page was already in the room for. Not rebuilding it is what removes it. Leaving the workspace altogether is unchanged: the session is torn down exactly as before, including on unload, where there is no navigation to ask about. One behaviour does change. A hand-over skips the broken-workflow warning that `loadWorkflowWithId` raises, because nothing is loaded; the warning belongs to reading a workflow from the server, and the graph the canvas receives is the one it or the Form View already validated. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FVvP3ttj22f9LB4p9u2anY --- frontend/src/app/app-routing.constant.ts | 41 ++++++++ .../component/menu/menu.component.spec.ts | 16 +++- .../component/menu/menu.component.ts | 15 +-- .../workflow-form.component.spec.ts | 85 ++++++++++++++++- .../workflow-form/workflow-form.component.ts | 94 ++++++++++++------- .../workflow-form.rendered.spec.ts | 6 +- .../workflow-form.spec-harness.ts | 16 +++- .../component/workspace.component.spec.ts | 60 +++++++++++- .../component/workspace.component.ts | 49 +++++++--- .../model/workflow-action.service.spec.ts | 18 ++++ .../model/workflow-action.service.ts | 13 +++ 11 files changed, 346 insertions(+), 67 deletions(-) diff --git a/frontend/src/app/app-routing.constant.ts b/frontend/src/app/app-routing.constant.ts index 072c378f768..224e283a007 100644 --- a/frontend/src/app/app-routing.constant.ts +++ b/frontend/src/app/app-routing.constant.ts @@ -17,6 +17,9 @@ * under the License. */ +// Type-only: this module is imported all over the app and must not pull the router in at runtime. +import type { Router } from "@angular/router"; + export const HOME = "/home"; export const ABOUT = "/about"; export const LOGIN = "/login"; @@ -35,6 +38,44 @@ export const HUB_MODEL_RESULT_DETAIL = `${HUB_MODEL_RESULT}/detail`; export const USER = "/user"; export const USER_WORKSPACE = `${USER}/workflow`; export const USER_WORKFLOW = `${USER}/workflow`; + +/** One workflow is open in the workspace under two views: the operator canvas and the Form View. */ +export const workspaceCanvasUrl = (wid: number): string => `${USER_WORKSPACE}/${wid}`; +export const workspaceFormUrl = (wid: number): string => `${workspaceCanvasUrl(wid)}/form`; + +/** + * Whether `url` is one of the two views workflow `wid` is open under. A falsy `wid` is no + * workflow: `DEFAULT_WORKFLOW` carries 0 until the first save gives it an id. + */ +function isWorkspaceViewOf(url: string, wid: number | undefined): boolean { + if (!wid) { + return false; + } + const path = url.split(/[?#]/)[0]; + return path === workspaceCanvasUrl(wid) || path === workspaceFormUrl(wid); +} + +/** + * Whether the navigation now in flight is leaving workflow `wid`, rather than moving between the + * two views it is open under. + * + * Both views ask this as they are destroyed, and both must answer it the same way: the session + * below them -- the shared document and the co-editing room it holds, the computing unit + * connection, the execution state -- belongs to the workflow, not to either view. Moving between + * the views hands it over; leaving the pair drops it. No navigation in flight means the view is + * being destroyed for some reason other than routing, and so has no successor to hand to, which + * counts as leaving. Unloading the page is not one of those: since #8600 neither view tears down + * on `beforeunload` at all, precisely so a document restored from the back/forward cache still + * has the session it was left with. + * + * `wid` is the workflow the caller is actually holding open, not the one in its route: a workflow + * created by the first autosave has no id in the route it was opened with. + */ +export function isLeavingWorkspace(router: Router, wid: number | undefined): boolean { + const target = router.getCurrentNavigation()?.finalUrl; + return !target || !isWorkspaceViewOf(router.serializeUrl(target), wid); +} + export const USER_DATASET = `${USER}/dataset`; export const USER_DATASET_CREATE = `${USER_DATASET}/create`; export const USER_MODEL = `${USER}/model`; 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 689e19eecd5..36d68606806 100644 --- a/frontend/src/app/workspace/component/menu/menu.component.spec.ts +++ b/frontend/src/app/workspace/component/menu/menu.component.spec.ts @@ -52,7 +52,7 @@ import type { ComputingUnitSelectionComponent } from "../power-button/computing- import { WorkflowContent } from "../../../common/type/workflow"; import { Router } from "@angular/router"; import { ReportGenerationService } from "../../service/report-generation/report-generation.service"; -import { USER_WORKFLOW } from "../../../app-routing.constant"; +import { USER_WORKFLOW, workspaceFormUrl } from "../../../app-routing.constant"; import { GuiConfigService } from "../../../common/service/gui-config.service"; import { MockGuiConfigService } from "../../../common/service/gui-config.service.mock"; import { JupyterPanelService } from "../../service/jupyter-panel/jupyter-panel.service"; @@ -122,11 +122,21 @@ describe("MenuComponent", () => { it("does not open the Form View for a workflow that has not been saved yet", () => { vi.spyOn(component["workflowActionService"], "getWorkflowMetadata").mockReturnValue({ wid: undefined } as any); - const href = window.location.href; + const navigate = vi.spyOn(component as any, "openFormViewPage"); component.onClickOpenFormView(); - expect(window.location.href).toBe(href); + expect(navigate).not.toHaveBeenCalled(); + }); + + // A route, not a page load: the workflow stays open across the switch, so the shared document, + // the computing unit connection and a running execution are handed over rather than rebuilt. + it("routes to the Form View rather than reloading the page", () => { + const navigateByUrl = vi.spyOn(TestBed.inject(Router), "navigateByUrl").mockResolvedValue(true); + + (component as any).openFormViewPage(42); + + expect(navigateByUrl).toHaveBeenCalledWith(workspaceFormUrl(42)); }); it("hands over to the id the save assigned when the canvas held a workflow never saved yet", () => { diff --git a/frontend/src/app/workspace/component/menu/menu.component.ts b/frontend/src/app/workspace/component/menu/menu.component.ts index 865e147ed05..1ccf632bd80 100644 --- a/frontend/src/app/workspace/component/menu/menu.component.ts +++ b/frontend/src/app/workspace/component/menu/menu.component.ts @@ -46,7 +46,7 @@ import { ResultExportationComponent } from "../result-exportation/result-exporta import { ReportGenerationService } from "../../service/report-generation/report-generation.service"; import { ShareAccessComponent } from "src/app/dashboard/component/user/share-access/share-access.component"; import { PanelService } from "../../service/panel/panel.service"; -import { USER_WORKFLOW, USER_WORKSPACE } from "../../../app-routing.constant"; +import { USER_WORKFLOW, workspaceFormUrl } from "../../../app-routing.constant"; import { ComputingUnitStatusService } from "../../../common/service/computing-unit/computing-unit-status/computing-unit-status.service"; import { WarehouseService } from "../../../common/service/warehouse/warehouse.service"; import { ComputingUnitState } from "../../../common/type/computing-unit-connection.interface"; @@ -777,15 +777,16 @@ export class MenuComponent implements OnInit, OnDestroy { } /** - * The full-page handover to the Form View, apart from the save so the order is testable. - * Excluded from coverage as a whole: jsdom cannot navigate, so the specs stub this method and - * assert when it is called rather than what it does. + * The hand-over to the Form View, apart from the save so the order is testable. + * + * A route, not a page load: the two views are views of one open workflow, and reloading threw + * away everything that made the workflow live -- the shared document, the computing unit + * connection, the execution state -- only to rebuild it on the other side. The canvas keeps + * the session on its way out (see its ngOnDestroy) and the Form View attaches to it. */ - /* v8 ignore start */ private openFormViewPage(wid: number): void { - window.location.href = `${USER_WORKSPACE}/${wid}/form`; + void this.router.navigateByUrl(workspaceFormUrl(wid)); } - /* v8 ignore stop */ /** * Calls Markdown Description Component 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 18088fdaf71..452a572ad3a 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 @@ -24,7 +24,7 @@ import { Workflow } from "../../../common/type/workflow"; import { WorkflowFormComponent } from "./workflow-form.component"; import { setupHarness, formViewWorkflow, resolved } from "./workflow-form.spec-harness"; -import { USER_WORKFLOW, USER_WORKSPACE } from "../../../app-routing.constant"; +import { USER_WORKFLOW, workspaceCanvasUrl, workspaceFormUrl } from "../../../app-routing.constant"; import { DefaultView } from "../../../dashboard/type/workflow-metadata.interface"; import { FORM_DEBOUNCE_TIME_MS } from "../../service/execute-workflow/execute-workflow.service"; import { ExecutionState } from "../../types/execute-workflow.interface"; @@ -40,7 +40,7 @@ import { ComputingUnitState } from "../../../common/type/computing-unit-connecti describe("WorkflowFormComponent", () => { let component: WorkflowFormComponent; let h: ReturnType; - let router: { navigate: ReturnType }; + let router: ReturnType["router"]; let workflowActionService: any; let workflowPersistService: any; let formBindingService: any; @@ -124,7 +124,7 @@ describe("WorkflowFormComponent", () => { build(formViewWorkflow).ngOnInit(); - expect(router.navigate).toHaveBeenCalledWith([USER_WORKSPACE, "7"], { replaceUrl: true }); + expect(router.navigateByUrl).toHaveBeenCalledWith(workspaceCanvasUrl(7), { replaceUrl: true }); expect(workflowPersistService.retrieveWorkflow).not.toHaveBeenCalled(); expect(workflowActionService.resetAsNewWorkflow).not.toHaveBeenCalled(); }); @@ -257,10 +257,11 @@ describe("WorkflowFormComponent", () => { expect(h.workflowResultService.clearResults).toHaveBeenCalled(); }); - // The canvas switch is a full-page navigation, and the browser may keep this document in its + // The switch used to be a full-page navigation, and the browser may keep this document in its // back/forward cache. Coming back restores the JavaScript state as it was left and re-runs // nothing, so anything torn down on the way out would stay torn down on a page that still - // looks live (issue #8599). + // looks live (issue #8599). The switch routes now, but leaving the app altogether still + // unloads, and that is what this covers. it("tears nothing down on beforeunload, so a page restored from the cache still works", () => { build(formViewWorkflow).ngOnInit(); @@ -272,6 +273,80 @@ describe("WorkflowFormComponent", () => { expect(h.workflowConsoleService.clearConsoleMessages).not.toHaveBeenCalled(); expect(h.workflowResultService.clearResults).not.toHaveBeenCalled(); }); + + // Handing the workflow to its own operator canvas is not leaving it. The session below the + // two views -- the shared document and its room, the computing unit, the running execution -- + // is the same one, and dropping it here would cost the canvas a reconnect for nothing. + it("keeps the shared services when this workflow's operator canvas takes over", () => { + build(formViewWorkflow).ngOnInit(); + router.getCurrentNavigation.mockReturnValue({ finalUrl: workspaceCanvasUrl(7) }); + + component.ngOnDestroy(); + + expect(workflowActionService.clearWorkflow).not.toHaveBeenCalled(); + expect(h.computingUnitStatusService.disconnect).not.toHaveBeenCalled(); + expect(h.executeWorkflowService.resetExecutionAndWorkers).not.toHaveBeenCalled(); + expect(h.workflowConsoleService.clearConsoleMessages).not.toHaveBeenCalled(); + expect(h.workflowResultService.clearResults).not.toHaveBeenCalled(); + }); + + // Another workflow's canvas is a different workflow: nothing here belongs to it. + it("releases them when the destination is a different workflow", () => { + build(formViewWorkflow).ngOnInit(); + router.getCurrentNavigation.mockReturnValue({ finalUrl: workspaceCanvasUrl(8) }); + + component.ngOnDestroy(); + + expect(workflowActionService.clearWorkflow).toHaveBeenCalled(); + expect(h.computingUnitStatusService.disconnect).toHaveBeenCalled(); + }); + }); + + describe("arriving with the workflow already open", () => { + // The operator canvas hands this workflow over still live: the same graph, already in the + // same co-editing room. Loading it again would destroy the document and rejoin the room, + // which is the whole cost the hand-over exists to avoid. + beforeEach(() => { + workflowActionService.hasWorkflowOpen.mockReturnValue(true); + }); + + it("takes what it needs from the open workflow instead of loading it", () => { + build(formViewWorkflow).ngOnInit(); + + expect(workflowPersistService.retrieveWorkflow).not.toHaveBeenCalled(); + expect(workflowActionService.resetAsNewWorkflow).not.toHaveBeenCalled(); + expect(workflowActionService.setNewSharedModel).not.toHaveBeenCalled(); + expect(workflowActionService.reloadWorkflow).not.toHaveBeenCalled(); + expect(component.workflowName).toBe("scGPT"); + expect(component.loading).toBe(false); + }); + + it("shows it read-only all the same, since editing still belongs to the other view", () => { + build(formViewWorkflow).ngOnInit(); + + expect(workflowActionService.disableWorkflowModification).toHaveBeenCalled(); + }); + }); + + describe("handing over to the operator canvas", () => { + it("routes there rather than reloading the page", () => { + build(formViewWorkflow).ngOnInit(); + + (component as any).openCanvasPage(); + + expect(router.navigateByUrl).toHaveBeenCalledWith(workspaceCanvasUrl(7)); + }); + + // save() runs its callback even for a workflow it declined to save, so this is reachable + // on a page that never got an id, and there is no canvas to go to. + it("goes nowhere when the page never got an id", () => { + build(formViewWorkflow).ngOnInit(); + component.wid = undefined; + + (component as any).openCanvasPage(); + + expect(router.navigateByUrl).not.toHaveBeenCalled(); + }); }); describe("title bar and saving", () => { 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 95b58b0b8d9..87906a6ce63 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 @@ -35,7 +35,7 @@ import { asapScheduler, EMPTY, forkJoin, merge, Observable, Subject, timer } fro import { catchError, concatMap, debounceTime, finalize, observeOn, switchMap, takeUntil, tap } from "rxjs/operators"; import { CdkDragDrop, DragDropModule } from "@angular/cdk/drag-drop"; -import { USER_WORKFLOW, USER_WORKSPACE } from "../../../app-routing.constant"; +import { isLeavingWorkspace, USER_WORKFLOW, workspaceCanvasUrl } from "../../../app-routing.constant"; import { EditableLabelWrapperComponent } from "../../../common/formly/editable-label-wrapper/editable-label-wrapper.component"; import { FormFieldBinding, Workflow, WorkflowContent } from "../../../common/type/workflow"; import { ComputingUnitStatusService } from "../../../common/service/computing-unit/computing-unit-status/computing-unit-status.service"; @@ -566,7 +566,18 @@ export class WorkflowFormComponent implements OnInit, OnDestroy { // without loading anything, so a request that then fails cannot strand the visitor on // an error instead of the page they would have gotten. if (!this.config.env.formViewEnabled) { - void this.router.navigate([USER_WORKSPACE, String(wid)], { replaceUrl: true }); + void this.router.navigateByUrl(workspaceCanvasUrl(wid), { replaceUrl: true }); + return; + } + // Arriving from the operator canvas of this same workflow, which kept its session for us. + // The graph is already here and already in its co-editing room, so there is nothing to fetch + // and nothing to rebuild: take the name and access from what is open and settle in. + if (this.workflowActionService.hasWorkflowOpen(wid)) { + const metadata = this.workflowActionService.getWorkflowMetadata(); + this.workflowName = metadata.name; + this.storedPositions = { ...(this.workflowActionService.getWorkflow().content?.operatorPositions ?? {}) }; + this.canEdit = !metadata.readonly; + this.settleIntoForm(); return; } this.workflowActionService.resetAsNewWorkflow(); @@ -587,18 +598,7 @@ export class WorkflowFormComponent implements OnInit, OnDestroy { this.canEdit = !workflow.readonly; this.workflowActionService.setNewSharedModel(wid, this.userService.getCurrentUser()); this.workflowActionService.reloadWorkflow(workflow); - // The workflow is shown, not edited, from here: dragging operators around or - // deleting them belongs to the operator canvas. Lock now, and keep it locked against - // anything else that unlocks the graph (clampEditability). - this.applyEditability(); - this.clampEditability(); - this.refreshSavedState(); - this.later(() => this.adjustWorkflowNameWidth(), 0); - this.readConfig(); - this.registerMetadataRefresh(); - this.registerAutoPersist(); - this.loading = false; - this.cdr.detectChanges(); + this.settleIntoForm(); }, // The load can fail for many reasons (no access, a network or server error, the // metadata call): a neutral message covers them without claiming it was permissions. @@ -609,6 +609,23 @@ export class WorkflowFormComponent implements OnInit, OnDestroy { }); } + /** What the page does once the workflow is in front of it, whichever way it got there. */ + private settleIntoForm(): void { + // The workflow is shown, not edited, from here: dragging operators around or deleting them + // belongs to the operator canvas. Lock now, and keep it locked against anything else that + // unlocks the graph (clampEditability). The clamp is dropped when this page is destroyed; + // the lock itself is the canvas's to lift when it takes the session back. + this.applyEditability(); + this.clampEditability(); + this.refreshSavedState(); + this.later(() => this.adjustWorkflowNameWidth(), 0); + this.readConfig(); + this.registerMetadataRefresh(); + this.registerAutoPersist(); + this.loading = false; + this.cdr.detectChanges(); + } + /** * Whether this page may hold the graph unlocked: a writer in edit mode, and no run in flight. The * run rule is the canvas's own (a run locks the graph until it ends), kept here too so that entering @@ -1787,31 +1804,32 @@ export class WorkflowFormComponent implements OnInit, OnDestroy { }); } - /** - * Switch to the operator canvas with a full page load, not a route. The two views share - * root-level singletons (the graph, the Yjs shared model, the CU connection); handing - * over in-process left the old state attached -- undraggable operators, a ghost coeditor - * of yourself, broken runs. A fresh document is the reliable handover. - */ + /** Switch to the operator canvas. */ public openRegularCanvas(): void { - // Save first and hand over only once the save has completed: the full-page load unloads this - // document, and a request still in flight at that moment is aborted, so navigating right after - // firing the save could lose the very edit the switch is meant to carry across. A save that - // fails keeps the author here with the error shown, rather than leaving with changes that were - // never stored. A reader, who has nothing to save, goes straight over. + // Save first and hand over only once the save has completed, so an edit made here cannot be + // left behind by the view that replaces this one. A save that fails keeps the author here + // with the error shown, rather than leaving with changes that were never stored. A reader, + // who has nothing to save, goes straight over. this.save(() => this.openCanvasPage()); } /** - * The full-page handover to the operator canvas, apart from the save so the order is testable. - * Excluded from coverage as a whole: jsdom cannot navigate, so the specs stub this method and - * assert when it is called rather than what it does. + * The hand-over to the operator canvas, apart from the save so the order is testable. + * + * A route, not a page load: the two views are views of one open workflow, and reloading threw + * away everything that made the workflow live -- the shared document, the computing unit + * connection, the execution state -- only to rebuild it on the other side. This page keeps the + * session on its way out (see ngOnDestroy) and the canvas attaches to it. */ - /* v8 ignore start */ private openCanvasPage(): void { - window.location.href = `${USER_WORKSPACE}/${this.wid}`; + // Reachable without an id: save() runs its callback even for a workflow it declined to save, + // and this page keeps none when the route carried no usable one (ngOnInit redirects instead). + // There is no canvas to go to then, and "/user/workflow/undefined" is not a place. + if (this.wid === undefined) { + return; + } + void this.router.navigateByUrl(workspaceCanvasUrl(this.wid)); } - /* v8 ignore stop */ /** * Save the same way the operator canvas does. Both views edit one workflow, so the @@ -2004,10 +2022,14 @@ export class WorkflowFormComponent implements OnInit, OnDestroy { // drain (not tied to this component) sends what is left in order and ends by itself. this.save(); this.persistQueue.complete(); - this.workflowActionService.clearWorkflow(); - this.computingUnitStatusService.disconnect(); - this.executeWorkflowService.resetExecutionAndWorkers(); - this.workflowConsoleService.clearConsoleMessages(); - this.workflowResultService.clearResults(); + // Kept when this workflow's own operator canvas is taking over: that is a hand-over, not a + // departure, and rebuilding all of it on the other side is the cost this avoids. + if (isLeavingWorkspace(this.router, this.workflowActionService.getWorkflowMetadata().wid)) { + this.workflowActionService.clearWorkflow(); + this.computingUnitStatusService.disconnect(); + this.executeWorkflowService.resetExecutionAndWorkers(); + this.workflowConsoleService.clearConsoleMessages(); + this.workflowResultService.clearResults(); + } } } diff --git a/frontend/src/app/workspace/component/workflow-form/workflow-form.rendered.spec.ts b/frontend/src/app/workspace/component/workflow-form/workflow-form.rendered.spec.ts index 8dc429dc275..059aa7442d7 100644 --- a/frontend/src/app/workspace/component/workflow-form/workflow-form.rendered.spec.ts +++ b/frontend/src/app/workspace/component/workflow-form/workflow-form.rendered.spec.ts @@ -136,10 +136,14 @@ describe("WorkflowFormComponent (rendered template)", () => { useValue: { coeditors: [{ clientId: "c1", userName: "co", color: "#888" }] }, }, { provide: ActivatedRoute, useValue: { snapshot: { params: { id: "7" } } } }, - { provide: Router, useValue: { navigate } }, + { + provide: Router, + useValue: { navigate, navigateByUrl: vi.fn(), getCurrentNavigation: () => null, serializeUrl: String }, + }, { provide: WorkflowActionService, useValue: { + hasWorkflowOpen: () => false, resetAsNewWorkflow: vi.fn(), setNewSharedModel: vi.fn(), reloadWorkflow: vi.fn(), diff --git a/frontend/src/app/workspace/component/workflow-form/workflow-form.spec-harness.ts b/frontend/src/app/workspace/component/workflow-form/workflow-form.spec-harness.ts index 9ca8be10018..8462cb73b80 100644 --- a/frontend/src/app/workspace/component/workflow-form/workflow-form.spec-harness.ts +++ b/frontend/src/app/workspace/component/workflow-form/workflow-form.spec-harness.ts @@ -51,7 +51,15 @@ export const resolved = (id: string, displayName: string, extra: Partial String(url), + }; const workflowChangedStream = new Subject(); // Announces every form-config write (see formBindingChanged$ and the form-binding mock below). const formBindingChanged = new Subject(); @@ -130,7 +138,11 @@ export function setupHarness() { workflowChanged: () => workflowChangedStream.asObservable(), workflowMetaDataChanged: () => workflowMetaDataChangedStream.asObservable(), getWorkflow: vi.fn().mockReturnValue({ wid: 7, content: { operators: [], operatorPositions: {} } }), - getWorkflowMetadata: () => ({ name: "scGPT", lastModifiedTime: 1767225600000 }), + // Carries the wid, as the real metadata does once a workflow is open: it is what tells the + // page, on the way out, whether the navigation is leaving this workflow or handing it over. + getWorkflowMetadata: () => ({ wid: 7, name: "scGPT", lastModifiedTime: 1767225600000 }), + // Off by default: most specs open a workflow that is not already live, and so load it. + hasWorkflowOpen: vi.fn().mockReturnValue(false), setWorkflowName: vi.fn(), setWorkflowMetadata: vi.fn(), setHighlightingEnabled: vi.fn(), diff --git a/frontend/src/app/workspace/component/workspace.component.spec.ts b/frontend/src/app/workspace/component/workspace.component.spec.ts index c651f37daae..19c33b4a150 100644 --- a/frontend/src/app/workspace/component/workspace.component.spec.ts +++ b/frontend/src/app/workspace/component/workspace.component.spec.ts @@ -48,7 +48,7 @@ import { ComputingUnitStatusService } from "../../common/service/computing-unit/ import { EntityType, HubService } from "../../hub/service/hub.service"; import { commonTestProviders } from "../../common/testing/test-utils"; import { WorkspaceComponent } from "./workspace.component"; -import { USER_WORKSPACE } from "../../app-routing.constant"; +import { USER_WORKSPACE, workspaceFormUrl } from "../../app-routing.constant"; describe("WorkspaceComponent", () => { let component: WorkspaceComponent; @@ -114,6 +114,8 @@ describe("WorkspaceComponent", () => { getTexeraGraph: vi.fn().mockReturnValue(stubGraph), getWorkflow: vi.fn().mockReturnValue(stubWorkflow), getWorkflowMetadata: vi.fn().mockReturnValue({ wid: 42, readonly: false }), + // Off by default: most specs open a workflow that is not already live, and so load it. + hasWorkflowOpen: vi.fn().mockReturnValue(false), workflowChanged: vi.fn().mockReturnValue(EMPTY), workflowMetaDataChanged: vi.fn().mockReturnValue(metadataChangedSubject.asObservable()), }; @@ -143,7 +145,14 @@ describe("WorkspaceComponent", () => { codeEditorService = { vc: undefined }; messageService = { error: vi.fn() }; - routerMock = { navigate: vi.fn() }; + // `getCurrentNavigation` answers what the page is being destroyed for: null stands for no + // navigation in flight, so nothing to hand the session to. `serializeUrl` is the real + // router's, turning a UrlTree back into a path; here the tests hand in the path itself. + routerMock = { + navigate: vi.fn(), + getCurrentNavigation: vi.fn().mockReturnValue(null), + serializeUrl: (url: unknown) => String(url), + }; locationMock = { go: vi.fn() }; connectionResetSubject = new Subject(); computingUnitStatusService = { @@ -235,6 +244,25 @@ describe("WorkspaceComponent", () => { expect(component.isLoading).toBe(true); expect(workflowActionService.disableWorkflowModification).toHaveBeenCalled(); }); + + // The Form View hands this workflow over still live: the same graph, already in the same + // co-editing room. Clearing it and fetching it again would undo exactly what was handed over. + // Only the lock the Form View put on the graph is lifted, since editing is what a canvas is for. + it("attaches to a workflow the Form View handed over, instead of loading it again", async () => { + await createFixture(configureRoute({ id: "42" })); + workflowActionService.hasWorkflowOpen.mockReturnValue(true); + + component.ngOnInit(); + component.ngAfterViewInit(); + + expect(workflowActionService.resetAsNewWorkflow).not.toHaveBeenCalled(); + expect(workflowPersistService.retrieveWorkflow).not.toHaveBeenCalled(); + expect(workflowActionService.setNewSharedModel).not.toHaveBeenCalled(); + expect(workflowActionService.reloadWorkflow).not.toHaveBeenCalled(); + expect(workflowActionService.enableWorkflowModification).toHaveBeenCalled(); + expect(component.isLoading).toBe(false); + expect(stubGraph.triggerCenterEvent).toHaveBeenCalled(); + }); }); describe("loadWorkflowWithId", () => { @@ -518,6 +546,23 @@ describe("WorkspaceComponent", () => { expect(workflowResultService.clearResults).not.toHaveBeenCalled(); }); + // Handing the workflow to its own Form View is not leaving it. The session below the two + // views -- the shared document and its room, the computing unit, the running execution -- + // is the same one, and dropping it here would cost the Form View a reconnect for nothing. + it("keeps the session when this workflow's Form View takes over", async () => { + await createFixture(); + fixture.detectChanges(); + routerMock.getCurrentNavigation.mockReturnValue({ finalUrl: workspaceFormUrl(42) }); + + component.ngOnDestroy(); + + expect(workflowActionService.clearWorkflow).not.toHaveBeenCalled(); + expect(computingUnitStatusService.disconnect).not.toHaveBeenCalled(); + expect(executeWorkflowService.resetExecutionAndWorkers).not.toHaveBeenCalled(); + expect(workflowConsoleService.clearConsoleMessages).not.toHaveBeenCalled(); + expect(workflowResultService.clearResults).not.toHaveBeenCalled(); + }); + it("skips even the save on beforeunload when the user is not signed in", async () => { await createFixture(); fixture.detectChanges(); @@ -528,6 +573,17 @@ describe("WorkspaceComponent", () => { expect(workflowPersistService.persistWorkflow).not.toHaveBeenCalled(); }); + it("tears it down when the destination is another workflow's Form View", async () => { + await createFixture(); + fixture.detectChanges(); + routerMock.getCurrentNavigation.mockReturnValue({ finalUrl: workspaceFormUrl(43) }); + + component.ngOnDestroy(); + + expect(workflowActionService.clearWorkflow).toHaveBeenCalled(); + expect(computingUnitStatusService.disconnect).toHaveBeenCalled(); + }); + it("clears the workflow session state when the computing unit is switched in-canvas (issue #3120)", async () => { await createFixture(); fixture.detectChanges(); diff --git a/frontend/src/app/workspace/component/workspace.component.ts b/frontend/src/app/workspace/component/workspace.component.ts index 4a26c2bc039..c660fa2d846 100644 --- a/frontend/src/app/workspace/component/workspace.component.ts +++ b/frontend/src/app/workspace/component/workspace.component.ts @@ -49,7 +49,7 @@ import { WorkflowMetadata } from "src/app/dashboard/type/workflow-metadata.inter import { EntityType, HubService } from "../../hub/service/hub.service"; import { THROTTLE_TIME_MS } from "../../hub/component/workflow/detail/hub-workflow-detail.component"; import { WorkflowCompilingService } from "../service/compile-workflow/workflow-compiling.service"; -import { USER_WORKSPACE } from "../../app-routing.constant"; +import { isLeavingWorkspace, USER_WORKSPACE } from "../../app-routing.constant"; import { GuiConfigService } from "../../common/service/gui-config.service"; import { ComputingUnitStatusService } from "../../common/service/computing-unit/computing-unit-status/computing-unit-status.service"; import { ExecuteWorkflowService } from "../service/execute-workflow/execute-workflow.service"; @@ -109,6 +109,13 @@ export class WorkspaceComponent implements AfterViewInit, OnInit, OnDestroy { */ private autoPersistRegistered = false; + /** + * Whether this page opened onto a workflow that was already live, handed over by its Form View + * rather than loaded from scratch. Decided once, in ngAfterViewInit, before anything can change + * what it is read from. + */ + private resumedSession = false; + constructor( private userService: UserService, // list additional 3 services in constructor so they are initialized even if no one use them directly @@ -161,13 +168,19 @@ export class WorkspaceComponent implements AfterViewInit, OnInit, OnDestroy { * WorkflowActionService is the single source of the workflow representation. WorkflowPersistService reflects * changes from WorkflowActionService. */ - // clear the current workspace, reset as `WorkflowActionService.DEFAULT_WORKFLOW` - this.workflowActionService.resetAsNewWorkflow(); - // if a workflow id is present in the route, display loading spinner immediately while loading const widInRoute = this.route.snapshot.params.id; - if (widInRoute) { - this.isLoading = true; - this.workflowActionService.disableWorkflowModification(); + // Arriving from the Form View of this same workflow, which kept its session for us (see its + // ngOnDestroy). The graph is already here and already in its co-editing room, so clearing it + // and fetching it again would undo exactly what was kept. + this.resumedSession = widInRoute !== undefined && this.workflowActionService.hasWorkflowOpen(Number(widInRoute)); + if (!this.resumedSession) { + // clear the current workspace, reset as `WorkflowActionService.DEFAULT_WORKFLOW` + this.workflowActionService.resetAsNewWorkflow(); + // if a workflow id is present in the route, display loading spinner immediately while loading + if (widInRoute) { + this.isLoading = true; + this.workflowActionService.disableWorkflowModification(); + } } this.onWIDChange(); this.updateViewCount(); @@ -197,11 +210,15 @@ export class WorkspaceComponent implements AfterViewInit, OnInit, OnDestroy { ngOnDestroy() { this.persistBeforeLeaving(); this.codeEditorViewRef.clear(); - this.workflowActionService.clearWorkflow(); // Tear down the connection and all websocket-derived session state so a - // re-entered workflow starts clean instead of reusing the previous one. - this.computingUnitStatusService.disconnect(); - this.resetWorkflowSessionState(); + // re-entered workflow starts clean instead of reusing the previous one -- unless the Form + // View of this same workflow is taking over, in which case the session is handed to it + // rather than rebuilt, which is what used to make the switch take seconds. + if (isLeavingWorkspace(this.router, this.workflowActionService.getWorkflowMetadata().wid)) { + this.workflowActionService.clearWorkflow(); + this.computingUnitStatusService.disconnect(); + this.resetWorkflowSessionState(); + } } private persistBeforeLeaving(): void { @@ -329,6 +346,16 @@ export class WorkspaceComponent implements AfterViewInit, OnInit, OnDestroy { registerLoadOperatorMetadata() { const wid = this.route.snapshot.params.id; + // The Form View handed this workflow over still open: the graph, its co-editing room and the + // computing unit connection are the ones it was using. Nothing to fetch, nothing to rebuild. + // Only the lock it put on the graph is undone, since editing is what the canvas is for, and + // the view is centred because this canvas's paper is a new one, at its default offset. + if (this.resumedSession) { + this.workflowActionService.enableWorkflowModification(); + this.registerAutoPersistWorkflow(); + this.triggerCenter(); + return; + } // load workflow with wid if presented in the URL if (wid) { // show loading spinner right away while waiting for workflow to load diff --git a/frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.spec.ts b/frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.spec.ts index 87e002a86a9..e78f49fde8a 100644 --- a/frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.spec.ts +++ b/frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.spec.ts @@ -74,6 +74,24 @@ describe("WorkflowActionService", () => { expect(injectedService).toBeTruthy(); })); + // The operator canvas and the Form View hand one open workflow between them; the arriving view + // asks this before loading, because seeding a second document for the same workflow would leave + // the co-editing room and rejoin it. + describe("hasWorkflowOpen", () => { + it("is true only for the workflow whose room the shared document is in", () => { + service.setNewSharedModel(42); + + expect(service.hasWorkflowOpen(42)).toBe(true); + expect(service.hasWorkflowOpen(43)).toBe(false); + }); + + it("is false for every workflow while none is open", () => { + service.setNewSharedModel(); + + expect(service.hasWorkflowOpen(42)).toBe(false); + }); + }); + it("should add an operator to both jointjs and texera graph correctly", () => { service.addOperator(mockScanPredicate, mockPoint); diff --git a/frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.ts b/frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.ts index d13e6cf10a0..c5d64cfef99 100644 --- a/frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.ts +++ b/frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.ts @@ -629,6 +629,19 @@ export class WorkflowActionService { this.texeraGraph.destroyYModel(); } + /** + * Whether this page already holds `workflowId` open: the shared document is in that workflow's + * co-editing room, so the graph, the undo history and the room membership are the live ones. + * + * The operator canvas and the Form View are two views of one open workflow and hand the session + * over between them rather than each building its own. The arriving view asks this before + * loading: seeding a second document for the same workflow would leave the room and rejoin it, + * which is what used to leave a ghost of yourself in the co-editor list. + */ + public hasWorkflowOpen(workflowId: number): boolean { + return this.texeraGraph.sharedModel.wid === workflowId; + } + /** * Reload the given workflow, update workflowMetadata and workflowContent. * This method is based on the assumption that this is on a new SharedModel. From 709de5a12c36389fdc9f3b9eb2281a7fd95357b0 Mon Sep 17 00:00:00 2001 From: Yang Zhang Date: Fri, 18 Sep 2026 18:38:31 -0700 Subject: [PATCH 02/13] fix(workflow): announce the metadata to a view handed an open workflow Self-review of the hand-over. `workflowMetaDataChanged()` is a plain Subject and carries no current value, so a subscriber that arrives after the metadata was set hears nothing until the next change. The hand-over creates a whole new view and never sets the metadata, because what is already open is already right -- so every component that view mounts sat at its initial value until some later edit happened to save: - the menu showed no workflow name, and writing that empty box back through `setWorkflowName` would have renamed the workflow to ""; - the menu had no workflow id, so the share dialog was opened on `undefined` -- the symptom of #8599, by a third route; - the workspace believed the user could not write, which sends the switch back to the Form View down the reader path, without saving; - the computing unit picker never restored the unit the workflow last ran on, in both directions: the Form View mounts one too. `setWorkflowMetadata` cannot do this, as it returns early for the value it already holds, so the service says it again explicitly. Also states, and pins with a test, why `hasWorkflowOpen` is false for a workflow that has been left: `destroyYModel` keeps the object and its wid, and it is `clearWorkflow` going on to `reloadWorkflow(undefined)` that seeds a fresh model with none. Were that to stop happening, a canvas re-entered from the dashboard would attach to a destroyed document instead of loading. Deletion-checked: dropping either call, emptying the service method, or stopping `reloadWorkflow(undefined)` re-seeding the model each turns exactly one named test red. Full frontend suite: 223 files, 6072 passed, 1 skipped, 0 failed. AOT build, eslint and prettier clean. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FVvP3ttj22f9LB4p9u2anY --- .../workflow-form.component.spec.ts | 13 +++++++ .../workflow-form/workflow-form.component.ts | 6 ++++ .../workflow-form.spec-harness.ts | 2 ++ .../component/workspace.component.spec.ts | 17 +++++++++ .../component/workspace.component.ts | 6 ++++ .../model/workflow-action.service.spec.ts | 35 +++++++++++++++++++ .../model/workflow-action.service.ts | 23 ++++++++++++ 7 files changed, 102 insertions(+) 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 452a572ad3a..b713214346b 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 @@ -326,6 +326,19 @@ describe("WorkflowFormComponent", () => { expect(workflowActionService.disableWorkflowModification).toHaveBeenCalled(); }); + + // The fields this page reads off the open workflow are its own. Everything else it mounts + // that shows the workflow -- the computing unit picker under the form, which restores the + // unit this workflow last ran on -- learns it from a stream that does not replay, and this + // page never sets the metadata, because what is already open is already right. + it("re-announces the metadata for the subscribers it has only just mounted", () => { + const seen: unknown[] = []; + h.workflowMetaDataChangedStream.subscribe(m => seen.push(m)); + + build(formViewWorkflow).ngOnInit(); + + expect(seen).toHaveLength(1); + }); }); describe("handing over to the operator canvas", () => { 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 87906a6ce63..1c84635ce6d 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 @@ -578,6 +578,12 @@ export class WorkflowFormComponent implements OnInit, OnDestroy { this.storedPositions = { ...(this.workflowActionService.getWorkflow().content?.operatorPositions ?? {}) }; this.canEdit = !metadata.readonly; this.settleIntoForm(); + // This page and everything on it is new, and the metadata it needs was set by the view that + // was here before: the stream that carries it does not replay, so say it again now that this + // page's own subscribers are listening -- after settling in, which is what mounts them. + // The fields read above are this component's own; the computing unit picker below the form + // has no such shortcut and would not restore the unit this workflow last ran on. + this.workflowActionService.republishWorkflowMetadata(); return; } this.workflowActionService.resetAsNewWorkflow(); diff --git a/frontend/src/app/workspace/component/workflow-form/workflow-form.spec-harness.ts b/frontend/src/app/workspace/component/workflow-form/workflow-form.spec-harness.ts index 8462cb73b80..c7abeebf8a0 100644 --- a/frontend/src/app/workspace/component/workflow-form/workflow-form.spec-harness.ts +++ b/frontend/src/app/workspace/component/workflow-form/workflow-form.spec-harness.ts @@ -137,6 +137,8 @@ export function setupHarness() { clearWorkflow: vi.fn(), workflowChanged: () => workflowChangedStream.asObservable(), workflowMetaDataChanged: () => workflowMetaDataChangedStream.asObservable(), + // As the real one does: the metadata it already holds, re-announced on the same stream. + republishWorkflowMetadata: vi.fn(() => workflowMetaDataChangedStream.next(undefined)), getWorkflow: vi.fn().mockReturnValue({ wid: 7, content: { operators: [], operatorPositions: {} } }), // Carries the wid, as the real metadata does once a workflow is open: it is what tells the // page, on the way out, whether the navigation is leaving this workflow or handing it over. diff --git a/frontend/src/app/workspace/component/workspace.component.spec.ts b/frontend/src/app/workspace/component/workspace.component.spec.ts index 19c33b4a150..77724910465 100644 --- a/frontend/src/app/workspace/component/workspace.component.spec.ts +++ b/frontend/src/app/workspace/component/workspace.component.spec.ts @@ -118,6 +118,8 @@ describe("WorkspaceComponent", () => { hasWorkflowOpen: vi.fn().mockReturnValue(false), workflowChanged: vi.fn().mockReturnValue(EMPTY), workflowMetaDataChanged: vi.fn().mockReturnValue(metadataChangedSubject.asObservable()), + // As the real one does: the metadata it already holds, re-announced on the same stream. + republishWorkflowMetadata: vi.fn(() => metadataChangedSubject.next()), }; workflowPersistService = { @@ -263,6 +265,21 @@ describe("WorkspaceComponent", () => { expect(component.isLoading).toBe(false); expect(stubGraph.triggerCenterEvent).toHaveBeenCalled(); }); + + // This page is new and so is everything on it, but the metadata was set by the view that was + // here before, and the stream carrying it does not replay. Everything that shows the workflow + // -- the menu's name and id, the computing unit picker, this page's own write access -- would + // otherwise sit at its initial value until some later edit happened to save. + it("re-announces the metadata for the subscribers this page has only just mounted", async () => { + await createFixture(configureRoute({ id: "42" })); + workflowActionService.hasWorkflowOpen.mockReturnValue(true); + expect(component.writeAccess).toBe(false); + + component.ngOnInit(); + component.ngAfterViewInit(); + + expect(component.writeAccess).toBe(true); + }); }); describe("loadWorkflowWithId", () => { diff --git a/frontend/src/app/workspace/component/workspace.component.ts b/frontend/src/app/workspace/component/workspace.component.ts index c660fa2d846..e5ee2d5637c 100644 --- a/frontend/src/app/workspace/component/workspace.component.ts +++ b/frontend/src/app/workspace/component/workspace.component.ts @@ -354,6 +354,12 @@ export class WorkspaceComponent implements AfterViewInit, OnInit, OnDestroy { this.workflowActionService.enableWorkflowModification(); this.registerAutoPersistWorkflow(); this.triggerCenter(); + // This page and everything on it is new, and the metadata it needs was set by the view that + // was here before: the stream that carries it does not replay, so say it again now that + // this page's own subscribers are listening. Without it the menu shows no workflow name + // and no id, the computing unit picker does not restore the unit this workflow last ran + // on, and this page believes the user cannot write to the workflow. + this.workflowActionService.republishWorkflowMetadata(); return; } // load workflow with wid if presented in the URL diff --git a/frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.spec.ts b/frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.spec.ts index e78f49fde8a..97768e0f261 100644 --- a/frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.spec.ts +++ b/frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.spec.ts @@ -42,6 +42,7 @@ import { LogicalPort, OperatorPredicate } from "../../../types/workflow-common.i import { WorkflowUtilService } from "../util/workflow-util.service"; import { commonTestProviders } from "../../../../common/testing/test-utils"; import { ExecutionMode, Workflow, WorkflowSettings } from "../../../../common/type/workflow"; +import { WorkflowMetadata } from "../../../../dashboard/type/workflow-metadata.interface"; describe("WorkflowActionService", () => { let service: WorkflowActionService; @@ -90,6 +91,40 @@ describe("WorkflowActionService", () => { expect(service.hasWorkflowOpen(42)).toBe(false); }); + + // Not local to this method: destroying the document keeps the object and its wid, and what + // clears it is clearWorkflow going on to reloadWorkflow(undefined), which seeds a fresh model + // with none. Were that to stop happening, a canvas re-entered from the dashboard would attach + // to a destroyed document instead of loading the workflow. + it("is false for a workflow that has been left, not only for one never opened", () => { + service.setNewSharedModel(42); + expect(service.hasWorkflowOpen(42)).toBe(true); + + service.clearWorkflow(); + + expect(service.hasWorkflowOpen(42)).toBe(false); + }); + }); + + // The stream is a plain Subject and carries no current value, so a view that attaches to an + // already-open workflow -- and therefore never sets the metadata, because it is already right -- + // has to say it again for the subscribers it has just mounted: the menu's name and id, the + // computing unit picker, the workspace's write access. + describe("republishWorkflowMetadata", () => { + it("re-announces the metadata it already holds, which setWorkflowMetadata will not", () => { + const metadata: WorkflowMetadata = { ...DEFAULT_WORKFLOW, wid: 42, name: "kept open" }; + service.setWorkflowMetadata(metadata); + const seen: WorkflowMetadata[] = []; + service.workflowMetaDataChanged().subscribe(m => seen.push(m)); + + // The same value a view would read back and hand straight to the setter: it returns early. + service.setWorkflowMetadata(service.getWorkflowMetadata()); + expect(seen).toEqual([]); + + service.republishWorkflowMetadata(); + + expect(seen).toEqual([metadata]); + }); }); it("should add an operator to both jointjs and texera graph correctly", () => { diff --git a/frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.ts b/frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.ts index c5d64cfef99..8de265227a2 100644 --- a/frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.ts +++ b/frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.ts @@ -637,11 +637,34 @@ export class WorkflowActionService { * over between them rather than each building its own. The arriving view asks this before * loading: seeding a second document for the same workflow would leave the room and rejoin it, * which is what used to leave a ghost of yourself in the co-editor list. + * + * A workflow that was open and has since been left does not answer true here, and the reason is + * worth stating because it is not local: `destroyYModel` destroys the document but keeps the + * object, `wid` and all. What clears it is `clearWorkflow` going on to `reloadWorkflow(undefined)`, + * which seeds a fresh model with no `wid`. Were that to stop happening, a canvas re-entered from + * the dashboard would attach to a destroyed document instead of loading. Pinned by a test. */ public hasWorkflowOpen(workflowId: number): boolean { return this.texeraGraph.sharedModel.wid === workflowId; } + /** + * Announce the metadata already in hand, unchanged, for a view that arrived on a workflow that + * was already open. + * + * `workflowMetaDataChanged()` is a plain Subject, so it carries no current value: a subscriber + * that arrives after the metadata was set hears nothing until the next change. Everything a + * view puts on screen about the workflow -- its name and id in the menu, the computing unit it + * last ran on, whether this user may write to it -- is learnt only from that stream, and a view + * handed an open workflow never sets the metadata, because it is already right. Without this + * they would each sit at their initial value until the next edit happened to save. + * + * `setWorkflowMetadata` cannot do the job: it returns early for the value it already holds. + */ + public republishWorkflowMetadata(): void { + this.workflowMetadataChangeSubject.next(this.workflowMetadata); + } + /** * Reload the given workflow, update workflowMetadata and workflowContent. * This method is based on the assumption that this is on a new SharedModel. From f2341cb32e2d402ac6e78d35ff9182638fde70cf Mon Sep 17 00:00:00 2001 From: Yang Zhang Date: Fri, 18 Sep 2026 19:59:02 -0700 Subject: [PATCH 03/13] fix(frontend): give the workflow editor its own container, not the document's first Found in the browser: switching to the Form View and back left the canvas blank -- nothing to pan, nothing to click -- while the graph itself was fine, as the Form View's preview went on showing it running. `WorkflowEditorComponent` found its container with `document.getElementById("workflow-editor")`, and both views render that same hardcoded id: the canvas's editor and the Form View's preview are the same component. While the switch was a full page load the two could never coexist, so the lookup was always right. Routing between the views overlaps them for a tick -- the arriving view runs `ngAfterViewInit` while the departing one is still in the DOM -- and a document-wide lookup then returns the departing view's container, first in document order. The paper was built into a div about to be removed, and the arriving canvas kept an empty one. Measured in a real browser, at the moment the canvas comes back: [GETBYID] workflow-editor: 2 in document, returned index 0 before: #workflow-editor 1399x1000, svg=false, cells=0 after: #workflow-editor 1399x1000, svg=true, cells=1 It resolves both elements from its own host now. No run is needed to reproduce; expanding the Form View's preview once is enough, which is why watching a run there made it certain. Deletion-checked: restoring the document-wide lookup turns exactly the new named test red. Full frontend suite: 224 files, 6150 passed, 1 skipped, 0 failed. AOT build, eslint and prettier clean. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FVvP3ttj22f9LB4p9u2anY --- .../workflow-editor.component.spec.ts | 24 +++++++++++++++++++ .../workflow-editor.component.ts | 10 ++++++-- 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.spec.ts b/frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.spec.ts index d397704d101..49d8f790f23 100644 --- a/frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.spec.ts +++ b/frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.spec.ts @@ -136,6 +136,30 @@ describe("WorkflowEditorComponent", () => { expect(wrapper.getHeatmapView()).toBeNull(); }); + // Two of these editors are in the page at once for one tick when the two views of a workflow + // hand over: the arriving one initialises while the departing one is still being removed, and + // both templates carry id="workflow-editor". Searching the document found the departing view's + // container, so the paper was built into a div about to disappear and the arriving canvas came + // up blank -- nothing to pan, nothing to click, while the graph itself was untouched. + it("builds its paper in its own container, not whichever the document holds first", () => { + const decoy = document.createElement("div"); + decoy.id = "workflow-editor"; + // Earlier in document order than the fixture, as the departing view's container is. + document.body.insertBefore(decoy, document.body.firstChild); + try { + const other = TestBed.createComponent(WorkflowEditorComponent); + other.detectChanges(); + + const host = other.nativeElement as HTMLElement; + expect(host.contains((other.componentInstance as any).editor)).toBe(true); + expect((other.componentInstance as any).editor).not.toBe(decoy); + expect(decoy.querySelector("svg")).toBeNull(); + other.destroy(); + } finally { + decoy.remove(); + } + }); + it("should hide operator status on the canvas by default", () => { // keeps the Status toggle off until the user enables it const editor = (component as any).editor as HTMLElement; diff --git a/frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.ts b/frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.ts index 92264762563..d087aeb5820 100644 --- a/frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.ts +++ b/frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.ts @@ -205,8 +205,14 @@ export class WorkflowEditorComponent implements OnInit, AfterViewInit, OnDestroy } ngAfterViewInit() { - this.editor = document.getElementById("workflow-editor")!; - this.editorWrapper = document.getElementById("workflow-editor-wrapper")!; + // This component's own elements, not whichever the document happens to hold first. Two of + // these editors are briefly in the page at once when the two views of a workflow hand over: + // the arriving one initialises while the departing one is still being removed. Searching the + // document returned the departing view's container, so the paper was built into a div about + // to disappear and the arriving canvas stayed blank, with nothing to pan and nothing to click. + const host = this.elementRef.nativeElement as HTMLElement; + this.editor = host.querySelector("#workflow-editor")!; + this.editorWrapper = host.querySelector("#workflow-editor-wrapper")!; document.addEventListener("keydown", this._handleKeyboardAction.bind(this)); this.initializeJointPaper(); this.handleDisableJointPaperInteractiveness(); From f407a331c94aa5cd329ec05c7c1c6f7cd8f2b82c Mon Sep 17 00:00:00 2001 From: Yang Zhang Date: Sat, 19 Sep 2026 00:20:49 -0700 Subject: [PATCH 04/13] fix(frontend): keep a handed-over run visible and locked, and let each view clean up after itself Addresses the Copilot review on #8581. All four are defects older than this PR that routing between a workflow's two views turns into real problems, because a mount now happens on every switch rather than once per page load. Two of them are the half of a root cause this PR already fixed for the workflow metadata, missed for the execution state. `getExecutionStateStream` is a plain Subject and carries no current value, so a view handed a live session subscribes after the last state change and hears nothing: - the Form View showed **Run** for a workflow that was running, and its lock rule, which reads `isRunning`, would have let edit mode unlock a graph mid-run. It reads `getExecutionState()` before settling in now. - the canvas unlocked the graph outright on arrival, and the execute service reapplies its state-to-lock rule only when the state changes, so a workflow that was still running stayed editable until its run happened to end. It asks the service to reapply that rule instead, through a new `republishExecutionState`, which also says the state again for the subscribers the new page has just mounted. The rule stays in the one place that owns it. The other two are teardown: - neither `WorkflowEditorComponent` nor `MiniMapComponent` disposed its JointJS paper, so each mount left one listening to the root-provided graph from a detached node (#8582). Both call `paper.remove()` now. - the editor added its keydown listener with one `.bind(this)` and tried to remove another, which never matched: one stale listener per mount, and a single Ctrl/Cmd-Z handled several times over after a few switches. One bound reference is stored and used for both. The mini-map also found its own container, and the main canvas's, by document-wide id lookup, which has the same problem the editor's did: both views mount a mini-map. It uses its own host and the main paper's own `el` now. Deletion-checked: removing either `paper.remove()`, restoring the mismatched `bind`, restoring the unconditional unlock, dropping the form's state read, or emptying `republishExecutionState` each turns exactly the intended named tests red. Full frontend suite: 224 files, 6157 passed, 1 skipped, 0 failed. AOT build, eslint and prettier clean. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FVvP3ttj22f9LB4p9u2anY --- .../mini-map/mini-map.component.spec.ts | 30 ++++++++++++-- .../mini-map/mini-map.component.ts | 27 +++++++++--- .../workflow-editor.component.spec.ts | 23 +++++++++++ .../workflow-editor.component.ts | 16 +++++++- .../workflow-form.component.spec.ts | 11 +++++ .../workflow-form/workflow-form.component.ts | 6 +++ .../workflow-form.rendered.spec.ts | 3 ++ .../workflow-form.spec-harness.ts | 8 ++++ .../component/workspace.component.spec.ts | 23 ++++++++++- .../component/workspace.component.ts | 11 +++-- .../execute-workflow.service.spec.ts | 41 ++++++++++++++++++- .../execute-workflow.service.ts | 16 ++++++++ 12 files changed, 198 insertions(+), 17 deletions(-) diff --git a/frontend/src/app/workspace/component/workflow-editor/mini-map/mini-map.component.spec.ts b/frontend/src/app/workspace/component/workflow-editor/mini-map/mini-map.component.spec.ts index 0a5c31c694b..e71f8dc4815 100644 --- a/frontend/src/app/workspace/component/workflow-editor/mini-map/mini-map.component.spec.ts +++ b/frontend/src/app/workspace/component/workflow-editor/mini-map/mini-map.component.spec.ts @@ -46,6 +46,12 @@ import { commonTestProviders } from "../../../../common/testing/test-utils"; * instead, and records the calls the component makes against it. */ class StubPaper { + /** + * The container the paper was built into. The mini-map measures the main canvas's viewport + * through this, rather than looking `#workflow-editor` up in the document: both views of a + * workflow mount an editor, and they overlap for a tick when the switch routes between them. + */ + public el: HTMLElement | undefined; public readonly handlers: Record void> = {}; public readonly pageToLocalPointArgs: { x: number; y: number }[] = []; public readonly translateArgs: [number, number][] = []; @@ -162,8 +168,9 @@ describe("MiniMapComponent", () => { } /** - * The mini-map reads the main editor's element out of the document by id, so - * mount a stand-in with an explicit size and viewport rect. + * A stand-in for the main canvas's container, with an explicit size and viewport rect. The + * mini-map reaches it through the main paper's own `el` (see StubPaper), so tests that need it + * measured also hand it to the paper they attach. */ function mountWorkflowEditorStub(width: number, height: number, left: number, top: number): HTMLDivElement { const editor = document.createElement("div"); @@ -176,8 +183,12 @@ describe("MiniMapComponent", () => { return editor; } - /** Publishes `paper` on the stream the mini-map subscribes to in ngAfterViewInit. */ + /** + * Publishes `paper` on the stream the mini-map subscribes to in ngAfterViewInit, standing it in + * the editor container the test mounted, as a real main paper is built into one. + */ function attachMainPaper(paper: StubPaper): void { + paper.el = paper.el ?? editorStub; mainPaper$.next(paper as unknown as joint.dia.Paper); } @@ -187,6 +198,19 @@ describe("MiniMapComponent", () => { }); describe("mini-map paper", () => { + // Bound to the root-provided joint graph, which outlives this component. Once the switch + // between a workflow's two views routes instead of reloading, this component is mounted on + // every switch, so an undisposed paper is left listening to that graph on each one. + it("disposes its own paper on destroy, so none is left listening to the shared graph", () => { + sizeMiniMapContainer(912, 100); + fixture.detectChanges(); + const remove = vi.spyOn((component as any).ownPaper, "remove"); + + fixture.destroy(); + + expect(remove).toHaveBeenCalled(); + }); + it("fits the whole main canvas into the mini-map container", () => { // 912 / (2688 - -960) == 0.25; the height (100) is deliberately different // so a width/height mix-up in the scale formula cannot pass. diff --git a/frontend/src/app/workspace/component/workflow-editor/mini-map/mini-map.component.ts b/frontend/src/app/workspace/component/workflow-editor/mini-map/mini-map.component.ts index 73a2c5bd089..3f9677db45e 100644 --- a/frontend/src/app/workspace/component/workflow-editor/mini-map/mini-map.component.ts +++ b/frontend/src/app/workspace/component/workflow-editor/mini-map/mini-map.component.ts @@ -17,7 +17,7 @@ * under the License. */ -import { AfterViewInit, Component, HostListener, OnDestroy, ViewChild } from "@angular/core"; +import { AfterViewInit, Component, ElementRef, HostListener, OnDestroy, ViewChild } from "@angular/core"; import { UntilDestroy, untilDestroyed } from "@ngneat/until-destroy"; import { WorkflowActionService } from "../../../service/workflow-graph/model/workflow-action.service"; import { MAIN_CANVAS } from "../workflow-editor.component"; @@ -50,18 +50,25 @@ export class MiniMapComponent implements AfterViewInit, OnDestroy { scale = 0; paper!: joint.dia.Paper; + /** The mini-map's own paper, as opposed to `paper`, which is the main canvas's. */ + private ownPaper!: joint.dia.Paper; + private map!: HTMLElement; dragging = false; hidden = false; constructor( private workflowActionService: WorkflowActionService, - private panelService: PanelService + private panelService: PanelService, + private elementRef: ElementRef ) {} ngAfterViewInit() { - const map = document.getElementById("mini-map")!; + // This component's own element, not whichever the document holds first: both views of a + // workflow mount a mini-map, and they overlap for a tick when the switch routes between them. + const map = (this.elementRef.nativeElement as HTMLElement).querySelector("#mini-map")!; + this.map = map; this.scale = map.offsetWidth / (MAIN_CANVAS.xMax - MAIN_CANVAS.xMin); - new joint.dia.Paper({ + this.ownPaper = new joint.dia.Paper({ el: map, model: this.workflowActionService.getJointGraphWrapper().jointGraph, background: { color: "#F6F6F6" }, @@ -90,6 +97,10 @@ export class MiniMapComponent implements AfterViewInit, OnDestroy { @HostListener("window:beforeunload") ngOnDestroy(): void { + // Bound to the root-provided joint graph, which outlives this component: an undisposed paper + // goes on listening to that graph from a detached node, and once the switch between a + // workflow's two views routes, one is left behind on every switch (issue #8582). + this.ownPaper?.remove(); localStorage.setItem("mini-map", JSON.stringify(this.hidden)); } @@ -102,8 +113,12 @@ export class MiniMapComponent implements AfterViewInit, OnDestroy { private updateNavigator(): void { if (!this.dragging) { - const editor = document.getElementById("workflow-editor")!; - const navigator = document.getElementById("mini-map-navigator")!; + // The main paper's own container, and this mini-map's own navigator: both views of a + // workflow mount one of each, so a document-wide lookup can answer with the other view's. + const editor = this.paper.el as HTMLElement; + const navigator = (this.elementRef.nativeElement as HTMLElement).querySelector( + "#mini-map-navigator" + )!; const editorRect = editor.getBoundingClientRect(); const point = this.paper.pageToLocalPoint({ diff --git a/frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.spec.ts b/frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.spec.ts index 49d8f790f23..d07669a02cd 100644 --- a/frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.spec.ts +++ b/frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.spec.ts @@ -160,6 +160,29 @@ describe("WorkflowEditorComponent", () => { } }); + // The paper is bound to the root-provided joint graph, which outlives this component. Once the + // switch between a workflow's two views routes instead of reloading, a mount happens on every + // switch, so an undisposed paper is left listening to that graph on each one. + it("disposes its paper on destroy, so none is left listening to the shared graph", () => { + const remove = vi.spyOn(component.paper, "remove"); + + fixture.destroy(); + + expect(remove).toHaveBeenCalled(); + }); + + // `.bind()` returns a new function every call, so removing a freshly bound one never matched + // what was added: one stale listener was left per mount, and after a few switches a single + // Ctrl/Cmd-Z was handled several times over. + it("removes the keydown listener it added, rather than a differently bound one", () => { + const handler = vi.spyOn(component as any, "_handleKeyboardAction"); + + fixture.destroy(); + document.dispatchEvent(new KeyboardEvent("keydown", { key: "z", ctrlKey: true })); + + expect(handler).not.toHaveBeenCalled(); + }); + it("should hide operator status on the canvas by default", () => { // keeps the Status toggle off until the user enables it const editor = (component as any).editor as HTMLElement; diff --git a/frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.ts b/frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.ts index d087aeb5820..afb51e3cfd7 100644 --- a/frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.ts +++ b/frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.ts @@ -109,6 +109,8 @@ export class WorkflowEditorComponent implements OnInit, AfterViewInit, OnDestroy editor!: HTMLElement; editorWrapper!: HTMLElement; paper!: joint.dia.Paper; + /** One bound reference, so the listener that is added is the one that can be removed. */ + private readonly keyboardActionListener = (event: KeyboardEvent) => this._handleKeyboardAction(event); // Heat-map hover tooltip (shown while the Performance overlay is on). Null when hidden. public heatmapTooltip: { x: number; @@ -213,7 +215,7 @@ export class WorkflowEditorComponent implements OnInit, AfterViewInit, OnDestroy const host = this.elementRef.nativeElement as HTMLElement; this.editor = host.querySelector("#workflow-editor")!; this.editorWrapper = host.querySelector("#workflow-editor-wrapper")!; - document.addEventListener("keydown", this._handleKeyboardAction.bind(this)); + document.addEventListener("keydown", this.keyboardActionListener); this.initializeJointPaper(); this.handleDisableJointPaperInteractiveness(); this.handleOperatorValidation(); @@ -261,7 +263,17 @@ export class WorkflowEditorComponent implements OnInit, AfterViewInit, OnDestroy ngOnDestroy(): void { this.paperResizeObserver?.disconnect(); - document.removeEventListener("keydown", this._handleKeyboardAction.bind(this)); + // The paper is bound to the joint graph, which is root-provided and outlives this component, + // so an undisposed one goes on listening to that graph from a DOM node no longer on the page. + // Harmless while every mount followed a page load; the switch between a workflow's two views + // routes now, so a mount happens on every switch and the papers pile up. Two live papers on + // one model both answer pointer events, and whichever answers decides whether an operator can + // be dragged -- measured: an operator was undraggable after two round-trips (issue #8582). + this.paper?.remove(); + // The same bound reference that was registered: `.bind()` returns a new function every call, + // so removing a freshly bound one never matched and left the listener behind. One stale + // listener per mount meant one Ctrl/Cmd-Z undoing several entries after a few switches. + document.removeEventListener("keydown", this.keyboardActionListener); // The overlay belongs to the canvas being viewed, but the wrapper holding // the view is root-provided and outlives this component, while the menu's // checkbox re-initializes to off and the metrics behind the overlay are 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 b713214346b..fcc3951eb4f 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 @@ -310,6 +310,17 @@ describe("WorkflowFormComponent", () => { workflowActionService.hasWorkflowOpen.mockReturnValue(true); }); + // The execution-state stream is a plain Subject too, so a page handed a session mid-run hears + // nothing about the run until it changes state: it showed Run for a workflow that was running, + // and the lock rule, which reads isRunning, would have let edit mode unlock a graph mid-run. + it("arrives knowing a run is already in flight", () => { + h.execution.state = ExecutionState.Running; + + build(formViewWorkflow).ngOnInit(); + + expect(component.executionState).toBe(ExecutionState.Running); + }); + it("takes what it needs from the open workflow instead of loading it", () => { build(formViewWorkflow).ngOnInit(); 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 1c84635ce6d..088e4048ab1 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 @@ -617,6 +617,12 @@ export class WorkflowFormComponent implements OnInit, OnDestroy { /** What the page does once the workflow is in front of it, whichever way it got there. */ private settleIntoForm(): void { + // The run this page arrived on top of. The state stream is a plain Subject and carries no + // current value, so a page handed a session mid-run hears nothing about it until the run + // changes state: it showed Run for a workflow that was running, and the lock rule below -- + // which reads `isRunning` -- would have let edit mode unlock a graph mid-run. On the loading + // path there is nothing in flight and this reads the same Uninitialized it started at. + this.executionState = this.executeWorkflowService.getExecutionState().state; // The workflow is shown, not edited, from here: dragging operators around or deleting them // belongs to the operator canvas. Lock now, and keep it locked against anything else that // unlocks the graph (clampEditability). The clamp is dropped when this page is destroyed; diff --git a/frontend/src/app/workspace/component/workflow-form/workflow-form.rendered.spec.ts b/frontend/src/app/workspace/component/workflow-form/workflow-form.rendered.spec.ts index 059aa7442d7..0a6a0bf692d 100644 --- a/frontend/src/app/workspace/component/workflow-form/workflow-form.rendered.spec.ts +++ b/frontend/src/app/workspace/component/workflow-form/workflow-form.rendered.spec.ts @@ -218,6 +218,9 @@ describe("WorkflowFormComponent (rendered template)", () => { provide: ExecuteWorkflowService, useValue: { getExecutionStateStream: () => EMPTY, + // The stream carries no current value, so the page reads the state it arrived on top + // of from here. Nothing is in flight in these tests. + getExecutionState: () => ({ state: ExecutionState.Uninitialized }), executeWorkflow: vi.fn(), killWorkflow: vi.fn(), resetExecutionAndWorkers: vi.fn(), diff --git a/frontend/src/app/workspace/component/workflow-form/workflow-form.spec-harness.ts b/frontend/src/app/workspace/component/workflow-form/workflow-form.spec-harness.ts index c7abeebf8a0..99898c86035 100644 --- a/frontend/src/app/workspace/component/workflow-form/workflow-form.spec-harness.ts +++ b/frontend/src/app/workspace/component/workflow-form/workflow-form.spec-harness.ts @@ -22,6 +22,7 @@ import { vi } from "vitest"; import { DefaultView } from "../../../dashboard/type/workflow-metadata.interface"; import { ResolvedField } from "../../service/form-binding/form-binding.service"; +import { ExecutionState } from "../../types/execute-workflow.interface"; /** The workflow every test opens by default: a form-default workflow, writable, empty content. */ export const formViewWorkflow = { name: "scGPT", defaultView: DefaultView.FORM, readonly: false, content: {} }; @@ -278,8 +279,14 @@ export function setupHarness() { const coeditorPresenceService = { coeditors: [] }; const route = { snapshot: { params: { id: "7" } } }; const operatorMetadataService = { getOperatorMetadata: () => of({}) }; + /** What the execute service currently holds, as the real one starts out. */ + const execution: { state: ExecutionState } = { state: ExecutionState.Uninitialized }; const executeWorkflowService = { getExecutionStateStream: () => executionStateStream.asObservable(), + // The state a page handed a live session arrives on top of: the stream above carries no + // current value, so this is the only way the page can learn a run is already in flight. + // Mutable, so a test can put a run in flight before the component is built. + getExecutionState: () => execution, executeWorkflow: vi.fn(), killWorkflow: vi.fn(), resetExecutionAndWorkers: vi.fn(), @@ -365,6 +372,7 @@ export function setupHarness() { datePipe, config, warehouseService, + execution, workflowChangedStream, formBindingChanged, workflowMetaDataChangedStream, diff --git a/frontend/src/app/workspace/component/workspace.component.spec.ts b/frontend/src/app/workspace/component/workspace.component.spec.ts index 77724910465..bf3bd1143d6 100644 --- a/frontend/src/app/workspace/component/workspace.component.spec.ts +++ b/frontend/src/app/workspace/component/workspace.component.spec.ts @@ -161,7 +161,12 @@ describe("WorkspaceComponent", () => { disconnect: vi.fn(), getConnectionResetStream: () => connectionResetSubject.asObservable(), }; - executeWorkflowService = { resetExecutionAndWorkers: vi.fn() }; + executeWorkflowService = { + resetExecutionAndWorkers: vi.fn(), + // As the real one does: reapplies the lock its current state implies, and says that state + // again for subscribers that arrived after the last change. + republishExecutionState: vi.fn(), + }; workflowConsoleService = { clearConsoleMessages: vi.fn() }; workflowResultService = { clearResults: vi.fn() }; @@ -261,11 +266,25 @@ describe("WorkspaceComponent", () => { expect(workflowPersistService.retrieveWorkflow).not.toHaveBeenCalled(); expect(workflowActionService.setNewSharedModel).not.toHaveBeenCalled(); expect(workflowActionService.reloadWorkflow).not.toHaveBeenCalled(); - expect(workflowActionService.enableWorkflowModification).toHaveBeenCalled(); expect(component.isLoading).toBe(false); expect(stubGraph.triggerCenterEvent).toHaveBeenCalled(); }); + // Not an unconditional unlock: a run may still be in flight, and the execute service reapplies + // its state-to-lock rule only when the state changes, so unlocking outright here left a running + // workflow editable until its run happened to end. The same call re-announces the state, which + // is what tells the page's own subscribers that a run is going. + it("asks the execute service to reapply its lock rather than unlocking the graph outright", async () => { + await createFixture(configureRoute({ id: "42" })); + workflowActionService.hasWorkflowOpen.mockReturnValue(true); + + component.ngOnInit(); + component.ngAfterViewInit(); + + expect(executeWorkflowService.republishExecutionState).toHaveBeenCalled(); + expect(workflowActionService.enableWorkflowModification).not.toHaveBeenCalled(); + }); + // This page is new and so is everything on it, but the metadata was set by the view that was // here before, and the stream carrying it does not replay. Everything that shows the workflow // -- the menu's name and id, the computing unit picker, this page's own write access -- would diff --git a/frontend/src/app/workspace/component/workspace.component.ts b/frontend/src/app/workspace/component/workspace.component.ts index e5ee2d5637c..5ee5fef37ab 100644 --- a/frontend/src/app/workspace/component/workspace.component.ts +++ b/frontend/src/app/workspace/component/workspace.component.ts @@ -348,10 +348,15 @@ export class WorkspaceComponent implements AfterViewInit, OnInit, OnDestroy { const wid = this.route.snapshot.params.id; // The Form View handed this workflow over still open: the graph, its co-editing room and the // computing unit connection are the ones it was using. Nothing to fetch, nothing to rebuild. - // Only the lock it put on the graph is undone, since editing is what the canvas is for, and - // the view is centred because this canvas's paper is a new one, at its default offset. + // The lock the Form View put on the graph is lifted, since editing is what the canvas is for, + // and the view is centred because this canvas's paper is a new one, at its default offset. if (this.resumedSession) { - this.workflowActionService.enableWorkflowModification(); + // Not an unconditional unlock: a run may still be in flight. The execute service owns the + // state-to-lock rule and reapplies it only when the state changes, so ask it to apply that + // rule again rather than restating it here. Unlocking outright left a workflow that was + // still running editable until its run happened to end. This also re-announces the state + // itself, which is what tells this page's own subscribers that a run is in flight. + this.executeWorkflowService.republishExecutionState(); this.registerAutoPersistWorkflow(); this.triggerCenter(); // This page and everything on it is new, and the metadata it needs was set by the view that diff --git a/frontend/src/app/workspace/service/execute-workflow/execute-workflow.service.spec.ts b/frontend/src/app/workspace/service/execute-workflow/execute-workflow.service.spec.ts index c097fbf5a1c..ed81aa5331e 100644 --- a/frontend/src/app/workspace/service/execute-workflow/execute-workflow.service.spec.ts +++ b/frontend/src/app/workspace/service/execute-workflow/execute-workflow.service.spec.ts @@ -20,7 +20,7 @@ import "zone.js/testing"; import { DOCUMENT } from "@angular/core"; -import { ExecutionState, LogicalPlan } from "../../types/execute-workflow.interface"; +import { ExecutionState, ExecutionStateInfo, LogicalPlan } from "../../types/execute-workflow.interface"; import { fakeAsync, flush, inject, TestBed, tick } from "@angular/core/testing"; import { ExecuteWorkflowService, FORM_DEBOUNCE_TIME_MS } from "./execute-workflow.service"; @@ -110,6 +110,45 @@ describe("ExecuteWorkflowService", () => { expect(injectedService).toBeTruthy(); })); + // A view handed a session mid-run subscribes after the last state change, and the stream carries + // no current value, so it heard nothing about the run: it showed Run for a workflow that was + // running. The lock is reapplied only when the state changes too, so a canvas that unlocked the + // graph on arrival left a running workflow editable until the run happened to end. + describe("republishExecutionState", () => { + it("says the current state again for subscribers that arrived after it was set", () => { + emitWsEvent({ type: "WorkflowStateEvent", state: ExecutionState.Running }); + const before = service.getExecutionState(); + const seen: ExecutionStateInfo[] = []; + service.getExecutionStateStream().subscribe(({ current }) => seen.push(current)); + + service.republishExecutionState(); + + expect(seen).toEqual([before]); + expect(service.getExecutionState()).toBe(before); + }); + + it("reapplies the lock the current state implies, rather than unlocking outright", () => { + const actionService = service["workflowActionService"]; + const enable = vi.spyOn(actionService, "enableWorkflowModification"); + const disable = vi.spyOn(actionService, "disableWorkflowModification"); + + // Uninitialized: nothing is running, so the graph may be edited. + service.republishExecutionState(); + expect(enable).toHaveBeenCalled(); + expect(disable).not.toHaveBeenCalled(); + + enable.mockClear(); + emitWsEvent({ type: "WorkflowStateEvent", state: ExecutionState.Running }); + enable.mockClear(); + disable.mockClear(); + + // Running: the graph stays locked, which is the case the canvas's hand-over got wrong. + service.republishExecutionState(); + expect(disable).toHaveBeenCalled(); + expect(enable).not.toHaveBeenCalled(); + }); + }); + it("resetExecutionAndWorkers() clears the execution state and worker assignments", () => { (service as any).currentState = { state: ExecutionState.Running }; (service as any).assignedWorkerIds.set("op1", ["w1", "w2"]); diff --git a/frontend/src/app/workspace/service/execute-workflow/execute-workflow.service.ts b/frontend/src/app/workspace/service/execute-workflow/execute-workflow.service.ts index 3f9f5c53a13..ecd5fc5c6f0 100644 --- a/frontend/src/app/workspace/service/execute-workflow/execute-workflow.service.ts +++ b/frontend/src/app/workspace/service/execute-workflow/execute-workflow.service.ts @@ -194,6 +194,22 @@ export class ExecuteWorkflowService { return this.currentState; } + /** + * Announce the execution state already in hand, unchanged, and reapply the graph lock it implies, + * for a view that arrived on a workflow whose run was already in flight. + * + * `getExecutionStateStream()` is a plain Subject, so it carries no current value: a view that + * attaches to a handed-over session subscribes after the last state change and hears nothing + * until the next one. Two things were then wrong at once. The arriving page showed **Run** for a + * workflow that was running, because its own `executionState` sat at its initial value. And the + * lock is only reapplied when the state changes (see `updateExecutionState`), so a canvas that + * unlocked the graph on arrival left a running workflow editable until the run happened to end. + */ + public republishExecutionState(): void { + this.updateWorkflowActionLock(this.currentState); + this.executionStateStream.next({ previous: this.currentState, current: this.currentState }); + } + public getErrorMessages(): ReadonlyArray { if (this.currentState?.state === ExecutionState.Failed) { return this.currentState.errorMessages; From 15fbb0e2f4744ec797f100b2367580eab88a2fdc Mon Sep 17 00:00:00 2001 From: Yang Zhang Date: Mon, 21 Sep 2026 11:30:29 -0700 Subject: [PATCH 05/13] fix(frontend): stop inventing a state transition, and keep the mini-map alive on unload Both of these were introduced by the previous commit, and both were found by the Copilot review on #8581. `republishExecutionState` announced the current state as a transition from itself. The stream carries transitions, not a current value, so that is a transition that never happened: the result panel reads one as a run just finishing, and the canvas editor throws outright on any event whose `previous` is `Recovering` and whose `current` is not a state recovery can end in -- which `Recovering -> Recovering` is not. Switching back to the canvas during recovery therefore raised an unhandled error. The announcement was not needed anyway. The menu already reads `getExecutionState()` when it is constructed, the Form View reads it before settling in, and every other subscriber wants transitions. So the method now only reapplies the lock, and is named for that: `reapplyExecutionLock`. The mini-map's `ngOnDestroy` carries a `beforeunload` host binding, and the previous commit put `paper.remove()` in it, so leaving the page disposed a paper the document needs if it comes back from the back/forward cache -- the same shape of bug #8599 was about. Split as that fix split the workspace's: `beforeunload` only remembers whether the mini-map was hidden, and the paper is disposed in `ngOnDestroy`, which runs when the component is genuinely destroyed. Deletion-checked: making the method announce again, or moving `paper.remove()` back into the unload path, each turns exactly one named test red. Full frontend suite: 224 files, 6158 passed, 1 skipped, 0 failed. AOT build, eslint and prettier clean. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FVvP3ttj22f9LB4p9u2anY --- .../mini-map/mini-map.component.spec.ts | 15 ++++++++ .../mini-map/mini-map.component.ts | 14 ++++++++ .../component/workspace.component.spec.ts | 11 +++--- .../component/workspace.component.ts | 7 ++-- .../execute-workflow.service.spec.ts | 36 ++++++++++--------- .../execute-workflow.service.ts | 23 ++++++------ 6 files changed, 71 insertions(+), 35 deletions(-) diff --git a/frontend/src/app/workspace/component/workflow-editor/mini-map/mini-map.component.spec.ts b/frontend/src/app/workspace/component/workflow-editor/mini-map/mini-map.component.spec.ts index e71f8dc4815..634b93663b1 100644 --- a/frontend/src/app/workspace/component/workflow-editor/mini-map/mini-map.component.spec.ts +++ b/frontend/src/app/workspace/component/workflow-editor/mini-map/mini-map.component.spec.ts @@ -211,6 +211,21 @@ describe("MiniMapComponent", () => { expect(remove).toHaveBeenCalled(); }); + // Not on the way out of the browser, though: the document may be kept in the back/forward + // cache and restored with its JavaScript state as it was left, re-running nothing, so a paper + // disposed there would stay disposed on a page that looks live (#8599). + it("keeps its paper when the browser unloads, and still remembers whether it was hidden", () => { + sizeMiniMapContainer(912, 100); + fixture.detectChanges(); + const remove = vi.spyOn((component as any).ownPaper, "remove"); + component.hidden = true; + + window.dispatchEvent(new Event("beforeunload")); + + expect(remove).not.toHaveBeenCalled(); + expect(localStorage.getItem("mini-map")).toBe("true"); + }); + it("fits the whole main canvas into the mini-map container", () => { // 912 / (2688 - -960) == 0.25; the height (100) is deliberately different // so a width/height mix-up in the scale formula cannot pass. diff --git a/frontend/src/app/workspace/component/workflow-editor/mini-map/mini-map.component.ts b/frontend/src/app/workspace/component/workflow-editor/mini-map/mini-map.component.ts index 3f9677db45e..9be00875bca 100644 --- a/frontend/src/app/workspace/component/workflow-editor/mini-map/mini-map.component.ts +++ b/frontend/src/app/workspace/component/workflow-editor/mini-map/mini-map.component.ts @@ -95,12 +95,26 @@ export class MiniMapComponent implements AfterViewInit, OnDestroy { this.panelService.resetPanelStream.pipe(untilDestroyed(this)).subscribe(() => (this.hidden = false)); } + /** + * The browser is leaving this document: remember whether the mini-map was hidden, and destroy + * nothing. The document may be kept in the back/forward cache and restored with its JavaScript + * state exactly as it was left, re-running nothing, so a paper disposed here would stay disposed + * on a page that looks live (the same reason the workspace stopped tearing down here, #8599). + */ @HostListener("window:beforeunload") + onBeforeUnload(): void { + this.rememberVisibility(); + } + ngOnDestroy(): void { // Bound to the root-provided joint graph, which outlives this component: an undisposed paper // goes on listening to that graph from a detached node, and once the switch between a // workflow's two views routes, one is left behind on every switch (issue #8582). this.ownPaper?.remove(); + this.rememberVisibility(); + } + + private rememberVisibility(): void { localStorage.setItem("mini-map", JSON.stringify(this.hidden)); } diff --git a/frontend/src/app/workspace/component/workspace.component.spec.ts b/frontend/src/app/workspace/component/workspace.component.spec.ts index bf3bd1143d6..0594d8e2899 100644 --- a/frontend/src/app/workspace/component/workspace.component.spec.ts +++ b/frontend/src/app/workspace/component/workspace.component.spec.ts @@ -163,9 +163,9 @@ describe("WorkspaceComponent", () => { }; executeWorkflowService = { resetExecutionAndWorkers: vi.fn(), - // As the real one does: reapplies the lock its current state implies, and says that state - // again for subscribers that arrived after the last change. - republishExecutionState: vi.fn(), + // As the real one does: reapplies the lock its current state implies, and says nothing on + // the state stream, which carries transitions rather than a current value. + reapplyExecutionLock: vi.fn(), }; workflowConsoleService = { clearConsoleMessages: vi.fn() }; workflowResultService = { clearResults: vi.fn() }; @@ -272,8 +272,7 @@ describe("WorkspaceComponent", () => { // Not an unconditional unlock: a run may still be in flight, and the execute service reapplies // its state-to-lock rule only when the state changes, so unlocking outright here left a running - // workflow editable until its run happened to end. The same call re-announces the state, which - // is what tells the page's own subscribers that a run is going. + // workflow editable until its run happened to end. it("asks the execute service to reapply its lock rather than unlocking the graph outright", async () => { await createFixture(configureRoute({ id: "42" })); workflowActionService.hasWorkflowOpen.mockReturnValue(true); @@ -281,7 +280,7 @@ describe("WorkspaceComponent", () => { component.ngOnInit(); component.ngAfterViewInit(); - expect(executeWorkflowService.republishExecutionState).toHaveBeenCalled(); + expect(executeWorkflowService.reapplyExecutionLock).toHaveBeenCalled(); expect(workflowActionService.enableWorkflowModification).not.toHaveBeenCalled(); }); diff --git a/frontend/src/app/workspace/component/workspace.component.ts b/frontend/src/app/workspace/component/workspace.component.ts index 5ee5fef37ab..118ad76fb6a 100644 --- a/frontend/src/app/workspace/component/workspace.component.ts +++ b/frontend/src/app/workspace/component/workspace.component.ts @@ -354,9 +354,10 @@ export class WorkspaceComponent implements AfterViewInit, OnInit, OnDestroy { // Not an unconditional unlock: a run may still be in flight. The execute service owns the // state-to-lock rule and reapplies it only when the state changes, so ask it to apply that // rule again rather than restating it here. Unlocking outright left a workflow that was - // still running editable until its run happened to end. This also re-announces the state - // itself, which is what tells this page's own subscribers that a run is in flight. - this.executeWorkflowService.republishExecutionState(); + // still running editable until its run happened to end. Nothing is announced: the menu + // reads the current state when it is constructed, and the rest of this page's subscribers + // want transitions, not a repeat of one that already happened. + this.executeWorkflowService.reapplyExecutionLock(); this.registerAutoPersistWorkflow(); this.triggerCenter(); // This page and everything on it is new, and the metadata it needs was set by the view that diff --git a/frontend/src/app/workspace/service/execute-workflow/execute-workflow.service.spec.ts b/frontend/src/app/workspace/service/execute-workflow/execute-workflow.service.spec.ts index ed81aa5331e..1f1b4b8eba8 100644 --- a/frontend/src/app/workspace/service/execute-workflow/execute-workflow.service.spec.ts +++ b/frontend/src/app/workspace/service/execute-workflow/execute-workflow.service.spec.ts @@ -114,39 +114,43 @@ describe("ExecuteWorkflowService", () => { // no current value, so it heard nothing about the run: it showed Run for a workflow that was // running. The lock is reapplied only when the state changes too, so a canvas that unlocked the // graph on arrival left a running workflow editable until the run happened to end. - describe("republishExecutionState", () => { - it("says the current state again for subscribers that arrived after it was set", () => { - emitWsEvent({ type: "WorkflowStateEvent", state: ExecutionState.Running }); - const before = service.getExecutionState(); - const seen: ExecutionStateInfo[] = []; - service.getExecutionStateStream().subscribe(({ current }) => seen.push(current)); - - service.republishExecutionState(); - - expect(seen).toEqual([before]); - expect(service.getExecutionState()).toBe(before); - }); - + // A view handed a session mid-run needs the lock the run implies; the lock is otherwise only + // reapplied when the state changes, so a canvas that unlocked on arrival left a running workflow + // editable until its run happened to end. + describe("reapplyExecutionLock", () => { it("reapplies the lock the current state implies, rather than unlocking outright", () => { const actionService = service["workflowActionService"]; const enable = vi.spyOn(actionService, "enableWorkflowModification"); const disable = vi.spyOn(actionService, "disableWorkflowModification"); // Uninitialized: nothing is running, so the graph may be edited. - service.republishExecutionState(); + service.reapplyExecutionLock(); expect(enable).toHaveBeenCalled(); expect(disable).not.toHaveBeenCalled(); - enable.mockClear(); emitWsEvent({ type: "WorkflowStateEvent", state: ExecutionState.Running }); enable.mockClear(); disable.mockClear(); // Running: the graph stays locked, which is the case the canvas's hand-over got wrong. - service.republishExecutionState(); + service.reapplyExecutionLock(); expect(disable).toHaveBeenCalled(); expect(enable).not.toHaveBeenCalled(); }); + + // The stream carries transitions. Repeating the current state as `previous -> current` of the + // same state is a transition that never happened: the result panel reads one as a run just + // finishing, and the canvas editor throws on any event whose `previous` is Recovering and whose + // `current` is not a state recovery can end in -- which a Recovering -> Recovering repeat is. + it("says nothing on the state stream, so no transition is invented", () => { + emitWsEvent({ type: "WorkflowStateEvent", state: ExecutionState.Recovering }); + const seen: ExecutionStateInfo[] = []; + service.getExecutionStateStream().subscribe(({ current }) => seen.push(current)); + + service.reapplyExecutionLock(); + + expect(seen).toEqual([]); + }); }); it("resetExecutionAndWorkers() clears the execution state and worker assignments", () => { diff --git a/frontend/src/app/workspace/service/execute-workflow/execute-workflow.service.ts b/frontend/src/app/workspace/service/execute-workflow/execute-workflow.service.ts index ecd5fc5c6f0..d7e6f2b138f 100644 --- a/frontend/src/app/workspace/service/execute-workflow/execute-workflow.service.ts +++ b/frontend/src/app/workspace/service/execute-workflow/execute-workflow.service.ts @@ -195,19 +195,22 @@ export class ExecuteWorkflowService { } /** - * Announce the execution state already in hand, unchanged, and reapply the graph lock it implies, - * for a view that arrived on a workflow whose run was already in flight. + * Apply the graph lock the current execution state implies, for a view that arrived on a workflow + * whose run was already in flight. * - * `getExecutionStateStream()` is a plain Subject, so it carries no current value: a view that - * attaches to a handed-over session subscribes after the last state change and hears nothing - * until the next one. Two things were then wrong at once. The arriving page showed **Run** for a - * workflow that was running, because its own `executionState` sat at its initial value. And the - * lock is only reapplied when the state changes (see `updateExecutionState`), so a canvas that - * unlocked the graph on arrival left a running workflow editable until the run happened to end. + * The lock is otherwise only reapplied when the state changes (see `updateExecutionState`), so a + * view that unlocked the graph on arrival left a workflow that was still running editable until + * its run happened to end. The rule itself stays in the one place that owns it. + * + * Deliberately silent. `executionStateStream` carries transitions, not a current value, and + * re-announcing the state as `previous -> current` of the same state would be a transition that + * never happened: the result panel would read it as a run just finishing, and the canvas editor + * throws outright on any event whose `previous` is `Recovering` and whose `current` is not one of + * the states recovery can end in. A view that needs the current state reads `getExecutionState()` + * -- as the menu does when it is constructed, and as the Form View does before it settles in. */ - public republishExecutionState(): void { + public reapplyExecutionLock(): void { this.updateWorkflowActionLock(this.currentState); - this.executionStateStream.next({ previous: this.currentState, current: this.currentState }); } public getErrorMessages(): ReadonlyArray { From d7f7cef1aea3a2e3d1e7fa150f62c98d7d84225a Mon Sep 17 00:00:00 2001 From: Yang Zhang Date: Mon, 21 Sep 2026 11:36:20 -0700 Subject: [PATCH 06/13] docs(frontend): say only what held up about the undisposed papers The comment on the paper disposal asserted both a mechanism (whichever of two live papers answers a pointer event decides whether an operator can be dragged) and a measurement (undraggable after two round-trips) as settled. Neither held up: the measurement came from a test that read "the first .joint-element", which is not the same element once one has been dragged or selected, and measuring one operator by model id across six round-trips gave a single failure that did not recur. The retraction was in a review thread, which will be buried long before the comment is. Found by @mengw15 in review. The leak justifies disposing the papers on its own: they pile up once remounting is the normal switch. Whether the undraggable operators recorded in #8580 follow from it is left to #8582, which is where it was already being tracked. The PR description carried the same overclaim, and said the disposal was deferred when it is now here; both corrected. Comment only; no behaviour change. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FVvP3ttj22f9LB4p9u2anY --- .../component/workflow-editor/workflow-editor.component.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.ts b/frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.ts index afb51e3cfd7..0983fe48216 100644 --- a/frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.ts +++ b/frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.ts @@ -266,9 +266,8 @@ export class WorkflowEditorComponent implements OnInit, AfterViewInit, OnDestroy // The paper is bound to the joint graph, which is root-provided and outlives this component, // so an undisposed one goes on listening to that graph from a DOM node no longer on the page. // Harmless while every mount followed a page load; the switch between a workflow's two views - // routes now, so a mount happens on every switch and the papers pile up. Two live papers on - // one model both answer pointer events, and whichever answers decides whether an operator can - // be dragged -- measured: an operator was undraggable after two round-trips (issue #8582). + // routes now, so a mount happens on every switch and the papers pile up. Whether the + // undraggable operators recorded in #8580 follow from that is not settled -- #8582 tracks it. this.paper?.remove(); // The same bound reference that was registered: `.bind()` returns a new function every call, // so removing a freshly bound one never matched and left the listener behind. One stale From bc4d6c9c6006fc6fabb6f60ff7f44c1307044a7d Mon Sep 17 00:00:00 2001 From: Yang Zhang Date: Mon, 21 Sep 2026 12:18:56 -0700 Subject: [PATCH 07/13] fix(frontend): give a handed-over view the rest of the run, not just its state Three more from the Copilot review on #8581, all the same shape as the ones before them: the hand-over retains the execution, and each view rebuilt only part of its picture of it. - The computing-unit selector asks the backend whether an execution is running, and its answer arrives after whatever the arriving page set. The query covers only Running and Initializing, so a handed-over Paused, Pausing, Recovering or Resuming run answered "none" and the selector unlocked the graph, undoing the lock the page had just been given. It defers to `reapplyExecutionLock` now, so "no execution someone else started" no longer means "nothing is running here". - The run clock is a websocket event the backend sends only when the run's start or end time changes, so a view created mid-run counted from zero until the run ended. `ExecuteWorkflowService` holds the last duration now, and both the menu and the Form View read it when they mount -- the menu had the same hole on a resumed canvas, which the review found on the Form View side. - Restoring the state enum alone left the Form View's failure banner off: no transition follows a hand-over, so the subscriber that raises it never ran, and the form showed nothing while the canvas it came from showed the failure. The banner logic moved into `showFailureIfAny`, called from the subscriber as before and once on arrival, after the fields exist -- it asks them whether a required one was left empty. Deletion-checked: restoring the selector's unconditional unlock, or dropping either of the form's two reads, each turns exactly the intended named test red. The selector's existing tests did not catch it, because with nothing running the two spellings behave alike; the new test is the case that differs. Full frontend suite: 224 files, 6161 passed, 1 skipped, 0 failed. AOT build, eslint and prettier clean. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FVvP3ttj22f9LB4p9u2anY --- .../component/menu/menu.component.ts | 4 ++ ...computing-unit-selection.component.spec.ts | 23 ++++++++++ .../computing-unit-selection.component.ts | 11 ++++- .../workflow-form.component.spec.ts | 23 ++++++++++ .../workflow-form/workflow-form.component.ts | 42 +++++++++++++------ .../workflow-form.rendered.spec.ts | 1 + .../workflow-form.spec-harness.ts | 7 +++- .../execute-workflow.service.ts | 13 ++++++ 8 files changed, 108 insertions(+), 16 deletions(-) diff --git a/frontend/src/app/workspace/component/menu/menu.component.ts b/frontend/src/app/workspace/component/menu/menu.component.ts index 1ccf632bd80..c865db0f787 100644 --- a/frontend/src/app/workspace/component/menu/menu.component.ts +++ b/frontend/src/app/workspace/component/menu/menu.component.ts @@ -210,6 +210,10 @@ export class MenuComponent implements OnInit, OnDestroy { this.executionDuration += 1000; }); this.executionState = executeWorkflowService.getExecutionState().state; + // The clock, for the same reason: the backend sends the duration only when the run's start or + // end time changes, so a menu created mid-run -- which is what a routed switch between a + // workflow's two views makes -- would count from zero until the run ended. + this.executionDuration = executeWorkflowService.getExecutionDuration(); // return the run button after the execution is finished, either // when the value is valid or invalid const initBehavior = this.getRunButtonBehavior(); diff --git a/frontend/src/app/workspace/component/power-button/computing-unit-selection.component.spec.ts b/frontend/src/app/workspace/component/power-button/computing-unit-selection.component.spec.ts index 3e0153b2683..432d97d379b 100644 --- a/frontend/src/app/workspace/component/power-button/computing-unit-selection.component.spec.ts +++ b/frontend/src/app/workspace/component/power-button/computing-unit-selection.component.spec.ts @@ -49,6 +49,7 @@ import { ComputingUnitCreateModalComponent } from "../../../common/component/com import { NoopAnimationsModule } from "@angular/platform-browser/animations"; import { DEFAULT_WORKFLOW, WorkflowActionService } from "../../service/workflow-graph/model/workflow-action.service"; import { WorkflowExecutionsService } from "../../../dashboard/service/user/workflow-executions/workflow-executions.service"; +import { ExecuteWorkflowService } from "../../service/execute-workflow/execute-workflow.service"; import { WorkflowExecutionsEntry } from "../../../dashboard/type/workflow-executions-entry"; import { WorkflowMetadata } from "../../../dashboard/type/workflow-metadata.interface"; import { ExecutionState } from "../../types/execute-workflow.interface"; @@ -1242,6 +1243,28 @@ describe("PowerButtonComponent", () => { expect(enableSpy).not.toHaveBeenCalled(); }); + // "No execution someone else started" is not "nothing is running here": this query asks only + // about Running and Initializing, and its answer lands asynchronously, after whatever the page + // set on arrival. A view handed a session with a run in flight had just been given the lock; + // enabling outright took it away again, leaving a running workflow editable. + it("leaves a run already in flight locked, even when the backend reports no executions", () => { + const actionService = TestBed.inject(WorkflowActionService); + vi.spyOn(actionService, "getWorkflowMetadata").mockReturnValue({ ...DEFAULT_WORKFLOW, wid: 42 }); + const disableSpy = vi.spyOn(actionService, "disableWorkflowModification").mockImplementation(() => {}); + const enableSpy = vi.spyOn(actionService, "enableWorkflowModification").mockImplementation(() => {}); + vi.spyOn(TestBed.inject(WorkflowExecutionsService), "retrieveWorkflowExecutions").mockReturnValue( + of([] as WorkflowExecutionsEntry[]) + ); + // This page's own execute service is holding a run, as it would after a hand-over. + (TestBed.inject(ExecuteWorkflowService) as any).currentState = { state: ExecutionState.Running }; + + const { selected$ } = bootWithSelectedStream(); + selected$.next(makeComputingUnit({ cuid: 7 })); + + expect(disableSpy).toHaveBeenCalled(); + expect(enableSpy).not.toHaveBeenCalled(); + }); + it("enables workflow modification when there are no ongoing executions", () => { const actionService = TestBed.inject(WorkflowActionService); vi.spyOn(actionService, "getWorkflowMetadata").mockReturnValue({ ...DEFAULT_WORKFLOW, wid: 42 }); diff --git a/frontend/src/app/workspace/component/power-button/computing-unit-selection.component.ts b/frontend/src/app/workspace/component/power-button/computing-unit-selection.component.ts index e624563b3bc..da869a6b62f 100644 --- a/frontend/src/app/workspace/component/power-button/computing-unit-selection.component.ts +++ b/frontend/src/app/workspace/component/power-button/computing-unit-selection.component.ts @@ -33,6 +33,7 @@ import { WarehouseActionsService } from "../../../common/service/warehouse/wareh import { DashboardWarehouse } from "../../../common/type/warehouse"; import { NzModalService, NzModalComponent, NzModalContentDirective } from "ng-zorro-antd/modal"; import { WorkflowExecutionsService } from "../../../dashboard/service/user/workflow-executions/workflow-executions.service"; +import { ExecuteWorkflowService } from "../../service/execute-workflow/execute-workflow.service"; import { WorkflowExecutionsEntry } from "../../../dashboard/type/workflow-executions-entry"; import { ExecutionState } from "../../types/execute-workflow.interface"; import { ShareAccessComponent } from "../../../dashboard/component/user/share-access/share-access.component"; @@ -210,7 +211,8 @@ export class ComputingUnitSelectionComponent implements OnInit { private workflowPveService: WorkflowPveService, private ngZone: NgZone, private warehouseService: WarehouseService, - private warehouseActionsService: WarehouseActionsService + private warehouseActionsService: WarehouseActionsService, + private executeWorkflowService: ExecuteWorkflowService ) {} ngOnInit(): void { @@ -310,7 +312,12 @@ export class ComputingUnitSelectionComponent implements OnInit { ); this.workflowActionService.disableWorkflowModification(); } else { - this.workflowActionService.enableWorkflowModification(); + // "No execution someone else started" is not "nothing is running here". This asks only + // about Running and Initializing, and the answer arrives asynchronously, after whatever + // the page set on arrival, so unlocking outright undid the lock a run in flight had just + // been given -- a handed-over Paused or Recovering run, say, which this query does not + // look for. Defer to the state-to-lock rule the execute service owns. + this.executeWorkflowService.reapplyExecutionLock(); } }); } 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 fcc3951eb4f..da413b382f2 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 @@ -321,6 +321,29 @@ describe("WorkflowFormComponent", () => { expect(component.executionState).toBe(ExecutionState.Running); }); + // The duration arrives as a websocket event the backend sends only when the run's start or end + // time changes, so a page that joined mid-run counted from zero until the run ended. + it("arrives with the clock the run is already at", () => { + h.execution.state = ExecutionState.Running; + h.execution.duration = 42_000; + + build(formViewWorkflow).ngOnInit(); + + expect(component.executionDuration).toBe(42_000); + }); + + // Restoring the state enum alone left the banner off: no transition follows the hand-over, so + // the subscriber that raises it never runs, and the form showed nothing while the canvas it + // came from still showed the failure. + it("arrives showing a failure the retained run already had", () => { + h.execution.state = ExecutionState.Failed; + h.execution.errorMessages = [{ message: "boom" }]; + + build(formViewWorkflow).ngOnInit(); + + expect(component.runError).toContain("boom"); + }); + it("takes what it needs from the open workflow instead of loading it", () => { build(formViewWorkflow).ngOnInit(); 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 088e4048ab1..b037afc511e 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 @@ -58,7 +58,7 @@ import { WorkflowConsoleService } from "../../service/workflow-console/workflow- import { WorkflowResultService } from "../../service/workflow-result/workflow-result.service"; import { PanelResizeService } from "../../service/workflow-result/panel-resize/panel-resize.service"; import { WorkflowWebsocketService } from "../../service/workflow-websocket/workflow-websocket.service"; -import { ExecutionState } from "../../types/execute-workflow.interface"; +import { ExecutionState, ExecutionStateInfo } from "../../types/execute-workflow.interface"; import { OperatorPredicate, Point } from "../../types/workflow-common.interface"; import { ComputingUnitSelectionComponent } from "../power-button/computing-unit-selection.component"; import { PropertyEditorComponent } from "../property-editor/property-editor.component"; @@ -463,17 +463,7 @@ export class WorkflowFormComponent implements OnInit, OnDestroy { if (!wasRunning && this.isRunning) { this.runError = ""; } - // Surface a failed run. Without this the spinner just stops and the form gives zero - // feedback -- the opposite of what a reader needs. - if (current.state === ExecutionState.Failed) { - // A required input left empty is by far the commonest reason a run fails here, and the - // engine reports it as an opaque "... is not contained in the schema". Answer with the - // same word the field itself already shows ("required"), so the two messages are - // consistent -- and it covers every operator, not just this one. - this.runError = this.hasEmptyRequiredInputs() - ? "Run failed: please fill in the required fields." - : this.friendlyRunError(current.errorMessages?.[0]?.message?.trim() ?? ""); - } + this.showFailureIfAny(current); // Fit the charts to their cards once a run has results. Deliberately not on run START: the // run repaints operators and a re-fit then zoomed the whole preview down. Deliberately does // not open the workflow either -- someone using the form came for the inputs and results. @@ -615,6 +605,23 @@ export class WorkflowFormComponent implements OnInit, OnDestroy { }); } + /** + * Surface a failed run. Without this the spinner just stops and the form gives zero feedback -- + * the opposite of what a reader needs. Reads the fields, so it runs after they are built. + */ + private showFailureIfAny(state: ExecutionStateInfo): void { + if (state.state !== ExecutionState.Failed) { + return; + } + // A required input left empty is by far the commonest reason a run fails here, and the engine + // reports it as an opaque "... is not contained in the schema". Answer with the same word the + // field itself already shows ("required"), so the two messages are consistent -- and it covers + // every operator, not just this one. + this.runError = this.hasEmptyRequiredInputs() + ? "Run failed: please fill in the required fields." + : this.friendlyRunError(state.errorMessages?.[0]?.message?.trim() ?? ""); + } + /** What the page does once the workflow is in front of it, whichever way it got there. */ private settleIntoForm(): void { // The run this page arrived on top of. The state stream is a plain Subject and carries no @@ -622,7 +629,12 @@ export class WorkflowFormComponent implements OnInit, OnDestroy { // changes state: it showed Run for a workflow that was running, and the lock rule below -- // which reads `isRunning` -- would have let edit mode unlock a graph mid-run. On the loading // path there is nothing in flight and this reads the same Uninitialized it started at. - this.executionState = this.executeWorkflowService.getExecutionState().state; + const retained = this.executeWorkflowService.getExecutionState(); + this.executionState = retained.state; + // Likewise the clock: the duration arrives as a websocket event the backend sends only when + // the run's start or end time changes, so a page that joined mid-run would have counted from + // zero -- or not at all -- until the run ended. + this.executionDuration = this.executeWorkflowService.getExecutionDuration(); // The workflow is shown, not edited, from here: dragging operators around or deleting them // belongs to the operator canvas. Lock now, and keep it locked against anything else that // unlocks the graph (clampEditability). The clamp is dropped when this page is destroyed; @@ -632,6 +644,10 @@ export class WorkflowFormComponent implements OnInit, OnDestroy { this.refreshSavedState(); this.later(() => this.adjustWorkflowNameWidth(), 0); this.readConfig(); + // After the fields exist: a failure banner asks them whether a required one was left empty. + // A page handed a session after a failed run would otherwise show no failure at all, while + // the canvas it came from still showed one. + this.showFailureIfAny(retained); this.registerMetadataRefresh(); this.registerAutoPersist(); this.loading = false; diff --git a/frontend/src/app/workspace/component/workflow-form/workflow-form.rendered.spec.ts b/frontend/src/app/workspace/component/workflow-form/workflow-form.rendered.spec.ts index 0a6a0bf692d..b4e61bfcd50 100644 --- a/frontend/src/app/workspace/component/workflow-form/workflow-form.rendered.spec.ts +++ b/frontend/src/app/workspace/component/workflow-form/workflow-form.rendered.spec.ts @@ -221,6 +221,7 @@ describe("WorkflowFormComponent (rendered template)", () => { // The stream carries no current value, so the page reads the state it arrived on top // of from here. Nothing is in flight in these tests. getExecutionState: () => ({ state: ExecutionState.Uninitialized }), + getExecutionDuration: () => 0, executeWorkflow: vi.fn(), killWorkflow: vi.fn(), resetExecutionAndWorkers: vi.fn(), diff --git a/frontend/src/app/workspace/component/workflow-form/workflow-form.spec-harness.ts b/frontend/src/app/workspace/component/workflow-form/workflow-form.spec-harness.ts index 99898c86035..aa392a69798 100644 --- a/frontend/src/app/workspace/component/workflow-form/workflow-form.spec-harness.ts +++ b/frontend/src/app/workspace/component/workflow-form/workflow-form.spec-harness.ts @@ -280,13 +280,18 @@ export function setupHarness() { const route = { snapshot: { params: { id: "7" } } }; const operatorMetadataService = { getOperatorMetadata: () => of({}) }; /** What the execute service currently holds, as the real one starts out. */ - const execution: { state: ExecutionState } = { state: ExecutionState.Uninitialized }; + const execution: { state: ExecutionState; errorMessages?: { message: string }[]; duration: number } = { + state: ExecutionState.Uninitialized, + duration: 0, + }; const executeWorkflowService = { getExecutionStateStream: () => executionStateStream.asObservable(), // The state a page handed a live session arrives on top of: the stream above carries no // current value, so this is the only way the page can learn a run is already in flight. // Mutable, so a test can put a run in flight before the component is built. getExecutionState: () => execution, + // The clock the backend last reported, for a page that mounted mid-run. + getExecutionDuration: () => execution.duration, executeWorkflow: vi.fn(), killWorkflow: vi.fn(), resetExecutionAndWorkers: vi.fn(), diff --git a/frontend/src/app/workspace/service/execute-workflow/execute-workflow.service.ts b/frontend/src/app/workspace/service/execute-workflow/execute-workflow.service.ts index d7e6f2b138f..259900a4852 100644 --- a/frontend/src/app/workspace/service/execute-workflow/execute-workflow.service.ts +++ b/frontend/src/app/workspace/service/execute-workflow/execute-workflow.service.ts @@ -95,6 +95,8 @@ export class ExecuteWorkflowService { // TODO: move this to another service, or redesign how this // information is stored on the frontend. private assignedWorkerIds: Map = new Map(); + /** The last duration the backend reported for the current run; see getExecutionDuration. */ + private executionDuration = 0; constructor( private workflowActionService: WorkflowActionService, @@ -117,6 +119,12 @@ export class ExecuteWorkflowService { case "WorkerAssignmentUpdateEvent": this.assignedWorkerIds.set(event.operatorId, event.workerIds); break; + case "ExecutionDurationUpdateEvent": + // Held for views that mount mid-run. The backend sends this only when the run's start or + // end time changes, so a view created afterwards -- which is what a routed switch between + // a workflow's two views makes -- never hears it and would count from zero. + this.executionDuration = event.duration; + break; default: // workflow status related event this.handleReconfigurationEvent(event); @@ -194,6 +202,11 @@ export class ExecuteWorkflowService { return this.currentState; } + /** How long the current run has been going, for a view that mounted after it started. */ + public getExecutionDuration(): number { + return this.executionDuration; + } + /** * Apply the graph lock the current execution state implies, for a view that arrived on a workflow * whose run was already in flight. From 343756f713f52093b3043cf6724480d69a5655f3 Mon Sep 17 00:00:00 2001 From: Yang Zhang Date: Mon, 21 Sep 2026 13:01:32 -0700 Subject: [PATCH 08/13] fix(frontend): anchor the run clock in the service instead of snapshotting it The previous commit cached the last duration the backend reported and had each view read it on mount. That does not work, and @mengw15 showed why from the engine: `ExecutionStatsService` emits `ExecutionDurationUpdateEvent` only when `startTimeStamp` or `endTimeStamp` changes, and the first of those carries `currentTime - startTimeStamp` computed just after the timestamp is written -- a few milliseconds. So the cached number stands still at roughly zero for the whole run, and the per-second advance was local to each view's `timer(1000, 1000)`, which sat downstream of the event: a view that mounted mid-run received no event and never started counting. A hand-over 60s into a run therefore showed `0s`, frozen, until the run ended. My test did not catch that because it was circular: the harness stubbed `getExecutionDuration`, the test set it to 42000 and asserted 42000 came back. It covered the form reading a getter, not what the service returns. `ExecuteWorkflowService` keeps an anchor now -- the reported duration, when it was reported, and whether the run is going -- and answers `reported + (now - at)` while it runs. It ticks that on a stream both views subscribe to, replaying the current value, so a view mounting mid-run is handed where the run has got to and keeps counting. The two views' duplicated timers are gone, and the clock is reset with the execution state, so the next workflow does not open showing the previous run's time (@copilot). The behaviour moved, so the tests moved: the ticking, the anchoring, the end of a run and the reset are covered on the service against real timers, and each view keeps one test that it shows what the service says. Deletion-checked: going back to returning the snapshot, dropping the ticker, or not resetting the clock each turns named tests red. Full frontend suite: 224 files, 6159 passed, 1 skipped, 0 failed. AOT build, eslint and prettier clean. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FVvP3ttj22f9LB4p9u2anY --- .../component/menu/menu.component.spec.ts | 116 +++--------------- .../component/menu/menu.component.ts | 23 ++-- .../workflow-form.component.spec.ts | 25 ++-- .../workflow-form/workflow-form.component.ts | 22 ++-- .../workflow-form.rendered.spec.ts | 2 +- .../workflow-form.spec-harness.ts | 10 +- .../execute-workflow.service.spec.ts | 65 ++++++++++ .../execute-workflow.service.ts | 49 ++++++-- 8 files changed, 157 insertions(+), 155 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 36d68606806..d7fbc0db6f3 100644 --- a/frontend/src/app/workspace/component/menu/menu.component.spec.ts +++ b/frontend/src/app/workspace/component/menu/menu.component.spec.ts @@ -1207,104 +1207,24 @@ describe("MenuComponent", () => { // (base-duration updates, 1s cadence, restart-on-event, stop-when-idle) and, // crucially, that the timer is torn down with the component so it cannot keep // firing or leak after destroy. - describe("execution duration timer", () => { - let durationEvents$: Subject<{ type: "ExecutionDurationUpdateEvent" } & ExecutionDurationUpdateEvent>; - let timerFixture: ComponentFixture; - let timerComponent: MenuComponent; - - function emitDuration(duration: number, isRunning: boolean): void { - durationEvents$.next({ type: "ExecutionDurationUpdateEvent", duration, isRunning }); - } - - beforeEach(() => { - vi.useFakeTimers(); - durationEvents$ = new Subject(); - const websocket = TestBed.inject(WorkflowWebsocketService); - const original = websocket.subscribeToEvent.bind(websocket); - // Only intercept the duration event; defer every other event type to the - // real implementation so unrelated subscriptions keep working. - vi.spyOn(websocket, "subscribeToEvent").mockImplementation((type: any) => - type === "ExecutionDurationUpdateEvent" ? (durationEvents$.asObservable() as any) : original(type) - ); - - timerFixture = TestBed.createComponent(MenuComponent); - timerComponent = timerFixture.componentInstance; - timerFixture.detectChanges(); - }); - - afterEach(() => { - vi.useRealTimers(); - }); - - it("sets executionDuration to the event's base duration on each event", () => { - emitDuration(5000, false); - expect(timerComponent.executionDuration).toBe(5000); - - emitDuration(8000, false); - expect(timerComponent.executionDuration).toBe(8000); - }); - - it("advances the duration by 1s every second while running", () => { - emitDuration(0, true); - expect(timerComponent.executionDuration).toBe(0); - - vi.advanceTimersByTime(1000); - expect(timerComponent.executionDuration).toBe(1000); - - vi.advanceTimersByTime(2000); - expect(timerComponent.executionDuration).toBe(3000); - }); - - it("does not start a timer when the execution is not running", () => { - emitDuration(7000, false); - - vi.advanceTimersByTime(5000); - - expect(timerComponent.executionDuration).toBe(7000); - }); - - it("restarts the 1s timer on each new running event, cancelling the previous one", () => { - emitDuration(0, true); - vi.advanceTimersByTime(1000); - expect(timerComponent.executionDuration).toBe(1000); - - // A new event resets the base duration and restarts the cadence; the - // previous timer must be cancelled (switchMap) so it cannot double-count. - emitDuration(10000, true); - expect(timerComponent.executionDuration).toBe(10000); - - vi.advanceTimersByTime(500); - expect(timerComponent.executionDuration).toBe(10000); - - vi.advanceTimersByTime(500); - expect(timerComponent.executionDuration).toBe(11000); - }); - - it("stops the timer when a running execution transitions to not running", () => { - emitDuration(0, true); - vi.advanceTimersByTime(1000); - expect(timerComponent.executionDuration).toBe(1000); - - emitDuration(2000, false); - vi.advanceTimersByTime(5000); - expect(timerComponent.executionDuration).toBe(2000); - }); - - it("tears down the timer on destroy so the duration stops advancing", () => { - emitDuration(0, true); - vi.advanceTimersByTime(1000); - expect(timerComponent.executionDuration).toBe(1000); - - timerFixture.destroy(); - - // The previously running timer must not keep firing after destroy... - vi.advanceTimersByTime(5000); - expect(timerComponent.executionDuration).toBe(1000); - - // ...nor should late events revive it (the source subscription is closed). - emitDuration(9999, true); - vi.advanceTimersByTime(5000); - expect(timerComponent.executionDuration).toBe(1000); + // The clock itself lives in ExecuteWorkflowService now -- anchored and ticked there, so a menu + // that mounts mid-run gets where the run has got to instead of starting from zero, which is what + // a routed switch between the canvas and the Form View makes. What is left here is that the menu + // shows what the service says. + describe("execution duration", () => { + it("shows the run clock the service reports", () => { + const ticks = new BehaviorSubject(7000); + vi.spyOn(executeWorkflowService, "getExecutionDurationStream").mockReturnValue(ticks.asObservable()); + + const f = TestBed.createComponent(MenuComponent); + f.detectChanges(); + + // Replayed on subscribe: the value the run was already at when this menu mounted. + expect(f.componentInstance.executionDuration).toBe(7000); + + ticks.next(8000); + expect(f.componentInstance.executionDuration).toBe(8000); + f.destroy(); }); }); diff --git a/frontend/src/app/workspace/component/menu/menu.component.ts b/frontend/src/app/workspace/component/menu/menu.component.ts index c865db0f787..465aea31dcc 100644 --- a/frontend/src/app/workspace/component/menu/menu.component.ts +++ b/frontend/src/app/workspace/component/menu/menu.component.ts @@ -198,22 +198,15 @@ export class MenuComponent implements OnInit, OnDestroy { private jupyterPanelService: JupyterPanelService, private fileSaverService: FileSaverService ) { - workflowWebsocketService - .subscribeToEvent("ExecutionDurationUpdateEvent") - .pipe( - tap(event => (this.executionDuration = event.duration)), - // restart the 1s timer on each event, only while running - switchMap(event => (event.isRunning ? timer(1000, 1000) : EMPTY)), - untilDestroyed(this) - ) - .subscribe(() => { - this.executionDuration += 1000; - }); + // From the service, not from the engine's event directly: that event arrives twice in a whole + // run, so a timer hung off it never started for a menu that mounted in between -- which is what + // a routed switch between the canvas and the Form View makes. The service anchors the clock and + // ticks it, and replays the current value to whoever subscribes. + executeWorkflowService + .getExecutionDurationStream() + .pipe(untilDestroyed(this)) + .subscribe(duration => (this.executionDuration = duration)); this.executionState = executeWorkflowService.getExecutionState().state; - // The clock, for the same reason: the backend sends the duration only when the run's start or - // end time changes, so a menu created mid-run -- which is what a routed switch between a - // workflow's two views makes -- would count from zero until the run ended. - this.executionDuration = executeWorkflowService.getExecutionDuration(); // return the run button after the execution is finished, either // when the value is valid or invalid const initBehavior = this.getRunButtonBehavior(); 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 da413b382f2..ea4a2948540 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 @@ -321,11 +321,12 @@ describe("WorkflowFormComponent", () => { expect(component.executionState).toBe(ExecutionState.Running); }); - // The duration arrives as a websocket event the backend sends only when the run's start or end - // time changes, so a page that joined mid-run counted from zero until the run ended. + // The backend sends the duration event twice in a whole run, and the old per-view timer sat + // downstream of it, so a page that mounted in between never received one and never started + // counting. It reads the service's clock now, which replays where the run has got to. it("arrives with the clock the run is already at", () => { h.execution.state = ExecutionState.Running; - h.execution.duration = 42_000; + h.durationTicks.next(42_000); build(formViewWorkflow).ngOnInit(); @@ -1804,24 +1805,16 @@ describe("WorkflowFormComponent", () => { expect(h.executeWorkflowService.killWorkflow).not.toHaveBeenCalled(); }); - it("counts the run clock off the engine's duration event", () => { + // The clock itself lives in ExecuteWorkflowService now -- anchored and ticked there, so a page + // that mounts mid-run gets where the run has got to instead of starting from zero. What is left + // here is that this page shows what the service says. + it("shows the run clock the service reports", () => { build(formViewWorkflow).ngOnInit(); - h.durationEvents.next({ duration: 5000, isRunning: false }); + h.durationTicks.next(5000); expect(component.executionDuration).toBe(5000); }); - - it("ticks the clock a second at a time while a run is going", () => { - vi.useFakeTimers(); - build(formViewWorkflow).ngOnInit(); - - h.durationEvents.next({ duration: 1000, isRunning: true }); - vi.advanceTimersByTime(1000); - vi.useRealTimers(); - - expect(component.executionDuration).toBe(2000); - }); }); describe("showing the chosen results", () => { 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 b037afc511e..601f495c9cf 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 @@ -401,15 +401,15 @@ export class WorkflowFormComponent implements OnInit, OnDestroy { // The run clock, reusing the operator canvas's source outright rather than timing anything // here: the engine is the only thing that knows when the run really began, so a stopwatch // started at the click would drift and would be wrong after a reload. - this.workflowWebsocketService - .subscribeToEvent("ExecutionDurationUpdateEvent") - .pipe( - tap(event => (this.executionDuration = event.duration)), - switchMap(event => (event.isRunning ? timer(1000, 1000) : EMPTY)), - untilDestroyed(this) - ) - .subscribe(() => { - this.executionDuration += 1000; + // From the service, not from the engine's event directly: that event arrives twice in a whole + // run, so a timer hung off it never started for a view that mounted in between -- which is what + // a routed switch between this page and the canvas makes. The service anchors the clock and + // ticks it, and replays the current value to whoever subscribes. + this.executeWorkflowService + .getExecutionDurationStream() + .pipe(untilDestroyed(this)) + .subscribe(duration => { + this.executionDuration = duration; this.cdr.markForCheck(); }); @@ -631,10 +631,6 @@ export class WorkflowFormComponent implements OnInit, OnDestroy { // path there is nothing in flight and this reads the same Uninitialized it started at. const retained = this.executeWorkflowService.getExecutionState(); this.executionState = retained.state; - // Likewise the clock: the duration arrives as a websocket event the backend sends only when - // the run's start or end time changes, so a page that joined mid-run would have counted from - // zero -- or not at all -- until the run ended. - this.executionDuration = this.executeWorkflowService.getExecutionDuration(); // The workflow is shown, not edited, from here: dragging operators around or deleting them // belongs to the operator canvas. Lock now, and keep it locked against anything else that // unlocks the graph (clampEditability). The clamp is dropped when this page is destroyed; diff --git a/frontend/src/app/workspace/component/workflow-form/workflow-form.rendered.spec.ts b/frontend/src/app/workspace/component/workflow-form/workflow-form.rendered.spec.ts index b4e61bfcd50..6858d6f9387 100644 --- a/frontend/src/app/workspace/component/workflow-form/workflow-form.rendered.spec.ts +++ b/frontend/src/app/workspace/component/workflow-form/workflow-form.rendered.spec.ts @@ -221,7 +221,7 @@ describe("WorkflowFormComponent (rendered template)", () => { // The stream carries no current value, so the page reads the state it arrived on top // of from here. Nothing is in flight in these tests. getExecutionState: () => ({ state: ExecutionState.Uninitialized }), - getExecutionDuration: () => 0, + getExecutionDurationStream: () => EMPTY, executeWorkflow: vi.fn(), killWorkflow: vi.fn(), resetExecutionAndWorkers: vi.fn(), diff --git a/frontend/src/app/workspace/component/workflow-form/workflow-form.spec-harness.ts b/frontend/src/app/workspace/component/workflow-form/workflow-form.spec-harness.ts index aa392a69798..a5f51f6c12c 100644 --- a/frontend/src/app/workspace/component/workflow-form/workflow-form.spec-harness.ts +++ b/frontend/src/app/workspace/component/workflow-form/workflow-form.spec-harness.ts @@ -17,7 +17,7 @@ * under the License. */ -import { of, Subject } from "rxjs"; +import { BehaviorSubject, of, Subject } from "rxjs"; import { vi } from "vitest"; import { DefaultView } from "../../../dashboard/type/workflow-metadata.interface"; @@ -74,6 +74,8 @@ export function setupHarness() { // computing-unit connection status, the workflow validity, and the websocket connection. const executionStateStream = new Subject(); const durationEvents = new Subject<{ duration: number; isRunning: boolean }>(); + // Replays, as the service's does: a page that mounts mid-run gets the clock straight away. + const durationTicks = new BehaviorSubject(0); const statusStream = new Subject(); // The picked computing unit (with its accessPrivilege), separate from the connection status. const selectedUnitStream = new Subject(); @@ -290,8 +292,9 @@ export function setupHarness() { // current value, so this is the only way the page can learn a run is already in flight. // Mutable, so a test can put a run in flight before the component is built. getExecutionState: () => execution, - // The clock the backend last reported, for a page that mounted mid-run. - getExecutionDuration: () => execution.duration, + // The run clock, ticked and replayed by the service; `durationEvents` is the tests' handle on + // it, and its current value is what a page mounting mid-run receives on subscribe. + getExecutionDurationStream: () => durationTicks.asObservable(), executeWorkflow: vi.fn(), killWorkflow: vi.fn(), resetExecutionAndWorkers: vi.fn(), @@ -385,6 +388,7 @@ export function setupHarness() { executionStateStream, modificationEnabled, durationEvents, + durationTicks, statusStream, selectedUnitStream, validationStream, diff --git a/frontend/src/app/workspace/service/execute-workflow/execute-workflow.service.spec.ts b/frontend/src/app/workspace/service/execute-workflow/execute-workflow.service.spec.ts index 1f1b4b8eba8..0e4f6674d5d 100644 --- a/frontend/src/app/workspace/service/execute-workflow/execute-workflow.service.spec.ts +++ b/frontend/src/app/workspace/service/execute-workflow/execute-workflow.service.spec.ts @@ -114,6 +114,71 @@ describe("ExecuteWorkflowService", () => { // no current value, so it heard nothing about the run: it showed Run for a workflow that was // running. The lock is reapplied only when the state changes too, so a canvas that unlocked the // graph on arrival left a running workflow editable until the run happened to end. + // The backend sends ExecutionDurationUpdateEvent exactly twice in a run: once just after + // startTimeStamp is written, carrying a few milliseconds, and once when endTimeStamp is. So the + // reported number stands still for the whole run, and a view that mounted in between and hung a + // timer off that event never received one and never started counting. The clock is anchored and + // ticked here instead, and replayed to whoever subscribes. + describe("the run clock", () => { + const emitDuration = (duration: number, isRunning: boolean) => + emitWsEvent({ type: "ExecutionDurationUpdateEvent", duration, isRunning } as TexeraWebsocketEvent); + + it("counts on from the reported value while the run is going, rather than repeating it", () => { + vi.useFakeTimers(); + try { + // What the backend actually sends at the start of a run: a handful of milliseconds. + emitDuration(3, true); + vi.advanceTimersByTime(60_000); + + // A view mounting a minute in asks and is told a minute, not 3ms. + expect(service.getExecutionDuration()).toBeGreaterThanOrEqual(60_000); + } finally { + vi.useRealTimers(); + } + }); + + it("hands a view that subscribes mid-run the clock straight away, then ticks it", () => { + vi.useFakeTimers(); + try { + emitDuration(0, true); + vi.advanceTimersByTime(30_000); + + const seen: number[] = []; + service.getExecutionDurationStream().subscribe(d => seen.push(d)); + expect(seen[0]).toBeGreaterThanOrEqual(30_000); + + vi.advanceTimersByTime(1000); + expect(seen[seen.length - 1]).toBeGreaterThanOrEqual(31_000); + } finally { + vi.useRealTimers(); + } + }); + + it("stops counting once the run ends, and reports what the run took", () => { + vi.useFakeTimers(); + try { + emitDuration(0, true); + vi.advanceTimersByTime(10_000); + // The second and last event: the real total. + emitDuration(12_345, false); + vi.advanceTimersByTime(10_000); + + expect(service.getExecutionDuration()).toBe(12_345); + } finally { + vi.useRealTimers(); + } + }); + + // Otherwise the next workflow's menu opens showing the previous run's time. + it("goes back to zero when the execution state is reset", () => { + emitDuration(9999, false); + + service.resetExecutionState(); + + expect(service.getExecutionDuration()).toBe(0); + }); + }); + // A view handed a session mid-run needs the lock the run implies; the lock is otherwise only // reapplied when the state changes, so a canvas that unlocked on arrival left a running workflow // editable until its run happened to end. diff --git a/frontend/src/app/workspace/service/execute-workflow/execute-workflow.service.ts b/frontend/src/app/workspace/service/execute-workflow/execute-workflow.service.ts index 259900a4852..1e518368de4 100644 --- a/frontend/src/app/workspace/service/execute-workflow/execute-workflow.service.ts +++ b/frontend/src/app/workspace/service/execute-workflow/execute-workflow.service.ts @@ -18,7 +18,7 @@ */ import { Inject, Injectable, DOCUMENT } from "@angular/core"; -import { Observable, Subject } from "rxjs"; +import { BehaviorSubject, interval, Observable, Subject, Subscription } from "rxjs"; import { WorkflowActionService } from "../workflow-graph/model/workflow-action.service"; import { WorkflowGraphReadonly } from "../workflow-graph/model/workflow-graph"; import { @@ -95,8 +95,21 @@ export class ExecuteWorkflowService { // TODO: move this to another service, or redesign how this // information is stored on the frontend. private assignedWorkerIds: Map = new Map(); - /** The last duration the backend reported for the current run; see getExecutionDuration. */ - private executionDuration = 0; + /** + * The clock for the current run, as an anchor rather than a snapshot: the backend sends + * `ExecutionDurationUpdateEvent` exactly twice per run -- once just after `startTimeStamp` is + * written, carrying a few milliseconds, and once when `endTimeStamp` is -- so the reported number + * stands still for the whole run and only the local tick advances it. Anchored, any view can work + * out how long the run has been going whenever it mounts. + */ + private durationAnchor: { reported: number; at: number; isRunning: boolean } = { + reported: 0, + at: 0, + isRunning: false, + }; + /** Ticks while a run is going; replays the current value to a view that mounts mid-run. */ + private readonly executionDuration = new BehaviorSubject(0); + private durationTicker?: Subscription; constructor( private workflowActionService: WorkflowActionService, @@ -120,10 +133,7 @@ export class ExecuteWorkflowService { this.assignedWorkerIds.set(event.operatorId, event.workerIds); break; case "ExecutionDurationUpdateEvent": - // Held for views that mount mid-run. The backend sends this only when the run's start or - // end time changes, so a view created afterwards -- which is what a routed switch between - // a workflow's two views makes -- never hears it and would count from zero. - this.executionDuration = event.duration; + this.anchorDuration(event.duration, event.isRunning); break; default: // workflow status related event @@ -202,9 +212,28 @@ export class ExecuteWorkflowService { return this.currentState; } - /** How long the current run has been going, for a view that mounted after it started. */ + /** How long the current run has been going, worked out from the anchor. */ public getExecutionDuration(): number { - return this.executionDuration; + const { reported, at, isRunning } = this.durationAnchor; + return isRunning ? reported + (Date.now() - at) : reported; + } + + /** + * The run clock, ticking while a run is going. Both views read it from here rather than each + * running their own timer off the backend event: that timer sat downstream of the event, so a + * view that mounted after the run started never received one and never began counting. + */ + public getExecutionDurationStream(): Observable { + return this.executionDuration.asObservable(); + } + + private anchorDuration(reported: number, isRunning: boolean): void { + this.durationAnchor = { reported, at: Date.now(), isRunning }; + this.durationTicker?.unsubscribe(); + this.durationTicker = isRunning + ? interval(1000).subscribe(() => this.executionDuration.next(this.getExecutionDuration())) + : undefined; + this.executionDuration.next(this.getExecutionDuration()); } /** @@ -415,6 +444,8 @@ export class ExecuteWorkflowService { this.currentState = { state: ExecutionState.Uninitialized, }; + // Otherwise the next workflow's menu opens showing the previous run's time. + this.anchorDuration(0, false); } /** From 7f2aceba27fd3ea635841aa58fbfc1f3abc4c105 Mon Sep 17 00:00:00 2001 From: Yang Zhang Date: Mon, 21 Sep 2026 14:11:59 -0700 Subject: [PATCH 09/13] fix(frontend): stop the run clock on the path leaving the workspace takes `resetExecutionAndWorkers` does not go through `resetExecutionState` -- it calls `updateExecutionState` directly -- so the clock added in the previous commit was not reset by it, and that is the path leaving the workspace takes. A completed workflow left its final time behind for the next one, and a workflow left while still running left the ticker going, so the next one opened on a clock that was still counting up. I claimed in review that this path went through the reset. It does not; I had not checked. Found by @copilot. Covered by a test that leaves a running clock through this path and finds it stopped at zero, and deletion-checked. Full frontend suite: 224 files, 6160 passed, 1 skipped, 0 failed. AOT build, eslint and prettier clean. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FVvP3ttj22f9LB4p9u2anY --- .../execute-workflow.service.spec.ts | 19 +++++++++++++++++++ .../execute-workflow.service.ts | 4 ++++ 2 files changed, 23 insertions(+) diff --git a/frontend/src/app/workspace/service/execute-workflow/execute-workflow.service.spec.ts b/frontend/src/app/workspace/service/execute-workflow/execute-workflow.service.spec.ts index 0e4f6674d5d..b3bdbc1ec74 100644 --- a/frontend/src/app/workspace/service/execute-workflow/execute-workflow.service.spec.ts +++ b/frontend/src/app/workspace/service/execute-workflow/execute-workflow.service.spec.ts @@ -177,6 +177,25 @@ describe("ExecuteWorkflowService", () => { expect(service.getExecutionDuration()).toBe(0); }); + + // Leaving the workspace takes this path, not resetExecutionState, and a run still going when + // it is taken would otherwise carry on counting into the next workflow. + it("stops a running clock when the execution and workers are reset", () => { + vi.useFakeTimers(); + try { + emitDuration(0, true); + vi.advanceTimersByTime(20_000); + + service.resetExecutionAndWorkers(); + const atReset = service.getExecutionDuration(); + vi.advanceTimersByTime(20_000); + + expect(atReset).toBe(0); + expect(service.getExecutionDuration()).toBe(0); + } finally { + vi.useRealTimers(); + } + }); }); // A view handed a session mid-run needs the lock the run implies; the lock is otherwise only diff --git a/frontend/src/app/workspace/service/execute-workflow/execute-workflow.service.ts b/frontend/src/app/workspace/service/execute-workflow/execute-workflow.service.ts index 1e518368de4..c3633721d70 100644 --- a/frontend/src/app/workspace/service/execute-workflow/execute-workflow.service.ts +++ b/frontend/src/app/workspace/service/execute-workflow/execute-workflow.service.ts @@ -456,6 +456,10 @@ export class ExecuteWorkflowService { public resetExecutionAndWorkers(): void { this.updateExecutionState({ state: ExecutionState.Uninitialized }); this.assignedWorkerIds.clear(); + // Leaving the workspace takes this path, not resetExecutionState, so the clock has to be + // stopped here as well: otherwise the next workflow opens showing the last run's time, and + // if that run was still going, showing it still counting up. + this.anchorDuration(0, false); } private updateExecutionState(stateInfo: ExecutionStateInfo): void { From 170f2c936b64dafe14744f91fabb342a667f5c97 Mon Sep 17 00:00:00 2001 From: Yang Zhang Date: Mon, 21 Sep 2026 23:10:20 -0700 Subject: [PATCH 10/13] fix(frontend): key both sides of the hand-over on the room the document is in Three from review on #8581. The two sides of the hand-over asked different questions about the same thing. The departing view keyed `isLeavingWorkspace` on the metadata's wid; the arriving view keyed `hasWorkflowOpen` on the shared document's room. A workflow created in this session -- "create new" seeds a document in a private uuid room, and the first autosave then gives the metadata an id while the document stays put -- was handed over by one side and declined by the other, so the canvas kept the session and the Form View destroyed it and reloaded, losing the undo history. Both key on the room now, through `getOpenWorkflowId`, and `hasWorkflowOpen` no longer matches on a falsy id. Such a workflow is simply rebuilt on its first switch, which is what happens today. The alternative -- recognising the metadata id on the arriving side -- would have had the Form View go on co-editing in a private room under the workflow's name, invisible to anyone else opening it (@mengw15). `MiniMapComponent` registered three handlers on the main paper and never removed them; the paper stream replays, so a departing mini-map still subscribed while its DOM is on its way out received the arriving paper too and left handlers on it. One bound reference, removed when a new paper arrives and on destroy. Imports orphaned by moving the clock into the service, and the doc on `onClickOpenFormView` that still described a full page load. Deletion-checked: each side back to the metadata id, the falsy guard, and the missing off() each turn exactly one named test red. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01FVvP3ttj22f9LB4p9u2anY --- .../component/menu/menu.component.ts | 11 ++++---- .../mini-map/mini-map.component.spec.ts | 26 +++++++++++++++++++ .../mini-map/mini-map.component.ts | 24 ++++++++++++++--- .../workflow-form.component.spec.ts | 12 +++++++++ .../workflow-form/workflow-form.component.ts | 4 +-- .../workflow-form.spec-harness.ts | 7 +++-- .../component/workspace.component.spec.ts | 17 ++++++++++++ .../component/workspace.component.ts | 2 +- .../model/workflow-action.service.spec.ts | 15 +++++++++++ .../model/workflow-action.service.ts | 19 ++++++++++++-- 10 files changed, 122 insertions(+), 15 deletions(-) diff --git a/frontend/src/app/workspace/component/menu/menu.component.ts b/frontend/src/app/workspace/component/menu/menu.component.ts index 465aea31dcc..231751b8d77 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, switchMap, tap } from "rxjs/operators"; +import { catchError, debounceTime, tap } 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"; @@ -40,7 +40,7 @@ import { FileSaverService } from "../../../dashboard/service/user/file/file-save import { NotificationService } from "src/app/common/service/notification/notification.service"; import { OperatorMenuService } from "../../service/operator-menu/operator-menu.service"; import { CoeditorPresenceService } from "../../service/workflow-graph/model/coeditor-presence.service"; -import { EMPTY, firstValueFrom, of, timer } from "rxjs"; +import { firstValueFrom, of } from "rxjs"; import { NzModalService } from "ng-zorro-antd/modal"; import { ResultExportationComponent } from "../result-exportation/result-exportation.component"; import { ReportGenerationService } from "../../service/report-generation/report-generation.service"; @@ -708,9 +708,10 @@ export class MenuComponent implements OnInit, OnDestroy { } /** - * Open the Form View -- a full page load, not a route: the two views share root-level - * singletons (graph, Yjs shared model), and routing left the old collaboration client - * alive (you appeared as your own coeditor). A fresh document is the clean handover. + * Open the Form View. A route, not a page load: the two views are views of one open workflow, + * and this canvas keeps the session -- the shared document and its room, the computing unit, + * the execution -- for the Form View to attach to (see WorkspaceComponent.ngOnDestroy). A + * writer's edits are saved first; see below for why the order matters. */ public onClickOpenFormView(): void { const wid = this.workflowActionService.getWorkflowMetadata().wid; diff --git a/frontend/src/app/workspace/component/workflow-editor/mini-map/mini-map.component.spec.ts b/frontend/src/app/workspace/component/workflow-editor/mini-map/mini-map.component.spec.ts index 634b93663b1..b0abb159e96 100644 --- a/frontend/src/app/workspace/component/workflow-editor/mini-map/mini-map.component.spec.ts +++ b/frontend/src/app/workspace/component/workflow-editor/mini-map/mini-map.component.spec.ts @@ -69,6 +69,12 @@ class StubPaper { this.handlers[event] = handler; } + off(event: string, handler: () => void): void { + if (this.handlers[event] === handler) { + delete this.handlers[event]; + } + } + scale(): { sx: number; sy: number } { return { sx: this.sx, sy: this.sy }; } @@ -226,6 +232,26 @@ describe("MiniMapComponent", () => { expect(localStorage.getItem("mini-map")).toBe("true"); }); + // The paper stream replays, so a departing mini-map -- still subscribed while its DOM is on + // its way out -- receives the arriving view's paper and registers on it, then is destroyed; + // without the matching off() that paper went on calling into a component that was gone. + it("stops following the main paper on destroy, and the previous one when a new one arrives", () => { + sizeMiniMapContainer(912, 100); + mountWorkflowEditorStub(800, 600, 0, 0); + fixture.detectChanges(); + const first = new StubPaper(); + attachMainPaper(first); + expect(Object.keys(first.handlers).sort()).toEqual(["resize", "scale", "translate"]); + + const second = new StubPaper(); + attachMainPaper(second); + expect(Object.keys(first.handlers)).toEqual([]); + expect(Object.keys(second.handlers).sort()).toEqual(["resize", "scale", "translate"]); + + fixture.destroy(); + expect(Object.keys(second.handlers)).toEqual([]); + }); + it("fits the whole main canvas into the mini-map container", () => { // 912 / (2688 - -960) == 0.25; the height (100) is deliberately different // so a width/height mix-up in the scale formula cannot pass. diff --git a/frontend/src/app/workspace/component/workflow-editor/mini-map/mini-map.component.ts b/frontend/src/app/workspace/component/workflow-editor/mini-map/mini-map.component.ts index 9be00875bca..8c61ae15b46 100644 --- a/frontend/src/app/workspace/component/workflow-editor/mini-map/mini-map.component.ts +++ b/frontend/src/app/workspace/component/workflow-editor/mini-map/mini-map.component.ts @@ -24,6 +24,9 @@ import { MAIN_CANVAS } from "../workflow-editor.component"; import * as joint from "jointjs"; import { JointGraphWrapper } from "../../../service/workflow-graph/model/joint-graph-wrapper"; import { PanelService } from "../../../service/panel/panel.service"; + +/** The main paper's events that move or resize its viewport, which is what the navigator tracks. */ +const MAIN_PAPER_EVENTS = ["translate", "scale", "resize"] as const; import { CdkDrag } from "@angular/cdk/drag-drop"; import { NzSpaceCompactItemDirective } from "ng-zorro-antd/space"; import { NzButtonComponent } from "ng-zorro-antd/button"; @@ -83,11 +86,16 @@ export class MiniMapComponent implements AfterViewInit, OnDestroy { .getMainJointPaperAttachedStream() .pipe(untilDestroyed(this)) .subscribe(mainPaper => { + // The stream replays, so the departing view -- still subscribed while its DOM is on its + // way out -- receives the arriving view's paper too. Whatever this component registered + // on the previous paper comes off before it registers on the next, and off again on + // destroy, so no paper is left calling into a component that is gone. + this.stopFollowingMainPaper(); this.paper = mainPaper; this.updateNavigator(); - mainPaper.on("translate", () => this.updateNavigator()); - mainPaper.on("scale", () => this.updateNavigator()); - mainPaper.on("resize", () => this.updateNavigator()); + for (const event of MAIN_PAPER_EVENTS) { + mainPaper.on(event, this.followMainPaper); + } }); this.hidden = JSON.parse(localStorage.getItem("mini-map") as string) || false; @@ -111,9 +119,19 @@ export class MiniMapComponent implements AfterViewInit, OnDestroy { // goes on listening to that graph from a detached node, and once the switch between a // workflow's two views routes, one is left behind on every switch (issue #8582). this.ownPaper?.remove(); + this.stopFollowingMainPaper(); this.rememberVisibility(); } + /** One bound reference, so what was registered on the main paper is what can be removed. */ + private readonly followMainPaper = (): void => this.updateNavigator(); + + private stopFollowingMainPaper(): void { + for (const event of MAIN_PAPER_EVENTS) { + this.paper?.off(event, this.followMainPaper); + } + } + private rememberVisibility(): void { localStorage.setItem("mini-map", JSON.stringify(this.hidden)); } 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 ea4a2948540..ee37b3545ff 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 @@ -290,6 +290,18 @@ describe("WorkflowFormComponent", () => { expect(h.workflowResultService.clearResults).not.toHaveBeenCalled(); }); + // Keyed on the room the document is in, not the metadata's id, so that both views answer the + // hand-over question the same way for a workflow created in this session (see the canvas). + it("releases them when the document is in no workflow's room, even bound for this one's canvas", () => { + build(formViewWorkflow).ngOnInit(); + workflowActionService.getOpenWorkflowId.mockReturnValue(undefined); + router.getCurrentNavigation.mockReturnValue({ finalUrl: workspaceCanvasUrl(7) }); + + component.ngOnDestroy(); + + expect(workflowActionService.clearWorkflow).toHaveBeenCalled(); + }); + // Another workflow's canvas is a different workflow: nothing here belongs to it. it("releases them when the destination is a different workflow", () => { build(formViewWorkflow).ngOnInit(); 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 601f495c9cf..a301e0ac197 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 @@ -32,7 +32,7 @@ import { UserIconComponent } from "../../../dashboard/component/user/user-icon/u import { cloneDeep } from "lodash-es"; import { MarkdownService } from "ngx-markdown"; import { asapScheduler, EMPTY, forkJoin, merge, Observable, Subject, timer } from "rxjs"; -import { catchError, concatMap, debounceTime, finalize, observeOn, switchMap, takeUntil, tap } from "rxjs/operators"; +import { catchError, concatMap, debounceTime, finalize, observeOn, takeUntil, tap } from "rxjs/operators"; import { CdkDragDrop, DragDropModule } from "@angular/cdk/drag-drop"; import { isLeavingWorkspace, USER_WORKFLOW, workspaceCanvasUrl } from "../../../app-routing.constant"; @@ -2048,7 +2048,7 @@ export class WorkflowFormComponent implements OnInit, OnDestroy { this.persistQueue.complete(); // Kept when this workflow's own operator canvas is taking over: that is a hand-over, not a // departure, and rebuilding all of it on the other side is the cost this avoids. - if (isLeavingWorkspace(this.router, this.workflowActionService.getWorkflowMetadata().wid)) { + if (isLeavingWorkspace(this.router, this.workflowActionService.getOpenWorkflowId())) { this.workflowActionService.clearWorkflow(); this.computingUnitStatusService.disconnect(); this.executeWorkflowService.resetExecutionAndWorkers(); diff --git a/frontend/src/app/workspace/component/workflow-form/workflow-form.spec-harness.ts b/frontend/src/app/workspace/component/workflow-form/workflow-form.spec-harness.ts index a5f51f6c12c..fa449d1893d 100644 --- a/frontend/src/app/workspace/component/workflow-form/workflow-form.spec-harness.ts +++ b/frontend/src/app/workspace/component/workflow-form/workflow-form.spec-harness.ts @@ -143,11 +143,14 @@ export function setupHarness() { // As the real one does: the metadata it already holds, re-announced on the same stream. republishWorkflowMetadata: vi.fn(() => workflowMetaDataChangedStream.next(undefined)), getWorkflow: vi.fn().mockReturnValue({ wid: 7, content: { operators: [], operatorPositions: {} } }), - // Carries the wid, as the real metadata does once a workflow is open: it is what tells the - // page, on the way out, whether the navigation is leaving this workflow or handing it over. + // Carries the wid, as the real metadata does once a workflow is open. getWorkflowMetadata: () => ({ wid: 7, name: "scGPT", lastModifiedTime: 1767225600000 }), // Off by default: most specs open a workflow that is not already live, and so load it. hasWorkflowOpen: vi.fn().mockReturnValue(false), + // The room the shared document is in: what tells the page, on the way out, whether the + // navigation is leaving this workflow or handing it over. Matches the metadata's wid above, + // as it does for a workflow that was loaded rather than created in this session. + getOpenWorkflowId: vi.fn().mockReturnValue(7), setWorkflowName: vi.fn(), setWorkflowMetadata: vi.fn(), setHighlightingEnabled: vi.fn(), diff --git a/frontend/src/app/workspace/component/workspace.component.spec.ts b/frontend/src/app/workspace/component/workspace.component.spec.ts index 0594d8e2899..94d7d50b286 100644 --- a/frontend/src/app/workspace/component/workspace.component.spec.ts +++ b/frontend/src/app/workspace/component/workspace.component.spec.ts @@ -116,6 +116,8 @@ describe("WorkspaceComponent", () => { getWorkflowMetadata: vi.fn().mockReturnValue({ wid: 42, readonly: false }), // Off by default: most specs open a workflow that is not already live, and so load it. hasWorkflowOpen: vi.fn().mockReturnValue(false), + // The room the shared document is in; the hand-over on the way out is keyed on this. + getOpenWorkflowId: vi.fn().mockReturnValue(42), workflowChanged: vi.fn().mockReturnValue(EMPTY), workflowMetaDataChanged: vi.fn().mockReturnValue(metadataChangedSubject.asObservable()), // As the real one does: the metadata it already holds, re-announced on the same stream. @@ -608,6 +610,21 @@ describe("WorkspaceComponent", () => { expect(workflowPersistService.persistWorkflow).not.toHaveBeenCalled(); }); + // A workflow created in this session has an id in its metadata after the first autosave, but + // its shared document stayed in the private room it was seeded with. Keyed on the metadata, + // this side handed such a workflow over while the arriving side, keyed on the room, declined + // it and reloaded -- so both key on the room, and this one is rebuilt on its first switch. + it("tears it down when the document is in no workflow's room, even bound for this one's form", async () => { + await createFixture(); + fixture.detectChanges(); + workflowActionService.getOpenWorkflowId.mockReturnValue(undefined); + routerMock.getCurrentNavigation.mockReturnValue({ finalUrl: workspaceFormUrl(42) }); + + component.ngOnDestroy(); + + expect(workflowActionService.clearWorkflow).toHaveBeenCalled(); + }); + it("tears it down when the destination is another workflow's Form View", async () => { await createFixture(); fixture.detectChanges(); diff --git a/frontend/src/app/workspace/component/workspace.component.ts b/frontend/src/app/workspace/component/workspace.component.ts index 118ad76fb6a..082e01ba4c0 100644 --- a/frontend/src/app/workspace/component/workspace.component.ts +++ b/frontend/src/app/workspace/component/workspace.component.ts @@ -214,7 +214,7 @@ export class WorkspaceComponent implements AfterViewInit, OnInit, OnDestroy { // re-entered workflow starts clean instead of reusing the previous one -- unless the Form // View of this same workflow is taking over, in which case the session is handed to it // rather than rebuilt, which is what used to make the switch take seconds. - if (isLeavingWorkspace(this.router, this.workflowActionService.getWorkflowMetadata().wid)) { + if (isLeavingWorkspace(this.router, this.workflowActionService.getOpenWorkflowId())) { this.workflowActionService.clearWorkflow(); this.computingUnitStatusService.disconnect(); this.resetWorkflowSessionState(); diff --git a/frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.spec.ts b/frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.spec.ts index 97768e0f261..d02862d1e4b 100644 --- a/frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.spec.ts +++ b/frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.spec.ts @@ -90,6 +90,21 @@ describe("WorkflowActionService", () => { service.setNewSharedModel(); expect(service.hasWorkflowOpen(42)).toBe(false); + // And asking about "no workflow" is never a match, even against a document in no room. + expect(service.hasWorkflowOpen(undefined)).toBe(false); + expect(service.hasWorkflowOpen(0)).toBe(false); + }); + + // Both views key the hand-over on this. A workflow created in this session has an id in its + // metadata after the first autosave while its document is still in the private room it was + // seeded with; keyed on the metadata the departing view handed it over, keyed on the room the + // arriving view declined it. Keyed on the room on both sides, it is simply rebuilt once. + it("names the room the document is in, and nothing while it is in no workflow's room", () => { + service.setNewSharedModel(42); + expect(service.getOpenWorkflowId()).toBe(42); + + service.setNewSharedModel(); + expect(service.getOpenWorkflowId()).toBeUndefined(); }); // Not local to this method: destroying the document keeps the object and its wid, and what diff --git a/frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.ts b/frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.ts index 8de265227a2..14168a9c03e 100644 --- a/frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.ts +++ b/frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.ts @@ -644,8 +644,23 @@ export class WorkflowActionService { * which seeds a fresh model with no `wid`. Were that to stop happening, a canvas re-entered from * the dashboard would attach to a destroyed document instead of loading. Pinned by a test. */ - public hasWorkflowOpen(workflowId: number): boolean { - return this.texeraGraph.sharedModel.wid === workflowId; + public hasWorkflowOpen(workflowId: number | undefined): boolean { + return !!workflowId && this.texeraGraph.sharedModel.wid === workflowId; + } + + /** + * The workflow whose co-editing room the shared document is in, or undefined when it is in none: + * a brand-new canvas, or a workflow created in this session, whose first autosave gave the + * metadata an id while the document stayed in the private room it was seeded with. + * + * This, and not the metadata's id, is what both views key the hand-over on. The departing view + * asks whether it is leaving this workflow; the arriving view asks whether this workflow is + * already open. Keyed on different ids, the two answered differently for a workflow created in + * this session -- the canvas kept the session, the Form View declined it and reloaded -- so both + * ask about the room, and such a workflow is simply rebuilt on its first switch, as it is today. + */ + public getOpenWorkflowId(): number | undefined { + return this.texeraGraph.sharedModel.wid || undefined; } /** From 69472c774c7c11d73ec78e0748b5ad5710fd8707 Mon Sep 17 00:00:00 2001 From: Yang Zhang Date: Tue, 22 Sep 2026 10:51:29 -0700 Subject: [PATCH 11/13] fix(frontend): audit what a view arriving on a live session has to be told, and tell it Review kept finding the same defect in different places: a view created by the switch subscribes to a stream that carries no current value, starts a field at its default, and waits for an event that does not follow a hand-over. Rather than keep finding them one at a time, every subscription in the components both views create (151 across the two views, the menu, the computing-unit picker, the editor, the mini-map, the result panel and the property panel) was checked against one question -- if this mounts after the state it shows exists, does it read that state? -- and every ngOnDestroy against another: does it undo what its mount created, and only what its mount created? The table is in the PR description. Most state already lives behind a BehaviorSubject, or on the shared JointJS model, and needed nothing. Six did not: - The zoom ratio lives in the root-provided wrapper while a new paper starts at scale 1; disagreeing, the first "zoom in" after a switch stepped from the wrapper's ratio and could shrink the canvas. The paper adopts the wrapper's ratio on mount. - The rendering context keeps a static reference to the attached paper; it is detached on destroy so a context exit cannot update a removed paper. A no-op once a newer paper has attached, the usual order. - The result panel re-rendered only on events, none of which follow a hand-over, so it came up empty beside an operator still shown selected. It renders once on init. - The property panel restored its saved placement through a document-wide `#right-container` lookup; both views mount it, and they overlap for a tick, so the placement could land on the departing view's. Own host now, as the editor and mini-map already do. - The menu resets the export flags on destroy, right on leaving the workspace and wrong on a hand-over where the results are kept; the arriving menu read `false` for results still there. It asks the service to recompute on mount. `resetFlags` also replaced the BehaviorSubject with a fresh one, orphaning subscribers; it emits now. - The editor reset the heat-map view on destroy so a re-entered workspace started with the overlay off. It is destroyed on every hand-over, after the arriving menu has restored the persisted overlay (#8552, which this rebases onto), so the reset switched the overlay off again on every switch. The reset moved to the two views' own teardown, beside the metrics it belongs with, where it runs only on a real departure. Deletion-checked, each turning exactly one named test red. Two of the new tests needed care to discriminate at all: jsdom's getElementById returns elements in the order their ids were registered, not tree order, so the decoy has to be in the document before the component is created; and the same change-detection pass that runs ngOnInit applies the template's width binding, so the restored placement is probed through `left`. Full frontend suite: 224 files, 6171 passed before the rebase; see the PR for the count after. AOT build, eslint and prettier clean. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01FVvP3ttj22f9LB4p9u2anY --- .../component/menu/menu.component.spec.ts | 15 +++++ .../component/menu/menu.component.ts | 5 +- .../property-editor.component.spec.ts | 28 ++++++++- .../property-editor.component.ts | 18 ++++-- .../result-panel.component.spec.ts | 13 +++++ .../result-panel/result-panel.component.ts | 5 ++ .../workflow-editor.component.spec.ts | 57 ++++++++++++++++--- .../workflow-editor.component.ts | 22 +++++-- .../workflow-form.component.spec.ts | 17 ++++++ .../workflow-form/workflow-form.component.ts | 3 + .../workflow-form.rendered.spec.ts | 4 ++ .../workflow-form.spec-harness.ts | 16 ++++-- .../component/workspace.component.spec.ts | 20 +++++++ .../component/workspace.component.ts | 4 ++ .../model/joint-graph-wrapper.ts | 17 ++++++ .../workflow-result-export.service.spec.ts | 21 +++++++ .../workflow-result-export.service.ts | 13 ++++- 17 files changed, 250 insertions(+), 28 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 d7fbc0db6f3..eef2c127633 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 { WorkflowResultExportService } from "../../service/workflow-result-export/workflow-result-export.service"; import { MenuComponent } from "./menu.component"; import { WorkflowWebsocketService } from "../../service/workflow-websocket/workflow-websocket.service"; @@ -832,6 +833,20 @@ describe("MenuComponent", () => { }); }); + // The export flags are reset when a menu is destroyed -- right on leaving the workspace, wrong on + // a hand-over between a workflow's two views, where the results are kept. A menu mounting on + // retained results asks for them to be recomputed rather than offering a dead button. + it("asks the export service to recompute its flags when it mounts", () => { + const exportService = TestBed.inject(WorkflowResultExportService); + const refresh = vi.spyOn(exportService, "refreshExportAvailability"); + + const fresh = TestBed.createComponent(MenuComponent); + fresh.detectChanges(); + + expect(refresh).toHaveBeenCalledTimes(1); + fresh.destroy(); + }); + describe("version history", () => { it("onClickGetAllVersions delegates to workflowVersionService.displayWorkflowVersions", () => { const displaySpy = vi.spyOn(workflowVersionService, "displayWorkflowVersions").mockImplementation(() => {}); diff --git a/frontend/src/app/workspace/component/menu/menu.component.ts b/frontend/src/app/workspace/component/menu/menu.component.ts index 231751b8d77..f0983cbf233 100644 --- a/frontend/src/app/workspace/component/menu/menu.component.ts +++ b/frontend/src/app/workspace/component/menu/menu.component.ts @@ -224,7 +224,10 @@ export class MenuComponent implements OnInit, OnDestroy { public ngOnInit(): void { this.restorePersistedHeatmapOverlay(); - + // The export flags are reset when a menu is destroyed, which is right when the workspace is + // left and wrong when a workflow's two views hand over and the results are kept. Recompute + // from what is in hand, so a menu arriving on retained results does not offer a dead button. + this.workflowResultExportService.refreshExportAvailability(); // Marks an edit for the Form View hand-over (see onClickOpenFormView): set the moment an edit is // reported, before the autosave debounce, cleared when the switch's save snapshots the workflow. this.workflowActionService diff --git a/frontend/src/app/workspace/component/property-editor/property-editor.component.spec.ts b/frontend/src/app/workspace/component/property-editor/property-editor.component.spec.ts index c0a805c720b..8faca7d0c0d 100644 --- a/frontend/src/app/workspace/component/property-editor/property-editor.component.spec.ts +++ b/frontend/src/app/workspace/component/property-editor/property-editor.component.spec.ts @@ -108,6 +108,31 @@ describe("PropertyEditorComponent", () => { expect(container.style.width).toBe("321px"); }); + // Both views of a workflow mount this panel, and they overlap for a tick when the switch routes + // between them; a document-wide lookup then finds the departing view's container first and + // restores the saved placement onto it, leaving this panel's own unstyled. + it("restores the saved placement onto its own container, not the first one in the document", () => { + // `left`, not width: the template binds the width (nz-resizable) on the same change-detection + // pass that runs ngOnInit, which would overwrite the restored value and hide the outcome. + localStorage.setItem("right-panel-style", "left: 33px;"); + // In the document before the panel is created, as the departing view's container is when the + // arriving one initialises. + const decoy = document.createElement("div"); + decoy.id = "right-container"; + document.body.insertBefore(decoy, document.body.firstChild); + try { + const arriving = TestBed.createComponent(PropertyEditorComponent); + arriving.detectChanges(); + const own = arriving.nativeElement.querySelector("#right-container") as HTMLElement; + + expect(own.style.left).toBe("33px"); + expect(decoy.style.left).toBe(""); + arriving.destroy(); + } finally { + decoy.remove(); + } + }); + // The crash this flag fixes: ngOnInit reads #right-container to restore the docked panel's // placement, and that element only exists in the canvas layout. The Form View mounts the panel // with persistPlacement=false, where the element is absent -- reading it there would throw. With @@ -427,7 +452,8 @@ describe("PropertyEditorComponent", () => { localStorage.removeItem("right-panel-style"); component.width = 137; component.height = 246; - vi.spyOn(document, "getElementById").mockReturnValue(null); + // The panel looks for the container inside its own host, so "missing" means gone from there. + (fixture.nativeElement.querySelector("#right-container") as HTMLElement).remove(); component.ngOnDestroy(); diff --git a/frontend/src/app/workspace/component/property-editor/property-editor.component.ts b/frontend/src/app/workspace/component/property-editor/property-editor.component.ts index 63b97a71c7a..af9312f69ff 100644 --- a/frontend/src/app/workspace/component/property-editor/property-editor.component.ts +++ b/frontend/src/app/workspace/component/property-editor/property-editor.component.ts @@ -134,7 +134,8 @@ export class PropertyEditorComponent implements OnInit, OnDestroy, OnChanges { private changeDetectorRef: ChangeDetectorRef, private panelService: PanelService, private formBindingService: FormBindingService, - private config: GuiConfigService + private config: GuiConfigService, + private elementRef: ElementRef ) { const width = localStorage.getItem("right-panel-width"); if (width) this.width = Number(width); @@ -161,10 +162,13 @@ export class PropertyEditorComponent implements OnInit, OnDestroy, OnChanges { // canvas layout. The Form View mounts this panel with persistPlacement=false, where that element // is absent, so skip the restore there (it would throw on the missing element). if (this.persistPlacement) { + // This panel's own container, not whichever the document holds first: both views of a + // workflow mount this panel, and they overlap for a tick when the switch routes between + // them, so a document-wide lookup could restore the placement onto the departing view's. + const container = this.rightContainer()!; const style = localStorage.getItem("right-panel-style"); - if (style) document.getElementById("right-container")!.style.cssText = style; - const translates = document.getElementById("right-container")!.style.transform; - const [xOffset, yOffset, _] = calculateTotalTranslate3d(translates); + if (style) container.style.cssText = style; + const [xOffset, yOffset, _] = calculateTotalTranslate3d(container.style.transform); this.returnPosition = { x: -xOffset, y: -yOffset }; } this.registerHighlightEventsHandler(); @@ -241,13 +245,17 @@ export class PropertyEditorComponent implements OnInit, OnDestroy, OnChanges { localStorage.setItem("right-panel-width", String(this.width)); localStorage.setItem("right-panel-height", String(this.height)); - const rightContainer = document.getElementById("right-container"); + const rightContainer = this.rightContainer(); if (rightContainer) { localStorage.setItem("right-panel-style", rightContainer.style.cssText); } } } + private rightContainer(): HTMLElement | null { + return (this.elementRef.nativeElement as HTMLElement).querySelector("#right-container"); + } + /** * This method changes the property editor according to how operators are highlighted on the workflow editor. * diff --git a/frontend/src/app/workspace/component/result-panel/result-panel.component.spec.ts b/frontend/src/app/workspace/component/result-panel/result-panel.component.spec.ts index 339c772b521..abe5fe1eb2a 100644 --- a/frontend/src/app/workspace/component/result-panel/result-panel.component.spec.ts +++ b/frontend/src/app/workspace/component/result-panel/result-panel.component.spec.ts @@ -113,6 +113,19 @@ describe("ResultPanelComponent", () => { it("should create", () => expect(component).toBeTruthy()); + // Every other re-render waits for an event, and none follows a hand-over between a workflow's + // two views: the highlight and the results are kept in root-provided services, so a panel + // mounted on top of them would come up empty beside an operator still shown selected. + it("renders what is already there once on init, without waiting for an event", () => { + const rerender = vi.spyOn(ResultPanelComponent.prototype, "rerenderResultPanel"); + + const fresh = TestBed.createComponent(ResultPanelComponent); + fresh.detectChanges(); + + expect(rerender).toHaveBeenCalledTimes(1); + fresh.destroy(); + }); + it("should show nothing by default", () => { expect(component.frameComponentConfigs.size).toBe(0); }); diff --git a/frontend/src/app/workspace/component/result-panel/result-panel.component.ts b/frontend/src/app/workspace/component/result-panel/result-panel.component.ts index d548bae4c77..88c29eed760 100644 --- a/frontend/src/app/workspace/component/result-panel/result-panel.component.ts +++ b/frontend/src/app/workspace/component/result-panel/result-panel.component.ts @@ -148,6 +148,11 @@ export class ResultPanelComponent implements OnInit, OnDestroy { this.closePanel(); } }); + // Once, for what is already there. Every re-render above waits for an event -- a state change, + // a highlight, a result arriving -- and none of those follows a hand-over between a workflow's + // two views: the highlight and the results are kept in root-provided services, so this panel + // would come up empty beside an operator that is still shown selected, until the next event. + this.rerenderResultPanel(); } @HostListener("window:beforeunload") diff --git a/frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.spec.ts b/frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.spec.ts index d07669a02cd..ed2d3141247 100644 --- a/frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.spec.ts +++ b/frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.spec.ts @@ -123,17 +123,20 @@ describe("WorkflowEditorComponent", () => { expect(component).toBeTruthy(); }); - it("should reset the heat-map view on destroy so a re-entered workspace starts with the overlay off", () => { - // The wrapper is root-provided and outlives the editor, while the menu's - // checkbox re-initializes to off on every workspace entry; without the - // reset the stale view repaints no-data colors and the first checkbox - // click re-publishes the view instead of clearing it. + // It used to reset the view on destroy so a re-entered workspace started with the overlay off. + // This component is now destroyed on every hand-over between a workflow's two views, after the + // arriving view has mounted and restored the persisted overlay (#8552), so a reset here switched + // it off again on every switch. Leaving the workspace resets it, in the views' own teardown. + it("leaves the heat-map view alone on destroy; leaving the workspace is what resets it", () => { const wrapper = TestBed.inject(WorkflowActionService).getJointGraphWrapper(); wrapper.setHeatmapView(HeatmapView.Runtime); + try { + fixture.destroy(); - fixture.destroy(); - - expect(wrapper.getHeatmapView()).toBeNull(); + expect(wrapper.getHeatmapView()).toBe(HeatmapView.Runtime); + } finally { + wrapper.setHeatmapView(null); + } }); // Two of these editors are in the page at once for one tick when the two views of a workflow @@ -183,6 +186,44 @@ describe("WorkflowEditorComponent", () => { expect(handler).not.toHaveBeenCalled(); }); + // The zoom ratio lives in the root-provided wrapper and outlives this component, while a new + // paper starts at scale 1. Once the switch between a workflow's two views routes, the paper is + // new and the ratio is whatever was last chosen; disagreeing, the first "zoom in" after a + // switch stepped from the wrapper's ratio and could shrink the canvas. + it("starts its paper at the wrapper's zoom ratio, so the two agree after a remount", () => { + const wrapper = TestBed.inject(WorkflowActionService).getJointGraphWrapper(); + wrapper.setZoomProperty(0.5); + try { + const other = TestBed.createComponent(WorkflowEditorComponent); + other.detectChanges(); + + expect(other.componentInstance.paper.scale().sx).toBe(0.5); + other.destroy(); + } finally { + wrapper.setZoomProperty(1); + } + }); + + // The context keeps a static reference to the attached paper for async rendering. It must not + // outlive the paper it points at, or a context exit would update the views of a removed one. + // It is a no-op once a newer paper has been attached, which is the usual hand-over order. + it("detaches its paper from the rendering context on destroy, unless a newer one has taken over", () => { + const wrapper = TestBed.inject(WorkflowActionService).getJointGraphWrapper(); + const context = (wrapper as any).jointGraphContext; + const mine = component.paper; + expect(context.jointPaper).toBe(mine); + + fixture.destroy(); + expect(context.jointPaper).toBeUndefined(); + + // A newer paper attached before the older editor goes: the older one leaves it alone. + const newer = TestBed.createComponent(WorkflowEditorComponent); + newer.detectChanges(); + wrapper.detachMainJointPaper(mine); + expect(context.jointPaper).toBe(newer.componentInstance.paper); + newer.destroy(); + }); + it("should hide operator status on the canvas by default", () => { // keeps the Status toggle off until the user enables it const editor = (component as any).editor as HTMLElement; diff --git a/frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.ts b/frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.ts index 0983fe48216..bc5a7f5c6bc 100644 --- a/frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.ts +++ b/frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.ts @@ -217,6 +217,13 @@ export class WorkflowEditorComponent implements OnInit, AfterViewInit, OnDestroy this.editorWrapper = host.querySelector("#workflow-editor-wrapper")!; document.addEventListener("keydown", this.keyboardActionListener); this.initializeJointPaper(); + // A new paper starts at scale 1, but the zoom ratio lives in the root-provided wrapper and + // outlives this component. With the switch between a workflow's two views routed, this paper + // is new while the ratio is whatever the user last chose, and the two disagreed: the zoom + // buttons stepped from the wrapper's ratio, so the first "zoom in" after a switch could shrink + // the canvas. The paper adopts the wrapper's ratio, which also carries the user's zoom across. + const zoom = this.wrapper.getZoomRatio(); + this.paper.scale(zoom, zoom); this.handleDisableJointPaperInteractiveness(); this.handleOperatorValidation(); this.handlePaperRestoreDefaultOffset(); @@ -268,17 +275,20 @@ export class WorkflowEditorComponent implements OnInit, AfterViewInit, OnDestroy // Harmless while every mount followed a page load; the switch between a workflow's two views // routes now, so a mount happens on every switch and the papers pile up. Whether the // undraggable operators recorded in #8580 follow from that is not settled -- #8582 tracks it. + // The context keeps a static reference to the attached paper for async rendering; it must not + // outlive the paper it points at, or a context exit would update the views of a removed one. + this.wrapper.detachMainJointPaper(this.paper); this.paper?.remove(); // The same bound reference that was registered: `.bind()` returns a new function every call, // so removing a freshly bound one never matched and left the listener behind. One stale // listener per mount meant one Ctrl/Cmd-Z undoing several entries after a few switches. document.removeEventListener("keydown", this.keyboardActionListener); - // The overlay belongs to the canvas being viewed, but the wrapper holding - // the view is root-provided and outlives this component, while the menu's - // checkbox re-initializes to off and the metrics behind the overlay are - // cleared on workspace teardown. Reset the view here so all three agree - // when a workspace is re-entered. - this.workflowActionService.getJointGraphWrapper().setHeatmapView(null); + // The heat-map view is deliberately not reset here any more. It used to be, so that a + // re-entered workspace started with the overlay off; but this component is destroyed on every + // hand-over between a workflow's two views, and destroyed after the arriving view has mounted + // and restored the persisted overlay (#8552), so a reset here switched it off again on every + // switch. The reset belongs to leaving the workspace, and lives with the rest of that teardown + // in the two views' ngOnDestroy. } private _handleKeyboardAction(event: any) { 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 ee37b3545ff..752816414f5 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 @@ -302,6 +302,23 @@ describe("WorkflowFormComponent", () => { expect(workflowActionService.clearWorkflow).toHaveBeenCalled(); }); + // The heat-map overlay's view lives in the root-provided wrapper. The editor used to reset it on + // destroy, which the switch turned into "off again on every switch", after the arriving view had + // just restored it (#8552). It goes with the metrics now: reset on leaving, kept on a hand-over. + it("resets the heat-map view on leaving and keeps it on a hand-over", () => { + const setHeatmapView = workflowActionService.getJointGraphWrapper().setHeatmapView; + + build(formViewWorkflow).ngOnInit(); + router.getCurrentNavigation.mockReturnValue({ finalUrl: workspaceCanvasUrl(7) }); + component.ngOnDestroy(); + expect(setHeatmapView).not.toHaveBeenCalled(); + + build(formViewWorkflow).ngOnInit(); + router.getCurrentNavigation.mockReturnValue(null); + component.ngOnDestroy(); + expect(setHeatmapView).toHaveBeenCalledWith(null); + }); + // Another workflow's canvas is a different workflow: nothing here belongs to it. it("releases them when the destination is a different workflow", () => { build(formViewWorkflow).ngOnInit(); 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 a301e0ac197..3a076ef7a7c 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 @@ -2054,6 +2054,9 @@ export class WorkflowFormComponent implements OnInit, OnDestroy { this.executeWorkflowService.resetExecutionAndWorkers(); this.workflowConsoleService.clearConsoleMessages(); this.workflowResultService.clearResults(); + // As the canvas does: the heat-map view goes with the metrics behind it, and only on a + // real departure -- on a hand-over the arriving view has already restored the overlay. + this.workflowActionService.getJointGraphWrapper().setHeatmapView(null); } } } diff --git a/frontend/src/app/workspace/component/workflow-form/workflow-form.rendered.spec.ts b/frontend/src/app/workspace/component/workflow-form/workflow-form.rendered.spec.ts index 6858d6f9387..100205864a1 100644 --- a/frontend/src/app/workspace/component/workflow-form/workflow-form.rendered.spec.ts +++ b/frontend/src/app/workspace/component/workflow-form/workflow-form.rendered.spec.ts @@ -144,6 +144,8 @@ describe("WorkflowFormComponent (rendered template)", () => { provide: WorkflowActionService, useValue: { hasWorkflowOpen: () => false, + // The room the shared document is in: what the page keys the hand-over on, on its way out. + getOpenWorkflowId: () => 7, resetAsNewWorkflow: vi.fn(), setNewSharedModel: vi.fn(), reloadWorkflow: vi.fn(), @@ -175,6 +177,8 @@ describe("WorkflowFormComponent (rendered template)", () => { updateSharedModelAwareness: vi.fn(), }), getJointGraphWrapper: () => ({ + // Reset by the page on leaving the workspace; the rendered tests leave, so it must exist. + setHeatmapView: () => {}, getJointOperatorHighlightStream: () => EMPTY, getJointOperatorUnhighlightStream: () => EMPTY, getCurrentHighlightedOperatorIDs: () => [], diff --git a/frontend/src/app/workspace/component/workflow-form/workflow-form.spec-harness.ts b/frontend/src/app/workspace/component/workflow-form/workflow-form.spec-harness.ts index fa449d1893d..0bce6e34ff9 100644 --- a/frontend/src/app/workspace/component/workflow-form/workflow-form.spec-harness.ts +++ b/frontend/src/app/workspace/component/workflow-form/workflow-form.spec-harness.ts @@ -130,6 +130,14 @@ export function setupHarness() { // The preview centres the embedded graph once it is built; tests assert this fired. const triggerCenterEvent = vi.fn(); + const jointGraphWrapper = { + getJointOperatorHighlightStream: () => highlightStream.asObservable(), + getJointOperatorUnhighlightStream: () => unhighlightStream.asObservable(), + getCurrentHighlightedOperatorIDs: () => highlightedIds, + unhighlightOperators, + // The heat-map overlay's view; reset by the views on leaving the workspace, kept on a hand-over. + setHeatmapView: vi.fn(), + }; const workflowActionService = { resetAsNewWorkflow: vi.fn(), setNewSharedModel: vi.fn(), @@ -181,12 +189,8 @@ export function setupHarness() { })), updateSharedModelAwareness, }), - getJointGraphWrapper: () => ({ - getJointOperatorHighlightStream: () => highlightStream.asObservable(), - getJointOperatorUnhighlightStream: () => unhighlightStream.asObservable(), - getCurrentHighlightedOperatorIDs: () => highlightedIds, - unhighlightOperators, - }), + // One stub object, so a spy on it is the same one a test reads back after the component acts. + getJointGraphWrapper: () => jointGraphWrapper, // Every config write announces on this stream (setFormBinding emits it); the form re-reads its // config on it unless the write is one of its own presentation edits. The form-binding mock's // writers below emit here, as the real service does, so that chain is under test. diff --git a/frontend/src/app/workspace/component/workspace.component.spec.ts b/frontend/src/app/workspace/component/workspace.component.spec.ts index 94d7d50b286..596ba71ff99 100644 --- a/frontend/src/app/workspace/component/workspace.component.spec.ts +++ b/frontend/src/app/workspace/component/workspace.component.spec.ts @@ -118,6 +118,8 @@ describe("WorkspaceComponent", () => { hasWorkflowOpen: vi.fn().mockReturnValue(false), // The room the shared document is in; the hand-over on the way out is keyed on this. getOpenWorkflowId: vi.fn().mockReturnValue(42), + // One stub object, so the spy on it is the same one the assertions read. + getJointGraphWrapper: vi.fn().mockReturnValue({ setHeatmapView: vi.fn() }), workflowChanged: vi.fn().mockReturnValue(EMPTY), workflowMetaDataChanged: vi.fn().mockReturnValue(metadataChangedSubject.asObservable()), // As the real one does: the metadata it already holds, re-announced on the same stream. @@ -625,6 +627,24 @@ describe("WorkspaceComponent", () => { expect(workflowActionService.clearWorkflow).toHaveBeenCalled(); }); + // The heat-map overlay's view lives in the root-provided wrapper. It used to be reset by the + // editor on destroy, which the switch turned into "off again on every switch", after the + // arriving menu had just restored it (#8552). It goes with the metrics now: reset on leaving, + // kept on a hand-over. + it("resets the heat-map view on leaving and keeps it on a hand-over", async () => { + await createFixture(); + fixture.detectChanges(); + const setHeatmapView = workflowActionService.getJointGraphWrapper().setHeatmapView; + + routerMock.getCurrentNavigation.mockReturnValue({ finalUrl: workspaceFormUrl(42) }); + component.ngOnDestroy(); + expect(setHeatmapView).not.toHaveBeenCalled(); + + routerMock.getCurrentNavigation.mockReturnValue(null); + component.ngOnDestroy(); + expect(setHeatmapView).toHaveBeenCalledWith(null); + }); + it("tears it down when the destination is another workflow's Form View", async () => { await createFixture(); fixture.detectChanges(); diff --git a/frontend/src/app/workspace/component/workspace.component.ts b/frontend/src/app/workspace/component/workspace.component.ts index 082e01ba4c0..b53fcc60658 100644 --- a/frontend/src/app/workspace/component/workspace.component.ts +++ b/frontend/src/app/workspace/component/workspace.component.ts @@ -218,6 +218,10 @@ export class WorkspaceComponent implements AfterViewInit, OnInit, OnDestroy { this.workflowActionService.clearWorkflow(); this.computingUnitStatusService.disconnect(); this.resetWorkflowSessionState(); + // The overlay's view lives in the root-provided wrapper; the metrics behind it are cleared + // just above, so the view goes with them. On a hand-over it stays: the arriving view has + // already restored the persisted overlay, and the metrics are kept. + this.workflowActionService.getJointGraphWrapper().setHeatmapView(null); } } diff --git a/frontend/src/app/workspace/service/workflow-graph/model/joint-graph-wrapper.ts b/frontend/src/app/workspace/service/workflow-graph/model/joint-graph-wrapper.ts index 66ddc42cdde..fe43258a115 100644 --- a/frontend/src/app/workspace/service/workflow-graph/model/joint-graph-wrapper.ts +++ b/frontend/src/app/workspace/service/workflow-graph/model/joint-graph-wrapper.ts @@ -212,6 +212,16 @@ export class JointGraphWrapper { return paper; } + /** + * Forget `paper` as the context's attached paper, if it still is. Called by the editor that + * built it, on destroy, before removing it. A no-op when a newer paper has already been attached, + * which is the usual order when the two views of a workflow hand over: the arriving editor + * attaches its paper before the departing one is destroyed. + */ + public detachMainJointPaper(paper: joint.dia.Paper | undefined): void { + this.jointGraphContext.detachPaper(paper); + } + public getMainJointPaper(): joint.dia.Paper { return this.mainPaper; } @@ -900,6 +910,13 @@ export class JointGraphWrapper { this.jointPaper.options.async = this.async(); } + /** Forget `jointPaper` if it is the attached one; `exit()` must never update a removed paper. */ + public static detachPaper(jointPaper: joint.dia.Paper | undefined) { + if (jointPaper !== undefined && this.jointPaper === jointPaper) { + this.jointPaper = undefined; + } + } + protected static enter(context: JointGraphContextType): void { super.enter(context); if (this.jointPaper !== undefined) { diff --git a/frontend/src/app/workspace/service/workflow-result-export/workflow-result-export.service.spec.ts b/frontend/src/app/workspace/service/workflow-result-export/workflow-result-export.service.spec.ts index c07deff0be1..b546e6a1a22 100644 --- a/frontend/src/app/workspace/service/workflow-result-export/workflow-result-export.service.spec.ts +++ b/frontend/src/app/workspace/service/workflow-result-export/workflow-result-export.service.spec.ts @@ -152,6 +152,27 @@ describe("WorkflowResultExportService", () => { expect(service.hasResultToExportOnAllOperators.value).toBe(false); }); + // It used to replace the subject with a fresh one, which silently orphaned whoever was subscribed. + it("resetFlags tells the existing subscribers, rather than replacing the subject under them", () => { + const seen: boolean[] = []; + service.getExportOnAllOperatorsStatusStream().subscribe(v => seen.push(v)); + service.hasResultToExportOnAllOperators.next(true); + + service.resetFlags(); + + expect(seen).toEqual([false, true, false]); + }); + + // A menu arriving on results kept across a hand-over between a workflow's two views asks for + // the flags to be recomputed, since the departing menu reset them on its way out. + it("refreshExportAvailability recomputes the flags from what is in hand", () => { + const recompute = vi.spyOn(service as any, "updateExportAvailabilityFlags"); + + service.refreshExportAvailability(); + + expect(recompute).toHaveBeenCalledTimes(1); + }); + // ---- Test helpers ---------------------------------------------------------- function enableExport(): void { diff --git a/frontend/src/app/workspace/service/workflow-result-export/workflow-result-export.service.ts b/frontend/src/app/workspace/service/workflow-result-export/workflow-result-export.service.ts index ce3a6db6d91..dad95c5d15e 100644 --- a/frontend/src/app/workspace/service/workflow-result-export/workflow-result-export.service.ts +++ b/frontend/src/app/workspace/service/workflow-result-export/workflow-result-export.service.ts @@ -354,7 +354,18 @@ export class WorkflowResultExportService { */ public resetFlags(): void { this.hasResultToExportOnHighlightedOperators = false; - this.hasResultToExportOnAllOperators = new BehaviorSubject(false); + // The same subject, not a fresh one: replacing it silently orphaned whoever was subscribed. + this.hasResultToExportOnAllOperators.next(false); + } + + /** + * Recompute the export flags from what is in hand. The menu resets them on destroy, which is + * right when the workspace is left and its results cleared, and wrong when a workflow's two + * views hand over and the results are kept: the arriving menu would otherwise read `false` for + * results that are still there, until the next execution or result event happened to recompute. + */ + public refreshExportAvailability(): void { + this.updateExportAvailabilityFlags(); } getExportOnAllOperatorsStatusStream(): Observable { From 4160529ad741aff860e682c68949d4c86a2bc8c4 Mon Sep 17 00:00:00 2001 From: Yang Zhang Date: Tue, 22 Sep 2026 12:40:41 -0700 Subject: [PATCH 12/13] fix(frontend): lower the hand-over flag when the route does not go through, and stop describing the switch as a page load Three from review on #8581. `handingOverToFormView` was raised before the save and lowered only when the save failed; the success path handed the navigation's promise to `void`. With `window.location.href` that was free: the document was discarded, and the flag with it. A route can be refused or cancelled and leaves this page in place, so a navigation that did not go through left the flag raised and the Form View button dead for the rest of the session, silently. The flag comes down on anything but success; on success the component is gone (@mengw15, who could not construct the case and filed it as a loose end of the old assumption, which it is). Eight comments across three files still described the switch as a full-page load. Two of them were the ones that explain why the `beforeunload` handlers tear nothing down, and named the switch as the navigation that fires them; it fires them no longer. The handlers are still right -- a refresh, a closed tab or a typed URL still unloads the document -- so the stated case is now those. The other six were the save-before-hand-over rationale, whose premise (a request aborted by the unload) is gone; the ordering is still right, for a different reason: a save that fails should keep the writer on the view they edited in, with the error in front of them, and an edit made during the save should be stored here rather than left to an autosave that would fire under the other view. The reason is what changed (@mengw15). `MAIN_PAPER_EVENTS` was declared between two groups of imports; it sits after the last one now (@copilot). Deletion-checked: discarding the navigation's result again turns both new tests red. Full frontend suite: 229 files, 6241 passed, 1 skipped, 0 failed. AOT build, eslint and prettier clean. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01FVvP3ttj22f9LB4p9u2anY --- .../component/menu/menu.component.spec.ts | 23 +++++++++++++ .../component/menu/menu.component.ts | 34 +++++++++++++------ .../mini-map/mini-map.component.ts | 6 ++-- .../workflow-form/workflow-form.component.ts | 12 ++++--- .../component/workspace.component.ts | 7 ++-- 5 files changed, 60 insertions(+), 22 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 eef2c127633..854822d9553 100644 --- a/frontend/src/app/workspace/component/menu/menu.component.spec.ts +++ b/frontend/src/app/workspace/component/menu/menu.component.spec.ts @@ -140,6 +140,29 @@ describe("MenuComponent", () => { expect(navigateByUrl).toHaveBeenCalledWith(workspaceFormUrl(42)); }); + // A page load left nothing behind; a route can be refused or cancelled and leaves this page in + // place. The hand-over flag then has to come down, or the Form View button is dead for the rest + // of the session with nothing logged. On success the component is destroyed, flag and all. + it("lowers the hand-over flag when the navigation does not go through", async () => { + vi.spyOn(TestBed.inject(Router), "navigateByUrl").mockResolvedValue(false); + (component as any).handingOverToFormView = true; + + (component as any).openFormViewPage(42); + await Promise.resolve(); + + expect((component as any).handingOverToFormView).toBe(false); + }); + + it("lowers the hand-over flag when the navigation fails outright", async () => { + vi.spyOn(TestBed.inject(Router), "navigateByUrl").mockRejectedValue(new Error("refused")); + (component as any).handingOverToFormView = true; + + (component as any).openFormViewPage(42); + await Promise.resolve(); + + expect((component as any).handingOverToFormView).toBe(false); + }); + it("hands over to the id the save assigned when the canvas held a workflow never saved yet", () => { // After "new workflow" the canvas holds the default workflow (wid 0); the switch's save creates // it, and the page to open is the created one, not /workflow/0/form. diff --git a/frontend/src/app/workspace/component/menu/menu.component.ts b/frontend/src/app/workspace/component/menu/menu.component.ts index f0983cbf233..e320fc484e2 100644 --- a/frontend/src/app/workspace/component/menu/menu.component.ts +++ b/frontend/src/app/workspace/component/menu/menu.component.ts @@ -132,7 +132,7 @@ export class MenuComponent implements OnInit, OnDestroy { public isWorkflowValid: boolean = true; // this will check whether the workflow error or not public isWorkflowEmpty: boolean = false; public isSaving: boolean = false; - /** A Form View hand-over is in progress (saving, then a full-page load); a second click is a no-op. */ + /** A Form View hand-over is in progress (saving, then the route); a second click is a no-op. */ private handingOverToFormView = false; /** An edit has been reported since the hand-over's last save snapshot (see onClickOpenFormView). */ private editedSinceSwitchSnapshot = false; @@ -727,18 +727,18 @@ export class MenuComponent implements OnInit, OnDestroy { this.openFormViewPage(wid); return; } - // Save first, and hand over only once the save has completed. The full-page load that - // follows unloads this document, and a request still in flight at that moment is aborted, so - // navigating right after firing the save could lose the very edit the switch is meant to carry - // across; the workspace's beforeunload save runs into the same unload and is no safety net. A - // 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. + // Save first, and hand over only once the save has completed. A route aborts no request, so + // this is no longer about losing the edit in flight; it is about where a failure lands. The + // switch is the moment a writer expects what they typed here to be stored, and a save that + // fails keeps them here, on the view they edited in, with the error in front of them -- rather + // 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 // 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. + // 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. this.handingOverToFormView = true; this.isSaving = true; this.saveThenOpenFormView(wid); @@ -767,7 +767,8 @@ export class MenuComponent implements OnInit, OnDestroy { }, complete: () => { if (this.editedSinceSwitchSnapshot) { - // An edit landed while the save was out; the full-page load would kill its autosave. + // 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; } @@ -784,9 +785,20 @@ export class MenuComponent implements OnInit, OnDestroy { * away everything that made the workflow live -- the shared document, the computing unit * connection, the execution state -- only to rebuild it on the other side. The canvas keeps * the session on its way out (see its ngOnDestroy) and the Form View attaches to it. + * + * A navigation can be refused or cancelled, and unlike a page load that leaves this page in + * place, with the hand-over flag still raised and the Form View button dead for the rest of the + * session. So the flag comes down on anything but success -- on success this component is gone. */ private openFormViewPage(wid: number): void { - void this.router.navigateByUrl(workspaceFormUrl(wid)); + this.router.navigateByUrl(workspaceFormUrl(wid)).then( + navigated => { + if (!navigated) { + this.handingOverToFormView = false; + } + }, + () => (this.handingOverToFormView = false) + ); } /** diff --git a/frontend/src/app/workspace/component/workflow-editor/mini-map/mini-map.component.ts b/frontend/src/app/workspace/component/workflow-editor/mini-map/mini-map.component.ts index 8c61ae15b46..2756a565c87 100644 --- a/frontend/src/app/workspace/component/workflow-editor/mini-map/mini-map.component.ts +++ b/frontend/src/app/workspace/component/workflow-editor/mini-map/mini-map.component.ts @@ -24,9 +24,6 @@ import { MAIN_CANVAS } from "../workflow-editor.component"; import * as joint from "jointjs"; import { JointGraphWrapper } from "../../../service/workflow-graph/model/joint-graph-wrapper"; import { PanelService } from "../../../service/panel/panel.service"; - -/** The main paper's events that move or resize its viewport, which is what the navigator tracks. */ -const MAIN_PAPER_EVENTS = ["translate", "scale", "resize"] as const; import { CdkDrag } from "@angular/cdk/drag-drop"; import { NzSpaceCompactItemDirective } from "ng-zorro-antd/space"; import { NzButtonComponent } from "ng-zorro-antd/button"; @@ -34,6 +31,9 @@ import { NzWaveDirective } from "ng-zorro-antd/core/wave"; import { ɵNzTransitionPatchDirective } from "ng-zorro-antd/core/transition-patch"; import { NzIconDirective } from "ng-zorro-antd/icon"; +/** The main paper's events that move or resize its viewport, which is what the navigator tracks. */ +const MAIN_PAPER_EVENTS = ["translate", "scale", "resize"] as const; + @UntilDestroy() @Component({ selector: "texera-mini-map", 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 3a076ef7a7c..9b06c05273e 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 @@ -241,7 +241,7 @@ export class WorkflowFormComponent implements OnInit, OnDestroy { private afterDrain: Array<() => void> = []; /** An edit has happened since the last snapshot was enqueued. The autosave behind workflowChanged * is debounced, so at the moment the queue drains such an edit may not be queued yet -- and the - * hand-over waiting on the drain would lose it to the full-page load. Set the moment an edit is + * hand-over waiting on the drain would leave it to an autosave firing under the other view. Set the moment an edit is * reported (before the debounce), cleared when a snapshot is enqueued (it carries everything up * to then), and checked by the drain, which flushes one more save instead of handing over. */ private dirtySinceLastEnqueue = false; @@ -479,8 +479,8 @@ export class WorkflowFormComponent implements OnInit, OnDestroy { // Attribute boxes become dropdowns only after compilation writes the column enums into each // operator's dynamic schema -- which lands after these cards were built. Rebuild on the - // compilation-state stream, a ReplaySubject(1) so a late subscriber (this page reloads fresh - // on every Canvas<->Form switch) gets the current state at once. Held, not dropped, while + // compilation-state stream, a ReplaySubject(1) so a late subscriber (this page is created anew + // on every Canvas<->Form switch, mid-session) gets the current state at once. Held, not dropped, while // someone is typing (see rebuildFormOrDefer), so it neither throws away a half-entered value // under the cursor nor goes missing. this.workflowCompilingService @@ -1885,7 +1885,8 @@ export class WorkflowFormComponent implements OnInit, OnDestroy { this.queuedSaves--; if (this.queuedSaves === 0) { // Hand-over is waiting, but an edit arrived after the last snapshot and its debounced - // autosave has not fired yet: the full-page load would kill that edit. Flush it into + // autosave has not fired yet: store it here rather than leave it to fire under the + // other view. Flush it into // the queue first; this drain check runs again once the flush has gone out. (When the // flush cannot be enqueued -- save()'s own guards -- fall through as save() itself // would: there is nothing left this page can store.) @@ -2022,7 +2023,8 @@ export class WorkflowFormComponent implements OnInit, OnDestroy { /** * The browser is leaving this document: save, and change nothing else. * - * The canvas switch is a full-page navigation, and the browser may keep this document in its + * Leaving the document -- a refresh, a closed tab, a URL typed over this one; the canvas switch + * used to be one, and routes now -- fires this, and the browser may keep this document in its * back/forward cache rather than discarding it. Coming back restores the JavaScript state as it * was left, and nothing re-runs, so anything torn down here would stay torn down on a page that * looks live. A document that really is discarded takes its websockets and its graph with it, so diff --git a/frontend/src/app/workspace/component/workspace.component.ts b/frontend/src/app/workspace/component/workspace.component.ts index b53fcc60658..3e43c07acd9 100644 --- a/frontend/src/app/workspace/component/workspace.component.ts +++ b/frontend/src/app/workspace/component/workspace.component.ts @@ -191,9 +191,10 @@ export class WorkspaceComponent implements AfterViewInit, OnInit, OnDestroy { /** * The browser is leaving this document: save the workflow, and change nothing else. * - * Tearing the session down here was the cause of a page that came back dead. A full-page - * navigation away (the Form View switch is one) fires this, and the browser may then keep the - * document in its back/forward cache rather than discarding it. Coming back restores the + * Tearing the session down here was the cause of a page that came back dead. Leaving the + * document -- a refresh, a closed tab, a URL typed over this one; the Form View switch used to + * be one, and routes now -- fires this, and the browser may then keep the document in its + * back/forward cache rather than discarding it. Coming back restores the * JavaScript state exactly as it was left, so whatever this method had already destroyed stayed * destroyed: an empty graph on a canvas that answered no clicks, and a workflow id reset to the * default, which the share dialog then asked the backend about and got an error for. Nothing From a6752f0111a8875eef2777730df138e45dced261 Mon Sep 17 00:00:00 2001 From: Yang Zhang Date: Tue, 22 Sep 2026 12:50:47 -0700 Subject: [PATCH 13/13] docs(frontend): finish the sweep for comments written under the page-load assumption The previous commit rewrote the eight review named. Sweeping the touched files and their specs for the rest of that vocabulary -- unload, discard, fresh document, navigates away -- found two more in the specs: the menu test that explains why the switch waits for the save ("the navigation unloads the document and aborts anything still in flight") and the workspace test that explains the beforeunload handler ("a full-page navigation away fires beforeunload"). Both now state the reason that survives the change. Everything else the sweep hit is about a page reload that still is one, or is written as history. Comments only; no behaviour change. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01FVvP3ttj22f9LB4p9u2anY --- .../src/app/workspace/component/menu/menu.component.spec.ts | 4 ++-- .../src/app/workspace/component/workspace.component.spec.ts | 5 +++-- 2 files changed, 5 insertions(+), 4 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 854822d9553..e9ab4f7e905 100644 --- a/frontend/src/app/workspace/component/menu/menu.component.spec.ts +++ b/frontend/src/app/workspace/component/menu/menu.component.spec.ts @@ -189,8 +189,8 @@ describe("MenuComponent", () => { component.onClickOpenFormView(); - // The navigation unloads the document and aborts anything still in flight, so it must wait for - // the save's completion rather than be fired right after the request. + // The switch waits for the save to complete rather than firing right after the request: a save + // that fails has to keep the writer here, on the view they edited in, with the error shown. expect(persistSpy).toHaveBeenCalled(); expect(metadataSpy).toHaveBeenCalledWith(saved); expect(navigate).toHaveBeenCalledWith(7); diff --git a/frontend/src/app/workspace/component/workspace.component.spec.ts b/frontend/src/app/workspace/component/workspace.component.spec.ts index 596ba71ff99..118fc9e4118 100644 --- a/frontend/src/app/workspace/component/workspace.component.spec.ts +++ b/frontend/src/app/workspace/component/workspace.component.spec.ts @@ -564,8 +564,9 @@ describe("WorkspaceComponent", () => { expect(workflowResultService.clearResults).toHaveBeenCalled(); }); - // A full-page navigation away fires beforeunload, and the browser may then keep this document - // in its back/forward cache instead of discarding it. Coming back restores the JavaScript + // Leaving the document (a refresh, a closed tab, a URL typed over this one) fires beforeunload, + // and the browser may then keep the document in its back/forward cache instead of discarding + // it. The Form View switch used to be such a navigation and routes now. Coming back restores the JavaScript // state as it was left and re-runs nothing, so anything torn down here stays torn down: the // graph came back empty, the workflow id came back as the default, and the still-subscribed // autosave then wrote that default out as a new, blank workflow (issue #8599).