fix(plugin-cc): pipeline fix for wxcc #3978
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe pull request introduces significant enhancements across multiple files, primarily focusing on the Webex Contact Center functionality. Key changes include the addition of a new deployment branch in the GitHub Actions workflow, the introduction of several new classes and methods for managing agent interactions, WebSocket connections, and error handling within the contact center environment. New documentation and test coverage have been established, and existing configurations have been updated to support the new plugin structure and dependencies. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Webex
participant ContactCenter
participant WebSocket
User->>Webex: Initialize with configuration
Webex->>ContactCenter: Create instance
ContactCenter->>WebSocket: Establish connection
WebSocket->>ContactCenter: Connection established
User->>ContactCenter: Register agent
ContactCenter->>WebSocket: Send registration request
WebSocket->>ContactCenter: Acknowledge registration
ContactCenter->>User: Confirm registration
Suggested labels
Suggested reviewers
Poem
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 (
|
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
🛑 Comments failed to post (100)
packages/@webex/plugin-cc/src/logger-proxy.ts (1)
3-9: 🛠️ Refactor suggestion
Add initialization safety checks and documentation.
While the static-only class pattern is acceptable for a singleton logger, consider these improvements:
+/** + * Proxy class for centralizing logging across the Webex Contact Center plugin. + * Ensures a single source of truth for logging operations. + * + * @example + * ```typescript + * // Initialize once at application startup + * LoggerProxy.initialize(new Logger()); + * + * // Use throughout the application + * LoggerProxy.logger.info('Message'); + * ``` + */ export default class LoggerProxy { - public static logger: Logger; + private static logger?: Logger; - public static initialize(logger: Logger): void { + /** + * Initializes the logger instance. Should be called once at application startup. + * @param logger The logger instance to use + * @throws Error if attempting to initialize multiple times + */ + public static initialize(logger: Logger): void { + if (LoggerProxy.logger) { + throw new Error('Logger is already initialized'); + } LoggerProxy.logger = logger; } + + /** + * Gets the logger instance. + * @throws Error if logger is not initialized + */ + public static getLogger(): Logger { + if (!LoggerProxy.logger) { + throw new Error('Logger is not initialized'); + } + return LoggerProxy.logger; + } }The changes above:
- Make the logger property private and optional
- Add initialization checks
- Provide a safe getter method
- Add comprehensive documentation with usage examples
🧰 Tools
🪛 Biome
[error] 3-9: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
packages/@webex/plugin-cc/src/services/core/GlobalTypes.ts (1)
8-14: 🛠️ Refactor suggestion
Remove redundant fields and add constraints.
The
Failuretype contains fields that are already present in the baseMsgtype (trackingIdandorgId). Additionally, thereasonCodeandreasonfields could benefit from more specific typing.+/** + * Represents a failure message with detailed error information + */ export type Failure = Msg<{ agentId: string; - trackingId: string; // Redundant: already in Msg - reasonCode: number; - orgId: string; // Redundant: already in Msg - reason: string; + reasonCode: ErrorCode; // Add enum for valid error codes + reason: ErrorReason; // Add enum for standardized error messages }>; +/** + * Enumeration of possible error codes + */ +enum ErrorCode { + AUTHENTICATION_FAILED = 1001, + CONNECTION_ERROR = 1002, + // Add other specific error codes +} + +/** + * Enumeration of standardized error reasons + */ +type ErrorReason = + | 'Invalid credentials' + | 'Network connection failed' + | 'Service unavailable' + // Add other specific error messages + | string; // Allow custom messages if neededCommittable suggestion skipped: line range outside the PR's diff.
packages/@webex/plugin-cc/__mocks__/workerMock.js (1)
1-12: 🛠️ Refactor suggestion
Enhance Worker mock to better match the Web Worker API.
The current implementation is missing several key aspects of the Web Worker API that might be needed for comprehensive testing:
Consider applying these improvements:
class Worker { constructor(stringUrl) { + if (typeof stringUrl !== 'string') { + throw new TypeError('Worker script URL must be a string'); + } this.url = stringUrl; this.onmessage = () => {}; + this.onerror = () => {}; + this.onmessageerror = () => {}; } postMessage(msg) { - this.onmessage(msg); + this.onmessage({ + data: msg, + type: 'message', + target: this + }); } terminate() {} + + addEventListener(type, listener) { + if (type === 'message') this.onmessage = listener; + if (type === 'error') this.onerror = listener; + if (type === 'messageerror') this.onmessageerror = listener; + } + + removeEventListener(type, listener) { + if (type === 'message' && this.onmessage === listener) this.onmessage = () => {}; + if (type === 'error' && this.onerror === listener) this.onerror = () => {}; + if (type === 'messageerror' && this.onmessageerror === listener) this.onmessageerror = () => {}; + } }📝 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.class Worker { constructor(stringUrl) { if (typeof stringUrl !== 'string') { throw new TypeError('Worker script URL must be a string'); } this.url = stringUrl; this.onmessage = () => {}; this.onerror = () => {}; this.onmessageerror = () => {}; } postMessage(msg) { this.onmessage({ data: msg, type: 'message', target: this }); } terminate() {} addEventListener(type, listener) { if (type === 'message') this.onmessage = listener; if (type === 'error') this.onerror = listener; if (type === 'messageerror') this.onmessageerror = listener; } removeEventListener(type, listener) { if (type === 'message' && this.onmessage === listener) this.onmessage = () => {}; if (type === 'error' && this.onerror === listener) this.onerror = () => {}; if (type === 'messageerror' && this.onmessageerror === listener) this.onmessageerror = () => {}; } }packages/@webex/plugin-cc/src/features/constants.ts (1)
7-7: 🛠️ Refactor suggestion
Add type annotation and documentation for DEFAULT_ATTRIBUTES
The constant's purpose and expected content type are unclear. Consider:
- Adding TSDoc comment explaining its usage
- Adding TypeScript type annotation
+/** Default attributes for team/aux code configuration */ +export const DEFAULT_ATTRIBUTES: string[] = []; -export const DEFAULT_ATTRIBUTES = [];📝 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./** Default attributes for team/aux code configuration */ export const DEFAULT_ATTRIBUTES: string[] = [];packages/@webex/plugin-cc/src/services/core/constants.ts (1)
3-3: 🛠️ Refactor suggestion
Consolidate duplicate socket timeout constants
CLOSE_SOCKET_TIMEOUT_DURATIONandCLOSE_SOCKET_TIMEOUThave the same value (16000ms). Consider consolidating these into a single constant to avoid confusion and maintenance issues.export const CLOSE_SOCKET_TIMEOUT_DURATION = 16000; -export const CLOSE_SOCKET_TIMEOUT = 16000;Also applies to: 11-11
packages/@webex/plugin-cc/src/config.ts (2)
3-24: 🛠️ Refactor suggestion
Add TypeScript interfaces for type safety
The configuration object lacks type definitions, which could lead to runtime errors. Consider adding TypeScript interfaces to enforce type safety.
Add these type definitions at the top of the file:
interface MetricsConfig { clientName: string; clientType: string; } interface ServiceData { indicator: string; domain: string; } interface CallingClientConfig { logger: { level: number; }; serviceData: ServiceData; } interface CCConfig { allowMultiLogin: boolean; clientType: string; isKeepAliveEnabled: boolean; force: boolean; metrics: MetricsConfig; callingClientConfig: CallingClientConfig; } interface Config { cc: CCConfig; }Then update the export:
-export default { +export default <Config>{
19-20:
⚠️ Potential issueAddress environment-based configuration
The hardcoded domain needs to be made configurable based on the environment as noted in the TODO comment.
Consider:
- Moving this configuration to an environment-specific config file
- Using environment variables
- Implementing a configuration service that can dynamically set the domain based on the deployment environment
Example approach:
const getDomain = () => { return process.env.CC_DOMAIN || 'rtw.prod-us1.rtmsprod.net'; }; // Then in the config domain: getDomain(),packages/@webex/plugin-cc/src/services/index.ts (1)
9-12: 🛠️ Refactor suggestion
Add error handling in the constructor.
The constructor should validate the WebSocketManager parameter and handle potential initialization failures of AqmReqs.
Consider applying this improvement:
constructor(webSocketManager: WebSocketManager) { + if (!webSocketManager) { + throw new Error('WebSocketManager is required'); + } const aqmReq = new AqmReqs(webSocketManager); + if (!aqmReq) { + throw new Error('Failed to initialize AqmReqs'); + } this.agent = routingAgent(aqmReq); }Committable suggestion skipped: line range outside the PR's diff.
packages/@webex/plugin-cc/src/services/core/Utils.ts (1)
13-18:
⚠️ Potential issueImprove type safety and error handling robustness
The current implementation has several potential issues:
- Using
anytype reduces type safety- Missing null checks could cause runtime errors
- Type casting without validation could fail silently
Consider applying these improvements:
-export const getErrorDetails = (error: any, methodName: string) => { +export const getErrorDetails = (error: Error & { details?: Failure }, methodName: string) => { const failure = error.details as Failure; - LoggerProxy.logger.error(`${methodName} failed with trackingId: ${failure?.trackingId}`); + LoggerProxy.logger.error( + `${methodName} failed with trackingId: ${failure?.trackingId || 'unknown'}` + ); return new Error(failure?.data?.reason ?? `Error while performing ${methodName}`); };📝 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.export const getErrorDetails = (error: Error & { details?: Failure }, methodName: string) => { const failure = error.details as Failure; LoggerProxy.logger.error( `${methodName} failed with trackingId: ${failure?.trackingId || 'unknown'}` ); return new Error(failure?.data?.reason ?? `Error while performing ${methodName}`); };packages/@webex/plugin-cc/src/services/core/HttpRequest.ts (2)
3-13: 🛠️ Refactor suggestion
Consider adding error handling for uninitialized instance access.
The singleton pattern is well-implemented, but accessing
getInstance()without proper initialization could lead to runtime issues.Consider this improvement:
public static getInstance(options?: {webex: WebexSDK}): HttpRequest { if (!HttpRequest.instance && options && options.webex) { HttpRequest.instance = new HttpRequest(options); + } else if (!HttpRequest.instance) { + throw new Error('HttpRequest must be initialized with WebexSDK instance first'); } return HttpRequest.instance; }📝 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.class HttpRequest { private webex: WebexSDK; private static instance: HttpRequest; public static getInstance(options?: {webex: WebexSDK}): HttpRequest { if (!HttpRequest.instance && options && options.webex) { HttpRequest.instance = new HttpRequest(options); } else if (!HttpRequest.instance) { throw new Error('HttpRequest must be initialized with WebexSDK instance first'); } return HttpRequest.instance; }
20-34: 🛠️ Refactor suggestion
Enhance request method with error handling and timeout support.
The current implementation lacks error handling and request timeout configuration, which are crucial for robust HTTP requests.
Consider implementing these improvements:
public async request(options: { service: string; resource: string; method: HTTP_METHODS; body?: RequestBody; + timeout?: number; }): Promise<IHttpResponse> { - const {service, resource, method, body} = options; + const {service, resource, method, body, timeout = 30000} = options; - return this.webex.request({ - service, - resource, - method, - body, - }); + try { + return await this.webex.request({ + service, + resource, + method, + body, + timeout, + }); + } catch (error) { + // Enhance error with additional context + const enhancedError = new Error( + `Request failed for service: ${service}, resource: ${resource} - ${error.message}` + ); + enhancedError.originalError = error; + throw enhancedError; + } }📝 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.public async request(options: { service: string; resource: string; method: HTTP_METHODS; body?: RequestBody; timeout?: number; }): Promise<IHttpResponse> { const {service, resource, method, body, timeout = 30000} = options; try { return await this.webex.request({ service, resource, method, body, timeout, }); } catch (error) { // Enhance error with additional context const enhancedError = new Error( `Request failed for service: ${service}, resource: ${resource} - ${error.message}` ); enhancedError.originalError = error; throw enhancedError; } }packages/@webex/plugin-cc/src/services/core/types.ts (2)
48-48: 🛠️ Refactor suggestion
Replace void with undefined in union type.
The static analysis tool correctly flags that
voidin a union type can be confusing. Usingundefinedis more appropriate in this context.-export type CbRes<TRes> = (res: any) => void | TRes; +export type CbRes<TRes> = (res: unknown) => undefined | TRes;This change also replaces
anywithunknownfor better type safety.📝 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.export type CbRes<TRes> = (res: unknown) => undefined | TRes;🧰 Tools
🪛 Biome
[error] 48-48: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
11-15: 🛠️ Refactor suggestion
Replace 'any' with a more specific type.
Using
anyreduces type safety. Consider defining a more specific type for thedataproperty based on its intended usage.export type BindType = string | string[] | {[key: string]: BindType}; interface Bind { type: BindType; - data?: any; + data?: unknown; }📝 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.export type BindType = string | string[] | {[key: string]: BindType}; interface Bind { type: BindType; data?: unknown; }packages/@webex/plugin-cc/src/services/WebSocket/config.ts (1)
1-27:
⚠️ Potential issueAdd type safety and proper environment variable parsing
The configuration needs stronger type safety and proper parsing of environment variables:
- Values from
process.envare strings but are being used where numbers are expected- Missing TypeScript interface definition
- Incorrect JSDoc type annotation for
forceCloseDelay- The
pongTimeout(14s) should be greater thanpingInterval(15s) to avoid race conditionsApply these changes:
+interface WebSocketConfig { + pingInterval: number; + pongTimeout: number; + backoffTimeMax: number; + backoffTimeReset: number; + forceCloseDelay: number; + beforeLogoutOptionsCloseReason: string; + authorizationRequired: boolean; + acknowledgementRequired: boolean; +} -const webSocketConfig = { +const webSocketConfig: WebSocketConfig = { pingInterval: process.env.MERCURY_PING_INTERVAL - || 15000, + ? parseInt(process.env.MERCURY_PING_INTERVAL, 10) + : 15000, pongTimeout: process.env.MERCURY_PONG_TIMEOUT - || 14000, + ? parseInt(process.env.MERCURY_PONG_TIMEOUT, 10) + : 16000, // Increased to be greater than pingInterval backoffTimeMax: process.env.MERCURY_BACKOFF_TIME_MAX - || 32000, + ? parseInt(process.env.MERCURY_BACKOFF_TIME_MAX, 10) + : 32000, backoffTimeReset: process.env.MERCURY_BACKOFF_TIME_RESET - || 1000, + ? parseInt(process.env.MERCURY_BACKOFF_TIME_RESET, 10) + : 1000, /** * Milliseconds to wait for a close frame before declaring the socket dead and * discarding it - * @type {[type]} + * @type {number} */ forceCloseDelay: process.env.MERCURY_FORCE_CLOSE_DELAY - || 2000, + ? parseInt(process.env.MERCURY_FORCE_CLOSE_DELAY, 10) + : 2000,📝 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.interface WebSocketConfig { pingInterval: number; pongTimeout: number; backoffTimeMax: number; backoffTimeReset: number; forceCloseDelay: number; beforeLogoutOptionsCloseReason: string; authorizationRequired: boolean; acknowledgementRequired: boolean; } const webSocketConfig: WebSocketConfig = { /** * Milliseconds between pings sent up the socket * @type {number} */ pingInterval: process.env.MERCURY_PING_INTERVAL ? parseInt(process.env.MERCURY_PING_INTERVAL, 10) : 15000, /** * Milliseconds to wait for a pong before declaring the connection dead * @type {number} */ pongTimeout: process.env.MERCURY_PONG_TIMEOUT ? parseInt(process.env.MERCURY_PONG_TIMEOUT, 10) : 16000, // Increased to be greater than pingInterval /** * Maximum milliseconds between connection attempts * @type {Number} */ backoffTimeMax: process.env.MERCURY_BACKOFF_TIME_MAX ? parseInt(process.env.MERCURY_BACKOFF_TIME_MAX, 10) : 32000, /** * Initial milliseconds between connection attempts * @type {Number} */ backoffTimeReset: process.env.MERCURY_BACKOFF_TIME_RESET ? parseInt(process.env.MERCURY_BACKOFF_TIME_RESET, 10) : 1000, /** * Milliseconds to wait for a close frame before declaring the socket dead and * discarding it * @type {number} */ forceCloseDelay: process.env.MERCURY_FORCE_CLOSE_DELAY ? parseInt(process.env.MERCURY_FORCE_CLOSE_DELAY, 10) : 2000,packages/@webex/plugin-cc/README.md (3)
67-67:
⚠️ Potential issueSpecify the complete CDN path.
The CDN script source uses a relative path which may not work in all contexts. Consider providing the full CDN URL.
- <script src="../contact-center.min.js"></script> + <script src="https://unpkg.com/@webex/plugin-cc/dist/contact-center.min.js"></script>📝 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.<script src="https://unpkg.com/@webex/plugin-cc/dist/contact-center.min.js"></script>
16-18:
⚠️ Potential issueAddress TODO and empty documentation links.
The empty documentation links should either be populated with actual URLs or removed until the documentation is ready. Consider adding a note about when these documents will be available.
-TODO: Add the documentation links once ready -- [Introduction to the Webex Web Calling SDK]() -- [Quickstart guide](). +Note: Documentation links will be available in the upcoming release. For now, please refer to the examples below.📝 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.Note: Documentation links will be available in the upcoming release. For now, please refer to the examples below.🧰 Tools
🪛 Markdownlint
17-17: null
No empty links(MD042, no-empty-links)
18-18: null
No empty links(MD042, no-empty-links)
30-31:
⚠️ Potential issueRemove placeholder text from Building section.
The current text appears to be a template placeholder and should be replaced with actual project-specific building instructions.
-If your project needs some additional steps for the developer to build the -project after some code changes, state them here: +To build the project after making code changes, run the following commands:Committable suggestion skipped: line range outside the PR's diff.
packages/@webex/plugin-cc/test/unit/spec/services/core/HttpRequest.ts (1)
20-67: 🛠️ Refactor suggestion
Enhance test coverage with additional test cases.
The current test suite could be more comprehensive. Consider adding the following test cases:
- Verify singleton behavior (multiple getInstance calls return same instance)
- Test requests without body parameter
- Test different HTTP methods (GET, PUT, DELETE)
- Verify error logging behavior
- Use more realistic HTTP response mock
Here's an example of additional test cases:
describe('HttpRequest', () => { // ... existing setup ... describe('getInstance', () => { it('should return the same instance for multiple calls', () => { const instance1 = HttpRequest.getInstance({webex: mockWebex}); const instance2 = HttpRequest.getInstance({webex: mockWebex}); expect(instance1).toBe(instance2); }); }); describe('request', () => { // ... existing tests ... it('should handle GET request without body', async () => { const mockResponse = { statusCode: 200, body: { data: 'test' }, headers: { 'content-type': 'application/json' }, method: 'GET', url: 'https://example.com/resource', }; mockRequest.mockResolvedValueOnce(mockResponse); await httpRequest.request({ service: 'service', resource: 'resource', method: HTTP_METHODS.GET, }); expect(mockRequest).toHaveBeenCalledWith({ service: 'service', resource: 'resource', method: HTTP_METHODS.GET, }); }); it('should log error details when request fails', async () => { const mockError = new Error('Request failed'); mockRequest.mockRejectedValueOnce(mockError); await expect(httpRequest.request({ service: 'service', resource: 'resource', method: HTTP_METHODS.GET, })).rejects.toThrow('Request failed'); expect(mockWebex.logger.error).toHaveBeenCalledWith( 'HttpRequest:request - Error:', mockError ); }); }); });packages/@webex/plugin-cc/src/services/WebCallingService.ts (4)
12-21: 🛠️ Refactor suggestion
Add null checks and optional chaining for potentially undefined properties
The
callingClient,line, andcallproperties are initialized after construction but don't have optional types. This could lead to runtime errors if accessed before initialization.Consider this improvement:
- private callingClient: ICallingClient; - private line: ILine; - private call: ICall; + private callingClient?: ICallingClient; + private line?: ILine; + private call?: ICall;📝 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.export default class WebCallingService { private callingClient?: ICallingClient; private callingClientConfig: CallingClientConfig; private line?: ILine; private call?: ICall; private webex: WebexSDK; constructor(webex: WebexSDK, callingClientConfig: CallingClientConfig) { this.webex = webex; this.callingClientConfig = callingClientConfig; }
60-62: 🛠️ Refactor suggestion
Add error handling and status reporting to deregistration
The current implementation could fail silently and provides no way to confirm successful deregistration.
Consider this improvement:
- public async deregisterWebCallingLine() { + public async deregisterWebCallingLine(): Promise<void> { + if (!this.line) { + throw new Error('No line to deregister'); + } + return new Promise<void>((resolve, reject) => { + const timeout = setTimeout(() => { + reject(new Error('Deregistration timed out')); + }, TIMEOUT_DURATION); + + this.line.on(LINE_EVENTS.UNREGISTERED, () => { + clearTimeout(timeout); + resolve(); + }); + + this.line.on(LINE_EVENTS.DEREGISTER_FAILED, (error) => { + clearTimeout(timeout); + reject(error); + }); + this.line?.deregister(); - } + }); + }Committable suggestion skipped: line range outside the PR's diff.
44-58: 🛠️ Refactor suggestion
Enhance error handling and timeout management
The current implementation has several areas for improvement:
- The timeout duration is not configurable
- The error message doesn't include context about why registration failed
- The promise might never resolve/reject if registration fails without triggering timeout
Consider these improvements:
public async registerWebCallingLine(): Promise<void> { + const timeoutDuration = this.callingClientConfig.timeoutDuration || TIMEOUT_DURATION; return new Promise<void>((resolve, reject) => { const timeout = setTimeout(() => { - reject(new Error('WebCallingService Registration timed out')); + reject(new Error(`WebCallingService Registration timed out after ${timeoutDuration}ms`)); - }, TIMEOUT_DURATION); + }, timeoutDuration); + const handleError = (error: Error) => { + clearTimeout(timeout); + reject(error); + }; + + this.line.on(LINE_EVENTS.REGISTER_FAILED, handleError); this.line.on(LINE_EVENTS.REGISTERED, (deviceInfo: ILine) => { // ... existing code ... }); this.line.register(); }); }Committable suggestion skipped: line range outside the PR's diff.
31-42:
⚠️ Potential issueAdd event listener cleanup to prevent memory leaks
The incoming call event listener is added but never removed. This could lead to memory leaks and duplicate event handling if registerWebCallingLine is called multiple times.
Consider adding a cleanup method:
+ private removeEventListeners() { + this.line?.removeAllListeners(LINE_EVENTS.INCOMING_CALL); + this.line?.removeAllListeners(LINE_EVENTS.UNREGISTERED); + this.line?.removeAllListeners(LINE_EVENTS.REGISTERED); + } public async registerWebCallingLine(): Promise<void> { + this.removeEventListeners(); // ... existing code ... } public async deregisterWebCallingLine() { + this.removeEventListeners(); this.line?.deregister(); }Committable suggestion skipped: line range outside the PR's diff.
packages/webex/test/unit/spec/webex.js (1)
18-27: 🛠️ Refactor suggestion
Enhance Worker and URL mocks with better test coverage
The current mock implementations could be improved for better test coverage and cleanup:
- The Worker mock is missing important Worker features
- URL.createObjectURL uses a hardcoded URL
- No cleanup is performed after tests
Consider this enhanced implementation:
const mockWorker = { postMessage: jest.fn(), onmessage: jest.fn(), + onerror: jest.fn(), + terminate: jest.fn(), }; global.Worker = jest.fn(() => mockWorker); -global.URL.createObjectURL = function (blob) { - return 'blob:http://localhost:3000/12345'; -}; +const originalCreateObjectURL = global.URL.createObjectURL; +global.URL.createObjectURL = jest.fn((blob) => `blob:${blob.type}/${Math.random().toString(36).slice(2)}`); + +afterEach(() => { + mockWorker.postMessage.mockClear(); + mockWorker.onmessage.mockClear(); + mockWorker.onerror.mockClear(); + mockWorker.terminate.mockClear(); + global.Worker.mockClear(); + global.URL.createObjectURL.mockClear(); +}); + +afterAll(() => { + global.URL.createObjectURL = originalCreateObjectURL; +});📝 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.const mockWorker = { postMessage: jest.fn(), onmessage: jest.fn(), onerror: jest.fn(), terminate: jest.fn(), }; global.Worker = jest.fn(() => mockWorker); const originalCreateObjectURL = global.URL.createObjectURL; global.URL.createObjectURL = jest.fn((blob) => `blob:${blob.type}/${Math.random().toString(36).slice(2)}`); afterEach(() => { mockWorker.postMessage.mockClear(); mockWorker.onmessage.mockClear(); mockWorker.onerror.mockClear(); mockWorker.terminate.mockClear(); global.Worker.mockClear(); global.URL.createObjectURL.mockClear(); }); afterAll(() => { global.URL.createObjectURL = originalCreateObjectURL; });packages/@webex/plugin-cc/test/unit/spec/services/agent/index.ts (2)
41-59: 🛠️ Refactor suggestion
Improve type safety and test coverage in API tests
The stationLogin, stateChange, and buddyAgents tests have several issues:
- Using
anytype defeats TypeScript's type checking- Not validating actual request parameters
- Missing error scenarios
- Weak assertions
Example improvement for stationLogin (apply similar pattern to others):
it('stationLogin', async () => { const reqSpy = jest.spyOn(fakeAqm, 'req'); - const req = await agent.stationLogin({data: {} as any}); - expect(req).toBeDefined(); - expect(reqSpy).toHaveBeenCalled(); + const stationData = { + data: { + stationId: 'STATION123', + extension: '1234' + } + }; + + await agent.stationLogin(stationData); + + expect(reqSpy).toHaveBeenCalledWith( + expect.objectContaining({ + data: expect.objectContaining(stationData.data) + }) + ); + + // Test error scenario + reqSpy.mockRejectedValueOnce(new Error('Station login failed')); + await expect(agent.stationLogin(stationData)).rejects.toThrow('Station login failed'); });Committable suggestion skipped: line range outside the PR's diff.
26-32: 🛠️ Refactor suggestion
Enhance error handling test coverage
The logout test mocks a rejection but doesn't verify the error handling behavior. Consider testing:
- The specific error message
- Error propagation
- Cleanup operations during failure
it('logout', async () => { const reqSpy = jest.spyOn(fakeAqm, 'reqEmpty'); - reqSpy.mockRejectedValue(new Error('dasd')); + const errorMessage = 'Logout failed'; + reqSpy.mockRejectedValue(new Error(errorMessage)); - const req = await agent.logout({data: {logoutReason: 'User requested logout'}}); - expect(req).toBeDefined(); - expect(reqSpy).toHaveBeenCalled(); + await expect( + agent.logout({data: {logoutReason: 'User requested logout'}}) + ).rejects.toThrow(errorMessage); + expect(reqSpy).toHaveBeenCalledWith( + expect.objectContaining({ + data: expect.objectContaining({ + logoutReason: 'User requested logout' + }) + }) + ); });📝 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.it('logout', async () => { const reqSpy = jest.spyOn(fakeAqm, 'reqEmpty'); const errorMessage = 'Logout failed'; reqSpy.mockRejectedValue(new Error(errorMessage)); await expect( agent.logout({data: {logoutReason: 'User requested logout'}}) ).rejects.toThrow(errorMessage); expect(reqSpy).toHaveBeenCalledWith( expect.objectContaining({ data: expect.objectContaining({ logoutReason: 'User requested logout' }) }) ); });packages/@webex/plugin-cc/package.json (4)
23-23: 🛠️ Refactor suggestion
Enhance npm deployment safety
The
deploy:npmscript lacks version and authentication checks.Consider enhancing the script:
- "deploy:npm": "yarn npm publish" + "deploy:npm": "yarn build && yarn test && yarn npm publish --access public"📝 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."deploy:npm": "yarn build && yarn test && yarn npm publish --access public"
16-16:
⚠️ Potential issueFix circular dependency in build script
The build script depends on
@webex/callingpackage's build, which could lead to circular dependencies and build issues.Consider refactoring to:
- "build": " yarn workspace @webex/calling run build:src && yarn run -T tsc --declaration true --declarationDir ./dist/types", + "build": "yarn run -T tsc --declaration true --declarationDir ./dist/types"📝 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."build": "yarn run -T tsc --declaration true --declarationDir ./dist/types"
31-31:
⚠️ Potential issueMove jest-html-reporters to devDependencies
The
jest-html-reporterspackage is a testing utility and should be indevDependenciesinstead ofdependencies.- "jest-html-reporters": "3.0.11"Add to devDependencies section:
+ "jest-html-reporters": "3.0.11"Committable suggestion skipped: line range outside the PR's diff.
11-13: 💡 Codebase verification
Node.js version requirement needs alignment with ecosystem
The Node.js engine requirement of
>=20.xin@webex/plugin-ccis significantly higher than the rest of the ecosystem:
- Most @webex packages require
>=16- Some config packages require
>=18.0.0- Only this new package requires
>=20.xPlease align the Node.js version requirement with the rest of the ecosystem by setting it to
>=16unless there's a specific technical requirement for Node.js 20.🔗 Analysis chain
Verify Node.js version requirement
The Node.js engine requirement of
>=20.xseems quite high. Please verify if this is intentional and aligns with the broader Webex JS SDK ecosystem requirements.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check Node.js requirements across other packages echo "Checking Node.js requirements in other packages..." fd -e json -x jq -r 'select(.engines.node != null) | "\(.name): \(.engines.node)"' {} \;Length of output: 4453
Script:
#!/bin/bash # Let's check Node.js version requirements more specifically in package.json files echo "Checking Node.js engine requirements in root package.json..." cat package.json | jq -r '.engines.node' echo -e "\nChecking Node.js engine requirements in workspace packages..." fd package.json -t f -x sh -c 'echo "{}:" && cat {} | jq -r ".engines.node // empty"' \;Length of output: 5252
packages/@webex/plugin-cc/src/services/core/WebSocket/keepalive.worker.js (2)
76-85: 🛠️ Refactor suggestion
Add error handling and prevent duplicate network checks
The network event listeners could race with the interval-based checks. Consider:
- Adding debouncing to prevent rapid consecutive checks
- Adding error handling for network events
- Standardizing the logging format
+let lastCheckTimestamp = 0; +const DEBOUNCE_INTERVAL = 1000; // 1 second +const debouncedNetworkCheck = () => { + const now = Date.now(); + if (now - lastCheckTimestamp > DEBOUNCE_INTERVAL) { + lastCheckTimestamp = now; + checkNetworkStatus(); + } +}; -self.addEventListener('online', () => { +self.addEventListener('online', (event) => { + try { console.log('Network status: online'); - checkNetworkStatus(); + debouncedNetworkCheck(); + } catch (error) { + console.error('Error handling online event:', error); + } });Committable suggestion skipped: line range outside the PR's diff.
25-49: 🛠️ Refactor suggestion
⚠️ Potential issueRefactor checkNetworkStatus for better separation of concerns
The
checkNetworkStatusfunction handles multiple responsibilities: checking status, managing timeouts, and controlling WebSocket closure. Consider breaking this into smaller, focused functions.Also, there's an inconsistency between the comment mentioning "16s timeout" and the actual timeout value of 5000ms (5s) set in the message handler.
-// Sets a timeout of 16s, checks if socket didn't close then it closes forcefully +// Sets a timeout to force close the socket if it hasn't closed naturallyCommittable suggestion skipped: line range outside the PR's diff.
packages/@webex/plugin-cc/src/services/config/types.ts (1)
20-22:
⚠️ Potential issueFix duplicate event value for buddy agents events.
The event value 'BuddyAgents' is used for both
AGENT_BUDDY_AGENTSandAGENT_BUDDY_AGENTS_SUCCESS. This could cause confusion in event handling logic.AGENT_BUDDY_AGENTS: 'BuddyAgents', - AGENT_BUDDY_AGENTS_SUCCESS: 'BuddyAgents', + AGENT_BUDDY_AGENTS_SUCCESS: 'BuddyAgentsSuccess', AGENT_BUDDY_AGENTS_RETRIEVE_FAILED: 'BuddyAgentsRetrieveFailed',📝 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.AGENT_BUDDY_AGENTS: 'BuddyAgents', AGENT_BUDDY_AGENTS_SUCCESS: 'BuddyAgentsSuccess', AGENT_BUDDY_AGENTS_RETRIEVE_FAILED: 'BuddyAgentsRetrieveFailed',packages/@webex/plugin-cc/src/services/agent/types.ts (3)
43-59: 🛠️ Refactor suggestion
Improve consistency and documentation of agent states
The current implementation has several issues:
AgentStatetype allows any string, reducing type safety- Inconsistent state definitions across types
- Missing documentation for valid states
+/** + * Represents all possible agent states in the system. + * - Available: Agent is ready to receive new interactions + * - Idle: Agent is logged in but not ready for interactions + * - RONA: Agent has not responded to an interaction + */ -export type AgentState = 'Available' | 'Idle' | 'RONA' | string; +export type AgentState = 'Available' | 'Idle' | 'RONA' | 'Busy' | 'NotResponding'; export type StateChangeSuccess = Msg<{ // ... other fields - subStatus: 'Available' | 'Idle'; + subStatus: AgentState; // ... remaining fields }>; export type StateChange = { - state: AgentState; + state: Exclude<AgentState, 'RONA'>; // RONA is system-set, not user-settable // ... remaining fields };Also applies to: 94-101
92-92: 🛠️ Refactor suggestion
Strengthen login-related type definitions
Several improvements needed:
- Expand logout reasons
- Make required fields explicit in UserStationLogin
- Restrict DeviceType to known values
+/** All possible logout reasons in the system */ +type LogoutReason = + | 'User requested logout' + | 'Inactivity Logout' + | 'System Logout' + | 'Session Expired' + | 'Force Logout'; -export type Logout = {logoutReason?: 'User requested logout' | 'Inactivity Logout'}; +export type Logout = {logoutReason?: LogoutReason}; export type UserStationLogin = { + // Required fields should be non-optional teamId: string | null; teamName: string | null; siteId: string; usesOtherDN: boolean; auxCodeId: string; + // Optional fields dialNumber?: string | null; // ... remaining optional fields }; -export type DeviceType = LoginOption | string; +export type DeviceType = LoginOption | 'DESK_PHONE' | 'SOFT_PHONE';Also applies to: 103-121
123-147: 🛠️ Refactor suggestion
Strengthen buddy agent type definitions
The buddy agent types need improvements in type safety and consistency:
- Define specific media types
- Use consistent state types
- Restrict message types
+/** Supported media types for buddy agents */ +type MediaType = 'voice' | 'chat' | 'email' | 'social'; export type BuddyAgents = { agentProfileId: string; - mediaType: string; + mediaType: MediaType; - state?: string; + state?: AgentState; }; export type BuddyDetails = { agentId: string; - state: string; + state: AgentState; // ... remaining fields }; export type BuddyAgentsSuccess = Msg<{ - eventType: string; + eventType: 'BuddyAgentMessage'; // ... other fields - type: string; + type: 'BuddyAgentsSuccess'; agentList: Array<BuddyDetails>; }>;Committable suggestion skipped: line range outside the PR's diff.
tooling/lib/build.js (1)
66-66: 💡 Codebase verification
⚠️ The cleanup for webexcc.min. files might be premature*Based on the verification results:
- The cleanup pattern for
webexcc.min.*files is added alongside other package cleanups- However, there's no evidence of webexcc-related source files, webpack configurations, or sample files that would generate these artifacts
- The only reference to webexcc is the cleanup command itself in
tooling/lib/build.jsThis cleanup might be premature or unnecessary without the corresponding build setup for webexcc files. Consider implementing the build configuration first or removing this cleanup if webexcc bundle generation is not planned.
🔗 Analysis chain
LGTM! The cleanup pattern follows existing conventions.
The addition of
webexcc.min.*cleanup is consistent with the existing patterns for other packages and aligns with the PR's objective to fix the wxcc package pipeline.Let's verify the build process and cleanup patterns:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the cleanup patterns and build artifacts # Check for consistency in cleanup patterns and verify if webexcc files are generated # Check all cleanup patterns in the build script rg "rimraf.*\.min\." tooling/lib/build.js # Check if webexcc build artifacts exist in docs/samples fd "webexcc.min" docs/samples/ # Check webpack configuration for webexcc entries rg "webexcc" webpack.config.jsLength of output: 288
Script:
#!/bin/bash # Let's verify the webpack configuration and build setup for webexcc # Check webpack configuration files recursively fd -e config.js -x rg -l "webexcc" # Check package.json for webexcc related entries rg "webexcc" package.json # Check for any webexcc related source files fd -e js -e ts webexcc # Check build scripts for webexcc references rg -g "*.js" "webexcc" tooling/Length of output: 184
Script:
#!/bin/bash # Let's check for any sample files or documentation related to webexcc # Check for webexcc samples fd -t f -p "**/*webexcc*" docs/samples/ # Check for any documentation mentioning webexcc rg -g "*.md" -i "webexcc|contact center" docs/ # Check for any entry points or source files that might generate webexcc bundles rg -g "*.js" -l "webexcc" src/ # Check for any test files related to webexcc rg -g "*.js" -l "webexcc" test/Length of output: 536
packages/@webex/plugin-cc/test/unit/spec/services/WebCallingService.ts (1)
135-146: 🛠️ Refactor suggestion
Enhance deregistration test coverage.
- The direct property access using
['line']could be improved- The test could be more comprehensive by:
- Verifying the line is actually deregistered
- Testing error scenarios
- Checking cleanup of event listeners
Consider these improvements:
describe('deregisterWebCallingLine', () => { beforeEach(() => { // Setup line through proper initialization webRTCCalling.registerWebCallingLine(); }); it('should deregister the web calling line successfully', async () => { await webRTCCalling.deregisterWebCallingLine(); expect(line.deregister).toHaveBeenCalled(); // Verify cleanup expect(line.on).not.toHaveBeenCalled(); }); it('should handle deregistration errors', async () => { line.deregister.mockRejectedValue(new Error('Deregistration failed')); await expect(webRTCCalling.deregisterWebCallingLine()).rejects.toThrow('Deregistration failed'); }); });docs/samples/contact-center/index.html (3)
102-126: 🛠️ Refactor suggestion
Improve form accessibility and validation
Several improvements needed:
- Add ARIA labels for form controls
- Add input validation for dial number
- Move inline styles to CSS
- <div style="display: flex; gap: 1rem;"> + <div class="agent-controls-container"> - <fieldset style="flex: 0.69;"> + <fieldset class="team-selection">Add to style.css:
.agent-controls-container { display: flex; gap: 1rem; } .team-selection { flex: 0.69; }For the dial number input:
- <input id="dialNumber" name="dialNumber" placeholder="Dial Number" value="" type="text"> + <input id="dialNumber" name="dialNumber" placeholder="Dial Number" value="" type="tel" + pattern="[0-9]+" aria-label="Dial Number" required>
6-6:
⚠️ Potential issueFix incorrect page title
The page title indicates "Meetings Plugin" but this is actually a Contact Center sample application.
- <title>Webex JavaScript SDK Sample: Meetings Plugin</title> + <title>Webex JavaScript SDK Sample: Contact Center Plugin</title>📝 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.<title>Webex JavaScript SDK Sample: Contact Center Plugin</title>
60-60:
⚠️ Potential issueEnhance security for access token input
The access token input should be treated securely:
- Use
type="password"to mask the token- Add
autocomplete="off"to prevent browser storage- <input id="access-token" name="accessToken" placeholder="Access Token" value="" type="text"> + <input id="access-token" name="accessToken" placeholder="Access Token" value="" type="password" autocomplete="off">📝 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.<input id="access-token" name="accessToken" placeholder="Access Token" value="" type="password" autocomplete="off">packages/@webex/plugin-cc/test/unit/spec/services/core/WebSocket/connection-service.ts (1)
127-164: 🛠️ Refactor suggestion
Enhance reconnection test coverage
Consider adding the following test scenarios to make the reconnection testing more robust:
- Multiple consecutive reconnection attempts
- Reconnection with different network states (switching between online/offline)
- Maximum retry limit scenarios
- Race conditions between reconnect attempts
Example test case:
it('should handle multiple consecutive reconnection attempts', async () => { Object.defineProperty(global, 'navigator', { value: { onLine: true }, configurable: true, }); // Simulate multiple connection failures mockWebSocketManager.initWebSocket .mockRejectedValueOnce(new Error('Connection failed')) .mockRejectedValueOnce(new Error('Connection failed')) .mockResolvedValueOnce({}); await connectionService['handleSocketClose'](); await connectionService['handleSocketClose'](); await connectionService['handleSocketClose'](); expect(mockWebSocketManager.initWebSocket).toHaveBeenCalledTimes(3); expect(LoggerProxy.logger.error).toHaveBeenCalledTimes(2); });docs/samples/contact-center/style.css (3)
56-60: 🛠️ Refactor suggestion
Add responsive design considerations
The current implementation uses fixed widths and lacks media queries. Consider adding responsive breakpoints and fluid units.
:root { + /* Breakpoints */ + --breakpoint-sm: 640px; + --breakpoint-md: 768px; + --breakpoint-lg: 1024px; } .box { - max-width: 70rem; + max-width: min(70rem, 100% - 2rem); margin-inline: auto; } .media-button { flex: 0.5; - width: 23rem; + width: min(23rem, 100%); } +@media (max-width: 768px) { + .container { + flex-direction: column; + } +}Also applies to: 361-363
20-35: 🛠️ Refactor suggestion
Remove duplicated button styles
There's duplicate code for disabled button states. Consider consolidating these styles.
-button.btn--green:disabled { - opacity: 0.5; -} -button.btn--red:disabled { - opacity: 0.5; -} -.btn--red:disabled, -.btn--green:disabled { +button[disabled] { opacity: 0.5; }📝 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.button.btn--red { color: #fff; color: var(--md-button-cancel-text-color, #fff); background-color: #f7644a; background-color: var(--md-button-cancel-bg-color, #f7644a); border-color: transparent; } button[disabled] { opacity: 0.5; }
154-180: 🛠️ Refactor suggestion
Implement z-index management system
The current z-index values lack a clear system. Consider implementing a z-index management system using CSS variables to prevent stacking context issues.
:root { + /* Z-index layers */ + --z-index-base: 0; + --z-index-overlay: 1; + --z-index-controls: 2; } .multistream-remote-video { - z-index: 0; + z-index: var(--z-index-base); } .video-overlay { - z-index: 1; + z-index: var(--z-index-overlay); } .video-label { - z-index: 2; + z-index: var(--z-index-controls); }Committable suggestion skipped: line range outside the PR's diff.
packages/@webex/plugin-cc/test/unit/spec/services/config/index.ts (3)
34-78: 🛠️ Refactor suggestion
Add edge cases to getUserUsingCI tests
The current tests cover basic success/error scenarios, but consider adding:
it('should handle empty response body', async () => { const mockResponse = { statusCode: 200, body: {}, }; (mockHttpRequest.request as jest.Mock).mockResolvedValue(mockResponse); const result = await agentConfigService.getUserUsingCI(); expect(result).toEqual({}); }); it('should validate required fields in response', async () => { const mockResponse = { statusCode: 200, body: { // missing required fields email: '[email protected]', }, }; (mockHttpRequest.request as jest.Mock).mockResolvedValue(mockResponse); await expect(agentConfigService.getUserUsingCI()).rejects.toThrow(); });
132-157:
⚠️ Potential issueAdd parameter validation and URL encoding
The current implementation might be vulnerable to URL injection through unencoded parameters:
- Add parameter validation:
it('should validate pagination parameters', async () => { const invalidPage = -1; const invalidPageSize = 0; await expect(agentConfigService.getListOfTeams( invalidPage, invalidPageSize, filter, attributes )).rejects.toThrow(); });
- Ensure parameters are properly encoded in the service implementation:
const encodedFilter = encodeURIComponent(`id=in=${filter}`); const encodedAttributes = encodeURIComponent(attributes.join(','));
107-116: 🛠️ Refactor suggestion
Maintain consistent error handling patterns
The error handling approach varies between test methods. Consider using
expect().rejects.toThrow()consistently across all error tests:- try { - await agentConfigService.getDesktopProfileById(desktopProfileId); - } catch (error) { - expect(error).toEqual(mockError); - } + await expect(agentConfigService.getDesktopProfileById(desktopProfileId)) + .rejects.toEqual(mockError);📝 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.it('should throw an error if the API call fails', async () => { const mockError = new Error('API call failed'); (mockHttpRequest.request as jest.Mock).mockRejectedValue(mockError); await expect(agentConfigService.getDesktopProfileById(desktopProfileId)) .rejects.toEqual(mockError); });packages/@webex/plugin-cc/test/unit/spec/services/core/WebSocket/WebSocketManager.ts (3)
201-201: 🛠️ Refactor suggestion
Use Jest timer mocks instead of real timeouts
Replace real timeouts with Jest timer mocks for more reliable tests. This will make the tests run faster and more deterministically.
// At the start of relevant test cases: jest.useFakeTimers(); // Replace await new Promise((resolve) => setTimeout(resolve, 10)); // With jest.advanceTimersByTime(10);Also applies to: 306-306, 335-335, 365-365
103-105: 🛠️ Refactor suggestion
Restore console methods after tests
Console methods are mocked but not restored after tests. This could affect other test suites. Consider using
afterEachto restore them.beforeEach(() => { console.log = jest.fn(); console.error = jest.fn(); }); + afterEach(() => { + console.log = global.console.log; + console.error = global.console.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.console.log = jest.fn(); console.error = jest.fn(); }); afterEach(() => { console.log = global.console.log; console.error = global.console.error; });
98-102: 🛠️ Refactor suggestion
Replace setTimeout with synchronous execution
Using
setTimeoutin test setup can make tests flaky. Consider moving this logic to individual tests where needed or use Jest's timer mocks for better control.- setTimeout(() => { - MockWebSocket.inst.onopen(); - MockWebSocket.inst.onmessage({ data: JSON.stringify({ type: "Welcome" }) }); - }, 1); + MockWebSocket.inst.onopen(); + MockWebSocket.inst.onmessage({ data: JSON.stringify({ type: "Welcome" }) });📝 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.MockWebSocket.inst.onopen(); MockWebSocket.inst.onmessage({ data: JSON.stringify({ type: "Welcome" }) });packages/@webex/internal-plugin-mercury/src/socket/socket-base.js (1)
266-277:
⚠️ Potential issueFix potential race condition in authorization bypass
When bypassing authorization, the socket's
onclosehandler is not being set, which could lead to unhandled close events. This differs from the authorized flow wheresocket.onclose = this.oncloseis explicitly set.Apply this fix:
if (this.authorizationRequired) { this._authorize() .then(() => { this.logger.info(`socket,${this._domain}: authorized`); socket.onclose = this.onclose; resolve(); }) .catch(reject); } else { + socket.onclose = this.onclose; this._ping(); + resolve(); }📝 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.// Added the "authorizationRequired" condition to bypass the "_authorize" in case of the contact center context, in which case it is configured as false. if (this.authorizationRequired) { this._authorize() .then(() => { this.logger.info(`socket,${this._domain}: authorized`); socket.onclose = this.onclose; resolve(); }) .catch(reject); } else { socket.onclose = this.onclose; this._ping(); resolve(); }packages/@webex/internal-plugin-mercury/src/mercury.js (1)
519-527:
⚠️ Potential issueFix duplicate event emission for namespaced events.
There's a duplicate event emission for namespaced events:
- Line 523 emits
event:${namespace}- Line 525 emits the same event again
Apply this fix to remove the duplicate emission:
if (data.eventType) { const [namespace] = data.eventType.split('.'); if (namespace === data.eventType) { this._emit(`event:${namespace}`, envelope); } else { - this._emit(`event:${namespace}`, envelope); this._emit(`event:${data.eventType}`, envelope); } }📝 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.if (data.eventType) { const [namespace] = data.eventType.split('.'); if (namespace === data.eventType) { this._emit(`event:${namespace}`, envelope); } else { this._emit(`event:${data.eventType}`, envelope); }packages/@webex/plugin-cc/test/unit/spec/features/AgentConfig.ts (2)
169-507: 🛠️ Refactor suggestion
Add test cases for invalid access code values
While the tests cover 'ALL' and 'SPECIFIC' values for accessWrapUpCode and accessIdleCode, there are no tests for invalid values.
Add test cases for invalid access code values:
it('should handle invalid accessWrapUpCode value', async () => { const invalidDesktopProfileResponse: DesktopProfileResponse = { loginVoiceOptions: ['option1'], accessWrapUpCode: 'INVALID' as any, // Type assertion to test invalid value accessIdleCode: 'ALL', wrapUpCodes: [], idleCodes: [], }; getDesktopProfileByIdSpy.mockResolvedValue(invalidDesktopProfileResponse); await expect(agentConfig.getAgentProfile()).rejects.toThrow( 'Invalid accessWrapUpCode value: INVALID' ); });
128-131:
⚠️ Potential issueFix incomplete spy assertions
The spy assertions are incomplete and won't verify the actual arguments passed to the methods.
Add the expected arguments to the assertions:
- expect(getUserUsingCISpy).toHaveBeenCalledOnceWith; - expect(getDesktopProfileByIdSpy).toHaveBeenCalledOnceWith; - expect(getListOfTeamsSpy).toHaveBeenCalledOnceWith; - expect(getListOfAuxCodesSpy).toHaveBeenCalledOnceWith; + expect(getUserUsingCISpy).toHaveBeenCalledWith('agent123'); + expect(getDesktopProfileByIdSpy).toHaveBeenCalledWith('profile123'); + expect(getListOfTeamsSpy).toHaveBeenCalledWith(['team1', 'team2']); + expect(getListOfAuxCodesSpy).toHaveBeenCalledWith();This issue is repeated in other test cases as well (lines 245-248, 330-333, 415-418, 500-503). Please fix all occurrences.
Committable suggestion skipped: line range outside the PR's diff.
packages/@webex/plugin-cc/test/unit/spec/services/core/aqm-reqs.ts (2)
109-329: 🛠️ Refactor suggestion
Improve test structure and error handling in notification tests.
Several improvements can be made to enhance the test suite:
- Empty catch blocks should verify error scenarios
- Duplicate test configuration should be extracted
- Magic numbers should be constants
+const TEST_TIMEOUT = 1000; +const TEST_CONFIG = { + url: '/url', + timeout: 4000, + baseNotifConfig: { + type: 'RoutingMessage', + interactionId: '6920dda3-337a-48b1-b82d-2333392f9906' + } +}; describe('Aqm notifs', () => { + const createTestConfig = (type) => ({ + ...TEST_CONFIG, + notifSuccess: { + bind: { + type: TEST_CONFIG.baseNotifConfig.type, + data: { + type: 'AgentConsultCreated', + interactionId: TEST_CONFIG.baseNotifConfig.interactionId + } + }, + msg: {} + }, + // ... other notification configs + }); it('AqmReqs notifcancel', async () => { // ... test implementation using createTestConfig }); });Also, improve error handling in catch blocks:
try { await req({}); } catch (e) { + expect(e).toBeInstanceOf(Error); + expect(e.message).toMatch(/expected error pattern/); }Committable suggestion skipped: line range outside the PR's diff.
80-107: 🛠️ Refactor suggestion
Enhance error handling in the test case.
The current test case doesn't properly verify the error scenario. Consider:
- Adding specific error type checking
- Verifying the error message
- Adding test description that clearly states the expected behavior
- it('AqmReqs should be defined', async () => { + it('should handle API request with proper error handling', async () => { httpRequestInstance.request.mockResolvedValueOnce(mockHttpRequestResolvedValue); const req = aqm.req(() => ({ url: '/url', timeout: 2000, notifSuccess: { bind: { type: 'RoutingMessage', data: {type: 'AgentConsultConferenced', interactionId: 'intrid'}, }, msg: {}, }, notifFail: { bind: { type: 'RoutingMessage', data: {type: 'AgentConsultConferenceFailed'}, }, errId: 'Service.aqm.contact.consult', }, })); try { await req({}); } catch (e) { - expect(e).toBeDefined(); + expect(e).toBeInstanceOf(Error); + expect(e.message).toContain('Service.aqm.contact.consult'); } });📝 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.it('should handle API request with proper error handling', async () => { httpRequestInstance.request.mockResolvedValueOnce(mockHttpRequestResolvedValue); const req = aqm.req(() => ({ url: '/url', timeout: 2000, notifSuccess: { bind: { type: 'RoutingMessage', data: {type: 'AgentConsultConferenced', interactionId: 'intrid'}, }, msg: {}, }, notifFail: { bind: { type: 'RoutingMessage', data: {type: 'AgentConsultConferenceFailed'}, }, errId: 'Service.aqm.contact.consult', }, })); try { await req({}); } catch (e) { expect(e).toBeInstanceOf(Error); expect(e.message).toContain('Service.aqm.contact.consult'); } });packages/@webex/internal-plugin-mercury/test/unit/spec/socket.js (1)
203-243:
⚠️ Potential issueCritical issues in authorization test cases.
The test cases for
authorizationRequiredoption have several issues:
- Syntax error: Missing promise chain in the first test case.
- Unused variable
sin both test cases.- Typo in the test title "calles" instead of "calls".
- Inconsistent test setup between the two cases.
Fix the syntax and consistency issues:
-it('accepts authorizationRequired option as false and skip authorize', () => { +it('skips authorization when authorizationRequired is false', () => { - const s = new Socket(); const authorizeSpy = sinon.spy(socket, '_authorize'); - socket.open('ws://example.com', { + return socket.open('ws://example.com', { - forceCloseDelay: mockoptions.forceCloseDelay, - pingInterval: mockoptions.pingInterval, - pongTimeout: mockoptions.pongTimeout, - logger: console, - token: 'mocktoken', - trackingId: 'mocktrackingid', - logLevelToken: 'mocklogleveltoken', + ...mockoptions, authorizationRequired: false + }).then(() => { + mockWebSocket.readyState = 1; + mockWebSocket.emit('open'); + assert.notCalled(authorizeSpy); + assert.called(socket._ping); }); +}); - mockWebSocket.readyState = 1; - mockWebSocket.emit('open'); - - assert.notCalled(authorizeSpy); - assert.called(socket._ping); -}); -it('accepts authorizationRequired option as true and calles authorize', () => { - const s = new Socket(); +it('calls authorize when authorizationRequired is true', () => { const authorizeSpy = sinon.spy(socket, '_authorize'); - socket.open('ws://example.com', { + return socket.open('ws://example.com', { - forceCloseDelay: mockoptions.forceCloseDelay, - pingInterval: mockoptions.pingInterval, - pongTimeout: mockoptions.pongTimeout, - logger: console, - token: 'mocktoken', - trackingId: 'mocktrackingid', - logLevelToken: 'mocklogleveltoken', + ...mockoptions, authorizationRequired: true + }).then(() => { + mockWebSocket.readyState = 1; + mockWebSocket.emit('open'); + assert.called(authorizeSpy); + assert.called(socket._ping); }); - - mockWebSocket.readyState = 1; - mockWebSocket.emit('open'); - - assert.called(authorizeSpy); - assert.called(socket._ping); });📝 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.it('skips authorization when authorizationRequired is false', () => { const authorizeSpy = sinon.spy(socket, '_authorize'); return socket.open('ws://example.com', { ...mockoptions, authorizationRequired: false }).then(() => { mockWebSocket.readyState = 1; mockWebSocket.emit('open'); assert.notCalled(authorizeSpy); assert.called(socket._ping); }); }); it('calls authorize when authorizationRequired is true', () => { const authorizeSpy = sinon.spy(socket, '_authorize'); return socket.open('ws://example.com', { ...mockoptions, authorizationRequired: true }).then(() => { mockWebSocket.readyState = 1; mockWebSocket.emit('open'); assert.called(authorizeSpy); assert.called(socket._ping); }); });packages/@webex/plugin-cc/src/services/WebSocket/index.ts (3)
13-13:
⚠️ Potential issueAdjust the type of 'webSocketUrl' to allow 'undefined'
The property
webSocketUrlis declared asstring, but it's set toundefinedindisconnectWebSocket(). Assigningundefinedto a variable typed asstringwill cause a TypeScript error.Apply this diff to update the type:
-private webSocketUrl: string; +private webSocketUrl?: string;This change allows
webSocketUrlto bestringorundefined, aligning with its usage in the class.Also applies to: 65-65
37-41: 🛠️ Refactor suggestion
Update 'connectWebSocket' to return a Promise
The
connectmethod fromMercuryis likely asynchronous and returns a Promise. TheconnectWebSocketmethod currently has a return type ofvoid, which might lead to missed opportunities for error handling or synchronization.Apply this diff to update the return type and properly handle the Promise:
-connectWebSocket(options: {webSocketUrl: string}): void { +connectWebSocket(options: {webSocketUrl: string}): Promise<void> { const {webSocketUrl} = options; this.webSocketUrl = webSocketUrl; - this.connect(webSocketUrl); + return this.connect(webSocketUrl); }This change ensures that any calling code can handle the asynchronous nature of the connection process.
📝 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.connectWebSocket(options: {webSocketUrl: string}): Promise<void> { const {webSocketUrl} = options; this.webSocketUrl = webSocketUrl; return this.connect(webSocketUrl); }
6-6:
⚠️ Potential issueAvoid using 'any' when extending classes
Casting
Mercurytoanydefeats the purpose of TypeScript's type safety. This can mask potential issues and make the code harder to maintain. Consider properly typingMercuryto maintain type safety.Apply this diff to remove the
anycast:-class WebSocket extends (Mercury as any) implements IWebSocket { +class WebSocket extends Mercury implements IWebSocket {If there are type incompatibilities, you might need to adjust the
Mercuryclass's typings or use a more specific interface instead ofany.Committable suggestion skipped: line range outside the PR's diff.
packages/@webex/plugin-cc/src/services/core/Err.ts (3)
62-62:
⚠️ Potential issueTypographical Error in Constructor Parameter Name
There's a typo in the constructor parameter name
errror. It should beerror.Apply this diff to fix the typo:
- constructor(id: IdsMessage, errror: Error); + constructor(id: IdsMessage, error: 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.constructor(id: IdsMessage, error: Error);
83-97: 🛠️ Refactor suggestion
Align Error Class Extension with Best Practices
Similar to the
Messageclass, theDetailsclass should pass a message tosuper()and set the prototype explicitly. This ensures proper error handling and compatibility withinstanceofchecks.Consider updating the constructor as follows:
constructor(id: T, details: IdsDetailsType[T]) { - super(); + super(typeof details === 'string' ? details : ''); + Object.setPrototypeOf(this, new.target.prototype); this.id = id; - this.stack = new Error().stack!; this.details = details; }📝 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.export class Details<T extends IdsDetails> extends Error { readonly id: Id; readonly details: IdsDetailsType[T]; constructor(id: T, details: IdsDetailsType[T]) { super(typeof details === 'string' ? details : ''); Object.setPrototypeOf(this, new.target.prototype); this.id = id; this.details = details; } // Marker to distinct Err class from other errors private isErr = 'yes'; }
63-77: 🛠️ Refactor suggestion
Properly Extend the Error Class in TypeScript
When extending the
Errorclass, it's recommended to pass the message tosuper()and set the prototype explicitly. This ensures that the error behaves correctly, especially when using features likeinstanceof.Consider updating the constructor as follows:
constructor(id: IdsMessage, value?: string | Error) { - super(); + super(typeof value === 'string' ? value : value?.message || ''); + Object.setPrototypeOf(this, new.target.prototype); this.id = id; - this.stack = new Error().stack!; - if (typeof value === 'string') { - this.message = value; - } else if (value instanceof Error) { - this.message = value.message; - this.name = value.name; - } else { - this.message = ''; - } + if (value instanceof Error) { + this.name = value.name || this.name; + } }📝 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.constructor(id: IdsMessage, value?: string | Error) { super(typeof value === 'string' ? value : value?.message || ''); Object.setPrototypeOf(this, new.target.prototype); this.id = id; if (value instanceof Error) { this.name = value.name || this.name; } }packages/@webex/plugin-cc/src/services/agent/index.ts (1)
62-65:
⚠️ Potential issueSafely access 'trackingId' to prevent runtime errors
Accessing
e.response?.headers?.trackingid?.split('_')[1]might cause an error iftrackingidis undefined or does not contain an underscore. Consider adding checks to ensuretrackingidis defined and properly formatted before splitting to prevent potential runtime errors.Apply this diff to safely access
trackingId:- trackingId: e.response?.headers?.trackingid?.split('_')[1], + trackingId: e.response?.headers?.trackingid + ? e.response.headers.trackingid.split('_')[1] + : undefined,📝 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.new Err.Details('Service.aqm.agent.stationLogin', { status: e.response?.status ?? 0, type: e.response?.data?.errorType, trackingId: e.response?.headers?.trackingid ? e.response.headers.trackingid.split('_')[1] : undefined,packages/@webex/plugin-cc/src/features/Agentconfig.ts (3)
131-134: 🛠️ Refactor suggestion
Simplify
asyncfunction return and error handlingWithin an
asyncfunction, you can return values directly and throw errors instead of usingPromise.resolveandPromise.reject. This simplifies the code and aligns with standardasync/awaitpractices.Apply this diff to simplify the return statements:
- return Promise.resolve(this.agentProfile); + return this.agentProfile; ... - return Promise.reject(new Error(`Fetching Agent Profile failed: ${error.message}`)); + throw new Error(`Fetching Agent Profile failed: ${error.message}`);📝 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.return this.agentProfile; } catch (error) { throw new Error(`Fetching Agent Profile failed: ${error.message}`); }
64-70:
⚠️ Potential issuePotential issue when adding aux codes to the filter
In the condition starting at line 64, you're pushing
agentDesktopProfile.wrapUpCodesandagentDesktopProfile.idleCodesintoauxCodeFilter. If these properties are arrays, usingpushwill add them as nested arrays, which may cause issues when theauxCodeFilteris used later. Consider using the spread operator to flatten the arrays intoauxCodeFilter.Apply this diff to fix the issue:
if ( agentDesktopProfile.accessWrapUpCode !== 'ALL' && agentDesktopProfile.accessIdleCode !== 'ALL' ) { - auxCodeFilter.push(agentDesktopProfile.wrapUpCodes); - auxCodeFilter.push(agentDesktopProfile.idleCodes); + auxCodeFilter.push(...agentDesktopProfile.wrapUpCodes); + auxCodeFilter.push(...agentDesktopProfile.idleCodes); }📝 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.if ( agentDesktopProfile.accessWrapUpCode !== 'ALL' && agentDesktopProfile.accessIdleCode !== 'ALL' ) { auxCodeFilter.push(...agentDesktopProfile.wrapUpCodes); auxCodeFilter.push(...agentDesktopProfile.idleCodes); }
97-105: 🛠️ Refactor suggestion
Avoid mutating data returned from API calls
Modifying
auxCodesList.datadirectly by pushing a new object can lead to side effects if the original data is used elsewhere in the application. It's safer to create a new array to prevent unintended mutations.Apply this diff to avoid mutating the API data:
- auxCodesList.data.push({ + const auxCodesDataWithAvailableState = [...auxCodesList.data, { id: AGENT_STATE_AVAILABLE_ID, active: true, defaultCode: true, name: AGENT_STATE_AVAILABLE, isSystemCode: false, workTypeCode: WORK_TYPE_CODE.IDLE_CODE, description: AGENT_STATE_AVAILABLE_DESCRIPTION, - }); ... - this.agentProfile.idleCodes = auxCodesList.data; + }]; + + // Use the new array for further processing + this.agentProfile.idleCodes = auxCodesDataWithAvailableState.filter( + (auxCode) => auxCode.workTypeCode === WORK_TYPE_CODE.IDLE_CODE + ); + this.agentProfile.wrapUpCodes = auxCodesDataWithAvailableState.filter( + (auxCode) => auxCode.workTypeCode === WORK_TYPE_CODE.WRAP_UP_CODE + );Committable suggestion skipped: line range outside the PR's diff.
packages/@webex/plugin-cc/src/services/core/WebSocket/connection-service.ts (4)
101-101:
⚠️ Potential issueCheck for the existence of
keepaliveproperty before usage.The
parsedEventobject may not always have thekeepaliveproperty, which could lead to unexpected behavior. Safely access the property to prevent potential errors.Apply this diff to safely access
keepalive:-this.isKeepAlive = parsedEvent.keepalive === 'true'; +this.isKeepAlive = parsedEvent?.keepalive === 'true';📝 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.this.isKeepAlive = parsedEvent?.keepalive === 'true';
93-94:
⚠️ Potential issueAdd error handling for JSON parsing.
Parsing the message without error handling may lead to uncaught exceptions if the message is not valid JSON. Consider wrapping
JSON.parse(msg)in a try-catch block to handle potential errors gracefully.Apply this diff to add error handling:
-const parsedEvent = JSON.parse(msg); +let parsedEvent; +try { + parsedEvent = JSON.parse(msg); +} catch (error) { + LoggerProxy.logger.error(`Failed to parse message: ${msg}`, error); + return; // Optionally handle the error or return early +}📝 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.const msg = (event as CustomEvent<string>).detail; let parsedEvent; try { parsedEvent = JSON.parse(msg); } catch (error) { LoggerProxy.logger.error(`Failed to parse message: ${msg}`, error); return; // Optionally handle the error or return early }
133-134:
⚠️ Potential issueHandle errors appropriately in async functions.
Throwing an error inside an async function without proper handling can lead to unhandled promise rejections. Instead of throwing an error, consider rejecting the promise or handling the error within the function.
Apply this diff to handle the error properly:
-throw new Error('event=socketConnectionRetry | browser network not available'); +LoggerProxy.logger.error('event=socketConnectionRetry | browser network not available'); +// Optionally, update state or notify listeners about the network issueCommittable suggestion skipped: line range outside the PR's diff.
141-142: 🛠️ Refactor suggestion
Implement a retry mechanism with limits.
Using
setIntervalwithout a retry limit can lead to infinite retries, potentially causing resource exhaustion. Implement a retry count or backoff strategy to prevent this.Consider modifying the code as follows:
+let retryCount = 0; +const maxRetries = 5; this.reconnectInterval = setInterval(async () => { + if (retryCount >= maxRetries) { + clearInterval(this.reconnectInterval); + LoggerProxy.logger.error('Maximum retry attempts reached.'); + return; + } await this.handleSocketClose(); + retryCount += 1; }, CONNECTIVITY_CHECK_INTERVAL);Committable suggestion skipped: line range outside the PR's diff.
packages/@webex/plugin-cc/src/services/config/index.ts (8)
58-61: 🛠️ Refactor suggestion
Simplify async function by removing unnecessary
Promise.resolveandPromise.reject.In the
getDesktopProfileByIdmethod, return the response directly and throw errors without wrapping them in Promises.Apply this refactor:
if (response.statusCode !== 200) { throw new Error(`API call failed with ${response.statusCode}`); } this.webex.logger.log('getDesktopProfileById api success.'); - return Promise.resolve(response.body); + return response.body;📝 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.if (response.statusCode !== 200) { throw new Error(`API call failed with ${response.statusCode}`); } this.webex.logger.log('getDesktopProfileById api success.'); return response.body;
136-139: 🛠️ Refactor suggestion
Simplify async function by removing unnecessary
Promise.resolveandPromise.reject.In the
getListOfAuxCodesmethod, remove unnecessary use ofPromise.resolveandPromise.reject.Apply this refactor:
if (response.statusCode !== 200) { throw new Error(`API call failed with ${response.statusCode}`); } this.webex.logger.log('getListOfAuxCodes api success.'); - return Promise.resolve(response.body); + return response.body; } catch (error) { - return Promise.reject(error); + throw error; }Committable suggestion skipped: line range outside the PR's diff.
24-42: 🛠️ Refactor suggestion
Simplify async function by removing unnecessary
Promise.resolveandPromise.reject.In the
getUserUsingCImethod, since it's anasyncfunction, you can return the response directly without wrapping it inPromise.resolve, and throw errors withoutPromise.reject.Apply this refactor:
public async getUserUsingCI(): Promise<AgentResponse> { try { const response = await this.httpReq.request({ service: WCC_API_GATEWAY, resource: `organization/${this.orgId}/user/by-ci-user-id/${this.agentId}`, method: HTTP_METHODS.GET, }); if (response.statusCode !== 200) { throw new Error(`API call failed with ${response.statusCode}`); } this.webex.logger.log('getUserUsingCI api success.'); - return Promise.resolve(response.body); + return response.body; } catch (error) { - return Promise.reject(error); + throw error; } }This change simplifies the code and adheres to standard
async/awaitpractices.📝 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.public async getUserUsingCI(): Promise<AgentResponse> { try { const response = await this.httpReq.request({ service: WCC_API_GATEWAY, resource: `organization/${this.orgId}/user/by-ci-user-id/${this.agentId}`, method: HTTP_METHODS.GET, }); if (response.statusCode !== 200) { throw new Error(`API call failed with ${response.statusCode}`); } this.webex.logger.log('getUserUsingCI api success.'); return response.body; } catch (error) { throw error; } }
79-106: 🛠️ Refactor suggestion
Specify default parameter values in method signature.
The method
getListOfTeamsmentions default values in its documentation but doesn't assign them in the signature. Including default parameter values improves usability and aligns the code with the documentation.Apply this change:
public async getListOfTeams( - page: number, - pageSize: number, - filter: string[], - attributes: string[] + page: number = 0, + pageSize: number = 10, + filter: string[] = [], + attributes: string[] = ['id', 'name'] ): Promise<Team> {📝 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.public async getListOfTeams( page: number = 0, pageSize: number = 10, filter: string[] = [], attributes: string[] = ['id', 'name'] ): Promise<Team> { try { const resource = `organization/${this.orgId}/team?page=${page}&pageSize=${pageSize}${ filter && filter.length > 0 ? `&filter=id=in=${filter}` : '' }&attributes=${attributes}`; const response = await this.httpReq.request({ service: WCC_API_GATEWAY, resource, method: HTTP_METHODS.GET, }); if (response.statusCode !== 200) { throw new Error(`API call failed with ${response.statusCode}`); } this.webex.logger.log('getListOfTeams api success.'); return Promise.resolve(response.body); } catch (error) { return Promise.reject(error); } }
117-146: 🛠️ Refactor suggestion
Specify default parameter values in method signature.
The method
getListOfAuxCodesmentions default values in its documentation but doesn't assign them in the signature. Assigning default values enhances clarity and usability.Apply this change:
public async getListOfAuxCodes( - page: number, - pageSize: number, - filter: string[], - attributes: string[] + page: number = 0, + pageSize: number = 10, + filter: string[] = [], + attributes: string[] = ['id', 'name', 'active'] ): Promise<ListAuxCodesResponse> {📝 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.public async getListOfAuxCodes( page: number = 0, pageSize: number = 10, filter: string[] = [], attributes: string[] = ['id', 'name', 'active'] ): Promise<ListAuxCodesResponse> { try { const resource = `organization/${ this.orgId }/v2/auxiliary-code?page=${page}&pageSize=${pageSize}${ filter && filter.length > 0 ? `&filter=id=in=${filter}` : '' }&attributes=${attributes}`; const response = await this.httpReq.request({ service: WCC_API_GATEWAY, resource, method: HTTP_METHODS.GET, }); if (response.statusCode !== 200) { throw new Error(`API call failed with ${response.statusCode}`); } this.webex.logger.log('getListOfAuxCodes api success.'); return Promise.resolve(response.body); } catch (error) { return Promise.reject(error); } }
50-68: 🛠️ Refactor suggestion
Simplify async function by removing unnecessary
Promise.resolveandPromise.reject.In the
getDesktopProfileByIdmethod, you can return the response directly and throw errors without wrapping them in Promises.Apply this refactor:
public async getDesktopProfileById(desktopProfileId: string): Promise<DesktopProfileResponse> { try { const response = await this.httpReq.request({ service: WCC_API_GATEWAY, resource: `organization/${this.orgId}/agent-profile/${desktopProfileId}`, method: HTTP_METHODS.GET, }); if (response.statusCode !== 200) { throw new Error(`API call failed with ${response.statusCode}`); } this.webex.logger.log('getDesktopProfileById api success.'); - return Promise.resolve(response.body); + return response.body; } catch (error) { - return Promise.reject(error); + throw error; } }This ensures consistency and cleaner code.
📝 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.public async getDesktopProfileById(desktopProfileId: string): Promise<DesktopProfileResponse> { try { const response = await this.httpReq.request({ service: WCC_API_GATEWAY, resource: `organization/${this.orgId}/agent-profile/${desktopProfileId}`, method: HTTP_METHODS.GET, }); if (response.statusCode !== 200) { throw new Error(`API call failed with ${response.statusCode}`); } this.webex.logger.log('getDesktopProfileById api success.'); return response.body; } catch (error) { throw error; } }
85-106: 🛠️ Refactor suggestion
Use
URLSearchParamsto build query parameters safely.Constructing the
resourceURL by concatenating strings can lead to issues with special characters. UsingURLSearchParamsensures proper encoding of query parameters.Refactor the code as follows:
public async getListOfTeams( page: number = 0, pageSize: number = 10, filter: string[] = [], attributes: string[] = ['id', 'name'] ): Promise<Team> { try { - const resource = `organization/${this.orgId}/team?page=${page}&pageSize=${pageSize}${ - filter && filter.length > 0 ? `&filter=id=in=${filter}` : '' - }&attributes=${attributes}`; + const params = new URLSearchParams(); + params.append('page', page.toString()); + params.append('pageSize', pageSize.toString()); + if (filter && filter.length > 0) { + params.append('filter', `id=in=${filter.join(',')}`); + } + params.append('attributes', attributes.join(',')); + const resource = `organization/${this.orgId}/team?${params.toString()}`; const response = await this.httpReq.request({ service: WCC_API_GATEWAY, resource, method: HTTP_METHODS.GET, });📝 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.try { const params = new URLSearchParams(); params.append('page', page.toString()); params.append('pageSize', pageSize.toString()); if (filter && filter.length > 0) { params.append('filter', `id=in=${filter.join(',')}`); } params.append('attributes', attributes.join(',')); const resource = `organization/${this.orgId}/team?${params.toString()}`; const response = await this.httpReq.request({ service: WCC_API_GATEWAY, resource, method: HTTP_METHODS.GET, }); if (response.statusCode !== 200) { throw new Error(`API call failed with ${response.statusCode}`); } this.webex.logger.log('getListOfTeams api success.'); return Promise.resolve(response.body); } catch (error) { return Promise.reject(error); } }
123-146: 🛠️ Refactor suggestion
Use
URLSearchParamsto build query parameters safely.Similar to
getListOfTeams, usingURLSearchParamsingetListOfAuxCodesensures that query parameters are properly encoded.Refactor the code as follows:
public async getListOfAuxCodes( page: number = 0, pageSize: number = 10, filter: string[] = [], attributes: string[] = ['id', 'name', 'active'] ): Promise<ListAuxCodesResponse> { try { - const resource = `organization/${ - this.orgId - }/v2/auxiliary-code?page=${page}&pageSize=${pageSize}${ - filter && filter.length > 0 ? `&filter=id=in=${filter}` : '' - }&attributes=${attributes}`; + const params = new URLSearchParams(); + params.append('page', page.toString()); + params.append('pageSize', pageSize.toString()); + if (filter && filter.length > 0) { + params.append('filter', `id=in=${filter.join(',')}`); + } + params.append('attributes', attributes.join(',')); + const resource = `organization/${this.orgId}/v2/auxiliary-code?${params.toString()}`; const response = await this.httpReq.request({ service: WCC_API_GATEWAY, resource, method: HTTP_METHODS.GET, });📝 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.try { const params = new URLSearchParams(); params.append('page', page.toString()); params.append('pageSize', pageSize.toString()); if (filter && filter.length > 0) { params.append('filter', `id=in=${filter.join(',')}`); } params.append('attributes', attributes.join(',')); const resource = `organization/${this.orgId}/v2/auxiliary-code?${params.toString()}`; const response = await this.httpReq.request({ service: WCC_API_GATEWAY, resource, method: HTTP_METHODS.GET, }); if (response.statusCode !== 200) { throw new Error(`API call failed with ${response.statusCode}`); } this.webex.logger.log('getListOfAuxCodes api success.'); return Promise.resolve(response.body); } catch (error) { return Promise.reject(error); } }packages/@webex/plugin-cc/src/services/core/WebSocket/WebSocketManager.ts (4)
126-129:
⚠️ Potential issueAdd error handling for JSON parsing in
onmessagehandlerParsing incoming messages without error handling can lead to unhandled exceptions if the message is not valid JSON.
Wrap
JSON.parsein a try-catch block to handle invalid JSON messages gracefully.this.websocket.onmessage = (e: MessageEvent) => { this.dispatchEvent(new CustomEvent('message', {detail: e.data})); - const eventData = JSON.parse(e.data); + let eventData; + try { + eventData = JSON.parse(e.data); + } catch (err) { + LoggerProxy.logger.error(`[WebSocketStatus] | Failed to parse incoming message: ${err}`); + return; + }This ensures that a malformed message doesn't crash the application and provides useful logging for debugging.
📝 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.this.websocket.onmessage = (e: MessageEvent) => { this.dispatchEvent(new CustomEvent('message', {detail: e.data})); let eventData; try { eventData = JSON.parse(e.data); } catch (err) { LoggerProxy.logger.error(`[WebSocketStatus] | Failed to parse incoming message: ${err}`); return; }
62-75:
⚠️ Potential issuePropagate errors from the
register()method to prevent silent failuresIn the
register()method, exceptions are caught and logged but not re-thrown or returned. This can lead to silent failures wherethis.urlremainsnull, causing subsequent methods to fail unexpectedly.Consider re-throwing the error after logging it to ensure that calling methods are aware of the failure.
} catch (e) { LoggerProxy.logger.error( `Register API Failed, Request to RoutingNotifs websocket registration API failed ${e}` ); + throw e; }This change ensures that if the registration fails, the error is propagated, allowing higher-level methods to handle it appropriately.
📝 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 register(connectionConfig: SubscribeRequest) { try { const subscribeResponse: SubscribeResponse = await this.webex.request({ service: WCC_API_GATEWAY, resource: SUBSCRIBE_API, method: HTTP_METHODS.POST, body: connectionConfig, }); this.url = subscribeResponse.body.webSocketUrl; } catch (e) { LoggerProxy.logger.error( `Register API Failed, Request to RoutingNotifs websocket registration API failed ${e}` ); throw e; }
42-48:
⚠️ Potential issueHandle the potential issue when
connect()returnsundefinedIf
this.urlis not set (e.g., whenregister()fails), theconnect()method returnsundefinedinstead of a Promise. In theinitWebSocketmethod, calling.catch()onthis.connect()will throw an error becauseundefineddoes not have a.catch()method.To fix this, ensure that
connect()always returns a Promise, even whenthis.urlis not set. Alternatively, handle the case ininitWebSocketwhereconnect()might returnundefined.Here's a possible fix for the
connect()method:private async connect() { if (!this.url) { - return undefined; + return Promise.reject(new Error('WebSocket URL is not set')); }This change ensures that
connect()always returns a Promise, allowinginitWebSocketto handle the rejection properly.📝 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.return new Promise((resolve, reject) => { this.welcomePromiseResolve = resolve; this.connect().catch((error) => { LoggerProxy.logger.error(`[WebSocketStatus] | Error in connecting Websocket ${error}`); reject(error); }); });
87-145:
⚠️ Potential issueEnsure all Promises in
connect()are settledThe Promise returned by the
connect()method might not be resolved or rejected in all execution paths, potentially leading to hanging Promises.Ensure that the Promise is either resolved or rejected in all cases. For example, add a resolve in the
onopenevent and adjust other handlers accordingly.return new Promise((resolve, reject) => { this.websocket.onopen = () => { // Existing code... + resolve(); }; this.websocket.onerror = (event: any) => { // Existing code... reject(event); }; // Ensure that `onclose` and other events also settle the Promise if appropriate. });This ensures that the calling code can properly await the connection process.
Committable suggestion skipped: line range outside the PR's diff.
packages/@webex/plugin-cc/src/types.ts (2)
130-130:
⚠️ Potential issueEnsure
Headerstype is properly defined or importedThe type
Headersused inIHttpResponse.headersshould be properly imported or defined to avoid type errors.If
Headersis from a library or global namespace, make sure to import it:import { Headers } from 'node-fetch'; // or the appropriate moduleIf it's a custom type, define it accordingly.
127-127: 🛠️ Refactor suggestion
Replace
anywith a more specific type inIHttpResponse.bodyIt's recommended to avoid using
anyto maintain type safety. Consider replacinganywith a more specific type orunknownif the exact type is not yet determined.Apply this diff to update the code:
export interface IHttpResponse { - body: any; + body: unknown; statusCode: number; method: string; headers: Headers; url: string; }Committable suggestion skipped: line range outside the PR's diff.
packages/@webex/plugin-cc/src/cc.ts (4)
151-156: 🛠️ Refactor suggestion
Address the TODO: Remove unnecessary properties from API request
The TODO comment indicates that certain properties should not be part of the public API. Filling them with empty values is a temporary fix. It's better to update the API to exclude these properties or handle them appropriately.
Consider modifying the API request to exclude these properties if they're not required. If you'd like assistance in updating the API definitions or refactoring the code, I can help with that.
95-102:
⚠️ Potential issueEnsure
agentConfigis initialized before useThe
agentConfigmight beundefinedifregister()has not been called beforegetBuddyAgents(), leading to a runtime error when accessingthis.agentConfig.agentProfileId. Please ensure thatregister()is called and completed successfully before invoking this method.Consider adding a check to verify
agentConfigis initialized:public async getBuddyAgents(data: BuddyAgents): Promise<BuddyAgentsResponse> { + if (!this.agentConfig) { + throw new Error('Agent configuration not initialized. Please call register() first.'); + } try { return await this.services.agent.buddyAgents({ data: {agentProfileId: this.agentConfig.agentProfileId, ...data}, }); } catch (error) { throw getErrorDetails(error, 'getBuddyAgents'); } }📝 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.if (!this.agentConfig) { throw new Error('Agent configuration not initialized. Please call register() first.'); } try { return await this.services.agent.buddyAgents({ data: {agentProfileId: this.agentConfig.agentProfileId, ...data}, }); } catch (error) { throw getErrorDetails(error, 'getBuddyAgents'); } }
44-49: 🛠️ Refactor suggestion
Avoid using
@ts-ignore; properly definethis.webexandthis.configtypesUsing
@ts-ignoresuppresses TypeScript errors, which can mask underlying issues and reduce type safety. It's better to explicitly define the types forthis.webexandthis.configto ensure type safety and maintainability.Apply this diff to properly define the types:
+ import {WebexSDK} from '@webex/webex-core'; // If not already imported export default class ContactCenter extends WebexPlugin implements IContactCenter { namespace = 'cc'; + public webex: WebexSDK; + public config: CCPluginConfig; private $config: CCPluginConfig; private $webex: WebexSDK; constructor(...args) { super(...args); - // @ts-ignore this.$webex = this.webex; this.$webex.once(READY, () => { - // @ts-ignore this.$config = this.config;Committable suggestion skipped: line range outside the PR's diff.
140-169:
⚠️ Potential issueHandle missing
agentConfiginstationLoginSimilar to
getBuddyAgents(), thestationLogin()method usesthis.agentConfigwithout checking if it's initialized. This could lead to errors ifregister()hasn't been called. EnsureagentConfigis available before proceeding.Add a check for
agentConfiginitialization:public async stationLogin(data: AgentLogin): Promise<StationLoginResponse> { + if (!this.agentConfig) { + throw new Error('Agent configuration not initialized. Please call register() first.'); + } try { const loginResponse = this.services.agent.stationLogin({ data: { dialNumber: data.loginOption === LoginOption.BROWSER ? this.agentConfig.agentId : data.dialNumber, // ... rest of the code📝 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.public async stationLogin(data: AgentLogin): Promise<StationLoginResponse> { if (!this.agentConfig) { throw new Error('Agent configuration not initialized. Please call register() first.'); } try { const loginResponse = this.services.agent.stationLogin({ data: { dialNumber: data.loginOption === LoginOption.BROWSER ? this.agentConfig.agentId : data.dialNumber, teamId: data.teamId, deviceType: data.loginOption, isExtension: data.loginOption === LoginOption.EXTENSION, deviceId: this.getDeviceId(data.loginOption, data.dialNumber), roles: [AGENT], // TODO: The public API should not have the following properties so filling them with empty values for now. If needed, we can add them in the future. teamName: EMPTY_STRING, siteId: EMPTY_STRING, usesOtherDN: false, auxCodeId: EMPTY_STRING, }, }); if (data.loginOption === LoginOption.BROWSER) { await this.webCallingService.registerWebCallingLine(); } await loginResponse; return loginResponse; } catch (error) { throw getErrorDetails(error, 'stationLogin'); } }packages/@webex/plugin-cc/src/services/core/aqm-reqs.ts (5)
141-141:
⚠️ Potential issueAvoid using 'window.setTimeout' for better environment compatibility
Using
window.setTimeoutmay cause issues in environments wherewindowis not defined, such as Node.js. Instead, use the globalsetTimeoutfunction to ensure compatibility across different environments.Apply this diff:
- window.setTimeout( + setTimeout(📝 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.setTimeout(
125-130: 🛠️ Refactor suggestion
Remove duplicate code when obfuscating 'Authorization' header
The code that obfuscates the
Authorizationheader is duplicated. You can streamline the code by removing the redundant check and assignment.Apply this diff to eliminate the duplicate code:
if (error?.headers) { error.headers.Authorization = '*'; - } - if (error?.headers) { - error.headers.Authorization = '*'; }📝 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.if (error?.headers) { error.headers.Authorization = '*'; }
150-150:
⚠️ Potential issueSanitize logged data to prevent exposure of sensitive information
Logging the entire
responseobject may inadvertently expose sensitive data. Limit the logged information to essential details.Apply this diff to adjust the log message:
- LoggerProxy.logger.error(`Routing request timeout${keySuccess}${response!}${c.url}`); + LoggerProxy.logger.error(`Routing request timeout | Key: ${keySuccess}, URL: ${c.url}`);📝 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.LoggerProxy.logger.error(`Routing request timeout | Key: ${keySuccess}, URL: ${c.url}`);
206-210:
⚠️ Potential issueAdd error handling for JSON parsing in 'onMessage'
The
JSON.parsecall may throw an error ifmsg.detailis not valid JSON. To prevent the application from crashing, add a try-catch block to handle potential parsing errors gracefully.Apply this diff to add error handling:
let event; + try { event = JSON.parse(msg.detail); + } catch (e) { + LoggerProxy.logger.error(`Failed to parse WebSocket message: ${e.message}`); + return; + } if (event.type === 'Welcome') { LoggerProxy.logger.info(`Welcome message from Notifs Websocket`);📝 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.let event; try { event = JSON.parse(msg.detail); } catch (e) { LoggerProxy.logger.error(`Failed to parse WebSocket message: ${e.message}`); return; } if (event.type === 'Welcome') { LoggerProxy.logger.info(`Welcome message from Notifs Websocket`); return;
97-99:
⚠️ Potential issueAvoid logging potentially sensitive information in error messages
Logging the entire
msgobject might expose sensitive information. To maintain security and privacy, log only the necessary details.Apply this diff to log essential information:
LoggerProxy.logger.log(`Routing request failed: ${msg}`); - const eerr = new Err.Details(notifFail.errId, msg as any); - LoggerProxy.logger.log(`Routing request failed: ${eerr}`); + const errorMessage = notifFail.errId; + LoggerProxy.logger.log(`Routing request failed: ${errorMessage}`);📝 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.LoggerProxy.logger.log(`Routing request failed: ${msg}`); const errorMessage = notifFail.errId; LoggerProxy.logger.log(`Routing request failed: ${errorMessage}`);docs/samples/contact-center/app.js (3)
198-198:
⚠️ Potential issueFix usage of undeclared variable
auxCodeIdThe variable
auxCodeIdis used at lines 198 and 205 without being declared, which will cause aReferenceErrorat runtime. DeclareauxCodeIdbefore using it to ensure the code functions correctly.Apply this diff to declare
auxCodeIdglobally:let agentStatus; +let auxCodeId; let agentId;Also applies to: 205-205
233-233:
⚠️ Potential issueCorrect the improper HTML closing tags in
fetchBuddyAgentsThe
<option>elements at lines 233, 239, and 253 are not properly closed, which can cause rendering issues in the dropdown. Replace<option>with</option>to properly close the tags.Apply this diff to fix the closing tags:
- buddyAgentsDropdownElm.innerHTML = `<option disabled="true">Failed to fetch buddy agents<option>`; + buddyAgentsDropdownElm.innerHTML = `<option disabled="true">Failed to fetch buddy agents</option>`; - buddyAgentsDropdownElm.innerHTML = `<option disabled="true">No buddy agents available<option>`; + buddyAgentsDropdownElm.innerHTML = `<option disabled="true">No buddy agents available</option>`; - buddyAgentsDropdownElm.innerHTML = `<option disabled="true">Failed to fetch buddy agents, ${error}<option>`; + buddyAgentsDropdownElm.innerHTML = `<option disabled="true">Failed to fetch buddy agents, ${error}</option>`;Also applies to: 239-239, 253-253
253-253:
⚠️ Potential issueSanitize error messages to prevent XSS vulnerabilities
In line 253, inserting
errordirectly intoinnerHTMLcan introduce Cross-Site Scripting (XSS) vulnerabilities if theerrorcontains malicious content. UsetextContentor sanitize theerrormessage before insertion.Apply this diff to prevent potential XSS attacks:
- buddyAgentsDropdownElm.innerHTML = `<option disabled="true">Failed to fetch buddy agents, ${error}</option>`; + const option = document.createElement('option'); + option.disabled = true; + option.textContent = `Failed to fetch buddy agents, ${error}`; + buddyAgentsDropdownElm.appendChild(option);📝 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.const option = document.createElement('option'); option.disabled = true; option.textContent = `Failed to fetch buddy agents, ${error}`; buddyAgentsDropdownElm.appendChild(option);packages/@webex/plugin-cc/test/unit/spec/cc.ts (2)
62-62:
⚠️ Potential issueReview the mocking of 'webex.once' method
Mocking
webex.onceto immediately invoke the callback may cause unintended side effects in tests that rely on actual event handling. This can lead to tests passing erroneously or masking potential issues in event-driven logic.Consider using the real
webex.oncemethod or adjusting the mock to more accurately reflect asynchronous event handling.
91-98:
⚠️ Potential issueFix asynchronous test handling in 'should initialize services and logger proxy on READY event'
The test may complete before the 'READY' event handler is called, leading to false positives or intermittent failures. Use a
donecallback to ensure that Jest waits for the asynchronous code to complete.Apply this diff to fix the test:
-it('should initialize services and logger proxy on READY event', () => { +it('should initialize services and logger proxy on READY event', (done) => { webex.once('READY', () => { expect(Services.getInstance).toHaveBeenCalled(); expect(LoggerProxy.initialize).toHaveBeenCalledWith(webex.logger); + done(); }); webex.emit('READY'); });📝 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.it('should initialize services and logger proxy on READY event', (done) => { webex.once('READY', () => { expect(Services.getInstance).toHaveBeenCalled(); expect(LoggerProxy.initialize).toHaveBeenCalledWith(webex.logger); done(); }); webex.emit('READY'); });packages/@webex/internal-plugin-mercury/test/unit/spec/mercury.js (1)
716-729:
⚠️ Potential issueEnsure all stubs and spies are properly restored to avoid side effects
In the test case
'_onmessage without eventType', you stubmercury.logger.errorat line 721~ but attempt to restore it before stubbing at line 720~. Additionally, you do not restoremercury.logger.errorafter the test, which could lead to side effects in other tests. Please ensure that you restoremercury.logger.errorafter the test completes.Apply the following diff to correct the order and ensure proper restoration:
716 it('_onmessage without eventType', () => { 717 sinon.spy(mercury, '_getEventHandlers'); 718 sinon.spy(mercury, '_emit'); -720 mercury.logger.error.restore(); 721 sinon.stub(mercury.logger, 'error'); 722 const event = {data: {data: {eventType: undefined, mydata: 'some data'}}}; 723 return Promise.resolve(mercury._onmessage(event)).then(() => { 724 assert.calledWith(mercury._getEventHandlers, undefined); 725 assert.calledWith(mercury._emit, 'event', event.data); 726 assert.notCalled(mercury.logger.error); 727 mercury._emit.restore(); 728 mercury._getEventHandlers.restore(); +729 mercury.logger.error.restore(); 730 }); 731 });📝 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.it('_onmessage without eventType', () => { sinon.spy(mercury, '_getEventHandlers'); sinon.spy(mercury, '_emit'); const event = {data: {data: {eventType: undefined, mydata: 'some data'}}}; sinon.stub(mercury.logger, 'error'); return Promise.resolve(mercury._onmessage(event)).then(() => { assert.calledWith(mercury._getEventHandlers, undefined); assert.calledWith(mercury._emit, 'event', event.data); assert.notCalled(mercury.logger.error); mercury._emit.restore(); mercury._getEventHandlers.restore(); mercury.logger.error.restore(); });
.github/workflows/deploy.yml
Outdated
| @@ -250,7 +250,7 @@ jobs: | |||
| run: yarn build:local | |||
|
|
|||
| - name: Build Docs | |||
| run: yarn run build:docs | |||
| run: yarn run build:docs --exclude '@webex/plugin-cc' | |||
There was a problem hiding this comment.
We need to skip the entire building of docs for the wxcc branch, not exclude the plugin-cc. The problem is that webex, calling and meeting packages will still get published with this change.
COMPLETES #AD-HOC
This pull request addresses
Updated package.json with depoly:npm command in wxcc. Updated deploy:yml script to avoid wxcc package for documentation branch publishing
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.
Summary by CodeRabbit
New Features
wxcc) to trigger deployments, enhancing deployment flexibility.@webex/plugin-ccpackage, detailing setup and usage.Bug Fixes
Documentation
Tests
Chores