From e84859210ee7ab7fe14a608afe5fece7ba868619 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Tue, 18 Jul 2023 09:49:09 +0100 Subject: [PATCH 1/3] Prevent re-filtering user directory results in spotlight As they were already filtered by the server and may be fuzzier than any filtering we can do locally, e.g. matching against email addresses or other fields not available to the client --- .../dialogs/spotlight/SpotlightDialog.tsx | 48 +++++++++++++------ .../views/dialogs/SpotlightDialog-test.tsx | 21 ++++++++ 2 files changed, 54 insertions(+), 15 deletions(-) diff --git a/src/components/views/dialogs/spotlight/SpotlightDialog.tsx b/src/components/views/dialogs/spotlight/SpotlightDialog.tsx index b5baa4e2534..caa7125c8f7 100644 --- a/src/components/views/dialogs/spotlight/SpotlightDialog.tsx +++ b/src/components/views/dialogs/spotlight/SpotlightDialog.tsx @@ -142,6 +142,10 @@ interface IRoomResult extends IBaseResult { interface IMemberResult extends IBaseResult { member: Member | RoomMember; + /** + * If the result is from a filtered server API then we set true here to avoid locally culling it in our own filters + */ + alreadyFiltered: boolean; } interface IResult extends IBaseResult { @@ -201,7 +205,8 @@ const toRoomResult = (room: Room): IRoomResult => { } }; -const toMemberResult = (member: Member | RoomMember): IMemberResult => ({ +const toMemberResult = (member: Member | RoomMember, alreadyFiltered: boolean): IMemberResult => ({ + alreadyFiltered, member, section: Section.Suggestions, filter: [Filter.People], @@ -333,21 +338,37 @@ const SpotlightDialog: React.FC = ({ initialText = "", initialFilter = n const possibleResults = useMemo(() => { const userResults: IMemberResult[] = []; const roomResults = findVisibleRooms(cli, msc3946ProcessDynamicPredecessor).map(toRoomResult); - // If we already have a DM with the user we're looking for, we will - // show that DM instead of the user themselves + + // If we already have a DM with the user we're looking for, we will show that DM instead of the user themselves const alreadyAddedUserIds = roomResults.reduce((userIds, result) => { const userId = DMRoomMap.shared().getUserIdForRoomId(result.room.roomId); if (!userId) return userIds; if (result.room.getJoinedMemberCount() > 2) return userIds; - userIds.add(userId); + userIds.set(userId, result); return userIds; - }, new Set()); - for (const user of [...findVisibleRoomMembers(cli, msc3946ProcessDynamicPredecessor), ...users]) { - // Make sure we don't have any user more than once - if (alreadyAddedUserIds.has(user.userId)) continue; - alreadyAddedUserIds.add(user.userId); - - userResults.push(toMemberResult(user)); + }, new Map()); + + function addUserResults(users: Array, alreadyFiltered: boolean): void { + for (const user of users) { + // Make sure we don't have any user more than once + if (alreadyAddedUserIds.has(user.userId)) { + const result = alreadyAddedUserIds.get(user.userId)!; + if (alreadyFiltered && isMemberResult(result) && !result.alreadyFiltered) { + // But if they were added as not yet filtered then mark them as already filtered to avoid + // culling this result based on local filtering. + result.alreadyFiltered = true; + } + continue; + } + const result = toMemberResult(user, alreadyFiltered); + alreadyAddedUserIds.set(user.userId, result); + userResults.push(result); + } + } + addUserResults(findVisibleRoomMembers(cli, msc3946ProcessDynamicPredecessor), false); + addUserResults(users, true); + if (profile) { + addUserResults([new DirectoryMember(profile)], true); } return [ @@ -369,9 +390,6 @@ const SpotlightDialog: React.FC = ({ initialText = "", initialFilter = n })), ...roomResults, ...userResults, - ...(profile && !alreadyAddedUserIds.has(profile.user_id) ? [new DirectoryMember(profile)] : []).map( - toMemberResult, - ), ...publicRooms.map(toPublicRoomResult), ].filter((result) => filter === null || result.filter.includes(filter)); }, [cli, users, profile, publicRooms, filter, msc3946ProcessDynamicPredecessor]); @@ -399,7 +417,7 @@ const SpotlightDialog: React.FC = ({ initialText = "", initialFilter = n ) return; // bail, does not match query } else if (isMemberResult(entry)) { - if (!entry.query?.some((q) => q.includes(lcQuery))) return; // bail, does not match query + if (!entry.alreadyFiltered && !entry.query?.some((q) => q.includes(lcQuery))) return; // bail, does not match query } else if (isPublicRoomResult(entry)) { if (!entry.query?.some((q) => q.includes(lcQuery))) return; // bail, does not match query } else { diff --git a/test/components/views/dialogs/SpotlightDialog-test.tsx b/test/components/views/dialogs/SpotlightDialog-test.tsx index 28572253092..d842ecb727a 100644 --- a/test/components/views/dialogs/SpotlightDialog-test.tsx +++ b/test/components/views/dialogs/SpotlightDialog-test.tsx @@ -338,6 +338,27 @@ describe("Spotlight Dialog", () => { }); }); + it("should not filter out users sent by the server", async () => { + mocked(mockedClient.searchUserDirectory).mockResolvedValue({ + results: [ + { user_id: "@user1:server", display_name: "User Alpha", avatar_url: "mxc://1/avatar" }, + { user_id: "@user2:server", display_name: "User Beta", avatar_url: "mxc://2/avatar" }, + ], + limited: false, + }); + + render( null} />); + // search is debounced + jest.advanceTimersByTime(200); + await flushPromisesWithFakeTimers(); + + const content = document.querySelector("#mx_SpotlightDialog_content")!; + const options = content.querySelectorAll("li.mx_SpotlightDialog_option"); + expect(options.length).toBeGreaterThanOrEqual(2); + expect(options[0]).toHaveTextContent("User Alpha"); + expect(options[1]).toHaveTextContent("User Beta"); + }); + it("should start a DM when clicking a person", async () => { render( Date: Tue, 18 Jul 2023 11:59:35 +0100 Subject: [PATCH 2/3] deduplicate work --- .../views/dialogs/spotlight/SpotlightDialog.tsx | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/src/components/views/dialogs/spotlight/SpotlightDialog.tsx b/src/components/views/dialogs/spotlight/SpotlightDialog.tsx index caa7125c8f7..6cd53873915 100644 --- a/src/components/views/dialogs/spotlight/SpotlightDialog.tsx +++ b/src/components/views/dialogs/spotlight/SpotlightDialog.tsx @@ -245,13 +245,9 @@ const findVisibleRooms = (cli: MatrixClient, msc3946ProcessDynamicPredecessor: b }); }; -const findVisibleRoomMembers = ( - cli: MatrixClient, - msc3946ProcessDynamicPredecessor: boolean, - filterDMs = true, -): RoomMember[] => { +const findVisibleRoomMembers = (visibleRooms: Room[], cli: MatrixClient, filterDMs = true): RoomMember[] => { return Object.values( - findVisibleRooms(cli, msc3946ProcessDynamicPredecessor) + visibleRooms .filter((room) => !filterDMs || !DMRoomMap.shared().getUserIdForRoomId(room.roomId)) .reduce((members, room) => { for (const member of room.getJoinedMembers()) { @@ -336,8 +332,9 @@ const SpotlightDialog: React.FC = ({ initialText = "", initialFilter = n useDebouncedCallback(filter === Filter.People, searchProfileInfo, searchParams); const possibleResults = useMemo(() => { + const visibleRooms = findVisibleRooms(cli, msc3946ProcessDynamicPredecessor); + const roomResults = visibleRooms.map(toRoomResult); const userResults: IMemberResult[] = []; - const roomResults = findVisibleRooms(cli, msc3946ProcessDynamicPredecessor).map(toRoomResult); // If we already have a DM with the user we're looking for, we will show that DM instead of the user themselves const alreadyAddedUserIds = roomResults.reduce((userIds, result) => { @@ -365,7 +362,7 @@ const SpotlightDialog: React.FC = ({ initialText = "", initialFilter = n userResults.push(result); } } - addUserResults(findVisibleRoomMembers(cli, msc3946ProcessDynamicPredecessor), false); + addUserResults(findVisibleRoomMembers(visibleRooms, cli), false); addUserResults(users, true); if (profile) { addUserResults([new DirectoryMember(profile)], true); From 1bec6adb9bdd1d0030e46007fa215cfc29a4e55d Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Tue, 18 Jul 2023 12:06:41 +0100 Subject: [PATCH 3/3] Improve coverage --- .../views/dialogs/SpotlightDialog-test.tsx | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/test/components/views/dialogs/SpotlightDialog-test.tsx b/test/components/views/dialogs/SpotlightDialog-test.tsx index d842ecb727a..9c987e9c1ca 100644 --- a/test/components/views/dialogs/SpotlightDialog-test.tsx +++ b/test/components/views/dialogs/SpotlightDialog-test.tsx @@ -359,6 +359,31 @@ describe("Spotlight Dialog", () => { expect(options[1]).toHaveTextContent("User Beta"); }); + it("should not filter out users sent by the server even if a local suggestion gets filtered out", async () => { + const member = new RoomMember(testRoom.roomId, testPerson.user_id); + member.name = member.rawDisplayName = testPerson.display_name!; + member.getMxcAvatarUrl = jest.fn().mockReturnValue("mxc://0/avatar"); + mocked(testRoom.getJoinedMembers).mockReturnValue([member]); + mocked(mockedClient.searchUserDirectory).mockResolvedValue({ + results: [ + { user_id: "@janedoe:matrix.org", display_name: "User Alpha", avatar_url: "mxc://1/avatar" }, + { user_id: "@johndoe:matrix.org", display_name: "User Beta", avatar_url: "mxc://2/avatar" }, + ], + limited: false, + }); + + render( null} />); + // search is debounced + jest.advanceTimersByTime(200); + await flushPromisesWithFakeTimers(); + + const content = document.querySelector("#mx_SpotlightDialog_content")!; + const options = content.querySelectorAll("li.mx_SpotlightDialog_option"); + expect(options.length).toBeGreaterThanOrEqual(2); + expect(options[0]).toHaveTextContent(testPerson.display_name!); + expect(options[1]).toHaveTextContent("User Beta"); + }); + it("should start a DM when clicking a person", async () => { render(