feat(consolidated-interstitial): add joinFlowVersion as CA event option #3991
Conversation
WalkthroughThe pull request introduces updates to the Changes
Possibly related PRs
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: 0
🧹 Outside diff range and nitpick comments (3)
packages/@webex/internal-plugin-metrics/test/unit/spec/call-diagnostic/call-diagnostic-metrics.ts (1)
2761-2778: Add a test case for whenjoinFlowVersionis undefinedCurrently, there is a test verifying that
joinFlowVersionis included in the event payload when provided. It's advisable to also test the scenario wherejoinFlowVersionis not provided to ensure the method handles undefined values correctly.Consider adding the following test case:
+ it('does not include joinFlowVersion when not provided', async () => { + const options = { + meetingId: fakeMeeting.id, + preLoginId: 'myPreLoginId', + // 'joinFlowVersion' not included + }; + + const fetchOptions = await cd.buildClientEventFetchRequestOptions({ + name: 'client.exit.app', + payload: {trigger: 'user-interaction', canProceed: false}, + options, + }); + + expect(fetchOptions.body.metrics[0].eventPayload.event.joinFlowVersion).toBeUndefined(); + });packages/@webex/internal-plugin-metrics/src/call-diagnostic/call-diagnostic-metrics.ts (2)
671-671: Consider adding type annotation for joinFlowVersion parameterThe
joinFlowVersionparameter is added without a type annotation. While TypeScript can infer types from theSubmitClientEventOptionsinterface, explicit type annotations in method signatures improve code readability and maintainability.- joinFlowVersion, + joinFlowVersion: MetricEventJoinFlowVersion,
Line range hint
671-724: Consider adding JSDoc documentation for the new parameterThe
joinFlowVersionparameter lacks documentation explaining its purpose and possible values. Adding JSDoc comments would help other developers understand when and how to use this parameter.Add documentation above the method:
/** * Create client event object for in meeting events * @param arg - create args * @param arg.name - event key * @param arg.options - options * @param arg.options.joinFlowVersion - Version of the join flow (e.g., 'Other' or 'NewFTE') * @param arg.errors - errors * @returns object */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
packages/@webex/internal-plugin-metrics/package.json(1 hunks)packages/@webex/internal-plugin-metrics/src/call-diagnostic/call-diagnostic-metrics.ts(2 hunks)packages/@webex/internal-plugin-metrics/src/metrics.types.ts(2 hunks)packages/@webex/internal-plugin-metrics/test/unit/spec/call-diagnostic/call-diagnostic-metrics.ts(11 hunks)
🔇 Additional comments (4)
packages/@webex/internal-plugin-metrics/package.json (1)
40-40: Verify version compatibility and requirements
The significant version jump of @webex/event-dictionary-ts from 1546 to 1594 appears to be related to supporting the new joinFlowVersion feature.
Let's verify the version requirements:
✅ Verification successful
Version 1.0.1594 is appropriate for joinFlowVersion support
The version bump to 1.0.1594 is justified as this version includes the required joinFlowVersion type definition with supported values "NewFTE" | "Other". The type is properly integrated across multiple event interfaces in the package. The older version 1.0.1546 does not contain this field, confirming this is the correct version for the new feature.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check version history and changes of @webex/event-dictionary-ts
# Check if this version is published and stable
curl -s "https://registry.npmjs.org/@webex/event-dictionary-ts" | jq '.versions["1.0.1594"]'
# Look for any breaking changes or issues between these versions
curl -s "https://registry.npmjs.org/@webex/event-dictionary-ts" | \
jq -r '.versions | keys[]' | \
awk -F. '$1=="1" && $2=="0" && $3>=1546 && $3<=1594' | \
while read -r version; do
echo "Checking version $version"
curl -s "https://registry.npmjs.org/@webex/event-dictionary-ts/$version" | \
jq -r '.description + "\n" + (.dist.tarball // "")'
done
# Check if there are any versions between 1546 and 1594 that also support joinFlowVersion
# This helps determine if we're using the minimum required version
curl -s "https://registry.npmjs.org/@webex/event-dictionary-ts" | \
jq -r '.versions | keys[]' | \
awk -F. '$1=="1" && $2=="0" && $3>=1546 && $3<=1594' | \
while read -r version; do
echo "Checking version $version for joinFlowVersion support"
curl -s "$(curl -s "https://registry.npmjs.org/@webex/event-dictionary-ts/$version" | \
jq -r '.dist.tarball')" | \
tar -xzO -f - package/dist/index.js 2>/dev/null | \
grep -l "joinFlowVersion" >/dev/null && \
echo "Version $version supports joinFlowVersion"
done
Length of output: 47624
Script:
#!/bin/bash
# Let's try a different approach to check for joinFlowVersion support
# by examining the package contents more directly
# 1. Get the tarball URL for version 1.0.1594
TARBALL_URL=$(curl -s "https://registry.npmjs.org/@webex/event-dictionary-ts/1.0.1594" | jq -r '.dist.tarball')
# 2. Download and examine the package contents for joinFlowVersion
curl -s "$TARBALL_URL" | tar -xzO -f - package/generated/typescript/index.d.ts 2>/dev/null | grep -A 5 -B 5 "joinFlowVersion"
# 3. Also check the same for version 1.0.1546 to compare
OLD_TARBALL_URL=$(curl -s "https://registry.npmjs.org/@webex/event-dictionary-ts/1.0.1546" | jq -r '.dist.tarball')
echo -e "\nChecking old version 1.0.1546:"
curl -s "$OLD_TARBALL_URL" | tar -xzO -f - package/generated/typescript/index.d.ts 2>/dev/null | grep -A 5 -B 5 "joinFlowVersion"
# 4. Check the changelog or commit history if available
curl -s "https://registry.npmjs.org/@webex/event-dictionary-ts/1.0.1594" | jq -r '.gitHead' | while read -r commit; do
echo -e "\nChecking commit message for version 1.0.1594 ($commit):"
curl -s "https://api.github.com/repos/webex/webex-js-sdk/commits/$commit" | jq -r '.commit.message' 2>/dev/null
done
Length of output: 4407
packages/@webex/internal-plugin-metrics/src/metrics.types.ts (2)
113-114: LGTM! Well-structured type definition.
The new MetricEventJoinFlowVersion type follows the existing naming conventions and provides clear, descriptive values.
128-128: LGTM! Clean interface extension.
The joinFlowVersion property is properly integrated into the SubmitClientEventOptions interface as an optional field.
Let's verify the usage of this new option:
✅ Verification successful
Property is properly integrated and actively used in the codebase
The verification shows that joinFlowVersion is:
- Correctly defined in the interface
- Actively used in
call-diagnostic-metrics.tsfor client event handling - Has test coverage verifying its functionality with the 'NewFTE' value
- Properly propagated to the client event object when present
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of joinFlowVersion in client events
# Expected: Find references to joinFlowVersion in related files
# Search for direct usage of joinFlowVersion
rg -t typescript "joinFlowVersion" --no-filename
# Search for SubmitClientEventOptions usage to ensure proper typing
ast-grep --pattern 'SubmitClientEventOptions'
Length of output: 337
Script:
#!/bin/bash
# Let's try with ripgrep's default behavior for text files and check the actual content
rg "joinFlowVersion" -A 3 -B 3
# Also check the files that import SubmitClientEventOptions to see their usage
rg "import.*SubmitClientEventOptions" -A 5
Length of output: 4657
packages/@webex/internal-plugin-metrics/src/call-diagnostic/call-diagnostic-metrics.ts (1)
722-724: LGTM! Conditional addition of joinFlowVersion
The implementation correctly adds the joinFlowVersion to the clientEventObject only when it's provided, maintaining backward compatibility with existing calls.
|
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 (1)
packages/@webex/internal-plugin-metrics/test/unit/spec/call-diagnostic/call-diagnostic-metrics.ts (1)
2761-2778: Test case needs additional assertionsThe test case for
joinFlowVersiononly verifies the presence of the field but doesn't validate the complete event payload structure.Consider adding assertions to verify:
- The complete event payload structure remains valid
- Edge cases like undefined/null joinFlowVersion
- Invalid joinFlowVersion values
Example:
it('includes expected joinFlowVersion', async () => { const options = { meetingId: fakeMeeting.id, preLoginId: 'myPreLoginId', joinFlowVersion: 'NewFTE' }; const fetchOptions = await cd.buildClientEventFetchRequestOptions({ name: 'client.exit.app', payload: {trigger: 'user-interaction', canProceed: false}, options, }); // Verify joinFlowVersion assert.equal( fetchOptions.body.metrics[0].eventPayload.event.joinFlowVersion, options.joinFlowVersion ); // Verify payload structure remains valid assert.hasAllKeys(fetchOptions.body.metrics[0].eventPayload.event, [ 'canProceed', 'eventData', 'identifiers', 'loginType', 'name', 'trigger', 'joinFlowVersion' ]); }); it('handles undefined joinFlowVersion', async () => { const options = { meetingId: fakeMeeting.id, preLoginId: 'myPreLoginId' }; const fetchOptions = await cd.buildClientEventFetchRequestOptions({ name: 'client.exit.app', payload: {trigger: 'user-interaction', canProceed: false}, options, }); assert.isUndefined( fetchOptions.body.metrics[0].eventPayload.event.joinFlowVersion ); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/@webex/internal-plugin-metrics/test/unit/spec/call-diagnostic/call-diagnostic-metrics.ts(11 hunks)
...ges/@webex/internal-plugin-metrics/test/unit/spec/call-diagnostic/call-diagnostic-metrics.ts
Outdated
Show resolved
Hide resolved
...ges/@webex/internal-plugin-metrics/test/unit/spec/call-diagnostic/call-diagnostic-metrics.ts
Outdated
Show resolved
Hide resolved
| @@ -718,6 +719,10 @@ export default class CallDiagnosticMetrics extends StatelessWebexPlugin { | |||
| webexSubServiceType: this.getSubServiceType(meeting), | |||
| }; | |||
|
|
|||
| if (joinFlowVersion) { | |||
There was a problem hiding this comment.
doesn't this need to be added to the preMeeting function and all the other functions in this file? similar to sessionCorrelationId?
There was a problem hiding this comment.
just preMeeting function, I think. Good catch. Added.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/@webex/internal-plugin-metrics/test/unit/spec/call-diagnostic/call-diagnostic-metrics.ts (1)
2760-2779: LGTM - Test coverage for joinFlowVersion, consider additional test casesGood test coverage for the new joinFlowVersion feature. The test properly validates that the joinFlowVersion is included in the metrics payload.
Consider adding test cases for:
- Invalid joinFlowVersion values
- Missing joinFlowVersion
- Edge cases in the joinFlowVersion handling
Example additional test:
it('handles missing joinFlowVersion gracefully', async () => { const options = { meetingId: fakeMeeting.id, preLoginId: 'myPreLoginId' }; const fetchOptions = await cd.buildClientEventFetchRequestOptions({ name: 'client.exit.app', payload: {trigger: 'user-interaction', canProceed: false}, options, }); assert.isUndefined( fetchOptions.body.metrics[0].eventPayload.event.joinFlowVersion ); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/@webex/internal-plugin-metrics/test/unit/spec/call-diagnostic/call-diagnostic-metrics.ts(11 hunks)
🔇 Additional comments (3)
packages/@webex/internal-plugin-metrics/test/unit/spec/call-diagnostic/call-diagnostic-metrics.ts (3)
59-59: LGTM - Comma addition for object property
1078-1078: LGTM - Enhanced test coverage for sessionCorrelationId
The changes appropriately add test coverage for sessionCorrelationId handling in different scenarios.
Also applies to: 1172-1172, 1185-1185
1465-1465: LGTM - Enhanced error reporting
Good addition of errorName to errorData for more detailed error reporting.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/@webex/internal-plugin-metrics/test/unit/spec/call-diagnostic/call-diagnostic-metrics.ts (1)
2759-2800: Consider adding edge case tests for joinFlowVersion.While the current test coverage is good, consider adding tests for:
- Invalid/unexpected joinFlowVersion values
- Undefined/null joinFlowVersion scenarios
- Interaction with other options
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/@webex/internal-plugin-metrics/src/call-diagnostic/call-diagnostic-metrics.ts(2 hunks)packages/@webex/internal-plugin-metrics/test/unit/spec/call-diagnostic/call-diagnostic-metrics.ts(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/@webex/internal-plugin-metrics/src/call-diagnostic/call-diagnostic-metrics.ts
🔇 Additional comments (3)
packages/@webex/internal-plugin-metrics/test/unit/spec/call-diagnostic/call-diagnostic-metrics.ts (3)
2759-2800: LGTM! The new tests for joinFlowVersion look good.
The added test cases thoroughly verify the joinFlowVersion functionality:
- Tests the in-meeting scenario by verifying joinFlowVersion is included when meetingId is present
- Tests the pre-join scenario by verifying joinFlowVersion is included when only correlationId is present
- Follows the existing test patterns and assertions
The test coverage is comprehensive and the implementation is correct.
1077-1077: LGTM! The sessionCorrelationId additions are consistent.
The changes correctly add sessionCorrelationId to the test options objects across different test cases, maintaining consistency with the existing test patterns.
Also applies to: 1171-1171, 1184-1184, 1295-1295
1464-1464: LGTM! The errorName additions are consistent.
The changes consistently add errorName to the error data objects across different test cases for error handling.
Also applies to: 1538-1538, 2253-2253
COMPLETES #https://jira-eng-gpk2.cisco.com/jira/browse/SPARK-576382
This pull request addresses
Added new joinFlowVersion CA event option that is needed for the consolidated interstitial feature.
https://sqbu-github.cisco.com/WebExSquared/event-dictionary/pull/1854
by making the following changes
MetricEventJoinFlowVersiontypecreateClientEventObjectInMeetingChange Type
The following scenarios were tested
Unit tests
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
joinFlowVersionfor enhanced event tracking in call diagnostics.MetricEventJoinFlowVersionto improve event categorization.submitClientEventmethod.Bug Fixes
Documentation