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..e9ab4f7e905 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"; @@ -52,7 +53,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 +123,44 @@ 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)); + }); + + // 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", () => { @@ -155,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); @@ -822,6 +856,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(() => {}); @@ -1197,104 +1245,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 865e147ed05..e320fc484e2 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,13 +40,13 @@ 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"; 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"; @@ -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; @@ -198,17 +198,14 @@ 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; // return the run button after the execution is finished, either // when the value is valid or invalid @@ -227,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 @@ -711,9 +711,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; @@ -726,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); @@ -766,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; } @@ -777,15 +779,27 @@ 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. + * + * 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. */ - /* v8 ignore start */ private openFormViewPage(wid: number): void { - window.location.href = `${USER_WORKSPACE}/${wid}/form`; + this.router.navigateByUrl(workspaceFormUrl(wid)).then( + navigated => { + if (!navigated) { + this.handingOverToFormView = false; + } + }, + () => (this.handingOverToFormView = false) + ); } - /* v8 ignore stop */ /** * Calls Markdown Description Component 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/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/mini-map/mini-map.component.spec.ts b/frontend/src/app/workspace/component/workflow-editor/mini-map/mini-map.component.spec.ts index 0a5c31c694b..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 @@ -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][] = []; @@ -63,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 }; } @@ -162,8 +174,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 +189,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 +204,54 @@ 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(); + }); + + // 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"); + }); + + // 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 73a2c5bd089..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 @@ -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"; @@ -31,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", @@ -50,18 +53,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" }, @@ -76,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; @@ -88,8 +103,36 @@ 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.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)); } @@ -102,8 +145,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 d397704d101..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,105 @@ 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(); + + 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 + // 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(); + } + }); + + // 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(); + }); + + // 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(wrapper.getHeatmapView()).toBeNull(); + 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", () => { 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..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 @@ -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; @@ -205,10 +207,23 @@ export class WorkflowEditorComponent implements OnInit, AfterViewInit, OnDestroy } ngAfterViewInit() { - this.editor = document.getElementById("workflow-editor")!; - this.editorWrapper = document.getElementById("workflow-editor-wrapper")!; - document.addEventListener("keydown", this._handleKeyboardAction.bind(this)); + // 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.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(); @@ -255,13 +270,25 @@ export class WorkflowEditorComponent implements OnInit, AfterViewInit, OnDestroy ngOnDestroy(): void { this.paperResizeObserver?.disconnect(); - document.removeEventListener("keydown", this._handleKeyboardAction.bind(this)); - // 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 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. 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 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 18088fdaf71..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 @@ -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,157 @@ 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(); + }); + + // 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(); + }); + + // 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(); + 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); + }); + + // 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); + }); + + // 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.durationTicks.next(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(); + + 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(); + }); + + // 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", () => { + 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", () => { @@ -1682,24 +1834,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 95b58b0b8d9..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 @@ -32,10 +32,10 @@ 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 { 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"; @@ -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"; @@ -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; @@ -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(); }); @@ -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. @@ -489,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 @@ -566,7 +556,24 @@ 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(); + // 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(); @@ -587,18 +594,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 +605,51 @@ 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 + // 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. + const retained = this.executeWorkflowService.getExecutionState(); + this.executionState = retained.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; + // 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(); + // 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; + 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 +1828,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 @@ -1843,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.) @@ -1980,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 @@ -2004,10 +2048,17 @@ 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.getOpenWorkflowId())) { + this.workflowActionService.clearWorkflow(); + this.computingUnitStatusService.disconnect(); + 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 8dc429dc275..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 @@ -136,10 +136,16 @@ 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, + // 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(), @@ -171,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: () => [], @@ -214,6 +222,10 @@ 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 }), + 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 9ca8be10018..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 @@ -17,11 +17,12 @@ * 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"; 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: {} }; @@ -51,7 +52,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(); @@ -65,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(); @@ -119,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(), @@ -129,8 +148,17 @@ 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: {} } }), - getWorkflowMetadata: () => ({ name: "scGPT", lastModifiedTime: 1767225600000 }), + // 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(), @@ -161,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. @@ -264,8 +288,20 @@ 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; 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 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(), @@ -351,6 +387,7 @@ export function setupHarness() { datePipe, config, warehouseService, + execution, workflowChangedStream, formBindingChanged, workflowMetaDataChangedStream, @@ -358,6 +395,7 @@ export function setupHarness() { executionStateStream, modificationEnabled, durationEvents, + durationTicks, statusStream, selectedUnitStream, validationStream, diff --git a/frontend/src/app/workspace/component/workspace.component.spec.ts b/frontend/src/app/workspace/component/workspace.component.spec.ts index c651f37daae..118fc9e4118 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,8 +114,16 @@ 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), + // 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. + republishWorkflowMetadata: vi.fn(() => metadataChangedSubject.next()), }; workflowPersistService = { @@ -143,14 +151,26 @@ 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 = { 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 nothing on + // the state stream, which carries transitions rather than a current value. + reapplyExecutionLock: vi.fn(), + }; workflowConsoleService = { clearConsoleMessages: vi.fn() }; workflowResultService = { clearResults: vi.fn() }; @@ -235,6 +255,53 @@ 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(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. + 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.reapplyExecutionLock).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 + // 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", () => { @@ -497,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). @@ -518,6 +586,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 +613,50 @@ 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(); + }); + + // 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(); + 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..3e43c07acd9 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(); @@ -178,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 @@ -197,11 +211,19 @@ 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.getOpenWorkflowId())) { + 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); + } } private persistBeforeLeaving(): void { @@ -329,6 +351,28 @@ 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. + // 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) { + // 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. 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 + // 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 if (wid) { // show loading spinner right away while waiting for workflow to load 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..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 @@ -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,133 @@ 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. + // 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); + }); + + // 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 + // 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.reapplyExecutionLock(); + expect(enable).toHaveBeenCalled(); + expect(disable).not.toHaveBeenCalled(); + + 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.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", () => { (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..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 @@ -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,6 +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 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, @@ -117,6 +132,9 @@ export class ExecuteWorkflowService { case "WorkerAssignmentUpdateEvent": this.assignedWorkerIds.set(event.operatorId, event.workerIds); break; + case "ExecutionDurationUpdateEvent": + this.anchorDuration(event.duration, event.isRunning); + break; default: // workflow status related event this.handleReconfigurationEvent(event); @@ -194,6 +212,49 @@ export class ExecuteWorkflowService { return this.currentState; } + /** How long the current run has been going, worked out from the anchor. */ + public getExecutionDuration(): number { + 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()); + } + + /** + * Apply the graph lock the current execution state implies, for a view that arrived on a workflow + * whose run was already in flight. + * + * 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 reapplyExecutionLock(): void { + this.updateWorkflowActionLock(this.currentState); + } + public getErrorMessages(): ReadonlyArray { if (this.currentState?.state === ExecutionState.Failed) { return this.currentState.errorMessages; @@ -383,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); } /** @@ -393,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 { 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-graph/model/workflow-action.service.spec.ts b/frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.spec.ts index 87e002a86a9..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 @@ -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; @@ -74,6 +75,73 @@ 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); + // 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 + // 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", () => { 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..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 @@ -629,6 +629,57 @@ 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. + * + * 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 | 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; + } + + /** + * 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. 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 {