Skip to content

feat(python-notebook-migration, frontend): add Jupyter notebook iframe panel component - #5271

Open
zyratlo wants to merge 24 commits into
apache:mainfrom
zyratlo:migration-tool-jupyter-panel
Open

feat(python-notebook-migration, frontend): add Jupyter notebook iframe panel component#5271
zyratlo wants to merge 24 commits into
apache:mainfrom
zyratlo:migration-tool-jupyter-panel

Conversation

@zyratlo

@zyratlo zyratlo commented May 28, 2026

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

Introduces JupyterNotebookPanelComponent, the draggable iframe shell that hosts JupyterLab inside the Texera workspace. It consumes the visibility surface added in migration-tool-panel-controls (jupyterNotebookPanelVisible$ / closeJupyterNotebookPanel / minimizeJupyterNotebookPanel) and registers itself with the panel service via setIframeRef once its ViewChild is in the DOM.

  • frontend/src/app/workspace/component/jupyter-notebook-panel/jupyter-notebook-panel.component.ts
    • JupyterNotebookPanelComponent
      • ngOnInit subscribes to jupyterPanelService.jupyterNotebookPanelVisible$; on each emission it sets isVisible, and when visibility flips to true it fetches the iframe URL from notebookMigrationService.getJupyterIframeURL(), runs it through DomSanitizer.bypassSecurityTrustResourceUrl, stores the result in jupyterUrl, and calls checkIframeRef()
      • ngAfterViewInit calls checkIframeRef(), which registers the iframe with the panel service via setIframeRef (deferred by a setTimeout and guarded on isVisible and the iframeRef being present)
      • closePanel() calls jupyterPanelService.closeJupyterNotebookPanel()
      • minimizePanel() sets isVisible to false and calls jupyterPanelService.minimizeJupyterNotebookPanel().
  • frontend/src/app/workspace/component/jupyter-notebook-panel/jupyter-notebook-panel.component.html
    • cdkDrag-enabled panel with header (title + close/minimize buttons; close uses nz-popconfirm for a "delete" confirmation)
    • <iframe [src]="jupyterUrl"> body bound to the sanitized URL, inside a panel container gated on *ngIf="isVisible".
  • frontend/src/app/workspace/component/jupyter-notebook-panel/jupyter-notebook-panel.component.scss
    • fixed-position panel styling, drag handle visuals, iframe sizing.
  • frontend/src/app/workspace/component/jupyter-notebook-panel/jupyter-notebook-panel.component.spec.ts
  • frontend/src/app/app.module.ts
    • imports JupyterNotebookPanelComponent and adds it to the @NgModule.declarations array.

Note on the close/delete button

The panel's close/delete button currently hides the panel and clears the in-memory notebook mapping. It does not yet delete the notebook from the database. Full end-to-end deletion (a backend delete endpoint, removing the persisted notebook and mapping rows, resetting the notebook-exists signal, and clearing the notebook loaded in Jupyter) is intentionally out of scope for this PR, which covers only the panel component.

That work will land in a follow-up PR merged after this one. The button's "delete" wording and icon are left as-is here so the follow-up does not need to churn them back; they become accurate once the deletion feature merges.

Any related issues, documentation, discussions?

Closes #5270
Parent issue #4301

How was this PR tested?

jupyter-notebook-panel.component.spec.ts

  • mocks JupyterPanelService (via a controllable Subject<boolean> for the visibility observable) and NotebookMigrationService (returning a fixture URL)
  • Covers visibility transitions, URL fetch + sanitization, ref registration, button delegation.

Was this PR authored or co-authored using generative AI tooling?

Generated-by: Claude Code (Claude Opus 4.8)

@github-actions github-actions Bot added the frontend Changes related to the frontend GUI label May 28, 2026
@zyratlo zyratlo changed the title added jupyter notebook panel iframe logic feat(python-notebook-migration, frontend): add Jupyter notebook iframe panel component May 28, 2026
@codecov-commenter

codecov-commenter commented Jun 1, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 95.83333% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.60%. Comparing base (51b6af3) to head (44679a9).

Files with missing lines Patch % Lines
...tebook-panel/jupyter-notebook-panel.component.html 87.50% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #5271      +/-   ##
============================================
+ Coverage     79.57%   79.60%   +0.02%     
  Complexity     3832     3832              
============================================
  Files          1160     1162       +2     
  Lines         46192    46210      +18     
  Branches       5145     5140       -5     
============================================
+ Hits          36757    36784      +27     
+ Misses         7797     7796       -1     
+ Partials       1638     1630       -8     
Flag Coverage Δ *Carryforward flag
access-control-service 70.00% <ø> (ø) Carriedforward from 2ec3b0a
agent-service 77.42% <ø> (ø) Carriedforward from 2ec3b0a
amber 73.07% <ø> (+0.01%) ⬆️ Carriedforward from 2ec3b0a
computing-unit-managing-service 20.49% <ø> (ø) Carriedforward from 2ec3b0a
config-service 66.66% <ø> (ø) Carriedforward from 2ec3b0a
file-service 66.80% <ø> (ø) Carriedforward from 2ec3b0a
frontend 83.11% <95.83%> (+0.03%) ⬆️
notebook-migration-service 78.69% <ø> (-0.20%) ⬇️ Carriedforward from 2ec3b0a
pyamber 97.36% <ø> (+0.04%) ⬆️ Carriedforward from 2ec3b0a
workflow-compiling-service 26.31% <ø> (ø) Carriedforward from 2ec3b0a

*This pull request uses carry forward flags. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@zyratlo
zyratlo marked this pull request as draft June 5, 2026 21:30
@github-actions

github-actions Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Automated Reviewer Suggestions

Based on the git blame history of the changed files, we recommend the following reviewers:

  • Contributors with relevant context: @bobbai00, @kunwp1
    You can notify them by mentioning @bobbai00, @kunwp1 in a comment.

zyratlo added a commit to zyratlo/texera that referenced this pull request Jul 23, 2026
adopt main's reviewed versions for the already merged migration slices: backend notebook-migration-service, frontend migration service, config, build, and sql.

pull the reviewed open PR content from its branches: jupyter panel visibility surface and mini-map button (apache#5265), jupyter notebook iframe panel (apache#5271), and the AI generate workflow modal and panel host (apache#5273).

reconcile app.module.ts by keeping main's JupyterPanelService bootstrap listener and registering JupyterNotebookPanelComponent.
@zyratlo

zyratlo commented Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

/request-review @mengw15

@github-actions
github-actions Bot requested a review from mengw15 July 27, 2026 23:20
@zyratlo
zyratlo marked this pull request as ready for review July 27, 2026 23:24
@github-actions github-actions Bot added the ci changes related to CI label Jul 27, 2026
@github-actions

Copy link
Copy Markdown
Contributor

⚠️ Benchmark changes need a look

🟢 0 better · 🔴 7 worse · ⚪ 8 noise (<±5%) · 0 without baseline

Compared against main 49f9e2c benchmarked on this same runner, so the delta is largely free of cross-runner hardware noise. The "7d avg" column still reflects the gh-pages dashboard. Treat <±5% as noise unless repeated.

Dashboard · Run

config throughput MB/s latency max Δ latest / 7d
🔴 bs=10 sw=10 sl=64 394 0.24 24,637/34,363/34,363 us 🔴 +20.8% / 🔴 +111.7%
🔴 bs=100 sw=10 sl=64 828 0.506 119,816/152,100/152,100 us 🔴 +9.9% / 🔴 +40.0%
bs=1000 sw=10 sl=64 946 0.578 1,059,104/1,104,183/1,104,183 us ⚪ within ±5% / 🔴 +6.0%
Baseline details

Latest main 49f9e2c from same runner

config metric PR latest main 7d avg Δ latest Δ 7d
bs=10 sw=10 sl=64 throughput 394 tuples/sec 463 tuples/sec 767.9 tuples/sec -14.9% -48.7%
bs=10 sw=10 sl=64 MB/s 0.24 MB/s 0.282 MB/s 0.469 MB/s -14.9% -48.8%
bs=10 sw=10 sl=64 p50 24,637 us 20,396 us 12,502 us +20.8% +97.1%
bs=10 sw=10 sl=64 p95 34,363 us 32,329 us 16,234 us +6.3% +111.7%
bs=10 sw=10 sl=64 p99 34,363 us 32,329 us 18,919 us +6.3% +81.6%
bs=100 sw=10 sl=64 throughput 828 tuples/sec 842 tuples/sec 974.8 tuples/sec -1.7% -15.1%
bs=100 sw=10 sl=64 MB/s 0.506 MB/s 0.514 MB/s 0.595 MB/s -1.6% -15.0%
bs=100 sw=10 sl=64 p50 119,816 us 117,517 us 102,449 us +2.0% +17.0%
bs=100 sw=10 sl=64 p95 152,100 us 138,351 us 108,652 us +9.9% +40.0%
bs=100 sw=10 sl=64 p99 152,100 us 138,351 us 116,310 us +9.9% +30.8%
bs=1000 sw=10 sl=64 throughput 946 tuples/sec 940 tuples/sec 1,004 tuples/sec +0.6% -5.8%
bs=1000 sw=10 sl=64 MB/s 0.578 MB/s 0.574 MB/s 0.613 MB/s +0.7% -5.7%
bs=1000 sw=10 sl=64 p50 1,059,104 us 1,066,446 us 999,606 us -0.7% +6.0%
bs=1000 sw=10 sl=64 p95 1,104,183 us 1,106,522 us 1,046,770 us -0.2% +5.5%
bs=1000 sw=10 sl=64 p99 1,104,183 us 1,106,522 us 1,076,937 us -0.2% +2.5%
Raw CSV
config_idx,batch_size,schema_width,string_len,num_batches,total_ms,total_tuples,total_bytes,tuples_per_sec,mb_per_sec,lat_p50_us,lat_p95_us,lat_p99_us
0,10,10,64,20,508.16,200,128000,394,0.240,24637.34,34362.92,34362.92
1,100,10,64,20,2414.25,2000,1280000,828,0.506,119816.24,152099.96,152099.96
2,1000,10,64,20,21134.48,20000,12800000,946,0.578,1059104.45,1104183.35,1104183.35

@zyratlo
zyratlo force-pushed the migration-tool-jupyter-panel branch from 4d27799 to 6cb1ef4 Compare July 28, 2026 00:01
@github-actions github-actions Bot removed the ci changes related to CI label Jul 28, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a new Angular workspace UI component intended to host JupyterLab inside Texera via a draggable/resizable iframe panel, wired to the existing JupyterPanelService visibility surface and accompanied by unit tests.

Changes:

  • Introduces JupyterNotebookPanelComponent (TS/HTML/SCSS) to render a draggable iframe-based Jupyter panel and delegate close/minimize actions to JupyterPanelService.
  • Adds a component-level spec file to cover visibility transitions, URL fetch/sanitization, iframe ref registration, and button delegation.
  • Registers the new component in AppModule.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
frontend/src/app/workspace/component/jupyter-notebook-panel/jupyter-notebook-panel.component.ts Implements visibility subscription, URL fetching + sanitization, and iframe ref registration for the Jupyter panel.
frontend/src/app/workspace/component/jupyter-notebook-panel/jupyter-notebook-panel.component.html Adds draggable panel markup with header controls and iframe binding.
frontend/src/app/workspace/component/jupyter-notebook-panel/jupyter-notebook-panel.component.scss Provides panel positioning, drag header styling, and iframe sizing styles.
frontend/src/app/workspace/component/jupyter-notebook-panel/jupyter-notebook-panel.component.spec.ts Adds unit tests for visibility, URL handling, iframe ref registration, and control actions.
frontend/src/app/app.module.ts Registers the new panel component with the application module.
Comments suppressed due to low confidence (2)

frontend/src/app/workspace/component/jupyter-notebook-panel/jupyter-notebook-panel.component.spec.ts:98

  • Avoid asserting on SafeResourceUrl stringification here; instead spy on DomSanitizer.bypassSecurityTrustResourceUrl and assert that the component stores the returned SafeResourceUrl when the panel becomes visible.
  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(component.jupyterUrl.toString()).toContain("http://localhost:8888");
  });

frontend/src/app/workspace/component/jupyter-notebook-panel/jupyter-notebook-panel.component.spec.ts:120

  • Asserting that the rendered iframe src contains the raw URL is brittle (DomSanitizer serialization is not stable). Just assert that the iframe exists and that the [src] binding produced a src attribute, similar to FlarumComponent tests.
  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();
    expect(iframe.getAttribute("src")).toContain("http://localhost:8888");
  });

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread frontend/src/app/app.module.ts

@mengw15 mengw15 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left one comment

<button
class="delete-button"
nz-popconfirm
nzPopconfirmTitle="Confirm to delete Jupyter notebook"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This button says "Confirm to delete Jupyter notebook", but closePanel()closeJupyterNotebookPanel() only hides the panel and drops the in-memory mapping — the DB rows stay. That leaves a state the user can't get out of from the UI:

  1. Click delete → mapping cleared, panel hidden. jupyterNotebookExists$ is not reset (it's only set in init()), so feat(python-notebook-migration, frontend): add AI-generate-workflow modal, panel host, and LLM loading spinner #5273's menu button stays visible.
  2. Click that button → openJupyterNotebookPanel() checks hasMapping(), finds nothing, and warns "No Jupyter notebook associated with this workflow" instead of opening.
  3. The notebook is still in the DB; the only way back is to switch workflows and return, which re-fetches and restores the mapping.

So today "delete" doesn't delete, but it does break reopening for the rest of the session. Deferring the real backend delete is fine — #4301 lists it as a requirement ("can also delete the notebook associated with the workflow"). Could you link the issue tracking it from the description and here?

A few things that might be worth capturing in that issue, since they aren't obvious from the current code:

  • closeJupyterNotebookPanel() is also called by init() on every workflow change. Dropping the in-memory mapping there is right (it's just cache eviction — fetchNotebookAndMapping restores it on the way back), but a backend delete can't live in the same method, or switching workflows would destroy the stored notebook. Splitting "hide the panel" from "delete the notebook" keeps the two apart.
  • notebook.wid is UNIQUE and workflow_notebook_mapping has FOREIGN KEY (wid, nid) ... ON DELETE CASCADE, so a single DELETE FROM notebook WHERE wid = ? covers both tables.
  • Beyond the DB rows: the in-memory mapping, jupyterNotebookExists$, cellToHighlightMapping, and the notebook.ipynb still sitting in the Jupyter container all need clearing — and ideally only after the backend confirms, so a failed request doesn't leave the same dead end.

Not blocking.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about this more — since the backend has no delete endpoint yet, would it be cleaner to either drop the delete button from this PR (minimize alone is enough for now) or land the backend delete first?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes more sense to remove the close/delete from this PR and add it in a separate one with the rest of the backend delete logic. So this PR only has the minimize feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

frontend Changes related to the frontend GUI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Notebook Migration] Add Jupyter notebook iframe panel component

4 participants