Fullscreen loader logging #8347
Conversation
| name: PropTypes.string, | ||
|
|
||
| /** Optional duration (ms) after which a message would be logged if the loader is still covering the screen */ | ||
| timeout: PropTypes.number, |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
| return null; | ||
| class FullScreenLoadingIndicator extends React.Component { | ||
| componentDidMount() { | ||
| if (!this.props.name || !this.props.timeout) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Left some notes, will address the changes tomorrow
BTW
I haven't checked some PR check items, because I'm not certain what I should do.
Waiting to be clarified in this slack thread: https://expensify.slack.com/archives/C01GTK53T8Q/p1648583058174529
| return null; | ||
| class FullScreenLoadingIndicator extends React.Component { | ||
| componentDidMount() { | ||
| if (!this.props.name || !this.props.timeout) { |
There was a problem hiding this comment.
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
- remove `name` and `timeout` props. Use fixed timeout and a `logDetail` prop - use fixed strings in log messages and pass details as additional data - extract timeout duration to CONST - throw if logging is not configured correctly - trigger a special CONST.ERROR.ENSURE_BUGBOT alert
There was a problem hiding this comment.
Added changes from the provided suggestions
The updated logging looks like this:
- note: since "Disappeared" is logged on unmount, the component didn't received the new props and logs whatever
logDetailit last had, hencereportIDis still""in the log
A new change is the Timing API
We capture the duration for which a loader was visible using Timing.start/end
If that's undesired we can just discard the last commit
| name: PropTypes.string, | ||
|
|
||
| /** Optional duration (ms) after which a message would be logged if the loader is still covering the screen */ | ||
| timeout: PropTypes.number, |
There was a problem hiding this comment.
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
| return null; | ||
| class FullScreenLoadingIndicator extends React.Component { | ||
| componentDidMount() { | ||
| if (!this.props.name || !this.props.timeout) { |
There was a problem hiding this comment.
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
|
@marcaaron PR is ready for review |
Let's undo this as it feels like we're just tacking on extra scope. The timing stuff is pretty focused on a few key metrics and we don't even know what our issue is here. Let's not just time things for the sake of timing them. |
Other components do not use class description Removed to be uniform with the rest of our components
2f6e18b to
2e50713
Compare
Dropped the commit that introduced this change Ready for review |
There was a problem hiding this comment.
LGTM.
cc: @thienlnam
PR Reviewer Checklist
- I verified the PR has a small number of commits behind
main - I verified the correct issue is linked in the
### Fixed Issuessection above - I verified testing steps are clear and they cover the changes made in this PR
- I verified the testing environment is mentioned in the test steps
- I verified testing steps cover success & fail scenarios (if applicable)
- I checked that screenshots or videos are included for tests on all platforms
- I verified tests pass on all platforms & I tested again on:
- iOS / native
- Android / native
- iOS / Safari
- Android / Chrome
- MacOS / Chrome
- MacOS / Desktop
- I verified there are no console errors related to changes in this PR
- I verified proper code patterns were followed (see Reviewing the code)
- I verified comments were added when the code was not self explanatory
- I verified any copy / text shown in the product was added in all
src/languages/*files (if applicable) - I verified proper naming convention for platform-specific files was followed (if applicable)
- I verified style guidelines were followed
- I verified the JSDocs style guidelines (in
STYLE.md) were followed
- I verified that this PR follows the guidelines as stated in the Review Guidelines
- I verified other components are not impacted by changes in this PR (i.e. if the PR modifies a shared library or component like
Avatar, I verified the components usingAvatarare working as expected) - I verified the UI performance was not affected (the performance is the same than
mainbranch) - If a new component is created I verified that a similar component doesn't exist in the codebase
🎀 👀 🎀 C+ reviewed
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by @marcaaron in version: 1.1.52-0 🚀
|
|
🚀 Deployed to production by @roryabraham in version: 1.1.52-0 🚀
|
Details
As per the ticket requirement we're adding logging to Fullscreen loaders in order to capture a loader never being removed from the screen
When
nameandtimeoutprops are used on the loader, it would log a message when it mounts/unmounts or stays longer than the specified timeoutIn order to have simpler component did mount / will unmount logic, the
visibleprop was removed and existing usages refactored.FullScreenLoadingIndicatoris used uniformly across the app by conditional rendering:Fixed Issues
$ #7424
Tests
[LoadingIndicator] "${this.props.name}" became visibleis logged when loader is visible[LoadingIndicator] "${this.props.name}" disappearedis logged when loader hides[LoadingIndicator] "${this.props.name}" is still visible after ${this.props.timeout} msis logged when loader is displayed for longer thantimeoutdurationFullscreenLoadingIndicatorI was able to test the
timeoutmessage (3rd bullet) when I forced the MainDrawerLoader to never hide. It also happened once (on dev) because loading was too slow and took longer than 15 sec. (after signing in)Sample logging output
note: Since "Disappeared" is logged on unmount, the component didn't received the new props and logs whatever
logDetailit last had, hencereportIDis still""in the logPR Review Checklist
Contributor (PR Author) Checklist
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*filesSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */displayNamepropertythisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)PR Reviewer Checklist
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick).src/languages/*filesSTYLE.md) were followed/** comment above it */displayNamepropertythisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)QA Steps
No feature or bug to be tested.
We're testing against any regressions around fullscreen loading indicators.
A fullscreen loading indicator is a small rotating circle in the middle of the page with semi-transparent (white/gray) background
On all platforms, verify loading circle still appears:
(These are the pages that had the
visibleprop which was removed from the loader)Screenshots
Web
Mobile Web
Desktop
iOS
Android
Android.Emulator.-.Pixel_2_API_29_5554.2022-03-29.22-00-54.mp4