Prevent unnecessary m.direct updates (#9805)

* Prevent unnecessary m.direct updates

Signed-off-by: Michael Weimann <[email protected]>

* Replace object with Map

* Clean up comment; simplify code

* Fix rte flaky test (#9811)

* Snap in PiP widget when content changed (#9797)

* Check modified at the end of setDMRoom

* Revert "Snap in PiP widget when content changed (#9797)"

This reverts commit be1e575.

* Revert "Fix rte flaky test (#9811)"

This reverts commit 20d9eb3.

* Update src/Rooms.ts

Co-authored-by: Richard van der Hoff <[email protected]>

Signed-off-by: Michael Weimann <[email protected]>
Co-authored-by: Florian Duros <[email protected]>
Co-authored-by: Richard van der Hoff <[email protected]>

src/Rooms.ts

@@ -57,42 +57,47 @@ export function guessAndSetDMRoom(room: Room, isDirect: boolean): Promise<void>
 /**
  * Marks or unmarks the given room as being as a DM room.
  * @param {string} roomId The ID of the room to modify
- * @param {string} userId The user ID of the desired DM
- room target user or null to un-mark
- this room as a DM room
+ * @param {string | null} userId The user ID of the desired DM room target user or
+ *                        null to un-mark this room as a DM room
  * @returns {object} A promise
  */
-export async function setDMRoom(roomId: string, userId: string): Promise<void> {
+export async function setDMRoom(roomId: string, userId: string | null): Promise<void> {
     if (MatrixClientPeg.get().isGuest()) return;
 
     const mDirectEvent = MatrixClientPeg.get().getAccountData(EventType.Direct);
-    let dmRoomMap = {};
+    const currentContent = mDirectEvent?.getContent() || {};
 
-    if (mDirectEvent !== undefined) dmRoomMap = { ...mDirectEvent.getContent() }; // copy as we will mutate
+    const dmRoomMap = new Map(Object.entries(currentContent));
+    let modified = false;
 
     // remove it from the lists of any others users
     // (it can only be a DM room for one person)
-    for (const thisUserId of Object.keys(dmRoomMap)) {
-        const roomList = dmRoomMap[thisUserId];
+    for (const thisUserId of dmRoomMap.keys()) {
+        const roomList = dmRoomMap.get(thisUserId) || [];
 
         if (thisUserId != userId) {
             const indexOfRoom = roomList.indexOf(roomId);
             if (indexOfRoom > -1) {
                 roomList.splice(indexOfRoom, 1);
+                modified = true;
             }
         }
     }
 
     // now add it, if it's not already there
     if (userId) {
-        const roomList = dmRoomMap[userId] || [];
+        const roomList = dmRoomMap.get(userId) || [];
         if (roomList.indexOf(roomId) == -1) {
             roomList.push(roomId);
+            modified = true;
         }
-        dmRoomMap[userId] = roomList;
+        dmRoomMap.set(userId, roomList);
     }
 
-    await MatrixClientPeg.get().setAccountData(EventType.Direct, dmRoomMap);
+    // prevent unnecessary calls to setAccountData
+    if (!modified) return;
+
+    await MatrixClientPeg.get().setAccountData(EventType.Direct, Object.fromEntries(dmRoomMap));
 }
 
 /**

test/Rooms-test.ts

@@ -0,0 +1,151 @@
+/*
+Copyright 2022 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 { EventType, MatrixClient, MatrixEvent } from "matrix-js-sdk/src/matrix";
+
+import { setDMRoom } from "../src/Rooms";
+import { mkEvent, stubClient } from "./test-utils";
+
+describe("setDMRoom", () => {
+    const userId1 = "@user1:example.com";
+    const userId2 = "@user2:example.com";
+    const userId3 = "@user3:example.com";
+    const roomId1 = "!room1:example.com";
+    const roomId2 = "!room2:example.com";
+    const roomId3 = "!room3:example.com";
+    const roomId4 = "!room4:example.com";
+    let client: MatrixClient;
+
+    beforeEach(() => {
+        client = mocked(stubClient());
+        client.getAccountData = jest.fn().mockImplementation((eventType: string): MatrixEvent | undefined => {
+            if (eventType === EventType.Direct) {
+                return mkEvent({
+                    event: true,
+                    content: {
+                        [userId1]: [roomId1, roomId2],
+                        [userId2]: [roomId3],
+                    },
+                    type: EventType.Direct,
+                    user: client.getSafeUserId(),
+                });
+            }
+
+            return undefined;
+        });
+    });
+
+    describe("when logged in as a guest and marking a room as DM", () => {
+        beforeEach(() => {
+            mocked(client.isGuest).mockReturnValue(true);
+            setDMRoom(roomId1, userId1);
+        });
+
+        it("should not update the account data", () => {
+            expect(client.setAccountData).not.toHaveBeenCalled();
+        });
+    });
+
+    describe("when adding a new room to an existing DM relation", () => {
+        beforeEach(() => {
+            setDMRoom(roomId4, userId1);
+        });
+
+        it("should update the account data accordingly", () => {
+            expect(client.setAccountData).toHaveBeenCalledWith(EventType.Direct, {
+                [userId1]: [roomId1, roomId2, roomId4],
+                [userId2]: [roomId3],
+            });
+        });
+    });
+
+    describe("when adding a new DM room", () => {
+        beforeEach(() => {
+            setDMRoom(roomId4, userId3);
+        });
+
+        it("should update the account data accordingly", () => {
+            expect(client.setAccountData).toHaveBeenCalledWith(EventType.Direct, {
+                [userId1]: [roomId1, roomId2],
+                [userId2]: [roomId3],
+                [userId3]: [roomId4],
+            });
+        });
+    });
+
+    describe("when trying to add a DM, that already exists", () => {
+        beforeEach(() => {
+            setDMRoom(roomId1, userId1);
+        });
+
+        it("should not update the account data", () => {
+            expect(client.setAccountData).not.toHaveBeenCalled();
+        });
+    });
+
+    describe("when removing an existing DM", () => {
+        beforeEach(() => {
+            setDMRoom(roomId1, null);
+        });
+
+        it("should update the account data accordingly", () => {
+            expect(client.setAccountData).toHaveBeenCalledWith(EventType.Direct, {
+                [userId1]: [roomId2],
+                [userId2]: [roomId3],
+            });
+        });
+    });
+
+    describe("when removing an unknown room", () => {
+        beforeEach(() => {
+            setDMRoom(roomId4, null);
+        });
+
+        it("should not update the account data", () => {
+            expect(client.setAccountData).not.toHaveBeenCalled();
+        });
+    });
+
+    describe("when the direct event is undefined", () => {
+        beforeEach(() => {
+            mocked(client.getAccountData).mockReturnValue(undefined);
+            setDMRoom(roomId1, userId1);
+        });
+
+        it("should update the account data accordingly", () => {
+            expect(client.setAccountData).toHaveBeenCalledWith(EventType.Direct, {
+                [userId1]: [roomId1],
+            });
+        });
+    });
+
+    describe("when the current content is undefined", () => {
+        beforeEach(() => {
+            // @ts-ignore
+            mocked(client.getAccountData).mockReturnValue({
+                getContent: jest.fn(),
+            });
+            setDMRoom(roomId1, userId1);
+        });
+
+        it("should update the account data accordingly", () => {
+            expect(client.setAccountData).toHaveBeenCalledWith(EventType.Direct, {
+                [userId1]: [roomId1],
+            });
+        });
+    });
+});