refactor: move commands away from recipe #2542

abeatrix · 2024-01-02T21:47:27Z

This PR includes the following changes:

Moves commands and custom commands away from recipes.
Addresses issue detailed in bug: currentDir context is not shown in UI #2548
Removes SimpleChatRecipeAdapter
Removes executeRecipe from SimpleChatPanelProvider and ChatManager
Removes the custom-prompts recipe which was used for commands and custom commands
Runs executeCommand from top level in main.ts and turns it into a vscode command
Fixes issue where all context is showing up as via embeddings
Adds range to editor context for commands RE: bug: "from ${n} files" missing line counts #1984
Updates to execute all edit commands in CommandRunner so that we won't need to rely on sidebar view to execute fixup or custom commands recipes.

Test plan

All the current tests for chat, commands, and custom commands should still pass and work correctly.

Run the explain command
- from sidebar
- from left-click menu
- from chat
Run the doc command
- from sidebar
- from left-click menu
- from chat
Run a custom command
- from sidebar
- from left-click menu
- from chat
Run an edit command

Make sure context for each command is showing up correctly

Demo

Screen.Recording.2024-01-03.at.7.57.41.AM.mov

Before - context for test command are now showing up in context list

After

Context sources for commands + Context for text command are showing up correctly
Context for commands include range

dominiccooney · 2024-01-04T02:50:26Z

vscode/src/chat/chat-view/SimpleChatPanelProvider.ts

+            (await this.editor.getActiveTextEditorSmartSelection()) ||
+            this.editor.getActiveTextEditorSelectionOrVisibleContent()
+        if (!selection || isCodyIgnoredFile(vscode.Uri.file(selection.fileName))) {
+            return []


Why do we use getActiveTextEditorSelectionOrVisibleContent some places (above, for executeCommand) but this "smart" selection when the command has the 'selection' property? And generate an error about opening a file in one case but not another? And checks for isCodyIgnoredFile happen within one (this one) but not the other?

Can these be brought together, simplified, and handle things like Cody ignored files consistently?

Good questions! We check if it's an ignored file or not when we try to create context item from a selection object.

If a command requires user to have a current selection, we will try to use Smart Selection to auto select code for the user. We won't try to fall back to use visible context as selection for commands that require selection specifically.

If a command doesn't require a selection, we will try to get the visible context to use as selection context, like the explain or smell commands for example, unless the command.context.selection for the command is specified to be false.

Does this answer your questions? If yes, I will add these as docstring!

TL;DR

we check for isCodyIgnoredFile in ContextProvider when creating context item

We generate error about no file opened in webview provider where we handle command execution

dominiccooney · 2024-01-04T03:11:55Z

vscode/test/e2e/at-file-context-selecting.test.ts

@@ -89,8 +89,9 @@ test('@-file empty state', async ({ page, sidebar }) => {
        )
    ).toBeVisible()

+    // TODO bee fix


What's the plan for this? If it's a TODO to land, let's specify what and when.

Woops i forgot to delete this, will update. Thank you!

dominiccooney · 2024-01-04T03:13:50Z

It would be great if we added automated tests to cover the essential functionality not already covered by tests.

dominiccooney

Looks good. Please consider adding automated tests in a follow up change so we can keep broadening the contributor base.

abeatrix · 2024-01-04T16:53:41Z

Looks good. Please consider adding automated tests in a follow up change so we can keep broadening the contributor base.

Updated existing tests and added new tests for the new simple chat provider! Thanks for the review @dominiccooney!

This reverts commit 7ea2514.

The agent tests started failing after merging the PR #2542 when you re-record the HTTP requests. The tests were passing in replay mode for some reason, and we suspect it's due to a buggy merge in `recording.har`. When you `rm -rf agent/recordings` and record from scratch then the tests fail. To prevent this from happening again, we're planning to provide scripts to cleanly update the recordings. ## Test plan Green CI.

)" This reverts commit e4b60d8.

@Gedochao

…th new fixes (#2583) Close #2588 - Reverts #2575 with updated fixes from #2580 that I've confirmed with @Gedochao on call. - Merged main and updated the recording to confirm there are no errors caused by the changes in this branch in all covered tests: ![image](https://github.com/sourcegraph/cody/assets/68532117/e54d893a-0798-48af-a838-061056fbc889) - Also confirmed this change also passed the new rate limit test introduced in #2535 - Fixed upresponsive stop button when error is presented https://github.com/sourcegraph/cody/assets/68532117/6d9263b0-3e9c-419e-939d-20a2ea011824 ## Test Plan - run `pnpm run test:unit` to confirm all the tests are passing

This PR includes the following changes: - Moves commands and custom commands away from recipes. - Addresses issue detailed in #2548 - Removes `SimpleChatRecipeAdapter` - Removes `executeRecipe` from SimpleChatPanelProvider and ChatManager - Removes the `custom-prompts` recipe which was used for commands and custom commands - Runs `executeCommand` from top level in `main.ts` and turns it into a vscode command - Fixes issue where all context is showing up as `via embeddings` - Adds range to editor context for commands RE: #1984 - Updates to execute all edit commands in CommandRunner so that we won't need to rely on sidebar view to execute fixup or custom commands recipes.  All the current tests for chat, commands, and custom commands should still pass and work correctly. 1. Run the explain command - [ ] from sidebar - [ ] from left-click menu - [ ] from chat 2. Run the doc command - [ ] from sidebar - [ ] from left-click menu - [ ] from chat 3. Run a custom command - [ ] from sidebar - [ ] from left-click menu - [ ] from chat 4. Run an edit command - Make sure context for each command is showing up correctly https://github.com/sourcegraph/cody/assets/68532117/7e4d9118-9a0a-48cd-823a-af2646e52357 list ![image](https://github.com/sourcegraph/cody/assets/68532117/9471dcee-cdce-499a-8eae-caf7dd3ce607) - Context sources for commands + Context for text command are showing up correctly - Context for commands include range ![image](https://github.com/sourcegraph/cody/assets/68532117/c95b4407-98c4-47af-a288-76fce10a0d5e)

This PR addresses some code quality issues and complexity in the chat implementation. * #2542 introduced a separate code path for handling command-initiated chats. This introduced a flag that special-cased handling of commands in a number of places—in the view controller (SimpleChatPanelProvider), prompt constructor, and context fetcher. These flags make chat more difficult to reason about, requiring the dev to reason about the conditional flag at multiple places in the lifecycle of a chat request. Code changes * Instead of checking a flag to see if the user request was a command in `DefaultPrompter`, introduces a new `CommandPrompter` to customize prompt construction for commands * Breaks apart `ContextProvider` into separate functions, moved to a `context.ts` file. Enhanced context can be fetched by either prompter. * Simplifies the core code paths in `SimpleChatPanelProvider`. Now there are two top-level message handlers: `handleNewUserMessage` (for regular chat) and `handleCommand` (for commands). These then execute the necessary steps without entangling other methods that must then support both code paths via conditional. * Previously, we had the following mess: * `executeChat` -> `handleHumanMessageSubmitted` * `handleHumanMessageSubmitted` -> 'handleChatRequest` OR `handleCommands` * `handleCommands` -> `handleChatRequest` * `handleChatRequest` -> `generateAssistantResponse` Behavior changes * This change removes the already defunct `cody.experimental.chatPredictions` setting. * This change disables codebase-wide context fetching for commands. This field appears unused by first-party commands, and it's unclear whether it was actually being used for custom commands. Test changes: * Fixes some flakiness in macOS e2e tests

abeatrix added 11 commits January 2, 2024 13:46

intergrate command in simple chat

c9b4e61

fix test

ceab310

Merge branch 'main' into bee/execute-command-simple

5715bbd

use node

939920f

fix selection source

395678b

add editor source to context

5581e79

update renderContextItem

1de4be8

update tests

1e5ca67

remove SimpleChatRecipeAdapter and executeRecipe

5d935b8

Merge branch 'main' into bee/execute-command-simple

0fef0ee

remove commands recipe

deecab3

abeatrix changed the title ~~intergrate command in simple chat~~ refactor: move commands away from recipe Jan 3, 2024

update recording

cbfdf74

abeatrix marked this pull request as ready for review January 3, 2024 23:54

abeatrix requested review from beyang, umpox and a team January 3, 2024 23:54

dominiccooney requested changes Jan 4, 2024

View reviewed changes

dominiccooney approved these changes Jan 4, 2024

View reviewed changes

abeatrix added 5 commits January 4, 2024 07:14

add new tests for simple chat, fix at-file test

0b75a27

update recording

6195746

public handleHumanMessageSubmitted

b1c5def

update changelog

68b3f0f

simplify chat

a91c350

abeatrix added 4 commits January 4, 2024 10:44

clean up

be8528a

Merge branch 'main' into bee/execute-command-simple

b096722

clean up

cc22531

Merge branch 'main' into bee/execute-command-simple

ec799cc

abeatrix added 2 commits January 4, 2024 14:49

update changelog

a88bf2b

fix tests

20f2ca2

abeatrix merged commit 7ea2514 into main Jan 5, 2024
16 checks passed

abeatrix deleted the bee/execute-command-simple branch January 5, 2024 00:29

olafurpg added a commit that referenced this pull request Jan 5, 2024

Revert "refactor: move commands away from recipe (#2542)"

2571f9c

This reverts commit 7ea2514.

olafurpg mentioned this pull request Jan 5, 2024

Revert "refactor: move commands away from recipe (#2542)" #2575

Merged

abeatrix added a commit that referenced this pull request Jan 5, 2024

Revert "Revert "refactor: move commands away from recipe (#2542)" (#2575

6a633b0

)" This reverts commit e4b60d8.

abeatrix self-assigned this Jan 9, 2024

beyang mentioned this pull request Jan 22, 2024

Refactor chat #2848

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: move commands away from recipe #2542

refactor: move commands away from recipe #2542

abeatrix commented Jan 2, 2024 •

edited

dominiccooney Jan 4, 2024

abeatrix Jan 4, 2024

abeatrix Jan 4, 2024 •

edited

dominiccooney Jan 4, 2024

abeatrix Jan 4, 2024

dominiccooney commented Jan 4, 2024

dominiccooney left a comment

abeatrix commented Jan 4, 2024

refactor: move commands away from recipe #2542

refactor: move commands away from recipe #2542

Conversation

abeatrix commented Jan 2, 2024 • edited

Test plan

Demo

Before - context for test command are now showing up in context list

After

dominiccooney Jan 4, 2024

Choose a reason for hiding this comment

abeatrix Jan 4, 2024

Choose a reason for hiding this comment

abeatrix Jan 4, 2024 • edited

Choose a reason for hiding this comment

dominiccooney Jan 4, 2024

Choose a reason for hiding this comment

abeatrix Jan 4, 2024

Choose a reason for hiding this comment

dominiccooney commented Jan 4, 2024

dominiccooney left a comment

Choose a reason for hiding this comment

abeatrix commented Jan 4, 2024

abeatrix commented Jan 2, 2024 •

edited

abeatrix Jan 4, 2024 •

edited