Improve ringing state transition to Active when WS is not connected#1617
Open
rahul-lohra wants to merge 5 commits intodevelopfrom
Open
Improve ringing state transition to Active when WS is not connected#1617rahul-lohra wants to merge 5 commits intodevelopfrom
rahul-lohra wants to merge 5 commits intodevelopfrom
Conversation
…ctive when we perform joinAndRing and WS is not connected To set WS connection to null set the return of waitForConnectionId to null
Contributor
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
rahul-lohra
changed the title
rahul-lohra
changed the title
Contributor
SDK Size Comparison 📏
|
# Conflicts: # stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/CallState.kt
…issues' into bugfix/rahullohra/join-and-ring-issues # Conflicts: # stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/CallState.kt
rahul-lohra
changed the title
|
WalkthroughThis PR customizes the Checkbox component in DirectCallJoinScreen with styling and offset, and refactors CallState.kt to use AtomicBoolean for thread-safe ringing state management, adds structured logging, and introduces new logic for handling join-and-ring scenarios. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/CallState.kt (1)
1026-1030:⚠️ Potential issue | 🟡 MinorChange
acceptedByCallee = truetoacceptedByCallee = falseat lines 1026–1030.At the time
JoinCallResponseEventis processed in thejoinAndRingflow, the callee has not yet accepted—settingacceptedByCallee = trueis semantically inaccurate. WhileactiveCallis set synchronously early in the join process (line 668 in_join), the flag's semantic meaning should reflect reality: the callee acceptance comes later viaCallAcceptedEvent. The transition toActivefor thejoinAndRingscenario is handled independently at line 1283 inupdateRingingStateand does not depend on this flag.🤖 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 1026 - 1030, The current branch sets _ringingState.value = RingingState.Outgoing(acceptedByCallee = true) when ringingStateUpdatesStopped is true; change that boolean to false because at the time JoinCallResponseEvent is handled in the joinAndRing flow the callee has not accepted yet — leave acceptedByCallee = false so the semantic matches the real acceptance event (CallAcceptedEvent) which is processed later in _join/CallAcceptedEvent handling and updateRingingState transitions to Active independently.
🧹 Nitpick comments (5)
demo-app/src/main/kotlin/io/getstream/video/android/ui/outgoing/DirectCallJoinScreen.kt (2)
177-179: Prefer using the lambda parameter instead of manually inverting state.
onCheckedChangeprovides the new checked value asit; ignoring it in favour of!callerJoinsFirstis non-idiomatic.♻️ Proposed fix
- onCheckedChange = { - callerJoinsFirst = !callerJoinsFirst - }, + onCheckedChange = { callerJoinsFirst = it },🤖 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/ui/outgoing/DirectCallJoinScreen.kt` around lines 177 - 179, The onCheckedChange handler currently ignores the provided new value and flips callerJoinsFirst manually; update the handler to use the lambda parameter directly (set callerJoinsFirst to the provided boolean) by replacing the body of the onCheckedChange lambda so it assigns the incoming parameter to callerJoinsFirst (referencing the onCheckedChange callback and the callerJoinsFirst state variable).
207-207: Remove commented-out debug code.This leftover comment adds noise with no clear intent to re-enable it.
♻️ Proposed fix
-// StreamCallId("default", UUID.randomUUID().toString()),🤖 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/ui/outgoing/DirectCallJoinScreen.kt` at line 207, Remove the leftover commented-out debug line StreamCallId("default", UUID.randomUUID().toString()) from DirectCallJoinScreen.kt; locate the commented line in the DirectCallJoinScreen composable or surrounding setup (where StreamCallId is referenced) and delete the commented code so the file contains only active, meaningful code without commented debug artifacts.stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/CallState.kt (3)
1316-1320: Add a comment explaining whyringingStateUpdatesStoppedis reset here.This reset enables the standard
hasActiveCall && !stopped → Activepath for all subsequentupdateRingingState()calls once the user is confirmed as a session participant. Without a comment, the intent is opaque and the line is easy to accidentally remove during future refactoring.♻️ Proposed fix
} else { // call is accepted and we are already in the call + // Re-enable normal ringing-state transitions: the user is already + // an SFU participant, so joinAndRing's suppression is no longer needed. ringingStateUpdatesStopped.set(false) cancelTimeout() ringingStateLogger.d { "RingingState.Active source 3" } RingingState.Active🤖 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 1316 - 1320, Add a brief inline comment above the ringingStateUpdatesStopped.set(false) call in the updateRingingState / RingingState.Active branch explaining that we reset this flag because once the call is accepted and the user is confirmed as a session participant we must re-enable the normal "hasActiveCall && !stopped → Active" transition for all subsequent updateRingingState() invocations; mention that this prevents future refactors from accidentally removing the reset and that cancelTimeout() is called to clear any pending fallback before returning RingingState.Active.
1242-1242: MoveringingStateLoggerto class scope — allocating a new logger delegate on everyupdateRingingState()call is wasteful.
updateRingingState()is invoked on virtually every call event; instantiating a freshtaggedLoggerdelegate each time adds unnecessary allocation overhead. Declare it alongside the existingloggerat class level.♻️ Proposed fix
private val logger by taggedLogger("CallState") +private val ringingStateLogger by taggedLogger("RingingState")private fun updateRingingState(rejectReason: RejectReason? = null) { - val ringingStateLogger by taggedLogger("RingingState") if (ringingState.value == 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` at line 1242, The local declaration val ringingStateLogger by taggedLogger("RingingState") inside updateRingingState() should be moved to the class scope alongside the existing logger to avoid allocating a new delegate on every updateRingingState() call; add a class-level property (e.g., private val ringingStateLogger by taggedLogger("RingingState")) near the existing logger declaration and remove the local ringingStateLogger declaration from updateRingingState(), updating any usages to reference the class-scoped ringingStateLogger.
1317-1320: Inconsistent logger: line 1319 usesloggerwhile the rest ofupdateRingingStateusesringingStateLogger.♻️ Proposed fix
- logger.d { "RingingState.Active source 3" } + ringingStateLogger.d { "RingingState.Active source 3" }🤖 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 1317 - 1320, The logging call in updateRingingState is inconsistent: it uses logger.d in the branch returning RingingState.Active while the rest of the function uses ringingStateLogger; update that call to use ringingStateLogger.d (or the same logging API used elsewhere in updateRingingState) so all log statements are uniform—modify the line that currently calls logger.d { "RingingState.Active source 3" } to ringgingStateLogger.d (matching the existing logging style) while keeping the surrounding logic (ringingStateUpdatesStopped.set(false), cancelTimeout(), returning RingingState.Active) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/CallState.kt`:
- Around line 1026-1030: The current branch sets _ringingState.value =
RingingState.Outgoing(acceptedByCallee = true) when ringingStateUpdatesStopped
is true; change that boolean to false because at the time JoinCallResponseEvent
is handled in the joinAndRing flow the callee has not accepted yet — leave
acceptedByCallee = false so the semantic matches the real acceptance event
(CallAcceptedEvent) which is processed later in _join/CallAcceptedEvent handling
and updateRingingState transitions to Active independently.
---
Nitpick comments:
In
`@demo-app/src/main/kotlin/io/getstream/video/android/ui/outgoing/DirectCallJoinScreen.kt`:
- Around line 177-179: The onCheckedChange handler currently ignores the
provided new value and flips callerJoinsFirst manually; update the handler to
use the lambda parameter directly (set callerJoinsFirst to the provided boolean)
by replacing the body of the onCheckedChange lambda so it assigns the incoming
parameter to callerJoinsFirst (referencing the onCheckedChange callback and the
callerJoinsFirst state variable).
- Line 207: Remove the leftover commented-out debug line StreamCallId("default",
UUID.randomUUID().toString()) from DirectCallJoinScreen.kt; locate the commented
line in the DirectCallJoinScreen composable or surrounding setup (where
StreamCallId is referenced) and delete the commented code so the file contains
only active, meaningful code without commented debug artifacts.
In
`@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/CallState.kt`:
- Around line 1316-1320: Add a brief inline comment above the
ringingStateUpdatesStopped.set(false) call in the updateRingingState /
RingingState.Active branch explaining that we reset this flag because once the
call is accepted and the user is confirmed as a session participant we must
re-enable the normal "hasActiveCall && !stopped → Active" transition for all
subsequent updateRingingState() invocations; mention that this prevents future
refactors from accidentally removing the reset and that cancelTimeout() is
called to clear any pending fallback before returning RingingState.Active.
- Line 1242: The local declaration val ringingStateLogger by
taggedLogger("RingingState") inside updateRingingState() should be moved to the
class scope alongside the existing logger to avoid allocating a new delegate on
every updateRingingState() call; add a class-level property (e.g., private val
ringingStateLogger by taggedLogger("RingingState")) near the existing logger
declaration and remove the local ringingStateLogger declaration from
updateRingingState(), updating any usages to reference the class-scoped
ringingStateLogger.
- Around line 1317-1320: The logging call in updateRingingState is inconsistent:
it uses logger.d in the branch returning RingingState.Active while the rest of
the function uses ringingStateLogger; update that call to use
ringingStateLogger.d (or the same logging API used elsewhere in
updateRingingState) so all log statements are uniform—modify the line that
currently calls logger.d { "RingingState.Active source 3" } to
ringgingStateLogger.d (matching the existing logging style) while keeping the
surrounding logic (ringingStateUpdatesStopped.set(false), cancelTimeout(),
returning RingingState.Active) unchanged.
| } | ||
|
|
||
| private fun updateRingingState(rejectReason: RejectReason? = null) { | ||
| val ringingStateLogger by taggedLogger("RingingState") |
Contributor
There was a problem hiding this comment.
ringingStateLogger is created inside updateRingingState() via delegate — this allocates a new logger on every call. This method fires frequently during call state changes. Should be a class-level property like the existing logger.
| RingingState.Active | ||
| } else if (isRejectedByMe) { | ||
| call.leave("updateRingingState-rejected-self") | ||
| cancelTimeout() |
Contributor
There was a problem hiding this comment.
The // for joinAndRing comment is unclear — doesn't explain why this branch exists alongside the first hasActiveCall check above.
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
Improve Ringing state transition to Active when WS is not connected during joinAndRing
Implementation
If the WebSocket connection is not established during
joinAndRing, the caller remains inRingingState.Outgoing. Once the receiver accepts the call, the caller must transition toRingingState.Activeto reflect the active call state correctly.CallState. ringingStateUpdatesStopped🎨 UI Changes
None
Add relevant videos
Todo
Testing
Steps to reproduce
nullfromprivate suspend fun waitForConnectionId()to simulate the behaviour when WS is not establishedjoinAndRingcheckbox is checkedBefore
RingingState.Outgoing. There is a race-condition caused byCallState. ringingStateUpdatesStoppedwhich can make it accept and we will not see this issueAfter (on this branch: bugfix/rahullohra/join-and-ring-issues)
RingingState.OutgoingtoRingingState.Activebecause of new logicSummary by CodeRabbit
New Features
Improvements