bugfix: fix an issue where the Notifier would incorrectly fire for non-timeline events (#9664)

* bugfix: fix an issue where the Notifier would incorrectly fire for non-timeline events

This was caused by listening for ClientEvent.Event, not RoomEvent.Timeline.
Fixes https://github.com/vector-im/element-web/issues/17263

* tsc strict checks maybe

* More types?

* Update src/Notifier.ts

Co-authored-by: Michael Telatynski <7t3chguy@gmail.com>

* Update src/Notifier.ts

* fix LL test; review comments

* More tests

* More tsc strict checks..

* More strict ts..

* More ts

Co-authored-by: Michael Telatynski <7t3chguy@gmail.com>

src/Notifier.ts

@@ -27,6 +27,7 @@ import {
     PermissionChanged as PermissionChangedEvent,
 } from "@matrix-org/analytics-events/types/typescript/PermissionChanged";
 import { ISyncStateData, SyncState } from "matrix-js-sdk/src/sync";
+import { IRoomTimelineData } from "matrix-js-sdk/src/matrix";
 
 import { MatrixClientPeg } from './MatrixClientPeg';
 import { PosthogAnalytics } from "./PosthogAnalytics";
@@ -217,7 +218,7 @@ export const Notifier = {
         this.boundOnRoomReceipt = this.boundOnRoomReceipt || this.onRoomReceipt.bind(this);
         this.boundOnEventDecrypted = this.boundOnEventDecrypted || this.onEventDecrypted.bind(this);
 
-        MatrixClientPeg.get().on(ClientEvent.Event, this.boundOnEvent);
+        MatrixClientPeg.get().on(RoomEvent.Timeline, this.boundOnEvent);
         MatrixClientPeg.get().on(RoomEvent.Receipt, this.boundOnRoomReceipt);
         MatrixClientPeg.get().on(MatrixEventEvent.Decrypted, this.boundOnEventDecrypted);
         MatrixClientPeg.get().on(ClientEvent.Sync, this.boundOnSyncStateChange);
@@ -227,7 +228,7 @@ export const Notifier = {
 
     stop: function(this: typeof Notifier) {
         if (MatrixClientPeg.get()) {
-            MatrixClientPeg.get().removeListener(ClientEvent.Event, this.boundOnEvent);
+            MatrixClientPeg.get().removeListener(RoomEvent.Timeline, this.boundOnEvent);
             MatrixClientPeg.get().removeListener(RoomEvent.Receipt, this.boundOnRoomReceipt);
             MatrixClientPeg.get().removeListener(MatrixEventEvent.Decrypted, this.boundOnEventDecrypted);
             MatrixClientPeg.get().removeListener(ClientEvent.Sync, this.boundOnSyncStateChange);
@@ -368,7 +369,15 @@ export const Notifier = {
         }
     },
 
-    onEvent: function(this: typeof Notifier, ev: MatrixEvent) {
+    onEvent: function(
+        this: typeof Notifier,
+        ev: MatrixEvent,
+        room: Room | undefined,
+        toStartOfTimeline: boolean | undefined,
+        removed: boolean,
+        data: IRoomTimelineData,
+    ) {
+        if (!data.liveEvent) return; // only notify for new things, not old.
         if (!this.isSyncing) return; // don't alert for any messages initially
         if (ev.getSender() === MatrixClientPeg.get().getUserId()) return;
 
@@ -428,6 +437,11 @@ export const Notifier = {
             }
         }
         const room = MatrixClientPeg.get().getRoom(roomId);
+        if (!room) {
+            // e.g we are in the process of joining a room.
+            // Seen in the cypress lazy-loading test.
+            return;
+        }
 
         const actions = MatrixClientPeg.get().getPushActionsForEvent(ev);
 

test/Notifier-test.ts

@@ -16,8 +16,8 @@ limitations under the License.
 
 import { mocked, MockedObject } from "jest-mock";
 import { ClientEvent, MatrixClient } from "matrix-js-sdk/src/client";
-import { Room } from "matrix-js-sdk/src/models/room";
+import { Room, RoomEvent } from "matrix-js-sdk/src/models/room";
-import { MatrixEvent } from "matrix-js-sdk/src/models/event";
+import { IContent, MatrixEvent } from "matrix-js-sdk/src/models/event";
 import { SyncState } from "matrix-js-sdk/src/sync";
 import { waitFor } from "@testing-library/react";
 
@@ -60,12 +60,19 @@ describe("Notifier", () => {
     let mockClient: MockedObject<MatrixClient>;
     let testRoom: Room;
     let accountDataEventKey: string;
-    let accountDataStore = {};
+    let accountDataStore: Record<string, MatrixEvent | undefined> = {};
 
     let mockSettings: Record<string, boolean> = {};
 
     const userId = "@bob:example.org";
 
+    const emitLiveEvent = (event: MatrixEvent): void => {
+        mockClient!.emit(RoomEvent.Timeline, event, testRoom, false, false, {
+            liveEvent: true,
+            timeline: testRoom.getLiveTimeline(),
+        });
+    };
+
     beforeEach(() => {
         accountDataStore = {};
         mockClient = getMockClientWithEventEmitter({
@@ -150,7 +157,7 @@ describe("Notifier", () => {
         });
 
         it('does not create notifications before syncing has started', () => {
-            mockClient!.emit(ClientEvent.Event, event);
+            emitLiveEvent(event);
 
             expect(MockPlatform.displayNotification).not.toHaveBeenCalled();
             expect(MockPlatform.loudNotification).not.toHaveBeenCalled();
@@ -160,7 +167,30 @@ describe("Notifier", () => {
             const ownEvent = new MatrixEvent({ sender: userId });
 
             mockClient!.emit(ClientEvent.Sync, SyncState.Syncing, null);
-            mockClient!.emit(ClientEvent.Event, ownEvent);
+            emitLiveEvent(ownEvent);
+
+            expect(MockPlatform.displayNotification).not.toHaveBeenCalled();
+            expect(MockPlatform.loudNotification).not.toHaveBeenCalled();
+        });
+
+        it('does not create notifications for non-live events (scrollback)', () => {
+            mockClient!.emit(ClientEvent.Sync, SyncState.Syncing, null);
+            mockClient!.emit(RoomEvent.Timeline, event, testRoom, false, false, {
+                liveEvent: false,
+                timeline: testRoom.getLiveTimeline(),
+            });
+
+            expect(MockPlatform.displayNotification).not.toHaveBeenCalled();
+            expect(MockPlatform.loudNotification).not.toHaveBeenCalled();
+        });
+
+        it('does not create notifications for rooms which cannot be obtained via client.getRoom', () => {
+            mockClient!.emit(ClientEvent.Sync, SyncState.Syncing, null);
+            mockClient.getRoom.mockReturnValue(null);
+            mockClient!.emit(RoomEvent.Timeline, event, testRoom, false, false, {
+                liveEvent: true,
+                timeline: testRoom.getLiveTimeline(),
+            });
 
             expect(MockPlatform.displayNotification).not.toHaveBeenCalled();
             expect(MockPlatform.loudNotification).not.toHaveBeenCalled();
@@ -175,7 +205,7 @@ describe("Notifier", () => {
             });
 
             mockClient!.emit(ClientEvent.Sync, SyncState.Syncing, null);
-            mockClient!.emit(ClientEvent.Event, event);
+            emitLiveEvent(event);
 
             expect(MockPlatform.displayNotification).not.toHaveBeenCalled();
             expect(MockPlatform.loudNotification).not.toHaveBeenCalled();
@@ -183,7 +213,7 @@ describe("Notifier", () => {
 
         it('creates desktop notification when enabled', () => {
             mockClient!.emit(ClientEvent.Sync, SyncState.Syncing, null);
-            mockClient!.emit(ClientEvent.Event, event);
+            emitLiveEvent(event);
 
             expect(MockPlatform.displayNotification).toHaveBeenCalledWith(
                 testRoom.name,
@@ -196,7 +226,7 @@ describe("Notifier", () => {
 
         it('creates a loud notification when enabled', () => {
             mockClient!.emit(ClientEvent.Sync, SyncState.Syncing, null);
-            mockClient!.emit(ClientEvent.Event, event);
+            emitLiveEvent(event);
 
             expect(MockPlatform.loudNotification).toHaveBeenCalledWith(
                 event, testRoom,
@@ -212,7 +242,7 @@ describe("Notifier", () => {
             });
 
             mockClient!.emit(ClientEvent.Sync, SyncState.Syncing, null);
-            mockClient!.emit(ClientEvent.Event, event);
+            emitLiveEvent(event);
 
             // desktop notification created
             expect(MockPlatform.displayNotification).toHaveBeenCalled();
@@ -222,12 +252,13 @@ describe("Notifier", () => {
     });
 
     describe("_displayPopupNotification", () => {
-        it.each([
+        const testCases: {event: IContent | undefined, count: number}[] = [
             { event: { is_silenced: true }, count: 0 },
             { event: { is_silenced: false }, count: 1 },
             { event: undefined, count: 1 },
-        ])("does not dispatch when notifications are silenced", ({ event, count }) => {
+        ];
-            mockClient.setAccountData(accountDataEventKey, event);
+        it.each(testCases)("does not dispatch when notifications are silenced", ({ event, count }) => {
+            mockClient.setAccountData(accountDataEventKey, event!);
             Notifier._displayPopupNotification(testEvent, testRoom);
             expect(MockPlatform.displayNotification).toHaveBeenCalledTimes(count);
         });
@@ -243,16 +274,17 @@ describe("Notifier", () => {
     });
 
     describe("_playAudioNotification", () => {
-        it.each([
+        const testCases: {event: IContent | undefined, count: number}[] = [
             { event: { is_silenced: true }, count: 0 },
             { event: { is_silenced: false }, count: 1 },
             { event: undefined, count: 1 },
-        ])("does not dispatch when notifications are silenced", ({ event, count }) => {
+        ];
+        it.each(testCases)("does not dispatch when notifications are silenced", ({ event, count }) => {
             // It's not ideal to only look at whether this function has been called
             // but avoids starting to look into DOM stuff
             Notifier.getSoundForRoom = jest.fn();
 
-            mockClient.setAccountData(accountDataEventKey, event);
+            mockClient.setAccountData(accountDataEventKey, event!);
             Notifier._playAudioNotification(testEvent, testRoom);
             expect(Notifier.getSoundForRoom).toHaveBeenCalledTimes(count);
         });
@@ -267,7 +299,7 @@ describe("Notifier", () => {
                 notify: true,
                 tweaks: {},
             });
-
+            Notifier.start();
             Notifier.onSyncStateChange(SyncState.Syncing);
         });
 
@@ -283,7 +315,7 @@ describe("Notifier", () => {
                 content: {},
                 event: true,
             });
-            Notifier.onEvent(callEvent);
+            emitLiveEvent(callEvent);
             return callEvent;
         };