[HOLD for payment 2023-07-24] [$1000] Correct chat is not opened by tapping chat link

[kavimuru]

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

---

This issue was found when retesting #19914

Action Performed:

1. Using Android m/Web, login to NewDot with account A and open any DM. Copy the report url from the address bar (like https://staging.new.expensify.com/r/1242891003534332
   )
2. Paste it in any external app (Slack or Telegram)
3. Log in with same account (account A) in android app
4. In the android app, open any other chat (not the one you opened in step 1)
5. Go back to the external app and tap link you pasted in external app

Expected Result:

Chat from step 1 should be opened without any visual issues.

Actual Result:

Chat from step 4 stays on the screen

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

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

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.3.26-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
Notes/Photos/Videos: Any additional supporting documentation

Bug6090395_video_22__5_.mp4

Expensify/Expensify Issue URL:
Issue reported by: applause internal team
Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01b971875418ce264c
• Upwork Job ID: 1668776814229225472
• Last Price Increase: 2023-07-05

[melvin-bot]

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

[melvin-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

[DanutGavrus]

Proposal

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

Correct chat is not opened by tapping chat link.

What is the root cause of that problem?

This is what happens now if the app is opened in background and we try to open a chat from a deep link:

1. In Expensify.js we call Linking.addEventListener('url', (state) => Report.openReportFromDeepLink(state.url, isAuthenticated));;
2. In Report.js we call Navigation.navigate(ROUTES.getReportRoute(reportID));;
3. In Navigation.js we call linkTo(navigationRef.current, route, type);;
4. In linkTo.js we call:

if (action.payload.name === NAVIGATORS.CENTRAL_PANE_NAVIGATOR && getTopmostReportId(root.getState()) !== getTopmostReportId(state)) {
            action.type = 'PUSH';

Now, here is the problem

Both getTopmostReportId(root.getState()) and getTopmostReportId(state) return the same report id, so we never set action.type = 'PUSH', so we never navigate to the new report.

Why does this happen?

getTopmostReportId(state) returns the reportId from the URL, which is the new report ID where we need to navigate to. But, when coming from a deep link, the NavigationContainer has the new state inserted automatically as this is it's default behavior(one may check this as onStateChange callback is automatically triggered when the app gets reopened from the deep link), so getTopmostReportId(root.getState()) returns the new report id, even if we did not navigate there yet.

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

1. As everything starts from openReportFromDeepLink, we may pass an optional parameter set to false by default, in order to let linkTo know when we come from a deep link.
2. We know getTopmostReportId(root.getState()) will return the same reportId when coming from a deep link, so we need another way to get the last visible reportId. I have some options in mind such as:
   2.1. Either, when onStateChange gets called for the NavigationContainer, in updateSavedNavigationStateAndLogRoute we may also save the previousState;
   2.2. Or, add an event listener for when app goes to background on native, same as we already have the one from when it comes back active. Inside it, save lastReportId which is going to be the current opened report id if any, or null/undefined otherwise.
   2.3 Or, I may think of other ways to get the topmost report id inside linkTo if previous 2 are not the best, let me know.
3. Now, inside linkTo, we may update the if, such that when coming from a deep link, will use the other way to get the topmost report id instead of getTopmostReportId(root.getState()).

What alternative solutions did you explore?

I. If we don't want to add and pass a new parameter when coming from a deep link, we may completely move from using getTopmostReportId(root.getState()) to always saving a report id in a context when opening a report, and resetting it when closing one, which will be used here. linkTo.js would look like:

const topmostReportId = the one from where we saved it;
if (action.payload.name === NAVIGATORS.CENTRAL_PANE_NAVIGATOR && topmostReportId !== getTopmostReportId(state)) {
            action.type = 'PUSH';

II. If we don't want to save the topmost report id, we may also just pass when coming from a deep link, and set action.type = 'REPLACE', thus we'll replace the current report with the one from the deep link every time, but this will mess the back navigation as the previous screen was replaced with this one.

[melvin-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.

[melvin-bot]

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

[melvin-bot]

Triggered auto assignment to @flaviadefaria (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[melvin-bot]

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

[melvin-bot]

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

[WikusKriek]

Proposal

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

Navigating to conversation via link does not work when you are already on a chat.

What is the root cause of that problem?

When you are already on a chat and try and navigate to a new chat via link the ReportScreen does not unmount.
The reportID prop also does not get updated from the new url it stays the same and the report never reloads with the reportID from the URL.

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

We need to add a check similar to the Notification navigation check.

App/src/libs/Notification/PushNotification/subscribeToReportCommentPushNotifications.js

Lines 21 to 25 in b77cf55

	// If a chat is visible other than the one we are trying to navigate to, then we need to navigate back
	if (Navigation.getActiveRoute().slice(1, 2) === ROUTES.REPORT && !Navigation.isActiveRoute(`r/${reportID}`)) {
	Navigation.goBack();
	}
	Navigation.navigate(ROUTES.getReportRoute(reportID));

In the componentDidUpdate of the ReportScreen

App/src/pages/home/ReportScreen.js

Line 155 in b77cf55

componentDidUpdate(prevProps) {

We just need to make a few changes to check that the active route is NOT the route of the current props.reportID.
We then need to call goBack and then navigate to the correct route.

What alternative solutions did you explore? (Optional)

N/A

[bernhardoj]

Proposal

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

Unable to open a different chat by deeplink

What is the root cause of that problem?

Currently, we have a deep link handler specifically for report screen introduced in this PR

App/src/Expensify.js

Lines 163 to 167 in 862955c

	// If the app is opened from a deep link, get the reportID (if exists) from the deep link and navigate to the chat report
	Linking.getInitialURL().then((url) => Report.openReportFromDeepLink(url, isAuthenticated));
	// Open chat report from a deep link (only mobile native)
	Linking.addEventListener('url', (state) => Report.openReportFromDeepLink(state.url, isAuthenticated));

Why we have it for report screen only? The best guess I have is, before the navigation reboot, we use a drawer navigator for LHN and report screen, so we need a custom handler for report screen that will handle whether to close or open the drawer. Below is the screenshot of the old Navigation.navigate. If it's a drawer route, which is true for report screen, it will do a custom action.

Now, we have completely moved to stack navigator and the navigate implementation is updated. When we navigate to a report screen, it will compare the current report screen id and the report id we want to navigate (both id is taken from navigation state). If it's different, we want to push it to the stack. This works well when we just doing a simple navigation, but not with deeplink.

App/src/libs/Navigation/linkTo.js

Lines 26 to 30 in 862955c

	// If action type is different than NAVIGATE we can't change it to the PUSH safely
	if (action.type === 'NAVIGATE') {
	// If this action is navigating to the report screen and the top most navigator is different from the one we want to navigate - PUSH
	if (action.payload.name === NAVIGATORS.CENTRAL_PANE_NAVIGATOR && getTopmostReportId(root.getState()) !== getTopmostReportId(state)) {
	action.type = 'PUSH';

When we are doing a deeplink, rn-navigation will navigate to the target report id, even without the custom deeplink handler we have. So, when we deeplink to report B, the root navigation state is already updated with the report B id, so when the custom handler runs, both current report screen id and target report id is the same, thus the navigation action type stays as NAVIGATE instead of PUSH.

NAVIGATE action won't push the new screen if it's already exist in the stack, that's why I believe we have this commit that will change the type to PUSH if the report id is different.

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

As the intention is to push the new report screen to stack if the report ID is different, I think the better way to handle it is by adding getId props to report screen here:

App/src/libs/Navigation/AppNavigator/Navigators/CentralPaneNavigator.js

Lines 18 to 30 in 862955c

	<Stack.Screen
	name={SCREENS.REPORT}
	// We do it this way to avoid adding this to url
	initialParams={{openOnAdminRoom: openOnAdminRoom ? 'true' : undefined}}
	options={{
	headerShown: false,
	title: 'New Expensify',
	// Prevent unnecessary scrolling
	cardStyle: styles.cardStyleNavigator,
	}}
	component={ReportScreenWrapper}
	/>

getId will return report id.

getId={({params}) => params.reportID}

This will always push the new report screen if the report ID is not in the stack yet. Also, we don't need this logic anymore.

App/src/libs/Navigation/linkTo.js

Lines 28 to 31 in 862955c

	// If this action is navigating to the report screen and the top most navigator is different from the one we want to navigate - PUSH
	if (action.payload.name === NAVIGATORS.CENTRAL_PANE_NAVIGATOR && getTopmostReportId(root.getState()) !== getTopmostReportId(state)) {
	action.type = 'PUSH';

Or is there any clear reason we are doing it that way?

[thesahindia]

@maddylewis @pecanoro, please reassign. I have multiple pending tasks right now, so I won't be able to take this.

[melvin-bot]

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

[melvin-bot]

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

[pecanoro]

Sorry, I missed this. I even checked the videos from the original PR and it seems it was working just fine back the, so something was introduced in between. Does this happen only on iOS? So I can test it as well.

[DanutGavrus]

@pecanoro It happens on Android too.

[pecanoro]

I can't reproduce it on Android on my phone, it's working well.

[pecanoro]

@maddylewis Are you able to reproduce the potential regression on your iOS phone? It's working well for me on 1.3.50-2

[maddylewis]

i am OOO until ~Aug 14. i can try to repro once i return or we can re-assign the bug label to have someone else try!

[abdulrahuman5196]

I think it is a regression actually - #23441 (comment)

[pecanoro]

@abdulrahuman5196 Got it! It seems a contributor on the other issue found the root cause and a better solution for this issue and the regression. Are we having them fix it? If they confirmed the regression, we have to cut the payment for this one in half.

[DanutGavrus]

@abdulrahuman5196 If the other contributor said in the alternative Proposal that removing only the change from that PR does not cause the original issue anymore, can't it be that something has changed meanwhile(which fixed the original Issue under the hood and created the new one), thus not being a regression? 🤷

[pecanoro]

The PR still caused a regression since once the PR is reverted, the new bug disappears. However, if you can find me the PR that introduced those changes that might have broken your PR, I will reconsider it. So far that has been a theory but we have yet to find such PR.

[melvin-bot]

@pecanoro, @allroundexperts, @maddylewis, @DanutGavrus Huh... This is 4 days overdue. Who can take care of this?

[melvin-bot]

@pecanoro, @allroundexperts, @maddylewis, @DanutGavrus Huh... This is 4 days overdue. Who can take care of this?

[pecanoro]

@maddylewis It's all yours for payment now!

[maddylewis]

Payments:

• no reporting bonus
• @allroundexperts ($500 - paid via NewDot)
• @DanutGavrus ($1000 - paid via Upwork - I originally paid $1500 and requested a $1k refund due to the regression, so @DanutGavrus is owed $1k at this point)

[allroundexperts]

@maddylewis It was concluded that the PR caused a regression so it's $500 for me and nothing for @DanutGavrus!

[DanutGavrus]

@pecanoro

once the PR is reverted, the new bug disappears

For some reason, same does the old bug.

So far that has been a theory but we have yet to find such PR.

Sorry, I wanted to try that sooner, but I was not able to run the app on Android for some time until now. So, I've gathered these facts:

1. The old issue was that: If chat A is opened, chat B won't get opened at all by deeplinking;
2. The new issue is that: If no chat is opened, chat B gets opened twice;
3. Reverting the PR fixes both old & new issues.

These facts give the impression that what made 3. possible, also added one extra navigation, thus it fixed 1. by navigating once instead of not at all, and created 2. which navigates twice instead of once now.

if you can find me the PR that introduced those changes that might have broken your PR, I will reconsider it

What is bothering me is that I couldn't find the PR which fixed the original issue neither, and we know there must be something.

Is anyone able to identify the change which fixed the old issue? I'd like to investigate if that is related to the new bug. Thanks!

@allroundexperts What would be your opinion about these?
@pecanoro If this remains a regression, I 100% accept it! Just wanted to express why I'm unsure. Thank you!

[maddylewis]

ahhh, thank you @allroundexperts! I needed some clarification on the status of payments for this one.

I'll let @pecanoro respond to @DanutGavrus' comment before closing this one!

[maddylewis]

@allroundexperts - i will trust your call on this one. double-checking that no payment should be issued to @DanutGavrus after they outlined these details - #20624 (comment)

[allroundexperts]

@maddylewis Lets treat it as a regression. Better be safe than sorry!

[JmillsExpensify]

Reviewed the details for @allroundexperts. $500 approved for payment via NewDot based on BZ summary.