Feature/notification settings#486
Conversation
|
Warning Rate limit exceeded@NielsPilgaard has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 16 minutes and 37 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. 📒 Files selected for processing (4)
WalkthroughAdds per-user notification preferences (chat and group-post), DB migration columns, services and DI for managing preferences, consumer and ChatNotificationService changes to send preference-aware emails, a settings UI and menu entry, and updated tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Consumer as SendMessageConsumer
participant DB as JordnaerDbContext
participant NotifSvc as ChatNotificationService
participant Pub as IPublishEndpoint
Consumer->>DB: Persist chat message
DB-->>Consumer: Save success
Consumer->>NotifSvc: NotifyRecipientsOfNewMessage(message, ct)
NotifSvc->>DB: Query chat participants + UserProfiles (preferences)
DB-->>NotifSvc: Users + preferences
Note over NotifSvc: Filter recipients by preference\n(FirstMessageOnly / AllMessages)
alt recipients found
NotifSvc->>Pub: Publish email messages
Pub-->>NotifSvc: Publish ack
else no recipients
NotifSvc-->>NotifSvc: Log no recipients
end
NotifSvc-->>Consumer: Completed
Consumer->>Consumer: Increment metrics
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/web/Jordnaer/Consumers/GroupPostCreatedConsumer.cs (1)
33-51: MissingEmailOnNewPostpreference filter.The consumer fetches all active members but does not respect the new
EmailOnNewPostpreference onGroupMembership. Users who have disabled email notifications for a group will still receive emails.🔎 Proposed fix to filter by EmailOnNewPost preference
// Get all active members excluding the post author -var activeMembers = context.GroupMemberships +var activeMembers = await context.GroupMemberships .AsNoTracking() .Where(x => x.GroupId == message.GroupId && x.MembershipStatus == MembershipStatus.Active && - x.UserProfileId != message.AuthorId) - .Select(x => x.UserProfileId); + x.UserProfileId != message.AuthorId && + x.EmailOnNewPost) + .Select(x => x.UserProfileId) + .ToListAsync(consumeContext.CancellationToken); // Get their email addresses var emails = await context.Users .AsNoTracking() - .Where(user => activeMembers.Any(userId => userId == user.Id) && + .Where(user => activeMembers.Contains(user.Id) && !string.IsNullOrEmpty(user.Email))
🧹 Nitpick comments (3)
src/web/Jordnaer/Pages/Groups/MyGroups.razor (1)
113-117: Consider avoiding !important for maintainability.The
!importantflag can make future styling adjustments more difficult. If the padding override is necessary due to MudBlazor's default styles, consider whether a more specific selector or a different approach could achieve the same result without!important.tests/web/Jordnaer.Tests/Chat/ChatNotificationServiceTests.cs (1)
38-47: Mixed mocking frameworks in the same test class.The test uses both Moq (for
JordnaerDbContext,IPublishEndpoint) and NSubstitute (forIOptions<AppOptions>). While functional, mixing frameworks reduces consistency.Consider standardizing on one mocking framework for consistency.
src/web/Jordnaer/Pages/Settings/Notifications.razor (1)
207-242: Sequential API calls in bulk operations may cause poor UX.
EnableAllGroupsandDisableAllGroupsmake individual API calls in a loop. For users with many group memberships, this could be slow and result in many snackbar messages if errors occur.Consider adding a bulk update endpoint in
INotificationSettingsServiceto handle all changes in a single database transaction.🔎 Example bulk update approach
// In INotificationSettingsService Task<OneOf<Success, NotFound>> SetAllGroupPostPreferencesAsync( string userId, bool enabled, CancellationToken cancellationToken = default); // In the page private async Task EnableAllGroups() { if (CurrentUser.Id is null) return; try { var result = await NotificationSettingsService .SetAllGroupPostPreferencesAsync(CurrentUser.Id, true); result.Switch( success => { foreach (var s in _groupSettings) s.EmailOnNewPost = true; Snackbar.Add("Alle gruppe notifikationer aktiveret", Severity.Success); }, notFound => Snackbar.Add("Kunne ikke opdatere indstillinger.", Severity.Error) ); } catch (Exception) { Snackbar.Add("Der opstod en fejl. Prøv igen senere.", Severity.Error); } }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
src/shared/Jordnaer.Shared/Database/Enums/ChatNotificationPreference.cssrc/shared/Jordnaer.Shared/Database/GroupMembership.cssrc/shared/Jordnaer.Shared/Database/UserProfile.cssrc/web/Jordnaer/Consumers/GroupPostCreatedConsumer.cssrc/web/Jordnaer/Consumers/SendMessageConsumer.cssrc/web/Jordnaer/Features/Chat/ChatNotificationService.cssrc/web/Jordnaer/Features/Notifications/NotificationSettingsService.cssrc/web/Jordnaer/Features/Notifications/WebApplicationBuilderExtensions.cssrc/web/Jordnaer/Migrations/20260102194640_Add_NotificationSettings.Designer.cssrc/web/Jordnaer/Migrations/20260102194640_Add_NotificationSettings.cssrc/web/Jordnaer/Migrations/JordnaerDbContextModelSnapshot.cssrc/web/Jordnaer/Migrations/migration_script.sqlsrc/web/Jordnaer/Pages/Groups/Members.razorsrc/web/Jordnaer/Pages/Groups/MyGroups.razorsrc/web/Jordnaer/Pages/Settings/Notifications.razorsrc/web/Jordnaer/Pages/Shared/TopBar.razorsrc/web/Jordnaer/Program.cstasks/02-group-notifications.mdtests/web/Jordnaer.Tests/Chat/ChatNotificationServiceTests.cs
💤 Files with no reviewable changes (1)
- tasks/02-group-notifications.md
🧰 Additional context used
📓 Path-based instructions (4)
**/WebApplicationBuilderExtensions.cs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
DI setup for features must be in
WebApplicationBuilderExtensions.csextension methods called fromProgram.cs
Files:
src/web/Jordnaer/Features/Notifications/WebApplicationBuilderExtensions.cs
**/{Features,Consumers}/**/*.cs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/{Features,Consumers}/**/*.cs: Always useawait using var context = await contextFactory.CreateDbContextAsync()for scoped DbContext instances
UseAsNoTracking()for read-only database queries in EF Core
UseIHubContextfor broadcasting messages to SignalR clients from outside the hub
Files:
src/web/Jordnaer/Features/Notifications/WebApplicationBuilderExtensions.cssrc/web/Jordnaer/Consumers/SendMessageConsumer.cssrc/web/Jordnaer/Consumers/GroupPostCreatedConsumer.cssrc/web/Jordnaer/Features/Chat/ChatNotificationService.cssrc/web/Jordnaer/Features/Notifications/NotificationSettingsService.cs
**/Features/**/*.cs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use OneOf return types for explicit error handling instead of exceptions
Files:
src/web/Jordnaer/Features/Notifications/WebApplicationBuilderExtensions.cssrc/web/Jordnaer/Features/Chat/ChatNotificationService.cssrc/web/Jordnaer/Features/Notifications/NotificationSettingsService.cs
**/Consumers/**/*.cs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/Consumers/**/*.cs: MassTransit consumers must be in the Jordnaer.Consumers namespace for auto-discovery
Track metrics in MassTransit consumers usingJordnaerMetrics.Counter.Add()
Files:
src/web/Jordnaer/Consumers/SendMessageConsumer.cssrc/web/Jordnaer/Consumers/GroupPostCreatedConsumer.cs
🧠 Learnings (8)
📚 Learning: 2025-12-30T22:07:15.171Z
Learnt from: CR
Repo: NielsPilgaard/Jordnaer PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-30T22:07:15.171Z
Learning: Applies to **/WebApplicationBuilderExtensions.cs : DI setup for features must be in `WebApplicationBuilderExtensions.cs` extension methods called from `Program.cs`
Applied to files:
src/web/Jordnaer/Features/Notifications/WebApplicationBuilderExtensions.cssrc/web/Jordnaer/Consumers/GroupPostCreatedConsumer.cssrc/web/Jordnaer/Program.cstests/web/Jordnaer.Tests/Chat/ChatNotificationServiceTests.cs
📚 Learning: 2025-12-29T18:32:51.546Z
Learnt from: NielsPilgaard
Repo: NielsPilgaard/Jordnaer PR: 481
File: src/web/Jordnaer/Pages/Groups/EditGroup.razor:137-139
Timestamp: 2025-12-29T18:32:51.546Z
Learning: In the Jordnaer codebase, Group.Categories is never null and is always initialized. When reviewing Razor components under src/web/Jordnaer/Pages, avoid adding null checks for Group.Categories and rely on its non-null guarantee. If a path could yield a null value due to data from an external source, add a guard only there, but otherwise remove redundant null checks for this property in these files.
Applied to files:
src/web/Jordnaer/Pages/Groups/Members.razorsrc/web/Jordnaer/Pages/Shared/TopBar.razorsrc/web/Jordnaer/Pages/Groups/MyGroups.razorsrc/web/Jordnaer/Pages/Settings/Notifications.razor
📚 Learning: 2025-12-30T22:07:15.171Z
Learnt from: CR
Repo: NielsPilgaard/Jordnaer PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-30T22:07:15.171Z
Learning: Applies to **/{Features,Consumers}/**/*.cs : Use `IHubContext` for broadcasting messages to SignalR clients from outside the hub
Applied to files:
src/web/Jordnaer/Consumers/SendMessageConsumer.cssrc/web/Jordnaer/Consumers/GroupPostCreatedConsumer.cssrc/web/Jordnaer/Features/Chat/ChatNotificationService.cstests/web/Jordnaer.Tests/Chat/ChatNotificationServiceTests.cs
📚 Learning: 2025-12-30T22:07:15.171Z
Learnt from: CR
Repo: NielsPilgaard/Jordnaer PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-30T22:07:15.171Z
Learning: Applies to **/SignalR/**/*.cs : SignalR hubs must define an interface (e.g., `IChatHub`) for type-safe client method invocations
Applied to files:
src/web/Jordnaer/Consumers/SendMessageConsumer.cs
📚 Learning: 2025-12-30T22:07:15.171Z
Learnt from: CR
Repo: NielsPilgaard/Jordnaer PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-30T22:07:15.171Z
Learning: Applies to **/Consumers/**/*.cs : MassTransit consumers must be in the Jordnaer.Consumers namespace for auto-discovery
Applied to files:
src/web/Jordnaer/Consumers/SendMessageConsumer.cssrc/web/Jordnaer/Consumers/GroupPostCreatedConsumer.cstests/web/Jordnaer.Tests/Chat/ChatNotificationServiceTests.cs
📚 Learning: 2025-12-30T22:07:15.171Z
Learnt from: CR
Repo: NielsPilgaard/Jordnaer PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-30T22:07:15.171Z
Learning: Applies to **/{Features,Consumers}/**/*.cs : Use `AsNoTracking()` for read-only database queries in EF Core
Applied to files:
src/web/Jordnaer/Consumers/GroupPostCreatedConsumer.cstests/web/Jordnaer.Tests/Chat/ChatNotificationServiceTests.cs
📚 Learning: 2025-12-30T22:07:15.171Z
Learnt from: CR
Repo: NielsPilgaard/Jordnaer PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-30T22:07:15.171Z
Learning: Applies to **/{Features,Consumers}/**/*.cs : Always use `await using var context = await contextFactory.CreateDbContextAsync()` for scoped DbContext instances
Applied to files:
src/web/Jordnaer/Consumers/GroupPostCreatedConsumer.cstests/web/Jordnaer.Tests/Chat/ChatNotificationServiceTests.cs
📚 Learning: 2025-12-30T22:07:15.171Z
Learnt from: CR
Repo: NielsPilgaard/Jordnaer PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-30T22:07:15.171Z
Learning: Applies to **/Consumers/**/*.cs : Track metrics in MassTransit consumers using `JordnaerMetrics.Counter.Add()`
Applied to files:
src/web/Jordnaer/Consumers/GroupPostCreatedConsumer.cstests/web/Jordnaer.Tests/Chat/ChatNotificationServiceTests.cs
🧬 Code graph analysis (8)
src/web/Jordnaer/Features/Notifications/WebApplicationBuilderExtensions.cs (1)
src/web/Jordnaer/Features/Notifications/NotificationSettingsService.cs (1)
NotificationSettingsService(18-89)
src/web/Jordnaer/Consumers/SendMessageConsumer.cs (1)
src/web/Jordnaer/Features/Chat/ChatNotificationService.cs (1)
ChatNotificationService(12-158)
src/web/Jordnaer/Consumers/GroupPostCreatedConsumer.cs (2)
src/web/Jordnaer/Extensions/AppOptions.cs (1)
AppOptions(5-11)src/shared/Jordnaer.Shared/Groups/Events/GroupPostCreated.cs (1)
GroupPostCreated(3-12)
src/web/Jordnaer/Migrations/20260102194640_Add_NotificationSettings.Designer.cs (2)
src/web/Jordnaer/Migrations/JordnaerDbContextModelSnapshot.cs (1)
DbContext(14-902)src/web/Jordnaer/Migrations/20260102194640_Add_NotificationSettings.cs (1)
Add_NotificationSettings(8-39)
src/web/Jordnaer/Features/Chat/ChatNotificationService.cs (4)
src/web/Jordnaer/Extensions/AppOptions.cs (1)
AppOptions(5-11)src/web/Jordnaer/Features/Email/SendEmail.cs (1)
SendEmail(5-33)src/web/Jordnaer/Features/Email/EmailRecipient.cs (1)
EmailRecipient(6-78)src/web/Jordnaer/Features/Email/EmailConstants.cs (2)
EmailConstants(3-15)Greeting(12-14)
tests/web/Jordnaer.Tests/Chat/ChatNotificationServiceTests.cs (2)
src/web/Jordnaer/Extensions/AppOptions.cs (1)
AppOptions(5-11)src/web/Jordnaer/Features/Chat/ChatNotificationService.cs (1)
ChatNotificationService(12-158)
src/web/Jordnaer/Features/Notifications/NotificationSettingsService.cs (1)
src/shared/Jordnaer.Shared/Database/GroupMembership.cs (1)
GroupMembership(3-29)
src/shared/Jordnaer.Shared/Database/Enums/ChatNotificationPreference.cs (1)
src/shared/Jordnaer.Shared/Extensions/EnumExtensions.cs (1)
EnumExtensions(5-22)
🔇 Additional comments (17)
src/web/Jordnaer/Consumers/SendMessageConsumer.cs (1)
69-70: Notification errors are uncaught and won't affect message delivery.The call to
NotifyRecipientsOfNewMessageis placed outside the try-catch block, which means:
- If notification sending fails, the exception will propagate and potentially cause the consumer to retry the entire message
- The message has already been saved and sent via SignalR at this point
Verify this is the intended behavior. If notifications should not affect message delivery reliability, consider wrapping this call in its own try-catch block.
src/web/Jordnaer/Pages/Shared/TopBar.razor (1)
135-139: LGTM! Clean UI integration for notification settings.The new notification settings menu items are consistently implemented for both desktop and mobile views, using appropriate icons and Danish labels.
Also applies to: 167-171
src/web/Jordnaer/Program.cs (2)
17-17: LGTM!The import is correctly added for the new Notifications feature namespace.
88-88: LGTM!The notification services registration follows the established DI pattern correctly. Based on learnings, extension methods are properly called from Program.cs for feature setup.
src/web/Jordnaer/Migrations/JordnaerDbContextModelSnapshot.cs (2)
304-306: LGTM - Auto-generated migration snapshot.The
EmailOnNewPostproperty is correctly reflected in the model snapshot. Note: This file is auto-generated by EF Core.
475-477: LGTM - Auto-generated migration snapshot.The
ChatNotificationPreferenceproperty is correctly reflected in the model snapshot. Note: This file is auto-generated by EF Core.src/web/Jordnaer/Features/Notifications/WebApplicationBuilderExtensions.cs (1)
1-10: LGTM!The extension method correctly registers the notification settings service as scoped, following the established DI pattern in
WebApplicationBuilderExtensions.csas per the coding guidelines. The fluent return pattern is consistent with other builders.src/web/Jordnaer/Consumers/GroupPostCreatedConsumer.cs (1)
63-64: URL construction change looks correct.The switch from
NavigationManager.BaseUritoIOptions<AppOptions>.Value.BaseUrlis appropriate. Using configuration-based URLs is more reliable in background consumers whereNavigationManagermay not have access to the request context.tests/web/Jordnaer.Tests/Chat/ChatNotificationServiceTests.cs (1)
224-237: Test correctly validates fallback behavior.The test verifies that when
BaseUrlis null, the service falls back to the default domain. The NSubstitute setup on line 229 correctly overrides the value for this specific test case sinceIOptions<T>.Valueis evaluated at call time.src/web/Jordnaer/Pages/Settings/Notifications.razor (1)
133-149: LGTM on initialization logic.The
OnInitializedAsynccorrectly handles null user ID, loads chat preference from the cachedCurrentUser.UserProfile, and fetches group preferences asynchronously. Thefinallyblock ensures loading state is always cleared.src/web/Jordnaer/Features/Chat/ChatNotificationService.cs (2)
23-47: Notification preferences are correctly integrated.The updated
NotifyRecipientsmethod properly joins withUserProfilesto filter recipients based on theirChatNotificationPreference. Users withNonepreference are correctly excluded. The early return for empty recipients prevents unnecessary processing.
83-143: NewNotifyRecipientsOfNewMessagemethod correctly handles "AllMessages" preference.The method properly:
- Excludes the sender from recipients
- Filters to only users with
AllMessagespreference- Handles missing sender gracefully with logging
- Uses
PublishBatchfor efficiencysrc/web/Jordnaer/Migrations/20260102194640_Add_NotificationSettings.Designer.cs (1)
1-906: Auto-generated migration designer file.This is an EF Core auto-generated file that captures the model snapshot at migration time. The new properties
EmailOnNewPost(line 307-308) andChatNotificationPreference(line 478-479) are correctly included in the model.src/web/Jordnaer/Features/Notifications/NotificationSettingsService.cs (4)
20-29: LGTM!The method correctly follows the coding guidelines by using
await using var contextandAsNoTracking()for the read-only query. Returning the default preference when the user profile is not found is a reasonable fallback behavior.
31-46: LGTM!The method correctly implements the update operation with proper error handling using OneOf return type. Correctly omits
AsNoTracking()since this is a write operation.
61-76: LGTM!The method correctly implements the update operation with proper error handling using OneOf return type and correctly omits
AsNoTracking()for the write operation.
78-88: LGTM!The method correctly follows the coding guidelines by using
await using var contextandAsNoTracking(). Including theGroupnavigation property is appropriate for returning complete preference information to callers, and filtering byActivemembership status is the correct approach.
github-project-automation
Bot
moved this from Todo
to In Progress
in Jordnaer Community Website
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/web/Jordnaer.Tests/Chat/ChatNotificationServiceTests.cs (1)
259-272: Test mutates shared_appOptionswhich may affect other tests.The test changes
_appOptions.Value.Returns(...)on a shared instance. If tests run in parallel or non-deterministic order, this could cause flaky behavior. Consider creating a separate service instance with null BaseUrl for this test.🔎 Proposed fix
[Fact] public void GetChatLink_ShouldReturnDefault_WhenServerAddressNotAvailable() { // Arrange var chatId = Guid.NewGuid(); - _appOptions.Value.Returns(new AppOptions { BaseUrl = null }); + var nullAppOptions = Substitute.For<IOptions<AppOptions>>(); + nullAppOptions.Value.Returns(new AppOptions { BaseUrl = null }); + var serviceWithNullBaseUrl = new ChatNotificationService( + Mock.Of<IDbContextFactory<JordnaerDbContext>>(), + new NullLogger<StartChatConsumer>(), + Mock.Of<IPublishEndpoint>(), + nullAppOptions + ); var defaultLink = $"https://mini-moeder.dk/chat/{chatId}"; // Act - var link = _service.GetChatLink(chatId); + var link = serviceWithNullBaseUrl.GetChatLink(chatId); // Assert link.Should().Be(defaultLink); }
♻️ Duplicate comments (1)
src/web/Jordnaer/Features/Notifications/NotificationSettingsService.cs (1)
49-60: Default return value for non-existent memberships should befalse.This issue was already flagged in a previous review. Returning
truewhen membership doesn't exist means emails would be sent to non-members.
🧹 Nitpick comments (4)
src/web/Jordnaer/Pages/Settings/Notifications.razor (2)
106-110: Consider adding save state tracking for individual group toggles.Currently, rapid toggling could trigger multiple concurrent saves and snackbar messages. For a smoother UX, consider tracking per-group save state or debouncing the save operation.
133-149: Exception inGetGroupPreferencesAsyncwill propagate unhandled.Unlike save methods,
OnInitializedAsyncdoesn't catch exceptions from the service call. If the database is unavailable, users will see an error page instead of a graceful message.🔎 Proposed fix
protected override async Task OnInitializedAsync() { try { if (CurrentUser.Id is null) { return; } _chatPreference = CurrentUser.UserProfile?.ChatNotificationPreference ?? ChatNotificationPreference.FirstMessageOnly; _groupSettings = await NotificationSettingsService.GetGroupPreferencesAsync(CurrentUser.Id); } + catch (Exception) + { + Snackbar.Add("Kunne ikke indlæse notifikationsindstillinger.", Severity.Error); + _groupSettings = new(); + } finally { _isLoading = false; } }tests/web/Jordnaer.Tests/Chat/ChatNotificationServiceTests.cs (2)
39-42: Mixing Moq and NSubstitute mocking frameworks.The test class uses
Mock<IPublishEndpoint>(Moq) alongsideSubstitute.For<IOptions<AppOptions>>(NSubstitute). Consider using one framework consistently for better maintainability.
144-147: Test assertion could be stronger.Only asserting
NotThrowAsyncdoesn't verify the notification logic works correctly. Consider verifying that_publishEndpointMock.Publishwas called with expected email content for recipients withAllMessagespreference.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/web/Jordnaer/Consumers/GroupPostCreatedConsumer.cssrc/web/Jordnaer/Features/Notifications/NotificationSettingsService.cssrc/web/Jordnaer/Pages/Groups/MyGroups.razorsrc/web/Jordnaer/Pages/Settings/Notifications.razortests/web/Jordnaer.Tests/Chat/ChatNotificationServiceTests.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/web/Jordnaer/Consumers/GroupPostCreatedConsumer.cs
🧰 Additional context used
📓 Path-based instructions (2)
**/{Features,Consumers}/**/*.cs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/{Features,Consumers}/**/*.cs: Always useawait using var context = await contextFactory.CreateDbContextAsync()for scoped DbContext instances
UseAsNoTracking()for read-only database queries in EF Core
UseIHubContextfor broadcasting messages to SignalR clients from outside the hub
Files:
src/web/Jordnaer/Features/Notifications/NotificationSettingsService.cs
**/Features/**/*.cs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use OneOf return types for explicit error handling instead of exceptions
Files:
src/web/Jordnaer/Features/Notifications/NotificationSettingsService.cs
🧠 Learnings (1)
📚 Learning: 2025-12-29T18:32:51.546Z
Learnt from: NielsPilgaard
Repo: NielsPilgaard/Jordnaer PR: 481
File: src/web/Jordnaer/Pages/Groups/EditGroup.razor:137-139
Timestamp: 2025-12-29T18:32:51.546Z
Learning: In the Jordnaer codebase, Group.Categories is never null and is always initialized. When reviewing Razor components under src/web/Jordnaer/Pages, avoid adding null checks for Group.Categories and rely on its non-null guarantee. If a path could yield a null value due to data from an external source, add a guard only there, but otherwise remove redundant null checks for this property in these files.
Applied to files:
src/web/Jordnaer/Pages/Groups/MyGroups.razorsrc/web/Jordnaer/Pages/Settings/Notifications.razor
🧬 Code graph analysis (1)
src/web/Jordnaer/Features/Notifications/NotificationSettingsService.cs (2)
src/shared/Jordnaer.Shared/Database/GroupMembership.cs (1)
GroupMembership(3-29)src/web/Jordnaer/Consumers/GroupPostCreatedConsumer.cs (1)
Task(20-88)
🔇 Additional comments (7)
src/web/Jordnaer/Pages/Groups/MyGroups.razor (1)
113-117: Inline styles are acceptable for component-specific styling.The CSS rule targets the component-specific class correctly. Consider moving to a shared stylesheet if this padding pattern is reused across multiple tab components.
src/web/Jordnaer/Pages/Settings/Notifications.razor (1)
151-175: Error handling pattern is well implemented.Good use of
OneOf.Switchfor explicit result handling with user-friendly Danish messages.src/web/Jordnaer/Features/Notifications/NotificationSettingsService.cs (4)
9-17: Well-designed interface with OneOf return types for explicit error handling.The interface follows the coding guidelines by using
OneOf<Success, NotFound>for operations that can fail, making error handling explicit at compile time.
21-30: Correct use ofAsNoTracking()for read-only query.As per coding guidelines, the read-only query uses
AsNoTracking()for better performance.
79-88: Efficient bulk update usingExecuteUpdateAsync.Good use of EF Core's bulk update capability to update all memberships in a single database operation rather than fetching and updating each entity individually.
90-100:Include(x => x.Group)may return detached entities with navigation property issues.Since
AsNoTracking()is used, the returnedGroupMembershipentities are detached from the context. The includedGroupnavigation property is populated but the context is disposed after the method returns. This is fine for read-only display but ensure the UI doesn't expect to track these entities.tests/web/Jordnaer.Tests/Chat/ChatNotificationServiceTests.cs (1)
109-111: No concern here—data persists correctly across context instances.The test uses Xunit's
[Collection]fixture pattern withSqlServerContainer, which manages a single persistent SQL Server database instance shared across all tests. Both_context(created at line 32) and contexts created by the factory (line 37) connect to the same database, so data seeded via_context.SaveChangesAsync()is immediately visible to the service when it calls the factory.Likely an incorrect or invalid review comment.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…elsPilgaard/Jordnaer into feature/notification-settings
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/web/Jordnaer/Pages/Settings/Notifications.razor (1)
89-96: Consider disabling bulk action buttons during save operations.The "Aktivér alle" and "Deaktivér alle" buttons are not disabled during the save operation, which could lead to multiple simultaneous requests if the user clicks repeatedly.
🔎 Proposed fix
Add a saving state for bulk operations:
private bool _isLoading = true; private bool _isSaving = false; +private bool _isBulkSaving = false; private ChatNotificationPreference _chatPreference = ChatNotificationPreference.FirstMessageOnly;Then disable the buttons:
<div class="mb-4 d-flex gap-2"> - <MudButton Size="Size.Small" Variant="Variant.Outlined" Color="Color.Primary" OnClick="EnableAllGroups"> + <MudButton Size="Size.Small" Variant="Variant.Outlined" Color="Color.Primary" OnClick="EnableAllGroups" Disabled="_isBulkSaving"> Aktivér alle </MudButton> - <MudButton Size="Size.Small" Variant="Variant.Outlined" Color="Color.Secondary" OnClick="DisableAllGroups"> + <MudButton Size="Size.Small" Variant="Variant.Outlined" Color="Color.Secondary" OnClick="DisableAllGroups" Disabled="_isBulkSaving"> Deaktivér alle </MudButton> </div>And wrap the bulk operations:
private async Task EnableAllGroups() { if (CurrentUser.Id is null) { return; } + _isBulkSaving = true; try { // ... existing code } catch (Exception) { Snackbar.Add("Der opstod en fejl. Prøv igen senere.", Severity.Error); } + finally + { + _isBulkSaving = false; + } }Apply the same pattern to
DisableAllGroups.Also applies to: 207-261
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/web/Jordnaer/Pages/Settings/Notifications.razor
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-29T18:32:51.546Z
Learnt from: NielsPilgaard
Repo: NielsPilgaard/Jordnaer PR: 481
File: src/web/Jordnaer/Pages/Groups/EditGroup.razor:137-139
Timestamp: 2025-12-29T18:32:51.546Z
Learning: In the Jordnaer codebase, Group.Categories is never null and is always initialized. When reviewing Razor components under src/web/Jordnaer/Pages, avoid adding null checks for Group.Categories and rely on its non-null guarantee. If a path could yield a null value due to data from an external source, add a guard only there, but otherwise remove redundant null checks for this property in these files.
Applied to files:
src/web/Jordnaer/Pages/Settings/Notifications.razor
🔇 Additional comments (7)
src/web/Jordnaer/Pages/Settings/Notifications.razor (7)
1-11: LGTM!The page setup is well-structured with appropriate authorization, service injections, and namespace imports.
12-19: LGTM!Good use of metadata for SEO and consistent styling with the application's design system.
21-74: LGTM!Well-designed chat notification section with clear option descriptions and proper loading state handling on the save button.
98-115: LGTM!The null guard on line 99 properly addresses the potential null reference issue. The group list rendering with individual toggles is well-implemented.
Minor note: Line 104 could use
@setting.Group!.Namesince theWhereclause already filters out null groups, but the current null-conditional approach is safe and doesn't cause issues.
127-149: LGTM!State initialization and
OnInitializedAsyncare well-implemented with proper null checks and try/finally to ensure the loading state is always reset.
151-175: LGTM!Clean implementation of the save method with proper loading state management, result handling via
Switch, and error handling.
177-205: LGTM!The method properly updates local state on success to keep the UI in sync with the server. Error handling is appropriate.
github-project-automation
Bot
moved this from In Progress
to Done
in Jordnaer Community Website
1 participant
No description provided.