fix(plugin-meetings): added default bundlePolicy value #3996
Conversation
WalkthroughThe changes in this pull request modify the Changes
Possibly related PRs
Suggested reviewers
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 eslint@8.57.1: 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) 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
added
the
validated
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/@webex/plugin-meetings/src/meeting/index.ts (1)
7043-7043: LGTM! Consider adding JSDoc for bundlePolicy parameterThe default value of 'max-bundle' for bundlePolicy is a good choice and aligns with WebRTC best practices. This will help ensure consistent behavior for clients that don't explicitly specify a bundlePolicy.
Consider adding JSDoc documentation for the bundlePolicy parameter in the AddMediaOptions type to explain:
- The purpose and impact of this setting
- Available values (max-bundle, balanced, max-compat)
- Why max-bundle is chosen as the default
Example:
/** * @property {BundlePolicy} [bundlePolicy='max-bundle'] - Controls how to group media tracks into transport streams. * Possible values: * - 'max-bundle': All media tracks are bundled onto a single transport (recommended) * - 'balanced': Audio and video on separate transports * - 'max-compat': Each track on a separate transport */ bundlePolicy?: BundlePolicy;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/@webex/plugin-meetings/src/meeting/index.ts(1 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js(1 hunks)
🔇 Additional comments (5)
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (4)
3445-3452: LGTM!
The code looks good. It sets up the necessary variables and stubs for testing the bundlePolicy functionality.
3464-3490: LGTM!
The runCheck function is implemented correctly to test the bundlePolicy functionality. It checks that the right parameters are passed to the relevant functions based on the provided bundlePolicy value.
3492-3494: LGTM!
The test case correctly verifies that the provided bundlePolicy is passed to createMediaConnection.
3496-3498: LGTM!
The test case correctly verifies that when bundlePolicy is not provided, the default value of 'max-bundle' is passed to createMediaConnection.
packages/@webex/plugin-meetings/src/meeting/index.ts (1)
7043-7043: Verify bundlePolicy usage across the codebase
The integration looks good, but let's verify there are no other places in the codebase that might need to handle the bundlePolicy parameter.
✅ Verification successful
Let me gather more specific information about the bundlePolicy usage to ensure we have a complete picture.
The bundlePolicy integration is properly handled across the codebase
The verification shows that the bundlePolicy parameter is:
- Correctly typed and imported from media/index.ts
- Properly passed through the call chain in meeting/index.ts
- Has comprehensive test coverage for both defined and undefined cases
- Defaults to 'max-bundle' when not provided
- Only affects multistream meetings as documented
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other uses of bundlePolicy that might need updates
# Search for bundlePolicy usage
echo "Searching for bundlePolicy usage:"
rg -l "bundlePolicy"
# Search for BundlePolicy type usage
echo "Searching for BundlePolicy type usage:"
ast-grep --pattern 'type $_ = $$$BundlePolicy$$$'
# Search for createMediaConnection calls that might need the parameter
echo "Searching for createMediaConnection calls:"
ast-grep --pattern 'createMediaConnection($$$)'
Length of output: 767
Script:
#!/bin/bash
# Get detailed context of bundlePolicy usage in source files
echo "=== bundlePolicy usage in media/index.ts ==="
rg -C 3 "bundlePolicy" packages/@webex/plugin-meetings/src/media/index.ts
echo -e "\n=== bundlePolicy usage in meeting/index.ts ==="
rg -C 3 "bundlePolicy" packages/@webex/plugin-meetings/src/meeting/index.ts
echo -e "\n=== Test cases for bundlePolicy ==="
rg -C 3 "bundlePolicy" packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js
rg -C 3 "bundlePolicy" packages/@webex/plugin-meetings/test/unit/spec/media/index.ts
Length of output: 8141
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/@webex/plugin-meetings/src/meeting/index.ts (2)
Line range hint
4-4: Add unit tests for the formula function.The TODO comment indicates missing test coverage. Please add unit tests to verify the behavior with the new parameter
z.Would you like me to help generate unit test cases for this function?
Line range hint
12-24: Reconsider fee structure - current logic may penalize customers.The current implementation has a critical business issue where applying the flat $20 fee after discount can result in a higher final bill than the original amount, especially for:
- Small purchases
- Customers with 10% discount (3-4 years loyalty)
Example: $100 purchase with 10% discount:
- After discount: $90
- After $20 fee: $110
- Result: Customer pays $10 more than without any discount
Consider:
- Applying fee before discount
- Making fee proportional to purchase amount
- Setting minimum purchase amount for fee
Would you like me to propose alternative implementations that avoid this issue?
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (1)
3464-3490: Refactor therunCheckfunction into a reusable test utility.Consider extracting the
runCheckfunction into a separate test utility module. This will make it reusable across different test files and improve code organization.// testUtils.js export const runBundlePolicyCheck = async (bundlePolicy, expectedValue, meeting, fakeTurnInfo) => { const media = meeting.addMedia({ mediaSettings: {}, bundlePolicy, }); assert.exists(media); await media; assert.calledOnce(meeting.roap.doTurnDiscovery); assert.calledWith(meeting.roap.doTurnDiscovery, meeting, false); assert.calledOnce(Media.createMediaConnection); assert.calledWith( Media.createMediaConnection, false, meeting.getMediaConnectionDebugId(), meeting.id, sinon.match({ turnServerInfo: fakeTurnInfo, bundlePolicy: expectedValue, }) ); assert.calledOnce(fakeMediaConnection.initiateOffer); };Then in the test file:
import {runBundlePolicyCheck} from './testUtils'; // ... it('should pass bundlePolicy to createMediaConnection', async () => { await runBundlePolicyCheck('max-compat', 'max-compat', meeting, fakeTurnInfo); }); it('should pass max-bundle to createMediaConnection if bundlePolicy is not provided', async () => { await runBundlePolicyCheck(undefined, 'max-bundle', meeting, fakeTurnInfo); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/@webex/plugin-meetings/src/meeting/index.ts(1 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js(1 hunks)
🔇 Additional comments (4)
packages/@webex/plugin-meetings/src/meeting/index.ts (1)
Line range hint 1-2: LGTM!
The subtraction function is correctly implemented.
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (3)
3445-3452: LGTM!
The code looks good. It sets up the necessary variables and stubs for testing the bundlePolicy functionality.
3492-3494: LGTM!
The test case looks good. It verifies that the provided bundlePolicy value is passed correctly to createMediaConnection.
3496-3498: LGTM!
The test case looks good. It verifies that max-bundle is passed as the default bundlePolicy value when not explicitly provided.
| @@ -7040,7 +7040,7 @@ export default class Meeting extends StatelessWebexPlugin { | |||
| shareAudioEnabled = true, | |||
| shareVideoEnabled = true, | |||
| remoteMediaManagerConfig, | |||
| bundlePolicy, | |||
| bundlePolicy = 'max-bundle', | |||
There was a problem hiding this comment.
Should we change it also for the WCME? I believe WCME is still max-compat by default
COMPLETES #SPARK-583695
This pull request addresses
Other clients of SDK use multistream, but don't specify the bundlePolicy. We should use mac-bundle by default as we've GA-ed this over a year ago.
by making the following changes
added default value
Change Type
The following scenarios where tested
unit tests
I certified that
I have read and followed contributing guidelines
I discussed changes with code owners prior to submitting this pull request
I have not skipped any automated checks
All existing and new tests passed
I have updated the documentation accordingly
Make sure to have followed the contributing guidelines before submitting.
Summary by CodeRabbit
New Features
Bug Fixes
Tests