From 122e527a810b752011b6b900831d4aaedafbe482 Mon Sep 17 00:00:00 2001 From: Connor Peet Date: Mon, 2 Mar 2026 15:53:01 -0800 Subject: [PATCH] chat: Use ref-counted model collection for tool confirmation inputs (#298829) * chat: Use ref-counted model collection for tool confirmation inputs Fixes duplicate model error when ToolConfirmationSubPart re-renders with the same toolCallId. Models keyed by URI are now managed via a ref-counted ReferenceCollection (InlineTextModelCollection) on IChatContentPartRenderContext, so multiple SubPart instances can safely share the same model. The model is only disposed when the last reference is released, preventing the 'Cannot add model because it already exists!' error during list re-rendering or parallel tool calls. - Add InlineTextModelCollection class wrapping ReferenceCollection to handle ref-counted model lifecycle keyed by URI - Add inlineTextModels field to IChatContentPartRenderContext - Wire InlineTextModelCollection into chatListRenderer, passing to all context sites - Update chatToolConfirmationSubPart to use context.inlineTextModels.acquire() instead of modelService.createModel() - Remove unused IModelService injection from chatToolConfirmationSubPart Fixes https://github.com/microsoft/vscode/issues/294345 (Commit message generated by Copilot) * ci failure --- .../chatContentParts/chatContentParts.ts | 39 ++++++++++++++++++- .../chatToolConfirmationSubPart.ts | 7 ++-- .../chat/browser/widget/chatListRenderer.ts | 6 ++- .../chatSubagentContentPart.test.ts | 3 +- .../chatThinkingContentPart.test.ts | 3 +- .../chatProgressContentPart.fixture.ts | 5 ++- .../chatQuestionCarousel.fixture.ts | 5 ++- 7 files changed, 56 insertions(+), 12 deletions(-) diff --git a/src/vs/workbench/contrib/chat/browser/widget/chatContentParts/chatContentParts.ts b/src/vs/workbench/contrib/chat/browser/widget/chatContentParts/chatContentParts.ts index e15b3d1fafd..e0d39e078e5 100644 --- a/src/vs/workbench/contrib/chat/browser/widget/chatContentParts/chatContentParts.ts +++ b/src/vs/workbench/contrib/chat/browser/widget/chatContentParts/chatContentParts.ts @@ -3,13 +3,17 @@ * Licensed under the MIT License. See License.txt in the project root for license information. *--------------------------------------------------------------------------------------------*/ -import { IDisposable } from '../../../../../../base/common/lifecycle.js'; +import { Disposable, IDisposable, ReferenceCollection } from '../../../../../../base/common/lifecycle.js'; import { ChatTreeItem, IChatCodeBlockInfo } from '../../chat.js'; import { IChatRendererContent, IChatRequestViewModel, IChatResponseViewModel } from '../../../common/model/chatViewModel.js'; import { CodeBlockModelCollection } from '../../../common/widget/codeBlockModelCollection.js'; import { DiffEditorPool, EditorPool } from './chatContentCodePools.js'; import { IObservable } from '../../../../../../base/common/observable.js'; import { Event } from '../../../../../../base/common/event.js'; +import { ITextModel } from '../../../../../../editor/common/model.js'; +import { ILanguageSelection } from '../../../../../../editor/common/languages/language.js'; +import { IModelService } from '../../../../../../editor/common/services/model.js'; +import { URI } from '../../../../../../base/common/uri.js'; export interface IChatContentPart extends IDisposable { domNode: HTMLElement | undefined; @@ -53,4 +57,37 @@ export interface IChatContentPartRenderContext { readonly codeBlockModelCollection: CodeBlockModelCollection; readonly currentWidth: IObservable; readonly onDidChangeVisibility: Event; + readonly inlineTextModels: InlineTextModelCollection; +} + +/** + * Ref-counted collection of inline text models keyed by URI. Models are + * created on first acquire and disposed only when the last reference is + * released, preventing duplicate-model errors during re-renders. + */ +export class InlineTextModelCollection extends Disposable { + private readonly _collection: InlineTextModelReferenceCollection; + + constructor(@IModelService modelService: IModelService) { + super(); + this._collection = new InlineTextModelReferenceCollection(modelService); + } + + acquire(uri: URI, value: string, languageSelection: ILanguageSelection | null, isForSimpleWidget: boolean) { + return this._collection.acquire(uri.toString(), uri, value, languageSelection, isForSimpleWidget); + } +} + +class InlineTextModelReferenceCollection extends ReferenceCollection { + constructor(private readonly modelService: IModelService) { + super(); + } + + protected override createReferencedObject(key: string, uri: URI, value: string, languageSelection: ILanguageSelection | null, isForSimpleWidget: boolean): ITextModel { + return this.modelService.createModel(value, languageSelection, uri, isForSimpleWidget); + } + + protected override destroyReferencedObject(_key: string, model: ITextModel): void { + model.dispose(); + } } diff --git a/src/vs/workbench/contrib/chat/browser/widget/chatContentParts/toolInvocationParts/chatToolConfirmationSubPart.ts b/src/vs/workbench/contrib/chat/browser/widget/chatContentParts/toolInvocationParts/chatToolConfirmationSubPart.ts index 4d442375562..93eb27ff036 100644 --- a/src/vs/workbench/contrib/chat/browser/widget/chatContentParts/toolInvocationParts/chatToolConfirmationSubPart.ts +++ b/src/vs/workbench/contrib/chat/browser/widget/chatContentParts/toolInvocationParts/chatToolConfirmationSubPart.ts @@ -13,7 +13,6 @@ import { isEmptyObject } from '../../../../../../../base/common/types.js'; import { generateUuid } from '../../../../../../../base/common/uuid.js'; import { ElementSizeObserver } from '../../../../../../../editor/browser/config/elementSizeObserver.js'; import { ILanguageService } from '../../../../../../../editor/common/languages/language.js'; -import { IModelService } from '../../../../../../../editor/common/services/model.js'; import { localize } from '../../../../../../../nls.js'; import { ICommandService } from '../../../../../../../platform/commands/common/commands.js'; import { IContextKeyService } from '../../../../../../../platform/contextkey/common/contextkey.js'; @@ -53,7 +52,6 @@ export class ToolConfirmationSubPart extends AbstractToolConfirmationSubPart { private readonly codeBlockStartIndex: number, @IInstantiationService instantiationService: IInstantiationService, @IKeybindingService keybindingService: IKeybindingService, - @IModelService private readonly modelService: IModelService, @ILanguageService private readonly languageService: ILanguageService, @IContextKeyService contextKeyService: IContextKeyService, @IChatWidgetService chatWidgetService: IChatWidgetService, @@ -185,13 +183,14 @@ export class ToolConfirmationSubPart extends AbstractToolConfirmationSubPart { const langId = this.languageService.getLanguageIdByLanguageName('json'); const rawJsonInput = JSON.stringify(inputData.rawInput ?? {}, null, 1); const canSeeMore = count(rawJsonInput, '\n') > 2; // if more than one key:value - const model = this._register(this.modelService.createModel( + const modelRef = this._register(this.context.inlineTextModels.acquire( + createToolInputUri(toolInvocation.toolCallId), // View a single JSON line by default until they 'see more' rawJsonInput.replace(/\n */g, ' '), this.languageService.createById(langId), - createToolInputUri(toolInvocation.toolCallId), true )); + const model = modelRef.object; const markerOwner = generateUuid(); const schemaUri = createToolSchemaUri(toolInvocation.toolId); diff --git a/src/vs/workbench/contrib/chat/browser/widget/chatListRenderer.ts b/src/vs/workbench/contrib/chat/browser/widget/chatListRenderer.ts index f0af7f89d3d..e4fc306046d 100644 --- a/src/vs/workbench/contrib/chat/browser/widget/chatListRenderer.ts +++ b/src/vs/workbench/contrib/chat/browser/widget/chatListRenderer.ts @@ -78,7 +78,7 @@ import { ChatCodeCitationContentPart } from './chatContentParts/chatCodeCitation import { ChatCommandButtonContentPart } from './chatContentParts/chatCommandContentPart.js'; import { ChatConfirmationContentPart } from './chatContentParts/chatConfirmationContentPart.js'; import { DiffEditorPool, EditorPool } from './chatContentParts/chatContentCodePools.js'; -import { IChatContentPart, IChatContentPartRenderContext } from './chatContentParts/chatContentParts.js'; +import { IChatContentPart, IChatContentPartRenderContext, InlineTextModelCollection } from './chatContentParts/chatContentParts.js'; import { ChatElicitationContentPart } from './chatContentParts/chatElicitationContentPart.js'; import { ChatErrorConfirmationContentPart } from './chatContentParts/chatErrorConfirmationPart.js'; import { ChatErrorContentPart } from './chatContentParts/chatErrorContentPart.js'; @@ -243,6 +243,7 @@ export class ChatListItemRenderer extends Disposable implements ITreeRenderer acc + (part.codeblocks?.length ?? 0), 0); }, @@ -1285,6 +1288,7 @@ export class ChatListItemRenderer extends Disposable implements ITreeRenderer acc + (part.codeblocks?.length ?? 0), 0); }, diff --git a/src/vs/workbench/contrib/chat/test/browser/widget/chatContentParts/chatSubagentContentPart.test.ts b/src/vs/workbench/contrib/chat/test/browser/widget/chatContentParts/chatSubagentContentPart.test.ts index 9afc0363511..5dc3df666c6 100644 --- a/src/vs/workbench/contrib/chat/test/browser/widget/chatContentParts/chatSubagentContentPart.test.ts +++ b/src/vs/workbench/contrib/chat/test/browser/widget/chatContentParts/chatSubagentContentPart.test.ts @@ -13,7 +13,7 @@ import { mainWindow } from '../../../../../../../base/browser/window.js'; import { workbenchInstantiationService } from '../../../../../../test/browser/workbenchTestServices.js'; import { ChatSubagentContentPart } from '../../../../browser/widget/chatContentParts/chatSubagentContentPart.js'; import { IChatMarkdownContent, IChatSubagentToolInvocationData, IChatToolInvocation, IChatToolInvocationSerialized, ToolConfirmKind } from '../../../../common/chatService/chatService.js'; -import { IChatContentPartRenderContext } from '../../../../browser/widget/chatContentParts/chatContentParts.js'; +import { IChatContentPartRenderContext, InlineTextModelCollection } from '../../../../browser/widget/chatContentParts/chatContentParts.js'; import { IChatResponseViewModel } from '../../../../common/model/chatViewModel.js'; import { IChatMarkdownAnchorService } from '../../../../browser/widget/chatContentParts/chatMarkdownAnchorService.js'; import { IMarkdownRenderer } from '../../../../../../../platform/markdown/browser/markdownRenderer.js'; @@ -55,6 +55,7 @@ suite('ChatSubagentContentPart', () => { return { element: mockElement as IChatResponseViewModel, + inlineTextModels: {} as InlineTextModelCollection, elementIndex: 0, container: mainWindow.document.createElement('div'), content: [], diff --git a/src/vs/workbench/contrib/chat/test/browser/widget/chatContentParts/chatThinkingContentPart.test.ts b/src/vs/workbench/contrib/chat/test/browser/widget/chatContentParts/chatThinkingContentPart.test.ts index 15aa7762d9f..f11478cebc5 100644 --- a/src/vs/workbench/contrib/chat/test/browser/widget/chatContentParts/chatThinkingContentPart.test.ts +++ b/src/vs/workbench/contrib/chat/test/browser/widget/chatContentParts/chatThinkingContentPart.test.ts @@ -15,7 +15,7 @@ import { IConfigurationService } from '../../../../../../../platform/configurati import { TestConfigurationService } from '../../../../../../../platform/configuration/test/common/testConfigurationService.js'; import { ChatThinkingContentPart } from '../../../../browser/widget/chatContentParts/chatThinkingContentPart.js'; import { IChatMarkdownContent, IChatThinkingPart } from '../../../../common/chatService/chatService.js'; -import { IChatContentPartRenderContext } from '../../../../browser/widget/chatContentParts/chatContentParts.js'; +import { IChatContentPartRenderContext, InlineTextModelCollection } from '../../../../browser/widget/chatContentParts/chatContentParts.js'; import { IChatRendererContent, IChatResponseViewModel } from '../../../../common/model/chatViewModel.js'; import { IChatMarkdownAnchorService } from '../../../../browser/widget/chatContentParts/chatMarkdownAnchorService.js'; import { IMarkdownRenderer } from '../../../../../../../platform/markdown/browser/markdownRenderer.js'; @@ -49,6 +49,7 @@ suite('ChatThinkingContentPart', () => { return { element: mockElement as IChatResponseViewModel, + inlineTextModels: {} as InlineTextModelCollection, elementIndex: 0, container: mainWindow.document.createElement('div'), content: [], diff --git a/src/vs/workbench/test/browser/componentFixtures/chatProgressContentPart.fixture.ts b/src/vs/workbench/test/browser/componentFixtures/chatProgressContentPart.fixture.ts index 4136612fa98..d22153e5766 100644 --- a/src/vs/workbench/test/browser/componentFixtures/chatProgressContentPart.fixture.ts +++ b/src/vs/workbench/test/browser/componentFixtures/chatProgressContentPart.fixture.ts @@ -9,11 +9,11 @@ import { Event } from '../../../../base/common/event.js'; import { observableValue } from '../../../../base/common/observable.js'; import { Codicon } from '../../../../base/common/codicons.js'; import { ThemeIcon } from '../../../../base/common/themables.js'; -import { mock } from '../../../../base/test/common/mock.js'; +import { mock, upcastPartial } from '../../../../base/test/common/mock.js'; import { IMarkdownRendererService, MarkdownRendererService } from '../../../../platform/markdown/browser/markdownRenderer.js'; import { ChatProgressContentPart } from '../../../contrib/chat/browser/widget/chatContentParts/chatProgressContentPart.js'; import { ChatContentMarkdownRenderer } from '../../../contrib/chat/browser/widget/chatContentMarkdownRenderer.js'; -import { IChatContentPartRenderContext } from '../../../contrib/chat/browser/widget/chatContentParts/chatContentParts.js'; +import { IChatContentPartRenderContext, InlineTextModelCollection } from '../../../contrib/chat/browser/widget/chatContentParts/chatContentParts.js'; import { IChatMarkdownAnchorService } from '../../../contrib/chat/browser/widget/chatContentParts/chatMarkdownAnchorService.js'; import { IChatProgressMessage } from '../../../contrib/chat/common/chatService/chatService.js'; import { IChatResponseViewModel } from '../../../contrib/chat/common/model/chatViewModel.js'; @@ -27,6 +27,7 @@ function createMockContext(opts?: { isComplete?: boolean; hasFollowingContent?: }(); return { element, + inlineTextModels: upcastPartial({}), elementIndex: 0, container: document.createElement('div'), content: opts?.hasFollowingContent ? [{ kind: 'progressMessage', content: new MarkdownString('test') }] : [], diff --git a/src/vs/workbench/test/browser/componentFixtures/chatQuestionCarousel.fixture.ts b/src/vs/workbench/test/browser/componentFixtures/chatQuestionCarousel.fixture.ts index 0cef5d23bd7..383f9dea3ab 100644 --- a/src/vs/workbench/test/browser/componentFixtures/chatQuestionCarousel.fixture.ts +++ b/src/vs/workbench/test/browser/componentFixtures/chatQuestionCarousel.fixture.ts @@ -7,9 +7,9 @@ import * as dom from '../../../../base/browser/dom.js'; import { IMarkdownRendererService, MarkdownRendererService } from '../../../../platform/markdown/browser/markdownRenderer.js'; import { IChatQuestion, IChatQuestionCarousel } from '../../../contrib/chat/common/chatService/chatService.js'; import { ChatQuestionCarouselPart, IChatQuestionCarouselOptions } from '../../../contrib/chat/browser/widget/chatContentParts/chatQuestionCarouselPart.js'; -import { IChatContentPartRenderContext } from '../../../contrib/chat/browser/widget/chatContentParts/chatContentParts.js'; +import { IChatContentPartRenderContext, InlineTextModelCollection } from '../../../contrib/chat/browser/widget/chatContentParts/chatContentParts.js'; import { ComponentFixtureContext, createEditorServices, defineComponentFixture, defineThemedFixtureGroup } from './fixtureUtils.js'; -import { mock } from '../../../../base/test/common/mock.js'; +import { mock, upcastPartial } from '../../../../base/test/common/mock.js'; import { Event } from '../../../../base/common/event.js'; import { observableValue } from '../../../../base/common/observable.js'; import { IChatRequestViewModel } from '../../../contrib/chat/common/model/chatViewModel.js'; @@ -26,6 +26,7 @@ function createCarousel(questions: IChatQuestion[], allowSkip: boolean = true): function createMockContext(): IChatContentPartRenderContext { return { element: new class extends mock() { }(), + inlineTextModels: upcastPartial({}), elementIndex: 0, container: document.createElement('div'), content: [],