Fix issues with duplicated MatrixEvent objects around threads

spec/test-utils/test-utils.js

@@ -85,7 +85,7 @@ export function mkEvent(opts) {
         room_id: opts.room,
         sender: opts.sender || opts.user, // opts.user for backwards-compat
         content: opts.content,
-        unsigned: opts.unsigned,
+        unsigned: opts.unsigned || {},
         event_id: "$" + Math.random() + "-" + Math.random(),
     };
     if (opts.skey !== undefined) {

src/client.ts

@@ -3951,7 +3951,6 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
     /**
      * Returns the eventType that should be used taking encryption into account
      * for a given eventType.
-     * @param {MatrixClient} client the client
      * @param {string} roomId the room for the events `eventType` relates to
      * @param {string} eventType the event type
      * @return {string} the event type taking encryption into account
@@ -6626,13 +6625,15 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
             originalEvent = mapper(result.original_event);
         }
         let events = result.chunk.map(mapper);
+
         if (fetchedEventType === EventType.RoomMessageEncrypted) {
             const allEvents = originalEvent ? events.concat(originalEvent) : events;
             await Promise.all(allEvents.map(e => this.decryptEventIfNeeded(e)));
             if (eventType !== null) {
                 events = events.filter(e => e.getType() === eventType);
             }
         }
+
         if (originalEvent && relationType === RelationType.Replace) {
             events = events.filter(e => e.getSender() === originalEvent.getSender());
         }
@@ -8866,12 +8867,11 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
         }
 
         const parentEventId = event.getAssociatedId();
-        const parentEvent = room?.findEventById(parentEventId) || events.find((mxEv: MatrixEvent) => (
+        const parentEvent = room?.findEventById(parentEventId) ?? events.find((mxEv: MatrixEvent) => (
             mxEv.getId() === parentEventId
         ));
 
-        // A reaction targetting the thread root needs to be routed to both the
-        // the main timeline and the associated thread
+        // A reaction targeting the thread root needs to be routed to both the main timeline and the associated thread
         const targetingThreadRoot = parentEvent?.isThreadRoot || roots.has(event.relationEventId);
         if (targetingThreadRoot) {
             return {
@@ -8887,18 +8887,21 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
         // we want that redaction to be pushed to both timeline
         if (parentEvent?.getAssociatedId()) {
             return this.eventShouldLiveIn(parentEvent, room, events, roots);
-        } else {
-            // We've exhausted all scenarios, can safely assume that this event
-            // should live in the room timeline
-            return {
-                shouldLiveInRoom: true,
-                shouldLiveInThread: false,
-            };
         }
+
+        // We've exhausted all scenarios, can safely assume that this event
+        // should live in the room timeline
+        return {
+            shouldLiveInRoom: true,
+            shouldLiveInThread: false,
+        };
     }
 
-    public partitionThreadedEvents(events: MatrixEvent[]): [MatrixEvent[], MatrixEvent[]] {
-        // Indices to the events array, for readibility
+    public partitionThreadedEvents(events: MatrixEvent[]): [
+        timelineEvents: MatrixEvent[],
+        threadedEvents: MatrixEvent[],
+    ] {
+        // Indices to the events array, for readability
         const ROOM = 0;
         const THREAD = 1;
         if (this.supportsExperimentalThreads()) {

src/event-mapper.ts

@@ -29,9 +29,22 @@ export function eventMapperFor(client: MatrixClient, options: MapperOpts): Event
     const decrypt = options.decrypt !== false;
 
     function mapper(plainOldJsObject: Partial<IEvent>) {
-        const event = new MatrixEvent(plainOldJsObject);
+        const room = client.getRoom(plainOldJsObject.room_id);
+
+        let event: MatrixEvent;
+        // If the event is already known to the room, let's re-use the model rather than duplicating.
+        // We avoid doing this to state events as they may be forward or backwards looking which tweaks behaviour.
+        if (room && plainOldJsObject.state_key === undefined) {
+            event = room.findEventById(plainOldJsObject.event_id);
+        }
+
+        if (!event || event.status) {
+            event = new MatrixEvent(plainOldJsObject);
+        } else {
+            // merge the latest unsigned data from the server
+            event.setUnsigned({ ...event.getUnsigned(), ...plainOldJsObject.unsigned });
+        }
 
-        const room = client.getRoom(event.getRoomId());
         if (room?.threads.has(event.getId())) {
             event.setThread(room.threads.get(event.getId()));
         }

src/models/event.ts

@@ -279,7 +279,7 @@ export class MatrixEvent extends TypedEventEmitter<EmittedEvents, MatrixEventHan
     public target: RoomMember = null;
     public status: EventStatus = null;
     public error: MatrixError = null;
-    public forwardLooking = true;
+    public forwardLooking = true; // only state events may be backwards looking
 
     /* If the event is a `m.key.verification.request` (or to_device `m.key.verification.start`) event,
      * `Crypto` will set this the `VerificationRequest` for the event

src/models/room.ts

@@ -47,6 +47,7 @@ import {
 } from "./thread";
 import { Method } from "../http-api";
 import { TypedEventEmitter } from "./typed-event-emitter";
+import { IMinimalEvent } from "../sync-accumulator";
 
 // These constants are used as sane defaults when the homeserver doesn't support
 // the m.room_versions capability. In practice, KNOWN_SAFE_ROOM_VERSION should be
@@ -1002,17 +1003,15 @@ export class Room extends TypedEventEmitter<EmittedEvents, RoomEventHandlerMap>
     }
 
     /**
-     * Get an event which is stored in our unfiltered timeline set or in a thread
+     * Get an event which is stored in our unfiltered timeline set, or in a thread
      *
-     * @param {string} eventId  event ID to look for
+     * @param {string} eventId event ID to look for
      * @return {?module:models/event.MatrixEvent} the given event, or undefined if unknown
      */
     public findEventById(eventId: string): MatrixEvent | undefined {
         let event = this.getUnfilteredTimelineSet().findEventById(eventId);
 
-        if (event) {
-            return event;
-        } else {
+        if (!event) {
             const threads = this.getThreads();
             for (let i = 0; i < threads.length; i++) {
                 const thread = threads[i];
@@ -1022,6 +1021,8 @@ export class Room extends TypedEventEmitter<EmittedEvents, RoomEventHandlerMap>
                 }
             }
         }
+
+        return event;
     }
 
     /**
@@ -1201,10 +1202,7 @@ export class Room extends TypedEventEmitter<EmittedEvents, RoomEventHandlerMap>
         timeline: EventTimeline,
         paginationToken?: string,
     ): void {
-        timeline.getTimelineSet().addEventsToTimeline(
-            events, toStartOfTimeline,
-            timeline, paginationToken,
-        );
+        timeline.getTimelineSet().addEventsToTimeline(events, toStartOfTimeline, timeline, paginationToken);
     }
 
     /**
@@ -1501,10 +1499,9 @@ export class Room extends TypedEventEmitter<EmittedEvents, RoomEventHandlerMap>
         } else {
             const events = [event];
             let rootEvent = this.findEventById(event.threadRootId);
-            // If the rootEvent does not exist in the current sync, then look for
-            // it over the network
+            // If the rootEvent does not exist in the current sync, then look for it over the network.
             try {
-                let eventData;
+                let eventData: IMinimalEvent;
                 if (event.threadRootId) {
                     eventData = await this.client.fetchRoomEvent(this.roomId, event.threadRootId);
                 }
@@ -1515,11 +1512,13 @@ export class Room extends TypedEventEmitter<EmittedEvents, RoomEventHandlerMap>
                     rootEvent.setUnsigned(eventData.unsigned);
                 }
             } finally {
-                // The root event might be not be visible to the person requesting
-                // it. If it wasn't fetched successfully the thread will work
-                // in "limited" mode and won't benefit from all the APIs a homeserver
-                // can provide to enhance the thread experience
+                // The root event might be not be visible to the person requesting it.
+                // If it wasn't fetched successfully the thread will work in "limited" mode and won't
+                // benefit from all the APIs a homeserver can provide to enhance the thread experience
                 thread = this.createThread(rootEvent, events, toStartOfTimeline);
+                if (thread) {
+                    rootEvent.setThread(thread);
+                }
             }
         }
 
@@ -1578,7 +1577,7 @@ export class Room extends TypedEventEmitter<EmittedEvents, RoomEventHandlerMap>
         }
     }
 
-    applyRedaction(event: MatrixEvent): void {
+    private applyRedaction(event: MatrixEvent): void {
         if (event.isRedaction()) {
             const redactId = event.event.redacts;
 
@@ -1794,6 +1793,14 @@ export class Room extends TypedEventEmitter<EmittedEvents, RoomEventHandlerMap>
         }
     }
 
+    private shouldAddEventToMainTimeline(thread: Thread, event: MatrixEvent): boolean {
+        if (!thread) {
+            return true;
+        }
+
+        return !event.isThreadRelation && thread.id === event.getAssociatedId();
+    }
+
     /**
      * Used to aggregate the local echo for a relation, and also
      * for re-applying a relation after it's redaction has been cancelled,
@@ -1806,11 +1813,9 @@ export class Room extends TypedEventEmitter<EmittedEvents, RoomEventHandlerMap>
      */
     private aggregateNonLiveRelation(event: MatrixEvent): void {
         const thread = this.findThreadForEvent(event);
-        if (thread) {
-            thread.timelineSet.aggregateRelations(event);
-        }
+        thread?.timelineSet.aggregateRelations(event);
 
-        if (thread?.id === event.getAssociatedId() || !thread) {
+        if (this.shouldAddEventToMainTimeline(thread, event)) {
             // TODO: We should consider whether this means it would be a better
             // design to lift the relations handling up to the room instead.
             for (let i = 0; i < this.timelineSets.length; i++) {
@@ -1867,11 +1872,9 @@ export class Room extends TypedEventEmitter<EmittedEvents, RoomEventHandlerMap>
         localEvent.handleRemoteEcho(remoteEvent.event);
 
         const thread = this.findThreadForEvent(remoteEvent);
-        if (thread) {
-            thread.timelineSet.handleRemoteEcho(localEvent, oldEventId, newEventId);
-        }
+        thread?.timelineSet.handleRemoteEcho(localEvent, oldEventId, newEventId);
 
-        if (thread?.id === remoteEvent.getAssociatedId() || !thread) {
+        if (this.shouldAddEventToMainTimeline(thread, remoteEvent)) {
             for (let i = 0; i < this.timelineSets.length; i++) {
                 const timelineSet = this.timelineSets[i];
 
@@ -1938,10 +1941,9 @@ export class Room extends TypedEventEmitter<EmittedEvents, RoomEventHandlerMap>
             event.replaceLocalEventId(newEventId);
 
             const thread = this.findThreadForEvent(event);
-            if (thread) {
-                thread.timelineSet.replaceEventId(oldEventId, newEventId);
-            }
-            if (thread?.id === event.getAssociatedId() || !thread) {
+            thread?.timelineSet.replaceEventId(oldEventId, newEventId);
+
+            if (this.shouldAddEventToMainTimeline(thread, event)) {
                 // if the event was already in the timeline (which will be the case if
                 // opts.pendingEventOrdering==chronological), we need to update the
                 // timeline map.
@@ -1952,12 +1954,10 @@ export class Room extends TypedEventEmitter<EmittedEvents, RoomEventHandlerMap>
         } else if (newStatus == EventStatus.CANCELLED) {
             // remove it from the pending event list, or the timeline.
             if (this.pendingEventList) {
-                const idx = this.pendingEventList.findIndex(ev => ev.getId() === oldEventId);
-                if (idx !== -1) {
-                    const [removedEvent] = this.pendingEventList.splice(idx, 1);
-                    if (removedEvent.isRedaction()) {
-                        this.revertRedactionLocalEcho(removedEvent);
-                    }
+                const removedEvent = this.getPendingEvent(oldEventId);
+                this.removePendingEvent(oldEventId);
+                if (removedEvent.isRedaction()) {
+                    this.revertRedactionLocalEcho(removedEvent);
                 }
             }
             this.removeEvent(oldEventId);

src/models/thread.ts

@@ -93,14 +93,9 @@ export class Thread extends TypedEventEmitter<EmittedEvents, EventHandlerMap> {
             RoomEvent.TimelineReset,
         ]);
 
-        // If we weren't able to find the root event, it's probably missing
+        // If we weren't able to find the root event, it's probably missing,
         // and we define the thread ID from one of the thread relation
-        if (!rootEvent) {
-            this.id = opts?.initialEvents
-                ?.find(event => event.isThreadRelation)?.relationEventId;
-        } else {
-            this.id = rootEvent.getId();
-        }
+        this.id = rootEvent?.getId() ?? opts?.initialEvents?.find(event => event.isThreadRelation)?.relationEventId;
         this.initialiseThread(this.rootEvent);
 
         opts?.initialEvents?.forEach(event => this.addEvent(event, false));
@@ -221,6 +216,7 @@ export class Thread extends TypedEventEmitter<EmittedEvents, EventHandlerMap> {
 
             const event = new MatrixEvent(bundledRelationship.latest_event);
             this.setEventMetadata(event);
+            event.setThread(this);
             this.lastEvent = event;
         }
     }
@@ -253,6 +249,11 @@ export class Thread extends TypedEventEmitter<EmittedEvents, EventHandlerMap> {
      * Finds an event by ID in the current thread
      */
     public findEventById(eventId: string) {
+        // Check the lastEvent as it may have been created based on a bundled relationship and not in a timeline
+        if (this.lastEvent?.getId() === eventId) {
+            return this.lastEvent;
+        }
+
         return this.timelineSet.findEventById(eventId);
     }