-
Notifications
You must be signed in to change notification settings - Fork 69
fix: Reduce perceived latency of fsWrite. Show fsWrite errors in the UX #1351
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
82c0ce1
8551e32
0ca3578
7e79396
d95e35b
67818b8
31e3af1
ca85056
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,7 @@ | |
* Will be deleted or merged. | ||
*/ | ||
|
||
import * as path from 'path' | ||
Check warning on line 6 in server/aws-lsp-codewhisperer/src/language-server/agenticChat/agenticChatController.ts
|
||
import { | ||
ChatTriggerType, | ||
CodeWhispererStreamingServiceException, | ||
|
@@ -91,7 +91,7 @@ | |
ChatResultWithMetadata as AgenticChatResultWithMetadata, | ||
} from './agenticChatEventParser' | ||
import { ChatSessionService } from '../chat/chatSessionService' | ||
import { AgenticChatResultStream, ResultStreamWriter } from './agenticChatResultStream' | ||
import { AgenticChatResultStream, progressPrefix, ResultStreamWriter } from './agenticChatResultStream' | ||
import { executeToolMessage, toolErrorMessage, toolResultMessage } from './textFormatting' | ||
import { | ||
AdditionalContentEntryAddition, | ||
|
@@ -834,18 +834,15 @@ | |
if (!availableToolNames.includes(toolUse.name)) { | ||
throw new Error(`Tool ${toolUse.name} is not available in the current mode`) | ||
} | ||
|
||
// remove progress UI | ||
await chatResultStream.removeResultBlockAndUpdateUI(progressPrefix + toolUse.toolUseId) | ||
|
||
// fsRead and listDirectory write to an existing card and could show nothing in the current position | ||
if (!['fsWrite', 'fsRead', 'listDirectory'].includes(toolUse.name)) { | ||
await this.#showUndoAllIfRequired(chatResultStream, session) | ||
} | ||
const { explanation } = toolUse.input as unknown as ExplanatoryParams | ||
if (explanation) { | ||
await chatResultStream.writeResultBlock({ | ||
type: 'directive', | ||
messageId: toolUse.toolUseId + '_explanation', | ||
body: explanation, | ||
}) | ||
} | ||
Comment on lines
-841
to
-848
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. wouldn't this remove the explanation showing for other tools such as executeBash? 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. Explanation is now rendered as soon as they are streamed even in the incomplete json response for all tool use events, 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. Added another video to explain bash. 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 the UX is worse for executeBash now, can we do this only for fsWrite? |
||
|
||
switch (toolUse.name) { | ||
case 'fsRead': | ||
case 'listDirectory': | ||
|
@@ -1056,6 +1053,36 @@ | |
continue | ||
} | ||
} | ||
// display fs write failure status in the UX of that file card | ||
if (toolUse.name === 'fsWrite' && toolUse.toolUseId) { | ||
const existingCard = chatResultStream.getMessageBlockId(toolUse.toolUseId) | ||
const fsParam = toolUse.input as unknown as FsWriteParams | ||
const fileName = path.basename(fsParam.path) | ||
const errorResult = { | ||
type: 'tool', | ||
messageId: toolUse.toolUseId, | ||
header: { | ||
fileList: { | ||
filePaths: [fileName], | ||
details: { | ||
[fileName]: { | ||
description: fsParam.path, | ||
}, | ||
}, | ||
}, | ||
status: { | ||
status: 'error', | ||
icon: 'error', | ||
text: 'Error', | ||
}, | ||
}, | ||
} as ChatResult | ||
if (existingCard) { | ||
await chatResultStream.overwriteResultBlock(errorResult, existingCard) | ||
} else { | ||
await chatResultStream.writeResultBlock(errorResult) | ||
} | ||
} | ||
const errMsg = err instanceof Error ? err.message : 'unknown error' | ||
this.#log(`Error running tool ${toolUse.name}:`, errMsg) | ||
results.push({ | ||
|
@@ -2203,6 +2230,64 @@ | |
} | ||
} | ||
|
||
async #showToolUseIntermediateResult( | ||
data: AgenticChatResultWithMetadata, | ||
chatResultStream: AgenticChatResultStream, | ||
streamWriter: ResultStreamWriter | ||
) { | ||
// extract the key value from incomplete JSON response stream | ||
function extractKey(incompleteJson: string, key: string): string | undefined { | ||
const pattern = new RegExp(`"${key}":\\s*"([^"]*)"`, 'g') | ||
const match = pattern.exec(incompleteJson) | ||
return match?.[1] | ||
} | ||
const toolUses = Object.values(data.toolUses) | ||
for (const toolUse of toolUses) { | ||
if (toolUse.name === 'fsWrite' && typeof toolUse.input === 'string') { | ||
const filepath = extractKey(toolUse.input, 'path') | ||
const msgId = progressPrefix + toolUse.toolUseId | ||
// render fs write UI as soon as fs write starts | ||
if (filepath && !chatResultStream.hasMessage(msgId)) { | ||
const fileName = path.basename(filepath) | ||
await streamWriter.close() | ||
await chatResultStream.writeResultBlock({ | ||
type: 'tool', | ||
messageId: msgId, | ||
header: { | ||
fileList: { | ||
filePaths: [fileName], | ||
details: { | ||
[fileName]: { | ||
description: filepath, | ||
}, | ||
}, | ||
}, | ||
status: { | ||
status: 'info', | ||
icon: 'progress', | ||
text: '', | ||
}, | ||
}, | ||
}) | ||
} | ||
} | ||
|
||
// render the tool use explanatory as soon as this is received for all tool uses. | ||
if (typeof toolUse.input === 'string') { | ||
const explanation = extractKey(toolUse.input, 'explanation') | ||
const messageId = progressPrefix + toolUse.toolUseId + '_explanation' | ||
if (explanation && !chatResultStream.hasMessage(messageId)) { | ||
await streamWriter.close() | ||
await chatResultStream.writeResultBlock({ | ||
type: 'directive', | ||
messageId: messageId, | ||
body: explanation, | ||
}) | ||
} | ||
} | ||
} | ||
} | ||
|
||
async #processGenerateAssistantResponseResponse( | ||
response: GenerateAssistantResponseCommandOutput, | ||
metric: Metric<AddMessageEvent>, | ||
|
@@ -2251,6 +2336,7 @@ | |
toolUseStartTimes, | ||
toolUseLoadingTimeouts | ||
) | ||
await this.#showToolUseIntermediateResult(result.data, chatResultStream, streamWriter) | ||
} | ||
} | ||
await streamWriter.close() | ||
|
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.
wouldn't this break other tool?
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 only removes the messages that has the progressPrefix, which is the in progress rendered UX. When the streaming result is finalized, we no longer need these in progress ux.