refactor(rust): modularize tc_helper, add typed errors, surface task …#649
Open
BrawlerXull wants to merge 1 commit into
Open
refactor(rust): modularize tc_helper, add typed errors, surface task …#649BrawlerXull wants to merge 1 commit into
BrawlerXull wants to merge 1 commit 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 adds typed Rust error handling and replica/task serialization helpers, rewrites task API calls to return ChangesRust API and Dart bridge taskflow updates
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant FlutterApp as Flutter app
participant DartBridge as Dart bridge
participant RustApi as Rust 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
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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
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