revert(DockView): revert implement IAsyncDisposable interface#978
Merged
revert(DockView): revert implement IAsyncDisposable interface#978
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideReverts the previous implementation of async and custom disposal in DockView components, removing IAsyncDisposable from DockViewTitleBar and the override of Dispose in DockViewComponent, effectively restoring default lifecycle behavior and preventing DockView from clearing callbacks and component state on disposal. Updated class diagram for DockView disposal behaviorclassDiagram
class DockViewTitleBar {
+OnClickBarCallback
+OnClickBar() Task
}
class DockViewComponent {
-Visible
-IsLock
-Key
+OnClickBar() Task
+SetVisible(visible) void
+SetLock(isLock) void
}
Class diagram for reverted async and custom disposal in DockViewclassDiagram
class IAsyncDisposable {
<<interface>>
+DisposeAsync() ValueTask
}
class DockViewTitleBar_Previous {
+OnClickBarCallback
+OnClickBar() Task
+DisposeAsync() ValueTask
#DisposeAsync(disposing bool) ValueTask
}
class DockViewComponent_Previous {
+OnClickTitleBarCallback
+OnClickBar() Task
+Dispose(disposing bool) void
}
DockViewTitleBar_Previous ..|> 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}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- By removing the
Disposeoverride fromDockViewComponent,DockView.RemoveComponentState(Key)is no longer called, which may leave stale component state hanging around; if this revert is intentional, consider documenting or moving that cleanup logic elsewhere. - The revert drops explicit nulling of
OnClickTitleBarCallback/OnClickBarCallback; if these callbacks can capture long-lived references, you may want to confirm that their lifetimes are now acceptable or add an alternative cleanup mechanism.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- By removing the `Dispose` override from `DockViewComponent`, `DockView.RemoveComponentState(Key)` is no longer called, which may leave stale component state hanging around; if this revert is intentional, consider documenting or moving that cleanup logic elsewhere.
- The revert drops explicit nulling of `OnClickTitleBarCallback`/`OnClickBarCallback`; if these callbacks can capture long-lived references, you may want to confirm that their lifetimes are now acceptable or add an alternative cleanup mechanism.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.
Pull request overview
This PR reverts prior disposal-related changes in DockView components (per issue #977), removing explicit async/sync disposal implementations.
Changes:
- Removed
IAsyncDisposableimplementation and async dispose logic fromDockViewTitleBar. - Removed the
Dispose(bool)override fromDockViewComponent.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/components/BootstrapBlazor.DockView/Components/DockViewTitleBar.razor.cs | Drops IAsyncDisposable and associated cleanup logic from the title bar component. |
| src/components/BootstrapBlazor.DockView/Components/DockViewComponent.razor.cs | Removes the component’s custom disposal override (previously used for cleanup). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
191
to
194
| internal void SetVisible(bool visible) => Visible = visible; | ||
|
|
||
| internal void SetLock(bool isLock) => IsLock = isLock; | ||
|
|
||
| /// <summary> | ||
| /// <inheritdoc/> | ||
| /// </summary> | ||
| /// <param name="disposing"></param> | ||
| protected override void Dispose(bool disposing) | ||
| { | ||
| base.Dispose(disposing); | ||
|
|
||
| if (OnClickTitleBarCallback != null) | ||
| { | ||
| OnClickTitleBarCallback = null; | ||
| } | ||
|
|
||
| DockView.RemoveComponentState(Key); | ||
| } | ||
| } |
There was a problem hiding this comment.
DockViewComponent no longer overrides Dispose(bool), so DockView.RemoveComponentState(Key) is never called. _componentStates in DockViewV2 holds a strong reference to DockViewComponent via DockViewComponentState.Component, which can cause memory leaks and can also break re-adding a component with the same Key (since TryAdd will fail while the stale entry remains). Consider restoring the Dispose(bool) override (calling base.Dispose(disposing) and using DockView?.RemoveComponentState(Key)), so component state is removed when the component is disposed.
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 #977
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Revert DockView async disposal and related cleanup behavior to restore the previous non-disposable components.
Enhancements: