-
-
Notifications
You must be signed in to change notification settings - Fork 253
Apply several enhancements in bit Boilerplate project template (#11512) #11513
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
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughMultiple enhancements to the boilerplate template addressing SignalR event handler disposal, removal of duplicate telemetry headers, SIMD optimization for non-development builds, standardization of embedding vector dimensions to 384, elimination of redundant version checks, configuration cleanup, infrastructure updates, telemetry endpoint filtering, and test database initialization refactoring. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/TestsInitializer.cs (1)
104-104: Consider clarifying the migration transition path.The change correctly implements the PR objective and simplifies test initialization. However, the comment could be more explicit about the transition path.
EnsureCreatedAsync()is incompatible with EF Core migrations—once migrations are added, this line must be changed toDatabase.MigrateAsync(), otherwise the test database schema won't match the migrated schema used in production.Consider enhancing the comment:
- await dbContext.Database.EnsureCreatedAsync(); // It's recommended to start using ef-core migrations. + await dbContext.Database.EnsureCreatedAsync(); // It's recommended to start using ef-core migrations. When migrations are added, replace EnsureCreatedAsync with MigrateAsync.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (13)
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/AppClientCoordinator.cs(3 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Extensions/IClientCoreServiceCollectionExtensions.cs(0 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Web/Boilerplate.Client.Web.csproj(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Data/Configurations/Product/ProductConfiguration.cs(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Program.Services.cs(0 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/SignalR/AppHub.cs(0 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/appsettings.json(0 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.AppHost/Program.cs(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.AppHost/appsettings.Development.json(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Shared/Extensions/WebApplicationBuilderExtensions.cs(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Shared/Services/AppOpenTelemetryProcessor.cs(2 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Web/appsettings.json(0 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/TestsInitializer.cs(1 hunks)
💤 Files with no reviewable changes (5)
- src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/appsettings.json
- src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Program.Services.cs
- src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Web/appsettings.json
- src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Extensions/IClientCoreServiceCollectionExtensions.cs
- src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/SignalR/AppHub.cs
🧰 Additional context used
🪛 Biome (2.1.2)
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.AppHost/appsettings.Development.json
[error] 20-20: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 20-20: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 20-20: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 20-20: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 21-21: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 21-21: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 21-21: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build and test
🔇 Additional comments (7)
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.AppHost/appsettings.Development.json (1)
20-21: LGTM!The keys have been correctly renamed from
minio-*tos3-*in a consistent manner, aligning with PR objective #7. The configuration values and comment text are properly maintained.Note: The static analysis errors are false positives; the Biome parser incorrectly flags valid JSON that contains template preprocessor directives.
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.AppHost/Program.cs (1)
101-103: LGTM! Correctly implements explicit start requirement.The Blazor WebAssembly project is now properly configured with
.WithExplicitStart(), requiring manual start instead of automatic launch. The placement inside theIsRunModeblock is appropriate since this project is for testing purposes only. This change aligns with PR objective #8 and maintains consistency with the Windows client project configuration at line 116.src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/AppClientCoordinator.cs (2)
180-230: Good defensive practice for preventing duplicate SignalR handlers.The
hubConnection.Remove(...)calls before each.On(...)registration ensure any existing handlers are unregistered before adding new ones. This prevents duplicate event handling and potential memory leaks ifSubscribeToSignalREventsMessageswere ever called multiple times.
313-329: Correct disposal order: own resources before base.The reordering ensures SignalR event handlers are properly disposed before calling the base class disposal. Explicitly resetting
signalROnDisposablesto an empty list (line 325) and movingawait base.DisposeAsync(disposing)to the end (line 328) follows disposal best practices.src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Web/Boilerplate.Client.Web.csproj (1)
12-12: Objective #3 wording may be backwards.The code correctly enables SIMD in non-Development environments (Production gets SIMD for performance; Development disables it for faster builds). However, PR objective #3 states "Enable Wasm SIMD in dev environment to avoid wasm native builds," which is the opposite of the implemented behavior. Confirm the objective description is correct, or update it to reflect that SIMD is enabled in production builds.
(The concern about unset Environment variables is not valid—Environment is always initialized via conditional logic based on Configuration.)
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Shared/Extensions/WebApplicationBuilderExtensions.cs (1)
135-150: LGTM! Clean refactor for filtering telemetry paths.The array-based approach with
StartsWithSegmentscorrectly filters health check and static asset requests from OpenTelemetry tracing. The use ofStringComparison.OrdinalIgnoreCaseis appropriate for path matching.src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Shared/Services/AppOpenTelemetryProcessor.cs (1)
5-5: LGTM! Public visibility is appropriate.Making the processor public is correct since it's registered as a generic type parameter in the OpenTelemetry configuration (
AddProcessor<AppOpenTelemetryProcessor>()). This visibility allows proper instantiation by the telemetry framework.
...rplate/src/Server/Boilerplate.Server.Api/Data/Configurations/Product/ProductConfiguration.cs
Outdated
Show resolved
Hide resolved
...e/Bit.Boilerplate/src/Server/Boilerplate.Server.Shared/Services/AppOpenTelemetryProcessor.cs
Show resolved
Hide resolved
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.
Pull Request Overview
This PR streamlines and optimizes the boilerplate template by removing unnecessary configurations and improving resource management in several areas:
- Removes AI embedding dimension configuration (now inferred from models)
- Simplifies database initialization for tests by using
EnsureCreatedAsync - Removes client app version checking mechanism from SignalR
- Enhances OpenTelemetry filtering and SignalR event handling
- Optimizes Blazor WebAssembly startup and SIMD configuration
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| TestsInitializer.cs | Simplified database initialization to use EnsureCreatedAsync instead of migration checks |
| appsettings.json (Server.Web & Server.Api) | Removed hardcoded EmbeddingOptions.Dimensions configuration |
| AppOpenTelemetryProcessor.cs | Added filtering for Blazor HandleEvent operations |
| WebApplicationBuilderExtensions.cs | Improved OpenTelemetry request filtering with cleaner logic for health checks and static files |
| appsettings.Development.json | Updated MinIO parameter names from "minio-" prefix to "s3-" prefix |
| Program.cs (AppHost) | Moved client WebAssembly project inside run mode check with explicit start |
| Program.Services.cs | Removed embedding options configuration binding for all AI providers |
| ProductConfiguration.cs | Updated vector dimensions from 768 to 384 with spelling error in comment |
| Boilerplate.Client.Web.csproj | Changed SIMD configuration to be enabled by default except in Development |
| IClientCoreServiceCollectionExtensions.cs | Removed app version headers from SignalR connection |
| AppHub.cs | Removed CheckClientAppVersion method and its invocation |
| AppClientCoordinator.cs | Added Remove calls before event subscriptions and reordered base.DisposeAsync |
closes #11512
Summary by CodeRabbit
Release Notes
Performance & Optimization
Bug Fixes
Changes