From 43595b62c861e21b95f43fea15216f29cf148733 Mon Sep 17 00:00:00 2001 From: R Midhun Suresh Date: Thu, 20 Mar 2025 13:28:49 +0530 Subject: [PATCH 1/4] Add new hook for sticky room This hook takes the filtered, sorted rooms and returns a new list of rooms such that the active room is kept in the same index even when the list has changes. --- .../roomlist/useIndexForActiveRoom.tsx | 44 ------- .../viewmodels/roomlist/useStickyRoomList.tsx | 117 ++++++++++++++++++ 2 files changed, 117 insertions(+), 44 deletions(-) delete mode 100644 src/components/viewmodels/roomlist/useIndexForActiveRoom.tsx create mode 100644 src/components/viewmodels/roomlist/useStickyRoomList.tsx diff --git a/src/components/viewmodels/roomlist/useIndexForActiveRoom.tsx b/src/components/viewmodels/roomlist/useIndexForActiveRoom.tsx deleted file mode 100644 index 210e0efd0f5..00000000000 --- a/src/components/viewmodels/roomlist/useIndexForActiveRoom.tsx +++ /dev/null @@ -1,44 +0,0 @@ -/* - * Copyright 2025 New Vector Ltd. - * - * SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only OR LicenseRef-Element-Commercial - * Please see LICENSE files in the repository root for full details. - */ - -import { useCallback, useEffect, useState } from "react"; - -import { SdkContextClass } from "../../../contexts/SDKContext"; -import { useDispatcher } from "../../../hooks/useDispatcher"; -import dispatcher from "../../../dispatcher/dispatcher"; -import { Action } from "../../../dispatcher/actions"; -import type { Room } from "matrix-js-sdk/src/matrix"; - -/** - * Tracks the index of the active room in the given array of rooms. - * @param rooms list of rooms - * @returns index of the active room or undefined otherwise. - */ -export function useIndexForActiveRoom(rooms: Room[]): number | undefined { - const [index, setIndex] = useState(undefined); - - const calculateIndex = useCallback( - (newRoomId?: string) => { - const activeRoomId = newRoomId ?? SdkContextClass.instance.roomViewStore.getRoomId(); - const index = rooms.findIndex((room) => room.roomId === activeRoomId); - setIndex(index === -1 ? undefined : index); - }, - [rooms], - ); - - // Re-calculate the index when the active room has changed. - useDispatcher(dispatcher, (payload) => { - if (payload.action === Action.ActiveRoomChanged) calculateIndex(payload.newRoomId); - }); - - // Re-calculate the index when the list of rooms has changed. - useEffect(() => { - calculateIndex(); - }, [calculateIndex, rooms]); - - return index; -} diff --git a/src/components/viewmodels/roomlist/useStickyRoomList.tsx b/src/components/viewmodels/roomlist/useStickyRoomList.tsx new file mode 100644 index 00000000000..faaa1a77e10 --- /dev/null +++ b/src/components/viewmodels/roomlist/useStickyRoomList.tsx @@ -0,0 +1,117 @@ +/* + * Copyright 2025 New Vector Ltd. + * + * SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only OR LicenseRef-Element-Commercial + * Please see LICENSE files in the repository root for full details. + */ + +import { useCallback, useEffect, useState } from "react"; + +import { SdkContextClass } from "../../../contexts/SDKContext"; +import { useDispatcher } from "../../../hooks/useDispatcher"; +import dispatcher from "../../../dispatcher/dispatcher"; +import { Action } from "../../../dispatcher/actions"; +import type { Room } from "matrix-js-sdk/src/matrix"; +import type { Optional } from "matrix-events-sdk"; + +function getIndexByRoomId(rooms: Room[], roomId: Optional): number | undefined { + const index = rooms.findIndex((room) => room.roomId === roomId); + return index === -1 ? undefined : index; +} + +function getRoomsWithStickyRoom( + rooms: Room[], + oldIndex: number | undefined, + newIndex: number | undefined, + isRoomChange: boolean, +): { newRooms: Room[]; newIndex: number | undefined } { + const updated = { newIndex, newRooms: rooms }; + if (isRoomChange) { + /** + * When opening another room, the index should obviously change. + */ + return updated; + } + if (newIndex === undefined || oldIndex === undefined) { + /** + * If oldIndex is undefined, then there was no active room before. + * So nothing to do in regards to sticky room. + * Similarly, if newIndex is undefined, there's no active room anymore. + */ + return updated; + } + if (newIndex === oldIndex) { + /** + * If the index hasn't changed, we have nothing to do. + */ + return updated; + } + if (oldIndex > rooms.length - 1) { + /** + * If the old index falls out of the bounds of the rooms array + * (usually because rooms were removed), we can no longer place + * the active room in the same old index. + */ + return updated; + } + + /** + * Making the active room sticky is as simple as removing it from + * its new index and placing it in the old index. + */ + const newRooms = [...rooms]; + const [newRoom] = newRooms.splice(newIndex, 1); + newRooms.splice(oldIndex, 0, newRoom); + + return { newIndex: oldIndex, newRooms }; +} + +interface StickyRoomListResult { + /** + * List of rooms with sticky active room. + */ + rooms: Room[]; + /** + * Index of the active room in the room list. + */ + activeIndex: number | undefined; +} + +/** + * - Provides a list of rooms such that the active room is sticky i.e the active room is kept + * in the same index even when the order of rooms in the list changes. + * - Provides the index of the active room. + * @param rooms list of rooms + * @see {@link StickyRoomListResult} details what this hook returns.. + */ +export function useStickyRoomList(rooms: Room[]): StickyRoomListResult { + const [listState, setListState] = useState<{ index: number | undefined; roomsWithStickyRoom: Room[] }>({ + index: undefined, + roomsWithStickyRoom: rooms, + }); + + const updateRoomsAndIndex = useCallback( + (newRoomId?: string, isRoomChange: boolean = false) => { + setListState((current) => { + const activeRoomId = newRoomId ?? SdkContextClass.instance.roomViewStore.getRoomId(); + const newActiveIndex = getIndexByRoomId(rooms, activeRoomId); + const oldIndex = current.index; + const { newIndex, newRooms } = getRoomsWithStickyRoom(rooms, oldIndex, newActiveIndex, isRoomChange); + return { index: newIndex, roomsWithStickyRoom: newRooms }; + }); + }, + [rooms], + ); + + // Re-calculate the index when the active room has changed. + useDispatcher(dispatcher, (payload) => { + if (payload.action === Action.ActiveRoomChanged) updateRoomsAndIndex(payload.newRoomId, true); + }); + + // Re-calculate the index when the list of rooms has changed. + useEffect(() => { + updateRoomsAndIndex(); + }, [rooms, updateRoomsAndIndex]); + + return { activeIndex: listState.index, rooms: listState.roomsWithStickyRoom }; +} From 2501c3c9729100c511c4556f38244142c04a666b Mon Sep 17 00:00:00 2001 From: R Midhun Suresh Date: Thu, 20 Mar 2025 13:33:12 +0530 Subject: [PATCH 2/4] Use new hook in view model --- .../viewmodels/roomlist/RoomListViewModel.tsx | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/components/viewmodels/roomlist/RoomListViewModel.tsx b/src/components/viewmodels/roomlist/RoomListViewModel.tsx index 584e436c578..217eaefbd98 100644 --- a/src/components/viewmodels/roomlist/RoomListViewModel.tsx +++ b/src/components/viewmodels/roomlist/RoomListViewModel.tsx @@ -18,7 +18,7 @@ import SpaceStore from "../../../stores/spaces/SpaceStore"; import dispatcher from "../../../dispatcher/dispatcher"; import { Action } from "../../../dispatcher/actions"; import { useMatrixClientContext } from "../../../contexts/MatrixClientContext"; -import { useIndexForActiveRoom } from "./useIndexForActiveRoom"; +import { useStickyRoomList } from "./useStickyRoomList"; export interface RoomListViewState { /** @@ -97,8 +97,14 @@ export interface RoomListViewState { */ export function useRoomListViewModel(): RoomListViewState { const matrixClient = useMatrixClientContext(); - const { primaryFilters, activePrimaryFilter, rooms, activateSecondaryFilter, activeSecondaryFilter } = - useFilteredRooms(); + const { + primaryFilters, + activePrimaryFilter, + rooms: filteredRooms, + activateSecondaryFilter, + activeSecondaryFilter, + } = useFilteredRooms(); + const { activeIndex, rooms } = useStickyRoomList(filteredRooms); const currentSpace = useEventEmitterState( SpaceStore.instance, @@ -107,7 +113,6 @@ export function useRoomListViewModel(): RoomListViewState { ); const canCreateRoom = hasCreateRoomRights(matrixClient, currentSpace); - const activeIndex = useIndexForActiveRoom(rooms); const { activeSortOption, sort } = useSorter(); const { shouldShowMessagePreview, toggleMessagePreview } = useMessagePreviewToggle(); From 9c6d6085ee70e6d0ef8a368116ea29a2aeb68f18 Mon Sep 17 00:00:00 2001 From: R Midhun Suresh Date: Thu, 20 Mar 2025 13:34:02 +0530 Subject: [PATCH 3/4] Add tests --- .../roomlist/RoomListViewModel-test.tsx | 123 +++++++++++++++--- 1 file changed, 104 insertions(+), 19 deletions(-) diff --git a/test/unit-tests/components/viewmodels/roomlist/RoomListViewModel-test.tsx b/test/unit-tests/components/viewmodels/roomlist/RoomListViewModel-test.tsx index f14a7e0acd9..978c0c6fe9d 100644 --- a/test/unit-tests/components/viewmodels/roomlist/RoomListViewModel-test.tsx +++ b/test/unit-tests/components/viewmodels/roomlist/RoomListViewModel-test.tsx @@ -291,34 +291,51 @@ describe("RoomListViewModel", () => { }); }); - describe("active index", () => { - it("should recalculate active index when list of rooms change", () => { + describe("Sticky room and active index", () => { + function expectActiveRoom(vm: ReturnType, i: number, roomId: string) { + expect(vm.activeIndex).toEqual(i); + expect(vm.rooms[i].roomId).toEqual(roomId); + } + + it("active room and active index are retained on order change", () => { const { rooms } = mockAndCreateRooms(); - // Let's say that the first room is the active room initially - jest.spyOn(SdkContextClass.instance.roomViewStore, "getRoomId").mockImplementation(() => rooms[0].roomId); + + // Let's say that the room at index 5 is active + const roomId = rooms[5].roomId; + jest.spyOn(SdkContextClass.instance.roomViewStore, "getRoomId").mockImplementation(() => roomId); const { result: vm } = renderHook(() => useRoomListViewModel()); - expect(vm.current.activeIndex).toEqual(0); + expect(vm.current.activeIndex).toEqual(5); + + // Let's say that room at index 9 moves to index 5 + const room9 = rooms[9]; + rooms.splice(9, 1); + rooms.splice(5, 0, room9); + act(() => RoomListStoreV3.instance.emit(LISTS_UPDATE_EVENT)); + + // Active room index should still be 5 + expectActiveRoom(vm.current, 5, roomId); - // Let's say that a new room is added and that becomes active - const newRoom = mkStubRoom("bar:matrix.org", "Bar", undefined); - jest.spyOn(SdkContextClass.instance.roomViewStore, "getRoomId").mockImplementation(() => newRoom.roomId); - rooms.push(newRoom); + // Let's add 2 new rooms from index 0 + const newRoom1 = mkStubRoom("bar1:matrix.org", "Bar 1", undefined); + const newRoom2 = mkStubRoom("bar2:matrix.org", "Bar 2", undefined); + rooms.unshift(newRoom1, newRoom2); act(() => RoomListStoreV3.instance.emit(LISTS_UPDATE_EVENT)); - // Now the active room should be the last room which we just added - expect(vm.current.activeIndex).toEqual(rooms.length - 1); + // Active room index should still be 5 + expectActiveRoom(vm.current, 5, roomId); }); - it("should recalculate active index when active room changes", () => { + it("active room and active index are updated when another room is opened", () => { const { rooms } = mockAndCreateRooms(); - const { result: vm } = renderHook(() => useRoomListViewModel()); + const roomId = rooms[5].roomId; + jest.spyOn(SdkContextClass.instance.roomViewStore, "getRoomId").mockImplementation(() => roomId); - // No active room yet - expect(vm.current.activeIndex).toBeUndefined(); + const { result: vm } = renderHook(() => useRoomListViewModel()); + expectActiveRoom(vm.current, 5, roomId); - // Let's say that room at index 5 becomes active - const room = rooms[5]; + // Let's say that room at index 9 becomes active + const room = rooms[9]; act(() => { dispatcher.dispatch( { @@ -330,8 +347,76 @@ describe("RoomListViewModel", () => { ); }); - // We expect index 5 to be active now - expect(vm.current.activeIndex).toEqual(5); + // Active room index should change to reflect new room + expectActiveRoom(vm.current, 9, room.roomId); + }); + + it("active room and active index are updated when active index spills out of rooms array bounds", () => { + const { rooms } = mockAndCreateRooms(); + // Let's say that the room at index 5 is active + const roomId = rooms[5].roomId; + jest.spyOn(SdkContextClass.instance.roomViewStore, "getRoomId").mockImplementation(() => roomId); + + const { result: vm } = renderHook(() => useRoomListViewModel()); + expectActiveRoom(vm.current, 5, roomId); + + // Let's say that we remove rooms from the start of the array + for (let i = 0; i < 4; ++i) { + // We should be able to do 4 deletions before we run out of rooms + rooms.splice(0, 1); + act(() => RoomListStoreV3.instance.emit(LISTS_UPDATE_EVENT)); + expectActiveRoom(vm.current, 5, roomId); + } + + // If we remove one more room from the start, there's not going to be enough rooms + // to maintain the active index. + rooms.splice(0, 1); + act(() => RoomListStoreV3.instance.emit(LISTS_UPDATE_EVENT)); + expectActiveRoom(vm.current, 0, roomId); + }); + + it("active room and active index are retained when rooms that appear after the active room are deleted", () => { + const { rooms } = mockAndCreateRooms(); + // Let's say that the room at index 5 is active + const roomId = rooms[5].roomId; + jest.spyOn(SdkContextClass.instance.roomViewStore, "getRoomId").mockImplementation(() => roomId); + + const { result: vm } = renderHook(() => useRoomListViewModel()); + expectActiveRoom(vm.current, 5, roomId); + + // Let's say that we remove rooms from the start of the array + for (let i = 0; i < 4; ++i) { + // Deleting rooms after index 5 (active) should not update the active index + rooms.splice(6, 1); + act(() => RoomListStoreV3.instance.emit(LISTS_UPDATE_EVENT)); + expectActiveRoom(vm.current, 5, roomId); + } + }); + + it("active room index becomes undefined when active room is deleted", () => { + const { rooms } = mockAndCreateRooms(); + // Let's say that the room at index 5 is active + let roomId: string | undefined = rooms[5].roomId; + jest.spyOn(SdkContextClass.instance.roomViewStore, "getRoomId").mockImplementation(() => roomId); + + const { result: vm } = renderHook(() => useRoomListViewModel()); + expectActiveRoom(vm.current, 5, roomId); + + // Let's remove the active room (i.e room at index 5) + rooms.splice(5, 1); + roomId = undefined; + act(() => RoomListStoreV3.instance.emit(LISTS_UPDATE_EVENT)); + expect(vm.current.activeIndex).toBeUndefined(); + }); + + it("active room index is initially undefined", () => { + mockAndCreateRooms(); + + // Let's say that there's no active room currently + jest.spyOn(SdkContextClass.instance.roomViewStore, "getRoomId").mockImplementation(() => undefined); + + const { result: vm } = renderHook(() => useRoomListViewModel()); + expect(vm.current.activeIndex).toEqual(undefined); }); }); }); From cc3bcd07684c5b7acfc27c7873123d3587fd9d72 Mon Sep 17 00:00:00 2001 From: R Midhun Suresh Date: Fri, 21 Mar 2025 17:30:55 +0530 Subject: [PATCH 4/4] Use single * in comments --- .../viewmodels/roomlist/useStickyRoomList.tsx | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/components/viewmodels/roomlist/useStickyRoomList.tsx b/src/components/viewmodels/roomlist/useStickyRoomList.tsx index faaa1a77e10..e8234d14ae0 100644 --- a/src/components/viewmodels/roomlist/useStickyRoomList.tsx +++ b/src/components/viewmodels/roomlist/useStickyRoomList.tsx @@ -27,13 +27,13 @@ function getRoomsWithStickyRoom( ): { newRooms: Room[]; newIndex: number | undefined } { const updated = { newIndex, newRooms: rooms }; if (isRoomChange) { - /** + /* * When opening another room, the index should obviously change. */ return updated; } if (newIndex === undefined || oldIndex === undefined) { - /** + /* * If oldIndex is undefined, then there was no active room before. * So nothing to do in regards to sticky room. * Similarly, if newIndex is undefined, there's no active room anymore. @@ -41,13 +41,13 @@ function getRoomsWithStickyRoom( return updated; } if (newIndex === oldIndex) { - /** + /* * If the index hasn't changed, we have nothing to do. */ return updated; } if (oldIndex > rooms.length - 1) { - /** + /* * If the old index falls out of the bounds of the rooms array * (usually because rooms were removed), we can no longer place * the active room in the same old index. @@ -55,7 +55,7 @@ function getRoomsWithStickyRoom( return updated; } - /** + /* * Making the active room sticky is as simple as removing it from * its new index and placing it in the old index. */