Web worker #4103
Web worker #4103
Conversation
📝 WalkthroughWalkthroughThe changes encompass updates to the deployment workflow, core functionality, testing, and registration processes. The deployment configuration now includes an additional branch ( 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) ✨ 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: 3
🔭 Outside diff range comments (1)
packages/calling/src/CallingClient/line/line.test.ts (1)
80-123: 🛠️ Refactor suggestionUpdate keepalive test for web worker implementation.
This test verifies the keepalive functionality using
jest.advanceTimersByTime. Since the PR implements web workers for keepalive signals, this test should be updated to:
- Verify web worker initialization
- Test message passing between the main thread and worker
- Ensure keepalive signals are sent from the worker
Would you like me to help update this test to verify the web worker implementation?
🧹 Nitpick comments (2)
packages/calling/src/CallingClient/registration/register.ts (2)
683-755: Consider adding an onerror handler for the worker and verifying concurrency.
- Adding
this.webWorker.onerrorcould help handle unexpected failures in the worker context more gracefully.- Verify that multiple consecutive calls to
startKeepaliveTimerwon’t produce extra or leftover workers if called concurrently.
760-766: Minor redundancy in the termination flow.
Since.terminate()kills the worker, postingCLEAR_KEEPALIVEmight be unnecessary. Consider removing the extra post or leaving a comment clarifying the intention.public clearKeepaliveTimer() { if (this.webWorker) { - this.webWorker.postMessage({type: WorkerMessageType.CLEAR_KEEPALIVE}); this.webWorker.terminate(); this.webWorker = undefined; } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.github/workflows/deploy.yml(1 hunks)packages/@webex/webex-core/src/webex-core.js(1 hunks)packages/calling/src/CallingClient/CallingClient.test.ts(2 hunks)packages/calling/src/CallingClient/line/line.test.ts(4 hunks)packages/calling/src/CallingClient/registration/register.test.ts(2 hunks)packages/calling/src/CallingClient/registration/register.ts(3 hunks)packages/calling/src/CallingClient/registration/webWorker.ts(1 hunks)packages/calling/src/common/types.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
packages/calling/src/CallingClient/registration/webWorker.ts (1)
Learnt from: Kesari3008
PR: webex/webex-js-sdk#4101
File: packages/calling/src/CallingClient/registration/webWorker.ts:4-8
Timestamp: 2025-02-17T22:31:35.639Z
Learning: In web worker context, using `onmessage` directly is equivalent to `self.onmessage` and is a valid practice. Both approaches are acceptable for handling web worker messages.
🪛 Biome (1.9.4)
packages/calling/src/CallingClient/registration/webWorker.ts
[error] 3-4: A global variable should not be reassigned.
Assigning to a global variable can override essential functionality.
(lint/suspicious/noGlobalAssign)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Initialize Project
- GitHub Check: AWS Amplify Console Web Preview
🔇 Additional comments (7)
packages/calling/src/CallingClient/registration/register.ts (2)
22-22: Looks good.
The addition ofWorkerMessageTypeimport is consistent with the rest of the file.
80-80: Browser environment check.
Ensure that the environment can support web workers before instantiating them at runtime.packages/calling/src/common/types.ts (1)
262-268: Enum addition looks great.
The newWorkerMessageTypevalues provide clear and extensible message types for keepalive operations.packages/@webex/webex-core/src/webex-core.js (1)
45-77: LGTM! Well-structured interceptors configuration.The new
interceptorsobject effectively centralizes the creation of interceptor instances, making the code more maintainable. The conditional handling of logging interceptors based on environment variables is a good practice.packages/calling/src/CallingClient/CallingClient.test.ts (1)
13-13: Create a plan to re-enable the skipped tests.Multiple test files have been disabled:
- Commented out imports:
createClientandLine- Skipped test suite: "CallingClient Tests"
This significantly reduces test coverage. While temporarily skipping tests during refactoring is understandable, we need a plan to re-enable them.
Please provide:
- Timeline for fixing and re-enabling these tests
- Confirmation that these tests will be updated to work with the new web worker implementation
- Any dependencies or blockers that need to be resolved first
Also applies to: 26-26, 52-52
packages/calling/src/CallingClient/registration/register.test.ts (1)
3-3: Re-enable registration tests after web worker implementation.The registration tests have been disabled:
- Commented out import:
createRegistration- Skipped test suite: "Registration Tests"
These tests are crucial for validating the registration functionality, especially with the new web worker implementation for keepalive signals.
Please provide:
- Plan for updating these tests to work with the web worker implementation
- Timeline for re-enabling the test suite
- Confirmation that the tests will cover both the old and new keepalive mechanisms
Also applies to: 43-43
.github/workflows/deploy.yml (1)
7-8: New Deployable Branch Added: "web-workers-keepalive"The addition of the
web-workers-keepalivebranch in the deployment trigger is correctly integrated. This aligns well with the PR objective to implement web workers for keepalive signals. Please ensure that any accompanying documentation or branch naming conventions across the project reflect this new branch.
| onmessage = (event: MessageEvent) => { | ||
| const {type} = event.data; | ||
| let keepaliveTimer: NodeJS.Timer | undefined; | ||
|
|
||
| const postKeepAlive = async (accessToken: string, deviceUrl: string, url: string) => { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Move “keepaliveTimer” to a higher scope for stronger consistency.
Currently, each message event redeclares keepaliveTimer. Move it outside the onmessage function to persist the reference across repeated starts/stops:
-let keepaliveTimer: NodeJS.Timer | undefined;
-onmessage = (event: MessageEvent) => {
- const {type} = event.data;
- let keepaliveTimer: NodeJS.Timer | undefined;
+let keepaliveTimer: NodeJS.Timer | undefined;
+onmessage = (event: MessageEvent) => {
const {type} = event.data;
...
};📝 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.
| onmessage = (event: MessageEvent) => { | |
| const {type} = event.data; | |
| let keepaliveTimer: NodeJS.Timer | undefined; | |
| const postKeepAlive = async (accessToken: string, deviceUrl: string, url: string) => { | |
| let keepaliveTimer: NodeJS.Timer | undefined; | |
| onmessage = (event: MessageEvent) => { | |
| const {type} = event.data; | |
| const postKeepAlive = async (accessToken: string, deviceUrl: string, url: string) => { | |
| // function implementation goes here | |
| }; | |
| // Other message handling logic... | |
| }; |
| @@ -16,20 +16,20 @@ import { | |||
| WebexRequestPayload, | |||
| } from '../../common/types'; | |||
| import {LINE_EVENTS} from './types'; | |||
| import Line from '.'; | |||
| // import Line from '.'; | |||
There was a problem hiding this comment.
Tests should not be skipped without tracking.
The entire test suite has been skipped and imports have been commented out with a TODO comment. While this might be temporary during the web worker implementation, we should:
- Create a tracking issue for fixing these tests
- Add a comment explaining why the tests are skipped
- Set a timeline for re-enabling them
This helps prevent the tests from being forgotten and ensures test coverage is restored.
Would you like me to help create a tracking issue for fixing these tests?
Also applies to: 24-24, 26-26
| const mutex = new Mutex(); | ||
| const webex = getTestUtilsWebex(); | ||
| SDKConnector.setWebex(webex); | ||
|
|
||
| const defaultServiceData = {indicator: ServiceIndicator.CALLING, domain: ''}; | ||
| const createRegistrationSpy = jest.spyOn(regUtils, 'createRegistration'); | ||
| // const createRegistrationSpy = jest.spyOn(regUtils, 'createRegistration'); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Registration spy assertions need to be updated for web worker changes.
The registration spy and its assertions have been commented out. Since this PR implements web workers for keepalive signals, these tests should be updated to verify the new behavior rather than being removed.
Let's check if there are any existing tests for the web worker implementation:
Also applies to: 84-91, 140-147
🏁 Script executed:
#!/bin/bash
# Description: Search for web worker related test files
# Test: Look for test files that might contain web worker tests
rg -t ts -t js "worker" . -l | grep "test"
# Description: Search for keepalive related tests
# Test: Look for test files that might contain keepalive tests
rg -t ts -t js "keepalive" . -l | grep "test"Length of output: 661
Action Required: Update Registration Spy Assertions for Web Worker Behavior
- In file
packages/calling/src/CallingClient/line/line.test.tsaround line 32 (and similarly at lines 84–91 and 140–147), the registration spy code is still commented out. - With the new web worker implementation for keepalive signals, update these tests to assert the new behavior rather than removing the assertions.
- Additionally, review and ensure consistency with related tests in files such as
packages/calling/src/CallingClient/registration/register.test.ts.
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
COMPLETES #https://jira-eng-gpk2.cisco.com/jira/browse/SPARK-617101
This pull request addresses
Adding web workers for sending keepalives
by making the following changes
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.