Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HOLD for payment 2025-02-04]App Crash at Start of Session for New User – JavaScript Errors Observed #55357

Closed
1 of 8 tasks
m-natarajan opened this issue Jan 16, 2025 · 30 comments
Assignees
Labels
AutoAssignerNewDotQuality Used to assign quality issues to engineers Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Needs Reproduction Reproducible steps needed

Comments

@m-natarajan
Copy link

m-natarajan commented Jan 16, 2025

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:
Reproducible in staging?: Needs Reproduction
Reproducible in production?: Needs Reproduction
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?:
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: @puneetlath
Slack conversation (hyperlinked to channel name): quality

Action Performed:

  1. Go to expensify.com
  2. Choose 1-10 company
  3. Add email and sign in

Expected Result:

The app should initialize smoothly for new users without any crashes or critical errors.

Actual Result:

The app crashes at the start of the session

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

  • Android: Standalone
  • Android: HybridApp
  • Android: mWeb Chrome
  • iOS: Standalone
  • iOS: HybridApp
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence
[Onyx] Warning: Trying to apply "merge" with array type to non-array type in the key "nvp_private_blockedFromConcierge"
[Onyx] Warning: Trying to apply "merge" with array type to non-array type in the key "private_personalDetails"
"Error: 'report_153180287332610' key can't be changed to 'report_'. useOnyx() only supports dynamic keys if they are both collection member keys from the same collection e.g. from 'collection_id1' to 'collection_id2'.,Error: 'report_153180287332610' key can't be changed to 'report_'. useOnyx() only supports dynamic keys if they are both collection member keys from the same collection e.g. from 'collection_id1' to 'collection_id2'.\n at https://new.expensify.com/vendors-c5d98afe914055e55e41.bundle.js:2:2969363/n at rs (https://new.expensify.com/vendors-c5d98afe914055e55e41.bundle.js:2:2860051)/n at Bl (https://new.expensify.com/vendors-c5d98afe914055e55e41.bundle.js:2:2879996)/n at https://new.expensify.com/vendors-c5d98afe914055e55e41.bundle.js:2:2878773/n at yl (https://new.expensify.com/vendors-c5d98afe914055e55e41.bundle.js:2:2878833)/n at sl (https://new.expensify.com/vendors-c5d98afe914055e55e41.bundle.js:2:2872649)/n at Hi (https://new.expensify.com/vendo..."
"Error: 'report_153180287332610' key can't be changed to 'report_'. useOnyx() only supports dynamic keys if they are both collection member keys from the same collection e.g. from 'collection_id1' to 'collection_id2'.,Error: 'report_153180287332610' key can't be changed to 'report_'. useOnyx() only supports dynamic keys if they are both collection member keys from the same collection e.g. from 'collection_id1' to 'collection_id2'.\n at https://new.expensify.com/vendors-c5d98afe914055e55e41.bundle.js:2:2969363/n at rs (https://new.expensify.com/vendors-c5d98afe914055e55e41.bundle.js:2:2860051)/n at Bl (https://new.expensify.com/vendors-c5d98afe914055e55e41.bundle.js:2:2879996)/n at https://new.expensify.com/vendors-c5d98afe914055e55e41.bundle.js:2:2878773/n at yl (https://new.expensify.com/vendors-c5d98afe914055e55e41.bundle.js:2:2878833)/n at sl (https://new.expensify.com/vendors-c5d98afe914055e55e41.bundle.js:2:2872649)/n at Hi (https://new.expensify.com/vendo..."

Image

Full story Link

View all open jobs on GitHub

Issue OwnerCurrent Issue Owner: @thienlnam
@m-natarajan m-natarajan added AutoAssignerNewDotQuality Used to assign quality issues to engineers Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Needs Reproduction Reproducible steps needed labels Jan 16, 2025
Copy link

melvin-bot bot commented Jan 16, 2025

Triggered auto assignment to @thienlnam (AutoAssignerNewDotQuality)

Copy link

melvin-bot bot commented Jan 16, 2025

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

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Jan 16, 2025
@MelvinBot
Copy link

This has been labelled "Needs Reproduction". Follow the steps here: https://stackoverflowteams.com/c/expensify/questions/16989

@puneetlath
Copy link
Contributor

@mountiny @VickyStash and @fabioh8010 have been discussing the solution to this in Slack.

@mountiny
Copy link
Contributor

@VickyStash did you manage to find any spot where the report key might be wrong?

@VickyStash
Copy link
Contributor

@VickyStash did you manage to find any spot where the report key might be wrong?

Unfortunately, no luck.
I have some capacity today, so I can take another look. Maybe I'll be able to find the spot or at least several potential places where something could go wrong.

@VickyStash
Copy link
Contributor

I still haven't defined any specific spot, but here is the sum up of what we have as for now:

  1. The crash was caused by Error: 'report_153180287332610' key can't be changed to 'report_'. useOnyx() only supports dynamic keys if they are both collection member keys from the same collection e.g. from 'collection_id1' to 'collection_id2'. This is a standard error triggered inside useOnyx hook in case the key passed to the hook was dynamically changed from item to collection key. 153180287332610 - Concierge report ID.

  2. That means, that during data loading the valid repordID disappears. According to fullstory, it can be something received from GetMissingOnyxMessages. The thing is that even if we add a check to escape a crash due to the useOnyx error, we still potentially have invalid data.

  3. My flow in the app, even on the prod, is a little bit different than the reporter had. For example - after setup, the list of important tasks to help get your team’s expenses under control is posted inside Concierge chat for me. For the user, reported the bug it was inside #admins chat. Is it possible for me to have a similar setup as the user had?

@mountiny
Copy link
Contributor

For 3 - you need to use email without + in it. If you add + its going to concierge and not to admins as its testing email most likely (we might remove this fork, but its there now)

@melvin-bot melvin-bot bot added the Overdue label Jan 20, 2025
Copy link

melvin-bot bot commented Jan 20, 2025

@thienlnam, @michaelkwardrop Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@muttmuure muttmuure moved this to CRITICAL in [#whatsnext] #quality Jan 20, 2025
@VickyStash
Copy link
Contributor

For 3 - you need to use email without + in it. If you add + its going to concierge and not to admins as its testing email most likely (we might remove this fork, but its there now)

That truly helped to follow the correct flow, thank you! But still, I havent' found any issues during the testing.

Maybe we can take a step back, and try to know why empty array values were pushed to the user (nvp_private_blockedFromConcierge and private_personalDetails ) and why it's not when I do testing on my side.

Image

I see there was a discussion in the slack in November and even a fix PR was merged by you @mountiny, but it looks like it's still there, do you have an idea why?
Note: when I test from my end, for me it returns as {}, which is not causing any errors.
Maybe if we know the difference we will get a step closer to the solution.

It looks like for the user the crash was potentially caused by the results of GetMissingOnyxMessages, overall it's strange that it was triggered in the start of the session. @mountiny is there a chance to know what was returned to the user with this call?

@VickyStash
Copy link
Contributor

Today I've investigated all of the potential places that could lead to the crash on the app side in the mentioned flow.

I've defined two spots.

  1. ReportActionItemMessage

It calls useOnyx hook with reportID property.

const [report] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`);

reportID is passed from PureReportActionItem, where it's defined as

const reportID = report?.reportID ?? '';

Which potentially can cause the crash.

  1. ReportActionItemCreated

It calls useOnyx hook with reportID property.

const [report] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`);

reportID is passed from ReportActionItemContentCreated

to ReportActionItemContentCreated it's passed from PureReportActionItem, where the defaulting value is empty string

report: {...report, reportID: report?.reportID ?? ''},

Which potentially can cause the crash.


So both cases are related to the PureReportActionItem component, which defaults reportIDs to the empty strings and that can cause the crash.
But it should be updated as part of the defaulting IDs issue. @kubabutkiewicz has already started to work on this file!

@VickyStash
Copy link
Contributor

I'm going to be OOO Jan 22 - 26 ❄️

@mountiny
Copy link
Contributor

Thank you! The array was fixed in the flows at the time hut seems like its coming from somewhere else now, going to try to poke around to see if anyone can help us identify the flow this stems from. Its hard without reproduction steps

@kubabutkiewicz can you please prioritize fixing the places @VickyStash found that might cause this crash?

@kubabutkiewicz
Copy link
Contributor

@mountiny Yes sure, I am currently testing changes so PR will be available to check soon!

@michaelkwardrop
Copy link
Contributor

Let me know if I can do anything useful wrt reproduction, but it seems like y'all got it.

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Jan 21, 2025
Copy link

melvin-bot bot commented Jan 27, 2025

@thienlnam, @michaelkwardrop Eep! 4 days overdue now. Issues have feelings too...

@mountiny
Copy link
Contributor

@kubabutkiewicz how is it looking?

@VickyStash
Copy link
Contributor

The PR with PureReportActionItem component updates is prepared for the final review, it looks like it should be merged really soon.

@kubabutkiewicz
Copy link
Contributor

@mountiny @VickyStash the PR was just merged 😄

@muttmuure muttmuure moved this from CRITICAL to MEDIUM in [#whatsnext] #quality Jan 28, 2025
Copy link

melvin-bot bot commented Jan 29, 2025

@thienlnam, @michaelkwardrop Still overdue 6 days?! Let's take care of this!

@thienlnam
Copy link
Contributor

Should be on production, pending 7 day regression for C+ payment

@melvin-bot melvin-bot bot removed the Overdue label Jan 29, 2025
Copy link

melvin-bot bot commented Jan 30, 2025

@thienlnam @michaelkwardrop this issue was created 2 weeks ago. Are we close to a solution? Let's make sure we're treating this as a top priority. Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

@michaelkwardrop
Copy link
Contributor

@thienlnam who is the C+ that needs payment here?

@melvin-bot melvin-bot bot added the Overdue label Jan 31, 2025
@thienlnam
Copy link
Contributor

@paultsimura for reviewing this PR, doesn't look like the automation worked here so I'll update it manually

@melvin-bot melvin-bot bot removed the Overdue label Jan 31, 2025
@thienlnam thienlnam changed the title App Crash at Start of Session for New User – JavaScript Errors Observed [HOLD for payment 2025-02-04]App Crash at Start of Session for New User – JavaScript Errors Observed Jan 31, 2025
@melvin-bot melvin-bot bot added the Overdue label Feb 3, 2025
Copy link

melvin-bot bot commented Feb 4, 2025

Skipping the payment summary for this issue since all the assignees are employees or vendors. If this is incorrect, please manually add the payment summary SO.

Copy link

melvin-bot bot commented Feb 4, 2025

@thienlnam, @michaelkwardrop Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@paultsimura
Copy link
Contributor

Payment "summary": #55357 (comment)

@michaelkwardrop
Copy link
Contributor

Contributor+: @paultsimura  paid $250 via Upwork (https://www.upwork.com/nx/wm/offer/106011831)

@paultsimura do we need a regression test for this?

@paultsimura
Copy link
Contributor

Thanks @michaelkwardrop.
IMO we don't need a regression test here - the issue was caused by incorrect defaulting of the report IDs. We've removed the defaulting and now it's covered by an ESlint rule, so this shouldn't happen again.

@michaelkwardrop
Copy link
Contributor

Paid, contract completed, job posted removed! Closing this out ✅

@github-project-automation github-project-automation bot moved this from MEDIUM to Done in [#whatsnext] #quality Feb 5, 2025
@melvin-bot melvin-bot bot removed the Overdue label Feb 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AutoAssignerNewDotQuality Used to assign quality issues to engineers Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Needs Reproduction Reproducible steps needed
Projects
Status: Done
Development

No branches or pull requests

9 participants