Fix handling of remote echoes doubling up (#2639)

* Fix handling of remote echoes doubling up

* Simplify code

* Make TSC strict happier

* Update `timelineWasEmpty` to match type

* Add tests

* Add tests

* Add lowly `!`

spec/unit/event-timeline-set.spec.ts

@@ -16,14 +16,15 @@ limitations under the License.
 
 import * as utils from "../test-utils/test-utils";
 import {
+    DuplicateStrategy,
     EventTimeline,
     EventTimelineSet,
     EventType,
+    Filter,
     MatrixClient,
     MatrixEvent,
     MatrixEventEvent,
     Room,
-    DuplicateStrategy,
 } from '../../src';
 import { Thread } from "../../src/models/thread";
 import { ReEmitter } from "../../src/ReEmitter";
@@ -291,4 +292,34 @@ describe('EventTimelineSet', () => {
             expect(eventTimelineSet.canContain(event)).toBeTruthy();
         });
     });
+
+    describe("handleRemoteEcho", () => {
+        it("should add to liveTimeline only if the event matches the filter", () => {
+            const filter = new Filter(client.getUserId()!, "test_filter");
+            filter.setDefinition({
+                room: {
+                    timeline: {
+                        types: [EventType.RoomMessage],
+                    },
+                },
+            });
+            const eventTimelineSet = new EventTimelineSet(room, { filter }, client);
+
+            const roomMessageEvent = new MatrixEvent({
+                type: EventType.RoomMessage,
+                content: { body: "test" },
+                event_id: "!test1:server",
+            });
+            eventTimelineSet.handleRemoteEcho(roomMessageEvent, "~!local-event-id:server", roomMessageEvent.getId());
+            expect(eventTimelineSet.getLiveTimeline().getEvents()).toContain(roomMessageEvent);
+
+            const roomFilteredEvent = new MatrixEvent({
+                type: "other_event_type",
+                content: { body: "test" },
+                event_id: "!test2:server",
+            });
+            eventTimelineSet.handleRemoteEcho(roomFilteredEvent, "~!local-event-id:server", roomFilteredEvent.getId());
+            expect(eventTimelineSet.getLiveTimeline().getEvents()).not.toContain(roomFilteredEvent);
+        });
+    });
 });

spec/unit/room.spec.ts

@@ -288,11 +288,11 @@ describe("Room", function() {
                 room.addLiveEvents(events);
                 expect(room.currentState.setStateEvents).toHaveBeenCalledWith(
                     [events[0]],
-                    { timelineWasEmpty: undefined },
+                    { timelineWasEmpty: false },
                 );
                 expect(room.currentState.setStateEvents).toHaveBeenCalledWith(
                     [events[1]],
-                    { timelineWasEmpty: undefined },
+                    { timelineWasEmpty: false },
                 );
                 expect(events[0].forwardLooking).toBe(true);
                 expect(events[1].forwardLooking).toBe(true);
@@ -426,6 +426,17 @@ describe("Room", function() {
             // but without the event ID matching we will still have the local event in pending events
             expect(room.getEventForTxnId(txnId)).toBeUndefined();
         });
+
+        it("should correctly handle remote echoes from other devices", () => {
+            const remoteEvent = utils.mkMessage({
+                room: roomId, user: userA, event: true,
+            });
+            remoteEvent.event.unsigned = { transaction_id: "TXN_ID" };
+
+            // add the remoteEvent
+            room.addLiveEvents([remoteEvent]);
+            expect(room.timeline.length).toEqual(1);
+        });
     });
 
     describe('addEphemeralEvents', () => {

src/models/event-timeline-set.ts

@@ -729,18 +729,10 @@ export class EventTimelineSet extends TypedEventEmitter<EmittedEvents, EventTime
         if (existingTimeline) {
             this._eventIdToTimeline.delete(oldEventId);
             this._eventIdToTimeline.set(newEventId, existingTimeline);
-        } else {
-            if (this.filter) {
-                if (this.filter.filterRoomTimeline([localEvent]).length) {
-                    this.addEventToTimeline(localEvent, this.liveTimeline, {
-                        toStartOfTimeline: false,
-                    });
-                }
-            } else {
-                this.addEventToTimeline(localEvent, this.liveTimeline, {
-                    toStartOfTimeline: false,
-                });
-            }
+        } else if (!this.filter || this.filter.filterRoomTimeline([localEvent]).length) {
+            this.addEventToTimeline(localEvent, this.liveTimeline, {
+                toStartOfTimeline: false,
+            });
         }
     }
 

src/models/room.ts

@@ -1981,22 +1981,14 @@ export class Room extends TypedEventEmitter<EmittedEvents, RoomEventHandlerMap>
                 }
             }
         }
-
-        if (event.getUnsigned().transaction_id) {
-            const existingEvent = this.txnToEvent[event.getUnsigned().transaction_id];
-            if (existingEvent) {
-                // remote echo of an event we sent earlier
-                this.handleRemoteEcho(event, existingEvent);
-            }
-        }
     }
 
     /**
      * Add an event to the end of this room's live timelines. Will fire
      * "Room.timeline".
      *
      * @param {MatrixEvent} event Event to be added
-     * @param {IAddLiveEventOptions} options addLiveEvent options
+     * @param {IAddLiveEventOptions} addLiveEventOptions addLiveEvent options
      * @fires module:client~MatrixClient#event:"Room.timeline"
      * @private
      */
@@ -2344,7 +2336,7 @@ export class Room extends TypedEventEmitter<EmittedEvents, RoomEventHandlerMap>
         fromCache = false,
     ): void {
         let duplicateStrategy = duplicateStrategyOrOpts as DuplicateStrategy;
-        let timelineWasEmpty: boolean;
+        let timelineWasEmpty = false;
         if (typeof (duplicateStrategyOrOpts) === 'object') {
             ({
                 duplicateStrategy,
@@ -2383,10 +2375,25 @@ export class Room extends TypedEventEmitter<EmittedEvents, RoomEventHandlerMap>
         const threadRoots = this.findThreadRoots(events);
         const eventsByThread: { [threadId: string]: MatrixEvent[] } = {};
 
+        const options: IAddLiveEventOptions = {
+            duplicateStrategy,
+            fromCache,
+            timelineWasEmpty,
+        };
+
         for (const event of events) {
             // TODO: We should have a filter to say "only add state event types X Y Z to the timeline".
             this.processLiveEvent(event);
 
+            if (event.getUnsigned().transaction_id) {
+                const existingEvent = this.txnToEvent[event.getUnsigned().transaction_id!];
+                if (existingEvent) {
+                    // remote echo of an event we sent earlier
+                    this.handleRemoteEcho(event, existingEvent);
+                    continue; // we can skip adding the event to the timeline sets, it is already there
+                }
+            }
+
             const {
                 shouldLiveInRoom,
                 shouldLiveInThread,
@@ -2399,11 +2406,7 @@ export class Room extends TypedEventEmitter<EmittedEvents, RoomEventHandlerMap>
             eventsByThread[threadId]?.push(event);
 
             if (shouldLiveInRoom) {
-                this.addLiveEvent(event, {
-                    duplicateStrategy,
-                    fromCache,
-                    timelineWasEmpty,
-                });
+                this.addLiveEvent(event, options);
             }
         }