fix(call-control): add-call-control-widget #362
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 WalkthroughThis pull request introduces a new Changes
Sequence DiagramsequenceDiagram
participant User
participant CallControl
participant Store
participant Task
User->>CallControl: Interact with call controls
CallControl->>Store: Update current task state
Store->>Task: Manage task actions
Task-->>CallControl: Return task status
CallControl-->>User: Update UI based on task state
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 (
|
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
Shreyas281299
requested review from
Kesari3008 and
mkesavan13
and removed request for
Kesari3008
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (14)
packages/contact-center/task/src/helper.ts (5)
69-69: Consider removing unnecessarystore.setCurrentTask(null)after declining a taskSetting
store.setCurrentTask(null);after declining a task may be redundant if the current task is alreadynull. Review whether this call is necessary to prevent unnecessary state updates.
116-117: Consolidate state resets into a single functionYou're resetting
isAnsweredandisEndedtofalsewhen a new task is assigned. Consider creating a helper function to reset these states, enhancing code readability and maintainability.
210-236: RefactorholdResumefunction to eliminate duplicated codeThe
holdResumefunction contains duplicated logic for holding and resuming calls. Refactoring can improve readability and reduce maintenance effort.Here’s a suggested refactor:
const holdResume = (hold: boolean) => { - if (hold) { - currentTask - .hold() - .then(() => { - onHoldResume(); - }) - .catch((error: Error) => { - logger.error(`Error holding call: ${error}`, { - module: 'widget-cc-task#helper.ts', - method: 'useCallControl#holdResume', - }); - }); - } else { - currentTask - .resume() - .then(() => { - onHoldResume(); - }) - .catch((error: Error) => { - logger.error(`Error resuming call: ${error}`, { - module: 'widget-cc-task#helper.ts', - method: 'useCallControl#holdResume', - }); - }); - } + const action = hold ? 'hold' : 'resume'; + currentTask[action]() + .then(() => { + onHoldResume(); + }) + .catch((error: Error) => { + logger.error(`Error ${hold ? 'holding' : 'resuming'} call: ${error}`, { + module: 'widget-cc-task#helper.ts', + method: 'useCallControl#holdResume', + }); + }); };
238-254: RefactorpauseResumeRecordingto reduce code duplicationThe
pauseResumeRecordingfunction has similar code blocks for pausing and resuming recording. Refactoring can simplify the code.Refactored function:
const pauseResumeRecording = (pause: boolean) => { - if (pause) { - currentTask.pauseRecording().catch((error: Error) => { - logger.error(`Error pausing recording: ${error}`, { - module: 'widget-cc-task#helper.ts', - method: 'useCallControl#pauseResumeRecording', - }); - }); - } else { - currentTask.resumeRecording().catch((error: Error) => { - logger.error(`Error resuming recording: ${error}`, { - module: 'widget-cc-task#helper.ts', - method: 'useCallControl#pauseResumeRecording', - }); - }); - } + const action = pause ? 'pauseRecording' : 'resumeRecording'; + currentTask[action]().catch((error: Error) => { + logger.error(`Error ${pause ? 'pausing' : 'resuming'} recording: ${error}`, { + module: 'widget-cc-task#helper.ts', + method: 'useCallControl#pauseResumeRecording', + }); + }); };
256-268: Consider resetting current task in the store after ending a callIn the
endCallfunction, after ending the task, you might want to reset the current task in the store withstore.setCurrentTask(null);to reflect that there's no active task.packages/contact-center/task/src/CallControl/index.tsx (1)
9-15: Consider adding error boundaries and prop validation.While the component implementation is clean, consider these improvements for robustness:
- Wrap with an error boundary to handle potential runtime errors
- Add prop-types or TypeScript interface validation for the callback props
- Consider adding loading and error states
Example error boundary implementation:
class CallControlErrorBoundary extends React.Component<{children: React.ReactNode}> { state = { hasError: false }; static getDerivedStateFromError(error: Error) { return { hasError: true }; } render() { if (this.state.hasError) { return <div>Error loading call controls</div>; } return this.props.children; } } // Usage const CallControl: React.FunctionComponent<CallControlProps> = observer(({onHoldResume, onEnd, onWrapUp}) => { // ... existing implementation }); export const CallControlWithErrorBoundary = (props: CallControlProps) => ( <CallControlErrorBoundary> <CallControl {...props} /> </CallControlErrorBoundary> );packages/contact-center/store/src/store.ts (1)
33-35: Add validation in setCurrentTask method.The method should validate the task object before assignment:
-setCurrentTask(task: any): void { +setCurrentTask(task: Task | null): void { + if (task !== null && !this.isValidTask(task)) { + throw new Error('Invalid task object'); + } this.currentTask = task; } + +private isValidTask(task: Task): boolean { + return typeof task.hold === 'function' && + typeof task.resume === 'function' && + typeof task.end === 'function' && + typeof task.wrapup === 'function'; +}packages/contact-center/task/src/task.types.ts (1)
123-135: Add JSDoc comments for the newControlPropsinterface.The interface lacks documentation for its properties and methods. Add JSDoc comments to improve code maintainability.
+/** + * Interface for call control functionality + */ export interface ControlProps { + /** Current task being handled */ currentTask: ITask; + /** Callback for hold/resume action */ onHoldResume: () => void; + /** Callback for end call action */ onEnd: () => void; + /** Callback for wrap-up action */ onWrapUp: () => void; logger: ILogger; wrapupCodes: any[]; wrapupRequired: boolean; + /** Function to hold or resume a call */ holdResume: (hold: boolean) => void; + /** Function to pause or resume call recording */ pauseResumeRecording: (pause: boolean) => void; + /** Function to end the current call */ endCall: () => void; + /** Function to wrap up a call with reason and ID */ wrapupCall: (wrapupReason: string, wrapupId: string) => void; }packages/contact-center/task/src/CallControl/call-control.presentational.tsx (2)
66-80: Add aria-label to improve accessibility.The select and button elements lack proper aria labels for screen readers.
- <select className="select" onChange={handleWrapupChange} defaultValue="" disabled={!wrapupRequired}> + <select + className="select" + onChange={handleWrapupChange} + defaultValue="" + disabled={!wrapupRequired} + aria-label="Select wrap-up reason" + > ... <button className="btn" onClick={handleWrapupCall} disabled={!wrapupRequired && !selectedWrapupReason} + aria-label="Wrap up call" >
21-28: Logic in handlePauseResumeRecording can be simplified.The conditional logic can be simplified by directly passing the negated state.
const handlePauseResumeRecording = () => { - if (isRecordingPaused) { - pauseResumeRecording(true); - } else { - pauseResumeRecording(false); - } + pauseResumeRecording(!isRecordingPaused); setIsRecordingPaused(!isRecordingPaused); };packages/contact-center/task/tests/CallControl/call-control.presentational.tsx (1)
17-24: Add type safety to defaultProps mock object.The mock object should use proper typing to ensure type safety in tests.
+import {CallControlPresentationalProps} from '../../src/task.types'; - const defaultProps = { + const defaultProps: CallControlPresentationalProps = {packages/contact-center/task/src/IncomingTask/incoming-task.presentational.tsx (2)
123-125: Consider using early return pattern for better readability.The conditional rendering can be made more explicit using early returns.
- if (!currentTask || isAnswered) { - return <></>; // hidden component - } + if (!currentTask) { + return null; // No task available + } + + if (isAnswered) { + return null; // Task already answered + }
Line range hint
4-122: Move inline styles to a separate CSS/SCSS file.Large inline style objects make the component harder to maintain and can impact performance. Consider moving styles to a separate CSS/SCSS file.
Create a new file
incoming-task.styles.scssand move the styles there. This will improve maintainability and enable better style reuse.packages/contact-center/task/tests/helper.ts (1)
512-795: Add missing test cases for better coverage.Consider adding the following test cases to improve coverage:
- Test behavior when callback handlers (onHoldResume, onEnd, onWrapUp) are null/undefined
- Test validation of task state before performing operations (e.g., can't hold an ended call)
Example test case for null callbacks:
it('should handle null callback handlers gracefully', async () => { const {result} = renderHook(() => useCallControl({ currentTask: mockCurrentTask, logger: mockLogger, }) ); await act(async () => { await result.current.holdResume(true); await result.current.endCall(); await result.current.wrapupCall('reason', 123); }); expect(mockCurrentTask.hold).toHaveBeenCalled(); expect(mockCurrentTask.end).toHaveBeenCalled(); expect(mockCurrentTask.wrapup).toHaveBeenCalled(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
docs/react-samples/src/App.tsx(2 hunks)packages/contact-center/cc-widgets/src/index.ts(1 hunks)packages/contact-center/store/src/store.ts(2 hunks)packages/contact-center/task/src/CallControl/call-control.presentational.tsx(1 hunks)packages/contact-center/task/src/CallControl/call-control.styles.scss(1 hunks)packages/contact-center/task/src/CallControl/index.tsx(1 hunks)packages/contact-center/task/src/IncomingTask/incoming-task.presentational.tsx(1 hunks)packages/contact-center/task/src/helper.ts(8 hunks)packages/contact-center/task/src/index.ts(1 hunks)packages/contact-center/task/src/task.types.ts(2 hunks)packages/contact-center/task/tests/CallControl/call-control.presentational.tsx(1 hunks)packages/contact-center/task/tests/CallControl/index.tsx(1 hunks)packages/contact-center/task/tests/helper.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/contact-center/task/src/CallControl/call-control.styles.scss
🔇 Additional comments (9)
packages/contact-center/task/src/helper.ts (3)
50-50: Verify the necessity of updatingstore.setCurrentTaskafter accepting a taskAfter accepting a task, you are updating the store with
store.setCurrentTask(task);. Ensure that this action aligns with your application's state management strategy and that it doesn't introduce unintended side effects.
146-146: Ensure consistency when updatingstore.setCurrentTaskafter accepting an incoming taskIn the
acceptfunction, you're setting the current task in the store withstore.setCurrentTask(currentTask);. Confirm that this is consistent with how tasks are managed elsewhere in your application.
165-165: Review the necessity ofstore.setCurrentTask(null)after declining an incoming taskAfter declining a task, you're setting
store.setCurrentTask(null);. Verify if this is required or if the store already reflects the absence of a current task.packages/contact-center/task/src/index.ts (1)
3-4: AddedCallControlcomponent to exportsThe
CallControlcomponent is correctly imported and exported, making it accessible to other modules.packages/contact-center/cc-widgets/src/index.ts (2)
3-3: ImportedCallControlfrom@webex/cc-taskmoduleSuccessfully imported the
CallControlcomponent, ensuring it's available for use within the package.
6-6: ExportedCallControlcomponent fromcc-widgetspackageIncluding
CallControlin the exports makes it available to consumers of thecc-widgetspackage.packages/contact-center/task/src/CallControl/index.tsx (1)
12-12: Verify the spread operation safety.The spread operation combining hook results with wrapupCodes could potentially overwrite properties. Consider using a more explicit merge strategy.
✅ Verification successful
The spread operation is safe and appropriate.
The
useCallControlhook returns a fixed set of properties (currentTask,endCall,holdResume,pauseResumeRecording,wrapupCall,wrapupRequired) that do not conflict with thewrapupCodesproperty being added.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for potential property conflicts in useCallControl hook result ast-grep --pattern 'useCallControl($$$)'Length of output: 7751
Script:
#!/bin/bash # Find useCallControl hook implementation ast-grep --pattern 'const useCallControl = $$$' ast-grep --pattern 'function useCallControl($$$)' ast-grep --pattern 'export const useCallControl = $$$'Length of output: 7756
packages/contact-center/task/tests/helper.ts (2)
2-2: LGTM!The import statement for
useCallControlis correctly placed alongside other hook imports.
512-795: Well-structured test suite with comprehensive coverage!The test implementation follows testing best practices:
- Proper setup and teardown
- Comprehensive error handling
- Good use of async/await
- Clear test descriptions
packages/contact-center/task/src/CallControl/call-control.presentational.tsx
Outdated
Show resolved
Hide resolved
packages/contact-center/task/tests/CallControl/call-control.presentational.tsx
Outdated
Show resolved
Hide resolved
| </div> | ||
| )} | ||
| </div> | ||
| {!currentTask && ( |
There was a problem hiding this comment.
Check the sdk for task limit and hide accordingly
|
|
||
| const {currentTask, holdResume, pauseResumeRecording, endCall, wrapupCall, wrapupCodes, wrapupRequired} = props; | ||
| const handleHoldResume = () => { | ||
| if (isHeld) { |
There was a problem hiding this comment.
See if there is any event or state change from the sdk
There was a problem hiding this comment.
This if-else block can be one liner
toggleHold(!isHeld);
There was a problem hiding this comment.
See if on promise resolution if we get any data.
There was a problem hiding this comment.
We are not getting any data
| }); | ||
| }); | ||
| } | ||
| }; |
There was a problem hiding this comment.
const toggleHold = (hold: boolean) => {
if(!onHoldResume) return;
const logLocation = {
module: 'widget-cc-task#helper.ts',
method: 'useCallControl#holdResume',
};
if (hold) {
currentTask.hold()
.then(() => onHoldResume())
.catch((error: Error) => {
logger.error(`Error holding call: ${error}`, logLocation);
});
return;
}
currentTask.resume()
.then(() => onHoldResume())
.catch((error: Error) => {
logger.error(`Error resuming call: ${error}`, logLocation);
});
};
We can update the code as shown and follow similar pattern to keep the code clean
|
Also, did we have any discussion from accessibility perspective? |
There was a problem hiding this comment.
Thanks for the Vidcasts. I notice that the User State goes to available as soon as we end the call. Ideally, the user state is not supposed to go to Available until the agent has wrapped up the call. Can we check how does the Agent Desktop work today on this and see why that's not happening in our case?
The agent state should remain |
Since the UI Is still placeholder we haven't, but when we will design the UI we will take accessibility with it. |
|
Tested using chrome extension Screen.Recording.2025-01-28.at.6.00.04.PM.mov |
There was a problem hiding this comment.
Did we test dial number scenario also for call control? I was trying to test and not sure it is able to handle the call control widget display along with incomingTask widget appearing/disappearing.
Please check this flow too before merging.
Appriving the code as I won't be available tomorrow but please ensure to address these commenst as they are critici=al
| <button className="btn" onClick={handletoggleRecording} disabled={wrapupRequired}> | ||
| {isRecording ? 'Pause Recording' : 'Resume Recording'} | ||
| </button> | ||
| <button className="btn" onClick={endCall} disabled={wrapupRequired}> |
There was a problem hiding this comment.
This button shouldn't always be enabled. We have a feature flag isEndCallEnabled which is by default false. So only end from call control widget should be possible if this is set
There was a problem hiding this comment.
I want unable to find this flag in agentProfile and in the task object. Creating a task for this to be handled later https://jira-eng-gpk2.cisco.com/jira/browse/SPARK-609158
| return <></>; // hidden component | ||
| } | ||
|
|
||
| const callAssociationDetails = currentTask.data.interaction.callAssociatedDetails; | ||
| const callAssociationDetails = incomingTask.data.interaction.callAssociatedDetails; |
There was a problem hiding this comment.
I only see the audio element added here in the incoming task object but not in call control. How are we ensuring for browser case that remote audio is received and played? I observed one-way audio in my testing. Please check this before merging.
There was a problem hiding this comment.
Great catch! Ive moved the audio to call-control after discussing with Arun and Kesava, we are assuming if a dev needs to have audio of the call they need to use the call-control widget. If we get an ask that wants otherwise we can pick this up again
We did a testing for direct number, We were able to connect and control the call from the widgets. |
|
🎉 This PR is included in version 1.28.0-ccwidgets.12 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
# [1.28.0-ccwidgets.12](webex/widgets@v1.28.0-ccwidgets.11...v1.28.0-ccwidgets.12) (2025-01-29) ### Bug Fixes * **call-control:** add-call-control-widget ([webex#362](webex#362)) ([a677f5e](webex@a677f5e))
|
🎉 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))
THIS PR COMPLETES: https://jira-eng-gpk2.cisco.com/jira/browse/SPARK-589818
DESCRIPTION
Changes made
Tests
Vidcasts
Summary by CodeRabbit
Release Notes
New Features
Improvements
UI Updates
The release introduces comprehensive call control features, allowing users to manage calls more effectively with intuitive controls and wrap-up options.