From 1f703b8898c67c7b03b37fdb4dd1b4aea7e2a099 Mon Sep 17 00:00:00 2001 From: kegsay Date: Thu, 1 Dec 2022 12:21:56 +0000 Subject: [PATCH] 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 | 20 +++++++++++-- test/Notifier-test.ts | 66 ++++++++++++++++++++++++++++++++----------- 2 files changed, 66 insertions(+), 20 deletions(-) diff --git a/src/Notifier.ts b/src/Notifier.ts index bc7bae71c4..d545984eb9 100644 --- a/src/Notifier.ts +++ b/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); diff --git a/test/Notifier-test.ts b/test/Notifier-test.ts index f15e798426..c09cc03baa 100644 --- a/test/Notifier-test.ts +++ b/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 { MatrixEvent } from "matrix-js-sdk/src/models/event"; +import { Room, RoomEvent } from "matrix-js-sdk/src/models/room"; +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; let testRoom: Room; let accountDataEventKey: string; - let accountDataStore = {}; + let accountDataStore: Record = {}; let mockSettings: Record = {}; 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; };