Skip to content
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

Do not create new chat session when an old chat session is deleted #669

Merged
merged 1 commit into from Mar 10, 2024

Conversation

debanjum
Copy link
Collaborator

@debanjum debanjum commented Mar 9, 2024

Issue

Previously deleting a chat session from the side panel on desktop, web app would sometimes result in also creating a new chat session

Fix

get_conversation_by_user shouldn't return new conversation if
conversation with requested id not found.

It should only return new conversation if no specific conversation
is requested and no conversations found for user at all

Miscellaneous Improvements

  • Chat history load should be logged as call to that chat_history api,
    not the "chat" api
  • Show status updates of clearing conversation history in chat input
  • Simplify web, desktop client code by removing unnecessary new variables

Repro

  • Delete a new chat, this calls loadChat via window.onload which
    calls server /chat/history API endpoint with conversationId set to
    that of just deleted conversation sporadically

    The call to GET chat/history API with conversationId set occurs
    when window.onload triggers before the conversationId is deleted
    by the delete button after the DELETE /chat/history API call (via race)

  • In such a scenario, get_conversation_by_user called by
    chat/history API with conversationId of deleted conversation
    returns a new conversation

- Fix
  `get_conversation_by_user' shouldn't return new conversation if
  conversation with requested id not found.

  It should only return new conversation if no specific conversation
  is requested and no conversations found for user at all

- Repro
  - Delete a new chat, this calls loadChat via window.onload which
    calls server /chat/history API endpoint with conversationId set to
    that of just deleted conversation sporadically

    The call to GET chat/history API with conversationId set occurs
    when window.onload triggers before the conversationId is deleted
    by the delete button after the DELETE /chat/history API call (via race)

  - In such a scenario, get_conversation_by_user called by
    chat/history API with conversationId of deleted conversation
    returns a new conversation

- Miscellaneous
  - Chat history load should be logged as call to that chat_history api,
    not the "chat" api
  - Show status updates of clearing conversation history in chat input
  - Simplify web, desktop client code by removing unnecessary new variables
@debanjum debanjum added the fix Fix something that isn't working as expected label Mar 9, 2024
Copy link
Collaborator

@sabaimran sabaimran left a comment

Choose a reason for hiding this comment

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

Awesome to have a fix for this. Just one minor question about how conversationId is being interpreted in the html pages, but looks good.

src/interface/desktop/chat.html Show resolved Hide resolved
src/khoj/database/adapters/__init__.py Show resolved Hide resolved
src/khoj/routers/api_chat.py Show resolved Hide resolved
@debanjum debanjum merged commit 8eb3c44 into master Mar 10, 2024
9 checks passed
@sabaimran
Copy link
Collaborator

I took a second look at the code. The method aget_conversation_by_user, which is in the hot path for the /chat/ endpoint hasn't been updated. For that one, it'll still create a new conversation if/when the conversation_id doesn't come back with a match.

@debanjum debanjum deleted the fix-to-not-create-new-chat-session-on-chat-delete branch March 11, 2024 16:45
@debanjum
Copy link
Collaborator Author

debanjum commented Mar 15, 2024

I took a second look at the code. The method aget_conversation_by_user, which is in the hot path for the /chat/ endpoint hasn't been updated. For that one, it'll still create a new conversation if/when the conversation_id doesn't come back with a match.

Great catch! Fixed in #677

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix Fix something that isn't working as expected
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants