[Search v1] Chat - "From" link in IOU report details page navigates to the main chat twice

[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?: Y Logs: https://stackoverflow.com/c/expensify/questions/4856 Issue reported by: Applause-Internal team

Action Performed:

1. Launch New Expensify app.
2. Submit an expense to a user.
3. In 1:1 DM, tap on the IOU preview.
4. Tap on the chat header.
5. Tap "From" link.
6. Tap on the back button.

Expected Result:

App will return to report details page.

Actual Result:

App returns to the same main chat. It only returns to report details page after tapping on the back button again.

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
• [ ] MacOS: Chrome / Safari
• [ ] MacOS: Desktop

Screenshots/Videos

https://github.com/Expensify/App/assets/115492554/43cf78dc-2cf2-41f7-ac6f-21b43286139e

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01c414e0c471cc81d1
• Upwork Job ID: 1786350359808901120
• Last Price Increase: 2024-05-03
• Automatic offers:
• s77rt | Reviewer | 0
• bernhardoj | Contributor | 0

Issue Owner

Current Issue Owner: @s77rt

[melvin-bot[bot]]

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

[izarutskaya]

We think this issue might be related to the #collect project.

[bernhardoj]

Proposal

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

The navigation to the parent report happens twice.

What is the root cause of that problem?

When we press the "From" link, it will call goBack with the report screen as the fallback which will be used twice. One without the report action ID and the other one with the report action ID for comment linking. https://github.com/Expensify/App/blob/35890a01755f716443ee4f0d3c3236ce749cea71/src/components/ParentNavigationSubtitle.tsx#L38-L43

The issue happens after this PR where we will push the screen if the params is different, in this case, the report action ID param. https://github.com/Expensify/App/blob/35890a01755f716443ee4f0d3c3236ce749cea71/src/libs/Navigation/linkTo.ts#L155-L176

Previously, we only compare the report ID to decide whether to push the report screen or not.

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

I think we want to have the old behavior, that is to push the report screen only if the topmost report ID is different than the destination report ID, if it's other screen, then we will use the current condition.

const areParamsDifferent = action.payload.params?.screen === SCREENS.REPORT
    ? getTopmostReportId(rootState) !== getTopmostReportId(stateFromPath)
    : !shallowCompare(topmostCentralPaneRoute?.params, action.payload.params?.params);

[melvin-bot[bot]]

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

[trjExpensify]

@luacmartins @grgia putting this on your radar! I'll file it under Search v1.

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[s77rt]

@bernhardoj Thanks for the proposal. Your RCA makes sense. Can we get ride of the double navigation? That change is also recent https://github.com/Expensify/App/pull/40315.

[bernhardoj]

@s77rt I think we can't get rid of the double goBack. If I understand correctly from that PR, when we press the "From" link, we want to GO BACK to the parent thread. The PR previously attempted to have 1 go back as shown below,

but it doesn't work because getDistanceFromPathInRootNavigator compares the fallback route path with the parent thread route path in the root stack. https://github.com/Expensify/App/blob/90b67a5cd60c86a57b4484847d0b3f30cf2b2430/src/libs/Navigation/Navigation.ts#L121-L124 https://github.com/Expensify/App/blob/90b67a5cd60c86a57b4484847d0b3f30cf2b2430/src/libs/Navigation/Navigation.ts#L224-L236

The fallback route path contains a reportActionID, while the parent thread in the stack doesn't contain a reportActionID, so FORCED_UP navigation is used instead as shown in this video.

[luacmartins]

CC @adamgrzybowski @WojtekBoman @Kicu if you wanna take a look at this one.

[s77rt]

@bernhardoj With the two goBack we still end up with a FORCED_UP navigation. With one goBack are you still able to reproduce this bug https://github.com/Expensify/App/pull/40315#pullrequestreview-2004637539?

[bernhardoj]

Yes, I can reproduce it with one goBack.

https://github.com/Expensify/App/assets/50919443/de97f9c0-082e-4f75-8797-4414e7d47c35

The real issue is actually the send button becomes unresponsive when coming back from a thread. We have an issue for that here. So, the double goBack is a workaround so the user doesn't need to press back to go back to the parent chat.

If we fix the unresponsive button issue, then I think we won't need the double goBack anymore. The issue might be related to fabric updates. The gesture here doesn't respond at all. https://github.com/Expensify/App/blob/84118d84da7415bf1ee71fbfcc304bffb75a5530/src/pages/home/report/ReportActionCompose/SendButton.tsx#L26-L28

With the two goBack we still end up with a FORCED_UP navigation.

With the solution that I propose, at least there won't be 2 report screen pushes to the nav stack anymore. There will only be 1 report screen pushed and it will be replaced with a report screen that has a reportActionID param.

[s77rt]

@bernhardoj Thanks for checking. I was able to reproduce the bug too. I think we should hold on https://github.com/Expensify/App/issues/41528 and then remove the double goBack.

With the solution that I propose, at least there won't be 2 report screen pushes to the nav stack anymore

This could break https://github.com/Expensify/App/pull/40280

[bernhardoj]

Why will it break https://github.com/Expensify/App/pull/40280? The areParamsDifferent condition will still compare the whole params if it's not a report screen. The solution will fix this too.

[s77rt]

@bernhardoj What I meant to say is that change is intended and it's probably needed to fix some case, reverting may cause an issue but perhaps we can fix the issue they faced in another way. I have left a comment here https://github.com/Expensify/App/pull/40280#discussion_r1593358084

[s77rt]

@bernhardoj We got some context; That change is only needed for the new search page, for the reports we should rely only on the report ids as you suggested 👍

🎀 👀 🎀 C+ reviewed Link to proposal

[melvin-bot[bot]]

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

[melvin-bot[bot]]

📣 @s77rt 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job

[melvin-bot[bot]]

📣 @bernhardoj 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

[bernhardoj]

PR is ready

cc: @s77rt