Fix default download directory and persist built-in aria2 sessions#75
Conversation
|
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 (2)
✅ Files skipped from review due to trivial changes (2)
📝 WalkthroughWalkthroughAsync default-download resolution added; path handling switched to package:path; Settings now normalizes and persists downloadDir defaults; Aria2 RPC gained a shutdown method; builtin instance stop attempts RPC save+shutdown with timed waits and kills; session saves are triggered for instances with tasks that transition to terminal states. ChangesInstance lifecycle, settings, and session persistence
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Review rate limit: 9/10 reviews remaining, refill in 6 minutes. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/services/builtin_instance_service.dart (1)
106-124: 💤 Low valueSynchronous
_defaultDownloadDir()duplicates logic fromsettings.dart.This method mirrors
_defaultDownloadDirectory()inlib/models/settings.dartbut is synchronous (doesn't usepath_provider). Consider extracting a shared utility to avoid drift between the two implementations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/services/builtin_instance_service.dart` around lines 106 - 124, Duplicate logic exists between the synchronous _defaultDownloadDir() and the async _defaultDownloadDirectory() (which uses path_provider); extract the shared decision logic into a single utility (e.g., getDefaultDownloadDirectorySync or a sync helper) and have _defaultDownloadDir() call that helper while refactoring the async _defaultDownloadDirectory() to reuse the same logic (or call a common async wrapper that uses path_provider when available). Update references in BuiltinInstanceService._defaultDownloadDir() and the settings model's _defaultDownloadDirectory() to call the new shared utility so the behavior stays consistent and avoids drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/services/builtin_instance_service.dart`:
- Around line 106-124: Duplicate logic exists between the synchronous
_defaultDownloadDir() and the async _defaultDownloadDirectory() (which uses
path_provider); extract the shared decision logic into a single utility (e.g.,
getDefaultDownloadDirectorySync or a sync helper) and have _defaultDownloadDir()
call that helper while refactoring the async _defaultDownloadDirectory() to
reuse the same logic (or call a common async wrapper that uses path_provider
when available). Update references in
BuiltinInstanceService._defaultDownloadDir() and the settings model's
_defaultDownloadDirectory() to call the new shared utility so the behavior stays
consistent and avoids drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6df8c363-dce3-40d5-a612-4db79a1246d8
📒 Files selected for processing (4)
lib/models/settings.dartlib/services/aria2_rpc_client.dartlib/services/builtin_instance_service.dartlib/services/download_data_service.dart
Summary
saveSession/shutdownRPC support for the built-in instance flow and normalize config/data file path handling.Testing
dart formaton the modified Dart files.dart analyzeon the modified Dart files: passed.flutter test test\download_task_service_test.dart: passed.flutter analyze: passed for the app code; one existing info-level lint remains inpackages/fl_liboutside this change.Summary by CodeRabbit
Bug Fixes
Improvements
New Features