refactor(rust): modularize tc_helper, add typed errors, surface task …#649
Open
BrawlerXull wants to merge 9 commits into
Open
refactor(rust): modularize tc_helper, add typed errors, surface task …#649BrawlerXull wants to merge 9 commits into
BrawlerXull wants to merge 9 commits into
Conversation
…attributes Overhauls the Rust FFI bridge (Deliverable 2): Build config - Pin Cargo.toml to edition 2021 for broader Rust/flutter_rust_bridge compatibility; add the thiserror dependency. - Add rust/build.rs: rerun-on-source-change tracking, target-platform detection (exposed via the TC_HELPER_PLATFORM compile-time env), and an opt-in (FRB_CODEGEN=1) binding-regeneration trigger. Modularization & typed errors - Extract the duplicated storage-open boilerplate into storage.rs, the task serializer into serialize.rs, and a thiserror-based TcHelperError into utils/error.rs. api.rs is now a thin FFI layer of delegators. - Remove every .unwrap() panic; each FFI entry point returns Result<_, String> so flutter_rust_bridge surfaces failures to Dart as catchable exceptions with a real message, instead of relying on panics. Surfaced task attributes - The serializer now emits annotations, dependencies (depends), is_blocked, is_blocking, and recur, which the Dart TaskForReplica model previously anticipated but never received. TaskForReplica gains those fields + parsing (reusing the existing Annotation model). - urgency is intentionally NOT surfaced: TaskChampion 2.0.3 does not compute or store an urgency value on Task, so there is nothing authoritative to expose. Regenerated the flutter_rust_bridge bindings for the new signatures (getAllTasksJson stays Future<String>; the mutating calls are now Future<void>, throwing on error — all existing Dart callers ignored the old int return). Deferred (needs mentor coordination + CI cross-compilation, not doable here): purging the tracked android jniLibs *.so binaries. There is currently no Android cargo/cargo-ndk integration, so Gradle bundles the committed .so directly; removing them before automated cross-compilation exists would break Android builds. cargo build + cargo test: pass. flutter analyze: 0 errors. Full test suite unchanged at +317 -20 (pre-existing headless failures only). Runtime FFI behaviour still needs validation via an on-device build.
📝 WalkthroughWalkthroughThis PR updates the Rust task backend, generated Dart/Rust bridge code, and Flutter task and tour UI to carry replica-backed task fields, return stringified task operation results, and use guarded page-tour display. It also adds a source build workflow and adjusts iOS framework metadata. ChangesRust API and Dart bridge taskflow updates
Flutter task and tour UI updates
Build and platform packaging
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant FlutterApp
participant DartBridge as lib/rust_bridge
participant RustApi as rust/src/api.rs
participant Storage as open_replica
participant Serializer as task_to_json
FlutterApp->>DartBridge: getAllTasksJson(taskdbDirPath)
DartBridge->>RustApi: crateApiGetAllTasksJson(...)
RustApi->>Storage: open_replica(taskdb_dir_path)
Storage-->>RustApi: Replica or TcHelperError
RustApi->>Serializer: task_to_json(task)
Serializer-->>RustApi: JSON Value
RustApi-->>DartBridge: Result<String, String>
DartBridge-->>FlutterApp: JSON payload
FlutterApp->>DartBridge: updateTask(uuidSt, taskdbDirPath, map)
DartBridge->>RustApi: crateApiUpdateTask(...)
RustApi->>Storage: open_replica(taskdb_dir_path)
RustApi->>RustApi: parse UUID, apply tag/status updates, commit
RustApi-->>DartBridge: Result<(), String>
DartBridge-->>FlutterApp: void completion
sequenceDiagram
participant Controller as Page controller
participant SaveTourStatus
participant SafeTour as safeShowTour
participant TutorialCoachMark
Controller->>SaveTourStatus: get...TourStatus()
SaveTourStatus-->>Controller: seen flag
Controller->>SafeTour: safeShowTour(...)
SafeTour->>TutorialCoachMark: show() when mounted and targets exist
TutorialCoachMark-->>SafeTour: shown or throws
SafeTour->>SaveTourStatus: markSeen() on skip/failure
🚥 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/app/modules/taskc_details/controllers/taskc_details_controller.dart (1)
263-324: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
saveTaskdropsdepends,recur, andannotationswhen saving a replica task.The controller initializes these fields from the replica model (lines 85-88) and they are user-editable, but the
modifiedTaskconstructed insaveTask(lines 267-324) does not pass them to theTaskForReplicaconstructor. User edits to these fields are silently lost on save.🐛 Proposed fix
priority: priority.string.isNotEmpty ? priority.string : null, project: project.string != 'None' ? project.string : null, + depends: depends.isNotEmpty ? depends.toList() : null, + recur: recur.string.isNotEmpty ? recur.string : null, + annotations: annotations.isNotEmpty ? annotations.toList() : null, );🤖 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/modules/taskc_details/controllers/taskc_details_controller.dart` around lines 263 - 324, The saveTask path for TaskForReplica is omitting user-editable fields, so edits to depends, recur, and annotations are lost when building modifiedTask. Update the TaskForReplica construction in saveTask to carry through the current values for those properties, using the same controller fields that are initialized earlier in taskc_details_controller.dart, alongside the existing status, description, tags, and date fields.
🧹 Nitpick comments (1)
lib/app/v3/champion/models/task_for_replica.dart (1)
182-183: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
hashCodedoesn't include the new fields checked byoperator ==.While the hashCode/equals contract is technically maintained (equal objects will still produce equal hashes), omitting
isBlocked,isBlocking,depends, andrecurfrom the hash causes excessive collisions in hash-based collections likeSetandMapkeys.♻️ Proposed fix
- int get hashCode => Object.hash(modified, due, status, description, uuid, - priority, tags == null ? 0 : tags.hashCode, start, wait); + int get hashCode => Object.hash( + modified, due, status, description, uuid, priority, + tags == null ? 0 : tags.hashCode, start, wait, + isBlocked, isBlocking, + depends == null ? 0 : depends.hashCode, + recur, + annotations == null ? 0 : annotations.hashCode);🤖 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/v3/champion/models/task_for_replica.dart` around lines 182 - 183, The TaskForReplica hashCode implementation is missing the new fields that operator == compares, which can cause avoidable hash collisions in Sets and Maps. Update TaskForReplica.hashCode to include isBlocked, isBlocking, depends, and recur alongside the existing properties so it stays aligned with operator ==. Keep the change localized to the hashCode getter in TaskForReplica and preserve the current hashing pattern for the other fields.
🤖 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/v3/champion/models/task_for_replica.dart`:
- Around line 174-178: `TaskForReplica.operator ==` is missing the `annotations`
field, so equality can incorrectly treat two tasks as the same. Update the
equality check in `TaskForReplica` to include `annotations` alongside the
existing fields (`priority`, `isBlocked`, `isBlocking`, `depends`, `recur`), and
make sure the corresponding `hashCode` logic stays consistent with that change.
---
Outside diff comments:
In `@lib/app/modules/taskc_details/controllers/taskc_details_controller.dart`:
- Around line 263-324: The saveTask path for TaskForReplica is omitting
user-editable fields, so edits to depends, recur, and annotations are lost when
building modifiedTask. Update the TaskForReplica construction in saveTask to
carry through the current values for those properties, using the same controller
fields that are initialized earlier in taskc_details_controller.dart, alongside
the existing status, description, tags, and date fields.
---
Nitpick comments:
In `@lib/app/v3/champion/models/task_for_replica.dart`:
- Around line 182-183: The TaskForReplica hashCode implementation is missing the
new fields that operator == compares, which can cause avoidable hash collisions
in Sets and Maps. Update TaskForReplica.hashCode to include isBlocked,
isBlocking, depends, and recur alongside the existing properties so it stays
aligned with operator ==. Keep the change localized to the hashCode getter in
TaskForReplica and preserve the current hashing pattern for the other fields.
🪄 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: ccbe89b7-140e-4862-8aa3-579e110db3fd
⛔ Files ignored due to path filters (1)
rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
lib/app/modules/taskc_details/controllers/taskc_details_controller.dartlib/app/v3/champion/models/task_for_replica.dartlib/rust_bridge/api.dartlib/rust_bridge/frb_generated.dartlib/rust_bridge/frb_generated.io.dartlib/rust_bridge/frb_generated.web.dartrust/Cargo.tomlrust/build.rsrust/src/api.rsrust/src/frb_generated.rsrust/src/lib.rsrust/src/serialize.rsrust/src/storage.rsrust/src/utils/error.rsrust/src/utils/mod.rs
💤 Files with no reviewable changes (1)
- lib/rust_bridge/frb_generated.io.dart
Comment on lines
+174
to
+178
| other.priority == priority && | ||
| other.isBlocked == isBlocked && | ||
| other.isBlocking == isBlocking && | ||
| _listEquals(other.depends, depends) && | ||
| other.recur == recur; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
annotations is missing from operator ==.
Two TaskForReplica objects with different annotations will be considered equal, which can cause incorrect behavior in collections and diffing logic.
🐛 Proposed fix
_listEquals(other.depends, depends) &&
- other.recur == recur;
+ other.recur == recur &&
+ _listEquals(other.annotations, annotations);📝 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
| other.priority == priority && | |
| other.isBlocked == isBlocked && | |
| other.isBlocking == isBlocking && | |
| _listEquals(other.depends, depends) && | |
| other.recur == recur; | |
| other.priority == priority && | |
| other.isBlocked == isBlocked && | |
| other.isBlocking == isBlocking && | |
| _listEquals(other.depends, depends) && | |
| other.recur == recur && | |
| _listEquals(other.annotations, annotations); |
🤖 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/v3/champion/models/task_for_replica.dart` around lines 174 - 178,
`TaskForReplica.operator ==` is missing the `annotations` field, so equality can
incorrectly treat two tasks as the same. Update the equality check in
`TaskForReplica` to include `annotations` alongside the existing fields
(`priority`, `isBlocked`, `isBlocking`, `depends`, `recur`), and make sure the
corresponding `hashCode` logic stays consistent with that change.
Member
|
@CodeRabbit full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
lib/app/v3/champion/models/task_for_replica.dart (1)
174-178: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
annotationsis still missing fromoperator ==.This was flagged in a previous review and remains unaddressed. Two
TaskForReplicaobjects with different annotations will compare as equal, which can cause incorrect behavior in collections and diffing logic.🐛 Proposed fix
_listEquals(other.depends, depends) && - other.recur == recur; + other.recur == recur && + _listEquals(other.annotations, annotations);🤖 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/v3/champion/models/task_for_replica.dart` around lines 174 - 178, The TaskForReplica equality check in operator == is still missing annotations, so objects with different annotations can compare equal. Update the equality comparison in TaskForReplica’s operator == to include annotations alongside the other compared fields, and make sure the corresponding hashCode logic in the same class stays consistent with that field.
🤖 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.
Duplicate comments:
In `@lib/app/v3/champion/models/task_for_replica.dart`:
- Around line 174-178: The TaskForReplica equality check in operator == is still
missing annotations, so objects with different annotations can compare equal.
Update the equality comparison in TaskForReplica’s operator == to include
annotations alongside the other compared fields, and make sure the corresponding
hashCode logic in the same class stays consistent with that field.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3c6d86ff-560a-4fdc-8bf9-3957ebe796f2
⛔ Files ignored due to path filters (1)
rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
lib/app/modules/taskc_details/controllers/taskc_details_controller.dartlib/app/v3/champion/models/task_for_replica.dartlib/rust_bridge/api.dartlib/rust_bridge/frb_generated.dartlib/rust_bridge/frb_generated.io.dartlib/rust_bridge/frb_generated.web.dartrust/Cargo.tomlrust/build.rsrust/src/api.rsrust/src/frb_generated.rsrust/src/lib.rsrust/src/serialize.rsrust/src/storage.rsrust/src/utils/error.rsrust/src/utils/mod.rs
💤 Files with no reviewable changes (1)
- lib/rust_bridge/frb_generated.io.dart
…rust_bridge to exact 2.11.1 Adds test_dependencies_and_annotations_surface asserting depends[]/is_blocked/is_blocking and annotation entry(RFC3339)+description. Pins flutter_rust_bridge (was ^2.11.1) to avoid resolving 2.12.0, which crashes at RustLib.init().
The committed .so predated the serializer overhaul (Feb build vs June source), so the app ran stale native code. Rebuilt via cargo-ndk. Stopgap until CI compiles from source (see build-tc-helper.yml).
- home_page_body: reactively read tasksFromReplica in the Obx so the replica list rebuilds when the async FFI fetch completes (was empty: taskReplica flips true before the list is populated). - task detail: surface annotations/depends/is_blocked/is_blocking/recur (read-only) for replica tasks. - safe_tour: guard tutorial_coach_mark.show() against unmounted target keys (fixes recurring 'obtain target position (null)' FormatException); applied to profile + manage-task-server tours.
Regenerates the native lib from rust/ on each run so it can't go stale; builds the APK against it. Unverified on CI. armeabi-v7a left as a documented TODO (needs aws-lc-sys bindgen).
The Taskchampion (replica) home never showed tasks despite the FFI returning them. Four compounding bugs:
- show_tasks_replica wrapped its body in an Obx that read no observable -> ObxError -> blank error widget. Reactivity now lives in the parent (home_page_body) Obx, so this is a plain build.
- home_controller.onInit never initialized pendingFilter/waitingFilter from their persisted values, so pendingFilter defaulted false and the list filtered for completed tasks only, hiding all pending ones.
- the project filter treated an empty-string projectFilter ('') as an active filter, dropping every task whose project is null.
Verified on-device: replica home lists tasks; detail view surfaces annotations/depends/is_blocked/is_blocking/recur.
…gen; rebuild both ABIs 32-bit ARM has no pre-generated aws-lc-sys bindings, so it needs the crate's 'bindgen' feature (libclang). Scoped that feature to ONLY [target.armv7-linux-androideabi] so arm64/iOS/host keep using pre-generated bindings (forcing bindgen globally broke the iOS link). Rebuilt both Android ABIs from source; CI installs libclang + builds both. Host cargo test green.
tc_helper only ever uses the remote TaskChampion sync server (ServerConfig::Remote). taskchampion's default 'sync' feature also pulled server-aws + server-gcp, dragging in the entire AWS + Google-Cloud SDKs and aws-lc-rs/aws-lc-sys. Switched to default-features=false + [server-sync, bundled] (ureq + ring). Wins: - iOS xcframework now builds (aws-lc-sys 0.30.0 failed to cross-compile PQ/kyber for iOS); rebuilt device arm64 + fat simulator from the new Rust. - No more bindgen hack for 32-bit ARM (aws-lc gone); dropped libcrc_fast entirely. - libtc_helper.so 29M->6.2M (arm64), 20M->4.7M (v7a); release APK 60M->36M. - Smaller dependency/attack surface; faster builds; simpler CI (no cmake/ninja/libclang). Verified: host cargo test (2) pass; both Android ABIs + all 3 iOS targets build; real-server sync returns Ok() (ureq+ring); on-device the replica home lists tasks + detail rows render with the new lib, no RustLib version crash.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
.github/workflows/build-tc-helper.yml (2)
46-50: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAdd Rust/cargo caching to avoid reinstalling
cargo-ndkand recompiling all dependencies on every run.
cargo install cargo-ndkand the full dependency tree build from scratch each time. AddingSwatinem/rust-cacheis a one-liner that caches the cargo registry, target directory, and installed binaries.♻️ Proposed addition
- name: Install Rust + Android targets + cargo-ndk run: | rustup toolchain install stable --profile minimal rustup target add aarch64-linux-android armv7-linux-androideabi cargo install cargo-ndk --version ^4 + + - uses: Swatinem/rust-cache@v2 + with: + workspaces: rust🤖 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 @.github/workflows/build-tc-helper.yml around lines 46 - 50, The Rust setup step in the build workflow is reinstalling cargo-ndk and rebuilding dependencies on every run, so add Swatinem/rust-cache in the workflow near the Rust toolchain/cargo-ndk setup. Wire the cache into the same job that runs rustup and cargo install cargo-ndk so it preserves the cargo registry, target directory, and installed binaries between runs, reducing repeated downloads and recompilation.
69-72: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSet
if-no-files-found: erroron artifact upload.Without this, the upload step silently succeeds (with a warning) if the APK is missing, masking build failures.
♻️ Proposed fix
- uses: actions/upload-artifact@v4 with: name: production-apk-from-source path: build/app/outputs/flutter-apk/app-production-release.apk + if-no-files-found: error🤖 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 @.github/workflows/build-tc-helper.yml around lines 69 - 72, The artifact upload step in the build workflow is too permissive because it can succeed with only a warning when the APK is missing. Update the `actions/upload-artifact@v4` step for `production-apk-from-source` to set `if-no-files-found: error` so missing `app-production-release.apk` fails the job instead of masking the build issue.lib/app/modules/taskc_details/views/taskc_details_view.dart (1)
114-152: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNew replica fields bypass the app's localization pattern.
All other detail labels in this view route through
SentenceManager(...).sentences.xxx, but the new Blocked/Blocking/Depends/Recur/Annotations labels (and their 'Yes'/'No'/'None' values) are hardcoded English strings. This breaks localization consistency for non-English users.♻️ Suggested direction
- _buildDetail( - context, - 'Blocked:', - controller.isBlocked.value ? 'Yes' : 'No', - ), + _buildDetail( + context, + '${SentenceManager(currentLanguage: AppSettings.selectedLanguage).sentences.detailPageBlocked}:', + controller.isBlocked.value + ? SentenceManager(currentLanguage: AppSettings.selectedLanguage).sentences.yes + : SentenceManager(currentLanguage: AppSettings.selectedLanguage).sentences.no, + ),Apply the same pattern to Blocking/Depends/Recur/Annotations labels and their string values, adding the corresponding keys to
SentenceManager.🤖 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/modules/taskc_details/views/taskc_details_view.dart` around lines 114 - 152, The replica-task detail labels in taskc_details_view are bypassing the existing localization flow and are hardcoded in English. Update the Blocked/Blocking/Depends/Recur/Annotations section to use SentenceManager(...).sentences like the other fields, and add the needed sentence keys for both labels and values such as Yes/No/None. Keep the implementation aligned with the existing _buildDetail usage in TaskcDetailsView so the new replica fields follow the app’s localization pattern.rust/Cargo.toml (1)
10-23: 📐 Maintainability & Code Quality | 🔵 TrivialConsider pinning
thiserrorto the current major version.This is a new dependency addition, and
thiserror2.x is the current major release. Since there's no existing 1.x usage to migrate away from, starting on 2.x avoids a future breaking-version bump.Please confirm the latest stable
thiserrorrelease and whether 2.x is compatible with the crate's other dependencies (e.g., MSRV viataskchampion'srust-version = "1.81.0").🤖 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 `@rust/Cargo.toml` around lines 10 - 23, The new thiserror dependency is pinned to an older major version, so update the Rust dependency declaration in Cargo.toml to the current stable 2.x line instead of 1.x. Check that the thiserror release you choose is compatible with the crate’s MSRV and with taskchampion’s rust-version setting, then keep the dependency declaration aligned with that compatible 2.x version.lib/app/tour/safe_tour.dart (1)
32-35: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider logging the swallowed exception for debuggability.
The
catch (_)block is intentionally defensive, but silently discarding the exception makes it hard to diagnose why a tour failed to display. A debug-level log would help troubleshooting without changing behavior.♻️ Optional: add debug logging
try { tutorialCoachMark.show(context: context); } catch (_) { // Defensive: never let a tour failure bubble up or repeat. + debugPrint('safeShowTour: tour show() failed, marking as seen'); await markSeen?.call(); }🤖 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/tour/safe_tour.dart` around lines 32 - 35, The `catch (_)` in `safeTour` is swallowing tour failures without any visibility. Update the exception handler in `safe_tour.dart` to capture the caught error (and stack trace if available) and emit a debug-level log before calling `markSeen?.call()`, keeping the defensive behavior unchanged. Use the `safeTour` function as the location anchor and preserve the current fallback flow.
🤖 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 @.github/workflows/build-tc-helper.yml:
- Line 32: The checkout step in the build-tc-helper workflow persists the
GITHUB_TOKEN by default, which is unsafe for pull_request runs that execute
untrusted code. Update the existing actions/checkout@v4 step to disable
credential persistence by setting persist-credentials to false so the workflow
no longer leaves the token in git config.
- Around line 62-67: The build job in the workflow needs an explicit timeout
guard to prevent runaway Rust/Flutter builds. Update the job that contains the
flutter-action step and the flutter build apk command to set a reasonable
`timeout-minutes` value, using the existing build job definition as the place to
add it. Keep the change focused on the workflow job configuration so the build
is automatically terminated if it hangs.
---
Nitpick comments:
In @.github/workflows/build-tc-helper.yml:
- Around line 46-50: The Rust setup step in the build workflow is reinstalling
cargo-ndk and rebuilding dependencies on every run, so add Swatinem/rust-cache
in the workflow near the Rust toolchain/cargo-ndk setup. Wire the cache into the
same job that runs rustup and cargo install cargo-ndk so it preserves the cargo
registry, target directory, and installed binaries between runs, reducing
repeated downloads and recompilation.
- Around line 69-72: The artifact upload step in the build workflow is too
permissive because it can succeed with only a warning when the APK is missing.
Update the `actions/upload-artifact@v4` step for `production-apk-from-source` to
set `if-no-files-found: error` so missing `app-production-release.apk` fails the
job instead of masking the build issue.
In `@lib/app/modules/taskc_details/views/taskc_details_view.dart`:
- Around line 114-152: The replica-task detail labels in taskc_details_view are
bypassing the existing localization flow and are hardcoded in English. Update
the Blocked/Blocking/Depends/Recur/Annotations section to use
SentenceManager(...).sentences like the other fields, and add the needed
sentence keys for both labels and values such as Yes/No/None. Keep the
implementation aligned with the existing _buildDetail usage in TaskcDetailsView
so the new replica fields follow the app’s localization pattern.
In `@lib/app/tour/safe_tour.dart`:
- Around line 32-35: The `catch (_)` in `safeTour` is swallowing tour failures
without any visibility. Update the exception handler in `safe_tour.dart` to
capture the caught error (and stack trace if available) and emit a debug-level
log before calling `markSeen?.call()`, keeping the defensive behavior unchanged.
Use the `safeTour` function as the location anchor and preserve the current
fallback flow.
In `@rust/Cargo.toml`:
- Around line 10-23: The new thiserror dependency is pinned to an older major
version, so update the Rust dependency declaration in Cargo.toml to the current
stable 2.x line instead of 1.x. Check that the thiserror release you choose is
compatible with the crate’s MSRV and with taskchampion’s rust-version setting,
then keep the dependency declaration aligned with that compatible 2.x version.
🪄 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: 2969d739-f44d-42ca-aa86-1f77e6bbde01
⛔ Files ignored due to path filters (5)
android/app/src/main/jniLibs/arm64-v8a/libcrc_fast-b3182f249ae653b7.sois excluded by!**/*.soandroid/app/src/main/jniLibs/arm64-v8a/libtc_helper.sois excluded by!**/*.soandroid/app/src/main/jniLibs/armeabi-v7a/libcrc_fast-c8bd19aec5c73f3a.sois excluded by!**/*.soandroid/app/src/main/jniLibs/armeabi-v7a/libtc_helper.sois excluded by!**/*.sorust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
.github/workflows/build-tc-helper.ymlios/tc_helper.xcframework/Info.plistios/tc_helper.xcframework/ios-arm64/tc_helper.framework/tc_helperios/tc_helper.xcframework/ios-arm64_x86_64-simulator/tc_helper.framework/tc_helperlib/app/modules/home/controllers/home_controller.dartlib/app/modules/home/views/home_page_body.dartlib/app/modules/home/views/show_tasks_replica.dartlib/app/modules/manageTaskServer/controllers/manage_task_server_controller.dartlib/app/modules/profile/controllers/profile_controller.dartlib/app/modules/taskc_details/controllers/taskc_details_controller.dartlib/app/modules/taskc_details/views/taskc_details_view.dartlib/app/tour/safe_tour.dartpubspec.yamlrust/Cargo.tomlrust/src/api.rs
✅ Files skipped from review due to trivial changes (1)
- ios/tc_helper.xcframework/Info.plist
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/app/modules/taskc_details/controllers/taskc_details_controller.dart
- rust/src/api.rs
| build: | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Set persist-credentials: false on checkout.
The actions/checkout@v4 action persists the GITHUB_TOKEN in .git/config by default. Since this workflow runs on pull_request and checks out untrusted PR code, disabling credential persistence eliminates a potential token-exfiltration vector.
🛡️ Proposed fix
- - uses: actions/checkout@v4
+ - uses: actions/checkout@v4
+ with:
+ persist-credentials: false📝 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
| - uses: actions/checkout@v4 | |
| - uses: actions/checkout@v4 | |
| with: | |
| persist-credentials: false |
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 32-32: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 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 @.github/workflows/build-tc-helper.yml at line 32, The checkout step in the
build-tc-helper workflow persists the GITHUB_TOKEN by default, which is unsafe
for pull_request runs that execute untrusted code. Update the existing
actions/checkout@v4 step to disable credential persistence by setting
persist-credentials to false so the workflow no longer leaves the token in git
config.
Source: Linters/SAST tools
Comment on lines
+62
to
+67
| - uses: subosito/flutter-action@v2 | ||
| with: | ||
| flutter-version: "3.44.5" | ||
|
|
||
| - run: flutter pub get | ||
| - run: flutter build apk --release --flavor production |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
What is the latest stable Flutter SDK version as of July 2026?
💡 Result:
As of July 9, 2026, the latest stable version of the Flutter SDK is 3.44.5 [1][2]. This version is part of the Flutter 3.44 release series, which was officially released in May 2026 [3]. You can ensure you are on the latest stable version by running the following commands in your terminal [4][2]: $ flutter channel stable $ flutter upgrade
Citations:
- 1: https://docs.flutter.dev/install/archive
- 2: https://github.com/flutter/flutter/blob/HEAD/CHANGELOG.md
- 3: https://github.com/flutter/website/blob/main/src/content/install/archive.md
- 4: https://github.com/flutter/flutter/blob/stable/CHANGELOG.md
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the workflow around the referenced lines
FILE=".github/workflows/build-tc-helper.yml"
wc -l "$FILE"
sed -n '1,140p' "$FILE" | nl -ba | sed -n '45,90p'Repository: CCExtractor/taskwarrior-flutter
Length of output: 250
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=".github/workflows/build-tc-helper.yml"
# Show the relevant section with line numbers using tools likely available in the sandbox
awk 'NR>=45 && NR<=75 { printf "%4d %s\n", NR, $0 }' "$FILE"Repository: CCExtractor/taskwarrior-flutter
Length of output: 1281
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=".github/workflows/build-tc-helper.yml"
# Show the job header and early lines for timeout settings
awk 'NR>=1 && NR<=45 { printf "%4d %s\n", NR, $0 }' "$FILE"Repository: CCExtractor/taskwarrior-flutter
Length of output: 2111
Add an explicit timeout to the build job. The workflow has no timeout-minutes, so a hung Rust/Flutter build can burn runner minutes for hours; timeout-minutes: 45 is a reasonable guard.
🤖 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 @.github/workflows/build-tc-helper.yml around lines 62 - 67, The build job in
the workflow needs an explicit timeout guard to prevent runaway Rust/Flutter
builds. Update the job that contains the flutter-action step and the flutter
build apk command to set a reasonable `timeout-minutes` value, using the
existing build job definition as the place to add it. Keep the change focused on
the workflow job configuration so the build is automatically terminated if it
hangs.
The search box only fed the local-taskc list (searchedTasks); the replica view (TaskReplicaViewBuilder) read tasksFromReplica directly and ignored the query, so typing in search did nothing in TaskChampion mode. Add a reactive searchQuery (kept in sync by search()/toggleSearch()) and filter the replica snapshot by task.description in home_page_body's Obx, so the list narrows on each keystroke and restores when the query is cleared. Matching is case-insensitive substring, consistent with the taskc path.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/app/modules/home/controllers/home_controller.dart (1)
94-99: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant filter initialization —
_profileSet()already sets these values.
_profileSet()is called at line 88 and already initializespendingFilter.value(line 237) andwaitingFilter.value(lines 238–243) from the sameQuery(storage.tabs.tab())source. Lines 98–99 repeat the same assignments, so the comment "Without this the RxBool defaults to false" is misleading — the values are already set before these lines execute.Consider removing the duplicate assignment or moving the comment to
_profileSet()if the intent was to document that method's initialization.🤖 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/modules/home/controllers/home_controller.dart` around lines 94 - 99, The pending/waiting filter setup in home_controller is duplicated because _profileSet() already assigns both pendingFilter.value and waitingFilter.value from Query(storage.tabs.tab()). Remove the repeated initialization here, or if the note is meant to document first-load behavior, move the comment and any related explanation to _profileSet() where the values are actually initialized. Keep the behavior centered around _profileSet() and avoid maintaining the same Query(storage.tabs.tab()) assignments in two places.
🤖 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.
Nitpick comments:
In `@lib/app/modules/home/controllers/home_controller.dart`:
- Around line 94-99: The pending/waiting filter setup in home_controller is
duplicated because _profileSet() already assigns both pendingFilter.value and
waitingFilter.value from Query(storage.tabs.tab()). Remove the repeated
initialization here, or if the note is meant to document first-load behavior,
move the comment and any related explanation to _profileSet() where the values
are actually initialized. Keep the behavior centered around _profileSet() and
avoid maintaining the same Query(storage.tabs.tab()) assignments in two places.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3c559344-a82f-4c29-8f14-f562153f6964
📒 Files selected for processing (2)
lib/app/modules/home/controllers/home_controller.dartlib/app/modules/home/views/home_page_body.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/app/modules/home/views/home_page_body.dart
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
2 participants
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.
…attributes
Overhauls the Rust FFI bridge (Deliverable 2):
Build config
Modularization & typed errors
Surfaced task attributes
Regenerated the flutter_rust_bridge bindings for the new signatures (getAllTasksJson stays Future; the mutating calls are now Future, throwing on error — all existing Dart callers ignored the old int return).
Deferred (needs mentor coordination + CI cross-compilation, not doable here): purging the tracked android jniLibs *.so binaries. There is currently no Android cargo/cargo-ndk integration, so Gradle bundles the committed .so directly; removing them before automated cross-compilation exists would break Android builds.
cargo build + cargo test: pass. flutter analyze: 0 errors. Full test suite unchanged at +317 -20 (pre-existing headless failures only). Runtime FFI behaviour still needs validation via an on-device build.
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
New Features
Bug Fixes
Chores