[CIS-784] Fix channel unread count calculations for new messages #969
Conversation
It's calculated using two fetch requests so it can get quite expensive.
It was there by accident. `MessageReadEvent` always resets the unread count to 0
| @@ -164,7 +165,7 @@ public struct _ChatChannel<ExtraData: ExtraDataTypes> { | |||
| currentlyTypingMembers: Set<_ChatChannelMember<ExtraData.User>> = [], | |||
| lastActiveWatchers: @escaping (() -> [_ChatUser<ExtraData.User>]) = { [] }, | |||
| team: TeamId? = nil, | |||
| unreadCount: ChannelUnreadCount = .noUnread, | |||
| unreadCount: @escaping () -> ChannelUnreadCount = { .noUnread }, | |||
There was a problem hiding this comment.
so unreadCount is not a value but a dynamic thingy now.. This makes us move further away from "snapshot models". Since we send channelUpdated delegate callbacks when unread count updates, why don't we still use static unread counts in models?
There was a problem hiding this comment.
Nope, this just means it's computed lazily. Getting unread count requires iterating over channel reads + an additional DB query. That's why it's computed lazily. See other @Cached properties in our models.
There was a problem hiding this comment.
OK, explanation + re-reading code made it clearer. Makes sense 👌
| userId = try response.value(at: \.user?.id) | ||
| cid = try response.value(at: \.cid) | ||
| readAt = try response.value(at: \.createdAt) | ||
| unreadCount = try response.value(at: \.unreadCount) |
There was a problem hiding this comment.
I'm confused, backend doesn't send this info anymore, or it never sent this, or this is not useful?
There was a problem hiding this comment.
This was actually a complete misunderstanding of how MessageReadEvent works :) The unread count is always zero hence is not sent. This unread count is the total unread count and is sent only for the current user -> see the PR description for more info.
There was a problem hiding this comment.
Shouldn't it be optional and useful when available? If it exists, we can use it to update current user's unread count
| userId = try response.value(at: \.message?.user.id) | ||
| cid = try response.value(at: \.channel?.cid) | ||
| messageId = try response.value(at: \.message?.id) | ||
| createdAt = try response.value(at: \.message?.createdAt) | ||
| unreadCount = try response.value(at: \.unreadCount) | ||
| unreadCount = try? response.value(at: \.unreadCount) |
There was a problem hiding this comment.
In which cases backend doesn't send this value? would be nice to comment this
There was a problem hiding this comment.
There are several conditions. See https://getstream.slack.com/archives/CE5N802GP/p1617740759146000 for more info. This whole thing about unread counts is still not ideal and will need a rethink.
Codecov Report
@@ Coverage Diff @@
## main #969 +/- ##
==========================================
- Coverage 89.44% 89.41% -0.04%
==========================================
Files 203 203
Lines 8330 8371 +41
==========================================
+ Hits 7451 7485 +34
- Misses 879 886 +7
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
This PR fixes a lot of small issues which together caused very strange behavior of channel unread count number.
The most important changes:
ChannelReadDTOnow triggerChannelDTOupdated callbacksMessageReadEventincorrectly had a mandatoryunreadCountparameter which made it impossible to parse and handle for other than the current user.ChannelReadUpdaterMiddlewarenow increases the read unread count for new incoming messages.unread_count.mov