Feat(webinar): Panelist Join new data channel in practice session & Attendee receive whiteboard as stream #4041
Conversation
…eat/dataChannel4PS
…eat/dataChannel4PS
…eat/dataChannel4PS
|
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) 📝 WalkthroughWalkthroughThis pull request introduces enhancements to the meetings plugin, focusing on webinar functionality, media sharing, and event handling. Key changes include the addition of new constants for event triggers and display hints, updates to the Changes
Sequence DiagramsequenceDiagram
participant Meeting
participant Webinar
participant LocusInfo
Meeting->>Webinar: Initialize with meetingId
Meeting->>LocusInfo: Set up resource links listener
alt Payload exists
LocusInfo->>Webinar: Update webcast URL
Webinar-->>Meeting: Trigger resource links update event
end
Meeting->>Webinar: Handle role changes
Webinar->>Webinar: Update status by role
Possibly related PRs
Suggested reviewers
Poem
Finishing Touches
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
|
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
…eat/dataChannel4PS
…eat/dataChannel4PS
…eat/dataChannel4PS
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (7)
10738-10740: Remove unnecessary empty lineThere is an unnecessary empty line that should be removed for better code readability.
meeting.processRelayEvent = sinon.stub(); meeting.webinar.isJoinPracticeSessionDataChannel = sinon.stub().returns(false);
10870-10882: Remove unnecessary empty lines and improve formattingThere are several unnecessary empty lines that should be removed for better code readability.
- - it('connect ps data channel if ps started in webinar', async () => { meeting.joinedWith = {state: 'JOINED'}; meeting.locusInfo = {url: 'a url', info: {datachannelUrl: 'a datachannel url', practiceSessionDatachannelUrl: 'a ps datachannel url'}}; meeting.webinar.isJoinPracticeSessionDataChannel = sinon.stub().returns(true); await meeting.updateLLMConnection(); - assert.notCalled(webex.internal.llm.disconnectLLM); assert.calledWith(webex.internal.llm.registerAndConnect, 'a url', 'a ps datachannel url'); - });
11074-11074: Remove unnecessary empty lineThere is an unnecessary empty line that should be removed for better code readability.
meeting.locusInfo.info = {isWebinar: false};
11300-11317: Improve code formatting and readabilityThe code has inconsistent indentation and unnecessary empty lines. Let's clean it up.
eventTrigger.share.push(meeting.webinar.selfIsAttendee ? { eventName: EVENT_TRIGGERS.MEETING_STARTED_SHARING_REMOTE, functionName: 'remoteShare', eventPayload: { memberId: null, url, shareInstanceId, annotationInfo: undefined, resourceType: undefined, }, } : { eventName: EVENT_TRIGGERS.MEETING_STARTED_SHARING_WHITEBOARD, functionName: 'startWhiteboardShare', eventPayload: {resourceUrl, memberId: beneficiaryId}, }); shareStatus = meeting.webinar.selfIsAttendee ? SHARE_STATUS.REMOTE_SHARE_ACTIVE : SHARE_STATUS.WHITEBOARD_SHARE_ACTIVE;
11349-11366: Improve code formatting and readabilityThe code has inconsistent indentation and unnecessary empty lines. Let's clean it up.
eventTrigger.share.push(meeting.webinar.selfIsAttendee ? { eventName: EVENT_TRIGGERS.MEETING_STARTED_SHARING_REMOTE, functionName: 'remoteShare', eventPayload: { memberId: null, url, shareInstanceId, annotationInfo: undefined, resourceType: undefined, }, } : { eventName: EVENT_TRIGGERS.MEETING_STARTED_SHARING_WHITEBOARD, functionName: 'startWhiteboardShare', eventPayload: {resourceUrl, memberId: beneficiaryId}, }); shareStatus = meeting.webinar.selfIsAttendee ? SHARE_STATUS.REMOTE_SHARE_ACTIVE : SHARE_STATUS.WHITEBOARD_SHARE_ACTIVE;
11493-11522: Refactor test structure to reduce nestingThe test suite has excessive nesting of
describe()blocks which makes it harder to read and maintain. Consider refactoring to reduce nesting levels.- describe('Whiteboard Share - Webinar Attendee', () => { - it('Scenario #1: Whiteboard sharing as a webinar attendee', () => { - // Test code... - }); - }); + // Flatten the structure by using a descriptive test name + it('handles whiteboard sharing as a webinar attendee', () => { + // Test code... + });🧰 Tools
🪛 Biome (1.9.4)
[error] 11493-11522: 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)
10870-10882: Improve test organization for webinar attendee scenariosThe webinar attendee test cases are scattered in different places. Consider grouping all webinar attendee related tests together for better organization and readability.
Move the
connect ps data channel if ps started in webinartest case to be grouped with other webinar attendee test cases for better organization. This will make it easier to find and maintain all webinar-related test cases.Also applies to: 11493-11522
packages/@webex/plugin-meetings/test/unit/spec/locus-info/index.js (1)
12-12: Fix the import path typo.The import path contains a double forward slash which should be corrected.
-import MediaSharesUtils from '@webex/plugin-meetings/src/locus-info//mediaSharesUtils'; +import MediaSharesUtils from '@webex/plugin-meetings/src/locus-info/mediaSharesUtils';packages/@webex/plugin-meetings/test/unit/spec/webinar/index.ts (1)
152-251: Consider adding edge case tests.While the test suite covers the main scenarios, consider adding tests for:
- Error cases in
updateMediaShares- Error cases in
updateLLMConnection- Concurrent role changes
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/@webex/plugin-meetings/src/constants.ts(2 hunks)packages/@webex/plugin-meetings/src/locus-info/index.ts(2 hunks)packages/@webex/plugin-meetings/src/meeting/index.ts(8 hunks)packages/@webex/plugin-meetings/src/meeting/util.ts(1 hunks)packages/@webex/plugin-meetings/src/webinar/index.ts(3 hunks)packages/@webex/plugin-meetings/test/unit/spec/locus-info/index.js(2 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js(6 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/utils.js(3 hunks)packages/@webex/plugin-meetings/test/unit/spec/webinar/index.ts(18 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/@webex/plugin-meetings/test/unit/spec/meeting/utils.js
🧰 Additional context used
🪛 Biome (1.9.4)
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js
[error] 11493-11522: 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)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Determine Changed Packages
- GitHub Check: AWS Amplify Console Web Preview
🔇 Additional comments (13)
packages/@webex/plugin-meetings/src/locus-info/index.ts (1)
Line range hint
1286-1307: LGTM! TheforceUpdateparameter addition enhances flexibility.The changes allow forcing a media share update even when the content hasn't changed, which is useful for scenarios where we need to re-trigger media share events. The implementation is clean and maintains backward compatibility with the default
falsevalue.packages/@webex/plugin-meetings/src/meeting/index.ts (3)
853-853: LGTM! Properly initialized Webinar class with meetingId.The change ensures that the Webinar instance is created with the correct meetingId parameter.
3068-3081: LGTM! Added resource links update event trigger.The change properly triggers the
MEETING_RESOURCE_LINKS_UPDATEevent when resource links are updated, allowing clients to react to resource link changes.
5604-5613: LGTM! Added support for practice session data channel.The change enables webinar panelists to use the practice session data channel URL when appropriate, improving the webinar practice session functionality.
packages/@webex/plugin-meetings/test/unit/spec/locus-info/index.js (1)
1641-1767: Well-structured test suite with comprehensive coverage!The test suite for
updateMediaSharesis well-implemented with:
- Clear test cases covering both normal and edge cases
- Proper setup and teardown
- Comprehensive assertions
- Good use of stubs and spies
packages/@webex/plugin-meetings/src/webinar/index.ts (4)
28-28: LGTM! Property addition looks good.The
meetingIdproperty is correctly typed as 'string' and aligns with the class's needs.
72-73: LGTM! Role change logic enhancement.The additional condition
!oldRoles.includes(SELF_ROLES.ATTENDEE) && newRoles.includes(SELF_ROLES.ATTENDEE)correctly handles the case when an attendee joins the meeting without previously holding the attendee role.
105-111: LGTM! Clear and concise implementation.The
isJoinPracticeSessionDataChannelmethod has a clear purpose and correctly checks both required conditions.
82-103: Verify error handling in updateStatusByRole.The method handles media share updates and practice session data channel connections, but there's no error handling for potential failures in
updateMediaSharesorupdateLLMConnection.Run this script to check for error handling patterns in similar methods:
packages/@webex/plugin-meetings/src/meeting/util.ts (1)
444-445: LGTM! Consistent with existing patterns.The
canStartBreakoutmethod follows the established pattern for display hint checks and correctly implements the inverse logic for theDISABLE_BREAKOUT_STARThint.packages/@webex/plugin-meetings/test/unit/spec/webinar/index.ts (2)
106-119: LGTM! Good test coverage for new join case.The test verifies all state changes and role transitions for the attendee join scenario.
286-308: LGTM! Comprehensive test coverage.The test suite thoroughly verifies all possible combinations of panelist status and practice session state.
packages/@webex/plugin-meetings/src/constants.ts (1)
967-967: LGTM! Consistent constant naming.The new
DISABLE_BREAKOUT_STARTconstant follows the established naming pattern for display hints.
mickelr
changed the title
mickelr
changed the title
arun3528
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (1)
11444-11473: Refactor test structure to reduce nesting.The test structure has excessive nesting of
describe()blocks which makes the tests harder to read and maintain. Consider flattening the test hierarchy by:
- Moving the webinar attendee test cases to a top-level describe block
- Using more descriptive test names to maintain context without deep nesting
- describe('Whiteboard Share - Webinar Attendee', () => { - it('Scenario #1: Whiteboard sharing as a webinar attendee', () => { + describe('webinar attendee whiteboard sharing', () => { + it('handles whiteboard sharing when user is a webinar attendee', () => {🧰 Tools
🪛 Biome (1.9.4)
[error] 11444-11473: 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)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/@webex/plugin-meetings/src/meeting/index.ts(8 hunks)packages/@webex/plugin-meetings/src/meeting/util.ts(1 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js(6 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/utils.js(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/@webex/plugin-meetings/src/meeting/util.ts
- packages/@webex/plugin-meetings/test/unit/spec/meeting/utils.js
- packages/@webex/plugin-meetings/src/meeting/index.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js
[error] 11444-11473: 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)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Initialize Project
- GitHub Check: AWS Amplify Console Web Preview
🔇 Additional comments (2)
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (2)
10689-10689: LGTM! Good handling of practice session data channel.The changes correctly handle the practice session data channel URL for webinar scenarios. The code checks if the user should join the practice session data channel and uses the appropriate URL.
Also applies to: 10821-10833
11251-11268: LGTM! Proper handling of whiteboard sharing for webinar attendees.The changes correctly handle whiteboard sharing events for webinar attendees by:
- Using remote share events instead of whiteboard events
- Setting the appropriate share status
- Maintaining consistent behavior with the rest of the codebase
Also applies to: 11300-11317
COMPLETES #< INSERT LINK TO ISSUE >
https://jira-eng-gpk2.cisco.com/jira/browse/SPARK-602151
https://jira-eng-gpk2.cisco.com/jira/browse/SPARK-585446
https://jira-eng-gpk2.cisco.com/jira/browse/SPARK-595509
This pull request addresses
by making the following changes
Change Type
The following scenarios were tested
< ENUMERATE TESTS PERFORMED, WHETHER MANUAL OR AUTOMATED >
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.