[receiver] add body of mms to webhook #303
Conversation
WalkthroughMMS handling now waits for MMS content in the Android MMS ContentProvider (with a 30s timeout) to extract text parts before processing; an optional Changes
Sequence Diagram(s)sequenceDiagram
participant Android as Android System
participant Receiver as MmsReceiver
participant Provider as MMS ContentProvider
participant Service as ReceiverService
participant Webhook as Webhook Endpoint
Android->>Receiver: MMS intent / notification (transactionId, messageId, etc.)
Receiver->>Receiver: Check de-dup cache (transactionId)
alt already processed
Receiver-->>Android: Skip processing
else new transaction
Receiver->>Provider: Observe/query for MMS (by transactionId/address/minMmsId)
par wait for parts (up to 30s)
Provider-->>Receiver: MMS parts (when available)
and timeout
Receiver->>Provider: Fallback query (latest message)
Provider-->>Receiver: MMS parts or none
end
Receiver->>Receiver: verify sender/address, extract text parts -> body (or null)
Receiver->>Service: processMmsMessage(..., body)
Service->>Service: Build MmsReceivedPayload(body=...)
Service->>Webhook: POST payload (includes body)
Webhook-->>Service: response
Receiver->>Receiver: Update de-dup cache / logs
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
app/src/main/java/me/capcom/smsgateway/modules/receiver/MmsReceiver.kt (1)
159-170: Consider limitingbodyPreviewlogging to avoid PII exposure.The body preview (up to 200 characters) is logged at DEBUG level. While useful for debugging, this could expose sensitive message content in logs. Consider reducing the preview length or ensuring DEBUG logs are not persisted in production.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/src/main/java/me/capcom/smsgateway/modules/receiver/MmsReceiver.ktapp/src/main/java/me/capcom/smsgateway/modules/receiver/ReceiverService.ktapp/src/main/java/me/capcom/smsgateway/modules/receiver/data/InboxMessage.ktapp/src/main/java/me/capcom/smsgateway/modules/webhooks/payload/MmsReceivedPayload.kt
🔇 Additional comments (5)
app/src/main/java/me/capcom/smsgateway/modules/webhooks/payload/MmsReceivedPayload.kt (1)
13-13: LGTM!The new
bodyfield is correctly added as nullable to handle cases where body extraction fails or MMS contains no text parts.app/src/main/java/me/capcom/smsgateway/modules/receiver/ReceiverService.kt (1)
98-98: LGTM!The body field is correctly propagated from the message to the webhook payload.
app/src/main/java/me/capcom/smsgateway/modules/receiver/MmsReceiver.kt (2)
69-83: De-duplication logic looks correct.The synchronized block properly protects the shared map, and cleanup of old entries prevents unbounded memory growth.
172-182: LGTM!The MMS message construction correctly includes the extracted body field.
app/src/main/java/me/capcom/smsgateway/modules/receiver/data/InboxMessage.kt (1)
39-39: LGTM!The
bodyfield is correctly added as nullable. The existingequals/hashCodeimplementation based ontransactionIdis appropriate since the body is derived content that shouldn't affect identity comparison.
app/src/main/java/me/capcom/smsgateway/modules/receiver/MmsReceiver.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/me/capcom/smsgateway/modules/receiver/MmsReceiver.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/me/capcom/smsgateway/modules/receiver/MmsReceiver.kt
Outdated
Show resolved
Hide resolved
capcom6
left a comment
capcom6
left a comment
There was a problem hiding this comment.
I appreciate your work. This is a valuable contribution to the project.
However, before I merge these changes, please address some of the issues raised by CodeRabbit and answer the questions in the comments, especially regarding deduplication and "magic numbers" like the delay in the main thread.
Thank you.
app/src/main/java/me/capcom/smsgateway/modules/receiver/MmsReceiver.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/me/capcom/smsgateway/modules/receiver/MmsReceiver.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/me/capcom/smsgateway/modules/receiver/MmsReceiver.kt
Outdated
Show resolved
Hide resolved
…had some issues. To MMS ID and sender verification to increase robustness.
There was a problem hiding this comment.
Actionable comments posted: 3
Fix all issues with AI Agents 🤖
In @app/src/main/java/me/capcom/smsgateway/modules/receiver/MmsReceiver.kt:
- Around line 312-321: In the while loop inside MmsReceiver where
partsCursor.getString(ctIndex) is assigned to ct, guard against ct being null
before calling startsWith by using a null-safe check (e.g., ct != null &&
ct.startsWith("text") or ct?.startsWith("text") == true); ensure you still
retrieve text via partsCursor.getString(textIndex) and keep the existing
isNullOrEmpty check, so the code only appends body when ct indicates text and ct
is non-null.
🧹 Nitpick comments (1)
app/src/main/java/me/capcom/smsgateway/modules/receiver/MmsReceiver.kt (1)
170-193: Consider usinguse {}for cleaner cursor management.The cursor is closed in multiple code paths (lines 183, 186, 188), which is error-prone. Kotlin's
use {}extension ensures the cursor is always closed.🔎 Proposed refactor
private fun getCurrentMaxMmsId(context: Context): Long { try { - val cursor = context.contentResolver.query( + context.contentResolver.query( Uri.parse("content://mms"), arrayOf("_id"), null, null, "_id DESC LIMIT 1" - ) - if (cursor != null && cursor.moveToFirst()) { - val idIndex = cursor.getColumnIndex("_id") - if (idIndex >= 0) { - val maxId = cursor.getLong(idIndex) - cursor.close() - return maxId + )?.use { cursor -> + if (cursor.moveToFirst()) { + val idIndex = cursor.getColumnIndex("_id") + if (idIndex >= 0) { + return cursor.getLong(idIndex) + } } - cursor.close() } - cursor?.close() } catch (e: Exception) { Log.w(TAG, "Error getting max MMS ID: ${e.message}", e) } return 0L }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/src/main/java/me/capcom/smsgateway/modules/receiver/MmsReceiver.kt
🔇 Additional comments (5)
app/src/main/java/me/capcom/smsgateway/modules/receiver/MmsReceiver.kt (5)
67-81: Clean delegation to watchForMms.The refactored flow that delegates MMS body extraction to
watchForMmsis well-structured. The early validation and logging are appropriate.
223-231: Sender verification with retry is a good safety measure.The address verification before extracting body content addresses the previous concern about extracting the wrong message. The retry mechanism handles the case where address data isn't immediately populated.
However, move this to a background thread to avoid the 500ms sleep on the main thread (covered in earlier comment).
276-279: Address normalization and comparison logic is appropriate.The bidirectional
endsWithcomparison handles international number formats correctly (e.g.,+1234567890vs234567890).
369-373: Well-documented constants.The constants are clearly named with appropriate comments explaining their purpose.
139-168: Main thread is still blocked by body extraction logic.This method runs on the main thread (via the main looper handler), but it calls
tryExtractMmsByAddressAndDatewhich containsThread.sleep(500)(line 229). Combined with multiple ContentProvider queries, this can still cause ANR issues.The previous review suggested using
goAsync()with a background thread. While the ContentObserver approach is an improvement, the extraction work should run on a background thread, not the main looper.Likely an incorrect or invalid review comment.
app/src/main/java/me/capcom/smsgateway/modules/receiver/MmsReceiver.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/me/capcom/smsgateway/modules/receiver/MmsReceiver.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
app/src/main/java/me/capcom/smsgateway/modules/receiver/MmsReceiver.kt (3)
150-168: Consider extracting parameters into a data class.This method has 17 parameters, which makes it difficult to read and maintain. Consider grouping related parameters into a data class.
🔎 Example approach
private data class MmsExtractionContext( val transactionId: String, val messageId: String?, val subject: String?, val size: Long?, val contentClass: String?, val address: String, val date: Date, val subscriptionId: Int? ) private fun attemptBodyExtractionAndProcess( context: Context, mmsContext: MmsExtractionContext, minMmsId: Long, startTime: Long, timeoutMs: Long, observer: ContentObserver, handler: Handler, processed: AtomicBoolean, timeoutRunnable: Runnable, handlerThread: HandlerThread ): Boolean
190-213: Simplify cursor handling withuseextension.The cursor closing logic is fragmented across multiple paths. Using Kotlin's
useextension ensures the cursor is always closed, even if an exception occurs.🔎 Proposed fix
private fun getCurrentMaxMmsId(context: Context): Long { try { - val cursor = context.contentResolver.query( + context.contentResolver.query( Uri.parse("content://mms"), arrayOf("_id"), null, null, "_id DESC LIMIT 1" - ) - if (cursor != null && cursor.moveToFirst()) { - val idIndex = cursor.getColumnIndex("_id") - if (idIndex >= 0) { - val maxId = cursor.getLong(idIndex) - cursor.close() - return maxId + )?.use { cursor -> + if (cursor.moveToFirst()) { + val idIndex = cursor.getColumnIndex("_id") + if (idIndex >= 0) { + return cursor.getLong(idIndex) + } } - cursor.close() } - cursor?.close() } catch (e: Exception) { Log.w(TAG, "Error getting max MMS ID: ${e.message}", e) } return 0L }
215-265: Simplify cursor handling withuseextension.Similar to
getCurrentMaxMmsId, this method would benefit from using theuseextension for cleaner cursor management.🔎 Proposed fix
private fun tryExtractMmsByAddressAndDate(context: Context, address: String, minMmsId: Long): String? { try { val mmsUri = Uri.parse("content://mms") - // Get MMS messages with ID greater than minMmsId (newer messages) - val cursor = context.contentResolver.query( + context.contentResolver.query( mmsUri, arrayOf("_id", "date"), "_id > ?", arrayOf(minMmsId.toString()), "_id DESC LIMIT 10" - ) - - Log.d(TAG, "Looking for MMS with ID > $minMmsId from address=$address, found ${cursor?.count ?: 0} candidates") - if (cursor != null && cursor.moveToFirst()) { + )?.use { cursor -> + Log.d(TAG, "Looking for MMS with ID > $minMmsId from address=$address, found ${cursor.count} candidates") val idIndex = cursor.getColumnIndex("_id") val dateIndex = cursor.getColumnIndex("date") if (idIndex < 0 || dateIndex < 0) { Log.w(TAG, "Required column not found in MMS query") - cursor.close() return null } - do { + while (cursor.moveToNext()) { val mmsId = cursor.getLong(idIndex) - val mmsDate = cursor.getLong(dateIndex) - - // Check if this MMS is from the expected address... + // ... rest of logic unchanged var senderMatch = checkMmsSender(context, mmsId.toString(), address) if (!senderMatch) { - // Wait for address data to be populated Thread.sleep(ADDRESS_DATA_RETRY_DELAY_MS) senderMatch = checkMmsSender(context, mmsId.toString(), address) } if (senderMatch) { - cursor.close() Log.d(TAG, "Found matching MMS ID=$mmsId for address=$address") return extractBodyFromMmsId(context, mmsId.toString()) } - } while (cursor.moveToNext()) - cursor.close() + } } } catch (e: Exception) { Log.w(TAG, "Error extracting MMS by address: ${e.message}", e) } return null }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/src/main/java/me/capcom/smsgateway/modules/receiver/MmsReceiver.kt
🧰 Additional context used
🧬 Code graph analysis (1)
app/src/main/java/me/capcom/smsgateway/modules/receiver/MmsReceiver.kt (2)
app/src/main/java/me/capcom/smsgateway/modules/receiver/ReceiverService.kt (1)
start(25-29)app/src/main/java/me/capcom/smsgateway/ui/HomeFragment.kt (1)
start(358-360)
🔇 Additional comments (5)
app/src/main/java/me/capcom/smsgateway/modules/receiver/MmsReceiver.kt (5)
29-88: LGTM - onReceive properly delegates to background thread.The
onReceivemethod now returns quickly after starting thewatchForMmsworkflow on a backgroundHandlerThread, avoiding the ANR risk from the previous implementation.
267-313: Address comparison logic looks reasonable.The phone number normalization and
endsWithcomparison handles common cases like international prefixes. The cursor handling could use theuseextension for consistency with other methods.
315-348: LGTM - Body extraction handles null content type correctly.The null-safe check
ct?.startsWith("text") == trueproperly handles cases where the content type column value is null.
350-376: LGTM - Clean delegation to ReceiverService.The
processMmsMessagemethod properly constructs theInboxMessage.Mmswith all extracted data including the optional body and delegates to the existing service.
378-403: LGTM - Constants and registration methods are well-structured.The constants are appropriately named and documented. The singleton registration pattern is suitable for broadcast receivers.
|
Just made some code changes. I fixed an issue in my previous code where it used transactional IDs to find a new MMS. This seemed to be unreliable and had me use a delay. I switched to a content Observer to keep track of MMS Message IDs and wait for a new one to appear. That way it wouldn't try to process an older MMS and only grabs the most recent. Using this process, I had to also check against the sender of the MMS in case multiple MMS' were sent at the same time by different senders. |
|
Thank you for your contribution and the detailed explanation of the changes. I appreciate the work you've put into improving MMS webhook functionality. I've reviewed the implementation and have some concerns about the complexity of the code. The solution involves multiple threads, timeouts, a method with 17 parameters, and several conditional blocks that could benefit from early returns to improve readability. Having all of this logic in a single file also makes it difficult to maintain. Since I don't use MMS myself and don't have the enough time to refactor this code in the near future, I'm afraid I can't approve this PR in its current form. Perhaps we should leave the current MMS webhook as is and introduce a new one that relies on monitoring the |
|
By the way, is it possible to completely replace the current MMS webhook logic with
However, if mobile data is disabled or background data is limited, the message can remain in the "notification" state (type 130) indefinitely until a manual download is triggered or data is re-enabled. Therefore, these should be two different webhook events. |
|
No worries. I completely understand about the PR not being ready. This was definitely more involved than I initially thought. Before working on this, I had no experience with Android SMS/MMS processing and limited Android exp in general. So, not too surprised things could be improved. I'm OK with closing this PR for now. |
2 participants
For MMS incoming webhook, there was no body captured. This adds the body content of an MMS to the mms/incoming webhook. This was tested and works for long text only messages 1000 chars+ and MMS with images and text (images are ignored)
Summary by CodeRabbit
New Features
Bug Fixes / Reliability
✏️ Tip: You can customize this high-level summary in your review settings.