Improve pinned participant state on rejoin with new session#1644
Open
rahul-lohra wants to merge 16 commits into
Open
Improve pinned participant state on rejoin with new session#1644rahul-lohra wants to merge 16 commits into
rahul-lohra wants to merge 16 commits into
Conversation
Contributor
PR checklist ✅All required conditions are satisfied:
🎉 Great job! This PR is ready for review. |
Contributor
SDK Size Comparison 📏
|
rahul-lohra
changed the title
WalkthroughA shell script automates SFU proto generation from the GetStream protocol repository. The signaling system gains a metrics-reporting RPC. Participant pinning state now uses SessionId types instead of raw strings. Event models are extended with pinned participant flags, and RTP-related protobuf messages are introduced for metrics collection. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 5
🧹 Nitpick comments (4)
generate-sfu.sh (3)
3-3: Harden shell options for safer failures.Line 3 uses
set -eonly;-uand-o pipefailmake failure behavior more deterministic in automation scripts.Suggested fix
-set -e +set -euo pipefail🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@generate-sfu.sh` at line 3, Replace the lone "set -e" shell option with hardened options to avoid silent failures: change the existing set -e line to set -euo pipefail (optionally also set IFS=$'\n\t' afterwards) so undefined variables and pipeline failures are treated as errors; update the single "set -e" occurrence in the script accordingly.
5-5: Consider HTTPS default for better portability.Line 5 requires SSH key setup (
git@github.com:...), which commonly breaks local/CI usage unless keys are preconfigured.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@generate-sfu.sh` at line 5, The REPO_URL variable in generate-sfu.sh is set to an SSH URL which requires SSH keys; change it to use the HTTPS form (e.g., https://github.com/GetStream/protocol.git) or make REPO_URL default to the HTTPS URL but allow overriding via an environment variable (keep the REPO_URL symbol and read from ${REPO_URL:-"https://github.com/GetStream/protocol.git"} so CI and local users without SSH keys can clone by default).
21-25: Usetrapfor guaranteed cleanup of the temp clone directory.If clone/copy/Spotless fails, cleanup at Line 59 is skipped and leaves build artifacts behind.
Suggested fix
rm -rf "$CLONE_DIR" mkdir -p "$BUILD_DIR" + +cleanup() { + rm -rf "$CLONE_DIR" +} +trap cleanup EXIT + git clone --depth=1 --branch "$REFERENCE_VALUE" "$REPO_URL" "$CLONE_DIR" @@ -# Step 6: Delete CLONE_DIR -echo "🗑 Cleaning up cloned repo..." -rm -rf "$CLONE_DIR" - echo "✅ Done."Also applies to: 57-59
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@generate-sfu.sh` around lines 21 - 25, Add a shell trap to guarantee cleanup of the temporary clone directory by registering a handler that removes "$CLONE_DIR" (and any other temp artifacts like "$BUILD_DIR" if desired) on EXIT and ERR; specifically, after creating or cloning into "$CLONE_DIR" (around where git clone and cd "$CLONE_DIR" occur) add a trap 'cleanup' or inline trap that runs rm -rf "$CLONE_DIR" (and optionally rm -rf "$BUILD_DIR") so the directory is removed regardless of script failure or early exit, and ensure any explicit cleanup at lines 57-59 is removed or made idempotent because the trap will already handle it.stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/trace/SignalingServiceTracerDecorator.kt (1)
128-130: Consider adding a comment for consistency.The
sendStatsmethod at line 123-126 has a// Not tracedcomment explaining why tracing is skipped. Adding a similar comment tosendMetricswould improve maintainability and make the intentional omission explicit.📝 Suggested comment addition
+ // Not traced - high-frequency metrics reporting override suspend fun sendMetrics(sendMetricsRequest: SendMetricsRequest): SendMetricsResponse { return target.sendMetrics(sendMetricsRequest) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/trace/SignalingServiceTracerDecorator.kt` around lines 128 - 130, Add a short explanatory comment above the sendMetrics override to match sendStats' style (e.g., "// Not traced") so maintainers know tracing is intentionally skipped; locate the sendMetrics(sendMetricsRequest: SendMetricsRequest): SendMetricsResponse method in SignalingServiceTracerDecorator and insert the same brief comment as used for sendStats to keep consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@generate-sfu.sh`:
- Around line 23-35: The git clone invocation always passes --branch
"$REFERENCE_VALUE", which breaks when REFERENCE_TYPE="commit" because commit
SHAs are not branch names; change the logic so clone does not include --branch
for commit mode (e.g., branch/tag: use --branch "$REFERENCE_VALUE", commit:
clone without --branch or clone with --no-checkout and then fetch/checkout the
SHA). Update the script around the git clone and the REFERENCE_TYPE conditional
to handle REFERENCE_TYPE ("branch", "tag", "commit") separately so that when
REFERENCE_TYPE is "commit" the clone succeeds and the subsequent git fetch/git
checkout of REFERENCE_VALUE (the SHA) in the commit branch-handling block (the
lines referencing REFERENCE_TYPE, REFERENCE_VALUE and the git fetch/checkout
commands) is reached and used.
In
`@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/CallState.kt`:
- Around line 1107-1112: Replace the leftover debug call using
android.util.Log.d("Noob", ...) with the module's logger: use the existing
logger instance (logger) to emit the same message at an appropriate level (e.g.,
logger.d or logger.v) and include the _serverPins.value.map { it.key
}.joinToString(",") payload; guard the log emission behind
StreamVideoImpl.developmentMode (or existing development-mode check) to respect
verbosity rules and avoid hardcoded tags.
- Around line 1268-1281: The debug Log.d call in updateServerSidePins (the one
using hardcoded "Noob" tag and printing
pins/internalParticipantsText/pinnedInCall) must be removed or replaced with the
module logger and guarded by StreamVideoImpl.developmentMode; locate the Log.d
invocation inside updateServerSidePins and either delete it or change it to use
the existing logger (e.g., logger.debug/trace) and wrap the log emit with a
developmentMode check so production builds don't leak verbose debug output.
- Around line 1505-1507: The code creates an unused local variable
participantNames via participants.joinToString in CallState.kt; remove the dead
assignment (the participantNames declaration) from the enclosing function or
block (or, if intended for logging, replace the unused variable with a direct
log/usage) so there is no unused local variable left; ensure no other logic
depends on participantNames before deleting.
In
`@stream-video-android-core/src/test/kotlin/io/getstream/video/android/core/CallStateTest.kt`:
- Around line 131-137: The test constructs a PinUpdate with arguments reversed
relative to the implementation: when map key "1" is the sessionId, the PinUpdate
must be created as PinUpdate(userId, sessionId) to match CallState.pin()
behavior; update the call to call.state.pins.updateLocalPins(...) so the map
entry uses PinUpdate("userId", "1") (wrapped in PinUpdateAtTime) instead of
PinUpdate("1", "userId") so keys and PinUpdate fields align with the
PinUpdateAtTime/PinUpdate usage.
---
Nitpick comments:
In `@generate-sfu.sh`:
- Line 3: Replace the lone "set -e" shell option with hardened options to avoid
silent failures: change the existing set -e line to set -euo pipefail
(optionally also set IFS=$'\n\t' afterwards) so undefined variables and pipeline
failures are treated as errors; update the single "set -e" occurrence in the
script accordingly.
- Line 5: The REPO_URL variable in generate-sfu.sh is set to an SSH URL which
requires SSH keys; change it to use the HTTPS form (e.g.,
https://github.com/GetStream/protocol.git) or make REPO_URL default to the HTTPS
URL but allow overriding via an environment variable (keep the REPO_URL symbol
and read from ${REPO_URL:-"https://github.com/GetStream/protocol.git"} so CI and
local users without SSH keys can clone by default).
- Around line 21-25: Add a shell trap to guarantee cleanup of the temporary
clone directory by registering a handler that removes "$CLONE_DIR" (and any
other temp artifacts like "$BUILD_DIR" if desired) on EXIT and ERR;
specifically, after creating or cloning into "$CLONE_DIR" (around where git
clone and cd "$CLONE_DIR" occur) add a trap 'cleanup' or inline trap that runs
rm -rf "$CLONE_DIR" (and optionally rm -rf "$BUILD_DIR") so the directory is
removed regardless of script failure or early exit, and ensure any explicit
cleanup at lines 57-59 is removed or made idempotent because the trap will
already handle it.
In
`@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/trace/SignalingServiceTracerDecorator.kt`:
- Around line 128-130: Add a short explanatory comment above the sendMetrics
override to match sendStats' style (e.g., "// Not traced") so maintainers know
tracing is intentionally skipped; locate the sendMetrics(sendMetricsRequest:
SendMetricsRequest): SendMetricsResponse method in
SignalingServiceTracerDecorator and insert the same brief comment as used for
sendStats to keep consistency.
🪄 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: 5997c08f-386f-412e-9052-dacf26066c5e
⛔ 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 (14)
generate-sfu.shstream-video-android-core/api/stream-video-android-core.apistream-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/SignalLostSignalingServiceDecorator.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/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-ui-compose/src/main/kotlin/io/getstream/video/android/compose/ui/components/call/renderer/ParticipantVideo.ktstream-video-android-ui-compose/src/main/kotlin/io/getstream/video/android/compose/ui/components/call/renderer/ParticipantsLayout.kt
💤 Files with no reviewable changes (1)
- stream-video-android-ui-compose/src/main/kotlin/io/getstream/video/android/compose/ui/components/call/renderer/ParticipantsLayout.kt
rahul-lohra
changed the title
rahul-lohra
changed the title
# 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
|
| val updatedList = tempPinUpdateList.toMutableList().apply { | ||
| add(pinUpdate) | ||
| } | ||
| updateServerSidePins(updatedList) |
Contributor
There was a problem hiding this comment.
updateServerSidePins(updatedList) rebuilds the entire _serverPins map with OffsetDateTime.now() for every entry — so when one new pinned participant joins, every other pinned user's timestamp also gets refreshed. Is that intentional?
| updateServerSidePins(updatedList) | ||
| } | ||
| } else { | ||
| _serverPins.value = _serverPins.value.filter { it.key != participantSessionId } |
Contributor
There was a problem hiding this comment.
The new participantSessionId can't already be in _serverPins (sessionIds are unique per session) — so this filter looks like a no-op for the isPinned == false branch. Defensive, or am I missing a case where the sessionId could repeat?
Contributor
Author
There was a problem hiding this comment.
Will remove this code
| } | ||
|
|
||
| if (_serverPins.value.containsKey(sessionId)) { | ||
| _serverPins.value = _serverPins.value.filter { it.key != event.participant.session_id } |
Contributor
There was a problem hiding this comment.
Could this be the actual root cause of the "Failed: User B rejoin" scenario in the PR description, rather than the sessionId mismatch? JS doesn't do this — its leave handler just removes the participant locally, leaving the server-side pin alone, which is why JS rejoin works. Android calls unpinForEveryone here, so when User B leaves the server is told to unpin them; by the time they rejoin, is_pinned on the server is false and the new event handler has nothing to act on.
| } | ||
| } | ||
|
|
||
| internal fun updateServerSidePins(internalParticipants: Map<SessionId, ParticipantState>, event: ParticipantJoinedEvent) { |
Contributor
There was a problem hiding this comment.
The new method is internal so directly testable. Worth adding a few cases to test the new behaviour?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Goal
Solves the gap where:
pinned participant leaves
joins again with another session
existing participants have obsolete pins information, and render the user unpinned
ParticipantJoinedSFU event now will carryis_pinnedproperty so you can use it for updating the client-side state correctlyImplementation
React PR
is_pinnedin ParticipantJoinedEventCore logic
There are auto generated files, which are not the core part of this PR
🎨 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:
Root Cause:
JoinCallResponseEventcontains a different sessionIdStatus: Failed (expected due to known issue)
Step 3: User B rejoins flow
State on User C
on User Cstill sees User B pinnedStatus: Passed ✅
Notes
Summary