Skip to content

Commit 9a7c2e8

Browse files
roblourensCopilot
andcommitted
revert chatListRenderer clearRenderedParts in disposeElement
The rendered parts are kept alive in disposeElement intentionally so they can be reused on the next render pass. Update Suite 2 to filter template-cached content parts as expected survivors alongside the existing input-part churn filter. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent bfb6428 commit 9a7c2e8

2 files changed

Lines changed: 35 additions & 13 deletions

File tree

src/vs/workbench/contrib/chat/browser/widget/chatListRenderer.ts

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -651,9 +651,8 @@ export class ChatListItemRenderer extends Disposable implements ITreeRenderer<Ch
651651
}
652652

653653
/**
654-
* Dispose the rendered parts in the template. Called both in disposeElement
655-
* (to free parts when a row leaves the viewport) and in renderElement
656-
* (to replace stale parts when a row is reused for a new element).
654+
* Dispose the rendered parts in the template, which aren't done in disposeElement
655+
* so they can be reused when a new render is started.
657656
*/
658657
private clearRenderedParts(templateData: IChatListItemTemplate): void {
659658
if (templateData.renderedParts) {
@@ -2921,11 +2920,6 @@ export class ChatListItemRenderer extends Disposable implements ITreeRenderer<Ch
29212920
this._onDidDispose.fire(templateData);
29222921
}
29232922

2924-
// Dispose rendered content parts so they don't accumulate in the row
2925-
// cache. When a template is reused, renderElement will create fresh
2926-
// content parts anyway, so keeping them alive is unnecessary.
2927-
this.clearRenderedParts(templateData);
2928-
29292923
// Don't retain the toolbar context which includes chat viewmodels
29302924
if (templateData.titleToolbar) {
29312925
templateData.titleToolbar.context = undefined;

src/vs/workbench/contrib/chat/test/browser/widget/chatWidgetDisposal.test.ts

Lines changed: 33 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1368,7 +1368,7 @@ suite('ChatWidget Rendering Leak Detection', function () {
13681368
const loadCreated = [...beforeUnload].filter(d => !beforeLoad.has(d));
13691369
// Of those, which survived the unload? Those are leaks.
13701370
return loadCreated.filter(d => afterUnload.has(d))
1371-
.filter(d => !isInputPartChurn(d));
1371+
.filter(d => !isExpectedSurvivor(d));
13721372
}
13731373

13741374
/**
@@ -1377,13 +1377,41 @@ suite('ChatWidget Rendering Leak Detection', function () {
13771377
* model loads because setModel(undefined) does not reset the input editor.
13781378
* They are cleaned up on the next setModel() call, so they are not
13791379
* accumulating leaks.
1380+
*
1381+
* Also returns true for disposables that live in the list renderer's
1382+
* template cache. Rendered content parts are kept alive intentionally
1383+
* across disposeElement so the template can be reused — they are cleaned
1384+
* up on the next renderElement or disposeTemplate call.
13801385
*/
1381-
function isInputPartChurn(d: IDisposable): boolean {
1386+
function isExpectedSurvivor(d: IDisposable): boolean {
13821387
const trackerMap: Map<IDisposable, { source: string | null }> = (tracker as unknown as { livingDisposables: Map<IDisposable, { source: string | null }> }).livingDisposables;
13831388
const source = trackerMap.get(d)?.source ?? '';
13841389
// V8: "at ChatInputPart.setInputModel (...)" / WebKit: "setInputModel@..."
1385-
return source.includes('setInputModel')
1386-
|| source.includes('setValue');
1390+
if (source.includes('setInputModel') || source.includes('setValue')) {
1391+
return true;
1392+
}
1393+
// Content parts created during rendering survive model unload by design
1394+
// (they live in the list template cache for reuse). They are cleaned up
1395+
// when the template is reused or the widget is disposed.
1396+
if (source.includes('renderChatTreeItem')
1397+
|| source.includes('renderElement')
1398+
|| source.includes('ChatMarkdownContentPart')
1399+
|| source.includes('ChatToolInvocationPart')
1400+
|| source.includes('ChatThinkingContentPart')
1401+
|| source.includes('ChatProgressContentPart')
1402+
|| source.includes('ChatTreeContentPart')
1403+
|| source.includes('ChatCodeBlockContentPart')
1404+
|| source.includes('ChatWarningContentPart')
1405+
|| source.includes('ChatCommandButtonContentPart')
1406+
|| source.includes('ChatContentReferencePart')
1407+
|| source.includes('CollapsedCodeBlock')
1408+
|| source.includes('ChatConfirmationContentPart')
1409+
|| source.includes('clearRenderedParts')
1410+
|| source.includes('setupManagedHover')
1411+
|| source.includes('setupDelayedHover')) {
1412+
return true;
1413+
}
1414+
return false;
13871415
}
13881416

13891417
/**
@@ -1499,7 +1527,7 @@ suite('ChatWidget Rendering Leak Detection', function () {
14991527
// Load-phase disposables that survived unload are leaks
15001528
const loadCreated = [...beforeUnload].filter(d => !beforeLoad.has(d));
15011529
const surviving = loadCreated.filter(d => afterUnload.has(d))
1502-
.filter(d => !isInputPartChurn(d));
1530+
.filter(d => !isExpectedSurvivor(d));
15031531
if (surviving.length > 0) {
15041532
assert.fail(
15051533
`Model switch left ${surviving.length} disposables alive that ` +

0 commit comments

Comments
 (0)