Improve incoming call flow for missing camera/audio permissions#1612
Conversation
PR checklist ✅All required conditions are satisfied:
🎉 Great job! This PR is ready for review. |
WalkthroughThe changes enhance the notification and permission handling system for incoming calls. They introduce permission-aware call acceptance, add notification ID retrieval utilities, improve error handling in call initialization, and implement permission validation across the call service layer. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CallActivity as StreamCallActivity
participant PermMgr as PermissionManager
participant CallService
participant NotifRetriever as ServiceNotificationRetriever
participant Lifecycle as CallServiceLifecycleManager
User->>CallActivity: Accept incoming call
CallActivity->>PermMgr: hasRequiredCallPermissions(call)?
PermMgr->>PermMgr: Check RECORD_AUDIO/CAMERA
PermMgr-->>CallActivity: Return permission status
alt Permissions Granted
CallActivity->>CallService: Accept call
CallService->>NotifRetriever: getNotificationId(trigger)
NotifRetriever-->>CallService: Return notification ID
CallService->>CallService: promoteToFgServiceIfNoActiveCall
CallService->>Lifecycle: initializeCallAndSocket(onError)
Lifecycle-->>CallService: Error or success callback
else Permissions Denied
CallActivity->>PermMgr: hasNotificationPermission()?
PermMgr-->>CallActivity: Return permission status
CallActivity->>CallActivity: updateNotificationWhenAppComesToForeground
CallActivity->>CallService: Fetch call state (GET)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
stream-video-android-ui-core/src/main/kotlin/io/getstream/video/android/ui/common/permission/PermissionManager.kt (1)
19-33:⚠️ Potential issue | 🟡 MinorDuplicate license header.
The file contains two license headers (lines 1–15 and lines 19–33). Remove the second one.
🤖 Fix all issues with AI agents
In
`@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/CallService.kt`:
- Around line 655-677: promoteToFgServiceIfNoActiveCall currently skips
startForeground when videoClient.getSettingUpCallNotification() returns null
(NoOpNotificationHandler), which can leave a service started via
ContextCompat.startForegroundService() without a foreground call and cause
ForegroundServiceDidNotStartInTimeException; update
promoteToFgServiceIfNoActiveCall to always call startForegroundWithServiceType
(or startForeground) when !hasActiveCall by providing a fallback notification
(build a minimal/default Notification) if getSettingUpCallNotification() returns
null, so the service is promoted to foreground even when the custom handler
returns null; keep references to videoClient.getSettingUpCallNotification(),
startForegroundWithServiceType(...,
permissionManager.getServiceType(baseContext, trigger)), and ensure this change
preserves the normal path to startForegroundForCall() and safe
stopServiceGracefully() behavior.
In
`@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/managers/CallServiceLifecycleManager.kt`:
- Around line 52-56: The error message concatenates a nullable from
update.errorOrNull(), producing e.g. "Fail to perform
initializeCallAndSocketnull" and lacking a separator; update the onError
invocation in CallServiceLifecycleManager (the initializeCallAndSocket error
path) to safely extract the error text and include a separator — e.g., use a
safe call and fallback (error?.message ?: error.toString() ?: "unknown") or a
default string, and prepend with a separator like ": " or " - " so the logged
message becomes "Fail to perform initializeCallAndSocket: <error details>"
instead of concatenating a nullable directly.
In
`@stream-video-android-ui-core/src/main/kotlin/io/getstream/video/android/ui/common/permission/PermissionManager.kt`:
- Around line 100-129: In hasRequiredCallPermissions, fix the typo in the
TODO/log comment ("exiting" → "existing") and change the failure handling for
call.get(): when result.isFailure, log the error at error level including the
failure details (use logger.e with the exception or result.error) instead of
logger.w, and return false (do not bypass permission checks) so callers will
enforce permissions; keep the rest of the capability checks unchanged and
reference call.get(), call.state.ownCapabilities, and logger for locating the
change.
In
`@stream-video-android-ui-core/src/main/kotlin/io/getstream/video/android/ui/common/StreamCallActivity.kt`:
- Around line 571-587: The current logic in handleAcceptCallIntent silently does
nothing when hasRequiredCallPermissions(...) is false and
hasNotificationPermission(...) is also false; change it so that when call
permissions are missing you always invoke get(call, onError=..., onSuccess=...)
to surface the incoming-call UI for permission prompting, and only call
updateNotificationWhenAppComesToForeground(call) beforehand when
PermissionManager.hasNotificationPermission(...) is true; keep the existing
accept(call, ...) path when permissions are present and preserve
onError/onSuccess propagation for both accept and get.
🧹 Nitpick comments (5)
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/managers/CallServiceLifecycleManager.kt (1)
32-41: Leftover TODO with developer name.This TODO references a specific developer (
Rahul) and contains speculative notes. Consider converting to a proper actionable TODO or removing before merge.stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/ServiceNotificationRetriever.kt (1)
135-159: Notification ID resolution logic is duplicated withgetNotificationPair.The
whenblock here mirrors the ID derivation ingetNotificationPair(lines 70–126). Consider extractinggetNotificationIdfirst and then calling it fromgetNotificationPairto keep a single source of truth for ID resolution.Sketch
// In getNotificationPair, replace inline ID resolution with: TRIGGER_ONGOING_CALL -> { logger.d { "[getNotificationPair] Creating ongoing call notification" } - val notificationId = call.state.notificationIdFlow.value - ?: streamCallId.getNotificationId(NotificationType.Ongoing) + val notificationId = getNotificationId(trigger, streamVideo, streamCallId) Pair( first = streamVideo.getOngoingCallNotification(...), second = notificationId, ) } // ... and similarly for the other branchesstream-video-android-ui-core/src/main/kotlin/io/getstream/video/android/ui/common/StreamCallActivity.kt (1)
589-600:@SuppressLint("MissingPermission")hides a runtime risk.This method suppresses the lint check for
POST_NOTIFICATIONS, but the only call-site protection is anifbranch inhandleAcceptCallIntent. If this method is ever called from another location without the guard,notify()will silently fail on API 33+. Consider adding a runtime guard inside this method or documenting the precondition clearly.stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/CallService.kt (2)
237-237: Destructured pair discards the notification ID — intentional and correct.The notification ID is now retrieved earlier (line 210) via
getNotificationId, so ignoring the second element of the pair is fine. However, this redundancy reinforces the suggestion to refactorgetNotificationPairinServiceNotificationRetrieverto avoid computing the ID twice.
308-308: Leftover TODO — track or resolve before merge.This TODO raises a concern about livestream guest/host getting stuck on a null notification. Consider filing an issue to track this if it's not being addressed in this PR.
Would you like me to open an issue to track this?
...rc/main/kotlin/io/getstream/video/android/core/notifications/internal/service/CallService.kt
Show resolved
Hide resolved
...am/video/android/core/notifications/internal/service/managers/CallServiceLifecycleManager.kt
Show resolved
Hide resolved
...ui-core/src/main/kotlin/io/getstream/video/android/ui/common/permission/PermissionManager.kt
Show resolved
Hide resolved
...o-android-ui-core/src/main/kotlin/io/getstream/video/android/ui/common/StreamCallActivity.kt
Show resolved
Hide resolved
SDK Size Comparison 📏
|
48903f0 to
b79a592
Compare
b79a592 to
98e764e
Compare
aleksandar-apostolov
left a comment
There was a problem hiding this comment.
LGTM. Critical fixes verified:
- Foreground service promoted early before permission check
- Silent no-op fixed —
get()always called when permissions missing - Incoming call trigger skips audio/video permission check correctly
Minor: error message string concat could use interpolation, but not blocking.
|


Goal
Improve incoming call flow for missing camera/audio permissions
Implementation
Improved permission handling
TRIGGER_INCOMING_CALL. As we don't need camera/audio just to render Incoming Calling ScreenImproved notification UX
NotificationConfig.hideRingingNotificationInForeground🎨 UI Changes
None.
Testing
Flow 1: Tap on incoming Call Notification
hideRingingNotificationInForegroundFlow 2: Tap on incoming Call's Notification's Accept Button
hideRingingNotificationInForegroundSummary by CodeRabbit
New Features
Improvements