Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Implement message-type Android push notifications #4270
Implement message-type Android push notifications #4270
Changes from all commits
23d1661
c3fc83e
259c993
c00d7d9
beaf6e0
7534047
9aa6a0d
f8bbdb6
640b3be
0bf877b
474203d
a68b744
c105f86
48a5218
6b61b14
fb98d87
0c938c2
d175673
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this mean? Is there a hard limit of 10 seconds?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's related to this: https://developer.android.com/about/versions/12/behavior-changes-all#foreground-service-notification-delay
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Android 12 change only applies to foreground services. Regular notifications have no such limit and should always display -- unless the UA implementation is doing something very weird.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Android is happy for us to do stuff when the app is backgrounded, provided we display a long-running notification (like FB and WhatsApp 'Checking for messages...' notification). But that is very different from what we're trying to do here, which is just display a notification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about this then; we should ask the UA team. Still, delaying notifications +10 seconds because we're fetching an avatar doesn't seem like a good idea even in bad network conditions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, why does this need to be static? Is this to ensure only one instance of NotificationCache exists?
Even as a static class, I would have thought that the cache would be lost when the app was killed? Unless
CustomNotificationProvider
has a lifecycle that extends beyond the life of the app? 😕There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are multiple instances of NotificationCache, one for each conversation that gets shown to the user in a notification. We only remove the cache after the user dismisses or opens a notification.
From my understanding, the CustomNotificationProvider lives after the app gets killed as a background service. I don't know how reliable it is, though, as I have no experience with Android dev. If we shouldn't rely on it, maybe it would be interesting to store the notification cache data in SQLite or something similar?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In which case, I don't think the class should be static.
That still leaves a dependency on a DB that will need to be run in the background. Ideally the data should be sent in the push notification payload, so we would send all unread messages.
This would be odd if true. Depending on how they implemented this there is no guarantee that it will remain alive. Would you mind sharing the documentation for this? Or any links you have to the code they gave us.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't allow me to remove the static modifier of the class, and if I do it, we won't be able to do
new NotificationCache.Message(...);
Yeah, I didn't explain correctly. This service gets launched when a notification arrives, but it will die after a while; as you say, we have no guarantee.
We can't send all data in the notification payload because of the payload size limits (~10KB). That's why it's better to cache the notification conversations on the device. I guess using the shared preferences could be enough for this use case instead of using a DB.
Also, having the conversation stored on the device will make it easier for us when we want to implement the quick reply from the notification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that they are making the cache class static so that it's lifecycle extends that of each individual
ReactNotificationProvider
instance. It works I guess, but it's a bit hacky and not guaranteed to be persisted.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But surely 10KB is fine for some simple objects? We don't need the full profile image but simply the URL, no?