fix: add ice candidates gathering timeout config #3951
Conversation
WalkthroughThis pull request introduces enhancements to the Webex Meetings plugin, focusing on media connection handling. A new property, Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
![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
🧹 Outside diff range and nitpick comments (4)
packages/@webex/plugin-meetings/src/config.ts (1)
95-95: Add documentation and type information for the new configuration.The new
iceCandidatesGatheringTimeoutconfiguration lacks documentation explaining its purpose, expected values, and default behavior. Additionally, since this is a TypeScript file, consider adding type information.Apply this diff to add documentation and type information:
+ /** + * Timeout duration (in milliseconds) for gathering ICE candidates during transcoded meetings. + * When undefined, the gathering process will not timeout. + */ - iceCandidatesGatheringTimeout: undefined, + iceCandidatesGatheringTimeout: number | undefined,packages/@webex/plugin-meetings/test/unit/spec/media/index.ts (2)
86-86: LGTM, but consider adding more test cases.The addition of
iceCandidatesTimeoutparameter is correct. However, consider adding test cases for:
- A specific timeout value (e.g., 5000ms)
- Edge cases (0, negative values)
Line range hint
86-318: Overall feedback on test coverage.The test changes appropriately cover the new
iceCandidatesTimeoutparameter. However, to ensure robust testing of this timeout feature, consider:
- Adding test cases for the timeout behavior:
- Test with specific timeout values
- Test timeout expiration scenarios
- Test edge cases (0, negative values)
- Adding integration tests to verify the actual ICE candidate gathering timeout behavior
Would you like me to provide example test cases for these scenarios?
packages/@webex/plugin-meetings/src/media/index.ts (1)
147-147: Add JSDoc documentation for iceCandidatesTimeout parameterConsider adding JSDoc documentation for the new parameter to help users understand its purpose and expected values. For example:
* @param {BundlePolicy} [options.bundlePolicy] + * @param {number} [options.iceCandidatesTimeout] Timeout in milliseconds for gathering ICE candidates */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
packages/@webex/plugin-meetings/src/config.ts(1 hunks)packages/@webex/plugin-meetings/src/media/index.ts(3 hunks)packages/@webex/plugin-meetings/src/meeting/index.ts(1 hunks)packages/@webex/plugin-meetings/test/unit/spec/media/index.ts(3 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js(1 hunks)
🔇 Additional comments (8)
packages/@webex/plugin-meetings/src/config.ts (1)
95-95: Verify the relationship with existing ICE timeout configuration.
The codebase already has an iceReconnectionTimeout under the reconnection section. While they serve different purposes (gathering vs reconnection), we should ensure their interaction is well-defined and documented.
Let's analyze the usage of both timeouts:
packages/@webex/plugin-meetings/test/unit/spec/media/index.ts (2)
104-104: LGTM! Proper assertion of configuration parameter.
The test correctly verifies that the iceCandidatesTimeout is passed through to the RoapMediaConnection constructor.
311-311: LGTM! Good edge case coverage.
The test properly verifies that iceCandidatesTimeout is handled correctly even when ICE servers are not available. This is an important edge case.
Let's verify if there are any other edge cases we should test:
Also applies to: 318-318
✅ Verification successful
Based on the search results, I can now generate the final response:
LGTM - Edge case coverage is comprehensive
The test coverage for iceCandidatesTimeout is thorough across the codebase:
- Tested in combination with undefined ICE servers in media tests
- Verified in turn discovery timeout scenarios (10s timeout case)
- Covered in various edge cases including:
- When TURN discovery is skipped due to reachability
- When TURN server info is undefined
- When ICE servers are not available
- When TURN discovery response is missing or incomplete
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other test files that might need similar coverage
fd -e ts -e js "test.*media" --exec rg -l "turnServerInfo|iceServers"
Length of output: 71
Script:
#!/bin/bash
# Let's try a broader search for test files related to media and ICE
rg -l "turnServerInfo|iceServers" packages/@webex/plugin-meetings/test/
Length of output: 428
Script:
#!/bin/bash
# Search for media test files specifically
fd -t f "media.*spec" packages/@webex/plugin-meetings/test/
Length of output: 61
Script:
#!/bin/bash
# Look for ICE-related test cases in the entire test directory
rg -l "describe.*ICE|test.*ICE" packages/@webex/plugin-meetings/test/
Length of output: 135
Script:
#!/bin/bash
# Let's examine the ICE-related test cases in these files to see their coverage
rg -A 5 -B 5 "turnServerInfo|iceServers" packages/@webex/plugin-meetings/test/unit/spec/roap/turnDiscovery.ts packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js
Length of output: 54753
Script:
#!/bin/bash
# Check if there are any other timeout-related tests
rg "timeout.*test|test.*timeout" packages/@webex/plugin-meetings/test/unit/spec/media/
Length of output: 88
packages/@webex/plugin-meetings/src/media/index.ts (3)
147-147: LGTM: Type definition for iceCandidatesTimeout is well-defined
The optional number type is appropriate for a timeout configuration parameter.
158-158: LGTM: Parameter destructuring is correctly implemented
The iceCandidatesTimeout parameter is properly extracted from the options object.
215-215: Verify if iceCandidatesTimeout should be supported for multistream connections
The timeout configuration is only passed to RoapMediaConnection but not to MultistreamRoapMediaConnection. Please verify if this is intentional or if multistream connections should also support ICE candidates gathering timeout.
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (2)
Line range hint 32-38: LGTM!
The imports look good and are in the correct order.
Line range hint 3707-3738: Verify the new iceCandidatesTimeout config value is handled correctly.
The changes look good, but please ensure that the new meeting.config.iceCandidatesGatheringTimeout value is being passed correctly to the RoapMediaConnection and MultistreamRoapMediaConnection constructors.
You should also add some new unit tests to verify that this config value is being used as expected.
To verify, search the codebase for usages of meeting.config.iceCandidatesGatheringTimeout and make sure it's passed correctly. Also, add a new unit test like this:
it('passes the iceCandidatesTimeout config to the media connection', async () => {
meeting.config.iceCandidatesGatheringTimeout = 5000;
await meeting.addMedia();
assert.calledWith(
roapMediaConnectionConstructorStub,
sinon.match({
iceCandidatesTimeout: 5000
})
);
});| // @ts-ignore - config coming from registerPlugin | ||
| iceCandidatesTimeout: this.config.iceCandidatesGatheringTimeout, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove @ts-ignore by updating TypeScript definitions
The use of @ts-ignore suppresses TypeScript errors and can hide potential issues. It's better to update the TypeScript definitions so that this.config includes iceCandidatesGatheringTimeout.
Apply this diff to update the type definitions and remove @ts-ignore:
+// In the appropriate type definition file (e.g., config.ts)
+export interface MeetingConfig {
+ // existing properties...
+ iceCandidatesGatheringTimeout?: number;
+}
// In the createMediaConnection function
- // @ts-ignore - config coming from registerPlugin
iceCandidatesTimeout: this.config.iceCandidatesGatheringTimeout,Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
packages/@webex/plugin-meetings/src/config.ts (1)
95-95: Add documentation for the new configuration property.Please add a comment explaining the purpose and units of
iceCandidatesGatheringTimeout, similar to howiceReconnectionTimeoutis documented. This will help users understand when and how to use this configuration.Example:
+ // Timeout duration (in milliseconds) to wait for ICE candidates gathering iceCandidatesGatheringTimeout: undefined,packages/@webex/plugin-meetings/src/media/index.ts (1)
215-215: Consider adding validation and documentation for iceCandidatesTimeoutWhile the implementation is correct, consider the following improvements:
- Add validation for the timeout value to ensure it's a positive number
- Add JSDoc documentation specifying:
- The expected range/format of the timeout value
- The unit of measurement (milliseconds/seconds)
- The default behavior when not provided
Example documentation:
/** * @param {Object} options * @param {number} [options.iceCandidatesTimeout] Timeout in milliseconds for gathering ICE candidates. * Must be a positive number. Default: no timeout */packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (1)
Line range hint
3707-3738: Consider adding a test case for the newiceCandidatesTimeoutconfig option.It would be good to add a test case that verifies the
iceCandidatesTimeoutconfig option is correctly passed to theRoapMediaConnectionorMultistreamRoapMediaConnectionconstructor.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
packages/@webex/plugin-meetings/src/config.ts(1 hunks)packages/@webex/plugin-meetings/src/media/index.ts(3 hunks)packages/@webex/plugin-meetings/src/meeting/index.ts(1 hunks)packages/@webex/plugin-meetings/test/unit/spec/media/index.ts(3 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js(1 hunks)
🔇 Additional comments (7)
packages/@webex/plugin-meetings/src/config.ts (1)
95-95: Verify interaction with existing ICE timeout configurations.
The implementation looks good. However, let's verify how this new timeout interacts with the existing iceReconnectionTimeout to ensure there are no conflicts or edge cases.
✅ Verification successful
Let me analyze the ICE timeout configurations and their interactions by searching for their usage.
Based on the search results, I can now provide a conclusive response about the interaction between the ICE timeout configurations.
No conflicts found between ICE timeout configurations
The iceCandidatesGatheringTimeout and iceReconnectionTimeout serve different purposes and operate independently:
iceReconnectionTimeout(10000ms) is used by the reconnection manager to handle ICE connection lossiceCandidatesGatheringTimeout(undefined by default) is passed asiceCandidatesTimeoutduring media connection creation
The timeouts are used in different contexts and don't interfere with each other:
- Reconnection: handles recovery after ICE disconnection
- Gathering: controls the initial ICE candidate collection phase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any other ICE-related timeout configurations and their usage
# to ensure proper integration of the new timeout.
# Search for ICE-related configurations
echo "Searching for ICE-related configurations..."
rg -i "ice.*timeout" --type ts
# Search for places where timeouts are consumed
echo "Searching for timeout usage in media connection code..."
rg -i "createMediaConnection.*timeout" --type ts
Length of output: 7567
Script:
#!/bin/bash
# Search for the usage of iceCandidatesGatheringTimeout and iceReconnectionTimeout
echo "Searching for iceCandidatesGatheringTimeout usage..."
rg -A 5 "iceCandidatesGatheringTimeout" --type ts
echo -e "\nSearching for iceReconnectionTimeout usage..."
rg -A 5 "iceReconnectionTimeout" --type ts
# Search for media connection creation to understand timeout handling
echo -e "\nSearching for media connection creation..."
ast-grep --pattern 'createMediaConnection($$$)'
Length of output: 2217
packages/@webex/plugin-meetings/test/unit/spec/media/index.ts (3)
Line range hint 1-353: Add test coverage for MultistreamRoapMediaConnection with iceCandidatesTimeout.
The test file covers iceCandidatesTimeout for RoapMediaConnection but lacks equivalent coverage for MultistreamRoapMediaConnection. This creates a gap in test coverage for multistream scenarios.
#!/bin/bash
# Check for MultistreamRoapMediaConnection tests with timeout
rg "MultistreamRoapMediaConnection.*iceCandidatesTimeout" --type tsAdd similar test cases for MultistreamRoapMediaConnection:
- Basic parameter passing with different timeout values
- Integration with ICE gathering process
- Error handling for timeout scenarios
311-311: Add integration test for ICE gathering timeout behavior.
The test suite verifies the parameter passing but lacks coverage for the actual timeout behavior during ICE candidate gathering. Consider adding an integration test that:
- Sets up a media connection with a specific timeout
- Verifies that the ICE gathering process times out as expected
- Ensures proper error handling when timeout occurs
#!/bin/bash
# Check if there are any integration tests for ICE gathering
rg -l "ice.*gather.*timeout" --type tsHere's a suggested integration test outline:
it('times out ICE gathering after specified duration', async () => {
const clock = sinon.useFakeTimers();
const roapMediaConnection = new RoapMediaConnection({
iceCandidatesTimeout: 5000,
// other required options...
});
// Start ICE gathering
const gatheringPromise = roapMediaConnection.startGathering();
// Advance timer past timeout
await clock.tickAsync(6000);
// Verify gathering was interrupted
await assert.isRejected(gatheringPromise, /ICE gathering timeout/);
clock.restore();
});Also applies to: 318-318
86-86: 🛠️ Refactor suggestion
Enhance test coverage for iceCandidatesTimeout parameter.
While the test verifies that iceCandidatesTimeout is passed through to RoapMediaConnection, it only covers the undefined case. Consider adding test cases for:
- When
iceCandidatesTimeoutis set to a specific number - When
iceCandidatesTimeoutis set to 0 - Edge cases (negative numbers, null)
Here's a suggested test case to add:
it('passes iceCandidatesTimeout correctly to RoapMediaConnection', () => {
const roapMediaConnectionConstructorStub = sinon
.stub(InternalMediaCoreModule, 'RoapMediaConnection')
.returns(fakeRoapMediaConnection);
// Test with a specific timeout value
Media.createMediaConnection(false, 'debug id', 'meetingId', {
mediaProperties: {
mediaDirection: {/* ... */},
},
iceCandidatesTimeout: 5000,
});
assert.calledWith(
roapMediaConnectionConstructorStub,
sinon.match({
iceCandidatesTimeout: 5000,
}),
sinon.match.any,
sinon.match.any
);
// Test with zero timeout
Media.createMediaConnection(false, 'debug id', 'meetingId', {
mediaProperties: {
mediaDirection: {/* ... */},
},
iceCandidatesTimeout: 0,
});
assert.calledWith(
roapMediaConnectionConstructorStub,
sinon.match({
iceCandidatesTimeout: 0,
}),
sinon.match.any,
sinon.match.any
);
});Also applies to: 104-104
packages/@webex/plugin-meetings/src/media/index.ts (3)
147-147: LGTM: Type definition for iceCandidatesTimeout is correct
The optional number type for iceCandidatesTimeout is appropriate and maintains backward compatibility.
158-158: LGTM: Parameter destructuring follows existing pattern
The iceCandidatesTimeout is correctly destructured alongside other configuration options.
Line range hint 147-215: Verify integration with MultistreamRoapMediaConnection
The iceCandidatesTimeout is only passed to RoapMediaConnection but not to MultistreamRoapMediaConnection. Please verify if this is intentional or if the timeout should also be supported for multistream connections.
| // @ts-ignore - config coming from registerPlugin | ||
| iceCandidatesTimeout: this.config.iceCandidatesGatheringTimeout, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using @ts-ignore; update type definitions for this.config
Using // @ts-ignore suppresses TypeScript errors, which can hide legitimate type issues. Instead, update the type definitions to include iceCandidatesGatheringTimeout in the MeetingConfig interface. This ensures type safety and maintains the benefits of TypeScript's checking.
To fix this, remove the // @ts-ignore directive:
- // @ts-ignore - config coming from registerPlugin
iceCandidatesTimeout: this.config.iceCandidatesGatheringTimeout,Then, update the MeetingConfig interface to include iceCandidatesGatheringTimeout:
// In config.ts or the relevant type definition file
export interface MeetingConfig {
// other properties
iceCandidatesGatheringTimeout?: number;
}
COMPLETES #SPARK-576423
This pull request addresses
Request from clients to be able to configure ice candidates gathering timeout.
by making the following changes
Enable setting ice candidates gathering timeout for transcoded meetings.
Change Type
The following scenarios were tested
Tested manually in JS-SDK sample app. Tested both cases, when timeout is set or not.
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
iceCandidatesGatheringTimeoutfor enhanced meeting configuration.iceCandidatesTimeoutparameter to improve media connection management.Bug Fixes
Documentation