Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Commit

Permalink
Rewrite doesRoomOrThreadHaveUnreadMessages to use the receipt rewrite…
Browse files Browse the repository at this point in the history 
… from js-sdk
  • Loading branch information
@andybalaam
andybalaam committed Nov 20, 2023
1 parent 29ed917 commit 3f806a8
Show file tree
Hide file tree
Showing 6 changed files with 216 additions and 166 deletions.
38 changes: 17 additions & 21 deletions cypress/e2e/read-receipts/editing-messages.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"]);
Expand All @@ -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", () => {
Expand Down Expand Up @@ -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")]);
Expand All @@ -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")]);
Expand All @@ -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);
Expand All @@ -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", () => {
Expand All @@ -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);

Expand Down Expand Up @@ -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")]);
Expand All @@ -402,19 +398,19 @@ 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
assertStillRead(room2);
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")]);
Expand All @@ -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
Expand Down
2 changes: 1 addition & 1 deletion cypress/e2e/read-receipts/high-level.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
11 changes: 5 additions & 6 deletions cypress/e2e/read-receipts/new-messages.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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")]);
Expand All @@ -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")]);
Expand All @@ -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")]);
Expand Down
133 changes: 47 additions & 86 deletions src/Unread.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<MatrixEvent>): 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>): 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);
}
Loading

0 comments on commit 3f806a8

Please sign in to comment.