Add Blazor support to Consolonia. #164
Conversation
* added example.blazor project * added example.blazor.components project * bumped .net version to 8.0 due to AvaloniaBlazorBindings projectdependency needing it * cleaned up project package dependencies to prepare publishing package dependencies. * tied consolonia package version to avalonia major/minor version since we depend on undocumented interfaces.
|
Warning Rate limit exceeded@tomlm has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 38 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request includes several updates across multiple files primarily focused on upgrading the .NET version and enhancing the integration of the Consolonia framework with Blazor. The .NET version in the workflow files Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (12)
src/Example.Blazor/App.axaml.cs (1)
6-8: Consider adding XML documentation.Adding XML documentation would improve IntelliSense support and make the purpose of this class clearer to other developers.
+ /// <summary> + /// Main application class for the Blazor example project. + /// Serves as the entry point and hosts the MainPage component. + /// </summary> public class App : ConsoloniaBlazorApplication<MainPage> { }src/Example.Blazor/Program.cs (1)
11-11: Consider improving or removing the ReSharper comment.The current comment doesn't clearly explain why the parameter needs to be kept. Consider either removing it (since args is a standard parameter for Main) or providing a more descriptive explanation.
- // ReSharper disable once ParameterOnlyUsedForPreconditionCheck.Local Exactly why we are keeping it here + // args parameter is required for console lifetime management and command-line argument processingsrc/Example.Blazor.Components/SubPage.razor (1)
7-13: Consider simplifying the UI structureA few suggestions for the UI:
- The
ScrollViewermight be unnecessary for this simple content. Consider removing it unless you're expecting dynamic content growth.- The blue text color seems arbitrary. Consider using theme colors or documenting the purpose of this specific styling.
-<ScrollViewer> <StackPanel HorizontalAlignment="HorizontalAlignment.Center" VerticalAlignment="VerticalAlignment.Center"> <Button OnClick="Back">Back</Button> - <TextBlock Text="This a second page." Foreground="Brushes.Blue"></TextBlock> + <TextBlock Text="This a second page."></TextBlock> </StackPanel> -</ScrollViewer>Directory.Build.props (1)
11-11: Update the copyright yearThe copyright notice shows "2021 - 2022" but should be updated to include 2024.
- <Copyright>Copyright © Evgeny Gorbovoy 2021 - 2022</Copyright> + <Copyright>Copyright © Evgeny Gorbovoy 2021 - 2024</Copyright>src/Consolonia.Themes.TurboVision/Themes/Material/MaterialTheme.cs (1)
10-12: Eliminate URI duplicationThe same URI string is duplicated between the constructor and the
Uriproperty. Consider using a private constant.public class MaterialTheme : ResourceIncludeBase { + private const string ThemeUri = "avares://Consolonia.Themes.TurboVision/Themes/Material/Material.axaml"; - public MaterialTheme() : base(new Uri("avares://Consolonia.Themes.TurboVision/Themes/Material/Material.axaml")) + public MaterialTheme() : base(new Uri(ThemeUri)) { } protected override Uri Uri => - new("avares://Consolonia.Themes.TurboVision/Themes/Material/Material.axaml"); + new(ThemeUri);src/Consolonia.Blazor/ConsoloniaBlazorApplication.cs (1)
22-24: Consider using singleton for keyboard navigation handlerThe
ArrowsAndKeyboardNavigationHandleris registered with a transient lifetime, which means a new instance will be created each time it's requested. Since this handler typically maintains application-wide state and doesn't need to be recreated, consider using a singleton lifetime instead.- AvaloniaLocator.CurrentMutable.Bind<IKeyboardNavigationHandler>() - .ToTransient<ArrowsAndKeyboardNavigationHandler>(); + AvaloniaLocator.CurrentMutable.Bind<IKeyboardNavigationHandler>() + .ToSingleton<ArrowsAndKeyboardNavigationHandler>();src/Example.Blazor.Components/MainPage.razor (2)
42-44: Consider more flexible navigation approachThe navigation to
SubPageis hardcoded, which makes the component less reusable and harder to test. Consider passing the target page type as a parameter or using a route-based approach.+ [Parameter] + public Type? TargetPage { get; set; } = typeof(SubPage); + void OnGotoSubPage() - => navigation.PushAsync<SubPage>(); + => navigation.PushAsync(TargetPage ?? typeof(SubPage));
13-19: Add keyboard shortcuts for common actionsConsider adding keyboard shortcuts for frequently used actions to improve accessibility and user experience.
<StackPanel Orientation="Orientation.Vertical" HorizontalAlignment="HorizontalAlignment.Center" Margin="@Thickness.Parse("4")"> - <Button OnClick="@OnIncrement">Increment counter support</Button> - <Button OnClick="@OnToggleCounter">@ToggleText</Button> - <Button OnClick="@OnMessageBox">Message Box</Button> - <Button OnClick="@OnGotoSubPage">Go to SubPage</Button> - <Button OnClick="@OnExit">Exit</Button> + <Button OnClick="@OnIncrement" HotKey="Ctrl+I">Increment counter support (Ctrl+I)</Button> + <Button OnClick="@OnToggleCounter" HotKey="Ctrl+T">@ToggleText (Ctrl+T)</Button> + <Button OnClick="@OnMessageBox" HotKey="Ctrl+M">Message Box (Ctrl+M)</Button> + <Button OnClick="@OnGotoSubPage" HotKey="Ctrl+G">Go to SubPage (Ctrl+G)</Button> + <Button OnClick="@OnExit" HotKey="Alt+F4">Exit (Alt+F4)</Button> </StackPanel>src/Consolonia.Blazor/BuilderExtensions.cs (2)
12-18: Enhance XML documentationThe XML documentation is incomplete. Consider adding:
- Description for
configureServicesparameter- Return value documentation
- Example usage
/// <summary> -/// Use Consolonize in Blazor mode +/// Configures the application to use Consolonia in Blazor mode /// </summary> /// <param name="builder"></param> -/// <param name="configureServices"></param> +/// <param name="configureServices">Optional delegate to configure additional services</param> +/// <returns>The configured AppBuilder instance</returns> +/// <example> +/// <code> +/// var builder = AppBuilder.Configure<App>() +/// .UseConsoloniaBlazor(services => +/// { +/// services.AddSingleton<IMyService, MyService>(); +/// }); +/// </code> +/// </example>
35-38: Consider using C# 8.0 null-coalescing assignmentThe null check can be simplified using modern C# features.
-if (configureServices != null) -{ - configureServices(sc); -} +configureServices?.Invoke(sc);src/Consolonia.Core/Helpers/Extensions.cs (2)
44-44: Consider extracting magic numbers as constantsThe font size value (16.0) should be defined as a constant for better maintainability.
+private const double DefaultDesignModeFontSize = 16.0; + // In the style setter -new Setter(TemplatedControl.FontSizeProperty, 16.0), +new Setter(TemplatedControl.FontSizeProperty, DefaultDesignModeFontSize),
51-63: Remove commented experimental codeThe experimental code should be removed or moved to a separate branch/issue if it needs to be preserved for future reference.
- // EXPERIMENTAL - // If you do RenderTRansform="scale(10.0,10.0) you can actually sort of see the UI get bigger - // but this doesn' seem to work when using these style setters. <sigh> - //this.Styles.Add(new Style(x => x.Is<Visual>()) - //{ - // Setters = - // { - // //new Setter(Visual.RenderTransformOriginProperty, RelativePoint.TopLeft), - // //new Setter(Visual.RenderTransformProperty, new ScaleTransform(2.0, 2.0)), - // //new Setter(Visual.RenderTransformProperty, new ScaleTransform(2.0, 2.0)), - // } - //});
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (25)
.github/workflows/editorconfig.yml(1 hunks).github/workflows/general_build.yml(1 hunks)Directory.Build.props(1 hunks)src/Consolonia.Blazor/BuilderExtensions.cs(1 hunks)src/Consolonia.Blazor/Consolonia.Blazor.csproj(1 hunks)src/Consolonia.Blazor/ConsoloniaBlazorApplication.cs(1 hunks)src/Consolonia.Core/Helpers/Extensions.cs(2 hunks)src/Consolonia.Core/Infrastructure/ConsoloniaApplication.cs(2 hunks)src/Consolonia.Core/Infrastructure/ConsoloniaException.cs(0 hunks)src/Consolonia.Core/Infrastructure/ConsoloniaNotSupportedException.cs(0 hunks)src/Consolonia.Designer/Consolonia.Designer.csproj(1 hunks)src/Consolonia.GuiCS/binding.cs(2 hunks)src/Consolonia.NUnit/Consolonia.NUnit.csproj(1 hunks)src/Consolonia.PlatformSupport/Consolonia.PlatformSupport.csproj(1 hunks)src/Consolonia.Themes.TurboVision/Consolonia.Themes.TurboVision.csproj(1 hunks)src/Consolonia.Themes.TurboVision/Themes/Material/MaterialTheme.cs(1 hunks)src/Consolonia.sln(2 hunks)src/Example.Blazor.Components/Example.Blazor.Components.csproj(1 hunks)src/Example.Blazor.Components/MainPage.razor(1 hunks)src/Example.Blazor.Components/SubPage.razor(1 hunks)src/Example.Blazor.Components/_Imports.razor(1 hunks)src/Example.Blazor/App.axaml(1 hunks)src/Example.Blazor/App.axaml.cs(1 hunks)src/Example.Blazor/Example.Blazor.csproj(1 hunks)src/Example.Blazor/Program.cs(1 hunks)
💤 Files with no reviewable changes (2)
- src/Consolonia.Core/Infrastructure/ConsoloniaException.cs
- src/Consolonia.Core/Infrastructure/ConsoloniaNotSupportedException.cs
✅ Files skipped from review due to trivial changes (4)
- src/Example.Blazor.Components/Example.Blazor.Components.csproj
- src/Example.Blazor/Example.Blazor.csproj
- src/Example.Blazor/App.axaml
- src/Example.Blazor.Components/_Imports.razor
🧰 Additional context used
📓 Learnings (1)
src/Consolonia.Designer/Consolonia.Designer.csproj (1)
Learnt from: tomlm
PR: jinek/Consolonia#150
File: src/Consolonia.Designer/Consolonia.Designer.csproj:14-15
Timestamp: 2024-11-19T07:19:33.261Z
Learning: In the Consolonia project, circular dependencies between `Consolonia.Designer` and `Consolonia.PreviewHost` are intentional and by design. The `ConsolePreview` control consumes `Consolonia.PreviewHost`, and `Avalonia.PreviewHost` consumes `ConsolePreview` control in the designer.
🔇 Additional comments (18)
src/Example.Blazor/App.axaml.cs (1)
1-9: LGTM! Clean and focused implementation.
The class follows good practices by inheriting from ConsoloniaBlazorApplication<MainPage> and maintains a minimal, focused implementation that serves as the application entry point.
src/Example.Blazor/Program.cs (3)
1-6: LGTM! Imports are well-organized and necessary.
The imports provide all required dependencies for Avalonia UI framework and Consolonia's Blazor integration.
9-17: LGTM! Proper setup for a Consolonia Blazor application.
The Main method correctly implements the standard Avalonia application startup pattern with console lifetime management. The [STAThread] attribute is appropriately used for Windows compatibility.
19-24: Verify exception logging configuration for production use.
The LogToException() call might expose sensitive information in production environments. Consider making this configurable based on the environment.
Consider implementing environment-specific logging configuration:
#if DEBUG
.LogToException()
#else
.LogToCustomProvider() // Production logging configuration
#endifsrc/Consolonia.NUnit/Consolonia.NUnit.csproj (1)
10-10:
Remove circular dependency between package and project references
Same issue as in Consolonia.Themes.TurboVision.csproj - circular dependency between package and project references.
src/Example.Blazor.Components/SubPage.razor (1)
16-19: LGTM! Navigation implementation looks good
The Back method correctly implements async/await pattern and uses the injected navigation service appropriately.
src/Consolonia.Core/Infrastructure/ConsoloniaApplication.cs (2)
3-3: LGTM: Clean dependency management
Good job removing unnecessary Avalonia-specific imports and adding only the required helper namespace.
21-21: Verify the AddConsoloniaDesignMode implementation
The design mode initialization has been nicely encapsulated into an extension method. However, we should verify its implementation to ensure it handles all the previous functionality.
✅ Verification successful
Design mode implementation is properly handled
The AddConsoloniaDesignMode implementation in src/Consolonia.Core/Helpers/Extensions.cs correctly handles the design mode functionality by:
- Setting up default theme colors (foreground/background) for Visual Studio designer preview
- Using theme-aware brushes when available through resource lookup
- Working in conjunction with other design mode checks in the codebase:
- Using a dummy console in platform support
- Configuring appropriate platform detection in designer mode
- Handling preview buffer rendering
The implementation maintains all necessary design mode functionality while being properly encapsulated in an extension method.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the implementation of AddConsoloniaDesignMode
ast-grep --pattern 'public static void AddConsoloniaDesignMode(this Application $_) {
$$$
}'
# Look for any design mode related code that might have been missed
rg -l "Design.IsDesignMode"
Length of output: 306
Script:
#!/bin/bash
# Let's check the implementation in PlatformSupportExtensions.cs
ast-grep --pattern 'AddConsoloniaDesignMode' src/Consolonia.PlatformSupport/PlatformSupportExtensions.cs
# Check design mode related code in all files
rg "Design.IsDesignMode" -A 5 src/Consolonia.PlatformSupport/PlatformSupportExtensions.cs
rg "Design.IsDesignMode" -A 5 src/Consolonia.Core/Helpers/Extensions.cs
rg "Design.IsDesignMode" -A 5 src/Consolonia.Designer/Extensions.cs
rg "Design.IsDesignMode" -A 5 src/Consolonia.Designer/ConsolePreview.cs
Length of output: 1668
Script:
#!/bin/bash
# Let's check the actual implementation of AddConsoloniaDesignMode method
rg -A 10 "public static.*AddConsoloniaDesignMode" src/Consolonia.PlatformSupport/PlatformSupportExtensions.cs
# Also check if there are any other design mode related methods that might be relevant
ast-grep --pattern 'public static $_ $_DesignMode($_) {
$$$
}'
Length of output: 178
Script:
#!/bin/bash
# Let's try a different approach to find the AddConsoloniaDesignMode implementation
rg -A 15 "AddConsoloniaDesignMode" --type cs
# Also check for any design mode setup methods
rg -A 10 "DesignMode" --type cs
Length of output: 8216
Directory.Build.props (1)
4-4: Verify .NET 8 compatibility across the solution
The upgrade from .NET 6 to .NET 8 is a major change. Ensure all dependencies and features are compatible with .NET 8.
✅ Verification successful
No .NET 6 references found, upgrade to .NET 8 appears clean
The verification shows that:
- No files contain references to
net6.0ornetcoreapp6.0 - All project files consistently use .NET 8 target framework
- All dependencies appear to be compatible with .NET 8:
- Avalonia packages are using version 11.2.1 (via AvaloniaVersion property)
- Consolonia packages are at version 11.2
- Other packages (CommunityToolkit.Mvvm, Newtonsoft.Json) are at recent versions known to work with .NET 8
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any .NET version references that might need updating
rg -l "net6\.0|netcoreapp6\.0"
# Look for potential compatibility issues in project files
fd -e csproj -x cat {} \; | grep -i "TargetFramework\|RuntimeIdentifier\|Version"
Length of output: 4945
src/Consolonia.Blazor/Consolonia.Blazor.csproj (2)
12-12: Verify the stability of BlazorBindingsAvalonia version 0.1.2
The version 0.1.2 appears to be an early release. This could pose stability risks in a production environment.
Consider:
- Documenting any known limitations or issues
- Adding fallback mechanisms for potential instability
- Setting up monitoring for Blazor-related issues
✅ Verification successful
BlazorBindingsAvalonia 0.1.2 is appropriately integrated and actively used
The package is properly integrated into the project with consistent usage patterns:
- Used in Example.Blazor.Components for UI components
- Integrated in ConsoloniaBlazorApplication.cs and BuilderExtensions.cs
- Released recently (November 2024) which aligns with the project's timeline
- The version is consistent with the project's "proof of concept" state mentioned in README.md
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check package info and release history
curl -s https://api.nuget.org/v3/registration5-semver1/blazorbindingsavalonia/index.json | jq '.items[0].items[].catalogEntry | {version, published}'
# Check GitHub issues/discussions for known stability issues
gh api graphql -f query='
{
repository(owner: "jinek", name: "Consolonia") {
discussions(first: 10, categoryId: "DIC_kwDOGM5jE84COYYl") {
nodes {
title
body
}
}
}
}'
Length of output: 728
Script:
#!/bin/bash
# Let's check for any direct dependencies on BlazorBindingsAvalonia in the codebase
rg -l "BlazorBindingsAvalonia" --type-add 'proj:*.{csproj,props,targets}' -t proj
# Check if there are any usage patterns or configurations
rg "BlazorBindingsAvalonia" -A 3 -B 3
# Look for any Blazor-related configurations or setups
ast-grep --pattern 'services.AddBlazor$$$'
# Check for any documentation about Blazor integration
fd README.md --exec cat {}
Length of output: 3142
10-13: Verify version compatibility between Avalonia and BlazorBindingsAvalonia
Ensure that the Avalonia version (referenced via variable) is compatible with BlazorBindingsAvalonia 0.1.2.
src/Consolonia.Themes.TurboVision/Themes/Material/MaterialTheme.cs (1)
10-12: Verify theme resource loading in Blazor context
The new parameterless constructor suggests Blazor integration. Ensure the theme resources load correctly in a Blazor environment.
✅ Verification successful
Let me gather more information about the theme handling and Blazor integration.
Let me check one more aspect about theme loading and resource handling.
Theme resource loading appears standard and compatible with both Blazor and non-Blazor contexts
The parameterless constructor follows the standard Avalonia theme initialization pattern using avares:// URI scheme, which is compatible across different hosting scenarios including Blazor. The Material.axaml file exists at the correct location and follows the standard Avalonia styling structure with proper resource inclusions. No Blazor-specific theme handling is required as the resource loading mechanism is uniform across platforms.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the theme AXAML file exists and is accessible
fd -t f "Material.axaml$"
# Look for any Blazor-specific theme handling
ast-grep --pattern 'class $_ : ResourceIncludeBase {
$$$
public $_() {
$$$
}
}'
Length of output: 184
Script:
#!/bin/bash
# Check for Blazor-specific configurations or theme handling
rg -l "blazor|Blazor" --type cs
# Check the content of Material.axaml to understand resource structure
cat src/Consolonia.Themes.TurboVision/Themes/Material/Material.axaml
# Look for other theme implementations for comparison
ast-grep --pattern 'class $_ : Theme {
$$$
}'
Length of output: 913
Script:
#!/bin/bash
# Check how themes are loaded in Blazor context
rg -A 5 "ConsoloniaBlazorApplication" src/Consolonia.Blazor/ConsoloniaBlazorApplication.cs
# Check if there are any special theme loading mechanisms
rg -l "Uri.*avares://" --type cs
# Look for theme registration or initialization
ast-grep --pattern 'UseTheme<$_>($$$)'
Length of output: 563
.github/workflows/general_build.yml (1)
25-25: Verify comprehensive .NET 8 upgrade across the solution
The workflow now uses .NET 8, which requires verification of:
- All project files targeting .NET 8
- Package compatibility with .NET 8
- Breaking changes impact
Consider:
- Documenting any breaking changes
- Adding migration guides if needed
- Updating development environment requirements
✅ Verification successful
The previous results show that there are multiple project files but we need to verify their target frameworks. Let's check if all projects are properly configured for .NET 8 and if there are any legacy framework references remaining.
✅ .NET 8 upgrade appears to be properly implemented
The verification shows that:
- All detected project files are targeting .NET 8.0
- No references to older .NET versions (6.0 or 7.0) were found
- No compatibility issues or obsolete API usage were detected
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check target framework in all project files
fd -e csproj | xargs grep -l "TargetFramework"
# Look for potential compatibility issues in package references
fd -e csproj | xargs grep -l "PackageReference"
# Check for obsolete API usage
ast-grep --pattern '[Obsolete]
$$$'
Length of output: 988
Script:
#!/bin/bash
# Check target framework versions in project files
fd -e csproj | xargs cat | grep -A 1 "TargetFramework"
# Check .NET SDK version in global.json if exists
fd global.json | xargs cat
# Look for any direct references to net6.0 or net7.0
rg "net[67]\.0" -A 2 -B 2
Length of output: 298
src/Consolonia.Blazor/ConsoloniaBlazorApplication.cs (1)
11-32: LGTM! Well-structured Blazor integration
The implementation provides a clean base class for Blazor integration with proper inheritance, service registration, and framework initialization. The XML documentation clearly explains the purpose of the class.
.github/workflows/editorconfig.yml (1)
30-30: Verify tool compatibility with .NET 8.0
The upgrade to .NET 8.0 looks good. However, ensure that all tools in the workflow, particularly the ReSharper CLI tools, are compatible with .NET 8.0.
✅ Verification successful
Tools are compatible with .NET 8.0
Based on the documentation and workflow files, the tools being used are fully compatible with .NET 8.0:
- The ReSharper CLI action (muno92/[email protected]) explicitly lists .NET 8.0.x as a supported version in its documentation
- The workflow is using ReSharper version 2024.2.6, which is a recent version that supports .NET 8
- The JetBrains.ReSharper.GlobalTools package being installed is compatible with all supported .NET versions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if workflow tools support .NET 8
# Check if ReSharper version supports .NET 8
gh api graphql -f query='
{
repository(owner: "JetBrains", name: "resharper-dotnet") {
releases(first: 1, orderBy: {field: CREATED_AT, direction: DESC}) {
nodes {
name
description
}
}
}
}'
Length of output: 520
Script:
#!/bin/bash
# Let's check the workflow file to see what ReSharper tools are being used
cat .github/workflows/editorconfig.yml
# Also check if there are any other .NET-related workflows that might provide context
fd -e yml -e yaml . .github/workflows --exec cat {}
Length of output: 5646
Script:
#!/bin/bash
# Let's check the specific ReSharper tool version being used
curl -s https://api.github.com/repos/muno92/resharper_inspectcode/releases/tags/1.12.0 | jq -r '.body'
# Also check the marketplace page for this action
curl -s https://raw.githubusercontent.com/muno92/resharper_inspectcode/1.12.0/README.md
Length of output: 4689
src/Consolonia.sln (2)
64-68: LGTM: New Blazor projects follow a clean architecture.
The addition of three new projects demonstrates a well-structured approach:
Consolonia.Blazor: Core library for Blazor integrationExample.Blazor: Demo applicationExample.Blazor.Components: Reusable components library
62-63: Verify SDK-style project upgrade for Consolonia.Designer.
The project type GUID has been changed from FAE04EC0-301F-11D3-BF4B-00C04F79EFBC to 9A19103F-16F7-4668-BE54-9A1E7A4F7556, indicating an upgrade to the SDK-style project format.
Run the following script to verify the project file format:
✅ Verification successful
Project successfully upgraded to SDK-style format
The verification confirms that Consolonia.Designer.csproj is using the modern SDK-style project format with <Project Sdk="Microsoft.NET.Sdk"> declaration, which aligns with the project type GUID change in the solution file.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that Consolonia.Designer.csproj uses the SDK-style format
# Check if the project file uses SDK-style format
cat src/Consolonia.Designer/Consolonia.Designer.csproj | grep -E '<Project Sdk="Microsoft\.NET\.Sdk'
Length of output: 136
src/Consolonia.GuiCS/binding.cs (1)
332-333: LGTM: Improved delegate declaration formatting.
The formatting changes to endwin and isendwin delegate declarations improve code readability while maintaining functionality.
src/Consolonia.Themes.TurboVision/Consolonia.Themes.TurboVision.csproj
Outdated
Show resolved
Hide resolved
src/Consolonia.PlatformSupport/Consolonia.PlatformSupport.csproj
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/Consolonia.Blazor/BuilderExtensions.cs (3)
12-18: Enhance XML documentation for better developer experienceThe current documentation could be more comprehensive. Consider adding:
- Detailed description of the method's purpose and behavior
- Parameter descriptions explaining the role of
configureServices- Return value documentation
- Example usage
/// <summary> -/// Use Consolonize in Blazor mode +/// Configures the application to use Consolonia with Blazor integration. +/// This sets up necessary services and bindings for Blazor functionality. /// </summary> /// <param name="builder"></param> /// <param name="configureServices"></param> +/// <param name="builder">The application builder to configure</param> +/// <param name="configureServices">Optional callback to configure additional services in the DI container</param> /// <returns></returns> +/// <returns>The configured application builder</returns> +/// <example> +/// <code> +/// var builder = AppBuilder.Configure<App>() +/// .UseConsoloniaBlazor(services => +/// { +/// services.AddSingleton<IMyService, MyService>(); +/// }); +/// </code> +/// </example>
25-30: Consider documenting lifetime requirementsThe lifetime registration assumes the application lifetime will be available, but this might not always be true during the application startup sequence.
Consider:
- Documenting when this method should be called in the application lifecycle
- Adding validation for the Application.Current state
- Including more descriptive error messages in the ArgumentNullException
31-38: Review service registration lifetimesAll window-related services are registered as transient, which might not be optimal for all cases.
Consider:
- Using singleton for services that don't change during the application lifetime (e.g., PlatformSettings)
- Using scoped registration for services that should be shared within a context
- Documenting the rationale for choosing transient lifetime for these services
🛑 Comments failed to post (0)
Co-authored-by: <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
src/Consolonia.Blazor/BuilderExtensions.cs (4)
12-17: Enhance XML documentation for better developer experience.The current documentation could be more comprehensive. Consider adding:
- Detailed description of what "Blazor mode" enables
- Parameter documentation explaining the purpose of
configureServices- Return value documentation
/// <summary> - Use Consolonize in Blazor mode + Enables Blazor support in Consolonia, allowing the use of Razor components + and providing integration with Avalonia's desktop features. /// </summary> /// <param name="builder"></param> +/// <param name="configureServices">Optional delegate to configure additional services +/// in the dependency injection container.</param> /// <returns></returns> +/// <returns>The configured AppBuilder instance for method chaining.</returns>
21-23: Document method call order dependency.The order of
UseConsolonia()beforeUseAvaloniaBlazorBindings()appears intentional. Consider adding a code comment explaining why this order is necessary to prevent future maintenance issues.return builder + // UseConsolonia must be called before UseAvaloniaBlazorBindings to ensure proper initialization .UseConsolonia() .UseAvaloniaBlazorBindings(sc =>
25-25: Fix typo in comment.The comment contains a typo: "injectoin" should be "injection".
- // Register services for injectoin + // Register services for injection
26-32: Improve lifetime service registration.The current implementation of lifetime registration could be more robust.
Consider these improvements:
- Move the registration to a separate method for better readability
- Add more context in the exception message
-sc.AddSingleton(_ => -{ - var lifetime = - (IClassicDesktopStyleApplicationLifetime)Application.Current?.ApplicationLifetime; - ArgumentNullException.ThrowIfNull(lifetime); - return lifetime; -}); +sc.AddSingleton<IClassicDesktopStyleApplicationLifetime>(sp => +{ + var app = Application.Current ?? + throw new InvalidOperationException("Application.Current is null. Ensure this is called after application initialization."); + + if (app.ApplicationLifetime is not IClassicDesktopStyleApplicationLifetime lifetime) + throw new InvalidOperationException( + $"Expected IClassicDesktopStyleApplicationLifetime but got {app.ApplicationLifetime?.GetType().Name ?? "null"}"); + + return lifetime; +});
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
src/Consolonia.Blazor/BuilderExtensions.cs(1 hunks)src/Consolonia.Blazor/ConsoloniaBlazorApplication.cs(1 hunks)src/Consolonia.Core/Helpers/Extensions.cs(2 hunks)src/Example.Blazor.Components/MainPage.razor(1 hunks)src/Example.Blazor.Components/SubPage.razor(1 hunks)src/Example.Blazor.Components/_Imports.razor(1 hunks)src/Example.Blazor/App.axaml(1 hunks)src/Example.Blazor/Program.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- src/Example.Blazor/App.axaml
- src/Example.Blazor.Components/SubPage.razor
- src/Example.Blazor/Program.cs
- src/Consolonia.Core/Helpers/Extensions.cs
- src/Consolonia.Blazor/ConsoloniaBlazorApplication.cs
- src/Example.Blazor.Components/MainPage.razor
- src/Example.Blazor.Components/_Imports.razor
🔇 Additional comments (1)
src/Consolonia.Blazor/BuilderExtensions.cs (1)
33-48:
Handle potential null reference exceptions for MainWindow access.
Multiple service registrations access MainWindow without proper null checking, which could lead to runtime issues.
Consider implementing a helper method to safely access window services:
+private static T GetWindowService<T>(IClassicDesktopStyleApplicationLifetime lifetime, Func<Window, T> serviceAccessor)
+ where T : class
+{
+ var window = lifetime.MainWindow ??
+ throw new InvalidOperationException("MainWindow is not available. Ensure the application is properly initialized.");
+ return serviceAccessor(window);
+}
sc.AddTransient(sp =>
- sp.GetRequiredService<IClassicDesktopStyleApplicationLifetime>().MainWindow?.StorageProvider);
+ GetWindowService(
+ sp.GetRequiredService<IClassicDesktopStyleApplicationLifetime>(),
+ window => window.StorageProvider));
src/Consolonia.PlatformSupport/Consolonia.PlatformSupport.csproj
Outdated
Show resolved
Hide resolved
Rest of package cleanup is in the Packages PR
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/Consolonia.Designer/Extensions.cs (1)
22-27: LGTM: Clean separation of design mode and runtime behaviorThe simplified logic improves maintainability by:
- Removing conditional compilation
- Providing clear separation between design-time and runtime behavior
- Using proper method chaining for builder configuration
This change aligns well with the Blazor integration objectives while maintaining clean architecture.
Consider documenting the design mode behavior in the XML comments above the method, as it's a crucial architectural decision that affects both development and runtime behavior.
src/Tools/Consolonia.PreviewHost/App.axaml.cs (1)
Line range hint
103-134: Remove commented-out codeThere's a significant block of commented-out code at the bottom of the file that appears to be unused. Since this PR introduces Blazor support and modernizes the codebase, this is a good opportunity to clean up technical debt.
Consider removing this commented-out code block. If this functionality needs to be preserved for reference, consider moving it to the PR or commit description instead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (14)
.gitignore(1 hunks)Directory.Build.props(1 hunks)src/Consolonia.Blazor/Consolonia.Blazor.csproj(1 hunks)src/Consolonia.Core/Infrastructure/ConsoloniaNotSupportedException.cs(0 hunks)src/Consolonia.Designer/ConsolePreview.cs(0 hunks)src/Consolonia.Designer/Consolonia.Designer.csproj(1 hunks)src/Consolonia.Designer/Extensions.cs(1 hunks)src/Consolonia.Gallery/Consolonia.Gallery.csproj(0 hunks)src/Consolonia.Gallery/Program.cs(0 hunks)src/Consolonia.PlatformSupport/Consolonia.PlatformSupport.csproj(1 hunks)src/Example.Blazor.Components/Example.Blazor.Components.csproj(1 hunks)src/Example.Blazor/Example.Blazor.csproj(1 hunks)src/Tools/Consolonia.PreviewHost/App.axaml.cs(1 hunks)src/Tools/Consolonia.PreviewHost/Properties/launchSettings.json(0 hunks)
💤 Files with no reviewable changes (5)
- src/Tools/Consolonia.PreviewHost/Properties/launchSettings.json
- src/Consolonia.Gallery/Consolonia.Gallery.csproj
- src/Consolonia.Core/Infrastructure/ConsoloniaNotSupportedException.cs
- src/Consolonia.Gallery/Program.cs
- src/Consolonia.Designer/ConsolePreview.cs
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (4)
- src/Example.Blazor.Components/Example.Blazor.Components.csproj
- src/Example.Blazor/Example.Blazor.csproj
- Directory.Build.props
- src/Consolonia.Blazor/Consolonia.Blazor.csproj
🔇 Additional comments (4)
src/Consolonia.Designer/Consolonia.Designer.csproj (2)
8-9: LGTM: Package references properly unconditional
The removal of conditional compilation for these Avalonia packages aligns with the modernization efforts and supports the new Blazor integration. This change simplifies the project structure and ensures consistent behavior across configurations.
15-15: LGTM: Project reference properly unconditional
The removal of the conditional project reference to Consolonia.PreviewHost aligns with the previous discussions and modern NuGet practices.
src/Consolonia.PlatformSupport/Consolonia.PlatformSupport.csproj (1)
Line range hint 4-7: Critical: Verify removal of core project references
The removal of references to Consolonia.Core and Consolonia.GuiCS projects seems concerning as these appear to be essential dependencies for the platform support project. This change could potentially break the build and functionality.
Let's verify if these dependencies are being provided through other means:
Please ensure that:
- The platform support functionality doesn't depend on these projects anymore, or
- These dependencies are being provided through an alternative mechanism, or
- This is an intentional change as part of the Blazor support addition (though this seems unlikely given the nature of these core dependencies)
src/Tools/Consolonia.PreviewHost/App.axaml.cs (1)
Line range hint 55-65: Consider adding Blazor file support
The current code only handles .axaml files explicitly. Since this PR adds Blazor support, consider extending the file type handling to include .razor files as well.
Let's verify if there are any Blazor-specific file handling requirements:
Co-authored-by: <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (8)
src/Consolonia.Designer/ConsolePreview.cs (8)
Line range hint
69-69: Remove outdated TODO comment in Dispose methodThe
Disposemethod already disposes of managed objects. The TODO comment is no longer necessary and can be removed to clean up the code.Apply this diff to remove the outdated comment:
- // TODO: dispose managed state (managed objects)
Line range hint
72-76: Check if the process has exited before calling_process.Kill()In the
Disposemethod, check whether_process.HasExitedis false before calling_process.Kill()to avoid exceptions when the process has already exited.Apply this diff to add the check:
if (_process != null) { + if (!_process.HasExited) + { _process.Kill(); + } _process.Dispose(); _process = null; }
Line range hint
192-196: Add check for process availability before writing to_process.StandardInputIn the
LoadXaml()method, ensure the process is still running before writing toStandardInputto prevent exceptions if the process has exited.Apply this diff to add the check:
else { + if (!_process.HasExited) + { // send request to previewHost to load the new xaml file. _process.StandardInput.WriteLine(xamlPath); + } + else + { + // Handle process exit + _process.Dispose(); + _process = null; + LoadXaml(); + } }
Line range hint
246-252: Avoid hardcoded default values for character dimensionsThe method
ComputeCharWidth()uses hardcoded default values for_charWidthand_charHeight. Consider calculating these values dynamically or making them configurable to accommodate different font sizes and styles.
Line range hint
283-313: Improve robustness of XAML attribute parsingThe method
GetDesignWidthAndHeight()uses string manipulation to extractd:DesignWidthandd:DesignHeightfrom the XAML content. Consider using an XML parser to reliably parse these attributes, enhancing robustness and maintainability.
Line range hint
294-352: ExtractTextBlockComposerinto a separate classTo improve code modularity and readability, consider moving the
TextBlockComposerclass into its own file. This aligns with the existing TODO comment and follows best practices for class organization.
42-42: Consolidate font settings to ensure consistencyThe font is set in both
_typefaceandFontFamilyusing "Cascadia Mono". To avoid potential inconsistencies, consider consolidating font settings by defining a single source of truth or using a shared constant.Also applies to: 54-54
Line range hint
65-82: Encapsulate process management into a separate helper classThe logic for starting, communicating with, and disposing the
_processis spread across multiple methods. Consider encapsulating the process management into a separate helper class to improve code maintainability and readability.Also applies to: 106-196, 199-219
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
src/Consolonia.Core/Infrastructure/ConsoloniaNotSupportedException.cs(1 hunks)src/Consolonia.Designer/ConsolePreview.cs(2 hunks)src/Consolonia.Designer/Extensions.cs(1 hunks)src/Tools/Consolonia.PreviewHost/App.axaml.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Consolonia.Core/Infrastructure/ConsoloniaNotSupportedException.cs
- src/Consolonia.Designer/Extensions.cs
- src/Tools/Consolonia.PreviewHost/App.axaml.cs
🧰 Additional context used
📓 Learnings (1)
src/Consolonia.Designer/ConsolePreview.cs (1)
Learnt from: tomlm
PR: jinek/Consolonia#150
File: src/Consolonia.Designer/Consolonia.Designer.csproj:14-15
Timestamp: 2024-11-19T07:19:33.261Z
Learning: In the Consolonia project, circular dependencies between `Consolonia.Designer` and `Consolonia.PreviewHost` are intentional and by design. The `ConsolePreview` control consumes `Consolonia.PreviewHost`, and `Avalonia.PreviewHost` consumes `ConsolePreview` control in the designer.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
src/Tools/Consolonia.PreviewHost/App.cs (2)
34-44: LGTM! Consider adding path validation logging.The improved path handling logic is more robust with explicit directory checks and proper separation of fully qualified vs relative paths. The addition of null checks enhances safety.
Consider adding debug logging to help troubleshoot path resolution issues. For example:
if (Path.IsPathFullyQualified(path)) { + Debug.WriteLine($"Resolving fully qualified path: {path}"); if (Directory.Exists(path)) + { folder = path; + Debug.WriteLine($"Using directory path: {folder}"); + } else { folder = Path.GetDirectoryName(path)!; + Debug.WriteLine($"Using parent directory: {folder}"); } } else { folder = Environment.CurrentDirectory; + Debug.WriteLine($"Using current directory: {folder}"); }
Line range hint
102-139: Remove commented-out code.The file contains a large block of commented-out code related to file watching. If this functionality has been moved elsewhere or is no longer needed, consider removing it to maintain code cleanliness.
src/Consolonia.Blazor/BuilderExtensions.cs (2)
12-14: Correct the typographical error in the summary commentThe word "Consolonize" in the summary comment should be corrected to "Consolonia" to match the method's intent and naming.
24-24: Fix the typo in the inline commentThe comment on line 24 contains a typo. Replace "injectoin" with "injection" for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
Directory.Build.props(1 hunks)src/Consolonia.Blazor/BuilderExtensions.cs(1 hunks)src/Consolonia.Blazor/Consolonia.Blazor.csproj(1 hunks)src/Consolonia.Core/Infrastructure/ConsoloniaApplication.cs(2 hunks)src/Consolonia.Gallery/App.cs(0 hunks)src/Consolonia.PlatformSupport/Consolonia.PlatformSupport.csproj(1 hunks)src/Consolonia.sln(2 hunks)src/Example.Blazor.Components/Example.Blazor.Components.csproj(1 hunks)src/Example.Blazor/App.axaml(1 hunks)src/Example.Blazor/Example.Blazor.csproj(1 hunks)src/Example.Blazor/Program.cs(1 hunks)src/Example/App.cs(1 hunks)src/Tools/Consolonia.PreviewHost/App.cs(1 hunks)
💤 Files with no reviewable changes (1)
- src/Consolonia.Gallery/App.cs
✅ Files skipped from review due to trivial changes (1)
- src/Example/App.cs
🚧 Files skipped from review as they are similar to previous changes (8)
- src/Example.Blazor/Example.Blazor.csproj
- src/Consolonia.Blazor/Consolonia.Blazor.csproj
- src/Example.Blazor/App.axaml
- src/Example.Blazor/Program.cs
- src/Example.Blazor.Components/Example.Blazor.Components.csproj
- src/Consolonia.PlatformSupport/Consolonia.PlatformSupport.csproj
- Directory.Build.props
- src/Consolonia.Core/Infrastructure/ConsoloniaApplication.cs
🔇 Additional comments (5)
src/Tools/Consolonia.PreviewHost/App.cs (1)
34-44: Verify impact on hot reload functionality.
Since this path resolution logic is crucial for file watching and hot reload (as indicated by the commented code at the bottom), ensure these changes don't affect the new Blazor hot reload functionality mentioned in the PR objectives.
Let's check for any file watcher implementations that might depend on this path resolution:
✅ Verification successful
Path resolution changes are safe for hot reload functionality
The path resolution changes in App.cs do not impact hot reload functionality. The file watching implementation is actually handled in XamlFileViewModel.cs where the WatchFileChanges() method correctly uses the full path from the FullName property to set up the FileSystemWatcher. The commented-out watcher code in App.cs is legacy code that's no longer in use.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for file watcher implementations
rg -l "FileSystemWatcher|EnableRaisingEvents"
# Search for hot reload related code
rg -l "HotReload|LiveReload|Watch"
Length of output: 294
Script:
#!/bin/bash
# Let's examine the implementation details in both files
echo "=== App.cs ==="
rg -C 5 "FileSystemWatcher|EnableRaisingEvents|HotReload|LiveReload|Watch" "src/Tools/Consolonia.PreviewHost/App.cs"
echo -e "\n=== XamlFileViewModel.cs ==="
rg -C 5 "FileSystemWatcher|EnableRaisingEvents|HotReload|LiveReload|Watch" "src/Tools/Consolonia.PreviewHost/ViewModels/XamlFileViewModel.cs"
Length of output: 4165
src/Consolonia.Blazor/BuilderExtensions.cs (2)
17-51: Overall implementation is well-structured
The UseConsoloniaBlazor extension method is effectively integrating Consolonia with Blazor and setting up the necessary services. The use of optional configureServices allows for flexible service customization.
32-47:
Handle potential null references when accessing MainWindow services
The services registered are accessing MainWindow using the null-conditional operator. If MainWindow is null, the registered services will be null, which could lead to NullReferenceException when these services are consumed. Consider throwing an exception if MainWindow is null during service resolution.
Apply this diff to enforce non-null MainWindow:
-sc.AddTransient(sp =>
- sp.GetRequiredService<IClassicDesktopStyleApplicationLifetime>().MainWindow?.StorageProvider);
+sc.AddTransient(sp =>
+{
+ var mainWindow = sp.GetRequiredService<IClassicDesktopStyleApplicationLifetime>().MainWindow
+ ?? throw new InvalidOperationException("MainWindow is not available");
+ return mainWindow.StorageProvider;
+});Repeat similar changes for the other services accessing MainWindow.
src/Consolonia.sln (2)
68-73: New projects added correctly to the solution
The projects Consolonia.Blazor, Example.Blazor, and Example.Blazor.Components are properly included in the solution file with correct paths and GUIDs.
132-151: Project configurations updated appropriately
The build configurations for the newly added projects are correctly defined for both Debug and Release modes, ensuring they are included in the solution build process.
|
sorry I had merge conflicts I had to resolve. |
TLDR
Details
Example Razor file
Things to note: