[$250] WS - App crashes when changing workspace ID to invalid ID while distance rate RHP is open

[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!

---

Version Number: 1.4.63-0
Reproducible in staging?: y
Reproducible in production?: y
Issue found when executing PR: #36409
Issue reported by: Applause - Internal Team

Action Performed:

1. Navigate to staging.new.expensify.com
2. Create a workspace
3. Click on More features and turn on distance rates
4. Open Distance rates settings
5. Click on one distance rate to open the RHP
6. Click on the URL and change the workspace ID to an invalid set of strings

Expected Result:

Not found view renders

Actual Result:

App crashes

Workaround:

n/a

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

Bug6453708_1713450539309.bandicam_2024-04-18_17-21-36-157.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~012a487426fc875ae6
• Upwork Job ID: 1781338802987253760
• Last Price Increase: 2024-04-19
• Automatic offers:
• ishpaul777 | Reviewer | 0
• Nodebrute | Contributor | 0

[melvin-bot]

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

[kbecciv]

We think that this bug might be related to #wave-collect - Release 1

[kbecciv]

@bfitzexpensify FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors.

[Nodebrute]

Proposal

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

App crashes when changing workspace ID to invalid ID while distance rate RHP is open

What is the root cause of that problem?

We need to improve our usage of optional chaining in this file, as currently, many values are left undefined PolicyDistanceRateDetailsPage.tsx

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

We should do proper optional chaining in this file.
For example (Pseudocode)

const currency = rate?.currency ?? CONST.CURRENCY.USD;
const canDeleteRate = Object.values(customUnit?.rates ?? {}).filter((distanceRate) => distanceRate?.enabled).length > 1 || !rate?.enabled;
const canDisableRate = Object.values(customUnit?.rates ?? {}).filter((distanceRate) => distanceRate?.enabled).length > 1;

We should address and fix the issues present throughout the entirety of this file.

What alternative solutions did you explore? (Optional)

[ZhenjaHorbach]

Regression from this PR #38060

Proposal

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

WS - App crashes when changing workspace ID to invalid ID while distance rate RHP is open

What is the root cause of that problem?

The problem is due to the fact that during the creation of the PR, not all cases were taken into account

https://github.com/Expensify/App/pull/38237/files#diff-7e73657dfb914a3edb6449a7b19dfe303dd7c1c21e3cfe4e8c6318c0411575e0R46-R51

As a result, we have cases when we try to get a value from undefined

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

To fix this issue we can check new components and add optional chains wherever needed

In fact, looking at the new changes, this is not the only component that needs refactoring

We have a similar situation on PolicyDistanceRateEditPage for example

d9e986d#diff-c4dba2a501b3bb84d9f473b51674f7bcb32b5518cd80d1ca653bc56703560d79R43-R48

What alternative solutions did you explore? (Optional)

NA

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~012a487426fc875ae6

[melvin-bot]

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

[minhbac333studyus]

I see the same issue on Setting of Distance Rate Page ( crash instead NOT FOUND)

[melvin-bot]

📣 @minhbac333studyus! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

1. Make sure you've read and understood the contributing guidelines.
2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
   
   Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[minhbac333studyus]

Proposal Response
Problem Restatement
The core issue was with the handling of potentially undefined properties when accessing nested data structures in JavaScript. The application encountered errors when trying to access rates by rateID within a potentially undefined customUnit object, leading to crashes or unexpected behaviors when the URL parameters were manually altered to non-existent IDs.

Root Cause
The root cause was improper handling of optional chaining and default values in a deeply nested data structure. Before the fix, the code did not properly fall back to default values when intermediate objects (customUnit or rates) were undefined. This led to attempts to access properties on undefined, resulting in runtime errors.

Proposed Changes
The changes implemented involved using optional chaining (?.) and nullish coalescing (??) operators to safely access nested properties and ensure defaults are used when data is missing. This approach prevents errors by ensuring that every level of property access is checked for nullity before proceeding to the next level:

const rate = customUnit?.rates[rateID] ?? {};
const currency = rate.currency ?? CONST.CURRENCY.USD;
const canDeleteRate = Object.values(customUnit?.rates ?? {}).filter((distanceRate) => distanceRate.enabled).length > 1 || !rate.enabled;
const canDisableRate = Object.values(customUnit?.rates ?? {}).filter((distanceRate) => distanceRate.enabled).length > 1;

Alternatives Explored
Error Boundary Handling: Implementing error boundaries to catch and handle errors in specific parts of the component tree. This would prevent the entire app from crashing but wouldn't solve the root cause.

Screen.Recording.2024-04-22.at.12.36.05.AM.mov

[bfitzexpensify]

Proposals ready for review @ishpaul777

[ishpaul777]

Thanks for your proposals everyone, since all proposals suggests the same solution, i lean on choosing the first one,

Proposal from @Nodebrute Looks good to me! lets find and fix this for all of the WS setting pages.

🎀 👀 🎀 C+ reviewed

[melvin-bot]

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

[ZhenjaHorbach]

Thanks for your proposals everyone, since all proposals suggests the same solution, i lean on choosing the first one,

Proposal from @Nodebrute Looks good to me! lets find and fix this for all of the WS setting pages.

🎀 👀 🎀 C+ reviewed

Sorry
I don't mind your decision
But there was no mention in the proposal that this should be fixed elsewhere

Plus I mentioned PR where these changes came from
So that we can focus on the components that have been changed and fix all potential problems

[minhbac333studyus]

Contributor details
Your Expensify account email: minhbacdev@gmail.com
Upwork Profile Link: https://www.upwork.com/freelancers/~01bc62fec324269a7e

[melvin-bot]

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

[ishpaul777]

I chose the first proposal given the simplicity for the solution, i really appreciate that you go extra mile to find other places where issue is reproducible but thats the only thing differentiate you proposal that I didn't find enough to discard the first one, My evaluation could be wrong so i'll let @AndrewGable make final call on assignment 😄

[melvin-bot]

📣 @ishpaul777 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

[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 📖

[Nodebrute]

I'll raise a PR soon.

[ishpaul777]

@Nodebrute Can you please update when we can expect a PR to be ready for review 🧑‍💻

[Nodebrute]

@ishpaul777 Sorry for the delay. I'm working on it, and it will be ready in a few hours.

[Nodebrute]

@ishpaul777 Hey, the PR is ready for review.

[AndrewGable]

Seems like there is one more instance of this: #41293 (comment)

Can we add this as a follow up to this issue and make a new PR with the fix? Thanks!

[ishpaul777]

Hey @Nodebrute, Would you be able to raise a new PR?

[Nodebrute]

Hey @ishpaul777 do we have a regression?

[ishpaul777]

Sadly the issue was reintroduced by another PR

[sobitneupane]

We addressed the issue in #42502 PR

[bfitzexpensify]

OK, seems like if this is resolved via #42502, which is connected to #41594, then we can close this out.

[Nodebrute]

@bfitzexpensify Hey, the payment is due here. Also, these are two separate issues. This was #42502 created to deal with a regression caused in this issue #41594

[bfitzexpensify]

Gotcha - offers sent @Nodebrute @ishpaul777

[bfitzexpensify]

Payments complete.