Make Aspire extension activity notifications dismissible - #19124
Make Aspire extension activity notifications dismissible#19124Adam Ratzman (adamint) wants to merge 1 commit into
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 19124Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 19124" |
There was a problem hiding this comment.
Pull request overview
Moves long-running Aspire extension activity from blocking notifications to a shared, dismissible status bar item.
Changes:
- Adds aggregated CLI status and stop controls.
- Hardens RPC and extension shutdown.
- Adds localized labels and extensive unit/E2E coverage.
Show a summary per file
| File | Description |
|---|---|
extension/src/views/AppHostDataRepository.ts |
Moves discovery progress to status bar. |
extension/src/utils/workspace.ts |
Generates concise workspace identities. |
extension/src/test/workspace.test.ts |
Tests Windows root identities. |
extension/src/test/rpc/interactionServiceTests.test.ts |
Tests status and RPC disposal behavior. |
extension/src/test/rpc/aspireRpcServer.test.ts |
Tests RPC ownership and shutdown races. |
extension/src/test/progressNotifier.test.ts |
Tests status notifier lifecycle. |
extension/src/test/cliStatusBar.test.ts |
Tests aggregation and stop interactions. |
extension/src/test/AspireExtensionContext.test.ts |
Tests asynchronous deactivation. |
extension/src/test/aspireDebugSession.test.ts |
Tests stop-request reuse. |
extension/src/test/appHostDataRepository.test.ts |
Updates discovery progress expectations. |
extension/src/test-e2e/helpers/vscode.ts |
Adds status-bar E2E helpers. |
extension/src/test-e2e/debugDashboard.e2e.test.ts |
Verifies CLI status and stopping end-to-end. |
extension/src/test-e2e/appHostTree.e2e.test.ts |
Verifies discovery avoids notifications. |
extension/src/server/rpcClient.ts |
Adds deterministic client disposal. |
extension/src/server/progressNotifier.ts |
Routes CLI progress to shared status. |
extension/src/server/interactionService.ts |
Supplies operation identities and disposal. |
extension/src/server/cliStatusBar.ts |
Implements shared CLI activity UI. |
extension/src/server/AspireRpcServer.ts |
Owns pending and active connections. |
extension/src/loc/strings.ts |
Adds localized status strings. |
extension/src/extension.ts |
Registers cleanup and async deactivation. |
extension/src/debugger/AspireDebugSession.ts |
Reuses shutdown stop requests. |
extension/src/AspireExtensionContext.ts |
Coordinates bounded asynchronous shutdown. |
extension/package.nls.json |
Adds localization source entries. |
extension/loc/xlf/aspire-vscode.xlf |
Updates generated localization data. |
Review details
- Files reviewed: 24/24 changed files
- Comments generated: 2
- Review effort level: Balanced
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
There was a problem hiding this comment.
Review details
Suppressed comments (4)
extension/src/AspireExtensionContext.ts:152
Promise.allSettleddoes not expose any result until every stop request settles. With multiple AppHosts, if one request rejects and another hangs, this race takes the timeout branch and the first failure is never logged (even if it rejects after the timeout); only the generic timeout is visible. Attach the expected/info vs unexpected/warn logging to each request as it settles, while retaining the aggregate timeout for teardown.
const allStops = Promise.allSettled(stopRequests);
let timeout: ReturnType<typeof setTimeout> | undefined;
const outcome = await Promise.race([
allStops.then(results => ({ timedOut: false as const, results })),
extension/src/utils/workspace.ts:87
- Forward-slash UNC roots are absolute under both parsers, so this condition selects the host
pathimplementation on POSIX. Consequently//server-a/share/and//server-b/share/both collapse toshare, breaking the promised distinct root identities. Select Win32 semantics when its parsed root is more specific than the POSIX root; forward-slash UNC inputs are already treated as valid elsewhere (for examplesrc/test/cliPath.test.ts:156-165).
const pathOperations = path.win32.isAbsolute(filePath) && !path.posix.isAbsolute(filePath)
? path.win32
: path;
extension/package.nls.json:191
- This new user-facing localization key is absent from
loc/xlf/aspire-vscode.xlf, while the other keys in this block are present.gulpfile.js:54-72generates that XLF directly frompackage.nls.json; regenerate it so this stop-failure message is included for translation.
"aspire-vscode.strings.failedToStopCliOperation": "Failed to stop the Aspire CLI operation: {0}",
extension/src/server/cliStatusBar.ts:275
slicecounts UTF-16 code units, butProgressNotifierhas already expanded CLI emoji shortcodes. If a non-BMP emoji crosses this boundary, truncation emits a lone surrogate and the status bar shows a replacement character. Truncate by code points instead.
const truncated = singleLine.length > maxRenderedStatusLength
? `${singleLine.slice(0, maxRenderedStatusLength - 1).trimEnd()}…`
: singleLine;
- Files reviewed: 24/24 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
Review details
Suppressed comments (4)
extension/src/server/cliStatusBar.ts:253
- A normal
stopClifollows this rejection path:ExtensionRpcTarget.StopCliAsync()callsEnvironment.Exitbefore sending a response, so disposal rejects the pending request. The connection-close handler has already disposed this entry, but the error is logged before that check, causing every successful status-bar stop to emit a misleading “Failed to stop CLI” error. Check disposal before logging so only failures for a still-live operation are reported.
this._owner.stop().catch(err => {
extensionLogOutputChannel.error(`Failed to stop CLI: ${err}`);
extension/package.nls.json:191
failedToStopCliOperationis the only new localization key missing fromextension/loc/xlf/aspire-vscode.xlf. That XLF is generated from this file byextension/gulpfile.js:54-73; regenerate it so the new user-facing error is included in translation input.
"aspire-vscode.strings.failedToStopCliOperation": "Failed to stop the Aspire CLI operation: {0}",
extension/src/utils/workspace.ts:60
- This privacy-safe identity helper is not used by the primary tree/editor launch path.
AppHostLaunchService.launch()still builds the initial debug configuration name withvscode.workspace.asRelativePath(appHostPath)(extension/src/services/AppHostLaunchService.ts:147), whileAspireEditorCommandProvidersupports launching an AppHost from an active editor outside the workspace. In that scenario VS Code returns the absolute path unchanged, so the debug label still exposes it despite the PR’s privacy claim. Use this helper for that launch configuration and its failure label too.
export function getRelativePathToWorkspace(filePath: string): string | undefined {
const uri = vscode.Uri.file(filePath);
const workspaceFolder = vscode.workspace.getWorkspaceFolder(uri);
if (!workspaceFolder) {
return getPathIdentity(filePath);
extension/src/server/cliStatusBar.ts:275
String.lengthandslicecount UTF-16 code units, so this truncation can split an emoji surrogate pair and render a replacement character. CLI status already supports emoji throughformatText; truncate by code points (or grapheme clusters) before appending the ellipsis.
const singleLine = collapseStatusText(statusText);
const truncated = singleLine.length > maxRenderedStatusLength
? `${singleLine.slice(0, maxRenderedStatusLength - 1).trimEnd()}…`
: singleLine;
- Files reviewed: 24/24 changed files
- Comments generated: 0 new
- Review effort level: Balanced
b2b7593 to
55a8351
Compare
There was a problem hiding this comment.
Review details
Suppressed comments (2)
extension/src/test-e2e/debugDashboard.e2e.test.ts:213
- This samples notifications only after
debugAppHosthas succeeded. The CLI'sShowStatusAsyncqueuesshowStatus(null)infinallybefore the command completes, so the old notification implementation can already be gone by this point; despite the comment above, there is no poll. This test can therefore pass before the fix. Observe one of the delayed status texts while the command is still in flight, then inspect notifications before awaiting the success outcome.
await waitForCommandOutcome('aspire-vscode.debugAppHost', 'success', 120000, before);
// A progress notification cannot be dismissed while the operation runs, so CLI status
// stayed on top of the editor for the whole run
// (https://github.com/microsoft/aspire/issues/19036).
const notificationMessages = await getNotificationMessages();
extension/src/server/progressNotifier.ts:72
ProgressLocation.Windowprovides no cancellation callback here, and this patch removes theICliRpcClient/token.onCancellationRequestedpath that calledstopCli(). The status UI therefore no longer has the “explicit stop action” promised by the PR, so a build or connection that hangs before an AppHost is available cannot be stopped from the activity indicator. Use a command-backed status-bar item (or another visible action) wired to the owning RPC client.
vscode.window.withProgress({
location: vscode.ProgressLocation.Window
}, async progress => {
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Balanced
55a8351 to
6b9f66b
Compare
There was a problem hiding this comment.
Review details
Suppressed comments (2)
extension/src/server/progressNotifier.ts:72
- This removes the only usable stop path for an active CLI operation:
ProgressNotifierno longer receives the RPC client,cancellableis omitted, and the cancellation handler that calledstopCli()is deleted. A repository-wide search now leavesstopCli()only as an uncalled method, which contradicts the PR's promise that the explicit stop action remains available. Please keep a status-bar stop action wired to the owning RPC client while making dismissal independent from stopping.
vscode.window.withProgress({
location: vscode.ProgressLocation.Window
}, async progress => {
extension/src/test-e2e/debugDashboard.e2e.test.ts:213
- This assertion runs only after
debugAppHostreports success, by which timeshowStatus(null)or RPC closure has already removed either kind of progress. The previous notification implementation therefore also passes this regression test. Observe a delayed CLI status in the workbench and inspect notifications while that status is still active, then wait for command completion afterward.
await waitForCommandOutcome('aspire-vscode.debugAppHost', 'success', 120000, before);
// A progress notification cannot be dismissed while the operation runs, so CLI status
// stayed on top of the editor for the whole run
// (https://github.com/microsoft/aspire/issues/19036).
const notificationMessages = await getNotificationMessages();
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Balanced
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
Long-running Aspire activity was reported with `ProgressLocation.Notification`. A progress notification cannot be dismissed while its operation is running, so "Building AppHost...", "Connecting to AppHost...", "Starting dashboard..." and "Discovering AppHosts..." sat on top of the editor for the whole run with no way for the user to get rid of them. Report both through `ProgressLocation.Window` instead, which renders in the status bar: it stays visible for the whole operation, is never modal, and the user can ignore or hide it. The notification's cancel button goes with it; stopping a run is already covered by the debug toolbar stop, the Aspire view's `aspire-vscode.stopAppHost` command, and `AspireDebugSession.stopDebugging()`. CLI-supplied status text is now collapsed to a single line and has its `$(name)` codicon syntax escaped, because the status bar renders icons and only shows one line. Both helpers live in the vscode-free `utils/strings.ts` so other callers that render untrusted CLI text can share them. Fixes microsoft#19036 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
6b9f66b to
fcc19ca
Compare
There was a problem hiding this comment.
Review details
Suppressed comments (2)
extension/src/test-e2e/debugDashboard.e2e.test.ts:213
- This one-time read can run before any CLI status has rendered. The debug command reports success once
vscode.debug.startDebuggingreturns (AppHostLaunchService.ts:190-205), while status RPCs arrive later; additionally, the E2E delay blocks beforeProgressNotifier.show(interactionService.ts:177-179), so it does not make a status visible before this assertion. Wait until one ofcliRunStatusTextsis present in the workbench, then inspect notifications while that text is still present.
const notificationMessages = await getNotificationMessages();
extension/src/server/progressNotifier.ts:72
- The PR description says the status-bar activity retains an explicit safe stop action, but this change drops the RPC client,
cancellable: true, and the cancellation handler that calledstopCli(). ThisWindowprogress defines no replacement command, so no stop action is retained here. Since the review thread says that affordance is deferred to #19134, update the PR description and user-facing usage rather than claiming it remains.
vscode.window.withProgress({
location: vscode.ProgressLocation.Window
}, async progress => {
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Description
Long-running build, AppHost connection, and dashboard startup notifications can cover other VS Code UI and cannot be dismissed without stopping the AppHost.
This moves Aspire-owned CLI activity out of persistent notification toasts and into one status bar item with the existing safe stop action. It also hardens extension shutdown so expected RPC closure does not surface as an unhandled failure, while unexpected stop failures remain visible in logs. Shared workspace identities keep status, quick-pick, and debug labels short, nonempty, privacy-safe, and distinct for Windows drive and UNC roots.
Fixes #19036
User-facing usage
Starting an AppHost now reports long-running Aspire activity in the VS Code status bar instead of a notification that blocks other UI. Users can dismiss the activity from view without terminating the AppHost, while the explicit stop action still stops the owned operation.
Screenshots / Recordings
TODO: Capture the status bar activity and dismiss/stop behavior in VS Code Insiders.
Validation
Checklist
<remarks />and<code />elements on your triple slash comments?