chore(llc): improve reject and leave reasons#1218
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughCoordinator-driven accept/reject events are routed through new async handlers that can locally reject when another device acted; new CallRejectReason factories and DisconnectReason.callEnded() were added; SFU leave-reason mapping introduced; RingingEvent suppression added in coordinator termination paths; leave/reject logging adjusted. Changes
Sequence Diagram(s)sequenceDiagram
participant Coordinator
participant CallHandler as Call (call.dart)
participant StateMgr as StateManager / Session
participant SFU
participant Notifier as RingingEventBroadcaster
Coordinator->>CallHandler: StreamCallRejectedEvent / StreamCallAcceptedEvent
CallHandler->>CallHandler: await _handleCoordinatorCallRejected/_handleCoordinatorCallAccepted()
alt other-device response && CallStatusIncoming && not acceptedByMe
CallHandler->>CallHandler: log and create CallRejectReason.userRespondedElsewhere()
CallHandler->>StateMgr: coordinatorCallRejected(userRespondedElsewhere)
CallHandler->>Notifier: suppressEvent()
else normal flow
CallHandler->>StateMgr: coordinatorCallAccepted/coordinatorCallRejected(with factory reason)
end
StateMgr->>SFU: leave(sessionId, reason via _sfuLeaveReason)
SFU-->>StateMgr: left
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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/stream_video_push_notification/lib/src/stream_video_push_notification.dart (1)
62-116: Narrow the suppression scope to reduce cross-call interference risk.The blanket
RingingEventBroadcaster().suppressEvent()calls suppress allRingingEventtypes for 500 ms. While this may work for swallowing the native event immediately followingendCallByCid, any unrelated event from a different call arriving on the platform channel during that window (e.g.,ActionCallIncomingon another call) would also be silently dropped.If feasible, narrow suppression to the specific event type expected after
endCallByCid—ideallyActionCallEnded—to avoid cross-call interference:RingingEventBroadcaster().suppressEvent( eventType: ActionCallEnded, );This pattern is already supported by
suppressEventand shown in test cases (seeringing_event_broadcaster_test.dart). Worth confirming which nativeRingingEvent(s) are actually emitted afterendCallByCidon iOS and Android so the suppression can be scoped appropriately.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stream_video_push_notification/lib/src/stream_video_push_notification.dart` around lines 62 - 116, The current blanket RingingEventBroadcaster().suppressEvent() calls in the CoordinatorCallEndedEvent, CoordinatorCallSessionParticipantCountUpdatedEvent, and CoordinatorCallRejectedEvent handlers suppress all ringing events and can drop unrelated call events; update each suppression to narrow the scope by passing the specific native event type expected after ending a call (e.g. call RingingEventBroadcaster().suppressEvent(eventType: ActionCallEnded)) right before calling endCallByCid(event.callCid.toString()), and confirm/adjust the eventType if the platform emits a different RingingEvent for end-call on iOS/Android.
🤖 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/stream_video/lib/src/call/call.dart`:
- Around line 650-688: The current early-return branches call reject() for
events originating from the same user on another device, which incorrectly sends
a coordinator reject; instead, remove the await reject(...) calls in
_handleCoordinatorCallAccepted and _handleCoordinatorCallRejected and, in those
branches, only update local state by invoking the coordinator handlers directly
(call _stateManager.coordinatorCallAccepted(event) in the accepted branch and
_stateManager.coordinatorCallRejected(event) in the rejected branch) and then
return so we clear local ringing without sending a new coordinator reject.
---
Nitpick comments:
In
`@packages/stream_video_push_notification/lib/src/stream_video_push_notification.dart`:
- Around line 62-116: The current blanket
RingingEventBroadcaster().suppressEvent() calls in the
CoordinatorCallEndedEvent, CoordinatorCallSessionParticipantCountUpdatedEvent,
and CoordinatorCallRejectedEvent handlers suppress all ringing events and can
drop unrelated call events; update each suppression to narrow the scope by
passing the specific native event type expected after ending a call (e.g. call
RingingEventBroadcaster().suppressEvent(eventType: ActionCallEnded)) right
before calling endCallByCid(event.callCid.toString()), and confirm/adjust the
eventType if the platform emits a different RingingEvent for end-call on
iOS/Android.
🪄 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: c55ebc72-4e2f-4297-92c4-71cd854bbef5
📒 Files selected for processing (9)
packages/stream_video/CHANGELOG.mdpackages/stream_video/lib/src/call/call.dartpackages/stream_video/lib/src/call/call_reject_reason.dartpackages/stream_video/lib/src/call/session/call_session.dartpackages/stream_video/lib/src/call/state/mixins/state_coordinator_mixin.dartpackages/stream_video/lib/src/models/disconnect_reason.dartpackages/stream_video/lib/src/stream_video.dartpackages/stream_video_flutter/CHANGELOG.mdpackages/stream_video_push_notification/lib/src/stream_video_push_notification.dart
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/stream_video/lib/src/call/call.dart (1)
656-665:⚠️ Potential issue | 🟠 MajorDon't send a second coordinator reject for same-user multi-device events.
Lines 664 and 685 still call
reject(), which sends_coordinatorClient.rejectCall(...). At that point the coordinator event is already authoritative, so this path should only clear the local ringing state; otherwise a same-user accept/reject on another device can be followed by a contradictory extra reject from this device.Suggested fix
if (event.acceptedByUserId == currentUserId && status is CallStatusIncoming && !status.acceptedByMe) { _logger.i( () => '[onCoordinatorEvent] call accepted on another device, ' 'rejecting locally with userRespondedElsewhere', ); - await reject(reason: CallRejectReason.userRespondedElsewhere()); + _stateManager.coordinatorCallAccepted(event); return; } @@ if (event.rejectedByUserId == currentUserId && status is CallStatusIncoming && !status.acceptedByMe) { _logger.i( () => '[onCoordinatorEvent] call rejected on another device, ' 'rejecting locally with userRespondedElsewhere', ); - await reject(reason: CallRejectReason.userRespondedElsewhere()); + _stateManager.coordinatorCallRejected(event); return; }Also applies to: 677-686
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stream_video/lib/src/call/call.dart` around lines 656 - 665, The handler is sending a coordinator reject after the coordinator already signaled acceptance on another device; instead of calling reject() (which triggers _coordinatorClient.rejectCall), simply clear the local ringing/incoming state and return. Locate the branches that check event.acceptedByUserId == currentUserId and status is CallStatusIncoming && !status.acceptedByMe (and the similar branch for rejected-by-same-user) and remove the reject(reason: CallRejectReason.userRespondedElsewhere()) / reject(...) calls; replace them with code that updates local state only (e.g., mark the incoming/ringing state as cleared or update the CallStatus locally) and keep the existing _logger.i call and return. Ensure no coordinator RPC is invoked from these branches by referencing the reject() method and CallRejectReason.* usages.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/stream_video/lib/src/call/call.dart`:
- Around line 656-665: The handler is sending a coordinator reject after the
coordinator already signaled acceptance on another device; instead of calling
reject() (which triggers _coordinatorClient.rejectCall), simply clear the local
ringing/incoming state and return. Locate the branches that check
event.acceptedByUserId == currentUserId and status is CallStatusIncoming &&
!status.acceptedByMe (and the similar branch for rejected-by-same-user) and
remove the reject(reason: CallRejectReason.userRespondedElsewhere()) /
reject(...) calls; replace them with code that updates local state only (e.g.,
mark the incoming/ringing state as cleared or update the CallStatus locally) and
keep the existing _logger.i call and return. Ensure no coordinator RPC is
invoked from these branches by referencing the reject() method and
CallRejectReason.* usages.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5def9871-6300-4780-a76b-c6dbc1e44a24
📒 Files selected for processing (1)
packages/stream_video/lib/src/call/call.dart
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1218 +/- ##
========================================
+ Coverage 5.79% 5.83% +0.03%
========================================
Files 676 676
Lines 49345 49394 +49
========================================
+ Hits 2862 2880 +18
- Misses 46483 46514 +31 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/stream_video/lib/src/call/call.dart (1)
656-665:⚠️ Potential issue | 🟠 MajorDo not echo a coordinator reject after another device already acted.
Both branches still call
reject(), andreject()sends_coordinatorClient.rejectCall(...). When the coordinator event is already the authoritative accept/reject from the same user on another device, this issues a second, contradictory reject instead of only clearing local state.🐛 Proposed fix
if (event.acceptedByUserId == currentUserId && status is CallStatusIncoming && !status.acceptedByMe) { @@ - await reject(reason: CallRejectReason.userRespondedElsewhere()); + await leave( + reason: DisconnectReason.rejected( + byUserId: currentUserId, + reason: CallRejectReason.userRespondedElsewhere(), + ), + ); return; } @@ if (event.rejectedByUserId == currentUserId && status is CallStatusIncoming && !status.acceptedByMe) { @@ - await reject(reason: CallRejectReason.userRespondedElsewhere()); + await leave( + reason: DisconnectReason.rejected( + byUserId: currentUserId, + reason: CallRejectReason.userRespondedElsewhere(), + ), + ); return; }Also applies to: 677-686
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stream_video/lib/src/call/call.dart` around lines 656 - 665, When handling coordinator events where the authoritative accept/reject already came from the same user on another device (the branches checking event.acceptedByUserId == currentUserId with status is CallStatusIncoming && !status.acceptedByMe, and the analogous reject branch), do NOT call reject() (which invokes _coordinatorClient.rejectCall(...)); instead only update/clear local state and emit the local status change (e.g., mark the incoming status as handled or set the local accepted/rejected flag) and return. Locate the logic in onCoordinatorEvent around the checks referencing currentUserId, CallStatusIncoming, and CallRejectReason.userRespondedElsewhere() and remove the secondary call to reject(); replace it with local-only cleanup/update code so no duplicate coordinator RPC is sent.
🤖 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/stream_video/lib/src/call/call.dart`:
- Around line 2081-2102: The _sfuLeaveReason function currently falls back to a
generic message for some DisconnectReason variants; explicitly handle
DisconnectReasonBlocked, DisconnectReasonCancelled, and
DisconnectReasonManuallyClosed (or the enum/constructors named blocked,
cancelled, manuallyClosed) inside _sfuLeaveReason to return specific messages
(e.g., "blocked", "cancelled", "manually closed") instead of the generic 'user
is leaving the call', ensuring those reasons are preserved when passed into
leave().
---
Duplicate comments:
In `@packages/stream_video/lib/src/call/call.dart`:
- Around line 656-665: When handling coordinator events where the authoritative
accept/reject already came from the same user on another device (the branches
checking event.acceptedByUserId == currentUserId with status is
CallStatusIncoming && !status.acceptedByMe, and the analogous reject branch), do
NOT call reject() (which invokes _coordinatorClient.rejectCall(...)); instead
only update/clear local state and emit the local status change (e.g., mark the
incoming status as handled or set the local accepted/rejected flag) and return.
Locate the logic in onCoordinatorEvent around the checks referencing
currentUserId, CallStatusIncoming, and CallRejectReason.userRespondedElsewhere()
and remove the secondary call to reject(); replace it with local-only
cleanup/update code so no duplicate coordinator RPC is sent.
🪄 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: 7f90eb69-9e51-49e9-a593-47f7fd41129c
📒 Files selected for processing (3)
packages/stream_video/CHANGELOG.mdpackages/stream_video/lib/src/call/call.dartpackages/stream_video/lib/src/call/session/call_session.dart
✅ Files skipped from review due to trivial changes (2)
- packages/stream_video/CHANGELOG.md
- packages/stream_video/lib/src/call/session/call_session.dart
2 participants
Summary by CodeRabbit
Bug Fixes
Documentation