Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Support dynamic room predecessors in RoomNotificationStateStore #10297

Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/stores/notifications/RoomNotificationStateStore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import { RoomNotificationState } from "./RoomNotificationState";
import { SummarizedNotificationState } from "./SummarizedNotificationState";
import { VisibilityProvider } from "../room-list/filters/VisibilityProvider";
import { PosthogAnalytics } from "../../PosthogAnalytics";
import SettingsStore from "../../settings/SettingsStore";

interface IState {}

Expand Down Expand Up @@ -96,8 +97,9 @@ export class RoomNotificationStateStore extends AsyncStoreWithClient<IState> {
private onSync = (state: SyncState, prevState: SyncState | null, data?: ISyncStateData): void => {
// Only count visible rooms to not torment the user with notification counts in rooms they can't see.
// This will include highlights from the previous version of the room internally
const msc3946ProcessDynamicPredecessor = SettingsStore.getValue("feature_dynamic_room_predecessors");
const globalState = new SummarizedNotificationState();
const visibleRooms = this.matrixClient.getVisibleRooms();
const visibleRooms = this.matrixClient.getVisibleRooms(msc3946ProcessDynamicPredecessor);
Comment on lines +124 to +126
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This codepath won't be hit if a user is changing the labs flag, as labs are stored in Local Storage and thus won't cause a /sync - so it won't update until the next /sync which for smaller accounts may be a little while away.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you think we should listen for changes to this labs flag and re-run this logic when they happen?

Doesn't that seem like overkill? Most users will hit this button once or twice ever, and, if I were hitting it, I don't think I'd expect it to notify me immediately about a room that became visible because I was switching off dynamic room predecessors.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be overkill but we do so in the other places we check this setting.

image
image

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those places already watched other settings, so the additional complexity was small, and at least in the case of RoomView, it seems useful to re-render them when the labs flag changes.

In RoomNotificationStateStore, we'd be adding the complexity of responding to a settings update while the benefit to the user is questionable. (In fact, I'd argue it would be quite confusing to receive a notification because of changing this labs flag.)

So I do think it's overkill, but happy to discuss further if you disagree.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The benefit to the user may be small, but a benefit to the developer is found too. Consistency and predictable data flow.

(In fact, I'd argue it would be quite confusing to receive a notification because of changing this labs flag.)

But that'd still happen, just on your next /sync so even less predictably.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I guess it's better to make it clearer to the user what caused the ping. The benefit to the developer of yet more listeners and events flying around is still definitely arguable but I take your point there too.


let numFavourites = 0;
for (const room of visibleRooms) {
Expand Down
111 changes: 111 additions & 0 deletions test/stores/RoomNotificationStateStore-test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
/*
Copyright 2023 The Matrix.org Foundation C.I.C.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

import { mocked } from "jest-mock";
import { ClientEvent, MatrixClient, Room } from "matrix-js-sdk/src/matrix";

import { createTestClient, setupAsyncStoreWithClient } from "../test-utils";
import {
RoomNotificationStateStore,
UPDATE_STATUS_INDICATOR,
} from "../../src/stores/notifications/RoomNotificationStateStore";
import SettingsStore from "../../src/settings/SettingsStore";

describe("RoomNotificationStateStore", function () {
let store: RoomNotificationStateStore;
const client: MatrixClient = createTestClient();

beforeEach(() => {
jest.resetAllMocks();
store = RoomNotificationStateStore.instance;
setupAsyncStoreWithClient(store, client);
});

it("Emits no event when a room has no unreads", async () => {
// Given a room with 0 unread messages
const room = fakeRoom(0);
store.emit = jest.fn();

// When we sync and the room is visible
mocked(client.getVisibleRooms).mockReturnValue([room]);
client.emit(ClientEvent.Sync, null, null);

// Then we emit an event from the store
expect(store.emit).not.toHaveBeenCalled();
});

it("Emits an event when a room has unreads", async () => {
// Given a room with 2 unread messages
const room = fakeRoom(2);
store.emit = jest.fn();

// When we sync and the room is visible
mocked(client.getVisibleRooms).mockReturnValue([room]);
client.emit(ClientEvent.Sync, null, null);

// Then we emit an event from the store
expect(store.emit).toHaveBeenCalledWith(UPDATE_STATUS_INDICATOR, expect.anything(), null, null, undefined);
});

describe("If the feature_dynamic_room_predecessors is not enabled", () => {
beforeEach(() => {
// Turn off feature_dynamic_room_predecessors setting
jest.spyOn(SettingsStore, "getValue").mockReturnValue(false);
});

it("Passes the dynamic predecessor flag to getVisibleRooms", async () => {
// When we sync
mocked(client.getVisibleRooms).mockReturnValue([]);
store.emit = jest.fn();
client.emit(ClientEvent.Sync, null, null);

// Then we check visible rooms, using the dynamic predecessor flag
expect(client.getVisibleRooms).toHaveBeenCalledWith(false);
expect(client.getVisibleRooms).not.toHaveBeenCalledWith(true);
});
});

describe("If the feature_dynamic_room_predecessors is enabled", () => {
beforeEach(() => {
// Turn on feature_dynamic_room_predecessors setting
jest.spyOn(SettingsStore, "getValue").mockImplementation(
(settingName) => settingName === "feature_dynamic_room_predecessors",
);
});

it("Passes the dynamic predecessor flag to getVisibleRooms", async () => {
// When we sync
mocked(client.getVisibleRooms).mockReturnValue([]);
store.emit = jest.fn();
client.emit(ClientEvent.Sync, null, null);

// Then we check visible rooms, using the dynamic predecessor flag
expect(client.getVisibleRooms).toHaveBeenCalledWith(true);
expect(client.getVisibleRooms).not.toHaveBeenCalledWith(false);
});
});

let roomIdx = 0;

function fakeRoom(numUnreads: number): Room {
roomIdx++;
const ret = new Room(`room${roomIdx}`, client, "@user:example.com");
ret.getPendingEvents = jest.fn().mockReturnValue([]);
ret.isSpaceRoom = jest.fn().mockReturnValue(false);
ret.getUnreadNotificationCount = jest.fn().mockReturnValue(numUnreads);
return ret;
}
});