Ability to reject an incoming call when you aleady have a ringing or active call#1614
Open
rahul-lohra wants to merge 14 commits intodevelopfrom
Open
Ability to reject an incoming call when you aleady have a ringing or active call#1614rahul-lohra wants to merge 14 commits intodevelopfrom
rahul-lohra wants to merge 14 commits intodevelopfrom
Conversation
Contributor
PR checklist ✅All required conditions are satisfied:
🎉 Great job! This PR is ready for review. |
rahul-lohra
changed the title
Contributor
SDK Size Comparison 📏
|
rahul-lohra
changed the title
WalkthroughThis PR introduces a Changes
Sequence DiagramsequenceDiagram
participant User as User (Incoming)
participant Client as StreamVideoClient
participant Policy as EventPropagationPolicy
participant Handler as CallBusyHandler
participant Notif as NotificationHandler
participant State as ClientState
User->>Client: CallRingEvent arrives
Client->>Policy: shouldPropagate(event)
Policy->>Handler: shouldPropagateEvent(event)
Handler->>State: Check rejectCallWhenBusy
Handler->>Handler: Check active/ringing calls
alt User is busy with another call
Handler-->>Policy: false
Policy-->>Client: false
Client->>Handler: isBusyWithAnotherCall(rejectViaApi=true)
Handler->>Client: rejectCall(RejectReason.Busy)
Handler-->>Policy: true (busy)
Client->>Notif: Skip notification (busy)
else User is not busy
Handler-->>Policy: true (not busy)
Policy-->>Client: true
Client->>Client: Propagate event
Client->>Notif: Show incoming call notification
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 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)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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: 5
🧹 Nitpick comments (14)
stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/EventPropagationPolicyTest.kt (1)
28-28: Preferorg.junit.Testfor consistency with the rest of the test suite.This file mixes
org.junit.Before(line 27) withkotlin.test.Test(line 28). Other test files in the module (RingTest.kt,StreamDefaultNotificationHandlerTest.kt, etc.) useorg.junit.Testthroughout. Keeping a consistent import avoids confusion about which runner applies.♻️ Proposed fix
-import kotlin.test.Test +import org.junit.Test🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/EventPropagationPolicyTest.kt` at line 28, The test imports kotlin.test.Test while the suite uses org.junit.Test; update the EventPropagationPolicyTest import by replacing kotlin.test.Test with org.junit.Test (and remove any other kotlin.test.Test usages) so the class EventPropagationPolicyTest and its test methods use org.junit.Test consistently with the other tests (e.g., RingTest, StreamDefaultNotificationHandlerTest).stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/notifications/handlers/StreamDefaultNotificationHandlerTest.kt (2)
210-213: RedundantunmockkObject(StreamVideo)inside the test body — rely on@After tearDown.
@After fun tearDown()at line 184 already callsunmockkAll(), which clears every mock includingStreamVideo. CallingunmockkObject(StreamVideo)mid-test is redundant and misleading — the parallelonRingingCall shows... not busytest doesn't do this, yet both are cleaned up the same way. Remove the redundant call for consistency.♻️ Proposed fix
// Then — NOTHING downstream should execute verify(exactly = 0) { mockIntentResolver.searchIncomingCallPendingIntent(any(), any()) } - unmockkObject(StreamVideo) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/notifications/handlers/StreamDefaultNotificationHandlerTest.kt` around lines 210 - 213, Remove the redundant mid-test unmockkObject(StreamVideo) call in StreamDefaultNotificationHandlerTest: the verify block that asserts mockIntentResolver.searchIncomingCallPendingIntent(...) is called exactly 0 times should remain, but delete the trailing unmockkObject(StreamVideo) line because `@After` tearDown() already calls unmockkAll(), so this explicit unmockkObject(StreamVideo) is unnecessary and inconsistent with the other test.
107-107: RenamesetUp2tosetUp— the2suffix is confusing.There is no other
@Beforemethod in this class, so the suffix implies a companion that doesn't exist. The@After/@Beforeconventions expectsetUp/tearDown.♻️ Proposed fix
- fun setUp2() { + fun setUp() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/notifications/handlers/StreamDefaultNotificationHandlerTest.kt` at line 107, Rename the test fixture method setUp2 to setUp in the StreamDefaultNotificationHandlerTest class and keep its `@Before` annotation so it follows standard lifecycle naming; update any internal references or test utilities that call setUp2 to call setUp instead, and ensure there are no duplicate `@Before` methods after the rename to avoid lifecycle conflicts.stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/handlers/StreamDefaultNotificationHandler.kt (2)
154-157:shouldShowIncomingCallNotificationshould beprivate, notinternal.This helper is only called from
onRingingCallwithin the same class. Declaring itinternalunnecessarily widens the module-visible API surface of apublic open class, since subclasses and the entire module can now see and override the decision logic. The tests exercise it indirectly viaonRingingCall(by mockingcallBusyHandler), so no test needsinternalaccess.♻️ Proposed fix
- internal fun shouldShowIncomingCallNotification( + private fun shouldShowIncomingCallNotification( callBusyHandler: CallBusyHandler, callCid: String, ) = !callBusyHandler.isBusyWithAnotherCall(callCid)🤖 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/handlers/StreamDefaultNotificationHandler.kt` around lines 154 - 157, Change the visibility of the helper function shouldShowIncomingCallNotification in StreamDefaultNotificationHandler from internal to private: locate the function signature "internal fun shouldShowIncomingCallNotification(callBusyHandler: CallBusyHandler, callCid: String)" and make it private so it is only callable within the class (ensure no external references rely on it; it's used by onRingingCall). Keep the rest of the method body unchanged.
167-173: Push-notification path suppresses the UI but never sends the busy-rejection API call.
shouldShowIncomingCallNotificationcallsisBusyWithAnotherCall(callCid)with the defaultrejectViaApi = false. This means when the SDK receives a push notification while the socket is disconnected (typical background scenario), the incoming-call notification is correctly suppressed, but the caller is never sent aRejectReason.Busyresponse — they continue ringing until timeout.The rejection API is only fired from the WebSocket event path (
EventPropagationPolicy→CallBusyHandler.shouldPropagateEvent→isBusyWithAnotherCall(cid, rejectViaApi = true)). If the socket is not connected when the push arrives, that path is never reached.Consider calling
isBusyWithAnotherCall(callId.cid, rejectViaApi = true)here as well, or triggering the rejection through a separate helper, so the caller receives timely feedback even in background/push-only scenarios.🤖 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/handlers/StreamDefaultNotificationHandler.kt` around lines 167 - 173, When suppressing the incoming-call notification in StreamDefaultNotificationHandler (inside the shouldShowIncomingCallNotification check), ensure you also trigger the busy-rejection API by calling isBusyWithAnotherCall(callId.cid, rejectViaApi = true) (or invoking the existing helper that sends RejectReason.Busy) so the caller receives a timely Reject response when the SDK is backgrounded and the socket is disconnected; update the branch that returns early after shouldShowIncomingCallNotification to call that method before returning.stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/CallBusyHandler.kt (1)
34-34: Usesplit(":", limit = 2)to guard against malformed CIDs.
"foo".split(":")returns a single-element list; the destructuringval (type, id) = ...then throwsIndexOutOfBoundsException. Usinglimit = 2is consistent with defensive coding, costs nothing, and avoids a potential crash if a server ever emits a CID without a colon.♻️ Proposed fix
- val (type, id) = callCid.split(":") + val parts = callCid.split(":", limit = 2) + if (parts.size < 2) return false + val (type, id) = parts🤖 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/call/CallBusyHandler.kt` at line 34, The destructuring in CallBusyHandler that does val (type, id) = callCid.split(":") can throw IndexOutOfBoundsException for malformed CIDs; change the split call to use a limit of 2 (i.e., split(":", limit = 2)) so it returns at most two parts and then handle the case where the second part is missing (e.g., provide a safe fallback or abort early) to avoid crashes when callCid lacks a colon.stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/ClientState.kt (1)
90-93: Redundant(client as StreamVideoClient)casts — usestreamVideoClient.
streamVideoClientat line 72 already holds the result ofclient as StreamVideoClient. Casting again on lines 90 and 92 is noise and repeats a fallible unsafe cast.♻️ Proposed fix
- val rejectCallWhenBusy: Boolean = (client as StreamVideoClient).rejectCallWhenBusy + public val rejectCallWhenBusy: Boolean = streamVideoClient.rejectCallWhenBusy - internal val callBusyHandler = CallBusyHandler(this.client as StreamVideoClient) + internal val callBusyHandler = CallBusyHandler(streamVideoClient) internal val eventPropagationPolicy = EventPropagationPolicy(callBusyHandler)🤖 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/ClientState.kt` around lines 90 - 93, The redundant unsafe casts (client as StreamVideoClient) used when initializing rejectCallWhenBusy, callBusyHandler, and eventPropagationPolicy should be replaced with the already-computed streamVideoClient property (defined earlier at line 72); update the initializers for rejectCallWhenBusy, callBusyHandler, and eventPropagationPolicy to use streamVideoClient instead of casting client again to avoid duplicate/unsafe casts and improve clarity.stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/StreamVideoClientTest.kt (3)
206-215:fireEvent full flowtest only asserts one side effect.The test name claims to verify the "full flow" but only checks that
state.handleEventis invoked. The intermediate steps —notifyClientSubscriptions,forwardToCallSubscriptions, andpropagateEventToCall— are not verified. Consider either narrowing the name to match the single assertion, or adding verification of the additional stages.🧪 Example extended assertions
`@Test` fun `fireEvent full flow executes in order when allowed`() { val event = mockk<VideoEvent>() + val call = mockk<Call>(relaxed = true) + val listener = mockk<VideoEventListener<VideoEvent>>(relaxed = true) + val sub = EventSubscription(listener) + client::class.java.getDeclaredField("subscriptions").apply { + isAccessible = true + set(client, mutableSetOf(sub)) + } every { policy.shouldPropagate(event) } returns true client.fireEvent(event) verify { state.handleEvent(event) } + verify { listener.onEvent(event) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/StreamVideoClientTest.kt` around lines 206 - 215, Test "fireEvent full flow executes in order when allowed" only asserts state.handleEvent; either rename it to reflect that single assertion or extend the test to verify the intermediate steps and ordering. If extending, after calling client.fireEvent(event) add verifications for policy.shouldPropagate(event) (already mocked), notifyClientSubscriptions(event), forwardToCallSubscriptions(event) and propagateEventToCall(event), and assert call order (e.g. using verifySequence or verifyOrder) that policy check happens first then notifyClientSubscriptions, forwardToCallSubscriptions, propagateEventToCall and finally state.handleEvent to match the "full flow" behavior.
41-41: Consider extendingTestBasefor fast unit tests.Per coding guidelines, fast unit tests should extend
TestBaserather than bare JUnit classes, for consistent test infrastructure across the module.Based on learnings: "Use
TestBasefor fast unit tests andIntegrationTestBasefor end-to-end call flows."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/StreamVideoClientTest.kt` at line 41, The test class StreamVideoClientTest is a fast unit test and should extend TestBase instead of a bare JUnit class; update the class declaration for StreamVideoClientTest to inherit from TestBase (and add the required import for TestBase if missing) so it uses the shared fast-test infrastructure and utilities provided by TestBase rather than a bare JUnit class.
148-167: Misleading test name forshouldProcessCallAcceptedEvent.The test name reads "not for outgoing call" but the setup does have an outgoing ringing call (
RingingState.Outgoingon CIDvideo:123). The gate fires because the accepted event targets a different call (video:999). A name like`returns false when accepted event is for a different call while ringing state is outgoing`better describes the scenario.♻️ Proposed rename
- fun `shouldProcessCallAcceptedEvent returns false when accepted event not for outgoing call`() { + fun `shouldProcessCallAcceptedEvent returns false when accepted event targets a different call during outgoing ring`() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/StreamVideoClientTest.kt` around lines 148 - 167, Rename the test function to accurately describe the scenario: it should assert that shouldProcessCallAcceptedEvent returns false when the accepted event targets a different call while there is an outgoing ringing call; update the test method name (currently `shouldProcessCallAcceptedEvent returns false when accepted event not for outgoing call`) to something like `returns false when accepted event is for a different call while ringing state is outgoing` so it clearly reflects the setup using CallAcceptedEvent, ringingCall with RingingState.Outgoing, and the mismatched callCid.stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/call/CallBusyHandlerTest.kt (2)
43-44: Consider extendingTestBasefor fast unit tests.Per coding guidelines, fast unit tests should use
TestBase. UsingTestBasecan provide consistent test infrastructure across the module.Based on learnings: "Use
TestBasefor fast unit tests andIntegrationTestBasefor end-to-end call flows."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/call/CallBusyHandlerTest.kt` around lines 43 - 44, Update the CallBusyHandlerTest class to extend TestBase to adopt the module's fast-unit-test infrastructure: modify the class declaration for CallBusyHandlerTest to inherit from TestBase (ensure the correct TestBase import is added) and move or remove any duplicate setup/teardown that TestBase already provides so tests reuse its shared coroutine dispatchers and helpers.
80-84: Blank line missing beforemockCallhelper, andringingCallbusy-path not tested.Two minor issues:
Style:
mockCallimmediately followstearDown()with no blank line, deviating from the surrounding formatting convention.Coverage gap: All "busy" test cases only exercise the
activeCallpath. There's no test verifying thatisBusyWithAnotherCallreturnstruewhen the user is busy with a ringing call (i.e.,ringingCallis non-null with a different ID), nor a case whererejectCallWhenBusy = truebut bothactiveCallandringingCallare null (should returnfalse).🧪 Example missing test cases
+ `@Test` + fun `returns true when busy with an incoming ringing call`() { + every { clientState.rejectCallWhenBusy } returns true + every { clientState.activeCall } returns MutableStateFlow(null) + every { clientState.ringingCall } returns MutableStateFlow(mockCall("999")) + + val result = handler.isBusyWithAnotherCall("video:123", rejectViaApi = false) + + assertTrue(result) + } + + `@Test` + fun `returns false when rejectCallWhenBusy is enabled but no active or ringing call`() { + every { clientState.rejectCallWhenBusy } returns true + every { clientState.activeCall } returns MutableStateFlow(null) + every { clientState.ringingCall } returns MutableStateFlow(null) + + val result = handler.isBusyWithAnotherCall("video:123") + + assertFalse(result) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/call/CallBusyHandlerTest.kt` around lines 80 - 84, Add a blank line before the mockCall helper to match file formatting and add two unit tests for CallBusyHandler: one that sets ringingCall to a Call with a different id (and activeCall null) and asserts isBusyWithAnotherCall(...) returns true, and another that sets rejectCallWhenBusy = true with both activeCall and ringingCall null and asserts isBusyWithAnotherCall(...) returns false; reference the mockCall helper to create ringingCall and exercise the isBusyWithAnotherCall logic alongside existing activeCall tests and keep tearDown() unchanged.stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/StreamVideoClient.kt (1)
611-613:deliverToDestroyedCallsis a one-liner wrapper with no value.This trivial delegation to
deliverIntentToDestroyedCallsadds indirection without abstraction benefit. SincedeliverIntentToDestroyedCallsis alreadyprivate, either inline the call at thefireEventcall site or remove the wrapper.♻️ Proposed simplification
- internal fun deliverToDestroyedCalls(event: VideoEvent) { - deliverIntentToDestroyedCalls(event) - } - private fun shouldProcessDestroyedCall(event: VideoEvent, callCid: String): Boolean {And in
fireEvent:- deliverToDestroyedCalls(event) + deliverIntentToDestroyedCalls(event)If the wrapper is kept for testability (to allow spying in
StreamVideoClientTest), makedeliverIntentToDestroyedCallsinternalinstead and remove the wrapper.🤖 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/StreamVideoClient.kt` around lines 611 - 613, deliverToDestroyedCalls is a redundant one-line wrapper around private deliverIntentToDestroyedCalls; remove the indirection by deleting deliverToDestroyedCalls and calling deliverIntentToDestroyedCalls directly from fireEvent, or if the wrapper exists only to enable test spying in StreamVideoClientTest, instead make deliverIntentToDestroyedCalls internal and remove deliverToDestroyedCalls so tests can access it; update references (deliverToDestroyedCalls -> deliverIntentToDestroyedCalls) and StreamVideoClientTest accordingly.demo-app/src/main/kotlin/io/getstream/video/android/util/StreamVideoInitHelper.kt (1)
349-349:rejectCallWhenBusy = falseis the parameter's default — explicit pass is redundant.Since
StreamVideoBuilderalready defaultsrejectCallWhenBusytofalse, this line has no behavioral effect. It can be safely removed, or kept as-is for discoverability in the demo app.♻️ Suggested simplification (demo app only)
- rejectCallWhenBusy = false,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@demo-app/src/main/kotlin/io/getstream/video/android/util/StreamVideoInitHelper.kt` at line 349, The call to StreamVideoBuilder sets rejectCallWhenBusy = false which is redundant because StreamVideoBuilder already defaults that parameter to false; remove the explicit rejectCallWhenBusy = false argument from the StreamVideoBuilder invocation in StreamVideoInitHelper to simplify the demo code (or if you prefer discoverability, keep a commented note instead). Locate the builder usage of StreamVideoBuilder in StreamVideoInitHelper and delete the rejectCallWhenBusy = false entry (or replace it with a short commented explanation) so behavior remains unchanged and the code is cleaner.
🤖 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/call/CallBusyHandler.kt`:
- Around line 40-44: The fire-and-forget in CallBusyHandler (the isBusy &&
rejectViaApi branch using streamVideo.scope.launch) swallows exceptions from
streamVideo.call(type, id).reject(RejectReason.Busy); change this to capture and
handle failures by wrapping the reject call in a try/catch (or use runCatching)
inside the coroutine, log the error via the appropriate logger, and surface the
failure upstream (e.g., return a Result or call a failure callback) instead of
silently dropping it; update the code around streamVideo.scope.launch and the
reject invocation to ensure errors are logged and propagated.
In
`@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/ClientState.kt`:
- Line 90: The property declaration for rejectCallWhenBusy in ClientState lacks
an explicit public visibility modifier; update the declaration of val
rejectCallWhenBusy in the ClientState class (the property that backs
getRejectCallWhenBusy()) to include the public visibility modifier so it matches
other client-facing properties and the public API surface.
In
`@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/StreamVideoBuilder.kt`:
- Around line 105-107: In the KDoc for the property rejectCallWhenBusy in
StreamVideoBuilder (the comment block that currently reads "CallRingEvent event
will not be propagated..."), remove the redundant word "event" so it reads
"CallRingEvent will not be propagated..." to avoid the duplicate noun; update
only the documentation text in the StreamVideoBuilder.kt KDoc for
rejectCallWhenBusy.
In
`@stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/stories/RingTest.kt`:
- Around line 115-116: The test function named `Reject a call` has its `@Test`
annotation commented out; restore test visibility by adding an `@Test`
annotation and mark it skipped using `@Ignore("reason")` to match the file's
convention (see other skipped tests in this file). Edit the `fun `Reject a
call`() = runTest { ... }` declaration in `RingTest` to use `@Test` together
with `@Ignore("temporarily disabled" or a short reason)` so the test appears in
reports but is skipped.
In
`@stream-video-android-ui-core/src/main/kotlin/io/getstream/video/android/ui/common/StreamCallActivity.kt`:
- Around line 297-298: The shouldAcceptNewCall implementation in
StreamCallActivity currently returns
StreamVideo.instanceOrNull()?.state?.rejectCallWhenBusy ?: true which inverts
the intended semantics; change it to return the negation of rejectCallWhenBusy
and use a safe false default: use
!(StreamVideo.instanceOrNull()?.state?.rejectCallWhenBusy ?: true) replaced with
!(... ?: true) or more clearly
(StreamVideo.instanceOrNull()?.state?.rejectCallWhenBusy == false) so that when
rejectCallWhenBusy is true we do not accept new calls, when false we accept, and
when null we default to false (do not switch).
---
Nitpick comments:
In
`@demo-app/src/main/kotlin/io/getstream/video/android/util/StreamVideoInitHelper.kt`:
- Line 349: The call to StreamVideoBuilder sets rejectCallWhenBusy = false which
is redundant because StreamVideoBuilder already defaults that parameter to
false; remove the explicit rejectCallWhenBusy = false argument from the
StreamVideoBuilder invocation in StreamVideoInitHelper to simplify the demo code
(or if you prefer discoverability, keep a commented note instead). Locate the
builder usage of StreamVideoBuilder in StreamVideoInitHelper and delete the
rejectCallWhenBusy = false entry (or replace it with a short commented
explanation) so behavior remains unchanged and the code is cleaner.
In
`@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/CallBusyHandler.kt`:
- Line 34: The destructuring in CallBusyHandler that does val (type, id) =
callCid.split(":") can throw IndexOutOfBoundsException for malformed CIDs;
change the split call to use a limit of 2 (i.e., split(":", limit = 2)) so it
returns at most two parts and then handle the case where the second part is
missing (e.g., provide a safe fallback or abort early) to avoid crashes when
callCid lacks a colon.
In
`@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/ClientState.kt`:
- Around line 90-93: The redundant unsafe casts (client as StreamVideoClient)
used when initializing rejectCallWhenBusy, callBusyHandler, and
eventPropagationPolicy should be replaced with the already-computed
streamVideoClient property (defined earlier at line 72); update the initializers
for rejectCallWhenBusy, callBusyHandler, and eventPropagationPolicy to use
streamVideoClient instead of casting client again to avoid duplicate/unsafe
casts and improve clarity.
In
`@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/handlers/StreamDefaultNotificationHandler.kt`:
- Around line 154-157: Change the visibility of the helper function
shouldShowIncomingCallNotification in StreamDefaultNotificationHandler from
internal to private: locate the function signature "internal fun
shouldShowIncomingCallNotification(callBusyHandler: CallBusyHandler, callCid:
String)" and make it private so it is only callable within the class (ensure no
external references rely on it; it's used by onRingingCall). Keep the rest of
the method body unchanged.
- Around line 167-173: When suppressing the incoming-call notification in
StreamDefaultNotificationHandler (inside the shouldShowIncomingCallNotification
check), ensure you also trigger the busy-rejection API by calling
isBusyWithAnotherCall(callId.cid, rejectViaApi = true) (or invoking the existing
helper that sends RejectReason.Busy) so the caller receives a timely Reject
response when the SDK is backgrounded and the socket is disconnected; update the
branch that returns early after shouldShowIncomingCallNotification to call that
method before returning.
In
`@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/StreamVideoClient.kt`:
- Around line 611-613: deliverToDestroyedCalls is a redundant one-line wrapper
around private deliverIntentToDestroyedCalls; remove the indirection by deleting
deliverToDestroyedCalls and calling deliverIntentToDestroyedCalls directly from
fireEvent, or if the wrapper exists only to enable test spying in
StreamVideoClientTest, instead make deliverIntentToDestroyedCalls internal and
remove deliverToDestroyedCalls so tests can access it; update references
(deliverToDestroyedCalls -> deliverIntentToDestroyedCalls) and
StreamVideoClientTest accordingly.
In
`@stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/call/CallBusyHandlerTest.kt`:
- Around line 43-44: Update the CallBusyHandlerTest class to extend TestBase to
adopt the module's fast-unit-test infrastructure: modify the class declaration
for CallBusyHandlerTest to inherit from TestBase (ensure the correct TestBase
import is added) and move or remove any duplicate setup/teardown that TestBase
already provides so tests reuse its shared coroutine dispatchers and helpers.
- Around line 80-84: Add a blank line before the mockCall helper to match file
formatting and add two unit tests for CallBusyHandler: one that sets ringingCall
to a Call with a different id (and activeCall null) and asserts
isBusyWithAnotherCall(...) returns true, and another that sets
rejectCallWhenBusy = true with both activeCall and ringingCall null and asserts
isBusyWithAnotherCall(...) returns false; reference the mockCall helper to
create ringingCall and exercise the isBusyWithAnotherCall logic alongside
existing activeCall tests and keep tearDown() unchanged.
In
`@stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/EventPropagationPolicyTest.kt`:
- Line 28: The test imports kotlin.test.Test while the suite uses
org.junit.Test; update the EventPropagationPolicyTest import by replacing
kotlin.test.Test with org.junit.Test (and remove any other kotlin.test.Test
usages) so the class EventPropagationPolicyTest and its test methods use
org.junit.Test consistently with the other tests (e.g., RingTest,
StreamDefaultNotificationHandlerTest).
In
`@stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/notifications/handlers/StreamDefaultNotificationHandlerTest.kt`:
- Around line 210-213: Remove the redundant mid-test unmockkObject(StreamVideo)
call in StreamDefaultNotificationHandlerTest: the verify block that asserts
mockIntentResolver.searchIncomingCallPendingIntent(...) is called exactly 0
times should remain, but delete the trailing unmockkObject(StreamVideo) line
because `@After` tearDown() already calls unmockkAll(), so this explicit
unmockkObject(StreamVideo) is unnecessary and inconsistent with the other test.
- Line 107: Rename the test fixture method setUp2 to setUp in the
StreamDefaultNotificationHandlerTest class and keep its `@Before` annotation so it
follows standard lifecycle naming; update any internal references or test
utilities that call setUp2 to call setUp instead, and ensure there are no
duplicate `@Before` methods after the rename to avoid lifecycle conflicts.
In
`@stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/StreamVideoClientTest.kt`:
- Around line 206-215: Test "fireEvent full flow executes in order when allowed"
only asserts state.handleEvent; either rename it to reflect that single
assertion or extend the test to verify the intermediate steps and ordering. If
extending, after calling client.fireEvent(event) add verifications for
policy.shouldPropagate(event) (already mocked),
notifyClientSubscriptions(event), forwardToCallSubscriptions(event) and
propagateEventToCall(event), and assert call order (e.g. using verifySequence or
verifyOrder) that policy check happens first then notifyClientSubscriptions,
forwardToCallSubscriptions, propagateEventToCall and finally state.handleEvent
to match the "full flow" behavior.
- Line 41: The test class StreamVideoClientTest is a fast unit test and should
extend TestBase instead of a bare JUnit class; update the class declaration for
StreamVideoClientTest to inherit from TestBase (and add the required import for
TestBase if missing) so it uses the shared fast-test infrastructure and
utilities provided by TestBase rather than a bare JUnit class.
- Around line 148-167: Rename the test function to accurately describe the
scenario: it should assert that shouldProcessCallAcceptedEvent returns false
when the accepted event targets a different call while there is an outgoing
ringing call; update the test method name (currently
`shouldProcessCallAcceptedEvent returns false when accepted event not for
outgoing call`) to something like `returns false when accepted event is for a
different call while ringing state is outgoing` so it clearly reflects the setup
using CallAcceptedEvent, ringingCall with RingingState.Outgoing, and the
mismatched callCid.
Comment on lines
+40
to
+44
| if (isBusy && rejectViaApi) { | ||
| streamVideo.scope.launch { | ||
| streamVideo.call(type, id).reject(RejectReason.Busy) | ||
| } | ||
| } |
There was a problem hiding this comment.
Silent rejection failure leaves the remote caller ringing indefinitely.
The fire-and-forget launch swallows any exception from reject(RejectReason.Busy). If the API call fails (network error, server error, etc.), the caller never receives a busy signal. At minimum, the failure should be logged; ideally it is also surfaced upstream.
🛡️ Proposed fix
if (isBusy && rejectViaApi) {
streamVideo.scope.launch {
- streamVideo.call(type, id).reject(RejectReason.Busy)
+ streamVideo.call(type, id).reject(RejectReason.Busy)
+ .onError { error ->
+ // Log so operators can observe rejection failures
+ streamLog(priority = StreamLog.WARN) {
+ "[CallBusyHandler] Failed to reject busy call $callCid: ${error.message}"
+ }
+ }
}
}📝 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.
Suggested change
| if (isBusy && rejectViaApi) { | |
| streamVideo.scope.launch { | |
| streamVideo.call(type, id).reject(RejectReason.Busy) | |
| } | |
| } | |
| if (isBusy && rejectViaApi) { | |
| streamVideo.scope.launch { | |
| streamVideo.call(type, id).reject(RejectReason.Busy) | |
| .onError { error -> | |
| // Log so operators can observe rejection failures | |
| streamLog(priority = StreamLog.WARN) { | |
| "[CallBusyHandler] Failed to reject busy call $callCid: ${error.message}" | |
| } | |
| } | |
| } | |
| } |
🤖 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/call/CallBusyHandler.kt`
around lines 40 - 44, The fire-and-forget in CallBusyHandler (the isBusy &&
rejectViaApi branch using streamVideo.scope.launch) swallows exceptions from
streamVideo.call(type, id).reject(RejectReason.Busy); change this to capture and
handle failures by wrapping the reject call in a try/catch (or use runCatching)
inside the coroutine, log the error via the appropriate logger, and surface the
failure upstream (e.g., return a Result or call a failure callback) instead of
silently dropping it; update the code around streamVideo.scope.launch and the
reject invocation to ensure errors are logged and propagated.
There was a problem hiding this comment.
@rahul-lohra, understood! I'll skip this suggestion for error handling in the busy call rejection flow.
/)_/)
(o.o)
/>📋 Noted!
✏️ Learnings added
Learnt from: rahul-lohra
Repo: GetStream/stream-video-android PR: 1614
File: stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/CallBusyHandler.kt:40-44
Timestamp: 2026-02-18T12:32:54.490Z
Learning: In the `CallBusyHandler` class in `stream-video-android-core`, fire-and-forget rejection calls (using `streamVideo.scope.launch` to call `reject(RejectReason.Busy)`) without explicit error handling are acceptable for the busy call rejection feature.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
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.
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/ClientState.kt
Outdated
Show resolved
Hide resolved
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/StreamVideoBuilder.kt
Outdated
Show resolved
Hide resolved
stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/stories/RingTest.kt
Outdated
Show resolved
Hide resolved
...o-android-ui-core/src/main/kotlin/io/getstream/video/android/ui/common/StreamCallActivity.kt
Outdated
Show resolved
Hide resolved
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Goal
Ability to reject an incoming call when you already have a ringing or active call
Implementation
Introduce a new parameter
rejectCallWhenBusyinStreamVideoBuilderwhich can used as followingSetting
rejectCallWhenBusyto true will do the followingsCallRingEventwill not be propagated if there is an active or ongoing ringing call.🎨 UI Changes
None
Testing
StreamVideoBuilder.rejectWhenBusyto trueSummary by CodeRabbit
New Features
Tests