[HOLD for payment - waiting on deploy][$250] Opening Two-Factor Authentication settings causes two duplicate API requests for EnableTwoFactorAuth

[m-natarajan]

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

---

Version Number: 9.0.24-4
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
Expensify/Expensify Issue URL:
Issue reported by: @tgolen
Slack conversation: https://expensify.slack.com/archives/C05LX9D6E07/p1724704755405909

Action Performed:

1. Go to staging.new.expensify.com
2. Click Account settings > Security > Two Factor authentication setting

Expected Result:

No duplicate calls API calls

Actual Result:

Two duplicate API requests for EnableTwoFactorAuth

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: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

2024-08-26_14-37-41.mp4

Add any screenshot/video evidence

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0184daf84295192016
• Upwork Job ID: 1828808342978232742
• Last Price Increase: 2024-09-04
• Automatic offers:
• Nodebrute | Contributor | 103846483

[melvin-bot]

Current assignee @tgolen is eligible for the AutoAssignerNewDotQuality assigner, not assigning anyone new.

[melvin-bot]

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

[Nodebrute]

Edited by proposal-police: This proposal was edited at 2024-08-28 16:13:58 UTC.

Proposal

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

Opening Two-Factor Authentication settings causes two duplicate API requests for EnableTwoFactorAuth

What is the root cause of that problem?

The behavior you're seeing, where not just the two-factor authentication call but all calls in useEffect are running twice, is due to React's Strict Mode. This change was introduced in this PR and is only in the development environment. We do not see this behavior in the production build.

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

I prefer turning off Strict Mode, as introduced in this PR This way, development and production environments will behave the same.

Note: StrictMode is not needed for concurrent react to work but it is much safer to use it.

We can disable strict mode from here

App/src/CONFIG.ts

Line 100 in cadd5a8

USE_REACT_STRICT_MODE_IN_DEV: true,

What alternative solutions did you explore? (Optional)

We can create a ref and then in useEffect we can early return if it's already mounted. With the implementation of useRef, we ensure that API calls within useEffect are made only once, even in development mode with React's Strict Mode enabled. This method prevents multiple invocations of the effect due to Strict Mode's behavior.

App/src/pages/settings/Security/TwoFactorAuth/Steps/CodesStep.tsx

Line 46 in a6564ac

useEffect(() => {

We can do something like this

    const isMounted = useRef(false);
    useEffect(() => {
        if (isMounted.current) {
            return; // Exit early if effect has already run
        }
        // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
        if (account?.requiresTwoFactorAuth || account?.recoveryCodes || !isUserValidated) {
            return;
        }
        Session.toggleTwoFactorAuth(true);
        isMounted.current = true;
     
    }, [isUserValidated]);

[abzokhattab]

Not able to reproduce the issue on prod

[tgolen]

Thanks for the investigation @Nodebrute. I think the thing that I really dislike about that is now dev is a very different environment than production, which makes testing and development much more difficult. We are being forced into some weird patterns (using a bunch of useRef) to try and get development to work more like we expect production to work.

Do you know if React is planning to make production work in this concurrent mode like dev is running in? I guess that would return it to an apples-to-apples situation at some point, which is better, but I really don't like having to guess "does this only happen in dev or not".

I'm going to assign this to external and get a C+ to review your proposal to implement (because even though it only happens in dev, it messed me up enough that I think it's worth fixing).

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~0184daf84295192016

[melvin-bot]

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

[tgolen]

@mollfpr Are you OK with the proposal here? I think it's OK to move forward with.

[johncschuster]

@mollfpr bump on the above!

[mollfpr]

Sorry for the delay!

Are you OK with the proposal #48034 (comment)? I think it's OK to move forward with.

@tgolen The issue seems to affect other pages for example OpenReport and OpenInitialSettingsPage . I'm onboard if we decide to specifically fix the issue for this 2FA page.

Also, the solution doesn't fix your issue here #48030 (comment).

[Nodebrute]

@mollfpr The issue affects any API calls made within useEffect because Strict Mode runs effects twice. To ensure consistency between production and development, we can turn off Strict Mode.

[tgolen]

Also, the solution doesn't fix your issue here #48030 (comment).

OK, thanks!

@tgolen The issue seems to affect other pages for example OpenReport and OpenInitialSettingsPage .

@mollfpr The issue affects any API calls made within useEffect because Strict Mode runs effects twice. To ensure consistency between production and development, we can turn off Strict Mode.

OK, that is a bit unfortunate. Maybe we should discuss this on Slack in the open-source channel?

[tgolen]

I started a conversation about it in Slack here.

[johncschuster]

Thanks for doing that, @tgolen! 🙌

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[melvin-bot]

@tgolen, @johncschuster, @mollfpr Whoops! This issue is 2 days overdue. Let's get this updated quick!

[johncschuster]

It looks like the discussion in Slack may still be ongoing.

[johncschuster]

@mollfpr I haven't been able to follow the discussion very closely. Do you feel we've come to a conclusion? If so, can you summarize where we've landed?

[mollfpr]

I haven't been able to follow the discussion very closely. Do you feel we've come to a conclusion? If so, can you summarize where we've landed?

We haven't had any conclusion yet, but we might have a holistic solution to fix this issue. I think it's better to move this issue weekly/monthly and I'll keep visiting the thread.

[tgolen]

I think there was enough support internally for disabling strict mode by default until there is a more clear problem statement and project detailing what is expected to be gained from having strict mode enabled.

@Nodebrute Can you please update your proposal and I'll go ahead and assign this to you.

[melvin-bot]

📣 @Nodebrute 🎉 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 📖

[johncschuster]

Waiting on QA to test the fix. @tgolen, it looks like Applause is asking on the PR which URL should be used for staging. Can you let them know?

[mollfpr]

I have replied to the question.

[johncschuster]

@mollfpr / @tgolen, I'm reviewing the linked PR to calculate the payment date, but I'm having trouble finding what I'm looking for. On #48722, I see it approved and merged but not deployed. I see our internal SO articles refer to using the merge date to calculate the regression threshold window, but without a deploy, I don't think we can really test for regressions. Can you help me sort out the date so I don't hold this one up?

[mollfpr]

@johncschuster I think we can use the date for this issue #48791 closed. It's 11th September.

[johncschuster]

Payment Summary:

Contributor: @Nodebrute paid $250 via Upwork - PAID

Contributor+: @mollfpr due $250 via NewDot

Upwork job here! Please apply

@mollfpr can you please provide regression test steps if they are necessary?

[mollfpr]

@johncschuster I think we don't need a regression step since it's only affecting the development side and it's knowingly a feature from React but we happen not to need it yet.

[johncschuster]

Ok great! Thanks for clarifying that! Please request payment via NewDot!

[garrettmknight]

$250 approved for @mollfpr