Fix thread staying unread after a non-counting event follows your reply

apps/web/src/Unread.test.ts

@@ -12,7 +12,7 @@ import { describe, it, expect, beforeEach, beforeAll, vi } from "vitest";
 import { MatrixEvent, EventType, MsgType, Room, ReceiptType } from "matrix-js-sdk/src/matrix";
 import { logger } from "matrix-js-sdk/src/logger";
 import { makeBeaconEvent, mkEvent, stubClient } from "test-utils";
-import { makeThreadEvents, mkThread, populateThread } from "test-utils/threads";
+import { makeThreadEvent, makeThreadEvents, mkThread, populateThread } from "test-utils/threads";
 
 import { haveRendererForEvent } from "./events/EventTileFactory";
 import {
@@ -164,6 +164,30 @@ describe("Unread", () => {
                 expect(doesRoomHaveUnreadMessages(room, false)).toBe(false);
             });
 
+            it("returns true when another user's message arrives after ours", () => {
+                room.addLiveEvents(
+                    [
+                        mkEvent({
+                            event: true,
+                            type: "m.room.message",
+                            user: myId,
+                            room: roomId,
+                            content: {},
+                        }),
+                        mkEvent({
+                            event: true,
+                            type: "m.room.message",
+                            user: aliceId,
+                            room: roomId,
+                            content: {},
+                        }),
+                    ],
+                    { addToState: true },
+                );
+
+                expect(doesRoomHaveUnreadMessages(room, false)).toBe(true);
+            });
+
             it("returns false for a room when the read receipt is at the latest event", () => {
                 const receipt = new MatrixEvent({
                     type: "m.receipt",
@@ -575,6 +599,56 @@ describe("Unread", () => {
             expect(doesRoomHaveUnreadThreads(room)).toBe(true);
         });
 
+        it("returns false when we replied and a non-counting event landed after our reply", async () => {
+            // Thread: root(alice) -> reply(me) -> redacted(alice). Our reply is newer than the
+            // only unread-triggering event, so we have clearly seen the thread, but we are no
+            // longer the sender of the literal last event.
+            const { rootEvent, events } = await populateThread({
+                room,
+                client,
+                authorId: aliceId,
+                participantUserIds: [myId],
+            });
+
+            const trailing = makeThreadEvent({
+                event: true,
+                user: aliceId,
+                room: roomId,
+                msg: "redacted",
+                rootEventId: rootEvent.getId()!,
+                replyToEventId: events.at(-1)!.getId()!,
+                ts: 100,
+            });
+            vi.spyOn(trailing, "isRedacted").mockReturnValue(true);
+            await room.addLiveEvents([trailing], { addToState: false });
+
+            expect(doesRoomHaveUnreadThreads(room)).toBe(false);
+        });
+
+        it("returns true when an incoming message landed after our reply", async () => {
+            // Thread: root(alice) -> reply(me) -> message(alice). The trailing message does
+            // trigger an unread count, so the thread is genuinely unread.
+            const { rootEvent, events } = await populateThread({
+                room,
+                client,
+                authorId: aliceId,
+                participantUserIds: [myId],
+            });
+
+            const trailing = makeThreadEvent({
+                event: true,
+                user: aliceId,
+                room: roomId,
+                msg: "a real reply",
+                rootEventId: rootEvent.getId()!,
+                replyToEventId: events.at(-1)!.getId()!,
+                ts: 100,
+            });
+            await room.addLiveEvents([trailing], { addToState: false });
+
+            expect(doesRoomHaveUnreadThreads(room)).toBe(true);
+        });
+
         it("return false when we have a receipt for the thread", async () => {
             const { events, rootEvent } = await populateThread({
                 room,

apps/web/src/Unread.ts

@@ -71,6 +71,12 @@ function doesTimelineHaveUnreadMessages(room: Room, timeline: Array<MatrixEvent>
     const myUserId = room.client.getSafeUserId();
     const latestImportantEventId = findLatestImportantEvent(room.client, timeline)?.getId();
     if (latestImportantEventId) {
+        // Sending a message means we have seen everything before it. The js-sdk applies this
+        // shortcut only when our event is the *literal* last one in the timeline, so any event
+        // that doesn't trigger an unread count (a reaction, an edit, a membership change...)
+        // landing after our message would otherwise make the timeline unread again.
+        if (userSentEventAfterLatestImportantEvent(room.client, timeline)) return false;
+
         return !room.hasUserReadEvent(myUserId, latestImportantEventId);
     } else {
         // We couldn't find an important event to check - check the unimportant ones.
@@ -141,6 +147,25 @@ function findLatestImportantEvent(client: MatrixClient, timeline: Array<MatrixEv
     return null;
 }
 
+/**
+ * Look backwards through the timeline for whichever comes first: an event we sent, or an
+ * important event. An event we sent is never important (see {@link eventTriggersUnreadCount}),
+ * so finding ours first means it is newer than every important event in the timeline.
+ *
+ * @returns true if we sent an event after the latest important event
+ */
+function userSentEventAfterLatestImportantEvent(client: MatrixClient, timeline: Array<MatrixEvent>): boolean {
+    const myUserId = client.getSafeUserId();
+    for (let index = timeline.length - 1; index >= 0; index--) {
+        const event = timeline[index];
+        if (isImportantEvent(client, event)) return false;
+        // Ignore local echoes that haven't made it to the server yet: they carry no timeline
+        // position, and a failed send shouldn't mark anything as read.
+        if (event.getSender() === myUserId && !event.status) return true;
+    }
+    return false;
+}
+
 /**
  * Given this event does not have a receipt, is it important enough to make
  * this room unread?