Fix Learning notebook autosave - #3683
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
saveCourseWorkbook can report success even when the notebook remains dirty after saving, which can lead to stale-tab closure still prompting to save (or risking loss of edits).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR improves the VS Code Learning experience for *.workbook.ipynb notebooks by ensuring learner workbooks are auto-saved when Jupyter updates notebook metadata and by attempting to save workbooks before closing stale learning tabs, reducing unwanted “unsaved changes” prompts.
Changes:
- Added
LearningService.isCourseWorkbookandLearningService.saveCourseWorkbook(with per-notebook save queueing) to centralize workbook detection and serialized auto-save. - Updated learning notebook sync + notebook change handling to call
saveCourseWorkbookafter sync and on metadata/execution changes. - Updated stale-tab closing to save workbooks first and skip closing ones that could not be saved; adjusted notebook opening flow to explicitly show the notebook after the Jupyter API opens it.
File summaries
| File | Description |
|---|---|
| source/vscode/src/learning/service.ts | Adds workbook detection and queued saving; ensures stale learning tabs are saved (and skips closing when save fails). |
| source/vscode/src/learning/notebookSync.ts | Saves a course workbook after syncing when restoring sessions and uses service.isCourseWorkbook for context. |
| source/vscode/src/learning/index.ts | Auto-saves workbooks on notebook metadata changes (and execution changes) via saveCourseWorkbook. |
| source/vscode/src/learning/commands.ts | After opening via Jupyter’s API, explicitly shows the notebook document to ensure it is visible/active. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| } | ||
| } | ||
|
|
||
| if (request !== this._closeStaleTabsRequest) { |
There was a problem hiding this comment.
This is in case the user switched notebooks while we were busy saving?
There was a problem hiding this comment.
Yes, that was the reason. It prevents an older tab closing operation from affecting the notebook the learner just opened. With your list before saving suggestion below, I think we can remove this counter.
There was a problem hiding this comment.
We might still want it (with an explanatory comment) because we don't want to close the notebook that's now open. (e.g. open A, open B, open A - don't want to close A as part of processing open B)
There was a problem hiding this comment.
Ah, I see. The list would handle A -> B, but not A -> B-> A while the first save is still running. I'll keep the request check and add a comment explaining that.
| // is a working copy we created on the user's behalf, we're free to | ||
| // auto-save. | ||
| void e.notebook.save(); | ||
| void learningService.saveCourseWorkbook(e.notebook); |
There was a problem hiding this comment.
Do we even need this if we save on close?
There was a problem hiding this comment.
To my understanding, I think we still need it. Saving before closing only handles moving between units. This listener saves the kernel metadata (that you had found that Jupyter adds) after opening, so the notebook doesn't show a save prompt when the learner hasn't edited anything. It saves the change it would not revert it.
There was a problem hiding this comment.
But why would it be showing a save prompt if you weren't closing the notebook?
There was a problem hiding this comment.
So you are right that the prompt only appears when the notebook is closed since I wanted to still test it, I asked Copilot to test the listener, and it created a temporary VS Code test that opened a copied course notebook and saw Jupyter change its metadata after opening. The listener saves that change before the notebook is later closed.
There was a problem hiding this comment.
Sorry, I'm not sure which side your comment supports. I agree that the notebook will be marked dirty when the language metadata changes, but I'm not sure I understand why that matters if we save before closing.
There was a problem hiding this comment.
Okay, I just misunderstood when the save was needed. I thought that when jupyter's metadata changed, we needed an immediate save because if we don't do that, it would prompt the "do you want to save" dialog box.
But, I just tested navigation between notebooks with and without that metadata save and as you mentioned saving before close was enough in both cases, no prompt. So, I was wrong for adding that metadata save.
Thanks for catching this, I will remove the metadata condition.
There was a problem hiding this comment.
🟡 Changes recommended
Metadata-only changes can still occur after synchronization without triggering a save.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
| // Save after cell execution and update exercise completion. | ||
| if (!learningService.isCourseWorkbook(e.notebook.uri)) { |
Learning notebooks are now saved after metadata changes and before moving to another unit, preventing unwanted save prompts and preserving learner edits.