refactor(core): decompose Call into focused internal components#1747
Open
PratimMallick wants to merge 7 commits into
Open
refactor(core): decompose Call into focused internal components#1747PratimMallick wants to merge 7 commits into
PratimMallick wants to merge 7 commits into
Conversation
Break the ~2,260-line Call class into 11 internal collaborators under call/components (CallApiClient, CallStatsReporter, CallRenderer, CallEventManager, CallMediaManager, CallSessionManager, CallIceConnectionMonitor, CallConnectivityMonitor, CallJoinCoordinator, CallReconnector, CallLifecycleManager). Call remains a thin, binary-compatible public facade that delegates to them; public API is unchanged (apiCheck passes). Update white-box reflection tests to target the new component owners after internals moved out of Call. Co-authored-by: Cursor <cursoragent@cursor.com>
…nto refactor/call-class-decomposition Co-authored-by: Cursor <cursoragent@cursor.com> # Conflicts: # stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/Call.kt
Contributor
PR checklist ✅All required conditions are satisfied:
🎉 Great job! This PR is ready for review. |
Contributor
SDK Size Comparison 📏
|
Contributor
WalkthroughChangesCall responsibility decomposition
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Call
participant CallJoinCoordinator
participant RtcSession
participant CallReconnector
participant CallLifecycleManager
Call->>CallJoinCoordinator: join()
CallJoinCoordinator->>RtcSession: connect to SFU
RtcSession-->>CallJoinCoordinator: connection result
Call->>CallReconnector: reconnect(strategy, reason)
CallReconnector->>CallJoinCoordinator: rejoin or migrate
CallJoinCoordinator->>RtcSession: create replacement session
CallReconnector-->>Call: connected or exhausted
CallReconnector->>CallLifecycleManager: leave after exhausted recovery
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/Call.kt`:
- Around line 413-420: Remove the duplicated join preflight logic from Call’s
public join flow, including analytics, permission observation/checking, and
guest-token waiting around the shown joinCoordinator.join call. Keep that setup
exclusively in CallJoinCoordinator.join(), leaving Call’s join method as a
direct delegation with the existing arguments.
- Around line 759-765: Update Call.shutDownJobsGracefully so teardown does not
immediately cancel scope while supervisorJob children are draining. Complete the
supervisor to reject new children, await existing children within an owned
structured client scope, then perform final scope cancellation; handle
cancellation and child failures deterministically without relying on the
detached UserScope launch.
In
`@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/components/CallConnectivityMonitor.kt`:
- Around line 73-92: Update the disconnect handling around
leaveTimeoutAfterDisconnect so the timeout coroutine is created only when
clientImpl.leaveAfterDisconnectSeconds is positive. Treat zero or negative
values as disabled, avoiding delay(0) and the subsequent call.leave flow;
preserve the existing reconnection check and timeout behavior for positive
values.
- Line 40: Remove or redact call.id from the logger tags used by
CallConnectivityMonitor, CallIceConnectionMonitor, CallJoinCoordinator, and
CallReconnector. Update the logger initialization in
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/components/CallConnectivityMonitor.kt:40-40,
CallIceConnectionMonitor.kt:35-35, CallJoinCoordinator.kt:52-52, and
CallReconnector.kt:69-69 while preserving non-sensitive call context such as
call.type.
In
`@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/components/CallEventManager.kt`:
- Around line 77-95: The fireEvent method currently invokes listeners while
holding the subscriptions monitor and allows listener exceptions to abort
dispatch. Snapshot active subscriptions under synchronized(subscriptions),
release the lock, invoke each matching listener with per-listener failure
isolation so remaining callbacks continue, then always call
events.tryEmit(event) afterward.
In
`@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/components/CallIceConnectionMonitor.kt`:
- Around line 69-83: Update startSubscriberMonitor to collect the session and
subscriber flows rather than reading call.session.value?.subscriber?.value once
before collecting iceState. Keep the monitor active when the subscriber is
initially null and automatically switch to observing each replacement
subscriber’s iceState, matching the publisher monitor’s flow-observation pattern
while preserving the existing restart and logging behavior.
In
`@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/components/CallJoinCoordinator.kt`:
- Around line 163-184: Update the join flow in CallJoinCoordinator so a failed
join also resets the join-and-ring progress state. Handle the error on the outer
join result, not only inside the flatMap block, and call
state.toggleJoinAndRingProgress(false) while preserving the existing
ring-failure handling.
In
`@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/components/CallLifecycleManager.kt`:
- Around line 63-80: Sanitize all raw call identifiers in component logging:
update CallLifecycleManager.leave and internalLeave to remove or hash call.cid
and call.id, and sanitize or remove call.id from logger tags in
CallLifecycleManager (39-39), CallMediaManager (47-47), CallRenderer (43-43),
and CallStatsReporter (35-35). Preserve the existing log context without
exposing call IDs or CIDs.
- Around line 107-118: Update the coroutine launched in the call-leave flow to
wrap the leave analytics, tracing, stats, and leave-event operations in a
try/finally block, with cleanup() in finally. Ensure cleanup always runs after
the leave markers are attempted, including when onCallLeave() throws or the
coroutine is cancelled.
In
`@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/components/CallMediaManager.kt`:
- Around line 270-272: Update CallMediaManager.cleanup() to dispose the owned
peer-connection factory during final teardown, in addition to calling
mediaManager.cleanup(). Reuse the existing factory instance and disposal
mechanism already used during recreation, ensuring native WebRTC resources are
released.
In
`@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/components/CallReconnector.kt`:
- Around line 254-271: Update the FAST reconnect failure handling in
CallReconnector’s ReconnectOutcome.Failed branch to consult
MAX_FAST_RECONNECT_ATTEMPTS and escalate to the REJOIN strategy once the
configured fast-retry limit is reached. Preserve the existing migration and
deadline escalation conditions, and apply the same logic to the corresponding
failure branch around the alternate referenced location.
- Around line 178-187: Update the reconnect loop in CallReconnector so the
network-availability wait is skipped when the active strategy is DISCONNECT,
allowing the server-requested disconnect path to execute offline and produce
SFU_DISCONNECT. Preserve the existing connectivity polling behavior for other
reconnect strategies.
In
`@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/components/CallRenderer.kt`:
- Around line 156-178: Update takeScreenshot around suspendCancellableCoroutine
to register an invokeOnCancellation handler that asynchronously removes
screenshotSink through the same call.scope cleanup path used after the first
frame. Ensure cancellation cleanup is safe if no frame has arrived, while
preserving the existing first-frame removal behavior.
In
`@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/components/CallStatsReporter.kt`:
- Around line 45-57: Update CallStatsReporter.start so the coroutine keeps the
initial reportingIntervalMs delay, then sends the first stats report immediately
upon entering the while (isActive) loop; retain the interval delay only for
subsequent reports.
In
`@stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/CallFieldDeclarationOrderTest.kt`:
- Around line 53-67: Update CallFieldDeclarationOrderTest to assert that
eventManagerLine precedes eventsLine, in addition to the existing ordering
checks. Keep the requirement that both eventManager and events are declared
before state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ce0c99a0-1916-4ef7-85a5-2b59619e7d82
📒 Files selected for processing (17)
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/Call.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/components/CallApiClient.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/components/CallConnectivityMonitor.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/components/CallEventManager.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/components/CallIceConnectionMonitor.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/components/CallJoinCoordinator.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/components/CallLifecycleManager.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/components/CallMediaManager.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/components/CallReconnector.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/components/CallRenderer.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/components/CallSessionManager.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/components/CallStatsReporter.ktstream-video-android-core/src/test/kotlin/io/getstream/video/android/core/CallFieldDeclarationOrderTest.ktstream-video-android-core/src/test/kotlin/io/getstream/video/android/core/reconnect/FailedSfuIdsTest.ktstream-video-android-core/src/test/kotlin/io/getstream/video/android/core/reconnect/ReconnectAttemptsCountTest.ktstream-video-android-core/src/test/kotlin/io/getstream/video/android/core/reconnect/ReconnectSessionIdTest.ktstream-video-android-core/src/test/kotlin/io/getstream/video/android/core/rtc/JoinRecoverableFailureTest.kt
Call.join() ran join analytics, permission checks, and the guest-token wait before delegating to CallJoinCoordinator.join(), which performed the exact same preflight — so every join() executed it twice. Make the facade a pure delegation so the preflight runs only once in the coordinator. Co-authored-by: Cursor <cursoragent@cursor.com>
…nto refactor/call-class-decomposition
Add JVM unit tests for the extracted Call collaborators (CallApiClient, CallEventManager, CallSessionManager, CallRenderer, CallMediaManager) to raise coverage on the refactor's new code toward the SonarCloud gate. Co-authored-by: Cursor <cursoragent@cursor.com>
…ents Broaden new-code coverage for the extracted Call components: exercise the CallJoinCoordinator retry loop and join-and-ring flow (via the RtcSession test factory), the CallConnectivityMonitor reconnect/leave listener, the reachable CallReconnector state-machine branches, the CallIceConnectionMonitor restart paths, plus additional CallMediaManager (monitorHeadset, not-selected devices) and CallApiClient (ring request, ringing create) cases. Co-authored-by: Cursor <cursoragent@cursor.com>
Add a unitTestRtcSessionFactory seam to CallReconnector's rejoin/migrate so the session-swap, monitor and finalize paths are unit-testable, and add tests for them (success + retry-until-exhausted). Also cover CallRenderer's incoming-audio track walking for all/selected participants. Co-authored-by: Cursor <cursoragent@cursor.com>
|
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
1 participant
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
closes AND-1318
Call.kthad grown to ~2,260 lines and mixed many unrelated responsibilities (REST API calls, media, session/RTC lifecycle, reconnect state machine, connectivity/ICE monitoring, events, stats, rendering, teardown). This made it hard to read, test, and evolve.This PR decomposes
Callinto focusedinternalcollaborators while keepingCallas a thin, binary-compatible public facade — no public API changes.Implementation
internalcomponents undercore/call/components/, each owning one concern;Calldelegates to them and keeps its existing member names as delegating accessors soRtcSession,CallState, and existing callers keep working:CallApiClient— coordinator REST wrappersCallEventManager—eventsflow, subscriptions, dispatchCallStatsReporter— stats collection/reporting,statsReport/statLatencyHistoryCallRenderer— renderer binding, visibility, screenshots, incoming media overridesCallMediaManager— peer-connection factory, media manager, audio pipeline, screen sharingCallSessionManager— RTC sessionStateFlow, session ids, location, reconnect countersCallIceConnectionMonitor— publisher/subscriber ICE-restart monitoringCallConnectivityMonitor— network listener + leave-on-disconnect timeoutCallJoinCoordinator— join flow, retry loop,RtcSessioncreation/connectionCallReconnector— FAST/REJOIN/MIGRATE reconnect state machine, failed-SFU trackingCallLifecycleManager— leave/end/cleanup, leave guard, destroyed flagCallremains the orchestrator for a few cross-component flows (e.g.monitorSession).developand reconciled itsCallchanges into the new components: the typed SFU connect-failure recovery (SfuConnectionResult.Success/Failure+SfuConnectFailureCause) now lives inCallJoinCoordinator; theunitTestRtcSessionFactorytest hook replacesTestInstanceProvider.rtcSessionCreator.Call.Testing
./gradlew :stream-video-android-core:testDebugUnitTest— pass (894 tests, 0 failures)./gradlew :stream-video-android-core:apiCheck— pass (public API unchanged / binary-compatible)./gradlew :stream-video-android-core:spotlessCheck— pass./gradlew assembleDebug— pass (all modules incl. demo-app)☑️Contributor Checklist
General
developbranchCode & documentation
stream-video-examples)☑️Reviewer Checklist
🎉 GIF
Skipped — internal refactor with no user-facing change.
Made with Cursor
Summary by CodeRabbit