-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Initialize AIActivationService from preferences #15044
base: master
Are you sure you want to change the base?
Initialize AIActivationService from preferences #15044
Conversation
In our Theia-derived IDE, the AI feature sometimes is not enabled properly, even though the setting is enabled. We traced this down to the AIActivationServer depending on a signal from the PreferenceService. Fixes eclipse-theia#15043 Signed-off-by: Florian Richter <[email protected]>
Signed-off-by: Florian Richter <[email protected]>
@@ -44,7 +44,8 @@ export class AIActivationService implements FrontendApplicationContribution { | |||
return this.isAiEnabledKey.get() ?? false; | |||
} | |||
|
|||
initialize(): MaybePromise<void> { | |||
async initialize(): Promise<void> { | |||
await this.preferenceService.ready; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sdirix Is this alright from a performance perspective, or should we rather use preferenceService.ready.then(() => ...)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, we should never await something which does not need to be awaited in the initialize
startup calls. Therefore I would prefer the ready.then
variant. I also don't see a harm for this case. Good catch!
Is there an explanation why the original code fails for the reported Theia based application but does not fail for the Theia IDE? From my understanding the existing code should work as for every non-default preference a change event should be sent when the preferences are loaded. This feels like fixing a symptom while missing some underlying root cause. Edit: Is it possibly a race condition between the preference service loading and the startup of the ai package, i.e. there is no guarantee that we catch all events? |
What it does
In our Theia-derived IDE, the AI feature sometimes is not enabled properly, even though the setting is enabled. We traced this down to the AIActivationServer depending on a signal from the PreferenceService.
Fixes #15043
How to test
We could not reproduce this with TheiaIDE, only with our Theia-derived IDE on Windows/electron. If you think, the proposed change should not be necessary, I will try to reduce our IDE to a minimal example.
Breaking changes
Attribution
Contributed by MVTec Software GmbH
Review checklist
Reminder for reviewers