Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the codebase by reorganizing namespace structures, upgrading to .NET 10, migrating from SQLite to MS SQL Server test containers, introducing a code formatter (CSharpier), and standardizing code formatting across both server and client codebases.
Key Changes:
- Migrated from .NET 9 to .NET 10 with updated package dependencies
- Reorganized domain namespaces from
Common.Domainsto top-levelDomainsandCommon.Adapters.DatatoCommon.Data - Replaced in-memory SQLite test database setup with a shared
DatabaseFixtureusing MS SQL Server test containers
Reviewed changes
Copilot reviewed 106 out of 117 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Evently.Server/Evently.Server.csproj | Updated target framework to net10.0 and upgraded NuGet package versions |
| tests/Evently.Server.Test/Evently.Server.Test.csproj | Updated target framework to net10.0 and upgraded test-related packages |
| src/Evently.Server/Dockerfile | Updated base images from .NET 9 to .NET 10 SDK and runtime |
| tests/Evently.Server.Test/Common/Setup/DatabaseFixture.cs | New shared test fixture using MS SQL Server test containers |
| src/Evently.Server/Domains/* | Moved domain entities, interfaces, models, and exceptions from Common.Domains to Domains namespace |
| src/Evently.Server/Common/Data/* | Moved data context and migrations from Common.Adapters.Data to Common.Data |
| Makefile | Replaced JetBrains cleanup with CSharpier formatter |
| dotnet-tools.json | Added CSharpier as a formatting tool |
| .editorconfig | Removed in favor of CSharpier configuration |
Files not reviewed (2)
- src/Evently.Server/Common/Data/Migrations/20250913035915_SQLServer.Designer.cs: Language not supported
- src/Evently.Server/Common/Data/Migrations/20250927053802_UpdateSeededDates.Designer.cs: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| public void Dispose() { | ||
| _dbContext?.Dispose(); | ||
| _container.DisposeAsync(); |
There was a problem hiding this comment.
Missing await keyword for async disposal. The DisposeAsync() method returns a ValueTask that should be awaited. Change to await _container.DisposeAsync(); or use synchronous disposal if the Dispose method cannot be async.
| attempts -= 1; | ||
| console.error(error); | ||
| } | ||
| } |
There was a problem hiding this comment.
The retry logic silently fails after 2 attempts without notifying the user or throwing an error. If both attempts fail, categories will be an empty array with no indication that data fetching failed. Consider either throwing an error after exhausting retries or providing user feedback about the failure.
| } | |
| } | |
| if (categories.length === 0) { | |
| throw new Error("Failed to fetch categories after 2 attempts."); | |
| } |
1 participant
No description provided.