Improve Boilerplate claim retrieval (#10933) #10934
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 WalkthroughThe changes overhaul the user claims retrieval process in the boilerplate project template for improved performance. They introduce a single-query, cached approach to fetching user claims, update the claims principal factory to use this method, and refactor related SignalR diagnostic log retrieval to use user session IDs instead of flexible queries. Changes
Sequence Diagram(s)sequenceDiagram
participant UsersPage
participant AppHub (Server)
participant SignalR Client
participant DiagnosticModal
UsersPage->>AppHub (Server): GetUserSessionLogs(userSessionId)
AppHub (Server)->>DB: Query SignalRConnectionId for userSessionId
AppHub (Server)->>SignalR Client: UPLOAD_DIAGNOSTIC_LOGGER_STORE (via SignalR)
SignalR Client-->>AppHub (Server): DiagnosticLogDto[]
AppHub (Server)-->>UsersPage: DiagnosticLogDto[]
UsersPage->>DiagnosticModal: Show logs
sequenceDiagram
participant AppUserClaimsPrincipalFactory
participant UserClaimsService
participant DB
AppUserClaimsPrincipalFactory->>UserClaimsService: GetAllUserClaims(userId)
UserClaimsService->>DB: Query user claims, role claims, role names (single query)
DB-->>UserClaimsService: Claims[]
UserClaimsService-->>AppUserClaimsPrincipalFactory: Claims[]
AppUserClaimsPrincipalFactory->>AppUserClaimsPrincipalFactory: Build ClaimsIdentity
Assessment against linked issues
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This PR improves the boilerplate claim retrieval by refactoring SignalR log retrieval methods and consolidating user claim queries. The key changes include:
- Replacing the GetUserDiagnosticLogs method with a streamlined GetUserSessionLogs that uses a session identifier.
- Refactoring the user claims retrieval by introducing an in-memory cache and using a consolidated query in UserClaimsService.
- Adjusting SignalR functionality on the client side to match the updated API for log retrieval.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| AppHub.cs | Updated to fetch logs by user session identifier and simplified connection handling. |
| UserClaimsService.cs | Added an in-memory cache for claims and replaced inline queries with a consolidated async query. |
| AppUserClaimsPrincipalFactory.cs | Modified constructor dependency and refactored claims generation to use the new UserClaimsService method. |
| UsersPage.razor.cs / UsersPage.razor | Integrated conditional SignalR-based log fetching on the client. |
| AppDiagnosticModal.razor.Utils.cs / AppDiagnosticModal.razor | Removed legacy SignalR log reading code for diagnostic modal display. |
...ate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Services/Identity/UserClaimsService.cs
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/Authorized/Management/UsersPage.razor (1)
178-182: Consider adding accessibility attributes for better UX.The new diagnostic logs button is well-positioned and uses consistent error handling. However, consider improving accessibility for screen readers.
<BitButton IconOnly AutoLoading OnClick="WrapHandled(() => ReadUserSessionLogs(session.Id))" Title="Read user session logs" + AriaLabel="Read user session logs" Color="BitColor.SecondaryBackground" IconName="@BitIconName.Download" />src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/Authorized/Management/UsersPage.razor.cs (1)
192-203: Consider thread safety when clearing the diagnostic store.The implementation correctly calls the updated SignalR method and displays the modal. However, clearing the entire
DiagnosticLogger.Storebefore adding new logs could potentially interfere with concurrent diagnostic operations.Consider a more targeted approach:
private async Task ReadUserSessionLogs(Guid userSessionId) { var logs = await hubConnection.InvokeAsync<DiagnosticLogDto[]>("GetUserSessionLogs", userSessionId, CurrentCancellationToken); - DiagnosticLogger.Store.Clear(); - foreach (var log in logs) - { - DiagnosticLogger.Store.Enqueue(log); - } + // Clear only if we have new logs to avoid clearing other concurrent operations + if (logs.Length > 0) + { + DiagnosticLogger.Store.Clear(); + foreach (var log in logs) + { + DiagnosticLogger.Store.Enqueue(log); + } + } PubSubService.Publish(ClientPubSubMessages.SHOW_DIAGNOSTIC_MODAL); }src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Services/Identity/AppUserClaimsPrincipalFactory.cs (1)
32-46: Add null checks for user properties to prevent potential NullReferenceException.The null-forgiving operators on
userNameandSecurityStampcould cause runtime exceptions if these values are unexpectedly null.Consider adding null checks:
id.AddClaim(new Claim(Options.ClaimsIdentity.UserIdClaimType, userId)); - id.AddClaim(new Claim(Options.ClaimsIdentity.UserNameClaimType, userName!)); + if (!string.IsNullOrEmpty(userName)) + id.AddClaim(new Claim(Options.ClaimsIdentity.UserNameClaimType, userName)); var email = user.Email; if (string.IsNullOrEmpty(email) is false) { id.AddClaim(new Claim(Options.ClaimsIdentity.EmailClaimType, email)); } - id.AddClaim(new Claim(Options.ClaimsIdentity.SecurityStampClaimType, user.SecurityStamp!)); + if (!string.IsNullOrEmpty(user.SecurityStamp)) + id.AddClaim(new Claim(Options.ClaimsIdentity.SecurityStampClaimType, user.SecurityStamp));
📜 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 (7)
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Layout/AppDiagnosticModal.razor(0 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Layout/AppDiagnosticModal.razor.Utils.cs(0 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/Authorized/Management/UsersPage.razor(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/Authorized/Management/UsersPage.razor.cs(3 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Services/Identity/AppUserClaimsPrincipalFactory.cs(3 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Services/Identity/UserClaimsService.cs(3 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/SignalR/AppHub.cs(1 hunks)
💤 Files with no reviewable changes (2)
- src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Layout/AppDiagnosticModal.razor
- src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Layout/AppDiagnosticModal.razor.Utils.cs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build and test
🔇 Additional comments (4)
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/SignalR/AppHub.cs (2)
80-88: Implementation is cleaner and more focused.The simplified query logic that directly looks up a single user session and its SignalR connection is much cleaner than the previous flexible query approach. The early return for empty connection IDs is appropriate.
75-75:✅ Verification successful
Verify all callers have been updated for the breaking change.
The method signature change from
GetUserDiagnosticLogs(string? userQuery)toGetUserSessionLogs(Guid userSessionId)is a breaking change that improves type safety and clarity.
🏁 Script executed:
#!/bin/bash # Description: Verify no remaining references to the old method name exist # Expected: No references to GetUserDiagnosticLogs should be found echo "Searching for references to old method name..." rg "GetUserDiagnosticLogs" --type cs echo "Searching for SignalR method invocations with old name..." rg "GetUserDiagnosticLogs" --type js --type tsLength of output: 311
No remaining references to
GetUserDiagnosticLogsVerification using
rg "GetUserDiagnosticLogs"across C# and JS/TS files returned no matches, confirming that all callers have been updated for the signature change.src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/Authorized/Management/UsersPage.razor.cs (2)
3-8: Conditional compilation is properly implemented.The conditional compilation directives for SignalR-related imports are correctly implemented and maintain clean separation of concerns.
34-36: HubConnection injection follows consistent pattern.The conditional injection of HubConnection is properly implemented and follows the same pattern as other dependencies.
...ate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Services/Identity/UserClaimsService.cs
Show resolved
Hide resolved
...ate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Services/Identity/UserClaimsService.cs
Outdated
Show resolved
Hide resolved
...ate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Services/Identity/UserClaimsService.cs
Outdated
Show resolved
Hide resolved
...erplate/src/Server/Boilerplate.Server.Api/Services/Identity/AppUserClaimsPrincipalFactory.cs
Outdated
Show resolved
Hide resolved
2 participants
closes #10933
Summary by CodeRabbit
New Features
Bug Fixes
Refactor