Fix thread staying unread after a non-counting event follows your reply - #34905
Open
nathanael-h wants to merge 1 commit into
Open
Fix thread staying unread after a non-counting event follows your reply#34905nathanael-h wants to merge 1 commit into
nathanael-h wants to merge 1 commit into
Conversation
`doesTimelineHaveUnreadMessages` judges the timeline against the latest *important* event, which is the newest event from someone else, since our own events never trigger an unread count. Whether that event counts as read then rests on the js-sdk's shortcut, which only fires when we sent the literal last event in the timeline (`userSentLatestEventInThread`). So replying in a thread and then receiving anything that doesn't trigger an unread count — a reaction, an edit, a redaction, a membership change — leaves the thread unread even though we have demonstrably read it. The main timeline is shielded from this by the synthetic read receipt the js-sdk records for our own events; threads have no equivalent, so this is where it surfaces. Treat the timeline as read when our own latest event is newer than the latest important event, rather than requiring it to be the very last one. Fixes element-hq#34904
This file contains hidden or 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
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.
Fixes #34904
Problem
doesTimelineHaveUnreadMessagesdecides whether a timeline is unread by finding the latest important event and asking whether we have read it:"Important" filters through
eventTriggersUnreadCount, which returnsfalsefor our own events — so the latest important event is always the newest event from someone else, never our reply. Whether that event counts as read then comes down to the js-sdk shortcut inRoomReceipts.hasUserReadEvent, which only fires when we sent the literal last event in the timeline:So: you reply in a thread, then anything that doesn't trigger an unread count arrives after your reply — a reaction, an edit, a redaction, a membership change — and the thread goes back to unread, judged against a message you had already read before replying.
The main timeline is shielded from this, because
Room.addLiveEventsynthesizes a read receipt for every event's sender, including ours. Threads have no equivalent, which is where the bug actually surfaces (and why the new test lives in thedoesRoomHaveUnreadThreads()block — an equivalent main-timeline test passes with or without this change).Change
Ask the right question — "is our latest event newer than the latest important event?" instead of "is our event the very last one?":
The helper walks the timeline backwards and returns on whichever comes first, one of our events or an important event. Since our own events are never important, there's no ordering ambiguity. Pending/failed local echoes are skipped (
!event.status), so a send that hasn't landed doesn't mark anything read.Tests
Two tests in
doesRoomHaveUnreadThreads():root(alice) -> reply(me) -> redacted(alice)is read — fails ondevelopwithexpected true to be false, passes with this change;root(alice) -> reply(me) -> message(alice)is still unread, so a genuinely newer incoming message isn't swallowed.Plus a main-timeline test that another user's message arriving after ours still reads as unread.
oxlint,oxfmt --checkandtsc --noEmitare clean for the changed files.Context
Found while working on #32851 (Threads Activity Centre cross-room threads panel), which currently carries a local guard in
useUnreadThreadRooms.tsto suppress the false positive. @florianduros asked in review that the helper be fixed rather than worked around — this is that fix; the TAC guard will be dropped once this lands.Checklist
public/exportedsymbols have accurate TSDoc documentation.