MM-33439: Fetch invited users independently #486

agarciamontoro · 2021-03-03T17:37:40Z

Summary

The component was filtering the invited members out of the list that is fetched when the component is mounted. This list is capped at 100 users (the default for getUsers), so if the invited member was not originally there, it was not shown in the component.

This PR fixes the bug by pre-fetching all the invited members. In particular:

Fetch all the invited members on component mount.
If the user has not typed anything in the search box, just pass the fetched members to react-select as the group of invited members. All the invited members will be there.
If the user has typed something in the search box, rely on the server-side filtering. This is the tricky part: if there are more than 100 hits for your search term, it is possible that the invited member is still lost, but this also happens with non-invited members. The alternative is to do the filtering client-side, but I took a look at the filtering on the server side to try and replicate it and it's quite complex: we're filtering not only by username, but also by the first and last name, and I am sure t]at if we try to replicate that here, it will come back to bite us. That's why I decided it was an acceptable trade-off to rely on the server filtering in this case.

This is a working fix for this dot release, but we still need something more robust for the future (for example, we don't even have pagination in the fetching of the users). I added this to my now long list of things I want to tackle as part of https://mattermost.atlassian.net/browse/MM-32861

About the release itself: I created a branch from the v1.5.1 tag, release-v1.5.2, which is the base for this PR.

Ticket Link

https://mattermost.atlassian.net/browse/MM-33439

webapp/src/components/backstage/automation/invite_users_selector.tsx

lieut-data

Good catch, @agarciamontoro! A few thoughts below.

webapp/src/components/backstage/automation/invite_users_selector.tsx

agarciamontoro · 2021-03-03T18:56:51Z

@lieut-data, addressed one (1) of your comments, I replied to the other ones in place to continue the conversation.

cpoile

Nice catch! This is a tricky one for sure.

Not sure if the comment I gave will help or not, but let me know.

cpoile · 2021-03-03T19:59:00Z

webapp/src/components/backstage/automation/invite_users_selector.tsx

    // When there are no users invited, options is UserProfile[], a plain list. When there is at least one user invited,
    // options contains two groups: the first with invited members, the second with non invited members. This is needed
    // because groups are rendered in the selector list only when there is at least one user invited.
    const [options, setOptions] = useState<UserProfile[] | GroupType<UserProfile>[]>([]);
    const [searchTerm, setSearchTerm] = useState('');
+    const invitedUsers = useSelector<GlobalState, UserProfile[]>((state: GlobalState) => props.userIds.map((id) => getUser(state, id)));


FYI, I believe this will cause the component to rerender everytime the store changes (even unrelated changes) because there's no way for React to know whether or not this selector's dependency inside state has changed. Maybe a better way do do this would be to put it inside a useEffect and state the dependency in dependency array.

Not related to this PR: we should think about cleaning up the other times we do this kind of thing in the rest of the code.

I vaguely remember this discussion in other parts of the codebase, but not the specifics on what caused the re-renders.

I took a look at the docs and, according to this, the component will re-render only if the returned object is different (as in === returning false). It's true that the selector is executed with every change in the state, but the reason for the re-render in this specific case would be that invitedUsers changes. As invitedUseres is an array that gets created every time the selector is executed, it will definitely re-render, but it looks like we can prevent that by using a custom comparison operator.

Or maybe not: reduxjs/react-redux#1654 I don't get it

Anyway, I tested this with the paint flashing tool and I am not able to reproduce what you say. The component does not re-render when I make changes to the store (I tried removing a user, creating a new channel, inviting the user that has the backstage open to a new channel and then kicking them, and posting messages in channels where the user is).
In none of the cases did I get a repaint, so I guess it is not being re-rendered. Maybe I'm missing something else here, of course.

I don't think the issue is the inability to track the selector's dependencies -- Redux always calls all mapStateToProps every time the store changes -- but the fact that this selector always returns a new array even if the underlying conditions haven't changed:

const userIds = ['userid1', 'userid2', 'userid3']; getUser = (userId) => userId; const call1 = userIds.map((id) => getUser(id)); const call2 = userIds.map((id) => getUser(id)); // returns false console.log(call1 === call2);

Redux selectors are /supposed/ to be pure, so I think the pattern above will unconditionally trigger React's "render" phase whenever anything in the state changes. However, React won't actually render anything new to the DOM, since the output of this function will be the same as before. This does come at a cost, however, over skipping the render phase altogether.

I add this comment not that I think we necessarily need to touch anything here -- I see this as a low-bandwidth page whose components won't be mounted very often -- but just to share what I imagine is going on under the covers.

Thank you for the insight! That's what I thought, but it's true that checking the repainting is not enough to test that the rendering phase is triggered again. Is there any easy way to test that? (just for the sake of knowing it for the future)

But I still don't get why the issue I linked before is like that: even when using a custom comparison function, the reload is triggered (the useEffect hook in the example in the issue).

Hmm, this is an interesting issue... Definitely we're good to go for this fix, but I really want to know what's going on under the hood here. Wonder if we should have webapp (or someone from our team) do a deep dive and then a bit of a presentation (a brown bag?) to the rest of the dev team

I've been wanting to do that for a while (also for the sake of my understanding, because I feel like I lack some knowledge on the low level aspects of react and redux)

I might be mistaken, but I think you can just console.log within the functional component. If it's called, that's React's "render" phase. But React reserves the right to render at any time, so it's not a bug to have it render more than strictly necessary.

A deep dive here would be awesome!

cpoile · 2021-03-03T20:00:16Z

webapp/src/components/backstage/automation/invite_users_selector.tsx

+
+    useEffect(() => {
+        dispatch(getProfilesByIds(props.userIds));
+    }, [props.userIds]);


Inside here, for example. Turn invitedUsers into a state hook above, and fill them inside this effect hook.

I /think/ this is perhaps an anti-pattern, having a component juggle second-order state like this. If this is a concern, perhaps we could introduced a memoized selector instead?

webapp/src/components/backstage/automation/invite_users_selector.tsx

agarciamontoro · 2021-03-04T11:18:16Z

I've simplified the useEffect hook by moving all the logic to build the options outside of it, so the dependency on the invitedUsers is implicit. I empirically checked that there is no re-painting happening, but another look into that would be appreciated, as I still don't understand it completely, I think.

lieut-data

Nice simplifications, @agarciamontoro!

cpoile

Nice, thanks @agarciamontoro !

prapti

Tested and verified that invited members and non-invited members still work as expected.
Approving.

* Fetch invited users independently * Use getProfilesByIds * Add a comment clarifying the filter(u => u) idiom * Make useEffect do one thing: get the list of users

Fetch invited users independently

39df20e

agarciamontoro added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Mar 3, 2021

agarciamontoro requested review from lieut-data and cpoile March 3, 2021 17:37

agarciamontoro commented Mar 3, 2021

View reviewed changes

webapp/src/components/backstage/automation/invite_users_selector.tsx Outdated Show resolved Hide resolved

lieut-data requested changes Mar 3, 2021

View reviewed changes

Use getProfilesByIds

65cc38a

agarciamontoro requested a review from lieut-data March 3, 2021 18:54

cpoile requested changes Mar 3, 2021

View reviewed changes

agarciamontoro added 2 commits March 4, 2021 10:43

Add a comment clarifying the filter(u => u) idiom

5d05311

Make useEffect do one thing: get the list of users

5b7f98c

agarciamontoro requested a review from cpoile March 4, 2021 11:29

lieut-data approved these changes Mar 4, 2021

View reviewed changes

cpoile approved these changes Mar 4, 2021

View reviewed changes

agarciamontoro removed the 2: Dev Review Requires review by a core committer label Mar 4, 2021

agarciamontoro requested a review from prapti March 4, 2021 19:16

prapti approved these changes Mar 4, 2021

View reviewed changes

prapti added 4: Reviews Complete All reviewers have approved the pull request and removed 3: QA Review Requires review by a QA tester labels Mar 4, 2021

crspeller changed the base branch from release-v1.5.2 to master March 4, 2021 21:52

crspeller merged commit 3a37483 into master Mar 4, 2021

crspeller deleted the MM-33439.fix-user-selector branch March 4, 2021 21:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MM-33439: Fetch invited users independently #486

MM-33439: Fetch invited users independently #486

agarciamontoro commented Mar 3, 2021 •

edited

Loading

lieut-data left a comment

agarciamontoro commented Mar 3, 2021 •

edited

Loading

cpoile left a comment

cpoile Mar 3, 2021

agarciamontoro Mar 4, 2021 •

edited

Loading

agarciamontoro Mar 4, 2021

agarciamontoro Mar 4, 2021

lieut-data Mar 4, 2021

agarciamontoro Mar 4, 2021

agarciamontoro Mar 4, 2021

cpoile Mar 4, 2021

agarciamontoro Mar 4, 2021

lieut-data Mar 4, 2021

cpoile Mar 3, 2021

lieut-data Mar 4, 2021

agarciamontoro commented Mar 4, 2021

lieut-data left a comment

cpoile left a comment

prapti left a comment

MM-33439: Fetch invited users independently #486

MM-33439: Fetch invited users independently #486

Conversation

agarciamontoro commented Mar 3, 2021 • edited Loading

Summary

Ticket Link

lieut-data left a comment

Choose a reason for hiding this comment

agarciamontoro commented Mar 3, 2021 • edited Loading

cpoile left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agarciamontoro Mar 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agarciamontoro commented Mar 4, 2021

lieut-data left a comment

Choose a reason for hiding this comment

cpoile left a comment

Choose a reason for hiding this comment

prapti left a comment

Choose a reason for hiding this comment

agarciamontoro commented Mar 3, 2021 •

edited

Loading

agarciamontoro commented Mar 3, 2021 •

edited

Loading

agarciamontoro Mar 4, 2021 •

edited

Loading