-
Notifications
You must be signed in to change notification settings - Fork 73
feat(amazonq): chat history and conversation persistence #941
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
feat(amazonq): chat history and conversation persistence #941
Conversation
* - Searching through conversations | ||
* - Managing conversation tabs | ||
* - Handling chat history operations (list, delete, etc.) | ||
* - TODO: Export chat |
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.
most likely, export chat will be handled on extension - we can remove this TODO
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 planned to make it purely Client to Extension flow, but I realize that we don't have good way to restore not opened tabs from History List in Chat UI. Since server is handling state and mapping between open tabId in UI and History item id, we'd probably need a roundtrip to server to restore tab first.
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.
right. mynah-ui requires us to open a tab before they can serialize it, so when a user wants to export an unopened tab from history, we have to restore it first.
alternatively, mynah-ui could update their serializeChat function to allow chatItems as a parameter, this would allow the server to send a list of chats directly from history without having to open the 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.
i would say that second option would result in a better UX. users dont expect a tab to open when exporting from history
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 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.
@avi-alpert you can leave placeholders for export here, I'll implement it through communication with Server after all.
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.
yeah, discussed offline and we changed the approach - just leave the comment there
} else if (params.action === 'delete') { | ||
this.#chatHistoryDb.deleteHistory(historyID) | ||
} | ||
// TODO: Handle Export action clicked |
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.
again, can be removed
this.#chatHistoryDb.deleteHistory(historyID) | ||
} | ||
// TODO: Handle Export action clicked | ||
// else if (params.action === 'export') { |
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 we return error if unsupported action was requested?
/** | ||
* Opens new tab with a conversation from history. | ||
*/ | ||
async restoreTab(selectedTab?: Tab | 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.
nit: why do we need to support null
as a param here?
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 case chatDb doesn't find the tab that we want to restore
updateOrCreateConversation, | ||
} from './util' | ||
import * as crypto from 'crypto' | ||
import * as fs from 'fs' |
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 fs
used in this file somewhere? I do not see any usages. This will not work for web-worker runtime.
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.
correct, i'll remove from this file. its only used in utils
import * as fs from 'fs' | ||
import * as path from 'path' | ||
import { homedir } from 'os' | ||
import { fileURLToPath } from 'url' |
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 there any cross-platform version of this?
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.
you mean web compatible? yeah i can likely switch to URL API
*/ | ||
|
||
import * as path from 'path' | ||
import * as fs from 'fs/promises' |
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.
Viktor is introducing enable flag for history, so we will be able to enable history for IDEs only. However, how much work would it be to make it web-compatible? We do provide fs
as a feature that is cross-platform: https://github.com/aws/language-server-runtimes/blob/main/runtimes/server-interface/workspace.ts#L22, but I think you are using methods that are not implemented in runtime fs
yet.
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.
oh great, yeah I can easily switch over to that fs. I was just concerned by the comment
May not provide full filesystem
* access to files that are not currently open or outside the
* workspace root
since history files are in the home directory. But i can try with your fs
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 only thing im not sure about is a web compatible version of import { homedir } from 'os'
. looking at what we do in vs code extension, if the user is on web we use extContext.globalStorageUri.toString()
as the "home directory" https://github.com/aws/aws-toolkit-vscode/blob/f540496854b5c902c772374f8354ebfd23b85397/packages/core/src/shared/fs/fs.ts#L502
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.
perhaps workspace.getServerDataDirPath or workspace.getTempDirPath?
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.
discussed in separate thread
Note: one of my new unit tests is failing, will push a fix |
|
||
export type Conversation = { | ||
conversationId: string | ||
clientType: 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.
nit: what's clientType
and why is it needed?
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 name of the client i.e. Visual Studio Code, IntelliJ IDEA, etc. we populate it from features.lsp.getClientInitializeParams().clientInfo.name
. its not needed for anything right now, but when we implemented it in vs code we wanted to future-proof in case we ever needed to know which conversations in history happened on which client
This reverts commit 0cc9bb1.
features.runtime.platform === 'browser' | ||
? features.workspace.fs.getServerDataDirPath('amazonq-chat') | ||
: getUserHomeDir(), | ||
'.aws/amazonq/history' |
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: I do not think we need this long path for browser case
Problem
Need to migrate conversation persistence and chat history features to flare.
Solution
Automatically persist conversations to JSON files in
~/.aws/amazonq/history
, one for each workspace where Amazon Q chats occur. User prompt and assistant responses are saved to chat history. Closing and reopening IDE automatically restores all tabs that were previously opened. The first user prompt sent after a conversation is restored includes up to last 10 messages from history.Clicking on the chat history button shows a chat history list. Users click on an conversation to open it back up (currently open conversations are in bold), or click on delete action to delete a conversation from history.
/clear
quick action deletes current conversation from history.Note: chat export is not part of the scope of this PR, that will be implemented in chat-client
See original vscode PR: link
License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.