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

🚑 fix(export): Export Issue with New Chat #2777

Merged
merged 8 commits into from
May 28, 2024

Conversation

ohneda
Copy link
Contributor

@ohneda ohneda commented May 18, 2024

Summary

When starting a new chat, sending a few messages, and then exporting the conversation, the exported text only includes the first message. This issue is reproducible across all output file formats.

This issue occurs in a rare use case, so it might be unimportant. However, I investigated it to learn the code. If you plan to refactor or fix this differently, please feel free to close this PR.

Issue

How to Reproduce

  1. Start a new chat.
  2. Send more than one message.
  3. Export the conversation.
  4. The exported text only includes the first message.

Cause

Premise

  • When a message is sent, it is added to the react-query cache using the setMessages function from useChatHelpers, with conversationId as the key.

Flow

  1. When a message is sent in a new chat, it is added to the react-query cache with the key 'new' because the actual conversationId has yet to be obtained.
  2. After the first message is successfully sent, an actual conversationId is assigned to the conversation.
  3. The useGetMessagesByConvoId function in useChatHelpers fetches messages from the API using the actual conversationId and sets them in the cache with the actual conversationId as the key. At this point, only one message exists for this conversationId.
  4. Subsequent messages in the new chat are added to the cache with the key 'new', but they are not added to the cache with the actual conversationId. As a result, only the first message remains under the actual conversationId.
  5. During export, the actual conversationId is used, and react-query returns the cache of that key, so only the first message (stored under the actual conversationId) is included in the exported text.

(Please let me know if my understanding is incorrect)

Fix

There are several options to fix this

  1. Disable export for new chats (for /c/new):
    Cons: This is very simple but the export icon might be inconsistently displayed for chats, which could confuse users.
  2. Remove useGetMessagesByConvoId from useChatHelpers to avoid caching the first message
    Cons: The reason for fetching messages here is unclear(these messages might not be used anywhere in the hook), and removing them might cause side effects.
  3. Set refetchOnMount to true in useGetMessagesByConvoId within useExportConversation to ensure the latest conversation,
    Cons: This might cause unnecessary network access every time the export modal is opened.

I chose the third option. Although it might cause unnecessary network access, it ensures that the latest conversation is always exported.

Again, if you choose a different option, do not worry about it and close this PR.

Change Type

  • Bug fix (non-breaking change which fixes an issue)

Testing

I verified that I could export correctly in the new chat by doing the reproduction steps above.

Checklist

  • My code adheres to this project's style guidelines
  • I have performed a self-review of my own code
  • I have commented in any complex areas of my code
  • Local unit tests pass with my changes

@danny-avila
Copy link
Owner

Thanks for studying this!

3 is not a good option because of the con you listed:

Cons: This might cause unnecessary network access every time the export modal is opened.

it's likely true. This is a known bug for me but it should be simple to fix with a 4th option

When the current path is "new" we can simply get the query data. I can get to it soon or you could try that yourself. Looking getQueryData as it's being used in the project.

@ohneda
Copy link
Contributor Author

ohneda commented May 18, 2024

@danny-avila Oh, so you mean that we should use the same logic as useChatHelpers to get data? I updated the implementation. Surely this is correct and works. Please review it when you have time.

At first, I thought there might be a case where no data existed for the key new, but I didn't have to worry about that in this case.

@danny-avila
Copy link
Owner

@danny-avila Oh, so you mean that we should use the same logic as useChatHelpers to get data? I updated the implementation. Surely this is correct and works. Please review it when you have time.

At first, I thought there might be a case where no data existed for the key new, but I didn't have to worry about that in this case.

Not quite, you don't need another query call, you can simply retrieve the latest cache. I'll show how soon

@ohneda
Copy link
Contributor Author

ohneda commented May 22, 2024

Oops, thanks, that helps me understand.

@danny-avila
Copy link
Owner

Was hoping to get to this today along with a few other PRs but ran out of time. Will try to be back on it Sunday/Monday

@danny-avila
Copy link
Owner

See 5addf12 for my main changes

@danny-avila danny-avila merged commit c9e7d4a into danny-avila:main May 28, 2024
2 checks passed
@ohneda ohneda deleted the fix/export-issue-for-new-chat branch May 31, 2024 07:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants