From 1e9f89121d06ab5ef59606b626d46c0979653b79 Mon Sep 17 00:00:00 2001 From: Ryan Zhang Date: Mon, 17 Aug 2026 11:57:46 -0700 Subject: [PATCH 1/2] feat(notebook-migration, frontend): upload notebooks under a per-workflow filename --- .../jupyter-notebook-panel.component.spec.ts | 22 +++++---------- .../jupyter-notebook-panel.component.ts | 6 ++-- .../jupyter-panel.service.spec.ts | 20 ++++++++++++- .../jupyter-panel/jupyter-panel.service.ts | 24 ++++++++++++++-- .../notebook-migration.service.spec.ts | 28 +++++++++++++++---- .../notebook-migration.service.ts | 28 +++++++++++-------- 6 files changed, 89 insertions(+), 39 deletions(-) diff --git a/frontend/src/app/workspace/component/jupyter-notebook-panel/jupyter-notebook-panel.component.spec.ts b/frontend/src/app/workspace/component/jupyter-notebook-panel/jupyter-notebook-panel.component.spec.ts index 74d2e702983..f6991c2cea7 100644 --- a/frontend/src/app/workspace/component/jupyter-notebook-panel/jupyter-notebook-panel.component.spec.ts +++ b/frontend/src/app/workspace/component/jupyter-notebook-panel/jupyter-notebook-panel.component.spec.ts @@ -20,7 +20,6 @@ import { ComponentFixture, fakeAsync, TestBed, tick } from "@angular/core/testing"; import { JupyterNotebookPanelComponent } from "./jupyter-notebook-panel.component"; import { JupyterPanelService } from "../../service/jupyter-panel/jupyter-panel.service"; -import { NotebookMigrationService } from "../../service/notebook-migration/notebook-migration.service"; import { Subject } from "rxjs"; import { ElementRef } from "@angular/core"; import { By, DomSanitizer } from "@angular/platform-browser"; @@ -30,7 +29,6 @@ describe("JupyterNotebookPanelComponent", () => { let fixture: ComponentFixture; let mockJupyterPanelService: any; - let mockNotebookMigrationService: any; let bypassSpy: ReturnType; beforeEach(async () => { @@ -39,18 +37,12 @@ describe("JupyterNotebookPanelComponent", () => { setIframeRef: vi.fn(), deleteJupyterNotebook: vi.fn(), minimizeJupyterNotebookPanel: vi.fn(), - }; - - mockNotebookMigrationService = { - getJupyterIframeURL: vi.fn().mockResolvedValue("http://localhost:8888"), + getJupyterIframeURLForWorkflow: vi.fn().mockResolvedValue("http://localhost:8888"), }; await TestBed.configureTestingModule({ imports: [JupyterNotebookPanelComponent], - providers: [ - { provide: JupyterPanelService, useValue: mockJupyterPanelService }, - { provide: NotebookMigrationService, useValue: mockNotebookMigrationService }, - ], + providers: [{ provide: JupyterPanelService, useValue: mockJupyterPanelService }], }).compileComponents(); }); @@ -98,7 +90,7 @@ describe("JupyterNotebookPanelComponent", () => { await fixture.whenStable(); fixture.detectChanges(); - expect(mockNotebookMigrationService.getJupyterIframeURL).toHaveBeenCalled(); + expect(mockJupyterPanelService.getJupyterIframeURLForWorkflow).toHaveBeenCalled(); expect(bypassSpy).toHaveBeenCalledWith("http://localhost:8888"); expect(component.jupyterUrl).toBe(bypassSpy.mock.results[0].value); }); @@ -144,20 +136,20 @@ describe("JupyterNotebookPanelComponent", () => { it("should not update jupyterUrl when the iframe URL fetch rejects", async () => { vi.spyOn(component, "checkIframeRef").mockImplementation(() => {}); vi.spyOn(console, "error").mockImplementation(() => {}); - mockNotebookMigrationService.getJupyterIframeURL.mockRejectedValueOnce(new Error("network error")); + mockJupyterPanelService.getJupyterIframeURLForWorkflow.mockRejectedValueOnce(new Error("network error")); mockJupyterPanelService.jupyterNotebookPanelVisible$.next(true); await fixture.whenStable(); - expect(mockNotebookMigrationService.getJupyterIframeURL).toHaveBeenCalled(); + expect(mockJupyterPanelService.getJupyterIframeURLForWorkflow).toHaveBeenCalled(); expect(component.jupyterUrl).toBeNull(); }); it("should keep handling visibility emissions after a failed fetch", async () => { vi.spyOn(component, "checkIframeRef").mockImplementation(() => {}); vi.spyOn(console, "error").mockImplementation(() => {}); - mockNotebookMigrationService.getJupyterIframeURL + mockJupyterPanelService.getJupyterIframeURLForWorkflow .mockRejectedValueOnce(new Error("network error")) .mockResolvedValueOnce("http://localhost:9999"); @@ -168,7 +160,7 @@ describe("JupyterNotebookPanelComponent", () => { await fixture.whenStable(); fixture.detectChanges(); - expect(mockNotebookMigrationService.getJupyterIframeURL).toHaveBeenCalledTimes(2); + expect(mockJupyterPanelService.getJupyterIframeURLForWorkflow).toHaveBeenCalledTimes(2); expect(bypassSpy).toHaveBeenCalledWith("http://localhost:9999"); expect(component.jupyterUrl).toBe(bypassSpy.mock.results[0].value); }); diff --git a/frontend/src/app/workspace/component/jupyter-notebook-panel/jupyter-notebook-panel.component.ts b/frontend/src/app/workspace/component/jupyter-notebook-panel/jupyter-notebook-panel.component.ts index 4f3d957b515..71b0411fd6b 100644 --- a/frontend/src/app/workspace/component/jupyter-notebook-panel/jupyter-notebook-panel.component.ts +++ b/frontend/src/app/workspace/component/jupyter-notebook-panel/jupyter-notebook-panel.component.ts @@ -22,7 +22,6 @@ import { JupyterPanelService } from "../../service/jupyter-panel/jupyter-panel.s import { from, of, Subject } from "rxjs"; import { catchError, switchMap, takeUntil } from "rxjs/operators"; import { DomSanitizer, SafeResourceUrl } from "@angular/platform-browser"; -import { NotebookMigrationService } from "../../service/notebook-migration/notebook-migration.service"; import { CommonModule } from "@angular/common"; import { DragDropModule } from "@angular/cdk/drag-drop"; import { NzButtonModule } from "ng-zorro-antd/button"; @@ -45,8 +44,7 @@ export class JupyterNotebookPanelComponent implements OnInit, AfterViewInit, OnD constructor( private jupyterPanelService: JupyterPanelService, - private sanitizer: DomSanitizer, - private notebookMigrationService: NotebookMigrationService + private sanitizer: DomSanitizer ) {} ngOnInit(): void { @@ -59,7 +57,7 @@ export class JupyterNotebookPanelComponent implements OnInit, AfterViewInit, OnD return of(null); } - return from(this.notebookMigrationService.getJupyterIframeURL()).pipe( + return from(this.jupyterPanelService.getJupyterIframeURLForWorkflow()).pipe( catchError(() => { console.error("Failed to fetch Jupyter iframe URL."); return of(null); diff --git a/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.spec.ts b/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.spec.ts index 43347dfdf1c..2cf0a4d9528 100644 --- a/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.spec.ts +++ b/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.spec.ts @@ -266,7 +266,8 @@ describe("JupyterPanelService", () => { // The mapping is stored before the notebook is handed to Jupyter, ... expect(mockNotebook.setMapping).toHaveBeenCalledWith("mapping_wid_1", mapping); // ... and the 0 came from Jupyter's own answer, not from a thrown error. - expect(mockNotebook.sendNotebookToJupyter).toHaveBeenCalledWith(notebook); + // Upload uses the wid-derived filename. + expect(mockNotebook.sendNotebookToJupyter).toHaveBeenCalledWith(notebook, "notebook_1.ipynb"); expect(consoleError).not.toHaveBeenCalled(); }); @@ -284,6 +285,16 @@ describe("JupyterPanelService", () => { expect(mockNotebook.sendNotebookToJupyter).not.toHaveBeenCalled(); }); + // Iframe URL must use the same wid-derived filename as the upload. + it("getJupyterIframeURLForWorkflow requests the current workflow's per-workflow filename", async () => { + mockNotebook.getJupyterIframeURL = vi.fn().mockResolvedValue("http://iframe"); + + const url = await service.getJupyterIframeURLForWorkflow(); + + expect(url).toBe("http://iframe"); + expect(mockNotebook.getJupyterIframeURL).toHaveBeenCalledWith("notebook_1.ipynb"); + }); + // jupyterNotebookExists$ starts false and flips true once init()'s fetch finds // a notebook for the workflow; the toolbar's expand button binds to this. it("sets jupyterNotebookExists$ true after a workflow's notebook is fetched", async () => { @@ -705,6 +716,13 @@ describe("JupyterPanelService", () => { expect(mockNotification.warning).not.toHaveBeenCalled(); }); + it("getJupyterIframeURLForWorkflow resolves null without calling the migration service", async () => { + mockNotebook.getJupyterIframeURL = vi.fn(); + const url = await service.getJupyterIframeURLForWorkflow(); + expect(url).toBeNull(); + expect(mockNotebook.getJupyterIframeURL).not.toHaveBeenCalled(); + }); + it("onWorkflowComponentClick does not postMessage to the iframe", async () => { const mockIframe = { contentWindow: { postMessage: vi.fn() }, diff --git a/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.ts b/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.ts index 6a14b98efed..d338da9fd4d 100644 --- a/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.ts +++ b/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.ts @@ -25,7 +25,11 @@ import { HttpClient, HttpHeaders } from "@angular/common/http"; import { NotificationService } from "src/app/common/service/notification/notification.service"; import { distinctUntilChanged, switchMap } from "rxjs/operators"; import { AppSettings } from "../../../common/app-setting"; -import { NotebookMigrationService, notebookMappingKey } from "../notebook-migration/notebook-migration.service"; +import { + NotebookMigrationService, + notebookMappingKey, + notebookFileName, +} from "../notebook-migration/notebook-migration.service"; import { GuiConfigService } from "../../../common/service/gui-config.service"; @Injectable({ @@ -139,7 +143,11 @@ export class JupyterPanelService { if (response.exists) { this.notebookMigrationService.setMapping(notebookMappingKey(workflowID), response.mapping); - if ((await this.notebookMigrationService.sendNotebookToJupyter(response.notebook)) == 1) { + const sent = await this.notebookMigrationService.sendNotebookToJupyter( + response.notebook, + this.currentNotebookFileName() + ); + if (sent == 1) { return 1; } else { return 0; @@ -205,6 +213,18 @@ export class JupyterPanelService { this.iframeRef = iframe; } + // Single source for the current workflow's notebook filename, used by both the upload + // and the iframe fetch so they can't derive different names. + private currentNotebookFileName(): string { + return notebookFileName(this.workflowActionService.getWorkflow().wid); + } + + // Iframe URL for the current workflow's notebook + public getJupyterIframeURLForWorkflow(): Promise { + if (!this.enabled) return Promise.resolve(null); + return this.notebookMigrationService.getJupyterIframeURL(this.currentNotebookFileName()); + } + // Open the Jupyter Notebook panel public openPanel(panelName: string): void { if (!this.enabled) return; diff --git a/frontend/src/app/workspace/service/notebook-migration/notebook-migration.service.spec.ts b/frontend/src/app/workspace/service/notebook-migration/notebook-migration.service.spec.ts index 2b47325fdbc..05699cfb218 100644 --- a/frontend/src/app/workspace/service/notebook-migration/notebook-migration.service.spec.ts +++ b/frontend/src/app/workspace/service/notebook-migration/notebook-migration.service.spec.ts @@ -18,7 +18,7 @@ */ import { TestBed } from "@angular/core/testing"; -import { NotebookMigrationService, notebookMappingKey } from "./notebook-migration.service"; +import { NotebookMigrationService, notebookMappingKey, notebookFileName } from "./notebook-migration.service"; import { HttpClient } from "@angular/common/http"; import { HttpClientTestingModule, HttpTestingController } from "@angular/common/http/testing"; import { NotificationService } from "src/app/common/service/notification/notification.service"; @@ -100,11 +100,12 @@ describe("NotebookMigrationService", () => { it("should send notebook successfully and return 1", async () => { const mockNotebook: any = { cells: [] }; - const promise = service.sendNotebookToJupyter(mockNotebook); + const promise = service.sendNotebookToJupyter(mockNotebook, "notebook_1.ipynb"); const req = httpMock.expectOne(req => req.url.includes("/notebook-migration/set-notebook")); expect(req.request.method).toBe("POST"); + expect(req.request.body.notebookName).toBe("notebook_1.ipynb"); req.flush({ success: true }); @@ -117,7 +118,7 @@ describe("NotebookMigrationService", () => { it("should handle error when sending notebook and return 0", async () => { const mockNotebook: any = { cells: [] }; - const promise = service.sendNotebookToJupyter(mockNotebook); + const promise = service.sendNotebookToJupyter(mockNotebook, "notebook_1.ipynb"); const req = httpMock.expectOne(req => req.url.includes("/notebook-migration/set-notebook")); @@ -135,7 +136,7 @@ describe("NotebookMigrationService", () => { // Error` branch. No request reaches the testing backend, so verify() stays happy. vi.spyOn(TestBed.inject(HttpClient), "post").mockReturnValue(throwError(() => new Error("network down"))); - const result = await service.sendNotebookToJupyter({ cells: [] } as any); + const result = await service.sendNotebookToJupyter({ cells: [] } as any, "notebook_1.ipynb"); expect(result).toBe(0); expect(mockNotificationService.error).toHaveBeenCalledWith(expect.stringContaining("network down")); @@ -175,6 +176,18 @@ describe("NotebookMigrationService", () => { const req = httpMock.expectOne(req => req.url.includes("/notebook-migration/get-jupyter-iframe-url")); expect(req.request.method).toBe("GET"); + // No name given, so no notebookName query param is sent. + expect(req.request.params.has("notebookName")).toBe(false); + req.flush({ success: true, url: "http://iframe" }); + + expect(await promise).toBe("http://iframe"); + }); + + it("sends the notebookName as a query param when one is given", async () => { + const promise = service.getJupyterIframeURL("notebook_1.ipynb"); + + const req = httpMock.expectOne(req => req.url.includes("/notebook-migration/get-jupyter-iframe-url")); + expect(req.request.params.get("notebookName")).toBe("notebook_1.ipynb"); req.flush({ success: true, url: "http://iframe" }); expect(await promise).toBe("http://iframe"); @@ -235,6 +248,11 @@ describe("NotebookMigrationService", () => { expect(notebookMappingKey(42)).toBe("mapping_wid_42"); }); + it("notebookFileName builds a per-workflow filename from the wid, defaulting when absent", () => { + expect(notebookFileName(42)).toBe("notebook_42.ipynb"); + expect(notebookFileName(undefined)).toBe("notebook.ipynb"); + }); + // deleteNotebookAndMapping it("should call deleteNotebookAndMapping API with the wid", () => { let result: any; @@ -318,7 +336,7 @@ describe("NotebookMigrationService", () => { }); it("sendNotebookToJupyter returns 0 with no HTTP call or notification", async () => { - const result = await service.sendNotebookToJupyter({ cells: [] } as any); + const result = await service.sendNotebookToJupyter({ cells: [] } as any, "notebook_1.ipynb"); expect(result).toBe(0); expect(mockNotificationService.success).not.toHaveBeenCalled(); expect(mockNotificationService.error).not.toHaveBeenCalled(); diff --git a/frontend/src/app/workspace/service/notebook-migration/notebook-migration.service.ts b/frontend/src/app/workspace/service/notebook-migration/notebook-migration.service.ts index 5c636240f36..6fb73ba3162 100644 --- a/frontend/src/app/workspace/service/notebook-migration/notebook-migration.service.ts +++ b/frontend/src/app/workspace/service/notebook-migration/notebook-migration.service.ts @@ -61,6 +61,12 @@ export function notebookMappingKey(wid: number | undefined): string { return "mapping_wid_" + wid; } +// Per-workflow notebook filename so workflows don't overwrite each other's notebook. +// Falls back to the default when there's no wid. +export function notebookFileName(wid: number | undefined): string { + return wid ? `notebook_${wid}.ipynb` : "notebook.ipynb"; +} + @Injectable({ providedIn: "root", }) @@ -132,16 +138,12 @@ export class NotebookMigrationService { return new NotebookMigrationLLM(this.config, this.workflowUtilService); } - public async sendNotebookToJupyter(notebookData: Notebook) { + public async sendNotebookToJupyter(notebookData: Notebook, notebookName: string) { if (!this.enabled) return 0; const jupyterAPIUrl = `${AppSettings.getApiEndpoint()}/notebook-migration/set-notebook`; const requestBody = { - // Fixed filename is intentional for the v1 per-user-pod design: each user runs - // their own notebook-migration-service and Jupyter, so a single notebook.ipynb - // never collides. A shared multi-user (global) service would need per-user or - // per-workflow keying here and for the backend's process-global jupyterIframeURL. - notebookName: "notebook.ipynb", + notebookName: notebookName, notebookData: notebookData, }; @@ -182,14 +184,16 @@ export class NotebookMigrationService { } } - public async getJupyterIframeURL(): Promise { + public async getJupyterIframeURL(notebookName?: string): Promise { if (!this.enabled) return null; try { - const data = await firstValueFrom( - this.http.get<{ success: boolean; url?: string }>( - `${AppSettings.getApiEndpoint()}/notebook-migration/get-jupyter-iframe-url` - ) - ); + const url = `${AppSettings.getApiEndpoint()}/notebook-migration/get-jupyter-iframe-url`; + // Send notebookName when given; otherwise the backend uses its default. + const params: Record = {}; + if (notebookName) { + params["notebookName"] = notebookName; + } + const data = await firstValueFrom(this.http.get<{ success: boolean; url?: string }>(url, { params })); if (!data.success || !data.url) { console.error("Jupyter server unavailable"); From d73efb99a302aafe332867d648a159ffac4ce6a8 Mon Sep 17 00:00:00 2001 From: Ryan Zhang Date: Mon, 17 Aug 2026 14:30:52 -0700 Subject: [PATCH 2/2] fix(notebook-migration, frontend): upload under the fetched workflow's filename --- .../jupyter-panel/jupyter-panel.service.spec.ts | 17 +++++++++++++++++ .../jupyter-panel/jupyter-panel.service.ts | 5 ++--- 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.spec.ts b/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.spec.ts index 2cf0a4d9528..cd4abcdabc2 100644 --- a/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.spec.ts +++ b/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.spec.ts @@ -285,6 +285,23 @@ describe("JupyterPanelService", () => { expect(mockNotebook.sendNotebookToJupyter).not.toHaveBeenCalled(); }); + it("uploads under the fetched workflow's filename even if the current workflow changed", async () => { + // Stale-fetch guard: a fetch for wid 2 that resolves after the user switched to wid 1 + // must still upload as notebook_2.ipynb, not overwrite wid 1's file. + mockNotebook.sendNotebookToJupyter = vi.fn().mockResolvedValue(1); + mockWorkflow.getWorkflow.mockReturnValue({ wid: 1 }); + const mapping = { cell_to_operator: {}, operator_to_cell: {} }; + const notebook = { cells: [] }; + + const resultPromise = firstValueFrom((service as any).fetchNotebookAndMapping(2, 1)); + httpMock + .expectOne(r => r.url.includes("/notebook-migration/fetch-notebook-and-mapping")) + .flush({ exists: true, mapping, notebook }); + + expect(await resultPromise).toBe(1); + expect(mockNotebook.sendNotebookToJupyter).toHaveBeenCalledWith(notebook, "notebook_2.ipynb"); + }); + // Iframe URL must use the same wid-derived filename as the upload. it("getJupyterIframeURLForWorkflow requests the current workflow's per-workflow filename", async () => { mockNotebook.getJupyterIframeURL = vi.fn().mockResolvedValue("http://iframe"); diff --git a/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.ts b/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.ts index d338da9fd4d..ecb5c90eca5 100644 --- a/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.ts +++ b/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.ts @@ -145,7 +145,7 @@ export class JupyterPanelService { const sent = await this.notebookMigrationService.sendNotebookToJupyter( response.notebook, - this.currentNotebookFileName() + notebookFileName(workflowID) ); if (sent == 1) { return 1; @@ -213,8 +213,7 @@ export class JupyterPanelService { this.iframeRef = iframe; } - // Single source for the current workflow's notebook filename, used by both the upload - // and the iframe fetch so they can't derive different names. + // Notebook filename for the workflow currently shown, used by the iframe fetch. private currentNotebookFileName(): string { return notebookFileName(this.workflowActionService.getWorkflow().wid); }