Closed kavimuru closed 1 year ago
Triggered auto assignment to @trjExpensify (Bug
), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
Platforms
in OP are β
)App crashes on login after opening split bill details page
When we open split bill page by deep link, we use HOC withReportOrNotFound
to display loading page if openApp
API is calling
But after this API is complete, props.reportActions
still is empty because it must wait openReport
API is complete. That makes reportAction
is empty and the app crashes when we get participantAccountIDs
from reportAction
https://github.com/Expensify/App/blob/031b4a3a0a64a1731a84ed5dbba172a7fae536dd/src/pages/iou/SplitBillDetailsPage.js#L66-L70
We should display loading page until both report data and report action data are loaded completely
Remove the HOC withReportOrNotFound
, subscribe report
and isLoadingReportData
from Onyx
We should add the check to return loading page before this line
const isLoadingInitialReport = props.isLoadingReportData && _.isEmpty(props.report);
const isLoadingInitialReportActions = _.isEmpty(props.reportActions) || (props.report.isLoadingReportActions && _.isEmpty(reportAction);
const isExistReport = isLoadingInitialReport || (!_.isEmpty(props.report) && props.report.reportID);
const shouldShowLoading = (isLoadingInitialReport || isLoadingInitialReportActions) && isExistReport;
if (shouldShowLoading) {
return <FullscreenLoadingIndicator />;
}
if (_.isEmpty(reportAction) || _.isEmpty(props.report)) {
return (
<FullPageNotFoundView shouldShow/>
)
}
NA
https://github.com/Expensify/App/assets/129500732/62afaf4c-14a7-403d-93aa-ee5ffc83763a
π @dukenv0307 how related is this to this issue? CC: @aldo-expensify
@trjExpensify This issue and https://github.com/Expensify/App/issues/21268 are different issue because the root cause is different.
If the root cause @dukenv0307 explained here is correct, they do appear unrelated to me.
Cool, will move on external
for now! :)
Job added to Upwork: https://www.upwork.com/jobs/~0168304d5ea41d503d
Current assignee @trjExpensify is eligible for the External assigner, not assigning anyone new.
Triggered auto assignment to Contributor-plus team member for initial proposal review - @mananjadhav (External
)
Root cause of the problem: The root cause of the app crash needs to be investigated further to provide a definitive answer. It could be due to a variety of reasons, such as a bug in the React Native code, compatibility issues between different platforms, memory management problems, or even server-related issues.
Changes to solve the problem: To address the app crash issue, the following steps can be taken:
Bug Investigation: Start by thoroughly investigating the crash logs and error messages reported by the app. Identify patterns or common scenarios that lead to the crash. This will help in pinpointing the cause of the issue.
Reproduce the Bug: Try to reproduce the crash on different devices and platforms (iOS and Android) using the same version of the app. This step is essential to ensure the bug is replicable and consistent across devices.
Inspect Split Bill Details Page: Examine the Split Bill Details page to check if there are any specific elements or interactions triggering the crash. Review the React Native code associated with this page to identify potential issues.
Memory Management: Analyze the memory usage of the app during the login process after opening the Split Bill Details page. Check for any memory leaks or excessive memory usage that could lead to crashes, especially if the user navigates between different pages frequently.
Unit Testing: Write unit tests to cover the critical parts of the code related to the login process and the Split Bill Details page. These tests will help catch regressions and ensure the stability of the app.
React Native Version and Dependencies: Ensure that the React Native version used is stable and compatible with the required dependencies. Consider upgrading to the latest stable version if it addresses any known issues.
Error Handling: Implement proper error handling mechanisms throughout the app to gracefully handle unexpected situations and prevent crashes.
Platform-Specific Optimization: If the crash is specific to a particular platform (iOS or Android), perform platform-specific optimizations and debugging to resolve the issue.
Beta Testing: Once the proposed changes are implemented, conduct beta testing with a group of users to validate the fix and gather feedback.
Remember that the actual solution will require a more detailed analysis of the codebase and the crash reports. The proposal should outline the general approach and steps to tackle the problem effectively.
Thanks for the proposal @dukenv0307. Your RCA is correct, but I have a few questions on the proposal.
withReportOrNotFound
page?We should add the check to return loading page before this line
which line?
Can we simplify the proposal because the isLoadingReportingData, etc. checks are already in the withReportOrNotFound
component?
@mananjadhav
Why do we want to remove withReportOrNotFound page?
We should remove this HOC because we should show loading until both report and report action data are loaded completely.
which line?
This line we should add the condition after this line and also return NotFoundPage here because if reportAction
make the get data after this line crash app
https://github.com/Expensify/App/blob/031b4a3a0a64a1731a84ed5dbba172a7fae536dd/src/pages/iou/SplitBillDetailsPage.js#L66
Can we simplify the proposal because the isLoadingReportingData, etc. checks are already in the withReportOrNotFound component?
This HOC is used in other places and this check in this isn't enough because report data is loaded in openApp
API, reportAction data is loaded in openReport
API, and isLoadingReportingData
will be false
after openApp
API complete
Thanks for the response, @dukenv0307. I meant why not just add checks for reportAction
. Why remove the withReportOrNotFound
hoc?
@mananjadhav If we add the check for reportAction
and doesn't remove HOC, page is flicker when loading because we show loading from HOC and then loading in the page.
I think we can go ahead with your proposal @dukenv0307. You've got the RCA as well as the suggestion. But I have two options of the implementation here.
report
checks code to the component and add reportAction
checksreportAction
checks, without touching much of the withReportOrNotFound
.I am going to confirm this from an internal engineer (@Li357) too.
π π π C+ reviewed.
Triggered auto assignment to @Li357, see https://stackoverflow.com/c/expensify/questions/7972 for more details.
I think we can create a new HOC for this because flag page also need this.
Yeah I thought so. Let's wait for @Li357 to respond. Thanks for the patience here.
β οΈ Looks like this issue was linked to a Deploy Blocker here
If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.
If a regression has occurred and you are the assigned CM follow the instructions here.
If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.
I agree, a new HOC would be great. I imagine future components that depend on a specific action being loaded would benefit from a central place of the logic.
π£ @mananjadhav Please request via NewDot manual requests for the Reviewer role ($1000)
π£ @dukenv0307 π 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 π
π£ @dhanashree-sawant π An offer has been automatically sent to your Upwork account for the Reporter role π Thanks for contributing to the Expensify app!
@dukenv0307 I see you've linked a PR, but I don't see the how its related to the current issue.
@mananjadhav We have a problem here that is in mWeb when we open this page by deep link this will display not found page because in mWeb openReport
doesn't call until we open this report.
@mananjadhav Open the PR to see more detail.
@mananjadhav Update: I updated HOC to resolve the problem above, please help to review the PR when you have a chance.
π― β‘οΈ Woah @mananjadhav / @dukenv0307, 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 π
On to the next one π
@trjExpensify This is ready for payout on 08/15 but the title wasn't updated.
Also I couldn't pinpoint to a specific PR causing this. I think we should add a regression test for this one as it is was crashing the app and related to IOU. The test steps from the PR are good enough for the regression test.
@Li357 wdyt?
Updated the title! :)
I think we should add a regression test for this one as it is was crashing the app and related to IOU. The test steps from the PR are good enough for the regression test.
Yeah cool, I'll ask Applause about what they think is best re: opening from deep links. We should do the same with request money as well.
Makes sense.
Okay, confirming payments as follows:
Iβve raised my request on NewDot.
Reviewed the details for @mananjadhav. Approved for payment in NewDot based on BZ summary above.
Dope, we're all set then. Closing!
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:
Expected Result:
App should not crash when we open split bill details page
Actual Result:
App crashes when we logout and login with split bill details page
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?
Version Number: 1.3.45-2 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
https://github.com/Expensify/App/assets/43996225/95e78c47-369a-497a-947f-5c939a61883d
https://github.com/Expensify/App/assets/43996225/90347da8-14b0-4525-962d-9139896d9876
Expensify/Expensify Issue URL: Issue reported by: @dhanashree-sawant Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1690262928105189
View all open jobs on GitHub
Upwork Automation - Do Not Edit