feat(llc): handle isPinned in joinParticipant event#1220
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds server-driven participant pinning support: protobuf schemas extended (participantJoined.isPinned, negotiationId fields), mappers and events carry isPinned, call-state initialization applies non-local pin when isPinned is true, and a changelog entry documents the feature. Changes
Sequence Diagram(s)sequenceDiagram
participant SFU as SFU Server
participant Mapper as Event Mapper
participant Domain as Call Events
participant StateMgr as State Manager
participant Store as Call State
SFU->>Mapper: ParticipantJoined(isPinned: true)
Mapper->>Domain: SfuParticipantJoinedEvent(isPinned: true)
Domain->>Domain: detect isPinned flag
alt isPinned == true
Domain->>StateMgr: sfuParticipantJoined(event with isPinned)
StateMgr->>StateMgr: create CallParticipantState with pin\n(CallParticipantPin{isLocalPin:false, pinnedAt: now})
StateMgr->>Store: update participants with pinned state
else isPinned == false
Domain->>StateMgr: sfuParticipantJoined(event)
StateMgr->>Store: update participants normally
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 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 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/stream_video/lib/protobuf/video/sfu/signal_rpc/signal.pb.dart (1)
1553-1643:⚠️ Potential issue | 🟠 MajorWire
negotiationIdthrough the answer flow.The generated protobuf models now include
negotiationIdin bothSubscriberOffer(incoming) andSendAnswerRequest(outgoing), but the intermediate layers haven't been updated:SfuSubscriberOfferEventlacks the field, the event mapper doesn't extract it,_onSubscriberOffer()doesn't pass it to the request, andSendAnswerRequestX.toJson()omits it. This means the new field will be unused end-to-end, breaking SFU answer correlation if the backend expects it.Add
negotiationIdtoSfuSubscriberOfferEvent, extract it in the mapper fromSubscriberOffer, pass it when constructingsfu.SendAnswerRequest(...), and include it inSendAnswerRequestX.toJson():Suggested updates
extension SendAnswerRequestX on sfu.SendAnswerRequest { Map<String, dynamic> toJson() { return { 'peerType': peerType.value, 'sdp': sdp, 'sessionId': sessionId, + if (hasNegotiationId()) 'negotiationId': negotiationId, }; } }Also propagate
negotiationIdthroughSfuSubscriberOfferEvent, the event mapper, and the request builder.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stream_video/lib/protobuf/video/sfu/signal_rpc/signal.pb.dart` around lines 1553 - 1643, The new negotiationId field must be propagated end-to-end: add a negotiationId (int?) property to SfuSubscriberOfferEvent, update the event mapper that converts SubscriberOffer -> SfuSubscriberOfferEvent to extract SubscriberOffer.negotiationId, update _onSubscriberOffer to include event.negotiationId when constructing the outgoing sfu.SendAnswerRequest, and modify SendAnswerRequestX.toJson() to include negotiationId in the serialized map; ensure you reference the protobuf types SubscriberOffer and sfu.SendAnswerRequest and the classes/methods SfuSubscriberOfferEvent, the event mapper function, _onSubscriberOffer, and SendAnswerRequestX so the new field is wired through all layers.packages/stream_video/lib/src/models/call_participant_state.dart (1)
19-24:⚠️ Potential issue | 🟠 MajorProvide a warning-free migration path for the deprecated
customparameter.The
customparameter is deprecated but still required in the public constructor, which forces external callers to continue passing it explicitly even when migrating tocustomData. This prevents clean deprecation for library users. Makecustomoptional with a default value in the public constructor to allow callers to stop using the deprecated parameter. The private constructor can keep it required without the deprecation annotation since it's internal.Proposed adjustment
CallParticipantState({ required this.userId, required this.roles, required this.name, - `@Deprecated`('Use customData instead') required this.custom, + `@Deprecated`('Use customData instead') this.custom = const {}, this.customData = const {}, this.image, required this.sessionId, @@ CallParticipantState._({ required this.userId, required this.roles, required this.name, - `@Deprecated`('Use customData instead') required this.custom, + required this.custom, required this.customData,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stream_video/lib/src/models/call_participant_state.dart` around lines 19 - 24, The public CallParticipantState constructor currently requires the deprecated parameter custom; make the deprecated custom parameter optional with a sensible default (e.g. an empty map/const {}) so callers can stop passing it and migrate to customData without warnings, while leaving the private/internal CallParticipantState._ (or any private constructor overloads) unchanged (they can keep custom required) and update any forwarding/redirecting constructors or factory methods that call the public constructor to rely on the new optional default for custom; ensure the `@Deprecated` annotation remains on the public parameter so users see the deprecation but are not forced to supply the value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/stream_video/lib/protobuf/video/sfu/signal_rpc/signal.pb.dart`:
- Around line 1553-1643: The new negotiationId field must be propagated
end-to-end: add a negotiationId (int?) property to SfuSubscriberOfferEvent,
update the event mapper that converts SubscriberOffer -> SfuSubscriberOfferEvent
to extract SubscriberOffer.negotiationId, update _onSubscriberOffer to include
event.negotiationId when constructing the outgoing sfu.SendAnswerRequest, and
modify SendAnswerRequestX.toJson() to include negotiationId in the serialized
map; ensure you reference the protobuf types SubscriberOffer and
sfu.SendAnswerRequest and the classes/methods SfuSubscriberOfferEvent, the event
mapper function, _onSubscriberOffer, and SendAnswerRequestX so the new field is
wired through all layers.
In `@packages/stream_video/lib/src/models/call_participant_state.dart`:
- Around line 19-24: The public CallParticipantState constructor currently
requires the deprecated parameter custom; make the deprecated custom parameter
optional with a sensible default (e.g. an empty map/const {}) so callers can
stop passing it and migrate to customData without warnings, while leaving the
private/internal CallParticipantState._ (or any private constructor overloads)
unchanged (they can keep custom required) and update any forwarding/redirecting
constructors or factory methods that call the public constructor to rely on the
new optional default for custom; ensure the `@Deprecated` annotation remains on
the public parameter so users see the deprecation but are not forced to supply
the value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fd6b996f-1576-41ad-97e4-a260e7522592
📒 Files selected for processing (10)
packages/stream_video/CHANGELOG.mdpackages/stream_video/lib/protobuf/video/sfu/event/events.pb.dartpackages/stream_video/lib/protobuf/video/sfu/event/events.pbjson.dartpackages/stream_video/lib/protobuf/video/sfu/signal_rpc/signal.pb.dartpackages/stream_video/lib/protobuf/video/sfu/signal_rpc/signal.pbjson.dartpackages/stream_video/lib/src/call/call_events.dartpackages/stream_video/lib/src/call/state/mixins/state_sfu_mixin.dartpackages/stream_video/lib/src/models/call_participant_state.dartpackages/stream_video/lib/src/sfu/data/events/sfu_event_mapper_extensions.dartpackages/stream_video/lib/src/sfu/data/events/sfu_events.dart
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1220 +/- ##
========================================
- Coverage 5.77% 5.77% -0.01%
========================================
Files 676 676
Lines 49296 49335 +39
========================================
+ Hits 2849 2850 +1
- Misses 46447 46485 +38 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
Summary by CodeRabbit
New Features
Deprecations
customfield on CallParticipantState is deprecated — usecustomDatainstead.Documentation