Skip to content

Conversation

thakaresakshi121
Copy link
Contributor

@thakaresakshi121 thakaresakshi121 commented Aug 18, 2025

What do you want to achieve?

  • fixed answer feedback icon position

PR Checklist

  • I have tested it locally and all functionalities are working fine.
  • I have compared it with mocks and all design elements are the same.
  • I have tested it in IE Browser.

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?

image

Screenshot

NOTE: Make sure you're comparing your branch with the correct base branch

Summary by CodeRabbit

  • Bug Fixes
    • Fixed misaligned or hidden answer feedback on messages, including rich text and attachments.
    • Positioning now updates reliably after content loads and when message content changes.
    • Cleaned up feedback state when messages or conversations are deleted to prevent stray UI artifacts.
  • Style
    • Feedback component now stays above overlapping elements, remains clickable, and animates smoothly when repositioning.

Copy link

github-actions bot commented Aug 18, 2025

Warnings
⚠️ Please add the JIRA issue key to the PR title (e.g. FW-123)
Messages
📖 New Files in this PR: - 0

Generated by 🚫 dangerJS against bc9607f

Copy link
Contributor

coderabbitai bot commented Aug 18, 2025

Walkthrough

Adds dynamic feedback positioning with retrying and MutationObserver-based repositioning, tracks per-message observers/timeouts, wires positioning through retries, updates CSS for stacking/interaction/transition, and cleans up observers when messages or conversations are deleted.

Changes

Cohort / File(s) Summary
Answer feedback positioning & observers
webplugin/js/app/components/answer-feedback-service.js
Replace fixed-width positioning with aggregated max-width calculation (+40px, clamped); add retryFeedbackPositioning(msgKey, maxRetries, delay) to retry until render ready; add setupMutationObserver and disconnectObserver; track repositionTimeouts and observers; debounce repositioning; wire attachEventListeners to retry flow; add observer cleanup method.
APZ client — observer cleanup
webplugin/js/app/apz-client-1.0.js
On message and conversation deletion paths, disconnect per-message or all answerFeedbackService observers where present; add guards checking Kommunicate.ui.answerFeedbackService before calling disconnectObserver; add local messageKey usage in relevant branches.
Feedback UI styling
webplugin/scss/components/_km-message-area.scss
Add z-index: 10; pointer-events: auto; transition: left 0.1s ease-out; to .km-answer-feedback.

Sequence Diagram(s)

sequenceDiagram
  participant UI as AnswerFeedback
  participant DOM as Message DOM
  participant Timer as Retry Timer
  participant MO as MutationObserver
  Note over UI,DOM #f0f4ff: Positioning flow after message render
  UI->>DOM: attachEventListeners(msgKey)
  UI->>Timer: retryFeedbackPositioning(msgKey, maxRetries, delay)
  Timer-->>UI: attempt positioning
  UI->>DOM: read widths (msg-box, rich-text, attachments, text)
  alt widths available
    UI->>DOM: handleFeedbackPositioning (set left/bottom, show)
    UI->>MO: setupMutationObserver(msgKey, container)
    MO-->>UI: on DOM changes (debounced)
    UI->>DOM: recompute & update positioning
  else widths not ready
    Timer-->>UI: retry until maxRetries
  end
  Note over UI,DOM #fff7f0: Cleanup on delete
  DOM->>UI: message/conversation deleted
  UI->>MO: disconnectObserver(msgKey) / disconnect all
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested labels

codex

Suggested reviewers

  • devashishmamgain
✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch new-CUS-1125

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (3)
webplugin/scss/components/_km-message-area.scss (1)

45-47: Confirm stacking order; consider transform-based transition for smoother repositioning

  • z-index: 10 might still fall under other floating UI (menus/tooltips). Please validate in all chat states (context menu open, attachment previews, etc.).
  • Transitioning left causes layout reflow; transform-based transitions are smoother. If keeping left for now, a small hint helps browsers.

Apply this incremental tweak to hint the browser and make the intent explicit:

 .km-answer-feedback {
   position: absolute;
   bottom: 5px;
   display: flex !important;
   justify-content: flex-end;
   align-items: center;
   gap: 6px;
   z-index: 10;
   pointer-events: auto;
   transition: left 0.1s ease-out;
+  will-change: left;
 }

Optionally, we can switch to translateX for smoother GPU-accelerated transitions and update JS to set transform instead of left. Want me to draft that refactor?

webplugin/js/app/components/answer-feedback-service.js (2)

171-198: Reduce magic numbers and remove redundant bottom assignment

  • The constants 60 (container margin), 40 (icon gap), and 120 (min left) are magic numbers. Make them self-documenting.
  • bottom is already set via CSS (Line 40 in SCSS); setting it again on every reposition is redundant.

Proposed cleanup:

-        const msgBoxHeight = msgBox.offsetHeight;
+        const msgBoxHeight = msgBox.offsetHeight; // retained if needed later

         const richMsgBox = msgContainer.querySelector('.mck-msg-box-rich-text-container');
         const attachmentBox = msgContainer.querySelector('.mck-attachment-box');
         const msgText = msgContainer.querySelector('.mck-msg-text');

         let finalWidth = msgBoxWidth;
         if (richMsgBox && richMsgBox.offsetWidth > 0) {
             finalWidth = Math.max(finalWidth, richMsgBox.offsetWidth);
         }

         if (attachmentBox && attachmentBox.offsetWidth > 0) {
             finalWidth = Math.max(finalWidth, attachmentBox.offsetWidth);
         }

         if (msgText && msgText.offsetWidth > 0) {
             finalWidth = Math.max(finalWidth, msgText.offsetWidth);
         }

-        const containerWidth = msgContainer.offsetWidth;
-        const maxAllowedWidth = containerWidth - 60; // Leave some margin
-        const feedbackWidth = Math.min(Math.max(finalWidth + 40, 120), maxAllowedWidth);
-
-        feedbackElement.style.left = `${feedbackWidth}px`;
-        if (msgBoxHeight > 0) {
-            feedbackElement.style.bottom = '5px';
-        }
+        const containerWidth = msgContainer.offsetWidth;
+        const CONTAINER_MARGIN = 60; // leave some margin with container edges
+        const ICON_GAP = 40; // spacing between bubble edge and feedback
+        const MIN_LEFT = 120; // minimum left to avoid overlapping short bubbles
+        const maxAllowedWidth = containerWidth - CONTAINER_MARGIN;
+        const feedbackLeft = Math.min(Math.max(finalWidth + ICON_GAP, MIN_LEFT), maxAllowedWidth);
+        feedbackElement.style.left = `${feedbackLeft}px`;
         feedbackElement.style.visibility = 'visible';

200-221: Retry strategy may be too short for slow, async content; make it configurable

Three attempts at 100ms can miss late-loading images or rich components. Allow configuration from options and slightly raise defaults.

-    retryFeedbackPositioning = (msgKey, maxRetries = 3, delay = 100) => {
+    retryFeedbackPositioning = (
+        msgKey,
+        maxRetries = this.options?.answerFeedback?.maxRetries ?? 10,
+        delay = this.options?.answerFeedback?.retryDelay ?? 120
+    ) => {
         let retryCount = 0;

         const attemptPositioning = () => {
             const msgContainer = document.querySelector(`[data-msgKey='${msgKey}']`);
             if (!msgContainer) return;

             const msgBox = msgContainer.querySelector('.mck-msg-box');
             if (!msgBox || msgBox.offsetWidth === 0) {
                 if (retryCount < maxRetries) {
                     retryCount++;
-                    setTimeout(attemptPositioning, delay);
+                    setTimeout(attemptPositioning, delay);
                 }
                 return;
             }
             this.handleFeedbackPositioning(msgKey);
             this.setupMutationObserver(msgKey, msgContainer);
         };

         attemptPositioning();
     };

If you prefer, we can also provide a requestAnimationFrame-based variant to avoid arbitrary delays.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 93f89de and 4609451.

📒 Files selected for processing (2)
  • webplugin/js/app/components/answer-feedback-service.js (2 hunks)
  • webplugin/scss/components/_km-message-area.scss (1 hunks)
🔇 Additional comments (1)
webplugin/js/app/components/answer-feedback-service.js (1)

271-273: LGTM: deferring positioning until render via retry reduces flakiness

Switching to a retry-based approach is a good call for async content.

To further harden:

  • Reposition on window resize (viewport changes) since MutationObserver won’t catch it.
  • Verify late-loading images/iframes in rich messages still reposition correctly (ResizeObserver would cover this best).
    Want me to add a guarded window resize handler or a ResizeObserver-based implementation?

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
webplugin/js/app/components/answer-feedback-service.js (1)

90-99: Prevent infinite loop when walking back to the user’s question; guard nulls

If there is no prior right-floated user message, question can become undefined, and the loop will never terminate. Also, dereferencing question.msg will throw when question is missing.

Apply this patch to make the traversal safe and avoid crashes:

-        // find the user message
-        while (!metadata.KM_CONTEXT_QUESTION && question?.floatWhere !== 'mck-msg-right') {
-            question = question.previousSibling;
-        }
+        // find the user message
+        while (!metadata.KM_CONTEXT_QUESTION && question && question.floatWhere !== 'mck-msg-right') {
+            question = question.previousSibling;
+        }
@@
-        const answerText = answer.msg.message;
-        const questionText = question.msg.message;
+        const answerText = answer?.msg?.message || '';
+        const questionText = question?.msg?.message || '';
♻️ Duplicate comments (2)
webplugin/js/app/components/answer-feedback-service.js (2)

26-26: Per-message debounce map added — addresses earlier leak/coordination concerns

Good call introducing this.repositionTimeouts = Object.create(null). This resolves the single-shared-timeout issue noted earlier and prevents cross-message interference.


223-264: Observer callback and per-message debounce look solid

  • closest(...) ancestor checks correctly capture deep descendant mutations.
  • Debouncing per msgKey prevents cross-message cancellation.
  • Disconnecting any prior observer for the same key avoids leaks/duplicates.

Nice work addressing the earlier concerns.

🧹 Nitpick comments (4)
webplugin/js/app/components/answer-feedback-service.js (2)

171-199: Clamp computed left to avoid negative offsets in narrow containers

When msgContainer.offsetWidth < 60, maxAllowedWidth becomes negative and left can be set to a negative pixel value, pushing the feedback off-canvas.

Patch:

-        const maxAllowedWidth = containerWidth - 60; // Leave some margin
-        const feedbackWidth = Math.min(Math.max(finalWidth + 40, 120), maxAllowedWidth);
+        const maxAllowedWidth = Math.max(containerWidth - 60, 120); // Ensure non-negative and reasonable minimum
+        const feedbackWidth = Math.min(Math.max(finalWidth + 40, 120), maxAllowedWidth);

Please sanity-check on very small iframes/containers (e.g., < 200px wide) to confirm desired behavior.


126-131: Minor naming nit: helpFulOnClick → helpfulOnClick

Spelling consistency aids discoverability. Consider renaming both helpers to helpfulOnClick and notHelpfulOnClick.

Confirm no external callers rely on the current names.

webplugin/js/app/apz-client-1.0.js (2)

81-95: De-duplicate “disconnect all” logic via a service helper

This loop is repeated elsewhere. To reduce duplication and centralize behavior, add a disconnectAllObservers() method on answerFeedbackService and call it here.

If you prefer not to add a new method, at least extract this loop to a local helper to avoid future drift.


556-571: Centralize “disconnect all” on conversation delete to reduce duplication

Same suggestion as earlier block: call a disconnectAllObservers() on the service instead of re-implementing the loop here.

I can draft disconnectAllObservers() on the service and update all call sites in this PR if you’d like.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 4609451 and bc9607f.

📒 Files selected for processing (2)
  • webplugin/js/app/apz-client-1.0.js (3 hunks)
  • webplugin/js/app/components/answer-feedback-service.js (3 hunks)
🔇 Additional comments (5)
webplugin/js/app/components/answer-feedback-service.js (2)

289-291: LGTM: Defer-and-retry positioning post-render

Using retryFeedbackPositioning before attaching observers is a pragmatic way to handle late layout. Good sequencing.


223-251: Compatibility check: optional chaining in hot paths

The observer callback uses el.closest?.(...) and other areas use ?.. If this file is shipped without transpilation to ES2019+ targets, older environments (e.g., legacy WebViews) will break.

If your build transpiles this file, ignore. Otherwise, add a guard or transpile.

Would you like a patch replacing ?. with guarded calls for environments without transpilation?

webplugin/js/app/apz-client-1.0.js (3)

69-77: Per-message observer cleanup on delete looks good

The guard and targeted disconnectObserver(obj.messageKey) is correct and idempotent.


535-549: APPLOZIC_05: immediate per-message cleanup is correct

Deriving messageKey once and using it for both the event and the cleanup keeps things coherent. Double-calling disconnectObserver (here and in onMessageDeleted) is harmless due to idempotency.


535-538: ES5 compatibility assured via Babel transpilation

The webplugin/.babelrc is configured with @babel/preset-env targeting IE11, and the project’s build pipeline (via gulp and gulp-babel) compiles all JS under webplugin/ before release. As a result, any const declarations—including those in apz-client-1.0.js—will be downleveled to var for legacy browsers. No change is needed here.

Comment on lines 305 to 314
resp.status = data === 'success' ? 'success' : 'error';
if (
resp.status === 'success' &&
window.Kommunicate &&
window.Kommunicate.ui &&
window.Kommunicate.ui.answerFeedbackService
) {
window.Kommunicate.ui.answerFeedbackService.disconnectObserver(key);
}
if (typeof callback === 'function') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Fix ReferenceError in deleteConversation success path (key is undefined)

disconnectObserver(key) references an undefined identifier. This will throw on successful conversation delete.

Replace with a safe, existing cleanup pattern:

-                            if (
-                                resp.status === 'success' &&
-                                window.Kommunicate &&
-                                window.Kommunicate.ui &&
-                                window.Kommunicate.ui.answerFeedbackService
-                            ) {
-                                window.Kommunicate.ui.answerFeedbackService.disconnectObserver(key);
-                            }
+                            if (
+                                resp.status === 'success' &&
+                                window.Kommunicate &&
+                                window.Kommunicate.ui &&
+                                window.Kommunicate.ui.answerFeedbackService
+                            ) {
+                                var afs = window.Kommunicate.ui.answerFeedbackService;
+                                if (afs.observers) {
+                                    Object.keys(afs.observers).forEach(function (msgKey) {
+                                        afs.disconnectObserver(msgKey);
+                                    });
+                                }
+                            }

If you can associate observers with a conversation id, prefer filtering instead of clearing all.


🏁 Script executed:

#!/bin/bash
# Verify whether `key` is defined in the deleteConversation success callback
rg -nC5 'disconnectObserver' webplugin/js/app/apz-client-1.0.js
rg -nC5 'deleteConversation' webplugin/js/app/apz-client-1.0.js

Length of output: 3659


Here’s the full deleteConversation implementation and its success handler for context:


🏁 Script executed:

#!/bin/bash
# Print lines 280–350 to inspect deleteConversation and its callback
sed -n '280,350p' webplugin/js/app/apz-client-1.0.js

Length of output: 3197


Fix undefined key in deleteConversation success handler

The deleteConversation AJAX success callback at webplugin/js/app/apz-client-1.0.js:305–314 calls

window.Kommunicate.ui.answerFeedbackService.disconnectObserver(key);

but key is never declared in that scope, causing a ReferenceError on successful delete.

To resolve:

  • Remove the incorrect disconnectObserver(key) call.
  • Instead, use the existing answerFeedbackService.observers map to safely disconnect all observers, or—if you have a mapping from conversation IDs to specific observer keys—filter by the relevant ID rather than clearing every observer.

Suggested patch:

-    if (
-        resp.status === 'success' &&
-        window.Kommunicate &&
-        window.Kommunicate.ui &&
-        window.Kommunicate.ui.answerFeedbackService
-    ) {
-        window.Kommunicate.ui.answerFeedbackService.disconnectObserver(key);
-    }
+    if (
+        resp.status === 'success' &&
+        window.Kommunicate &&
+        window.Kommunicate.ui &&
+        window.Kommunicate.ui.answerFeedbackService
+    ) {
+        var afs = window.Kommunicate.ui.answerFeedbackService;
+        if (afs.observers) {
+            Object.keys(afs.observers).forEach(function (msgKey) {
+                afs.disconnectObserver(msgKey);
+            });
+        }
+    }

If you later introduce a way to link params.id (conversation or user ID) to observer keys, replace the .forEach with a filter that only disconnects matching observers.

🤖 Prompt for AI Agents
In webplugin/js/app/apz-client-1.0.js around lines 305 to 314, the success
handler calls
window.Kommunicate.ui.answerFeedbackService.disconnectObserver(key) but `key` is
undefined causing a ReferenceError; remove that call and instead safely
disconnect observers by iterating over
window.Kommunicate.ui.answerFeedbackService.observers (after null checks) and
calling disconnectObserver for each matching observer or for all if no mapping
exists; if you later map params.id to observer keys, replace the iteration with
a filtered iteration that only disconnects observers tied to that conversation
ID.

Comment on lines +26 to 27
this.repositionTimeouts = Object.create(null);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Initialize retry timer map in constructor

Add one line under the existing repositionTimeouts init:

 this.msgKeyMap = [];
 this.repositionTimeouts = Object.create(null);
+this.retryTimeouts = Object.create(null);
📝 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.

Suggested change
this.repositionTimeouts = Object.create(null);
}
constructor() {
// existing mappings
this.msgKeyMap = [];
this.repositionTimeouts = Object.create(null);
// initialize retry timer map
this.retryTimeouts = Object.create(null);
}
🤖 Prompt for AI Agents
In webplugin/js/app/components/answer-feedback-service.js around lines 26 to 27,
add initialization for the retry timer map directly under the existing
this.repositionTimeouts = Object.create(null); line — e.g. set this.retryTimers
= Object.create(null); — so the property exists before any code references it;
maintain the same placement and spacing as the surrounding constructor code.

Comment on lines +201 to 221
retryFeedbackPositioning = (msgKey, maxRetries = 3, delay = 100) => {
let retryCount = 0;

const attemptPositioning = () => {
const msgContainer = document.querySelector(`[data-msgKey='${msgKey}']`);
if (!msgContainer) return;

const msgBox = msgContainer.querySelector('.mck-msg-box');
if (!msgBox || msgBox.offsetWidth === 0) {
if (retryCount < maxRetries) {
retryCount++;
setTimeout(attemptPositioning, delay);
}
return;
}
this.handleFeedbackPositioning(msgKey);
this.setupMutationObserver(msgKey, msgContainer);
};

const feedbackWidth = msgBoxWidth + 40;
feedbackElement.style.left = `${feedbackWidth > 120 ? feedbackWidth : 120}px`;
attemptPositioning();
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Track and clear retry timers to avoid dangling timeouts and post-removal callbacks

retryFeedbackPositioning schedules timeouts that aren’t tracked/cleared. If a message is removed before retries finish, callbacks can run against detached DOM nodes and cause needless work.

  • Store retry timer per message (e.g., this.retryTimeouts[msgKey]).
  • Clear and delete it on success and in disconnectObserver.

Patch for this block:

-                if (retryCount < maxRetries) {
-                    retryCount++;
-                    setTimeout(attemptPositioning, delay);
-                }
+                if (retryCount < maxRetries) {
+                    retryCount++;
+                    // Track per-message retry timer
+                    this.retryTimeouts = this.retryTimeouts || Object.create(null);
+                    clearTimeout(this.retryTimeouts[msgKey]);
+                    this.retryTimeouts[msgKey] = setTimeout(attemptPositioning, delay);
+                }
                 return;
             }
-            this.handleFeedbackPositioning(msgKey);
+            this.handleFeedbackPositioning(msgKey);
+            // Clear any outstanding retry timer for this message
+            if (this.retryTimeouts?.[msgKey]) {
+                clearTimeout(this.retryTimeouts[msgKey]);
+                delete this.retryTimeouts[msgKey];
+            }
             this.setupMutationObserver(msgKey, msgContainer);

And initialize the map in the constructor (see a separate note below), plus clear it in disconnectObserver.

📝 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.

Suggested change
retryFeedbackPositioning = (msgKey, maxRetries = 3, delay = 100) => {
let retryCount = 0;
const attemptPositioning = () => {
const msgContainer = document.querySelector(`[data-msgKey='${msgKey}']`);
if (!msgContainer) return;
const msgBox = msgContainer.querySelector('.mck-msg-box');
if (!msgBox || msgBox.offsetWidth === 0) {
if (retryCount < maxRetries) {
retryCount++;
setTimeout(attemptPositioning, delay);
}
return;
}
this.handleFeedbackPositioning(msgKey);
this.setupMutationObserver(msgKey, msgContainer);
};
const feedbackWidth = msgBoxWidth + 40;
feedbackElement.style.left = `${feedbackWidth > 120 ? feedbackWidth : 120}px`;
attemptPositioning();
};
retryFeedbackPositioning = (msgKey, maxRetries = 3, delay = 100) => {
let retryCount = 0;
const attemptPositioning = () => {
const msgContainer = document.querySelector(`[data-msgKey='${msgKey}']`);
if (!msgContainer) return;
const msgBox = msgContainer.querySelector('.mck-msg-box');
if (!msgBox || msgBox.offsetWidth === 0) {
if (retryCount < maxRetries) {
retryCount++;
// Track per-message retry timer
this.retryTimeouts = this.retryTimeouts || Object.create(null);
clearTimeout(this.retryTimeouts[msgKey]);
this.retryTimeouts[msgKey] = setTimeout(attemptPositioning, delay);
}
return;
}
this.handleFeedbackPositioning(msgKey);
// Clear any outstanding retry timer for this message
if (this.retryTimeouts?.[msgKey]) {
clearTimeout(this.retryTimeouts[msgKey]);
delete this.retryTimeouts[msgKey];
}
this.setupMutationObserver(msgKey, msgContainer);
};
attemptPositioning();
};
🤖 Prompt for AI Agents
In webplugin/js/app/components/answer-feedback-service.js around lines 201 to
221, retryFeedbackPositioning schedules setTimeouts that aren’t tracked so timed
callbacks can run after a message is removed; store each timeout id on
this.retryTimeouts[msgKey] (ensure this.retryTimeouts is initialized in the
constructor), clear and delete the timeout when positioning succeeds (before
calling handleFeedbackPositioning and setupMutationObserver), and also clear and
delete any pending timeout for that msgKey inside disconnectObserver so no
dangling timeouts fire against removed DOM nodes.

Comment on lines +266 to +275
disconnectObserver(msgKey) {
if (this.observers?.[msgKey]) {
this.observers[msgKey].disconnect();
delete this.observers[msgKey];
}
if (this.repositionTimeouts?.[msgKey]) {
clearTimeout(this.repositionTimeouts[msgKey]);
delete this.repositionTimeouts[msgKey];
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Also clear retry timers during teardown

To complete the teardown story and avoid late callbacks after message removal:

     disconnectObserver(msgKey) {
         if (this.observers?.[msgKey]) {
             this.observers[msgKey].disconnect();
             delete this.observers[msgKey];
         }
         if (this.repositionTimeouts?.[msgKey]) {
             clearTimeout(this.repositionTimeouts[msgKey]);
             delete this.repositionTimeouts[msgKey];
         }
+        if (this.retryTimeouts?.[msgKey]) {
+            clearTimeout(this.retryTimeouts[msgKey]);
+            delete this.retryTimeouts[msgKey];
+        }
     }
📝 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.

Suggested change
disconnectObserver(msgKey) {
if (this.observers?.[msgKey]) {
this.observers[msgKey].disconnect();
delete this.observers[msgKey];
}
if (this.repositionTimeouts?.[msgKey]) {
clearTimeout(this.repositionTimeouts[msgKey]);
delete this.repositionTimeouts[msgKey];
}
}
disconnectObserver(msgKey) {
if (this.observers?.[msgKey]) {
this.observers[msgKey].disconnect();
delete this.observers[msgKey];
}
if (this.repositionTimeouts?.[msgKey]) {
clearTimeout(this.repositionTimeouts[msgKey]);
delete this.repositionTimeouts[msgKey];
}
if (this.retryTimeouts?.[msgKey]) {
clearTimeout(this.retryTimeouts[msgKey]);
delete this.retryTimeouts[msgKey];
}
}
🤖 Prompt for AI Agents
In webplugin/js/app/components/answer-feedback-service.js around lines 266 to
275, the teardown currently clears observers and repositionTimeouts but does not
clear any retry timers, which can cause late callbacks after a message is
removed; modify disconnectObserver to also check this.retryTimers?.[msgKey],
call clearTimeout on it if present, and delete this.retryTimers[msgKey] to fully
cancel pending retries during teardown.

@thakaresakshi121
Copy link
Contributor Author

created new
#1376

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants