-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Feature: Store chat history in Cosmos DB #2063
Changes from 3 commits
bbc74de
6846c4d
07fe2ae
0189dfc
44bfc40
11c4d9f
9863e67
b0aacb2
e0e85f1
ec12bca
867d3d3
50c6ea9
4dc3b48
c2a810f
e48535d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -90,6 +90,7 @@ export type Config = { | |
showSpeechOutputBrowser: boolean; | ||
showSpeechOutputAzure: boolean; | ||
showChatHistoryBrowser: boolean; | ||
showChatHistoryCosmos: boolean; | ||
}; | ||
|
||
export type SimpleAPIResponse = { | ||
|
@@ -103,3 +104,21 @@ export interface SpeechConfig { | |
isPlaying: boolean; | ||
setIsPlaying: (isPlaying: boolean) => void; | ||
} | ||
|
||
export type HistoryListApiResponse = { | ||
items: { | ||
id: string; | ||
entra_id: string; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is oid, right? Maybe entra_oid would be slightly clearer (versus an Entra group id) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a typo. I will fix it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 0189dfc |
||
title?: string; | ||
_ts: number; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What/why is _ts? It's a funny variable name, so it could benefit from either an inline comment or a rename. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
https://learn.microsoft.com/en-us/rest/api/cosmos-db/documents There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm, maybe we should change it in the server-side when we pass back the results? I'm imagining that if we had an Azure SQL history provider, then we wouldn't want to use _ts, we'd want them both to pass back timestamp. And ideally use the same TypeScript models. Does that make sense? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated in 11c4d9f |
||
}[]; | ||
continuation_token?: string; | ||
}; | ||
|
||
export type HistroyApiResponse = { | ||
id: string; | ||
entra_id: string; | ||
title?: string; | ||
answers: any; | ||
_ts: number; | ||
}; |
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.
Do you think the chat history endpoints could be in a different file/blueprint? That would make it to bring in additional providers (I've been asked about Azure SQL, for example) without bloating app.py.
I know we only have a single file/blueprint for routes now though, so it's possible that I haven't organized it in such a way that it's feasible.
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.
Certainly,
app.py
is getting too large, so I think it's good to separate the blueprint. I'll try to restructure it to separate it.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.
Configure separate blueprint in 44bfc40