feat(widgets): added-blind-transfer #422
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 📝 WalkthroughWalkthroughThis PR introduces a new boolean property Possibly related PRs
🪧 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
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
![coderabbitai[bot]](/https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (16)
packages/contact-center/task/src/CallControl/CallControlCustomComponents/call-control-popover.presentational.tsx (3)
19-19: Consider renaming the variable or implementing actual filteringThe variable name
filteredAgentssuggests that some filtering is being applied, but it's directly assigned frombuddyAgentswithout any filtering logic. Either rename it to something likedisplayedAgentsor implement actual filtering if that's the intended functionality.- const filteredAgents = buddyAgents; + const displayedAgents = buddyAgents;
37-52: Consider displaying a loading state when fetching agentsThere's currently no handling for the loading state when fetching buddy agents. Users might benefit from seeing a loading indicator while agents are being loaded.
Consider adding a loading state to improve user experience:
- <ListNext listSize={filteredAgents.length} className="agent-agent-list"> - {filteredAgents.map((agent) => ( + {isLoading ? ( + <div className="agent-loading"> + <Text tagName="small" type="body-secondary"> + Loading agents... + </Text> + </div> + ) : ( + <ListNext listSize={displayedAgents.length} className="agent-agent-list"> + {displayedAgents.map((agent) => (
33-35: The Tab system appears to be over-engineered for a single tabThe component is using a TabList component with only one tab option. If there are no plans to add more tabs in the future, consider simplifying this to a plain header.
If no additional tabs are planned, consider simplifying:
- <TabListNext - aria-label="Tabs" - className="agent-tablist" - hasBackground={false} - style={{marginTop: '0'}} - onTabSelection={(key) => setSelectedTab(key as string)} - > - <TabNext key="Agents" active={selectedTab === 'Agents'}> - Agents - </TabNext> - </TabListNext> + <div className="agent-tablist" style={{marginTop: '0'}}> + <Text tagName="h4" type="body-medium-bold"> + Agents + </Text> + </div>packages/contact-center/store/src/store.types.ts (3)
44-44: Add documentation for the newisAgentTransferredpropertyThe new property lacks JSDoc comments to explain its purpose and usage. Adding documentation will help other developers understand when and how to use this property.
Add appropriate documentation:
+ /** + * Flag indicating whether the agent has been transferred. + */ isAgentTransferred: boolean;
70-70: Add documentation for the newsetIsAgentTransferredmethodThis method lacks JSDoc comments to explain its purpose. Adding documentation will improve code maintainability.
Add appropriate documentation:
+ /** + * Sets the agent transfer status. + * @param value - Boolean indicating whether the agent has been transferred. + */ setIsAgentTransferred(value: boolean): void;
97-97: Add documentation for the newAGENT_BLIND_TRANSFERREDenum valueThe new enum value lacks an explanatory comment. Adding a brief comment will clarify its purpose.
Add a comment for the new enum value:
+ // Event triggered when an agent is blind transferred AGENT_BLIND_TRANSFERRED = 'AgentBlindTransferred',packages/contact-center/task/tests/CallControl/call-control-list-item.presentational.tsx (1)
31-37: Be more specific about which console errors to suppressCurrently, all console errors are being suppressed, which could hide actual issues during testing. It would be better to specifically mock only the expected errors.
Consider a more targeted approach to error suppression:
beforeAll(() => { - jest.spyOn(console, 'error').mockImplementation(() => {}); + // Only mock specific React-related warnings that are expected + const originalError = console.error; + jest.spyOn(console, 'error').mockImplementation((message, ...args) => { + // Filter out specific expected warnings + if (message.includes('Warning: ReactDOM.render is no longer supported') || + message.includes('Warning: Failed prop type')) { + return; + } + // Log other errors normally + originalError(message, ...args); + }); });packages/contact-center/task/src/CallControl/call-control.presentational.tsx (2)
153-156: Consider adding a loading state when fetching buddy agents.The code calls
loadBuddyAgents()when the popover opens, but doesn't provide any visual feedback to the user while the agents are being loaded.Add a loading state:
const [showAgentMenu, setShowAgentMenu] = useState(false); const [agentMenuType, setAgentMenuType] = useState<'Consult' | 'Transfer' | null>(null); +const [isLoadingAgents, setIsLoadingAgents] = useState(false); // ... onPress={() => { // If popover is already visible, we close it if (showAgentMenu && agentMenuType === button.menuType) { setShowAgentMenu(false); setAgentMenuType(null); } else { setAgentMenuType(button.menuType as 'Consult' | 'Transfer'); setShowAgentMenu(true); + setIsLoadingAgents(true); loadBuddyAgents() + .finally(() => setIsLoadingAgents(false)); } }}Then in the
CallControlPopoverPresentationalcomponent, pass the loading state:<CallControlPopoverPresentational heading={button.menuType} buttonIcon={button.icon} buddyAgents={buddyAgents} + isLoading={isLoadingAgents} onAgentSelect={(agentId) => { // ... }} />
173-188: Add empty state handling for buddy agents list.The code doesn't handle the case when
buddyAgentsis empty or failed to load. Providing feedback to the user in these scenarios would improve the user experience.Consider adding an empty state message:
{showAgentMenu && agentMenuType === button.menuType ? ( <CallControlPopoverPresentational heading={button.menuType} buttonIcon={button.icon} buddyAgents={buddyAgents} + emptyStateMessage="No available agents found" onAgentSelect={(agentId) => { setShowAgentMenu(false); setAgentMenuType(null); if (agentMenuType === 'Consult') { consultCall(); } else { transferCall(agentId, 'agent'); } }} /> ) : null}packages/contact-center/task/src/CallControl/call-control.styles.scss (2)
151-160: Add keyboard focus styles for accessibility.The hover button styles only address mouse interactions. For better accessibility, add equivalent focus styles for keyboard navigation.
.call-control-list-item .hover-button { opacity: 0; transition: opacity 0.3s; pointer-events: none; } .call-control-list-item:hover .hover-button { opacity: 1; pointer-events: auto; } + +.call-control-list-item .hover-button:focus-visible { + opacity: 1; + pointer-events: auto; + outline: 2px solid var(--mds-color-theme-outline-focus-normal); +}
162-165: Consider more descriptive class name.The class name
.custom-textis very generic and might clash with other styles. Consider using a more specific name related to its usage context.-.custom-text { +.agent-popover-description-text { margin: 0 !important; line-height: 1.2 !important; }packages/contact-center/store/src/storeEventsWrapper.ts (2)
246-249: Improve handling of unused parameter.Instead of using an eslint-disable comment for the unused parameter, it's better to follow the JavaScript convention of prefixing unused parameters with an underscore.
-// eslint-disable-next-line @typescript-eslint/no-unused-vars -handleAgentBlindTransferred = (event) => { +handleAgentBlindTransferred = (_event) => { this.setIsAgentTransferred(true); };
441-455: Well-implemented getBuddyAgents method with error handling.The method correctly fetches buddy agents with proper error handling and logging. The implementation follows the pattern of other methods in the file.
Consider enhancing the method to support more filtering options:
-getBuddyAgents = async (): Promise<Array<BuddyDetails>> => { +getBuddyAgents = async ( + options: { mediaType?: string; state?: string } = { mediaType: 'telephony', state: 'Available' } +): Promise<Array<BuddyDetails>> => { try { const response = await this.store.cc.getBuddyAgents({ - mediaType: 'telephony', - state: 'Available', + mediaType: options.mediaType || 'telephony', + state: options.state || 'Available', }); return response?.data?.agentList || []; } catch (error) { this.store.logger.error(`Error fetching buddy agents: ${error}`, { module: 'cc-store#storeEventsWrapper.ts', method: 'getBuddyAgents', }); return []; } };packages/contact-center/task/tests/CallControl/call-control.presentational.tsx (3)
43-53: Well structured mock for CallControlPopoverPresentational.The mock implementation includes a test button that triggers the onAgentSelect callback, which allows for testing agent selection functionality.
Consider using a dynamic agent ID instead of hardcoding 'agent1' to make the tests more flexible and maintainable:
- <button data-testid="AgentSelectButton" onClick={() => props.onAgentSelect('agent1')}> + <button data-testid="AgentSelectButton" onClick={() => props.onAgentSelect(props.buddyAgents?.[0]?.id || 'agent1')}>
105-110: Targeted test for hold button icon.Good test that verifies the correct icon is displayed when the call is not on hold.
Consider adding a test for when the call is on hold as well:
it('renders the correct hold button icon based on currentTask (on hold)', () => { const onHoldProps = { ...baseProps, currentTask: { ...baseProps.currentTask, data: { ...baseProps.currentTask.data, interaction: { ...baseProps.currentTask.data.interaction, media: { '1': { isHold: true }, }, }, }, }, }; render(<CallControlPresentational {...onHoldProps} />); const holdButton = screen.getAllByTestId('ButtonCircle')[0]; expect(holdButton).toHaveTextContent('play-bold'); });
55-144: Consider adding tests for recording functionality.I notice you have mock functions for toggleRecording and setIsRecording, but no tests that specifically verify this functionality. Consider adding a test to ensure recording can be toggled correctly:
it('calls toggleRecording when recording button is clicked', () => { render(<CallControlPresentational {...baseProps} />); // Assuming the recording button is the 1st or 5th button in the array // Adjust index based on actual button order const buttons = screen.getAllByTestId('ButtonCircle'); fireEvent.click(buttons[1]); // Adjust index as needed expect(mockToggleRecording).toHaveBeenCalled(); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
packages/contact-center/store/src/store.ts(1 hunks)packages/contact-center/store/src/store.types.ts(5 hunks)packages/contact-center/store/src/storeEventsWrapper.ts(8 hunks)packages/contact-center/task/src/CallControl/CallControlCustomComponents/call-control-list-item.presentational.tsx(1 hunks)packages/contact-center/task/src/CallControl/CallControlCustomComponents/call-control-popover.presentational.tsx(1 hunks)packages/contact-center/task/src/CallControl/call-control.presentational.tsx(4 hunks)packages/contact-center/task/src/CallControl/call-control.styles.scss(2 hunks)packages/contact-center/task/src/helper.ts(4 hunks)packages/contact-center/task/src/task.types.ts(3 hunks)packages/contact-center/task/tests/CallControl/call-control-list-item.presentational.tsx(1 hunks)packages/contact-center/task/tests/CallControl/call-control-popover.presentational.tsx(1 hunks)packages/contact-center/task/tests/CallControl/call-control.presentational.tsx(3 hunks)packages/contact-center/task/tests/helper.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
packages/contact-center/task/tests/CallControl/call-control-popover.presentational.tsx (1)
Learnt from: Shreyas281299
PR: webex/widgets#362
File: packages/contact-center/task/tests/CallControl/call-control.presentational.tsx:0-0
Timestamp: 2025-03-20T09:47:38.389Z
Learning: UI testing is not mandatory for React components in the webex/widgets repository.
packages/contact-center/task/tests/CallControl/call-control-list-item.presentational.tsx (1)
Learnt from: Shreyas281299
PR: webex/widgets#362
File: packages/contact-center/task/tests/CallControl/call-control.presentational.tsx:0-0
Timestamp: 2025-03-20T09:47:38.389Z
Learning: UI testing is not mandatory for React components in the webex/widgets repository.
🧬 Code Definitions (2)
packages/contact-center/task/src/task.types.ts (1)
packages/contact-center/store/src/store.types.ts (1)
BuddyDetails(135-135)
packages/contact-center/task/src/CallControl/CallControlCustomComponents/call-control-popover.presentational.tsx (1)
packages/contact-center/store/src/storeEventsWrapper.ts (1)
agentId(44-46)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: AWS Amplify Console Web Preview
🔇 Additional comments (32)
packages/contact-center/store/src/store.ts (1)
38-38: Good implementation of the isAgentTransferred property!This property addition supports the blind transfer functionality as specified in the PR objectives. The naming is clear and follows the same conventions as other boolean state flags in the Store class.
packages/contact-center/task/tests/CallControl/call-control-popover.presentational.tsx (1)
1-74: Well-structured test suite for the CallControlPopoverPresentational component!The tests thoroughly verify rendering and interaction behaviors, including heading display, agent list rendering, empty state handling, and agent selection functionality. The mocks are set up appropriately to isolate component testing.
Note from learnings: I acknowledge that UI testing is not mandatory for React components in this repository, but these tests add good coverage to the new functionality.
packages/contact-center/task/src/CallControl/CallControlCustomComponents/call-control-list-item.presentational.tsx (1)
1-59: Clean implementation of the call control list item component!This component is well-designed with:
- Clear props interface
- Good use of Momentum UI components
- Smart handling of avatar initials
- Consistent styling approach
- Proper separation of concerns
The implementation supports the blind transfer functionality by providing a reusable list item that can display agent information with action buttons.
packages/contact-center/task/tests/helper.ts (2)
1087-1108: Good test coverage for buddy agent loading functionality!The test properly verifies that the
loadBuddyAgentsmethod correctly fetches and stores buddy agents. The use of mocks and assertions is appropriate for validating this functionality.
1110-1130: Well-implemented test for the transferCall method!This test properly verifies that the
transferCallmethod correctly calls the task's transfer function with the expected parameters. The test confirms essential functionality for the blind transfer feature.packages/contact-center/task/tests/CallControl/call-control-list-item.presentational.tsx (2)
53-59: LGTM! Good test coverage for the component renderingThe test effectively verifies that the component renders with the correct content and properly displays user initials.
69-74: LGTM! Good interaction testingThe test properly verifies that clicking the button triggers the callback function.
packages/contact-center/task/src/task.types.ts (2)
201-221: Well-documented new properties and methods!The new properties and methods for buddy agents and call transfers have clear and comprehensive JSDoc comments, which is excellent for code maintainability.
240-243: LGTM! Properly updated type exportsThe
CallControlPresentationalPropstype has been correctly updated to include the new properties.packages/contact-center/task/src/CallControl/call-control.presentational.tsx (4)
8-8: Good import of the new component.The addition of
CallControlPopoverPresentationalenhances the component architecture by separating the popover UI logic into its own component.
13-14: State variables appropriately manage popover visibility and type.These state variables cleanly control the display and behavior of the agent menu, supporting both Consult and Transfer operations.
29-32: New props enhance component functionality.The addition of
buddyAgents,loadBuddyAgents,transferCall, andconsultCallprops properly enables the new agent selection and transfer features.
89-102: Button configurations properly define new call features.The new Consult and Transfer buttons are well-configured with appropriate icons, tooltips, and menuType properties.
packages/contact-center/task/src/helper.ts (3)
133-142: Well-implemented buddy agents loading with error handling.The implementation for loading buddy agents is clean, with proper error handling and state management. The use of
useCallbackis appropriate to prevent unnecessary function recreations.
272-283: Good implementation of transferCall with error handling.The transfer call implementation is solid with proper error handling. The function is well-structured and follows the pattern of other methods in this file.
301-304: Properly exposed new functionality in hook return.The new functions are correctly added to the hook's return object, making them available to components.
packages/contact-center/task/src/CallControl/call-control.styles.scss (1)
118-149: Well-structured styles for the agent popover.The styles for the agent popover are well-organized, following the existing style patterns in the file. The CSS nesting and naming conventions are consistent.
packages/contact-center/store/src/storeEventsWrapper.ts (5)
88-90: Good implementation of isAgentTransferred getter.The getter correctly accesses the underlying store state.
139-141: Well-implemented setter for isAgentTransferred.The setter follows the same pattern as other setters in the file.
256-256: Good condition update to consider transferred state.Adding the check for
this.store.isAgentTransferredensures proper handling of transferred calls.
279-279: Appropriately reset transfer state on wrap-up.Resetting
isAgentTransferredto false on task wrap-up ensures clean state management.
302-303: Good implementation of event listeners for blind transfers.The event listeners are correctly set up for the new blind transfer functionality, following the same pattern as other event listeners in the file.
Also applies to: 338-339
packages/contact-center/task/tests/CallControl/call-control.presentational.tsx (10)
3-3: Good addition of fireEvent for interaction testing.Adding the fireEvent import enables you to test user interactions like button clicks, which is essential for properly testing the component's behavior.
7-32: Well implemented mock components with interactive functionality.The enhanced mock implementations for ButtonPill, ButtonCircle, PopoverNext, SelectNext, and TooltipNext now properly handle props like onPress, disabled, and className. This allows for more realistic testing of component interactions.
35-41: Improved Icon mock with className support.The updated Icon mock now handles the className prop, which provides better testing for styling conditions.
64-67: Good addition of mock functions for new features.The new mock functions for buddy agents and call operations are essential for testing the new transfer and consult functionality.
69-94: Well structured baseProps with all required component properties.Organizing the props in a baseProps object makes the tests more maintainable and reduces duplication. All necessary props for the component are included.
100-104: Simple test for component rendering.This test ensures the basic component renders correctly.
111-117: Good interaction test for the hold button.This test verifies that the toggleHold function is called with the correct value when the hold button is clicked.
118-124: Good interaction test for the end call button.This test verifies that the endCall function is called when the end call button is clicked.
125-134: Well structured test for agent consultation.This test properly verifies the consultation flow by clicking the consult button and then selecting an agent.
135-143: Comprehensive test for agent transfer.This test correctly verifies that transferCall is called with the right parameters when an agent is selected for transfer.
packages/contact-center/task/src/CallControl/call-control.presentational.tsx
Show resolved
Hide resolved
| @@ -35,6 +35,7 @@ class Store implements IStore { | |||
| lastStateChangeTimestamp?: number; | |||
| lastIdleCodeChangeTimestamp?: number; | |||
| showMultipleLoginAlert: boolean = false; | |||
| isAgentTransferred: boolean = false; | |||
There was a problem hiding this comment.
State so we can check when the agent has been transferred.
| @@ -93,6 +94,7 @@ enum TASK_EVENTS { | |||
| CONTACT_RECORDING_PAUSED = 'ContactRecordingPaused', | |||
| CONTACT_RECORDING_RESUMED = 'ContactRecordingResumed', | |||
| AGENT_WRAPPEDUP = 'AgentWrappedUp', | |||
| AGENT_BLIND_TRANSFERRED = 'AgentBlindTransferred', | |||
There was a problem hiding this comment.
As discussed, we will update above state on this event.
| @@ -10,6 +10,7 @@ import { | |||
| IdleCode, | |||
| IContactCenter, | |||
| ITask, | |||
| BuddyDetails, | |||
There was a problem hiding this comment.
We need this for the budyd agents details.
| @@ -418,6 +437,22 @@ class StoreWrapper implements IStoreWrapper { | |||
| }); | |||
| }); | |||
| }; | |||
|
|
|||
| getBuddyAgents = async (): Promise<Array<BuddyDetails>> => { | |||
There was a problem hiding this comment.
Method so we can extract buddy agents - queues will also be similar to this.
| import {Icon} from '@momentum-design/components/dist/react'; | ||
| import classnames from 'classnames'; | ||
|
|
||
| export interface CallControlListItemPresentationalProps { |
There was a problem hiding this comment.
This entire class is for the custom list item, right now all it consists of is an avatar, title, subtitle and a button which are all highly customisable.
Open to any suggestions to make it better.
| @@ -130,6 +130,17 @@ export const useCallControl = (props: useCallControlProps) => { | |||
| const [isHeld, setIsHeld] = useState<boolean | undefined>(undefined); | |||
| const [isRecording, setIsRecording] = useState(true); | |||
|
|
|||
| const [buddyAgents, setBuddyAgents] = useState<BuddyDetails[]>([]); | |||
There was a problem hiding this comment.
Code to extract buddy agents, used logger to log any errors here.
| }; | ||
|
|
||
| // New consult callback method (empty for now) | ||
| const consultCall = async () => { |
There was a problem hiding this comment.
Temporary, will add in the next PR.
| import '@testing-library/jest-dom'; | ||
| import CallControlListItemPresentational from '../../src/CallControl/CallControlCustomComponents/call-control-list-item.presentational'; | ||
|
|
||
| jest.mock('@momentum-ui/react-collaboration', () => ({ |
There was a problem hiding this comment.
Added tests here to get 100% coverage.
| import '@testing-library/jest-dom'; | ||
| import CallControlPopoverPresentational from '../../src/CallControl/CallControlCustomComponents/call-control-popover.presentational'; | ||
|
|
||
| jest.mock('../../src/CallControl/CallControlCustomComponents/call-control-list-item.presentational', () => { |
| import React from 'react'; | ||
| import {render, screen} from '@testing-library/react'; | ||
| import {render, screen, fireEvent} from '@testing-library/react'; |
There was a problem hiding this comment.
Here coverage is still an issue, added tests for cases like hold/end etc. which were missed before, however testing the popover is an issue - some rendering problems for the test. WIP, but because of that coverage is a bit low.
bhabalan
changed the title
adhmenon
changed the title
adhmenon
changed the title
packages/contact-center/task/src/CallControl/call-control.presentational.tsx
Show resolved
Hide resolved
| if (agentMenuType === 'Consult') { | ||
| consultCall(); | ||
| } else { | ||
| transferCall(agentId, 'agent'); |
There was a problem hiding this comment.
We should have second parameter being picked up based on selected type right. Also these buttons should appear as radio buttons and option to select them. Queue is not implemented yet but dial number can be added right as one of the menu options
There was a problem hiding this comment.
Figma doesn't seem to have dial number... we only have "Agents" and "Queues" as of now.
Also, yes this method will change once I have consult implemented and also once I add queues.
| .agent-popover-content { | ||
| display: flex; | ||
| flex-direction: column; | ||
| padding: 8px; |
There was a problem hiding this comment.
Can these be not converted to rem? 1 rem is 16 px so we can convert the values
| @@ -286,6 +299,9 @@ class StoreWrapper implements IStoreWrapper { | |||
|
|
|||
| task.on(TASK_EVENTS.AGENT_WRAPPEDUP, this.handleTaskWrapUp); | |||
|
|
|||
| // Listen for AGENT_BLIND_TRANSFERRED event | |||
| task.on(TASK_EVENTS.AGENT_BLIND_TRANSFERRED, () => this.handleAgentBlindTransferred(task)); | |||
There was a problem hiding this comment.
Why are we listening for this event? I don't see SDK emitting any event for trasnfer. Complete dependency is there on promise resolve for transfer
There was a problem hiding this comment.
Edit:
I did some testing... and it was working with just promise based... I was in the wrong here.. thought it might cause some async issues and that end call would arrive before we set the state in store...
| mediaType: 'telephony', | ||
| state: 'Available', | ||
| }); | ||
| return response?.data?.agentList || []; |
There was a problem hiding this comment.
Why do we have ? after response? In case of success, response will always be there and have data right. And even agentList is mandatory field but it can be empty array sometime.
And failure should go to catch block
| }; | ||
|
|
||
| try { | ||
| await currentTask.transfer(transferPayload); |
There was a problem hiding this comment.
Why are we not using the response here that we receive as part of promise resolve?
There was a problem hiding this comment.
I think we don't need to use the response here, not sure how helpful it would be also.
| * @param destination - The destination to transfer the call to. | ||
| * @param destinationType - The type of destination. | ||
| */ | ||
| transferCall: (destination: string, destinationType: string) => void; |
There was a problem hiding this comment.
We have a type in SDK for destinationType so instead of keeping it as string why don't we use that enum type
| <TooltipNext | ||
| key={index} | ||
| triggerComponent={ | ||
| <ButtonCircle className={button.className} onPress={button.onClick} disabled={button.disabled}> |
There was a problem hiding this comment.
We should add aria-label to all the Button or icons we have for accessibility purpose
|
Closing this PR, opened a new one #423 |
COMPLETES SPARK-568988
Description
Vidcast - https://app.vidcast.io/share/24962b05-3a16-4b60-8c83-9333efa121f3?playerMode=vidcast
Considerations
Tests