feat(plugin-meetings): add brb logic for plugin meetings when current user steps away or returns #4031
Conversation
…PIs when current user steps away or returns (#3942) Co-authored-by: Anna Tsukanova <antsukan@cisco.com> Co-authored-by: Filip Nowakowski <fnowakow@cisco.com> Co-authored-by: evujici <evujici@cisco.com>
|
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) WalkthroughThis pull request introduces a comprehensive enhancement to the Webex Meetings Plugin, focusing on participant management and meeting functionalities. The changes primarily revolve around implementing a "Be Right Back" (BRB) feature, which allows participants to temporarily indicate their unavailability. The implementation spans multiple files across the meetings plugin, adding new methods, constants, and event handling to support this functionality. The changes improve user experience by providing more granular control over participant states during meetings. Changes
Possibly related PRs
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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). |
Co-authored-by: Anna Tsukanova <antsukan@cisco.com>
| @@ -23,6 +23,14 @@ export type ParticipantWithRoles = { | |||
| }; | |||
| }; | |||
|
|
|||
| export type ParticipantWithBrb = { | |||
| controls?: { | |||
There was a problem hiding this comment.
One thing that I haven't found easily is in what situations controls can be undefined. I would appreciate it if someone could highlight that. For now, I will leave this part consistent with other member logic and add optional controls logic.
![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 comments (2)
packages/@webex/plugin-meetings/src/meeting/index.ts (2)
Line range hint
12-24: Consider adjusting the fee structure or discount policy.The implementation of a flat $20 fee on discounted bills could negate the benefit of the discount, especially for smaller purchases or marginal loyalty tiers. This might lead to customer dissatisfaction, as the intent to reward loyalty paradoxically increases the bill.
Consider revising either the discount percentages or the flat fee application to better align with customer incentives.
Would you like assistance in generating a revised implementation?
Update requests library to version 2.32.3 to address security vulnerabilities
The current version 2.26.0 is vulnerable to:
- Session verification bypass (fixed in 2.32.0)
- Proxy-Authorization header leak (fixed in 2.31.0)
🔗 Analysis chain
Line range hint
6-6: Verify the latest stable versions and any security advisoriesEnsure that the fixed version of the
requestslibrary is secure and free from vulnerabilities.Run the following script to verify the fixed version of the
requestslibrary:🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for security advisories and latest versions of the `requests` library. # Check PyPI for latest versions curl -s https://pypi.org/pypi/requests/json | jq '.info.version' # Check for security advisories gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: PIP, package: "requests") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }'Length of output: 1597
🧹 Nitpick comments (8)
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (2)
Line range hint
3637-3732: Consider adding edge case tests for BRB functionalityConsider adding tests for the following edge cases:
it('should handle BRB enable when already enabled', async () => { meeting.isMultistream = true; meeting.meetingRequest.sendBrb = sinon.stub().resolves({body: 'test'}); // Set initial state to enabled await meeting.beRightBack(true); // Try enabling again await meeting.beRightBack(true); assert.calledOnce(meeting.meetingRequest.sendBrb); }); it('should handle BRB disable when already disabled', async () => { meeting.isMultistream = true; meeting.meetingRequest.sendBrb = sinon.stub().resolves({body: 'test'}); // Try disabling when already disabled await meeting.beRightBack(false); assert.calledOnce(meeting.meetingRequest.sendBrb); });
Line range hint
8632-8713: Consider adding error handling testsConsider adding tests for error handling scenarios:
it('should handle errors in BRB event processing', () => { const errorPayload = new Error('BRB processing failed'); meeting.locusInfo.emit( {function: 'test', file: 'test'}, LOCUSINFO.EVENTS.SELF_MEETING_BRB_CHANGED, errorPayload ); // Verify error is handled gracefully assert.calledWith( TriggerProxy.trigger, meeting, {file: 'meeting/index', function: 'setUpLocusInfoSelfListener'}, EVENT_TRIGGERS.MEETING_SELF_BRB_UPDATE, {payload: errorPayload} ); });packages/@webex/plugin-meetings/src/meeting/request.type.ts (1)
15-20: LGTM! Consider adding JSDoc comments.The
BrbOptionstype definition is well-structured with clear, required fields. Consider adding JSDoc comments to document the purpose of each field for better developer experience.Example documentation:
/** * Options for updating a participant's "Be Right Back" status */ export type BrbOptions = { /** Whether the participant is in BRB state */ enabled: boolean; /** URL of the locus where the BRB status should be updated */ locusUrl: string; /** URL of the device sending the BRB update */ deviceUrl: string; /** ID of the participant changing their BRB status */ selfId: string; };packages/@webex/plugin-meetings/src/member/types.ts (1)
26-32: LGTM! Consider documenting the optional fields.The
ParticipantWithBrbtype correctly uses optional chaining for bothcontrolsandbrbproperties, maintaining backward compatibility with existing participant objects that may not have these fields.Consider adding documentation to explain when these fields might be undefined:
/** * Represents a participant with BRB (Be Right Back) status capabilities * Note: controls may be undefined for participants without BRB capabilities * or when the participant data is not fully loaded */ export type ParticipantWithBrb = { controls?: { /** BRB status - undefined when participant hasn't used BRB feature */ brb?: { enabled: boolean; }; }; };packages/@webex/plugin-meetings/src/multistream/sendSlotManager.ts (1)
87-115: LGTM! Minor suggestion for error message consistency.The implementation is well-structured with proper validation, error handling, and logging. The restriction to
VideoMainmedia type is appropriate for BRB functionality.Consider making the error message more consistent with other methods in the class:
- `sendSlotManager cannot set source state override which media type is ${mediaType}` + `SendSlotManager cannot set source state override for media type ${mediaType}`packages/@webex/plugin-meetings/src/member/util.ts (1)
54-60: LGTM! Consider enhancing the documentation.The implementation is clean and follows the established patterns. The use of optional chaining provides good null safety.
Consider adding an example to the documentation:
/** * Checks if the participant has the brb status enabled. * * @param {ParticipantWithBrb} participant - The locus participant object. * @returns {boolean} - True if the participant has brb enabled, false otherwise. + * @example + * const isBrb = MemberUtil.isBrb(participant); // returns true if participant is in BRB state */packages/@webex/plugin-meetings/src/locus-info/selfUtils.ts (1)
165-167: Consider simplifying the undefined check.The implementation is correct, but the undefined check might be redundant since
isEqualwould handle this case.Consider simplifying to:
-SelfUtils.brbChanged = (previous, current) => - !isEqual(previous?.brb, current?.brb) && current?.brb !== undefined; +SelfUtils.brbChanged = (previous, current) => + !isEqual(previous?.brb, current?.brb);packages/@webex/plugin-meetings/src/meeting/index.ts (1)
Line range hint
4-4: Reminder: Address the TODO comment.The TODO comment indicates that tests are missing for this function. Please ensure that the additional parameter change is thoroughly tested to confirm that it behaves as expected.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (19)
docs/samples/browser-plugin-meetings/app.js(12 hunks)docs/samples/browser-plugin-meetings/index.html(0 hunks)docs/samples/browser-plugin-meetings/style.css(3 hunks)packages/@webex/media-helpers/package.json(1 hunks)packages/@webex/plugin-meetings/package.json(1 hunks)packages/@webex/plugin-meetings/src/constants.ts(2 hunks)packages/@webex/plugin-meetings/src/locus-info/index.ts(1 hunks)packages/@webex/plugin-meetings/src/locus-info/selfUtils.ts(4 hunks)packages/@webex/plugin-meetings/src/meeting/index.ts(2 hunks)packages/@webex/plugin-meetings/src/meeting/request.ts(2 hunks)packages/@webex/plugin-meetings/src/meeting/request.type.ts(1 hunks)packages/@webex/plugin-meetings/src/member/index.ts(3 hunks)packages/@webex/plugin-meetings/src/member/types.ts(1 hunks)packages/@webex/plugin-meetings/src/member/util.ts(19 hunks)packages/@webex/plugin-meetings/src/multistream/sendSlotManager.ts(2 hunks)packages/@webex/plugin-meetings/test/unit/spec/locus-info/index.js(1 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js(5 hunks)packages/@webex/plugin-meetings/test/unit/spec/member/util.js(10 hunks)packages/calling/package.json(1 hunks)
💤 Files with no reviewable changes (1)
- docs/samples/browser-plugin-meetings/index.html
✅ Files skipped from review due to trivial changes (1)
- docs/samples/browser-plugin-meetings/style.css
🧰 Additional context used
🪛 Biome (1.9.4)
packages/@webex/plugin-meetings/src/member/util.ts
[error] 113-113: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 119-119: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (24)
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (3)
Line range hint 3637-3732: New test suite added for beRightBack method
The changes add a new test suite for the beRightBack method which tests the BRB (Be Right Back) functionality in meetings. The tests cover:
- Basic method existence check
- Multistream meeting scenarios:
- Enabling BRB state
- Disabling BRB state
- Transcoded meeting scenarios:
- Ignoring BRB enable attempts
- Ignoring BRB disable attempts
The test setup includes mocking of necessary dependencies like sendBrb and media connection slots.
Line range hint 8632-8713: New test suite added for setUpLocusInfoSelfListener BRB event handling
The changes add tests for handling BRB state changes through the locus info self listener. The tests verify:
- Proper event triggering when BRB state changes
- Correct payload handling for both enable and disable states
- Integration with the trigger proxy system
The implementation looks solid and provides good coverage of the BRB event handling functionality.
8713-8732: New test suite added for setUpLocusInfoSelfListener interpretation events
The changes add tests for handling interpretation-related events through the locus info self listener. The tests verify:
- Proper event triggering for interpretation changes
- Correct payload handling
- Integration with the trigger proxy system
The implementation provides good coverage of the interpretation event handling functionality.
packages/@webex/plugin-meetings/src/multistream/sendSlotManager.ts (1)
Line range hint 1-8: LGTM! Import changes are clean and well-organized.
The addition of StreamState to the named imports is properly grouped with related types from @webex/internal-media-core.
packages/@webex/plugin-meetings/src/member/util.ts (1)
7-7: LGTM!
The type import is correctly placed and follows the established pattern.
packages/@webex/plugin-meetings/src/member/index.ts (2)
231-237: LGTM!
The property declaration and documentation are well-structured and follow the established patterns.
306-306: LGTM!
The BRB state initialization is correctly implemented and properly placed within the processParticipant method.
packages/@webex/plugin-meetings/src/locus-info/selfUtils.ts (3)
79-79: LGTM!
The getBrb utility method is well-implemented with proper null safety.
69-69: LGTM!
The brb property is correctly added to the parsed object using the getBrb utility method.
133-133: LGTM!
The brbChanged update is correctly added to the updates object.
packages/@webex/plugin-meetings/src/constants.ts (2)
306-306: LGTM: Event trigger constant follows naming convention.
The MEETING_SELF_BRB_UPDATE event trigger follows the established pattern of prefixing with MEETING_SELF_ for self-related events.
704-704: LGTM: Locus info event constant follows naming convention.
The SELF_MEETING_BRB_CHANGED event constant follows the established pattern of prefixing with SELF_MEETING_ for self-related meeting events.
packages/@webex/plugin-meetings/src/meeting/request.ts (2)
31-31: LGTM: Import statement is properly organized.
The BrbOptions type import is correctly added alongside related types.
919-943: LGTM: sendBrb method implementation is correct.
The implementation:
- Uses appropriate HTTP verb (PATCH) for partial resource update
- Follows RESTful URL pattern using participant controls endpoint
- Properly structured with TypeScript types
- Includes clear JSDoc documentation
Let's verify the endpoint structure matches other participant control endpoints:
✅ Verification successful
Let me try a more focused search to verify the endpoint pattern consistency.
Let me try one more search to specifically look for participant control endpoints in the codebase.
LGTM: The sendBrb endpoint follows the standard participant control endpoint pattern
The verification confirms that the endpoint structure ${locusUrl}/${PARTICIPANT}/${selfId}/${CONTROLS} is consistent with other participant control operations in the codebase. The pattern is used in the same file at line 931, which validates that this is the standard way to construct participant control endpoints.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify endpoint pattern consistency
# Expected: Other participant control endpoints should follow similar pattern
rg "/${PARTICIPANT}/.+/${CONTROLS}" --type ts
Length of output: 41708
Script:
#!/bin/bash
# Search for participant control endpoints in the meetings plugin
rg "/${PARTICIPANT}/.+/${CONTROLS}" packages/@webex/plugin-meetings/
Length of output: 41888
Script:
#!/bin/bash
# Search for participant control endpoints in the meetings plugin
ast-grep --pattern 'const uri = `${locusUrl}/${PARTICIPANT}/${$_}/${CONTROLS}`'
Length of output: 208
packages/@webex/plugin-meetings/test/unit/spec/member/util.js (1)
355-394: LGTM: Comprehensive test coverage for isBrb method.
The test suite:
- Covers all key scenarios (enabled, disabled, missing controls)
- Follows the established testing patterns
- Uses clear and descriptive test cases
- Properly structured test data
Let's verify test patterns match other similar utility tests:
packages/@webex/plugin-meetings/src/locus-info/index.ts (1)
1336-1347: LGTM! BRB event handling is well integrated.
The BRB state change detection and event emission follow the established patterns in the codebase. The implementation is clean and consistent with other similar event handling logic.
packages/@webex/plugin-meetings/test/unit/spec/locus-info/index.js (1)
741-808: LGTM! Comprehensive test coverage for BRB functionality.
The test cases thoroughly cover the BRB feature with both positive and negative scenarios:
- Verifies event emission when BRB state changes
- Confirms no event emission when state remains unchanged
- Validates proper handling of undefined state
docs/samples/browser-plugin-meetings/app.js (3)
3055-3058: LGTM! Clean helper function implementation.
Simple and focused helper function that improves code readability.
3338-3355: LGTM! Well-implemented BRB toggle with proper error handling.
The function:
- Correctly toggles BRB state
- Includes error handling and logging
- Appropriately mutes audio/video streams when stepping away
Line range hint 3739-3800: LGTM! UI implementation follows good practices.
The UI changes:
- Add BRB status column to the members table
- Show appropriate controls based on user role
- Display current BRB state for all participants
packages/@webex/plugin-meetings/src/meeting/index.ts (1)
Line range hint 1-2: LGTM!
The function logic is correct, and the implementation is accurate.
packages/@webex/media-helpers/package.json (1)
25-25: Verify dependency update's relationship to BRB feature
The version bump of @webex/internal-media-core to 2.12.0 appears to be part of a coordinated update across multiple packages. Since this package provides media-related functionality and the BRB feature involves managing media states (audio/video), please confirm:
- Is this version update necessary for the BRB feature?
- Does version 2.12.0 contain specific changes needed for the BRB functionality?
packages/@webex/plugin-meetings/package.json (1)
65-65: Verify version compatibility with dependent packages
Since this package is implementing the BRB feature and depends on both @webex/internal-media-core and @webex/media-helpers, please ensure:
- The version 2.12.0 of
@webex/internal-media-coreis compatible with all dependent packages - The workspace dependencies (marked with
workspace:*) are using compatible versions
✅ Verification successful
Let me generate additional scripts to verify the version compatibility.
Version compatibility check passed
The version compatibility check shows:
- All dependent packages are using
workspace:*for@webex/media-helpers, ensuring consistent versioning within the monorepo - The version
2.12.0of@webex/internal-media-coreis consistently used across all packages
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for version inconsistencies in package.json files
fd -e package.json | xargs rg "@webex/internal-media-core.*2\.\d+\.\d+"
Length of output: 66128
Script:
#!/bin/bash
# Check for version dependencies in package.json files
fd -e package.json | xargs rg "@webex/media-helpers.*workspace:|@webex/media-helpers.*\d+\.\d+\.\d+"
Length of output: 562
packages/calling/package.json (1)
40-40: Verify if this update is required for calling package
The version bump of @webex/internal-media-core to 2.12.0 is consistent with other packages, but since this is the calling package:
- Is this update necessary for the calling package, or is it just for consistency?
- Are there any potential impacts on calling functionality with this version update?
✅ Verification successful
Based on the search results, I can now generate a final response:
Update of @webex/internal-media-core is necessary for the calling package
The calling package heavily integrates with @webex/internal-media-core for its core media functionality, as evidenced by multiple direct dependencies and usage:
- Core media functionality in
CallingClient.ts - Media stream handling in
types.ts - Test configurations specifically excluding it from transform ignore patterns
- Multiple imports for media-related features
The version bump maintains consistency across the codebase and is required since the calling package directly uses the media core's functionality for its operations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any calling-related changes in the new version
gh api repos/webex/webex-js-sdk/compare/internal-media-core@2.11.3...internal-media-core@2.12.0 | jq -r '.files[].filename' | rg -i 'call'
Length of output: 221
Script:
#!/bin/bash
# Let's check how internal-media-core is used in the calling package
rg -A 5 "@webex/internal-media-core" packages/calling/
# Also check for any BRB (Background Replacement) related code in calling package
rg -i "background.?replacement|brb" packages/calling/
Length of output: 5214
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
packages/@webex/plugin-meetings/src/meeting/index.ts (2)
3365-3378: Consider verifying payload for completeness.
It would be beneficial to ensure the BRB payload contains all necessary properties before dispatching the event. This offers better stability and avoids potential undefined/null references in downstream handlers.
3582-3625: Validate concurrency and error propagation.
The new beRightBack() method correctly checks if the meeting is multistream and if a media connection exists. However, consider adding concurrency guards if multiple overlapping calls to beRightBack() are possible. This helps prevent race conditions or unwanted overrides in status.packages/@webex/plugin-meetings/src/constants.ts (3)
306-306: Adhere to naming conventions for new events.
The newly introduced MEETING_SELF_BRB_UPDATE event is consistent with existing naming but ensure it’s documented in your event schema references for clarity.
713-713: Document the new SELF_MEETING_BRB_CHANGED event.
Add developer documentation indicating how and when SELF_MEETING_BRB_CHANGED is emitted, and what data is contained in its payload, for easier maintainability.
Line range hint
9999-9999: Consider making BRB refresh threshold configurable.
MEETING_PERMISSION_TOKEN_REFRESH_THRESHOLD_IN_SEC and MEETING_PERMISSION_TOKEN_REFRESH_REASON appear to be static. Evaluate adding optional configuration parameters to allow flexible overrides across various deployments and compliance scenarios.docs/samples/browser-plugin-meetings/app.js (2)
Line range hint
3739-3806: Consider extracting BRB column logic for better readability.The BRB column integration looks good, but the conditional logic for rendering the BRB button/status could be extracted into a separate helper function to improve readability.
Consider this refactor:
-if (isUserSelf(member) && member.isInMeeting) { - td6.appendChild(createButton(member.isBrb ? 'Back to meeting' : 'Step away', toggleBrb, {id: 'brb-btn'})); -} else { - td6.appendChild(createLabel(member.id, member.isBrb ? 'YES' : 'NO')); -} +function createBrbCell(member) { + if (isUserSelf(member) && member.isInMeeting) { + return createButton(member.isBrb ? 'Back to meeting' : 'Step away', toggleBrb, {id: 'brb-btn'}); + } + return createLabel(member.id, member.isBrb ? 'YES' : 'NO'); +} + +td6.appendChild(createBrbCell(member));
3338-3355: Consider adding loading state during API call.The implementation looks good but could benefit from showing a loading state while the API call is in progress to prevent multiple clicks.
Consider this enhancement:
async function toggleBrb() { const meeting = getCurrentMeeting(); if (meeting) { const brbButton = document.getElementById('brb-btn'); + const originalText = brbButton.innerText; const isBrbEnabled = brbButton.innerText === 'Step away'; try { + brbButton.disabled = true; + brbButton.innerText = 'Please wait...'; const result = await meeting.beRightBack(isBrbEnabled); console.log(`meeting.beRightBack(${isBrbEnabled}): success. Result: ${result}`); } catch (error) { console.error(`meeting.beRightBack({${isBrbEnabled}): error: `, error); } finally { localMedia?.microphoneStream?.setUserMuted(isBrbEnabled); localMedia?.cameraStream?.setUserMuted(isBrbEnabled); + brbButton.disabled = false; + brbButton.innerText = originalText; } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
docs/samples/calling.min.jsis excluded by!**/*.min.jsdocs/samples/calling.min.js.mapis excluded by!**/*.map,!**/*.min.js.mapdocs/samples/meetings.min.jsis excluded by!**/*.min.jsdocs/samples/meetings.min.js.mapis excluded by!**/*.map,!**/*.min.js.mapdocs/samples/webex.min.jsis excluded by!**/*.min.jsdocs/samples/webex.min.js.mapis excluded by!**/*.map,!**/*.min.js.map
📒 Files selected for processing (7)
docs/samples/browser-plugin-meetings/app.js(12 hunks)packages/@webex/plugin-meetings/src/constants.ts(2 hunks)packages/@webex/plugin-meetings/src/locus-info/index.ts(1 hunks)packages/@webex/plugin-meetings/src/meeting/index.ts(2 hunks)packages/@webex/plugin-meetings/src/meeting/request.ts(2 hunks)packages/@webex/plugin-meetings/test/unit/spec/locus-info/index.js(1 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/@webex/plugin-meetings/src/meeting/request.ts
- packages/@webex/plugin-meetings/test/unit/spec/locus-info/index.js
- packages/@webex/plugin-meetings/src/locus-info/index.ts
🔇 Additional comments (4)
docs/samples/browser-plugin-meetings/app.js (1)
3055-3058: LGTM! Clear and focused utility function.
The function provides a clean way to check if a member is the current user by comparing IDs.
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (3)
35-36: LGTM! Added required imports for BRB functionality.
The new imports for HTTP_VERBS, PARTICIPANT, and CONTROLS constants are correctly added to support the BRB feature implementation.
3704-3776: LGTM! Well-structured test suite for BRB functionality.
The test suite thoroughly covers the BRB feature implementation:
- Tests both enabling and disabling BRB state
- Verifies correct handling for multistream and transcoded meetings
- Checks proper interaction with the meeting request API
- Validates the sendBrb method behavior
8835-8854: LGTM! Comprehensive test coverage for BRB state change events.
The test properly verifies:
- Event emission for BRB state changes
- Correct payload structure with enabled flag
- Event trigger with proper file and function context
antsukanova
requested review from
marcin-bazyl,
szotrabh,
brycetham and
k-wasniowski
packages/@webex/plugin-meetings/test/unit/spec/locus-info/index.js
Outdated
Show resolved
Hide resolved
|
LGTM |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/@webex/plugin-meetings/test/unit/spec/locus-info/index.js (2)
818-843: Consider Parametrizing the Test for Reusability
The sequence of "did not change" tests is correct. For maintainability, you might consider using a parameterized approach or loops for multiple unchanged states (e.g. brb = true and brb = false) to reduce code duplication in the future.
845-863: Test Undefined vs. Null BRB for Completeness
Currently, the test verifies behavior when brb is undefined. You may also consider adding a test case for brb = null to confirm consistent handling of other falsy or uninitialized states.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/@webex/plugin-meetings/test/unit/spec/locus-info/index.js(1 hunks)
🔇 Additional comments (1)
packages/@webex/plugin-meetings/test/unit/spec/locus-info/index.js (1)
796-816: Test Coverage Verified for BRB State Changes
These lines effectively confirm that SELF_MEETING_BRB_CHANGED is emitted when transitioning between true and false states. The logic is solid, and coverage appears thorough.
There was a problem hiding this comment.
I've got a few comments regarding the UT. Please check.
Additionally, please see this vidcast for a discrepancy I've observed between the SDK and desktop app behavior with regards to step-away.
Is there a reason we have created this PR from another branch in upstream rather than a fork?
| @@ -3094,7 +3099,7 @@ participantTable.addEventListener('click', (event) => { | |||
| } | |||
| const muteButton = document.getElementById('mute-participant-btn') | |||
| if (selectedParticipant.isAudioMuted) { | |||
| muteButton.innerText = meeting.selfId === selectedParticipant.id ? 'Unmute' : 'Request to unmute'; | |||
| muteButton.innerText = isUserSelf(selectedParticipant) ? 'Unmute' : 'Request to unmute'; | |||
There was a problem hiding this comment.
Were we observing any issues with this logic, leading to the change?
| * @param {string} options.selfId - The ID of the participant. | ||
| * @returns {Promise} | ||
| */ | ||
| sendBrb({enabled, locusUrl, deviceUrl, selfId}: BrbOptions) { |
There was a problem hiding this comment.
Would toggleBrb or setBrb be better alternatives for the function name?
| ROAP_OFFER_ANSWER_EXCHANGE_TIMEOUT, | ||
| } from '@webex/plugin-meetings/src/constants'; | ||
| ROAP_OFFER_ANSWER_EXCHANGE_TIMEOUT, HTTP_VERBS, PARTICIPANT, CONTROLS, | ||
| } from "@webex/plugin-meetings/src/constants"; |
There was a problem hiding this comment.
Any reason for this change?
| } from "@webex/plugin-meetings/src/constants"; | |
| } from '@webex/plugin-meetings/src/constants'; |
| @@ -4796,6 +4870,7 @@ describe('plugin-meetings', () => { | |||
| }); | |||
| }); | |||
|
|
|||
There was a problem hiding this comment.
This change isn't needed right?
| .then(() => { | ||
| this.sendSlotManager.setSourceStateOverride(MediaType.VideoMain, enabled ? 'away' : null); | ||
| }) | ||
| .catch((error) => { |
There was a problem hiding this comment.
I'm not able to find a test for this catch block. Could you point me to the test where this is being tested?
| assertBrbUnchanged(false); | ||
| }); | ||
|
|
||
| it('should not trigger SELF_MEETING_BRB_CHANGED when brb state is undefined', () => { |
There was a problem hiding this comment.
Can we also add a test when controls is undefined because that is also an optional object?
There was a problem hiding this comment.
I don't see a changeset for selfUtils test file. Could we add them as well?
COMPLETES # SPARK-559645
This pull request addresses
This PR introduces a new meeting API
sendBrbto allow clients to enable step away feature.by making the following changes
This PR introduces a couple of important updates:
beRightBack()Test app additions to test this feature
Change Type
The following scenarios were tested
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
Documentation
Tests