Merge pull request #9934 from matrix-org/kegan/lists-as-keys

andybalaam · web-flow · commit 51b45551060b · 2023-01-23T15:26:42.000Z
refactor: sliding sync: convert to lists-as-keys rather than indexes
diff --git a/cypress.config.ts b/cypress.config.ts
@@ -30,8 +30,8 @@ export default defineConfig({
         specPattern: "cypress/e2e/**/*.{js,jsx,ts,tsx}",
     },
     env: {
-        // Docker tag to use for `ghcr.io/matrix-org/sliding-sync-proxy` image.
-        SLIDING_SYNC_PROXY_TAG: "v0.6.0",
+        // Docker tag to use for `ghcr.io/matrix-org/sliding-sync` image.
+        SLIDING_SYNC_PROXY_TAG: "v0.99.0-rc1",
         HOMESERVER: "synapse",
     },
     retries: {
diff --git a/cypress/e2e/sliding-sync/sliding-sync.ts b/cypress/e2e/sliding-sync/sliding-sync.ts
@@ -21,8 +21,6 @@ import { MatrixClient } from "matrix-js-sdk/src/matrix";
 import { Interception } from "cypress/types/net-stubbing";
 
 import { HomeserverInstance } from "../../plugins/utils/homeserver";
-import { SettingLevel } from "../../../src/settings/SettingLevel";
-import { Layout } from "../../../src/settings/enums/Layout";
 import { ProxyInstance } from "../../plugins/sliding-sync";
 
 describe("Sliding Sync", () => {
@@ -102,21 +100,6 @@ describe("Sliding Sync", () => {
         });
     };
 
-    // sanity check everything works
-    it("should correctly render expected messages", () => {
-        cy.get<string>("@roomId").then((roomId) => cy.visit("/#/room/" + roomId));
-        cy.setSettingValue("layout", null, SettingLevel.DEVICE, Layout.IRC);
-
-        // Wait until configuration is finished
-        cy.contains(
-            ".mx_RoomView_body .mx_GenericEventListSummary .mx_GenericEventListSummary_summary",
-            "created and configured the room.",
-        );
-
-        // Click "expand" link button
-        cy.get(".mx_GenericEventListSummary_toggle[aria-expanded=false]").click();
-    });
-
     it("should render the Rooms list in reverse chronological order by default and allowing sorting A-Z", () => {
         // create rooms and check room names are correct
         cy.createRoom({ name: "Apple" }).then(() => cy.contains(".mx_RoomSublist", "Apple"));
diff --git a/cypress/plugins/sliding-sync/index.ts b/cypress/plugins/sliding-sync/index.ts
@@ -23,7 +23,7 @@ import { getFreePort } from "../utils/port";
 import { HomeserverInstance } from "../utils/homeserver";
 
 // A cypress plugin to add command to start & stop https://github.com/matrix-org/sliding-sync
-// SLIDING_SYNC_PROXY_TAG env used as the docker tag to use for `ghcr.io/matrix-org/sliding-sync-proxy` image.
+// SLIDING_SYNC_PROXY_TAG env used as the docker tag to use for `ghcr.io/matrix-org/sliding-sync` image.
 
 export interface ProxyInstance {
     containerId: string;
@@ -72,7 +72,7 @@ async function proxyStart(dockerTag: string, homeserver: HomeserverInstance): Pr
     const port = await getFreePort();
     console.log(new Date(), "starting proxy container...", dockerTag);
     const containerId = await dockerRun({
-        image: "ghcr.io/matrix-org/sliding-sync-proxy:" + dockerTag,
+        image: "ghcr.io/matrix-org/sliding-sync:" + dockerTag,
         containerName: "react-sdk-cypress-sliding-sync-proxy",
         params: [
             "--rm",
diff --git a/src/SlidingSyncManager.ts b/src/SlidingSyncManager.ts
@@ -119,12 +119,10 @@ export class SlidingSyncManager {
 
     public slidingSync: SlidingSync;
     private client: MatrixClient;
-    private listIdToIndex: Record<string, number>;
 
     private configureDefer: IDeferred<void>;
 
     public constructor() {
-        this.listIdToIndex = {};
         this.configureDefer = defer<void>();
     }
 
@@ -134,13 +132,18 @@ export class SlidingSyncManager {
 
     public configure(client: MatrixClient, proxyUrl: string): SlidingSync {
         this.client = client;
-        this.listIdToIndex = {};
         // by default use the encrypted subscription as that gets everything, which is a safer
         // default than potentially missing member events.
-        this.slidingSync = new SlidingSync(proxyUrl, [], ENCRYPTED_SUBSCRIPTION, client, SLIDING_SYNC_TIMEOUT_MS);
+        this.slidingSync = new SlidingSync(
+            proxyUrl,
+            new Map(),
+            ENCRYPTED_SUBSCRIPTION,
+            client,
+            SLIDING_SYNC_TIMEOUT_MS,
+        );
         this.slidingSync.addCustomSubscription(UNENCRYPTED_SUBSCRIPTION_NAME, UNENCRYPTED_SUBSCRIPTION);
         // set the space list
-        this.slidingSync.setList(this.getOrAllocateListIndex(SlidingSyncManager.ListSpaces), {
+        this.slidingSync.setList(SlidingSyncManager.ListSpaces, {
             ranges: [[0, 20]],
             sort: ["by_name"],
             slow_get_all_rooms: true,
@@ -173,47 +176,16 @@ export class SlidingSyncManager {
         return this.slidingSync;
     }
 
-    public listIdForIndex(index: number): string | null {
-        for (const listId in this.listIdToIndex) {
-            if (this.listIdToIndex[listId] === index) {
-                return listId;
-            }
-        }
-        return null;
-    }
-
-    /**
-     * Allocate or retrieve the list index for an arbitrary list ID. For example SlidingSyncManager.ListSpaces
-     * @param listId A string which represents the list.
-     * @returns The index to use when registering lists or listening for callbacks.
-     */
-    public getOrAllocateListIndex(listId: string): number {
-        let index = this.listIdToIndex[listId];
-        if (index === undefined) {
-            // assign next highest index
-            index = -1;
-            for (const id in this.listIdToIndex) {
-                const listIndex = this.listIdToIndex[id];
-                if (listIndex > index) {
-                    index = listIndex;
-                }
-            }
-            index++;
-            this.listIdToIndex[listId] = index;
-        }
-        return index;
-    }
-
     /**
      * Ensure that this list is registered.
-     * @param listIndex The list index to register
+     * @param listKey The list key to register
      * @param updateArgs The fields to update on the list.
      * @returns The complete list request params
      */
-    public async ensureListRegistered(listIndex: number, updateArgs: PartialSlidingSyncRequest): Promise<MSC3575List> {
-        logger.debug("ensureListRegistered:::", listIndex, updateArgs);
+    public async ensureListRegistered(listKey: string, updateArgs: PartialSlidingSyncRequest): Promise<MSC3575List> {
+        logger.debug("ensureListRegistered:::", listKey, updateArgs);
         await this.configureDefer.promise;
-        let list = this.slidingSync.getList(listIndex);
+        let list = this.slidingSync.getListParams(listKey);
         if (!list) {
             list = {
                 ranges: [[0, 20]],
@@ -252,14 +224,14 @@ export class SlidingSyncManager {
         try {
             // if we only have range changes then call a different function so we don't nuke the list from before
             if (updateArgs.ranges && Object.keys(updateArgs).length === 1) {
-                await this.slidingSync.setListRanges(listIndex, updateArgs.ranges);
+                await this.slidingSync.setListRanges(listKey, updateArgs.ranges);
             } else {
-                await this.slidingSync.setList(listIndex, list);
+                await this.slidingSync.setList(listKey, list);
             }
         } catch (err) {
             logger.debug("ensureListRegistered: update failed txn_id=", err);
         }
-        return this.slidingSync.getList(listIndex);
+        return this.slidingSync.getListParams(listKey)!;
     }
 
     public async setRoomVisible(roomId: string, visible: boolean): Promise<string> {
@@ -304,7 +276,6 @@ export class SlidingSyncManager {
      */
     public async startSpidering(batchSize: number, gapBetweenRequestsMs: number): Promise<void> {
         await sleep(gapBetweenRequestsMs); // wait a bit as this is called on first render so let's let things load
-        const listIndex = this.getOrAllocateListIndex(SlidingSyncManager.ListSearch);
         let startIndex = batchSize;
         let hasMore = true;
         let firstTime = true;
@@ -316,7 +287,7 @@ export class SlidingSyncManager {
                     [startIndex, endIndex],
                 ];
                 if (firstTime) {
-                    await this.slidingSync.setList(listIndex, {
+                    await this.slidingSync.setList(SlidingSyncManager.ListSearch, {
                         // e.g [0,19] [20,39] then [0,19] [40,59]. We keep [0,20] constantly to ensure
                         // any changes to the list whilst spidering are caught.
                         ranges: ranges,
@@ -342,15 +313,17 @@ export class SlidingSyncManager {
                         },
                     });
                 } else {
-                    await this.slidingSync.setListRanges(listIndex, ranges);
+                    await this.slidingSync.setListRanges(SlidingSyncManager.ListSearch, ranges);
                 }
-                // gradually request more over time
-                await sleep(gapBetweenRequestsMs);
             } catch (err) {
                 // do nothing, as we reject only when we get interrupted but that's fine as the next
                 // request will include our data
+            } finally {
+                // gradually request more over time, even on errors.
+                await sleep(gapBetweenRequestsMs);
             }
-            hasMore = endIndex + 1 < this.slidingSync.getListData(listIndex)?.joinedCount;
+            const listData = this.slidingSync.getListData(SlidingSyncManager.ListSearch)!;
+            hasMore = endIndex + 1 < listData.joinedCount;
             startIndex += batchSize;
             firstTime = false;
         }
diff --git a/src/components/views/rooms/RoomSublist.tsx b/src/components/views/rooms/RoomSublist.tsx
@@ -340,9 +340,8 @@ export default class RoomSublist extends React.Component<IProps, IState> {
 
     private onShowAllClick = async (): Promise<void> => {
         if (this.slidingSyncMode) {
-            const slidingSyncIndex = SlidingSyncManager.instance.getOrAllocateListIndex(this.props.tagId);
             const count = RoomListStore.instance.getCount(this.props.tagId);
-            await SlidingSyncManager.instance.ensureListRegistered(slidingSyncIndex, {
+            await SlidingSyncManager.instance.ensureListRegistered(this.props.tagId, {
                 ranges: [[0, count]],
             });
         }
@@ -566,10 +565,9 @@ export default class RoomSublist extends React.Component<IProps, IState> {
             let isAlphabetical = RoomListStore.instance.getTagSorting(this.props.tagId) === SortAlgorithm.Alphabetic;
             let isUnreadFirst = RoomListStore.instance.getListOrder(this.props.tagId) === ListAlgorithm.Importance;
             if (this.slidingSyncMode) {
-                const slidingSyncIndex = SlidingSyncManager.instance.getOrAllocateListIndex(this.props.tagId);
-                const slidingList = SlidingSyncManager.instance.slidingSync.getList(slidingSyncIndex);
-                isAlphabetical = slidingList.sort[0] === "by_name";
-                isUnreadFirst = slidingList.sort[0] === "by_notification_level";
+                const slidingList = SlidingSyncManager.instance.slidingSync.getListParams(this.props.tagId);
+                isAlphabetical = (slidingList?.sort || [])[0] === "by_name";
+                isUnreadFirst = (slidingList?.sort || [])[0] === "by_notification_level";
             }
 
             // Invites don't get some nonsense options, so only add them if we have to.
diff --git a/src/hooks/useSlidingSyncRoomSearch.ts b/src/hooks/useSlidingSyncRoomSearch.ts
@@ -34,7 +34,6 @@ export const useSlidingSyncRoomSearch = (): {
     const [rooms, setRooms] = useState<Room[]>([]);
 
     const [loading, setLoading] = useState(false);
-    const listIndex = SlidingSyncManager.instance.getOrAllocateListIndex(SlidingSyncManager.ListSearch);
 
     const [updateQuery, updateResult] = useLatestResult<{ term: string; limit?: number }, Room[]>(setRooms);
 
@@ -50,14 +49,16 @@ export const useSlidingSyncRoomSearch = (): {
 
             try {
                 setLoading(true);
-                await SlidingSyncManager.instance.ensureListRegistered(listIndex, {
+                await SlidingSyncManager.instance.ensureListRegistered(SlidingSyncManager.ListSearch, {
                     ranges: [[0, limit]],
                     filters: {
                         room_name_like: term,
                     },
                 });
                 const rooms = [];
-                const { roomIndexToRoomId } = SlidingSyncManager.instance.slidingSync.getListData(listIndex);
+                const { roomIndexToRoomId } = SlidingSyncManager.instance.slidingSync.getListData(
+                    SlidingSyncManager.ListSearch,
+                )!;
                 let i = 0;
                 while (roomIndexToRoomId[i]) {
                     const roomId = roomIndexToRoomId[i];
@@ -78,7 +79,7 @@ export const useSlidingSyncRoomSearch = (): {
                 // TODO: delete the list?
             }
         },
-        [updateQuery, updateResult, listIndex],
+        [updateQuery, updateResult],
     );
 
     return {
diff --git a/src/stores/room-list/SlidingRoomListStore.ts b/src/stores/room-list/SlidingRoomListStore.ts
@@ -84,20 +84,20 @@ export class SlidingRoomListStoreClass extends AsyncStoreWithClient<IState> impl
     public constructor(dis: MatrixDispatcher, private readonly context: SdkContextClass) {
         super(dis);
         this.setMaxListeners(20); // RoomList + LeftPanel + 8xRoomSubList + spares
+        this.stickyRoomId = null;
     }
 
     public async setTagSorting(tagId: TagID, sort: SortAlgorithm): Promise<void> {
         logger.info("SlidingRoomListStore.setTagSorting ", tagId, sort);
         this.tagIdToSortAlgo[tagId] = sort;
-        const slidingSyncIndex = this.context.slidingSyncManager.getOrAllocateListIndex(tagId);
         switch (sort) {
             case SortAlgorithm.Alphabetic:
-                await this.context.slidingSyncManager.ensureListRegistered(slidingSyncIndex, {
+                await this.context.slidingSyncManager.ensureListRegistered(tagId, {
                     sort: SlidingSyncSortToFilter[SortAlgorithm.Alphabetic],
                 });
                 break;
             case SortAlgorithm.Recent:
-                await this.context.slidingSyncManager.ensureListRegistered(slidingSyncIndex, {
+                await this.context.slidingSyncManager.ensureListRegistered(tagId, {
                     sort: SlidingSyncSortToFilter[SortAlgorithm.Recent],
                 });
                 break;
@@ -164,8 +164,7 @@ export class SlidingRoomListStoreClass extends AsyncStoreWithClient<IState> impl
         // check all lists for each tag we know about and see if the room is there
         const tags: TagID[] = [];
         for (const tagId in this.tagIdToSortAlgo) {
-            const index = this.context.slidingSyncManager.getOrAllocateListIndex(tagId);
-            const listData = this.context.slidingSyncManager.slidingSync.getListData(index);
+            const listData = this.context.slidingSyncManager.slidingSync.getListData(tagId);
             if (!listData) {
                 continue;
             }
@@ -251,19 +250,19 @@ export class SlidingRoomListStoreClass extends AsyncStoreWithClient<IState> impl
         }
 
         // now set the rooms
-        const rooms = orderedRoomIds.map((roomId) => {
-            return this.matrixClient.getRoom(roomId);
+        const rooms: Room[] = [];
+        orderedRoomIds.forEach((roomId) => {
+            const room = this.matrixClient.getRoom(roomId);
+            if (!room) {
+                return;
+            }
+            rooms.push(room);
         });
         tagMap[tagId] = rooms;
         this.tagMap = tagMap;
     }
 
-    private onSlidingSyncListUpdate(
-        listIndex: number,
-        joinCount: number,
-        roomIndexToRoomId: Record<number, string>,
-    ): void {
-        const tagId = this.context.slidingSyncManager.listIdForIndex(listIndex);
+    private onSlidingSyncListUpdate(tagId: string, joinCount: number, roomIndexToRoomId: Record<number, string>): void {
         this.counts[tagId] = joinCount;
         this.refreshOrderedLists(tagId, roomIndexToRoomId);
         // let the UI update
@@ -295,8 +294,7 @@ export class SlidingRoomListStoreClass extends AsyncStoreWithClient<IState> impl
             if (room) {
                 // resort it based on the slidingSync view of the list. This may cause this old sticky
                 // room to cease to exist.
-                const index = this.context.slidingSyncManager.getOrAllocateListIndex(tagId);
-                const listData = this.context.slidingSyncManager.slidingSync.getListData(index);
+                const listData = this.context.slidingSyncManager.slidingSync.getListData(tagId);
                 if (!listData) {
                     continue;
                 }
@@ -334,9 +332,8 @@ export class SlidingRoomListStoreClass extends AsyncStoreWithClient<IState> impl
             const sort = SortAlgorithm.Recent; // default to recency sort, TODO: read from config
             this.tagIdToSortAlgo[tagId] = sort;
             this.emit(LISTS_LOADING_EVENT, tagId, true);
-            const index = this.context.slidingSyncManager.getOrAllocateListIndex(tagId);
             this.context.slidingSyncManager
-                .ensureListRegistered(index, {
+                .ensureListRegistered(tagId, {
                     filters: filter,
                     sort: SlidingSyncSortToFilter[sort],
                 })
@@ -361,15 +358,17 @@ export class SlidingRoomListStoreClass extends AsyncStoreWithClient<IState> impl
                     if (roomId === activeSpace) {
                         return;
                     }
+                    if (!filters.spaces) {
+                        filters.spaces = [];
+                    }
                     filters.spaces.push(roomId); // add subspace
                 },
                 false,
             );
 
             this.emit(LISTS_LOADING_EVENT, tagId, true);
-            const index = this.context.slidingSyncManager.getOrAllocateListIndex(tagId);
             this.context.slidingSyncManager
-                .ensureListRegistered(index, {
+                .ensureListRegistered(tagId, {
                     filters: filters,
                 })
                 .then(() => {
diff --git a/test/SlidingSyncManager-test.ts b/test/SlidingSyncManager-test.ts
diff --git a/test/hooks/useSlidingSyncRoomSearch-test.tsx b/test/hooks/useSlidingSyncRoomSearch-test.tsx
diff --git a/test/stores/room-list/SlidingRoomListStore-test.ts b/test/stores/room-list/SlidingRoomListStore-test.ts
