fix: Incomplete reports would cause the app to crash on startup #2649

kidroca · 2021-04-30T14:33:38Z

<If necessary, assign reviewers that know the area or changes well. Feel free to tag any additional reviewers you see fit.>
@marcaaron

Added filtering so that only valid reports are processed by findLastAccessedReport
Updated getInitialReport to getInitialReportScreenParams and moved to utils

This is a follow up to address the issue found in #2575

Fixes #2497

Added filtering so that only valid reports are processed by findLastAccessedReport Updated getInitialReport to getInitialReportScreenParams and moved to utils

src/libs/reportUtils.js

marcaaron

Looks good but agree about putting the method back to the MainDrawerNavigator

src/libs/reportUtils.js

kidroca · 2021-04-30T17:26:02Z

Updated

kidroca · 2021-04-30T17:29:23Z

BTW I noticed that the Sidebar component uses Onyx to get the currently viewed report, while the Report Screen would use routing information

https://github.com/Expensify/Expensify.cash/blob/6c7a420a565ff8eab9e2fc5c05d60304f68d9f39/src/pages/home/sidebar/SidebarLinks.js#L100

These might need to be updated so they either use only nav routing or onyx. I think with the latest updates you can be sure that the data provided by navigation is accurate

marcaaron · 2021-04-30T17:40:03Z

These might need to be updated so they either use only nav routing or onyx. I think with the latest updates you can be sure that the data provided by navigation is accurate

What exactly did you mean by this? It sort of makes sense that the report can use the routing since it's what sets the "current reportID in view" then anything else that needs to know about this can refer to that? But maybe you have some specific improvement in mind.

marcaaron

LGTM

kidroca · 2021-04-30T17:52:30Z

What exactly did you mean by this?

The ReportScreen have to set the current reportID namely because of others usages that rely on Onyx to get the currently viewed report, they can just as easily get it from react-navigation

It's not really about improvement but about consistency you can get the it from Onyx or from navigation and right now we're doing both - We should not be doing the same thing in two different ways as it can be confusing

marcaaron · 2021-04-30T17:57:52Z

It's not really about improvement but about consistency

I'd say consistency is a kind of improvement we value 🙃

others usages that rely on Onyx to get the currently viewed report, they can just as easily get it from react-navigation

I guess this is the part I'm curious about. Is there some way to access the route params via the navigation object?

kidroca · 2021-04-30T18:23:18Z

I guess this is the part I'm curious about. Is there some way to access the route params via the navigation object?

A screen would receive them naturally from props the way ReportScreen works
You can also use the useRoute hook - https://reactnavigation.org/docs/use-route

I guess you might say "What about usages like in Report.js and User.js" (The only such usages BTW)

https://github.com/Expensify/Expensify.cash/blob/a2d2f4ecd42aa9f2912db7dac92583498532c218/src/libs/actions/User.js#L144

I don't think you need the reportID here - this would get resolved by initalParams, you can just redirect to HOME, or unmount the ValidateLogin screen

Now the usage in Report.js is a bit harder to get around, because it probably is event driven - you can't just pass the reportId as a parameter at the start

https://github.com/Expensify/Expensify.cash/blob/a2d2f4ecd42aa9f2912db7dac92583498532c218/src/libs/actions/Report.js#L491

So to cover both the above cases you can maybe capture reportID in Navigation and provide it / export it from there to everyone else

https://github.com/Expensify/Expensify.cash/blob/a2d2f4ecd42aa9f2912db7dac92583498532c218/src/libs/Navigation/Navigation.js#L62-L65

Maybe there's an easier way...

OSBotify · 2021-04-30T18:28:51Z

🚀 Deployed to staging in version: 1.0.34-5🚀

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

isagoico · 2021-04-30T20:38:38Z

@kidroca @marcaaron Any QA needed for this PR?

kidroca · 2021-04-30T20:55:19Z

@isagoico

This addressed something missed in #2575

I guess the steps on the previous PR need to be re-run, but I'm not sure how you handle this internally

isagoico · 2021-04-30T20:59:30Z

Mmmm I think we could rerun the previous PR? @marcaaron can you confirm this is ok?

marcaaron · 2021-04-30T22:16:04Z

Yep re-running the previous PR sounds OK. However, it's not likely you'll run into the issue as we aren't sure what causes it and it should be fixed now.

roryabraham · 2021-05-03T17:12:47Z

I am suspicious that this PR may have caused a regression

roryabraham · 2021-05-03T17:17:18Z

Nevermind, doesn't seem like it did

OSBotify · 2021-05-08T01:43:19Z

🚀 Deployed to production in version: 1.0.39-5🚀

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

roryabraham · 2022-10-06T20:38:33Z

src/libs/Navigation/AppNavigator/MainDrawerNavigator.js

@@ -85,3 +90,4 @@ export default compose(
        },
    }),
 )(MainDrawerNavigator);
+export {getInitialReportScreenParams};


Old PR now, but any idea why this export was added? Doesn't seem to be used anywhere AFAICT

Maybe we were using it somewhere at some point. I don't really know tbh.

Originally it was part of reportUtils and imported to MainDrawerNavigator: bc63c49#diff-82f2652821babb48b600a87486fa1191db7b3cc300845573a5757676e13cfc26L44

I decided it's tied to the MainDrawerNavigator (see _.once decorator and description) and moved it for better context
I guess I used automatic refactoring, which preserves exporting the file after it's moved

fix: Incomplete reports would cause the app to crash on start up

3edeb90

Added filtering so that only valid reports are processed by findLastAccessedReport Updated getInitialReport to getInitialReportScreenParams and moved to utils

kidroca requested a review from a team as a code owner April 30, 2021 14:33

MelvinBot requested review from Gonals and removed request for a team April 30, 2021 14:33

kidroca commented Apr 30, 2021

View reviewed changes

src/libs/reportUtils.js Outdated Show resolved Hide resolved

src/libs/reportUtils.js Outdated Show resolved Hide resolved

kidroca mentioned this pull request Apr 30, 2021

Fix: Blank screen after creating an account #2575

Merged

5 tasks

marcaaron reviewed Apr 30, 2021

View reviewed changes

src/libs/reportUtils.js Outdated Show resolved Hide resolved

src/libs/reportUtils.js Show resolved Hide resolved

marcaaron assigned kidroca Apr 30, 2021

refactor: move getInitialReportScreenParams to MainDrawerNavigator

bc63c49

kidroca requested a review from marcaaron April 30, 2021 17:26

marcaaron approved these changes Apr 30, 2021

View reviewed changes

marcaaron merged commit 57fcfb6 into Expensify:main Apr 30, 2021

OSBotify mentioned this pull request Apr 30, 2021

Deploy Checklist: Expensify.cash 2021-04-29 #2639

Closed

23 tasks

kidroca deleted the kidroca/main-drawer-nav-initial-report-fix branch April 30, 2021 20:55

roryabraham reviewed Oct 6, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Incomplete reports would cause the app to crash on startup #2649

fix: Incomplete reports would cause the app to crash on startup #2649

kidroca commented Apr 30, 2021 •

edited

Loading

marcaaron left a comment

kidroca commented Apr 30, 2021

kidroca commented Apr 30, 2021

marcaaron commented Apr 30, 2021

marcaaron left a comment

kidroca commented Apr 30, 2021

marcaaron commented Apr 30, 2021

kidroca commented Apr 30, 2021

OSBotify commented Apr 30, 2021

isagoico commented Apr 30, 2021

kidroca commented Apr 30, 2021

isagoico commented Apr 30, 2021

marcaaron commented Apr 30, 2021

roryabraham commented May 3, 2021

roryabraham commented May 3, 2021

OSBotify commented May 8, 2021

roryabraham Oct 6, 2022

marcaaron Oct 6, 2022

kidroca Oct 7, 2022

fix: Incomplete reports would cause the app to crash on startup #2649

fix: Incomplete reports would cause the app to crash on startup #2649

Conversation

kidroca commented Apr 30, 2021 • edited Loading

marcaaron left a comment

Choose a reason for hiding this comment

kidroca commented Apr 30, 2021

kidroca commented Apr 30, 2021

marcaaron commented Apr 30, 2021

marcaaron left a comment

Choose a reason for hiding this comment

kidroca commented Apr 30, 2021

marcaaron commented Apr 30, 2021

kidroca commented Apr 30, 2021

OSBotify commented Apr 30, 2021

isagoico commented Apr 30, 2021

kidroca commented Apr 30, 2021

isagoico commented Apr 30, 2021

marcaaron commented Apr 30, 2021

roryabraham commented May 3, 2021

roryabraham commented May 3, 2021

OSBotify commented May 8, 2021

roryabraham Oct 6, 2022

Choose a reason for hiding this comment

marcaaron Oct 6, 2022

Choose a reason for hiding this comment

kidroca Oct 7, 2022

Choose a reason for hiding this comment

kidroca commented Apr 30, 2021 •

edited

Loading