Skip to content

Commit 8ccd1da

Browse files
roblourensroblourens
authored
Some chat service cleanup (#280105)
* Clean up unused cancel token * Simplify shouldBeInHistory * Use real DisposableResourceMap
1 parent ceeabb7 commit 8ccd1da

File tree

15 files changed

+44
-96
lines changed

15 files changed

+44
-96
lines changed

‎src/vs/workbench/contrib/chat/browser/chatEditorInput.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -269,10 +269,10 @@ export class ChatEditorInput extends EditorInput implements IEditorCloseHandler
269269

270270
// For local session only, if we find no existing session, create a new one
271271
if (!this.model && LocalChatSessionUri.parseLocalSessionId(this._sessionResource)) {
272-
this.modelRef.value = this.chatService.startSession(ChatAgentLocation.Chat, CancellationToken.None, { canUseTools: true });
272+
this.modelRef.value = this.chatService.startSession(ChatAgentLocation.Chat, { canUseTools: true });
273273
}
274274
} else if (!this.options.target) {
275-
this.modelRef.value = this.chatService.startSession(ChatAgentLocation.Chat, CancellationToken.None, { canUseTools: !inputType });
275+
this.modelRef.value = this.chatService.startSession(ChatAgentLocation.Chat, { canUseTools: !inputType });
276276
} else if (this.options.target.data) {
277277
this.modelRef.value = this.chatService.loadSessionFromContent(this.options.target.data);
278278
}

‎src/vs/workbench/contrib/chat/browser/chatQuick.ts‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@
66
import * as dom from '../../../../base/browser/dom.js';
77
import { Orientation, Sash } from '../../../../base/browser/ui/sash/sash.js';
88
import { disposableTimeout } from '../../../../base/common/async.js';
9-
import { CancellationToken } from '../../../../base/common/cancellation.js';
109
import { Emitter, Event } from '../../../../base/common/event.js';
1110
import { MarkdownString } from '../../../../base/common/htmlContent.js';
1211
import { Disposable, DisposableStore, IDisposable, MutableDisposable } from '../../../../base/common/lifecycle.js';
@@ -395,7 +394,7 @@ class QuickChat extends Disposable {
395394
}
396395

397396
private updateModel(): void {
398-
this.modelRef ??= this.chatService.startSession(ChatAgentLocation.Chat, CancellationToken.None, { disableBackgroundKeepAlive: true });
397+
this.modelRef ??= this.chatService.startSession(ChatAgentLocation.Chat, { disableBackgroundKeepAlive: true });
399398
const model = this.modelRef?.object;
400399
if (!model) {
401400
throw new Error('Could not start chat session');

‎src/vs/workbench/contrib/chat/browser/chatViewPane.ts‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3,16 +3,16 @@
33
* Licensed under the MIT License. See License.txt in the project root for license information.
44
*--------------------------------------------------------------------------------------------*/
55

6-
import './media/chatViewPane.css';
76
import { $, append, getWindow, setVisibility } from '../../../../base/browser/dom.js';
8-
import { MenuWorkbenchToolBar } from '../../../../platform/actions/browser/toolbar.js';
9-
import { MenuId } from '../../../../platform/actions/common/actions.js';
107
import { CancellationToken } from '../../../../base/common/cancellation.js';
8+
import { Event } from '../../../../base/common/event.js';
119
import { MutableDisposable, toDisposable } from '../../../../base/common/lifecycle.js';
1210
import { MarshalledId } from '../../../../base/common/marshallingIds.js';
1311
import { autorun, IReader } from '../../../../base/common/observable.js';
1412
import { URI } from '../../../../base/common/uri.js';
1513
import { localize } from '../../../../nls.js';
14+
import { MenuWorkbenchToolBar } from '../../../../platform/actions/browser/toolbar.js';
15+
import { MenuId } from '../../../../platform/actions/common/actions.js';
1616
import { IConfigurationService } from '../../../../platform/configuration/common/configuration.js';
1717
import { IContextKeyService } from '../../../../platform/contextkey/common/contextkey.js';
1818
import { IContextMenuService } from '../../../../platform/contextview/browser/contextView.js';
@@ -42,10 +42,10 @@ import { LocalChatSessionUri } from '../common/chatUri.js';
4242
import { ChatAgentLocation, ChatConfiguration, ChatModeKind } from '../common/constants.js';
4343
import { showCloseActiveChatNotification } from './actions/chatCloseNotification.js';
4444
import { AgentSessionsControl } from './agentSessions/agentSessionsControl.js';
45+
import { AgentSessionsListDelegate } from './agentSessions/agentSessionsViewer.js';
4546
import { ChatWidget } from './chatWidget.js';
47+
import './media/chatViewPane.css';
4648
import { ChatViewWelcomeController, IViewWelcomeDelegate } from './viewsWelcome/chatViewWelcomeController.js';
47-
import { AgentSessionsListDelegate } from './agentSessions/agentSessionsViewer.js';
48-
import { Event } from '../../../../base/common/event.js';
4949

5050
interface IChatViewPaneState extends Partial<IChatModelInputState> {
5151
sessionId?: string;
@@ -179,7 +179,7 @@ export class ChatViewPane extends ViewPane implements IViewWelcomeDelegate {
179179

180180
const ref = modelRef ?? (this.chatService.transferredSessionData?.sessionId && this.chatService.transferredSessionData?.location === this.chatOptions.location
181181
? await this.chatService.getOrRestoreSession(LocalChatSessionUri.forSession(this.chatService.transferredSessionData.sessionId))
182-
: this.chatService.startSession(this.chatOptions.location, CancellationToken.None));
182+
: this.chatService.startSession(this.chatOptions.location));
183183
if (!ref) {
184184
throw new Error('Could not start chat session');
185185
}

‎src/vs/workbench/contrib/chat/common/chatModelStore.ts‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@
33
* Licensed under the MIT License. See License.txt in the project root for license information.
44
*--------------------------------------------------------------------------------------------*/
55

6-
import { CancellationToken } from '../../../../base/common/cancellation.js';
76
import { Emitter } from '../../../../base/common/event.js';
87
import { DisposableStore, IDisposable, IReference, ReferenceCollection } from '../../../../base/common/lifecycle.js';
98
import { ObservableMap } from '../../../../base/common/observable.js';
@@ -16,7 +15,6 @@ import { ChatAgentLocation } from './constants.js';
1615
export interface IStartSessionProps {
1716
readonly initialData?: IExportableChatData | ISerializableChatData;
1817
readonly location: ChatAgentLocation;
19-
readonly token: CancellationToken;
2018
readonly sessionResource: URI;
2119
readonly sessionId?: string;
2220
readonly canUseTools: boolean;

‎src/vs/workbench/contrib/chat/common/chatService.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -953,7 +953,7 @@ export interface IChatService {
953953

954954
isEnabled(location: ChatAgentLocation): boolean;
955955
hasSessions(): boolean;
956-
startSession(location: ChatAgentLocation, token: CancellationToken, options?: IChatSessionStartOptions): IChatModelReference;
956+
startSession(location: ChatAgentLocation, options?: IChatSessionStartOptions): IChatModelReference;
957957

958958
/**
959959
* Get an active session without holding a reference to it.

‎src/vs/workbench/contrib/chat/common/chatServiceImpl.ts‎

Lines changed: 8 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ import { BugIndicatingError, ErrorNoTelemetry } from '../../../../base/common/er
1010
import { Emitter, Event } from '../../../../base/common/event.js';
1111
import { MarkdownString } from '../../../../base/common/htmlContent.js';
1212
import { Iterable } from '../../../../base/common/iterator.js';
13-
import { Disposable, DisposableMap, DisposableStore, IDisposable, MutableDisposable } from '../../../../base/common/lifecycle.js';
13+
import { Disposable, DisposableResourceMap, DisposableStore, IDisposable, MutableDisposable } from '../../../../base/common/lifecycle.js';
1414
import { revive } from '../../../../base/common/marshalling.js';
1515
import { Schemas } from '../../../../base/common/network.js';
1616
import { autorun, derived, IObservable } from '../../../../base/common/observable.js';
@@ -72,38 +72,6 @@ class CancellableRequest implements IDisposable {
7272
}
7373
}
7474

75-
76-
77-
class DisposableResourceMap<T extends IDisposable> extends Disposable {
78-
79-
private readonly _map = this._register(new DisposableMap<string, T>());
80-
81-
get(sessionResource: URI) {
82-
return this._map.get(this.toKey(sessionResource));
83-
}
84-
85-
set(sessionResource: URI, value: T) {
86-
this._map.set(this.toKey(sessionResource), value);
87-
}
88-
89-
has(sessionResource: URI) {
90-
return this._map.has(this.toKey(sessionResource));
91-
}
92-
93-
deleteAndLeak(sessionResource: URI) {
94-
return this._map.deleteAndLeak(this.toKey(sessionResource));
95-
}
96-
97-
deleteAndDispose(sessionResource: URI) {
98-
this._map.deleteAndDispose(this.toKey(sessionResource));
99-
}
100-
101-
private toKey(uri: URI): string {
102-
return uri.toString();
103-
}
104-
}
105-
106-
10775
export class ChatService extends Disposable implements IChatService {
10876
declare _serviceBrand: undefined;
10977

@@ -396,7 +364,7 @@ export class ChatService extends Disposable implements IChatService {
396364
async getHistorySessionItems(): Promise<IChatDetail[]> {
397365
const index = await this._chatSessionStore.getIndex();
398366
return Object.values(index)
399-
.filter(entry => !this._sessionModels.has(LocalChatSessionUri.forSession(entry.sessionId)) && this.shouldBeInHistory(entry) && !entry.isEmpty)
367+
.filter(entry => !this._sessionModels.has(LocalChatSessionUri.forSession(entry.sessionId)) && entry.initialLocation === ChatAgentLocation.Chat && !entry.isEmpty)
400368
.map((entry): IChatDetail => {
401369
const sessionResource = LocalChatSessionUri.forSession(entry.sessionId);
402370
return ({
@@ -407,11 +375,8 @@ export class ChatService extends Disposable implements IChatService {
407375
});
408376
}
409377

410-
private shouldBeInHistory(entry: Partial<ChatModel>) {
411-
if (entry.sessionResource) {
412-
return !entry.isImported && LocalChatSessionUri.parseLocalSessionId(entry.sessionResource) && entry.initialLocation === ChatAgentLocation.Chat;
413-
}
414-
return !entry.isImported && entry.initialLocation === ChatAgentLocation.Chat;
378+
private shouldBeInHistory(entry: ChatModel): boolean {
379+
return !entry.isImported && !!LocalChatSessionUri.parseLocalSessionId(entry.sessionResource) && entry.initialLocation === ChatAgentLocation.Chat;
415380
}
416381

417382
async removeHistoryEntry(sessionResource: URI): Promise<void> {
@@ -422,14 +387,13 @@ export class ChatService extends Disposable implements IChatService {
422387
await this._chatSessionStore.clearAllSessions();
423388
}
424389

425-
startSession(location: ChatAgentLocation, token: CancellationToken, options?: IChatSessionStartOptions): IChatModelReference {
390+
startSession(location: ChatAgentLocation, options?: IChatSessionStartOptions): IChatModelReference {
426391
this.trace('startSession');
427392
const sessionId = generateUuid();
428393
const sessionResource = LocalChatSessionUri.forSession(sessionId);
429394
return this._sessionModels.acquireOrCreate({
430395
initialData: undefined,
431396
location,
432-
token,
433397
sessionResource,
434398
sessionId,
435399
canUseTools: options?.canUseTools ?? true,
@@ -438,17 +402,17 @@ export class ChatService extends Disposable implements IChatService {
438402
}
439403

440404
private _startSession(props: IStartSessionProps): ChatModel {
441-
const { initialData, location, token, sessionResource, sessionId, canUseTools, transferEditingSession, disableBackgroundKeepAlive } = props;
405+
const { initialData, location, sessionResource, sessionId, canUseTools, transferEditingSession, disableBackgroundKeepAlive } = props;
442406
const model = this.instantiationService.createInstance(ChatModel, initialData, { initialLocation: location, canUseTools, resource: sessionResource, sessionId, disableBackgroundKeepAlive });
443407
if (location === ChatAgentLocation.Chat) {
444408
model.startEditingSession(true, transferEditingSession);
445409
}
446410

447-
this.initializeSession(model, token);
411+
this.initializeSession(model);
448412
return model;
449413
}
450414

451-
private initializeSession(model: ChatModel, token: CancellationToken): void {
415+
private initializeSession(model: ChatModel): void {
452416
this.trace('initializeSession', `Initialize session ${model.sessionResource}`);
453417

454418
// Activate the default extension provided agent but do not wait
@@ -516,7 +480,6 @@ export class ChatService extends Disposable implements IChatService {
516480
const sessionRef = this._sessionModels.acquireOrCreate({
517481
initialData: sessionData,
518482
location: sessionData.initialLocation ?? ChatAgentLocation.Chat,
519-
token: CancellationToken.None,
520483
sessionResource,
521484
sessionId,
522485
canUseTools: true,
@@ -583,7 +546,6 @@ export class ChatService extends Disposable implements IChatService {
583546
return this._sessionModels.acquireOrCreate({
584547
initialData: data,
585548
location: data.initialLocation ?? ChatAgentLocation.Chat,
586-
token: CancellationToken.None,
587549
sessionResource,
588550
sessionId,
589551
canUseTools: true,
@@ -609,7 +571,6 @@ export class ChatService extends Disposable implements IChatService {
609571
const modelRef = this._sessionModels.acquireOrCreate({
610572
initialData: undefined,
611573
location,
612-
token: CancellationToken.None,
613574
sessionResource: chatSessionResource,
614575
canUseTools: false,
615576
transferEditingSession: providedSession.initialEditingSession,

‎src/vs/workbench/contrib/chat/test/browser/chatEditingService.test.ts‎

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@
44
*--------------------------------------------------------------------------------------------*/
55

66
import assert from 'assert';
7-
import { CancellationToken } from '../../../../../base/common/cancellation.js';
87
import { Disposable, DisposableStore, IDisposable } from '../../../../../base/common/lifecycle.js';
98
import { waitForState } from '../../../../../base/common/observable.js';
109
import { isEqual } from '../../../../../base/common/resources.js';
@@ -140,7 +139,7 @@ suite('ChatEditingService', function () {
140139
test('create session', async function () {
141140
assert.ok(editingService);
142141

143-
const modelRef = chatService.startSession(ChatAgentLocation.EditorInline, CancellationToken.None);
142+
const modelRef = chatService.startSession(ChatAgentLocation.EditorInline);
144143
const model = modelRef.object as ChatModel;
145144
const session = editingService.createEditingSession(model, true);
146145

@@ -161,7 +160,7 @@ suite('ChatEditingService', function () {
161160

162161
const uri = URI.from({ scheme: 'test', path: 'HelloWorld' });
163162

164-
const modelRef = store.add(chatService.startSession(ChatAgentLocation.Chat, CancellationToken.None));
163+
const modelRef = store.add(chatService.startSession(ChatAgentLocation.Chat));
165164
const model = modelRef.object as ChatModel;
166165
const session = model.editingSession;
167166
if (!session) {
@@ -218,7 +217,7 @@ suite('ChatEditingService', function () {
218217

219218
const uri = URI.from({ scheme: 'test', path: 'abc\n' });
220219

221-
const modelRef = store.add(chatService.startSession(ChatAgentLocation.Chat, CancellationToken.None));
220+
const modelRef = store.add(chatService.startSession(ChatAgentLocation.Chat));
222221
const model = modelRef.object as ChatModel;
223222
const session = model.editingSession;
224223
assertType(session, 'session not created');
@@ -252,7 +251,7 @@ suite('ChatEditingService', function () {
252251

253252
const uri = URI.from({ scheme: 'test', path: 'abc\n' });
254253

255-
const modelRef = store.add(chatService.startSession(ChatAgentLocation.Chat, CancellationToken.None));
254+
const modelRef = store.add(chatService.startSession(ChatAgentLocation.Chat));
256255
const model = modelRef.object as ChatModel;
257256
const session = model.editingSession;
258257
assertType(session, 'session not created');
@@ -286,7 +285,7 @@ suite('ChatEditingService', function () {
286285

287286
const uri = URI.from({ scheme: 'test', path: 'abc\n' });
288287

289-
const modelRef = store.add(chatService.startSession(ChatAgentLocation.Chat, CancellationToken.None));
288+
const modelRef = store.add(chatService.startSession(ChatAgentLocation.Chat));
290289
const model = modelRef.object as ChatModel;
291290
const session = model.editingSession;
292291
assertType(session, 'session not created');
@@ -322,7 +321,7 @@ suite('ChatEditingService', function () {
322321

323322
const modified = store.add(await textModelService.createModelReference(uri)).object.textEditorModel;
324323

325-
const modelRef = store.add(chatService.startSession(ChatAgentLocation.Chat, CancellationToken.None));
324+
const modelRef = store.add(chatService.startSession(ChatAgentLocation.Chat));
326325
const model = modelRef.object as ChatModel;
327326
const session = model.editingSession;
328327
assertType(session, 'session not created');

‎src/vs/workbench/contrib/chat/test/browser/localAgentSessionsProvider.test.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ import { workbenchInstantiationService } from '../../../../test/browser/workbenc
1717
import { LocalAgentsSessionsProvider } from '../../browser/agentSessions/localAgentSessionsProvider.js';
1818
import { ModifiedFileEntryState } from '../../common/chatEditingService.js';
1919
import { IChatModel, IChatRequestModel, IChatResponseModel } from '../../common/chatModel.js';
20-
import { IChatDetail, IChatService } from '../../common/chatService.js';
20+
import { IChatDetail, IChatService, IChatSessionStartOptions } from '../../common/chatService.js';
2121
import { ChatSessionStatus, IChatSessionsService, localChatSessionType } from '../../common/chatSessionsService.js';
2222
import { LocalChatSessionUri } from '../../common/chatUri.js';
2323
import { ChatAgentLocation } from '../../common/constants.js';
@@ -76,7 +76,7 @@ class MockChatService implements IChatService {
7676
return [];
7777
}
7878

79-
startSession(_location: ChatAgentLocation, _token: CancellationToken): any {
79+
startSession(_location: ChatAgentLocation, _options?: IChatSessionStartOptions): any {
8080
throw new Error('Method not implemented.');
8181
}
8282

‎src/vs/workbench/contrib/chat/test/common/chatModelStore.test.ts‎

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@
55

66
import assert from 'assert';
77
import { DeferredPromise } from '../../../../../base/common/async.js';
8-
import { CancellationToken } from '../../../../../base/common/cancellation.js';
98
import { URI } from '../../../../../base/common/uri.js';
109
import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../../base/test/common/utils.js';
1110
import { NullLogService } from '../../../../../platform/log/common/log.js';
@@ -43,7 +42,6 @@ suite('ChatModelStore', () => {
4342
const props: IStartSessionProps = {
4443
sessionResource: uri,
4544
location: ChatAgentLocation.Chat,
46-
token: CancellationToken.None,
4745
canUseTools: true
4846
};
4947

@@ -64,7 +62,6 @@ suite('ChatModelStore', () => {
6462
const props: IStartSessionProps = {
6563
sessionResource: uri,
6664
location: ChatAgentLocation.Chat,
67-
token: CancellationToken.None,
6865
canUseTools: true
6966
};
7067

@@ -96,7 +93,6 @@ suite('ChatModelStore', () => {
9693
const props: IStartSessionProps = {
9794
sessionResource: uri,
9895
location: ChatAgentLocation.Chat,
99-
token: CancellationToken.None,
10096
canUseTools: true
10197
};
10298

@@ -117,7 +113,6 @@ suite('ChatModelStore', () => {
117113
const props: IStartSessionProps = {
118114
sessionResource: uri,
119115
location: ChatAgentLocation.Chat,
120-
token: CancellationToken.None,
121116
canUseTools: true
122117
};
123118

@@ -140,13 +135,11 @@ suite('ChatModelStore', () => {
140135
const props1: IStartSessionProps = {
141136
sessionResource: uri1,
142137
location: ChatAgentLocation.Chat,
143-
token: CancellationToken.None,
144138
canUseTools: true
145139
};
146140
const props2: IStartSessionProps = {
147141
sessionResource: uri2,
148142
location: ChatAgentLocation.Chat,
149-
token: CancellationToken.None,
150143
canUseTools: true
151144
};
152145

@@ -170,7 +163,6 @@ suite('ChatModelStore', () => {
170163
const props: IStartSessionProps = {
171164
sessionResource: uri,
172165
location: ChatAgentLocation.Chat,
173-
token: CancellationToken.None,
174166
canUseTools: true
175167
};
176168

0 commit comments

Comments
 (0)