perf(ContextMenu): improve performance #5766
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's Guide by SourceryThis pull request refactors the Sequence diagram for ContextMenu initializationsequenceDiagram
participant document
participant ContextMenu.razor.js
participant utility.js
document->>ContextMenu.razor.js: init(id)
ContextMenu.razor.js->>utility.js: registerBootstrapBlazorModule("ContextMenu", id, callback)
utility.js->>utility.js: _items.push({identifier, cb})
utility.js->>utility.js: initModule()
utility.js->>utility.js: cb(this) // callback function
utility.js-->>ContextMenu.razor.js: Context
ContextMenu.razor.js->>document: EventHandler.on(document, 'click', context.cancelContextMenuHandler)
ContextMenu.razor.js->>document: EventHandler.on(document, 'contextmenu', context.cancelContextMenuHandler)
Sequence diagram for ContextMenu disposalsequenceDiagram
participant document
participant ContextMenu.razor.js
participant utility.js
document->>ContextMenu.razor.js: dispose(id)
ContextMenu.razor.js->>utility.js: ContextMenu.dispose(id, callback)
utility.js->>utility.js: disposeModule()
utility.js->>utility.js: cb(this) // callback function
utility.js-->>ContextMenu.razor.js: Context
ContextMenu.razor.js->>document: EventHandler.off(document, 'click', context.cancelContextMenuHandler)
ContextMenu.razor.js->>document: EventHandler.off(document, 'contextmenu', context.cancelContextMenuHandler)
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @ArgoZhang - I've reviewed your changes - here's some feedback:
Overall Comments:
- The changes to
utility.jslook like they might be useful in other modules - consider if they should be applied more widely. - The removal of the
continuestatement inContextMenu.razoris a good simplification.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue 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.
| const cm = { el, zone: getZone(el) }; | ||
| Data.set(id, cm); | ||
|
|
||
| registerBootstrapBlazorModule("ContextMenu", id, context => { |
There was a problem hiding this comment.
issue (complexity): Consider extracting the event registration and disposal logic into helper functions to reduce nested callbacks.
You can reduce the nested callback complexity by extracting the event registration/disposal logic into helper functions. This will keep your module registration intact but flatten the structure of your code, making it easier to follow.
For example:
// Define helpers to attach and detach events
function setupContextMenuEvents(context) {
if (context.cancelContextMenuHandler === void 0) {
context.cancelContextMenuHandler = e => {
const menu = document.querySelector('.bb-cm.show');
if (menu) {
const menuId = menu.getAttribute('id');
const cm = Data.get(menuId);
if (cm && cm.popper) {
cm.popper();
}
menu.classList.remove('show');
const zone = getZone(menu);
if (zone) {
zone.appendChild(menu);
}
}
};
}
EventHandler.on(document, 'click', context.cancelContextMenuHandler);
EventHandler.on(document, 'contextmenu', context.cancelContextMenuHandler);
}
function disposeContextMenuEvents(context) {
EventHandler.off(document, 'click', context.cancelContextMenuHandler);
EventHandler.off(document, 'contextmenu', context.cancelContextMenuHandler);
}Then, modify your module registration and disposal calls as follows:
// In init using the module registration call
registerBootstrapBlazorModule("ContextMenu", id, context => {
setupContextMenuEvents(context);
});// In cleanup, dispose the events with a similar pattern
const { ContextMenu } = window.BootstrapBlazor;
ContextMenu.dispose(id, context => {
disposeContextMenuEvents(context);
});This refactoring keeps the same functionality but reduces the depth of nesting by separating concerns into dedicated functions.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5766 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 657 658 +1
Lines 29926 29996 +70
Branches 4248 4253 +5
=========================================
+ Hits 29926 29996 +70 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Link issues
fixes #5765
Summary By Copilot
This pull request includes several changes to the
BootstrapBlazorcomponents and their associated tests. The most important changes focus on refactoring theContextMenucomponent and updating unit tests to improve functionality and maintainability.Refactoring of
ContextMenucomponent:src/BootstrapBlazor/Components/ContextMenu/ContextMenu.razor: Removed unnecessarycontinuestatement in theifblock forContextMenuDivider.src/BootstrapBlazor/Components/ContextMenu/ContextMenu.razor.js: Refactored theinitfunction to useregisterBootstrapBlazorModulefor better module management, and updated event handler registration accordingly. [1] [2]src/BootstrapBlazor/Components/ContextMenu/ContextMenu.razor.js: Updated thedisposefunction to properly unregister event handlers using theContextMenu.disposemethod fromBootstrapBlazor.Updates to utility functions:
src/BootstrapBlazor/wwwroot/modules/utility.js: Modified theregisterBootstrapBlazorModulefunction to pass the context to the callback for better control over module initialization and disposal. [1] [2]Unit test improvements:
test/UnitTest/Components/LayoutTest.cs: Updated theAuthorized_Oktest to useMarkupMatchesfor more accurate markup validation.test/UnitTest/Services/ThrottleTest.cs: Removed an unnecessary assertion in theThrottletest to streamline the test logic.Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Improve performance and refactor the ContextMenu component by updating module registration, event handling, and removing unnecessary code
Bug Fixes:
Enhancements:
Tests: