Conversation
There was a problem hiding this comment.
Pull request overview
This PR renames the blockOnResponse property to resolveOnResponse and changes the behavior of the inline chat run() method. The property now controls whether to wait for the full session lifecycle (accept/reject) or just for the file modification to complete.
Key changes:
- Renamed
blockOnResponsetoresolveOnResponseinInlineChatRunOptions - Modified the
run()method to support two wait strategies based onresolveOnResponse - Changed
StartSessionActionto always await therun()call instead of conditionally
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/vs/workbench/contrib/inlineChat/browser/inlineChatController.ts | Renamed blockOnResponse to resolveOnResponse, added conditional logic to wait for either session disposal or file modification based on the flag value |
| src/vs/workbench/contrib/inlineChat/browser/inlineChatActions.ts | Simplified to always await the run() call, removing the conditional await based on blockOnResponse |
Comments suppressed due to low confidence (2)
src/vs/workbench/contrib/inlineChat/browser/inlineChatController.ts:68
- The renamed property
resolveOnResponsehas confusing semantics. WhenresolveOnResponseis false (the default), the function waits for the session to be accepted or rejected (longer wait). When it's true, it resolves earlier when the file is modified. This is counterintuitive - the name suggests it should resolve on response, but that's actually what happens when it's set to true, not the default behavior. Consider renaming toresolveEarlyorresolveOnModifiedto better reflect its purpose, or inverting the logic to make the default match the name.
resolveOnResponse?: boolean;
src/vs/workbench/contrib/inlineChat/browser/inlineChatController.ts:68
- Missing documentation comment for the
resolveOnResponseproperty. This property controls important behavior (whether to wait for session disposal or just file modification), and should have a JSDoc comment explaining when to use true vs false and what the implications are.
resolveOnResponse?: boolean;
| position?: IPosition; | ||
| modelSelector?: ILanguageModelChatSelector; | ||
| blockOnResponse?: boolean; | ||
| resolveOnResponse?: boolean; |
There was a problem hiding this comment.
The property has been renamed from blockOnResponse to resolveOnResponse, but this change is incomplete. The file src/vs/workbench/api/common/extHostApiCommands.ts still references blockOnResponse in lines 553, 567, and 577. These references must be updated to resolveOnResponse to maintain API consistency and prevent runtime errors when the API is invoked.
This issue also appears in the following locations of the same file:
- line 68
- line 68
No description provided.