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

Hide ignored invites #9255

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all 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
40 changes: 27 additions & 13 deletions src/stores/notifications/RoomNotificationStateStore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -108,32 +108,46 @@ export class RoomNotificationStateStore extends AsyncStoreWithClient<IState> {
return RoomNotificationStateStore.internalInstance;
}

private onSync = (state: SyncState, prevState?: SyncState, data?: ISyncStateData) => {
private onSync = async (state: SyncState, prevState?: SyncState, data?: ISyncStateData) => {
// 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

// Async phase: gather data. Do *not* perform any side-effect.
const globalState = new SummarizedNotificationState();
const visibleRooms = this.matrixClient.getVisibleRooms();

let numFavourites = 0;
for (const room of visibleRooms) {
if (VisibilityProvider.instance.isRoomVisible(room)) {
if (await VisibilityProvider.instance.isRoomVisible(room)) {
globalState.add(this.getRoomState(room));

if (room.tags[DefaultTagID.Favourite] && !room.getType()) numFavourites++;
}
}

PosthogAnalytics.instance.setProperty("numFavouriteRooms", numFavourites);

if (this.globalState.symbol !== globalState.symbol ||
this.globalState.count !== globalState.count ||
this.globalState.color !== globalState.color ||
this.globalState.numUnreadStates !== globalState.numUnreadStates ||
state !== prevState
) {
this._globalState = globalState;
this.emit(UPDATE_STATUS_INDICATOR, globalState, state, prevState, data);
}
// Sync phrase: perform side-effects.
// By making sure that we perform side-effects after the last call to `await`, we make sure that
// the side-effects represent *some* snapshot of reality, rather than a mix of two ore more
// snapshots.
//
// Normally, calls to `VisibilityProvider.instance.isRoomVisible` should resolve in the order
// in which they have been enqueued. As long as this holds, we have guaranteeds that the side-
// effects we're causing correspond to the latest snapshot of reality.
(() => {
// Do NOT make this function `async`.
// Its sole purpose is to make sure that we do not call `await` while performing side-effects.
PosthogAnalytics.instance.setProperty("numFavouriteRooms", numFavourites);

if (this.globalState.symbol !== globalState.symbol ||
this.globalState.count !== globalState.count ||
this.globalState.color !== globalState.color ||
this.globalState.numUnreadStates !== globalState.numUnreadStates ||
state !== prevState
) {
this._globalState = globalState;
this.emit(UPDATE_STATUS_INDICATOR, globalState, state, prevState, data);
}
})();
};

protected async onReady() {
Expand Down
22 changes: 14 additions & 8 deletions src/stores/room-list/RoomListStore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -318,7 +318,7 @@ export class RoomListStoreClass extends AsyncStoreWithClient<IState> implements
await VisibilityProvider.instance.onNewInvitedRoom(room);
}

if (!VisibilityProvider.instance.isRoomVisible(room)) {
if (!await VisibilityProvider.instance.isRoomVisible(room)) {
return; // don't do anything on rooms that aren't visible
}

Expand All @@ -342,7 +342,7 @@ export class RoomListStoreClass extends AsyncStoreWithClient<IState> implements
this.algorithm.updatesInhibited = true;

// Figure out which rooms are about to be valid, and the state of affairs
const rooms = this.getPlausibleRooms();
const rooms = await this.getPlausibleRooms();
const currentSticky = this.algorithm.stickyRoom;
const stickyIsStillPresent = currentSticky && rooms.includes(currentSticky);

Expand Down Expand Up @@ -486,10 +486,16 @@ export class RoomListStoreClass extends AsyncStoreWithClient<IState> implements
this.updateFn.trigger();
};

private getPlausibleRooms(): Room[] {
private async getPlausibleRooms(): Promise<Room[]> {
if (!this.matrixClient) return [];

let rooms = this.matrixClient.getVisibleRooms().filter(r => VisibilityProvider.instance.isRoomVisible(r));
const allRooms = this.matrixClient.getVisibleRooms();
let rooms = [];
for (const room of allRooms) {
if (await VisibilityProvider.instance.isRoomVisible(room)) {
rooms.push(allRooms);
}
}

if (this.prefilterConditions.length > 0) {
rooms = rooms.filter(r => {
Expand All @@ -513,10 +519,10 @@ export class RoomListStoreClass extends AsyncStoreWithClient<IState> implements
* @param trigger Set to false to prevent a list update from being sent. Should only
* be used if the calling code will manually trigger the update.
*/
public regenerateAllLists({ trigger = true }) {
public async regenerateAllLists({ trigger = true }) {
logger.warn("Regenerating all room lists");

const rooms = this.getPlausibleRooms();
const rooms = await this.getPlausibleRooms();

const sorts: ITagSortingMap = {};
const orders: IListOrderingMap = {};
Expand All @@ -528,8 +534,8 @@ export class RoomListStoreClass extends AsyncStoreWithClient<IState> implements
RoomListLayoutStore.instance.ensureLayoutExists(tagId);
}

this.algorithm.populateTags(sorts, orders);
this.algorithm.setKnownRooms(rooms);
await this.algorithm.populateTags(sorts, orders);
await this.algorithm.setKnownRooms(rooms);

this.initialListsGenerated = true;

Expand Down
16 changes: 9 additions & 7 deletions src/stores/room-list/algorithms/Algorithm.ts
Original file line number Diff line number Diff line change
Expand Up @@ -163,18 +163,18 @@ export class Algorithm extends EventEmitter {
this.recalculateActiveCallRooms(tagId);
}

private updateStickyRoom(val: Room) {
this.doUpdateStickyRoom(val);
private async updateStickyRoom(val: Room) {
await this.doUpdateStickyRoom(val);
this._lastStickyRoom = null; // clear to indicate we're done changing
}

private doUpdateStickyRoom(val: Room) {
private async doUpdateStickyRoom(val: Room) {
Comment on lines +166 to +171
Copy link
Member

@t3chguy t3chguy Sep 7, 2022

Choose a reason for hiding this comment

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

These cannot be async, this causes race conditions, please see git history around this file. You would need to introduce a semaphore to prevent updates racing with each other.

Copy link
Member

Choose a reason for hiding this comment

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

e.g. #6391

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ouch. Will do, thanks!

if (val?.isSpaceRoom() && val.getMyMembership() !== "invite") {
// no-op sticky rooms for spaces - they're effectively virtual rooms
val = null;
}

if (val && !VisibilityProvider.instance.isRoomVisible(val)) {
if (val && !await VisibilityProvider.instance.isRoomVisible(val)) {
val = null; // the room isn't visible - lie to the rest of this function
}

Expand Down Expand Up @@ -402,7 +402,7 @@ export class Algorithm extends EventEmitter {
* @param {ITagSortingMap} tagSortingMap The tags to generate.
* @param {IListOrderingMap} listOrderingMap The ordering of those tags.
*/
public populateTags(tagSortingMap: ITagSortingMap, listOrderingMap: IListOrderingMap): void {
public async populateTags(tagSortingMap: ITagSortingMap, listOrderingMap: IListOrderingMap): Promise<void> {
if (!tagSortingMap) throw new Error(`Sorting map cannot be null or empty`);
if (!listOrderingMap) throw new Error(`Ordering ma cannot be null or empty`);
if (arrayHasDiff(Object.keys(tagSortingMap), Object.keys(listOrderingMap))) {
Expand Down Expand Up @@ -442,7 +442,7 @@ export class Algorithm extends EventEmitter {
* previously known information and instead use these rooms instead.
* @param {Room[]} rooms The rooms to force the algorithm to use.
*/
public setKnownRooms(rooms: Room[]): void {
public async setKnownRooms(rooms: Room[]): Promise<void> {
if (isNullOrUndefined(rooms)) throw new Error(`Array of rooms cannot be null`);
if (!this.sortAlgorithms) throw new Error(`Cannot set known rooms without a tag sorting map`);

Expand All @@ -456,7 +456,9 @@ export class Algorithm extends EventEmitter {
// Before we go any further we need to clear (but remember) the sticky room to
// avoid accidentally duplicating it in the list.
const oldStickyRoom = this._stickyRoom;
if (oldStickyRoom) this.updateStickyRoom(null);
if (oldStickyRoom) {
await this.updateStickyRoom(null);
}

this.rooms = rooms;

Expand Down
18 changes: 17 additions & 1 deletion src/stores/room-list/filters/VisibilityProvider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import LegacyCallHandler from "../../../LegacyCallHandler";
import { RoomListCustomisations } from "../../../customisations/RoomList";
import { isLocalRoom } from "../../../utils/localRoom/isLocalRoom";
import VoipUserMapper from "../../../VoipUserMapper";
import { MatrixClientPeg } from "../../../MatrixClientPeg";

export class VisibilityProvider {
private static internalInstance: VisibilityProvider;
Expand All @@ -38,7 +39,7 @@ export class VisibilityProvider {
await VoipUserMapper.sharedInstance().onNewInvitedRoom(room);
}

public isRoomVisible(room?: Room): boolean {
public async isRoomVisible(room?: Room): Promise<boolean> {
Copy link
Member

Choose a reason for hiding this comment

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

This is a breaking change, existing customisations will fail to compile with this change due to the return type changing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What would you suggest?

Copy link
Member

Choose a reason for hiding this comment

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

Move it into the js-sdk as a background operation, so consumers don't have to make all their APIs async. Feels a bit strange for getRuleForInvite to be an async function which actually creates rooms, it seems like that one should be read-only, and something else should create and uphold the policy rooms, emitting "PolicyUpdated" or similar events for consumers to subscribe and react to.

Copy link
Member

Choose a reason for hiding this comment

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

It also looks like getOrCreateTargetRoom is flawed if called multiple times whilst the room is being created, it'll stack multiple creations instead of being idempotent. This could also occur with this PR where multiple operations overlap and call isRoomVisible causing overlapping calls to getOrCreateTargetRoom

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It also looks like getOrCreateTargetRoom is flawed if called multiple times whilst the room is being created, it'll stack multiple creations instead of being idempotent. This could also occur with this PR where multiple operations overlap and call isRoomVisible causing overlapping calls to getOrCreateTargetRoom

You're right, I should at least lock it to avoid that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Move it into the js-sdk as a background operation, so consumers don't have to make all their APIs async. Feels a bit strange for getRuleForInvite to be an async function which actually creates rooms, it seems like that one should be read-only, and something else should create and uphold the policy rooms, emitting "PolicyUpdated" or similar events for consumers to subscribe and react to.

That was the design at some point but I dropped it because it felt like I would be reimplementing a non-trivial subset of the state event store. On the other hand, it is probably needed to achieve any kind of performance anyway, so now might be the right time to revisit this.

I'll think this over.

Copy link
Contributor Author

@Yoric Yoric Sep 8, 2022

Choose a reason for hiding this comment

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

In case someone else reads this, this conversation has spawned a patch in matrix-js-sdk: matrix-org/matrix-js-sdk#2653 .

if (!room) {
return false;
}
Expand All @@ -50,6 +51,21 @@ export class VisibilityProvider {
return false;
}

if (room.getMyMembership() === "invite") {
// Find out whether the invite should be hidden.
const cli = MatrixClientPeg.get();
const myUserId = cli.getUserId();
const inviter = room.currentState.getMember(myUserId);
if (inviter?.events?.member) {
const inviterUserId = inviter.events.member.getSender();
const rule = await cli.ignoredInvites.getRuleForInvite({ roomId: room.roomId, sender: inviterUserId });
if (rule) {
// Indeed, there is a rule that specifies we should hide the invite.
return false;
}
}
}

// hide space rooms as they'll be shown in the SpacePanel
if (room.isSpaceRoom()) {
return false;
Expand Down
6 changes: 3 additions & 3 deletions test/stores/room-list/algorithms/Algorithm-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,14 +38,14 @@ describe("Algorithm", () => {
let client: MockedObject<MatrixClient>;
let algorithm: Algorithm;

beforeEach(() => {
beforeEach(async () => {
stubClient();
client = mocked(MatrixClientPeg.get());
DMRoomMap.makeShared();

algorithm = new Algorithm();
algorithm.start();
algorithm.populateTags(
await algorithm.populateTags(
{ [DefaultTagID.Untagged]: SortAlgorithm.Alphabetic },
{ [DefaultTagID.Untagged]: ListAlgorithm.Natural },
);
Expand Down Expand Up @@ -75,7 +75,7 @@ describe("Algorithm", () => {
client.reEmitter.reEmit(roomWithCall, [RoomStateEvent.Events]);

for (const room of client.getRooms()) jest.spyOn(room, "getMyMembership").mockReturnValue("join");
algorithm.setKnownRooms(client.getRooms());
await algorithm.setKnownRooms(client.getRooms());

setupAsyncStoreWithClient(CallStore.instance, client);
setupAsyncStoreWithClient(WidgetMessagingStore.instance, client);
Expand Down
72 changes: 57 additions & 15 deletions test/stores/room-list/filters/VisibilityProvider-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,9 @@ import LegacyCallHandler from "../../../../src/LegacyCallHandler";
import VoipUserMapper from "../../../../src/VoipUserMapper";
import { LocalRoom, LOCAL_ROOM_ID_PREFIX } from "../../../../src/models/LocalRoom";
import { RoomListCustomisations } from "../../../../src/customisations/RoomList";
import { createTestClient } from "../../../test-utils";
import { createTestClient, IGNORE_INVITES_FROM_THIS_USER, IGNORE_INVITES_TO_THIS_ROOM, stubClient }
from "../../../test-utils";
import { MatrixClientPeg } from "../../../../src/MatrixClientPeg";

jest.mock("../../../../src/VoipUserMapper", () => ({
sharedInstance: jest.fn(),
Expand All @@ -40,9 +42,29 @@ jest.mock("../../../../src/customisations/RoomList", () => ({
},
}));

const createRoom = (isSpaceRoom = false): Room => {
const createRoom = ({ isSpaceRoom, inviter, roomId }: { isSpaceRoom?: boolean, inviter?: string, roomId?: string } =
{ isSpaceRoom: false, roomId: `${Math.random()}:example.org` }): Room => {
return {
roomId,
isSpaceRoom: () => isSpaceRoom,
getMyMembership: () =>
inviter ? "invite" : "join",
currentState: {
getMember(userId: string): any | null {
if (userId != MatrixClientPeg.get().getUserId()) {
return null;
}
return {
events: {
member: {
getSender() {
return inviter;
},
},
},
};
},
},
} as unknown as Room;
};

Expand All @@ -61,6 +83,7 @@ describe("VisibilityProvider", () => {
isVirtualRoom: jest.fn(),
} as unknown as VoipUserMapper;
mocked(VoipUserMapper.sharedInstance).mockReturnValue(mockVoipUserMapper);
stubClient();
});

describe("instance", () => {
Expand All @@ -86,39 +109,58 @@ describe("VisibilityProvider", () => {
mocked(mockVoipUserMapper.isVirtualRoom).mockReturnValue(true);
});

it("should return return false", () => {
it("should return return false", async () => {
const room = createRoom();
expect(VisibilityProvider.instance.isRoomVisible(room)).toBe(false);
expect(await VisibilityProvider.instance.isRoomVisible(room)).toBe(false);
expect(mockVoipUserMapper.isVirtualRoom).toHaveBeenCalledWith(room);
});
});

it("should return false without room", () => {
expect(VisibilityProvider.instance.isRoomVisible()).toBe(false);
it("should return false without room", async () => {
expect(await VisibilityProvider.instance.isRoomVisible()).toBe(false);
});

it("should return false for a space room", () => {
const room = createRoom(true);
expect(VisibilityProvider.instance.isRoomVisible(room)).toBe(false);
it("should return false for a space room", async () => {
const room = createRoom({ isSpaceRoom: true });
expect(await VisibilityProvider.instance.isRoomVisible(room)).toBe(false);
});

it("should return false for a local room", () => {
it("should return false for a local room", async () => {
const room = createLocalRoom();
expect(VisibilityProvider.instance.isRoomVisible(room)).toBe(false);
expect(await VisibilityProvider.instance.isRoomVisible(room)).toBe(false);
});

it("should return false if visibility customisation returns false", () => {
it("should return false if visibility customisation returns false", async () => {
mocked(RoomListCustomisations.isRoomVisible).mockReturnValue(false);
const room = createRoom();
expect(VisibilityProvider.instance.isRoomVisible(room)).toBe(false);
expect(await VisibilityProvider.instance.isRoomVisible(room)).toBe(false);
expect(RoomListCustomisations.isRoomVisible).toHaveBeenCalledWith(room);
});

it("should return true if visibility customisation returns true", () => {
it("should return true if visibility customisation returns true", async () => {
mocked(RoomListCustomisations.isRoomVisible).mockReturnValue(true);
const room = createRoom();
expect(VisibilityProvider.instance.isRoomVisible(room)).toBe(true);
expect(await VisibilityProvider.instance.isRoomVisible(room)).toBe(true);
expect(RoomListCustomisations.isRoomVisible).toHaveBeenCalledWith(room);
});

it("should return true if the room is an invite but hasn't been marked as ignored", async () => {
mocked(RoomListCustomisations.isRoomVisible).mockReturnValue(true);
const room = createRoom({ inviter: "@good-user:example.org" });
expect(await VisibilityProvider.instance.isRoomVisible(room)).toBe(true);
expect(RoomListCustomisations.isRoomVisible).toHaveBeenCalledWith(room);
});

it("should return false if the room is an invite and the sender has been marked as ignored", async () => {
mocked(RoomListCustomisations.isRoomVisible).mockReturnValue(true);
const room = createRoom({ inviter: IGNORE_INVITES_FROM_THIS_USER });
expect(await VisibilityProvider.instance.isRoomVisible(room)).toBe(false);
});

it("should return false if the room is an invite and the roomId has been marked as ignored", async () => {
mocked(RoomListCustomisations.isRoomVisible).mockReturnValue(true);
const room = createRoom({ inviter: "@good-user:example.org", roomId: IGNORE_INVITES_TO_THIS_ROOM });
expect(await VisibilityProvider.instance.isRoomVisible(room)).toBe(false);
});
});
});
20 changes: 20 additions & 0 deletions test/test-utils/test-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,11 @@ export function stubClient() {
MatrixClientBackedSettingsHandler.matrixClient = client;
}

export const IGNORE_INVITES_TO_THIS_ROOM = "$ignore-invites-to-this-room:example.org";
export const IGNORE_INVITES_TO_THIS_ROOM_ISSUER = "@user-who-decided-to-ignore-invites-to-this-room:example.org";
export const IGNORE_INVITES_FROM_THIS_USER = "@ignore-invites-from-this-sender:example.org";
export const IGNORE_INVITES_FROM_THIS_USER_ISSUER = "@user-who-decided-to-ignore-invites-from-this-user:example.org";

/**
* Create a stubbed-out MatrixClient
*
Expand Down Expand Up @@ -174,6 +179,21 @@ export function createTestClient(): MatrixClient {
sendToDevice: jest.fn().mockResolvedValue(undefined),
queueToDevice: jest.fn().mockResolvedValue(undefined),
encryptAndSendToDevices: jest.fn().mockResolvedValue(undefined),
ignoredInvites: {
getRuleForInvite: jest.fn().mockImplementation(({ roomId, sender }) => {
if (roomId === IGNORE_INVITES_TO_THIS_ROOM) {
return Promise.resolve(new MatrixEvent({
sender: IGNORE_INVITES_TO_THIS_ROOM_ISSUER,
}));
}
if (sender === IGNORE_INVITES_FROM_THIS_USER) {
return Promise.resolve(new MatrixEvent({
sender: IGNORE_INVITES_FROM_THIS_USER_ISSUER,
}));
}
return Promise.resolve(null);
}),
},
} as unknown as MatrixClient;

client.reEmitter = new ReEmitter(client);
Expand Down