fix(meetings): Meeting joined time header #4124
Conversation
📝 WalkthroughWalkthroughThis update introduces a new property, Possibly related PRs
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
yarn install v1.22.22 (For a CapTP with native promises, see @endo/eventual-send and @endo/captp) ✨ 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
|
chrisadubois
added
the
validated
arun3528
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/@webex/plugin-meetings/src/meeting/util.ts (1)
183-183: Implementation looks good, consider adding fallback handling.The code correctly retrieves the meeting join time from the response headers using safe optional chaining. This aligns well with the PR objectives to record when a meeting is joined.
While the comment mentions falling back to the system clock if the header doesn't exist, the fallback implementation isn't visible in this line. Consider either:
- Adding explicit fallback here:
-meeting.isoLocalClientMeetingJoinTime = res?.headers?.date; // read from header if exist, else fall back to system clock : https://jira-eng-gpk2.cisco.com/jira/browse/SPARK-555657 +meeting.isoLocalClientMeetingJoinTime = res?.headers?.date || new Date().toISOString(); // read from header if exist, else fall back to system clock : https://jira-eng-gpk2.cisco.com/jira/browse/SPARK-555657
- Or confirming the fallback is handled in the setter method for
isoLocalClientMeetingJoinTimein the Meeting class.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/@webex/internal-plugin-metrics/src/call-diagnostic/call-diagnostic-metrics.ts(1 hunks)packages/@webex/internal-plugin-metrics/test/unit/spec/call-diagnostic/call-diagnostic-metrics.ts(2 hunks)packages/@webex/plugin-meetings/src/meeting/index.ts(1 hunks)packages/@webex/plugin-meetings/src/meeting/util.ts(1 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js(2 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/utils.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Packages
- GitHub Check: AWS Amplify Console Web Preview
🔇 Additional comments (7)
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (1)
209-209: New test variable for meeting join time tracking.Adding the
isoLocalClientMeetingJoinTimevariable to capture the join time aligns well with the PR objective of reading joined time from headers.packages/@webex/internal-plugin-metrics/src/call-diagnostic/call-diagnostic-metrics.ts (1)
739-742: Addition of meeting joined time to client event object looks good.This change correctly implements the feature for recording the time a meeting is joined by adding the
meetingJoinedTimeproperty to the client event object when available. The implementation properly checks if the value exists before assigning it.packages/@webex/plugin-meetings/test/unit/spec/meeting/utils.js (1)
464-505: Good test coverage for the meeting join time header feature.This test case properly verifies that the
isoLocalClientMeetingJoinTimeproperty is correctly set from the date header in the response when joining a meeting. The test adequately sets up the necessary mocks and assertions to validate this functionality.packages/@webex/plugin-meetings/src/meeting/index.ts (1)
1637-1650: Well-implemented setter method for meeting join time tracking.The new setter method for
isoLocalClientMeetingJoinTimecorrectly implements the logic to record meeting join time. It ensures the value is set only once (the first time it's called) and handles both cases appropriately - using the provided timestamp if one exists, or generating a new ISO timestamp if none is provided.This implementation aligns perfectly with the PR objective to add functionality for recording meeting join time, reading from the header if it exists or using the system clock as a fallback.
packages/@webex/internal-plugin-metrics/test/unit/spec/call-diagnostic/call-diagnostic-metrics.ts (3)
61-65: Good addition of test fixture for meeting join time.The new
fakeMeeting4test fixture correctly extends the base meeting object while adding the newisoLocalClientMeetingJoinTimeproperty, which aligns with the PR's purpose of tracking meeting join times.
71-71: LGTM: Properly registered the new test fixture.The new test fixture is correctly added to the
fakeMeetingsobject, making it accessible for tests.
1071-1204: Well-structured test case for meeting join time functionality.This test case thoroughly validates that the
submitClientEventmethod correctly incorporates theisoLocalClientMeetingJoinTimeproperty from the meeting object into the event payload asmeetingJoinedTime. The test covers all necessary assertions:
- Verification that the correct identifiers are fetched from the meeting
- Confirmation that the
meetingJoinedTimeproperty is set correctly in the diagnostic event- Validation of the full call chain with appropriate spy checks
The test case aligns perfectly with the PR objective of adding functionality to record meeting join times.
| describe('#setIsoLocalClientMeetingJoinTime', () => { | ||
| it('should set the isoLocalClientMeetingJoinTime once and only once when passed in', () => { | ||
| assert.equal(meeting.isoLocalClientMeetingJoinTime, isoLocalClientMeetingJoinTime); | ||
| meeting.isoLocalClientMeetingJoinTime = 'test'; | ||
| assert.equal(meeting.isoLocalClientMeetingJoinTime, 'test'); | ||
| meeting.isoLocalClientMeetingJoinTime = 'test2'; | ||
| assert.equal(meeting.isoLocalClientMeetingJoinTime, 'test'); | ||
| }); | ||
|
|
||
| it('should set the isoLocalClientMeetingJoin time once and only once when not passed in', () => { | ||
| assert.equal(meeting.isoLocalClientMeetingJoinTime, isoLocalClientMeetingJoinTime); | ||
| meeting.isoLocalClientMeetingJoinTime = undefined; | ||
| const time = meeting.isoLocalClientMeetingJoinTime; | ||
| assert.equal(meeting.isoLocalClientMeetingJoinTime, time); | ||
| meeting.isoLocalClientMeetingJoinTime = 'test2'; | ||
| assert.equal(meeting.isoLocalClientMeetingJoinTime, time); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Good test coverage for the new setter behavior.
The tests correctly verify that the isoLocalClientMeetingJoinTime property can only be set once, which is an important safeguard to prevent accidental overwrites of the join time. This matches the PR objective of reliably tracking when a meeting is joined.
However, I notice a few points that could be improved:
-
The second test doesn't explicitly verify what happens when
undefinedis set - it just stores the result in a variable and checks that it can't be changed after. Consider asserting the expected behavior more clearly. -
There's no test for the fallback to system clock mentioned in the PR objective when no header exists.
🏁 Script executed:
#!/bin/bash
# Check if the Meeting class implementation properly handles the fallback to system clock
rg -A 5 "isoLocalClientMeetingJoinTime" --type ts packages/@webex/plugin-meetings/src/meeting/index.tsLength of output: 1274
Action Required: Enhance Test Assertions for Fallback Behavior
Good test coverage overall for ensuring the setter only takes effect once. However, please consider the following improvements:
- Explicit Fallback Verification: The second test currently stores the fallback value in a variable and only checks for immutability after subsequent assignment. It would be more robust to assert that when
undefinedis passed, the actual fallback value meets the expected criteria (e.g., matching an ISO 8601 format). - Dedicated Fallback Test: Introduce a separate test case to explicitly verify that when no header value is provided, the setter correctly falls back to using the system clock (i.e., by asserting that
new Date().toISOString()is effectively returned).
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
|
Waiting for response from CA and Locus on Date header |
| * @param {string | undefined} isoLocalClientMeetingJoinTime | ||
| */ | ||
| set isoLocalClientMeetingJoinTime(isoLocalClientMeetingJoinTime: string | undefined) { | ||
| if (!this.#isoLocalClientMeetingJoinTime) { |
There was a problem hiding this comment.
question (just to double check): in a scenario where user joins, leaves and then joins the same meeting again, are we supposed to still use the fist meeting join time and not the 2nd one?
| @@ -180,7 +180,7 @@ const MeetingUtil = { | |||
| .then((res) => { | |||
| const parsed = MeetingUtil.parseLocusJoin(res); | |||
| meeting.setLocus(parsed); | |||
|
|
|||
| meeting.isoLocalClientMeetingJoinTime = res?.headers?.date; // read from header if exist, else fall back to system clock : https://jira-eng-gpk2.cisco.com/jira/browse/SPARK-555657 | |||
There was a problem hiding this comment.
wouldn't it be better to add this to the common code that's always executed when any Locus is parsed? I think we are not guaranteed to have the join time in the http response here, it might come in a delta DTO afterwards. Actually if we always parse it out of Locus DTO then we would end up setting it also in the case when we just load the page and Locus thinks we're already in the meeting, because user was in a meeting and just did a page refresh, so maybe it is better to keep it here as it is and my comment is useless - just making this comment to make sure that all these scenarios were considered.
Also just wondering if there are any other means of ending up in the meeting where we should set the joined time, like maybe for example Meeting.moveFrom()?
COMPLETES https://jira-eng-gpk2.cisco.com/jira/browse/SPARK-555657
This pull request addresses
add meeting joined time, read from header if exists, and set to system clock if not. Apply to CA metrics thereafter
by making the following changes
Change Type
The following scenarios were tested
manual,
unit
I certified that
Make sure to have followed the contributing guidelines before submitting.