Improve disposable implementation in BlazorUI components (#9919)#9924
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes update the disposal pattern across multiple UI components within Bit.BlazorUI. Updates include introducing new Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant Component as Component
participant Base as Base Disposal
Caller->>Component: Call Dispose()/DisposeAsync()
Note right of Component: Check if already disposed (IsDisposed flag)
alt Already Disposed or disposing == false
Component-->>Caller: Early return (no action)
else Not Disposed
Component->>Component: Unsubscribe from events, cleanup resources
Component->>Component: Mark as disposed (set flag/property)
Component->>Component: GC.SuppressFinalize(this) [if synchronous]
Component->>Base: Call base.Dispose*/DisposeAsync*
Component-->>Caller: Disposal complete
end
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/BlazorUI/Bit.BlazorUI/Components/Inputs/_Pickers/CircularTimePicker/BitCircularTimePicker.razor.cs (1)
291-306:⚠️ Potential issueDispose the DotNetObjectReference in cleanup.
The
_dotnetObjcreated usingDotNetObjectReference.Create(this)should be disposed during cleanup to prevent memory leaks.Add the disposal to the
DisposeAsyncmethod:protected override async ValueTask DisposeAsync(bool disposing) { if (IsDisposed || disposing is false) return; OnValueChanged -= HandleOnValueChanged; + _dotnetObj?.Dispose(); try { await _js.BitCalloutClearCallout(_calloutId);
🧹 Nitpick comments (8)
src/BlazorUI/Bit.BlazorUI.Extras/Components/ModalService/BitModalContainer.razor.cs (1)
64-72: Optimize the condition order in Dispose(bool).Consider checking
_disposedfirst to potentially avoid thedisposingcheck.- if (disposing is false || _disposed) return; + if (_disposed || disposing is false) return;src/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/BitNavBarOption.cs (1)
90-97: Optimize the condition order in Dispose(bool).Consider checking
_disposedfirst to potentially avoid thedisposingcheck. The use of the null-conditional operator forNavBar?.UnregisterOption(this)is good practice.- if (disposing is false || _disposed) return; + if (_disposed || disposing is false) return;src/BlazorUI/Bit.BlazorUI/Components/Inputs/_Pickers/CircularTimePicker/BitCircularTimePicker.razor.cs (2)
314-318: Consider adding error handling for JS interop during initialization.The JS interop calls during initialization could fail and should be handled gracefully.
Add try-catch blocks:
- await _js.BitSwipesSetup(_calloutId, 0.25m, BitPanelPosition.Top, Dir is BitDir.Rtl, BitSwipeOrientation.Vertical, _dotnetObj); - - _pointerUpAbortControllerId = await _js.BitCircularTimePickerRegisterPointerUp(_dotnetObj, nameof(_HandlePointerUp)); - _pointerMoveAbortControllerId = await _js.BitCircularTimePickerRegisterPointerMove(_dotnetObj, nameof(_HandlePointerMove)); + try + { + await _js.BitSwipesSetup(_calloutId, 0.25m, BitPanelPosition.Top, Dir is BitDir.Rtl, BitSwipeOrientation.Vertical, _dotnetObj); + + _pointerUpAbortControllerId = await _js.BitCircularTimePickerRegisterPointerUp(_dotnetObj, nameof(_HandlePointerUp)); + _pointerMoveAbortControllerId = await _js.BitCircularTimePickerRegisterPointerMove(_dotnetObj, nameof(_HandlePointerMove)); + } + catch (JSException ex) + { + // Log the error or handle it appropriately + }
20-22: Consider using nullable types for abort controller IDs.The abort controller IDs should be nullable to indicate registration failure, which would help prevent null reference exceptions during cleanup.
Apply this diff:
- private string? _pointerUpAbortControllerId; - private string? _pointerMoveAbortControllerId; + private string? _pointerUpAbortControllerId = null; + private string? _pointerMoveAbortControllerId = null;Then update the cleanup code:
try { await _js.BitCalloutClearCallout(_calloutId); await _js.BitSwipesDispose(_calloutId); - await _js.BitCircularTimePickerAbort(_pointerUpAbortControllerId); - await _js.BitCircularTimePickerAbort(_pointerMoveAbortControllerId); + if (_pointerUpAbortControllerId is not null) + await _js.BitCircularTimePickerAbort(_pointerUpAbortControllerId); + if (_pointerMoveAbortControllerId is not null) + await _js.BitCircularTimePickerAbort(_pointerMoveAbortControllerId); }src/BlazorUI/Bit.BlazorUI/Components/Inputs/_Pickers/DateRangePicker/BitDateRangePicker.razor.cs (4)
1868-1893: Consider refactoring time handling logic to improve maintainability.The time handling logic could be simplified by:
- Extracting common time range validation logic into a shared method
- Consolidating AM/PM conversion logic
- Breaking down complex conditions in
CanChangeTimeandIsButtonDisabledinto smaller, more focused methodsExample refactor for time range validation:
+private bool IsTimeRangeValid(TimeSpan startTime, TimeSpan endTime) +{ + if (MaxRange is null) return true; + return new TimeSpan(MaxRange.Value.Hours, MaxRange.Value.Minutes, MaxRange.Value.Seconds) + .TotalMinutes > Math.Abs((startTime - endTime).TotalMinutes); +} private bool CanChangeTime(int? startTimeHour = null, int? startTimeMinute = null, int? endTimeHour = null, int? endTimeMinute = null) { if (MaxRange.HasValue is false) return true; var startTime = new TimeSpan(startTimeHour ?? _startTimeHour, startTimeMinute ?? _startTimeMinute, 0); var endTime = new TimeSpan(endTimeHour ?? _endTimeHour, endTimeMinute ?? _endTimeMinute, 0); var currentValueHasValue = CurrentValue?.StartDate is not null && CurrentValue.EndDate.HasValue; if (currentValueHasValue && CurrentValue!.StartDate!.Value.Date == CurrentValue.EndDate!.Value.Date && startTime > endTime) { return false; } - if (currentValueHasValue) - { - var startDate = ChangeTimeInDateTimeOffset(CurrentValue!.StartDate!.Value, startTimeHour, startTimeMinute); - var endDate = ChangeTimeInDateTimeOffset(CurrentValue!.EndDate!.Value, endTimeHour, endTimeMinute); - var maxDate = new DateTimeOffset(GetMaxEndDate(), CurrentValue!.StartDate.Value.Offset); - var minDate = new DateTimeOffset(GetMinEndDate(), CurrentValue!.StartDate.Value.Offset); - - return startDate >= minDate && endDate <= maxDate; - } - - return new TimeSpan(MaxRange.Value.Hours, MaxRange.Value.Minutes, MaxRange.Value.Seconds).TotalMinutes > Math.Abs((startTime - endTime).TotalMinutes); + return currentValueHasValue + ? IsDateRangeValid(startTimeHour, startTimeMinute, endTimeHour, endTimeMinute) + : IsTimeRangeValid(startTime, endTime); } +private bool IsDateRangeValid(int? startTimeHour, int? startTimeMinute, int? endTimeHour, int? endTimeMinute) +{ + var startDate = ChangeTimeInDateTimeOffset(CurrentValue!.StartDate!.Value, startTimeHour, startTimeMinute); + var endDate = ChangeTimeInDateTimeOffset(CurrentValue!.EndDate!.Value, endTimeHour, endTimeMinute); + var maxDate = new DateTimeOffset(GetMaxEndDate(), CurrentValue!.StartDate.Value.Offset); + var minDate = new DateTimeOffset(GetMinEndDate(), CurrentValue!.StartDate.Value.Offset); + return startDate >= minDate && endDate <= maxDate; +}Also applies to: 1937-2006
1330-1424: Improve date range validation for better maintainability and edge case handling.The date range validation could be improved by:
- Extracting common validation logic into reusable methods
- Adding validation for edge cases like leap years
- Using DateTimeOffset's built-in comparison methods
Example refactor:
+private record struct DateComponents(int Year, int Month, int Day); +private DateComponents GetDateComponents(DateTimeOffset date) +{ + return new DateComponents( + _culture.Calendar.GetYear(date.DateTime), + _culture.Calendar.GetMonth(date.DateTime), + _culture.Calendar.GetDayOfMonth(date.DateTime) + ); +} +private bool IsDateInRange(DateComponents date, DateComponents min, DateComponents max) +{ + if (date.Year < min.Year || date.Year > max.Year) return false; + if (date.Year == min.Year && date.Month < min.Month) return false; + if (date.Year == max.Year && date.Month > max.Month) return false; + if (date.Year == min.Year && date.Month == min.Month && date.Day < min.Day) return false; + if (date.Year == max.Year && date.Month == max.Month && date.Day > max.Day) return false; + return true; +} private bool IsWeekDayOutOfMinAndMaxDate(int dayIndex, int weekIndex) { var day = _daysOfCurrentMonth[weekIndex, dayIndex]; var month = FindMonth(weekIndex, dayIndex); - if (MaxDate.HasValue) - { - var maxDateYear = _culture.Calendar.GetYear(MaxDate.Value.DateTime); - var maxDateMonth = _culture.Calendar.GetMonth(MaxDate.Value.DateTime); - var maxDateDay = _culture.Calendar.GetDayOfMonth(MaxDate.Value.DateTime); - - if (_currentYear > maxDateYear || - (_currentYear == maxDateYear && month > maxDateMonth) || - (_currentYear == maxDateYear && month == maxDateMonth && day > maxDateDay)) return true; - } + var date = new DateComponents(_currentYear, month, day); + var maxDate = MaxDate.HasValue ? GetDateComponents(MaxDate.Value) : default; + var minDate = MinDate.HasValue ? GetDateComponents(MinDate.Value) : default; + if (MaxDate.HasValue && !IsDateInRange(date, default, maxDate)) return true; + if (MinDate.HasValue && !IsDateInRange(date, minDate, default)) return true; // ... rest of the method }
2018-2025: Consider using state pattern for UI state management.The current UI state management using multiple boolean flags is complex and prone to invalid states. Consider:
- Using an enum or state pattern to manage picker states
- Centralizing state transitions
- Adding state validation
Example refactor:
+public enum PickerState +{ + DayPicker, + MonthPicker, + YearPicker, + TimePicker +} +private PickerState _currentState; +private bool _isOverlayVisible; -private void ResetPickersState() +private void ResetPickersState() { - _showMonthPicker = true; - _isMonthPickerOverlayOnTop = false; - _showMonthPickerAsOverlayInternal = ShowMonthPickerAsOverlay; - _isTimePickerOverlayOnTop = false; - _showTimePickerAsOverlayInternal = ShowTimePickerAsOverlay; + _currentState = PickerState.DayPicker; + _isOverlayVisible = false; } -private bool ShowDayPicker() +private bool ShowDayPicker() { - if (ShowTimePicker) - { - if (ShowTimePickerAsOverlay) - { - return _showMonthPickerAsOverlayInternal is false || (_showMonthPickerAsOverlayInternal && _isMonthPickerOverlayOnTop is false && _isTimePickerOverlayOnTop is false); - } - // ... complex conditions - } + return _currentState == PickerState.DayPicker && !_isOverlayVisible; }Also applies to: 1849-1866
1691-1719: Improve event handling for better reliability.The pointer event handling could be improved by:
- Using
usingstatements for CancellationTokenSource- Adding error handling for task cancellation
- Implementing IAsyncDisposable for cleanup
Example refactor:
private async Task HandleOnPointerDown(bool isNext, bool isHour, bool isStartTime) { if (IsEnabled is false) return; await ChangeTime(isNext, isHour, isStartTime); ResetCts(); - var cts = _cancellationTokenSource; - await Task.Run(async () => - { - await InvokeAsync(async () => - { - await Task.Delay(400); - await ContinuousChangeTime(isNext, isHour, isStartTime, cts); - }); - }, cts.Token); + using var cts = new CancellationTokenSource(); + try + { + await Task.Run(async () => + { + await Task.Delay(400, cts.Token); + await InvokeAsync(() => ContinuousChangeTime(isNext, isHour, isStartTime, cts)); + }, cts.Token); + } + catch (OperationCanceledException) + { + // Handle cancellation + } } private async Task ContinuousChangeTime(bool isNext, bool isHour, bool isStartTime, CancellationTokenSource cts) { if (cts.IsCancellationRequested) return; await ChangeTime(isNext, isHour, isStartTime); StateHasChanged(); - await Task.Delay(75); + try + { + await Task.Delay(75, cts.Token); + } + catch (OperationCanceledException) + { + return; + } await ContinuousChangeTime(isNext, isHour, isStartTime, cts); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
src/BlazorUI/Bit.BlazorUI.Extras/Components/ModalService/BitModalContainer.razor.cs(2 hunks)src/BlazorUI/Bit.BlazorUI/Components/Inputs/BitInputBase.cs(2 hunks)src/BlazorUI/Bit.BlazorUI/Components/Inputs/Calendar/BitCalendar.razor.cs(1 hunks)src/BlazorUI/Bit.BlazorUI/Components/Inputs/Checkbox/BitCheckbox.razor.cs(2 hunks)src/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroupOption.cs(1 hunks)src/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdown.razor.cs(2 hunks)src/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdownOption.cs(1 hunks)src/BlazorUI/Bit.BlazorUI/Components/Inputs/NumberField/BitNumberField.razor.cs(1 hunks)src/BlazorUI/Bit.BlazorUI/Components/Inputs/OtpInput/BitOtpInput.razor.cs(1 hunks)src/BlazorUI/Bit.BlazorUI/Components/Inputs/SearchBox/BitSearchBox.razor.cs(1 hunks)src/BlazorUI/Bit.BlazorUI/Components/Inputs/SpinButton/BitSpinButton.razor.cs(1 hunks)src/BlazorUI/Bit.BlazorUI/Components/Inputs/Toggle/BitToggle.razor.cs(1 hunks)src/BlazorUI/Bit.BlazorUI/Components/Inputs/_Pickers/CircularTimePicker/BitCircularTimePicker.razor.cs(2 hunks)src/BlazorUI/Bit.BlazorUI/Components/Inputs/_Pickers/DatePicker/BitDatePicker.razor.cs(2 hunks)src/BlazorUI/Bit.BlazorUI/Components/Inputs/_Pickers/DateRangePicker/BitDateRangePicker.razor.cs(2 hunks)src/BlazorUI/Bit.BlazorUI/Components/Inputs/_Pickers/TimePicker/BitTimePicker.razor.cs(2 hunks)src/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/BitTimelineOption.cs(1 hunks)src/BlazorUI/Bit.BlazorUI/Components/Navs/Breadcrumb/BitBreadcrumbOption.cs(1 hunks)src/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNavOption.razor.cs(1 hunks)src/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/BitNavBarOption.cs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build and test
🔇 Additional comments (23)
src/BlazorUI/Bit.BlazorUI.Extras/Components/ModalService/BitModalContainer.razor.cs (2)
7-7: LGTM!The private
_disposedfield is correctly added to track the disposal state.
58-62: LGTM!The
Disposemethod correctly implements the dispose pattern by callingDispose(true)and suppressing finalization.src/BlazorUI/Bit.BlazorUI/Components/Navs/Breadcrumb/BitBreadcrumbOption.cs (2)
66-70: LGTM!The
Disposemethod correctly implements the dispose pattern by callingDispose(true)and suppressing finalization.
72-79: LGTM!The
Dispose(bool)method correctly implements the disposal logic with optimal condition ordering and proper cleanup.src/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdownOption.cs (2)
83-87: LGTM!The
Disposemethod correctly implements the dispose pattern by callingDispose(true)and suppressing finalization.
89-96: LGTM!The
Dispose(bool)method correctly implements the disposal logic with optimal condition ordering and proper cleanup.src/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/BitNavBarOption.cs (1)
84-88: LGTM!The
Disposemethod correctly implements the dispose pattern by callingDispose(true)and suppressing finalization.src/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/BitTimelineOption.cs (1)
102-115: LGTM! Clean implementation of the dispose pattern.The implementation follows the standard dispose pattern correctly, ensuring proper cleanup by unregistering from the parent component.
src/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroupOption.cs (1)
102-115: LGTM! Proper implementation of the dispose pattern.The implementation correctly handles cleanup by unregistering from the parent component.
src/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNavOption.razor.cs (1)
147-167: LGTM! Thorough implementation of the dispose pattern.The implementation handles both registration paths correctly:
- Unregisters from Nav when there's no parent
- Removes itself from parent's ChildItems when there is a parent
src/BlazorUI/Bit.BlazorUI/Components/Inputs/Toggle/BitToggle.razor.cs (1)
158-165: LGTM! Complete cleanup with event unsubscription.The implementation:
- Properly checks disposal state
- Correctly unsubscribes from the OnValueChanged event
- Calls base.Dispose for additional cleanup
src/BlazorUI/Bit.BlazorUI/Components/Inputs/Checkbox/BitCheckbox.razor.cs (1)
8-8: LGTM! Improved disposal pattern implementation.The changes enhance resource management by:
- Removing redundant
IDisposableinterface since it's already implemented by the base class- Adding proper disposal state checks
- Following correct cleanup order: unsubscribe from events before calling base disposal
Also applies to: 247-254
src/BlazorUI/Bit.BlazorUI/Components/Inputs/OtpInput/BitOtpInput.razor.cs (1)
432-440: LGTM! Improved async disposal pattern implementation.The changes enhance resource management by:
- Adding proper disposal state checks
- Implementing robust cleanup order
- Adding proper exception handling for JS disconnection scenarios
src/BlazorUI/Bit.BlazorUI/Components/Inputs/SearchBox/BitSearchBox.razor.cs (1)
460-480: LGTM! Improved async disposal pattern implementation.The changes enhance resource management by:
- Adding proper disposal state checks
- Implementing robust cleanup order
- Adding proper null checks and exception handling
- Ensuring proper cleanup of all resources (DotNetObjectReference, JS resources, cancellation token, event handlers)
src/BlazorUI/Bit.BlazorUI/Components/Inputs/SpinButton/BitSpinButton.razor.cs (1)
525-532: LGTM! Improved disposal pattern implementation.The changes enhance resource management by:
- Adding proper disposal state checks
- Implementing robust cleanup order
- Ensuring proper cleanup of cancellation token source
src/BlazorUI/Bit.BlazorUI/Components/Inputs/BitInputBase.cs (1)
483-508: Well-implemented disposal pattern!The disposal implementation follows best practices:
- Prevents multiple disposals with
IsDisposedcheck- Properly implements the dispose pattern with
Dispose(bool disposing)- Correctly suppresses finalization
- Safely unsubscribes from event handlers
- Provides async disposal support
src/BlazorUI/Bit.BlazorUI/Components/Inputs/_Pickers/TimePicker/BitTimePicker.razor.cs (1)
665-680: Robust disposal implementation with proper exception handling!The disposal implementation:
- Prevents multiple disposals
- Safely cleans up JS interop resources
- Properly handles
JSDisconnectedException- Correctly calls base class disposal
src/BlazorUI/Bit.BlazorUI/Components/Inputs/NumberField/BitNumberField.razor.cs (1)
591-598: Clean disposal implementation!The disposal implementation:
- Prevents multiple disposals
- Properly disposes of the cancellation token source
- Correctly calls base class disposal
src/BlazorUI/Bit.BlazorUI/Components/Inputs/Calendar/BitCalendar.razor.cs (1)
1118-1126: Thorough disposal implementation!The disposal implementation:
- Prevents multiple disposals
- Properly disposes of the cancellation token source
- Safely unsubscribes from event handler
- Correctly calls base class disposal
src/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdown.razor.cs (1)
1471-1483: LGTM! Well-implemented disposal pattern.The disposal implementation follows best practices:
- Checks disposal state before proceeding
- Safely cleans up JS interop resources
- Properly handles JSDisconnectedException
- Calls base class disposal
src/BlazorUI/Bit.BlazorUI/Components/Inputs/_Pickers/DatePicker/BitDatePicker.razor.cs (1)
1526-1541: LGTM! Comprehensive disposal implementation.The disposal implementation is thorough and follows best practices:
- Checks disposal state before proceeding
- Properly cleans up managed resources (CancellationTokenSource)
- Unsubscribes from event handlers
- Safely cleans up JS interop resources
- Properly handles JSDisconnectedException
- Calls base class disposal
src/BlazorUI/Bit.BlazorUI/Components/Inputs/_Pickers/CircularTimePicker/BitCircularTimePicker.razor.cs (1)
701-717: LGTM! The disposal pattern implementation follows best practices.The implementation properly handles resource cleanup by:
- Checking disposal state
- Unsubscribing from events
- Cleaning up JS interop resources
- Handling JS disconnection exceptions
- Calling base class disposal
src/BlazorUI/Bit.BlazorUI/Components/Inputs/_Pickers/DateRangePicker/BitDateRangePicker.razor.cs (1)
2068-2083: LGTM! The disposal pattern implementation follows best practices.The changes improve the disposal pattern by:
- Using
IsDisposedproperty for better encapsulation- Preventing multiple disposals
- Ensuring proper cleanup by calling base disposal
- Appropriately handling JSDisconnectedException
2 participants
closes #9919
Summary by CodeRabbit