Skip to content

Commit

Permalink
Fix edge cases around non-thread relations to thread roots and read r…
Browse files Browse the repository at this point in the history 
…eceipts (#3607)

* Ensure non-thread relations to a thread root are actually in both timelines

* Make thread in sendReceipt & sendReadReceipt explicit rather than guessing it

* Apply suggestions from code review

* Fix Room::eventShouldLiveIn to better match Synapse to diverging ideas of notifications

* Update read receipt sending behaviour to align with Synapse

* Fix tests

* Fix thread rel type
  • Loading branch information
@t3chguy
t3chguy authored Jul 19, 2023
1 parent 43b2404 commit 66492e7
Show file tree
Hide file tree
Showing 7 changed files with 178 additions and 101 deletions.
1 change: 0 additions & 1 deletion spec/integ/matrix-client-event-timeline.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1284,7 +1284,6 @@ describe("MatrixClient event timelines", function () {
THREAD_ROOT.event_id,
THREAD_REPLY.event_id,
THREAD_REPLY2.getId(),
THREAD_ROOT_REACTION.getId(),
THREAD_REPLY3.getId(),
]);
});
Expand Down
38 changes: 11 additions & 27 deletions spec/integ/matrix-client-methods.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -656,7 +656,7 @@ describe("MatrixClient", function () {
expect(threaded).toEqual([]);
});

it("copies pre-thread in-timeline vote events onto both timelines", function () {
it("should not copy pre-thread in-timeline vote events onto both timelines", function () {
// @ts-ignore setting private property
client.clientOpts = {
...defaultClientOpts,
Expand Down Expand Up @@ -684,10 +684,10 @@ describe("MatrixClient", function () {
const eventRefWithThreadId = withThreadId(eventPollResponseReference, eventPollStartThreadRoot.getId()!);
expect(eventRefWithThreadId.threadRootId).toBeTruthy();

expect(threaded).toEqual([eventPollStartThreadRoot, eventMessageInThread, eventRefWithThreadId]);
expect(threaded).toEqual([eventPollStartThreadRoot, eventMessageInThread]);
});

it("copies pre-thread in-timeline reactions onto both timelines", function () {
it("should not copy pre-thread in-timeline reactions onto both timelines", function () {
// @ts-ignore setting private property
client.clientOpts = {
...defaultClientOpts,
Expand All @@ -704,14 +704,10 @@ describe("MatrixClient", function () {

expect(timeline).toEqual([eventPollStartThreadRoot, eventReaction]);

expect(threaded).toEqual([
eventPollStartThreadRoot,
eventMessageInThread,
withThreadId(eventReaction, eventPollStartThreadRoot.getId()!),
]);
expect(threaded).toEqual([eventPollStartThreadRoot, eventMessageInThread]);
});

it("copies post-thread in-timeline vote events onto both timelines", function () {
it("should not copy post-thread in-timeline vote events onto both timelines", function () {
// @ts-ignore setting private property
client.clientOpts = {
...defaultClientOpts,
Expand All @@ -728,14 +724,10 @@ describe("MatrixClient", function () {

expect(timeline).toEqual([eventPollStartThreadRoot, eventPollResponseReference]);

expect(threaded).toEqual([
eventPollStartThreadRoot,
withThreadId(eventPollResponseReference, eventPollStartThreadRoot.getId()!),
eventMessageInThread,
]);
expect(threaded).toEqual([eventPollStartThreadRoot, eventMessageInThread]);
});

it("copies post-thread in-timeline reactions onto both timelines", function () {
it("should not copy post-thread in-timeline reactions onto both timelines", function () {
// @ts-ignore setting private property
client.clientOpts = {
...defaultClientOpts,
Expand All @@ -752,11 +744,7 @@ describe("MatrixClient", function () {

expect(timeline).toEqual([eventPollStartThreadRoot, eventReaction]);

expect(threaded).toEqual([
eventPollStartThreadRoot,
eventMessageInThread,
withThreadId(eventReaction, eventPollStartThreadRoot.getId()!),
]);
expect(threaded).toEqual([eventPollStartThreadRoot, eventMessageInThread]);
});

it("sends room state events to the main timeline only", function () {
Expand Down Expand Up @@ -809,11 +797,7 @@ describe("MatrixClient", function () {
]);

// Thread should contain only stuff that happened in the thread - no room state events
expect(threaded).toEqual([
eventPollStartThreadRoot,
withThreadId(eventPollResponseReference, eventPollStartThreadRoot.getId()!),
eventMessageInThread,
]);
expect(threaded).toEqual([eventPollStartThreadRoot, eventMessageInThread]);
});

it("sends redactions of reactions to thread responses to thread timeline only", () => {
Expand Down Expand Up @@ -878,9 +862,9 @@ describe("MatrixClient", function () {

const [timeline, threaded] = room.partitionThreadedEvents(events);

expect(timeline).toEqual([threadRootEvent, replyToThreadResponse]);
expect(timeline).toEqual([threadRootEvent]);

expect(threaded).toEqual([threadRootEvent, eventMessageInThread]);
expect(threaded).toEqual([threadRootEvent, eventMessageInThread, replyToThreadResponse]);
});
});

Expand Down
4 changes: 2 additions & 2 deletions spec/test-utils/thread.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import { RelationType } from "../../src/@types/event";
import { MatrixClient } from "../../src/client";
import { MatrixEvent, MatrixEventEvent } from "../../src/models/event";
import { Room } from "../../src/models/room";
import { Thread } from "../../src/models/thread";
import { Thread, THREAD_RELATION_TYPE } from "../../src/models/thread";
import { mkMessage } from "./test-utils";

export const makeThreadEvent = ({
Expand All @@ -34,7 +34,7 @@ export const makeThreadEvent = ({
...props,
relatesTo: {
event_id: rootEventId,
rel_type: "m.thread",
rel_type: THREAD_RELATION_TYPE.name,
["m.in_reply_to"]: {
event_id: replyToEventId,
},
Expand Down
98 changes: 67 additions & 31 deletions spec/unit/read-receipt.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import MockHttpBackend from "matrix-mock-request";

import { MAIN_ROOM_TIMELINE, ReceiptType } from "../../src/@types/read_receipts";
import { MatrixClient } from "../../src/client";
import { EventType } from "../../src/matrix";
import { EventType, MatrixEvent, Room } from "../../src/matrix";
import { synthesizeReceipt } from "../../src/models/read-receipt";
import { encodeUri } from "../../src/utils";
import * as utils from "../test-utils/test-utils";
Expand All @@ -42,42 +42,45 @@ let httpBackend: MockHttpBackend;
const THREAD_ID = "$thread_event_id";
const ROOM_ID = "!123:matrix.org";

const threadEvent = utils.mkEvent({
event: true,
type: EventType.RoomMessage,
user: "@bob:matrix.org",
room: ROOM_ID,
content: {
"body": "Hello from a thread",
"m.relates_to": {
"event_id": THREAD_ID,
"m.in_reply_to": {
event_id: THREAD_ID,
},
"rel_type": "m.thread",
},
},
});

const roomEvent = utils.mkEvent({
event: true,
type: EventType.RoomMessage,
user: "@bob:matrix.org",
room: ROOM_ID,
content: {
body: "Hello from a room",
},
});

describe("Read receipt", () => {
let threadEvent: MatrixEvent;
let roomEvent: MatrixEvent;

beforeEach(() => {
httpBackend = new MockHttpBackend();
client = new MatrixClient({
userId: "@user:server",
baseUrl: "https://my.home.server",
accessToken: "my.access.token",
fetchFn: httpBackend.fetchFn as typeof global.fetch,
});
client.isGuest = () => false;

threadEvent = utils.mkEvent({
event: true,
type: EventType.RoomMessage,
user: "@bob:matrix.org",
room: ROOM_ID,
content: {
"body": "Hello from a thread",
"m.relates_to": {
"event_id": THREAD_ID,
"m.in_reply_to": {
event_id: THREAD_ID,
},
"rel_type": "m.thread",
},
},
});
roomEvent = utils.mkEvent({
event: true,
type: EventType.RoomMessage,
user: "@bob:matrix.org",
room: ROOM_ID,
content: {
body: "Hello from a room",
},
});
});

describe("sendReceipt", () => {
Expand Down Expand Up @@ -143,13 +146,46 @@ describe("Read receipt", () => {
await httpBackend.flushAllExpected();
await flushPromises();
});

it("should send a main timeline read receipt for a reaction to a thread root", async () => {
roomEvent.event.event_id = THREAD_ID;
const reaction = utils.mkReaction(roomEvent, client, client.getSafeUserId(), ROOM_ID);
const thread = new Room(ROOM_ID, client, client.getSafeUserId()).createThread(
THREAD_ID,
roomEvent,
[threadEvent],
false,
);
threadEvent.setThread(thread);
reaction.setThread(thread);

httpBackend
.when(
"POST",
encodeUri("/rooms/$roomId/receipt/$receiptType/$eventId", {
$roomId: ROOM_ID,
$receiptType: ReceiptType.Read,
$eventId: reaction.getId()!,
}),
)
.check((request) => {
expect(request.data.thread_id).toEqual(MAIN_ROOM_TIMELINE);
})
.respond(200, {});

client.sendReceipt(reaction, ReceiptType.Read, {});

await httpBackend.flushAllExpected();
await flushPromises();
});
});

describe("synthesizeReceipt", () => {
it.each([
{ event: roomEvent, destinationId: MAIN_ROOM_TIMELINE },
{ event: threadEvent, destinationId: threadEvent.threadRootId! },
])("adds the receipt to $destinationId", ({ event, destinationId }) => {
{ getEvent: () => roomEvent, destinationId: MAIN_ROOM_TIMELINE },
{ getEvent: () => threadEvent, destinationId: THREAD_ID },
])("adds the receipt to $destinationId", ({ getEvent, destinationId }) => {
const event = getEvent();
const userId = "@bob:example.org";
const receiptType = ReceiptType.Read;

Expand Down
86 changes: 64 additions & 22 deletions spec/unit/room.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2849,7 +2849,7 @@ describe("Room", function () {
Thread.setServerSideSupport(FeatureSupport.Stable);
const room = new Room(roomId, client, userA);

it("thread root and its relations&redactions should be in both", () => {
it("thread root and its relations&redactions should be in main timeline", () => {
const randomMessage = mkMessage();
const threadRoot = mkMessage();
const threadResponse1 = mkThreadResponse(threadRoot);
Expand All @@ -2867,6 +2867,9 @@ describe("Room", function () {
threadReaction2Redaction,
];

const thread = room.createThread(threadRoot.getId()!, threadRoot, [], false);
events.slice(1).forEach((ev) => ev.setThread(thread));

expect(room.eventShouldLiveIn(randomMessage, events, roots).shouldLiveInRoom).toBeTruthy();
expect(room.eventShouldLiveIn(randomMessage, events, roots).shouldLiveInThread).toBeFalsy();

Expand All @@ -2878,14 +2881,11 @@ describe("Room", function () {
expect(room.eventShouldLiveIn(threadResponse1, events, roots).threadId).toBe(threadRoot.getId());

expect(room.eventShouldLiveIn(threadReaction1, events, roots).shouldLiveInRoom).toBeTruthy();
expect(room.eventShouldLiveIn(threadReaction1, events, roots).shouldLiveInThread).toBeTruthy();
expect(room.eventShouldLiveIn(threadReaction1, events, roots).threadId).toBe(threadRoot.getId());
expect(room.eventShouldLiveIn(threadReaction1, events, roots).shouldLiveInThread).toBeFalsy();
expect(room.eventShouldLiveIn(threadReaction2, events, roots).shouldLiveInRoom).toBeTruthy();
expect(room.eventShouldLiveIn(threadReaction2, events, roots).shouldLiveInThread).toBeTruthy();
expect(room.eventShouldLiveIn(threadReaction2, events, roots).threadId).toBe(threadRoot.getId());
expect(room.eventShouldLiveIn(threadReaction2, events, roots).shouldLiveInThread).toBeFalsy();
expect(room.eventShouldLiveIn(threadReaction2Redaction, events, roots).shouldLiveInRoom).toBeTruthy();
expect(room.eventShouldLiveIn(threadReaction2Redaction, events, roots).shouldLiveInThread).toBeTruthy();
expect(room.eventShouldLiveIn(threadReaction2Redaction, events, roots).threadId).toBe(threadRoot.getId());
expect(room.eventShouldLiveIn(threadReaction2Redaction, events, roots).shouldLiveInThread).toBeFalsy();
});

it("thread response and its relations&redactions should be only in thread timeline", () => {
Expand All @@ -2909,25 +2909,39 @@ describe("Room", function () {
expect(room.eventShouldLiveIn(threadReaction2Redaction, events, roots).threadId).toBe(threadRoot.getId());
});

it("reply to thread response and its relations&redactions should be only in main timeline", () => {
it("reply to thread response and its relations&redactions should be only in thread timeline", () => {
const threadRoot = mkMessage();
const threadResponse1 = mkThreadResponse(threadRoot);
const reply1 = mkReply(threadResponse1);
const reaction1 = utils.mkReaction(reply1, room.client, userA, roomId);
const reaction2 = utils.mkReaction(reply1, room.client, userA, roomId);
const reaction2Redaction = mkRedaction(reply1);
const threadResp1 = mkThreadResponse(threadRoot);
const threadResp1Reply1 = mkReply(threadResp1);
const threadResp1Reply1Reaction1 = utils.mkReaction(threadResp1Reply1, room.client, userA, roomId);
const threadResp1Reply1Reaction2 = utils.mkReaction(threadResp1Reply1, room.client, userA, roomId);
const thResp1Rep1React2Redaction = mkRedaction(threadResp1Reply1);

const roots = new Set([threadRoot.getId()!]);
const events = [threadRoot, threadResponse1, reply1, reaction1, reaction2, reaction2Redaction];
const events = [
threadRoot,
threadResp1,
threadResp1Reply1,
threadResp1Reply1Reaction1,
threadResp1Reply1Reaction2,
thResp1Rep1React2Redaction,
];

expect(room.eventShouldLiveIn(reply1, events, roots).shouldLiveInRoom).toBeTruthy();
expect(room.eventShouldLiveIn(reply1, events, roots).shouldLiveInThread).toBeFalsy();
expect(room.eventShouldLiveIn(reaction1, events, roots).shouldLiveInRoom).toBeTruthy();
expect(room.eventShouldLiveIn(reaction1, events, roots).shouldLiveInThread).toBeFalsy();
expect(room.eventShouldLiveIn(reaction2, events, roots).shouldLiveInRoom).toBeTruthy();
expect(room.eventShouldLiveIn(reaction2, events, roots).shouldLiveInThread).toBeFalsy();
expect(room.eventShouldLiveIn(reaction2Redaction, events, roots).shouldLiveInRoom).toBeTruthy();
expect(room.eventShouldLiveIn(reaction2Redaction, events, roots).shouldLiveInThread).toBeFalsy();
const thread = room.createThread(threadRoot.getId()!, threadRoot, [], false);
events.forEach((ev) => ev.setThread(thread));

expect(room.eventShouldLiveIn(threadResp1Reply1, events, roots).shouldLiveInRoom).toBeFalsy();
expect(room.eventShouldLiveIn(threadResp1Reply1, events, roots).shouldLiveInThread).toBeTruthy();
expect(room.eventShouldLiveIn(threadResp1Reply1, events, roots).threadId).toBe(thread.id);
expect(room.eventShouldLiveIn(threadResp1Reply1Reaction1, events, roots).shouldLiveInRoom).toBeFalsy();
expect(room.eventShouldLiveIn(threadResp1Reply1Reaction1, events, roots).shouldLiveInThread).toBeTruthy();
expect(room.eventShouldLiveIn(threadResp1Reply1Reaction1, events, roots).threadId).toBe(thread.id);
expect(room.eventShouldLiveIn(threadResp1Reply1Reaction2, events, roots).shouldLiveInRoom).toBeFalsy();
expect(room.eventShouldLiveIn(threadResp1Reply1Reaction2, events, roots).shouldLiveInThread).toBeTruthy();
expect(room.eventShouldLiveIn(threadResp1Reply1Reaction2, events, roots).threadId).toBe(thread.id);
expect(room.eventShouldLiveIn(thResp1Rep1React2Redaction, events, roots).shouldLiveInRoom).toBeFalsy();
expect(room.eventShouldLiveIn(thResp1Rep1React2Redaction, events, roots).shouldLiveInThread).toBeTruthy();
expect(room.eventShouldLiveIn(thResp1Rep1React2Redaction, events, roots).threadId).toBe(thread.id);
});

it("reply to reply to thread root should only be in the main timeline", () => {
Expand All @@ -2939,12 +2953,40 @@ describe("Room", function () {
const roots = new Set([threadRoot.getId()!]);
const events = [threadRoot, threadResponse1, reply1, reply2];

const thread = room.createThread(threadRoot.getId()!, threadRoot, [], false);
threadResponse1.setThread(thread);

expect(room.eventShouldLiveIn(reply1, events, roots).shouldLiveInRoom).toBeTruthy();
expect(room.eventShouldLiveIn(reply1, events, roots).shouldLiveInThread).toBeFalsy();
expect(room.eventShouldLiveIn(reply2, events, roots).shouldLiveInRoom).toBeTruthy();
expect(room.eventShouldLiveIn(reply2, events, roots).shouldLiveInThread).toBeFalsy();
});

it("edit to thread root should live in main timeline only", () => {
const threadRoot = mkMessage();
const threadResponse1 = mkThreadResponse(threadRoot);
const threadRootEdit = mkEdit(threadRoot);
threadRoot.makeReplaced(threadRootEdit);

const thread = room.createThread(threadRoot.getId()!, threadRoot, [threadResponse1], false);
threadResponse1.setThread(thread);
threadRootEdit.setThread(thread);

const roots = new Set([threadRoot.getId()!]);
const events = [threadRoot, threadResponse1, threadRootEdit];

expect(room.eventShouldLiveIn(threadRoot, events, roots).shouldLiveInRoom).toBeTruthy();
expect(room.eventShouldLiveIn(threadRoot, events, roots).shouldLiveInThread).toBeTruthy();
expect(room.eventShouldLiveIn(threadRoot, events, roots).threadId).toBe(threadRoot.getId());

expect(room.eventShouldLiveIn(threadResponse1, events, roots).shouldLiveInRoom).toBeFalsy();
expect(room.eventShouldLiveIn(threadResponse1, events, roots).shouldLiveInThread).toBeTruthy();
expect(room.eventShouldLiveIn(threadResponse1, events, roots).threadId).toBe(threadRoot.getId());

expect(room.eventShouldLiveIn(threadRootEdit, events, roots).shouldLiveInRoom).toBeTruthy();
expect(room.eventShouldLiveIn(threadRootEdit, events, roots).shouldLiveInThread).toBeFalsy();
});

it("should aggregate relations in thread event timeline set", async () => {
Thread.setServerSideSupport(FeatureSupport.Stable);
const threadRoot = mkMessage();
Expand Down
Loading

0 comments on commit 66492e7

Please sign in to comment.