feat(amazonq): Use Chat History DB instead of in-memory store #6985
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
justinmk3
reviewed
Apr 9, 2025
tsmithsz
force-pushed
the
feature/agentic-chat
branch
from
87284a8 to
9bc7ee8
Compare
Contributor
Author
|
/retrybuilds |
avi-alpert
approved these changes
Apr 10, 2025
mr-lee
reviewed
Apr 10, 2025
jguoamz
reviewed
Apr 10, 2025
tsmithsz
force-pushed
the
feature/agentic-chat
branch
from
9bc7ee8 to
96f76c3
Compare
jguoamz
reviewed
Apr 10, 2025
packages/core/src/codewhispererChat/controllers/chat/controller.ts
Outdated
Show resolved
Hide resolved
tsmithsz
force-pushed
the
feature/agentic-chat
branch
2 times, most recently
from
de4747d to
b4f8fef
Compare
jguoamz
reviewed
Apr 10, 2025
tsmithsz
force-pushed
the
feature/agentic-chat
branch
from
b4f8fef to
0398a10
Compare
tsmithsz
force-pushed
the
feature/agentic-chat
branch
2 times, most recently
from
feece49 to
c6c760e
Compare
tsmithsz
force-pushed
the
feature/agentic-chat
branch
from
c6c760e to
e58911b
Compare
Hweinstock
reviewed
Apr 11, 2025
| } else { | ||
| allMessages.splice(-2) | ||
| this.logger.info(`Removed last assistant message and user message`) | ||
| } |
Contributor
There was a problem hiding this comment.
What is the case where the end of the chat history is a user message, shouldn't the assistant always respond?
| } | ||
|
|
||
| // Removes the most recent message(s) from the chat history for a given tab | ||
| clearRecentHistory(tabId: string): void { |
Contributor
There was a problem hiding this comment.
"recent" is ambiguous. Based on implementation it looks like it removes the last "exchange" (pair of messages). Anyway to make this more clear in docstring + name of the function?
| * 4. If the last message is from the assistant and it contains tool uses, and a next user | ||
| * message is set without tool results, then the user message will have cancelled tool results. | ||
| */ | ||
| fixHistory(tabId: string, newUserMessage: ChatMessage): void { |
Contributor
There was a problem hiding this comment.
There is definitely some non-trivial logic in this class that would benefit from some testing.
Hweinstock
approved these changes
Apr 11, 2025
Contributor
Hweinstock
left a comment
Hweinstock
left a comment
There was a problem hiding this comment.
Approving and merging to unblock, while @tsmithsz is OOTO. Hope there is time to add a solid test suite for this logic in a follow-up.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
We need to switch to using the Chat History DB instead of the local store
Solution
Tested manually
feature/xbranches will not be squash-merged at release time.