-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Refactor chat components to utilize memoization for performance optimization rather than virtualization #2613
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
…ization rather than virtualization - Removed all Virtuoso logic, replaced with normal stuff. - Updated `BrowserSessionRowContent` and `ChatRowContent` components to use `memo`. - Refactored scrolling logic in `ChatView`, ensuring smooth scrolling behavior and maintaining scroll position during row expansion. - Adjusted event handling for scroll and wheel events to better manage auto-scroll functionality and user interactions.
|
|
Thanks @NateMateS - I agree with your assessment here. Do you mind taking a look at the test failure? I can help get this tested and over the finish line. |
No problem. Yes, I looked at it just now. I think it's likely just a jsdom caused error that could be fixed with a simple check: if (container && typeof container.scrollTo === "function") { |
| import debounce from "debounce" | ||
| import { useCallback, useEffect, useMemo, useRef, useState } from "react" | ||
| import { useDeepCompareEffect, useEvent, useMount } from "react-use" | ||
| import { Virtuoso, type VirtuosoHandle } from "react-virtuoso" |
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.
why you remove Virtuoso ?, this will make memory go up
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.
The PR description explains the reasoning and also mentions:
No observed performance loss, no observed memory rise.
I want to spend some time profiling the memory though, just for my own edification.
|
Hey @NateMateS, Thank you for your contribution, we apologize for taking so long to review this PR. This PR de-abstracts the scrolling logic of I'll close this PR temporarily, as we now shifted to a issue-first workflow. This doesn't mean there's something wrong with your implementation. We appreciate your patience and apologize again for the delay, I'm looking forward to improving our performance with your implementation! |
Context
With the growing context limits, the recent popularity of Orchestrator-based workflows, and UI updates that push larger messages into chat; it is increasingly becoming important that the users of Roo can reliably scroll through their conversations. Virtualization, admittedly as far as I know of the existing solutions myself, they do not handle dynamic elements that wildly differ in dimensions well. Usually, such applications of virtualization end in the scrollbar staggering, unexpectedly resizing as if content was being loaded and unloaded (because technically, it is); jumping; and even the entire chat flashing repeatedly. I also observed a lot of hacky methods used just to get Virtuoso into a semi-functional state. I do not consider it a good path towards optimization for a chat interface use-case.
I expect little loss of performance and low memory usage rise from the replacement of Virtuoso from the chat interface, and I've experienced no less performance than the Virtuoso implementation even on a chat that has a token total of ~80m.
The provided solution entirely eliminates the unreliable scrolling issues you experience with virtualization methods, and I consider possible further optimizations of such implementation the rational path forward.
Implementation
Replaced
react-virtuosowith Native Scrolling (webview-ui/src/components/chat/ChatView.tsx):Virtuosocomponent and its associated logic.divwithoverflow-y: scroll.scrollToBottomSmooth,scrollToBottomAuto), event handlers (handleScroll,handleWheel), and row expansion logic (toggleRowExpansion,handleRowHeightChange) to work with the native scroll container.groupedMessagesarray within the scrollablediv, instead of usingVirtuoso's item renderer.Performance Optimizations:
BrowserSessionRowContentinReact.memo(webview-ui/src/components/chat/BrowserSessionRow.tsx).ChatRowContentinReact.memo(webview-ui/src/components/chat/ChatRow.tsx).handleSuggestionClickcallback function inChatView.tsxusinguseCallback.contain: contentwas added for potential row rendering optimization.Affected Files:
webview-ui/src/components/chat/BrowserSessionRow.tsxwebview-ui/src/components/chat/ChatRow.tsxwebview-ui/src/components/chat/ChatView.tsxImpact:
React.memoshould reduce unnecessary re-renders of chat rows, 'contain' should improve layout calculation, painting performance.Screenshots
--
How to Test
I suppose a good way to test the changes would be to A/B test on large chats, and try scrolling. Watch memory usage, and watch for performance issues. I've experienced little difference in both, personally - nothing I could even take note of.
As always, do test for any possible regressions in functionality, just in case.
Get in Touch
Discord: @dr.fumesb
I am present in the server.
Important
Replace chat virtualization with native scrolling and optimize chat row rendering using React.memo and CSS containment for improved performance.
react-virtuosovirtualization fromChatView.tsx; replace with native scrollabledivand manual scroll management.scrollToBottomSmooth,scrollToBottomAuto), row expansion (toggleRowExpansion), and scroll event handling (handleScroll,handleWheel) to work with native scrolling.groupedMessagesdirectly inChatView.tsx.contain: contentto chat row containers for layout/rendering optimization.ChatRowContentinReact.memoinChatRow.tsxto prevent unnecessary re-renders.BrowserSessionRowContentinReact.memoinBrowserSessionRow.tsx.handleSuggestionClickinChatView.tsxwithuseCallback.Virtuosoand related logic fromChatView.tsx.This description was created by
for 2af4ef5. It will automatically update as commits are pushed.