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-5271-ba63cedf0185dc1cd92fd4eec5f175a660639c02 in repository https://gitbox.apache.org/repos/asf/texera.git
commit 0f7a42610e124e3860e74780ed62bc59ec5afe67 Author: Ryan Zhang <[email protected]> AuthorDate: Fri Jul 31 14:56:32 2026 -0700 feat(python-notebook-migration, frontend): add Jupyter notebook iframe panel component (#5271) ### What changes were proposed in this PR? Adds `JupyterNotebookPanelComponent`, a draggable iframe panel that hosts JupyterLab inside the Texera workspace, mounts it in the workspace shell, and wires its delete button to remove the current workflow's stored notebook through the panel service. - `frontend/src/app/workspace/component/jupyter-notebook-panel/jupyter-notebook-panel.component.ts` - `JupyterNotebookPanelComponent` (standalone) - `jupyterUrl` is typed `SafeResourceUrl | null` and starts as `null`. - `ngOnInit` subscribes to `jupyterPanelService.jupyterNotebookPanelVisible$`. Each emission sets `isVisible`. When the panel becomes visible it fetches the iframe URL from `notebookMigrationService.getJupyterIframeURL()` (a rejected fetch is caught and yields `null` without ending the subscription), then assigns `jupyterUrl` to the sanitized URL (via `DomSanitizer.bypassSecurityTrustResourceUrl`) or to `null`, and calls `checkIframeRef()`. A `null` result (panel hidden, or a failed or empty fetch) clears `jupyterUrl` so a stale URL is never shown. - `ngAfterViewInit` calls `checkIframeRef()`, which registers the iframe with the panel service via `setIframeRef` inside a `setTimeout`, only when the panel is visible and `iframeRef` is present, and stays silent when the panel is hidden. - `deletePanel()` calls `jupyterPanelService.deleteJupyterNotebook()`. - `minimizePanel()` calls `jupyterPanelService.minimizeJupyterNotebookPanel()`; visibility is driven by the observable. - `frontend/src/app/workspace/component/jupyter-notebook-panel/jupyter-notebook-panel.component.html` - `cdkDrag` panel gated on `*ngIf="isVisible"`, with `cdkDragHandle` on the header so only the header is draggable. - Header title plus a minimize button (tooltip "Minimize") and a delete button (tooltip "Delete notebook") that opens an `nz-popconfirm` confirmation before deleting. - `<iframe [src]="jupyterUrl">` gated on `*ngIf="jupyterUrl"`, so the iframe renders only once a sanitized URL is available. - `frontend/src/app/workspace/component/jupyter-notebook-panel/jupyter-notebook-panel.component.scss` - Fixed-position panel styling, header visuals, a muted hover color shared by the two header buttons, and iframe sizing. - `frontend/src/app/workspace/component/workspace.component.ts` and `workspace.component.html` - The standalone `WorkspaceComponent` imports `JupyterNotebookPanelComponent` and mounts `<texera-jupyter-notebook-panel>` in the workspace shell. - `frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.ts` - `deleteJupyterNotebook()`: deletes the current workflow's stored notebook via `notebookMigrationService.deleteNotebookAndMapping(wid)`. On success it hides the panel, drops the in-memory mapping, resets `jupyterNotebookExists$`, and unhighlights all operators and links. On failure it shows a notification and leaves the panel open. An unsaved workflow (no `wid`) resets local state without a backend call. - `hideAndClearLocalState()` (private): hides the panel and drops the current workflow's in-memory mapping only. The workflow-change handler in `init()` uses this, so switching workflows never deletes a notebook from the backend. - `clearHighlights()` (private): unhighlights all operators and links and clears the highlight index. - `frontend/src/app/workspace/service/notebook-migration/notebook-migration.service.ts` - `deleteNotebookAndMapping(wid)`: sends a `POST` to `/notebook-migration/delete-notebook-and-mapping`, short-circuiting when the feature is disabled. ### Any related issues, documentation, discussions? Closes #5270 Parent issue #4301 ### How was this PR tested? Unit tests (Vitest): - `jupyter-notebook-panel.component.spec.ts`: visibility transitions, URL fetch and sanitization, the rejected-fetch path (the subscription stays alive and `jupyterUrl` stays `null`), the iframe rendering only when a URL is present and being removed on hide, iframe ref registration and the silent-when-hidden behavior, and delegation of the minimize and delete buttons. - `jupyter-panel.service.spec.ts`: `deleteJupyterNotebook` calls the backend, hides the panel, resets `jupyterNotebookExists$`, and unhighlights on success; keeps the panel open and notifies on failure; and a guard that switching workflows never triggers a backend delete. ### Was this PR authored or co-authored using generative AI tooling? Generated-by: Claude Code (Claude Opus 4.8) --- .../jupyter-notebook-panel.component.html | 67 +++++ .../jupyter-notebook-panel.component.scss | 77 ++++++ .../jupyter-notebook-panel.component.spec.ts | 279 +++++++++++++++++++++ .../jupyter-notebook-panel.component.ts | 115 +++++++++ .../workspace/component/workspace.component.html | 1 + .../app/workspace/component/workspace.component.ts | 2 + .../jupyter-panel/jupyter-panel.service.spec.ts | 83 +++++- .../service/jupyter-panel/jupyter-panel.service.ts | 67 ++++- .../notebook-migration.service.spec.ts | 19 ++ .../notebook-migration.service.ts | 22 ++ 10 files changed, 718 insertions(+), 14 deletions(-) diff --git a/frontend/src/app/workspace/component/jupyter-notebook-panel/jupyter-notebook-panel.component.html b/frontend/src/app/workspace/component/jupyter-notebook-panel/jupyter-notebook-panel.component.html new file mode 100644 index 0000000000..edca1ba47b --- /dev/null +++ b/frontend/src/app/workspace/component/jupyter-notebook-panel/jupyter-notebook-panel.component.html @@ -0,0 +1,67 @@ +<!-- + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, + software distributed under the License is distributed on an + "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + KIND, either express or implied. See the License for the + specific language governing permissions and limitations + under the License. +--> + +<div + class="draggable-panel" + *ngIf="isVisible" + cdkDrag + cdkDragBoundary="texera-workspace"> + <div + class="panel-header" + cdkDragHandle> + <span>Jupyter Notebook</span> + <div class="panel-buttons"> + <!-- Minimize button --> + <button + class="minimize-button" + nz-tooltip + nzTooltipTitle="Minimize" + (click)="minimizePanel()"> + <i + nz-icon + nzType="minus" + nzTheme="outline"></i> + </button> + <!-- Delete button --> + <button + class="delete-button" + nz-tooltip + nzTooltipTitle="Delete notebook" + nz-popconfirm + nzPopconfirmTitle="Delete this workflow's Jupyter notebook? This cannot be undone." + (nzOnConfirm)="deletePanel()"> + <i + nz-icon + nzType="delete" + nzTheme="outline"></i> + </button> + </div> + </div> + + <div class="iframe-container"> + <iframe + *ngIf="jupyterUrl" + #iframeRef + [src]="jupyterUrl" + title="Jupyter Notebook" + width="100%" + height="100%" + style="border: none"></iframe> + </div> +</div> diff --git a/frontend/src/app/workspace/component/jupyter-notebook-panel/jupyter-notebook-panel.component.scss b/frontend/src/app/workspace/component/jupyter-notebook-panel/jupyter-notebook-panel.component.scss new file mode 100644 index 0000000000..8d8bb0631f --- /dev/null +++ b/frontend/src/app/workspace/component/jupyter-notebook-panel/jupyter-notebook-panel.component.scss @@ -0,0 +1,77 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +.draggable-panel { + position: absolute; + top: 50%; + left: 50%; + margin-top: 200px; + width: 660px; + height: 400px; + background-color: #fff; + box-shadow: 0 1px 5px rgba(0, 0, 0, 0.3); + border: 1px solid #ccc; + border-radius: 5px; + z-index: 1000; + resize: both; + overflow: auto; +} + +.panel-header { + display: flex; + justify-content: space-between; + align-items: center; + background-color: #547baa; + color: white; + padding: 5px 10px 5px 10px; + font-weight: bold; + text-align: center; + cursor: move; +} + +.panel-buttons { + display: flex; + align-items: center; + gap: 8px; +} + +.minimize-button, +.delete-button { + background: none; + border: none; + color: white; + font-size: 16px; + cursor: pointer; +} + +.minimize-button:hover, +.delete-button:hover { + color: #bfbfbf; +} + +.iframe-container { + width: 100%; + height: calc(100% - 40px); /* Adjust height for the header */ +} + +.iframe-container iframe { + width: 100%; + height: 100%; + border: none; +} 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 new file mode 100644 index 0000000000..74d2e70298 --- /dev/null +++ b/frontend/src/app/workspace/component/jupyter-notebook-panel/jupyter-notebook-panel.component.spec.ts @@ -0,0 +1,279 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +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"; + +describe("JupyterNotebookPanelComponent", () => { + let component: JupyterNotebookPanelComponent; + let fixture: ComponentFixture<JupyterNotebookPanelComponent>; + + let mockJupyterPanelService: any; + let mockNotebookMigrationService: any; + let bypassSpy: ReturnType<typeof vi.spyOn>; + + beforeEach(async () => { + mockJupyterPanelService = { + jupyterNotebookPanelVisible$: new Subject<boolean>(), + setIframeRef: vi.fn(), + deleteJupyterNotebook: vi.fn(), + minimizeJupyterNotebookPanel: vi.fn(), + }; + + mockNotebookMigrationService = { + getJupyterIframeURL: vi.fn().mockResolvedValue("http://localhost:8888";), + }; + + await TestBed.configureTestingModule({ + imports: [JupyterNotebookPanelComponent], + providers: [ + { provide: JupyterPanelService, useValue: mockJupyterPanelService }, + { provide: NotebookMigrationService, useValue: mockNotebookMigrationService }, + ], + }).compileComponents(); + }); + + beforeEach(() => { + fixture = TestBed.createComponent(JupyterNotebookPanelComponent); + component = fixture.componentInstance; + // Spy on the shared DomSanitizer so tests can assert the raw URL was trusted, + // without matching Angular's serialized SafeResourceUrl string. + bypassSpy = vi.spyOn(TestBed.inject(DomSanitizer), "bypassSecurityTrustResourceUrl"); + fixture.detectChanges(); + }); + + // Destroy the component so its subscriptions complete, and restore spies so a + // shared spy's call history (e.g. console.error) does not accumulate across + // tests. Mocks are not auto-reset by the Vitest config. + afterEach(() => { + fixture.destroy(); + vi.restoreAllMocks(); + }); + + it("should create", () => { + vi.spyOn(component, "checkIframeRef").mockImplementation(() => {}); + expect(component).toBeTruthy(); + }); + + it("should be hidden by default", () => { + vi.spyOn(component, "checkIframeRef").mockImplementation(() => {}); + expect(component.isVisible).toBe(false); + }); + + it("should update visibility when service emits", async () => { + vi.spyOn(component, "checkIframeRef").mockImplementation(() => {}); + mockJupyterPanelService.jupyterNotebookPanelVisible$.next(true); + + await fixture.whenStable(); + fixture.detectChanges(); + + expect(component.isVisible).toBe(true); + }); + + it("should fetch and sanitize URL when panel becomes visible", async () => { + vi.spyOn(component, "checkIframeRef").mockImplementation(() => {}); + mockJupyterPanelService.jupyterNotebookPanelVisible$.next(true); + + await fixture.whenStable(); + fixture.detectChanges(); + + expect(mockNotebookMigrationService.getJupyterIframeURL).toHaveBeenCalled(); + expect(bypassSpy).toHaveBeenCalledWith("http://localhost:8888";); + expect(component.jupyterUrl).toBe(bypassSpy.mock.results[0].value); + }); + + it("should not render the iframe while visible without a URL", () => { + vi.spyOn(component, "checkIframeRef").mockImplementation(() => {}); + component.isVisible = true; + + // Rendering with an unset jupyterUrl must not throw NG0904 (unsafe resource + // URL); the iframe should simply be absent until a URL is available. + expect(() => fixture.detectChanges()).not.toThrow(); + expect(fixture.nativeElement.querySelector("iframe")).toBeNull(); + }); + + it("should render the iframe once a URL is available", async () => { + vi.spyOn(component, "checkIframeRef").mockImplementation(() => {}); + mockJupyterPanelService.jupyterNotebookPanelVisible$.next(true); + + await fixture.whenStable(); + fixture.detectChanges(); + + const iframe = fixture.nativeElement.querySelector("iframe"); + expect(iframe).not.toBeNull(); + // Don't match the serialized SafeResourceUrl; assert the binding produced a src. + expect(iframe.hasAttribute("src")).toBe(true); + }); + + it("should clear jupyterUrl and remove the iframe when hidden", async () => { + vi.spyOn(component, "checkIframeRef").mockImplementation(() => {}); + mockJupyterPanelService.jupyterNotebookPanelVisible$.next(true); + await fixture.whenStable(); + fixture.detectChanges(); + expect(component.jupyterUrl).not.toBeNull(); + expect(fixture.nativeElement.querySelector("iframe")).not.toBeNull(); + + mockJupyterPanelService.jupyterNotebookPanelVisible$.next(false); + await fixture.whenStable(); + fixture.detectChanges(); + expect(component.jupyterUrl).toBeNull(); + expect(fixture.nativeElement.querySelector("iframe")).toBeNull(); + }); + + 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.jupyterNotebookPanelVisible$.next(true); + + await fixture.whenStable(); + + expect(mockNotebookMigrationService.getJupyterIframeURL).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 + .mockRejectedValueOnce(new Error("network error")) + .mockResolvedValueOnce("http://localhost:9999";); + + mockJupyterPanelService.jupyterNotebookPanelVisible$.next(true); + await fixture.whenStable(); + + mockJupyterPanelService.jupyterNotebookPanelVisible$.next(true); + await fixture.whenStable(); + fixture.detectChanges(); + + expect(mockNotebookMigrationService.getJupyterIframeURL).toHaveBeenCalledTimes(2); + expect(bypassSpy).toHaveBeenCalledWith("http://localhost:9999";); + expect(component.jupyterUrl).toBe(bypassSpy.mock.results[0].value); + }); + + it("should call setIframeRef when iframe exists and visible", fakeAsync(() => { + component.isVisible = true; + + const mockIframe = document.createElement("iframe"); + component.iframeRef = new ElementRef(mockIframe); + + component.checkIframeRef(); + tick(0); + + expect(mockJupyterPanelService.setIframeRef).toHaveBeenCalledWith(mockIframe); + })); + + it("should NOT call setIframeRef if not visible", fakeAsync(() => { + component.isVisible = false; + + const mockIframe = document.createElement("iframe"); + component.iframeRef = new ElementRef(mockIframe); + + component.checkIframeRef(); + tick(0); + + expect(mockJupyterPanelService.setIframeRef).not.toHaveBeenCalled(); + })); + + it("should not log an error when checkIframeRef runs while the panel is hidden", fakeAsync(() => { + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + component.isVisible = false; + + component.checkIframeRef(); + tick(0); + + expect(errorSpy).not.toHaveBeenCalled(); + expect(mockJupyterPanelService.setIframeRef).not.toHaveBeenCalled(); + })); + + it("should log an error when visible but the iframe ref is missing", fakeAsync(() => { + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + component.isVisible = true; + component.iframeRef = undefined as any; + + component.checkIframeRef(); + tick(0); + + expect(errorSpy).toHaveBeenCalledWith("Jupyter Iframe reference not found."); + expect(mockJupyterPanelService.setIframeRef).not.toHaveBeenCalled(); + })); + + it("should delete notebook via service", () => { + vi.spyOn(component, "checkIframeRef").mockImplementation(() => {}); + component.deletePanel(); + expect(mockJupyterPanelService.deleteJupyterNotebook).toHaveBeenCalled(); + }); + + it("should minimize panel and update visibility", () => { + vi.spyOn(component, "checkIframeRef").mockImplementation(() => {}); + // The real service emits false on minimize; visibility is observable-driven, + // so the mock must emit for isVisible to update. + mockJupyterPanelService.minimizeJupyterNotebookPanel = vi.fn(() => + mockJupyterPanelService.jupyterNotebookPanelVisible$.next(false) + ); + component.isVisible = true; + + component.minimizePanel(); + + expect(component.isVisible).toBe(false); + expect(mockJupyterPanelService.minimizeJupyterNotebookPanel).toHaveBeenCalled(); + }); + + it("should minimize via the header minimize button click", () => { + vi.spyOn(component, "checkIframeRef").mockImplementation(() => {}); + component.isVisible = true; + fixture.detectChanges(); + + const minimizeButton = fixture.debugElement.query(By.css(".minimize-button")); + expect(minimizeButton).not.toBeNull(); + minimizeButton.triggerEventHandler("click", new MouseEvent("click")); + + expect(mockJupyterPanelService.minimizeJupyterNotebookPanel).toHaveBeenCalled(); + }); + + it("should delete via the header delete button popconfirm", () => { + vi.spyOn(component, "checkIframeRef").mockImplementation(() => {}); + component.isVisible = true; + fixture.detectChanges(); + + const deleteButton = fixture.debugElement.query(By.css(".delete-button")); + expect(deleteButton).not.toBeNull(); + // Fire the popconfirm's confirm output, which runs the (nzOnConfirm) binding. + deleteButton.triggerEventHandler("nzOnConfirm", undefined); + + expect(mockJupyterPanelService.deleteJupyterNotebook).toHaveBeenCalled(); + }); + + it("should clean up on destroy", () => { + vi.spyOn(component, "checkIframeRef").mockImplementation(() => {}); + const nextSpy = vi.spyOn(component["destroy$"] as any, "next"); + const completeSpy = vi.spyOn(component["destroy$"] as any, "complete"); + + component.ngOnDestroy(); + + expect(nextSpy).toHaveBeenCalled(); + expect(completeSpy).toHaveBeenCalled(); + }); +}); 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 new file mode 100644 index 0000000000..4f3d957b51 --- /dev/null +++ b/frontend/src/app/workspace/component/jupyter-notebook-panel/jupyter-notebook-panel.component.ts @@ -0,0 +1,115 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { Component, ElementRef, OnDestroy, OnInit, ViewChild, AfterViewInit } from "@angular/core"; +import { JupyterPanelService } from "../../service/jupyter-panel/jupyter-panel.service"; +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"; +import { NzIconModule } from "ng-zorro-antd/icon"; +import { NzPopconfirmModule } from "ng-zorro-antd/popconfirm"; +import { NzTooltipModule } from "ng-zorro-antd/tooltip"; + +@Component({ + selector: "texera-jupyter-notebook-panel", + templateUrl: "./jupyter-notebook-panel.component.html", + styleUrls: ["./jupyter-notebook-panel.component.scss"], + imports: [CommonModule, DragDropModule, NzButtonModule, NzIconModule, NzPopconfirmModule, NzTooltipModule], +}) +export class JupyterNotebookPanelComponent implements OnInit, AfterViewInit, OnDestroy { + @ViewChild("iframeRef", { static: false }) iframeRef!: ElementRef<HTMLIFrameElement>; // Use static: false + + isVisible: boolean = false; // Initialize to false, meaning the panel is hidden by default + jupyterUrl: SafeResourceUrl | null = null; // Store the notebook URL dynamically; null when no URL is loaded + private destroy$ = new Subject<void>(); + + constructor( + private jupyterPanelService: JupyterPanelService, + private sanitizer: DomSanitizer, + private notebookMigrationService: NotebookMigrationService + ) {} + + ngOnInit(): void { + this.jupyterPanelService.jupyterNotebookPanelVisible$ + .pipe( + switchMap((visible: boolean) => { + this.isVisible = visible; + + if (!visible) { + return of(null); + } + + return from(this.notebookMigrationService.getJupyterIframeURL()).pipe( + catchError(() => { + console.error("Failed to fetch Jupyter iframe URL."); + return of(null); + }) + ); + }), + takeUntil(this.destroy$) + ) + .subscribe(url => { + // Always reflect the latest fetch result. A null url (panel hidden, or a + // failed/empty fetch) clears jupyterUrl so a stale URL is never rendered. + this.jupyterUrl = url ? this.sanitizer.bypassSecurityTrustResourceUrl(url) : null; + if (url) { + this.checkIframeRef(); + } + }); + } + + ngAfterViewInit(): void { + // Ensure iframe is handled after it's available in the DOM + this.checkIframeRef(); + } + + checkIframeRef(): void { + setTimeout(() => { + if (!this.isVisible) { + // Panel hidden; no iframe to register. + return; + } + if (this.iframeRef?.nativeElement) { + this.jupyterPanelService.setIframeRef(this.iframeRef.nativeElement); + } else { + console.error("Jupyter Iframe reference not found."); + } + }, 0); // Small timeout to ensure DOM is updated + } + + ngOnDestroy(): void { + this.destroy$.next(); + this.destroy$.complete(); // Cleanup subscriptions to avoid memory leaks + } + + // Delete the workflow's Jupyter notebook by invoking the service method + deletePanel(): void { + this.jupyterPanelService.deleteJupyterNotebook(); + } + + // Minimize the jupyter notebook by invoking the service method. Visibility is + // driven by jupyterNotebookPanelVisible$, so no local isVisible mutation here. + minimizePanel(): void { + this.jupyterPanelService.minimizeJupyterNotebookPanel(); + } +} diff --git a/frontend/src/app/workspace/component/workspace.component.html b/frontend/src/app/workspace/component/workspace.component.html index c54446fb31..89eeb656f1 100644 --- a/frontend/src/app/workspace/component/workspace.component.html +++ b/frontend/src/app/workspace/component/workspace.component.html @@ -37,4 +37,5 @@ *ngIf="copilotEnabled" [agentIdToActivate]="agentIdToActivate"></texera-agent-panel> <texera-property-editor></texera-property-editor> +<texera-jupyter-notebook-panel></texera-jupyter-notebook-panel> <ng-template #codeEditor></ng-template> diff --git a/frontend/src/app/workspace/component/workspace.component.ts b/frontend/src/app/workspace/component/workspace.component.ts index 2aa1c43db9..b978107c83 100644 --- a/frontend/src/app/workspace/component/workspace.component.ts +++ b/frontend/src/app/workspace/component/workspace.component.ts @@ -63,6 +63,7 @@ import { MiniMapComponent } from "./workflow-editor/mini-map/mini-map.component" import { LeftPanelComponent } from "./left-panel/left-panel.component"; import { AgentPanelComponent } from "./agent/agent-panel/agent-panel.component"; import { PropertyEditorComponent } from "./property-editor/property-editor.component"; +import { JupyterNotebookPanelComponent } from "./jupyter-notebook-panel/jupyter-notebook-panel.component"; export const SAVE_DEBOUNCE_TIME_IN_MS = 5000; @@ -85,6 +86,7 @@ export const SAVE_DEBOUNCE_TIME_IN_MS = 5000; NgIf, AgentPanelComponent, PropertyEditorComponent, + JupyterNotebookPanelComponent, ], }) export class WorkspaceComponent implements AfterViewInit, OnInit, OnDestroy { 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 17c8c0d33f..c60b06d368 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 @@ -24,7 +24,7 @@ import { HttpClientTestingModule, HttpTestingController } from "@angular/common/ import { NotificationService } from "src/app/common/service/notification/notification.service"; import { NotebookMigrationService } from "../notebook-migration/notebook-migration.service"; import { GuiConfigService } from "src/app/common/service/gui-config.service"; -import { firstValueFrom, of } from "rxjs"; +import { firstValueFrom, of, throwError } from "rxjs"; describe("JupyterPanelService", () => { let service: JupyterPanelService; @@ -59,6 +59,7 @@ describe("JupyterPanelService", () => { mockNotification = { warning: vi.fn(), + error: vi.fn(), }; mockNotebook = { @@ -72,6 +73,7 @@ describe("JupyterPanelService", () => { deleteMapping: vi.fn(), setMapping: vi.fn(), getJupyterURL: vi.fn().mockResolvedValue("http://jupyter";), + deleteNotebookAndMapping: vi.fn().mockReturnValue(of({ success: true, deleted: 1 })), }; mockGuiConfig = { env: { pythonNotebookMigrationEnabled: true } }; @@ -96,7 +98,7 @@ describe("JupyterPanelService", () => { }); // Panel visibility - it("should open and close panel", () => { + it("should open panel and hide it after deleting the notebook", () => { let state: boolean | null = null; service.jupyterNotebookPanelVisible$.subscribe(v => (state = v)); @@ -104,10 +106,59 @@ describe("JupyterPanelService", () => { service.openPanel("JupyterNotebookPanel"); expect(state).toBe(true); - service.closeJupyterNotebookPanel(); + service.deleteJupyterNotebook(); + expect(mockNotebook.deleteNotebookAndMapping).toHaveBeenCalledWith(1); expect(state).toBe(false); }); + it("deleteJupyterNotebook clears local state and unhighlights on success", () => { + let visible: boolean | null = null; + let exists: boolean | null = null; + service.jupyterNotebookPanelVisible$.subscribe(v => (visible = v)); + service.jupyterNotebookExists$.subscribe(v => (exists = v)); + (service as any).jupyterNotebookPanelVisible.next(true); + (service as any).jupyterNotebookExists.next(true); + + service.deleteJupyterNotebook(); + + expect(mockNotebook.deleteNotebookAndMapping).toHaveBeenCalledWith(1); + expect(mockNotebook.deleteMapping).toHaveBeenCalledWith("mapping_wid_1"); + expect(visible).toBe(false); + expect(exists).toBe(false); + expect(mockWorkflow.unhighlightOperators).toHaveBeenCalled(); + expect(mockWorkflow.unhighlightLinks).toHaveBeenCalled(); + }); + + it("deleteJupyterNotebook keeps the panel open and notifies on failure", () => { + mockNotebook.deleteNotebookAndMapping.mockReturnValueOnce(throwError(() => new Error("boom"))); + let visible: boolean | null = null; + service.jupyterNotebookPanelVisible$.subscribe(v => (visible = v)); + (service as any).jupyterNotebookPanelVisible.next(true); + + service.deleteJupyterNotebook(); + + expect(mockNotification.error).toHaveBeenCalled(); + expect(visible).toBe(true); + expect(mockNotebook.deleteMapping).not.toHaveBeenCalled(); + }); + + it("deleteJupyterNotebook only resets local state for the default wid 0 (no backend call)", () => { + mockWorkflow.getWorkflow.mockReturnValue({ wid: 0 }); + let visible: boolean | null = null; + let exists: boolean | null = null; + service.jupyterNotebookPanelVisible$.subscribe(v => (visible = v)); + service.jupyterNotebookExists$.subscribe(v => (exists = v)); + (service as any).jupyterNotebookPanelVisible.next(true); + (service as any).jupyterNotebookExists.next(true); + + service.deleteJupyterNotebook(); + + // wid 0 is the unsaved default workflow, so no backend delete should fire. + expect(mockNotebook.deleteNotebookAndMapping).not.toHaveBeenCalled(); + expect(visible).toBe(false); + expect(exists).toBe(false); + }); + it("should minimize panel", () => { let state: boolean | null = true; @@ -152,6 +203,20 @@ describe("JupyterPanelService", () => { expect(state).toBe(true); }); + it("openPanel flags jupyterNotebookExists$ so the toolbar expand button appears after an in-place import", () => { + const states: boolean[] = []; + service.jupyterNotebookExists$.subscribe(v => states.push(v)); + expect(states.at(-1)).toBe(false); + + // Wrong panel name does not flip the flag. + service.openPanel("WrongPanel"); + expect(states.at(-1)).toBe(false); + + // Opening the jupyter panel records that the workflow now has a notebook. + service.openPanel("JupyterNotebookPanel"); + expect(states.at(-1)).toBe(true); + }); + // HTTP fetchNotebookAndMapping it("should return 0 when exists=false", async () => { const resultPromise = firstValueFrom((service as any).fetchNotebookAndMapping(1, 1)); @@ -186,6 +251,9 @@ describe("JupyterPanelService", () => { expect(mockWorkflow.workflowMetaDataChanged).toHaveBeenCalled(); expect(mockNotebook.deleteMapping).toHaveBeenCalledWith("mapping_wid_1"); + // Data-loss guard: switching workflows must never delete a notebook from the + // backend. It only drops the in-memory mapping. + expect(mockNotebook.deleteNotebookAndMapping).not.toHaveBeenCalled(); const req = httpMock.expectOne(r => r.url.includes("/notebook-migration/fetch-notebook-and-mapping")); req.flush({ exists: false }); @@ -391,10 +459,11 @@ describe("JupyterPanelService", () => { expect(state).toBe(false); }); - it("closeJupyterNotebookPanel does not flip visibility or delete the mapping", () => { - // BehaviorSubject's initial value is false; the meaningful assertion is - // that the side effect (deleteMapping) was never called. - service.closeJupyterNotebookPanel(); + it("deleteJupyterNotebook does not call the backend or delete the mapping when disabled", () => { + // When the feature is disabled the method returns early, so neither the + // backend delete nor the local mapping drop should run. + service.deleteJupyterNotebook(); + expect(mockNotebook.deleteNotebookAndMapping).not.toHaveBeenCalled(); expect(mockNotebook.deleteMapping).not.toHaveBeenCalled(); }); 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 ceccff995a..c4f8ffcfbb 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 @@ -99,11 +99,12 @@ export class JupyterPanelService { distinctUntilChanged() ) .subscribe(wid => { - // On every workflow change, close the panel (which also drops the - // outgoing workflow's stale mapping) and clear the highlight index, so a - // switch to a workflow without a stored notebook can't leave the - // previous workflow's highlights active. - this.closeJupyterNotebookPanel(); + // On every workflow change, hide the panel and drop the outgoing + // workflow's stale in-memory mapping, and clear the highlight index, so a + // switch to a workflow without a stored notebook can't leave the previous + // workflow's highlights active. This is local cleanup only: it must never + // delete from the backend, or switching workflows would erase notebooks. + this.hideAndClearLocalState(); this.cellToHighlightMapping = {}; this.jupyterNotebookExists.next(false); // Skip unsaved workflows (wid undefined) and wid 0; both would POST @@ -209,12 +210,46 @@ export class JupyterPanelService { if (!this.enabled) return; if (panelName === "JupyterNotebookPanel") { this.jupyterNotebookPanelVisible.next(true); + // Opening the panel means the current workflow has an associated notebook, so + // surface the toolbar "expand" button (jupyterNotebookExists$) right away. Needed + // after an in-place import where the wid does not change and init() does not re-run + // to detect the notebook. + this.jupyterNotebookExists.next(true); } } - // Close the Jupyter Notebook panel - public closeJupyterNotebookPanel(): void { + // Delete the current workflow's stored notebook from the backend, then hide the + // panel and clear all local notebook state. This is the user-initiated action + // behind the panel's delete button, and is distinct from the workflow-switch + // cleanup (hideAndClearLocalState), which must never touch the backend. + public deleteJupyterNotebook(): void { if (!this.enabled) return; + const wid = this.workflowActionService.getWorkflow().wid; + // Unsaved workflow (wid undefined or the default wid 0): nothing is persisted, + // and a delete POST with such a wid would 500, so just reset local state. + if (!wid) { + this.hideAndClearLocalState(); + this.jupyterNotebookExists.next(false); + this.clearHighlights(); + return; + } + this.notebookMigrationService.deleteNotebookAndMapping(wid).subscribe({ + next: () => { + this.hideAndClearLocalState(); + this.jupyterNotebookExists.next(false); + this.clearHighlights(); + }, + error: (err: unknown) => { + // Keep the panel open on failure so the user sees the notebook wasn't removed. + console.error("Failed to delete Jupyter notebook:", err); + this.notificationService.error("Failed to delete the Jupyter notebook."); + }, + }); + } + + // Hide the panel and drop the current workflow's in-memory mapping. Local only; + // never calls the backend. Used on workflow switch and after a successful delete. + private hideAndClearLocalState(): void { this.jupyterNotebookPanelVisible.next(false); const wid = this.workflowActionService.getWorkflow().wid; if (wid != undefined) { @@ -222,6 +257,24 @@ export class JupyterPanelService { } } + // Unhighlight all operators and links and drop the highlight index, used once + // the notebook is gone so no stale cell-to-operator highlights remain. + private clearHighlights(): void { + this.workflowActionService.unhighlightOperators( + ...this.workflowActionService + .getTexeraGraph() + .getAllOperators() + .map(op => op.operatorID) + ); + this.workflowActionService.unhighlightLinks( + ...this.workflowActionService + .getTexeraGraph() + .getAllLinks() + .map(link => link.linkID) + ); + this.cellToHighlightMapping = {}; + } + // Minimize the Jupyter Notebook panel public minimizeJupyterNotebookPanel(): 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 6de43b4146..6570e47a00 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 @@ -229,6 +229,19 @@ describe("NotebookMigrationService", () => { req.flush({ success: true, message: "stored" }); }); + // deleteNotebookAndMapping + it("should call deleteNotebookAndMapping API with the wid", () => { + let result: any; + service.deleteNotebookAndMapping(7).subscribe(r => (result = r)); + + const req = httpMock.expectOne(req => req.url.includes("/notebook-migration/delete-notebook-and-mapping")); + + expect(req.request.method).toBe("POST"); + expect(req.request.body).toEqual({ wid: 7 }); + req.flush({ success: true, deleted: 1 }); + expect(result).toEqual({ success: true, deleted: 1 }); + }); + // sendToAIGenerateWorkflow (enabled) — drives the NotebookMigrationLLM lifecycle. // The service builds the client through its createMigrationLLM() seam, so stub // that with a plain fake. This keeps the real NotebookMigrationLLM (and its "ai" @@ -323,5 +336,11 @@ describe("NotebookMigrationService", () => { expect(result.success).toBe(false); httpMock.expectNone(req => req.url.includes("/notebook-migration/store-notebook-and-mapping")); }); + + it("deleteNotebookAndMapping emits a disabled result without making an HTTP call", async () => { + const result = await firstValueFrom(service.deleteNotebookAndMapping(7)); + expect(result.success).toBe(false); + httpMock.expectNone(req => req.url.includes("/notebook-migration/delete-notebook-and-mapping")); + }); }); }); 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 7bfd3ef541..3d6600c2a2 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 @@ -48,6 +48,12 @@ interface StoreNotebookResponse { message: string; } +interface DeleteNotebookResponse { + success: boolean; + deleted?: number; + message?: string; +} + @Injectable({ providedIn: "root", }) @@ -209,6 +215,22 @@ export class NotebookMigrationService { return this.http.post<StoreNotebookResponse>(dbAPIUrl, payload, { headers }); } + // Delete the stored notebook and its mapping for a workflow. The backend's + // notebook -> workflow_notebook_mapping FK is ON DELETE CASCADE, and wid alone + // identifies the notebook, so only wid is sent. `deleted` is 1 when a notebook + // was removed, 0 when nothing was stored. + public deleteNotebookAndMapping(wid: number | undefined): Observable<DeleteNotebookResponse> { + if (!this.enabled) { + return of({ success: false, message: "Notebook migration feature is disabled" }); + } + const dbAPIUrl = `${AppSettings.getApiEndpoint()}/notebook-migration/delete-notebook-and-mapping`; + const headers = new HttpHeaders({ "Content-Type": "application/json" }); + + const payload = { wid }; + + return this.http.post<DeleteNotebookResponse>(dbAPIUrl, payload, { headers }); + } + public hasMapping(id: string): boolean { return id in this.mapping; }
