[HOLD for payment 2023-10-23] [HOLD for payment 2023-10-23] [HOLD for payment 2023-10-20] [HOLD for payment 2023-10-20] [$500] iOS native - App crashes when going back on a Manual request #29114

trjExpensify · 2023-10-09T18:12:31Z

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: v1.3.79-3
Reproducible in staging?: Y
Reproducible in production?:
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: @MitchExpensify
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1696874025934959

Action Performed:

"+"
"Request money"
Enter manual amount
"Next"
Click a contact (Do not "split")
"<" Go back in the header beside "Manual"
"<" Go back in the header beside "Split"
"Next"

Expected Result:

App doesn't crash

Actual Result:

App crashes

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?

Screenshots/Videos

Android: Native

Android: mWeb Chrome

iOS: Native

RPReplay_Final1696873974.mov

iOS: mWeb Safari

MacOS: Chrome / Safari

MacOS: Desktop

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01426d1c07ab30e760
Upwork Job ID: 1711444561430691840
Last Price Increase: 2023-10-09
Automatic offers:

aimane-chnaif | Reviewer | 27141426
hungvu193 | Contributor | 27141428

melvin-bot · 2023-10-09T18:12:35Z

Current assignee @trjExpensify is eligible for the Bug assigner, not assigning anyone new.

melvin-bot · 2023-10-09T18:12:37Z

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 · 2023-10-09T18:12:37Z

Job added to Upwork: https://www.upwork.com/jobs/~01426d1c07ab30e760

melvin-bot · 2023-10-09T18:12:42Z

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

yh-0218 · 2023-10-09T20:12:34Z

Proposal

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

App crashes when going back on a Manual request

What is the root cause of that problem?

This error happen on all devices, not only native mobiles.
On native mobiles, app will be crashed.
On chrome, only console error.
The root cause of this error is same screen is appeared when click Next Button.
When click Next Button, we must move to Split screen from Manual Amount Input Screen

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

App/src/libs/actions/IOU.js

Lines 2274 to 2276 in 8d42ad3

    
           if (shouldReset) { 
        
               resetMoneyRequestInfo(moneyRequestID); 
        
           }

This function reset all request info and go to input screen again.
We need to call secondResetMoneyRequestInfo instead of resetMoneyRequestInfo

function secondResetMoneyRequestInfo(id = '') {
    Onyx.merge(ONYXKEYS.IOU, {
        id,
        billable: null,
    });
}

Result

Screen.Recording.2023-10-09.at.11.10.49.PM.mov

Screen.Recording.2023-10-09.at.11.11.49.PM.mov

What alternative solutions did you explore? (Optional)

Vadym-33 · 2023-10-09T20:33:20Z

Proposal

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

App crashes when going back on a Manual request

What is the root cause of that problem?

App/src/components/OptionsSelector/BaseOptionsSelector.js

Lines 73 to 75 in 8d42ad3

    
           setTimeout(() => { 
        
               this.textInput.focus(); 
        
           }, CONST.ANIMATED_TRANSITION);

Because of calling focus of null.

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

We can add check logic there and this works very well on my side.

setTimeout(() => {
        if (!this.textInput) {
                return;
        }
         this.textInput.focus();
 }, CONST.ANIMATED_TRANSITION);

What alternative solutions did you explore? (Optional)

mountiny · 2023-10-09T23:20:35Z

@aimane-chnaif lets prioritize this one as its important flow in coming 2 weeks. thanks!

mkhutornyi · 2023-10-10T03:37:57Z

Proposal

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

There are 2 issues here

When click Next on amount input screen, participants selector screen shows briefly and disappears. Instead amount input screen reappears again. - [$500] Request Money - Request money screen opens again on click of Next button #28129
App crashes during this transition

As first bug is dupe of #28129, I will provide root cause & solution only for 2nd bug here

What is the root cause of that problem?

Participants selector screen uses BaseOptionsSelector component.
It uses setTimeout to autofocus input. But if component is unmounted quickly before 300ms, this.textInput becomes undefined and it tries to call undefined.focus() which makes crash.
This happens because there's no clearTimeout call on component unmount.

App/src/components/OptionsSelector/BaseOptionsSelector.js

Lines 73 to 76 in d916eff

    
               setTimeout(() => { 
        
                   this.textInput.focus(); 
        
               }, CONST.ANIMATED_TRANSITION); 
        
           }

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

We already have clear logic here

App/src/components/OptionsSelector/BaseOptionsSelector.js

Lines 136 to 139 in d916eff

    
           componentWillUnmount() { 
        
               if (this.focusTimeout) { 
        
                   clearTimeout(this.focusTimeout); 
        
               }

this.focusTimeout definition was somehow missing so we can add it back:

     this.focusTimeout = setTimeout(() => { 
         this.textInput.focus(); 
     }, CONST.ANIMATED_TRANSITION)

aimane-chnaif · 2023-10-10T04:13:29Z

I can confirm that navigation bug was unintentionally fixed in #27936.
So we can just fix crash here.
@mkhutornyi's proposal looks good to me.
🎀 👀 🎀 C+ reviewed

melvin-bot · 2023-10-10T04:13:35Z

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

yh-0218 · 2023-10-10T05:02:20Z

@aimane-chnaif I think that this crash will not happened if that navigation is fixed.

aimane-chnaif · 2023-10-10T05:17:01Z

@yh-0218 I faced this many times on the pages where options list is used. We should fix that bug

Here's another reproducible step: click Search icon and Esc key at the same time

Screen.Recording.2023-10-10.at.6.14.54.AM.mov

hungvu193 · 2023-10-10T08:01:08Z

This is dupe of: #28318

cc @fedirjh

aimane-chnaif · 2023-10-10T08:58:08Z

@hungvu193 no, it's different bug

hungvu193 · 2023-10-10T08:59:10Z

Actually both of it crashed because of the timeout wasn't cleared. I also pointed it out in comment here:
#28318 (comment)

aimane-chnaif · 2023-10-10T09:02:00Z

Seems like the main purpose of that GH is different from this one - #28318 (comment)
Also what @fedirjh found is different crash - #28318 (comment)

hungvu193 · 2023-10-10T09:07:00Z

It was first created to resolve the console error (which because of the timeout), however while discussing it, @iwiznia pointed out that, we also need to display the NotFoundView instead of do nothing while clicking the deeplink., so I updated the proposal to cover 2 cases.

The console error (which is also this issue)
The NotFoundView when user clicked the link.

So the issue (#28318) can cover this one. Anyway I think let hear other thoughts from @iwiznia and @fedirjh

trjExpensify · 2023-10-10T22:56:13Z

👋 @iwiznia & @fedirjh we haven't really got time to dally on this issue if it's not the same root cause. It's a crash in the middle of the request money flow, and it's going live to TU on Tuesday. Can you check the above, please? In the meantime, @AndrewGable can you put your thoughts on this so we're ready to move forward if we need to.

fedirjh · 2023-10-11T00:28:30Z

I faced this many times on the pages where options list is used. We should fix that bug

@aimane-chnaif It's the same bug initially reported in #28318

@trjExpensify I agree with @hungvu193 that this has the same root cause as #28318 , Also this solution is the same as what @hungvu193 has suggested in #28318 (comment)

aimane-chnaif · 2023-10-11T04:54:54Z

This is what I suggest as this seems like urgent, #28318 is non-urgent. No need to waste time arguing because of issue conflict.

If #28318 will be fixed quickly (in a day or so), go ahead and close this one.
If not, we can do quick fix here for the crash. And implement not found logic separately on #28318.

I just found another crash case on split bill.

trjExpensify · 2023-10-11T11:06:29Z

Yeah, I think we need to get a PR up to fix this crash in the request money flow today, especially if it's also in the split bill case.

I like the idea of using this issue to fix the crash with the above solution, and #28318 to introduce this "no page found" blocking form in the scenario being discussed there.

@mountiny do you agree? We need to resolve this pretty urgently.

mountiny · 2023-10-11T14:02:47Z

Coming from Slack thread here, going to assing @hungvu193 as their solution was proposed earlier albeit on different issue. it was related.

melvin-bot · 2023-10-16T21:43:52Z

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

fix crash on manual request #29326

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

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:

@hungvu193 requires payment offer (Contributor)
@aimane-chnaif 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 · 2023-10-23T12:29:02Z

@AndrewGable, @trjExpensify, @hungvu193, @aimane-chnaif Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

trjExpensify · 2023-10-23T15:37:17Z

Can we get the checklist filled out please, @aimane-chnaif? Also, not sure why 4 checklists got triggered. 😅

aimane-chnaif · 2023-10-23T15:42:54Z

The PR that introduced the bug has been identified. Link to the PR: Simplify Global Create menu #25564
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: https://github.com/Expensify/App/pull/25564/files#r1368889880
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: N/A
Determine if we should create a regression test for this bug.
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.

This is already added in guideline - https://github.com/Expensify/App/blob/main/contributingGuides/STYLE.md#how-do-i-auto-focus-a-textinput-using-usefocuseffect
If everyone follows this pattern correctly, this bug won't happen.

aimane-chnaif · 2023-10-23T15:43:28Z

Also, not sure why 4 checklists got triggered. 😅

https://expensify.slack.com/archives/C01GTK53T8Q/p1698059102119519 😄

trjExpensify · 2023-10-23T16:06:07Z

Okay, so confirming payments as follows:

$750 to @hungvu193 for the fix
$750 to @aimane-chnaif for C+

#urgency bonus applied as per here.

trjExpensify · 2023-10-23T16:08:30Z

Settled up!

trjExpensify added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Oct 9, 2023

trjExpensify self-assigned this Oct 9, 2023

melvin-bot bot changed the title ~~iOS native - App crashes when going back on a Manual request~~ [$500] iOS native - App crashes when going back on a Manual request Oct 9, 2023

melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Oct 9, 2023

melvin-bot bot assigned aimane-chnaif Oct 9, 2023

melvin-bot bot assigned AndrewGable Oct 10, 2023

melvin-bot bot removed the Reviewing Has a PR in review label Oct 13, 2023

This comment was marked as duplicate.

Sign in to view

melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Oct 13, 2023

melvin-bot bot changed the title ~~[HOLD for payment 2023-10-20] [$500] iOS native - App crashes when going back on a Manual request~~ [HOLD for payment 2023-10-20] [HOLD for payment 2023-10-20] [$500] iOS native - App crashes when going back on a Manual request Oct 13, 2023

This comment was marked as duplicate.

Sign in to view

melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Oct 16, 2023

This comment was marked as duplicate.

Sign in to view

melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Oct 16, 2023

This comment was marked as duplicate.

Sign in to view

melvin-bot bot added Daily KSv2 Overdue and removed Weekly KSv2 labels Oct 20, 2023

melvin-bot bot removed the Overdue label Oct 23, 2023

aimane-chnaif mentioned this issue Oct 23, 2023

Simplify Global Create menu #25564

Merged

58 tasks

trjExpensify closed this as completed Oct 23, 2023

[HOLD for payment 2023-10-23] [HOLD for payment 2023-10-23] [HOLD for payment 2023-10-20] [HOLD for payment 2023-10-20] [$500] iOS native - App crashes when going back on a Manual request #29114

[HOLD for payment 2023-10-23] [HOLD for payment 2023-10-23] [HOLD for payment 2023-10-20] [HOLD for payment 2023-10-20] [$500] iOS native - App crashes when going back on a Manual request #29114

Comments

trjExpensify commented Oct 9, 2023 • edited by melvin-bot bot Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

melvin-bot bot commented Oct 9, 2023

melvin-bot bot commented Oct 9, 2023 • edited by trjExpensify Loading

Bug0 Triage Checklist (Main S/O)

melvin-bot bot commented Oct 9, 2023

melvin-bot bot commented Oct 9, 2023

yh-0218 commented Oct 9, 2023

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

Vadym-33 commented Oct 9, 2023

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

mountiny commented Oct 9, 2023

mkhutornyi commented Oct 10, 2023

Proposal

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

What is the root cause of that problem?

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

aimane-chnaif commented Oct 10, 2023

melvin-bot bot commented Oct 10, 2023

yh-0218 commented Oct 10, 2023

aimane-chnaif commented Oct 10, 2023

hungvu193 commented Oct 10, 2023

aimane-chnaif commented Oct 10, 2023

hungvu193 commented Oct 10, 2023

aimane-chnaif commented Oct 10, 2023

hungvu193 commented Oct 10, 2023

trjExpensify commented Oct 10, 2023

fedirjh commented Oct 11, 2023

aimane-chnaif commented Oct 11, 2023 • edited Loading

trjExpensify commented Oct 11, 2023

mountiny commented Oct 11, 2023

This comment was marked as duplicate.

This comment was marked as duplicate.

This comment was marked as duplicate.

This comment was marked as duplicate.

This comment was marked as duplicate.

This comment was marked as duplicate.

melvin-bot bot commented Oct 16, 2023 • edited by trjExpensify Loading

This comment was marked as duplicate.

melvin-bot bot commented Oct 23, 2023

trjExpensify commented Oct 23, 2023

aimane-chnaif commented Oct 23, 2023

aimane-chnaif commented Oct 23, 2023 • edited Loading

trjExpensify commented Oct 23, 2023

trjExpensify commented Oct 23, 2023

trjExpensify commented Oct 9, 2023 •

edited by melvin-bot bot

Loading

melvin-bot bot commented Oct 9, 2023 •

edited by trjExpensify

Loading

aimane-chnaif commented Oct 11, 2023 •

edited

Loading

melvin-bot bot commented Oct 16, 2023 •

edited by trjExpensify

Loading

aimane-chnaif commented Oct 23, 2023 •

edited

Loading