Refactor chat #2848
Refactor chat #2848
Conversation
beyang
force-pushed
the
bl/chat-refactor
branch
from
137277f
to
9b4818b
Compare
beyang
force-pushed
the
bl/chat-refactor
branch
from
9b4818b
to
83ab28e
Compare
There was a problem hiding this comment.
Approving, this is an improvement. @philipp-spiess was pursuing similar breakout of context in #2804 so it is clearly time. Suggestions inline.
@abeatrix how well are commands covered by E2E tests?
I see the TODO about enhanced context, @beyang what's the bottom line on the change to context?
| @@ -118,11 +118,10 @@ export class ChatManager implements vscode.Disposable { | |||
| const requestID = uuid.v4() | |||
| telemetryService.log('CodyVSCodeExtension:chat-question:submitted', { requestID, ...args }) | |||
| const chatProvider = await this.getChatProvider() | |||
| await chatProvider.handleHumanMessageSubmitted(requestID, question, 'user', [], false) | |||
| await chatProvider.handleNewUserMessage(requestID, question, 'user-newchat', [], true) | |||
There was a problem hiding this comment.
This is why I'm not a fan of boolean parameters... They are mysterious at the callsite.
There was a problem hiding this comment.
Yeah, I agree. In this case, there is a semantic change—we now use enhanced context when starting a chat from the command palette.
| return | ||
| } | ||
|
|
||
| if (!this.editor.getActiveTextEditorSelectionOrVisibleContent()) { |
There was a problem hiding this comment.
It would be nice if the check for command.context.selection, etc., fetching that context, and the error reporting was all together instead of spread out like this.
| if (command.mode !== 'ask') { | ||
| return | ||
| } | ||
| // trigger the context progress indicator |
There was a problem hiding this comment.
Given we have had this in a couple of places with this comment, why not just add a method triggerContextProgressIndicator?
| if (isRateLimitError(error)) { | ||
| this.postError(error, 'transcript') | ||
| } else { | ||
| this.postError(isError(error) ? error : new Error(`Error generating assistant response: ${error}`)) | ||
| } |
There was a problem hiding this comment.
While you're cleaning things up, we can de-dup the postError here.
There was a problem hiding this comment.
Thanks. Left a comment in line.
Edit: for dom's question, the e2e test for the core command should cover if the command includes the selection in file, and the response in transcript should be covered by the integration tests in both vs code and agent.
Ill try to clean up the code in Command controller next week too when i work on removing slash commands from chat 😅
| return await vscode.commands.executeCommand('cody.action.chat', input, { | ||
| source: 'command', | ||
| }) | ||
| await vscode.commands.executeCommand('cody.action.commands.exec', `${selectedCommandID} input`) |
There was a problem hiding this comment.
| await vscode.commands.executeCommand('cody.action.commands.exec', `${selectedCommandID} input`) | |
| - await vscode.commands.executeCommand('cody.action.chat', input) |
When the selectedCommandID is equal to "/ask" , it means this is a chat question submitted from the command menu, so we can send process this as a chat question with "cody.action.chat".
There was a problem hiding this comment.
The issue with executing the command cody.action.chat is that we lose the context of the user selection, so the response ends up being very low quality.
| return await vscode.commands.executeCommand( | ||
| 'cody.action.commands.exec', | ||
| `${selectedCommandID} ${input}` | ||
| ) |
There was a problem hiding this comment.
| return await vscode.commands.executeCommand( | |
| 'cody.action.commands.exec', | |
| `${selectedCommandID} ${input}` | |
| ) | |
| return await vscode.commands.executeCommand( | |
| 'cody.chat.action', | |
| input | |
| ) |
Same as above
The only change to context should be:
|
Implements some follow-up tasks from #2848: * Move methods around in SimpleChatPanelProvider for greater clarity (not doing in this PR to avoid making the diff larger) * #2848 (comment) We establish strong naming conventions for methods and document invariants that should hold to prevent future quality regressions that may make the logic more difficult to reason about. ## Test plan There should be no behavior changes
This PR addresses some code quality issues and complexity in the chat implementation.
Code changes
DefaultPrompter, introduces a newCommandPrompterto customize prompt construction for commandsContextProviderinto separate functions, moved to acontext.tsfile. Enhanced context can be fetched by either prompter.SimpleChatPanelProvider. Now there are two top-level message handlers:handleNewUserMessage(for regular chat) andhandleCommand(for commands). These then execute the necessary steps without entangling other methods that must then support both code paths via conditional.executeChat->handleHumanMessageSubmittedhandleHumanMessageSubmitted-> 'handleChatRequestORhandleCommands`handleCommands->handleChatRequesthandleChatRequest->generateAssistantResponseBehavior changes
cody.experimental.chatPredictionssetting.Follow-up work: This PR also flags some outstanding bugs uncovered in the refactor:
Test plan