Conversation
📝 WalkthroughWalkthroughThis PR introduces Pure.DI-based dependency injection composition classes for Core, Cloud, Linux, Mac, and Windows modules, replacing the previous ChangesCentral package management setup
Pure.DI composition migration
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant ProgramMain as Program.Main/MainAsync
participant CreateServiceProvider
participant CoreComposition
participant CloudComposition
participant PlatformComposition as Platform Composition (Linux/Mac/Windows)
participant ServiceLocator
ProgramMain->>CreateServiceProvider: invoke
CreateServiceProvider->>CoreComposition: CreateBuilder(services)
CreateServiceProvider->>CloudComposition: CreateBuilder(services)
CreateServiceProvider->>PlatformComposition: CreateBuilder(services)
CreateServiceProvider->>CreateServiceProvider: ApplicationServiceCollection.Configure/ConfigureAliases
CreateServiceProvider->>CreateServiceProvider: services.BuildServiceProvider()
CreateServiceProvider->>CoreComposition: assign built ServiceProvider
CreateServiceProvider->>CloudComposition: assign built ServiceProvider
CreateServiceProvider->>PlatformComposition: assign built ServiceProvider
CreateServiceProvider-->>ProgramMain: return ServiceProvider
ProgramMain->>ServiceLocator: SetProvider(serviceProvider)
ProgramMain->>ProgramMain: BuildAvaloniaApp(serviceProvider)
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/Everywhere.Core/Common/ServiceLocator.cs (1)
15-21: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify
Resolve: thekey == nullcheck and the trailing throw are unreachable dead code.After line 18 (
if (key != null) throw ...),keyis guaranteed to benull, so theif (key == null)guard is always true and the finalthrowon line 20 can never execute.♻️ Proposed simplification
public static object Resolve(Type type, object? key = null) { if (_serviceProvider == null) throw new InvalidOperationException($"{nameof(ServiceLocator)} is not built."); if (key != null) throw new NotSupportedException("Keyed service resolution is not supported by the source-generated provider."); - if (key == null) return _serviceProvider.GetRequiredService(type); - throw new InvalidOperationException("Unreachable service resolution branch."); + return _serviceProvider.GetRequiredService(type); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Everywhere.Core/Common/ServiceLocator.cs` around lines 15 - 21, Simplify ServiceLocator.Resolve by removing the unreachable null check and dead trailing throw: after the key validation branch, key is guaranteed to be null, so return the required service from _serviceProvider directly. Keep the existing _serviceProvider null guard and the NotSupportedException for keyed resolution, but eliminate the redundant if (key == null) branch and the final “Unreachable service resolution branch” exception.src/Everywhere.Core/DependencyInjection/CoreComposition.cs (1)
29-29: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
#pragma warning disable CA1416scope is broader than necessary.The disable covers the entire class, but only the
#if WINDOWS-guardedRestartAsAdministratorControlroot (Line 63) needs it. Consider scoping the pragma to just that line/root, or re-enabling (#pragma warning restore CA1416) after it, to avoid silently suppressing platform-compatibility diagnostics for the rest of the file.Also applies to: 38-38, 58-58
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Everywhere.Core/DependencyInjection/CoreComposition.cs` at line 29, The CA1416 suppression in CoreComposition is too broad and is masking platform-compatibility warnings beyond the Windows-only root. Move the `#pragma warning disable CA1416` so it only surrounds the `RestartAsAdministratorControl` registration guarded by `#if WINDOWS`, and add `#pragma warning restore CA1416` immediately after that root (or otherwise narrowly scope it) so the rest of the `CoreComposition` class keeps normal diagnostics.src/Everywhere.Cloud/DependencyInjection/CloudComposition.cs (1)
12-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated
Hint()boilerplate across every composition class.This exact
.Hint(...)chain (lines 14-24) is repeated verbatim inLinuxComposition(and presumablyCoreComposition/MacComposition/WindowsComposition). Pure.DI supports.DependsOn(Base)to share a base setup across compositions — extracting this into a common baseDI.Setup(...)shared viaDependsOnwould remove the duplication and keep the hint list in sync across all five modules going forward.♻️ Sketch of shared base setup
// e.g. in a shared static class private static void SetupBase() => DI.Setup("Base") .Hint(Hint.OnCannotResolve, "On") .Hint(Hint.OnCannotResolvePartial, "Off") .Hint(Hint.OnNewRoot, "On") .Hint(Hint.OnNewRootPartial, "Off") .Hint(Hint.OnCannotResolveContractTypeNameWildcard, "Microsoft.Extensions.*") .Hint(Hint.OnCannotResolveContractTypeNameWildcard, "Everywhere.*"); // in CloudComposition private static void SetupCloudServices() => DI.Setup().DependsOn("Base") .Bind<OAuthCloudClient>()...🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Everywhere.Cloud/DependencyInjection/CloudComposition.cs` around lines 12 - 24, The `SetupCloudServices` method in `CloudComposition` repeats the same `Hint(...)` boilerplate used by other composition classes; extract the shared Pure.DI setup into a common base configuration and reuse it via `DependsOn` so the hint chain is defined once. Update `DI.Setup()` usage in `SetupCloudServices` to depend on the shared base setup, and move the duplicated `Hint` list into a shared helper/static setup used by `LinuxComposition`, `CoreComposition`, `MacComposition`, and `WindowsComposition` as well.src/Everywhere.Mac/Program.cs (1)
34-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffOptional: the composition build/assign sequence is duplicated across platform
Program.csfiles.The create-compositions →
CreateBuilder→ build → assignServiceProvidersequence is repeated verbatim in Mac/Windows (and Linux). Forgetting to assignServiceProvideron any composition would surface only as a runtime failure. Consider a small shared helper that takes the platform composition and centralizes the build + back-assignment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Everywhere.Mac/Program.cs` around lines 34 - 56, The composition setup in CreateServiceProvider is duplicated across platform Program.cs files and can be centralized to avoid missing the ServiceProvider back-assignment. Extract the create-compositions → CreateBuilder → BuildServiceProvider → assign ServiceProvider flow into a small shared helper, then have CoreComposition, CloudComposition, and MacComposition use that helper so the build and assignment happen consistently in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Everywhere.Core/DependencyInjection/AvaloniaServices.cs`:
- Around line 24-38: The SettingsPage registration in AvaloniaServices is
inconsistent with the other IMainViewNavigationItem page bindings because it is
missing As(Singleton). Update the SettingsPage binding to match the other
navigation item pages so the same instance is used both in the navigation
collection and when resolved as a root view, and verify the binding chain around
Bind<SettingsPage>() stays consistent with the other page registrations.
In `@src/Everywhere.Core/DependencyInjection/CoreComposition.cs`:
- Line 120: `SettingsPage` is bound inconsistently between direct root export
and the nav-item collection, which can create different instances depending on
how it is resolved. Update the `SettingsPage` registration in `AvaloniaServices`
to match the other navigation pages by using the same singleton lifetime as
`HomePage`, `CustomAssistantPage`, `PromptPage`, `ChatPluginPage`, `SkillPage`,
and `WebSearchEnginePage`, or add a clear comment in
`CoreComposition`/`AvaloniaServices` if the separate lifetime is intentional.
Reference the `Root<SettingsPage>` export and the
`Root<IEnumerable<IMainViewNavigationItem>>` aggregation when verifying the fix.
---
Nitpick comments:
In `@src/Everywhere.Cloud/DependencyInjection/CloudComposition.cs`:
- Around line 12-24: The `SetupCloudServices` method in `CloudComposition`
repeats the same `Hint(...)` boilerplate used by other composition classes;
extract the shared Pure.DI setup into a common base configuration and reuse it
via `DependsOn` so the hint chain is defined once. Update `DI.Setup()` usage in
`SetupCloudServices` to depend on the shared base setup, and move the duplicated
`Hint` list into a shared helper/static setup used by `LinuxComposition`,
`CoreComposition`, `MacComposition`, and `WindowsComposition` as well.
In `@src/Everywhere.Core/Common/ServiceLocator.cs`:
- Around line 15-21: Simplify ServiceLocator.Resolve by removing the unreachable
null check and dead trailing throw: after the key validation branch, key is
guaranteed to be null, so return the required service from _serviceProvider
directly. Keep the existing _serviceProvider null guard and the
NotSupportedException for keyed resolution, but eliminate the redundant if (key
== null) branch and the final “Unreachable service resolution branch” exception.
In `@src/Everywhere.Core/DependencyInjection/CoreComposition.cs`:
- Line 29: The CA1416 suppression in CoreComposition is too broad and is masking
platform-compatibility warnings beyond the Windows-only root. Move the `#pragma
warning disable CA1416` so it only surrounds the `RestartAsAdministratorControl`
registration guarded by `#if WINDOWS`, and add `#pragma warning restore CA1416`
immediately after that root (or otherwise narrowly scope it) so the rest of the
`CoreComposition` class keeps normal diagnostics.
In `@src/Everywhere.Mac/Program.cs`:
- Around line 34-56: The composition setup in CreateServiceProvider is
duplicated across platform Program.cs files and can be centralized to avoid
missing the ServiceProvider back-assignment. Extract the create-compositions →
CreateBuilder → BuildServiceProvider → assign ServiceProvider flow into a small
shared helper, then have CoreComposition, CloudComposition, and MacComposition
use that helper so the build and assignment happen consistently in one place.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ed9743f0-f67c-4a13-a5eb-869f454ab4fa
📒 Files selected for processing (36)
3rd/Directory.Packages.propsDirectory.Packages.propsEverywhere.slnxpatches/Directory.Packages.propssrc/Build.Pure.DI.MS.targetssrc/Directory.Packages.propssrc/Everywhere.Cloud/DependencyInjection/CloudComposition.cssrc/Everywhere.Cloud/DependencyInjection/CloudServiceCollection.cssrc/Everywhere.Cloud/Everywhere.Cloud.csprojsrc/Everywhere.Core/App.axaml.cssrc/Everywhere.Core/Chat/Plugins/Mcp/McpServiceExtension.cssrc/Everywhere.Core/Common/ServiceLocator.cssrc/Everywhere.Core/DependencyInjection/ApplicationServiceCollection.cssrc/Everywhere.Core/DependencyInjection/ApplicationServiceProviderFactories.cssrc/Everywhere.Core/DependencyInjection/AvaloniaServices.cssrc/Everywhere.Core/DependencyInjection/ChatPluginServices.cssrc/Everywhere.Core/DependencyInjection/ChatServices.cssrc/Everywhere.Core/DependencyInjection/CoreComposition.cssrc/Everywhere.Core/DependencyInjection/ExternalBoundaryServices.cssrc/Everywhere.Core/DependencyInjection/InteropServices.cssrc/Everywhere.Core/DependencyInjection/NetworkServices.cssrc/Everywhere.Core/DependencyInjection/SettingsServices.cssrc/Everywhere.Core/DependencyInjection/StorageServices.cssrc/Everywhere.Core/DependencyInjection/StrategyEngineServices.cssrc/Everywhere.Core/Everywhere.Core.csprojsrc/Everywhere.Linux/DependencyInjection/LinuxComposition.cssrc/Everywhere.Linux/Everywhere.Linux.csprojsrc/Everywhere.Linux/Interop/X11WindowBackend.cssrc/Everywhere.Linux/Program.cssrc/Everywhere.Mac/DependencyInjection/MacComposition.cssrc/Everywhere.Mac/Everywhere.Mac.csprojsrc/Everywhere.Mac/Program.cssrc/Everywhere.Windows/DependencyInjection/WindowsComposition.cssrc/Everywhere.Windows/Everywhere.Windows.csprojsrc/Everywhere.Windows/Program.cstests/Directory.Packages.props
💤 Files with no reviewable changes (1)
- Directory.Packages.props
Comment on lines
+24
to
+38
| .Bind<HomePageViewModel>().As(Singleton).To<HomePageViewModel>() | ||
| .Bind<HomePage>().Bind<IMainViewNavigationItem>(Tag.Unique).As(Singleton).To<HomePage>() | ||
| .Bind<CustomAssistantPageViewModel>().As(Singleton).To<CustomAssistantPageViewModel>() | ||
| .Bind<CustomAssistantPage>().Bind<IMainViewNavigationItem>(Tag.Unique).As(Singleton).To<CustomAssistantPage>() | ||
| .Bind<PromptPageViewModel>().As(Singleton).To<PromptPageViewModel>() | ||
| .Bind<PromptPage>().Bind<IMainViewNavigationItem>(Tag.Unique).As(Singleton).To<PromptPage>() | ||
| .Bind<PromptEditorViewModel>().To<PromptEditorViewModel>() | ||
| .Bind<PromptEditorPage>().To<PromptEditorPage>() | ||
| .Bind<ChatPluginPageViewModel>().As(Singleton).To<ChatPluginPageViewModel>() | ||
| .Bind<ChatPluginPage>().Bind<IMainViewNavigationItem>(Tag.Unique).As(Singleton).To<ChatPluginPage>() | ||
| .Bind<SkillPageViewModel>().As(Singleton).To<SkillPageViewModel>() | ||
| .Bind<SkillPage>().Bind<IMainViewNavigationItem>(Tag.Unique).As(Singleton).To<SkillPage>() | ||
| .Bind<WebSearchEnginePageViewModel>().As(Singleton).To<WebSearchEnginePageViewModel>() | ||
| .Bind<WebSearchEnginePage>().Bind<IMainViewNavigationItem>(Tag.Unique).As(Singleton).To<WebSearchEnginePage>() | ||
| .Bind<SettingsPage>().Bind<IMainViewNavigationItem>(Tag.Unique).To<SettingsPage>() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
SettingsPage missing .As(Singleton).
Every other nav-item page binding in this block uses .As(Singleton) (Lines 24-37), but SettingsPage (Line 38) is bound as transient. Since SettingsPage is also exported as a standalone root in CoreComposition.cs (Line 120), this could yield a different instance than the one appearing in the IMainViewNavigationItem collection.
🔧 Proposed fix
- .Bind<SettingsPage>().Bind<IMainViewNavigationItem>(Tag.Unique).To<SettingsPage>()
+ .Bind<SettingsPage>().Bind<IMainViewNavigationItem>(Tag.Unique).As(Singleton).To<SettingsPage>()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| .Bind<HomePageViewModel>().As(Singleton).To<HomePageViewModel>() | |
| .Bind<HomePage>().Bind<IMainViewNavigationItem>(Tag.Unique).As(Singleton).To<HomePage>() | |
| .Bind<CustomAssistantPageViewModel>().As(Singleton).To<CustomAssistantPageViewModel>() | |
| .Bind<CustomAssistantPage>().Bind<IMainViewNavigationItem>(Tag.Unique).As(Singleton).To<CustomAssistantPage>() | |
| .Bind<PromptPageViewModel>().As(Singleton).To<PromptPageViewModel>() | |
| .Bind<PromptPage>().Bind<IMainViewNavigationItem>(Tag.Unique).As(Singleton).To<PromptPage>() | |
| .Bind<PromptEditorViewModel>().To<PromptEditorViewModel>() | |
| .Bind<PromptEditorPage>().To<PromptEditorPage>() | |
| .Bind<ChatPluginPageViewModel>().As(Singleton).To<ChatPluginPageViewModel>() | |
| .Bind<ChatPluginPage>().Bind<IMainViewNavigationItem>(Tag.Unique).As(Singleton).To<ChatPluginPage>() | |
| .Bind<SkillPageViewModel>().As(Singleton).To<SkillPageViewModel>() | |
| .Bind<SkillPage>().Bind<IMainViewNavigationItem>(Tag.Unique).As(Singleton).To<SkillPage>() | |
| .Bind<WebSearchEnginePageViewModel>().As(Singleton).To<WebSearchEnginePageViewModel>() | |
| .Bind<WebSearchEnginePage>().Bind<IMainViewNavigationItem>(Tag.Unique).As(Singleton).To<WebSearchEnginePage>() | |
| .Bind<SettingsPage>().Bind<IMainViewNavigationItem>(Tag.Unique).To<SettingsPage>() | |
| .Bind<HomePageViewModel>().As(Singleton).To<HomePageViewModel>() | |
| .Bind<HomePage>().Bind<IMainViewNavigationItem>(Tag.Unique).As(Singleton).To<HomePage>() | |
| .Bind<CustomAssistantPageViewModel>().As(Singleton).To<CustomAssistantPageViewModel>() | |
| .Bind<CustomAssistantPage>().Bind<IMainViewNavigationItem>(Tag.Unique).As(Singleton).To<CustomAssistantPage>() | |
| .Bind<PromptPageViewModel>().As(Singleton).To<PromptPageViewModel>() | |
| .Bind<PromptPage>().Bind<IMainViewNavigationItem>(Tag.Unique).As(Singleton).To<PromptPage>() | |
| .Bind<PromptEditorViewModel>().To<PromptEditorViewModel>() | |
| .Bind<PromptEditorPage>().To<PromptEditorPage>() | |
| .Bind<ChatPluginPageViewModel>().As(Singleton).To<ChatPluginPageViewModel>() | |
| .Bind<ChatPluginPage>().Bind<IMainViewNavigationItem>(Tag.Unique).As(Singleton).To<ChatPluginPage>() | |
| .Bind<SkillPageViewModel>().As(Singleton).To<SkillPageViewModel>() | |
| .Bind<SkillPage>().Bind<IMainViewNavigationItem>(Tag.Unique).As(Singleton).To<SkillPage>() | |
| .Bind<WebSearchEnginePageViewModel>().As(Singleton).To<WebSearchEnginePageViewModel>() | |
| .Bind<WebSearchEnginePage>().Bind<IMainViewNavigationItem>(Tag.Unique).As(Singleton).To<WebSearchEnginePage>() | |
| .Bind<SettingsPage>().Bind<IMainViewNavigationItem>(Tag.Unique).As(Singleton).To<SettingsPage>() |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Everywhere.Core/DependencyInjection/AvaloniaServices.cs` around lines 24
- 38, The SettingsPage registration in AvaloniaServices is inconsistent with the
other IMainViewNavigationItem page bindings because it is missing As(Singleton).
Update the SettingsPage binding to match the other navigation item pages so the
same instance is used both in the navigation collection and when resolved as a
root view, and verify the binding chain around Bind<SettingsPage>() stays
consistent with the other page registrations.
| .Root<SkillPage>(kind: ExportedRoot) | ||
| .Root<WebSearchEnginePageViewModel>(kind: ExportedRoot) | ||
| .Root<WebSearchEnginePage>(kind: ExportedRoot) | ||
| .Root<SettingsPage>(kind: ExportedRoot) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Verify SettingsPage lifetime consistency with other nav items.
Root<SettingsPage> is exported here, and SettingsPage is also aggregated into Root<IEnumerable<IMainViewNavigationItem>>. In AvaloniaServices.cs (Line 38), SettingsPage is the only nav-item binding missing .As(Singleton) — every sibling page (HomePage, CustomAssistantPage, PromptPage, ChatPluginPage, SkillPage, WebSearchEnginePage) is Singleton. If this is intentional it's worth a comment; otherwise resolving SettingsPage directly vs. through the nav-item collection will produce two different instances.
Also applies to: 158-158
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Everywhere.Core/DependencyInjection/CoreComposition.cs` at line 120,
`SettingsPage` is bound inconsistently between direct root export and the
nav-item collection, which can create different instances depending on how it is
resolved. Update the `SettingsPage` registration in `AvaloniaServices` to match
the other navigation pages by using the same singleton lifetime as `HomePage`,
`CustomAssistantPage`, `PromptPage`, `ChatPluginPage`, `SkillPage`, and
`WebSearchEnginePage`, or add a clear comment in
`CoreComposition`/`AvaloniaServices` if the separate lifetime is intentional.
Reference the `Root<SettingsPage>` export and the
`Root<IEnumerable<IMainViewNavigationItem>>` aggregation when verifying the fix.
Member
Author
|
Given the complexity of the DI in this project and its heavy reliance on MSDI, this PR will be put on hold. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR migrates reflection-based Microsoft.Extensions.DependencyInjection to hybrid Pure.DI. This makes DI more AOT compatible, compile-time diagnosable and (maybe) better performance.
Type of Change
Checklist
Summary by CodeRabbit
New Features
Bug Fixes