feat(ValidateForm): add UnregisterAsyncSubmitButton method#7889
Merged
feat(ValidateForm): add UnregisterAsyncSubmitButton method#7889
Conversation
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds support for unregistering async submit buttons from ValidateForm and updates ButtonBase to use typed MouseEventArgs EventCallback for OnClick, including proper cleanup on disposal. Sequence diagram for async submit button registration lifecyclesequenceDiagram
participant ButtonBase
participant ValidateForm
Note over ButtonBase,ValidateForm: ButtonBase is rendered with IsAsync = true inside a ValidateForm
ButtonBase->>ValidateForm: RegisterAsyncSubmitButton(button)
ValidateForm->>ValidateForm: AsyncSubmitButtons.Add(button)
Note over ButtonBase,ValidateForm: Later, when ButtonBase is disposed (e.g., removed from UI)
ButtonBase->>ValidateForm: UnregisterAsyncSubmitButton(button)
ValidateForm->>ValidateForm: AsyncSubmitButtons.Remove(button)
Note over ButtonBase: During disposal
ButtonBase->>ButtonBase: if OnClick.HasDelegate
ButtonBase->>ButtonBase: OnClick = EventCallback<MouseEventArgs>.Empty
Updated class diagram for ValidateForm and ButtonBaseclassDiagram
class ValidateForm {
- TaskCompletionSource~bool~ _tcs
+ void RegisterAsyncSubmitButton(ButtonBase button)
+ void UnregisterAsyncSubmitButton(ButtonBase button)
- Task OnValidSubmitForm(EditContext context)
}
class ButtonBase {
<<abstract>>
+ EventCallback<MouseEventArgs> OnClick
+ bool IsAsync
+ ValueTask DisposeAsync(bool disposing)
}
class EditContext
class MouseEventArgs
ValidateForm --> ButtonBase : manages AsyncSubmitButtons
ButtonBase ..> ValidateForm : uses
ButtonBase ..> MouseEventArgs : OnClick payload
ValidateForm ..> EditContext : used in OnValidSubmitForm()
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}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7889 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 764 764
Lines 34162 34173 +11
Branches 4700 4702 +2
=========================================
+ Hits 34162 34173 +11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Adds support for unregistering async submit buttons from ValidateForm to avoid retaining disposed buttons, and updates button click callback typing.
Changes:
- Add
ValidateForm.UnregisterAsyncSubmitButtonand remove disposed async buttons from the form’s tracking list. - Unregister async submit buttons during
ButtonBase.DisposeAsyncto prevent stale references. - Change
ButtonBase.OnClickfromEventCallbacktoEventCallback<MouseEventArgs>.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/BootstrapBlazor/Components/ValidateForm/ValidateForm.razor.cs | Introduces UnregisterAsyncSubmitButton to remove tracked async submit buttons. |
| src/BootstrapBlazor/Components/Button/ButtonBase.cs | Unregisters async buttons on dispose; changes OnClick callback type and adjusts disposal cleanup. |
Comments suppressed due to low confidence (1)
src/BootstrapBlazor/Components/Button/ButtonBase.cs:345
- New behavior relies on unregistering async submit buttons during ButtonBase disposal, but there are existing unit tests around ValidateForm async submit behavior and none appear to cover unregister/dispose interactions. Please add a test that removes/disposes an async submit button and verifies ValidateForm no longer tries to TriggerAsync on it during submit (and does not keep it disabled).
if (IsAsync && ValidateForm != null)
{
ValidateForm.UnregisterAsyncSubmitButton(this);
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// </summary> | ||
| [Parameter] | ||
| public EventCallback OnClick { get; set; } | ||
| public EventCallback<MouseEventArgs> OnClick { get; set; } |
There was a problem hiding this comment.
Changing ButtonBase.OnClick from untyped EventCallback to EventCallback is a public API breaking change and appears unrelated to the ValidateForm unregister feature. This will break consumers that set OnClick from C# (or via reflection/parameter dictionaries) expecting EventCallback. Consider keeping the existing EventCallback OnClick for compatibility (or adding a new typed callback alongside it / marking the old one obsolete) and update release notes accordingly if the breaking change is intended.
Suggested change
| public EventCallback<MouseEventArgs> OnClick { get; set; } | |
| public EventCallback OnClick { get; set; } |
Comment on lines
+585
to
+588
| internal void UnregisterAsyncSubmitButton(ButtonBase button) | ||
| { | ||
| AsyncSubmitButtons.Remove(button); | ||
| } |
There was a problem hiding this comment.
UnregisterAsyncSubmitButton mutates AsyncSubmitButtons while OnValidSubmitForm/OnInvalidSubmitForm iterate that same list across multiple awaits. If an async submit button is disposed/removed while a submit callback is awaiting, this can trigger 'Collection was modified' exceptions during the subsequent foreach loops. To make this robust, consider iterating over a snapshot (e.g., ToArray()) or otherwise guarding list access during submit processing.
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 #7888
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Add support for unregistering async submit buttons from ValidateForm and align button click handling with typed mouse event callbacks.
New Features:
Enhancements: