Skip to content

get hidden chat terminal count to update #276163

Merged
meganrogge merged 8 commits intomainfrom
merogge/hidden-bg
Nov 7, 2025
Merged

get hidden chat terminal count to update #276163
meganrogge merged 8 commits intomainfrom
merogge/hidden-bg

Conversation

@meganrogge
Copy link
Contributor

@meganrogge meganrogge commented Nov 7, 2025

address #275789 in main

This specifically fixes the background terminal case, where onDidChangeInstances was not firing, so the entry would not get updated.

Also, the terminalTabbedView was not updated to only consider hidden instances.

@meganrogge meganrogge requested review from Tyriar and Copilot and removed request for Copilot November 7, 2025 19:03
@meganrogge meganrogge self-assigned this Nov 7, 2025
@meganrogge meganrogge added this to the October 2025 milestone Nov 7, 2025
@meganrogge meganrogge enabled auto-merge (squash) November 7, 2025 19:03
Copilot AI review requested due to automatic review settings November 7, 2025 19:07
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This pull request refactors the logic for handling hidden chat terminals by centralizing the filtering logic within getToolSessionTerminalInstances(). The main goal is to improve code maintainability by eliminating duplicate logic for determining which chat terminals are hidden.

  • Added an optional hiddenOnly parameter to getToolSessionTerminalInstances() to filter for hidden terminals
  • Refactored callers to use the new parameter instead of duplicating filtering logic
  • Fixed a menu visibility condition to use the correct context key

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/vs/workbench/contrib/terminalContrib/chat/browser/terminalChatService.ts Added hiddenOnly parameter to getToolSessionTerminalInstances() and refactored _updateHasToolTerminalContextKeys() to use it
src/vs/workbench/contrib/terminalContrib/chat/browser/terminalChatActions.ts Fixed menu visibility condition to use hasHiddenChatTerminals instead of hasChatTerminals
src/vs/workbench/contrib/terminal/browser/terminalTabsChatEntry.ts Refactored to use new hiddenOnly parameter and replaced context key service with terminal service
src/vs/workbench/contrib/terminal/browser/terminalTabbedView.ts Separated event listeners and updated to use hiddenOnly parameter for determining tab visibility
src/vs/workbench/contrib/terminal/browser/terminalService.ts Added onDidChangeInstances event firing when creating instances
src/vs/workbench/contrib/terminal/browser/terminal.ts Updated interface documentation to reflect new hiddenOnly parameter
Comments suppressed due to low confidence (1)

src/vs/workbench/contrib/terminal/browser/terminalTabbedView.ts:153

  • Both event handlers execute identical code. Consider using Event.any() to combine these two event listeners into a single handler to reduce code duplication: this._register(Event.any(this._terminalChatService.onDidRegisterTerminalInstanceWithToolSession, this._terminalService.onDidChangeInstances)(() => { this._refreshShowTabs(); this._updateChatTerminalsEntry(); }));
		this._register(Event.any(this._terminalChatService.onDidRegisterTerminalInstanceWithToolSession, this._terminalService.onDidChangeInstances)(() => {
			this._refreshShowTabs();
			this._updateChatTerminalsEntry();
		}));

		this._register(contextKeyService.onDidChangeContext(e => {
			if (e.affectsSome(new Set(['hasHiddenChatTerminals']))) {
				this._refreshShowTabs();

@meganrogge meganrogge merged commit 1be0303 into main Nov 7, 2025
28 checks passed
@meganrogge meganrogge deleted the merogge/hidden-bg branch November 7, 2025 19:39
meganrogge added a commit that referenced this pull request Nov 7, 2025
get hidden chat terminal count to update  (#276163)
@vs-code-engineering vs-code-engineering bot locked and limited conversation to collaborators Dec 22, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants