fix(plugin-meetings): skip empty ice candidates for Firefox #4003
Conversation
WalkthroughThe changes in this pull request enhance the handling of ICE candidates within the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
k-wasniowski
changed the title
k-wasniowski
added
the
validated
![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
🧹 Outside diff range and nitpick comments (7)
packages/@webex/plugin-meetings/src/meeting/index.ts (2)
6324-6325: Use optional chaining for cleaner null checksThe null checks can be simplified using optional chaining for better readability.
- if (event.candidate && event.candidate.candidate && event.candidate.candidate.length > 0) { + if (event.candidate?.candidate?.length > 0) {🧰 Tools
🪛 Biome (1.9.4)
[error] 6325-6325: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
6324-6326: Add comment explaining empty candidate handlingConsider adding a comment explaining why we skip empty ICE candidates, as this is specifically handling Firefox behavior.
+ // Skip empty ICE candidates that may be emitted by Firefox if (event.candidate && event.candidate.candidate && event.candidate.candidate.length > 0) { this.iceCandidatesCount += 1; }🧰 Tools
🪛 Biome (1.9.4)
[error] 6325-6325: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (5)
3814-3819: The change looks good, but add a test to verify the behavior.The change to use the provided headers from the mocked join request response is fine.
Consider adding a test case that verifies the expected behavior - that the
trackingidfrom the mocked join response headers is set correctly on theLocusMediaRequestinstance.
4012-4015: Refactor to extract common logic into a helper function.The logic for handling a microphone stream with ended readyState is duplicated for camera, screen share audio and screen share video streams.
Consider extracting this into a common helper function that takes the stream type and correlationId as parameters. This will reduce code duplication and improve maintainability.
function handleEndedStream(streamType, correlationId) { assert.throws( () => meeting.publishStreams(localStreams), `Attempted to publish ${streamType} stream with ended readyState, correlationId=${correlationId}` ); }Then replace the duplicated code blocks with calls to this helper:
handleEndedStream('microphone', meeting.correlationId);
4358-4358: Remove commented out code.Remove the commented out line
//calling handleDeviceLogging with audioEnaled as true adn videoEnabled as false.
Commented out code should not be checked in. If this comment is important, rephrase it and keep it, otherwise just delete it.
4365-4365: Fix typo in comment.Fix the typo in the comment:
//calling handleDeviceLogging audioEnabled as true videoEnabled as false.
It should be//calling handleDeviceLogging with audioEnabled as true and videoEnabled as false.
9174-9174: Remove extra blank line.Remove the extra blank line before the
assert.calledWith(meeting.webinar.updateWebcastUrl, newLocusResources);line to be consistent with the rest of the code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/@webex/plugin-meetings/src/meeting/index.ts(2 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js(15 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/@webex/plugin-meetings/src/meeting/index.ts
[error] 6325-6325: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (10)
packages/@webex/plugin-meetings/src/meeting/index.ts (1)
6324-6326: LGTM! Proper handling of empty ICE candidates
The core logic change correctly handles empty ICE candidates by checking for both existence and non-empty content before incrementing the counter. This fixes the issue with Firefox's ICE candidate handling.
🧰 Tools
🪛 Biome (1.9.4)
[error] 6325-6325: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (9)
93-94: Looks good, the empty ICE candidate is now skipped.
The change correctly skips counting empty ICE candidates.
380-383: Verify the callStateForMetrics is set correctly when correlation ID is not provided.
The change looks correct. When the correlation ID is not provided in the constructor, it uses the meeting ID as the correlation ID and sets it in callStateForMetrics.
To verify, check that when joining a meeting without providing a correlation ID:
- The meeting's correlationId property is set to the meeting ID
- The callStateForMetrics object has the correct correlationId and sessionCorrelationId
4028-4031: ****
This comment is a duplicate of the previous one for microphone stream. Please refer to that comment for the refactoring suggestion.
4044-4047: ****
This comment is a duplicate of the previous one for microphone stream. Please refer to that comment for the refactoring suggestion.
4060-4063: ****
This comment is a duplicate of the previous one for microphone stream. Please refer to that comment for the refactoring suggestion.
6293-6308: The error handling for MeetingInfoV2WebinarRegistrationError looks good.
The change correctly handles the MeetingInfoV2WebinarRegistrationError by:
- Rejecting with a
WebinarRegistrationError - Setting
meeting.meetingInfoto the meeting info from the error - Setting
meeting.meetingInfoFailureCodeandmeeting.meetingInfoFailureReasonappropriately
7023-7026: Verify the callStateForMetrics is updated correctly when setting a new correlationId.
The change looks correct. When setCorrelationId is called with a new correlation ID, it updates both the correlationId property and the callStateForMetrics.correlationId.
To verify, check that after calling meeting.setCorrelationId(newCorrelationId):
meeting.correlationIdis set tonewCorrelationIdmeeting.callStateForMetrics.correlationIdis also set tonewCorrelationId
7777-7781: The check for not collecting empty or null ICE candidates looks good.
The change correctly skips incrementing meeting.iceCandidatesCount when the ICE candidate is an empty string or null.
Line range hint 12332-12355: The LOCAL_UNMUTE_REQUIRED event handling looks good.
The changes look good:
- It correctly triggers the
MEETING_SELF_UNMUTED_BY_OTHERSevent with theunmuteAllowedpayload - It calls
meeting.audio.handleServerLocalUnmuteRequiredwith the right arguments
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/@webex/plugin-meetings/src/meeting/index.ts (1)
6324-6324: Consider using optional chaining for better readabilityThe validation logic is good but can be made more concise using optional chaining. This maintains the same null/undefined safety while being more readable.
-if (event.candidate && event.candidate.candidate && event.candidate.candidate.length > 0) { +if (event.candidate?.candidate?.length > 0) { this.iceCandidatesCount += 1; }Consider also adding a comment explaining that this check is needed specifically for Firefox which can emit empty ICE candidates.
🧰 Tools
🪛 Biome (1.9.4)
[error] 6324-6324: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/@webex/plugin-meetings/src/meeting/index.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/@webex/plugin-meetings/src/meeting/index.ts
[error] 6324-6324: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/@webex/plugin-meetings/src/meeting/index.ts (1)
6326-6328: LGTM! Consider using optional chaining for more concise validation.The additional validation for empty ICE candidates is a good defensive programming practice, especially for Firefox compatibility. The code can be made slightly more concise using optional chaining.
- if (event.candidate && event.candidate.candidate && event.candidate.candidate.length > 0) { + if (event.candidate?.candidate?.length > 0) { this.iceCandidatesCount += 1; }🧰 Tools
🪛 Biome (1.9.4)
[error] 6326-6326: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/@webex/plugin-meetings/src/meeting/index.ts(1 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js
🧰 Additional context used
🪛 Biome (1.9.4)
packages/@webex/plugin-meetings/src/meeting/index.ts
[error] 6326-6326: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
k-wasniowski
deleted the
fix-ice-candidates-skip-firefox-empty-candidate
branch
PART OF #SPARK-584958
This pull request addresses
Added skipping of empty ice candidates.
by making the following changes
Skip empty ice candidates counting
Change Type
The following scenarios were tested
Tested with JS-SDK
I certified that
I have read and followed contributing guidelines
I discussed changes with code owners prior to submitting this pull request
I have not skipped any automated checks
All existing and new tests passed
I have updated the documentation accordingly
Make sure to have followed the contributing guidelines before submitting.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor