Destroy the Sortable instance when the component is disposed - #15
Destroy the Sortable instance when the component is disposed#15Kebechet wants to merge 2 commits into
Conversation
SortableList never released its Sortable instance, JS module reference or DotNetObjectReference.
|
Warning Review limit reached
Next review available in: 52 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe component now implements asynchronous disposal, retains its imported JavaScript module, and coordinates cleanup with tracked JavaScript Sortable instances. ChangesSortable lifecycle
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant SortableList
participant JSModule
participant Sortable
SortableList->>JSModule: initialize Sortable instance
JSModule->>Sortable: create and track instance
SortableList->>JSModule: destroy tracked instance
JSModule->>Sortable: destroy instance
SortableList->>JSModule: dispose module
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR addresses resource leaks in the SortableList Blazor component by ensuring the underlying SortableJS instance, JS module reference, and .NET interop references are properly torn down when the component is disposed.
Changes:
- Add JS-side tracking of Sortable instances keyed by element id, with a new
destroy(id)export and automatic re-init cleanup. - Implement component disposal via
IAsyncDisposable, keeping the imported JS module in a field and invokingdestroyduring teardown. - Update the
.razorto@implements IAsyncDisposableso Blazor actually calls the disposal method.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| BlazorSortableList.Lib/SortableList.razor.js | Track Sortable instances in a module-level Map, add destroy(id), and register/destroy instances by id. |
| BlazorSortableList.Lib/SortableList.razor.cs | Persist JS module reference, implement DisposeAsync teardown, and call JS destroy + dispose module/self reference. |
| BlazorSortableList.Lib/SortableList.razor | Implement IAsyncDisposable on the component so disposal is invoked by Blazor. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| //await JS.InvokeVoidAsync("console.log", $"***id:{Id}, group:{Group},pull: {Pull},put:{Put},sort:{Sort}, handle:{Handle}, filter:{Filter}, forceFallback:{ForceFallback}"); | ||
| await module.InvokeAsync<string>( | ||
| await _module.InvokeAsync<string>( |
There was a problem hiding this comment.
Fixed - switched to InvokeVoidAsync. init has no return value, so typing the call as <string> was relying on null marshalling.
| if (_module != null) | ||
| { | ||
| try | ||
| { | ||
| await _module.InvokeVoidAsync("destroy", Id); | ||
| await _module.DisposeAsync(); | ||
| } | ||
| catch (JSDisconnectedException) | ||
| { | ||
| // The circuit is already gone, so there is nothing left to clean up on the JS side. | ||
| } | ||
|
|
||
| _module = null; | ||
| } |
There was a problem hiding this comment.
Agreed, fixed. Managed cleanup now sits in a finally so _module and selfReference are always released, with JSDisconnectedException and JSException both swallowed. Module disposal is itself in an inner finally so a failing destroy still releases the module.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@BlazorSortableList.Lib/SortableList.razor.cs`:
- Around line 73-80: Update DisposeAsync and the initialization state so cleanup
uses the ID captured when the JavaScript Sortable instance was created, rather
than the mutable Id parameter. Store that initialized ID during init and pass it
to the destroy invocation, preserving the existing disposal flow.
- Around line 73-92: Update SortableList.DisposeAsync so managed cleanup always
runs when JavaScript disposal fails: retain the special handling for
JSDisconnectedException, but move _module = null and selfReference disposal into
appropriate finally blocks that execute regardless of other exceptions. Preserve
the existing destroy and module-disposal operations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e9bb6579-cecd-4058-b2a2-cf19269e2136
📒 Files selected for processing (3)
BlazorSortableList.Lib/SortableList.razorBlazorSortableList.Lib/SortableList.razor.csBlazorSortableList.Lib/SortableList.razor.js
Problem
SortableListnever releases anything it allocates:public void Dispose(), but it does not implementIDisposableand the.razorhas no@implements, so the method is dead code and is never invoked.Sortableinstance created ininit()is stored in a function-localvarand never returned or registered, so nothing can reach it to call.destroy().IJSObjectReferencemodule is a local inOnAfterRenderAsyncand is likewise never disposed.Every mount/unmount cycle (a list inside a dialog or popup, a route change) leaves behind a live Sortable instance with document-level listeners plus a leaked
DotNetObjectReference.Implementation
Mapkeyed by element id and add adestroy(id)export.initnow destroys a pre-existing instance under the same id before creating a new one._modulefield, implementIAsyncDisposable, and on disposal calldestroy, dispose the module, and disposeselfReference.JSDisconnectedExceptionis swallowed, since a torn-down circuit has nothing left to clean up.SortableList.razorgains@implements IAsyncDisposableso Blazor actually calls it.Dispose()is replaced byDisposeAsync(). Since the old method was unreachable (the type never implementedIDisposable), no working call site can exist.Builds clean; the two remaining CS8618 warnings are pre-existing.
Summary by CodeRabbit