VS Code extension: Auto-restore on workspace open and config change#15546
VS Code extension: Auto-restore on workspace open and config change#15546adamint wants to merge 5 commits intomicrosoft:mainfrom
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 15546Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 15546" |
There was a problem hiding this comment.
Pull request overview
Adds an auto-restore mechanism to the VS Code extension so aspire restore is run automatically to keep integration packages in sync with aspire.config.json, reducing editor squiggles after branch switches/config edits.
Changes:
- Introduces
AspirePackageRestoreProviderto runaspire restoreon workspace open and onaspire.config.jsonchanges (with a concurrency cap and status bar progress). - Adds the
aspire.enableAutoRestoresetting (defaulttrue) plus localized strings for restore progress and results. - Updates TypeScript playground
aspire.config.jsonfiles and refreshes generated AppHost.modulesoutputs.
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| playground/TypeScriptApps/RpsArena/aspire.config.json | Moves package versions to 13.2.0 and removes daily SDK/channel fields. |
| playground/TypeScriptApps/AzureFunctionsSample/aspire.config.json | Adds appHost metadata and moves package versions to 13.2.0. |
| playground/TypeScriptApps/AzureFunctionsSample/AppHost/.modules/transport.ts | Updates generated transport layer (cancellation, marshalling, connection/auth flow). |
| playground/TypeScriptApps/AzureFunctionsSample/AppHost/.modules/base.ts | Updates generated base SDK types (ReferenceExpression enhancements, transport value serialization). |
| playground/TypeScriptApps/AzureFunctionsSample/AppHost/.modules/.codegen-hash | Updates codegen hash to reflect regenerated modules. |
| extension/src/utils/settings.ts | Adds accessor for enableAutoRestore. |
| extension/src/utils/AspirePackageRestoreProvider.ts | New provider that runs/monitors aspire restore and shows status bar progress. |
| extension/src/loc/strings.ts | Adds localized runtime strings for restore progress/results. |
| extension/src/extension.ts | Wires up the new auto-restore provider during activation. |
| extension/package.nls.json | Adds localized description for the new setting and restore messages. |
| extension/package.json | Defines the new aspire.enableAutoRestore configuration setting. |
| extension/loc/xlf/aspire-vscode.xlf | Adds localization units for new strings/setting description. |
playground/TypeScriptApps/AzureFunctionsSample/AppHost/.modules/transport.ts
Outdated
Show resolved
Hide resolved
…g restores, separate errors, fix timeout leak, handle unhandled promise
…s-15546 # Conflicts: # playground/TypeScriptApps/AzureFunctionsSample/AppHost/.modules/.codegen-hash # playground/TypeScriptApps/AzureFunctionsSample/AppHost/.modules/aspire.ts # playground/TypeScriptApps/AzureFunctionsSample/AppHost/.modules/base.ts # playground/TypeScriptApps/AzureFunctionsSample/AppHost/.modules/transport.ts
| "Aspire.Hosting.Python": "13.3.0-preview.1.26167.8", | ||
| "Aspire.Hosting.PostgreSQL": "13.3.0-preview.1.26167.8", | ||
| "Aspire.Hosting.JavaScript": "13.3.0-preview.1.26167.8" | ||
| "Aspire.Hosting.Python": "13.2.0", | ||
| "Aspire.Hosting.PostgreSQL": "13.2.0", | ||
| "Aspire.Hosting.JavaScript": "13.2.0" |
There was a problem hiding this comment.
Shouldn't be using the latest main builds? What effect would this change have?
There was a problem hiding this comment.
Practically, no difference. What would be really nice is if there were a way to avoid putting a version altogether and just taking the latest (or just allowing a “latest” tag).
| for (const proc of this._childProcesses) { | ||
| proc.kill(); | ||
| } | ||
| this._childProcesses.clear(); | ||
| for (const timeout of this._timeouts) { | ||
| clearTimeout(timeout); | ||
| } | ||
| this._timeouts.clear(); | ||
| for (const d of this._disposables) { | ||
| d.dispose(); | ||
| } | ||
| this._disposables.length = 0; |
There was a problem hiding this comment.
I'm a TS noob. Do we need to handle exceptions here like if killing a process fails?
| export const cliNotAvailable = vscode.l10n.t('Aspire CLI is not available on PATH. Please install it and restart VS Code.'); | ||
| export const cliFoundAtDefaultPath = (path: string) => vscode.l10n.t('Aspire CLI found at {0}. The extension will use this path.', path); | ||
| export const selectDirectoryTitle = vscode.l10n.t('Select directory'); | ||
| export const runningAspireRestore = (configPath: string) => vscode.l10n.t('Running aspire restore on {0}...', configPath); |
There was a problem hiding this comment.
| export const runningAspireRestore = (configPath: string) => vscode.l10n.t('Running aspire restore on {0}...', configPath); | |
| export const runningAspireRestore = (configPath: string) => vscode.l10n.t('Running aspire restore on {0} ...', configPath); |
| if (!getEnableAutoRestore()) { | ||
| return; | ||
| } | ||
| if (this._active.size === 0) { |
There was a problem hiding this comment.
Do we need to worry about _onChanged getting called multiple times in parallel?
There was a problem hiding this comment.
It would be safer to cancel an existing restore task for a given uri, if it exists. I’ll make that change
| const hideTimeout = setTimeout(() => { | ||
| this._timeouts.delete(hideTimeout); | ||
| if (this._active.size === 0) { this._statusBarItem.hide(); } | ||
| }, 5000); |
There was a problem hiding this comment.
5000 - should this be a const from somewhere?
There was a problem hiding this comment.
Yeah, it would be a good idea to-I’ll add
Co-authored-by: Ankit Jain <radical@gmail.com>
JamesNK
left a comment
There was a problem hiding this comment.
Code review — focused on problems only. 5 inline comments on the new AspirePackageRestoreProvider.
| }); | ||
| }).finally(() => { | ||
| this._active.delete(configDir); | ||
| this._completed++; |
There was a problem hiding this comment.
When the timeout fires, it calls proc.kill() then reject(new Error('restore timed out')). Killing the process triggers the close event, which fires exitCallback with a non-zero/null code, calling reject() a second time. While a second reject on an already-settled promise is a no-op in JS, the exitCallback still logs a misleading "aspire restore failed … exit code null" warning after the real "timed out" error.
Consider adding a settled flag to skip exitCallback/errorCallback once the timeout has already rejected.
| // If a change arrived while we were restoring, re-read and restore again | ||
| if (this._pendingRestore.delete(configDir)) { | ||
| await this._restoreIfChanged(uri, false); | ||
| } |
There was a problem hiding this comment.
The comment on line 124 says "Don't update baseline until restore succeeds" but this._lastContent.set(uri.fsPath, content) here runs unconditionally before _runRestore is awaited. If the restore fails, the baseline is already updated, so a subsequent file-watcher event with the same content will be skipped (the prev === content check on line 112 will match).
The content should only be saved in _lastContent after a successful restore, or reverted on failure.
| this._restoreAll(); | ||
| } | ||
| }) | ||
| ); |
There was a problem hiding this comment.
_watchConfigFiles() is only called once at the end of activate(). When the user toggles the setting on, this handler only calls _restoreAll() — watchers are never created. Changes to aspire.config.json after toggling the setting on won't be detected until the extension is reloaded.
This should also call _watchConfigFiles() (guarding against duplicate watchers), or the watchers should be set up unconditionally and just check the setting in _onChanged (which it already does).
| this._restoreAll(); | ||
| } | ||
| }) | ||
| ); |
There was a problem hiding this comment.
this._restoreAll() returns a promise but is called inside a synchronous onDidChangeConfiguration callback without void or .catch(), producing an unhandled floating promise. If it rejects, the error is silently lost.
The call site in extension.ts correctly uses void ... .catch() — apply the same pattern here:
void this._restoreAll().catch(err => {
extensionLogOutputChannel.warn(`Auto-restore failed: ${String(err)}`);
});| await this._restoreIfChanged(uri, false); | ||
| } | ||
|
|
||
| private async _restoreIfChanged(uri: vscode.Uri, isInitial: boolean): Promise<void> { |
There was a problem hiding this comment.
When _active.size === 0, the counters are reset to total=1, completed=0. But if the watcher fires for two different files in quick succession while no restore is active, the first call sets total=1, then the second call also resets total=1 before the first restore finishes. The progress display will show (0/1 projects) even though 2 restores are now pending.
Minor UX issue — consider incrementing _total instead of resetting, or tracking pending items more precisely.
Description
Adds automatic
aspire restoreexecution in the VS Code extension to keep integration packages in sync withaspire.config.json.Problem: When switching git branches that have different Aspire integrations configured, the editor shows squiggly errors until you manually run
aspire restore. This is a common friction point during development.Solution: The new
AspirePackageRestoreProviderautomatically runsaspire restore:aspire.config.jsonfiles found in the workspaceFileSystemWatchermonitorsaspire.config.jsonfiles; when content actually changes (e.g., branch switch, manual edit), it runs restoreI tested this using:
Checklist