[webhooks] add sms:received webhook registration notification#229
[webhooks] add sms:received webhook registration notification#229
sms:received webhook registration notification#229Conversation
WalkthroughThe updates introduce a notification mechanism for newly registered SMS-received webhooks. MainActivity now supports launching directly to a specific tab via intent extras. NotificationsService can create and display a new type of notification for webhook registration events. WebHooksService detects new webhook registrations and triggers the notification. Supporting database queries, icon, string resources, and a periodic worker for reviewing webhooks were also added. Changes
Sequence Diagram(s)sequenceDiagram
participant ExternalComponent
participant MainActivity
participant NotificationsService
participant WebHooksService
participant WebHooksDao
ExternalComponent->>MainActivity: Start with Intent (tabIndex)
MainActivity->>MainActivity: processIntent(intent)
MainActivity->>MainActivity: Set ViewPager to tabIndex
Note over WebHooksService,NotificationsService: Webhook registration flow
WebHooksService->>WebHooksDao: exists(source, id)
WebHooksDao-->>WebHooksService: Boolean (exists)
WebHooksService->>WebHooksService: Check if webhook is new and type=SmsReceived
alt New SMSReceived webhook
WebHooksService->>NotificationsService: notify(context, NOTIFICATION_ID_SMS_RECEIVED_WEBHOOK, message)
NotificationsService->>MainActivity: Create PendingIntent (tabIndex=MESSAGES)
NotificationsService->>NotificationsService: Build and display notification
end
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
app/src/main/java/me/capcom/smsgateway/modules/webhooks/WebHooksService.kt (1)
54-57: Consider refactoring duplicate notification logic.The notification logic for new SMS-received webhooks is duplicated between
sync()andreplace()methods. Consider extracting this into a helper method to follow the DRY principle.Extract the notification logic into a helper method:
+ private fun notifyIfNewSmsReceivedWebhook(webHooks: List<WebHookDTO>, existingIds: Set<String>) { + if (webHooks.any { it.id !in existingIds && it.event == WebHookEvent.SmsReceived }) { + notifyUser() + } + } + fun sync(source: EntitySource, webHooks: List<WebHookDTO>) { val ids = webHooksDao.selectBySource(source).map { it.id }.toSet() - if (webHooks.any { it.id !in ids && it.event == WebHookEvent.SmsReceived }) { - notifyUser() - } + notifyIfNewSmsReceivedWebhook(webHooks, ids)app/src/main/java/me/capcom/smsgateway/modules/notifications/NotificationsService.kt (1)
71-81: Complex content intent logic could be simplified.The content intent resolution logic is functional but complex. Consider extracting it into a separate method for better readability.
+ private fun resolveContentIntent(context: Context, id: Int, contentIntent: PendingIntent?): PendingIntent { + return contentIntent ?: PendingIntent.getActivity( + context, + 0, + contentIntentFactories[id]?.invoke(context) + ?: context.packageManager.getLaunchIntentForPackage(context.packageName), + PendingIntent.FLAG_IMMUTABLE + ) + } + fun makeNotification( context: Context, id: Int, contentText: String, contentIntent: PendingIntent? = null ): Notification { return NotificationCompat.Builder(context, NOTIFICATION_CHANNEL_ID) .setContentTitle(context.getText(R.string.notification_title)) .setContentText(contentText) .setSmallIcon(icons[id] ?: R.drawable.ic_sms) - .setContentIntent( - contentIntent - ?: PendingIntent.getActivity( - context, - 0, - contentIntentFactories[id]?.invoke(context) - ?: context.packageManager.getLaunchIntentForPackage(context.packageName), - PendingIntent.FLAG_IMMUTABLE - ) - ) + .setContentIntent(resolveContentIntent(context, id, contentIntent)) .apply { builders[id]?.invoke(this) } .build() }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/src/main/java/me/capcom/smsgateway/MainActivity.kt(3 hunks)app/src/main/java/me/capcom/smsgateway/modules/notifications/NotificationsService.kt(4 hunks)app/src/main/java/me/capcom/smsgateway/modules/webhooks/WebHooksService.kt(5 hunks)app/src/main/java/me/capcom/smsgateway/modules/webhooks/db/WebHooksDao.kt(1 hunks)app/src/main/res/drawable/notif_webhook_registered.xml(1 hunks)app/src/main/res/values/strings.xml(1 hunks)
🔇 Additional comments (18)
app/src/main/res/values/strings.xml (1)
128-128: LGTM! String resource properly added.The new string resource follows proper naming conventions and provides clear descriptive text for the webhook registration notification feature.
app/src/main/java/me/capcom/smsgateway/modules/webhooks/db/WebHooksDao.kt (1)
24-25: Excellent implementation of the exists method.The use of
SELECT EXISTSis more efficient than counting rows for existence checking. The method signature and SQL query are correctly structured to check for webhook existence by source and ID.app/src/main/res/drawable/notif_webhook_registered.xml (1)
1-12: Well-implemented notification icon.The vector drawable follows Android standards with appropriate sizing (24dp) and white tint for notification display. The warning triangle with exclamation mark is a fitting icon for webhook registration alerts.
app/src/main/java/me/capcom/smsgateway/MainActivity.kt (4)
3-4: LGTM! Proper imports added.The Context and Intent imports are necessary for the new tab navigation functionality.
46-47: Good placement of processIntent call.Calling
processIntentafter setting up the adapter ensures the ViewPager is properly initialized before attempting to set the current item.
49-52: Proper handling of new intents.Overriding
onNewIntentensures that tab navigation works even when the activity is already running.
75-87: Well-designed companion object with proper constants.The tab index constants correctly match the actual tab positions, and the starter method provides a clean API for external components to launch specific tabs.
app/src/main/java/me/capcom/smsgateway/modules/webhooks/WebHooksService.kt (4)
6-6: Import addition looks good.The R import is necessary for accessing string resources in the notification method.
10-10: Dependency injection properly implemented.The NotificationsService dependency is correctly added to the constructor and will be properly injected by the DI framework.
Also applies to: 26-26
137-144: Notification method implementation looks correct.The
notifyUser()method properly retrieves the Android context through Koin's dependency injection and calls the notification service with the appropriate notification ID and localized string resource.
88-88:Details
✅ Verification successful
Verify the existence of the
existsmethod.The code uses
webHooksDao.exists(source, webHookEntity.id)which is mentioned as newly added in the summary. Ensure this method is properly implemented in the WebHooksDao interface.
🏁 Script executed:
#!/bin/bash # Description: Verify the exists method is implemented in WebHooksDao # Expected: Find the exists method declaration and implementation ast-grep --pattern 'fun exists($_, $_): $_'Length of output: 45
🏁 Script executed:
#!/bin/bash # Search for the 'exists' method declaration in the WebHooksDao interface rg "interface WebHooksDao" -n . rg "fun exists" -n . # Also check for any extension or Java implementation rg "exists(" -n app/src/main/java/me/capcom/smsgateway/modules/webhooksLength of output: 435
existsmethod implementation verified`
- Confirmed
fun exists(source: EntitySource, id: String): Booleanis declared in
app/src/main/java/me/capcom/smsgateway/modules/webhooks/db/WebHooksDao.kt:25No further action required.
app/src/main/java/me/capcom/smsgateway/modules/notifications/NotificationsService.kt (7)
6-6: Import additions are necessary and appropriate.The PendingIntent and MainActivity imports are required for the new notification content intent functionality.
Also applies to: 10-10
28-32: Notification configuration is appropriate.Setting high priority and auto-cancel for webhook registration notifications is a good UX choice - users should be promptly informed of new webhooks and the notification should dismiss when tapped.
53-59: New notify method provides good API simplification.The simplified
notifymethod is a good addition that makes the notification service easier to use for common cases.
61-66: Method signature enhancement is well-designed.Making the
contentIntentparameter optional with a default value maintains backward compatibility while allowing for customization.
92-92: New notification ID constant follows the existing pattern.The new notification ID constant is appropriately defined and follows the sequential numbering pattern.
25-25:Details
✅ Verification successful
Verify the notification icon resource exists.
Ensure the
notif_webhook_registered.xmldrawable resource has been created and is available in the resources.
🏁 Script executed:
#!/bin/bash # Description: Verify the notification icon resource exists # Expected: Find the notif_webhook_registered.xml file fd "notif_webhook_registered.xml" --type fLength of output: 97
No action needed –
notif_webhook_registered.xmlexists
- Verified the drawable resource at
app/src/main/res/drawable/notif_webhook_registered.xml.- The notification icon reference in
NotificationsService.ktis correct and requires no changes.
35-37:Details
❓ Verification inconclusive
Verify the correct tab index is being used.
The code uses
MainActivity.TAB_INDEX_SETTINGSbut the AI summary mentions this should navigate to the MESSAGES tab. Please verify which tab is intended for webhook notifications.
🏁 Script executed:
#!/bin/bash # Description: Verify MainActivity tab constants and their intended usage # Expected: Find tab index constants and understand which one is appropriate for webhooks ast-grep --pattern 'const val TAB_INDEX_$_ = $_'Length of output: 50
🏁 Script executed:
#!/bin/bash # Search for all TAB_INDEX constants across Kotlin files rg -n "TAB_INDEX_" -A2 -w --glob '*.kt'Length of output: 39
🏁 Script executed:
#!/bin/bash # Locate MainActivity.kt and search for tab index constants MAIN_FILE=$(find . -path '*app/src/main/java*' -type f -name 'MainActivity.kt' | head -n1) echo "MainActivity file: $MAIN_FILE" if [[ -n "$MAIN_FILE" ]]; then rg -n "TAB_INDEX_" -A2 -B2 "$MAIN_FILE" || echo "No TAB_INDEX_ constants found in $MAIN_FILE" else echo "MainActivity.kt not found" fiLength of output: 873
Confirm webhook notification tab index
The
NotificationsServiceis currently opening MainActivity withTAB_INDEX_SETTINGS(value 2), but if webhook notifications should land in the Messages tab you’ll need to switch toTAB_INDEX_MESSAGES(value 1).• File: app/src/main/java/me/capcom/smsgateway/modules/notifications/NotificationsService.kt
Lines 35–37:NOTIFICATION_ID_SMS_RECEIVED_WEBHOOK to { context: Context -> MainActivity.starter(context, MainActivity.TAB_INDEX_SETTINGS) }• MainActivity tab constants (app/src/main/java/me/capcom/smsgateway/MainActivity.kt):
const val TAB_INDEX_HOME = 0 const val TAB_INDEX_MESSAGES = 1 const val TAB_INDEX_SETTINGS = 2Please verify which tab users should see when tapping a webhook notification and update accordingly.
| private fun processIntent(intent: Intent) { | ||
| val tabIndex = intent.getIntExtra(EXTRA_TAB_INDEX, TAB_INDEX_HOME) | ||
|
|
||
| binding.viewPager.currentItem = tabIndex | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for tab index parameter.
The method should validate that the tab index is within the valid range to prevent potential issues.
private fun processIntent(intent: Intent) {
val tabIndex = intent.getIntExtra(EXTRA_TAB_INDEX, TAB_INDEX_HOME)
-
- binding.viewPager.currentItem = tabIndex
+
+ // Validate tab index is within valid range
+ if (tabIndex in 0..2) {
+ binding.viewPager.currentItem = tabIndex
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private fun processIntent(intent: Intent) { | |
| val tabIndex = intent.getIntExtra(EXTRA_TAB_INDEX, TAB_INDEX_HOME) | |
| binding.viewPager.currentItem = tabIndex | |
| } | |
| private fun processIntent(intent: Intent) { | |
| val tabIndex = intent.getIntExtra(EXTRA_TAB_INDEX, TAB_INDEX_HOME) | |
| // Validate tab index is within valid range | |
| if (tabIndex in 0..2) { | |
| binding.viewPager.currentItem = tabIndex | |
| } | |
| } |
🤖 Prompt for AI Agents
In app/src/main/java/me/capcom/smsgateway/MainActivity.kt around lines 54 to 58,
the processIntent method retrieves a tab index from the intent but does not
validate it. Add validation to ensure the tab index is within the valid range of
available tabs before setting binding.viewPager.currentItem. If the index is out
of range, default to a safe value like TAB_INDEX_HOME to prevent potential
crashes or unexpected behavior.
| if (!exists && webHook.event == WebHookEvent.SmsReceived) { | ||
| notifyUser() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for notification calls.
The notification call should be wrapped in error handling to prevent the webhook registration from failing if the notification system encounters issues.
// Show notification if this is an sms:received webhook
if (!exists && webHook.event == WebHookEvent.SmsReceived) {
- notifyUser()
+ try {
+ notifyUser()
+ } catch (e: Exception) {
+ // Log error but don't fail webhook registration
+ // Consider using a logging framework here
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!exists && webHook.event == WebHookEvent.SmsReceived) { | |
| notifyUser() | |
| } | |
| // Show notification if this is an sms:received webhook | |
| if (!exists && webHook.event == WebHookEvent.SmsReceived) { | |
| try { | |
| notifyUser() | |
| } catch (e: Exception) { | |
| // Log error but don't fail webhook registration | |
| // Consider using a logging framework here | |
| } | |
| } |
🤖 Prompt for AI Agents
In app/src/main/java/me/capcom/smsgateway/modules/webhooks/WebHooksService.kt
around lines 92 to 94, the notifyUser() call is not wrapped in error handling,
which could cause the webhook registration to fail if notification fails. Wrap
the notifyUser() call in a try-catch block to catch any exceptions and handle
them gracefully, such as logging the error without interrupting the webhook
registration flow.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/src/main/java/me/capcom/smsgateway/modules/webhooks/workers/ReviewWebhooksWorker.kt (1)
42-63: Consider making the review interval configurable.The companion object methods are well-implemented with proper use of WorkManager's unique periodic work functionality. The KEEP policy correctly prevents duplicate scheduling.
Consider making the 30-day interval configurable:
companion object { private const val NAME = "ReviewWebhooksWorker" + private const val DEFAULT_REVIEW_INTERVAL_DAYS = 30L - fun start(context: Context) { + fun start(context: Context, intervalDays: Long = DEFAULT_REVIEW_INTERVAL_DAYS) { val work = PeriodicWorkRequestBuilder<ReviewWebhooksWorker>( - 30, + intervalDays, TimeUnit.DAYS )This would allow for different review intervals in testing or configuration while maintaining the default behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/src/main/java/me/capcom/smsgateway/modules/webhooks/WebHooksService.kt(5 hunks)app/src/main/java/me/capcom/smsgateway/modules/webhooks/workers/ReviewWebhooksWorker.kt(1 hunks)app/src/main/res/values/strings.xml(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/src/main/res/values/strings.xml
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/me/capcom/smsgateway/modules/webhooks/WebHooksService.kt
🔇 Additional comments (2)
app/src/main/java/me/capcom/smsgateway/modules/webhooks/workers/ReviewWebhooksWorker.kt (2)
1-16: LGTM! Well-organized imports.The imports are comprehensive and all appear to be used in the implementation. Good use of Android WorkManager for background tasks and Koin for dependency injection.
17-22: LGTM! Proper dependency injection setup.Good use of CoroutineWorker for background tasks and Koin for dependency injection. This promotes testability and follows Android best practices.
| override suspend fun doWork(): Result { | ||
| val webhooks = webHooksService.select(null) | ||
| val smsReceivedWebhooks = webhooks.filter { it.event == WebHookEvent.SmsReceived } | ||
| if (smsReceivedWebhooks.isEmpty()) { | ||
| return Result.success() | ||
| } | ||
|
|
||
| notificationsService.notify( | ||
| applicationContext, | ||
| NotificationsService.NOTIFICATION_ID_SMS_RECEIVED_WEBHOOK, | ||
| applicationContext.getString( | ||
| R.string.you_have_sms_received_webhooks_registered_please_review_them_to_avoid_any_security_risks, | ||
| smsReceivedWebhooks.size | ||
| ) | ||
| ) | ||
|
|
||
| return Result.success() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and consider performance optimization.
The main work logic has several areas for improvement:
- Missing error handling: Wrap the work in try-catch to handle potential exceptions from service calls.
- Performance concern: Filtering webhooks in memory may not scale well. Consider adding a database query method to filter
SmsReceivedwebhooks directly. - Missing logging: Add logging for debugging and monitoring purposes.
override suspend fun doWork(): Result {
+ return try {
val webhooks = webHooksService.select(null)
val smsReceivedWebhooks = webhooks.filter { it.event == WebHookEvent.SmsReceived }
if (smsReceivedWebhooks.isEmpty()) {
return Result.success()
}
notificationsService.notify(
applicationContext,
NotificationsService.NOTIFICATION_ID_SMS_RECEIVED_WEBHOOK,
applicationContext.getString(
R.string.you_have_sms_received_webhooks_registered_please_review_them_to_avoid_any_security_risks,
smsReceivedWebhooks.size
)
)
return Result.success()
+ } catch (e: Exception) {
+ Log.e("ReviewWebhooksWorker", "Failed to review webhooks", e)
+ Result.failure()
+ }
}Performance optimization suggestion: Consider adding a method to WebHooksService like selectByEvent(event: WebHookEvent) to filter at the database level instead of in memory.
🤖 Prompt for AI Agents
In
app/src/main/java/me/capcom/smsgateway/modules/webhooks/workers/ReviewWebhooksWorker.kt
around lines 23 to 40, the doWork method lacks error handling, logging, and has
a performance issue due to filtering webhooks in memory. Wrap the entire work
logic in a try-catch block to handle exceptions gracefully and log errors or
important steps for monitoring. Refactor the code to call a new method in
WebHooksService, such as selectByEvent(WebHookEvent.SmsReceived), to fetch only
relevant webhooks directly from the database instead of filtering in memory,
improving performance and scalability.
🤖 Pull request artifacts
|
054dcdf to
93b8964
Compare
93b8964 to
738f178
Compare
Summary by CodeRabbit