Refactor: use our own uuids as PK - #116
Conversation
WalkthroughMigrate storage to UUID primary keys: add Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/storage/tasks.rs (1)
267-305: Missing UUID resolution instore_single_task.
store_single_taskconvertsTasktoLocalTaskwithout resolving foreign key UUIDs (project_uuid, section_uuid, parent_uuid), unlikestore_taskswhich performs explicit lookups. This will insert remote IDs into UUID columns, causing foreign key violations or data integrity issues.Apply this diff to add UUID resolution:
pub async fn store_single_task(&self, task: Task) -> Result<()> { let label_names = task.labels.clone(); - let local_task: LocalTask = task.into(); + let mut local_task: LocalTask = task.clone().into(); // Use transaction for atomic operation let mut tx = self.pool.begin().await?; + + // Look up local project UUID from remote project_id + if let Some(local_project_uuid) = self.find_uuid_by_remote_id(&mut tx, "projects", &task.project_id).await? { + local_task.project_uuid = local_project_uuid; + } + + // Look up local section UUID from remote section_id if present + if let Some(remote_section_id) = &task.section_id { + if let Some(local_section_uuid) = self.find_uuid_by_remote_id(&mut tx, "sections", remote_section_id).await? { + local_task.section_uuid = Some(local_section_uuid); + } + } + + // Look up local parent UUID from remote parent_id if present + if let Some(remote_parent_id) = &task.parent_id { + if let Some(local_parent_uuid) = self.find_uuid_by_remote_id(&mut tx, "tasks", remote_parent_id).await? { + local_task.parent_uuid = Some(local_parent_uuid); + } + } sqlx::query(src/storage/labels.rs (1)
99-108: Useremote_idfor local storage updates
The sync service passes the Todoist label’s remote ID intoupdate_label_name, but the SQL query filters on the localuuidcolumn. Either change the query insrc/storage/labels.rs:101toUPDATE labels SET name = ? WHERE remote_id = ?or resolve the remote ID to the localuuidbefore calling.src/storage/projects.rs (1)
96-118: Correct parent UUID resolution and use proper upsert by remote_id
- Don’t bind
project.parent_iddirectly—lookup the local parent UUID viafind_uuid_by_remote_id(as instore_projects) before insertion soparent_uuidholds a local UUID, not the remote ID.- Don’t rely on
INSERT OR REPLACEwith a freshly generateduuid(it never conflicts). Useor lookup the existingINSERT INTO projects (…) VALUES (…) ON CONFLICT(remote_id) DO UPDATE SET name=excluded.name, color=excluded.color, …, parent_uuid=excluded.parent_uuid;uuidbyremote_idand reuse it to ensure existing rows are actually updated.
🧹 Nitpick comments (4)
src/storage/db.rs (1)
201-211: Table name interpolation is safe — optional enum refactor
Verified thatfind_uuid_by_remote_idis only invoked with string literals ("projects","sections","tasks"), so there’s no SQL injection risk today. For stricter future guarantees, you may refactortable: &strinto aTableNameenum.src/storage/tasks.rs (1)
198-255: Two-pass insertion approach is sound but complex.The two-pass approach correctly handles parent-child relationships by first inserting all tasks, then updating
parent_uuidreferences. This avoids foreign key constraint violations when parent tasks appear after child tasks in the input vector.However, the complexity could be reduced if the tasks were topologically sorted by parent relationships before insertion.
Consider sorting tasks before insertion to enable single-pass insertion:
// Helper function to add fn topological_sort_tasks(tasks: &[Task]) -> Vec<&Task> { let mut sorted = Vec::new(); let mut visited = std::collections::HashSet::new(); fn visit<'a>( task: &'a Task, tasks: &'a [Task], visited: &mut std::collections::HashSet<String>, sorted: &mut Vec<&'a Task>, ) { if visited.contains(&task.id) { return; } visited.insert(task.id.clone()); // Visit parent first if exists if let Some(parent_id) = &task.parent_id { if let Some(parent) = tasks.iter().find(|t| &t.id == parent_id) { visit(parent, tasks, visited, sorted); } } sorted.push(task); } for task in tasks { visit(task, tasks, &mut visited, &mut sorted); } sorted }Then use single-pass insertion with resolved parent_uuid.
src/storage/projects.rs (2)
10-11: Consider usinguuid::Uuidtype instead ofString.The fields
uuid,remote_id, andparent_uuidare all typed asString. While this works with SQLite and simplifies serialization, using theuuid::Uuidtype foruuidandparent_uuidwould provide:
- Compile-time validation that these fields contain valid UUIDs
- Prevention of accidental assignment of non-UUID strings
- More explicit domain modeling
You can still serialize to/from strings with serde derives.
Example refactor:
pub struct LocalProject { - pub uuid: String, + pub uuid: uuid::Uuid, pub remote_id: String, pub name: String, pub color: String, pub is_favorite: bool, pub is_inbox_project: bool, pub order_index: i32, - pub parent_uuid: Option<String>, + pub parent_uuid: Option<uuid::Uuid>, }Note: This would require updating conversions to use
.to_string()where needed and corresponding changes to SQL binding/extraction logic.Also applies to: 17-17
77-90: Add debug logs for missing parent mappings and batch resolution
- The second pass silently skips missing
parent_id → parent_uuidmappings for projects (and similarly for tasks/sections) without any log; add a debug-level warning whenfind_uuid_by_remote_idreturnsNoneto surface orphaned references.- Each record issues its own UPDATE (O(n) queries); for large imports, preload all remote→local UUID mappings into a map and apply updates in bulk or with a single multi-row query to reduce database round trips.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
Cargo.toml(1 hunks)src/storage/db.rs(7 hunks)src/storage/labels.rs(6 hunks)src/storage/projects.rs(7 hunks)src/storage/sections.rs(5 hunks)src/storage/tasks.rs(19 hunks)src/sync.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
src/storage/db.rs (4)
src/storage/projects.rs (1)
sqlx(151-151)src/storage/sections.rs (2)
sqlx(79-79)sqlx(93-93)src/storage/tasks.rs (3)
sqlx(60-60)sqlx(111-111)sqlx(159-159)src/storage/labels.rs (1)
sqlx(90-90)
src/storage/labels.rs (1)
src/storage/tasks.rs (3)
sqlx(60-60)sqlx(111-111)sqlx(159-159)
src/storage/tasks.rs (5)
src/storage/projects.rs (1)
sqlx(151-151)src/storage/sections.rs (2)
sqlx(79-79)sqlx(93-93)src/storage/labels.rs (1)
sqlx(90-90)src/storage/db.rs (1)
new(16-48)src/sync.rs (2)
new(94-105)get_tasks_for_project(143-146)
src/storage/sections.rs (2)
src/storage/projects.rs (3)
from(21-30)from(34-45)sqlx(151-151)src/storage/tasks.rs (4)
from(28-54)sqlx(60-60)sqlx(111-111)sqlx(159-159)
src/storage/projects.rs (3)
src/storage/sections.rs (4)
from(17-24)from(28-36)sqlx(79-79)sqlx(93-93)src/storage/tasks.rs (4)
from(28-54)sqlx(60-60)sqlx(111-111)sqlx(159-159)src/storage/labels.rs (2)
from(23-32)sqlx(90-90)
🔇 Additional comments (20)
Cargo.toml (1)
43-43: LGTM! UUID dependency properly configured.The uuid crate version 1.11 with v4 (random UUID generation) and serde features aligns well with the PR's goal of introducing UUID-based primary keys.
src/storage/db.rs (6)
71-79: LGTM! Projects table schema correctly migrated to UUIDs.The schema properly introduces
uuidas the primary key, addsremote_idwith a UNIQUE constraint, and updates the parent reference toparent_uuidwith the correct foreign key constraint.
88-93: LGTM! Sections table schema correctly migrated.The uuid/remote_id pattern is consistent, and the foreign key to
projects(uuid)is properly configured.
102-121: LGTM! Tasks table schema correctly handles UUID-based relationships.The migration properly introduces uuid/remote_id columns and updates all foreign key relationships (
parent_uuid,project_uuid,section_uuid) with appropriate CASCADE and SET NULL behaviors.
130-136: LGTM! Labels table schema correctly migrated.The uuid/remote_id pattern is consistently applied.
145-150: LGTM! Junction table correctly migrated to UUID-based keys.The composite primary key and foreign key references are properly updated to use
task_uuidandlabel_uuidcolumns.
156-174: LGTM! Indexes correctly updated to target UUID columns.All index definitions properly reference the new
*_uuidcolumns, ensuring efficient foreign key lookups.src/sync.rs (1)
189-190: LGTM! Label display ID correctly updated to use UUID.The conversion from
LocalLabeltoLabelDisplaynow correctly useslocal.uuidas the identifier, aligning with the UUID-based primary key migration.src/storage/sections.rs (3)
47-71: LGTM: UUID resolution properly implemented instore_sections.The UUID resolution logic correctly looks up the local project UUID from the remote project_id before insertion, maintaining referential integrity with the new UUID-based schema.
16-24: SectionDisplay.id mapping verified
Conversion fromLocalSectionexposinglocal.uuidand from APISectionexposingsection.idis intentional and consistent with all existing usages.
92-104: Verify callers pass local project UUID: The SQL query filters onproject_uuid(the local UUID). Audit all consumers ofSyncService::get_sections_for_project(e.g., UI/CLI modules) to ensure they supply the local project UUID—not the remote Todoist project ID—and consider updating the method’s docstring to clarify this.src/storage/labels.rs (4)
22-33: LGTM: UUID generation is correct.The
From<Label>implementation correctly generates a new local UUID and stores the original label ID inremote_id. Since labels have no foreign key dependencies, no additional resolution is needed.
35-56: LGTM: UUID-based insertion is correct.The
store_single_labelfunction correctly bindsuuidandremote_idin the INSERT statement, maintaining the new schema.
58-86: LGTM: Batch insertion correctly uses UUIDs.The
store_labelsfunction correctly converts each label and inserts with proper UUID/remote_id binding.
88-97: LGTM: Query correctly selects UUID columns.The
get_all_labelsquery correctly selects the newuuidandremote_idcolumns.src/storage/tasks.rs (2)
308-311: Ensureproject_idis a local UUID at each call site
- In
src/sync.rs(around line 145):storage.get_tasks_for_project(project_id).await- In
src/ui/core/task_manager.rs(around line 210):sync_service.get_tasks_for_project(&project.id)…
Confirm that the values passed (project_idandproject.id) are local database UUIDs, not remote IDs.
59-81: All call sites use local task.uuid — No changes needed.src/storage/projects.rs (3)
20-31: LGTM!The conversion from
LocalProjecttoProjectDisplaycorrectly maps the UUID-based local fields to the display model.
120-137: LGTM!The deletion logic correctly:
- Uses transactions for atomicity
- Deletes related tasks first to respect foreign key constraints
- Uses UUID-based identifiers consistently
139-161: LGTM!Both methods correctly use UUID-based identifiers:
update_project_nameupdates byuuidget_projectsselects all necessary fields and properly converts toProjectDisplay
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 (6)
src/storage/projects.rs (2)
96-118: INSERT OR REPLACE generates new UUID, breaking foreign key relationships.The
store_single_projectmethod usesINSERT OR REPLACE, but the conversion (line 98) always generates a new UUID viaFrom<Project>(line 36). This means:
- Existing projects are replaced with a new UUID
- Foreign key references from tasks (
project_uuid) become invalid- Parent-child relationships within projects break
Consider preserving the existing UUID by looking it up before insertion:
pub async fn store_single_project(&self, project: Project) -> Result<()> { let mut local_project: LocalProject = project.into(); // Preserve existing UUID if project already exists if let Some(existing_uuid) = self.find_uuid_by_remote_id( &mut *self.pool.begin().await?, "projects", &local_project.remote_id ).await? { local_project.uuid = existing_uuid; } sqlx::query(/* ... */) // ... }Alternatively, use
INSERT ... ON CONFLICT(remote_id) DO UPDATEwith a unique constraint onremote_id.
97-118: Resolve parent_uuid in store_single_project
Thestore_single_projectconversion usesFrom<Project>which setsparent_uuid = None, so any existingproject.parent_idis dropped. You need to resolve the parent UUID before inserting, mirroring the second pass instore_projects:pub async fn store_single_project(&self, project: Project) -> Result<()> { - let local_project: LocalProject = project.into(); + let mut local_project: LocalProject = project.clone().into(); + + // resolve parent_uuid if this project has a parent + if let Some(remote_parent_id) = &project.parent_id { + if let Some(local_parent_uuid) = + self.find_uuid_by_remote_id(&mut *self.pool.begin().await?, "projects", remote_parent_id).await? + { + local_project.parent_uuid = Some(local_parent_uuid); + } + }Then bind
local_project.parent_uuidas before. This preserves parent relationships when storing a single project.src/storage/tasks.rs (3)
266-324: INSERT OR REPLACE generates new UUID, breaking foreign key relationships.Similar to
store_single_projectinprojects.rs, this method usesINSERT OR REPLACEbut always generates a new UUID viaFrom<Task>(line 37). This breaks:
- Subtask relationships (tasks with
parent_uuidreferencing this task)- Task-label relationships in the
task_labelsjunction tableThe
clear_task_labelscall (line 321) mitigates the junction table issue, but subtask relationships will break.Consider preserving the existing UUID by looking it up before insertion:
pub async fn store_single_task(&self, task: Task) -> Result<()> { let label_names = task.labels.clone(); let mut local_task: LocalTask = task.clone().into(); let mut tx = self.pool.begin().await?; // Preserve existing UUID if task already exists if let Some(existing_uuid) = self.find_uuid_by_remote_id( &mut tx, "tasks", &local_task.remote_id ).await? { local_task.uuid = existing_uuid; } // ... rest of resolution logic ... }
399-457: Standardize Task ID usage between display and storage methods
The CRUD methods in src/storage/tasks.rs (lines 399–457) all filter onWHERE uuid = ?, butTaskDisplay.idis currently populated fromtask.remote_id(src/storage/tasks.rs:121–123), so passingTaskDisplay.idwill never match.
Choose one:
- Update these queries to
WHERE remote_id = ?- Or change
TaskDisplay.idto usetask.uuidinstead oftask.remote_id
188-264: Two issues: destructive DELETE and incomplete FK resolution.Issue 1: Destructive DELETE risks data loss on sync failure.
The
DELETE FROM tasksstatement (line 193) clears all existing tasks before inserting new data. If the subsequent insertion fails, local tasks are permanently lost even though the transaction will roll back the deletion.Issue 2: Incomplete FK resolution in first pass.
Lines 195-241 insert tasks with
parent_uuidset toNULL(line 230), deferring parent resolution to the second pass (lines 243-255). However,project_uuidandsection_uuidare resolved immediately (lines 202-215). This is inconsistent—if a task's project or section hasn't been inserted yet, the FK resolution will fail silently, leaving empty/NULL values.Consider using an upsert strategy to preserve existing UUIDs and ensure all FK resolution happens before insertion:
// Clear existing tasks and their label relationships sqlx::query("DELETE FROM task_labels").execute(&mut *tx).await?; - sqlx::query("DELETE FROM tasks").execute(&mut *tx).await?; - // First pass: Insert all tasks without parent_id relationships + // Insert all tasks with FK resolution let mut task_labels: Vec<(String, Vec<String>)> = Vec::new(); for task in &tasks { let label_names = task.labels.clone(); let mut local_task: LocalTask = task.clone().into(); // Look up local project UUID from remote project_id if let Some(local_project_uuid) = self.find_uuid_by_remote_id(&mut tx, "projects", &task.project_id).await? { local_task.project_uuid = local_project_uuid; } // Look up local section UUID from remote section_id if present if let Some(remote_section_id) = &task.section_id { if let Some(local_section_uuid) = self.find_uuid_by_remote_id(&mut tx, "sections", remote_section_id).await? { local_task.section_uuid = Some(local_section_uuid); } } + + // Look up local parent UUID from remote parent_id if present + if let Some(remote_parent_id) = &task.parent_id { + if let Some(local_parent_uuid) = + self.find_uuid_by_remote_id(&mut tx, "tasks", remote_parent_id).await? + { + local_task.parent_uuid = Some(local_parent_uuid); + } + } task_labels.push((local_task.uuid.clone(), label_names)); sqlx::query( r" - INSERT INTO tasks (uuid, remote_id, content, project_uuid, section_uuid, parent_uuid, priority, order_index, due_date, due_datetime, is_recurring, deadline, duration, description) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) + INSERT INTO tasks (uuid, remote_id, content, project_uuid, section_uuid, parent_uuid, priority, order_index, due_date, due_datetime, is_recurring, deadline, duration, description) + VALUES ( + COALESCE((SELECT uuid FROM tasks WHERE remote_id = ?), ?), + ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ? + ) + ON CONFLICT(uuid) DO UPDATE SET + content = excluded.content, + project_uuid = excluded.project_uuid, + section_uuid = excluded.section_uuid, + parent_uuid = excluded.parent_uuid, + priority = excluded.priority, + order_index = excluded.order_index, + due_date = excluded.due_date, + due_datetime = excluded.due_datetime, + is_recurring = excluded.is_recurring, + deadline = excluded.deadline, + duration = excluded.duration, + description = excluded.description ", ) + .bind(&local_task.remote_id) .bind(&local_task.uuid) .bind(&local_task.remote_id) .bind(&local_task.content) .bind(&local_task.project_uuid) .bind(&local_task.section_uuid) - .bind(None::<String>) // parent_uuid set to NULL for now + .bind(&local_task.parent_uuid) .bind(local_task.priority) .bind(local_task.order_index) .bind(&local_task.due_date) .bind(&local_task.due_datetime) .bind(local_task.is_recurring) .bind(&local_task.deadline) .bind(&local_task.duration) .bind(&local_task.description) .execute(&mut *tx) .await?; } - - // Second pass: Update parent_uuid references to use local UUIDs - for task in &tasks { - if let Some(remote_parent_id) = &task.parent_id { - if let Some(local_parent_uuid) = self.find_uuid_by_remote_id(&mut tx, "tasks", remote_parent_id).await? - { - sqlx::query("UPDATE tasks SET parent_uuid = ? WHERE remote_id = ?") - .bind(&local_parent_uuid) - .bind(&task.id) - .execute(&mut *tx) - .await?; - } - } - }Note: Parent resolution in a single pass requires tasks to be inserted in dependency order (parents before children), or tasks are inserted first without parents and then a second UPDATE pass is performed. The current two-pass approach is safe if all tasks are inserted before the second pass.
src/storage/sections.rs (1)
41-75: Destructive DELETE risks data loss on sync failure.The
DELETE FROM sectionsstatement clears all existing sections before inserting new data. If the subsequent insertion fails (network error, constraint violation, etc.), local sections are permanently lost even though the transaction will roll back the deletion.Consider using an upsert strategy to preserve existing UUIDs:
- // Clear existing sections - sqlx::query("DELETE FROM sections").execute(&mut *tx).await?; - // Insert new sections with mapped project_id for section in §ions { let mut local_section: LocalSection = section.clone().into(); // Look up local project UUID from remote project_id if let Some(local_project_uuid) = self.find_uuid_by_remote_id(&mut tx, "projects", §ion.project_id).await? { local_section.project_uuid = local_project_uuid; } sqlx::query( r" - INSERT INTO sections (uuid, remote_id, name, project_uuid, order_index) - VALUES (?, ?, ?, ?, ?) + INSERT INTO sections (uuid, remote_id, name, project_uuid, order_index) + VALUES ( + COALESCE((SELECT uuid FROM sections WHERE remote_id = ?), ?), + ?, ?, ?, ? + ) + ON CONFLICT(uuid) DO UPDATE SET + name = excluded.name, + project_uuid = excluded.project_uuid, + order_index = excluded.order_index ", ) + .bind(&local_section.remote_id) .bind(&local_section.uuid) .bind(&local_section.remote_id) .bind(&local_section.name) .bind(&local_section.project_uuid) .bind(local_section.order_index) .execute(&mut *tx) .await?; }This preserves existing UUIDs by matching on
remote_idand ensures the old data remains intact if the sync operation fails.
♻️ Duplicate comments (3)
src/storage/projects.rs (2)
54-54: DELETE risks data loss on sync failure.The
DELETE FROM projectsstatement clears all existing projects before inserting new data. If the subsequent insertion fails, local projects are lost even though the transaction will roll back.Consider implementing an upsert strategy (e.g.,
INSERT ... ON CONFLICT) that preserves existing UUIDs by matching onremote_id, or use a temporary table swap pattern.
50-94: Destructive DELETE risks data loss on sync failure.The
DELETE FROM projectsstatement clears all existing projects before inserting new data. If the subsequent insertion fails, local projects are permanently lost even though the transaction will roll back the deletion.Consider using an upsert strategy to preserve existing UUIDs and handle the two-pass parent resolution:
- // Clear existing projects - sqlx::query("DELETE FROM projects").execute(&mut *tx).await?; - // First pass: Insert all projects without parent_id relationships for project in &projects { let local_project: LocalProject = project.clone().into(); sqlx::query( r" - INSERT INTO projects (uuid, remote_id, name, color, is_favorite, is_inbox_project, order_index, parent_uuid) - VALUES (?, ?, ?, ?, ?, ?, ?, NULL) + INSERT INTO projects (uuid, remote_id, name, color, is_favorite, is_inbox_project, order_index, parent_uuid) + VALUES ( + COALESCE((SELECT uuid FROM projects WHERE remote_id = ?), ?), + ?, ?, ?, ?, ?, ?, NULL + ) + ON CONFLICT(uuid) DO UPDATE SET + name = excluded.name, + color = excluded.color, + is_favorite = excluded.is_favorite, + is_inbox_project = excluded.is_inbox_project, + order_index = excluded.order_index ", ) + .bind(&local_project.remote_id) .bind(&local_project.uuid) .bind(&local_project.remote_id) .bind(&local_project.name) .bind(&local_project.color) .bind(local_project.is_favorite) .bind(local_project.is_inbox_project) .bind(local_project.order_index) .execute(&mut *tx) .await?; }This preserves existing UUIDs by matching on
remote_idand ensures old data remains intact if sync fails.src/storage/tasks.rs (1)
192-193: DELETE risks data loss on sync failure.The
DELETE FROM task_labelsandDELETE FROM tasksstatements clear all existing data before inserting new data. If the subsequent insertion fails, local tasks are lost even though the transaction will roll back.Consider implementing an upsert strategy that preserves existing UUIDs by matching on
remote_id.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/storage/projects.rs(7 hunks)src/storage/sections.rs(5 hunks)src/storage/tasks.rs(19 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/storage/projects.rs (3)
src/storage/sections.rs (4)
from(17-24)from(28-36)sqlx(79-79)sqlx(93-93)src/storage/tasks.rs (5)
from(28-54)None(230-230)sqlx(60-60)sqlx(111-111)sqlx(159-159)src/storage/labels.rs (2)
from(23-32)sqlx(90-90)
src/storage/tasks.rs (2)
src/storage/db.rs (1)
new(16-48)src/sync.rs (2)
new(94-105)get_tasks_for_project(143-146)
src/storage/sections.rs (2)
src/storage/projects.rs (3)
from(21-30)from(34-45)sqlx(151-151)src/storage/tasks.rs (4)
from(28-54)sqlx(60-60)sqlx(111-111)sqlx(159-159)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Test (windows-latest, stable)
- GitHub Check: Security Audit
- GitHub Check: Test (macos-latest, beta)
- GitHub Check: Test (macos-latest, stable)
- GitHub Check: Test (windows-latest, beta)
🔇 Additional comments (37)
src/storage/sections.rs (10)
8-14: LGTM! Struct correctly migrated to UUID-based fields.The
LocalSectionstruct now usesuuid,remote_id, andproject_uuidfields, consistent with the UUID-based storage pattern across the codebase.
16-25: LGTM! Display conversion correctly maps UUIDs.The conversion maps
uuid → idandproject_uuid → project_id, exposing local UUIDs to the display layer, consistent with the pattern inprojects.rs.
27-37: LGTM! Conversion correctly defers UUID resolution.The
From<Section>implementation now correctly setsproject_uuidtoString::new()with a comment indicating resolution happens at the storage layer. This addresses the previous review concern and aligns with the pattern inprojects.rsandtasks.rs.
48-71: LGTM! UUID resolution correctly implemented.The storage logic properly resolves the local
project_uuidfrom the remoteproject_idbefore insertion. This two-stage pattern (convert, then resolve) is consistent with the approach inprojects.rsandtasks.rs.
78-104: LGTM! Query methods correctly use UUID-based columns.The
get_sectionsandget_sections_for_projectmethods have been properly updated to select and filter on the new UUID-based columns (uuid,remote_id,project_uuid).
8-14: LGTM! Clean UUID-based structure.The
LocalSectionstruct correctly separates local UUID (uuid) from remote ID (remote_id) and usesproject_uuidfor the foreign key reference.
16-25: LGTM! Correct UUID-to-display mapping.The conversion properly maps local
uuidto the displayidandproject_uuidtoproject_id, maintaining consistency with the storage layer's UUID-based approach.
27-37: LGTM! Correct pattern for deferred FK resolution.Setting
project_uuid: String::new()is correct—the storage layer resolves it (lines 52-56) before insertion, consistent with the pattern intasks.rsandprojects.rs.
78-89: LGTM! Query updated for UUID schema.The SELECT correctly retrieves
uuid, remote_id, name, project_uuid, order_indexfields and orders byproject_uuid.
92-104: LGTM! Correct project filtering by UUID.The query properly filters by
project_uuid = ?and the caller passesproject_id(which is now the local UUID after the migration).src/storage/projects.rs (12)
9-18: LGTM! Struct correctly migrated to UUID-based fields.The
LocalProjectstruct now usesuuid,remote_id, andparent_uuidfields, consistent with the UUID-based storage pattern.
20-31: LGTM! Display conversion correctly maps UUIDs.The conversion correctly maps
uuid → idandparent_uuid → parent_idfor the display layer.
33-46: LGTM! Conversion correctly defers parent UUID resolution.The
From<Project>implementation now correctly setsparent_uuidtoNone, deferring resolution to the storage layer. This addresses the previous review concern.
56-90: LGTM! Two-pass insert/update correctly resolves parent UUIDs.The two-pass approach correctly:
- Inserts all projects with
NULLparent_uuid (lines 56-75)- Updates parent_uuid by resolving remote parent_id to local UUID (lines 77-90)
This ensures parent-child relationships use local UUIDs consistently.
120-161: LGTM! CRUD methods correctly use UUID-based columns.The
delete_project,update_project_name, andget_projectsmethods have been properly updated to reference the new UUID-based columns (uuid,remote_id,parent_uuid).
9-18: LGTM! Clean UUID-based project structure.The
LocalProjectstruct correctly separates local UUID (uuid) from remote ID (remote_id) and usesparent_uuidfor the self-referential foreign key.
20-31: LGTM! Correct UUID-to-display mapping.The conversion properly maps local
uuidto the displayidandparent_uuidtoparent_id.
33-46: LGTM! Correct pattern for deferred parent resolution.Setting
parent_uuid: Noneis correct—the storage layer resolves it (lines 77-90) in a second pass after all projects are inserted, preventing FK constraint violations.
77-90: LGTM! Correct two-pass parent UUID resolution.The second pass correctly resolves
remote_parent_idtolocal_parent_uuidusingfind_uuid_by_remote_id, ensuring parent references use local UUIDs rather than remote IDs.
121-137: LGTM! Correct cascade deletion using UUID.The method correctly deletes tasks by
project_uuidfirst, then deletes the project byuuid. This maintains referential integrity.
140-147: LGTM! Correct update using UUID.The update statement correctly targets projects by
uuid.
150-161: LGTM! Query updated for UUID schema.The SELECT correctly retrieves
uuid, remote_id, name, color, is_favorite, parent_uuid, is_inbox_project, order_indexfields.src/storage/tasks.rs (15)
8-25: LGTM! Struct correctly migrated to UUID-based fields.The
LocalTaskstruct now usesuuid,remote_id,project_uuid,section_uuid, andparent_uuidfields, consistent with the UUID-based storage pattern.
27-55: LGTM! Conversion correctly defers UUID resolution.The
From<Task>implementation correctly setsproject_uuid,section_uuid, andparent_uuidto empty/None values, deferring resolution to the storage layer. This addresses the previous review concern about direct assignment of remote IDs.
59-81: LGTM! Label retrieval correctly uses UUID-based columns.The query correctly joins on
l.uuid = tl.label_uuidand filters bytask_uuid. TheLabelDisplayconstruction mapsuuid → id, consistent with the pattern in other display conversions.
143-185: LGTM! Label relationship methods correctly use UUID-based columns.The
store_task_labelsandclear_task_labelsmethods correctly referenceuuid,task_uuid, andlabel_uuidcolumns in the junction table.
195-255: LGTM! Two-pass insert correctly resolves all UUID relationships.The storage logic correctly:
- Converts tasks and resolves
project_uuidandsection_uuid(lines 198-215)- Inserts with
NULLparent_uuid (lines 219-241)- Updates parent_uuid by resolving remote parent_id to local UUID (lines 243-255)
This ensures all foreign key relationships use local UUIDs consistently.
326-391: LGTM! Query methods correctly use UUID-based columns.All task retrieval methods (
get_tasks_for_project,get_all_tasks,search_tasks, date-based queries) correctly reference theproject_uuidand other UUID-based columns.
120-127: Ignore identifier‐mapping concern. TaskDisplay.id intentionally uses the remote API ID needed for sync operations, while project_id/section_id/parent_id are local-DB UUIDs used by the UI for grouping. This matches storage and UI code and requires no change.Likely an incorrect or invalid review comment.
8-25: LGTM! Clean UUID-based task structure.The
LocalTaskstruct correctly separates local UUID (uuid) from remote ID (remote_id) and uses*_uuidfields for all foreign key references (project, section, parent).
27-55: LGTM! Correct pattern for deferred FK resolution.Setting
project_uuid: String::new(),section_uuid: None, andparent_uuid: Noneis correct—the storage layer resolves these before insertion (lines 202-215, 274-291), consistent with the pattern in other storage modules.
59-81: LGTM! Correct label retrieval using UUIDs.The query correctly joins
labels.uuidwithtask_labels.label_uuidand filters bytask_labels.task_uuid. The mapping toLabelDisplay.iduseslabel.uuid, maintaining consistency with the UUID-based storage.
83-141: LGTM! Correct task retrieval and TaskDisplay mapping.The query correctly selects UUID-based fields (
uuid, remote_id, project_uuid, section_uuid, parent_uuid). Line 122 correctly mapstask.remote_idtoTaskDisplay.id, preserving the API contract where the display ID is the remote identifier, not the local UUID.
143-176: LGTM! Correct label relationship storage using UUIDs.The method correctly queries labels by
uuid, then insertstask_uuidandlabel_uuidinto the junction table. The use ofINSERT OR IGNOREprevents duplicate relationships.
179-185: LGTM! Correct label relationship cleanup.The method correctly deletes by
task_uuid.
267-324: LGTM! Correct single-task storage with full FK resolution.The method correctly resolves all foreign keys (
project_uuid,section_uuid,parent_uuid) before insertion usingfind_uuid_by_remote_id. This ensures no dangling references.
327-457: LGTM! All query methods correctly use UUID fields.All methods correctly reference
uuidfor task identification andproject_uuid/section_uuid/parent_uuidfor filtering and relationships.
this will be needed for the multi-backend coming-soon-ish implementation
884a934 to
b9c39b2
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/storage/labels.rs (1)
41-53: AvoidINSERT OR REPLACE— it can cascade-delete relations and lose UUIDs.REPLACE deletes the existing row (triggering cascades on
task_labels) and inserts a fresh row with a new UUID. Use an upsert that preserves UUIDs, mirroringstore_labels.Apply this diff:
- sqlx::query( - r" - INSERT OR REPLACE INTO labels (uuid, remote_id, name, color, order_index, is_favorite) - VALUES (?, ?, ?, ?, ?, ?) - ", - ) + sqlx::query( + r#" + INSERT INTO labels (uuid, remote_id, name, color, order_index, is_favorite) + VALUES (?, ?, ?, ?, ?, ?) + ON CONFLICT(remote_id) DO UPDATE SET + name = excluded.name, + color = excluded.color, + order_index = excluded.order_index, + is_favorite = excluded.is_favorite + "#, + )src/storage/tasks.rs (1)
415-471: All mutating ops should key by remote_id (to match Display.id).Update, soft-delete, restore, hard-delete, and field updates currently filter by
uuid. Switch toremote_idto acceptTaskDisplay.idconsistently.- sqlx::query("UPDATE tasks SET is_completed = 1 WHERE uuid = ?") + sqlx::query("UPDATE tasks SET is_completed = 1 WHERE remote_id = ?") - sqlx::query("UPDATE tasks SET is_deleted = 1 WHERE uuid = ?") + sqlx::query("UPDATE tasks SET is_deleted = 1 WHERE remote_id = ?") - sqlx::query("UPDATE tasks SET is_deleted = 0, is_completed = 0 WHERE uuid = ?") + sqlx::query("UPDATE tasks SET is_deleted = 0, is_completed = 0 WHERE remote_id = ?") - sqlx::query("DELETE FROM tasks WHERE uuid = ?") + sqlx::query("DELETE FROM tasks WHERE remote_id = ?") - sqlx::query("UPDATE tasks SET due_date = ? WHERE uuid = ?") + sqlx::query("UPDATE tasks SET due_date = ? WHERE remote_id = ?") - sqlx::query("UPDATE tasks SET priority = ? WHERE uuid = ?") + sqlx::query("UPDATE tasks SET priority = ? WHERE remote_id = ?")
♻️ Duplicate comments (2)
src/storage/projects.rs (1)
101-123: Make single insert transactional, upsert without REPLACE, and resolveparent_uuid.
INSERT OR REPLACEcan delete the row and cascade-delete tasks. Also,parent_uuidstays None.Apply this transactional upsert mirroring the batch path:
- pub async fn store_single_project(&self, project: Project) -> Result<()> { - let local_project: LocalProject = project.into(); - - sqlx::query( - r" - INSERT OR REPLACE INTO projects (uuid, remote_id, name, color, is_favorite, is_inbox_project, order_index, parent_uuid) - VALUES (?, ?, ?, ?, ?, ?, ?, ?) - ", - ) - .bind(&local_project.uuid) - .bind(&local_project.remote_id) - .bind(&local_project.name) - .bind(&local_project.color) - .bind(local_project.is_favorite) - .bind(local_project.is_inbox_project) - .bind(local_project.order_index) - .bind(&local_project.parent_uuid) - .execute(&self.pool) - .await?; - - Ok(()) - } + pub async fn store_single_project(&self, project: Project) -> Result<()> { + let mut tx = self.pool.begin().await?; + let remote_parent_id = project.parent_id.clone(); + let local_project: LocalProject = project.into(); + + // Upsert without touching UUID; defer parent linkage to second pass. + sqlx::query( + r#" + INSERT INTO projects (uuid, remote_id, name, color, is_favorite, is_inbox_project, order_index, parent_uuid) + VALUES (?, ?, ?, ?, ?, ?, ?, NULL) + ON CONFLICT(remote_id) DO UPDATE SET + name = excluded.name, + color = excluded.color, + is_favorite = excluded.is_favorite, + is_inbox_project = excluded.is_inbox_project, + order_index = excluded.order_index + "#, + ) + .bind(&local_project.uuid) + .bind(&local_project.remote_id) + .bind(&local_project.name) + .bind(&local_project.color) + .bind(local_project.is_favorite) + .bind(local_project.is_inbox_project) + .bind(local_project.order_index) + .execute(&mut *tx) + .await?; + + // Second pass: resolve and set parent_uuid if present. + if let Some(remote_parent_id) = remote_parent_id { + if let Some(local_parent_uuid) = + self.find_uuid_by_remote_id(&mut tx, "projects", &remote_parent_id).await? + { + sqlx::query("UPDATE projects SET parent_uuid = ? WHERE remote_id = ?") + .bind(&local_parent_uuid) + .bind(&local_project.remote_id) + .execute(&mut *tx) + .await?; + } + } + + tx.commit().await?; + Ok(()) + }src/storage/tasks.rs (1)
409-411: get_task_by_id must match Display.id (use remote_id).Bind against
remote_idso callers can passTaskDisplay.iddirectly.- let tasks = self.get_tasks_with_labels_joined("WHERE uuid = ?", "", &[task_id]).await?; + let tasks = self.get_tasks_with_labels_joined("WHERE remote_id = ?", "", &[task_id]).await?;
🧹 Nitpick comments (7)
src/storage/db.rs (2)
50-63: Optional: expose a shutdown path for the keepalive task.Return/store the JoinHandle and add a stop signal to avoid task leaks in tests/shutdown scenarios. Pool options already keep conns alive; this task is best-effort.
69-79: Future-proof for multi-backend: make(backend, remote_id)unique instead ofremote_idalone.The PR targets multi-backend “soon-ish”;
remote_idmay collide across providers. Add abackend TEXT NOT NULLcolumn and use a composite UNIQUE index per table, and update lookups to include backend.Example migration (SQLite):
ALTER TABLE projects ADD COLUMN backend TEXT NOT NULL DEFAULT 'todoist'; CREATE UNIQUE INDEX IF NOT EXISTS ux_projects_backend_remote ON projects(backend, remote_id); ALTER TABLE sections ADD COLUMN backend TEXT NOT NULL DEFAULT 'todoist'; CREATE UNIQUE INDEX IF NOT EXISTS ux_sections_backend_remote ON sections(backend, remote_id); ALTER TABLE tasks ADD COLUMN backend TEXT NOT NULL DEFAULT 'todoist'; CREATE UNIQUE INDEX IF NOT EXISTS ux_tasks_backend_remote ON tasks(backend, remote_id); ALTER TABLE labels ADD COLUMN backend TEXT NOT NULL DEFAULT 'todoist'; CREATE UNIQUE INDEX IF NOT EXISTS ux_labels_backend_remote ON labels(backend, remote_id);Then, extend
find_uuid_by_remote_idto bind bothbackendandremote_id.Also applies to: 86-97, 100-123, 126-139
src/storage/projects.rs (2)
129-136: Rely on FK cascade to delete tasks; drop the manual delete.
tasks.project_uuidhasON DELETE CASCADE; the extra DELETE is redundant work.Apply:
- // Delete tasks first, then the project - sqlx::query("DELETE FROM tasks WHERE project_uuid = ?") - .bind(project_id) - .execute(&mut *tx) - .await?; - sqlx::query("DELETE FROM projects WHERE uuid = ?") .bind(project_id) .execute(&mut *tx) .await?;
145-152: Rename parameters to*_uuidfor clarity
Change theproject_id: &strparameters inupdate_project_nameanddelete_projecttoproject_uuid: &strto match theuuidcolumn; no call-site changes are needed since Rust uses positional arguments.src/storage/labels.rs (1)
101-109: Renamelabel_id→label_uuidinupdate_label_namefor accuracy. If you also rename the locallabel_idvariable insrc/sync.rs, update its usage and the log message accordingly.src/storage/sections.rs (1)
94-99: Nit: parameter naming for clarity.
project_idactually expects a local UUID. Consider renaming toproject_uuidto avoid ambiguity (and update call sites).src/storage/tasks.rs (1)
83-141: Reduce N+1 queries for labels.
get_tasks_with_labels_joinedfetches labels per task, which doesn’t scale. Consider a single LEFT JOIN (tasks ↔ task_labels ↔ labels), then group labels per task in memory. This will cut DB round-trips and speed up large listings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
Cargo.toml(1 hunks)src/storage/db.rs(7 hunks)src/storage/labels.rs(6 hunks)src/storage/projects.rs(7 hunks)src/storage/sections.rs(5 hunks)src/storage/tasks.rs(18 hunks)src/sync.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/sync.rs
- Cargo.toml
🧰 Additional context used
🧬 Code graph analysis (5)
src/storage/db.rs (4)
src/storage/labels.rs (1)
sqlx(92-92)src/storage/projects.rs (1)
sqlx(156-156)src/storage/sections.rs (2)
sqlx(81-81)sqlx(95-95)src/storage/tasks.rs (3)
sqlx(60-60)sqlx(111-111)sqlx(159-159)
src/storage/sections.rs (3)
src/storage/labels.rs (1)
from(23-32)src/storage/projects.rs (2)
from(21-30)from(34-45)src/storage/tasks.rs (1)
from(28-54)
src/storage/projects.rs (3)
src/storage/labels.rs (2)
from(23-32)sqlx(92-92)src/storage/sections.rs (4)
from(17-24)from(28-36)sqlx(81-81)sqlx(95-95)src/storage/tasks.rs (4)
from(28-54)sqlx(60-60)sqlx(111-111)sqlx(159-159)
src/storage/tasks.rs (2)
src/storage/db.rs (1)
new(16-48)src/sync.rs (2)
new(94-105)get_tasks_for_project(143-146)
src/storage/labels.rs (1)
src/storage/tasks.rs (3)
sqlx(60-60)sqlx(111-111)sqlx(159-159)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Test (windows-latest, beta)
- GitHub Check: Test (macos-latest, stable)
- GitHub Check: Test (macos-latest, beta)
- GitHub Check: Test (windows-latest, stable)
- GitHub Check: Security Audit
🔇 Additional comments (5)
src/storage/projects.rs (1)
82-95: Nit: small consistency tweak in second-pass parent resolution.Use
&mut *tx(as elsewhere) for executor; current style already matches, just keep it consistent across files after fixingdb.rs.src/storage/sections.rs (2)
27-36: Fromleaves project_uuid resolution to storage — good. Generation of a fresh local UUID and deferring parent resolution to the storage layer matches the intended pattern and avoids mixing remote IDs into local UUID fields. Ensure callers always resolve project_uuid before insert to avoid FK issues at runtime.
19-23: Clarify what SectionDisplay.id should represent (local UUID vs remote ID).Here
id = local.uuid. Tasks exposeTaskDisplay.id = remote_id, which may confuse consumers. Please confirm the contract and align across Display types (or expose bothidandlocal_uuidconsistently).src/storage/tasks.rs (2)
37-53: From local mapping looks correct.UUID generation and deferring of project/section/parent resolution to storage are consistent and avoid mixing remote IDs into local UUID fields.
59-79: Confirm LabelDisplay.id contract (local vs remote).Here labels are exposed with
id = label.uuid(local). If external consumers expect remote IDs like tasks, consider exposing both or switching to remote for consistency.
| /// Generic helper to get local UUID from remote ID for any table | ||
| pub(crate) async fn find_uuid_by_remote_id( | ||
| &self, | ||
| tx: &mut sqlx::Transaction<'_, sqlx::Sqlite>, | ||
| table: &str, | ||
| remote_id: &str, | ||
| ) -> Result<Option<String>> { | ||
| let query = format!("SELECT uuid FROM {} WHERE remote_id = ?", table); | ||
| let uuid = sqlx::query_scalar(&query).bind(remote_id).fetch_optional(&mut **tx).await?; | ||
| Ok(uuid) | ||
| } |
There was a problem hiding this comment.
Block SQL injection: whitelist table names and avoid format! in queries.
format!("... FROM {} ...", table) is a SQL injection vector if table is ever non-constant. Also, prefer &mut *tx for the executor.
Apply this diff to harden the helper:
- pub(crate) async fn find_uuid_by_remote_id(
+ pub(crate) async fn find_uuid_by_remote_id(
&self,
tx: &mut sqlx::Transaction<'_, sqlx::Sqlite>,
- table: &str,
+ table: &str,
remote_id: &str,
) -> Result<Option<String>> {
- let query = format!("SELECT uuid FROM {} WHERE remote_id = ?", table);
- let uuid = sqlx::query_scalar(&query).bind(remote_id).fetch_optional(&mut **tx).await?;
+ let sql = match table {
+ "projects" => "SELECT uuid FROM projects WHERE remote_id = ?",
+ "sections" => "SELECT uuid FROM sections WHERE remote_id = ?",
+ "tasks" => "SELECT uuid FROM tasks WHERE remote_id = ?",
+ "labels" => "SELECT uuid FROM labels WHERE remote_id = ?",
+ _ => return Err(anyhow::anyhow!(format!("disallowed table name: {}", table))),
+ };
+ let uuid = sqlx::query_scalar(sql)
+ .bind(remote_id)
+ .fetch_optional(&mut *tx)
+ .await?;
Ok(uuid)
}📝 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.
| /// Generic helper to get local UUID from remote ID for any table | |
| pub(crate) async fn find_uuid_by_remote_id( | |
| &self, | |
| tx: &mut sqlx::Transaction<'_, sqlx::Sqlite>, | |
| table: &str, | |
| remote_id: &str, | |
| ) -> Result<Option<String>> { | |
| let query = format!("SELECT uuid FROM {} WHERE remote_id = ?", table); | |
| let uuid = sqlx::query_scalar(&query).bind(remote_id).fetch_optional(&mut **tx).await?; | |
| Ok(uuid) | |
| } | |
| /// Generic helper to get local UUID from remote ID for any table | |
| pub(crate) async fn find_uuid_by_remote_id( | |
| &self, | |
| tx: &mut sqlx::Transaction<'_, sqlx::Sqlite>, | |
| table: &str, | |
| remote_id: &str, | |
| ) -> Result<Option<String>> { | |
| let sql = match table { | |
| "projects" => "SELECT uuid FROM projects WHERE remote_id = ?", | |
| "sections" => "SELECT uuid FROM sections WHERE remote_id = ?", | |
| "tasks" => "SELECT uuid FROM tasks WHERE remote_id = ?", | |
| "labels" => "SELECT uuid FROM labels WHERE remote_id = ?", | |
| _ => return Err(anyhow::anyhow!(format!("disallowed table name: {}", table))), | |
| }; | |
| let uuid = sqlx::query_scalar(sql) | |
| .bind(remote_id) | |
| .fetch_optional(&mut *tx) | |
| .await?; | |
| Ok(uuid) | |
| } |
| // Look up local project UUID from remote project_id | ||
| if let Some(local_project_uuid) = | ||
| self.find_uuid_by_remote_id(&mut tx, "projects", §ion.project_id).await? | ||
| { | ||
| local_section.project_uuid = local_project_uuid; | ||
| } | ||
|
|
There was a problem hiding this comment.
Guard against unresolved project mapping (avoid inserting with empty FK).
If find_uuid_by_remote_id returns None, project_uuid stays empty. With FK constraints and NOT NULL, this will fail; without them, it creates broken references. Prefer skipping such sections until the project exists.
- if let Some(local_project_uuid) =
- self.find_uuid_by_remote_id(&mut tx, "projects", §ion.project_id).await?
- {
- local_section.project_uuid = local_project_uuid;
- }
+ if let Some(local_project_uuid) =
+ self.find_uuid_by_remote_id(&mut tx, "projects", §ion.project_id).await?
+ {
+ local_section.project_uuid = local_project_uuid;
+ } else {
+ // Parent project not present locally yet; skip for now
+ continue;
+ }📝 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.
| // Look up local project UUID from remote project_id | |
| if let Some(local_project_uuid) = | |
| self.find_uuid_by_remote_id(&mut tx, "projects", §ion.project_id).await? | |
| { | |
| local_section.project_uuid = local_project_uuid; | |
| } | |
| // Look up local project UUID from remote project_id | |
| if let Some(local_project_uuid) = | |
| self.find_uuid_by_remote_id(&mut tx, "projects", §ion.project_id).await? | |
| { | |
| local_section.project_uuid = local_project_uuid; | |
| } else { | |
| // Parent project not present locally yet; skip for now | |
| continue; | |
| } |
🤖 Prompt for AI Agents
In src/storage/sections.rs around lines 49 to 55, the code sets
local_section.project_uuid only when find_uuid_by_remote_id returns Some,
leaving project_uuid empty otherwise which can violate FK/NOT NULL or create
broken references; change the logic to detect the None case and skip
processing/inserting this section (e.g., log a warning/info with section.remote
id and continue to next item), do not attempt to insert or update a section
without a resolved project UUID, and ensure the function returns success for
skipped items while preserving transaction integrity.
| INSERT INTO sections (uuid, remote_id, name, project_uuid, order_index) | ||
| VALUES (?, ?, ?, ?, ?) | ||
| ON CONFLICT(remote_id) DO UPDATE SET | ||
| name = excluded.name, | ||
| project_uuid = excluded.project_uuid, | ||
| order_index = excluded.order_index | ||
| ", |
There was a problem hiding this comment.
Upsert preserves UUIDs — add pruning of stale sections after successful upserts.
Current logic doesn’t delete sections removed upstream, leaving stale rows. After the loop (before commit), delete rows whose remote_id isn’t in the incoming set.
for section in §ions {
/* upsert as implemented */
}
+
+ // Prune sections not present in the incoming snapshot
+ let remote_ids: Vec<&str> = sections.iter().map(|s| s.id.as_str()).collect();
+ if !remote_ids.is_empty() {
+ let placeholders = std::iter::repeat("?").take(remote_ids.len()).collect::<Vec<_>>().join(",");
+ let sql = format!("DELETE FROM sections WHERE remote_id NOT IN ({})", placeholders);
+ let mut qb = sqlx::query(&sql);
+ for id in &remote_ids {
+ qb = qb.bind(id);
+ }
+ qb.execute(&mut *tx).await?;
+ }📝 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.
| INSERT INTO sections (uuid, remote_id, name, project_uuid, order_index) | |
| VALUES (?, ?, ?, ?, ?) | |
| ON CONFLICT(remote_id) DO UPDATE SET | |
| name = excluded.name, | |
| project_uuid = excluded.project_uuid, | |
| order_index = excluded.order_index | |
| ", | |
| for section in §ions { | |
| /* upsert as implemented */ | |
| } | |
| // Prune sections not present in the incoming snapshot | |
| let remote_ids: Vec<&str> = sections.iter().map(|s| s.id.as_str()).collect(); | |
| if !remote_ids.is_empty() { | |
| let placeholders = std::iter::repeat("?") | |
| .take(remote_ids.len()) | |
| .collect::<Vec<_>>() | |
| .join(","); | |
| let sql = format!( | |
| "DELETE FROM sections WHERE remote_id NOT IN ({})", | |
| placeholders | |
| ); | |
| let mut qb = sqlx::query(&sql); | |
| for id in &remote_ids { | |
| qb = qb.bind(id); | |
| } | |
| qb.execute(&mut *tx).await?; | |
| } |
🤖 Prompt for AI Agents
In src/storage/sections.rs around lines 58 to 64, the upsert SQL correctly
updates/inserts sections but never removes sections deleted upstream; after
performing all upserts and before committing the transaction, collect the set of
incoming remote_id values and execute a parameterized DELETE that removes rows
for the same project_uuid whose remote_id is NOT in that set (if the incoming
set is empty, delete all sections for that project_uuid). Ensure this deletion
runs inside the same transaction and uses prepared/parameterized arguments to
avoid SQL injection, then commit.
| let labels = self.get_labels_for_task(&task.uuid).await?; | ||
| task_displays.push(TaskDisplay { | ||
| id: task.id, | ||
| id: task.remote_id, | ||
| content: task.content, | ||
| project_id: task.project_id, | ||
| section_id: task.section_id, | ||
| parent_id: task.parent_id, | ||
| project_id: task.project_uuid, | ||
| section_id: task.section_uuid, | ||
| parent_id: task.parent_uuid, | ||
| priority: task.priority, |
There was a problem hiding this comment.
ID semantics mismatch: Display uses remote_id; write paths below expect uuid.
TaskDisplay.id = task.remote_id, but methods like get_task_by_id, mark_task_completed/deleted, etc., query by uuid. This is a foot‑gun for callers passing TaskDisplay.id. Pick one identifier across reads/writes (suggest: remote_id externally), and update write queries accordingly (see diffs below).
🤖 Prompt for AI Agents
In src/storage/tasks.rs around lines 120–127 the TaskDisplay.id is set to
task.remote_id while downstream write operations (get_task_by_id,
mark_task_completed/deleted, etc.) expect the internal task.uuid, causing an
identifier mismatch; choose a single external identifier (suggest using
remote_id externally) and update the write paths to either accept remote_id and
resolve it to the internal uuid before executing DB queries or change their
queries to directly filter on remote_id; implement a small helper (e.g.,
get_uuid_by_remote_id) to translate remote_id→uuid and call it at the start of
each write path so all subsequent database updates continue to use uuid, or
alternatively set TaskDisplay.id = task.uuid and update any callsites that
expect remote_id—pick one approach and apply it consistently across read and
write code paths.
| // Clear and recreate label relationships | ||
| sqlx::query("DELETE FROM task_labels").execute(&self.pool).await?; | ||
| for (task_uuid, label_names) in task_labels { | ||
| self.store_task_labels(&task_uuid, &label_names).await?; | ||
| } |
There was a problem hiding this comment.
Avoid global wipe of task-label relations; delete only for touched tasks.
DELETE FROM task_labels drops relationships for tasks not in this sync batch. Restrict deletion to the updated task UUIDs.
- // Clear and recreate label relationships
- sqlx::query("DELETE FROM task_labels").execute(&self.pool).await?;
- for (task_uuid, label_names) in task_labels {
- self.store_task_labels(&task_uuid, &label_names).await?;
- }
+ // Clear and recreate label relationships only for tasks we upserted
+ let touched: Vec<String> = task_labels.iter().map(|(u, _)| u.clone()).collect();
+ if !touched.is_empty() {
+ let ph = std::iter::repeat("?").take(touched.len()).collect::<Vec<_>>().join(",");
+ let sql = format!("DELETE FROM task_labels WHERE task_uuid IN ({})", ph);
+ let mut qb = sqlx::query(&sql);
+ for id in &touched {
+ qb = qb.bind(id);
+ }
+ qb.execute(&self.pool).await?;
+ }
+ for (task_uuid, label_names) in task_labels {
+ self.store_task_labels(&task_uuid, &label_names).await?;
+ }📝 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.
| // Clear and recreate label relationships | |
| sqlx::query("DELETE FROM task_labels").execute(&self.pool).await?; | |
| for (task_uuid, label_names) in task_labels { | |
| self.store_task_labels(&task_uuid, &label_names).await?; | |
| } | |
| // Clear and recreate label relationships only for tasks we upserted | |
| let touched: Vec<String> = task_labels.iter().map(|(u, _)| u.clone()).collect(); | |
| if !touched.is_empty() { | |
| let ph = std::iter::repeat("?") | |
| .take(touched.len()) | |
| .collect::<Vec<_>>() | |
| .join(","); | |
| let sql = format!("DELETE FROM task_labels WHERE task_uuid IN ({})", ph); | |
| let mut qb = sqlx::query(&sql); | |
| for id in &touched { | |
| qb = qb.bind(id); | |
| } | |
| qb.execute(&self.pool).await?; | |
| } | |
| for (task_uuid, label_names) in task_labels { | |
| self.store_task_labels(&task_uuid, &label_names).await?; | |
| } |
🤖 Prompt for AI Agents
In src/storage/tasks.rs around lines 271 to 275, the code currently issues a
global "DELETE FROM task_labels" which removes label relations for all tasks;
instead collect the task UUIDs present in the task_labels input and delete only
rows matching those UUIDs (e.g., use a parameterized DELETE ... WHERE task_uuid
IN (...) or issue a DELETE per UUID) before recreating relations by calling
store_task_labels for each task; ensure the delete uses proper parameter binding
to avoid SQL injection and consider wrapping the delete+insert loop in the same
transaction to maintain consistency.
| INSERT OR REPLACE INTO tasks (uuid, remote_id, content, project_uuid, section_uuid, parent_uuid, priority, order_index, due_date, due_datetime, is_recurring, deadline, duration, description, is_completed, is_deleted) | ||
| VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) | ||
| ", |
There was a problem hiding this comment.
INSERT OR REPLACE churns UUIDs and can break FKs; use upsert and preserve UUID + canonicalize label updates.
OR REPLACE deletes then inserts, assigning a fresh UUID (generated above), which can sever label relations or parent links. Mirror the upsert used in store_tasks, and base label relations on the canonical UUID looked up by remote_id.
- sqlx::query(
- r"
- INSERT OR REPLACE INTO tasks (uuid, remote_id, content, project_uuid, section_uuid, parent_uuid, priority, order_index, due_date, due_datetime, is_recurring, deadline, duration, description, is_completed, is_deleted)
- VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)
- ",
- )
+ sqlx::query(
+ r"
+ INSERT INTO tasks (uuid, remote_id, content, project_uuid, section_uuid, parent_uuid, priority, order_index, due_date, due_datetime, is_recurring, deadline, duration, description, is_completed, is_deleted)
+ VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)
+ ON CONFLICT(remote_id) DO UPDATE SET
+ content = excluded.content,
+ project_uuid = excluded.project_uuid,
+ section_uuid = excluded.section_uuid,
+ parent_uuid = excluded.parent_uuid,
+ priority = excluded.priority,
+ order_index = excluded.order_index,
+ due_date = excluded.due_date,
+ due_datetime = excluded.due_datetime,
+ is_recurring = excluded.is_recurring,
+ deadline = excluded.deadline,
+ duration = excluded.duration,
+ description = excluded.description,
+ is_completed = excluded.is_completed,
+ is_deleted = excluded.is_deleted
+ "
+ )
.bind(&local_task.uuid)
.bind(&local_task.remote_id)
.bind(&local_task.content)
.bind(&local_task.project_uuid)
.bind(&local_task.section_uuid)
.bind(&local_task.parent_uuid)
.bind(local_task.priority)
.bind(local_task.order_index)
.bind(&local_task.due_date)
.bind(&local_task.due_datetime)
.bind(local_task.is_recurring)
.bind(&local_task.deadline)
.bind(&local_task.duration)
.bind(&local_task.description)
.bind(local_task.is_completed)
.bind(local_task.is_deleted)
.execute(&mut *tx)
.await?;
- tx.commit().await?;
-
- // Store label relationships after transaction commits
- self.clear_task_labels(&local_task.uuid).await?;
- self.store_task_labels(&local_task.uuid, &label_names).await?;
+ // Resolve canonical UUID (preserved on upsert) for label relations
+ let task_uuid = self.find_uuid_by_remote_id(&mut tx, "tasks", &task.id).await?;
+
+ tx.commit().await?;
+
+ if let Some(task_uuid) = task_uuid {
+ self.clear_task_labels(&task_uuid).await?;
+ self.store_task_labels(&task_uuid, &label_names).await?;
+ }Also applies to: 333-338
1 participant
this will be needed for the multi-backend coming-soon-ish implementation
Summary by CodeRabbit
Refactor
Chores
Notes