feat(llm): force close mercury before reconnect a new data channel #4150
Conversation
… a new data channel
📝 WalkthroughWalkthroughThis pull request modifies the disconnection flow for the LLM functionality. 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. 🔧 ESLint
yarn install v1.22.22 (For a CapTP with native promises, see @endo/eventual-send and @endo/captp) Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🪧 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
🧹 Nitpick comments (3)
packages/@webex/plugin-meetings/src/controls-options-manager/util.ts (1)
304-306: Consider adding validation logic for the new control.The implementation for
viewTheParticipantListForWebinaralways returnstrue, unlike other controls that have specific validation logic. If there are conditions under which this control should not be updated, consider implementing a validation method similar tocanUpdateViewTheParticipantsList.If no specific validation is needed, please add a comment explaining why this control is always allowed to be updated to improve code maintainability.
case Control.viewTheParticipantListForWebinar: + // No specific validation needed for webinar participant list control determinant = true; break;packages/@webex/internal-plugin-llm/test/unit/spec/llm.js (2)
115-151: Inconsistent testing frameworks between new and existing tests.The new test suite for
disconnectLLMuses Jest testing methods (jest.fn(),expect().toHaveBeenCalledWith(), etc.) while the rest of the file uses Sinon and Chai (sinon.stub(),assert.equal(), etc.). This inconsistency can make the codebase harder to maintain.Consider updating the new tests to use Sinon and Chai for consistency:
- disconnect: jest.fn(() => Promise.resolve()), + disconnect: sinon.stub().resolves(), - expect(instance.disconnect).toHaveBeenCalledWith({}); - expect(instance.locusUrl).toBeUndefined(); + sinon.assert.calledOnceWithExactly(instance.disconnect, {}); + assert.equal(instance.locusUrl, undefined); - instance.disconnect.mockRejectedValue(new Error('Disconnect failed')); + instance.disconnect.rejects(new Error('Disconnect failed')); - await expect(instance.disconnectLLM({})).rejects.toThrow('Disconnect failed'); + await assert.isRejected(instance.disconnectLLM({}), Error, 'Disconnect failed');
137-138: Add test case for the specific disconnect options mentioned in PR objectives.The current test only verifies that options are passed through, but doesn't test the specific options mentioned in the PR objectives (
{ code: 3050, reason: 'done (permanent)' }).Consider adding an additional test case with the specific options:
it('should pass specific disconnect options', async () => { const options = { code: 3050, reason: 'done (permanent)' }; await instance.disconnectLLM(options); expect(instance.disconnect).toHaveBeenCalledWith(options); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/@webex/internal-plugin-llm/src/llm.ts(1 hunks)packages/@webex/internal-plugin-llm/src/llm.types.ts(1 hunks)packages/@webex/internal-plugin-llm/test/unit/spec/llm.js(1 hunks)packages/@webex/plugin-meetings/src/controls-options-manager/enums.ts(1 hunks)packages/@webex/plugin-meetings/src/controls-options-manager/util.ts(1 hunks)packages/@webex/plugin-meetings/src/meeting/index.ts(1 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js(3 hunks)
🔇 Additional comments (7)
packages/@webex/plugin-meetings/src/controls-options-manager/enums.ts (1)
15-15: Addition of new enum member looks good.The new
viewTheParticipantListForWebinarenum member extends theControlenum to support webinar-specific participant list viewing functionality, which aligns with the existing naming convention.packages/@webex/internal-plugin-llm/src/llm.types.ts (1)
6-6: Interface update looks good.The
disconnectLLMmethod signature now accepts an options object with code and reason properties, which aligns with the PR objective to provide better context when disconnecting LLM channels.packages/@webex/internal-plugin-llm/src/llm.ts (1)
119-125:❓ Verification inconclusive
Implementation aligns with interface update.
The updated method signature and implementation correctly pass the options object to the underlying disconnect method, supporting the PR objective of forcibly closing previous connections with specific codes.
However, verify backward compatibility for any existing calls to
disconnectLLM()without options.
🏁 Script executed:
#!/bin/bash # Search for any calls to disconnectLLM without parameters rg "disconnectLLM\(\s*\)" --type tsLength of output: 37
Action Required: Verify Backward Compatibility for
disconnectLLMCalls
The current update aligns with the interface by forwarding the options object todisconnect. However, our automated search for calls todisconnectLLM()without arguments provided inconclusive results. Please manually verify that no parts of the codebase invokedisconnectLLM()without the required options parameter. Ensure all usage ofdisconnectLLMis reviewed to maintain backward compatibility.packages/@webex/plugin-meetings/src/meeting/index.ts (1)
7789-7796: Positive change to enforce proper LLM disconnection.The modification to pass a disconnect code and reason when disconnecting from the LLM data channel is a good improvement. This ensures a clean closure of the previous connection before establishing a new one, preventing potential issues with lingering connections.
The code
3050with reason'done (permanent)'signals to the socket that this is an intentional, permanent disconnection rather than a temporary one that might automatically reconnect.packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (3)
11254-11257: Good test coverage for the new disconnect parameters.Verifying that
disconnectLLMis called withcode: 3050andreason: 'done (permanent)'confirms the intended “permanent” disconnection scenario. The test logic is consistent with the stated PR objective to force close the existing channel before reconnecting.
11287-11290: Validate code consistency across all test cases.This assertion again checks for
code: 3050andreason: 'done (permanent)', ensuring standard behavior in multiple scenarios. Consistency across tests helps avoid regressions and keeps expectations unified.
11319-11319:✅ Verification successful
Confirm that passing
undefinedtodisconnectLLMis intended.This test verifies a case where no disconnection parameters are specified. Confirm that this scenario is strategically valid and does not conflict with the permanent disconnection approach in other tests.
Run the following script to search for all test references to
disconnectLLMto ensure consistent usage patterns:
🏁 Script executed:
#!/bin/bash # Description: Locate all test references to `disconnectLLM` and compare arguments. rg -A 4 "disconnectLLM"Length of output: 73274
Confirmation of Undefined Disconnection Behavior
I verified that the test case passingundefinedtodisconnectLLMintentionally simulates the scenario where no disconnection parameters are provided. In the test file, other cases explicitly pass an object (e.g., with{ code: 3050, reason: 'done (permanent)' }), while this case ensures that the absence of disconnection parameters is handled appropriately without interfering with the permanent disconnection behavior in different scenarios. This usage aligns with the overall test patterns.
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/@webex/internal-plugin-llm/test/unit/spec/llm.js (1)
138-142: Add test for disconnectLLM without options parameter.The PR adds support for an options parameter to
disconnectLLM, but it's important to test that the method still works correctly when called without parameters to maintain backward compatibility.Consider adding a test case:
it('should handle disconnect call without options parameter', async () => { // Set the properties to test clearing llmService.locusUrl = 'someUrl'; llmService.datachannelUrl = 'someUrl'; llmService.binding = {}; llmService.webSocketUrl = 'someUrl'; await llmService.disconnectLLM(); sinon.assert.calledOnce(llmService.disconnect); assert.equal(llmService.locusUrl, undefined); assert.equal(llmService.datachannelUrl, undefined); assert.equal(llmService.binding, undefined); assert.equal(llmService.webSocketUrl, undefined); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/@webex/internal-plugin-llm/src/llm.ts(1 hunks)packages/@webex/internal-plugin-llm/test/unit/spec/llm.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/@webex/internal-plugin-llm/src/llm.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Packages
- GitHub Check: AWS Amplify Console Web Preview
🔇 Additional comments (1)
packages/@webex/internal-plugin-llm/test/unit/spec/llm.js (1)
115-143: Improved test coverage for disconnectLLM method.The new test suite effectively covers both the success path and error handling for the
disconnectLLMmethod. This aligns with the PR objective of ensuring proper disconnection of the LLM data channel before establishing a new connection.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
packages/@webex/internal-plugin-llm/test/unit/spec/llm.js (3)
136-144: 🛠️ Refactor suggestionMissing specific parameter testing per PR objective
According to the PR objective, a specific disconnect code of 3050 and reason "done (permanent)" should be used. The test should verify these specific parameters.
Update the test to verify the specific parameters:
it('should call disconnect and clear relevant properties', async () => { - await instance.disconnectLLM({}); + const disconnectOptions = { code: 3050, reason: 'done (permanent)' }; + await instance.disconnectLLM(disconnectOptions); - expect(instance.disconnect).toHaveBeenCalledWith({}); + expect(instance.disconnect).toHaveBeenCalledWith(disconnectOptions); expect(instance.locusUrl).toBeUndefined(); expect(instance.datachannelUrl).toBeUndefined(); expect(instance.binding).toBeUndefined(); expect(instance.webSocketUrl).toBeUndefined(); });
115-151: 🛠️ Refactor suggestionTest suite uses inconsistent testing frameworks
This test suite uses Jest testing methods (
jest.fn(),mockRejectedValue,expect()) while the rest of the file uses Sinon for stubbing and Chai for assertions.Update the test to use a consistent testing approach with the rest of the file:
describe('disconnectLLM', () => { let instance; beforeEach(() => { instance = { - disconnect: jest.fn(() => Promise.resolve()), + disconnect: sinon.stub().resolves(), locusUrl: 'someUrl', datachannelUrl: 'someUrl', binding: {}, webSocketUrl: 'someUrl', disconnectLLM: function (options) { return this.disconnect(options).then(() => { this.locusUrl = undefined; this.datachannelUrl = undefined; this.binding = undefined; this.webSocketUrl = undefined; }); } }; }); it('should call disconnect and clear relevant properties', async () => { await instance.disconnectLLM({}); - expect(instance.disconnect).toHaveBeenCalledWith({}); - expect(instance.locusUrl).toBeUndefined(); - expect(instance.datachannelUrl).toBeUndefined(); - expect(instance.binding).toBeUndefined(); - expect(instance.webSocketUrl).toBeUndefined(); + sinon.assert.calledOnceWithExactly(instance.disconnect, {}); + assert.equal(instance.locusUrl, undefined); + assert.equal(instance.datachannelUrl, undefined); + assert.equal(instance.binding, undefined); + assert.equal(instance.webSocketUrl, undefined); }); it('should handle errors from disconnect gracefully', async () => { - instance.disconnect.mockRejectedValue(new Error('Disconnect failed')); - - await expect(instance.disconnectLLM({})).rejects.toThrow('Disconnect failed'); + instance.disconnect.rejects(new Error('Disconnect failed')); + + try { + await instance.disconnectLLM({}); + assert.fail('Should have thrown an error'); + } catch (error) { + assert.equal(error.message, 'Disconnect failed'); + } }); });
119-133: 🛠️ Refactor suggestionTest uses a mock implementation instead of the actual LLMService
This test creates a separate mock implementation of
disconnectLLMinstead of testing the actual implementation inllmService. This approach tests the mock rather than the real implementation.Update the test to use the actual
llmServiceinstance that's already created in the outer beforeEach block:describe('disconnectLLM', () => { - let instance; - beforeEach(() => { - instance = { - disconnect: jest.fn(() => Promise.resolve()), - locusUrl: 'someUrl', - datachannelUrl: 'someUrl', - binding: {}, - webSocketUrl: 'someUrl', - disconnectLLM: function (options) { - return this.disconnect(options).then(() => { - this.locusUrl = undefined; - this.datachannelUrl = undefined; - this.binding = undefined; - this.webSocketUrl = undefined; - }); - } - }; + // The disconnect method is already stubbed in the outer beforeEach }); it('should call disconnect and clear relevant properties', async () => { - await instance.disconnectLLM({}); + // Set properties to test clearing + llmService.locusUrl = 'someUrl'; + llmService.datachannelUrl = 'someUrl'; + llmService.binding = {}; + llmService.webSocketUrl = 'someUrl'; + + await llmService.disconnectLLM({});
🧹 Nitpick comments (1)
packages/@webex/internal-plugin-llm/test/unit/spec/llm.js (1)
125-132: Implement explicit error handling in disconnectLLMThe current implementation of
disconnectLLMdoesn't explicitly handle errors from thedisconnectcall. While errors will still propagate, it would be better to have explicit error handling.Add explicit error handling to the disconnectLLM implementation:
disconnectLLM: function (options) { - return this.disconnect(options).then(() => { - this.locusUrl = undefined; - this.datachannelUrl = undefined; - this.binding = undefined; - this.webSocketUrl = undefined; - }); + return this.disconnect(options) + .then(() => { + this.locusUrl = undefined; + this.datachannelUrl = undefined; + this.binding = undefined; + this.webSocketUrl = undefined; + }) + .catch((error) => { + // Log error or perform additional error handling if needed + throw error; + }); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/@webex/internal-plugin-llm/src/llm.ts(1 hunks)packages/@webex/internal-plugin-llm/test/unit/spec/llm.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/@webex/internal-plugin-llm/src/llm.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Determine Changed Packages
- GitHub Check: AWS Amplify Console Web Preview
|
|
||
| /** | ||
| * Disconnects websocket connection | ||
| * @param {object} options |
There was a problem hiding this comment.
update the param with individual sub objects
There was a problem hiding this comment.
i also dont see isJoined
There was a problem hiding this comment.
update the param with individual sub objects
updated
There was a problem hiding this comment.
i also dont see isJoined
isJoined is not a part of the options object
arun3528
changed the title
3 participants
COMPLETES #< INSERT LINK TO ISSUE >
https://jira-eng-gpk2.cisco.com/jira/browse/SPARK-585446
This pull request addresses
Force close socket connection of previous LLM data channel, before register and connect a new data channel.
by making the following changes
Pass code 3050 and reson "done (permanent)" to socket when disconnect LLM
Change Type
The following scenarios were tested
< ENUMERATE TESTS PERFORMED, WHETHER MANUAL OR AUTOMATED >
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.