fix: initial message load issues #2292
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
myandrienko
commented
Feb 22, 2024
Comment on lines
+370
to
+378
| const channelQueryOptions: ChannelQueryOptions<StreamChatGenerics> & { | ||
| messages: { limit: number }; | ||
| } = useMemo( | ||
| () => | ||
| defaultsDeep(propChannelQueryOptions, { | ||
| messages: { limit: DEFAULT_INITIAL_CHANNEL_PAGE_SIZE }, | ||
| }), | ||
| [propChannelQueryOptions], | ||
| ); |
There was a problem hiding this comment.
Use DEFAULT_INITIAL_CHANNEL_PAGE_SIZE when loading the first page of messages.
myandrienko
commented
Feb 22, 2024
Comment on lines
-586
to
+604
| hasMore: hasMoreMessagesProbably( | ||
| channel.state.messages.length, | ||
| channelQueryOptions?.messages?.limit ?? DEFAULT_INITIAL_CHANNEL_PAGE_SIZE, | ||
| ), | ||
| hasMore: | ||
| channelInitializedExternally || | ||
| hasMoreMessagesProbably( | ||
| channel.state.messages.length, | ||
| channelQueryOptions.messages.limit, | ||
| ), |
There was a problem hiding this comment.
If it wasn't the Channel component itself that loaded the first page of messages, don't trust the message count to determine if there's more messages to load, since the function that loaded messages could have totally different limits.
myandrienko
commented
Feb 22, 2024
| @@ -79,6 +80,7 @@ export const usePaginatedChannels = < | |||
|
|
|||
| const newOptions = { | |||
| limit: options?.limit ?? MAX_QUERY_CHANNELS_LIMIT, | |||
| message_limit: options?.message_limit ?? DEFAULT_INITIAL_CHANNEL_PAGE_SIZE, | |||
There was a problem hiding this comment.
Use DEFAULT_INITIAL_CHANNEL_PAGE_SIZE to limit the number of messages when loading channel list.
myandrienko
commented
Feb 22, 2024
|
|
||
| return () => { | ||
| scrollElement.removeEventListener('scroll', scrollListener, useCapture); | ||
| scrollElement.removeEventListener('resize', scrollListener, useCapture); | ||
| }; | ||
| }, [initialLoad, scrollListener, useCapture]); | ||
| }, [initialLoad, useCapture]); |
There was a problem hiding this comment.
Prevent this effect from firing excessively. Note that scrollListener is now a ref, so it always has access to the latest props.
myandrienko
commented
Feb 22, 2024
| @@ -68,7 +69,7 @@ const MessageListWithContext = < | |||
| headerPosition, | |||
| read, | |||
| renderMessages = defaultRenderMessages, | |||
| messageLimit = 100, | |||
| messageLimit = DEFAULT_NEXT_CHANNEL_PAGE_SIZE, | |||
There was a problem hiding this comment.
Remove hard-coded limit for message page size.
myandrienko
commented
Feb 22, 2024
| @@ -184,7 +185,7 @@ const VirtualizedMessageListWithContext = < | |||
| loadMoreNewer, | |||
| Message: MessageUIComponentFromProps, | |||
| messageActions, | |||
| messageLimit = 100, | |||
| messageLimit = DEFAULT_NEXT_CHANNEL_PAGE_SIZE, | |||
There was a problem hiding this comment.
Remove hard-coded limit for message page size.
|
Size Change: +30.1 kB (+2%) Total Size: 1.79 MB
ℹ️ View Unchanged
|
myandrienko
commented
Feb 22, 2024
| scrollElement.addEventListener('scroll', scrollListener, useCapture); | ||
| scrollElement.addEventListener('resize', scrollListener, useCapture); | ||
| scrollListener(); |
There was a problem hiding this comment.
Note that we'll only load a single additional page of messages in case the first page is not enough to cause overflow of the message list.
To keep loading pages until there is overflow, or there is no more pages to load, we can do something like this:
useLayoutEffect(() => {
if (!isLoading) {
scrollListenerRef.current?.();
}
}, [isLoading]);However, I don't see a way to have the same behavior in the VirtualizedMessageList, and since our default page size is rather big (100 messages), I think it's better to keep things consistent.
myandrienko
requested review from
oliverlaz,
MartinCupela and
arnautov-anton
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2292 +/- ##
==========================================
+ Coverage 85.03% 85.04% +0.01%
==========================================
Files 344 344
Lines 7689 7702 +13
Branches 2140 2143 +3
==========================================
+ Hits 6538 6550 +12
+ Misses 807 806 -1
- Partials 344 346 +2 ☔ View full report in Codecov by Sentry. |
|
Thank you, @myandrienko, closing #2275. |
arnautov-anton
approved these changes
Feb 23, 2024
github-actions bot
pushed a commit
that referenced
this pull request
Feb 23, 2024
## [11.10.0](v11.9.0...v11.10.0) (2024-02-23) ### Bug Fixes * initial message load issues ([#2292](#2292)) ([3685030](3685030)) ### Features * add customizable reaction details sorting ([#2290](#2290)) ([652e3a5](652e3a5)), closes [#2289](#2289) * add customizable reactions sorting ([#2289](#2289)) ([78c6107](78c6107))
|
🎉 This PR is included in version 11.10.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
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.
🎯 Goal
This PR covers a range of issues that originate from incorrect initialization of message list state. All the cases are covered below.
🛠 Implementation details
ChannelList,Channel,MessageList,VirtualizedMessageListnot respecting the message limitsWe have several default limits listed in the
limit.tsfile. However they were not always respected:MessageList,VirtualizedMessageListdidn't useDEFAULT_NEXT_CHANNEL_PAGE_SIZEand defaulted to the hard-coded message limit of 100.Channeldidn't useDEFAULT_INITIAL_CHANNEL_PAGE_SIZEwhen fetching the first page of messages.ChannelListdidn't useDEFAULT_INITIAL_CHANNEL_PAGE_SIZEas the message limit per channel when fetching channels.This is now fixed.
False-negative
hasMorewhen messages are fetched byChannelListWe have two main ways to initialize the message list: it can be initialized by the
Channelcomponent; or it can be initialized by theChannelListcomponent (usingusePaginatedChannelshook). If the message list is initialized by theChannelList, theChannelcomponent will not try to query the channel, and will use messages fetched by theChannelListinstead.The problem here is that to determine if the channel has more messages to load, we used to compare the number of messages currently in the channel with the limit set on the
Channel. But since it was not theChannelbut theChannelListthat fetched the messages, this check makes no sense:ChannelListhas its own separate limits.Consider this example:
The
Channelcomponent will compare the number of messages fetched by theChannelList(5) with its own limit (10) and determine that the channel has no more messages.This is fixed by always setting
hasMoreto false in channel if it was not theChannelcomponent that fetched the messages. In the worst case we'll just make one redundant query.The "tall window" problem
In case the message container is very tall, or the message limit for the initial load is very small, we can run into a situation when the first page of messages is not long enough to trigger overflow in the message list. Without overflow, scroll events don't fire, so the next page of messages is never fetched.
This is not usually a problem in the
VirtualizedMessageList, since it will immediately load the next page once in this case. However, theMessageListdidn't have this logic in place. It is now added (by invoking thescrollListeneronce on mount).Note: both
VirtualizedMessageListandMessageListwill only try loading the next page once if the first page of messages is not long enough to cause overflow. A better solution would be to keep loading pages until there's overflow or there is no more pages. This solution is not currently possible to implement inVirtualizedMessageList, so for the sake of consistency I didn't implement itMessageList(event though it is possible).In most cases this not an issue, since our default page size is 100 messages which is enough to fill even very tall containers.