feat: premise recording sdk support #4021
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 [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) WalkthroughThe pull request introduces several modifications across multiple files in the Changes
Possibly related PRs
Suggested labels
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (7)
packages/@webex/plugin-meetings/src/recording-controller/index.ts (2)
231-234: Add missing JSDoc comment for therecordingTypeparameterThe
recordingServicemethod's JSDoc is missing documentation for therecordingTypeparameter. Please add a@paramtag for it.* @param {RecordingAction} action + * @param {RecordingType} recordingType * @private * @memberof RecordingController * @returns {Promise}
289-294: SimplifyrecordingTypeassignment using a ternary operatorYou can simplify the assignment of
recordingTypeby using a ternary operator for conciseness.- let recordingType: RecordingType; - if (premiseRecStatus) { - recordingType = RecordingType.Premise; - } else { - recordingType = RecordingType.Cloud; - } + const recordingType = premiseRecStatus ? RecordingType.Premise : RecordingType.Cloud;packages/@webex/webex-core/src/lib/webex-http-error.js (1)
30-30: Simplify conditional with optional chaining operatorYou can simplify the conditional check by using the optional chaining operator
?..if (this.body && this.body.errorCode) { - message += `\nerrorCode : ${this.body.errorCode}`; + message += `\nerrorCode : ${this.body?.errorCode}`; }🧰 Tools
🪛 Biome (1.9.4)
[error] 30-30: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/@webex/plugin-meetings/src/constants.ts (1)
907-910: LGTM! Consider adding JSDoc comments.The new premise recording control constants are well-structured and follow the existing patterns. They're appropriately placed in alphabetical order within the
DISPLAY_HINTSobject.Consider adding JSDoc comments to document the purpose and usage of these new constants, similar to how other sections like
LAYOUT_TYPESare documented. For example:/** * @description Display hints for premise recording controls * @constant */packages/@webex/plugin-meetings/test/unit/spec/recording-controller/index.js (1)
231-240: Consider parameterizing premise recording tests.The premise recording tests follow the same pattern for all actions (start, stop, pause, resume). Consider using test parameterization to reduce code duplication.
Example refactor:
const recordingActions = [ { action: 'start', hint: 'PREMISE_RECORDING_CONTROL_START' }, { action: 'stop', hint: 'PREMISE_RECORDING_CONTROL_STOP' }, { action: 'pause', hint: 'PREMISE_RECORDING_CONTROL_PAUSE' }, { action: 'resume', hint: 'PREMISE_RECORDING_CONTROL_RESUME' } ]; recordingActions.forEach(({action, hint}) => { it(`can ${action} premise recording when the correct display hint is present`, () => { controller.setDisplayHints([hint]); const result = controller[`${action}Recording`](); assert.calledWith(request.request, { uri: `test/loci/id/recording`, body: { meetingInfo: {locusSessionId: 'testId'}, recording: {action}, recordingType: 'premise' }, method: HTTP_VERBS.PUT }); assert.deepEqual(result, request.request.firstCall.returnValue); }); });Also applies to: 269-277, 307-315, 345-353
packages/@webex/plugin-meetings/test/unit/spec/recording-controller/util.js (2)
32-39: Consider adding more comprehensive test coverage for premise recording.While the basic test cases for premise recording controls are good, consider adding:
- Tests verifying behavior when both cloud and premise recording hints are present
- Tests for policy checks with premise recording (similar to existing cloud recording tests)
- Negative test cases specific to premise recording
Example test case to add:
it('prioritizes premise recording when both cloud and premise hints are present', () => { locusInfo.parsedLocus.info.userDisplayHints.push('RECORDING_CONTROL_START'); locusInfo.parsedLocus.info.userDisplayHints.push('PREMISE_RECORDING_CONTROL_START'); assert.equal( RecordingUtil.canUserStart(locusInfo.parsedLocus.info.userDisplayHints), true ); // Add assertions to verify which type of recording is prioritized });Also applies to: 81-88, 130-137, 181-188
32-32: Add JSDoc comments explaining premise recording test scenarios.To improve maintainability and align with the PR's objective of adding premise recording support, consider adding JSDoc comments explaining:
- The purpose of premise recording tests
- The difference between cloud and premise recording controls
- Any specific requirements or constraints for premise recording
Example:
/** * Test suite for premise recording controls. * Premise recording differs from cloud recording in that... [explain difference] * These tests verify that the SDK correctly handles premise recording display hints * and maintains compatibility with existing cloud recording functionality. */ describe('premise recording tests', () => { // ... existing tests ... });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
packages/@webex/plugin-meetings/src/constants.ts(1 hunks)packages/@webex/plugin-meetings/src/meeting/in-meeting-actions.ts(3 hunks)packages/@webex/plugin-meetings/src/recording-controller/enums.ts(1 hunks)packages/@webex/plugin-meetings/src/recording-controller/index.ts(4 hunks)packages/@webex/plugin-meetings/src/recording-controller/util.ts(2 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/in-meeting-actions.ts(2 hunks)packages/@webex/plugin-meetings/test/unit/spec/recording-controller/index.js(4 hunks)packages/@webex/plugin-meetings/test/unit/spec/recording-controller/util.js(6 hunks)packages/@webex/webex-core/src/lib/webex-http-error.js(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/@webex/webex-core/src/lib/webex-http-error.js
[error] 30-30: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (3)
packages/@webex/plugin-meetings/src/recording-controller/enums.ts (1)
1-1: Verify impact of changing default export to named export for RecordingAction
By changing RecordingAction from a default export to a named export, you may affect modules that import it as a default export. Ensure that all imports of RecordingAction are updated to use named imports to prevent breaking changes.
Run the following script to find all imports of RecordingAction and check if they use default imports:
packages/@webex/plugin-meetings/test/unit/spec/recording-controller/index.js (1)
224-229: Verify HTTP method consistency.
The test now uses PUT instead of PATCH for recording actions. Ensure this change is intentional and documented in the API specifications.
✅ Verification successful
HTTP method change is intentional and consistent with implementation
The change from PATCH to PUT for recording actions is intentional and consistent across the codebase. The implementation in src/recording-controller/index.ts uses HTTP_VERBS.PUT for all recording actions (start, stop, pause, resume) for both cloud and premise recording types. The test changes align with this implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any remaining PATCH endpoints for recording
rg -A 2 "method: HTTP_VERBS.PATCH.*recording"
Length of output: 47
Script:
#!/bin/bash
# Let's search for all recording-related HTTP methods to understand the pattern
rg -B 2 -A 2 "recording.*method: HTTP_VERBS\." --type js
# Also search for any recording-related API documentation
fd -e md -e txt . | xargs rg -l "recording.*api"
# Check the actual implementation of recording endpoints
ast-grep --pattern 'recording($$$) {
$$$
method: $_
$$$
}'
Length of output: 329
Script:
#!/bin/bash
# Let's try a broader search for recording-related HTTP methods
rg -B 2 -A 2 "method.*HTTP_VERBS" packages/@webex/plugin-meetings
# Search specifically in the recording controller implementation
fd "recording-controller" --type f --exec cat {}
# Look for any recording-related tests
rg -B 2 -A 2 "recording.*test|test.*recording" packages/@webex/plugin-meetings/test
Length of output: 52668
packages/@webex/plugin-meetings/test/unit/spec/recording-controller/util.js (1)
2-2: Verify the impact of import change across the codebase.
The change from default to named import of RecordingAction suggests a refactor in the enums file. Let's verify that all imports are consistently updated.
✅ Verification successful
Import change is consistent with the codebase
The RecordingAction enum is properly exported in the enums file and the test file is the only one importing it. The named import usage is correct as the enum is exported using a named export. No issues were found with the import changes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining default imports of RecordingAction
rg "import RecordingAction from.*recording-controller/enums" --type js --type ts
Length of output: 82
Script:
#!/bin/bash
# Search for any imports of RecordingAction (both named and default)
rg "import.*RecordingAction.*from.*recording-controller/enums" --type js --type ts
# Also check the enum file itself to understand the export
fd "enums.ts|enums.js" -t f --exec cat {}
Length of output: 848
packages/@webex/plugin-meetings/src/recording-controller/index.ts
Outdated
Show resolved
Hide resolved
packages/@webex/plugin-meetings/src/recording-controller/util.ts
Outdated
Show resolved
Hide resolved
packages/@webex/plugin-meetings/test/unit/spec/meeting/in-meeting-actions.ts
Outdated
Show resolved
Hide resolved
packages/@webex/plugin-meetings/src/meeting/in-meeting-actions.ts
Outdated
Show resolved
Hide resolved
packages/@webex/plugin-meetings/src/meeting/in-meeting-actions.ts
Outdated
Show resolved
Hide resolved
packages/@webex/plugin-meetings/src/meeting/in-meeting-actions.ts
Outdated
Show resolved
Hide resolved
packages/@webex/plugin-meetings/test/unit/spec/recording-controller/util.js
Show resolved
Hide resolved
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
packages/@webex/plugin-meetings/src/recording-controller/index.ts (2)
289-295: Simplify conditional assignment ofrecordingTypeYou can simplify the assignment of
recordingTypeby using a ternary operator:- let recordingType: RecordingType; - if (premiseRecStatus) { - recordingType = RecordingType.Premise; - } else { - recordingType = RecordingType.Cloud; - } + const recordingType = premiseRecStatus ? RecordingType.Premise : RecordingType.Cloud;
231-231: Add description forrecordingTypeparameter in the documentationPlease include a brief description for the
recordingTypeparameter in the method's documentation to enhance clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
packages/@webex/plugin-meetings/src/meeting/in-meeting-actions.ts(3 hunks)packages/@webex/plugin-meetings/src/meeting/index.ts(1 hunks)packages/@webex/plugin-meetings/src/recording-controller/index.ts(4 hunks)packages/@webex/plugin-meetings/src/recording-controller/util.ts(2 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/in-meeting-actions.ts(2 hunks)packages/@webex/plugin-meetings/test/unit/spec/recording-controller/util.js(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/@webex/plugin-meetings/test/unit/spec/meeting/in-meeting-actions.ts
- packages/@webex/plugin-meetings/src/meeting/in-meeting-actions.ts
- packages/@webex/plugin-meetings/test/unit/spec/recording-controller/util.js
- packages/@webex/plugin-meetings/src/recording-controller/util.ts
🔇 Additional comments (1)
packages/@webex/plugin-meetings/src/meeting/index.ts (1)
3769-3772: Code addition for 'isPremiseRecordingEnabled' looks good
The isPremiseRecordingEnabled property is correctly added and utilizes the RecordingUtil.isPremiseRecordingEnabled function with appropriate arguments.
packages/@webex/plugin-meetings/src/recording-controller/index.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/@webex/plugin-meetings/src/recording-controller/index.ts (3)
236-236: Consider specifying a more precise return typeThe Promise return type could be more specific to improve type safety and documentation.
- private recordingService(action: RecordingAction, recordingType: RecordingType): Promise<any> { + private recordingService(action: RecordingAction, recordingType: RecordingType): Promise<{success: boolean}> {
281-284: Consider renaming the boolean variable for clarityThe variable
isPremiseRecordingEnabledis named like a function but holds a boolean value.- const isPremiseRecordingEnabled = Util.isPremiseRecordingEnabled( + const premiseRecordingEnabled = Util.isPremiseRecordingEnabled( this.displayHints, this.selfUserPolicies );
289-295: Add logging for selected recording typeConsider logging which recording type was selected for better debugging and monitoring.
let recordingType: RecordingType; if (isPremiseRecordingEnabled) { recordingType = RecordingType.Premise; } else { recordingType = RecordingType.Cloud; } + LoggerProxy.logger.log( + `RecordingController:index#recordingFacade --> selected recording type [${recordingType}]` + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/@webex/plugin-meetings/src/recording-controller/index.ts(4 hunks)
🔇 Additional comments (2)
packages/@webex/plugin-meetings/src/recording-controller/index.ts (2)
5-5: LGTM: Import changes align with premise recording support
The addition of RecordingType import is appropriate for the new premise recording functionality.
Line range hint 231-246: LGTM: recordingService method properly integrates recordingType
The implementation correctly handles the new recordingType parameter in both the method signature and request body.
| @@ -22,6 +22,15 @@ export default class WebexHttpError extends HttpError { | |||
| value: res.options, | |||
| }); | |||
|
|
|||
| Reflect.defineProperty(this, 'body', { | |||
There was a problem hiding this comment.
what is this change made for?
There was a problem hiding this comment.
This change is added to get the error code if available from the failed API response so that we can log it for better troubleshooting.
There was a problem hiding this comment.
I'm kind of concerned about this change. Not sure why we have not needed this before. But I have not tested it and if it's needed, it does seem fine.
| @@ -22,6 +22,15 @@ export default class WebexHttpError extends HttpError { | |||
| value: res.options, | |||
| }); | |||
|
|
|||
| Reflect.defineProperty(this, 'body', { | |||
There was a problem hiding this comment.
I'm kind of concerned about this change. Not sure why we have not needed this before. But I have not tested it and if it's needed, it does seem fine.
COMPLETES # SPARK-560871
This pull request addresses
Video Mesh has a requirement to support premise recordings. This is irrespective of any meeting type (Private or non-private). Detailed business justification can be found here: https://cisco-my.sharepoint.com/:w:/p/roshin/EXmmBsz8qHdJknee8fKiFnQBDCXOH9YBGL_S_MyhjCtLrw?e=P2Se1V
Documentation: https://confluence-eng-gpk2.cisco.com/conf/display/WTWC/SPARK-560871+-+VMN-Recording%3A+Web+Client+support
by making the following changes
Adding the support for recordingType in the rss recording api.
Checking the displayhints to make sure whether it is a premise recording or cloud recording
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.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Tests