refactor(notebook-migration-service): compute jupyter iframe url per request - #7602
Open
zyratlo wants to merge 3 commits into
Open
refactor(notebook-migration-service): compute jupyter iframe url per request#7602zyratlo wants to merge 3 commits into
zyratlo wants to merge 3 commits into
Conversation
Contributor
Automated Reviewer SuggestionsBased on the
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7602 +/- ##
============================================
- Coverage 89.62% 89.39% -0.24%
+ Complexity 4395 4375 -20
============================================
Files 1177 1177
Lines 46884 46835 -49
Branches 5239 5225 -14
============================================
- Hits 42022 41868 -154
- Misses 3109 3220 +111
+ Partials 1753 1747 -6
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
zyratlo
marked this pull request as ready for review
August 13, 2026 17:53
Contributor
Author
|
/request-review @mengw15 |
Contributor
There was a problem hiding this comment.
Pull request overview
Makes notebook iframe URL generation stateless by deriving it from each request instead of shared process state.
Changes:
- Removes the process-global iframe URL state.
- Adds optional, validated notebook-name selection with a backward-compatible default.
- Adds tests for default, explicit, invalid, and stateless URL generation.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
NotebookMigrationResource.scala |
Computes validated iframe URLs per request and removes shared state. |
NotebookMigrationResourceSpec.scala |
Tests explicit, invalid, default, and stateless iframe URL behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| @Path("/get-jupyter-iframe-url") | ||
| def getJupyterIframeURL(@Auth user: SessionUser): Response = { | ||
| def getJupyterIframeURL( | ||
| @QueryParam("notebookName") notebookName: String, |
Comment on lines
+490
to
+496
| it should "build the iframe URL from an explicit notebook name" in { | ||
| withFakeJupyter(contentsStatus = 201) { | ||
| val resp = NotebookMigrationResource.getJupyterIframeURL("other.ipynb") | ||
| resp.getStatus shouldBe Response.Status.OK.getStatusCode | ||
| resp.getEntity.toString should include("/notebooks/work/other.ipynb") | ||
| } | ||
| } |
Comment on lines
+79
to
+81
| // Default notebook name used when a request does not specify one, so a param-less | ||
| // getJupyterIframeURL call reproduces the URL from before this service became stateless. | ||
| private val defaultNotebookName = "notebook.ipynb" |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this PR?
Makes
notebook-migration-servicestateless so it can later run as a single global instance instead of one instance per user. This is the first backend stage of moving the service onto Texera's "orchestrator services are global, stateful resources are per user" pattern.Today the service keeps a shared
@volatile jupyterIframeURL:set-notebookwrites it andget-jupyter-iframe-urlreads it back. That shared state is only safe because each user happens to run their own pod, and even within one user it lets two browser tabs race. This PR removes the shared state and builds the URL from the request instead.NotebookMigrationResource.scala@volatile var jupyterIframeURLsingleton and the warning comment that documented its per-user-pod assumption. Adds adefaultNotebookNameconstant (notebook.ipynb).getJupyterIframeURLnow takes anotebookNameargument and builds the URL on each call. The name is validated with the same plain.ipynbregexsetNotebookuses, since it now flows straight into the returned URL (blocks path traversal). The argument defaults todefaultNotebookName.setNotebookno longer mutates any shared state; the assignment that wrote the singleton is gone. Its upload behavior is unchanged./get-jupyter-iframe-urlendpoint accepts an optionalnotebookNamequery parameter and falls back to the default when it is absent.The change is backward compatible. The existing frontend calls the endpoint with no query parameter, which resolves to
notebook.ipynb, exactly the URL it received before. No frontend, config, or deployment change is needed in this PR, and no other service or branch consumes the removed state.Any related issues, documentation, discussions?
Closes #7390
Parent-issue #4301
How was this PR tested?
Extends the existing suite in
NotebookMigrationResourceSpec.scala:notebookNameis honored in the returned URL.notebookNameis rejected with 400 before any Jupyter call.setNotebookuploadsother.ipynb, a parameter-lessgetJupyterIframeURLreturns the defaultnotebook.ipynb, proving the result no longer depends on state left bysetNotebook.Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 4.8)