App: [HOLD for payment 2024-04-05] [$100] Remove the closeFullScreen() method from code since it is no longer used

@kosmydel would you be able to provide more insights on this commit: 404b0b6

My understanding is that StackActions.popToTop goes back to the first screen in the stack, while StackActions.pop goes back to the previous one, which is what we want here.

The StackActions.pop function goes back to the previous screen. The problem with it is, that it is noticeably slow. Switching to popToTop helped, but apparently, it introduced this bug.

We can consider switching back to the .pop function, however, this will result in a slower performance (and that is probably not what we want). On the other hand, pressing the browser back button is also slow (the performance is similar to what we have with the .pop function).

So the aim is, to find a solution, which will be both performant and not use the popToTop function.

Let’s hire @barros001 because they were first to mention the root cause? https://github.com/Expensify/App/issues/36509#issuecomment-1961909146

@tgolen 🎀👀🎀

looks like it worked this time!

@adelekennedy offer accepted

EDIT: Spoke too soon, looks like Upwork having issues and I can’t accept it… I’ll try again shortly.

That’s a good point. It’s a shame that closeFullScreen has been abandoned. It should be cleaned up now. I’m going to reduce the bounty of this issue to $100 and change the scope to just be a simple cleanup.

@tgolen I think we should measure the delay of StackActions.pop vs StackActions. popToTop.

If the delay is measureable and user can feel the lag, then we could go with @bi-kash’s proposal because I can’t find this issue happening anywhere else in the app.

No worries, it’s all good 😃

OK, I read through most of this issue and I hope I caught everything. I understand the focus on pop and popToTop but I am concerned that it could have far-reaching effects across the app. Are we sure we want to affect the entire app?

If so, then I think this problem is not really related to the workspace selector only, and that’s just one way the problem is manifesting. I agree with Rushat’s plan to fix that.

If we don’t want to affect the entire app, then @bi-kash’s proposal is the correct one I think.

Proposal

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

Clicking the back button in profile settings will not take you back to the same conversation.

What is the root cause of that problem?

The root cause of this problem is this commit: https://github.com/Expensify/App/commit/404b0b65ec58bbe3b072d366d78f266f8de832bd

It replaced pop with popToTop which makes the navigator navigate back to the first screen and not the previous one.

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

Reverting https://github.com/Expensify/App/commit/404b0b65ec58bbe3b072d366d78f266f8de832bd is not really an option here because the change was made to solve another problem. We can also see by this comment that pop is the right method to call here but it does cause delays, which we also don’t want.

I also found some interesting things when using pop (this only applies to Web). For example, when we use pop, we the browser’s forward navigation will activate, which means the pop behaves similarly to the browser’s back button. If you go to settings, then navigate to Wallet, then Preferences, then Security and then click the back button, you’d be able to hit forward and backtrack these steps (although it doesn’t appear to always be that way). With popToTop, nont of this happens. It behaves as we would expect, it pushes a new url to the history and you can hit back on the browser’s button to go back to the settings back, etc… Except that it does not work 😃

Going back to the OG problem, where pop adds a delay, I found this commit that’s interesting:

https://github.com/react-navigation/react-navigation/commit/b1f13774295465942aafa1b0ff611b9eebccbd77

Looks like react-navigation adds some artificial delay when navigating under certain codnitions. I didn’t dig too deep but this could be the reason why we see a slight delay when using pop.

So, popToTop is not the correct method, it doesn’t do what we need, and while pop does, it has a delay and on web it acts as clicking the browser’s back button which IMO is wrong. Clicking the < button on the app shouldn’t bring us “back”, we’re moving “forward” to the previous url, if that makes sense.

My proposed solution for this problem is to rewrite closeFullScreen to be like this:

function closeFullScreen() {
    const rootState = navigationRef.getRootState();

    if(rootState.routes.length < 2) {
        navigationRef.dispatch({...StackActions.popToTop(), target: rootState.key});
    }

    const parentRoute = rootState.routes[rootState.routes.length - 2];
    navigationRef.dispatch({...StackActions.push(parentRoute.name, parentRoute.params), target: rootState.key});
}

This will use the push method to navigate us forward to the previous screen, but grabbing the previous screen in the stack and navigating to it. This will make the app navigate to the correct page, without the delay and as a bonus will not break the browser’s back/forward navigation.

The check for length < 2 was added as a failsafe but I believe that scenario won’t happen, especially because closeFullScreen is only used in one place throughout the entire codebase.

EDIT: This doesn’t really work well on mobile because hitting back is “navigating forward” and the navigation animation is not correct. Also, I believe using push adds another stack on top and I don’t think this is a good thing as we want to remove the top screen out of the stack instead of adding another screen on top of it. I’ll leave this proposal up as there might still be some good info in here.

What alternative solutions did you explore? (Optional)

Proceed with reverting https://github.com/Expensify/App/commit/404b0b65ec58bbe3b072d366d78f266f8de832bd and dealing with the delay issue separately. The original fix for the delay issue was wrong and should be reverted and a new fix for the delay should be created.

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

Proposal

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

The current issue pertains to the functionality of the back button in profile settings, which erroneously navigates to the conversation before workspace switching.

What is the root cause of that problem?

When the user selects a workspace from the workspace switcher, The code first executes Navigation.goBack(), navigating to the previous screen in the stack. And only after that it checks if it has to switch to new screen.

Navigation.goBack();
if (policyID !== activeWorkspaceID) {
    Navigation.navigateWithSwitchPolicyID({policyID, isPolicyAdmin});
}

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

Instead of unconditionally navigating back to the previous screen, We only navigate to the previous screen if and only if the user selects the same or active workspace. Otherwise, it directly navigates to the new workspace.

if (policyID !== activeWorkspaceID) {
    Navigation.navigateWithSwitchPolicyID({policyID, isPolicyAdmin});
} else {
    Navigation.goBack();
}

Here If a new workspace was selected, the first screen of that new workspace will be the “top” of the stack, since we’re not navigating back before switching workspace.

Updated Result:

https://github.com/Expensify/App/assets/32809050/2780ecfe-b3d9-4035-a479-8ccc9ee34754

What alternative solutions did you explore? (Optional)

We can simply use .pop() instead of .popToTop(). However, it is too slow. So above solution is fast and robust and solves the problem from the root.