Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Do not filter users post search #9556

Merged
merged 5 commits into from
Jul 17, 2023
Merged

Do not filter users post search #9556

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c51b5d5
Do not filter users post search
maxmalek Nov 8, 2022
750299c
Merge branch 'develop' into no-user-filter
t3chguy Mar 24, 2023
29c3be1
Merge branch 'develop' of github.com:matrix-org/matrix-react-sdk into…
t3chguy Jul 17, 2023
1eafd8d
Iterate
t3chguy Jul 17, 2023
7bc920f
Iterate
t3chguy Jul 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/components/views/dialogs/spotlight/SpotlightDialog.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -412,7 +412,7 @@ const SpotlightDialog: React.FC<IProps> = ({ initialText = "", initialFilter = n
)
return; // bail, does not match query
} else if (isMemberResult(entry)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to remove the if statement completely

if (!entry.query?.some((q) => q.includes(lcQuery))) return; // bail, does not match query
// Do not filter users
Copy link
Member

@t3chguy t3chguy Mar 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment needs to say WHY (because we rely on the server to filter DIRECTORY results for us)

Copy link
Contributor

@HarHarLinks HarHarLinks Mar 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate why clientside filtering is done in the first place? I think the spec suggests nothing of the kind, and I think additional validation seems superfluous since the server should be trustworthy enough.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably a copy-pasta, but you'd need to ask whomever wrote it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought I was, @t3chguy :) https://github.com/matrix-org/matrix-react-sdk/pull/7804/files#diff-bd33652e7c8d0c14883372ef5febdaeb8a8ce8a53220d72b13a0c953132265feR261

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@HarHarLinks and me investigated it further and found the following:

The component has different sources for the users: (1) the results from the user profile endpoint and (2) all users that joined any room that I am in.

(1) was already server-side filtered using the user entered query:

matrix-react-sdk/src/components/views/dialogs/spotlight/SpotlightDialog.tsx

Line 343 in 7a250f4

useDebouncedCallback(filter === Filter.People, searchPeople, searchParams);

(2) is not pre-filtered, but every user is added to the possible user results:

matrix-react-sdk/src/components/views/dialogs/spotlight/SpotlightDialog.tsx

Lines 358 to 364 in 7a250f4

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));
}

The conditional in discussion is thus required to filter the users in (2):

matrix-react-sdk/src/components/views/dialogs/spotlight/SpotlightDialog.tsx

Lines 414 to 415 in 7a250f4

} else if (isMemberResult(entry)) {
if (!entry.query?.some((q) => q.includes(lcQuery))) return; // bail, does not match query

However, this line will also filter the results that were already filtered by the server. This shouldn't be necessary because I would assume that we can trust that the homeserver returns proper results.

Proposal

We would propose some refactorings to remove the double-filtering of the server results:

  // ...
+ const localUsers = findVisibleRoomMembers(cli, msc3946ProcessDynamicPredecessor).filter(... apply the filtering by trimmedQuery ...);
+ for (const user of [...localUsers, ...users]) {
- 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));
  }
  
  // ...

With this change, the filtering of the possibleResults can be skipped:

- } else if (isMemberResult(entry)) {
-   if (!entry.query?.some((q) => q.includes(lcQuery))) return; // bail, does not match query

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Further, we suggest to add our use-case to the tests.

} else if (isPublicRoomResult(entry)) {
if (!entry.query?.some((q) => q.includes(lcQuery))) return; // bail, does not match query
} else {
Expand Down