From d240f0681038d60e501f1e801776f4f4a5aa8738 Mon Sep 17 00:00:00 2001 From: Milton Moura Date: Thu, 10 Aug 2023 17:27:24 +0000 Subject: [PATCH] Include non-matching DMs in Spotlight recent conversations when the DM's userId is part of the search API results (#11374) * This addresses two issues: 1. Include non-matching DMs in Spotlight suggestions if the userId of the DM is included in the user directory search results 2. The user directory search results order is kept when there is no relevant activity between users, instead of sorting by MXID * Applying feedback from PR: 1. Updated comments 2. Renamed users to userDirectorySearchResults 3. Makes sure linter is happy --- .../dialogs/spotlight/SpotlightDialog.tsx | 26 +++++--- src/utils/SortMembers.ts | 5 +- .../views/dialogs/SpotlightDialog-test.tsx | 61 +++++++++++++++++++ 3 files changed, 80 insertions(+), 12 deletions(-) diff --git a/src/components/views/dialogs/spotlight/SpotlightDialog.tsx b/src/components/views/dialogs/spotlight/SpotlightDialog.tsx index 1ac24c9e7b..8ccffaf1b8 100644 --- a/src/components/views/dialogs/spotlight/SpotlightDialog.tsx +++ b/src/components/views/dialogs/spotlight/SpotlightDialog.tsx @@ -315,7 +315,7 @@ const SpotlightDialog: React.FC = ({ initialText = "", initialFilter = n } = usePublicRoomDirectory(); const [showRooms, setShowRooms] = useState(true); const [showSpaces, setShowSpaces] = useState(false); - const { loading: peopleLoading, users, search: searchPeople } = useUserDirectory(); + const { loading: peopleLoading, users: userDirectorySearchResults, search: searchPeople } = useUserDirectory(); const { loading: profileLoading, profile, search: searchProfileInfo } = useProfileInfo(); const searchParams: [IDirectoryOpts] = useMemo( () => [ @@ -363,7 +363,7 @@ const SpotlightDialog: React.FC = ({ initialText = "", initialFilter = n } } addUserResults(findVisibleRoomMembers(visibleRooms, cli), false); - addUserResults(users, true); + addUserResults(userDirectorySearchResults, true); if (profile) { addUserResults([new DirectoryMember(profile)], true); } @@ -389,7 +389,7 @@ const SpotlightDialog: React.FC = ({ initialText = "", initialFilter = n ...userResults, ...publicRooms.map(toPublicRoomResult), ].filter((result) => filter === null || result.filter.includes(filter)); - }, [cli, users, profile, publicRooms, filter, msc3946ProcessDynamicPredecessor]); + }, [cli, userDirectorySearchResults, profile, publicRooms, filter, msc3946ProcessDynamicPredecessor]); const results = useMemo>(() => { const results: Record = { @@ -407,12 +407,18 @@ const SpotlightDialog: React.FC = ({ initialText = "", initialFilter = n possibleResults.forEach((entry) => { if (isRoomResult(entry)) { - if ( - !entry.room.normalizedName?.includes(normalizedQuery) && - !entry.room.getCanonicalAlias()?.toLowerCase().includes(lcQuery) && - !entry.query?.some((q) => q.includes(lcQuery)) - ) - return; // bail, does not match query + // If the room is a DM with a user that is part of the user directory search results, + // we can assume the user is a relevant result, so include the DM with them too. + const userId = DMRoomMap.shared().getUserIdForRoomId(entry.room.roomId); + if (!userDirectorySearchResults.some((user) => user.userId === userId)) { + if ( + !entry.room.normalizedName?.includes(normalizedQuery) && + !entry.room.getCanonicalAlias()?.toLowerCase().includes(lcQuery) && + !entry.query?.some((q) => q.includes(lcQuery)) + ) { + return; // bail, does not match query + } + } } else if (isMemberResult(entry)) { if (!entry.alreadyFiltered && !entry.query?.some((q) => q.includes(lcQuery))) return; // bail, does not match query } else if (isPublicRoomResult(entry)) { @@ -463,7 +469,7 @@ const SpotlightDialog: React.FC = ({ initialText = "", initialFilter = n } return results; - }, [trimmedQuery, filter, cli, possibleResults, memberComparator]); + }, [trimmedQuery, filter, cli, possibleResults, userDirectorySearchResults, memberComparator]); const numResults = sum(Object.values(results).map((it) => it.length)); useWebSearchMetrics(numResults, query.length, true); diff --git a/src/utils/SortMembers.ts b/src/utils/SortMembers.ts index 8be4e8a939..534fe6a82e 100644 --- a/src/utils/SortMembers.ts +++ b/src/utils/SortMembers.ts @@ -16,7 +16,6 @@ limitations under the License. import { groupBy, mapValues, maxBy, minBy, sumBy, takeRight } from "lodash"; import { MatrixClient, Room, RoomMember } from "matrix-js-sdk/src/matrix"; -import { compare } from "matrix-js-sdk/src/utils"; import { Member } from "./direct-messages"; import DMRoomMap from "./DMRoomMap"; @@ -39,7 +38,9 @@ export const compareMembers = if (aScore === bScore) { if (aNumRooms === bNumRooms) { - return compare(a.userId, b.userId); + // If there is no activity between members, + // keep the order received from the user directory search results + return 0; } return bNumRooms - aNumRooms; diff --git a/test/components/views/dialogs/SpotlightDialog-test.tsx b/test/components/views/dialogs/SpotlightDialog-test.tsx index d969998378..0a0c1eb6c6 100644 --- a/test/components/views/dialogs/SpotlightDialog-test.tsx +++ b/test/components/views/dialogs/SpotlightDialog-test.tsx @@ -143,7 +143,11 @@ describe("Spotlight Dialog", () => { guest_can_join: false, }; + const testDMRoomId = "!testDM:example.com"; + const testDMUserId = "@alice:matrix.org"; + let testRoom: Room; + let testDM: Room; let testLocalRoom: LocalRoom; let mockedClient: MatrixClient; @@ -159,6 +163,19 @@ describe("Spotlight Dialog", () => { jest.spyOn(DMRoomMap, "shared").mockReturnValue({ getUserIdForRoomId: jest.fn(), } as unknown as DMRoomMap); + + testDM = mkRoom(mockedClient, testDMRoomId); + testDM.name = "Chat with Alice"; + mocked(testDM.getMyMembership).mockReturnValue("join"); + + mocked(DMRoomMap.shared().getUserIdForRoomId).mockImplementation((roomId: string) => { + if (roomId === testDMRoomId) { + return testDMUserId; + } + return undefined; + }); + + mocked(mockedClient.getVisibleRooms).mockReturnValue([testRoom, testLocalRoom, testDM]); }); describe("should apply filters supplied via props", () => { @@ -391,6 +408,50 @@ describe("Spotlight Dialog", () => { expect(options[1]).toHaveTextContent("User Beta"); }); + it("show non-matching query members with DMs if they are present in the server search results", async () => { + mocked(mockedClient.searchUserDirectory).mockResolvedValue({ + results: [ + { user_id: testDMUserId, display_name: "Alice Wonder", avatar_url: "mxc://1/avatar" }, + { user_id: "@bob:matrix.org", display_name: "Bob Wonder", 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(testDMUserId); + expect(options[1]).toHaveTextContent("Bob Wonder"); + }); + + it("don't sort the order of users sent by the server", async () => { + const serverList = [ + { user_id: "@user2:server", display_name: "User Beta", avatar_url: "mxc://2/avatar" }, + { user_id: "@user1:server", display_name: "User Alpha", avatar_url: "mxc://1/avatar" }, + ]; + mocked(mockedClient.searchUserDirectory).mockResolvedValue({ + results: serverList, + 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 Beta"); + expect(options[1]).toHaveTextContent("User Alpha"); + }); + it("should start a DM when clicking a person", async () => { render(