-
Notifications
You must be signed in to change notification settings - Fork 0
fix: improve push notification processing logic and deduplication handling #16
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -57,39 +57,25 @@ internal class NotificationService( | |
| createNotificationChannel() | ||
| } | ||
|
|
||
| suspend fun handleNotificationReceived( | ||
| suspend fun handlePushReceived( | ||
| payload: ClixPushNotificationPayload, | ||
| notificationData: Map<String, Any?>, | ||
| autoHandleLandingURL: Boolean = true, | ||
| ) { | ||
| try { | ||
| if (hasNotificationPermission(context)) { | ||
| val shouldTrack = recordReceivedMessageId(payload.messageId) | ||
| if (!shouldTrack) { | ||
| val eventName = NotificationEvent.PUSH_NOTIFICATION_RECEIVED.name | ||
| ClixLogger.debug( | ||
| "Skipping duplicate $eventName for messageId: ${payload.messageId}" | ||
| ) | ||
| return | ||
| } | ||
|
|
||
| try { | ||
| showNotification(payload, notificationData, autoHandleLandingURL) | ||
| trackPushNotificationReceivedEvent( | ||
| payload.messageId, | ||
| payload.userJourneyId, | ||
| payload.userJourneyNodeId, | ||
| ) | ||
| ClixLogger.debug("Message received, notification sent to Clix SDK") | ||
| } catch (e: Exception) { | ||
| recoverReceivedMessageId(payload.messageId) | ||
| throw e | ||
| } | ||
| showNotification(payload, notificationData, autoHandleLandingURL) | ||
| trackPushNotificationReceivedEvent( | ||
| payload.messageId, | ||
| payload.userJourneyId, | ||
| payload.userJourneyNodeId, | ||
| ) | ||
| ClixLogger.debug("Message received, notification sent to Clix SDK") | ||
| } else { | ||
| ClixLogger.warn("Notification permission not granted, cannot show notification") | ||
| } | ||
| } catch (e: Exception) { | ||
| ClixLogger.error("Failed to handle notification received", e) | ||
| ClixLogger.error("Failed to handle push received", e) | ||
| } | ||
|
Comment on lines
77
to
79
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The new Useful? React with 👍 / 👎. |
||
| } | ||
|
|
||
|
|
@@ -191,7 +177,7 @@ internal class NotificationService( | |
| .setStyle(NotificationCompat.BigTextStyle().bigText(payload.body.orEmpty())) | ||
| .setTicker(payload.body.orEmpty()) | ||
| .setAutoCancel(true) | ||
| .setPriority(NotificationCompat.PRIORITY_HIGH) | ||
| .setPriority(NotificationCompat.PRIORITY_MAX) | ||
| .setCategory(NotificationCompat.CATEGORY_MESSAGE) | ||
| .setVisibility(NotificationCompat.VISIBILITY_PUBLIC) | ||
| .setDefaults(NotificationCompat.DEFAULT_ALL) | ||
|
|
@@ -239,7 +225,7 @@ internal class NotificationService( | |
| ) | ||
| } | ||
|
|
||
| private fun recordReceivedMessageId(messageId: String): Boolean { | ||
| internal fun recordReceivedMessageId(messageId: String): Boolean { | ||
| synchronized(this) { | ||
| val previous = storageService.get<String>(lastReceivedMessageIdKey) | ||
| if (previous == messageId) { | ||
|
|
@@ -250,7 +236,7 @@ internal class NotificationService( | |
| } | ||
| } | ||
|
|
||
| private fun recoverReceivedMessageId(messageId: String) { | ||
| internal fun recoverReceivedMessageId(messageId: String) { | ||
| synchronized(this) { | ||
| val previous = storageService.get<String>(lastReceivedMessageIdKey) | ||
| if (previous == messageId) { | ||
|
|
@@ -268,6 +254,7 @@ internal class NotificationService( | |
| description = descriptionText | ||
| enableVibration(true) | ||
| enableLights(true) | ||
| setShowBadge(true) | ||
| } | ||
| notificationManager.createNotificationChannel(channel) | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| [versions] | ||
| clix = "1.3.2" | ||
| clix = "1.3.3" | ||
| # Plugins | ||
| android-gradle-plugin = "8.9.2" | ||
| gms = "4.4.2" | ||
|
|
||
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.
Recovery block unreachable due to exception swallowing in handlePushReceived.
The try-catch at lines 276-279 intends to recover the messageId if
handlePushReceivedfails. However,handlePushReceivedinNotificationService.ktcatches all exceptions internally (lines 77-79) and never rethrows them. This means:handlePushReceivedfailuresshowNotificationortrackPushNotificationReceivedEventfails, the messageId remains marked as processedThis effectively makes transient failures permanent—the notification is never shown and the receive event is never tracked, with no retry possible.
To fix this, either:
handlePushReceivedso exceptions propagate (recommended)handlePushReceivedreturn a success/failure result that the caller can checkBased on learnings, side effects should be handled explicitly within service classes, but error propagation should allow callers to manage recovery.