fix: end meeting for all getCurUserType type issue #4038
Conversation
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
warning eslint@8.57.1: This version is no longer supported. Please see https://eslint.org/version-support for other options. (For a CapTP with native promises, see @endo/eventual-send and @endo/captp) WalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🔇 Additional comments (4)packages/@webex/plugin-meetings/src/meetings/index.ts (2)
The additions to
The implementation correctly stores the additional meeting properties (roles, callStateForMetrics, and getCurUserType) when destroying meetings, ensuring important state information is preserved. packages/@webex/plugin-meetings/test/unit/spec/meetings/index.js (2)
The test assertions properly verify that roles and callStateForMetrics are preserved in deletedMeetings after meeting destruction. Line range hint The setup function changes properly handle different user types and scenarios, ensuring robust testing of the new functionality. 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). |
![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 (1)
packages/@webex/plugin-meetings/test/unit/spec/meetings/index.js (1)
2099-2099: Consistent spacing in setup function parameters.Minor formatting improvement for better code consistency.
Also applies to: 2120-2120, 2122-2122
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/@webex/plugin-meetings/src/meetings/index.ts(2 hunks)packages/@webex/plugin-meetings/test/unit/spec/meetings/index.js(5 hunks)
🔇 Additional comments (3)
packages/@webex/plugin-meetings/src/meetings/index.ts (2)
158-160: LGTM! Type definition enhancement.
The new properties added to BasicMeetingInformation type appropriately capture additional meeting state information that needs to be preserved.
1149-1151: LGTM! Preserving additional meeting state.
The destroy method now correctly preserves roles, call state metrics, and user type information in the deletedMeetings map, allowing this data to be accessed even after meeting destruction.
packages/@webex/plugin-meetings/test/unit/spec/meetings/index.js (1)
1988-1989: LGTM! Comprehensive test coverage for new properties.
The test has been properly updated to verify that roles and callStateForMetrics are correctly preserved in deletedMeetings and can be retrieved via getBasicMeetingInformation.
Also applies to: 2026-2027
| @@ -1143,6 +1146,9 @@ export default class Meetings extends WebexPlugin { | |||
| sessionId: meeting.locusInfo?.fullState?.sessionId, | |||
| }, | |||
| }, | |||
| roles: meeting.roles, | |||
| callStateForMetrics: meeting.callStateForMetrics, | |||
| getCurUserType: meeting.getCurUserType, | |||
There was a problem hiding this comment.
initially I didn't like this approach, because I thought that having a reference to the method of a meeting will prevent that meeting object from being garbage collected, but I think it's OK, you can have references to methods of an object and these methods on their own don't have a reference to the object itself (as long as they don't use any closures), so it should be fine.
| @@ -155,6 +155,9 @@ export type BasicMeetingInformation = { | |||
| }; | |||
| meetingInfo: any; | |||
| sessionCorrelationId: string; | |||
| roles: string[]; | |||
There was a problem hiding this comment.
question. Why have we also added roles and callStateForMetrics?
There was a problem hiding this comment.
roles being read in getCurUserType utility function and callStateForMetrics is being read in next line where previous error occured.
COMPLETES # CSCwn60262
This pull request addresses
fixing Console error when we call meeting.EndMeetingForAll()
by making the following changes
Added below properties and function to BasicMeetingInformation type so that they will be accessible after ending the meeting as well.
roles: string[];
getCurUserType: () => string | null;
callStateForMetrics: CallStateForMetrics;
Change Type
The following scenarios were tested
Joined meeting as host in sample app and called endMeetingForAll() => there were no errors observed.
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
Tests