feat(BootstrapInput): add OnBlurAsync parameter #4521
Conversation
|
Warning Rate limit exceeded@ArgoZhang has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 56 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes in this pull request introduce a new Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
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
🧹 Outside diff range and nitpick comments (3)
src/BootstrapBlazor/Components/Input/BootstrapInput.razor (1)
10-10: Consider adding error handling for the blur event.The blur event handler implementation looks correct. However, since
OnBluris an async method that invokes a user-provided callback, consider adding error handling to gracefully handle any exceptions that might occur during the callback execution.Consider wrapping the OnBlur implementation with try-catch:
protected virtual async Task OnBlur() { try { if (OnBlurAsync != null) { await OnBlurAsync(Value); } } catch (Exception ex) { // Log the error or notify through your preferred error handling mechanism Logger.LogError(ex, "Error occurred during OnBlur callback"); } }src/BootstrapBlazor/Components/Input/BootstrapInput.razor.cs (1)
25-30: Enhance the XML documentation for OnBlurAsync parameter.While the basic documentation is present, it would be helpful to add more details about:
- When exactly this callback is invoked
- What the TValue parameter represents (the current input value)
- Whether this is called before or after internal value validation
Consider expanding the documentation like this:
/// <summary> - /// 获得/设置 失去焦点回调方法 默认 null + /// 获得/设置 失去焦点回调方法 默认 null + /// Gets or sets the callback that is invoked when the input loses focus. + /// The callback receives the current input value and is called after internal validation. /// </summary>test/UnitTest/Components/InputTest.cs (1)
360-375: LGTM! Consider adding more test cases for comprehensive coverage.The test correctly verifies the basic functionality of the OnBlurAsync callback. However, to ensure robust testing, consider adding the following test cases:
+ [Fact] + public async Task OnBlurAsync_NullCallback_Ok() + { + var cut = Context.RenderComponent<BootstrapInput<string>>(); + var input = cut.Find("input"); + await cut.InvokeAsync(() => { input.Blur(); }); + // Should not throw + } + + [Fact] + public async Task OnBlurAsync_VerifyValue_Ok() + { + var expectedValue = "test"; + string? actualValue = null; + var cut = Context.RenderComponent<BootstrapInput<string>>(builder => + { + builder.Add(a => a.Value, expectedValue); + builder.Add(a => a.OnBlurAsync, v => + { + actualValue = v; + return Task.CompletedTask; + }); + }); + var input = cut.Find("input"); + await cut.InvokeAsync(() => { input.Blur(); }); + Assert.Equal(expectedValue, actualValue); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- src/BootstrapBlazor/Components/Input/BootstrapInput.razor (1 hunks)
- src/BootstrapBlazor/Components/Input/BootstrapInput.razor.cs (2 hunks)
- test/UnitTest/Components/InputTest.cs (1 hunks)
🔇 Additional comments (3)
src/BootstrapBlazor/Components/Input/BootstrapInput.razor (1)
10-10: Verify async event handling implementation.The async event handler looks properly integrated. Let's verify the implementation details to ensure proper async/await usage and event handling.
✅ Verification successful
Let me gather more information about the OnBlur implementation in the BootstrapInput component to verify the async event handling.
Based on the gathered information, I can now provide the final response:
Async event handling is properly implemented
The implementation shows a well-structured async event handling pattern:
- The
OnBlurmethod is correctly marked asvirtual async Task- It properly checks for null before invoking the async delegate
- The
OnBlurAsyncis defined asFunc<TValue, Task>?which is the correct signature for async event handlers- The implementation is consistent with other components (AutoComplete, Textarea) as seen in the codebase
- Unit tests verify the blur functionality across components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the OnBlur implementation and related async handling # Test 1: Check the base class implementation for any existing blur handling echo "Checking base class implementation..." ast-grep --pattern 'class BootstrapInputEventBase<$_> { $$$ OnBlur($$$) { $$$ } $$$ }' # Test 2: Look for similar blur event implementations in other components echo "Checking other components for blur handling patterns..." rg -l "protected\s+virtual\s+async\s+Task\s+OnBlur" # Test 3: Verify the test implementation echo "Checking test implementation..." rg -A 5 "OnBlurAsync_Ok"Length of output: 2474
Script:
#!/bin/bash # Check the actual implementation of OnBlur in BootstrapInput and related components # Look for OnBlur implementation in BootstrapInput echo "Checking BootstrapInput OnBlur implementation..." rg -A 10 "OnBlur\(" "src/BootstrapBlazor/Components/Input/BootstrapInput.razor.cs" # Check for OnBlurAsync property/method echo "Checking OnBlurAsync definition..." rg -A 5 "OnBlurAsync" "src/BootstrapBlazor/Components/Input/BootstrapInput.razor.cs" # Check base class for any event handling patterns echo "Checking base class event handling..." ast-grep --pattern 'class BootstrapInputBase<$_> { $$$ protected $$ OnBlur($$$) { $$$ } $$$ }'Length of output: 969
src/BootstrapBlazor/Components/Input/BootstrapInput.razor.cs (2)
60-65: Verify the interaction between validation and blur handling.There might be a race condition between validation and the blur event. Ensure that OnBlurAsync is called with the validated value, not the raw input value.
Let's check the component's event ordering:
#!/bin/bash # Search for validation-related code in the component rg -l "OnValidate|ValidationMessage" --type cs # Check if there are any existing tests covering this scenario rg -l "OnBlurAsync.*Invalid" --type cs
56-66: 🛠️ Refactor suggestionConsider improving the OnBlur method implementation.
The current implementation has several areas that could be enhanced:
- Exception handling for the callback
- Documentation for proper virtual method usage
- Support for cancellation in long-running operations
Consider this improved implementation:
/// <summary> - /// OnBlur 方法 + /// Called when the input loses focus. + /// Override this method to customize blur behavior while ensuring to call base.OnBlur(). /// </summary> + /// <returns>A task representing the asynchronous operation.</returns> + /// <exception cref="Exception">Any exceptions from the OnBlurAsync callback are propagated.</exception> protected virtual async Task OnBlur() { if (OnBlurAsync != null) { - await OnBlurAsync(Value); + try + { + using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(30)); + await OnBlurAsync(Value).WaitAsync(cts.Token); + } + catch (OperationCanceledException) + { + // Log or handle timeout + throw; + } } }This implementation:
- Adds proper documentation for inheritors
- Includes timeout protection for the callback
- Makes exception handling explicit
Let's check if there are any existing derived classes that might be affected:
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4521 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 631 631
Lines 27063 27070 +7
Branches 3926 3927 +1
=========================================
+ Hits 27063 27070 +7 ☔ View full report in Codecov by Sentry. |
add OnBlurAsync parameter
Summary of the changes (Less than 80 chars)
简单描述你更改了什么, 不超过80个字符；如果有关联 Issue 请在下方填写相关编号
Description
fixes #4520
Regression?
[If yes, specify the version the behavior has regressed from]
[是否影响老版本]
Risk
[Justify the selection above]
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by CodeRabbit
New Features
OnBlurAsynccallback parameter to theBootstrapInputcomponent for handling focus loss events.@onblurevent handler to enhance input interaction.Bug Fixes
OnBlurAsynccallback is triggered correctly when the input loses focus.Tests
BootstrapInputcomponent.