Skip to content

Commit 36b8f6d

Browse files
committed
inline chat: fix leaks found in testing
Fixes some leaks I found while looking at #190444, by adding ensureNoDisposablesAreLeakedInTestSuite to the suite. I don't have a dev setup for inline chat, so I have not tested this beyond running tests and verifying the fix
1 parent 6aba138 commit 36b8f6d

File tree

9 files changed

+26
-29
lines changed

9 files changed

+26
-29
lines changed

src/vs/base/browser/ui/toolbar/toolbar.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -212,6 +212,7 @@ export class ToolBar extends Disposable {
212212

213213
override dispose(): void {
214214
this.clear();
215+
this.disposables.dispose();
215216
super.dispose();
216217
}
217218
}

src/vs/editor/browser/widget/diffEditorWidget2/diffEditorEditors.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -35,8 +35,8 @@ export class DiffEditorEditors extends Disposable {
3535
) {
3636
super();
3737

38-
this.original = this._createLeftHandSideEditor(_options.editorOptions.get(), codeEditorWidgetOptions.originalEditor || {});
39-
this.modified = this._createRightHandSideEditor(_options.editorOptions.get(), codeEditorWidgetOptions.modifiedEditor || {});
38+
this.original = this._register(this._createLeftHandSideEditor(_options.editorOptions.get(), codeEditorWidgetOptions.originalEditor || {}));
39+
this.modified = this._register(this._createRightHandSideEditor(_options.editorOptions.get(), codeEditorWidgetOptions.modifiedEditor || {}));
4040

4141
this._register(autorunHandleChanges({
4242
createEmptyChangeSummary: () => ({} as IDiffEditorConstructionOptions),

src/vs/editor/contrib/suggest/browser/suggestController.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -143,7 +143,7 @@ export class SuggestController implements IEditorContribution {
143143
// context key: update insert/replace mode
144144
const ctxInsertMode = SuggestContext.InsertMode.bindTo(_contextKeyService);
145145
ctxInsertMode.set(editor.getOption(EditorOption.suggest).insertMode);
146-
this.model.onDidTrigger(() => ctxInsertMode.set(editor.getOption(EditorOption.suggest).insertMode));
146+
this._toDispose.add(this.model.onDidTrigger(() => ctxInsertMode.set(editor.getOption(EditorOption.suggest).insertMode)));
147147

148148
this.widget = this._toDispose.add(new IdleValue(() => {
149149

src/vs/platform/actions/browser/buttonbar.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ export class MenuWorkbenchButtonBar extends ButtonBar {
5555
'workbenchActionExecuted',
5656
{ id: e.action.id, from: options.telemetrySource! }
5757
);
58-
}, this._store);
58+
}, undefined, this._store);
5959
}
6060

6161
const conifgProvider: IButtonConfigProvider = options?.buttonConfigProvider ?? (() => ({ showLabel: true }));

src/vs/workbench/contrib/inlineChat/browser/inlineChatController.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -100,7 +100,7 @@ export class InlineChatController implements IEditorContribution {
100100

101101
private _messages = this._store.add(new Emitter<Message>());
102102

103-
private readonly _sessionStore: DisposableStore = new DisposableStore();
103+
private readonly _sessionStore: DisposableStore = this._store.add(new DisposableStore());
104104
private readonly _stashedSession: MutableDisposable<StashedSession> = this._store.add(new MutableDisposable());
105105
private _activeSession?: Session;
106106
private _strategy?: EditModeStrategy;
@@ -146,6 +146,7 @@ export class InlineChatController implements IEditorContribution {
146146
}
147147

148148
dispose(): void {
149+
this._strategy?.dispose();
149150
this._stashedSession.clear();
150151
this.finishExistingSession();
151152
this._store.dispose();

src/vs/workbench/contrib/inlineChat/browser/inlineChatSession.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -385,6 +385,8 @@ export interface IInlineChatSessionService {
385385
//
386386

387387
recordings(): readonly Recording[];
388+
389+
dispose(): void;
388390
}
389391

390392
type SessionData = {

src/vs/workbench/contrib/inlineChat/browser/inlineChatWidget.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -230,7 +230,7 @@ export class InlineChatWidget {
230230
}));
231231

232232
const uri = URI.from({ scheme: 'vscode', authority: 'inline-chat', path: `/inline-chat/model${InlineChatWidget._modelPool++}.txt` });
233-
this._inputModel = this._modelService.getModel(uri) ?? this._modelService.createModel('', null, uri);
233+
this._inputModel = this._store.add(this._modelService.getModel(uri) ?? this._modelService.createModel('', null, uri));
234234
this._inputEditor.setModel(this._inputModel);
235235

236236
// --- context keys
@@ -359,13 +359,13 @@ export class InlineChatWidget {
359359
this._store.add(feedbackToolbar);
360360

361361
// preview editors
362-
this._previewDiffEditor = new IdleValue(() => this._store.add(_instantiationService.createInstance(EmbeddedDiffEditorWidget2, this._elements.previewDiff, {
362+
this._previewDiffEditor = this._store.add(new IdleValue(() => this._store.add(_instantiationService.createInstance(EmbeddedDiffEditorWidget2, this._elements.previewDiff, {
363363
..._previewEditorEditorOptions,
364364
onlyShowAccessibleDiffViewer: this._accessibilityService.isScreenReaderOptimized(),
365-
}, { modifiedEditor: codeEditorWidgetOptions, originalEditor: codeEditorWidgetOptions }, parentEditor)));
365+
}, { modifiedEditor: codeEditorWidgetOptions, originalEditor: codeEditorWidgetOptions }, parentEditor))));
366366

367367
this._previewCreateTitle = this._store.add(_instantiationService.createInstance(ResourceLabel, this._elements.previewCreateTitle, { supportIcons: true }));
368-
this._previewCreateEditor = new IdleValue(() => this._store.add(_instantiationService.createInstance(EmbeddedCodeEditorWidget, this._elements.previewCreate, _previewEditorEditorOptions, codeEditorWidgetOptions, parentEditor)));
368+
this._previewCreateEditor = this._store.add(new IdleValue(() => this._store.add(_instantiationService.createInstance(EmbeddedCodeEditorWidget, this._elements.previewCreate, _previewEditorEditorOptions, codeEditorWidgetOptions, parentEditor))));
369369

370370
this._elements.message.tabIndex = 0;
371371
this._elements.message.ariaLabel = this._accessibleViewService.getOpenAriaHint(AccessibilityVerbositySettingId.InlineChat);

src/vs/workbench/contrib/inlineChat/test/browser/inlineChatController.test.ts

Lines changed: 10 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -24,11 +24,11 @@ import { IEditorProgressService, IProgressRunner } from 'vs/platform/progress/co
2424
import { mock } from 'vs/base/test/common/mock';
2525
import { Emitter, Event } from 'vs/base/common/event';
2626
import { equals } from 'vs/base/common/arrays';
27-
import { timeout } from 'vs/base/common/async';
2827
import { IChatAccessibilityService } from 'vs/workbench/contrib/chat/browser/chat';
2928
import { IChatResponseViewModel } from 'vs/workbench/contrib/chat/common/chatViewModel';
3029
import { IAccessibleViewService } from 'vs/workbench/contrib/accessibility/browser/accessibleView';
3130
import { AccessibilityVerbositySettingId } from 'vs/workbench/contrib/accessibility/browser/accessibilityConfiguration';
31+
import { ensureNoDisposablesAreLeakedInTestSuite } from 'vs/base/test/common/utils';
3232

3333
suite('InteractiveChatController', function () {
3434

@@ -114,11 +114,11 @@ suite('InteractiveChatController', function () {
114114
}]
115115
);
116116

117-
instaService = workbenchInstantiationService(undefined, store).createChild(serviceCollection);
118-
inlineChatSessionService = instaService.get(IInlineChatSessionService);
117+
instaService = store.add(workbenchInstantiationService(undefined, store).createChild(serviceCollection));
118+
inlineChatSessionService = store.add(instaService.get(IInlineChatSessionService));
119119

120-
model = instaService.get(IModelService).createModel('Hello\nWorld\nHello Again\nHello World\n', null);
121-
editor = instantiateTestCodeEditor(instaService, model);
120+
model = store.add(instaService.get(IModelService).createModel('Hello\nWorld\nHello Again\nHello World\n', null));
121+
editor = store.add(instantiateTestCodeEditor(instaService, model));
122122

123123
store.add(inlineChatService.addProvider({
124124
debugName: 'Unit Test',
@@ -142,13 +142,14 @@ suite('InteractiveChatController', function () {
142142
});
143143

144144
teardown(function () {
145-
editor.dispose();
146-
model.dispose();
147145
store.clear();
148146
ctrl?.dispose();
149147
});
150148

149+
ensureNoDisposablesAreLeakedInTestSuite();
150+
151151
test('creation, not showing anything', function () {
152+
for (let deadline = Date.now() + 1000; Date.now() < deadline;) { }
152153
ctrl = instaService.createInstance(TestController, editor);
153154
assert.ok(ctrl);
154155
assert.strictEqual(ctrl.getWidgetPosition(), undefined);
@@ -295,19 +296,8 @@ suite('InteractiveChatController', function () {
295296
wholeRange: new Range(3, 1, 3, 3)
296297
};
297298
},
298-
async provideResponse(session, request) {
299-
300-
// SLOW response
301-
await timeout(50000);
302-
303-
return {
304-
type: InlineChatResponseType.EditorEdit,
305-
id: Math.random(),
306-
edits: [{
307-
range: new Range(1, 1, 1, 1), // EDIT happens outside of whole range
308-
text: `${request.prompt}\n${request.prompt}`
309-
}]
310-
};
299+
provideResponse(session, request) {
300+
return new Promise<never>(() => { });
311301
}
312302
});
313303
store.add(d);

src/vs/workbench/contrib/markers/test/browser/markersModel.test.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import { URI } from 'vs/base/common/uri';
88
import { IMarker, MarkerSeverity, IRelatedInformation } from 'vs/platform/markers/common/markers';
99
import { MarkersModel, Marker, ResourceMarkers, RelatedInformation } from 'vs/workbench/contrib/markers/browser/markersModel';
1010
import { groupBy } from 'vs/base/common/collections';
11+
import { ensureNoDisposablesAreLeakedInTestSuite } from 'vs/base/test/common/utils';
1112

1213
class TestMarkersModel extends MarkersModel {
1314

@@ -27,6 +28,8 @@ class TestMarkersModel extends MarkersModel {
2728

2829
suite('MarkersModel Test', () => {
2930

31+
ensureNoDisposablesAreLeakedInTestSuite();
32+
3033
test('marker ids are unique', function () {
3134
const marker1 = anErrorWithRange(3);
3235
const marker2 = anErrorWithRange(3);

0 commit comments

Comments
 (0)