-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Move message queue to the extension host #7604
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
| } | ||
|
|
||
| async getStateToPostToWebview() { | ||
| async getStateToPostToWebview(): Promise<ExtensionState> { |
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 was surprised to see that we didn't have very strict type safety on getState and getStateToPostToWebview.
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.
Thank you for your contribution! I've reviewed the changes to move the message queue to the extension host. The implementation looks good overall, but I found some issues that need attention before merging.
| | "hasOpenedModeSelector" | ||
| | "version" | ||
| | "shouldShowAnnouncement" | ||
| | "hasSystemPromptOverride" |
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 think it's a bit strange that getState omits certain fields and relies on getStateToPostToWebview to fill in what's missing, but this will get us on a better path to strict type safety and we can clean this up later.
Co-authored-by: roomote[bot] <219738659+roomote[bot]@users.noreply.github.com>
Co-authored-by: roomote[bot] <219738659+roomote[bot]@users.noreply.github.com>
Co-authored-by: roomote[bot] <219738659+roomote[bot]@users.noreply.github.com>
daniel-lxs
left a comment
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.
Looks good, just noticed these lines
| } | ||
|
|
||
| public get queuedMessages(): QueuedMessage[] { | ||
| return this.messageQueueService.messages |
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.
Consider returning a shallow copy of the message queue (e.g. using slice()) in the getter to avoid external mutations of the internal state.
| return this.messageQueueService.messages | |
| return this.messageQueueService.messages.slice() |
queued-messages.mp4
This makes it easier to interface with the extension bridge.
Important
Move message queue to the extension host by introducing
MessageQueueServiceand updatingTaskandChatViewcomponents to handle queued messages.MessageQueueServiceinMessageQueueService.tsto manage message queuing.Taskclass inTask.tsto useMessageQueueServicefor handling user messages.ChatView.tsxto handle message queuing, including adding, removing, and editing queued messages.queueMessage,removeQueuedMessage, andeditQueuedMessageinWebviewMessage.ts.ExtensionMessage.tsandWebviewMessage.tsto includeQueuedMessagetype.ChatView.spec.tsxto mockQueuedMessagesand test message queuing behavior.package.metadata.jsonfrom1.66.0to1.67.0.This description was created by
for d4fe861. You can customize this summary. It will automatically update as commits are pushed.