feat(CC): add oauth login to samples #4022
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 changes introduced in this pull request enhance the contact center application by implementing OAuth authentication alongside the existing access token method. The modifications include the addition of new UI elements and functions for managing the OAuth process, as well as a new HTML file that serves as a sample interface for the Webex JavaScript SDK. The existing functionality for access tokens remains intact, ensuring a seamless integration of the new authentication method. Changes
Possibly related PRs
Suggested reviewers
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 review |
✅ Actions performedReview triggered.
|
![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 (31)
packages/@webex/plugin-cc/src/services/index.ts (1)
31-35: 🛠️ Refactor suggestion
Use class name instead of 'this' in static context for clarity
In the
getInstancestatic method, usingthiscan be confusing because it refers to the class itself. It's clearer and more maintainable to use the class name directly.Apply this diff to replace
thiswithServices: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; }📝 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 (!Services.instance) { Services.instance = new Services(options); } return Services.instance;🧰 Tools
🪛 Biome (1.9.4)
[error] 31-31: 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] 32-32: 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)
packages/@webex/plugin-cc/src/services/WebCallingService.ts (1)
48-63:
⚠️ Potential issueEnsure timeout is cleared on both resolve and reject
In the
registerWebCallingLinemethod, if the registration times out, thetimeoutis cleared in the resolve path but not in the reject path. This could lead to memory leaks.Apply this diff to clear the timeout in both cases:
return new Promise<void>((resolve, reject) => { const timeout = setTimeout(() => { + clearTimeout(timeout); reject(new Error('WebCallingService Registration timed out')); }, TIMEOUT_DURATION); this.line.on(LINE_EVENTS.REGISTERED, (deviceInfo: ILine) => { clearTimeout(timeout); LoggerProxy.log( `WxCC-SDK: Desktop registered successfully, mobiusDeviceId: ${deviceInfo.mobiusDeviceId}`, {module: WEB_CALLING_SERVICE_FILE, method: this.registerWebCallingLine.name} ); resolve(); }); this.line.register(); });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.return new Promise<void>((resolve, reject) => { const timeout = setTimeout(() => { clearTimeout(timeout); reject(new Error('WebCallingService Registration timed out')); }, TIMEOUT_DURATION); this.line.on(LINE_EVENTS.REGISTERED, (deviceInfo: ILine) => { clearTimeout(timeout); LoggerProxy.log( `WxCC-SDK: Desktop registered successfully, mobiusDeviceId: ${deviceInfo.mobiusDeviceId}`, {module: WEB_CALLING_SERVICE_FILE, method: this.registerWebCallingLine.name} ); resolve(); }); this.line.register(); }); }packages/@webex/plugin-cc/src/services/core/Err.ts (1)
63-77: 🛠️ Refactor suggestion
Pass the error message to the superclass constructor
In the
Messageclass constructor, passing the error message tosuper()ensures that theErrorbase class properly initializes the message and stack trace. This enhances error handling and debugging.Apply this diff to pass the message to the
Errorconstructor:constructor(id: IdsMessage, value?: string | Error) { - super(); + super(typeof value === 'string' ? value : value?.message); this.id = id; - this.stack = new Error().stack!; if (typeof value === 'string') { - this.message = value; } else if (value instanceof Error) { - this.message = value.message; this.name = value.name; } else { - this.message = ''; } }Repeat similar changes in the
Detailsclass constructor.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.constructor(id: IdsMessage, value?: string | Error) { super(typeof value === 'string' ? value : value?.message); this.id = id; if (typeof value === 'string') { } else if (value instanceof Error) { this.name = value.name; } else { } }packages/@webex/plugin-cc/src/services/core/WebSocket/connection-service.ts (2)
137-140:
⚠️ Potential issueHandle potential errors in async 'setInterval' callback
The
handleSocketClosemethod can throw an error, but within thesetInterval, any rejected promises won't be caught, leading to unhandled promise rejections.Wrap the async function in a try-catch block:
this.reconnectInterval = setInterval(async () => { + try { await this.handleSocketClose(); + } catch (error) { + LoggerProxy.error(`Error during socket reconnection attempt: ${error}`, { + module: CONNECTION_SERVICE_FILE, + method: this.onSocketClose.name, + }); + } }, CONNECTIVITY_CHECK_INTERVAL);📝 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.reconnectInterval = setInterval(async () => { try { await this.handleSocketClose(); } catch (error) { LoggerProxy.error(`Error during socket reconnection attempt: ${error}`, { module: CONNECTION_SERVICE_FILE, method: this.onSocketClose.name, }); } }, CONNECTIVITY_CHECK_INTERVAL); };
113-116:
⚠️ Potential issueAvoid potential 'false' timeout value in 'setTimeout'
In the
setTimeoutcall, using an expression that might evaluate tofalsecan result in an unintended timeout value of0. This could causehandleRestoreFailedto be called immediately.Ensure that the timeout duration is a valid number. Apply this diff:
this.restoreTimer = setTimeout( this.handleRestoreFailed, - this.connectionProp && this.connectionProp.lostConnectionRecoveryTimeout + this.connectionProp?.lostConnectionRecoveryTimeout ?? DEFAULT_TIMEOUT );Replace
DEFAULT_TIMEOUTwith an appropriate default value.Committable suggestion skipped: line range outside the PR's diff.
🧰 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/core/WebSocket/WebSocketManager.ts (1)
90-100:
⚠️ Potential issueEnsure the Promise in
connectmethod resolves or rejects appropriatelyThe
connectmethod returns a Promise that may remain pending indefinitely because it only rejects onthis.websocket.onerrorbut never resolves on successful connection. To prevent the Promise from hanging, consider resolving it in theonopenhandler.Apply this diff to resolve the Promise upon successful WebSocket connection:
return new Promise((resolve, reject) => { this.websocket.onopen = () => { this.isSocketClosed = false; this.shouldReconnect = true; this.websocket.send(JSON.stringify({keepalive: 'true'})); // ... existing code ... + resolve(); }; this.websocket.onerror = (event: any) => { // ... existing error handling ...Committable suggestion skipped: line range outside the PR's diff.
docs/samples/contact-center/app.js (1)
115-116:
⚠️ Potential issueRemove hardcoded client credentials to prevent security vulnerabilities.
Hardcoding
client_idandclient_secretin the source code exposes sensitive information and poses significant security risks. It's recommended to use environment variables or a secure configuration service to manage these credentials securely.Apply this diff to remove the hardcoded credentials:
credentials: { - client_id: 'C70599433db154842e919ad9e18273d835945ff198251c82204b236b157b3a213', - client_secret: '575ba9f5034f8a28dfef2770870c50bfc6e0b2b749f14e6a14845a1a47622f87', + client_id: process.env.WEBEX_CLIENT_ID, + client_secret: process.env.WEBEX_CLIENT_SECRET, redirect_uri: redirectUri, scope: requestedScopes, }📝 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.client_id: process.env.WEBEX_CLIENT_ID, client_secret: process.env.WEBEX_CLIENT_SECRET,🧰 Tools
🪛 Gitleaks (8.21.2)
116-116: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
packages/@webex/plugin-cc/test/unit/spec/services/core/aqm-reqs.ts (1)
68-69: 🛠️ Refactor suggestion
Avoid using
Functionas a type; explicitly define the function signature.Using
Functionas a type is discouraged because it accepts any function-like value, which can lead to bugs. Define the function signature explicitly for better type safety.Apply this diff to specify the function type:
- const eventListeners: { [key: string]: Function[] } = {}; + const eventListeners: { [key: string]: Array<(...args: any[]) => void> } = {};Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 68-68: 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] 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)
packages/@webex/plugin-cc/test/unit/spec/cc.ts (2)
585-663:
⚠️ Potential issueAdd error handling for potential null
agentConfigIn the
getBuddyAgentstests,webex.cc.agentConfigis accessed without checking if it's defined. Ensure thatagentConfigis properly initialized before accessing its properties to prevent runtime errors.
95-102:
⚠️ Potential issueEnsure proper handling of asynchronous code in tests
In the test
should initialize services and logger proxy on READY event, the assertions are inside an event handler callback. Jest may not wait for the asynchronous code to complete before finishing the test. To ensure the test waits for the event, consider using thedonecallback or returning a Promise.Apply this diff to fix the test using the
donecallback:-it('should initialize services and logger proxy on READY event', () => { +it('should initialize services and logger proxy on READY event', (done) => { webex.once('READY', () => { expect(Services.getInstance).toHaveBeenCalled(); expect(LoggerProxy.initialize).toHaveBeenCalledWith(webex.logger); + done(); }); webex.emit('READY'); });Alternatively, you can return a Promise:
-it('should initialize services and logger proxy on READY event', () => { +it('should initialize services and logger proxy on READY event', async () => { + await new Promise((resolve) => { webex.once('READY', () => { expect(Services.getInstance).toHaveBeenCalled(); expect(LoggerProxy.initialize).toHaveBeenCalledWith(webex.logger); + resolve(); }); + }); webex.emit('READY'); });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.it('should initialize services and logger proxy on READY event', (done) => { webex.once('READY', () => { expect(Services.getInstance).toHaveBeenCalled(); expect(LoggerProxy.initialize).toHaveBeenCalledWith(webex.logger); done(); }); webex.emit('READY'); });it('should initialize services and logger proxy on READY event', async () => { await new Promise((resolve) => { webex.once('READY', () => { expect(Services.getInstance).toHaveBeenCalled(); expect(LoggerProxy.initialize).toHaveBeenCalledWith(webex.logger); resolve(); }); }); webex.emit('READY'); });packages/@webex/plugin-cc/src/config.ts (1)
20-21:
⚠️ Potential issueReplace hardcoded domain with environment-based configuration
The TODO comment indicates this domain should be dynamic, but it's currently hardcoded to a production URL. This could cause issues when switching between environments.
Consider implementing environment-based configuration:
- domain: 'rtw.prod-us1.rtmsprod.net', + domain: process.env.CC_DOMAIN || 'rtw.prod-us1.rtmsprod.net',📝 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.// TODO: This should be dynamic based on the environment domain: process.env.CC_DOMAIN || 'rtw.prod-us1.rtmsprod.net',packages/@webex/plugin-cc/src/services/core/HttpRequest.ts (1)
20-34: 🛠️ Refactor suggestion
Add error handling and request validation
The request method should include:
- Request timeout
- Error handling with proper types
- Input validation for required fields
public async request(options: { service: string; resource: string; method: HTTP_METHODS; body?: RequestBody; }): Promise<IHttpResponse> { const {service, resource, method, body} = options; + if (!service || !resource || !method) { + throw new Error('Missing required request parameters'); + } return this.webex.request({ service, resource, method, body, + timeout: 30000, // 30 seconds timeout }).catch((error) => { + throw createErrDetailsObject(error); }); }Committable suggestion skipped: line range outside the PR's diff.
packages/@webex/plugin-cc/src/services/core/Utils.ts (1)
13-25: 🛠️ Refactor suggestion
Enhance error details logging and typing
The error handling could be improved with better typing and structured logging:
-export const getErrorDetails = (error: any, methodName: string, moduleName: string) => { +export const getErrorDetails = ( + error: Error & { details?: Failure }, + methodName: string, + moduleName: string +) => { const failure = error.details as Failure; const reason = failure?.data?.reason ?? `Error while performing ${methodName}`; + + const errorContext = { + module: moduleName, + method: methodName, + trackingId: failure?.trackingId, + error: error.message, + }; + LoggerProxy.error(`${methodName} failed with trackingId: ${failure?.trackingId}`, { - module: moduleName, - method: methodName, + ...errorContext, }); return { error: new Error(reason ?? `Error while performing ${methodName}`), reason, + context: errorContext, }; };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.export const getErrorDetails = ( error: Error & { details?: Failure }, methodName: string, moduleName: string ) => { const failure = error.details as Failure; const reason = failure?.data?.reason ?? `Error while performing ${methodName}`; const errorContext = { module: moduleName, method: methodName, trackingId: failure?.trackingId, error: error.message, }; LoggerProxy.error(`${methodName} failed with trackingId: ${failure?.trackingId}`, { ...errorContext, }); return { error: new Error(reason ?? `Error while performing ${methodName}`), reason, context: errorContext, }; };packages/@webex/plugin-cc/jest.config.js (1)
5-5: 🛠️ Refactor suggestion
Consider specifying transformIgnorePatterns
An empty
transformIgnorePatternsarray means Jest will transform all files, including those innode_modules. This could significantly slow down your tests.- transformIgnorePatterns: [], + transformIgnorePatterns: ['/node_modules/(?!(@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.transformIgnorePatterns: ['/node_modules/(?!(@webex)/)'],packages/@webex/plugin-cc/src/services/core/types.ts (2)
14-14: 🛠️ Refactor suggestion
Avoid using 'any' type
The
dataproperty in theBindinterface usesany. Consider using a generic type parameter for better type safety.-interface Bind { +interface Bind<T = unknown> { type: BindType; - data?: any; + data?: T; }📝 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 Bind<T = unknown> { type: BindType; data?: T; }
48-48: 🛠️ Refactor suggestion
Replace void in union type
As flagged by static analysis, using
voidin a union type can be confusing. Consider usingundefinedinstead.-export type CbRes<TRes> = (res: any) => void | TRes; +export type CbRes<TRes> = (res: unknown) => undefined | TRes;Also, replace
anywithunknownfor better type safety.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.export type CbRes<TRes> = (res: unknown) => undefined | TRes;🧰 Tools
🪛 Biome (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/README.md (1)
16-18:
⚠️ Potential issueAdd documentation links
The TODO comment indicates missing documentation links. These should be completed before merging.
-TODO: Add the documentation links once ready -- [Introduction to the Webex Web Calling SDK]() -- [Quickstart guide](). +- [Introduction to the Webex Web Calling SDK](link-to-introduction) +- [Quickstart guide](link-to-quickstart)Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Markdownlint (0.35.0)
17-17: null
No empty links(MD042, no-empty-links)
18-18: null
No empty links(MD042, no-empty-links)
packages/@webex/plugin-cc/src/logger-proxy.ts (1)
4-48: 🛠️ Refactor suggestion
Consider refactoring to a module with standalone functions
The current implementation uses a static-only class which goes against TypeScript best practices. Consider refactoring to a module with standalone functions for better maintainability and testing.
-export default class LoggerProxy { - public static logger: Logger; +let logger: Logger; - public static initialize(logger: Logger): void { - LoggerProxy.logger = logger; +export function initialize(loggerInstance: Logger): void { + logger = loggerInstance; } - public static log(message: string, context: LogContext = {}): void { - if (LoggerProxy.logger) { - LoggerProxy.logger.log(LoggerProxy.format(LOGGING_LEVEL.log, message, context)); +export function log(message: string, context: LogContext = {}): void { + if (logger) { + logger.log(format(LOGGING_LEVEL.log, message, context)); } } // ... similar changes for other methodsCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 4-48: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
packages/@webex/plugin-cc/src/services/config/constants.ts (1)
16-46:
⚠️ Potential issueAdd URL parameter encoding for security
The endpoint mapping functions don't encode URL parameters, which could lead to URL injection vulnerabilities.
listTeams: ( orgId: string, page: number, pageSize: number, filter: string[], attributes: string[] ) => - `organization/${orgId}/v2/team?page=${page}&pageSize=${pageSize}${ - filter && filter.length > 0 ? `&filter=id=in=${filter}` : '' - }&attributes=${attributes}`, + `organization/${encodeURIComponent(orgId)}/v2/team?page=${encodeURIComponent(page)}&pageSize=${encodeURIComponent(pageSize)}${ + filter && filter.length > 0 ? `&filter=id=in=${encodeURIComponent(filter.join(','))}` : '' + }&attributes=${encodeURIComponent(attributes.join(','))}`,📝 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 endPointMap = { userByCI: (orgId: string, agentId: string) => `organization/${orgId}/user/by-ci-user-id/${agentId}`, desktopProfile: (orgId: string, desktopProfileId: string) => `organization/${orgId}/agent-profile/${desktopProfileId}`, listTeams: ( orgId: string, page: number, pageSize: number, filter: string[], attributes: string[] ) => `organization/${encodeURIComponent(orgId)}/v2/team?page=${encodeURIComponent(page)}&pageSize=${encodeURIComponent(pageSize)}${ filter && filter.length > 0 ? `&filter=id=in=${encodeURIComponent(filter.join(','))}` : '' }&attributes=${encodeURIComponent(attributes.join(','))}`, listAuxCodes: ( orgId: string, page: number, pageSize: number, filter: string[], attributes: string[] ) => `organization/${orgId}/v2/auxiliary-code?page=${page}&pageSize=${pageSize}${ filter && filter.length > 0 ? `&filter=id=in=${filter}` : '' }&attributes=${attributes}`, orgInfo: (orgId: string) => `organization/${orgId}`, orgSettings: (orgId: string) => `organization/${orgId}/v2/organization-setting?agentView=true`, tenantData: (orgId: string) => `organization/${orgId}/v2/tenant-configuration?agentView=true`, urlMapping: (orgId: string) => `organization/${orgId}/v2/org-url-mapping?sort=name,ASC`, dialPlan: (orgId: string) => `organization/${orgId}/dial-plan?agentView=true`, };packages/@webex/plugin-cc/test/unit/spec/services/agent/index.ts (1)
43-43: 🛠️ Refactor suggestion
Avoid using 'any' type in TypeScript tests
The test data uses
anytype which bypasses TypeScript's type checking benefits. Consider creating proper interface types for the test data.- const req = await agent.stationLogin({data: {} as any}); + interface StationLoginData { + // Add required properties based on actual implementation + stationId?: string; + extension?: string; + } + const req = await agent.stationLogin({data: {} as StationLoginData});Also applies to: 50-50, 56-56
packages/webex/src/webex.js (1)
30-30: 💡 Codebase verification
Warning: Duplicate plugin registration detected
The CC plugin is being required in both
webex.jsandcontact-center.js, which could lead to initialization issues. The plugin should only be registered once.
- Remove the duplicate
require('@webex/plugin-cc')from eitherwebex.jsorcontact-center.js- Recommend keeping it in
webex.jswhere other plugins are registered for consistency🔗 Analysis chain
LGTM: CC plugin integration follows established patterns
The Contact Center plugin is properly integrated following the existing plugin registration pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify CC plugin is properly exported and documented rg -g '!node_modules' -g '*.{js,ts,md}' '@webex/plugin-cc'Length of output: 478
Script:
#!/bin/bash # Check if plugin is properly registered and exported ast-grep --pattern 'export default $_ = $_;' packages/@webex/plugin-cc/src/*.js # Check if plugin is properly documented cat packages/@webex/plugin-cc/README.md # Check for duplicate requires rg -l "require\('@webex/plugin-cc'\)" packages/webex/src/Length of output: 1844
Script:
#!/bin/bash # Check content of contact-center.js to understand duplicate require cat packages/webex/src/contact-center.js # Check if plugin is properly registered in index files rg -g '*.js' -l "ContactCenter" packages/@webex/plugin-cc/Length of output: 550
packages/@webex/plugin-cc/src/services/core/WebSocket/keepalive.worker.js (2)
31-38:
⚠️ Potential issueConsider making timeout duration configurable
The comment mentions a 16s timeout, but the actual timeout uses
closeSocketTimeoutwhich defaults to 5000ms (5s). This discrepancy between the comment and implementation could lead to confusion.- // Sets a timeout of 16s, checks if socket didn't close then it closes forcefully + // Sets a timeout using closeSocketTimeout (default: 5s), checks if socket didn't close then it closes forcefully📝 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.// Sets a timeout using closeSocketTimeout (default: 5s), checks if socket didn't close then it closes forcefully timeOutId = setTimeout(() => { if (!isSocketClosed) { initiateWebSocketClosure = true; postMessage({ type: "closeSocket" }); } }, closeSocketTimeout); }
51-74: 🛠️ Refactor suggestion
Enhance error handling in message event listener
The message event listener should include error handling for malformed messages and invalid interval durations.
addEventListener("message", (event) => { + try { if (event.data?.type === "start") { + if (intervalDuration < 1000) { + throw new Error("Invalid interval duration: must be at least 1000ms"); + } intervalDuration = event.data?.intervalDuration || 4000; closeSocketTimeout = event.data?.closeSocketTimeout || 5000; 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(); } + } catch (error) { + console.error("Error in keepalive worker:", error); + postMessage({ type: "error", error: error.message }); + } });Committable suggestion skipped: line range outside the PR's diff.
packages/@webex/plugin-cc/src/services/agent/types.ts (2)
50-51: 🛠️ Refactor suggestion
Consolidate agent status/subStatus types
The
statusandsubStatustypes are inconsistently defined across different message types. Consider creating shared enums or type unions for these values.+export type AgentStatus = 'LoggedIn' | 'LoggedOut' | string; +export type AgentSubStatus = 'Available' | 'Idle' | string; export type StateChangeSuccess = Msg<{ // ... - status: string; - subStatus: 'Available' | 'Idle'; + status: AgentStatus; + subStatus: AgentSubStatus; // ... }>; export type StationLoginSuccess = Msg<{ // ... - status: string; - subStatus: 'Available' | 'Idle'; + status: AgentStatus; + subStatus: AgentSubStatus; // ... }>;Also applies to: 70-71
85-85: 🛠️ Refactor suggestion
Define exhaustive agent states
The
AgentStatetype uses a string fallback which could lead to type safety issues. Consider defining all possible states explicitly.-export type AgentState = 'Available' | 'Idle' | 'RONA' | string; +export type AgentState = + | 'Available' + | 'Idle' + | 'RONA' + | 'Busy' + | 'OnBreak' + | 'Training' + | 'Meeting' + | 'Reserved';Committable suggestion skipped: line range outside the PR's diff.
packages/@webex/plugin-cc/test/unit/spec/services/WebCallingService.ts (1)
103-145: 🛠️ Refactor suggestion
Enhance incoming call test coverage
The incoming call test could be more comprehensive:
- Missing test for call rejection scenarios
- No verification of call properties
- Event listener cleanup not handled
Add the following test cases:
it('should handle call rejection', async () => { // Setup line = callingClient.getLines().line1 as ILine; const callObj = { on: jest.fn(), reject: jest.fn(), } as unknown as ICall; // Test await webRTCCalling.registerWebCallingLine(); await webRTCCalling.rejectCall(callObj); // Assert expect(callObj.reject).toHaveBeenCalled(); }); afterEach(() => { // Cleanup event listeners window.removeEventListener('line:incoming_call', eventListener); });docs/samples/contact-center/index.html (3)
14-34: 🛠️ Refactor suggestion
Improve accessibility for expand/collapse buttons
The expand/collapse functionality needs accessibility improvements.
- <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: 37-42
61-62:
⚠️ Potential issueEnhance security for access token input
The access token input should be treated as sensitive information.
- <input id="access-token" name="accessToken" placeholder="Access Token" value="" type="text"> + <input id="access-token" name="accessToken" placeholder="Access Token" value="" type="password" autocomplete="off">📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.<input id="access-token" name="accessToken" placeholder="Access Token" value="" type="password" autocomplete="off"> <button id="access-token-save" class="btn-code" type="submit">webex.init()</button>
145-147: 💡 Codebase verification
Missing required dependency: contact-center.min.js
The script loading sequence in the HTML file has a critical issue:
webex.min.jsexists and is correctly referencedcontact-center.min.jsis missing from the expected pathapp.jsexists and depends on both scripts, as evidenced by the initialization codeThe missing
contact-center.min.jswill cause runtime errors asapp.jstries to accesswebex.cc.register().🔗 Analysis chain
Verify script dependencies and loading order
The script loading order is critical for proper initialization.
Let's check the dependencies:
Let me check the content of app.js to understand the initialization sequence and dependencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the required scripts exist ls docs/samples/webex.min.js docs/samples/contact-center.min.js docs/samples/contact-center/app.js 2>/dev/null || echo "Missing required scripts" # Verify script contents rg -l "initWebex|initContactCenter" docs/samples/contact-center/Length of output: 322
Script:
#!/bin/bash # Check the content of app.js to understand initialization rg -A 5 "initWebex|initContactCenter" docs/samples/contact-center/app.js # Check if contact-center.min.js is in a different location fd -g "*contact-center.min.js"Length of output: 697
webpack.config.js (1)
50-56: 💡 Codebase verification
Entry point file exists but needs CommonJS to UMD export alignment
The contact-center entry point exists but uses
module.exports(CommonJS) while the webpack config expects a UMD module with a default export. This mismatch should be addressed:
packages/webex/src/contact-center.js: Changemodule.exports = Webextoexport default Webexto align with UMD library configuration🔗 Analysis chain
Verify contact-center entry point configuration
The configuration looks correct but needs verification of the entry point file.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the entry point file exists test -f packages/webex/src/contact-center.js || echo "Entry point file missing" # Check exports in the entry file ast-grep --pattern 'export default $_' packages/webex/src/contact-center.jsLength of output: 124
Script:
#!/bin/bash # Let's first check if the file exists in a different location or with a different extension fd "contact-center.js" --type f # Let's also check for any references to contact-center in the codebase rg -l "contact-center" --type jsLength of output: 119
Script:
#!/bin/bash # Let's examine the content of the contact-center.js file cat packages/webex/src/contact-center.js # Also check if there are any imports to this file rg -l "import.*contact-center" --type jsLength of output: 552
packages/@webex/internal-plugin-mercury/test/unit/spec/socket.js (1)
203-243:
⚠️ Potential issueFix syntax error in test cases for authorizationRequired option.
The test cases have syntax errors:
- Missing promise chain in both test cases
- Incorrect indentation
Apply this diff to fix the syntax:
it('accepts authorizationRequired option as false and skip authorize', () => { const s = new Socket(); const authorizeSpy = sinon.spy(socket, '_authorize'); - socket.open('ws://example.com', { + return socket.open('ws://example.com', { forceCloseDelay: mockoptions.forceCloseDelay, pingInterval: mockoptions.pingInterval, pongTimeout: mockoptions.pongTimeout, logger: console, token: 'mocktoken', trackingId: 'mocktrackingid', logLevelToken: 'mocklogleveltoken', authorizationRequired: false, }).then(() => { mockWebSocket.readyState = 1; mockWebSocket.emit('open'); assert.notCalled(authorizeSpy); assert.called(socket._ping); }); }); it('accepts authorizationRequired option as true and calles authorize', () => { const s = new Socket(); const authorizeSpy = sinon.spy(socket, '_authorize'); - socket.open('ws://example.com', { + return socket.open('ws://example.com', { forceCloseDelay: mockoptions.forceCloseDelay, pingInterval: mockoptions.pingInterval, pongTimeout: mockoptions.pongTimeout, logger: console, token: 'mocktoken', trackingId: 'mocktrackingid', logLevelToken: 'mocklogleveltoken', authorizationRequired: true, }).then(() => { mockWebSocket.readyState = 1; mockWebSocket.emit('open'); assert.called(authorizeSpy); assert.called(socket._ping); }); });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.it('accepts authorizationRequired option as false and skip authorize', () => { const s = new Socket(); const authorizeSpy = sinon.spy(socket, '_authorize'); return socket.open('ws://example.com', { forceCloseDelay: mockoptions.forceCloseDelay, pingInterval: mockoptions.pingInterval, pongTimeout: mockoptions.pongTimeout, logger: console, token: 'mocktoken', trackingId: 'mocktrackingid', logLevelToken: 'mocklogleveltoken', authorizationRequired: false, }).then(() => { mockWebSocket.readyState = 1; mockWebSocket.emit('open'); assert.notCalled(authorizeSpy); assert.called(socket._ping); }); }); it('accepts authorizationRequired option as true and calles authorize', () => { const s = new Socket(); const authorizeSpy = sinon.spy(socket, '_authorize'); return socket.open('ws://example.com', { forceCloseDelay: mockoptions.forceCloseDelay, pingInterval: mockoptions.pingInterval, pongTimeout: mockoptions.pongTimeout, logger: console, token: 'mocktoken', trackingId: 'mocktrackingid', logLevelToken: 'mocklogleveltoken', authorizationRequired: true, }).then(() => { mockWebSocket.readyState = 1; mockWebSocket.emit('open'); assert.called(authorizeSpy); assert.called(socket._ping); }); });
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
docs/samples/contact-center/app.js (2)
88-111: Consider consolidating scope declarationsWhile the scopes are well documented, the current implementation could be improved by:
- Moving scopes to a configuration object
- Combining related scopes into named groups
- Using a more efficient method to combine arrays
- const ccMandatoryScopes = [ - "cjp:config_read", - "cjp:config_write", - "cjp:config", - "cjp:user", - ]; - - const webRTCCallingScopes = [ - "spark:webrtc_calling", - "spark:calls_read", - "spark:calls_write", - "spark:xsi" - ]; - - const additionalScopes = [ - "spark:kms", - ]; - - const requestedScopes = Array.from( - new Set( - ccMandatoryScopes - .concat(webRTCCallingScopes) - .concat(additionalScopes)) - ).join(' '); + const OAUTH_SCOPES = { + contactCenter: [ + "cjp:config_read", + "cjp:config_write", + "cjp:config", + "cjp:user", + ], + webRTCCalling: [ + "spark:webrtc_calling", + "spark:calls_read", + "spark:calls_write", + "spark:xsi" + ], + additional: ["spark:kms"] + }; + + const requestedScopes = [...new Set([ + ...OAUTH_SCOPES.contactCenter, + ...OAUTH_SCOPES.webRTCCalling, + ...OAUTH_SCOPES.additional + ])].join(' ');
124-124: Consider using sessionStorage for OAuth stateUsing localStorage for OAuth state persistence could lead to state inconsistencies across tabs. Consider using sessionStorage instead, which is already used for access tokens in this application.
- localStorage.setItem('OAuth', true); + sessionStorage.setItem('OAuth', true);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
docs/samples/contact-center/app.js(5 hunks)
🧰 Additional context used
🪛 Gitleaks (8.21.2)
docs/samples/contact-center/app.js
117-117: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (3)
docs/samples/contact-center/app.js (3)
27-28: LGTM: DOM element references properly declared
The new OAuth-related DOM element references follow the existing patterns and best practices.
59-63: LGTM: OAuth type handling properly implemented
The OAuth case is properly integrated into the existing authentication type switch statement, maintaining consistency with the existing pattern.
222-224: LGTM: Proper handling of idle codes list
The condition properly handles the agent status button enablement based on the idle codes list availability.
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| - | - | Generic High Entropy Secret | 19a528e | docs/samples/contact-center/app.js | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
COMPLETES #SPARK-583528
This pull request addresses
Need to do all testing with an integration that has minimal scopes instead of copying from the dev portal or agent desktop
by making the following changes
Added oauth login to cc samples
Vidcast: https://app.vidcast.io/share/86fafaad-afd3-4218-b885-78aa90ca34a3
Change Type
The following scenarios were tested
Scopes used:
The integration was created using the credentials listed in https://confluence-eng-gpk2.cisco.com/conf/x/C86lIw
More details about the discovery are listed in the same confluence
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