Improve Boilerplate SignalR PubSub integration (#9526) #9527
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 pull request introduces comprehensive modifications to the Boilerplate project template's SignalR and PubSub integration. Changes span multiple components across client and server layers, focusing on enhancing real-time messaging, user context propagation, and notification mechanisms. Key modifications include updating message handling in various components, adjusting SignalR message publishing strategies, and refactoring pub-sub message classes to support more flexible messaging architectures. Changes
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
…mplates-signalr-pubsub-integration-needs-improvements
…ration-needs-improvements' of https://github.com/ysmoradi/bitplatform into 9526-boilerplate-project-templates-signalr-pubsub-integration-needs-improvements
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/ClientAppCoordinator.cs (2)
183-183: Validate payload before publishing.Although the method signature accepts a nullable object payload, there's currently no runtime check to confirm it's well-formed or expected. If the payload must be a specific type or format, consider validating or casting similarly to how you handle the message string.
186-186: Log or handle publish errors.While publishing the message and payload works well under normal conditions, unexpected serialization or consumption errors may occur in downstream subscribers. Consider wrapping the publish call in error handling or adding additional logging to isolate problems in the subscription pipeline.
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Controllers/AttachmentController.cs (1)
74-77: Add logging for failed publishes
Consider adding try-catch or logging around the publish call to better handle failures or unexpected exceptions.src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Controllers/Identity/UserController.cs (1)
110-119: Ensure minimal data payload for efficiency.
Currently, the entire updated user object is being sent to all other sessions. If the UserDto is large, it might degrade performance. Consider sending only the relevant user info or a more constrained payload if you anticipate large user objects.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.github/workflows/bit.full.ci.yml(0 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/ClientAppCoordinator.cs(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Layout/AuthorizedHeader.razor.cs(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Layout/SnackBar.razor.cs(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Layout/UserMenu.razor.cs(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/Authorized/Dashboard/DashboardPage.razor.cs(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Services/ClientPubSubMessages.cs(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Controllers/AttachmentController.cs(4 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Controllers/Categories/CategoryController.cs(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Controllers/Identity/UserController.cs(2 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Controllers/Products/ProductController.cs(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Services/SharedPubSubMessages.cs(1 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/bit.full.ci.yml
🔇 Additional comments (14)
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Controllers/Products/ProductController.cs (1)
98-98: Confirm client-side handling of the null payload.
This update to pass 'null' as the third argument to SendAsync is logically valid if the client-side is prepared to handle a null payload. Otherwise, clients might throw errors or fail to parse the incoming message. Please verify that the "DASHBOARD_DATA_CHANGED" handlers gracefully handle this null argument.
To confirm that no client-side listener expects a non-null payload, you can run the following script to check all relevant consumer code:
✅ Verification successful
Passing null payload is safe - client code only triggers navigation
The client-side handler for DASHBOARD_DATA_CHANGED in DashboardPage.razor.cs simply triggers a page navigation and doesn't attempt to access the payload at all:
PubSubService.Subscribe(ClientPubSubMessages.DASHBOARD_DATA_CHANGED, async _ =>
{
NavigationManager.NavigateTo(Urls.DashboardPage, replace: true);
});The underscore parameter (_) indicates the payload is intentionally ignored, making the null payload perfectly safe.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify whether client code usage of "DASHBOARD_DATA_CHANGED" expects a non-null payload
rg -A 5 "DASHBOARD_DATA_CHANGED"
Length of output: 3440
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Controllers/AttachmentController.cs (4)
1-7: Conditional references for SignalR
The #if directive is set up properly for optional SignalR usage.
18-21: Injection of IHubContext
The injection of appHubContext is consistent with the #if directive. This ensures minimal overhead when SignalR is not used.
102-105: Repeat comment: Add logging for publish updates
Same suggestion as lines 74-77 regarding potential logging or error handling.
125-137: Verify personal data in user notifications
Ensure that UserDto does not contain sensitive or private info that shouldn’t be exposed to other sessions.
✅ Verification successful
Let me gather information about the UserDto structure and its properties to verify if it contains any sensitive data.
Let me search for the UserDto definition to understand its properties.
Let me try a different approach to find the UserDto properties by looking at the EditUserDto since they are mapped to each other.
Based on the context gathered, I can now generate the final response since we can see that EditUserDto (which maps to UserDto) only contains non-sensitive personal information like FullName, Gender, and BirthDate. Additionally, from the earlier search results, we can see that UserDto is used in UI components and is meant to be displayed to users in their profile sections.
No sensitive data exposed in user notifications
The UserDto shared across sessions contains only basic profile information (FullName, Gender, BirthDate) that is already visible to users in the UI. No sensitive data like passwords, security tokens, or private identifiers are included in the notification payload.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for UserDto definition and its properties
ast-grep --pattern 'class UserDto {
$$$
}'
# Also search for any references to UserDto properties
rg "UserDto" -A 5 -B 5
Length of output: 62608
Script:
#!/bin/bash
# Search for UserDto class definition and its properties
ast-grep --pattern 'public partial class UserDto {
$$$
}'
# Also search for the file content directly
rg -p "public partial class UserDto" -A 20 "UserDto.cs"
Length of output: 168
Script:
#!/bin/bash
# Search for EditUserDto class definition and its properties
rg -p "public partial class EditUserDto" -A 20
# Also search for any property mappings between UserDto and EditUserDto
ast-grep --pattern 'Patch(this EditUserDto source, UserDto destination)'
Length of output: 755
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Layout/SnackBar.razor.cs (1)
13-13: Validate argument type
Ensure that the args object is always the expected (string, string, BitColor) tuple to avoid runtime casting exceptions.
✅ Verification successful
Let me gather information about how this SnackBar component is used and how the args parameter is passed to it.
Let me search for the implementation of the SnackBar component to see how it handles the args parameter.
Let me search for where this SnackBar component is being used to understand how the args are being passed to it.
Let me check the SnackBarService implementation to see how the args are being constructed and validated.
Based on the code analysis, I can now provide a final response about the args validation concern:
Type safety is properly handled
The args parameter's type safety is ensured through the following implementation chain:
- The SnackBarService.Show method constructs the tuple with strongly-typed parameters: (string title, string body, BitColor color)
- The PubSubService.Publish method passes this tuple as args
- The SnackBar component's subscription handler uses pattern matching to safely deconstruct the tuple:
var (title, body, color) = ((string, string, BitColor))args! - The BitSnackBar.Show method receives these strongly-typed parameters
The casting operation is safe because:
- The tuple is always constructed with the correct types in SnackBarService
- The PubSubService maintains the tuple structure during transport
- The type pattern in the cast matches the original tuple structure exactly
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# First, let's find the SnackBar component implementation
ast-grep --pattern 'class SnackBar {
$$$
}'
# Let's also search for any usages of SnackBar component
rg "SnackBar" -A 5 -B 5
# Let's specifically look for any invocations or event handlers that might pass args to SnackBar
rg "ShowSnackBar|SnackBar.*args"
Length of output: 97328
Script:
#!/bin/bash
# Let's look for the SnackBar.razor.cs file in the Templates/Boilerplate directory
cat src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Layout/SnackBar.razor.cs
# Let's also search for any Show or ShowSnackBar methods that might be using this component
rg -A 5 "Show.*SnackBar|ShowSnackBar"
Length of output: 2705
Script:
#!/bin/bash
# Let's search for usages of the PubSubService.Publish with SHOW_SNACK message
rg -A 5 "PubSubService.*SHOW_SNACK|Publish.*SHOW_SNACK"
# Let's also look for any other files that might be using this SnackBar component
rg -l "Components.Layout.SnackBar"
Length of output: 1950
Script:
#!/bin/bash
# Let's look at the SnackBarService implementation
cat src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Services/SnackBarService.cs
# Let's also check if there are any tests that verify this functionality
rg -l "SnackBarService.*Test"
Length of output: 695
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Layout/AuthorizedHeader.razor.cs (1)
14-14: Ensure correct payload structure
Confirm that the payload can consistently be cast to (string, string). If there's a possibility of null or different object shapes, a safer pattern matching approach might be needed.
✅ Verification successful
Let me search for relevant code to understand the payload structure and its usage.
Let me search for the specific component and its usage of the payload.
Let me search for the payload type and how it's being published.
I have enough information to provide the final response based on the code analysis:
Cast is safe as payload structure is consistently enforced
The payload cast to (string, string) is safe because:
- The payload is published in
AppPageBase.cswith a tuple of(Title, Subtitle)which are both string properties - The publish call uses
persistent: truewhich ensures consistent payload type - The subscription and publishing are using the same message constant
ClientPubSubMessages.PAGE_TITLE_CHANGED - The PubSub pattern implementation maintains type consistency between publisher and subscriber
The current implementation is correct and type-safe.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for the component and its usage
ast-grep --pattern 'class AuthorizedHeader'
# Search for any payload assignments or handling in the codebase
rg -l "payload" --type cs
Length of output: 65734
Script:
#!/bin/bash
# Search for AuthorizedHeader component implementation
rg -A 10 "class AuthorizedHeader" --type cs
# Search for payload usage in the component
rg -A 5 "payload" "src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Layout/AuthorizedHeader.razor.cs"
Length of output: 2133
Script:
#!/bin/bash
# Search for PAGE_TITLE_CHANGED message definition and usage
rg -A 5 "PAGE_TITLE_CHANGED" --type cs
# Search for where this payload is being published
rg "PubSubService.*PAGE_TITLE_CHANGED" --type cs
Length of output: 4349
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Services/ClientPubSubMessages.cs (1)
6-6: Confirm references to removed constant
Ensure that all code references to the removed PROFILE_UPDATED constant have been updated or removed to prevent runtime errors.
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/Authorized/Dashboard/DashboardPage.razor.cs (1)
21-21: Subscription updated to ClientPubSubMessages
Switching from SharedPubSubMessages to ClientPubSubMessages is consistent with the new partial class structure.
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Controllers/Identity/UserController.cs (2)
122-122: LGTM for returning the updated user.
Returning the updated user is useful for confirmation. Ensure the client uses this updated object for UI synchronization to avoid additional network calls.
91-91: Validate the null payload usage.
Sending a null payload may be intentional if the client side strictly responds to the event type. However, confirm that the client logic does not break due to a null payload.
If you want to confirm no client references rely on a non-null payload:
✅ Verification successful
Let me check the client-side handler implementation to verify how the payload is handled.
Let me try a different approach to check the client-side handler.
Let me check the PubSubService implementation to see how messages are handled.
Null payload usage is safe and verified.
The code review has confirmed that the null payload usage is safe because:
- In AuthManager.cs, the SESSION_REVOKED handler
_ => SignOut(default)uses a discard parameter (_), indicating it doesn't use the payload at all - The PubSubService implementation properly handles null payloads in both its Publish and Subscribe methods
- The SignOut method called by the handler doesn't require any payload data to function
The current implementation is correct and won't cause any issues on the client side.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Searching for references to SharedPubSubMessages.SESSION_REVOKED usage that expect a non-null payload
rg 'SESSION_REVOKED'
Length of output: 923
Script:
#!/bin/bash
# Get the AuthManager implementation to see how SESSION_REVOKED is handled
ast-grep --pattern 'Subscribe($_, $_, $_)'
Length of output: 160
Script:
#!/bin/bash
# Get the content of the AuthManager.cs file to see the SESSION_REVOKED handler implementation
cat "src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Services/AuthManager.cs"
Length of output: 8597
Script:
#!/bin/bash
# Get the content of PubSubService to see how messages are handled
cat "src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Services/PubSubService.cs"
Length of output: 2144
src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Services/SharedPubSubMessages.cs (2)
17-17: New constant for PROFILE_UPDATED.
This addition is consistent with the usage noted in UserController and the client. No issues spotted.
9-9: Check usage of partial vs. static.
Changing from static to partial allows instantiating this class, which is unusual for a messages constants container. Ensure no unintended instance usage is introduced.
✅ Verification successful
Let me gather information about the usage of this class and its structure.
No concerns with partial class usage - it's a constants container with no instantiation
The verification shows that SharedPubSubMessages is used exclusively as a constants container, with all references accessing its constants statically (e.g., SharedPubSubMessages.SESSION_REVOKED). No instances of the class are created in the codebase, and the partial modifier appears to be used for code organization purposes. The class maintains its intended purpose as a message constants container.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# First, let's look at the full content of the SharedPubSubMessages class
rg -l "SharedPubSubMessages" --type cs
# Then get the content of the file
cat src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Services/SharedPubSubMessages.cs
# Look for any instantiation of SharedPubSubMessages
ast-grep --pattern 'new SharedPubSubMessages()'
# Look for other partial declarations of this class
rg "partial class SharedPubSubMessages" --type cs
# Look for references to this class to understand its usage
rg "SharedPubSubMessages\." --type cs
Length of output: 4356
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Layout/UserMenu.razor.cs (1)
42-44: Conditionally parsing JSON payload.
This approach cleanly handles server-invoked events carrying JSON data. Ensure that the fallback cast to (UserDto) remains valid under all client-side usage scenarios.
...t.Boilerplate/src/Server/Boilerplate.Server.Api/Controllers/Categories/CategoryController.cs
Show resolved
Hide resolved
ysmoradi
deleted the
9526-boilerplate-project-templates-signalr-pubsub-integration-needs-improvements
branch
This closes #9526
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores