feat(notifications): add backend-agnostic push notifications common module - #3338
feat(notifications): add backend-agnostic push notifications common module#3338ekjotmultani wants to merge 2 commits into
Conversation
…odule Introduce aws-push-notifications-common, a vendor-neutral library of push notification primitives that does not depend on any specific backend. It provides: - PushNotificationPayload: parses a NotificationPayload / FCM data map / RemoteMessage / Intent into a displayable payload using standard payload keys. Unlike the previous parser it never drops a valid FCM message, so pushes without backend-specific keys are parsed and displayed. - PushNotificationsUtils: showNotification, isAppInForeground and areNotificationsEnabled backed by NotificationCompat. - PushNotificationPermission and PermissionRequestResult for POST_NOTIFICATIONS handling on API 33+. - PushNotificationsConstants and the notification channel DSL. The public NotificationPayload and NotificationContentProvider types continue to come from common-core and are re-exported. Depends only on common-core and androidx/firebase-messaging.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3338 +/- ##
==========================================
- Coverage 56.44% 56.30% -0.15%
==========================================
Files 1088 1095 +7
Lines 32175 32386 +211
Branches 4799 4834 +35
==========================================
+ Hits 18162 18235 +73
- Misses 12124 12250 +126
- Partials 1889 1901 +12 🚀 New features to boost your workflow:
|
| @SuppressLint("NewApi") | ||
| fun showNotification(notificationId: Int, payload: PushNotificationPayload, targetClass: Class<*>?) { | ||
| CoroutineScope(Dispatchers.IO).launch { | ||
| val largeImageIcon = payload.imageUrl?.let { downloadImage(it) } |
There was a problem hiding this comment.
[major] CoroutineScope(Dispatchers.IO) with no Job or lifecycle attachment creates a scope that's never cancelled. If the context is destroyed while the image download is running, the coroutine keeps going with a captured context reference and then calls notify() on it. Pass a CoroutineScope into the constructor so callers can tie the lifetime to a LifecycleOwner.lifecycleScope or cancel it themselves:
class PushNotificationsUtils(
private val context: Context,
private val scope: CoroutineScope,
private val channelId: String = ...
)
| setContentTitle(payload.title) | ||
| setContentText(payload.body) | ||
| setSmallIcon(R.drawable.ic_launcher_foreground) | ||
| setContentIntent(pendingIntent) |
There was a problem hiding this comment.
[major] R.drawable.ic_launcher_foreground references this library's own resource, not the consuming app's. On API 26+ notification small icons must be monochrome/silhouette — adaptive launcher icons render as a solid white square in the status bar. There's also no way for library consumers to provide their own icon.
The small icon should be a constructor parameter:
class PushNotificationsUtils(
private val context: Context,
private val smallIconRes: Int,
...
)
|
|
||
| with(NotificationManagerCompat.from(context)) { | ||
| notify(notificationId, builder.build()) | ||
| } |
There was a problem hiding this comment.
[major] On API 33+, calling notify() without POST_NOTIFICATIONS permission silently drops the notification — no exception, no log. There's already an areNotificationsEnabled() helper on this class; worth calling it here and returning early so callers aren't left debugging phantom missing notifications. (Pre-existing behaviour carried over from aws-push-notifications-pinpoint-common, but worth fixing before this surface goes public.)
| ) { | ||
| init { | ||
| retrieveNotificationChannel() | ||
| } |
There was a problem hiding this comment.
[nit] retrieveNotificationChannel() returns NotificationChannel? and the result is silently dropped here. Since the channel is re-fetched on every showNotification() call anyway, this eager call is mostly a side-effect trigger — a debug log on null return would make channel-creation failures visible during integration testing.
| private val flow = MutableSharedFlow<IdAndResult>( | ||
| extraBufferCapacity = 1, | ||
| onBufferOverflow = BufferOverflow.DROP_OLDEST | ||
| ) |
There was a problem hiding this comment.
[minor] With extraBufferCapacity = 1 and DROP_OLDEST, if two requestPermission() calls are in-flight simultaneously and both activities post results before either collector runs, the first result is dropped and the corresponding listen().first() suspends forever (no timeout, no cancellation signal). Bumping the buffer capacity or adding a withTimeout on the first() call side would close this window.
| action[PushNotificationsConstants.URL] = it.replaceFirst("http://", "https://") | ||
| } | ||
| data[PushNotificationsConstants.DEEPLINK]?.let { action[PushNotificationsConstants.DEEPLINK] = it } | ||
| return action |
There was a problem hiding this comment.
[minor] replaceFirst("http://", "https://") mutates the sender-provided URL silently. If the target is an intranet address that only speaks HTTP, the tap action will fail quietly. Also replaceFirst matches on substring position rather than the URI scheme component — URI.scheme would be more defensive. Worth a note in the PushNotificationsConstants.URL KDoc so senders know the library enforces HTTPS.
| } | ||
|
|
||
| var sound: Uri? = Settings.System.DEFAULT_NOTIFICATION_URI | ||
| set(value) { |
There was a problem hiding this comment.
[nit] sound and audioAttributes are mutually dependent: each setter calls builder.setSound(sound, audioAttributes) using the current value of the other. The interaction is subtle: a caller who sets audioAttributes first then sound = null gets the right result; the reverse order leaves the builder with the default sound URI even though silence was intended. A brief comment spelling out the dependency would save the next reader a trip through both setters.
| val uri = Uri.fromParts("package", context.packageName, null) | ||
| val intent = Intent(Settings.ACTION_APPLICATION_DETAILS_SETTINGS, uri) | ||
| context.startActivity(intent) | ||
| } |
There was a problem hiding this comment.
[major] startActivity() from a non-Activity Context (service, application context, background receiver) requires Intent.FLAG_ACTIVITY_NEW_TASK — without it this crashes at runtime with AndroidRuntimeException. Since PushNotificationPermission accepts a bare Context, this is reachable in practice. requestPermission() correctly sets the flag on its intent; openSettings() needs the same:
intent.flags = Intent.FLAG_ACTIVITY_NEW_TASK
context.startActivity(intent)(Pre-existing in aws-push-notifications-pinpoint-common, but a public API is a good opportunity to fix it.)
soberm
left a comment
soberm
left a comment
There was a problem hiding this comment.
Parity & integration notes
This is a well-scoped foundation layer and the Pinpoint-free design is the right direction. A few behavioral changes and one structural concern are worth flagging before this gets consumed by downstream packages.
body / message key preference flip (silent behavioral change)
The old parser preferred data["message"] and fell back to data["pinpoint.notification.body"]. The new parser prefers data["body"] and falls back to data["message"]. For any sender that includes both keys in the same payload the displayed body will silently change after migration. Low probability in the wild, but worth a note in the migration guide.
Default channel ID changed: "PINPOINT.NOTIFICATION" → "amplify.notifications"
On devices that have already configured per-channel notification preferences (muted, custom sound, etc.) for the old channel, those settings will be lost silently on migration — Android channel prefs are keyed by ID with no migration path. Worth calling out in the changelog/migration guide so app developers can decide whether to keep the old channel alive as an alias.
notificationId deduplication will degrade for Pinpoint-originated pushes
The RN bridge currently computes a stable notification ID by hashing pinpoint.campaign.campaign_id + campaign_activity_id (or journey equivalents), which prevents duplicate notifications from the same campaign activity stacking up. Those keys are not present in this module. Once the bridge migrates, campaign/journey pushes will get random IDs on every delivery, so a second delivery of the same campaign activity will show two notifications instead of replacing the first. If deduplication matters for downstream consumers, the new module (or the bridge migration PR) should define a notificationId strategy — either a standard "notificationId" FCM data key, or a hash of title+body.
Four [major] items in PushNotificationsUtils — see inline comments. The openSettings() FLAG_ACTIVITY_NEW_TASK gap and the missing areNotificationsEnabled() pre-check are pre-existing bugs carried over from aws-push-notifications-pinpoint-common (not regressions introduced here), but since this is now a public API it's a good opportunity to fix them before they reach more consumers.
- PushNotificationsUtils takes a CoroutineScope so callers own the lifetime of the image download and notification post instead of a scope that is never cancelled. - Small icon is now a required constructor parameter so consumers supply an icon that renders correctly on API 26+; the bundled drawable is removed. - showNotification returns early with a log when notifications are disabled, instead of silently dropping on API 33+ without POST_NOTIFICATIONS. - retrieveNotificationChannel logs when no channel could be resolved. - PushNotificationPermission.openSettings sets FLAG_ACTIVITY_NEW_TASK so it works from a non-Activity context. - PermissionRequestChannel buffer capacity increased so concurrent results are not dropped, which could leave a request suspended. - URL https enforcement checks the parsed URI scheme and rewrites only the leading scheme, documented on PushNotificationsConstants.URL. - Add a notificationId strategy: a standard notificationId key with a stable title and body hash fallback, with migration notes on the payload. - Clarify the sound and audioAttributes setter interdependence. Adds unit tests for the URL scheme handling, the permission pre-check, the NEW_TASK flag, and the notificationId derivation, and refreshes the API dump.
2 participants
Summary
Adds
aws-push-notifications-common, a backend-agnostic library of Android push notification primitives. It is a Pinpoint-free successor toaws-push-notifications-pinpoint-common, providing the same building blocks without any coupling to Amazon Pinpoint or the AWS SDK.The module provides:
PushNotificationPayload): builds a displayable payload from aNotificationPayload, a raw FCM data map, aRemoteMessage, or anIntent, reading standard payload keys (title, body, imageUrl, url, deeplink, openApp, channelId, silentPush). It parses plain FCM pushes that carry no backend-specific keys and never drops them, so a valid non-Pinpoint push is still displayable.PushNotificationsUtils):showNotificationbacked byNotificationCompat, plusisAppInForegroundandareNotificationsEnabled.PushNotificationPermission,PermissionRequestResult):POST_NOTIFICATIONShandling for API 33+.PushNotificationsConstants,PushNotificationChannels): the generic payload keys and a notification channel DSL.The public
NotificationPayloadandNotificationContentProvidertypes continue to come fromcommon-coreand are re-exported, so consumers get them transitively.Why
This is a foundation library that other consumers depend on:
aws-push-notifications-pinpoint-common. This module gives it a Pinpoint-free replacement so it can migrate off Pinpoint with close to a dependency swap.The public symbol names and shapes are kept aligned with what these consumers already import so the migration stays small.
Dependencies
Depends only on
common-core,annotations, androidx, andfirebase-messaging. No AWS SDK and no Pinpoint dependency.Testing
16 unit tests, all passing, including a regression guard that a payload with no Pinpoint keys is parsed and not dropped. Build, ktlintCheck, apiCheck, and lintRelease all pass against current
mainafter rebase.Open questions for reviewers
@InternalAmplifyApi(ERROR level opt-in). Making it public means consumers can drop their@OptInannotations, but it also commits us to binary compatibility on these symbols. Happy to mark them internal instead if we would rather not commit yet.firebase-messagingas anapidependency.PushNotificationPayload.fromRemoteMessage(RemoteMessage)exposes a Firebase type, sofirebase-messagingis currently anapidependency and leaks transitively. If we would rather keep Firebase off the public surface, I can drop that overload and keep onlyfromData(Map<String, String>), which is what the RN bridge already uses.