-
-
Notifications
You must be signed in to change notification settings - Fork 363
fix(AutoComplete): network delay causes input lag #5555
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
After testing live, i could still see problems, less when using OnBlurAsync but it was more noticeable in OnValueChanged. These changes address the core issue where the input text was being overwritten by stale data from older asynchronous operations. The dual approach (both C# and JavaScript checks) provides protection against race conditions. Even if one side fails, the other will catch it.
Reviewer's Guide by SourceryThis pull request addresses a race condition in the AutoComplete component where stale data from older asynchronous operations could overwrite the input text. It introduces both C# and JavaScript checks to ensure that only the latest input value is used to update the component's state. Sequence diagram for TriggerFilter with stale data protectionsequenceDiagram
participant User
participant InputElement
participant AutoCompleteJs
participant AutoCompleteCs
User->>InputElement: Types in input
InputElement->>AutoCompleteJs: Triggers Input.composition with value v
AutoCompleteJs->>AutoCompleteJs: debounce(filterCallback(v))
AutoCompleteJs->>InputElement: Stores v in data-lastValue
alt Input value has not changed
AutoCompleteJs->>AutoCompleteCs: invokeMethodAsync('TriggerFilter', v)
AutoCompleteCs->>AutoCompleteCs: _spinLock.Enter()
AutoCompleteCs->>AutoCompleteCs: _userCurrentInput = v
AutoCompleteCs->>AutoCompleteCs: _spinLock.Exit()
AutoCompleteCs->>AutoCompleteCs: OnCustomFilter(v) or Items.Where(s => s.StartsWith(v))
AutoCompleteCs->>AutoCompleteCs: _spinLock.Enter()
AutoCompleteCs->>AutoCompleteCs: latestInput = _userCurrentInput
AutoCompleteCs->>AutoCompleteCs: _spinLock.Exit()
alt latestInput == v
AutoCompleteCs->>AutoCompleteCs: CurrentValue = v
AutoCompleteCs->>AutoCompleteCs: ValueChanged.HasDelegate ? skip : StateHasChanged()
else latestInput != v
AutoCompleteCs-->>AutoCompleteCs: Discard stale data
end
else Input value has changed
AutoCompleteJs-->>AutoCompleteJs: Discard stale operation
end
Updated class diagram for AutoComplete componentclassDiagram
class AutoComplete {
-string _userCurrentInput
-SpinLock _spinLock
+Task TriggerFilter(string val)
}
note for AutoComplete "Added _userCurrentInput and _spinLock for handling race conditions"
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Thanks for your PR, @celadaris. Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @celadaris - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider if
SpinLockis the best choice here, given its potential for starvation and the relatively short critical section. - The javascript changes introduce a data attribute; ensure this doesn't conflict with any existing attributes or introduce security concerns.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
src/BootstrapBlazor/Components/AutoComplete/AutoComplete.razor.cs
Outdated
Show resolved
Hide resolved
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5555 +/- ##
===========================================
+ Coverage 99.98% 100.00% +0.01%
===========================================
Files 649 650 +1
Lines 29605 29627 +22
Branches 4172 4170 -2
===========================================
+ Hits 29602 29627 +25
+ Misses 2 0 -2
+ Partials 1 0 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
sourcery-ai gave good feedback, ill update with SemaphoreSlim instead... just a moment... |
|
when network lag is introduced the problem is still present, just a moment for a fix... |
|
just updated for the network problems |
|
@celadaris Thank you very much for submitting the PR. Could you let us know what problem you encountered, why you changed it this way, whether there is a better solution, and whether this change will affect other components? So can you create an Issue to provide a min-repro project for the problem? This PR is associated with this Issue so that we can track it. Thank you very much for your hard work |
# Conflicts: # src/BootstrapBlazor/Components/AutoComplete/AutoComplete.razor.cs
|
@sourcery-ai review |
celadaris
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tested, and works!
|
its asking for your review @ArgoZhang and i cant assign myself as a reviewer, but I did review the changes and it looks good! |
After testing live from the pull request I did before, I could still see problems, less when using OnBlurAsync but it was more noticeable in OnValueChanged.
These changes address the core issue where the input text was being overwritten by stale data from older asynchronous operations. The dual approach (both C# and JavaScript checks) provides protection against race conditions. Even if one side fails, the other will catch it.
Link issues
fixes #5110
fixes #5574
[Please fill in the relevant Issue number after the # above, such as #42]
[请在上方 # 后面填写相关 Issue 编号,如 #42]
Summary By Copilot
This pull request includes several changes to the
AutoCompletecomponent in theBootstrapBlazorproject, focusing on improving its rendering and behavior. The most important changes include updating the version, modifying the component structure, and introducing a newAutoCompleteItemsclass.Version Update:
9.4.8to9.4.9-beta01inBootstrapBlazor.csproj.Component Structure Improvements:
ulelement with aRenderTemplateinAutoComplete.razorto improve rendering flexibility. [1] [2]RenderFragmentnamedRenderDropdownto encapsulate dropdown rendering logic inAutoComplete.razor.New Class Introduction:
AutoCompleteItemsclass to handle the rendering of autocomplete items, including methods for attaching the render handle and rendering content.Behavioral Enhancements:
_renderflag and overridden theShouldRendermethod inAutoComplete.razor.csto control rendering behavior.TriggerFilterandTriggerChangemethods to use the new rendering logic and_renderflag inAutoComplete.razor.cs. [1] [2] [3]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 Sourcery
Addresses an issue where the input text in the AutoComplete component was being overwritten by stale data from older asynchronous operations by adding checks in both C# and JavaScript to ensure that the component only processes the latest user input.
Bug Fixes:
Enhancements: