fix(llc): ongoing call ringing fixes#1250
Conversation
|
Warning Review limit reached
More reviews will be available in 54 minutes and 51 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughRefactors the dogfooding call participants screen to separate participants and non-participants, adds Share/Add/Ring actions and tile widgets; scopes coordinator rejection auto-disconnect to the ringing flow; ends native push calls on accept failure; and adds tests, a models export, changelog updates, and CI threshold tweaks. ChangesCall Participants Screen UI Redesign
Call Accept Failure Cleanup & Exports
Ringing Flow State Guard
Tests, Fixtures, Changelog, and CI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/stream_video/lib/src/stream_video.dart (1)
779-804:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle native-call teardown on all post-accept failures and make teardown non-throwing.
Line 779 and Line 924 can still exit after native accept without ending the native call; teardown currently only runs on Line 802/Line 947. Also,
endCallByCidshould be wrapped so a platform error does not escape this failure path.Suggested patch
@@ Future<bool> consumeAndAcceptActiveCall({ @@ if (callResult.isFailure) { _logger.d( () => '[consumeAndAcceptActiveCall] error consuming incoming call: ' '${callResult.getErrorOrNull()}', ); + await _safeEndNativeCallByCid( + calls.first.callCid!, + context: 'consumeAndAcceptActiveCall', + ); return false; } @@ final call = callResult.getDataOrNull(); - if (call == null) return false; + if (call == null) { + await _safeEndNativeCallByCid( + calls.first.callCid!, + context: 'consumeAndAcceptActiveCall', + ); + return false; + } @@ - await pushNotificationManager?.endCallByCid(call.callCid.value); + await _safeEndNativeCallByCid( + call.callCid.value, + context: 'consumeAndAcceptActiveCall', + ); return false; } @@ Future<void> _onCallAccept( @@ if (consumeResult.isFailure) { _logger.w( () => '[onCallAccept] error consuming incoming call: ${consumeResult.getErrorOrNull()}', ); + await _safeEndNativeCallByCid(cid, context: 'onCallAccept'); return; } @@ final callToJoin = consumeResult.getDataOrNull(); - if (callToJoin == null) return; + if (callToJoin == null) { + await _safeEndNativeCallByCid(cid, context: 'onCallAccept'); + return; + } @@ - await pushNotificationManager?.endCallByCid(cid); + await _safeEndNativeCallByCid(cid, context: 'onCallAccept'); return; } @@ + Future<void> _safeEndNativeCallByCid( + String cid, { + required String context, + }) async { + try { + await pushNotificationManager?.endCallByCid(cid); + } catch (e, stk) { + _logger.w( + () => '[$context] failed to end native call for cid=$cid: $e', + e, + stk, + ); + } + }Also applies to: 924-949
🤖 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 `@packages/stream_video/lib/src/stream_video.dart` around lines 779 - 804, After accepting a native call, ensure native-call teardown always runs and cannot throw: in consumeAndAcceptActiveCall (the code paths that check callResult/isFailure and acceptResult/isFailure and any other early returns after call.accept()), call pushNotificationManager?.endCallByCid(call.callCid.value) before every return that follows a successful native accept; wrap that endCallByCid invocation in a try/catch (or otherwise swallow platform errors) so teardown failures do not propagate and the function still returns false as intended.
🤖 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 `@dogfooding/lib/screens/call_participants_list.dart`:
- Around line 273-276: The Text widget currently renders raw participant.name
which can be empty; change it to use the same fallback as the avatar by
rendering participant.name when non-empty otherwise participant.userId (e.g.
replace participant.name with something like participant.name?.isNotEmpty ==
true ? participant.name : participant.userId) so the label never appears blank
in the participant tile.
- Around line 132-140: The share sheet anchor is being taken from the
StreamBuilder's context instead of the actual "Share link" button, so change
_shareLink to use the button's RenderBox: add a GlobalKey (or wrap the "Share
link" button in a RepaintBoundary with a key) on the StreamButton widget, use
that key to get the RenderBox (key.currentContext!.findRenderObject() as
RenderBox) and compute origin = box.localToGlobal(Offset.zero) & box.size, then
pass that origin into ShareParams.sharePositionOrigin; update any callers of
_shareLink to reference the keyed widget if needed.
---
Outside diff comments:
In `@packages/stream_video/lib/src/stream_video.dart`:
- Around line 779-804: After accepting a native call, ensure native-call
teardown always runs and cannot throw: in consumeAndAcceptActiveCall (the code
paths that check callResult/isFailure and acceptResult/isFailure and any other
early returns after call.accept()), call
pushNotificationManager?.endCallByCid(call.callCid.value) before every return
that follows a successful native accept; wrap that endCallByCid invocation in a
try/catch (or otherwise swallow platform errors) so teardown failures do not
propagate and the function still returns false as intended.
🪄 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: 32d1325b-c27d-43ea-9beb-d754bf661137
📒 Files selected for processing (5)
dogfooding/lib/screens/call_participants_list.dartpackages/stream_video/CHANGELOG.mdpackages/stream_video/lib/src/call/state/mixins/state_coordinator_mixin.dartpackages/stream_video/lib/src/models/models.dartpackages/stream_video/lib/src/stream_video.dart
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1250 +/- ##
========================================
+ Coverage 9.68% 9.71% +0.03%
========================================
Files 676 676
Lines 49689 49695 +6
========================================
+ Hits 4810 4826 +16
+ Misses 44879 44869 -10 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/pana.yml (1)
27-36: ⚡ Quick winConsider adding explicit permissions blocks for security hardening.
The static analysis tool flags these jobs for using default permissions. Following GitHub Actions security best practices, explicitly declare minimum required permissions:
🔒 Recommended permissions blocks
For both
stream_video_flutterandstream_video_push_notificationjobs, add:stream_video_flutter: runs-on: ubuntu-latest + permissions: + contents: read steps:stream_video_push_notification: runs-on: ubuntu-latest + permissions: + contents: read steps:This makes the security posture explicit and follows the principle of least privilege. Consider applying this pattern to all jobs in the workflow.
As per static analysis hints: zizmor flags "overly broad permissions (excessive-permissions): default permissions used due to no permissions: block" for these job definitions.
Also applies to: 38-47
🤖 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 @.github/workflows/pana.yml around lines 27 - 36, Add explicit minimal permissions blocks to the job definitions (e.g., stream_video_flutter and stream_video_push_notification) to avoid using default workspace permissions; update each job (the job blocks named stream_video_flutter and stream_video_push_notification) to declare only the required GitHub Actions permissions (for example, at minimum contents: read and actions: read, plus any additional scopes the custom ./.github/actions/pana action actually needs) instead of relying on defaults so the workflow follows least-privilege security best practices.
🤖 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.
Nitpick comments:
In @.github/workflows/pana.yml:
- Around line 27-36: Add explicit minimal permissions blocks to the job
definitions (e.g., stream_video_flutter and stream_video_push_notification) to
avoid using default workspace permissions; update each job (the job blocks named
stream_video_flutter and stream_video_push_notification) to declare only the
required GitHub Actions permissions (for example, at minimum contents: read and
actions: read, plus any additional scopes the custom ./.github/actions/pana
action actually needs) instead of relying on defaults so the workflow follows
least-privilege security best practices.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8b54c27c-1c7e-4aab-942a-bfe08f7096b2
📒 Files selected for processing (3)
.github/workflows/pana.ymldogfooding/lib/screens/call_participants_list.dartpackages/stream_video/test/src/call/call_ring_test.dart
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/stream_video/test/src/call/call_ring_test.dart
- dogfooding/lib/screens/call_participants_list.dart
xsahil03x
left a comment
xsahil03x
left a comment
There was a problem hiding this comment.
Added some comments, but LGTM ✅
2 participants
Added option to add a call member and ring them during the ongoing call in Dogfooding app.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation