fixed position of answer feedback icons #1376
Conversation
WalkthroughIntroduces per-message visibility control and delayed positioning for the feedback UI in answer-feedback-service, and conditionally attaches feedback listeners in mck-sidebox based on showHelpfulButtons computed via handleFeedbackBtnVisible. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant SB as Sidebox (mck-sidebox)
participant AFS as AnswerFeedbackService
participant DOM as DOM/UI
SB->>AFS: handleFeedbackBtnVisible(msg, floatWhere, contact)
AFS-->>SB: showHelpfulButtons (bool)
alt showHelpfulButtons == true
SB->>AFS: attachEventListeners(msg,...)
AFS->>DOM: Hide feedback container (vis->n-vis)
AFS->>DOM: Set per-message selectors
AFS->>AFS: setTimeout(500ms) position
AFS->>DOM: Compute & apply position
else
SB-->>SB: Skip attaching listeners
end
sequenceDiagram
autonumber
actor U as User
participant AFS as AnswerFeedbackService
participant DOM as DOM/UI
participant H as Handlers
U->>AFS: Select feedback (helpful/not)
AFS->>DOM: Show container (n-vis->vis), hide sticker (vis->n-vis)
alt helpful
AFS->>H: helpFulOnClick(msg,...)
else not helpful
AFS->>H: notHelpFulOnClick(msg,...)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
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. 🔧 Biome (2.1.2)webplugin/js/app/mck-sidebox-1.0.jsThe --json option is unstable/experimental and its output might change between patches/minor releases. ... [truncated 99999690 characters] ... g-12").after(elem);\n } else {\n console.log('msg.contentType === 23 && metadata.msg_type === BUTTON');\n Biome encountered an unexpected error This is a bug in Biome, not an error in your code, and we would appreciate it if you could report it to https://github.com/biomejs/biome/issues/ along with the following information to help us fixing the issue. When opening the issue, please provide a minimal reproduction, or identify and share the file/code that triggers it. Without a way to reproduce the error, the error can't be fixed: Source Location: crates/biome_console/src/lib.rs:151:14 🔧 ast-grep (0.38.6)webplugin/js/app/mck-sidebox-1.0.js✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![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
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)
89-96: Unsafe backward traversal can throw when previousSibling is nullIf metadata.KM_CONTEXT_QUESTION is falsy and there’s no right-aligned user message in the chain, question becomes undefined and question = question.previousSibling will throw. Guard the loop and use optional chaining when reading question.msg.
Apply this diff:
- // find the user message - while (!metadata.KM_CONTEXT_QUESTION && question?.floatWhere !== 'mck-msg-right') { - question = question.previousSibling; - } + // find the user message (right-aligned); stop if chain ends + if (!metadata.KM_CONTEXT_QUESTION) { + while (question && question.floatWhere !== 'mck-msg-right') { + question = question.previousSibling; + } + } @@ - const questionText = question.msg.message; + const questionText = question?.msg?.message;
🧹 Nitpick comments (9)
webplugin/js/app/mck-sidebox-1.0.js (4)
8452-8456: Guard may misfire if showHelpfulButtons is a string like 'false'Elsewhere in this file we compare metadata flags to the string 'true'. If
showHelpfulButtonsever becomes a string,'false'is truthy and will still attach listeners. Normalize to a strict boolean at source, or harden this guard.Apply one of the following diffs:
Option A — minimal hardening at the call site:
- showHelpfulButtons && - answerFeedbackService.attachEventListeners({ + (showHelpfulButtons === true || String(showHelpfulButtons).toLowerCase() === 'true') && + answerFeedbackService.attachEventListeners({ msg, assigneeKey: groupAssigneeKey, });Option B — preferred: ensure boolean earlier where
showHelpfulButtonsis computed (no change needed here). If you want a local readability tweak too, see next comment’s if-statement suggestion.
8452-8456: Prefer explicit if over short-circuit for side effectsShort-circuit with && hides control flow and complicates later logging/metrics. An explicit block reads clearer and is easier to extend.
Apply:
- showHelpfulButtons && - answerFeedbackService.attachEventListeners({ - msg, - assigneeKey: groupAssigneeKey, - }); + if (showHelpfulButtons) { + answerFeedbackService.attachEventListeners({ + msg, + assigneeKey: groupAssigneeKey, + }); + }
8452-8456: Ensure idempotent listener attachment to avoid duplicates across re-rendersThis path can execute multiple times per message (e.g., message updates, DOM reflows). Verify
answerFeedbackService.attachEventListenersis idempotent (e.g., checks a data-flag or removes existing listeners) to prevent duplicated handlers.If not already handled inside the service, consider marking the container once wired:
- Inside the service: set
el.dataset.kmFeedbackWired = '1'and early-return if already set.- Or use
{ once: true }where applicable withaddEventListener.
Would you like a patch in answer-feedback-service.js to add this guard?
8452-8456: Wrap listener wiring in try/catch to isolate UI failuresIf DOM nodes are missing or change during delayed positioning,
attachEventListenerscould throw and block the rest of message rendering.Apply:
- if (showHelpfulButtons) { - answerFeedbackService.attachEventListeners({ - msg, - assigneeKey: groupAssigneeKey, - }); - } + if (showHelpfulButtons) { + try { + answerFeedbackService.attachEventListeners({ + msg, + assigneeKey: groupAssigneeKey, + }); + } catch (e) { + // Replace with project logger if available + console.error('Failed to attach answer feedback listeners', e, { messageKey: msg?.key }); + } + }webplugin/js/app/components/answer-feedback-service.js (5)
164-172: Redundant hide and premature DOM ops inside handleFeedbackPositioningYou’re hiding the container here and again at attach time. The double swap increases flicker risk and does work before confirming DOM nodes exist. Prefer hiding once (at attach), then only compute and set position here.
Apply this diff to drop the extra hide call:
- const feedbackContainerSelector = `[data-msgKey="${msgKey}"] .km-answer-feedback`; - kommunicateCommons.modifyClassList( - { - class: [feedbackContainerSelector], - }, - 'vis', - 'n-vis', - true - );If you want to keep a defensive hide, move it below the null checks (after Line 178) and only when feedbackElement exists.
208-211: Avoid fixed 500 ms delay; use layout-driven schedulingA hardcoded 500 ms can be flaky across devices and render paths. Prefer running positioning on the next paint (or when the element’s dimensions stabilize) via requestAnimationFrame or a ResizeObserver.
Minimal change:
- setTimeout(() => { - this.handleFeedbackPositioning(msg.key); - }, 500); + requestAnimationFrame(() => { + this.handleFeedbackPositioning(msg.key); + });More robust (optional): attach a one-shot ResizeObserver to feedbackElement and run handleFeedbackPositioning when its box has a non-zero width, then disconnect.
66-73: Missed in-memory metadata update when metadata was undefinedWhen result.code == 'SUCCESS' and metadata was previously undefined, you update a local object but don’t persist it back to the msg. Persist the merged metadata to keep UI state consistent without relying on a subsequent fetch.
Apply this diff:
- if (result.code == 'SUCCESS') { - const msgMeta = this.msgMap[data.msg.key]?.msg?.metadata || {}; - msgMeta.KM_ANSWER_FEEDBACK = feedback; + if (result.code == 'SUCCESS') { + const msgObj = this.msgMap[data.msg.key]; + if (msgObj?.msg) { + msgObj.msg.metadata = { + ...(msgObj.msg.metadata || {}), + KM_ANSWER_FEEDBACK: feedback, + }; + }
191-196: Null-guard queried elements before adding listenersIf the container renders late or conditionally (per showHelpfulButtons), these queries can return null, and addEventListener will throw. Guard and bail out gracefully.
Example guard (outside diff scope):
const stickySticker = msgContainer?.querySelector('.mck-msg-feedback-sticker'); const helpfulButton = msgContainer?.querySelector('.answer-feedback-helpful'); const notHelpfulButton = msgContainer?.querySelector('.answer-feedback-not-helpful'); if (!stickySticker || !helpfulButton || !notHelpfulButton) { // Element(s) not yet in DOM; consider retrying once via rAF or a short timeout. return; }If you expect delayed render, consider moving listener binding into the same rAF used for positioning or using event delegation from msgContainer.
181-183: Magic numbers for positioning; consider CSS clamp or computed widthsleft = msgBoxWidth + 40 with a 120 px floor is brittle across themes and zoom levels.
Consider expressing this in CSS with clamp(), or compute against the actual sticker/container widths. Example:
const minLeft = 120; const offset = 40; const left = Math.max(minLeft, msgBox.offsetWidth + offset); feedbackElement.style.left = `${left}px`;Or move to CSS: left: clamp(120px, calc(var(--msg-box-width) + 40px), 100vw).
📜 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.
📒 Files selected for processing (2)
webplugin/js/app/components/answer-feedback-service.js(2 hunks)webplugin/js/app/mck-sidebox-1.0.js(1 hunks)
🔇 Additional comments (2)
webplugin/js/app/mck-sidebox-1.0.js (1)
8452-8456: Nice: conditionalizing listener wiring reduces unnecessary workAttaching listeners only when the UI is intended to show is a good perf/readability win and aligns with the new per-message visibility logic in the service.
webplugin/js/app/components/answer-feedback-service.js (1)
217-224: Verify intended UX: setFeedback currently shows container and hides stickerTypical flow is: after choosing feedback, hide the container and show the sticker with the selected icon. Your current swaps do the opposite. If that’s not intended, invert the class swaps below.
Proposed diff if you want “container -> sticker after selection”:
- kommunicateCommons.modifyClassList( - { - class: [feedbackContainerSelector], - }, - 'n-vis', - 'vis', - true - ); + // hide container after selection + kommunicateCommons.modifyClassList( + { + class: [feedbackContainerSelector], + }, + 'vis', + 'n-vis', + true + ); @@ - kommunicateCommons.modifyClassList( - { - class: [stickyStickerSelector], - }, - 'vis', - 'n-vis', - true - ); + // show sticker with chosen icon + kommunicateCommons.modifyClassList( + { + class: [stickyStickerSelector], + }, + 'n-vis', + 'vis', + true + );If the current behavior is by design, ignore this.
Also applies to: 226-233
| // Initially hide the feedback container | ||
| kommunicateCommons.modifyClassList( | ||
| { | ||
| class: [feedbackContainerSelector], | ||
| }, | ||
| 'n-vis', | ||
| 'vis', | ||
| true | ||
| ); |
There was a problem hiding this comment.
Incorrect initial visibility toggle: this shows the container instead of hiding it
The comment says “Initially hide the feedback container,” but passing ('n-vis', 'vis') to modifyClassList removes n-vis and adds vis, which makes it visible. Invert the args to actually hide it.
Apply this diff:
- // Initially hide the feedback container
- kommunicateCommons.modifyClassList(
- {
- class: [feedbackContainerSelector],
- },
- 'n-vis',
- 'vis',
- true
- );
+ // Initially hide the feedback container
+ kommunicateCommons.modifyClassList(
+ {
+ class: [feedbackContainerSelector],
+ },
+ 'vis',
+ 'n-vis',
+ 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.
| // Initially hide the feedback container | |
| kommunicateCommons.modifyClassList( | |
| { | |
| class: [feedbackContainerSelector], | |
| }, | |
| 'n-vis', | |
| 'vis', | |
| true | |
| ); | |
| // Initially hide the feedback container | |
| kommunicateCommons.modifyClassList( | |
| { | |
| class: [feedbackContainerSelector], | |
| }, | |
| 'vis', | |
| 'n-vis', | |
| true | |
| ); |
🤖 Prompt for AI Agents
In webplugin/js/app/components/answer-feedback-service.js around lines 197 to
205, the call to kommunicateCommons.modifyClassList currently passes ('n-vis',
'vis') which removes 'n-vis' and adds 'vis' (making the container visible)
contrary to the intent to hide it; swap the class arguments so the call removes
'vis' and adds 'n-vis' (e.g., pass 'vis' then 'n-vis' with the same boolean
flag) to actually hide the feedback container.
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
Improvements
Bug Fixes