fix: message list freezes on selectChat #4638
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
FYI this moves the jumpToMessage function without changing it. This should fix the bug where we'd try to load too many messages, namely (almost) all messages of the chat. The problem is that `loadChat` might return early, to delegate its job to `effect.jumpToMessage`, but there could already be other effects (such as `fetchMoreMessagesTop`) already queued up, so the `effect.jumpToMessage` would get added to the back of the queue, resulting in other operations getting executed prior to the `jumpToMessage`. Those other operations do not work well with an uninitialized store state, namely when `oldestFetchedMessageListItemIndex` is `-1`. In particular, `fetchMoreMessagesTop` would call `loadMessages` with `newestFetchedMessageListItemIndex === -2` and `oldestFetchedMessageListItemIndex === 0`, which, in turn, would call `getMessages()` with the list of almost all message IDs. Reproduction steps (not 100% reliable): 1. Open chat 1. 2. Receive a message in chat 2. Use a different device to send the message. It is preferable to keep the window focused. 3. Wait ~5 seconds (not sure why). 4. Click on chat 2 in the chat list. Chat 2 will open, but with a much greater delay. This commit also has potential to fix the issue where the messages list does not show any messages when the chat gets opened. And maybe other problems might get fixed, because this fixes a race condition. The problem has likely been surfaced by the recent "performance" changes and bug fixes related to messagelist.ts. Users report that it has become noticeable on 1.53.0, see diff: v1.52.1...v1.53.0 Co-Authored-By: ralphtheninja <[email protected]> Co-authored-by: Nico de Haen <[email protected]>
WofWca
force-pushed
the
wofwca/fix-message-list-freezes
branch
from
a58cee7 to
89af4c3
Compare
Simon-Laux
approved these changes
Feb 14, 2025
|
@WofWca I'm seeing errors in the log when deleting a chat:
After some digging (
|
|
@ralphtheninja This seems like a different issue. |
Yes, maybe. But it's called from |
|
Not off the top of my head. Maybe this code is executed on the old instance of |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
FYI this moves the jumpToMessage function without changing it.
This should fix the bug where we'd try to load too many messages,
namely (almost) all messages of the chat.
The problem is that
loadChatmight return early,to delegate its job to
effect.jumpToMessage,but there could already be other effects
(such as
fetchMoreMessagesTop) already queued up,so the
effect.jumpToMessagewould get addedto the back of the queue, resulting in other operations getting
executed prior to the
jumpToMessage.Those other operations do not work well with an uninitialized
store state, namely when
oldestFetchedMessageListItemIndexis-1.In particular,
fetchMoreMessagesTopwould callloadMessageswith
newestFetchedMessageListItemIndex === -2andoldestFetchedMessageListItemIndex === 0, which, in turn,would call
getMessages()with the list of almost all message IDs.Reproduction steps (not 100% reliable):
Use a different device to send the message.
It is preferable to keep the window focused.
Chat 2 will open, but with a much greater delay.
2025-02-14-GuZP878AWW.mp4
This commit also has potential to fix the issue
where the messages list does not show any messages
when the chat gets opened.
And maybe other problems might get fixed, because this fixes
a race condition.
The problem has likely been surfaced by the recent "performance"
changes and bug fixes related to messagelist.ts.
Users report that it has become noticeable on 1.53.0, see diff:
v1.52.1...v1.53.0
I tested this for ~15 minutes with various other
jumpToMessages (e.g. from gallery, from notification), and found no issues.