fix(store): minor-fixes-after-storeEventsWrapper #390
Conversation
📝 WalkthroughWalkthroughThis pull request removes the Possibly related PRs
✨ Finishing Touches
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
Documentation and Community
|
Shreyas281299
changed the title
| @@ -8,18 +8,13 @@ export const useStationLogin = (props: UseStationLoginProps) => { | |||
| const loginCb = props.onLogin; | |||
| const logoutCb = props.onLogout; | |||
| const logger = props.logger; | |||
| const [isAgentLoggedIn, setIsAgentLoggedIn] = useState(props.isAgentLoggedIn); | |||
There was a problem hiding this comment.
original comment- See if we can remove the isAgentLogged in props as we already have this in the store
Removed the local state
| @@ -22,8 +21,10 @@ const StationLoginComponent: React.FunctionComponent<StationLoginProps> = ({onLo | |||
| teams, | |||
| loginOptions, | |||
| deviceType, | |||
| isAgentLoggedIn, | |||
| showMultipleLoginAlert, | |||
There was a problem hiding this comment.
Original comment- we can move showMultipleLoginAlert to results as we already have it rather than putting it directly in the presentation component
Moved
| incomingTask: observable, | ||
| taskList: observable, | ||
| wrapupRequired: observable, | ||
| currentState: observable, |
There was a problem hiding this comment.
Store is already updating these values and the latest values are reaching the components, so we dont need them as observable here, makeAutoObservable method is doing this for us.
| @@ -97,6 +97,7 @@ enum CC_EVENTS{ | |||
|
|
|||
| export type { | |||
| IContactCenter, | |||
| ITask, | |||
There was a problem hiding this comment.
Importing ITask from cc-sdk and exporting it from store, all the types from cc-sdk wll be imported and exported from the store, this will ensure cc-store is the only place where we use cc-sdk as a dependency
| IWrapupCode, | ||
| WithWebex, | ||
| IContactCenter, | ||
| ITask, |
There was a problem hiding this comment.
ITask imported from store/types
| @@ -235,14 +244,15 @@ class StoreWrapper implements IStoreWrapper { | |||
| } | |||
| }; | |||
|
|
|||
| setupIncomingTaskHandler = (ccSDK: any) => { | |||
| setupIncomingTaskHandler = (ccSDK: IContactCenter) => { | |||
| // return this.store.registerCC(webex); | ||
| // }; | ||
|
|
||
| it('should call registerCC', () => { |
There was a problem hiding this comment.
For coverage, I forgot to add test for this
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
| @@ -156,7 +177,7 @@ describe('storeEventsWrapper', () => { | |||
| }); | |||
|
|
|||
| it('should setWrapupCodes', () => { | |||
| const mockCodes = [{code1: 'code1'}, {code2: 'code2'}]; | |||
| const mockCodes = [{id: 'code1', name: 'code1'}]; | |||
There was a problem hiding this comment.
The types was mismatching, so fixed this
| @@ -20,8 +20,7 @@ | |||
| }, | |||
| "dependencies": { | |||
| "@webex/cc-store": "workspace:*", | |||
| "mobx-react-lite": "^4.1.0", | |||
| "webex": "3.7.0-wxcc.15" | |||
There was a problem hiding this comment.
webex will be dependency only in cc-store and no where else.
| const result = { | ||
| ...useCallControl({currentTask, deviceType, onHoldResume, onEnd, onWrapUp, logger}), | ||
| wrapupRequired, | ||
| wrapupCodes, | ||
| }; |
There was a problem hiding this comment.
refactor to avoid passing props directly to the presentational
| @@ -114,7 +115,7 @@ export const useCallControl = (props: useCallControlProps) => { | |||
| ); | |||
|
|
|||
| useEffect(() => { | |||
| if (!currentTask) return; | |||
| if (!currentTask || !isBrowser) return; | |||
There was a problem hiding this comment.
We will only add media listeners when the login type is BROWSER as thats the only case when we will ahve this event
| onTaskAccepted: null, | ||
| onTaskDeclined: null, |
There was a problem hiding this comment.
These are optional, so we can just remove it rather than keeping them as null
| }) | ||
| ); | ||
|
|
||
| await act(async () => { | ||
| await result.current.wrapupCall('Wrap reason', 123); | ||
| await result.current.wrapupCall('Wrap reason', '123'); |
| } else { | ||
| workerRef.current.postMessage({type: 'start', startTime: Date.now()}); | ||
| } | ||
| if (workerRef.current && lastStateChangeTimestamp) { |
There was a problem hiding this comment.
I don't know why this code was not working when I tried earlier but I just reverted to the original code and the timer was working fine, so reverted these changes. Since this was already tested no new tests are required, The changes were added as part of #371
|
@CodeRabbit 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: 0
🧹 Nitpick comments (2)
packages/contact-center/task/tests/IncomingTask/index.tsx (1)
18-38: Consider adding test cases for non-null incomingTask scenarios.While the current test verifies the basic component setup, consider adding test cases to verify:
- Component behavior when incomingTask is non-null
- Error handling scenarios
- Edge cases like undefined or invalid task data
packages/contact-center/task/tests/helper.ts (1)
869-895: Improve test description.The test description "test undefined audioRef.current" could be more descriptive. Consider renaming it to something like "should handle undefined audioRef.current gracefully".
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (18)
packages/contact-center/station-login/src/helper.ts(0 hunks)packages/contact-center/station-login/src/station-login/index.tsx(1 hunks)packages/contact-center/station-login/src/station-login/station-login.types.ts(1 hunks)packages/contact-center/station-login/tests/helper.ts(0 hunks)packages/contact-center/station-login/tests/station-login/index.tsx(2 hunks)packages/contact-center/store/src/store.ts(0 hunks)packages/contact-center/store/src/store.types.ts(2 hunks)packages/contact-center/store/src/storeEventsWrapper.ts(2 hunks)packages/contact-center/store/tests/store.ts(0 hunks)packages/contact-center/store/tests/storeEventsWrapper.ts(4 hunks)packages/contact-center/task/package.json(1 hunks)packages/contact-center/task/src/CallControl/index.tsx(1 hunks)packages/contact-center/task/src/helper.ts(2 hunks)packages/contact-center/task/src/task.types.ts(3 hunks)packages/contact-center/task/tests/CallControl/index.tsx(1 hunks)packages/contact-center/task/tests/IncomingTask/index.tsx(1 hunks)packages/contact-center/task/tests/helper.ts(21 hunks)packages/contact-center/user-state/src/helper.ts(1 hunks)
💤 Files with no reviewable changes (4)
- packages/contact-center/store/src/store.ts
- packages/contact-center/station-login/src/helper.ts
- packages/contact-center/store/tests/store.ts
- packages/contact-center/station-login/tests/helper.ts
🧰 Additional context used
🧠 Learnings (5)
packages/contact-center/user-state/src/helper.ts (1)
Learnt from: mkesavan13
PR: webex/widgets#347
File: packages/contact-center/user-state/src/helper.ts:10-19
Timestamp: 2024-12-06T01:07:12.444Z
Learning: In `packages/contact-center/user-state/src/helper.ts`, within the `useUserState` hook, the timer reset in the `useEffect` hook is handled during the state change success. Therefore, modifying the dependency array is unnecessary.
packages/contact-center/station-login/src/station-login/station-login.types.ts (2)
Learnt from: pagour98
PR: webex/widgets#385
File: packages/contact-center/station-login/src/station-login/station-login.types.ts:129-132
Timestamp: 2025-02-13T06:59:01.103Z
Learning: In the station-login component's type definitions, it's acceptable for `UseStationLoginProps` to have a subset of props (`cc`, `onLogin`, `onLogout`, `logger`, `isAgentLoggedIn`) compared to `IStationLoginProps` and `StationLoginPresentationalProps`, even if properties like `handleContinue` and `showMultipleLoginAlert` exist in the latter interfaces.
Learnt from: pagour98
PR: webex/widgets#385
File: packages/contact-center/station-login/src/station-login/index.tsx:10-10
Timestamp: 2025-02-13T06:58:01.805Z
Learning: In the StationLogin component (packages/contact-center/station-login/src/station-login/index.tsx), `handleContinue` and `showMultipleLoginAlert` props have been intentionally removed from store destructuring as part of the Agent Multi-Login configurability changes.
packages/contact-center/station-login/src/station-login/index.tsx (4)
Learnt from: pagour98
PR: webex/widgets#385
File: packages/contact-center/station-login/src/station-login/index.tsx:10-10
Timestamp: 2025-02-13T06:58:01.805Z
Learning: In the StationLogin component (packages/contact-center/station-login/src/station-login/index.tsx), `handleContinue` and `showMultipleLoginAlert` props have been intentionally removed from store destructuring as part of the Agent Multi-Login configurability changes.
Learnt from: pagour98
PR: webex/widgets#385
File: packages/contact-center/station-login/src/station-login/station-login.types.ts:129-132
Timestamp: 2025-02-13T06:59:01.103Z
Learning: In the station-login component's type definitions, it's acceptable for `UseStationLoginProps` to have a subset of props (`cc`, `onLogin`, `onLogout`, `logger`, `isAgentLoggedIn`) compared to `IStationLoginProps` and `StationLoginPresentationalProps`, even if properties like `handleContinue` and `showMultipleLoginAlert` exist in the latter interfaces.
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.
Learnt from: Kesari3008
PR: webex/widgets#239
File: packages/contact-center/station-login/tests/station-login/station-login.presentational.tsx:0-0
Timestamp: 2024-11-19T13:44:14.152Z
Learning: The file `packages/contact-center/station-login/tests/station-login/station-login.presentational.tsx` is going to change completely, so writing tests for these changes are being ignored.
packages/contact-center/task/tests/CallControl/index.tsx (2)
Learnt from: Shreyas281299
PR: webex/widgets#362
File: packages/contact-center/task/tests/CallControl/index.tsx:29-46
Timestamp: 2025-01-20T09:52:57.754Z
Learning: UI testing is not mandatory for components in the @webex/widgets repository. Focus should be on functional testing rather than UI element verification.
Learnt from: Shreyas281299
PR: webex/widgets#362
File: packages/contact-center/task/tests/CallControl/call-control.presentational.tsx:0-0
Timestamp: 2025-01-20T09:52:20.775Z
Learning: UI testing is not mandatory for React components in the webex/widgets repository.
packages/contact-center/station-login/tests/station-login/index.tsx (2)
Learnt from: Kesari3008
PR: webex/widgets#239
File: packages/contact-center/station-login/tests/station-login/station-login.presentational.tsx:0-0
Timestamp: 2024-11-19T13:44:14.152Z
Learning: The file `packages/contact-center/station-login/tests/station-login/station-login.presentational.tsx` is going to change completely, so writing tests for these changes are being ignored.
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 (23)
packages/contact-center/task/tests/IncomingTask/index.tsx (1)
22-28: LGTM! The mock return value aligns with the hook's new structure.The changes correctly reflect the restructured return value of the useIncomingTask hook, maintaining the essential test coverage while removing obsolete properties.
packages/contact-center/task/src/CallControl/index.tsx (2)
10-15: LGTM! Props consolidation improves code organization.The refactoring to consolidate all props into a single
resultobject improves code organization and follows the previous review suggestion to avoid passing props directly.
17-17: LGTM! Clean prop spreading.The simplified prop spreading to the presentational component makes the code more maintainable.
packages/contact-center/station-login/src/station-login/index.tsx (2)
24-25: LGTM! Props consolidation aligns with previous feedback.The addition of
isAgentLoggedInandshowMultipleLoginAlertto the props object aligns with the previous review feedback about moving these properties to the result object.
27-27: LGTM! Clean prop spreading.The simplified prop spreading to the presentational component improves code maintainability.
packages/contact-center/task/tests/CallControl/index.tsx (1)
36-42: LGTM! Test assertions are more flexible and complete.The changes improve test robustness by:
- Using
expect.any(Object)forcurrentTaskmakes the test more flexible- Adding
deviceTypeassertion ensures the new property is properly testedpackages/contact-center/station-login/tests/station-login/index.tsx (3)
8-16: LGTM! Mock objects improve test isolation.The introduction of dedicated mock objects improves test isolation and makes the test setup more maintainable.
36-38: LGTM! Presentational component spy improves test coverage.The addition of the presentational component spy ensures proper rendering is tested.
42-48: LGTM! Updated assertions match component changes.The test assertions correctly reflect the changes in the component's props structure.
packages/contact-center/user-state/src/helper.ts (1)
39-44: LGTM! Timer reset logic has been simplified.The changes align with the previous learning that timer reset should be handled during state change success. The simplified logic correctly calculates elapsed time and resets the worker timer.
packages/contact-center/station-login/src/station-login/station-login.types.ts (1)
129-129: LGTM! Removal of isAgentLoggedIn aligns with Agent Multi-Login changes.The removal of
isAgentLoggedInfromUseStationLoginPropsis consistent with the previous learnings about Agent Multi-Login configurability changes.packages/contact-center/store/src/store.types.ts (1)
100-110: LGTM! Centralizing type exports in cc-store.The addition of
ITaskto the exports aligns with the goal of centralizing all cc-sdk type exports in cc-store, improving dependency management.packages/contact-center/task/src/task.types.ts (3)
1-1: LGTM! Imports consolidated from cc-store.The consolidation of imports from
@webex/cc-storealigns with the centralization of type exports.
191-196: LGTM! Well-documented deviceType property addition.The addition of the
deviceTypeproperty toControlPropsis properly documented with JSDoc comments.
212-215: LGTM! Consistent type updates.The
useCallControlPropstype has been correctly updated to include the newdeviceTypeproperty.packages/contact-center/task/package.json (1)
21-24: Webex Dependency Removal is Correct and Consistent.The removal of the
"webex": "3.7.0-wxcc.15"dependency aligns with the overall refactoring strategy—ensuring that any Webex-related functionality is now consolidated within@webex/cc-store. This change is consistent with prior review guidance (see previous comment by Shreyas281299). Please verify that no residual references to the removed dependency exist elsewhere in the codebase.packages/contact-center/task/src/helper.ts (2)
97-99: LGTM! Good defensive programming.The addition of
deviceTypeparameter andisBrowsercheck improves the robustness of the hook by ensuring device-specific functionality is handled correctly.
118-118: LGTM! Improved conditional check.The addition of
!isBrowserto the condition ensures media listeners are only added for browser devices, which is the correct behavior as confirmed by past review comments.packages/contact-center/store/src/storeEventsWrapper.ts (2)
247-247: LGTM! Improved type safety.Replacing
anywithIContactCentertype improves type safety and code maintainability.
255-255: Track the TODO for removing event listeners.The TODO comment indicates a potential memory leak issue that needs to be addressed. Please ensure this is tracked in JIRA ticket SPARK-617635.
packages/contact-center/store/tests/storeEventsWrapper.ts (3)
127-140: LGTM! Good test coverage for registerCC.The test verifies both the basic call and the call with parameters, ensuring the method works correctly in different scenarios.
176-176: LGTM! Updated mock codes structure.The mock codes structure has been updated to use
idandnameproperties, aligning with the expected format.
896-912: LGTM! Good test for device type handling.The test verifies that media listeners are not added for non-browser devices, which is an important edge case.
|
Changes look alright. Can we have the updated test plan with the tests done? (just to see that nothing breaks). |
|
🎉 This PR is included in version 1.28.0-ccwidgets.19 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
# [1.28.0-ccwidgets.19](webex/widgets@v1.28.0-ccwidgets.18...v1.28.0-ccwidgets.19) (2025-02-20) ### Bug Fixes * **store:** minor-fixes-after-storeEventsWrapper ([webex#390](webex#390)) ([8738ab2](webex@8738ab2))
COMPLETES #SPARK-610353
This pull request addresses
The final round of review for the PR -#369
Since we were on a clock these non-blocking comments were left.
by making the following changes
Change Type
The following scenarios were tested
Screen.Recording.2025-02-19.at.11.23.19.AM.mov
Checklist before merging
Make sure to have followed the contributing guidelines before submitting.