feat(widgets): added-agent-rona-popup - #389
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 📝 WalkthroughWalkthroughThe pull request introduces a new optional callback, Possibly related PRs
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
|
|
|
||
| class StoreWrapper implements IStoreWrapper { | ||
| store: IStore; | ||
| onTaskRejected?: (reason: string) => void; |
There was a problem hiding this comment.
I kept it as void return type and have added this in the wrapper as well.
I felt we should input it here as store will be used regardless of whether the user is using task list or incoming task. Further, we are handling the event here so I added this here.
Open to any suggestion however.
| expect(storeWrapper['cc'].off).toHaveBeenCalledWith(CC_EVENTS.AGENT_MULTI_LOGIN, expect.any(Function)); | ||
| }); | ||
|
|
||
| it('should handle task rejection event and call onTaskRejected with the provided reason', () => { |
There was a problem hiding this comment.
Test to check if the reason / rejected task etc. are coming properly.
| setSelectedState(''); | ||
| }; | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
Wanted to add this code here as I wanted to set up the callback on the first mounting itself.
| setSelectedWidgets((prev) => ({...prev, [name]: checked})); | ||
| }; | ||
|
|
||
| const changeAgentState = (newState: string) => { |
There was a problem hiding this comment.
Method to handle agent state change using cc from the store instance.
Please do correct me if there is an issue with this method (I didn't see anything else).
| // If the selected state is "Available", we pass it as is; otherwise, we consider it "Idle". | ||
| const chosenState = newState === 'Available' ? 'Available' : 'Idle'; | ||
| // In the idle codes, we need to search for the 'Idle' state with code name 'Meeting'. | ||
| const lookupCodeName = newState === 'Available' ? 'Available' : 'Meeting'; |
There was a problem hiding this comment.
Sadly the above 2 are needed as backend only recognises Available and Idle.
Also for idle codes, we sadly only have the above 2, hence we have to add this check (note that this is only for sample app, however). We are doing the same in the cc sdk sample app.
| } | ||
|
|
||
| // Helper to show the task rejected popup. | ||
| function showTaskRejectedPopup(reason) { |
There was a problem hiding this comment.
Again, this is so that we can get the popup similar to react sample app, open to suggestions here also.
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://cdn.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
widgets-samples/cc/samples-cc-wc-app/app.js (2)
146-174: Improve error handling and code style.
- The state lookup logic could be more robust
- The optional chaining suggestion from static analysis is valid
Apply these improvements:
- const idleCode = - store.idleCodes && - store.idleCodes.find(function(code) { - return code.name === lookupCodeName; - }); + const idleCode = store.idleCodes?.find((code) => code.name === lookupCodeName);🧰 Tools
🪛 Biome (1.9.4)
[error] 150-153: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
177-201: Consider enhancing the popup implementation.The current implementation has some potential improvements:
- No error handling for missing DOM elements
- Event listener cleanup is missing
- Direct DOM manipulation could be encapsulated better
Would you like me to propose a more robust implementation that addresses these concerns?
packages/contact-center/store/src/storeEventsWrapper.ts (1)
187-193: Reduce code duplication in task rejection handlers.The task rejection handling logic is duplicated between the incoming task and hydrate handlers.
Consider extracting the common logic:
+ private handleTaskReject = (task: ITask, reason: string) => { + if (this.onTaskRejected) { + this.onTaskRejected(reason || 'No reason provided'); + } + this.handleTaskRemove(task.data.interactionId); + }; - task.on(TASK_EVENTS.TASK_REJECT, (reason: string) => { - if (this.onTaskRejected) { - this.onTaskRejected(reason || 'No reason provided'); - } - this.handleTaskRemove(task.data.interactionId); - }); + task.on(TASK_EVENTS.TASK_REJECT, (reason: string) => + this.handleTaskReject(task, reason) + );Also applies to: 224-230
widgets-samples/cc/samples-cc-react-app/src/App.tsx (2)
84-111: LGTM with a minor suggestion!The function correctly handles agent state changes with proper error handling and state updates. Consider adding a type for the
newStateparameter to restrict it to valid values.Apply this diff to add type safety:
-const changeAgentState = (newState: string) => { +type AgentState = 'Available' | 'Busy' | 'On Break'; +const changeAgentState = (newState: AgentState) => {
245-268: Consider extracting popup styles and component.The popup implementation works correctly but could benefit from the following improvements:
- Extract inline styles to a CSS file or styled component.
- Move the popup to a separate component for better maintainability.
- Add keyboard navigation and accessibility attributes.
Example of extracting the popup component:
+const TaskRejectionPopup = ({ reason, selectedState, onStateChange, onSubmit }) => ( + <div className="task-rejection-popup"> + <h2>Task Rejected</h2> + <p>Reason: {reason}</p> + <select + value={selectedState} + onChange={onStateChange} + aria-label="Select agent state" + > + <option value="">Select a state</option> + <option value="Available">Available</option> + <option value="Busy">Busy</option> + <option value="On Break">On Break</option> + </select> + <button onClick={onSubmit}>Submit</button> + </div> +);widgets-samples/cc/samples-cc-wc-app/index.html (2)
11-20: Consider enhancing popup styles for better UX.The current styles provide basic positioning and appearance. Consider adding:
- A semi-transparent overlay to dim the background
- Transition animations for smooth appearance/disappearance
- Mobile-friendly styles with media queries
Example enhancement:
+.popup-overlay { + position: fixed; + top: 0; + left: 0; + width: 100%; + height: 100%; + background: rgba(0, 0, 0, 0.5); + z-index: 999; +} #task-rejected-popup { position: fixed; top: 20%; left: 50%; transform: translate(-50%, 0); background: white; padding: 20px; border: 1px solid #ccc; z-index: 1000; + border-radius: 4px; + box-shadow: 0 2px 10px rgba(0, 0, 0, 0.1); + transition: opacity 0.2s ease-in-out; } +@media (max-width: 768px) { + #task-rejected-popup { + width: 90%; + top: 10%; + } +}
64-64: Consider moving styles to CSS.Move the inline style to the CSS block for better maintainability and consistency.
Apply this diff:
-<div id="popup-container" style="display: none;"></div> +<div id="popup-container"></div>And add to the CSS block:
#popup-container { display: none; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/contact-center/store/src/store.types.ts(1 hunks)packages/contact-center/store/src/storeEventsWrapper.ts(3 hunks)packages/contact-center/store/tests/storeEventsWrapper.ts(1 hunks)packages/contact-center/task/src/task.types.ts(1 hunks)widgets-samples/cc/samples-cc-react-app/src/App.tsx(4 hunks)widgets-samples/cc/samples-cc-wc-app/app.js(3 hunks)widgets-samples/cc/samples-cc-wc-app/index.html(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
widgets-samples/cc/samples-cc-wc-app/app.js
[error] 150-153: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: setup_test_release
- GitHub Check: AWS Amplify Console Web Preview
🔇 Additional comments (8)
packages/contact-center/store/src/store.types.ts (1)
55-55: LGTM! Well-designed callback interface.The optional callback is well-typed and maintains backward compatibility.
packages/contact-center/task/src/task.types.ts (1)
123-123: LGTM! Consistent enum extension.The new event type follows the established naming pattern and string format.
widgets-samples/cc/samples-cc-wc-app/app.js (1)
31-33: LGTM! Clean callback setup.The callback registration is straightforward and follows the store's API.
packages/contact-center/store/src/storeEventsWrapper.ts (1)
8-8: LGTM! Well-typed callback property.The optional callback property is properly typed and matches the interface.
widgets-samples/cc/samples-cc-react-app/src/App.tsx (3)
19-21: LGTM!The state variables are well-named and appropriately scoped for managing the task rejection popup.
113-119: LGTM!The function correctly handles the popup submission by updating the agent state and resetting the popup state.
121-130: LGTM!The effect hook correctly sets up and cleans up the task rejection callback, following React's best practices for side effects.
packages/contact-center/store/tests/storeEventsWrapper.ts (1)
612-633: LGTM!The test case thoroughly verifies the task rejection flow by:
- Setting up the necessary mocks and spies
- Verifying the registration of the rejection callback
- Testing the execution of the callback with the correct reason
- Ensuring proper task cleanup
|
@coderabbitai resolved all your comments. |
|
@adhmenon Thank you for resolving the comments. I'll review the changes again to ensure everything is in order. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
widgets-samples/cc/samples-cc-wc-app/app.js (1)
21-197: Consider extracting popup logic into a reusable component.The current implementation mixes DOM manipulation, state management, and UI logic. Consider creating a reusable web component for the popup to:
- Encapsulate popup-related DOM manipulation and styling
- Make the implementation more consistent with the widget-based architecture
- Improve reusability across different scenarios
Would you like me to provide an example implementation of a reusable popup web component?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/contact-center/store/src/storeEventsWrapper.ts(4 hunks)widgets-samples/cc/samples-cc-wc-app/app.js(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/contact-center/store/src/storeEventsWrapper.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: setup_test_release
- GitHub Check: AWS Amplify Console Web Preview
🔇 Additional comments (2)
widgets-samples/cc/samples-cc-wc-app/app.js (2)
21-21: LGTM!The popup container reference is correctly placed with other DOM element references.
31-33: LGTM!The task rejection callback is properly set up to handle RONA events by showing the popup.
|
Please do a testing with multiple tab with the same user
|
Yes, I have tested this scenario and as mentioned in the test plan - the state syncs, however we do see modals in both windows. I think this behaviour is alright (considering it's not widget specific) and that in each tab we essentially are handling the modal (again a sample app issue). |
|
|
||
| class StoreWrapper implements IStoreWrapper { | ||
| store: IStore; | ||
| onTaskRejected?: (reason: string) => void; |
There was a problem hiding this comment.
Still wondering if we need this or not? Can you try to remove this and see if something breaks? If it breaks something then we can add this and leave it.
Just a question- when we have modal in both the sample pages and we click say change my state to available, do both the modals go away? or only in 1 tab the modal goes away. |
Only 1 goes away actually. |
|
@adhmenon I saw that when Im on a call and I dont do anything the UI is stuck, The incoming task and task-list were still. Can you check that scenario please. |
@Shreyas281299 , checked it in multiple scenarios... I'm not seeing this actually. https://app.vidcast.io/share/538c1c99-512c-4fac-b9cf-9ba0d2b1bc83 |
|
🎉 This PR is included in version 1.28.0-ccwidgets.20 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
# [1.28.0-ccwidgets.20](webex/widgets@v1.28.0-ccwidgets.19...v1.28.0-ccwidgets.20) (2025-02-21) ### Features * **widgets:** added-agent-rona-popup ([webex#389](webex#389)) ([9c52c53](webex@9c52c53))
4 participants
COMPLETES SPARK-604350
ISSUE
FIX
taskRejectionCallbackhandler inside of the widgets store so users can specify their own custom task rejection methodVidcasts
Manual Test Cases
Test Doc Link - https://confluence-eng-gpk2.cisco.com/conf/display/WSDK/CC+Widgets+Test+Plan