Skip to content

fix(amazonq): Grouping read tool messages under contextList #6975

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

Merged
merged 8 commits into from
Apr 10, 2025

Conversation

laileni-aws
Copy link
Contributor

@laileni-aws laileni-aws commented Apr 9, 2025

Problem

  • We show read tool message for each file read which is occupying the entire chat with these messages which is not super important.

Solution

  • Group all the read tool messaged under ContextList.
    image

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

Copy link

github-actions bot commented Apr 9, 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.

@laileni-aws laileni-aws marked this pull request as ready for review April 9, 2025 18:14
@laileni-aws laileni-aws requested a review from a team as a code owner April 9, 2025 18:14
@laileni-aws laileni-aws force-pushed the feature/agentic-chat branch from 652ae34 to 234e0c5 Compare April 9, 2025 19:40
@laileni-aws laileni-aws requested a review from jguoamz April 9, 2025 19:41
@ctlai95
Copy link
Contributor

ctlai95 commented Apr 9, 2025

This doesn't group listDirectory messages yet, right?

Comment on lines 654 to 655
if (toolUse?.name === ToolType.FsRead) {
this.dispatcher.sendToolMessage(
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 it might be cleaner if you move this check inside sendToolMessage

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 am using sendToolMessage for FsRead and ListDirectory and sendChatMessage for FsWrite and ExecuteBash tool output.

Copy link
Contributor

Choose a reason for hiding this comment

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

but can we not always use sendToolMessage and then inside that function have a condition to use onChatAnswerUpdated or onChatAnswerReceived

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Where do we have this conditional logic?
We can not do this in webview ?
I think not in connector.ts too So, messenger will be the best place to do this!

@laileni-aws laileni-aws requested a review from ctlai95 April 9, 2025 22:58
@laileni-aws laileni-aws force-pushed the feature/agentic-chat branch from b41dca8 to 940e55a Compare April 9, 2025 23:11
@laileni-aws
Copy link
Contributor Author

/retryBuilds

Comment on lines +642 to +644
if (toolUse?.name === ToolType.FsRead || toolUse?.name === ToolType.ListDirectory) {
return this.sendReadAndListDirToolMessage(toolUse, session, tabID, triggerID, messageIdToUpdate)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this is still confusing, I think the condition should not happen in this function and always call processToolMessage

Copy link
Contributor

@ctlai95 ctlai95 left a comment

Choose a reason for hiding this comment

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

Approving to unblock, please follow up in a future PR

@zixlin7 zixlin7 merged commit db64637 into aws:feature/agentic-chat Apr 10, 2025
19 of 22 checks passed
laileni-aws added a commit to laileni-aws/aws-toolkit-vscode that referenced this pull request Apr 10, 2025
laileni-aws added a commit to laileni-aws/aws-toolkit-vscode that referenced this pull request Apr 10, 2025
laileni-aws added a commit to laileni-aws/aws-toolkit-vscode that referenced this pull request Apr 10, 2025
laileni-aws added a commit to laileni-aws/aws-toolkit-vscode that referenced this pull request Apr 10, 2025
zixlin7 pushed a commit that referenced this pull request Apr 10, 2025
## Problem
- Reverting PR: #6975  as this is causing regression in some cases.

## Solution


---

- 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.
laileni-aws added a commit to laileni-aws/aws-toolkit-vscode that referenced this pull request Apr 10, 2025
- We show read tool message for each file read which is occupying the
entire chat with these messages which is not super important.
- Group all the read tool messaged under ContextList.

![image](https://github.com/user-attachments/assets/194e4ca1-7645-4419-8b9b-b5ac9a58279f)

---

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

5 participants