feat(plugin-meetings): brbState #4110
Conversation
📝 WalkthroughWalkthroughThe changes introduce a new module for managing the "Be Right Back" (BRB) state in meeting sessions through the addition of the Possibly related PRs
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) 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/@webex/plugin-meetings/src/meeting/index.ts (1)
738-739: 💡 Verification agent🧩 Analysis chain
Verify BRB state cleanup.
The
clearMeetingDatamethod should clean up thebrbStatewhen the meeting ends to prevent memory leaks and ensure proper state management.
🏁 Script executed:
#!/bin/bash # Search for cleanup of other state objects to verify if brbState needs similar cleanup ast-grep --pattern 'clearMeetingData() { $$$ this.$_ = null; $$$ }'Length of output: 75
I'll run an additional verification to check the implementation details of the
clearMeetingDatamethod and locate any references tobrbStatein the file. This should help confirm whether the cleanup ofbrbStateis handled correctly. For that, please execute the following composite shell script:Once you verify the output of these commands, we'll be in a better position to decide if a cleanup is missing or if the behavior is already as expected.
🏁 Script executed:
#!/bin/bash echo "Searching for clearMeetingData implementation context..." rg -A 50 "clearMeetingData" packages/@webex/plugin-meetings/src/meeting/index.ts echo "Searching for usages of 'brbState' within the file..." rg "brbState" packages/@webex/plugin-meetings/src/meeting/index.tsLength of output: 4849
Action Required: Ensure proper cleanup for brbState in clearMeetingData method.
The current implementation of
clearMeetingDatacleans up several state properties (like audio, video, and transcription) but does not include any cleanup forbrbState. To prevent memory leaks and stale state issues, please add a cleanup step forbrbState—for example, setting it tonull(or applying any appropriate cleanup logic) at the end of the method. Review all related usages ofbrbStateto ensure that resetting it does not cause unintended side effects elsewhere in the code.
🧹 Nitpick comments (6)
packages/@webex/plugin-meetings/src/meeting/index.ts (1)
3620-3622: Public method implementation looks good but needs documentation.The
beRightBackmethod correctly delegates to thebrbState.enable()method, passing the required parameters. However, the method lacks proper JSDoc documentation.+/** + * Manages be right back status updates for the current participant. + * + * @param {boolean} enabled - Indicates whether the user enabled brb or not. + * @returns {Promise<void>} resolves when the brb status is updated or does nothing if not in a multistream meeting. + * @throws {Error} - Throws an error if the request fails. + * @public + */ public async beRightBack(enabled: boolean): Promise<void> { return this.brbState.enable(enabled, this.sendSlotManager); }packages/@webex/plugin-meetings/src/meeting/brbState.ts (4)
7-15: Add explicit return type annotation.Consider adding an explicit return type annotation to improve code clarity and type safety.
-export const createBrbState = (meeting: Meeting, enabled: boolean) => { +export const createBrbState = (meeting: Meeting, enabled: boolean): BrbState => {
23-31: Consider using readonly properties for state.To prevent accidental state mutations, consider making the state properties readonly where appropriate.
- state: { + private readonly state: { client: { - enabled: boolean; + enabled: boolean; }; server: { - enabled: boolean; + enabled: boolean; }; syncToServerInProgress: boolean; };
61-65: Add parameter validation.Consider adding validation for the sendSlotManager parameter to prevent potential null pointer issues.
public enable(enabled: boolean, sendSlotManager: SendSlotManager) { + if (!sendSlotManager) { + LoggerProxy.logger.error('Meeting:brbState#enable: sendSlotManager is required'); + return; + } this.state.client.enabled = enabled; this.applyClientStateToServer(sendSlotManager); }
120-159: Improve error handling consistency and simplify Promise chain.The error handling could be more consistent, and the Promise chain could be simplified using async/await.
private async sendLocalBrbStateToServer(sendSlotManager: SendSlotManager) { const {enabled} = this.state.client; if (!this.meeting.isMultistream) { - const errorMessage = - 'Meeting:brbState#sendLocalBrbStateToServer --> Not a multistream meeting'; - const error = new Error(errorMessage); - - LoggerProxy.logger.error(error); - - return Promise.reject(error); + throw new Error('Meeting:brbState#sendLocalBrbStateToServer --> Not a multistream meeting'); } if (!this.meeting.mediaProperties.webrtcMediaConnection) { - const errorMessage = - 'Meeting:brbState#sendLocalBrbStateToServer --> WebRTC media connection is not defined'; - const error = new Error(errorMessage); - - LoggerProxy.logger.error(error); - - return Promise.reject(error); + throw new Error('Meeting:brbState#sendLocalBrbStateToServer --> WebRTC media connection is not defined'); } // this logic should be applied only to multistream meetings - return this.meeting.meetingRequest - .setBrb({ + try { + await this.meeting.meetingRequest.setBrb({ enabled, locusUrl: this.meeting.locusUrl, deviceUrl: this.meeting.deviceUrl, selfId: this.meeting.selfId, - }) - .then(() => { - sendSlotManager.setSourceStateOverride(MediaType.VideoMain, enabled ? 'away' : null); - }) - .catch((error) => { - LoggerProxy.logger.error('Meeting:brbState#sendLocalBrbStateToServer --> Error ', error); - - return Promise.reject(error); }); + sendSlotManager.setSourceStateOverride(MediaType.VideoMain, enabled ? 'away' : null); + } catch (error) { + LoggerProxy.logger.error('Meeting:brbState#sendLocalBrbStateToServer --> Error ', error); + throw error; + } }packages/@webex/plugin-meetings/test/unit/spec/meeting/brbState.ts (1)
1-112: Add test cases for edge scenarios.While the current test coverage is good, consider adding the following test cases:
- Error handling when setBrb request fails
- Race condition scenarios with multiple enable/disable calls
- Edge cases around the syncToServerInProgress flag
- Validation of error messages
Example test case for error handling:
it('handles setBrb request failure', async () => { meeting.meetingRequest.setBrb.rejects(new Error('Network error')); brbState.enable(true, meeting.sendSlotManager); await testUtils.flushPromises(); assert.isFalse(brbState.state.syncToServerInProgress); assert.isTrue(meeting.meetingRequest.setBrb.calledOnce); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/@webex/plugin-meetings/src/meeting/brbState.ts(1 hunks)packages/@webex/plugin-meetings/src/meeting/index.ts(5 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/brbState.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Determine Changed Packages
🔇 Additional comments (8)
packages/@webex/plugin-meetings/src/meeting/index.ts (7)
164-164: Import statement looks good.The import of
BrbStateandcreateBrbStatefrom./brbStateis properly defined.
650-650: Property declaration follows class field conventions.The
brbStateproperty is correctly declared as an instance field of typeBrbState.
3394-3407: Event handling for BRB state changes is properly implemented.The event handler correctly:
- Delegates server BRB updates to the
brbStateinstance- Triggers appropriate events to notify clients about BRB state changes
- Follows the established pattern for event handling in the codebase
7387-7387: BRB state initialization follows established patterns.The initialization of
brbStateusingcreateBrbStatefollows the same pattern as other state objects likemuteState, passing the required parameters.
3394-3407:✅ Verification successful
Verify event trigger consistency.
The event trigger for BRB state changes follows the established pattern but we should verify if any additional events need to be triggered for analytics or metrics.
🏁 Script executed:
#!/bin/bash # Search for similar event patterns to ensure consistency rg "EVENT_TRIGGERS\.MEETING_SELF.*_UPDATE" packages/@webex/plugin-meetings/src/meeting/Length of output: 301
Event trigger consistency confirmed.
The BRB state trigger follows the established pattern (as withMEETING_SELF_PHONE_AUDIO_UPDATE) and no additional analytics or metrics events are required.
8738-8739:❓ Verification inconclusive
Verify BRB state cleanup in leave scenarios.
The
clearMeetingDatamethod is called during leave/end scenarios, but we should verify ifbrbStateneeds any special cleanup like unsubscribing from events.
🏁 Script executed:
#!/bin/bash # Search for cleanup patterns in other state objects rg -A 5 "\.off\(" packages/@webex/plugin-meetings/src/meeting/brbState.tsLength of output: 75
Action: Confirm BRB state event listener cleanup in leave scenarios
The
clearMeetingDatamethod takes care of resetting the audio and video states, but it’s unclear whether any event listeners registered by thebrbStaterequire explicit cleanup (such as unsubscribing via.off). The initial grep for.off(inpackages/@webex/plugin-meetings/src/meeting/brbState.tsreturned no results, which may indicate that either no cleanup is needed or that event subscriptions use a different pattern.Before moving forward, please manually verify whether:
- The
brbStatemodule registers event listeners (e.g., using.on() that could accumulate if not properly cleaned up.- If such listeners exist, appropriate cleanup (similar to that for audio and video) is performed in leave scenarios (possibly in
clearMeetingDataor another cleanup method).Once you confirm that either no event subscriptions exist or that the necessary cleanup is handled elsewhere, let us know if additional changes are required.
3620-3622:❓ Verification inconclusive
Verify multistream support for BRB functionality.
The
beRightBackmethod usessendSlotManagerwhich is specific to multistream meetings. We should verify if BRB functionality should be supported in non-multistream meetings as well.
🏁 Script executed:
#!/bin/bash # Search for other features that require multistream support ast-grep --pattern 'if (!this.isMultistream) { $$$ }'Length of output: 66934
Action Required: Confirm BRB Behavior for Non-Multistream Meetings
The
beRightBackmethod callsthis.brbState.enable(enabled, this.sendSlotManager), andsendSlotManageris used elsewhere exclusively for multistream meetings (evidenced by various checks forif (!this.isMultistream) { ... }). Please verify if the BRB functionality is meant to function only when multistream is enabled. If BRB should also be supported in non-multistream meetings, consider whether an additional condition or alternate handling is required to avoid unintended side effects in those scenarios.packages/@webex/plugin-meetings/src/meeting/brbState.ts (1)
180-202: LGTM! Clean and well-implemented state check methods.The implementation is clear, concise, and follows good practices with proper return type annotations.
| private applyClientStateToServer(sendSlotManager: SendSlotManager) { | ||
| if (this.state.syncToServerInProgress) { | ||
| LoggerProxy.logger.info( | ||
| `Meeting:brbState#applyClientStateToServer: request to server in progress, we need to wait for it to complete` | ||
| ); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| const remoteBrbRequiresSync = this.state.client.enabled !== this.state.server.enabled; | ||
|
|
||
| LoggerProxy.logger.info( | ||
| `Meeting:brbState#applyClientStateToServer: remoteBrbRequiresSync: ${remoteBrbRequiresSync}` | ||
| ); | ||
|
|
||
| if (!remoteBrbRequiresSync) { | ||
| LoggerProxy.logger.info( | ||
| `Meeting:brbState#applyClientStateToServer: client state already matching server state, nothing to do` | ||
| ); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| this.state.syncToServerInProgress = true; | ||
|
|
||
| this.sendLocalBrbStateToServer(sendSlotManager) | ||
| .then(() => { | ||
| this.state.syncToServerInProgress = false; | ||
| LoggerProxy.logger.info( | ||
| `Meeting:brbState#applyClientStateToServer: sync with server completed` | ||
| ); | ||
|
|
||
| // need to check if a new sync is required, because this.state.client may have changed while we were doing the current sync | ||
| this.applyClientStateToServer(sendSlotManager); | ||
| }) | ||
| .catch((e) => { | ||
| this.state.syncToServerInProgress = false; | ||
| LoggerProxy.logger.warn(`Meeting:brbState#applyClientStateToServer: error: ${e}`); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and prevent potential stack overflow.
The recursive call to applyClientStateToServer could potentially lead to a stack overflow if the state keeps changing rapidly. Consider using a queue or debounce mechanism instead.
private applyClientStateToServer(sendSlotManager: SendSlotManager) {
if (this.state.syncToServerInProgress) {
LoggerProxy.logger.info(
`Meeting:brbState#applyClientStateToServer: request to server in progress, we need to wait for it to complete`
);
return;
}
const remoteBrbRequiresSync = this.state.client.enabled !== this.state.server.enabled;
LoggerProxy.logger.info(
`Meeting:brbState#applyClientStateToServer: remoteBrbRequiresSync: ${remoteBrbRequiresSync}`
);
if (!remoteBrbRequiresSync) {
LoggerProxy.logger.info(
`Meeting:brbState#applyClientStateToServer: client state already matching server state, nothing to do`
);
return;
}
this.state.syncToServerInProgress = true;
this.sendLocalBrbStateToServer(sendSlotManager)
.then(() => {
this.state.syncToServerInProgress = false;
LoggerProxy.logger.info(
`Meeting:brbState#applyClientStateToServer: sync with server completed`
);
- // need to check if a new sync is required, because this.state.client may have changed while we were doing the current sync
- this.applyClientStateToServer(sendSlotManager);
+ // Schedule the next sync check using setTimeout to prevent stack overflow
+ setTimeout(() => {
+ if (!this.state.syncToServerInProgress) {
+ this.applyClientStateToServer(sendSlotManager);
+ }
+ }, 0);
})
.catch((e) => {
this.state.syncToServerInProgress = false;
- LoggerProxy.logger.warn(`Meeting:brbState#applyClientStateToServer: error: ${e}`);
+ LoggerProxy.logger.error(`Meeting:brbState#applyClientStateToServer: error: ${e.message}`);
+ // Consider emitting an error event or notifying the user
});
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private applyClientStateToServer(sendSlotManager: SendSlotManager) { | |
| if (this.state.syncToServerInProgress) { | |
| LoggerProxy.logger.info( | |
| `Meeting:brbState#applyClientStateToServer: request to server in progress, we need to wait for it to complete` | |
| ); | |
| return; | |
| } | |
| const remoteBrbRequiresSync = this.state.client.enabled !== this.state.server.enabled; | |
| LoggerProxy.logger.info( | |
| `Meeting:brbState#applyClientStateToServer: remoteBrbRequiresSync: ${remoteBrbRequiresSync}` | |
| ); | |
| if (!remoteBrbRequiresSync) { | |
| LoggerProxy.logger.info( | |
| `Meeting:brbState#applyClientStateToServer: client state already matching server state, nothing to do` | |
| ); | |
| return; | |
| } | |
| this.state.syncToServerInProgress = true; | |
| this.sendLocalBrbStateToServer(sendSlotManager) | |
| .then(() => { | |
| this.state.syncToServerInProgress = false; | |
| LoggerProxy.logger.info( | |
| `Meeting:brbState#applyClientStateToServer: sync with server completed` | |
| ); | |
| // need to check if a new sync is required, because this.state.client may have changed while we were doing the current sync | |
| this.applyClientStateToServer(sendSlotManager); | |
| }) | |
| .catch((e) => { | |
| this.state.syncToServerInProgress = false; | |
| LoggerProxy.logger.warn(`Meeting:brbState#applyClientStateToServer: error: ${e}`); | |
| }); | |
| } | |
| private applyClientStateToServer(sendSlotManager: SendSlotManager) { | |
| if (this.state.syncToServerInProgress) { | |
| LoggerProxy.logger.info( | |
| `Meeting:brbState#applyClientStateToServer: request to server in progress, we need to wait for it to complete` | |
| ); | |
| return; | |
| } | |
| const remoteBrbRequiresSync = this.state.client.enabled !== this.state.server.enabled; | |
| LoggerProxy.logger.info( | |
| `Meeting:brbState#applyClientStateToServer: remoteBrbRequiresSync: ${remoteBrbRequiresSync}` | |
| ); | |
| if (!remoteBrbRequiresSync) { | |
| LoggerProxy.logger.info( | |
| `Meeting:brbState#applyClientStateToServer: client state already matching server state, nothing to do` | |
| ); | |
| return; | |
| } | |
| this.state.syncToServerInProgress = true; | |
| this.sendLocalBrbStateToServer(sendSlotManager) | |
| .then(() => { | |
| this.state.syncToServerInProgress = false; | |
| LoggerProxy.logger.info( | |
| `Meeting:brbState#applyClientStateToServer: sync with server completed` | |
| ); | |
| // Schedule the next sync check using setTimeout to prevent stack overflow | |
| setTimeout(() => { | |
| if (!this.state.syncToServerInProgress) { | |
| this.applyClientStateToServer(sendSlotManager); | |
| } | |
| }, 0); | |
| }) | |
| .catch((e) => { | |
| this.state.syncToServerInProgress = false; | |
| LoggerProxy.logger.error(`Meeting:brbState#applyClientStateToServer: error: ${e.message}`); | |
| // Consider emitting an error event or notifying the user | |
| }); | |
| } |
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (2)
3824-3825: Consider expanding test coverage for BrbState initialization.While the basic setup is good, consider adding tests to verify:
- The
createBrbStateparameters (especially thefalseparameter)- Error scenarios during BrbState initialization
meeting.brbState = createBrbState(meeting, false); meeting.brbState.enable = sinon.stub().resolves(); +// Add test cases for initialization +it('should initialize brbState with correct parameters', () => { + assert.strictEqual(meeting.brbState.initialState, false); +}); + +it('should handle errors during brbState initialization', () => { + const error = new Error('initialization failed'); + createBrbState.throws(error); + assert.throws(() => meeting.brbState = createBrbState(meeting, false), error); +});
3847-3860: Enhance test assertions for better coverage.While the basic assertions are good, consider enhancing them to:
- Verify the parameters passed to
enable- Check the actual state after enable/disable operations
-assert.calledOnce(meeting.brbState.enable); +assert.calledOnceWithExactly(meeting.brbState.enable, true); +assert.strictEqual(meeting.brbState.isEnabled, true); it('should disable #beRightBack and return a promise', async () => { const brbResult = meeting.beRightBack(false); await brbResult; assert.exists(brbResult.then); - assert.calledOnce(meeting.brbState.enable); + assert.calledOnceWithExactly(meeting.brbState.enable, false); + assert.strictEqual(meeting.brbState.isEnabled, false); })packages/@webex/plugin-meetings/src/meeting/brbState.ts (1)
73-80: Consider returning the promise for consistent chain handling.
Currently, if a request is already in progress, this branch returnsPromise.resolve()immediately. You might want to store and return the ongoing Promise to maintain a single chain of resolution or rejection, ensuring any external callers can properly await and handle results or errors.if (this.state.syncToServerInProgress) { LoggerProxy.logger.info( `Meeting:brbState#applyClientStateToServer: request to server in progress, we need to wait for it to complete` ); - return Promise.resolve(); + return this.currentSyncPromise || Promise.resolve(); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/@webex/plugin-meetings/src/meeting/brbState.ts(1 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Packages
- GitHub Check: AWS Amplify Console Web Preview
🔇 Additional comments (4)
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (1)
117-117: LGTM! Import statement correctly added.The import for
createBrbStateis properly placed and aligns with the new BRB state management feature.packages/@webex/plugin-meetings/src/meeting/brbState.ts (3)
1-6: Nice setup and imports.
The initial structure and descriptive imports establish a solid foundation for BRB state management without introducing extraneous dependencies.
105-107: Prevent potential stack overflow.
This direct recursive call toapplyClientStateToServerreintroduces the risk of unbounded recursion if the state changes repeatedly. Previously, asetTimeoutscheduling approach was suggested to break the recursion chain, which helps avoid stack overflows during rapid toggles.- this.applyClientStateToServer(sendSlotManager); + // Schedule the next sync check using a short delay + setTimeout(() => { + if (!this.state.syncToServerInProgress) { + this.applyClientStateToServer(sendSlotManager); + } + }, 0);
120-142: Verify partial fallback or alternative flows.
If the client is not in a multistream meeting or lacks a WebRTC media connection, the current logic rejects the operation. Confirm whether a fallback or partial enablement is ever desired (e.g., local-only BRB or a graceful no-op), especially if toggling BRB in non-multistream/partial connection scenarios could still make sense for specific use cases.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (1)
11890-11891: 🛠️ Refactor suggestionSimplify complex test logic
The offset calculation logic in the payloadTestHelper function is overly complex and hard to understand:
if (share.length + 1 > offset) { offset = (offset + share.length + 1) / 2; } else if (share.length + 1 < offset) { offset = share.length + 1 + 0.5; }Consider refactoring this to be more straightforward and easier to maintain.
🧹 Nitpick comments (15)
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (15)
116-117: Add missing import for createBrbStateThe test file is importing
createBrbStatefrom@webex/plugin-meetings/src/meeting/brbStatebut this import is missing from the imports section.+import { createBrbState } from '@webex/plugin-meetings/src/meeting/brbState';
13824-13825: Fix test description to be more descriptiveThe test description "should toggle the reactions with the right data and return a promise" is too vague. Consider making it more specific about what is being tested.
-'should toggle the reactions with the right data and return a promise' +'should enable reactions by sending toggle request with enable=true when reactions are disabled'
13867-13868: Fix test description to be more descriptiveSimilar to above, the test description "should resolve immediately if already enabled" could be more descriptive.
-'should resolve immediately if already enabled' +'should resolve without sending toggle request when trying to enable already enabled reactions'
13879-13880: Fix test description to be more descriptiveSimilar to above, the test description "should resolve immediately if already disabled" could be more descriptive.
-'should resolve immediately if already disabled' +'should resolve without sending toggle request when trying to disable already disabled reactions'
13891-13892: Fix test description to be more descriptiveSimilar to above, the test description "should toggle reactions on if controls is undefined and enable = true" could be more descriptive.
-'should toggle reactions on if controls is undefined and enable = true' +'should enable reactions by sending toggle request when controls is undefined and enable=true'
12989-13023: Add missing test cases for buildLeaveFetchRequestOptionsThe test coverage for buildLeaveFetchRequestOptions is minimal. Consider adding test cases for:
- When resourceId is not provided
- When reason is provided
- When both resourceId and reason are provided
- Error cases
13024-13152: Add missing test cases for getPermissionTokenExpiryInfoThe test coverage for getPermissionTokenExpiryInfo could be improved by adding test cases for:
- Invalid exp value (non-numeric string)
- Edge cases around time differences
- Handling of null/undefined permissionTokenReceivedLocalTime
- Handling of null/undefined permissionTokenPayload
13206-13246: Add missing test cases for roapMessageReceivedThe test coverage for roapMessageReceived could be improved by adding test cases for:
- Different message types (OFFER, ERROR, etc.)
- Invalid message format
- Missing sdp field
- Error handling when getMediaServer fails
- Edge cases around multistream support
13154-13204: Add missing test cases for checkAndRefreshPermissionTokenThe test coverage for checkAndRefreshPermissionToken could be improved by adding test cases for:
- Error handling when refreshPermissionToken fails
- Edge cases around timing thresholds
- Different ttl values
- Invalid input parameters
11902-11903: Fix test description to be more descriptiveThe test description "Scenario #1: Whiteboard sharing as a webinar attendee" could be more descriptive about what is being tested.
-'Scenario #1: Whiteboard sharing as a webinar attendee' +'Scenario #1: Verify remote share events are triggered when whiteboard is shared in webinar as attendee'
12031-12032: Fix test description to be more descriptiveThe test description "Scenario #1: you share both whiteboards" could be more descriptive about what is being tested.
-'Scenario #1: you share both whiteboards' +'Scenario #1: Verify share events when switching between two whiteboards shared by local user'
12032-12581: Improve test organizationThe test file has a lot of nested describe blocks and repeated test setup. Consider:
- Moving common test setup to beforeEach blocks
- Extracting helper functions to reduce code duplication
- Organizing tests into smaller, focused files
- Using shared test fixtures
🧰 Tools
🪛 Biome (1.9.4)
[error] 12032-12231: Excessive
describe()nesting detected.Excessive nesting of describe() calls can hinder test readability.
Consider refactoring and reduce the level of nested describe to improve code clarity.(lint/complexity/noExcessiveNestedTestSuites)
[error] 12233-12363: Excessive
describe()nesting detected.Excessive nesting of describe() calls can hinder test readability.
Consider refactoring and reduce the level of nested describe to improve code clarity.(lint/complexity/noExcessiveNestedTestSuites)
[error] 12364-12551: Excessive
describe()nesting detected.Excessive nesting of describe() calls can hinder test readability.
Consider refactoring and reduce the level of nested describe to improve code clarity.(lint/complexity/noExcessiveNestedTestSuites)
[error] 12553-12580: Excessive
describe()nesting detected.Excessive nesting of describe() calls can hinder test readability.
Consider refactoring and reduce the level of nested describe to improve code clarity.(lint/complexity/noExcessiveNestedTestSuites)
12582-12747: Add missing test cases for keepAlive functionalityThe test coverage for keepAlive functionality could be improved by adding test cases for:
- Race conditions in startKeepAlive/stopKeepAlive
- Error handling for network failures
- Edge cases around timing
- Retry logic
- State transitions
12748-12905: Add missing test cases for reactions functionalityThe test coverage for reactions functionality could be improved by adding test cases for:
- Rate limiting
- Error handling for network failures
- Validation of reaction types
- State transitions
- Event handling
12906-12986: Add missing test cases for video mute functionalityThe test coverage for video mute functionality could be improved by adding test cases for:
- Race conditions
- State transitions
- Error handling
- Event propagation
- Edge cases
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Packages
- GitHub Check: AWS Amplify Console Web Preview
packages/@webex/plugin-meetings/test/unit/spec/meeting/brbState.ts
Outdated
Show resolved
Hide resolved
packages/@webex/plugin-meetings/test/unit/spec/meeting/brbState.ts
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/meeting/brbState.ts (1)
81-87: Minor grammar issue: “won’t sent” → “won’t send.”
Please update the test description to fix the verb tense:- it("won't sent request twice when in progress", async () => { + it("won't send request twice when in progress", async () => {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/@webex/plugin-meetings/src/meeting/brbState.ts(1 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/brbState.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Packages
- GitHub Check: AWS Amplify Console Web Preview
🔇 Additional comments (20)
packages/@webex/plugin-meetings/test/unit/spec/meeting/brbState.ts (13)
1-3: Looks good.
No issues at a glance with these import statements or test helper setup.
4-6: All set here.
Test utilities import is straightforward and consistent with existing patterns.
7-10: No problems with describe or variable declarations.
Initialization approach looks tidy.
11-30: Clear and well-structured test setup.
The mockmeetingobject is adequately populated with necessary stubs.
32-45: Instantiation logic test looks solid.
It tests both enabled=true and enabled=false cases properly.
47-53: Well-defined “enable” behavior test.
Ensures local state and server update calls remain in sync.
55-61: Disable behavior is tested properly.
Covers the scenario where BRB is turned off and validated.
63-70: Grammar/style note: “won’t” vs “will not.”
This was suggested in a past review. If you prefer to keep the contraction, you can. Otherwise, consider “it('will not be able to send ...')” for consistency.
72-79: Generally consistent. No major issues.
Similar test to line 63-70 verifying webrtc connection constraints.
89-95: Sync behavior test is comprehensive.
Verifies minimal server updates and respects state changes.
97-103: Source state override test is robust.
Confirms the correct sendSlotManager call.
105-110: Handling server updates is properly covered.
Checks that local state changes when the server toggles the BRB state.
112-112: No further issues.
End of file looks tidy.packages/@webex/plugin-meetings/src/meeting/brbState.ts (7)
1-5: Imports look appropriate.
References to logger, media core, and meeting type definitions are clear.
6-15: Factory function is straightforward.
Logging message is helpful for debugging.
17-50: Class structure and constructor are clear.
Storing separate client/server states is a clean approach.
52-63: Enable method logic is concise.
Successfully toggles local state and triggers server sync.
65-110: Recursive call may risk repeated sync triggers.
A previous review suggested preventing stack overflow or repeated calls if toggled rapidly. Since this was already raised, tagging as repeated feedback.
112-139: Local brb state server updates are well-handled.
You correctly reject when conditions aren’t met (non-multistream or missing media connection).
159-181: Server updates & combined state check are understandable.
isEnabled()returning true if either client or server is true matches the intended design.
packages/@webex/plugin-meetings/test/unit/spec/meeting/brbState.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/@webex/plugin-meetings/src/meeting/brbState.ts (1)
102-104: 🛠️ Refactor suggestionImprove error handling and prevent potential stack overflow.
The recursive call to
applyClientStateToServercould potentially lead to a stack overflow if the state keeps changing rapidly. Consider usingsetTimeoutto break the call stack.// need to check if a new sync is required, because this.state.client may have changed while we were doing the current sync - this.applyClientStateToServer(sendSlotManager); + // Schedule the next sync check using setTimeout to prevent stack overflow + setTimeout(() => { + if (!this.state.syncToServerInProgress) { + this.applyClientStateToServer(sendSlotManager); + } + }, 0);
🧹 Nitpick comments (3)
packages/@webex/plugin-meetings/src/meeting/brbState.ts (3)
105-108: Improve error logging.Consider enhancing the error logging with more details and using
errorlevel instead ofwarnfor consistency with other error handling in this class..catch((e) => { this.state.syncToServerInProgress = false; - LoggerProxy.logger.warn(`Meeting:brbState#applyClientStateToServer: error: ${e}`); + LoggerProxy.logger.error(`Meeting:brbState#applyClientStateToServer: error: ${e.message}`); + // Consider emitting an error event or notifying the user });
120-138: Consider refactoring error handling for better code maintainability.Both error handling blocks follow the same pattern. Consider extracting this into a helper method to reduce code duplication.
private async sendLocalBrbStateToServer(sendSlotManager: SendSlotManager) { const {enabled} = this.state.client; + const handleError = (message: string) => { + const errorMessage = `Meeting:brbState#sendLocalBrbStateToServer --> ${message}`; + const error = new Error(errorMessage); + LoggerProxy.logger.error(error); + return Promise.reject(error); + }; + if (!this.meeting.isMultistream) { - const errorMessage = - 'Meeting:brbState#sendLocalBrbStateToServer --> Not a multistream meeting'; - const error = new Error(errorMessage); - - LoggerProxy.logger.error(error); - - return Promise.reject(error); + return handleError('Not a multistream meeting'); } if (!this.meeting.mediaProperties.webrtcMediaConnection) { - const errorMessage = - 'Meeting:brbState#sendLocalBrbStateToServer --> WebRTC media connection is not defined'; - const error = new Error(errorMessage); - - LoggerProxy.logger.error(error); - - return Promise.reject(error); + return handleError('WebRTC media connection is not defined'); }
19-170: Add an explicitisEnabledmethod.Consider adding an
isEnabledmethod to provide a consistent way to check the BRB state.// Add this method to the BrbState class + /** + * Checks if the BRB state is currently enabled + * + * @returns {boolean} - true if BRB is enabled, false otherwise + */ + public isEnabled(): boolean { + return this.state.client.enabled; + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/@webex/plugin-meetings/src/meeting/brbState.ts(1 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/brbState.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Packages
- GitHub Check: AWS Amplify Console Web Preview
🔇 Additional comments (7)
packages/@webex/plugin-meetings/test/unit/spec/meeting/brbState.ts (3)
65-65: Consider updating the test name for consistency.The test name "won't be able to send..." doesn't follow consistent naming patterns. Consider using "does not send..." instead.
- it("won't be able to send local brb state to server if it is not a multistream meeting", async () => { + it("does not send local brb state to server if it is not a multistream meeting", async () => {
74-74: Consider updating the test name for consistency.Similar to the previous test case, consider using consistent naming patterns.
- it("won't be able to send local brb state to server if webrtc media connection is not defined", async () => { + it("does not send local brb state to server if webrtc media connection is not defined", async () => {
83-83: Consider updating the test name for consistency.Maintain consistency in test naming conventions.
- it("won't send request twice when in progress", async () => { + it("does not send request twice when in progress", async () => {packages/@webex/plugin-meetings/src/meeting/brbState.ts (4)
16-18: Improve class documentation.The class documentation could be more descriptive to clarify the purpose and behavior of the BRB state management.
-/** The purpose of this class is to manage the local and remote brb state - * and make sure that the server state always matches the last requested state by the client. - */ +/** The purpose of this class is to manage the local and remote brb state and make sure that the server state always matches the last requested state by the client. + * It prevents sending excessive requests to the server by tracking the sync status and ensuring that only one request is in progress at a time. + * This helps mitigate potential server-side issues from processing too many rapid requests. + */
164-169: Rename parameter for consistency.Consider renaming the parameter from
enabledtobrbEnabledto match other naming conventions in the codebase.- public handleServerBrbUpdate(enabled?: boolean) { + public handleServerBrbUpdate(brbEnabled?: boolean) { LoggerProxy.logger.info( - `Meeting:brbState#handleServerBrbUpdate: updating server brb to (${enabled})` + `Meeting:brbState#handleServerBrbUpdate: updating server brb to (${brbEnabled})` ); - this.state.server.enabled = !!enabled; + this.state.server.enabled = !!brbEnabled; }
79-83: Clarify the terminology around "remote" BRB state.The variable name
remoteBrbRequiresSyncmight be confusing as BRB doesn't have a concept of remote control like mute does. Consider renaming for clarity.- const remoteBrbRequiresSync = this.state.client.enabled !== this.state.server.enabled; + const serverSyncRequired = this.state.client.enabled !== this.state.server.enabled; LoggerProxy.logger.info( - `Meeting:brbState#applyClientStateToServer: remoteBrbRequiresSync: ${remoteBrbRequiresSync}` + `Meeting:brbState#applyClientStateToServer: serverSyncRequired: ${serverSyncRequired}` ); - if (!remoteBrbRequiresSync) { + if (!serverSyncRequired) {
149-149:❓ Verification inconclusive
Ensure the correct pattern for SendSlotManager.
Make sure the
setSourceStateOverridecall follows the correct pattern expected by the SendSlotManager class. Verify that 'away' is a valid state.
🏁 Script executed:
#!/bin/bash # Check how setSourceStateOverride is used in the codebase ast-grep --pattern $'setSourceStateOverride($_, $_)'Length of output: 53
Action Required: Manual Verification of setSourceStateOverride Usage
The current call in
packages/@webex/plugin-meetings/src/meeting/brbState.tsat line 149sendSlotManager.setSourceStateOverride(MediaType.VideoMain, enabled ? 'away' : null);needs to be manually verified to ensure it adheres to the expected pattern defined in the SendSlotManager class. In particular, please confirm that:
- The method signature of
setSourceStateOverridein SendSlotManager accepts the provided parameters.- The string
'away'is a valid state according to the SendSlotManager implementation.Additional searches (e.g., using AST and regex searches) have not produced conclusive evidence about the usage pattern. It is recommended to review the SendSlotManager definition and its valid states to confirm this usage.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/@webex/plugin-meetings/test/unit/spec/meeting/brbState.ts (1)
74-81: 🛠️ Refactor suggestionTest missing for 'false' webrtcMediaConnection case.
The test only verifies behavior when
webrtcMediaConnectionisundefined, but not when it's explicitlyfalse. As noted in previous reviews, we should test both scenarios.Consider adding an additional test case:
it('does not send local brb state to server if webrtc media connection is false', async () => { meeting.mediaProperties.webrtcMediaConnection = false; brbState.enable(true, meeting.sendSlotManager); brbState.handleServerBrbUpdate(true); await testUtils.flushPromises(); assert.isTrue(meeting.meetingRequest.setBrb.notCalled); });
🧹 Nitpick comments (1)
packages/@webex/plugin-meetings/test/unit/spec/meeting/brbState.ts (1)
18-19: Set webrtcMediaConnection more consistently for testing.The property is set to
truein the setup but tests change it toundefined. For better test isolation, consider initializing it differently or creating a helper to reset test state.One approach could be:
meeting = { isMultistream: true, locusUrl: 'locus url', deviceUrl: 'device url', selfId: 'self id', mediaProperties: { - webrtcMediaConnection: true, + webrtcMediaConnection: undefined, }, // ... }; // Then in tests that need it: // meeting.mediaProperties.webrtcMediaConnection = true;Or create a helper to reset state between tests if needed.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/@webex/plugin-meetings/test/unit/spec/meeting/brbState.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Packages
- GitHub Check: AWS Amplify Console Web Preview
🔇 Additional comments (2)
packages/@webex/plugin-meetings/test/unit/spec/meeting/brbState.ts (2)
65-72: Standardize test naming conventions across the test suite.The test case naming here doesn't follow the consistent pattern suggested in previous review comments. Consider following a single naming convention for all test cases.
- it('cannot send local brb state to server if it is not a multistream meeting', async () => { + it('does not send local brb state to server if it is not a multistream meeting', async () => {
1-115: Tests look comprehensive for the BrbState implementation.The test suite provides good coverage of the BrbState functionality, testing initialization, state changes, server synchronization, and error conditions. The implementation follows best practices for asynchronous testing.
packages/@webex/plugin-meetings/test/unit/spec/meeting/brbState.ts
Outdated
Show resolved
Hide resolved
packages/@webex/plugin-meetings/test/unit/spec/meeting/brbState.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/@webex/plugin-meetings/test/unit/spec/meeting/brbState.ts (1)
74-81: 🛠️ Refactor suggestionAdd test for the case when webrtcMediaConnection is false
The current test only verifies behavior when
webrtcMediaConnectionisundefined, but not when it's explicitlyfalse. This was raised in previous reviews - we should add a dedicated test for thefalsescenario.it('does not send local brb state to server if webrtc media connection is not defined', async () => { meeting.mediaProperties.webrtcMediaConnection = undefined; brbState.enable(true, meeting.sendSlotManager); brbState.handleServerBrbUpdate(true); await testUtils.flushPromises(); assert.isTrue(meeting.meetingRequest.setBrb.notCalled); }); +it('does not send local brb state to server if webrtc media connection is false', async () => { + meeting.mediaProperties.webrtcMediaConnection = false; + brbState.enable(true, meeting.sendSlotManager); + brbState.handleServerBrbUpdate(true); + await testUtils.flushPromises(); + + assert.isTrue(meeting.meetingRequest.setBrb.notCalled); +});packages/@webex/plugin-meetings/src/meeting/brbState.ts (1)
102-104: 🛠️ Refactor suggestionPrevent potential stack overflow with recursive call
The recursive call to
applyClientStateToServercould potentially lead to a stack overflow if the state keeps changing rapidly. Consider using setTimeout to break the call stack.- // need to check if a new sync is required, because this.state.client may have changed while we were doing the current sync - this.applyClientStateToServer(sendSlotManager); + // need to check if a new sync is required, because this.state.client may have changed while we were doing the current sync + // use setTimeout to prevent potential stack overflow + setTimeout(() => { + if (!this.state.syncToServerInProgress) { + this.applyClientStateToServer(sendSlotManager); + } + }, 0);
🧹 Nitpick comments (4)
packages/@webex/plugin-meetings/src/meeting/brbState.ts (4)
16-18: Improve class documentation to clarify the concept of "remote" stateThe current documentation might lead to confusion about what "remote" means in this context. It's about the server state rather than another user's BRB state.
-/** The purpose of this class is to manage the local and remote brb state - * and make sure that the server state always matches the last requested state by the client. +/** The purpose of this class is to manage the client and server BRB state + * and make sure that the server state always matches the last state requested by the client. + * Unlike mute functionality, BRB state can only be changed by the user themselves.
79-79: Clarify variable naming to avoid confusionThe variable
remoteBrbRequiresSyncmight be confusing as "remote" could be interpreted as another user's state. Consider renaming to better reflect that this is checking if the server state needs updating.-const remoteBrbRequiresSync = this.state.client.enabled !== this.state.server.enabled; +const serverStateMismatch = this.state.client.enabled !== this.state.server.enabled;Then update all references to this variable in the subsequent code as well.
129-137: Enhance condition to explicitly check for false webrtcMediaConnectionThe current condition checks if
webrtcMediaConnectionis undefined, but not if it's explicitly false. Since both cases need to be handled, modify the check to be more explicit.-if (!this.meeting.mediaProperties.webrtcMediaConnection) { +// Explicitly check for both undefined and false values +if (this.meeting.mediaProperties.webrtcMediaConnection === undefined || + this.meeting.mediaProperties.webrtcMediaConnection === false) { const errorMessage = - 'Meeting:brbState#sendLocalBrbStateToServer: WebRTC media connection is not defined'; + 'Meeting:brbState#sendLocalBrbStateToServer: WebRTC media connection is not available'; const error = new Error(errorMessage); LoggerProxy.logger.error(error); return Promise.reject(error); }
107-107: Improve error logging with structured detailsThe current error logging only includes the error object, which might not provide sufficient context when debugging. Consider adding more structured information.
-LoggerProxy.logger.warn(`Meeting:brbState#applyClientStateToServer: error: ${e}`); +LoggerProxy.logger.warn(`Meeting:brbState#applyClientStateToServer: error syncing BRB state`, { + error: e.message || e, + clientState: this.state.client.enabled, + serverState: this.state.server.enabled, + meetingId: this.meeting?.id +});
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/@webex/plugin-meetings/src/meeting/brbState.ts(1 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/brbState.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Determine Changed Packages
| @@ -3390,6 +3392,7 @@ export default class Meeting extends StatelessWebexPlugin { | |||
| }); | |||
|
|
|||
| this.locusInfo.on(LOCUSINFO.EVENTS.SELF_MEETING_BRB_CHANGED, (payload) => { | |||
| this.brbState.handleServerBrbUpdate(payload?.brb?.enabled); | |||
There was a problem hiding this comment.
not sure if this is possible, but can Locus change our BRB state without us being in a meeting (so without Meeting.addMedia() being called and therefore with this.brbState being undefined)?
There was a problem hiding this comment.
Locus will notify us about self brb change only as a response to us changing brb so it should never be called with brbState being undefined, but just to make sure I've added ? so it won't break anything
|
|
||
| return Promise.reject(error); | ||
| }); | ||
| return this.brbState.enable(enabled, this.sendSlotManager); |
There was a problem hiding this comment.
if someone calls beRightBack() before calling addMedia(), this.brbState will be undefined. Previously this case was taken care of by the check for this.mediaProperties.webrtcMediaConnection.
There was a problem hiding this comment.
I've readded checks to this method for this case
COMPLETES SPARK-610065
This pull request addresses
Users can quickly toggle the "step away" feature on and off through the web client. To prevent issues on the server side caused by processing too many rapid requests, it feels good to limit the amount of these requests. This logic could similar to how the mute/unmute microphone functionality works.
by making the following changes
Based on
MuteStatenow there isBrbStatethat won't send request to server until first one finishes.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.