fixed speech to text issues #1460
Conversation
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
WalkthroughAdds explicit TTS cancellation calls in UI handlers and introduces tokenization-based gating and message cleaning in the media service so speech is only played when it matches the current tokenization setting. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.41.0)webplugin/js/app/mck-sidebox-1.0.jsThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
🤖 Fix all issues with AI agents
In `@webplugin/js/app/media/media-service.js`:
- Around line 122-125: The TTS gating reads CURRENT_GROUP_DATA.TOKENIZE_RESPONSE
which can be stale during conversation switches; update
voiceOutputIncomingMessage to validate the incoming message's groupId against
the current active tab/group before using CURRENT_GROUP_DATA.TOKENIZE_RESPONSE
(or defer processing until the bot-details callback that sets TOKENIZE_RESPONSE
completes). Specifically, in voiceOutputIncomingMessage ensure you check
message.groupId (or incomingMessage.groupId) equals
CURRENT_GROUP_DATA.tabId/current group identifier and skip or queue the message
if they differ, or block handling until the bot details fetch callback has set
CURRENT_GROUP_DATA.TOKENIZE_RESPONSE so the setting cannot leak from the prior
conversation.
🧹 Nitpick comments (3)
webplugin/js/app/media/media-service.js (2)
127-133: The!messageguard on line 128 is redundant.
messageis already dereferenced on line 118 (Kommunicate.visibleMessage(message)), so if it were nullish the function would have thrown before reaching this point. The check isn't harmful but adds unnecessary noise.Simplify the null check
if (isTokenizedResponseEnabled) { - if (!message || !message.tokenMessage) { + if (!message.tokenMessage) { return; }
102-201: NospeechSynthesis.cancel()before queuing a new utterance in this function.The PR objective states that speech continues when the widget is closed or a new conversation starts. The related
stopVoiceOutput()inmck-sidebox-1.0.jsaddresses widget close/minimize, but within this function itself, new utterances are simply appended to the synthesis queue (line 198). If multiple tokenized messages arrive in quick succession (common with streaming/generative responses), they will stack up. Consider callingspeechSynth.cancel()beforespeechSynth.speak(utterance)to ensure only the latest message is spoken, or confirm that the queuing behavior is intentional.webplugin/js/app/mck-sidebox-1.0.js (1)
4514-4518: DuplicatestopVoiceOutput()definition — consider consolidating to avoid DRY violation.This function is defined identically at lines 3137–3141 and again at lines 4514–4518 in different parent scopes. While both definitions work correctly, consider extracting this to a module-level utility function or a shared helper object accessible from both locations to eliminate duplication and improve maintainability.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
webplugin/js/app/media/media-service.js (3)
120-133: Redundant null check onmessageat line 128.
messageis already dereferenced at line 118 (Kommunicate.visibleMessage(message)), so if it were nullish the function would have thrown before reaching line 128. The!messageguard is unnecessary.Suggested fix
if (isTokenizedResponseEnabled) { - if (!message || !message.tokenMessage) { + if (!message.tokenMessage) { return; } - } else if (message && message.tokenMessage) { + } else if (message.tokenMessage) { return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webplugin/js/app/media/media-service.js` around lines 120 - 133, Redundant null check: remove the unnecessary "!message" guard in the tokenized-response branch and simplify both branches to rely only on message.tokenMessage (since Kommunicate.visibleMessage(message) already dereferences message); update the condition in the isTokenizedResponseEnabled block to check only message.tokenMessage, and likewise simplify the else branch to return when message.tokenMessage is truthy—references: CURRENT_GROUP_DATA, TOKENIZE_RESPONSE, message, and Kommunicate.visibleMessage.
161-162:constis inconsistent withvarused throughout this function.Every other variable in
voiceOutputIncomingMessage(and the entire file) usesvar. Mixingconsthere is a style inconsistency—usevarto match, or migrate the whole function in a separate commit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webplugin/js/app/media/media-service.js` around lines 161 - 162, In function voiceOutputIncomingMessage replace the inconsistent declaration "const messageText = ..." with "var messageText = ..." so it matches the existing var style used throughout the function and file (update the line that currently reads const messageText = cleanMessage(JSON.parse(JSON.stringify(message)))?.message; and ensure subsequent uses of messageText and textToSpeak remain unchanged).
134-139: SimplifycleanMessageto a pure function.The
cleanMessagefunction both mutates its argument and returns it. Since the call site already deep-clones before passing it in (JSON.parse(JSON.stringify(message))), consider simplifying it to operate on a string directly:Suggested simplification
- function cleanMessage(messageObj) { - if (!messageObj?.message) return messageObj; - - messageObj.message = messageObj.message.replace(/\*\*(.*?)\*\*/g, '$1'); - return messageObj; - } + function cleanMessage(text) { + return text ? text.replace(/\*\*(.*?)\*\*/g, '$1') : ''; + }Then at the usage site:
- const messageText = cleanMessage(JSON.parse(JSON.stringify(message)))?.message; - textToSpeak += messageText; + textToSpeak += cleanMessage(message.message) || '';This eliminates the deep clone, the mutation, and the optional chaining.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webplugin/js/app/media/media-service.js` around lines 134 - 139, cleanMessage currently mutates its input object and uses optional chaining; change it to a pure function that accepts a string and returns a cleaned string (e.g., cleanMessage(messageText) -> cleanedText) instead of mutating messageObj.message, remove the optional chaining and cloning inside it, then update the call site that does JSON.parse(JSON.stringify(message)) to stop deep-cloning and instead call message.message = cleanMessage(message.message) (or assign the returned string into the message object after calling cleanMessage), ensuring no in-place mutation or unnecessary cloning remains.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@webplugin/js/app/media/media-service.js`:
- Around line 120-133: Redundant null check: remove the unnecessary "!message"
guard in the tokenized-response branch and simplify both branches to rely only
on message.tokenMessage (since Kommunicate.visibleMessage(message) already
dereferences message); update the condition in the isTokenizedResponseEnabled
block to check only message.tokenMessage, and likewise simplify the else branch
to return when message.tokenMessage is truthy—references: CURRENT_GROUP_DATA,
TOKENIZE_RESPONSE, message, and Kommunicate.visibleMessage.
- Around line 161-162: In function voiceOutputIncomingMessage replace the
inconsistent declaration "const messageText = ..." with "var messageText = ..."
so it matches the existing var style used throughout the function and file
(update the line that currently reads const messageText =
cleanMessage(JSON.parse(JSON.stringify(message)))?.message; and ensure
subsequent uses of messageText and textToSpeak remain unchanged).
- Around line 134-139: cleanMessage currently mutates its input object and uses
optional chaining; change it to a pure function that accepts a string and
returns a cleaned string (e.g., cleanMessage(messageText) -> cleanedText)
instead of mutating messageObj.message, remove the optional chaining and cloning
inside it, then update the call site that does
JSON.parse(JSON.stringify(message)) to stop deep-cloning and instead call
message.message = cleanMessage(message.message) (or assign the returned string
into the message object after calling cleanMessage), ensuring no in-place
mutation or unnecessary cloning remains.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
webplugin/js/app/media/media-service.js (1)
134-139: Avoid deep-cloning the whole message on the TTS path.
JSON.parse(JSON.stringify(message))copies the entire payload on every spoken default message, but onlymessage.messageis needed. Cleaning the string directly is cheaper and keeps this hot path simpler.Suggested refactor
- function cleanMessage(messageObj) { - if (!messageObj?.message) return messageObj; - - messageObj.message = messageObj.message.replace(/\*\*(.*?)\*\*/g, '$1'); - return messageObj; + function cleanMessageText(text) { + return typeof text === 'string' + ? text.replace(/\*\*(.*?)\*\*/g, '$1') + : ''; } ... - const messageText = cleanMessage(JSON.parse(JSON.stringify(message)))?.message; + var messageText = cleanMessageText(message.message); textToSpeak += messageText;Also applies to: 161-162
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webplugin/js/app/media/media-service.js` around lines 134 - 139, The TTS hot path is doing a deep clone of the whole message but only needs to sanitize message.message; update the cleanMessage function to avoid JSON.parse(JSON.stringify(...)) and instead either modify messageObj.message in place or return a shallow copy with the cleaned message (e.g., {...messageObj, message: cleanedString}) so only the string is copied; apply the same change for the other occurrence referenced around lines 161-162 to stop cloning the entire payload and only clean the message field in functions like cleanMessage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@webplugin/js/app/mck-sidebox-1.0.js`:
- Around line 3239-3243: The stopVoiceOutput() function is missing its closing
braces, which causes stopVoiceOnWidgetClose() and runCloseChatBoxActions() to be
nested inside it; close the if block and the function by adding the two missing
closing braces immediately after window.speechSynthesis.cancel(); so that
stopVoiceOutput(), stopVoiceOnWidgetClose(), and runCloseChatBoxActions() are
defined as sibling top-level functions (referencing stopVoiceOutput,
stopVoiceOnWidgetClose, runCloseChatBoxActions).
- Around line 4716-4720: In stopVoiceOutput(), replace the direct browser call
window.speechSynthesis.cancel() with a call to mckVoice.cancelNativeSpeech() so
mckVoice can perform its cleanup (clearing nativeSpeechUtterance and internal
state); update the stopVoiceOutput function to use the managed shutdown method
(referencing stopVoiceOutput and mckVoice.cancelNativeSpeech and the
nativeSpeechUtterance cleanup) so minimize/new-conversation stops don't leave
dangling utterance references.
In `@webplugin/js/app/media/media-service.js`:
- Around line 127-133: The tokenMessage filtering is inverted: swap the checks
so that when isTokenizedResponseEnabled is true you skip intermediate streaming
chunks (return if message && message.tokenMessage) and when it's false you skip
tokenized logic and only ignore final messages (return if !message ||
!message.tokenMessage); update the branch around isTokenizedResponseEnabled in
media-service.js (the conditions using message.tokenMessage) to the inverted
checks so final bot messages (message.tokenMessage === undefined/false) are
spoken instead of transient chunks.
---
Nitpick comments:
In `@webplugin/js/app/media/media-service.js`:
- Around line 134-139: The TTS hot path is doing a deep clone of the whole
message but only needs to sanitize message.message; update the cleanMessage
function to avoid JSON.parse(JSON.stringify(...)) and instead either modify
messageObj.message in place or return a shallow copy with the cleaned message
(e.g., {...messageObj, message: cleanedString}) so only the string is copied;
apply the same change for the other occurrence referenced around lines 161-162
to stop cloning the entire payload and only clean the message field in functions
like cleanMessage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0a1d2951-a9d7-4182-be31-cd3db4bf57cb
📒 Files selected for processing (2)
webplugin/js/app/mck-sidebox-1.0.jswebplugin/js/app/media/media-service.js
| function stopVoiceOutput() { | ||
| if (appOptions.voiceOutput && 'speechSynthesis' in window) { | ||
| window.speechSynthesis.cancel(); | ||
|
|
||
| function stopVoiceOnWidgetClose() { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Expect `stopVoiceOnWidgetClose` and `runCloseChatBoxActions` to remain outside `stopVoiceOutput`.
echo "== surrounding lines =="
sed -n '3237,3272p' webplugin/js/app/mck-sidebox-1.0.js
echo
echo "== symbol references =="
rg -n -C2 '\b(stopVoiceOnWidgetClose|runCloseChatBoxActions)\s*\(' webplugin/js/app/mck-sidebox-1.0.jsRepository: Kommunicate-io/Kommunicate-Web-SDK
Length of output: 2764
Add closing braces to complete the stopVoiceOutput() function before stopVoiceOnWidgetClose() definition.
The stopVoiceOutput() function (line 3240) is missing its closing braces. Without them, stopVoiceOnWidgetClose() and runCloseChatBoxActions() become nested inside stopVoiceOutput() instead of being sibling functions, breaking their scope and making them inaccessible to callers at lines 3262 and 3328.
Suggested fix
function stopVoiceOutput() {
if (appOptions.voiceOutput && 'speechSynthesis' in window) {
window.speechSynthesis.cancel();
+ }
+ }
function stopVoiceOnWidgetClose() {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function stopVoiceOutput() { | |
| if (appOptions.voiceOutput && 'speechSynthesis' in window) { | |
| window.speechSynthesis.cancel(); | |
| function stopVoiceOnWidgetClose() { | |
| function stopVoiceOutput() { | |
| if (appOptions.voiceOutput && 'speechSynthesis' in window) { | |
| window.speechSynthesis.cancel(); | |
| } | |
| } | |
| function stopVoiceOnWidgetClose() { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@webplugin/js/app/mck-sidebox-1.0.js` around lines 3239 - 3243, The
stopVoiceOutput() function is missing its closing braces, which causes
stopVoiceOnWidgetClose() and runCloseChatBoxActions() to be nested inside it;
close the if block and the function by adding the two missing closing braces
immediately after window.speechSynthesis.cancel(); so that stopVoiceOutput(),
stopVoiceOnWidgetClose(), and runCloseChatBoxActions() are defined as sibling
top-level functions (referencing stopVoiceOutput, stopVoiceOnWidgetClose,
runCloseChatBoxActions).
| function stopVoiceOutput() { | ||
| if (appOptions.voiceOutput && 'speechSynthesis' in window) { | ||
| window.speechSynthesis.cancel(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Expect sidebox shutdown helpers to use the same cleanup path as `mckVoice.cancelNativeSpeech()`.
echo "== raw browser cancel sites =="
rg -n -C2 'speechSynthesis\.cancel\(' \
webplugin/js/app/mck-sidebox-1.0.js \
webplugin/js/app/media/media-service.js
echo
echo "== native speech manager cleanup =="
rg -n -C3 'cancelNativeSpeech|nativeSpeechUtterance|removeAllAnimation|advanceQueueAfterPlayback' \
webplugin/js/app/voice/mck-voice.jsRepository: Kommunicate-io/Kommunicate-Web-SDK
Length of output: 6730
🏁 Script executed:
#!/bin/bash
# Check the context around lines 4731 and 4830 to see if they call stopVoiceOutput()
echo "=== Lines 4710-4740 (around line 4731) ==="
sed -n '4710,4740p' webplugin/js/app/mck-sidebox-1.0.js | cat -n
echo
echo "=== Lines 4820-4840 (around line 4830) ==="
sed -n '4820,4840p' webplugin/js/app/mck-sidebox-1.0.js | cat -n
echo
echo "=== Checking mckVoice variable scope near line 4716 ==="
# Look for mckVoice declarations and usages in surrounding context
sed -n '4700,4750p' webplugin/js/app/mck-sidebox-1.0.js | grep -n 'mckVoice\|var\|let\|const' | head -20Repository: Kommunicate-io/Kommunicate-Web-SDK
Length of output: 3377
🏁 Script executed:
#!/bin/bash
# Find where mckVoice is defined or referenced in mck-sidebox-1.0.js
echo "=== mckVoice references in mck-sidebox-1.0.js ==="
rg -n 'mckVoice' webplugin/js/app/mck-sidebox-1.0.js | head -20
echo
echo "=== Check broader scope context - look for where stopVoiceOutput is defined ==="
# Get context showing what variables are in scope at line 4716
sed -n '4600,4750p' webplugin/js/app/mck-sidebox-1.0.js | grep -n 'var\|let\|const\|this\.\|mckVoice\|function stopVoiceOutput' | head -30
echo
echo "=== Check for Kommunicate global usage in mck-sidebox-1.0.js ==="
rg -n 'Kommunicate\.' webplugin/js/app/mck-sidebox-1.0.js | head -10Repository: Kommunicate-io/Kommunicate-Web-SDK
Length of output: 3616
🏁 Script executed:
#!/bin/bash
# Check the context around lines 1143-1146 to understand when mediaService is called
echo "=== Context around lines 1143-1146 (mediaService call) ==="
sed -n '1135,1160p' webplugin/js/app/mck-sidebox-1.0.js | cat -n
echo
echo "=== What does mckVoice.cancelNativeSpeech do (from mck-voice.js) ==="
sed -n '613,618p' webplugin/js/app/voice/mck-voice.js | cat -n
echo
echo "=== Compare: what does mediaService.stopVoiceOutput do (from media-service.js) ==="
sed -n '210,214p' webplugin/js/app/media/media-service.js | cat -nRepository: Kommunicate-io/Kommunicate-Web-SDK
Length of output: 1966
🏁 Script executed:
#!/bin/bash
# Check if mediaService tracks any speech state beyond what mckVoice handles
echo "=== mediaService.stopVoiceOutput context and related code ==="
sed -n '205,225p' webplugin/js/app/media/media-service.js | cat -n
echo
echo "=== Check if there are other stopVoiceOutput implementations or related cleanup ==="
rg -n 'stopVoiceOutput|stopSpeech|cancelSpeech' webplugin/js/app/media/media-service.js
echo
echo "=== Verify: are mckVoice and mediaService both needed or just one? ==="
rg -B3 -A3 'mckVoice.cancelNativeSpeech|mckVoice.stopVoiceMode' webplugin/js/app/voice/mck-voice.js | head -30Repository: Kommunicate-io/Kommunicate-Web-SDK
Length of output: 1129
Use mckVoice's managed speech shutdown method here instead of raw browser cancel.
Calling window.speechSynthesis.cancel() directly on line 4718 bypasses mckVoice's cleanup logic. The mckVoice.cancelNativeSpeech() method (defined in webplugin/js/app/voice/mck-voice.js:613-618) properly clears the nativeSpeechUtterance reference and internal state. Without this, minimize (line 4731) and new-conversation (line 4830) trigger stops leave mckVoice with dangling utterance references.
Replace the raw cancel with:
function stopVoiceOutput() {
- if (appOptions.voiceOutput && 'speechSynthesis' in window) {
- window.speechSynthesis.cancel();
+ if (
+ typeof mckVoice !== 'undefined' &&
+ mckVoice &&
+ typeof mckVoice.cancelNativeSpeech === 'function'
+ ) {
+ mckVoice.cancelNativeSpeech();
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function stopVoiceOutput() { | |
| if (appOptions.voiceOutput && 'speechSynthesis' in window) { | |
| window.speechSynthesis.cancel(); | |
| } | |
| } | |
| function stopVoiceOutput() { | |
| if ( | |
| appOptions.voiceOutput && | |
| typeof mckVoice !== 'undefined' && | |
| mckVoice && | |
| typeof mckVoice.cancelNativeSpeech === 'function' | |
| ) { | |
| mckVoice.cancelNativeSpeech(); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@webplugin/js/app/mck-sidebox-1.0.js` around lines 4716 - 4720, In
stopVoiceOutput(), replace the direct browser call
window.speechSynthesis.cancel() with a call to mckVoice.cancelNativeSpeech() so
mckVoice can perform its cleanup (clearing nativeSpeechUtterance and internal
state); update the stopVoiceOutput function to use the managed shutdown method
(referencing stopVoiceOutput and mckVoice.cancelNativeSpeech and the
nativeSpeechUtterance cleanup) so minimize/new-conversation stops don't leave
dangling utterance references.
| if (isTokenizedResponseEnabled) { | ||
| if (!message || !message.tokenMessage) { | ||
| return; | ||
| } | ||
| } else if (message && message.tokenMessage) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
tokenMessage filtering is inverted here.
In this codebase, tokenMessage=true is the intermediate streaming chunk, and the final bot message is the one without tokenMessage (webplugin/js/app/voice/mck-voice-message-handler.js:61-69). This branch now suppresses the final message and speaks the transient chunks instead, which reintroduces repeated/fragmented playback and queueing.
Suggested fix
- if (isTokenizedResponseEnabled) {
- if (!message || !message.tokenMessage) {
- return;
- }
- } else if (message && message.tokenMessage) {
+ if (!message || message.tokenMessage) {
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (isTokenizedResponseEnabled) { | |
| if (!message || !message.tokenMessage) { | |
| return; | |
| } | |
| } else if (message && message.tokenMessage) { | |
| return; | |
| } | |
| if (!message || message.tokenMessage) { | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@webplugin/js/app/media/media-service.js` around lines 127 - 133, The
tokenMessage filtering is inverted: swap the checks so that when
isTokenizedResponseEnabled is true you skip intermediate streaming chunks
(return if message && message.tokenMessage) and when it's false you skip
tokenized logic and only ignore final messages (return if !message ||
!message.tokenMessage); update the branch around isTokenizedResponseEnabled in
media-service.js (the conditions using message.tokenMessage) to the inverted
checks so final bot messages (message.tokenMessage === undefined/false) are
spoken instead of transient chunks.
| }); | ||
| var closeButton = document.getElementById('km-chat-widget-close-button'); | ||
|
|
||
| function stopVoiceOutput() { |
| }); | ||
| var closeButton = document.getElementById('km-chat-widget-close-button'); | ||
|
|
||
| function stopVoiceOutput() { |
There was a problem hiding this comment.
is this a duplicate function, another place also have function stopVoiceOutput
3 participants
What do you want to achieve?
When chat widget is closed/minimized or new conversation started, the AI Agent will keep speaking
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