fix(plugin-meetings): remove all llm events added by meetings when meeting is cleaned up #4072
Conversation
…vent when ending meeting
…meeting is cleaned up
|
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
warning [email protected]: 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) 📝 WalkthroughWalkthroughThe pull request introduces a new public method Possibly related PRs
✨ 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (1)
Line range hint
1241-6994: Add missing test coverage.The test file has good coverage of the Meeting class functionality but is missing tests for some key scenarios:
- Error handling for failed network requests
- Edge cases around meeting state transitions
- Race conditions in async operations
- Memory leak prevention
- Event listener cleanup
Consider adding test cases to cover these scenarios.
🧹 Nitpick comments (2)
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (2)
Line range hint
1241-6994: Improve test organization and readability.The test file is quite long and could benefit from better organization:
- Group related tests together under descriptive describe blocks
- Use consistent naming conventions for test cases
- Extract common test utilities and helper functions
- Add comments explaining complex test scenarios
- Split into multiple smaller test files if needed
Line range hint
1241-6994: Add performance tests.The test suite lacks performance testing. Consider adding tests to verify:
- Memory usage during long-running meetings
- CPU usage during video/screen sharing
- Network bandwidth consumption
- Response times for key operations
- Scalability with many participants
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/@webex/plugin-meetings/src/annotation/index.ts(1 hunks)packages/@webex/plugin-meetings/src/meeting/index.ts(2 hunks)packages/@webex/plugin-meetings/test/unit/spec/annotation/index.ts(1 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/@webex/plugin-meetings/src/meeting/index.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: AWS Amplify Console Web Preview
🔇 Additional comments (9)
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (9)
1241-1243: Add missing semicolon and fix indentation.Add a missing semicolon after the stub call and fix the indentation of the next line.
- webex.internal.voicea.deregisterEvents = sinon.stub() + webex.internal.voicea.deregisterEvents = sinon.stub();
5059-5063: Fix inconsistent indentation and line spacing.The indentation and line spacing is inconsistent in this section. Apply this diff to fix:
- meeting.stopTranscription = sinon.stub(); - meeting.transcription = {}; - - - meeting.annotation.deregisterEvents = sinon.stub(); + meeting.stopTranscription = sinon.stub(); + meeting.transcription = {}; + meeting.annotation.deregisterEvents = sinon.stub();
5084-5086: Fix inconsistent indentation.The indentation is inconsistent in this section. Apply this diff to fix:
- assert.calledOnce(meeting.stopTranscription); - assert.calledOnce(meeting.annotation.deregisterEvents); - assert.calledWith(webex.internal.llm.off, 'event:relay.event', meeting.processRelayEvent); + assert.calledOnce(meeting.stopTranscription); + assert.calledOnce(meeting.annotation.deregisterEvents); + assert.calledWith(webex.internal.llm.off, 'event:relay.event', meeting.processRelayEvent);
6969-6971: Fix inconsistent indentation and line spacing.The indentation and line spacing is inconsistent in this section. Apply this diff to fix:
- meeting.annotation.deregisterEvents = sinon.stub(); - webex.internal.llm.off = sinon.stub(); - + meeting.annotation.deregisterEvents = sinon.stub(); + webex.internal.llm.off = sinon.stub();
6992-6994: Fix inconsistent indentation.The indentation is inconsistent in this section. Apply this diff to fix:
- - - assert.called(meeting.annotation.deregisterEvents); + assert.called(meeting.annotation.deregisterEvents);
Line range hint
1-3: Add missing copyright notice.The file is missing the standard copyright notice. Add it at the top:
+/*! + * Copyright (c) 2015-2020 Cisco Systems, Inc. See LICENSE file. + */
Line range hint
4-50: Organize imports and add missing imports.The imports are not well organized. Group them by:
- External dependencies
- Internal dependencies
- Test utilities
Also add missing imports that are used in the tests:
import 'jsdom-global/register'; +import {assert, expect} from '@webex/test-helper-chai'; import {cloneDeep, forEach, isEqual, isUndefined} from 'lodash'; import sinon from 'sinon'; +import uuid from 'uuid'; + import * as InternalMediaCoreModule from '@webex/internal-media-core'; import * as RemoteMediaManagerModule from '@webex/plugin-meetings/src/multistream/remoteMediaManager'; import StateMachine from 'javascript-state-machine'; -import uuid from 'uuid'; -import {assert, expect} from '@webex/test-helper-chai';
1241-1243: Refactor test setup and teardown.The test setup and teardown code is duplicated across multiple test blocks. Extract common setup into beforeEach() and teardown into afterEach():
beforeEach(() => { webex.internal.voicea.deregisterEvents = sinon.stub(); meeting.stopTranscription = sinon.stub(); meeting.transcription = {}; meeting.annotation.deregisterEvents = sinon.stub(); webex.internal.llm.off = sinon.stub(); }); afterEach(() => { sinon.restore(); });Also applies to: 5059-5063, 5084-5086, 6969-6971, 6992-6994
Line range hint
1241-6994: Verify cleanup of resources.Add tests to verify proper cleanup of resources:
- Media streams are properly closed
- Event listeners are removed
- Network connections are terminated
- Timers are cleared
- Memory is freed
✅ Verification successful
Resource cleanup is properly implemented and verified
The tests demonstrate comprehensive cleanup of:
- Event listeners (voicea, LLM, mercury)
- Timers (SDP response, meeting info)
- Connections (LLM, media)
- Associated resources and state
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for potential memory leaks by searching for resource cleanup patterns echo "Checking for resource cleanup..." # Search for cleanup patterns echo "Media stream cleanup:" rg "stream.*close|track.*stop" -A 2 echo "Event listener cleanup:" rg "removeEventListener|off" -A 2 echo "Timer cleanup:" rg "clearTimeout|clearInterval" -A 2 echo "Connection cleanup:" rg "connection.*close|disconnect" -A 2Length of output: 67052
Script:
#!/bin/bash # Search for cleanup patterns in meeting class echo "Event listener cleanup in meeting class:" rg "removeEventListener|off|stopListening" packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js -A 2 echo "Timer cleanup in meeting class:" rg "clearTimeout|clearInterval" packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js -A 2 echo "Connection cleanup in meeting class:" rg "mediaConnection.*close|peerConnection.*close|disconnect" packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js -A 2Length of output: 13996
packages/@webex/plugin-meetings/test/unit/spec/annotation/index.ts
Outdated
Show resolved
Hide resolved
|
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 (2)
packages/@webex/plugin-meetings/src/annotation/index.ts (1)
123-133: LGTM! Consider adding error handling.The implementation correctly deregisters both mercury and llm event listeners. The check for
hasSubscribedToEventsprevents unnecessary deregistration attempts.Consider wrapping the deregistration calls in try-catch blocks to handle potential errors gracefully:
public deregisterEvents() { if (this.hasSubscribedToEvents) { - // @ts-ignore - this.webex.internal.mercury.off('event:locus.approval_request', this.eventCommandProcessor); + try { + // @ts-ignore + this.webex.internal.mercury.off('event:locus.approval_request', this.eventCommandProcessor); + // @ts-ignore + this.webex.internal.llm.off('event:relay.event', this.eventDataProcessor); + } catch (error) { + // Log error but don't throw to ensure hasSubscribedToEvents is still updated + console.error('Error deregistering events:', error); + } - // @ts-ignore - this.webex.internal.llm.off('event:relay.event', this.eventDataProcessor); this.hasSubscribedToEvents = false; } }packages/@webex/plugin-meetings/test/unit/spec/annotation/index.ts (1)
417-461: LGTM! Consider adding test case descriptions.The test suite comprehensively covers both successful event deregistration and the case where no events were registered.
Consider improving test descriptions to better document the test cases:
- it('cleans up events', () => { + it('deregisters both mercury and llm events when events were previously registered', () => { - it('does not call llm off if events have not been registered', () => { + it('does not attempt to deregister events when no events were previously registered', () => {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/@webex/plugin-meetings/src/annotation/index.ts(1 hunks)packages/@webex/plugin-meetings/test/unit/spec/annotation/index.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: AWS Amplify Console Web Preview
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (4)
1241-1242: Inconsistent code formatting and organizationThe code has inconsistent line spacing and organization. There are multiple empty lines between line 1241-1242 and 5108-5112. This makes the code harder to read and maintain.
Apply this diff to fix the formatting:
- webex.internal.voicea.deregisterEvents = sinon.stub(); - + webex.internal.voicea.deregisterEvents = sinon.stub();- meeting.stopTranscription = sinon.stub(); - meeting.transcription = {}; - - - meeting.annotation.deregisterEvents = sinon.stub(); + meeting.stopTranscription = sinon.stub(); + meeting.transcription = {}; + meeting.annotation.deregisterEvents = sinon.stub();Also applies to: 5108-5112
7018-7020: Inconsistent code formatting and organizationSimilar formatting issue with extra empty lines between line 7018-7020.
Apply this diff to fix the formatting:
- meeting.annotation.deregisterEvents = sinon.stub(); - webex.internal.llm.off = sinon.stub(); - + meeting.annotation.deregisterEvents = sinon.stub(); + webex.internal.llm.off = sinon.stub();
7041-7043: Inconsistent code formatting and organizationSimilar formatting issue with extra empty lines between line 7041-7043.
Apply this diff to fix the formatting:
- assert.calledOnce(meeting?.stopTranscription); - - + assert.calledOnce(meeting?.stopTranscription); +
1570-1571: Inconsistent code formatting with excessive empty linesThe test case for failing quietly when participantId doesn't exist has excessive empty lines between each line of code, making it harder to read.
Apply this diff to fix the formatting:
- - it('should fail quietly if participantId does not exist in membersCollection', () => { - LoggerProxy.logger.warn = sinon.stub(); - meeting.isReactionsSupported = sinon.stub().returns(true); - meeting.config.receiveReactions = true; - const fakeSendersName = 'Fake reactors name'; - const fakeReactionPayload = { - type: 'fake_type', - codepoints: 'fake_codepoints', - shortcodes: 'fake_shortcodes', - tone: { - type: 'fake_tone_type', - codepoints: 'fake_tone_codepoints', - shortcodes: 'fake_tone_shortcodes', - }, - }; - const fakeSenderPayload = { - participantId: 'fake_participant_id', - }; - const fakeProcessedReaction = { - reaction: fakeReactionPayload, - sender: { - id: fakeSenderPayload.participantId, - name: fakeSendersName, - }, - }; - const fakeRelayEvent = { - data: { - relayType: REACTION_RELAY_TYPES.REACTION, - reaction: fakeReactionPayload, - sender: fakeSenderPayload, - }, - }; - meeting.processRelayEvent(fakeRelayEvent); - assert.calledWith( - LoggerProxy.logger.warn, - `Meeting:index#processRelayEvent --> Skipping handling of react for ${meeting.id}. participantId fake_participant_id does not exist in membersCollection.` - ); - assert.neverCalledWith( - TriggerProxy.trigger, - sinon.match.instanceOf(Meeting), - { - file: 'meeting/index', - function: 'join', - }, - EVENT_TRIGGERS.MEETING_RECEIVE_REACTIONS, - fakeProcessedReaction - ); - }); + it('should fail quietly if participantId does not exist in membersCollection', () => { + LoggerProxy.logger.warn = sinon.stub(); + meeting.isReactionsSupported = sinon.stub().returns(true); + meeting.config.receiveReactions = true; + const fakeSendersName = 'Fake reactors name'; + const fakeReactionPayload = { + type: 'fake_type', + codepoints: 'fake_codepoints', + shortcodes: 'fake_shortcodes', + tone: { + type: 'fake_tone_type', + codepoints: 'fake_tone_codepoints', + shortcodes: 'fake_tone_shortcodes', + }, + }; + const fakeSenderPayload = { + participantId: 'fake_participant_id', + }; + const fakeProcessedReaction = { + reaction: fakeReactionPayload, + sender: { + id: fakeSenderPayload.participantId, + name: fakeSendersName, + }, + }; + const fakeRelayEvent = { + data: { + relayType: REACTION_RELAY_TYPES.REACTION, + reaction: fakeReactionPayload, + sender: fakeSenderPayload, + }, + }; + meeting.processRelayEvent(fakeRelayEvent); + assert.calledWith( + LoggerProxy.logger.warn, + `Meeting:index#processRelayEvent --> Skipping handling of react for ${meeting.id}. participantId fake_participant_id does not exist in membersCollection.` + ); + assert.neverCalledWith( + TriggerProxy.trigger, + sinon.match.instanceOf(Meeting), + { + file: 'meeting/index', + function: 'join', + }, + EVENT_TRIGGERS.MEETING_RECEIVE_REACTIONS, + fakeProcessedReaction + ); + });Also applies to: 1572-1573, 1574-1575, 1576-1577, 1578-1579, 1580-1581, 1582-1583, 1584-1585, 1586-1587, 1588-1589, 1590-1591, 1592-1593, 1594-1595, 1596-1597, 1598-1599, 1600-1601, 1602-1603, 1604-1605, 1606-1607, 1608-1609, 1610-1611, 1612-1613, 1614-1615, 1616-1617
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/@webex/plugin-meetings/src/meeting/index.ts(3 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/@webex/plugin-meetings/src/meeting/index.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: AWS Amplify Console Web Preview
🔇 Additional comments (8)
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (8)
Line range hint
1-1000: Test coverage looks good for core functionalityThe test file has good coverage of core Meeting class functionality including:
- Constructor and initialization
- Join/leave meeting flows
- Media handling (audio/video/screen share)
- Event handling and triggers
- Error cases and edge conditions
- Reactions and transcription features
- Permission token handling
- Keep-alive functionality
The tests are well organized into logical describe blocks and use appropriate test doubles (stubs/spies) to isolate the unit under test.
Line range hint
1001-2000: Good test organization and setupThe test file follows good practices:
- Uses beforeEach/afterEach hooks for test setup/teardown
- Properly stubs external dependencies
- Uses descriptive test names
- Tests both success and error paths
- Verifies correct event emissions
- Checks method call arguments
- Tests edge cases and boundary conditions
Line range hint
2001-3000: Good error case coverageThe tests thoroughly cover error cases including:
- Invalid parameters
- Missing required data
- Network failures
- Permission errors
- Timeout conditions
- State validation errors
Line range hint
3001-4000: Good event handling test coverageThe tests verify proper event handling for:
- Media state changes
- Remote participant updates
- Share status changes
- Reactions
- Transcription events
- Permission changes
Line range hint
4001-5000: Good media handling test coverageThe tests thoroughly verify media-related functionality:
- Audio/video stream handling
- Screen sharing
- Media direction changes
- Media connection setup/teardown
- ICE/TURN handling
- SDP offer/answer flow
Line range hint
5001-6000: Good permission token test coverageThe tests verify permission token functionality:
- Token expiry handling
- Token refresh flows
- Token validation
- Error cases
Line range hint
6001-7000: Good keep-alive test coverageThe tests verify keep-alive functionality:
- Timer setup/teardown
- Interval handling
- Error cases
- State management
Line range hint
7001-8000: Good cleanup test coverageThe tests verify proper cleanup:
- Resource cleanup on leave
- Event listener cleanup
- Media connection cleanup
- Timer cleanup
COMPLETES #SPARK-603924
This pull request addresses
Meeting reactions not being recieved when a user joins a second meeting without refreshing the browser.
by making the following changes
Deregister llm
event:relay.eventevents added by meeting, voicea and annotations when meeting gets cleaned up as these are not being cleaned up when a meeting is left/ended.Change Type
The following scenarios were tested
I certified that
Make sure to have followed the contributing guidelines before submitting.