Page MenuHomePhabricator

[keyserver] Update userStore from chat creation url
AbandonedPublic

Authored by kuba on Oct 24 2022, 6:14 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Feb 22, 2:02 AM
Unknown Object (File)
Thu, Feb 22, 2:02 AM
Unknown Object (File)
Thu, Feb 22, 2:02 AM
Unknown Object (File)
Thu, Feb 22, 2:01 AM
Unknown Object (File)
Thu, Feb 22, 12:55 AM
Unknown Object (File)
Thu, Feb 22, 12:55 AM
Unknown Object (File)
Thu, Feb 22, 12:55 AM
Unknown Object (File)
Thu, Feb 22, 12:55 AM
Subscribers

Details

Summary

Part of ENG-1988
This diff modifies website-responders.js to update the userStore when you navigate to a thread composer with users that you don't have in the userStore. (/chat/thread/new/<user-id-not-in-user-store>), by updating the relationship between the users.

Test Plan
  • Navigate to a page with an id of a user that isn't in userStore. The user should be added to the userStore
  • Try a non-existant userID, it should get removed from the url.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

keyserver/src/responders/website-responders.js
152–169 ↗(On Diff #17845)

Can we avoid directly awaiting here? This approach leads to blocking the rest of execution, but instead we could start the Promise and await it where initialNavInfo is needed, eg. fetchEntryInfos and setNewSession which both need calendarQuery, and of course navInfoPromise itself.

Changed awaits to not use them directly. Now we await in two places:

  • await before fetchKnownUserInfos so the relationships are updated
  • inside navInfoPromise to get validUserIDs

We don't have to await before calendarQuery because it uses only the startDate and endData fields.

web/chat/chat-message-list-container.react.js
110–136 ↗(On Diff #17852)

Interesting!! Looks like this code was intentionally introduced in D4465.

Are you sure it needs to be removed? If the user ID isn't in Redux, we won't be able to show them in the UI, right?

michal added inline comments.
web/chat/chat-message-list-container.react.js
110–136 ↗(On Diff #17852)

From the inline comments in D4465:

What is the scenario in which this effect is needed?

When user enter manually IDs into URL (like http://localhost/comm/chat/thread/new/84072,83850,99999) and some of them are not in userStore.

This is handled by this diff. It's not possible to enter IDs that are not in the userStore from the UI. (this is changed in the next diff, but it's handled there correctly)

ashoat requested changes to this revision.Oct 25 2022, 5:44 AM
ashoat added inline comments.
web/chat/chat-message-list-container.react.js
110–136 ↗(On Diff #17852)

I'm confused why we need to remove this code that clears IDs that aren't in the userStore. We can rely on the changes in website-responders.js to make sure that the IDs entered manually into the URL are in the userStore, and if that code was broken for some reason, then we wouldn't be able to display those users in the UI since they are not in the userStore... so it seems to me like we should keep this code.

Let me know if I'm missing something!

This revision now requires changes to proceed.Oct 25 2022, 5:44 AM
web/chat/chat-message-list-container.react.js
110–136 ↗(On Diff #17852)

Even if the code in website-responders.js is broken the UI will be correct. The ids from navInfo are assigned to the selectedUserIDs but then they are immediately filtered using data in the userStore:

const selectedUserIDs = useSelector(state => state.navInfo.selectedUserList);
const otherUserInfos = useSelector(userInfoSelectorForPotentialMembers);
const userInfoInputArray: $ReadOnlyArray<AccountUserInfo> = React.useMemo(
    () => selectedUserIDs?.map(id => otherUserInfos[id]).filter(Boolean) ?? [],
    [otherUserInfos, selectedUserIDs],
);

And selectedUserIDs isn't used anywhere else (later we only use userInfoInputArray, which contains only users that are in the userStore).

The only thing that's changed by removal of this fragment of code is that the URL won't be updated on page load, but only when user tries to add/remove someone in the chat composer.

But it's going to be changed in the next diff anyway (where the mentioned change will be handled correctly) so I'm going to revert this deletion here.

Revert the removal of useEffect

ashoat requested changes to this revision.Oct 25 2022, 7:26 AM
ashoat added inline comments.
keyserver/src/responders/website-responders.js
194–195 ↗(On Diff #17896)

We should absolutely not be awaiting these sequentially!!

This revision now requires changes to proceed.Oct 25 2022, 7:26 AM

By the way, I'm generally not a good first-pass reviewer... I usually only have time to do a quick skim, and so it can lead to multiple rounds of review, compared to if somebody who has more time takes a look 😅

Apologies for the multiple rounds here!! In the future you might consider leaving me off of the review until the final step, although that could potentially lead to more churn if I'm the only one who notices a serious issue...

keyserver/src/responders/website-responders.js
172 ↗(On Diff #17896)

Do we really need to create this relationship? It means that any user can fill up another user's userStore just by constructing URLs. Can we avoid this issue somehow?

194–195 ↗(On Diff #17896)

Ignore this, I think I can tell what's going on now...

In the future you might consider leaving me off of the review until the final step, although that could potentially lead to more churn if I'm the only one who notices a serious issue...

Realizing I may have just added myself... if so, double apologies!

I think you were added when you requested changes? :)

keyserver/src/responders/website-responders.js
172 ↗(On Diff #17896)

It means that any user can fill up another user's userStore just by constructing URLs

This should add only a relationship between the current user and some other user (and not between any pair of users). Currently the same can be achieved by just blocking/ sending a friend request to someone. So I can't see how you could fill up someone else's userStore?

Do we really need to create this relationship?

I assumed that we need to keep in sync relationships on keyserver and userStore (you have someone in userStore <=> you have at least a KNOW_OF relationship). If we don't we could just append userInfos from the URL.

194–195 ↗(On Diff #17896)

If we continue with this approach do you think I should add a comment or use then? Or is it clear enough?

keyserver/src/responders/website-responders.js
172 ↗(On Diff #17896)

If we don't we could just append userInfos from the URL.

I think that would be the best approach in this case. I don't think we need to permanently create this relationship... on the other hand, blocking/sending a friend request requires some permanent data storage on the client.

(Good point that friend request has a similar effect, though...)

194–195 ↗(On Diff #17896)

No, if we continue with this approach honestly your code is fine. I was just not reading thoroughly enough – this is my bad.

I think you were added when you requested changes? :)

Yeah, that's definitely what happened... sorry for the confusion!

Don't create the relationship

This revision is now accepted and ready to land.Nov 2 2022, 4:55 AM
kuba added a reviewer: michal.

As discussed with @michal, it is now done with a different approach.