[HOLD for payment 2023-08-08] [$1000] Web - App crashes after pressing Backspace/Delete key in Incorrect magic code page

[kbecciv]

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

---

Action Performed:

1. On login page, enter an email/phone number and click Continue
2. Enter WRONG Magic code
3. When the error is showing, hold the left-mouse button and drag over the Magic code field - (like we are selecting text BUT do not show the selection)
4. Press Backspace/Delete key on the keyboard

Expected Result:

Application should not crash

Actual Result:

Application crashes

Workaround:

Unknown

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

CRASH.mp4

Recording.3910.mp4

Expensify/Expensify Issue URL:
Issue reported by: @tranvantoan-qn
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1690254789469459

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~017414e8528c6cdd44
• Upwork Job ID: 1684228115617501184
2023-07-26
• Automatic offers:
• s77rt | Reviewer | 25775610
• Pujan92 | Contributor | 25775614
• tranvantoan-qn | Reporter | 25775617

[melvin-bot]

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

[kbecciv]

Proposal: @tranvantoan-qn
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1690254789469459

Proposal

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

App crashes after pressing Backspace/Delete key in Incorrect magic code page

What is the root cause of that problem?

After dragging, the editIndex will be undefined, making it unable to retrieve the object. Unfortunately, in the following code, we did not check for this scenario, which results in an app crash:
https://github.com/Expensify/App/blob/main/src/components/MagicCodeInput.js#L105

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

To prevent the crash, add the following condition:

        if (inputRefs.current[editIndex]) { 
            inputRefs.current[editIndex].blur();    
        }

[huzaifa-99]

Proposal

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

We want the app to not crash when backspace key is pressed on the magic code input (after drag select)

What is the root cause of that problem?

When we submit the form (it happens when we fill the last cell of input), then we set the focused index to undefined here.

And next, when we drag over the whole input, the focus event gets triggered, but we don't update the focused and edit index in that event. And next, when we press backspace, the focused index will still be undefined which causes the editIndex to also become undefined, and on blur, this undefined editIndex crashes the app.

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

I think we should use a single type for focusedIndex, and not use undefined (we can add -1 in place of that). This is important because eventually, we will be moving to typescript.

So when we blur the input we should set the focused index to -1 here.

And on focus event, when focusedIndex is -1, we update the focused index to the current cell index like this

if (focusedIndex == -1) {
    setInput('');
    setEditIndex(index)
    setFocusedIndex(index)
}

and we update the focus event of text input like this

onFocus={(event) => onFocus(event, index)}

Now when backspace is pressed, the focusedIndex will be properly defined.

I used -1 to indicate that no cell is focused, we can use any other number value here

Focus event triggers before the press event. We can optimize the state updates in this case

What alternative solutions did you explore? (Optional)

Block the drag selection on the magic code input

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~017414e8528c6cdd44

[melvin-bot]

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

[melvin-bot]

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

[CortneyOfstad]

@s77rt we have a couple of proposals for your review above 👍 TIA!

[Pujan92]

Proposal

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

App crashes when we press Backspace/Delete after dragging the mouse from input box

What is the root cause of that problem?

Once magic code has been fulfilled, we called blurMagicCodeInput which will set the focused index to undefined. It seems when we start dragging from the input box, it will execute the focus event of the text where we aren't setting the index which we are doing within onPress callback. As the dragged Text input is focused it will fire the onKeyPress when we press Backspace and that is causing the issue of calling the blur method on undefined.

App/src/components/MagicCodeInput.js

Lines 184 to 186 in 75e737b

	const onFocus = (event) => {
	event.preventDefault();
	};

App/src/components/MagicCodeInput.js

Lines 195 to 200 in 75e737b

	const onPress = (event, index) => {
	event.preventDefault();
	setInput('');
	setFocusedIndex(index);
	setEditIndex(index);
	};

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

We can remove onPress and shift its logic to onFocus method where we perform the below steps which will set the correct index when the drag starts with the input box. As onFocus will be responsible for setting the indexes(focus & edit), we can take advantage of it and remove the same indexes setting code parts from onKeyPress.

App/src/components/MagicCodeInput.js

Lines 197 to 199 in 75e737b

	setInput('');
	setFocusedIndex(index);
	setEditIndex(index);

[s77rt]

@tranvantoan-qn Thanks for the proposal. I don't think your RCA is correct.

After dragging, the editIndex will be undefined

This does not seem to be the case, dragging does not affect the editIndex.

[s77rt]

@huzaifa-99 Thanks for the proposal. Your RCA is not exactly right.

when we drag over the whole input (important: without focusing any cell) ... and on blur

This is a contradiction, if the blur event occurs then the input is actually focused. The root cause here seems to be that even the input is actually focused we don't update the MagicCodeInput with such info.

[tranvantoan-qn]

@s77rt

It actually does in this case - please note that even though we were dragging, in order to see the crash the text selection must not be shown (dragging but no text selection sounds weird, but it happened in this case 😄)

• See my video at 0:03 or 0:10 where I started holding the left-mouse button and dragging

CRASH.mp4

The issue won't happen if the text selection is showing like this:

[s77rt]

@Pujan92 Thanks for the proposal. Your RCA is the most correct here.

It seems when we start dragging from the input box, it will execute the focus event of the text but we aren't setting the index (focus index / edit index)

Your solution makes sense but I'm afraid it causes a regression (similar to an old bug):

1. Start typing magic code
2. Switch tab
3. Back to tab
4. Notice the position of the input did change

Kooha-2023-07-26-20-03-43.mp4

[tranvantoan-qn]

My question with his solution is: If the issue is caused by the onKeyPress event, why did the crash only happen with Backspace or Delete key? and as mentioned "Crash happens only when the text selection is not showing" - so what should be the value for focus index/edit index

[s77rt]

@tranvantoan-qn Thanks for the update, but I still don't think this is the root cause. The root cause is that when you try to drag, onMouseDown you will focus the input. But we don't take that focus event into account i.e. MagicCodeInput still thinks that no input is focused (focusedIndex is still undefined). Then the blur event will occur and MagicCodeInput will blur the input that it has but it does not have the correct values for focusedIndex and editIndex.

[s77rt]

@tranvantoan-qn

Crash only happens when text selection is not showing because in that case the text input won't be focused. You can verify and type document.activeElement in the console. Notice that the issue will only happen if the activeElement is the text input.

[huzaifa-99]

@s77rt Thank you for the review, how about we add this condition to the focus event

 if (focusedIndex == -1) {
            setInput('');
            setEditIndex(index)
            setFocusedIndex(index)
}

Updating my proposal....

[huzaifa-99]

@s77rt I have updated my proposal

[s77rt]

@huzaifa-99 How would that fix the root cause? We need to ensure the following:
As long as the input is focused i.e. as long as document.activeElement return a text input (from the magic input). The state focusedIndex should have the index of that input and not undefined.

The below image shows a scenario that should never exist

[tranvantoan-qn]

@s77rt Following your explanation, I saw that the activeElement is always pointed to the last field >> so it means editIndex should have that field's index?

[huzaifa-99]

The root cause here seems to be that even the input is actually focused we don't update the MagicCodeInput with such info.
@huzaifa-99 How would that fix the root cause? We need to ensure the following:
As long as the input is focused i.e. as long as document.activeElement return a text input (from the magic input). The state focusedIndex should have the index of that input and not undefined.

@s77rt whenever the input is focused, the onFocus is guaranteed to be triggered. When we start to drag over the inputs (important: without pressing on them), the input we drag from becomes focused ex: lets say we drag from the 5th cell, 5th cell will get focused, and this triggers the focus event. In my updated proposal, this is how the onFocus looks like, so whenever input gets focused we will check if no cell is focused i.e focusedIndex == -1 and set state accordingly

const onFocus = (event, index) => {
    event.preventDefault();

    if (focusedIndex == -1) {
        setInput('');
        setEditIndex(index)
        setFocusedIndex(index)
    }
};

In this demo, i drag from the last input and state updates correctly on focus

Screen.Recording.2023-07-27.at.12.47.46.AM.mp4

[Pujan92]

Your solution makes sense but I'm afraid it causes a regression (similar to an old bug):

1. Start typing magic code
2. Switch tab
3. Back to tab
4. Notice the position of the input did change

@s77rt I think it is bcoz on changing the text we are not actually focusing on those input boxes. For that, we do need to use inputRefs.current[updatedFocusedIndex].focus(); by replacing this code here.

App/src/components/MagicCodeInput.js

Lines 227 to 228 in ecfbafe

	setFocusedIndex(updatedFocusedIndex);
	setInput(value);

Screen.Recording.2023-07-27.at.2.16.45.AM.mp4

[s77rt]

@huzaifa-99 I don't see how this is different than @Pujan92's proposal. Also there is no reason to add the focusedIndex == -1 check.

[Pujan92]

@s77rt PR is ready for review!

[melvin-bot]

🎯 ⚡️ Woah @s77rt / @Pujan92, great job pushing this forwards! ⚡️

The pull request got merged within 3 working days of assignment, so this job is eligible for a 50% #urgency bonus 🎉

• when @Pujan92 got assigned: 2023-07-27 00:00:39 Z
• when the PR got merged: 2023-07-28 17:38:20 UTC

On to the next one 🚀

[CortneyOfstad]

I'm heading OoO until 8/14, so reassigning BZ to keep an eye on it while I'm out 👍

[melvin-bot]

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

[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.48-5 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Fix: App crashes after pressing Backspace/Delete key in Incorrect magic code page #23734

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

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

For reference, here are some details about the assignees on this issue:

• @Pujan92 requires payment offer (Contributor)
• @s77rt requires payment offer (Reviewer)

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:

• [@s77rt] The PR that introduced the bug has been identified. Link to the PR:
• [@s77rt] 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:
• [@s77rt] 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:
• [@s77rt] Determine if we should create a regression test for this bug.
• [@s77rt] 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.
• [@JmillsExpensify / @CortneyOfstad] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[s77rt]

• The PR that introduced the bug has been identified: feat: magic code input (passwordless login) #16076
• The offending PR has been commented on: feat: magic code input (passwordless login) #16076 (comment)
• A discussion in #expensify-bugs has been started: I don't think this is needed, the bug was introduced on a commit that was meant to follow another approach, seems like a one time bug.
• Determine if we should create a regression test for this bug: No

[CortneyOfstad]

I am back from OoO, so taking this back over. Thanks @s77rt for the review above!

[CortneyOfstad]

Payments have been made!

@tranvantoan-qn was paid the reporting amount of $250
@Pujan92 and @s77rt were both paid $1,500 (contributor and C+ fee, plus bonus due to the PR being merged within 1 day of assignment).

Closing!