-
Notifications
You must be signed in to change notification settings - Fork 33
feat: extended protocol for conversation-based agents experience support #368
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
Conversation
f6d6870
to
46f881c
Compare
@@ -81,3 +90,9 @@ export const followUpClickNotificationType = new ProtocolNotificationType<Follow | |||
export const openTabRequestType = new ProtocolRequestType<OpenTabParams, OpenTabResult, never, void, void>( | |||
OPEN_TAB_REQUEST_METHOD | |||
) | |||
export const chatUpdateNotificationType = new ProtocolNotificationType<ChatUpdateParams, void>( |
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.
Does this update existing chat messages in the current tab or add new ones?
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.
Add and update - depends on messageId
provided.
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.
Ok! No deletion.
runtimes/runtimes/base-runtime.ts
Outdated
@@ -122,6 +127,7 @@ export const baseRuntime = (connections: { reader: MessageReader; writer: Messag | |||
onChatPrompt: handler => lspConnection.onRequest(chatRequestType.method, handler), | |||
onEndChat: handler => lspConnection.onRequest(endChatRequestType.method, handler), | |||
onQuickAction: handler => lspConnection.onRequest(quickActionRequestType.method, handler), | |||
onQuickActionTrigger: handler => lspConnection.onNotification(quickActionNotificationType.method, handler), |
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.
There is a request onQuickAction
, and a notification onQuickActionTrigger
. Why we need both is a bit unclear.
And the type of onQuickActionTrigger
is quickActionNotification
.
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.
Same as above.
SelectWorkspaceItemResult, | ||
} from './lsp' | ||
|
||
export const selectWorkspaceItemRequestType = new ProtocolRequestType< |
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.
What;s the difference between this and showDocument
?
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.
The intent and protocol is different. We need to get selected items back in server.
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.
FYI: I think we usually run into issues when we use ProtocolRequestType and therefore usually use the AutoParameterStructuresProtocolRequestType
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.
What kind of issues?
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.
When would the client send this? What is the use case?
runtimes/runtimes/base-runtime.ts
Outdated
@@ -134,6 +140,8 @@ export const baseRuntime = (connections: { reader: MessageReader; writer: Messag | |||
onSourceLinkClick: handler => lspConnection.onNotification(sourceLinkClickNotificationType.method, handler), | |||
onFollowUpClicked: handler => lspConnection.onNotification(followUpClickNotificationType.method, handler), | |||
openTab: params => lspConnection.sendRequest(openTabRequestType.method, params), | |||
chatUpdate: params => lspConnection.sendNotification(chatUpdateNotificationType.method, params), |
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.
The wording of this seems to be reversed compared to the others:
onLinkClick
openFileDiff
showMessage
openTab
chatUpdate
should that be updateChat
? or sendChatUpdate
?
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.
I agree with Kamiel, updateChat
and sendChatUpdate
feel easier to understand
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.
renamed
@@ -120,14 +132,18 @@ export interface QuickActions { | |||
quickActionsCommandGroups: QuickActionCommandGroup[] | |||
} | |||
|
|||
export interface TabData { | |||
placeholderText?: string | |||
messages: ChatMessage[] |
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.
New messages? Replace matched messages? Replace entire tab?
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 is an update with new or updated messages - depends on messageId
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 that insight/info be a docstring comment on the field here (or somewhere)?
@@ -0,0 +1,24 @@ | |||
import { URI, TextDocument } from './lsp' | |||
|
|||
export const SELECT_WORKSPACE_ITEM_REQUEST_METHOD = 'aws/selectWorkspaceItem' |
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.
Is this coming from the client or the server? In what situation will this be used?
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.
Server to client, to request customer request to select folder if workspace folder is too big.
items: WorkspaceItem[] | ||
} | ||
|
||
export interface OpenFileDiffParams { |
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.
In this case originalFileUri
is the initial version of the file, and isDeleted
specificies if the original file was deleted and fileContent
contains the new/updated file contents, is this understanding correct?
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.
yes
@@ -319,6 +321,9 @@ export const standalone = (props: RuntimeProps) => { | |||
onDidDeleteFiles: params => lspConnection.workspace.onDidDeleteFiles(params), | |||
onDidRenameFiles: params => lspConnection.workspace.onDidRenameFiles(params), | |||
onUpdateConfiguration: lspServer.setUpdateConfigurationHandler, | |||
selectWorkspaceItem: params => | |||
lspConnection.sendRequest(selectWorkspaceItemRequestType.method, params), |
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.
nit: Do we have to specify selectWorkspaceItemRequestType.method
? I remember if we just passed selectWorkspaceItemRequestType
it also worked and I remember when you pass the .method
which is a string, you lose some benefits that come with typescript
Same applies to openFileDiff
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.
I've seen different versions used throughout the codebase. If you know what it impacts exactly, I can change.
runtimes/server-interface/lsp.ts
Outdated
selectWorkspaceItem: ( | ||
handler: RequestHandler< | ||
SelectWorkspaceItemParams, | ||
SelectWorkspaceItemResult | undefined | null, |
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.
Isn't this second field for partial result? Does this mean select workspace item will support partial results?
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.
good point
types/chat.ts
Outdated
data?: TabData | ||
} | ||
|
||
export type FileAction = 'accept-change' | 'reject-change' | string |
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.
what is the purpose of string
here? Is it for future expansion?
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.
I'll remove it - whatever is in the string needs to be anyway supported by mynah-ui so better to keep it explicit.
export interface OpenTabResult extends TabEventParams {} | ||
|
||
export interface TabState { |
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.
what does it mean for a tab to be in progress and cancellable?
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.
"cancellable" - practically means stop button will be shown
"inProgress" - it's being worked on by server, so no input should be accepted and progress bar shown
…t and options for open tab
a61df5f
to
e7e4a9f
Compare
@@ -136,6 +139,10 @@ export type Lsp = { | |||
onDidDeleteFiles: (handler: NotificationHandler<DeleteFilesParams>) => void | |||
onDidRenameFiles: (handler: NotificationHandler<RenameFilesParams>) => void | |||
onUpdateConfiguration: (handler: RequestHandler<UpdateConfigurationParams, void, void>) => void | |||
selectWorkspaceItem: ( | |||
handler: RequestHandler<SelectWorkspaceItemParams, SelectWorkspaceItemResult | undefined | null, void> |
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 do we allow for undefined or null as a result?
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.
Customer could click cancel in the dialog popup, something needs to be returned in this case.
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.
What operations trigger this dialog popup? Is there another (existing or new) request that this relates 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.
Followup action called "Choose another folder in your workspace". This is needed to request customer select a folder for context.
|
||
| Description | Method | Params | Method type | Response Type | | ||
| ------------------------------------ | --------------------------------- | ------------------------------ | ------------------------------------------------------------------------------------------------------------------------------- | --------------- | | ||
| Request to select workspace item (folder, file) with the selected items returned | `aws/selectWorkspaceItem` | `SelectWorkspaceItemParams` | [Request](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#requestMessage) Server to Client | `SelectWorkspaceItemResult` | |
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.
I don't understand what this means. Does this return the file contents? Show it on screen?
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 returns URI to selected folder/file back to LSP server. I do not think we need to return any contents as LSP server has access to contents if needed. The purpose of this method is to give customer possibility to select.
|
||
export interface TabState { | ||
inProgress?: boolean | ||
cancellable?: boolean |
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.
/** Server is working, should show progress bar, should not accept input. */
inProgress?: boolean
/** Should show stop button. */
cancellable?: boolean
Problem
We need to extend chat protocol to support agent-specific flow.
Solution
Protocol and server interface were extended to support:
'aws/chat/sendChatUpdate'
- for pushing async updates from server to client'aws/chat/fileClick'
- for opening file or executing related file action, if provided'aws/selectWorkspaceItem'
- to give possibility to prompt customer select another workspace folder in IDE if current is too big'aws/openFileDiff'
- to trigger open file differences in editorChatOptions
QuickActionCommand
were extended:disabled
was removed - disabling quick actions within quick action tabs needs to be re-modelled. Current approach will not work as chat options from different servers are merged in Flare runtimes. No real client is using this field now.OpenTabParams
params were extended to give possibility to configure contents and state of the tab if a new tab is requested.Other minor changes:
fileList
andprompts
supported in chat message responses.placeholderText
can be set.License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.