From 47f80126910ae037c25c77839d1c021b9d8e05b0 Mon Sep 17 00:00:00 2001 From: Florian Duros Date: Wed, 13 May 2026 17:50:33 +0200 Subject: [PATCH] Room list: add robustness to custom section loading (#33475) * refactor: move all access to custom sections settings into `section.ts` * fix: add robustness when getting the order list of custom sections * fix: add robustness when getting the custom section data * fix: ignore malformed section but don't erase them * fix: remove useless await operator * test: add more tests --- .../stores/room-list-v3/RoomListStoreV3.ts | 4 +- apps/web/src/stores/room-list-v3/section.ts | 71 +++++++++++--- .../room-list/RoomListItemViewModel.ts | 4 +- .../RoomListSectionHeaderViewModel.ts | 5 +- .../viewmodels/room-list/RoomListViewModel.ts | 11 ++- .../room-list-v3/RoomListStoreV3-test.ts | 6 +- .../stores/room-list-v3/section-test.ts | 96 +++++++++++++++++-- .../room-list/RoomListItemViewModel-test.tsx | 2 + .../RoomListSectionHeaderViewModel-test.ts | 13 +++ 9 files changed, 178 insertions(+), 34 deletions(-) diff --git a/apps/web/src/stores/room-list-v3/RoomListStoreV3.ts b/apps/web/src/stores/room-list-v3/RoomListStoreV3.ts index a4a64344b1..50feaa83df 100644 --- a/apps/web/src/stores/room-list-v3/RoomListStoreV3.ts +++ b/apps/web/src/stores/room-list-v3/RoomListStoreV3.ts @@ -40,7 +40,7 @@ import { DefaultTagID } from "./skip-list/tag"; import { ExcludeTagsFilter } from "./skip-list/filters/ExcludeTagsFilter"; import { TagFilter } from "./skip-list/filters/TagFilter"; import { filterBoolean } from "../../utils/arrays"; -import { CHATS_TAG, createSection, deleteSection, editSection } from "./section"; +import { CHATS_TAG, createSection, deleteSection, editSection, getOrderedCustomSections } from "./section"; /** * These are the filters passed to the room skip list. @@ -522,7 +522,7 @@ export class RoomListStoreV3Class extends AsyncStoreWithClient { * Load the custom sections from the settings store and update the sorted tags. */ private loadCustomSections(): void { - const orderedCustomSections = SettingsStore.getValue("RoomList.OrderedCustomSections"); + const orderedCustomSections = getOrderedCustomSections(); this.sortedTags = [DefaultTagID.Favourite, ...orderedCustomSections, CHATS_TAG, DefaultTagID.LowPriority]; } } diff --git a/apps/web/src/stores/room-list-v3/section.ts b/apps/web/src/stores/room-list-v3/section.ts index 1fceba2c67..16e1b9a374 100644 --- a/apps/web/src/stores/room-list-v3/section.ts +++ b/apps/web/src/stores/room-list-v3/section.ts @@ -14,8 +14,6 @@ import { CreateSectionDialog } from "../../components/views/dialogs/CreateSectio import { RemoveSectionDialog } from "../../components/views/dialogs/RemoveSectionDialog"; import { DefaultTagID, type TagID } from "./skip-list/tag"; -type Tag = string; - /** * A synthetic tag used to represent the "Chats" section, which contains * every room that does not belong to any other explicit tag section. @@ -27,12 +25,14 @@ export const CHATS_TAG = "chats"; */ export const CUSTOM_SECTION_TAG_PREFIX = "element.io.section."; +type CustomTag = `${typeof CUSTOM_SECTION_TAG_PREFIX}${string}`; + /** * Checks if a given tag is a custom section tag. * @param tag - The tag to check. * @returns True if the tag is a custom section tag, false otherwise. */ -export function isCustomSectionTag(tag: string): boolean { +export function isCustomSectionTag(tag: string): tag is CustomTag { return tag.startsWith(CUSTOM_SECTION_TAG_PREFIX); } @@ -58,18 +58,56 @@ export function isSectionTag(tagId: TagID): boolean { * Structure of the custom section stored in the settings. The tag is used as a unique identifier for the section, and the name is given by the user. */ type CustomSection = { - tag: Tag; + tag: CustomTag; name: string; }; +/** + * Type guard to check if a value is a valid CustomSection object. + */ +function isValidCustomSection(value: unknown): value is CustomSection { + return ( + typeof value === "object" && + value !== null && + isCustomSectionTag((value as Record).tag as string) && + typeof (value as Record).name === "string" + ); +} + /** * The custom sections data is stored as a record in the settings, where the key is the section tag and the value is the section data (name and tag). */ -export type CustomSectionsData = Record; +export type CustomSectionsData = Record; /** * Ordered list of custom section tags. */ -export type OrderedCustomSections = Tag[]; +export type OrderedCustomSections = CustomTag[]; + +/** + * Retrieves the custom sections data from the settings. + * Invalid or malformed entries are dropped and the cleaned data is persisted back to settings. + */ +export function getCustomSectionData(): CustomSectionsData { + const raw = SettingsStore.getValue("RoomList.CustomSectionData"); + // Data are malformed + if (typeof raw !== "object" || raw === null || Array.isArray(raw)) return {}; + + // Filter out invalid entries + return Object.fromEntries( + Object.entries(raw).filter(([key, value]) => isValidCustomSection(value) && value.tag === key), + ) as CustomSectionsData; +} + +/** + * Retrieves the ordered list of custom section tags from the settings. + * If the settings contain tags that are not present in the custom section data, they will be filtered out and the settings will be updated to remove the unknown tags. + */ +export function getOrderedCustomSections(): OrderedCustomSections { + const sectionData = getCustomSectionData(); + const rawValue = SettingsStore.getValue("RoomList.OrderedCustomSections"); + const orderedSections: OrderedCustomSections = Array.isArray(rawValue) ? rawValue : []; + return orderedSections.filter((tag) => tag in sectionData); +} /** * Creates a new custom section by showing a dialog to the user to enter the section name. @@ -83,16 +121,16 @@ export async function createSection(): Promise { const [shouldCreateSection, sectionName] = await modal.finished; if (!shouldCreateSection || !sectionName) return undefined; - const tag = `${CUSTOM_SECTION_TAG_PREFIX}${window.crypto.randomUUID()}`; + const tag: CustomTag = `${CUSTOM_SECTION_TAG_PREFIX}${window.crypto.randomUUID()}`; const newSection: CustomSection = { tag, name: sectionName }; // Save the new section data - const sectionData = SettingsStore.getValue("RoomList.CustomSectionData") || {}; + const sectionData = getCustomSectionData(); sectionData[tag] = newSection; await SettingsStore.setValue("RoomList.CustomSectionData", null, SettingLevel.ACCOUNT, sectionData); // Add the new section to the ordered list of sections - const orderedSections = SettingsStore.getValue("RoomList.OrderedCustomSections") || []; + const orderedSections = getOrderedCustomSections(); orderedSections.push(tag); await SettingsStore.setValue("RoomList.OrderedCustomSections", null, SettingLevel.ACCOUNT, orderedSections); return tag; @@ -103,7 +141,11 @@ export async function createSection(): Promise { * @param tag - The tag of the section to edit. */ export async function editSection(tag: string): Promise { - const sectionData = SettingsStore.getValue("RoomList.CustomSectionData") || {}; + if (!isCustomSectionTag(tag)) { + logger.info("Unknown section tag, cannot edit section", tag); + return; + } + const sectionData = getCustomSectionData(); const section = sectionData[tag]; if (!section) { logger.info("Unknown section tag, cannot edit section", tag); @@ -127,7 +169,11 @@ export async function editSection(tag: string): Promise { * @param isEmpty - Whether the section is empty (has no rooms). If the section is not empty, the confirmation dialog will show a warning message. */ export async function deleteSection(tag: string, isEmpty: boolean): Promise { - const sectionData = SettingsStore.getValue("RoomList.CustomSectionData"); + if (!isCustomSectionTag(tag)) { + logger.info("Unknown section tag, cannot delete section", tag); + return; + } + const sectionData = getCustomSectionData(); if (!sectionData[tag]) { logger.info("Unknown section tag, cannot delete section", tag); return; @@ -138,8 +184,7 @@ export async function deleteSection(tag: string, isEmpty: boolean): Promise sectionTag !== tag); + const newOrderedSections = getOrderedCustomSections().filter((sectionTag) => sectionTag !== tag); await SettingsStore.setValue("RoomList.OrderedCustomSections", null, SettingLevel.ACCOUNT, newOrderedSections); // Remove the section data diff --git a/apps/web/src/viewmodels/room-list/RoomListItemViewModel.ts b/apps/web/src/viewmodels/room-list/RoomListItemViewModel.ts index 0959f962bc..c2006c84b7 100644 --- a/apps/web/src/viewmodels/room-list/RoomListItemViewModel.ts +++ b/apps/web/src/viewmodels/room-list/RoomListItemViewModel.ts @@ -39,8 +39,8 @@ import type { ViewRoomPayload } from "../../dispatcher/payloads/ViewRoomPayload" import PosthogTrackers from "../../PosthogTrackers"; import { type Call, CallEvent } from "../../models/Call"; import RoomListStoreV3 from "../../stores/room-list-v3/RoomListStoreV3"; +import { getCustomSectionData, isDefaultSectionTag } from "../../stores/room-list-v3/section"; import { _t } from "../../languageHandler"; -import { isDefaultSectionTag } from "../../stores/room-list-v3/section"; interface RoomItemProps { room: Room; @@ -424,7 +424,7 @@ export class RoomListItemViewModel * Order follows the canonical section order from RoomListStoreV3. */ private static buildSections(roomTags: Room["tags"]): Section[] { - const customSectionData = SettingsStore.getValue("RoomList.CustomSectionData") || {}; + const customSectionData = getCustomSectionData(); return ( RoomListStoreV3.instance.orderedSectionTags diff --git a/apps/web/src/viewmodels/room-list/RoomListSectionHeaderViewModel.ts b/apps/web/src/viewmodels/room-list/RoomListSectionHeaderViewModel.ts index f20438cf9d..a7aebddffb 100644 --- a/apps/web/src/viewmodels/room-list/RoomListSectionHeaderViewModel.ts +++ b/apps/web/src/viewmodels/room-list/RoomListSectionHeaderViewModel.ts @@ -17,7 +17,7 @@ import { NotificationStateEvents } from "../../stores/notifications/Notification import { type RoomNotificationState } from "../../stores/notifications/RoomNotificationState"; import SettingsStore from "../../settings/SettingsStore"; import RoomListStoreV3 from "../../stores/room-list-v3/RoomListStoreV3"; -import { isDefaultSectionTag } from "../../stores/room-list-v3/section"; +import { getCustomSectionData, isCustomSectionTag, isDefaultSectionTag } from "../../stores/room-list-v3/section"; interface RoomListSectionHeaderViewModelProps { tag: string; @@ -139,8 +139,7 @@ export class RoomListSectionHeaderViewModel * Handle changes to custom section data. */ private onCustomSectionDataChange(): void { - const customSectionData = SettingsStore.getValue("RoomList.CustomSectionData") || {}; - const sectionData = customSectionData[this.props.tag]; + const sectionData = isCustomSectionTag(this.props.tag) ? getCustomSectionData()[this.props.tag] : undefined; if (sectionData) { this.snapshot.merge({ title: sectionData.name }); } diff --git a/apps/web/src/viewmodels/room-list/RoomListViewModel.ts b/apps/web/src/viewmodels/room-list/RoomListViewModel.ts index 9ffaa3ebae..55a2708e36 100644 --- a/apps/web/src/viewmodels/room-list/RoomListViewModel.ts +++ b/apps/web/src/viewmodels/room-list/RoomListViewModel.ts @@ -36,10 +36,10 @@ import { hasCreateRoomRights } from "./utils"; import { keepIfSame } from "../../utils/keepIfSame"; import { DefaultTagID } from "../../stores/room-list-v3/skip-list/tag"; import { RoomListSectionHeaderViewModel } from "./RoomListSectionHeaderViewModel"; +import { getCustomSectionData, isCustomSectionTag, CHATS_TAG } from "../../stores/room-list-v3/section"; import SettingsStore from "../../settings/SettingsStore"; import { tagRoom } from "../../utils/room/tagRoom"; import { getSectionTagForRoom } from "../../utils/room/getSectionTagForRoom"; -import { CHATS_TAG } from "../../stores/room-list-v3/section"; /** * Tracks the position of the active room within a specific section. @@ -287,8 +287,7 @@ export class RoomListViewModel public getSectionHeaderViewModel(tag: string): RoomListSectionHeaderViewModel { if (this.roomSectionHeaderViewModels.has(tag)) return this.roomSectionHeaderViewModels.get(tag)!; - const customSections = SettingsStore.getValue("RoomList.CustomSectionData"); - const title = TAG_TO_TITLE_MAP[tag] || customSections[tag]?.name || tag; + const title = TAG_TO_TITLE_MAP[tag] || (isCustomSectionTag(tag) && getCustomSectionData()[tag]?.name) || tag; const viewModel = new RoomListSectionHeaderViewModel({ tag, title, @@ -692,11 +691,13 @@ function computeSections( roomsResult: RoomsResult, isSectionExpanded: (tag: string) => boolean, ): { sections: Section[]; isFlatList: boolean } { - const customSections = SettingsStore.getValue("RoomList.CustomSectionData"); + const customSections = getCustomSectionData(); const sections = roomsResult.sections // Only include sections that have rooms or are custom sections (which may be empty but should still be shown) - .filter((section) => section.rooms.length > 0 || customSections[section.tag]) + .filter( + (section) => section.rooms.length > 0 || (isCustomSectionTag(section.tag) && customSections[section.tag]), + ) // Remove roomIds for sections that are currently collapsed according to their section header view model .map((section) => ({ ...section, diff --git a/apps/web/test/unit-tests/stores/room-list-v3/RoomListStoreV3-test.ts b/apps/web/test/unit-tests/stores/room-list-v3/RoomListStoreV3-test.ts index 1b85a18751..adc53d7477 100644 --- a/apps/web/test/unit-tests/stores/room-list-v3/RoomListStoreV3-test.ts +++ b/apps/web/test/unit-tests/stores/room-list-v3/RoomListStoreV3-test.ts @@ -833,6 +833,7 @@ describe("RoomListStoreV3", () => { jest.spyOn(SettingsStore, "getValue").mockImplementation((setting: string) => { if (setting === "feature_room_list_sections") return true; if (setting === "RoomList.OrderedCustomSections") return []; + if (setting === "RoomList.CustomSectionData") return {}; return false; }); } @@ -1093,6 +1094,7 @@ describe("RoomListStoreV3", () => { jest.spyOn(SettingsStore, "getValue").mockImplementation((setting: string) => { if (setting === "feature_room_list_sections") return true; if (setting === "RoomList.OrderedCustomSections") return []; + if (setting === "RoomList.CustomSectionData") return {}; return false; }); @@ -1107,11 +1109,13 @@ describe("RoomListStoreV3", () => { jest.spyOn(SettingsStore, "getValue").mockImplementation((setting: string) => { if (setting === "feature_room_list_sections") return true; if (setting === "RoomList.OrderedCustomSections") return [customTag]; + if (setting === "RoomList.CustomSectionData") + return { [customTag]: { tag: customTag, name: "Custom" } }; return false; }); // Trigger the settings watcher - settingsWatcher("RoomList.OrderedCustomSections"); + await Promise.resolve(settingsWatcher("RoomList.OrderedCustomSections")); // Now there should be 4 sections (Favourite, custom, Chats, LowPriority) expect(store.getSortedRoomsInActiveSpace().sections).toHaveLength(4); diff --git a/apps/web/test/unit-tests/stores/room-list-v3/section-test.ts b/apps/web/test/unit-tests/stores/room-list-v3/section-test.ts index 2aa17e71bc..8ba9da11c0 100644 --- a/apps/web/test/unit-tests/stores/room-list-v3/section-test.ts +++ b/apps/web/test/unit-tests/stores/room-list-v3/section-test.ts @@ -8,12 +8,14 @@ import Modal from "../../../../src/Modal"; import SettingsStore from "../../../../src/settings/SettingsStore"; import { - CHATS_TAG, - CUSTOM_SECTION_TAG_PREFIX, createSection, editSection, deleteSection, + getCustomSectionData, + getOrderedCustomSections, isDefaultSectionTag, + CHATS_TAG, + CUSTOM_SECTION_TAG_PREFIX, isSectionTag, } from "../../../../src/stores/room-list-v3/section"; import { CreateSectionDialog } from "../../../../src/components/views/dialogs/CreateSectionDialog"; @@ -25,6 +27,68 @@ describe("section", () => { jest.restoreAllMocks(); }); + describe("getCustomSectionData", () => { + const validTag = "element.io.section.valid"; + const invalidTag = "element.io.section.invalid"; + const validEntry = { tag: validTag, name: "Valid" }; + + beforeEach(() => { + jest.spyOn(SettingsStore, "setValue").mockResolvedValue(undefined); + }); + + it.each([null, false, 42, "string", []] as const)("returns an empty object when the raw value is %p", (raw) => { + jest.spyOn(SettingsStore, "getValue").mockReturnValue(raw as any); + expect(getCustomSectionData()).toEqual({}); + }); + + it("returns valid entries and drops invalid ones", () => { + jest.spyOn(SettingsStore, "getValue").mockReturnValue({ + [validTag]: validEntry, + [invalidTag]: { tag: "element.io.section.mismatch", name: "Bad" }, + }); + expect(getCustomSectionData()).toEqual({ [validTag]: validEntry }); + }); + + it("drops entries that fail the isValidCustomSection check", () => { + jest.spyOn(SettingsStore, "getValue").mockReturnValue({ + "element.io.section.null-val": null, + "element.io.section.str-val": "not-an-object", + "element.io.section.bad-tag": { tag: "not-a-custom-tag", name: "Bad" }, + "element.io.section.bad-name": { tag: "element.io.section.bad-name", name: 42 }, + }); + expect(getCustomSectionData()).toEqual({}); + }); + }); + + describe("getOrderedCustomSections", () => { + const tag = "element.io.section.abc"; + + beforeEach(() => { + jest.spyOn(SettingsStore, "setValue").mockResolvedValue(undefined); + }); + + it("returns an empty array when the raw value is not an array", () => { + jest.spyOn(SettingsStore, "getValue").mockImplementation((setting) => { + if (setting === "RoomList.OrderedCustomSections") return "not-an-array"; + return null; + }); + + const result = getOrderedCustomSections(); + expect(result).toEqual([]); + }); + + it("removes unknown sections and saves the cleaned list", () => { + const knownTag = "element.io.section.known"; + jest.spyOn(SettingsStore, "getValue").mockImplementation((setting) => { + if (setting === "RoomList.CustomSectionData") return { [knownTag]: { tag: knownTag, name: "Known" } }; + if (setting === "RoomList.OrderedCustomSections") return [knownTag, tag]; + return null; + }); + + expect(getOrderedCustomSections()).toEqual([knownTag]); + }); + }); + describe("createSection", () => { beforeEach(() => { jest.spyOn(SettingsStore, "getValue").mockReturnValue(null); @@ -69,6 +133,8 @@ describe("section", () => { const existingTag = "element.io.section.existing"; jest.spyOn(SettingsStore, "getValue").mockImplementation((setting) => { if (setting === "RoomList.OrderedCustomSections") return [existingTag]; + if (setting === "RoomList.CustomSectionData") + return { [existingTag]: { tag: existingTag, name: "Existing" } }; return null; }); jest.spyOn(Modal, "createDialog").mockReturnValue({ @@ -79,15 +145,16 @@ describe("section", () => { await createSection(); - const customDataCall = setValueSpy.mock.calls.find(([name]) => name === "RoomList.CustomSectionData"); - const savedSection = Object.values(customDataCall![3] as Record)[0]; - expect(savedSection.name).toBe("My Section"); - expect(savedSection.tag).toMatch(/^element\.io\.section\./); - const orderedCall = setValueSpy.mock.calls.find(([name]) => name === "RoomList.OrderedCustomSections"); const savedOrder = orderedCall![3] as string[]; expect(savedOrder[0]).toBe(existingTag); expect(savedOrder[1]).toMatch(/^element\.io\.section\./); + + const newTag = savedOrder[1]; + const customDataCall = setValueSpy.mock.calls.find(([name]) => name === "RoomList.CustomSectionData"); + const savedSection = (customDataCall![3] as Record)[newTag]; + expect(savedSection.name).toBe("My Section"); + expect(savedSection.tag).toBe(newTag); }); }); @@ -100,6 +167,12 @@ describe("section", () => { jest.spyOn(SettingsStore, "setValue").mockResolvedValue(undefined); }); + it("does nothing if the tag is not a custom section tag", async () => { + const createDialogSpy = jest.spyOn(Modal, "createDialog"); + await editSection("m.favourite"); + expect(createDialogSpy).not.toHaveBeenCalled(); + }); + it("does nothing if the section does not exist", async () => { jest.spyOn(SettingsStore, "getValue").mockReturnValue({}); const createDialogSpy = jest.spyOn(Modal, "createDialog"); @@ -157,13 +230,20 @@ describe("section", () => { beforeEach(() => { jest.spyOn(SettingsStore, "getValue").mockImplementation((setting) => { - if (setting === "RoomList.CustomSectionData") return { [tag]: { tag, name: "My Section" } }; + if (setting === "RoomList.CustomSectionData") + return { [tag]: { tag, name: "My Section" }, [otherTag]: { tag: otherTag, name: "Other Section" } }; if (setting === "RoomList.OrderedCustomSections") return [otherTag, tag]; return null; }); jest.spyOn(SettingsStore, "setValue").mockResolvedValue(undefined); }); + it("does nothing if the tag is not a custom section tag", async () => { + const createDialogSpy = jest.spyOn(Modal, "createDialog"); + await deleteSection("m.favourite", false); + expect(createDialogSpy).not.toHaveBeenCalled(); + }); + it("does nothing if the section does not exist", async () => { jest.spyOn(SettingsStore, "getValue").mockReturnValue({}); const createDialogSpy = jest.spyOn(Modal, "createDialog"); diff --git a/apps/web/test/viewmodels/room-list/RoomListItemViewModel-test.tsx b/apps/web/test/viewmodels/room-list/RoomListItemViewModel-test.tsx index 3834803d07..6dd9de7590 100644 --- a/apps/web/test/viewmodels/room-list/RoomListItemViewModel-test.tsx +++ b/apps/web/test/viewmodels/room-list/RoomListItemViewModel-test.tsx @@ -80,8 +80,10 @@ describe("RoomListItemViewModel", () => { jest.spyOn(SettingsStore, "getValue").mockImplementation((setting) => { if (setting === "RoomList.showMessagePreview") return false; if (setting === "RoomList.OrderedCustomSections") return []; + if (setting === "RoomList.CustomSectionData") return {}; return false; }); + jest.spyOn(SettingsStore, "setValue").mockResolvedValue(undefined); jest.spyOn(SettingsStore, "watchSetting").mockImplementation(() => "watcher-id"); jest.spyOn(MessagePreviewStore.instance, "getPreviewForRoom").mockResolvedValue(null); diff --git a/apps/web/test/viewmodels/room-list/RoomListSectionHeaderViewModel-test.ts b/apps/web/test/viewmodels/room-list/RoomListSectionHeaderViewModel-test.ts index bd39d6a17b..a68cc188f9 100644 --- a/apps/web/test/viewmodels/room-list/RoomListSectionHeaderViewModel-test.ts +++ b/apps/web/test/viewmodels/room-list/RoomListSectionHeaderViewModel-test.ts @@ -154,6 +154,19 @@ describe("RoomListSectionHeaderViewModel", () => { expect(vm.getSnapshot().title).toBe("My Section"); }); + + it("should not update title when tag is not a custom section tag", () => { + const vm = new RoomListSectionHeaderViewModel({ + tag: "m.favourite", + title: "Favourites", + spaceId: "!space:server", + onToggleExpanded, + }); + + watchCallback(); + + expect(vm.getSnapshot().title).toBe("Favourites"); + }); }); describe("editSection", () => {