feat: code smell command, display commands error in chat

[abeatrix]

RE feedback from beyang:

• Could we add tab-to-complete behavior, so if the user has typed in a partial match for the command name (e.g., “/expla”) and then hits Tab, we complete the command name (in this case “/explain”)?
• I noticed nothing happens if I execute /explain without anything selected. It seems intuitive to me to use the currently open file as context in this case.
• I think it makes sense to display any errors (like you need to have something selected) as a response in the chat, rather than as a separate error pop-up

This PR includes the following change:

1. add /smell for the improved version of the old Code Smell recipe
2. for the default /explain & /smell command, run on visible content when there is no selection in the current text document
3. Display error message from commands in chat as an error message from the assistant when selection is required for a command (e.g. /doc and /test) but currently missing:
4. log basic telemetryService for commands
5. also cleaned up the custom-prompt recipe that all commands depend on.
6. show error for invalid commands:

1. add `/smell`, command for code smell as the default command 1. display the file name and selection range when available

Some UI changes were moved to separate PRs

See moved to #606 & #648

• add tab-to-complete + enter-to-complete behavior in the command list
• update the walkthrough to replace recipes with commands and add a new commands section:
• add short instructions on how to use commands with the new menu. The link also works as a button to open the command menu:
• update shortcut for commands menu to alt+c as it was using the same key as cody.fixup.new

Test plan

See the complete demo:

all.changes.covered.in.pull_602.mp4

1. start cody from this branch
2. run /doc without code selection - error expected
3. run non-exisit command:/exit - error expected
4. run /explain and smell without code selection - both show work with visible content
5. clicking on the Cody Menubutton in the cody intro should open the command menu for you
6. shortcut alt+c should bring up the command menu
7. run commands from the right-click menu, everything should work accordingly

[toolmantim]

I gave the tab complete behaviour a go locally, and looks like it needs a small fix to allow you to hit enter after hitting tab to complete.

I noticed nothing happens if I execute /explain without anything selected. It seems intuitive to me to use the currently open file as context in this case.

I don't think we want to do this so generically — it looks like if you use /document with no code selected (but a file open) it now tries to explain the whole file?

And if we scope it to explain: how good is the experience explaining a whole file, vs nudging them to select a specific bit of code? Should we consider cursor position too?

I think it makes sense to display any errors (like you need to have something selected) as a response in the chat, rather than as a separate error pop-up

Yeah, if you're triggering it from the chat window that is nice. Not sure if we want it as permanent part of the transcript though? More like a small error overlay that pops up above the compose box and hides after 5 seconds?

When you trigger stuff in-editor, without the chat window open, it would be nice if we didn't focus our chat pane if we didn't need to (add documentation, do an edit, etc). In that case, we'd want to keep the current behaviour of a native notification.

(Maybe worth breaking out the small tab completion fix from the rest?)

[abeatrix]

I don't think we want to do this so generically — it looks like if you use /document with no code selected (but a file open) it now tries to explain the whole file?

And if we scope it to explain: how good is the experience explaining a whole file, vs nudging them to select a specific bit of code? Should we consider cursor position too?

@toolmantim I think i used the wrong selection. Instead of selection or entire file, it should be selection or visible content, do you think that makes more sense? (Will take a look and make an update tomorrow if that sounds good to you)

[abeatrix]

@toolmantim and what do you think about using visible content for /doc and /test if no selection was made?

i think what you said about I don't think we want to do this so generically makes sense esp for /doc and /test that works best with selected code 🤔

(I just reread beyang's message and I think I misunderstood his message. Will continue tomorrow 🫠)

[abeatrix]

( removing beyang as reviewer for now )

[toolmantim]

And if we scope it to explain: how good is the experience explaining a whole file, vs nudging them to select a specific bit of code? Should we consider cursor position too?

I think i used the wrong selection. Instead of selection or entire file, it should be selection or visible content, do you think that makes more sense? (Will take a look and make an update tomorrow if that sounds good to you)

I'm not sure! I didn't meant to suggest the code in the PR was wrong… it definitely works as advertised.

I was more just wanting to check if the results it was returning was good? I was getting mixed results when I quickly tested it locally, and wasn't sure if it was a default path we wanted to push people down.

But I think what you've done matches what @beyang was suggesting, and it matches the context UI that's below the message box (the part that shows the current file)

Maybe it would be better if we took into account editor cursor as well (and make sure the context UI updates when you move lines)? That would hugely help for /document, where you have to select a whole line (otherwise it tries to document the whole file). And if you have the code selected, and use "Insert at cursor", it replaces the code itself with the documentation 😅

[toolmantim]

I just ran another test if it helps. /explain seems to work well on a whole file, but /doc and /test don't seem to work well:

In theory /doc could work on a whole file, just like /explain… generating a big summary comment you might want to insert at the top?

I'm not sure how /test could be changed to make sense for the whole file? 🤔

[toolmantim]

With how the errors are displayed, we already have an in-chat error mechanism for when we executing multiple recipes at once:

We should avoid adding yet another way of showing errors. We're still going to need to use VS Code native ones for in-editor experiences (if we keep the chat sidebar hidden).

Here's a better in-chat error design for something we can use for both the above error message, a /test error, and any custom commands that require code to be selected:

Screen.Recording.2023-08-08.at.5.53.39.pm.mov

This avoids cleaning the input box, making it an expensive mistake for the user. Instead it keeps the input box and cursor unchanged, so you can go select code and easily try again… a nice fast & snappy feedback loop, helping people to just click around and try commands without it feeling too heavyweight if they make a mistake.

[abeatrix]

Thanks for the thorough testing TIm! Much appreciated 🙇‍♀️

I just ran another test if it helps. /explain seems to work well on a whole file, but /doc and /test don't seem to work well

I will clarify with Beyang but I think I misunderstood his initial feedback. I think was talking about /explain specifically and not /doc and /test, so that was my bad 😅

We should avoid adding yet another way of showing errors.

@toolmantim this.transcript.addErrorAsAssistantResponse(errorMsg) is an existing to add error to display as a response from Cody. Beyang was thinking "that's the way it would show up if you executed a command in the terminal, and this is similar to that", but from design perspective, do you think displaying this error in the banner is more appropriate in this case?

P.S. We can update the existing error banner style to what you have in the design in another PR

[toolmantim]

Yeah, I think the error banner is more appropriate in this context… even a terminal application might do smart things to not clear your REPL input buffer on validation fail. But definitely keen to hear any strong opinions otherwise!

[beyang]

I think the most natural form for error messages would just be as a response message in the REPL. Similar to how in your terminal, if you type in an invalid command, the output will be an error message:

See also slash-commands in Slack:

Non-existent command:

Invalid command invocation:

In a REPL, I expect responses—whether success or error—to print in the main text area. So if you tried to run /doc without a selection of code, the response could be a message like "Select some code to document and re-run /doc".

[abeatrix]

@beyang @toolmantim I've updated the PR so that:

• when no code is selected when running /explain, it will use visible content automatically
• when there are selected code when running /explain, use selected code
• when no code is selected for /doc and /test, display error in chat instead of the vs code system error pop up
  See demo video listed under Test plan for details.

I've also updated the walkthrough to replace recipes and commands.

[beyang]

s/context/requiredContext/ perhaps?

[abeatrix]

Keeping this as it is for now if that's not an issue, as we already have users setting up commands with this field.