feat(plugin-meetings): add toggle for audio main dtx #4278
Conversation
📝 WalkthroughWalkthroughThe changes update the 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
yarn install v1.22.22 (For a CapTP with native promises, see @endo/eventual-send and @endo/captp) Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
Documentation and Community
|
k-wasniowski
added
the
validated
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/@webex/plugin-meetings/test/unit/spec/meetings/index.js (1)
403-414: Tests for new Audio Main DTX toggle featureNew test suite added for the
_toggleDisableAudioMainDtxmethod, which verifies that the method exists and properly updates the configuration when called withtrue.Consider adding a test case for toggling the feature off (passing
falseto the method) to ensure bidirectional functionality works as expected.describe('success', () => { it('should update meetings to disable audio main dtx', () => { webex.meetings._toggleDisableAudioMainDtx(true); assert.equal(webex.meetings.config.experimental.disableAudioMainDtx, true); }); + + it('should update meetings to enable audio main dtx', () => { + webex.meetings._toggleDisableAudioMainDtx(false); + assert.equal(webex.meetings.config.experimental.disableAudioMainDtx, false); + }); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
packages/@webex/media-helpers/package.json(1 hunks)packages/@webex/plugin-meetings/package.json(1 hunks)packages/@webex/plugin-meetings/src/media/index.ts(3 hunks)packages/@webex/plugin-meetings/src/meetings/index.ts(1 hunks)packages/@webex/plugin-meetings/test/unit/spec/meetings/index.js(3 hunks)packages/calling/package.json(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Initialize Project
- GitHub Check: AWS Amplify Console Web Preview
🔇 Additional comments (9)
packages/calling/package.json (1)
40-40: Dependency bump aligned with related packages
The update of@webex/internal-media-coreto version2.15.0matches the bumps in@webex/plugin-meetingsand@webex/media-helpers, ensuring consistent use of the new Audio Main DTX feature support across the SDK.packages/@webex/media-helpers/package.json (1)
25-25: Dependency version updateThis updates the
@webex/internal-media-corepackage from2.14.7to2.15.0to support the Audio Main DTX toggle feature.packages/@webex/plugin-meetings/package.json (1)
66-66: Dependency version updateThis updates the
@webex/internal-media-corepackage from2.14.7to2.15.0to support the Audio Main DTX toggle feature.packages/@webex/plugin-meetings/test/unit/spec/meetings/index.js (2)
674-674: Minor formatting improvementAdded trailing comma for better consistency in object declarations.
690-690: Minor formatting improvementAdded trailing comma for better consistency in object declarations.
packages/@webex/plugin-meetings/src/meetings/index.ts (1)
810-828: Implementation looks clean and follows existing patternsThe new
_toggleDisableAudioMainDtxmethod follows the established pattern of other toggle methods in this class, with proper type checking and conditional state update. The method is well-documented, clearly indicating it should be called beforewebex.meetings.register().packages/@webex/plugin-meetings/src/media/index.ts (3)
145-145: Parameter addition is well-placedThe new optional
disableAudioMainDtxboolean parameter is properly added to the options interface.
157-158: Clean parameter destructuringThe new parameter is correctly destructured from the options object.
175-176: Properly applied configuration parameterThe
disableAudioMainDtxflag is correctly passed into the configuration forMultistreamRoapMediaConnection, which aligns with the PR objectives for controlling Audio Main DTX functionality.
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
k-wasniowski
requested review from
SomeBody16,
antsukanova,
brycetham,
edvujic,
marcin-bazyl and
szotrabh
edvujic
left a comment
edvujic
left a comment
There was a problem hiding this comment.
Looks good, but could you check why unit tests are failing?
➤ YN0000: [@webex/plugin-meetings]: AssertError: expected MultistreamRoapMediaConnection to be called with arguments
➤ YN0000: [@webex/plugin-meetings]: { iceServers: [], disableAudioMainDtx: undefined } { iceServers: [] }
➤ YN0000: [@webex/plugin-meetings]: '"meeting id"'
➤ YN0000: [@webex/plugin-meetings]: [Function (anonymous)]
➤ YN0000: [@webex/plugin-meetings]: [Function (anonymous)]
➤ YN0000: [@webex/plugin-meetings]: [Function (anonymous)]
Yeah, looking into it |
| } | ||
|
|
||
| /** | ||
| * API to toggle usage of audio main DTX, needs to be called before webex.meetings.register() |
There was a problem hiding this comment.
the comment says "needs to be called before webex.meetings.register()" - I don't think that's true, it can be called later as long as it's done before joinWithMedia() or addMedia()
4 participants
COMPLETES #SPARK-627888
This pull request addresses
Adds a feature toggle which will help rolling out of the Audio Main DTX from the web client
Change Type
The following scenarios were tested
Tested with JS-SDK
The GAI Coding Policy And Copyright Annotation Best Practices
I certified that
Make sure to have followed the contributing guidelines before submitting.