[Enhancement] Extend CallJoinInterceptor with will-join and did-join hooks#1736
[Enhancement] Extend CallJoinInterceptor with will-join and did-join hooks#1736rahul-lohra wants to merge 11 commits into
Conversation
PR checklist ✅All required conditions are satisfied:
🎉 Great job! This PR is ready for review. |
SDK Size Comparison 📏
|
rahul-lohra
changed the title
rahul-lohra
changed the title
rahul-lohra
changed the title
rahul-lohra
changed the title
WalkthroughIntroduces a new CallJoinLifecycleInterceptor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 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}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
demo-app/src/main/kotlin/io/getstream/video/android/DemoCallJoinInterceptor.kt (1)
49-50: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider
privatevisibility for the mutable lifecycle state.
observeJobandobserverScopeare exposed as public mutable members; marking themprivatekeeps the interceptor's coroutine state encapsulated. As per coding guidelines: "Prefer explicit visibility modifiers; limitinternalleakage across modules".🤖 Prompt for 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. In `@demo-app/src/main/kotlin/io/getstream/video/android/DemoCallJoinInterceptor.kt` around lines 49 - 50, The interceptor’s coroutine lifecycle state is currently exposed as public mutable members, so update the declarations in DemoCallJoinInterceptor to make observeJob and observerScope private and keep them encapsulated. Use the DemoCallJoinInterceptor class as the place to locate the fix, and preserve the existing coroutine behavior while restricting visibility as per the guideline to prefer explicit visibility modifiers and avoid leaking mutable state across modules.Source: Coding guidelines
🤖 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
`@demo-app/src/main/kotlin/io/getstream/video/android/DemoCallJoinInterceptor.kt`:
- Line 63: The log message in DemoCallJoinInterceptor.callWillJoin contains
leftover debug text (“noob”) and should be cleaned up. Update the logger.d call
in callWillJoin to use a professional message that only describes disabling
audio tracks, keeping the rest of the behavior unchanged.
In
`@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/ActiveStateGate.kt`:
- Around line 87-92: Guard the user callback in ActiveStateGate so interceptor
failures do not skip teardown: in the launch path around the
CallJoinLifecycleInterceptor.callDidJoin(call) hook, catch non-cancellation
exceptions and still allow cancelInterceptorJob() to run, and apply the same
protection in the main-transition path before cleanup() is invoked. Keep
CancellationException behavior unchanged by rethrowing it, and use the existing
ActiveStateGate, callDidJoin, cancelInterceptorJob, and cleanup symbols to place
the fix in both branches.
In
`@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/Call.kt`:
- Around line 629-633: The post-join hook `callWillJoin` in `Call` should be
protected from interceptor failures so a third-party exception does not turn a
successful join into a failed `join()` result. Update the `if
(callJoinInterceptor is CallJoinLifecycleInterceptor)` block to invoke
`callWillJoin(this)` through the same kind of `try/catch` shielding used for
`callReadyToJoin` via `ActiveStateGate.invokeInterceptor`, and log or swallow
the interceptor error so the join flow still completes successfully.
---
Nitpick comments:
In
`@demo-app/src/main/kotlin/io/getstream/video/android/DemoCallJoinInterceptor.kt`:
- Around line 49-50: The interceptor’s coroutine lifecycle state is currently
exposed as public mutable members, so update the declarations in
DemoCallJoinInterceptor to make observeJob and observerScope private and keep
them encapsulated. Use the DemoCallJoinInterceptor class as the place to locate
the fix, and preserve the existing coroutine behavior while restricting
visibility as per the guideline to prefer explicit visibility modifiers and
avoid leaking mutable state across modules.
🪄 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: 8d2281af-e686-439a-ad95-f4ae82ed9b0c
📒 Files selected for processing (6)
demo-app/src/main/kotlin/io/getstream/video/android/DemoCallJoinInterceptor.ktstream-video-android-core/api/stream-video-android-core.apistream-video-android-core/src/main/kotlin/io/getstream/video/android/core/ActiveStateGate.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/Call.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/CallJoinInterceptor.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/interceptor/CallJoinLifecycleInterceptor.kt
|
aleksandar-apostolov
left a comment
aleksandar-apostolov
left a comment
There was a problem hiding this comment.
Nice feature. Two things before it hits the public API:
1. Consider default methods instead of a new interface. We build with -Xjvm-default=all, so we can add callWillJoin/callDidJoin directly to CallJoinInterceptor with empty default bodies — existing implementers keep compiling, callReadyToJoin stays required. That avoids introducing CallJoinLifecycleInterceptor and deprecating the base interface (which pushes a warning onto everyone implementing callReadyToJoin, even though it isn't going away). Fewer types, no deprecation churn.
2. callWillJoin isn't best-effort. In Call.kt it's unwrapped, so a throwing hook propagates out of join() and the successful join is never returned. callDidJoin is wrapped — callWillJoin should be too.
| } | ||
|
|
||
| if (callJoinInterceptor is CallJoinLifecycleInterceptor) { | ||
| callJoinInterceptor.callWillJoin(this) |
There was a problem hiding this comment.
Wrap this like callDidJoin in ActiveStateGate (rethrow CancellationException, log-and-proceed) — otherwise a throwing hook fails the whole join.
| if (!isActive) return@launch | ||
|
|
||
| if (shouldProceed) onReady() | ||
| if (shouldProceed) { |
There was a problem hiding this comment.
Same callDidJoin try/catch at both onReady() sites — extract to a private helper.
| const val CALLER_READY_TO_JOIN_EVENT_TYPE = "caller_ready_join" | ||
| const val CALLEE_READY_TO_JOIN_EVENT_TYPE = "callee_ready_join" | ||
| } | ||
| var observeJob: Job? = null |
There was a problem hiding this comment.
observerScope is never cancelled and observeJob keeps running if callDidJoin never fires (aborted/failed join) — remote audio stays muted. Tie the scope to a cancellable lifecycle and make observeJob/observerScope private. Integrators copy the demo.
2 participants
Goal
IOS PR: GetStream/stream-video-swift#1176
Closes AND-1269
CallJoinInterceptorpreviously exposed a single gate,callReadyToJoin(), which only let integrators run (and optionally veto) work at the very last step of a join. That isn't enough for use cases that need to bracket the entire connection window — for example muting remote media while the peer connection is still negotiating and restoring it once the call is live. This PR adds two best-effort lifecycle hooks around the existing gate so integrators get a clear before/after pair.Implementation
New public API
CallJoinLifecycleInterceptor(core/call/interceptor) extendingCallJoinInterceptorwithcallWillJoin(call)andcallDidJoin(call).CallJoinInterceptor(WARNING) pointing to the new interface.Lifecycle wiring
Call.join()invokescallWillJoin(this)once the join result is produced, when the interceptor implementsCallJoinLifecycleInterceptor.ActiveStateGate.invokeInterceptor()invokescallDidJoin(call)right afteronReady()callback (which transitions to RingingState to Active)Demo App
callWillJoinand restores it incallDidJoin.Example Usage
Other
🎨 UI Changes
None
Testing
Summary by CodeRabbit
New Features
Bug Fixes
Chores