Skip to content
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

Split TS' AI-backed code actions into separate entries #201140

Merged
merged 14 commits into from Jan 31, 2024

Conversation

sandersn
Copy link
Member

Lets the user decide whether to add AI to their code action, which shows intent, which is good for us to learn whether people actually want this.

Related: this should be unflagged for insiders. To do this, do I just delete the flags? But I think the code should check whether github copilot chat is installed then, instead of checking for the flag.

Lets the user decide whether to add AI to their code action, which shows
intent, which is good for us to learn whether people actually want this.

Related: this should be unflagged for insiders. To do this, do I just
delete the flags?
@mjbvz mjbvz added this to the December / January 2024 milestone Jan 4, 2024
mjbvz
mjbvz previously approved these changes Jan 4, 2024
@@ -336,7 +346,7 @@ class TypeScriptQuickFixProvider implements vscode.CodeActionProvider<VsCodeCode
expand = { kind: 'code-action', action };
}
else if (action.fixName === fixNames.fixMissingFunctionDeclaration && vscode.workspace.getConfiguration('typescript').get('experimental.aiCodeActions.missingFunctionDeclaration')) {
title += `Implement missing function declaration '${document.getText(diagnostic.range)}' using Copilot`;
title = `Implement missing function declaration '${document.getText(diagnostic.range)}' using Copilot`;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like a few of these messages could be marked to be localized

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I'm working with the JS/TS team to share resources with an eventual VS version of these code actions, which will make it more complex, because the title will likely be loaded from a json file or something.

TylerLeonhardt
TylerLeonhardt previously approved these changes Jan 4, 2024
@sandersn
Copy link
Member Author

sandersn commented Jan 5, 2024

Quick note: don't merge this yet; my current testing still shows it replacing the original codefix. Not sure what I was doing when I tested it 3 weeks ago.

Edit: Looks like it's because both the original code action and the aiCodeAction have codeAction.edit = getEditForCodeAction(this.client, action). I guess there's some kind of de-duping code later that I'll need to work around.

It's a workaround--I'm not sure of the right way to do this.
@sandersn sandersn dismissed stale reviews from TylerLeonhardt and mjbvz via 6558500 January 5, 2024 17:51
@sandersn
Copy link
Member Author

sandersn commented Jan 5, 2024

I pushed a hacky workaround for quickfixes -- refactors are already fine, which must have been what I actually tested when I wrote this in December.

Edit: I guess that the code action for refactors doesn't set an edit until the action is resolved, so it's not available for deduping.
Edit 2: No, quickfix.ts just has an ad-hoc CodeActionSet class dedicated to deduping and re-ordering fixes. This class needs to change.

I still haven't figured out how to mark actions with isAI, but maybe that's not worth putting in this PR.

@sandersn
Copy link
Member Author

sandersn commented Jan 9, 2024

I modified the CodeActionSet class, but the change is only worthwhile if I have access to isAI so that I can skip the deduping code when isAI=true. @mjbvz, you linked me to a commit with a proposed change -- is this available on main? How do I get access to it in the typecript extension?

@mjbvz
Copy link
Contributor

mjbvz commented Jan 12, 2024

@sandersn I've pushed the changes to use isAi. Let me know if you want me to review the PR again and do some local testing

@sandersn
Copy link
Member Author

Sorry, I'm not sure how I missed this. I'll rewrite my change to use isAI and push another commit.

@sandersn
Copy link
Member Author

I couldn't get the codeActionAI proposal to compile, and it's not compiling on CI either, but I changed the code to use isAI to put AI-backed codeactions last.

@mjbvz mjbvz modified the milestones: December / January 2024, February 2024 Jan 23, 2024
@sandersn
Copy link
Member Author

sandersn commented Jan 23, 2024

Compiles on my (mac) laptop, so I pushed a small fix + a merge from main.

Edit: But not on CI machines. =(
Edit: And not on my Linux dev box.

It's possible to have copilot installed without copilot-chat.
@sandersn
Copy link
Member Author

sandersn commented Jan 23, 2024

I removed the experimental flag for AI code actions and instead check for github.copilot-chat.

@mjbvz mjbvz enabled auto-merge (squash) January 31, 2024 22:12
@mjbvz mjbvz merged commit 5e6ec06 into microsoft:main Jan 31, 2024
6 checks passed
@sandersn sandersn deleted the separate-ts/ai-codeactions branch February 1, 2024 16:55
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.

None yet

3 participants