-
Notifications
You must be signed in to change notification settings - Fork 77
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
Conversation
Size Change: +9.77 kB (+2.56%) Total Size: 392 kB
|
"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. |
There was a problem hiding this 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?
src/types.ts
Outdated
isCurrent: boolean; | ||
isLatest: boolean; | ||
messages: FormatMessageResponse<StreamChatGenerics>[]; | ||
pagination: { hasNext?: boolean; hasPrev?: boolean }; |
There was a problem hiding this comment.
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 loadfalse
means there's definitely nothing else to loadundefined
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 }
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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 :)).
There was a problem hiding this comment.
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.
3324d44
to
f9431b2
Compare
f2f46bf
to
a6d9a10
Compare
369f108
to
9dfea3e
Compare
@@ -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 = |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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 = { |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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.
Description of the changes, What, Why and How?
stream-chat
) performs redundant channel queries to retrieve more non-existent messages.client.queryChannels()
call, the SDK's flag determining if there are more messages to load (hasMore
) is optimistically set totrue
. This breaks customer UIs, which would like to display components based on the value ofhasMore
when it is false in channels with few messages (show component ifhasMore
isfalse
).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:
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?