Improve CallService lifecycle, notification handling, and ringing-to-active transition #1588
Conversation
Adds more detailed logging to the `CallEventObserver` to provide better insight into the handling of incoming call events.
When a `CallRejectedEvent` is received, the `createdBy` field on the call state is now updated with the user who created the call.
Introduces `LocalCallAcceptedEvent` and `LocalCallRejectedEvent` to represent call state changes originating from the local device. These new local events are now used in `CallEventObserver` instead of the generic `CallAcceptedEvent` and `CallRejectedEvent`, providing a clearer distinction for handling call lifecycle states like accepting a call. Additionally, `CallState` now fires these local events, improving the accuracy of event-driven logic.
# Conflicts: # stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/Call.kt # stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/CallService.kt
Adds a reason when leaving a call from the `CallLifecycleManager`. This provides more context for why the call was ended, distinguishing between different scenarios such as the task being removed for an ongoing or incoming call.
PR checklist ✅All required conditions are satisfied:
🎉 Great job! This PR is ready for review. |
SDK Size Comparison 📏
|
Renames several components within the `CallService` infrastructure to improve clarity by prefixing them with "CallService". This includes managers and observers. The events `LocalCallAcceptedEvent` and `LocalCallRejectedEvent` are renamed to `LocalCallAcceptedPostEvent` and `LocalCallRejectedPostEvent` respectively to better reflect their purpose. Additionally, this change introduces a comprehensive suite of unit tests for the `CallService` and its related components, covering: - Lifecycle and notification management - Event, ringing state, and notification update observers - Foreground service permission handling - Debouncer and Throttler utilities
Renames `LocalCallAcceptedEvent` and `LocalCallRejectedEvent` to `LocalCallAcceptedPostEvent` and `LocalCallRejectedPostEvent` respectively, to better reflect that they are events sent *after* the action has occurred.
Adds `notificationId` as a parameter to the `updateNotification` function. This allows for updating the notification with a specific ID. The previous `updateNotification` function without the ID has been deprecated.
Adds an overloaded version of the `updateNotification` method that doesn't require a notification ID, simplifying its usage.
The `transitionToAcceptCall` function in `ClientState` is now marked as `internal`, restricting its usage to within the same module. This change also updates the public API definition.
Removes an unnecessary `stopService` call that was being made immediately after `startService` in `ServiceLauncher`.
Adds a TODO comment to `DefaultNotificationDispatcher.kt` to ensure a new fix is verified before merging.
Adds a TODO comment to `DefaultNotificationDispatcher.kt` to ensure a new fix is verified before merging.
Adds the `RobolectricTestRunner` to `ServiceStateTest` to address test failures that occurred when running all tests together. The `unregisterToggleCameraBroadcastReceiver` test has been moved to the end of the file as it was failing for unknown reasons, and a `println` has been added for debugging.
When a call is initiated, it now transitions through the `RingingState.Outgoing` state. This change ensures that a foreground service with an outgoing call notification is started, providing immediate feedback to the user that a call is being placed. A delay is introduced before starting the foreground service for outgoing calls to allow the system to properly transition the call state.
This commit introduces several improvements to the handling of call notifications and the lifecycle of the `CallService`. The `handleNotification` method in `CallService` now returns a `CallServiceHandleNotificationResult` enum. This allows for more granular control over the service's state, enabling it to continue running without re-initialization (`START_NO_CHANGE`) or to be redelivered if an error occurs. Additionally, the logic for handling `LocalCallMissedEvent` has been moved from `CallServiceEventObserver` directly into the `CallState`. This change ensures that incoming call notifications are correctly dismissed when another call is already active, without unnecessarily stopping the foreground service. Other changes include: - Preventing the start of a new `CallService` if one is already running for another call. - Adding a short delay before starting the foreground service when a call is accepted to prevent race conditions. - Enhancing logging for better debugging of service and notification lifecycles.
Removes a leftover development comment in `ServiceLauncher.kt`. Adds a clarifying comment in `ClientState.kt` to explain the logic for transitioning incoming/outgoing calls to an active state within the same service.
Removes temporary "Noob" log messages from `ClientState`, `CallServiceEventObserver`, and `CallService`. Also removes some commented-out code from `CallServiceEventObserver`.
Updates the `isServiceRunning` check to return `false` when the service is not running.
Removes the static `runningServiceClassName` set from `CallService` and updates the `isServiceRunning` check. The `ServiceIntentBuilder.isServiceRunning` method now uses `ActivityManager.getRunningServices()` to determine if a service is active. This provides a more reliable check based on the Android system's state. The method signature for `isServiceRunning` has been updated to require a `Context` parameter.
Extracts the `Throttler` object from `Debouncer.kt` into its own dedicated file, `Throttler.kt`. This change also moves the `debouncer` instance in `CallService` to be a top-level property and adds documentation explaining its purpose.
Throttles the `stopService` requests in `ServiceLauncher` to prevent the service from being stopped and restarted too frequently. The throttling is set to `1000ms` using the new `SERVICE_DESTROY_THROTTLE_TIME_MS` constant defined in `CallService`.
This commit re-enables the `stopServiceGracefully()` call, which was previously commented out. The `source` parameter has also been removed from the `stopServiceGracefully()` method signature.
Removes a `// TODO` comment from `DefaultNotificationDispatcher`.
Retrieves the `notificationId` from the `call.state` when available, instead of calculating it. This ensures a consistent notification ID is used for ongoing, incoming, and outgoing call notifications within the `ServiceNotificationRetriever` and `CallServiceNotificationUpdateObserver`.
Updates the `ServiceNotificationRetrieverTest` to use the call-specific notification ID generated by `callId.getNotificationId()` instead of the call ID's hashcode. This aligns the test with the actual implementation for generating notification IDs.
When cancelling notifications for a call, the `CallServiceNotificationManager` now correctly uses the `notificationId` from the call's state. Previously, it was attempting to cancel a hardcoded `Incoming` notification type ID, which might not be the correct one. This change ensures the notification that was actually shown is the one that gets cancelled.
Adds more detailed logging to the `CallServiceNotificationUpdateObserver` for better insight into notification updates. Also, this change ensures that an existing notification ID from the call state is used before generating a new one when showing call notifications.
This commit refactors the `CallService` to improve clarity and maintainability. Key changes include: - Extracting the notification handling logic into smaller, more focused functions (`handleNullNotification`, `startForegroundForCall`). - Passing the `serviceScope` to `CallServiceEventObserver` to ensure its lifecycle is tied to the service. - Enhancing logging throughout the call service and related managers to provide more detailed information for debugging, including hashcodes to identify specific object instances. - Adding `serviceScope.cancel()` in `onDestroy` to ensure all coroutines are properly cancelled when the service is destroyed.
rahul-lohra
changed the title
Refactors the `IncomingCallPresenter` to simplify its logic by extracting functionality into smaller, more descriptive private functions. This improves readability and makes the different paths for handling incoming calls clearer. The `buildStopIntent` method in `ServiceIntentBuilder` is updated to only return an intent if the corresponding service is actually running, preventing unnecessary stop commands. Additionally, this commit: - Removes the handling of `LocalCallMissedEvent` from `CallServiceEventObserver` as it's no longer needed. - Enhances tests for `IncomingCallPresenter` and `ServiceIntentBuilder` to cover the new logic and edge cases.
WalkthroughAdds observer-driven foreground-service lifecycle, permission managers, notification ID tracking, debouncing/throttling utilities, expands missed-call event payload, and restructures CallService, notification handling, and ringing-state orchestration across multiple new internal managers and observers. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Service as CallService
participant Lifecycle as CallServiceLifecycleManager
participant EventObs as CallServiceEventObserver
participant RingObs as CallServiceRingingStateObserver
participant NotifObs as CallServiceNotificationUpdateObserver
participant Client as StreamVideoClient
participant Call as Call / CallState
participant Notifier as NotificationDispatcher
Service->>Lifecycle: initializeCallAndSocket(callId)
Lifecycle->>Client: fetch/ensure call & socket
Service->>EventObs: observe(onServiceStop, onRemoveIncoming)
Service->>RingObs: observe(onStopService)
Service->>NotifObs: observe(notificationUpdateTriggers)
alt Incoming ringing
Call-->>RingObs: ringingState = Incoming
RingObs->>Service: play sound & (maybe) vibrate
RingObs->>NotifObs: emit update trigger
NotifObs->>Service: produce Notification + notificationId
Service->>Notifier: notify(notificationId, Notification) / startForeground
end
alt Accept transition
Client->>Service: accept/join call
Service->>Client: state.transitionToAcceptCall(call)
Service->>RingObs: stop sound
NotifObs->>Service: update ongoing notification (notificationId)
end
alt Missed / Rejected / Ended
Call-->>EventObs: LocalCallMissedEvent / LocalCallRejectedPostEvent / CallEndedEvent
EventObs->>Service: remove incoming OR stop service
Service->>Lifecycle: endCall()
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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
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: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/ServiceLauncher.kt (1)
19-33: Remove duplicate license header.Lines 19-33 contain a duplicate of the license header that already appears at lines 1-15. This duplication should be removed.
🔎 Proposed fix
package io.getstream.video.android.core.notifications.internal.service -/* - * Copyright (c) 2014-2024 Stream.io Inc. All rights reserved. - * - * Licensed under the Stream License; - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * https://github.com/GetStream/stream-video-android/blob/main/LICENSE - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - import android.annotation.SuppressLintstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/Call.kt (1)
465-477: Remove redundant direct assignment of_ringingCall.valuebeforeaddRingingCall.Lines 470 and 474 both set
_ringingCall.valueto the same call. SinceaddRingingCall()already assigns the value and additionally handles foreground service setup, the direct assignment at line 470 is redundant.The comment claims that
updateFromResponsereads_ringingCall, but inspection of that method shows it only updates call state fields and does not accessringingCallat all. Consolidating to a single call toaddRingingCallwould be clearer.
🧹 Nitpick comments (20)
stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/notifications/internal/service/ServiceNotificationRetrieverTest.kt (1)
61-64: Consider structured test coroutine patterns for TestScope.The TestScope is created to satisfy
mockStreamVideoClient.scope, but it's not leveraged with structured patterns likerunTestor dispatcher control. If the code under test performs asynchronous work that needs deterministic execution in tests, consider usingStandardTestDispatcherorUnconfinedTestDispatcherwith proper test runners.However, if
ServiceNotificationRetrieveroperates synchronously or the current setup sufficiently covers the test scenarios, the current approach is acceptable.Based on coding guidelines regarding structured coroutines and test best practices.
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/models/CallIntentParams.kt (1)
22-27: Consider using a sealed class or enum for thetriggerparameter.The
triggerparameter is currently aString, which allows any value at runtime. Using a type-safe alternative would prevent invalid trigger values and improve compile-time safety.🔎 Example refactor using a sealed class
Define a sealed class for triggers:
internal sealed class CallTrigger { data object IncomingCall : CallTrigger() data object OutgoingCall : CallTrigger() data object OngoingCall : CallTrigger() data object RemoveIncomingCall : CallTrigger() }Then update the data class:
internal data class CallIntentParams( val streamVideo: StreamVideoClient, val callId: StreamCallId, - val trigger: String, + val trigger: CallTrigger, val displayName: String?, )stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/models/ServiceState.kt (2)
50-52: Log swallowed exceptions for diagnostics.Static analysis flagged these silently swallowed exceptions. While failing silently may be intentional for robustness, logging at debug level preserves diagnostic info without impacting release builds.
🔎 Proposed fix to log exceptions
+import io.getstream.log.taggedLogger + internal class ServiceState { + private val logger by taggedLogger("ServiceState") // ... existing fields ... internal fun registerToggleCameraBroadcastReceiver(service: Service, scope: CoroutineScope) { if (!isReceiverRegistered) { try { // ... registration logic ... isReceiverRegistered = true } catch (e: Exception) { - // Silently fail + logger.d(e) { "Failed to register camera broadcast receiver" } } } } internal fun unregisterToggleCameraBroadcastReceiver(service: Service) { if (isReceiverRegistered) { try { toggleCameraBroadcastReceiver?.let { service.unregisterReceiver(it) } isReceiverRegistered = false } catch (e: Exception) { - // Silently fail + logger.d(e) { "Failed to unregister camera broadcast receiver" } } } } }Also applies to: 61-63
28-35: Consider thread safety for mutable state.
ServiceStateholds mutable properties that may be accessed from different threads (e.g., main thread vs. coroutine scope). If concurrent access is expected, consider using@Volatilefor primitive flags orAtomicReferencefor object references.stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/notifications/internal/service/IncomingCallPresenterTest.kt (1)
53-66: Remove unusedcallServiceConfigvariable.The
callServiceConfiginitialized on line 66 withenableTelecom = trueis never used—tests useconfig(line 59) instead.🔎 Proposed fix
private lateinit var context: Context private lateinit var serviceIntentBuilder: ServiceIntentBuilder private lateinit var presenter: IncomingCallPresenter - private lateinit var callServiceConfig: CallServiceConfig private lateinit var notification: Notification private lateinit var streamVideoClient: StreamVideoClient private val callId = StreamCallId("default", "123", "default:123") private val serviceClass = CallService::class.java private val config = CallServiceConfig(serviceClass = serviceClass) @Before fun setup() { MockKAnnotations.init(this, relaxed = true) context = mockk(relaxed = true) serviceIntentBuilder = mockk(relaxed = true) - callServiceConfig = CallServiceConfig(enableTelecom = true) notification = mockk(relaxed = true)stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/observers/CallServiceRingingStateObserver.kt (1)
42-49: Observer usescall.scopeinstead of constructorscope.The observer collects
ringingStateoncall.scope(line 43), but the constructor accepts ascopeparameter. This inconsistency could cause lifecycle issues if the provided scope has different cancellation semantics. Consider using the providedscopeconsistently.🔎 Proposed fix
fun observe(onStopService: () -> Unit) { - call.scope.launch { + scope.launch { call.state.ringingState.collect { state -> logger.i { "Ringing state: $state" } handleRingingState(state, onStopService) } } }stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/observers/CallServiceEventObserver.kt (1)
136-144: Inconsistent scope usage between observers.
observeCallEvents(line 52) uses the constructorscope, butobserveConnectionState(line 137) usescall.scope. Use the providedscopeconsistently for predictable lifecycle management.🔎 Proposed fix
private fun observeConnectionState(onServiceStop: () -> Unit) { - call.scope.launch { + scope.launch { call.state.connection.collectLatest { event -> if (event is RealtimeConnection.Failed) { handleConnectionFailure() } } } }stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/observers/CallServiceNotificationUpdateObserver.kt (1)
183-190: Considerinternalvisibility forstartForegroundWithServiceType.This method is a simple delegation to
onStartService. If it's only called within the module, marking itinternalreduces public API surface.🔎 Proposed fix
- fun startForegroundWithServiceType( + internal fun startForegroundWithServiceType( notificationId: Int, notification: Notification, trigger: String, foregroundServiceType: Int, ) { onStartService(notificationId, notification, trigger, foregroundServiceType) }stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/notifications/internal/service/permissions/AudioCallPermissionManagerTest.kt (1)
33-44: Test coverage is adequate for the current implementation.The test correctly validates that both
PHONE_CALLandMICROPHONEtypes are declared. While additional tests could verify parent class integration (e.g.,androidQServiceType(),noPermissionServiceType()behavior across SDK versions as inLivestreamViewerPermissionManagerTest), the current coverage is sufficient since this class only overridesrequiredForegroundTypes.stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/handlers/StreamDefaultNotificationHandler.kt (1)
215-219: Prefer null-safe casting over broad exception handling.The current try-catch swallows all exceptions silently. Using null-safe casting is more idiomatic in Kotlin and avoids masking unexpected errors.
🔎 Proposed fix
- val createdByUserId = try { - payload["created_by_id"] as String - } catch (ex: Exception) { - "" - } + val createdByUserId = (payload["created_by_id"] as? String).orEmpty()stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/notifications/internal/service/observers/CallServiceEventObserverTest.kt (1)
188-188: Optional: Remove leftover comment.The comment
// nextappears to be a leftover debugging or TODO note.🔎 Proposed cleanup
- @Test // next + @Test fun `call ended stops service`() = runTest {stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/notifications/internal/service/observers/CallServiceNotificationUpdateObserverTest.kt (1)
137-137: Clarify commented-out code.The
advanceUntilIdle()call is commented out without explanation. If this is intentional for testing timing behavior, a comment would help future maintainers understand why. If it's leftover debug code, it should be removed.🔎 Suggested improvement
If intentional:
fun `incoming ringing state starts incoming foreground notification`() = runTest { observer.observe(context) -// advanceUntilIdle() + // Don't advance here to test immediate notification behaviorIf not needed:
fun `incoming ringing state starts incoming foreground notification`() = runTest { observer.observe(context) -// advanceUntilIdle()stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/notifications/internal/service/managers/CallServiceLifecycleManagerTest.kt (1)
129-143: MissingmockkObjectcleanup may cause test pollution.The tests using
mockkObject(StreamVideo.Companion)(lines 133, 150, 165, 179) do not unmock in@After. This can leak mocked state to other tests in the suite.🔎 Proposed fix
Add cleanup in the
tearDownmethod:@After fun tearDown() { Dispatchers.resetMain() + io.mockk.unmockkObject(StreamVideo.Companion) }Alternatively, use
unmockkAll()if other static mocks are also present.stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/Throttler.kt (1)
63-67: Stack trace index 4 is brittle and may break with call site changes.The hardcoded index assumes a specific call depth. Inline functions, lambda wrappers, or compiler optimizations could shift the index. The fallback to
action.hashCode()mitigates this but loses caller context.Consider requiring an explicit key for production callers to avoid relying on stack inspection.
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/managers/CallServiceNotificationManager.kt (1)
56-59: Redundantletblock on non-null parameter.
callIdis already non-null in the function signature, so theletblock adds no value.🔎 Proposed simplification
fun cancelNotifications(service: Service, callId: StreamCallId) { val notificationManager = NotificationManagerCompat.from(service) - callId.let { - logger.d { "[cancelNotifications], notificationId via hashcode: ${it.hashCode()}" } - notificationManager.cancel(it.hashCode()) - } + logger.d { "[cancelNotifications], notificationId via hashcode: ${callId.hashCode()}" } + notificationManager.cancel(callId.hashCode())stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/IncomingCallPresenter.kt (1)
102-126: Unusederrorparameter inshowNotification.The
errorparameter is passed but never used in the function body. Consider removing it or logging it for debugging purposes.🔎 Proposed fix
private fun showNotification( context: Context, notification: Notification?, callId: StreamCallId, - error: Any?, ): ShowIncomingCallResult {Or log the error:
if (!hasNotificationPermission(context) || notification == null) { logger.w { "[showIncomingCall] Cannot show notification - " + "permission=${hasNotificationPermission(context)}, " + - "notification=${notification != null}" + "notification=${notification != null}, " + + "error=$error" }stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/managers/CallServiceLifecycleManager.kt (2)
63-91: InconsistentStreamVideoaccess pattern.
initializeCallAndSocketandupdateRingingCallreceivestreamVideoas a parameter, butendCallaccessesStreamVideo.instanceOrNull()directly. Consider passingstreamVideoas a parameter for consistency and testability.🔎 Proposed signature change
-fun endCall(scope: CoroutineScope, callId: StreamCallId?) { +fun endCall(scope: CoroutineScope, streamVideo: StreamVideo?, callId: StreamCallId?) { callId?.let { id -> - StreamVideo.Companion.instanceOrNull()?.let { streamVideo -> + streamVideo?.let { val call = streamVideo.call(id.type, id.id)
93-106: Magic number2for member count check.The hardcoded
memberCount == 2assumes 1:1 call semantics. Consider extracting to a named constant with documentation explaining the business logic.🔎 Proposed improvement
companion object { /** In a 1:1 call (2 members), rejecting ends the call for both parties */ private const val ONE_TO_ONE_CALL_MEMBER_COUNT = 2 } // Usage: if (memberCount == ONE_TO_ONE_CALL_MEMBER_COUNT) {stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/CallService.kt (2)
108-110: Optional: Remove redundant exception string interpolation.The exception is already passed to
logger.e(exception), so including$exceptionin the message string is redundant. The logger will format the stack trace appropriately.🔎 Proposed refinement
val handler = CoroutineExceptionHandler { _, exception -> - logger.e(exception) { "[CallService#Scope] Uncaught exception: $exception" } + logger.e(exception) { "[CallService#Scope] Uncaught exception" } }
614-617: Address TODO: Verify sound ownership before stopping.The TODO comment indicates uncertainty about which call owns the sound. This could cause issues if multiple calls are active. Consider tracking sound ownership explicitly or verifying the current call before stopping sound.
Do you want me to help design a solution for tracking sound ownership across multiple calls?
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/ClientState.kt
Show resolved
Hide resolved
...oid-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/Debouncer.kt
Show resolved
Hide resolved
.../getstream/video/android/core/notifications/internal/receivers/LeaveCallBroadcastReceiver.kt
Outdated
Show resolved
Hide resolved
...rc/main/kotlin/io/getstream/video/android/core/notifications/internal/service/CallService.kt
Outdated
Show resolved
Hide resolved
...o/android/core/notifications/internal/service/managers/CallServiceNotificationManagerTest.kt
Show resolved
Hide resolved
...o/android/core/notifications/internal/service/managers/CallServiceNotificationManagerTest.kt
Outdated
Show resolved
Hide resolved
...in/io/getstream/video/android/core/notifications/internal/service/models/ServiceStateTest.kt
Outdated
Show resolved
Hide resolved
...n/io/getstream/video/android/core/notifications/internal/service/ServiceIntentBuilderTest.kt
Show resolved
Hide resolved
...n/io/getstream/video/android/core/notifications/internal/service/ServiceIntentBuilderTest.kt
Show resolved
Hide resolved
This commit introduces several improvements to enhance the stability of the foreground `CallService` and increases test coverage across various components. Key changes include: - A `Debouncer` was added to `CallService` to gracefully handle service stop requests, preventing premature termination. - Introduced `debouncer.cancel()` in `CallService.onDestroy()` to clean up pending actions. - The `ClientState.setActiveCall` logic now uses a constant for the service transition delay. - Added and fixed tests for `CallServiceLifecycleManager`, `CallServiceNotificationManager`, and `ServiceIntentBuilder` to ensure more reliable behavior. - Minor fixes in logging and code cleanup.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 576-579: notificationConfig() currently does an unsafe cast using
"as StreamVideoClient" which can throw ClassCastException; change it to use a
safe cast (as?) and handle the null case the same way as
streamDefaultNotificationHandler() and extractIntentParams() do (e.g., val
client = StreamVideo.instanceOrNull() as? StreamVideoClient ?: throw
IllegalStateException("...") or return a sensible default), then return
client.streamNotificationManager.notificationConfig; this keeps behavior
consistent and prevents ClassCastException.
- Around line 586-602: stopServiceGracefully is comparing differenceInSeconds
(seconds) to SERVICE_DESTROY_THRESHOLD_TIME_MS (milliseconds), inverting the
debounce logic; fix by measuring the elapsed time in milliseconds (use
Duration.toMillis() into differenceInMillis) and compare that to
SERVICE_DESTROY_THRESHOLD_TIME_MS, and when scheduling the debouncer use the
remaining delay (max(0, SERVICE_DESTROY_THRESHOLD_TIME_MS - differenceInMillis))
for debouncer.submit so internalStopServiceGracefully() is run immediately if
elapsed >= threshold or after the correct remaining ms otherwise.
In
@stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/notifications/internal/service/managers/CallServiceNotificationManagerTest.kt:
- Around line 121-132: The test for
CallServiceNotificationManagerTest::justNotify stubs
ActivityCompat.checkSelfPermission but the implementation uses
ContextCompat.checkSelfPermission (other tests stub ContextCompat), causing
flaky stubbing; update the stub in the justNotify test to use
ContextCompat.checkSelfPermission with the same any() arguments and return
PackageManager.PERMISSION_GRANTED so the permission check is correctly
intercepted when calling sut.justNotify(service, callId, 1001, notification) and
StreamVideo.instanceOrNull() remains mocked to null.
🧹 Nitpick comments (2)
stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/notifications/internal/service/managers/CallServiceNotificationManagerTest.kt (1)
52-52: Remove unusedcontextvariable.The
contextvariable is declared and initialized but never referenced in any test. Theservicemock is used directly as the Context argument throughout the tests.♻️ Proposed refactor
private lateinit var sut: CallServiceNotificationManager - private lateinit var context: Context private val service: Service = mockk(relaxed = true)@Before fun setup() { - context = mockk(relaxed = true) sut = CallServiceNotificationManager()Also applies to: 63-63
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/CallService.kt (1)
80-81: Scope cancelled in multiple locations - potential flow issue.The
serviceScopeis cancelled in bothonDestroy()(line 560) andinternalStopServiceGracefully()(line 619). Whilecancel()is idempotent and won't crash, this suggests a potential flow issue: ifinternalStopServiceGracefully()is called, it cancels the scope and callsstopSelf(), which triggersonDestroy(), leading to a second cancellation.Consider removing the cancellation from one location. Typically,
onDestroy()is the appropriate place for cleanup since it's guaranteed to be called when the service is destroyed, regardless of how the service is stopped.♻️ Proposed refactor
private fun internalStopServiceGracefully() { logger.d { "[internalStopServiceGracefully] hashcode: ${hashCode()}" } stopForeground(STOP_FOREGROUND_REMOVE) serviceState.currentCallId?.let { notificationManager.cancelNotifications(this, it) } serviceState.unregisterToggleCameraBroadcastReceiver(this) /** * Temp Fix!! The observeRingingState scope was getting cancelled and as a result, * ringing state was not properly updated */ serviceState.soundPlayer?.stopCallSound() // TODO should check which call owns the sound - serviceScope.cancel() stopSelf() }Also applies to: 554-562, 604-621
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
demo-app/src/main/kotlin/io/getstream/video/android/util/StreamVideoInitHelper.ktstream-video-android-core/api/stream-video-android-core.apistream-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/Debouncer.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/receivers/LeaveCallBroadcastReceiver.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/CallService.ktstream-video-android-core/src/test/kotlin/io/getstream/video/android/core/notifications/internal/service/ServiceIntentBuilderTest.ktstream-video-android-core/src/test/kotlin/io/getstream/video/android/core/notifications/internal/service/managers/CallServiceLifecycleManagerTest.ktstream-video-android-core/src/test/kotlin/io/getstream/video/android/core/notifications/internal/service/managers/CallServiceNotificationManagerTest.ktstream-video-android-core/src/test/kotlin/io/getstream/video/android/core/notifications/internal/service/models/ServiceStateTest.kt
🚧 Files skipped from review as they are similar to previous changes (4)
- stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/notifications/internal/service/ServiceIntentBuilderTest.kt
- stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/receivers/LeaveCallBroadcastReceiver.kt
- stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/notifications/internal/service/models/ServiceStateTest.kt
- stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/Debouncer.kt
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{kt,java}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{kt,java}: Use Kotlin with JVM toolchain 17; Java is legacy-only
Use 4-space indentation with no trailing whitespace
Avoid wildcard imports
Files:
stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/notifications/internal/service/managers/CallServiceLifecycleManagerTest.ktstream-video-android-core/src/test/kotlin/io/getstream/video/android/core/notifications/internal/service/managers/CallServiceNotificationManagerTest.ktdemo-app/src/main/kotlin/io/getstream/video/android/util/StreamVideoInitHelper.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/CallService.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/ClientState.kt
**/*.{kt,kts}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{kt,kts}: Use PascalCase for types and Composables (e.g.,StreamCallActivity,ParticipantGrid)
Use camelCase for functions and values
Use UPPER_SNAKE_CASE for constants only when truly constant
Prefer explicit visibility modifiers; limitinternalleakage across modules
Keep critical RTC paths off the main thread; prefer coroutines with structured scopes
Monitor logging verbosity; rely onStreamVideoImpl.developmentModefor guardrails
Use KDoc (/** ... */) for public APIs and complex subsystems; link to Stream docs when relevant
Group large files with// regionjudiciously; keep commentary purposeful
Sanitize logs to avoid dumping JWTs, ICE tokens, or call IDs in verbose logs
Pause/resume capture on lifecycle changes; ensure background audio routing is intentional
Validate orientation, aspect ratio, and dynascale handling for both portrait/landscape phones and tablets
Keep concurrency deterministic—use structured coroutines and avoid global scope
Ensure cleanup/teardown paths handle cancellation and failure (important for sockets, queues, retries)
Files:
stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/notifications/internal/service/managers/CallServiceLifecycleManagerTest.ktstream-video-android-core/src/test/kotlin/io/getstream/video/android/core/notifications/internal/service/managers/CallServiceNotificationManagerTest.ktdemo-app/src/main/kotlin/io/getstream/video/android/util/StreamVideoInitHelper.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/CallService.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/ClientState.kt
**/*.{kt,java,kts,gradle.kts}
📄 CodeRabbit inference engine (AGENTS.md)
Follow Spotless formatting; ensure custom license headers are in
spotless/directory
Files:
stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/notifications/internal/service/managers/CallServiceLifecycleManagerTest.ktstream-video-android-core/src/test/kotlin/io/getstream/video/android/core/notifications/internal/service/managers/CallServiceNotificationManagerTest.ktdemo-app/src/main/kotlin/io/getstream/video/android/util/StreamVideoInitHelper.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/CallService.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/ClientState.kt
**/src/test/**/*.{kt,kts}
📄 CodeRabbit inference engine (AGENTS.md)
**/src/test/**/*.{kt,kts}: Use descriptive backtick test names (e.g.,fun `joining a call publishes participant tracks`())
UseTestBasefor fast unit tests andIntegrationTestBasefor end-to-end call flows
Files:
stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/notifications/internal/service/managers/CallServiceLifecycleManagerTest.ktstream-video-android-core/src/test/kotlin/io/getstream/video/android/core/notifications/internal/service/managers/CallServiceNotificationManagerTest.kt
🧠 Learnings (8)
📚 Learning: 2025-12-19T09:15:37.269Z
Learnt from: CR
Repo: GetStream/stream-video-android PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-19T09:15:37.269Z
Learning: Applies to stream-video-android-previewdata/**/*.{kt,kts} : Keep test fixtures in `stream-video-android-previewdata`; avoid duplicating builder logic
Applied to files:
stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/notifications/internal/service/managers/CallServiceLifecycleManagerTest.ktstream-video-android-core/src/test/kotlin/io/getstream/video/android/core/notifications/internal/service/managers/CallServiceNotificationManagerTest.ktdemo-app/src/main/kotlin/io/getstream/video/android/util/StreamVideoInitHelper.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/CallService.kt
📚 Learning: 2025-12-19T09:15:37.269Z
Learnt from: CR
Repo: GetStream/stream-video-android PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-19T09:15:37.269Z
Learning: Applies to **/src/test/**/*.{kt,kts} : Use descriptive backtick test names (e.g., ``fun `joining a call publishes participant tracks`()``)
Applied to files:
stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/notifications/internal/service/managers/CallServiceLifecycleManagerTest.ktstream-video-android-core/src/test/kotlin/io/getstream/video/android/core/notifications/internal/service/managers/CallServiceNotificationManagerTest.kt
📚 Learning: 2025-12-19T09:15:37.269Z
Learnt from: CR
Repo: GetStream/stream-video-android PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-19T09:15:37.269Z
Learning: Applies to **/src/test/**/*.{kt,kts} : Use `TestBase` for fast unit tests and `IntegrationTestBase` for end-to-end call flows
Applied to files:
stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/notifications/internal/service/managers/CallServiceLifecycleManagerTest.kt
📚 Learning: 2025-12-19T14:10:16.577Z
Learnt from: aleksandar-apostolov
Repo: GetStream/stream-video-android PR: 1587
File: stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/Call.kt:795-798
Timestamp: 2025-12-19T14:10:16.577Z
Learning: In the `rejoin()` function in `stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/Call.kt`, the design invariant is that `rejoin()` should only be called when there is an active session (`this.session != null`). Therefore, using `this.session!!` is safe and intentional.
Applied to files:
stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/notifications/internal/service/managers/CallServiceLifecycleManagerTest.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/CallService.kt
📚 Learning: 2025-12-19T09:15:37.269Z
Learnt from: CR
Repo: GetStream/stream-video-android PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-19T09:15:37.269Z
Learning: Applies to **/*.{kt,kts} : Ensure cleanup/teardown paths handle cancellation and failure (important for sockets, queues, retries)
Applied to files:
stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/notifications/internal/service/managers/CallServiceLifecycleManagerTest.ktstream-video-android-core/src/test/kotlin/io/getstream/video/android/core/notifications/internal/service/managers/CallServiceNotificationManagerTest.kt
📚 Learning: 2025-12-19T09:15:37.269Z
Learnt from: CR
Repo: GetStream/stream-video-android PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-19T09:15:37.269Z
Learning: Applies to **/*.{kt,kts} : Keep concurrency deterministic—use structured coroutines and avoid global scope
Applied to files:
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/CallService.kt
📚 Learning: 2025-12-19T09:15:37.269Z
Learnt from: CR
Repo: GetStream/stream-video-android PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-19T09:15:37.269Z
Learning: Applies to **/*.{kt,kts} : Keep critical RTC paths off the main thread; prefer coroutines with structured scopes
Applied to files:
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/CallService.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/ClientState.kt
📚 Learning: 2025-12-19T09:15:37.269Z
Learnt from: CR
Repo: GetStream/stream-video-android PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-19T09:15:37.269Z
Learning: Applies to **/*.{kt,kts} : Pause/resume capture on lifecycle changes; ensure background audio routing is intentional
Applied to files:
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/CallService.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/ClientState.kt
🧬 Code graph analysis (2)
demo-app/src/main/kotlin/io/getstream/video/android/util/StreamVideoInitHelper.kt (1)
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/CallServiceConfigRegistry.kt (3)
register(76-78)register(95-99)register(115-119)
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/CallService.kt (5)
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/ServiceNotificationRetriever.kt (1)
getNotificationPair(56-133)stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/IncomingCallPresenter.kt (1)
showIncomingCall(34-63)stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/sounds/RingingCallVibrationConfig.kt (1)
hashCode(42-46)stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/observers/CallServiceNotificationUpdateObserver.kt (1)
startForegroundWithServiceType(183-190)stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/utils/AndroidUtils.kt (1)
startForegroundWithServiceType(190-219)
⏰ 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). (5)
- GitHub Check: base-android-ci / Build
- GitHub Check: compare-sdk-sizes / Compare SDK sizes
- GitHub Check: base-android-ci / Run unit tests
- GitHub Check: base-android-ci / Run static checks
- GitHub Check: Build / compose apks
🔇 Additional comments (17)
stream-video-android-core/api/stream-video-android-core.api (1)
7783-7784: LGTM!The new
updateNotification(int, Notification)overload is a clean, non-breaking API addition that supports the PR objective of reusing notification IDs during service transitions from ringing to active calls.demo-app/src/main/kotlin/io/getstream/video/android/util/StreamVideoInitHelper.kt (1)
240-243: Good refactor toward centralized config reuse.This change simplifies audio call registration by reusing
DefaultCallConfigurations.audioCall—which correctly setsAudioCallServicewithUSAGE_VOICE_COMMUNICATION—and overriding onlyenableTelecom. This aligns well with the PR's goal of centralizing configuration handling.stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/notifications/internal/service/managers/CallServiceNotificationManagerTest.kt (5)
79-99: LGTM!The test correctly verifies that notifications are dispatched when POST_NOTIFICATIONS permission is granted.
101-119: LGTM!The test correctly verifies that notifications are not dispatched when POST_NOTIFICATIONS permission is denied.
134-154: LGTM!The test correctly verifies that call notifications are cancelled using both the callId hashCode and the stored notificationId from call state.
156-183: LGTM!The test correctly verifies that the media session is cleared through the notification handler chain when notifications are cancelled.
185-194: LGTM!The test correctly verifies that notification cancellation still occurs (using the fallback callId hashCode) even when the StreamVideo instance is null, ensuring defensive cleanup behavior.
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/ClientState.kt (2)
208-210: Good observability improvements.The added logging statements help track call ID comparisons and service trigger values, making call lifecycle debugging easier.
Also applies to: 240-242, 264-264
252-257: Well-structured helper function.The
transitionToAcceptCallfunction appropriately centralizes the ringing-to-active transition logic. It correctly checks call ID matching, stops the call sound, and clears the ringing state, preventing code duplication in thesetActiveCallbranches.stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/notifications/internal/service/managers/CallServiceLifecycleManagerTest.kt (1)
47-199: LGTM! Comprehensive and well-structured test suite.The test coverage is thorough, covering both success and error paths for
initializeCallAndSocket, state updates viaupdateRingingCall, and multipleendCallscenarios (outgoing, incoming with varying member counts, unknown ringing state, and null callId). The use ofTestScopewithStandardTestDispatcherandadvanceUntilIdle()ensures deterministic coroutine testing. Mock setup withMutableStateFlowfor reactive state propagation is appropriate, and all tests follow a clear arrange-act-assert pattern with descriptive backtick names per coding guidelines.stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/CallService.kt (7)
115-118: LGTM - Clean lifecycle initialization.The
onCreate()implementation correctly initializes the service start time, which is used for debouncer logic instopServiceGracefully().
178-245: LGTM - Well-structured command handling with proper early exits.The refactored
onStartCommand()follows best practices with early exit conditions, null-safe parameter extraction, and appropriate return values for different scenarios. The delegation to helper methods improves readability and maintainability.
247-270: LGTM - Clean initialization with structured concurrency.The
initializeService()method properly uses the instance-scopedserviceScopeand coordinates initialization across lifecycle manager, sound player, and observers. Based on learnings, keeping critical RTC paths off the main thread with structured coroutines is followed correctly.
272-320: LGTM - Clear notification routing with type-safe results.The centralized
handleNotification()provides clear routing for different call triggers and uses an enum return type to communicate results. Good separation of null-handling concerns.
425-432: LGTM - Safe parameter extraction pattern.The
extractIntentParams()method correctly uses safe casting (as?) and returns null if any required parameter is missing, preventingClassCastExceptionand ensuring type safety.
500-538: LGTM - Observer setup with structured concurrency.The
observeCall()method properly sets up observers with the instance-scopedserviceScope, maintaining deterministic concurrency as per coding guidelines. The conditional notification observer setup is a good optimization.
95-113: LGTM - Well-defined constants and exception handling.The companion object provides clear constant definitions with proper unit suffixes (
_MSfor milliseconds) and includes aCoroutineExceptionHandlerfor logging uncaught exceptions, which is a good defensive practice.
...rc/main/kotlin/io/getstream/video/android/core/notifications/internal/service/CallService.kt
Outdated
Show resolved
Hide resolved
...o/android/core/notifications/internal/service/managers/CallServiceNotificationManagerTest.kt
Show resolved
Hide resolved
Makes `CallService` more robust by handling cases where `StreamVideo.instanceOrNull()` might return null. The `notificationConfig()` function now returns a nullable `NotificationConfig?` to prevent potential crashes. This change is propagated to `maybeHandleMediaIntent` to ensure it safely handles the nullable configuration.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/CallService.kt:
- Line 106: Remove the unused constant SERVICE_DESTROY_THROTTLE_TIME_MS from
CallService (in CallService.kt); delete the const val declaration and any
surrounding comment/whitespace, ensure no other code references
SERVICE_DESTROY_THROTTLE_TIME_MS remain, then run a quick build/lint to confirm
no usages or unused-import warnings are introduced.
- Around line 564-567: The private function debugPrintLastStackFrames(count: Int
= 10) is unused; remove it to eliminate dead code, or if you intend to keep it
for future debugging, add a clear TODO comment above debugPrintLastStackFrames
explaining its purpose and planned usage (or annotate it with
@Suppress("unused") and the TODO) so reviewers know it’s intentionally retained.
In
@stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/notifications/internal/service/managers/CallServiceNotificationManagerTest.kt:
- Around line 122-132: The test "justNotify is safe when StreamVideo instance is
null" currently only ensures no crash but lacks an explicit assertion; after
calling sut.justNotify(service, callId, 1001, notification) add a MockK
verification that notificationDispatcher was not invoked (e.g., verify {
notificationDispatcher wasNot Called } or verify(exactly = 0) {
notificationDispatcher(any(), any(), any()) }) to assert null-safety behavior
programmatically, referencing the notificationDispatcher mock and the justNotify
call.
🧹 Nitpick comments (4)
stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/notifications/internal/service/managers/CallServiceNotificationManagerTest.kt (4)
52-52: Remove unusedcontextvariable.The
contextvariable is declared and initialized but never used in any test. Remove it to eliminate dead code.🧹 Proposed cleanup
class CallServiceNotificationManagerTest { private lateinit var sut: CallServiceNotificationManager - private lateinit var context: Context private val service: Service = mockk(relaxed = true)@Before fun setup() { - context = mockk(relaxed = true) sut = CallServiceNotificationManager()Also applies to: 63-63
66-66: Remove unusedActivityCompatmock.All tests use
ContextCompat.checkSelfPermission. TheActivityCompatmock on line 66 is never referenced.🧹 Proposed cleanup
- mockkStatic(ActivityCompat::class) mockkStatic(ContextCompat::class)
124-124: Use consistent permission check parameters.Other tests stub
ContextCompat.checkSelfPermission(any(), Manifest.permission.POST_NOTIFICATIONS)with the specific permission constant, while this test usesany(), any(). Use the explicit permission constant for consistency and clarity.♻️ Proposed fix
every { - ContextCompat.checkSelfPermission(any(), any()) + ContextCompat.checkSelfPermission(any(), Manifest.permission.POST_NOTIFICATIONS) } returns PackageManager.PERMISSION_GRANTED
144-144: Remove duplicate mock setup.Line 138 already configures
StreamVideo.instanceOrNull()to returnstreamVideoClient. This line duplicates that setup.🧹 Proposed cleanup
val call = Call(streamVideoClient, callId.type, callId.id, mockk()) every { streamVideoClient.call(callId.type, callId.id) } returns call - every { StreamVideo.instanceOrNull() } returns streamVideoClient sut.cancelNotifications(service, callId)
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/CallService.ktstream-video-android-core/src/test/kotlin/io/getstream/video/android/core/notifications/internal/service/managers/CallServiceNotificationManagerTest.kt
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{kt,java}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{kt,java}: Use Kotlin with JVM toolchain 17; Java is legacy-only
Use 4-space indentation with no trailing whitespace
Avoid wildcard imports
Files:
stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/notifications/internal/service/managers/CallServiceNotificationManagerTest.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/CallService.kt
**/*.{kt,kts}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{kt,kts}: Use PascalCase for types and Composables (e.g.,StreamCallActivity,ParticipantGrid)
Use camelCase for functions and values
Use UPPER_SNAKE_CASE for constants only when truly constant
Prefer explicit visibility modifiers; limitinternalleakage across modules
Keep critical RTC paths off the main thread; prefer coroutines with structured scopes
Monitor logging verbosity; rely onStreamVideoImpl.developmentModefor guardrails
Use KDoc (/** ... */) for public APIs and complex subsystems; link to Stream docs when relevant
Group large files with// regionjudiciously; keep commentary purposeful
Sanitize logs to avoid dumping JWTs, ICE tokens, or call IDs in verbose logs
Pause/resume capture on lifecycle changes; ensure background audio routing is intentional
Validate orientation, aspect ratio, and dynascale handling for both portrait/landscape phones and tablets
Keep concurrency deterministic—use structured coroutines and avoid global scope
Ensure cleanup/teardown paths handle cancellation and failure (important for sockets, queues, retries)
Files:
stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/notifications/internal/service/managers/CallServiceNotificationManagerTest.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/CallService.kt
**/*.{kt,java,kts,gradle.kts}
📄 CodeRabbit inference engine (AGENTS.md)
Follow Spotless formatting; ensure custom license headers are in
spotless/directory
Files:
stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/notifications/internal/service/managers/CallServiceNotificationManagerTest.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/CallService.kt
**/src/test/**/*.{kt,kts}
📄 CodeRabbit inference engine (AGENTS.md)
**/src/test/**/*.{kt,kts}: Use descriptive backtick test names (e.g.,fun `joining a call publishes participant tracks`())
UseTestBasefor fast unit tests andIntegrationTestBasefor end-to-end call flows
Files:
stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/notifications/internal/service/managers/CallServiceNotificationManagerTest.kt
🧠 Learnings (8)
📚 Learning: 2025-12-19T09:15:37.269Z
Learnt from: CR
Repo: GetStream/stream-video-android PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-19T09:15:37.269Z
Learning: Applies to stream-video-android-previewdata/**/*.{kt,kts} : Keep test fixtures in `stream-video-android-previewdata`; avoid duplicating builder logic
Applied to files:
stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/notifications/internal/service/managers/CallServiceNotificationManagerTest.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/CallService.kt
📚 Learning: 2025-12-19T09:15:37.269Z
Learnt from: CR
Repo: GetStream/stream-video-android PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-19T09:15:37.269Z
Learning: Applies to **/src/test/**/*.{kt,kts} : Use descriptive backtick test names (e.g., ``fun `joining a call publishes participant tracks`()``)
Applied to files:
stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/notifications/internal/service/managers/CallServiceNotificationManagerTest.kt
📚 Learning: 2025-12-19T09:15:37.269Z
Learnt from: CR
Repo: GetStream/stream-video-android PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-19T09:15:37.269Z
Learning: Applies to **/*.{kt,kts} : Ensure cleanup/teardown paths handle cancellation and failure (important for sockets, queues, retries)
Applied to files:
stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/notifications/internal/service/managers/CallServiceNotificationManagerTest.kt
📚 Learning: 2026-01-07T09:52:45.549Z
Learnt from: rahul-lohra
Repo: GetStream/stream-video-android PR: 1588
File: stream-video-android-core/api/stream-video-android-core.api:3561-3569
Timestamp: 2026-01-07T09:52:45.549Z
Learning: The `LocalCallMissedEvent` class in `stream-video-android-core` is only instantiated internally by the library maintainers. Clients only read these instances and never construct them directly, so breaking changes to the constructor are acceptable.
Applied to files:
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/CallService.kt
📚 Learning: 2025-12-19T09:15:37.269Z
Learnt from: CR
Repo: GetStream/stream-video-android PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-19T09:15:37.269Z
Learning: Applies to **/*.{kt,kts} : Keep concurrency deterministic—use structured coroutines and avoid global scope
Applied to files:
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/CallService.kt
📚 Learning: 2025-12-19T09:15:37.269Z
Learnt from: CR
Repo: GetStream/stream-video-android PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-19T09:15:37.269Z
Learning: Applies to **/*.{kt,kts} : Keep critical RTC paths off the main thread; prefer coroutines with structured scopes
Applied to files:
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/CallService.kt
📚 Learning: 2025-12-19T14:10:16.577Z
Learnt from: aleksandar-apostolov
Repo: GetStream/stream-video-android PR: 1587
File: stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/Call.kt:795-798
Timestamp: 2025-12-19T14:10:16.577Z
Learning: In the `rejoin()` function in `stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/Call.kt`, the design invariant is that `rejoin()` should only be called when there is an active session (`this.session != null`). Therefore, using `this.session!!` is safe and intentional.
Applied to files:
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/CallService.kt
📚 Learning: 2025-12-19T09:15:37.269Z
Learnt from: CR
Repo: GetStream/stream-video-android PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-19T09:15:37.269Z
Learning: Applies to **/*.{kt,kts} : Pause/resume capture on lifecycle changes; ensure background audio routing is intentional
Applied to files:
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/CallService.kt
⏰ 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). (5)
- GitHub Check: base-android-ci / Build
- GitHub Check: base-android-ci / Run unit tests
- GitHub Check: base-android-ci / Run static checks
- GitHub Check: Build / compose apks
- GitHub Check: compare-sdk-sizes / Compare SDK sizes
🔇 Additional comments (4)
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/CallService.kt (4)
80-81: LGTM: Instance-level scope properly tied to service lifecycle.The
serviceScopeis now correctly defined as an instance property and is cancelled in bothonDestroy()(line 560) andinternalStopServiceGracefully()(line 619), preventing the previous issue where a shared companion scope was cancelled prematurely.
428-428: LGTM: Safe casts prevent ClassCastException.All
StreamVideo.instanceOrNull()calls now use safe casts (as?) with proper null handling, addressing the previous unsafe cast issues. The pattern is applied consistently acrossextractIntentParams(),streamDefaultNotificationHandler(), andnotificationConfig().Also applies to: 570-570, 577-577
500-538: LGTM: Well-structured observer pattern with conditional registration.The observer setup properly separates concerns:
- Ringing state changes
- Call events (stop service, remove incoming)
- Notification updates (only when
enableCallNotificationUpdatesis true)The conditional registration at line 517 is a good optimization that prevents unnecessary observation when the feature is disabled.
115-118: LGTM: Proper lifecycle management with resource cleanup.The
onCreate()method initializes service start time for debouncer calculations, andonDestroy()properly cleans up all resources:
- Sound player audio resources
- Debouncer cancellation prevents pending tasks
- Service scope cancellation ensures structured concurrency cleanup
This ensures no resource leaks or lingering coroutines.
Also applies to: 554-562
| const val EXTRA_STOP_SERVICE = "io.getstream.video.android.core.stop_service" | ||
|
|
||
| const val SERVICE_DESTROY_THRESHOLD_TIME_MS = 2_000L | ||
| const val SERVICE_DESTROY_THROTTLE_TIME_MS = 1_000L |
There was a problem hiding this comment.
Remove unused constant.
SERVICE_DESTROY_THROTTLE_TIME_MS is defined but never used anywhere in the file. Remove it to reduce maintenance burden.
🧹 Proposed fix
- const val SERVICE_DESTROY_THROTTLE_TIME_MS = 1_000L🤖 Prompt for AI Agents
In
@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/CallService.kt
at line 106, Remove the unused constant SERVICE_DESTROY_THROTTLE_TIME_MS from
CallService (in CallService.kt); delete the const val declaration and any
surrounding comment/whitespace, ensure no other code references
SERVICE_DESTROY_THROTTLE_TIME_MS remain, then run a quick build/lint to confirm
no usages or unused-import warnings are introduced.
There was a problem hiding this comment.
It is used in io.getstream.video.android.core.notifications.internal.service.ServiceLaucher.stopService()
...rc/main/kotlin/io/getstream/video/android/core/notifications/internal/service/CallService.kt
Outdated
Show resolved
Hide resolved
| fun `justNotify is safe when StreamVideo instance is null`() { | ||
| every { | ||
| ContextCompat.checkSelfPermission(any(), any()) | ||
| } returns PackageManager.PERMISSION_GRANTED | ||
|
|
||
| every { StreamVideo.instanceOrNull() } returns null | ||
|
|
||
| sut.justNotify(service, callId, 1001, notification) | ||
|
|
||
| // Should not crash | ||
| } |
There was a problem hiding this comment.
Add explicit verification to null-safety test.
The test validates that justNotify doesn't crash when StreamVideo.instanceOrNull() returns null, but lacks an explicit assertion. Add a verification that notificationDispatcher was not invoked to make the expected behavior programmatic.
✅ Proposed fix
every { StreamVideo.instanceOrNull() } returns null
sut.justNotify(service, callId, 1001, notification)
- // Should not crash
+ verify { notificationDispatcher wasNot Called }
}🤖 Prompt for AI Agents
In
@stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/notifications/internal/service/managers/CallServiceNotificationManagerTest.kt
around lines 122 - 132, The test "justNotify is safe when StreamVideo instance
is null" currently only ensures no crash but lacks an explicit assertion; after
calling sut.justNotify(service, callId, 1001, notification) add a MockK
verification that notificationDispatcher was not invoked (e.g., verify {
notificationDispatcher wasNot Called } or verify(exactly = 0) {
notificationDispatcher(any(), any(), any()) }) to assert null-safety behavior
programmatically, referencing the notificationDispatcher mock and the justNotify
call.
There was a problem hiding this comment.
Please ignore this
There was a problem hiding this comment.
@rahul-lohra, understood! I'll skip this suggestion. 👍
🧠 Learnings used
Learnt from: aleksandar-apostolov
Repo: GetStream/stream-video-android PR: 1587
File: stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/Call.kt:795-798
Timestamp: 2025-12-19T14:10:16.577Z
Learning: In the `rejoin()` function in `stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/Call.kt`, the design invariant is that `rejoin()` should only be called when there is an active session (`this.session != null`). Therefore, using `this.session!!` is safe and intentional.
Learnt from: CR
Repo: GetStream/stream-video-android PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-19T09:15:37.269Z
Learning: Applies to stream-video-android-previewdata/**/*.{kt,kts} : Keep test fixtures in `stream-video-android-previewdata`; avoid duplicating builder logic
Learnt from: CR
Repo: GetStream/stream-video-android PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-19T09:15:37.269Z
Learning: Applies to **/src/test/**/*.{kt,kts} : Use descriptive backtick test names (e.g., ``fun `joining a call publishes participant tracks`()``)
Learnt from: CR
Repo: GetStream/stream-video-android PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-19T09:15:37.269Z
Learning: Applies to **/src/test/**/*.{kt,kts} : Use `TestBase` for fast unit tests and `IntegrationTestBase` for end-to-end call flows
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 581-597: The debouncer currently always waits the full
SERVICE_DESTROY_THRESHOLD_TIME_MS, ignoring elapsed time; update
stopServiceGracefully to compute the remaining wait as (thresholdSeconds -
differenceInSeconds) (clamped to >=0) and pass that remainingSeconds to
debouncer.submit, calling internalStopServiceGracefully immediately if
remainingSeconds == 0; reference serviceState.startTime,
SERVICE_DESTROY_THRESHOLD_TIME_MS, stopServiceGracefully,
internalStopServiceGracefully, and debouncer.submit when making the change.
In
@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/utils/AndroidUtils.kt:
- Around line 261-265: The debugPrintLastStackFrames function currently uses
Thread.currentThread().stackTrace.takeLast(count) which returns the oldest
frames; change it to capture the most recent call frames by skipping the
internal stack entries and taking the next 'count' entries instead (e.g., use
stack.drop(2).take(count) or equivalent), then join and Log.d as before; update
debugPrintLastStackFrames and ensure you handle cases where the stack has fewer
entries than count.
🧹 Nitpick comments (2)
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/utils/AndroidUtils.kt (2)
261-265: Use StreamLog for consistency with the rest of the codebase.This file consistently uses
StreamLog.efor logging (see lines 120, 137, 159, 178, 232, 248). UsingLog.ddirectly breaks that pattern and bypasses any centralized logging configuration.♻️ Proposed refactor to use StreamLog
internal fun debugPrintLastStackFrames(tag: String, count: Int = 10) { val stack = Thread.currentThread().stackTrace val message = stack.drop(2).take(count).joinToString("\n") - Log.d(tag, message) + StreamLog.d(tag) { message } }
261-265: Consider respectingdevelopmentModefor debug utilities.As per coding guidelines: "Monitor logging verbosity; rely on
StreamVideoImpl.developmentModefor guardrails." Since this is a debug utility that could be called in production, consider gating it with a development-mode check to prevent unwanted logging overhead.Based on coding guidelines regarding development mode guardrails for logging.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/CallService.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/utils/AndroidUtils.kt
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{kt,java}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{kt,java}: Use Kotlin with JVM toolchain 17; Java is legacy-only
Use 4-space indentation with no trailing whitespace
Avoid wildcard imports
Files:
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/utils/AndroidUtils.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/CallService.kt
**/*.{kt,kts}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{kt,kts}: Use PascalCase for types and Composables (e.g.,StreamCallActivity,ParticipantGrid)
Use camelCase for functions and values
Use UPPER_SNAKE_CASE for constants only when truly constant
Prefer explicit visibility modifiers; limitinternalleakage across modules
Keep critical RTC paths off the main thread; prefer coroutines with structured scopes
Monitor logging verbosity; rely onStreamVideoImpl.developmentModefor guardrails
Use KDoc (/** ... */) for public APIs and complex subsystems; link to Stream docs when relevant
Group large files with// regionjudiciously; keep commentary purposeful
Sanitize logs to avoid dumping JWTs, ICE tokens, or call IDs in verbose logs
Pause/resume capture on lifecycle changes; ensure background audio routing is intentional
Validate orientation, aspect ratio, and dynascale handling for both portrait/landscape phones and tablets
Keep concurrency deterministic—use structured coroutines and avoid global scope
Ensure cleanup/teardown paths handle cancellation and failure (important for sockets, queues, retries)
Files:
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/utils/AndroidUtils.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/CallService.kt
**/*.{kt,java,kts,gradle.kts}
📄 CodeRabbit inference engine (AGENTS.md)
Follow Spotless formatting; ensure custom license headers are in
spotless/directory
Files:
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/utils/AndroidUtils.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/CallService.kt
🧠 Learnings (6)
📚 Learning: 2026-01-07T09:52:52.695Z
Learnt from: rahul-lohra
Repo: GetStream/stream-video-android PR: 1588
File: stream-video-android-core/api/stream-video-android-core.api:3561-3569
Timestamp: 2026-01-07T09:52:52.695Z
Learning: The `LocalCallMissedEvent` class in `stream-video-android-core` is only instantiated internally by the library maintainers. Clients only read these instances and never construct them directly, so breaking changes to the constructor are acceptable.
Applied to files:
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/CallService.kt
📚 Learning: 2025-12-19T09:15:37.269Z
Learnt from: CR
Repo: GetStream/stream-video-android PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-19T09:15:37.269Z
Learning: Applies to **/*.{kt,kts} : Keep concurrency deterministic—use structured coroutines and avoid global scope
Applied to files:
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/CallService.kt
📚 Learning: 2025-12-19T09:15:37.269Z
Learnt from: CR
Repo: GetStream/stream-video-android PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-19T09:15:37.269Z
Learning: Applies to **/*.{kt,kts} : Keep critical RTC paths off the main thread; prefer coroutines with structured scopes
Applied to files:
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/CallService.kt
📚 Learning: 2025-12-19T14:10:16.577Z
Learnt from: aleksandar-apostolov
Repo: GetStream/stream-video-android PR: 1587
File: stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/Call.kt:795-798
Timestamp: 2025-12-19T14:10:16.577Z
Learning: In the `rejoin()` function in `stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/Call.kt`, the design invariant is that `rejoin()` should only be called when there is an active session (`this.session != null`). Therefore, using `this.session!!` is safe and intentional.
Applied to files:
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/CallService.kt
📚 Learning: 2025-12-19T09:15:37.269Z
Learnt from: CR
Repo: GetStream/stream-video-android PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-19T09:15:37.269Z
Learning: Applies to stream-video-android-previewdata/**/*.{kt,kts} : Keep test fixtures in `stream-video-android-previewdata`; avoid duplicating builder logic
Applied to files:
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/CallService.kt
📚 Learning: 2025-12-19T09:15:37.269Z
Learnt from: CR
Repo: GetStream/stream-video-android PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-19T09:15:37.269Z
Learning: Applies to **/*.{kt,kts} : Pause/resume capture on lifecycle changes; ensure background audio routing is intentional
Applied to files:
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/CallService.kt
🧬 Code graph analysis (1)
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/CallService.kt (6)
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/ServiceNotificationRetriever.kt (2)
getNotificationPair(56-133)notificationConfig(135-138)stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/IncomingCallPresenter.kt (1)
showIncomingCall(34-63)stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/ServiceLauncher.kt (2)
showIncomingCall(69-117)removeIncomingCall(211-236)stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/sounds/RingingCallVibrationConfig.kt (1)
hashCode(42-46)stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/utils/AndroidUtils.kt (1)
startForegroundWithServiceType(190-219)stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/observers/CallServiceNotificationUpdateObserver.kt (1)
startForegroundWithServiceType(183-190)
⏰ 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). (5)
- GitHub Check: base-android-ci / Run unit tests
- GitHub Check: base-android-ci / Build
- GitHub Check: base-android-ci / Run static checks
- GitHub Check: compare-sdk-sizes / Compare SDK sizes
- GitHub Check: Build / compose apks
🔇 Additional comments (6)
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/CallService.kt (6)
80-81: Good concurrency design with serialized execution.The use of
limitedParallelism(1)ensures all service-scoped work is serialized, preventing race conditions. The combination withSupervisorJoband the exception handler provides robust error isolation.
178-199: Well-structured intent handling with clear flow.The refactored
onStartCommandfollows a clean pattern: early exits, parameter extraction, and delegation. The error handling properly stops the service on failures.
353-382: Excellent notification ID tracking for service reuse.The notification ID resolution strategy (using
call.state.notificationIdas the primary source with fallbacks) directly supports the PR goal of reusing the same foreground service when transitioning from ringing to active states.
500-538: Well-designed observer architecture with proper lifecycle management.The observer setup is clean and modular. The conditional registration of
CallServiceNotificationUpdateObserverbased onenableCallNotificationUpdatesprevents unnecessary overhead when the feature is disabled.
63-93: Excellent modular architecture achieving PR goals.The refactored
CallServicesuccessfully addresses all four PR objectives through:
- Debouncer for notification dismissal fixes
- Observer pattern for ringing-state updates
- Notification ID tracking in
call.statefor service reuse- Separation into focused managers and observers
The concurrency model with serialized
serviceScopeand structured observers provides a solid foundation for reliable lifecycle management.
554-562: The review comment is based on an incorrect assumption about Android service lifecycle. Each timeCallService.onDestroy()is called, the service instance is destroyed and eligible for garbage collection. When the service is restarted, Android creates a newCallServiceinstance with fresh fields.Since
serviceStateis an instance field (line 69:internal val serviceState = ServiceState()), not a static/shared holder, it cannot be reused across service instances. Additionally,startTimeis properly initialized inonCreate()(line 117) for each new service instance. Therefore, explicit cleanup ofstartTimeorcurrentCallIdinonDestroy()is unnecessary—the fields are implicitly reset when the new service instance is created.The existing cleanup in
onDestroy()is appropriate:soundPlayer,debouncer, andserviceScoperequire explicit cancellation/cleanup before destruction, which is correctly handled.Likely an incorrect or invalid review comment.
| private fun stopServiceGracefully(source: String? = null) { | ||
| serviceState.startTime?.let { startTime -> | ||
|
|
||
| val currentTime = OffsetDateTime.now() | ||
| val duration = Duration.between(startTime, currentTime) | ||
| val differenceInSeconds = duration.seconds.absoluteValue | ||
| val debouncerThresholdTimeInSeconds = SERVICE_DESTROY_THRESHOLD_TIME_MS / 1_000 | ||
| logger.d { "[stopServiceGracefully] differenceInSeconds: $differenceInSeconds" } | ||
| if (differenceInSeconds >= debouncerThresholdTimeInSeconds) { | ||
| internalStopServiceGracefully() | ||
| } else { | ||
| debouncer.submit(debouncerThresholdTimeInSeconds) { | ||
| internalStopServiceGracefully() | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Debouncing logic keeps service alive longer than the threshold.
The current implementation debounces for the full SERVICE_DESTROY_THRESHOLD_TIME_MS regardless of how long the service has already been running. This means if the service has been running for 1 second, it will debounce for an additional 2 seconds (total 3 seconds), exceeding the 2-second threshold.
The logic should debounce for the remaining time to reach the threshold:
⏱️ Proposed fix for debounce timing
private fun stopServiceGracefully(source: String? = null) {
serviceState.startTime?.let { startTime ->
-
val currentTime = OffsetDateTime.now()
val duration = Duration.between(startTime, currentTime)
val differenceInSeconds = duration.seconds.absoluteValue
val debouncerThresholdTimeInSeconds = SERVICE_DESTROY_THRESHOLD_TIME_MS / 1_000
logger.d { "[stopServiceGracefully] differenceInSeconds: $differenceInSeconds" }
if (differenceInSeconds >= debouncerThresholdTimeInSeconds) {
internalStopServiceGracefully()
} else {
- debouncer.submit(debouncerThresholdTimeInSeconds) {
+ val remainingSeconds = debouncerThresholdTimeInSeconds - differenceInSeconds
+ debouncer.submit(remainingSeconds) {
internalStopServiceGracefully()
}
}
}
}📝 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 stopServiceGracefully(source: String? = null) { | |
| serviceState.startTime?.let { startTime -> | |
| val currentTime = OffsetDateTime.now() | |
| val duration = Duration.between(startTime, currentTime) | |
| val differenceInSeconds = duration.seconds.absoluteValue | |
| val debouncerThresholdTimeInSeconds = SERVICE_DESTROY_THRESHOLD_TIME_MS / 1_000 | |
| logger.d { "[stopServiceGracefully] differenceInSeconds: $differenceInSeconds" } | |
| if (differenceInSeconds >= debouncerThresholdTimeInSeconds) { | |
| internalStopServiceGracefully() | |
| } else { | |
| debouncer.submit(debouncerThresholdTimeInSeconds) { | |
| internalStopServiceGracefully() | |
| } | |
| } | |
| } | |
| } | |
| private fun stopServiceGracefully(source: String? = null) { | |
| serviceState.startTime?.let { startTime -> | |
| val currentTime = OffsetDateTime.now() | |
| val duration = Duration.between(startTime, currentTime) | |
| val differenceInSeconds = duration.seconds.absoluteValue | |
| val debouncerThresholdTimeInSeconds = SERVICE_DESTROY_THRESHOLD_TIME_MS / 1_000 | |
| logger.d { "[stopServiceGracefully] differenceInSeconds: $differenceInSeconds" } | |
| if (differenceInSeconds >= debouncerThresholdTimeInSeconds) { | |
| internalStopServiceGracefully() | |
| } else { | |
| val remainingSeconds = debouncerThresholdTimeInSeconds - differenceInSeconds | |
| debouncer.submit(remainingSeconds) { | |
| internalStopServiceGracefully() | |
| } | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In
@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/CallService.kt
around lines 581 - 597, The debouncer currently always waits the full
SERVICE_DESTROY_THRESHOLD_TIME_MS, ignoring elapsed time; update
stopServiceGracefully to compute the remaining wait as (thresholdSeconds -
differenceInSeconds) (clamped to >=0) and pass that remainingSeconds to
debouncer.submit, calling internalStopServiceGracefully immediately if
remainingSeconds == 0; reference serviceState.startTime,
SERVICE_DESTROY_THRESHOLD_TIME_MS, stopServiceGracefully,
internalStopServiceGracefully, and debouncer.submit when making the change.
| /** | ||
| * Temp Fix!! The observeRingingState scope was getting cancelled and as a result, | ||
| * ringing state was not properly updated | ||
| */ | ||
| callSoundAndVibrationPlayer?.stopCallSound() | ||
|
|
||
| // Stop any jobs | ||
| serviceState.soundPlayer?.stopCallSound() // TODO should check which call owns the sound | ||
| serviceScope.cancel() |
There was a problem hiding this comment.
Potential issue with sound management in multi-call scenarios.
The TODO comment and "Temp Fix!!" note indicate that stopCallSound() may stop the sound for the wrong call in multi-call scenarios. This could cause incorrect audio behavior when transitioning between calls.
Consider adding call-specific sound ownership tracking to ensure the correct call's sound is stopped. Would you like me to help design a solution for call-specific sound management?
| internal fun debugPrintLastStackFrames(tag: String, count: Int = 10) { | ||
| val stack = Thread.currentThread().stackTrace | ||
| val message = stack.takeLast(count).joinToString("\n") | ||
| Log.d(tag, message) | ||
| } |
There was a problem hiding this comment.
Logic issue: takeLast(count) retrieves the oldest stack frames, not the most recent.
Stack traces from Thread.currentThread().stackTrace are ordered with the most recent calls at the beginning (indices 0-2 typically include getStackTrace(), this function itself, and the immediate caller) and the oldest calls at the end (e.g., main()). Using takeLast(count) captures the bottom of the stack, which is rarely useful for debugging.
For debugging, you typically want the most recent frames. Consider using drop(2).take(count) to skip the utility method frames and capture the actual call context.
🔧 Proposed fix to capture recent frames instead of oldest
internal fun debugPrintLastStackFrames(tag: String, count: Int = 10) {
val stack = Thread.currentThread().stackTrace
- val message = stack.takeLast(count).joinToString("\n")
+ // Drop first 2 frames (getStackTrace + this function) and take the next N frames
+ val message = stack.drop(2).take(count).joinToString("\n")
Log.d(tag, message)
}🤖 Prompt for AI Agents
In
@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/utils/AndroidUtils.kt
around lines 261 - 265, The debugPrintLastStackFrames function currently uses
Thread.currentThread().stackTrace.takeLast(count) which returns the oldest
frames; change it to capture the most recent call frames by skipping the
internal stack entries and taking the next 'count' entries instead (e.g., use
stack.drop(2).take(count) or equivalent), then join and Log.d as before; update
debugPrintLastStackFrames and ensure you handle cases where the stack has fewer
entries than count.
|
Goal
Implementation
Core Changes
Core classes
-Since we will use same service to transition from ringing to active so we need to store a reference of original notification Id
- Process
LocalCallMissedEventhere so that it will work for calls which are not activeService classes refactoring
Util Classes
Testing
Fix FG Service Notification dismiss from Notification Tray
Service lifecycle
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.