feat(ai-core): add AiConfigurationService for AI preferences - #319
feat(ai-core): add AiConfigurationService for AI preferences#319AlexandraBuzila wants to merge 4 commits into
Conversation
Introduces a framework API wrapping PreferenceService for `ai-features.*` preferences, as the intended extension point for reading/writing AI configuration.
ndoschek
left a comment
ndoschek
left a comment
There was a problem hiding this comment.
Thanks a lot @AlexandraBuzila, code looks great to me, I just added a few minor inline comments. Could you have a quick look? TIA!
| const newPatterns = validated.filter(p => !currentPatterns.includes(p)); | ||
| if (newPatterns.length > 0) { | ||
| this.preferenceService.updateValue( | ||
| this.aiConfigurationService.update( |
There was a problem hiding this comment.
This write is fire-and-forget, so a rejected update() is silently swallowed and can surface as an unhandled rejection. Returning the promise from addPatternsToList/removePatternFromList (and the public add*/remove* methods) would let callers observe write failures. Same applies to the update() call in removePatternFromList below.
| protected readonly onDidChangeEmitter = new Emitter<AiConfigurationChange>(); | ||
| readonly onDidChange: Event<AiConfigurationChange> = this.onDidChangeEmitter.event; | ||
|
|
||
| protected _ready: Promise<void>; |
There was a problem hiding this comment.
_ready stays undefined until init() runs, so ready can be read before it is assigned. The sibling TrustAwarePreferenceReader avoids this by creating a Deferred eagerly; doing the same here keeps the two consistent and robust against ordering surprises.
| this.trustAwareReader.onDidChangeTrust(() => this.onDidChangeEmitter.fire({ | ||
| preferenceName: undefined, | ||
| affects: () => true, | ||
| affectsPreference: () => true |
There was a problem hiding this comment.
On a trust transition this sentinel returns true for every key, including non-ai-features.* ones, so a consumer that checks an unrelated key like editor.fontSize gets a false positive. The interface doc says a trust transition affects every trust-gated key, so gating this on the ai-features. prefix would match the documented semantics.
|
|
||
| bind(TrustAwarePreferenceReader).toSelf().inSingletonScope(); | ||
|
|
||
| bind(AiConfigurationServiceImpl).toSelf().inSingletonScope(); |
There was a problem hiding this comment.
TrustAwarePreferenceReader is still bound and still injected directly in frontend-language-model-service.ts and ai-ide/.../workspace-functions.ts. So after this PR there are two services that do the same trust-aware ai-features.* read. Since AiConfigurationService is meant to be the place to do this, it would be cleaner to move those two consumers over too and make TrustAwarePreferenceReader internal. Otherwise the same logic lives in two spots (TrustAwarePreferenceReader.get and AiConfigurationServiceImpl.enrichInspection) and they can easily fall out of sync.
…ervice Move FrontendLanguageModelService and WorkspaceFunctionScope off TrustAwarePreferenceReader onto AiConfigurationService, make the reader internal, and centralize the trust-suppression rule in one place.
|
Thank you, Nina! I addressed your comments in the last commits and also made the TrustAwarePreferenceReader internal to the new configuration service, since it was used only by the AI features for now. |
|
Closing in favor of eclipse-theia#17768 |
2 participants
What it does
Introduces a framework API wrapping PreferenceService for
ai-features.*preferences, as the intended extension point for reading/writing AI configuration.Closes #310
How to test
Follow-ups
Breaking changes
Attribution
Review checklist
nlsservice (for details, please see the Internationalization/Localization section in the Coding Guidelines)Reminder for reviewers