diff --git a/Audit.md b/Audit.md index 3a28260..0204e0b 100644 --- a/Audit.md +++ b/Audit.md @@ -1,5 +1,14 @@ # Audit Log +## 2026-04-20 + +Found and fixed 4 issues: + +1. **Dead code in conflict recovery** (sync.rs:756) — `parts[1] != ".listdata.json"` was unreachable because the branch is already gated on `parts[1].ends_with(".md")`, which `.listdata.json` cannot satisfy. Removed the redundant check. +2. **O(n²) cascade delete** (tauri/lib.rs) — descendant traversal in `delete_task` used `Vec::contains` inside the inner loop, making it quadratic in the number of tasks per list. Swapped the visited set to `HashSet`; `HashSet::insert` folds the contains+push into one call. +3. **Silent cascade failure in toggle_task** (tauri/lib.rs) — subtask `update_task` errors were discarded with `let _ = ...`, leaving subtasks stuck at the old status with no UI feedback. Propagate the error so the frontend can surface it. +4. **Duplicated UUID-parse boilerplate** (tauri/lib.rs) — 17 commands repeated `Uuid::parse_str(&x).map_err(|e| e.to_string())?`. Extracted a `parse_uuid` helper so callers read as `let id = parse_uuid(&list_id)?;`. + ## 2026-04-15 Found and fixed 4 issues: diff --git a/apps/tauri/src-tauri/src/lib.rs b/apps/tauri/src-tauri/src/lib.rs index 154f6d0..d71e981 100644 --- a/apps/tauri/src-tauri/src/lib.rs +++ b/apps/tauri/src-tauri/src/lib.rs @@ -60,6 +60,11 @@ fn lock_state(state: &Mutex) -> Result Result { + Uuid::parse_str(s).map_err(|e| e.to_string()) +} + impl AppState { /// Persist config to disk, converting errors to String for Tauri commands. fn save_config(&self) -> Result<(), String> { @@ -366,7 +371,7 @@ fn delete_list( let mut s = lock_state(&state)?; ensure_repo(&mut s)?; mute_watcher(&mut s); - let id = Uuid::parse_str(&list_id).map_err(|e| e.to_string())?; + let id = parse_uuid(&list_id)?; repo_mut(&mut s)? .delete_list(id) .map_err(|e| e.to_string()) @@ -381,7 +386,7 @@ fn list_tasks( ) -> Result, String> { let mut s = lock_state(&state)?; ensure_repo(&mut s)?; - let id = Uuid::parse_str(&list_id).map_err(|e| e.to_string())?; + let id = parse_uuid(&list_id)?; repo_ref(&s)? .list_tasks(id) .map_err(|e| e.to_string()) @@ -400,13 +405,13 @@ fn create_task( let mut s = lock_state(&state)?; ensure_repo(&mut s)?; mute_watcher(&mut s); - let id = Uuid::parse_str(&list_id).map_err(|e| e.to_string())?; + let id = parse_uuid(&list_id)?; let mut task = Task::new(title); if let Some(desc) = description.filter(|d| !d.is_empty()) { task.description = desc; } if let Some(pid) = parent_id { - let parent_uuid = Uuid::parse_str(&pid).map_err(|e| e.to_string())?; + let parent_uuid = parse_uuid(&pid)?; task.parent_id = Some(parent_uuid); } // Accept the date fields at creation time so callers don't have to do a @@ -428,7 +433,7 @@ fn update_task( let mut s = lock_state(&state)?; ensure_repo(&mut s)?; mute_watcher(&mut s); - let id = Uuid::parse_str(&list_id).map_err(|e| e.to_string())?; + let id = parse_uuid(&list_id)?; repo_mut(&mut s)? .update_task(id, task) .map_err(|e| e.to_string()) @@ -443,19 +448,18 @@ fn delete_task( let mut s = lock_state(&state)?; ensure_repo(&mut s)?; mute_watcher(&mut s); - let lid = Uuid::parse_str(&list_id).map_err(|e| e.to_string())?; - let tid = Uuid::parse_str(&task_id).map_err(|e| e.to_string())?; + let lid = parse_uuid(&list_id)?; + let tid = parse_uuid(&task_id)?; let repo = repo_mut(&mut s)?; // Cascade-delete the full descendant subtree (not just direct children) // so deleting a parent can't leave grandchildren orphaned with a // parent_id pointing at a deleted task. let all_tasks = repo.list_tasks(lid).map_err(|e| e.to_string())?; - let mut to_delete: Vec = Vec::new(); + let mut to_delete: std::collections::HashSet = std::collections::HashSet::new(); let mut frontier: Vec = vec![tid]; while let Some(parent) = frontier.pop() { for t in &all_tasks { - if t.parent_id == Some(parent) && !to_delete.contains(&t.id) { - to_delete.push(t.id); + if t.parent_id == Some(parent) && to_delete.insert(t.id) { frontier.push(t.id); } } @@ -478,8 +482,8 @@ fn toggle_task( let mut s = lock_state(&state)?; ensure_repo(&mut s)?; mute_watcher(&mut s); - let lid = Uuid::parse_str(&list_id).map_err(|e| e.to_string())?; - let tid = Uuid::parse_str(&task_id).map_err(|e| e.to_string())?; + let lid = parse_uuid(&list_id)?; + let tid = parse_uuid(&task_id)?; let repo = repo_mut(&mut s)?; let mut task = repo.get_task(lid, tid).map_err(|e| e.to_string())?; match task.status { @@ -496,7 +500,9 @@ fn toggle_task( TaskStatus::Backlog => child.uncomplete(), TaskStatus::Completed => child.complete(), } - let _ = repo.update_task(lid, child); + let child_id = child.id; + repo.update_task(lid, child) + .map_err(|e| format!("Failed to cascade to subtask {}: {}", child_id, e))?; } } Ok(task) @@ -512,8 +518,8 @@ fn reorder_task( let mut s = lock_state(&state)?; ensure_repo(&mut s)?; mute_watcher(&mut s); - let lid = Uuid::parse_str(&list_id).map_err(|e| e.to_string())?; - let tid = Uuid::parse_str(&task_id).map_err(|e| e.to_string())?; + let lid = parse_uuid(&list_id)?; + let tid = parse_uuid(&task_id)?; repo_mut(&mut s)? .reorder_task(lid, tid, new_position) .map_err(|e| e.to_string()) @@ -531,9 +537,9 @@ fn move_task( let mut s = lock_state(&state)?; ensure_repo(&mut s)?; mute_watcher(&mut s); - let from = Uuid::parse_str(&from_list_id).map_err(|e| e.to_string())?; - let to = Uuid::parse_str(&to_list_id).map_err(|e| e.to_string())?; - let tid = Uuid::parse_str(&task_id).map_err(|e| e.to_string())?; + let from = parse_uuid(&from_list_id)?; + let to = parse_uuid(&to_list_id)?; + let tid = parse_uuid(&task_id)?; repo_mut(&mut s)? .move_task(from, to, tid) .map_err(|e| e.to_string()) @@ -548,7 +554,7 @@ fn rename_list( let mut s = lock_state(&state)?; ensure_repo(&mut s)?; mute_watcher(&mut s); - let id = Uuid::parse_str(&list_id).map_err(|e| e.to_string())?; + let id = parse_uuid(&list_id)?; repo_mut(&mut s)? .rename_list(id, new_name) .map_err(|e| e.to_string()) @@ -563,7 +569,7 @@ fn set_group_by_date( let mut s = lock_state(&state)?; ensure_repo(&mut s)?; mute_watcher(&mut s); - let id = Uuid::parse_str(&list_id).map_err(|e| e.to_string())?; + let id = parse_uuid(&list_id)?; repo_mut(&mut s)? .set_group_by_date(id, enabled) .map_err(|e| e.to_string()) @@ -576,7 +582,7 @@ fn get_group_by_date( ) -> Result { let mut s = lock_state(&state)?; ensure_repo(&mut s)?; - let id = Uuid::parse_str(&list_id).map_err(|e| e.to_string())?; + let id = parse_uuid(&list_id)?; repo_ref(&s)? .get_group_by_date(id) .map_err(|e| e.to_string()) diff --git a/crates/onyx-core/src/sync.rs b/crates/onyx-core/src/sync.rs index 17c10ae..6eb2b57 100644 --- a/crates/onyx-core/src/sync.rs +++ b/crates/onyx-core/src/sync.rs @@ -753,7 +753,7 @@ async fn execute_action( // For .md task files inside a list dir, create a duplicate of the local version let parts: Vec<&str> = path.split('/').collect(); - if parts.len() == 2 && parts[1].ends_with(".md") && parts[1] != ".listdata.json" { + if parts.len() == 2 && parts[1].ends_with(".md") { let local_content = String::from_utf8_lossy(&local_data); if let Ok((frontmatter, description)) = parse_frontmatter_for_conflict(&local_content) { let original_id = frontmatter.id;