feat: implement native ICS file import feature - #637
Conversation
Sorry, something went wrong.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: c2b315e7-b59e-4c89-b8fb-a8225fd3fdd9 📥 CommitsReviewing files that changed from the base of the PR and between 21947dd and 2c2e8f7. 📒 Files selected for processing (1)
📝 Walkthrough WalkthroughAdds in-app .ics import: a parser service, a GetX controller, and a bottom-sheet UI to pick an .ics file, preview parsed VEVENT/VTODO items with checkboxes, and import selected tasks into Replica or local storage. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant IcsBottomSheet as IcsImport<br/>Bottom Sheet
participant IcsController as IcsImport<br/>Controller
participant FileSystem as File Picker/<br/>File System
participant IcsParser as Ics Parser<br/>Service
participant HomeCtrl as Home<br/>Controller
participant TaskStorage as Task Storage<br/>(Replica or Local)
User->>IcsBottomSheet: Open ICS Import UI
User->>IcsBottomSheet: Tap "Pick File"
IcsBottomSheet->>IcsController: pickAndParseFile()
IcsController->>FileSystem: Show file picker
FileSystem-->>IcsController: Selected .ics content
IcsController->>IcsParser: parseIcsContent(rawIcs)
IcsParser-->>IcsController: List<IcsParsedTask>
IcsController->>HomeCtrl: Check existing tasks (dedup)
IcsController-->>IcsBottomSheet: Update parsedTasks / selectedTasks
User->>IcsBottomSheet: Toggle checkboxes
IcsBottomSheet->>IcsController: toggleSelection(index)
User->>IcsBottomSheet: Tap "Import Selected Tasks"
IcsBottomSheet->>IcsController: importSelectedTasks(context)
alt Replica Mode
IcsController->>TaskStorage: Replica.addTaskToReplica(task)
TaskStorage-->>IcsController: Confirm
else Standard Mode
IcsController->>HomeCtrl: mergeTask(parsedTask)
HomeCtrl->>TaskStorage: Save locally
end
IcsController->>HomeCtrl: Trigger sync/refresh & widget update
IcsController->>IcsBottomSheet: Show success snackbar & close
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem🚥 Pre-merge checks | ✅ 5 ✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. ❤️ ShareComment @coderabbitai help to get the list of available commands and usage tips. |
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)lib/app/modules/ics_import/controllers/ics_import_controller.dart (1)🤖 Prompt for all review comments with AI agentslib/app/modules/ics_import/views/ics_import_bottom_sheet.dart (1)131-134: Add user feedback on import failure for consistency.
Unlike pickAndParseFile() which shows a Get.snackbar on error, importSelectedTasks() only sets errorMessage without visible feedback. Since the sheets may have already been dismissed (or partially), the user won't see the error message in the UI.
♻️ Proposed fix to add error snackbar🤖 Prompt for AI Agents} catch (e) { errorMessage.value = "Import failed: $e"; + Get.snackbar( + 'Import Failed', + 'Could not import tasks. Please try again.', + snackPosition: SnackPosition.BOTTOM, + backgroundColor: Colors.redAccent, + colorText: Colors.white, + ); } finally {Verify each finding against the current code and only fix it if needed. In `@lib/app/modules/ics_import/controllers/ics_import_controller.dart` around lines 131 - 134, importSelectedTasks() currently only sets errorMessage.value on catch, so users may not see the failure if UI was dismissed; mirror pickAndParseFile() by showing a visible error snackbar: inside the catch block of importSelectedTasks() (where errorMessage.value = "Import failed: $e";) call Get.snackbar with a short title and the error string (or use errorMessage.value) and appropriate snackPosition to ensure visibility, then keep setting isLoading.value = false in finally; this will provide consistent user feedback like pickAndParseFile().19-29: Consider guarding against controller collision on rapid open/close.
Registering the controller in initState without a unique tag can cause issues if the bottom sheet is opened, quickly dismissed, and re-opened before dispose fully completes. This could lead to a "Controller already registered" error or stale state.
♻️ Proposed fix using `Get.isRegistered` guard🤖 Prompt for AI Agents`@override` void initState() { super.initState(); - controller = Get.put(IcsImportController(widget.homeController)); + if (Get.isRegistered<IcsImportController>()) { + Get.delete<IcsImportController>(force: true); + } + controller = Get.put(IcsImportController(widget.homeController)); } `@override` void dispose() { - Get.delete<IcsImportController>(); + Get.delete<IcsImportController>(force: true); super.dispose(); }Verify each finding against the current code and only fix it if needed. In `@lib/app/modules/ics_import/views/ics_import_bottom_sheet.dart` around lines 19 - 29, The initState registers IcsImportController with Get.put without a tag, which can cause "Controller already registered" on rapid reopen; modify initState to check Get.isRegistered<IcsImportController>() (or use a unique tag derived from widget.homeController or instance) before calling Get.put(IcsImportController(widget.homeController)), and in dispose only call Get.delete<IcsImportController>() (or delete by the same tag) if the controller is registered to avoid deleting non-existent controllers and to prevent stale registrations; update references to the controller variable accordingly so the controller lookup uses the same registration key (type or tag) in both initState and dispose.
Verify each finding against the current code and only fix it if needed. Inline comments: In `@lib/app/modules/ics_import/controllers/ics_import_controller.dart`: - Around line 125-130: The snackbar is shown after calling Get.back() twice which can use a deactivated BuildContext; move the ScaffoldMessenger.of(context).showSnackBar(...) call to before the Get.back() calls (or replace it with Get.snackbar(...) which doesn't rely on the widget tree) so the message is displayed safely from the importSelectedTasks flow; update the code around Get.back(), ScaffoldMessenger.of(context).showSnackBar, or switch to Get.snackbar(...) accordingly. - Around line 49-53: The deduplication currently builds existingDescriptions from homeController.tasks regardless of storage mode; change it to choose the correct task list based on replica mode (use homeController.tasksFromReplica when importing to Replica and homeController.tasks for TaskChampion). Specifically, update the existingDescriptions assignment (used by selectedTasks.assignAll and the tasks list) to conditionally map the appropriate list (e.g., use homeController.taskReplica.value or the local isReplica flag to decide between homeController.tasksFromReplica.map((t)=>t.description) and homeController.tasks.map((t)=>t.description)) so duplicates are detected against the storage you will actually import into. --- Nitpick comments: In `@lib/app/modules/ics_import/controllers/ics_import_controller.dart`: - Around line 131-134: importSelectedTasks() currently only sets errorMessage.value on catch, so users may not see the failure if UI was dismissed; mirror pickAndParseFile() by showing a visible error snackbar: inside the catch block of importSelectedTasks() (where errorMessage.value = "Import failed: $e";) call Get.snackbar with a short title and the error string (or use errorMessage.value) and appropriate snackPosition to ensure visibility, then keep setting isLoading.value = false in finally; this will provide consistent user feedback like pickAndParseFile(). In `@lib/app/modules/ics_import/views/ics_import_bottom_sheet.dart`: - Around line 19-29: The initState registers IcsImportController with Get.put without a tag, which can cause "Controller already registered" on rapid reopen; modify initState to check Get.isRegistered<IcsImportController>() (or use a unique tag derived from widget.homeController or instance) before calling Get.put(IcsImportController(widget.homeController)), and in dispose only call Get.delete<IcsImportController>() (or delete by the same tag) if the controller is registered to avoid deleting non-existent controllers and to prevent stale registrations; update references to the controller variable accordingly so the controller lookup uses the same registration key (type or tag) in both initState and dispose.
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
Run ID: 36bc1a72-1029-497b-97db-1d15f1595a4b
📥 CommitsReviewing files that changed from the base of the PR and between f058b4a and 7907ebc.
⛔ Files ignored due to path filters (1)- pubspec.lock is excluded by !**/*.lock
- lib/app/data/services/ics_parser_service.dart
- lib/app/modules/home/views/add_task_bottom_sheet_new.dart
- lib/app/modules/ics_import/controllers/ics_import_controller.dart
- lib/app/modules/ics_import/views/ics_import_bottom_sheet.dart
- pubspec.yaml
Sorry, something went wrong.
| // Prevent duplicate imports by default-unchecking tasks that already exist | ||
| final existingDescriptions = homeController.tasks.map((t) => t.description).toSet(); | ||
| selectedTasks.assignAll( | ||
| List.generate(tasks.length, (i) => !existingDescriptions.contains(tasks[i].description)) | ||
| ); |
There was a problem hiding this comment.
⚠️ Potential issue | 🔴 Critical
🧩 Analysis chain🏁 Script executed:
#!/bin/bash
# Check how homeController.tasks is populated and whether it reflects replica tasks
ast-grep --pattern 'tasks = $_'
rg -n 'tasks\s*=' --type=dart -C3 | head -80Repository: CCExtractor/taskwarrior-flutter
Length of output: 4947
🏁 Script executed:
# Find HomeController definition
find . -name "*home*controller*" -type f | head -20Repository: CCExtractor/taskwarrior-flutter
Length of output: 130
🏁 Script executed:
# Read the ics_import_controller.dart file to understand the full context
wc -l lib/app/modules/ics_import/controllers/ics_import_controller.dartRepository: CCExtractor/taskwarrior-flutter
Length of output: 144
🏁 Script executed:
# Look at the ics_import_controller.dart file around the problematic lines and beyond
cat -n lib/app/modules/ics_import/controllers/ics_import_controller.dart | sed -n '25,102p'Repository: CCExtractor/taskwarrior-flutter
Length of output: 3331
🏁 Script executed:
# Read HomeController to see how tasks are managed
cat -n lib/app/modules/home/controllers/home_controller.dart | head -100Repository: CCExtractor/taskwarrior-flutter
Length of output: 5180
🏁 Script executed:
# Search for taskReplica and taskchampion properties in HomeController
rg -n "taskReplica|taskchampion" lib/app/modules/home/controllers/home_controller.dart -B2 -A2Repository: CCExtractor/taskwarrior-flutter
Length of output: 3517
🏁 Script executed:
# Search for Replica class and how it manages tasks
find . -name "*replica*" -type f | grep -E "\.(dart|java|cpp)$"Repository: CCExtractor/taskwarrior-flutter
Length of output: 457
Check deduplication against the correct task list based on storage mode.
The deduplication logic at lines 50–53 only checks homeController.tasks (TaskChampion tasks), but line 79 determines that imports can go to either TaskChampion or Replica storage. When isReplica is true, the code calls Replica.addTaskToReplica() to add tasks to Replica storage, yet deduplication still compares against the wrong list. Duplicates will not be detected when importing to Replica mode because homeController.tasksFromReplica contains the actual existing Replica tasks, not homeController.tasks.
Update the deduplication logic to check homeController.tasksFromReplica when in Replica mode and homeController.tasks for TaskChampion mode, similar to how other parts of HomeController (e.g., allTagsInCurrentTasks getter, getUniqueProjects()) correctly condition on taskReplica.value.
🤖 Prompt for AI AgentsVerify each finding against the current code and only fix it if needed. In `@lib/app/modules/ics_import/controllers/ics_import_controller.dart` around lines 49 - 53, The deduplication currently builds existingDescriptions from homeController.tasks regardless of storage mode; change it to choose the correct task list based on replica mode (use homeController.tasksFromReplica when importing to Replica and homeController.tasks for TaskChampion). Specifically, update the existingDescriptions assignment (used by selectedTasks.assignAll and the tasks list) to conditionally map the appropriate list (e.g., use homeController.taskReplica.value or the local isReplica flag to decide between homeController.tasksFromReplica.map((t)=>t.description) and homeController.tasks.map((t)=>t.description)) so duplicates are detected against the storage you will actually import into.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)lib/app/modules/ics_import/controllers/ics_import_controller.dart (1)🤖 Prompt for all review comments with AI agents98-105: Consider consolidating chained rebuild() calls.
The sequential .rebuild() calls work correctly but could be combined into a single rebuild for slightly cleaner code. This is a minor style preference.
♻️ Optional: Consolidate rebuild calls🤖 Prompt for AI Agents- var rTask = taskParser(task.description) - .rebuild((b) => b..due = task.due?.toUtc()) - .rebuild((p) => p..priority = "M") - .rebuild((p) => p..project = task.project); - if (task.tags.isNotEmpty) { - rTask = rTask.rebuild((t) => t..tags.replace(task.tags)); - } + var rTask = taskParser(task.description).rebuild((b) => b + ..due = task.due?.toUtc() + ..priority = "M" + ..project = task.project + ..tags.replace(task.tags));Verify each finding against the current code and only fix it if needed. In `@lib/app/modules/ics_import/controllers/ics_import_controller.dart` around lines 98 - 105, Consolidate the multiple chained .rebuild() calls on the task produced by taskParser into a single .rebuild(...) call before calling homeController.mergeTask: in the rebuild callback set due = task.due?.toUtc(), priority = "M", and project = task.project, and if task.tags.isNotEmpty call tags.replace(task.tags) inside that same callback so the rTask construction (variable rTask) is done in one rebuild invocation before mergeTask.
Verify each finding against the current code and only fix it if needed. Inline comments: In `@lib/app/modules/ics_import/controllers/ics_import_controller.dart`: - Around line 89-96: The replica import is using task.due directly and ignoring the result of Replica.addTaskToReplica; change the payload to send the due as an ISO8601 UTC string (use task.due.toUtc().toIso8601String() like the non-replica path) and capture the return value of Replica.addTaskToReplica (it returns "scc" or "err"); if the result is not "scc" handle it (log the failure or surface an error/collect failed imports) so failed replica imports are not silently dropped. Ensure you update the HashMap value for "due" and add error handling around the call to Replica.addTaskToReplica. --- Nitpick comments: In `@lib/app/modules/ics_import/controllers/ics_import_controller.dart`: - Around line 98-105: Consolidate the multiple chained .rebuild() calls on the task produced by taskParser into a single .rebuild(...) call before calling homeController.mergeTask: in the rebuild callback set due = task.due?.toUtc(), priority = "M", and project = task.project, and if task.tags.isNotEmpty call tags.replace(task.tags) inside that same callback so the rTask construction (variable rTask) is done in one rebuild invocation before mergeTask.
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
Run ID: 5e376300-220a-4230-8d18-bd82b8332b77
📥 CommitsReviewing files that changed from the base of the PR and between 7907ebc and 21947dd.
📒 Files selected for processing (1)- lib/app/modules/ics_import/controllers/ics_import_controller.dart
Sorry, something went wrong.
Reviewers
At least 1 approving review is required to merge this pull request.
Description
This PR drops in a native .ics (iCalendar) file import feature so users don't have to manually type out every single calendar event or meeting invitation. They can just grab an .ics file right from their phone and import it directly into their Taskwarrior database.
Here is a breakdown of the heavy lifting:
Dependencies Added:
Fixes #636
Screenshots
ics_Issue.mp4Checklist
Summary by CodeRabbit