Improve pinned participant state on rejoin with new session#1683
Open
rahul-lohra wants to merge 22 commits into
Open
Improve pinned participant state on rejoin with new session#1683rahul-lohra wants to merge 22 commits into
rahul-lohra wants to merge 22 commits into
Conversation
# Conflicts: # stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/utils/SignalLostSignalingServiceDecorator.kt # stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/trace/SignalingServiceTracerDecorator.kt
13 tasks
Contributor
PR checklist ✅All required conditions are satisfied:
🎉 Great job! This PR is ready for review. |
WalkthroughThis PR introduces SFU metrics reporting via a new ChangesSFU Metrics and Participant Pinning
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 1
🧹 Nitpick comments (6)
generate-sfu.sh (2)
5-5: 💤 Low valueConsider supporting HTTPS URLs for broader environment compatibility.
The SSH URL
git@github.com:GetStream/protocol.gitrequires SSH key authentication, which may not be configured in all CI/CD environments. Consider parameterizing the URL or defaulting to HTTPS (https://github.com/GetStream/protocol.git) for wider compatibility.🤖 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 `@generate-sfu.sh` at line 5, The REPO_URL variable is hardcoded to an SSH URL which can fail in CI; update the script to accept an override (e.g., via environment variable or script arg) and default to an HTTPS URL; specifically change the REPO_URL assignment (REPO_URL) to use something like ${REPO_URL:-https://github.com/GetStream/protocol.git} or parse a passed parameter so callers can supply an SSH URL when needed.
23-39: 💤 Low valueSimplify git checkout logic for the default branch case.
When
REFERENCE_TYPEis"branch"(the default), line 23'sgit clone --branch "$REFERENCE_VALUE"already checks out the specified branch, making the subsequentgit checkout "$REFERENCE_VALUE"at line 29 redundant. For tags and commits, the shallow clone won't fetch the ref, so the checkout logic is still needed for those cases.Consider restructuring to only checkout when needed:
♻️ Proposed refactor
git clone --depth=1 --branch "$REFERENCE_VALUE" "$REPO_URL" "$CLONE_DIR" cd "$CLONE_DIR" -# Step 3: Checkout to the correct branch, tag, or commit if [ "$REFERENCE_TYPE" == "branch" ]; then - git checkout "$REFERENCE_VALUE" + # Already on the correct branch from clone + : elif [ "$REFERENCE_TYPE" == "tag" ]; then git fetch --tags git checkout "tags/$REFERENCE_VALUE"🤖 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 `@generate-sfu.sh` around lines 23 - 39, The branch-case checkout is redundant because git clone --branch "$REFERENCE_VALUE" already checks out the branch; remove the branch branch (the branch branch of the if) or change the logic so you only run git checkout for tag and commit cases (keep the tag branch using git fetch --tags + git checkout "tags/$REFERENCE_VALUE" and the commit branch using git fetch --depth=1 origin "$REFERENCE_VALUE" + git checkout "$REFERENCE_VALUE"), and ensure the ELSE still prints the invalid REFERENCE_TYPE error; reference REFERENCE_TYPE, REFERENCE_VALUE, the initial git clone --branch invocation, git fetch --tags, and git fetch --depth=1 origin when making the change.stream-video-android-core/api/stream-video-android-core.api (1)
11944-11958: ⚡ Quick winBreaking constructor change on public
ParticipantJoinedEvent— add a default forisPinnedand call out in release notes.The primary constructor went from
(Participant, String)to(Participant, String, Z). While this is an SDK-emitted event (consumers typically observe rather than construct it — consistent with the prior guidance on internal-only-constructed event types in this file), any external/test code that instantiatesParticipantJoinedEventpositionally (e.g., in user-side unit tests or fixtures) will break at source and binary level.Consider giving
isPinneda default value in the Kotlin data class so source compatibility is preserved for existing Kotlin callers, and mention the constructor shape change in the release notes/migration guide.- data class ParticipantJoinedEvent(val participant: Participant, val callCid: String, val isPinned: Boolean) : SfuDataEvent() + data class ParticipantJoinedEvent( + val participant: Participant, + val callCid: String, + val isPinned: Boolean = false, + ) : SfuDataEvent()Based on learnings: prior guidance for this
.apifile (PR 1588,LocalCallMissedEvent) is that breaking changes to internal/SDK-instantiated event constructors are acceptable but should be documented in release notes and the public-surface impact should be minimized.🤖 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 `@stream-video-android-core/api/stream-video-android-core.api` around lines 11944 - 11958, The public data class ParticipantJoinedEvent changed its primary constructor from (Participant, String) to (Participant, String, Boolean) causing a binary/source break; restore source compatibility by giving the new Boolean parameter a default (e.g., isPinned: Boolean = false) in the ParticipantJoinedEvent declaration so existing positional callers continue to compile, ensure the generated copy/copy$default signatures remain compatible, and update release notes/migration guide to call out the constructor shape change and the default value.stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/trace/SignalingServiceTracerDecorator.kt (1)
102-103: ⚡ Quick winConsider adding a comment explaining why
sendMetricsbypasses tracing.Unlike most other RPC methods (which use the
traced()wrapper),sendMetricscalls the target directly without tracing—matching the pattern ofsendStats(line 99). If this is intentional to reduce log volume for high-frequency metrics reporting, a brief comment would clarify the design choice and prevent confusion.For example:
// Skip tracing for sendMetrics to avoid high-frequency log noise override suspend fun sendMetrics(sendMetricsRequest: SendMetricsRequest): SendMetricsResponse = target.sendMetrics(sendMetricsRequest)🤖 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 `@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/trace/SignalingServiceTracerDecorator.kt` around lines 102 - 103, The sendMetrics method in SignalingServiceTracerDecorator directly delegates to target.sendMetrics without using the traced() wrapper (like sendStats does), which is unclear to readers; add a concise inline comment above the sendMetrics override explaining that tracing is intentionally skipped to avoid high-frequency log noise (or whatever the intended rationale) so future maintainers understand why sendMetrics bypasses traced(), referencing the sendMetrics override and the traced() wrapper for context.stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/utils/RetryableSignalingServiceDecorator.kt (1)
122-123: ⚡ Quick winAdd a comment documenting why
sendMetricsuses a null error extractor.
SendMetricsResponseis an empty proto message with no error field, making{ null }the only valid extractor. UnlikesendStats(which uses{ it.error }) where the response includes amodels.Error errorfield, metrics responses cannot contain SFU errors. A brief inline comment (e.g.,retryCall({ null }) // SendMetricsResponse has no error field) will clarify this for readers comparing against other RPC methods.🤖 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 `@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/utils/RetryableSignalingServiceDecorator.kt` around lines 122 - 123, The retry call for sendMetrics currently passes a null error extractor (retryCall({ null }) { decorated.sendMetrics(sendMetricsRequest) }) which is necessary because SendMetricsResponse is an empty proto with no error field; add a brief inline comment on that line (e.g., "// SendMetricsResponse has no error field; extractor must be null") to explain why this differs from sendStats (which uses { it.error }) so readers understand the choice; update the comment next to the retryCall invocation in RetryableSignalingServiceDecorator.sendMetrics.stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/CallState.kt (1)
1097-1102: ⚡ Quick winSimplify participant-left pin cleanup.
Lines 1097-1099 explicitly check
localPinsand callpinManager.unpin, butpinManager.onParticipantLeft(called at line 1102) already performs the same check and callsunpininternally (seePinManager.ktlines 52-56). The explicitunpincall is redundant.♻️ Simplify by removing redundant unpin call
- if (pinManager.localPins.value.containsKey(sessionId)) { - // Remove any pins for the participant - pinManager.unpin(sessionId) - } - pinManager.onParticipantLeft(sessionId)🤖 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 `@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/CallState.kt` around lines 1097 - 1102, The code redundantly calls pinManager.unpin(sessionId) after checking pinManager.localPins when pinManager.onParticipantLeft(sessionId) already handles that; remove the explicit containsKey check and the pinManager.unpin(sessionId) call in CallState.kt and keep only the single pinManager.onParticipantLeft(sessionId) invocation so cleanup is performed via PinManager.onParticipantLeft.
🤖 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/test/kotlin/io/getstream/video/android/core/CallStateTest.kt`:
- Around line 131-136: The test data has an inconsistent userId: update the
PinUpdate userId used in the call.state.pinManager.updateLocalPins call so it
matches the ParticipantState userId ("1") — locate the call to
call.state.pinManager.updateLocalPins that creates a PinEntry with
PinUpdate("1", "userId") and change the second argument to "1" (i.e.,
PinUpdate("1", "1")) so PinUpdate, PinEntry and the ParticipantState share the
same userId.
---
Nitpick comments:
In `@generate-sfu.sh`:
- Line 5: The REPO_URL variable is hardcoded to an SSH URL which can fail in CI;
update the script to accept an override (e.g., via environment variable or
script arg) and default to an HTTPS URL; specifically change the REPO_URL
assignment (REPO_URL) to use something like
${REPO_URL:-https://github.com/GetStream/protocol.git} or parse a passed
parameter so callers can supply an SSH URL when needed.
- Around line 23-39: The branch-case checkout is redundant because git clone
--branch "$REFERENCE_VALUE" already checks out the branch; remove the branch
branch (the branch branch of the if) or change the logic so you only run git
checkout for tag and commit cases (keep the tag branch using git fetch --tags +
git checkout "tags/$REFERENCE_VALUE" and the commit branch using git fetch
--depth=1 origin "$REFERENCE_VALUE" + git checkout "$REFERENCE_VALUE"), and
ensure the ELSE still prints the invalid REFERENCE_TYPE error; reference
REFERENCE_TYPE, REFERENCE_VALUE, the initial git clone --branch invocation, git
fetch --tags, and git fetch --depth=1 origin when making the change.
In `@stream-video-android-core/api/stream-video-android-core.api`:
- Around line 11944-11958: The public data class ParticipantJoinedEvent changed
its primary constructor from (Participant, String) to (Participant, String,
Boolean) causing a binary/source break; restore source compatibility by giving
the new Boolean parameter a default (e.g., isPinned: Boolean = false) in the
ParticipantJoinedEvent declaration so existing positional callers continue to
compile, ensure the generated copy/copy$default signatures remain compatible,
and update release notes/migration guide to call out the constructor shape
change and the default value.
In
`@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/utils/RetryableSignalingServiceDecorator.kt`:
- Around line 122-123: The retry call for sendMetrics currently passes a null
error extractor (retryCall({ null }) { decorated.sendMetrics(sendMetricsRequest)
}) which is necessary because SendMetricsResponse is an empty proto with no
error field; add a brief inline comment on that line (e.g., "//
SendMetricsResponse has no error field; extractor must be null") to explain why
this differs from sendStats (which uses { it.error }) so readers understand the
choice; update the comment next to the retryCall invocation in
RetryableSignalingServiceDecorator.sendMetrics.
In
`@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/CallState.kt`:
- Around line 1097-1102: The code redundantly calls pinManager.unpin(sessionId)
after checking pinManager.localPins when pinManager.onParticipantLeft(sessionId)
already handles that; remove the explicit containsKey check and the
pinManager.unpin(sessionId) call in CallState.kt and keep only the single
pinManager.onParticipantLeft(sessionId) invocation so cleanup is performed via
PinManager.onParticipantLeft.
In
`@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/trace/SignalingServiceTracerDecorator.kt`:
- Around line 102-103: The sendMetrics method in SignalingServiceTracerDecorator
directly delegates to target.sendMetrics without using the traced() wrapper
(like sendStats does), which is unclear to readers; add a concise inline comment
above the sendMetrics override explaining that tracing is intentionally skipped
to avoid high-frequency log noise (or whatever the intended rationale) so future
maintainers understand why sendMetrics bypasses traced(), referencing the
sendMetrics override and the traced() wrapper for context.
🪄 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: 50da1809-e10f-4c41-bb83-31f725cc762e
⛔ Files ignored due to path filters (6)
stream-video-android-core/src/main/proto/video/sfu/event/events.pb.gois excluded by!**/*.pb.gostream-video-android-core/src/main/proto/video/sfu/event/events_vtproto.pb.gois excluded by!**/*.pb.gostream-video-android-core/src/main/proto/video/sfu/models/models.pb.gois excluded by!**/*.pb.gostream-video-android-core/src/main/proto/video/sfu/models/models_vtproto.pb.gois excluded by!**/*.pb.gostream-video-android-core/src/main/proto/video/sfu/signal_rpc/signal.pb.gois excluded by!**/*.pb.gostream-video-android-core/src/main/proto/video/sfu/signal_rpc/signal_vtproto.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (18)
generate-sfu.shstream-video-android-core/api/stream-video-android-core.apistream-video-android-core/src/main/kotlin/io/getstream/video/android/core/Call.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/CallState.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/signal/socket/RTCEventMapper.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/call/utils/RetryableSignalingServiceDecorator.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/events/SfuDataEvent.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/pinning/PinManager.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/pinning/PinUpdates.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/trace/SignalingServiceTracerDecorator.ktstream-video-android-core/src/main/proto/video/sfu/event/events.protostream-video-android-core/src/main/proto/video/sfu/models/models.protostream-video-android-core/src/main/proto/video/sfu/signal_rpc/signal.protostream-video-android-core/src/main/proto/video/sfu/signal_rpc/signal.twirp.gostream-video-android-core/src/test/kotlin/io/getstream/video/android/core/CallStateTest.ktstream-video-android-core/src/test/kotlin/io/getstream/video/android/core/SfuSocketStateEventTest.ktstream-video-android-core/src/test/kotlin/io/getstream/video/android/core/pinning/PinManagerTest.ktstream-video-android-ui-compose/src/main/kotlin/io/getstream/video/android/compose/ui/components/call/renderer/ParticipantVideo.kt
Comment on lines
+131
to
136
| call.state.pinManager.updateLocalPins( | ||
| mutableMapOf( | ||
| "1" to PinEntry( | ||
| PinUpdate("1", "userId"), OffsetDateTime.now(Clock.systemUTC()), PinType.Local, | ||
| ), | ||
| ), |
There was a problem hiding this comment.
Fix userId inconsistency in test data.
The PinUpdate uses userId = "userId" (literal string), but the corresponding ParticipantState at line 156 uses userId = "1". While the test may pass due to sessionId matching, this inconsistency reduces test clarity and could cause confusion.
🔧 Proposed fix
call.state.pinManager.updateLocalPins(
mutableMapOf(
"1" to PinEntry(
- PinUpdate("1", "userId"), OffsetDateTime.now(Clock.systemUTC()), PinType.Local,
+ PinUpdate("1", "1"), OffsetDateTime.now(Clock.systemUTC()), PinType.Local,
),
),
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| call.state.pinManager.updateLocalPins( | |
| mutableMapOf( | |
| "1" to PinEntry( | |
| PinUpdate("1", "userId"), OffsetDateTime.now(Clock.systemUTC()), PinType.Local, | |
| ), | |
| ), | |
| call.state.pinManager.updateLocalPins( | |
| mutableMapOf( | |
| "1" to PinEntry( | |
| PinUpdate("1", "1"), OffsetDateTime.now(Clock.systemUTC()), PinType.Local, | |
| ), | |
| ), |
🤖 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
`@stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/CallStateTest.kt`
around lines 131 - 136, The test data has an inconsistent userId: update the
PinUpdate userId used in the call.state.pinManager.updateLocalPins call so it
matches the ParticipantState userId ("1") — locate the call to
call.state.pinManager.updateLocalPins that creates a PinEntry with
PinUpdate("1", "userId") and change the second argument to "1" (i.e.,
PinUpdate("1", "1")) so PinUpdate, PinEntry and the ParticipantState share the
same userId.
rahul-lohra
changed the title
rahul-lohra
changed the title
|
Contributor
SDK Size Comparison 📏
|
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
Server-side pin synchronization was previously tightly coupled to
sessionIdsonly. This created issues during participant reconnection flows where a participant could reconnect with a newsessionIdwhile still representing the same user.The goal of this change is to make server pin resolution resilient to reconnect scenarios while preserving the server-defined pin priority order.
Additionally:
TimeProviderParticipantJoinedSFU event now will carryis_pinnedproperty so you can use it for updating the client-side state correctlyImplementation
React PR
is_pinnedin ParticipantJoinedEventPin Resolution
A new PinManager has been introduced as the central owner of:
The main entry points are:
New PinManager class (pinning/PinManager.kt)
pin(),unpin(),setServerPins(),onParticipantJoined()— that were previously scattered acrossCallStatepinnedAt = now + (pins.size - index)so the server's priority order is preserved as a descending timestamp sequencesessionId→ exact match,userId→ fallback). Duplicate userId entries are dropped so the wrong participant is never pinned on reconnectCallState cleanup
pin()/unpin()delegate toPinManagerupdateServerSidePins()private function removed — replaced byPinManager.setServerPins()SessionIdtypealias introduced for readabilityThere are auto generated files, which are not the core part of this PR
SignalLostSignalingServiceDecorator.ktSignalingServiceTracerDecorator.kt🎨 UI Changes
None
Testing
Scenario A: Pin propagation on join
Status: Passed ✅
Scenario A – Rejoin Flow (User C)
On User Cstill sees User B pinnedStatus: Passed ✅
Scenario A – Rejoin Flow (User B)
Step 1: User B leaves
on User Cthat no participants are pinnedStatus: Passed ✅
Step 2: User B rejoins flow
on User Bthat User B appears pinnedObserved:
Status: Passed (expected due to known issue) ✅
Reason: Pins from JoinCallResponseEvent may contain a different sessionId, but the fallback userId matching successfully restores the pin association.
Step 3: User B rejoins flow
State on User C
on User Cstill sees User B pinnedStatus: Passed ✅
Summary by CodeRabbit
New Features
Improvements