From 03dd0b90c837253050ba98b9cab5de1ef462338e Mon Sep 17 00:00:00 2001 From: Kegan Dougal Date: Wed, 30 Nov 2022 17:32:06 +0000 Subject: [PATCH 01/10] 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 --- src/Notifier.ts | 12 +++++++++--- test/Notifier-test.ts | 25 ++++++++++++++++--------- 2 files changed, 25 insertions(+), 12 deletions(-) diff --git a/src/Notifier.ts b/src/Notifier.ts index bc7bae71c4a..3d5e5c03a53 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,12 @@ 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; // on notify for new things, not old. if (!this.isSyncing) return; // don't alert for any messages initially if (ev.getSender() === MatrixClientPeg.get().getUserId()) return; diff --git a/test/Notifier-test.ts b/test/Notifier-test.ts index f15e7984267..9cf7b5ce790 100644 --- a/test/Notifier-test.ts +++ b/test/Notifier-test.ts @@ -16,7 +16,7 @@ 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 { SyncState } from "matrix-js-sdk/src/sync"; import { waitFor } from "@testing-library/react"; @@ -66,6 +66,13 @@ describe("Notifier", () => { const userId = "@bob:example.org"; + const emitLiveEvent = (event: MatrixEvent) => { + 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,7 @@ 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(); @@ -175,7 +182,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 +190,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 +203,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 +219,7 @@ describe("Notifier", () => { }); mockClient!.emit(ClientEvent.Sync, SyncState.Syncing, null); - mockClient!.emit(ClientEvent.Event, event); + emitLiveEvent(event); // desktop notification created expect(MockPlatform.displayNotification).toHaveBeenCalled(); @@ -267,7 +274,7 @@ describe("Notifier", () => { notify: true, tweaks: {}, }); - + Notifier.start(); Notifier.onSyncStateChange(SyncState.Syncing); }); @@ -283,7 +290,7 @@ describe("Notifier", () => { content: {}, event: true, }); - Notifier.onEvent(callEvent); + emitLiveEvent(callEvent); return callEvent; }; From 253502b5cff13f7cbe452ff3fa7cfdc85a6007bd Mon Sep 17 00:00:00 2001 From: Kegan Dougal Date: Wed, 30 Nov 2022 17:43:54 +0000 Subject: [PATCH 02/10] tsc strict checks maybe --- test/Notifier-test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/Notifier-test.ts b/test/Notifier-test.ts index 9cf7b5ce790..65cdf9043e6 100644 --- a/test/Notifier-test.ts +++ b/test/Notifier-test.ts @@ -66,7 +66,7 @@ describe("Notifier", () => { const userId = "@bob:example.org"; - const emitLiveEvent = (event: MatrixEvent) => { + const emitLiveEvent = (event: MatrixEvent): void => { mockClient!.emit(RoomEvent.Timeline, event, testRoom, false, false, { liveEvent: true, timeline: testRoom.getLiveTimeline(), From 3ce5447f11c5315ac2675ecb0934b6fa6beedbe0 Mon Sep 17 00:00:00 2001 From: Kegan Dougal Date: Wed, 30 Nov 2022 17:53:36 +0000 Subject: [PATCH 03/10] More types? --- test/Notifier-test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/Notifier-test.ts b/test/Notifier-test.ts index 65cdf9043e6..a827bb76c17 100644 --- a/test/Notifier-test.ts +++ b/test/Notifier-test.ts @@ -60,7 +60,7 @@ describe("Notifier", () => { let mockClient: MockedObject; let testRoom: Room; let accountDataEventKey: string; - let accountDataStore = {}; + let accountDataStore: Record = {}; let mockSettings: Record = {}; From b8c219c88565d5fb46be0afed387a7aefef3e846 Mon Sep 17 00:00:00 2001 From: kegsay Date: Wed, 30 Nov 2022 18:30:18 +0000 Subject: [PATCH 04/10] Update src/Notifier.ts Co-authored-by: Michael Telatynski <7t3chguy@gmail.com> --- src/Notifier.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/Notifier.ts b/src/Notifier.ts index 3d5e5c03a53..8fe4dd1ef3d 100644 --- a/src/Notifier.ts +++ b/src/Notifier.ts @@ -369,7 +369,10 @@ export const Notifier = { } }, - onEvent: function(this: typeof Notifier, ev: MatrixEvent, room: Room | undefined, + onEvent: function( + this: typeof Notifier, + ev: MatrixEvent, + room: Room | undefined, toStartOfTimeline: boolean | undefined, removed: boolean, data: IRoomTimelineData, From 328e1c8f15685e3a5026a8a2dd47d3d1f154f40f Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Thu, 1 Dec 2022 04:09:36 +0000 Subject: [PATCH 05/10] Update src/Notifier.ts --- src/Notifier.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Notifier.ts b/src/Notifier.ts index 8fe4dd1ef3d..7b38321b5ca 100644 --- a/src/Notifier.ts +++ b/src/Notifier.ts @@ -370,9 +370,9 @@ export const Notifier = { }, onEvent: function( - this: typeof Notifier, - ev: MatrixEvent, - room: Room | undefined, + this: typeof Notifier, + ev: MatrixEvent, + room: Room | undefined, toStartOfTimeline: boolean | undefined, removed: boolean, data: IRoomTimelineData, From 434ca897b880dce4c87ac0cacf1fe7d0830ef193 Mon Sep 17 00:00:00 2001 From: Kegan Dougal Date: Thu, 1 Dec 2022 09:44:00 +0000 Subject: [PATCH 06/10] fix LL test; review comments --- src/Notifier.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/Notifier.ts b/src/Notifier.ts index 7b38321b5ca..d545984eb9e 100644 --- a/src/Notifier.ts +++ b/src/Notifier.ts @@ -377,7 +377,7 @@ export const Notifier = { removed: boolean, data: IRoomTimelineData, ) { - if (!data.liveEvent) return; // on notify for new things, not old. + 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; @@ -437,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); From 1713eecd889b1d5e6368776332036fbb3b1530d7 Mon Sep 17 00:00:00 2001 From: Kegan Dougal Date: Thu, 1 Dec 2022 10:23:52 +0000 Subject: [PATCH 07/10] More tests --- test/Notifier-test.ts | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/test/Notifier-test.ts b/test/Notifier-test.ts index a827bb76c17..312e60236dd 100644 --- a/test/Notifier-test.ts +++ b/test/Notifier-test.ts @@ -173,6 +173,29 @@ describe("Notifier", () => { 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 events with no Room objects', () => { + mockClient!.emit(ClientEvent.Sync, SyncState.Syncing, null); + mockClient.getRoom.mockReturnValue(null); + mockClient!.emit(RoomEvent.Timeline, event, null, false, false, { + liveEvent: true, + timeline: testRoom.getLiveTimeline(), + }); + + expect(MockPlatform.displayNotification).not.toHaveBeenCalled(); + expect(MockPlatform.loudNotification).not.toHaveBeenCalled(); + }); + it('does not create notifications when event does not have notify push action', () => { mockClient.getPushActionsForEvent.mockReturnValue({ notify: false, From c3294bea333003cf0578014005fdc56ee9d8697e Mon Sep 17 00:00:00 2001 From: Kegan Dougal Date: Thu, 1 Dec 2022 10:44:54 +0000 Subject: [PATCH 08/10] More tsc strict checks.. --- test/Notifier-test.ts | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/test/Notifier-test.ts b/test/Notifier-test.ts index 312e60236dd..6ff24140df8 100644 --- a/test/Notifier-test.ts +++ b/test/Notifier-test.ts @@ -17,7 +17,7 @@ limitations under the License. import { mocked, MockedObject } from "jest-mock"; import { ClientEvent, MatrixClient } from "matrix-js-sdk/src/client"; 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"; @@ -184,10 +184,10 @@ describe("Notifier", () => { expect(MockPlatform.loudNotification).not.toHaveBeenCalled(); }); - it('does not create notifications for events with no Room objects', () => { + 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, null, false, false, { + mockClient!.emit(RoomEvent.Timeline, event, testRoom, false, false, { liveEvent: true, timeline: testRoom.getLiveTimeline(), }); @@ -252,11 +252,12 @@ describe("Notifier", () => { }); describe("_displayPopupNotification", () => { - it.each([ + const testCases: {event: IContent, 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 }) => { mockClient.setAccountData(accountDataEventKey, event); Notifier._displayPopupNotification(testEvent, testRoom); expect(MockPlatform.displayNotification).toHaveBeenCalledTimes(count); From c0318157303a335236678a5ca85e08e3676d1b3c Mon Sep 17 00:00:00 2001 From: Kegan Dougal Date: Thu, 1 Dec 2022 11:46:14 +0000 Subject: [PATCH 09/10] More strict ts.. --- test/Notifier-test.ts | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/test/Notifier-test.ts b/test/Notifier-test.ts index 6ff24140df8..686c1e037fe 100644 --- a/test/Notifier-test.ts +++ b/test/Notifier-test.ts @@ -258,7 +258,7 @@ describe("Notifier", () => { { event: undefined, count: 1 }, ]; it.each(testCases)("does not dispatch when notifications are silenced", ({ event, count }) => { - mockClient.setAccountData(accountDataEventKey, event); + mockClient.setAccountData(accountDataEventKey, event!); Notifier._displayPopupNotification(testEvent, testRoom); expect(MockPlatform.displayNotification).toHaveBeenCalledTimes(count); }); @@ -274,16 +274,17 @@ describe("Notifier", () => { }); describe("_playAudioNotification", () => { - it.each([ + const testCases: {event: IContent, 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); }); From 7f4bd00138b1ad56b0c7ec5029fc4f0675f14de7 Mon Sep 17 00:00:00 2001 From: Kegan Dougal Date: Thu, 1 Dec 2022 11:53:20 +0000 Subject: [PATCH 10/10] More ts --- test/Notifier-test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/Notifier-test.ts b/test/Notifier-test.ts index 686c1e037fe..c09cc03baac 100644 --- a/test/Notifier-test.ts +++ b/test/Notifier-test.ts @@ -252,7 +252,7 @@ describe("Notifier", () => { }); describe("_displayPopupNotification", () => { - const testCases: {event: IContent, count: number}[] = [ + const testCases: {event: IContent | undefined, count: number}[] = [ { event: { is_silenced: true }, count: 0 }, { event: { is_silenced: false }, count: 1 }, { event: undefined, count: 1 }, @@ -274,7 +274,7 @@ describe("Notifier", () => { }); describe("_playAudioNotification", () => { - const testCases: {event: IContent, count: number}[] = [ + const testCases: {event: IContent | undefined, count: number}[] = [ { event: { is_silenced: true }, count: 0 }, { event: { is_silenced: false }, count: 1 }, { event: undefined, count: 1 },