Conversation
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR updates the ButtonBase component’s click handling and lifecycle cleanup: it simplifies the OnClick event signature and ensures click handlers and async submit registration are properly cleared during disposal, while also refactoring some lifecycle XML docs to inherit from the base class. Sequence diagram for ButtonBase disposal and click handler resetsequenceDiagram
participant B as ButtonBase
participant VF as ValidateForm
participant T as TooltipService
participant Base as TooltipWrapperBase
B->>B: DisposeAsync(disposing=true)
alt disposing is true
alt OnClick.HasDelegate is true
B->>B: OnClick = EventCallback.Empty
end
alt IsAsync is true and ValidateForm not null
B->>VF: UnregisterAsyncSubmitButton(this)
end
B->>T: RemoveTooltip()
end
B->>Base: base.DisposeAsync(disposing)
Base-->>B: ValueTask completed
Class diagram for updated ButtonBase click and dispose behaviorclassDiagram
class TooltipWrapperBase
class ButtonBase {
<<abstract>>
+EventCallback OnClick
+bool IsAsync
+ValidateForm ValidateForm
+override void OnParametersSet()
+override Task OnAfterRenderAsync(bool firstRender)
+virtual Task RemoveTooltip()
+override ValueTask DisposeAsync(bool disposing)
}
class ValidateForm {
+void UnregisterAsyncSubmitButton(ButtonBase button)
}
TooltipWrapperBase <|-- ButtonBase
ValidateForm --> ButtonBase : managesAsyncSubmitButtons
%% Key change: OnClick no longer generic, cleared in DisposeAsync
ButtonBase : OnClick = EventCallback.Empty in DisposeAsync when HasDelegate
ButtonBase : calls ValidateForm.UnregisterAsyncSubmitButton when IsAsync and ValidateForm not null
File-Level Changes
Assessment against linked issues
Possibly 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}
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Changing
OnClickfromEventCallback<MouseEventArgs>to parameterlessEventCallbackis a breaking API change; consider whether this should be introduced via a new parameter (or overload) or at least ensure all consumer usages that rely onMouseEventArgsare intentionally migrated. - Resetting the
[Parameter]OnClickinsideDisposeAsyncmutates an incoming parameter, which is unusual in Blazor patterns; you might prefer guarding invocations with a_disposedflag instead of overwriting the parameter to avoid surprising parent components or future maintainers.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Changing `OnClick` from `EventCallback<MouseEventArgs>` to parameterless `EventCallback` is a breaking API change; consider whether this should be introduced via a new parameter (or overload) or at least ensure all consumer usages that rely on `MouseEventArgs` are intentionally migrated.
- Resetting the `[Parameter]` `OnClick` inside `DisposeAsync` mutates an incoming parameter, which is unusual in Blazor patterns; you might prefer guarding invocations with a `_disposed` flag instead of overwriting the parameter to avoid surprising parent components or future maintainers.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the ButtonBase component’s click callback API and adds disposal-time cleanup intended to “reset” click handling.
Changes:
- Changes
ButtonBase.OnClickfromEventCallback<MouseEventArgs>to non-genericEventCallback. - Adds disposal logic to clear
OnClickand keeps async-submit-button unregister behavior. - Replaces several method XML docs with
<inheritdoc/>.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// </summary> | ||
| [Parameter] | ||
| public EventCallback<MouseEventArgs> OnClick { get; set; } | ||
| public EventCallback OnClick { get; set; } |
There was a problem hiding this comment.
Changing OnClick from EventCallback<MouseEventArgs> to non-generic EventCallback is a breaking API change and will also break existing usages inside this repo that bind parameterized handlers (e.g., OnClick="@(e => ...)" in src/BootstrapBlazor.Server/Components/Components/WinButton.razor:1 and tests like test/UnitTest/Components/ButtonTest.cs:163). If the intent is to keep supporting click handlers that accept MouseEventArgs, keep the generic type and (ideally) pass the actual MouseEventArgs through from the @onclick handler; otherwise this needs a compatibility path (e.g., add a new parameter and obsolete the old one).
Suggested change
| public EventCallback OnClick { get; set; } | |
| public EventCallback<MouseEventArgs> OnClick { get; set; } |
Comment on lines
+335
to
+343
| if (OnClick.HasDelegate) | ||
| { | ||
| OnClick = EventCallback.Empty; | ||
| } | ||
|
|
||
| if (IsAsync && ValidateForm != null) | ||
| { | ||
| ValidateForm.UnregisterAsyncSubmitButton(this); | ||
| } |
There was a problem hiding this comment.
DisposeAsync is mutating a [Parameter] (OnClick = EventCallback.Empty). This is unconventional and easy to miss, and if the goal is to break references for GC it should be done consistently (e.g., clear OnClickWithoutRender as well) and without changing the public API shape. Consider removing this assignment entirely, or at least clearing the callback(s) using default for the existing callback type rather than introducing a new EventCallback type.
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
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 #7886
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Reset button click callbacks and unregister async submit buttons on disposal to avoid lingering event handlers and form registrations.
New Features:
Enhancements: