fix(plugin-meetings): clear sdp timer on answer processing failure #4196
Conversation
📝 WalkthroughWalkthroughThe pull request modifies the error handling logic in the 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
🪧 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)
8698-8701: Consider assigning a real timer ID
Storing'1234'as the timer ID can make the test scenario less realistic. Instead, you can create an actual timer viaclock.setTimeout(...)and use the returned handle to confirm that the correct timer is being cleared.Here is a possible diff to illustrate the change:
- meeting.sdpResponseTimer = '1234'; + meeting.sdpResponseTimer = clock.setTimeout(() => {}, 100);Additionally, consider verifying the timer was cleared using the exact ID or variable holding that handle.
8716-8716: Enhance the assertion to verify the cleared timer
Currently, the test checks only ifclearTimeoutwas called once. For more robust test coverage, you can also verify that the exact timer ID stored inmeeting.sdpResponseTimerwas cleared:assert.calledOnce(clearTimeoutSpy); +assert.calledWith(clearTimeoutSpy, meeting.sdpResponseTimer);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/@webex/plugin-meetings/src/meeting/index.ts(1 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: AWS Amplify Console Web Preview
🔇 Additional comments (2)
packages/@webex/plugin-meetings/src/meeting/index.ts (1)
6209-6216: Good fix for resource leak prevention!This change properly clears the SDP response timer when encountering SDP offer or answer handling errors. Without this fix, the timer could remain active even after error handling, potentially leading to unexpected behavior or memory leaks. The implementation correctly checks if the timer exists before clearing it and follows the same pattern used elsewhere in the codebase.
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (1)
8617-8628: Correct usage of sinon fake timers
Usingsinon.useFakeTimers()inbeforeEachand restoring the clock inafterEachis a good practice to isolate test timers from real time. This approach helps ensure that time-dependent logic is fully controlled and predictable within your tests.
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (1)
8698-8704: Consider using a more realistic timer ID
Assigning'1234'tomeeting.sdpResponseTimerworks for verifying thatclearTimeoutis called, but you might store an actual numeric handle for clarity.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/@webex/plugin-meetings/src/meeting/index.ts(2 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js(3 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
packages/@webex/plugin-meetings/src/meeting/index.ts (3)
packages/@webex/plugin-meetings/src/meetings/index.ts (1)
error(81-83)packages/@webex/plugin-meetings/src/metrics/index.ts (1)
sendBehavioralMetric(61-67)packages/@webex/plugin-meetings/src/metrics/constants.ts (1)
BEHAVIORAL_METRICS(81-81)
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (1)
packages/@webex/internal-plugin-metrics/src/metrics.js (1)
Metrics(37-149)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Test - Unit
- GitHub Check: Test - Integration
- GitHub Check: Test - Style
- GitHub Check: Test - Browser
- GitHub Check: AWS Amplify Console Web Preview
🔇 Additional comments (3)
packages/@webex/plugin-meetings/src/meeting/index.ts (1)
6208-6237: Looks good! Properly handles SDP answer timer cleanup on errors.The changes properly separate the handling of
SdpOfferHandlingErrorandSdpAnswerHandlingErrorinto distinct conditional blocks. Now when anSdpAnswerHandlingErroroccurs, the code correctly clears the SDP response timer and rejects the deferred promise, preventing the meeting from being incorrectly marked as missing a remote answer.One minor suggestion would be to pass the error to the reject call:
- this.deferSDPAnswer.reject(); + this.deferSDPAnswer.reject(error);This would provide more context to the caller about why the promise was rejected.
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (2)
8617-8617: Confirm correct usage of faked timers
You're properly setting upsinon.useFakeTimers()and restoring the clock inafterEach, which helps avoid test pollution and ensures consistent timer behavior across tests.Also applies to: 8620-8620, 8626-8628
8719-8720: Good assertion coverage
Verifying that bothmeeting.deferSDPAnswer.rejectandclearTimeoutSpyare called once effectively tests the error handling logic. Nicely done!
3 participants
This pull request addresses
This PR resolves an issue which causes meeting to be marked as missing remote answer, even though answer was received.
It can happen, when SDP Answer processing failed from any reason:
by making the following changes
Changes in this PR will ensure that sdpAnswerTimer will be cleared up not only in successful scenarios, but also in failure cases.
Change Type
The following scenarios were tested
Tested with JS-SDK Sample 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.