-
-
Notifications
You must be signed in to change notification settings - Fork 363
fix(AutoComplete): studder on long running OnValueChanged function call #5819
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
|
Thanks for your PR, @celadaris. Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
Reviewer's Guide by SourceryThis pull request addresses the issue of stuttering in the AutoComplete component when a long-running OnValueChanged function is called. The changes include refactoring the JavaScript event handling, enhancing component initialization and disposal, and improving dropdown menu rendering and data filtering. These changes aim to improve performance, prevent memory leaks, and enhance the overall user experience. Sequence diagram for handling keyup events in AutoCompletesequenceDiagram
participant Input as Input Field
participant AutoComplete as AutoComplete Component
participant Invoke as DotNet Invoke
Input->>AutoComplete: keyup event
alt Key is Enter or NumpadEnter
AutoComplete->>AutoComplete: Check if skipEnter is true
alt skipEnter is false
AutoComplete->>menu: querySelector('.active')
menu-->>AutoComplete: current item
alt current item is not null
AutoComplete->>current: click()
current-->>AutoComplete: Event
AutoComplete->>AutoComplete: close()
end
AutoComplete->>Invoke: EnterCallback()
Invoke-->>AutoComplete: Task
end
else Key is Escape
AutoComplete->>AutoComplete: Check if skipEsc is false
alt skipEsc is false
AutoComplete->>Invoke: EscCallback()
Invoke-->>AutoComplete: Task
AutoComplete->>AutoComplete: close()
end
else Key is ArrowUp or ArrowDown
AutoComplete->>menu: querySelector('.active')
menu-->>AutoComplete: current item
AutoComplete->>AutoComplete: Handle arrow keys
else Key is Backspace or Delete
AutoComplete->>AutoComplete: Check if trigger delete is true
alt trigger delete is true
AutoComplete->>Invoke: TriggerDeleteCallback(input.value)
Invoke-->>AutoComplete: Task
end
end
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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:
- The javascript changes are quite large - could you extract some of the logic into smaller functions to improve readability?
- The C# code now bypasses the
CurrentValuesetter - is this the right approach, or should the setter be modified instead?
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 2 issues found
- 🟢 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
src/BootstrapBlazor/Components/AutoComplete/AutoComplete.razor.cs
Outdated
Show resolved
Hide resolved
src/BootstrapBlazor/Components/AutoComplete/AutoComplete.razor.js
Outdated
Show resolved
Hide resolved
src/BootstrapBlazor/Components/AutoComplete/AutoComplete.razor.js
Outdated
Show resolved
Hide resolved
src/BootstrapBlazor/Components/AutoComplete/AutoComplete.razor.js
Outdated
Show resolved
Hide resolved
src/BootstrapBlazor/Components/AutoComplete/AutoComplete.razor.js
Outdated
Show resolved
Hide resolved
src/BootstrapBlazor/Components/AutoComplete/AutoComplete.razor.js
Outdated
Show resolved
Hide resolved
studder fix will be applied immediately rather than after a second or two after the component loads
Explanation for Failing Unit TestsThis PR fixes a stuttering issue that occurs when using To fix this reliably, the following core changes were made:
Impact on Tests: This necessary change in the timing of the C#
This PR prioritizes fixing the user-facing stuttering bug. The failing tests highlight behavior that was intentionally changed, and adapting the tests is the appropriate next step. |
Added a fix for #5821
re-apply fix, but use main branch as reference
Signed-off-by: celadaris <[email protected]>
accidently added duplicate GetFilterItemsByValue, oops. now its removed
This reverts commit c15ac74.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5819 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 660 660
Lines 30136 30117 -19
Branches 4253 4252 -1
=========================================
- Hits 30136 30117 -19 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@sourcery-ai review |
1 similar comment
|
@sourcery-ai review |
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 renaming the 'ac' object to something more descriptive to improve code readability.
- The removal of 'triggerBlur' and 'handlerKeydown' functions might affect the component's behavior; ensure these changes are intentional and tested.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 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.
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:
- The javascript changes look good, but there are a lot of C# changes that just remove code - can you confirm that all of this code is truly no longer needed?
- The javascript changes introduce
ac.showandac.closemethods, but these are only called internally - should these be exposed for external use?
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.
|
@celadaris use |
# Conflicts: # src/BootstrapBlazor/Components/AutoComplete/AutoComplete.razor # src/BootstrapBlazor/Components/AutoComplete/AutoComplete.razor.cs # src/BootstrapBlazor/Components/AutoComplete/AutoComplete.razor.js
Link issues
fixes #5812
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Refactor the AutoComplete component to improve performance and reduce stuttering during value changes by simplifying event handling and removing unnecessary rendering logic.
Bug Fixes:
Enhancements:
Tests: