- Notifications
You must be signed in to change notification settings - Fork37k
use v2 controller with old notebook prompt#284160
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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 refactors the inline chat controller architecture to always use the v2 controller (InlineChatController2), even in notebook contexts, while preserving the "old prompt" behavior when the notebook agent configuration is disabled. Previously, the code dynamically switched between v1 and v2 controllers based on whether the editor was in a notebook and the notebook agent config setting. Now, v2 is always used, and the notebook agent config only affects whether location resolve data is provided.
Key Changes:
- Simplified
CTX_INLINE_CHAT_V2_ENABLEDcontext key expression to enable v2 whenever agent2 is available, regardless of notebook context - Removed dynamic controller delegation from
InlineChatController, now always delegates toInlineChatController2 - Moved notebook agent configuration check from controller selection logic into location resolution within
InlineChatController2
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/common/inlineChat.ts | Updated context key expression to enable v2 controller in notebooks with agent2, not just when notebook agent is explicitly available |
| src/vs/workbench/contrib/inlineChat/browser/inlineChatController.ts | Simplified InlineChatController wrapper to always use v2; moved notebook agent config check into InlineChatController2's location setup to conditionally provide resolve data |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| constgroupKind=progress.kind==='textEdit'&&!notebookUri ?'textEditGroup' :'notebookEditGroup'; | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| constedits:any=groupKind==='textEditGroup' ?progress.edits :progress.edits.map(edit=>TextEdit.isTextEdit(edit) ?{uri:progress.uri, edit} :edit); | ||
| constedits:any=// TextEdit[] | (ICellTextEditOperation | ICellEditOperation)[] = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this is a typing puzzle that I wasn't able to solve. I don't think it likes the mixture of cell / celltext edit operations. Co-pilot's solution was to triplicate this section of code and still involved anas cast.
I think we're forcing too many operation types through the same method here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yea I think we should just split this out, or maybe have a helper function with some generic parts that lets us express things correctly. THis specificany cast was already a source of a bug in the past
| this._responseParts.push({ | ||
| kind:groupKind, | ||
| uri, | ||
| edits:groupKind==='textEditGroup' ?[edits] :edits, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
according to the types (that are still being ignored), all edits in this branch should be put into an array.
Nothing was broken before because notebook edits from the panel/agent always initially send a notebook entry through with 0 edits (to establish that edit group?), so only appended to the found edit group above
| }elseif(progress.kind==='textEdit'||progress.kind==='notebookEdit'){ | ||
| // If the progress.uri is a cell Uri, its possible its part of the inline chat. | ||
| // Old approach of notebook inline chat would not start and end with notebook Uri, so we need to check for old approach. | ||
| constuseOldApproachForInlineNotebook=progress.uri.scheme===Schemas.vscodeNotebookCell&&!this._responseParts.find(part=>part.kind==='notebookEditGroup'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
the v1 backend still doesn't start with a notebook URI, but we don't want to create a raw text edit without an associated notebook URI. This is why this path was previously broken when mixing inline v1/v2 with notebooks
| exportconstCTX_INLINE_CHAT_REQUEST_IN_PROGRESS=newRawContextKey<boolean>('inlineChatRequestInProgress',false,localize('inlineChatRequestInProgress',"Whether an inline chat request is currently in progress")); | ||
| exportconstCTX_INLINE_CHAT_RESPONSE_TYPE=newRawContextKey<InlineChatResponseType>('inlineChatResponseType',InlineChatResponseType.None,localize('inlineChatResponseTypes',"What type was the responses have been receieved, nothing yet, just messages, or messaged and local edits")); | ||
| exportconstCTX_INLINE_CHAT_V1_ENABLED=ContextKeyExpr.or( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this will no longer be true, but removing it will be a large change that i'd like to keep independent
connor4312 left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
edit part looks fine, I'll jet@jrieken review the inline chat parts
#282015