From ea959a9027c1aea2039a829932a29ed8fa44caee Mon Sep 17 00:00:00 2001 From: Martin Aeschlimann Date: Sat, 28 Mar 2026 07:39:07 +0100 Subject: [PATCH] PromptsService: remove unnecessary change events (#305662) * remove unnecessary change events * Update src/vs/workbench/contrib/chat/common/promptSyntax/service/promptsServiceImpl.ts Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update src/vs/workbench/contrib/chat/common/promptSyntax/service/promptsServiceImpl.ts Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- .../service/promptsServiceImpl.ts | 96 ++++++++++++------- .../promptSyntax/utils/promptFilesLocator.ts | 18 ++-- .../service/promptsService.test.ts | 17 +++- 3 files changed, 82 insertions(+), 49 deletions(-) diff --git a/src/vs/workbench/contrib/chat/common/promptSyntax/service/promptsServiceImpl.ts b/src/vs/workbench/contrib/chat/common/promptSyntax/service/promptsServiceImpl.ts index 4e617edc4ae..2220a859130 100644 --- a/src/vs/workbench/contrib/chat/common/promptSyntax/service/promptsServiceImpl.ts +++ b/src/vs/workbench/contrib/chat/common/promptSyntax/service/promptsServiceImpl.ts @@ -101,12 +101,12 @@ export class PromptsService extends Disposable implements IPromptsService { private readonly internalCustomizations: ChatInternalCustomizations; /** - * Cached custom agents. Caching only happens if the `onDidChangeCustomAgents` event is used. + * Cached custom agents. */ private readonly cachedCustomAgents: CachedPromise; /** - * Cached slash commands. Caching only happens if the `onDidChangeSlashCommands` event is used. + * Cached slash commands. */ private readonly cachedSlashCommands: CachedPromise; @@ -116,10 +116,15 @@ export class PromptsService extends Disposable implements IPromptsService { private readonly cachedHooks: CachedPromise; /** - * Cached skills. Caching only happens if the `onDidChangeSkills` event is used. + * Cached skills. */ private readonly cachedSkills: CachedPromise; + /** + * Cached instructions. + */ + private readonly cachedInstructions: CachedPromise; + /** * Cache for parsed prompt files keyed by URI. * The number in the returned tuple is textModel.getVersionId(), which is an internal VS Code counter that increments every time the text model's content changes. @@ -231,7 +236,11 @@ export class PromptsService extends Disposable implements IPromptsService { this.cachedSkills = this._register(new CachedPromise( (token) => this.computeAgentSkills(token), - () => Event.any(this.getFileLocatorEvent(PromptsType.skill), Event.filter(modelChangeEvent, e => e.promptType === PromptsType.skill), this._onDidContributedWhenChange.event, this._onDidPluginPromptFilesChange.event) + () => Event.any( + this.getFileLocatorEvent(PromptsType.skill), + Event.filter(modelChangeEvent, e => e.promptType === PromptsType.skill), + this._onDidContributedWhenChange.event, + this._onDidPluginPromptFilesChange.event) )); this.cachedHooks = this._register(new CachedPromise( @@ -244,9 +253,15 @@ export class PromptsService extends Disposable implements IPromptsService { ) )); - // Hack: Subscribe to activate caching (CachedPromise only caches when onDidChange has listeners) - this._register(this.cachedSkills.onDidChange(() => { })); - this._register(this.cachedHooks.onDidChange(() => { })); + this.cachedInstructions = this._register(new CachedPromise( + (token) => this.computeInstructionFiles(token), + () => Event.any( + this.getFileLocatorEvent(PromptsType.instructions), + this._onDidContributedWhenChange.event, + this._onDidChangeInstructions.event, + this._onDidPluginPromptFilesChange.event, + ) + )); this._register(this.watchPluginPromptFilesForType( PromptsType.prompt, @@ -587,7 +602,7 @@ export class PromptsService extends Disposable implements IPromptsService { * Emitter for slash commands change events. */ public get onDidChangeSlashCommands(): Event { - return this.cachedSlashCommands.onDidChange; + return this.cachedSlashCommands.onDidChangePromise; } public async getPromptSlashCommands(token: CancellationToken, sessionResource?: URI): Promise { @@ -684,16 +699,11 @@ export class PromptsService extends Disposable implements IPromptsService { * Emitter for custom agents change events. */ public get onDidChangeCustomAgents(): Event { - return this.cachedCustomAgents.onDidChange; + return this.cachedCustomAgents.onDidChangePromise; } public get onDidChangeInstructions(): Event { - return Event.any( - this.getFileLocatorEvent(PromptsType.instructions), - this._onDidContributedWhenChange.event, - this._onDidChangeInstructions.event, - this._onDidPluginPromptFilesChange.event, - ); + return this.cachedInstructions.onDidChangePromise; } public async getCustomAgents(token: CancellationToken, sessionResource?: URI): Promise { @@ -1099,7 +1109,7 @@ export class PromptsService extends Disposable implements IPromptsService { } public get onDidChangeSkills(): Event { - return this.cachedSkills.onDidChange; + return this.cachedSkills.onDidChangePromise; } public async findAgentSkills(token: CancellationToken, sessionResource?: URI): Promise { @@ -1259,7 +1269,7 @@ export class PromptsService extends Disposable implements IPromptsService { public async getInstructionFiles(token: CancellationToken, sessionResource?: URI): Promise { const sw = StopWatch.create(); - const result = await this.listPromptFiles(PromptsType.instructions, token); + const result = await this.cachedInstructions.get(token); if (sessionResource) { const elapsed = sw.elapsed(); void this.getInstructionsDiscoveryInfo(token).catch(() => undefined).then(discoveryInfo => { @@ -1278,6 +1288,10 @@ export class PromptsService extends Disposable implements IPromptsService { return result; } + private async computeInstructionFiles(token: CancellationToken): Promise { + return await this.listPromptFiles(PromptsType.instructions, token); + } + private async computeHooks(token: CancellationToken): Promise { const useChatHooks = this.configurationService.getValue(PromptsConfig.USE_CHAT_HOOKS); if (!useChatHooks) { @@ -1715,21 +1729,19 @@ export class PromptsService extends Disposable implements IPromptsService { class CachedPromise extends Disposable { private cachedPromise: Promise | undefined = undefined; - private onDidUpdatePromiseEmitter: Emitter | undefined = undefined; + private readonly onDidUpdatePromiseEmitter: Emitter; constructor(private readonly computeFn: (token: CancellationToken) => Promise, private readonly getEvent: () => Event, private readonly delay: number = 0) { super(); + this.onDidUpdatePromiseEmitter = this._register(new Emitter()); + const delayer = this._register(new Delayer(this.delay)); + this._register(this.getEvent()(() => { + this.cachedPromise = undefined; + delayer.trigger(() => this.onDidUpdatePromiseEmitter.fire()); + })); } - public get onDidChange(): Event { - if (!this.onDidUpdatePromiseEmitter) { - const emitter = this.onDidUpdatePromiseEmitter = this._register(new Emitter()); - const delayer = this._register(new Delayer(this.delay)); - this._register(this.getEvent()(() => { - this.cachedPromise = undefined; - delayer.trigger(() => emitter.fire()); - })); - } + public get onDidChangePromise(): Event { return this.onDidUpdatePromiseEmitter.event; } @@ -1737,13 +1749,14 @@ class CachedPromise extends Disposable { if (this.cachedPromise !== undefined) { return this.cachedPromise; } - const result = this.computeFn(token); - if (!this.onDidUpdatePromiseEmitter) { - return result; // only cache if there is an event listener - } - this.cachedPromise = result; - this.onDidUpdatePromiseEmitter.fire(); - return result; + const promise = this.computeFn(token).catch(err => { + if (this.cachedPromise === promise) { + this.cachedPromise = undefined; + } + throw err; + }); + this.cachedPromise = promise; + return promise; } public refresh(): void { @@ -1774,19 +1787,28 @@ class ModelChangeTracker extends Disposable { if (promptType !== undefined) { this.listeners.set(model.uri, model.onDidChangeContent(() => this.onDidPromptModelChange.fire({ uri: model.uri, promptType }))); } + return promptType; }; const onRemove = (languageId: string, uri: URI) => { const promptType = getPromptsTypeForLanguageId(languageId); if (promptType !== undefined) { this.listeners.get(uri)?.dispose(); this.listeners.delete(uri); - this.onDidPromptModelChange.fire({ uri, promptType }); } + return promptType; }; this._register(modelService.onModelAdded(model => onAdd(model))); this._register(modelService.onModelLanguageChanged(e => { - onRemove(e.oldLanguageId, e.model.uri); - onAdd(e.model); + const removedPromptType = onRemove(e.oldLanguageId, e.model.uri); + const addedPromptType = onAdd(e.model); + if (removedPromptType !== addedPromptType) { + if (removedPromptType) { + this.onDidPromptModelChange.fire({ uri: e.model.uri, promptType: removedPromptType }); + } + if (addedPromptType) { + this.onDidPromptModelChange.fire({ uri: e.model.uri, promptType: addedPromptType }); + } + } })); this._register(modelService.onModelRemoved(model => onRemove(model.getLanguageId(), model.uri))); } diff --git a/src/vs/workbench/contrib/chat/common/promptSyntax/utils/promptFilesLocator.ts b/src/vs/workbench/contrib/chat/common/promptSyntax/utils/promptFilesLocator.ts index 21de3116a39..131e6615c75 100644 --- a/src/vs/workbench/contrib/chat/common/promptSyntax/utils/promptFilesLocator.ts +++ b/src/vs/workbench/contrib/chat/common/promptSyntax/utils/promptFilesLocator.ts @@ -194,7 +194,7 @@ export class PromptFilesLocator { } }; - const update = async (emitEvent: boolean) => { + const update = async () => { try { const configuredLocations = PromptsConfig.promptSourceFolders(this.configService, type); parentFolders = await this.toAbsoluteLocations(type, configuredLocations, undefined); @@ -203,23 +203,23 @@ export class PromptFilesLocator { return; } updateExternalFolderWatchers(); - if (emitEvent) { - eventEmitter.fire(); - } } catch (err) { this.logService.error(`Error updating prompt file watchers after config change:`, err); } }; disposables.add(this.configService.onDidChangeConfiguration(e => { - if (e.affectsConfiguration(key)) { - void update(true); + if (e.affectsConfiguration(key) || e.affectsConfiguration(PromptsConfig.USE_CUSTOMIZATIONS_IN_PARENT_REPOS)) { + void update(); + eventEmitter.fire(); } })); disposables.add(this.onDidChangeWorkspaceFolders()(() => { - void update(true); + void update(); + eventEmitter.fire(); })); disposables.add(this.workspaceTrustManagementService.onDidChangeTrustedFolders(() => { - void update(true); + void update(); + eventEmitter.fire(); })); disposables.add(this.fileService.onDidFilesChange(e => { if (e.affects(userDataFolder)) { @@ -233,7 +233,7 @@ export class PromptFilesLocator { })); disposables.add(this.fileService.watch(userDataFolder)); - void update(false); + void update(); return { event: eventEmitter.event, dispose: () => disposables.dispose() }; } diff --git a/src/vs/workbench/contrib/chat/test/common/promptSyntax/service/promptsService.test.ts b/src/vs/workbench/contrib/chat/test/common/promptSyntax/service/promptsService.test.ts index cd90c0c7e7f..1f81fac3ddb 100644 --- a/src/vs/workbench/contrib/chat/test/common/promptSyntax/service/promptsService.test.ts +++ b/src/vs/workbench/contrib/chat/test/common/promptSyntax/service/promptsService.test.ts @@ -18,7 +18,7 @@ import { Range } from '../../../../../../../editor/common/core/range.js'; import { ILanguageService } from '../../../../../../../editor/common/languages/language.js'; import { IModelService } from '../../../../../../../editor/common/services/model.js'; import { ModelService } from '../../../../../../../editor/common/services/modelService.js'; -import { IConfigurationService } from '../../../../../../../platform/configuration/common/configuration.js'; +import { IConfigurationChangeEvent, IConfigurationService } from '../../../../../../../platform/configuration/common/configuration.js'; import { TestConfigurationService } from '../../../../../../../platform/configuration/test/common/testConfigurationService.js'; import { IExtensionDescription } from '../../../../../../../platform/extensions/common/extensions.js'; import { IFileService } from '../../../../../../../platform/files/common/files.js'; @@ -2101,8 +2101,10 @@ suite('PromptsService', () => { }, ]); - testConfigService.setUserConfiguration(PromptsConfig.INCLUDE_APPLYING_INSTRUCTIONS, true); - testConfigService.setUserConfiguration(PromptsConfig.USE_CUSTOMIZATIONS_IN_PARENT_REPOS, false); + + + await testConfigService.setUserConfiguration(PromptsConfig.INCLUDE_APPLYING_INSTRUCTIONS, true); + await testConfigService.setUserConfiguration(PromptsConfig.USE_CUSTOMIZATIONS_IN_PARENT_REPOS, false); // With parent search disabled, should not find parent files let promptFiles = await service.listPromptFiles(PromptsType.prompt, CancellationToken.None); @@ -2115,6 +2117,7 @@ suite('PromptsService', () => { // With parent search enabled, should find parent files testConfigService.setUserConfiguration(PromptsConfig.USE_CUSTOMIZATIONS_IN_PARENT_REPOS, true); + fireConfigChange(testConfigService, PromptsConfig.USE_CUSTOMIZATIONS_IN_PARENT_REPOS); promptFiles = await service.listPromptFiles(PromptsType.prompt, CancellationToken.None); agentFiles = await service.listPromptFiles(PromptsType.agent, CancellationToken.None); @@ -2181,6 +2184,8 @@ suite('PromptsService', () => { testConfigService.setUserConfiguration(PromptsConfig.INCLUDE_APPLYING_INSTRUCTIONS, true); testConfigService.setUserConfiguration(PromptsConfig.USE_CUSTOMIZATIONS_IN_PARENT_REPOS, true); + fireConfigChange(testConfigService, PromptsConfig.INCLUDE_APPLYING_INSTRUCTIONS, PromptsConfig.USE_CUSTOMIZATIONS_IN_PARENT_REPOS); + // Mark the parent repo root as untrusted workspaceTrustService.getUriTrustInfo = (uri: URI) => { @@ -3992,3 +3997,9 @@ suite('PromptsService', () => { }); }); }); + +function fireConfigChange(configService: TestConfigurationService, ...key: string[]): void { + configService.onDidChangeConfigurationEmitter.fire({ + affectsConfiguration: (k: string) => key.includes(k), + } satisfies Partial as unknown as IConfigurationChangeEvent); +}