feat(plugin-cc): Spark 562169 task accept api implementation #4005
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 several changes across multiple files, primarily focusing on enhancing the deployment workflow, adding new features for a contact center application, and improving error handling in the Mercury class. Key modifications include the addition of a new branch for deployment, updates to the documentation and sample applications, and the introduction of a comprehensive error handling framework. Additionally, new configuration files and service classes are created to support the contact center functionalities, along with corresponding unit tests to ensure robustness. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Webex
participant ContactCenter
participant WebSocketManager
participant AgentConfigService
User->>Webex: Initialize Webex instance
Webex->>ContactCenter: Register Contact Center plugin
ContactCenter->>AgentConfigService: Fetch agent configurations
AgentConfigService-->>ContactCenter: Return configurations
ContactCenter->>WebSocketManager: Initialize WebSocket connection
WebSocketManager-->>ContactCenter: Connection established
User->>ContactCenter: Perform agent actions
ContactCenter->>WebSocketManager: Send agent actions
WebSocketManager-->>ContactCenter: Acknowledge actions
Possibly related PRs
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 (
|
![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 (111)
packages/@webex/plugin-cc/src/logger-proxy.ts (1)
3-9: 💡 Codebase verification
🛠️ Refactor suggestion
Refactoring needed: LoggerProxy is widely used across multiple packages
The suggested refactoring would require changes across multiple packages:
- @webex/plugin-cc: Used extensively in services and tests
- @webex/plugin-meetings: Heavy usage across meeting management, media handling, and various subsystems
The proposed changes would impact:
- 200+ direct logger access points
- Multiple test files that mock logger behavior
- Critical functionality in both CC and Meetings plugins
🔗 Analysis chain
Refactor to improve type safety and error handling
The current implementation has several areas for improvement:
- Using a class with only static members is an anti-pattern
- No validation in initialize method
- No protection against using uninitialized logger
- Missing type safety for logger methods
Consider this alternative implementation:
-export default class LoggerProxy { - public static logger: Logger; - - public static initialize(logger: Logger): void { - LoggerProxy.logger = logger; - } -} +let logger: Logger | undefined; + +export function initializeLogger(newLogger: Logger): void { + if (!newLogger) { + throw new Error('Logger instance is required'); + } + logger = newLogger; +} + +export function getLogger(): Logger { + if (!logger) { + throw new Error('Logger not initialized. Call initializeLogger first.'); + } + return logger; +}This implementation:
- Uses a module-level singleton pattern instead of static class members
- Adds validation in the initialize function
- Provides type-safe access to the logger
- Throws clear errors when used incorrectly
Consider adding a default logger implementation to avoid runtime errors:
class NoOpLogger implements Logger { // Implement with no-op methods } let logger: Logger = new NoOpLogger();Let's check if there are any direct static accesses to LoggerProxy.logger that would need to be updated:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find direct static logger access # Expected: Instances where LoggerProxy.logger is accessed directly rg "LoggerProxy\.logger\." -t tsLength of output: 57012
🧰 Tools
🪛 Biome (1.9.4)
[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/index.ts (1)
10-10: 💡 Codebase verification
Export additional types and interfaces from the plugin
Based on the search results, there are several important types and interfaces in
types.tsthat should be exported for plugin consumers:
IContactCenter: Main interface defining the plugin's functionalityCCPluginConfig: Configuration interface for the pluginLoginOption: Type for login optionsTeam,AgentLogin,BuddyAgents: Types for core agent functionality- Various response types like
StationLoginResponse,StationLogoutResponse, etc.Add these exports to
index.ts:export type { IContactCenter, CCPluginConfig, LoginOption, Team, AgentLogin, BuddyAgents, StationLoginResponse, StationLogoutResponse, StationReLoginResponse, SetStateResponse, BuddyAgentsResponse } from './types';🔗 Analysis chain
Consider exporting plugin types and interfaces.
While the default export is appropriate, consider also exporting any relevant types, interfaces, or enums that consumers of this plugin might need. This is particularly important for TypeScript users of the SDK.
Let's check for any types that might need to be exported:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Look for type definitions in related files that might need to be exported # Expected: Find type definitions in cc.ts and related files # Search for type definitions in the CC plugin directory rg -t ts "^(export )?type |^(export )?interface " "packages/@webex/plugin-cc/src/"Length of output: 10398
packages/@webex/plugin-cc/src/services/core/GlobalTypes.ts (1)
8-14: 🛠️ Refactor suggestion
Improve Failure type definition and remove redundancy.
The current implementation has the following concerns:
orgIdis redundant as it's already present in the baseMsgtypereasonCodecould benefit from a more specific type definition+/** Error codes for failure messages */ +export enum FailureReasonCode { + INVALID_REQUEST = 400, + UNAUTHORIZED = 401, + NOT_FOUND = 404, + INTERNAL_ERROR = 500, +} + +/** + * Failure message type for error scenarios + */ export type Failure = Msg<{ agentId: string; trackingId: string; - reasonCode: number; - orgId: string; // Remove redundant field + reasonCode: FailureReasonCode; reason: string; }>;📝 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./** Error codes for failure messages */ export enum FailureReasonCode { INVALID_REQUEST = 400, UNAUTHORIZED = 401, NOT_FOUND = 404, INTERNAL_ERROR = 500, } /** * Failure message type for error scenarios */ export type Failure = Msg<{ agentId: string; trackingId: string; reasonCode: FailureReasonCode; reason: string; }>;packages/@webex/plugin-cc/src/services/Task/constants.ts (2)
9-9:
⚠️ Potential issueFix inconsistent path format in RESUME constant.
The RESUME constant is missing a leading slash, unlike other endpoint constants.
-export const RESUME = 'record/resume'; +export const RESUME = '/record/resume';📝 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 RESUME = '/record/resume';
1-11: 💡 Codebase verification
Based on the search results, I can now provide a definitive response about the completeness of task operation endpoints:
Task operation endpoints should include
/acceptand/declineendpointsThe codebase shows that:
- The Task service has
accept()anddecline()methods inTask/index.ts- The contact service implements the
/acceptendpoint at${TASK_API}${interactionId}/accept- The code handles both accepting and declining tasks
However, while
/acceptis defined as an API endpoint, theACCEPTandDECLINEconstants are missing fromTask/constants.ts. These should be added for consistency with other endpoint constants:export const ACCEPT = '/accept'; export const DECLINE = '/decline';🔗 Analysis chain
Verify completeness of task operation endpoints.
Given that this PR implements the "call accept flow", consider if endpoints for accepting/declining tasks should be included in these constants.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for accept/decline related API calls in the codebase rg -g '*.{ts,js}' -i "('|\")/(accept|decline)" # Search for any task-related API endpoint definitions rg -g '*.{ts,js}' "const.*task.*=.*'/v1/tasks.*'"Length of output: 108
Script:
#!/bin/bash # Search for any task-related API calls or endpoints with broader patterns rg -g '*.{ts,js}' "'/v1/tasks" -A 2 -B 2 # Search for task-related HTTP methods or operations rg -g '*.{ts,js}' -i "(post|put|get|delete).*task" -A 2 -B 2 # Look for task service implementation files fd -e ts -e js "task(service|handler|controller)"Length of output: 131531
Script:
#!/bin/bash # Search for task-related API endpoints with a broader pattern rg -g '*.{ts,js}' -i "(accept|decline|task.*api)" -A 2 -B 2 # Look for task-related service files to understand the API structure fd -e ts -e js "task" -d 3 # Search for task-related HTTP methods or operations rg -g '*.{ts,js}' -i "(post|put|get|delete).*task.*accept" -A 2 -B 2Length of output: 132360
packages/webex/src/contact-center.js (1)
14-18: 🛠️ Refactor suggestion
Improve initialization method robustness
The current implementation has a few concerns:
- It mutates the input parameter (hence the eslint disable)
- Lacks input validation
- No error handling for invalid configurations
Consider this more robust implementation:
-Webex.init = function init(attrs = {}) { - attrs.config = merge({}, config, attrs.config); // eslint-disable-line no-param-reassign +Webex.init = function init(attrs = {}) { + if (attrs.config && typeof attrs.config !== 'object') { + throw new TypeError('Config must be an object'); + } + + const mergedAttrs = { + ...attrs, + config: merge({}, config, attrs.config) + }; - return new Webex(attrs); + return new Webex(mergedAttrs); };📝 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.Webex.init = function init(attrs = {}) { if (attrs.config && typeof attrs.config !== 'object') { throw new TypeError('Config must be an object'); } const mergedAttrs = { ...attrs, config: merge({}, config, attrs.config) }; return new Webex(mergedAttrs); };packages/@webex/plugin-cc/src/config.ts (2)
3-24: 🛠️ Refactor suggestion
Add TypeScript interfaces for type safety
The configuration object would benefit from explicit TypeScript interfaces to ensure type safety and provide better documentation of the expected structure.
Consider adding interfaces like this:
interface CCMetrics { clientName: string; clientType: string; } interface ServiceData { indicator: string; domain: string; } interface CallingClientConfig { logger: { level: typeof LOGGER.INFO; }; serviceData: ServiceData; } interface CCConfig { allowMultiLogin: boolean; clientType: string; isKeepAliveEnabled: boolean; force: boolean; metrics: CCMetrics; callingClientConfig: CallingClientConfig; } const config: {cc: CCConfig} = { // ... existing configuration }; export default config;
19-20:
⚠️ Potential issueCritical: Hardcoded production domain needs to be environment-aware
The TODO comment indicates that the domain should be dynamic, but currently it's hardcoded to a production URL. This could cause issues:
- Risk of accidentally hitting production in non-prod environments
- Lack of flexibility for different deployments
- Potential security risk if credentials are sent to wrong environment
Consider implementing environment-based configuration:
const domains = { production: 'rtw.prod-us1.rtmsprod.net', staging: 'rtw.stage-us1.rtmsprod.net', development: 'rtw.dev-us1.rtmsprod.net', }; const domain = domains[process.env.NODE_ENV || 'development'];packages/@webex/plugin-cc/src/services/core/Utils.ts (2)
6-11: 🛠️ Refactor suggestion
Enhance type safety and error handling
While the function works, there are several improvements that could make it more robust:
-const getCommonErrorDetails = (errObj: WebexRequestPayload) => { +interface ErrorDetails { + trackingId: string | undefined; + msg: unknown; +} + +const getCommonErrorDetails = (errObj: WebexRequestPayload | undefined): ErrorDetails => { + if (!errObj) { + return { + trackingId: undefined, + msg: 'No error object provided' + }; + } return { - trackingId: errObj?.headers?.trackingid || errObj?.headers?.TrackingID, + trackingId: errObj?.headers?.['trackingid']?.toLowerCase() || errObj?.headers?.['TrackingID']?.toLowerCase(), msg: errObj?.body, }; };Changes suggested:
- Added explicit return type interface
- Added null/undefined check for errObj
- Normalized trackingId case handling
📝 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 ErrorDetails { trackingId: string | undefined; msg: unknown; } const getCommonErrorDetails = (errObj: WebexRequestPayload | undefined): ErrorDetails => { if (!errObj) { return { trackingId: undefined, msg: 'No error object provided' }; } return { trackingId: errObj?.headers?.['trackingid']?.toLowerCase() || errObj?.headers?.['TrackingID']?.toLowerCase(), msg: errObj?.body, }; };
13-18:
⚠️ Potential issueFix type safety issues in error handling
The current implementation has several type safety concerns that could lead to runtime errors:
-export const getErrorDetails = (error: any, methodName: string) => { +export const getErrorDetails = (error: { details?: Failure }, methodName: string): Error => { - const failure = error.details as Failure; + const failure = error?.details; LoggerProxy.logger.error(`${methodName} failed with trackingId: ${failure?.trackingId}`); - return new Error(failure?.data?.reason ?? `Error while performing ${methodName}`); + return new Error( + failure?.data?.reason || `Error while performing ${methodName}. ${ + failure?.trackingId ? `TrackingId: ${failure.trackingId}` : '' + }` + ); };Changes needed:
- Replace
anywith a proper type definition- Remove unsafe type assertion
- Include tracking ID in the default error message for better traceability
📝 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: { details?: Failure }, methodName: string): Error => { const failure = error?.details; LoggerProxy.logger.error(`${methodName} failed with trackingId: ${failure?.trackingId}`); return new Error( failure?.data?.reason || `Error while performing ${methodName}. ${ failure?.trackingId ? `TrackingId: ${failure.trackingId}` : '' }` ); };packages/@webex/plugin-cc/src/services/core/HttpRequest.ts (3)
20-34: 🛠️ Refactor suggestion
Enhance request method with validation, error handling, and resilience
The request method should include input validation, proper error handling, and resilience mechanisms.
public async request(options: { service: string; resource: string; method: HTTP_METHODS; body?: RequestBody; }): Promise<IHttpResponse> { const {service, resource, method, body} = options; + // Input validation + if (!service?.trim()) throw new Error('Service is required'); + if (!resource?.trim()) throw new Error('Resource is required'); + if (!Object.values(HTTP_METHODS).includes(method)) { + throw new Error('Invalid HTTP method'); + } + + try { return this.webex.request({ service, resource, method, body, + timeout: 30000, // Add reasonable timeout }); + } catch (error) { + // Wrap error with context + throw new Error(`Request failed for ${service}/${resource}: ${error.message}`); + } }Consider implementing a retry mechanism for transient failures:
private async withRetry<T>( operation: () => Promise<T>, maxRetries: number = 3 ): Promise<T> { let lastError: Error; for (let attempt = 1; attempt <= maxRetries; attempt++) { try { return await operation(); } catch (error) { lastError = error; if (!this.isTransientError(error) || attempt === maxRetries) { throw error; } await new Promise(resolve => setTimeout(resolve, Math.pow(2, attempt) * 1000)); } } throw lastError; }Consider implementing request caching for frequently accessed resources to improve performance.
15-18: 🛠️ Refactor suggestion
Add webex instance validation in constructor
Consider validating the webex instance to ensure it has the required request method.
private constructor(options: {webex: WebexSDK}) { const {webex} = options; + if (typeof webex.request !== 'function') { + throw new Error('Invalid WebexSDK instance: missing request method'); + } this.webex = webex; }📝 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 constructor(options: {webex: WebexSDK}) { const {webex} = options; if (typeof webex.request !== 'function') { throw new Error('Invalid WebexSDK instance: missing request method'); } this.webex = webex; }
7-13: 🛠️ Refactor suggestion
Improve error handling in getInstance
The method should handle invalid options more explicitly and provide meaningful error messages.
public static getInstance(options?: {webex: WebexSDK}): HttpRequest { + if (!options?.webex) { + throw new Error('WebexSDK instance is required to initialize HttpRequest'); + } - if (!HttpRequest.instance && options && options.webex) { + if (!HttpRequest.instance) { HttpRequest.instance = new HttpRequest(options); } 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.public static getInstance(options?: {webex: WebexSDK}): HttpRequest { if (!options?.webex) { throw new Error('WebexSDK instance is required to initialize HttpRequest'); } if (!HttpRequest.instance) { HttpRequest.instance = new HttpRequest(options); } return HttpRequest.instance; }packages/@webex/plugin-cc/src/services/index.ts (1)
13-19: 🛠️ Refactor suggestion
Add input validation and error handling.
The constructor should validate the webSocketManager input and handle potential initialization failures.
Consider applying these improvements:
constructor(options: {webSocketManager: WebSocketManager}) { const {webSocketManager} = options; + if (!webSocketManager) { + throw new Error('WebSocketManager is required'); + } + try { const aqmReq = new AqmReqs(webSocketManager); this.config = new AgentConfigService(); this.agent = routingAgent(aqmReq); this.contact = routingContact(aqmReq); + } catch (error) { + throw new Error(`Failed to initialize Services: ${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.constructor(options: {webSocketManager: WebSocketManager}) { const {webSocketManager} = options; if (!webSocketManager) { throw new Error('WebSocketManager is required'); } try { const aqmReq = new AqmReqs(webSocketManager); this.config = new AgentConfigService(); this.agent = routingAgent(aqmReq); this.contact = routingContact(aqmReq); } catch (error) { throw new Error(`Failed to initialize Services: ${error.message}`); } }packages/@webex/plugin-cc/jest.config.js (1)
21-40: 💡 Codebase verification
Reporter configuration needs alignment across Jest configs
The review needs revision as there are inconsistencies in reporter configurations across the codebase:
- The root
jest.config.jsandpackages/calling/jest.config.jshave similar but not identical configurations- The HTML reporter is only present in the
plugin-ccconfig but not in others- Output paths vary slightly (e.g., './coverage/junit' vs 'coverage/junit')
For better maintainability and consistency:
- Consider standardizing reporter configurations across all Jest configs
- Align output directory paths (
./coverage/junitvscoverage/junit)- Evaluate if HTML reporter should be added to other configs as well
🔗 Analysis chain
Comprehensive reporter setup with proper paths
The reporter configuration is well-structured with appropriate output paths and settings. The disabled auto-open for HTML reports is suitable for CI environments.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify no conflicting reporter configurations exist in other Jest configs rg -l "jest-junit|jest-html-reporters" $(fd -t f "jest.config.js")Length of output: 3900
Script:
#!/bin/bash # Let's check the content of these files to see if there are any conflicting configurations rg -A 10 "reporters.*:" jest.config.js packages/calling/jest.config.jsLength of output: 1076
packages/@webex/plugin-cc/src/services/WebSocket/types.ts (1)
8-21: 🛠️ Refactor suggestion
Improve type safety for event handling
The current string-based event system could benefit from stronger typing to prevent runtime errors and improve developer experience.
Consider these improvements:
+/** + * Supported WebSocket event types + */ +export enum WebSocketEventType { + CONNECTED = 'connected', + DISCONNECTED = 'disconnected', + MESSAGE = 'message', + ERROR = 'error' +} interface IWebSocket { /** * Subscribe to the WebSocket events - * @param {string} event + * @param {WebSocketEventType} event - The event type to subscribe to * @param {function} callback + * @throws {Error} If the event type is not supported * @returns {void} */ - on(event: string, callback: (event: WebSocketEvent) => void): void; + on(event: WebSocketEventType, callback: (event: WebSocketEvent) => void): void; /** * Unsubscribe from the WebSocket events - * @param {string} event + * @param {WebSocketEventType} event - The event type to unsubscribe from * @param {function} callback * @returns {void} */ - off(event: string, callback: (event: WebSocketEvent) => void): void; + off(event: WebSocketEventType, callback: (event: WebSocketEvent) => void): void;Committable suggestion skipped: line range outside the PR's diff.
packages/@webex/plugin-cc/src/services/core/types.ts (1)
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. In TypeScript,voidrepresents the absence of a return value, whileundefinedis a value that can be returned.Apply this change:
-export type CbRes<TRes> = (res: any) => void | TRes; +export type CbRes<TRes> = (res: any) => undefined | TRes;📝 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: any) => undefined | TRes;🧰 Tools
🪛 Biome (1.9.4)
[error] 48-48: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
packages/@webex/plugin-cc/src/services/WebSocket/config.ts (1)
25-26:
⚠️ Potential issueFix incorrect JSDoc type annotation
The type annotation
@type {[type]}is incorrect and should be@type {number}to match the actual type of the configuration value.- * @type {[type]} + * @type {number}📝 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.* @type {number} */packages/@webex/plugin-cc/README.md (1)
22-26: 🛠️ Refactor suggestion
Update the repository URL in clone instructions
The git clone URL contains a placeholder
<your-fork>which might confuse users.-git clone https://github.com/\<your-fork\>/webex-js-sdk.git +git clone https://github.com/webex/webex-js-sdk.git📝 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.```bash git clone https://github.com/webex/webex-js-sdk.git cd webex-js-sdk/ yarn install ```packages/@webex/plugin-cc/src/services/Task/index.ts (2)
45-45:
⚠️ Potential issueFix method name typo
The method name 'declinecall' violates camelCase convention and contains a typo.
- this.webCallingService.declinecall(taskId); + this.webCallingService.declineCall(taskId);📝 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.webCallingService.declineCall(taskId);
23-35:
⚠️ Potential issueAdd cleanup for microphone stream on error
The microphone stream should be cleaned up if the call fails to prevent resource leaks.
public async accept(loginOption: LoginOption, taskId: string): Promise<void> { try { if (loginOption === LoginOption.BROWSER) { this.localAudioStream = await createMicrophoneStream({audio: true}); - this.webCallingService.answerCall(this.localAudioStream, taskId); + try { + await this.webCallingService.answerCall(this.localAudioStream, taskId); + } catch (error) { + await this.localAudioStream.stop(); + throw error; + } } else { // TODO: Invoke the accept API from services layer. This is going to be used in Outbound Dialer scenario this.services.contact.accept({interactionId: taskId}); } } catch (error) { throw getErrorDetails(error, 'accept'); } }📝 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 accept(loginOption: LoginOption, taskId: string): Promise<void> { try { if (loginOption === LoginOption.BROWSER) { this.localAudioStream = await createMicrophoneStream({audio: true}); try { await this.webCallingService.answerCall(this.localAudioStream, taskId); } catch (error) { await this.localAudioStream.stop(); throw error; } } else { // TODO: Invoke the accept API from services layer. This is going to be used in Outbound Dialer scenario this.services.contact.accept({interactionId: taskId}); } } catch (error) { throw getErrorDetails(error, 'accept'); } }packages/@webex/plugin-cc/src/services/config/constants.ts (1)
16-46: 💡 Codebase verification
⚠️ Potential issueBased on the inspection of the code and search results, I can now provide the final response:
URL parameters need proper encoding to prevent injection vulnerabilities
The endpoint mapping in
constants.tshas several query parameters that are directly interpolated into URLs without proper encoding:
filterarray is joined without encodingattributesarray is joined without encodingpageandpageSizenumeric parameters lack type validation
- Create a dedicated URL builder utility:
export class URLBuilder { private params = new URLSearchParams(); constructor(private baseUrl: string) {} addParam(key: string, value: string | number | string[]) { if (Array.isArray(value)) { this.params.append(key, value.map(v => encodeURIComponent(v)).join(',')); } else { this.params.append(key, encodeURIComponent(String(value))); } return this; } build() { const queryString = this.params.toString(); return `${this.baseUrl}${queryString ? '?' + queryString : ''}`; } } // Usage example: export const endPointMap = { listTeams: (orgId: string, page: number, pageSize: number, filter: string[], attributes: string[]) => { const builder = new URLBuilder(`organization/${encodeURIComponent(orgId)}/v2/team`); builder.addParam('page', page) .addParam('pageSize', pageSize) .addParam('attributes', attributes); if (filter?.length > 0) { builder.addParam('filter', `id=in=${filter}`); } return builder.build(); }, // ... other endpoints };This change:
- Properly encodes all URL parameters
- Provides a consistent way to build URLs
- Makes the code more maintainable and secure
- Prevents URL injection vulnerabilities
🔗 Analysis chain
Address potential security issues and inconsistencies in endpoint mapping.
Several concerns need to be addressed:
- URL parameters should be properly encoded to prevent URL injection
- API versioning is inconsistent (some endpoints use v2, others don't)
- No parameter validation before URL construction
Consider implementing these improvements:
import {encodeURIComponent} from 'querystring'; export const endPointMap = { // ... other endpoints ... listTeams: ( orgId: string, page: number, pageSize: number, filter: string[], attributes: string[] ) => { if (!orgId) throw new Error('orgId is required'); const params = new URLSearchParams({ page: String(page), pageSize: String(pageSize), attributes: attributes.join(',') }); if (filter?.length > 0) { params.append('filter', `id=in=${filter.map(f => encodeURIComponent(f)).join(',')}`); } return `organization/${encodeURIComponent(orgId)}/v2/team?${params.toString()}`; }, // Apply similar changes to other endpoints };Consider:
- Creating a dedicated URL builder utility
- Standardizing API versions across endpoints
- Adding TypeScript interfaces for endpoint parameters
- Adding input validation for all parameters
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for consistent API versioning rg -t typescript "organization/\$\{.*?\}/v\d" --no-filename # Look for potential URL encoding usage rg -t typescript "encodeURIComponent|URLSearchParams" --no-filenameLength of output: 200
Script:
#!/bin/bash # Check for consistent API versioning without file type filter rg "organization/\${.*?}/v\d" # Look for potential URL encoding usage without file type filter rg "encodeURIComponent|URLSearchParams" # Find all TypeScript/JavaScript files that might contain endpoint definitions fd "\.(ts|js)$" | grep -i "endpoint\|api\|constant"Length of output: 132201
Script:
#!/bin/bash # Search for URL encoding usage in TypeScript/JavaScript files rg "encodeURIComponent|URLSearchParams" --type-add 'ts:*.ts' --type-add 'js:*.js' -t ts -t js # Search for API version patterns in endpoint definitions rg "organization/\${.*?}/v\d" --type-add 'ts:*.ts' --type-add 'js:*.js' -t ts -t js # Find all files that might contain endpoint definitions fd -e ts -e js | grep -i "endpoint\|api\|constant"Length of output: 132308
Script:
#!/bin/bash # Search for API version patterns with fixed regex rg "organization/\$\{.*?\}/v[0-9]" --type-add 'ts:*.ts' --type-add 'js:*.js' -t ts -t js # Search for URL encoding usage in the specific file rg "encodeURIComponent|URLSearchParams" packages/@webex/plugin-cc/src/services/config/constants.ts # Find all files that might contain endpoint definitions fd -e ts -e js | grep -i "endpoint\|api\|constant"Length of output: 3104
Script:
#!/bin/bash # Check the content of the specific constants file cat packages/@webex/plugin-cc/src/services/config/constants.ts # Look for similar endpoint mapping patterns in other files rg "export const.*?Map.*?=.*?\{.*?organization/\$\{.*?\}" --type-add 'ts:*.ts' --type-add 'js:*.js' -t ts -t js -A 10 # Check for URL parameter encoding in other similar endpoint files rg "encodeURIComponent|URLSearchParams" --type-add 'ts:*.ts' --type-add 'js:*.js' -t ts -t js packages/@webex/plugin-cc/src/Length of output: 2146
packages/@webex/plugin-cc/test/unit/spec/services/core/HttpRequest.ts (2)
20-25: 🛠️ Refactor suggestion
Add test cases for singleton pattern and initialization
The test suite should verify the singleton behavior and handle invalid initialization scenarios.
Consider adding these test cases:
describe('getInstance', () => { it('should return the same instance when called multiple times', () => { const instance1 = HttpRequest.getInstance({webex: mockWebex}); const instance2 = HttpRequest.getInstance({webex: mockWebex}); expect(instance1).toBe(instance2); }); it('should throw error when initialized without webex', () => { expect(() => HttpRequest.getInstance({})).toThrow(); }); });
53-65: 🛠️ Refactor suggestion
Enhance error handling test coverage
The current error test could be more comprehensive. Consider testing:
- Different error types (network errors, timeout, etc.)
- Verify error logging behavior
- HTTP error status codes (4xx, 5xx)
it('should properly log and handle HTTP errors', async () => { const mockHttpError = { statusCode: 404, body: { error: 'Not Found' }, message: 'HTTP Error' }; mockRequest.mockRejectedValueOnce(mockHttpError); await expect( httpRequest.request({ service: 'service', resource: 'resource', method: HTTP_METHODS.GET }) ).rejects.toThrow('HTTP Error'); expect(mockWebex.logger.error).toHaveBeenCalledWith( expect.stringContaining('404'), mockHttpError ); });packages/@webex/plugin-cc/test/unit/spec/services/agent/index.ts (4)
41-46: 🛠️ Refactor suggestion
Improve test data types and payload validation
The tests use 'as any' type assertions and lack validation of the actual request payloads. Consider:
- Using proper types instead of 'as any'
- Validating the actual request payloads
- Testing with different input scenarios
Example improvement for stationLogin (apply similar pattern to other tests):
interface StationLoginData { stationId: string; extension?: string; } it('stationLogin', async () => { const reqSpy = jest.spyOn(fakeAqm, 'req'); const testData: StationLoginData = { stationId: 'STATION123', extension: '1234' }; const req = await agent.stationLogin({data: testData}); expect(req).toBeDefined(); expect(reqSpy).toHaveBeenCalledWith( expect.any(Function) ); // Validate the actual request payload const requestFn = reqSpy.mock.calls[0][0]; const requestPayload = requestFn(); expect(requestPayload).toEqual( expect.objectContaining({ stationId: testData.stationId, extension: testData.extension }) ); });Also applies to: 48-53, 54-59
19-23: 🛠️ Refactor suggestion
Enhance type safety in mock implementations
The current mock setup could be improved with proper typing to catch potential type-related issues early:
- fakeAqm = new AqmReqs() as jest.Mocked<AqmReqs>; - fakeAqm.reqEmpty = jest.fn().mockImplementation((fn) => fn); - fakeAqm.req = jest.fn().mockImplementation((fn) => fn); + fakeAqm = new AqmReqs() as jest.Mocked<AqmReqs>; + fakeAqm.reqEmpty = jest.fn().mockImplementation(<T>(fn: () => T) => fn()); + fakeAqm.req = jest.fn().mockImplementation(<T>(fn: () => T) => fn());📝 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.fakeAqm = new AqmReqs() as jest.Mocked<AqmReqs>; fakeAqm.reqEmpty = jest.fn().mockImplementation(<T>(fn: () => T) => fn()); fakeAqm.req = jest.fn().mockImplementation(<T>(fn: () => T) => fn()); agent = routingAgent(fakeAqm);
26-32: 🛠️ Refactor suggestion
Enhance error handling test coverage
While the logout test includes an error scenario, it doesn't properly assert the error handling behavior. Also, other methods should have similar error scenario coverage.
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.any(Function) + ); });📝 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.any(Function) ); });
12-60: 💡 Codebase verification
⚠️ Potential issueReview comment is valid - test coverage needed for accept functionality
The codebase shows clear implementation of accept functionality in
services/Task/index.tsandservices/Task/contact.ts, including:
- Task accept method in the Task service
- Accept API endpoint handling in contact service
- Error handling for accept operations
However, the current test file lacks coverage for these implemented features. Please add test cases for:
- Task accept success via
services.contact.accept- Error handling scenario using
getErrorDetails- Accept API payload validation with
interactionIdLocations needing test coverage:
packages/@webex/plugin-cc/src/services/Task/index.ts:accept()packages/@webex/plugin-cc/src/services/Task/contact.ts:accept🔗 Analysis chain
Add tests for call accept flow
According to the PR objectives, this change implements the call accept flow. However, there are no tests covering this functionality. Please add test cases for:
- Call accept success scenario
- Call accept failure scenarios
- Validation of accept-related payloads
- Edge cases in the accept flow
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for call accept implementation to ensure test coverage matches rg -t typescript "accept" packages/@webex/plugin-cc/src/Length of output: 91
Script:
#!/bin/bash # Search for call accept implementation with correct file extension rg "accept" packages/@webex/plugin-cc/src/ # Also search for any routing agent implementation files fd "routing.*agent" packages/@webex/plugin-cc/src/ # Look for any contact or call related files fd "contact|call" packages/@webex/plugin-cc/src/Length of output: 1401
packages/@webex/plugin-cc/src/services/core/WebSocket/keepalive.worker.js (2)
25-49:
⚠️ Potential issueCritical: Fix potential race conditions and timeout inconsistency
The
checkNetworkStatusfunction has several issues:
- Race condition:
initialisedandisSocketClosedflags are modified from multiple places without proper synchronization- Inconsistency: Comment mentions 16s timeout but code uses
closeSocketTimeoutwhich defaults to 5s- State management: Multiple state flags make the logic hard to follow
Consider refactoring to:
const checkNetworkStatus = function () { const onlineStatus = checkOnlineStatus(); postMessage({ type: "keepalive", onlineStatus }); - if (!onlineStatus && !initialised) { + if (!onlineStatus && !timeOutId) { // Use timeoutId as state indicator initialised = true; - // 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 remains open while offline timeOutId = setTimeout(() => { if (!isSocketClosed) { - initiateWebSocketClosure = true; postMessage({ type: "closeSocket" }); } + timeOutId = null; + initialised = false; }, closeSocketTimeout); } - if (onlineStatus && initialised) { + if (onlineStatus && timeOutId) { initialised = false; + clearTimeout(timeOutId); + timeOutId = null; } - - if (initiateWebSocketClosure) { - initiateWebSocketClosure = false; - clearTimeout(timeOutId); - timeOutId = null; - } };📝 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.// Checks network status and if it's offline then force closes WebSocket const checkNetworkStatus = function () { const onlineStatus = checkOnlineStatus(); postMessage({ type: "keepalive", onlineStatus }); if (!onlineStatus && !timeOutId) { // Use timeoutId as state indicator initialised = true; // Sets a timeout to force close the socket if it remains open while offline timeOutId = setTimeout(() => { if (!isSocketClosed) { postMessage({ type: "closeSocket" }); } timeOutId = null; initialised = false; }, closeSocketTimeout); } if (onlineStatus && timeOutId) { initialised = false; clearTimeout(timeOutId); timeOutId = null; } };
51-74: 🛠️ Refactor suggestion
Improve message handler robustness and configuration
The message event handler could be improved in several areas:
- Error handling for invalid messages
- Documentation for configuration values
- Type checking for event data
Consider this improvement:
+const DEFAULT_INTERVAL = 4000; // 4 seconds +const DEFAULT_TIMEOUT = 5000; // 5 seconds + addEventListener("message", (event) => { + if (!event.data) { + console.error('Invalid message received'); + return; + } + if (event.data?.type === "start") { - intervalDuration = event.data?.intervalDuration || 4000; - closeSocketTimeout = event.data?.closeSocketTimeout || 5000; + intervalDuration = event.data.intervalDuration || DEFAULT_INTERVAL; + closeSocketTimeout = event.data.closeSocketTimeout || DEFAULT_TIMEOUT; + + if (intervalId) { + console.warn('Worker already running, clearing previous interval'); + clearInterval(intervalId); + } + console.log("event=Websocket startWorker | keepalive Worker started"); intervalId = setInterval( (checkIfSocketClosed) => { checkNetworkStatus(); isSocketClosed = checkIfSocketClosed; }, intervalDuration, - event.data?.isSocketClosed + !!event.data.isSocketClosed );📝 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 DEFAULT_INTERVAL = 4000; // 4 seconds const DEFAULT_TIMEOUT = 5000; // 5 seconds addEventListener("message", (event) => { if (!event.data) { console.error('Invalid message received'); return; } if (event.data?.type === "start") { intervalDuration = event.data.intervalDuration || DEFAULT_INTERVAL; closeSocketTimeout = event.data.closeSocketTimeout || DEFAULT_TIMEOUT; if (intervalId) { console.warn('Worker already running, clearing previous interval'); clearInterval(intervalId); } console.log("event=Websocket startWorker | keepalive Worker started"); intervalId = setInterval( (checkIfSocketClosed) => { checkNetworkStatus(); isSocketClosed = checkIfSocketClosed; }, intervalDuration, !!event.data.isSocketClosed ); resetOfflineHandler(); } if (event.data?.type === "terminate" && intervalId) { console.log("event=Websocket terminateWorker | keepalive Worker stopped"); clearInterval(intervalId); intervalId = null; resetOfflineHandler(); } });packages/@webex/plugin-cc/src/services/agent/types.ts (3)
16-41: 🛠️ Refactor suggestion
Enhance type safety and consider PII implications.
A few suggestions for improvement:
channelsMapuses a genericRecord<string, string[]>. Consider defining specific channel types.- Fields like
dialNumberanddeviceIdcontain PII. Ensure these are properly handled in logging and storage.- channelsMap: Record<string, string[]>; + /** Map of channel types to their capabilities */ + channelsMap: Record<ChannelType, ChannelCapability[]>; + /** @sensitive Phone number for dialing */ dialNumber?: string; + /** @sensitive Unique identifier of the device */ deviceId?: string | null;Also, consider adding these types:
export type ChannelType = 'voice' | 'chat' | 'email' | 'social'; export type ChannelCapability = 'inbound' | 'outbound' | 'transfer' | 'conference';
61-81: 🛠️ Refactor suggestion
Reduce duplication through type composition.
The
StationLoginSuccesstype shares many fields withReloginSuccess. Consider extracting common fields into a base type.type BaseLoginSuccess = Msg<{ eventType: 'AgentDesktopMessage'; agentId: string; trackingId: string; auxCodeId: string; teamId: string; agentSessionId: string; // ... other common fields }>; export type StationLoginSuccess = BaseLoginSuccess & { supervisorSessionId?: string; type: 'AgentStationLoginSuccess'; }; export type ReloginSuccess = BaseLoginSuccess & { isExtension: boolean; type: 'AgentReloginSuccess'; };
94-108: 🛠️ Refactor suggestion
Add validation constraints and improve null handling.
The type could benefit from:
- Adding validation constraints through branded types
- Better handling of nullable fields
- Documentation for security-sensitive fields
// Add these type definitions type PhoneNumber = string & { readonly brand: unique symbol }; type TeamId = string & { readonly brand: unique symbol }; export type UserStationLogin = { /** @sensitive Phone number for dialing */ dialNumber?: PhoneNumber | null; /** @sensitive Extension number */ dn?: PhoneNumber | null; /** Team identifier */ teamId: TeamId | null; // ... rest of the fields }; // Add validation functions function isValidPhoneNumber(value: string): value is PhoneNumber { return /^\+?[\d-]{10,}$/.test(value); }packages/@webex/plugin-cc/src/services/agent/index.ts (2)
61-66: 🛠️ Refactor suggestion
Standardize error handling across endpoints
The stationLogin endpoint implements a more detailed error handler that captures status, error type, and tracking ID. Consider applying this enhanced error handling pattern to other endpoints for consistency and better debugging capabilities.
Example implementation for other endpoints:
err: (e: any) => new Err.Details('Service.aqm.agent.[endpoint]', { status: e.response?.status ?? 0, type: e.response?.data?.errorType, trackingId: e.response?.headers?.trackingid?.split('_')[1], })
27-27: 🛠️ Refactor suggestion
Improve type safety for success messages
The current implementation uses empty objects with type assertions, which could hide potential runtime issues. Consider creating proper message structures:
// Instead of: msg: {} as Agent.ReloginSuccess, // Consider: msg: { success: true, // ... other required properties based on Agent.ReloginSuccess type } satisfies Agent.ReloginSuccess,This change would:
- Make the code more type-safe
- Ensure required properties are properly initialized
- Prevent potential runtime issues
Also applies to: 47-47, 72-72, 93-93, 114-114
packages/@webex/plugin-cc/test/unit/spec/services/WebCallingService.ts (3)
90-132: 🛠️ Refactor suggestion
Add error handling test cases and improve incoming call test.
The test suite is missing coverage for error scenarios during registration. Additionally, the incoming call test could be improved:
- No verification that the call object is properly handled
- Potential race condition between registration and incoming call handling
Consider adding these test cases:
it('should handle registration errors', async () => { line = callingClient.getLines().line1 as ILine; const error = new Error('Registration failed'); jest.spyOn(line, 'register').mockRejectedValue(error); await expect(webRTCCalling.registerWebCallingLine()) .rejects.toThrow('Registration failed'); }); // Improve incoming call test it('should handle incoming calls after registration', async () => { // First ensure registration is complete const registrationPromise = webRTCCalling.registerWebCallingLine(); // Trigger registration success line.on.mock.calls .find((call) => call[0] === LINE_EVENTS.REGISTERED)[1]({ mobiusDeviceId: 'device123', status: 'registered' }); await registrationPromise; // Now test incoming call handling const callObj = { id: 'call123', on: jest.fn() } as unknown as ICall; // Trigger incoming call line.on.mock.calls .find((call) => call[0] === LINE_EVENTS.INCOMING_CALL)[1](callObj); // Verify call handling expect(callObj.on).toHaveBeenCalled(); // Add more specific assertions about call handling });
135-147: 🛠️ Refactor suggestion
Enhance deregistration test coverage.
The current deregistration test is basic and could be improved in several ways:
- Missing error handling test cases
- Using direct property access
['line']is not ideal- No verification of cleanup after deregistration
Consider adding these test cases:
describe('deregisterWebCallingLine', () => { beforeEach(() => { line = callingClient.getLines().line1 as ILine; // Use a proper method to set the line if available webRTCCalling['line'] = line; }); it('should deregister the web calling line', async () => { const deregisterSpy = jest.spyOn(line, 'deregister'); await webRTCCalling.deregisterWebCallingLine(); expect(deregisterSpy).toHaveBeenCalled(); // Verify line is cleaned up expect(webRTCCalling['line']).toBeNull(); }); it('should handle deregistration errors', async () => { const error = new Error('Deregistration failed'); jest.spyOn(line, 'deregister').mockRejectedValue(error); await expect(webRTCCalling.deregisterWebCallingLine()) .rejects.toThrow('Deregistration failed'); }); it('should handle case when line is not registered', async () => { webRTCCalling['line'] = null; await expect(webRTCCalling.deregisterWebCallingLine()) .resolves.toBeUndefined(); }); });
1-147: 💡 Codebase verification
Missing test coverage for call accept/answer functionality
The implementation shows two distinct call acceptance paths that need test coverage:
- WebCallingService's
answerCallmethod for WebRTC calls- Task service's call acceptance through
contact.acceptfor outbound scenariosNeither of these functionalities are covered in the current test suite. Consider adding:
- Tests for WebCallingService's
answerCallmethod, verifying both successful call answer and error scenarios- Tests for Task service's call acceptance flow, including the outbound dialer scenario
🔗 Analysis chain
Verify coverage for call accept flow.
While the tests cover registration and incoming calls, there seems to be missing coverage for the call accept flow mentioned in the PR objectives.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for call accept related tests and implementations echo "Searching for call accept related code..." rg -l "acceptCall|answerCall" --type ts echo "Searching for call accept related test files..." rg -l "test.*accept.*call|test.*answer.*call" --type ts echo "Checking for any TODO comments related to call acceptance..." rg -i "todo.*accept|todo.*answer" --type tsLength of output: 753
Script:
#!/bin/bash # Let's examine the implementation of call accept in WebCallingService and Task echo "WebCallingService implementation:" rg -A 10 "acceptCall|answerCall" packages/@webex/plugin-cc/src/services/WebCallingService.ts echo -e "\nTask service implementation:" rg -A 10 "acceptCall|answerCall" packages/@webex/plugin-cc/src/services/Task/index.ts echo -e "\nChecking test coverage in Task service tests:" fd "Task.*spec.*ts" packages/@webex/plugin-cc/test/Length of output: 1252
Script:
#!/bin/bash # Let's check Task service test implementation echo "Task service test content:" rg -A 10 "test.*accept.*call|test.*answer.*call|describe.*accept" packages/@webex/plugin-cc/test/unit/spec/services/Task/ # Let's also check WebCallingService test implementation for answer call echo -e "\nWebCallingService test content for answer call:" rg -A 10 "test.*answer.*call|describe.*answer" packages/@webex/plugin-cc/test/unit/spec/services/WebCallingService.tsLength of output: 513
docs/samples/contact-center/index.html (2)
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</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</title>
88-135: 🛠️ Refactor suggestion
Improve accessibility and user experience
The Agent Station section needs accessibility and UX enhancements:
- Add ARIA labels for screen readers
- Include form validation feedback
- Add loading states for async operations
- <select id="teamsDropdown" class="form-control w-auto my-3">Teams</select> + <select id="teamsDropdown" class="form-control w-auto my-3" + aria-label="Select team" + aria-describedby="teamsHelp">Teams</select> + <div id="teamsHelp" class="form-text">Select your team to proceed with login</div> - <input id="dialNumber" name="dialNumber" placeholder="Dial Number" value="" type="text"> + <input id="dialNumber" name="dialNumber" + placeholder="Dial Number" + pattern="[0-9]+" + aria-label="Enter dial number" + required> + <div class="invalid-feedback">Please enter a valid dial number</div> - <button id="loginAgent" disabled class="btn btn-primary my-3" onclick="doAgentLogin()"> + <button id="loginAgent" disabled class="btn btn-primary my-3" onclick="doAgentLogin()"> + <span class="spinner-border spinner-border-sm hidden" role="status" aria-hidden="true"></span> Login With Selected Team </button>Committable suggestion skipped: line range outside the PR's diff.
packages/@webex/plugin-cc/test/unit/spec/services/core/WebSocket/connection-service.ts (3)
72-81: 🛠️ Refactor suggestion
Add error scenario test for ping message parsing.
The ping message handling test should also verify behavior with invalid JSON messages.
it('should handle invalid ping message gracefully', () => { const invalidMessage = new CustomEvent<string>('message', { detail: 'invalid json', }); connectionService['onPing'](invalidMessage); expect(LoggerProxy.logger.error).toHaveBeenCalled(); expect(connectionService['isKeepAlive']).toBe(false); });
127-164: 🛠️ Refactor suggestion
Add offline and timeout scenario tests.
The reconnection test suite should include tests for:
- Offline browser scenario
- Reconnection timeout handling
it('should not attempt reconnection when offline', async () => { Object.defineProperty(global, 'navigator', { value: { onLine: false }, configurable: true, }); await connectionService['handleSocketClose'](); expect(mockWebSocketManager.initWebSocket).not.toHaveBeenCalled(); }); it('should handle reconnection timeout', async () => { connectionService['connectionProp'].lostConnectionRecoveryTimeout = 1000; connectionService['onSocketClose'](); jest.advanceTimersByTime(1000); expect(connectionService['isRestoreFailed']).toBe(true); });
32-61: 🛠️ Refactor suggestion
Add cleanup for navigator mock in afterEach.
The test setup is good, but we should clean up any global mocks in the afterEach block to prevent test pollution.
afterEach(() => { jest.useRealTimers(); jest.clearAllTimers(); + // Reset navigator mock if it was modified + Object.defineProperty(global, 'navigator', { + value: undefined, + configurable: 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.describe('ConnectionService', () => { let connectionService: ConnectionService; let mockWebSocketManager: jest.Mocked<WebSocketManager>; const mockSubscribeRequest: SubscribeRequest = { force: true, isKeepAliveEnabled: false, clientType: 'WebexCCSDK', allowMultiLogin: true, }; beforeEach(() => { jest.clearAllMocks(); jest.useFakeTimers(); mockWebSocketManager = new WebSocketManager({ webex: {} as any, }) as jest.Mocked<WebSocketManager>; mockWebSocketManager.initWebSocket = jest.fn().mockResolvedValue({}); // Mock the addEventListener method mockWebSocketManager.addEventListener = jest.fn(); connectionService = new ConnectionService(mockWebSocketManager, mockSubscribeRequest); }); afterEach(() => { jest.useRealTimers(); jest.clearAllTimers(); // Reset navigator mock if it was modified Object.defineProperty(global, 'navigator', { value: undefined, configurable: true, }); });docs/samples/contact-center/style.css (3)
92-96:
⚠️ Potential issueRemove commented code and consolidate duplicate container styles
There's commented out code in the join-meeting-btn-grid class and a duplicate container class definition.
.join-meeting-btn-grid { display: grid; grid-template-columns: 1fr 1fr 1fr; - /* border: 10px solid black */ } -/* Container - Docs / Streams Fixed */ -.container { - display: flex; - width: 100%; - gap: 1rem; - flex-direction: row; -}Update the original container class to include the gap property:
.container { display: flex; width: 100%; flex-direction: row; justify-content: space-between; margin-bottom: 1rem; + gap: 1rem; }Also applies to: 333-339
12-39: 🛠️ Refactor suggestion
Consolidate duplicate button styles and use CSS variables consistently
There are duplicate opacity rules for disabled buttons, and the color values could benefit from using CSS variables.
button.btn--green { color: #fff; color: var(--md-button-join-text-color, #fff); - background-color: #00ab50; + background-color: var(--color-success); - background-color: var(--md-button-join-bg-color, #00ab50); border-color: transparent; } -button.btn--green:disabled { - opacity: 0.5; -} button.btn--red { color: #fff; color: var(--md-button-cancel-text-color, #fff); - background-color: #f7644a; + background-color: var(--color-danger); - background-color: var(--md-button-cancel-bg-color, #f7644a); border-color: transparent; } .btn--red:disabled, .btn--green:disabled { opacity: 0.5; }Committable suggestion skipped: line range outside the PR's diff.
1-448: 💡 Codebase verification
Add focus styles and improve color contrast ratios
The verification confirms accessibility concerns in the codebase:
Multiple interactive elements with
cursor: pointerlack focus styles:
.collapsible(line 81).clickable(line 193).global-toggle-btnneeds focus styles beyond just box-shadowSeveral color combinations need contrast verification:
- Text color
#555on light backgrounds- Warning text
#7d4705- Error text
#de3434- Link colors
#0052bfand#1a73e8- Button text colors on various backgrounds
Recommended fixes:
/* Add to style.css */ .collapsible:focus, .clickable:focus, .global-toggle-btn:focus { outline: 2px solid #0052bf; outline-offset: 2px; } /* Update colors for better contrast */ .note { color: #444; /* Darker than #555 */ } .webex-warning { color: #5c3504; /* Darker than #7d4705 */ } .webex-error { color: #c41e1e; /* Darker than #de3434 */ }🔗 Analysis chain
Enhance accessibility compliance
Consider the following accessibility improvements:
- Add visible focus indicators for interactive elements
- Verify color contrast ratios meet WCAG guidelines, especially for:
- Text using
#555on light backgrounds- Warning text using
#7d4705- Error text using
#de3434🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for potential accessibility issues in the codebase # Search for interactive elements that might need focus styles echo "Interactive elements without focus styles:" rg -n "cursor:\s*pointer" --type css # Search for potentially problematic color values echo "\nColor values to verify for contrast:" rg -n "color:\s*#[0-9a-f]{3,6}" --type cssLength of output: 6084
packages/@webex/plugin-cc/test/unit/spec/services/core/WebSocket/WebSocketManager.ts (2)
103-105: 🛠️ Refactor suggestion
Restore console mocks after tests
Console methods are mocked but not restored. Add an afterEach block to restore them.
+ afterEach(() => { + (console.log as jest.Mock).mockRestore(); + (console.error as jest.Mock).mockRestore(); + });📝 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 as jest.Mock).mockRestore(); (console.error as jest.Mock).mockRestore(); });
98-102: 🛠️ Refactor suggestion
Replace setTimeout with Jest timer mocks
Using real timeouts in tests can lead to flaky tests. Consider using Jest's timer mocks for more reliable test execution.
- setTimeout(() => { + jest.useFakeTimers(); + process.nextTick(() => { MockWebSocket.inst.onopen(); MockWebSocket.inst.onmessage({ data: JSON.stringify({ type: "Welcome" }) }); - }, 1); + }); + jest.runAllTimers();Committable suggestion skipped: line range outside the PR's diff.
.github/workflows/deploy.yml (1)
8-8: 💡 Codebase verification
Branch naming convention is compliant, but documentation skip needs attention
The
wxccbranch follows the required naming convention as it doesn't contain any/characters. However, skipping documentation generation for this branch (line 212:if: github.ref != 'refs/heads/wxcc') is concerning as it may lead to documentation gaps. Documentation should be maintained for all deployable branches to ensure proper API documentation and changelog updates.
- Line 212: Remove the condition
if: github.ref != 'refs/heads/wxcc'to ensure documentation is generated for all deployable branches- Line 8: The
wxccbranch name is compliant with the stated requirement🔗 Analysis chain
Verify branch naming convention and deployment implications.
The addition of
wxccbranch to deployable branches needs verification:
- Does it follow the comment's requirement that "all white-listed branches cannot include any
/characters"? ✓- Will this branch be used for long-term development or is it temporary?
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there are any existing branches with similar naming pattern git branch -r | grep -i "wxcc" # Check recent commits to understand branch usage git log --oneline --grep="wxcc" --allLength of output: 30
Script:
#!/bin/bash # Let's check the workflow file content to understand the branch configuration cat .github/workflows/deploy.yml # Also check if there are any other workflow files that might have branch configurations fd -e yml -e yaml . .github/workflows/Length of output: 12752
packages/@webex/internal-plugin-mercury/test/unit/spec/socket.js (1)
203-243:
⚠️ Potential issueFix syntax and logical errors in authorization tests.
The test cases for authorizationRequired have several issues:
- Incorrect test structure: The promise chain is missing
- Mixing socket instances: Using both
sandsocketvariables- Missing assertions for the authorization behavior
Apply this diff to fix the issues:
- it('accepts authorizationRequired option as false and skip authorize', () => { - const s = new Socket(); - const authorizeSpy = sinon.spy(socket, '_authorize'); - socket.open('ws://example.com', { + it('accepts authorizationRequired option as false and skip authorize', () => { + const s = new Socket(); + const authorizeSpy = sinon.spy(s, '_authorize'); + const promise = s.open('ws://example.com', { forceCloseDelay: mockoptions.forceCloseDelay, pingInterval: mockoptions.pingInterval, pongTimeout: mockoptions.pongTimeout, logger: console, token: 'mocktoken', trackingId: 'mocktrackingid', logLevelToken: 'mocklogleveltoken', authorizationRequired: false, }); - - mockWebSocket.readyState = 1; - mockWebSocket.emit('open'); - - assert.notCalled(authorizeSpy); - assert.called(socket._ping); + + mockWebSocket.readyState = 1; + mockWebSocket.emit('open'); + + return promise.then(() => { + assert.notCalled(authorizeSpy); + assert.called(s._ping); + return s.close(); }); + }); - it('accepts authorizationRequired option as true and calles authorize', () => { - const s = new Socket(); - const authorizeSpy = sinon.spy(socket, '_authorize'); - socket.open('ws://example.com', { + it('accepts authorizationRequired option as true and calls authorize', () => { + const s = new Socket(); + const authorizeSpy = sinon.spy(s, '_authorize'); + const promise = s.open('ws://example.com', { forceCloseDelay: mockoptions.forceCloseDelay, pingInterval: mockoptions.pingInterval, pongTimeout: mockoptions.pongTimeout, logger: console, token: 'mocktoken', trackingId: 'mocktrackingid', logLevelToken: 'mocklogleveltoken', authorizationRequired: true, }); - - mockWebSocket.readyState = 1; - mockWebSocket.emit('open'); - - assert.called(authorizeSpy); - assert.called(socket._ping); + + mockWebSocket.readyState = 1; + mockWebSocket.emit('open'); + + return promise.then(() => { + assert.called(authorizeSpy); + assert.called(s._ping); + return s.close(); + }); });📝 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('accepts authorizationRequired option as false and skip authorize', () => { const s = new Socket(); const authorizeSpy = sinon.spy(s, '_authorize'); const promise = s.open('ws://example.com', { forceCloseDelay: mockoptions.forceCloseDelay, pingInterval: mockoptions.pingInterval, pongTimeout: mockoptions.pongTimeout, logger: console, token: 'mocktoken', trackingId: 'mocktrackingid', logLevelToken: 'mocklogleveltoken', authorizationRequired: false, }); mockWebSocket.readyState = 1; mockWebSocket.emit('open'); return promise.then(() => { assert.notCalled(authorizeSpy); assert.called(s._ping); return s.close(); }); }); it('accepts authorizationRequired option as true and calls authorize', () => { const s = new Socket(); const authorizeSpy = sinon.spy(s, '_authorize'); const promise = s.open('ws://example.com', { forceCloseDelay: mockoptions.forceCloseDelay, pingInterval: mockoptions.pingInterval, pongTimeout: mockoptions.pongTimeout, logger: console, token: 'mocktoken', trackingId: 'mocktrackingid', logLevelToken: 'mocklogleveltoken', authorizationRequired: true, }); mockWebSocket.readyState = 1; mockWebSocket.emit('open'); return promise.then(() => { assert.called(authorizeSpy); assert.called(s._ping); return s.close(); }); });packages/@webex/plugin-cc/src/services/WebSocket/index.ts (4)
6-6:
⚠️ Potential issueAvoid naming conflicts: Rename the class 'WebSocket'
The name
WebSocketis a global interface in browsers for WebSocket communication. Naming your classWebSocketcould cause confusion or potential conflicts. Consider renaming the class to something more specific, such asCCWebSocketorCustomWebSocket, to avoid ambiguity.
6-6: 🛠️ Refactor suggestion
Avoid using 'any' when extending 'Mercury'
Casting
Mercuryasanyto extend it bypasses TypeScript's type checking, which can lead to runtime errors and defeats the purpose of using TypeScript. Instead of casting toany, update theMercuryclass or interface definitions to properly allow extension, or use generics to achieve the desired inheritance without losing type safety.
13-13: 🛠️ Refactor suggestion
Adjust 'webSocketUrl' type to include 'undefined'
The
webSocketUrlproperty is initially undefined and is reset toundefinedindisconnectWebSocket, but it's currently typed asstring. Update its type tostring | undefinedto accurately reflect its possible values.Apply this change:
- private webSocketUrl: string; + private webSocketUrl: string | 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.private webSocketUrl: string | undefined;
31-36:
⚠️ Potential issueUpdate JSDoc comments to match the method signature
The JSDoc comments for
connectWebSocketdo not align with the method's parameters and return type. The comments referenceparams.datachannelUrlandparams.body, and specify a return type ofPromise<void>, whereas the method acceptsoptions.webSocketUrland returnsvoid. Update the JSDoc to accurately reflect the method signature.Suggested change:
/** - * Subscribe and connect to the websocket - * @param {object} params - * @param {string} params.datachannelUrl - * @param {SubscribeRequest} params.body - * @returns {Promise<void>} + * Connects to the WebSocket using the provided URL + * @param {object} options + * @param {string} options.webSocketUrl - The WebSocket URL to connect to + * @returns {void} */📝 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./** * Connects to the WebSocket using the provided URL * @param {object} options * @param {string} options.webSocketUrl - The WebSocket URL to connect to * @returns {void} */packages/@webex/plugin-cc/src/services/WebCallingService.ts (3)
88-89:
⚠️ Potential issueCorrect the log message in
declineCallmethodIn the
elseblock of thedeclineCallmethod, the log message incorrectly states "Cannot mute a non WebRtc Call" instead of "Cannot decline a non WebRtc Call".Apply this diff to fix the log message:
- this.webex.logger.log(`[WebRtc]: Cannot mute a non WebRtc Call: ${taskId}`); + this.webex.logger.log(`[WebRtc]: Cannot decline a non WebRtc Call: ${taskId}`);📝 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.webex.logger.log(`[WebRtc]: Cannot decline a non WebRtc Call: ${taskId}`); }
83-90: 🛠️ Refactor suggestion
Rename
declinecalltodeclineCallto follow naming conventionsThe method
declinecallshould be renamed todeclineCallto maintain consistency with camelCase naming conventions for method names in TypeScript.Apply this diff to rename the method:
- public declinecall(taskId: string) { + public declineCall(taskId: string) {Also, make sure to update any references to this method elsewhere in 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 declineCall(taskId: string) { if (this.call) { this.webex.logger.info(`[WebRtc]: Call end requested: ${taskId}`); this.call.end(); } else { this.webex.logger.log(`[WebRtc]: Cannot mute a non WebRtc Call: ${taskId}`); } }
24-51: 🛠️ Refactor suggestion
Add error handling for potential exceptions in
registerWebCallingLineCurrently,
registerWebCallingLinedoes not handle exceptions that may occur during registration, such as errors fromcreateClientorthis.line.register().Wrap the registration logic in a try-catch block to handle potential exceptions:
public async registerWebCallingLine(): Promise<void> { + try { this.callingClient = await createClient(this.webex as any, this.callingClientConfig); this.line = Object.values(this.callingClient.getLines())[0]; // Existing event listeners and promise handling... this.line.register(); + } catch (error) { + this.webex.logger.error(`Error registering web calling line: ${error.message}`); + throw error; + } }Committable suggestion skipped: line range outside the PR's diff.
packages/@webex/plugin-cc/src/services/core/Err.ts (3)
87-100: 🛠️ Refactor suggestion
Properly initialize the Error class with a message.
In the
Messageclass constructor, thesuper()call should include the error message to ensure that theErrorbase class is properly initialized with the message. This allows for accurate error messages and stack traces.Update the
super()call to pass the error message:- super(); + super(this.message);Ensure that
this.messageis assigned before thesuper()call, or adjust the code accordingly to pass the correct message.Alternatively, you can restructure the constructor:
constructor(id: IdsMessage, value?: string | Error) { const message = typeof value === 'string' ? value : value?.message || ''; super(message); this.id = id; if (value instanceof Error) { this.name = value.name; } }
85-85:
⚠️ Potential issueFix typo in parameter name 'errror' to 'error' in constructor overload.
The parameter name in the constructor overload on line 85 has a typo. It should be corrected from
errrortoerrorto maintain consistency and prevent potential confusion.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);
90-91: 🛠️ Refactor suggestion
Avoid manually assigning the stack trace.
Manually reassigning the stack trace using
this.stack = new Error().stack!;is unnecessary since theErrorbase class already captures the stack trace when you callsuper(message);. Removing this assignment prevents potential issues with stack trace accuracy.Remove the manual stack assignment:
- this.stack = new Error().stack!;📝 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.packages/@webex/plugin-cc/src/services/core/WebSocket/connection-service.ts (1)
140-143: 🛠️ Refactor suggestion
Limit reconnection attempts to prevent infinite retries
Currently, the reconnection attempts may continue indefinitely in case of persistent connectivity issues. Consider implementing a maximum retry limit or an exponential backoff strategy to prevent resource exhaustion.
Here's an example of limiting reconnection attempts:
+ let retryCount = 0; + const MAX_RETRIES = 5; this.reconnectInterval = setInterval(async () => { + if (retryCount >= MAX_RETRIES) { + clearInterval(this.reconnectInterval); + LoggerProxy.logger.error('Maximum reconnection attempts reached.'); + return; + } await this.handleSocketClose(); + retryCount++; }, CONNECTIVITY_CHECK_INTERVAL);Committable suggestion skipped: line range outside the PR's diff.
packages/@webex/plugin-cc/src/types.ts (1)
199-211: 🛠️ Refactor suggestion
Ensure consistent naming conventions in type aliases
There are inconsistencies in the casing of type names in the
RequestBodyunion:
Contact.HoldResumePayloadvsContact.WrapupPayLoad(difference in 'Payload' vs 'PayLoad')Contact.cancelCtquses camelCase, whereas other types use PascalCase.Recommend standardizing the naming conventions to improve code consistency.
packages/@webex/plugin-cc/src/services/core/WebSocket/WebSocketManager.ts (5)
115-120: 🛠️ Refactor suggestion
Provide error details when rejecting the Promise
In the
onerrorhandler of the WebSocket, the Promise is rejected without providing the error details, which can make debugging difficult.Modify the
onerrorhandler to pass the error event when rejecting:this.websocket.onerror = (event: any) => { LoggerProxy.logger.error( `[WebSocketStatus] | event=socketConnectionFailed | WebSocket connection failed ${event}` ); - reject(); + reject(event); };This provides more context to the caller about why the connection failed.
📝 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.onerror = (event: any) => { LoggerProxy.logger.error( `[WebSocketStatus] | event=socketConnectionFailed | WebSocket connection failed ${event}` ); reject(event); };
28-28: 🛠️ Refactor suggestion
Initialize
websockettonullinstead of an empty objectIn the constructor,
this.websocketis initialized as an empty object cast toWebSocket. This can lead to runtime errors ifthis.websocketmethods are called before assignment. It's safer to initialize it tonulland perform null checks where necessary.Apply this diff to initialize
websockettonull:- this.websocket = {} as WebSocket; + this.websocket = null;Ensure that all usages of
this.websocketinclude null checks to prevent potential errors.📝 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 = null;
62-76:
⚠️ Potential issueHandle failure in the
registermethod appropriatelyIf the API request in
registerfails,this.urlremainsnull, but the code proceeds without signaling this failure to the caller. This can lead to issues later whenconnectis called with anullURL, potentially causing the application to hang without a clear error.Modify the
registermethod to throw an error when the API call fails:... } catch (e) { LoggerProxy.logger.error( `Register API Failed, Request to RoutingNotifs websocket registration API failed ${e}` ); + throw e; }This ensures that failures are propagated and can be handled appropriately by the caller.
📝 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; } }
38-49:
⚠️ Potential issueEnsure
initWebSockethandles registration failuresIn
initWebSocket, ifregisterfails and throws an error, the rejection is not caught, which can lead to unhandled promise rejections. Additionally, ifthis.urlremainsnull, the subsequent calls may not behave as expected.Update
initWebSocketto handle errors fromregister:async initWebSocket(options: {body: SubscribeRequest}): Promise<WelcomeResponse> { const connectionConfig = options.body; - await this.register(connectionConfig); + try { + await this.register(connectionConfig); + } catch (e) { + return Promise.reject(e); + } return new Promise((resolve, reject) => { this.welcomePromiseResolve = resolve; this.connect().catch((error) => { LoggerProxy.logger.error(`[WebSocketStatus] | Error in connecting Websocket ${error}`); reject(error); }); }); }This ensures that any errors during registration are properly propagated.
📝 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.async initWebSocket(options: {body: SubscribeRequest}): Promise<WelcomeResponse> { const connectionConfig = options.body; try { await this.register(connectionConfig); } catch (e) { return Promise.reject(e); } return new Promise((resolve, reject) => { this.welcomePromiseResolve = resolve; this.connect().catch((error) => { LoggerProxy.logger.error(`[WebSocketStatus] | Error in connecting Websocket ${error}`); reject(error); }); }); }
87-146:
⚠️ Potential issueResolve or reject the Promise in
connectThe
connectmethod creates a Promise but does not resolve or reject it within the WebSocket event handlers, potentially causing the Promise to remain pending indefinitely.Modify the
connectmethod to resolve or reject the Promise appropriately:return new Promise((resolve, reject) => { this.websocket.onopen = () => { // Existing code... + resolve(); }; this.websocket.onerror = (event: any) => { LoggerProxy.logger.error( `[WebSocketStatus] | event=socketConnectionFailed | WebSocket connection failed ${event}` ); - reject(); + reject(event); }; this.websocket.onclose = async (event: any) => { // Existing code... + reject(new Error('WebSocket connection closed unexpectedly')); }; });This ensures that the caller can handle the outcome of the connection attempt.
Committable suggestion skipped: line range outside the PR's diff.
packages/@webex/plugin-cc/src/services/Task/types.ts (4)
28-69: 🛠️ Refactor suggestion
Define specific types instead of using 'any' and optional properties
In the
AgentContacttype:
- The
participantsproperty in theInteractiontype (line 136) is defined asany; // todo. Usinganycan lead to less type safety. Consider defining a specific interface or type forparticipants.- Several properties like
eventTime,participantId,fromOwner, etc., are marked as optional (?), but may be required for proper functioning. Ensure that optional properties are correctly identified.- There's inconsistency in the type of
isWebCallMute(line 62) andisConferencing(line 49). InAgentContact,isConferencingis a required boolean, whereas inInteraction,isConferencingis optional and possibly a string. Ensure consistent types across the application.
136-136: 🛠️ Refactor suggestion
Replace 'any' with a specific type for 'participants'
Using
anyfor theparticipantsproperty reduces type safety and can lead to runtime errors. Define a specific interface or type forparticipantsto ensure better type checking.Example:
type Participant = { id: string; name: string; role: string; // other relevant fields }; // Update the Interaction type participants: Participant[];
182-182:
⚠️ Potential issueInconsistent type for 'isConferencing' property
In the
callProcessingDetailsof theInteractiontype:
- The
isConferencingproperty is defined as an optional string (isConferencing?: string;). However, in theAgentContacttype,isConferencingis defined as a required boolean (isConferencing: boolean;).To ensure type consistency, consider aligning the types:
- isConferencing?: string; + isConferencing?: boolean;Or if the property can be both boolean and string, consider unifying the type definition.
📝 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.isConferencing?: boolean;
81-101: 🛠️ Refactor suggestion
⚠️ Potential issueReview property names and consistency in 'Contact' type
- Property
channelTypeexport(line 82) seems to have an incorrect name. It might be a result of a misnamed variable or typo.- There is a mix of optional (
?) and required properties. Verify if properties likesubject,terminationType, andoutdialTypeshould be optional or required.- Ensure that the naming conventions are consistent (e.g., using camelCase for property names).
Apply these corrections:
-/** Channel type pof the contact e.g. email|chat|telephony */ -channelTypeexport: string; +/** Channel type of the contact e.g. email|chat|telephony */ +channelType: string;Committable suggestion skipped: line range outside the PR's diff.
packages/@webex/plugin-cc/src/cc.ts (4)
121-122: 🛠️ Refactor suggestion
Handle potential undefined
orgIdwhen fetching agent configurationIn
connectWebsocket,orgIdis retrieved usingthis.$webex.credentials.getOrgId(). IforgIdisundefined, subsequent calls may fail. Implement a check to handle this scenario gracefully.Include the following check:
+ if (!orgId) { + throw new Error('Organization ID is undefined. Ensure the user is authenticated.'); + }📝 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 orgId = this.$webex.credentials.getOrgId(); if (!orgId) { throw new Error('Organization ID is undefined. Ensure the user is authenticated.'); } this.agentConfig = await this.services.config.getAgentConfig(orgId, agentId);
211-217:
⚠️ Potential issueCheck for uninitialized
agentConfigingetDeviceIdIn the
getDeviceIdmethod,this.agentConfig.agentIdis accessed (line 216) without confirming ifthis.agentConfigis initialized. This could lead to a runtime error ifgetDeviceIdis called beforeregister(). Ensure thatthis.agentConfigis available before using it.Add a validation step:
+ if (!this.agentConfig) { + throw new Error('agentConfig is not initialized. Please call register() before getDeviceId().'); + }📝 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 getDeviceId(loginOption: string, dialNumber: string): string { if (!this.agentConfig) { throw new Error('agentConfig is not initialized. Please call register() before getDeviceId().'); } if (loginOption === LoginOption.EXTENSION || loginOption === LoginOption.AGENT_DN) { return dialNumber; } return WEB_RTC_PREFIX + this.agentConfig.agentId; }
97-105:
⚠️ Potential issueEnsure
agentConfigis initialized before using ingetBuddyAgentsIn the
getBuddyAgentsmethod,this.agentConfig.agentProfileIDis used without checking ifthis.agentConfigis initialized. Ifregister()has not been called prior to this method,this.agentConfigcould beundefined, leading to a runtime error. Consider adding a check to ensurethis.agentConfigis initialized or enforce thatregister()must be called before using this method.Apply the following check:
+ if (!this.agentConfig) { + throw new Error('agentConfig is not initialized. Please call register() before 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.public async getBuddyAgents(data: BuddyAgents): Promise<BuddyAgentsResponse> { try { if (!this.agentConfig) { throw new Error('agentConfig is not initialized. Please call register() before getBuddyAgents().'); } return await this.services.agent.buddyAgents({ data: {agentProfileId: this.agentConfig.agentProfileID, ...data}, }); } catch (error) { throw getErrorDetails(error, 'getBuddyAgents'); } }
143-172:
⚠️ Potential issuePrevent potential runtime errors by verifying
agentConfiginstationLoginThe
stationLoginmethod utilizesthis.agentConfig.agentId(lines 148 and 152) without verifying ifthis.agentConfigis initialized. Ifregister()hasn't been called beforestationLogin(),this.agentConfigmay beundefined, causing a runtime error. It's essential to ensurethis.agentConfigis initialized before proceeding.Consider adding the following check:
+ if (!this.agentConfig) { + throw new Error('agentConfig is not initialized. Please call register() before stationLogin().'); + }📝 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> { try { if (!this.agentConfig) { throw new Error('agentConfig is not initialized. Please call register() before stationLogin().'); } 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 (2)
12-12: 🛠️ Refactor suggestion
Inconsistent variable naming: Rename 'pendingNotifCancelrequest' to 'pendingNotifCancelRequests'
The variable
pendingNotifCancelrequestshould be renamed topendingNotifCancelRequeststo follow camelCase conventions and to reflect that it stores multiple requests. This improves code readability and consistency.Apply this diff to rename the variable:
- private pendingNotifCancelrequest: Record<string, Pending> = {}; + private pendingNotifCancelRequests: Record<string, Pending> = {}; ... - delete this.pendingNotifCancelrequest[keyCancel]; + delete this.pendingNotifCancelRequests[keyCancel]; ... - this.pendingNotifCancelrequest[keyCancel] = { + this.pendingNotifCancelRequests[keyCancel] = { ... - const kReqAlt = Object.keys(this.pendingNotifCancelrequest); + const kReqAlt = Object.keys(this.pendingNotifCancelRequests); - const req = this.pendingNotifCancelrequest[thisReq]; + const req = this.pendingNotifCancelRequests[thisReq];Also applies to: 64-64, 78-78, 231-233
141-141: 🛠️ Refactor suggestion
Use 'setTimeout' instead of 'window.setTimeout' for environment compatibility
Using
window.setTimeoutassumes a browser environment. To ensure compatibility with Node.js or other environments, usesetTimeoutdirectly.Apply this diff to update the function call:
- 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(packages/@webex/plugin-cc/src/services/config/Util.ts (1)
72-82:
⚠️ Potential issueCorrect variable name from 'dailPlan' to 'dialPlan'
The variable
dailPlanappears to be misspelled throughout this block. To maintain consistency and avoid confusion, it should be corrected todialPlan.Apply this diff to fix the typo:
dialPlanData.forEach((dailPlan: DialPlanEntity) => { + dialPlanData.forEach((dialPlan: DialPlanEntity) => { if (profileDialPlans.includes(dialPlan.id)) { const filteredPlan = { - regex: dailPlan.regularExpression, - prefix: dailPlan.prefix, - strippedChars: dailPlan.strippedChars, - name: dailPlan.name, + regex: dialPlan.regularExpression, + prefix: dialPlan.prefix, + strippedChars: dialPlan.strippedChars, + name: dialPlan.name, }; dialPlanEntries.push(filteredPlan); } });Committable suggestion skipped: line range outside the PR's diff.
docs/samples/contact-center/app.js (3)
233-234:
⚠️ Potential issueFix Incorrect HTML
optionTags in Error HandlingThe
innerHTMLstrings for error messages infetchBuddyAgentshave improperly formatted<option>tags. Missing closing</option>tags can lead to rendering issues.Apply this diff to correct the HTML strings:
- 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">Failed to fetch buddy agents, ${error}<option>`; + buddyAgentsDropdownElm.innerHTML = `<option disabled="true">Failed to fetch buddy agents, ${error}</option>`;Also applies to: 253-254
177-186: 🛠️ Refactor suggestion
Remove Hardcoded Re-Login Logic Used for Testing
The
setTimeoutfunction re-logs the agent after 15 seconds for testing purposes. Hardcoded testing code should be removed or properly managed to prevent unintended behavior in production.Apply this diff to remove the testing code:
- // Re-Login Agent after 15 seconds for testing purpose - setTimeout(async () => { - try { - const response = await webex.cc.stationReLogin(); - console.log('Agent Re-Login successful', response); - } catch (error) { - console.log('Agent Re-Login failed', error); - } - }, 15000);📝 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.
198-200:
⚠️ Potential issueDeclare
auxCodeIdBefore UsageThe variable
auxCodeIdis used without being declared, which can cause aReferenceErrorin strict mode. Ensure all variables are properly declared before use.Apply this diff to declare
auxCodeId:+ let auxCodeId; ... function handleAgentStatus(event) { const select = document.getElementById('idleCodesDropdown'); auxCodeId = event.target.value; agentStatus = select.options[select.selectedIndex].text; }Alternatively, declare
auxCodeIdat the top along with other global variables.Committable suggestion skipped: line range outside the PR's diff.
packages/@webex/plugin-cc/src/services/Task/contact.ts (4)
4-4:
⚠️ Potential issueUndefined 'err' variable used in request configurations
The 'err' variable is used in multiple request configurations but is not defined or imported. Ensure that 'err' is properly defined or imported before use to prevent reference errors.
144-177:
⚠️ Potential issueInconsistent usage of 'this.aqm.req' instead of 'aqm.req'
In the methods
consult,end, andcancelTask,this.aqm.reqis used, whereas in other methods,aqm.reqis used. This inconsistency may lead to errors, asthis.aqmmay not be undefined in the current context. Ensure thataqm.reqis used consistently across all methods.Apply this diff to correct the inconsistency:
- consult: this.aqm.req((p: {interactionId: string; data: Contact.ConsultData; url: string}) => ({ + consult: aqm.req((p: {interactionId: string; data: Contact.ConsultData; url: string}) => ({ ... - end: this.aqm.req((p: {interactionId: string}) => ({ + end: aqm.req((p: {interactionId: string}) => ({ ... - cancelTask: this.aqm.req((p: {interactionId: string}) => ({ + cancelTask: aqm.req((p: {interactionId: string}) => ({Also applies to: 283-302, 329-348
221-221: 🛠️ Refactor suggestion
Use constants from 'CC_EVENTS' instead of hardcoded event type strings
In the
blindTransferandvteamTransfermethods, event types are hardcoded as strings. For consistency and to prevent typos, use the constants defined inCC_EVENTS.Apply this diff to replace hardcoded strings with constants:
// In 'blindTransfer' method - data: {type: 'AgentBlindTransferFailedEvent'}, + data: {type: CC_EVENTS.AGENT_BLIND_TRANSFER_FAILED}, // In 'vteamTransfer' method - data: {type: 'AgentVteamTransferFailed'}, + data: {type: CC_EVENTS.AGENT_VTEAM_TRANSFER_FAILED},Also applies to: 246-246
320-320:
⚠️ Potential issueTypographical error in 'CC_EVENTS.AGENt_WRAPUP_FAILED'
There is a typographical error in the event type
AGENt_WRAPUP_FAILED. The correct constant isCC_EVENTS.AGENT_WRAPUP_FAILED, with 'AGENT' fully in uppercase.Apply this diff to fix the typo:
- data: {type: CC_EVENTS.AGENt_WRAPUP_FAILED}, + data: {type: CC_EVENTS.AGENT_WRAPUP_FAILED},📝 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.data: {type: CC_EVENTS.AGENT_WRAPUP_FAILED},packages/@webex/webex-core/src/lib/services/service-catalog.js (1)
248-254:
⚠️ Potential issueImport 'URL' from the 'url' module to ensure 'URL' is defined
The code uses the
URLconstructor to parse URLs, butURLis not imported. This will lead to aReferenceErrorat runtime. To fix this, importURLfrom the 'url' module at the top of the file.Apply this diff to fix the issue:
- import Url from 'url'; + import {URL} from 'url';Also applies to: 262-267, 285-300
packages/@webex/plugin-cc/src/services/config/types.ts (4)
19-20:
⚠️ Potential issueDuplicate event values in 'CC_EVENTS'
Both
AGENT_BUDDY_AGENTSandAGENT_BUDDY_AGENTS_SUCCESSare assigned the same value'BuddyAgents'. This could cause confusion or bugs when handling these events, as they would be indistinguishable by their value. Please assign a unique string value to each event to ensure correct event handling.
139-139: 🛠️ Refactor suggestion
Restrict 'accessWrapUpCode' and 'accessIdleCode' to specific allowed values
The properties
accessWrapUpCodeandaccessIdleCodeare currently typed asstring, but according to the comments, they can only take the values'ALL'or'SPECIFIC'. To enhance type safety and prevent invalid values, consider defining them as string literal union types:- accessWrapUpCode: string; + accessWrapUpCode: 'ALL' | 'SPECIFIC'; - accessIdleCode: string; + accessIdleCode: 'ALL' | 'SPECIFIC';Also applies to: 144-144
70-125: 🛠️ Refactor suggestion
Ensure consistent naming conventions for type names
Some type names, such as
agentWrapUpCodesandagentDefaultWrapupCode, start with a lowercase letter, while others likeAgentResponseandDesktopProfileResponsestart with an uppercase letter. According to TypeScript conventions, type and interface names should use PascalCase. Please update the type names to ensure consistency and improve code readability.Also applies to: 130-269, 482-555
498-499: 🛠️ Refactor suggestion
Use consistent types for 'regexUS' and 'regexOther' properties
The properties
regexUSandregexOtherare typed asRegExp | string, allowing them to be either aRegExpobject or astring. Mixing types can lead to runtime errors if not handled carefully. It's advisable to choose a single type, eitherRegExporstring, to ensure consistent usage and simplify type checking.packages/@webex/plugin-cc/src/services/config/index.ts (6)
66-74:
⚠️ Potential issueHandle potential null or undefined
teamIdsinuserConfigDataIn the
getAgentConfigmethod, when accessinguserConfigData.teamIds, there's a possibility thatteamIdsmight benullorundefined, which could lead to unexpected behavior or errors when callinggetAllTeams.Apply this diff to safely handle the
teamIdsproperty:- const userTeamPromise = userConfigData.teamIds + const userTeamPromise = userConfigData.teamIds && userConfigData.teamIds.length > 0Updated code:
const userTeamPromise = userConfigData.teamIds && userConfigData.teamIds.length > 0 ? this.getAllTeams( orgId, DEFAULT_PAGE_SIZE, userConfigData.teamIds, DEFAULT_TEAM_ATTRIBUTES ) : Promise.resolve([]);This ensures that
getAllTeamsis only called whenteamIdsis a non-empty array.
376-377:
⚠️ Potential issueHandle potential undefined
response.body.dataIn the
getOrganizationSettingmethod, there is an assumption thatresponse.body.data[0]exists. Ifdataisundefinedor an empty array, accessing[0]could lead to errors.Add a check to ensure
response.body.dataexists and has elements before accessing the first item:+ if (!response.body.data || response.body.data.length === 0) { + throw new Error('No organization settings data found in response.'); + } return Promise.resolve(response.body.data[0]);This ensures that the method gracefully handles unexpected API responses and provides a clear 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.if (!response.body.data || response.body.data.length === 0) { throw new Error('No organization settings data found in response.'); } return Promise.resolve(response.body.data[0]); } catch (error) {
417-434:
⚠️ Potential issueEnsure proper error handling in
getURLMappingIn the
getURLMappingmethod, when returningresponse.body.data, there is no check for the existence ofdata. Ifdataisundefined, it could cause an error.Add a null check before returning the data:
+ if (!response.body || !response.body.data) { + throw new Error('No URL mapping data found in response.'); + } return Promise.resolve(response.body.data);This ensures that the method handles unexpected API responses 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.const resource = endPointMap.urlMapping(orgId); 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}`); } LoggerProxy.logger.log('getURLMapping api success.'); if (!response.body || !response.body.data) { throw new Error('No URL mapping data found in response.'); } return Promise.resolve(response.body.data); } catch (error) { LoggerProxy.logger.error(`getURLMapping API call failed with ${error}`); throw error; }
312-315:
⚠️ Potential issueConsistent pagination logic in
getAllAuxCodesSimilar to
getAllTeams, the pagination loop ingetAllAuxCodesmay have an off-by-one error. To ensure all pages are fetched, adjust the loop condition.Apply this diff:
- for (page = DEFAULT_PAGE + 1; page < totalPages; page += 1) { + for (page = DEFAULT_PAGE + 1; page <= totalPages; page += 1) {This modification ensures that the last page of auxiliary codes is included in the results.
📝 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.for (page = DEFAULT_PAGE + 1; page <= totalPages; page += 1) { promises.push(this.getListOfAuxCodes(orgId, page, pageSize, filter, attributes)); }
84-93: 🛠️ Refactor suggestion
Handle rejected promises in
Promise.allIn the
getAgentConfigmethod, usingPromise.allwithout individualtry-catchblocks can cause the entire method to fail if any single promise rejects. This may result in a loss of partial data that could be useful.Consider using
Promise.allSettledto handle each promise individually:- const [ - agentProfileConfigData, - userDialPlanData, - userTeamData, - orgInfo, - orgSettingsData, - tenantData, - urlMappingData, - auxCodesData, - ] = await Promise.all([ + const results = await Promise.allSettled([ agentProfilePromise, userDialPlanPromise, userTeamPromise, orgInfoPromise, orgSettingsPromise, tenantDataPromise, urlMappingPromise, auxCodesPromise, ]); + // Process the results, checking for fulfillment or rejection + // and handling accordinglyThis allows for more granular error handling and the ability to proceed with available data where appropriate.
Committable suggestion skipped: line range outside the PR's diff.
236-239:
⚠️ Potential issuePotential off-by-one error in pagination loop in
getAllTeamsIn the
getAllTeamsmethod, the for-loop condition may skip the last page due to the use of a zero-based index forpageand possibly a one-based index fortotalPages. IftotalPagesrepresents the total number of pages and is inclusive, the conditionpage < totalPagesmight not retrieve the last page.Consider adjusting the loop condition to ensure all pages are fetched:
- for (page = DEFAULT_PAGE + 1; page < totalPages; page += 1) { + for (page = DEFAULT_PAGE + 1; page <= totalPages; page += 1) {Ensure that
DEFAULT_PAGEandtotalPagesare consistently defined (either zero-based or one-based) and adjust the loop accordingly.Committable suggestion skipped: line range outside the PR's diff.
packages/@webex/plugin-cc/test/unit/spec/services/core/aqm-reqs.ts (3)
69-74:
⚠️ Potential issueImproper mocking of
addEventListeneranddispatchEvent.Assigning
dispatchEventwithin theaddEventListenermock is not standard practice and may lead to unexpected behavior.Apply the following changes to correctly mock event handling:
-webSocketManagerInstance.addEventListener = jest.fn((event, callback) => { - if (event === 'message') { - webSocketManagerInstance.dispatchEvent = callback; - } -}); +webSocketManagerInstance.addEventListener = jest.fn(); +webSocketManagerInstance.dispatchEvent = jest.fn();When dispatching events, call
dispatchEventdirectly with the appropriate event object.📝 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.// Mock the addEventListener method webSocketManagerInstance.addEventListener = jest.fn(); webSocketManagerInstance.dispatchEvent = jest.fn();
80-107:
⚠️ Potential issueTest may pass without verifying expected behavior.
The test will pass even if
req({})does not throw an error, as there is no assertion in thetryblock.Ensure the test fails when the expected error is not thrown:
try { await req({}); + fail('Expected error was not thrown'); } catch (e) { expect(e).toBeDefined(); }Alternatively, use:
-await req({}); +await expect(req({})).rejects.toBeDefined();📝 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('AqmReqs should be defined', 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', }, })); await expect(req({})).rejects.toBeDefined(); });
229-268:
⚠️ Potential issueTest lacks proper error assertion.
The test catches an error but doesn't verify its content, which may lead to false positives.
Enhance the test to assert the error message:
try { await req({}); } catch (e) { expect(e).toBeDefined(); + expect(e.message).toBe('Async 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.httpRequestInstance.request.mockRejectedValueOnce(new Error('Async error')); const req = aqm.req(() => ({ url: '/url', timeout: 4000, notifSuccess: { bind: { type: 'RoutingMessage', data: { type: 'AgentConsultCreated', interactionId: '6920dda3-337a-48b1-b82d-2333392f9906', }, }, msg: {}, }, notifFail: { bind: { type: 'RoutingMessage', data: {type: 'AgentConsultFailed'}, }, errId: 'Service.aqm.contact.consult', }, notifCancel: { bind: { type: 'RoutingMessage', data: { type: 'AgentCtqCancelled', interactionId: '6920dda3-337a-48b1-b82d-2333392f9906', }, }, msg: {}, }, })); try { await req({}); } catch (e) { expect(e).toBeDefined(); expect(e.message).toBe('Async error'); } });packages/@webex/plugin-cc/test/unit/spec/cc.ts (1)
57-57:
⚠️ Potential issueIncorrect mocking of 'once' method may lead to unreliable tests
The
oncemethod is mocked to immediately invoke the callback:once: jest.fn((event, callback) => callback()),This causes the callback to be executed immediately upon registration, rather than when the
'READY'event is emitted. As a result, in the test case starting at line 89, the callback withinwebex.once('READY', callback)is called beforewebex.emit('READY')is invoked, which does not accurately simulate the event-driven behavior.To fix this issue, consider updating the mock to properly register the event and callback, and invoke the callback only when
emitis called with the corresponding event. Here's a possible fix:- once: jest.fn((event, callback) => callback()), + const events = {}; + webex.on = jest.fn((event, callback) => { + events[event] = callback; + }); + webex.once = jest.fn((event, callback) => { + events[event] = (...args) => { + callback(...args); + delete events[event]; // For 'once' behavior + }; + }); + webex.emit = jest.fn((event, ...args) => { + if (events[event]) { + events[event](...args); + } + });Adjust your tests to use the updated mock and ensure that event listeners are correctly handled when
emitis called.Also applies to: 89-96
packages/@webex/plugin-cc/test/unit/spec/services/config/index.ts (11)
413-423:
⚠️ Potential issueCorrect error handling when API returns non-200 status code
In the
getURLMappingtest, ensure you're simulating errors by rejecting the promise and adjust your assertions for consistency.Apply this diff to correct the test:
-it('should throw an error if API call returns non-200 status code', async () => { - const mockError = {statusCode: 500}; - mockHttpRequest.request.mockResolvedValue(mockError); - await expect(agentConfigService.getURLMapping(mockOrgId)).rejects.toThrow( - 'API call failed with 500' - ); - expect(LoggerProxy.logger.error).toHaveBeenCalledWith( - expect.stringContaining('getURLMapping API call failed with') - ); -}); +it('should throw an error if API call returns non-200 status code', async () => { + const mockError = new Error('API call failed with 500'); + mockHttpRequest.request.mockRejectedValue(mockError); + await expect(agentConfigService.getURLMapping(mockOrgId)).rejects.toThrow( + 'API call failed with 500' + ); + expect(LoggerProxy.logger.error).toHaveBeenCalledWith( + expect.stringContaining('getURLMapping API call failed with') + ); +});📝 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 API call returns non-200 status code', async () => { const mockError = new Error('API call failed with 500'); mockHttpRequest.request.mockRejectedValue(mockError); await expect(agentConfigService.getURLMapping(mockOrgId)).rejects.toThrow( 'API call failed with 500' ); expect(LoggerProxy.logger.error).toHaveBeenCalledWith( expect.stringContaining('getURLMapping API call failed with') ); });
71-82:
⚠️ Potential issueCorrect error handling when API returns non-200 status code
In the
getUserUsingCItest, when the API call returns a non-200 status code, the test currently mocks the request to resolve with a response object containingstatusCode: 400. However, in such cases, the service method should reject the promise with an error. To accurately test this behavior, you should mock the request to reject with an error and useawait expect(...).rejects.toThrow(...)in your test.Apply this diff to correct the test:
-it('should throw an error if the getUserUsingCI call fails with other than 200', async () => { - const mockResponse = { - statusCode: 400, - }; - (mockHttpRequest.request as jest.Mock).mockResolvedValue(mockResponse); - try { - await agentConfigService.getUserUsingCI(mockOrgId, mockAgentId); - } catch (error) { - expect(error).toEqual(new Error(`API call failed with ${mockResponse.statusCode}`)); - } -}); +it('should throw an error if the getUserUsingCI call fails with other than 200', async () => { + const mockError = new Error('API call failed with 400'); + (mockHttpRequest.request as jest.Mock).mockRejectedValue(mockError); + await expect(agentConfigService.getUserUsingCI(mockOrgId, mockAgentId)).rejects.toThrow( + 'API call failed with 400' + ); +});📝 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 getUserUsingCI call fails with other than 200', async () => { const mockError = new Error('API call failed with 400'); (mockHttpRequest.request as jest.Mock).mockRejectedValue(mockError); await expect(agentConfigService.getUserUsingCI(mockOrgId, mockAgentId)).rejects.toThrow( 'API call failed with 400' ); });
380-390:
⚠️ Potential issueCorrect error handling when API returns non-200 status code
Modify the
getTenantDatatest to reject the promise when simulating a non-200 status code response.Apply this diff to correct the test:
-it('should throw an error if API call returns non-200 status code', async () => { - const mockError = {statusCode: 500}; - mockHttpRequest.request.mockResolvedValue(mockError); - await expect(agentConfigService.getTenantData(mockOrgId)).rejects.toThrow( - 'API call failed with 500' - ); - expect(LoggerProxy.logger.error).toHaveBeenCalledWith( - expect.stringContaining('getTenantData API call failed with') - ); -}); +it('should throw an error if API call returns non-200 status code', async () => { + const mockError = new Error('API call failed with 500'); + mockHttpRequest.request.mockRejectedValue(mockError); + await expect(agentConfigService.getTenantData(mockOrgId)).rejects.toThrow( + 'API call failed with 500' + ); + expect(LoggerProxy.logger.error).toHaveBeenCalledWith( + expect.stringContaining('getTenantData API call failed with') + ); +});📝 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 API call returns non-200 status code', async () => { const mockError = new Error('API call failed with 500'); mockHttpRequest.request.mockRejectedValue(mockError); await expect(agentConfigService.getTenantData(mockOrgId)).rejects.toThrow( 'API call failed with 500' ); expect(LoggerProxy.logger.error).toHaveBeenCalledWith( expect.stringContaining('getTenantData API call failed with') ); });
290-300:
⚠️ Potential issueCorrect error handling when API returns non-200 status code
In the
getOrgInfotest, ensure that the promise is rejected with an error when simulating a non-200 status code. Update the test to reflect this behavior.Apply this diff to correct the test:
-it('should throw an error if API call returns non-200 status code', async () => { - const mockError = {statusCode: 500}; - mockHttpRequest.request.mockResolvedValue(mockError); - await expect(agentConfigService.getOrgInfo(mockOrgId)).rejects.toThrow( - 'API call failed with 500' - ); - expect(LoggerProxy.logger.error).toHaveBeenCalledWith( - expect.stringContaining('getOrgInfo API call failed with') - ); -}); +it('should throw an error if API call returns non-200 status code', async () => { + const mockError = new Error('API call failed with 500'); + mockHttpRequest.request.mockRejectedValue(mockError); + await expect(agentConfigService.getOrgInfo(mockOrgId)).rejects.toThrow('API call failed with 500'); + expect(LoggerProxy.logger.error).toHaveBeenCalledWith( + expect.stringContaining('getOrgInfo API call failed with') + ); +});📝 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 API call returns non-200 status code', async () => { const mockError = new Error('API call failed with 500'); mockHttpRequest.request.mockRejectedValue(mockError); await expect(agentConfigService.getOrgInfo(mockOrgId)).rejects.toThrow('API call failed with 500'); expect(LoggerProxy.logger.error).toHaveBeenCalledWith( expect.stringContaining('getOrgInfo API call failed with') ); });
567-581:
⚠️ Potential issueCorrect error handling when API returns non-200 status code
Similarly, for the
getAllAuxCodestest, ensure error handling is consistent by mocking a rejected promise and usingawait expect(...).rejects.toThrow(...).Apply this diff to correct the test:
-it('should throw an error if API call returns non-200 status code', async () => { - const pageSize = 10; - const filter = ['filter1']; - const attributes = ['attribute1']; - const mockError = {statusCode: 500}; - (mockHttpRequest.request as jest.Mock).mockResolvedValue(mockError); - await expect( - agentConfigService.getAllAuxCodes(mockOrgId, pageSize, filter, attributes) - ).rejects.toThrow('API call failed with 500'); - expect(LoggerProxy.logger.error).toHaveBeenCalledWith( - expect.stringContaining('getListOfAuxCodes API call failed with') - ); -}); +it('should throw an error if API call returns non-200 status code', async () => { + const pageSize = 10; + const filter = ['filter1']; + const attributes = ['attribute1']; + const mockError = new Error('API call failed with 500'); + (mockHttpRequest.request as jest.Mock).mockRejectedValue(mockError); + await expect( + agentConfigService.getAllAuxCodes(mockOrgId, pageSize, filter, attributes) + ).rejects.toThrow('API call failed with 500'); + expect(LoggerProxy.logger.error).toHaveBeenCalledWith( + expect.stringContaining('getListOfAuxCodes API call failed with') + ); +});📝 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 API call returns non-200 status code', async () => { const pageSize = 10; const filter = ['filter1']; const attributes = ['attribute1']; const mockError = new Error('API call failed with 500'); (mockHttpRequest.request as jest.Mock).mockRejectedValue(mockError); await expect( agentConfigService.getAllAuxCodes(mockOrgId, pageSize, filter, attributes) ).rejects.toThrow('API call failed with 500'); expect(LoggerProxy.logger.error).toHaveBeenCalledWith( expect.stringContaining('getListOfAuxCodes API call failed with') ); });
333-343:
⚠️ Potential issueCorrect error handling when API returns non-200 status code
Adjust the
getOrganizationSettingtest to mock a rejected promise for non-200 status codes and use consistent error assertion methods.Apply this diff to correct the test:
-it('should throw an error if API call returns non-200 status code', async () => { - const mockError = {statusCode: 500}; - mockHttpRequest.request.mockResolvedValue(mockError); - await expect(agentConfigService.getOrganizationSetting(mockOrgId)).rejects.toThrow( - 'API call failed with 500' - ); - expect(LoggerProxy.logger.error).toHaveBeenCalledWith( - expect.stringContaining('getOrganizationSetting API call failed with') - ); -}); +it('should throw an error if API call returns non-200 status code', async () => { + const mockError = new Error('API call failed with 500'); + mockHttpRequest.request.mockRejectedValue(mockError); + await expect(agentConfigService.getOrganizationSetting(mockOrgId)).rejects.toThrow( + 'API call failed with 500' + ); + expect(LoggerProxy.logger.error).toHaveBeenCalledWith( + expect.stringContaining('getOrganizationSetting API call failed with') + ); +});📝 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 API call returns non-200 status code', async () => { const mockError = new Error('API call failed with 500'); mockHttpRequest.request.mockRejectedValue(mockError); await expect(agentConfigService.getOrganizationSetting(mockOrgId)).rejects.toThrow( 'API call failed with 500' ); expect(LoggerProxy.logger.error).toHaveBeenCalledWith( expect.stringContaining('getOrganizationSetting API call failed with') ); });
181-193:
⚠️ Potential issueCorrect error handling when API returns non-200 status code
In the
getListOfTeamstest, the error scenario should mock a rejected promise instead of resolving with a response containingstatusCode: 400. This ensures the test accurately reflects the service's error handling.Apply this diff to correct the test:
-it('should throw an error if the getListOfTeams call fails with other than 200', async () => { - const mockResponse = { - statusCode: 400, - }; - (mockHttpRequest.request as jest.Mock).mockResolvedValue(mockResponse); - try { - await agentConfigService.getListOfTeams(mockOrgId, page, pageSize, filter, attributes); - } catch (error) { - expect(error).toEqual(new Error(`API call failed with ${mockResponse.statusCode}`)); - } -}); +it('should throw an error if the getListOfTeams call fails with other than 200', async () => { + const mockError = new Error('API call failed with 400'); + (mockHttpRequest.request as jest.Mock).mockRejectedValue(mockError); + await expect( + agentConfigService.getListOfTeams(mockOrgId, page, pageSize, filter, attributes) + ).rejects.toThrow('API call failed with 400'); +});📝 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 getListOfTeams call fails with other than 200', async () => { const mockError = new Error('API call failed with 400'); (mockHttpRequest.request as jest.Mock).mockRejectedValue(mockError); await expect( agentConfigService.getListOfTeams(mockOrgId, page, pageSize, filter, attributes) ).rejects.toThrow('API call failed with 400'); }); });
446-456:
⚠️ Potential issueCorrect error handling when API returns non-200 status code
Update the
getDialPlanDatatest to correctly simulate a rejected promise for non-200 status codes.Apply this diff to correct the test:
-it('should throw an error if API call returns non-200 status code', async () => { - const mockError = {statusCode: 500}; - mockHttpRequest.request.mockResolvedValue(mockError); - await expect(agentConfigService.getDialPlanData(mockOrgId)).rejects.toThrow( - 'API call failed with 500' - ); - expect(LoggerProxy.logger.error).toHaveBeenCalledWith( - expect.stringContaining('getDialPlanData API call failed with') - ); -}); +it('should throw an error if API call returns non-200 status code', async () => { + const mockError = new Error('API call failed with 500'); + mockHttpRequest.request.mockRejectedValue(mockError); + await expect(agentConfigService.getDialPlanData(mockOrgId)).rejects.toThrow( + 'API call failed with 500' + ); + expect(LoggerProxy.logger.error).toHaveBeenCalledWith( + expect.stringContaining('getDialPlanData API call failed with') + ); +});📝 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 API call returns non-200 status code', async () => { const mockError = new Error('API call failed with 500'); mockHttpRequest.request.mockRejectedValue(mockError); await expect(agentConfigService.getDialPlanData(mockOrgId)).rejects.toThrow( 'API call failed with 500' ); expect(LoggerProxy.logger.error).toHaveBeenCalledWith( expect.stringContaining('getDialPlanData API call failed with') ); });
507-521:
⚠️ Potential issueCorrect error handling when API returns non-200 status code
In the
getAllTeamstest, adjust the error simulation to reject the promise and unify the error assertion pattern.Apply this diff to correct the test:
-it('should throw an error if API call returns non-200 status code', async () => { - const pageSize = 10; - const filter = ['filter1']; - const attributes = ['attribute1']; - const mockError = {statusCode: 500}; - (mockHttpRequest.request as jest.Mock).mockResolvedValue(mockError); - await expect( - agentConfigService.getAllTeams(mockOrgId, pageSize, filter, attributes) - ).rejects.toThrow('API call failed with 500'); - expect(LoggerProxy.logger.error).toHaveBeenCalledWith( - expect.stringContaining('getListOfTeams API call failed with') - ); -}); +it('should throw an error if API call returns non-200 status code', async () => { + const pageSize = 10; + const filter = ['filter1']; + const attributes = ['attribute1']; + const mockError = new Error('API call failed with 500'); + (mockHttpRequest.request as jest.Mock).mockRejectedValue(mockError); + await expect( + agentConfigService.getAllTeams(mockOrgId, pageSize, filter, attributes) + ).rejects.toThrow('API call failed with 500'); + expect(LoggerProxy.logger.error).toHaveBeenCalledWith( + expect.stringContaining('getListOfTeams API call failed with') + ); +});📝 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 API call returns non-200 status code', async () => { const pageSize = 10; const filter = ['filter1']; const attributes = ['attribute1']; const mockError = new Error('API call failed with 500'); (mockHttpRequest.request as jest.Mock).mockRejectedValue(mockError); await expect( agentConfigService.getAllTeams(mockOrgId, pageSize, filter, attributes) ).rejects.toThrow('API call failed with 500'); expect(LoggerProxy.logger.error).toHaveBeenCalledWith( expect.stringContaining('getListOfTeams API call failed with') ); });
123-135:
⚠️ Potential issueCorrect error handling when API returns non-200 status code
Similar to the previous comment, in the
getDesktopProfileByIdtest, the API call failure with a non-200 status code should be simulated by rejecting the promise with an error. Modify the test to mock a rejected promise and useawait expect(...).rejects.toThrow(...).Apply this diff to correct the test:
-it('should throw an error if the getDesktopProfileById call fails with other than 200', async () => { - const mockResponse = { - statusCode: 400, - }; - (mockHttpRequest.request as jest.Mock).mockResolvedValue(mockResponse); - try { - await agentConfigService.getDesktopProfileById(mockOrgId, desktopProfileId); - } catch (error) { - expect(error).toEqual(new Error(`API call failed with ${mockResponse.statusCode}`)); - } -}); +it('should throw an error if the getDesktopProfileById call fails with other than 200', async () => { + const mockError = new Error('API call failed with 400'); + (mockHttpRequest.request as jest.Mock).mockRejectedValue(mockError); + await expect( + agentConfigService.getDesktopProfileById(mockOrgId, desktopProfileId) + ).rejects.toThrow('API call failed with 400'); +});📝 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 getDesktopProfileById call fails with other than 200', async () => { const mockError = new Error('API call failed with 400'); (mockHttpRequest.request as jest.Mock).mockRejectedValue(mockError); await expect( agentConfigService.getDesktopProfileById(mockOrgId, desktopProfileId) ).rejects.toThrow('API call failed with 400'); }); });
256-268:
⚠️ Potential issueCorrect error handling when API returns non-200 status code
For the
getListOfAuxCodestest, modify the error simulation to reject the promise with an error and adjust the assertions accordingly.Apply this diff to correct the test:
-it('should throw an error if the getListOfAuxCodes call fails with other than 200', async () => { - const mockResponse = { - statusCode: 400, - }; - (mockHttpRequest.request as jest.Mock).mockResolvedValue(mockResponse); - try { - await agentConfigService.getListOfAuxCodes(mockOrgId, page, pageSize, filter, attributes); - } catch (error) { - expect(error).toEqual(new Error(`API call failed with ${mockResponse.statusCode}`)); - } -}); +it('should throw an error if the getListOfAuxCodes call fails with other than 200', async () => { + const mockError = new Error('API call failed with 400'); + (mockHttpRequest.request as jest.Mock).mockRejectedValue(mockError); + await expect( + agentConfigService.getListOfAuxCodes(mockOrgId, page, pageSize, filter, attributes) + ).rejects.toThrow('API call failed with 400'); +});📝 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 getListOfAuxCodes call fails with other than 200', async () => { const mockError = new Error('API call failed with 400'); (mockHttpRequest.request as jest.Mock).mockRejectedValue(mockError); await expect( agentConfigService.getListOfAuxCodes(mockOrgId, page, pageSize, filter, attributes) ).rejects.toThrow('API call failed with 400'); }); });
There was a problem hiding this comment.
Please update the PR description with details of the new code added such as a new class called Task. Additionally, it would be good if you could talk about what the contact.ts is and what changes if any were made to the agentx-services code while moving the file here.
| @@ -1,5 +1,6 @@ | |||
| import {CallingClientConfig} from '@webex/calling'; | |||
| import * as Agent from './services/agent/types'; | |||
| import * as Contact from './services/Task/types'; | |||
There was a problem hiding this comment.
Could we stick to the convention of naming the folder in all small letters?
| import * as Contact from './services/Task/types'; | |
| import * as Contact from './services/task/types'; |
| @@ -217,7 +218,14 @@ export type RequestBody = | |||
| | Agent.Logout | |||
| | Agent.UserStationLogin | |||
| | Agent.StateChange | |||
| | Agent.BuddyAgents; | |||
| | Agent.BuddyAgents | |||
| | Contact.HoldResumePayload | |||
There was a problem hiding this comment.
Is there a reason why we are importing as Contact? What's the significance of that term?
There was a problem hiding this comment.
In agent Desktop, these call controls are referred as contact controls and we planned to call them task so replaced agent with contact
There was a problem hiding this comment.
@Kesari3008 would the developer see these names?
|
|
||
| export default class Task extends EventEmitter { | ||
| private localAudioStream: LocalMicrophoneStream; | ||
| private services: Services; // This will be used later for invoking Contact APIs |
There was a problem hiding this comment.
Does the Task need the entire Services object? Doesn't services have unnecessary members which don't pertain to the Task?
There was a problem hiding this comment.
Perhaps we could only pass the members of services that are needed?
There was a problem hiding this comment.
We do not need the entire services object but we need to keep the contact layer behind the services layer and aqmReqs needs to be passed to the contact layer which is present within the services layer
There was a problem hiding this comment.
So we only need to pass the services objects which are needed here in task right?
There was a problem hiding this comment.
Passing contact and websocketmanager instead of complete services object
| this.registerCallingEvent(); | ||
| } | ||
|
|
||
| private registerCallingEvent() { |
There was a problem hiding this comment.
| private registerCallingEvent() { | |
| private registerIncomingCallEvent() { |
There was a problem hiding this comment.
The way this class has been structured makes me feel like this is a bit of a task manager but also a task.
Also shouldn't we have types of Tasks? We won't only be doing calls in the future right?
There was a problem hiding this comment.
Also, how do we plan to handle multiple tasks at the same time?
| @@ -57,6 +59,7 @@ export default class ContactCenter extends WebexPlugin implements IContactCenter | |||
| }); | |||
|
|
|||
| this.webCallingService = new WebCallingService(this.$webex, this.$config.callingClientConfig); | |||
| this.task = new Task(this.services, this.webCallingService); | |||
There was a problem hiding this comment.
Why are we initializing a task here.. at the start, there won't be any tasks right?
| @@ -132,7 +159,7 @@ function register() { | |||
| agentLogin.innerHTML = '<option value="" selected>Choose Agent Login ...</option>'; // Clear previously selected option on agentLogin. | |||
| dialNumber.value = ''; | |||
| dialNumber.disabled = true; | |||
| if(loginVoiceOptions.length > 0) agentLoginButton.disabled = false; | |||
| if(loginVoiceOptions.length > 0) loginAgentElm.disabled = false; | |||
There was a problem hiding this comment.
| if(loginVoiceOptions.length > 0) loginAgentElm.disabled = false; | |
| if (loginVoiceOptions.length > 0) loginAgentElm.disabled = false; |
| }, | ||
| }); | ||
|
|
||
| function registerListeners(task) { |
There was a problem hiding this comment.
This part is confusing because of the way the Task class has been implemented. I'm listening on webex.cc.task, but the payload is also a task? What would the difference between tasks of different types be.. how would the payload vary..
| taskId = event.detail.task.interactionId; | ||
| const callerDisplay = event.detail.task.interaction.callAssociatedDetails.ani; | ||
|
|
||
| incomingDetailsElm.innerText = `Call from ${callerDisplay}`; |
There was a problem hiding this comment.
In the case of non-desktop mode, prehaps it might be better to say "Call from ..., please answer on the endpoint where the agent's extension is registered".
|
@Kesari3008, are we using the latest wxcc branch - only reason I ask is because I think I see some old code (which has now been changed). |
| this.taskPayload = payload.data; | ||
| if (this.webCallingService.loginOption !== LoginOption.BROWSER) { | ||
| this.emit('task:incoming', payload.data); | ||
| } else if (this.call) { |
There was a problem hiding this comment.
Just curious, is it possible to encounter a situation where we do not have the calling service created and we are receicing web socket events (for BROWSER) - I would assume if either was present we would be having both (as per the initialisation we do in cc.ts).
Only reason I'm asking is because we can avoid this double if condition. Just a query, however.
| @@ -142,7 +142,7 @@ export class WebSocketManager extends EventEmitter { | |||
| this.emit('message', e.data); | |||
| const eventData = JSON.parse(e.data); | |||
|
|
|||
| if (eventData.type === 'Welcome') { | |||
| if (eventData.type === CC_EVENTS.WELCOME) { | |||
COMPLETES #SPARK-562169
This pull request addresses
Call Accept Flow Implementation in CC SDK
by making the following changes
Created a new file contact.ts and moved the contact API code from the agent desktop to CC SDK. This file contains the API invocation a handling of success and failure events as per the agent desktop. This layer is present behind the services layer.
Create a new class called TaksCControl, a public module to access task-specific public methods. Implemented the call accept flow for the browser login option there.
Updated sample app to test the call accept flow.
Added UT for the changes done
Change Type
The following scenarios were tested
Scenario 1: Agent login with Browser option. Initiated an incoming call and answered it from the sample app.
Scenario 2: Agent login with Browser option. Initiated an incoming call and declined it from the sample app.
Scenario 3: Agent login with an Extension option. Initiated an incoming call, answer and decline buttons are disabled as the call needs to be answered from an actual device.
https://app.vidcast.io/share/358a656c-ca26-4d69-8aee-526b33345fe3
I certified that
I have read and followed contributing guidelines
I discussed changes with code owners prior to submitting this pull request
I have not skipped any automated checks
All existing and new tests passed
I have updated the documentation accordingly
Make sure to have followed the contributing guidelines before submitting.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests