Improve UX of chat background tasks, implement for terminal command finished task#310331
Improve UX of chat background tasks, implement for terminal command finished task#310331meganrogge wants to merge 2 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a new “chat background task” concept so long-running terminal commands (run via the chat terminal tool) can surface progress/completion as accessible UI chips instead of only via steering text in the conversation.
Changes:
- Add
IChatBackgroundTaskServiceand a browser implementation to create/track background tasks. - Update
RunInTerminalToolto create a background task for background terminal commands and complete/fail it on command finish. - Render background task chips in the chat input attachments area and wire dismissal/cancel behavior.
Show a summary per file
| File | Description |
|---|---|
| src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/runInTerminalTool.ts | Creates/updates background tasks for background terminal command execution. |
| src/vs/workbench/contrib/chat/common/chatBackgroundTask.ts | Defines background-task types, status enum, and service interface. |
| src/vs/workbench/contrib/chat/browser/chatBackgroundTaskService.ts | Implements in-memory background task tracking, lookup, and eviction. |
| src/vs/workbench/contrib/chat/common/attachments/chatVariableEntries.ts | Adds a new variable entry type for background tasks so results can be attached. |
| src/vs/workbench/contrib/chat/browser/widget/input/chatInputPart.ts | Renders background task chips and includes completed/failed tasks in attached context. |
| src/vs/workbench/contrib/chat/browser/widget/chatWidget.ts | Evicts finished tasks after submit and re-renders attachments after session becomes available. |
| src/vs/workbench/contrib/chat/browser/chat.contribution.ts | Registers IChatBackgroundTaskService singleton. |
| src/vs/workbench/contrib/chat/browser/attachments/chatAttachmentWidgets.ts | Adds BackgroundTaskAttachmentWidget for reactive chip UI/hover/cancel-dismiss. |
Copilot's findings
Comments suppressed due to low confidence (3)
src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/runInTerminalTool.ts:2253
- The failure message passed to
backgroundTask.failincludes the hard-coded English text "Exited with code" which is shown in the background task hover. User-facing status strings should be localized; consider localizing the prefix and keeping the raw terminal output separate (or appending it as-is after a localized header).
// Update the background task with the result instead of sending a steering message
if (exitCode !== undefined && exitCode !== 0) {
backgroundTask.fail(`Exited with code ${exitCode}\n${currentOutput ?? ''}`);
} else {
backgroundTask.complete(currentOutput ?? '');
src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/runInTerminalTool.ts:2254
- In this completion handler,
backgroundTask.complete/failis skipped whenhandleSessionCancelled()returns true (the early-return is just above this hunk). That means cancelling the chat request can leave the background task stuck in Working with no future listeners to update it. Consider decoupling "suppress steering messages" from updating the background task so completion still updates task status even when the request was cancelled.
disposeNotification();
const exitCode = command.exitCode;
const currentOutput = execution.getOutput();
this._logService.debug(`RunInTerminalTool: Command completed in background terminal ${termId}, updating background task`);
// Update the background task with the result instead of sending a steering message
if (exitCode !== undefined && exitCode !== 0) {
backgroundTask.fail(`Exited with code ${exitCode}\n${currentOutput ?? ''}`);
} else {
backgroundTask.complete(currentOutput ?? '');
}
src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/runInTerminalTool.ts:2283
terminalInstance.onDisposedunconditionally callsbackgroundTask.fail(...). If the command already completed successfully (or was cancelled), terminal disposal will overwrite the final status/result. Gate this handler so it only fails the task while it is still in Working state (or makefail()ignore non-Working states).
store.add(terminalInstance.onDisposed(() => {
backgroundTask.fail(localize('terminalDisposed', "Terminal was closed"));
disposeNotification();
- Files reviewed: 8/8 changed files
- Comments generated: 6
| @IWorkspaceContextService private readonly _workspaceContextService: IWorkspaceContextService, | ||
| @IChatWidgetService private readonly _chatWidgetService: IChatWidgetService, | ||
| @IAgentSessionsService private readonly _agentSessionsService: IAgentSessionsService, | ||
| @IChatBackgroundTaskService private readonly _chatBackgroundTaskService: IChatBackgroundTaskService, | ||
| ) { |
There was a problem hiding this comment.
Adding IChatBackgroundTaskService as a required constructor dependency will break existing RunInTerminalTool unit tests that instantiate the tool via workbenchInstantiationService without stubbing this service. Update the relevant tests to provide a stub/impl of IChatBackgroundTaskService (and consider adding assertions that command completion updates the task instead of sending a steering request).
| // Evict completed background tasks so their context is included in this request | ||
| if (submittedSessionResource) { | ||
| const tasks = this.chatBackgroundTaskService.getTasksForSession(submittedSessionResource).get(); | ||
| for (const task of tasks) { | ||
| if (task.status.get() === BackgroundTaskStatus.Completed || task.status.get() === BackgroundTaskStatus.Failed || task.status.get() === BackgroundTaskStatus.Cancelled) { | ||
| this.chatBackgroundTaskService.evictTask(task.taskId); |
There was a problem hiding this comment.
The comment says tasks are evicted "so their context is included in this request", but eviction happens after sendRequest has already captured attachedContext. This is misleading; update the comment to reflect the actual intent (e.g. to avoid re-attaching the same completed tasks to subsequent requests / to remove completed tasks after they've been consumed).
|
|
||
| cancel(): void { | ||
| if (this._status.get() !== BackgroundTaskStatus.Working) { | ||
| return; | ||
| } | ||
| this._status.set(BackgroundTaskStatus.Cancelled, undefined); | ||
| this._cancel(); | ||
| } | ||
|
|
||
| complete(result: unknown): void { | ||
| if (this._status.get() !== BackgroundTaskStatus.Working) { | ||
| return; | ||
| } | ||
| this._result.set(result, undefined); | ||
| this._status.set(BackgroundTaskStatus.Completed, undefined); | ||
| } | ||
|
|
There was a problem hiding this comment.
ChatBackgroundTask.cancel() sets status to Cancelled, but fail()/complete() can still overwrite it later. This causes cancel to end up as Failed when the terminal disposal handler runs (and can also flip Completed→Failed). Make complete()/fail() no-op unless the task is still in Working state (similar to cancel()), or otherwise enforce a one-way state transition.
| const map = this._tasksBySession.get(key); | ||
| return map ? [...map.values()] : []; | ||
| }); | ||
| } | ||
|
|
||
| getTask(taskId: string): IChatBackgroundTask | undefined { | ||
| for (const map of this._tasksBySession.values()) { | ||
| const task = map.get(taskId); |
There was a problem hiding this comment.
evictTask removes the task from the per-session map but leaves an empty session map behind. Over time, sessions that have had tasks will accumulate empty DisposableMaps. After deleting a task, if the per-session map is empty, deleteAndDispose that session entry too.
| store.add(bgTaskStore); | ||
| store.add(autorun(reader => { | ||
| const tasks = this._chatBackgroundTaskService.getTasksForSession(sessionResource).read(reader); | ||
| bgTaskStore.clear(); | ||
| for (const task of tasks) { | ||
| const attachment: IChatBackgroundTaskVariableEntry = { | ||
| kind: 'backgroundTask', | ||
| id: `backgroundTask:${task.taskId}`, | ||
| name: task.name, | ||
| icon: Codicon.loading, | ||
| value: '', |
There was a problem hiding this comment.
Inside the autorun, bgTaskStore.clear() disposes the old widgets but does not remove their root DOM nodes (the attachment widget base class doesn't remove this.element on dispose). This will leave stale background-task chips in the container and append duplicates on updates. Consider rendering background tasks into a dedicated sub-container and dom.clearNode that container on each run (or register an explicit disposable to remove each widget's element).
| terminalInstance.dispose(); | ||
| disposeNotification(); |
There was a problem hiding this comment.
onCancel disposes the terminal before calling disposeNotification(). If terminal disposal fires onDisposed synchronously, the onDisposed handler will mark the task as Failed ("Terminal was closed"), overriding the Cancelled state. Dispose the notification listeners before disposing the terminal, and/or gate the onDisposed failure handler on backgroundTask.status === Working.
This issue also appears in the following locations of the same file:
- line 2242
- line 2249
- line 2281
| terminalInstance.dispose(); | |
| disposeNotification(); | |
| disposeNotification(); | |
| terminalInstance.dispose(); |
Screenshot ChangesBase: Changed (2)
|
| Before | After |
|---|---|
 |
 |
editor/inlineCompletions/other/JumpToHint/Dark
| Before | After |
|---|---|
 |
 |
Removed (14)
chat/input/chatInput/Default/Dark
chat/input/chatInput/Default/Light
chat/input/chatInput/WithArtifacts/Dark
chat/input/chatInput/WithArtifacts/Light
chat/input/chatInput/WithFileChanges/Dark
chat/input/chatInput/WithFileChanges/Light
chat/input/chatInput/WithTodos/Dark
chat/input/chatInput/WithTodos/Light
chat/input/chatInput/WithTodosAndFileChanges/Dark
chat/input/chatInput/WithTodosAndFileChanges/Light
chat/input/chatInput/WithArtifactsAndFileChanges/Dark
chat/input/chatInput/WithArtifactsAndFileChanges/Light
chat/input/chatInput/Full/Dark
chat/input/chatInput/Full/Light
blocks-ci screenshots changed
Replace the contents of test/componentFixtures/blocks-ci-screenshots.md with:
Updated blocks-ci-screenshots.md
<!-- auto-generated by CI — do not edit manually -->
#### editor/codeEditor/CodeEditor/Dark
#### editor/codeEditor/CodeEditor/Light
fixes #308549
What It Does
This branch replaces the steering-message-based notification system for background terminal commands with a new
IChatBackgroundTaskService. Instead of sending a chat message when a background terminal command completes, the result is now captured as a background task attachment chip in the chat input area.Key Improvements
New service layer — chatBackgroundTask.ts defines the
IChatBackgroundTaskServiceinterface with observable status tracking (Working/Completed/Failed/Cancelled), and chatBackgroundTaskService.ts implements it.Visual attachment chips —
BackgroundTaskAttachmentWidgetin chatAttachmentWidgets.ts shows a reactive chip with a spinner while running, a checkmark on completion, or an error icon on failure. The clear button contextually says "Cancel" vs "Dismiss".User-driven consumption — Completed/failed task results are included as
IChatBackgroundTaskVariableEntrycontext when the user sends their next message (chatInputPart.ts), and evicted afterward (chatWidget.ts). This gives the user control over when the result is consumed rather than auto-injecting a steering message.Terminal tool integration — runInTerminalTool.ts creates a background task when a terminal enters background mode and updates it on completion/failure/disposal, with a cancellation callback that kills the terminal.
background-task.mov
cc @connor4312