Skip to content

Avoid firing _onDidChangeItemHeight when row height is only being initialized#315486

Merged
roblourens merged 1 commit into
mainfrom
roblou/extensive-fowl
May 11, 2026
Merged

Avoid firing _onDidChangeItemHeight when row height is only being initialized#315486
roblourens merged 1 commit into
mainfrom
roblou/extensive-fowl

Conversation

@roblourens
Copy link
Copy Markdown
Member

@roblourens roblourens commented May 9, 2026

Alternate proposal for #314882

For #293359

Copy link
Copy Markdown
Contributor

Copilot AI left a 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 adjusts ChatListItemRenderer.fireItemHeightChange to avoid emitting the _onDidChangeItemHeight event when the row height is being initialized, by only firing the event if there was a previously stored numeric height.

Changes:

  • Track the previous currentRenderedHeight before overwriting it.
  • Gate _onDidChangeItemHeight.fire(...) behind an additional typeof originalStoredHeight === 'number' check.
Show a summary per file
File Description
src/vs/workbench/contrib/chat/browser/widget/chatListRenderer.ts Changes height-change event emission logic by capturing the original stored height and adding an additional guard before firing the event.

Copilot's findings

  • Files reviewed: 1/1 changed files
  • Comments generated: 1

Comment thread src/vs/workbench/contrib/chat/browser/widget/chatListRenderer.ts
const originalStoredHeight = template.currentElement.currentRenderedHeight;
template.currentElement.currentRenderedHeight = normalizedHeight;
if (template.currentElement !== this._elementBeingRendered) {
if (template.currentElement !== this._elementBeingRendered && typeof originalStoredHeight === 'number') {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this check be originalStoredHeight === undefined?

I think it should work to avoid the re-flow, just have a concern: will this break the spec of _onDidChangeItemHeight? my mental model for this event is that it should fire whenever the height got changed, and in the first entry here, it did change from a 'undefined' state (with a default height) to another height.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this check be originalStoredHeight === undefined

I think I want to fire the event when the original height was already initialized

I think it should work to avoid the re-flow, just have a concern: will this break the spec of _onDidChangeItemHeight? my mental model for this event is that it should fire whenever the height got changed, and in the first entry here, it did change from a 'undefined' state (with a default height) to another height.

It's a bit weird, but we need to fire the event when the row's height changed after it was rendered, not when it was simply initialized. Then we pass that on to the list.

@roblourens
Copy link
Copy Markdown
Member Author

Let's merge this on Monday (for 1.121) if that's ok with you, it may be risky

@roblourens roblourens marked this pull request as ready for review May 11, 2026 17:45
@roblourens roblourens merged commit 2890faa into main May 11, 2026
29 checks passed
@roblourens roblourens deleted the roblou/extensive-fowl branch May 11, 2026 17:45
@vs-code-engineering vs-code-engineering Bot added this to the 1.121.0 milestone May 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants