chat: add requestNeedsInput on the chat model#278171
Merged
connor4312 merged 1 commit intomainfrom Nov 18, 2025
Merged
Conversation
With a side quest that makes elicitations correctly trigger this, which involved refactoring their state into an observable. Also swap requestInProgress/Obs to a single requestInProgress observable. Synchronous callers can just `.get()` it as needed.
📬 CODENOTIFYThe following users are being notified based on files changed in this PR: @jriekenMatched files:
@roblourensMatched files:
@bpaseroMatched files:
|
21 tasks
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR refactors chat elicitation state management from simple properties to observables and adds a new requestNeedsInput observable to track when user input is required. The changes improve reactivity and enable better UI state management.
Key Changes:
- Refactored elicitation state from string property to
IObservable<ElicitationState>for reactive updates - Consolidated
requestInProgressfrom separate boolean property and observable into a singleIObservable<boolean> - Added new
requestNeedsInputobservable to track pending confirmations and elicitations
Reviewed Changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/vs/workbench/contrib/chat/common/chatService.ts | Added ElicitationState enum; updated IChatElicitationRequest interface to use observable state; added IChatElicitationRequestSerialized type for serialization |
| src/vs/workbench/contrib/chat/common/chatModel.ts | Replaced requestInProgress property with observable; added requestNeedsInput observable; updated switch statements to handle both elicitation kinds |
| src/vs/workbench/contrib/chat/browser/chatElicitationRequestPart.ts | Changed state to observable; renamed kind to 'elicitation2'; refactored accept/_accept to return ElicitationState; updated serialization to 'elicitationSerialized' |
| src/vs/workbench/contrib/chat/common/chatViewModel.ts | Removed requestInProgress property delegation (now accessed from model) |
| src/vs/workbench/contrib/chat/common/chatServiceImpl.ts | Updated to use model.requestInProgress.read() and optimized with Iterable.some() |
| src/vs/workbench/contrib/chat/browser/chatWidget.ts | Updated to access requestInProgress via viewModel.model.requestInProgress.get() |
| src/vs/workbench/contrib/chat/browser/chatContentParts/chatElicitationContentPart.ts | Added reactive handling for state changes via autorun; conditional setup for live vs serialized elicitations |
| src/vs/workbench/contrib/chat/browser/chatListRenderer.ts | Added visibility check for hidden elicitations; handles both 'elicitation2' and 'elicitationSerialized' kinds |
| src/vs/workbench/contrib/mcp/browser/mcpElicitationService.ts | Updated callbacks to return ElicitationState; added backward compatibility for pre-2025-11-25 params |
| src/vs/workbench/contrib/mcp/browser/mcpCommands.ts | Updated kind check from 'elicitation' to 'elicitation2' |
| src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/monitoring/outputMonitor.ts | Updated accept/reject callbacks to return ElicitationState instead of void |
| src/vs/workbench/contrib/inlineChat/browser/inlineChatController.ts | Updated all requestInProgress accesses to use observable .get() or .read() |
| src/vs/workbench/contrib/inlineChat/browser/inlineChatWidget.ts | Changed to read from viewModel.model.requestInProgress.get() |
| src/vs/workbench/contrib/inlineChat/browser/inlineChatSessionServiceImpl.ts | Updated to use .read() on requestInProgress observable |
| src/vs/workbench/contrib/inlineChat/test/browser/inlineChatController.test.ts | Updated test assertion to use .get() on requestInProgress observable |
| src/vs/workbench/contrib/chat/browser/chatSessions/localChatSessionsProvider.ts | Updated to use observable pattern for requestInProgress |
| src/vs/workbench/contrib/chat/browser/chatSessions/chatSessionTracker.ts | Updated to use .get() on requestInProgress observable |
| src/vs/workbench/contrib/chat/browser/chatResponseAccessibleView.ts | Updated filter to handle both 'elicitation2' and 'elicitationSerialized' kinds |
| src/vs/workbench/contrib/chat/browser/chatAccessibilityService.ts | Updated to use observable state comparison with ElicitationState.Pending |
| src/vs/workbench/contrib/chat/browser/chatAccessibilityProvider.ts | Updated filter to handle both elicitation kinds |
| src/vs/workbench/contrib/chat/browser/actions/chatElicitationActions.ts | Updated to check observable state and compare with ElicitationState.Pending |
| src/vs/workbench/contrib/chat/browser/chatEditing/chatEditingEditorOverlay.ts | Changed property name from requestInProgressObs to requestInProgress |
| src/vs/workbench/contrib/chat/browser/chatEditing/chatEditingEditorContextKeys.ts | Updated to use .read() on requestInProgress observable |
| this.state.set(state, undefined); | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
The ChatElicitationRequestPart class is missing the reject() method implementation. While the interface IChatElicitationRequest declares reject as optional, the class has a _reject constructor parameter that should be wrapped in a public reject() method similar to how _accept is wrapped in accept().
Add the missing method:
reject(): Promise<void> {
if (!this._reject) {
return Promise.resolve();
}
return this._reject().then(state => {
this.state.set(state, undefined);
});
}
Suggested change
| reject(): Promise<void> { | |
| if (!this._reject) { | |
| return Promise.resolve(); | |
| } | |
| return this._reject().then(state => { | |
| this.state.set(state, undefined); | |
| }); | |
| } |
roblourens
reviewed
Nov 18, 2025
| return response?.isInProgress.read(r) ?? false; | ||
| }); | ||
|
|
||
| this.requestNeedsInput = lastResponse.map((response, r) => { |
Member
There was a problem hiding this comment.
How do you feel about two observables vs one state enum like Idle, Running, WaitingForConfirmation? The number of states might increase
Member
There was a problem hiding this comment.
I mean on second thought we should keep the basic requestInProgress regardless, and then could refactor if there are more things to track.
roblourens
approved these changes
Nov 18, 2025
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
With a side quest that makes elicitations correctly trigger this, which
involved refactoring their state into an observable.
Also swap requestInProgress/Obs to a single requestInProgress observable.
Synchronous callers can just
.get()it as needed.Refs #277318