regression: Hangup button not working on clients other than the one where the call is happening #37151
Conversation
…e call is happening
|
Looks like this PR is ready to merge! 🎉 |
|
WalkthroughAdds a new hangup reason 'another-client', updates schemas/types accordingly, adjusts client-side hangup flow to send an 'another-client' hangup when a normal hangup occurs while the contract is ignored, and updates server-side signal processing to skip contract validation for 'another-client' hangups. Also adds a clarifying comment in call signal processing. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as Client UI
participant Call as ClientMediaCall
participant Tx as Transporter
participant S as Server
participant SP as SignalProcessor
participant OC as Other Client(s)
rect rgba(220,240,255,0.5)
note over Call: New/changed flow when contract is "ignored"
UI->>Call: hangup(reason="normal")
alt contract is "ignored"
Call->>Tx: send hangup(reason="another-client")
Tx-->>S: ClientMediaSignalHangup
S->>SP: process hangup
note over SP: skipContractCheck for "another-client"
SP-->>OC: propagate hangup
Call-->>UI: return (early)
else normal flow
Call->>Tx: send hangup(reason="normal")
Tx-->>S: ClientMediaSignalHangup
S->>SP: process hangup (normal checks)
SP-->>OC: propagate hangup
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ 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: 0
🧹 Nitpick comments (3)
ee/packages/media-calls/src/internal/SignalProcessor.ts (1)
89-94: Bypass looks correct; add audit log for traceability.The skip is safe given the earlier actor check (Lines 71–87). Consider logging when skipping the contract check to aid auditing/debugging.
Suggested tweak:
// Hangup requests from different clients won't be coming from the signed client const skipContractCheck = signal.type === 'hangup' && signal.reason === 'another-client'; // Ignore signals from different sessions if the actor is already signed -if (!skipContractCheck && callActor.contractId && callActor.contractId !== signal.contractId) { +if (!skipContractCheck && callActor.contractId && callActor.contractId !== signal.contractId) { return; } +if (skipContractCheck) { + logger.info({ + msg: 'Processing hangup from another client session', + callId: call._id, + uid, + actorRole: role, + actorContractId: callActor.contractId, + signalContractId: signal.contractId, + }); +}packages/media-signaling/src/lib/Call.ts (1)
518-523: Debounce 'another-client' hangup to avoid duplicate sends.Multiple quick clicks can send repeated hangups before server ends the call. Set a local guard:
-// If the hangup was requested by the user but the call is not happening here, send an 'another-client' hangup request to the server and wait for the server to hangup the call -if (reason === 'normal' && this.contractState === 'ignored') { - this.config.transporter.hangup(this.callId, 'another-client'); - return; -} +// If the hangup was requested by the user but the call is not happening here, +// send an 'another-client' hangup request and avoid repeated sends +if (reason === 'normal' && this.contractState === 'ignored') { + this.endedLocally = true; + this.config.transporter.hangup(this.callId, 'another-client'); + return; +}packages/media-signaling/src/definition/signals/client/hangup.ts (1)
43-47: Schema sync looks good; add cross‑ref comment.Enum matches CallHangupReason updates. Consider adding a note to keep this list in sync with IClientMediaCall.ts to avoid drift.
📜 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 (5)
ee/packages/media-calls/src/internal/SignalProcessor.ts(1 hunks)ee/packages/media-calls/src/internal/agents/CallSignalProcessor.ts(1 hunks)packages/media-signaling/src/definition/call/IClientMediaCall.ts(2 hunks)packages/media-signaling/src/definition/signals/client/hangup.ts(1 hunks)packages/media-signaling/src/lib/Call.ts(1 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). (4)
- GitHub Check: 📦 Build Packages
- GitHub Check: Builds matrix rust bindings against alpine
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (2)
ee/packages/media-calls/src/internal/agents/CallSignalProcessor.ts (1)
85-90: Good documentation update.The added scenario note aligns with the server-side skip for 'another-client' hangups.
packages/media-signaling/src/definition/call/IClientMediaCall.ts (1)
29-44: Type addition is correct and consistent.'another-client' reason is properly added and documented to sync with schema.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release-7.11.0 #37151 +/- ##
==================================================
+ Coverage 67.41% 67.44% +0.03%
==================================================
Files 3329 3329
Lines 113383 113383
Branches 20569 20575 +6
==================================================
+ Hits 76433 76471 +38
+ Misses 34351 34318 -33
+ Partials 2599 2594 -5
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
pierre-lehnen-rc
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
Proposed changes (including videos or screenshots)
Issue(s)
VAI-152
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Bug Fixes
Documentation