Skip to content

Fix edge cases around non-thread relations to thread roots and read receipts #3607

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Jul 19, 2023
Merged

Fix edge cases around non-thread relations to thread roots and read receipts #3607

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c350296
Ensure non-thread relations to a thread root are actually in both tim…
t3chguy Jul 18, 2023
06bdb4f
Make thread in sendReceipt & sendReadReceipt explicit rather than gue…
t3chguy Jul 18, 2023
b7948b2
Apply suggestions from code review
t3chguy Jul 18, 2023
bed148f
Fix Room::eventShouldLiveIn to better match Synapse to diverging idea…
t3chguy Jul 18, 2023
d1c79fb
Update read receipt sending behaviour to align with Synapse
t3chguy Jul 18, 2023
83e5683
Fix tests
t3chguy Jul 18, 2023
9b23016
Fix thread rel type
t3chguy Jul 18, 2023
6752cd3
Merge branch 'develop' into t3chguy/stuck-edits-thread-root
t3chguy Jul 19, 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
1 change: 0 additions & 1 deletion spec/integ/matrix-client-event-timeline.spec.ts
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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