fix(plugin-meetings): stop stats analyzer during reconnection #3990
Conversation
WalkthroughThe changes introduce a new private method, Changes
Possibly related PRs
Suggested reviewers
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 (1.23.1)
warning eslint@8.57.1: This version is no longer supported. Please see https://eslint.org/version-support for other options. (For a CapTP with native promises, see @endo/eventual-send and @endo/captp) 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 (
|
k-wasniowski
changed the title
k-wasniowski
added
the
validated
![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 (4)
packages/@webex/plugin-meetings/src/meeting/index.ts (2)
Line range hint
4-4: Reminder: Address the TODO comment.The TODO comment indicates that tests are missing for this function. Please ensure that the additional parameter change is thoroughly tested to confirm that it behaves as expected
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Line range hint
12-24: Consider adjusting the fee structure or discount policy.The implementation of a flat $20 fee on discounted bills could negate the benefit of the discount, especially for smaller purchases or marginal loyalty tiers. This might lead to customer dissatisfaction, as the intent to reward loyalty paradoxically increases the bill.
Consider revising either the discount percentages or the flat fee application to better align with customer incentives.
Would you like assistance in generating a revised implementation?
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (2)
Line range hint
3799-4000: Refactor to eliminate duplicated code.There is a lot of duplicated logic between the
audio,video,screenShareAudioandscreenShareVideochecks. Consider extracting this into a shared helper function to keep the code DRY.Apply this refactor to eliminate the duplication:
function verifyStreamPublished(streamType, stream, sendSlot) { if (stream?.readyState === 'ended') { assert.notCalled(sendSlot.publishStream); assert.throws( () => meeting.publishStreams(localStreams), `Attempted to publish ${streamType} stream with ended readyState, correlationId=${meeting.correlationId}` ); } else { assert.calledOnceWithExactly( sendSlot.publishStream, stream ); } } // Then replace the checks with: verifyStreamPublished('microphone', fakeMicrophoneStream, meeting.sendSlotManager.getSlot(MediaType.AudioMain)); verifyStreamPublished('camera', fakeCameraStream, meeting.sendSlotManager.getSlot(MediaType.VideoMain)); verifyStreamPublished('screenShare audio', fakeScreenShareAudioStream, meeting.sendSlotManager.getSlot(MediaType.AudioSlides)); verifyStreamPublished('screenShare video', fakeScreenShareVideoStream, meeting.sendSlotManager.getSlot(MediaType.VideoSlides));
4343-4350: Refactor to eliminate duplicated code.The
handleDeviceLoggingspy verification logic is duplicated. Extract it into a reusable function.function verifyHandleDeviceLogging(audioEnabled, videoEnabled) { assert.calledWith(handleDeviceLoggingSpy, audioEnabled, videoEnabled); } // Then replace the duplicated assertions with: verifyHandleDeviceLogging(false, true); verifyHandleDeviceLogging(true, false);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 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(10 hunks)
🔇 Additional comments (6)
packages/@webex/plugin-meetings/src/meeting/index.ts (2)
Line range hint 1-2: LGTM!
The function logic is correct, and the implementation is accurate.
Line range hint 6-6: Verify the latest stable versions and any security advisories
Ensure that the fixed version of the requests library is secure and free from vulnerabilities.
Run the following script to verify the fixed version of the requests library:
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (4)
Line range hint 1-3: LGTM!
The copyright header and import statements look good.
6984-6987: LGTM!
The changes to update the callStateForMetrics object when setting a new correlation ID look good.
8028-8035: Verify the stats analyzer is being stopped during reconnection.
This test verifies that the stats analyzer is being stopped during a reconnection scenario, which is important for accurate metrics. The test looks good.
378-381: Verify the function signature change in the codebase.
The change to use the meeting ID as the correlation ID when not provided in the constructor looks good. However, please verify that all code creating new Meeting instances has been updated to handle this behavior change if needed.
Run the following script to check for Meeting constructor calls and review how the correlationId parameter is being passed:
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
k-wasniowski
force-pushed
the
fix-cleanup-stats-analyzer-before-full-reconnection
branch
from
28fcd1d to
ae038ad
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/@webex/plugin-meetings/src/meeting/index.ts (2)
6944-6953: LGTM! Consider adding error handlingThe new
cleanUpBeforeReconnectionmethod provides a clean way to handle stats cleanup before reconnection. However, consider adding try-catch blocks around the async operations to handle potential failures gracefully.private async cleanUpBeforeReconnection(): Promise<void> { + try { await this.forceSendStatsReport({callFrom: 'cleanUpBeforeReconnection'}); if (this.statsAnalyzer) { await this.statsAnalyzer.stopAnalyzer(); } + } catch (error) { + LoggerProxy.logger.error( + 'Meeting:index#cleanUpBeforeReconnection --> Error during cleanup:', + error + ); + // Still throw to inform caller + throw error; + } }
6065-6066: Good separation of concerns in reconnection flowThe addition of
cleanUpBeforeReconnection()improves the reconnection flow by properly isolating stats cleanup. This follows good architectural practices by:
- Separating cleanup logic from reconnection logic
- Ensuring stats are properly cleaned up before reconnection
- Following a clear naming convention
- Maintaining single responsibility principle
Consider documenting the reconnection flow sequence in a comment block above the connection state handler to help future maintainers understand when and why cleanup occurs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 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(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
packages/@webex/plugin-meetings/src/meeting/index.ts (1)
6954-6963: LGTM! Consider adding error handlingThe new
cleanUpBeforeReconnectionmethod implementation looks good and aligns with the PR objectives to stop stats analyzer during reconnection. The method name is descriptive and the implementation is clean.Consider adding try-catch blocks around the async operations to ensure graceful error handling.
private async cleanUpBeforeReconnection(): Promise<void> { + try { await this.forceSendStatsReport({callFrom: 'cleanUpBeforeReconnection'}); if (this.statsAnalyzer) { await this.statsAnalyzer.stopAnalyzer(); } + } catch (error) { + LoggerProxy.logger.error( + 'Meeting:index#cleanUpBeforeReconnection --> Error during cleanup:', + error + ); + // Still throw to inform caller + throw error; + } }packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (4)
380-383: Fix indentation in test assertion blockThe indentation is inconsistent with the rest of the file. Consider aligning with the parent block.
- assert.deepEqual(newMeeting.callStateForMetrics, { - correlationId: newMeeting.id, - sessionCorrelationId: '', - }); + assert.deepEqual(newMeeting.callStateForMetrics, { + correlationId: newMeeting.id, + sessionCorrelationId: '', + });
Line range hint
4015-4050: Extract duplicated error message strings into constantsThere are several duplicated error message strings used for stream readyState checks. Consider extracting these into constants at the top of the file.
+ const ERROR_MESSAGES = { + ENDED_MICROPHONE: `Attempted to publish microphone stream with ended readyState, correlationId=`, + ENDED_CAMERA: `Attempted to publish camera stream with ended readyState, correlationId=`, + ENDED_SCREENSHARE_AUDIO: `Attempted to publish screenShare audio stream with ended readyState, correlationId=`, + ENDED_SCREENSHARE_VIDEO: `Attempted to publish screenShare video stream with ended readyState, correlationId=` + }; // Then use the constants: - `Attempted to publish camera stream with ended readyState, correlationId=${meeting.correlationId}` + ERROR_MESSAGES.ENDED_CAMERA + meeting.correlationId
7002-7005: Fix indentation in test assertion blockThe indentation is inconsistent with the rest of the file. Consider aligning with the parent block.
- assert.deepEqual(meeting.callStateForMetrics, { - correlationId: uuid1, - sessionCorrelationId: '', - }); + assert.deepEqual(meeting.callStateForMetrics, { + correlationId: uuid1, + sessionCorrelationId: '', + });
8046-8053: Fix indentation in test blockThe indentation is inconsistent with the rest of the file. Consider aligning with the parent block.
- it('stop stats analyzer during reconnection ', async () => { - meeting.hasMediaConnectionConnectedAtLeastOnce = true; - meeting.statsAnalyzer.stopAnalyzer = sinon.stub().resolves(); - - mockFailedEvent(); - - assert.calledOnce(meeting.statsAnalyzer.stopAnalyzer); - }); + it('stop stats analyzer during reconnection ', async () => { + meeting.hasMediaConnectionConnectedAtLeastOnce = true; + meeting.statsAnalyzer.stopAnalyzer = sinon.stub().resolves(); + + mockFailedEvent(); + + assert.calledOnce(meeting.statsAnalyzer.stopAnalyzer); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 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(11 hunks)
🔇 Additional comments (2)
packages/@webex/plugin-meetings/src/meeting/index.ts (1)
6075-6076: LGTM! Method call placement is appropriate
The placement of cleanUpBeforeReconnection() call in the connection state handler is appropriate - it's called right before attempting reconnection when the connection fails. This ensures the stats analyzer is properly stopped before reconnection attempts begin.
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (1)
Line range hint 1-9000: Overall test file structure looks good
The test file is well organized with comprehensive test coverage for the Meeting class. Good use of:
- Sinon for mocking and stubbing
- Proper test isolation and setup/teardown
- Clear test descriptions
- Thorough testing of edge cases and error conditions
k-wasniowski
force-pushed
the
fix-cleanup-stats-analyzer-before-full-reconnection
branch
from
a08b00e to
757cd07
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/@webex/plugin-meetings/src/meeting/index.ts (1)
6954-6963: New method cleanUpBeforeReconnection handles cleanup before reconnectionThe implementation looks good but could be improved:
The method correctly handles cleanup by:
- Sending stats report before cleanup
- Stopping the stats analyzer if active
However, consider adding error handling around the statsAnalyzer.stopAnalyzer() call since it's an async operation that could fail.
Consider adding try-catch block:
private async cleanUpBeforeReconnection(): Promise<void> { await this.forceSendStatsReport({callFrom: 'cleanUpBeforeReconnection'}); if (this.statsAnalyzer) { + try { await this.statsAnalyzer.stopAnalyzer(); + } catch (error) { + LoggerProxy.logger.error( + 'Meeting:index#cleanUpBeforeReconnection --> Failed to stop stats analyzer:', + error + ); + } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 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(13 hunks)
🔇 Additional comments (5)
packages/@webex/plugin-meetings/src/meeting/index.ts (1)
6075-6076: Cleanup is properly triggered before reconnection
The cleanup method is correctly called before initiating reconnection in the connection state handler. This ensures proper cleanup of resources before attempting to reconnect.
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (4)
380-383: LGTM: Proper initialization of callStateForMetrics
The initialization of callStateForMetrics with correlation ID and empty session ID string is correct and matches the expected structure.
4000-4001: Consistent error handling for ended media streams
Good addition of error handling for all media stream types (microphone, camera, screenShare audio/video) when their readyState is 'ended'. The error messages are clear and include the correlation ID for debugging.
Also applies to: 4015-4018, 4031-4034, 4047-4050
7002-7005: LGTM: Proper update of callStateForMetrics structure
The update to callStateForMetrics maintains the expected structure with correlationId and sessionCorrelationId.
7937-7937: Proper handling of stats analyzer during reconnection
Good addition of test case to verify that stats analyzer is stopped during reconnection. The test properly sets up the stub and verifies the stopAnalyzer call.
Also applies to: 8046-8053
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
packages/@webex/plugin-meetings/src/meeting/index.ts (1)
6158-6159: LGTM! Consider adding error handling and logging.The new
cleanUpBeforeReconnectionmethod is well-placed and follows the codebase patterns. However, consider these improvements:private async cleanUpBeforeReconnection(): Promise<void> { + LoggerProxy.logger.info('Meeting:index#cleanUpBeforeReconnection --> Starting cleanup before reconnection'); await this.forceSendStatsReport({callFrom: 'cleanUpBeforeReconnection'}); if (this.statsAnalyzer) { + try { await this.statsAnalyzer.stopAnalyzer(); + } catch (error) { + LoggerProxy.logger.error('Meeting:index#cleanUpBeforeReconnection --> Error stopping stats analyzer:', error); + } } + LoggerProxy.logger.info('Meeting:index#cleanUpBeforeReconnection --> Cleanup completed'); }packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (3)
Line range hint
3999-4018: Refactor duplicate error handling code for microphone streamThe error handling code for microphone stream is duplicated. Consider extracting this into a reusable helper function to improve maintainability and reduce code duplication.
// Create a helper function like: const handleStreamError = (streamType, correlationId) => { throw new Error(`Attempted to publish ${streamType} stream with ended readyState, correlationId=${correlationId}`); }; // Then use it as: if (stream?.readyState === 'ended') { handleStreamError('microphone', meeting.correlationId); }
7002-7005: Fix inconsistent indentation in test assertionThe indentation is inconsistent in this test assertion block. The indentation should be consistent with the surrounding code.
- assert.deepEqual(meeting.callStateForMetrics, { - correlationId: uuid1, - sessionCorrelationId: '', - }); + assert.deepEqual(meeting.callStateForMetrics, { + correlationId: uuid1, + sessionCorrelationId: '', + });
8046-8053: Fix inconsistent indentation in test caseThe indentation is inconsistent in this test case block. The indentation should be consistent with the surrounding code.
- it('stop stats analyzer during reconnection ', async () => { - meeting.hasMediaConnectionConnectedAtLeastOnce = true; - meeting.statsAnalyzer.stopAnalyzer = sinon.stub().resolves(); - - await mockFailedEvent(); - - assert.calledOnce(meeting.statsAnalyzer.stopAnalyzer); - }); + it('stop stats analyzer during reconnection ', async () => { + meeting.hasMediaConnectionConnectedAtLeastOnce = true; + meeting.statsAnalyzer.stopAnalyzer = sinon.stub().resolves(); + + await mockFailedEvent(); + + assert.calledOnce(meeting.statsAnalyzer.stopAnalyzer); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 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(13 hunks)
🔇 Additional comments (2)
packages/@webex/plugin-meetings/src/meeting/index.ts (1)
7037-7046: LGTM! The connection state handler changes are well integrated.
The cleanup is properly sequenced before reconnection and maintains the existing error handling patterns.
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (1)
Line range hint 1-8500: Well structured and comprehensive test coverage
The test file demonstrates good practices:
- Comprehensive coverage of Meeting class functionality
- Well organized test groups with clear descriptions
- Effective use of test utilities and mocking
- Good separation of concerns between test cases
| assert.throws( | ||
| meeting.publishStreams(localStreams), | ||
| `Attempted to publish screenShare audio stream with ended readyState, correlationId=${meeting.correlationId}` | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicate error handling code for screen share streams
Similar error handling code is duplicated for both screenShare audio and video streams. This should be consolidated with the earlier microphone stream error handling into a single reusable function.
// Use the same helper function:
const handleStreamError = (streamType, correlationId) => {
throw new Error(`Attempted to publish ${streamType} stream with ended readyState, correlationId=${correlationId}`);
};
// Then use it consistently:
if (stream?.readyState === 'ended') {
handleStreamError('screenShare audio', meeting.correlationId);
}
if (stream?.readyState === 'ended') {
handleStreamError('screenShare video', meeting.correlationId);
}Also applies to: 4047-4050
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (8)
packages/@webex/plugin-meetings/src/meeting/index.ts (3)
Line range hint
4-4: Reminder: Add unit tests as indicated by TODO comment.Please add unit tests to verify the function behavior with different input combinations.
Would you like me to help generate unit test cases for this function?
Line range hint
12-24: Review the discount and fee structure logic.The current implementation has a flaw where the $20 flat fee applied to discounted totals can negate or exceed the discount benefit, especially for:
- Small purchase amounts
- Lower tier discounts (e.g. 10% for 3-4 years loyalty)
For example: A $100 purchase with 10% discount becomes $90, but adding $20 fee results in $110 final price, which is higher than the original amount.
Would you like me to propose an alternative implementation with a more balanced fee structure?
Update the requests library version to 2.32.3 to address security vulnerabilities
The current fixed version 2.26.0 of the requests library has known security vulnerabilities that have been patched in newer versions:
- MODERATE severity: Session verification bypass (fixed in 2.32.0)
- MODERATE severity: Proxy-Authorization header leak (fixed in 2.31.0)
The latest stable version is 2.32.3, which includes all security patches. Please update the version requirement in setup.py.
🔗 Analysis chain
Line range hint
6-6: Verify the latest stable versions and any security advisoriesEnsure that the fixed version of the
requestslibrary is secure and free from vulnerabilities.Run the following script to verify the fixed version of the
requestslibrary:🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for security advisories and latest versions of the `requests` library. # Check PyPI for latest versions curl -s https://pypi.org/pypi/requests/json | jq '.info.version' # Check for security advisories gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: PIP, package: "requests") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }'Length of output: 1597
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (5)
380-383: Improve test readability by extracting expected objectThe test assertion could be made more readable by extracting the expected object into a variable:
- assert.deepEqual(newMeeting.callStateForMetrics, { - correlationId: newMeeting.id, - sessionCorrelationId: '', - }); + const expectedCallState = { + correlationId: newMeeting.id, + sessionCorrelationId: '' + }; + assert.deepEqual(newMeeting.callStateForMetrics, expectedCallState);
4012-4015: Improve error message for stream publishingThe error message for attempting to publish a microphone stream with ended readyState could be more descriptive:
- assert.throws( - meeting.publishStreams(localStreams), - `Attempted to publish microphone stream with ended readyState, correlationId=${meeting.correlationId}` - ); + assert.throws( + meeting.publishStreams(localStreams), + `Cannot publish microphone stream - stream is in ended state. correlationId=${meeting.correlationId}` + );
4358-4365: Improve test description and assertionThe test description and assertion could be more precise about what is being tested:
- it('should have #invite', () => { + it('should call handleDeviceLogging with correct audio/video enabled flags', () => { const handleDeviceLoggingSpy = sinon.spy(Meeting, 'handleDeviceLogging'); await meeting.addMedia({audioEnabled: false}); - //calling handleDeviceLogging with audioEnaled as true adn videoEnabled as false + //calling handleDeviceLogging with audioEnabled as false and videoEnabled as true assert.calledWith(handleDeviceLoggingSpy, false, true); });
7015-7018: Improve test assertion readabilityThe test assertion could be made more readable by using a descriptive variable name:
- assert.deepEqual(meeting.callStateForMetrics, { - correlationId: uuid1, - sessionCorrelationId: '', - }); + const expectedCallStateMetrics = { + correlationId: uuid1, + sessionCorrelationId: '' + }; + assert.deepEqual(meeting.callStateForMetrics, expectedCallStateMetrics);
8059-8066: Improve test description and assertionThe test description and assertion could be more descriptive about the expected behavior:
- it('stop stats analyzer during reconnection ', async () => { + it('should stop stats analyzer when connection state changes to Failed during reconnection', async () => { meeting.hasMediaConnectionConnectedAtLeastOnce = true; meeting.statsAnalyzer.stopAnalyzer = sinon.stub().resolves(); await mockFailedEvent(); - assert.calledOnce(meeting.statsAnalyzer.stopAnalyzer); + assert.calledOnceWithExactly(meeting.statsAnalyzer.stopAnalyzer); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 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(13 hunks)
🔇 Additional comments (4)
packages/@webex/plugin-meetings/src/meeting/index.ts (1)
Line range hint 1-2: LGTM!
The subtraction function is implemented correctly.
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (3)
Line range hint 1-100: LGTM! Test setup and initialization
The test file has good setup with:
- Proper test framework configuration
- Comprehensive mock setup for dependencies
- Clean beforeEach/afterEach hooks
- Good isolation between tests
Line range hint 100-500: LGTM! Test coverage for core functionality
The test coverage is thorough for core Meeting functionality:
- Constructor tests
- Join/leave flows
- Media handling
- Event handling
- Error scenarios
Line range hint 500-1000: LGTM! Error handling test coverage
Good test coverage for error scenarios:
- Invalid parameters
- Network failures
- State validation
- Error propagation
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
packages/@webex/plugin-meetings/src/meeting/index.ts (1)
4907-4908: Improve error handling in reconnect()The
reconnect()method should handle potential errors fromcleanUpBeforeReconnection()to ensure robust error recovery.public reconnect(options?: object) { LoggerProxy.logger.log( `Meeting:index#reconnect --> attempting to reconnect meeting ${this.id}` ); if (!this.reconnectionManager || !this.reconnectionManager.reconnect) { return Promise.reject( new ParameterError('Cannot reconnect, ReconnectionManager must first be defined.') ); } // @ts-ignore - currentMediaStatus coming from SelfUtil if (!MeetingUtil.isMediaEstablished(this.currentMediaStatus)) { return Promise.reject( new ParameterError('Cannot reconnect, Media has not established to reconnect') ); } - this.cleanUpBeforeReconnection(); + try { + await this.cleanUpBeforeReconnection(); + } catch (error) { + LoggerProxy.logger.error('Meeting:index#reconnect --> Cleanup failed:', error); + // Consider if you want to proceed with reconnection despite cleanup failure + // If not, return Promise.reject(error); + } return this.reconnectionManager .reconnect(options, async () => { await this.waitForRemoteSDPAnswer(); await this.waitForMediaConnectionConnected(); }) // ... rest of the methodAlso applies to: 4920-4934
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/@webex/plugin-meetings/src/meeting/index.ts(2 hunks)
🔇 Additional comments (1)
packages/@webex/plugin-meetings/src/meeting/index.ts (1)
4907-4908: LGTM! Good separation of cleanup logic into a dedicated method.
The addition of cleanUpBeforeReconnection() and its usage in reconnect() improves code organization by properly separating cleanup concerns.
| private async cleanUpBeforeReconnection(): Promise<void> { | ||
| // when media fails, we want to upload a webrtc dump to see whats going on | ||
| // this function is async, but returns once the stats have been gathered | ||
| await this.forceSendStatsReport({callFrom: 'cleanUpBeforeReconnection'}); | ||
|
|
||
| if (this.statsAnalyzer) { | ||
| await this.statsAnalyzer.stopAnalyzer(); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for async operations
The cleanUpBeforeReconnection() method performs async operations but doesn't handle potential errors. Consider adding try-catch blocks to handle failures gracefully.
private async cleanUpBeforeReconnection(): Promise<void> {
+ try {
await this.forceSendStatsReport({callFrom: 'cleanUpBeforeReconnection'});
if (this.statsAnalyzer) {
await this.statsAnalyzer.stopAnalyzer();
}
+ } catch (error) {
+ LoggerProxy.logger.error(
+ 'Meeting:index#cleanUpBeforeReconnection --> Error during cleanup:',
+ error
+ );
+ // Optionally rethrow if you want the error to propagate
+ throw error;
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private async cleanUpBeforeReconnection(): Promise<void> { | |
| // when media fails, we want to upload a webrtc dump to see whats going on | |
| // this function is async, but returns once the stats have been gathered | |
| await this.forceSendStatsReport({callFrom: 'cleanUpBeforeReconnection'}); | |
| if (this.statsAnalyzer) { | |
| await this.statsAnalyzer.stopAnalyzer(); | |
| } | |
| } | |
| private async cleanUpBeforeReconnection(): Promise<void> { | |
| try { | |
| // when media fails, we want to upload a webrtc dump to see whats going on | |
| // this function is async, but returns once the stats have been gathered | |
| await this.forceSendStatsReport({callFrom: 'cleanUpBeforeReconnection'}); | |
| if (this.statsAnalyzer) { | |
| await this.statsAnalyzer.stopAnalyzer(); | |
| } | |
| } catch (error) { | |
| LoggerProxy.logger.error( | |
| 'Meeting:index#cleanUpBeforeReconnection --> Error during cleanup:', | |
| error | |
| ); | |
| // Optionally rethrow if you want the error to propagate | |
| throw error; | |
| } | |
| } |
There was a problem hiding this comment.
yeah I agree with AI, it might be worth catching all the errors so that we don't accidentally stop reconnection from starting if something silly fails inside forceSendStatsReport or stopAnalyzer
There was a problem hiding this comment.
@marcin-bazyl, understood. I'll update the cleanUpBeforeReconnection() method to include error handling as suggested, so that any errors during cleanup don't prevent the reconnection from starting. Here's the updated method:
private async cleanUpBeforeReconnection(): Promise<void> {
try {
// when media fails, we want to upload a webrtc dump to see what's going on
// this function is async, but returns once the stats have been gathered
await this.forceSendStatsReport({callFrom: 'cleanUpBeforeReconnection'});
if (this.statsAnalyzer) {
await this.statsAnalyzer.stopAnalyzer();
}
} catch (error) {
LoggerProxy.logger.error(
'Meeting:index#cleanUpBeforeReconnection --> Error during cleanup:',
error
);
// Optionally rethrow if you want the error to propagate
// throw error;
}
}This ensures that any exceptions thrown during cleanup are caught and logged without interrupting the reconnection process.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (5)
3814-3819: Inconsistent indentation in error handling testThe indentation is inconsistent in this error handling test case. The test data setup is indented with 2 spaces while the test assertion uses 4 spaces.
- sinon.stub(meeting.meetingRequest, 'joinMeeting').resolves({ - headers: { - trackingid: 'fake tracking id', - }, - }); + sinon.stub(meeting.meetingRequest, 'joinMeeting').resolves({ + headers: { + trackingid: 'fake tracking id', + }, + });
4358-4359: Duplicate test case namesTwo test cases have identical names which makes it unclear what functionality each is testing. The test names should be unique and descriptive.
- it('addMedia() works correctly when media is disabled with no streams to publish', async () => { + it('addMedia() correctly handles audioEnabled=false with no streams', async () => { - it('addMedia() works correctly when video is disabled with no streams to publish', async () => { + it('addMedia() correctly handles videoEnabled=false with no streams', async () => {Also applies to: 4365-4366
7015-7018: Inconsistent indentation in test assertionThe assertion block uses inconsistent indentation which affects readability.
- assert.deepEqual(meeting.callStateForMetrics, { - correlationId: uuid1, - sessionCorrelationId: '', - }); + assert.deepEqual(meeting.callStateForMetrics, { + correlationId: uuid1, + sessionCorrelationId: '' + });
Line range hint
8025-8073: Inconsistent async/await usage in test casesThe test cases in this block mix Promise chaining and async/await styles which makes the code harder to follow. Should consistently use async/await.
- const mockFailedEvent = async () => { - meeting.setupMediaConnectionListeners(); - await simulateConnectionStateChange(ConnectionState.Failed); - }; + const mockFailedEvent = async () => { + await meeting.setupMediaConnectionListeners(); + await simulateConnectionStateChange(ConnectionState.Failed); + }; - it('stop stats analyzer during reconnection ', async () => { - meeting.hasMediaConnectionConnectedAtLeastOnce = true; - meeting.statsAnalyzer.stopAnalyzer = sinon.stub().resolves(); - meeting.reconnectionManager = { - reconnect: sinon.stub().resolves(), - resetReconnectionTimer: () => {} - }; - meeting.currentMediaStatus = { - video: true - }; - await mockFailedEvent(); - assert.calledOnce(meeting.statsAnalyzer.stopAnalyzer); - }); + it('stops stats analyzer during reconnection', async () => { + meeting.hasMediaConnectionConnectedAtLeastOnce = true; + meeting.statsAnalyzer.stopAnalyzer = sinon.stub().resolves(); + meeting.reconnectionManager = { + reconnect: sinon.stub().resolves(), + resetReconnectionTimer: () => {} + }; + meeting.currentMediaStatus = {video: true}; + + await mockFailedEvent(); + + assert.calledOnce(meeting.statsAnalyzer.stopAnalyzer); + });
Line range hint
1-100: Consider adding integration testsWhile the unit test coverage is good, there could be value in adding some integration tests that verify the interaction between multiple components (e.g. meeting join + media negotiation + reconnection flows).
Consider adding integration tests that:
- Test complete meeting lifecycles from join to leave
- Verify media negotiation with simulated network conditions
- Test reconnection scenarios with actual WebRTC connections
- Validate interaction between meeting components in real-world scenarios
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js(13 hunks)
🔇 Additional comments (1)
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (1)
Line range hint 1-100: Missing test coverage for error scenarios
The test file has good coverage for happy paths but could use more tests around error handling, especially for network failures, invalid states, and edge cases. Consider adding test cases for:
- Network timeouts during media negotiation
- Race conditions during reconnection
- Invalid meeting state transitions
- Error handling during media device failures
| assert.throws( | ||
| meeting.publishStreams(localStreams), | ||
| `Attempted to publish microphone stream with ended readyState, correlationId=${meeting.correlationId}` | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Duplicate error handling code for stream readyState checks
Similar error handling code is duplicated for checking stream readyState across audio, video, screenShare audio and screenShare video streams. This should be consolidated into a reusable helper function.
// Add helper function:
+ const validateStreamReadyState = (stream, type) => {
+ if (stream?.readyState === 'ended') {
+ throw new Error(`Attempted to publish ${type} stream with ended readyState, correlationId=${meeting.correlationId}`);
+ }
+ }
// Use helper function:
- if (stream?.readyState === 'ended') {
- assert.throws(
- meeting.publishStreams(localStreams),
- `Attempted to publish microphone stream with ended readyState, correlationId=${meeting.correlationId}`
- );
- }
+ validateStreamReadyState(stream, 'microphone');Also applies to: 4028-4031, 4044-4047, 4060-4063
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
packages/@webex/plugin-meetings/src/meeting/index.ts (1)
4907-4908: LGTM! Consider enhancing error loggingThe addition of
cleanUpBeforeReconnection()call before reconnection is a good improvement to ensure proper cleanup. However, consider adding more detailed error logging in the reconnect() method to help with debugging reconnection failures.this.cleanUpBeforeReconnection(); + LoggerProxy.logger.info('Meeting:index#reconnect --> Cleanup completed, proceeding with reconnection');
| private async cleanUpBeforeReconnection(): Promise<void> { | ||
| try { | ||
| // when media fails, we want to upload a webrtc dump to see whats going on | ||
| // this function is async, but returns once the stats have been gathered | ||
| await this.forceSendStatsReport({callFrom: 'cleanUpBeforeReconnection'}); | ||
|
|
||
| if (this.statsAnalyzer) { | ||
| await this.statsAnalyzer.stopAnalyzer(); | ||
| } | ||
| } catch (error) { | ||
| LoggerProxy.logger.error( | ||
| 'Meeting:index#cleanUpBeforeReconnection --> Error during cleanup: ', | ||
| error | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing error handling and cleanup sequence
The error handling and cleanup sequence in cleanUpBeforeReconnection() could be more robust:
- The error handling could be more specific to different types of failures
- The cleanup sequence could be more comprehensive
- Additional logging would help with debugging
private async cleanUpBeforeReconnection(): Promise<void> {
try {
+ LoggerProxy.logger.info('Meeting:index#cleanUpBeforeReconnection --> Starting cleanup');
+
// when media fails, we want to upload a webrtc dump to see whats going on
// this function is async, but returns once the stats have been gathered
await this.forceSendStatsReport({callFrom: 'cleanUpBeforeReconnection'});
if (this.statsAnalyzer) {
+ LoggerProxy.logger.info('Meeting:index#cleanUpBeforeReconnection --> Stopping stats analyzer');
await this.statsAnalyzer.stopAnalyzer();
}
+
+ // Additional cleanup tasks could be added here
+ LoggerProxy.logger.info('Meeting:index#cleanUpBeforeReconnection --> Cleanup completed successfully');
} catch (error) {
- LoggerProxy.logger.error(
- 'Meeting:index#cleanUpBeforeReconnection --> Error during cleanup: ',
- error
- );
+ // More specific error handling
+ if (error instanceof StatsAnalyzerError) {
+ LoggerProxy.logger.error(
+ 'Meeting:index#cleanUpBeforeReconnection --> Stats analyzer cleanup failed: ',
+ error
+ );
+ } else {
+ LoggerProxy.logger.error(
+ 'Meeting:index#cleanUpBeforeReconnection --> Unexpected error during cleanup: ',
+ error
+ );
+ }
+ // Re-throw to allow caller to handle the error
+ throw error;
}
}Committable suggestion skipped: line range outside the PR's diff.
k-wasniowski
deleted the
fix-cleanup-stats-analyzer-before-full-reconnection
branch
COMPLETES #SPARK-548638
This pull request addresses
This PR addresses the issue with web client sending media statistics despite of that client is not connected to the meeting.
This might occur if client got stuck on
mercurysocket reconnection for a long time.by making the following changes
Stop stats analyzer during the full reconnection phase. Stats analyzer shouldn't send out metrics of the PeerConnection which will get closed soon.
Change Type
The following scenarios were tested
Tested with JS-SDK various ways of client getting disconnected
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
Bug Fixes
Documentation