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

Cody: Consolidate inline and non-stop fixups #510

Merged
merged 27 commits into from
Aug 7, 2023
Merged

Conversation

umpox
Copy link
Contributor

@umpox umpox commented Aug 1, 2023

Description

This PR consolidates the inline fixes with the non-stop fixup functionality. Changes:

  • It is now possible to run fixups in parallel. There is no longer a need to schedule fixups.
  • The inline fix flow now users the non-stop fixup UX.
  • It is now possible to trigger a fixup on an empty selection. I have added a special case for this: add. We change the prompt and context slighly.
  • Better formatting on fixup responses.
  • Fixups are now available as code actions on errors and warnings (see screenshot)
  • Improved non-stop fixup flow. Added a quick pick that has a slash command picker. This allows users to skip ahead of the LLM intent detection by being explicit. (see screenshot)
  • Integrated the fixup input flow with the new commands input

Screenshots / Demos

Quick Pick:

QuickPickFlow.mov

Quick Fixes:

CodyQuickFix.mov

Explicit fix intent:

image

Updated command menu:

image

Test plan

Local testing, probably need to write some updated E2E tests now too

@umpox umpox force-pushed the tr/fixup-and-non-stop branch 2 times, most recently from f5482ad to 6dc438c Compare August 2, 2023 13:22
@sqs
Copy link
Member

sqs commented Aug 3, 2023

Minor, but I like the look of the fixup input box if it is a quickpick a bit better because it doesn't have that annoying Press 'Enter' to confirm your input or 'Escape' to cancel text.

Here is a quick hacky addition to FixupTypingUI.ts to get this (doesn't handle disposals or deleting the other code, you get the idea):

+        const qp = vscode.window.createQuickPick()
+        qp.title = 'Cody'
+        qp.placeholder = "Ask Cody to do something, or type '/' for commands"
+        qp.buttons = [{ tooltip: 'Cody', iconPath: new vscode.ThemeIcon('cody-logo-heavy') }]
+        qp.ignoreFocusOut = true
+        qp.show()
+
+        qp.onDidTriggerButton(() => {
+            void vscode.commands.executeCommand('cody.focus')
+            qp.hide()
+        })
+        const text = await new Promise(resolve => qp.onDidAccept(resolve))

We could also then make it autocomplete /-commands in the quickpick (but it's nice to have no quickpick items showing by default because it's super clean).

Feel free to disregard if you don't like this.

image

Compare to the current:

image

@umpox
Copy link
Contributor Author

umpox commented Aug 3, 2023

@sqs I like it, I think adding autocomplete / commands here too will be useful - as it's an option for users can bypass the LLM prompt detection for more consistent results (thinking for complex instructions where we might struggle to infer their intent).

Will add to the new command menu too and make it prominent (open to naming suggestions, Fix this code?):
image

@umpox
Copy link
Contributor Author

umpox commented Aug 3, 2023

I think quick picks have a limitation that they require an existing item to be selected. I saw other extensions can work around this by dynamically adding a new item based on the users input, there's a slight delay but I think it's worth it still.

@sqs
Copy link
Member

sqs commented Aug 3, 2023

@umpox A quick pick can have zero items or a disabled item with placeholder text, which probably accomplishes what we need. (BTW as a user I would strongly prefer for the fixup box to not have a bunch of items below it when I first open it, to make it clean.)

@umpox
Copy link
Contributor Author

umpox commented Aug 3, 2023

@sqs Thoughts on this?

QuickPickFlow.mov

@umpox umpox force-pushed the tr/fixup-and-non-stop branch 2 times, most recently from ebd7339 to 2f3d2d7 Compare August 3, 2023 15:37
I have kept the task view behind a flag as we should move it to a quick pick in a follow up PR
@umpox
Copy link
Contributor Author

umpox commented Aug 3, 2023

@abeatrix, @sqs, @dominiccooney At the end of my day so opening this up for 👀 on an initial review of this.

I think the remaining work left is:

  • Fix tests
  • Check everything works as expected for fixups.
  • Check the formatting logic. I'm not 100% if using the cursor position is the right approach.
  • Make sure we're doing appropriate cleanup. We should dispose of the specific fixup provider once a fixup is accepted

Follow up work:

  • Remove fixup references from inline controller
  • Remove idle references from fixup controller
  • Move fixup task list to quick pick so it doesn't obstruct the chat window.

@umpox umpox marked this pull request as ready for review August 3, 2023 16:06
Copy link
Contributor

@abeatrix abeatrix left a comment

Choose a reason for hiding this comment

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

Still in the process of testing but here are a few things I have noticed so far:

  1. Should we name /document or /test to something else if they are not using the same commands as the /doc and /test commands in chat? I was expecting it to work the same as those chat commands and was confused what I need to enter in the input box in the next step:
    image

  2. I also find the cody icon in the upper right misleading because it works as a close button?

  3. In the main Cody Commands menu, maybe we can group the Ask a Question and Refactor this code under the inline separator? CC @toolmantim for design input

image

  1. Clicking on the Fixup ready code lenses is supposed to be no ops, but it adds a new line to the code
    cl

5 Code lens to apply changes add new line above the function

  1. add it back to submenu?
    image

  2. /fix /test command add the new test to the current file, which doesn't seem to make sense to me 🤔

LOVE the code action! It was something on our backlogs that we didn't get to for last sprint and i'm so glad you picked this up 😭
(maybe it's just me but I was expecting Explain with Cody to explain the function and not just the error, maybe updating
it to something more descriptive? e.g Explain Error with Codyor Ask Cody to Fix/ Ask Cody to Explain? )
image

vscode/package.json Outdated Show resolved Hide resolved
vscode/package.json Outdated Show resolved Hide resolved
Copy link
Contributor

@abeatrix abeatrix left a comment

Choose a reason for hiding this comment

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

Everything works amazingly for me 🎉 There are some steps that confused me when i was trying it out (see comment above), but I don't think they are blockers.

I do have a question about having two separate icons in the title bar for Cody and Fixup, though. @sqs mentioned having an additional icon in the title bar might not be something users appreciate and prefer to have it set behind a feature flag. Since the Cody icon brings up the Command QuickPick that already has Fixup as an option, maybe we can remove the dedicated icon for fix?
image

@umpox
Copy link
Contributor Author

umpox commented Aug 4, 2023

THANK YOU for the thorough review @abeatrix, it's really appreciated.

I also find the cody icon in the upper right misleading because it works as a close button?

It actually shows and focuses the Cody sidebar. I think most users won't have the sidebar open so it might actually be OK to keep this as is. I'm going to make a PR to stop opening the sidebar by default for local dev as this confused me at first too

In the main Cody Commands menu, maybe we can group the Ask a Question and Refactor this code under the inline separator?

I agree, will update.

image

Clicking on the Fixup ready code lenses is supposed to be no ops, but it adds a new line to the code

Code lens to apply changes add new line above the function

I believe this is from the formatting logic I added. I'm removing that, I want to rethink it to come up with a better approach.

add it back to submenu?

Will do!

image

Should we name /document or /test to something else if they are not using the same commands as the /doc and /test commands in chat?

/fix /test command add the new test to the current file, which doesn't seem to make sense to me 🤔

I think the commands are a bit confusing right now. I'll rethink this a little and update. I think we might want an initial subset of commands ('fix', 'edit', 'document') and then the rest we detect from the users instructions and provide additional context for.

or Ask Cody to Fix/ Ask Cody to Explain?

I like these for the code actions, will update

image

@umpox umpox force-pushed the tr/fixup-and-non-stop branch 2 times, most recently from 7f0ad2e to 6faff21 Compare August 7, 2023 08:50
Comment on lines -37 to -40
if (selection.label === 'Submit question') {
selection.detail = input
}
resolve(selection)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@abeatrix FYI, this didn't seem to work for me, I got weird responses 🤔 I just updated this function to return and object containing selection and input separately

@umpox
Copy link
Contributor Author

umpox commented Aug 7, 2023

Getting this in, and getting a follow-up PR with a proposal for the command situation

@umpox umpox merged commit 74d2cd8 into main Aug 7, 2023
8 checks passed
@umpox umpox deleted the tr/fixup-and-non-stop branch August 7, 2023 11:16
@umpox umpox mentioned this pull request Aug 7, 2023
abeatrix added a commit that referenced this pull request Aug 21, 2023
RE: #765

#510 (comment)
added the nested commands to the `Refactor Menu` with menu title `Cody`
that is similar to the `Cody Commands Menu` which has been causing a lot
of confusion.

There are follow-up works that @umpox has planned for, but before those
are completed, I proposed the following in this PR:
- remove the nested commands from the refactor menu
- rename the refactor menu from `Cody` to `Cody: Refactor Code` to avoid
confusion
- replace placeholder text for the refactor menu from `Tell Cody what to
do, or type '/' for commands` to `Enter your refactoring instruction
here...`

## Test plan

<!-- Required. See
https://docs.sourcegraph.com/dev/background-information/testing_principles.
-->

### Before


![image](https://github.com/sourcegraph/cody/assets/68532117/a34eed35-24c4-456d-b785-e1e886caf787)


### After


![image](https://github.com/sourcegraph/cody/assets/68532117/57d49521-e3b2-43dc-b71d-89ef36320a3c)


![refactormenu](https://github.com/sourcegraph/cody/assets/68532117/0e4bc151-a41a-47fe-9a33-dc3272a1c3b0)

---------

Co-authored-by: Tim Lucas <t@toolmantim.com>
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

4 participants