Split Learning reset into exercise and unit - #3635
Dhairya Patel (HABER7789) wants to merge 10 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Splits QDK Learning resets into activity-level and unit-level operations.
Changes:
- Restores individual notebook cells without affecting other work.
- Adds whole-unit reset support for Q# and notebook courses.
- Exposes the new tool through Copilot, commands, telemetry, and agent guidance.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
source/vscode/src/telemetry.ts |
Adds unit-reset telemetry. |
source/vscode/src/learning/service.ts |
Implements activity and unit reset behavior. |
source/vscode/src/learning/python/materialization.ts |
Restores individual notebook cells. |
source/vscode/src/learning/notebookExercises.ts |
Adds cell source lookup and replacement. |
source/vscode/src/learning/commands.ts |
Updates reset commands and notebook reopening. |
source/vscode/src/gh-copilot/tools.ts |
Registers the unit-reset tool. |
source/vscode/src/gh-copilot/learningTools.ts |
Adds tool orchestration and selected-cell synchronization. |
source/vscode/package.json |
Contributes the command and tool metadata. |
source/vscode/ai/qdk-learning.agent.md |
Documents activity versus unit resets. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
160de31 to
652220d
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Notebook close, edit, save, and rematerialization failures can currently be reported as successful resets.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
source/vscode/src/learning/python/materialization.ts:136
NotebookDocument.save()returnsfalsewhen the document was not saved, but that result is ignored and the reset is reported as successful. In that case progress is cleared even though the restored cell was never persisted. Treat a false save result as a restoration failure rather than returning true.
await notebook.save();
return true;
source/vscode/src/learning/service.ts:1032
rematerializeUnitWorkbookdelegates tomaterializeNotebook, which catches every filesystem/parsing failure and returnsvoid(materialization.ts:147-170). Consequently this call always proceeds tofinishUnitReset, clearing completion and telling the command/tool that the unit was reset even when the workbook was unchanged. Propagate a failure result here and only clear progress after the workbook was successfully restored.
await rematerializeUnitWorkbook(unit);
- Files reviewed: 9/9 changed files
- Comments generated: 2
- Review effort level: Balanced
- resetExercise now restores only current activity - copilot can now reset a whole unit and can target a unit by name - resetting something with no code, like a markdowncell, will now report instead of silently resetting another cell
652220d to
528285f
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Failed save paths can still discard learner work or incorrectly report a successful reset.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 2
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Q# unit resets can partially overwrite files when a later save or write fails.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Balanced
Q# exercises are already separate files with their own reset, so a whole-unit reset added complexity for little gain. Unit reset is now notebook-only and recopies the notebook from the original; Q# courses reset exercises one at a time.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔵 Needs a closer look
Unit-reset telemetry currently records an unrelated activity type, particularly when resetting a non-current unit.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
source/vscode/src/learning/service.ts:1124
reset-unitis a unit-level action, but this call letssendActivityActionTelemetryfillactivityTypefrom the current position. In the chat path, a named non-current unit is reset beforegoToruns, so the event records an activity type from an unrelated unit; even for the current unit, one type cannot describe all activities being reset. Model this as unit-level telemetry (for example, allow an explicit"unit"target type or use a separate event) instead of defaulting to the current activity.
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
The current workbook guard can still reset the wrong activity during asynchronous notebook synchronization.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
source/vscode/src/learning/commands.ts:79
- This command also backs the notebook toolbar (
package.json:464-469), so hard-coding"tree"misattributes every toolbar unit reset in telemetry. Use the invocation context here (a tree invocation supplieslocation; the toolbar does not) to report"tree"or"notebook"accurately.
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Confirmation and toolbar-target races can reset notebook content without confirming or targeting the visible unit.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
source/vscode/src/learning/commands.ts:79
- This command is shared by the progress tree and notebook toolbar, but every reset is recorded as coming from
tree. Toolbar resets will therefore corrupt the new telemetry breakdown; use the invocation context to reportnotebookwhen no tree node/location was supplied.
- Files reviewed: 9/9 changed files
- Comments generated: 2
- Review effort level: Balanced
- The chat reset now always asks for confirmation instead of guessing from stored state, which could skip the prompt and reset the wrong cell. - The notebook toolbar reset now targets the notebook you're looking at, not a stale stored unit. Removes the dead confirmReset, selectedNotebookCellId, and getCurrentActivityType.
There was a problem hiding this comment.
🟡 Changes recommended
Default chat unit resets can target a stale stored unit instead of the active workbook.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
Activity reset can report success while leaving a converted notebook cell as Markdown instead of restoring runnable code.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
source/vscode/src/learning/python/materialization.ts:62
- Only the authored source is carried forward here, while both replacement paths preserve the learner's current cell kind (
existing.kindin the open path andcell_typein the JSON path). If an exercise/code cell was converted to Markdown, reset therefore inserts the starter code into a Markdown cell, clears completion, and reports success, but the activity is no longer runnable. Read the authored cell kind along with its source and restore that kind in both paths (also clearing code execution state).
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
Closing a workbook can change the active course or unit, causing reset callers to reopen or navigate to the wrong notebook.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
source/vscode/src/gh-copilot/learningTools.ts:398
- If closing this workbook activates a notebook from another course, notebook synchronization can change the active course before this line runs.
goTodeliberately resolves only within the active course, so it may throw after the reset already succeeded or navigate to a same-named unit in the wrong course. Capture the target course before resetting and restore it before navigating.
source/vscode/src/learning/commands.ts:98 - Closing the target workbook can activate another open course workbook, whose
onDidChangeActiveNotebookEditorhandler asynchronously changes the service position. This call then resolves the notebook URI from that new position and can reopen the wrong unit. Preserve the target course/unit and navigate back to it afterresetUnitreturns, before callingopenCourseNotebook.
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Balanced
The learning experience keeps a single course notebook open at a time focusing or opening another closes the previous via closeStaleEditorTabs. So when reset closes the current notebook there's no other course workbook to activate, and the position can't shift under the reopen. The precondition here isn't reachable through normal use. Leaving as-is |
| * Reset an entire unit, clearing completion for all of its activities. | ||
| * Defaults to the current unit. | ||
| */ | ||
| async resetUnit(input?: { |
There was a problem hiding this comment.
Why parameterize it? Reset exercise targets the current exercise or nothing.
| // Python-notebook courses: close the notebook, re-copy the entire unit | ||
| // from source, and clear completion. | ||
| const course = this.activeCourse; | ||
| await this.resetExerciseAt( |
There was a problem hiding this comment.
It seems like the VS Code command will call this one, regardless of whether or not the current unit is a notebook. What will it do in that case?
| "Click into the exercise cell you want to reset and try again, or reset the whole unit.", | ||
| ); | ||
| } | ||
| const state = this.serializeState(true); |
There was a problem hiding this comment.
Existing calls to serializeState explain why they're passing true or false. (This isn't the only instance.)
| // if a workbook is focused but its selected cell can't be identified, | ||
| // fail loudly instead of falling back to the stored position. | ||
| if (this.notebookSelectionUnidentified()) { | ||
| throw new CopilotToolError( |
There was a problem hiding this comment.
I think this message is an explanation to copilot, rather than to the user. I'm also not sure what "no stable id yet" means. I might go with something closer to "the current activity isn't an exercise".
There was a problem hiding this comment.
Actually, I'd probably eliminate this whole check and have resetExerciseAt throw if it's unhappy with the state. this.invoke exists to translate service errors into copilot errors.
| * position — unsafe for reset. Judged from the active editor's URI so a | ||
| * different unit's workbook is still covered. | ||
| */ | ||
| private notebookSelectionUnidentified(): boolean { |
There was a problem hiding this comment.
I'm not sure I understand why we needed a new method. Does this do something meaningfully different than serializeState/notebookLearningState?
| return replaceOpenCell(open, cellId, authored); | ||
| } | ||
|
|
||
| const destText = new TextDecoder().decode( |
There was a problem hiding this comment.
Why would we be operating on a cell in a closed notebook?
There was a problem hiding this comment.
This file is really just for file operations - it probably shouldn't be talking to the editor at all. It might be cleaner to just expose a helper that returns a cell and its metadata.
| // courses — when the notebook declares no kernel language. | ||
| const isCode = authored.kind === "code"; | ||
| const data = new vscode.NotebookCellData( | ||
| isCode ? vscode.NotebookCellKind.Code : vscode.NotebookCellKind.Markup, |
There was a problem hiding this comment.
Is this just authored.kind?
| const data = new vscode.NotebookCellData( | ||
| isCode ? vscode.NotebookCellKind.Code : vscode.NotebookCellKind.Markup, | ||
| authored.source, | ||
| isCode ? (authored.language ?? "python") : "markdown", |
There was a problem hiding this comment.
existing.language?
| ), | ||
| ]); | ||
| if (!(await vscode.workspace.applyEdit(edit))) { | ||
| return false; |
There was a problem hiding this comment.
Should we log something?
| // Persist the reset best-effort. The edit already updated the editor, so a | ||
| // save that doesn't land just leaves the cell unsaved, not un-reset; log it | ||
| // for diagnostics but still report the cell as reset. | ||
| try { |
resetExercisereset one.qsfile for Q# courses but wiped the entire notebook for notebook courses, while telling Copilot it "only resets an exercise." It now restores just the current activity one file, or one cell.Adds
resetUnitand aqdk-learning-reset-unittool so chat can reset a whole unit, including by name.Reset was picking the wrong notebook cell, because the stored position only moves when you run a cell, not when you click one. Now that #3627 has merged, reset resolves the selected cell by location the same way the hint and solution tools do. I also hardened the reset paths so a cancelled notebook close, rejected edit, or failed save aborts and keeps the user's progress instead of reporting success.