-
Notifications
You must be signed in to change notification settings - Fork 751
feat(amazonq): Adding openDiff and acceptDiff UX to the agentic chat. #6846
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
Changes from all commits
8d6467b
20b166e
f2d1467
8822a39
02478e3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -3,7 +3,14 @@ | |||
| * SPDX-License-Identifier: Apache-2.0 | ||||
| */ | ||||
|
|
||||
| import { ChatItemButton, ChatItemFormItem, ChatItemType, MynahUIDataModel, QuickActionCommand } from '@aws/mynah-ui' | ||||
| import { | ||||
| ChatItem, | ||||
| ChatItemButton, | ||||
| ChatItemFormItem, | ||||
| ChatItemType, | ||||
| MynahUIDataModel, | ||||
| QuickActionCommand, | ||||
| } from '@aws/mynah-ui' | ||||
| import { TabType } from '../storages/tabsStorage' | ||||
| import { CWCChatItem } from '../connector' | ||||
| import { BaseConnector, BaseConnectorProps } from './baseConnector' | ||||
|
|
@@ -18,12 +25,14 @@ export interface ConnectorProps extends BaseConnectorProps { | |||
| title?: string, | ||||
| description?: string | ||||
| ) => void | ||||
| onChatAnswerUpdated?: (tabID: string, message: ChatItem) => void | ||||
| } | ||||
|
|
||||
| export class Connector extends BaseConnector { | ||||
| private readonly onCWCContextCommandMessage | ||||
| private readonly onContextCommandDataReceived | ||||
| private readonly onShowCustomForm | ||||
| private readonly onChatAnswerUpdated | ||||
|
|
||||
| override getTabType(): TabType { | ||||
| return 'cwc' | ||||
|
|
@@ -34,6 +43,7 @@ export class Connector extends BaseConnector { | |||
| this.onCWCContextCommandMessage = props.onCWCContextCommandMessage | ||||
| this.onContextCommandDataReceived = props.onContextCommandDataReceived | ||||
| this.onShowCustomForm = props.onShowCustomForm | ||||
| this.onChatAnswerUpdated = props.onChatAnswerUpdated | ||||
| } | ||||
|
|
||||
| onSourceLinkClick = (tabID: string, messageId: string, link: string): void => { | ||||
|
|
@@ -91,16 +101,14 @@ export class Connector extends BaseConnector { | |||
| messageId: messageData.messageID ?? messageData.triggerID, | ||||
| body: messageData.message, | ||||
| followUp: followUps, | ||||
| canBeVoted: true, | ||||
| canBeVoted: messageData.canBeVoted ?? false, | ||||
| codeReference: messageData.codeReference, | ||||
| userIntent: messageData.userIntent, | ||||
| codeBlockLanguage: messageData.codeBlockLanguage, | ||||
| contextList: messageData.contextList, | ||||
| } | ||||
|
|
||||
| // If it is not there we will not set it | ||||
| if (messageData.messageType === 'answer-part' || messageData.messageType === 'answer') { | ||||
| answer.canBeVoted = true | ||||
| title: messageData.title, | ||||
| buttons: messageData.buttons ?? undefined, | ||||
| fileList: messageData.fileList ?? undefined, | ||||
| } | ||||
|
|
||||
| if (messageData.relatedSuggestions !== undefined) { | ||||
|
|
@@ -137,6 +145,8 @@ export class Connector extends BaseConnector { | |||
| options: messageData.followUps, | ||||
| } | ||||
| : undefined, | ||||
| buttons: messageData.buttons ?? undefined, | ||||
| canBeVoted: messageData.canBeVoted ?? false, | ||||
| } | ||||
| this.onChatAnswerReceived(messageData.tabID, answer, messageData) | ||||
|
|
||||
|
|
@@ -204,7 +214,7 @@ export class Connector extends BaseConnector { | |||
| } | ||||
|
|
||||
| if (messageData.type === 'customFormActionMessage') { | ||||
| this.onCustomFormAction(messageData.tabID, messageData.action) | ||||
| this.onCustomFormAction(messageData.tabID, messageData.messageId, messageData.action) | ||||
| return | ||||
| } | ||||
| // For other message types, call the base class handleMessageReceive | ||||
|
|
@@ -235,6 +245,7 @@ export class Connector extends BaseConnector { | |||
|
|
||||
| onCustomFormAction( | ||||
| tabId: string, | ||||
| messageId: string, | ||||
| action: { | ||||
| id: string | ||||
| text?: string | undefined | ||||
|
|
@@ -248,14 +259,53 @@ export class Connector extends BaseConnector { | |||
| this.sendMessageToExtension({ | ||||
| command: 'form-action-click', | ||||
| action: action, | ||||
| formSelectedValues: action.formItemValues, | ||||
| tabType: this.getTabType(), | ||||
| tabID: tabId, | ||||
| }) | ||||
|
|
||||
| if (!this.onChatAnswerUpdated || !['accept-code-diff', 'reject-code-diff'].includes(action.id)) { | ||||
| return | ||||
| } | ||||
| const answer: ChatItem = { | ||||
| type: ChatItemType.ANSWER, | ||||
| messageId: messageId, | ||||
| buttons: [], | ||||
| } | ||||
| switch (action.id) { | ||||
| case 'accept-code-diff': | ||||
| answer.buttons = [ | ||||
| { | ||||
| keepCardAfterClick: true, | ||||
| text: 'Accepted code', | ||||
| id: 'accepted-code-diff', | ||||
| status: 'success', | ||||
| position: 'outside', | ||||
| disabled: true, | ||||
| }, | ||||
| ] | ||||
| break | ||||
| case 'reject-code-diff': | ||||
| answer.buttons = [ | ||||
| { | ||||
| keepCardAfterClick: true, | ||||
| text: 'Rejected code', | ||||
| id: 'rejected-code-diff', | ||||
| status: 'error', | ||||
| position: 'outside', | ||||
| disabled: true, | ||||
| }, | ||||
| ] | ||||
| break | ||||
| default: | ||||
| break | ||||
| } | ||||
| this.onChatAnswerUpdated(tabId, answer) | ||||
|
Comment on lines
+267
to
+303
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I feel like this shouldn't live in this function. Is there somewhere else we can move this to?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is existing way how the buttons work/setup in both VSC and JB.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But this logic is specific to diffs which I don't think belongs here. Why not create a separate diff clicked handler? I see we already have some custom logic in
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This logic is not specific to diff's we can extend this to feedback button and even for |
||||
| } | ||||
|
|
||||
| onFileClick = (tabID: string, filePath: string, messageId?: string) => { | ||||
| this.sendMessageToExtension({ | ||||
| command: 'file-click', | ||||
| command: messageId === '' ? 'file-click' : 'open-diff', | ||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This feels a bit hacky. Can there be a separate onOpenDiff handler?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There used to be onOpenDiff function previously but this got deprecated
So Right now if user click on any file within fileTreeDiff Mynah sends event through onFileClick.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we need to think about this more. It's not clear to me why if messageId is empty that it should be
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||
| tabID, | ||||
| messageId, | ||||
| filePath, | ||||
|
|
||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -352,6 +352,7 @@ export const createMynahUI = ( | |
| ...(item.followUp !== undefined ? { followUp: item.followUp } : {}), | ||
| ...(item.fileList !== undefined ? { fileList: item.fileList } : {}), | ||
| ...(item.header !== undefined ? { header: item.header } : { header: undefined }), | ||
| ...(item.buttons !== undefined ? { buttons: item.buttons } : { buttons: undefined }), | ||
| }) | ||
| if ( | ||
| item.messageId !== undefined && | ||
|
|
@@ -374,7 +375,7 @@ export const createMynahUI = ( | |
| fileList: { | ||
| fileTreeTitle: '', | ||
| filePaths: item.contextList.map((file) => file.relativeFilePath), | ||
| rootFolderTitle: 'Context', | ||
| rootFolderTitle: item.title, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe fallback to
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Got it, the reason I'm suggesting this is because this code is shared with all agents. So unless we are sure that we always pass "Context" in the title for all messages everywhere, there might be a UI regression.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Got it, right now only Q chat uses |
||
| flatList: true, | ||
| collapsed: true, | ||
| hideFileCount: true, | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -17,6 +17,14 @@ import { UserWrittenCodeTracker } from '../../../../codewhisperer/tracker/userWr | |||||
|
|
||||||
| export class ChatSession { | ||||||
| private sessionId?: string | ||||||
| /** | ||||||
| * _readFiles = list of files read from the project to gather context before generating response. | ||||||
| * _filePath = The path helps the system locate exactly where to make the necessary changes in the project structure | ||||||
| * _tempFilePath = Used to show the code diff view in the editor including LLM changes. | ||||||
| */ | ||||||
| private _readFiles: string[] = [] | ||||||
| private _filePath: string | undefined | ||||||
| private _tempFilePath: string | undefined | ||||||
| private _toolUse: ToolUse | undefined | ||||||
|
|
||||||
| contexts: Map<string, { first: number; second: number }[]> = new Map() | ||||||
|
|
@@ -48,6 +56,27 @@ export class ChatSession { | |||||
| public setSessionID(id?: string) { | ||||||
| this.sessionId = id | ||||||
| } | ||||||
| public get listOfReadFiles(): string[] { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| return this._readFiles | ||||||
| } | ||||||
| public get filePath(): string | undefined { | ||||||
| return this._filePath | ||||||
| } | ||||||
| public get tempFilePath(): string | undefined { | ||||||
| return this._tempFilePath | ||||||
| } | ||||||
| public setFilePath(filePath: string | undefined) { | ||||||
| this._filePath = filePath | ||||||
| } | ||||||
| public setTempFilePath(tempFilePath: string | undefined) { | ||||||
| this._tempFilePath = tempFilePath | ||||||
| } | ||||||
| public pushToListOfReadFiles(filePath: string) { | ||||||
| this._readFiles.push(filePath) | ||||||
| } | ||||||
| public clearListOfReadFiles() { | ||||||
| this._readFiles = [] | ||||||
| } | ||||||
| async chatIam(chatRequest: SendMessageRequest): Promise<SendMessageCommandOutput> { | ||||||
| const client = await createQDeveloperStreamingClient() | ||||||
|
|
||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,6 +32,7 @@ import { | |
| DocumentReference, | ||
| FileClick, | ||
| RelevantTextDocumentAddition, | ||
| OpenDiff, | ||
| } from './model' | ||
| import { | ||
| AppToWebViewMessageDispatcher, | ||
|
|
@@ -81,6 +82,7 @@ import { | |
| } from '../../constants' | ||
| import { ChatSession } from '../../clients/chat/v0/chat' | ||
| import { ChatHistoryManager } from '../../storages/chatHistory' | ||
| import { amazonQTabSuffix } from '../../../shared/constants' | ||
| import { FsRead, FsReadParams } from '../../tools/fsRead' | ||
|
|
||
| export interface ChatControllerMessagePublishers { | ||
|
|
@@ -91,6 +93,7 @@ export interface ChatControllerMessagePublishers { | |
| readonly processInsertCodeAtCursorPosition: MessagePublisher<InsertCodeAtCursorPosition> | ||
| readonly processAcceptDiff: MessagePublisher<AcceptDiff> | ||
| readonly processViewDiff: MessagePublisher<ViewDiff> | ||
| readonly processOpenDiff: MessagePublisher<OpenDiff> | ||
| readonly processCopyCodeToClipboard: MessagePublisher<CopyCodeToClipboard> | ||
| readonly processContextMenuCommand: MessagePublisher<EditorContextCommand> | ||
| readonly processTriggerTabIDReceived: MessagePublisher<TriggerTabIDReceived> | ||
|
|
@@ -116,6 +119,7 @@ export interface ChatControllerMessageListeners { | |
| readonly processInsertCodeAtCursorPosition: MessageListener<InsertCodeAtCursorPosition> | ||
| readonly processAcceptDiff: MessageListener<AcceptDiff> | ||
| readonly processViewDiff: MessageListener<ViewDiff> | ||
| readonly processOpenDiff: MessageListener<OpenDiff> | ||
| readonly processCopyCodeToClipboard: MessageListener<CopyCodeToClipboard> | ||
| readonly processContextMenuCommand: MessageListener<EditorContextCommand> | ||
| readonly processTriggerTabIDReceived: MessageListener<TriggerTabIDReceived> | ||
|
|
@@ -214,6 +218,10 @@ export class ChatController { | |
| return this.processViewDiff(data) | ||
| }) | ||
|
|
||
| this.chatControllerMessageListeners.processOpenDiff.onMessage((data) => { | ||
| return this.processOpenDiff(data) | ||
| }) | ||
|
|
||
| this.chatControllerMessageListeners.processCopyCodeToClipboard.onMessage((data) => { | ||
| return this.processCopyCodeToClipboard(data) | ||
| }) | ||
|
|
@@ -389,6 +397,45 @@ export class ChatController { | |
| }) | ||
| } | ||
|
|
||
| private async processOpenDiff(message: OpenDiff) { | ||
| const session = this.sessionStorage.getSession(message.tabID) | ||
| const filePath = session.filePath ?? message.filePath | ||
| const fileExists = await fs.existsFile(filePath) | ||
| // Check if fileExists=false, If yes, return instead of showing broken diff experience. | ||
| if (!session.tempFilePath) { | ||
| return | ||
| } | ||
|
Comment on lines
+403
to
+407
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. does this notify the user?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No It wont notify the user for now, but good to have a Information notification message. I will add this to the TODO. |
||
| const leftUri = fileExists ? vscode.Uri.file(filePath) : vscode.Uri.from({ scheme: 'untitled' }) | ||
| const rightUri = vscode.Uri.file(session.tempFilePath ?? filePath) | ||
| const fileName = path.basename(filePath) | ||
| await vscode.commands.executeCommand('vscode.diff', leftUri, rightUri, `${fileName} ${amazonQTabSuffix}`) | ||
| } | ||
|
|
||
| private async processAcceptCodeDiff(message: CustomFormActionMessage) { | ||
| const session = this.sessionStorage.getSession(message.tabID ?? '') | ||
| const filePath = session.filePath ?? '' | ||
| const fileExists = await fs.existsFile(filePath) | ||
| const tempFilePath = session.tempFilePath | ||
| const tempFileExists = await fs.existsFile(tempFilePath ?? '') | ||
| if (fileExists && tempFileExists) { | ||
| const fileContent = await fs.readFileText(filePath) | ||
| const tempFileContent = await fs.readFileText(tempFilePath ?? '') | ||
| if (fileContent !== tempFileContent) { | ||
| await fs.writeFile(filePath, tempFileContent) | ||
| } | ||
| await fs.delete(tempFilePath ?? '') | ||
| await vscode.commands.executeCommand('vscode.open', vscode.Uri.file(filePath)) | ||
| } else if (!fileExists && tempFileExists) { | ||
| const fileContent = await fs.readFileText(tempFilePath ?? '') | ||
| await fs.writeFile(filePath, fileContent) | ||
| await fs.delete(tempFilePath ?? '') | ||
| await vscode.commands.executeCommand('vscode.open', vscode.Uri.file(filePath)) | ||
| } | ||
| // Reset the filePaths to undefined | ||
| session.setFilePath(undefined) | ||
| session.setTempFilePath(undefined) | ||
| } | ||
|
|
||
| private async processCopyCodeToClipboard(message: CopyCodeToClipboard) { | ||
| this.telemetryHelper.recordInteractWithMessage(message) | ||
| } | ||
|
|
@@ -578,6 +625,12 @@ export class ChatController { | |
| const newFileDoc = await vscode.workspace.openTextDocument(newFilePath) | ||
| await vscode.window.showTextDocument(newFileDoc) | ||
| telemetry.ui_click.emit({ elementId: 'amazonq_createSavedPrompt' }) | ||
| } else if (message.action.id === 'accept-code-diff') { | ||
| await this.processAcceptCodeDiff(message) | ||
| } else if (message.action.id === 'reject-code-diff') { | ||
| // Reset the filePaths to undefined | ||
| this.sessionStorage.getSession(message.tabID ?? '').setFilePath(undefined) | ||
| this.sessionStorage.getSession(message.tabID ?? '').setTempFilePath(undefined) | ||
| } else if (message.action.id === 'confirm-tool-use') { | ||
| await this.processToolUseMessage(message) | ||
| } | ||
|
|
@@ -936,6 +989,8 @@ export class ChatController { | |
| } | ||
|
|
||
| private async processPromptMessageAsNewThread(message: PromptMessage) { | ||
| const session = this.sessionStorage.getSession(message.tabID) | ||
| session.clearListOfReadFiles() | ||
| this.editorContextExtractor | ||
| .extractContextForTrigger('ChatMessage') | ||
| .then(async (context) => { | ||
|
|
||

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it be
string | undefined? Then you don't need to fallback to''There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This wont be undefined and this is for
onCustomFormActionnot foronFileClick. Can you elaborate more on this?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I didn't understand, aren't you passing in an empty string in connector.ts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohh I got it,
I am passing
''as part of messageId because of this codeI tested passing undefined and got undesirable result. So, I am passing
''I remember testing this for RIV for
/testtoo but I will give a shot once again after UX integration.