Merge pull request #5943 from matrix-org/t3chguy/fix/17082

Sort rooms in the add existing to space dialog based on recency

Author: t3chguy

src/components/views/dialogs/AddExistingToSpaceDialog.tsx

@@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-import React, {useContext, useState} from "react";
+import React, {useContext, useMemo, useState} from "react";
 import classNames from "classnames";
 import {Room} from "matrix-js-sdk/src/models/room";
 import {MatrixClient} from "matrix-js-sdk/src/client";
@@ -34,6 +34,7 @@ import DMRoomMap from "../../../utils/DMRoomMap";
 import {calculateRoomVia} from "../../../utils/permalinks/Permalinks";
 import StyledCheckbox from "../elements/StyledCheckbox";
 import MatrixClientContext from "../../../contexts/MatrixClientContext";
+import {sortRooms} from "../../../stores/room-list/algorithms/tag-sorting/RecentAlgorithm";
 
 interface IProps extends IDialogProps {
     matrixClient: MatrixClient;
@@ -57,6 +58,8 @@ interface IAddExistingToSpaceProps {
 
 export const AddExistingToSpace: React.FC<IAddExistingToSpaceProps> = ({ space, selected, onChange }) => {
     const cli = useContext(MatrixClientContext);
+    const visibleRooms = useMemo(() => sortRooms(cli.getVisibleRooms()), [cli]);
+
     const [query, setQuery] = useState("");
     const lcQuery = query.toLowerCase();
 
@@ -65,7 +68,7 @@ export const AddExistingToSpace: React.FC<IAddExistingToSpaceProps> = ({ space,
     const existingRoomsSet = new Set(SpaceStore.instance.getChildRooms(space.roomId));
 
     const joinRule = space.getJoinRule();
-    const [spaces, rooms, dms] = cli.getVisibleRooms().reduce((arr, room) => {
+    const [spaces, rooms, dms] = visibleRooms.reduce((arr, room) => {
         if (room.getMyMembership() !== "join") return arr;
         if (!room.name.toLowerCase().includes(lcQuery)) return arr;
 

src/stores/room-list/algorithms/tag-sorting/RecentAlgorithm.ts

@@ -21,79 +21,83 @@ import { MatrixClientPeg } from "../../../../MatrixClientPeg";
 import * as Unread from "../../../../Unread";
 import { EffectiveMembership, getEffectiveMembership } from "../../../../utils/membership";
 
-/**
- * Sorts rooms according to the last event's timestamp in each room that seems
- * useful to the user.
- */
-export class RecentAlgorithm implements IAlgorithm {
-    public async sortRooms(rooms: Room[], tagId: TagID): Promise<Room[]> {
-        // We cache the timestamp lookup to avoid iterating forever on the timeline
-        // of events. This cache only survives a single sort though.
-        // We wouldn't need this if `.sort()` didn't constantly try and compare all
-        // of the rooms to each other.
-
-        // TODO: We could probably improve the sorting algorithm here by finding changes.
-        // See https://github.com/vector-im/element-web/issues/14459
-        // For example, if we spent a little bit of time to determine which elements have
-        // actually changed (probably needs to be done higher up?) then we could do an
-        // insertion sort or similar on the limited set of changes.
-
-        // TODO: Don't assume we're using the same client as the peg
-        // See https://github.com/vector-im/element-web/issues/14458
-        let myUserId = '';
-        if (MatrixClientPeg.get()) {
-            myUserId = MatrixClientPeg.get().getUserId();
+export const sortRooms = (rooms: Room[]): Room[] => {
+    // We cache the timestamp lookup to avoid iterating forever on the timeline
+    // of events. This cache only survives a single sort though.
+    // We wouldn't need this if `.sort()` didn't constantly try and compare all
+    // of the rooms to each other.
+
+    // TODO: We could probably improve the sorting algorithm here by finding changes.
+    // See https://github.com/vector-im/element-web/issues/14459
+    // For example, if we spent a little bit of time to determine which elements have
+    // actually changed (probably needs to be done higher up?) then we could do an
+    // insertion sort or similar on the limited set of changes.
+
+    // TODO: Don't assume we're using the same client as the peg
+    // See https://github.com/vector-im/element-web/issues/14458
+    let myUserId = '';
+    if (MatrixClientPeg.get()) {
+        myUserId = MatrixClientPeg.get().getUserId();
+    }
+
+    const tsCache: { [roomId: string]: number } = {};
+    const getLastTs = (r: Room) => {
+        if (tsCache[r.roomId]) {
+            return tsCache[r.roomId];
         }
 
-        const tsCache: { [roomId: string]: number } = {};
-        const getLastTs = (r: Room) => {
-            if (tsCache[r.roomId]) {
-                return tsCache[r.roomId];
+        const ts = (() => {
+            // Apparently we can have rooms without timelines, at least under testing
+            // environments. Just return MAX_INT when this happens.
+            if (!r || !r.timeline) {
+                return Number.MAX_SAFE_INTEGER;
             }
 
-            const ts = (() => {
-                // Apparently we can have rooms without timelines, at least under testing
-                // environments. Just return MAX_INT when this happens.
-                if (!r || !r.timeline) {
-                    return Number.MAX_SAFE_INTEGER;
-                }
-
-                // If the room hasn't been joined yet, it probably won't have a timeline to
-                // parse. We'll still fall back to the timeline if this fails, but chances
-                // are we'll at least have our own membership event to go off of.
-                const effectiveMembership = getEffectiveMembership(r.getMyMembership());
-                if (effectiveMembership !== EffectiveMembership.Join) {
-                    const membershipEvent = r.currentState.getStateEvents("m.room.member", myUserId);
-                    if (membershipEvent && !Array.isArray(membershipEvent)) {
-                        return membershipEvent.getTs();
-                    }
+            // If the room hasn't been joined yet, it probably won't have a timeline to
+            // parse. We'll still fall back to the timeline if this fails, but chances
+            // are we'll at least have our own membership event to go off of.
+            const effectiveMembership = getEffectiveMembership(r.getMyMembership());
+            if (effectiveMembership !== EffectiveMembership.Join) {
+                const membershipEvent = r.currentState.getStateEvents("m.room.member", myUserId);
+                if (membershipEvent && !Array.isArray(membershipEvent)) {
+                    return membershipEvent.getTs();
                 }
+            }
 
-                for (let i = r.timeline.length - 1; i >= 0; --i) {
-                    const ev = r.timeline[i];
-                    if (!ev.getTs()) continue; // skip events that don't have timestamps (tests only?)
+            for (let i = r.timeline.length - 1; i >= 0; --i) {
+                const ev = r.timeline[i];
+                if (!ev.getTs()) continue; // skip events that don't have timestamps (tests only?)
 
-                    if (ev.getSender() === myUserId || Unread.eventTriggersUnreadCount(ev)) {
-                        return ev.getTs();
-                    }
+                if (ev.getSender() === myUserId || Unread.eventTriggersUnreadCount(ev)) {
+                    return ev.getTs();
                 }
+            }
 
-                // we might only have events that don't trigger the unread indicator,
-                // in which case use the oldest event even if normally it wouldn't count.
-                // This is better than just assuming the last event was forever ago.
-                if (r.timeline.length && r.timeline[0].getTs()) {
-                    return r.timeline[0].getTs();
-                } else {
-                    return Number.MAX_SAFE_INTEGER;
-                }
-            })();
+            // we might only have events that don't trigger the unread indicator,
+            // in which case use the oldest event even if normally it wouldn't count.
+            // This is better than just assuming the last event was forever ago.
+            if (r.timeline.length && r.timeline[0].getTs()) {
+                return r.timeline[0].getTs();
+            } else {
+                return Number.MAX_SAFE_INTEGER;
+            }
+        })();
+
+        tsCache[r.roomId] = ts;
+        return ts;
+    };
 
-            tsCache[r.roomId] = ts;
-            return ts;
-        };
+    return rooms.sort((a, b) => {
+        return getLastTs(b) - getLastTs(a);
+    });
+};
 
-        return rooms.sort((a, b) => {
-            return getLastTs(b) - getLastTs(a);
-        });
+/**
+ * Sorts rooms according to the last event's timestamp in each room that seems
+ * useful to the user.
+ */
+export class RecentAlgorithm implements IAlgorithm {
+    public async sortRooms(rooms: Room[], tagId: TagID): Promise<Room[]> {
+        return sortRooms(rooms);
     }
 }