feat(meetings): added support for "early-call-min-clusters" option in Orpheus API #4013
Conversation
WalkthroughThe changes in this pull request focus on enhancing media handling and error management within the Changes
Possibly related PRs
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
warning [email protected]: This version is no longer supported. Please see https://eslint.org/version-support for other options. (For a CapTP with native promises, see @endo/eventual-send and @endo/captp) 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🔇 Additional comments (3)packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (3)
The added stubs for
The test correctly verifies that:
This provides good test coverage for the reachability enhancements.
The test correctly checks that the reachability report and client media preferences are included in follow-up requests as well, ensuring consistent behavior throughout the meeting lifecycle. 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). |
marcin-bazyl
force-pushed
the
spark-577185
branch
from
589152e to
a8072cd
Compare
marcin-bazyl
changed the title
marcin-bazyl
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: 13
🧹 Outside diff range and nitpick comments (23)
packages/@webex/plugin-meetings/test/unit/spec/roap/request.ts (2)
85-127: Consider adding more test scenariosThe test effectively verifies the happy path for sendRoap with the new clientMediaPreferences and reachability integration. However, consider adding tests for:
- Error scenarios
- Different isMultistream values
- Edge cases with various clientMediaPreferences configurations
Example additional test:
it('handles errors when reachability data cannot be retrieved', async () => { webex.meetings.reachability.getReachabilityReportToAttachToRoap.rejects(new Error('Network error')); // Test error handling }); it('works correctly with isMultistream set to false', async () => { // Test with different isMultistream value });
129-181: Remove commented out test codeThe commented out test for
attachReachabilityDataappears to be obsolete now that we have the newsendRoaptest implementation. Keeping commented out code can lead to maintenance confusion.Consider removing this block entirely or if it's needed for reference, document it properly in the codebase's documentation instead.
packages/@webex/plugin-meetings/test/unit/spec/meeting/locusMediaRequest.ts (1)
37-49: LGTM with a suggestion for timestamp handling!The clientMediaPreferences structure is well-defined and matches the expected schema. Consider using a constant or helper function for the timestamp format to ensure consistency across tests.
- timeShot: '2023-05-23T08:03:49Z', + timeShot: TEST_TIMESTAMP, // Define at the top of the filepackages/@webex/plugin-meetings/src/roap/turnDiscovery.ts (2)
452-453: LGTM! Consider updating documentationThe implementation maintains consistency with
sendRoapTurnDiscoveryRequestby adding the same multistream support parameters.Consider updating the method's JSDoc to document the new parameters being passed in the ROAP message:
/** * Sends the OK message that server expects to receive * after it sends us TURN_DISCOVERY_RESPONSE * * @param {Meeting} meeting + * @remarks The ROAP message includes isMultistream and locusMediaRequest parameters from the meeting object * @returns {Promise} */
411-412: Consider adding parameter validationWhile the implementation is solid, consider adding validation for the new parameters to ensure robust error handling.
Example validation in both methods:
private sendRoapTurnDiscoveryRequest( meeting: Meeting, isReconnecting: boolean ): Promise<TurnDiscoveryResult> { + if (meeting.isMultistream === undefined) { + LoggerProxy.logger.warn( + 'Roap:turnDiscovery#sendRoapTurnDiscoveryRequest --> isMultistream is undefined' + ); + } + if (this.defer) { // ... existing codeAlso applies to: 452-453
packages/@webex/plugin-meetings/src/meeting/request.ts (1)
Line range hint
4-4: Add unit tests for the newclientMediaPreferencesfunctionalityThe TODO comment indicates missing tests. Please add unit tests to verify:
- Handling of valid
clientMediaPreferencesobjects- Validation of invalid properties
- Integration with the join meeting flow
Would you like me to help generate the unit test cases or create a GitHub issue to track this task?
packages/@webex/plugin-meetings/test/unit/spec/meeting/request.js (2)
Line range hint
199-208: Enhance test data with realistic mock object.Instead of using a string literal, consider using a mock object that accurately represents the expected structure of
clientMediaPreferences. This would provide better test coverage and documentation of the expected format.- const clientMediaPreferences = 'clientMediaPreferences'; + const clientMediaPreferences = { + earlyCallMinClusters: 2, + // Add other relevant properties based on API requirements + };
Line range hint
199-219: Add test cases for edge cases and validation.The current test coverage for
clientMediaPreferencesis limited to the happy path. Consider adding test cases for:
- Invalid values for
earlyCallMinClusters- Missing
clientMediaPreferences- Boundary conditions
Example additional test:
it('handles missing clientMediaPreferences gracefully', async () => { await meetingsRequest.joinMeeting({ locusUrl, deviceUrl, correlationId, roapMessage, reachability, permissionToken }); const requestParams = meetingsRequest.request.getCall(0).args[0]; assert.isUndefined(requestParams.body.clientMediaPreferences); }); it('validates earlyCallMinClusters value', async () => { const invalidPreferences = { earlyCallMinClusters: -1 // Invalid value }; await assert.isRejected(meetingsRequest.joinMeeting({ locusUrl, deviceUrl, correlationId, roapMessage, reachability, permissionToken, clientMediaPreferences: invalidPreferences })); });packages/@webex/plugin-meetings/test/unit/spec/roap/turnDiscovery.ts (1)
872-1014: Consider extracting common setup code in test cases.While the new test cases provide good coverage for HTTP response handling and abort functionality, there's repeated setup code that could be extracted to improve maintainability.
Consider extracting the common setup code into helper functions:
+ const setupHttpResponseTest = (roapMessage) => { + return { + mediaConnections: [{ + remoteSdp: JSON.stringify({roapMessage}) + }] + }; + }; + + const setupMockRoapRequest = () => { + return sinon.fake.resolves({ + mediaConnections: [{ + mediaId: '464ff97f-4bda-466a-ad06-3a22184a2274', + remoteSdp: `{"roapMessage": {"messageType":"TURN_DISCOVERY_RESPONSE","seq":"0","headers": ["x-cisco-turn-url=${FAKE_TURN_URL}","x-cisco-turn-username=${FAKE_TURN_USERNAME}","x-cisco-turn-password=${FAKE_TURN_PASSWORD}", "noOkInTransaction"]}}` + }] + }); + }; describe('handleTurnDiscoveryHttpResponse', () => { it('works as expected when called with valid httpResponse', async () => { - const httpResponse = {mediaConnections: [{remoteSdp: JSON.stringify({roapMessage})}]}; + const httpResponse = setupHttpResponseTest(roapMessage); // ... rest of the test }); }); describe('abort', () => { it('allows starting a new TURN discovery', async () => { - mockRoapRequest.sendRoap = sinon.fake.resolves({ - mediaConnections: [ - { - mediaId: '464ff97f-4bda-466a-ad06-3a22184a2274', - remoteSdp: `{"roapMessage": {"messageType":"TURN_DISCOVERY_RESPONSE","seq":"0","headers": ["x-cisco-turn-url=${FAKE_TURN_URL}","x-cisco-turn-username=${FAKE_TURN_USERNAME}","x-cisco-turn-password=${FAKE_TURN_PASSWORD}", "noOkInTransaction"]}}` - } - ] - }); + mockRoapRequest.sendRoap = setupMockRoapRequest(); // ... rest of the test }); });packages/@webex/plugin-meetings/test/unit/spec/meeting/utils.js (1)
417-449: Consider enhancing test coverage and mock objects.The test case properly verifies the new reachability functionality, but could be improved in the following ways:
- Add negative test cases for different IP support scenarios
- Use more realistic mock objects that match the expected structure
- Update the test description to be more specific about testing reachability integration
Consider adding these test cases:
it('should handle IPv4-only environment', async () => { meeting.isMultistream = true; sinon.stub(webex.internal.device.ipNetworkDetector, 'supportsIpV4').get(() => true); sinon.stub(webex.internal.device.ipNetworkDetector, 'supportsIpV6').get(() => false); await MeetingUtil.joinMeeting(meeting, {}); assert.calledWith( webex.meetings.reachability.getClientMediaPreferences, meeting.isMultistream, IP_VERSION.only_ipv4 ); }); it('should use realistic reachability data', async () => { const REALISTIC_REACHABILITY_REPORT = { mediaConnections: [{ mediaType: 'audio', candidates: [], iceServers: [] }], reachabilityStatus: 'success' }; webex.meetings.reachability.getReachabilityReportToAttachToRoap.resolves(REALISTIC_REACHABILITY_REPORT); await MeetingUtil.joinMeeting(meeting, {}); assert.equal(parameter.reachability, REALISTIC_REACHABILITY_REPORT); });packages/@webex/plugin-meetings/src/reachability/request.ts (5)
35-36: Add documentation for the 'previousReport' parameterThe
previousReportparameter lacks a JSDoc comment. Please add documentation to explain its purpose and expected structure.
41-41: Specify a more precise type for 'previousReport' parameterThe
previousReportparameter is currently typed asany. Consider defining a specific interface or type to enhance type safety and code clarity.
44-45: Provide specific types for 'joinCookie' and 'discoveryOptions' in the return type
joinCookieis typed asany, anddiscoveryOptionsis typed asRecord<string, any>. Defining specific interfaces or types for these will improve type safety and maintainability.
39-39: Define a specific type or enum for the 'trigger' parameterThe
triggerparameter appears to accept specific string values like'startup'. Consider defining a string literal union type or an enum to restrict allowed values and prevent potential typos at compile time.
48-53: Refactor 'callWrapper' assignment for improved readabilityThe multi-line ternary operator used in assigning
callWrappercan be difficult to read. Refactoring the logic using anif-elsestatement or extracting functions into named variables can enhance readability.Consider refactoring as follows:
let callWrapper; if (trigger === 'startup') { callWrapper = this.webex.internal.newMetrics.callDiagnosticLatencies.measureLatency.bind( this.webex.internal.newMetrics.callDiagnosticLatencies ); } else { callWrapper = (func) => func(); }packages/@webex/plugin-meetings/test/unit/spec/reachability/request.js (1)
66-116: Consider refactoring tests to reduce duplicationThe two test cases share a significant amount of code. Consider refactoring by parameterizing the trigger value and consolidating common logic to enhance maintainability and reduce code duplication.
packages/@webex/plugin-meetings/src/reachability/index.ts (5)
87-89: Consider initializing optional class propertiesThe properties
minRequiredClustersandorpheusApiVersionare declared but not initialized. If they should have default values, consider initializing them in the constructor to avoid potentialundefinedissues.
134-134: Specify a more precise type instead ofanyforpreviousReportThe
previousReportparameter is currently typed asany. To leverage TypeScript's type-checking capabilities and improve code safety, consider defining a specific interface or type forpreviousReport.
217-219: Correct the function name in error loggingThe error log references
MeetingRequest#constructor, but the error occurs inReachability#getJoinCookie. Update the log message to reflect the correct function name for accurate debugging.Apply this diff to correct the log message:
-`MeetingRequest#constructor --> Error in parsing join cookie data: ${e}` +`Reachability#getJoinCookie --> Error in parsing join cookie data: ${e}`
786-791: Type safety inclearTimermethodIn the
clearTimermethod, accessingthis[timer]may not be type-safe becausetimeris a string. Consider defining a type or an enum for the timer names to ensure TypeScript can check the validity ofthis[timer].
963-983: SimplifygetClientMediaPreferencesmethodThe method contains duplicate code for setting
ipverandpreferTranscoding. Consider simplifying the return statements to reduce redundancy.Apply this diff to streamline the method:
async getClientMediaPreferences(isMultistream: boolean, ipver?: IP_VERSION) { const basePreferences = { ipver, preferTranscoding: !isMultistream, }; if (!this.orpheusApiVersion) { return { ...basePreferences, joinCookie: await this.getJoinCookie(), }; } const reachabilityReport = await this.getReachabilityReport(); return { ...basePreferences, ...reachabilityReport, }; }packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (2)
3689-3690: Verify that getClientMediaPreferences() and getReachabilityReportToAttachToRoap() are called with the right parametersThe code checks that
meeting.webex.meetings.reachability.getClientMediaPreferences()andmeeting.webex.meetings.reachability.getReachabilityReportToAttachToRoap()are called with the expected parameters when sending an SDP offer.Consider also verifying:
- The return values of these functions are included in the /media request body as expected
- Error handling if these functions throw or return unexpected values
3918-3922: Verify that getClientMediaPreferences() is called with the right parametersSimilar to the previous comment, the code checks that
meeting.webex.meetings.reachability.getClientMediaPreferences()is called with the expected parameters when sending a ROAP OK message.Consider also verifying:
- The return value is included in the /media request body as expected
- Error handling if the function throws or returns an unexpected value
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (18)
packages/@webex/plugin-meetings/src/meeting/index.ts(0 hunks)packages/@webex/plugin-meetings/src/meeting/locusMediaRequest.ts(2 hunks)packages/@webex/plugin-meetings/src/meeting/request.ts(3 hunks)packages/@webex/plugin-meetings/src/meeting/util.ts(3 hunks)packages/@webex/plugin-meetings/src/reachability/index.ts(18 hunks)packages/@webex/plugin-meetings/src/reachability/request.ts(1 hunks)packages/@webex/plugin-meetings/src/roap/index.ts(4 hunks)packages/@webex/plugin-meetings/src/roap/request.ts(3 hunks)packages/@webex/plugin-meetings/src/roap/turnDiscovery.ts(2 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js(4 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/locusMediaRequest.ts(2 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/request.js(3 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/utils.js(2 hunks)packages/@webex/plugin-meetings/test/unit/spec/reachability/index.ts(1 hunks)packages/@webex/plugin-meetings/test/unit/spec/reachability/request.js(2 hunks)packages/@webex/plugin-meetings/test/unit/spec/roap/index.ts(1 hunks)packages/@webex/plugin-meetings/test/unit/spec/roap/request.ts(4 hunks)packages/@webex/plugin-meetings/test/unit/spec/roap/turnDiscovery.ts(15 hunks)
💤 Files with no reviewable changes (1)
- packages/@webex/plugin-meetings/src/meeting/index.ts
🔇 Additional comments (32)
packages/@webex/plugin-meetings/test/unit/spec/roap/request.ts (2)
27-30: LGTM: Test setup properly initialized
The setup correctly stubs the new reachability methods and IP version functionality needed for the updated tests.
Also applies to: 45-46
Line range hint 1-181: Verify complete feature coverage for early-call-min-clusters
While the test changes cover the new clientMediaPreferences and reachability integration, we should verify that all aspects of the 'early-call-min-clusters' feature are properly tested.
✅ Verification successful
Test coverage for early-call-min-clusters feature is adequate
The feature is well-tested across multiple test files with comprehensive coverage:
reachability/index.tstests various scenarios with differentearly-call-min-clustersvalues (0, 1, 2)reachability/request.jsverifies the feature in request handling- The current PR's test file (
roap/request.ts) properly tests the integration withgetClientMediaPreferencesand reachability reporting
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other test files that might need similar updates for early-call-min-clusters
rg -t typescript -l "early.*call.*min.*clusters" "packages/@webex/plugin-meetings/test"
# Check if there are any related configuration or constant definitions
rg -t typescript "early.*call.*min.*clusters" "packages/@webex/plugin-meetings/src"
Length of output: 241
Script:
#!/bin/bash
# Search for other test files that might need similar updates for early-call-min-clusters
rg -l "early.*call.*min.*clusters" "packages/@webex/plugin-meetings/test"
# Check if there are any related configuration or constant definitions
rg "early.*call.*min.*clusters" "packages/@webex/plugin-meetings/src"
# Look for reachability-related tests and implementations
rg -l "getClientMediaPreferences|getReachabilityReportToAttachToRoap" "packages/@webex/plugin-meetings/test"
rg -l "getClientMediaPreferences|getReachabilityReportToAttachToRoap" "packages/@webex/plugin-meetings/src"
Length of output: 1221
Script:
#!/bin/bash
# Check the content of reachability tests to verify early-call-min-clusters coverage
rg -A 10 "early.*call.*min.*clusters" "packages/@webex/plugin-meetings/test/unit/spec/reachability/index.ts"
rg -A 10 "early.*call.*min.*clusters" "packages/@webex/plugin-meetings/test/unit/spec/reachability/request.js"
# Check the implementation of getClientMediaPreferences to understand test coverage needs
ast-grep --pattern 'getClientMediaPreferences($_) {
$$$
}'
Length of output: 2664
packages/@webex/plugin-meetings/src/roap/index.ts (4)
144-145: LGTM! Consistent with sendRoapOK implementation.
The changes maintain consistency in how ROAP messages are handled across different message types.
173-174: LGTM! Error handling remains robust.
The changes maintain consistency with other ROAP message types while preserving proper error handling functionality.
210-212: Verify transcoding preference logic.
The inverse relationship between preferTranscoding and isMultistream suggests an important architectural decision. Please confirm:
- Is it intentional that transcoding is preferred only when multistream is disabled?
- Are there any edge cases where we might want transcoding even with multistream enabled?
✅ Verification successful
The inverse relationship between multistream and transcoding is intentionally designed.
The codebase shows consistent inverse relationship between preferTranscoding and isMultistream flags across multiple files:
- In roap/index.ts:
preferTranscoding: !meeting.isMultistream- In reachability/index.ts:
preferTranscoding: !isMultistreamThis pattern appears in both media request and reachability contexts, indicating it's a deliberate architectural decision to disable transcoding when multistream is enabled and vice versa. The lack of any documentation or comments about edge cases suggests this is the intended behavior without exceptions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other instances of preferTranscoding to verify consistency
rg "preferTranscoding" -B 2 -A 2
# Search for documentation or comments about the relationship between transcoding and multistream
rg -i "multistream.*transcoding|transcoding.*multistream" -g "*.{md,txt,js,ts}"
Length of output: 132387
110-111: LGTM! Verify meeting object properties.
The addition of isMultistream and locusMediaRequest properties aligns with the PR objectives. The properties are correctly accessed from the meeting object.
✅ Verification successful
Based on the search results, I can now generate the final response since we have found sufficient evidence about both properties:
Properties verified in Meeting class implementation
The properties are correctly defined and used in the Meeting class:
isMultistreamis defined as a boolean property and initialized tofalseby defaultlocusMediaRequestis defined as an optional property and properly instantiated withLocusMediaRequestclass- Both properties are used extensively throughout the codebase for media handling and ROAP message processing
The changes in the ROAP implementation are consistent with the codebase's architecture and the properties' intended usage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that Meeting class has these properties defined
ast-grep --pattern 'class Meeting {
$$$
isMultistream: $_
$$$
locusMediaRequest: $_
$$$
}'
Length of output: 102
Script:
#!/bin/bash
# Let's try a different approach to find these properties in the Meeting class
# First, let's find the Meeting class file
fd "Meeting" --type f
# Then let's search for these properties using ripgrep for better context
rg "isMultistream|locusMediaRequest" -A 2 -B 2
Length of output: 143070
Script:
#!/bin/bash
# Let's try to find the Meeting class definition and its properties
rg "class Meeting" -A 20
# Also search for where isMultistream and locusMediaRequest are set
rg "isMultistream = |locusMediaRequest = " -A 2
Length of output: 42470
packages/@webex/plugin-meetings/test/unit/spec/roap/index.ts (2)
164-164: LGTM: Correct verification of isMultistream parameter
The test correctly verifies that the sendRoap method is called with the isMultistream parameter, aligning with the architectural changes replacing meetingId.
Line range hint 1-300: Add test coverage for 'early-call-min-clusters' feature
The PR objectives mention adding support for 'early-call-min-clusters' in the Orpheus API, but there are no tests covering this new feature. Consider adding test cases to verify the behavior of this new option.
Let me check if there are any existing tests for this feature:
Would you like me to help draft test cases for the 'early-call-min-clusters' feature?
packages/@webex/plugin-meetings/src/meeting/locusMediaRequest.ts (1)
Line range hint 1-1: Add test coverage for clientMediaPreferences
Please add test cases to cover:
- Validation of clientMediaPreferences
- Error handling for invalid preferences
- Successful request handling with valid preferences
packages/@webex/plugin-meetings/src/roap/turnDiscovery.ts (1)
411-412: LGTM! Verify multistream parameter handling
The addition of multistream support parameters is consistent with the PR objectives. The implementation correctly passes the parameters from the meeting object.
Let's verify the multistream parameter handling across the codebase:
✅ Verification successful
Let me analyze the results from the executed scripts to verify the multistream parameter handling.
The search results show consistent handling of isMultistream parameter across multiple files:
- In ROAP request handling:
packages/@webex/plugin-meetings/src/roap/request.ts- Defines the interface withisMultistreamparameterpackages/@webex/plugin-meetings/src/roap/index.ts- Multiple usages showing consistent handlingpackages/@webex/plugin-meetings/src/roap/turnDiscovery.ts- The file under review
Let me verify the locusMediaRequest parameter as well:
Based on the search results, I can verify the handling of both isMultistream and locusMediaRequest parameters:
isMultistreamis consistently handled across ROAP-related files:
- In request handling (
request.ts) - In ROAP index (
index.ts) - In turn discovery (
turnDiscovery.ts)
locusMediaRequestis also consistently passed through:
- Defined in meeting types (
meeting/index.ts) - Used in ROAP request handling (
roap/request.ts) - Properly passed in turn discovery (
roap/turnDiscovery.ts) - Used in ROAP index (
roap/index.ts)
The changes in turnDiscovery.ts follow the established pattern of passing these parameters through the ROAP messaging system.
LGTM! The multistream and locus media request parameters are handled consistently
The changes correctly pass through the multistream and locus media request parameters from the meeting object, following the established patterns in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent handling of isMultistream parameter
# Expected: All ROAP-related files should handle isMultistream consistently
# Search for ROAP message handling with isMultistream
ast-grep --pattern 'sendRoap({
$$$
isMultistream: $_
$$$
})'
# Search for potential missing isMultistream handling
rg -l 'sendRoap\(' | xargs rg -L 'isMultistream'
Length of output: 131110
Script:
#!/bin/bash
# Search for locusMediaRequest handling
rg -l 'locusMediaRequest' | xargs rg 'locusMediaRequest.*:'
Length of output: 132937
packages/@webex/plugin-meetings/src/meeting/request.ts (2)
154-154: LGTM!
The parameter destructuring is properly placed with related media options.
176-176: 🛠️ Refactor suggestion
Add validation for clientMediaPreferences
The clientMediaPreferences object is directly assigned to the request body without validation. Consider adding validation to ensure the object contains required properties and correct data types before sending the request.
// Add validation before assigning to body
if (clientMediaPreferences) {
// Validate required properties
const validKeys = ['preferTranscoding']; // Add other valid keys
const invalidKeys = Object.keys(clientMediaPreferences).filter(
key => !validKeys.includes(key)
);
if (invalidKeys.length > 0) {
throw new Error(`Invalid properties in clientMediaPreferences: ${invalidKeys.join(', ')}`);
}
body.clientMediaPreferences = clientMediaPreferences;
}packages/@webex/plugin-meetings/test/unit/spec/meeting/request.js (1)
Line range hint 199-219: Verify implementation against Orpheus API requirements.
The test implementation seems to only superficially cover the 'early-call-min-clusters' feature mentioned in PR #4013. Please ensure that:
- The test cases accurately reflect the Orpheus API requirements
- The
clientMediaPreferencesstructure matches the API specification
✅ Verification successful
Based on the search results, I can see that the implementation of early-call-min-clusters is properly handled in the reachability module:
- The feature is set in the discovery options:
this.minRequiredClusters = discoveryOptions?.['early-call-min-clusters'] - It's included in the reachability report:
'early-call-min-clusters': this.minRequiredClusters - Tests verify this functionality
Additionally, I found references to the Orpheus API version in the reachability module which is used to determine the reachability report format.
Therefore, the implementation aligns with the Orpheus API requirements and the test changes appropriately cover this feature.
The test implementation properly validates the feature and aligns with the API requirements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Orpheus API related files and documentation
fd -t f -e md -e txt . | xargs rg -l "early.*call.*min.*clusters|Orpheus.*API"
# Search for related test files
fd -t f "test" | xargs rg -l "clientMediaPreferences|earlyCallMinClusters"
Length of output: 308
Script:
#!/bin/bash
# Let's search for the implementation of joinMeeting method and clientMediaPreferences
ast-grep --pattern 'joinMeeting($$$) { $$$ }'
# Search for any references to clientMediaPreferences in the codebase
rg "clientMediaPreferences" -B 2 -A 2
# Search for any documentation or comments about media preferences
rg -g "*.{js,md,txt}" "(?i)media.*preferences" -B 2 -A 2
Length of output: 131136
Script:
#!/bin/bash
# Search for implementation of clientMediaPreferences in reachability module
ast-grep --pattern 'getClientMediaPreferences($$$) { $$$ }'
# Search for any references to early call min clusters
rg -i "early.*call.*min.*clusters|earlyCallMinClusters"
# Search for any references to Orpheus API
rg -i "orpheus.*api|orpheusApiVersion"
Length of output: 132385
packages/@webex/plugin-meetings/test/unit/spec/roap/turnDiscovery.ts (2)
41-59: LGTM! Well-structured mock object.
The mock object is well-organized and includes all necessary properties for comprehensive testing, including audio/video states, reachability checks, and media request handling.
318-323: LGTM! Improved error message clarity in assertions.
The updated error messages in assertions are more descriptive and follow a consistent format, making test failures easier to debug and fix.
Also applies to: 387-392, 560-563, 588-591, 608-611, 631-634
packages/@webex/plugin-meetings/test/unit/spec/meeting/utils.js (1)
25-29: LGTM: Well-structured test setup for reachability stubs.
The stubs are properly initialized and follow good testing practices by providing default resolved values.
packages/@webex/plugin-meetings/src/roap/request.ts (2)
27-27: Function signature updated to include isMultistream parameter
The addition of isMultistream enhances the sendRoap method to support multistream scenarios effectively.
68-69: Ensure reachability and clientMediaPreferences are correctly handled
Including reachability and clientMediaPreferences in the locusMediaRequest.send call appears appropriate. Verify that the send method in LocusMediaRequest accepts and processes these parameters as intended.
Run the following script to confirm that send method accepts the new parameters:
✅ Verification successful
Based on the search results, I can now generate a final response. The code shows that reachability and clientMediaPreferences are valid parameters in the RoapRequest type, which is one of the types that can be passed to the send method of LocusMediaRequest. The implementation shows these parameters are properly handled.
Parameters are correctly defined and handled in the request type
The RoapRequest type definition explicitly includes both reachability and clientMediaPreferences as valid fields:
export type RoapRequest = {
type: 'RoapMessage';
selfUrl: string;
mediaId: string;
roapMessage: any;
reachability: any;
clientMediaPreferences: any;
sequence?: any;
};🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that LocusMediaRequest.send includes 'reachability' and 'clientMediaPreferences' parameters.
# Search for the send method definition that includes the new parameters.
ast-grep --pattern $'send({$$$ reachability, clientMediaPreferences $$$})' --lang javascript
Length of output: 93
Script:
#!/bin/bash
# Let's first find the LocusMediaRequest class definition and its send method
ast-grep --pattern 'class LocusMediaRequest {
$$$
send($$$) {
$$$
}
$$$
}'
# Also search for any usage of send method with these parameters
rg "send.*reachability.*clientMediaPreferences" -A 5
Length of output: 131744
Script:
#!/bin/bash
# Let's search for LocusMediaRequest class and its send method implementation
rg "class LocusMediaRequest" -A 10
# Also search for any usage of reachability and clientMediaPreferences in the context of media requests
rg "reachability.*clientMediaPreferences" -B 5 -A 5
Length of output: 136463
Script:
#!/bin/bash
# Let's search for the send method implementation in LocusMediaRequest
rg "send.*\{" -A 10 packages/@webex/plugin-meetings/src/meeting/locusMediaRequest.ts
# Also search for any usage of reachability and clientMediaPreferences in the context of send
rg "send.*\(.*reachability.*clientMediaPreferences" -A 5 packages/@webex/plugin-meetings/src/
Length of output: 1219
Script:
#!/bin/bash
# Let's search for the Request type definition to understand its parameters
rg "type Request" -A 10 packages/@webex/plugin-meetings/src/meeting/locusMediaRequest.ts
# Also search for where reachability and clientMediaPreferences are being used in the request
rg "reachability.*clientMediaPreferences.*request" -B 5 -A 5 packages/@webex/plugin-meetings/src/
Length of output: 764
packages/@webex/plugin-meetings/src/reachability/request.ts (1)
78-78: Confirm handling of undefined 'hybridMedia' in 'clusterClasses'
When setting isVideoMesh, the code uses optional chaining with res.body.clusterClasses?.hybridMedia?.includes(key). If hybridMedia is undefined, includes will return undefined, and !! will convert it to false. If this behavior is intended, no action is needed. Otherwise, consider providing a default empty array to hybridMedia.
Would you like to verify if hybridMedia can be undefined and ensure that clusters[key].isVideoMesh is set accurately?
packages/@webex/plugin-meetings/test/unit/spec/reachability/request.js (5)
38-38: Declaration of previousReport variable for test accessibility
Declaring previousReport at this scope ensures it is accessible within the beforeEach setup and all test cases, which is appropriate for shared test data.
57-60: Initialization of previousReport with mock data
Setting up previousReport with mock data in the beforeEach block ensures that the tests have consistent and predictable input for simulation.
62-65: Proper cleanup of Sinon stubs and spies after tests
Including sinon.restore() in the afterEach block ensures that all Sinon stubs and spies are restored after each test case, preventing side effects between tests.
66-90: Test correctly verifies behavior when trigger is "startup"
The test case accurately checks that when the trigger is "startup", the getClusters method sends a POST request with the correct parameters and that measureLatency is called with the expected arguments.
92-116: Test correctly verifies behavior when trigger is not "startup"
The test case appropriately verifies that when the trigger is not "startup", the getClusters method sends a POST request with the correct parameters and confirms that measureLatency is not called.
packages/@webex/plugin-meetings/src/meeting/util.ts (1)
118-118: Conversion of joinMeeting to an async function is appropriate
The joinMeeting method is correctly updated to be asynchronous to accommodate the use of await for asynchronous operations within the function.
packages/@webex/plugin-meetings/src/reachability/index.ts (6)
532-554: Ensure correct counting in getNumberOfReachableClusters method
The method getNumberOfReachableClusters counts clusters where any protocol is reachable. Verify that this logic aligns with the intended behavior, especially if certain protocols are prioritized over others.
729-730: Ensure proper handling when minimum required clusters are not met
The comment indicates a need to perform a new call when minimum required clusters are not reached. Confirm that this logic is implemented correctly and that the system gracefully handles cases where the requirement cannot be satisfied.
793-799: Ensure proper abort logic in abortCurrentChecks
The abortCurrentChecks method calls clearTimer and abortClusterReachability. Verify that all timers and ongoing tasks are correctly cleared or aborted to prevent memory leaks or unintended behavior.
873-887: Handle empty clusterList appropriately
When clusterList is empty or undefined, the code resolves the reachability promise immediately. Confirm that this early exit does not cause issues elsewhere in the application and that downstream processes can handle an empty result.
923-923: Clear overallTimer when all results are ready
Clearing the overallTimer upon receiving all reachability results is a good practice to avoid unnecessary timeouts. Ensure that this timer is always cleared to prevent potential side effects.
985-994: Clarify return value in getReachabilityReportToAttachToRoap
The method returns undefined for orpheusApiVersion version 1. Ensure that consumers of this method handle undefined appropriately to avoid unexpected errors.
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (1)
3892-3897:
Ensure localSdp is properly escaped and encoded
The localSdp value being sent in the /media request body contains JSON with the SDP offer. Make sure this value is properly escaped and encoded to avoid any injection vulnerabilities or parsing issues on the server side.
Consider:
- Using a well-tested JSON serialization library
- Escaping any special characters in the SDP
- Validating and sanitizing the SDP before including it
- Adding test cases with SDPs containing special characters
Run this script to check for proper JSON encoding and escaping of localSdp in recent /media requests:
If the script outputs any issues, work with the team to fix the JSON encoding and escaping of localSdp to prevent potential security vulnerabilities.
packages/@webex/plugin-meetings/src/meeting/locusMediaRequest.ts
Outdated
Show resolved
Hide resolved
packages/@webex/plugin-meetings/test/unit/spec/meeting/locusMediaRequest.ts
Show resolved
Hide resolved
marcin-bazyl
force-pushed
the
spark-577185
branch
from
a8072cd to
32ebed8
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (11)
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (6)
3689-3690: Improve code formatting and consistencyThe line numbers are misaligned and there's inconsistent indentation. Consider fixing the formatting.
- meeting.webex.meetings.reachability.getReachabilityReportToAttachToRoap = sinon.stub().resolves({id: 'fake reachability'}); - meeting.webex.meetings.reachability.getClientMediaPreferences = sinon.stub().resolves({id: 'fake clientMediaPreferences'}); + meeting.webex.meetings.reachability.getReachabilityReportToAttachToRoap = + sinon.stub().resolves({id: 'fake reachability'}); + meeting.webex.meetings.reachability.getClientMediaPreferences = + sinon.stub().resolves({id: 'fake clientMediaPreferences'});
3876-3878: Improve test readability by extracting repeated assertionsThese assertions are repeated multiple times in the test file. Consider extracting them into a helper function.
- assert.calledWith(meeting.webex.meetings.reachability.getClientMediaPreferences, meeting.isMultistream, 0); - assert.calledWith(meeting.webex.meetings.reachability.getReachabilityReportToAttachToRoap); - + const checkReachabilityAssertions = () => { + assert.calledWith(meeting.webex.meetings.reachability.getClientMediaPreferences, meeting.isMultistream, 0); + assert.calledWith(meeting.webex.meetings.reachability.getReachabilityReportToAttachToRoap); + }; + checkReachabilityAssertions();
3892-3897: Consider using template literals for better readabilityThe string concatenation in the localSdp could be more readable using template literals.
- localSdp: `{"audioMuted":${audioMuted},"videoMuted":${videoMuted},"roapMessage":{"messageType":"OFFER","sdps":["${sdp}"],"version":"2","seq":"${seq}","tieBreaker":"${tieBreaker}","headers":["includeAnswerInHttpResponse","noOkInTransaction"]},"reachability":{"id":"fake reachability"}}`, + localSdp: `{ + "audioMuted": ${audioMuted}, + "videoMuted": ${videoMuted}, + "roapMessage": { + "messageType": "OFFER", + "sdps": ["${sdp}"], + "version": "2", + "seq": "${seq}", + "tieBreaker": "${tieBreaker}", + "headers": ["includeAnswerInHttpResponse","noOkInTransaction"] + }, + "reachability": {"id": "fake reachability"} + }`,
3918-3922: Consider extracting repeated test data into constantsThe clientMediaPreferences object is repeated in multiple places. Consider extracting it into a constant.
+const FAKE_CLIENT_MEDIA_PREFERENCES = {id: 'fake clientMediaPreferences'}; + - id: 'fake clientMediaPreferences', + ...FAKE_CLIENT_MEDIA_PREFERENCES,
Line range hint
2500-2600: Consider using test data builders for complex test scenariosThe test data setup is quite complex with many nested objects. Consider using the Builder pattern to make test data creation more maintainable.
class MeetingTestDataBuilder { constructor() { this.data = { mediaConnections: [], locusUrl: '', locusId: '', // ... other defaults }; } withMediaConnections(connections) { this.data.mediaConnections = connections; return this; } withLocusUrl(url) { this.data.locusUrl = url; return this; } build() { return {...this.data}; } }
Based on the analysis of the test files, I can now provide specific recommendations about the test organization:
Test Suite Organization Review
- The index.js file contains over 70 describe blocks testing individual methods, making it too large and difficult to maintain. The file should be split into logical groupings:
- Media-related tests (addMedia, joinWithMedia, updateMedia, etc.)
- Meeting lifecycle tests (join, leave, decline, etc.)
- Transcription/caption tests
- Configuration/state tests
The connection state and media request tests show good focused testing of specific functionality with clear test cases.
Missing integration tests between components - need more tests covering interactions between modules.
Test utilities could be enhanced to provide more reusable test setup code.
Specific recommendations:
- Split index.js into 4-5 focused test files based on functionality domains
- Add more integration tests between components
- Create shared test fixtures and setup helpers
- Add more descriptive test names following given/when/then pattern
The current test organization makes maintenance difficult and misses important integration test scenarios. The large index.js file should be refactored into smaller, focused test files to improve maintainability and test coverage.
🔗 Analysis chain
Line range hint
1-5000: Overall test suite structure and organizationThe test suite is well-organized but could benefit from some improvements:
- Consider grouping related tests into separate files to improve maintainability
- Add more descriptive test names that follow the given/when/then pattern
- Consider using shared setup code for common test scenarios
- Add more integration tests for complex interactions between components
The test files are already reasonably well organized into separate modules based on functionality:
- Core meeting functionality (index.js)
- Connection state handling (connectionStateHandler.ts)
- In-meeting actions (in-meeting-actions.ts)
- Media request handling (locusMediaRequest.ts)
- Mute state management (muteState.js)
- Meeting requests (request.js)
- Utility functions (utils.js)
- Voice assistant integration (voicea-meeting.ts)
Let me verify the test coverage and organization within these files to provide more specific recommendations.
Let me check the actual test implementations to provide more specific recommendations about test organization and coverage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check current test organization fd -e js -e ts . packages/@webex/plugin-meetings/test/unit/spec/meeting/Length of output: 643
Script:
#!/bin/bash # Check test coverage and organization in index.js rg -A 2 "^describe\(" packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js # Check test organization in other files for f in packages/@webex/plugin-meetings/test/unit/spec/meeting/*.{js,ts}; do if [ "$f" != "packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js" ]; then echo "=== $f ===" rg -A 2 "^describe\(" "$f" echo fi doneLength of output: 4622
Script:
#!/bin/bash # Check test implementations for a few key files # Check index.js describe blocks and test organization rg "^\s+describe\('#" packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js # Check test coverage in connection state handler rg "^\s+it\(" packages/@webex/plugin-meetings/test/unit/spec/meeting/connectionStateHandler.ts # Check test coverage in media request handling rg "^\s+it\(" packages/@webex/plugin-meetings/test/unit/spec/meeting/locusMediaRequest.ts # Check shared test utilities/helpers rg "function" packages/@webex/plugin-meetings/test/unit/spec/meeting/utils.jsLength of output: 7408
packages/@webex/plugin-meetings/test/unit/spec/reachability/index.ts (1)
1680-1760: Refactor test setup to reduce duplicationThe test cases within this range have similar setup code for mocking
getClustersresults and received events. Consider refactoring the common setup logic into helper functions or utilizing parameterized tests to improve maintainability and reduce code duplication.packages/@webex/plugin-meetings/src/reachability/index.ts (2)
Line range hint
132-167: Consider enhancing error handling in the retry mechanism.While the retry logic is good, consider adding:
- A maximum retry count to prevent infinite retries
- Exponential backoff between retries
- Different handling for specific error types
async getClusters( trigger: string, previousReport?: any, - isRetry = false + {isRetry = false, retryCount = 0, maxRetries = 3}: { + isRetry?: boolean; + retryCount?: number; + maxRetries?: number; + } = {} ): Promise<{ clusters: ClusterList; joinCookie: any; }> { try { const {clusters, joinCookie, discoveryOptions} = await this.reachabilityRequest.getClusters( trigger, MeetingUtil.getIpVersion(this.webex), previousReport ); this.minRequiredClusters = discoveryOptions?.['early-call-min-clusters']; this.orpheusApiVersion = discoveryOptions?.['report-version']; await this.webex.boundedStorage.put( this.namespace, REACHABILITY.localStorageJoinCookie, JSON.stringify(joinCookie) ); return {clusters, joinCookie}; } catch (error) { - if (isRetry) { + if (retryCount >= maxRetries) { throw error; } LoggerProxy.logger.error( - `Reachability:index#getClusters --> Failed with error: ${error}, retrying...` + `Reachability:index#getClusters --> Failed with error: ${error}, retry ${retryCount + 1}/${maxRetries}...` ); + await new Promise(resolve => setTimeout(resolve, Math.pow(2, retryCount) * 1000)); + - return this.getClusters(trigger, previousReport, true); + return this.getClusters(trigger, previousReport, { + isRetry: true, + retryCount: retryCount + 1, + maxRetries, + }); } }
959-979: Enhance type safety for API versioning.The API version handling could benefit from stronger typing to prevent potential runtime errors.
+type ClientMediaPreferencesV0 = { + ipver?: IP_VERSION; + joinCookie?: any; + preferTranscoding: boolean; +}; + +type ClientMediaPreferencesV1 = ClientMediaPreferencesV0 & { + reachability: { + version: 1; + result: { + usedDiscoveryOptions: { + 'early-call-min-clusters'?: number; + }; + metrics: { + 'total-duration-ms'?: number; + }; + tests: any; + }; + }; +}; -async getClientMediaPreferences(isMultistream: boolean, ipver?: IP_VERSION) { +async getClientMediaPreferences( + isMultistream: boolean, + ipver?: IP_VERSION +): Promise<ClientMediaPreferencesV0 | ClientMediaPreferencesV1> { if (!this.orpheusApiVersion) { return { ipver, joinCookie: await this.getJoinCookie(), preferTranscoding: !isMultistream, }; } const reachabilityReport = await this.getReachabilityReport(); return { ipver, preferTranscoding: !isMultistream, ...reachabilityReport, }; }packages/@webex/plugin-meetings/src/meeting/util.ts (1)
147-159: Consider adding reachability metrics to join events.The metrics could be enhanced to include reachability-related information for better monitoring.
webex.internal.newMetrics.submitClientEvent({ name: 'client.locus.join.request', - options: {meetingId: meeting.id}, + options: { + meetingId: meeting.id, + reachabilityApiVersion: webex.meetings.reachability.orpheusApiVersion, + minRequiredClusters: webex.meetings.reachability.minRequiredClusters, + }, });packages/@webex/plugin-meetings/test/unit/spec/roap/request.ts (1)
27-30: Consider adding negative test cases for reachability stubsWhile the setup is correct, consider enhancing test coverage by:
- Testing error scenarios when reachability methods fail
- Testing with different IP version configurations
Example addition:
it('handles reachability data fetch failures gracefully', async () => { webex.meetings.reachability.getReachabilityReportToAttachToRoap.rejects(new Error('Network error')); // Test error handling });Also applies to: 45-45
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (18)
packages/@webex/plugin-meetings/src/meeting/index.ts(0 hunks)packages/@webex/plugin-meetings/src/meeting/locusMediaRequest.ts(2 hunks)packages/@webex/plugin-meetings/src/meeting/request.ts(3 hunks)packages/@webex/plugin-meetings/src/meeting/util.ts(3 hunks)packages/@webex/plugin-meetings/src/reachability/index.ts(18 hunks)packages/@webex/plugin-meetings/src/reachability/request.ts(1 hunks)packages/@webex/plugin-meetings/src/roap/index.ts(4 hunks)packages/@webex/plugin-meetings/src/roap/request.ts(3 hunks)packages/@webex/plugin-meetings/src/roap/turnDiscovery.ts(2 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js(4 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/locusMediaRequest.ts(2 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/request.js(3 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/utils.js(2 hunks)packages/@webex/plugin-meetings/test/unit/spec/reachability/index.ts(1 hunks)packages/@webex/plugin-meetings/test/unit/spec/reachability/request.js(2 hunks)packages/@webex/plugin-meetings/test/unit/spec/roap/index.ts(1 hunks)packages/@webex/plugin-meetings/test/unit/spec/roap/request.ts(4 hunks)packages/@webex/plugin-meetings/test/unit/spec/roap/turnDiscovery.ts(15 hunks)
💤 Files with no reviewable changes (1)
- packages/@webex/plugin-meetings/src/meeting/index.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/@webex/plugin-meetings/test/unit/spec/roap/turnDiscovery.ts
- packages/@webex/plugin-meetings/test/unit/spec/reachability/request.js
- packages/@webex/plugin-meetings/test/unit/spec/roap/index.ts
- packages/@webex/plugin-meetings/test/unit/spec/meeting/request.js
- packages/@webex/plugin-meetings/src/roap/index.ts
🔇 Additional comments (17)
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (2)
Line range hint 4000-4100: Add test coverage for network status changes
The tests for network status changes could be more comprehensive.
Consider adding tests for:
- Network transitions between different states
- Error handling during network status changes
- Edge cases in network status handling
Line range hint 1-100: Add test coverage for error cases in Meeting class initialization
The test suite has good coverage for successful scenarios but could use more tests for error handling during Meeting class initialization.
Consider adding tests for:
- Invalid configuration parameters
- Missing required dependencies
- Error handling during initialization
packages/@webex/plugin-meetings/test/unit/spec/reachability/index.ts (1)
1849-1849: Correct the assertion for getClusters call count
The assertion assert.calledOnce(reachability.reachabilityRequest.getClusters); contradicts the expectation that getClusters should be called again when the minimum required clusters are not reached. To accurately reflect the expected behavior, the assertion should verify that getClusters is called twice.
Apply this diff to correct the assertion:
- assert.calledOnce(reachability.reachabilityRequest.getClusters);
+ assert.calledTwice(reachability.reachabilityRequest.getClusters);packages/@webex/plugin-meetings/src/reachability/request.ts (1)
38-93: Ensure proper error handling in the getClusters method
The getClusters method handles successful responses but does not catch and handle errors that might occur during the request. Without error handling, any exceptions thrown will be unhandled, which can lead to unexpected behaviors.
Apply this diff to add error handling:
return callWrapper(
() =>
this.webex.request({
// request configuration
}),
'internal.get.cluster.time'
)
.then((res) => {
// success handling
})
+ .catch((error) => {
+ LoggerProxy.logger.error(
+ `Reachability:request#getClusters --> failed to get clusters: ${error}`
+ );
+ throw error;
+ });This addition will log the error and re-throw it for upstream handling.
packages/@webex/plugin-meetings/src/meeting/locusMediaRequest.ts (2)
17-17: Define a specific type for clientMediaPreferences instead of any
Using any for clientMediaPreferences reduces the benefits of TypeScript's type safety. Defining a specific interface or type will enhance code reliability and maintainability.
Consider defining an interface for clientMediaPreferences, for example:
interface ClientMediaPreferences {
earlyCallMinClusters?: number;
// Add other properties as needed
}Then update the type in your code:
- clientMediaPreferences: any;
+ clientMediaPreferences: ClientMediaPreferences;221-221: Validate clientMediaPreferences before including it in the request
The clientMediaPreferences are added to the request body without validation. It's important to ensure that the preferences are correctly structured and contain valid values to prevent potential errors in the request.
Consider adding validation for clientMediaPreferences:
if (request.clientMediaPreferences) {
// Validate the structure and values of clientMediaPreferences
// For example:
if (
'earlyCallMinClusters' in request.clientMediaPreferences &&
(typeof request.clientMediaPreferences.earlyCallMinClusters !== 'number' ||
request.clientMediaPreferences.earlyCallMinClusters < 0)
) {
throw new Error('Invalid value for earlyCallMinClusters in clientMediaPreferences');
}
body.clientMediaPreferences = request.clientMediaPreferences;
}This ensures that only valid preferences are sent in the request.
packages/@webex/plugin-meetings/test/unit/spec/meeting/locusMediaRequest.ts (2)
37-49: LGTM! Well-structured test data for clientMediaPreferences.
The test data properly represents the expected structure with all necessary fields: preferTranscoding, joinCookie, ipver, and reachability.
63-73: 🛠️ Refactor suggestion
Maintain consistency in IP version specification.
The ipver value should use the IP_VERSION.only_ipv4 constant instead of the hardcoded number 4 to maintain consistency with the example request body.
clientMediaPreferences: {
preferTranscoding: false,
- ipver: 4,
+ ipver: IP_VERSION.only_ipv4,
joinCookie: {
anycastEntryPoint: 'aws-eu-west-1',
clientIpAddress: 'some ip',
timeShot: '2023-05-23T08:03:49Z'
},
reachability: {
version: '1',
result: 'some fake reachability result',
}
}Likely invalid or redundant comment.
packages/@webex/plugin-meetings/src/roap/turnDiscovery.ts (2)
411-411: LGTM! Consistent parameter update.
The addition of isMultistream parameter aligns with the broader changes in the codebase for handling multistream capabilities.
452-452: LGTM! Consistent parameter update.
The addition of isMultistream parameter in sendRoapOK maintains consistency with the changes in sendRoapTurnDiscoveryRequest.
packages/@webex/plugin-meetings/src/meeting/request.ts (2)
176-176: LGTM! Proper integration of clientMediaPreferences.
The parameter is correctly integrated into the request body construction.
132-132: 🛠️ Refactor suggestion
Define a proper interface for clientMediaPreferences.
Using any type reduces type safety. Consider creating a dedicated interface to define the structure.
interface ClientMediaPreferences {
preferTranscoding?: boolean;
joinCookie?: {
anycastEntryPoint: string;
clientIpAddress: string;
timeShot: string;
};
ipver?: number;
reachability?: {
version: string;
result: string;
};
}packages/@webex/plugin-meetings/src/reachability/index.ts (2)
87-89: LGTM! Well-structured property declarations.
The optional properties minRequiredClusters and orpheusApiVersion are appropriately typed and align with the PR's objective of supporting the 'early-call-min-clusters' option.
266-280:
Prevent potential infinite recursion in fallback mechanism.
The fallback mechanism could potentially cause infinite recursion if the minimum required clusters are never reached, as each fallback attempt triggers another check that might fail again.
Add a maximum fallback attempt limit:
+private fallbackAttempts = 0;
+private readonly MAX_FALLBACK_ATTEMPTS = 3;
public async gatherReachabilityFallback(): Promise<void> {
try {
+ if (this.fallbackAttempts >= this.MAX_FALLBACK_ATTEMPTS) {
+ LoggerProxy.logger.warn(
+ 'Reachability:index#gatherReachabilityFallback --> Max fallback attempts reached'
+ );
+ return;
+ }
+ this.fallbackAttempts++;
const reachabilityReport = await this.getReachabilityReport();
const {clusters} = await this.getClusters('early-call/no-min-reached', reachabilityReport);
this.abortCurrentChecks();
await this.performReachabilityChecks(clusters);
} catch (error) {
LoggerProxy.logger.error(
`Reachability:index#gatherReachabilityFallback --> Error:`,
error
);
}
}Likely invalid or redundant comment.
packages/@webex/plugin-meetings/src/meeting/util.ts (1)
130-135: 🛠️ Refactor suggestion
Enhance error handling for reachability operations.
The consecutive await calls could fail independently, and the error handling could be more specific.
- const reachability = await webex.meetings.reachability.getReachabilityReportToAttachToRoap();
- const clientMediaPreferences = await webex.meetings.reachability.getClientMediaPreferences(
- meeting.isMultistream,
- MeetingUtil.getIpVersion(webex)
- );
+ let reachability;
+ let clientMediaPreferences;
+
+ try {
+ reachability = await webex.meetings.reachability.getReachabilityReportToAttachToRoap();
+ } catch (error) {
+ LoggerProxy.logger.error(
+ 'Meeting:util#joinMeeting --> Failed to get reachability report:',
+ error
+ );
+ // Continue with undefined reachability
+ }
+
+ try {
+ clientMediaPreferences = await webex.meetings.reachability.getClientMediaPreferences(
+ meeting.isMultistream,
+ MeetingUtil.getIpVersion(webex)
+ );
+ } catch (error) {
+ LoggerProxy.logger.error(
+ 'Meeting:util#joinMeeting --> Failed to get client media preferences:',
+ error
+ );
+ // Provide fallback preferences
+ clientMediaPreferences = {
+ ipver: MeetingUtil.getIpVersion(webex),
+ preferTranscoding: !meeting.isMultistream,
+ };
+ }Likely invalid or redundant comment.
packages/@webex/plugin-meetings/test/unit/spec/roap/request.ts (2)
7-7: LGTM: Import addition is appropriate
The MeetingUtil import is necessary for the IP version functionality used in the tests.
Line range hint 1-126: Verify alignment with early-call-min-clusters feature
While the test covers reachability and media preferences, it's not explicitly testing the 'early-call-min-clusters' functionality mentioned in the PR objectives.
✅ Verification successful
Based on the extensive test files examined, I can now generate the final response:
Test coverage aligns with early-call-min-clusters feature
The test file under review is correctly testing reachability and media preferences, which are complementary to the early-call-min-clusters feature. The early-call-min-clusters functionality is thoroughly tested in other test files:
reachability/request.jstests the feature flag in cluster requestsreachability/index.tsextensively tests the fallback mechanism and cluster discovery with early-call-min-clusters option
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other test files that might cover early-call-min-clusters functionality
rg -t ts -t js "early.*call.*min.*clusters" "packages/@webex/plugin-meetings/test"
# Search for the implementation of this feature
ast-grep --pattern 'earlyCallMinClusters'
Length of output: 909
Script:
#!/bin/bash
# Let's examine the test files that contain early-call-min-clusters references
cat "packages/@webex/plugin-meetings/test/unit/spec/reachability/request.js"
cat "packages/@webex/plugin-meetings/test/unit/spec/reachability/index.ts"
# Also check the implementation files for reachability
rg -g "!test" -t ts -t js "early.*call.*min.*clusters" "packages/@webex/plugin-meetings"
Length of output: 69504
marcin-bazyl
force-pushed
the
spark-577185
branch
from
32ebed8 to
9da0b21
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (13)
packages/@webex/plugin-meetings/test/unit/spec/roap/index.ts (1)
Line range hint
229-283: Consider adding test cases for additional scenarios.While the error handling test coverage is good, consider adding these test cases:
- Test when
isMultistreamis false to ensure both paths are covered- Test different IP versions since
MeetingUtil.getIpVersionis stubbedExample test case structure:
it('handles non-multistream meetings correctly', async () => { meeting.isMultistream = false; // ... test implementation }); [IP_VERSION.IPv4, IP_VERSION.IPv6].forEach((ipVersion) => { it(`handles ${ipVersion} correctly`, async () => { MeetingUtil.getIpVersion.returns(ipVersion); // ... test implementation }); });packages/@webex/plugin-meetings/src/reachability/index.ts (5)
158-158: Specify a more precise type thananyforpreviousReportThe parameter
previousReportis currently typed asany. To improve type safety and code clarity, consider specifying a more precise type or interface forpreviousReport.
89-89: Specify a more precise type thananyforjoinCookieIn the
ClientMediaPreferencesinterface,joinCookieis typed asany. Consider defining a specific type or interface forjoinCookieto enhance type safety and maintainability.
442-442: Correct the log message to reflect the accurate module and function nameThe log message refers to
Roap:request#getReachabilityMetrics, but this appears in theReachabilitymodule. Update the log message to reflect the correct module and function, e.g.,Reachability:index#getReachabilityMetrics.
122-123: Declare and initializestartTimeandtotalDurationwith appropriate typesThe properties
startTimeandtotalDurationare assignedundefined, but their types are not explicitly declared. Consider declaring them with appropriate types, such asnumber | undefined, to enhance type safety.
171-173: Define a type fordiscoveryOptionsto avoid using magic stringsCurrently, the code accesses
discoveryOptionsusing string literals like'early-call-min-clusters'and'report-version'. Consider defining a TypeScript interface or type fordiscoveryOptionsto provide better type safety and prevent errors due to typos.packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (2)
3892-3896: Consider extracting the localSdp JSON string into a separate variable for better readability.The
localSdpproperty value is a long JSON string that includes theaudioMuted,videoMuted,roapMessageandreachabilityproperties. To improve readability, consider extracting this JSON string into a separate variable before passing it to thelocalMediasarray.Apply this refactor:
+const localSdp = `{"audioMuted":${audioMuted},"videoMuted":${videoMuted},"roapMessage":{"messageType":"OFFER","sdps":["${sdp}"],"version":"2","seq":"${seq},"tieBreaker":"${tieBreaker}","headers":["includeAnswerInHttpResponse","noOkInTransaction"]},"reachability":{"id":"fake reachability"}}`; + localMedias: [ { - localSdp: `{"audioMuted":${audioMuted},"videoMuted":${videoMuted},"roapMessage":{"messageType":"OFFER","sdps":["${sdp}"],"version":"2","seq":"${seq},"tieBreaker":"${tieBreaker}","headers":["includeAnswerInHttpResponse","noOkInTransaction"]},"reachability":{"id":"fake reachability"}}`, + localSdp, mediaId: 'fake media id', }, ],
3918-3922: Consider extracting the localSdp JSON string into a separate variable for better readability.Similar to the previous comment, the
localSdpproperty value is a long JSON string that includes theaudioMuted,videoMuted,roapMessageandreachabilityproperties. To improve readability, consider extracting this JSON string into a separate variable before passing it to thelocalMediasarray.Apply this refactor:
+const localSdp = `{"audioMuted":${audioMuted},"videoMuted":${videoMuted},"roapMessage":{"messageType":"OK","version":"2","seq":"${seq}"},"reachability":{"id":"fake reachability"}}`; + localMedias: [ { - localSdp: `{"audioMuted":${audioMuted},"videoMuted":${videoMuted},"roapMessage":{"messageType":"OK","version":"2","seq":"${seq}"},"reachability":{"id":"fake reachability"}}`, + localSdp, mediaId: 'fake media id', }, ],packages/@webex/plugin-meetings/src/reachability/request.ts (2)
33-46: Improve type definitions for method parameters and return typeThe type definitions could be more specific to improve type safety and documentation.
interface DiscoveryOptions { 'report-version': number; 'early-call-min-clusters': boolean; [key: string]: any; } interface GetClustersResponse { clusters: ClusterList; joinCookie: unknown; discoveryOptions?: DiscoveryOptions; } interface PreviousReport { // Add specific properties based on the actual structure [key: string]: any; } getClusters( trigger: 'startup' | 'manual' | string, ipVersion?: IP_VERSION, previousReport?: PreviousReport ): Promise<GetClustersResponse>
64-67: Document the early-call-min-clusters optionThe new option is added without documentation explaining its purpose and impact.
Add a comment explaining the purpose and behavior of the early-call-min-clusters option:
'supported-options': { 'report-version': 1, + // Enables early call optimization by requiring minimum number of clusters + // This helps reduce call setup time by proceeding with fewer clusters 'early-call-min-clusters': true, },packages/@webex/plugin-meetings/test/unit/spec/roap/request.ts (1)
91-97: Use meaningful test dataThe fake data objects could be more representative of actual production data.
const FAKE_REACHABILITY_REPORT = { clusterId: { udp: { result: 'reachable', latencyInMilliseconds: 10 }, tcp: { result: 'unreachable' }, isVideoMesh: false, } }; const FAKE_CLIENT_MEDIA_PREFERENCES = { ipver: IP_VERSION.ipv4_and_ipv6, earlyCallMinClusters: true, supportedCodecs: ['H264', 'VP8'] };packages/@webex/plugin-meetings/test/unit/spec/reachability/index.ts (2)
1691-1828: Consider using constants for timing valuesThe test case thoroughly validates the retry mechanism, but uses magic numbers for timing (3000ms, 20000ms). Consider extracting these to named constants for better maintainability and clarity.
+ const SHORT_TIMEOUT_MS = 3000; + const LONG_TIMEOUT_MS = 20000; - clock.tick(3000); + clock.tick(SHORT_TIMEOUT_MS); - clock.tick(20000); + clock.tick(LONG_TIMEOUT_MS);
1830-1919: Consider extracting common assertion patternThe edge case tests have duplicate assertion patterns for verifying done events and getClusters calls. Consider extracting these into a helper function for better maintainability.
const verifyCommonAssertions = (reachability, receivedEvents, expectedCallCount) => { assert.equal(receivedEvents.done, 1); assert.equal(await reachability.isWebexMediaBackendUnreachable(), false); assert.callCount(reachability.reachabilityRequest.getClusters, expectedCallCount); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (18)
packages/@webex/plugin-meetings/src/meeting/index.ts(0 hunks)packages/@webex/plugin-meetings/src/meeting/locusMediaRequest.ts(2 hunks)packages/@webex/plugin-meetings/src/meeting/request.ts(3 hunks)packages/@webex/plugin-meetings/src/meeting/util.ts(3 hunks)packages/@webex/plugin-meetings/src/reachability/index.ts(19 hunks)packages/@webex/plugin-meetings/src/reachability/request.ts(1 hunks)packages/@webex/plugin-meetings/src/roap/index.ts(4 hunks)packages/@webex/plugin-meetings/src/roap/request.ts(3 hunks)packages/@webex/plugin-meetings/src/roap/turnDiscovery.ts(2 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js(4 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/locusMediaRequest.ts(2 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/request.js(3 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/utils.js(2 hunks)packages/@webex/plugin-meetings/test/unit/spec/reachability/index.ts(1 hunks)packages/@webex/plugin-meetings/test/unit/spec/reachability/request.js(2 hunks)packages/@webex/plugin-meetings/test/unit/spec/roap/index.ts(1 hunks)packages/@webex/plugin-meetings/test/unit/spec/roap/request.ts(4 hunks)packages/@webex/plugin-meetings/test/unit/spec/roap/turnDiscovery.ts(15 hunks)
💤 Files with no reviewable changes (1)
- packages/@webex/plugin-meetings/src/meeting/index.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/@webex/plugin-meetings/test/unit/spec/reachability/request.js
- packages/@webex/plugin-meetings/test/unit/spec/roap/turnDiscovery.ts
- packages/@webex/plugin-meetings/test/unit/spec/meeting/utils.js
- packages/@webex/plugin-meetings/src/roap/index.ts
🔇 Additional comments (20)
packages/@webex/plugin-meetings/test/unit/spec/roap/index.ts (2)
164-166: LGTM! Verification of isMultistream parameter is properly implemented.
The test correctly verifies that the isMultistream parameter is passed to the sendRoap call, replacing the previous meetingId parameter.
Line range hint 1-283: Verify coverage of early-call-min-clusters feature.
While the test changes look good, I don't see any test coverage for the new 'early-call-min-clusters' option mentioned in the PR objectives. Consider adding test cases to verify this new functionality.
packages/@webex/plugin-meetings/src/reachability/index.ts (3)
174-178: 🛠️ Refactor suggestion
Avoid using @ts-ignore to suppress TypeScript errors
Using @ts-ignore suppresses TypeScript errors, which may hide underlying issues. It's better to resolve the type error by properly typing boundedStorage.put. Ensure that the method signature is correctly defined in the TypeScript definitions.
233-236: 🛠️ Refactor suggestion
Avoid using @ts-ignore to suppress TypeScript errors
Using @ts-ignore suppresses TypeScript errors, which may hide underlying issues. It's better to address the type error by correctly typing boundedStorage.get. Ensure that the method signature is accurately defined in the TypeScript definitions.
630-647:
Ensure termination condition to prevent infinite recursion
The resolveReachabilityPromise method may invoke gatherReachabilityFallback if the minimum required clusters are not reached, which could lead to repeated calls without a termination condition. Please ensure there is a proper termination condition to prevent potential infinite loops.
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (3)
3897-3898: Verify that the id property is the expected value returned by getClientMediaPreferences().
The code sets the id property of clientMediaPreferences to the string 'fake clientMediaPreferences'. Please confirm that this is the expected value that getClientMediaPreferences() should return in this test scenario.
Run the following script to check the return value of getClientMediaPreferences():
✅ Verification successful
The test correctly stubs getClientMediaPreferences() to return the expected value
The verification shows that in the test file, getClientMediaPreferences() is explicitly stubbed to return an object with id: 'fake clientMediaPreferences', which matches exactly with the value being tested. This is consistent within the test scenario and represents a valid mock value for testing purposes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that getClientMediaPreferences() returns an object with id: 'fake clientMediaPreferences'.
# Test: Search for the stub of getClientMediaPreferences(). Expect: Resolves with {id: 'fake clientMediaPreferences'}.
rg -A 10 $'meetings.reachability.getClientMediaPreferences = sinon.stub\\(\\).resolves\\('
Length of output: 1360
3689-3690: Verify that getClientMediaPreferences() is called with the right parameters.
The code calls meeting.webex.meetings.reachability.getClientMediaPreferences() with meeting.isMultistream and 0 as arguments. Please verify that these are the expected parameters that should be passed.
Run the following script to check the usage of getClientMediaPreferences() across the codebase:
3876-3878: Verify that getReachabilityReportToAttachToRoap() is called without any arguments.
The code calls meeting.webex.meetings.reachability.getReachabilityReportToAttachToRoap() without passing any arguments. Please confirm that this method does not expect any parameters.
Run the following script to check the usage of getReachabilityReportToAttachToRoap() in the codebase:
✅ Verification successful
Let me check the implementation of getReachabilityReportToAttachToRoap to verify its signature and requirements.
Let me try a different pattern to find the implementation since it's a TypeScript file.
From the earlier ripgrep results, I can see the implementation in packages/@webex/plugin-meetings/src/reachability/index.ts and all its usages. The method is defined with no parameters and is consistently called without arguments throughout the codebase. Let me generate the final response:
✓ Verified: getReachabilityReportToAttachToRoap() is correctly called without arguments
The method is defined in src/reachability/index.ts as async getReachabilityReportToAttachToRoap(): Promise<ReachabilityReportV0 | undefined> and is consistently called without arguments across the codebase:
- In
src/roap/request.ts - In
src/meeting/util.ts - In the test file under review
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that getReachabilityReportToAttachToRoap() is always called without arguments.
# Test: Search for calls to getReachabilityReportToAttachToRoap(). Expect: No arguments passed.
rg -A 5 $'getReachabilityReportToAttachToRoap\\('
Length of output: 1725
Script:
#!/bin/bash
# Search for the implementation of getReachabilityReportToAttachToRoap
ast-grep --pattern 'getReachabilityReportToAttachToRoap() {
$$$
}'
Length of output: 70
Script:
#!/bin/bash
# Search for the implementation of getReachabilityReportToAttachToRoap with TypeScript syntax
ast-grep --pattern 'async getReachabilityReportToAttachToRoap(): $_$ {
$$$
}'
Length of output: 81
packages/@webex/plugin-meetings/src/roap/request.ts (1)
44-54: Remove TypeScript error suppressions
Multiple instances of // @ts-ignore are used to bypass type checking. This can hide potential type safety issues.
Consider properly typing the webex object and its methods:
interface WebexMeetings {
reachability: {
getReachabilityReportToAttachToRoap(): Promise<Record<string, any>>;
getClientMediaPreferences(isMultistream: boolean, ipVersion: IP_VERSION): Promise<Record<string, any>>;
};
}
interface Webex {
meetings: WebexMeetings;
}packages/@webex/plugin-meetings/src/meeting/locusMediaRequest.ts (1)
221-221: Add validation for clientMediaPreferences
The clientMediaPreferences are added to the request body without validation. Consider:
- Validating the preferences before adding to request
- Adding error handling for invalid preferences
- Including preference data in metrics for better monitoring
packages/@webex/plugin-meetings/src/roap/turnDiscovery.ts (1)
411-411: LGTM! Consistent implementation of isMultistream property
The addition of isMultistream property is consistent across both methods and aligns well with the PR objectives. The implementation maintains proper error handling and metrics collection.
Also applies to: 452-452
packages/@webex/plugin-meetings/src/meeting/request.ts (2)
132-132: Define a proper interface for clientMediaPreferences
Using any type reduces type safety. Consider creating a dedicated interface to define the structure of clientMediaPreferences.
154-154: LGTM! The clientMediaPreferences parameter is correctly added to the request body.
The parameter is properly integrated into the request body construction.
Also applies to: 176-176
packages/@webex/plugin-meetings/test/unit/spec/meeting/request.js (2)
Line range hint 199-208: LGTM! Test setup for clientMediaPreferences is properly implemented.
The test case correctly includes the new parameter in the joinMeeting call.
219-219: Use deep equality assertion for object comparison.
When testing object properties, use assert.deepEqual instead of assert.equal to properly verify the structure and values of clientMediaPreferences.
packages/@webex/plugin-meetings/src/meeting/util.ts (2)
130-135: Add error handling for asynchronous calls
The calls to getReachabilityReportToAttachToRoap() and getClientMediaPreferences() are asynchronous and may fail. Consider wrapping these calls in a try...catch block to handle potential exceptions and improve the robustness of the code.
147-147: LGTM! The new parameters are correctly passed to the joinMeeting call.
The reachability and clientMediaPreferences parameters are properly integrated into the request.
Also applies to: 159-159
packages/@webex/plugin-meetings/test/unit/spec/reachability/index.ts (1)
1667-1690: Well-structured test suite setup!
The test suite setup is comprehensive with proper mock data configuration and event tracking for the fallback mechanism.
packages/@webex/plugin-meetings/test/unit/spec/meeting/locusMediaRequest.ts (2)
37-49: LGTM! Well-structured client media preferences.
The new clientMediaPreferences object is well-organized and properly uses the IP_VERSION constant instead of hardcoding the value.
63-73: Maintain consistency in IP version specification and fix indentation.
This issue was previously identified. Please:
- Use
IP_VERSION.only_ipv4constant instead of hardcoded number 4 - Fix the indentation of the reachability object to match the parent structure
marcin-bazyl
force-pushed
the
spark-577185
branch
from
9da0b21 to
14861b0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (13)
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (4)
3689-3690: Inconsistent indentation in newly added linesThe indentation of these new lines is incorrect compared to the surrounding code. They should be indented at the same level as the surrounding code.
- meeting.webex.meetings.reachability.getReachabilityReportToAttachToRoap = sinon.stub().resolves({id: 'fake reachability'}); - meeting.webex.meetings.reachability.getClientMediaPreferences = sinon.stub().resolves({id: 'fake clientMediaPreferences'}); + meeting.webex.meetings.reachability.getReachabilityReportToAttachToRoap = sinon.stub().resolves({id: 'fake reachability'}); + meeting.webex.meetings.reachability.getClientMediaPreferences = sinon.stub().resolves({id: 'fake clientMediaPreferences'});
3876-3878: Inconsistent indentation in newly added linesThe indentation of these new lines is incorrect compared to the surrounding code. They should be indented at the same level as the surrounding code.
- assert.calledWith(meeting.webex.meetings.reachability.getClientMediaPreferences, meeting.isMultistream, 0); - assert.calledWith(meeting.webex.meetings.reachability.getReachabilityReportToAttachToRoap); + assert.calledWith(meeting.webex.meetings.reachability.getClientMediaPreferences, meeting.isMultistream, 0); + assert.calledWith(meeting.webex.meetings.reachability.getReachabilityReportToAttachToRoap); +
3892-3897: Inconsistent indentation in newly added linesThe indentation of these new lines is incorrect compared to the surrounding code. They should be indented at the same level as the surrounding code.
- localSdp: `{"audioMuted":${audioMuted},"videoMuted":${videoMuted},"roapMessage":{"messageType":"OFFER","sdps":["${sdp}"],"version":"2","seq":"${seq}","tieBreaker":"${tieBreaker}","headers":["includeAnswerInHttpResponse","noOkInTransaction"]},"reachability":{"id":"fake reachability"}}`, - mediaId: 'fake media id', - }, - ], - clientMediaPreferences: { - id: 'fake clientMediaPreferences', + localSdp: `{"audioMuted":${audioMuted},"videoMuted":${videoMuted},"roapMessage":{"messageType":"OFFER","sdps":["${sdp}"],"version":"2","seq":"${seq}","tieBreaker":"${tieBreaker}","headers":["includeAnswerInHttpResponse","noOkInTransaction"]},"reachability":{"id":"fake reachability"}}`, + mediaId: 'fake media id', + }, + ], + clientMediaPreferences: { + id: 'fake clientMediaPreferences',
3918-3922: Inconsistent indentation in newly added linesThe indentation of these new lines is incorrect compared to the surrounding code. They should be indented at the same level as the surrounding code.
- id: 'fake clientMediaPreferences', - }, - localMedias: [ - { - localSdp: `{"audioMuted":${audioMuted},"videoMuted":${videoMuted},"roapMessage":{"messageType":"OK","version":"2","seq":"${seq}"},"reachability":{"id":"fake reachability"}}`, + id: 'fake clientMediaPreferences', + }, + localMedias: [ + { + localSdp: `{"audioMuted":${audioMuted},"videoMuted":${videoMuted},"roapMessage":{"messageType":"OK","version":"2","seq":"${seq}"},"reachability":{"id":"fake reachability"}}`,packages/@webex/plugin-meetings/src/reachability/index.ts (4)
64-65: Explicitly define types forstartTimeandtotalDurationThe properties
startTimeandtotalDurationare initialized toundefined. To enhance type safety and code clarity, consider explicitly typing these properties, for example:startTime: number | undefined = undefined; totalDuration: number | undefined = undefined;
98-105: Refine the type ofpreviousReportparameterThe
previousReportparameter in thegetClustersmethod is currently typed asany. To improve type safety, consider defining a specific interface or type forpreviousReportthat matches its expected structure.
941-975: Improve typing ingetClientMediaPreferencesmethodIn the
getClientMediaPreferencesmethod, the return type includes a spread operator withreachabilityReport. SincereachabilityReportcan vary based onorpheusApiVersion, ensure that theClientMediaPreferencesinterface accurately reflects all possible properties to maintain type safety.
984-994: Clarify return type forgetReachabilityReportToAttachToRoapThe method
getReachabilityReportToAttachToRoapreturns aPromise<ReachabilityReportV0 | undefined>. WhenorpheusApiVersionis defined (version 1), it returnsundefined. Consider documenting this behavior clearly or returning a more explicit type to avoid confusion.packages/@webex/plugin-meetings/src/reachability/reachability.types.ts (2)
36-41: SimplifyTransportResultForBackendtype definitionsThe
TransportResultForBackendtype uses strings'true'and'false'instead of boolean values. To prevent confusion and potential errors, consider using boolean types and handling the conversion to strings where necessary for backend communication.
77-81: Ensure all properties inClientMediaPreferencesare properly definedIn the
ClientMediaPreferencesinterface, thejoinCookieproperty is typed asany. To improve type safety, define a specific interface or type forjoinCookiebased on its structure.packages/@webex/plugin-meetings/test/unit/spec/roap/request.ts (1)
85-126: Enhance test coverage forsendRoapmethodThe current test only covers the standard case. To improve test robustness, consider adding test cases for scenarios such as:
- Handling failures when fetching reachability data.
- Verifying behavior when
isMultistreamisfalse.- Testing error handling when
locusMediaRequestis undefined.packages/@webex/plugin-meetings/test/unit/spec/reachability/request.js (1)
66-90: Add test cases for edge cases in 'early-call-min-clusters' handlingWhile the happy path is well tested, consider adding test cases for:
- Invalid 'early-call-min-clusters' values
- Undefined previousReport
it('sends a POST request with the correct params when trigger is "startup"', async () => { const res = await reachabilityRequest.getClusters('startup', IP_VERSION.only_ipv4, previousReport); // ... existing test ... }); +it('handles undefined previousReport correctly', async () => { + const res = await reachabilityRequest.getClusters('startup', IP_VERSION.only_ipv4, undefined); + const requestParams = webex.request.getCall(0).args[0]; + assert.deepEqual(requestParams.body['previous-report'], undefined); +}); + +it('validates early-call-min-clusters option', async () => { + const invalidReport = { + 'early-call-min-clusters': -1, + }; + await assert.isRejected( + reachabilityRequest.getClusters('startup', IP_VERSION.only_ipv4, invalidReport), + /Invalid early-call-min-clusters value/ + ); +});packages/@webex/plugin-meetings/src/meeting/locusMediaRequest.ts (1)
Line range hint
222-229: Enhance metrics with clientMediaPreferences dataThe metrics submission should include clientMediaPreferences data for better monitoring and debugging capabilities.
this.webex.internal.newMetrics.submitClientEvent({ name: 'client.locus.media.request', options: { meetingId: this.config.meetingId, + clientMediaPreferences: request.clientMediaPreferences, + earlyCallMinClusters: request.clientMediaPreferences?.earlyCallMinClusters, }, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (20)
packages/@webex/plugin-meetings/src/meeting/index.ts(0 hunks)packages/@webex/plugin-meetings/src/meeting/locusMediaRequest.ts(3 hunks)packages/@webex/plugin-meetings/src/meeting/request.ts(4 hunks)packages/@webex/plugin-meetings/src/meeting/util.ts(3 hunks)packages/@webex/plugin-meetings/src/reachability/clusterReachability.ts(1 hunks)packages/@webex/plugin-meetings/src/reachability/index.ts(18 hunks)packages/@webex/plugin-meetings/src/reachability/reachability.types.ts(1 hunks)packages/@webex/plugin-meetings/src/reachability/request.ts(1 hunks)packages/@webex/plugin-meetings/src/roap/index.ts(4 hunks)packages/@webex/plugin-meetings/src/roap/request.ts(3 hunks)packages/@webex/plugin-meetings/src/roap/turnDiscovery.ts(2 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js(4 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/locusMediaRequest.ts(2 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/request.js(3 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/utils.js(2 hunks)packages/@webex/plugin-meetings/test/unit/spec/reachability/index.ts(1 hunks)packages/@webex/plugin-meetings/test/unit/spec/reachability/request.js(2 hunks)packages/@webex/plugin-meetings/test/unit/spec/roap/index.ts(1 hunks)packages/@webex/plugin-meetings/test/unit/spec/roap/request.ts(4 hunks)packages/@webex/plugin-meetings/test/unit/spec/roap/turnDiscovery.ts(15 hunks)
💤 Files with no reviewable changes (1)
- packages/@webex/plugin-meetings/src/meeting/index.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/@webex/plugin-meetings/src/meeting/util.ts
- packages/@webex/plugin-meetings/test/unit/spec/roap/turnDiscovery.ts
- packages/@webex/plugin-meetings/test/unit/spec/meeting/utils.js
- packages/@webex/plugin-meetings/test/unit/spec/meeting/request.js
- packages/@webex/plugin-meetings/src/roap/index.ts
🔇 Additional comments (15)
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (1)
Line range hint 1-4000: LGTM! The test file is well structured and provides good coverage
The test file demonstrates good testing practices:
- Comprehensive test coverage of Meeting class functionality
- Good use of test doubles (stubs, spies) via sinon
- Well organized test structure with clear describe/it blocks
- Good error case coverage
- Proper cleanup in afterEach blocks
The only minor issues are some inconsistent indentation in the newly added lines, which I've highlighted above.
packages/@webex/plugin-meetings/src/reachability/index.ts (4)
116-120: 🛠️ Refactor suggestion
Avoid using @ts-ignore to suppress TypeScript errors
The @ts-ignore directive before this.webex.boundedStorage.put suppresses TypeScript errors, which may hide underlying issues. Instead, address the type mismatch directly by ensuring that boundedStorage.put and its parameters are correctly typed.
175-178: 🛠️ Refactor suggestion
Remove @ts-ignore and resolve TypeScript errors
Using @ts-ignore on this.webex.boundedStorage.get hides potential type errors. Properly type the boundedStorage.get method and its return value to enhance code reliability and maintainability.
210-211: 🛠️ Refactor suggestion
Eliminate @ts-ignore and ensure correct typing
The @ts-ignore directive before this.getReachabilityResults() suppresses TypeScript checks. Instead, ensure that getReachabilityResults is accurately typed and that any returned promises are properly handled to maintain strong type safety.
572-589:
Ensure termination condition in resolveReachabilityPromise
When resolveReachabilityPromise detects that the minimum required clusters are not reached, it calls gatherReachabilityFallback, which in turn may call getClusters again. Ensure there is a mechanism to prevent infinite recursion or excessive retries, such as a maximum retry count.
packages/@webex/plugin-meetings/src/roap/request.ts (1)
43-54:
Add error handling for asynchronous calls
The asynchronous calls to getReachabilityReportToAttachToRoap and getClientMediaPreferences lack error handling. If these calls fail, it could lead to unhandled promise rejections or undefined values. Consider wrapping these calls in try-catch blocks to handle potential errors gracefully.
Apply the following changes to handle errors:
+ try {
const reachability =
await this.webex.meetings.reachability.getReachabilityReportToAttachToRoap();
const clientMediaPreferences =
await this.webex.meetings.reachability.getClientMediaPreferences(
isMultistream,
MeetingUtil.getIpVersion(this.webex)
);
+ } catch (error) {
+ LoggerProxy.logger.error('RoapRequest#sendRoap --> Error fetching reachability data:', error);
+ // Handle the error or rethrow
+ throw error;
+ }Likely invalid or redundant comment.
packages/@webex/plugin-meetings/src/reachability/request.ts (1)
38-93:
Add error handling to the getClusters method
The getClusters method lacks error handling for the HTTP request. In case of server errors or network issues, the promise may reject without proper logging or retry logic. Consider adding a .catch() block to handle errors and provide informative logs.
Apply this diff to add error handling:
}),
'internal.get.cluster.time'
).then((res) => {
// success handling
+ }).catch((error) => {
+ LoggerProxy.logger.error(`ReachabilityRequest#getClusters --> Failed to get clusters: ${error}`);
+ throw error;
});packages/@webex/plugin-meetings/test/unit/spec/roap/index.ts (1)
164-166: LGTM! Test coverage is comprehensive
The test updates properly verify the isMultistream parameter and include thorough error handling scenarios.
packages/@webex/plugin-meetings/src/meeting/locusMediaRequest.ts (1)
18-18: Add proper validation for clientMediaPreferences
The clientMediaPreferences are added to the request body without validation.
packages/@webex/plugin-meetings/src/reachability/clusterReachability.ts (1)
9-9: LGTM! Good type organization.
Moving the type definition to a separate types file improves code organization and maintainability.
packages/@webex/plugin-meetings/test/unit/spec/meeting/locusMediaRequest.ts (2)
37-49: LGTM! Well-structured test data.
The clientMediaPreferences object is well-structured with all required fields for testing:
- preferTranscoding
- joinCookie with required fields
- ipver using the constant
- reachability with version and result
63-73: Fix indentation in clientMediaPreferences object.
The indentation in the reachability object is inconsistent with the parent structure.
packages/@webex/plugin-meetings/src/roap/turnDiscovery.ts (1)
411-411: LGTM! Consistent addition of isMultistream parameter.
The isMultistream parameter has been consistently added to both ROAP request methods:
- sendRoapTurnDiscoveryRequest
- sendRoapOK
This aligns with the architectural changes moving from meetingId to isMultistream parameter.
Let's verify the consistent usage of isMultistream across the codebase:
Also applies to: 452-452
✅ Verification successful
isMultistream parameter is consistently used across ROAP-related files
The verification confirms that the isMultistream parameter is properly integrated across all relevant ROAP files:
- In
request.ts: Used in the request interface and passed to getClientMediaPreferences - In
index.ts: Consistently used in multiple ROAP operations (sendRoapMessage, sendRoapAnswer) - In
turnDiscovery.ts: Properly added to both TURN discovery and response handling
The changes align with the architectural shift and maintain consistency throughout the ROAP implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of isMultistream parameter in ROAP-related files
# Test: Search for isMultistream usage in ROAP-related files
ast-grep --pattern 'isMultistream: $_' packages/@webex/plugin-meetings/src/roap/
Length of output: 82
Script:
#!/bin/bash
# Let's try a different approach using ripgrep to search for isMultistream
rg -l "isMultistream" packages/@webex/plugin-meetings/src/roap/
# Then get more context around the usage
rg -A 2 -B 2 "isMultistream" packages/@webex/plugin-meetings/src/roap/
Length of output: 5079
packages/@webex/plugin-meetings/src/meeting/request.ts (1)
33-33: LGTM! Clean implementation of client media preferences.
The changes properly integrate the new clientMediaPreferences parameter into the joinMeeting method, replacing the previous media preferences handling with a more structured approach.
Also applies to: 132-132, 154-154, 176-176
packages/@webex/plugin-meetings/test/unit/spec/reachability/index.ts (1)
1668-1928: LGTM! Comprehensive test coverage for the fallback mechanism.
The new test suite thoroughly covers:
- Retry mechanism when minimum required clusters are not reached
- Handling of empty cluster lists on first and subsequent attempts
- Proper state management during retries
- Verification of join cookie handling
- Edge cases for isWebexMediaBackendUnreachable
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
packages/@webex/plugin-meetings/src/roap/request.ts (2)
28-28: Verify parameter naming consistencyThe
isMultistreamparameter has been added, but its name could be more descriptive. Consider renaming toenableMultistreamoruseMultistreamModeto better indicate its boolean nature and purpose.
46-51: Consider extracting default media preferencesThe default media preferences object should be extracted into a constant or utility function for reusability and maintainability.
+const DEFAULT_CLIENT_MEDIA_PREFERENCES: ClientMediaPreferences = { + joinCookie: undefined, + ipver: IP_VERSION.unknown, + preferTranscoding: false, +}; let clientMediaPreferences: ClientMediaPreferences = { - // bare minimum fallback value that should allow us to join; - joinCookie: undefined, - ipver: IP_VERSION.unknown, - preferTranscoding: !isMultistream, + ...DEFAULT_CLIENT_MEDIA_PREFERENCES, + preferTranscoding: !isMultistream, };packages/@webex/plugin-meetings/test/unit/spec/roap/request.ts (1)
89-127: Enhance test coverage for sendRoap success pathWhile the basic success path is covered, additional test cases would improve coverage:
- Test with different
isMultistreamvalues- Verify sequence number handling
- Test media connection response parsing
it('handles different isMultistream values correctly', async () => { const locusMediaRequest = {send: sinon.stub().resolves({body: {locus: {}}})}; await roapRequest.sendRoap({ locusSelfUrl: locusUrl, mediaId: 'mediaId', roapMessage: {seq: 'seq'}, meetingId: 'meeting-id', isMultistream: false, locusMediaRequest, }); assert.calledWith( webex.meetings.reachability.getClientMediaPreferences, false, IP_VERSION.ipv4_and_ipv6 ); const requestParams = locusMediaRequest.send.getCall(0).args[0]; assert.isTrue(requestParams.clientMediaPreferences.preferTranscoding); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
packages/@webex/plugin-meetings/src/meeting/util.ts(3 hunks)packages/@webex/plugin-meetings/src/roap/request.ts(3 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/utils.js(3 hunks)packages/@webex/plugin-meetings/test/unit/spec/roap/request.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/@webex/plugin-meetings/test/unit/spec/meeting/utils.js
- packages/@webex/plugin-meetings/src/meeting/util.ts
🔇 Additional comments (3)
packages/@webex/plugin-meetings/src/roap/request.ts (2)
80-81: Verify reachability data structure
The reachability and clientMediaPreferences are being passed directly to the request. Ensure that these objects match the expected structure on the server side.
✅ Verification successful
Based on the search results, I can now generate a final response. The code shows that:
- The reachability types are well-defined in
reachability.types.tswith proper interfaces for both V0 and V1 versions - The reachability and clientMediaPreferences are handled by the reachability service which properly constructs these objects
- The test files show proper validation of these objects in the request
- The code has proper error handling for cases where reachability data cannot be obtained
The reachability data structure is properly handled and matches server expectations
- The reachability report has two versions (V0 and V1) with proper type definitions
- The clientMediaPreferences object is constructed by the reachability service with proper error handling
- Test coverage validates the correct structure of these objects in requests
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the interface definitions and usages
ast-grep --pattern 'interface $name {
$$$
reachability?: $_
clientMediaPreferences?: $_
$$$
}'
Length of output: 104
Script:
#!/bin/bash
# Let's try to find the type definitions and usages in a different way
rg -A 5 "reachability.*clientMediaPreferences"
# Also search for any type definitions or interfaces related to these properties
rg "type.*reachability|interface.*reachability" -A 5
# Search for actual usage of these properties
ast-grep --pattern 'reachability: $_'
Length of output: 68493
Script:
#!/bin/bash
# Let's search for the type definitions and usages in a more specific way
rg "type.*ReachabilityReport|interface.*ReachabilityReport" -A 10
# Also search for the actual usage of these properties in request construction
rg "clientMediaPreferences.*reachability" -B 5 -A 5
Length of output: 71243
53-66:
Improve error handling and type safety
Several issues in this code block need attention:
- Multiple
@ts-ignorecomments are masking potential type safety issues - The error handling could be more specific and provide better error context
try {
clientMediaPreferences =
- // @ts-ignore
- await this.webex.meetings.reachability.getClientMediaPreferences(
+ await (this.webex.meetings.reachability as any).getClientMediaPreferences(
isMultistream,
- // @ts-ignore
MeetingUtil.getIpVersion(this.webex)
);
reachability =
- // @ts-ignore
- await this.webex.meetings.reachability.getReachabilityReportToAttachToRoap();
+ await (this.webex.meetings.reachability as any).getReachabilityReportToAttachToRoap();
} catch (error) {
- LoggerProxy.logger.error('Roap:request#sendRoap --> reachability error:', error);
+ LoggerProxy.logger.error(
+ 'Roap:request#sendRoap --> Failed to fetch reachability data or client media preferences:',
+ {error, isMultistream}
+ );
}✅ Verification successful
Let me analyze the reachability module to understand the type definitions and verify if we can properly type these calls.
Let me try a different approach to find the reachability implementation:
Based on the search results, I can now provide a final response:
Type definitions exist and can be properly typed
The @ts-ignore comments can be removed as the Reachability class has proper type definitions:
getClientMediaPreferencesis defined with types:(isMultistream: boolean, ipver?: IP_VERSION) => Promise<ClientMediaPreferences>getReachabilityReportToAttachToRoapis defined with type:() => Promise<ReachabilityReportV0 | undefined>
The suggested changes in the review are correct and will improve type safety by:
- Using type assertions instead of ignoring type checks
- Providing better error context in the catch block
The implementation matches the test usage shown in test/unit/spec/roap/request.ts, confirming these are the correct method signatures.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other occurrences of @ts-ignore to ensure consistent type handling
rg "@ts-ignore" --type ts
Length of output: 44191
Script:
#!/bin/bash
# Check the reachability module implementation and type definitions
ast-grep --pattern 'class Reachability {
$$$
getClientMediaPreferences($$$) {
$$$
}
$$$
}'
ast-grep --pattern 'class Reachability {
$$$
getReachabilityReportToAttachToRoap($$$) {
$$$
}
$$$
}'
Length of output: 219
Script:
#!/bin/bash
# Search for reachability class and method implementations
rg -A 5 "class Reachability" --type ts
# Search for the specific method implementations
rg -A 5 "getClientMediaPreferences|getReachabilityReportToAttachToRoap" --type ts
Length of output: 8861
packages/@webex/plugin-meetings/test/unit/spec/roap/request.ts (1)
27-30: LGTM: Proper test setup for reachability stubs
The reachability stubs are correctly set up with appropriate mock implementations.
| * @param {boolean} isRetry | ||
| * @private | ||
| * @returns {Promise<{clusters: ClusterList, joinCookie: any}>} | ||
| */ | ||
| async getClusters(isRetry = false): Promise<{clusters: ClusterList; joinCookie: any}> { | ||
| async getClusters( | ||
| trigger: string, |
There was a problem hiding this comment.
Question: Can we type the trigger or do we not know all the values that orpheus supports?
There was a problem hiding this comment.
yeah we can type it, or at least the values that we use, I'll add that
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
packages/@webex/plugin-meetings/src/reachability/reachability.types.ts (1)
77-82: Consider improving type safety forjoinCookie.The
joinCookieproperty is typed asany, which reduces type safety. Consider defining a specific type for the join cookie structure.- joinCookie: any; + joinCookie: { + // Define specific join cookie properties here + id?: string; + // Add other properties as needed + };packages/@webex/plugin-meetings/src/reachability/request.ts (1)
Line range hint
117-122: Consider adding type definitions for boundedStorage.The use of
@ts-ignoresuggests missing type definitions for the storage operations.Consider defining an interface for the storage operations:
interface BoundedStorage { put(namespace: string, key: string, value: string): Promise<void>; get(namespace: string, key: string): Promise<string>; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
packages/@webex/plugin-meetings/src/reachability/index.ts(18 hunks)packages/@webex/plugin-meetings/src/reachability/reachability.types.ts(1 hunks)packages/@webex/plugin-meetings/src/reachability/request.ts(2 hunks)packages/@webex/plugin-meetings/test/unit/spec/reachability/request.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/@webex/plugin-meetings/test/unit/spec/reachability/request.js
🔇 Additional comments (11)
packages/@webex/plugin-meetings/src/reachability/reachability.types.ts (5)
4-15: LGTM! Well-structured type definitions for transport results.
The types are well-documented and properly structured to handle different transport protocols and their results.
17-30: LGTM! Comprehensive metrics structure.
The metrics type effectively covers all combinations of protocols (UDP, TCP, XTLS) and network types (public, VMN) for both success and failure cases.
32-47: LGTM! Well-documented backend-specific types.
The types are properly structured for backend communication with clear documentation explaining the string-based boolean representation.
62-75: LGTM! Well-structured report type for API v1.
The type properly implements the 'early-call-min-clusters' option with room for future extensions, as indicated by the comments.
84-85: LGTM! Well-defined trigger type.
The type correctly defines the currently supported triggers with clear documentation about potential future extensions.
packages/@webex/plugin-meetings/src/reachability/request.ts (2)
39-43: LGTM! Well-structured method signature.
The method signature properly defines the new parameters with appropriate types.
Line range hint 108-116: LGTM! Proper handling of discovery options.
The code correctly extracts and stores the discovery options, including the new 'early-call-min-clusters' option.
packages/@webex/plugin-meetings/src/reachability/index.ts (4)
54-55: LGTM! Well-defined class properties.
The new properties minRequiredClusters and orpheusApiVersion are appropriately typed as optional numbers.
942-976: LGTM! Well-implemented client media preferences.
The getClientMediaPreferences method properly handles both API versions and correctly includes the reachability report for v1.
978-995: LGTM! Clear version-based ROAP report handling.
The getReachabilityReportToAttachToRoap method properly differentiates between API versions and returns appropriate data.
239-260:
Consider implementing a retry limit in gatherReachabilityFallback.
The fallback mechanism could potentially lead to an infinite loop if the minimum required clusters are never reached.
Add a retry limit:
+ private maxFallbackRetries = 3;
+ private fallbackRetryCount = 0;
public async gatherReachabilityFallback(): Promise<void> {
try {
+ if (this.fallbackRetryCount >= this.maxFallbackRetries) {
+ LoggerProxy.logger.warn(
+ 'Reachability:index#gatherReachabilityFallback --> Max retries reached'
+ );
+ return;
+ }
+ this.fallbackRetryCount++;
const reachabilityReport = await this.getReachabilityReport();Likely invalid or redundant comment.
COMPLETES #SPARK-577185
This pull request addresses
As parf of IPv6 feature Orpheus has extended their API. Now the /clusters API response may contain a field "early-call-min-clusters" which says the minimum required number of clusters that we should reach. If we fail to reach them, we should call /clusters API again with our results and they will give us a different set of URLs to try (and new value for "early-call-min-clusters"). If we fail to reach minimum number of clusters we should keep calling them.
Each time we call them they also give us updated
joinCookie, so when we join and pass that cookie, it will allow Orpheus to see the history of all our calls to /clusters API and reachability results sent to them in previous calls.To use this new API feature we had to move to "version 1" of Orpheus API, which has slightly different format for reachability. Main difference is that in v1 reachability results are inside
clientMediaPreferencesobject.wiki pages for reference:
https://confluence-eng-gpk2.cisco.com/conf/display/CLO/Discovery+Algorithm+Improvements
https://confluence-eng-gpk2.cisco.com/conf/display/CLO/New+Reachability+Format+and+Metrics
https://confluence-eng-gpk2.cisco.com/conf/pages/viewpage.action?pageId=563898338
by making the following changes
to test this:
the new Orpheus API is available only in integration environment and you have to ask Rafa from Orpheus team to enable a feature toggle for your user or you can use one of my users.
Change Type
The following scenarios where tested
unit tests and manual testing in integration with Orpheus and web app
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
Release Notes
New Features
joinMeetingmethod to includeclientMediaPreferencesfor better media management.Bug Fixes
Tests