This is an automated email from the ASF dual-hosted git repository.
github-merge-queue[bot] pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/texera.git
The following commit(s) were added to refs/heads/main by this push:
new 60300e3a1c feat(notebook-migration, frontend): upload notebooks under
a per-work… (#7738)
60300e3a1c is described below
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");
