Master 13.8.1 #1319
Master 13.8.1 #1319
Conversation
generative repsonse implementation
This reverts commit a01c5f6.
Cus-479-new UI is breaking in widget and dashboard if any msg have table component
fix the talk to human bugs
…essage is triggering
TDH-1788 | On restarting the conversation, default language Welcome message is triggering
TDH-1971 -: Fix the html symbol error
TDH-1893 -: Fix the email is not loading
UI table chanes revert
This reverts commit 956a078.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (15)
package.json (1)
4-4: Reconsider using HTML in package.json descriptionThe description field has been updated to include an HTML image tag. While this works, it's unconventional and may not display properly in all contexts where package.json is viewed (like npm registry pages or GitHub).
Consider using a plain text description that explains the package's purpose instead of HTML content. If you want to keep the image reference, consider moving it to documentation files where HTML is expected.
webplugin/js/app/components/custom-element.js (1)
8-8: Consider adding class documentationA new
MckEmailComponentclass has been created, but it currently doesn't add any functionality beyond the parent class.Add a JSDoc comment to explain the purpose of this class and how it differs from
ShadowDomComponent, even if it's just for future extensibility.+/** + * Custom component for rendering rich email messages + * Currently extends base functionality from ShadowDomComponent + * but can be extended for email-specific rendering needs + */ class MckEmailComponent extends ShadowDomComponent {}webplugin/js/app/km-richtext-markup-1.0.js (1)
861-862: Improved email rendering architecture using custom elements.The replacement of iframe-based rendering with a custom element (
<mck-email-rich-message>) is a significant architectural improvement. This approach:
- Enhances security by eliminating iframe-related vulnerabilities
- Provides better styling control and component isolation
- Creates a more maintainable and extensible email rendering solution
- Ensures proper message isolation through unique identifiers based on both groupId and key
This change aligns with modern web component practices and creates a foundation for more sophisticated email content rendering.
Consider adding a brief comment explaining the purpose of the custom element and how it relates to the mail parser infrastructure for better code documentation.
webplugin/js/app/km-utils.js (1)
523-538: New utility function for email processing.This utility function efficiently fetches and parses .eml files using modern async/await patterns and the Fetch API. It integrates with the external kmPostalMime library for parsing the email content.
Consider these improvements:
- The commented-out code about
responseTypeheader suggests incomplete implementation or testing.- While rethrow-only catch blocks are sometimes flagged by static analysis, keeping it provides a point for future error handling enhancement.
getAndParseEml: async function (emlUrl) { try { const response = await fetch(emlUrl, { method: 'GET', - // headers: { - // responseType: 'arraybuffer', - // }, }); const emlData = await response.arrayBuffer(); const data = await kmPostalMime.parse(emlData); return data; } catch (e) { - throw e; + console.error('Error parsing email file:', e); + throw e; // Rethrow to allow caller to handle } },🧰 Tools
🪛 Biome (1.9.4)
[error] 536-536: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.(lint/complexity/noUselessCatch)
webplugin/js/app/conversation/gen-ai-service.js (1)
22-26: Improve sequence validation with strict equality comparisonThis validation correctly ensures tokens are processed in order, but uses non-strict equality comparison.
- if (this.currentIndex != msg.index - 1) { + if (this.currentIndex !== msg.index - 1) { // if any token is missed then stop there return; }webplugin/js/app/conversation/mail-parser/dom-service.js (2)
6-8: Add more contextual information to the error messageThe error message could be more helpful by including context about why the container wasn't found.
- throw new Error(`Email container not found: ${containerId}`); + throw new Error(`Email container not found: ${containerId}. Make sure the custom element is registered and the container is created before calling this method.`);
57-67: Remove or document commented-out codeThere's a significant block of commented-out code that should either be removed if no longer needed or documented with a TODO comment explaining why it's preserved.
Either remove the commented-out code or add a clear explanation:
- // if (!emlToHtml) { - // const users = kmMailProcessor.getToUsersFromGroup(group, message.to); - // const fromAddress = group.members.find((mem) => mem.includes(message.to)); - // const fromMarkup = EmailDOMService.getFromMarkup({ - // name: message.to, - // address: fromAddress || '', - // }); - // const toAndCcMarkup = EmailDOMService.getToAndCCMarkup(users); - // kmEmailMainContainer.insertAdjacentHTML('afterbegin', toAndCcMarkup); - // kmEmailMainContainer.insertAdjacentHTML('afterbegin', fromMarkup); - // } + // TODO: Code for adding From/To/CC markup is commented out because [reason]. + // Uncomment and complete implementation when [condition].webplugin/js/app/conversation/mail-parser/style.js (3)
27-28: Replace commented-out style and improve overflow handlingThe CSS contains a commented-out style and uses
overflow: scrollwhich forces scrollbars even when not needed.- // border-top: none; - overflow: scroll; + overflow: auto;
75-76: Avoid using !important in CSS rulesUsing
!importantoverrides the normal cascade and can make styles harder to maintain.Consider refactoring the CSS selector specificity instead of using
!importantto ensure the correct styles are applied.Also applies to: 133-135
109-111: Remove duplicate styling for .km-email-show-more-btnThere appears to be duplicate styling for
.km-email-show-more-btnwhich was already defined in lines 85-108.- .km-email-show-more-btn { - margin-bottom: 6px; - }webplugin/js/app/conversation/mail-parser/eml-parser.js (2)
8-8: Add a comment explaining the regular expressionThe regular expression in
getFormattedMessageis complex and would benefit from a comment explaining what pattern it's targeting.- return message.replaceAll('\n', '<br>').replace(/\[\s*([a-zA-Z0-9]+):[^\]]*\s*\]/g, ''); + // Replace newlines with <br> tags and remove image placeholders of the format [image: image.png] + return message.replaceAll('\n', '<br>').replace(/\[\s*([a-zA-Z0-9]+):[^\]]*\s*\]/g, '');
25-30: Remove or document commented-out codeSimilar to other files, there's commented-out code that should either be removed if no longer needed or documented with a TODO comment.
Either remove the commented-out code or add a clear explanation about its purpose and future use.
webplugin/js/app/conversation/mail-parser/index.js (1)
109-113: Use standard method syntax – arrow field adds unnecessary overheadDeclaring a class field with
= function…creates a new function per instance and slightly complicates type inference. Prefer regular method syntax:-getToUsersFromGroup = function (group, msgFrom) { - … -}; +getToUsersFromGroup(group, msgFrom) { + … +}webplugin/js/app/conversation/mail-parser/attachment-service.js (1)
16-19: Ensure global availability ofDOMPurify
DOMPurifyis referenced but not imported. If it isn’t globally attached before this script runs, the call will throw. Verify bundler order or import explicitly.webplugin/js/app/mck-sidebox-1.0.js (1)
21-25: Preferconstovervarfor immutable configuration objects
userOverrideis never reassigned, so declaring it withvaris unnecessary and can invite accidental re-assignment or hoisting surprises elsewhere in this 8 000-line file.-var userOverride = { +const userOverride = { voiceOutput: true, };If runtime mutation is required, consider freezing the object (
Object.freeze) to make intent explicit.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (18)
package.json(1 hunks)webplugin/bundleFiles.js(2 hunks)webplugin/css/app/km-rich-message.css(1 hunks)webplugin/css/app/mck-sidebox-1.0.css(1 hunks)webplugin/js/app/components/custom-element.js(1 hunks)webplugin/js/app/components/typing-service.js(3 hunks)webplugin/js/app/conversation/gen-ai-service.js(3 hunks)webplugin/js/app/conversation/mail-parser/attachment-service.js(1 hunks)webplugin/js/app/conversation/mail-parser/dom-service.js(1 hunks)webplugin/js/app/conversation/mail-parser/eml-parser.js(1 hunks)webplugin/js/app/conversation/mail-parser/index.js(1 hunks)webplugin/js/app/conversation/mail-parser/style.js(1 hunks)webplugin/js/app/km-richtext-markup-1.0.js(1 hunks)webplugin/js/app/km-utils.js(2 hunks)webplugin/js/app/kommunicate-client.js(1 hunks)webplugin/js/app/kommunicate-ui.js(1 hunks)webplugin/js/app/kommunicate.js(2 hunks)webplugin/js/app/mck-sidebox-1.0.js(19 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
webplugin/js/app/conversation/mail-parser/index.js (1)
webplugin/js/app/conversation/mail-parser/eml-parser.js (1)
kmEmlToHtml(154-154)
🪛 Biome (1.9.4)
webplugin/js/app/km-utils.js
[error] 536-536: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.
(lint/complexity/noUselessCatch)
webplugin/js/app/conversation/mail-parser/dom-service.js
[error] 1-84: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
🔇 Additional comments (20)
webplugin/js/app/kommunicate-client.js (1)
68-68: Simple state initialization for loader control.This new line sets the
isWaitingQueueflag tofalseon the globalCURRENT_GROUP_DATAobject, which controls the display of a loader on the screen during conversation creation.webplugin/css/app/km-rich-message.css (1)
782-785: Style addition for email rich message layout.The new CSS class
.mck-email-rich-msgdefines width and spacing for email rich message components, supporting the new email parsing and rendering feature.webplugin/css/app/mck-sidebox-1.0.css (1)
4983-4983: Padding adjustment for email display container.The padding property has been commented out for the
.km-mail-fixed-viewclass. This change likely improves the layout of email content within the container, allowing the email's own spacing to control the presentation.webplugin/js/app/components/custom-element.js (1)
15-17: LGTM - Custom element registration looks goodProperly registering the new custom element for email messages, following the pattern used for the HTML rich message component.
webplugin/bundleFiles.js (2)
13-19: LGTM - Well-organized mail parser filesThis new constant containing the mail parser file paths is well-structured and follows the existing code patterns.
43-43: LGTM - Clean integration of mail parser filesProperly integrating the mail parser files into the plugin bundle.
webplugin/js/app/kommunicate-ui.js (1)
1582-1590:✅ Verification successful
LGTM - Properly handling UI state for waiting queue
The changes correctly toggle visibility of the "talk to human" elements and set the waiting queue flag on the current group data.
Let's verify that this state flag is properly reset when a conversation exits the waiting queue:
🏁 Script executed:
#!/bin/bash # Check where CURRENT_GROUP_DATA.isWaitingQueue gets reset rg -A 3 "CURRENT_GROUP_DATA.isWaitingQueue = false" --type jsLength of output: 1581
Waiting queue state reset verified across client and sidebox modules
grep shows
CURRENT_GROUP_DATA.isWaitingQueueis explicitly reset tofalsein the following locations, confirming the flag is cleared when exiting the waiting queue:
- webplugin/js/app/kommunicate-client.js (loader logic)
- webplugin/js/app/mck-sidebox-1.0.js (multiple exit flows)
No further changes are required—this correctly manages the waiting‐queue UI state.
webplugin/js/app/kommunicate.js (2)
579-582: Proper extension of email message handling to include new ELECTRONIC_MAIL content type.The condition is appropriately extended to handle both traditional HTML emails (content type TEXT_HTML with source MAIL_INTERCEPTOR) and the newly added ELECTRONIC_MAIL content type. This ensures consistency in how email content is processed and displayed throughout the application.
723-726: Improved text area enablement logic for message processing.This change enhances the user experience by ensuring the text area is properly enabled in two scenarios: when the last token is received or when processing a non-token message. The reset of the GenAI state in conjunction with enablement ensures a clean state for new interactions.
webplugin/js/app/km-utils.js (1)
115-115: New email content type constant is properly defined.Adding the
ELECTRONIC_MAILcontent type with value104to theMESSAGE_CONTENT_TYPEenum properly extends the messaging capabilities to support email messages throughout the application.webplugin/js/app/components/typing-service.js (4)
12-12: Added property for tracking talk-to-human messages.This property enables the typing service to be aware of special "talk to human" messages, which require different handling for typing indicators and other UI elements.
24-26: Proper setter method implementation for the new property.The setter method follows good encapsulation practices by providing a controlled way to update the
isKmTalkToHumanMsgstate.
29-33: Prevent duplicate tokenized responses in the message queue.This check prevents duplicate UI effects by ensuring tokenized responses aren't added to the queue multiple times. This improves performance and avoids repetitive visual elements.
64-65: Enhanced typing indicator visibility logic.This condition intelligently suppresses the typing indicator in two special cases:
- When a "talk to human" message is being processed
- When the user is in a waiting queue state
This prevents confusing UI feedback in situations where showing a typing indicator would be misleading, improving the overall user experience.
webplugin/js/app/conversation/gen-ai-service.js (4)
5-5: LGTM: Added token index tracking stateThe addition of
currentIndexproperty initialized to -1 provides a way to track the sequential processing of tokens, which is necessary for maintaining message integrity.
10-10: LGTM: Reset element at method startResetting the
currentElementat the beginning of each method call ensures a clean slate for token processing and prevents state-related bugs.
27-27: LGTM: Added space after each tokenAdding a space after each token ensures proper word separation in the rendered message, which improves readability.
39-39: LGTM: Reset token index trackingResetting the
currentIndexproperty in theresetStatemethod ensures complete state cleanup when needed.webplugin/js/app/conversation/mail-parser/style.js (1)
3-16: LGTM: Well-defined scrollbar stylingThe scrollbar styling is comprehensive and provides a consistent appearance across different browsers that support these properties.
webplugin/js/app/mck-sidebox-1.0.js (1)
8372-8377:kmMailProcessorinvocation is unconditional – add a safety netIf
kmMailProcessorfailed to load (e.g. bundle order issue) the whole message-rendering flow will halt. Consider guarding or logging:- kmMailProcessor.processMail(msg, contact, emlMessage); + if (emlMessage && typeof kmMailProcessor?.processMail === 'function') { + kmMailProcessor.processMail(msg, contact, true); + }
| const anchors = kmEmailMainContainer.querySelectorAll('a'); | ||
|
|
||
| anchors.forEach((anchor) => { | ||
| anchor.setAttribute('target', '_blank'); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add security attributes to anchor tags
When setting target="_blank" on anchor tags, also set rel="noopener noreferrer" to prevent potential security issues known as "reverse tabnabbing".
anchors.forEach((anchor) => {
anchor.setAttribute('target', '_blank');
+ anchor.setAttribute('rel', 'noopener noreferrer');
});📝 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.
| const anchors = kmEmailMainContainer.querySelectorAll('a'); | |
| anchors.forEach((anchor) => { | |
| anchor.setAttribute('target', '_blank'); | |
| }); | |
| const anchors = kmEmailMainContainer.querySelectorAll('a'); | |
| anchors.forEach((anchor) => { | |
| anchor.setAttribute('target', '_blank'); | |
| anchor.setAttribute('rel', 'noopener noreferrer'); | |
| }); |
🤖 Prompt for AI Agents
In webplugin/js/app/conversation/mail-parser/dom-service.js around lines 69 to
73, when setting target="_blank" on anchor tags, also add rel="noopener
noreferrer" to each anchor element to prevent reverse tabnabbing security
issues. Update the code to set both attributes on each anchor within the forEach
loop.
| class EmailDOMService { | ||
| static createEmailContainer(message, group) { | ||
| const containerId = `km-email-${message.groupId}-${message.key}`; | ||
| const container = document.getElementById(containerId); | ||
|
|
||
| if (!container) { | ||
| throw new Error(`Email container not found: ${containerId}`); | ||
| } | ||
|
|
||
| const shadowRoot = container.shadowRoot; | ||
| const style = document.createElement('style'); | ||
| style.setAttribute('type', 'text/css'); | ||
| style.innerHTML = kmMailProcessor.getEmailRichMsgStyle(); | ||
|
|
||
| const mailElement = document.createElement('div'); | ||
| mailElement.setAttribute( | ||
| 'class', | ||
| `km-email-rich-msg-container ${ | ||
| group.metadata?.source == 'MAIL_INTERCEPTOR' | ||
| ? 'km-email-source-mail-interceptor' | ||
| : '' | ||
| }` | ||
| ); | ||
|
|
||
| shadowRoot.append(style, mailElement); | ||
| return mailElement; | ||
| } | ||
|
|
||
| static getToAndCCMarkup(headers = []) { | ||
| const { toUsers, ccUsers, bccUsers } = kmMailProcessor.getCcToBccUsers(headers); | ||
|
|
||
| const isCcUserVisible = ccUsers ? 'vis' : 'n-vis'; | ||
| const isBccUserVisible = bccUsers ? 'vis' : 'n-vis'; | ||
| return `<div class="km-email-to-cc-container"> | ||
| <div class="km-email-to-heading km-email-headers" title="${toUsers}"><strong>${'to'}:</strong> <span>${toUsers} </span></div> | ||
| <div class="km-email-cc-heading km-email-headers ${isCcUserVisible}" title="${ccUsers}"><strong>${'cc'}:</strong> <span>${ccUsers}</span></div> | ||
| <div class="km-email-bcc-heading ${isBccUserVisible}" title="${bccUsers}"><strong>${'bcc'}:</strong> <span>${bccUsers}</span></div> | ||
| </div>`; | ||
| } | ||
|
|
||
| static appendEmailRichMsg(message, group, emlToHtml) { | ||
| const richText = | ||
| Kommunicate.isRichTextMessage(message.metadata) || | ||
| message.contentType == 3 || | ||
| emlToHtml; | ||
|
|
||
| if ((richText && message.source === 7 && message.message) || emlToHtml) { | ||
| const kmEmailRichMsg = document.getElementById( | ||
| 'km-email-' + message.groupId + '-' + message.key | ||
| ); | ||
| const kmEmailMainContainer = kmEmailRichMsg.shadowRoot.querySelector( | ||
| '.km-email-rich-msg-container' | ||
| ); | ||
|
|
||
| kmEmailMainContainer.innerHTML = emlToHtml ? message.emlContent : message.message; | ||
|
|
||
| // if (!emlToHtml) { | ||
| // const users = kmMailProcessor.getToUsersFromGroup(group, message.to); | ||
| // const fromAddress = group.members.find((mem) => mem.includes(message.to)); | ||
| // const fromMarkup = EmailDOMService.getFromMarkup({ | ||
| // name: message.to, | ||
| // address: fromAddress || '', | ||
| // }); | ||
| // const toAndCcMarkup = EmailDOMService.getToAndCCMarkup(users); | ||
| // kmEmailMainContainer.insertAdjacentHTML('afterbegin', toAndCcMarkup); | ||
| // kmEmailMainContainer.insertAdjacentHTML('afterbegin', fromMarkup); | ||
| // } | ||
|
|
||
| const anchors = kmEmailMainContainer.querySelectorAll('a'); | ||
|
|
||
| anchors.forEach((anchor) => { | ||
| anchor.setAttribute('target', '_blank'); | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| static getLoadingSpinner() { | ||
| return `<div class="km-eml-loading">Loading...</div>`; | ||
| } | ||
|
|
||
| static getFromMarkup(from = {}) { | ||
| return `<div class="km-email-from-heading" title="${from.address}"><strong class="km-email-from">${from.name}</strong> <p><${from.address}></p></div>`; | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider using utility functions instead of a class with only static methods
This class contains only static methods, which suggests it might be better structured as a collection of utility functions rather than a class.
-class EmailDOMService {
- static createEmailContainer(message, group) {
+const EmailDOMService = {};
+
+EmailDOMService.createEmailContainer = function(message, group) {🏁 Script executed:
#!/bin/bash
# Check if other classes in the codebase follow a similar pattern or use regular utility functions
rg -A 1 "class [A-Za-z]+" | grep -A 1 "static " | head -10Length of output: 170
Refactor static-only class to a utility module
This file (webplugin/js/app/conversation/mail-parser/dom-service.js, lines 1–84) defines a class containing only static methods. You can simplify it by exporting standalone functions instead of wrapping them in a class:
-// webplugin/js/app/conversation/mail-parser/dom-service.js
-class EmailDOMService {
- static createEmailContainer(message, group) { … }
- static getToAndCCMarkup(headers = []) { … }
- static appendEmailRichMsg(message, group, emlToHtml) { … }
- static getLoadingSpinner() { … }
- static getFromMarkup(from = {}) { … }
-}
-export { EmailDOMService };
+// webplugin/js/app/conversation/mail-parser/dom-service.js
+export function createEmailContainer(message, group) { … }
+export function getToAndCCMarkup(headers = []) { … }
+export function appendEmailRichMsg(message, group, emlToHtml) { … }
+export function getLoadingSpinner() { … }
+export function getFromMarkup(from = {}) { … }- Eliminates an unnecessary class wrapper
- Aligns with standard ES module patterns
- Improves tree-shaking and reduces bundle size
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 1-84: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
🤖 Prompt for AI Agents
In webplugin/js/app/conversation/mail-parser/dom-service.js lines 1 to 84, the
EmailDOMService class contains only static methods, which is an unnecessary
class wrapper. Refactor by converting each static method into a standalone
exported function, removing the class declaration entirely. Update all method
references accordingly to use these functions directly, improving modularity and
enabling better tree-shaking.
| attachments.map(async (attachment) => { | ||
| switch (true) { | ||
| case attachment.mimeType?.includes('image/'): | ||
| attachMentService.setAttachmentType(imageService); | ||
| break; | ||
| case attachment.mimeType?.includes('video/'): | ||
| attachMentService.setAttachmentType(videoService); | ||
| break; | ||
| default: | ||
| attachMentService.setAttachmentType(otherService); | ||
| break; | ||
| } | ||
|
|
||
| const htmlString = await attachMentService.getAttachment(attachment); | ||
| attachmentWrapper.insertAdjacentHTML('beforeend', htmlString); | ||
|
|
||
| emailContainer.append(wrapper); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use forEach instead of map when not using the return value
The code uses map but doesn't use its return value, which is not the intended purpose of map.
- attachments.map(async (attachment) => {
+ attachments.forEach(async (attachment) => {Also consider using Promise.all to process all attachments in parallel:
Promise.all(
attachments.map(async (attachment) => {
// Process each attachment and return the HTML string
// ...
return htmlString;
})
).then(htmlStrings => {
// Append all HTML strings at once
htmlStrings.forEach(html => {
attachmentWrapper.insertAdjacentHTML('beforeend', html);
});
emailContainer.append(wrapper);
});🤖 Prompt for AI Agents
In webplugin/js/app/conversation/mail-parser/eml-parser.js around lines 109 to
127, the code uses map to iterate over attachments without using the returned
array, which is incorrect. Replace the map call with Promise.all wrapping a map
that returns the awaited HTML strings for each attachment. Then, in the
Promise.all then callback, insert all HTML strings into attachmentWrapper and
append the wrapper to emailContainer once, ensuring parallel processing and
proper handling of asynchronous operations.
| img.src = URL.createObjectURL( | ||
| new Blob([attachment.content], { | ||
| type: attachment.mimeType, | ||
| }) | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prevent memory leaks with URL.createObjectURL
The code creates object URLs but doesn't revoke them, which can lead to memory leaks if many emails with attachments are processed.
Consider revoking the object URL when it's no longer needed:
if (attachment) {
- img.src = URL.createObjectURL(
+ const objectUrl = URL.createObjectURL(
new Blob([attachment.content], {
type: attachment.mimeType,
})
);
+ img.src = objectUrl;
+ // Revoke the URL when the image has loaded to free memory
+ img.onload = () => URL.revokeObjectURL(objectUrl);
}📝 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.
| img.src = URL.createObjectURL( | |
| new Blob([attachment.content], { | |
| type: attachment.mimeType, | |
| }) | |
| ); | |
| } | |
| if (attachment) { | |
| const objectUrl = URL.createObjectURL( | |
| new Blob([attachment.content], { | |
| type: attachment.mimeType, | |
| }) | |
| ); | |
| img.src = objectUrl; | |
| // Revoke the URL when the image has loaded to free memory | |
| img.onload = () => URL.revokeObjectURL(objectUrl); | |
| } |
🤖 Prompt for AI Agents
In webplugin/js/app/conversation/mail-parser/eml-parser.js around lines 144 to
149, the code creates object URLs using URL.createObjectURL but does not revoke
them, which can cause memory leaks. To fix this, add a call to
URL.revokeObjectURL for the created object URL when the image is no longer
needed, such as after the image has loaded or been removed, to properly release
the allocated memory.
| await this.loadPostalMimeLib(); | ||
| this.appendEmailWrapper(msg, group); | ||
|
|
||
| if (this.checkIfMsgInEmlFormat(msg)) { | ||
| await kmEmlToHtml.getEmlToHtml(msg, group, emlMessage); | ||
| return; | ||
| } | ||
| // Process old flow | ||
| EmailDOMService.appendEmailRichMsg(msg, group); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Wrap network-dependent logic in a try/catch to avoid breaking the UI
If the external script fails to load, loadPostalMimeLib() rejects, propagating an unhandled promise rejection and leaving the message container empty.
Consider graceful degradation:
-await this.loadPostalMimeLib();
-this.appendEmailWrapper(msg, group);
+try {
+ await this.loadPostalMimeLib();
+} catch (e) {
+ console.error('Falling back – postal-mime lib not available', e);
+}
+this.appendEmailWrapper(msg, group);📝 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.
| await this.loadPostalMimeLib(); | |
| this.appendEmailWrapper(msg, group); | |
| if (this.checkIfMsgInEmlFormat(msg)) { | |
| await kmEmlToHtml.getEmlToHtml(msg, group, emlMessage); | |
| return; | |
| } | |
| // Process old flow | |
| EmailDOMService.appendEmailRichMsg(msg, group); | |
| } | |
| try { | |
| await this.loadPostalMimeLib(); | |
| } catch (e) { | |
| console.error('Falling back – postal-mime lib not available', e); | |
| } | |
| this.appendEmailWrapper(msg, group); | |
| if (this.checkIfMsgInEmlFormat(msg)) { | |
| await kmEmlToHtml.getEmlToHtml(msg, group, emlMessage); | |
| return; | |
| } | |
| // Process old flow | |
| EmailDOMService.appendEmailRichMsg(msg, group); | |
| } |
🤖 Prompt for AI Agents
In webplugin/js/app/conversation/mail-parser/index.js around lines 5 to 14, the
call to loadPostalMimeLib() is not wrapped in a try/catch block, causing
unhandled promise rejections if the external script fails to load. Wrap the
await this.loadPostalMimeLib() call in a try/catch block to handle errors
gracefully, ensuring the UI does not break and the message container is not left
empty. In the catch block, log the error and allow the code to continue with
fallback behavior.
| arrayBufferToVideo(arrayBuffer, attachment) { | ||
| // Create object URL directly instead of using FileReader | ||
| const blob = new Blob([arrayBuffer], { | ||
| type: attachment.mimeType || 'video/mp4', | ||
| }); | ||
| const objectUrl = URL.createObjectURL(blob); | ||
|
|
||
| return new Promise((resolve, reject) => { | ||
| const video = document.createElement('video'); | ||
| video.controls = true; | ||
| video.preload = 'metadata'; // Only load metadata initially | ||
| video.setAttribute('data-size', alFileService.getFilePreviewSize(blob.size)); | ||
| // Use loadedmetadata instead of loadeddata for faster loading | ||
| video.addEventListener( | ||
| 'loadedmetadata', | ||
| () => { | ||
| resolve(video); | ||
| }, | ||
| { once: true } | ||
| ); // Use once:true for automatic cleanup | ||
|
|
||
| video.addEventListener( | ||
| 'error', | ||
| () => { | ||
| URL.revokeObjectURL(objectUrl); | ||
| reject(new Error('Failed to load video')); | ||
| }, | ||
| { once: true } | ||
| ); | ||
|
|
||
| video.src = objectUrl; | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Video blob URL never revoked
The object URL should be freed after the video element is ready to prevent leaks:
video.addEventListener(
'loadedmetadata',
() => {
+ URL.revokeObjectURL(objectUrl);
resolve(video);
},
{ once: true }
);📝 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.
| arrayBufferToVideo(arrayBuffer, attachment) { | |
| // Create object URL directly instead of using FileReader | |
| const blob = new Blob([arrayBuffer], { | |
| type: attachment.mimeType || 'video/mp4', | |
| }); | |
| const objectUrl = URL.createObjectURL(blob); | |
| return new Promise((resolve, reject) => { | |
| const video = document.createElement('video'); | |
| video.controls = true; | |
| video.preload = 'metadata'; // Only load metadata initially | |
| video.setAttribute('data-size', alFileService.getFilePreviewSize(blob.size)); | |
| // Use loadedmetadata instead of loadeddata for faster loading | |
| video.addEventListener( | |
| 'loadedmetadata', | |
| () => { | |
| resolve(video); | |
| }, | |
| { once: true } | |
| ); // Use once:true for automatic cleanup | |
| video.addEventListener( | |
| 'error', | |
| () => { | |
| URL.revokeObjectURL(objectUrl); | |
| reject(new Error('Failed to load video')); | |
| }, | |
| { once: true } | |
| ); | |
| video.src = objectUrl; | |
| }); | |
| } | |
| arrayBufferToVideo(arrayBuffer, attachment) { | |
| // Create object URL directly instead of using FileReader | |
| const blob = new Blob([arrayBuffer], { | |
| type: attachment.mimeType || 'video/mp4', | |
| }); | |
| const objectUrl = URL.createObjectURL(blob); | |
| return new Promise((resolve, reject) => { | |
| const video = document.createElement('video'); | |
| video.controls = true; | |
| video.preload = 'metadata'; // Only load metadata initially | |
| video.setAttribute('data-size', alFileService.getFilePreviewSize(blob.size)); | |
| // Use loadedmetadata instead of loadeddata for faster loading | |
| video.addEventListener( | |
| 'loadedmetadata', | |
| () => { | |
| URL.revokeObjectURL(objectUrl); | |
| resolve(video); | |
| }, | |
| { once: true } | |
| ); // Use once:true for automatic cleanup | |
| video.addEventListener( | |
| 'error', | |
| () => { | |
| URL.revokeObjectURL(objectUrl); | |
| reject(new Error('Failed to load video')); | |
| }, | |
| { once: true } | |
| ); | |
| video.src = objectUrl; | |
| }); | |
| } |
🤖 Prompt for AI Agents
In webplugin/js/app/conversation/mail-parser/attachment-service.js around lines
104 to 136, the object URL created for the video blob is never revoked, causing
a memory leak. To fix this, add a call to URL.revokeObjectURL(objectUrl) inside
the 'loadedmetadata' event listener after the video is resolved, ensuring the
URL is freed once the video metadata is loaded.
| return new Promise((resolve, reject) => { | ||
| const blob = new Blob([arrayBuffer], { type: attachment.mimeType }); | ||
|
|
||
| const imgElement = document.createElement('img'); | ||
|
|
||
| imgElement.setAttribute('class', 'file-preview-link fancybox-media kmfancybox'); | ||
| imgElement.setAttribute('data-type', attachment.mimeType); | ||
| imgElement.setAttribute('data-name', attachment.filename); | ||
| imgElement.setAttribute('data-url', imgElement.src); | ||
| imgElement.setAttribute('data-size', alFileService.getFilePreviewSize(blob.size)); | ||
|
|
||
| imgElement.onload = () => { | ||
| resolve(imgElement); | ||
| }; | ||
| imgElement.onerror = () => { | ||
| URL.revokeObjectURL(blob); | ||
| reject(new Error('Failed to load image')); | ||
| }; | ||
| imgElement.src = URL.createObjectURL(blob); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Memory leak & incorrect data-url attribute
data-urlis set beforesrc, so it stays empty.- The created object-URL is never revoked on successful load – may leak blobs.
-imgElement.setAttribute('data-url', imgElement.src);
+// set src first, then mirror it in data-url
+imgElement.src = URL.createObjectURL(blob);
+imgElement.setAttribute('data-url', imgElement.src);
…
-imgElement.onload = () => {
- resolve(imgElement);
-};
+imgElement.onload = () => {
+ URL.revokeObjectURL(imgElement.src); // free memory
+ resolve(imgElement);
+};Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In webplugin/js/app/conversation/mail-parser/attachment-service.js between lines
62 and 82, the data-url attribute is set before the imgElement.src, so it
remains empty, and the created object URL is never revoked on successful image
load, causing a memory leak. Fix this by setting data-url after assigning
imgElement.src to the object URL, and revoke the object URL inside the
imgElement.onload handler to release the blob resource properly.
| if (validated) { | ||
| mckMessageLayout.messageContextMenu(messageKey); | ||
| } | ||
| if (messagePxy.metadata.hasOwnProperty('TALK_TO_HUMAN')) { | ||
| typingService.setTalkToHumanMsg(true); | ||
| } | ||
|
|
There was a problem hiding this comment.
Guard against missing metadata before property access
messagePxy.metadata may legitimately be null/undefined. Accessing .hasOwnProperty without a check will crash the render pipeline.
-if (messagePxy.metadata.hasOwnProperty('TALK_TO_HUMAN')) {
+if (
+ messagePxy.metadata &&
+ Object.prototype.hasOwnProperty.call(messagePxy.metadata, 'TALK_TO_HUMAN')
+) {🤖 Prompt for AI Agents
In webplugin/js/app/mck-sidebox-1.0.js around lines 5615 to 5621, the code
accesses messagePxy.metadata.hasOwnProperty without verifying if metadata
exists, which can cause a crash if metadata is null or undefined. Add a guard
condition to check that messagePxy.metadata is truthy before calling
hasOwnProperty on it to prevent runtime errors.
| console.log('conversation created successfully'); | ||
| kmWidgetEvents.eventTracking(eventMapping.onStartNewConversation); | ||
| KommunicateUI.activateTypingField(); | ||
| kmNavBar.hideAndShowTalkToHumanBtn(); | ||
|
|
||
| !data?.groupFeeds.length && kmNavBar.hideAndShowTalkToHumanBtn(); | ||
| } |
There was a problem hiding this comment.
groupFeeds?.length can throw – add another optional-chaining level
data?.groupFeeds.length will still dereference .length even when groupFeeds is undefined, leading to a TypeError.
-!data?.groupFeeds.length && kmNavBar.hideAndShowTalkToHumanBtn();
+!(data?.groupFeeds?.length) && kmNavBar.hideAndShowTalkToHumanBtn();This tiny change prevents a hard crash when the conversation is created without feeds.
🤖 Prompt for AI Agents
In webplugin/js/app/mck-sidebox-1.0.js around lines 670 to 675, the expression
data?.groupFeeds.length can cause a TypeError if groupFeeds is undefined because
it tries to access .length directly. To fix this, add an additional optional
chaining operator before .length, changing it to data?.groupFeeds?.length,
ensuring safe access and preventing crashes when groupFeeds is missing.
|
|
||
| if ( | ||
| $applozic('#mck-message-cell .' + msg.key).length > 0 && | ||
| !(CURRENT_GROUP_DATA.TOKENIZE_RESPONSE && msg.type !== 5) | ||
| !(msg.tokenMessage && msg.type !== 5) | ||
| ) { | ||
| // if message with same key already rendered skiping rendering it again. | ||
| return; | ||
| } | ||
| if (CURRENT_GROUP_DATA.TOKENIZE_RESPONSE && !msg.tokenMessage) { | ||
| // deleting tokenized message after receiving complete message from chat server | ||
| const element = document.querySelector(`div[data-msgkey="tokenized_response"]`); | ||
| if (element) { | ||
| console.log('deleted'); | ||
| element.remove(); | ||
| genAiService.resetState(); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Duplicate–message filter misses edge-cases & risks invalid class selectors
'.' + msg.keyassumesmsg.keyis a valid CSS class (cannot start with a digit or contain spaces).- The filter ignores tokenised messages when
msg.type === 5, but elsewhere the code usesmsg.contentTypefor similar checks (“104”, “3”, etc.). Verify which field is authoritative. - Consider replacing the class-based lookup with the existing
data-msgkeyattribute you already rely on later – this avoids CSS validity issues entirely.
Example refactor:
- if ($applozic('#mck-message-cell .' + msg.key).length > 0 ...
+ if (document.querySelector(`div[data-msgkey="${msg.key}"]`) && ...📝 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 ( | |
| $applozic('#mck-message-cell .' + msg.key).length > 0 && | |
| !(CURRENT_GROUP_DATA.TOKENIZE_RESPONSE && msg.type !== 5) | |
| !(msg.tokenMessage && msg.type !== 5) | |
| ) { | |
| // if message with same key already rendered skiping rendering it again. | |
| return; | |
| } | |
| if (CURRENT_GROUP_DATA.TOKENIZE_RESPONSE && !msg.tokenMessage) { | |
| // deleting tokenized message after receiving complete message from chat server | |
| const element = document.querySelector(`div[data-msgkey="tokenized_response"]`); | |
| if (element) { | |
| console.log('deleted'); | |
| element.remove(); | |
| genAiService.resetState(); | |
| } | |
| } | |
| if ( | |
| - $applozic('#mck-message-cell .' + msg.key).length > 0 && | |
| + document.querySelector(`div[data-msgkey="${msg.key}"]`) && | |
| !(msg.tokenMessage && msg.type !== 5) | |
| ) { | |
| // if message with same key already rendered skiping rendering it again. | |
| return; | |
| } | |
| if (CURRENT_GROUP_DATA.TOKENIZE_RESPONSE && !msg.tokenMessage) { | |
| // deleting tokenized message after receiving complete message from chat server | |
| const element = document.querySelector(`div[data-msgkey="tokenized_response"]`); | |
| if (element) { | |
| console.log('deleted'); | |
| element.remove(); | |
| genAiService.resetState(); | |
| } | |
| } |
🤖 Prompt for AI Agents
In webplugin/js/app/mck-sidebox-1.0.js around lines 7993 to 8010, the duplicate
message filter uses a class selector with msg.key, which may be invalid as a CSS
class and can cause edge-case failures. Also, the code inconsistently checks
msg.type instead of msg.contentType for message type validation. To fix this,
replace the class-based selector with a querySelector that uses the data-msgkey
attribute for message lookup, ensuring valid selectors. Additionally, verify and
consistently use the correct field (likely msg.contentType) for type checks to
align with the rest of the code.
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