Fix crashes when personal details are not available #9385

aldo-expensify · 2022-06-09T22:37:49Z

Details

This is fixing two cases where we can end up with a crash after tapping a user's avatar to see the personal details:

Problem 1: Only Android/IOS - tapping the avatar after opening a conversation with an account that doesn't exist

Bug and solution described here: #9182 (comment)

Problem 2: You tapped too fast and the request PersonalDetails_GetForEmails has not finished yet

When we select a new contact from the search panel, the following API requests happen:

CreateChatReport to create the chat
PersonalDetails_GetForEmails to get the personal details

If the user clicks the avatar in the conversation that opens the personal details panels before PersonalDetails_GetForEmails finishes, the app crashes.

Note on offline behaviour: You cannot create a new chat while offline, so I think there is nothing to test when offline

Fixed Issues

$ #9182

#9182 (comment)

Tests

Problem 1: Only Android/IOS

Using the native app in Android or IOS
Open the contact search panel
Search for an account that doesn't exist
Tap the account to create a new conversation
Tap on the avatar of the other user to open the personal details tab
Personal details tab should open just fine (you may see the loading indicator for a short moment if the request PersonalDetails_GetForEmails has not finished yet)

Problem 2: You tapped too fast and the request PersonalDetails_GetForEmails has not finished yet

Open the contact search panel
Search for an account you haven't messaged (it can be an account that doesn't exist)
Click the account to open the conversation
Very quickly click the avatar of the other user (can be in the center or in the bar on top) to open the personal details panel
If the personal details have not loaded yet, it should show an loading indicator and not crash the App

If you have the PHP Api running locally, it is very easy to reproduce by just adding a sleep(10); in the part of the API handling the command PersonalDetails_GetForEmails

Another way of making it easier to reproduce is using chrome tools > network > throttle

QA

Same steps as Tests, and you can try using chrome tools > network > throttle to make it easier to click the avatar before the request has finished.

Verify that no errors appear in the JS console

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Verify that no errors appear in the JS console

Screenshots

Web

Mobile Web

Simulator.Screen.Recording.-.iPhone.13.-.2022-06-10.at.11.02.39.mp4

Desktop

Screen.Recording.2022-06-10.at.11.00.09.AM.mov

iOS

Simulator.Screen.Recording.-.iPhone.13.-.2022-06-10.at.10.51.28.mp4

Android

XRecorder_10062022_103832.mp4

…p into aldo_fix-crash-missing-personal-details

aldo-expensify · 2022-06-10T19:50:42Z

I think I may have miss-clicked and dragged Marc into the review, unless there is a feature that I don't know of that always includes Marc 😆

marcaaron · 2022-06-10T19:54:44Z

I just added myself as a "code owner" (kind of hate this name as I don't own anything) as it's the easiest way to subscribe to changes on some specific files that I'm interested in. Usually removing myself if I have nothing to add.

aldo-expensify · 2022-06-10T19:56:47Z

I just added myself as a "code owner" (kind of hate this name as I don't own anything) as it's the easiest way to subscribe to changes on some specific files that I'm interested in. Usually removing myself if I have nothing to add.

I see! good to know :)

aldo-expensify · 2022-06-10T19:58:35Z

src/pages/home/report/ReportActionsView.js

+        // Waiting until ONYX variables are loaded before displaying the component
+        if (_.isEmpty(this.props.personalDetails)) {
+            return null;
+        }
+


Hum, I think this may cause a white screen to show for a few moments. Maybe I should have put a spinner. I'll try that if the whole approach makes sense.

marcaaron · 2022-06-10T19:56:28Z

src/pages/home/report/ReportActionsView.js

@@ -403,6 +407,11 @@ class ReportActionsView extends React.Component {
            return null;
        }

+        // Waiting until ONYX variables are loaded before displaying the component


Probably we are not waiting for Onyx, but the API response to set these expected details?

Would also maybe consider moving this to the ReportScreen... seems we have a few "loading" type interactions there and instead of returning null we can show the loading state?

I retested this and I didn't find any case where _.isEmpty(this.props.personalDetails) is true. At this point I'm not sure why I added it.

I just removed it, I don't think it makes sense because:

if this.props.personalDetails is an empty object, we won't crash when we open the DetailsPage because of the new spinner

ReportActionsView doesn't really use the personal details, so it is wrong to hold it from rendering because PersonalDetails are not ready

if this.props.personalDetails is null or undefined, it would crash earlier here

aldo-expensify · 2022-06-13T16:29:48Z

Update: removed useless check

marcaaron · 2022-06-13T18:32:47Z

src/pages/home/report/ReportActionsView.js

@@ -127,7 +131,7 @@ class ReportActionsView extends React.Component {

        // If the reportID is not found then we have either not loaded this chat or the user is unable to access it.
        // We will attempt to fetch it and redirect if still not accessible.
-        if (!this.props.report.reportID) {
+        if (!this.props.report.reportID || !this.props.personalDetails[this.props.report.reportID]) {


The personalDetails object has a key that is a reportID? IIRC personalDetails are keyed by logins.

You are totally right about that... investigating more...

Sorry for the long wait. I think I found a better way to handle this stuff:

Just like Report.fetchChatReportsByIDs chains a call to PersonalDetails.getFromReportParticipants here, I chained a PersonalDetails.getFromReportParticipants call here when we call Reports.fetchOrCreateChatReport. This way we don't depend on Report.fetchChatReportsByIDs being called (which happens inconsistently depending on the platform as explained here)

I know the new API refactor avoids chaining, but this is an easy fix, consistent with the old chaining approach and fixes the crash for IOS/Android, so I think it is fine as a temporary solution until all this gets refactored.

amyevans · 2022-06-13T19:46:28Z

src/pages/DetailsPage.js

+            <View style={[styles.flex1, styles.alignItemsCenter, styles.justifyContentCenter]}>
+                <ActivityIndicator color={themeColors.spinner} size="large" />
+            </View>


NAB, but you could use the <FullscreenLoadingIndicator /> component here

I'll give it a try, thanks!

Yep, that works. Because of the name, I assumed it would block the whole screen and not just the sidebar.

…issing-personal-details

aldo-expensify · 2022-06-29T18:59:24Z

Friendly bump @marcaaron

OSBotify · 2022-07-01T01:20:26Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

tgolen · 2022-07-04T20:16:04Z

src/pages/DetailsPage.js

@@ -66,6 +67,10 @@ const getPhoneNumber = (details) => {

 const DetailsPage = (props) => {
    const details = props.personalDetails[props.route.params.login];


I think that this line is probably what is causing this crash: #9545

It doesn't seem like a very safe way to access properties. It appears like there can be cases where props.route.params is undefined. Is that a valid case? Should this use lodashGet instead?

I think you are right. If you just open https://new.expensify.com/details without the param login, it crashes. I'll spin a small PR to fix it.

Also, this feels low priority, I don't know how a user ended up with the URL without the parameter.

OSBotify · 2022-07-08T01:34:15Z

🚀 Deployed to staging by @marcaaron in version: 1.1.81-0 🚀

platform	result
🤖 android 🤖	failure ❌
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	failure ❌

OSBotify · 2022-07-13T01:20:30Z

🚀 Deployed to production by @roryabraham in version: 1.1.82-5 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	failure ❌
🕸 web 🕸	success ✅

aldo-expensify added 2 commits June 9, 2022 15:18

Fix crash when personalDetails are missing

dbd8656

Show spinner when personal details are not available

07e0997

aldo-expensify self-assigned this Jun 9, 2022

aldo-expensify mentioned this pull request Jun 9, 2022

[$250] Android/IOS - Chat - App is crashed after tap on avatar for non-existing account #9182

Closed

aldo-expensify added 2 commits June 9, 2022 19:13

Merge branch 'aldo_test-proposed-solution' of github.com:Expensify/Ap…

dea9de5

…p into aldo_fix-crash-missing-personal-details

Fix import

c379cc6

aldo-expensify changed the title ~~Show spinner when personal details are not available~~ Fix crashes when personal details are not available Jun 10, 2022

Add isRequired to personalDetails prop

d47836b

aldo-expensify marked this pull request as ready for review June 10, 2022 19:49

aldo-expensify requested review from marcaaron and a team as code owners June 10, 2022 19:49

melvin-bot bot requested review from amyevans and removed request for a team June 10, 2022 19:49

aldo-expensify commented Jun 10, 2022

View reviewed changes

marcaaron reviewed Jun 10, 2022

View reviewed changes

Remove unnecessary check

7b1660c

marcaaron reviewed Jun 13, 2022

View reviewed changes

amyevans reviewed Jun 13, 2022

View reviewed changes

aldo-expensify added 3 commits June 21, 2022 12:17

Find personal details when creating new chat

bb05c28

Merge branch 'main' of github.com:Expensify/App into aldo_fix-crash-m…

dd87f7e

…issing-personal-details

Replace ActivityIndicator by FullscreenLoadingIndicator

1cc5eea

amyevans approved these changes Jun 22, 2022

View reviewed changes

marcaaron merged commit f19a4d9 into main Jul 1, 2022

marcaaron deleted the aldo_fix-crash-missing-personal-details branch July 1, 2022 01:15

tgolen reviewed Jul 4, 2022

View reviewed changes

roryabraham mentioned this pull request Jul 8, 2022

Deploy Checklist: New Expensify 2022-07-07 #9781

Closed

kbecciv mentioned this pull request Jul 9, 2022

[$250] Web - Chats - When clicking on a searched user in LHN the page closes #8641

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix crashes when personal details are not available #9385

Fix crashes when personal details are not available #9385

aldo-expensify commented Jun 9, 2022 •

edited

Loading

aldo-expensify commented Jun 10, 2022 •

edited

Loading

marcaaron commented Jun 10, 2022

aldo-expensify commented Jun 10, 2022

aldo-expensify Jun 10, 2022 •

edited

Loading

marcaaron Jun 10, 2022

marcaaron Jun 10, 2022

aldo-expensify Jun 13, 2022

aldo-expensify commented Jun 13, 2022

marcaaron Jun 13, 2022

aldo-expensify Jun 13, 2022

aldo-expensify Jun 21, 2022

amyevans Jun 13, 2022

aldo-expensify Jun 21, 2022

aldo-expensify Jun 21, 2022

aldo-expensify commented Jun 29, 2022

OSBotify commented Jul 1, 2022

tgolen Jul 4, 2022

aldo-expensify Jul 6, 2022

aldo-expensify Jul 6, 2022

OSBotify commented Jul 8, 2022

OSBotify commented Jul 13, 2022

		@@ -66,6 +67,10 @@ const getPhoneNumber = (details) => {

		const DetailsPage = (props) => {
		const details = props.personalDetails[props.route.params.login];

Fix crashes when personal details are not available #9385

Fix crashes when personal details are not available #9385

Conversation

aldo-expensify commented Jun 9, 2022 • edited Loading

Details

Fixed Issues

Tests

QA

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Screenshots

Web

Mobile Web

Desktop

iOS

Android

aldo-expensify commented Jun 10, 2022 • edited Loading

marcaaron commented Jun 10, 2022

aldo-expensify commented Jun 10, 2022

aldo-expensify Jun 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aldo-expensify commented Jun 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aldo-expensify commented Jun 29, 2022

OSBotify commented Jul 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OSBotify commented Jul 8, 2022

OSBotify commented Jul 13, 2022

aldo-expensify commented Jun 9, 2022 •

edited

Loading

aldo-expensify commented Jun 10, 2022 •

edited

Loading

aldo-expensify Jun 10, 2022 •

edited

Loading