[$250] Web - Workspace - Members are unselected when refresh Add message page and go back

IuliiaHerets commented 2 weeks ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Version Number: 9.0.34-2 Reproducible in staging?: Y Reproducible in production?: Y Email or phone of affected tester (no customers): applausetester+gm103117@applause.expensifail.com Issue reported by: Applause Internal Team

Action Performed:

Create Workspace
Go to Members> Invite> Enter any email> Next
Refresh the Add message page
Go back to Invite members page

Expected Result:

Members should be selected

Actual Result:

Members are deselected

Workaround:

Unknown

Platforms:

[ ] Android: Native
[ ] Android: mWeb Chrome
[ ] iOS: Native
[ ] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[ ] MacOS: Desktop

Screenshots/Videos

https://github.com/user-attachments/assets/c53b091f-41d8-46cc-beb4-65f0d46339ce

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~021836242370984040873
Upwork Job ID: 1836242370984040873
Last Price Increase: 2024-09-25

Issue Owner

Current Issue Owner: @alitoshmatov

melvin-bot[bot] commented 2 weeks ago

Triggered auto assignment to @RachCHopkins (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

IuliiaHerets commented 2 weeks ago

@RachCHopkins FYI 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

nyomanjyotisa commented 2 weeks ago

Proposal

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

Web - Workspace - Members are unselected when refresh Add message page and go back

What is the root cause of that problem?

The useEffect in here clears selected members when the component unmounts

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

Remove the following codes https://github.com/Expensify/App/blob/19d037b3a900b08de1f8ba2f22624ba445abe3a5/src/pages/workspace/WorkspaceInvitePage.tsx#L78-L83

What alternative solutions did you explore? (Optional)

Krishna2323 commented 2 weeks ago

Proposal 

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

Web - Workspace - Members are unselected when refresh Add message page and go back

What is the root cause of that problem?

The useEffect to clear the selected participants runs on initial render because we have route.params.policyID in the dependencies. https://github.com/Expensify/App/blob/d0a51b846cb42e230cd7ca659193fc147bf5ebbe/src/pages/workspace/WorkspaceInvitePage.tsx#L78-L83
What changes do you think we should make in order to solve the problem? 
We can remove route.params.policyID from the dependencies list.

What alternative solutions did you explore? (Optional)

We can update the useEffect to only run the cleanup function when component has been already mounted for once.

    useEffect(() => {
        return () => {
            if (firstRenderRef.current) {
                return;
            }
            Member.setWorkspaceInviteMembersDraft(route.params.policyID, {});
        };
        // eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps
    }, [route.params.policyID]);

Result

RachCHopkins commented 1 week ago

Reproduced!

melvin-bot[bot] commented 1 week ago

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

melvin-bot[bot] commented 1 week ago

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

melvin-bot[bot] commented 1 week ago

@RachCHopkins, @alitoshmatov Huh... This is 4 days overdue. Who can take care of this?

truph01 commented 1 week ago

Proposal

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

Members are deselected

What is the root cause of that problem?

This feature worked well after PR but PR caused this bug. Reverting that PR will fix this bug. In that PR, we introduce a new condition isLoadingPolicy in: https://github.com/Expensify/App/blob/f8add4e2a04b5e3e2aee05fcfe17771bf595051f/src/pages/workspace/withPolicyAndFullscreenLoading.tsx#L41 The WorkspaceInvitePage component is wrapped in withPolicyAndFullscreenLoading. In withPolicyAndFullscreenLoading, the isLoadingPolicy will be changed from false > true > false ... > false, hence the WorkspaceInvitePage component is mounted > unmounted > mounted.
As observed, when we visit the workspace invite page, the WorkspaceInvitePage component mounts twice and unmounts once. This causes the cleanup function

https://github.com/Expensify/App/blob/f8add4e2a04b5e3e2aee05fcfe17771bf595051f/src/pages/workspace/WorkspaceInvitePage.tsx#L79-L81

to be called unnecessarily, contributing to the current bug and impacting the app's performance.

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

The cleanup function: https://github.com/Expensify/App/blob/f8add4e2a04b5e3e2aee05fcfe17771bf595051f/src/pages/workspace/WorkspaceInvitePage.tsx#L80 should be called when the workspace invite page is unmounted, not when the WorkspaceInvitePage is unmounted. These two components are different: WorkspaceInvitePage is wrapped inside withPolicyAndFullscreenLoading, in other words, WorkspaceInvitePage is wrapped inside the workspace invite page.
So we need to remove that clean-up function:

https://github.com/Expensify/App/blob/f8add4e2a04b5e3e2aee05fcfe17771bf595051f/src/pages/workspace/WorkspaceInvitePage.tsx#L78-L83

and use:

    useEffect(() => {
        const unsubscribe = navigation.addListener('beforeRemove', () => {
            Member.setWorkspaceInviteMembersDraft(route.params.policyID, {});
        });

        return unsubscribe;
    }, [navigation, route.params.policyID]);

What alternative solutions did you explore? (Optional)

RachCHopkins commented 5 days ago

@alitoshmatov can you take a look at this proposal please?

alitoshmatov commented 5 days ago

Working on it

melvin-bot[bot] commented 5 days ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

alitoshmatov commented 4 days ago

@nyomanjyotisa Thank you for your proposal. I think your RCA is not correct, that piece of code was added to solve this exact issue

alitoshmatov commented 4 days ago

@Krishna2323 @truph01 Thank you for your proposals. If the root cause is component mounting and unmounting multiple times, shouldn't we fix this problem rather than changing cleanup function to accommodate this multiple mounting.

truph01 commented 4 days ago

shouldn't we fix this problem

Why did you think like that?

truph01 commented 4 days ago

rather than changing cleanup function to accommodate this multiple mounting.

My proposal does not do like that.

The current page's structure is <page><component /></page>. The bug occurs because <component /> is mounting and unmounting multiple times, and the cleanup function is inside <component />. My proposal is to move the cleanup function to <page /> instead of <component />, which should resolve the issue.

melvin-bot[bot] commented 8 hours ago

@RachCHopkins @alitoshmatov this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

Expensify / App