[$1000] Dev: Navigation to workspace from room leads to a different page while returning back

[kbecciv]

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

---

Action Performed:

1. Go to '+' -> new room
2. add name -> select workspace ( lets say workspace A) here -> pick visibility
3. now on the room 's page , go to header and click on #rooms' name. Now it shows a details page on RHN
4. Now hover to workspace name and click, It will show Workspace page
5. IMPORTANT STEP, now click on settings, on settings page, hit back.
6. Now again click back from the workspace page.
7. Notice that now it shows the list of workspaces.
8. Now on clicking consecutive back, it leads to settings page of profile. i.e we can't back navigate the RHN room's details page.

Expected Result:

On clicking consecutive back, it should lead to details page of room i.e we can't back navigate the RHN room's details page.

Actual Result:

Navigation to workspace from room leads to a different page while returning back

Workaround:

Unknown

Platforms:

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

• [ ] Android / native
• [ ] Android / Chrome
• [ ] iOS / native
• [ ] iOS / Safari
• [x] MacOS / Chrome / Safari
• [ ] MacOS / Desktop

Version Number: DEV(v1.3.32-5) and staging(v1.3.33-3) Reproducible in staging?: n/a Reproducible in production?: n/a 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 Notes/Photos/Videos: Any additional supporting documentation

https://github.com/Expensify/App/assets/93399543/4cb66664-96ef-4fc3-a014-6ae0e0e3fcd1

Expensify/Expensify Issue URL: Issue reported by: @ashimsharma10 Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1687880333737179

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0130170e7fe486522b
• Upwork Job ID: 1676172619682504704
• Last Price Increase: 2023-07-25

[melvin-bot[bot]]

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

[melvin-bot[bot]]

Bug0 Triage Checklist (Main S/O)

• [ ] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• [ ] This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• [ ] This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• [ ] This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• [ ] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[ashimsharma10]

Dear all,

In order to reproduce, Dont miss step 4 ( from actions performed above) . Step 4 & 5 are important !!

[bernhardoj]

Proposal

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

To make the explanation easier, let's separate it into 2 cases:

1. If we navigate from room details > workspace page, then go back, it will bring us back to room details.
2. If we navigate from room details > workspace page > any workspace sub-page, then go back, it will bring us back to workspace list page instead of room details.

The 2nd case is the issue.

What is the root cause of that problem?

When we press the back button on workspace page, it will call Navigation.goBack with ROUTES.SETTINGS_WORKSPACES as the fallback route. ROUTES.SETTINGS_WORKSPACES is the workspace list page. In the 2nd case, the fallback route is used on going back. How can the behavior is different? To find the answer, we must first understand when is exactly the fallback route will be used.

The answer is when the current route index is 0. Route index tells which route in the stack is currently active. https://github.com/Expensify/App/blob/485d68ae5826c19feaf1003f5274c848d2214f90/src/libs/Navigation/Navigation.js#L99-L102

To get the route index, we need to get it from the navigation state. It will recursively access the child route and look up their index. If the route doesn't have an index, it will default to 0. If the route doesn't have any more children, it will return the parent index. https://github.com/Expensify/App/blob/485d68ae5826c19feaf1003f5274c848d2214f90/src/libs/Navigation/Navigation.js#L45-L61

Below is the minified version of the navigation state (when pressing the back button) for both cases. (navigation state could be different depending on what page you have visited) We will use this as an example.

Note: ReportDetails is the room details page and CentralPaneNavigator is the room report page 1st

index: 2,
routes: [
    Home,
    CentralPaneNavigator,
    {
        name: RightModalNavigator,
        state: {
            index: 1,
            routes: [
                ReportDetails,
                {name: Settings, params: {screen: WorkspacePage}}
            ]
        }
    }
]

In the first case, the root index is 2, which means the RightModalNavigator route is the page that we see on the screen. RightModalNavigator is a navigator, so we could have several screens or even another navigator inside.

In this first case, we have ReportDetails screen and Settings navigator and the index (1) tells us that Settings is the one that we see on the screen. Settings is a navigator, but it doesn't have a state property here, so getActiveRouteIndex assume Settings does not have a child screen, but it actually has it, which is WorkspacePage, so getActiveRouteIndex returns the parent index which is 1. Because it's not 0, the fallback route won't be used.

I don't have an explanation for why it doesn't have the state property, but from my observation, this is the behavior of visiting a navigator for the first time. More explanation is on the 2nd case.

2nd

index: 2,
routes: [
    Home,
    CentralPaneNavigator,
    {
        name: RightModalNavigator,
        state: {
            index: 1,
            routes: [
                ReportDetails,
                {
                    name: Settings,
                    state: {index: 0, routes: [WorkspacePage]}
                }
            ]
        }
    }
]

The difference in the 2nd case is, we have a state property now for Settings navigator. The state property started to appear after we navigate to the workspace sub-page. Continuing from the above assumption, I'm guessing the state property will be added once the navigator has at least once more than 1 screen. So, in the 2nd case, 0 is returned as the route index. Because it's 0, the fallback route will be used. This is the issue.

As far as I understand, the idea of a fallback route is if the current screen is the only one in the RightModalNavigator stack/routes, then navigate to the fallback route. This is useful for users when they deep link to a deeper page, for example, workspace setting.

The problem is, getActiveRouteIndex keeps recursively accessing the child route and, eventually accessing the wrong navigator. In this case, we get the index of the Settings navigator (root -> RightModalNavigator -> Settings). Because Settings is a navigator, it has its own index, and when the index is 0, the logic thought that Settings is the only screen/navigator in RightModalNavigator, but in fact, there is a ReportDetails screen in the stack/routes.

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

We should only get the index of RightModalNavigator, not its children's index. To fix it, I suggest to:

1. Rename getActiveRouteIndex to getActiveRHNIndex.
2. RightModalNavigator is placed in the Root navigator, this means we don't need to do it recursively. We just need to access the routes stack with the root index. If it's RightModalNavigator, then return its index. Else, return -1, indicating that RightModalNavigator is not found.

if (route.routes) {
    const childActiveRoute = route.routes[route.index || 0];
    if (childActiveRoute.name === NAVIGATORS.RIGHT_MODAL_NAVIGATOR) {
        return lodashGet(childActiveRoute, 'state.index', 0);
    }
}

return -1;

I also suggest to have SWM team to review this as this is related to navigation

[melvin-bot[bot]]

Looks like something related to react-navigation may have been mentioned in this issue discussion.

As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js files should not be accepted.

Feel free to drop a note in #expensify-open-source with any questions.

[StevenKKC]

Proposal

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

Clicking back button after clicking on menu gets back to list of workspace on admins channel.

What is the root cause of that problem?

If the user clicks on the workspace name on the report detail page, the navigation will create a new item in the RightModalNavigator because Report_Details_Root and Workspace_Initial are different screens in linkingConfig. Therefore, there are two items in the RightModalNavigator.

When user click menu, the menu item and workspace initial page are on the same screen in linkingConfig, new item will be added to Workspace_Initial.

When the user clicks the back button, navigation will iterate Workspace_Settings screen routes and return 0. https://github.com/Expensify/App/blob/b0add9454e4e1a3fcc94acdbf838040c4c15acbe/src/libs/Navigation/Navigation.js#L45-L61 And since fallbackRoute is set, fallbackRoute will be navigated accordingly. https://github.com/Expensify/App/blob/b0add9454e4e1a3fcc94acdbf838040c4c15acbe/src/libs/Navigation/Navigation.js#L99-L102

fallbackRoute is set to ROUTES.SETTINGS_WORKSPACES, so we'll be returned to the list of workspaces. https://github.com/Expensify/App/blob/b0add9454e4e1a3fcc94acdbf838040c4c15acbe/src/pages/workspace/WorkspaceInitialPage.js#L197-L197

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

We can modify this line as below.

-   if (shouldEnforceFallback || (!getActiveRouteIndex(navigationRef.current.getState()) && fallbackRoute)) {
+   if (shouldEnforceFallback || (getActiveRouteIndex(navigationRef.current.getState()) == null && fallbackRoute)) {

What alternative solutions did you explore? (Optional)

We can remove fallbackRoute in WorkspaceInitialPage.js.

-   onBackButtonPress={() => Navigation.goBack(ROUTES.SETTINGS_WORKSPACES)}
+   onBackButtonPress={() => Navigation.goBack()}

• I have posted my proposal in here.

[laurenreidexpensify]

Not overdue, reveiwing

[melvin-bot[bot]]

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

[melvin-bot[bot]]

Current assignee @laurenreidexpensify is eligible for the External assigner, not assigning anyone new.

[melvin-bot[bot]]

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

[melvin-bot[bot]]

@laurenreidexpensify, @aimane-chnaif Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[samh-nl]

Proposal

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

When navigating to a workspace via a room, clicking back leads to a different page.

What is the root cause of that problem?

Navigation.goBack() uses the fallback route (workspaces list) inappropriately, caused by an incorrect traversal of the history.

The fallback route is used when getActiveRouteIndex returns 0. This index reflects the depth of the history. A value of zero, therefore means that the beginning has been reached and we are unable to determine the prior route, thus we use the fallback.

https://github.com/Expensify/App/blob/60f48175c9e3342ef192d878218820e5ed5a76cd/src/libs/Navigation/Navigation.js#L117-L120

When the route changes from /r/(id)/details to /workspace/(id), the index is determined relative to the current stack navigation which is the workspace route, but the workspace route should not be used as reference here.

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

The index merely serves as a proxy to see whether we have reached the "end of the line". What we are truly interested in is whether the RHN contains no history stack. The complex logic in getActiveRouteIndex can be replaced with a new function named shouldUseFallback.

function shouldUseFallback() {
    const { routes, index } = navigationRef.current.getState();
    const { name, state } = routes[index];
    return name === NAVIGATORS.RIGHT_MODAL_NAVIGATOR && !Array.isArray(state?.routes);
}

This can then be used in the if-statement in Navigation.goBack:

if ((shouldEnforceFallback || shouldUseFallback()) && fallbackRoute) {

Also note the incorrect parenthesis in the existing code. fallbackRoute must be set also if shouldEnforceFallback is true.

Result

https://github.com/Expensify/App/assets/137707942/f37cfb85-d942-49c1-992a-db0922a544a2

What alternative solutions did you explore? (Optional)

We could use the old approach (relying on index) but fix it by recursively incrementing the index to accurately reflect the depth of the history. However, the added complexity is unnecessary. Given that the index is not actually used except for determining whether we should use the fallback route, the proposal above is a far more elegant approach.

[samh-nl]

@laurenreidexpensify @aimane-chnaif Hi both, considering it was mentioned that the review is taking place, please ensure my proposal is evaluated also. Thank you.

[b4s36t4]

Proposal

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

Dev: Navigation to workspace from room leads to a different page while returning back

What is the root cause of that problem?

RCA for this issue is navigating from one stack to another stack. As seen in the video, when we click on workspace we're navigating to the user to Workspace_Initial which lives in a different stack navigator SettingsModalStackNavigator than one of the screen from ReportDetailsModalStackNavigator.

When we do this and when we try to come back we do calculate the ActiveRouteIndex https://github.com/Expensify/App/blob/60f48175c9e3342ef192d878218820e5ed5a76cd/src/libs/Navigation/Navigation.js#L54 here. If it's 0 means we reached the bottom of the navigation.

The logic is straight forward and working good unless if the route is from another stackNavigator it does create problems and because of activeIndex is 0 we're going to the fallbackRoute provided into the screen logic which shouldn't happen because the Navigation still holds some screens to goback.

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

While calculating the activeIndex from https://github.com/Expensify/App/blob/60f48175c9e3342ef192d878218820e5ed5a76cd/src/libs/Navigation/Navigation.js#L54 this function we should return other 0 if it's a Stack instead of a screen.

const getActiveRouteIndex = function (route, index) {
          // Existing code
          if (route.state && route.state.routes) {
               if(route.state.key.includes("stack"){ // Meaning the screen we're seeing is a stackNavigator
                      return 1; // Do the screen pop (going back)
               }
               const childActiveRoute = route.state.routes[route.state.index || 0];
               return getActiveRouteIndex(childActiveRoute, route.state.index || 0);
          }

          // Existing Code
}

This solution is also working with #22537 issue too.

What alternative solutions did you explore? (Optional)

NA

[alitoshmatov]

Duplicate of https://github.com/Expensify/App/issues/20164

[bernhardoj]

I don't think we should close this one. It was previously said that it will be solved in a navigation reboot, but the fact is this is still happening after the navigation reboot (the root cause is also completely different).

cc: @laurenreidexpensify

[aimane-chnaif]

Let's re-open this as https://github.com/Expensify/App/issues/20164 is closed before making external. To be fair, I am still considering proposals made in that issue.

[bernhardoj]

Bump @laurenreidexpensify

[aimane-chnaif]

@bernhardoj I am now not able to reproduce on any env. Can you test again?

[bernhardoj]

@aimane-chnaif Still reproducible on the latest main

https://github.com/Expensify/App/assets/50919443/3030a462-a4bf-48e6-9ee1-bf7611aeee43

[aimane-chnaif]

@bernhardoj what's the bug exactly?

Now on clicking consecutive back, it leads to settings page of profile. i.e we can't back navigate the RHN room's details page.

You expect to go to room detail page directly instead of settings page in the middle?

[bernhardoj]

Oh, I just realized that previously we were completely unable to go back to the room details and now we can, but yes I expect it to directly go to the room detail page.

If we just open the workspace page and not go deeper into its subpages, the issue above won't happen.

[ashimsharma10]

Updates on this?

[maddylewis]

@laurenreidexpensify - just checking in to make sure you meant to close this one. there are some recent comments from contributors so i reopened to double-check!

[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? 💸

[laurenreidexpensify]

Sorry I completely missed the notifications here, I closed this in reaction to the dupe comment here, but looks like that was a false positive. Thanks for reopening @maddylewis

[laurenreidexpensify]

@aimane-chnaif sorry for the delays here - what are you thinking for next steps here?

[melvin-bot[bot]]

@laurenreidexpensify @aimane-chnaif 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 @aimane-chnaif is eligible for the Internal assigner, not assigning anyone new.

[laurenreidexpensify]

@aimane-chnaif bump for the above ^^

[aimane-chnaif]

You expect to go to room detail page directly instead of settings page in the middle?

@laurenreidexpensify do you agree with the expected behavior?

[melvin-bot[bot]]

@laurenreidexpensify, @aimane-chnaif Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[laurenreidexpensify]

@aimane-chnaif sorry for delay, but I agree with @bernhardoj yes that that is expected behaviour, but am going to tag a few others to confirm:

@zanyrenney @dylanexpensify @twisterdotcom do you agree?

[laurenreidexpensify]

@zanyrenney @dylanexpensify @twisterdotcom can one of you buddy check and confirm this? Thanks!

[dylanexpensify]

Yes, will do now @laurenreidexpensify !

[dylanexpensify]

Oh, I just realized that previously we were completely unable to go back to the room details and now we can, but yes I expect it to directly go to the room detail page. If we just open the workspace page and not go deeper into its subpages, the issue above won't happen.

Basically, we're missing this step, right? It's going back to all workspaces, not the "newnew" one created in the OP, right? If so, then I agree with the approach from @bernhardoj cc @aimane-chnaif @laurenreidexpensify

[aimane-chnaif]

As suggested by @bernhardoj, I'd like to get 2nd opinion from @adamgrzybowski or @wojtus7 on this proposal along with discussions so far.

[adamgrzybowski]

I can't create a room but I am assuming that workspaces have a similar flow. Is the recorded behavior as expected?

https://github.com/Expensify/App/assets/67908363/e04de053-1d63-4918-a4f0-810a5e5bf5a4

[adamgrzybowski]

In theory this should be fixed by this PR https://github.com/Expensify/App/pull/23007. Could you please test this again?

[laurenreidexpensify]

@aimane-chnaif can you confirm @adamgrzybowski's theory above? Thanks

[laurenreidexpensify]

@aimane-chnaif bump ^^

[aimane-chnaif]

I am not able to reproduce anymore. @bernhardoj can you confirm?

[bernhardoj]

@aimane-chnaif yes, I'm not able to reproduce anymore too.

[aimane-chnaif]

@laurenreidexpensify let's close this as fixed already