[HOLD for Payment 2024-09-06] [$250] mWeb - Submit expense - Site crashes sending a distance expense after two scan expenses

[lanitochka17]

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: 9.0.13-0
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4765667
Email or phone of affected tester (no customers): nahummelaku9+d27@gmail.com
Issue reported by: Applause - Internal Team

Action Performed:

1. Login to staging.new.expensify.com
2. Navigate to FAB > Submit expense > Scan
3. Upload a receipt and click on next
4. Choose a workspace and submit it
5. Navigate to FAB>QAB(Scan IOU)
6. Upload a receipt and submit it to the same workspace
7. Navigate to FAB>Submit expense > Distance
8. Enter start and finish locations and click next
9. Select the same workspace you submitted the scan expenses to and click submit

Expected Result:

Distance expense is created and navigated to the conversation

Actual Result:

The site crashes with a message "Uh-oh, something went wrong!"

Workaround:

Unknown

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

Add any screenshot/video evidence

Bug6552463_1721899827253.video_2024-07-25_12-17-52.mp4

logs (2).txt

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01890de5ca6ea4b22c
• Upwork Job ID: 1818469788247083194
• Last Price Increase: 2024-08-07
• Automatic offers:
• ikevin127 | Reviewer | 103438207
• ShridharGoel | Contributor | 103438209

Issue Owner

Current Issue Owner: @ikevin127

[melvin-bot]

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

[ShridharGoel]

Proposal

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

Site crashes when distance map calls fitBounds with high padding on low screen size.

What is the root cause of that problem?

This happens when fitBounds is called with high padding, but the screen size is small.

App/src/components/MapView/MapView.website.tsx

Line 148 in 2583e96

map.fitBounds([northEast, southWest], {padding: mapPadding});

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

We can make the mapPadding dependent on the screen size.

In usages of DistanceMapView, update mapPadding to below:

mapPadding={getIsNarrowLayout ? 0 : CONST.MAP_PADDING}

This is used in ConfirmedRoute and DistanceRequestFooter.

Else, we can put the getIsNarrowLayout check while calling fitBounds.

[bernhardoj]

Proposal

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

The app crash when adding a distance request after 2 scan request.

What is the root cause of that problem?

When we have 2 scan expense + 1 distance, the size of the preview becomes small.

Then, we set a padding of 50 to the map which will be used when trying to center the map.

App/src/components/ConfirmedRoute.tsx

Line 111 in 5e65276

mapPadding={CONST.MAP_PADDING}

App/src/components/MapView/MapView.website.tsx

Line 148 in 5e65276

map.fitBounds([northEast, southWest], {padding: mapPadding});

However, the size of the map is smaller than the padding combined (left+right / top+bottom).

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

Change the padding to the smallest size (or 0 or decrease the value) if it's bigger than the map size.

let newMapPadding = mapPadding;
if (mapPadding && mapPadding > 0 && (mapPadding * 2 > mapRef.getCanvas().clientHeight || mapPadding * 2 > mapRef.getCanvas().clientWidth)) {
    newMapPadding = Math.min(mapRef.getCanvas().clientHeight, mapRef.getCanvas().clientWidth) / 2; // or 0
}
map.fitBounds([northEast, southWest], {padding: newMapPadding});

[melvin-bot]

@greg-schroeder Whoops! This issue is 2 days overdue. Let's get this updated quick!

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01890de5ca6ea4b22c

[melvin-bot]

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

[ikevin127]

♻️ Reviewing proposals today.

[ikevin127]

@ShridharGoel's proposal looks good to me. They were first to identify the root cause and the proposed solution fixes the issue. However, there's some important context to understand here before moving forward with a fix - and I think @Expensify/design might want to weigh in here regarding the best choice 👇

Context

Notes:

• I reproduced / tested this on MacOS Chrome using Toggle devices toolbar which allowed me to test on different phones which have different screen sizes (width / height).
• On narrow layout screens I noticed that the site crash happens around the padding value 40-42 and over, depending on device screen size.
• I tested from Samsung Galaxy S8+ (360x740) to iPhone 4 (320x480) to iPhone 5 (320 x 568) and the best looking value that doesn't crash the app even on iPhone 4 is 20 Padding.

Here's how the map preview looks on narrow layouts with different values vs on large layouts:

0 Padding	20 Padding	40 Padding (crashes on smaller devices)	50 Padding (Large Layout)

As one can see, the lower the padding, the more zoomed-in the map preview is and vice-versa.
This applies on the individual distance request preview as well:

20 Padding (Narrow Layout)

Given the above ^ context, I think that going with @ShridharGoel's proposal is the right path, with some slight changes:

• like we have CONST.MAP_PADDING for large layout devices which is currently used for narrow layouts as well (causing the crash), instead of having 0 Padding for narrow layouts, have a new CONST variable with 20 Padding value, something like CONST.MAP_PADDING_NARROW (or similar)
• use CONST.MAP_PADDING_NARROW if getIsNarrowLayout() is true

Important

We should wait for design's team input here before moving forward.

🎀👀🎀 C+ reviewed

[melvin-bot]

Current assignee @marcaaron is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[ikevin127]

⚠️ @bernhardoj I tested your proposed solution on the following narrow layout device: Samsung Galaxy S8+ (360x740) and the calculated newMapPadding was 42 which was still crashing the app. Check out my previous comment for more context on how this padding affects different screen sizes which still crash the app if the padding is over a certain value (which differs based on device screen size).

[ShridharGoel]

@shawnborton Can you have a look? #46296 (comment)

[shawnborton]

Oh wow, great investigation! So just to be clear, what are you recommending we use for the paddings for narrow screens vs wide screens? Personally I don't think there needs to be a huge difference, given that the size of the map preview is pretty much the same on mobile vs desktop.

Also, I think this "more zoomed in" screenshot looks good IMO, but would love to see it with a route that is horizontal too.

cc @dubielzyk-expensify (skipping Danny ping while he's 🌴) for vis

[ShridharGoel]

@shawnborton For wide screens, we can keep the existing padding of 50.

For narrow screens, either we can just go to 0, or we can set it to 20 based on @ikevin127's testing.

[shawnborton]

Can you show me the side-by-side difference though? The receipt thumbnail is virtually the same size on both wide and small screens, so I'm not sure if we actually want a difference here.

[ikevin127]

I stand by my previous #46296 (comment) and considering the context from that comment:

Note

Based on the results below I think we should keep 50 Padding on wide screens layout and adjust to 30 Padding only for narrow screens layout - this would ensure things look good on narrow layout devices and also keep things within scope as the main thing here is to ensure the app doesn't crash.

Here's the side by side in different scenarios:

Note:

1. I left out the ones where there's no difference between 20 vs 50 Padding (longer routes).
2. Keep in mind the context shown regarding the shorter vertical route from my comment above.

1. 20 vs 50 Padding - Wide screen layout on longer horizontal / vertical routes

Caution

This is why I think we shouldn't touch the wide screen layout value of 50 Padding.

Flow: Submit expense (Waypoints) Screen

20 Padding - Horizontal Route	50 Padding - Horizontal Route

Flow: Confirm Distance expense Screen

20 Padding - Horizontal Route	50 Padding - Horizontal Route	20 Padding - Vertical Route	50 Padding - Vertical Route

2. 20 vs 50 Padding - Narrow screeen layout on longer horizontal / vertical routes

Caution

Here we have to decrease the padding because otherwise the app will crash.

Flow: Submit expense (Waypoints) Screen

20 Padding - Horizontal Route	50 Padding - Horizontal Route	20 Padding - Vertical Route	50 Padding - Vertical Route

Flow: Confirm Distance expense Screen

20 Padding - Horizontal Route	50 Padding - Horizontal Route	20 Padding - Vertical Route	50 Padding - Vertical Route

@ShridharGoel Given the results on narrow layout, I think the best option would be to go with 30 Padding which is closer to 50 but not close enough to crash the app. I tested with 30 Padding on the smaller device: iPhone 4 (320x480) and it doesn't crash. @shawnborton Wdyt ?

[melvin-bot]

@greg-schroeder, @marcaaron, @ikevin127 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[ikevin127]

Awaiting input from the design team on how we want the UI to look, then will be able to move on with opening a PR.

[izarutskaya]

This issue reproducible also after third distance request. Reproduce in production
Let me know please if need to create a new GH. Thanks

Screenrecorder-2024-08-13-12-28-01-781_compress_1.mp4

[ShridharGoel]

Will be opening PR soon. Don't have Mac access since few days, so setting up WSL.

[greg-schroeder]

Thanks for the update @ShridharGoel

[ikevin127]

Not overdue, currently awaiting for contributor to open PR based on #46296 (comment).

[melvin-bot]

@greg-schroeder, @marcaaron, @ShridharGoel, @ikevin127 Eep! 4 days overdue now. Issues have feelings too...

[ikevin127]

@ShridharGoel Please let us know if you have the capacity to open the PR or if we should move forward with another Contributor for this one. Thanks!

[ShridharGoel]

Draft PR is already up since few days, just waiting for adhoc builds.

[ShridharGoel]

@ikevin127 Maybe you can start the testing from your side till the adhoc builds are available. I need the adhoc builds since I'm unable to build locally.

[greg-schroeder]

This on staging. Awaiting deploy to prod

[ikevin127]

⚠️ We just got confirmation on Slack that the Deploy Checklist: New Expensify 2024-08-26 which includes the PR of this issue was only deployed to production today in Deploy Checklist: New Expensify 2024-08-28. More context on why this happened can be found in this Slack thread and this Slack comment.

Given the context above, this issue should be on Payment 2024-09-06 according to 4 days ago's production deploy from Deploy Checklist: New Expensify 2024-08-28.

cc @greg-schroeder @marcaaron

[greg-schroeder]

Updated

[ikevin127]

• [@ikevin127] The PR that introduced the bug has been identified. Link to the PR: Modify MoneyRequestConfirmPage for Distance Request #25707
• [@ikevin127] 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/25707/files#r1744654059
• [@ikevin127] 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.
• [@ikevin127] Determine if we should create a regression test for this bug.
• [@ikevin127] 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.

Regression Test Proposal

1. Login to staging.new.expensify.com .
2. Navigate to FAB > Submit expense > Scan.
3. Upload a receipt and click on Next.
4. Choose a workspace and submit it.
5. Navigate to FAB > QAB (Scan IOU).
6. Upload a receipt and submit it to the same workspace.
7. Navigate to FAB > Submit expense > Distance.
8. Enter start and finish locations and click Next.
9. Select the same workspace you submitted the scan expenses to and click Submit.
10. Test on different screen sizes / heights (iPhone 4 / 5, Samsung Galaxy S8, etc) and verify that the app doesn't crash when submitting the 3rd Distance request.

Do we agree 👍 or 👎.

[ikevin127]

cc @greg-schroeder

[greg-schroeder]

Sorry, processing

[greg-schroeder]

Paid @ikevin127 , sent new offer to @ShridharGoel. Will pay ASAP once accepted