channels: more accurate unread counts #3119
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
Instead of re-deriving recency based on the unread threads. That re-deriving was causing inaccurate results for whatever reason, and was 100% going to be slower than just reading the recency. Fixes LAND-1393.
In addition to providing a total "count", we now also provide an individual count for top-level "unread", as well as each "threads" entry. This way, clients may display accurate unread counts for each context. Partially fixes LAND-1394, frontend changes still pending.
Otherwise we run a small risk of ordering shenanigans.
be58f62 updated the unread types for channels to provide per-context counts. Here, we do the same for chats.
We now have access to per-context unread counts. We make sure to include those in the DateDividers.
After review, we have concluded one of these functions can go entirely (it is unused), and the TODO for the other is invalid: the response still depends on ordering of the respective unreads.
…the main chat, not just the oldest one
…s rather than relying on derrived values
midsum-salrux
approved these changes
Dec 14, 2023
| (lot:on-v-replies:c replies.u.u.parent `last-read.remark.channel ~) | ||
| |= [tim=time reply=(unit v-reply:c)] | ||
| ?& ?=(^ reply) | ||
| !=(author.u.reply our.bowl) | ||
| == | ||
| :- (add sum (lent unreads)) | ||
| =/ count=@ud (lent unreads) |
There was a problem hiding this comment.
NB: I don't like reusing the name here but it was already doing that for unreads.
|
Actually the top-level count (not the total) provided by this is inaccurate for cases where both a top-level message with replies is below the top-level marker: in that case the replies should be included in the top-level count, but aren't. Fixing... Edit: Apparently the current behavior is desired. Not fixing! |
…, rename unread method to make its purpose clearer and handle empty unreads properly without relying on separate checks at callee site
…when the component unmounts instead of any time the dependencies update
… an instance of simplified code we may want to revisit
latter-bolden
approved these changes
Dec 15, 2023
|
Label |
arthyn
approved these changes
Dec 19, 2023
Comment on lines
+352
to
353
| // TODO: chesterton's fence, but why execute a read here? | ||
| useChatStore.getState().read(whom); |
There was a problem hiding this comment.
holdover from bad behavior during rewrite
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.
The improvement here is two-fold:
First, we no longer recalculate the "most recent content" timestamp when marking a channel as read. Instead, we simply reuse the
.recencywhich we were already tracking anyway. I don't remember why we thought we had to re-derive that on the spot. (2928b4b)Second, we update the
$unreadtype to provide, in addition to the total unread count, sub-counts for each context in which unreads may exist: top-level and within reply threads. (be58f62)Draft because this does not yet include the frontend updates required for the latter change. cc @patosullivan
For LAND-1393 and LAND-1394.
Fixes LAND-1412