diff --git a/apps/web/src/actions/RoomListActions.ts b/apps/web/src/actions/RoomListActions.ts index 14343b5bf8..b91be93042 100644 --- a/apps/web/src/actions/RoomListActions.ts +++ b/apps/web/src/actions/RoomListActions.ts @@ -15,8 +15,6 @@ import Modal from "../Modal"; import * as Rooms from "../Rooms"; import { _t } from "../languageHandler"; import { type AsyncActionPayload } from "../dispatcher/payloads"; -import RoomListStore from "../stores/room-list/RoomListStore"; -import { SortAlgorithm } from "../stores/room-list/algorithms/models"; import { DefaultTagID, type TagID } from "../stores/room-list-v3/skip-list/tag"; import ErrorDialog from "../components/views/dialogs/ErrorDialog"; @@ -25,45 +23,22 @@ export default class RoomListActions { * Creates an action thunk that will do an asynchronous request to * tag room. * - * @param {MatrixClient} matrixClient the matrix client to set the + * @param matrixClient the matrix client to set the * account data on. - * @param {Room} room the room to tag. - * @param {string} oldTag the tag to remove (unless oldTag ==== newTag) - * @param {string} newTag the tag with which to tag the room. - * @param {?number} oldIndex the previous position of the room in the + * @param room the room to tag. + * @param oldTag the tag to remove (unless oldTag ==== newTag) + * @param newTag the tag with which to tag the room. + * @param oldIndex the previous position of the room in the * list of rooms. - * @param {?number} newIndex the new position of the room in the list - * of rooms. - * @returns {AsyncActionPayload} an async action payload + * @returns an async action payload * @see asyncAction */ public static tagRoom( matrixClient: MatrixClient, room: Room, - oldTag: TagID | null, - newTag: TagID | null, - newIndex: number, + oldTag: TagID | null | undefined, + newTag: TagID | null | undefined, ): AsyncActionPayload { - let metaData: Parameters[2] | undefined; - - // Is the tag ordered manually? - const store = RoomListStore.instance; - if (newTag && store.getTagSorting(newTag) === SortAlgorithm.Manual) { - const newList = [...store.orderedLists[newTag]]; - - newList.sort((a, b) => a.tags[newTag].order - b.tags[newTag].order); - - const indexBefore = newIndex - 1; - const indexAfter = newIndex; - - const prevOrder = indexBefore <= 0 ? 0 : newList[indexBefore].tags[newTag].order; - const nextOrder = indexAfter >= newList.length ? 1 : newList[indexAfter].tags[newTag].order; - - metaData = { - order: (prevOrder + nextOrder) / 2.0, - }; - } - return asyncAction( "RoomListActions.tagRoom", () => { @@ -103,8 +78,8 @@ export default class RoomListActions { } // if we moved lists or the ordering changed, add the new tag - if (newTag && newTag !== DefaultTagID.DM && (hasChangedSubLists || metaData)) { - const promiseToAdd = matrixClient.setRoomTag(roomId, newTag, metaData).catch(function (err) { + if (newTag && newTag !== DefaultTagID.DM && hasChangedSubLists) { + const promiseToAdd = matrixClient.setRoomTag(roomId, newTag).catch(function (err) { logger.error("Failed to add tag " + newTag + " to room: " + err); Modal.createDialog(ErrorDialog, { title: _t("room_list|failed_add_tag", { tagName: newTag }), @@ -125,7 +100,6 @@ export default class RoomListActions { room, oldTag, newTag, - metaData, }; }, ); diff --git a/apps/web/src/components/views/context_menus/RoomGeneralContextMenu.tsx b/apps/web/src/components/views/context_menus/RoomGeneralContextMenu.tsx index dd3143bd49..d60221d8e8 100644 --- a/apps/web/src/components/views/context_menus/RoomGeneralContextMenu.tsx +++ b/apps/web/src/components/views/context_menus/RoomGeneralContextMenu.tsx @@ -150,7 +150,7 @@ export const RoomGeneralContextMenu: React.FC = ({ const isApplied = getTagsForRoom(room).includes(tagId); const removeTag = isApplied ? tagId : inverseTag; const addTag = isApplied ? null : tagId; - dis.dispatch(RoomListActions.tagRoom(cli, room, removeTag, addTag, 0)); + dis.dispatch(RoomListActions.tagRoom(cli, room, removeTag, addTag)); } else { logger.warn(`Unexpected tag ${tagId} applied to ${room.roomId}`); } diff --git a/apps/web/src/stores/room-list/algorithms/list-ordering/ImportanceAlgorithm.ts b/apps/web/src/stores/room-list/algorithms/list-ordering/ImportanceAlgorithm.ts index 77b3662d55..1e37c61c23 100644 --- a/apps/web/src/stores/room-list/algorithms/list-ordering/ImportanceAlgorithm.ts +++ b/apps/web/src/stores/room-list/algorithms/list-ordering/ImportanceAlgorithm.ts @@ -12,7 +12,7 @@ import { logger } from "matrix-js-sdk/src/logger"; import { type TagID } from "../../../room-list-v3/skip-list/tag"; import { RoomUpdateCause } from "../../models"; -import { SortAlgorithm } from "../models"; +import { type SortAlgorithm } from "../models"; import { sortRoomsWithAlgorithm } from "../tag-sorting"; import { OrderingAlgorithm } from "./OrderingAlgorithm"; import { NotificationLevel } from "../../../notifications/NotificationLevel"; @@ -93,32 +93,23 @@ export class ImportanceAlgorithm extends OrderingAlgorithm { } public setRooms(rooms: Room[]): void { - if (this.sortingAlgorithm === SortAlgorithm.Manual) { - this.cachedOrderedRooms = sortRoomsWithAlgorithm(rooms, this.tagId, this.sortingAlgorithm); - } else { - // Every other sorting type affects the categories, not the whole tag. - const categorized = this.categorizeRooms(rooms); - for (const category of Object.keys(categorized)) { - const notificationColor = category as unknown as NotificationLevel; - const roomsToOrder = categorized[notificationColor]; - categorized[notificationColor] = sortRoomsWithAlgorithm( - roomsToOrder, - this.tagId, - this.sortingAlgorithm, - ); - } - - const newlyOrganized: Room[] = []; - const newIndices: CategoryIndex = {}; - - for (const category of CATEGORY_ORDER) { - newIndices[category] = newlyOrganized.length; - newlyOrganized.push(...categorized[category]); - } - - this.indices = newIndices; - this.cachedOrderedRooms = newlyOrganized; + const categorized = this.categorizeRooms(rooms); + for (const category of Object.keys(categorized)) { + const notificationColor = category as unknown as NotificationLevel; + const roomsToOrder = categorized[notificationColor]; + categorized[notificationColor] = sortRoomsWithAlgorithm(roomsToOrder, this.tagId, this.sortingAlgorithm); } + + const newlyOrganized: Room[] = []; + const newIndices: CategoryIndex = {}; + + for (const category of CATEGORY_ORDER) { + newIndices[category] = newlyOrganized.length; + newlyOrganized.push(...categorized[category]); + } + + this.indices = newIndices; + this.cachedOrderedRooms = newlyOrganized; } private getCategoryIndex(category: NotificationLevel): number { @@ -172,10 +163,6 @@ export class ImportanceAlgorithm extends OrderingAlgorithm { return false; } - if (this.sortingAlgorithm === SortAlgorithm.Manual) { - return false; // Nothing to do here. - } - const category = this.getRoomCategory(room); const roomIdx = this.getRoomIndex(room); diff --git a/apps/web/src/stores/room-list/algorithms/models.ts b/apps/web/src/stores/room-list/algorithms/models.ts index 7000be601b..99d5874e2d 100644 --- a/apps/web/src/stores/room-list/algorithms/models.ts +++ b/apps/web/src/stores/room-list/algorithms/models.ts @@ -12,7 +12,6 @@ import { type TagID } from "../../room-list-v3/skip-list/tag"; import { type OrderingAlgorithm } from "./list-ordering/OrderingAlgorithm"; export enum SortAlgorithm { - Manual = "MANUAL", Alphabetic = "ALPHABETIC", Recent = "RECENT", } diff --git a/apps/web/src/stores/room-list/algorithms/tag-sorting/ManualAlgorithm.ts b/apps/web/src/stores/room-list/algorithms/tag-sorting/ManualAlgorithm.ts deleted file mode 100644 index 940fa7fd4e..0000000000 --- a/apps/web/src/stores/room-list/algorithms/tag-sorting/ManualAlgorithm.ts +++ /dev/null @@ -1,24 +0,0 @@ -/* -Copyright 2024 New Vector Ltd. -Copyright 2020 The Matrix.org Foundation C.I.C. - -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 { type Room } from "matrix-js-sdk/src/matrix"; - -import { type TagID } from "../../../room-list-v3/skip-list/tag"; -import { type IAlgorithm } from "./IAlgorithm"; - -/** - * Sorts rooms according to the tag's `order` property on the room. - */ -export class ManualAlgorithm implements IAlgorithm { - public sortRooms(rooms: Room[], tagId: TagID): Room[] { - const getOrderProp = (r: Room): number => r.tags[tagId].order || 0; - return rooms.sort((a, b) => { - return getOrderProp(a) - getOrderProp(b); - }); - } -} diff --git a/apps/web/src/stores/room-list/algorithms/tag-sorting/index.ts b/apps/web/src/stores/room-list/algorithms/tag-sorting/index.ts index edf308fb39..236fde6b8d 100644 --- a/apps/web/src/stores/room-list/algorithms/tag-sorting/index.ts +++ b/apps/web/src/stores/room-list/algorithms/tag-sorting/index.ts @@ -9,7 +9,6 @@ Please see LICENSE files in the repository root for full details. import { type Room } from "matrix-js-sdk/src/matrix"; import { SortAlgorithm } from "../models"; -import { ManualAlgorithm } from "./ManualAlgorithm"; import { type IAlgorithm } from "./IAlgorithm"; import { type TagID } from "../../../room-list-v3/skip-list/tag"; import { RecentAlgorithm } from "./RecentAlgorithm"; @@ -18,7 +17,6 @@ import { AlphabeticAlgorithm } from "./AlphabeticAlgorithm"; const ALGORITHM_INSTANCES: { [algorithm in SortAlgorithm]: IAlgorithm } = { [SortAlgorithm.Recent]: new RecentAlgorithm(), [SortAlgorithm.Alphabetic]: new AlphabeticAlgorithm(), - [SortAlgorithm.Manual]: new ManualAlgorithm(), }; /** diff --git a/apps/web/src/utils/room/tagRoom.ts b/apps/web/src/utils/room/tagRoom.ts index 6e3d6e4a52..ae9b52a174 100644 --- a/apps/web/src/utils/room/tagRoom.ts +++ b/apps/web/src/utils/room/tagRoom.ts @@ -25,7 +25,7 @@ export function tagRoom(room: Room, tagId: TagID): void { const isApplied = getTagsForRoom(room).includes(tagId); const removeTag = isApplied ? tagId : inverseTag; const addTag = isApplied ? null : tagId; - dis.dispatch(RoomListActions.tagRoom(room.client, room, removeTag, addTag, 0)); + dis.dispatch(RoomListActions.tagRoom(room.client, room, removeTag, addTag)); } else { logger.warn(`Unexpected tag ${tagId} applied to ${room.roomId}`); } diff --git a/apps/web/test/test-utils/test-utils.ts b/apps/web/test/test-utils/test-utils.ts index 09d37e50c3..54969a4774 100644 --- a/apps/web/test/test-utils/test-utils.ts +++ b/apps/web/test/test-utils/test-utils.ts @@ -356,6 +356,8 @@ export function createTestClient(): MatrixClient { kick: jest.fn(), ban: jest.fn(), sendTextMessage: jest.fn(), + deleteRoomTag: jest.fn().mockResolvedValue({}), + setRoomTag: jest.fn().mockResolvedValue({}), } as unknown as MatrixClient; client.reEmitter = new ReEmitter(client); diff --git a/apps/web/test/unit-tests/actions/RoomListActions-test.ts b/apps/web/test/unit-tests/actions/RoomListActions-test.ts new file mode 100644 index 0000000000..ecdf6f676d --- /dev/null +++ b/apps/web/test/unit-tests/actions/RoomListActions-test.ts @@ -0,0 +1,174 @@ +/* + * Copyright 2026 Element Creations 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 { type MatrixClient, type Room } from "matrix-js-sdk/src/matrix"; +import { mocked } from "jest-mock"; + +import RoomListActions from "../../../src/actions/RoomListActions"; +import { DefaultTagID } from "../../../src/stores/room-list-v3/skip-list/tag"; +import Modal from "../../../src/Modal"; +import * as Rooms from "../../../src/Rooms"; +import { createTestClient, flushPromises, mkRoom } from "../../test-utils"; + +jest.mock("../../../src/Modal"); +jest.mock("../../../src/Rooms"); + +describe("RoomListActions", () => { + const ROOM_ID = "!room:example.org"; + + let client: MatrixClient; + let room: Room; + const dispatch = jest.fn(); + + beforeEach(() => { + client = createTestClient(); + room = mkRoom(client, ROOM_ID); + mocked(Rooms.guessAndSetDMRoom).mockResolvedValue(undefined); + }); + + afterEach(() => { + jest.clearAllMocks(); + }); + + describe("tagRoom", () => { + /** + * Invoke the async payload returned by tagRoom and wait for all promises to settle. + */ + async function invokeTagRoom( + oldTag: Parameters[2], + newTag: Parameters[3], + ): Promise { + const payload = RoomListActions.tagRoom(client, room, oldTag, newTag); + + // Execute the async function embedded in the payload. + payload.fn(dispatch); + + // Flush all microtasks / pending promises. + await flushPromises(); + } + + it("dispatches a pending action immediately with the optimistic update data", () => { + const payload = RoomListActions.tagRoom(client, room, DefaultTagID.Favourite, DefaultTagID.LowPriority); + + payload.fn(dispatch); + + expect(dispatch).toHaveBeenCalledWith( + expect.objectContaining({ + action: "RoomListActions.tagRoom.pending", + request: { room, oldTag: DefaultTagID.Favourite, newTag: DefaultTagID.LowPriority }, + }), + ); + }); + + describe("DM tag handling", () => { + it.each([ + [undefined, DefaultTagID.DM], + [DefaultTagID.DM, undefined], + ])( + "treats oldTag=%s and newTag=%s as a DM tag change and does not call setRoomTag or deleteRoomTag", + async (oldTag, newTag) => { + await invokeTagRoom(oldTag, newTag as unknown as null); + + expect(Rooms.guessAndSetDMRoom).toHaveBeenCalledWith(room, newTag === DefaultTagID.DM); + expect(client.deleteRoomTag).not.toHaveBeenCalled(); + expect(client.setRoomTag).not.toHaveBeenCalled(); + }, + ); + + it("opens an ErrorDialog and swallows the error if guessAndSetDMRoom rejects", async () => { + const error = new Error("DM tag error"); + mocked(Rooms.guessAndSetDMRoom).mockRejectedValue(error); + + await invokeTagRoom(undefined, DefaultTagID.DM); + + expect(Modal.createDialog).toHaveBeenCalledWith( + expect.anything(), + expect.objectContaining({ description: error.message }), + ); + // Error is swallowed — success is still dispatched. + expect(dispatch).toHaveBeenCalledWith( + expect.objectContaining({ action: "RoomListActions.tagRoom.success" }), + ); + }); + }); + + describe("regular tag changes (non-DM)", () => { + it("deletes the old tag and adds the new tag when moving between two non-DM tags", async () => { + await invokeTagRoom(DefaultTagID.Favourite, DefaultTagID.LowPriority); + + expect(client.deleteRoomTag).toHaveBeenCalledWith(ROOM_ID, DefaultTagID.Favourite); + expect(client.setRoomTag).toHaveBeenCalledWith(ROOM_ID, DefaultTagID.LowPriority); + expect(dispatch).toHaveBeenCalledWith( + expect.objectContaining({ action: "RoomListActions.tagRoom.success" }), + ); + }); + + it("only calls setRoomTag when there was no previous tag", async () => { + await invokeTagRoom(null, DefaultTagID.Favourite); + + expect(client.deleteRoomTag).not.toHaveBeenCalled(); + expect(client.setRoomTag).toHaveBeenCalledWith(ROOM_ID, DefaultTagID.Favourite); + }); + + it.each([null, DefaultTagID.DM])( + "only calls deleteRoomTag when moving from %s to another non-DM tag", + async (newTag) => { + await invokeTagRoom(DefaultTagID.Favourite, newTag); + + expect(client.deleteRoomTag).toHaveBeenCalledWith(ROOM_ID, DefaultTagID.Favourite); + expect(client.setRoomTag).not.toHaveBeenCalled(); + }, + ); + + it("makes no API calls when oldTag equals newTag", async () => { + await invokeTagRoom(DefaultTagID.Favourite, DefaultTagID.Favourite); + + expect(client.deleteRoomTag).not.toHaveBeenCalled(); + expect(client.setRoomTag).not.toHaveBeenCalled(); + }); + + it("skips deleteRoomTag for the DM tag but still sets the new tag", async () => { + await invokeTagRoom(DefaultTagID.DM, DefaultTagID.Favourite); + + expect(client.deleteRoomTag).not.toHaveBeenCalled(); + expect(client.setRoomTag).toHaveBeenCalledWith(ROOM_ID, DefaultTagID.Favourite); + }); + + it("shows an ErrorDialog but still dispatches success when deleteRoomTag fails", async () => { + const error = new Error("delete failed"); + jest.spyOn(client, "deleteRoomTag").mockRejectedValue(error); + + await invokeTagRoom(DefaultTagID.Favourite, DefaultTagID.LowPriority); + + expect(Modal.createDialog).toHaveBeenCalledWith( + expect.anything(), + expect.objectContaining({ description: error.message }), + ); + // deleteRoomTag swallows the error, so Promise.all still resolves. + expect(dispatch).toHaveBeenCalledWith( + expect.objectContaining({ action: "RoomListActions.tagRoom.success" }), + ); + }); + + it("shows an ErrorDialog and dispatches failure when setRoomTag fails", async () => { + const error = new Error("set failed"); + jest.spyOn(client, "setRoomTag").mockRejectedValue(error); + + await invokeTagRoom(DefaultTagID.Favourite, DefaultTagID.LowPriority); + + expect(Modal.createDialog).toHaveBeenCalledWith( + expect.anything(), + expect.objectContaining({ description: error.message }), + ); + // setRoomTag rethrows, so Promise.all rejects → failure dispatched. + expect(dispatch).toHaveBeenCalledWith( + expect.objectContaining({ action: "RoomListActions.tagRoom.failure" }), + ); + }); + }); + }); +}); diff --git a/apps/web/test/unit-tests/stores/room-list/algorithms/list-ordering/ImportanceAlgorithm-test.ts b/apps/web/test/unit-tests/stores/room-list/algorithms/list-ordering/ImportanceAlgorithm-test.ts index 6e8f6f605e..5e63dbb831 100644 --- a/apps/web/test/unit-tests/stores/room-list/algorithms/list-ordering/ImportanceAlgorithm-test.ts +++ b/apps/web/test/unit-tests/stores/room-list/algorithms/list-ordering/ImportanceAlgorithm-test.ts @@ -88,72 +88,6 @@ describe("ImportanceAlgorithm", () => { return algorithm; }; - describe("When sortAlgorithm is manual", () => { - const sortAlgorithm = SortAlgorithm.Manual; - it("orders rooms by tag order without categorizing", () => { - jest.spyOn(RoomNotificationStateStore.instance, "getRoomState"); - const algorithm = setupAlgorithm(sortAlgorithm); - - // didn't check notif state - expect(RoomNotificationStateStore.instance.getRoomState).not.toHaveBeenCalled(); - // sorted according to room tag order - expect(algorithm.orderedRooms).toEqual([roomC, roomA, roomB]); - }); - - describe("handleRoomUpdate", () => { - // XXX: This doesn't work because manual ordered rooms dont get categoryindices - // possibly related https://github.com/vector-im/element-web/issues/25099 - it.skip("removes a room", () => { - const algorithm = setupAlgorithm(sortAlgorithm); - - const shouldTriggerUpdate = algorithm.handleRoomUpdate(roomA, RoomUpdateCause.RoomRemoved); - - expect(shouldTriggerUpdate).toBe(true); - expect(algorithm.orderedRooms).toEqual([roomC, roomB]); - }); - - // XXX: This doesn't work because manual ordered rooms dont get categoryindices - it.skip("adds a new room", () => { - const algorithm = setupAlgorithm(sortAlgorithm); - - const shouldTriggerUpdate = algorithm.handleRoomUpdate(roomD, RoomUpdateCause.NewRoom); - - expect(shouldTriggerUpdate).toBe(true); - expect(algorithm.orderedRooms).toEqual([roomC, roomB, roomD, roomE]); - }); - - it("does nothing and returns false for a timeline update", () => { - const algorithm = setupAlgorithm(sortAlgorithm); - - const beforeRooms = algorithm.orderedRooms; - const shouldTriggerUpdate = algorithm.handleRoomUpdate(roomA, RoomUpdateCause.Timeline); - - expect(shouldTriggerUpdate).toBe(false); - // strict equal - expect(algorithm.orderedRooms).toBe(beforeRooms); - }); - - it("does nothing and returns false for a read receipt update", () => { - const algorithm = setupAlgorithm(sortAlgorithm); - - const beforeRooms = algorithm.orderedRooms; - const shouldTriggerUpdate = algorithm.handleRoomUpdate(roomA, RoomUpdateCause.ReadReceipt); - - expect(shouldTriggerUpdate).toBe(false); - // strict equal - expect(algorithm.orderedRooms).toBe(beforeRooms); - }); - - it("throws for an unhandle update cause", () => { - const algorithm = setupAlgorithm(sortAlgorithm); - - expect(() => - algorithm.handleRoomUpdate(roomA, "something unexpected" as unknown as RoomUpdateCause), - ).toThrow("Unsupported update cause: something unexpected"); - }); - }); - }); - describe("When sortAlgorithm is alphabetical", () => { const sortAlgorithm = SortAlgorithm.Alphabetic; diff --git a/apps/web/test/unit-tests/utils/room/tagRoom-test.ts b/apps/web/test/unit-tests/utils/room/tagRoom-test.ts index 4a017a9617..20e5931a87 100644 --- a/apps/web/test/unit-tests/utils/room/tagRoom-test.ts +++ b/apps/web/test/unit-tests/utils/room/tagRoom-test.ts @@ -61,7 +61,6 @@ describe("tagRoom()", () => { room, DefaultTagID.LowPriority, // remove DefaultTagID.Favourite, // add - 0, ); }); @@ -76,7 +75,6 @@ describe("tagRoom()", () => { room, DefaultTagID.Favourite, // remove DefaultTagID.LowPriority, // add - 0, ); }); }); @@ -93,7 +91,6 @@ describe("tagRoom()", () => { room, DefaultTagID.Favourite, // remove null, // add - 0, ); }); @@ -108,7 +105,6 @@ describe("tagRoom()", () => { room, DefaultTagID.Favourite, // remove DefaultTagID.LowPriority, // add - 0, ); }); }); @@ -124,7 +120,6 @@ describe("tagRoom()", () => { room, DefaultTagID.LowPriority, // remove DefaultTagID.Favourite, // add - 0, ); }); @@ -139,7 +134,6 @@ describe("tagRoom()", () => { room, DefaultTagID.LowPriority, // remove null, // add - 0, ); }); });