Improve stats collection and reconnect flow #1587
Conversation
PR checklist ✅All required conditions are satisfied:
🎉 Great job! This PR is ready for review. |
aleksandar-apostolov
added
the
pr:improvement
WalkthroughReconnection and leave APIs now require/rely on explicit reason strings; socket whenConnected gains a connectionFailed callback and is wrapped in try/catch; RtcSession exposes sfuTracer, buffers ICE candidates, propagates unified_session_id, and mediaScope; Subscriber/Factory wired with SfuConnectionModule; stats delta includes all fields on first report. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Call
participant RtcSession
participant SocketConn as SocketConnection
participant SFU
User->>Call: fastReconnect(reason)
Call->>Call: collectStats(oldSessionStats)
Call->>RtcSession: request reconnect(reason, oldSessionStats)
RtcSession->>SocketConn: whenConnected(timeout, connectionFailed, connected)
alt Socket connects
SocketConn->>SFU: establish connection
SFU-->>SocketConn: ok
SocketConn->>RtcSession: connected(connectionId)
RtcSession->>Call: sendCallStats(oldSessionStats)
else Socket errors
SocketConn->>SFU: establish connection
SFU--x SocketConn: error
SocketConn->>RtcSession: connectionFailed(error)
RtcSession->>Call: handle reconnect failure(reason, error)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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: 6
🧹 Nitpick comments (2)
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/CallService.kt (1)
846-849: Consider a more descriptive reason string.The else branch handles the "ongoing call" scenario per the comment on line 847, but the reason string says
"unknown". A more specific reason like"call-service-end-call-ongoing"would improve traceability.🔎 Proposed fix
} else { // If I'm in an ongoing call, end the call for me - call.leave("call-service-end-call-unknown") + call.leave("call-service-end-call-ongoing") logger.i { "[onTaskRemoved] Ended ongoing call for me." } }stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/connection/Subscriber.kt (1)
344-348: Consider removing or completing the TODO block.This commented-out code introduces dead code into the codebase. If this is planned work, consider either completing it or tracking it in an issue and removing the commented code.
Would you like me to open an issue to track this socket connectivity enhancement?
📜 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 (17)
stream-video-android-core/api/stream-video-android-core.api(6 hunks)stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/Call.kt(14 hunks)stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/CallState.kt(5 hunks)stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/ExternalCallRejectionHandler.kt(1 hunks)stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/StreamVideoClient.kt(1 hunks)stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/RtcSession.kt(30 hunks)stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/connection/StreamPeerConnection.kt(1 hunks)stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/connection/StreamPeerConnectionFactory.kt(2 hunks)stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/connection/Subscriber.kt(4 hunks)stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/connection/stats/StatsTracer.kt(1 hunks)stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/receivers/LeaveCallBroadcastReceiver.kt(1 hunks)stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/CallService.kt(1 hunks)stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/socket/common/SocketActions.kt(1 hunks)stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/socket/coordinator/CoordinatorSocketConnection.kt(1 hunks)stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/socket/sfu/SfuSocketConnection.kt(1 hunks)stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/utils/SerialProcessor.kt(1 hunks)stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/call/connection/SubscriberTest.kt(3 hunks)
🧰 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/main/kotlin/io/getstream/video/android/core/socket/sfu/SfuSocketConnection.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/socket/common/SocketActions.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/utils/SerialProcessor.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/connection/StreamPeerConnectionFactory.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/CallState.ktstream-video-android-core/src/test/kotlin/io/getstream/video/android/core/call/connection/SubscriberTest.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/connection/Subscriber.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/socket/coordinator/CoordinatorSocketConnection.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/ExternalCallRejectionHandler.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/StreamVideoClient.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/connection/StreamPeerConnection.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/connection/stats/StatsTracer.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/RtcSession.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/Call.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/socket/sfu/SfuSocketConnection.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/socket/common/SocketActions.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/utils/SerialProcessor.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/connection/StreamPeerConnectionFactory.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/CallState.ktstream-video-android-core/src/test/kotlin/io/getstream/video/android/core/call/connection/SubscriberTest.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/connection/Subscriber.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/socket/coordinator/CoordinatorSocketConnection.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/ExternalCallRejectionHandler.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/StreamVideoClient.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/connection/StreamPeerConnection.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/connection/stats/StatsTracer.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/RtcSession.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/Call.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/socket/sfu/SfuSocketConnection.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/socket/common/SocketActions.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/utils/SerialProcessor.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/connection/StreamPeerConnectionFactory.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/CallState.ktstream-video-android-core/src/test/kotlin/io/getstream/video/android/core/call/connection/SubscriberTest.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/connection/Subscriber.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/socket/coordinator/CoordinatorSocketConnection.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/ExternalCallRejectionHandler.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/StreamVideoClient.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/connection/StreamPeerConnection.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/connection/stats/StatsTracer.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/RtcSession.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/Call.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/call/connection/SubscriberTest.kt
🧠 Learnings (6)
📚 Learning: 2025-12-19T09:15:37.256Z
Learnt from: CR
Repo: GetStream/stream-video-android PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-19T09:15:37.256Z
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/main/kotlin/io/getstream/video/android/core/socket/sfu/SfuSocketConnection.kt
📚 Learning: 2025-12-19T09:15:37.256Z
Learnt from: CR
Repo: GetStream/stream-video-android PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-19T09:15:37.256Z
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/call/RtcSession.kt
📚 Learning: 2025-12-19T09:15:37.256Z
Learnt from: CR
Repo: GetStream/stream-video-android PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-19T09:15:37.256Z
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/call/RtcSession.kt
📚 Learning: 2025-12-19T09:15:37.256Z
Learnt from: CR
Repo: GetStream/stream-video-android PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-19T09:15:37.256Z
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/call/RtcSession.kt
📚 Learning: 2025-12-19T09:15:37.256Z
Learnt from: CR
Repo: GetStream/stream-video-android PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-19T09:15:37.256Z
Learning: Prefer Jetpack Compose for UI (`stream-video-android-ui-compose`); XML views supported via `stream-video-android-ui-xml`
Applied to files:
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/RtcSession.kt
📚 Learning: 2025-12-19T09:15:37.256Z
Learnt from: CR
Repo: GetStream/stream-video-android PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-19T09:15:37.256Z
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/call/RtcSession.kt
🧬 Code graph analysis (2)
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/RtcSession.kt (3)
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/utils/AndroidUtils.kt (1)
safeCall(129-139)stream-video-android-core/src/main/proto/video/sfu/event/events.pb.go (6)
ICETrickle(682-689)ICETrickle(704-704)ICETrickle(719-721)JoinRequest(1180-1216)JoinRequest(1231-1231)JoinRequest(1246-1248)stream-video-android-core/src/main/proto/video/sfu/models/models.pb.go (3)
ICETrickle(1750-1758)ICETrickle(1773-1773)ICETrickle(1788-1790)
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/Call.kt (4)
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/RtcSession.kt (2)
fastReconnect(1766-1819)cleanup(953-1000)stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/utils/AndroidUtils.kt (1)
safeCall(129-139)stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/StreamVideoClient.kt (1)
cleanup(219-243)stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/scope/ScopeProviderImpl.kt (1)
cleanup(91-99)
⏰ 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). (6)
- GitHub Check: compare-sdk-sizes / Compare SDK sizes
- GitHub Check: build
- GitHub Check: Spotless check
- GitHub Check: API check
- GitHub Check: Unit Tests
- GitHub Check: Build / compose apks
🔇 Additional comments (34)
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/ExternalCallRejectionHandler.kt (1)
71-71: LGTM: Reason parameter improves traceability.The addition of
"rejected-on-wearable"as a reason parameter to thecall.leave()invocation enhances visibility into call rejection flows triggered from wearable devices, supporting the PR's goal of improving visibility and stats collection. The kebab-case format is consistent with the predominant pattern used across other reason parameters in the codebase (e.g., "accepted-on-another-device", "start-ringing-timeout", "call-service-end-call-incomming").stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/receivers/LeaveCallBroadcastReceiver.kt (1)
40-40: LGTM!Passing a descriptive reason string to
leave()improves traceability and aligns with the PR objective to enhance stats collection and visibility of the disconnect/leave flow.stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/StreamVideoClient.kt (1)
242-242: LGTM!Adding the
"client-cleanup"reason string provides clear context for tracing and stats when the call is left due to client cleanup.stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/connection/StreamPeerConnection.kt (1)
133-150: Good enhancement to connection health detection.Expanding
isFailedOrClosed()to also considerIceConnectionState.FAILEDandIceConnectionState.CLOSEDis a robust improvement. ICE layer failures can occur independently of PeerConnection state transitions, so checking both ensures more accurate failure detection for reconnect logic.stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/connection/stats/StatsTracer.kt (1)
242-244: LGTM!This fix ensures the first stats report includes all fields rather than an empty diff. When
oldStats.isEmpty(), skipping the value comparison correctly captures the complete initial state for delta compression.stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/socket/common/SocketActions.kt (1)
81-85: LGTM!Clean addition of the
connectionFailedcallback with a sensible default empty lambda, maintaining backward compatibility.stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/socket/sfu/SfuSocketConnection.kt (1)
148-164: LGTM with a minor observation.The error handling correctly routes exceptions to the
connectionFailedcallback. However, consider adding a debug log before invokingconnectionFailedto aid troubleshooting connection issues.The 500ms delay at line 157 with the comment "Wait for the connection to settle" seems like a workaround. If this is intentional, consider documenting why this settling period is necessary.
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/CallState.kt (5)
748-748: LGTM!Adding the reason string "accepted-on-another-device" improves traceability for debugging cross-device call scenarios.
776-776: LGTM!Reason string correctly identifies this leave path.
785-790: LGTM!Both
CallEndedEventandCallEndedSfuEventleave paths now have clear reason strings for diagnostics.
1192-1192: LGTM!Reason string clarifies this is a rejection-triggered leave.
1304-1304: LGTM!Timeout-based leave is now clearly identified.
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/socket/coordinator/CoordinatorSocketConnection.kt (1)
209-224: LGTM!The error handling correctly routes exceptions to the
connectionFailedcallback. Implementation is consistent with the interface contract.stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/RtcSession.kt (9)
232-239: LGTM!Visibility change from
privatetointernalforsfuTraceris appropriate to allow access from other components within the module for enhanced tracing capabilities.
544-564: Lifecycle observer registration looks correct.The observer is properly guarded against null and registered on the Main dispatcher. Good use of
safeCallto handle potential exceptions during lifecycle registration.
625-634: Pending ICE trickle events flush logic is correct.The pattern of copying the list, clearing it, then iterating over the copy prevents concurrent modification issues. The synchronized list backing ensures thread-safe access.
1434-1435: LGTM!Using
Collections.synchronizedListforiceTricklePendingEventsensures thread-safe access for the ICE candidate buffering.
1444-1464: ICE candidate buffering logic is well implemented.The check for
SfuSocketState.Connectedbefore sending, and buffering candidates otherwise, prevents race conditions during connection establishment. This aligns well with the PR objective of improving the reconnect flow.
1580-1586: Good defensive pattern for tracer rollback.Capturing tracer slices before serialization and rolling them back on failure (lines 1624-1631) prevents data loss when stats transmission fails.
1082-1086: Empty callbacks intentionally defer handling.The empty callbacks for
rejoinandfastReconnectincreateSubscriberandcreatePublisherare documented with comments. The actual reconnect logic is handled elsewhere based on SFU error events.
1770-1786: Fast reconnect tracing is comprehensive.Good addition of tracing for the fast reconnect flow and join request details. This will significantly improve debugging of reconnection scenarios.
1840-1846: Leave with reason properly cleans up resources.The
mediaScope.cancel()and tracing with the reason string provide good cleanup and observability for session termination.stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/connection/Subscriber.kt (1)
280-286: LGTM! Good error recovery mechanism.Calling
rejoin()whensetRemoteDescriptionfails during subscriber negotiation is a reasonable recovery strategy to re-establish the session. This aligns with the PR objective of improving the reconnect flow.stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/call/connection/SubscriberTest.kt (1)
75-76: LGTM! Test correctly updated to match constructor changes.The test appropriately adds a
RelaxedMockKforSfuConnectionModuleand passes it to theSubscriberconstructor, maintaining test consistency with the production code changes.stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/connection/StreamPeerConnectionFactory.kt (1)
469-481: LGTM! Dependency correctly threaded through factory.The
sfuConnectionModuleparameter is properly added tomakeSubscriberand forwarded to theSubscriberconstructor. This follows good dependency injection patterns.stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/Call.kt (3)
682-694: LGTM! Symmetric ICE monitoring for publisher.The publisher ICE state monitoring mirrors the existing subscriber pattern, providing consistent reconnection behavior for both peer connections. The implementation correctly restarts ICE on
FAILEDorDISCONNECTEDstates.
755-755: LGTM! Reason parameters improve observability.Adding
reasonparameters tofastReconnect,rejoin, andleavemethods with sensible defaults enhances debugging and tracing capabilities, aligning with the PR objective of improving visibility.Also applies to: 784-784, 925-925
1742-1745: LGTM!The Debug helper correctly exposes the reason parameter for testing reconnect scenarios.
stream-video-android-core/api/stream-video-android-core.api (6)
7574-7575: API signature change with default parameter support looks correct.The addition of a
Stringreason parameter tofastReconnectwith the corresponding$defaultsynthetic method maintains backward compatibility for Kotlin consumers. Java consumers will need to update their calls to provide the reason parameter explicitly.
7612-7613: LGTM!The
leavemethod now accepts a reason string with default parameter support, consistent with the other connection lifecycle methods.
7631-7632: LGTM!The
rejoinmethod follows the same pattern asfastReconnectandleave, adding a reason parameter for improved visibility into the reconnect flow.
12984-12985: Interface method updated with error handling callback.The
whenConnectedmethod now accepts a secondFunction2parameter (presumably for error/timeout handling based on the AI summary mentioning "fallback/alternative path"). The$defaultsynthetic method allows existing Kotlin callers to omit this parameter.
13163-13163: LGTM!Implementation correctly matches the updated
SocketActionsinterface signature.
13373-13373: LGTM!Implementation correctly matches the updated
SocketActionsinterface signature, ensuring consistency across all socket connection types.Note on API compatibility: These changes add new required parameters to public API methods (
fastReconnect,leave,rejoin,whenConnected). While Kotlin consumers benefit from default parameter support via the$defaultsynthetic methods, Java consumers will experience a breaking change and must update their calls to provide the new parameters. Ensure this is documented in release notes/changelog if this library is consumed externally.
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/Call.kt
Show resolved
Hide resolved
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/Call.kt
Show resolved
Hide resolved
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/RtcSession.kt
Show resolved
Hide resolved
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/RtcSession.kt
Show resolved
Hide resolved
...rc/main/kotlin/io/getstream/video/android/core/notifications/internal/service/CallService.kt
Outdated
Show resolved
Hide resolved
...-video-android-core/src/main/kotlin/io/getstream/video/android/core/utils/SerialProcessor.kt
Outdated
Show resolved
Hide resolved
SDK Size Comparison 📏
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/RtcSession.kt (1)
1624-1631: Consider safeguarding rollback operations in error handler.If
slice.rollback()throws an exception, it could mask the original send failure. Consider wrapping the rollback loop insafeCallor handling rollback failures gracefully.🔎 Proposed safeguard
} catch (e: Exception) { sfuTracer.trace("send-stats-failed", "${e.message}") - tracerSlices.forEach { slice -> slice.rollback() } + tracerSlices.forEach { slice -> + safeCall { slice.rollback() } + } }
📜 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 (4)
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/RtcSession.kt(30 hunks)stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/CallService.kt(1 hunks)stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/utils/SerialProcessor.kt(2 hunks)stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/rtc/RtcSessionTest2.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/utils/SerialProcessor.kt
- stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/notifications/internal/service/CallService.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/rtc/RtcSessionTest2.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/RtcSession.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/rtc/RtcSessionTest2.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/RtcSession.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/rtc/RtcSessionTest2.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/RtcSession.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/rtc/RtcSessionTest2.kt
🧠 Learnings (7)
📚 Learning: 2025-12-19T09:15:37.256Z
Learnt from: CR
Repo: GetStream/stream-video-android PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-19T09:15:37.256Z
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/rtc/RtcSessionTest2.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/RtcSession.kt
📚 Learning: 2025-12-19T09:15:37.256Z
Learnt from: CR
Repo: GetStream/stream-video-android PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-19T09:15:37.256Z
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/main/kotlin/io/getstream/video/android/core/call/RtcSession.kt
📚 Learning: 2025-12-19T09:15:37.256Z
Learnt from: CR
Repo: GetStream/stream-video-android PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-19T09:15:37.256Z
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/call/RtcSession.kt
📚 Learning: 2025-12-19T09:15:37.256Z
Learnt from: CR
Repo: GetStream/stream-video-android PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-19T09:15:37.256Z
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/call/RtcSession.kt
📚 Learning: 2025-12-19T09:15:37.256Z
Learnt from: CR
Repo: GetStream/stream-video-android PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-19T09:15:37.256Z
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/call/RtcSession.kt
📚 Learning: 2025-12-19T09:15:37.256Z
Learnt from: CR
Repo: GetStream/stream-video-android PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-19T09:15:37.256Z
Learning: Prefer Jetpack Compose for UI (`stream-video-android-ui-compose`); XML views supported via `stream-video-android-ui-xml`
Applied to files:
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/RtcSession.kt
📚 Learning: 2025-12-19T09:15:37.256Z
Learnt from: CR
Repo: GetStream/stream-video-android PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-19T09:15:37.256Z
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/call/RtcSession.kt
🧬 Code graph analysis (1)
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/RtcSession.kt (3)
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/utils/AndroidUtils.kt (1)
safeCall(129-139)stream-video-android-core/src/main/proto/video/sfu/event/events.pb.go (9)
ICETrickle(682-689)ICETrickle(704-704)ICETrickle(719-721)JoinRequest(1180-1216)JoinRequest(1231-1231)JoinRequest(1246-1248)LeaveCallRequest(880-887)LeaveCallRequest(902-902)LeaveCallRequest(917-919)stream-video-android-core/src/main/proto/video/sfu/models/models.pb.go (3)
ICETrickle(1750-1758)ICETrickle(1773-1773)ICETrickle(1788-1790)
⏰ 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). (6)
- GitHub Check: compare-sdk-sizes / Compare SDK sizes
- GitHub Check: Build / compose apks
- GitHub Check: build
- GitHub Check: Spotless check
- GitHub Check: Unit Tests
- GitHub Check: API check
🔇 Additional comments (13)
stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/rtc/RtcSessionTest2.kt (1)
464-473: LGTM! Test updated to match new whenConnected API.The mock correctly reflects the updated
whenConnectedsignature with three parameters (timeout, connectionFailed, connected). The test simulates the success path by invoking the third callback with a connection ID.stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/RtcSession.kt (12)
625-634: LGTM! ICE candidate buffering implemented correctly.The code properly flushes buffered ICE candidates when the WebSocket connection is established. The snapshot-clear-process pattern prevents concurrent modification issues.
781-788: LGTM! Connection failure callback properly integrated.The
connectionFailedcallback correctly traces the failure and sends stats for visibility into connection issues.
985-994: LGTM! Lifecycle observer cleanup properly implemented.The cleanup correctly removes the observer on the main thread and nulls out the reference. The assignment bug from the previous review has been fixed.
1351-1354: LGTM! ICE restart tracing improved with reason string.The addition of a reason parameter to
restartIce()improves visibility into why ICE restarts occur.
1434-1435: LGTM! ICE candidate buffering properly implemented.The
iceTricklePendingEventsbuffer ensures ICE candidates are queued until the WebSocket connection is ready, preventing lost candidates during connection establishment.
1445-1464: LGTM! ICE candidate sending logic properly gated on connection state.The logic correctly buffers ICE candidates when the WebSocket is not connected and sends them immediately when connected. The retry mechanism handles transient failures gracefully.
1793-1798: LGTM! Fast reconnect flow properly enhanced with failure handling.The
connectionFailedcallback ensures stats are collected and traced even when fast reconnect fails, improving visibility into reconnection issues.
249-249: LGTM! Reconnect calls enhanced with explicit reason strings.The addition of reason parameters improves visibility into why reconnections occur. Error messages are included for debugging without apparent PII leakage.
Also applies to: 662-662, 666-666
763-763: LGTM! Unified session ID properly propagated to SFU.The
unified_session_idfield helps the SFU identify users across reconnects, as documented in the proto definition.Also applies to: 1777-1777
1770-1770: LGTM! Enhanced tracing improves visibility into session lifecycle.The additional trace events for fast reconnect, join requests, and leave operations provide better observability without exposing sensitive data.
Also applies to: 1786-1786, 1846-1846
544-564: Observer is properly cleaned up; no race condition exists.The lifecycle observer is explicitly removed in
cleanup()at lines 985–994 vialifecycle.removeObserver(), wrapped in the same coroutineScope and Main dispatcher as the setup. Both setup and cleanup are tied tosupervisorJob, so whencleanup()callssupervisorJob.cancel()(line 997), any pending setup coroutine is cancelled before the observer is added. If setup completes first, cleanup safely removes it. No leak or race condition occurs.Likely an incorrect or invalid review comment.
1080-1086: Empty reconnect callbacks are properly delegated to higher-level handlers.The
rejoinandfastReconnectcallbacks in the Subscriber are intentionally empty—reconnection logic is centralized at the RtcSession and Call levels. TheconnectionFailedcallback insfuConnectionModule.socketConnection.whenConnected()handles socket failures by tracing and collecting stats, while successful connections validate peer state and delegate tocall.rejoin()when needed. All reconnection paths flow throughCall.fastReconnect()andCall.rejoin(), ensuring consistent handling.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/connection/Subscriber.kt (1)
283-286: Consider passing an explicit reason to rejoin().The rejoin call on negotiation failure is appropriate for recovery. However, passing an explicit reason (e.g.,
rejoin("negotiate-setRemoteDescription-failed")) would improve observability and tracing, helping diagnose reconnect triggers in production.🔎 Proposed enhancement
.onErrorSuspend { tracer.trace("negotiate-error-setremotedescription", it.message ?: "unknown") - rejoin() + rejoin("negotiate-setRemoteDescription-failed") }stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/RtcSession.kt (2)
625-634: Consider synchronizing the drain operation.The pending ICE trickle event drain creates a race condition window between
toList()(line 629) andclear()(line 630). If a new candidate arrives on another thread during this window, it could be lost.🔎 Proposed fix
is SfuSocketState.Connected -> { call.state._connection.value = RealtimeConnection.Connected - val pendingTrickleEvents = iceTricklePendingEvents.toList() - iceTricklePendingEvents.clear() - pendingTrickleEvents.forEach { - sendIceCandidate(it.first, it.second) - } + synchronized(iceTricklePendingEvents) { + val pendingTrickleEvents = iceTricklePendingEvents.toList() + iceTricklePendingEvents.clear() + pendingTrickleEvents.forEach { + sendIceCandidate(it.first, it.second) + } + } }
814-814: Consider tying mediaScope to the supervisor job.The
mediaScopecorrectly isolates media-related coroutines. However, it's currently independent of thesupervisorJob, which means media coroutines might outlive the RtcSession ifcleanup()is called withoutleaveWithReason().Alternative approach
- internal val mediaScope = CoroutineScope(Dispatchers.Default) + internal val mediaScope = CoroutineScope(Dispatchers.Default + supervisorJob)This would ensure
mediaScopeis automatically cancelled whensupervisorJob.cancel()is called incleanup()(line 997), providing more robust cleanup semantics while still preserving the ability to explicitly cancel vialeaveWithReason().Note: Based on past review comments, the current design intentionally keeps
mediaScopealive during rejoin. This suggestion offers an alternative that ties lifecycle more explicitly to the supervisor job.Based on learnings, ...
📜 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/call/RtcSession.kt(30 hunks)stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/connection/Subscriber.kt(3 hunks)
🧰 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/call/connection/Subscriber.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/RtcSession.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/call/connection/Subscriber.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/RtcSession.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/call/connection/Subscriber.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/RtcSession.kt
🧠 Learnings (7)
📚 Learning: 2025-12-19T09:15:37.256Z
Learnt from: CR
Repo: GetStream/stream-video-android PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-19T09:15:37.256Z
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/main/kotlin/io/getstream/video/android/core/call/RtcSession.kt
📚 Learning: 2025-12-19T09:15:37.256Z
Learnt from: CR
Repo: GetStream/stream-video-android PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-19T09:15:37.256Z
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/call/RtcSession.kt
📚 Learning: 2025-12-19T09:15:37.256Z
Learnt from: CR
Repo: GetStream/stream-video-android PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-19T09:15:37.256Z
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/call/RtcSession.kt
📚 Learning: 2025-12-19T09:15:37.256Z
Learnt from: CR
Repo: GetStream/stream-video-android PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-19T09:15:37.256Z
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/call/RtcSession.kt
📚 Learning: 2025-12-19T09:15:37.256Z
Learnt from: CR
Repo: GetStream/stream-video-android PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-19T09:15:37.256Z
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/main/kotlin/io/getstream/video/android/core/call/RtcSession.kt
📚 Learning: 2025-12-19T09:15:37.256Z
Learnt from: CR
Repo: GetStream/stream-video-android PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-19T09:15:37.256Z
Learning: Prefer Jetpack Compose for UI (`stream-video-android-ui-compose`); XML views supported via `stream-video-android-ui-xml`
Applied to files:
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/RtcSession.kt
📚 Learning: 2025-12-19T09:15:37.256Z
Learnt from: CR
Repo: GetStream/stream-video-android PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-19T09:15:37.256Z
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/call/RtcSession.kt
🧬 Code graph analysis (2)
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/connection/Subscriber.kt (1)
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/Call.kt (2)
rejoin(784-846)rejoin(1722-1726)
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/RtcSession.kt (3)
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/utils/AndroidUtils.kt (1)
safeCall(129-139)stream-video-android-core/src/main/proto/video/sfu/event/events.pb.go (6)
ICETrickle(682-689)ICETrickle(704-704)ICETrickle(719-721)JoinRequest(1180-1216)JoinRequest(1231-1231)JoinRequest(1246-1248)stream-video-android-core/src/main/proto/video/sfu/models/models.pb.go (3)
ICETrickle(1750-1758)ICETrickle(1773-1773)ICETrickle(1788-1790)
⏰ 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). (6)
- GitHub Check: compare-sdk-sizes / Compare SDK sizes
- GitHub Check: Build / compose apks
- GitHub Check: Unit Tests
- GitHub Check: API check
- GitHub Check: Spotless check
- GitHub Check: build
🔇 Additional comments (16)
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/connection/Subscriber.kt (2)
30-30: LGTM!The import addition correctly supports the new
sfuConnectionModuleconstructor parameter, enabling SFU connection module integration into the Subscriber.
80-80: LGTM!The
sfuConnectionModuleparameter correctly integrates the SFU connection module into Subscriber's constructor, supporting enhanced signaling and reconnect flows.stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/RtcSession.kt (14)
29-33: LGTM!The lifecycle-related imports and coroutine utilities correctly support the new lifecycle observer integration and mediaScope management introduced in this PR.
Also applies to: 88-88, 102-102, 107-107, 120-120
232-239: LGTM!The visibility change from
privatetointernalappropriately exposes thesfuTracerfor use across reconnect and stats collection flows, improving tracing visibility as intended by the PR.
249-249: LGTM!Adding explicit reason strings to
fastReconnect()andrejoin()calls significantly improves observability of reconnect triggers, making it easier to diagnose connection issues in production logs.Also applies to: 662-662, 666-666
511-512: LGTM!The lifecycle observer correctly traces foreground/background transitions on the Main thread. The implementation properly uses
withContext(Dispatchers.Main)for lifecycle operations andsafeCallfor exception handling.Minor observation: The null check at line 546 is redundant since
lifecycleObserveris always null during initialization, but this doesn't affect correctness.Also applies to: 544-564
1444-1465: LGTM!The rewritten
sendIceCandidateproperly handles connection state by buffering ICE candidates when the SFU socket is not connected and sending them immediately when connected. The retry logic (3 attempts) provides appropriate resilience.
763-763: LGTM!Including
unified_session_idin JoinRequest constructions correctly propagates session context to the SFU, enabling proper user identification across reconnects as intended by the PR.Also applies to: 1777-1777
781-788: LGTM!The
connectionFailedcallback enhances error visibility by tracing connection failures and collecting stats when connection attempts fail, directly supporting the PR's stats collection improvements.
985-994: LGTM!The lifecycle observer cleanup correctly removes the observer on the Main thread using
withContext(Dispatchers.Main)and properly nullifies the reference. The previously reported bug (using==instead of=) has been fixed.
1071-1090: LGTM!The
sfuConnectionModuleis correctly wired into the Subscriber, and the emptyrejoin/fastReconnectcallbacks indicate intentional centralization of reconnect logic elsewhere in the call flow, reducing duplication.
1165-1183: LGTM!The
rejoin()call now includes an explicit reason ("Publisher#ERROR_CODE_REQUEST_VALIDATION_FAILED"), improving observability. The emptyfastReconnectcallback aligns with the centralized reconnect handling pattern.
1347-1361: LGTM!The ICE restart handling now properly handles null publisher by queuing the event for later processing, and includes an explicit reason string for better tracing.
1544-1641: LGTM!The stats collection improvements correctly implement transactional semantics by collecting tracer slices and rolling them back on failure. This prevents inconsistent state and improves reliability of stats reporting.
1766-1819: LGTM!The fast reconnect flow enhancements add comprehensive tracing, proper connection failure handling, and explicit reason strings. The dual peer connection health check (line 1800) correctly validates both subscriber and publisher states before proceeding.
1840-1846: LGTM!The
leaveWithReason()method correctly cancels themediaScopewhen truly leaving the call (as opposed to rejoining) and adds appropriate tracing for the leave operation.
…ascope is cancelled 2. Some renaming of variables and method
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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 (1)
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/Call.kt(13 hunks)
🧰 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/Call.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/Call.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/Call.kt
🧠 Learnings (1)
📚 Learning: 2025-12-19T09:15:37.256Z
Learnt from: CR
Repo: GetStream/stream-video-android PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-19T09:15:37.256Z
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/main/kotlin/io/getstream/video/android/core/Call.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). (6)
- GitHub Check: compare-sdk-sizes / Compare SDK sizes
- GitHub Check: Unit Tests
- GitHub Check: build
- GitHub Check: API check
- GitHub Check: Spotless check
- GitHub Check: Build / compose apks
🔇 Additional comments (6)
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/Call.kt (6)
366-372: LGTM!Adding descriptive reason strings to reconnect calls improves observability and aligns with the PR's goal of enhancing reconnect flow visibility.
580-580: LGTM!Including the error message in the leave reason provides useful context for debugging ring failures.
682-694: LGTM - Publisher ICE monitoring added symmetrically with subscriber.The implementation mirrors the subscriber monitoring pattern (lines 696-709), providing consistent handling of ICE connection failures for both peer connections. The null-safe chain ensures graceful handling if session or publisher is unavailable.
755-778: LGTM!The reason propagation to
ReconnectDetailsenables better tracing. The stats collection timing (afterfastReconnect) was discussed in previous reviews and confirmed as intentional since the stats are sent with the new session ID.
926-982: LGTM - Clean teardown with proper error handling.The refactored leave/internalLeave functions properly:
- Cancel and null out monitor jobs (lines 933-936)
- Use
safeCallto protect stats collection from exceptions- Sequence cleanup after stats are sent
The
safeCallwrapper ensures that if tracing or stats collection fails, it won't prevent the cleanup from executing. Based on learnings, this aligns with ensuring cleanup/teardown paths handle cancellation and failure.
1743-1747: LGTM!Propagating the reason parameter through the Debug API maintains consistency with the updated
fastReconnectsignature. The default value "Debug" appropriately identifies the source.
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/Call.kt
Show resolved
Hide resolved
|
Goal
Improve visibility on reconnect flow and stats.
Implementation
Added more traces.
Send stats when leaving
Send stats when rejoining
Send stats to track app lifecycle - foreground and background
Improve fast reconnect logic (and when its called)
Modified delta compression logic to include all fields on initial stats
Introduced iceTricklePendingEvents for buffering ICE candidates until web socket connection ready.
Introduced connectionFailed: suspend (Throwable) -> Unit callback parameter to whenConnected() methods.
Enhanced isFailedOrClosed() to check both PeerConnection and ICE connection states.
Testing
E2E Test + Run manual tests and check stats.
Summary by CodeRabbit
Bug Fixes
Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.