wip terminal inline chat #270821

meganrogge · 2025-10-10T21:03:58Z

Branched from #261503 because there were many merge conflicts and I was nervous to break something

Current status:

To do:

when the instance is disposed, ensure the parts are hidden / disposed?
all of the comments on here

Part of #257468

meganrogge · 2025-10-10T21:04:46Z

...ch/contrib/chat/browser/chatContentParts/toolInvocationParts/chatTerminalToolProgressPart.ts

+import type { ITerminalInstance } from '../../../../terminal/browser/terminal.js';
+import { TerminalInstance, TerminalInstanceColorProvider } from '../../../../terminal/browser/terminalInstance.js';
+import { XtermTerminal } from '../../../../terminal/browser/xterm/xtermTerminal.js';
+// TODO@meganrogge fix


To do: fix somehow, this was also in initial PR

Copilot

Pull Request Overview

This PR implements terminal inline chat functionality by adding the ability to track terminal execution strategies and display terminal output inline within chat responses. The implementation allows chat responses to show live terminal output by creating dedicated terminal instances that mirror the execution happening in the background.

Key changes:

Enhanced terminal execute strategies with marker tracking and update events
Added terminal output display capabilities to chat terminal tool progress parts
Implemented terminal instance tracking and content synchronization

Reviewed Changes

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

Show a summary per file

File	Description
runInTerminalTool.ts	Adds execution tracking with UUID generation and strategy association
strategyHelpers.ts	Modifies marker disposal to prevent cleanup for persistent UI access
richExecuteStrategy.ts	Extends Disposable and adds marker/update event properties
noneExecuteStrategy.ts	Extends Disposable and adds marker/update event properties
executeStrategy.ts	Defines new interface properties for markers and update events
basicExecuteStrategy.ts	Extends Disposable and adds marker/update event properties
xtermTerminal.ts	Adds VT sequence serialization method for terminal content extraction
terminalInstance.ts	Updates color provider constructor to support flexible location types
terminal.ts	Adds interface method for VT range serialization
chatToolInvocationPart.ts	Reorders import statements
chatTerminalToolProgressPart.ts	Major enhancement adding terminal instance tracking and live output display

Copilot · 2025-10-10T21:06:13Z

.../workbench/contrib/terminalContrib/chatAgentTools/browser/executeStrategy/strategyHelpers.ts

+		// TODO@meganrogge !!! does this cause disposables to be leaked?
+		// Don't clear the start marker on disposal - we want it to persist
+		// so terminal UI parts can still access it
+		console.log('strategy disposed but keeping start marker');


Replace console.log with proper logging using the ITerminalLogService that's available in the execute strategy classes.

Suggested change

console.log('strategy disposed but keeping start marker');

log?.('strategy disposed but keeping start marker');

Copilot · 2025-10-10T21:06:13Z

...ch/contrib/chat/browser/chatContentParts/toolInvocationParts/chatTerminalToolProgressPart.ts

+			}
+		} else if (!this.xterm) {
+			// Missing required services, can't create terminal yet
+			console.log(`Can't create terminal for ${this.externalInstanceId} yet - missing required services`);


Replace console.log statements with proper logging. Consider using ILogService or another appropriate logging mechanism available through dependency injection.

Copilot · 2025-10-10T21:06:14Z

...ch/contrib/chat/browser/chatContentParts/toolInvocationParts/chatTerminalToolProgressPart.ts

+				console.log(`Start marker already exists for ${this.externalInstanceId} - using for initial data`);
+				await this.updateTerminalContent(instance, executeStrategy, executeStrategy.startMarker);
+			} catch (e) {
+				console.error(`Error getting initial terminal data for ${this.externalInstanceId}:`, e);
+			}
+		} else {
+			console.log(`No start marker available yet for ${this.externalInstanceId} - waiting for marker creation event. Has strategy: ${!!executeStrategy}`);


Replace all console.log/console.error statements with proper logging using an injected logging service. This will provide better debugging capabilities and follow VS Code's logging patterns.

Copilot · 2025-10-10T21:06:14Z

...ch/contrib/chat/browser/chatContentParts/toolInvocationParts/chatTerminalToolProgressPart.ts

+	): Promise<void> {
+		const latestPartId = ChatTerminalToolProgressPart.latestPartPerInstance.get(this.instanceType!);
+		if (latestPartId !== this.externalInstanceId) {
+			console.log(`Skipping update for ${this.externalInstanceId} as ${latestPartId} is the latest`);


Replace all console.log/console.error statements with proper logging using an injected logging service. This will provide better debugging capabilities and follow VS Code's logging patterns.

Copilot · 2025-10-10T21:06:14Z

...ch/contrib/chat/browser/chatContentParts/toolInvocationParts/chatTerminalToolProgressPart.ts

+			// Try persistent marker first
+			if (this.persistentStartMarker) {
+				startMarker = this.persistentStartMarker;
+				console.log(`Using persistent start marker for ${this.externalInstanceId}`);


Replace all console.log/console.error statements with proper logging using an injected logging service. This will provide better debugging capabilities and follow VS Code's logging patterns.

...ch/contrib/chat/browser/chatContentParts/toolInvocationParts/chatTerminalToolProgressPart.ts

Copilot · 2025-10-10T21:06:15Z

...ch/contrib/chat/browser/chatContentParts/toolInvocationParts/chatTerminalToolProgressPart.ts

+// eslint-disable-next-line local/code-import-patterns
+import type { ITerminalExecuteStrategy } from '../../../../terminalContrib/chatAgentTools/browser/executeStrategy/executeStrategy.js';


The eslint-disable comment indicates an import pattern violation that should be addressed. Consider restructuring the imports to follow the established patterns or moving the shared types to a more appropriate location.

Suggested change

// eslint-disable-next-line local/code-import-patterns

import type { ITerminalExecuteStrategy } from '../../../../terminalContrib/chatAgentTools/browser/executeStrategy/executeStrategy.js';

import type { ITerminalExecuteStrategy } from '../../../../terminalContrib/chatAgentTools/common/executeStrategyTypes.js';

Copilot · 2025-10-10T21:06:16Z

...ch/contrib/chat/browser/chatContentParts/toolInvocationParts/chatTerminalToolProgressPart.ts

+		// const icon = !toolInvocation.isConfirmed ?
+		// 	Codicon.error :
+		// 	toolInvocation.isComplete ?
+		// 		Codicon.check : ThemeIcon.modify(Codicon.loading, 'spin');


Remove commented-out code. If this icon logic is needed for future implementation, consider adding a proper TODO comment or implement it immediately.

Suggested change

// const icon = !toolInvocation.isConfirmed ?

// Codicon.error :

// toolInvocation.isComplete ?

// Codicon.check : ThemeIcon.modify(Codicon.loading, 'spin');

// TODO: Implement icon logic for toolInvocation status if needed (see previous commented-out code).

meganrogge · 2025-10-10T21:11:55Z

.../workbench/contrib/terminalContrib/chatAgentTools/browser/executeStrategy/strategyHelpers.ts

+		// TODO@meganrogge !!! does this cause disposables to be leaked?
+		// Don't clear the start marker on disposal - we want it to persist
+		// so terminal UI parts can still access it
+		console.log('strategy disposed but keeping start marker');
+		// Don't fire undefined marker event - keep the last valid marker
 	}));
-	store.add(startMarker);
+	// We don't register startMarker with the store to prevent it from being auto-disposed


todo: fix somehow, with this clearing code, it doesn't work.

But the peristentMarker thing should make this ok 🤔 . maybe timing issue

Tyriar · 2025-10-11T00:28:54Z

...ch/contrib/chat/browser/chatContentParts/toolInvocationParts/chatTerminalToolProgressPart.ts

+	// Map from real terminal instance IDs to chat terminal parts
+	private static readonly instanceToPartMap = new Map<string, Set<ChatTerminalToolProgressPart>>();
+
+	// Track the latest part for each terminal instance type (command, output display, etc)
+	private static readonly latestPartPerInstance = new Map<string, string>();


This thing was really hacky just to get something working, we need to come up with a proper way of transferring the terminal over to the tool. Since we don't necessarily know the instance to be used before this object is constructed it makes things more complicated.

Perhaps a new ITerminalChatService who's job it is initially purely to track these things and make sure they're serialized correctly (so things don't break on reload). Might be worth chatting about this in a sync together.

Agreed, and this file is getting way too big, needs separation.

Here are some notes I made:

// Chat Session // ├── Chat Request → Xterm → Instance (start marker → end marker) // ├── Chat Request → Xterm → Instance (start marker → end marker) // └── Chat Request → Xterm → Instance (start marker → end marker) // ... // Notes: // - Each Instance should be headless, hidden, and per Chat Request? // - Should allow opening terminals from the chat-embedded one? interface ChatEmbeddedTerminalService { map: Map<ChatSessionId, Xterm[]>; createTerminal(): Xterm; serialize(): string; deserialize(data: string): void;

Tyriar and others added 6 commits August 13, 2025 10:57

Inline terminal WIP

d888f3b

Part of #257468

Prefer h, add css file

6b5eb0b

Don't show standard progress part

0441a19

Merge branch 'main' into merogge/inline_t

569e8b4

revert a change

2621a64

pass back start marker

1d62dea

Copilot AI review requested due to automatic review settings October 10, 2025 21:04

meganrogge marked this pull request as draft October 10, 2025 21:04

meganrogge self-assigned this Oct 10, 2025

meganrogge commented Oct 10, 2025

View reviewed changes

Copilot AI reviewed Oct 10, 2025

View reviewed changes

get it to work per response

0e74037

meganrogge force-pushed the merogge/inline_t branch from 5b2a5e8 to 0e74037 Compare October 10, 2025 21:06

meganrogge commented Oct 10, 2025

View reviewed changes

rm console logs

141b0b0

Tyriar requested changes Oct 11, 2025

View reviewed changes

	console.log('strategy disposed but keeping start marker');
	log?.('strategy disposed but keeping start marker');

		// eslint-disable-next-line local/code-import-patterns
		import type { ITerminalExecuteStrategy } from '../../../../terminalContrib/chatAgentTools/browser/executeStrategy/executeStrategy.js';

wip terminal inline chat #270821

Are you sure you want to change the base?

wip terminal inline chat #270821

Uh oh!

Conversation

meganrogge commented Oct 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

meganrogge Oct 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

meganrogge Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

Tyriar Oct 11, 2025

Choose a reason for hiding this comment

Uh oh!

meganrogge Oct 11, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

meganrogge commented Oct 10, 2025 •

edited

Loading

meganrogge Oct 10, 2025 •

edited

Loading