MGMT-21394: Improve chatbot scroll behavior & msg focusing#3106
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@rawagner: This pull request references MGMT-21394 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.20.0" version, but no target version was set. DetailsIn response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
bot
added
approved
rawagner
force-pushed
the
chatbot_scroll
branch
from
37b3224 to
484e315
Compare
WalkthroughReplaces callback-based bottom scrolling with anchor-based smooth scrolling and dynamic minHeight handling in BotMessage (new initHeight/isLastMsg props, innerRef wiring). ChatBotWindow adds typed MessageBoxHandle ref, last-user message ref, visible-height calculation, passes initHeight to the last bot message, and revises scrolling/send/stream flow. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ChatBotWindow
participant MessageBox
participant Message(User)
participant BotMessage
User->>ChatBotWindow: sendMessage(text)
ChatBotWindow->>MessageBox: ensure ref (msgBoxRef)
ChatBotWindow->>Message(User): render (innerRef -> lastUserMsgRef)
ChatBotWindow->>ChatBotWindow: getVisibleHeight(msgBoxRef, lastUserMsgRef)
ChatBotWindow->>BotMessage: render (initHeight = visibleHeight for last bot msg, isLastMsg=true)
ChatBotWindow->>ChatBotWindow: trigger scroll (auto on first, smooth anchored afterwards)
sequenceDiagram
participant User
participant BotMessage
participant Anchor
User->>BotMessage: open negative feedback
BotMessage->>Anchor: anchor.scrollIntoView({ behavior: "smooth" })
BotMessage->>BotMessage: clear minHeight if content height > stored height
Estimated code review effort🎯 4 (Complex) | ⏱️ ~35 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ 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
CodeRabbit Configuration File (
|
|
@rawagner: This pull request references MGMT-21394 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.20.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
libs/chatbot/lib/components/ChatBot/BotMessage.tsx (2)
151-152: Remove unnecessary React Fragment wrapperThe fragment wrapper is not needed since you're only rendering a single child element.
- return ( - <> - <Message + return ( + <MessageAnd at line 189:
- /> - </> + />Also applies to: 189-189
12-14: Address TypeScript type issue instead of ignoring itUsing
@ts-ignorebypasses TypeScript's type checking. Consider properly typing theMessageLoadingcomponent or filing an issue with the library if there's a type definition problem.libs/chatbot/lib/components/ChatBot/ChatBotWindow.tsx (2)
117-122: Extract magic number as a named constantThe hardcoded value
64should be extracted as a named constant with a comment explaining what it represents (padding, margin, etc.).+ const MESSAGE_SPACING_OFFSET = 64; // Adjust based on your UI spacing requirements + const getVisibleHeight = () => { if (lastUserMsgRef.current && msgBoxRef.current) { - return msgBoxRef.current.clientHeight - lastUserMsgRef.current.clientHeight - 64; + return msgBoxRef.current.clientHeight - lastUserMsgRef.current.clientHeight - MESSAGE_SPACING_OFFSET; } return undefined; };
169-179: Avoid empty catch block that silently ignores errorsThe empty catch block could hide useful error information during debugging. Consider at least logging the error in development mode.
if (!resp.ok) { let errMsg = 'An error occured'; try { const detailMsg = ((await resp.json()) as { detail: string }).detail; if (detailMsg) { errMsg = detailMsg; } - } catch {} + } catch (e) { + // Failed to parse error response, use default message + console.warn('Failed to parse error response:', e); + } throw Error(`${resp.status}: ${errMsg}`); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libs/chatbot/lib/components/ChatBot/BotMessage.tsx(4 hunks)libs/chatbot/lib/components/ChatBot/ChatBotWindow.tsx(10 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
libs/chatbot/lib/components/ChatBot/ChatBotWindow.tsx (1)
libs/chatbot/lib/components/ChatBot/helpers.ts (1)
userRole(11-11)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: translation-files
- GitHub Check: circular-deps
- GitHub Check: tests
- GitHub Check: format
- GitHub Check: unit-tests
- GitHub Check: lint
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ammont82, rawagner The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
rawagner
force-pushed
the
chatbot_scroll
branch
from
484e315 to
76bf689
Compare
|
@rawagner: This pull request references MGMT-21394 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.20.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
libs/chatbot/lib/components/ChatBot/BotMessage.tsx (2)
77-77: Consider usingundefinedas the initial state for heightStarting with
undefinedwould be more semantically correct since the height is unknown initially, rather than potentially using a stale or incorrect value.- const [height, setHeight] = React.useState(initHeight); + const [height, setHeight] = React.useState<number | undefined>(initHeight);
157-157: Empty div used as scroll anchor could be more semanticConsider adding an aria-hidden attribute to the scroll anchor div to ensure it's properly ignored by screen readers.
- <div ref={scrollToMsgRef} /> + <div ref={scrollToMsgRef} aria-hidden="true" />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libs/chatbot/lib/components/ChatBot/BotMessage.tsx(4 hunks)libs/chatbot/lib/components/ChatBot/ChatBotWindow.tsx(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/chatbot/lib/components/ChatBot/ChatBotWindow.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: translation-files
- GitHub Check: tests
- GitHub Check: format
- GitHub Check: unit-tests
- GitHub Check: lint
- GitHub Check: circular-deps
🔇 Additional comments (3)
libs/chatbot/lib/components/ChatBot/BotMessage.tsx (3)
83-93: Good implementation of smooth scrolling with double RAFThe use of double
requestAnimationFrameensures proper layout completion before scrolling, which is a solid approach for avoiding visual glitches.
147-186: Clean component structure with proper ref forwardingThe component properly forwards refs using
innerRefand maintains a clear separation between scroll anchors and content measurement refs. The conditional rendering of loading states and actions is well-organized.
150-150: VerifyminHeightonly on last messagesI confirmed the only application of
minHeightis inlibs/chatbot/lib/components/ChatBot/BotMessage.tsx(line 150), gated byheight && isLastMsg. There are no other height-preservation styles orscrollHeightchecks elsewhere in the codebase. Consequently, non-last messages will naturally resize as their content loads. If you need to prevent layout shifts on earlier messages as well, you’ll want to apply a similarminHeightrule for those cases.• BotMessage.tsx
– height state initialized at line 77
– scrollHeight-based reset effect at lines 96–99
–minHeightapplied only whenisLastMsgat line 150Please review whether this behavior matches your UI requirements.
rawagner
force-pushed
the
chatbot_scroll
branch
from
76bf689 to
e4ae4f9
Compare
|
@rawagner: This pull request references MGMT-21394 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.20.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
libs/chatbot/lib/components/ChatBot/BotMessage.tsx (1)
89-95: useLayoutEffect without deps can cause layout thrashing; add deps or observe size changesThis effect runs after every render and performs a DOM read (scrollHeight). It can trigger unnecessary reflows.
Apply the minimal fix by scoping execution to relevant changes:
-// run on every re-render -// eslint-disable-next-line react-hooks/exhaustive-deps -React.useLayoutEffect(() => { - if (height && !isLoading && msgRef.current && msgRef.current.scrollHeight > height) { - setHeight(undefined); - } -}); +React.useLayoutEffect(() => { + if (height && !isLoading && msgRef.current && msgRef.current.scrollHeight > height) { + setHeight(undefined); + } +}, [height, isLoading]);Optional robustness: if content growth can occur without React re-renders (e.g., images/fonts), prefer a ResizeObserver to eliminate polling-by-render:
React.useLayoutEffect(() => { if (!msgRef.current || !height || isLoading) return; const el = msgRef.current; const ro = new ResizeObserver(() => { if (el.scrollHeight > height) { setHeight(undefined); } }); ro.observe(el); return () => ro.disconnect(); }, [height, isLoading]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libs/chatbot/lib/components/ChatBot/BotMessage.tsx(2 hunks)libs/chatbot/lib/components/ChatBot/ChatBotWindow.tsx(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/chatbot/lib/components/ChatBot/ChatBotWindow.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-18T12:35:50.945Z
Learnt from: celdrake
PR: openshift-assisted/assisted-installer-ui#3051
File: libs/chatbot/lib/components/ChatBot/ChatBotWindow.tsx:196-222
Timestamp: 2025-07-18T12:35:50.945Z
Learning: In the assisted-installer-ui chatbot feedback implementation, the onFeedbackSubmit callback requires access to the messages array to retrieve both the bot response content and the associated user question for the API call, making it necessary to include messages in the useCallback dependency array rather than passing message content as props to avoid duplicating potentially long message data.
Applied to files:
libs/chatbot/lib/components/ChatBot/BotMessage.tsx
🧬 Code Graph Analysis (1)
libs/chatbot/lib/components/ChatBot/BotMessage.tsx (1)
libs/chatbot/lib/components/ChatBot/helpers.ts (1)
MsgProps(4-7)
🔇 Additional comments (3)
libs/chatbot/lib/components/ChatBot/BotMessage.tsx (3)
49-58: Good a11y improvement: focus the feedback form on openSetting focusOnLoad: true improves keyboard and screen reader UX when the feedback form opens.
83-88: Smooth, anchored scroll on opening negative feedback looks goodUsing scrollIntoView with behavior: 'smooth' on a dedicated anchor is a clean replacement for bottom-scroll callbacks and reads well.
60-67: Verify BotMessage API changes across all callersPlease ensure that the following breaking changes in BotMessageProps have been applied consistently:
- The onScrollToBottom prop has been removed everywhere
- Each usage now passes the newly added initHeight and isLastMsg props
You can confirm with these commands:
# No remaining onScrollToBottom references rg -n --pretty 'onScrollToBottom' # All <BotMessage> invocations include initHeight rg -n --pretty '<BotMessage[^>]*initHeight' # All <BotMessage> invocations include isLastMsg rg -n --pretty '<BotMessage[^>]*isLastMsg'
|
/lgtm |
openshift-merge-bot
bot
merged commit 5ebf8ed
into
openshift-assisted:master
|
/cherry-pick releases/v0.1-chatbot |
|
@rawagner: new pull request created: #3108 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
4 participants
Summary by CodeRabbit
New Features
Bug Fixes