[HOLD for payment 2024-04-15] [$250] [Wave Collect] As we switch between the features of Collect workspace the read api commands are not triggered everytime

[mountiny]

Problem

As you can see in the video here #38135 (review), in Collect workspace (needs to be created from OldDot) settings in NewDot, user has multiple features admin can open in the LHP.

When the feature page is open a read API command should get triggered to make sure all the data are up-to-date.

But as you can see it does not happen reliably. @fabioh8010 has investigated and left some information here #38135 (comment)

Solution

Fix this for all the workspace pages so opening the page will always call the appropriate API command.

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01fe46178cda4b28aa
• Upwork Job ID: 1768569366516178944
• Last Price Increase: 2024-03-15
• Automatic offers:
• tienifr | Contributor | 0

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

Triggered auto assignment to @lschurr (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details.

[melvin-bot]

Upwork job price has been updated to $250

[tienifr]

Proposal

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

When the feature page is open a read API command should get triggered to make sure all the data are up-to-date.

What is the root cause of that problem?

Currently we don't have any logic to trigger the read API command when the page is opened, only when it's mounted for the first time.

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

Add logic to trigger the read API command when the page is opened, we can use useIsFocused, and trigger the read command whenever isFocused comes from false to true.

useEffect(() => {
    if (isFocused && !prevIsFocused) {
        getWorkspaceMembers();
    }
}, [isFocused, prevIsFocused]);

Or we can refactor a bit by removing the "trigger read API command" logic on mount like here, and rely totally in isFocused, because the useEffect that has isFocused as dependency, will also trigger the first time the screen is mounted.

useEffect(() => {
    if (isFocused) {
        getWorkspaceMembers();
    }
}, [isFocused]);

We should apply similar logic to all the feature page (or if not required in all, in those pages that we want the behavior). Each feature page will have 1 such "trigger read command" method, like here or here

What alternative solutions did you explore? (Optional)

NA

[ikevin127]

Proposal

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

As we switch between the pages of Collect workspace via LHN on large screen devices, the API read calls are not triggered reliably.

What is the root cause of that problem?

Note

This works as expected (not reproducible) on narrow layout devices (mweb / native) where LHN is its own page (in the navigation stack) instead of a sidebar with the pages opening on the right side of the LHN.

The issue arises due to the way component lifecycle works in conjunction with React Navigation on large screen devices. On narrow layout devices each LHN menu item opens a new page (in the navigation stack) which causes the component to always mount -> triggering the useEffect API read call reliably everytime a page is navigated to.

Conversely, on large screens where LHN acts as a sidebar, the pages are not always unmounting when navigating to another page by clicking on another LHN menu item, having the components remain mounted in the DOM -> leading to the useEffect not being run -> not calling the API read reliably.

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

Given how the navigation logic was implemented within the WorkspaceInitialPage.tsx component my proposed solution is to take advantage of existing Navigation.goBack() function and change the way we handle navigating between the pages in a way which aligns the large screen devices behaviour with the one experienced on narrow layout devices.

For this we have to replace the menu item action navigation function like so (Profile example):

- Navigation.navigate(ROUTES.WORKSPACE_PROFILE.getRoute(policyID))
+ Navigation.goBack(ROUTES.WORKSPACE_PROFILE.getRoute(policyID), true, true)

Note

We will replace the navigation function in the same way for all LHN menu item pages:

• Common: Profile, Members
• Free: Cards, Reimbursements, Bills, Invoices, Travel, Bank account
• Collect: Distance rates, Workflows, Categories, Tags, Taxes, More features

To better understand what all the Navigation.goBack() arguments are doing:

Spoiler (open)

/**
 * @param fallbackRoute - Fallback route if pop/goBack action should, but is not possible within RHP
 * @param shouldEnforceFallback - Enforces navigation to fallback route
 * @param shouldPopToTop - Should we navigate to LHN on back press
 */
function goBack(fallbackRoute?: Route, shouldEnforceFallback = false, shouldPopToTop = false) {
    //...existing logic

Important

By implementing this solution we ensure that on large screen devices where LHN acts as a sidebar, whenever we click on a LHN menu item we always unmount (shouldPopToTop) and re-mount the page.

Videos

MacOS: Chrome / Safari

Before	After
before.mov	after.mov

What alternative solutions did you explore? (Optional)

Warning

The alternative solution would be using navigation focus as suggested by #38378 (comment), but this would not work for the case where say I'm already on the Profile page and I click the LHN Profile menu item again and again -> the API read call would not be made since there's no previous / current page focus change.

[wildan-m]

Proposal

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

As we switch between the features of Collect workspace the read api commands are not triggered everytime

What is the root cause of that problem?

We only fetch data on mount, and the screen is not always unmounted in React Navigation.

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

We can refetch the data when it is refocused. We can combine useFocusEffect with useCallback to optimize it.

Replace onMount useEffect with this to each menu page.

    useFocusEffect(
            fetchData();
    );

Or

    useFocusEffect(
        React.useCallback(() => {
            fetchData();
        }, [])
    );

This can be applied to all menu features

What alternative solutions did you explore? (Optional)

[mountiny]

Waiting for C+ review @eVoloshchak

[eVoloshchak]

Thank you all for the proposals!

Ultimately there are two ways to resolve the issue

1. Using useIsFocused to re-fetch data every time the screen is focused (originally proposed by @fabioh8010 in Integrate OpenPolicyMoreFeaturesPage command #38135 (comment), proposed here by @tienifr and @wildan-m(this is ultimately the same approach that was proposed by @fabioh8010 and @tienifr, but extended))

2. Using shouldPopToTop to un-mount the page then we're navigating from it to ensure the data is refreshed when we come back to the page (proposed by @ikevin127 in [HOLD for payment 2024-04-15] [$250] [Wave Collect] As we switch between the features of Collect workspace the read api commands are not triggered everytime #38378 (comment))

Both approaches do resolve the issue, however I'm a bit wary of using Navigation.goBack for a button that isn't, in fact, a back button. But the main thing is, subscribing to isFocused is less expensive from the performance standpoint than re-rendering the whole page.
Based on that, I think we should proceed with @tienifr's proposal

🎀👀🎀 C+ reviewed!

[melvin-bot]

Current assignee @mountiny is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[mountiny]

Thanks @eVoloshchak I agree with your conclussion

[melvin-bot]

📣 @tienifr 🎉 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 📖

[wildan-m]

@eVoloshchak, the solution to this issue is straightforward, and I believe a little optimization could make a difference. In this case, useFocusEffect not only provides an efficient and optimized approach to managing side effects in React but also enhances code readability. Would you please reconsider?

[tienifr]

Thanks for the assignment! I'll open the PR shortly 👍

the solution to this issue is straightforward, and I believe a little optimization could make a difference.

@wildan-m That's something that can be handled in the PR phase. Code diff were only provided as example for ease of testing, the core approach here is "call the API when the page is focused", and yours are the same.

a little optimization could make a difference

For something that is called only once when entering a page, there'll be no noticeable difference.

[wildan-m]

If the decision is final, that's fine.

the core approach here is "call the API when the page is focused"

@tienifr By the way, that doesn't belong to you either, it's already identified in the original post link. #38135 (comment).

[tienifr]

PR ready for review #38613.

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.60-13 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Fix: Read API is not triggered consistently when switching between features of a Collect workspace #38613

If no regressions arise, payment will be issued on 2024-04-15. 🎊

For reference, here are some details about the assignees on this issue:

• @eVoloshchak requires payment through NewDot Manual Requests
• @tienifr requires payment automatic offer (Contributor)

[melvin-bot]

BugZero Checklist: The PR adding this new feature has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@eVoloshchak] Please propose regression test steps to ensure the new feature will work correctly on production in further releases.
• [@lschurr] Link the GH issue for creating/updating the regression test once above steps have been agreed upon.

[lschurr]

@eVoloshchak - Do we need a regression test for this one?

[lschurr]

Payment summary:

• Contributor: @tienifr $250 (paid in Upwork)
• Contributor+: @eVoloshchak $250 (please request in Newdot)

[eVoloshchak]

Regression Test Proposal

1. Open the network console (web/Desktop) or Show Element Inspector -> Network (native iOS/Android)
2. Open a collect workspace settings page
3. Try to switch between Taxes and More features pages
4. Verify that every time you switch to a new page, the corresponding Open API is called (OpenPolicyTaxesPage and OpenPolicyMoreFeaturesPage respectively in this case)
5. Verify the above with other all other pages

Do we agree 👍 or 👎

[JmillsExpensify]

$250 approved for @eVoloshchak