[Hold for payment 2024-04-30][$250] Workspace switcher - Expensify is no longer bolded and missing RBR after refreshing the page

[kbecciv]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number:
Reproducible in staging?:
Reproducible in production?:
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by:
Slack conversation:

Action Performed:

Precondition:

• There are a few workspaces with RBR (can be invoked by creating scan request with invalid receipt).

1. Go to staging.new.expensify.com
2. Open the workspace switcher.
3. Select Expensify in the list (if not selected).
4. Refresh the page.

Expected Result:

After refreshing the page,

• the selected Expensify should remain bolded in the list
• RBR will remain next to the workspaces that have it

Actual Result:

After refreshing the page,

• the selected Expensify is not longer bolded in the list
• RBRs disappear

Workaround:

n/a

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6443113_1712641035847.20240409_133128.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01d57cfb7d1d01cfcd
• Upwork Job ID: 1777718400311140352
• Last Price Increase: 2024-04-09
• Automatic offers:
• eh2077 | Reviewer | 0
• ikevin127 | Contributor | 0

[melvin-bot]

Triggered auto assignment to @lschurr (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[kbecciv]

We think that this bug might be related to #vip-vsb

[kbecciv]

@lschurr I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

[FitseTLT]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Workspace switcher - Expensify is no longer bolded and missing RBR after refreshing the page

What is the root cause of that problem?

These functions are returning empty object because the moment they are called (on first render)allReports (which is a global scoped var populated with withonyx) is not popuplated

App/src/pages/WorkspaceSwitcherPage.tsx

Lines 66 to 67 in ad9b5d7

	const brickRoadsForPolicies = useMemo(() => getWorkspacesBrickRoads(), []);
	const unreadStatusesForPolicies = useMemo(() => getWorkspacesUnreadStatuses(), []);

App/src/libs/WorkspacesSettingsUtils.ts

Lines 25 to 26 in ad9b5d7

	callback: (value) => (allReports = value),
	});

What changes do you think we should make in order to solve the problem?

We need make our calculation depend on a component cycle variable (prop), i.e. subscribe to reports collection in WorkspaceSwitcherPage and a pass allReports as a param to both functions and calculated based on that value so that it will be calculated and updated correctly

    const brickRoadsForPolicies = useMemo(() => getWorkspacesBrickRoads(allReports), [allReports]);
    const unreadStatusesForPolicies = useMemo(() => getWorkspacesUnreadStatuses(allReports), [allReports]);

What alternative solutions did you explore? (Optional)

We have many options to solve it we can remove the memo and make it like this

    const brickRoadsForPolicies = getWorkspacesBrickRoads();
    const unreadStatusesForPolicies = getWorkspacesUnreadStatuses();

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01d57cfb7d1d01cfcd

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @eh2077 (External)

[ikevin127]

Proposal

Please re-state the problem that we are trying to solve in this issue

After refreshing the page:

• the selected Expensify is not longer bolded in the list
• RBRs disappear from the other workspaces

What is the root cause of that problem?

We're memoizing the brickRoadsForPolicies and unreadStatusesForPolicies here.

The two functions coming from WorkspacesSettingsUtils are using allReports which is populated via Onyx.connect in the action file (outside of any component lifecycle) and additionally it has waitForCollectionCallback set to true which means: If set to true, it will return the entire collection to the callback as a single object.

Because this is not instant, it causes the memoized functions to always return the data from when allReports is not populated yet, hence brickRoadsForPolicies and unreadStatusesForPolicies don't contain any data -> causing:

• the selected Expensify to not longer have the bold text style
• RBRs disappear from the other workspaces

What changes do you think we should make in order to solve the problem?

Important

We want to preserve the memoization because brickRoadsForPolicies and unreadStatusesForPolicies can contain a lot of data and we don't want to recalculate both functions with every render (unnecessarily).

Therefore I propose adding the reports prop from the bottom of the component via withOnyx then pass reports to both useMemo's dependency array. The change will look like this:

    const brickRoadsForPolicies = useMemo(() => getWorkspacesBrickRoads(), [reports]);
    const unreadStatusesForPolicies = useMemo(() => getWorkspacesUnreadStatuses(), [reports]);

By doing this we make sure that we only recalculate the functions when reports change which is exactly the equivalent of allReports from WorkspacesSettingsUtils here -> so when it changes there, our memoized functions will recalculate, we'll get the data and our issue will be fixed.

Videos (before / after)

MacOS: Chrome / Safari

Before	After
before.mov	after.mov

What alternative solutions did you explore? (Optional)

Since useOnyx was introduced recently within react-native-onyx, first used within this recently merged PR #38924 and confirmed here (Slack) 🧵 that this will be the standard moving forward we can do the following:

Given that both getWorkspacesBrickRoads and getWorkspacesUnreadStatuses WorkspacesSettingsUtils functions are only used within the WorkspaceSwitcherPage, we can make it such that we actually pass the reports and policies as arguments directly from the WorkspaceSwitcherPage component, keeping the memoization while leveraging useOnyx like so:

    const [reports] = useOnyx(ONYXKEYS.COLLECTION.REPORT);
    const [policies] = useOnyx(ONYXKEYS.COLLECTION.POLICY);

    const brickRoadsForPolicies = useMemo(() => getWorkspacesBrickRoads(reports, policies), [reports, policies]);
    const unreadStatusesForPolicies = useMemo(() => getWorkspacesUnreadStatuses(reports), [reports]);

As a clean-up after doing this we can remove the withOnyx HOC and all the associated logic and types from WorkspaceSwitcherPage as useOnyx handles all that 😎

Note

Compred to the video results of the first solution or other solutions, this one is instant -> there's no visible UI transition from non-bold to bold or from no RBR/GBR to visible RBR/GBR, check this out:

MacOS: Chrome / Safari

Screen.Recording.2024-04-11.at.02.46.15.mov

[Skakruk]

Proposal

Please re-state the problem that we are trying to solve in this issue.

After refreshing the page:

• the selected Expensify is not longer bolded in the list
• RBRs disappear from the other workspaces

What is the root cause of that problem?

Both of the functions are memorized

App/src/pages/WorkspaceSwitcherPage.tsx

Lines 66 to 67 in 342b16d

	const brickRoadsForPolicies = useMemo(() => getWorkspacesBrickRoads(), []);
	const unreadStatusesForPolicies = useMemo(() => getWorkspacesUnreadStatuses(), []);

and will not retrieve new data coming from Onyx.connect

App/src/libs/WorkspacesSettingsUtils.ts

Lines 22 to 26 in b394f3d

	Onyx.connect({
	key: ONYXKEYS.COLLECTION.REPORT,
	waitForCollectionCallback: true,
	callback: (value) => (allReports = value),
	});

What changes do you think we should make to solve the problem?

I propose to wrap Onyx.connect, getWorkspacesBrickRoads() and getWorkspacesUnreadStatuses() in separate custom hooks.

Both of them will be similar. We subscribe to connection callback, setting the internal state for retrieved data.
Pseudocode

const [state, setState] = useState();
useEffect(() => {
    const connectionID = Onyx.connect(...., callback: (val) => setState(val));
    return () => Onyx.disconnect(connectionID);
}, []);

Then we can process the retrieved data (process brick data or determine unread status) and memorize it.

Later, in WorkspaceSwitcherPage.tsx we use those hooks, replacing existing ones. So that data is already memoized.

const unreadStatusesForPolicies = useUnreadStatusesForPolicies();

In this case, we are getting the correct information about reports and policies immediately. No matter if it's on web app start or later in use when we receive updates on reports.

What alternative solutions did you explore? (Optional)

[allgandalf]

Proposal

Please re-state the problem that we are trying to solve in this issue.

State of workspaces like selected option boldness and RBR don't stay consistent after reload

What is the root cause of that problem?

Currently we use useMemo to memorise the values of brickRoadsForPolicies and unreadStatusesForPolicies:

App/src/pages/WorkspaceSwitcherPage.tsx

Lines 66 to 67 in ad9b5d7

	const brickRoadsForPolicies = useMemo(() => getWorkspacesBrickRoads(), []);
	const unreadStatusesForPolicies = useMemo(() => getWorkspacesUnreadStatuses(), []);

But during the very first render, these field are undefined as still have received the values of these consts from getWorkspacesBrickRoads and getWorkspacesUnreadStatuses are empty objects {}

What changes do you think we should make in order to solve the problem?

We already have subscribed to policies data from Onyx:

App/src/pages/WorkspaceSwitcherPage.tsx

Lines 285 to 288 in ad9b5d7

	export default withOnyx<WorkspaceSwitcherPageProps, WorkspaceSwitcherPageOnyxProps>({
	policies: {
	key: ONYXKEYS.COLLECTION.POLICY,
	},

So we should put policies as a dependency of useMemo, this will automatically store the latest data about all the policies when we have populated all the data about policies from Onyx:

    const brickRoadsForPolicies = useMemo(() => getWorkspacesBrickRoads(), [policies]);
    const unreadStatusesForPolicies = useMemo(() => getWorkspacesUnreadStatuses(), [policies]);

Result Video

Screen.Recording.2024-04-11.at.5.00.54.AM.mov

[ikevin127]

Updated proposal

• added alternative solution motivated by recent developments / addition of useOnyx hook within react-native-onyx and first usage within PR: Refactor some components / screens to use useOnyx #38924

[eh2077]

Reviewing proposals

[eh2077]

Thank you all for your proposals!

@ikevin127 's proposal looks good to me. I agree with their RCA and their alternative solution to use the new hook useOnyx.

🎀👀🎀 C+ reviewed

[melvin-bot]

Triggered auto assignment to @iwiznia, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[FitseTLT]

@eh2077 I really disagree with your choice because the selected proposal doesn't have any significant difference in RCA and also solution than mine which was proposed way before the selected one. Using a useOnyx over withOnyx is a tiny implementation detail, infact which we are just only starting to use, and that can be easily negotiated in PR stage. Please reconsider your review.
cc @iwiznia

[Skakruk]

While debugging this issue, I spotted a bug with userWorkspaces list, that WorkspaceSwitcherPage always display workspace names in bold styles, no matter if there are any unread messages or not.

It's because UserListItem expects item to have property isBold,

App/src/components/SelectionList/UserListItem.tsx

Line 120 in 1eb3065

item.isBold !== false && styles.sidebarLinkTextBold,

but we provide property boldStyle

App/src/pages/WorkspaceSwitcherPage.tsx

Line 136 in 479efd6

boldStyle: hasUnreadData(policy?.id),

Changing line to isBold: hasUnreadData(policy?.id) ?? false, fixes the issue

[eh2077]

@eh2077 I really disagree with your choice because the selected proposal doesn't have any significant difference in RCA and also solution than mine which was proposed way before the selected one. Using a useOnyx over withOnyx is a tiny implementation detail, infact which we are just only starting to use, and that can be easily negotiated in PR stage. Please reconsider your review. cc @iwiznia

@FitseTLT Thanks for your comment. I would definitely vote for your proposal if useOnyx were just a new syntactic sugar, but it's not. It eliminates the UI transition. So, I tend to stick to my original decision.

[eh2077]

@Skakruk Thank you for sharing your finding!

[ikevin127]

@iwiznia All good for assignment here based on #39928 (comment) and #39928 (comment) ?
I noticed you 👍 the second one yesterday!

[melvin-bot]

📣 @eh2077 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

[melvin-bot]

📣 @ikevin127 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[ikevin127]

PR #40197 is ready for review! 🎉

[ikevin127]

⚠️ Automation failed here -> this should be on Hold for payment [2024-04-30] according to yesterday's production deploy from #40197 (comment).

cc @lschurr

[lschurr]

Thanks @ikevin127!

[lschurr]

Payment summary:

• Contributor: @ikevin127 $250 (paid in Upwork)
• Contributor+: @eh2077 $250 (paid in Upwork)