fix: improve push notification processing logic and deduplication handling#16
fix: improve push notification processing logic and deduplication handling#16pitzcarraldo wants to merge 3 commits intomainfrom
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughBumps version to 1.3.3 and refactors notification processing: renames ClixNotification.handleIncomingPayload → handleNotificationReceived and NotificationService.handleNotificationReceived → handlePushReceived, moves duplicate-message filtering into ClixNotification, and makes record/recoverReceivedMessageId internal. Changes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } catch (e: Exception) { | ||
| ClixLogger.error("Failed to handle notification received", e) | ||
| ClixLogger.error("Failed to handle push received", e) | ||
| } |
There was a problem hiding this comment.
The new handlePushReceived swallows all exceptions (logging at lines 77-79), but ClixNotification.handleNotificationReceived now records the messageId before delegating (lines 231-235) and only clears it in its own catch (275-277). If showNotification or trackPushNotificationReceivedEvent throws, the exception is absorbed here so the caller never recovers the recorded messageId, leaving it marked as processed. Any retry or duplicate delivery of that messageId is then skipped as a duplicate, meaning transient notification/rendering failures now permanently drop the notification and receive event instead of retrying as before.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
clix/src/main/kotlin/so/clix/notification/ClixMessagingService.kt (2)
51-56: Consider adding error handling for consistency withonNewToken.The
onNewTokenhandler (lines 72-80) wraps its coroutine body in try-catch, butonMessageReceiveddoes not. IfhandleNotificationReceivedthrows, the exception will be silently swallowed by the coroutine scope. Based on learnings, side effects should be handled explicitly within service classes.🔎 Suggested improvement:
Clix.coroutineScope.launch { - Clix.Notification.handleNotificationReceived( - notificationData = notificationData, - payload = payload, - ) + try { + Clix.Notification.handleNotificationReceived( + notificationData = notificationData, + payload = payload, + ) + } catch (e: Exception) { + ClixLogger.error("handleNotificationReceived failure:", e) + } }
31-31: Minor: Non-idiomatic use of.let.The
letblock's return value is unused. Consider using anifstatement for clarity.🔎 Suggested improvement:
- message.data.isNotEmpty().let { ClixLogger.debug("Message data payload: ${message.data}") } + if (message.data.isNotEmpty()) { + ClixLogger.debug("Message data payload: ${message.data}") + }gradle/libs.versions.toml (1)
15-15: Update Kotlin version to 2.x in the version catalog.Kotlin 2.x is stable and recommended (released May 2024). Consider upgrading from 1.9.25 to a current Kotlin 2.x version in the catalog to align with coding guidelines and enable modern compiler features. This change is optional if reserved for a future release.
clix/src/main/kotlin/so/clix/services/NotificationService.kt (1)
228-246: Consider reordering internal methods to follow coding guidelines.The internal methods
recordReceivedMessageIdandrecoverReceivedMessageIdshould appear before private methods to follow the guideline "Order declarations Public → Internal → Private".As per coding guidelines, the declaration order should be Public → Internal → Private.
Suggested ordering
Move lines 228-246 to appear after line 140 (after the
reset()method and before the first private methodshowNotification).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
CHANGELOG.md(3 hunks)README.md(10 hunks)clix/src/main/kotlin/so/clix/core/ClixNotification.kt(2 hunks)clix/src/main/kotlin/so/clix/notification/ClixMessagingService.kt(1 hunks)clix/src/main/kotlin/so/clix/services/NotificationService.kt(3 hunks)gradle/libs.versions.toml(1 hunks)samples/basic-app/build.gradle.kts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (11)
clix/src/main/kotlin/so/clix/**
📄 CodeRabbit inference engine (.cursor/rules/guidelines.mdc)
Follow the specified library source structure under clix/src/main/kotlin/so/clix/(core|models|notification|services|utils)
Files:
clix/src/main/kotlin/so/clix/core/ClixNotification.ktclix/src/main/kotlin/so/clix/services/NotificationService.ktclix/src/main/kotlin/so/clix/notification/ClixMessagingService.kt
clix/src/main/kotlin/so/clix/{core,models}/**/*.kt
📄 CodeRabbit inference engine (.cursor/rules/guidelines.mdc)
Use sealed classes for finite states or results
Files:
clix/src/main/kotlin/so/clix/core/ClixNotification.kt
clix/src/main/kotlin/so/clix/core/**/*.kt
📄 CodeRabbit inference engine (.cursor/rules/guidelines.mdc)
clix/src/main/kotlin/so/clix/core/**/*.kt: Use functional error wrappers (sealed classes) in domain logic
Keep domain logic pure and side-effect-free
Files:
clix/src/main/kotlin/so/clix/core/ClixNotification.kt
clix/src/main/kotlin/so/clix/{core,services}/**/*.kt
📄 CodeRabbit inference engine (.cursor/rules/guidelines.mdc)
Throw exceptions only for unrecoverable errors
Files:
clix/src/main/kotlin/so/clix/core/ClixNotification.ktclix/src/main/kotlin/so/clix/services/NotificationService.kt
clix/src/main/kotlin/**/*.kt
📄 CodeRabbit inference engine (.cursor/rules/guidelines.mdc)
clix/src/main/kotlin/**/*.kt: Use Kotlin Coroutines for asynchronous operations
Ensure thread safety for shared mutable state
Write small, pure, composable functions; favor immutability and explicit side effects
Use descriptive names and prefer expression-style functions where clear
Order declarations Public → Internal → Private
Avoid reflection, annotations, and metaprogramming unless necessary
Do not use service locators or singletons (unless absolutely necessary)
No reflection under any circumstances
Use DataStore Preferences for local storage
Files:
clix/src/main/kotlin/so/clix/core/ClixNotification.ktclix/src/main/kotlin/so/clix/services/NotificationService.ktclix/src/main/kotlin/so/clix/notification/ClixMessagingService.kt
{gradle/libs.versions.toml,**/*.gradle.kts}
📄 CodeRabbit inference engine (.cursor/rules/guidelines.mdc)
Use Firebase via BOM and version catalog
Files:
gradle/libs.versions.tomlsamples/basic-app/build.gradle.kts
{gradle/libs.versions.toml,**/*.gradle.kts,detekt.yml}
📄 CodeRabbit inference engine (.cursor/rules/guidelines.mdc)
Use ktfmt, detekt, and jacoco via the Gradle version catalog
Files:
gradle/libs.versions.tomlsamples/basic-app/build.gradle.kts
**/*.gradle.kts
📄 CodeRabbit inference engine (.cursor/rules/guidelines.mdc)
Use Kotlin 2.x as the minimum Kotlin version (configure Kotlin plugin accordingly)
Files:
samples/basic-app/build.gradle.kts
samples/**
📄 CodeRabbit inference engine (.cursor/rules/guidelines.mdc)
Place example applications under samples/
Files:
samples/basic-app/build.gradle.kts
clix/src/main/kotlin/so/clix/services/**/*.kt
📄 CodeRabbit inference engine (.cursor/rules/guidelines.mdc)
clix/src/main/kotlin/so/clix/services/**/*.kt: Classes interacting with external services must use Service or Repository suffix
Handle side effects explicitly within repository/service classes
Files:
clix/src/main/kotlin/so/clix/services/NotificationService.kt
clix/src/main/kotlin/so/clix/{models,services}/**/*.kt
📄 CodeRabbit inference engine (.cursor/rules/guidelines.mdc)
clix/src/main/kotlin/so/clix/{models,services}/**/*.kt: Use kotlinx.serialization for JSON handling
Map between domain models and DTOs at the boundaries
Files:
clix/src/main/kotlin/so/clix/services/NotificationService.kt
🧠 Learnings (13)
📚 Learning: 2025-10-15T05:36:01.516Z
Learnt from: CR
Repo: clix-so/clix-android-sdk PR: 0
File: .cursor/rules/guidelines.mdc:0-0
Timestamp: 2025-10-15T05:36:01.516Z
Learning: Applies to clix/src/main/kotlin/so/clix/** : Follow the specified library source structure under clix/src/main/kotlin/so/clix/(core|models|notification|services|utils)
Applied to files:
gradle/libs.versions.tomlsamples/basic-app/build.gradle.ktsREADME.md
📚 Learning: 2025-10-15T05:36:01.516Z
Learnt from: CR
Repo: clix-so/clix-android-sdk PR: 0
File: .cursor/rules/guidelines.mdc:0-0
Timestamp: 2025-10-15T05:36:01.516Z
Learning: Applies to {clix/build.gradle.kts,clix/src/main/AndroidManifest.xml} : Set Android minSdk to API 21 or higher
Applied to files:
gradle/libs.versions.tomlsamples/basic-app/build.gradle.ktsREADME.md
📚 Learning: 2025-10-15T05:36:01.516Z
Learnt from: CR
Repo: clix-so/clix-android-sdk PR: 0
File: .cursor/rules/guidelines.mdc:0-0
Timestamp: 2025-10-15T05:36:01.516Z
Learning: Applies to {build.gradle.kts,settings.gradle.kts,clix/build.gradle.kts,gradle.properties,gradle/wrapper/gradle-wrapper.properties} : Target Gradle 8.x with Kotlin DSL and JVM 21 toolchain
Applied to files:
samples/basic-app/build.gradle.kts
📚 Learning: 2025-10-15T05:36:01.516Z
Learnt from: CR
Repo: clix-so/clix-android-sdk PR: 0
File: .cursor/rules/guidelines.mdc:0-0
Timestamp: 2025-10-15T05:36:01.516Z
Learning: Applies to clix/src/main/kotlin/so/clix/{models,services}/**/*.kt : Use kotlinx.serialization for JSON handling
Applied to files:
samples/basic-app/build.gradle.ktsREADME.mdclix/src/main/kotlin/so/clix/notification/ClixMessagingService.kt
📚 Learning: 2025-10-15T05:36:01.516Z
Learnt from: CR
Repo: clix-so/clix-android-sdk PR: 0
File: .cursor/rules/guidelines.mdc:0-0
Timestamp: 2025-10-15T05:36:01.516Z
Learning: Applies to clix/src/{test,androidTest}/kotlin/**/*.kt : Use JUnit4/Kotest for unit tests and AndroidX Test/Robolectric for instrumentation
Applied to files:
samples/basic-app/build.gradle.ktsREADME.md
📚 Learning: 2025-10-15T05:36:01.516Z
Learnt from: CR
Repo: clix-so/clix-android-sdk PR: 0
File: .cursor/rules/guidelines.mdc:0-0
Timestamp: 2025-10-15T05:36:01.516Z
Learning: Applies to clix/src/test/kotlin/so/clix/** : Mirror main source structure for unit tests under clix/src/test/kotlin/so/clix
Applied to files:
samples/basic-app/build.gradle.ktsREADME.md
📚 Learning: 2025-10-15T05:36:01.516Z
Learnt from: CR
Repo: clix-so/clix-android-sdk PR: 0
File: .cursor/rules/guidelines.mdc:0-0
Timestamp: 2025-10-15T05:36:01.516Z
Learning: Applies to clix/src/main/kotlin/**/*.kt : Use Kotlin Coroutines for asynchronous operations
Applied to files:
samples/basic-app/build.gradle.ktsREADME.md
📚 Learning: 2025-10-15T05:36:01.516Z
Learnt from: CR
Repo: clix-so/clix-android-sdk PR: 0
File: .cursor/rules/guidelines.mdc:0-0
Timestamp: 2025-10-15T05:36:01.516Z
Learning: Applies to clix/src/main/kotlin/so/clix/utils/**/*.kt : Group utility classes/functions logically under utils (and its subpackages)
Applied to files:
samples/basic-app/build.gradle.ktsREADME.md
📚 Learning: 2025-10-15T05:36:01.516Z
Learnt from: CR
Repo: clix-so/clix-android-sdk PR: 0
File: .cursor/rules/guidelines.mdc:0-0
Timestamp: 2025-10-15T05:36:01.516Z
Learning: Applies to clix/src/main/kotlin/so/clix/models/**/*.kt : Keep domain models serialization-agnostic (no framework annotations in core models)
Applied to files:
samples/basic-app/build.gradle.ktsREADME.md
📚 Learning: 2025-10-15T05:36:01.516Z
Learnt from: CR
Repo: clix-so/clix-android-sdk PR: 0
File: .cursor/rules/guidelines.mdc:0-0
Timestamp: 2025-10-15T05:36:01.516Z
Learning: Applies to clix/src/main/kotlin/so/clix/models/**/*.kt : Define domain models as immutable data classes
Applied to files:
samples/basic-app/build.gradle.kts
📚 Learning: 2025-10-15T05:36:01.516Z
Learnt from: CR
Repo: clix-so/clix-android-sdk PR: 0
File: .cursor/rules/guidelines.mdc:0-0
Timestamp: 2025-10-15T05:36:01.516Z
Learning: Applies to clix/src/main/kotlin/**/*.kt : Write small, pure, composable functions; favor immutability and explicit side effects
Applied to files:
samples/basic-app/build.gradle.kts
📚 Learning: 2025-10-15T05:36:01.516Z
Learnt from: CR
Repo: clix-so/clix-android-sdk PR: 0
File: .cursor/rules/guidelines.mdc:0-0
Timestamp: 2025-10-15T05:36:01.516Z
Learning: Applies to clix/src/main/kotlin/so/clix/services/**/*.kt : Classes interacting with external services must use Service or Repository suffix
Applied to files:
README.mdclix/src/main/kotlin/so/clix/notification/ClixMessagingService.kt
📚 Learning: 2025-10-15T05:36:01.516Z
Learnt from: CR
Repo: clix-so/clix-android-sdk PR: 0
File: .cursor/rules/guidelines.mdc:0-0
Timestamp: 2025-10-15T05:36:01.516Z
Learning: Applies to clix/src/main/kotlin/so/clix/services/**/*.kt : Handle side effects explicitly within repository/service classes
Applied to files:
clix/src/main/kotlin/so/clix/services/NotificationService.ktclix/src/main/kotlin/so/clix/notification/ClixMessagingService.kt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (9)
gradle/libs.versions.toml (1)
2-2: LGTM - Version bump aligned with changelog.The version bump to 1.3.3 is consistent with the CHANGELOG.md entry and the PR's deduplication fix.
samples/basic-app/build.gradle.kts (1)
40-41: LGTM - Version reference updated in comment.The commented dependency correctly reflects the new 1.3.3 version. Using
project(":clix")for local development is appropriate, with the comment serving as documentation for the published artifact version.CHANGELOG.md (1)
7-12: LGTM - Changelog entry accurately documents the fix.The entry clearly describes the duplicate event tracking fix, follows the Keep a Changelog format, and aligns with the code changes in
ClixNotification.ktandNotificationService.kt.README.md (2)
23-23: LGTM - Version updated consistently.The dependency version is correctly updated to 1.3.3, matching the version catalog and changelog.
3-5: Formatting changes look good.The line wrapping adjustments throughout the README improve readability without changing content.
clix/src/main/kotlin/so/clix/services/NotificationService.kt (2)
60-80: LGTM! Clean separation of concerns.The refactored logic correctly delegates deduplication to the calling layer while focusing this method on notification display and event tracking. The error handling appropriately catches and logs exceptions without rethrowing, following the guideline for service classes.
228-246: Well-designed deduplication helpers with proper thread safety.The synchronized blocks ensure thread-safe access to shared state, and the conditional removal in
recoverReceivedMessageId(line 241-243) correctly handles cases where the stored ID might have been replaced by a different message.clix/src/main/kotlin/so/clix/core/ClixNotification.kt (2)
55-57: LGTM! Cleaner log formatting.The single-line string template improves readability without changing functionality.
227-279: Excellent refactoring with robust error handling and deduplication.The refactored flow correctly implements:
- Early duplicate detection via
recordReceivedMessageId(line 231)- Proper recovery when the foreground handler suppresses notifications (line 255)
- Comprehensive error handling with nested try-catch blocks
- Thread-safe handler access by capturing volatile references
The error recovery strategy is sound: message IDs are recovered only when suppressed or when unexpected exceptions occur in the handling logic, but not when
handlePushReceivedfails (since it catches exceptions internally). This prevents duplicate notifications from being shown on re-delivery while accepting that individual push failures won't be retried.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
clix/src/main/kotlin/so/clix/services/NotificationService.kt (1)
60-80: Error handling now defers dedup recovery to caller.The simplified
handlePushReceivedno longer manages deduplication internally. This is appropriate sinceClixNotification.handleNotificationReceivednow handlesrecordReceivedMessageIdbefore calling this method andrecoverReceivedMessageIdin its own catch block (lines 276-279 in ClixNotification.kt).However, if
showNotificationortrackPushNotificationReceivedEventthrows, the exception is caught here (line 77-79) and logged, but it won't propagate to the caller. This meansClixNotificationwon't know to recover the messageId for transient failures.Consider re-throwing or returning a result to allow the caller to handle recovery:
🔎 Suggested approach
- suspend fun handlePushReceived( + suspend fun handlePushReceived( payload: ClixPushNotificationPayload, notificationData: Map<String, Any?>, autoHandleLandingURL: Boolean = true, ) { - try { - if (hasNotificationPermission(context)) { - 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 push received", e) + if (hasNotificationPermission(context)) { + 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") } }This allows the caller (
ClixNotification.handleNotificationReceived) to catch exceptions and properly recover the messageId.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
clix/src/main/kotlin/so/clix/core/ClixNotification.kt(1 hunks)clix/src/main/kotlin/so/clix/services/NotificationService.kt(5 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
clix/src/main/kotlin/so/clix/**
📄 CodeRabbit inference engine (.cursor/rules/guidelines.mdc)
Follow the specified library source structure under clix/src/main/kotlin/so/clix/(core|models|notification|services|utils)
Files:
clix/src/main/kotlin/so/clix/core/ClixNotification.ktclix/src/main/kotlin/so/clix/services/NotificationService.kt
clix/src/main/kotlin/so/clix/{core,models}/**/*.kt
📄 CodeRabbit inference engine (.cursor/rules/guidelines.mdc)
Use sealed classes for finite states or results
Files:
clix/src/main/kotlin/so/clix/core/ClixNotification.kt
clix/src/main/kotlin/so/clix/core/**/*.kt
📄 CodeRabbit inference engine (.cursor/rules/guidelines.mdc)
clix/src/main/kotlin/so/clix/core/**/*.kt: Use functional error wrappers (sealed classes) in domain logic
Keep domain logic pure and side-effect-free
Files:
clix/src/main/kotlin/so/clix/core/ClixNotification.kt
clix/src/main/kotlin/so/clix/{core,services}/**/*.kt
📄 CodeRabbit inference engine (.cursor/rules/guidelines.mdc)
Throw exceptions only for unrecoverable errors
Files:
clix/src/main/kotlin/so/clix/core/ClixNotification.ktclix/src/main/kotlin/so/clix/services/NotificationService.kt
clix/src/main/kotlin/**/*.kt
📄 CodeRabbit inference engine (.cursor/rules/guidelines.mdc)
clix/src/main/kotlin/**/*.kt: Use Kotlin Coroutines for asynchronous operations
Ensure thread safety for shared mutable state
Write small, pure, composable functions; favor immutability and explicit side effects
Use descriptive names and prefer expression-style functions where clear
Order declarations Public → Internal → Private
Avoid reflection, annotations, and metaprogramming unless necessary
Do not use service locators or singletons (unless absolutely necessary)
No reflection under any circumstances
Use DataStore Preferences for local storage
Files:
clix/src/main/kotlin/so/clix/core/ClixNotification.ktclix/src/main/kotlin/so/clix/services/NotificationService.kt
clix/src/main/kotlin/so/clix/services/**/*.kt
📄 CodeRabbit inference engine (.cursor/rules/guidelines.mdc)
clix/src/main/kotlin/so/clix/services/**/*.kt: Classes interacting with external services must use Service or Repository suffix
Handle side effects explicitly within repository/service classes
Files:
clix/src/main/kotlin/so/clix/services/NotificationService.kt
clix/src/main/kotlin/so/clix/{models,services}/**/*.kt
📄 CodeRabbit inference engine (.cursor/rules/guidelines.mdc)
clix/src/main/kotlin/so/clix/{models,services}/**/*.kt: Use kotlinx.serialization for JSON handling
Map between domain models and DTOs at the boundaries
Files:
clix/src/main/kotlin/so/clix/services/NotificationService.kt
🧠 Learnings (1)
📚 Learning: 2025-10-15T05:36:01.516Z
Learnt from: CR
Repo: clix-so/clix-android-sdk PR: 0
File: .cursor/rules/guidelines.mdc:0-0
Timestamp: 2025-10-15T05:36:01.516Z
Learning: Applies to clix/src/main/kotlin/so/clix/services/**/*.kt : Handle side effects explicitly within repository/service classes
Applied to files:
clix/src/main/kotlin/so/clix/services/NotificationService.kt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (6)
clix/src/main/kotlin/so/clix/services/NotificationService.kt (3)
180-180: Notification priority set to MAX.
PRIORITY_MAXis intended for time-critical notifications that require immediate user attention (e.g., incoming calls, alarms). For general push notifications,PRIORITY_HIGHis typically more appropriate to avoid aggressive notification behavior that users may find intrusive.Is
PRIORITY_MAXintentional for this SDK's use case? If the SDK handles various notification types, consider making the priority configurable or defaulting toPRIORITY_HIGH.
228-246: Visibility changes enable proper deduplication flow.Making
recordReceivedMessageIdandrecoverReceivedMessageIdinternal allowsClixNotificationto manage the deduplication lifecycle correctly—recording before processing and recovering on failure.Note that this approach only deduplicates against the single last-received messageId. If multiple notifications arrive in rapid succession before processing completes, earlier duplicates won't be caught. This is likely acceptable for typical push notification patterns but worth documenting.
257-257: Badge display enabled.
setShowBadge(true)enables app icon badges for this notification channel. This is a reasonable default for push notifications to improve user awareness of unread notifications.clix/src/main/kotlin/so/clix/core/ClixNotification.kt (3)
228-236: Deduplication check at entry point is well-placed.Recording the messageId at the start of processing and immediately returning for duplicates is the correct approach. This prevents duplicate processing regardless of where failures occur later in the flow.
240-259: Foreground handler suppression correctly recovers messageId.When the foreground handler returns
falseto suppress the notification, callingrecoverReceivedMessageId(line 256) ensures the message can be reprocessed if needed. The fallback totrueon handler exception (line 250) is sensible—display the notification rather than silently drop it.
260-269: Background handler errors are appropriately isolated.The background handler's exceptions are caught and logged without affecting the notification display flow. This matches the documented behavior: "notification always displayed" for background messages.
| Clix.notificationService.handlePushReceived( | ||
| payload = payload, | ||
| notificationData = notificationData, | ||
| autoHandleLandingURL = autoHandleLandingURL, | ||
| ) | ||
| } catch (e: Exception) { | ||
| ClixLogger.error("Failed to handle notification received", e) | ||
| Clix.notificationService.recoverReceivedMessageId(payload.messageId) | ||
| } |
There was a problem hiding this comment.
Recovery block unreachable due to exception swallowing in handlePushReceived.
The try-catch at lines 276-279 intends to recover the messageId if handlePushReceived fails. However, handlePushReceived in NotificationService.kt catches all exceptions internally (lines 77-79) and never rethrows them. This means:
- The catch block here will never execute for
handlePushReceivedfailures - If
showNotificationortrackPushNotificationReceivedEventfails, the messageId remains marked as processed - Subsequent retries or duplicate deliveries of that message will be skipped as duplicates
This 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:
- Remove the try-catch in
handlePushReceivedso exceptions propagate (recommended) - Have
handlePushReceivedreturn a success/failure result that the caller can check
Based on learnings, side effects should be handled explicitly within service classes, but error propagation should allow callers to manage recovery.
1 participant
Summary by CodeRabbit
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.