Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 59 additions & 9 deletions packages/core/src/amazonq/webview/ui/apps/cwChatConnector.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand All @@ -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'
Expand All @@ -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 => {
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -235,6 +245,7 @@ export class Connector extends BaseConnector {

onCustomFormAction(
tabId: string,
messageId: string,
Copy link
Contributor

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 ''

Copy link
Contributor Author

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 onCustomFormAction not for onFileClick. Can you elaborate more on this?

Copy link
Contributor

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?

                    this.cwChatConnector.onCustomFormAction(tabId, messageId ?? '', action)

Copy link
Contributor Author

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 code

I tested passing undefined and got undesirable result. So, I am passing ''
I remember testing this for RIV for /test too but I will give a shot once again after UX integration.

action: {
id: string
text?: string | undefined
Expand All @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.
We can create a function and return answer but basically the functionality does not change.

Copy link
Contributor

@ctlai95 ctlai95 Mar 25, 2025

Choose a reason for hiding this comment

The 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

if (action.id === `open-settings`) {
can't we just add to that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 Confirm button!
Here we show a UX to disable the button once user click on this button.

}

onFileClick = (tabID: string, filePath: string, messageId?: string) => {
this.sendMessageToExtension({
command: 'file-click',
command: messageId === '' ? 'file-click' : 'open-diff',
Copy link
Contributor

@ctlai95 ctlai95 Mar 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels a bit hacky. Can there be a separate onOpenDiff handler?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There used to be onOpenDiff function previously but this got deprecated

@deprecated since version 4.6.3. Will be dropped after version 5.x.x. Use {@link onFileClick} instead

So Right now if user click on any file within fileTreeDiff Mynah sends event through onFileClick.

Copy link
Contributor

Choose a reason for hiding this comment

The 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 file-click and not open-diff

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, basically the question is, How can we differentiate the ContextList vs FileListTree ?
image

tabID,
messageId,
filePath,
Expand Down
3 changes: 2 additions & 1 deletion packages/core/src/amazonq/webview/ui/connector.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ export interface CWCChatItem extends ChatItem {
userIntent?: UserIntent
codeBlockLanguage?: string
contextList?: Context[]
title?: string
}

export interface Context {
Expand Down Expand Up @@ -711,7 +712,7 @@ export class Connector {
tabType: 'cwc',
})
} else {
this.cwChatConnector.onCustomFormAction(tabId, action)
this.cwChatConnector.onCustomFormAction(tabId, messageId ?? '', action)
}
break
case 'agentWalkthrough': {
Expand Down
3 changes: 2 additions & 1 deletion packages/core/src/amazonq/webview/ui/main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 &&
Expand All @@ -374,7 +375,7 @@ export const createMynahUI = (
fileList: {
fileTreeTitle: '',
filePaths: item.contextList.map((file) => file.relativeFilePath),
rootFolderTitle: 'Context',
rootFolderTitle: item.title,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe fallback to ?? 'Context'

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Context is not a generic text across Q so, I feel this should be either undefined or we can supply proper text as part of title which we do in our changes.

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, right now only Q chat uses Context or contextList in project falcon but going forward we need to pass title from the agent controller.ts instead of static text.

flatList: true,
collapsed: true,
hideFileCount: true,
Expand Down
4 changes: 4 additions & 0 deletions packages/core/src/codewhispererChat/app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import {
AcceptDiff,
QuickCommandGroupActionClick,
FileClick,
OpenDiff,
} from './controllers/chat/model'
import { EditorContextCommand, registerCommands } from './commands/registerCommands'
import { ContextSelectedMessage, CustomFormActionMessage } from './view/connector/connector'
Expand All @@ -41,6 +42,7 @@ export function init(appContext: AmazonQAppInitContext) {
processInsertCodeAtCursorPosition: new EventEmitter<InsertCodeAtCursorPosition>(),
processAcceptDiff: new EventEmitter<AcceptDiff>(),
processViewDiff: new EventEmitter<ViewDiff>(),
processOpenDiff: new EventEmitter<OpenDiff>(),
processCopyCodeToClipboard: new EventEmitter<CopyCodeToClipboard>(),
processContextMenuCommand: new EventEmitter<EditorContextCommand>(),
processTriggerTabIDReceived: new EventEmitter<TriggerTabIDReceived>(),
Expand Down Expand Up @@ -76,6 +78,7 @@ export function init(appContext: AmazonQAppInitContext) {
),
processAcceptDiff: new MessageListener<AcceptDiff>(cwChatControllerEventEmitters.processAcceptDiff),
processViewDiff: new MessageListener<ViewDiff>(cwChatControllerEventEmitters.processViewDiff),
processOpenDiff: new MessageListener<OpenDiff>(cwChatControllerEventEmitters.processOpenDiff),
processCopyCodeToClipboard: new MessageListener<CopyCodeToClipboard>(
cwChatControllerEventEmitters.processCopyCodeToClipboard
),
Expand Down Expand Up @@ -137,6 +140,7 @@ export function init(appContext: AmazonQAppInitContext) {
),
processAcceptDiff: new MessagePublisher<AcceptDiff>(cwChatControllerEventEmitters.processAcceptDiff),
processViewDiff: new MessagePublisher<ViewDiff>(cwChatControllerEventEmitters.processViewDiff),
processOpenDiff: new MessagePublisher<OpenDiff>(cwChatControllerEventEmitters.processOpenDiff),
processCopyCodeToClipboard: new MessagePublisher<CopyCodeToClipboard>(
cwChatControllerEventEmitters.processCopyCodeToClipboard
),
Expand Down
29 changes: 29 additions & 0 deletions packages/core/src/codewhispererChat/clients/chat/v0/chat.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,14 @@ import { UserWrittenCodeTracker } from '../../../../codewhisperer/tracker/userWr

export class ChatSession {
private sessionId?: string
/**
* _listOfReadFiles = list of files read from the project to gather context before generating response.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do you need to pass this around?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To count # of files read as per UX requirement.

* _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 _listOfReadFiles: string[] = []
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

list is redundant

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Required for the Reading files UX which still a TODO. I can revisit on this once we have final UX into place.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant that the variable name can just be _readFiles

private _filePath: string | undefined
private _tempFilePath: string | undefined
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check the naming or add some documentation, it's very unclear what these are used for

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will add some comments on top of this variable. There might be some duplication due to lot of dependencies on our PR's.
I will add a TODO to revisit and remove necessary variables if required.

private _toolUse: ToolUse | undefined

contexts: Map<string, { first: number; second: number }[]> = new Map()
Expand Down Expand Up @@ -48,6 +56,27 @@ export class ChatSession {
public setSessionID(id?: string) {
this.sessionId = id
}
public get listOfReadFiles(): string[] {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public get listOfReadFiles(): string[] {
public get readFiles(): string[] {

return this._listOfReadFiles
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I think we can get this information from chatHistory?

Copy link
Contributor Author

@laileni-aws laileni-aws Mar 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please can you point me the code where this was implemented? I checked the latest PR and looks like its still TODO

}
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._listOfReadFiles.push(filePath)
}
public clearListOfReadFiles() {
this._listOfReadFiles = []
}
async chatIam(chatRequest: SendMessageRequest): Promise<SendMessageCommandOutput> {
const client = await createQDeveloperStreamingClient()

Expand Down
55 changes: 55 additions & 0 deletions packages/core/src/codewhispererChat/controllers/chat/controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import {
DocumentReference,
FileClick,
RelevantTextDocumentAddition,
OpenDiff,
} from './model'
import {
AppToWebViewMessageDispatcher,
Expand Down Expand Up @@ -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 {
Expand All @@ -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>
Expand All @@ -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>
Expand Down Expand Up @@ -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)
})
Expand Down Expand Up @@ -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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this notify the user?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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)
}
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,10 @@ export class Messenger {
userIntent: undefined,
codeBlockLanguage: undefined,
contextList: mergedRelevantDocuments,
title: 'Context',
buttons: undefined,
fileList: undefined,
canBeVoted: false,
},
tabID
)
Expand Down Expand Up @@ -486,6 +490,7 @@ export class Messenger {
userIntent: undefined,
codeBlockLanguage: undefined,
contextList: undefined,
title: undefined,
},
tabID
)
Expand Down
8 changes: 8 additions & 0 deletions packages/core/src/codewhispererChat/controllers/chat/model.ts
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,14 @@ export interface ViewDiff {
totalCodeBlocks?: number
}

export interface OpenDiff {
command: string | undefined
tabID: string
messageId: string
filePath: string
referenceTrackerInformation?: CodeReference[]
}

export type ChatPromptCommandType =
| 'help'
| 'clear'
Expand Down
Loading
Loading