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 <[email protected]> Co-authored-by: Filip Nowakowski <[email protected]> Co-authored-by: evujici <[email protected]>
|
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) 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
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 <[email protected]>
![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/[email protected]@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?
There was a problem hiding this comment.
@sreenara This change is just adding a new function, isUserSelf, that utilises logic for checking meeting.selfId === member.id in a couple of places. It does not change the logic for this function.
I need the same check for brb and came up with a utility function for this. We can discuss whether the test app needs such a 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?
There was a problem hiding this comment.
I like the idea of renaming it to setBrb, makes sense thanks for it.
| 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'; |
There was a problem hiding this comment.
I'm not sure why linter didn't catch that single quotes were changed (I will double check this), I believe this is an issue with style changes js files. I assume just didn't catch that. Will update. Thanks
| @@ -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?
There was a problem hiding this comment.
@sreenara haven't found that case, will add it, thank you for pointing this.
| 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.
@sreenara will extend with new test case for updateSelf for all event that relies on check that controls is optional, because I haven't found this and it's not hard to do, so I will appreciate your feedback on that idea
There was a problem hiding this comment.
I don't see a changeset for selfUtils test file. Could we add them as well?
There was a problem hiding this comment.
@sreenara Thanks for catching that, I will update this file
|
@sreenara, thank you for the review. Regarding the branch, I have created the upstream version because we divided these changes between the WCME team and did them simultaneously. Once it was finished in one branch (this one brb-feature), I created a PR for the SDK team to review. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/@webex/plugin-meetings/test/unit/spec/locus-info/selfUtils.js (1)
63-70: Consider adding a negative test case.
Although this test thoroughly checks thatgetBrbis called and its returned attribute matchesself.controls.brb, it might be helpful to add a scenario wherebrbis absent or null in theselfobject to confirm robust behavior under edge conditions.packages/@webex/plugin-meetings/src/meeting/request.ts (1)
912-936: Add client-side validation or logging for partial request data.
While thesetBrbmethod is straightforward, consider validating each parameter before sending the request (e.g.,locusUrl,deviceUrl, etc.). This can help catch configuration issues earlier and provide clear error feedback.packages/@webex/plugin-meetings/src/meeting/index.ts (1)
3582-3625: Avoid repeating the error handling pattern multiple times.
Currently, the code throws in two checks before callingmeetingRequest.setBrb. Consider a helper function or shared utility to reduce repetition and unify the error-handling approach (such as building uniform error logs or analytics reporting). This also simplifies future expansions of BRB-related logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
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(2 hunks)packages/@webex/plugin-meetings/test/unit/spec/locus-info/selfConstant.js(1 hunks)packages/@webex/plugin-meetings/test/unit/spec/locus-info/selfUtils.js(2 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/@webex/plugin-meetings/test/unit/spec/locus-info/index.js
🔇 Additional comments (4)
packages/@webex/plugin-meetings/src/meeting/request.ts (1)
30-30: Good addition of BrbOptions import.
BringingBrbOptionsinto the same import statement allows for a consolidated view of related request types.packages/@webex/plugin-meetings/src/meeting/index.ts (1)
3365-3378: Ensure test coverage for the new BRB event.
This event handler forSELF_MEETING_BRB_CHANGEDis a welcome addition. Verify that corresponding unit tests validate its behavior, particularly around edge or unexpected payloads (e.g., missing fields, etc.).packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (2)
3704-3789: LGTM! Well-structured test suite for the BRB functionality.The test suite thoroughly covers:
- BRB functionality for both multistream and transcoded meetings
- Error handling
- Interaction with meeting request and media slots
- Different scenarios (enable/disable)
8847-8866: LGTM! Good test coverage for BRB state change event handling.The test verifies:
- Correct event triggering for both enable and disable cases
- Proper payload structure
- Event listener registration
packages/@webex/plugin-meetings/test/unit/spec/locus-info/selfConstant.js
Show resolved
Hide resolved
packages/@webex/plugin-meetings/test/unit/spec/locus-info/selfUtils.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/@webex/plugin-meetings/test/unit/spec/locus-info/selfUtils.js (1)
181-210: Consider adding more edge cases to the test suite.While the current test cases cover the basic scenarios, consider adding tests for:
- Both
previousandcurrenthaving undefinedbrbbrbobject with missingenabledpropertyenabledproperty with non-boolean valuesHere's an example of additional test cases:
it('should return false if brb is undefined in both objects', () => { const current = {}; const previous = {}; assert.isFalse(SelfUtils.brbChanged(previous, current)); }); it('should handle brb object with missing enabled property', () => { const current = {brb: {}}; const previous = {brb: {enabled: true}}; assert.isTrue(SelfUtils.brbChanged(previous, current)); });🧰 Tools
🪛 Biome (1.9.4)
[error] 181-181: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.(lint/suspicious/noFocusedTests)
📜 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/selfUtils.js(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/@webex/plugin-meetings/test/unit/spec/locus-info/selfUtils.js
[error] 181-181: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.
(lint/suspicious/noFocusedTests)
🔇 Additional comments (1)
packages/@webex/plugin-meetings/test/unit/spec/locus-info/selfUtils.js (1)
64-70: LGTM! Well-structured test case.The test case follows the established patterns and properly verifies both the function call and return value.
packages/@webex/plugin-meetings/test/unit/spec/locus-info/selfUtils.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/@webex/plugin-meetings/test/unit/spec/locus-info/selfUtils.js (1)
204-209: Test description doesn't match implementation.The test description states it checks for "brb in current is undefined", but the test actually verifies the case where the
brbproperty is completely missing from the current object. Consider updating the description to better reflect the test's purpose.Apply this diff to fix the description:
- it('should return false if brb in current is undefined', () => { + it('should return false if brb is missing in current object', () => {
📜 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/selfUtils.js(2 hunks)
🔇 Additional comments (2)
packages/@webex/plugin-meetings/test/unit/spec/locus-info/selfUtils.js (2)
64-70: LGTM! Well-structured test case.The test case follows the established pattern and properly verifies both the function call and return value.
181-181:⚠️ Potential issueRemove
.onlyfrom the test suite.The
.onlymodifier prevents other tests from running. This was likely left in during development.Apply this diff to fix the issue:
- describe.only('brbChanged', () => { + describe('brbChanged', () => {Likely invalid or redundant comment.
| * @returns {void} | ||
| */ | ||
| public setSourceStateOverride(mediaType: MediaType, state: StreamState | null) { | ||
| if (mediaType !== MediaType.VideoMain) { |
There was a problem hiding this comment.
just out of curiosity: does Homer support sending 'away' status for video slides? If it does, then maybe we should also allow that in the SDK?
When testing this feature in UCF, when I step away while sharing, UCF stops the screen share, so I guess we will do the same logic in the webex web app, but in theory some other app might want to have different logic and keep sharing but with sourceState overridden to "away" maybe?
There was a problem hiding this comment.
@marcin-bazyl from the JMP spec side and WCME it's possible, so your point is good to verify. Thanks. I will ask Homer team about their expectations regarding 'away' status for video slides. I believe it possible that other sdk clients would like to rely on overridden value for video slides.
There was a problem hiding this comment.
if Homer confirms that they support "away" for video slides, then I think it would be best to add a parameter to Meeting.beRightBack() so that the app calling that API can decide if they want the status to be applied to just main video, video slides or both
There was a problem hiding this comment.
@marcin-bazyl Yes, I believe it can be an optional additional param, good idea, thank you
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/samples/browser-plugin-meetings/app.js (1)
3338-3355: Solid implementation of the BRB functionality!The implementation is well-structured with proper async/await usage, error handling, and automatic media muting.
Consider these improvements:
- Add JSDoc documentation to describe the function's purpose and behavior
- Enhance error handling to provide user feedback, not just console errors
+/** + * Toggles the "Be Right Back" status for the current user. + * When enabled, automatically mutes audio and video. + * When disabled, returns the user to their previous state. + * @returns {Promise<void>} + * @throws {Error} If the BRB status cannot be updated + */ async function toggleBrb() { const meeting = getCurrentMeeting(); if (meeting) { const brbButton = document.getElementById('brb-btn'); const enableBrb = brbButton.innerText === 'Step away'; try { const result = await meeting.beRightBack(enableBrb); console.log(`meeting.beRightBack(${enableBrb}): success. Result: ${result}`); + // Update UI to show success + brbButton.innerText = enableBrb ? 'Back to meeting' : 'Step away'; } catch (error) { console.error(`meeting.beRightBack({${enableBrb}): error: `, error); + // Provide user feedback + brbButton.innerText = enableBrb ? 'Step away' : 'Back to meeting'; + alert('Failed to update status. Please try again.'); + return; } finally { localMedia?.microphoneStream?.setUserMuted(enableBrb); localMedia?.cameraStream?.setUserMuted(enableBrb); } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/samples/browser-plugin-meetings/app.js(12 hunks)
🔇 Additional comments (2)
docs/samples/browser-plugin-meetings/app.js (2)
3055-3058: Clean and focused implementation!The utility function has a clear purpose and follows good practices. The name
isMemberSelfaccurately describes its functionality, as suggested in previous reviews.
Line range hint
3739-3806: Well-structured table modifications!The new "AWAY" column is cleanly integrated into the existing table structure with appropriate UI elements:
- Toggle button for the current user
- Status display for other participants
- Consistent with the existing table layout and patterns
|
@sreenara I would appreciate your additional review. The only question regarding the test app logic for now you can omit. I would like to check it by myself and discuss it with the team next week, and then come back to you with a final suggestion. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/@webex/plugin-meetings/src/member/util.ts (2)
59-60: Consider adding parameter validation for consistency.Other methods in this file (e.g., isAudioMuted, isVideoMuted, isHandRaised) validate the participant parameter and throw ParameterError when undefined. Consider adding similar validation:
MemberUtil.isBrb = (participant: ParticipantWithBrb): boolean => { + if (!participant) { + throw new ParameterError('BRB status could not be processed, participant is undefined.'); + } return participant.controls.brb?.enabled || false; };
113-113: Use optional chaining for safer property access.Replace direct property access with optional chaining to handle potential undefined values more safely:
-MemberUtil.isUser = (participant) => participant && participant.type === _USER_; +MemberUtil.isUser = (participant) => participant?.type === _USER_; -MemberUtil.isGuest = (participant) => participant && participant.guest; +MemberUtil.isGuest = (participant) => participant?.guest || false;Also applies to: 119-119
🧰 Tools
🪛 Biome (1.9.4)
[error] 113-113: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/@webex/plugin-meetings/src/member/types.ts(1 hunks)packages/@webex/plugin-meetings/src/member/util.ts(19 hunks)packages/@webex/plugin-meetings/test/unit/spec/member/util.js(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/@webex/plugin-meetings/src/member/types.ts
- packages/@webex/plugin-meetings/test/unit/spec/member/util.js
🧰 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)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Packages
- GitHub Check: AWS Amplify Console Web Preview
🔇 Additional comments (3)
packages/@webex/plugin-meetings/src/member/util.ts (3)
7-7: LGTM! Clean type import addition.The ParticipantWithBrb type import follows the existing pattern and is appropriately placed with other participant-related types.
54-60: LGTM! Clean implementation of isBrb method.The implementation is well-documented, type-safe, and handles undefined cases gracefully using optional chaining.
47-50: LGTM! Improved type safety with TypeScript annotations.The addition of explicit return types and parameter types enhances code quality and maintainability.
Also applies to: 73-76, 80-83, 87-90, 94-97
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