-
Notifications
You must be signed in to change notification settings - Fork 37.4k
Lazy render collapsed thinking group parts #288535
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
base: main
Are you sure you want to change the base?
Conversation
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 pull request implements lazy rendering for collapsed thinking group parts to improve performance when scrolling through chat sessions with many tool invocations. By deferring rendering of tool invocations until the thinking section is expanded, the change avoids rendering hundreds of hidden elements.
Changes:
- Introduced factory pattern for creating tool invocation parts, allowing deferred instantiation
- Added lazy item tracking in
ChatThinkingContentPartwith materialization on first expansion - Removed unused
contextparameter fromsetupThinkingContainermethod
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| chatServiceImpl.ts | Commented out isImported check in session storage logic (appears unrelated to lazy rendering) |
| chatListRenderer.ts | Refactored tool invocation and markdown rendering to use factory pattern for lazy instantiation |
| chatThinkingContentPart.ts | Added lazy items support with materialization autorun, refactored appendItem to accept factory functions |
Comments suppressed due to low confidence (1)
src/vs/workbench/contrib/chat/browser/widget/chatContentParts/chatThinkingContentPart.ts:193
- When lazy items are materialized on expansion,
_onDidChangeHeight.fire()is called once after materializing all items. However, individual items may have their own height change listeners (like the autorun at line 1665-1667 in chatListRenderer.ts). This could lead to multiple height updates being triggered. Consider whether firing the height change event once after all items are materialized is sufficient, or if individual item height listeners might cause layout issues.
// Materialize lazy items when first expanded
this._register(autorun(r => {
if (this._isExpanded.read(r) && !this.hasExpandedOnce && this.lazyItems.length > 0) {
this.hasExpandedOnce = true;
for (const item of this.lazyItems) {
this.materializeLazyItem(item);
}
this._onDidChangeHeight.fire();
src/vs/workbench/contrib/chat/browser/widget/chatContentParts/chatThinkingContentPart.ts
Show resolved
Hide resolved
src/vs/workbench/contrib/chat/browser/widget/chatContentParts/chatThinkingContentPart.ts
Show resolved
Hide resolved
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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
src/vs/workbench/contrib/chat/browser/widget/chatContentParts/chatThinkingContentPart.ts
Outdated
Show resolved
Hide resolved
…chatThinkingContentPart.ts Co-authored-by: Justin Chen <54879025+justschen@users.noreply.github.com>
…not streaming before rendering it, so that we don't render the tool parts then immediate finalize and collapse the group - Diffing with lazily created tool parts is a bit weird, led to rendering extra tool parts at the bottom of the response. One thing I added to help with this is clearing out all rendered parts when a new model is swapped in. I wanted to do this anyway because keeping all those parts around can lead to leaks.
Fix #287176
fyi @justschen this is a pretty complicated change to do something seemingly simple, not rendering tool invocations that are collapsed away. Without this, I can scroll through a long chat session and render 100s of tool invocations that aren't displayed, and this is pretty slow.
This is kind of a mess but the thinking part logic is complex. Lmk if there's anything you're concerned about