From 36850305435757090f28d63bed4306ec9d7481e1 Mon Sep 17 00:00:00 2001 From: Matvei Andrienko Date: Fri, 23 Feb 2024 16:20:52 +0100 Subject: [PATCH] fix: initial message load issues (#2292) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ### 🎯 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`, `VirtualizedMessageList` not respecting the message limits We have several default limits listed in the `limit.ts` file. However they were not always respected: 1. `MessageList`, `VirtualizedMessageList` didn't use `DEFAULT_NEXT_CHANNEL_PAGE_SIZE` and defaulted to the hard-coded message limit of 100. 2. `Channel` didn't use `DEFAULT_INITIAL_CHANNEL_PAGE_SIZE` when fetching the first page of messages. 3. `ChannelList` didn't use `DEFAULT_INITIAL_CHANNEL_PAGE_SIZE` as the message limit per channel when fetching channels. This is now fixed. #### False-negative `hasMore` when messages are fetched by `ChannelList` We have two main ways to initialize the message list: it can be initialized by the `Channel` component; or it can be initialized by the `ChannelList` component (using `usePaginatedChannels` hook). If the message list is initialized by the `ChannelList`, the `Channel` component will not try to query the channel, and will use messages fetched by the `ChannelList` instead. 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 the `Channel` but the `ChannelList` that fetched the messages, this check makes no sense: `ChannelList` has its own separate limits. Consider this example: ```tsx const App = () => ( {/* ... */} ); ``` The `Channel` component will compare the number of messages fetched by the `ChannelList` (5) with its own limit (10) and determine that the channel has no more messages. This is fixed by always setting `hasMore` to false in channel if it was not the `Channel` component 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, the `MessageList` didn't have this logic in place. It is now added (by invoking the `scrollListener` _once_ on mount). **Note:** both `VirtualizedMessageList` and `MessageList` will 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 in `VirtualizedMessageList`, so for the sake of consistency I didn't implement it `MessageList` (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. --- .../core-components/message-list.mdx | 4 +++ .../core-components/virtualized-list.mdx | 4 +++ examples/typescript/src/index.tsx | 2 +- package.json | 2 ++ src/components/Channel/Channel.tsx | 25 +++++++++++++++---- .../Channel/__tests__/Channel.test.js | 2 +- .../ChannelList/hooks/usePaginatedChannels.ts | 2 ++ .../InfiniteScroll.tsx | 20 ++++++--------- src/components/MessageList/MessageList.tsx | 3 ++- .../MessageList/VirtualizedMessageList.tsx | 7 ++++-- yarn.lock | 12 +++++++++ 11 files changed, 61 insertions(+), 22 deletions(-) diff --git a/docusaurus/docs/React/components/core-components/message-list.mdx b/docusaurus/docs/React/components/core-components/message-list.mdx index 8b76c63f9..99792cf03 100644 --- a/docusaurus/docs/React/components/core-components/message-list.mdx +++ b/docusaurus/docs/React/components/core-components/message-list.mdx @@ -476,6 +476,10 @@ Array of allowed message actions (ex: 'edit', 'delete', 'reply'). To disable all The limit to use when paginating new messages (the page size). +:::caution +After mounting, the `MessageList` component checks if the list is completely filled with messages. If there is some space left in the list, `MessageList` will load the next page of messages, but it will do so _only once_. This means that if your `messageLimit` is too low, or if your viewport is very large, the list will not be completely filled. Set the limit with this in mind. +::: + | Type | Default | | ------ | ------- | | number | 100 | diff --git a/docusaurus/docs/React/components/core-components/virtualized-list.mdx b/docusaurus/docs/React/components/core-components/virtualized-list.mdx index 7ede49006..48f0a1509 100644 --- a/docusaurus/docs/React/components/core-components/virtualized-list.mdx +++ b/docusaurus/docs/React/components/core-components/virtualized-list.mdx @@ -186,6 +186,10 @@ Custom UI component to display an individual message. The limit to use when paginating messages (the page size). +:::caution +After mounting, the `VirtualizedMessageList` component checks if the list is completely filled with messages. If there is some space left in the list, `VirtualizedMessageList` will load the next page of messages, but it will do so _only once_. This means that if your `messageLimit` is too low, or if your viewport is very large, the list will not be completely filled. Set the limit with this in mind. +::: + | Type | Default | | ------ | ------- | | number | 100 | diff --git a/examples/typescript/src/index.tsx b/examples/typescript/src/index.tsx index 0a9c84979..a6e481519 100644 --- a/examples/typescript/src/index.tsx +++ b/examples/typescript/src/index.tsx @@ -10,7 +10,7 @@ import * as serviceWorker from './serviceWorker'; createRoot(document.getElementById('root')!).render( - + , ); // If you want your app to work offline and load faster, you can change diff --git a/package.json b/package.json index 297008ec1..0def1d0bb 100644 --- a/package.json +++ b/package.json @@ -71,6 +71,7 @@ "isomorphic-ws": "^4.0.1", "linkifyjs": "^4.1.0", "lodash.debounce": "^4.0.8", + "lodash.defaultsdeep": "^4.6.1", "lodash.throttle": "^4.1.1", "lodash.uniqby": "^4.7.0", "nanoid": "^3.3.4", @@ -155,6 +156,7 @@ "@types/jsdom": "^21.1.5", "@types/linkifyjs": "^2.1.3", "@types/lodash.debounce": "^4.0.7", + "@types/lodash.defaultsdeep": "^4.6.9", "@types/lodash.throttle": "^4.1.7", "@types/lodash.uniqby": "^4.7.7", "@types/moment": "^2.13.0", diff --git a/src/components/Channel/Channel.tsx b/src/components/Channel/Channel.tsx index b7c560164..35e41b0e1 100644 --- a/src/components/Channel/Channel.tsx +++ b/src/components/Channel/Channel.tsx @@ -10,6 +10,7 @@ import React, { } from 'react'; import debounce from 'lodash.debounce'; +import defaultsDeep from 'lodash.defaultsdeep'; import throttle from 'lodash.throttle'; import { ChannelAPIResponse, @@ -346,7 +347,7 @@ const ChannelInner = < acceptedFiles, activeUnreadHandler, channel, - channelQueryOptions, + channelQueryOptions: propChannelQueryOptions, children, doDeleteMessageRequest, doMarkReadRequest, @@ -366,6 +367,16 @@ const ChannelInner = < skipMessageDataMemoization, } = props; + const channelQueryOptions: ChannelQueryOptions & { + messages: { limit: number }; + } = useMemo( + () => + defaultsDeep(propChannelQueryOptions, { + messages: { limit: DEFAULT_INITIAL_CHANNEL_PAGE_SIZE }, + }), + [propChannelQueryOptions], + ); + const { client, customClasses, @@ -546,6 +557,7 @@ const ChannelInner = < useLayoutEffect(() => { let errored = false; let done = false; + let channelInitializedExternally = true; (async () => { if (!channel.initialized && initializeOnMount) { @@ -571,6 +583,7 @@ const ChannelInner = < await getChannel({ channel, client, members, options: channelQueryOptions }); const config = channel.getConfig(); setChannelConfig(config); + channelInitializedExternally = false; } catch (e) { dispatch({ error: e as Error, type: 'setError' }); errored = true; @@ -583,10 +596,12 @@ const ChannelInner = < if (!errored) { dispatch({ channel, - hasMore: hasMoreMessagesProbably( - channel.state.messages.length, - channelQueryOptions?.messages?.limit ?? DEFAULT_INITIAL_CHANNEL_PAGE_SIZE, - ), + hasMore: + channelInitializedExternally || + hasMoreMessagesProbably( + channel.state.messages.length, + channelQueryOptions.messages.limit, + ), type: 'initStateFromChannel', }); diff --git a/src/components/Channel/__tests__/Channel.test.js b/src/components/Channel/__tests__/Channel.test.js index d7fdf6bd3..151cebba5 100644 --- a/src/components/Channel/__tests__/Channel.test.js +++ b/src/components/Channel/__tests__/Channel.test.js @@ -290,7 +290,7 @@ describe('Channel', () => { await waitFor(() => { expect(watchSpy).toHaveBeenCalledTimes(1); - expect(watchSpy).toHaveBeenCalledWith(undefined); + expect(watchSpy).toHaveBeenCalledWith({ messages: { limit: 25 } }); }); }); diff --git a/src/components/ChannelList/hooks/usePaginatedChannels.ts b/src/components/ChannelList/hooks/usePaginatedChannels.ts index ad79cdaa9..8387bdc09 100644 --- a/src/components/ChannelList/hooks/usePaginatedChannels.ts +++ b/src/components/ChannelList/hooks/usePaginatedChannels.ts @@ -9,6 +9,7 @@ import { useChatContext } from '../../../context/ChatContext'; import type { DefaultStreamChatGenerics } from '../../../types/types'; import type { ChannelsQueryState } from '../../Chat/hooks/useChannelsQueryState'; +import { DEFAULT_INITIAL_CHANNEL_PAGE_SIZE } from '../../../constants/limits'; const RECOVER_LOADED_CHANNELS_THROTTLE_INTERVAL_IN_MS = 5000; const MIN_RECOVER_LOADED_CHANNELS_THROTTLE_INTERVAL_IN_MS = 2000; @@ -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, offset, ...options, }; diff --git a/src/components/InfiniteScrollPaginator/InfiniteScroll.tsx b/src/components/InfiniteScrollPaginator/InfiniteScroll.tsx index 156c4f1e9..c8941fb25 100644 --- a/src/components/InfiniteScrollPaginator/InfiniteScroll.tsx +++ b/src/components/InfiniteScrollPaginator/InfiniteScroll.tsx @@ -1,4 +1,4 @@ -import React, { PropsWithChildren, useCallback, useEffect, useLayoutEffect, useRef } from 'react'; +import React, { PropsWithChildren, useEffect, useLayoutEffect, useRef } from 'react'; import type { PaginatorProps } from '../../types/types'; import { deprecationAndReplacementWarning } from '../../utils/deprecationWarning'; @@ -73,7 +73,8 @@ export const InfiniteScroll = (props: PropsWithChildren) => const scrollComponent = useRef(); - const scrollListener = useCallback(() => { + const scrollListenerRef = useRef<() => void>(); + scrollListenerRef.current = () => { const element = scrollComponent.current; if (!element || element.offsetParent === null) { @@ -103,15 +104,7 @@ export const InfiniteScroll = (props: PropsWithChildren) => if (offset < Number(threshold) && typeof loadNextPageFn === 'function' && hasNextPageFlag) { loadNextPageFn(); } - }, [ - hasPreviousPageFlag, - hasNextPageFlag, - isLoading, - listenToScroll, - loadPreviousPageFn, - loadNextPageFn, - threshold, - ]); + }; useEffect(() => { deprecationAndReplacementWarning( @@ -130,14 +123,17 @@ export const InfiniteScroll = (props: PropsWithChildren) => const scrollElement = scrollComponent.current?.parentNode; if (!scrollElement) return; + const scrollListener = () => scrollListenerRef.current?.(); + scrollElement.addEventListener('scroll', scrollListener, useCapture); scrollElement.addEventListener('resize', scrollListener, useCapture); + scrollListener(); return () => { scrollElement.removeEventListener('scroll', scrollListener, useCapture); scrollElement.removeEventListener('resize', scrollListener, useCapture); }; - }, [initialLoad, scrollListener, useCapture]); + }, [initialLoad, useCapture]); useEffect(() => { const scrollElement = scrollComponent.current?.parentNode; diff --git a/src/components/MessageList/MessageList.tsx b/src/components/MessageList/MessageList.tsx index 047fd3390..34fc34dc7 100644 --- a/src/components/MessageList/MessageList.tsx +++ b/src/components/MessageList/MessageList.tsx @@ -38,6 +38,7 @@ import type { MessageProps } from '../Message/types'; import type { StreamMessage } from '../../context/ChannelStateContext'; import type { DefaultStreamChatGenerics } from '../../types/types'; +import { DEFAULT_NEXT_CHANNEL_PAGE_SIZE } from '../../constants/limits'; type MessageListWithContextProps< StreamChatGenerics extends DefaultStreamChatGenerics = DefaultStreamChatGenerics @@ -68,7 +69,7 @@ const MessageListWithContext = < headerPosition, read, renderMessages = defaultRenderMessages, - messageLimit = 100, + messageLimit = DEFAULT_NEXT_CHANNEL_PAGE_SIZE, loadMore: loadMoreCallback, loadMoreNewer: loadMoreNewerCallback, hasMoreNewer = false, diff --git a/src/components/MessageList/VirtualizedMessageList.tsx b/src/components/MessageList/VirtualizedMessageList.tsx index 5b8a9deae..acc046b2b 100644 --- a/src/components/MessageList/VirtualizedMessageList.tsx +++ b/src/components/MessageList/VirtualizedMessageList.tsx @@ -53,6 +53,7 @@ import { ComponentContextValue, useComponentContext } from '../../context/Compon import type { Channel, ChannelState as StreamChannelState, UserResponse } from 'stream-chat'; import type { DefaultStreamChatGenerics, UnknownType } from '../../types/types'; +import { DEFAULT_NEXT_CHANNEL_PAGE_SIZE } from '../../constants/limits'; type VirtualizedMessageListPropsForContext = | 'additionalMessageInputProps' @@ -184,7 +185,7 @@ const VirtualizedMessageListWithContext = < loadMoreNewer, Message: MessageUIComponentFromProps, messageActions, - messageLimit = 100, + messageLimit = DEFAULT_NEXT_CHANNEL_PAGE_SIZE, messages, notifications, // TODO: refactor to scrollSeekPlaceHolderConfiguration and components.ScrollSeekPlaceholder, like the Virtuoso Component @@ -386,7 +387,9 @@ const VirtualizedMessageListWithContext = < } }; const atTopStateChange = (isAtTop: boolean) => { - if (isAtTop) loadMore?.(messageLimit); + if (isAtTop) { + loadMore?.(messageLimit); + } }; useEffect(() => { diff --git a/yarn.lock b/yarn.lock index c564a5c71..3a76886d1 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2497,6 +2497,13 @@ dependencies: "@types/lodash" "*" +"@types/lodash.defaultsdeep@^4.6.9": + version "4.6.9" + resolved "https://registry.yarnpkg.com/@types/lodash.defaultsdeep/-/lodash.defaultsdeep-4.6.9.tgz#050fbe389a7a6e245b15da9ee83a8a62f047a1c4" + integrity sha512-pLtCFK0YkHfGtGLYLNMTbFB5/G5+RsmQCIbbHH8GOAXjv+gDkVilY98kILfe8JH2Kev0OCReYxp1AjxEjP8ixA== + dependencies: + "@types/lodash" "*" + "@types/lodash.throttle@^4.1.7": version "4.1.7" resolved "https://registry.yarnpkg.com/@types/lodash.throttle/-/lodash.throttle-4.1.7.tgz#4ef379eb4f778068022310ef166625f420b6ba58" @@ -9391,6 +9398,11 @@ lodash.deburr@^4.1.0: resolved "https://registry.yarnpkg.com/lodash.deburr/-/lodash.deburr-4.1.0.tgz#ddb1bbb3ef07458c0177ba07de14422cb033ff9b" integrity sha1-3bG7s+8HRYwBd7oH3hRCLLAz/5s= +lodash.defaultsdeep@^4.6.1: + version "4.6.1" + resolved "https://registry.yarnpkg.com/lodash.defaultsdeep/-/lodash.defaultsdeep-4.6.1.tgz#512e9bd721d272d94e3d3a63653fa17516741ca6" + integrity sha512-3j8wdDzYuWO3lM3Reg03MuQR957t287Rpcxp1njpEa8oDrikb+FwGdW3n+FELh/A6qib6yPit0j/pv9G/yeAqA== + lodash.escaperegexp@^4.1.2: version "4.1.2" resolved "https://registry.yarnpkg.com/lodash.escaperegexp/-/lodash.escaperegexp-4.1.2.tgz#64762c48618082518ac3df4ccf5d5886dae20347"