Meeting joined time convert #4185
Conversation
📝 WalkthroughWalkthroughThis pull request updates the meeting diagnostics and related meeting modules by introducing 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. 🔧 ast-grep (0.31.1)packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js🔧 ESLint
yarn install v1.22.22 (For a CapTP with native promises, see @endo/eventual-send and @endo/captp) ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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
|
![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 (2)
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (2)
7531-7535: Avoid potential test flakiness due to exact time comparison.
Usingnew Date().toISOString()inline may cause rare mismatches if the test and the setter occur a few milliseconds apart. Consider stubbing or mockingDateto always return a fixed value during tests.it('should fallback to system clock ISO string when given an undefined value', () => { - const currentSystemTime = new Date().toISOString(); + const fixedTime = '2025-04-01T13:00:36.000Z'; // example + sandbox.stub(Date.prototype, 'toISOString').returns(fixedTime); meeting.isoLocalClientMeetingJoinTime = undefined; - assert.equal(meeting.isoLocalClientMeetingJoinTime, currentSystemTime); + assert.equal(meeting.isoLocalClientMeetingJoinTime, fixedTime); });
7537-7541: Same time comparison issue for invalid date test.
This scenario faces the same potential risk of test flakiness as the undefined-value test. Mocking or stubbingDatehelps ensure predictable results.
📜 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(3 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)
🧰 Additional context used
🧬 Code Definitions (2)
packages/@webex/internal-plugin-metrics/src/call-diagnostic/call-diagnostic-metrics.ts (1)
docs/samples/browser-plugin-meetings/app.js (15)
meeting(441-441)meeting(460-460)meeting(490-490)meeting(616-616)meeting(753-753)meeting(1007-1007)meeting(1029-1029)meeting(1051-1051)meeting(1087-1087)meeting(1100-1100)meeting(1113-1113)meeting(1149-1149)meeting(1196-1196)meeting(1218-1218)meeting(1240-1240)
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (1)
packages/@webex/plugin-meetings/src/meeting/index.ts (2)
isoLocalClientMeetingJoinTime(1685-1687)isoLocalClientMeetingJoinTime(1696-1714)
🔇 Additional comments (9)
packages/@webex/internal-plugin-metrics/src/call-diagnostic/call-diagnostic-metrics.ts (1)
739-742: LGTM: Properly adding the meeting joined time to the client event object.This implementation correctly adds the
meetingJoinedTimeto the client event object when available, which enhances the metrics by tracking when users join meetings.packages/@webex/plugin-meetings/src/meeting/util.ts (1)
183-183: LGTM: Properly capturing meeting join time from response headers.This implementation captures the meeting join time from the response headers, correctly providing a timestamp from the server. The comment indicates that there's a fallback to system clock when the header is not available, which is likely handled by the setter method in the Meeting class.
packages/@webex/plugin-meetings/test/unit/spec/meeting/utils.js (1)
464-505: LGTM: Good test coverage for the new functionality.This test properly verifies that the
isoLocalClientMeetingJoinTimeproperty is correctly set from the response headers when joining a meeting. The test setup is thorough and covers the essential functionality.packages/@webex/internal-plugin-metrics/test/unit/spec/call-diagnostic/call-diagnostic-metrics.ts (3)
64-68: Good addition of the test fixture with isoLocalClientMeetingJoinTime property.The fakeMeeting4 object properly includes the new property that this PR is implementing, which will allow testing how the system handles the new ISO format meeting joined time.
82-82: LGTM! Properly added to the fakeMeetings collection.This ensures the new test fixture is available for lookup by ID in tests.
1089-1225: Well-structured test for the new meeting joined time functionality.This test effectively verifies that when a meeting has an
isoLocalClientMeetingJoinTimeproperty, it is correctly:
- Passed as
meetingJoinedTimeto theprepareDiagnosticEventmethod- Included in the submitted event object
- Used consistently throughout the event processing pipeline
The test follows the established pattern of the other tests in this file and provides thorough coverage of the new functionality.
packages/@webex/plugin-meetings/src/meeting/index.ts (1)
1689-1714: Well-implemented setter with proper validation and fallback.The new
isoLocalClientMeetingJoinTimesetter properly handles conversion of meeting joined time to ISO format with appropriate validation and fallback logic. If the provided time is undefined or invalid, it correctly falls back to the current system time, logs the issue, and ensures the property is always set as a valid ISO string.This implementation aligns well with the PR objectives of converting the meeting joined time from a locus date string to an ISO format, which will help ensure consistent timestamp formatting throughout the application.
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (2)
213-213: Variable naming and usage appear consistent.
A new variableisoLocalClientMeetingJoinTimeis introduced for test setup. This aligns with theMeetingclass property and ensures clarity in the tests.
7543-7548: Comprehensive check for valid date strings.
Testing with a valid date string is important. This test thoroughly verifies that the setter correctly normalizes the provided date to ISO format.
3 participants
This pull request addresses
fixes meeting joined time to convert the date string from locus to an ISO format
by making the following changes
this is the only thing that changed vs the revert
validated live with @Coread
Change Type
The following scenarios were tested
manual test and verify with correlationID
update unit tests
I certified that
Make sure to have followed the contributing guidelines before submitting.