fix: cannot unmute web client in moderated unmute mode when unmuteAllowed is false #3992
Conversation
marcin-bazyl
added
the
validated
WalkthroughThe changes in this pull request involve modifications to the Changes
Possibly related PRs
Suggested labels
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 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 (
|
![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 (2)
packages/@webex/plugin-meetings/src/meeting/muteState.ts (1)
413-415: Consider adding parameter validation and handling race conditionsWhile the state updates are logically correct, consider these improvements:
- Add validation for the
unmuteAllowedparameter at the start of the method- Consider potential race conditions with concurrent state updates
Add parameter validation at the start of the method:
public handleServerLocalUnmuteRequired(meeting: any, unmuteAllowed: boolean) { + if (typeof unmuteAllowed !== 'boolean') { + throw new ParameterError('unmuteAllowed must be a boolean'); + } + if (!this.state.client.enabled) { // ... rest of the methodpackages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (1)
12205-12232: Consider adding error case test coverageWhile the happy path is well tested, consider adding test cases for error scenarios:
- What happens if audio is not initialized?
- What happens if the handler throws an error?
it('handles errors when audio is not initialized', async () => { meeting.audio = null; await meeting.locusInfo.emitScoped( {}, LOCUSINFO.EVENTS.LOCAL_UNMUTE_REQUIRED, {unmuteAllowed: true} ); // Add assertions }); it('handles errors from handleServerLocalUnmuteRequired', async () => { meeting.audio = { handleServerLocalUnmuteRequired: sinon.stub().throws(new Error('test error')) }; await meeting.locusInfo.emitScoped( {}, LOCUSINFO.EVENTS.LOCAL_UNMUTE_REQUIRED, {unmuteAllowed: true} ); // Add assertions });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
packages/@webex/plugin-meetings/src/meeting/index.ts(1 hunks)packages/@webex/plugin-meetings/src/meeting/muteState.ts(1 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js(1 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/muteState.js(6 hunks)
🔇 Additional comments (10)
packages/@webex/plugin-meetings/src/meeting/muteState.ts (3)
397-397: LGTM: Method signature update aligns with PR objective
The addition of the required unmuteAllowed parameter correctly implements the fix for handling unmute requests in moderated mode.
Also applies to: 400-400
407-407: LGTM: Enhanced logging improves debuggability
The updated log message now includes the unmuteAllowed state, which will help with troubleshooting moderated unmute issues.
Line range hint 400-424: Verify security implications of unmute state changes
Let's ensure this change doesn't inadvertently bypass moderation controls or create race conditions in state transitions.
packages/@webex/plugin-meetings/test/unit/spec/meeting/muteState.js (5)
Line range hint 154-165: LGTM! Comprehensive test coverage for audio unmute.
The test case thoroughly verifies that:
- The local stream is unmuted when
handleServerLocalUnmuteRequiredis called - The
unmuteAllowedflag is properly set - The unmute operation is propagated to the server
189-190: LGTM! Proper handling of system mute state.
The test case correctly verifies that the unmute operation respects system mute state, preventing server updates when system is muted.
Line range hint 220-231: LGTM! Comprehensive test coverage for video unmute.
The test case thoroughly verifies that:
- The local stream is unmuted when
handleServerLocalUnmuteRequiredis called - The
unmuteAllowedflag is properly set - The unmute operation is propagated to the server
255-255: LGTM! Proper handling of system mute state for video.
The test case correctly verifies that the video unmute operation respects system mute state, preventing server updates when system is muted.
Line range hint 1-700: Verify edge cases for unmute operations.
While the test coverage is comprehensive for the happy path and system mute scenarios, consider adding test cases for:
- Network failures during unmute
- Race conditions between multiple unmute requests
- Edge cases where
unmuteAllowedis toggled rapidly
packages/@webex/plugin-meetings/src/meeting/index.ts (1)
Line range hint 3107-3116: LGTM! Bug fix for unmute handling
The changes correctly propagate the unmuteAllowed flag from the payload to the audio handler's handleServerLocalUnmuteRequired method. This fixes the issue where users couldn't unmute in moderated unmute mode when unmuteAllowed is false.
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (1)
12203-12239: LGTM! Well structured test cases for LOCAL_UNMUTE_REQUIRED event handling
The test implementation follows good practices:
- Tests both true/false cases for unmuteAllowed parameter
- Properly verifies event payload structure and handler calls
- Uses clean test setup with proper stubs and assertions
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
|
when testing, be aware that there is a separate issue related to moderated unmute mode: https://jira-eng-gpk2.cisco.com/jira/browse/SPARK-583170 which will be fixed in a separate PR |
COMPLETES #SPARK-480563
This pull request addresses
When user is not allowed to unmute himself/herself, then when we receive unmute required from Locus and SDK calls setUserMuted(false), it fails because of the
unmuteAllowedcheck.by making the following changes
When Locus sends unmute required, they also send unmuteAllowed=true in the same update, so we need to make sure we process it and update unmuteAllowed when we handle unmute required in handleServerLocalUnmuteRequired()
Change Type
The following scenarios where tested
unit tests, manual test with 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
New Features
LOCAL_UNMUTE_REQUIREDevent that triggers appropriate actions based on user permissions.Bug Fixes
Tests