This is an automated email from the ASF dual-hosted git repository. github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-7738-e80add40cc5564af7dd9d989b1dd2b0943633422 in repository https://gitbox.apache.org/repos/asf/texera.git
commit 60300e3a1ce28ac9d6c698d3bd0d8b202a697411 Author: Ryan Zhang <[email protected]> AuthorDate: Mon Aug 17 22:45:19 2026 +0000 feat(notebook-migration, frontend): upload notebooks under a per-work… (#7738) ### What changes were proposed in this PR? Uploads each workflow's notebook to Jupyter under a per-workflow filename instead of a single shared `notebook.ipynb`. Before this change the frontend always uploaded to `work/notebook.ipynb`. That was safe across users (each runs their own pod) but not across one user's workflows: every workflow wrote to the same file, so opening a second workflow overwrote the first, and because nothing writes back from Jupyter, any edits made in the panel were lost. Two tabs on different workflows also collided on the same file. This PR keys the notebook file on the workflow id (`notebook_<wid>.ipynb`) so each workflow has its own. The backend already accepts this (from #7602): `get-jupyter-iframe-url` takes an optional `notebookName` query param and `set-notebook` accepts any `[A-Za-z0-9._-]+\.ipynb` name, which `notebook_<wid>.ipynb` satisfies. No backend change is needed. **`NotebookMigrationService`** - Adds an exported `notebookFileName(wid)` helper (mirrors the existing `notebookMappingKey`) that returns `notebook_<wid>.ipynb`, or the default when there is no wid. - `sendNotebookToJupyter(notebookData, notebookName)` takes the name instead of hardcoding it. - `getJupyterIframeURL(notebookName?)` sends the name as the `notebookName` query param when given, and omits it otherwise so the backend default still applies. **`JupyterPanelService` (owns the name)** - Adds a private `currentNotebookFileName()` that derives the filename from the current workflow's wid, and uses it for both the upload and the iframe fetch so the two can never derive different names. - Adds a public `getJupyterIframeURLForWorkflow()` that the panel calls to get the URL for the current workflow's notebook. **`JupyterNotebookPanelComponent` (view)** - Calls `jupyterPanelService.getJupyterIframeURLForWorkflow()` and drops its now-unused direct dependency on `NotebookMigrationService`. Because the upload and the iframe fetch both go through `currentNotebookFileName()`, the file that is written and the file the panel requests are always the same, and switching workflows produces a distinct `notebook_<wid>.ipynb` rather than overwriting a shared one. ### Any related issues, documentation, discussions? Closes #7671 Parent issue #4301 Follow-up: deleting a notebook now leaves its `notebook_<wid>.ipynb` file in the Jupyter pod, since `deleteNotebookAndMapping` only removes the database rows. This was self-limiting under the old single-file scheme. Tracked in #7737 ### How was this PR tested? - `notebook-migration.service.spec.ts`: `notebookFileName` mapping, the request body carrying the name on `sendNotebookToJupyter`, and the `notebookName` query param being present when a name is given and absent when it is not. - `jupyter-panel.service.spec.ts`: the upload uses the wid-derived filename, `getJupyterIframeURLForWorkflow` forwards that same filename to the HTTP client, and the disabled-flag path returns null without any HTTP call. - `jupyter-notebook-panel.component.spec.ts`: the panel fetches its URL through `getJupyterIframeURLForWorkflow`. ### Was this PR authored or co-authored using generative AI tooling? Generated-by: Claude Code (Claude Opus 4.8) --- .../jupyter-notebook-panel.component.spec.ts | 22 ++++--------- .../jupyter-notebook-panel.component.ts | 6 ++-- .../jupyter-panel/jupyter-panel.service.spec.ts | 37 +++++++++++++++++++++- .../service/jupyter-panel/jupyter-panel.service.ts | 23 ++++++++++++-- .../notebook-migration.service.spec.ts | 28 +++++++++++++--- .../notebook-migration.service.ts | 28 +++++++++------- 6 files changed, 105 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 74d2e70298..f6991c2cea 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<JupyterNotebookPanelComponent>; let mockJupyterPanelService: any; - let mockNotebookMigrationService: any; let bypassSpy: ReturnType<typeof vi.spyOn>; 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 4f3d957b51..71b0411fd6 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 43347dfdf1..cd4abcdabc 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,33 @@ 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";); + + 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 +733,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 6a14b98efe..ecb5c90eca 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, + notebookFileName(workflowID) + ); + if (sent == 1) { return 1; } else { return 0; @@ -205,6 +213,17 @@ export class JupyterPanelService { this.iframeRef = iframe; } + // Notebook filename for the workflow currently shown, used by the iframe fetch. + private currentNotebookFileName(): string { + return notebookFileName(this.workflowActionService.getWorkflow().wid); + } + + // Iframe URL for the current workflow's notebook + public getJupyterIframeURLForWorkflow(): Promise<string | null> { + 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 2b47325fdb..05699cfb21 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 5c636240f3..6fb73ba316 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<string | null> { + public async getJupyterIframeURL(notebookName?: string): Promise<string | null> { 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<string, string> = {}; + 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");
