feat: added meetingJoinPhase to CA metrics #3954
Conversation
WalkthroughThe changes involve modifications to 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 (
|
|
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
🧹 Outside diff range and nitpick comments (3)
packages/@webex/internal-plugin-metrics/src/metrics.types.ts (1)
288-289: LGTM with minor formatting suggestions.The type definition is well-structured and correctly extracts the type from the existing
RawEvent. Consider removing the extra blank line before the type definition to maintain consistent spacing with other type definitions in the file.- export type MeetingJoinPhase = RawEvent['event']['meetingJoinPhase']; +export type MeetingJoinPhase = RawEvent['event']['meetingJoinPhase'];packages/@webex/internal-plugin-metrics/src/call-diagnostic/call-diagnostic-metrics.ts (1)
123-137: Documentation needs improvement.The getter and setter implementations have several documentation issues:
- Incorrect parameter name in setter's JSDoc (shows 'correlationId')
- Incorrect return type in getter's JSDoc (shows 'string')
- Missing clear descriptions of their purpose
Apply these documentation improvements:
/** - * Getter - Returns current meetingJoinPhase value - * @returns {string} + * Gets the current phase of the meeting join process + * @returns {MeetingJoinPhase} The current meeting join phase */ get meetingJoinPhase() { return this.joinMeetingPhase; } /** - * Setter - sets meetingJoinPhase value - * @param {string} correlationId + * Sets the current phase of the meeting join process + * @param {MeetingJoinPhase} joinMeetingPhase The new meeting join phase */ set meetingJoinPhase(joinMeetingPhase: MeetingJoinPhase) { this.joinMeetingPhase = joinMeetingPhase; }packages/@webex/internal-plugin-metrics/test/unit/spec/call-diagnostic/call-diagnostic-metrics.ts (1)
702-716: Test coverage could be enhanced.The test suite for
meetingJoinPhaseprovides good basic coverage. However, consider adding the following test cases to make it more robust:
- Validation of invalid values
- Edge cases (null, undefined)
- Error handling scenarios
Here's a suggested enhancement:
describe('#meetingJoinPhase', () => { it('should have a default value', () => { const res = cd.meetingJoinPhase; assert.equal(res, 'pre-join'); }); it('should have a getter and setter', () => { cd.meetingJoinPhase = 'join'; assert.equal(cd.meetingJoinPhase, 'join'); cd.meetingJoinPhase = 'in-meeting'; assert.equal(cd.meetingJoinPhase, 'in-meeting'); }); + it('should handle invalid values', () => { + assert.throws(() => { + cd.meetingJoinPhase = 'invalid-phase'; + }, /Invalid meeting join phase/); + }); + + it('should handle edge cases', () => { + assert.throws(() => { + cd.meetingJoinPhase = null; + }, /Invalid meeting join phase/); + + assert.throws(() => { + cd.meetingJoinPhase = undefined; + }, /Invalid meeting join phase/); + }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
packages/@webex/internal-plugin-metrics/src/call-diagnostic/call-diagnostic-metrics.ts(3 hunks)packages/@webex/internal-plugin-metrics/src/metrics.types.ts(1 hunks)packages/@webex/internal-plugin-metrics/test/unit/spec/call-diagnostic/call-diagnostic-metrics.ts(1 hunks)
🔇 Additional comments (3)
packages/@webex/internal-plugin-metrics/src/call-diagnostic/call-diagnostic-metrics.ts (3)
43-43: LGTM: Import added correctly.
The MeetingJoinPhase type is properly imported from the metrics.types.ts file.
99-99: LGTM: Property declaration is well-structured.
The private property joinMeetingPhase is properly typed and initialized with a default value of 'pre-join'.
43-43: Verify the MeetingJoinPhase implementation.
The implementation looks good, but we should verify:
- The existence and correctness of the
MeetingJoinPhasetype - The usage of this new property in the codebase
Also applies to: 99-99, 123-137
✅ Verification successful
Let me gather more information about the RawEvent type to understand the complete type definition.
Let me try a different approach to check the RawEvent definition.
MeetingJoinPhase implementation is properly integrated
The verification confirms:
MeetingJoinPhasetype is correctly defined inmetrics.types.tsasRawEvent['event']['meetingJoinPhase']- The implementation has proper test coverage showing valid values: 'pre-join' (default), 'join', and 'in-meeting'
- The getter and setter are properly implemented and tested in
call-diagnostic-metrics.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify MeetingJoinPhase type definition and usage
# Test 1: Check if MeetingJoinPhase type exists and its definition
echo "Checking MeetingJoinPhase type definition..."
rg -p "type MeetingJoinPhase" --type ts
# Test 2: Find usage of meetingJoinPhase getter/setter
echo "Checking meetingJoinPhase usage..."
rg "meetingJoinPhase" --type ts
Length of output: 1909
Script:
#!/bin/bash
# Check the RawEvent type definition to understand meetingJoinPhase's type structure
ast-grep --pattern 'type RawEvent = {
$$$
}'
# Also check for interface definition as an alternative
ast-grep --pattern 'interface RawEvent {
$$$
}'
Length of output: 100
Script:
#!/bin/bash
# Search for RawEvent definition with context
rg -p "RawEvent" -B 2 -A 10 --type ts packages/@webex/internal-plugin-metrics/src/metrics.types.ts
# Also check the test file to understand valid values for meetingJoinPhase
rg -p "describe.*meetingJoinPhase" -B 2 -A 15 packages/@webex/internal-plugin-metrics/test/unit/spec/call-diagnostic/call-diagnostic-metrics.ts
Length of output: 2781
COMPLETES #SPARK-576651
This pull request addresses
CA events sent by web app are missing metingJoinPhase
by making the following changes
Added metingJoinPhase to the new metrics plugin, the CallDiagnosticMetrics class that is responsible for CA events. Added a getter and setter so that both app and SDK can use and control the value. There is only 1 value, so this won't work if we ever want to join multiple meetings in parallel, but anyway SDK doesn't support that right now and there are other things in the metrics plugin that are done in the same way and assume only 1 meeting join at a time
Change Type
The following scenarios where tested
unit tests and manually with the web app
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
Tests