Fix scroll behavior when switching between chats#2643
Conversation
- Detect chat switches vs stream count changes to handle scrolling differently - Use double requestAnimationFrame when switching chats with existing messages to wait for Virtuoso to render before scrolling - Don't scroll immediately when switching to a chat with no messages yet (messages will be fetched and trigger a re-render)
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Summary of ChangesHello @wwwillchen, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refines the chat panel's scrolling mechanism to provide a more consistent and user-friendly experience. The changes specifically address issues where the chat view might not correctly position itself at the bottom when switching between conversations or when new messages arrive, by introducing logic to intelligently manage scroll events based on the context of the chat interaction. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request aims to fix the scroll behavior when switching between chats. The main change in ChatPanel.tsx correctly identifies the need for different scroll logic for chat switches versus new messages. However, the current implementation has a subtle bug where switching to a chat with uncached messages results in a smooth scroll animation instead of an instant one. I've provided a suggestion to fix this by using an additional useRef to correctly track the chat switch state across renders, ensuring a consistent and better user experience.
| const messages = chatId ? (messagesById.get(chatId) ?? []) : []; | ||
|
|
||
| // Track previous chatId to detect chat switches | ||
| const prevChatIdRef = useRef<number | undefined>(undefined); | ||
|
|
||
| useEffect(() => { | ||
| const isChatSwitch = prevChatIdRef.current !== chatId; | ||
| prevChatIdRef.current = chatId; | ||
|
|
||
| isAtBottomRef.current = true; | ||
| setShowScrollButton(false); | ||
| scrollToBottom(); | ||
| }, [chatId, streamCount, scrollToBottom]); | ||
|
|
||
| if (isChatSwitch && messages.length > 0) { | ||
| // When switching chats with existing messages, wait for Virtuoso to render | ||
| // then scroll to ensure we're at the bottom | ||
| requestAnimationFrame(() => { | ||
| requestAnimationFrame(() => { | ||
| scrollToBottom("instant"); | ||
| }); | ||
| }); | ||
| } else if (!isChatSwitch) { | ||
| // For stream count changes (new message sent), scroll immediately | ||
| scrollToBottom(); | ||
| } | ||
| // Note: if isChatSwitch && messages.length === 0, we don't scroll yet. | ||
| // The messages will be fetched and this effect will re-run with messages.length > 0. | ||
| }, [chatId, streamCount, messages.length, scrollToBottom]); |
There was a problem hiding this comment.
This logic has a subtle bug. When switching to a chat where messages aren't cached, they are fetched asynchronously. When they arrive, this useEffect runs again, but isChatSwitch is false. This leads to a 'smooth' scroll instead of the expected 'instant' scroll.
To fix this and ensure an instant scroll in all chat-switch scenarios, we can use an additional useRef to track the chat switch state across renders until the messages are loaded. This provides a better user experience.
const messages = chatId ? (messagesById.get(chatId) ?? []) : [];
// Track previous chatId to detect chat switches
const prevChatIdRef = useRef<number | undefined>(undefined);
const justSwitchedChat = useRef(false);
useEffect(() => {
const isChatSwitch = prevChatIdRef.current !== chatId;
prevChatIdRef.current = chatId;
if (isChatSwitch) {
justSwitchedChat.current = true; // A chat switch occurred
}
isAtBottomRef.current = true;
setShowScrollButton(false);
// If we just switched chat and messages are available (either immediately or after fetch)
if (justSwitchedChat.current && messages.length > 0) {
// Wait for Virtuoso to render, then scroll instantly
requestAnimationFrame(() => {
requestAnimationFrame(() => {
scrollToBottom("instant");
justSwitchedChat.current = false; // Reset for this chat
});
});
} else if (!isChatSwitch) {
// For stream count changes (new message sent in the same chat), scroll smoothly
scrollToBottom();
}
}, [chatId, streamCount, messages.length, scrollToBottom]);
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Greptile OverviewGreptile SummaryImproved scroll behavior when switching between chats by distinguishing chat switches from new message sends. Uses double Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant ChatPanel
participant Effect
participant Virtuoso
participant ScrollHandler
Note over User,ScrollHandler: Scenario 1: Switch to chat with existing messages
User->>ChatPanel: Switch to Chat B
ChatPanel->>Effect: chatId changes (A→B)
Effect->>Effect: isChatSwitch = true, messages.length > 0
Effect->>ScrollHandler: requestAnimationFrame()
ScrollHandler->>ScrollHandler: requestAnimationFrame()
ScrollHandler->>Virtuoso: scrollToBottom("instant")
Note over User,ScrollHandler: Scenario 2: Send new message in current chat
User->>ChatPanel: Send message
ChatPanel->>Effect: streamCount changes
Effect->>Effect: isChatSwitch = false
Effect->>ScrollHandler: requestAnimationFrame()
ScrollHandler->>ScrollHandler: requestAnimationFrame()
ScrollHandler->>Virtuoso: scrollToBottom() [smooth]
Note over User,ScrollHandler: Scenario 3: Switch to chat without messages
User->>ChatPanel: Switch to Chat C (no messages)
ChatPanel->>Effect: chatId changes (A→C)
Effect->>Effect: isChatSwitch = true, messages.length = 0
Note over Effect: No scroll (waiting for messages)
ChatPanel->>ChatPanel: fetchChatMessages()
ChatPanel->>Effect: messages.length changes (0→N)
Effect->>Effect: isChatSwitch = false (ref already updated)
Effect->>ScrollHandler: requestAnimationFrame()
ScrollHandler->>ScrollHandler: requestAnimationFrame()
ScrollHandler->>Virtuoso: scrollToBottom() [smooth]
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| useEffect(() => { | ||
| const isChatSwitch = prevChatIdRef.current !== chatId; | ||
| prevChatIdRef.current = chatId; | ||
|
|
||
| isAtBottomRef.current = true; | ||
| setShowScrollButton(false); | ||
| scrollToBottom(); | ||
| }, [chatId, streamCount, scrollToBottom]); | ||
|
|
||
| if (isChatSwitch && messages.length > 0) { | ||
| // When switching chats with existing messages, wait for Virtuoso to render | ||
| // then scroll to ensure we're at the bottom | ||
| requestAnimationFrame(() => { | ||
| requestAnimationFrame(() => { | ||
| scrollToBottom("instant"); | ||
| }); | ||
| }); | ||
| } else if (!isChatSwitch) { | ||
| // For stream count changes (new message sent), scroll immediately | ||
| scrollToBottom(); | ||
| } | ||
| // Note: if isChatSwitch && messages.length === 0, we don't scroll yet. | ||
| // The messages will be fetched and this effect will re-run with messages.length > 0. | ||
| }, [chatId, streamCount, messages.length, scrollToBottom]); |
There was a problem hiding this comment.
🟡 Chat switch scroll logic fails when messages are fetched asynchronously (uncached chats)
When switching to a chat whose messages are not yet in the messagesById atom (i.e., not cached), the double-RAF instant scroll intended for chat switches is never executed.
Root Cause
The effect at lines 87-108 tracks chat switches via prevChatIdRef. On a chat switch to an uncached chat:
- Effect runs:
isChatSwitch = true,messages.length === 0→ no scroll is performed, butprevChatIdRef.currentis already updated to the newchatId(line 89). fetchChatMessages(line 110-121) asynchronously loads messages into the atom.- When messages arrive,
messages.lengthchanges from 0 to >0, triggering the effect again. - On this re-run,
prevChatIdRef.current === chatIdsoisChatSwitch = false. - The code enters the
else if (!isChatSwitch)branch (line 100) and callsscrollToBottom()with default smooth behavior.
This means:
- The double
requestAnimationFramepattern (lines 95-101) that waits for Virtuoso to render is bypassed for uncached chats. - The scroll uses
"smooth"animation instead of"instant"jump. - The scroll may execute before Virtuoso has finished rendering the newly-fetched messages, resulting in incomplete or incorrect scroll positioning.
The comment at lines 104-105 acknowledges this re-run scenario but incorrectly assumes it will be handled properly. In practice, the isChatSwitch flag is already consumed/lost by the first run.
Impact: When switching to a chat that hasn't been visited yet in the current session (messages not cached), the user may see a brief flash of content at the wrong scroll position, or a smooth scroll animation instead of an instant jump to the bottom.
Prompt for agents
The core issue is that prevChatIdRef.current is updated on the first run of the effect (when messages.length === 0 for uncached chats), so when the effect re-runs after messages are fetched, isChatSwitch is false.
A fix would be to use a separate ref to track whether we're still waiting for messages after a chat switch, rather than relying solely on prevChatIdRef for the isChatSwitch check. For example:
1. Add a new ref: const pendingChatSwitchRef = useRef(false);
2. In the effect, when isChatSwitch && messages.length === 0, set pendingChatSwitchRef.current = true (instead of doing nothing).
3. When !isChatSwitch && pendingChatSwitchRef.current && messages.length > 0, treat it as a deferred chat switch: use the double-RAF instant scroll pattern, then set pendingChatSwitchRef.current = false.
4. The existing else if (!isChatSwitch && !pendingChatSwitchRef.current) branch handles the normal stream count change case with scrollToBottom().
This ensures that the chat-switch scroll behavior (double-RAF + instant) is applied even when messages are fetched asynchronously after the chat switch.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
@BugBot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| requestAnimationFrame(() => { | ||
| scrollToBottom("instant"); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Stale RAF causes cross-chat scroll jumps
Medium Severity
The double requestAnimationFrame scheduled on chat switch has no cleanup, so pending callbacks from a previous switch can run after another chatId change and still call scrollToBottom("instant"). This can apply a stale scroll action to the newly active chat and cause unexpected jump-to-bottom behavior.
github-actions
Bot
added
the
needs-human:review-issue
Wait for Virtuoso to render the placeholder message before scrolling by using a double requestAnimationFrame. This ensures the scroll target exists before we attempt to scroll to it. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
wwwillchen-bot
temporarily deployed
to
ai-bots
GitHub Actions
Inactive
wwwillchen-bot
temporarily deployed
to
ai-bots
GitHub Actions
Inactive
|
@BugBot run |
🔍 Dyadbot Code Review SummaryFound 4 issue(s) flagged by 3 independent reviewers (consensus voting: 2+ agents must agree, at least one rates MEDIUM+). Summary
New Issues to Address
Already Commented (skipped)
🟢 Low Priority Issues (0 items)No low priority issues to report. See inline comments for details. Generated by Dyadbot multi-agent code review (3 independent reviewers with consensus voting) |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| isAtBottomRef.current = true; | ||
| setShowScrollButton(false); |
There was a problem hiding this comment.
🔴 HIGH: Unconditionally resetting isAtBottomRef defeats user scroll position during streaming
Flagged by 2/3 independent reviewers (HIGH, MEDIUM)
Lines 91-92 unconditionally set isAtBottomRef.current = true and setShowScrollButton(false) on every effect run. Since messages.length is now in the dependency array, this means whenever a new message arrives during streaming, the user's scroll position state is forcibly reset to "at bottom" and the scroll button is hidden — even if the user has scrolled up to read earlier messages.
The previous code only did this on chatId or streamCount changes, which was more intentional. Now with messages.length as a trigger, this reset happens too frequently and can break the user's ability to stay scrolled up during a stream.
| isAtBottomRef.current = true; | |
| setShowScrollButton(false); | |
| if (isChatSwitch) { | |
| isAtBottomRef.current = true; | |
| setShowScrollButton(false); | |
| } |
Move the unconditional reset inside the if (isChatSwitch) block so it only fires on actual chat switches, not on every messages.length change during streaming.
Generated by Dyadbot multi-agent code review
| } else if (!isChatSwitch) { | ||
| // For stream count changes (new message sent), wait for Virtuoso to render | ||
| // the placeholder message before scrolling | ||
| requestAnimationFrame(() => { | ||
| requestAnimationFrame(() => { | ||
| scrollToBottom(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🟡 MEDIUM: messages.length dependency triggers unnecessary scrolls in non-chat-switch branch
Flagged by 3/3 independent reviewers (MEDIUM, LOW, LOW)
The else if (!isChatSwitch) branch runs whenever any dependency changes and it is not a chat switch. Because messages.length is in the dependency array, this branch fires every time the message count changes during streaming (new message chunks, tool-use results, etc.), not just on streamCount changes.
The comment says "For stream count changes (new message sent)" but the code also triggers on message-count changes. This creates additional scroll-to-bottom calls during streaming that may conflict with Virtuoso's followOutput behavior or the test-mode auto-scroll effect, potentially causing janky scrolling.
Consider guarding this branch with a check that streamCount actually changed (e.g., track previous streamCount in a ref), or separate the messages.length-dependent logic (for chat-switch deferred scroll) from the streamCount-dependent logic (for new message scroll) into distinct effects.
Generated by Dyadbot multi-agent code review
🎭 Playwright Test Results
|
| OS | Passed | Failed | Flaky | Skipped |
|---|---|---|---|---|
| 🍎 macOS | 221 | 5 | 8 | 6 |
Summary: 221 passed, 5 failed, 8 flaky, 6 skipped
Failed Tests
🍎 macOS
capacitor.spec.ts > capacitor upgrade and sync works- TimeoutError: locator.click: Timeout 30000ms exceeded.
debugging_logs.spec.ts > network requests and responses should appear in the console- Error: expect(locator).toBeEnabled() failed
security_review.spec.ts > security review - edit and use knowledge- Error: expect(string).toMatchSnapshot(expected) failed
select_component.spec.ts > select component next.js- TimeoutError: locator.click: Timeout 120000ms exceeded.
template-create-nextjs.spec.ts > create next.js app- Error: expect(locator).toBeVisible() failed
📋 Re-run Failing Tests (macOS)
Copy and paste to re-run all failing spec files locally:
npm run e2e \
e2e-tests/capacitor.spec.ts \
e2e-tests/debugging_logs.spec.ts \
e2e-tests/security_review.spec.ts \
e2e-tests/select_component.spec.ts \
e2e-tests/template-create-nextjs.spec.ts⚠️ Flaky Tests
🍎 macOS
annotator.spec.ts > annotator - capture and submit screenshot(passed after 1 retry)chat_history.spec.ts > should open, navigate, and select from history menu(passed after 1 retry)debugging_logs.spec.ts > console logs should appear in the console(passed after 1 retry)local_agent_basic.spec.ts > local-agent - dump request(passed after 1 retry)refresh.spec.ts > preview navigation - forward and back buttons work(passed after 1 retry)select_component.spec.ts > deselect component(passed after 1 retry)setup_flow.spec.ts > Setup Flow > setup banner shows correct state when node.js is installed(passed after 1 retry)setup.spec.ts > setup ai provider(passed after 1 retry)
## Summary - Fix scroll behavior when switching between chats with existing messages - Use double `requestAnimationFrame` to wait for Virtuoso to render before scrolling to bottom when switching chats - Distinguish between chat switches and new message sends to handle scrolling appropriately - Avoid premature scrolling when switching to chats where messages haven't been fetched yet ## Test plan 1. Open Dyad and start a chat with some messages 2. Start another chat with messages 3. Switch between chats and verify scroll position stays at the bottom 4. Send a new message and verify it scrolls to show the new message 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- devin-review-badge-begin --> --- <a href="https://app.devin.ai/review/dyad-sh/dyad/pull/2643" target="_blank"> <picture> <source media="(prefers-color-scheme: dark)" srcset="https://static.devin.ai/assets/gh-open-in-devin-review-dark.svg?v=1"> <img src="https://static.devin.ai/assets/gh-open-in-devin-review-light.svg?v=1" alt="Open with Devin"> </picture> </a> <!-- devin-review-badge-end --> <!-- This is an auto-generated description by cubic. --> --- ## Summary by cubic Fixes chat auto-scroll to keep the view anchored at the bottom when switching chats or sending messages. Prevents flicker and jumps before messages render. - **Bug Fixes** - Distinguish chat switches vs new sends; adjust scroll timing accordingly. - On chat switch with existing messages, wait for Virtuoso to render (double requestAnimationFrame), then scroll to bottom instantly. - On new message send, wait for the placeholder to render (double requestAnimationFrame) before scrolling; skip auto-scroll when switching to a chat with no messages yet. <sup>Written for commit 8814a60. Summary will update on new commits.</sup> <!-- End of auto-generated description by cubic. --> <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Scoped to `ChatPanel` scroll timing logic; risk is limited to possible UI regressions (missed/extra scroll) when switching chats or starting streams. > > **Overview** > Fixes `ChatPanel` auto-scroll behavior by **distinguishing chat switches from new stream starts** and delaying the scroll until after Virtuoso has rendered. > > On chat switch, it now scrolls to bottom *only after messages exist* (avoiding premature scroll before fetch/render) and uses a double `requestAnimationFrame` with `instant` scrolling; on new message sends (`streamCount` changes), it similarly waits for render before scrolling with the default behavior. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 8814a60. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
## Summary - Fix scroll behavior when switching between chats with existing messages - Use double `requestAnimationFrame` to wait for Virtuoso to render before scrolling to bottom when switching chats - Distinguish between chat switches and new message sends to handle scrolling appropriately - Avoid premature scrolling when switching to chats where messages haven't been fetched yet ## Test plan 1. Open Dyad and start a chat with some messages 2. Start another chat with messages 3. Switch between chats and verify scroll position stays at the bottom 4. Send a new message and verify it scrolls to show the new message 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- devin-review-badge-begin --> --- <a href="https://app.devin.ai/review/dyad-sh/dyad/pull/2643" target="_blank"> <picture> <source media="(prefers-color-scheme: dark)" srcset="https://static.devin.ai/assets/gh-open-in-devin-review-dark.svg?v=1"> <img src="https://static.devin.ai/assets/gh-open-in-devin-review-light.svg?v=1" alt="Open with Devin"> </picture> </a> <!-- devin-review-badge-end --> <!-- This is an auto-generated description by cubic. --> --- ## Summary by cubic Fixes chat auto-scroll to keep the view anchored at the bottom when switching chats or sending messages. Prevents flicker and jumps before messages render. - **Bug Fixes** - Distinguish chat switches vs new sends; adjust scroll timing accordingly. - On chat switch with existing messages, wait for Virtuoso to render (double requestAnimationFrame), then scroll to bottom instantly. - On new message send, wait for the placeholder to render (double requestAnimationFrame) before scrolling; skip auto-scroll when switching to a chat with no messages yet. <sup>Written for commit 8814a60. Summary will update on new commits.</sup> <!-- End of auto-generated description by cubic. --> <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Scoped to `ChatPanel` scroll timing logic; risk is limited to possible UI regressions (missed/extra scroll) when switching chats or starting streams. > > **Overview** > Fixes `ChatPanel` auto-scroll behavior by **distinguishing chat switches from new stream starts** and delaying the scroll until after Virtuoso has rendered. > > On chat switch, it now scrolls to bottom *only after messages exist* (avoiding premature scroll before fetch/render) and uses a double `requestAnimationFrame` with `instant` scrolling; on new message sends (`streamCount` changes), it similarly waits for render before scrolling with the default behavior. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 8814a60. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
2 participants
Summary
requestAnimationFrameto wait for Virtuoso to render before scrolling to bottom when switching chatsTest plan
🤖 Generated with Claude Code
Summary by cubic
Fixes chat auto-scroll to keep the view anchored at the bottom when switching chats or sending messages. Prevents flicker and jumps before messages render.
Written for commit 8814a60. Summary will update on new commits.
Note
Low Risk
Scoped to
ChatPanelscroll timing logic; risk is limited to possible UI regressions (missed/extra scroll) when switching chats or starting streams.Overview
Fixes
ChatPanelauto-scroll behavior by distinguishing chat switches from new stream starts and delaying the scroll until after Virtuoso has rendered.On chat switch, it now scrolls to bottom only after messages exist (avoiding premature scroll before fetch/render) and uses a double
requestAnimationFramewithinstantscrolling; on new message sends (streamCountchanges), it similarly waits for render before scrolling with the default behavior.Written by Cursor Bugbot for commit 8814a60. This will update automatically on new commits. Configure here.