Improve Boilerplate exception handling (#9635) #9636
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 modifications to the exception handling mechanism across multiple client-side service files. The changes primarily focus on enhancing the flexibility of exception display and handling, particularly for scenarios involving server connection and token refresh issues. A new 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Services/ClientExceptionHandlerBase.cs (1)
72-78: Consider expanding exception type handling.The
GetDisplayKindmethod currently handles specific exceptions. Consider making this method protected virtual to allow derived classes to customize the display kind for additional exception types.- private ExceptionDisplayKind GetDisplayKind(Exception exception) + protected virtual ExceptionDisplayKind GetDisplayKind(Exception exception)src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Extensions/IClientCoreServiceCollectionExtensions.cs (2)
163-167: Consider enhancing the exception handling strategy.While catching
ServerConnectionExceptionprevents annoying errors during frequent disconnections, the empty catch block might hide important issues. Consider:
- Adding debug logging to track these occurrences
- Implementing a circuit breaker pattern to temporarily stop refresh attempts after X failures
- Distinguishing between temporary network issues and permanent server problems
Here's a suggested implementation with logging and basic circuit breaking:
+ private static int _failedAttempts = 0; + private static DateTime _lastFailure = DateTime.MinValue; + private const int MAX_ATTEMPTS = 3; + private const int RESET_INTERVAL_MINUTES = 5; try { + if (_failedAttempts >= MAX_ATTEMPTS && DateTime.UtcNow.Subtract(_lastFailure).TotalMinutes < RESET_INTERVAL_MINUTES) + { + return null; // Circuit is open, skip refresh attempts + } return await authManager.RefreshToken(requestedBy: nameof(HubConnectionBuilder)); } catch (ServerConnectionException ex) { + _failedAttempts++; + _lastFailure = DateTime.UtcNow; + logger.LogDebug(ex, "Token refresh failed due to server connection issue. Attempt {Attempt}", _failedAttempts); }
163-167: Improve inline comment clarity.The current comment
/* If client gets disconnected... */could be more descriptive about the rationale behind this implementation choice.Consider updating the comment to:
- /* If client gets disconnected and access token become expired, then this code will be called every few seconds and will show annoying error to the user. */ + /* Suppress connection errors during token refresh attempts to prevent frequent error messages when: + * 1. Client loses connection + * 2. Access token expires + * 3. SignalR attempts to reconnect (which triggers token refresh) + */src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Services/AuthManager.cs (1)
Line range hint
126-141: Consider maintaining consistent exception handling patterns.While the code correctly handles token cleanup for critical failures, there's an inconsistency in exception handling patterns within the same file (compare with
TryEnterElevatedAccessModemethod which uses explicitdisplayKind).Consider either:
- Consistently using the new default behavior throughout the file, or
- Explicitly specifying the display kind for clarity in security-critical operations:
exceptionHandler.Handle(exp, parameters: new() { { "AdditionalData", "Refreshing access token failed." }, { "RefreshTokenRequestedBy", requestedBy } -}); +}, displayKind: exp is ReusedRefreshTokenException ? + ExceptionDisplayKind.NonInterrupting : + ExceptionDisplayKind.Default);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/AppComponentBase.cs(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Extensions/IClientCoreServiceCollectionExtensions.cs(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Services/AuthManager.cs(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Services/ClientExceptionHandlerBase.cs(3 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Services/Contracts/IExceptionHandler.cs(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Services/HttpMessageHandlers/ExceptionDelegatingHandler.cs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build and test
🔇 Additional comments (7)
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Services/Contracts/IExceptionHandler.cs (2)
18-22: LGTM! Well-documented enum extension.The addition of the
Defaultvalue with clear documentation improves the flexibility of exception handling by allowing automatic selection of display kind based on exception type.
28-28: LGTM! Consistent parameter update.Changing the default parameter to
ExceptionDisplayKind.Defaultaligns with the new dynamic exception handling approach.src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Services/ClientExceptionHandlerBase.cs (1)
53-57: LGTM! Clean implementation of dynamic display kind selection.The conditional check ensures that the display kind is only determined dynamically when explicitly requested through
Default.src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/AppComponentBase.cs (1)
263-263: LGTM! Consistent use of default display kind.The change aligns with the new dynamic exception handling approach, allowing the display kind to be determined based on the exception type.
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Services/HttpMessageHandlers/ExceptionDelegatingHandler.cs (1)
65-68: LGTM! Improved readability of catch conditions.The addition of parentheses makes the complex catch conditions more readable without changing the logic.
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Extensions/IClientCoreServiceCollectionExtensions.cs (1)
163-167: Verify error handling consistency across the codebase.The approach to handling
ServerConnectionExceptionhere might need to be consistent with other parts of the application that perform token refresh operations.Let's verify the error handling approach in other token refresh scenarios:
✅ Verification successful
Silent catch of ServerConnectionException is appropriate and consistent with codebase patterns
The current error handling approach is consistent with how connection-related errors are managed across the codebase. The silent catch is intentional and well-documented, preventing unnecessary error messages during disconnected states while other components (AuthManager, RetryHandler) maintain proper error handling and logging when needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other token refresh implementations and their error handling rg -A 5 "RefreshToken|ServerConnectionException" --type csLength of output: 37956
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Services/AuthManager.cs (1)
128-128: Verify exception handling behavior for security-critical operations.The removal of explicit
displayKindparameter relies on the new default behavior. While this aligns with the broader refactoring, we should ensure it maintains appropriate user feedback for security-critical token refresh failures.Let's verify the new behavior:
✅ Verification successful
The removal of explicit displayKind parameter aligns with codebase patterns.
The change follows the consistent exception handling pattern seen across the authentication flows, where default behavior is predominantly used. The security-critical nature of token refresh operations is preserved as the exception handling cleanup remains intact.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other exception handling patterns in the codebase # to ensure consistent behavior for security operations # Look for other Handle calls with explicit displayKind rg "Handle.*displayKind:" -A 2 # Look for exception handler configuration ast-grep --pattern 'class $_ : ClientExceptionHandlerBase { $$$ GetDisplayKind($_) { $$$ } }'Length of output: 690
Script:
#!/bin/bash # Search for ClientExceptionHandlerBase implementation ast-grep --pattern 'class $_ : ClientExceptionHandlerBase { $$$ }' # Find security-critical operations and their exception handling rg "RefreshToken|SignIn|SignOut|Authenticate" -A 2 # Look for Handle method calls without displayKind rg "\.Handle\([^,]+\)" --type csLength of output: 67314
closes #9635
Summary by CodeRabbit
New Features
Defaultoption to exception display handlingBug Fixes
Refactor