fix: ignore late ICE candidates after cleanup for RN speech detector#2193
Merged
santhoshvai merged 1 commit intomainfrom Apr 9, 2026
Merged
fix: ignore late ICE candidates after cleanup for RN speech detector#2193santhoshvai merged 1 commit intomainfrom
santhoshvai merged 1 commit intomainfrom
Conversation
|
santhoshvai
changed the title
santhoshvai
changed the title
Contributor
📝 WalkthroughWalkthroughThe RNSpeechDetector class is refactored to use readonly peer connection fields, add an Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/client/src/helpers/RNSpeechDetector.ts (1)
20-70:⚠️ Potential issue | 🟠 MajorEnsure partial
start()failures run full cleanup.If any step throws after Line 43 listener registration or after Line 56 subscription setup, the catch block (Line 66) only logs and returns a noop, leaving resources/listeners active.
Proposed fix
public async start(onSoundDetectedStateChanged: SoundStateChangeHandler) { + let onPc1IceCandidate: + | ((e: RTCPeerConnectionIceEvent) => void) + | undefined; + let onPc2IceCandidate: + | ((e: RTCPeerConnectionIceEvent) => void) + | undefined; + let onTrackPc2: ((e: RTCTrackEvent) => void) | undefined; + let unsubscribe: (() => void) | undefined; try { this.isStopped = false; @@ - const onPc1IceCandidate = (e: RTCPeerConnectionIceEvent) => { + onPc1IceCandidate = (e: RTCPeerConnectionIceEvent) => { this.forwardIceCandidate(this.pc2, e.candidate); }; - const onPc2IceCandidate = (e: RTCPeerConnectionIceEvent) => { + onPc2IceCandidate = (e: RTCPeerConnectionIceEvent) => { this.forwardIceCandidate(this.pc1, e.candidate); }; - const onTrackPc2 = (e: RTCTrackEvent) => { + onTrackPc2 = (e: RTCTrackEvent) => { @@ - const unsubscribe = this.onSpeakingDetectedStateChange( + unsubscribe = this.onSpeakingDetectedStateChange( onSoundDetectedStateChanged, ); return () => { @@ } catch (error) { + if (onPc1IceCandidate) { + this.pc1.removeEventListener('icecandidate', onPc1IceCandidate); + } + if (onPc2IceCandidate) { + this.pc2.removeEventListener('icecandidate', onPc2IceCandidate); + } + if (onTrackPc2) { + this.pc2.removeEventListener('track', onTrackPc2); + } + unsubscribe?.(); + this.stop(); const logger = videoLoggerSystem.getLogger('RNSpeechDetector'); logger.error('error handling permissions: ', error); return () => {}; } }As per coding guidelines: "Unregister event listeners in cleanup/disposal code to prevent memory leaks".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/client/src/helpers/RNSpeechDetector.ts` around lines 20 - 70, The try block can throw after listeners (onPc1IceCandidate, onPc2IceCandidate, onTrackPc2) and after subscription (unsubscribe) are created, but the catch only logs and returns a noop leaving listeners, tracks and peers active; add a deterministic cleanup path and call it from the catch (or use finally). Implement a local cleanup routine inside start() that removes the three listeners from this.pc1/this.pc2, calls unsubscribe() if it was set, stops and nulls audioStream tracks, and calls this.stop() / closes pc1/pc2 as appropriate, then invoke that cleanup from the catch so partial failures free resources and prevent leaks.
🧹 Nitpick comments (1)
packages/client/src/helpers/__tests__/RNSpeechDetector.test.ts (1)
31-50: Extend this regression to cover both ICE forwarding directions.Great regression test for
pc1 -> pc2. Since the implementation also wirespc2 -> pc1, add a symmetric assertion for the reverse handler to prevent half-fixed regressions.Based on learnings: "Add regression tests for bug fixes".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/client/src/helpers/__tests__/RNSpeechDetector.test.ts` around lines 31 - 50, The test currently simulates a late ICE candidate from pc1 and asserts it was removed and not forwarded to pc2; extend it symmetrically for pc2->pc1 by locating the second registered ICE callback from rtcPeerConnectionMockCtor.mock.results (use the same pattern that finds onIceCandidate), invoke that callback with a fake ICE candidate, then assert pc2.removeEventListener was called with that handler and that pc1.addIceCandidate was not called; reference rtcPeerConnectionMockCtor, pc1, pc2, addEventListener, removeEventListener, and addIceCandidate when making the additions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/client/src/helpers/RNSpeechDetector.ts`:
- Around line 20-70: The try block can throw after listeners (onPc1IceCandidate,
onPc2IceCandidate, onTrackPc2) and after subscription (unsubscribe) are created,
but the catch only logs and returns a noop leaving listeners, tracks and peers
active; add a deterministic cleanup path and call it from the catch (or use
finally). Implement a local cleanup routine inside start() that removes the
three listeners from this.pc1/this.pc2, calls unsubscribe() if it was set, stops
and nulls audioStream tracks, and calls this.stop() / closes pc1/pc2 as
appropriate, then invoke that cleanup from the catch so partial failures free
resources and prevent leaks.
---
Nitpick comments:
In `@packages/client/src/helpers/__tests__/RNSpeechDetector.test.ts`:
- Around line 31-50: The test currently simulates a late ICE candidate from pc1
and asserts it was removed and not forwarded to pc2; extend it symmetrically for
pc2->pc1 by locating the second registered ICE callback from
rtcPeerConnectionMockCtor.mock.results (use the same pattern that finds
onIceCandidate), invoke that callback with a fake ICE candidate, then assert
pc2.removeEventListener was called with that handler and that
pc1.addIceCandidate was not called; reference rtcPeerConnectionMockCtor, pc1,
pc2, addEventListener, removeEventListener, and addIceCandidate when making the
additions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 816bdcfc-bab2-48dd-a4e2-b693bc268c18
📒 Files selected for processing (2)
packages/client/src/helpers/RNSpeechDetector.tspackages/client/src/helpers/__tests__/RNSpeechDetector.test.ts
oliverlaz
approved these changes
Apr 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
💡 Overview
Properly remove event listeners early in RN-speech-detector and avoid sending invalid candidates to native side
Summary by CodeRabbit
Bug Fixes
Tests