Improve Foreground Service orchestration when transitioning from outgoing call to active call#1627
Conversation
… if permissions are missing when making outgoing call
PR checklist ✅All required conditions are satisfied:
🎉 Great job! This PR is ready for review. |
SDK Size Comparison 📏
|
rahul-lohra
changed the title
rahul-lohra
changed the title
rahul-lohra
changed the title
rahul-lohra
changed the title
WalkthroughThe changes implement a permission-based UI flow for foreground services in video calls. A new permission handling system checks required permissions before call initialization, shows conditional UI for permission requests, and updates service startup logic to verify service running state rather than automatically starting foreground services. Capabilities are now set based on available foreground permissions. Changes
Sequence DiagramsequenceDiagram
participant User
participant StreamCallActivity
participant PermissionUI as Permission UI Flow
participant PermissionManager
participant ServiceChecker
participant Dispatcher as Notification Dispatcher
User->>StreamCallActivity: Start Call
StreamCallActivity->>PermissionManager: Check required foreground permissions
alt Permissions Missing
PermissionManager-->>StreamCallActivity: Missing permissions
StreamCallActivity->>PermissionUI: Show permission request UI
PermissionUI->>User: Request foreground permissions
User->>PermissionUI: Grant/Deny permissions
PermissionUI->>StreamCallActivity: Permissions result
StreamCallActivity->>StreamCallActivity: Update capabilities based on granted permissions
else All Permissions Available
PermissionManager-->>StreamCallActivity: All permissions available
StreamCallActivity->>StreamCallActivity: Set full capabilities
end
StreamCallActivity->>ServiceChecker: Verify call service is running
alt Service Running
ServiceChecker-->>StreamCallActivity: Service confirmed running
else Service Not Running
ServiceChecker-->>StreamCallActivity: Service not running (log error)
end
StreamCallActivity->>Dispatcher: Dispatch call notification
Dispatcher->>Dispatcher: Handle notification with foreground permissions
Dispatcher-->>User: Display call UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
![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 (5)
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/CallState.kt (1)
1784-1787: Parameter name should be plural to match type.The parameter
ownCapabilityis singular but expects aList<OwnCapability>. UseownCapabilitiesfor consistency with the internal_ownCapabilitiesfield.✨ Proposed fix
`@InternalStreamVideoApi` - fun setOwnCapabilities(ownCapability: List<OwnCapability>) { - this._ownCapabilities.value = ownCapability + fun setOwnCapabilities(ownCapabilities: List<OwnCapability>) { + this._ownCapabilities.value = ownCapabilities }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/CallState.kt` around lines 1784 - 1787, The parameter name in setOwnCapabilities should be plural to match its type and the internal field; rename the parameter ownCapability to ownCapabilities in the function signature of setOwnCapabilities and update all usages inside the function (this._ownCapabilities.value) and any call sites referencing that parameter to use the new name; ensure imports/overloads are unchanged and rebuild to catch any unresolved references.stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/StreamForegroundPermissionUtil.kt (2)
24-24: Remove unnecessary import.The import
kotlin.jvm.javais not needed. The.javaextension property for getting a JavaClassfrom a KotlinKClassis available without explicit import.✨ Proposed fix
import io.getstream.video.android.core.notifications.internal.service.permissions.AudioCallPermissionManager import io.getstream.video.android.core.notifications.internal.service.permissions.ForegroundServicePermissionManager -import kotlin.jvm.java🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/StreamForegroundPermissionUtil.kt` at line 24, The file StreamForegroundPermissionUtil.kt contains an unnecessary import `kotlin.jvm.java`; remove that import line so the code relies on the built-in `.java` extension on KClass (no other code changes required), e.g., delete the `import kotlin.jvm.java` entry in StreamForegroundPermissionUtil.kt to clean up unused imports.
29-40: Consider caching permission managers and documenting the sentinel value.
ForegroundServicePermissionManager()andAudioCallPermissionManager()are instantiated on every call. Consider caching them sincerequiredForegroundTypesis a constant set.The return value
setOf(-1)is a sentinel value that should be documented for callers.♻️ Proposed improvement
`@InternalStreamVideoApi` public object StreamForegroundPermissionUtil { + private val callServicePermissionManager = ForegroundServicePermissionManager() + private val audioCallPermissionManager = AudioCallPermissionManager() + + /** + * Returns the required foreground service types for the given call type. + * `@param` callType The call type string. + * `@return` Set of ServiceInfo.FOREGROUND_SERVICE_TYPE_* constants, or setOf(-1) if + * StreamVideo client is not initialized or an unknown service class is configured. + */ public fun getForegroundPermissionsForCallType(callType: String): Set<Int> { val client = StreamVideo.instanceOrNull() as? StreamVideoClient if (client != null) { val config = client.callServiceConfigRegistry.get(callType) return when (config.serviceClass) { - CallService::class.java -> ForegroundServicePermissionManager().requiredForegroundTypes - AudioCallService::class.java -> AudioCallPermissionManager().requiredForegroundTypes + CallService::class.java -> callServicePermissionManager.requiredForegroundTypes + AudioCallService::class.java -> audioCallPermissionManager.requiredForegroundTypes else -> setOf(-1) } } return setOf(-1) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/StreamForegroundPermissionUtil.kt` around lines 29 - 40, getForegroundPermissionsForCallType currently instantiates ForegroundServicePermissionManager and AudioCallPermissionManager on every call and returns a magic sentinel setOf(-1) undocumented; fix by adding cached instances (e.g., private val foregroundPermissionManager and audioCallPermissionManager) and reuse their requiredForegroundTypes in getForegroundPermissionsForCallType, and replace the inline sentinel with a named constant (e.g., UNKNOWN_FOREGROUND_PERMISSION = setOf(-1)) documented via KDoc on the function or constant to explain the sentinel meaning for callers.stream-video-android-ui-compose/src/main/kotlin/io/getstream/video/android/compose/ui/StreamCallActivityComposeDelegate.kt (1)
127-135: Consider guarding against redundant recomposition.When
AllPermissionsGrantedfires,onAllPermissionGranted()setsrenderPermissionUitofalse. However, sinceAllPermissionsGrantedfires on every recomposition while permissions are granted (see context snippet 3), there's a brief window where the callback may execute multiple times before the state change propagates.While this is harmless (setting
falsetofalseis idempotent), you could add a guard for clarity.✨ Optional guard
- if (renderPermissionUi) { + val renderPermissionUi by activity.renderPermissionUi.collectAsStateWithLifecycle() + if (renderPermissionUi) { NoPermissionUi(activity, call) { - activity.updateRenderPermissionUi(false) + if (activity.renderPermissionUi.value) { + activity.updateRenderPermissionUi(false) + } } } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stream-video-android-ui-compose/src/main/kotlin/io/getstream/video/android/compose/ui/StreamCallActivityComposeDelegate.kt` around lines 127 - 135, The NoPermissionUi callback can trigger redundant updates because AllPermissionsGranted may fire multiple times before state updates; modify the callback in StreamCallActivityComposeDelegate to check the current renderPermissionUi state (from activity.renderPermissionUi.collectAsStateWithLifecycle()/renderPermissionUi) or query activity before calling activity.updateRenderPermissionUi(false) so you only call updateRenderPermissionUi(false) when renderPermissionUi is true, e.g., add a guard around the lambda passed to NoPermissionUi that references renderPermissionUi (or calls a new activity.isRenderPermissionUiVisible()) to avoid redundant state writes from onAllPermissionGranted.stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/observers/CallServiceNotificationUpdateObserver.kt (1)
144-151: Verify notification dispatcher fallback when service is not running.The change delegates active call notification to
StreamVideo.instanceOrNull()?.getStreamNotificationDispatcher()?.notify(), which only posts a notification without starting the foreground service (as confirmed byDefaultNotificationDispatcher.notify()at lines 27-40). This is correct for the outgoing→active transition where the service should already be running.However, if
StreamVideo.instanceOrNull()returnsnullor the dispatcher is unavailable, this silently fails without logging or fallback.♻️ Proposed improvement with fallback logging
private fun showActiveCallNotification( context: Context, callId: StreamCallId, notification: Notification, ) { logger.d { "[showActiveCallNotification] Showing active call notification" } val notificationId = call.state.notificationIdFlow.value ?: callId.getNotificationId(NotificationType.Ongoing) - StreamVideo.instanceOrNull() - ?.getStreamNotificationDispatcher() - ?.notify( - callId, - notificationId, - notification, - ) + val dispatcher = StreamVideo.instanceOrNull()?.getStreamNotificationDispatcher() + if (dispatcher != null) { + dispatcher.notify(callId, notificationId, notification) + } else { + logger.w { "[showActiveCallNotification] StreamVideo dispatcher unavailable, notification not posted" } + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/observers/CallServiceNotificationUpdateObserver.kt` around lines 144 - 151, CallServiceNotificationUpdateObserver currently forwards active-call notifications via StreamVideo.instanceOrNull()?.getStreamNotificationDispatcher()?.notify(...), which silently does nothing when StreamVideo or the dispatcher is null; update the observer to handle that fallback by checking for null and either logging a clear warning/error (including callId/notificationId) and/or starting the foreground call service directly when the dispatcher is unavailable; reference StreamVideo.instanceOrNull(), getStreamNotificationDispatcher(), notify(), and DefaultNotificationDispatcher.notify() when making the change so the fix ensures the active call is surfaced or at least diagnostic logs are emitted when the dispatcher/service isn't present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/ClientState.kt`:
- Around line 188-198: The check uses reflection incorrectly:
callServiceConfig.serviceClass is already a Class<out Service>, so replace the
redundant serviceClass::class.java usage in the
ServiceIntentBuilder().isServiceRunning call with the serviceClass value
directly; update the invocation in the ClientState code that uses
callServiceConfig and ServiceIntentBuilder.isServiceRunning (passing
this.client.context and serviceClass) to avoid taking the KClass of the Class
object.
In
`@stream-video-android-ui-core/src/main/kotlin/io/getstream/video/android/ui/common/StreamCallActivity.kt`:
- Around line 875-880: The call to uiDelegate.setContent(...) and any UI state
reads/updates (_renderPermissionUi.value, renderPermissionUi.first {...}) are
being executed on Dispatchers.IO; move those UI interactions onto the Main
dispatcher (e.g., with withContext(Dispatchers.Main) or
lifecycleScope.launch(Dispatchers.Main)) before calling create(...). Also
prevent infinite recursion by adding a retry guard: introduce a permission retry
counter (e.g., permissionRetryCount field and MAX_PERMISSION_RETRIES constant)
and increment it each time create(...) is re-invoked from the permission flow
(referencing create and renderPermissionUi); when the max is exceeded, perform a
graceful fallback such as finishing the activity with an error instead of
recursing.
- Around line 884-904: The current setCallCapabilitiesIfNotPresent overwrites
existing capabilities with an empty list because
call.state.setOwnCapabilities(outgoingCallCapabilities) is executed even for
non-outgoing intents or when no permissions match; update the logic in
setCallCapabilitiesIfNotPresent so that you only compute and set capabilities
when intent.action == NotificationHandler.ACTION_OUTGOING_CALL and only call
call.state.setOwnCapabilities(...) when outgoingCallCapabilities is non-empty
(or at least inside that outgoing-call branch), using
StreamForegroundPermissionUtil.getForegroundPermissionsForCallType to populate
outgoingCallCapabilities and keeping the early return that checks
call.state.ownCapabilities.value.
---
Nitpick comments:
In
`@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/CallState.kt`:
- Around line 1784-1787: The parameter name in setOwnCapabilities should be
plural to match its type and the internal field; rename the parameter
ownCapability to ownCapabilities in the function signature of setOwnCapabilities
and update all usages inside the function (this._ownCapabilities.value) and any
call sites referencing that parameter to use the new name; ensure
imports/overloads are unchanged and rebuild to catch any unresolved references.
In
`@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/observers/CallServiceNotificationUpdateObserver.kt`:
- Around line 144-151: CallServiceNotificationUpdateObserver currently forwards
active-call notifications via
StreamVideo.instanceOrNull()?.getStreamNotificationDispatcher()?.notify(...),
which silently does nothing when StreamVideo or the dispatcher is null; update
the observer to handle that fallback by checking for null and either logging a
clear warning/error (including callId/notificationId) and/or starting the
foreground call service directly when the dispatcher is unavailable; reference
StreamVideo.instanceOrNull(), getStreamNotificationDispatcher(), notify(), and
DefaultNotificationDispatcher.notify() when making the change so the fix ensures
the active call is surfaced or at least diagnostic logs are emitted when the
dispatcher/service isn't present.
In
`@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/StreamForegroundPermissionUtil.kt`:
- Line 24: The file StreamForegroundPermissionUtil.kt contains an unnecessary
import `kotlin.jvm.java`; remove that import line so the code relies on the
built-in `.java` extension on KClass (no other code changes required), e.g.,
delete the `import kotlin.jvm.java` entry in StreamForegroundPermissionUtil.kt
to clean up unused imports.
- Around line 29-40: getForegroundPermissionsForCallType currently instantiates
ForegroundServicePermissionManager and AudioCallPermissionManager on every call
and returns a magic sentinel setOf(-1) undocumented; fix by adding cached
instances (e.g., private val foregroundPermissionManager and
audioCallPermissionManager) and reuse their requiredForegroundTypes in
getForegroundPermissionsForCallType, and replace the inline sentinel with a
named constant (e.g., UNKNOWN_FOREGROUND_PERMISSION = setOf(-1)) documented via
KDoc on the function or constant to explain the sentinel meaning for callers.
In
`@stream-video-android-ui-compose/src/main/kotlin/io/getstream/video/android/compose/ui/StreamCallActivityComposeDelegate.kt`:
- Around line 127-135: The NoPermissionUi callback can trigger redundant updates
because AllPermissionsGranted may fire multiple times before state updates;
modify the callback in StreamCallActivityComposeDelegate to check the current
renderPermissionUi state (from
activity.renderPermissionUi.collectAsStateWithLifecycle()/renderPermissionUi) or
query activity before calling activity.updateRenderPermissionUi(false) so you
only call updateRenderPermissionUi(false) when renderPermissionUi is true, e.g.,
add a guard around the lambda passed to NoPermissionUi that references
renderPermissionUi (or calls a new activity.isRenderPermissionUiVisible()) to
avoid redundant state writes from onAllPermissionGranted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1f94450e-0056-4b12-b8f3-755fd3801401
📒 Files selected for processing (9)
demo-app/src/main/kotlin/io/getstream/video/android/util/StreamVideoInitHelper.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/CallState.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/ClientState.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/StreamForegroundPermissionUtil.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/observers/CallServiceNotificationUpdateObserver.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/permissions/ForegroundServicePermissionManager.ktstream-video-android-ui-compose/src/main/kotlin/io/getstream/video/android/compose/ui/StreamCallActivityComposeDelegate.ktstream-video-android-ui-core/api/stream-video-android-ui-core.apistream-video-android-ui-core/src/main/kotlin/io/getstream/video/android/ui/common/StreamCallActivity.kt
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/ClientState.kt
Show resolved
Hide resolved
...o-android-ui-core/src/main/kotlin/io/getstream/video/android/ui/common/StreamCallActivity.kt
Show resolved
Hide resolved
...o-android-ui-core/src/main/kotlin/io/getstream/video/android/ui/common/StreamCallActivity.kt
Show resolved
Hide resolved
| public final fun setRenderPermissionUi (Lkotlinx/coroutines/flow/StateFlow;)V | ||
| public final fun updateRenderPermissionUi (Z)V |
There was a problem hiding this comment.
This shouldn't be here. I already marked these variables with @InternalStreamVideoApi @aleksandar-apostolov @gpunto
There was a problem hiding this comment.
If they are public, they are still part of the public api, the annotation just forces consumers to opt in
There was a problem hiding this comment.
Yeah, I understand that if something is public it is technically part of the public API, and @InternalStreamVideoApi only forces consumers to opt-in.
What I’m observing is that the API validator behaves differently across modules.
For example, when a public API is annotated with @InternalStreamVideoApi:
• In stream-video-android-core, the API does not appear in the .api file, which is the behavior I expected.
• In stream-video-android-ui-compose, the API **does get added to the .api file`.
For reference, this public API added in the core module in the same PR did not update the .api file:
https://github.com/GetStream/stream-video-android/pull/1627/changes#diff-f229deea2aae6b806a751cc451836b9720165dd57e4de173cc40c2fd43169965R27-R29
My guess is that the apiValidation configuration is not applied consistently across modules.
It is currently configured in stream-video-android-core:
stream-video-android-core/build.gradle
But it is missing in stream-video-android-ui-compose:
stream-video-android-ui-compose/build.gradle
To make the behavior consistent, we could define the marker once in a shared configuration so all stream-* modules inherit the same rule:
apiValidation {
nonPublicMarkers += listOf(
"io.getstream.video.android.core.internal.InternalStreamVideoApi"
)
}This way any public API annotated with @InternalStreamVideoApi would be treated as non-public by the validator across all modules and we avoid these inconsistencies.
For this PR, I’ll add the apiValidation configuration to stream-video-android-ui-compose to make the behavior consistent.
There was a problem hiding this comment.
For example, when a public API is annotated with @InternalStreamVideoApi:
• In stream-video-android-core, the API does not appear in the .api file, which is the behavior I expected.
• In stream-video-android-ui-compose, the API **does get added to the .api file`.
Good point, I didn't know that!
My guess is that the apiValidation configuration is not applied consistently across modules.
That definitely seems to be the case so it makes sense to also add the same config to the module 👍
|
| withContext(Dispatchers.Main) { | ||
| uiDelegate.setContent(activity = this@StreamCallActivity, call) | ||
| } | ||
| renderPermissionUi.first { !it } |
There was a problem hiding this comment.
If the user denies permissions, NoPermissionUi calls InternalPermissionContent for SomeGranted/NoneGranted but never calls onAllPermissionGranted() — wouldn't _renderPermissionUi never flip to false, causing .first { \!it } to suspend forever? Dead coroutine on IO, no error callback, no way out for the user. Should there be a cancel/deny path that sets _renderPermissionUi = false and calls onError?
| activity.RootContent(call = call) | ||
|
|
||
| val renderPermissionUi by activity.renderPermissionUi.collectAsStateWithLifecycle() | ||
| if (renderPermissionUi) { |
There was a problem hiding this comment.
Related to the above — NoPermissionUi only provides onAllPermissionGranted as a dismiss path. How do SomeGranted and NoneGranted signal failure back to the create() coroutine?
| AudioCallService::class.java -> AudioCallPermissionManager().requiredForegroundTypes | ||
| else -> setOf(-1) | ||
| } | ||
| } |
There was a problem hiding this comment.
setOf(-1) as a sentinel for "no permissions" is fragile — if anything downstream iterates expecting valid ServiceInfo.FOREGROUND_SERVICE_TYPE_* constants, -1 could cause unexpected behavior. emptySet() would be clearer.
3 participants
Goal
Implementation
Root Cause - Launching FG Service even when the application is in background and ringing state transitions from outgoing to active
Proposed Solution
Because of this we need to correctly handle the following
2. Correctly update the notification transition from outgoing to active
🎨 UI Changes
Just a no permission ui in outgoing-call flow
Add relevant videos
Testing
Scenario: Outgoing call → background → call accepted
1. Make an outgoing call to another user.
2. While the call is still in the outgoing state, press the Home button to send the app to the background.
3. Accept the call on the other device.
Expected behavior
• The caller should transition smoothly from outgoing → active state.
• PiP should not be rendered, since it was never shown before the app went to the background.
• The Call Service should continue running without interruption.
• The notification should update from outgoing → active.
End condition
• If either the caller or callee declines/ends the call, the other participant should also leave the call and the session should terminate correctly.
Summary by CodeRabbit
New Features
Bug Fixes