TAC: Order rooms by most recent after notification level (#12329)

* Order room by thread timestamp

* Fix key errors in test

* Update jest snapshots

* Update snapshots

* Rename alpha/beta to numbers

* Add playwright test

playwright/e2e/spaces/threads-activity-centre/index.ts

@@ -30,30 +30,30 @@ import { ElementAppPage } from "../../../pages/ElementAppPage";
  * - Invite the bot to both rooms and ensure that it has joined
  */
 export const test = base.extend<{
-    roomAlphaName?: string;
+    room1Name?: string;
-    roomAlpha: { name: string; roomId: string };
+    room1: { name: string; roomId: string };
-    roomBetaName?: string;
+    room2Name?: string;
-    roomBeta: { name: string; roomId: string };
+    room2: { name: string; roomId: string };
     msg: MessageBuilder;
     util: Helpers;
 }>({
     displayName: "Mae",
     botCreateOpts: { displayName: "Other User" },
 
-    roomAlphaName: "Room Alpha",
+    room1Name: "Room 1",
-    roomAlpha: async ({ roomAlphaName: name, app, user, bot }, use) => {
+    room1: async ({ room1Name: name, app, user, bot }, use) => {
         const roomId = await app.client.createRoom({ name, invite: [bot.credentials.userId] });
         await use({ name, roomId });
     },
-    roomBetaName: "Room Beta",
+    room2Name: "Room 2",
-    roomBeta: async ({ roomBetaName: name, app, user, bot }, use) => {
+    room2: async ({ room2Name: name, app, user, bot }, use) => {
         const roomId = await app.client.createRoom({ name, invite: [bot.credentials.userId] });
         await use({ name, roomId });
     },
     msg: async ({ page, app, util }, use) => {
         await use(new MessageBuilder(page, app, util));
     },
-    util: async ({ roomAlpha, roomBeta, page, app, bot }, use) => {
+    util: async ({ room1, room2, page, app, bot }, use) => {
         await use(new Helpers(page, app, bot));
     },
 });
@@ -337,23 +337,27 @@ export class Helpers {
      * @param room1
      * @param room2
      * @param msg - MessageBuilder
+     * @param hasMention - whether to include a mention in the first message
      */
     async populateThreads(
         room1: { name: string; roomId: string },
         room2: { name: string; roomId: string },
         msg: MessageBuilder,
+        hasMention = true,
     ) {
-        await this.receiveMessages(room2, [
+        if (hasMention) {
-            "Msg1",
+            await this.receiveMessages(room2, [
-            msg.threadedOff("Msg1", {
+                "Msg1",
-                "body": "User",
+                msg.threadedOff("Msg1", {
-                "format": "org.matrix.custom.html",
+                    "body": "User",
-                "formatted_body": "<a href='https://matrix.to/#/@user:localhost'>User</a>",
+                    "format": "org.matrix.custom.html",
-                "m.mentions": {
+                    "formatted_body": "<a href='https://matrix.to/#/@user:localhost'>User</a>",
-                    user_ids: ["@user:localhost"],
+                    "m.mentions": {
-                },
+                        user_ids: ["@user:localhost"],
-            }),
+                    },
-        ]);
+                }),
+            ]);
+        }
         await this.receiveMessages(room2, ["Msg2", msg.threadedOff("Msg2", "Resp2")]);
         await this.receiveMessages(room1, ["Msg3", msg.threadedOff("Msg3", "Resp3")]);
     }

playwright/e2e/spaces/threads-activity-centre/threadsActivityCentre.spec.ts

@@ -35,7 +35,7 @@ test.describe("Threads Activity Centre", () => {
         await expect(util.getSpacePanel()).toMatchScreenshot("tac-button-expanded.png");
     });
 
-    test("should not show indicator when there is no thread", async ({ roomAlpha: room1, util }) => {
+    test("should not show indicator when there is no thread", async ({ room1, util }) => {
         // No indicator should be shown
         await util.assertNoTacIndicator();
 
@@ -46,11 +46,7 @@ test.describe("Threads Activity Centre", () => {
         await util.assertNoTacIndicator();
     });
 
-    test("should show a notification indicator when there is a message in a thread", async ({
+    test("should show a notification indicator when there is a message in a thread", async ({ room1, util, msg }) => {
-        roomAlpha: room1,
-        util,
-        msg,
-    }) => {
         await util.goTo(room1);
         await util.receiveMessages(room1, ["Msg1", msg.threadedOff("Msg1", "Resp1")]);
 
@@ -58,11 +54,7 @@ test.describe("Threads Activity Centre", () => {
         await util.assertNotificationTac();
     });
 
-    test("should show a highlight indicator when there is a mention in a thread", async ({
+    test("should show a highlight indicator when there is a mention in a thread", async ({ room1, util, msg }) => {
-        roomAlpha: room1,
-        util,
-        msg,
-    }) => {
         await util.goTo(room1);
         await util.receiveMessages(room1, [
             "Msg1",
@@ -80,7 +72,7 @@ test.describe("Threads Activity Centre", () => {
         await util.assertHighlightIndicator();
     });
 
-    test("should show the rooms with unread threads", async ({ roomAlpha: room1, roomBeta: room2, util, msg }) => {
+    test("should show the rooms with unread threads", async ({ room1, room2, util, msg }) => {
         await util.goTo(room2);
         await util.populateThreads(room1, room2, msg);
         // The indicator should be shown
@@ -97,7 +89,7 @@ test.describe("Threads Activity Centre", () => {
         await expect(util.getTacPanel()).toMatchScreenshot("tac-panel-mix-unread.png");
     });
 
-    test("should update with a thread is read", async ({ roomAlpha: room1, roomBeta: room2, util, msg }) => {
+    test("should update with a thread is read", async ({ room1, room2, util, msg }) => {
         await util.goTo(room2);
         await util.populateThreads(room1, room2, msg);
 
@@ -120,6 +112,17 @@ test.describe("Threads Activity Centre", () => {
         await expect(util.getTacPanel()).toMatchScreenshot("tac-panel-notification-unread.png");
     });
 
+    test("should order by recency after notification level", async ({ room1, room2, util, msg }) => {
+        await util.goTo(room2);
+        await util.populateThreads(room1, room2, msg, false);
+
+        await util.openTac();
+        await util.assertRoomsInTac([
+            { room: room1.name, notificationLevel: "notification" },
+            { room: room2.name, notificationLevel: "notification" },
+        ]);
+    });
+
     test("should block the Spotlight to open when the TAC is opened", async ({ util, page }) => {
         const toggleSpotlight = () => page.keyboard.press(`${CommandOrControl}+k`);
 

src/components/views/spaces/threads-activity-centre/useUnreadThreadRooms.ts

@@ -89,12 +89,12 @@ function computeUnreadThreadRooms(mxClient: MatrixClient, msc3946ProcessDynamicP
     const visibleRooms = mxClient.getVisibleRooms(msc3946ProcessDynamicPredecessor);
 
     let greatestNotificationLevel = NotificationLevel.None;
-    const rooms = [];
+    const rooms: Result["rooms"] = [];
 
     for (const room of visibleRooms) {
         // We only care about rooms with unread threads
         if (VisibilityProvider.instance.isRoomVisible(room) && doesRoomHaveUnreadThreads(room)) {
-            // Get the greatest notification level of all rooms
+            // Get the greatest notification level of all threads
             const notificationLevel = getThreadNotificationLevel(room);
 
             // If the room has an activity notification or less, we ignore it
@@ -110,20 +110,35 @@ function computeUnreadThreadRooms(mxClient: MatrixClient, msc3946ProcessDynamicP
         }
     }
 
-    const sortedRooms = rooms.sort((a, b) => sortRoom(a.notificationLevel, b.notificationLevel));
+    const sortedRooms = rooms.sort((a, b) => sortRoom(a, b));
     return { greatestNotificationLevel, rooms: sortedRooms };
 }
 
+/**
+ * Store the room and its thread notification level
+ */
+type RoomData = Result["rooms"][0];
+
 /**
  * Sort notification level by the most important notification level to the least important
  * Highlight > Notification > Activity
- * @param notificationLevelA - notification level of room A
+ * If the notification level is the same, we sort by the most recent thread
- * @param notificationLevelB - notification level of room B
+ * @param roomDataA - room and notification level of room A
+ * @param roomDataB - room and notification level of room B
  * @returns {number}
  */
-function sortRoom(notificationLevelA: NotificationLevel, notificationLevelB: NotificationLevel): number {
+function sortRoom(roomDataA: RoomData, roomDataB: RoomData): number {
+    const { notificationLevel: notificationLevelA, room: roomA } = roomDataA;
+    const { notificationLevel: notificationLevelB, room: roomB } = roomDataB;
+
+    const timestampA = roomA.getLastThread()?.events.at(-1)?.getTs();
+    const timestampB = roomB.getLastThread()?.events.at(-1)?.getTs();
+
     // NotificationLevel is a numeric enum, so we can compare them directly
     if (notificationLevelA > notificationLevelB) return -1;
     else if (notificationLevelB > notificationLevelA) return 1;
-    else return 0;
+    // Display most recent first
+    else if (!timestampA) return 1;
+    else if (!timestampB) return -1;
+    else return timestampB - timestampA;
 }

test/components/views/spaces/ThreadsActivityCentre-test.tsx

@@ -59,16 +59,23 @@ describe("ThreadsActivityCentre", () => {
     });
     roomWithActivity.name = "Just activity";
 
-    const roomWithNotif = new Room("!room:server", cli, cli.getSafeUserId(), {
+    const roomWithNotif = new Room("!room2:server", cli, cli.getSafeUserId(), {
         pendingEventOrdering: PendingEventOrdering.Detached,
     });
     roomWithNotif.name = "A notification";
 
-    const roomWithHighlight = new Room("!room:server", cli, cli.getSafeUserId(), {
+    const roomWithHighlight = new Room("!room3:server", cli, cli.getSafeUserId(), {
         pendingEventOrdering: PendingEventOrdering.Detached,
     });
     roomWithHighlight.name = "This is a real highlight";
 
+    const getDefaultThreadArgs = (room: Room) => ({
+        room: room,
+        client: cli,
+        authorId: "@foo:bar",
+        participantUserIds: ["@fee:bar"],
+    });
+
     beforeAll(async () => {
         jest.spyOn(MatrixClientPeg, "get").mockReturnValue(cli);
         jest.spyOn(MatrixClientPeg, "safeGet").mockReturnValue(cli);
@@ -77,26 +84,15 @@ describe("ThreadsActivityCentre", () => {
         jest.spyOn(dmRoomMap, "getUserIdForRoomId");
         jest.spyOn(DMRoomMap, "shared").mockReturnValue(dmRoomMap);
 
-        await populateThread({
+        await populateThread(getDefaultThreadArgs(roomWithActivity));
-            room: roomWithActivity,
-            client: cli,
-            authorId: "@foo:bar",
-            participantUserIds: ["@fee:bar"],
-        });
 
-        const notifThreadInfo = await populateThread({
+        const notifThreadInfo = await populateThread(getDefaultThreadArgs(roomWithNotif));
-            room: roomWithNotif,
-            client: cli,
-            authorId: "@foo:bar",
-            participantUserIds: ["@fee:bar"],
-        });
         roomWithNotif.setThreadUnreadNotificationCount(notifThreadInfo.thread.id, NotificationCountType.Total, 1);
 
         const highlightThreadInfo = await populateThread({
-            room: roomWithHighlight,
+            ...getDefaultThreadArgs(roomWithHighlight),
-            client: cli,
+            // timestamp
-            authorId: "@foo:bar",
+            ts: 5,
-            participantUserIds: ["@fee:bar"],
         });
         roomWithHighlight.setThreadUnreadNotificationCount(
             highlightThreadInfo.thread.id,
@@ -181,6 +177,52 @@ describe("ThreadsActivityCentre", () => {
         expect(screen.getByRole("menu")).toMatchSnapshot();
     });
 
+    it("should order the room with the same notification level by most recent", async () => {
+        // Generate two new rooms with threads
+        const secondRoomWithHighlight = new Room("!room4:server", cli, cli.getSafeUserId(), {
+            pendingEventOrdering: PendingEventOrdering.Detached,
+        });
+        secondRoomWithHighlight.name = "This is a second real highlight";
+
+        const secondHighlightThreadInfo = await populateThread({
+            ...getDefaultThreadArgs(secondRoomWithHighlight),
+            // timestamp
+            ts: 1,
+        });
+        secondRoomWithHighlight.setThreadUnreadNotificationCount(
+            secondHighlightThreadInfo.thread.id,
+            NotificationCountType.Highlight,
+            1,
+        );
+
+        const thirdRoomWithHighlight = new Room("!room5:server", cli, cli.getSafeUserId(), {
+            pendingEventOrdering: PendingEventOrdering.Detached,
+        });
+        thirdRoomWithHighlight.name = "This is a third real highlight";
+
+        const thirdHighlightThreadInfo = await populateThread({
+            ...getDefaultThreadArgs(thirdRoomWithHighlight),
+            // timestamp
+            ts: 7,
+        });
+        thirdRoomWithHighlight.setThreadUnreadNotificationCount(
+            thirdHighlightThreadInfo.thread.id,
+            NotificationCountType.Highlight,
+            1,
+        );
+
+        cli.getVisibleRooms = jest
+            .fn()
+            .mockReturnValue([roomWithHighlight, secondRoomWithHighlight, thirdRoomWithHighlight]);
+
+        renderTAC();
+        await userEvent.click(getTACButton());
+
+        // The room should be ordered by the most recent thread
+        // thirdHighlightThreadInfo (timestamp 7) > highlightThreadInfo (timestamp 5) > secondHighlightThreadInfo (timestamp 1)
+        expect(screen.getByRole("menu")).toMatchSnapshot();
+    });
+
     it("should block Ctrl/CMD + k shortcut", async () => {
         cli.getVisibleRooms = jest.fn().mockReturnValue([roomWithHighlight]);
 

test/components/views/spaces/__snapshots__/ThreadsActivityCentre-test.tsx.snap

@@ -38,7 +38,7 @@ exports[`ThreadsActivityCentre renders notifications matching the snapshot 1`] =
       >
         <span
           class="_avatar_mcap2_17 mx_BaseAvatar _avatar-imageless_mcap2_61"
-          data-color="6"
+          data-color="3"
           data-testid="avatar-img"
           data-type="round"
           role="presentation"
@@ -86,7 +86,7 @@ exports[`ThreadsActivityCentre renders notifications matching the snapshot 1`] =
       >
         <span
           class="_avatar_mcap2_17 mx_BaseAvatar _avatar-imageless_mcap2_61"
-          data-color="6"
+          data-color="2"
           data-testid="avatar-img"
           data-type="round"
           role="presentation"
@@ -158,3 +158,176 @@ exports[`ThreadsActivityCentre should match snapshot when empty 1`] = `
   </div>
 </div>
 `;
+
+exports[`ThreadsActivityCentre should order the room with the same notification level by most recent 1`] = `
+<div
+  aria-labelledby="radix-31"
+  aria-orientation="vertical"
+  class="_menu_1x5h1_17"
+  data-align="end"
+  data-orientation="vertical"
+  data-radix-menu-content=""
+  data-side="right"
+  data-state="open"
+  dir="ltr"
+  id="radix-32"
+  role="menu"
+  style="outline: none; --radix-dropdown-menu-content-transform-origin: var(--radix-popper-transform-origin); --radix-dropdown-menu-content-available-width: var(--radix-popper-available-width); --radix-dropdown-menu-content-available-height: var(--radix-popper-available-height); --radix-dropdown-menu-trigger-width: var(--radix-popper-anchor-width); --radix-dropdown-menu-trigger-height: var(--radix-popper-anchor-height); pointer-events: auto;"
+  tabindex="-1"
+>
+  <h3
+    class="_typography_yh5dq_162 _font-body-sm-semibold_yh5dq_45 _title_1x5h1_83"
+    id=":r8:"
+  >
+    Threads activity
+  </h3>
+  <div
+    class="mx_ThreadsActivityCentre_rows"
+  >
+    <button
+      class="mx_ThreadsActivityCentreRow _item_1gwvj_17 _interactive_1gwvj_36"
+      data-kind="primary"
+      data-orientation="vertical"
+      data-radix-collection-item=""
+      role="menuitem"
+      tabindex="-1"
+    >
+      <div
+        class="_icon_1gwvj_44"
+      >
+        <span
+          class="_avatar_mcap2_17 mx_BaseAvatar _avatar-imageless_mcap2_61"
+          data-color="5"
+          data-testid="avatar-img"
+          data-type="round"
+          role="presentation"
+          style="--cpd-avatar-size: 32px;"
+        >
+          T
+        </span>
+      </div>
+      <span
+        class="_typography_yh5dq_162 _font-body-md-medium_yh5dq_69 _label_1gwvj_53"
+      >
+        This is a third real highlight
+      </span>
+      <svg
+        aria-hidden="true"
+        class="_nav-hint_1gwvj_60"
+        fill="currentColor"
+        height="24"
+        viewBox="8 0 8 24"
+        width="8"
+        xmlns="http://www.w3.org/2000/svg"
+      >
+        <path
+          d="M8.7 17.3a.948.948 0 0 1-.275-.7.95.95 0 0 1 .275-.7l3.9-3.9-3.9-3.9a.948.948 0 0 1-.275-.7.95.95 0 0 1 .275-.7.948.948 0 0 1 .7-.275.95.95 0 0 1 .7.275l4.6 4.6c.1.1.17.208.213.325.041.117.062.242.062.375s-.02.258-.063.375a.876.876 0 0 1-.212.325l-4.6 4.6a.948.948 0 0 1-.7.275.948.948 0 0 1-.7-.275Z"
+        />
+      </svg>
+      <div
+        class="mx_NotificationBadge mx_NotificationBadge_visible mx_NotificationBadge_level_highlight mx_NotificationBadge_dot"
+      >
+        <span
+          class="mx_NotificationBadge_count"
+        />
+      </div>
+    </button>
+    <button
+      class="mx_ThreadsActivityCentreRow _item_1gwvj_17 _interactive_1gwvj_36"
+      data-kind="primary"
+      data-orientation="vertical"
+      data-radix-collection-item=""
+      role="menuitem"
+      tabindex="-1"
+    >
+      <div
+        class="_icon_1gwvj_44"
+      >
+        <span
+          class="_avatar_mcap2_17 mx_BaseAvatar _avatar-imageless_mcap2_61"
+          data-color="3"
+          data-testid="avatar-img"
+          data-type="round"
+          role="presentation"
+          style="--cpd-avatar-size: 32px;"
+        >
+          T
+        </span>
+      </div>
+      <span
+        class="_typography_yh5dq_162 _font-body-md-medium_yh5dq_69 _label_1gwvj_53"
+      >
+        This is a real highlight
+      </span>
+      <svg
+        aria-hidden="true"
+        class="_nav-hint_1gwvj_60"
+        fill="currentColor"
+        height="24"
+        viewBox="8 0 8 24"
+        width="8"
+        xmlns="http://www.w3.org/2000/svg"
+      >
+        <path
+          d="M8.7 17.3a.948.948 0 0 1-.275-.7.95.95 0 0 1 .275-.7l3.9-3.9-3.9-3.9a.948.948 0 0 1-.275-.7.95.95 0 0 1 .275-.7.948.948 0 0 1 .7-.275.95.95 0 0 1 .7.275l4.6 4.6c.1.1.17.208.213.325.041.117.062.242.062.375s-.02.258-.063.375a.876.876 0 0 1-.212.325l-4.6 4.6a.948.948 0 0 1-.7.275.948.948 0 0 1-.7-.275Z"
+        />
+      </svg>
+      <div
+        class="mx_NotificationBadge mx_NotificationBadge_visible mx_NotificationBadge_level_highlight mx_NotificationBadge_dot"
+      >
+        <span
+          class="mx_NotificationBadge_count"
+        />
+      </div>
+    </button>
+    <button
+      class="mx_ThreadsActivityCentreRow _item_1gwvj_17 _interactive_1gwvj_36"
+      data-kind="primary"
+      data-orientation="vertical"
+      data-radix-collection-item=""
+      role="menuitem"
+      tabindex="-1"
+    >
+      <div
+        class="_icon_1gwvj_44"
+      >
+        <span
+          class="_avatar_mcap2_17 mx_BaseAvatar _avatar-imageless_mcap2_61"
+          data-color="4"
+          data-testid="avatar-img"
+          data-type="round"
+          role="presentation"
+          style="--cpd-avatar-size: 32px;"
+        >
+          T
+        </span>
+      </div>
+      <span
+        class="_typography_yh5dq_162 _font-body-md-medium_yh5dq_69 _label_1gwvj_53"
+      >
+        This is a second real highlight
+      </span>
+      <svg
+        aria-hidden="true"
+        class="_nav-hint_1gwvj_60"
+        fill="currentColor"
+        height="24"
+        viewBox="8 0 8 24"
+        width="8"
+        xmlns="http://www.w3.org/2000/svg"
+      >
+        <path
+          d="M8.7 17.3a.948.948 0 0 1-.275-.7.95.95 0 0 1 .275-.7l3.9-3.9-3.9-3.9a.948.948 0 0 1-.275-.7.95.95 0 0 1 .275-.7.948.948 0 0 1 .7-.275.95.95 0 0 1 .7.275l4.6 4.6c.1.1.17.208.213.325.041.117.062.242.062.375s-.02.258-.063.375a.876.876 0 0 1-.212.325l-4.6 4.6a.948.948 0 0 1-.7.275.948.948 0 0 1-.7-.275Z"
+        />
+      </svg>
+      <div
+        class="mx_NotificationBadge mx_NotificationBadge_visible mx_NotificationBadge_level_highlight mx_NotificationBadge_dot"
+      >
+        <span
+          class="mx_NotificationBadge_count"
+        />
+      </div>
+    </button>
+  </div>
+</div>
+`;