feat(user-state): load and change state, client timer #347
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 primarily introduce enhancements to user authentication and state management across various components. A new state variable 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.
Actionable comments posted: 12
🧹 Outside diff range and nitpick comments (10)
docs/web-component-samples/index.html (1)
25-27: LGTM! Consider adding ARIA attributes for accessibility.The restructuring of the disabled state from container to individual widgets improves control granularity.
Consider adding ARIA attributes to improve accessibility:
<div id="widgets-container"> - <widget-cc-station-login class="disabled" id="cc-station-login"></widget-cc-station-login> - <widget-cc-user-state class="disabled" id="cc-user-state"></widget-cc-user-state> + <widget-cc-station-login class="disabled" id="cc-station-login" aria-hidden="true"></widget-cc-station-login> + <widget-cc-user-state class="disabled" id="cc-user-state" aria-hidden="true"></widget-cc-user-state>docs/react-samples/src/App.tsx (2)
Line range hint
41-44: Add error handling for store initialization.The store.init call should handle potential errors to prevent silent failures.
Add error handling:
onClick={() => { - store.init({webexConfig, access_token: accessToken}).then(() => { - setIsSdkReady(true); - }); + store.init({webexConfig, access_token: accessToken}) + .then(() => { + setIsSdkReady(true); + }) + .catch((error) => { + webexConfig.logger.error('Failed to initialize store:', error); + // Consider adding user feedback here + }); }}
49-51: Add unit tests for conditional rendering logic.As mentioned in the PR objectives, unit tests are pending. Ensure coverage for the conditional rendering of UserState component.
Would you like me to help generate unit tests for:
- Login state management
- Conditional rendering logic
- Store initialization error handling
packages/contact-center/station-login/src/helper.ts (1)
Line range hint
23-26: Improve error handling in catch blocksConsider propagating errors to the caller instead of just logging them. This would allow proper error handling at the component level.
}).catch((error: Error) => { console.error(error); setLoginFailure(error); + return Promise.reject(error); }); }).catch((error: Error) => { console.error(error); + setLogoutFailure(error); + return Promise.reject(error); });Also applies to: 36-38
packages/contact-center/store/src/store.ts (1)
Line range hint
39-41: Convert inline comment to JSDoc documentationThe comment about Webex SDK initialization timing is important and should be more visible.
- // If devs decide to go with webex, they will have to listen to the ready event before calling init - // This has to be documented + /** + * @important Developers must listen to the 'ready' event before calling init when using the Webex SDK. + * This ensures proper initialization sequence and prevents timing issues. + */packages/contact-center/user-state/tests/user-state/user-state.presentational.tsx (2)
6-16: Test suite looks good, but could benefit from additional test cases.While the basic rendering test provides good coverage, consider adding these test cases:
- Verify select is disabled when
isSettingAgentStatusis true- Verify component behavior with empty
idleCodesarray- Verify loading state UI when
isSettingAgentStatusis true
24-29: Enhance setAgentStatus test coverage.The current test verifies basic functionality, but consider adding assertions for:
- Select element's disabled state during status update
- Multiple consecutive state changes
- Edge cases with special characters in state names
packages/contact-center/user-state/src/user-state/user-state.presentational.tsx (1)
89-103: Optimize event handler implementation.The onChange event handler could be optimized and made more type-safe.
+ const handleStateChange = React.useCallback((event: React.ChangeEvent<HTMLSelectElement>) => { + const { value: auxCodeId, options, selectedIndex } = event.target; + const state = options[selectedIndex].text; + setAgentStatus({ auxCodeId, state }); + }, [setAgentStatus]); <select id="idleCodes" style={styles.select} - onChange={ - (event) => { - const select = event.target; - const auxCodeId = select.value; - const state = select.options[select.selectedIndex].text; - setAgentStatus({ - auxCodeId, - state - }); - } - } + onChange={handleStateChange} disabled={isSettingAgentStatus} >packages/contact-center/store/tests/store.ts (1)
60-61: Consider adding type validation for idleCodesWhile the assertions verify the presence of the new properties, consider adding type validation for the
idleCodesarray elements to ensure they match the expectedIdleCodeinterface structure.- expect(store.idleCodes).toEqual(mockResponse.idleCodes); + expect(store.idleCodes).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + id: expect.any(String), + name: expect.any(String), + isSystem: expect.any(Boolean), + isDefault: expect.any(Boolean) + }) + ]) + );packages/contact-center/station-login/tests/helper.ts (1)
91-92: Consider adding null callback testsWhile testing undefined callbacks is good, also consider testing explicit null callbacks for more complete coverage.
Add these test cases:
it('should handle null login callback', async () => { ccMock.stationLogin.mockResolvedValue({}); const { result, waitForNextUpdate } = renderHook(() => useStationLogin({cc: ccMock, onLogin: null, onLogout: logoutCb}) ); act(() => { result.current.login(); }); await waitForNextUpdate(); expect(loginCb).not.toHaveBeenCalled(); }); it('should handle null logout callback', async () => { ccMock.stationLogout.mockResolvedValue({}); const { result, waitForNextUpdate } = renderHook(() => useStationLogin({cc: ccMock, onLogin: loginCb, onLogout: null}) ); act(() => { result.current.logout(); }); await waitForNextUpdate(); expect(logoutCb).not.toHaveBeenCalled(); });Also applies to: 193-194
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (17)
docs/react-samples/src/App.tsx(3 hunks)docs/web-component-samples/app.js(2 hunks)docs/web-component-samples/index.html(1 hunks)packages/contact-center/station-login/src/helper.ts(2 hunks)packages/contact-center/station-login/tests/helper.ts(3 hunks)packages/contact-center/store/package.json(0 hunks)packages/contact-center/store/src/store.ts(3 hunks)packages/contact-center/store/src/store.types.ts(2 hunks)packages/contact-center/store/tests/store.ts(1 hunks)packages/contact-center/user-state/package.json(1 hunks)packages/contact-center/user-state/src/helper.ts(1 hunks)packages/contact-center/user-state/src/user-state/index.tsx(1 hunks)packages/contact-center/user-state/src/user-state/use-state.types.ts(1 hunks)packages/contact-center/user-state/src/user-state/user-state.presentational.tsx(1 hunks)packages/contact-center/user-state/tests/helper.ts(1 hunks)packages/contact-center/user-state/tests/user-state/index.tsx(1 hunks)packages/contact-center/user-state/tests/user-state/user-state.presentational.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- packages/contact-center/store/package.json
🧰 Additional context used
📓 Learnings (1)
packages/contact-center/user-state/src/helper.ts (1)
Learnt from: Kesari3008
PR: webex/widgets#239
File: packages/contact-center/user-state/src/helper.ts:2-10
Timestamp: 2024-11-19T13:50:41.173Z
Learning: The `user-state` component currently contains placeholder code and will be addressed in future PRs.
🪛 Biome (1.9.4)
packages/contact-center/user-state/src/user-state/user-state.presentational.tsx
[error] 104-115: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 113-113: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
🔇 Additional comments (11)
packages/contact-center/user-state/src/helper.ts (1)
21-34: Verify error handling in setAgentStatus function
The setAgentStatus function handles setting the agent status and updates state variables appropriately. Ensure that any errors returned from cc.setAgentState provide meaningful feedback to the user and consider logging additional error details if necessary.
packages/contact-center/user-state/src/user-state/use-state.types.ts (1)
1-18: IUserState interface correctly defines user state properties
The IUserState interface accurately represents the user state with the added properties idleCodes, isSettingAgentStatus, errorMessage, and elapsedTime. The method setAgentStatus is properly updated to accept the necessary parameters.
packages/contact-center/user-state/src/user-state/index.tsx (1)
8-16: Component updated to pass necessary props to useUserState
The UserState component now correctly destructures cc, idleCodes, and agentId from the store and passes them to the useUserState hook. This ensures that the hook receives all necessary data.
packages/contact-center/store/src/store.types.ts (1)
14-20: Type definitions for IdleCode added appropriately
The IdleCode type is correctly defined and added to the IStore interface. Exporting IdleCode ensures it can be used throughout the application where needed.
Also applies to: 25-25, 37-37
packages/contact-center/user-state/package.json (1)
49-53: LGTM: Jest configuration follows best practices
The Jest configuration changes are well-structured:
- Test patterns correctly target TypeScript test files
- Verbose output will help in debugging test failures
packages/contact-center/station-login/src/helper.ts (2)
20-22: LGTM: Added null check for loginCb
The null check before invoking the callback prevents potential runtime errors.
33-35: LGTM: Added null check for logoutCb
The null check before invoking the callback prevents potential runtime errors.
packages/contact-center/store/src/store.ts (2)
8-8: LGTM: Well-structured store property additions
The new properties (idleCodes and agentId) are properly typed and initialized, maintaining type safety throughout the store.
Also applies to: 17-18
29-30: LGTM: Proper assignment of new properties from response
The new properties are correctly populated from the registration response.
packages/contact-center/store/tests/store.ts (1)
50-52: LGTM: Mock response structure matches expected data model
The mock response correctly includes the new properties idleCodes and agentId with appropriate test data structure.
packages/contact-center/station-login/tests/helper.ts (1)
22-24: LGTM: Good practice for test isolation
Adding jest.clearAllMocks() in afterEach ensures proper test isolation.
| console.log('Agent login has been succesful'); | ||
| ccUserState.classList.remove('disabled'); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace console logs with proper logging utility.
Production code should use a proper logging utility instead of console.log/error.
Consider using the logger from webexConfig:
function loginSuccess(){
- console.log('Agent login has been succesful');
+ webexConfig.logger.log('Agent login has been succesful');
ccUserState.classList.remove('disabled');
}
function logoutSuccess(){
- console.log('Agent logout has been succesful');
+ webexConfig.logger.log('Agent logout has been succesful');
ccUserState.classList.add('disabled');
}Also applies to: 35-36
There was a problem hiding this comment.
This is a sample app change. Not going to do it.
There was a problem hiding this comment.
@mkesavan13, understood. Since this is a sample app change, it's acceptable to use console.log here. I'll keep that in mind for future reviews.
✏️ Learnings added
Learnt from: mkesavan13
PR: webex/widgets#347
File: docs/web-component-samples/app.js:30-31
Timestamp: 2024-12-06T00:30:30.668Z
Learning: In sample app code within this repository, using `console.log` for logging is acceptable, and replacing it with a proper logging utility is not required.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
docs/web-component-samples/app.js
Outdated
| const accessTokenElem = document.getElementById('access_token_elem'); | ||
| const ccStationLogin = document.getElementById('cc-station-login'); | ||
| const ccUserState = document.getElementById('cc-user-state'); |
There was a problem hiding this comment.
Add null check for DOM element reference.
The DOM element reference could be null if the element isn't found.
Add a null check:
- const ccUserState = document.getElementById('cc-user-state');
+ const ccUserState = document.getElementById('cc-user-state');
+ if (!ccUserState) {
+ throw new Error('User state widget element not found');
+ }📝 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 ccUserState = document.getElementById('cc-user-state'); | |
| const ccUserState = document.getElementById('cc-user-state'); | |
| if (!ccUserState) { | |
| throw new Error('User state widget element not found'); | |
| } |
There was a problem hiding this comment.
Addressed this with a console error and here's the code,
if (!ccStationLogin && !ccUserState) {
console.error('Failed to find the required elements');
}There was a problem hiding this comment.
@mkesavan13, thanks for addressing this issue. Just a note: with your current code, the error message will only be logged if both ccStationLogin and ccUserState are not found, due to the use of the logical AND (&&) operator. If your intention is to log an error when either of the elements is missing, you might want to use the logical OR (||) operator instead:
if (!ccStationLogin || !ccUserState) {
console.error('Failed to find the required elements');
}This way, the error message will be logged if either ccStationLogin or ccUserState is missing.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| const formatTime = (time) => { | ||
| const hours = Math.floor(time / 3600); | ||
| const minutes = Math.floor((time % 3600) / 60); | ||
| const seconds = time % 60; | ||
| return `${hours.toString().padStart(2, '0')}:${minutes.toString().padStart(2, '0')}:${seconds.toString().padStart(2, '0')}`; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type safety to formatTime function.
The time parameter lacks type annotation which could lead to runtime errors.
- const formatTime = (time) => {
+ const formatTime = (time: number): string => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const formatTime = (time) => { | |
| const hours = Math.floor(time / 3600); | |
| const minutes = Math.floor((time % 3600) / 60); | |
| const seconds = time % 60; | |
| return `${hours.toString().padStart(2, '0')}:${minutes.toString().padStart(2, '0')}:${seconds.toString().padStart(2, '0')}`; | |
| }; | |
| const formatTime = (time: number): string => { | |
| const hours = Math.floor(time / 3600); | |
| const minutes = Math.floor((time % 3600) / 60); | |
| const seconds = time % 60; | |
| return `${hours.toString().padStart(2, '0')}:${minutes.toString().padStart(2, '0')}:${seconds.toString().padStart(2, '0')}`; | |
| }; |
There was a problem hiding this comment.
@mkesavan13, thank you for addressing the issue!
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
packages/contact-center/user-state/src/user-state/user-state.presentational.tsx
Outdated
Show resolved
Hide resolved
packages/contact-center/user-state/src/user-state/user-state.presentational.tsx
Outdated
Show resolved
Hide resolved
| it('should not call logout callback if not present', async () => { | ||
| ccMock.stationLogout.mockResolvedValue({}); | ||
|
|
||
| const {result, waitForNextUpdate} = renderHook(() => | ||
| //@ts-ignore | ||
| useStationLogin({cc: ccMock, onLogin: loginCb}) | ||
| ); | ||
|
|
||
| act(() => { | ||
| result.current.logout(); | ||
| }); | ||
|
|
||
| await waitForNextUpdate(); | ||
|
|
||
| expect(logoutCb).not.toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Apply the same typing fix for logout callback test
Similar to the login callback test, avoid @ts-ignore and use proper typing.
- //@ts-ignore
- useStationLogin({cc: ccMock, onLogin: loginCb})
+ useStationLogin({cc: ccMock, onLogin: loginCb})Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@mkesavan13 please add the tests to the PR description that you've done as well.
Done, Sreekanth |
packages/contact-center/user-state/src/user-state/use-state.types.ts
Outdated
Show resolved
Hide resolved
| it('setAgentStatus should handle error', async () => { | ||
| ccMock.setAgentState.mockRejectedValueOnce(new Error('Network error')); | ||
| const { result } = renderHook(() => useUserState({ idleCodes: [], agentId: '123', cc: ccMock })); | ||
|
|
||
| act(() => { | ||
| result.current.setAgentStatus({ auxCodeId: '001', state: 'Available' }); | ||
| }); | ||
|
|
||
| expect(result.current.isSettingAgentStatus).toBe(true); | ||
|
|
||
| waitFor(() => { | ||
| expect(result.current.isSettingAgentStatus).toBe(false); | ||
| expect(result.current.errorMessage).toBe('Error: Network error'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Add tests for error timer scenario
packages/contact-center/user-state/src/user-state/user-state.presentational.tsx
Show resolved
Hide resolved
|
🎉 This PR is included in version 1.28.0-ccwidgets.3 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
# [1.28.0-ccwidgets.3](webex/widgets@v1.28.0-ccwidgets.2...v1.28.0-ccwidgets.3) (2024-12-16) ### Features * **user-state:** load and change state, client timer ([webex#347](webex#347)) ([f1ccaeb](webex@f1ccaeb))
|
🎉 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-584812
This PR addresses
onLogincallback of Station Login WidgetPending Items
Manual Tests Done
Summary by CodeRabbit
Release Notes
New Features
UserStatecomponent to utilize specific properties for better data handling.Bug Fixes
Tests
useUserStatehook andUserStatecomponent, ensuring robust functionality and error handling.