regression: invalid call states when call initialization signal is missed #37101
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
WalkthroughExpanded hidden-state criteria, added early exit when a call is already over, refined pre-remote signal handling to immediately hang up on hangup notifications, and added guards in initialization and signal processing to prevent work when the call is over or not ready. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User/Remote
participant C as Call
participant R as RemoteData
participant Q as SignalQueue
Note over C: Initialization
U->>C: initializeRemoteCall()
alt Call is over
C-->>U: exit (no further init)
else Not over
C->>C: set hidden if uninitialized/ignored
C->>R: ensure basic info
end
Note over C: Signal processing
U->>C: processSignal(signal)
alt Call is over or not ready
C-->>U: skip processing (fill basics only)
else Remote not present
alt signal is hangup
C->>C: immediate hangup()
else other signal
C->>Q: enqueue(signal)
end
else Remote present
C->>C: normal processing path
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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: 0
🧹 Nitpick comments (1)
packages/media-signaling/src/lib/Call.ts (1)
435-439: LGTM! Immediate hangup on pre-initialization hangup notifications is correct.This change appropriately handles the edge case where a hangup notification arrives before call initialization data. By immediately ending the call instead of queuing the signal, it prevents the call from remaining in limbo waiting for data that's no longer needed.
Optional: Consider clearing
this.earlySignalswhen immediately hanging up, since queued signals will never be processed:if (signal.type === 'notification' && signal.notification === 'hangup') { this.changeState('hangup'); + this.earlySignals.clear(); return; }This isn't critical since
processSignalalready guards against processing whenisOver()is true (line 419), but it would make the intent clearer and free memory sooner.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/media-signaling/src/lib/Call.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: 📦 Build Packages
- GitHub Check: Builds matrix rust bindings against alpine
🔇 Additional comments (2)
packages/media-signaling/src/lib/Call.ts (2)
92-100: LGTM! This correctly prevents uninitialized calls from appearing in the UI.The expanded hidden state logic appropriately addresses the root cause described in the PR: calls that miss initialization signals will now remain hidden instead of appearing with incomplete data. The added documentation clearly explains the three conditions for a hidden call.
277-280: LGTM! Appropriate guard against processing already-ended calls.This early return correctly prevents unnecessary initialization work (WebRTC setup, acknowledgment, timeouts) for calls that are already over, while still populating basic call metadata. This aligns with the fix objective of ensuring calls are properly marked as ended even without initialization.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release-7.11.0 #37101 +/- ##
===============================================
Coverage 67.39% 67.39%
===============================================
Files 3328 3328
Lines 113341 113353 +12
Branches 20564 20585 +21
===============================================
+ Hits 76387 76397 +10
- Misses 34348 34351 +3
+ Partials 2606 2605 -1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Proposed changes (including videos or screenshots)
If a client misses a call initialization signal but receives some other signal about it, then that call is being added to the internal list of calls but never initialized and also never cleared. It doesn't trigger any events so it doesn't cause any problems in the UI, but if a different call happens, then this second call will trigger the session events and once it is over, it'll roll back to the first call even though it is not initialized.
This PR makes two fixes:
Issue(s)
VAI-149
Steps to test or reproduce
Further comments
Summary by CodeRabbit