chore: New Backend Architecture for Voice Calls #36718
Conversation
![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
📜 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/freeswitch/src/eventParser/parseChannelUsername.ts(2 hunks)
🧰 Additional context used
🪛 GitHub Check: CodeQL
packages/freeswitch/src/eventParser/parseChannelUsername.ts
[failure] 31-31: Polynomial regular expression used on uncontrolled data
This regular expression that depends on library input may run slow on strings starting with 'sofia/' and with many repetitions of 'sofia/'.
⏰ 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). (1)
- GitHub Check: 🚢 Build Docker Images for Testing (alpine)
🔇 Additional comments (1)
packages/freeswitch/src/eventParser/parseChannelUsername.ts (1)
31-31: Anchor and tighten the regex to avoid misses and quell CodeQL’s regex warning.Current pattern uses
\S+and isn’t anchored; it can over‑consume and trigger backtracking on long inputs. Use a start anchor and restrict the profile to[^/]+. Also reuse a precompiled constant.Apply:
-return channelName.match(/sofia\/\S+\/(\d+)[\@\-]/)?.[1]; +return SOFIA_USERNAME_RE.exec(channelName)?.[1];Add near the imports:
const SOFIA_USERNAME_RE = /^sofia\/[^/]+\/(\d+)(?:[@-])/;This matches
sofia/<profile>/<digits>followed by@or-, e.g.,sofia/internal/1001@hostandsofia/internal/1000-XYZ@host, and should satisfy the CodeQL alert.Run a quick check:
#!/bin/bash python - << 'PY' import re pat = re.compile(r'^sofia/[^/]+/(\d+)(?:[@-])') tests = [ 'sofia/internal/1001@voip.example:5060', 'sofia/internal/1000-ABC123@rc.example', 'sofia/external/200@carrier.example', 'sofia/internal/1001', # no terminator -> None 'loopback/xyz', # filtered earlier -> None ] for s in tests: m = pat.match(s) print(f"{s} -> {m.group(1) if m else None}") PY
KevLehman
left a comment
KevLehman
left a comment
There was a problem hiding this comment.
Smol things, but one question: do we really need all these logs? 🙈
At least until its out of Beta, yes. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ee/packages/media-calls/src/server/CallDirector.ts (1)
137-141: Contract validation is incorrect (type mismatch) — will always fail for callee and likely fails type‑check.
getMyCallActor()returnsMediaCallContact(nocontractId). Accessingactor.contractIdis a type error and, at runtime, the check will fail for the callee (who isn’t signed). Validate against the signed party on the call instead.Apply:
- const actor = fromAgent.getMyCallActor(call); - if (!actor.contractId || actor.contractId !== contractId) { - throw new Error('invalid-contract'); - } + // Only the caller is signed on the call record; validate when applicable + if (fromAgent.role === 'caller' && call.caller.contractId !== contractId) { + throw new Error('invalid-contract'); + }
🧹 Nitpick comments (8)
ee/packages/media-calls/src/server/CallDirector.ts (2)
47-49: Await notification to avoid unhandled rejection and keep sequencing predictable.Returning the promise unawaited can leak errors to callers and reorder logs.
- this.scheduleExpirationCheckByCallId(call._id); - return actorAgent.oppositeAgent?.onCallActive(call._id); + this.scheduleExpirationCheckByCallId(call._id); + await actorAgent.oppositeAgent?.onCallActive(call._id);
349-372: Clear per‑call expiration timer on successful hangup to avoid dangling timeouts.Currently the timeout is only cleared when it fires. Free it immediately after a successful hangup.
const result = await MediaCalls.hangupCallById(callId, cleanedParams).catch((error) => { @@ }); - return Boolean(result.modifiedCount); + const modified = Boolean(result.modifiedCount); + if (modified) { + const h = scheduledExpirationChecks.get(callId); + if (h) { + clearTimeout(h); + scheduledExpirationChecks.delete(callId); + } + } + return modified;ee/packages/media-calls/src/sip/providers/IncomingSipCall.ts (4)
256-269: Map remote hangup (CANCEL) to 487 Request Terminated for pending INVITEs.Current default maps to 480, which is misleading for CANCEL semantics.
switch (call.hangupReason) { case 'service-error': this.cancelPendingInvites(SipErrorCodes.NOT_ACCEPTABLE_HERE); break; case 'rejected': this.cancelPendingInvites(SipErrorCodes.DECLINED); break; + case 'remote': + this.cancelPendingInvites(SipErrorCodes.REQUEST_TERMINATED); + break; default: this.cancelPendingInvites(SipErrorCodes.TEMPORARILY_UNAVAILABLE); break; }
324-334: Clean up served renegotiations to avoid memory growth.Negotiations remain in
inboundRenegotiationsafter replying 200 OK.localNegotiation.res.send(200, { body: localNegotiation.answer.sdp, }); + this.inboundRenegotiations.delete(localNegotiation.id);Also consider deleting the first negotiation after
createDialogsucceeds.
128-176: Await callee notification on renegotiation; fail fast on errors.Non‑awaited
onRemoteDescriptionChangedcan hide failures and race withres.send.- calleeAgent.onRemoteDescriptionChanged(this.call._id, negotiationId); + await calleeAgent.onRemoteDescriptionChanged(this.call._id, negotiationId);Optionally wrap in try/catch and send 5xx if it fails before replying.
284-301: Guard against stale negotiations and free resources.If
findOneByIdreturns null, you send 500 but keep the entry. Delete it to avoid repeated work.if (!negotiation) { logger.error({ msg: 'Invalid Negotiation reference on IncomingSipCall.', localNegotiation }); - this.inboundRenegotiations.delete(localNegotiation.id); + this.inboundRenegotiations.delete(localNegotiation.id); if (localNegotiation.res) { localNegotiation.res.send(SipErrorCodes.INTERNAL_SERVER_ERROR); } continue; }(Deletion already present — keep it; this is an approval of the approach.)
packages/media-signaling/src/lib/Call.ts (2)
799-799: Add context to error log for easier triageInclude a stable message with the error; raw object alone is harder to grep.
- this.config.logger?.error(e); + this.config.logger?.error('Failed to create WebRTC answer', e);
1036-1037: Log critical internal errors at error level; keep non‑critical at debugCritical errors should surface in prod logs without raising log volume for non‑critical ones.
- this.config.logger?.debug('ClientMediaCall.onWebRTCInternalError', critical, error); + if (critical) { + this.config.logger?.error('ClientMediaCall.onWebRTCInternalError', error); + } else { + this.config.logger?.debug('ClientMediaCall.onWebRTCInternalError', error); + }
📜 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 ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
ee/packages/media-calls/src/server/CallDirector.ts(19 hunks)ee/packages/media-calls/src/sip/providers/IncomingSipCall.ts(1 hunks)package.json(1 hunks)packages/media-signaling/src/lib/Call.ts(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- package.json
🧰 Additional context used
🧬 Code graph analysis (2)
ee/packages/media-calls/src/sip/providers/IncomingSipCall.ts (6)
ee/packages/media-calls/src/sip/Session.ts (2)
SipServerSession(17-199)sessionId(28-30)packages/core-typings/src/mediaCalls/IMediaCall.ts (4)
IMediaCall(35-63)MediaCallContact(28-28)MediaCallSignedContact(30-30)MediaCallContactInformation(22-26)ee/packages/media-calls/src/server/BroadcastAgent.ts (1)
BroadcastActorAgent(14-69)ee/packages/media-calls/src/sip/errorCodes.ts (2)
SipError(25-32)SipErrorCodes(1-23)packages/media-signaling/src/lib/services/states.ts (1)
isBusyState(9-11)packages/models/src/index.ts (1)
MediaCallNegotiations(188-188)
ee/packages/media-calls/src/server/CallDirector.ts (6)
packages/core-typings/src/mediaCalls/IMediaCall.ts (3)
IMediaCall(35-63)MediaCallContact(28-28)MediaCallSignedContact(30-30)ee/packages/media-calls/src/definition/IMediaCallAgent.ts (1)
IMediaCallAgent(4-26)packages/media-signaling/src/definition/call/IClientMediaCall.ts (2)
CallHangupReason(29-40)CallRole(17-17)ee/packages/media-calls/src/definition/common.ts (1)
MediaCallHeader(20-20)packages/core-typings/src/mediaCalls/IMediaCallNegotiation.ts (1)
IMediaCallNegotiation(3-15)ee/packages/media-calls/src/definition/IMediaCallCastDirector.ts (1)
IMediaCallCastDirector(7-32)
⏰ 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: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (3)
ee/packages/media-calls/src/sip/providers/IncomingSipCall.ts (2)
119-126: Mark response as sent immediately after 200 OK (prevents later erroneous replies).After
createUAS, later error paths may still try to respond to the initial INVITE.const uas = await this.srf.createUAS(this.req, this.res, { localSdp, }); @@ - this.sipDialog = uas; + // Flag to avoid re-sending responses on the original INVITE + (uas as any).responseSent = true; + this.sipDialog = uas;Also applies to: 184-185
58-63: Race: early CANCEL beforesipCallassignment drops the event.Buffer and replay CANCEL after instantiation.
- let sipCall: IncomingSipCall | null = null; + let sipCall: IncomingSipCall | null = null; + let pendingCancel: SipMessage | null = null; - req.on('cancel', (message) => { - sipCall?.cancel(message); - }); + req.on('cancel', (message) => { + pendingCancel = message; + sipCall?.cancel(message); + }); @@ - sipCall = new IncomingSipCall(session, call, callerAgent, channel, srf, req, res); + sipCall = new IncomingSipCall(session, call, callerAgent, channel, srf, req, res); + if (pendingCancel) { + sipCall.cancel(pendingCancel); + pendingCancel = null; + }Also applies to: 97-101
packages/media-signaling/src/lib/Call.ts (1)
633-642: Harden sendDTMF: enforce state guards, normalize tone, validate duration; use this.throwError
- Return early when the call is over or hidden (avoid emitting tones from non‑active/hidden sessions).
- Normalize tone: const tone = String(dtmf).trim().toUpperCase(); accept only 0‑9, A‑D, * and # (RFC 4733); reject ',' and other non‑DTMF characters.
- Validate duration: if provided, must be a finite number > 0; send Math.round(duration) or omit the field if invalid.
- Use this.throwError('invalid-dtmf') instead of new Error for consistent logging/telemetry; add config.logger?.debug('ClientMediaCall.sendDTMF') before sending.
- Ensure IClientMediaCall declares sendDTMF and confirm server-side handling/semantics for the "dtmf" payload and duration.
Verification: sandbox searches failed; manually confirm and apply to packages/media-signaling/src/lib/Call.ts (around lines 633–642).
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (9)
ee/packages/media-calls/src/sip/providers/IncomingSipCall.ts (6)
147-159: Await callee notification and avoid logging the full request object.Prevents unhandled rejections and reduces log volume/PII.
Apply:
- calleeAgent.onRemoteDescriptionChanged(this.call._id, negotiationId); - - logger.debug({ msg: 'modify', method: 'IncomingSipCall.createDialog', req }); + await calleeAgent.onRemoteDescriptionChanged(this.call._id, negotiationId); + + logger.debug({ + msg: 'modify', + method: 'IncomingSipCall.createDialog', + hasBody: Boolean(req?.body), + callingNumber: req?.callingNumber, + calledNumber: req?.calledNumber, + });
187-191: Avoid shadowingresand logging entire SIP message.Rename param for clarity; omit raw message from logs.
Apply:
- protected cancel(res: SipMessage): void { - logger.debug({ msg: 'IncomingSipCall.cancel', res }); + protected cancel(message: SipMessage): void { + logger.debug({ msg: 'IncomingSipCall.cancel' });
285-308: Usefor...ofinstead offor await...ofover a Map.Iteration is synchronous; the async DB calls inside already use
await. This avoids unnecessary async iterator wrapping.Apply:
- for await (const localNegotiation of this.inboundRenegotiations.values()) { + for (const localNegotiation of this.inboundRenegotiations.values()) {
275-282: Clear back-reference on agent when call ends.Prevents stale routing via
BroadcastActorAgent.provider.Apply:
const { sipDialog } = this; this.sipDialog = null; this.lastCallState = 'hangup'; if (sipDialog) { sipDialog.destroy(); } + this.agent.provider = null;
195-197: DTMF default duration seems high.2000 ms is atypical; common ranges are ~50–250 ms. Consider lowering default unless upstream requires 2 s.
Would you confirm the expected default? If flexible, I can send a patch to use 200 ms and make duration configurable via settings.
14-16: Logging volume: consider sampling or conditional payloads before GA.Given the beta note, keep debug logs for now, but plan to downgrade noisy ones or guard with
logger.isLevelEnabled('debug')and avoid logging full SDPs/requests.packages/media-signaling/src/lib/Call.ts (3)
799-801: Add context to error logInclude callId and negotiationId to make incidents traceable.
- this.config.logger?.error(e); + this.config.logger?.error('ClientMediaCall.processAnswerRequest: failed to create answer', { + callId: this.callId, + negotiationId, + error: e, + });
1023-1023: Make internal state log line searchablePrefer a single structured line with a clear prefix to avoid noisy split logs.
- this.config.logger?.debug(stateName, stateValue); + this.config.logger?.debug('ClientMediaCall.onWebRTCInternalStateChange', { stateName, stateValue });
1037-1037: Elevate logging level on critical WebRTC errorsUse error level when critical to align with alerting; keep debug otherwise.
- this.config.logger?.debug('ClientMediaCall.onWebRTCInternalError', critical, error); + if (critical) { + this.config.logger?.error('ClientMediaCall.onWebRTCInternalError', { + critical, + error, + callId: this.callId, + negotiationId: this.currentNegotiationId, + }); + } else { + this.config.logger?.debug('ClientMediaCall.onWebRTCInternalError', { critical, error }); + }
📜 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 ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
ee/packages/media-calls/package.json(1 hunks)ee/packages/media-calls/src/sip/providers/IncomingSipCall.ts(1 hunks)packages/media-signaling/src/lib/Call.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ee/packages/media-calls/package.json
🧰 Additional context used
🧬 Code graph analysis (1)
ee/packages/media-calls/src/sip/providers/IncomingSipCall.ts (6)
ee/packages/media-calls/src/sip/Session.ts (2)
SipServerSession(17-199)sessionId(28-30)packages/core-typings/src/mediaCalls/IMediaCall.ts (4)
IMediaCall(35-63)MediaCallContact(28-28)MediaCallSignedContact(30-30)MediaCallContactInformation(22-26)ee/packages/media-calls/src/server/BroadcastAgent.ts (1)
BroadcastActorAgent(14-69)ee/packages/media-calls/src/sip/errorCodes.ts (2)
SipError(25-32)SipErrorCodes(1-23)packages/media-signaling/src/lib/services/states.ts (1)
isBusyState(9-11)packages/models/src/index.ts (1)
MediaCallNegotiations(188-188)
🔇 Additional comments (3)
ee/packages/media-calls/src/sip/providers/IncomingSipCall.ts (2)
58-63: Fix race: early CANCEL can arrive beforesipCallis assigned.Buffer and replay CANCEL after instantiation to avoid dropping it.
Apply:
- let sipCall: IncomingSipCall | null = null; + let sipCall: IncomingSipCall | null = null; + let pendingCancel: SipMessage | null = null; - req.on('cancel', (message) => { - sipCall?.cancel(message); - }); + req.on('cancel', (message) => { + pendingCancel = message; + sipCall?.cancel(message); + }); @@ - sipCall = new IncomingSipCall(session, call, callerAgent, channel, srf, req, res); + sipCall = new IncomingSipCall(session, call, callerAgent, channel, srf, req, res); + if (pendingCancel) { + sipCall.cancel(pendingCancel); + pendingCancel = null; + }Verify no other
req.on('cancel', ...)listeners suffer the same issue:#!/bin/bash rg -nC2 "req\.on\(['\"]cancel['\"]" ee/packages/media-callsAlso applies to: 97-101
312-334: Cleanup negotiations after answering to prevent leaks and double replies.Entries are never removed once answered; they linger and can be re‑used by error paths.
Apply:
- if (localNegotiation.isFirst) { - return this.createDialog(localNegotiation.answer.sdp).catch(() => { - logger.error('Failed to create incoming call dialog.'); - this.hangupPendingCall(SipErrorCodes.INTERNAL_SERVER_ERROR); - }); - } + if (localNegotiation.isFirst) { + try { + await this.createDialog(localNegotiation.answer.sdp); + } catch { + logger.error('Failed to create incoming call dialog.'); + this.hangupPendingCall(SipErrorCodes.INTERNAL_SERVER_ERROR); + } finally { + this.inboundRenegotiations.delete(localNegotiation.id); + } + return; + } localNegotiation.res.send(200, { body: localNegotiation.answer.sdp, }); + this.inboundRenegotiations.delete(localNegotiation.id);packages/media-signaling/src/lib/Call.ts (1)
633-642: DTMF: normalize input, guard state, validate duration
- Trim + uppercase the tone; accept only a single valid DTMF character.
- Bail when the call is over or hidden (consistent with other methods).
- Validate duration (>0, integer); omit duration when undefined; use this.throwError('invalid-dtmf') for consistent errors.
- public sendDTMF(dtmf: string, duration?: number): void { - if (!dtmf || !/^[0-9A-D#*,]$/.exec(dtmf)) { - throw new Error('Invalid DTMF tone.'); - } - - this.config.transporter.sendToServer(this.callId, 'dtmf', { - dtmf, - duration, - }); - } + public sendDTMF(dtmf: string, duration?: number): void { + if (this.isOver() || this.hidden) { + return; + } + const tone = dtmf.trim().toUpperCase(); + if (tone.length !== 1 || !/^[0-9A-D#*,]$/.test(tone)) { + this.throwError('invalid-dtmf'); + } + const dur = typeof duration === 'number' && duration > 0 ? Math.floor(duration) : undefined; + this.config.transporter.sendToServer(this.callId, 'dtmf', { + dtmf: tone, + ...(dur !== undefined && { duration: dur }), + }); + }To confirm server semantics (single tone vs. sequences / comma/pause handling), search server-side handlers:
rg -nP -C3 "\bdtmf\b" packages apps server
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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 ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (1)
apps/meteor/package.json(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). (2)
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
pierre-lehnen-rc
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/media-signaling/src/lib/Call.ts (2)
799-802: Structure error logging with context; avoid bare object loggingLog a clear message plus error object; current line logs the object alone, which is easy to miss in aggregated logs.
- this.config.logger?.error(e); + this.config.logger?.error('Failed to create WebRTC answer', e);
1037-1044: Use level-based logging for internal errorsCritical errors should be logged at error level; non-critical at warn. Debug hides important failures.
- this.config.logger?.debug('ClientMediaCall.onWebRTCInternalError', critical, error); + if (critical) { + this.config.logger?.error('ClientMediaCall.onWebRTCInternalError', error); + } else { + this.config.logger?.warn('ClientMediaCall.onWebRTCInternalError', error); + }
📜 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 ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
apps/meteor/package.json(1 hunks)ee/packages/media-calls/package.json(1 hunks)package.json(1 hunks)packages/media-signaling/src/lib/Call.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ee/packages/media-calls/package.json
⏰ 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). (1)
- GitHub Check: CodeQL-Build
🔇 Additional comments (4)
apps/meteor/package.json (1)
348-348: Use root‑level patch; keep workspace dep as plain semver (also fix invalid~/.yarn/patchespath).Workspace‑scoped
patch:locators fight dedupe and portability; keep the patch centralized under the rootresolutionsand pin the workspace dep to the plain version. Also,~/.yarn/patcheswon’t expand under Yarn; use a repo‑relative path at the root instead.Apply in this file:
- "drachtio-srf": "patch:drachtio-srf@npm%3A5.0.12#~/.yarn/patches/drachtio-srf-npm-5.0.12-b0b1afaad6.patch", + "drachtio-srf": "5.0.12",Verify and clean up duplicates:
#!/bin/bash set -euo pipefail echo ">> Check for workspace-level patch specs (should be none after change)" rg -nP '"drachtio-srf"\s*:\s*"patch:' -C2 echo ">> Confirm root resolution exists and uses a repo-relative path" rg -nP '"resolutions"\s*:\s*{' -n package.json -n -C3 rg -nP '"drachtio-srf"\s*:\s*"patch:.*#\./\.yarn/patches/' package.json || true echo ">> List available drachtio patch files at root" fd -a 'drachtio-srf-npm-5.0.12-*.patch' .yarn/patches || true echo ">> Show all references to drachtio-srf across workspaces" rg -nP '"drachtio-srf"\s*:\s*".*"' apps ee/packages packages -g '!**/node_modules/**' -C2package.json (1)
81-83: Fix patch locator: use repo-relative path & confirm patch file existsFile: package.json (lines 81–83). Replace the '~' locator with a repo-relative path and ensure the patch file exists at .yarn/patches/drachtio-srf-npm-5.0.12-b0b1afaad6.patch — automated check produced no output; manual verification required.
"resolutions": { @@ - "cross-spawn": "7.0.6", - "drachtio-srf": "patch:drachtio-srf@npm%3A5.0.12#~/.yarn/patches/drachtio-srf-npm-5.0.12-b0b1afaad6.patch" + "cross-spawn": "7.0.6", + "drachtio-srf": "patch:drachtio-srf@npm%3A5.0.12#./.yarn/patches/drachtio-srf-npm-5.0.12-b0b1afaad6.patch" },Verify locally: test -f .yarn/patches/drachtio-srf-npm-5.0.12-b0b1afaad6.patch && echo EXISTS || echo MISSING
packages/media-signaling/src/lib/Call.ts (2)
1023-1033: LGTM; keep an eye on log volumeGood to only log when the value changes. Given rapid ICE/PC events, this can still be chatty; keep it at debug as you did.
If debug logs become noisy, consider throttling:
- this.config.logger?.debug(stateName, stateValue); + // Optionally throttle if emitting too frequently + this.config.logger?.debug('webrtc.state', { name: stateName, value: stateValue });
1081-1082: Add grace timer for persistent WebRTC 'disconnected'Waiting indefinitely risks stuck calls that never hang up.
case 'disconnected': - // Disconnected state is temporary, so let's wait for it to change into something else before reacting. + // Disconnected can be transient; start a grace timer to auto‑hangup if it persists. + if (!this.webrtcDisconnectedTimer) { + this.webrtcDisconnectedTimer = setTimeout(() => { + if (this.serviceStates.get('connection') === 'disconnected' && !this.isOver()) { + this.hangup('service-error'); + } + }, DISCONNECTED_GRACE_MS); + } break;Additions outside this hunk (supporting code):
// Near other constants const DISCONNECTED_GRACE_MS = 15_000; // As a private field: private webrtcDisconnectedTimer: ReturnType<typeof setTimeout> | null = null;And clear the timer when connection stabilizes or terminal states occur:
case 'connected': if (this.state === 'accepted') this.changeState('active'); if (this.webrtcDisconnectedTimer) { clearTimeout(this.webrtcDisconnectedTimer); this.webrtcDisconnectedTimer = null; } break; case 'failed': case 'closed': if (this.webrtcDisconnectedTimer) { clearTimeout(this.webrtcDisconnectedTimer); this.webrtcDisconnectedTimer = null; } if (!this.isOver()) this.hangup('service-error'); break;
pierre-lehnen-rc
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
| } | ||
| } | ||
|
|
||
| export const mediaCallDirector = new MediaCallDirector(); |
There was a problem hiding this comment.
[nitpick] No real gain here... Methods are still unrelated, without a shared state to manipulate.
pierre-lehnen-rc
requested review from
KevLehman
and removed request for
KevLehman
| diff --git a/lib/@types/index.d.ts b/lib/@types/index.d.ts | ||
| index f71a82f458c1432202be8d4585fc70ba94bee4a4..b874ee9e1fa8a051f06d4824fc12161acfadb78a 100644 | ||
| --- a/lib/@types/index.d.ts | ||
| +++ b/lib/@types/index.d.ts | ||
| @@ -119,12 +119,12 @@ declare namespace Srf { | ||
| on(messageType: "modify", callback: (req: SrfRequest, res: SrfResponse) => void): void; | ||
| once(messageType: string, callback: (msg: SrfResponse) => void): void; | ||
| listeners(messageType: string): any[]; | ||
| - request(opts?: SrfRequest): Promise<SrfResponse>; | ||
| - request(opts: SrfRequest, callback?: (err: any, msg: SrfResponse) => void): void; | ||
| + request(opts?: Partial<SrfRequest>): Promise<SrfResponse>; | ||
| + request(opts: Partial<SrfRequest>, callback?: (err: any, msg: SrfResponse) => void): void; | ||
| } | ||
|
|
||
| export interface CreateUASOptions { | ||
| - localSdp: string; | ||
| + localSdp: string | (() => string | Promise<string>); | ||
| headers?: SipMessageHeaders; | ||
| } | ||
|
|
||
| @@ -135,6 +135,8 @@ declare namespace Srf { | ||
| localSdp?: string; | ||
| proxy?: string; | ||
| auth?: { username: string; password: string; }; | ||
| + callingName?: string; | ||
| + callingNumber?: string; | ||
| } | ||
|
|
There was a problem hiding this comment.
I think if all we're changing is types, we could get away with module / namespace / interface augmentation. 🤔
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Improvements
Localization
Chores