[$250] Group chat - App redirects to Concierge chat when leaving group chat and room

[izarutskaya]

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: 1.4.67-0 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. Visit a few reports.
3. Go to FAB > Start chat.
4. Create a group chat.
5. Click on the chat header.
6. Click Leave.

Expected Result:

• When a user leaves a room/group, we should navigate them to their last opened report
• If it's a new user who opens the link from incognito mode and then leaves the room, they will be directed to the concierge

Actual Result:

• User is always redirected to Concierge chat, the same issue happens when leaving the room
  • On Android app, it redirects to the previous report, but it opens Concierge chat when tapping back button on the previous report.
  • On iOS app, it returns to group details page.

Workaround:

Unknown

Platforms:

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

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

Screenshots/Videos

https://github.com/Expensify/App/assets/115492554/72c339dc-5f39-4aa9-8434-079d590d95ba

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01de0c9a8aaa7e6078
• Upwork Job ID: 1784843922902315008
• Last Price Increase: 2024-04-29
• Automatic offers:
• ahmedGaber93 | Reviewer | 0
• Nodebrute | Contributor | 0

[melvin-bot[bot]]

Triggered auto assignment to @dylanexpensify (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.

[melvin-bot[bot]]

Triggered auto assignment to @Julesssss (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ 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.

[izarutskaya]

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

[izarutskaya]

We think this issue might be related to the #vip-vsb.

[izarutskaya]

Production

https://github.com/Expensify/App/assets/115492554/8f91f94c-ce06-4752-b1dc-b22b836ca288

[Julesssss]

Not critical enough to block deploy. Opening up to contributors.

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[Julesssss]

Shared with C+ team here.

[Julesssss]

After reviewing all the merged PRs, these 3 seem like the only possible causes:

• https://github.com/Expensify/App/pull/40701
• https://github.com/Expensify/App/pull/36420
• https://github.com/Expensify/App/pull/39993 :x:

[Julesssss]

I can't reproduce on web/dev v1.4.67-0 <-- I can now, but I can also reproduce on production

[Nodebrute]

Proposal

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

Group chat - App redirects to Concierge chat when leaving group chat and room

What is the root cause of that problem?

It's coming from this PR https://github.com/Expensify/App/pull/40701. In this PR, we added a fallback route for cases where the navigation stack is empty. However, instead of popping the current screen, the goBack function navigates to the fallback route and here we are navigating again to the lastAccessedReportID https://github.com/Expensify/App/blob/e0b6a06d2f34f35cda96ccc4d05252ede111c732/src/libs/actions/Report.ts#L2525 This leads to the lastAccessedReportID being visited twice, causing it to appear twice in the navigation stack. Due to this broken navigation, the incorrect prevReport is passed, causing the isGroupChat check to return true, which triggers the code block that navigates to concierge. This is why this issue only occurs for groups and not for rooms. https://github.com/Expensify/App/blob/e0b6a06d2f34f35cda96ccc4d05252ede111c732/src/pages/home/ReportScreen.tsx#L515 https://github.com/Expensify/App/blob/e0b6a06d2f34f35cda96ccc4d05252ede111c732/src/pages/home/ReportScreen.tsx#L520-L527

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

I recommend we avoid using goBack() when it's the first route in the navigator and remove the fallbackRoute altogether. This change will resolve two issues Check if the current route index is 1 here

  const isFirstRoute = navigationRef.current?.getState().index === 1;

If that's the case, we don't need to use goBack().

 if (!isChatThread) {
            if(!isFirstIndex){
                Navigation.goBack();
            }
        }

Reverting this PR should resolve the issue. The original PR was created to address issue #40680 but that problem still persists. I suggest we revert this PR and explore a better solution for https://github.com/Expensify/App/issues/40680.

What alternative solutions did you explore? (Optional)

[shubham1206agra]

@Nodebrute See the above comment. Your RCA is not correct.

[Nodebrute]

@shubham1206agra Hi, I tested it locally, and it seems this https://github.com/Expensify/App/pull/40701 is causing the problem.

[Nodebrute]

To test quickly, just revert the changes in this block, and you'll see the issue is resolved. https://github.com/Expensify/App/pull/40701/files#diff-ce6bb391d0654bf99a360da7d25fd73b85bb120211c8b4c2432ef7c7d8c708f3R2471-R2474

[Nodebrute]

@Julesssss @ahmedGaber93 @shubham1206agra results

https://github.com/Expensify/App/assets/93134676/1fb53811-f8c3-4bd1-b0d2-0111fbafb362

[dylanexpensify]

@shubham1206agra mind confirming? Nice catch @Nodebrute!

[shubham1206agra]

@dylanexpensify Not sure about that as we have repro this issue in prod too. cc @Julesssss for confirmation.

[Nodebrute]

@shubham1206agra I just tested, and it's not reproducible in production.

https://github.com/Expensify/App/assets/93134676/a57ae245-4303-4d40-8f4b-158cc72e66d2

[shubham1206agra]

@Nodebrute Can you give proper RCA atleast? This might help us to determine what is the actual problem in the PR.

[melvin-bot[bot]]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[Pujan92]

@MonilBhavsar Seems an issue comes out after our PR but while debugging I see that this part of the ReportScreen gets executed which is responsible for navigating to the ConciergeChat.

https://github.com/Expensify/App/blob/6a37cd86f89e22f09b018cc756f4347b7a3004b7/src/pages/home/ReportScreen.tsx#L514-L538

I am not sure but something looks fishy with the navigation and it triggering above while passing the fallbackRoute for goBack.

[Nodebrute]

@shubham1206agra With the fallback route added, this code block is executed. I'm not sure why, but when the fallback route is added, we end up navigating to Concierge. It looks like this issue was already present, and this PR made it more noticeable. https://github.com/Expensify/App/blob/30bfabc7559318bf64c888a735c338656680933e/src/libs/Navigation/Navigation.ts#L218-L221

[ahmedGaber93]

Hi @shubham1206agra , if you have the time, you can take over this as you spent some effort on it.

[shubham1206agra]

@MonilBhavsar Seems an issue comes out after our PR but while debugging I see that this part of the ReportScreen gets executed which is responsible for navigating to the ConciergeChat.

https://github.com/Expensify/App/blob/6a37cd86f89e22f09b018cc756f4347b7a3004b7/src/pages/home/ReportScreen.tsx#L514-L538

I am not sure but something looks fishy with the navigation and it triggering above while passing the fallbackRoute for goBack.

@MonilBhavsar If you agree with this, please take over this issue.

[Nodebrute]

@shubham1206agra @MonilBhavsar Upon further investigation, I found that the root cause is related to the ReportUtils.isGroupChat(prevReport) here. When we call goBack() without a fallback, isRemovalExpectedForReportTyp is false, so we don't navigate to the Concierge chat. However, when we use goBack() with a fallback, isRemovalExpectedForReportTyp is true, which causes us to navigate to Concierge. https://github.com/Expensify/App/blob/6a37cd86f89e22f09b018cc756f4347b7a3004b7/src/pages/home/ReportScreen.tsx#L507-L509

Removing ReportUtils.isGroupChat(prevReport) also solves the issue

[shubham1206agra]

@marcaaron Can you look into this? As you have implemented https://github.com/Expensify/App/pull/39757

[Pujan92]

@Nodebrute I don't think we can remove that bcoz it is added for the below reason

https://github.com/Expensify/App/blob/6a37cd86f89e22f09b018cc756f4347b7a3004b7/src/pages/home/ReportScreen.tsx#L514

[dylanexpensify]

Bump @marcaaron when you can 🙇‍♂️

[marcaaron]

@shubham1206agra I'm not sure what you want me to look into, but happy to answer a more specific question if you've got one, thanks!

[Julesssss]

Okay, so the issue is resolved for me when reverting https://github.com/Expensify/App/pull/40701, however that PR doesn't seem to have introduced the issue:

It looks like this issue was already present, and this PR made it more noticeable.

@Nodebrute as you have correctly located two potential causes already, maybe you want to suggest another fix?

[Nodebrute]

@Julesssss This PR https://github.com/Expensify/App/pull/40701 fixed an issue where we had no routes in the navigator. However, it introduced a regression. Previously, we used this goBack to pop the current route, but with the added fallbackRoute, we're now navigating to the last read report, adding an extra route to the navigation stack instead of popping it. https://github.com/Expensify/App/blob/3b188e74f9ef7088f4c3f3a3fafc7f4f316e7298/src/libs/actions/Report.ts#L2510-L2513

I recommend we avoid using goBack() when it's the first route in the navigator and remove the fallbackRoute altogether. This change will resolve two issues https://github.com/Expensify/App/issues/41128 https://github.com/Expensify/App/issues/40680

[Nodebrute]

https://github.com/Expensify/App/assets/93134676/64c80cac-587d-4407-84fe-1cf7827fd4e9

[Julesssss]

Thanks @Nodebrute, that sounds good solution to me. Would you mind submitting a new proposal like you did here so we can review your new suggestion? Thanks.

[Nodebrute]

@Julesssss working on it.

[Nodebrute]

@shubham1206agra I've updated the proposal; you can try this solution

[ahmedGaber93]

@shubham1206agra let me know your answer here https://github.com/Expensify/App/issues/41128#issuecomment-2082633177.

[Julesssss]

Hi @ahmedGaber93 can you please take a look at these proposals? This updated proposal from @Nodebrute is the one I prefer currently.

[ahmedGaber93]

@Julesssss I will review it today.

[ahmedGaber93]

@Nodebrute Thanks for your proposal update.

Related to your RCA, I think you missed updating it, you just say It's coming from this PR #40701, I read the full discussion here, and it has a lot of different thoughts, so please make sure to update your proposal with the explained root cause.

Besides, your fix failed to pass https://github.com/Expensify/App/pull/40701 tests from 1 to 6, your video here https://github.com/Expensify/App/issues/41128#issuecomment-2093151005 seem follow different steps.

1. Create a public room as an admin
2. As a user A, join the public room
3. Copy the public room URL and signout
4. Paste the URL and signin
5. Leave the room
6. Ensure you're redirected to Concierge chat

https://github.com/Expensify/App/assets/41129870/299c7a43-5ab5-4078-9099-a7c126ea9a91

[Nodebrute]

@ahmedGaber93 Hey, I don't think users should be redirected to the concierge chat after leaving a room. That's not the current behaviour in our app.

After leaving room the user should be navigated to most recent report https://github.com/Expensify/App/blob/55c56bffbee8afd2fb5f5b2abfe30a92487bab52/src/libs/actions/Report.ts#L2646

[ahmedGaber93]

@Julesssss this issue is a regression from #40701, should we fix it here or by the #40701?

[ahmedGaber93]

Hey, I don't think users should be redirected to the concierge chat after leaving a room. That's not the current behaviour in our app.

@Nodebrute What I'm understanding from #40701 QA steps is we have two cases.

• After user login and he didn't visit any report (step 1 to 6), user should redirect to Concierge chat.
• if user visit some reports before the room he leaves it (steps 7-10), user should be navigated to most recent report

[Nodebrute]

When a user leaves a room/group, we navigate them to their last read report. In this scenario, the user will be directed to their last read report regardless of the device/tab/incognito they use to leave the room.

If it's a new user who opens the link from incognito mode and then leaves the room, they will be directed to the concierge.

@Julesssss, could you please confirm if this behavior aligns with the desired outcome?

Additionally, if we want to direct users to the concierge instead of their last read report when they leave the room in a new tab/incognito session, we can utilize the same check isFirstRoute

https://github.com/Expensify/App/blob/cd9a9ad735605624623ccf2ee4f9e6c0cda57af7/src/libs/actions/Report.ts#L2515

[Julesssss]

@Julesssss this issue is a regression from https://github.com/Expensify/App/pull/40701, should we fix it here

@ahmedGaber93 we don't want to revert that PR as because it was an important fix and it has been too long to safely revert. Also we seem to have a simple proposal to fix.

@Julesssss, could you please confirm if this behavior aligns with the desired outcome?

@Nodebrute yeah I agree that this is the desired behaviour 👍 I updated the issue description to make this clearer

[ahmedGaber93]

@ahmedGaber93 we don't want to revert that PR as because it was an important fix and it has been too long to safely revert. Also we seem to have a simple proposal to fix.

@Julesssss I just think it will be better if the issue fixed by #40701 author and C+ as they work on it before on some hours and have the full context of the issue instead of new contributors.

Also, the above proposal tries to make code works without finding the root cause of the issue and fix it.

@Nodebrute can you explain the root cause of the issue, the code which navigate to concierge is in ReportScreen.tsx and it targets another goal below https://github.com/Expensify/App/blob/6a37cd86f89e22f09b018cc756f4347b7a3004b7/src/pages/home/ReportScreen.tsx#L514

Why we need to edit navigateToMostRecentReport instead of ReportScreen.tsx?

Also, the first route condition should be === 0 and not === 1

  const isFirstRoute = navigationRef.current?.getState().index === 1;

@Nodebrute hope you do not take my words as if I am trying to fight you with words or trying to prove you wrong, I just want us to work together to understand the root cause of the problem to make sure that what we are doing is the right solution.

Thanks!

[ahmedGaber93]

After last test in the latest main in web, I found the issue is not reproduced when leaving room, regardless it is the first root or not, but reproduced when leaving group.

Both action leaving room and leaving group use the same function navigateToMostRecentReport, why it works on room and failed in group?

I think we may have a hidden issue in some place that lead to this behavior.

https://github.com/Expensify/App/assets/41129870/77d8c8d2-86c6-42d4-b3db-ab266dae8fcb

[Nodebrute]

@ahmedGaber93 No probs. I have updated my RCA.

[Julesssss]

@Julesssss I just think it will be better if the issue fixed by https://github.com/Expensify/App/pull/40701 author and C+ as they work on it before on some hours and have the full context

I get your point, but internal employees are very busy with other internal tasks currently. I'll ask Monil to take a look at the most recent proposal once it has been reviewed though. @Nodebrute could you please link to the most recent RCA? I'm struggling to find which of your comments was most recently updated. Thanks