chore(voip): delete dead method-call queue and extract deviceId() helper#7294
Conversation
The queue API (queueMethodCall et al.) was never called by any producer; removing it eliminates dead code paths and the deviceId() extraction collapses four identical Settings.Secure.getString calls into one site.
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Failure
WalkthroughRemoved the queued method call buffering mechanism from ChangesRemoval of Queued Method Call System and Device ID Refactoring
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
diegolmello
had a problem deploying
to
experimental_ios_build
GitHub Actions
Failure
diegolmello
had a problem deploying
to
official_android_build
GitHub Actions
Failure
diegolmello
had a problem deploying
to
official_ios_build
GitHub Actions
Failure
diegolmello
had a problem deploying
to
experimental_android_build
GitHub Actions
Failure
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.kt`:
- Around line 85-88: deviceId currently assumes Settings.Secure.ANDROID_ID is
non-null; change the handling so deviceId(context) can be null and callers treat
null as "unknown local device" (i.e., do not classify events as "other device").
Update the deviceId function to return a nullable String (capture
Settings.Secure.getString(...) as String?), and then replace direct comparisons
like signedContractId != deviceId(context) with a null-safe check such as: if
(deviceId(context) == null) { /* treat as same/unknown, not "other device" */ }
else { signedContractId != deviceId(context) }. Apply the same null-aware
handling at the other call sites referenced (the usages around the deviceId
function at the locations noted).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 508f9a8f-10b8-46c3-af7c-6d4a724afbfb
📒 Files selected for processing (2)
android/app/src/main/java/chat/rocket/reactnative/voip/DDPClient.ktandroid/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.kt
💤 Files with no reviewable changes (1)
- android/app/src/main/java/chat/rocket/reactnative/voip/DDPClient.kt
📜 Review details
⏰ 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: ESLint and Test / run-eslint-and-test
🔇 Additional comments (1)
android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.kt (1)
173-173: Nice cleanup oncontractIdde-duplication.Replacing repeated
Settings.Securelookups withdeviceId(context)keeps behavior consistent and reduces call-site drift.Also applies to: 320-320, 335-335, 506-506
| /** Returns the stable Android device ID used as `contractId` in media-calls.answer requests. */ | ||
| private fun deviceId(context: Context): String = | ||
| Settings.Secure.getString(context.contentResolver, Settings.Secure.ANDROID_ID) | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In current Android documentation, what is the nullability contract of Settings.Secure.getString(ContentResolver, String), and under what conditions can ANDROID_ID be null?
💡 Result:
The nullability contract of Settings.Secure.getString(ContentResolver resolver, String name) in the current Android documentation states that it returns the corresponding value as a String, or null if the setting is not present in the database. For ANDROID_ID specifically, the official documentation describes it as always being set to a 64-bit hexadecimal string value generated at device setup (pre-Android 8.0) or scoped per app-signing key, user, and device (Android 8.0+), with no mention of it being null under normal conditions. However, empirical reports indicate it can return null rarely on certain devices (e.g., some Blackview models), in emulators, or testing frameworks like Robolectric. No official conditions for ANDROID_ID being null are documented; the method returns null generally when any Secure setting key has no value.
Citations:
- 1: https://developer.android.com/reference/kotlin/android/provider/Settings.Secure
- 2: https://developer.android.com/reference/android/provider/Settings.Secure
- 3: https://developers.android.google.cn/reference/kotlin/android/provider/Settings.Secure
🏁 Script executed:
cat -n android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.kt | sed -n '85,95p'Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 711
🏁 Script executed:
cat -n android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.kt | sed -n '555,565p'Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 922
🏁 Script executed:
wc -l android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.ktRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 157
🏁 Script executed:
cat -n android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.kt | grep -n "deviceId" | head -20Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 519
🏁 Script executed:
# Search for all invocations of deviceId in the file
rg "deviceId\(" android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.kt -nRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 477
🏁 Script executed:
# Check if there are any elvis operators or null coalescing near the deviceId declaration
cat -n android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.kt | sed -n '83,92p'Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 603
🏁 Script executed:
# Check the imports to see if Settings is from android.provider or a wrapper
head -50 android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.kt | grep -E "^import|^package"Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 1441
🏁 Script executed:
# Check how contractId is typed in the places where deviceId is assigned
cat -n android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.kt | sed -n '170,176p'Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 387
🏁 Script executed:
# Check another usage to see the full context
cat -n android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.kt | sed -n '316,322p'Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 426
🏁 Script executed:
# Find MediaCallsAnswerRequest to see what type contractId expects
fd MediaCallsAnswerRequest --type fRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 311
🏁 Script executed:
# Search for MediaCallsAnswerRequest class definition
rg "class MediaCallsAnswerRequest|fun fetch" --type kotlin | head -20Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 414
🏁 Script executed:
# Look for the MediaCallsAnswerRequest.fetch function signature
rg "fun fetch" android/app/src/main/java/chat/rocket/reactnative/ -A 5 | head -30Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 749
🏁 Script executed:
# Check the MediaCallsAnswerRequest class definition to confirm contractId type
cat -n android/app/src/main/java/chat/rocket/reactnative/voip/MediaCallsAnswerRequest.kt | head -80Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 3686
🏁 Script executed:
# Let's check what the actual return type of Settings.Secure.getString is in Kotlin
# by checking if there are any type annotations or elvis operators used
rg "Settings.Secure.getString" android/app/src/main/java/chat/rocket/reactnative/ -B 2 -A 2Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 719
🏁 Script executed:
# Check if there are any compilation suppression annotations or type aliases
grep -r "@Suppress\|@SuppressLint" android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.ktRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 60
🏁 Script executed:
# Check the build.gradle to see if there are any Kotlin compiler settings
cat android/app/build.gradle | grep -A 10 "kotlinOptions\|compileOptions"Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 60
🏁 Script executed:
# Verify if this code actually compiles by checking if there's a built artifact
find . -name "VoipNotification.class" -o -name "VoipNotification.kt.class" 2>/dev/null | head -5Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 60
🏁 Script executed:
# Check if there's a different version or wrapper for Settings.Secure.getString
rg "fun getString|getString.*=" android/app/src --type kotlin | grep -i settingsRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 60
🏁 Script executed:
# Look at the exact lines again more carefully to see if there's an elvis operator or !! I missed
sed -n '85,88p' android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.kt | cat -ARepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 336
🏁 Script executed:
# Check if there's a Gradle or Maven wrapper that might have type annotations
find . -name "build.gradle.kts" -o -name "build.gradle" | grep -E "app|android" | head -5Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 128
🏁 Script executed:
# Search for any recent changes or git history related to deviceId
git log --oneline -n 20 -- android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.kt 2>/dev/null || echo "Git history not available"Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 160
🏁 Script executed:
# Check the Android SDK version and dependencies
cat android/app/build.gradle | grep -E "compileSdk|targetSdk|android.targetSdk|settings.gradle"Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 185
🏁 Script executed:
# Look for any `@Nullable` or `@NonNull` annotations on the Settings class itself
rg "Settings" android/app/build.gradle -A 2 -B 2Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 60
🏁 Script executed:
# Check if there's a lint configuration that might suppress nullability warnings
cat android/app/build.gradle | grep -A 20 "lintOptions\|lint {"Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 60
🏁 Script executed:
# Let's directly check what the Kotlin compiler sees - look at the actual types in dependencies
cat android/build.gradle | head -100Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 1595
🏁 Script executed:
# Check if there's an elvis operator or default value being used anywhere in the file
rg "\?: |?: null|?: \"\"" android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.kt | head -10Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 177
🏁 Script executed:
# Let's check the exact commit that added this function to understand the intent
git show c2d55a98:android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.kt 2>/dev/null | sed -n '85,88p' || echo "Git show not available"Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 332
🏁 Script executed:
# Check line 559 context more thoroughly to see how the null case is actually handled
sed -n '555,565p' android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.ktRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 845
🏁 Script executed:
# Check if there's an elvis operator by looking at surrounding context with different search
sed -n '85,88p' android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.kt | od -c | head -20Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 119
🏁 Script executed:
# Let's check Android SDK type declarations for Settings.Secure.getString in context
# First, let's see what Kotlin version they're using and how it handles Java interop
grep -r "kotlinVersion\|kotlin_version" android/build.gradleRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 150
🏁 Script executed:
# Check if there are any `@Suppress` or lint suppressions globally
grep -r "SuppressWarnings\|Suppress.*Nullable" android/app/src/main/java/chat/rocket/reactnative/voip/Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 60
🏁 Script executed:
# Let's check how Settings.Secure is imported and if there are any extension functions
grep -A 5 -B 5 "Settings.Secure" android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.kt | head -20Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 618
🏁 Script executed:
# Check if Android SDK stubs might provide type information
# Look at what Kotlin actually infers for the return type
rg "private fun deviceId" android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.kt -A 2Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 227
🏁 Script executed:
# Let's verify by checking if the code compilation would work - look for gradle wrapper or compilation outputs
find . -name "*.gradle" -type f -exec grep -l "kotlin\|Kotlin" {} \; | head -5Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 128
🏁 Script executed:
# Check the actual issue: does line 559 handle null properly?
sed -n '559p' android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.ktRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 198
🏁 Script executed:
# Check if there's a proguard or R8 configuration that might affect this
find . -name "proguard-rules.pro" -o -name "proguard*.txt" | xargs cat 2>/dev/null | grep -i "settings\|android" | head -10Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 226
🏁 Script executed:
# Let's verify the actual type by checking Android SDK documentation behavior
# First, confirm if this is truly a compilation issue or if Kotlin allows this
cat android/app/build.gradle | grep -E "kotlin|suppress" -iRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 123
🏁 Script executed:
# Let's look at the full context around line 559 to see if there's any null handling before that point
sed -n '540,570p' android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.ktRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 2015
Guard nullable ANDROID_ID to prevent logic errors in device-call comparison.
At line 87, Settings.Secure.getString() can return null (confirmed on emulators and certain devices), but the return type is declared as non-null String. This causes line 559's comparison signedContractId != deviceId(context) to incorrectly classify events as "other device accepted" when the local device ID is unavailable, since null will always compare as not equal to any non-empty string.
Suggested patch
- private fun deviceId(context: Context): String =
+ private fun deviceId(context: Context): String? =
Settings.Secure.getString(context.contentResolver, Settings.Secure.ANDROID_ID)
@@
- (!signedContractId.isNullOrEmpty() && signedContractId != deviceId(context)) ||
+ (!signedContractId.isNullOrEmpty() &&
+ !deviceId(context).isNullOrEmpty() &&
+ signedContractId != deviceId(context)) ||Also applies to: 173, 320, 335, 506
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.kt`
around lines 85 - 88, deviceId currently assumes Settings.Secure.ANDROID_ID is
non-null; change the handling so deviceId(context) can be null and callers treat
null as "unknown local device" (i.e., do not classify events as "other device").
Update the deviceId function to return a nullable String (capture
Settings.Secure.getString(...) as String?), and then replace direct comparisons
like signedContractId != deviceId(context) with a null-safe check such as: if
(deviceId(context) == null) { /* treat as same/unknown, not "other device" */ }
else { signedContractId != deviceId(context) }. Apply the same null-aware
handling at the other call sites referenced (the usages around the deviceId
function at the locations noted).
1 participant
Proposed changes
Removes the permanently-empty method-call queue infrastructure from
DDPClient.ktand collapses four identicalSettings.Secure.getString(...ANDROID_ID)duplications inVoipNotification.ktinto a singledeviceId(context)helper.Issue(s)
Part of PR #6918 review cleanup — slice 05.
How to test or reproduce
Existing flow: trigger an incoming VoIP call on Android. Verify accept and reject both reach the server as before. No behaviour change — pure dead-code removal and de-duplication.
Types of changes
Checklist
Further comments
A — Dead queue removal (
DDPClient.kt)queueMethodCall,queuedMethodCalls,hasQueuedMethodCalls,flushQueuedMethodCalls,clearQueuedMethodCalls, and theQueuedMethodCalldata class are deleted. A pre-flightgrep -rn queueMethodCall android/confirmed no producer ever calledqueueMethodCall; the queue was permanently empty.flushPendingQueuedSignalsIfNeededinVoipNotification.kt(which only ever returnedfalse) and its one call site are also removed.B —
deviceId()helper (VoipNotification.kt)Settings.Secure.getString(context.contentResolver, Settings.Secure.ANDROID_ID)appeared four times. Extracted intoprivate fun deviceId(context: Context): Stringin the companion object; all four call sites now invoke it.Summary by CodeRabbit