-
Notifications
You must be signed in to change notification settings - Fork 16
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
fix(ai): history fix #423
fix(ai): history fix #423
Conversation
src/api/ai_help.rs
Outdated
// If not, we are in a conversation that started while history was disabled. | ||
// We do not store either the history/chat nor the message itself in this case. | ||
if let Some(parent_id) = parent_id { | ||
let parent_message = help_history_get_message(&mut conn, user, &parent_id)?; |
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'd prefer we keep the happy path fast. So I'd suggest we do two checks:
-
Introduce
update_help_history
which we call ifparent_id
is set instead ofadd_help_history
(so we don't create a new entry if we don't store anything). -
Handle the foreign key violation in
add_help_history_message
gracefully.
If that seems to optimized and complicated I at least would suggest adding a help_history_has_message
that doesn't read the whole message.
Both applies to the other occurrence.
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.
👍
src/db/ai_help.rs
Outdated
.set(( | ||
ai_help_history::label.eq(label), | ||
ai_help_history::updated_at.eq(diesel::dsl::now), | ||
)) |
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.
.set(( | |
ai_help_history::label.eq(label), | |
ai_help_history::updated_at.eq(diesel::dsl::now), | |
)) | |
.set(ai_help_history::label.eq(label)) |
I see that the naming is bad but updated_at
refers to the messages and not the with that chat id and the label is not considered an update. Currently (if not mistaken) the front-end will trigger the label creation even for old chats (if the user closed the tab before the label was generated). And those would receive a "wrong"/ misleading updated_at.
Prevent persisting to ai help history if a message's parent message can not be found in the database.
(MP-870)
This resolves the special case where a user turns on history mid-conversation (in a different tab). Conversations that were started while history was turned off are not stored now and do not result in an error.