Skip to content

Fix chat perf to addRequest first#307729

Draft
pwang347 wants to merge 5 commits intomainfrom
pawang/chatPerfFix
Draft

Fix chat perf to addRequest first#307729
pwang347 wants to merge 5 commits intomainfrom
pawang/chatPerfFix

Conversation

@pwang347
Copy link
Copy Markdown
Member

@pwang347 pwang347 commented Apr 3, 2026

No description provided.

Copilot AI review requested due to automatic review settings April 3, 2026 21:22
Copy link
Copy Markdown
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 PR restructures chat request sending to improve perceived performance by creating the request model (and triggering UI update) before doing potentially slow async work like hook/instruction collection.

Changes:

  • Refactors ChatServiceImpl to call model.addRequest(...) before awaiting hook/instruction collection, and collects hooks + instructions in parallel afterward.
  • Moves automatic-instruction collection (previously done in the widget) into the service behind a new instructionContext option.
  • Updates ChatWidget to pass instructionContext and removes its prior _autoAttachInstructions flow.
Show a summary per file
File Description
src/vs/workbench/contrib/chat/common/chatService/chatServiceImpl.ts Creates request model earlier; adds async hook/instruction collection helpers; integrates collected instructions into variableData before invoking the agent.
src/vs/workbench/contrib/chat/common/chatService/chatService.ts Adds instructionContext to IChatSendRequestOptions (and imports ChatModeKind).
src/vs/workbench/contrib/chat/browser/widget/chatWidget.ts Removes widget-side auto instruction attachment and passes instructionContext into sendRequest.

Copilot's findings

Comments suppressed due to low confidence (2)

src/vs/workbench/contrib/chat/common/chatService/chatService.ts:1432

  • instructionContext is required for instruction collection in ChatServiceImpl, but pending/queued requests are persisted with a serialized sendOptions that currently does not include this field (see serializeSendOptions/ISerializableSendOptions in chatModel.ts). After reload/restore, queued requests may run without automatic instruction collection. Consider adding instructionContext (or enough data to recompute it) to the serializable send-options schema so restored pending requests behave the same as live ones.
	instructionContext?: {
		modeKind: ChatModeKind;
		enabledTools?: UserSelectedTools;
		enabledSubAgents?: readonly string[];
	};

src/vs/workbench/contrib/chat/common/chatService/chatServiceImpl.ts:1165

  • collectInstructions logs an error for any thrown exception, including request cancellation. Since this uses the request cancellation token, cancellations will likely produce noisy error logs. Consider checking isCancellationError(err) (or token.isCancellationRequested) and skipping logging (or lowering to trace) for cancellations.
				} catch (err) {
					this.logService.error('[ChatService] Failed to collect instructions:', err);
					return [];
  • Files reviewed: 3/3 changed files
  • Comments generated: 3

pwang347 and others added 3 commits April 3, 2026 14:30
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants