Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve hasUserReadEvent and getUserReadUpTo realibility with threads #3031

Merged
merged 29 commits into from
Jan 11, 2023
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
d1db4db
Add failing test scenario when multiple receipts are in the same event
germain-gg Jan 5, 2023
03968c3
Fix cached read receipts
germain-gg Jan 4, 2023
a776ff8
Improve hasUserReadEvent and getUserReadUpTo realibility with threads
germain-gg Jan 5, 2023
23cd620
Reword code comments and improve readibility
Jan 5, 2023
1f3760f
Optimise code paths
Jan 5, 2023
be7fb61
fix getEventReadUpTo logic with unthreaded receipts
germain-gg Jan 5, 2023
0d4da28
Re-introduce optional chaining
germain-gg Jan 5, 2023
1ccf954
fixes
germain-gg Jan 5, 2023
c24f027
mend
germain-gg Jan 6, 2023
435f485
Add tests for getEventReadUpTo and hasUserReadEvent
germain-gg Jan 6, 2023
3fe9d8a
Merge branch 'develop' into gsouquet/better-getreadupto
germain-gg Jan 6, 2023
2f8328d
Reword code comments and improve readibility
Jan 6, 2023
e634807
Add comments to methods
germain-gg Jan 6, 2023
2d147d4
Make properties private and provide accessors
germain-gg Jan 6, 2023
c9827de
Remove unwanted change
germain-gg Jan 6, 2023
d1ab2de
Improve thread spec
germain-gg Jan 6, 2023
02f5a6c
Explain the unthreaded receipt logic in comments
germain-gg Jan 6, 2023
420d0c3
Merge branch 'develop' into gsouquet/better-getreadupto
Jan 6, 2023
feb8f5e
Apply comments readibility suggestions
Jan 9, 2023
5c5cbbf
Clarify code comments based on PR feedback
germain-gg Jan 9, 2023
0ee16d3
Remove unneeded nullish coalescing check
germain-gg Jan 9, 2023
889c0b4
Merge branch 'develop' into gsouquet/better-getreadupto
Jan 9, 2023
83e0b71
Merge branch 'develop' into gsouquet/better-getreadupto
Jan 10, 2023
253427d
Improve comments wording
Jan 10, 2023
4efb717
Clarify comments
germain-gg Jan 10, 2023
d2935d8
fix tests
germain-gg Jan 10, 2023
1d88910
lint fix
germain-gg Jan 10, 2023
dae36b7
Final comment wording updates
Jan 11, 2023
64ab5a0
Merge branch 'develop' into gsouquet/better-getreadupto
Jan 11, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
187 changes: 179 additions & 8 deletions spec/unit/models/thread.spec.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
Copyright 2022 The Matrix.org Foundation C.I.C.
Copyright 2022 - 2023 The Matrix.org Foundation C.I.C.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
Expand All @@ -19,8 +19,12 @@ import { Room } from "../../../src/models/room";
import { Thread, THREAD_RELATION_TYPE, ThreadEvent } from "../../../src/models/thread";
import { mkThread } from "../../test-utils/thread";
import { TestClient } from "../../TestClient";
import { emitPromise, mkMessage } from "../../test-utils/test-utils";
import { EventStatus } from "../../../src";
import { emitPromise, mkMessage, mock } from "../../test-utils/test-utils";
import { EventStatus, MatrixEvent } from "../../../src";
import { ReceiptType } from "../../../src/@types/read_receipts";
import { getMockClientWithEventEmitter, mockClientMethodsUser } from "../../test-utils/client";
import { ReEmitter } from "../../../src/ReEmitter";
import { Feature, ServerSupport } from "../../../src/feature";

describe("Thread", () => {
describe("constructor", () => {
Expand Down Expand Up @@ -71,17 +75,54 @@ describe("Thread", () => {
});

describe("hasUserReadEvent", () => {
const myUserId = "@bob:example.org";
let myUserId: string;
let client: MatrixClient;
let room: Room;

beforeEach(() => {
const testClient = new TestClient(myUserId, "DEVICE", "ACCESS_TOKEN", undefined, {
timelineSupport: false,
client = getMockClientWithEventEmitter({
...mockClientMethodsUser(),
getRoom: jest.fn().mockImplementation(() => room),
decryptEventIfNeeded: jest.fn().mockResolvedValue(void 0),
supportsExperimentalThreads: jest.fn().mockReturnValue(true),
});
client.reEmitter = mock(ReEmitter, "ReEmitter");
client.canSupport = new Map();
Object.keys(Feature).forEach((feature) => {
client.canSupport.set(feature as Feature, ServerSupport.Stable);
});
client = testClient.client;

myUserId = client.getUserId()!;

room = new Room("123", client, myUserId);

const receipt = new MatrixEvent({
type: "m.receipt",
room_id: "!foo:bar",
content: {
// first threaded receipt
"$event0:localhost": {
[ReceiptType.Read]: {
[client.getUserId()!]: { ts: 100, thread_id: "$threadId:localhost" },
},
},
// last unthreaded receipt
"$event1:localhost": {
[ReceiptType.Read]: {
[client.getUserId()!]: { ts: 200 },
["@alice:example.org"]: { ts: 200 },
},
},
// last threaded receipt
"$event2:localhost": {
[ReceiptType.Read]: {
[client.getUserId()!]: { ts: 300 },
},
},
richvdh marked this conversation as resolved.
Show resolved Hide resolved
},
});
room.addReceipt(receipt);

jest.spyOn(client, "getRoom").mockReturnValue(room);
});

Expand All @@ -96,21 +137,151 @@ describe("Thread", () => {
authorId: myUserId,
participantUserIds: [myUserId],
length: 2,
ts: 201,
richvdh marked this conversation as resolved.
Show resolved Hide resolved
});

expect(thread.hasUserReadEvent(myUserId, events.at(-1)!.getId() ?? "")).toBeTruthy();
});

it("considers other events with no RR as unread", () => {
const { thread, events } = mkThread({
room,
client,
authorId: myUserId,
participantUserIds: [myUserId],
length: 25,
ts: 190,
});

// Before alice's last unthreaded receipt
expect(thread.hasUserReadEvent("@alice:example.org", events.at(1)!.getId() ?? "")).toBeTruthy();

// After alice's last unthreaded receipt
expect(thread.hasUserReadEvent("@alice:example.org", events.at(-1)!.getId() ?? "")).toBeFalsy();
});

it("considers event as read if there's a more recent unthreaded receipt", () => {
const { thread, events } = mkThread({
room,
client,
authorId: myUserId,
participantUserIds: ["@alice:example.org"],
length: 2,
ts: 150, // before the latest unthreaded receipt
});
expect(thread.hasUserReadEvent(client.getUserId()!, events.at(-1)!.getId() ?? "")).toBe(true);
});

expect(thread.hasUserReadEvent("@alice:example.org", events.at(-1)!.getId() ?? "")).toBeFalsy();
it("considers event as unread if there's no more recent unthreaded receipt", () => {
const { thread, events } = mkThread({
room,
client,
authorId: myUserId,
participantUserIds: ["@alice:example.org"],
length: 2,
ts: 1000,
});
expect(thread.hasUserReadEvent(client.getUserId()!, events.at(-1)!.getId() ?? "")).toBe(false);
});
});

describe("getEventReadUpTo", () => {
let myUserId: string;
let client: MatrixClient;
let room: Room;

beforeEach(() => {
client = getMockClientWithEventEmitter({
...mockClientMethodsUser(),
getRoom: jest.fn().mockImplementation(() => room),
decryptEventIfNeeded: jest.fn().mockResolvedValue(void 0),
supportsExperimentalThreads: jest.fn().mockReturnValue(true),
});
client.reEmitter = mock(ReEmitter, "ReEmitter");
client.canSupport = new Map();
Object.keys(Feature).forEach((feature) => {
client.canSupport.set(feature as Feature, ServerSupport.Stable);
});

myUserId = client.getUserId()!;

room = new Room("123", client, myUserId);

jest.spyOn(client, "getRoom").mockReturnValue(room);
});

afterAll(() => {
jest.resetAllMocks();
});

it("uses unthreaded receipt to figure out read up to", () => {
const receipt = new MatrixEvent({
type: "m.receipt",
room_id: "!foo:bar",
content: {
// last unthreaded receipt
"$event1:localhost": {
[ReceiptType.Read]: {
["@alice:example.org"]: { ts: 200 },
},
},
},
});
room.addReceipt(receipt);

const { thread, events } = mkThread({
room,
client,
authorId: myUserId,
participantUserIds: [myUserId],
length: 25,
ts: 190,
});

// The 10th event has been read, as alice's last unthreaded receipt is at ts 200
// and `mkThread` increment every thread response by 1ms.
expect(thread.getEventReadUpTo("@alice:example.org")).toBe(events.at(9)!.getId());
});

it("considers thread created before the first threaded receipt to be read", () => {
const receipt = new MatrixEvent({
type: "m.receipt",
room_id: "!foo:bar",
content: {
// last unthreaded receipt
"$event1:localhost": {
[ReceiptType.Read]: {
[myUserId]: { ts: 200, thread_id: "$threadId" },
},
},
},
});
room.addReceipt(receipt);

const { thread, events } = mkThread({
room,
client,
authorId: "@alice:example.org",
participantUserIds: ["@alice:example.org"],
length: 2,
ts: 10,
});

// The 10th event has been read, as alice's last unthreaded receipt is at ts 200
// and `mkThread` increment every thread response by 1ms.
expect(thread.getEventReadUpTo(myUserId)).toBe(events.at(-1)!.getId());
germain-gg marked this conversation as resolved.
Show resolved Hide resolved

const { thread: thread2 } = mkThread({
room,
client,
authorId: "@alice:example.org",
participantUserIds: ["@alice:example.org"],
length: 2,
ts: 1000,
});

// Nothing has been read, this thread is after the first threaded receipt...
expect(thread2.getEventReadUpTo(myUserId)).toBe(null);
});
});
});
30 changes: 29 additions & 1 deletion spec/unit/notifications.spec.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
Copyright 2022 The Matrix.org Foundation C.I.C.
Copyright 2022 - 2023 The Matrix.org Foundation C.I.C.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
Expand All @@ -14,6 +14,7 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

import { ReceiptType } from "../../src/@types/read_receipts";
import { Feature, ServerSupport } from "../../src/feature";
import {
EventType,
Expand Down Expand Up @@ -64,6 +65,30 @@ describe("fixNotificationCountOnDecryption", () => {
});

room = new Room(ROOM_ID, mockClient, mockClient.getUserId() ?? "");

const receipt = new MatrixEvent({
type: "m.receipt",
room_id: "!foo:bar",
content: {
"$event0:localhost": {
[ReceiptType.Read]: {
[mockClient.getUserId()!]: { ts: 123 },
},
},
"$event1:localhost": {
[ReceiptType.Read]: {
[mockClient.getUserId()!]: { ts: 666, thread_id: THREAD_ID },
},
},
"$otherevent:localhost": {
[ReceiptType.Read]: {
[mockClient.getUserId()!]: { ts: 999, thread_id: "$otherthread:localhost" },
},
},
},
});
room.addReceipt(receipt);

room.setUnreadNotificationCount(NotificationCountType.Total, 1);
room.setUnreadNotificationCount(NotificationCountType.Highlight, 0);

Expand All @@ -75,6 +100,7 @@ describe("fixNotificationCountOnDecryption", () => {
body: "Hello world!",
},
event: true,
ts: 1234,
},
mockClient,
);
Expand All @@ -90,6 +116,7 @@ describe("fixNotificationCountOnDecryption", () => {
"msgtype": MsgType.Text,
"body": "Thread reply",
},
ts: 5678,
event: true,
});
room.createThread(THREAD_ID, event, [threadEvent], false);
Expand Down Expand Up @@ -155,6 +182,7 @@ describe("fixNotificationCountOnDecryption", () => {
"msgtype": MsgType.Text,
"body": "Thread reply",
},
ts: 8901,
event: true,
});

Expand Down
4 changes: 2 additions & 2 deletions src/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -601,9 +601,9 @@ interface IJoinRequestBody {
third_party_signed?: IThirdPartySigned;
}

interface ITagMetadata {
export interface ITagMetadata {
germain-gg marked this conversation as resolved.
Show resolved Hide resolved
[key: string]: any;
order: number;
order?: number;
}

interface IMessagesResponse {
Expand Down
17 changes: 16 additions & 1 deletion src/models/room.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
Copyright 2015 - 2022 The Matrix.org Foundation C.I.C.
Copyright 2015 - 2023 The Matrix.org Foundation C.I.C.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -309,6 +309,10 @@ export class Room extends ReadReceipt<RoomEmittedEvents, RoomEventHandlerMap> {
private notificationCounts: NotificationCount = {};
private readonly threadNotifications = new Map<string, NotificationCount>();
public readonly cachedThreadReadReceipts = new Map<string, CachedReceiptStructure[]>();
// Useful to know at what point the current user has started using threads in this room
public oldestThreadedReceiptTs = Infinity;
// Important to compute `hasUserReadEvent` and similar methods correctly.
public unthreadedReceipts = new Map<string, Receipt>();
germain-gg marked this conversation as resolved.
Show resolved Hide resolved
germain-gg marked this conversation as resolved.
Show resolved Hide resolved
private readonly timelineSets: EventTimelineSet[];
public readonly threadsTimelineSets: EventTimelineSet[] = [];
// any filtered timeline sets we're maintaining for this room
Expand Down Expand Up @@ -2727,6 +2731,17 @@ export class Room extends ReadReceipt<RoomEmittedEvents, RoomEventHandlerMap> {
{ eventId, receiptType, userId, receipt, synthetic },
]);
}

const me = this.client.getUserId();
// Track the time of the current user's oldest threaded receipt in the room.
if (userId === me && !receiptForMainTimeline && receipt.ts < this.oldestThreadedReceiptTs) {
this.oldestThreadedReceiptTs = receipt.ts;
}
germain-gg marked this conversation as resolved.
Show resolved Hide resolved

// Track each user's unthreaded read receipt.
if (!receipt.thread_id && receipt.ts > (this.unthreadedReceipts.get(userId)?.ts ?? 0)) {
this.unthreadedReceipts.set(userId, receipt);
}
germain-gg marked this conversation as resolved.
Show resolved Hide resolved
});
});
});
Expand Down
Loading