Repository navigation
fix: avoid JS interop errors during board teardown - #182
Merged
Merged
Conversation
📝 WalkthroughWalkthroughBoardView now tracks disposal state, stops post-disposal UI and hub work, handles protected storage disconnects separately, and disposes its SignalR connection asynchronously with debug logging on failure. ChangesBoardView lifecycle and disconnect handling
Sequence Diagram(s)sequenceDiagram
participant BlazorRuntime
participant BoardView
participant ProtectedLocalStorage
participant HubConnection
BlazorRuntime->>BoardView: DisposeAsync()
BoardView->>BoardView: set _isDisposed = true
BoardView->>HubConnection: DisposeAsync()
HubConnection-->>BoardView: success or failure
BoardView->>ProtectedLocalStorage: load/save preferences
alt JSDisconnectedException
BoardView->>BoardView: reset model or ignore save
end
BoardView->>BoardView: skip StateHasChanged when disposed
Estimated code review effort: High Related issues: None specified Related PRs: None specified Suggested labels: blazor, lifecycle, bug Suggested reviewers: None specified 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@Ticky.Web/Components/Pages/BoardView.razor`:
- Around line 545-548: LoadBoardAsync still invokes a final StateHasChanged
after LoadCardsAsync may have already exited because _isDisposed is true. Add
the same _isDisposed guard in LoadBoardAsync immediately after awaiting
LoadCardsAsync, before the final InvokeAsync(StateHasChanged), so the component
does not attempt a render after disposal. Use the existing _isDisposed check
pattern already present in LoadCardsAsync and keep the render call guarded in
BoardView.razor.
- Around line 554-556: Update the initial preference-loading logic in
BoardView’s protected local storage reads so they handle JS disconnects the same
way as the save paths. In the code around the GetAsync calls in the BoardView
component, add JSDisconnectedException handling alongside the existing
CryptographicException catch blocks, using the same try/catch pattern already
used for the SetAsync paths, and keep the relevant load methods in BoardView
consistent.
- Around line 563-579: Suppress hub startup failures after disposal in
BoardView’s connection flow: when `HandleConnectionAsync()` is running
fire-and-forget, disposal can race with `StartAsync()` or `SendAsync()` and
incorrectly trigger the “Real-time updates unavailable” notification. Update the
exception handling around the hub startup/send path to check `_isDisposed` and
return early from the `catch` when the component has already been disposed,
while keeping normal error handling for non-disposal failures.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 97d3a7a8-0c5b-480b-b63b-faf00cdae92c
📒 Files selected for processing (1)
Ticky.Web/Components/Pages/BoardView.razor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Ticky.Web/Components/Pages/BoardView.razor (1)
511-521: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winStop caller-side work after disposal, not only renders.
The new guard exits
LoadBoardAsync, butHandleUpdate()still continues toOnDataChanged(), andOnAfterRenderAsync()still continues afterHandleUpdate(). If disposal starts while loading is awaiting, teardown can still send hub messages or update layout/visit state after the component is disposed.🐛 Proposed fix
protected override async Task HandleUpdate() { + if (_isDisposed) + return; + await LoadBoardAsync(); + + if (_isDisposed) + return; + await OnDataChanged(); }await HandleUpdate(); + + if (_isDisposed) + return; if (_board is null) return;private async Task OnDataChanged() { - if(_hubConnection is null || _hubConnection.State != HubConnectionState.Connected) + if(_isDisposed || _hubConnection is null || _hubConnection.State != HubConnectionState.Connected) return; await _hubConnection.SendAsync(nameof(UpdateHub.BoardChange), Id, _pageId); }🤖 Prompt for 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. In `@Ticky.Web/Components/Pages/BoardView.razor` around lines 511 - 521, The disposal guard only stops rendering, but caller-side work can still continue in HandleUpdate and OnAfterRenderAsync after the component is disposed. Add early _isDisposed checks after awaited calls and before invoking OnDataChanged or any follow-up work in HandleUpdate, and likewise after awaiting HandleUpdate in OnAfterRenderAsync. Use the existing _isDisposed guard pattern in BoardView to ensure teardown stops hub/layout/visit-state updates as well as renders.
🤖 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.
Outside diff comments:
In `@Ticky.Web/Components/Pages/BoardView.razor`:
- Around line 511-521: The disposal guard only stops rendering, but caller-side
work can still continue in HandleUpdate and OnAfterRenderAsync after the
component is disposed. Add early _isDisposed checks after awaited calls and
before invoking OnDataChanged or any follow-up work in HandleUpdate, and
likewise after awaiting HandleUpdate in OnAfterRenderAsync. Use the existing
_isDisposed guard pattern in BoardView to ensure teardown stops
hub/layout/visit-state updates as well as renders.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 82bc1acb-07dc-4b53-aee8-6174accf399d
📒 Files selected for processing (1)
Ticky.Web/Components/Pages/BoardView.razor
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
2 participants
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.
Summary by CodeRabbit