fix the talk to human bugs #1307
Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
webplugin/js/app/components/typing-service.js(3 hunks)webplugin/js/app/kommunicate-ui.js(1 hunks)webplugin/js/app/mck-sidebox-1.0.js(5 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
webplugin/js/app/mck-sidebox-1.0.js (1)
webplugin/js/app/components/typing-service.js (1)
typingService(180-180)
🔇 Additional comments (9)
webplugin/js/app/mck-sidebox-1.0.js (6)
4539-4539: Properly initializing waiting queue state.The addition of
CURRENT_GROUP_DATA.isWaitingQueue = falsehere ensures the waiting queue state is properly reset when transitioning out of a waiting queue scenario.
5123-5123: Initializing waiting queue flag during conversation setup.Setting
isWaitingQueueto false during conversation initialization is appropriate to ensure consistent behavior across different entry points to conversation threads.
5610-5613: Correctly handling talk-to-human metadata.This addition properly detects when a message contains the
TALK_TO_HUMANmetadata and informs the typing service to adjust its behavior accordingly. This is crucial for the bug fix described in the PR objectives.
6867-6867: Consistent initialization of waiting queue state.Setting
isWaitingQueueto false during group data initialization ensures consistent behavior in the application.
7928-7928: Resetting talk-to-human flag at message processing start.This change effectively prevents the typing indicator state from being carried over between messages, which is important for ensuring the correct display of typing indicators.
5614-5616:❓ Verification inconclusive
Verify bot-specific typing indicator behavior.
Make sure this condition appropriately handles all edge cases, especially transitions between bot and human agents when "talk to human" is triggered.
🏁 Script executed:
#!/bin/bash # Check for any other conditions where typing indicators might be shown or hidden rg -A 5 -B 5 "showTypingIndicator" webplugin/js/Length of output: 4166
Action: Verify Bot Typing Indicator Behavior
After reviewing the code and the search results:
- The condition in
webplugin/js/app/mck-sidebox-1.0.js(lines 5614–5616) callstypingService.showTypingIndicator()only whenKommunicateUtils.isCurrentAssigneeBot()is true. This suggests that bot sessions are the target for displaying the typing indicator.- Within
webplugin/js/app/components/typing-service.js, theshowTypingIndicator()method already checks for cases where a "talk to human" action is in process (viathis.isKmTalkToHumanMsgorCURRENT_GROUP_DATA.isWaitingQueue) to prevent showing the indicator. This appears to help handle transitions between bot and human agents.- There are additional invocations of
typingService.showTypingIndicator()in the file. Please double-check that these do not conflict with the intended behavior during transitions and that there are no duplicated calls resulting in inconsistent UI states.Please verify manually (or via integration tests) that the logic properly handles all edge cases, especially when a user triggers the "talk to human" option and the session transitions from a bot to a human agent.
webplugin/js/app/components/typing-service.js (2)
12-12: Good addition of the isKmTalkToHumanMsg property.The new property is properly initialized as false in the constructor and will be used to control typing indicator visibility.
24-26: Well implemented setter method for isKmTalkToHumanMsg.This method follows a good pattern for modifying the property state.
webplugin/js/app/kommunicate-ui.js (1)
1578-1586: Good UI enhancement for waiting queue state.The modifications correctly show the "talk to human" and "restart conversation" buttons when in a waiting queue. Setting the
isWaitingQueueflag enables the typing indicator to be suppressed in this scenario as implemented in the typing-service.js file.One suggestion: consider adding a comment explaining the purpose of setting
CURRENT_GROUP_DATA.isWaitingQueue = trueto make the code more maintainable.
There was a problem hiding this comment.
Pull Request Overview
This PR aims to fix bugs in the talk-to-human interaction by adjusting state management for the waiting queue and updating the corresponding UI and typing indicator behavior.
- Removes redundant or misaligned resets of the waiting queue flag in the sidebox.
- Updates the UI to correctly mark the waiting queue state for talk-to-human interactions.
- Enhances the typing service logic to conditionally suppress the typing indicator when in talk-to-human mode.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| webplugin/js/app/mck-sidebox-1.0.js | Adjusts the CURRENT_GROUP_DATA.isWaitingQueue flag and toggles messaging based on state |
| webplugin/js/app/kommunicate-ui.js | Modifies UI elements and sets the waiting queue flag for talk-to-human options |
| webplugin/js/app/components/typing-service.js | Introduces and uses a talk-to-human flag to conditionally suppress the typing indicator |
Comments suppressed due to low confidence (3)
webplugin/js/app/mck-sidebox-1.0.js:4539
- [nitpick] The waiting queue flag is reset here; please ensure that this reset does not conflict with its expected state in other modules, and consider centralizing its management for clarity.
CURRENT_GROUP_DATA.isWaitingQueue = false;
webplugin/js/app/kommunicate-ui.js:1586
- [nitpick] Setting the waiting queue flag to true at this point should be reviewed alongside the resets in other parts of the code to avoid any state inconsistencies during the talk-to-human workflow.
CURRENT_GROUP_DATA.isWaitingQueue = true;
webplugin/js/app/components/typing-service.js:64
- [nitpick] Verify that using an OR condition for both the talk-to-human flag and the waiting queue state correctly reflects the intended behavior for suppressing the typing indicator.
if (this.isKmTalkToHumanMsg || CURRENT_GROUP_DATA.isWaitingQueue) return; // don't show loader when user clicking on the talk to human button
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
webplugin/js/app/kommunicate-client.js (1)
68-68: Initialization added for waiting queue stateThis line initializes the waiting queue state to false when creating a conversation, which is a good addition to ensure consistent initial state for the "talk to human" functionality.
Consider improving the code in two ways:
- Add a more descriptive comment explaining the purpose (e.g., "Initialize waiting queue state to prevent showing loading indicator initially")
- Add a safety check to ensure CURRENT_GROUP_DATA exists before setting the property
- CURRENT_GROUP_DATA.isWaitingQueue = false; // for showing the loader in screen + // Initialize waiting queue state to prevent showing loading indicator initially + if (CURRENT_GROUP_DATA) { + CURRENT_GROUP_DATA.isWaitingQueue = false; + }webplugin/js/app/mck-sidebox-1.0.js (2)
5608-5614: Set talk to human message flag when message contains TALK_TO_HUMAN metadataThis code correctly identifies when a "Talk to Human" request is made by checking for the 'TALK_TO_HUMAN' property in the message metadata and updates the typing service accordingly.
However, I recommend checking if
messagePxy.metadataexists before accessing its properties to prevent potential errors.-if (messagePxy.metadata.hasOwnProperty('TALK_TO_HUMAN')) { +if (messagePxy.metadata && messagePxy.metadata.hasOwnProperty('TALK_TO_HUMAN')) { typingService.setTalkToHumanMsg(true); }
7173-7184: Prevent CTA setup when contact list is presentGood addition that prevents setting up the primary CTA when the contact list is present, avoiding potential UI conflicts.
The commented-out code on line 7184 should either be removed or uncommented if it's needed.
-// kommunicateCommons.modifyClassList({ id: ['km-header-cta'] }, '', 'n-vis');
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (4)
webplugin/js/app/components/typing-service.js(3 hunks)webplugin/js/app/kommunicate-client.js(1 hunks)webplugin/js/app/kommunicate-ui.js(1 hunks)webplugin/js/app/mck-sidebox-1.0.js(7 hunks)
🧰 Additional context used
🧠 Learnings (1)
webplugin/js/app/components/typing-service.js (1)
Learnt from: panwaranuj01
PR: Kommunicate-io/Kommunicate-Web-SDK#1307
File: webplugin/js/app/components/typing-service.js:64-65
Timestamp: 2025-03-28T12:35:47.893Z
Learning: In the Kommunicate-Web-SDK codebase, CURRENT_GROUP_DATA is guaranteed to be present when the showTypingIndicator method in typing-service.js is called, so null checks are not required when accessing its properties.
🧬 Code Definitions (1)
webplugin/js/app/mck-sidebox-1.0.js (1)
webplugin/js/app/components/typing-service.js (1)
typingService(180-180)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Chat-Widget-Test (km-dev-434106)
🔇 Additional comments (9)
webplugin/js/app/mck-sidebox-1.0.js (5)
671-675: Conditionally hide the "Talk to Human" button when no group feeds existGood addition that prevents the "Talk to Human" button from displaying when there are no group feeds. The use of optional chaining helps prevent errors if
datais undefined.
4537-4540: Reset waiting queue state when conversation is transferredThis change correctly resets the waiting queue state when a conversation is transferred. This aligns with initialization logic in other parts of the code.
5121-5124: Initialize waiting queue stateProper initialization of the waiting queue state, which helps prevent issues with typing indicators and UI elements that depend on this flag.
6865-6868: Initialize waiting queue state during group creationProperly initializes the waiting queue state when setting up group data, ensuring consistent state management.
7928-7931: Reset talk to human message flag when processing new messagesThe addition correctly resets the "Talk to Human" message flag at the beginning of what appears to be a message processing function, ensuring proper state management.
webplugin/js/app/kommunicate-ui.js (1)
1578-1586: Good implementation for handling waiting queue UI elements.The modifications to show the "Talk to Human" and "Restart Conversation" buttons when in waiting queue state are well-implemented. Setting
CURRENT_GROUP_DATA.isWaitingQueue = trueprovides a useful flag that other components can use to adjust their behavior accordingly.webplugin/js/app/components/typing-service.js (3)
12-12: Properly initialized the new flag for talk to human message state.The new property initializes correctly with a default value, following the existing pattern in the class constructor.
24-26: Well-implemented setter method for the new flag.The arrow function syntax is appropriate here as it preserves the
thiscontext, and the method follows a clean, simple implementation pattern.
64-65: Effectively prevents typing indicator in talk-to-human scenarios.This condition correctly prevents the typing indicator from displaying when either:
- The user is clicking on the talk to human button (
isKmTalkToHumanMsgis true)- The conversation is in waiting queue state (
CURRENT_GROUP_DATA.isWaitingQueueis true)The comment clarifies the purpose of this check, which is helpful for future maintenance.
What do you want to achieve?
PR Checklist
How was the code tested?
What new thing you came across while writing this code?
In case you fixed a bug then please describe the root cause of it?
Screenshot
NOTE: Make sure you're comparing your branch with the correct base branch
Summary by CodeRabbit