feat(DynamicElement): implement IDisposeAsync interface#7896
Merged
Conversation
Contributor
Reviewer's GuideImplements asynchronous disposal for DynamicElement and improves documentation comments in BootstrapModuleComponentBase, ensuring proper cleanup of event handlers and consistent XML doc strings. Sequence diagram for DynamicElement asynchronous disposal flowsequenceDiagram
participant Caller
participant DynamicElement
participant GC
Caller->>DynamicElement: DisposeAsync()
activate DynamicElement
DynamicElement->>DynamicElement: DisposeAsync(true)
activate DynamicElement
DynamicElement->>DynamicElement: Clear OnClick
DynamicElement->>DynamicElement: Clear OnDoubleClick
DynamicElement->>DynamicElement: Clear OnContextMenu
DynamicElement->>DynamicElement: Clear ChildContent
deactivate DynamicElement
DynamicElement->>GC: SuppressFinalize(this)
deactivate DynamicElement
GC-->>Caller: Finalization suppressed for DynamicElement
Class diagram for DynamicElement implementing IAsyncDisposableclassDiagram
class BootstrapComponentBase
class IAsyncDisposable {
<<interface>>
+DisposeAsync() ValueTask
}
class DynamicElement {
+string TagName
+RenderFragment ChildContent
+EventCallback OnClick
+EventCallback OnDoubleClick
+EventCallback OnContextMenu
+Task OnTriggerClick(MouseEventArgs e)
+Task OnTriggerDoubleClick(MouseEventArgs e)
+Task OnTriggerContextMenu(MouseEventArgs e)
+DisposeAsync() ValueTask
#DisposeAsync(bool disposing) ValueTask
}
DynamicElement --|> BootstrapComponentBase
DynamicElement ..|> IAsyncDisposable
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:
- In
DynamicElement,DisposeAsync(bool disposing)never awaits anything, so it doesn’t need to beasync—you can removeasyncand returnValueTask.CompletedTaskwhen appropriate, and haveDisposeAsync()directly return theValueTaskinstead of awaiting it. - Consider aligning the disposal pattern in
DynamicElementwith the existing async disposal conventions in the codebase (e.g., aDisposeAsyncCore-style method) for consistency with other components implementingIAsyncDisposable.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `DynamicElement`, `DisposeAsync(bool disposing)` never awaits anything, so it doesn’t need to be `async`—you can remove `async` and return `ValueTask.CompletedTask` when appropriate, and have `DisposeAsync()` directly return the `ValueTask` instead of awaiting it.
- Consider aligning the disposal pattern in `DynamicElement` with the existing async disposal conventions in the codebase (e.g., a `DisposeAsyncCore`-style method) for consistency with other components implementing `IAsyncDisposable`.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
Implements async disposal support for DynamicElement and aligns XML documentation wording around async resource disposal in the base JS-module component.
Changes:
- Make
DynamicElementimplementIAsyncDisposableand addDisposeAsyncmethods to clear delegate/content references. - Update
BootstrapModuleComponentBaseXML docs to use consistent “BootstrapBlazor” naming and improve the Chinese description for async disposal.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/BootstrapBlazor/Components/BaseComponents/DynamicElement.cs | Adds IAsyncDisposable implementation and async-dispose pattern methods. |
| src/BootstrapBlazor/Components/BaseComponents/BootstrapModuleComponentBase.cs | Documentation-only adjustments for naming and async-dispose summary text. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+185
to
+194
| protected virtual async ValueTask DisposeAsync(bool disposing) | ||
| { | ||
| if (disposing) | ||
| { | ||
| OnClick = null; | ||
| OnDoubleClick = null; | ||
| OnContextMenu = null; | ||
| ChildContent = null; | ||
| } | ||
| } |
There was a problem hiding this comment.
DisposeAsync(bool disposing) is marked async but contains no await. This will trigger CS1998 and also allocates an unnecessary async state machine on every dispose. Since the method only clears references, make it non-async and return a completed ValueTask (or make it ValueTask returning without async).
Comment on lines
+199
to
+203
| public async ValueTask DisposeAsync() | ||
| { | ||
| await DisposeAsync(true); | ||
| GC.SuppressFinalize(this); | ||
| } |
There was a problem hiding this comment.
DisposeAsync() is async solely to await DisposeAsync(true). If DisposeAsync(bool) is changed to return a completed ValueTask synchronously, consider making this method non-async too and returning the ValueTask directly (still calling GC.SuppressFinalize(this)), to avoid an extra state machine allocation.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7896 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 764 764
Lines 34200 34213 +13
Branches 4705 4706 +1
=========================================
+ Hits 34200 34213 +13
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:
|
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 #7895
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Implement asynchronous disposal support for dynamic UI elements and align related documentation comments.
Enhancements: