Feature/group invite display#485
Conversation
WalkthroughAdds group-post publish events and consumer-driven email notifications, real-time pending-membership deltas via SignalR with admin notifications, pending-count APIs and UI badges, DTOs/models, DI/metrics wiring, and membership leave/re-application handling. Changes
Sequence Diagram(s)sequenceDiagram
%% Group post publish & email consumer flow
autonumber
participant Client as Web Client
participant Server as GroupPostService
participant DB as Database
participant MT as MassTransit
participant Consumer as GroupPostCreatedConsumer
participant Email as Email pipeline
Client->>Server: CreatePostAsync()
Server->>DB: Save GroupPost
DB-->>Server: Persisted
Server->>Server: Query Group & Author profile
alt Group & Author found
Server->>MT: Publish GroupPostCreated
MT->>Consumer: Deliver GroupPostCreated
Consumer->>DB: Query active group members (exclude author)
DB-->>Consumer: Member emails
Consumer->>MT: Publish SendEmail (Bcc, HTML)
MT->>Email: Deliver SendEmail
else Missing data
Server-->>Server: Log warning (no publish)
end
sequenceDiagram
%% Pending-membership delta notification via SignalR
autonumber
participant AdminUI as Admin client
participant Server as GroupService
participant DB as Database
participant Hub as GroupMembershipHub
participant AdminClients as Admin clients (group-admins-{id})
AdminUI->>Server: UpdateMembership(...)
Server->>DB: Update membership status
DB-->>Server: Saved
alt Pending status toggled
Server->>Hub: Notify MembershipStatusChanged(delta)
Hub->>AdminClients: MembershipStatusChanged(notification)
AdminClients->>AdminUI: Update badge/count UI
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 6
🧹 Nitpick comments (4)
src/web/Jordnaer/Pages/Groups/GroupDetails.razor (1)
280-286: Consider adding real-time updates for pending count.The pending count is fetched once during initialization. Unlike
TopBar.razorwhich subscribes toGroupMembershipSignalRClient.OnMembershipStatusChanged, this component won't reflect real-time changes if an admin approves/denies requests in another tab or if new requests arrive.This is acceptable for initial implementation, but you may want to consider subscribing to the SignalR updates for a consistent real-time experience across all admin-facing components.
src/shared/Jordnaer.Shared/Groups/GroupMembershipStatusChanged.cs (1)
1-7: LGTM!The DTO is appropriately designed for SignalR notifications. Using
PendingCountChangeas a delta allows efficient updates without requiring clients to re-fetch counts.Consider using a
recordfor immutability and concise syntax:🔎 Optional refactor to record
namespace Jordnaer.Shared; -public class GroupMembershipStatusChanged -{ - public required Guid GroupId { get; init; } - public int PendingCountChange { get; init; } -} +public record GroupMembershipStatusChanged(Guid GroupId, int PendingCountChange = 0);src/web/Jordnaer/Features/GroupPosts/GroupPostService.cs (1)
64-89: Consider logging when event publishing is skipped.If
grouporauthoris null, the event is silently skipped. This could mask data integrity issues where a post references a non-existent group or user profile.🔎 Proposed improvement
if (group is not null && author is not null) { await publishEndpoint.Publish(new GroupPostCreated { PostId = post.Id, GroupId = group.Id, GroupName = group.Name, AuthorId = author.Id, AuthorDisplayName = author.DisplayName, PostText = post.Text, CreatedUtc = post.CreatedUtc }, cancellationToken); } + else + { + // Log warning - this indicates a data integrity issue + // Consider injecting ILogger if not already available + }src/web/Jordnaer/Features/Dashboard/GroupsHub.razor (1)
165-199: LGTM with consideration for code duplication.The implementation correctly handles the SignalR lifecycle. However, this initialization pattern is duplicated in
TopBar.razor. Consider extracting a shared service or base component if this pattern spreads further.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
src/shared/Jordnaer.Shared/Groups/Events/GroupPostCreated.cssrc/shared/Jordnaer.Shared/Groups/GroupMembershipStatusChanged.cssrc/web/Jordnaer/Consumers/GroupPostCreatedConsumer.cssrc/web/Jordnaer/Features/Dashboard/GroupsHub.razorsrc/web/Jordnaer/Features/Email/EmailService.cssrc/web/Jordnaer/Features/GroupPosts/GroupPostService.cssrc/web/Jordnaer/Features/Groups/GroupMembershipHub.cssrc/web/Jordnaer/Features/Groups/GroupMembershipSignalRClient.cssrc/web/Jordnaer/Features/Groups/GroupService.cssrc/web/Jordnaer/Features/Groups/GroupSummaryCard.razorsrc/web/Jordnaer/Features/Groups/WebApplicationBuilderExtensions.cssrc/web/Jordnaer/Features/Posts/PostCardComponent.razorsrc/web/Jordnaer/Features/Sharing/SocialShareButtons.razorsrc/web/Jordnaer/Pages/Groups/GroupDetails.razorsrc/web/Jordnaer/Pages/Groups/MyGroups.razorsrc/web/Jordnaer/Pages/Shared/TopBar.razorsrc/web/Jordnaer/Program.cs
🧰 Additional context used
📓 Path-based instructions (4)
**/{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/GroupPosts/GroupPostService.cssrc/web/Jordnaer/Consumers/GroupPostCreatedConsumer.cssrc/web/Jordnaer/Features/Groups/WebApplicationBuilderExtensions.cssrc/web/Jordnaer/Features/Email/EmailService.cssrc/web/Jordnaer/Features/Groups/GroupService.cssrc/web/Jordnaer/Features/Groups/GroupMembershipSignalRClient.cssrc/web/Jordnaer/Features/Groups/GroupMembershipHub.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/GroupPosts/GroupPostService.cssrc/web/Jordnaer/Features/Groups/WebApplicationBuilderExtensions.cssrc/web/Jordnaer/Features/Email/EmailService.cssrc/web/Jordnaer/Features/Groups/GroupService.cssrc/web/Jordnaer/Features/Groups/GroupMembershipSignalRClient.cssrc/web/Jordnaer/Features/Groups/GroupMembershipHub.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/GroupPostCreatedConsumer.cs
**/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/Groups/WebApplicationBuilderExtensions.cs
🧠 Learnings (7)
📚 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/GroupDetails.razorsrc/web/Jordnaer/Pages/Shared/TopBar.razorsrc/web/Jordnaer/Pages/Groups/MyGroups.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 **/SignalR/**/*.cs : SignalR hubs must define an interface (e.g., `IChatHub`) for type-safe client method invocations
Applied to files:
src/web/Jordnaer/Program.cssrc/web/Jordnaer/Features/Groups/GroupService.cssrc/web/Jordnaer/Pages/Shared/TopBar.razorsrc/web/Jordnaer/Features/Groups/GroupMembershipSignalRClient.cssrc/web/Jordnaer/Features/Groups/GroupMembershipHub.cssrc/web/Jordnaer/Features/Dashboard/GroupsHub.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/Program.cssrc/web/Jordnaer/Features/Groups/WebApplicationBuilderExtensions.cssrc/web/Jordnaer/Features/Groups/GroupService.cssrc/web/Jordnaer/Pages/Shared/TopBar.razorsrc/web/Jordnaer/Features/Groups/GroupMembershipSignalRClient.cssrc/web/Jordnaer/Features/Groups/GroupMembershipHub.cssrc/web/Jordnaer/Features/Dashboard/GroupsHub.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 **/Consumers/**/*.cs : MassTransit consumers must be in the Jordnaer.Consumers namespace for auto-discovery
Applied to files:
src/web/Jordnaer/Features/GroupPosts/GroupPostService.cssrc/web/Jordnaer/Consumers/GroupPostCreatedConsumer.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/Features/GroupPosts/GroupPostService.cssrc/web/Jordnaer/Consumers/GroupPostCreatedConsumer.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 **/WebApplicationBuilderExtensions.cs : DI setup for features must be in `WebApplicationBuilderExtensions.cs` extension methods called from `Program.cs`
Applied to files:
src/web/Jordnaer/Features/Groups/WebApplicationBuilderExtensions.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/Authentication/**/*.cs : Use `UserCircuitHandler` for managing SignalR circuit state in authentication flows
Applied to files:
src/web/Jordnaer/Features/Groups/GroupService.cssrc/web/Jordnaer/Features/Groups/GroupMembershipSignalRClient.cssrc/web/Jordnaer/Features/Groups/GroupMembershipHub.cssrc/web/Jordnaer/Features/Dashboard/GroupsHub.razor
🧬 Code graph analysis (4)
src/web/Jordnaer/Features/Groups/WebApplicationBuilderExtensions.cs (1)
src/web/Jordnaer/Features/Groups/GroupMembershipSignalRClient.cs (1)
GroupMembershipSignalRClient(9-35)
src/web/Jordnaer/Features/Groups/GroupService.cs (3)
src/web/Jordnaer/Features/Authentication/CurrentUser.cs (1)
CurrentUser(7-25)src/web/Jordnaer/Extensions/LoggerExtensions.cs (1)
LogFunctionBegan(8-8)src/shared/Jordnaer.Shared/Groups/GroupMembershipStatusChanged.cs (1)
GroupMembershipStatusChanged(3-7)
src/web/Jordnaer/Features/Groups/GroupMembershipSignalRClient.cs (3)
src/web/Jordnaer/Features/Authentication/CurrentUser.cs (1)
CurrentUser(7-25)src/shared/Jordnaer.Shared/Groups/GroupMembershipStatusChanged.cs (1)
GroupMembershipStatusChanged(3-7)src/web/Jordnaer/Features/Groups/GroupMembershipHub.cs (4)
Task(9-9)Task(15-20)Task(22-36)Task(38-46)
src/web/Jordnaer/Features/Groups/GroupMembershipHub.cs (3)
src/shared/Jordnaer.Shared/Groups/GroupMembershipStatusChanged.cs (1)
GroupMembershipStatusChanged(3-7)src/web/Jordnaer/Features/Groups/GroupMembershipSignalRClient.cs (1)
Task(26-34)src/shared/Jordnaer.Shared/Extensions/ClaimsPrincipalExtensions.cs (1)
GetId(37-37)
🔇 Additional comments (14)
src/web/Jordnaer/Features/Posts/PostCardComponent.razor (1)
46-51: Icon colors added for social share menu items.The addition of explicit icon colors improves visual consistency. Note that the Bluesky color (
Color.Tertiary) differs from the Bluesky button inSocialShareButtons.razor(which usesColor.Info). Please verify this is intentional.src/web/Jordnaer/Features/Email/EmailService.cs (1)
78-85: LGTM!The email template changes correctly incorporate the group name in both the subject and body. The implementation follows the coding guidelines with proper use of
await usingfor the DbContext andAsNoTracking()for read-only queries.src/web/Jordnaer/Features/Groups/WebApplicationBuilderExtensions.cs (1)
8-8: LGTM!The
GroupMembershipSignalRClientis correctly registered as a scoped service in the extension method, following the established DI pattern. Based on learnings, this aligns with the guideline that DI setup for features must be inWebApplicationBuilderExtensions.cs.src/web/Jordnaer/Program.cs (1)
159-160: LGTM!The
GroupMembershipHubendpoint registration follows the established pattern used forChatHub. The path/hubs/group-membershipaligns with the client-sideGroupMembershipSignalRClientconfiguration.src/web/Jordnaer/Pages/Groups/GroupDetails.razor (1)
66-83: LGTM!The pending requests alert for admins is well-implemented with proper conditional rendering and correct pluralization logic. The UI provides a clear call-to-action to manage pending membership requests.
src/web/Jordnaer/Pages/Groups/MyGroups.razor (2)
45-46: LGTM!The
PendingRequestCountis correctly passed to eachGroupSummaryCardwith safe dictionary access usingGetValueOrDefault.
152-154: LGTM!The pending counts are fetched after loading groups, which is an efficient approach. The data flow is clear: groups are loaded first, then pending counts are retrieved for all groups the user can manage in a single call.
src/web/Jordnaer/Features/Groups/GroupSummaryCard.razor (1)
59-64: LGTM!The new
PendingRequestCountparameter with a sensible default of 0 and theMembersPageUrlcomputed property are well-implemented.src/shared/Jordnaer.Shared/Groups/Events/GroupPostCreated.cs (1)
1-12: No action needed. TheCreatedUtcfield is explicitly set from the persisted post entity (post.CreatedUtc) when the event is published on line 87 ofGroupPostService.cs, so the default value is not relied upon and there are no timing inconsistencies.src/web/Jordnaer/Features/Groups/GroupMembershipSignalRClient.cs (1)
9-34: LGTM!The client implementation correctly handles null
HubConnectionand connection state checks. The Remove/On pattern for event subscription prevents duplicate handlers.src/web/Jordnaer/Pages/Shared/TopBar.razor (1)
198-259: LGTM!The initialization flow correctly:
- Fetches initial pending counts
- Subscribes to real-time updates before starting the client (preventing race conditions)
- Starts the SignalR client
- Joins admin groups only when there are groups to join
The pending count clamping to 0 is a good defensive measure.
src/web/Jordnaer/Features/Groups/GroupService.cs (1)
183-236: LGTM - Well-structured pending count methods.The implementation correctly:
- Uses
AsNoTracking()for read-only queries- Combines admin group discovery and pending counts efficiently
- Returns empty dictionary early when no admin groups exist
src/web/Jordnaer/Features/Dashboard/GroupsHub.razor (1)
213-216: Approve disposal implementation.The
DisposeAsynccorrectly stops the SignalR client to prevent resource leaks.src/web/Jordnaer/Consumers/GroupPostCreatedConsumer.cs (1)
56-56: UseIOptions<AppOptions>instead ofNavigationManagerfor the base URL.
NavigationManageris a Blazor component service that requires HTTP context initialization. MassTransit consumers run in background context outside the HTTP request pipeline, makingNavigationManager.BaseUriunreliable or null. The project already providesAppOptionswith a configuredBaseUrlproperty—injectIOptions<AppOptions>and use itsValue.BaseUrlinstead.Similar code in
DeleteUserServicelogs an error whennavigationManager.BaseUriis empty, indicating this pattern has caused issues before.
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: 0
♻️ Duplicate comments (2)
src/web/Jordnaer/Features/Groups/GroupMembershipHub.cs (1)
42-78: Authorization validation correctly implemented.The
JoinAdminGroupsmethod now properly validates that the user is an admin or owner before allowing them to join admin notification groups. The implementation correctly:
- Validates user authentication (lines 44-49)
- Queries the database to verify admin/owner status (lines 56-63)
- Logs warnings for unauthorized attempts (lines 65-70)
- Only joins validated groups (lines 72-75)
The code follows coding guidelines by using
await using var context = await contextFactory.CreateDbContextAsync()andAsNoTracking()for read-only queries.src/web/Jordnaer/Features/Groups/GroupService.cs (1)
275-320: SignalR notification correctly separated from DB transaction.The implementation properly addresses the concern about notification failures affecting the response:
- The database transaction completes first (lines 288-296)
- SignalR notification occurs after
SaveChangesAsyncsucceeds (lines 298-320)- Notification failures are caught separately and logged without changing the success response (lines 313-319)
- Line 299's comment clearly documents this design decision
The code correctly follows coding guidelines by using
IHubContextto broadcast to thegroup-admins-{groupId}SignalR group.
🧹 Nitpick comments (3)
tests/web/Jordnaer.Tests/Groups/GroupServiceTests.cs (1)
63-64: Consider adding test coverage for new pending membership methods.The mock setup for
IHubContext<GroupMembershipHub, IGroupMembershipHub>is correct. However, the new public methodsGetPendingMembershipCountAsyncandGetPendingMembershipCountsForUserAsyncintroduced inGroupServicelack test coverage.src/web/Jordnaer/Features/Groups/GroupService.cs (1)
204-236: Consider replacing Debug.Assert with a guard clause.The method uses
Debug.Assert(line 206) to verifycurrentUser.Idis not null. Since this is a public method, a guard clause with an early return would be more appropriate than a debug-only assertion, which is stripped in release builds.🔎 Proposed guard clause
public async Task<Dictionary<Guid, int>> GetPendingMembershipCountsForUserAsync(CancellationToken cancellationToken = default) { - Debug.Assert(currentUser.Id is not null, "Current user must be set when fetching pending counts."); - logger.LogFunctionBegan(); + if (currentUser.Id is null) + { + return new Dictionary<Guid, int>(); + } + await using var context = await contextFactory.CreateDbContextAsync(cancellationToken);The database queries correctly follow coding guidelines by using
await using var contextandAsNoTracking()for read-only operations.src/web/Jordnaer/Consumers/GroupPostCreatedConsumer.cs (1)
33-50: Consider optimizing the query pattern for better performance.The current pattern uses
activeMembers.Any(userId => userId == user.Id), which creates a correlated subquery. For large groups, this might not be optimal.🔎 Proposed optimization using a join
-// Get all active members excluding the post author -var activeMembers = context.GroupMemberships - .AsNoTracking() - .Where(x => x.GroupId == message.GroupId && - x.MembershipStatus == MembershipStatus.Active && - x.UserProfileId != message.AuthorId) - .Select(x => x.UserProfileId); - -// Get their email addresses -var emails = await context.Users - .AsNoTracking() - .Where(user => activeMembers.Any(userId => userId == user.Id) && - !string.IsNullOrEmpty(user.Email)) - .Select(user => new EmailRecipient - { - Email = user.Email!, - DisplayName = user.UserName - }) - .ToListAsync(consumeContext.CancellationToken); +// Get email addresses for all active members excluding the post author +var emails = await (from membership in context.GroupMemberships + join user in context.Users on membership.UserProfileId equals user.Id + where membership.GroupId == message.GroupId && + membership.MembershipStatus == MembershipStatus.Active && + membership.UserProfileId != message.AuthorId && + !string.IsNullOrEmpty(user.Email) + select new EmailRecipient + { + Email = user.Email!, + DisplayName = user.UserName + }) + .AsNoTracking() + .ToListAsync(consumeContext.CancellationToken);
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/web/Jordnaer/Consumers/GroupPostCreatedConsumer.cssrc/web/Jordnaer/Features/GroupPosts/GroupPostService.cssrc/web/Jordnaer/Features/Groups/GroupMembershipHub.cssrc/web/Jordnaer/Features/Groups/GroupService.cssrc/web/Jordnaer/Features/Metrics/JordnaerMetrics.cstests/web/Jordnaer.Tests/Groups/GroupServiceTests.cs
🧰 Additional context used
📓 Path-based instructions (3)
**/{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/GroupPosts/GroupPostService.cssrc/web/Jordnaer/Features/Groups/GroupMembershipHub.cssrc/web/Jordnaer/Features/Metrics/JordnaerMetrics.cssrc/web/Jordnaer/Features/Groups/GroupService.cssrc/web/Jordnaer/Consumers/GroupPostCreatedConsumer.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/GroupPosts/GroupPostService.cssrc/web/Jordnaer/Features/Groups/GroupMembershipHub.cssrc/web/Jordnaer/Features/Metrics/JordnaerMetrics.cssrc/web/Jordnaer/Features/Groups/GroupService.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/GroupPostCreatedConsumer.cs
🧠 Learnings (5)
📚 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/Features/GroupPosts/GroupPostService.cstests/web/Jordnaer.Tests/Groups/GroupServiceTests.cssrc/web/Jordnaer/Consumers/GroupPostCreatedConsumer.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/Features/GroupPosts/GroupPostService.cstests/web/Jordnaer.Tests/Groups/GroupServiceTests.cssrc/web/Jordnaer/Features/Metrics/JordnaerMetrics.cssrc/web/Jordnaer/Consumers/GroupPostCreatedConsumer.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 `IHubContext` for broadcasting messages to SignalR clients from outside the hub
Applied to files:
tests/web/Jordnaer.Tests/Groups/GroupServiceTests.cssrc/web/Jordnaer/Features/Groups/GroupMembershipHub.cssrc/web/Jordnaer/Features/Groups/GroupService.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:
tests/web/Jordnaer.Tests/Groups/GroupServiceTests.cssrc/web/Jordnaer/Features/Groups/GroupMembershipHub.cssrc/web/Jordnaer/Features/Groups/GroupService.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/Authentication/**/*.cs : Use `UserCircuitHandler` for managing SignalR circuit state in authentication flows
Applied to files:
tests/web/Jordnaer.Tests/Groups/GroupServiceTests.cssrc/web/Jordnaer/Features/Groups/GroupMembershipHub.cssrc/web/Jordnaer/Features/Groups/GroupService.cs
🧬 Code graph analysis (4)
src/web/Jordnaer/Features/GroupPosts/GroupPostService.cs (1)
src/shared/Jordnaer.Shared/Groups/Events/GroupPostCreated.cs (1)
GroupPostCreated(3-12)
src/web/Jordnaer/Features/Groups/GroupMembershipHub.cs (2)
src/shared/Jordnaer.Shared/Groups/GroupMembershipStatusChanged.cs (1)
GroupMembershipStatusChanged(3-7)src/shared/Jordnaer.Shared/Extensions/ClaimsPrincipalExtensions.cs (1)
GetId(37-37)
src/web/Jordnaer/Features/Groups/GroupService.cs (2)
src/web/Jordnaer/Extensions/LoggerExtensions.cs (1)
LogFunctionBegan(8-8)src/shared/Jordnaer.Shared/Groups/GroupMembershipStatusChanged.cs (1)
GroupMembershipStatusChanged(3-7)
src/web/Jordnaer/Consumers/GroupPostCreatedConsumer.cs (5)
src/shared/Jordnaer.Shared/Groups/Events/GroupPostCreated.cs (1)
GroupPostCreated(3-12)src/web/Jordnaer/Features/Metrics/JordnaerMetrics.cs (1)
JordnaerMetrics(6-57)src/web/Jordnaer/Features/Email/EmailRecipient.cs (1)
EmailRecipient(6-78)src/web/Jordnaer/Features/Email/SendEmail.cs (1)
SendEmail(5-33)src/web/Jordnaer/Features/Email/EmailConstants.cs (1)
EmailConstants(3-15)
🔇 Additional comments (6)
src/web/Jordnaer/Features/Groups/GroupService.cs (2)
21-22: IHubContext dependency correctly added.The constructor now includes
IHubContext<GroupMembershipHub, IGroupMembershipHub>for broadcasting SignalR messages from the service, following the coding guidelines for usingIHubContextoutside hub classes.
183-198: LGTM!The method correctly uses
await using var context = await contextFactory.CreateDbContextAsync()andAsNoTracking()for the read-only query, following the coding guidelines.src/web/Jordnaer/Features/Metrics/JordnaerMetrics.cs (1)
51-56: LGTM!The new consumer metrics follow the established naming convention and lifecycle pattern (received/succeeded/failed), which aligns well with the GroupPostCreatedConsumer implementation.
src/web/Jordnaer/Features/GroupPosts/GroupPostService.cs (1)
65-99: LGTM!The event publishing logic is well-implemented:
- Correctly queries for group and author data after persisting the post
- Uses
AsNoTracking()for read-only queries per guidelines- Includes defensive logging when data is unexpectedly missing
- Properly publishes the GroupPostCreated event for downstream consumers
src/web/Jordnaer/Consumers/GroupPostCreatedConsumer.cs (2)
19-84: Well-implemented consumer with proper metrics and error handling.The consumer correctly:
- Tracks metrics at all lifecycle stages (received/succeeded/failed) per guidelines
- Uses
AsNoTracking()for read-only queries- Handles exceptions without rethrowing, appropriate for async notifications
- HTML-encodes all user inputs (lines 97-104) to prevent XSS
13-17: NavigationManager should not be used in MassTransit consumers.NavigationManager is Blazor-scoped and depends on an active HTTP request context. MassTransit consumers run as background services without this context, making
NavigationManager.BaseUriat line 62 unreliable or unavailable at runtime.Instead, inject
IOptions<ApplicationSettings>(or similar configuration) with aBaseUrlproperty to construct URLs in background services.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/web/Jordnaer/Features/Sharing/SocialShareButtons.razor (1)
7-7: Brand colors applied, consistency achieved.The switch from
Colorconstants to inline hex values ensures brand-accurate colors for both social buttons, which addresses the previous consistency concern. Using exact brand colors (#1877F2 for Facebook, #1185FE for Bluesky) is appropriate for external platform integrations.If future theming flexibility is desired, consider defining these brand colors as CSS custom properties or theme palette extensions rather than inline styles, though the current approach is perfectly valid for fixed brand requirements.
Also applies to: 12-12
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/web/Jordnaer/Features/Posts/PostCardComponent.razorsrc/web/Jordnaer/Features/Sharing/SocialShareButtons.razor
🚧 Files skipped from review as they are similar to previous changes (1)
- src/web/Jordnaer/Features/Posts/PostCardComponent.razor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/web/Jordnaer/Pages/Groups/GroupDetails.razor (2)
317-328: Consider updating the member list on successful leave.After leaving the group, the
_groupMemberslist still contains the current user until page refresh. Consider removing the user from_groupMembersfor immediate visual feedback.🔎 Proposed enhancement
leaveResponse.Switch( _ => { Snackbar.Add("Du har forladt gruppen", Severity.Success); _isMember = false; + _groupMembers = _groupMembers?.Where(m => m.Id != _currentUser?.Id).ToList(); StateHasChanged(); }, error => Snackbar.Add(error.Value, Severity.Warning));
269-276: Duplicate query can be consolidated.Both authenticated and unauthenticated paths call
GetGroupMembersWithRolesByPredicateAsyncwith identical predicates. Consider moving the call after the auth check to avoid repetition.🔎 Proposed refactor
- if (_currentUser is null) - { - _groupMembers = await GroupService.GetGroupMembersWithRolesByPredicateAsync(x => x.GroupId == _group.Id && - x.MembershipStatus == MembershipStatus.Active); - _isLoading = false; - return; - } - - _groupMembers = await GroupService.GetGroupMembersWithRolesByPredicateAsync(x => x.GroupId == _group.Id && - x.MembershipStatus == MembershipStatus.Active); + _groupMembers = await GroupService.GetGroupMembersWithRolesByPredicateAsync(x => x.GroupId == _group.Id && + x.MembershipStatus == MembershipStatus.Active); + + if (_currentUser is null) + { + _isLoading = false; + return; + }src/web/Jordnaer/Features/Dashboard/GroupsHub.razor (1)
173-191: Unused variableadminGroupIds.The
adminGroupIdsvariable is extracted frompendingCounts.Keysbut never used. This appears to be leftover from development.🔎 Proposed fix
// Get pending counts and admin group IDs var pendingCounts = await GroupService.GetPendingMembershipCountsForUserAsync(); _totalPendingRequests = pendingCounts.Values.Sum(); - var adminGroupIds = pendingCounts.Keys.ToList(); // Subscribe to group membership status changes
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/shared/Jordnaer.Shared/Database/Enums/MembershipStatus.cssrc/shared/Jordnaer.Shared/Groups/GroupMemberSlim.cssrc/web/Jordnaer/Components/CookieBanner.razorsrc/web/Jordnaer/Features/Dashboard/GroupsHub.razorsrc/web/Jordnaer/Features/Groups/GroupMemberListComponent.razorsrc/web/Jordnaer/Features/Groups/GroupMembershipHub.cssrc/web/Jordnaer/Features/Groups/GroupMembershipNotificationService.cssrc/web/Jordnaer/Features/Groups/GroupMembershipSignalRClient.cssrc/web/Jordnaer/Features/Groups/GroupService.cssrc/web/Jordnaer/Features/Groups/WebApplicationBuilderExtensions.cssrc/web/Jordnaer/Features/Membership/MembershipService.cssrc/web/Jordnaer/Pages/Groups/GroupDetails.razorsrc/web/Jordnaer/Pages/Shared/TopBar.razor
🧰 Additional context used
📓 Path-based instructions (3)
**/{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/Groups/GroupMembershipSignalRClient.cssrc/web/Jordnaer/Features/Groups/GroupMembershipHub.cssrc/web/Jordnaer/Features/Groups/GroupMembershipNotificationService.cssrc/web/Jordnaer/Features/Groups/WebApplicationBuilderExtensions.cssrc/web/Jordnaer/Features/Groups/GroupService.cssrc/web/Jordnaer/Features/Membership/MembershipService.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/Groups/GroupMembershipSignalRClient.cssrc/web/Jordnaer/Features/Groups/GroupMembershipHub.cssrc/web/Jordnaer/Features/Groups/GroupMembershipNotificationService.cssrc/web/Jordnaer/Features/Groups/WebApplicationBuilderExtensions.cssrc/web/Jordnaer/Features/Groups/GroupService.cssrc/web/Jordnaer/Features/Membership/MembershipService.cs
**/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/Groups/WebApplicationBuilderExtensions.cs
🧠 Learnings (6)
📚 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/Features/Groups/GroupMembershipSignalRClient.cssrc/web/Jordnaer/Features/Groups/GroupMembershipHub.cssrc/web/Jordnaer/Features/Dashboard/GroupsHub.razorsrc/web/Jordnaer/Features/Groups/GroupMembershipNotificationService.cssrc/web/Jordnaer/Pages/Shared/TopBar.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/Authentication/**/*.cs : Use `UserCircuitHandler` for managing SignalR circuit state in authentication flows
Applied to files:
src/web/Jordnaer/Features/Groups/GroupMembershipSignalRClient.cssrc/web/Jordnaer/Features/Groups/GroupMembershipHub.cssrc/web/Jordnaer/Features/Groups/GroupService.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 `IHubContext` for broadcasting messages to SignalR clients from outside the hub
Applied to files:
src/web/Jordnaer/Features/Groups/GroupMembershipSignalRClient.cssrc/web/Jordnaer/Features/Groups/GroupMembershipHub.cssrc/web/Jordnaer/Features/Dashboard/GroupsHub.razorsrc/web/Jordnaer/Features/Groups/GroupMembershipNotificationService.cssrc/web/Jordnaer/Features/Groups/WebApplicationBuilderExtensions.cssrc/web/Jordnaer/Pages/Shared/TopBar.razor
📚 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/GroupDetails.razorsrc/web/Jordnaer/Pages/Shared/TopBar.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 **/WebApplicationBuilderExtensions.cs : DI setup for features must be in `WebApplicationBuilderExtensions.cs` extension methods called from `Program.cs`
Applied to files:
src/web/Jordnaer/Features/Groups/WebApplicationBuilderExtensions.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/Features/Membership/MembershipService.cs
🧬 Code graph analysis (6)
src/web/Jordnaer/Features/Groups/GroupMembershipSignalRClient.cs (3)
src/web/Jordnaer/Features/Authentication/CurrentUser.cs (1)
CurrentUser(7-25)src/shared/Jordnaer.Shared/Groups/GroupMembershipStatusChanged.cs (1)
GroupMembershipStatusChanged(3-7)src/web/Jordnaer/Features/Groups/GroupMembershipHub.cs (3)
Task(9-9)Task(15-20)Task(22-36)
src/web/Jordnaer/Features/Groups/GroupMembershipHub.cs (2)
src/shared/Jordnaer.Shared/Groups/GroupMembershipStatusChanged.cs (1)
GroupMembershipStatusChanged(3-7)src/shared/Jordnaer.Shared/Extensions/ClaimsPrincipalExtensions.cs (1)
GetId(37-37)
src/web/Jordnaer/Features/Groups/GroupMembershipNotificationService.cs (2)
src/web/Jordnaer/Features/Groups/GroupMembershipHub.cs (3)
Task(9-9)Task(15-20)Task(22-36)src/shared/Jordnaer.Shared/Groups/GroupMembershipStatusChanged.cs (1)
GroupMembershipStatusChanged(3-7)
src/web/Jordnaer/Features/Groups/WebApplicationBuilderExtensions.cs (2)
src/web/Jordnaer/Features/Groups/GroupMembershipSignalRClient.cs (1)
GroupMembershipSignalRClient(9-25)src/web/Jordnaer/Features/Groups/GroupMembershipNotificationService.cs (1)
GroupMembershipNotificationService(22-74)
src/web/Jordnaer/Features/Groups/GroupService.cs (5)
src/web/Jordnaer/Features/Authentication/CurrentUser.cs (1)
CurrentUser(7-25)src/web/Jordnaer/Features/Groups/GroupMembershipHub.cs (3)
Task(9-9)Task(15-20)Task(22-36)src/shared/Jordnaer.Shared/Groups/GroupMemberSlim.cs (1)
GroupMemberSlim(3-18)src/shared/Jordnaer.Shared/Database/GroupMembership.cs (1)
GroupMembership(3-23)src/web/Jordnaer/Extensions/LoggerExtensions.cs (1)
LogFunctionBegan(8-8)
src/web/Jordnaer/Features/Membership/MembershipService.cs (2)
src/web/Jordnaer/Extensions/LoggerExtensions.cs (3)
Error(12-17)Error(19-24)LogFunctionBegan(8-8)src/web/Jordnaer/Features/Authentication/CurrentUser.cs (1)
CurrentUser(7-25)
🔇 Additional comments (19)
src/web/Jordnaer/Features/Groups/GroupMembershipHub.cs (1)
7-37: LGTM! Clean SignalR hub implementation.The hub correctly defines a type-safe interface per best practices, applies authorization, and includes appropriate connection lifecycle logging. The design as a server-to-client notification hub (without client-invoked methods) is appropriate for this use case.
Based on learnings, SignalR hubs should define interfaces for type-safe client invocations, which is properly implemented here.
src/web/Jordnaer/Features/Groups/GroupMembershipNotificationService.cs (1)
8-74: LGTM! Well-structured notification service.The implementation correctly follows all coding guidelines:
- Uses
await using var contextfor DbContext lifecycle- Applies
AsNoTracking()for the read-only admin query- Broadcasts via
IHubContextas specified in the guidelines- Appropriately swallows notification exceptions to prevent impacting core operations
The query logic correctly targets active admins and owners for notifications.
As per coding guidelines, this correctly uses
IHubContextfor broadcasting from outside the hub.src/web/Jordnaer/Features/Groups/WebApplicationBuilderExtensions.cs (1)
8-9: LGTM! Proper DI registration.The service registrations are correctly placed in
WebApplicationBuilderExtensionsper coding guidelines, and scoped lifetime is appropriate for these services.As per coding guidelines, DI setup is properly located in WebApplicationBuilderExtensions.cs.
src/web/Jordnaer/Features/Groups/GroupMemberListComponent.razor (1)
30-40: LGTM! Clear role visualization.The updated parameter type aligns with the new
GroupMemberSlimDTO, and the badge rendering logic appropriately prioritizes Owner designation over Admin permission. The visual distinction (filled primary chip for Owner, outlined info chip for Admin) provides good UX clarity.src/shared/Jordnaer.Shared/Groups/GroupMemberSlim.cs (1)
1-18: LGTM! Well-designed immutable DTO.The DTO follows best practices with required init-only properties for immutability. The nullable
UserNameproperty accommodates users who may not have usernames set, while theToString()override provides a sensible default representation.src/web/Jordnaer/Pages/Groups/GroupDetails.razor (2)
54-60: LGTM! Leave group button correctly conditioned.The button visibility logic properly excludes owners and only shows for active members. The
_currentUserIsOwnercheck aligns with the backend validation inLeaveMembershipthat prevents owners from leaving.
74-91: LGTM! Pending requests alert for admins.Clean implementation with proper pluralization for Danish ("er" suffix for plural). The conditional rendering based on
_currentUserIsAdminand count > 0 is appropriate.src/web/Jordnaer/Features/Groups/GroupMembershipSignalRClient.cs (1)
9-24: LGTM! SignalR client follows established patterns.The implementation correctly:
- Inherits from
AuthenticatedSignalRClientBase- Uses
RemovebeforeOnto prevent duplicate handler subscriptions- Handles null
HubConnectiondefensively- References
IGroupMembershipHub.MembershipStatusChangedfor type-safe method namingBased on learnings, this aligns with the guideline to use
IHubContextfor broadcasting from outside the hub, and the client properly connects to the/hubs/group-membershipendpoint.src/web/Jordnaer/Pages/Shared/TopBar.razor (3)
24-41: LGTM! Groups button with pending count badge.Good use of
AuthorizeViewto differentiate between authenticated and non-authenticated states. The badge displays only when count > 0 with appropriate warning color.
239-251: LGTM! Real-time pending count updates with proper bounds checking.The subscription follows the same pattern as the chat message handling. The bounds check preventing negative counts is a good defensive measure.
301-306: LGTM! Proper cleanup in DisposeAsync.Both SignalR clients are stopped during disposal, preventing resource leaks.
src/web/Jordnaer/Features/Dashboard/GroupsHub.razor (2)
19-36: LGTM! Pending requests alert consistent with GroupDetails.The alert styling and pluralization logic matches the pattern used in
GroupDetails.razor, providing a consistent UX across pages.
207-210: LGTM! Proper SignalR client disposal.The
DisposeAsynccorrectly stops the SignalR client to prevent resource leaks.src/web/Jordnaer/Features/Membership/MembershipService.cs (2)
52-65: Good enhancement: Re-application support for Left/Rejected users.This allows users who previously left or were rejected to request membership again, with proper status reset and admin notification.
98-139: LGTM! LeaveMembership implementation is well-structured.The method correctly:
- Uses
await using var contextper coding guidelines- Validates membership existence and active status
- Prevents owners from leaving (must transfer ownership first)
- Uses soft delete pattern with
MembershipStatus.Left- Returns appropriate error messages in Danish
src/web/Jordnaer/Features/Groups/GroupService.cs (4)
72-75: LGTM! Added null guard for unauthenticated users.Early return with empty list for null
currentUser.Idprevents potential NullReferenceException and avoids unnecessary database queries.
138-160: LGTM! New method follows established patterns.
GetGroupMembersWithRolesByPredicateAsynccorrectly:
- Uses
AsNoTracking()for read-only queries per coding guidelines- Uses
await using var contextper coding guidelines- Follows the same structure as
GetGroupMembersByPredicateAsync- Projects to the new
GroupMemberSlimtype including role information
211-267: LGTM! Pending count methods are well-implemented.Both methods:
- Use
AsNoTracking()for read-only queries- Handle null
currentUser.IdappropriatelyGetPendingMembershipCountsForUserAsyncefficiently queries only groups where user is admin/owner before fetching pending counts
330-336: Good: Notification decoupled from DB transaction.The SignalR notification is correctly placed outside the try-catch block that handles the database operation, addressing the previous review feedback. This ensures notification failures don't affect the success response when the DB update succeeds.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/web/Jordnaer/Features/Groups/GroupService.cs (1)
233-268: Consider consolidating into a single query for better performance.The current implementation uses two separate queries: one to fetch admin groups (lines 245-251) and another to count pending memberships (lines 259-265). This could potentially be optimized into a single query, especially if a user is an admin of many groups.
💡 Potential optimization with a single query
- // Get all groups where current user is admin or owner - var adminGroupIds = await context.GroupMemberships - .AsNoTracking() - .Where(x => x.UserProfileId == currentUser.Id && - (x.PermissionLevel == PermissionLevel.Admin || - x.OwnershipLevel == OwnershipLevel.Owner)) - .Select(x => x.GroupId) - .ToListAsync(cancellationToken); - - if (adminGroupIds.Count == 0) - { - return new Dictionary<Guid, int>(); - } - - // Get pending counts for those groups in a single query - var pendingCounts = await context.GroupMemberships + // Get pending counts for groups where current user is admin or owner in a single query + var adminGroupIds = await context.GroupMemberships .AsNoTracking() - .Where(x => adminGroupIds.Contains(x.GroupId) && - x.MembershipStatus == MembershipStatus.PendingApprovalFromGroup) + .Where(x => x.UserProfileId == currentUser.Id && + (x.PermissionLevel == PermissionLevel.Admin || + x.OwnershipLevel == OwnershipLevel.Owner)) + .Select(x => x.GroupId) + .ToListAsync(cancellationToken); + + if (adminGroupIds.Count == 0) + { + return new Dictionary<Guid, int>(); + } + + var pendingCounts = await context.GroupMemberships + .AsNoTracking() + .Where(x => adminGroupIds.Contains(x.GroupId) && + x.MembershipStatus == MembershipStatus.PendingApprovalFromGroup) .GroupBy(x => x.GroupId) .Select(g => new { GroupId = g.Key, Count = g.Count() }) .ToDictionaryAsync(x => x.GroupId, x => x.Count, cancellationToken); + + return pendingCounts;Note: The optimization benefit would be marginal for most cases. Consider this only if performance profiling indicates this is a bottleneck.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/web/Jordnaer/Components/CookieBanner.razorsrc/web/Jordnaer/Features/Groups/GroupService.cssrc/web/Jordnaer/Features/Membership/MembershipService.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/web/Jordnaer/Components/CookieBanner.razor
🧰 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/Membership/MembershipService.cssrc/web/Jordnaer/Features/Groups/GroupService.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/Membership/MembershipService.cssrc/web/Jordnaer/Features/Groups/GroupService.cs
🧠 Learnings (3)
📓 Common learnings
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
📚 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/Features/Membership/MembershipService.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/Authentication/**/*.cs : Use `UserCircuitHandler` for managing SignalR circuit state in authentication flows
Applied to files:
src/web/Jordnaer/Features/Groups/GroupService.cs
🧬 Code graph analysis (1)
src/web/Jordnaer/Features/Membership/MembershipService.cs (2)
src/web/Jordnaer/Extensions/LoggerExtensions.cs (4)
Error(12-17)Error(19-24)LogFunctionBegan(8-8)LogException(10-10)src/web/Jordnaer/Features/Authentication/CurrentUser.cs (1)
CurrentUser(7-25)
🔇 Additional comments (11)
src/web/Jordnaer/Features/Groups/GroupService.cs (6)
20-21: LGTM! Dependency injection for notification service added.The constructor now includes
IGroupMembershipNotificationServicewhich is used for notifying admins of pending membership changes. This is a clean dependency injection pattern.
72-75: Good defensive guard against null user ID.Early return with an empty list when
currentUser.Idis null prevents potential null reference issues downstream and avoids unnecessary database queries.
81-82: LGTM! Correctly excludes Left memberships.The enhanced filtering now excludes both
RejectedandLeftmembership statuses, which makes sense for displaying the user's active group list.
139-161: LGTM! New method follows coding guidelines.The implementation correctly uses
AsNoTracking()for the read-only query andawait using var contextfor the scoped DbContext instance, as per coding guidelines.
215-227: LGTM! Pending count query is straightforward and efficient.The method correctly counts pending memberships for a specific group using
AsNoTracking()and follows the coding guidelines.
331-337: No action required—notification service handles exceptions internally.The
NotifyAdminsOfPendingCountChangeAsyncmethod inGroupMembershipNotificationServicealready wraps its logic in a try-catch block (lines 58-64) that logs errors but does not rethrow exceptions. This prevents notification failures from propagating to the caller, so the method returns a successful response even if the notification fails. No additional try-catch is needed inGroupService.Likely an incorrect or invalid review comment.
src/web/Jordnaer/Features/Membership/MembershipService.cs (5)
27-28: LGTM! Notification service dependency added.The constructor now includes
IGroupMembershipNotificationServicefor broadcasting membership changes to admins. Clean dependency injection.
19-21: LGTM! New interface method for leaving groups.The interface correctly declares the new
LeaveMembershipmethod with appropriate return types using OneOf for explicit error handling.
52-81: Excellent resilient notification handling!The re-application logic correctly wraps both email and SignalR notifications in individual try-catch blocks, ensuring that notification failures don't cause the membership request to fail. This addresses the previous review concern about notification failures masking DB success.
100-118: LGTM! Consistent notification error handling.The notification handling for new memberships follows the same fire-and-forget pattern with try-catch blocks, ensuring reliability and consistent user experience.
129-170: LeaveMembership implementation looks solid.The method correctly:
- Uses
await using var contextas per coding guidelines ✓- Validates membership existence and active status
- Prevents owners from leaving (requiring ownership transfer first)
- Implements soft delete by setting status to
LeftOne question: Should pending members be able to cancel their membership requests? Currently, only
Activemembers can leave (line 148). If a user has a pending request (MembershipStatus.PendingApprovalFromGroup), they cannot cancel it using this method.Is it intentional that users with pending membership requests cannot cancel them via
LeaveMembership? If cancellation should be supported, you might want to either:
- Allow pending users to call this method (and notify admins of the pending count change), or
- Create a separate
CancelMembershipRequestmethod
github-project-automation
Bot
moved this from In Progress
to Done
in Jordnaer Community Website
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/web/Jordnaer.Tests/Groups/GroupServiceTests.cs (1)
62-63: Constructor update is correct; consider test coverage for new functionality.The addition of
IGroupMembershipNotificationServicesubstitute correctly aligns with the updatedGroupServiceconstructor. The unconfigured mock is acceptable for these existing tests.However, the AI summary indicates that
GroupServicenow has new methods (GetPendingMembershipCountAsync,GetPendingMembershipCountsForUserAsync,GetGroupMembersWithRolesByPredicateAsync) and new notification behavior for pending membership changes, but there's no test coverage for these features in this file.💡 Optional: Extract mock to field for better testability
Consider extracting the notification service mock to a private field to allow individual tests to configure or verify its behavior:
public class GroupServiceTests : IAsyncLifetime { private readonly GroupService _groupService; private readonly IDbContextFactory<JordnaerDbContext> _contextFactory = Substitute.For<IDbContextFactory<JordnaerDbContext>>(); private readonly JordnaerDbContext _context; + private readonly IGroupMembershipNotificationService _notificationService = Substitute.For<IGroupMembershipNotificationService>(); private readonly string _userProfileId; // ... constructor ... _groupService = new GroupService(_contextFactory, Substitute.For<ILogger<GroupService>>(), Substitute.For<IDiagnosticContext>(), new CurrentUser { User = new ClaimsPrincipal( new ClaimsIdentity( [new Claim(ClaimTypes.NameIdentifier, _userProfileId)] )) }, - Substitute.For<IGroupMembershipNotificationService>()); + _notificationService);This would enable future tests to verify notification calls.
Would you like me to generate test cases for the new
GroupServicemethods and notification behavior, or open an issue to track this task?
1 participant
No description provided.