Skip to content

feat(python-notebook-migration): make embedded jupyter notebook read-only - #6945

Open
zyratlo wants to merge 3 commits into
apache:mainfrom
zyratlo:migration-tool-jupyter-read-only
Open

feat(python-notebook-migration): make embedded jupyter notebook read-only#6945
zyratlo wants to merge 3 commits into
apache:mainfrom
zyratlo:migration-tool-jupyter-read-only

Conversation

@zyratlo

@zyratlo zyratlo commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

The notebook migration service generates a Jupyter notebook and embeds it in the Texera app as a reference view of the migrated workflow. It is meant for reading, not editing. This PR configures the embedded Jupyter container so the notebook renders as a clean, read-only view.

Changes, all scoped to the notebook migration service container assets:

  1. custom.css (new): hides the classic Notebook chrome (header with logo and logout, kernel and trust indicators, the File/Edit/View menu, and the toolbar), and fixes a second, non-functional scrollbar by making #site the single scroll container.
  2. Dockerfile: copies custom.css into /home/jovyan/.jupyter/custom/custom.css, next to the existing custom.js.
  3. custom.js: read-only hardening. Sets every code cell's CodeMirror editor to readOnly: "nocursor", calls keyboard_manager.disable() so cells cannot be edited or executed, and overrides MarkdownCell.prototype.unrender so markdown cells stay rendered. The existing click-to-highlight postMessage integration is unchanged.

This does not touch the Texera frontend or backend.

UI Comparison

Before this PR:
2026-07-27_13-38-52

After this PR:
image

Any related issues, documentation, discussions?

Closes #6943
Parent issue #4301

How was this PR tested?

Tested manually by building the container and opening the embedded notebook in the Texera app:

  1. The classic Notebook chrome (header, menu, toolbar, kernel and trust indicators) is hidden; only the notebook cells are visible.
  2. Clicking into a code cell does not put it in edit mode, typing does nothing, and cells cannot be run.
  3. Double-clicking a markdown cell keeps it rendered instead of dropping into its editable source.
  4. Only one working scrollbar is present.

No automated tests were added: the change is static container assets (CSS and client-side JS in the classic Notebook UI) with no code path exercisable by the existing unit test suites.

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

Generated-by: Claude Code (Claude Opus 4.8)

@zyratlo

zyratlo commented Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

/request-review @mengw15

@mengw15
mengw15 requested a review from Copilot July 27, 2026 20:53
@github-actions
github-actions Bot requested a review from mengw15 July 27, 2026 20:59
@github-actions

Copy link
Copy Markdown
Contributor

Automated Reviewer Suggestions

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

  • No candidates found from git blame history.

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

Configures the notebook-migration-service’s embedded classic Jupyter Notebook container to behave as a clean, read-only reference view for migrated workflows, aligning with the intended “view-only” UX for embedded notebooks.

Changes:

  • Adds a custom.css that hides classic Notebook chrome and enforces a single scroll container inside the iframe.
  • Extends the container Dockerfile to ship the new CSS alongside the existing custom.js.
  • Hardens read-only behavior in custom.js by disabling CodeMirror editing, disabling keyboard shortcuts, and preventing Markdown cells from entering edit mode.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
notebook-migration-service/src/main/resources/Dockerfile Copies custom.css into the Jupyter custom directory in the container image.
notebook-migration-service/src/main/resources/custom.js Disables editing/execution interactions and keeps Markdown cells rendered for read-only viewing.
notebook-migration-service/src/main/resources/custom.css Hides classic Notebook UI chrome and fixes the double-scrollbar issue by making #site the sole scroll container.

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

@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.

LGTM

@mengw15
mengw15 added this pull request to the merge queue Jul 27, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jul 27, 2026
@Yicong-Huang

Copy link
Copy Markdown
Contributor

@zyratlo can we add some tests?

@mengw15 can we look into why this PR does not trigger coverage report? thanks

@Yicong-Huang Yicong-Huang 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.

please add some tests to verify the feature.

@zyratlo

zyratlo commented Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

please add some tests to verify the feature.

I can add tests, but I want to discuss what's testable here. This change is all static assets that live in the Jupyter container: custom.css is plain CSS, custom.js is classic-Notebook code that runs against window.Jupyter, CodeMirror, and the textcell module inside the image, and the Dockerfile just copies them in. There's no JS or DOM test setup in this module, only the Scala suites for the service and endpoints.

On testing custom.js the traditional way: it's doable, but the catch is placement, not the file. custom.js is classic-Notebook AMD glue that assumes window.Jupyter, CodeMirror, and the textcell module and exports nothing. I can shim the AMD loader and those globals, fire the notebook_loaded handler, and assert CodeMirror got setOption("readOnly", "nocursor"), keyboard_manager.disable() was called, and unrender was overridden.

Problem: this is a Scala/sbt module with no JS runtime. The only JS runner in the repo is the frontend project. So either:

  1. Put a Vitest test in frontend/. It's the real behavioral test, but it reaches into notebook-migration-service resources and its coverage lands under the frontend flag, not this service's.
  2. Add a Scala spec here that loads the three files and asserts each ships the read-only config. It's a regression guard, not runtime verification, but it lives in the right module and runs in the platform stack that produces this service's coverage.

@github-actions github-actions Bot added the ci changes related to CI label Jul 28, 2026
@codecov-commenter

codecov-commenter commented Jul 28, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.99%. Comparing base (31f1e7b) to head (e29f62a).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #6945   +/-   ##
=========================================
  Coverage     78.99%   78.99%           
  Complexity     3788     3788           
=========================================
  Files          1160     1160           
  Lines         46105    46105           
  Branches       5115     5115           
=========================================
  Hits          36419    36419           
  Misses         8067     8067           
  Partials       1619     1619           
Flag Coverage Δ *Carryforward flag
access-control-service 70.00% <ø> (ø)
agent-service 76.76% <ø> (ø)
amber 72.20% <ø> (ø) Carriedforward from 31f1e7b
computing-unit-managing-service 20.49% <ø> (ø)
config-service 66.66% <ø> (ø)
file-service 67.21% <ø> (ø) Carriedforward from 31f1e7b
frontend 82.99% <ø> (ø) Carriedforward from 31f1e7b
notebook-migration-service 78.94% <ø> (ø)
pyamber 95.38% <ø> (ø)
workflow-compiling-service 26.31% <ø> (ø) Carriedforward from 31f1e7b

*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.

@mengw15

mengw15 commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

@zyratlo can we add some tests?

@mengw15 can we look into why this PR does not trigger coverage report? thanks

The Codecov report is working now, @zyratlo added the platform label to this micro-service

@github-actions

Copy link
Copy Markdown
Contributor

⚠️ Benchmark changes need a look

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

Compared against main 31f1e7b 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 410 0.25 22,947/31,644/31,644 us 🔴 -7.4% / 🔴 +94.9%
🔴 bs=100 sw=10 sl=64 799 0.488 123,072/173,136/173,136 us 🔴 +17.9% / 🔴 +59.4%
bs=1000 sw=10 sl=64 926 0.565 1,074,735/1,150,123/1,150,123 us ⚪ within ±5% / 🔴 +9.9%
Baseline details

Latest main 31f1e7b from same runner

config metric PR latest main 7d avg Δ latest Δ 7d
bs=10 sw=10 sl=64 throughput 410 tuples/sec 442 tuples/sec 767.9 tuples/sec -7.2% -46.6%
bs=10 sw=10 sl=64 MB/s 0.25 MB/s 0.27 MB/s 0.469 MB/s -7.4% -46.7%
bs=10 sw=10 sl=64 p50 22,947 us 21,739 us 12,502 us +5.6% +83.6%
bs=10 sw=10 sl=64 p95 31,644 us 31,528 us 16,234 us +0.4% +94.9%
bs=10 sw=10 sl=64 p99 31,644 us 31,528 us 18,919 us +0.4% +67.3%
bs=100 sw=10 sl=64 throughput 799 tuples/sec 826 tuples/sec 974.8 tuples/sec -3.3% -18.0%
bs=100 sw=10 sl=64 MB/s 0.488 MB/s 0.504 MB/s 0.595 MB/s -3.2% -18.0%
bs=100 sw=10 sl=64 p50 123,072 us 121,033 us 102,449 us +1.7% +20.1%
bs=100 sw=10 sl=64 p95 173,136 us 146,824 us 108,652 us +17.9% +59.4%
bs=100 sw=10 sl=64 p99 173,136 us 146,824 us 116,310 us +17.9% +48.9%
bs=1000 sw=10 sl=64 throughput 926 tuples/sec 934 tuples/sec 1,004 tuples/sec -0.9% -7.8%
bs=1000 sw=10 sl=64 MB/s 0.565 MB/s 0.57 MB/s 0.613 MB/s -0.9% -7.8%
bs=1000 sw=10 sl=64 p50 1,074,735 us 1,066,164 us 999,606 us +0.8% +7.5%
bs=1000 sw=10 sl=64 p95 1,150,123 us 1,136,643 us 1,046,770 us +1.2% +9.9%
bs=1000 sw=10 sl=64 p99 1,150,123 us 1,136,643 us 1,076,937 us +1.2% +6.8%
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,488.22,200,128000,410,0.250,22947.49,31643.58,31643.58
1,100,10,64,20,2503.58,2000,1280000,799,0.488,123072.30,173136.18,173136.18
2,1000,10,64,20,21590.92,20000,12800000,926,0.565,1074735.03,1150123.35,1150123.35

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

Labels

ci changes related to CI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Notebook Migration] Make Jupyter microservice read-only and clean up UI

5 participants