-
Notifications
You must be signed in to change notification settings - Fork 34.3k
Update confirmations to use new proposed terminal style #260770
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 our terms of service and privacy 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
this._getUniqueCodeBlockUri(), | ||
true | ||
); | ||
textModelService.createModelReference(model.uri).then(ref => { |
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.
See my helper thenifNotDisposed
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 fact that we have to do this is confusing.
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.
I don't think I can use that since I need to call dispose
on it to free the reference when the chat part is disposed?
...rib/chat/browser/chatContentParts/toolInvocationParts/chatTerminalToolConfirmationSubPart.ts
Outdated
Show resolved
Hide resolved
@connor4312 mentioned that above, actually not a regression #260806 |
Part of #257468
Fixes #260705
Fixes #260776
Fixes #259088
This adds
ChatCustomConfirmationWidget2
that aims to be this new style for all tools.I did not test the following:
InstallExtensionsTool
/ExtensionsInstallConfirmationWidgetSubPart
as it's not hooked up. Adoption for others didn't have any problems though.kind: 'confirmation
types as defined in here, not sure where they show upInitial state:
Dropdown:
Delayed hover on command:
Terminal output polling:
Extension install (from copilot ext):
Task run:
Test run:
MCP (disclaimer wrapping is existing issue #260806):