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.
Details
- 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 | From the inline comments in D4465:
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) |
web/chat/chat-message-list-container.react.js | ||
---|---|---|
110–136 | 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! |
web/chat/chat-message-list-container.react.js | ||
---|---|---|
110–136 | 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. |
keyserver/src/responders/website-responders.js | ||
---|---|---|
194–195 ↗ | (On Diff #17896) | We should absolutely not be awaiting these sequentially!! |
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) |
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?
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) |
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!