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

[kbecciv]

The back button was removed from the modals and so now the closeFullScreen() method is no longer used in the code, but it was never removed. Let's remove it!

Old Issue Description

If you haven’t already, check out our [contributing guidelines](https://github.com/Expensify/ReactNativeChat/blob/main/contributingGuides/CONTRIBUTING.md) for onboarding and email contributors@expensify.com to request to join our Slack channel! ___ **Version Number:** v1.4.41-3 **Reproducible in staging?:** y **Reproducible in production?:** y **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:** Applause - Internal **Slack conversation:** ## Action Performed: Precondition: - Workspace filter is set to "Expensify". 1. Go to staging.new.expensify.com 2. Go to any chat and send a message. 3. Open workspace switcher. 4. Select any workspace. 5. Click Profile settings. 6. Click back button on the top left. ## Expected Result: App closes profile settings and reopens the chat that shows up after switching workspace in Step 4. ## Actual Result: App reopens the chat that is opened in Step 2, which is before workspace filter is applied, while the workspace filter is still applied. ## 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 Add any screenshot/video evidence https://github.com/Expensify/App/assets/93399543/32828cc3-8d1c-4347-a25d-f7b624a03ccc

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0124a682d645bc91de
• Upwork Job ID: 1757785433393643520
• Last Price Increase: 2024-03-15

Issue Owner

Current Issue Owner: @adelekennedy

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[kbecciv]

We think that this bug might be related #wave8-collect-admins CC @zanyrenney

[barros001]

This seems to be a regression caused by https://github.com/expensify/App/commit/404b0b65ec58bbe3b072d366d78f266f8de832bd which is part of a large PR. Reverting this commit solves the problem but I'm sure it will break whatever that commit fixed (I could not find any references to it), so I'm not gonna make a formal proposal yet.

[rushatgabhane]

Let's fix the issue then instead of reverting

[melvin-bot[bot]]

@rushatgabhane, @adelekennedy Whoops! This issue is 2 days overdue. Let's get this updated quick!

[rushatgabhane]

looking for proposals

[barros001]

@kosmydel would you be able to provide more insights on this commit: https://github.com/Expensify/App/commit/404b0b65ec58bbe3b072d366d78f266f8de832bd

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.

[melvin-bot[bot]]

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

[bi-kash]

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.

[melvin-bot[bot]]

@rushatgabhane, @adelekennedy Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[adelekennedy]

waiting for additional proposals

[barros001]

[deleted]

[barros001]

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.

[brandonhenry]

Proposal

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

Clicking the back button in profile settings navigates to the first conversation instead of the previous conversation.

What is the root cause of that problem?

The root cause of this problem is the use of the popToTop method here in closeFullScreen, which navigates back to the first screen in the stack, instead of navigating to the previous screen.

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

To solve this problem, we should replace the use of popToTop with the navigate action. The navigate action allows us to navigate to a specific route, and if that route already exists in the stack, it will go back to that screen and remove any screens after that. This behavior aligns with our desired outcome of navigating back to the previous conversation and keeps the visuals on mobile in order as well as the stack order.

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

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

In this implementation, we first check if there are at least two routes in the stack. If there are fewer than two routes, we use popToTop to navigate to the first screen. Otherwise, we use the navigate action to go back to the previous route.

What alternative solutions did you explore? (Optional)

• Get rid of the closeFullScreen function and just use the goBack method from this same file.

https://github.com/Expensify/App/blob/e9fb49d9f41972a04bf07905b228314a3fdce8d4/src/libs/Navigation/Navigation.ts#L185

[garatkarr]

Proposal

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

After navigation around various chats and workspaces and then the user brings up the Profile page, when user click the Left Chevron at the top left it does not go back to the last chat page viewed. The user expectation is it should go back to the chat page immediately before opening the Profile page.

What is the root cause of that problem?

The root cause of the problem is the wrong action is called when the Left Chevron is clicked. StackActions.popToTop() is the wrong action introduced in 404b0b6

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

We should revert 404b0b6 to restore correct functionality of the Left Chevron.

404b0b6 was supposed to fix "Clicking back from the Account Settings back in top left corner takes noticeably longer to open the chats then other back buttons" but I have not been able to reproduce this slower performance on Web. We should fix this issue (#36509) now because it's broken functionality which is more important than performance. Separately, another issue should be opened to properly investigate and address the slower performance. It's possible this performance issue no longer exists.

What alternative solutions did you explore? (Optional)

[kosmydel]

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

[adelekennedy]

Proposals to review above - we're just coming back from the weekend

[melvin-bot[bot]]

@rushatgabhane @adelekennedy 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!

[rushatgabhane]

on my plate

[melvin-bot[bot]]

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

[bi-kash]

Proposal

Updated

[rushatgabhane]

~I like @bi-kash's proposal https://github.com/Expensify/App/issues/36509#issuecomment-1959508184~

~🎀 👀 🎀~

[melvin-bot[bot]]

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

[rushatgabhane]

@tgolen the root cause is using StackActions.popToTop because it takes you to first screen in stack. https://github.com/Expensify/App/issues/36509#issuecomment-1964380991

I need your feedback on next steps.

I think the solution should be to

1. use StackActions.pop
2. measure the delay
3. find root cause of the delay
4. fix the delay

A stack pop action should be O(1), we should investigate if it's an issue in react navigation What do you think?

[rushatgabhane]

@barros001 the commit you're referring to in react navigation is 4 years old. I can't find it in main branch. Maybe they removed the delay?

[tgolen]

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.

[barros001]

@barros001 the commit you're referring to in react navigation is 4 years old. I can't find it in main branch. Maybe they removed the delay?

Very possible. My bad though, I should've looked deeper before mentioning a 4 year old commit.

[rushatgabhane]

No worries, it's all good :)

[melvin-bot[bot]]

@tgolen, @rushatgabhane, @adelekennedy Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[tgolen]

@rushatgabhane How do you want to move forward on this one? Is the root problem happening elsewhere in the app and this should be a holistic solution for the entire app? OR does this problem only affect this single flow and a more simple fix is in order?

[mvtglobally]

Issue not reproducible during KI retests. (First week)

[rushatgabhane]

Yeah sure! Let me deep dive what to do here

[melvin-bot[bot]]

@tgolen @rushatgabhane @adelekennedy this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

[rushatgabhane]

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

[rushatgabhane]

If there are no posts, I'll find a way to measure the delay over the weekend.

[s77rt]

Besides the delay issue there is also this issue https://github.com/Expensify/App/issues/36900

[melvin-bot[bot]]

@tgolen, @rushatgabhane, @adelekennedy Whoops! This issue is 2 days overdue. Let's get this updated quick!

[adelekennedy]

clearing the overdue here - @rushatgabhane were you able to measure the delay over the weekend?

[adelekennedy]

I've added this to wave collect as I think it belongs there and under the Release 1 status

[melvin-bot[bot]]

@tgolen @rushatgabhane @adelekennedy this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:

• Decide whether any proposals currently meet our guidelines and can be approved as-is today
• If no proposals meet that standard, please take this issue internal and treat it as one of your highest priorities
• If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

[melvin-bot[bot]]

Current assignee @rushatgabhane is eligible for the Internal assigner, not assigning anyone new.

[rushatgabhane]

Hey, now that we've removed back button from the full screen modal, do we really need to fix this bug? It can't be reproduced after click browser back. The function closeFullScreen where the root cause is isn't being used.

search gh

[tgolen]

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.

[melvin-bot[bot]]

Upwork job price has been updated to $100

[adelekennedy]

@trjExpensify with the change above should this be under polish vs release 1?

[rushatgabhane]

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

@tgolen 🎀👀🎀

[melvin-bot[bot]]

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

[trjExpensify]

yep, polish makes sense.