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

Prevent sleep timer from running on native platforms #1786

Merged
merged 6 commits into from
Mar 22, 2021

Conversation

marcaaron
Copy link
Contributor

@marcaaron marcaaron commented Mar 16, 2021

Details

This sleep timer thing is wreaking certain havoc on native iOS and Android. Not sure why, but probably it should be disabled for now to restore performance on native apps. My current theory is that other work on the JS thread can cause the timer to get out of whack and cause reconnection callbacks to fire.

Fixed Issues

Fixes https://github.com/Expensify/Expensify/issues/157445

Tests

iOS/Android

  1. Test navigating around the app
  2. Verify that the reconnection callbacks are not firing randomly
  3. Verify that putting the app into the background and bringing it active again does fire the reconnection callbacks

Web/Desktop

  1. Test navigating around the app
  2. Close your laptop screen and reopen it
  3. Verify the clock timer works for this scenario
  4. Verify that minimizing the browser or app window and maximizing it again does not cause the reconnection callbacks to fire

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Mobile Web

Desktop

iOS

Android

@marcaaron marcaaron self-assigned this Mar 16, 2021
@marcaaron marcaaron marked this pull request as ready for review March 22, 2021 18:54
@marcaaron marcaaron requested a review from a team as a code owner March 22, 2021 18:54
@botify botify requested review from thienlnam and removed request for a team March 22, 2021 18:55
Copy link
Contributor

@thienlnam thienlnam left a comment

Choose a reason for hiding this comment

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

Code looks good and it works, just had some questions about some of the code

Comment on lines +19 to +21
shouldReportActivity
&& (appState === CONST.APP_STATE.INACTIVE || appState === CONST.APP_STATE.BACKGROUND)
&& state === CONST.APP_STATE.ACTIVE
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I understand this but basically this is only change the app state if the user has become active and the stored state is not active right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct. We set up the state (assume to be active when the app inits) then listen for changes and save it locally each time so we can detect a change from one state to another.

Comment on lines +27 to +30
AppState.addEventListener('change', appStateChangeCallback);
return () => {
AppState.removeEventListener('change', appStateChangeCallback);
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you explain what is happening here? I'm not totally sure what this is doing, is it initializing with an event listener and then whenever it is returned it gets removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! So, addBecameActiveListener is both setting up an event listener and then returning a function that we can call later to remove the listener. The appStateChangeCallback exists in the closure created by calling the outermost function. It's just sort of a convenient pattern for cleaning up the listener we created and I borrowed it from how NetInfo works.

@thienlnam
Copy link
Contributor

Sweet, thanks for the explanations, makes sense!

@thienlnam thienlnam merged commit 5e68017 into master Mar 22, 2021
@thienlnam thienlnam deleted the marcaaron-sleepTimer branch March 22, 2021 22:09
@github-actions github-actions bot locked and limited conversation to collaborators Mar 22, 2021
@isagoico
Copy link

@marcaaron Any way we could test the callbacks on our side? Or should we just test that the app is updated after reopening it from background?

@marcaaron
Copy link
Contributor Author

Or should we just test that the app is updated after reopening it from background?

That should work! It's the only way to test this without having to look at the logs to verify that they are running correctly.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants