feat(notebook-migration, frontend): upload notebooks under a per-work… - #7738
feat(notebook-migration, frontend): upload notebooks under a per-work…#7738zyratlo wants to merge 2 commits into
Conversation
Automated Reviewer SuggestionsBased on the
|
|
/request-review @mengw15 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7738 +/- ##
=========================================
Coverage 91.03% 91.04%
Complexity 4459 4459
=========================================
Files 1174 1174
Lines 47139 47146 +7
Branches 5284 5287 +3
=========================================
+ Hits 42915 42922 +7
Misses 2549 2549
Partials 1675 1675
*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:
|
There was a problem hiding this comment.
Pull request overview
Adds per-workflow Jupyter notebook filenames to prevent workflows from overwriting one another.
Changes:
- Derives notebook filenames from workflow IDs.
- Uses the same naming scheme for uploads and iframe URLs.
- Updates component wiring and tests.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
notebook-migration.service.ts |
Adds filename derivation and request parameters. |
notebook-migration.service.spec.ts |
Tests filenames and HTTP payloads. |
jupyter-panel.service.ts |
Coordinates workflow-specific upload and iframe names. |
jupyter-panel.service.spec.ts |
Tests workflow filename forwarding. |
jupyter-notebook-panel.component.ts |
Fetches iframe URLs through the panel service. |
jupyter-notebook-panel.component.spec.ts |
Updates component service mocks and assertions. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
What changes were proposed in this PR?
Uploads each workflow's notebook to Jupyter under a per-workflow filename instead of a single shared
notebook.ipynb.Before this change the frontend always uploaded to
work/notebook.ipynb. That was safe across users (each runs their own pod) but not across one user's workflows: every workflow wrote to the same file, so opening a second workflow overwrote the first, and because nothing writes back from Jupyter, any edits made in the panel were lost. Two tabs on different workflows also collided on the same file. This PR keys the notebook file on the workflow id (notebook_<wid>.ipynb) so each workflow has its own.The backend already accepts this (from #7602):
get-jupyter-iframe-urltakes an optionalnotebookNamequery param andset-notebookaccepts any[A-Za-z0-9._-]+\.ipynbname, whichnotebook_<wid>.ipynbsatisfies. No backend change is needed.NotebookMigrationServicenotebookFileName(wid)helper (mirrors the existingnotebookMappingKey) that returnsnotebook_<wid>.ipynb, or the default when there is no wid.sendNotebookToJupyter(notebookData, notebookName)takes the name instead of hardcoding it.getJupyterIframeURL(notebookName?)sends the name as thenotebookNamequery param when given, and omits it otherwise so the backend default still applies.JupyterPanelService(owns the name)currentNotebookFileName()that derives the filename from the current workflow's wid, and uses it for both the upload and the iframe fetch so the two can never derive different names.getJupyterIframeURLForWorkflow()that the panel calls to get the URL for the current workflow's notebook.JupyterNotebookPanelComponent(view)jupyterPanelService.getJupyterIframeURLForWorkflow()and drops its now-unused direct dependency onNotebookMigrationService.Because the upload and the iframe fetch both go through
currentNotebookFileName(), the file that is written and the file the panel requests are always the same, and switching workflows produces a distinctnotebook_<wid>.ipynbrather than overwriting a shared one.Any related issues, documentation, discussions?
Closes #7671
Parent issue #4301
Follow-up: deleting a notebook now leaves its
notebook_<wid>.ipynbfile in the Jupyter pod, sincedeleteNotebookAndMappingonly removes the database rows. This was self-limiting under the old single-file scheme. Tracked in #7737How was this PR tested?
notebook-migration.service.spec.ts:notebookFileNamemapping, the request body carrying the name onsendNotebookToJupyter, and thenotebookNamequery param being present when a name is given and absent when it is not.jupyter-panel.service.spec.ts: the upload uses the wid-derived filename,getJupyterIframeURLForWorkflowforwards that same filename to the HTTP client, and the disabled-flag path returns null without any HTTP call.jupyter-notebook-panel.component.spec.ts: the panel fetches its URL throughgetJupyterIframeURLForWorkflow.Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 4.8)