Skip to content

Conversation

@nkomonen-amazon
Copy link
Contributor

@nkomonen-amazon nkomonen-amazon commented Feb 7, 2025

Problem:

When a user starts up VSC, hasn't opened Q chat yet, then highlights text and does a right click option like "send to prompt" THEN the whole prompt fails to make it to chat.

This is because there is a race condition between the message being sent to the webview, and the webview completed loading and ready to recieve messages.

Solution:

In the appToMessage publisher, verify that the chat is ready to recieve messages based on if the webview sent the chat ui ready message. Otherwise wait until we get it, and then send the message.

Demo

Before

Before.mov

After 

After.mov

  • Treat all work as PUBLIC. Private feature/x branches will not be squash-merged at release time.
  • Your code changes must meet the guidelines in CONTRIBUTING.md.
  • License: I confirm that my contribution is made under the terms of the Apache 2.0 license.

@github-actions
Copy link

github-actions bot commented Feb 7, 2025

  • This pull request modifies code in src/* but no tests were added/updated.
    • Confirm whether tests should be added or ensure the PR description explains why tests are not required.
  • This pull request implements a feat or fix, so it must include a changelog entry (unless the fix is for an unreleased feature). Review the changelog guidelines.
    • Note: beta or "experiment" features that have active users should announce fixes in the changelog.
    • If this is not a feature or fix, use an appropriate type from the title guidelines. For example, telemetry-only changes should use the telemetry type.

Copy link
Contributor

@jpinkney-aws jpinkney-aws left a comment

Choose a reason for hiding this comment

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

This PR is more or less what I had in mind to solve the problem!

) {
super(eventEmitter)
}
public override publish(event: T): void {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to encapsulate the logic for isUiReady in this class somehow? Technically, it seems like this class is meant to hold requests until the UI is ready so keeping the uiReady logic close might make more sense

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah this makes sense, updated

return
}
// Otherwise wait, since it is probably still loading
void waitUntil(
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you experiment with trying to use a buffer that keeps filling up until the chat ui is ready? Then you can just send an event when the chat UI is ready that publishes everthing in the buffer.

I.e

if (this.isChatUIReady()) {
    super.publish(event)
    return
}
buffer.push(event)

and then you'd do something like this

onChatReady(() => {
    for (const event of this.buffer) {
        super.publish(event)
    }
})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

great point, updated

@nkomonen-amazon nkomonen-amazon marked this pull request as ready for review February 7, 2025 17:28
@nkomonen-amazon nkomonen-amazon requested review from a team as code owners February 7, 2025 17:28
Problem:

When a user starts up VSC, hasn't opened Q chat yet, then highlights text and
does a right click option like "send to prompt" THEN the whole prompt fails to
make it to chat.

This is because there is a race condition between the message being sent to the webview,
and the webview completed loading and ready to recieve messages.

Solution:

In the appToMessage publisher, verify that the chat is ready to recieve messages based
on if the webview sent the chat ui ready message. Otherwise wait until we get it,
and then send the message.

Signed-off-by: nkomonen-amazon <[email protected]>
Copy link
Contributor

@jpinkney-aws jpinkney-aws left a comment

Choose a reason for hiding this comment

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

I think its good now, just a couple more separate thoughts:

  1. Does it make sense to have a seperate uiMessagePublisher or is this something that should just live in the regular MessagePublisher? I think the upside of having it in one is technically the extension side who calls getAppsToWebViewMessagePublisher also has to wait for the ui to load before their event gets sent? It might prevent a few possible bugs? (not that I can think of any concrete examples off of the top of my head)

  2. Does it make sense to focus the webview if the chat isn't ready yet? Or can we open it if its not already open? I'm guessing theres logic in the send to prompt/all the other codewhisperer commands that does something to that effect but does it make sense to put all that logic in one place?

@nkomonen-amazon
Copy link
Contributor Author

nkomonen-amazon commented Feb 7, 2025

  1. Yeah I was thinking about directly adding it to MessagePublisher, but since MessagePublisher is used in the opposite direction from WebviewToApp, it wasn't completely obvious. Though we can still add the new logic in but just know which scenario to use it. Any thoughts?
  2. Thats a good point. I think we can probably call focusAmazonQPanel.execute(). Currently it is redundant since all the relevant calls in the fix call it before. But it will be a good fallback in future cases.

@jpinkney-aws

Signed-off-by: nkomonen-amazon <[email protected]>
@nkomonen-amazon
Copy link
Contributor Author

/runIntegrationTests

@nkomonen-amazon nkomonen-amazon merged commit 19d0346 into aws:master Feb 7, 2025
32 checks passed
@nkomonen-amazon nkomonen-amazon deleted the sendToQUiReady branch February 7, 2025 22:19
s7ab059789 pushed a commit to s7ab059789/aws-toolkit-vscode that referenced this pull request Feb 19, 2025
aws#6527)

## Problem:

When a user starts up VSC, hasn't opened Q chat yet, then highlights
text and does a right click option like "send to prompt" THEN the whole
prompt fails to make it to chat.

This is because there is a race condition between the message being sent
to the webview, and the webview completed loading and ready to recieve
messages.

## Solution:

In the appToMessage publisher, verify that the chat is ready to recieve
messages based on if the webview sent the chat ui ready message.
Otherwise wait until we get it, and then send the message.

### Demo

#### Before


https://github.com/user-attachments/assets/baf86737-4a5a-4f80-86fb-9abca2c56827

#### After 


https://github.com/user-attachments/assets/c3361403-5dae-49df-a878-94722eb4fc62

---

- Treat all work as PUBLIC. Private `feature/x` branches will not be
squash-merged at release time.
- Your code changes must meet the guidelines in
[CONTRIBUTING.md](https://github.com/aws/aws-toolkit-vscode/blob/master/CONTRIBUTING.md#guidelines).
- License: I confirm that my contribution is made under the terms of the
Apache 2.0 license.

---------

Signed-off-by: nkomonen-amazon <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants