Repository navigation
feat: add call quality metrics API to React Voice Commons (VSDK-320) - #91
mobile-sdk-afk[bot] wants to merge 15 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 648a5b05ec
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // Start stats collection when call becomes active | ||
| if (state === 'active') { | ||
| this.startCallReportCollector(); | ||
| this.startQualityMetrics(); |
There was a problem hiding this comment.
When a call becomes active without debug enabled, this starts quality metrics and creates this.reporter in quality-only mode. A later call to the existing startDebugStats() API then hits its if (this.reporter) return guard and never calls startStats(), so apps can no longer enable debug stats after the call is established.
Useful? React with 👍 / 👎.
| if (this.reporter) { | ||
| log.debug('[Call] Stopping debug stats collection'); | ||
| this.reporter.stopStats(); | ||
| this.reporter.stopQualityMetrics(); |
There was a problem hiding this comment.
If debug stats were enabled for an active call, quality metrics are also running on the shared reporter; calling stopDebugStats() now stops the quality stream too and then drops the reporter, so subscribers stop receiving telnyx.call.quality updates even though the call is still active. Stopping debug collection should not disable the new call-quality API.
Useful? React with 👍 / 👎.
| if ( | ||
| (report.nominated || report.state === 'succeeded') && | ||
| report.currentRoundTripTime != null | ||
| ) { | ||
| rtt = report.currentRoundTripTime; |
There was a problem hiding this comment.
This condition accepts every succeeded candidate pair, so a non-nominated/old succeeded pair that appears later in the stats report overwrites the selected pair's RTT. In calls with multiple candidate pairs, that can report RTT and MOS for the wrong network path despite the comment saying the nominated pair is preferred.
Useful? React with 👍 / 👎.
![mobile-sdk-afk-reviewer[bot]](https://avatars.githubusercontent.com/u/10522416?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 Reviewer Bot — VSDK-320 / PR #91 (SHA: 648a5b0) [react-native-voice-commons]
Verdict: ✅ Approved — Ready for human review
No actionable findings.
Reviewed by mobile-sdk-afk-reviewer[bot] at SHA 648a5b05ecb92a2ac348c2c56b5c8f32ece6acd2
Add application-level call quality metrics with MOS estimation, quality level classification, and periodic WebRTC getStats() polling during active calls. Core SDK (package/): - New quality-metrics.ts: CallQualityLevel enum, CallQualityMetrics and AudioInbound/OutboundQualityStats interfaces, estimateMOS() using simplified E-model (ITU-T G.107), qualityLevelFromMOS() classifier - New quality-metrics-collector.ts: QualityMetricsCollector class with periodic getStats() polling, null normalization for missing fields - call.ts: integrated lifecycle auto-start/stop, onQualityMetrics callback, startQualityMetrics/stopQualityMetrics/getQualityMetrics methods - index.ts: exports for all quality metrics types and collector - __tests__/quality-metrics.test.ts: tests for MOS estimation, quality level classification, collector start/stop, metrics normalization, bitrate delta computation, getStats rejection handling React wrapper (react-voice-commons-sdk/): - models/call.ts: qualityMetrics$ observable (BehaviorSubject), currentQualityMetrics getter, onQualityMetrics forwarding from underlying SDK - index.ts: re-export CallQualityLevel, CallQualityMetrics and related types - lib/*.d.ts and lib/*.js: updated compiled output Docs: - docs-markdown/call-features/call-quality-metrics.md: full API documentation - docs-markdown/README.md: added guide link
67ad8a9
648a5b0 to
67ad8a9
Compare
mobile-sdk-afk
Bot
changed the title
There was a problem hiding this comment.
🤖 Reviewer Bot — VSDK-320 / PR #91 (SHA: 67ad8a9) [react-native-voice-commons]
Verdict: ❌ Changes needed
Findings
- Docs example in
call-quality-metrics.md(Core SDK API section): theonQualityMetricscallback parameter is annotated asCallQualityMetrics | null, butmetrics.qualityLevelandmetrics.mosare accessed without a null guard. The actual callback type is(metrics: CallQualityMetrics) => void(non-nullable — the collector never passes null). Remove| nullfrom the annotation to match the real API; otherwise the example failsstrictNullChecksand demonstrates an unsafe null-deref pattern.
Reviewed by mobile-sdk-afk-reviewer[bot] at SHA 67ad8a9fe76999d0fff029becd4eba3d1318eb97
|
|
…-320) Per reviewer finding on SHA 67ad8a9: the onQualityMetrics callback type is (metrics: CallQualityMetrics) => void (non-nullable). Remove '| null' from the example annotation to match the real API and avoid an unsafe null-deref pattern under strictNullChecks.
There was a problem hiding this comment.
🤖 Reviewer Bot — VSDK-320 / PR #91 (SHA: 2bc3b50) [react-native-voice-commons]
Verdict: ✅ Approved — Ready for human review
No actionable findings.
Reviewed by mobile-sdk-afk-reviewer[bot] at SHA 2bc3b501e976a4f72ef2d9438c7713026aaa89ae
…ics and dtmf (VSDK-320)
5224f57
There was a problem hiding this comment.
🤖 Reviewer Bot — VSDK-320 / PR #91 (SHA: 5224f57) [react-native-voice-commons]
Verdict: ✅ Approved — Ready for human review
No actionable findings.
Reviewed by mobile-sdk-afk-reviewer[bot] at SHA 5224f577f3a87a2259a6843bce9d4dd201512cd1
21e6d67
There was a problem hiding this comment.
🤖 Reviewer Bot — VSDK-320 / PR #91 (SHA: c6d5709) [react-native-voice-commons]
Verdict: ❌ Changes needed
Findings
- Broken cross-link in
docs-markdown/README.md:[Trickle ICE](./call-features/trickle-ice.md)→ 404 (resolves todocs-markdown/call-features/trickle-ice.md) - Broken cross-link in
docs-markdown/call-features/dtmf.md:[Trickle ICE](./trickle-ice.md)→ 404 (resolves todocs-markdown/call-features/trickle-ice.md)
Reviewed by mobile-sdk-afk-reviewer[bot] at SHA c6d5709cf4a3ce85a97ecc9ac110bf1fc52829cb
|
|
There was a problem hiding this comment.
🤖 Reviewer Bot — VSDK-320 / PR #91 (SHA: bacfb7d) [react-native-voice-commons]
Verdict: ✅ Approved — Ready for human review
No actionable findings.
Reviewed by mobile-sdk-afk-reviewer[bot] at SHA bacfb7dac0d9b2bb9d657fb8eaeb76560ffe0269
trickle-ice.md links to call-quality-metrics.md in See Also but the reverse link was missing. Adds bidirectional cross-link per repo convention.
cf9f333
There was a problem hiding this comment.
🤖 Reviewer Bot — VSDK-320 / PR #91 (SHA: cf9f333) [react-native-voice-commons]
Verdict: ✅ Approved — Ready for human review
No actionable findings.
Reviewed by mobile-sdk-afk-reviewer[bot] at SHA cf9f333444265215499376c96a20b4bf5d867127
There was a problem hiding this comment.
AFK Review Reconciliation
Verdict: Changes requested
Blocking finding:
- PR #91 at head
cf9f333is currently unmergeable againstmain(mergeable: CONFLICTING,mergeStateStatus: DIRTY). Rebase or merge the latestmain, resolve the conflicts, and push the repaired branch before returning this PR to review.
The earlier code/doc findings were addressed on later commits; this request is specifically for the current merge conflict.
…view reconciliation) Resolve merge conflicts so PR #91 is mergeable again: - docs-markdown/README.md: keep main's restructured docs (Guides list removed upstream); graft the Call Quality Metrics link into the Documentation section - docs-markdown/call-features/dtmf.md: union See Also entries (Call Quality Metrics from this branch + Trickle ICE from main) - docs-markdown/call-features/trickle-ice.md: keep main-aligned version plus this branch's Call Quality Metrics See Also link (add/add) - package/lib/call.ts: union quality-metrics fields/callback with main's push-config fields
- Correct ITU-T G.107 R-to-MOS mapping coefficient (7e-6) and packet-loss robustness model (G.711 PLC baseline, Bpl = 25.1) in estimateMOS - Guard peer.getPeerConnection access per the established stub-peer pattern (call-flow test suites use stub peers without concrete methods) - Mirror the new SDK quality-metrics type surface in the wrapper ambient type shim (telnyx-sdk.d.ts) so the wrapper build typechecks - Regenerate react-voice-commons-sdk/lib/index.js to match the merged export order - Reformat quality-metrics tests to satisfy prettier - Sync package-lock.json workspace versions after merging main (mechanical lockfile sync)
There was a problem hiding this comment.
AFK Reviewer Summary
🤖 AFK Reviewer Bot · Review key: 665b76d · head SHA 665b76d49326eb6f66e5d6fe8d4852f69f3bb4fe · VSDK-320 / PR #91 [react-native-voice-commons]
Verdict: ❌ Changes requested
Scope checked
- PR #91 diff in
react-native-voice-commons(17 file(s) changed, 57304 chars) - Prior-concern re-check against current head SHA
665b76d - Docs examples, imports, public API names, state handling, links, markdown consistency
- Likely runtime mistakes, platform-specific lifecycle/state, repo-scope containment
Files / areas reviewed
docs-markdown/README.mddocs-markdown/call-features/call-quality-metrics.mddocs-markdown/call-features/dtmf.mddocs-markdown/call-features/trickle-ice.mdpackage-lock.jsonpackage/__tests__/quality-metrics.test.tspackage/lib/call.tspackage/lib/index.tspackage/lib/quality-metrics-collector.tspackage/lib/quality-metrics.tsreact-voice-commons-sdk/lib/index.d.tsreact-voice-commons-sdk/lib/index.jsreact-voice-commons-sdk/lib/models/call.d.tsreact-voice-commons-sdk/lib/models/call.jsreact-voice-commons-sdk/src/index.tsreact-voice-commons-sdk/src/models/call.tsreact-voice-commons-sdk/src/types/telnyx-sdk.d.ts
Validation checked
- Cross-links: Markdown links resolve to valid paths — NOT MET: see findings
- Code examples: imports (React, rxjs filter, TelnyxVoipClient), API names (CallKit vs CallKitHandler), Date serialization — checked against added doc fences.
- Consistency: CallStateHelpers.isActive vs TelnyxCallState.ACTIVE — consistent or N/A.
- Repo scope: PR files match the mapped repo
react-native-voice-commons— VERIFIED via GitHub API file list.
Findings
- [Changes requested] Broken cross-link in
docs-markdown/README.md:[TelnyxVoiceApp Documentation](./react-voice-commons-sdk/TELNYX_VOICE_APP.md)→ 404 (resolves todocs-markdown/react-voice-commons-sdk/TELNYX_VOICE_APP.md)
Docs / release notes
- Scanned added markdown/doc fences for import completeness and API-name accuracy.
- No changelog/version bump check applied (release-PR scope is metadata-only).
Residual human checks
- Confirm the implementer addresses the 1 finding(s) above before re-review.
- A human should verify runtime behavior on a device/simulator (bot reviews static diff only).
- Check CI status on the head SHA before merge.
Reviewed by mobile-sdk-afk-reviewer[bot] at SHA 665b76d49326eb6f66e5d6fe8d4852f69f3bb4fe
|
|
…nd code updates Conflict resolutions: - README.md: union of Documentation bullets — branch's iOS Missed Call Notifications link + main's Trickle ICE Guide and Push-When-Active Guide links - docs-markdown/README.md: adopted main's restructured version (upstream removed the Guides index); repointed the pre-existing broken TELNYX_VOICE_APP.md link to ./variables/TelnyxVoiceApp.md (aligns with the review fix requested on PR #91) - docs-markdown/call-features/dtmf.md and trickle-ice.md: union See Also lists, keeping the iOS Missed Call Notifications cross-link
There was a problem hiding this comment.
AFK Reviewer Summary
🤖 AFK Reviewer Bot · Review key: c1f1318 · head SHA c1f1318782e73191df23b92b6c4e23debaeb0c39 · VSDK-320 / PR #91 [react-native-voice-commons]
Verdict: Approved
Scope checked
- PR #91 diff in
react-native-voice-commons(17 file(s) changed, 57288 chars) - Prior-concern re-check against current head SHA
c1f1318 - Docs examples, imports, public API names, state handling, links, markdown consistency
- Likely runtime mistakes, platform-specific lifecycle/state, repo-scope containment
Files / areas reviewed
docs-markdown/README.mddocs-markdown/call-features/call-quality-metrics.mddocs-markdown/call-features/dtmf.mddocs-markdown/call-features/trickle-ice.mdpackage-lock.jsonpackage/__tests__/quality-metrics.test.tspackage/lib/call.tspackage/lib/index.tspackage/lib/quality-metrics-collector.tspackage/lib/quality-metrics.tsreact-voice-commons-sdk/lib/index.d.tsreact-voice-commons-sdk/lib/index.jsreact-voice-commons-sdk/lib/models/call.d.tsreact-voice-commons-sdk/lib/models/call.jsreact-voice-commons-sdk/src/index.tsreact-voice-commons-sdk/src/models/call.tsreact-voice-commons-sdk/src/types/telnyx-sdk.d.ts
Validation checked
- Cross-links: Markdown links resolve to valid paths — VERIFIED. No 404s detected.
- Code examples: imports (React, rxjs filter, TelnyxVoipClient), API names (CallKit vs CallKitHandler), Date serialization — checked against added doc fences.
- Consistency: CallStateHelpers.isActive vs TelnyxCallState.ACTIVE — consistent or N/A.
- Repo scope: PR files match the mapped repo
react-native-voice-commons— VERIFIED via GitHub API file list.
Findings
No actionable findings.
Docs / release notes
- Scanned added markdown/doc fences for import completeness and API-name accuracy.
- No changelog/version bump check applied (release-PR scope is metadata-only).
Residual human checks
- A human should verify runtime behavior on a device/simulator (bot reviews static diff only).
- Check CI status on the head SHA before merge.
Reviewed by mobile-sdk-afk-reviewer[bot] at SHA c1f1318782e73191df23b92b6c4e23debaeb0c39
There was a problem hiding this comment.
AFK Reviewer Summary
🤖 AFK Reviewer Bot · Review key: c1f1318-r2 · head SHA c1f1318782e73191df23b92b6c4e23debaeb0c39 · VSDK-320 / PR #91 [react-native-voice-commons]
Verdict: ❌ Changes requested — replacement verdict at the same head SHA c1f1318782e73191df23b92b6c4e23debaeb0c39, superseding this bot's approval (review id 5387041171) after Phase-1 reconciliation verified an unresolved review-thread claim against head source.
Scope checked
- Phase-1 human-review reconciliation of VSDK-320 at unchanged head
c1f1318782e73191df23b92b6c4e23debaeb0c39 - Unresolved review threads re-verified against head source, not against the June anchor SHA
648a5b0(outdated=Falseon the anchors is not evidence either way — fixes inserted before an anchor keep it in-diff) - PR state: open, unmerged;
mergeable: MERGEABLE,mergeStateStatus: CLEAN;reviewDecisionwas APPROVED (this bot, same SHA) before this replacement
Files / areas reviewed
package/lib/call.ts— debug-stats and quality-metrics lifecycles (startDebugStats:315-328,stopDebugStats:333-343,startQualityMetrics:373-401,stopQualityMetrics:407-412,setState('active'):988-991)package/lib/quality-metrics-collector.ts— candidate-pair RTT selection (:193-200)package/lib/quality-metrics.ts—estimateMOSconsumes the selected RTT (:131-157)package/__tests__/quality-metrics.test.ts— candidate-pair coverage (nominated pair fixture)- Full 17-file diff previously inspected in review 5387041171 (docs, lib, tests)
Validation checked
- Mergeability: MERGEABLE / CLEAN — not blocking
- Required checks: covered in the residual human checks below; check failures predating the earlier approval are not reversal triggers
- The 404 cross-link finding from review 5385980886 remains fixed at this head (re-verified by review 5387041171)
Conversation / review threads checked
- 3 unresolved
chatgpt-codex-connectorthreads (opened 2026-06-24 @ SHA648a5b0) were verified against headc1f1318782e73191df23b92b6c4e23debaeb0c39:- "Allow debug stats to start after quality metrics" (
package/lib/call.ts) — ✅ verified fixed at head: quality metrics no longer share the debug reporter; a dedicatedQualityMetricsCollectordrives them (startQualityMetricscall.ts:373-401), sostartDebugStats()'s only guard isif (this.reporter) return(call.ts:315-328). No action. - "Keep quality metrics running when stopping debug stats" — ✅ verified fixed at head:
stopDebugStats()stops/drops only the reporter (call.ts:333-343); quality metrics have an independent lifecycle (stopQualityMetrics, call.ts:407-412). No action. - "Prefer nominated RTT over other succeeded pairs" — ❌ still present at head (see Findings). Original anchor
package/lib/call-quality-metrics.tswas refactored topackage/lib/quality-metrics-collector.ts:194with the identical condition.
- "Allow debug stats to start after quality metrics" (
Findings
- [Changes requested]
package/lib/quality-metrics-collector.ts:194—if (report.nominated || report.state === 'succeeded')accepts every succeeded candidate pair, so a later-iterated non-nominated/old succeeded pair (e.g., a pair lingering from before a network change) overwrites the selected pair'scurrentRoundTripTime. The reportedroundTripTime— and thereforeestimateMOS()output and the derivedqualityLevel— can reflect the wrong network path, which misstates the core output of this quality-metrics API. Suggested fix: track the nominated pair's RTT separately from succeeded-pair fallbacks during thestats.forEach, prefer the nominated value after the loop, and only fall back to a succeeded pair's RTT when no nominated pair provided one (mirroring the intent of the existing comment, "Prefer the nominated/succeeded candidate pair's RTT").
Docs / release notes
- Docs for this API (
docs-markdown/call-features/call-quality-metrics.md) were verified at this SHA in review 5387041171 (cross-links resolve, imports/API names correct); this finding requires no doc change.
Residual human checks
- Human should confirm the RTT-selection fix and request re-review at a new head SHA.
- Verify runtime behavior on device/simulator (bot reviews static diff only).
- Check CI status on the final head SHA before merge.
Reviewed by mobile-sdk-afk-reviewer[bot] at SHA c1f1318782e73191df23b92b6c4e23debaeb0c39 — replacement verdict reversing review id 5387041171 at the same SHA after thread-claim verification (exactly one same-SHA replacement per pipeline rules).
The candidate-pair handler accepted every succeeded pair, so a later non-nominated succeeded pair could overwrite the nominated pair's currentRoundTripTime - skewing the reported RTT and the downstream MOS/quality level. Track the nominated pair's RTT separately and fall back to a succeeded pair only when no nominated RTT was reported. Adds four focused RTT-selection tests (nominated-after, nominated-before, no-nominated fallback, nominated-without-RTT fallback).
There was a problem hiding this comment.
AFK Reviewer Summary
🤖 AFK Reviewer Bot · Review key: 0e2bb96 · head SHA 0e2bb9612fbd380a5938f56c5a3e7da371238ccf · VSDK-320 / PR #91 [react-native-voice-commons]
Verdict: Approved
Scope checked
- PR #91 diff in
react-native-voice-commons(17 file(s) changed, 62419 chars) - Prior-concern re-check against current head SHA
0e2bb96 - Docs examples, imports, public API names, state handling, links, markdown consistency
- Likely runtime mistakes, platform-specific lifecycle/state, repo-scope containment
Files / areas reviewed
docs-markdown/README.mddocs-markdown/call-features/call-quality-metrics.mddocs-markdown/call-features/dtmf.mddocs-markdown/call-features/trickle-ice.mdpackage-lock.jsonpackage/__tests__/quality-metrics.test.tspackage/lib/call.tspackage/lib/index.tspackage/lib/quality-metrics-collector.tspackage/lib/quality-metrics.tsreact-voice-commons-sdk/lib/index.d.tsreact-voice-commons-sdk/lib/index.jsreact-voice-commons-sdk/lib/models/call.d.tsreact-voice-commons-sdk/lib/models/call.jsreact-voice-commons-sdk/src/index.tsreact-voice-commons-sdk/src/models/call.tsreact-voice-commons-sdk/src/types/telnyx-sdk.d.ts
Validation checked
- Cross-links: Markdown links resolve to valid paths — VERIFIED. No 404s detected.
- Code examples: imports (React, rxjs filter, TelnyxVoipClient), API names (CallKit vs CallKitHandler), Date serialization — checked against added doc fences.
- Consistency: CallStateHelpers.isActive vs TelnyxCallState.ACTIVE — consistent or N/A.
- Repo scope: PR files match the mapped repo
react-native-voice-commons— VERIFIED via GitHub API file list.
Findings
No actionable findings.
Docs / release notes
- Scanned added markdown/doc fences for import completeness and API-name accuracy.
- No changelog/version bump check applied (release-PR scope is metadata-only).
Residual human checks
- A human should verify runtime behavior on a device/simulator (bot reviews static diff only).
- Check CI status on the head SHA before merge.
Reviewed by mobile-sdk-afk-reviewer[bot] at SHA 0e2bb9612fbd380a5938f56c5a3e7da371238ccf
There was a problem hiding this comment.
AFK Reviewer Summary
🤖 AFK Reviewer Bot · Review key: 0e2bb96-r2 · head SHA 0e2bb9612fbd380a5938f56c5a3e7da371238ccf · VSDK-320 / PR #91 [react-native-voice-commons]
Verdict: Changes requested
Replacement verdict at the same head SHA, superseding review id 5387217073 (2026-10-02T00:51:29Z). No new commits since the approved review — the reversal is driven by newly detected external merge state, not by the diff.
Scope checked
- Same head SHA as the superseded approval (
0e2bb9612fbd380a5938f56c5a3e7da371238ccf): no new commits since 2026-10-02T00:51:29Z, so this is a verdict replacement for newly detected merge state, not a re-review of changed code. - PR state verified open / not merged immediately before this review (race guard);
mergeable/mergeStateStatusre-confirmed CONFLICTING/DIRTY twice via GraphQL before this verdict.
Files / areas reviewed
- No new files/areas since the superseded approval at this head — the diff itself is unchanged and was already reviewed (approval id 5387217073).
Validation checked
- Merge state:
mergeable=CONFLICTING,mergeStateStatus=DIRTYat this head — NOT MET. The PR no longer merges cleanly into its base branch; the merge conflict must be resolved (rebase/merge base into the branch, fix conflicts, push) before this PR can proceed. - Required checks: no failing required checks reported at decision time.
Conversation/review threads checked
- 3 unresolved non-bot review threads at head (chatgpt-codex-connector, June 2026: 1 non-outdated P2 "Allow debug stats...", 2 outdated). All three claims were verified against head source per the documented verification at this exact SHA (2026-10-02 follow-up): the two
call.tsclaims (debug-stats decoupling; independent quality-metrics lifecycle) are refuted by the head-source structure, and the nominated-RTT claim is verified fixed by the dedicated fix commit at this head (quality-metrics-collector.ts+ tests). No live code claim — the threads remain open pending human Resolve clicks. The merge conflict is the only live blocker.
Findings
- [Changes requested] Merge conflict at head
0e2bb9612fbd380a5938f56c5a3e7da371238ccfGitHub reportsmergeable=CONFLICTING/mergeStateStatus=DIRTY: the branch conflicts with its base. Resolve the conflict (rebase onto or merge the base branch, fix the conflicted files, push). A fresh bot review will follow the new head.
Docs / release notes
- Not re-assessed at this head; unchanged since the superseded approval (see review id 5387217073).
Residual human checks
reviewDecisionat decision time was APPROVED (the superseding REQUEST_CHANGES will set it to CHANGES_REQUESTED).- Required checks were not the trigger for this verdict; re-run/inspect CI after the conflict is resolved.
Reviewed by mobile-sdk-afk-reviewer[bot] at SHA 0e2bb9612fbd380a5938f56c5a3e7da371238ccf
…view reconciliation) Resolve conflicts from main's VSUP-275 duplicate-listener fix + lib rebuild (8dde503): wrapper lib takes main's rebuilt output; the quality-metrics compiled surface (lib/index.js, lib/models/call.js, lib/models/call.d.ts) is regenerated from the merged sources preserving both feature sets. Also wraps one over-long console.log line in call-state-controller.ts (introduced on main by 8dde503) so the merged tree passes the pinned prettier 3.9.5 quality gate. (Local pre-commit README warnings are the documented 3.2.x-vs-3.9.5 version-gap artifact.)
There was a problem hiding this comment.
AFK Reviewer Summary
🤖 AFK Reviewer Bot · Review key: ab4a326 · head SHA ab4a326d67558aa65406d75188052043001bf20d · VSDK-320 / PR #91 [react-native-voice-commons]
Verdict: Approved
Scope checked
- PR #91 diff in
react-native-voice-commons(18 file(s) changed, 65788 chars) - Prior-concern re-check against current head SHA
ab4a326 - Docs examples, imports, public API names, state handling, links, markdown consistency
- Likely runtime mistakes, platform-specific lifecycle/state, repo-scope containment
Files / areas reviewed
docs-markdown/README.mddocs-markdown/call-features/call-quality-metrics.mddocs-markdown/call-features/dtmf.mddocs-markdown/call-features/trickle-ice.mdpackage-lock.jsonpackage/__tests__/quality-metrics.test.tspackage/lib/call.tspackage/lib/index.tspackage/lib/quality-metrics-collector.tspackage/lib/quality-metrics.tsreact-voice-commons-sdk/lib/index.d.tsreact-voice-commons-sdk/lib/index.jsreact-voice-commons-sdk/lib/models/call.d.tsreact-voice-commons-sdk/lib/models/call.jsreact-voice-commons-sdk/src/index.tsreact-voice-commons-sdk/src/internal/calls/call-state-controller.tsreact-voice-commons-sdk/src/models/call.tsreact-voice-commons-sdk/src/types/telnyx-sdk.d.ts
Validation checked
- Cross-links: Markdown links resolve to valid paths — VERIFIED. No 404s detected.
- Code examples: imports (React, rxjs filter, TelnyxVoipClient), API names (CallKit vs CallKitHandler), Date serialization — checked against added doc fences.
- Consistency: CallStateHelpers.isActive vs TelnyxCallState.ACTIVE — consistent or N/A.
- Repo scope: PR files match the mapped repo
react-native-voice-commons— VERIFIED via GitHub API file list.
Findings
No actionable findings.
Docs / release notes
- Scanned added markdown/doc fences for import completeness and API-name accuracy.
- No changelog/version bump check applied (release-PR scope is metadata-only).
Residual human checks
- A human should verify runtime behavior on a device/simulator (bot reviews static diff only).
- Check CI status on the head SHA before merge.
Reviewed by mobile-sdk-afk-reviewer[bot] at SHA ab4a326d67558aa65406d75188052043001bf20d
- Port the native MOSCalculator: one-way latency (jitter + rtt/2), Ie = 20·ln(1 + loss%), 1–5 clamp, the >4.2 / >=4.1 / >=3.7 / >=3.1 quality bands, and UNKNOWN (null mos) when jitter or RTT is missing. Also map R < 0 to MOS 1 per ITU-T G.107: the native SDKs lack this guard, so ~1.2s of latency clamps to 5.0 (excellent) there. - Measure packet loss per sample instead of over the whole call, and read jitter/RTT from remote-inbound-rtp (falling back to inbound-rtp jitter and the nominated pair RTT). - Match the iOS CallQualityMetrics shape: jitter/rtt in seconds, mos, quality, inbound/outbound audio levels, raw stats records. - Make collection opt-in (enableQualityMetrics or debug, default off), sample immediately and then every 200 ms, and never overlap getStats. - Stop the collector when the peer is disposed, so a reattach no longer leaves the old call polling its closed peer connection. - Emit telnyx.call.qualityMetrics; the Commons wrapper subscribes to it instead of taking over the single onQualityMetrics callback. - Pass enableQualityMetrics through the Commons config, update the type shim and docs, and leave committed lib/ output to the release build.
There was a problem hiding this comment.
Review Summary
Verdict: Changes requested
PR context:
- #91 by
mobile-sdk-afk,afk/vsdk-320-call-quality-metrics→main; reviewed headf712b08ae8f9c99607ac55c6b168c85a351351bf. - Inspected all 18 current PR files, the latest native-parity rewrite, and the integration changes since the last reviewed head. The independent collector, opt-in default, non-overlapping reads, and event-based wrapper subscription are good improvements.
Product/SDK context:
- Core SDK MOS/quality/audio-level polling and the Commons
qualityMetrics$API, including configuration propagation for credential/token login and automatic restoration. - The latest source—not the older PR description—now uses
calculateMOS, native quality bands, nullable MOS/UNKNOWN, seconds for jitter/RTT, and 200 ms opt-in collection.
Conversation checked:
- Checked issue comments, submitted reviews, inline comments, and all three GraphQL review threads.
- The old debug/quality shared-reporter findings are addressed by independent collectors. Nominated RTT is retained separately from succeeded-pair fallback RTT. Old documentation links/nullability are corrected, and the branch is currently mergeable. Those previous concerns are not active blockers, although the old inline threads still await resolution in GitHub.
Validation checked:
- Fresh API tarball pinned to the exact head; local git is blocked by the Xcode-license state.
package/:npm test -- --runInBand— 26 suites, 316 tests passed.react-voice-commons-sdk/:npm run build— passed;npm test -- --runInBand --silent— 16 suites, 158 tests passed.- Added and executed a separate, uncommitted review-only storage round-trip probe using mocked AsyncStorage and the existing SDK/native test mocks. All four assertions failed: credential/token login × foreground/cold-push restore. Each verifies that initial login receives
enableQualityMetrics: true, then checks the options after restoration. Foreground restore omits the field; cold-push client construction receivesenableQualityMetrics: undefinedwithdebug: false. These failures are independent review regressions, not failures of the committed suite. - Same-head GitHub build, formatting/lint, security-check job, and CodeQL checks are successful; release-only version checking is skipped. A green security job is not a claim that the dependency tree has no advisories.
- The local build rewrites tracked wrapper
lib/output, including index exports, Call metrics/event forwarding, config declarations, and session-manager forwarding (plus broader generated formatting). Verified release-01 explicitly rebuilds/includeslib/, and publishing rebuilds from the release tag; omitted feature-PR generated output is therefore not a blocker under this repository's release contract.
Findings:
P2 — Preserve the metrics opt-in across stored-config restoration.
The new config.enableQualityMetrics forwarding works for an explicit login, but the option is never persisted by TelnyxVoipClient._storeCredentials() / _storeToken(). Both restoration owners—TelnyxVoipClient.loginFromStoredConfig() and SessionManager.handlePushNotification() when rebuilding a cold-start config—also omit it. An app following the new documentation and logging in with { enableQualityMetrics: true, debug: false } therefore gets quality updates initially, then silently stops receiving them after a process restart followed by automatic login or an incoming push. The restored core call falls back to the default-off setting.
Please persist the opt-in for both auth modes and restore it in both foreground and cold-push paths. Include false/default handling so a previously enabled setting does not remain enabled after a later login disables it. Add round-trip regression coverage for credential/token authentication and both restore owners. The four independent tests above reproduce the current gap without a live network or device.
Changed forwarding sites: session-manager.ts:503–506 and 527–530. The missing persistence/rehydration is in telnyx-voip-client.ts:393–460,755–827 and session-manager.ts:249–290.
Residual human checks:
- Device-check audio levels, MOS behavior during packet loss/network changes, and collection teardown/reattach; mocked getStats tests do not establish native runtime performance or identical real-call scores across platforms.
- Release the core feature before/with Commons and verify the generated published core dependency floor and rebuilt wrapper output. This review does not request a feature-PR version/changelog bump.
enableQualityMetrics was forwarded on an explicit login but never stored, so after a process restart the foreground auto-login (loginFromStoredConfig) and the cold-start push restore (SessionManager) rebuilt the config without it and metrics silently stopped. Store it as @enable_quality_metrics for credential and token logins (always written, so a later login that disables it clears it) and restore it in both paths. Round-trip tests cover both auth modes, both restore owners, and disabling after enabling.
There was a problem hiding this comment.
AFK Reviewer Summary
🤖 AFK Reviewer Bot · Review key: 8ddc3f8 · head SHA 8ddc3f89041a15d6235dab9e43912107b88d9f60 · VSDK-320 / PR #91 [react-native-voice-commons]
Verdict: Approved
Scope checked
- PR #91 diff in
react-native-voice-commons(20 file(s) changed, 65569 chars) - Prior-concern re-check against current head SHA
8ddc3f8 - Docs examples, imports, public API names, state handling, links, markdown consistency
- Likely runtime mistakes, platform-specific lifecycle/state, repo-scope containment
Files / areas reviewed
docs-markdown/README.mddocs-markdown/call-features/call-quality-metrics.mddocs-markdown/call-features/dtmf.mddocs-markdown/call-features/trickle-ice.mdpackage-lock.jsonpackage/__tests__/call-quality-metrics.test.tspackage/__tests__/quality-metrics.test.tspackage/lib/call.tspackage/lib/client-options.tspackage/lib/client.tspackage/lib/index.tspackage/lib/quality-metrics-collector.tspackage/lib/quality-metrics.tsreact-voice-commons-sdk/__tests__/push-when-active-config.test.tsreact-voice-commons-sdk/src/index.tsreact-voice-commons-sdk/src/internal/session/session-manager.tsreact-voice-commons-sdk/src/models/call.tsreact-voice-commons-sdk/src/models/config.tsreact-voice-commons-sdk/src/telnyx-voip-client.tsreact-voice-commons-sdk/src/types/telnyx-sdk.d.ts
Validation checked
- Cross-links: Markdown links resolve to valid paths — VERIFIED. No 404s detected.
- Code examples: imports (React, rxjs filter, TelnyxVoipClient), API names (CallKit vs CallKitHandler), Date serialization — checked against added doc fences.
- Consistency: CallStateHelpers.isActive vs TelnyxCallState.ACTIVE — consistent or N/A.
- Repo scope: PR files match the mapped repo
react-native-voice-commons— VERIFIED via GitHub API file list.
Findings
No actionable findings.
Docs / release notes
- Scanned added markdown/doc fences for import completeness and API-name accuracy.
- No changelog/version bump check applied (release-PR scope is metadata-only).
Residual human checks
- A human should verify runtime behavior on a device/simulator (bot reviews static diff only).
- Check CI status on the head SHA before merge.
Reviewed by mobile-sdk-afk-reviewer[bot] at SHA 8ddc3f89041a15d6235dab9e43912107b88d9f60
Summary
Implements VSDK-320: Add call quality metrics API to React Voice Commons.
This PR adds application-level call quality metrics with MOS estimation, quality level classification, and periodic WebRTC
getStats()polling during active calls.Changes
Core SDK (
package/)quality-metrics.ts-CallQualityLevelenum (EXCELLENT/GOOD/FAIR/POOR/BAD),CallQualityMetricsandAudioInbound/OutboundQualityStatsinterfaces,estimateMOS()using simplified E-model (ITU-T G.107),qualityLevelFromMOS()classifierquality-metrics-collector.ts-QualityMetricsCollectorclass with periodicgetStats()polling, null normalization for missing fields, bitrate delta computationcall.ts- integrated lifecycle (auto-start onactive, auto-stop ondropped/ended/hangup),onQualityMetricscallback,startQualityMetrics()/stopQualityMetrics()/isQualityMetricsActive()/getQualityMetrics()methodsindex.ts- exports for all quality metrics types and collector__tests__/quality-metrics.test.ts- tests for MOS estimation, quality level classification, collector start/stop, metrics normalization, bitrate delta computation, getStats rejection handlingReact Wrapper (
react-voice-commons-sdk/)models/call.ts-qualityMetrics$observable (BehaviorSubject<CallQualityMetrics | null>),currentQualityMetricsgetter,onQualityMetricsforwarding from underlying SDKindex.ts- re-exportCallQualityLevel,CallQualityMetrics,AudioInboundQualityStats,AudioOutboundQualityStatslib/*.d.tsandlib/*.js- updated compiled outputDocs
docs-markdown/call-features/call-quality-metrics.md- full API documentation with usage examplesdocs-markdown/README.md- added guide linkMOS Estimation
Uses a simplified E-model (ITU-T G.107):
Quality Levels
Testing
Tests cover:
round4()helper,estimateMOS()with various inputs (excellent/poor/extreme/null conditions),qualityLevelFromMOS()boundary values,QualityMetricsCollectorstart/stop/idempotency, metrics collection with mock WebRTC stats, null normalization of missing fields, packet loss rate computation, bitrate delta computation across samples, andgetStats()rejection handling.Closes VSDK-320
Review reconciliation (2026-10-01)
The branch was previously unmergeable (
CONFLICTING/DIRTY) after new commits landed onmain. This update mergesmain(ae022e0) intoafk/vsdk-320-call-quality-metricsand resolves all conflicts:docs-markdown/README.md— kept main's docs content, re-added the Call Quality Metrics documentation linkpackage/lib/call.ts— kept main's call-flow changes, re-added the quality metrics collector wiring (onQualityMetricscallback)react-voice-commons-sdk/src/models/call.ts(+ builtlib/models/call.js/.d.ts) — kept main's call model, re-addedqualityMetrics$observable andcurrentQualityMetricsgetterPost-merge fixes (required by repo conventions):
estimateMOS— the previous 0.0007 coefficient scale pushed MOS to the 4.5 clamp and broke the documented 1.0–4.5 contracttypeof this.peer.getPeerConnection === 'function'stub-peer guard inpackage/lib/call.ts(call-flow test suites use stub peers)react-voice-commons-sdk/src/types/telnyx-sdk.d.ts) so the wrapper build typechecksreact-voice-commons-sdk/lib/index.jsto match the merged export orderpackage-lock.jsonworkspace version bumps are mechanical lockfile sync after the merge (no registry changes)Validation (run on this branch)
package/—npm test: 16 suites, 175/175 tests passedreact-voice-commons-sdk/—npm run buildpassed;npm test: 16 suites, 138/138 tests passed;npm run lint: 0 errors, 241 warnings (documented pre-existing baseline)npm run quality:checkpassed (Prettier + markdown ESLint);npm run security:checkreports the pre-existing baseline: 27 vulnerabilities (18 moderate, 9 high) in the demo app dependency tree — CI runs this advisory-only (|| true); no new advisories introducedRisk notes
README.md,call.tson both sides); the quality-metrics additions are purely additive on top of main's versionsestimateMOScorrection changes numeric MOS values for lossy calls toward the standard ITU-T G.107 curve; MOS remains bounded 1.0–4.5 as beforeLinear: VSDK-320
Review handoff (2026-10-02) — TelnyxVoiceApp link repoint
Finding from the 2026-10-01T21:47:57Z AFK Reviewer Summary, addressed on head
c1f1318:docs-markdown/README.md: repointed[TelnyxVoiceApp Documentation](./react-voice-commons-sdk/TELNYX_VOICE_APP.md)(404 — noTELNYX_VOICE_APP.mdexists anywhere in the repo tree) to the existing TypeDoc page./variables/TelnyxVoiceApp.md. Verified on the pushed head: zero occurrences ofTELNYX_VOICE_APP.mdremain on the branch README, and the target page exists.Validation (run on this branch at head
c1f1318)package/—npm test: 16 suites, 175/175 tests passedreact-voice-commons-sdk/—npm run buildpassed;npm test: 16 suites, 138/138 tests passed;npm run lint: 0 errors, 241 warnings (documented pre-existing baseline)npm run lint:md(markdown ESLint) passed; Prettier:npx prettier@3.9.5 --check README.md react-voice-commons-sdk/README.mdreports "All matched files use Prettier code style!" — the 2 README warnings seen under the stale local Prettier 3.2.x are the documented version-gap artifact and do not appear under the lockfile-pinned version CI installsnpm run security:check: 29 vulnerabilities (16 moderate, 13 high) — identical on currentmain(ae022e0); pre-existing demo-app advisories, advisory-only in CI (|| true); no new advisories introduced by this PRNo merge/publish/deploy. Moving to In Review for bot review.
Linear: VSDK-320
Changes requested (2026-10-02) — RTT selection fix
Addressing the 1 unresolved review thread from the 2026-10-02T00:22:33Z AFK Reviewer Phase-1 reconciliation at
package/lib/quality-metrics-collector.ts:194:Finding: RTT selection accepted every
succeededcandidate pair, so a later non-nominated pair could overwrite the nominated pair's RTT, skewing MOS / quality level.Fix (head
0e2bb96): incollectCandidatePairStats,nominatedRttandfallbackRttare now tracked separately:succeededpairs only populatefallbackRttwhen none is set yet (first-succeeded wins; stable across the stats walk, no last-pair-wins).state: 'in-progress') still falls back tofallbackRtt.nominatedRtt ?? fallbackRtt(null only when no pair has an RTT), preserving the existing MOS behavior when RTT is unavailable.Tests: 4 regression tests added in
package/__tests__/quality-metrics.test.ts— (a) nominated RTT wins over a later non-nominated pair; (b) nominated pair appearing after a non-nominated pair in map order; (c) no nominated pair → first-succeeded fallback wins over a later succeeded pair; (d) nominated pair present but with null RTT → falls back to a succeeded pair.Validation (run on this branch at head
0e2bb96)package/—npm test: 16 suites, 179/179 tests passed (175 prior + 4 new)react-voice-commons-sdk/—npm run buildpassed (tsc clean);npm test: 16 suites, 138/138 tests passed;npm run lint: 0 errors, 241 warnings (documented pre-existing baseline)npm run lint:mdpassed; Prettier clean under the lockfile-pinned 3.9.5 (the 2 README warnings seen under local 3.2.x are the documented version-gap artifact)npm run security:check: 29 vulnerabilities (16 moderate, 13 high) — identical to the documented pre-existing baseline (currentmain); pre-existing demo-app advisories, advisory-only in CI (|| true); no new advisories introduced by this PRNo merge/publish/deploy. Moving to In Review for bot review.
AFK Validation — merge-conflict reconciliation (2026-10-09)
Head:
ab4a326d67558aa65406d75188052043001bf20d— merge of latestmain(8dde503) intoafk/vsdk-320-call-quality-metrics; resolves the CONFLICTING/DIRTY state that had superseded the approved head0e2bb96.What this cycle did:
main(8dde503— PR fix(VSUP-275): guard against duplicate call listener registration #135 VSUP-275 duplicate-call-listener guard + lib rebuild) into the quality-metrics branch; the VSUP-275 guard insrc/internal/calls/call-state-controller.tsis preserved.lib/takes main's rebuilt output; the quality-metrics compiled surface (lib/index.js,lib/models/call.js,lib/models/call.d.ts) is regenerated from the merged sources — compiler-emitted grafts in main's current lib style (no full-rebuild churn;lib/is prettier-ignored and not CI-gated).lib/index.d.tsauto-merged cleanly (main's rebuild + the branch's multi-line quality type export).console.logline insrc/internal/calls/call-state-controller.ts(introduced on main by8dde503) so the merged tree passes the pinned prettier 3.9.5 gate.Validation (run on this branch at head
ab4a326):react-voice-commons-sdk/:npm run build— tsc clean;npm test— 16 suites, 138/138 passed;npm run lint— 0 errors, 243 warnings (241 documented branch baseline + 2 console-statement warnings from main's new call-state-controller guard, pre-existing on main)package/:npm test— 16 suites, 179/179 passed (incl. the 4 RTT-selection regression tests)npx prettier@3.9.5 --check "**/*.{js,jsx,ts,tsx,json,md}"(lockfile-pinned version) — all clean;npm run lint:md— clean. (Localnpm run quality:checkreports only 2 README warnings from the stale shared node_modules prettier 3.2.5 — the documented version-gap artifact; the lockfile pins 3.9.5.)npm run security:check: 83 vulnerabilities (20 moderate, 61 high, 2 critical) — pre-existing, advisory-only in CI. Main's own lockfile audits at 87 (23 moderate, 62 high, 2 critical) at the same registry date — the PR introduces no new advisories (growth is new upstream advisories on unchanged deps since the Oct-2 baseline of 29).CI on head
ab4a326: CodeQL ✓ · CodeQL Analysis ✓ · Formatting & Lint Check ✓ · Security Check ✓PR state:
mergeable=MERGEABLE(was CONFLICTING/DIRTY at0e2bb96)No merge/publish/deploy. VSDK-320 moved to In Review for bot review.