Fix "Downloading .NET..." status bar indicator not clearing after download completes#2607
Fix "Downloading .NET..." status bar indicator not clearing after download completes#2607
Conversation
Co-authored-by: nagilson <23152278+nagilson@users.noreply.github.com>
vscode-dotnet-runtime-library/src/EventStream/StatusBarObserver.ts
Outdated
Show resolved
Hide resolved
… requests Address review feedback: two extensions (e.g. C# and C# Dev Kit) can request the same installId simultaneously. In the global SDK path, DotnetAcquisitionStarted fires before the inner installer lock, so duplicate start events are possible with only one completion event following. Changing from string[] to Set<string> makes tracking idempotent - duplicate starts for the same installId are naturally deduplicated, so a single completion correctly clears the tracking and hides the status bar. Added tests for: - Duplicate installId starts with single completion - Completion for unknown installId (graceful no-op) - Mixed completion/error with duplicate starts Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes the “Downloading .NET...” status bar indicator lifecycle in the runtime acquisition library by tracking concurrent acquisitions and clearing the indicator only when all downloads have finished (including failure/abort paths).
Changes:
- Add
inProgressDownloadstracking inStatusBarObserverto avoid clearing the status bar while other downloads are still running. - Handle
DotnetAcquisitionFinalErrorto ensure the indicator clears on download failure. - Add unit tests covering single and concurrent download lifecycles, aborts, and final-error scenarios.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| vscode-dotnet-runtime-library/src/EventStream/StatusBarObserver.ts | Track active installIds and only hide/reset the status bar when the last in-progress acquisition completes/aborts/fails. |
| vscode-dotnet-runtime-library/src/test/unit/StatusBarObserver.test.ts | New unit tests to validate correct status bar visibility with concurrency and error/abort flows. |
| test('Status bar hides correctly when same installId is started twice (duplicate request)', () => | ||
| { | ||
| // Simulates the global SDK path where DotnetAcquisitionStarted fires before the inner lock, | ||
| // so two extensions requesting the same version can each trigger a start event. | ||
| const install = makeInstall('8.0~x64'); | ||
| observer.post(new DotnetAcquisitionStarted(install, 'ext-csharp')); | ||
| observer.post(new DotnetAcquisitionStarted(install, 'ext-csharpdk')); | ||
|
|
There was a problem hiding this comment.
The tests that aim to simulate “different extensions requesting the same version” are passing the extension id as the 2nd constructor argument to DotnetAcquisitionStarted, but that parameter is startingVersion (the requesting extension id is the 3rd parameter). This makes the scenario/comment misleading and could hide future regressions if the observer ever starts relying on requestingExtensionId.
Consider constructing DotnetAcquisitionStarted with both a realistic startingVersion (e.g., the version being acquired) and the requestingExtensionId as the 3rd argument, and/or updating the comment to match what the test actually does.
| if (installId) | ||
| { |
There was a problem hiding this comment.
removeFromInProgress uses a different brace style than the rest of this file (and the surrounding code in this method), which is likely to trip formatting/lint rules or at least makes the style inconsistent. Consider formatting the if block consistently with the file’s existing style.
| if (installId) | |
| { | |
| if (installId) { |
StatusBarObserverhad no awareness of concurrent downloads — it hid the status bar on the firstDotnetAcquisitionCompletedevent received, even if other downloads were still in progress. It also never handledDotnetAcquisitionFinalError, leaving the indicator permanently visible on download failure.Changes
StatusBarObserver: AddinProgressDownloads: string[]tracking (consistent withOutputChannelObserver) to gateresetAndHideStatusBar()on all downloads finishing:DotnetAcquisitionStart→ pushinstallId, show indicatorDotnetAcquisitionCompleted/DotnetInstallExpectedAbort→ removeinstallId, hide if emptyDotnetAcquisitionFinalError(new) → removeinstallId, hide if empty (indicator was stuck forever on failure)DotnetAcquisitionError→ unchanged (always show error state)StatusBarObserver.test.ts: New unit tests covering single download lifecycle, concurrent download tracking (stays visible while any download is in progress), abort, and final-error scenarios.Warning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
microsoft.com/home/REDACTED/work/_temp/ghcca-node/node/bin/node node /home/REDACTED/work/vscode-dotnet-runtime/vscode-dotnet-runtime/vscode-dotnet-runtime-library/node_modules/.bin/mocha -u tdd -- dist/test/unit/DebianDistroTests.test.js dist/test/unit/DotnetConditionValidator.test.js dist/test/unit/DotnetCoreAcquisitionWorker.test.js dist/test/unit/DotnetPathFinder.test.js dist/test/unit/DotnetResolver.test.js dist/test/unit/ErrorHandler.test.js dist/test/unit/ExecutableArchitectureDetector.test.js dist/test/unit/ExistingPathResolver.test.js dist/test/unit/GlobalInstallerResolver.test.js dist/test/unit/InstallScriptAcquisitionWorker.test.js dist/test/unit/InstallTracker.test.js dist/test/unit/InstallationValidator.test.js dist/test/unit/IssueReporter.test.js dist/test/unit/JsonInstaller.test.js dist/test/unit/LinuxDistroTests.test.js(dns block)If you need me to access, download, or install something from one of these locations, you can either:
Original prompt
🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.