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.

Sign up for GitHub

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

Fullscreen loader logging #8347

Merged
merged 5 commits into from
Apr 5, 2022
Merged

Fullscreen loader logging #8347

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ed18c49
Add logging to fullscreen loaders to investigate infinite loading scr…
kidroca Mar 29, 2022
5590d10
FullscreenLoadingIndicator - don't print additional noise from stackt…
kidroca Mar 29, 2022
48ebd2a
Add loader timeout logs for MainDrawerNavigator and NavigationRoot
kidroca Mar 29, 2022
cd397ae
Update FullScreenLoadingIndicator with PR suggestions
kidroca Mar 30, 2022
2e50713
Remove FullscreenLoadingIndicator class description
kidroca Apr 4, 2022
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
55 changes: 42 additions & 13 deletions src/components/FullscreenLoadingIndicator.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,17 +5,22 @@ import {ActivityIndicator, StyleSheet, View} from 'react-native';
import styles from '../styles/styles';
import themeColors from '../styles/themes/default';
import stylePropTypes from '../styles/stylePropTypes';
import Log from '../libs/Log';

const propTypes = {
/** Controls whether the loader is mounted and displayed */
visible: PropTypes.bool,
/** Name used in the logs if the loader is displayed for too long time */
name: PropTypes.string,

/** Optional duration (ms) after which a message would be logged if the loader is still covering the screen */
timeout: PropTypes.number,
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: Rather than have a custom timeout here we can just make this shouldLogAlertOnTimeout since we use 15 seconds everywhere anyway and there are not cases where we need a more specific timeout (and maybe never will be)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In order to capture meaningful logs we need to have logDetail with at least the screen name, or some name we can use to distinct log messages

Instead of introducing a flag like shouldLogAlertOnTimeout we use the optional logDetail prop - if it is provided it's an indication that we'd like to capture logs

If you prefer to have an explicit flag anyway let me know and I'll update


/** Additional style props */
style: stylePropTypes,
};

const defaultProps = {
visible: true,
timeout: 0,
name: '',
style: [],
};

Expand All @@ -25,18 +30,42 @@ const defaultProps = {
* @param {Object} props
* @returns {JSX.Element}
*/
const FullScreenLoadingIndicator = (props) => {
if (!props.visible) {
return null;
class FullScreenLoadingIndicator extends React.Component {
componentDidMount() {
if (!this.props.name || !this.props.timeout) {
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: We could still log without additional context. But if we want to enforce additional context then maybe we should throw and return so that someone implementing this will remember to add details.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point in order to distinct the loader, we should require some context - at least the screen name where it's used, so we should throw an error - if someone tries to setup a new or existing loader with logging incorrectly they'll know right away

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the logic to throw an error if props.logDetail exists but logDetail.name is not set

But since logDetail.name is flagged as PropTypes.required I don't know whether we need to throw an exception, and maybe we should remove it and just return

return;
}

Log.info(`[LoadingIndicator] "${this.props.name}" became visible`);

this.timeoutId = setTimeout(
() => Log.alert(
`[LoadingIndicator] "${this.props.name}" is still visible after ${this.props.timeout} ms`,
'',
false,
),
this.props.timeout,
);
}

const additionalStyles = _.isArray(props.style) ? props.style : [props.style];
return (
<View style={[StyleSheet.absoluteFillObject, styles.fullScreenLoading, ...additionalStyles]}>
<ActivityIndicator color={themeColors.spinner} size="large" />
</View>
);
};
componentWillUnmount() {
if (!this.timeoutId) {
return;
}

clearTimeout(this.timeoutId);
Log.info(`[LoadingIndicator] "${this.props.name}" disappeared`);
}

render() {
const additionalStyles = _.isArray(this.props.style) ? this.props.style : [this.props.style];
return (
<View style={[StyleSheet.absoluteFillObject, styles.fullScreenLoading, ...additionalStyles]}>
<ActivityIndicator color={themeColors.spinner} size="large" />
</View>
);
}
}

FullScreenLoadingIndicator.propTypes = propTypes;
FullScreenLoadingIndicator.defaultProps = defaultProps;
Expand Down
4 changes: 1 addition & 3 deletions src/components/WalletStatementModal/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,7 @@ class WalletStatementModal extends React.Component {
const authToken = lodashGet(this.props, 'session.authToken', null);
return (
<>
<FullScreenLoadingIndicator
visible={this.state.isLoading}
/>
{this.state.isLoading && <FullScreenLoadingIndicator />}
<View style={[styles.flex1]}>
<iframe
src={`${this.props.statementPageURL}&authToken=${authToken}`}
Expand Down
2 changes: 1 addition & 1 deletion src/libs/Navigation/AppNavigator/MainDrawerNavigator.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ const MainDrawerNavigator = (props) => {

// Wait until reports are fetched and there is a reportID in initialParams
if (!initialParams.reportID) {
return <FullScreenLoadingIndicator />;
return <FullScreenLoadingIndicator name="Main Drawer Loader" timeout={15 * 1000} />;
}

// After the app initializes and reports are available the home navigation is mounted
Expand Down
8 changes: 7 additions & 1 deletion src/libs/Navigation/NavigationRoot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,13 @@ class NavigationRoot extends Component {
render() {
return (
<NavigationContainer
fallback={<FullScreenLoadingIndicator style={styles.navigatorFullScreenLoading} />}
fallback={(
<FullScreenLoadingIndicator
name="Navigation Fallback Loader"
timeout={15 * 1000}
style={styles.navigatorFullScreenLoading}
/>
)}
onStateChange={this.parseAndStoreRoute}
onReady={this.props.onReady}
theme={navigationTheme}
Expand Down
2 changes: 1 addition & 1 deletion src/pages/NewChatPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,7 @@ class NewChatPage extends Component {
onCloseButtonPress={() => Navigation.dismissModal(true)}
/>
<View style={[styles.flex1, styles.w100, styles.pRelative]}>
<FullScreenLoadingIndicator visible={!didScreenTransitionEnd} />
{!didScreenTransitionEnd && <FullScreenLoadingIndicator />}
{didScreenTransitionEnd && (
<>
<OptionsSelector
Expand Down
2 changes: 1 addition & 1 deletion src/pages/SearchPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ class SearchPage extends Component {
onCloseButtonPress={() => Navigation.dismissModal(true)}
/>
<View style={[styles.flex1, styles.w100, styles.pRelative]}>
<FullScreenLoadingIndicator visible={!didScreenTransitionEnd} />
{!didScreenTransitionEnd && <FullScreenLoadingIndicator />}
{didScreenTransitionEnd && (
<OptionsSelector
sections={sections}
Expand Down
2 changes: 1 addition & 1 deletion src/pages/home/ReportScreen.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ class ReportScreen extends React.Component {
nativeID={CONST.REPORT.DROP_NATIVE_ID}
style={[styles.flex1, styles.justifyContentEnd, styles.overflowHidden]}
>
<FullScreenLoadingIndicator visible={this.shouldShowLoader()} />
{this.shouldShowLoader() && <FullScreenLoadingIndicator />}
{!this.shouldShowLoader() && (
<ReportActionsView
reportID={reportID}
Expand Down
4 changes: 1 addition & 3 deletions src/pages/iou/IOUModal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -387,9 +387,7 @@ class IOUModal extends Component {
</View>
</View>
<View style={[styles.pRelative, styles.flex1]}>
<FullScreenLoadingIndicator
visible={!didScreenTransitionEnd}
/>
{!didScreenTransitionEnd && <FullScreenLoadingIndicator />}
{didScreenTransitionEnd && (
<>
{currentStep === Steps.IOUAmount && (
Expand Down
2 changes: 1 addition & 1 deletion src/pages/workspace/WorkspaceInvitePage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,7 @@ class WorkspaceInvitePage extends React.Component {
onBackButtonPress={() => Navigation.goBack()}
/>
<View style={[styles.flex1]}>
<FullScreenLoadingIndicator visible={!didScreenTransitionEnd} />
{!didScreenTransitionEnd && <FullScreenLoadingIndicator />}
{didScreenTransitionEnd && (
<OptionsSelector
autoFocus={false}
Expand Down