[web] update toggleUser to use previouslySelectedUsers map if previously selected user is toggled
ClosedPublic
Actions

Authored by ginsu on Jan 25 2024, 1:18 PM.

Details

Reviewers

atul
inka

Commits

rCOMM40e2da777056: [web] update toggleUser to use previouslySelectedUsers map if previously…

Summary

PLEASE NOTE THAT THIS DIFF AND SUBSEQUENT DIFFS IN THIS STACK WILL NOT BE LANDED UNTIL MORE OF THE REDESIGN IS READY SINCE THIS WILL CAUSE REGRESSIONS IN PROD

As I was testing the new add users list, I ran into a case where the app would crash if I selected a user that I had searched for a user that was selected from the userStoreSearchResults cleared the search text, so this user was part of the previously selected users section and then unselected that user and then reselected the user again

This is because w/o the search text present mergedUserInfos does not have context regarding that searched user's user info. However, the previouslySelectedUsers still has the GlobalAccountUserInfo for that user stored.

We need to add a check to toggleUser that if the user toggled is a previously selected user, we should just grab the GlobalAccountUserInfo for that user from previouslySelectedUsers to prevent this crash from happening

Linear task: https://linear.app/comm/issue/ENG-5960/selected-users-section

Depends on D10822

Test Plan

Please see demo video below

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

ginsu created this revision.Jan 25 2024, 1:18 PM

Herald added subscribers: tomek, ashoat. · View Herald TranscriptJan 25 2024, 1:18 PM

ginsu edited the summary of this revision. (Show Details)Jan 25 2024, 1:30 PM

ginsu edited the test plan for this revision. (Show Details)

ginsu added reviewers: atul, inka.

ginsu added a parent revision: D10822: [web] introduce previouslySelectedUsers ui to add users list.

ginsu attached a referenced file: F1088852: Screen Recording 2024-01-25 at 4.29.32 PM.mov. (Show Details)

ginsu attached a referenced file: F1088842: Screen Recording 2024-01-25 at 3.36.29 PM.mov. (Show Details)

ginsu edited the summary of this revision. (Show Details)

Harbormaster completed remote builds in B26166: Diff 36143.Jan 25 2024, 1:37 PM

ginsu requested review of this revision.Jan 25 2024, 1:37 PM

ginsu added a child revision: D10849: [web] fix button in add friends modal not visible on small screens.Jan 26 2024, 2:04 PM

Accepting, but think it would make sense to replace invariant with a simple if statement.

web/settings/relationship/add-users-list.react.js
126–129 ↗	(On Diff #36143)	Could we replace `invariant` with a simple truthiness check? Something like const newPendingUser = vipPendingUsers.get(userID); if (newPendingUser) { newPendingUsers.set(userID, newPendingUser); }

This revision is now accepted and ready to land.Jan 29 2024, 8:41 PM

ginsu mentioned this in D10822: [web] introduce previouslySelectedUsers ui to add users list.Jan 31 2024, 2:55 PM

address comments

ginsu edited the summary of this revision. (Show Details)Jan 31 2024, 4:40 PM

ginsu retitled this revision from [web] update toggleUser to use vip users map if vip user is toggled to [web] update toggleUser to use previouslySelectedUsers map if previously selected user is toggled.

ginsu edited the summary of this revision. (Show Details)

Harbormaster completed remote builds in B26396: Diff 36497.Jan 31 2024, 4:58 PM

rebase before landing

This revision was landed with ongoing or failed builds.Feb 15 2024, 1:19 AM

Closed by commit rCOMM40e2da777056: [web] update toggleUser to use previouslySelectedUsers map if previously… (authored by ginsu). · Explain Why

This revision was automatically updated to reflect the committed changes.

ginsu added a commit: rCOMM40e2da777056: [web] update toggleUser to use previouslySelectedUsers map if previously….

Harbormaster completed remote builds in B26890: Diff 37181.Feb 15 2024, 1:43 AM