Refactor 4#80
Conversation
Bug fixes: - SystemTrayService.updateMenuState now rebuilds tray context menu after updating labels (menu was stale until next right-click) - BuiltinInstanceService.dispose() explicitly discards stopInstance() future instead of fire-and-forget, and removes duplicate _upnpService.shutdown() call (stopInstance() already handles UPnP shutdown) - AppearanceDialog: await async setThemeMode/setPrimaryColor calls so try-catch blocks can actually catch failures; add mounted guard before showing SnackBar after await - SettingsPage locale dialog: await setLocale() before Navigator.pop so save failures are not silently swallowed; add mounted guard - DebugProvider: replace in-place VNode list mutation (add/removeRange/clear) with immutable list reassignment to maintain VNode setter contract - Input: add didUpdateWidget to sync _obscureText when parent changes obscureText prop - App: remove duplicate loadSettings() call from _ThemeProviderState.initState (HomeWrapper._initialize already handles loading with proper await)
Performance: - Refactor _getConfiguredRpcPort/Secret to accept optional settings map parameter, eliminating 2 redundant _readSettingsSnapshot() calls in both _buildArgs() and getBuiltinInstanceConfig() (6 sync file reads → 4) - Remove unreachable duplicate guards in SystemTrayService.initialize() (dead code after in-flight await)
Code quality: - SettingsPage._buildSettingsGroup: remove no-op identity map (.map((child) => child)) - Settings._settingsFileName: change from final to static const - Settings: inline trivial _getDataDirectory() wrapper (single caller) - CustomAppBar/CardX: remove redundant key forwarding to inner widget (Flutter handles key reconciliation on the outermost widget) - App: extract _swapListener<T> helper to deduplicate 3 near-identical listener swap blocks in didChangeDependencies
Cleanup: - Remove unused dart:async import in AutoHideWindowService - Fix _formatVersionLabel to display full version string (was showing only patch number, e.g. 'v3' instead of 'v1.2.3') - Remove redundant inline comments on enum values in enums.dart (comments restated the value names) - Simplify VirtualWindowFrame switch to a single conditional expression (first and third switch arms produced identical output)
Bug fixes: - DebugProvider: trim widgets.value alongside lines and _widgetCounts (widgets list was growing unboundedly while lines was capped at 100) - Aria2RpcClient._handleWebSocketMessage: add type guard for non-Map JSON responses (arrays/primitives caused NoSuchMethodError) - Aria2RpcClient._callHttpRpc: replace _httpClient! force-unwrap with null check + ConnectionFailedException (prevents crash after close()) - Aria2RpcClient: store WebSocket StreamSubscription in _webSocketSubscription field; cancel in close() and _connectWebSocket to prevent callbacks firing on disposed client - DownloadPage._pruneSelection: skip setState when no keys were removed (prevents unnecessary rebuild cycles on every data change)
Performance: - DownloadPage._filterTasks: combine category filter, status/type filter, and search filter into a single retainWhere pass (was creating 3-5 intermediate lists via .where().toList() per filter call) - DownloadPage._filterTasks: inline sort key computation in comparator (eliminates O(n) sortKeys map allocation per sort) - TaskDetailsBtHelpers._buildPiecesGrid: replace Wrap+List.generate with CustomPaint painter (renders thousands of pieces without creating individual Container widgets, eliminates O(n) widget allocation)
…odels
Code quality:
- Aria2RpcClient._callHttpRpc: remove redundant response.body.contains
check for 'Unauthorized' (structured JSON check is sufficient)
- Aria2RpcClient.getVersion: delegate to getVersionInfo() to eliminate
duplicate RPC call
- Aria2RpcClient._handleWebSocketError: always wrap errors in
ConnectionFailedException instead of leaking raw error types
- AddTaskDialog: move outputField creation inside two-column branch
(was allocated but unused in single-column path)
- DownloadTask: add key getter ('::') to eliminate
duplicate _taskKey function in download_page.dart and task_list_view.dart
- FilterSelector: inline trivial _getInstanceFilterOptions wrapper
Cleanup:
- DownloadTaskService: inline trivial _stoppingSeedingTip and
_failedToStopSeedingMessage wrappers (single-line l10n accessors)
- task_utils: use Uri.file() instead of Uri.parse('file://...') for
correct platform-specific file URI construction (handles paths with
spaces and special characters on Linux/macOS)
- TaskDetailsBtHelpers: add explanatory comment to catch(_) block in
parseTorrentMetadata (best-effort parsing returns empty on malformed
torrent data)
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughThis PR consolidates infrastructure and refactoring improvements across the download page, app lifecycle, RPC client, and UI kit. Task selection logic is shifted to use a new composite ChangesDownload Selection and Service Improvements
Possibly Related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
lib/pages/download_page/services/download_task_service.dart (1)
1-669:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winApply Dart formatting to pass CI.
flutter analysisindicates this file would be changed by formatter. Please rundart format lib/pages/download_page/services/download_task_service.dart(orflutter format .).As per coding guidelines, "Use
flutter format .for automatic code formatting with max line length of 80 characters."🤖 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 `@lib/pages/download_page/services/download_task_service.dart` around lines 1 - 669, Run the Dart formatter on the file to satisfy CI: execute `dart format lib/pages/download_page/services/download_task_service.dart` (or `flutter format .`) to apply the project's formatting rules (max line length 80); this will reflow imports, line breaks and indentation for the DownloadTaskService class and its functions (e.g. deleteTaskWithClient, _deleteDownloadedFiles, _cleanupEmptyDirectories, getStatusInfo) so no manual code changes are needed—commit the formatted file.Sources: Coding guidelines, Pipeline failures
lib/pages/download_page/download_page.dart (1)
1-1034:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFormat this file to satisfy the analysis check.
The pipeline indicates
dart formatwould modify this file. Please rundart format lib/pages/download_page/download_page.dart(orflutter format .).As per coding guidelines, "Use
flutter format .for automatic code formatting with max line length of 80 characters."🤖 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 `@lib/pages/download_page/download_page.dart` around lines 1 - 1034, This file fails the formatter; run the Flutter/Dart formatter to fix style issues across DownloadPage and its state (e.g., class DownloadPage, DownloadPageState, and _SelectionToolbar) — run "flutter format ." (or "dart format lib/pages/download_page/download_page.dart") in the repo root to apply automatic formatting with the project's line-length rules (max 80 chars), then re-run the analysis checks and commit the formatted changes.Sources: Coding guidelines, Pipeline failures
lib/pages/download_page/components/task_details_bt_helpers.dart (1)
1-524:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRun formatter for this file before merge.
The pipeline reports this file is not formatted. Please run
dart format lib/pages/download_page/components/task_details_bt_helpers.dart(orflutter format .).As per coding guidelines, "Use
flutter format .for automatic code formatting with max line length of 80 characters."🤖 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 `@lib/pages/download_page/components/task_details_bt_helpers.dart` around lines 1 - 524, The file containing TaskDetailsBtHelpers (classes TaskDetailsBtHelpers, _PiecesGridPainter, TaskDetailsTorrentOverviewMetadata and methods like buildBitfieldVisualization, buildPeersView, parseTorrentMetadata, _formatPeerVersion) is not formatted; run the project formatter (flutter format . or dart format) to apply the standard style (max line length 80) and re-run the pipeline so the spacing, imports and line breaks around these symbols are normalized before merging.Sources: Coding guidelines, Pipeline failures
lib/pages/download_page/enums.dart (1)
1-31:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRun formatter to unblock CI.
flutter analysisreports this file would be reformatted. Please rundart format lib/pages/download_page/enums.dart(orflutter format .) before merge.As per coding guidelines, "Use
flutter format .for automatic code formatting with max line length of 80 characters."🤖 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 `@lib/pages/download_page/enums.dart` around lines 1 - 31, This file's formatting is off; run the Dart/Flutter formatter to fix style for the enum declarations (DownloadStatus, CategoryType, FilterOption, TaskSortOption) — run `dart format lib/pages/download_page/enums.dart` or `flutter format .` (with project max line length 80) and commit the reformatted file so CI passes.Sources: Coding guidelines, Pipeline failures
lib/pages/settings_page/components/appearance_dialog.dart (1)
269-309:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd error handling to the G/B async color-save callbacks.
The
GandBonChangeEndhandlers awaitsetPrimaryColorwithouttry/catch, unlike theRhandler. This leaves async failures unhandled in UI callbacks.Proposed fix
onChangeEnd: (value) async { final newColor = Color.fromRGBO( (_selectedColor.r * 255.0).round() & 0xff, value.toInt(), (_selectedColor.b * 255.0).round() & 0xff, 1.0, ); - await widget.settings.setPrimaryColor( - newColor, - isCustom: true, - ); + try { + await widget.settings.setPrimaryColor( + newColor, + isCustom: true, + ); + } catch (e) { + if (!mounted) return; + ScaffoldMessenger.of(context).showSnackBar( + SnackBar( + content: Text( + l10n.failedToSetCustomThemeColor('$e'), + ), + ), + ); + } }, @@ onChangeEnd: (value) async { final newColor = Color.fromRGBO( (_selectedColor.r * 255.0).round() & 0xff, (_selectedColor.g * 255.0).round() & 0xff, value.toInt(), 1.0, ); - await widget.settings.setPrimaryColor( - newColor, - isCustom: true, - ); + try { + await widget.settings.setPrimaryColor( + newColor, + isCustom: true, + ); + } catch (e) { + if (!mounted) return; + ScaffoldMessenger.of(context).showSnackBar( + SnackBar( + content: Text( + l10n.failedToSetCustomThemeColor('$e'), + ), + ), + ); + } },As per coding guidelines: "Use
async/awaitfor asynchronous code instead of raw Futures, and handle async errors with try-catch."🤖 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 `@lib/pages/settings_page/components/appearance_dialog.dart` around lines 269 - 309, The G and B slider onChangeEnd handlers call await widget.settings.setPrimaryColor without error handling; wrap each async call in a try/catch to mirror the R handler: inside the onChangeEnd for the 'G' and for the 'B' _buildColorSlider callbacks, perform the await widget.settings.setPrimaryColor(newColor, isCustom: true) inside a try { ... } catch (e, st) { widget.settingsLogger?.error(...) or print/handle the error } and (optionally) restore UI state or show a user-facing error if needed; reference the _selectedColor, widget.settings.setPrimaryColor, and the onChangeEnd closures to locate the exact spots to modify.Source: Coding guidelines
lib/pages/settings_page/settings_page.dart (1)
726-751:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winWrap async locale persistence in try/catch before closing the dialog.
These three async
onTaphandlers awaitsetLocalebut don't catch errors. Failures can escape the callback and skip user feedback.Proposed fix
onTap: () async { - await settings.setLocale(null); + try { + await settings.setLocale(null); + } catch (e, stackTrace) { + this.e('Failed to set locale', error: e, stackTrace: stackTrace); + if (!context.mounted) return; + _showErrorSnackBar(l10n.saveSettingsFailed); + return; + } if (!context.mounted) return; Navigator.pop(context); }, @@ onTap: () async { - await settings.setLocale(const Locale('en')); + try { + await settings.setLocale(const Locale('en')); + } catch (e, stackTrace) { + this.e('Failed to set locale', error: e, stackTrace: stackTrace); + if (!context.mounted) return; + _showErrorSnackBar(l10n.saveSettingsFailed); + return; + } if (!context.mounted) return; Navigator.pop(context); }, @@ onTap: () async { - await settings.setLocale(const Locale('zh')); + try { + await settings.setLocale(const Locale('zh')); + } catch (e, stackTrace) { + this.e('Failed to set locale', error: e, stackTrace: stackTrace); + if (!context.mounted) return; + _showErrorSnackBar(l10n.saveSettingsFailed); + return; + } if (!context.mounted) return; Navigator.pop(context); },As per coding guidelines: "Use
async/awaitfor asynchronous code instead of raw Futures, and handle async errors with try-catch."🤖 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 `@lib/pages/settings_page/settings_page.dart` around lines 726 - 751, Each async onTap handler that calls settings.setLocale (the three closures containing await settings.setLocale(...)) should wrap the await in a try/catch so errors are caught before closing the dialog; call await settings.setLocale(...) inside try, check if (!context.mounted) return, then Navigator.pop(context) on success, and handle the error in catch (e.g., show an error UI or log) without allowing the exception to escape; reference the onTap closures, the settings.setLocale calls, context.mounted checks, and Navigator.pop to locate and update all three handlers.Source: Coding guidelines
lib/services/system_tray_service.dart (1)
1-345:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRun
dart formaton this file before merge.CI is currently failing because formatting is out of sync for this file.
As per coding guidelines, "Use
flutter format .for automatic code formatting with max line length of 80 characters."🤖 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 `@lib/services/system_tray_service.dart` around lines 1 - 345, The file lib/services/system_tray_service.dart is out of formatter sync; run the project formatter (e.g., flutter format . or dart format .) and reformat this file (containing the SystemTrayService class) to comply with the project's 80-column rule, then stage and commit the formatted changes so CI passes.Sources: Coding guidelines, Pipeline failures
🤖 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 `@lib/kit/widgets/virtual_window_frame.dart`:
- Around line 25-34: The file fails dart formatting; run the Dart formatter
(flutter format . or dart format) targeting this file to fix line breaks and max
line length (80) so the conditional assignment creating content (involving
CustomAppBar.sysStatusBarHeight, WindowFrameConfig.showCaption, _WindowCaption
and Expanded) matches project style; ensure the resulting code compiles and
update the PR with the formatted file.
In `@lib/pages/download_page/components/task_details_bt_helpers.dart`:
- Around line 428-437: Guard against an empty pieces list inside the
LayoutBuilder builder before computing geometry: at the top of the builder
closure (before calculating pieceSize/spacing/cols/rows/gridHeight) check if
pieces.isEmpty and immediately return a zero-size widget (e.g.,
SizedBox.shrink()) so you don't call .clamp(1, pieces.length) with pieces.length
== 0 or compute rows via division by zero; update the closure that defines
pieceSize, spacing, cols, rows, gridHeight to be after this early-return guard.
In `@lib/services/aria2_rpc_client.dart`:
- Around line 198-209: In _connectWebSocket(), await the previous subscription
cancellation before tearing down and creating a new listener: call and await the
Future returned by _webSocketSubscription?.cancel() (or assign it to a variable
and await it) prior to setting _webSocketSubscription/_webSocket to null and
before calling _webSocket!.listen(_handleWebSocketMessage); this ensures any
pending onDone/onError handlers (_handleWebSocketDone/_handleWebSocketError)
from the old subscription finish before the new subscription is attached and
prevents race conditions clearing _webSocket or _pendingRequests for the new
connection.
- Around line 80-85: Guard decoded JSON shapes and await websocket cancellation:
in _callHttpRpc, after jsonDecode(response.body) verify the decoded value is a
Map<String, dynamic> before using containsKey or indexing into data['error'],
and ensure data['error'] is a Map with a String 'message' key before comparing
to 'Unauthorized' so malformed/non-map payloads don't throw; map unexpected
shapes to a generic RPC/parse error or rethrow wrapped in UnauthorizedException
where appropriate. In _handleWebSocketMessage, check that the parsed data is a
Map and that data['error'] is a Map with a 'message' String before accessing
data['error']['message'], and ensure any path that can leave the related
Completer unresolved instead completesError or completes with an error to avoid
leaked pending completers. In _connectWebSocket, await the result of
_webSocketSubscription?.cancel() (i.e., await the Future) before replacing the
subscription to avoid races when swapping listeners.
---
Outside diff comments:
In `@lib/pages/download_page/components/task_details_bt_helpers.dart`:
- Around line 1-524: The file containing TaskDetailsBtHelpers (classes
TaskDetailsBtHelpers, _PiecesGridPainter, TaskDetailsTorrentOverviewMetadata and
methods like buildBitfieldVisualization, buildPeersView, parseTorrentMetadata,
_formatPeerVersion) is not formatted; run the project formatter (flutter format
. or dart format) to apply the standard style (max line length 80) and re-run
the pipeline so the spacing, imports and line breaks around these symbols are
normalized before merging.
In `@lib/pages/download_page/download_page.dart`:
- Around line 1-1034: This file fails the formatter; run the Flutter/Dart
formatter to fix style issues across DownloadPage and its state (e.g., class
DownloadPage, DownloadPageState, and _SelectionToolbar) — run "flutter format ."
(or "dart format lib/pages/download_page/download_page.dart") in the repo root
to apply automatic formatting with the project's line-length rules (max 80
chars), then re-run the analysis checks and commit the formatted changes.
In `@lib/pages/download_page/enums.dart`:
- Around line 1-31: This file's formatting is off; run the Dart/Flutter
formatter to fix style for the enum declarations (DownloadStatus, CategoryType,
FilterOption, TaskSortOption) — run `dart format
lib/pages/download_page/enums.dart` or `flutter format .` (with project max line
length 80) and commit the reformatted file so CI passes.
In `@lib/pages/download_page/services/download_task_service.dart`:
- Around line 1-669: Run the Dart formatter on the file to satisfy CI: execute
`dart format lib/pages/download_page/services/download_task_service.dart` (or
`flutter format .`) to apply the project's formatting rules (max line length
80); this will reflow imports, line breaks and indentation for the
DownloadTaskService class and its functions (e.g. deleteTaskWithClient,
_deleteDownloadedFiles, _cleanupEmptyDirectories, getStatusInfo) so no manual
code changes are needed—commit the formatted file.
In `@lib/pages/settings_page/components/appearance_dialog.dart`:
- Around line 269-309: The G and B slider onChangeEnd handlers call await
widget.settings.setPrimaryColor without error handling; wrap each async call in
a try/catch to mirror the R handler: inside the onChangeEnd for the 'G' and for
the 'B' _buildColorSlider callbacks, perform the await
widget.settings.setPrimaryColor(newColor, isCustom: true) inside a try { ... }
catch (e, st) { widget.settingsLogger?.error(...) or print/handle the error }
and (optionally) restore UI state or show a user-facing error if needed;
reference the _selectedColor, widget.settings.setPrimaryColor, and the
onChangeEnd closures to locate the exact spots to modify.
In `@lib/pages/settings_page/settings_page.dart`:
- Around line 726-751: Each async onTap handler that calls settings.setLocale
(the three closures containing await settings.setLocale(...)) should wrap the
await in a try/catch so errors are caught before closing the dialog; call await
settings.setLocale(...) inside try, check if (!context.mounted) return, then
Navigator.pop(context) on success, and handle the error in catch (e.g., show an
error UI or log) without allowing the exception to escape; reference the onTap
closures, the settings.setLocale calls, context.mounted checks, and
Navigator.pop to locate and update all three handlers.
In `@lib/services/system_tray_service.dart`:
- Around line 1-345: The file lib/services/system_tray_service.dart is out of
formatter sync; run the project formatter (e.g., flutter format . or dart format
.) and reformat this file (containing the SystemTrayService class) to comply
with the project's 80-column rule, then stage and commit the formatted changes
so CI passes.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 52523a9c-6f93-4422-ad80-b919d11f36be
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
lib/app.dartlib/kit/provider/debug.dartlib/kit/widgets/appbar.dartlib/kit/widgets/card.dartlib/kit/widgets/input.dartlib/kit/widgets/virtual_window_frame.dartlib/models/settings.dartlib/pages/download_page/components/add_task_dialog.dartlib/pages/download_page/components/filter_selector.dartlib/pages/download_page/components/task_details_bt_helpers.dartlib/pages/download_page/components/task_list_view.dartlib/pages/download_page/download_page.dartlib/pages/download_page/enums.dartlib/pages/download_page/models/download_task.dartlib/pages/download_page/services/download_task_service.dartlib/pages/download_page/utils/task_utils.dartlib/pages/settings_page/components/appearance_dialog.dartlib/pages/settings_page/settings_page.dartlib/services/aria2_rpc_client.dartlib/services/auto_hide_window_service.dartlib/services/builtin_instance_service.dartlib/services/system_tray_service.dart
💤 Files with no reviewable changes (3)
- lib/kit/widgets/appbar.dart
- lib/services/auto_hide_window_service.dart
- lib/kit/widgets/card.dart
Bug fixes: - TaskDetailsBtHelpers._buildPiecesGrid: add empty pieces guard to prevent ArgumentError from .clamp(1, 0) when pieces list is empty - Aria2RpcClient._callHttpRpc: guard data['error'] is Map before accessing ['message'] to prevent TypeError on malformed responses - Aria2RpcClient._handleWebSocketMessage: same guard for data['error'] is Map before accessing ['message'] - AppearanceDialog: add try-catch to G and B slider onChangeEnd handlers (R slider already had error handling, G/B were missing) Formatting: - Run dart format on 7 files to comply with project 80-column style: virtual_window_frame, task_details_bt_helpers, download_page, enums, download_task_service, system_tray_service, aria2_rpc_client Skipped findings (not valid): - _connectWebSocket await subscription cancel: cancel() returns synchronously for non-pending operations, no race condition risk - settings_page setLocale try-catch: already has await + context.mounted guard, setLocale failures propagate as unhandled which is acceptable
Summary by CodeRabbit
Bug Fixes
New Features
Performance Improvements
Improvements