Don't dispose background terminals the user revealed#316295
Merged
Merged
Conversation
The previous heuristic used shellLaunchConfig.hideFromUser, which stays true for the lifetime of a tool-created terminal. Once the user clicks 'Show', the terminal moves into a group/editor and joins foregroundInstances, but hideFromUser is never flipped — so the post-completion cleanup was disposing revealed terminals. Use foregroundInstances membership instead, which correctly reflects whether the user has surfaced the terminal.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adjusts the background-terminal auto-disposal logic in the terminal chat agent tools so that terminals a user has revealed (via the “Show” link) are no longer disposed when their command completes. It aligns disposal decisions with the terminal’s actual UI visibility state rather than the immutable shellLaunchConfig.hideFromUser creation-time flag.
Changes:
- Switch the “should dispose finished background terminal” check from
shellLaunchConfig.hideFromUsertoITerminalService.foregroundInstancesmembership. - Reuse the computed
willDisposefor both the steering message text and the actual disposal decision. - Update inline comments to document the new visibility/disposal rule.
Show a summary per file
| File | Description |
|---|---|
| src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/runInTerminalTool.ts | Uses foregroundInstances to decide whether a finished background terminal should be disposed after completion. |
Copilot's findings
- Files reviewed: 1/1 changed files
- Comments generated: 1
Address review: the willDispose decision was computed before the async capture path, so a terminal revealed by the user during capture would still be disposed. Now re-check foregroundInstances inside the .then() callback right before disposal. Also add foregroundInstances to the ITerminalService test mocks to fix the 'Cannot read properties of undefined' test failure. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
dmitrivMS
previously approved these changes
May 13, 2026
…o execution Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
dmitrivMS
approved these changes
May 13, 2026
NikolaRHristov
pushed a commit
to CodeEditorLand/Editor
that referenced
this pull request
May 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The post-completion auto-disposal added in #316278 used
shellLaunchConfig.hideFromUserto decide whether to clean up a finished background terminal. That flag is set once at terminal creation and never flipped when the user clicks the 'Show' link, so revealed terminals were still being disposed.Use
ITerminalService.foregroundInstancesmembership instead —showBackgroundTerminalmoves the instance into a group/editor, putting it inforegroundInstances, which correctly reflects 'user has surfaced this terminal'.Follow-up to #316278.