From ceb2076620ab855fae53dc0be66ba70ca660b3d4 Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Fri, 6 Oct 2023 11:56:45 +0100 Subject: [PATCH 1/5] Warn when we drop an event trying to add it to a thread Added to try and help debug https://github.com/vector-im/element-web/issues/26254 --- src/models/thread.ts | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/src/models/thread.ts b/src/models/thread.ts index 2d8228d71ea..66e963cc666 100644 --- a/src/models/thread.ts +++ b/src/models/thread.ts @@ -367,6 +367,26 @@ export class Thread extends ReadReceipt Date: Fri, 6 Oct 2023 14:25:10 +0100 Subject: [PATCH 2/5] Comment trying to explain lastEvent --- src/models/thread.ts | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/src/models/thread.ts b/src/models/thread.ts index 66e963cc666..fab71df5515 100644 --- a/src/models/thread.ts +++ b/src/models/thread.ts @@ -90,7 +90,39 @@ export class Thread extends ReadReceipt; + /** + * The last event in this thread, if we don't yet have this in the timeline. + * + * When we run processRootEvent (which I think happens during the setting-up + * of the thread), we set this to the event pointed to by the server in + * latest_event https://github.com/matrix-org/matrix-spec-proposals/blob/main/proposals/3440-threading-via-relations.md#event-format + * that came through with the thread root. + * + * Later, when we have populated the timeline, this is set to undefined, so + * that methods like replyToEvent fall through to use lastReply, which looks + * in the timeline for the latest event that is a "thread reply" i.e. + * directly refers to the thread root with an m.thread relation. + * + * So it looks like this is only really relevant when initialEventsFetched + * is false, because as soon as the initial events have been fetched, we + * should have a timeline (I think). + * + * If all replies in this thread are redacted, this is set to the root + * event. I'm not clear what the meaning of this is, since usually after the + * initial events have been fetched, lastEvent should be undefined. + * In fact, the whole usage inside onRedaction looks suspect - it may be + * that we were thinking lastEvent always refers to the actual last event, + * but it only does so before initialEventsFetched becomes true. + * + * The usage of lastEvent inside onEcho looks suspicious, since I'd think we + * probably mean replyToEvent there - we are trying not to echo a duplicate + * event, and we probably want that behaviour even after + * initialEventsFetched has become true. + * + * -- andyb + */ private lastEvent: MatrixEvent | undefined; + private replyCount = 0; private lastPendingEvent: MatrixEvent | undefined; private pendingReplyCount = 0; From 549983433e2843e8b0dfa68589bffbb9d0462db2 Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Fri, 6 Oct 2023 15:08:38 +0100 Subject: [PATCH 3/5] Document some of my understanding of the addEvent logic --- src/models/thread.ts | 38 ++++++++++++++++++++++++++++++++------ 1 file changed, 32 insertions(+), 6 deletions(-) diff --git a/src/models/thread.ts b/src/models/thread.ts index fab71df5515..846512e9200 100644 --- a/src/models/thread.ts +++ b/src/models/thread.ts @@ -356,25 +356,34 @@ export class Thread extends ReadReceipt { + // Modify this event to point at our room's state, and mark its thread + // as this. this.setEventMetadata(event); + // Decide whether this event is going to be added at the end of the timeline. const lastReply = this.lastReply(); const isNewestReply = !lastReply || event.localTimestamp >= lastReply!.localTimestamp; - // Add all incoming events to the thread's timeline set when there's no server support if (!Thread.hasServerSideSupport) { - // all the relevant membership info to hydrate events with a sender - // is held in the main room timeline - // We want to fetch the room state from there and pass it down to this thread - // timeline set to let it reconcile an event with its relevant RoomMember + // When there's no server-side support, just add it to the end of the timeline. this.addEventToTimeline(event, toStartOfTimeline); - this.client.decryptEventIfNeeded(event, {}); } else if (!toStartOfTimeline && this.initialEventsFetched && isNewestReply) { + // When we've asked for the event to be added to the end, and we're + // not in the initial state, and this event belongs at the end, add it. this.addEventToTimeline(event, false); this.fetchEditsWhereNeeded(event); } else if (event.isRelation(RelationType.Annotation) || event.isRelation(RelationType.Replace)) { + // If this event is not a direct member of the thread, but is a + // reference to something that is, then we have two cases: + if (!this.initialEventsFetched) { + // Either we haven't yet fetched events from the server. In this + // case, when we do, the events we get back might only be the + // first-order ones, so this event (which is second-order - a + // reference to something directly in the thread) needs to be + // kept so we can replay it when the first-order ones turn up. + /** * A thread can be fully discovered via a single sync response * And when that's the case we still ask the server to do an initialisation @@ -386,6 +395,23 @@ export class Thread extends ReadReceipt Date: Mon, 9 Oct 2023 13:35:42 +0100 Subject: [PATCH 4/5] Refer to stable spec instead of MSC Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> --- src/models/thread.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/models/thread.ts b/src/models/thread.ts index 846512e9200..0f184efac52 100644 --- a/src/models/thread.ts +++ b/src/models/thread.ts @@ -95,8 +95,9 @@ export class Thread extends ReadReceipt Date: Mon, 9 Oct 2023 13:40:37 +0100 Subject: [PATCH 5/5] Re-word comments based on review --- src/models/thread.ts | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/src/models/thread.ts b/src/models/thread.ts index 0f184efac52..4003f810f0f 100644 --- a/src/models/thread.ts +++ b/src/models/thread.ts @@ -93,16 +93,16 @@ export class Thread extends ReadReceipt