From 81b58388ee177672a0b55fcac8e5294529c12065 Mon Sep 17 00:00:00 2001
From: David Baker <dbkr@users.noreply.github.com>
Date: Wed, 17 Jan 2024 15:20:11 +0000
Subject: [PATCH] Fix new threads not appearing. (#4009)

* Fix new threads not appearing.

We try to update the thread roots when creating a thread, but a thread
can take some time to be ready after being created so we were calling it
too soon. Add a listener for the Update event to update the thread roots
once it's ready.

Fixes https://github.com/element-hq/element-web/issues/26799

* Don't recreate the event when we update

and also add a comment to the test

* Hopefully make sonarcloud happy
---
 .../matrix-client-event-timeline.spec.ts      |   2 +-
 src/models/room.ts                            |   9 +-
 src/models/thread.ts                          | 112 ++++++++++--------
 3 files changed, 70 insertions(+), 53 deletions(-)

diff --git a/spec/integ/matrix-client-event-timeline.spec.ts b/spec/integ/matrix-client-event-timeline.spec.ts
index 1d80cdd98d5..fe7823d448f 100644
--- a/spec/integ/matrix-client-event-timeline.spec.ts
+++ b/spec/integ/matrix-client-event-timeline.spec.ts
@@ -1555,7 +1555,7 @@ describe("MatrixClient event timelines", function () {
                 expect(threadIds).toContain(THREAD2_ROOT.event_id);
                 const [allThreads] = timelineSets!;
                 const timeline = allThreads.getLiveTimeline()!;
-                // Test threads are in chronological order
+                // Test threads are in chronological order (first thread should be first because it has a more recent reply)
                 expect(timeline.getEvents().map((it) => it.event.event_id)).toEqual([
                     THREAD_ROOT.event_id,
                     THREAD2_ROOT.event_id,
diff --git a/src/models/room.ts b/src/models/room.ts
index 48fabcfce46..3d1f727da67 100644
--- a/src/models/room.ts
+++ b/src/models/room.ts
@@ -1979,6 +1979,7 @@ export class Room extends ReadReceipt<RoomEmittedEvents, RoomEventHandlerMap> {
         }
 
         this.on(ThreadEvent.NewReply, this.onThreadReply);
+        this.on(ThreadEvent.Update, this.onThreadUpdate);
         this.on(ThreadEvent.Delete, this.onThreadDelete);
         this.threadsReady = true;
     }
@@ -2082,6 +2083,10 @@ export class Room extends ReadReceipt<RoomEmittedEvents, RoomEventHandlerMap> {
         }
     }
 
+    private onThreadUpdate(thread: Thread): void {
+        this.updateThreadRootEvents(thread, false, false);
+    }
+
     private onThreadReply(thread: Thread): void {
         this.updateThreadRootEvents(thread, false, true);
     }
@@ -2329,7 +2334,9 @@ export class Room extends ReadReceipt<RoomEmittedEvents, RoomEventHandlerMap> {
             this.lastThread = thread;
         }
 
-        if (this.threadsReady) {
+        // We need to update the thread root events, but the thread may not be ready yet.
+        // If it isn't, it will fire ThreadEvent.Update when it is and we'll call updateThreadRootEvents then.
+        if (this.threadsReady && thread.initialEventsFetched) {
             this.updateThreadRootEvents(thread, toStartOfTimeline, false);
         }
         this.emit(ThreadEvent.New, thread, toStartOfTimeline);
diff --git a/src/models/thread.ts b/src/models/thread.ts
index 855febb1ab2..96cea941b3c 100644
--- a/src/models/thread.ts
+++ b/src/models/thread.ts
@@ -133,7 +133,13 @@ export class Thread extends ReadReceipt<ThreadEmittedEvents, ThreadEventHandlerM
     private readonly pendingEventOrdering: PendingEventOrdering;
     private processRootEventPromise?: Promise<void>;
 
+    /**
+     * Whether or not we need to fetch the initial set of events for the thread. We can
+     * only do this if the server has support for it, so if it doesn't we just pretend
+     * that we've already fetched them.
+     */
     public initialEventsFetched = !Thread.hasServerSideSupport;
+
     /**
      * An array of events to add to the timeline once the thread has been initialised
      * with server suppport.
@@ -363,7 +369,7 @@ export class Thread extends ReadReceipt<ThreadEmittedEvents, ThreadEventHandlerM
      * to the start (and not the end) of the timeline.
      * @param emit - whether to emit the Update event if the thread was updated or not.
      */
-    public async addEvent(event: MatrixEvent, toStartOfTimeline: boolean, emit = true): Promise<void> {
+    public addEvent(event: MatrixEvent, toStartOfTimeline: boolean, emit = true): void {
         // Modify this event to point at our room's state, and mark its thread
         // as this.
         this.setEventMetadata(event);
@@ -382,56 +388,7 @@ export class Thread extends ReadReceipt<ThreadEmittedEvents, ThreadEventHandlerM
             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) {
-                // Case 1: 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
-                 * as it's the safest to ensure we have everything.
-                 * However when we are in that scenario we might loose annotation or edits
-                 *
-                 * This fix keeps a reference to those events and replay them once the thread
-                 * has been initialised properly.
-                 */
-                this.replayEvents?.push(event);
-            } else {
-                // Case 2: this is happening later, and we have a timeline. In
-                // this case, these events might be out-of order.
-                //
-                // Specifically, if the server doesn't support recursion, so we
-                // only get these events through sync, they might be coming
-                // later than the first-order ones, so we insert them based on
-                // timestamp (despite the problems with this documented in
-                // #3325).
-                //
-                // If the server does support recursion, we should have got all
-                // the interspersed events from the server when we fetched the
-                // initial events, so if they are coming via sync they should be
-                // the latest ones, so we can add them as normal.
-                //
-                // (Note that both insertEventIntoTimeline and addEventToTimeline
-                // do nothing if we have seen this event before.)
-
-                const recursionSupport =
-                    this.client.canSupport.get(Feature.RelationsRecursion) ?? ServerSupport.Unsupported;
-
-                if (recursionSupport === ServerSupport.Unsupported) {
-                    this.insertEventIntoTimeline(event);
-                } else {
-                    this.addEventToTimeline(event, toStartOfTimeline);
-                }
-            }
-            // Apply annotations and replace relations to the relations of the timeline only
-            this.timelineSet.relations?.aggregateParentEvent(event);
-            this.timelineSet.relations?.aggregateChildEvent(event, this.timelineSet);
+            this.addRelatedThreadEvent(event, toStartOfTimeline);
             return;
         } else if (this.initialEventsFetched) {
             // If initial events have not been fetched, we are OK to throw away
@@ -468,6 +425,59 @@ export class Thread extends ReadReceipt<ThreadEmittedEvents, ThreadEventHandlerM
         }
     }
 
+    private addRelatedThreadEvent(event: MatrixEvent, toStartOfTimeline: boolean): void {
+        // 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) {
+            // Case 1: 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
+             * as it's the safest to ensure we have everything.
+             * However when we are in that scenario we might loose annotation or edits
+             *
+             * This fix keeps a reference to those events and replay them once the thread
+             * has been initialised properly.
+             */
+            this.replayEvents?.push(event);
+        } else {
+            // Case 2: this is happening later, and we have a timeline. In
+            // this case, these events might be out-of order.
+            //
+            // Specifically, if the server doesn't support recursion, so we
+            // only get these events through sync, they might be coming
+            // later than the first-order ones, so we insert them based on
+            // timestamp (despite the problems with this documented in
+            // #3325).
+            //
+            // If the server does support recursion, we should have got all
+            // the interspersed events from the server when we fetched the
+            // initial events, so if they are coming via sync they should be
+            // the latest ones, so we can add them as normal.
+            //
+            // (Note that both insertEventIntoTimeline and addEventToTimeline
+            // do nothing if we have seen this event before.)
+
+            const recursionSupport =
+                this.client.canSupport.get(Feature.RelationsRecursion) ?? ServerSupport.Unsupported;
+
+            if (recursionSupport === ServerSupport.Unsupported) {
+                this.insertEventIntoTimeline(event);
+            } else {
+                this.addEventToTimeline(event, toStartOfTimeline);
+            }
+        }
+        // Apply annotations and replace relations to the relations of the timeline only
+        this.timelineSet.relations?.aggregateParentEvent(event);
+        this.timelineSet.relations?.aggregateChildEvent(event, this.timelineSet);
+    }
+
     public async processEvent(event: Optional<MatrixEvent>): Promise<void> {
         if (event) {
             this.setEventMetadata(event);