fix(cc-store): receive webex on init, add store types #341
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 in this pull request introduce a new state management mechanism in a React application and enhance user interactions in a web component. The Changes
Possibly related PRs
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 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.
Actionable comments posted: 12
🧹 Outside diff range and nitpick comments (9)
packages/contact-center/store/src/store.ts (1)
24-24: Remove unnecessaryconsole.tracestatementThe
console.trace(this.cc.register);statement appears to be left over from debugging and may not be necessary in production code. It's generally best to remove such statements to keep the console output clean.Apply this diff to remove the unnecessary statement:
- console.trace(this.cc.register);packages/contact-center/store/src/store.types.ts (1)
14-21: Add JSDoc comments to document the interface.Consider adding JSDoc comments to document:
- The purpose of each property
- Parameters and return values for methods
- Expected behavior of async operations
Example documentation:
interface IStore { + /** List of teams available to the agent */ teams: Team[]; + /** Available login options for the agent */ loginOptions: string[]; + /** Contact center instance */ cc: IContactCenter; + /** + * Registers the contact center with the provided webex instance + * @param webex - The webex instance containing contact center capability + * @returns Promise resolving to the agent's profile + */ registerCC(webex: WithWebex['webex']): Promise<Profile>; + /** + * Initializes the store with either a webex instance or config with token + * @param params - Initialization parameters + */ init(params: InitParams): Promise<void>; }packages/contact-center/station-login/src/station-login/index.tsx (1)
22-27: LGTM! Consider adding more specific function signatures.The addition of prop types for
onLoginandonLogoutimproves type safety. However, consider being more specific about the function signatures to improve developer experience.const WebStationLogin = r2wc(StationLogin, { props: { - onLogin: "function", - onLogout: "function" + onLogin: { + type: "function", + value: (token: string) => void + }, + onLogout: { + type: "function", + value: () => void + } } });packages/contact-center/store/package.json (1)
Line range hint
47-50: Update Jest configuration to enforce test coverageThe PR objectives emphasize "ensuring comprehensive test coverage", but the Jest configuration allows passing with no tests. Consider removing
passWithNoTestsand adding coverage thresholds.Apply this diff to enforce test coverage:
"jest": { "testEnvironment": "jsdom", - "//": "We can remove this when we have tests", - "passWithNoTests": true + "coverageThreshold": { + "global": { + "statements": 80, + "branches": 80, + "functions": 80, + "lines": 80 + } + } },docs/react-samples/src/App.tsx (2)
Line range hint
18-24: Enhance login/logout handlers with proper error handling and state management.The current handlers only log to console. Consider:
- Adding error handling
- Updating relevant application state
- Implementing proper cleanup on logout
const onLogin = () => { - console.log('Agent login has been succesful'); + try { + console.log('Agent login has been successful'); + // Update relevant application state + } catch (error) { + console.error('Login failed:', error); + // Handle error appropriately + } } const onLogout = () => { - console.log('Agent logout has been succesful'); + try { + console.log('Agent logout has been successful'); + // Cleanup and reset relevant state + } catch (error) { + console.error('Logout failed:', error); + // Handle error appropriately + } }
43-51: Consider adding loading states for child components.The conditional rendering could be improved to handle loading states of child components.
-{/* write code to check if sdk is ready and load components */} { isSdkReady && ( <> + <ErrorBoundary fallback={<div>Something went wrong</div>}> <StationLogin onLogin={onLogin} onLogout={onLogout} /> <UserState /> + </ErrorBoundary> </> ) }docs/web-component-samples/index.html (1)
56-62: Replace console.log with proper logging mechanism.Using console.log in production code is not recommended. Consider implementing a proper logging system.
function loginSuccess(){ - console.log('Agent login has been succesful'); + // Use the logger from webexConfig + webexConfig.logger.log('Agent login has been successful'); } function logoutSuccess(){ - console.log('Agent logout has been succesful'); + // Use the logger from webexConfig + webexConfig.logger.log('Agent logout has been successful'); }Also, note the typo in "succesful" -> "successful".
packages/contact-center/store/tests/store.test.ts (2)
24-33: Consider using a reset method for store cleanup.Direct mutation of store properties in tests could be brittle. Consider adding a
reset()method to the store class for cleaner test setup.beforeEach(() => { - // Reset teams and loginOptions before each test since store is a singleton - store.teams = []; - store.loginOptions = []; + store.reset(); // Add this method to store class mockWebex = { cc: { register: jest.fn() } }; });
93-103: Improve error test specificity.The error test could be more specific about the expected error type and ensure the error is properly propagated.
it('should reject the promise if registerCC fails in init method', async () => { const initParams = { webexConfig: { anyConfig: true }, access_token: 'fake_token' }; - jest.spyOn(store, 'registerCC').mockRejectedValue(new Error('registerCC failed')); + const expectedError = new Error('registerCC failed'); + jest.spyOn(store, 'registerCC').mockRejectedValue(expectedError); - await expect(store.init(initParams)).rejects.toThrow('registerCC failed'); + await expect(store.init(initParams)).rejects.toEqual(expectedError); + // Verify error is instance of Error + await expect(store.init(initParams)).rejects.toBeInstanceOf(Error); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (10)
docs/react-samples/src/App.tsx(2 hunks)docs/web-component-samples/index.html(1 hunks)docs/web-component-samples/src/index.ts(1 hunks)packages/contact-center/station-login/src/station-login/index.tsx(1 hunks)packages/contact-center/store/package.json(1 hunks)packages/contact-center/store/src/index.ts(1 hunks)packages/contact-center/store/src/store.ts(2 hunks)packages/contact-center/store/src/store.types.ts(1 hunks)packages/contact-center/store/tests/store.test.ts(1 hunks)packages/contact-center/store/tests/store.ts(0 hunks)
💤 Files with no reviewable changes (1)
- packages/contact-center/store/tests/store.ts
✅ Files skipped from review due to trivial changes (1)
- docs/web-component-samples/src/index.ts
🧰 Additional context used
📓 Learnings (1)
packages/contact-center/station-login/src/station-login/index.tsx (1)
Learnt from: Kesari3008
PR: webex/widgets#239
File: packages/contact-center/station-login/src/station-login/station-login.presentational.tsx:0-0
Timestamp: 2024-11-19T13:45:03.435Z
Learning: In `packages/contact-center/station-login/src/station-login/station-login.presentational.tsx`, certain code sections are temporary or removable, and comments regarding these sections can be ignored in future reviews.
🔇 Additional comments (12)
packages/contact-center/store/src/store.ts (1)
33-36: 🛠️ Refactor suggestion
Ensure consistent error handling in init method
In the init method, when the 'webex' property exists in options, the returned promise might not reject properly if registerCC encounters an error, due to error propagation issues in registerCC.
After updating registerCC to properly propagate errors, ensure that the init method correctly handles these errors. No changes are needed here if registerCC is fixed, but verify that error scenarios are correctly managed.
packages/contact-center/store/src/index.ts (1)
2-2: LGTM! Clean implementation of type exports.
The barrel export pattern is well-implemented and aligns with TypeScript best practices. This change improves type safety while maintaining a clean public API.
Let's verify the exported types are properly defined:
✅ Verification successful
Types are properly defined and exported
The store.types.ts file contains well-structured type definitions that align with the PR's objective of enhancing Store initialization with webex object support. The types include:
- Essential interfaces (
IStore,WithWebex,WithWebexConfigAndToken) - Re-exports from
@webex/plugin-cc(IContactCenter,Profile,Team,AgentLogin) - New type
InitParamsfor flexible initialization
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the store.types exports and their usage
# Check if store.types exists and contains type definitions
ast-grep --pattern 'export (type|interface) $_'
# Look for any usage of these types in the codebase
rg -t typescript "import.*from ['\"].*contact-center/store['\"]" -A 2
Length of output: 160
Script:
#!/bin/bash
# Let's try different approaches to verify the types
# 1. First, let's check if store.types.ts exists and see its content
fd "store.types.ts" --type f --exec cat {}
# 2. Look for imports using ripgrep without file type restriction
rg "import.*from ['\"].*contact-center/store['\"]"
# 3. Check for type exports using a more general pattern
ast-grep --pattern 'export $_'
Length of output: 787
packages/contact-center/store/src/store.types.ts (4)
1-1: LGTM! Imports are clean and necessary.
12-12: LGTM! Union type provides good flexibility for initialization.
The union type correctly allows initialization with either a webex instance or config with token.
23-31: LGTM! Exports are comprehensive and well-organized.
3-5: LGTM! Interface structure matches the webex SDK pattern.
The nested structure correctly represents the webex object's shape.
Let's verify the webex object structure in the codebase:
✅ Verification successful
LGTM! Interface structure and usage are correctly implemented.
The verification confirms:
- The
WithWebexinterface correctly defines the webex object structure withcc: IContactCenter - The Store class properly initializes and uses the webex.cc property
- The implementation matches the SDK pattern with proper registration flow through
webex.cc.register()
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the webex object structure usage
# Look for patterns where webex.cc is accessed to confirm the interface matches usage
rg -t typescript -t javascript "webex\.cc\." -A 2
Length of output: 86
Script:
#!/bin/bash
# Search for webex.cc pattern without file type restrictions
rg "webex\.cc\." -A 2
# Also search for IContactCenter interface usage
rg "IContactCenter" -A 2
# Look for any webex object access patterns
rg "webex\." -A 2
Length of output: 135258
Script:
#!/bin/bash
# Search for webex object initialization and usage
ast-grep --pattern 'webex: { cc: $_ }'
# Search for IContactCenter interface usage and implementation
rg -A 5 "interface IContactCenter"
# Search for webex object initialization
rg -A 5 "registerCC\(.*webex.*\)"
Length of output: 40
Script:
#!/bin/bash
# Look for imports of IContactCenter to understand the interface structure
rg -p "import.*IContactCenter"
# Look for files that use webex.cc pattern
rg -p "webex\.cc"
# Look for files that define or use contact center related interfaces
fd -e ts -e js | xargs rg -l "ContactCenter"
Length of output: 657
Script:
#!/bin/bash
# Check the actual implementation in store.ts to verify usage
rg -A 10 "class.*Store" packages/contact-center/store/src/store.ts
# Check how IContactCenter is used in the codebase
rg -A 5 "cc: IContactCenter" packages/contact-center/store/src/store.ts packages/contact-center/store/src/store.types.ts
# Look at the actual initialization
rg -A 5 "this\.cc = webex\.cc" packages/contact-center/store/src/store.ts
Length of output: 2031
packages/contact-center/station-login/src/station-login/index.tsx (1)
22-27: Verify callback handling in consuming code.
Since these callbacks are crucial for the login/logout functionality, let's verify they are properly handled in the consuming HTML/JS code.
✅ Verification successful
Callbacks are properly implemented in the sample code
The sample implementation in docs/web-component-samples/index.html demonstrates the correct usage of both callbacks:
onLoginandonLogoutproperties are properly set after store initialization- Corresponding handler functions
loginSuccessandlogoutSuccessare defined and handle the events
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how these callbacks are used in HTML/JS files
# Look for usage of the web component with these callbacks
rg -t html -t js "widget-cc-station-login.*onLogin" -A 5 -B 5
# Look for any event listeners that might be handling these callbacks
rg -t html -t js "loginSuccess.*function|logoutSuccess.*function" -A 5 -B 5
Length of output: 141
Script:
#!/bin/bash
# Let's try a broader search for the web component usage
rg -t html -t js "widget-cc-station-login" -A 5 -B 5
# Search for the component name in different formats
rg -t html -t js "station-login" -A 5 -B 5
# Look for any login/logout related event handlers with different naming patterns
rg -t html -t js "on-login|on-logout|handleLogin|handleLogout" -A 5 -B 5
# Check for any test files that might demonstrate usage
fd -e test.tsx -e test.js -e spec.tsx -e spec.js --exec rg -l "StationLogin|station-login"
Length of output: 8853
Script:
#!/bin/bash
# Let's check the sample implementation in docs to see if there are any event handlers
cat docs/web-component-samples/index.html
# Let's also check for any JavaScript files in the samples directory
fd -e js . docs/web-component-samples/ --exec cat {}
# Check for any test files that might show usage patterns
fd -e test.tsx -e test.js -e spec.tsx -e spec.js . packages/contact-center/station-login/ --exec cat {}
Length of output: 2477
docs/react-samples/src/App.tsx (3)
1-1: LGTM! State management is well-structured.
The state variables are appropriately initialized with meaningful names and follow React conventions.
Also applies to: 7-8
38-38: Verify store.init interface matches updated specifications.
The PR objectives mention changes to store initialization to accept a webex object, but the current implementation only shows webexConfig and access_token.
✅ Verification successful
Store initialization interface matches the implementation
The code is correctly using the expected interface. From the search results:
- The interface
WithWebexConfigAndTokeninstore.types.tsdefines the required parameters:webexConfigandaccess_token - The store implementation in
store.tsuses these parameters correctly - The usage in
App.tsxmatches this interface by passing both required parameters
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check store.init interface implementation
rg -g '*.{ts,tsx}' 'store\.init.*interface|type.*StoreInit'
Length of output: 61
Script:
#!/bin/bash
# Let's try a broader search for store initialization and its interface
rg -g '*.{ts,tsx}' 'interface.*Store|class.*Store.*{|init\(' -A 5
# Also search for webexConfig usage
rg -g '*.{ts,tsx}' 'webexConfig' -B 2 -A 2
Length of output: 2992
10-16: Consider environment-specific configuration and validation.
- The logger level 'log' might be too verbose for production. Consider using environment variables to set different log levels for development and production.
- It would be good to validate that credentials being undefined is an expected state before SDK initialization.
✅ Verification successful
The logger level and credentials configuration are appropriate for this context
The current configuration is appropriate because:
-
The logger level 'log' is used in a React sample/demo app context (docs/react-samples/), which is suitable for development and demonstration purposes.
-
Setting credentials to undefined is valid as the SDK initialization pattern shows that actual credentials are injected during runtime via props.accessToken, as seen in the codebase:
const webex = new Webex({
credentials: {
access_token: props.accessToken,
}
})
- The environment-specific configuration is properly handled where needed, as evidenced by the NODE_ENV check for the app name:
const appName = process.env.NODE_ENV === 'production' ? 'webex-widgets-meetings' : 'webex-widgets-meetings-dev';
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any environment-specific configurations in the codebase
rg -g '*.{ts,tsx,js,jsx}' -A 3 'webexConfig.*=.*{'
Length of output: 150
Script:
#!/bin/bash
# Check for webexConfig usage patterns in the codebase
rg -g '*.{ts,tsx,js,jsx}' 'webexConfig\s*=' -A 3
# Look for environment-specific configuration patterns
rg -g '*.{ts,tsx,js,jsx}' 'process\.env' -A 2
# Search for SDK initialization patterns
rg -g '*.{ts,tsx,js,jsx}' 'Webex.*init|createWebex' -A 3
Length of output: 134390
Script:
#!/bin/bash
# Search for webexConfig usage in the codebase with more context
rg -g '*.{ts,tsx,js,jsx}' -A 5 -B 5 'webexConfig\s*=\s*\{.*\}'
# Look for environment-specific configuration patterns in the codebase
rg -g '*.{ts,tsx,js,jsx}' -A 3 'process\.env\.(NODE_ENV|REACT_APP|WEBEX)'
# Search for SDK initialization patterns
rg -g '*.{ts,tsx,js,jsx}' -A 5 'createWebex|Webex\('
Length of output: 4200
docs/web-component-samples/index.html (1)
7-11: LGTM! CSS implementation is clean and purposeful.
The .disabled class implementation is appropriate for managing widget visibility.
packages/contact-center/store/tests/store.test.ts (1)
1-20: LGTM! Clean mock setup.
The mock implementations for both mobx and webex are well-structured and provide the necessary functionality for testing.
Thanks for the approval @Kesari3008. There's only one test comment that's not addressed. It somehow got missed while committing. Will address it once am back. |
|
🎉 This PR is included in version 1.28.0-ccwidgets.2 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
# [1.28.0-ccwidgets.2](webex/widgets@v1.28.0-ccwidgets.1...v1.28.0-ccwidgets.2) (2024-11-26) ### Bug Fixes * **cc-store:** receive webex on init, add store types ([webex#341](webex#341)) ([9648969](webex@9648969))
|
🎉 This PR is included in version 1.28.0-ccconnectors.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
# [1.28.0-ccconnectors.1](webex/widgets@v1.27.5...v1.28.0-ccconnectors.1) (2025-02-05) ### Bug Fixes * add material to all components ([webex#376](webex#376)) ([de0ca28](webex@de0ca28)) * **bundling:** fixed the multiple React instance and useRef errors ([webex#355](webex#355)) ([473cd4f](webex@473cd4f)) * **call-control:** add-call-control-widget ([webex#362](webex#362)) ([a677f5e](webex@a677f5e)) * **cc-station-login:** material design ui ([webex#377](webex#377)) ([aec7034](webex@aec7034)) * **cc-store:** receive webex on init, add store types ([webex#341](webex#341)) ([9648969](webex@9648969)) * **cc-widgets:** ship-all-widgets-together ([webex#345](webex#345)) ([83d5a37](webex@83d5a37)) * cleanup and using mobx-react-lite ([webex#356](webex#356)) ([0b304c4](webex@0b304c4)) * convert npm to yarn ([webex#234](webex#234)) ([432ed3c](webex@432ed3c)) * **release:** add-publish-step-in-tooling ([webex#334](webex#334)) ([ca32235](webex@ca32235)) * rename agent state to user state ([webex#361](webex#361)) ([fe409db](webex@fe409db)) * **samples:** change samples index html hrefs ([webex#367](webex#367)) ([ff126ab](webex@ff126ab)) * **user-state:** receive agent stateChange event ([webex#350](webex#350)) ([21d6ce7](webex@21d6ce7)) ### Features * **cc-components:** setup and move user state sample ui comp ([webex#359](webex#359)) ([16a44d0](webex@16a44d0)) * **cc-store:** add logger from sdk ([webex#354](webex#354)) ([a62494b](webex@a62494b)) * **cc-widgets:** added Agent-Multi-Login-Alert Feature ([webex#364](webex#364)) ([f7d75ca](webex@f7d75ca)) * **release:** add new branch to circleci ([18f7bec](webex@18f7bec)) * **release:** publish pipeline for wxcc widgets ([webex#324](webex#324)) ([864fb52](webex@864fb52)) * taskList and IncomingTask widgets added ([webex#348](webex#348)) ([ce3a619](webex@ce3a619)) * **user-state:** load and change state, client timer ([webex#347](webex#347)) ([f1ccaeb](webex@f1ccaeb)) * **widget-cc-station-login:** Spark 575845 login widget ([webex#239](webex#239)) ([66b8a20](webex@66b8a20)) * **widgets:** added-relogin-logic ([webex#357](webex#357)) ([94dd415](webex@94dd415)) * **widgets:** shifted-timer-to-worker ([webex#352](webex#352)) ([c06fe9c](webex@c06fe9c))
Closes SPARK-580566
This PR addresses:
webexobject input as an optionTesting done
Store.initparameters for webexConfig and access token on React SamplesStore.initparameters for webexConfig and access token on Web Component SamplesPending testing
Store.initparameters for the webex object on React SamplesStore.initparameters for the webex object on Web Component SamplesDemo
https://app.vidcast.io/share/a394ecfa-02b3-4ab4-a2f8-f14b1afbcfe3
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
Chores