-
Notifications
You must be signed in to change notification settings - Fork 237
chore(compass-assistant): add auto-scroll to bottom with entry points and message sending COMPASS-9887 #7374
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
… and message sending COMPASS-9887
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds auto-scroll functionality to the assistant chat component that automatically scrolls to the bottom when new user messages are sent. The feature includes scroll behavior for both message sending and entry points as mentioned in the description.
- Implements auto-scroll to bottom functionality triggered by new user messages
- Adds comprehensive test coverage for the new scroll behavior
- Refactors message handling to be asynchronous with proper chat stopping
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
packages/compass-assistant/src/components/assistant-chat.tsx | Adds scroll-to-bottom logic with refs, useEffect hooks, and async message handling |
packages/compass-assistant/src/components/assistant-chat.spec.tsx | Adds test coverage for scroll behavior and refactors existing test assertions |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
5c1e661
to
6df0be8
Compare
const darkMode = useDarkMode(); | ||
const messagesContainerRef = useRef<HTMLDivElement>(null); | ||
const previousLastMessageId = useRef<string | undefined>(undefined); | ||
const { id: lastMessageId, role: lastMessageIsUser } = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't role "system" | "user" | "assistant"
yet here you're using it as a boolean lastMessageIsUser?
useEffect(() => { | ||
if ( | ||
lastMessageId && | ||
previousLastMessageId.current !== undefined && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to check if there was a message and one before that? If not, what's the harm in scrolling to the bottom if the list is and was empty? If there's a reason we should probably spell it all out in a comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just wanted to avoid unnecessary behavior of scroll to bottom, either is fine.
10be809
to
9c05d92
Compare
scrolling in unit tests isn't really straightforward.... so going to follow up on this in e2e tests separately |
Adds this to both message sending and entry points as it seems to make sense in both cases.
TBC by design.