Feat/retire ccsync#650
Open
BrawlerXull wants to merge 4 commits into
Open
Conversation
Removes the deprecated HTTP sync path so all synchronization flows through
the native TaskChampion Rust FFI bridge, matching the single-path target
architecture.
- Delete lib/app/v3/net/{fetch,add_task,complete,delete,modify,origin}.dart
and lib/app/v3/db/update.dart (the HTTP push-sync helper).
- Rewrite saveCredentials() to validate credentials via the native sync_()
FFI call instead of an HTTP GET /tasks probe; drop the taskReplica branch.
- Remove the taskc-HTTP sync call sites (home_controller.refreshTasks and its
callers, show_tasks, taskc_details, home_page_app_bar) while preserving all
local SQLite operations.
- Rebrand the 8 ccsync* localization keys to syncServer* and reword copy from
"CCSync" to "TaskChampion sync server" across all 9 language files; collapse
the mode-branched sync-URL label to the single path.
- Drop the obsolete HTTP fetchTasks test group and its generated mocks; realign
the localization snapshot tests to the reworded strings.
http stays in pubspec (still used by pushNotification_service.dart).
flutter analyze: 0 errors. No test regressions vs main (pre-existing headless
plugin/MethodChannel failures unchanged).
Follow-up hardening after an adversarial review of the CCSync retirement:
- saveCredentials(): validate the entered credentials with sync_() BEFORE
persisting them via setTaskcCreds(), so invalid credentials are never
written to the active profile when validation fails. (The prior order —
which mirrored the proposal's example — persisted first, leaving bad creds
on disk while telling the user the check failed.) Validation is a live sync,
so saving now requires connectivity to the sync server.
- Remove the orphaned taskchampionBackendUrl localization getter from the
abstract Sentences class and all 8 locale implementations; it became dead
when the credentials view's mode-branched URL label collapsed to the single
syncServerBackendUrl path.
- Fix a stray non-Urdu token ("używaj") left in syncServerEasySyncTitle on a
line the rebrand already touched.
flutter analyze: 0 errors. Full suite unchanged at +317 -18 (no new failures).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR removes CCSync HTTP-based task synchronization, switches task refresh and mutation paths to local ChangesCCSync removal and TaskChampion local sync migration
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/utils/taskchampion/taskchampion_test.dart (1)
40-52: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMock the Rust bridge in this unit test.
saveCredentials()callssync_()throughRustLib.instance, but this test only sets upSharedPreferences. Without a mockRustLib/sync_implementation, the sync call throws and the credentials are never persisted, so theprefs.getString(...)assertions fail.🤖 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 `@test/utils/taskchampion/taskchampion_test.dart` around lines 40 - 52, The unit test for saving credentials is missing a mock for the Rust bridge, so `controller.saveCredentials()` ends up calling `RustLib.instance.sync_()` and fails before persistence completes. Update the test setup to stub the `RustLib`/`sync_` call used by `saveCredentials()` in `taskchampion_test.dart`, ensuring the method completes successfully before asserting the `SharedPreferences` values.
🧹 Nitpick comments (2)
test/utils/language/sentences_test.dart (2)
11-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
GermanSentencesandUrduSentencesto the shared contract test list.The PR updates German and Urdu sentence classes with the
syncServer*rename, but neither is included insentenceClasses. While Dart's abstract class enforcement catches missing implementations at compile time, adding them to this runtime contract test ensures they don't throw and return validStringvalues.♻️ Proposed addition
final sentenceClasses = [ EnglishSentences(), HindiSentences(), MarathiSentences(), FrenchSentences(), SpanishSentences(), BengaliSentences(), + GermanSentences(), + UrduSentences(), ];🤖 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 `@test/utils/language/sentences_test.dart` around lines 11 - 18, Add the missing GermanSentences and UrduSentences instances to the sentenceClasses list in the shared contract test so they are exercised alongside the existing sentence implementations. Update the test setup in sentences_test.dart using the sentenceClasses collection to include these two classes, ensuring the contract test covers their syncServer* methods and verifies they return valid String values without throwing.
20-177: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding
expectcalls for the remaining newsyncServer*getters.The test verifies
syncServerCredentials(line 63) but notsyncServerLoginInstruction,syncServerEasySyncTitle,syncServerOpenButton,syncServerIntro,syncServerSelfHosted,syncServerBackendUrl, orsyncServerClientId. Since the test is named "should have all required getters," adding these would align the test with its stated purpose and guard against regressions in future language additions.♻️ Proposed additions
expect(sentences.navDrawerSettings, isA<String>()); expect(sentences.syncServerCredentials, isA<String>()); + expect(sentences.syncServerLoginInstruction, isA<String>()); + expect(sentences.syncServerEasySyncTitle, isA<String>()); + expect(sentences.syncServerOpenButton, isA<String>()); + expect(sentences.syncServerIntro, isA<String>()); + expect(sentences.syncServerSelfHosted, isA<String>()); + expect(sentences.syncServerBackendUrl, isA<String>()); + expect(sentences.syncServerClientId, isA<String>()); expect(sentences.deleteTaskTitle, isA<String>());🤖 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 `@test/utils/language/sentences_test.dart` around lines 20 - 177, The sentence getter coverage in the test is incomplete: it checks `syncServerCredentials` but skips the other new `syncServer*` getters. Update the `should have all required getters` case in `sentences_test.dart` to add `expect` assertions for `syncServerLoginInstruction`, `syncServerEasySyncTitle`, `syncServerOpenButton`, `syncServerIntro`, `syncServerSelfHosted`, `syncServerBackendUrl`, and `syncServerClientId` on each `sentenceClasses` entry so the test matches its intended coverage.
🤖 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/app/utils/language/german_sentences.dart`:
- Around line 5-15: The German localization string in german_sentences.dart has
a typo in syncServerIntro: “erhälst” should be corrected to “erhältst”. Update
the String getter syncServerIntro in GermanSentences so the TaskChampion intro
sentence uses the proper second-person singular form of erhalten.
In `@lib/app/utils/language/sentences.dart`:
- Around line 2-9: The TaskChampion test fixture is still using the old
ccsyncBackendUrl field, but ManageTaskChampionCredsController now reads
backend_url_tc through CredentialsStorage. Update taskchampion_test.dart to seed
and assert the backend URL via the same storage path CredentialsStorage uses, or
mock CredentialsStorage so the test exercises the current code path in
ManageTaskChampionCredsController rather than the legacy field.
---
Outside diff comments:
In `@test/utils/taskchampion/taskchampion_test.dart`:
- Around line 40-52: The unit test for saving credentials is missing a mock for
the Rust bridge, so `controller.saveCredentials()` ends up calling
`RustLib.instance.sync_()` and fails before persistence completes. Update the
test setup to stub the `RustLib`/`sync_` call used by `saveCredentials()` in
`taskchampion_test.dart`, ensuring the method completes successfully before
asserting the `SharedPreferences` values.
---
Nitpick comments:
In `@test/utils/language/sentences_test.dart`:
- Around line 11-18: Add the missing GermanSentences and UrduSentences instances
to the sentenceClasses list in the shared contract test so they are exercised
alongside the existing sentence implementations. Update the test setup in
sentences_test.dart using the sentenceClasses collection to include these two
classes, ensuring the contract test covers their syncServer* methods and
verifies they return valid String values without throwing.
- Around line 20-177: The sentence getter coverage in the test is incomplete: it
checks `syncServerCredentials` but skips the other new `syncServer*` getters.
Update the `should have all required getters` case in `sentences_test.dart` to
add `expect` assertions for `syncServerLoginInstruction`,
`syncServerEasySyncTitle`, `syncServerOpenButton`, `syncServerIntro`,
`syncServerSelfHosted`, `syncServerBackendUrl`, and `syncServerClientId` on each
`sentenceClasses` entry so the test matches its intended coverage.
🪄 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: 0b40e57d-3070-4e16-b684-e7d6b710ad7b
📒 Files selected for processing (33)
lib/app/modules/home/controllers/home_controller.dartlib/app/modules/home/views/home_page_app_bar.dartlib/app/modules/home/views/show_tasks.dartlib/app/modules/manage_task_champion_creds/controllers/manage_task_champion_creds_controller.dartlib/app/modules/manage_task_champion_creds/views/manage_task_champion_creds_view.dartlib/app/modules/taskc_details/controllers/taskc_details_controller.dartlib/app/utils/language/bengali_sentences.dartlib/app/utils/language/english_sentences.dartlib/app/utils/language/french_sentences.dartlib/app/utils/language/german_sentences.dartlib/app/utils/language/hindi_sentences.dartlib/app/utils/language/marathi_sentences.dartlib/app/utils/language/sentences.dartlib/app/utils/language/spanish_sentences.dartlib/app/utils/language/urdu_sentences.dartlib/app/v3/db/update.dartlib/app/v3/models/task.dartlib/app/v3/net/add_task.dartlib/app/v3/net/complete.dartlib/app/v3/net/delete.dartlib/app/v3/net/fetch.dartlib/app/v3/net/modify.dartlib/app/v3/net/origin.darttest/api_service_test.darttest/api_service_test.mocks.darttest/utils/language/bengali_sentences_test.darttest/utils/language/english_sentences_test.darttest/utils/language/french_sentences_test.darttest/utils/language/hindi_sentences_test.darttest/utils/language/marathi_sentences_test.darttest/utils/language/sentences_test.darttest/utils/language/spanish_sentences_test.darttest/utils/taskchampion/taskchampion_test.dart
💤 Files with no reviewable changes (10)
- lib/app/v3/net/fetch.dart
- test/api_service_test.mocks.dart
- lib/app/v3/net/add_task.dart
- lib/app/v3/net/delete.dart
- lib/app/v3/net/modify.dart
- lib/app/v3/net/complete.dart
- lib/app/modules/taskc_details/controllers/taskc_details_controller.dart
- lib/app/modules/home/views/show_tasks.dart
- lib/app/v3/db/update.dart
- lib/app/v3/net/origin.dart
Comment on lines
+5
to
+15
| String get syncServerLoginInstruction => | ||
| 'Melde dich bei TaskChampion an, kopiere deine Anmeldedaten und füge sie oben ein.'; | ||
| @override | ||
| String get ccsyncEasySyncTitle => 'CCSync nutzen für einfachen Sync'; | ||
| String get syncServerEasySyncTitle => 'TaskChampion nutzen für einfachen Sync'; | ||
| @override | ||
| String get ccsyncOpenButton => 'CCSync öffnen'; | ||
| String get syncServerOpenButton => 'TaskChampion öffnen'; | ||
| @override | ||
| String get ccsyncIntro => | ||
| 'CCSync nutzt TaskChampion, um Aufgaben nahtlos über mehrere Geräte hinweg zu synchronisieren. Außerdem erhälst du ein Web-Dashboard, über das du deine Aufgaben von jedem Browser aus verwalten kannst.'; | ||
| String get syncServerIntro => | ||
| 'TaskChampion synchronisiert deine Aufgaben nahtlos über mehrere Geräte hinweg. Außerdem erhälst du ein Web-Dashboard, über das du deine Aufgaben von jedem Browser aus verwalten kannst.'; | ||
| @override | ||
| String get ccsyncSelfHosted => | ||
| String get syncServerSelfHosted => |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Typo in German syncServerIntro: "erhälst" should be "erhältst".
The 2nd-person singular of erhalten is du erhältst. The current text is missing a 't', producing the invalid form "erhälst".
✏️ Proposed fix
String get syncServerIntro =>
- 'TaskChampion synchronisiert deine Aufgaben nahtlos über mehrere Geräte hinweg. Außerdem erhälst du ein Web-Dashboard, über das du deine Aufgaben von jedem Browser aus verwalten kannst.';
+ 'TaskChampion synchronisiert deine Aufgaben nahtlos über mehrere Geräte hinweg. Außerdem erhältst du ein Web-Dashboard, über das du deine Aufgaben von jedem Browser aus verwalten kannst.';📝 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.
Suggested change
| String get syncServerLoginInstruction => | |
| 'Melde dich bei TaskChampion an, kopiere deine Anmeldedaten und füge sie oben ein.'; | |
| @override | |
| String get ccsyncEasySyncTitle => 'CCSync nutzen für einfachen Sync'; | |
| String get syncServerEasySyncTitle => 'TaskChampion nutzen für einfachen Sync'; | |
| @override | |
| String get ccsyncOpenButton => 'CCSync öffnen'; | |
| String get syncServerOpenButton => 'TaskChampion öffnen'; | |
| @override | |
| String get ccsyncIntro => | |
| 'CCSync nutzt TaskChampion, um Aufgaben nahtlos über mehrere Geräte hinweg zu synchronisieren. Außerdem erhälst du ein Web-Dashboard, über das du deine Aufgaben von jedem Browser aus verwalten kannst.'; | |
| String get syncServerIntro => | |
| 'TaskChampion synchronisiert deine Aufgaben nahtlos über mehrere Geräte hinweg. Außerdem erhälst du ein Web-Dashboard, über das du deine Aufgaben von jedem Browser aus verwalten kannst.'; | |
| @override | |
| String get ccsyncSelfHosted => | |
| String get syncServerSelfHosted => | |
| String get syncServerLoginInstruction => | |
| 'Melde dich bei TaskChampion an, kopiere deine Anmeldedaten und füge sie oben ein.'; | |
| `@override` | |
| String get syncServerEasySyncTitle => 'TaskChampion nutzen für einfachen Sync'; | |
| `@override` | |
| String get syncServerOpenButton => 'TaskChampion öffnen'; | |
| `@override` | |
| String get syncServerIntro => | |
| 'TaskChampion synchronisiert deine Aufgaben nahtlos über mehrere Geräte hinweg. Außerdem erhältst du ein Web-Dashboard, über das du deine Aufgaben von jedem Browser aus verwalten kannst.'; | |
| `@override` | |
| String get syncServerSelfHosted => |
🤖 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/app/utils/language/german_sentences.dart` around lines 5 - 15, The German
localization string in german_sentences.dart has a typo in syncServerIntro:
“erhälst” should be corrected to “erhältst”. Update the String getter
syncServerIntro in GermanSentences so the TaskChampion intro sentence uses the
proper second-person singular form of erhalten.
Comment on lines
+2
to
+9
| /// TaskChampion UI additional sentences | ||
| String get syncServerLoginInstruction; | ||
| String get syncServerEasySyncTitle; | ||
| String get syncServerOpenButton; | ||
|
|
||
| /// TaskChampion intro and self-hosted sentences | ||
| String get syncServerIntro; | ||
| String get syncServerSelfHosted; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for any remaining references to the old getter names across the codebase.
rg -n 'ccsyncLoginInstruction|ccsyncEasySyncTitle|ccsyncOpenButton|ccsyncIntro|ccsyncSelfHosted|ccsyncCredentials|ccsyncBackendUrl|ccsyncClientId|taskchampionBackendUrl' --type=dartRepository: CCExtractor/taskwarrior-flutter
Length of output: 169
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search the entire repository for any remaining old getter names or backend URL references.
rg -n --hidden --glob '!.git' \
'ccsyncLoginInstruction|ccsyncEasySyncTitle|ccsyncOpenButton|ccsyncIntro|ccsyncSelfHosted|ccsyncCredentials|ccsyncBackendUrl|ccsyncClientId|taskchampionBackendUrl' .Repository: CCExtractor/taskwarrior-flutter
Length of output: 398
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the test file and the production code that reads these preference keys.
git ls-files 'test/utils/taskchampion/taskchampion_test.dart' 'lib/**/taskchampion*' 'lib/**/sentences.dart' | sed 's#^`#FILE`: #'
printf '\n--- taskchampion_test.dart ---\n'
cat -n test/utils/taskchampion/taskchampion_test.dart
printf '\n--- references to ccsyncBackendUrl / taskchampionBackendUrl in lib and test ---\n'
rg -n --hidden --glob '!.git' 'ccsyncBackendUrl|taskchampionBackendUrl' lib testRepository: CCExtractor/taskwarrior-flutter
Length of output: 3146
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- credentials_storage.dart ---\n'
cat -n lib/app/utils/taskchampion/credentials_storage.dart
printf '\n--- taskchampion_config_file_paths.dart ---\n'
cat -n lib/app/utils/home_path/impl/taskchampion_config_file_paths.dart
printf '\n--- all ccsync references ---\n'
rg -n --hidden --glob '!.git' 'ccsync[A-Za-z0-9_]*' lib testRepository: CCExtractor/taskwarrior-flutter
Length of output: 3275
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- manage_task_champion_creds_controller.dart ---\n'
cat -n lib/app/modules/manage_task_champion_creds/controllers/manage_task_champion_creds_controller.dart
printf '\n--- shared_preferences key usage in taskchampion area ---\n'
rg -n --hidden --glob '!.git' "getString\\(|setString\\(|ccsyncBackendUrl|taskc_client_secret|taskc_client_id|backend_url_tc" lib testRepository: CCExtractor/taskwarrior-flutter
Length of output: 6953
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the profile persistence method that writes the TaskChampion credentials.
sed -n '130,190p' lib/app/utils/taskfunctions/profiles.dart | cat -nRepository: CCExtractor/taskwarrior-flutter
Length of output: 2656
Update the TaskChampion test fixture to match CredentialsStorage
test/utils/taskchampion/taskchampion_test.dart still seeds/asserts ccsyncBackendUrl, but ManageTaskChampionCredsController now reads backend_url_tc from the profile files via CredentialsStorage. Update the test to use the current storage path or mock the storage layer so it exercises the live code path.
🤖 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/app/utils/language/sentences.dart` around lines 2 - 9, The TaskChampion
test fixture is still using the old ccsyncBackendUrl field, but
ManageTaskChampionCredsController now reads backend_url_tc through
CredentialsStorage. Update taskchampion_test.dart to seed and assert the backend
URL via the same storage path CredentialsStorage uses, or mock
CredentialsStorage so the test exercises the current code path in
ManageTaskChampionCredsController rather than the legacy field.
Follow-up to the CCSync retirement: the profile page still surfaced the "CCSync" brand -- the "changed profile mode to CCSync" snackbar label and a commented-out "CCSync (v3)" mode radio. Rebrand the label to "Taskchampion" (matching the retained TW3C mode) and delete the dead commented block.
pubspec used a caret constraint, so `flutter pub get` could resolve the newer 2.12.0 runtime while the committed FFI bindings were generated with codegen 2.11.1. That mismatch crashed the app at RustLib.init() on startup. Pinning the exact 2.11.1 (matching rust/Cargo.toml's =2.11.1) keeps the Dart package, generated bindings, and native library in lockstep.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Please include a summary of the change and which issue is fixed. List any dependencies that are required for this change.
Fixes #(issue_no)
Replace
issue_nowith the issue number which is fixed in this PRScreenshots
Checklist
Summary by CodeRabbit