[$500] Workspaces - Unsynchronized transition of left and right page when deleting workspace

[izarutskaya]

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: 1.4.56-1 Reproducible in staging?: Y Reproducible in production?: N Logs: https://stackoverflow.com/c/expensify/questions/4856 Issue reported by: applause-Internal team

Action Performed:

1. Go to staging.new.expensify.com.
2. Go to Profile > Workspaces.
3. Go to any workspace.
4. Delete the workspace.

Expected Result:

Both left and right page will disappear at the same time.

Actual Result:

The left page disappears first, then followed by the right page.

Workaround:

Unknown

Platforms:

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

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

Screenshots/Videos

https://github.com/Expensify/App/assets/115492554/64bc7c7b-c327-43be-9a46-1b427f083333

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0173bde0810a3af269
• Upwork Job ID: 1772269977938833408
• Last Price Increase: 2024-04-01

Issue Owner

Current Issue Owner: @mallenexpensify

[melvin-bot[bot]]

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

[github-actions[bot]]

:wave: Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[melvin-bot[bot]]

Triggered auto assignment to @neil-marcellini (Engineering), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

[izarutskaya]

@mallenexpensify 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.

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

[izarutskaya]

Different behavior on production.

https://github.com/Expensify/App/assets/115492554/2276b714-9630-4d05-aa79-e3cc04708777

[ahmedGaber93]

It seems a regression from #38321, I think we need to back using goBack here https://github.com/Expensify/App/blob/3d780639eb260c1165e884032f4a92aa7dbbb4ca/src/libs/PolicyUtils.ts#L276

[neil-marcellini]

I think this behavior is actually better than what's on production, and it's not a big deal that one page disappears slightly before the other. I'm going to remove the blocker label and make this a low priority external issue.

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[allgandalf]

Proposal

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

LHP and Central Pane don't transition at the same time when deleting workspace

What is the root cause of that problem?

1. Currently when we delete any workspace, essentially we set action to pending delete: https://github.com/Expensify/App/blob/250baa9926cdf492785002100bad60455ccb567e/src/libs/actions/Policy.ts#L309

Now in workspace initial page, if we have the current action to pending delete we essentially hide the setting items using OfflineWithFeedback

2. As the Workspace Profile page is still in pending delete state, it transitions some time later to the workspace list page.

3. If we only pass shouldHideOnDelete={false} as a prop to OfflineWithFeedback then we will have regression if we delete workspace from another tab and the same profile page is open on another tab as mentioned by @tienifr . This is because we have not defined any way to reroute the user to the workspace list page.

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

Now, when we have the condition activeWorkspaceID === policy?.id true, we essentially call navigateWithSwitchPolicyID which calls switchPolicyID and passes the props policy and route, which essentially get passed and we navigate to the workspace list page by using the following logic: https://github.com/Expensify/App/blob/469b11237c3079232d9bbf65899c3a4caa387b26/src/libs/Navigation/switchPolicyID.ts#L130-L136

In case we don't pass route then it takes the default value:

https://github.com/Expensify/App/blob/469b11237c3079232d9bbf65899c3a4caa387b26/src/libs/Navigation/switchPolicyID.ts#L105-L106

Now, here we will update the code such that we will include the else block which switches the Workspace profile page to Workspace list page: https://github.com/Expensify/App/blob/469b11237c3079232d9bbf65899c3a4caa387b26/src/ROUTES.ts#L80

--- a/src/pages/workspace/WorkspaceProfilePage.tsx
+++ b/src/pages/workspace/WorkspaceProfilePage.tsx
@@ -104,6 +104,8 @@ function WorkspaceProfilePage({policy, currencyList = {}, route}: WorkSpaceProfi
         if (activeWorkspaceID === policy?.id) {
             setActiveWorkspaceID(undefined);
             Navigation.navigateWithSwitchPolicyID({policyID: undefined});
+        } else {
+            Navigation.navigateWithSwitchPolicyID({policyID: activeWorkspaceID, route: ROUTES.SETTINGS_WORKSPACES});
         }

This reroutes the user to the workspace list page.

This will make sure that we transition the LHN and Central Pane at the same time.

I guess this covers the scope of this bug as intented in the GH :)

Result Video:

[allgandalf]

Updated Proposal:

Updated proposal with detailed RCA and added result video, no change in main solution

[hayes102]

i am not sure but maybe it's a regression from this PR https://github.com/Expensify/App/pull/38365

[melvin-bot[bot]]

📣 @hayes102! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

1. Make sure you've read and understood the contributing guidelines.
2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[ZhenjaHorbach]

Proposal

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

Workspaces - Unsynchronized transition of left and right page when deleting workspace

What is the root cause of that problem?

The main problem with this issue is that when we delete the workspace

https://github.com/Expensify/App/blob/3d780639eb260c1165e884032f4a92aa7dbbb4ca/src/pages/workspace/WorkspaceProfilePage.tsx#L99

We set the pending action for WorkspaceInitialPage as DELETE

https://github.com/Expensify/App/blob/250baa9926cdf492785002100bad60455ccb567e/src/libs/actions/Policy.ts#L309

And by default when we have pending action as DELETE we hide children components for OfflineWithFeedback

https://github.com/Expensify/App/blob/59f0b06534a0f537593a3c852d00e982345d0005/src/pages/workspace/WorkspaceInitialPage.tsx#L267-L272

https://github.com/Expensify/App/blob/c80ab583cc52e8ddec60bbea75893226647468d7/src/components/OfflineWithFeedback.tsx#L101

And since for navigation we need more time, we have empty space

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

To fix this issue we can pass shouldHideOnDelete={false} for OfflineWithFeedback to not hide content

https://github.com/Expensify/App/blob/59f0b06534a0f537593a3c852d00e982345d0005/src/pages/workspace/WorkspaceInitialPage.tsx#L267-L272

This should not cause regression because in the absence of workspace data or when we finally deleted workspace We will show FullPageNotFound

https://github.com/Expensify/App/blob/59f0b06534a0f537593a3c852d00e982345d0005/src/pages/workspace/WorkspaceInitialPage.tsx#L230-L233

https://github.com/Expensify/App/blob/59f0b06534a0f537593a3c852d00e982345d0005/src/pages/workspace/WorkspaceInitialPage.tsx#L253-L258

RESULT

https://github.com/Expensify/App/assets/68128028/eb8e0615-7350-4044-8e02-ca9c5a0e8603

Additional:

I notice when we have an active workspace and delete workspace

We immediately go to another screen without waiting for the workspace to be deleted (because inside navigateWithSwitchPolicyID we use root.dispatch which immediately changes the navigation state )

As a result, we will see the deleted workspace in the list for a short period of time

https://github.com/Expensify/App/blob/45c79232471ae1248a14ac4c3c7cd9aad016f41b/src/pages/workspace/WorkspaceProfilePage.tsx#L105-L108

To fix this We can update code like

        if (activeWorkspaceID === policy?.id) {
            setActiveWorkspaceID(undefined);
            InteractionManager.runAfterInteractions(() => {
                Navigation.navigateWithSwitchPolicyID({policyID: undefined});
            });
        }

What alternative solutions did you explore? (Optional)

As alternative we can set: shouldHideOnDelete={PolicyUtils.isPendingDeletePolicy(policy) && PolicyUtils.isPendingDeletePolicy(prevPolicy)}

This means that if during the last render, we received pending action equal to Delete (pending action from policy is DELETE, pending action from prevPolicy is not DELETE ), we won't hide WorkspaceInitialPage

[cubuspl42]

@GandalfGwaihir

I don't get it.

// If the workspace being deleted is the active workspace, switch to the "All Workspaces" view 

As this comment states, this is intended to be executed only if the workspace being deleted is the active workspace. Have you tested your code when, for example, workspace A is active, and you delete the workspace B?

[allgandalf]

As this comment states, this is intended to be executed only if the workspace being deleted is the active workspace

I checked the PR which added this code, this was acutally more appropriate for the WorkspaceListPage but if we delete from WorkspaceProfilePage, we essentially set the activeID to undefined/null as we have deleted the workspace from it's settings page, so that condition will never be true in profiles page.

Have you tested your code when, for example, workspace A is active, and you delete the workspace B?

Yes, here is the test with my suggested code:

https://github.com/Expensify/App/assets/110545952/18a38e67-8de3-4b72-bc76-5d8264e830dd

[ghost]

Proposal

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

Workspaces - Unsynchronized transition of left and right page when deleting workspace

What is the root cause of that problem?

The main root cause here is, here : https://github.com/Expensify/App/blob/d8361eee3486ccb5472060f2af7655e96461471e/src/pages/workspace/WorkspaceProfilePage.tsx#L94-L108 inside, the goBackFromInvalidPolicy() function, we have defined it like this : https://github.com/Expensify/App/blob/d8361eee3486ccb5472060f2af7655e96461471e/src/libs/PolicyUtils.ts#L275-L277 which is causing delay in left and right page when deleting a workspace.

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

We need to change it from Navigation.navigate(ROUTES.SETTINGS_WORKSPACES); to Navigation.goBack(ROUTES.SETTINGS_WORKSPACES); from both the pages going away simulatenously.

What alternative solutions did you explore? (Optional)

N/A

RESULT

https://github.com/Expensify/App/assets/158435970/1f66c946-8554-4082-897e-cfa5e179b13b

[ghost]

updated proposal with working video. No other changes done

[cubuspl42]

@GandalfGwaihir

Yes, here is the test with my suggested code:

No need for two windows. I just tested this by returning to the "Chats" view, and the behavior seemed to be a regression.

[cubuspl42]

@godofoutcasts94

Sounds good. Possibly we'll go with this one.

[ZhenjaHorbach]

@cubuspl42 What do you think about the other proposals?

I don't think that changing Navigation.navigate(ROUTES.SETTINGS_WORKSPACES) to Navigation.goBack(ROUTES.SETTINGS_WORKSPACES) is the best option

Because we will have regression That the workspace will not have enough time to finally delete

As a result, we will see the deleted workspace in the list for a short period of time

https://github.com/Expensify/App/assets/68128028/b9790e4a-9d72-4f82-b880-a563d122324e

[cubuspl42]

@ZhenjaHorbach

Sorry for not commenting on your proposal.

Would you also provide a video showing how your proposed solution works, including the path that leads to FullPageNotFound (I'm not sure if I understand this yet)?

[ZhenjaHorbach]

@ZhenjaHorbach

Sorry for not commenting on your proposal.

Would you also provide a video showing how your proposed solution works, including the path that leads to FullPageNotFound (I'm not sure if I understand this yet)?

I added a video in the proposition)

And I mentioned FullPageNotFound for extra information Why I don't see anything wrong, set the value of shouldHideOnDelete to false

Because after we finally deleted the workspace We will see only FullPageNotFound

https://github.com/Expensify/App/assets/68128028/8a1da70f-3d85-41ab-b665-d96332300de5

[ghost]

@godofoutcasts94

Sounds good. Possibly we'll go with this one.

Will I get assigned on it @cubuspl42 ? Just asking

[cubuspl42]

Come on, I said "possibly". I gave my feedback and shared my thoughts at the time of writing.

Since then, there were some valid concerns raised about your proposal in its current state.

[ghost]

Come on, I said "possibly". I gave my feedback and shared my thoughts at the time of writing.

Since then, there were some valid concerns raised about your proposal in its current state.

Oh sorry I didn't follow the entire thread

[tienifr]

Proposal

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

The left page disappears first, then followed by the right page.

What is the root cause of that problem?

1. When we delete the workspace, the workspace initial page's menu items will disappear immediately due to the OfflineWithFeedback here, the default behavior of OfflineWithFeedback is to hide the content if it's pending delete
2. When opening the workspace profile page in 2 tabs, then delete the workspace in 1 tab, when navigating to the other tab, the workspace there is still present (if offline mode), or still present for a few seconds then becomes a not found page (in online mode)

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

1. Pass shouldHideOnDelete={false} here so that the content of OfflineWithFeedback will not disappear when the workspace is pending deletion
2. After the fix above, there'll be a regression described in RCA -> 2 above, the workspace initial page will now be present for a few seconds even though we already deleted the workspace in the other tab.

This is because currently we don't have any logic to navigate the user to the workspace list page after the workspace has been deleted from another tab, we're only navigating at the same tab where the deletion takes place.

To fix this, we should add an useEffect here to check if the workspace was present, but becomes pending delete, then we'll navigate to workspace list

useEffect(() => {
      if (!isEmptyObject(prevPolicy) && !PolicyUtils.isPendingDeletePolicy(prevPolicy) && PolicyUtils.isPendingDeletePolicy(policy)) {
          Navigation.navigateWithSwitchPolicyID({policyID: undefined});
      }
  }, [policy, prevPolicy]);

(This is the same navigation logic as when workspace is deleted here)

3. Optionally, I also see an issue where in offline mode, the workspace initial page will briefly have strike-through styles in all the menu items when the workspace deleted. This is not intended because the user will be navigated back to workspace list immediately after they delete the workspace, in both online and offline mode, so no strike-through flickering should happen in that short timeframe, if we want to polish this, we can add shouldDisableStrikeThrough here

What alternative solutions did you explore? (Optional)

NA

[allgandalf]

Updated proposal:

@cubuspl42 I have updated my proposal to cover the regression which you pointed out, it should be fixed how, can you please review it again

[cubuspl42]

@GandalfGwaihir

The policy is already set as invalid, hence we never reach that state

Do I understand correctly that you're suggesting that the condition of this if statement...

if (activeWorkspaceID === policy?.id) {
    // ...
}

...is never true in practice? Making its body dead code?

[allgandalf]

Actually i misunderstood at first, now that i have a look that condition is meant to be executed when the below happens as you pointed out:

Have you tested your code when, for example, workspace A is active, and you delete the workspace B?

So the condition gets activated when the active workspace is the one which is deleted