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

feat: add channel messages pagination indicators #1332

Merged
merged 12 commits into from
Aug 22, 2024

Conversation

MartinCupela
Copy link
Contributor

@MartinCupela MartinCupela commented Aug 6, 2024

Description of the changes, What, Why and How?

  1. The React SDK (and probably other SDKs using stream-chat) performs redundant channel queries to retrieve more non-existent messages.
  2. Also, as the SDK components do not know, whether there are more messages to be loaded after the initial client.queryChannels() call, the SDK's flag determining if there are more messages to load (hasMore) is optimistically set to true. This breaks customer UIs, which would like to display components based on the value of hasMore when it is false in channels with few messages (show component if hasMore is false).

It makes sense to keep the pagination information in the client instead of the component SDK as the client performs the queries and knows the query options.

The pagination information has to be stored for every message set separately. The pagination is determined separately for created_at_around, id_around and the rest of the query params. If the query options object contains multiple params, then the pagination is determined as if there was only one of them in the following order: 1. created_at_around, 2. id_around and 3. the rest of the query params.

Usage in components SDK:

const hasMore = channel.state.messagesPagination.hasPrev;
const hasMoreNewer = channel.state.messagesPagination.hasNext;

Questions I could not answer

Why the thread messages are not stored in message sets as it is done for channel messages even though they are queried with MessagePaginationOptions? Probably jumping to a reply is not possible?
Why pinned messages are not stored in message sets as it is done for channel messages even though they are queried with PinnedMessagePaginationOptions? Probably jumping to a pinned message is not possible?

Copy link
Contributor

github-actions bot commented Aug 6, 2024

Size Change: +9.77 kB (+2.56%)

Total Size: 392 kB

Filename Size Change
dist/browser.es.js 84.8 kB +2.18 kB (+2.63%)
dist/browser.full-bundle.min.js 50.3 kB +1.1 kB (+2.23%)
dist/browser.js 85.8 kB +2.17 kB (+2.59%)
dist/index.es.js 84.8 kB +2.17 kB (+2.63%)
dist/index.js 85.9 kB +2.16 kB (+2.58%)

compressed-size-action

@szuperaz
Copy link
Contributor

szuperaz commented Aug 7, 2024

"Why the thread messages are not stored in message sets as it is done for channel messages even though they are queried with MessagePaginationOptions? Probably jumping to a reply is not possible?" -> Thread replies should be stored in message sets, this is a bug. You can jump to thread replies, we have a public method for this, jumping will work fine, but due to the lack of message sets scrolling won't work in a lot of cases. This was an oversight from me when I implemented this years ago, I'm pretty sure I had this reported somewhere before, but I couldn't find it, so created a new issue for it: https://stream-io.atlassian.net/jira/software/c/projects/PBE/boards/168?selectedIssue=PBE-5451

"Why pinned messages are not stored in message sets as it is done for channel messages even though they are queried with PinnedMessagePaginationOptions? Probably jumping to a pinned message is not possible?" -> There are two things here: jumping to a pinned message is possible, it's works just like jumping to any channel/thread message. The message set would be necessary if someone were to build a message list that only contains pinned messages, and want to jump inside that list, that is not possible, nor do I think we ever wanted to support that, so it's fine that pinned messages don't have a message sets.

szuperaz
szuperaz previously approved these changes Aug 7, 2024
oliverlaz
oliverlaz previously approved these changes Aug 7, 2024
Copy link
Member

@oliverlaz oliverlaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good from my side as well.
One nitpick I have is, that I believe the prettier settings were not picked up by your IDE and the formatting is a bit non-standard. Can you please fix this?

@MartinCupela MartinCupela dismissed stale reviews from oliverlaz and szuperaz via a7e63d7 August 7, 2024 09:08
src/types.ts Outdated
isCurrent: boolean;
isLatest: boolean;
messages: FormatMessageResponse<StreamChatGenerics>[];
pagination: { hasNext?: boolean; hasPrev?: boolean };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand the semantics correctly:

  • true means there's maybe more to load
  • false means there's definitely nothing else to load
  • undefined means nothing has been queried yet

Because both true and undefined mean "try querying", an easy mistake to make will be writing if (messages.pagination.hasNext) { ... }. Maybe we can make this type a bit more verbose:

pagination: { maybeHasNext: boolean; maybeHasPrev: boolean }

And always initialize with { maybeHasNext: true; maybeHasPrev: true }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, and just as a suggestion, hasOlder and hasNewer are probably more accurate, right? Because next and prev are subjective in this case :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to keep it true. But I would keep maybe out of the name as it reveals our internals that can change in the future (maybe once this will be fixed on the BE side and each response will provide information about the next page availability :)).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hasOlder - I am not sure whether the messages have to be always ordered by creation date. Also if we wanted to paginate other types of items, they may not be sorted by date - that is why I think it does not give us added value and the integrators should know, how they sort their items.

@MartinCupela MartinCupela force-pushed the feat/messages-pagination-indicators branch from 3324d44 to f9431b2 Compare August 7, 2024 14:22
myandrienko
myandrienko previously approved these changes Aug 7, 2024
@MartinCupela MartinCupela force-pushed the feat/messages-pagination-indicators branch from f2f46bf to a6d9a10 Compare August 19, 2024 13:53
@MartinCupela MartinCupela force-pushed the feat/messages-pagination-indicators branch from 369f108 to 9dfea3e Compare August 20, 2024 09:55
@@ -846,6 +854,14 @@ export class ChannelState<StreamChatGenerics extends ExtendableGenerics = Defaul
sources.forEach((messageSet) => {
target.isLatest = target.isLatest || messageSet.isLatest;
target.isCurrent = target.isCurrent || messageSet.isCurrent;
target.pagination.hasPrev =
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merging pagination objects when merging message sets.

@@ -1601,7 +1603,7 @@ export class StreamChat<StreamChatGenerics extends ExtendableGenerics = DefaultG
},
});

return this.hydrateActiveChannels(data.channels, stateOptions);
return this.hydrateActiveChannels(data.channels, stateOptions, options);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spaghetti code architecture forced me to do this.

}

if (updatedMessagesSet) {
updatedMessagesSet.pagination = {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pagination indicators are calculated once the requested page is merged into the parent message set.

return params.parentSet.pagination;
}

if (params.messagePaginationOptions?.created_at_around) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the query options object contains multiple params, then the pagination is determined as if there was only one of them in the following order: 1. created_at_around, 2. id_around and 3. the rest of the query params.

src/utils.ts Outdated Show resolved Hide resolved
oliverlaz
oliverlaz previously approved these changes Aug 22, 2024
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.

4 participants