Compare commits

..

1 commit

10 changed files with 43 additions and 127 deletions

View file

@ -1,4 +1,9 @@
{ {
"sandbox": {
"network": {
"allowedDomains": ["nx71726.your-storageshare.de"]
}
},
"hooks": { "hooks": {
"PreToolUse": [ "PreToolUse": [
{ {

View file

@ -1,29 +1,5 @@
# Audit Log # Audit Log
## 2026-04-27
Found and fixed 3 issues:
1. **Perf: needless clone of upload payload** (sync.rs:733) — the `SyncAction::Upload` arm read the file into `data`, computed `compute_checksum(&data)`, then called `client.put_file(path, data.clone())`. The clone existed only because the next statement needed `data.len()` for the sync-state record. Captured `data.len() as u64` into `len` first, moved `data` into `put_file`, and used `len` afterwards — one full byte copy avoided per uploaded file.
2. **Bug: Google Tasks sync silently drops metadata-write failures** (google_tasks.rs:361, 377) — both `.listdata.json` and `.onyx-workspace.json` were written via `if let Ok(meta_content) = serde_json::to_string_pretty(...) { let _ = atomic_write(...); }`, so a serialization or atomic-write error returned `Ok(GoogleSyncResult { downloaded: N, errors: [] })` even though list/workspace ordering was never persisted. Both writes now push their errors into the `errors` vec already returned in `GoogleSyncResult`.
3. **Code quality: unreachable dead-error path in storage dedup** (storage.rs:447) — the dedup loop computed `Option<Task>` from each `by_id` group and then `ok_or_else(|| Error::InvalidData("Empty dedup entries for task"))?`. `by_id` is only populated by `entry(uuid).or_default().push(entry)`, so every group has ≥1 element and the `None` branch is unreachable. Replaced the `Option`+`?` with direct `expect` calls (one per branch) that document the non-empty invariant; the loop now yields `Task` directly.
## 2026-04-25
Found and fixed 3 issues:
1. **Perf: O(n²) deletion-detection in `get_sync_status`** (sync.rs:918) — for every path tracked in `sync_state.files`, the loop scanned `local_files` linearly via `.any(|f| f.path == *path)` to decide whether to count it as a deleted-locally pending change. The earlier "modified or new" loop already used the inverse direction with `sync_state.files.get(...)` (O(1)), so the second loop was the inconsistent one. Built a `HashSet<&str>` of local paths once and used `contains` for the membership check.
2. **Perf: cascade delete walks all_tasks per frontier pop** (tauri/lib.rs:460) — `delete_task`'s descendant BFS scanned the full task list on every parent popped from the frontier, making the work O(n × depth). Built a `parent_id -> [child_id]` `HashMap` once, then the BFS visits each descendant in O(1) amortised, dropping total cost to O(n).
3. **Code quality: duplicate atomic-write in `AppConfig::save_to_file`** (config.rs:114) — the function had its own copy of the temp-file + rename + cleanup-on-failure dance even though `storage::atomic_write` is `pub(crate)` and was already shared by `google_tasks.rs`. Replaced the inline implementation with a call to `crate::storage::atomic_write` so the crate has one canonical atomic write path.
## 2026-04-24
Found and fixed 3 issues:
1. **Bug: orphan base entries never cleaned from sync state** (sync.rs) — when a file was deleted both locally and remotely, `compute_sync_actions` emitted no action (the `(None, None, Some(_))` arm), so the base entry in `.syncstate.json` persisted forever. On each subsequent sync the same no-op case fired and the state file grew. Added `prune_orphan_bases` pass in `sync_workspace_inner` that drops base entries not present in either scan.
2. **Code quality: redundant is_some_and on already-matched Option** (sync.rs:208) — the `(None, Some(_), Some(b))` arm re-checked `remote` via `remote.is_some_and(|r| ...)` even though the pattern had just proven `remote` is `Some(_)`. Bound the inner value with `Some(r)` in the pattern and used `r` directly.
3. **Code quality: single-caller sanitize_filename wrapper** (storage.rs) — `FileSystemStorage::sanitize_filename` was a one-line forwarder to `crate::sanitize_filename` with one call site. Inlined the crate call and removed the method.
## 2026-04-20 ## 2026-04-20
Found and fixed 4 issues: Found and fixed 4 issues:

View file

@ -64,7 +64,7 @@ The GUI uses Svelte 5 runes mode (`$state`, `$derived`, `$effect`, `$props()`).
Pre-alpha. No users, no released builds, no data to migrate. Breaking changes to on-disk formats, config structure, or sync conventions are free — do not add migration logic. Pre-alpha. No users, no released builds, no data to migrate. Breaking changes to on-disk formats, config structure, or sync conventions are free — do not add migration logic.
### Current state (2026-04-27) ### Current state (2026-04-15)
- **Phase 1** (Core + CLI): Complete - **Phase 1** (Core + CLI): Complete
- **Phase 2** (WebDAV sync): Complete — remote folder browsing, checksum-based conflict resolution, auto-sync lifecycle, per-workspace sync interval - **Phase 2** (WebDAV sync): Complete — remote folder browsing, checksum-based conflict resolution, auto-sync lifecycle, per-workspace sync interval
@ -106,7 +106,7 @@ Pre-alpha. No users, no released builds, no data to migrate. Breaking changes to
- Task deduplication on load (handles sync conflict duplicates) - Task deduplication on load (handles sync conflict duplicates)
- Subtask hierarchy: subtask count shown on parent tasks in list, subtask detail via three-panel slide navigation, inline add at top of subtask list (new subtasks prepend), collapsible completed subtasks section, cascade delete (parent deletion removes all subtasks with confirmation warning) - Subtask hierarchy: subtask count shown on parent tasks in list, subtask detail via three-panel slide navigation, inline add at top of subtask list (new subtasks prepend), collapsible completed subtasks section, cascade delete (parent deletion removes all subtasks with confirmation warning)
- Custom confirmation dialogs (ConfirmDialog component replaces native confirm()) - Custom confirmation dialogs (ConfirmDialog component replaces native confirm())
- Workspace path validation (rejects filesystem root `/` and system directories: `/etc`, `/usr`, `/bin`, `/sbin`, `/var`, `/proc`, `/sys`, `/dev`) - Workspace path validation (rejects system directories)
- Task detail auto-cleanup (taskStack clears when viewed task is deleted or list switches) - Task detail auto-cleanup (taskStack clears when viewed task is deleted or list switches)
- Swipe gestures on mobile: swipe left/right on a task to toggle completion (swipe direction depends on current status) - Swipe gestures on mobile: swipe left/right on a task to toggle completion (swipe direction depends on current status)
- Accessibility: ARIA labels/roles on interactive components, keyboard handlers, `prefers-reduced-motion` CSS support - Accessibility: ARIA labels/roles on interactive components, keyboard handlers, `prefers-reduced-motion` CSS support

View file

@ -765,7 +765,7 @@ WorkspaceConfig {
- [x] List rename (inline input via list kebab menu in drawer) - [x] List rename (inline input via list kebab menu in drawer)
- [x] Keyboard shortcuts (Escape closes settings → detail → drawer → menus in priority order) - [x] Keyboard shortcuts (Escape closes settings → detail → drawer → menus in priority order)
- [x] Sync status indicators (last-sync time + upload/download counts chip in TasksScreen) - [x] Sync status indicators (last-sync time + upload/download counts chip in TasksScreen)
- [ ] Push/pull sync mode selection (session-only sync direction selector in SettingsScreen) - [x] Push/pull sync mode selection (session-only sync direction selector in SettingsScreen)
- [x] Group-by-date toggle per list (checkmark toggle in list kebab menu) - [x] Group-by-date toggle per list (checkmark toggle in list kebab menu)
- [x] Subtask hierarchy (expand/collapse, inline add, cascade toggle/delete) - [x] Subtask hierarchy (expand/collapse, inline add, cascade toggle/delete)
- [ ] Search/filter tasks - [ ] Search/filter tasks
@ -1058,6 +1058,6 @@ This project is free and open-source software licensed under GPL v3.
--- ---
**Last Updated**: 2026-04-27 **Last Updated**: 2026-04-23
**Document Version**: 4.5 **Document Version**: 4.4
**Status**: Ready to Implement - Milestone-Driven Plan **Status**: Ready to Implement - Milestone-Driven Plan

View file

@ -455,23 +455,12 @@ fn delete_task(
// so deleting a parent can't leave grandchildren orphaned with a // so deleting a parent can't leave grandchildren orphaned with a
// parent_id pointing at a deleted task. // parent_id pointing at a deleted task.
let all_tasks = repo.list_tasks(lid).map_err(|e| e.to_string())?; let all_tasks = repo.list_tasks(lid).map_err(|e| e.to_string())?;
// Build a parent -> children index in one pass so the BFS below is O(n)
// instead of O(n * depth) scanning all tasks for each frontier pop.
let mut children_by_parent: std::collections::HashMap<Uuid, Vec<Uuid>> =
std::collections::HashMap::new();
for t in &all_tasks {
if let Some(pid) = t.parent_id {
children_by_parent.entry(pid).or_default().push(t.id);
}
}
let mut to_delete: std::collections::HashSet<Uuid> = std::collections::HashSet::new(); let mut to_delete: std::collections::HashSet<Uuid> = std::collections::HashSet::new();
let mut frontier: Vec<Uuid> = vec![tid]; let mut frontier: Vec<Uuid> = vec![tid];
while let Some(parent) = frontier.pop() { while let Some(parent) = frontier.pop() {
if let Some(children) = children_by_parent.get(&parent) { for t in &all_tasks {
for &child_id in children { if t.parent_id == Some(parent) && to_delete.insert(t.id) {
if to_delete.insert(child_id) { frontier.push(t.id);
frontier.push(child_id);
}
} }
} }
} }

View file

@ -116,7 +116,13 @@ impl AppConfig {
std::fs::create_dir_all(parent)?; std::fs::create_dir_all(parent)?;
} }
let content = serde_json::to_string_pretty(&self)?; let content = serde_json::to_string_pretty(&self)?;
crate::storage::atomic_write(path, content.as_bytes())?; // Atomic write: write to temp file then rename to prevent corruption on crash
let temp = path.with_extension("tmp");
std::fs::write(&temp, &content)?;
if let Err(e) = std::fs::rename(&temp, path) {
let _ = std::fs::remove_file(&temp);
return Err(e.into());
}
Ok(()) Ok(())
} }

View file

@ -358,15 +358,8 @@ pub async fn sync_google_tasks(
list_meta.task_order = task_order; list_meta.task_order = task_order;
list_meta.updated_at = Utc::now(); list_meta.updated_at = Utc::now();
match serde_json::to_string_pretty(&list_meta) { if let Ok(meta_content) = serde_json::to_string_pretty(&list_meta) {
Ok(meta_content) => { let _ = atomic_write(&listdata_path, meta_content.as_bytes());
if let Err(e) = atomic_write(&listdata_path, meta_content.as_bytes()) {
errors.push(format!("Failed to write metadata for list '{}': {}", gt_list.title, e));
}
}
Err(e) => {
errors.push(format!("Failed to serialize metadata for list '{}': {}", gt_list.title, e));
}
} }
} }
@ -381,15 +374,8 @@ pub async fn sync_google_tasks(
RootMetadata::default() RootMetadata::default()
}; };
root_meta.list_order = new_list_order; root_meta.list_order = new_list_order;
match serde_json::to_string_pretty(&root_meta) { if let Ok(meta_content) = serde_json::to_string_pretty(&root_meta) {
Ok(meta_content) => { let _ = atomic_write(&root_meta_path, meta_content.as_bytes());
if let Err(e) = atomic_write(&root_meta_path, meta_content.as_bytes()) {
errors.push(format!("Failed to write workspace metadata: {}", e));
}
}
Err(e) => {
errors.push(format!("Failed to serialize workspace metadata: {}", e));
}
} }
Ok(GoogleSyncResult { downloaded, errors }) Ok(GoogleSyncResult { downloaded, errors })

View file

@ -236,8 +236,12 @@ impl FileSystemStorage {
Ok(path) Ok(path)
} }
fn sanitize_filename(name: &str) -> String {
crate::sanitize_filename(name)
}
fn task_file_path(&self, list_dir: &Path, task: &Task) -> PathBuf { fn task_file_path(&self, list_dir: &Path, task: &Task) -> PathBuf {
let safe_title = crate::sanitize_filename(&task.title); let safe_title = Self::sanitize_filename(&task.title);
let filename = if safe_title.is_empty() { let filename = if safe_title.is_empty() {
task.id.to_string() task.id.to_string()
} else { } else {
@ -454,9 +458,7 @@ impl Storage for FileSystemStorage {
let mut tasks = Vec::new(); let mut tasks = Vec::new();
for (_id, entries) in by_id { for (_id, entries) in by_id {
// `by_id` only inserts non-empty groups, so each `entries` has at let winner = if entries.len() > 1 {
// least one element.
let task = if entries.len() > 1 {
// Read mtime once per file so sort_by doesn't hit the filesystem // Read mtime once per file so sort_by doesn't hit the filesystem
// O(n log n) times and can't produce inconsistent orderings if a // O(n log n) times and can't produce inconsistent orderings if a
// file is touched mid-sort. // file is touched mid-sort.
@ -481,14 +483,12 @@ impl Storage for FileSystemStorage {
eprintln!("Warning: failed to remove stale duplicate task file {:?}: {}", stale_path, e); eprintln!("Warning: failed to remove stale duplicate task file {:?}: {}", stale_path, e);
} }
} }
let (_, t, _) = with_mtime.into_iter().next() with_mtime.into_iter().next().map(|(_, t, _)| t)
.expect("dedup group is non-empty after drain(1..)");
t
} else { } else {
let (_, t) = entries.into_iter().next() entries.into_iter().next().map(|(_, t)| t)
.expect("dedup group is non-empty");
t
}; };
let task = winner
.ok_or_else(|| Error::InvalidData("Empty dedup entries for task".to_string()))?;
tasks.push(task); tasks.push(task);
} }

View file

@ -204,9 +204,8 @@ pub fn compute_sync_actions(
} }
// Remote present, local gone, base known: local was deleted // Remote present, local gone, base known: local was deleted
(None, Some(r), Some(b)) => { (None, Some(_), Some(b)) => {
let remote_changed = r.size != b.size let remote_changed = remote.is_some_and(|r| r.size != b.size || !timestamps_equal(r.last_modified.as_deref(), b.modified_at.as_deref()));
|| !timestamps_equal(r.last_modified.as_deref(), b.modified_at.as_deref());
if remote_changed { if remote_changed {
// deleted locally + modified remotely -> download (remote wins) // deleted locally + modified remotely -> download (remote wins)
actions.push(SyncAction::Download { path: path.to_string() }); actions.push(SyncAction::Download { path: path.to_string() });
@ -230,22 +229,6 @@ pub fn compute_sync_actions(
actions actions
} }
/// Remove base entries for files that are gone from both local and remote.
/// `compute_sync_actions` emits no action for the both-deleted case, so without
/// this pass those entries would persist in `.syncstate.json` indefinitely.
fn prune_orphan_bases(
sync_state: &mut SyncState,
local_files: &[LocalFileInfo],
remote_files: &[RemoteFileSnapshot],
) {
let live_paths: std::collections::HashSet<&str> = local_files
.iter()
.map(|f| f.path.as_str())
.chain(remote_files.iter().map(|f| f.path.as_str()))
.collect();
sync_state.files.retain(|p, _| live_paths.contains(p.as_str()));
}
/// Compare two timestamps for equality by parsing both, tolerating format differences. /// Compare two timestamps for equality by parsing both, tolerating format differences.
fn timestamps_equal(a: Option<&str>, b: Option<&str>) -> bool { fn timestamps_equal(a: Option<&str>, b: Option<&str>) -> bool {
match (a, b) { match (a, b) {
@ -621,12 +604,6 @@ async fn sync_workspace_inner(
} }
}; };
// Purge orphan base entries: files we previously tracked that are now gone
// from both local and remote. Without this, `.syncstate.json` accumulates
// ghost entries forever because the both-deleted diff case emits no action
// and so nothing else would clean them.
prune_orphan_bases(&mut sync_state, &local_files, &remote_files);
// Compute actions from three-way diff // Compute actions from three-way diff
let fresh_actions = compute_sync_actions(&local_files, &remote_files, &sync_state); let fresh_actions = compute_sync_actions(&local_files, &remote_files, &sync_state);
@ -724,20 +701,19 @@ async fn execute_action(
Err(e) => return Err(e.into()), Err(e) => return Err(e.into()),
}; };
let checksum = compute_checksum(&data); let checksum = compute_checksum(&data);
let len = data.len() as u64;
if let Some(parent) = path_parent(path) { if let Some(parent) = path_parent(path) {
client.ensure_dir(parent).await?; client.ensure_dir(parent).await?;
} }
report(&format!(" ^ Uploading {}", path)); report(&format!(" ^ Uploading {}", path));
client.put_file(path, data).await?; client.put_file(path, data.clone()).await?;
// Record in sync state using local file metadata // Record in sync state using local file metadata
let modified = std::fs::metadata(&local_path).ok() let modified = std::fs::metadata(&local_path).ok()
.and_then(|m| m.modified().ok()) .and_then(|m| m.modified().ok())
.map(|t| { let dt: DateTime<Utc> = t.into(); dt.to_rfc3339() }); .map(|t| { let dt: DateTime<Utc> = t.into(); dt.to_rfc3339() });
sync_state.record_file(path, &checksum, modified.as_deref(), len); sync_state.record_file(path, &checksum, modified.as_deref(), data.len() as u64);
} }
SyncAction::Conflict { path } => { SyncAction::Conflict { path } => {
@ -915,15 +891,9 @@ pub fn get_sync_status(workspace_path: &Path) -> Result<SyncStatusInfo> {
} }
} }
// Count files in base that are now missing locally (deleted). // Count files in base that are now missing locally (deleted)
// Build a set of local paths once so the membership check is O(1) per
// tracked file instead of scanning local_files linearly each time.
let local_paths: std::collections::HashSet<&str> = local_files
.iter()
.map(|f| f.path.as_str())
.collect();
for path in sync_state.files.keys() { for path in sync_state.files.keys() {
if !local_paths.contains(path.as_str()) { if !local_files.iter().any(|f| f.path == *path) {
pending_changes += 1; pending_changes += 1;
} }
} }
@ -1136,22 +1106,6 @@ mod tests {
assert!(actions.is_empty()); assert!(actions.is_empty());
} }
#[test]
fn test_prune_orphan_bases() {
let mut state = SyncState::default();
state.files.insert("kept_local.md".to_string(), make_base("a"));
state.files.insert("kept_remote.md".to_string(), make_base("b"));
state.files.insert("orphan.md".to_string(), make_base("c"));
let local = vec![make_local("kept_local.md", "a")];
let remote = vec![make_remote("kept_remote.md")];
prune_orphan_bases(&mut state, &local, &remote);
assert!(state.files.contains_key("kept_local.md"));
assert!(state.files.contains_key("kept_remote.md"));
assert!(!state.files.contains_key("orphan.md"));
}
#[test] #[test]
fn test_multiple_files_mixed() { fn test_multiple_files_mixed() {
let local = vec![ let local = vec![

View file

@ -456,7 +456,7 @@ All metadata and state files use an atomic write pattern (write to `.tmp` then r
- **List names**: Rejected if they contain `/`, `\`, or `..` components. Canonicalized and verified to stay within workspace root. - **List names**: Rejected if they contain `/`, `\`, or `..` components. Canonicalized and verified to stay within workspace root.
- **Sync paths**: Validated to reject `..` components and backslashes anywhere in the path before any file system operation. - **Sync paths**: Validated to reject `..` components and backslashes anywhere in the path before any file system operation.
- **Workspace paths** (Tauri): Rejected if they point to the filesystem root (`/`) or system directories (`/etc`, `/usr`, `/bin`, `/sbin`, `/var`, `/proc`, `/sys`, `/dev`). - **Workspace paths** (Tauri): Rejected if they point to system directories (`/etc`, `/usr`, `/bin`, etc.).
- **Filenames**: Sanitized to replace `/ \ : * ? " < > |` and control characters with `_`. - **Filenames**: Sanitized to replace `/ \ : * ? " < > |` and control characters with `_`.
## Example: Complete Workflow ## Example: Complete Workflow
@ -523,9 +523,9 @@ Key test areas:
## Thread Safety ## Thread Safety
`TaskRepository` holds its storage as `Box<dyn Storage + Send + Sync>`, so any concrete storage implementation passed in must be `Send + Sync`. Repository instances can be shared across threads behind a `Mutex` — the Tauri GUI uses `Mutex<AppState>` for this purpose. The `Storage` trait requires `Send + Sync`, and `TaskRepository` wraps `Box<dyn Storage + Send + Sync>`, so repository instances can be shared across threads behind a `Mutex`. The Tauri GUI uses `Mutex<AppState>` for this purpose.
For concurrent access: For concurrent access:
1. Wrap `TaskRepository` in `Mutex` or `RwLock` (the Tauri app does this) 1. Wrap `TaskRepository` in `Mutex` or `RwLock` (the Tauri app does this)
2. Or create separate repository instances per thread. Note that `FileSystemStorage` does not coordinate writes between processes — concurrent multi-process writes to the same workspace are not supported outside the WebDAV sync flow, which uses a `.sync.lock` file. 2. Or create separate repository instances per thread (file system handles locking)