Spark 586376 add calling rtms to u2c #4053
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 WalkthroughThis pull request introduces a comprehensive implementation of the Webex Contact Center (CC) plugin, adding a new package Changes
Sequence DiagramsequenceDiagram
participant Agent
participant WebexSDK
participant ContactCenter
participant WebSocket
participant TaskManager
Agent->>ContactCenter: Initialize
ContactCenter->>WebSocket: Connect
WebSocket-->>ContactCenter: Establish Connection
ContactCenter->>TaskManager: Register Listeners
Agent->>ContactCenter: Login
ContactCenter->>WebSocket: Send Login Request
WebSocket-->>ContactCenter: Login Response
ContactCenter->>TaskManager: Ready for Incoming Tasks
WebSocket->>TaskManager: New Task Notification
TaskManager->>Agent: Display Task Details
Agent->>ContactCenter: Accept/Decline Task
ContactCenter->>WebSocket: Task Action
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.
Actionable comments posted: 37
🧹 Nitpick comments (82)
packages/@webex/plugin-cc/src/services/core/websocket/types.ts (2)
9-15: Add JSDoc comments to clarify state combinationsWhile the boolean flags are clear individually, it would be helpful to document:
- Valid combinations of these states
- The lifecycle of connection states
- When each flag is set/unset
Add documentation like this:
+/** + * Represents the detailed state of a connection loss event. + * State combinations: + * - When connection is lost: {isConnectionLost: true, isRestoreFailed: false, isSocketReconnected: false, isKeepAlive: false} + * - During reconnection: {isConnectionLost: true, isRestoreFailed: false, isSocketReconnected: true, isKeepAlive: true} + * - After restore failure: {isConnectionLost: true, isRestoreFailed: true, isSocketReconnected: false, isKeepAlive: false} + */ export type ConnectionLostDetails = {
16-18: Enhance the timeout property definitionThe
lostConnectionRecoveryTimeoutproperty needs clarification:
- Add unit specification (milliseconds/seconds)
- Consider adding validation constraints
Consider updating the type like this:
export type ConnectionProp = { - lostConnectionRecoveryTimeout: number; + /** + * Timeout in milliseconds for connection recovery attempts. + * Must be between 1000ms and 30000ms. + * @minimum 1000 + * @maximum 30000 + */ + lostConnectionRecoveryTimeout: number; };packages/@webex/plugin-cc/src/services/core/Err.ts (1)
87-100: Avoid manually setting 'this.stack'; use 'super(value)' insteadIn the constructors of both
MessageandDetailsclasses, you're manually settingthis.stackafter callingsuper(). Instead, you can pass the error message or the error object tosuper()to automatically set the message and stack trace.Apply this diff to simplify the constructor:
export class Message extends Error { readonly id: Id; constructor(id: IdsMessage, value?: string | Error) { this.id = id; - super(); - this.stack = new Error().stack!; if (typeof value === 'string') { - this.message = value; + super(value); } else if (value instanceof Error) { - this.message = value.message; - this.name = value.name; + super(value.message); + this.name = value.name; } else { - this.message = ''; + super(); } } // Marker to distinguish Err class from other errors private isErr = 'yes'; }Similarly, apply the same changes to the
Detailsclass constructor.packages/@webex/plugin-cc/src/services/WebCallingService.ts (2)
58-63: Remove event listeners to prevent memory leaksThe event listener registered for
LINE_EVENTS.UNREGISTEREDonthis.lineis not removed, which can lead to memory leaks ifregisterWebCallingLineis called multiple times or if the line is replaced.Modify the
deregisterWebCallingLinemethod to remove the event listener:public async deregisterWebCallingLine() { + this.line?.off(LINE_EVENTS.UNREGISTERED, this.handleUnregistered); this.line?.deregister(); } +private handleUnregistered = () => { + LoggerProxy.log(`WxCC-SDK: Desktop unregistered successfully`, { + module: WEB_CALLING_SERVICE_FILE, + method: this.registerWebCallingLine.name, + }); +};And update the event listener registration:
- this.line.on(LINE_EVENTS.UNREGISTERED, () => { - LoggerProxy.log(`WxCC-SDK: Desktop unregistered successfully`, { - module: WEB_CALLING_SERVICE_FILE, - method: this.registerWebCallingLine.name, - }); - }); + this.line.on(LINE_EVENTS.UNREGISTERED, this.handleUnregistered);
66-70: Ensure event listeners are properly removed to prevent memory leaksSimilar to the previous comment, the event listener for
LINE_EVENTS.INCOMING_CALLshould be removed when it's no longer needed.Modify the
deregisterWebCallingLinemethod:public async deregisterWebCallingLine() { + this.line?.off(LINE_EVENTS.INCOMING_CALL, this.handleIncomingCall); this.line?.deregister(); } +private handleIncomingCall = (call: ICall) => { + this.call = call; + this.emit(LINE_EVENTS.INCOMING_CALL, call); +};And update the event listener registration:
- this.line.on(LINE_EVENTS.INCOMING_CALL, (call: ICall) => { - this.call = call; - this.emit(LINE_EVENTS.INCOMING_CALL, call); - }); + this.line.on(LINE_EVENTS.INCOMING_CALL, this.handleIncomingCall);packages/@webex/plugin-cc/src/services/core/websocket/connection-service.ts (2)
91-91: Avoid using 'any' type; specify the correct event typeUsing
anyreduces type safety and can lead to runtime errors. Please specify a more precise type for theeventparameter in theonPingmethod. Ifeventis a stringified JSON message, it should be typed asstring.private onPing = (event: string): void => {Alternatively, if
eventis aMessageEvent, adjust the code accordingly and useevent.datawhen parsing.
115-115: Simplify property access with optional chainingYou can make the code more concise and handle undefined values safely by using optional chaining. Replace:
this.connectionProp && this.connectionProp.lostConnectionRecoveryTimeoutwith:
this.connectionProp?.lostConnectionRecoveryTimeoutThis change ensures that if
this.connectionPropisundefinedornull, the expression will evaluate toundefined, preventing potential errors.🧰 Tools
🪛 Biome (1.9.4)
[error] 115-115: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/@webex/plugin-cc/src/services/task/TaskManager.ts (3)
19-19: Specify type for static 'taskManager' propertyThe
taskManagerstatic property lacks a type annotation. To enhance type safety and clarity, specify the type explicitly:private static taskManager: TaskManager;This ensures that the property is consistently used as a
TaskManagerinstance throughout the class.
101-101: Simplify conditional checks with optional chainingYou can simplify the condition by using optional chaining. Replace:
if (this.currentTask && this.currentTask.data && this.currentTask.data.interactionId) {with:
if (this.currentTask?.data?.interactionId) {This makes the code cleaner and easier to read by reducing repetitive null checks.
🧰 Tools
🪛 Biome (1.9.4)
[error] 101-101: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
113-115: Specify return type for 'getTask' methodFor improved type safety and readability, specify the return type of the
getTaskmethod. Replace:public getTask = (taskId: string) => {with:
public getTask = (taskId: string): ITask | undefined => {This explicitly indicates what the method returns, making the code more self-documenting.
packages/@webex/plugin-cc/src/services/core/websocket/WebSocketManager.ts (3)
129-129: Specify correct event type in 'onerror' handlerThe
eventparameter in theonerrorhandler should be typed asEventinstead ofany. Update the code to:this.websocket.onerror = (event: Event) => {This enhances type safety and code clarity, helping prevent misuse of the event object.
137-137: Specify correct event type in 'onclose' handlerThe
eventparameter in theonclosehandler should be typed asCloseEventinstead ofany. Update the code to:this.websocket.onclose = async (event: CloseEvent) => { this.webSocketOnCloseHandler(event); };This ensures that you have access to all the properties of the
CloseEventtype.
165-165: Specify correct event type in 'webSocketOnCloseHandler' methodThe
eventparameter inwebSocketOnCloseHandlershould be typed asCloseEventinstead ofany. Update the method signature to:private async webSocketOnCloseHandler(event: CloseEvent) {This change improves type safety and provides better context for the
eventobject.packages/@webex/plugin-cc/src/services/task/index.ts (5)
79-79: Address the TODO: Return the task object upon acceptanceThere's a TODO comment indicating that the
acceptmethod should be updated to return the task object received inAgentContactAssigned. Implementing this will provide the caller with the necessary task details after acceptance.Do you want me to implement the necessary changes to return the task object in the
acceptmethod or open a GitHub issue to track this task?
82-83: Implement accept API for Outbound Dialer scenarioThe TODO comment suggests invoking the accept API from the services layer for the outbound dialer scenario. This functionality is currently unimplemented, which may affect outbound call handling.
Would you like assistance in implementing the accept API call for the outbound dialer scenario or opening a GitHub issue to track this task?
85-87: Provide more context in error handlingWhen catching errors, consider adding additional context or using custom error classes to make debugging easier. This will help in identifying where and why the error occurred.
Apply this diff to include method names in the error details:
const {error: detailedError} = getErrorDetails(error, 'accept', CC_FILE); + detailedError.methodName = 'accept'; throw detailedError;
182-192: Use custom error classes for validation errorsIn the
wrapupmethod, genericErrorobjects are thrown for validation issues. Using custom error classes will make error handling more precise and informative.Define custom error classes and throw them for specific validation errors:
- throw new Error('AuxCodeId is required'); + throw new ValidationError('AuxCodeId is required'); - throw new Error('WrapUpReason is required'); + throw new ValidationError('WrapUpReason is required'); + class ValidationError extends Error { + constructor(message: string) { + super(message); + this.name = 'ValidationError'; + } + }
243-243: Implement consult and transfer methodsThere's a TODO comment indicating that public methods for consult and transfer need to be implemented. Adding these methods will enhance the task management capabilities.
Do you want me to help implement the
consultandtransfermethods or open a GitHub issue to track this task?packages/@webex/plugin-cc/src/services/config/Util.ts (2)
72-72: Correct the variable name fromdailPlantodialPlanThere's a typo in the variable name
dailPlan. It should bedialPlanto maintain consistency and prevent potential confusion.Apply this diff to fix the typo:
- dialPlanData.forEach((dailPlan: DialPlanEntity) => { + dialPlanData.forEach((dialPlan: DialPlanEntity) => {
245-248: Simplify nullish coalescing for boolean valuesIn the assignment of
maskSensitiveData, the current code checks the value twice, which can be simplified using a nullish coalescing operator.Apply this diff to simplify the assignment:
- maskSensitiveData: orgSettingsData.maskSensitiveData - ? orgSettingsData.maskSensitiveData - : false, + maskSensitiveData: orgSettingsData.maskSensitiveData ?? false,packages/@webex/plugin-cc/src/services/core/aqm-reqs.ts (3)
134-139: Remove redundant code when masking headersThe code that masks the
Authorizationheader in the error object is duplicated and can be consolidated to avoid redundancy.Apply this diff to remove the redundant code:
clear(); if (error?.headers) { error.headers.Authorization = '*'; } -if (error?.headers) { - error.headers.Authorization = '*'; -}
158-161: Ensure correct function name is logged in error messagesUsing
this.createPromise.namemay not return the intended function name, especially if the function is anonymous or bound differently. Explicitly provide the method name in the log.Apply this diff to specify the method name:
LoggerProxy.error(`Routing request timeout${keySuccess}${response!}${c.url}`, { module: AQM_REQS_FILE, - method: this.createPromise.name, + method: 'createPromise', });
268-271: Ensure correct function name is logged in info messagesSimilarly, in the
onMessagemethod, ensure that the correct function name is logged by explicitly specifying it.Apply this diff:
LoggerProxy.info(`event=missingEventHandler | [AqmReqs] missing routing message handler`, { module: AQM_REQS_FILE, - method: this.onMessage.name, + method: 'onMessage', });packages/@webex/plugin-cc/src/services/task/contact.ts (1)
23-378: Reduce code duplication by abstracting common patternsMany methods within the
routingContactfunction share similar structures, particularly in constructing request objects and handling notifications. Refactoring to abstract these common patterns can improve maintainability and reduce code duplication.Consider creating a helper function or using a factory pattern to generate request configurations. For example:
function createAqmRequestConfig(params: { interactionId: string; endpoint: string; data?: any; successEventType: string; failEventType: string; errId: string; timeout?: number | string; }) { return { url: `${TASK_API}${params.interactionId}${params.endpoint}`, data: params.data || {}, host: WCC_API_GATEWAY, err, timeout: params.timeout || TIMEOUT_REQ, notifSuccess: { bind: { type: TASK_MESSAGE_TYPE, data: { type: params.successEventType, interactionId: params.interactionId }, }, msg: {} as Contact.AgentContact, }, notifFail: { bind: { type: TASK_MESSAGE_TYPE, data: { type: params.failEventType }, }, errId: params.errId, }, }; }This function can then be used in the methods to reduce redundancy.
packages/@webex/plugin-cc/src/cc.ts (3)
146-146: Simplify Condition with Optional ChainingYou can simplify the condition at line 146 using optional chaining for improved readability.
Apply this diff:
- if (this.$config && this.$config.allowAutomatedRelogin) { + if (this.$config?.allowAutomatedRelogin) {🧰 Tools
🪛 Biome (1.9.4)
[error] 146-146: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
329-329: Simplify Condition with Optional ChainingSimilarly, the condition at line 329 can be simplified using optional chaining.
Apply this diff:
- if (this.$config && this.$config.allowAutomatedRelogin) { + if (this.$config?.allowAutomatedRelogin) {🧰 Tools
🪛 Biome (1.9.4)
[error] 329-329: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
253-257: Consistent Use of Device ID RetrievalIn the
getDeviceIdmethod, consider handling the default case explicitly to enhance code clarity.Apply this diff:
if (loginOption === LoginOption.EXTENSION || loginOption === LoginOption.AGENT_DN) { return dialNumber; } + // Handle default case explicitly + else if (loginOption === LoginOption.BROWSER) { return WEB_RTC_PREFIX + this.agentConfig.agentId; + } else { + throw new Error(`Unsupported login option: ${loginOption}`); + }This ensures that any unsupported
loginOptionvalues are caught and handled appropriately.packages/@webex/plugin-cc/src/services/config/index.ts (4)
157-157: Avoid Redundant Use ofPromise.resolvein Async FunctionsSince the function is
async, you can directly return the value without wrapping it inPromise.resolve.Apply this diff:
- return Promise.resolve(response.body); + return response.body;
195-195: Avoid Redundant Use ofPromise.resolvein Async FunctionsSimilarly, you can simplify the return statement in
getDesktopProfileById.Apply this diff:
- return Promise.resolve(response.body); + return response.body;
237-237: Avoid Redundant Use ofPromise.resolvein Async FunctionsSimplify the return statement in
getListOfTeams.Apply this diff:
- return Promise.resolve(response.body); + return response.body;
320-320: Avoid Redundant Use ofPromise.resolvein Async FunctionsSimplify the return statement in
getListOfAuxCodes.Apply this diff:
- return Promise.resolve(response.body); + return response.body;docs/samples/contact-center/app.js (4)
249-249: Simplify boolean assignment by avoiding unnecessary ternary operatorThe expression
dialNumber.disabled = agentProfile.defaultDn ? false : true;can be simplified by directly assigning the negated value ofagentProfile.defaultDn.Apply this diff to simplify the code:
-dialNumber.disabled = agentProfile.defaultDn ? false : true; +dialNumber.disabled = !agentProfile.defaultDn;
250-250: Simplify conditional setting ofloginAgentElm.disabledInstead of using an
ifstatement, you can assign thedisabledproperty directly based on the length ofloginVoiceOptions.Apply this diff to simplify the code:
-if (loginVoiceOptions.length > 0) loginAgentElm.disabled = false; +loginAgentElm.disabled = loginVoiceOptions.length === 0;🧰 Tools
🪛 Biome (1.9.4)
[error] 250-250: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
305-305: Remove unnecessaryasynckeyword fromhandleAgentLoginThe function
handleAgentLoginis declared asasyncbut does not contain anyawaitexpressions. Removing theasynckeyword improves readability.Apply this diff to remove the unnecessary
asynckeyword:-async function handleAgentLogin(e) { +function handleAgentLogin(e) {
328-328: Remove unnecessaryasynckeyword fromhandleAgentStatusThe function
handleAgentStatusis declared asasyncbut does not contain anyawaitexpressions. Removing theasynckeyword improves readability.Apply this diff to remove the unnecessary
asynckeyword:-async function handleAgentStatus(event) { +function handleAgentStatus(event) {packages/@webex/plugin-cc/test/unit/spec/services/config/index.ts (3)
90-95: Simplify async error handling in testsInstead of using
try-catchblocks to test rejected promises, useawait expect(...).rejects.toThrow(...)for cleaner and more concise code.Apply this diff to simplify the test:
-try { - await agentConfigService.getUserUsingCI(mockOrgId, mockAgentId); -} catch (error) { - expect(error).toEqual(new Error(`API call failed with ${mockResponse.statusCode}`)); -} +await expect(agentConfigService.getUserUsingCI(mockOrgId, mockAgentId)).rejects.toThrow(`API call failed with ${mockResponse.statusCode}`);
130-136: Simplify async error handling in testsReplace the
try-catchblock withawait expect(...).rejects.toThrow(...)for consistency and clarity.Apply this diff:
-const mockError = new Error('API call failed'); -(mockHttpRequest.request as jest.Mock).mockRejectedValue(mockError); - -try { - await agentConfigService.getDesktopProfileById(mockOrgId, desktopProfileId); -} catch (error) { - expect(error).toEqual(mockError); -} +const mockError = new Error('API call failed'); +(mockHttpRequest.request as jest.Mock).mockRejectedValue(mockError); +await expect(agentConfigService.getDesktopProfileById(mockOrgId, desktopProfileId)).rejects.toThrow('API call failed');
316-322: Consistently use async error handling patterns in testsUpdate this test to use
await expect(...).rejects.toThrow(...)instead of atry-catchblock for uniformity across tests.Apply this diff:
-await expect(agentConfigService.getOrgInfo(mockOrgId)).rejects.toThrow( - 'API call failed with 500' -); -expect(LoggerProxy.error).toHaveBeenCalledWith( - 'getOrgInfo API call failed with Error: API call failed with 500', - {module: CONFIG_FILE_NAME, method: 'getOrgInfo'} -); +await expect(agentConfigService.getOrgInfo(mockOrgId)).rejects.toThrow('API call failed with 500'); +expect(LoggerProxy.error).toHaveBeenCalledWith( + 'getOrgInfo API call failed with Error: API call failed with 500', + {module: CONFIG_FILE_NAME, method: 'getOrgInfo'} +);packages/@webex/plugin-cc/src/services/core/GlobalTypes.ts (2)
1-6: Consider restricting the default generic type parameterInstead of using
anyas the default type parameter, consider using a more restrictive type likeunknownorRecord<string, unknown>to improve type safety.-export type Msg<T = any> = { +export type Msg<T = unknown> = {
8-14: Avoid duplicate properties in Failure typeThe Failure type includes
trackingIdandorgIdwhich are already present in the parent Msg type. Consider removing these duplicates to maintain DRY principles.export type Failure = Msg<{ agentId: string; - trackingId: string; reasonCode: number; - orgId: string; reason: string; }>;packages/@webex/plugin-cc/src/services/core/constants.ts (2)
1-11: Add JSDoc comments for constantsConsider adding JSDoc comments to explain the purpose and rationale behind each timeout/interval constant. This will help maintainers understand why specific values were chosen.
Example:
/** Interval for worker keepalive checks in milliseconds */ export const KEEPALIVE_WORKER_INTERVAL = 4000; /** Delay before resolving notifications in milliseconds */ export const NOTIFS_RESOLVE_DELAY = 1200;
1-11: Consider using an enum or const object for related constantsGroup related constants together using an enum or const object for better organization and type safety.
export const TimeoutDurations = { /** Worker keepalive interval */ KEEPALIVE_WORKER: 4000, /** Notification resolution delay */ NOTIFS_RESOLVE: 1200, /** Socket timeout duration */ CLOSE_SOCKET: 16000, // ... other timeout related constants } as const; export const ApiEndpoints = { PING: '/health' } as const;packages/@webex/plugin-cc/src/services/task/constants.ts (2)
1-13: Add JSDoc comments to document the API endpoints and their purposes.Consider adding JSDoc comments to document:
- The purpose and expected behavior of each API endpoint
- Any required parameters or payload structures
- Expected responses and status codes
2-2: Consider version handling strategy for the API endpoint.The hardcoded
/v1/in the API path might need a versioning strategy for future API updates. Consider:
- Making the version configurable
- Adding a version constant that can be updated centrally
packages/@webex/plugin-cc/src/constants.ts (1)
1-2: Maintain consistent event naming pattern.Events should follow a consistent naming pattern:
EVENTandREADYuse simple stringsAGENT_STATE_CHANGEuses colon delimiter
Consider standardizing the event naming convention across all events.Also applies to: 14-14
packages/@webex/plugin-cc/src/services/core/HttpRequest.ts (1)
1-1: Consider adding request/response interceptors.To enhance functionality, consider adding support for:
- Request/response interceptors
- Default headers
- Response transformation
packages/@webex/plugin-cc/src/services/core/Utils.ts (2)
13-13: Improve type safety of error parameter.The
anytype for the error parameter reduces type safety. Consider creating a specific error interface.-export const getErrorDetails = (error: any, methodName: string, moduleName: string) => { +interface ServiceError { + details: Failure; +} +export const getErrorDetails = (error: ServiceError, methodName: string, moduleName: string) => {
23-27: Consider extracting error message construction.The error message construction logic is duplicated. Consider extracting it to a separate function.
+const getErrorMessage = (reason: string | undefined, methodName: string) => + reason ?? `Error while performing ${methodName}`; + return { - error: new Error(reason ?? `Error while performing ${methodName}`), + error: new Error(getErrorMessage(reason, methodName)), reason, };packages/@webex/plugin-cc/src/services/core/types.ts (2)
48-48: Replace void with undefined in union type.Using
voidin a union type can be confusing as it suggests a function return type rather than a value type.-export type CbRes<TRes> = (res: any) => void | TRes; +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)
19-42: Add JSDoc documentation for complex type.The
Reqtype is complex and would benefit from documentation explaining its purpose and parameters.+/** + * Represents a service request configuration. + * @template TRes - The expected response type + * @template TErr - The expected error type + */ export type Req<TRes, TErr> = {packages/@webex/plugin-cc/src/logger-proxy.ts (2)
11-15: Add error handling for undefined loggerCurrently, the code silently ignores logging when the logger is undefined. Consider either:
- Throwing an error if logger is undefined
- Providing a fallback logging mechanism
- At minimum, console.warn when logging is attempted without initialization
private static ensureLogger() { if (!LoggerProxy.logger) { console.warn('Logger not initialized. Log messages will be ignored.'); } }Also applies to: 17-21, 23-27, 29-33, 35-39
41-47: Validate LogContext structureThe
formatmethod assumescontexthas optionalmoduleandmethodproperties but doesn't validate the object structure. Consider adding type guards or validation:private static validateContext(context: unknown): context is LogContext { return ( typeof context === 'object' && context !== null && (('module' in context && typeof context.module === 'string') || !('module' in context)) && (('method' in context && typeof context.method === 'string') || !('method' in context)) ); }packages/@webex/plugin-cc/src/services/config/constants.ts (2)
2-3: Document pagination constantsThe default page size of 100 seems arbitrary. Consider adding documentation explaining why these values were chosen and their implications on performance and API limits.
8-15: Consider using TypeScript enums or const assertionsFor better type safety and IDE support, consider using TypeScript enums or const assertions for the attribute lists:
export const DEFAULT_AUXCODE_ATTRIBUTES = [ 'id', 'isSystemCode', 'name', 'defaultCode', 'workTypeCode', 'active', ] as const; export type AuxCodeAttribute = typeof DEFAULT_AUXCODE_ATTRIBUTES[number];packages/@webex/plugin-cc/test/unit/spec/services/core/HttpRequest.ts (1)
5-11: Improve mock type safetyInstead of type assertion, consider creating a proper mock factory:
function createMockWebex(): jest.Mocked<WebexSDK> { return { request: jest.fn(), logger: { log: jest.fn(), error: jest.fn(), }, }; } const mockWebex = createMockWebex();packages/@webex/plugin-cc/test/unit/spec/services/agent/index.ts (1)
42-42: Removeanytype usageInstead of using
anytype for test data, create proper type-safe mock objects:interface StationLoginData { // Add required properties } const mockStationLoginData: StationLoginData = { // Add mock data }; const req = await agent.stationLogin({data: mockStationLoginData});Also applies to: 49-49, 55-55
packages/webex/test/unit/spec/webex.js (1)
18-27: Consider enhancing the Worker mock implementationWhile the current implementation provides basic Worker functionality, consider adding error handling and event listeners for a more complete mock.
const mockWorker = { postMessage: jest.fn(), onmessage: jest.fn(), + onerror: jest.fn(), + addEventListener: jest.fn(), + removeEventListener: jest.fn(), + terminate: jest.fn(), };packages/@webex/plugin-cc/src/services/core/websocket/keepalive.worker.js (1)
4-6: Extract magic numbers into named constantsVariables are declared but their values are set through messages. Consider defining default values as named constants for better maintainability.
+const DEFAULT_INTERVAL_DURATION = 4000; +const DEFAULT_CLOSE_SOCKET_TIMEOUT = 5000; let intervalId, intervalDuration, timeOutId, isSocketClosed, closeSocketTimeout; let initialised = false; let initiateWebSocketClosure = false;packages/@webex/plugin-cc/src/services/agent/types.ts (2)
3-14: Extract common message properties into a base typeLogoutSuccess and ReloginSuccess share common properties. Consider creating a base type to reduce duplication.
+type BaseAgentMessage = { + eventType: 'AgentDesktopMessage'; + agentId: string; + trackingId: string; + agentSessionId: string; + orgId: string; + status: string; +}; -export type LogoutSuccess = Msg<{ +export type LogoutSuccess = Msg<BaseAgentMessage & { - eventType: 'AgentDesktopMessage'; - agentId: string; - trackingId: string; - agentSessionId: string; - orgId: string; - status: string; subStatus: string; loggedOutBy?: string; roles?: string[]; type: 'AgentLogoutSuccess'; }>;Also applies to: 16-41
114-120: Enhance documentation for BuddyAgents typeWhile there's a comment for the state property, other properties lack documentation.
export type BuddyAgents = { + /** Unique identifier for the agent profile */ agentProfileId: string; + /** Type of media channel (e.g., 'voice', 'chat', 'email') */ mediaType: string; /** Filter for agent state eg : Available | Idle */ state?: string; };packages/@webex/plugin-cc/src/services/agent/index.ts (2)
15-36: Consider adding JSDoc documentation for the routingAgent functionWhile the function implementation is solid, adding comprehensive JSDoc documentation would improve maintainability and developer experience.
+/** + * Creates an agent routing service with various agent management operations + * @param {AqmReqs} routing - The AQM requests instance for handling API calls + * @returns {Object} Object containing agent management methods + */ export default function routingAgent(routing: AqmReqs) {
61-66: Consider enhancing error handling for station loginThe error handling for station login could be more robust by including additional error details.
Consider adding more specific error types and handling network-related errors:
err: /* istanbul ignore next */ (e: any) => new Err.Details('Service.aqm.agent.stationLogin', { status: e.response?.status ?? 0, type: e.response?.data?.errorType, trackingId: e.response?.headers?.trackingid?.split('_')[1], + networkError: e.message, + timestamp: new Date().toISOString(), }),packages/@webex/plugin-cc/test/unit/spec/services/task/contact.ts (1)
158-166: Enhance test coverage for wrapup contactThe test case should validate the response structure and error scenarios.
Add additional test cases:
it("wrapup contact with invalid data", () => { fakeAqm.pendingRequests = {}; expect(() => contact.wrapup({ interactionId: "", data: { wrapUpReason: "" } })).toThrow(); }); it("wrapup contact success response", async () => { const response = { success: true }; fakeAqm.req.mockResolvedValueOnce(response); const result = await contact.wrapup({ interactionId: "test", data: { wrapUpReason: "reason" } }); expect(result).toEqual(response); });packages/@webex/plugin-cc/src/types.ts (1)
235-246: Consider using string literal union for mediaTypeInstead of using string literals in the documentation, define them as a union type for better type safety.
+type MediaType = 'telephony' | 'chat' | 'social' | 'email'; +type AgentState = 'Available' | 'Idle'; + export type BuddyAgents = { - mediaType: 'telephony' | 'chat' | 'social' | 'email'; + mediaType: MediaType; - state?: 'Available' | 'Idle'; + state?: AgentState; };packages/@webex/plugin-cc/test/unit/spec/services/core/websocket/connection-service.ts (2)
112-112: Consider reducing test timeouts.The test timeouts are set to 20 seconds (20000ms), which is excessive for unit tests. Unit tests should complete quickly, typically under 1000ms. Consider reducing these timeouts unless there's a specific reason for the long duration.
- }, 20000); // Increased timeout to 20 seconds + }, 1000); // 1 second should be sufficient for unit testsAlso applies to: 144-144, 152-152, 190-190
Line range hint
1-293: Add missing test cases for error scenarios.The test suite could benefit from additional test cases:
- Error handling for invalid ping messages
- Network timeout scenarios
- WebSocket connection failures
- Edge cases for timer management
Would you like me to provide example test cases for these scenarios?
packages/@webex/plugin-cc/test/unit/spec/services/WebCallingService.ts (1)
206-292: Enhance test coverage for call management scenarios.Consider adding test cases for:
- Handling concurrent calls
- Validating audio stream properties
- Testing call state transitions
Example test case for concurrent calls:
it('should handle concurrent calls correctly', () => { const mockCall1 = { /* ... */ }; const mockCall2 = { /* ... */ }; // Simulate incoming concurrent calls webRTCCalling['call'] = mockCall1; const eventListener = jest.fn(); webRTCCalling.on('line:incoming_call', eventListener); // Verify behavior expect(eventListener).toHaveBeenCalledWith(mockCall1); expect(() => webRTCCalling.answerCall(localAudioStream, 'task-2')).toThrow(); });packages/@webex/plugin-cc/test/unit/spec/services/core/websocket/WebSocketManager.ts (3)
92-93: Avoid hardcoding URLs in tests.Consider using a constant or configuration value instead of hardcoding the blob URL.
- return 'blob:http://localhost:3000/12345'; + const MOCK_BLOB_URL = 'blob:mock-url'; + return MOCK_BLOB_URL;
97-101: Standardize timeout handling in tests.The test file uses different approaches for handling timeouts. Consider standardizing the timeout handling using Jest's timer mocks consistently throughout the tests.
Example standardization:
beforeEach(() => { jest.useFakeTimers(); }); afterEach(() => { jest.useRealTimers(); }); it('should handle event', () => { // Trigger event MockWebSocket.inst.onopen(); // Advance timers jest.advanceTimersByTime(10); // Assert expect(/* ... */); });Also applies to: 157-165, 190-197, 297-304, 330-337
1-378: Add test cases for WebSocket reconnection scenarios.The test suite would benefit from additional test cases covering:
- Maximum reconnection attempts
- Exponential backoff
- Recovery after temporary network issues
Would you like me to provide example test cases for these scenarios?
packages/@webex/plugin-cc/test/unit/spec/services/task/index.ts (1)
1-93: Test setup looks good but could be improved with shared test fixtures.The test setup is comprehensive with proper mocking of dependencies. However, there's repetition in test data like
taskDataMockthat could be moved to a shared fixture.Consider creating a shared test fixture:
// test/fixtures/taskData.ts export const createTaskDataMock = (overrides = {}) => ({ type: CC_EVENTS.AGENT_CONTACT_RESERVED, agentId: "723a8ffb-a26e-496d-b14a-ff44fb83b64f", // ... other default properties ...overrides });packages/@webex/plugin-cc/test/unit/spec/services/task/TaskManager.ts (1)
249-282: Improve test coverage for call listener cleanup.The test verifies basic cleanup but should check for memory leaks.
Add memory leak detection:
it('should properly clean up all listeners on call end', async () => { const getListenerCount = (emitter, event) => emitter.listeners(event).length; // Store initial listener counts const initialCounts = { webSocket: getListenerCount(webSocketManagerMock, 'message'), webCalling: getListenerCount(webCallingService, LINE_EVENTS.INCOMING_CALL) }; // Simulate task lifecycle webSocketManagerMock.emit('message', JSON.stringify({data: taskDataMock})); await task.accept(); // End task webSocketManagerMock.emit('message', JSON.stringify(payload)); taskManager.unregisterIncomingCallEvent(); // Verify all listeners are removed expect(getListenerCount(webSocketManagerMock, 'message')) .toBe(initialCounts.webSocket); expect(getListenerCount(webCallingService, LINE_EVENTS.INCOMING_CALL)) .toBe(initialCounts.webCalling); });packages/@webex/plugin-cc/src/services/config/types.ts (1)
491-562: Improve Profile type with stricter validation and defaults.The Profile type has many optional fields without clear documentation on when they're required.
Consider using branded types and providing helper functions:
// Add branded types for better type safety type AgentId = string & { readonly brand: unique symbol }; type ProfileId = string & { readonly brand: unique symbol }; export type Profile = { // Required fields agentId: AgentId; agentProfileID: ProfileId; webRtcEnabled: boolean; // Optional fields with clear conditions microsoftConfig?: { showUserDetailsMS: boolean; stateSynchronizationMS: boolean; }; // ... rest of the fields }; // Add helper functions export const createProfile = ( required: Pick<Profile, 'agentId' | 'agentProfileID' | 'webRtcEnabled'>, optional?: Partial<Omit<Profile, 'agentId' | 'agentProfileID' | 'webRtcEnabled'>> ): Profile => ({ ...required, ...optional });packages/@webex/internal-plugin-mercury/src/mercury.js (1)
519-527: Consider simplifying the event emission logic.The event emission logic could be simplified to reduce code duplication.
Consider this refactoring:
- if (data.eventType) { - const [namespace] = data.eventType.split('.'); - - if (namespace === data.eventType) { - this._emit(`event:${namespace}`, envelope); - } else { - this._emit(`event:${namespace}`, envelope); - this._emit(`event:${data.eventType}`, envelope); - } - } + if (data.eventType) { + const [namespace] = data.eventType.split('.'); + this._emit(`event:${namespace}`, envelope); + if (namespace !== data.eventType) { + this._emit(`event:${data.eventType}`, envelope); + } + }packages/@webex/plugin-cc/test/unit/spec/services/core/aqm-reqs.ts (1)
89-116: Consider adding more assertions to the basic definition test.The test verifies error handling but doesn't assert the successful case.
Add assertions for the successful case:
it('AqmReqs should be defined', async () => { httpRequestInstance.request.mockResolvedValueOnce(mockHttpRequestResolvedValue); const req = aqm.req(() => ({ // ... existing config ... })); // Add success case webSocketManagerInstance.emit( 'message', JSON.stringify({ type: 'RoutingMessage', data: { type: 'AgentConsultConferenced', interactionId: 'intrid' } }) ); const result = await req({}); expect(result).toBeDefined(); expect(result.type).toBe('RoutingMessage'); });packages/@webex/internal-plugin-mercury/test/unit/spec/socket.js (1)
203-243: Fix typo in test description.There's a typo in the test description "calles" should be "calls".
- it('accepts authorizationRequired option as true and calles authorize', () => { + it('accepts authorizationRequired option as true and calls authorize', () => {packages/@webex/plugin-cc/test/unit/spec/cc.ts (3)
74-89: Consider adding test coverage for error scenarios in contact methods.The mock contact object is well-defined but lacks error scenarios. Consider adding test cases for when these methods fail.
171-259: Enhance test coverage for register method.While the happy path is well tested, consider adding test cases for:
- Network timeouts
- Invalid responses
- Partial success scenarios where some operations succeed but others fail
738-855: Add test coverage for concurrent silentRelogin calls.The silentRelogin tests are thorough for individual scenarios, but missing coverage for potential race conditions when multiple relogin attempts occur simultaneously.
docs/samples/contact-center/style.css (1)
13-29: Standardize color values and improve accessibility.The CSS uses a mix of hex and rgb color values. Additionally, some color combinations might have insufficient contrast for accessibility.
- Standardize color format (prefer hex)
- Consider using CSS custom properties for colors
- Verify color contrast ratios meet WCAG guidelines
Example refactor:
:root { + /* Theme colors */ + --color-primary: #00ab50; + --color-danger: #f7644a; + --color-text-light: #ffffff; + /* ... add other colors ... */ } button.btn--green { - color: #fff; + color: var(--color-text-light); - background-color: #00ab50; + background-color: var(--color-primary); } button.btn--red { - color: #fff; + color: var(--color-text-light); - background-color: #f7644a; + background-color: var(--color-danger); }Also applies to: 383-386
docs/samples/contact-center/index.html (2)
185-187: Add error handling for script loading.The script tags lack error handling and loading status indication.
Add error handling:
- <script src="../webex.min.js"></script> - <script src="../contact-center.min.js"></script> - <script src="app.js"></script> + <script src="../webex.min.js" onerror="handleScriptError('webex.min.js')"></script> + <script src="../contact-center.min.js" onerror="handleScriptError('contact-center.min.js')"></script> + <script src="app.js" onerror="handleScriptError('app.js')"></script> + <script> + function handleScriptError(script) { + console.error(`Failed to load ${script}`); + // Add user notification logic + } + </script>
46-49: Consider moving inline event handlers to JavaScript file.Inline event handlers are used throughout the HTML. This practice is discouraged for security reasons (CSP) and maintainability.
Example refactor:
- <select name="auth-type" id="auth-type" onchange="changeAuthType()"> + <select name="auth-type" id="auth-type">Then in app.js:
document.getElementById('auth-type').addEventListener('change', changeAuthType);Also applies to: 116-118
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (67)
.github/workflows/deploy.yml(3 hunks)docs/index.html(1 hunks)docs/samples/contact-center/app.js(1 hunks)docs/samples/contact-center/index.html(1 hunks)docs/samples/contact-center/style.css(1 hunks)packages/@webex/internal-plugin-mercury/src/mercury.js(3 hunks)packages/@webex/internal-plugin-mercury/src/socket/socket-base.js(4 hunks)packages/@webex/internal-plugin-mercury/test/unit/spec/mercury.js(1 hunks)packages/@webex/internal-plugin-mercury/test/unit/spec/socket.js(3 hunks)packages/@webex/plugin-cc/README.md(1 hunks)packages/@webex/plugin-cc/__mocks__/workerMock.js(1 hunks)packages/@webex/plugin-cc/babel.config.json(1 hunks)packages/@webex/plugin-cc/jest.config.js(1 hunks)packages/@webex/plugin-cc/package.json(1 hunks)packages/@webex/plugin-cc/src/cc.ts(1 hunks)packages/@webex/plugin-cc/src/config.ts(1 hunks)packages/@webex/plugin-cc/src/constants.ts(1 hunks)packages/@webex/plugin-cc/src/index.ts(1 hunks)packages/@webex/plugin-cc/src/logger-proxy.ts(1 hunks)packages/@webex/plugin-cc/src/services/WebCallingService.ts(1 hunks)packages/@webex/plugin-cc/src/services/agent/index.ts(1 hunks)packages/@webex/plugin-cc/src/services/agent/types.ts(1 hunks)packages/@webex/plugin-cc/src/services/config/Util.ts(1 hunks)packages/@webex/plugin-cc/src/services/config/constants.ts(1 hunks)packages/@webex/plugin-cc/src/services/config/index.ts(1 hunks)packages/@webex/plugin-cc/src/services/config/types.ts(1 hunks)packages/@webex/plugin-cc/src/services/constants.ts(1 hunks)packages/@webex/plugin-cc/src/services/core/Err.ts(1 hunks)packages/@webex/plugin-cc/src/services/core/GlobalTypes.ts(1 hunks)packages/@webex/plugin-cc/src/services/core/HttpRequest.ts(1 hunks)packages/@webex/plugin-cc/src/services/core/Utils.ts(1 hunks)packages/@webex/plugin-cc/src/services/core/aqm-reqs.ts(1 hunks)packages/@webex/plugin-cc/src/services/core/constants.ts(1 hunks)packages/@webex/plugin-cc/src/services/core/types.ts(1 hunks)packages/@webex/plugin-cc/src/services/core/websocket/WebSocketManager.ts(1 hunks)packages/@webex/plugin-cc/src/services/core/websocket/connection-service.ts(1 hunks)packages/@webex/plugin-cc/src/services/core/websocket/keepalive.worker.js(1 hunks)packages/@webex/plugin-cc/src/services/core/websocket/types.ts(1 hunks)packages/@webex/plugin-cc/src/services/index.ts(1 hunks)packages/@webex/plugin-cc/src/services/task/TaskManager.ts(1 hunks)packages/@webex/plugin-cc/src/services/task/constants.ts(1 hunks)packages/@webex/plugin-cc/src/services/task/contact.ts(1 hunks)packages/@webex/plugin-cc/src/services/task/index.ts(1 hunks)packages/@webex/plugin-cc/src/services/task/types.ts(1 hunks)packages/@webex/plugin-cc/src/types.ts(1 hunks)packages/@webex/plugin-cc/test/unit/spec/cc.ts(1 hunks)packages/@webex/plugin-cc/test/unit/spec/services/WebCallingService.ts(1 hunks)packages/@webex/plugin-cc/test/unit/spec/services/agent/index.ts(1 hunks)packages/@webex/plugin-cc/test/unit/spec/services/config/index.ts(1 hunks)packages/@webex/plugin-cc/test/unit/spec/services/core/HttpRequest.ts(1 hunks)packages/@webex/plugin-cc/test/unit/spec/services/core/aqm-reqs.ts(1 hunks)packages/@webex/plugin-cc/test/unit/spec/services/core/websocket/WebSocketManager.ts(1 hunks)packages/@webex/plugin-cc/test/unit/spec/services/core/websocket/connection-service.ts(1 hunks)packages/@webex/plugin-cc/test/unit/spec/services/task/TaskManager.ts(1 hunks)packages/@webex/plugin-cc/test/unit/spec/services/task/contact.ts(1 hunks)packages/@webex/plugin-cc/test/unit/spec/services/task/index.ts(1 hunks)packages/@webex/plugin-cc/tsconfig.json(1 hunks)packages/@webex/test-helper-mock-webex/src/index.js(1 hunks)packages/@webex/webex-core/src/lib/services/service-catalog.js(1 hunks)packages/@webex/webex-core/test/unit/spec/services/service-catalog.js(1 hunks)packages/calling/src/index.ts(1 hunks)packages/webex/package.json(2 hunks)packages/webex/src/contact-center.js(1 hunks)packages/webex/src/webex.js(1 hunks)packages/webex/test/unit/spec/webex.js(2 hunks)tooling/lib/build.js(1 hunks)webpack.config.js(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- packages/@webex/plugin-cc/tsconfig.json
- packages/@webex/plugin-cc/babel.config.json
- packages/@webex/plugin-cc/src/services/constants.ts
🧰 Additional context used
🪛 Markdownlint (0.37.0)
packages/@webex/plugin-cc/README.md
70-70: Expected: h2; Actual: h3
Heading levels should only increment by one level at a time
(MD001, heading-increment)
71-71: null
Bare URL used
(MD034, no-bare-urls)
17-17: null
No empty links
(MD042, no-empty-links)
18-18: null
No empty links
(MD042, no-empty-links)
🪛 Biome (1.9.4)
packages/@webex/plugin-cc/test/unit/spec/services/core/aqm-reqs.ts
[error] 69-69: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
[error] 70-70: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
packages/@webex/plugin-cc/src/services/core/websocket/connection-service.ts
[error] 115-115: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
docs/samples/contact-center/app.js
[error] 250-250: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
packages/@webex/internal-plugin-mercury/src/mercury.js
[error] 406-406: Illegal return statement outside of a function
(parse)
packages/@webex/plugin-cc/src/services/index.ts
[error] 34-34: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 35-35: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 38-38: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
packages/@webex/plugin-cc/src/services/task/TaskManager.ts
[error] 101-101: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/@webex/plugin-cc/src/cc.ts
[error] 146-146: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 329-329: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/@webex/plugin-cc/src/services/core/types.ts
[error] 48-48: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
packages/@webex/plugin-cc/src/services/task/types.ts
[error] 326-326: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
packages/@webex/plugin-cc/src/logger-proxy.ts
[error] 4-48: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Initialize Project
- GitHub Check: AWS Amplify Console Web Preview
🔇 Additional comments (30)
packages/@webex/plugin-cc/src/services/core/websocket/types.ts (2)
4-8: LGTM! Well-structured type definitionThe
ConnectionServiceOptionstype is well-defined with clear, required properties that ensure proper initialization of the connection service.
1-2: Verify the imported types exist in the referenced pathsThe imports suggest a well-organized module structure. Let's verify these dependencies exist.
✅ Verification successful
Imports verified successfully
Both imported types exist and are correctly referenced:
SubscribeRequestis properly exported as a type fromtypes.tsWebSocketManageris properly exported as a class fromWebSocketManager.ts🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the imported types exist in their respective paths # Check for SubscribeRequest type echo "Checking SubscribeRequest type..." ast-grep --pattern 'export type SubscribeRequest = $_' packages/@webex/plugin-cc/src/types.ts # Check for WebSocketManager class/type echo "Checking WebSocketManager type..." ast-grep --pattern 'export (class|type|interface) WebSocketManager' packages/@webex/plugin-cc/src/services/core/websocket/WebSocketManager.tsLength of output: 786
Script:
#!/bin/bash # Broader search for WebSocketManager using different patterns echo "Searching for WebSocketManager with different patterns..." # Check for default export ast-grep --pattern 'export default class $_WebSocketManager' packages/@webex/plugin-cc/src/services/core/websocket/WebSocketManager.ts # Check for any class definition containing WebSocketManager ast-grep --pattern 'class $_WebSocketManager' packages/@webex/plugin-cc/src/services/core/websocket/WebSocketManager.ts # Fallback to ripgrep for case-insensitive search of the file content echo "Performing case-insensitive content search..." rg -i "websocketmanager" packages/@webex/plugin-cc/src/services/core/websocket/WebSocketManager.ts # Check if the file exists echo "Checking if the file exists..." ls -l packages/@webex/plugin-cc/src/services/core/websocket/WebSocketManager.tsLength of output: 912
packages/@webex/webex-core/test/unit/spec/services/service-catalog.js (1)
211-312: Well-written tests covering multiple scenariosThe newly added test cases effectively validate the
findServiceUrlFromUrlmethod, ensuring it handles various scenarios correctly, including invalid URLs, alternate hostnames, and different protocols. This improves the robustness of the function and enhances code quality.packages/@webex/webex-core/src/lib/services/service-catalog.js (1)
248-307: Enhanced URL Parsing and Matching Logic Looks GoodThe updated implementation of the
findServiceUrlFromUrlmethod provides robust URL parsing with proper error handling. The logic for exact and any protocol matches improves the flexibility of service URL detection.packages/@webex/plugin-cc/src/index.ts (1)
1-10: LGTM!The plugin registration and export are correctly implemented.
packages/@webex/plugin-cc/src/constants.ts (1)
3-3: Document the rationale for the 20-second timeout.While the comment indicates this is for WebRTC registration, consider:
- Documenting why 20 seconds was chosen
- Making this value configurable for different environments
- Adding retry logic if needed
packages/@webex/plugin-cc/src/config.ts (1)
5-6: Document security implications of login settings.The
allowMultiLoginandallowAutomatedReloginflags have security implications:
- Document the security considerations
- Add validation for concurrent sessions
- Consider rate limiting for automated relogin
packages/@webex/plugin-cc/jest.config.js (2)
11-18: LGTM! Good coverage thresholds.Setting an 85% coverage threshold for lines, functions, branches, and statements is a good practice for a new package. This ensures high-quality test coverage from the start.
5-5: Review empty transformIgnorePatterns array.An empty
transformIgnorePatternsarray means Jest will transform all files, including those innode_modules. This can significantly slow down tests and may cause issues with some packages.Consider adding the default pattern
node_modules/(?!(@webex))to transform only @webex packages while excluding other node_modules:- transformIgnorePatterns: [], + transformIgnorePatterns: ['node_modules/(?!(@webex))'],✅ Verification successful
Empty transformIgnorePatterns is acceptable for TypeScript packages
The empty transformIgnorePatterns array is appropriate for this TypeScript-based package as it ensures all dependencies can be properly transformed, especially when dealing with ESM and TypeScript modules. This configuration provides maximum compatibility during development and testing.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Find all Jest config files fd jest.config.js # Check package.json for dependencies cat packages/@webex/plugin-cc/package.json # Look for test setup documentation fd -t f -e md | xargs rg -l "jest|test" --ignore-caseLength of output: 7515
Script:
#!/bin/bash # Check transformIgnorePatterns in other Jest configs rg "transformIgnorePatterns" packages/@webex/*/jest.config.js -A 1 -B 1Length of output: 3862
packages/calling/src/index.ts (1)
55-55: LGTM! Clean export addition.The export of
CallingClientConfigfollows the established pattern and integrates well with existing exports.packages/@webex/plugin-cc/test/unit/spec/services/core/HttpRequest.ts (1)
27-51: Add more edge cases to request testsThe success test case is good, but consider adding tests for:
- Network timeouts
- Rate limiting responses
- Malformed responses
- Different HTTP methods
- Query parameter handling
packages/webex/test/unit/spec/webex.js (1)
4-4: LGTM! Essential DOM environment setup for testsThe addition of jsdom-global/register is necessary for simulating a DOM environment in Node.js tests.
packages/webex/src/webex.js (1)
30-30: LGTM! Proper integration of the CC pluginThe Contact Center plugin is correctly integrated following the established pattern of plugin registration.
packages/@webex/plugin-cc/src/services/core/websocket/keepalive.worker.js (1)
1-2: Address TODO comment regarding Blob usageThe comment indicates a known issue with using Blob for worker script creation. Consider using a separate file and proper bundling strategy instead of string templates.
Would you like me to propose an alternative implementation using proper module bundling?
tooling/lib/build.js (1)
66-66: LGTM! Clean up of webexcc.min. files*The addition follows the existing pattern of cleaning up minified files before building samples.
packages/@webex/plugin-cc/src/services/agent/index.ts (1)
1-8: LGTM! Well-structured imports and dependenciesThe imports are properly organized and typed, with clear separation of concerns between error handling, utilities, and types.
packages/@webex/test-helper-mock-webex/src/index.js (1)
17-17: LGTM!The addition of 'cc' to nonInternalPlugins is correctly placed and aligns with the plugin system's requirements.
packages/@webex/internal-plugin-mercury/src/mercury.js (2)
220-221: LGTM! Good use of nullish coalescing operator.The new options
authorizationRequiredandacknowledgementRequiredare well-implemented with sensible defaults using the nullish coalescing operator (??). This maintains backward compatibility while allowing these features to be optionally disabled.
405-408: LGTM! Defensive programming to handle undefined eventType.Good addition of the null check for
eventType. This prevents potential runtime errors when splitting undefined values.🧰 Tools
🪛 Biome (1.9.4)
[error] 406-406: Illegal return statement outside of a function
(parse)
packages/@webex/plugin-cc/test/unit/spec/services/core/aqm-reqs.ts (3)
173-174: LGTM! Good test coverage for acknowledgement feature.The test properly verifies that the
_acknowledgemethod is not called whenacknowledgementRequiredis false.Also applies to: 198-199
204-206: LGTM! Good test coverage for authorization feature.The test properly verifies that the
_authorizemethod is not called whenauthorizationRequiredis false.Also applies to: 220-221
482-520: LGTM! Good error handling test coverage.The test properly verifies custom error handling with the
errfunction.packages/@webex/internal-plugin-mercury/test/unit/spec/socket.js (2)
Line range hint
142-170: LGTM! Good test coverage for acknowledgement feature.The test properly verifies that messages are acknowledged when
acknowledgementRequiredis true.
172-201: LGTM! Good test coverage for skipping acknowledgement.The test properly verifies that messages are not acknowledged when
acknowledgementRequiredis false.packages/@webex/internal-plugin-mercury/test/unit/spec/mercury.js (1)
717-730: LGTM! Good test coverage for undefined eventType handling.The test thoroughly verifies the behavior when eventType is undefined:
- Checks that _getEventHandlers is called with undefined
- Verifies that _emit is called with the correct event data
- Ensures no errors are logged
packages/@webex/plugin-cc/test/unit/spec/cc.ts (1)
1-19: LGTM! Well-organized test setup with comprehensive imports.The test file is well-structured with clear organization of imports and necessary mock setups.
packages/webex/package.json (1)
21-21: LGTM! Clean integration of the contact center plugin.The changes properly integrate the new contact center plugin through both exports and dependencies.
Also applies to: 81-81
docs/index.html (1)
80-80: LGTM!The new link for the Contact Center sample follows the established pattern and integrates well with the existing samples section.
webpack.config.js (1)
50-56: LGTM!The new webpack entry point for 'contact-center' follows the established pattern and correctly configures the library name and type.
.github/workflows/deploy.yml (1)
8-8: LGTM!The deployment configuration changes correctly:
- Add 'wxcc' as a deployable branch
- Skip documentation publishing for the 'wxcc' branch
Also applies to: 212-212
| if (!this.instance) { | ||
| this.instance = new Services(options); |
There was a problem hiding this comment.
Replace 'this' with the class name 'Services' in static method
Using this in a static context can be confusing because it refers to the class itself, not an instance. It's clearer to use the class name Services directly.
Apply this diff to fix the issue:
public static getInstance(options: {
webex: WebexSDK;
connectionConfig: SubscribeRequest;
}): Services {
- if (!this.instance) {
- this.instance = new Services(options);
+ if (!Services.instance) {
+ Services.instance = new Services(options);
}
- return this.instance;
+ return Services.instance;
}Also applies to: 38-38
🧰 Tools
🪛 Biome (1.9.4)
[error] 34-34: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 35-35: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
| public declineCall(taskId: string) { | ||
| if (this.call) { | ||
| try { | ||
| this.webex.logger.info(`Call end requested: ${taskId}`); | ||
| this.call.end(); | ||
| this.unregisterCallListeners(); | ||
| } catch (error) { | ||
| this.webex.logger.error(`Failed to end call: ${taskId}. Error: ${error}`); | ||
| // Optionally, throw the error to allow the invoker to handle it | ||
| throw error; | ||
| } | ||
| } else { | ||
| this.webex.logger.log(`Cannot end a non WebRtc Call: ${taskId}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
Validate task ID before declining the call
Similarly, in the declineCall method, ensure that the call corresponds to the provided taskId.
Modify the method as follows:
public declineCall(taskId: string) {
if (this.call) {
+ const callTaskId = this.call.id; // Assuming 'id' represents the task ID
+ if (callTaskId !== taskId) {
+ this.webex.logger.error(`Task ID mismatch: expected ${taskId}, got ${callTaskId}`);
+ return;
+ }
try {
this.webex.logger.info(`Call end requested: ${taskId}`);
this.call.end();
this.unregisterCallListeners();
} catch (error) {
this.webex.logger.error(`Failed to end call: ${taskId}. Error: ${error}`);
throw error;
}
} else {
this.webex.logger.log(`Cannot end a non WebRTC Call: ${taskId}`);
}
}Ensure you replace this.call.id with the correct property or method to obtain the call's task ID.
Committable suggestion skipped: line range outside the PR's diff.
| public answerCall(localAudioStream: LocalMicrophoneStream, taskId: string) { | ||
| if (this.call) { | ||
| try { | ||
| this.webex.logger.info(`Call answered: ${taskId}`); | ||
| this.call.answer(localAudioStream); | ||
| this.registerCallListeners(); | ||
| } catch (error) { | ||
| this.webex.logger.error(`Failed to answer call for ${taskId}. Error: ${error}`); | ||
| // Optionally, throw the error to allow the invoker to handle it | ||
| throw error; | ||
| } | ||
| } else { | ||
| this.webex.logger.log(`Cannot answer a non WebRtc Call: ${taskId}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
Validate task ID before answering the call
In the answerCall method, it's important to verify that the incoming call's task ID matches the provided taskId to ensure the correct call is handled.
Update the method to include task ID validation:
public answerCall(localAudioStream: LocalMicrophoneStream, taskId: string) {
if (this.call) {
+ const callTaskId = this.call.id; // Assuming 'id' represents the task ID
+ if (callTaskId !== taskId) {
+ this.webex.logger.error(`Task ID mismatch: expected ${taskId}, got ${callTaskId}`);
+ return;
+ }
try {
this.webex.logger.info(`Call answered: ${taskId}`);
this.call.answer(localAudioStream);
this.registerCallListeners();
} catch (error) {
this.webex.logger.error(`Failed to answer call for ${taskId}. Error: ${error}`);
throw error;
}
} else {
this.webex.logger.log(`Cannot answer a non WebRTC Call: ${taskId}`);
}
}Replace this.call.id with the appropriate property or method to retrieve the call's task ID.
Committable suggestion skipped: line range outside the PR's diff.
| const {webex} = options; | ||
| this.webex = webex; | ||
| this.shouldReconnect = true; | ||
| this.websocket = {} as WebSocket; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Properly initialize 'this.websocket' to avoid runtime errors
Assigning an empty object and casting it to WebSocket is unsafe and can lead to runtime errors. Instead, initialize this.websocket to null and adjust its type accordingly. Update the declaration to:
private websocket: WebSocket | null = null;Then, ensure all usages of this.websocket handle the null case appropriately, adding necessary null checks where needed.
| const localStream = await navigator.mediaDevices.getUserMedia(constraints); | ||
| const audioTrack = localStream.getAudioTracks()[0]; | ||
| this.localAudioStream = new LocalMicrophoneStream(new MediaStream([audioTrack])); | ||
| this.webCallingService.answerCall(this.localAudioStream, this.data.interactionId); |
There was a problem hiding this comment.
Handle potential errors when accessing audio tracks
In the accept method, after obtaining the localStream, there's no check to ensure that audio tracks are available. If localStream.getAudioTracks() returns an empty array, accessing audioTrack could lead to undefined behavior.
Apply this diff to handle the case where no audio tracks are available:
const localStream = await navigator.mediaDevices.getUserMedia(constraints);
- const audioTrack = localStream.getAudioTracks()[0];
+ const audioTracks = localStream.getAudioTracks();
+ if (audioTracks.length === 0) {
+ throw new Error('No audio tracks available. Please ensure a microphone is connected.');
+ }
+ const audioTrack = audioTracks[0];
this.localAudioStream = new LocalMicrophoneStream(new MediaStream([audioTrack]));📝 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 localStream = await navigator.mediaDevices.getUserMedia(constraints); | |
| const audioTrack = localStream.getAudioTracks()[0]; | |
| this.localAudioStream = new LocalMicrophoneStream(new MediaStream([audioTrack])); | |
| this.webCallingService.answerCall(this.localAudioStream, this.data.interactionId); | |
| const localStream = await navigator.mediaDevices.getUserMedia(constraints); | |
| const audioTracks = localStream.getAudioTracks(); | |
| if (audioTracks.length === 0) { | |
| throw new Error('No audio tracks available. Please ensure a microphone is connected.'); | |
| } | |
| const audioTrack = audioTracks[0]; | |
| this.localAudioStream = new LocalMicrophoneStream(new MediaStream([audioTrack])); | |
| this.webCallingService.answerCall(this.localAudioStream, this.data.interactionId); |
packages/@webex/plugin-cc/README.md
Outdated
| ### Kitchen Sink App | ||
| To test Contact Center SDK API, use this Kitchen Sink app: https://webex.github.io/webex-js-sdk/samples/contact-center/ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix heading hierarchy and URL formatting.
Two issues need to be addressed:
- The "Kitchen Sink App" section uses h3 (###) when it should use h2 (##) to maintain heading hierarchy
- The bare URL should be properly formatted as a markdown link
Apply this diff to fix the issues:
-### Kitchen Sink App
-To test Contact Center SDK API, use this Kitchen Sink app: https://webex.github.io/webex-js-sdk/samples/contact-center/
+## Kitchen Sink App
+To test Contact Center SDK API, use this Kitchen Sink app: [Contact Center Kitchen Sink](https://webex.github.io/webex-js-sdk/samples/contact-center/)📝 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.
| ### Kitchen Sink App | |
| To test Contact Center SDK API, use this Kitchen Sink app: https://webex.github.io/webex-js-sdk/samples/contact-center/ | |
| ## Kitchen Sink App | |
| To test Contact Center SDK API, use this Kitchen Sink app: [Contact Center Kitchen Sink](https://webex.github.io/webex-js-sdk/samples/contact-center/) |
🧰 Tools
🪛 Markdownlint (0.37.0)
70-70: Expected: h2; Actual: h3
Heading levels should only increment by one level at a time
(MD001, heading-increment)
71-71: null
Bare URL used
(MD034, no-bare-urls)
| "dependencies": { | ||
| "@types/platform": "1.3.4", | ||
| "@webex/calling": "workspace:*", | ||
| "@webex/internal-plugin-mercury": "workspace:*", | ||
| "@webex/webex-core": "workspace:*", | ||
| "buffer": "6.0.3", | ||
| "jest-html-reporters": "3.0.11" | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Review production dependencies.
jest-html-reporters should be moved to devDependencies as it's typically only needed during testing.
| "engines": { | ||
| "node": ">=20.x" | ||
| }, |
There was a problem hiding this comment.
💡 Codebase verification
Align Node.js version requirement with other @webex packages
The Node.js version requirement of >=20.x makes this package incompatible with projects using Node.js 16-19, while all other @webex runtime packages support Node.js >=16. Consider changing it to >=16 to maintain consistency with the rest of the ecosystem.
🔗 Analysis chain
Verify Node.js version requirement.
The package requires Node.js >=20.x, but the main webex package supports Node.js >=16. This might cause compatibility issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check Node.js version requirements across all package.json files
fd package.json --exec jq -r '. | select(.engines.node != null) | {name: .name, node: .engines.node}'
Length of output: 5333
| /* Container - Docs / Streams Fixed */ | ||
| .container { | ||
| display: flex; | ||
| width: 100%; | ||
| gap: 1rem; | ||
| flex-direction: row; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove duplicate container styles.
The container styles are duplicated. The first definition appears at lines 41-47.
Remove the duplicate styles:
-/* Container - Docs / Streams Fixed */
-.container {
- display: flex;
- width: 100%;
- gap: 1rem;
- flex-direction: row;
-}| <button onclick="expandAll()" class="global-toggle-btn" title="Expand all"> | ||
| <div style="display: flex; align-items: center; gap: 0.5rem;"> | ||
| <div style="display: flex; flex-direction: column;"> | ||
| <i class="g-arrow fa fa-angle-up" aria-hidden="true"></i> | ||
| <i class="g-arrow fa fa-angle-down" aria-hidden="true" style="margin-top: -6px;"></i> | ||
| </div> | ||
| <div>EXPAND ALL</div> | ||
| </div> | ||
| </button> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance accessibility with ARIA labels.
The expand/collapse buttons lack proper ARIA attributes for accessibility.
Add ARIA attributes:
- <button onclick="expandAll()" class="global-toggle-btn" title="Expand all">
+ <button onclick="expandAll()" class="global-toggle-btn" title="Expand all" aria-expanded="false" aria-controls="all-sections">Also applies to: 25-33
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
|
@coderabbitai configuration |
CodeRabbit configurationlanguage: en-US
tone_instructions: ''
early_access: false
enable_free_tier: true
reviews:
profile: chill
request_changes_workflow: false
high_level_summary: true
high_level_summary_placeholder: '@coderabbitai summary'
auto_title_placeholder: '@coderabbitai'
auto_title_instructions: ''
review_status: true
commit_status: true
fail_commit_status: false
collapse_walkthrough: false
changed_files_summary: true
sequence_diagrams: true
assess_linked_issues: true
related_issues: true
related_prs: true
suggested_labels: true
auto_apply_labels: false
suggested_reviewers: true
poem: true
labeling_instructions: []
path_filters: []
path_instructions: []
abort_on_close: true
auto_review:
enabled: true
auto_incremental_review: true
ignore_title_keywords: []
labels: []
drafts: false
base_branches: []
tools:
shellcheck:
enabled: true
ruff:
enabled: true
markdownlint:
enabled: true
github-checks:
enabled: true
timeout_ms: 90000
languagetool:
enabled: true
enabled_only: false
level: default
biome:
enabled: true
hadolint:
enabled: true
swiftlint:
enabled: true
phpstan:
enabled: true
level: default
golangci-lint:
enabled: true
yamllint:
enabled: true
gitleaks:
enabled: true
checkov:
enabled: true
detekt:
enabled: true
eslint:
enabled: true
rubocop:
enabled: true
buf:
enabled: true
regal:
enabled: true
actionlint:
enabled: true
pmd:
enabled: true
cppcheck:
enabled: true
semgrep:
enabled: true
circleci:
enabled: true
chat:
auto_reply: true
knowledge_base:
opt_out: false
learnings:
scope: auto
issues:
scope: auto
jira:
project_keys: []
linear:
team_keys: []
pull_requests:
scope: auto
|
rsarika
force-pushed
the
SPARK-586376-Add-calling-RTMS-to-u2c
branch
from
206b33e to
241b5c2
Compare
COMPLETES #https://jira-eng-gpk2.cisco.com/jira/browse/SPARK-586376
This pull request addresses
The issue of hardcode of RTMS URL
by making the following changes
Getting RTMS URL from U2C catalogue.
Change Type
The following scenarios were tested
Check u2c catalogue response and see RTMS url is present or not.
Check register line and see RTMS URL in network logs.
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
Based on the comprehensive summary, here are the concise release notes:
New Features
Documentation
Performance
Infrastructure