[HOLD for payment 2023-05-22] [$1000] Infinite loading when returning back from Plaid if user hits CTRL+SHIFT+K before the Plaid page opens up

[kavimuru]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

1. Go to web chrome
2. Go to Payments
3. Click on Add payment method
4. (Very important Step) Click on Bank account and immediately click on CTRL+SHIFT+K key to open up the new group page (The shortcut keys should be pressed before the Plaid URL bank account page opens up )
5. Now click on exit
6. Notice that there is infinite loading on the right side menu

Expected Result:

There should not be infinite loading on returning back from Plaid URL if user hits CTRL+SHIFT+K key before the plaid URL opens up

Actual Result:

There is infinite loading on the right hand side menu

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

error-2023-04-17_19.50.34.mp4

Recording.267.mp4

Expensify/Expensify Issue URL:
Issue reported by: @priya-zha
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1681740790785869

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01bb5f680944cd2fd8
• Upwork Job ID: 1648640859123245056
• Last Price Increase: 2023-05-03

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[huzaifa-99]

Proposal

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

We are trying to solve the broken infinite loader when coming back from plaid if user had pressed CTRL+SHIFT+K before the plaid page is opened

What is the root cause of that problem?

In the AddPersonalBankAccountPage, onExitPlaid will navigate to the last screen.

1. In normal secnario, /settings/payments is the last screen before going to /settings/payments/add-bank-account, so when onExitPlaid is triggered, we are redirected to the previous page which is settings/payments.
2. When we press CTRL+SHIFT+K, /new/group is added to navigation, so the stack now looks like this

TOP -> /new/group
2nd -> /settings/payments/add-bank-account
3rd -> /settings/payments

and we come back from the plaid page we trigger the onExitPlaid callback, this will remove the /new/group from navigation and will redirect us to /settings/payments/add-bank-account when it should redirect us back to /settings/payments

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

I think we should not mix workflows here as creating a new group and adding a bank/plaid account are 2 separate workflows, instead we should block one of these. I would suggest to implement an option from the following

1. We should block the CTRL+SHIFT+K shortcut when user visits the /settings/payments/add-bank-account page by subscribing to the keyboard shortcuts lib and prevent the group shortcuts keys event from bubbling to stop the /new/group (group chat modal) from opening by subscribing to KEYBOARD_SHORTCUTS.NEW_GROUP in the componentDidMount before this line

const groupShortcutConfig = CONST.KEYBOARD_SHORTCUTS.NEW_GROUP;
this.unsubscribeGroupShortcut = KeyboardShortcut.subscribe(
    groupShortcutConfig.shortcutKey, 
    () => {}, // do nothing
    groupShortcutConfig.descriptionKey, 
    groupShortcutConfig.modifiers, 
    true, // captureOnInputs
    false, // shouldBubble
    0, // priority (0=high)
    true // shouldPreventDefault (could be false as well)
);

2. Or we should stop the plaid account creation by triggering onExitPlaid when user presses CTRL+SHIFT+K shortcut and allow the user to create the group chat by subscribing to KEYBOARD_SHORTCUTS.NEW_GROUP in the componentDidMount before this line

const groupShortcutConfig = CONST.KEYBOARD_SHORTCUTS.NEW_GROUP;
this.unsubscribeGroupShortcut = KeyboardShortcut.subscribe(
    groupShortcutConfig.shortcutKey, 
    this.props.onExitPlaid, // call the exit plaid cb
    groupShortcutConfig.descriptionKey, 
    groupShortcutConfig.modifiers, 
    true, // captureOnInputs
    true, // shouldBubble
    0, // priority (0=high)
    false // shouldPreventDefault
);

With both the options above we should unsubscribe to the group shortcut when the component unmounts

componentWillUnmount() {
    if (this.unsubscribeGroupShortcut) {
        this.unsubscribeGroupShortcut();
    }
}

What alternative solutions did you explore? (Optional)

We could allow the user to use both the shortcut key and be able to use plaid, but I think it as mixing cross cutting concerns/workflows. To implement this we need change the order of navigation i.e put /new/group before /settings/payments/add-bank-account, so the navigation stack becomes

TOP -> /settings/payments/add-bank-account
2nd -> /new/group
3rd -> /settings/payments

now when the user will be navigated back from plaid he will see the /new/group page

[MelvinBot]

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.

[hoangzinh]

Proposal

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

Infinite loading when returning back from Plaid if user hits CTRL+SHIFT+K before the Plaid page opens up

What is the root cause of that problem?

• Currently, when exitPlaid, we call Navigation.goBack at this line. For example, how we navigate in normal case:
  
  Payment page => Add bank account page => when exitPlaid, it goes back to Payment page

• So what happens when user hits CTRL+SHIFT+K during Plaid login:
  
  Payment page => Add bank account page => Search page => when exitPlaid, it goes back to Add bank account page.

• It shows infinite loading, because in Add bank account page, if we haven't had any data yet => it will show loading indicator
  

  App/src/components/AddPlaidBankAccount.js

  Lines 135 to 139 in c144663

  	{lodashGet(this.props.plaidData, 'isLoading') && (
  	<View style={[styles.flex1, styles.alignItemsCenter, styles.justifyContentCenter]}>
  	<ActivityIndicator color={themeColors.spinner} size="large" />
  	</View>
  	)}

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

I think when we exit Plaid, we would like go back to the payment page so user can select another method or do something else. So I think we can replace Navigation.goBack by explicitly navigate to route of the payment page in this line

Code reference:

onExitPlaid={() => { Navigation.navigate(ROUTES.SETTINGS_PAYMENTS); }}

[HezekielT]

Proposal

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

Infinite loading when returning back from plaid if user hits CTRL+SHIFT+K before plaids page open up

What is root cause of that problem?

When the user exits plaid, the onExitPlaid, which causes the screen to navigate back to previous one, is called. Now normally when we open the plaid page, the flow could be something as shown below:
/setting => /setting/payments/ => /setting/payments/add-bank-account.

• Now when the user exits plaid the onExitPlaid navigates back to the previous screen which is /setting/payments.
• But if the user hits CTRL+SHIFT+K before the plaid page opens up, then the new/group route will be added to the stack and the stack will be something like as shown below:
  /setting => /setting/payments/ => /setting/payments/add-bank-account => /new/group
• Now this time when the user exits plaid the navigation.goBack method in onExitPlaid() causes the screen to return back from /new/group to /setting/payments/add-bank-account and given that this.props.plaidData.isLoading is true once we are in /setting/payments/add-bank-account, the infinite loading happens.

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

Logically, if a user hits CTRL+SHIFT+K, then the user must have made a change of mind and wants to create a new group instead of adding a bank account or opening the plaid page. Therefore we should cancel/prevent the plaid page from being opened and allow the user to proceed by creating a new group. This can be achieved by removing the /setting/payments/add-bank-account before adding /new/group to the navigation.

We can implement the above solution simply by navigating back before opening the modal- that is the new group modal - in AuthScreen here.

Code Reference:

this.unsubscribeGroupShortcut = KeyboardShortcut.subscribe(groupShortcutConfig.shortcutKey, () => {
      Navigation.goBack()
      Modal.close(() => Navigation.navigate(ROUTES.NEW_GROUP));
  ...
}

If it is necessary, we can execute the above Navigation.goBack() method only if the current screen is /settings/payments/add-bank-account.

This issue also occurs if the user hit CTRL+K in addition to CTRL+SHIFT+K so we need to add Navigation.goBack() for CTRL+K as well.

This way we can ensure that the user doesn’t have to exit the plaid first in order to use the shortcut keys again.

What alternative solution did you explore? (Optional)

Result

Before the fix:

Screencast-before.mov

Working well after the fix:

Screencast.mov

[HezekielT]

Alternatively, if we don’t want the plaid page to be prevented from being opened but want to show the new group modal when the user exits the plaid instead of the infinite loading, We can achieve that as shown below.

1. create a method called navigateBack and bind it in the constructor in AddPersonalBankAccountPage.js

 navigateBack() {
	let currentPath = new URL(getCurrentUrl()).pathname;

	/* now if the user clicked on the CTRL+SHIFT+K  shortcut before the plaid page was opened,
	the value of currentPath will be `/new/group` otherwise `/setting/payments/add-bank-account`
	*/

	Navigation.goBack();
	if(_.isEqual(currentPath, ‘/’ + ROUTES.NEW_GROUP)
	|| _.isEqual(currentPath, ‘/’ + ROUTES.SEARCH)) {
		Navigation.navigate(currentPath)
	}
}

2. change the onExitPlaid={Navigation.goBack} to onExitPlaid={this.navigateBack}
3. Import getCurrentUrl => import getCurrentUrl from '../libs/Navigation/currentUrl'; to AddPersonalBankAccountPage.js

Result

Working well after using the alternative approach

Screencast-another.mov

[laurenreidexpensify]

@0xmiroslav bump for review ^^

[hellohublot]

Proposal

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

Infinite loading when returning back from Plaid if user hits CTRL+SHIFT+K before the Plaid page opens up

What is the root cause of that problem?

onExitPlaid={Navigation.goBack}
onExitPlaid={() => BankAccounts.setBankAccountSubStep(null)}

Navigation.goBack only go back the NewGroupPage

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

Solution A:

When we execute the shortcut key to open a new window, we should actually close the old window. As we add more and more shortcut keys, users may misremember the shortcut keys, so we need to give user feedback every time when the shortcut key is pressed

I hope this solution can both solves #17598 and #17482

• Step 1: Turn off repeating opacity animations

_before.mov

It Can be reproduced in production environment, We can clearly see that when multiple Modals overlap, the background opacity also overlaps,
So we can judge whether it is the lowest Modal, and we only perform opacity animation on the lowest Modal

App/src/libs/Navigation/AppNavigator/modalCardStyleInterpolator.js

Lines 39 to 43 in 6fb0969

	opacity: progress.interpolate({
	inputRange: [0, 1],
	outputRange: [0, variables.overlayOpacity],
	extrapolate: 'clamp',
	}),

+   index,
    current: {progress},

    ...

-   opacity: progress.interpolate({
+   opacity: index == 0 ? progress.interpolate({
        inputRange: [0, 1],
        outputRange: [0, variables.overlayOpacity],
        extrapolate: 'clamp',
-   }),
+   }) : 0,

• Step 2: Replace the existing Modal

We add the resetModalRoute method to DeprecatedCustomActions, we reference the resetModalRoute method in Navigation, and then we replace AuthScreens.Navigation.navigate with resetModalRoute

function resetModalRoute(route) {
    const activeState = getActiveState();
    const newStateFromRoute = getStateFromPath(route, linkingConfig.config);
    const action = getActionFromState(newStateFromRoute, linkingConfig.config);
    let actionType = activeState.routes.length > 1 ? StackActions.replace('').type : CommonActions.navigate('').type
    
    navigationRef.current.dispatch({
        ...action,
        type: actionType,
        target: activeState.key,
    });
}

_after.mov

Solution B:

• If the user opens the other page, we should probably interrupt the plaid process immediately, So we can add a withNavigationFocus to PlaidLink, then we execute if (!props.isFocused) { return } before calling open()

Solution C:

• We can imitate /lib/actions/Modal, add /lib/actions/PlaidLink, and then we execute PlaidLink.close when AuthScreens executes Modal.close

What alternative solution did you explore? (Optional)

Not Yet

[laurenreidexpensify]

@0xmiroslav bump ^^

[0xmiros]

reviewing proposals

[aldo-expensify]

Tested more the option of blocking based on the URL and we ended up deciding that it blocks the shortcut for more time (views) that we would like.

We are going to go with @huzaifa-99 's first option in this proposal, but with slightly different parameters to not block the shortcuts when the plaid modal is gone:

this.unsubscribeGroupShortcut = KeyboardShortcut.subscribe(
    groupShortcutConfig.shortcutKey,
    () => {}, // do nothing
    groupShortcutConfig.descriptionKey,
    groupShortcutConfig.modifiers,
    false,
    () => lodashGet(this.props.plaidData, 'bankAccounts', []).length > 0,
);

The redefinition of shortcuts to block them is also a pattern here:

App/src/components/KeyboardShortcutsModal.js

Lines 74 to 93 in bb13d03

	const closeShortcutEscapeModalConfig = CONST.KEYBOARD_SHORTCUTS.ESCAPE;
	this.subscribedOpenModalShortcuts.push(KeyboardShortcut.subscribe(closeShortcutEscapeModalConfig.shortcutKey, () => {
	ModalActions.close();
	KeyboardShortcutsActions.hideKeyboardShortcutModal();
	}, closeShortcutEscapeModalConfig.descriptionKey, closeShortcutEscapeModalConfig.modifiers, true, true));
	const closeShortcutEnterModalConfig = CONST.KEYBOARD_SHORTCUTS.ENTER;
	this.subscribedOpenModalShortcuts.push(KeyboardShortcut.subscribe(closeShortcutEnterModalConfig.shortcutKey, () => {
	ModalActions.close();
	KeyboardShortcutsActions.hideKeyboardShortcutModal();
	}, closeShortcutEnterModalConfig.descriptionKey, closeShortcutEnterModalConfig.modifiers, true));
	// Intercept arrow up and down keys to prevent scrolling ArrowKeyFocusManager while this modal is open
	const arrowUpConfig = CONST.KEYBOARD_SHORTCUTS.ARROW_UP;
	this.subscribedOpenModalShortcuts.push(KeyboardShortcut.subscribe(arrowUpConfig.shortcutKey, () => {
	}, arrowUpConfig.descriptionKey, arrowUpConfig.modifiers, true));
	const arrowDownConfig = CONST.KEYBOARD_SHORTCUTS.ARROW_DOWN;
	this.subscribedOpenModalShortcuts.push(KeyboardShortcut.subscribe(arrowDownConfig.shortcutKey, () => {
	}, arrowDownConfig.descriptionKey, arrowDownConfig.modifiers, true));

Also, to keep things easier to maintain.. we could consider is to adding a property these shortcut constants like this:

        SEARCH: {
            descriptionKey: 'search',
            shortcutKey: 'K',
            modifiers: ['CTRL'],
            trigger: {
                DEFAULT: {input: 'k', modifierFlags: keyModifierControl},
                [PLATFORM_OS_MACOS]: {input: 'k', modifierFlags: keyModifierCommand},
                [PLATFORM_IOS]: {input: 'k', modifierFlags: keyModifierCommand},
            },
+.         type: NAVIGATION_SHORTCUT_TYPE
        },

where NAVIGATION_SHORTCUT_TYPE is just a string. Then we could iterate KEYBOARD_SHORTCUTS to subscribe and block all shortcuts that could navigate (type === NAVIGATION_SHORTCUT_TYPE).

[melvin-bot]

📣 @huzaifa-99 You have been assigned to this job by @aldo-expensify!
Please apply to this job in Upwork 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 📖

[huzaifa-99]

@aldo-expensify @0xmiroslav PR#18462 is up for review.

@aldo-expensify I followed the shortcuts redefinition pattern and also added a type key to the shortcuts for easier maintenance as you mentioned

[laurenreidexpensify]

Note to self: offers sent to @priya-zha @0xmiroslav @huzaifa-99 in Upwork

[huzaifa-99]

Accepted, Thank you @laurenreidexpensify

[aldo-expensify]

No overdue, PR under review

[aldo-expensify]

PR merged

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.13-5 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Block navigation shortcuts when plaid modal is loading/shown #18462

If no regressions arise, payment will be issued on 2023-05-22. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@0xmiroslav] The PR that introduced the bug has been identified. Link to the PR:
• [@0xmiroslav] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@0xmiroslav] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@0xmiroslav] Determine if we should create a regression test for this bug.
• [@0xmiroslav] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@laurenreidexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[laurenreidexpensify]

Everyone has been paid - @0xmiroslav let me know when you've completed steps above so we can close. Thanks!

[0xmiros]

No PR caused regression. The issue always existed after we implement shortcut keys.
As this is edge case, no need regression test step.

[0xmiros]

@laurenreidexpensify this is eligible for timeline bonus. PR was merged within 5 days including weekend.

[huzaifa-99]

@laurenreidexpensify this is eligible for timeline bonus. PR was merged within 5 days including weekend.

@0xmiroslav I had the same thought. Thanks for pointing out!

[laurenreidexpensify]

Ah sorry folks I missed that was during the weekend. Will issue a bonus now.

[laurenreidexpensify]

bonuses issued. confirmed no further regression steps. closing. thanks everyone!