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 NotificationResponse race condition #3740

Merged
merged 1 commit into from
Jun 24, 2021

Conversation

roryabraham
Copy link
Contributor

@roryabraham roryabraham commented Jun 24, 2021

Details

For a while, the callback for the NotificationResponse event (i.e: when a push notification is tapped) was usually not being executed. The previous code falsely assumed that the NotificationResponse callback would happen while the app was still in the background, because it is when the event occurs. It did rarely happen this way, because there was a race condition in play between these two responses to this event:

  1. The OS natively brings the app to the foreground.
  2. The JavaScript callback is executed.

The native OS response is almost always faster, so by the time the JS callback executes, the app is in the foreground and we skipped the callback. Instead, we'll only skip the callback for PushReceived events that happen when the app is in the foreground.

Fixed Issues

Fixes (partial) https://github.com/Expensify/Expensify/issues/158830

Tests / QA Steps

  1. (iOS dev only) Launch the app on a physical iPhone device. (SO)
  2. Log into the app on mobile as user A.
  3. Log into the app on another platform as user B.
  4. As user A, open a chat between A and a user other than B.
  5. As user A, background the mobile app
  6. As user B, send a message to user A.
  7. Verify that a push notification appears for user A.
  8. Tap on the notification. Verify that the app opens to the chat between user A and user B (a different chat than you had open when you backgrounded the app).

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Mobile Web

Desktop

iOS

RPReplay_Final1624493296.MP4

Android

@roryabraham roryabraham requested a review from a team as a code owner June 24, 2021 00:20
@roryabraham roryabraham self-assigned this Jun 24, 2021
@MelvinBot MelvinBot requested review from MariaHCD and removed request for a team June 24, 2021 00:20
@MariaHCD MariaHCD merged commit 3fdea74 into main Jun 24, 2021
@MariaHCD MariaHCD deleted the Rory-NotificationResponseRaceCondition branch June 24, 2021 14:30
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify
Copy link
Contributor

🚀 Deployed to staging in version: 1.0.73-4🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production in version: 1.0.74-0🚀

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

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

Successfully merging this pull request may close these issues.

3 participants