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

Fix: Blank screen after creating an account #2575

Merged
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
529f0fb
feat: Navigation navigateLater and enableNavigation
kidroca Apr 26, 2021
78490e7
refactor: AuthScreens empty string is a valid initial value for initi…
kidroca Apr 26, 2021
8aeafcc
fix: Report.fetchAllReports - fetchChatReportsByIDs would be called w…
kidroca Apr 26, 2021
da36c16
fix: Report.fetchAllReports - new report created by `fetchOrCreateCha…
kidroca Apr 26, 2021
c4d25f5
fix: Report.fetchAllReports - Navigation.navigate does not work when …
kidroca Apr 26, 2021
d6596c5
refactor: Move initialReportID from AuthScreens to MainDrawerNavigator
kidroca Apr 27, 2021
1591d76
refactor: Mount the Report Screen only when initial data is resolved
kidroca Apr 27, 2021
f394b98
refactor: Reuse `updateCurrentlyViewedReportID`
kidroca Apr 27, 2021
9faf5e4
Revert "feat: Navigation navigateLater and enableNavigation"
kidroca Apr 27, 2021
fdc8259
Revert "fix: Report.fetchAllReports - Navigation.navigate does not wo…
kidroca Apr 27, 2021
d0a952f
refactor: Bring back the cast logic to fetchAllReports
kidroca Apr 28, 2021
bfdbb5f
refactor: Move Onyx updating code to promise finally
kidroca Apr 28, 2021
c71967f
fix: Remove `loading` from the URL path in the address bar
kidroca Apr 28, 2021
5c91d54
update: Keep the Report Screen mounted since the first time it's reso…
kidroca Apr 28, 2021
b20db83
refactor: Replace inline Screen names with values from SCREENS
kidroca Apr 28, 2021
a2aa335
refactor: convert MainDrawerNavigator to class to avoid `route.state`…
kidroca Apr 28, 2021
6b224e1
fix: The logic moved to finally was not correctly translated
kidroca Apr 28, 2021
6326456
refactor: remove memo usage
kidroca Apr 29, 2021
3395cda
Revert "refactor: convert MainDrawerNavigator to class to avoid `rout…
kidroca Apr 29, 2021
8bc3104
refactor: base home navigator mounting on whether there are any repor…
kidroca Apr 29, 2021
14df6cb
Revert "refactor: Reuse `updateCurrentlyViewedReportID`"
kidroca Apr 29, 2021
fa3eaa2
refactor: remove the `shouldRedirectToReport` and lastReportID setting
kidroca Apr 29, 2021
558babb
feat: findLastAccessedReport a function to derive the last viewed report
kidroca Apr 29, 2021
f0050f6
refactor: move `findLastAccessedReport` to `reportUtils`
kidroca Apr 29, 2021
48f2427
refactor: remove `initialReportID` no longer needed
kidroca Apr 29, 2021
13435af
fix: When `fetchAllReports` creates a chat with concierge it don't ha…
kidroca Apr 29, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/SCREENS.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
*/
export default {
HOME: 'Home',
LOADING: 'Loading',
REPORT: 'Report',
SIGN_IN: 'SignIn',
};
29 changes: 0 additions & 29 deletions src/libs/Navigation/AppNavigator/AuthScreens.js
Original file line number Diff line number Diff line change
Expand Up @@ -84,15 +84,11 @@ const propTypes = {
isOffline: PropTypes.bool,
}),

// The initial report for the home screen
initialReportID: PropTypes.string,

...windowDimensionsPropTypes,
};

const defaultProps = {
network: {isOffline: true},
initialReportID: null,
};

class AuthScreens extends React.Component {
Expand All @@ -101,8 +97,6 @@ class AuthScreens extends React.Component {

Timing.start(CONST.TIMING.HOMEPAGE_INITIAL_RENDER);
Timing.start(CONST.TIMING.HOMEPAGE_REPORTS_LOADED);

this.initialReportID = props.initialReportID;
}

componentDidMount() {
Expand Down Expand Up @@ -148,15 +142,6 @@ class AuthScreens extends React.Component {
return true;
}

// Skip when `this.initialReportID` is already assigned. We no longer want to update it
if (!this.initialReportID) {
// Either we have a reportID or fetchAllReports resolved with no reports. Otherwise keep waiting
if (nextProps.initialReportID || nextProps.initialReportID === '') {
this.initialReportID = nextProps.initialReportID;
return true;
}
}

return false;
}

Expand All @@ -168,11 +153,6 @@ class AuthScreens extends React.Component {
}

render() {
// Wait to resolve initial Home route params.
if (!this.initialReportID) {
return null;
}

const modalScreenOptions = {
headerShown: false,
cardStyle: getNavigationModalCardStyle(this.props.isSmallScreenWidth),
Expand All @@ -196,12 +176,6 @@ class AuthScreens extends React.Component {
headerShown: false,
title: 'Expensify.cash',
}}
initialParams={{
screen: SCREENS.REPORT,
params: {
reportID: this.initialReportID,
},
}}
component={MainDrawerNavigator}
/>
<RootStack.Screen
Expand Down Expand Up @@ -278,8 +252,5 @@ export default compose(
network: {
key: ONYXKEYS.NETWORK,
},
initialReportID: {
key: ONYXKEYS.CURRENTLY_VIEWED_REPORTID,
},
}),
)(AuthScreens);
77 changes: 64 additions & 13 deletions src/libs/Navigation/AppNavigator/MainDrawerNavigator.js
Original file line number Diff line number Diff line change
@@ -1,22 +1,62 @@
import React from 'react';
import PropTypes from 'prop-types';
import _ from 'underscore';
import {createDrawerNavigator} from '@react-navigation/drawer';
import {withOnyx} from 'react-native-onyx';

import withWindowDimensions, {windowDimensionsPropTypes} from '../../../components/withWindowDimensions';
import FullScreenLoadingIndicator from '../../../components/FullscreenLoadingIndicator';
import styles, {
getNavigationDrawerType,
getNavigationDrawerStyle,
} from '../../../styles/styles';
import ONYXKEYS from '../../../ONYXKEYS';
import compose from '../../compose';
import SCREENS from '../../../SCREENS';

// Screens
import SidebarScreen from '../../../pages/home/sidebar/SidebarScreen';
import ReportScreen from '../../../pages/home/ReportScreen';

const LoadingScreen = React.memo(() => <FullScreenLoadingIndicator visible />, () => true);

const propTypes = {
// Initial report to be used if nothing else is specified by routing
initialReportID: PropTypes.string,

// Route data passed by react navigation
route: PropTypes.shape({
state: PropTypes.shape({
// A list of mounted screen names
routeNames: PropTypes.arrayOf(PropTypes.string),
}),
}).isRequired,

...windowDimensionsPropTypes,
};

const defaultProps = {
initialReportID: null,
};

const Drawer = createDrawerNavigator();

/**
* Derives whether it's time to mount the ReportScreen from current component props
*
* @param {String} initialReportID
* @param {Object} route
* @returns {boolean}
*/
const shouldMountReportScreen = ({initialReportID, route}) => {
if (_.isString(initialReportID)) {
return true;
}

// After the ReportScreen is mounted it should stay mounted no matter initial report ID
return Boolean(route.state) && route.state.routeNames.includes(SCREENS.REPORT);
};

kidroca marked this conversation as resolved.
Show resolved Hide resolved
const MainDrawerNavigator = props => (
<Drawer.Navigator
openByDefault
Expand All @@ -28,22 +68,33 @@ const MainDrawerNavigator = props => (
sceneContainerStyle={styles.navigationSceneContainer}
edgeWidth={500}
drawerContent={() => <SidebarScreen />}
screenOptions={{
cardStyle: styles.navigationScreenCardStyle,
headerShown: false,
}}
>
<Drawer.Screen
name="Report"
component={ReportScreen}

// Providing an empty string here will ensure that the ReportScreen does not show as '/r/undefined'
// eslint-disable-next-line react/jsx-props-no-multi-spaces
initialParams={{reportID: ''}}
options={{
cardStyle: styles.navigationScreenCardStyle,
headerShown: false,
}}
/>
{
shouldMountReportScreen(props)
? (
<Drawer.Screen
name={SCREENS.REPORT}
component={ReportScreen}
initialParams={{reportID: props.initialReportID}}
/>
)
: <Drawer.Screen name={SCREENS.LOADING} component={LoadingScreen} />
}
</Drawer.Navigator>
);

MainDrawerNavigator.propTypes = propTypes;
MainDrawerNavigator.defaultProps = defaultProps;
MainDrawerNavigator.displayName = 'MainDrawerNavigator';
export default withWindowDimensions(MainDrawerNavigator);
export default compose(
withWindowDimensions,
withOnyx({
initialReportID: {
key: ONYXKEYS.CURRENTLY_VIEWED_REPORTID,
},
}),
)(MainDrawerNavigator);
1 change: 1 addition & 0 deletions src/libs/Navigation/linkingConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ export default {
screens: {
// Report route
[SCREENS.REPORT]: ROUTES.REPORT_WITH_ID,
[SCREENS.LOADING]: ROUTES.REPORT,
},
},

Expand Down
28 changes: 16 additions & 12 deletions src/libs/actions/Report.js
Original file line number Diff line number Diff line change
Expand Up @@ -727,15 +727,20 @@ function fetchAllReports(
return;
}

// The string cast here is necessary as Get rvl='chatList' may return an int
reportIDs = String(response.chatList).split(',');
kidroca marked this conversation as resolved.
Show resolved Hide resolved
// The cast here is necessary as Get rvl='chatList' may return an int or Array
reportIDs = String(response.chatList)
.split(',')
.filter(_.identity);
kidroca marked this conversation as resolved.
Show resolved Hide resolved

// Get all the chat reports if they have any, otherwise create one with concierge
if (reportIDs.length) {
if (reportIDs.length > 0) {
return fetchChatReportsByIDs(reportIDs);
}

return fetchOrCreateChatReport([currentUserEmail, 'concierge@expensify.com']);
return fetchOrCreateChatReport([currentUserEmail, 'concierge@expensify.com'])
.then((createdReportID) => {
reportIDs = [createdReportID];
});
kidroca marked this conversation as resolved.
Show resolved Hide resolved
})
.then(() => {
Onyx.set(ONYXKEYS.INITIAL_REPORT_DATA_LOADED, true);
Expand All @@ -754,16 +759,15 @@ function fetchAllReports(
// We are waiting 8 seconds since this provides a good time window to allow the UI to finish loading before
// bogging it down with more requests and operations.
}, shouldDelayActionsFetch ? 8000 : 0);

})
.finally(() => {
// Update currentlyViewedReportID to be our first reportID from our report collection if we don't have
// one already.
if (!shouldRedirectToReport || lastViewedReportID) {
return;
if (shouldRedirectToReport || !lastViewedReportID) {
const firstReportID = _.first(reportIDs);
// eslint-disable-next-line no-use-before-define
updateCurrentlyViewedReportID(firstReportID);
}

const firstReportID = _.first(reportIDs);
const currentReportID = firstReportID ? String(firstReportID) : '';
Onyx.merge(ONYXKEYS.CURRENTLY_VIEWED_REPORTID, currentReportID);
kidroca marked this conversation as resolved.
Show resolved Hide resolved
});
}

Expand Down Expand Up @@ -950,7 +954,7 @@ function handleReportChanged(report) {
* @param {Number} reportID
*/
function updateCurrentlyViewedReportID(reportID) {
Onyx.merge(ONYXKEYS.CURRENTLY_VIEWED_REPORTID, String(reportID));
Onyx.merge(ONYXKEYS.CURRENTLY_VIEWED_REPORTID, String(reportID || ''));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated this so it can be reused in fetchAllReports and to guard against null , undefined as String(null) would create a "null" (similar situation for undefined and NaN)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this is the one thing that I'm not so sure about. If we are calling this with a reportID that is null or undefined why try to set it at all? Couldn't we just leave whatever existing value we have? Seems not ideal to fail silently and set the currentlyViewedReportID to an empty string. Maybe it would be appropriate to log an alert of some kind when this happens instead?

We have some ways to do that e.g.

https://github.com/Expensify/Expensify.cash/blob/f213be0a1be5a04599d2f17d8848b0a7824eb8be/src/libs/actions/Report.js#L717-L720

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this is the one thing that I'm not so sure about. If we are calling this with a reportID that is null or undefined why try to set it at all?

As you can see the original logic would save anything without any consideration, I've just updated it so that if called with null or undefined they are converted

I'll just revert this, this will also address the used-before-defined problem

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I see. Only mentioning this since logging an alert would perhaps be useful. But no need to add it in this PR.

}

Onyx.connect({
Expand Down