Correctly update Ringing State on CallEnded event#1621
Correctly update Ringing State on CallEnded event#1621rahul-lohra wants to merge 2 commits intodevelopfrom
Conversation
PR checklist ✅All required conditions are satisfied:
🎉 Great job! This PR is ready for review. |
rahul-lohra
changed the title
rahul-lohra
changed the title
WalkthroughRefactoring of the CallState class to consolidate logging through a structured per-module logger (ringingLogger) and augment ringing state logic with call-ended state awareness, enabling proper state transitions when calls end. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/CallState.kt (1)
1261-1262: MoveringingLoggerto a class-level property to avoid recreating the delegate wrapper on everyupdateRingingState()call.The local delegated
val ringingLogger by taggedLogger("RingingState")at line 1261 creates a new delegate wrapper object on each invocation. SinceupdateRingingState()is called frequently (at least 7+ times across state changes: accepted/rejected events, member updates, session changes, etc.), this incurs unnecessary allocation overhead.Follow the existing pattern used for
loggerat line 226:private val logger by taggedLogger("CallState") +private val ringingLogger by taggedLogger("RingingState")Then remove the local declaration at line 1261.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/CallState.kt` around lines 1261 - 1262, Declare a class-level property for the logger instead of recreating it inside updateRingingState(): move the local delegated val ringingLogger by taggedLogger("RingingState") to a top-level property in the class (mirroring the existing logger declaration used at line 226) and then remove the local declaration inside updateRingingState(); update usages in updateRingingState() to reference the new class-level ringingLogger to avoid repeated delegate-wrapper allocations.
🤖 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/CallState.kt`:
- Around line 1280-1282: The isCallEnded branch in updateRingingState()
transitions to RingingState.RejectedByAll but omits call.leave(), causing
resources to leak when updateRingingState() is invoked from other paths
(updateFromResponse, getOrCreateMembers, CallSessionParticipantJoinedEvent);
modify the isCallEnded branch in updateRingingState() to call call.leave(...)
with a clear reason (e.g., "updateRingingState-ended") prior to cancelTimeout(),
mirroring the other rejection branches so call.leave() is always invoked before
returning RingingState.RejectedByAll.
---
Nitpick comments:
In
`@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/CallState.kt`:
- Around line 1261-1262: Declare a class-level property for the logger instead
of recreating it inside updateRingingState(): move the local delegated val
ringingLogger by taggedLogger("RingingState") to a top-level property in the
class (mirroring the existing logger declaration used at line 226) and then
remove the local declaration inside updateRingingState(); update usages in
updateRingingState() to reference the new class-level ringingLogger to avoid
repeated delegate-wrapper allocations.
| } else if (isCallEnded) { | ||
| cancelTimeout() | ||
| RingingState.RejectedByAll |
There was a problem hiding this comment.
isCallEnded branch is missing call.leave(), unlike every other rejection branch
Every other branch that transitions to RejectedByAll in this function also calls call.leave():
} else if (isRejectedByMe) {
call.leave("updateRingingState-rejected-self") // ← always called
cancelTimeout()
RingingState.RejectedByAll
} else if (...rejectedBy...) {
call.leave("updateRingingState-rejected") // ← always called
cancelTimeout()
...
RingingState.RejectedByAllThe new isCallEnded branch does not:
} else if (isCallEnded) {
cancelTimeout()
RingingState.RejectedByAll // no call.leave()This works for the primary path (CallEndedEvent → call.leave() is called explicitly at line 841). However, updateRingingState() is also invoked from updateFromResponse(CallResponse) (line 1540), getOrCreateMembers (line 1502), and CallSessionParticipantJoinedEvent (line 1195). If any of those paths execute with _endedAt != null — for example, a CallSessionEndedEvent or CallUpdatedEvent carrying event.call.endedAt != null in a race condition — the ringing state flips to RejectedByAll but call.leave() is never triggered, leaving call resources unreleased.
🐛 Proposed fix — align with other rejection branches
} else if (isCallEnded) {
+ call.leave("updateRingingState-call-ended")
cancelTimeout()
RingingState.RejectedByAll🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/CallState.kt`
around lines 1280 - 1282, The isCallEnded branch in updateRingingState()
transitions to RingingState.RejectedByAll but omits call.leave(), causing
resources to leak when updateRingingState() is invoked from other paths
(updateFromResponse, getOrCreateMembers, CallSessionParticipantJoinedEvent);
modify the isCallEnded branch in updateRingingState() to call call.leave(...)
with a clear reason (e.g., "updateRingingState-ended") prior to cancelTimeout(),
mirroring the other rejection branches so call.leave() is always invoked before
returning RingingState.RejectedByAll.
There was a problem hiding this comment.
Our scope is limited to correctly updating the RingingState when the call is ended. No additional changes are required.
There was a problem hiding this comment.
@rahul-lohra, understood! I'll respect the scope you've defined for this PR. The fix correctly addresses the ringing state transition when a call ends, which is the stated objective. Thank you for the clarification.
🧠 Learnings used
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:33:02.111Z
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.
SDK Size Comparison 📏
|
|
| val userIsParticipant = | ||
| _session.value?.participants?.find { it.user.id == client.userId } != null | ||
| val outgoingMembersCount = _members.value.filter { it.value.user.id != client.userId }.size | ||
| val isCallEnded: Boolean = _endedAt.value != null |
There was a problem hiding this comment.
taggedLogger is a delegate — creating it inside updateRingingState() means a new instance allocates on every call. This method runs frequently (every state change, every event). Should be class-level: private val ringingLogger by taggedLogger("RingingState")
Same issue flagged on #1617
2 participants
Goal
Correctly update
RingingStateonCallEndedeventBackground: When an incoming call is declined by the caller via
call.end()rather thancall.reject(), the callee currently transitions fromRingingState.IncomingtoRingingState.Idle. This change corrects the transition toRingingState.RejectedByAll.Implementation
Used
call.endedAtto determine if call has been ended🎨 UI Changes
Before
Screen.Recording.2026-02-20.at.6.39.14.PM.mov
After
Screen.Recording.2026-02-20.at.6.34.22.PM.mov
Testing
call.reject()in StreamActivity.cancel() withcall.endto reproduce this behaviour.Summary by CodeRabbit