-
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 1 commit
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 | ||||
|---|---|---|---|---|---|---|
|
|
@@ -13,6 +13,9 @@ import { UserWrittenCodeTracker } from '../../../../codewhisperer/tracker/userWr | |||||
|
|
||||||
| export class ChatSession { | ||||||
| private sessionId?: string | ||||||
| private _listOfReadFiles: string[] = [] | ||||||
|
||||||
| private _filePath: string | undefined | ||||||
| private _tempFilePath: string | undefined | ||||||
|
||||||
|
|
||||||
| contexts: Map<string, { first: number; second: number }[]> = new Map() | ||||||
| // TODO: doesn't handle the edge case when two files share the same relativePath string but from different root | ||||||
|
|
@@ -35,6 +38,28 @@ 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._listOfReadFiles | ||||||
|
||||||
| } | ||||||
| public get getFilePath(): string | undefined { | ||||||
|
||||||
| public get getFilePath(): string | undefined { | |
| public get filePath(): string | undefined { |
Outdated
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.
Why does this reset the list? Looks like it would behave unexpectedly if I just look at the name of the function and not the implementation
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.
If user tries to ask any other question in between generation or if user gets any error we need to reset to files read
To show proper UX of
2 files read, reading Sample.java
3 files read, reading foo.java
etc..
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.
Sure but that shouldn't happen here. Someone reading the code in the future is not going to know why it's being reset
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.
Let me write a separate method(clearListOfReadFiles()) to reset the files.

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.