fix: ensure atomic DB updates for tasks and tags#644
fix: ensure atomic DB updates for tasks and tags#644sahu-virendra-1908 wants to merge 2 commits intoCCExtractor:mainfrom
Conversation
📝 WalkthroughWalkthroughUpdated Flutter version pins; added/updated native plugin registrations for Linux, macOS, and Windows; and refactored task database edits so task fields and tags are updated atomically inside a single SQLite transaction. Many files received formatting/whitespace adjustments. Changes
Sequence Diagram(s)mermaid (Note: rectangles omitted; primary flow between App, TaskDatabase, and SQLite transaction shown.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/app/v3/db/task_database.dart`:
- Line 279: The line assigning taskId uses an unsafe cast (final taskId =
taskMaps.first['id'] as int;) which will throw if id is null; change it to a
null-safe cast and handle the null case explicitly (e.g. final int? taskId =
taskMaps.first['id'] as int?; then either throw a descriptive error like throw
StateError('Missing task id for taskMap: $taskMaps.first') or handle/return a
nullable value as appropriate), and update any downstream code that assumes
taskId is non-null to account for the new nullable type or the explicit error
path.
- Around line 282-301: The current conditional prevents clearing tags because
the txn.delete call is only run when newTags.isNotEmpty; always perform the
delete of the 'Tags' rows for the given task (use the existing txn.delete with
where 'task_uuid = ? AND task_id = ?' and whereArgs [uuid, taskId]) before
inserting any new tags, and then only iterate/insert when newTags contains
non-empty values (preserve the tag.trim().isNotEmpty check) so an empty newTags
list will correctly clear existing tags.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 070ad8b6-0307-4758-8f46-445f4c31f93e
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
.fvmrc.vscode/settings.jsonlib/app/v3/db/task_database.dartlinux/flutter/generated_plugin_registrant.cclinux/flutter/generated_plugins.cmakemacos/Flutter/GeneratedPluginRegistrant.swiftwindows/flutter/generated_plugin_registrant.ccwindows/flutter/generated_plugins.cmake
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
lib/app/v3/db/task_database.dart (1)
253-317: Please add a rollback regression test for this transaction path.Given this is a data-integrity fix, add at least one test that forces failure during tag insert and asserts task fields and tags both remain unchanged after rollback.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/app/v3/db/task_database.dart` around lines 253 - 317, Add a regression test that verifies the transaction rolls back when tag insertion fails: create a fixture task row in the Tasks table with initial fields and associated Tags, then call the transaction wrapper (the update method that contains the shown transaction code—e.g., updateTask) with new field values and newTags that will force a failure during tag insertion (for example, include a tag value that triggers a DB constraint error or arrange a test double to throw when inserting into 'Tags'); catch the thrown exception, then query the 'Tasks' row (columns: description, project, status, priority, due, modified) and the 'Tags' for that task_uuid/task_id and assert they match the original pre-update values—this ensures both the Tasks and Tags tables were rolled back on failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/app/utils/themes/theme_extension.dart`:
- Line 82: The current code always passes the starting IconData via "icons:
icons" so the visual doesn't flip during the theme animation; change the
property to pick between the start and end icon at the animation midpoint (e.g.
use the animation's value or t and set icons: (t.value < 0.5) ? startIcon :
endIcon). Update the call site that currently sets "icons: icons" to instead
reference the start/end icon variables (e.g. beginIcon / endIcon) and switch
when t >= 0.5 so the icon swaps at the midpoint of the transition.
- Around line 42-56: The copyWith implementation for TaskwarriorColorTheme
incorrectly uses parameterName ?? parameterName for all fields so it never falls
back to existing instance values; update each null-coalescing expression to use
the incoming parameter or this.field (e.g., dialogBackgroundColor ??
this.dialogBackgroundColor, primaryBackgroundColor ??
this.primaryBackgroundColor, etc.), and add purpleShade to the copyWith method
signature so it can be overridden (ensure the returned TaskwarriorColorTheme
uses purpleShade ?? this.purpleShade and any other fields similarly).
In `@lib/app/v3/db/task_database.dart`:
- Around line 270-290: The new saveEditedTaskInDB method now throws when a task
is missing or the id is null; update its caller(s) to catch those exceptions and
surface user-friendly feedback instead of letting them bubble as unhandled async
errors. Wrap the await call to saveEditedTaskInDB inside saveTask() (and
similarly in handleWillPop() if it calls saveEditedTaskInDB) with a try-catch
that catches Exception/StateError, logs the error, updates any UI state (e.g.,
stop loading spinner), and shows an error message to the user (toast/dialog) or
falls back to a safe path; ensure you reference saveEditedTaskInDB, saveTask,
and handleWillPop when locating where to add the try-catch and cleanup.
---
Nitpick comments:
In `@lib/app/v3/db/task_database.dart`:
- Around line 253-317: Add a regression test that verifies the transaction rolls
back when tag insertion fails: create a fixture task row in the Tasks table with
initial fields and associated Tags, then call the transaction wrapper (the
update method that contains the shown transaction code—e.g., updateTask) with
new field values and newTags that will force a failure during tag insertion (for
example, include a tag value that triggers a DB constraint error or arrange a
test double to throw when inserting into 'Tags'); catch the thrown exception,
then query the 'Tasks' row (columns: description, project, status, priority,
due, modified) and the 'Tags' for that task_uuid/task_id and assert they match
the original pre-update values—this ensures both the Tasks and Tags tables were
rolled back on failure.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: aa15fc1f-4350-4589-ba96-c6ef08ad4849
📒 Files selected for processing (62)
lib/app/models/data.dartlib/app/models/filters.dartlib/app/models/json/annotation.dartlib/app/models/json/serializers.dartlib/app/models/json/task.dartlib/app/models/models.dartlib/app/models/onboarding_model.dartlib/app/models/size_config_model.dartlib/app/models/storage.dartlib/app/models/storage/set_config.dartlib/app/modules/about/views/about_page_body.dartlib/app/modules/about/views/about_view.dartlib/app/modules/detailRoute/views/dateTimePicker.dartlib/app/modules/detailRoute/views/priority_widget.dartlib/app/modules/detailRoute/views/status_widget.dartlib/app/modules/home/views/home_page_nav_drawer_menu_item.dartlib/app/modules/home/views/home_view.dartlib/app/modules/home/views/project_column_home_page.dartlib/app/modules/home/views/tag_filters_wrap.dartlib/app/modules/home/views/tas_list_item.dartlib/app/modules/home/views/tasks_builder.dartlib/app/modules/manageTaskServer/views/manage_task_server_page_app_bar.dartlib/app/modules/manageTaskServer/views/manage_task_server_page_body.dartlib/app/modules/manageTaskServer/views/manage_task_server_view.dartlib/app/modules/manageTaskServer/views/pem_widget.dartlib/app/modules/onboarding/views/onboarding_page_bottom_section.dartlib/app/modules/onboarding/views/onboarding_view.dartlib/app/modules/profile/controllers/profile_controller.dartlib/app/modules/reports/views/burn_down_monthly.dartlib/app/modules/reports/views/burn_down_weekly.dartlib/app/modules/reports/views/common_chart_indicator.dartlib/app/modules/settings/views/settings_page_highlist_task_list_tile_trailing.dartlib/app/modules/settings/views/settings_page_list_tile.dartlib/app/modules/settings/views/settings_page_on_task_create_list_tile_trailing.dartlib/app/modules/settings/views/settings_page_on_task_start_list_tile_trailing.dartlib/app/services/tag_filter.dartlib/app/utils/constants/constants.dartlib/app/utils/constants/onboarding_screen_content.dartlib/app/utils/constants/permissions.dartlib/app/utils/constants/utilites.dartlib/app/utils/home_path/home_path.dartlib/app/utils/home_path/impl/data.dartlib/app/utils/home_path/impl/gui_pem_file_paths.dartlib/app/utils/home_path/impl/taskchampion_config_file_paths.dartlib/app/utils/home_path/impl/taskd_client.dartlib/app/utils/language/german_sentences.dartlib/app/utils/permissions/permissions_manager.dartlib/app/utils/taskfunctions/comparator.dartlib/app/utils/taskfunctions/query.dartlib/app/utils/taskfunctions/tags.dartlib/app/utils/taskfunctions/urgency.dartlib/app/utils/themes/light_theme.dartlib/app/utils/themes/theme_extension.dartlib/app/utils/themes/themes.dartlib/app/v3/db/task_database.darttest/api_service_test.darttest/models/chart_test.darttest/sentence_manager/sentence_manager_test.darttest/taskfunctions/comparator_test.darttest/taskfunctions/draft_test.darttest/taskfunctions/projects_test.darttest/taskfunctions/validate_test.dart
💤 Files with no reviewable changes (17)
- lib/app/models/json/task.dart
- lib/app/models/storage/set_config.dart
- lib/app/modules/home/views/tag_filters_wrap.dart
- lib/app/models/json/serializers.dart
- lib/app/utils/home_path/impl/gui_pem_file_paths.dart
- lib/app/models/storage.dart
- lib/app/modules/settings/views/settings_page_highlist_task_list_tile_trailing.dart
- lib/app/models/json/annotation.dart
- lib/app/modules/home/views/home_view.dart
- lib/app/utils/taskfunctions/tags.dart
- lib/app/utils/taskfunctions/comparator.dart
- lib/app/utils/home_path/impl/data.dart
- lib/app/modules/profile/controllers/profile_controller.dart
- lib/app/utils/taskfunctions/urgency.dart
- lib/app/utils/home_path/impl/taskd_client.dart
- lib/app/modules/settings/views/settings_page_on_task_create_list_tile_trailing.dart
- lib/app/services/tag_filter.dart
✅ Files skipped from review due to trivial changes (42)
- lib/app/models/data.dart
- test/taskfunctions/draft_test.dart
- lib/app/modules/manageTaskServer/views/manage_task_server_page_app_bar.dart
- lib/app/models/onboarding_model.dart
- lib/app/modules/about/views/about_view.dart
- lib/app/utils/taskfunctions/query.dart
- lib/app/utils/constants/permissions.dart
- lib/app/models/models.dart
- lib/app/utils/permissions/permissions_manager.dart
- lib/app/modules/home/views/home_page_nav_drawer_menu_item.dart
- lib/app/utils/constants/utilites.dart
- test/models/chart_test.dart
- lib/app/utils/themes/themes.dart
- lib/app/modules/manageTaskServer/views/manage_task_server_view.dart
- lib/app/models/size_config_model.dart
- lib/app/utils/constants/onboarding_screen_content.dart
- lib/app/modules/detailRoute/views/priority_widget.dart
- test/taskfunctions/projects_test.dart
- test/taskfunctions/validate_test.dart
- lib/app/models/filters.dart
- lib/app/modules/detailRoute/views/dateTimePicker.dart
- lib/app/modules/settings/views/settings_page_list_tile.dart
- lib/app/utils/home_path/impl/taskchampion_config_file_paths.dart
- lib/app/modules/manageTaskServer/views/pem_widget.dart
- lib/app/modules/manageTaskServer/views/manage_task_server_page_body.dart
- test/api_service_test.dart
- lib/app/modules/home/views/tasks_builder.dart
- lib/app/modules/about/views/about_page_body.dart
- test/taskfunctions/comparator_test.dart
- lib/app/modules/home/views/tas_list_item.dart
- lib/app/utils/home_path/home_path.dart
- lib/app/modules/reports/views/burn_down_weekly.dart
- lib/app/modules/onboarding/views/onboarding_view.dart
- lib/app/modules/reports/views/burn_down_monthly.dart
- lib/app/modules/onboarding/views/onboarding_page_bottom_section.dart
- lib/app/utils/constants/constants.dart
- test/sentence_manager/sentence_manager_test.dart
- lib/app/modules/detailRoute/views/status_widget.dart
- lib/app/utils/themes/light_theme.dart
- lib/app/modules/settings/views/settings_page_on_task_start_list_tile_trailing.dart
- lib/app/modules/reports/views/common_chart_indicator.dart
- lib/app/utils/language/german_sentences.dart
| return TaskwarriorColorTheme( | ||
| dialogBackgroundColor: dialogBackgroundColor ?? dialogBackgroundColor, | ||
| primaryBackgroundColor: | ||
| primaryBackgroundColor ?? primaryBackgroundColor, | ||
| primaryDisabledTextColor: | ||
| primaryDisabledTextColor ?? primaryDisabledTextColor, | ||
| primaryTextColor: primaryTextColor ?? primaryTextColor, | ||
| secondaryBackgroundColor: | ||
| secondaryBackgroundColor ?? secondaryBackgroundColor, | ||
| secondaryTextColor: secondaryTextColor ?? secondaryTextColor, | ||
| dividerColor: dividerColor ?? dividerColor, | ||
| purpleShade: purpleShade ?? purpleShade, | ||
| greyShade: greyShade ?? greyShade, | ||
| icons: icons ?? icons, | ||
| dimCol: dimCol ?? dimCol); |
There was a problem hiding this comment.
copyWith fallback logic is broken—always evaluates to the parameter, never the current field.
All null-coalescing expressions like dialogBackgroundColor ?? dialogBackgroundColor reference the same parameter name on both sides, so they never fall back to the existing instance values. This makes copyWith unable to preserve current values when only some fields are provided.
The correct pattern is parameterName ?? this.fieldName.
Additionally, purpleShade is missing from the method parameters (lines 31-40), so it cannot be overridden.
🐛 Proposed fix
`@override`
ThemeExtension<TaskwarriorColorTheme> copyWith({
Color? dialogBackgroundColor,
Color? primaryBackgroundColor,
Color? secondaryBackgroundColor,
Color? primaryTextColor,
Color? secondaryTextColor,
Color? primaryDisabledTextColor,
Color? dividerColor,
+ Color? purpleShade,
Color? greyShade,
Color? dimCol,
IconData? icons,
}) {
return TaskwarriorColorTheme(
- dialogBackgroundColor: dialogBackgroundColor ?? dialogBackgroundColor,
- primaryBackgroundColor:
- primaryBackgroundColor ?? primaryBackgroundColor,
- primaryDisabledTextColor:
- primaryDisabledTextColor ?? primaryDisabledTextColor,
- primaryTextColor: primaryTextColor ?? primaryTextColor,
- secondaryBackgroundColor:
- secondaryBackgroundColor ?? secondaryBackgroundColor,
- secondaryTextColor: secondaryTextColor ?? secondaryTextColor,
- dividerColor: dividerColor ?? dividerColor,
- purpleShade: purpleShade ?? purpleShade,
- greyShade: greyShade ?? greyShade,
- icons: icons ?? icons,
- dimCol: dimCol ?? dimCol);
+ dialogBackgroundColor: dialogBackgroundColor ?? this.dialogBackgroundColor,
+ primaryBackgroundColor: primaryBackgroundColor ?? this.primaryBackgroundColor,
+ primaryDisabledTextColor: primaryDisabledTextColor ?? this.primaryDisabledTextColor,
+ primaryTextColor: primaryTextColor ?? this.primaryTextColor,
+ secondaryBackgroundColor: secondaryBackgroundColor ?? this.secondaryBackgroundColor,
+ secondaryTextColor: secondaryTextColor ?? this.secondaryTextColor,
+ dividerColor: dividerColor ?? this.dividerColor,
+ purpleShade: purpleShade ?? this.purpleShade,
+ greyShade: greyShade ?? this.greyShade,
+ icons: icons ?? this.icons,
+ dimCol: dimCol ?? this.dimCol);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/app/utils/themes/theme_extension.dart` around lines 42 - 56, The copyWith
implementation for TaskwarriorColorTheme incorrectly uses parameterName ??
parameterName for all fields so it never falls back to existing instance values;
update each null-coalescing expression to use the incoming parameter or
this.field (e.g., dialogBackgroundColor ?? this.dialogBackgroundColor,
primaryBackgroundColor ?? this.primaryBackgroundColor, etc.), and add
purpleShade to the copyWith method signature so it can be overridden (ensure the
returned TaskwarriorColorTheme uses purpleShade ?? this.purpleShade and any
other fields similarly).
| purpleShade: Color.lerp(purpleShade, other.purpleShade, t), | ||
| greyShade: Color.lerp(greyShade, other.greyShade, t), | ||
| dimCol: Color.lerp(dimCol, other.dimCol, t), | ||
| icons: icons); |
There was a problem hiding this comment.
Icon doesn't switch during theme transition—consider flipping at midpoint.
icons: icons always uses the starting theme's icon throughout the entire animation. Since IconData can't be interpolated, the standard approach is to switch at t >= 0.5 so the icon flips at the midpoint of the transition rather than staying on the initial value until the animation completes.
💡 Suggested fix
- icons: icons);
+ icons: t < 0.5 ? icons : other.icons);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| icons: icons); | |
| icons: t < 0.5 ? icons : other.icons); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/app/utils/themes/theme_extension.dart` at line 82, The current code
always passes the starting IconData via "icons: icons" so the visual doesn't
flip during the theme animation; change the property to pick between the start
and end icon at the animation midpoint (e.g. use the animation's value or t and
set icons: (t.value < 0.5) ? startIcon : endIcon). Update the call site that
currently sets "icons: icons" to instead reference the start/end icon variables
(e.g. beginIcon / endIcon) and switch when t >= 0.5 so the icon swaps at the
midpoint of the transition.
| if (updatedCount == 0) { | ||
| throw Exception("Task not found for uuid: $uuid"); | ||
| } | ||
|
|
||
| // 2. Get task ID safely | ||
| final taskMaps = await txn.query( | ||
| 'Tasks', | ||
| columns: ['id'], | ||
| where: 'uuid = ?', | ||
| whereArgs: [uuid], | ||
| ); | ||
|
|
||
| if (taskMaps.isEmpty) { | ||
| throw Exception("No task found after update for uuid: $uuid"); | ||
| } | ||
|
|
||
| final int? taskId = taskMaps.first['id'] as int?; | ||
|
|
||
| if (taskId == null) { | ||
| throw StateError("Missing task id for task: ${taskMaps.first}"); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all call sites of saveEditedTaskInDB with context
rg -n -C6 --type=dart '\bawait\s+[^;]*saveEditedTaskInDB\s*\('
# Inspect try/catch presence in the known primary caller
rg -n -C6 --type=dart '\bsaveTask\s*\(|\btry\s*\{|\bcatch\s*\(' lib/app/modules/taskc_details/controllers/taskc_details_controller.dartRepository: CCExtractor/taskwarrior-flutter
Length of output: 8263
🏁 Script executed:
# Get complete view of saveTask method around the TaskForC call
sed -n '230,265p' lib/app/modules/taskc_details/controllers/taskc_details_controller.dart
# Verify there are no other calls to saveEditedTaskInDB elsewhere
rg -c 'saveEditedTaskInDB' --type=dartRepository: CCExtractor/taskwarrior-flutter
Length of output: 1394
🏁 Script executed:
# Check what calls saveTask() and if there's error handling at the call site
rg -n -B5 -A10 'await.*saveTask\(\)' --type=dart lib/app/modules/taskc_details/controllers/taskc_details_controller.dart
# Also check the complete method signature and any wrapping
rg -n -B3 'Future<void> saveTask' --type=dart lib/app/modules/taskc_details/controllers/taskc_details_controller.dartRepository: CCExtractor/taskwarrior-flutter
Length of output: 634
🏁 Script executed:
# Find ALL calls to saveTask()
rg -n 'saveTask\(\)' --type=dart
# Get full context of handleWillPop method to see if there's any outer error handling
sed -n '337,351p' lib/app/modules/taskc_details/controllers/taskc_details_controller.dartRepository: CCExtractor/taskwarrior-flutter
Length of output: 658
🏁 Script executed:
# Verify saveEditedTaskInDB is only called from saveTask()
rg -n 'saveEditedTaskInDB' --type=dart
# Verify the actual exception-throwing code in saveEditedTaskInDB
sed -n '270,290p' lib/app/v3/db/task_database.dartRepository: CCExtractor/taskwarrior-flutter
Length of output: 853
Add error handling for new exceptions from saveEditedTaskInDB at call sites.
This method now throws when the task is missing or id is null. The only call site (saveTask() at line 240) awaits it without local handling, so failed edits will bubble as unhandled async errors and break UX state after submit. Add try-catch handling in saveTask() or handleWillPop() to gracefully handle these exceptions and provide user feedback.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/app/v3/db/task_database.dart` around lines 270 - 290, The new
saveEditedTaskInDB method now throws when a task is missing or the id is null;
update its caller(s) to catch those exceptions and surface user-friendly
feedback instead of letting them bubble as unhandled async errors. Wrap the
await call to saveEditedTaskInDB inside saveTask() (and similarly in
handleWillPop() if it calls saveEditedTaskInDB) with a try-catch that catches
Exception/StateError, logs the error, updates any UI state (e.g., stop loading
spinner), and shows an error message to the user (toast/dialog) or falls back to
a safe path; ensure you reference saveEditedTaskInDB, saveTask, and
handleWillPop when locating where to add the try-catch and cleanup.
Description
This PR fixes a data inconsistency issue in the local database when editing tasks.
Previously, task details and associated tags were updated using separate database operations without a shared transaction. This could lead to partial updates if an error or app crash occurred between operations.
Changes made
txn)Result
Fixes #643
Screenshots
Not applicable (database-level fix)
Checklist
Summary by CodeRabbit
New Features
Improvements
Chores