diff --git a/frontend/src/app/dashboard/component/user/user-workflow/user-workflow.component.html b/frontend/src/app/dashboard/component/user/user-workflow/user-workflow.component.html index 492efc53766..d945a21d36e 100644 --- a/frontend/src/app/dashboard/component/user/user-workflow/user-workflow.component.html +++ b/frontend/src/app/dashboard/component/user/user-workflow/user-workflow.component.html @@ -51,6 +51,20 @@

Workflows

nzTheme="outline"> + + - - - Upload Python Jupyter Notebook - - -
- - - + + Selected file: {{ fileName }} + +
+
+
- - Selected file: {{ importForm.get('file')?.value?.name }} - - - - + + + Select Model Type + - - - Select Model Type - + + + + + + + + + - - - - - - + - + + + - - - - - - + - diff --git a/frontend/src/app/workspace/component/notebook-import-modal/notebook-import-modal.component.scss b/frontend/src/app/workspace/component/notebook-import-modal/notebook-import-modal.component.scss index 2f898489821..ed2b53d725d 100644 --- a/frontend/src/app/workspace/component/notebook-import-modal/notebook-import-modal.component.scss +++ b/frontend/src/app/workspace/component/notebook-import-modal/notebook-import-modal.component.scss @@ -57,22 +57,67 @@ } &-upload-row { - display: inline-flex; + display: flex; align-items: center; gap: 8px; button { white-space: normal; + flex: none; } } + &-selected-file { + flex: 1; + min-width: 0; + overflow: hidden; + text-overflow: ellipsis; + white-space: nowrap; + } + &-select { width: 50%; } - &-warning { - display: block; - margin-bottom: 12px; + // Positioning context for the loading overlay, so covering the form does not resize the modal. + &-content { + position: relative; + } + + &-loading { + position: absolute; + inset: 0; + z-index: 1; + // Solid background so the form underneath is fully hidden. + background-color: #fff; + display: flex; + flex-direction: column; + align-items: center; + justify-content: center; + gap: 14px; + padding: 16px 24px 28px; + text-align: center; + } + + &-loading-title { + margin: 8px 0 0; + font-size: 20px; + font-weight: 700; + } + + &-loading-elapsed { + margin: 0; + font-size: 18px; + font-weight: 600; + font-variant-numeric: tabular-nums; + } + + &-loading-text { + margin: 0; + max-width: 480px; + font-size: 16px; + line-height: 1.5; + color: rgba(0, 0, 0, 0.65); } &-footer { diff --git a/frontend/src/app/workspace/component/notebook-import-modal/notebook-import-modal.component.spec.ts b/frontend/src/app/workspace/component/notebook-import-modal/notebook-import-modal.component.spec.ts index eb6d199bd17..2e46f2ee655 100644 --- a/frontend/src/app/workspace/component/notebook-import-modal/notebook-import-modal.component.spec.ts +++ b/frontend/src/app/workspace/component/notebook-import-modal/notebook-import-modal.component.spec.ts @@ -32,7 +32,7 @@ describe("NotebookImportModalComponent", () => { let fixture: ComponentFixture; let component: NotebookImportModalComponent; let notebookMigrationService: NotebookMigrationService; - let modalRef: { close: ReturnType }; + let modalRef: { close: ReturnType; updateConfig: ReturnType }; // The opener-supplied gate; tests set its resolved value to drive close vs stay-open. let requestImport: ReturnType; @@ -56,19 +56,47 @@ describe("NotebookImportModalComponent", () => { } beforeEach(() => { - modalRef = { close: vi.fn() }; + modalRef = { close: vi.fn(), updateConfig: vi.fn() }; requestImport = vi.fn().mockResolvedValue(true); }); - it("renders the warning, diagram, and a usable model select once models load", async () => { + it("renders the diagram and a usable model select once models load", async () => { await createWith(of([{ name: "gpt-4" }])); const root = fixture.nativeElement as HTMLElement; - expect(root.querySelector(".import-modal-warning")).not.toBeNull(); expect(root.querySelector("img[alt='Notebook to Workflow']")).not.toBeNull(); expect(root.querySelector("nz-select")).not.toBeNull(); expect(root.textContent).toContain("Select a model"); }); + it("shows the loading spinner only while a submission is in flight", async () => { + await createWith(of([{ name: "gpt-4" }])); + const spinning = () => (fixture.nativeElement as HTMLElement).querySelector(".ant-spin-spinning") !== null; + expect(spinning()).toBe(false); + + component.isSubmitting = true; + fixture.detectChanges(); + expect(spinning()).toBe(true); + }); + + it("makes the form and footer inert and announces the overlay while submitting", async () => { + await createWith(of([{ name: "gpt-4" }])); + const root = fixture.nativeElement as HTMLElement; + const form = () => root.querySelector(".import-modal-form"); + const footer = () => root.querySelector(".import-modal-footer"); + + expect(form()?.hasAttribute("inert")).toBe(false); + expect(footer()?.hasAttribute("inert")).toBe(false); + + component.isSubmitting = true; + fixture.detectChanges(); + + // While generating, the covered form and footer are pulled out of the focus/a11y tree, + // and the overlay is a live region so its status is announced. + expect(form()?.hasAttribute("inert")).toBe(true); + expect(footer()?.hasAttribute("inert")).toBe(true); + expect(root.querySelector(".import-modal-loading")?.getAttribute("role")).toBe("status"); + }); + it("shows the disabled 'no models available' select when the list is empty", async () => { await createWith(of([])); expect((fixture.nativeElement as HTMLElement).textContent).toContain("No models available"); @@ -125,7 +153,7 @@ describe("NotebookImportModalComponent", () => { }); it("onSubmit keeps the modal open when the opener declines", async () => { - // e.g. the user backed out of the overwrite confirmation. + // e.g. generation failed and the user can retry. requestImport.mockResolvedValue(false); await createWith(of([{ name: "gpt-4" }])); component.importForm.setValue({ file: { name: "x.ipynb" } as NzUploadFile, model: "gpt-4" }); @@ -136,6 +164,80 @@ describe("NotebookImportModalComponent", () => { expect(modalRef.close).not.toHaveBeenCalled(); }); + it("locks the modal shut while generating and restores the close controls on failure", async () => { + let resolveRequest!: (proceed: boolean) => void; + requestImport.mockReturnValue(new Promise(resolve => (resolveRequest = resolve))); + await createWith(of([{ name: "gpt-4" }])); + component.importForm.setValue({ file: { name: "x.ipynb" } as NzUploadFile, model: "gpt-4" }); + + const submitting = component.onSubmit(); + // While generation is pending, the X, mask click, and ESC are disabled. + expect(modalRef.updateConfig).toHaveBeenCalledWith({ + nzClosable: false, + nzMaskClosable: false, + nzKeyboard: false, + }); + + resolveRequest(false); // generation failed + await submitting; + // The modal stayed open, so the close controls are restored. + expect(modalRef.updateConfig).toHaveBeenLastCalledWith({ + nzClosable: true, + nzMaskClosable: true, + nzKeyboard: true, + }); + expect(modalRef.close).not.toHaveBeenCalled(); + }); + + it("computes the elapsed time from the start timestamp as mm:ss", async () => { + await createWith(of([{ name: "gpt-4" }])); + // No generation started yet -> no start timestamp -> zero. + expect(component.formattedElapsedTime).toBe("0:00"); + (component as any).startTime = 1000; + vi.spyOn(Date, "now").mockReturnValue(1000 + 62_000); + expect(component.formattedElapsedTime).toBe("1:02"); + }); + + it("runs the stopwatch off wall-clock time while generating and stops the interval when done", async () => { + let resolveRequest!: (proceed: boolean) => void; + requestImport.mockReturnValue(new Promise(resolve => (resolveRequest = resolve))); + await createWith(of([{ name: "gpt-4" }])); + component.importForm.setValue({ file: { name: "x.ipynb" } as NzUploadFile, model: "gpt-4" }); + + vi.useFakeTimers(); + try { + vi.setSystemTime(new Date(0)); + const submitting = component.onSubmit(); + expect(component.formattedElapsedTime).toBe("0:00"); + + // Advancing the clock (even if the interval were throttled) yields the correct elapsed time. + vi.advanceTimersByTime(75_000); + expect(component.formattedElapsedTime).toBe("1:15"); + + resolveRequest(false); + await submitting; + expect((component as any).timerHandle).toBeNull(); + } finally { + vi.useRealTimers(); + } + }); + + it("has a visibilitychange handler that is safe to call (repaint is driven by the zone event)", async () => { + await createWith(of([{ name: "gpt-4" }])); + expect(() => component.onVisibilityChange()).not.toThrow(); + }); + + it("clears the stopwatch interval on destroy", async () => { + await createWith(of([{ name: "gpt-4" }])); + const clearSpy = vi.spyOn(globalThis, "clearInterval"); + (component as any).timerHandle = setInterval(() => {}, 1000); + + component.ngOnDestroy(); + + expect(clearSpy).toHaveBeenCalled(); + expect((component as any).timerHandle).toBeNull(); + }); + it("ignores a second submit while the first is still pending", async () => { // A pending requestImport models the opener still showing its overwrite confirmation. let resolveRequest!: (proceed: boolean) => void; diff --git a/frontend/src/app/workspace/component/notebook-import-modal/notebook-import-modal.component.ts b/frontend/src/app/workspace/component/notebook-import-modal/notebook-import-modal.component.ts index 70ee5745d1e..7c9ad86cd66 100644 --- a/frontend/src/app/workspace/component/notebook-import-modal/notebook-import-modal.component.ts +++ b/frontend/src/app/workspace/component/notebook-import-modal/notebook-import-modal.component.ts @@ -17,7 +17,7 @@ * under the License. */ -import { Component, inject } from "@angular/core"; +import { Component, HostListener, inject, OnDestroy } from "@angular/core"; import { FormBuilder, FormGroup, Validators, ReactiveFormsModule } from "@angular/forms"; import { NZ_MODAL_DATA, NzModalRef } from "ng-zorro-antd/modal"; import { NzUploadComponent, NzUploadFile } from "ng-zorro-antd/upload"; @@ -25,26 +25,20 @@ import { Observable } from "rxjs"; import { AsyncPipe, NgIf, NgFor, NgOptimizedImage } from "@angular/common"; import { NzFormModule } from "ng-zorro-antd/form"; import { NzSelectModule } from "ng-zorro-antd/select"; -import { NzAlertModule } from "ng-zorro-antd/alert"; +import { NzSpinComponent } from "ng-zorro-antd/spin"; import { NzButtonComponent } from "ng-zorro-antd/button"; import { NzIconDirective } from "ng-zorro-antd/icon"; import { NotebookMigrationService } from "../../service/notebook-migration/notebook-migration.service"; -// Passed in via nzData. The modal delegates "may I proceed?" to the opener so the -// opener can keep the overwrite-confirm and generation logic (and the workflow state it -// needs) without the modal knowing about them. Resolve true to close the modal (the -// import has started), false to keep it open with the user's selection intact. +// Passed in via nzData. requestImport resolves true to close the modal, false to keep it open +// with the user's selection intact (bad file or a retryable failure). export interface NotebookImportModalData { requestImport: (file: NzUploadFile, model: string) => Promise; } /** - * The "AI Generate Workflow from Python Notebook" modal body. It owns the upload form and - * the three model-dropdown states (loading / has models / none). On Submit it delegates the - * decision to proceed to its opener via the requestImport callback (passed in through - * nzData); the opener runs the overwrite-confirm and generation pipeline and the modal - * closes itself only when that resolves true. Mirrors the component-as-nzContent pattern - * used by the other modals opened from the menu (ResultExportationComponent, ...). + * The "AI Generate Workflow from Python Notebook" modal body: the upload form and model dropdown. + * On Submit it hands the file and model to requestImport and shows a loading state until it resolves. */ @Component({ selector: "texera-notebook-import-modal", @@ -58,13 +52,13 @@ export interface NotebookImportModalData { ReactiveFormsModule, NzFormModule, NzSelectModule, - NzAlertModule, + NzSpinComponent, NzUploadComponent, NzButtonComponent, NzIconDirective, ], }) -export class NotebookImportModalComponent { +export class NotebookImportModalComponent implements OnDestroy { private readonly fb = inject(FormBuilder); private readonly modalRef = inject(NzModalRef); private readonly notebookMigrationService = inject(NotebookMigrationService); @@ -90,24 +84,59 @@ export class NotebookImportModalComponent { this.modalRef.close(); } - // Guards against a second submit while the opener callback (which may show an - // overwrite confirmation) is still pending, so a double-click cannot start two imports. + // True while generation runs: guards against a second submit and drives the loading overlay. public isSubmitting = false; + private startTime: number | null = null; + private timerHandle: ReturnType | null = null; + + public ngOnDestroy(): void { + this.stopTimer(); + } + + public get formattedElapsedTime(): string { + const diffMs = this.startTime === null ? 0 : Date.now() - this.startTime; + const totalSeconds = Math.floor(diffMs / 1000); + const minutes = Math.floor(totalSeconds / 60); + const seconds = totalSeconds % 60; + return `${minutes}:${seconds.toString().padStart(2, "0")}`; + } + + // Empty body on purpose: the zone-patched event firing is itself what repaints the stopwatch, + // so it catches up when the user returns to a backgrounded tab. Same reason as the timer below. + @HostListener("document:visibilitychange") + public onVisibilityChange(): void {} + + private startTimer(): void { + this.stopTimer(); + this.startTime = Date.now(); + // Empty body: elapsed is computed from startTime; the zone-patched tick just triggers a repaint. + this.timerHandle = setInterval(() => {}, 1000); + } + + private stopTimer(): void { + if (this.timerHandle !== null) { + clearInterval(this.timerHandle); + this.timerHandle = null; + } + } public async onSubmit(): Promise { if (this.isSubmitting || !this.importForm.valid) return; const file: NzUploadFile = this.importForm.get("file")?.value; const model: string = this.importForm.get("model")?.value; this.isSubmitting = true; + this.startTimer(); + this.modalRef.updateConfig({ nzClosable: false, nzMaskClosable: false, nzKeyboard: false }); try { - // Ask the opener whether to proceed; close only if it does, so cancelling the - // overwrite-confirm leaves this modal open with the selection preserved. + // Close only on success, so a failure leaves the modal open with the selection preserved. if (await this.data.requestImport(file, model)) { this.modalRef.close(); + return; } + this.modalRef.updateConfig({ nzClosable: true, nzMaskClosable: true, nzKeyboard: true }); } finally { - // Re-enable submit if the modal is still open (import declined or it threw). this.isSubmitting = false; + this.stopTimer(); } } } diff --git a/frontend/src/app/workspace/component/workspace.component.spec.ts b/frontend/src/app/workspace/component/workspace.component.spec.ts index f85294e42ad..a17d2dbff28 100644 --- a/frontend/src/app/workspace/component/workspace.component.spec.ts +++ b/frontend/src/app/workspace/component/workspace.component.spec.ts @@ -106,6 +106,7 @@ describe("WorkspaceComponent", () => { disableWorkflowModification: vi.fn(), enableWorkflowModification: vi.fn(), reloadWorkflow: vi.fn(), + autoLayoutWorkflow: vi.fn(), setNewSharedModel: vi.fn(), setWorkflowMetadata: vi.fn(), clearWorkflow: vi.fn(), @@ -253,7 +254,7 @@ describe("WorkspaceComponent", () => { await createFixture(configureRoute({ id: "42" })); fixture.detectChanges(); expect(workflowActionService.setNewSharedModel).toHaveBeenCalledWith(42, { uid: 7 }); - expect(workflowActionService.reloadWorkflow).toHaveBeenCalledWith(stubWorkflow); + expect(workflowActionService.reloadWorkflow).toHaveBeenCalledWith(stubWorkflow, undefined); expect(undoRedoService.clearUndoStack).toHaveBeenCalled(); expect(undoRedoService.clearRedoStack).toHaveBeenCalled(); expect(component.isLoading).toBe(false); @@ -283,7 +284,28 @@ describe("WorkspaceComponent", () => { fixture.detectChanges(); expect(notificationService.error).toHaveBeenCalledWith(expect.stringContaining("broken")); // Workflow still flows through reload — the error is informational, not blocking. - expect(workflowActionService.reloadWorkflow).toHaveBeenCalledWith(brokenWorkflow); + expect(workflowActionService.reloadWorkflow).toHaveBeenCalledWith(brokenWorkflow, undefined); + }); + + it("with autolayout=1: renders synchronously and lays the workflow out once", async () => { + await createFixture(configureRoute({ id: "42" }, { autolayout: "1" })); + const registerSpy = vi.spyOn(component, "registerAutoPersistWorkflow"); + fixture.detectChanges(); + // asyncRendering=false so the operators exist in the graph before layout runs. + expect(workflowActionService.reloadWorkflow).toHaveBeenCalledWith(stubWorkflow, false); + expect(workflowActionService.autoLayoutWorkflow).toHaveBeenCalledTimes(1); + // Auto-persistence must be registered before the layout runs, otherwise the layout's + // position-change events fire into no subscriber and the tidied layout is never saved. + expect(registerSpy.mock.invocationCallOrder[0]).toBeLessThan( + workflowActionService.autoLayoutWorkflow.mock.invocationCallOrder[0] + ); + }); + + it("without autolayout: uses the default rendering and does not lay out", async () => { + await createFixture(configureRoute({ id: "42" })); + fixture.detectChanges(); + expect(workflowActionService.reloadWorkflow).toHaveBeenCalledWith(stubWorkflow, undefined); + expect(workflowActionService.autoLayoutWorkflow).not.toHaveBeenCalled(); }); it("when URL fragment matches an element in the graph, highlights it", async () => { diff --git a/frontend/src/app/workspace/component/workspace.component.ts b/frontend/src/app/workspace/component/workspace.component.ts index 2f95ccccfae..b451527498b 100644 --- a/frontend/src/app/workspace/component/workspace.component.ts +++ b/frontend/src/app/workspace/component/workspace.component.ts @@ -259,9 +259,17 @@ export class WorkspaceComponent implements AfterViewInit, OnInit, OnDestroy { this.workflowActionService.setNewSharedModel(wid, this.userService.getCurrentUser()); // remember URL fragment const fragment = this.route.snapshot.fragment; - // load the fetched workflow - this.workflowActionService.reloadWorkflow(workflow); + // An AI-generated workflow arrives with autolayout=1. Render synchronously + // (asyncRendering = false) so the operators exist before the one-shot layout runs. + const shouldAutoLayout = this.route.snapshot.queryParams.autolayout === "1"; + this.workflowActionService.reloadWorkflow(workflow, shouldAutoLayout ? false : undefined); this.workflowActionService.enableWorkflowModification(); + // Register before autoLayoutWorkflow(): workflowChanged() streams are hot, so subscribing + // afterward would drop the layout's position events and the tidied layout would never save. + this.registerAutoPersistWorkflow(); + if (shouldAutoLayout) { + this.workflowActionService.autoLayoutWorkflow(); + } // set the URL fragment to previous value // because reloadWorkflow will highlight/unhighlight all elements // which will change the URL fragment @@ -284,7 +292,6 @@ export class WorkspaceComponent implements AfterViewInit, OnInit, OnDestroy { this.undoRedoService.clearUndoStack(); this.undoRedoService.clearRedoStack(); this.setLoadingState(false); - this.registerAutoPersistWorkflow(); this.triggerCenter(); }, () => { 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 c4f8ffcfbbc..6a14b98efed 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,7 @@ 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 } from "../notebook-migration/notebook-migration.service"; +import { NotebookMigrationService, notebookMappingKey } from "../notebook-migration/notebook-migration.service"; import { GuiConfigService } from "../../../common/service/gui-config.service"; @Injectable({ @@ -137,7 +137,7 @@ export class JupyterPanelService { switchMap(async (response: any) => { // Only load mapping and workflow if they exist if (response.exists) { - this.notebookMigrationService.setMapping("mapping_wid_" + workflowID, response.mapping); + this.notebookMigrationService.setMapping(notebookMappingKey(workflowID), response.mapping); if ((await this.notebookMigrationService.sendNotebookToJupyter(response.notebook)) == 1) { return 1; @@ -166,7 +166,7 @@ export class JupyterPanelService { console.warn("Workflow ID is undefined. Cannot compute highlight mapping."); return; } - const mappingKey = "mapping_wid_" + wid; + const mappingKey = notebookMappingKey(wid); const mapping = this.notebookMigrationService.getMapping(mappingKey); if (mapping == undefined) { @@ -253,7 +253,7 @@ export class JupyterPanelService { this.jupyterNotebookPanelVisible.next(false); const wid = this.workflowActionService.getWorkflow().wid; if (wid != undefined) { - this.notebookMigrationService.deleteMapping("mapping_wid_" + wid); + this.notebookMigrationService.deleteMapping(notebookMappingKey(wid)); } } @@ -285,7 +285,7 @@ export class JupyterPanelService { public openJupyterNotebookPanel(): void { if (!this.enabled) return; const wid = this.workflowActionService.getWorkflow().wid; - const mappingKey = "mapping_wid_" + wid; + const mappingKey = notebookMappingKey(wid); // Check if there is corresponding mapping data if (wid === undefined || !this.notebookMigrationService.hasMapping(mappingKey)) { this.notificationService.warning("No Jupyter notebook associated with this workflow."); @@ -359,7 +359,7 @@ export class JupyterPanelService { return; } - const mappingKey = "mapping_wid_" + wid; + const mappingKey = notebookMappingKey(wid); const mappingEntry = this.notebookMigrationService.getMapping(mappingKey); if (!mappingEntry) { diff --git a/frontend/src/app/workspace/service/notebook-migration/migration-llm.spec.ts b/frontend/src/app/workspace/service/notebook-migration/migration-llm.spec.ts index 40714dd64a5..c1ef22e6afc 100644 --- a/frontend/src/app/workspace/service/notebook-migration/migration-llm.spec.ts +++ b/frontend/src/app/workspace/service/notebook-migration/migration-llm.spec.ts @@ -17,7 +17,7 @@ * under the License. */ -import { NotebookMigrationLLM, Notebook } from "./migration-llm"; +import { NotebookMigrationLLM, Notebook, LLM_REQUEST_TIMEOUT_MS } from "./migration-llm"; import { GuiConfigService } from "../../../common/service/gui-config.service"; import { WorkflowUtilService } from "../workflow-graph/util/workflow-util.service"; import { AuthService } from "../../../common/service/user/auth.service"; @@ -371,7 +371,8 @@ describe("NotebookMigrationLLM", () => { it("returns true and pings the model with a capped token budget on success", async () => { const ok = await makeLLM().verifyConnection(); expect(ok).toBe(true); - expect(callModelSpy).toHaveBeenCalledWith([{ role: "user", content: "ping" }], 10); + // The ping goes through the timeout wrapper, so it also carries an abort signal. + expect(callModelSpy).toHaveBeenCalledWith([{ role: "user", content: "ping" }], 10, expect.any(AbortSignal)); }); it("returns false and logs the error when the ping fails", async () => { @@ -385,6 +386,66 @@ describe("NotebookMigrationLLM", () => { }); }); + describe("callModel transport", () => { + it("forwards messages and the abort signal to generateText and returns its text", async () => { + // Exercise the real callModel body (the ai-SDK seam every other test stubs) with a minimal + // LanguageModelV2 fake, so no network call and no "ai" module mock is involved. + callModelSpy.mockRestore(); + const llm = makeLLM(); + (llm as any).model = { + specificationVersion: "v2", + provider: "mock", + modelId: "mock", + supportedUrls: {}, + doGenerate: async () => ({ + content: [{ type: "text", text: "pong" }], + finishReason: "stop", + usage: { inputTokens: 1, outputTokens: 1, totalTokens: 2 }, + warnings: [], + }), + }; + + const result = await (llm as any).callModel( + [{ role: "user", content: "ping" }], + 10, + new AbortController().signal + ); + + expect(result.text).toBe("pong"); + }); + }); + + describe("request timeout", () => { + it("rejects a stalled model call once the timeout elapses so the caller can recover", async () => { + vi.useFakeTimers(); + try { + // A request that never settles: only the timeout can end it. + callModelSpy.mockReturnValue(new Promise<{ text: string }>(() => {})); + const pending = makeLLM().convertNotebookToWorkflow({ cells: [codeCell("AAA", "a = 1")] }); + // Surface the rejection instead of letting it float as unhandled while timers advance. + const assertion = expect(pending).rejects.toThrow(/timed out/); + await vi.advanceTimersByTimeAsync(LLM_REQUEST_TIMEOUT_MS); + await assertion; + } finally { + vi.useRealTimers(); + } + }); + + it("does not reject a call that resolves before the timeout", async () => { + vi.useFakeTimers(); + try { + mockResponses( + JSON.stringify({ code: { UDF1: "code1" }, edges: [], outputs: { UDF1: ["a"] } }), + JSON.stringify({ UDF1: ["AAA"] }) + ); + const result = await makeLLM().convertNotebookToWorkflow({ cells: [codeCell("AAA", "a = 1")] }); + expect(JSON.parse(result).workflowJSON.operators).toHaveLength(1); + } finally { + vi.useRealTimers(); + } + }); + }); + describe("initialization guards", () => { it("convertNotebookToWorkflow() rejects when the session is enabled but not initialized", async () => { const llm = makeUninitializedLLM(); diff --git a/frontend/src/app/workspace/service/notebook-migration/migration-llm.ts b/frontend/src/app/workspace/service/notebook-migration/migration-llm.ts index 6a1ba8b4894..ebfc82907f5 100644 --- a/frontend/src/app/workspace/service/notebook-migration/migration-llm.ts +++ b/frontend/src/app/workspace/service/notebook-migration/migration-llm.ts @@ -79,6 +79,8 @@ interface CombinedMapping { * Terminal UDFs (no outgoing edge) declare their outputs as `string` so the result panel * renders viewable values rather than opaque binary blobs. */ +export const LLM_REQUEST_TIMEOUT_MS = 10 * 60 * 1000; + @Injectable() export class NotebookMigrationLLM { private model: any; @@ -175,7 +177,7 @@ export class NotebookMigrationLLM { } try { - await this.callModel([{ role: "user", content: "ping" }], 10); + await this.callModelWithTimeout([{ role: "user", content: "ping" }], 10); return true; } catch (err) { @@ -184,13 +186,31 @@ export class NotebookMigrationLLM { } } - // Seam over the `ai` transport. Specs stub this by spying the method, instead of - // mocking the "ai" module — module mocks are unreliable in the Angular unit-test - // builder when "ai" is also loaded by a sibling spec (e.g. via - // NotebookMigrationService), which silently breaks the mock and hangs these - // tests on a real network call. - protected callModel(messages: ModelMessage[], maxOutputTokens?: number): Promise<{ text: string }> { - return generateText({ model: this.model, messages, maxOutputTokens }); + // Seam over the `ai` transport. Specs spy this method rather than mocking the "ai" module: + // a module mock leaks across specs that share the "ai" import and hangs on a real network call. + protected callModel( + messages: ModelMessage[], + maxOutputTokens?: number, + abortSignal?: AbortSignal + ): Promise<{ text: string }> { + return generateText({ model: this.model, messages, maxOutputTokens, abortSignal }); + } + + // Wraps callModel with a hard timeout so a stalled request cannot hang forever. The abort + // cancels the underlying request when the transport honors it; the race guarantees rejection + // even if it does not, so the caller's error path always runs. + private callModelWithTimeout(messages: ModelMessage[], maxOutputTokens?: number): Promise<{ text: string }> { + const controller = new AbortController(); + let timer: ReturnType | undefined; + const timeout = new Promise((_resolve, reject) => { + timer = setTimeout(() => { + controller.abort(); + reject(new Error(`LLM request timed out after ${LLM_REQUEST_TIMEOUT_MS} ms`)); + }, LLM_REQUEST_TIMEOUT_MS); + }); + return Promise.race([this.callModel(messages, maxOutputTokens, controller.signal), timeout]).finally(() => + clearTimeout(timer) + ); } /** @@ -207,7 +227,7 @@ export class NotebookMigrationLLM { content: prompt, }); - const result = await this.callModel(this.messages); + const result = await this.callModelWithTimeout(this.messages); this.messages.push({ role: "assistant", 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 6570e47a00d..2b47325fdbc 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 } from "./notebook-migration.service"; +import { NotebookMigrationService, notebookMappingKey } 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"; @@ -220,15 +220,21 @@ describe("NotebookMigrationService", () => { }); // storeNotebookAndMapping - it("should call storeNotebookAndMapping API", () => { - service.storeNotebookAndMapping(1, 1, {}, {}).subscribe(); + it("should call storeNotebookAndMapping API with the default vid", () => { + service.storeNotebookAndMapping(1, {}, {}).subscribe(); const req = httpMock.expectOne(req => req.url.includes("/notebook-migration/store-notebook-and-mapping")); expect(req.request.method).toBe("POST"); + expect(req.request.body.wid).toBe(1); + expect(req.request.body.vid).toBe(1); req.flush({ success: true, message: "stored" }); }); + it("notebookMappingKey builds the in-memory cache key from the wid", () => { + expect(notebookMappingKey(42)).toBe("mapping_wid_42"); + }); + // deleteNotebookAndMapping it("should call deleteNotebookAndMapping API with the wid", () => { let result: any; @@ -332,7 +338,7 @@ describe("NotebookMigrationService", () => { }); it("storeNotebookAndMapping emits a disabled result without making an HTTP call", async () => { - const result = await firstValueFrom(service.storeNotebookAndMapping(1, 1, {}, {})); + const result = await firstValueFrom(service.storeNotebookAndMapping(1, {}, {})); expect(result.success).toBe(false); httpMock.expectNone(req => req.url.includes("/notebook-migration/store-notebook-and-mapping")); }); @@ -343,4 +349,50 @@ describe("NotebookMigrationService", () => { httpMock.expectNone(req => req.url.includes("/notebook-migration/delete-notebook-and-mapping")); }); }); + + // parseAndTagNotebook (reads + validates + uuid-tags an uploaded .ipynb) + describe("parseAndTagNotebook", () => { + it("parses a valid notebook and tags every cell with a uuid", async () => { + const notebook = { + cells: [ + { cell_type: "code", source: "print(1)", metadata: {} }, + // No metadata: the parser must create it before setting the uuid. + { cell_type: "markdown", source: "# title" }, + ], + }; + const file = new File([JSON.stringify(notebook)], "analysis.ipynb"); + + const parsed = await service.parseAndTagNotebook(file); + + expect(parsed.cells.length).toBe(2); + expect(parsed.cells[0].metadata?.uuid).toBeTruthy(); + expect(parsed.cells[1].metadata?.uuid).toBeTruthy(); + }); + + it("rejects when the notebook is not valid JSON", async () => { + const file = new File(["not json"], "x.ipynb"); + await expect(service.parseAndTagNotebook(file)).rejects.toThrow(); + }); + + it("rejects a notebook without a cells array", async () => { + const file = new File([JSON.stringify({ nbformat: 4 })], "x.ipynb"); + await expect(service.parseAndTagNotebook(file)).rejects.toThrow(/Invalid notebook structure/); + }); + + it("rejects when the file content is not a string", async () => { + vi.spyOn(FileReader.prototype, "readAsText").mockImplementation(function (this: any) { + // result is a getter-only property, so shadow it with an own non-string value. + Object.defineProperty(this, "result", { value: null, configurable: true }); + this.onload?.(new ProgressEvent("load")); + }); + await expect(service.parseAndTagNotebook(new File([""], "x.ipynb"))).rejects.toThrow(/not a valid string/); + }); + + it("rejects when the file cannot be read", async () => { + vi.spyOn(FileReader.prototype, "readAsText").mockImplementation(function (this: any) { + this.onerror?.(new ProgressEvent("error")); + }); + await expect(service.parseAndTagNotebook(new File([""], "x.ipynb"))).rejects.toThrow(/Failed to read/); + }); + }); }); 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 3d6600c2a2e..5c636240f36 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 @@ -24,7 +24,9 @@ import { HttpClient, HttpHeaders } from "@angular/common/http"; import { NotificationService } from "src/app/common/service/notification/notification.service"; import { GuiConfigService } from "../../../common/service/gui-config.service"; import { WorkflowUtilService } from "../workflow-graph/util/workflow-util.service"; +import { WorkflowContent } from "../../../common/type/workflow"; import { catchError, firstValueFrom, map, Observable, of } from "rxjs"; +import { v4 as uuidv4 } from "uuid"; interface LiteLLMModel { id: string; @@ -38,7 +40,7 @@ interface LiteLLMModelsResponse { object: string; } -interface MappingContent { +export interface MappingContent { cell_to_operator: Record; operator_to_cell: Record; } @@ -54,6 +56,11 @@ interface DeleteNotebookResponse { message?: string; } +// Single source of truth for the mapping cache key, shared with JupyterPanelService so it can't drift. +export function notebookMappingKey(wid: number | undefined): string { + return "mapping_wid_" + wid; +} + @Injectable({ providedIn: "root", }) @@ -86,7 +93,10 @@ export class NotebookMigrationService { ); } - public async sendToAIGenerateWorkflow(notebookContent: Notebook, modelType: string) { + public async sendToAIGenerateWorkflow( + notebookContent: Notebook, + modelType: string + ): Promise<{ workflowContent: WorkflowContent; mappingContent: MappingContent }> { if (!this.enabled) throw new Error("Notebook migration feature is disabled"); const migrationLLM = this.createMigrationLLM(); // initialize() defaults to the user's Texera JWT via AuthService.getAccessToken(). @@ -195,9 +205,9 @@ export class NotebookMigrationService { public storeNotebookAndMapping( wid: number | undefined, - vid: number = 1, mappingContent: any, - notebookContent: any + notebookContent: any, + vid: number = 1 ): Observable { if (!this.enabled) { return of({ success: false, message: "Notebook migration feature is disabled" }); @@ -246,4 +256,35 @@ export class NotebookMigrationService { public deleteMapping(id: string): void { delete this.mapping[id]; } + + // Reads and parses an .ipynb file, then tags each cell with a uuid (the mapping keys off these). + // Rejects on a read error, invalid JSON, or a missing cells array. Uses FileReader rather than + // file.text() because jsdom (the test environment) does not implement Blob/File.text(). + public parseAndTagNotebook(file: File): Promise { + return new Promise((resolve, reject) => { + const reader = new FileReader(); + reader.onerror = () => reject(new Error("Failed to read the notebook file.")); + reader.onload = () => { + try { + if (typeof reader.result !== "string") { + throw new Error("File content is not a valid string."); + } + const notebook = JSON.parse(reader.result) as Notebook; + if (!notebook || !Array.isArray(notebook.cells)) { + throw new Error("Invalid notebook structure."); + } + for (const cell of notebook.cells) { + if (!cell.metadata) { + cell.metadata = {}; + } + cell.metadata.uuid = uuidv4(); + } + resolve(notebook); + } catch (error) { + reject(error); + } + }; + reader.readAsText(file); + }); + } }