Skip to content

Commit 5595e84

Browse files
andybalaamandybalaam
authored
Clarify code that chooses a thread ID to include in a receipt (#3797)
* Extract threadIdForReceipt function from sendReceipt * Tests for threadIdForReceipt * Correct test of threadIdForReceipt to expect main for redaction of threaded * Expand and comment implementation of threadIdForReceipt
1 parent 5d233f3 commit 5595e84

File tree

2 files changed

+265
-18
lines changed

2 files changed

+265
-18
lines changed

‎spec/unit/read-receipt.spec.ts

Lines changed: 197 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ import MockHttpBackend from "matrix-mock-request";
1818

1919
import { MAIN_ROOM_TIMELINE, ReceiptType, WrappedReceipt } from "../../src/@types/read_receipts";
2020
import { MatrixClient } from "../../src/client";
21-
import { EventType, MatrixEvent, Room } from "../../src/matrix";
21+
import { EventType, MatrixEvent, RelationType, Room, threadIdForReceipt } from "../../src/matrix";
2222
import { synthesizeReceipt } from "../../src/models/read-receipt";
2323
import { encodeUri } from "../../src/utils";
2424
import * as utils from "../test-utils/test-utils";
@@ -258,4 +258,200 @@ describe("Read receipt", () => {
258258
expect(room.getEventReadUpTo(userId)).toBe(mainTimelineReceipt.eventId);
259259
});
260260
});
261+
262+
describe("Determining the right thread ID for a receipt", () => {
263+
it("provides the thread root ID for a normal threaded message", () => {
264+
const event = utils.mkEvent({
265+
event: true,
266+
type: EventType.RoomMessage,
267+
user: "@bob:matrix.org",
268+
room: "!roomx",
269+
content: {
270+
"body": "Hello from a thread",
271+
"m.relates_to": {
272+
"event_id": "$thread1",
273+
"m.in_reply_to": {
274+
event_id: "$thread1",
275+
},
276+
"rel_type": "m.thread",
277+
},
278+
},
279+
});
280+
281+
expect(threadIdForReceipt(event)).toEqual("$thread1");
282+
});
283+
284+
it("provides 'main' for a non-thread message", () => {
285+
const event = utils.mkEvent({
286+
event: true,
287+
type: EventType.RoomMessage,
288+
user: "@bob:matrix.org",
289+
room: "!roomx",
290+
content: { body: "Hello" },
291+
});
292+
293+
expect(threadIdForReceipt(event)).toEqual("main");
294+
});
295+
296+
it("provides 'main' for a thread root", () => {
297+
const event = utils.mkEvent({
298+
event: true,
299+
type: EventType.RoomMessage,
300+
user: "@bob:matrix.org",
301+
room: "!roomx",
302+
content: { body: "Hello" },
303+
});
304+
// Set thread ID to this event's ID, meaning this is the thread root
305+
event.setThreadId(event.getId());
306+
307+
expect(threadIdForReceipt(event)).toEqual("main");
308+
});
309+
310+
it("provides 'main' for a reaction to a thread root", () => {
311+
const event = utils.mkEvent({
312+
event: true,
313+
type: EventType.Reaction,
314+
user: "@bob:matrix.org",
315+
room: "!roomx",
316+
content: {
317+
"m.relates_to": {
318+
rel_type: RelationType.Annotation,
319+
event_id: "$thread1",
320+
key: Math.random().toString(),
321+
},
322+
},
323+
});
324+
325+
// Set thread Id, meaning this looks like it's in the thread (this
326+
// happens for relations like this, so that they appear in the
327+
// thread's timeline).
328+
event.setThreadId("$thread1");
329+
330+
// But because it's a reaction to the thread root, it's in main
331+
expect(threadIdForReceipt(event)).toEqual("main");
332+
});
333+
334+
it("provides the thread ID for a reaction to a threaded message", () => {
335+
const event = utils.mkEvent({
336+
event: true,
337+
type: EventType.Reaction,
338+
user: "@bob:matrix.org",
339+
room: "!roomx",
340+
content: {
341+
"m.relates_to": {
342+
rel_type: RelationType.Annotation,
343+
event_id: "$withinthread2",
344+
key: Math.random().toString(),
345+
},
346+
},
347+
});
348+
349+
// Set thread Id, to say this message is in the thread. This happens
350+
// when the message arrived and is classified.
351+
event.setThreadId("$thread1");
352+
353+
// It's in the thread because it refers to something else, not the
354+
// thread root
355+
expect(threadIdForReceipt(event)).toEqual("$thread1");
356+
});
357+
358+
it("(suprisingly?) provides 'main' for a redaction of a threaded message", () => {
359+
const event = utils.mkEvent({
360+
event: true,
361+
type: EventType.RoomRedaction,
362+
content: {
363+
reason: "Spamming",
364+
},
365+
redacts: "$withinthread2",
366+
room: "!roomx",
367+
user: "@bob:matrix.org",
368+
});
369+
370+
// Set thread Id, to say this message is in the thread.
371+
event.setThreadId("$thread1");
372+
373+
// Because redacting a message removes all its m.relations, the
374+
// message is no longer in the thread, so we must send a receipt for
375+
// it in the main timeline.
376+
//
377+
// This is surprising, but it follows the spec (at least up to
378+
// current latest room version, 11). In fact, the event should no
379+
// longer have a thread ID set on it, so this testcase should not
380+
// come up. (At time of writing, this is not the case though - it
381+
// does still have threadId set.)
382+
expect(threadIdForReceipt(event)).toEqual("main");
383+
});
384+
385+
it("provides the thread ID for an edit of a threaded message", () => {
386+
const event = utils.mkEvent({
387+
event: true,
388+
type: EventType.RoomRedaction,
389+
content: {
390+
"body": "Edited!",
391+
"m.new_content": {
392+
body: "Edited!",
393+
},
394+
"m.relates_to": {
395+
rel_type: RelationType.Replace,
396+
event_id: "$withinthread2",
397+
},
398+
},
399+
room: "!roomx",
400+
user: "@bob:matrix.org",
401+
});
402+
403+
// Set thread Id, to say this message is in the thread.
404+
event.setThreadId("$thread1");
405+
406+
// It's in the thread, because it redacts something inside the
407+
// thread (not the thread root)
408+
expect(threadIdForReceipt(event)).toEqual("$thread1");
409+
});
410+
411+
it("provides 'main' for an edit of a thread root", () => {
412+
const event = utils.mkEvent({
413+
event: true,
414+
type: EventType.RoomRedaction,
415+
content: {
416+
"body": "Edited!",
417+
"m.new_content": {
418+
body: "Edited!",
419+
},
420+
"m.relates_to": {
421+
rel_type: RelationType.Replace,
422+
event_id: "$thread1",
423+
},
424+
},
425+
room: "!roomx",
426+
user: "@bob:matrix.org",
427+
});
428+
429+
// Set thread Id, to say this message is in the thread.
430+
event.setThreadId("$thread1");
431+
432+
// It's in the thread, because it redacts something inside the
433+
// thread (not the thread root)
434+
expect(threadIdForReceipt(event)).toEqual("main");
435+
});
436+
437+
it("provides 'main' for a redaction of the thread root", () => {
438+
const event = utils.mkEvent({
439+
event: true,
440+
type: EventType.RoomRedaction,
441+
content: {
442+
reason: "Spamming",
443+
},
444+
redacts: "$thread1",
445+
room: "!roomx",
446+
user: "@bob:matrix.org",
447+
});
448+
449+
// Set thread Id, to say this message is in the thread.
450+
event.setThreadId("$thread1");
451+
452+
// It's in the thread, because it redacts something inside the
453+
// thread (not the thread root)
454+
expect(threadIdForReceipt(event)).toEqual("main");
455+
});
456+
});
261457
});

‎src/client.ts

Lines changed: 68 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -5157,24 +5157,12 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
51575157
$eventId: event.getId()!,
51585158
});
51595159

5160-
if (!unthreaded && this.supportsThreads()) {
5161-
// XXX: the spec currently says a threaded read receipt can be sent for the root of a thread,
5162-
// but in practice this isn't possible and the spec needs updating.
5163-
const isThread =
5164-
!!event.threadRootId &&
5165-
// A thread cannot be just a thread root and a thread root can only be read in the main timeline
5166-
!event.isThreadRoot &&
5167-
// Similarly non-thread relations upon the thread root (reactions, edits) should also be for the main timeline.
5168-
event.isRelation() &&
5169-
(event.isRelation(THREAD_RELATION_TYPE.name) || event.relationEventId !== event.threadRootId);
5170-
body = {
5171-
...body,
5172-
// Only thread replies should define a specific thread. Thread roots can only be read in the main timeline.
5173-
thread_id: isThread ? event.threadRootId : MAIN_ROOM_TIMELINE,
5174-
};
5175-
}
5160+
// Unless we're explicitly making an unthreaded receipt or we don't
5161+
// support threads, include the `thread_id` property in the body.
5162+
const shouldAddThreadId = !unthreaded && this.supportsThreads();
5163+
const fullBody = shouldAddThreadId ? { ...body, thread_id: threadIdForReceipt(event) } : body;
51765164

5177-
const promise = this.http.authedRequest<{}>(Method.Post, path, undefined, body || {});
5165+
const promise = this.http.authedRequest<{}>(Method.Post, path, undefined, fullBody || {});
51785166

51795167
const room = this.getRoom(event.getRoomId());
51805168
if (room && this.credentials.userId) {
@@ -9925,3 +9913,66 @@ export function fixNotificationCountOnDecryption(cli: MatrixClient, event: Matri
99259913
}
99269914
}
99279915
}
9916+
9917+
/**
9918+
* Given an event, figure out the thread ID we should use for it in a receipt.
9919+
*
9920+
* This will either be "main", or event.threadRootId. For the thread root, or
9921+
* e.g. reactions to the thread root, this will be main. For events inside the
9922+
* thread, or e.g. reactions to them, this will be event.threadRootId.
9923+
*
9924+
* (Exported for test.)
9925+
*/
9926+
export function threadIdForReceipt(event: MatrixEvent): string {
9927+
return inMainTimelineForReceipt(event) ? MAIN_ROOM_TIMELINE : event.threadRootId!;
9928+
}
9929+
9930+
/**
9931+
* a) True for non-threaded messages, thread roots and non-thread relations to thread roots.
9932+
* b) False for messages with thread relations to the thread root.
9933+
* c) False for messages with any kind of relation to a message from case b.
9934+
*
9935+
* Note: true for redactions of messages that are in threads. Redacted messages
9936+
* are not really in threads (because their relations are gone), so if they look
9937+
* like they are in threads, that is a sign of a bug elsewhere. (At time of
9938+
* writing, this bug definitely exists - messages are not moved to another
9939+
* thread when they are redacted.)
9940+
*
9941+
* @returns true if this event is considered to be in the main timeline as far
9942+
* as receipts are concerned.
9943+
*/
9944+
function inMainTimelineForReceipt(event: MatrixEvent): boolean {
9945+
if (!event.threadRootId) {
9946+
// Not in a thread: then it is in the main timeline
9947+
return true;
9948+
}
9949+
9950+
if (event.isThreadRoot) {
9951+
// Thread roots are in the main timeline. Note: the spec is ambiguous (or
9952+
// wrong) on this - see
9953+
// https://github.com/matrix-org/matrix-spec-proposals/pull/4037
9954+
return true;
9955+
}
9956+
9957+
if (!event.isRelation()) {
9958+
// If it's not related to anything, it can't be related via a chain of
9959+
// relations to a thread root.
9960+
//
9961+
// Note: this is a bug, because how does it have a threadRootId if it is
9962+
// neither a thread root, nor related to one?
9963+
logger.warn(`Event is not a relation or a thread root, but still has a threadRootId! id=${event.getId()}`);
9964+
return true;
9965+
}
9966+
9967+
if (event.isRelation(THREAD_RELATION_TYPE.name)) {
9968+
// It's a message in a thread - definitely not in the main timeline.
9969+
return false;
9970+
}
9971+
9972+
const isRelatedToRoot = event.relationEventId === event.threadRootId;
9973+
9974+
// If it's related to the thread root (and we already know it's not a thread
9975+
// relation) then it's in the main timeline. If it's related to something
9976+
// else, then it's in the thread (because it has a thread ID).
9977+
return isRelatedToRoot;
9978+
}

0 commit comments

Comments
 (0)