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: branching and forking sometimes break conversation structure #4772

Closed
wants to merge 2 commits into from
Closed

fix: branching and forking sometimes break conversation structure #4772

wants to merge 2 commits into from

Conversation

xyqyear
Copy link
Contributor

@xyqyear xyqyear commented Nov 22, 2024

Summary

As reported in #4761 and #3813, conversations will seemingly randomly collapse when regenerating or forking. I believe this is because in buildTree.ts, the function assumes messages are in the correct order when building the tree (parents are before their children). But for some reason, this is only the case when there are less than 17 messages in the conversation.

If the messages are not in correct order, when looking for a message's parent in messageMap, the parent might not have put into the map yet resulting in the current message becoming a root message.

What I've done is to populate messageMap first in one loop, and then in another loop look for a message's parent. This way, the parent should be found wherever it is in the message list.

I don't know if messages should be in the correct order when queried from the api given the previous assumption in buildTree.ts but this change should resolve the issue anyway.

Change Type

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

Testing

npm run test:client is passing.

I have not introduced any changes to the api.

I've tested my changes with a conversation described in #4761 (comment)

Before the change: conversatios with more than 16 messages will collapse after forking. Conversations with more than 16 messages and include any branching will collapse.

After the changes: all is good.

Checklist

Please delete any irrelevant options.

  • My code adheres to this project's style guidelines
  • I have performed a self-review of my own code

@xyqyear
Copy link
Contributor Author

xyqyear commented Nov 22, 2024

The first commit worked but somehow it lags the ui when send any message after a while. The second commit fixed the lag problem but it will change the order of branches.

forking with option "include related branches" is totally broken when there are more than 16 messages. What's this magic number 16? 😂

Might be better to just fix api/server/utils/import/fork.js @danny-avila

After further testing, fork.js seems to be fine. Might be something to do with saving to or querying from db?

@xyqyear xyqyear closed this Nov 22, 2024
@danny-avila
Copy link
Owner

hey @xyqyear thanks for investigating this, it has eluded me for some time. In general, we can never trust the order of arrays/lists unless we explicitly sort at some step. Maybe a sorting algo is in order server-side?

@xyqyear
Copy link
Contributor Author

xyqyear commented Nov 22, 2024

The thing is, when the total number of messages in a conversation is less than 16, all forking and branching funtionality works fine. But when it exceeds 16, all sort of bad things happen. From what I've observed, the direct cause in the frontend is messages are not in order in buildTree.

Hopefully the consistent reproduction steps help with resolving this issue. I will keep investigating as well.

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.

2 participants