-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Local notifications on iOS and Android #3024
Conversation
# Conflicts: # ios/ExpensifyCash.xcodeproj/project.pbxproj
# Conflicts: # ios/ExpensifyCash.xcodeproj/project.pbxproj # ios/Podfile.lock # package-lock.json # package.json
Okay, a more comprehensive update from my testing on iOS so far today: Ideal
Actual (production)
Actual (in this branch)Disclaimer: The last two rows in this table are guesses, AFAIK there's no way to test a closed RN app on dev, and if you leave your phone locked for long enough for the server to realize you're not connected to Pusher, XCode will also kill the metro process.
So overall, it's a step in the right direction, but still not perfect. From what I can tell, the reason we Still haven't figured out the best solution here. Also, I discovered a bug in my current implementation that seems to stem from this problem:
I think if I fix this bug then we could merge this, because it's not making anything worse (at least not on iOS). Some things that aren't clear to me are: What causes the client to disconnect? Can we prevent that entirely? Can we notify the server before it happens? |
Oh yeah, another bug I've found: When opening a chat from a push notification or local notification, the chat does not mark as read unless you leave the chat and come back. Seems weird, but is a completely separate problem. Can maybe squeeze a fix in without causing a regression in the unread indicators/unread badge count. |
Also iOS E2E tests are failing 😞 |
Also – this will blast the user with local "push" notifications, even if the user has E.cash open on web/desktop. Brought this up in the parent issue, but imo that should also block this PR |
I think my previous understanding of the problem may have been wrong – debugging shows that pusher is still connected and the callback is being executed. However, the local notification is not always displaying when the app is in the background. This might be related to zo0r/react-native-push-notification#1574 |
# Conflicts: # src/libs/Notification/PushNotification/index.native.js
react-native-push-notification library has been abandoned, so we'd probably need to use something else for local notifications. Probably notifee |
Note: As of now this has been tested and is working well on iOS. Still need to test on Android.
Details
This PR introduces a local notification library for iOS and Android. It will display a locally-generated push notification whenever a local notification is generated on web/desktop, so:
# This first command will reset the cache then not do anything react-native start --reset-cache npm run ios
Caveat: I'm not sure the rules for how long the app will stay connected to Pusher when it is backgrounded. During my testing, I've found some puzzling behavior...
TODO: discovered a bug where, if you receive push UA push notifications and they're displayed, then you tap to open that chat, then background the app, those message still appear as unread per the unread counter
Update: I think I understand the scenario in which notifications do not display in the background. This will happen when the client disconnects from the Pusher channel (happens after some undetermined amount of time with the app backgrounded), but the server still thinks they're connected. Need to dig into how this can happen and how to fix it.
Fixed Issues
Fixes (partial) https://github.com/Expensify/Expensify/issues/158830
Note that this doesn't improve push notifications in any other states (and in fact, still needs to be tested to make sure it doesn't break UA push notifications, which it may well do...) But assuming that it doesn't break UA push notifications, it will be a substantial improvement to the notification experience on iOS/Android.
Tests / QA Steps
Tested On
Screenshots
Web
Mobile Web
Desktop
iOS
Android