fix(client): support server-side pinning on participant join#2190
fix(client): support server-side pinning on participant join#2190
Conversation
Apply a server-side pin when a participant joins with isPinned flag set by the SFU. Updated SFU protobuf types to include the new isPinned field on ParticipantJoined, SendMetrics RPC, and negotiationId on SendAnswerRequest.
|
📝 WalkthroughWalkthroughAdded server-side pin state management for participants upon join events. When a participant joins with Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/client/src/events/__tests__/participant.test.ts (1)
78-98: Add regression coverage for join-pin → server-pin reconciliationThese tests verify
isPinnedon join, but they don’t cover the follow-up case where server pin state is reconciled (e.g., viasetServerSidePins). Please add a regression test that joins withisPinned: true, then applies server pins, and asserts the participant keeps a server-side pin with server-authoritative ordering semantics.Based on learnings: "Add regression tests for bug fixes".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/client/src/events/__tests__/participant.test.ts` around lines 78 - 98, Add a regression test that uses CallState and watchParticipantJoined to simulate a join with isPinned: true, then call the state method that applies server pins (e.g., setServerSidePins) with a server-authoritative pin list for that participant's userId/sessionId, and assert that findParticipantBySessionId still shows a pin that is server-side (pin.isLocalPin === false) and that the pin's timestamp/ordering reflects the server-provided value (e.g., pinnedAt equals or is derived from the server pin entry) to validate server-pin reconciliation keeps server-authoritative ordering.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/client/src/events/participant.ts`:
- Line 41: The join-time pin currently sets pin.pinnedAt = Date.now() which
blocks later server-authoritative updates; change the participant join logic
(the spread creating pin in participant.ts) to avoid seeding a numeric pinnedAt
(omit or set to undefined) for non-local pins, and update
CallState.setServerSidePins to accept and overwrite pins when the incoming
server timestamp is newer or when the existing pinnedAt is "unset" (undefined)
while still preserving isLocalPin precedence (do not overwrite if
existing.isLocalPin === true). Also implement the timestamp/instance-id
comparison guard in setServerSidePins so server updates win only when
serverTimestamp > existingTimestamp (or existing is unset) to prevent race
conditions.
---
Nitpick comments:
In `@packages/client/src/events/__tests__/participant.test.ts`:
- Around line 78-98: Add a regression test that uses CallState and
watchParticipantJoined to simulate a join with isPinned: true, then call the
state method that applies server pins (e.g., setServerSidePins) with a
server-authoritative pin list for that participant's userId/sessionId, and
assert that findParticipantBySessionId still shows a pin that is server-side
(pin.isLocalPin === false) and that the pin's timestamp/ordering reflects the
server-provided value (e.g., pinnedAt equals or is derived from the server pin
entry) to validate server-pin reconciliation keeps server-authoritative
ordering.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 106dd125-b547-44a4-bbd4-f26995e0d80f
⛔ Files ignored due to path filters (2)
packages/client/src/gen/video/sfu/event/events.tsis excluded by!**/gen/**packages/client/src/gen/video/sfu/signal_rpc/signal.tsis excluded by!**/gen/**
📒 Files selected for processing (2)
packages/client/src/events/__tests__/participant.test.tspackages/client/src/events/participant.ts
💡 Overview
Support server-side pinning when a participant joins a call. When the SFU signals
isPinned: trueon aParticipantJoinedevent, the participant is automatically pinned withisLocalPin: false.📝 Implementation notes
watchParticipantJoinedto apply a server-side pin ({ isLocalPin: false, pinnedAt: Date.now() }) whene.isPinnedis truthyisPinned: true, another verifying no pin is set whenisPinned: falseRef: GetStream/protocol#1772
Summary by CodeRabbit
New Features
Tests