From 3f806a8265d6d9030a7da046321d665ab3d2e9db Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Fri, 17 Nov 2023 17:05:37 +0000 Subject: [PATCH 01/11] Rewrite doesRoomOrThreadHaveUnreadMessages to use the receipt rewrite from js-sdk --- .../read-receipts/editing-messages.spec.ts | 38 ++-- cypress/e2e/read-receipts/high-level.spec.ts | 2 +- .../e2e/read-receipts/new-messages.spec.ts | 11 +- src/Unread.ts | 133 +++++--------- test/Unread-test.ts | 164 ++++++++++++------ test/test-utils/threads.ts | 34 +++- 6 files changed, 216 insertions(+), 166 deletions(-) diff --git a/cypress/e2e/read-receipts/editing-messages.spec.ts b/cypress/e2e/read-receipts/editing-messages.spec.ts index 04b21acd1a7..7b50e435ab8 100644 --- a/cypress/e2e/read-receipts/editing-messages.spec.ts +++ b/cypress/e2e/read-receipts/editing-messages.spec.ts @@ -134,7 +134,7 @@ describe("Read receipts", () => { goTo(room1); assertStillRead(room2); }); - it("Editing a message after marking as read makes the room unread", () => { + it("Editing a message after marking as read leaves the room read", () => { // Given the room is marked as read goTo(room1); receiveMessages(room2, ["Msg1"]); @@ -145,7 +145,7 @@ describe("Read receipts", () => { // When a message is edited receiveMessages(room2, [editOf("Msg1", "Msg1 Edit1")]); - // Then the room remains unread + // Then the room remains read assertStillRead(room2); }); it("Editing a reply after reading it makes the room unread", () => { @@ -264,8 +264,7 @@ describe("Read receipts", () => { assertStillRead(room2); assertReadThread("Msg1"); }); - // XXX: fails because the unread dot remains after marking as read - it.skip("Marking a room as read after an edit in a thread makes it read", () => { + it("Marking a room as read after an edit in a thread makes it read", () => { // Given an edit in a thread is making the room unread goTo(room1); receiveMessages(room2, ["Msg1", threadedOff("Msg1", "Resp1"), editOf("Resp1", "Edit1")]); @@ -277,8 +276,7 @@ describe("Read receipts", () => { // Then it is read assertRead(room2); }); - // XXX: fails because the unread dot remains after marking as read - it.skip("Editing a thread message after marking as read leaves the room read", () => { + it("Editing a thread message after marking as read leaves the room read", () => { // Given a room is marked as read goTo(room1); receiveMessages(room2, ["Msg1", threadedOff("Msg1", "Resp1")]); @@ -292,9 +290,8 @@ describe("Read receipts", () => { // Then the room becomes unread assertStillRead(room2); }); - // XXX: fails because we see a dot instead of an unread number - probably the server and client disagree - it.skip("A room with an edited threaded message is still read after restart", () => { - // Given an edit in a thread is making a room unread + it("A room with an edited threaded message is still read after restart", () => { + // Given an edit in a thread is leaving a room read goTo(room1); receiveMessages(room2, ["Msg1", threadedOff("Msg1", "Resp1")]); markAsRead(room2); @@ -304,7 +301,7 @@ describe("Read receipts", () => { // When I restart saveAndReload(); - // Then is it still unread + // Then is it still read assertRead(room2); }); it("A room where all threaded edits are read is still read after restart", () => { @@ -319,11 +316,11 @@ describe("Read receipts", () => { saveAndReload(); assertRead(room2); }); - // XXX: fails because we see a dot instead of an unread number - probably the server and client disagree + // XXX: fails because the room becomes unread after restart it.skip("A room where all threaded edits are marked as read is still read after restart", () => { goTo(room1); receiveMessages(room2, ["Msg1", threadedOff("Msg1", "Resp1"), editOf("Resp1", "Edit1")]); - assertUnread(room2, 3); + assertUnread(room2, 2); markAsRead(room2); assertRead(room2); @@ -392,8 +389,7 @@ describe("Read receipts", () => { // Then the room stays read assertStillRead(room2); }); - // XXX: fails because the room has an unread dot after I marked it as read - it.skip("Marking a room as read after an edit of a thread root keeps it read", () => { + it("Marking a room as read after an edit of a thread root keeps it read", () => { // Given a fully-read thread exists goTo(room2); receiveMessages(room2, ["Msg1", threadedOff("Msg1", "Resp1")]); @@ -402,10 +398,11 @@ describe("Read receipts", () => { goTo(room1); assertRead(room2); - // When the thread root is edited - receiveMessages(room2, [editOf("Msg1", "Msg1 Edit1")]); + // When the thread root is edited (and I receive another message + // to allow Mark as read) + receiveMessages(room2, [editOf("Msg1", "Msg1 Edit1"), "Msg2"]); - // And I mark the room as read + // And when I mark the room as read markAsRead(room2); // Then the room becomes read and stays read @@ -413,8 +410,7 @@ describe("Read receipts", () => { goTo(room1); assertStillRead(room2); }); - // XXX: fails because the room has an unread dot after I marked it as read - it.skip("Editing a thread root that is a reply after marking as read leaves the room read", () => { + it("Editing a thread root that is a reply after marking as read leaves the room read", () => { // Given a thread based on a reply exists and is read because it is marked as read goTo(room1); receiveMessages(room2, ["Msg", replyTo("Msg", "Reply"), threadedOff("Reply", "InThread")]); @@ -423,14 +419,14 @@ describe("Read receipts", () => { assertRead(room2); // When I edit the thread root - receiveMessages(room1, [editOf("Reply", "Edited Reply")]); + receiveMessages(room2, [editOf("Reply", "Edited Reply")]); // Then the room is read assertStillRead(room2); // And the thread is read goTo(room2); - assertReadThread("EditedReply"); + assertReadThread("Edited Reply"); }); it("Marking a room as read after an edit of a thread root that is a reply leaves it read", () => { // Given a thread based on a reply exists and the reply has been edited diff --git a/cypress/e2e/read-receipts/high-level.spec.ts b/cypress/e2e/read-receipts/high-level.spec.ts index 2c49e1cfc95..aa7489181f5 100644 --- a/cypress/e2e/read-receipts/high-level.spec.ts +++ b/cypress/e2e/read-receipts/high-level.spec.ts @@ -188,7 +188,7 @@ describe("Read receipts", () => { }); describe("Paging up", () => { - // Flaky test https://github.com/vector-im/element-web/issues/26437 + // XXX: Fails because flaky test https://github.com/vector-im/element-web/issues/26437 it.skip("Paging up through old messages after a room is read leaves the room read", () => { // Given lots of messages are in the room, but we have read them goTo(room1); diff --git a/cypress/e2e/read-receipts/new-messages.spec.ts b/cypress/e2e/read-receipts/new-messages.spec.ts index 76ccdbfe542..1cd625811f5 100644 --- a/cypress/e2e/read-receipts/new-messages.spec.ts +++ b/cypress/e2e/read-receipts/new-messages.spec.ts @@ -221,6 +221,8 @@ describe("Read receipts", () => { assertRead(room2); }); // XXX: fails because the room remains unread even though I sent a message + // Note: this test should not re-use the same MatrixClient - it + // should create a new one logged in as the same user. it.skip("Me sending a message from a different client marks room as read", () => { // Given I have unread messages goTo(room1); @@ -345,8 +347,7 @@ describe("Read receipts", () => { // Then thread does appear unread assertUnreadThread("Msg1"); }); - // XXX: fails because the room is still "bold" even though the notification counts all disappear - it.skip("Marking a room with unread threads as read makes it read", () => { + it("Marking a room with unread threads as read makes it read", () => { // Given I have an unread thread goTo(room1); receiveMessages(room2, ["Msg1", threadedOff("Msg1", "Resp1"), threadedOff("Msg1", "Resp2")]); @@ -358,8 +359,7 @@ describe("Read receipts", () => { // Then the room is read assertRead(room2); }); - // XXX: fails for the same reason as "Marking a room with unread threads as read makes it read" - it.skip("Sending a new thread message after marking as read makes it unread", () => { + it("Sending a new thread message after marking as read makes it unread", () => { // Given a thread exists goTo(room1); receiveMessages(room2, ["Msg1", threadedOff("Msg1", "Resp1"), threadedOff("Msg1", "Resp2")]); @@ -374,8 +374,7 @@ describe("Read receipts", () => { // Then the room becomes unread assertUnread(room2, 1); }); - // XXX: fails for the same reason as "Marking a room with unread threads as read makes it read" - it.skip("Sending a new different-thread message after marking as read makes it unread", () => { + it("Sending a new different-thread message after marking as read makes it unread", () => { // Given 2 threads exist, and Thread2 has the latest message in it goTo(room1); receiveMessages(room2, ["Thread1", "Thread2", threadedOff("Thread1", "t1a")]); diff --git a/src/Unread.ts b/src/Unread.ts index 45f4f1fb820..b4ade931790 100644 --- a/src/Unread.ts +++ b/src/Unread.ts @@ -57,110 +57,71 @@ export function doesRoomHaveUnreadMessages(room: Room): boolean { return false; } - for (const timeline of [room, ...room.getThreads()]) { - // If the current timeline has unread messages, we're done. - if (doesRoomOrThreadHaveUnreadMessages(timeline)) { + for (const withTimeline of [room, ...room.getThreads()]) { + if (doesTimelineHaveUnreadMessages(room, withTimeline.timeline)) { + // We found an unread, so the room is unread return true; } } + // If we got here then no timelines were found with unread messages. return false; } -export function doesRoomOrThreadHaveUnreadMessages(roomOrThread: Room | Thread): boolean { - // NOTE: this shares logic with hasUserReadEvent in - // matrix-js-sdk/src/models/read-receipt.ts. They are not combined (yet) - // because hasUserReadEvent is focussed on a single event, and this is - // focussed on the whole room/thread. - - // If there are no messages yet in the timeline then it isn't fully initialised - // and cannot be unread. - if (!roomOrThread || roomOrThread.timeline.length === 0) { - return false; - } - - const myUserId = roomOrThread.client.getSafeUserId(); - - // as we don't send RRs for our own messages, make sure we special case that - // if *we* sent the last message into the room, we consider it not unread! - // Should fix: https://github.com/vector-im/element-web/issues/3263 - // https://github.com/vector-im/element-web/issues/2427 - // ...and possibly some of the others at - // https://github.com/vector-im/element-web/issues/3363 - if (roomOrThread.timeline[roomOrThread.timeline.length - 1]?.getSender() === myUserId) { - return false; - } - - const readUpToId = roomOrThread.getEventReadUpTo(myUserId); - const hasReceipt = makeHasReceipt(roomOrThread, readUpToId, myUserId); - - // Loop through messages, starting with the most recent... - for (let i = roomOrThread.timeline.length - 1; i >= 0; --i) { - const ev = roomOrThread.timeline[i]; - - if (hasReceipt(ev)) { - // If we've read up to this event, there's nothing more recent - // that counts and we can stop looking because the user's read - // this and everything before. +function doesTimelineHaveUnreadMessages(room: Room, timeline: Array): boolean { + const myUserId = room.client.getSafeUserId(); + const latestImportantEventId = findLatestImportantEvent(room.client, timeline)?.getId(); + if (latestImportantEventId) { + return !room.hasUserReadEvent(myUserId, latestImportantEventId); + } else { + // We couldn't find an important event to check - check the unimportant ones. + const earliestUnimportantEventId = timeline.at(0)?.getId(); + if (!earliestUnimportantEventId) { + // There are no events in this timeline - it is uninitialised, so we + // consider it read return false; - } else if (isImportantEvent(roomOrThread.client, ev)) { - // We've found a message that counts before we hit - // the user's read receipt, so this room is definitely unread. + } else if (room.hasUserReadEvent(myUserId, earliestUnimportantEventId)) { + // Some of the unimportant events are read, and there are no + // important ones after them, so we've read everything. + return false; + } else { + // We have events. and none of them are read. We must guess that + // the timeline is unread, because there could be older unread + // important events that we don't have loaded. + logger.warn("Falling back to unread room because of no read receipt or counting message found", { + roomId: room.roomId, + earliestUnimportantEventId: earliestUnimportantEventId, + }); return true; } } - - // If we got here, we didn't find a message was important but didn't find - // the user's read receipt either, so we guess and say that the room is - // unread on the theory that false positives are better than false - // negatives here. - logger.warn("Falling back to unread room because of no read receipt or counting message found", { - roomOrThreadId: roomOrThread.roomId, - readUpToId, - }); - return true; } -/** - * Given this event does not have a receipt, is it important enough to make - * this room unread? - */ -function isImportantEvent(client: MatrixClient, event: MatrixEvent): boolean { - return !shouldHideEvent(event) && eventTriggersUnreadCount(client, event); +export function doesRoomOrThreadHaveUnreadMessages(roomOrThread: Room | Thread): boolean { + const room = roomOrThread instanceof Thread ? roomOrThread.room : roomOrThread; + return doesTimelineHaveUnreadMessages(room, roomOrThread.timeline); } /** - * @returns a function that tells us whether a given event matches our read - * receipt. - * - * We have the ID of an event based on a read receipt. If we can find the - * corresponding event, then it's easy - our returned function just decides - * whether the receipt refers to the event we are asking about. + * Look backwards through the timeline and find the last event that is + * "important" in the sense of isImportantEvent. * - * If we can't find the event, we guess by saying of the receipt's timestamp is - * after this event's timestamp, then it's probably saying this event is read. + * @returns the latest important event, or null if none were found */ -function makeHasReceipt( - roomOrThread: Room | Thread, - readUpToId: string | null, - myUserId: string, -): (event: MatrixEvent) => boolean { - // get the most recent read receipt sent by our account. - // N.B. this is NOT a read marker (RM, aka "read up to marker"), - // despite the name of the method :(( - const readEvent = readUpToId ? roomOrThread.findEventById(readUpToId) : null; - - if (readEvent) { - // If we found an event matching our receipt, then it's easy: this event - // has a receipt if its ID is the same as the one in the receipt. - return (ev) => ev.getId() == readUpToId; +function findLatestImportantEvent(client: MatrixClient, timeline: Array): MatrixEvent | null { + for (let index = timeline.length - 1; index >= 0; index--) { + const event = timeline[index]; + if (isImportantEvent(client, event)) { + return event; + } } + return null; +} - // If we didn't, we have to guess by saying if this event is before the - // receipt's ts, then it we pretend it has a receipt. - const receiptTs = roomOrThread.getReadReceiptForUserId(myUserId)?.data.ts ?? 0; - const unthreadedReceiptTs = roomOrThread.getLastUnthreadedReceiptFor(myUserId)?.ts ?? 0; - // We pick the more recent of the two receipts as the latest - const receiptTimestamp = Math.max(receiptTs, unthreadedReceiptTs); - return (ev) => ev.getTs() < receiptTimestamp; +/** + * Given this event does not have a receipt, is it important enough to make + * this room unread? + */ +function isImportantEvent(client: MatrixClient, event: MatrixEvent): boolean { + return !shouldHideEvent(event) && eventTriggersUnreadCount(client, event); } diff --git a/test/Unread-test.ts b/test/Unread-test.ts index 46221d75d20..b56856fa8ce 100644 --- a/test/Unread-test.ts +++ b/test/Unread-test.ts @@ -20,7 +20,7 @@ import { logger } from "matrix-js-sdk/src/logger"; import { haveRendererForEvent } from "../src/events/EventTileFactory"; import { makeBeaconEvent, mkEvent, stubClient } from "./test-utils"; -import { mkThread } from "./test-utils/threads"; +import { makeThreadEvents, populateThread } from "./test-utils/threads"; import { doesRoomHaveUnreadMessages, doesRoomOrThreadHaveUnreadMessages, @@ -213,7 +213,7 @@ describe("Unread", () => { expect(doesRoomHaveUnreadMessages(room)).toBe(true); }); - it("returns true for a room with an unread message in a thread", () => { + it("returns true for a room with an unread message in a thread", async () => { // Mark the main timeline as read. const receipt = new MatrixEvent({ type: "m.receipt", @@ -229,12 +229,12 @@ describe("Unread", () => { room.addReceipt(receipt); // Create a thread as a different user. - mkThread({ room, client, authorId: myId, participantUserIds: [aliceId] }); + await populateThread({ room, client, authorId: myId, participantUserIds: [aliceId] }); expect(doesRoomHaveUnreadMessages(room)).toBe(true); }); - it("returns false for a room when the latest thread event was sent by the current user", () => { + it("returns false for a room when the latest thread event was sent by the current user", async () => { // Mark the main timeline as read. const receipt = new MatrixEvent({ type: "m.receipt", @@ -250,12 +250,12 @@ describe("Unread", () => { room.addReceipt(receipt); // Create a thread as the current user. - mkThread({ room, client, authorId: myId, participantUserIds: [myId] }); + await populateThread({ room, client, authorId: myId, participantUserIds: [myId] }); expect(doesRoomHaveUnreadMessages(room)).toBe(false); }); - it("returns false for a room with read thread messages", () => { + it("returns false for a room with read thread messages", async () => { // Mark the main timeline as read. let receipt = new MatrixEvent({ type: "m.receipt", @@ -271,7 +271,12 @@ describe("Unread", () => { room.addReceipt(receipt); // Create threads. - const { rootEvent, events } = mkThread({ room, client, authorId: myId, participantUserIds: [aliceId] }); + const { rootEvent, events } = await populateThread({ + room, + client, + authorId: myId, + participantUserIds: [aliceId], + }); // Mark the thread as read. receipt = new MatrixEvent({ @@ -290,7 +295,7 @@ describe("Unread", () => { expect(doesRoomHaveUnreadMessages(room)).toBe(false); }); - it("returns true for a room when read receipt is not on the latest thread messages", () => { + it("returns true for a room when read receipt is not on the latest thread messages", async () => { // Mark the main timeline as read. let receipt = new MatrixEvent({ type: "m.receipt", @@ -306,7 +311,12 @@ describe("Unread", () => { room.addReceipt(receipt); // Create threads. - const { rootEvent, events } = mkThread({ room, client, authorId: myId, participantUserIds: [aliceId] }); + const { rootEvent, events } = await populateThread({ + room, + client, + authorId: myId, + participantUserIds: [aliceId], + }); // Mark the thread as read. receipt = new MatrixEvent({ @@ -325,7 +335,7 @@ describe("Unread", () => { expect(doesRoomHaveUnreadMessages(room)).toBe(true); }); - it("returns false when the event for a thread receipt can't be found, but the receipt ts is late", () => { + it("returns false when the event for a thread receipt can't be found, but the receipt ts is late", async () => { // Given a room that is read let receipt = new MatrixEvent({ type: "m.receipt", @@ -341,7 +351,12 @@ describe("Unread", () => { room.addReceipt(receipt); // And a thread - const { rootEvent, events } = mkThread({ room, client, authorId: myId, participantUserIds: [aliceId] }); + const { rootEvent, events } = await populateThread({ + room, + client, + authorId: myId, + participantUserIds: [aliceId], + }); // When we provide a receipt that points at an unknown event, // but its timestamp is after all events in the thread @@ -366,7 +381,7 @@ describe("Unread", () => { expect(doesRoomHaveUnreadMessages(room)).toBe(false); }); - it("returns true when the event for a thread receipt can't be found, and the receipt ts is early", () => { + it("returns true when the event for a thread receipt can't be found, and the receipt ts is early", async () => { // Given a room that is read let receipt = new MatrixEvent({ type: "m.receipt", @@ -382,7 +397,12 @@ describe("Unread", () => { room.addReceipt(receipt); // And a thread - const { rootEvent, events } = mkThread({ room, client, authorId: myId, participantUserIds: [aliceId] }); + const { rootEvent, events } = await populateThread({ + room, + client, + authorId: myId, + participantUserIds: [aliceId], + }); // When we provide a receipt that points at an unknown event, // but its timestamp is before some of the events in the thread @@ -426,8 +446,8 @@ describe("Unread", () => { expect(logger.warn).toHaveBeenCalledWith( "Falling back to unread room because of no read receipt or counting message found", { - roomOrThreadId: room.roomId, - readUpToId: null, + roomId: room.roomId, + earliestUnimportantEventId: redactedEvent.getId(), }, ); }); @@ -446,59 +466,105 @@ describe("Unread", () => { beforeEach(() => { room = new Room(roomId, client, myId); jest.spyOn(logger, "warn"); - event = mkEvent({ - event: true, - type: "m.room.message", - user: aliceId, - room: roomId, - content: {}, - }); - room.addLiveEvents([event]); // Don't care about the code path of hidden events. mocked(haveRendererForEvent).mockClear().mockReturnValue(true); }); - it("should consider unthreaded read receipts for main timeline", () => { - // Send unthreaded receipt into room pointing at the latest event - room.addReceipt( - new MatrixEvent({ + describe("with a single event on the main timeline", () => { + beforeEach(() => { + event = mkEvent({ + event: true, + type: "m.room.message", + user: aliceId, + room: roomId, + content: {}, + }); + room.addLiveEvents([event]); + }); + + it("an unthreaded receipt for the event makes the room read", () => { + // Send unthreaded receipt into room pointing at the latest event + room.addReceipt( + new MatrixEvent({ + type: "m.receipt", + room_id: "!foo:bar", + content: { + [event.getId()!]: { + [ReceiptType.Read]: { + [myId]: { ts: 1 }, + }, + }, + }, + }), + ); + + expect(doesRoomOrThreadHaveUnreadMessages(room)).toBe(false); + }); + + it("an unthreaded receipt with later timestamp makes a new thread read", async () => { + // Provide an unthreaded read receipt with ts greater than the latest thread event + const receipt = new MatrixEvent({ type: "m.receipt", room_id: "!foo:bar", content: { [event.getId()!]: { [ReceiptType.Read]: { - [myId]: { ts: 1 }, + [myId]: { ts: 10000000000 }, }, }, }, - }), - ); + }); + room.addReceipt(receipt); + + const { thread } = await populateThread({ + room, + client, + authorId: myId, + participantUserIds: [aliceId], + }); - expect(doesRoomOrThreadHaveUnreadMessages(room)).toBe(false); + expect(thread.replyToEvent!.getTs()).toBeLessThan( + receipt.getContent()[event.getId()!][ReceiptType.Read][myId].ts, + ); + expect(doesRoomOrThreadHaveUnreadMessages(thread)).toBe(false); + }); }); - it("should consider unthreaded read receipts for thread timelines", () => { - // Provide an unthreaded read receipt with ts greater than the latest thread event - const receipt = new MatrixEvent({ - type: "m.receipt", - room_id: "!foo:bar", - content: { - [event.getId()!]: { - [ReceiptType.Read]: { - [myId]: { ts: 10000000000 }, - }, - }, - }, + describe("with an event on the main timeline and a later one in a thread", () => { + let threadEvent: MatrixEvent; + + beforeEach(() => { + const { events } = makeThreadEvents({ + roomId: roomId, + authorId: aliceId, + participantUserIds: ["@x:s.co"], + length: 2, + ts: 100, + currentUserId: myId, + }); + room.addLiveEvents(events); + threadEvent = events[1]; }); - room.addReceipt(receipt); - const { thread } = mkThread({ room, client, authorId: myId, participantUserIds: [aliceId] }); + it("an unthreaded receipt for the later threaded event makes the room read", () => { + // Send unthreaded receipt into room pointing at the latest event + room.addReceipt( + new MatrixEvent({ + type: "m.receipt", + room_id: roomId, + content: { + [threadEvent.getId()!]: { + [ReceiptType.Read]: { + [myId]: { ts: 1 }, + }, + }, + }, + }), + ); - expect(thread.replyToEvent!.getTs()).toBeLessThan( - receipt.getContent()[event.getId()!][ReceiptType.Read][myId].ts, - ); - expect(doesRoomOrThreadHaveUnreadMessages(thread)).toBe(false); + expect(doesRoomOrThreadHaveUnreadMessages(room)).toBe(false); + }); }); }); }); diff --git a/test/test-utils/threads.ts b/test/test-utils/threads.ts index 4536ab8b1a1..7f6dd8ac4c1 100644 --- a/test/test-utils/threads.ts +++ b/test/test-utils/threads.ts @@ -111,6 +111,13 @@ type MakeThreadProps = { ts?: number; }; +/** + * Create a thread but don't actually populate it with events - see + * populateThread for what you probably want to do. + * + * Leaving this here in case it is needed by some people, but I (andyb) would + * expect us to move to use populateThread exclusively. + */ export const mkThread = ({ room, client, @@ -135,8 +142,29 @@ export const mkThread = ({ const thread = room.createThread(rootEvent.getId()!, rootEvent, events, true); - // So that we do not have to mock the thread loading - thread.initialEventsFetched = true; - return { thread, rootEvent, events }; }; + +/** + * Create a thread, and make sure the events added to the thread and the room's + * timeline as if they came in via sync. + * + * Note that mkThread doesn't actually add the events properly to the room. + */ +export const populateThread = async ({ + room, + client, + authorId, + participantUserIds, + length = 2, + ts = 1, +}: MakeThreadProps): Promise<{ thread: Thread; rootEvent: MatrixEvent; events: MatrixEvent[] }> => { + const ret = mkThread({ room, client, authorId, participantUserIds, length, ts }); + + // So that we do not have to mock the thread loading, tell the thread + // that it is already loaded, and send the events again to the room + // so they are added to the thread timeline. + ret.thread.initialEventsFetched = true; + await room.addLiveEvents(ret.events); + return ret; +}; From 91cbdc8cf873c9b40dd2e85a463fceb8687e2486 Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Tue, 21 Nov 2023 10:55:38 +0000 Subject: [PATCH 02/11] Remove unit tests that rely on receipt timestamps Previously, if we found a receipt for an unknown event, we would use the receipt timestamp and declare all events before that time to be read. Now, we ignore such "dangling" receipts until we find the event they refer to. This new behaviour is more correct, but does lead to more messages being considered unread. This commit deletes tests that checked for the old behaviour. --- test/Unread-test.ts | 85 ++++++++------------------------------------- 1 file changed, 15 insertions(+), 70 deletions(-) diff --git a/test/Unread-test.ts b/test/Unread-test.ts index b56856fa8ce..562fb078aee 100644 --- a/test/Unread-test.ts +++ b/test/Unread-test.ts @@ -335,53 +335,7 @@ describe("Unread", () => { expect(doesRoomHaveUnreadMessages(room)).toBe(true); }); - it("returns false when the event for a thread receipt can't be found, but the receipt ts is late", async () => { - // Given a room that is read - let receipt = new MatrixEvent({ - type: "m.receipt", - room_id: "!foo:bar", - content: { - [event.getId()!]: { - [ReceiptType.Read]: { - [myId]: { ts: 1 }, - }, - }, - }, - }); - room.addReceipt(receipt); - - // And a thread - const { rootEvent, events } = await populateThread({ - room, - client, - authorId: myId, - participantUserIds: [aliceId], - }); - - // When we provide a receipt that points at an unknown event, - // but its timestamp is after all events in the thread - // - // (This could happen if we mis-filed a reaction into the main - // thread when it should actually have gone into this thread, or - // maybe the event is just not loaded for some reason.) - const receiptTs = Math.max(...events.map((e) => e.getTs())) + 100; - receipt = new MatrixEvent({ - type: "m.receipt", - room_id: "!foo:bar", - content: { - ["UNKNOWN_EVENT_ID"]: { - [ReceiptType.Read]: { - [myId]: { ts: receiptTs, threadId: rootEvent.getId()! }, - }, - }, - }, - }); - room.addReceipt(receipt); - - expect(doesRoomHaveUnreadMessages(room)).toBe(false); - }); - - it("returns true when the event for a thread receipt can't be found, and the receipt ts is early", async () => { + it("returns true when the event for a thread receipt can't be found", async () => { // Given a room that is read let receipt = new MatrixEvent({ type: "m.receipt", @@ -502,32 +456,23 @@ describe("Unread", () => { expect(doesRoomOrThreadHaveUnreadMessages(room)).toBe(false); }); - it("an unthreaded receipt with later timestamp makes a new thread read", async () => { - // Provide an unthreaded read receipt with ts greater than the latest thread event - const receipt = new MatrixEvent({ - type: "m.receipt", - room_id: "!foo:bar", - content: { - [event.getId()!]: { - [ReceiptType.Read]: { - [myId]: { ts: 10000000000 }, + it("a threaded receipt for the event makes the room read", () => { + // Send threaded receipt into room pointing at the latest event + room.addReceipt( + new MatrixEvent({ + type: "m.receipt", + room_id: "!foo:bar", + content: { + [event.getId()!]: { + [ReceiptType.Read]: { + [myId]: { ts: 1, thread_id: "main" }, + }, }, }, - }, - }); - room.addReceipt(receipt); - - const { thread } = await populateThread({ - room, - client, - authorId: myId, - participantUserIds: [aliceId], - }); - - expect(thread.replyToEvent!.getTs()).toBeLessThan( - receipt.getContent()[event.getId()!][ReceiptType.Read][myId].ts, + }), ); - expect(doesRoomOrThreadHaveUnreadMessages(thread)).toBe(false); + + expect(doesRoomOrThreadHaveUnreadMessages(room)).toBe(false); }); }); From baf5d241b3041d11a37a3017f1315c924885ba95 Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Tue, 21 Nov 2023 13:57:07 +0000 Subject: [PATCH 03/11] Check for a missing thread in determineUnreadState --- src/RoomNotifs.ts | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/RoomNotifs.ts b/src/RoomNotifs.ts index 988a3a5c219..aa696f6e29c 100644 --- a/src/RoomNotifs.ts +++ b/src/RoomNotifs.ts @@ -261,9 +261,16 @@ export function determineUnreadState( } // We don't have any notified messages, but we might have unread messages. Let's find out. - let hasUnread: boolean; - if (threadId) hasUnread = doesRoomOrThreadHaveUnreadMessages(room.getThread(threadId)!); - else hasUnread = doesRoomHaveUnreadMessages(room); + let hasUnread = false; + if (threadId) { + const thread = room.getThread(threadId); + if (thread) { + hasUnread = doesRoomOrThreadHaveUnreadMessages(thread); + } + // If the thread does not exist, assume it contains no unreads + } else { + hasUnread = doesRoomHaveUnreadMessages(room); + } return { symbol: null, From f6b8afd4e87b9046a932a3cf1088585d962c6d8f Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Tue, 21 Nov 2023 13:57:48 +0000 Subject: [PATCH 04/11] Fix incorrect way to find room timeline --- src/Unread.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Unread.ts b/src/Unread.ts index b4ade931790..7b2da855dd7 100644 --- a/src/Unread.ts +++ b/src/Unread.ts @@ -99,7 +99,8 @@ function doesTimelineHaveUnreadMessages(room: Room, timeline: Array export function doesRoomOrThreadHaveUnreadMessages(roomOrThread: Room | Thread): boolean { const room = roomOrThread instanceof Thread ? roomOrThread.room : roomOrThread; - return doesTimelineHaveUnreadMessages(room, roomOrThread.timeline); + const events = roomOrThread instanceof Thread ? roomOrThread.timeline : room.getLiveTimeline().getEvents(); + return doesTimelineHaveUnreadMessages(room, events); } /** From 7fd01e785eeb9f58d3148a818c109bbb0a726427 Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Tue, 21 Nov 2023 13:58:22 +0000 Subject: [PATCH 05/11] More realistic test setup to support new receipt code --- test/components/views/rooms/EventTile-test.tsx | 1 + test/components/views/rooms/RoomTile-test.tsx | 1 + 2 files changed, 2 insertions(+) diff --git a/test/components/views/rooms/EventTile-test.tsx b/test/components/views/rooms/EventTile-test.tsx index 92a0aa27aa2..b87ddb6ead6 100644 --- a/test/components/views/rooms/EventTile-test.tsx +++ b/test/components/views/rooms/EventTile-test.tsx @@ -94,6 +94,7 @@ describe("EventTile", () => { room = new Room(ROOM_ID, client, client.getSafeUserId(), { pendingEventOrdering: PendingEventOrdering.Detached, + timelineSupport: true }); jest.spyOn(client, "getRoom").mockReturnValue(room); diff --git a/test/components/views/rooms/RoomTile-test.tsx b/test/components/views/rooms/RoomTile-test.tsx index 4e8e6311954..59965a31296 100644 --- a/test/components/views/rooms/RoomTile-test.tsx +++ b/test/components/views/rooms/RoomTile-test.tsx @@ -378,6 +378,7 @@ describe("RoomTile", () => { { lastReply: () => null, timeline: [], + findEventById: () => {} } as Thread, ]); }); From 8831b3bedbc490f717a7c507973ba03f8655a1db Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Tue, 21 Nov 2023 13:58:44 +0000 Subject: [PATCH 06/11] Update snapshot to expect a room to be unread when there are no receipts --- .../__snapshots__/RoomTile-test.tsx.snap | 30 +++++++------------ 1 file changed, 11 insertions(+), 19 deletions(-) diff --git a/test/components/views/rooms/__snapshots__/RoomTile-test.tsx.snap b/test/components/views/rooms/__snapshots__/RoomTile-test.tsx.snap index 85e5c1c100b..1be3bf2d41e 100644 --- a/test/components/views/rooms/__snapshots__/RoomTile-test.tsx.snap +++ b/test/components/views/rooms/__snapshots__/RoomTile-test.tsx.snap @@ -78,7 +78,7 @@ exports[`RoomTile when message previews are enabled and there is a message in th
@@ -127,23 +127,15 @@ exports[`RoomTile when message previews are enabled and there is a message in th
+