Merge pull request #75 from sohamM97/code-review

Round 11 code review: sync/state-sync fixes + coverage

Author: sohamM97

CLAUDE.md

@@ -31,7 +31,7 @@ flutter test --coverage                           # with coverage
 
 - **App shell:** `AppShell` in `main.dart` hosts a 3-tab `PageView` (Today, Starred, All Tasks) with `NavigationBar`. Completed tasks are accessed via archive icon in each tab's AppBar. Tabs use `AutomaticKeepAliveClientMixin` to preserve state across switches. `TaskProvider` manages a navigation stack (`_parentStack`) for drill-down within All Tasks.
 - **DB schema:** Core tables: `tasks`, `task_relationships` (parent_id, child_id), `task_dependencies` (blocker relationships), `todays_five_state` (daily pin state), `task_schedules` (recurring), `sync_queue` (pending mutations). Multi-parent DAG with cycle prevention via recursive CTE (`DatabaseHelper.hasPath()`).
-- **DB migrations:** Sequential `onUpgrade` in `DatabaseHelper` (currently at v22, constant `_dbVersion`). Foreign keys via `PRAGMA foreign_keys = ON`. **NEVER modify a released migration** — if a previous version shipped with migration N, new DDL must go in migration N+1 or later. Retroactive changes to released migrations are silently skipped on devices already past that version. Also update `_validateBackup` version check (uses `_dbVersion` automatically) and the backup version test.
+- **DB migrations:** Sequential `onUpgrade` in `DatabaseHelper` (currently at v23, constant `_dbVersion`). Foreign keys via `PRAGMA foreign_keys = ON`. **NEVER modify a released migration** — if a previous version shipped with migration N, new DDL must go in migration N+1 or later. Retroactive changes to released migrations are silently skipped on devices already past that version. Also update `_validateBackup` version check (uses `_dbVersion` automatically) and the backup version test.
 - **Cloud sync:** Optional Google Sign-In + Firestore via REST APIs (no Firebase SDK). `SyncService` orchestrates push/pull; mutations queued in `sync_queue` table and debounced. **Mutation flow:** `TaskProvider.onMutation` callback → `SyncService.schedulePush()` (5s debounce) → batch REST calls to Firestore. Pull runs on startup + every 5 min.
 - **When adding a column to `tasks`:** Also add to `Task` model (`toMap`/`fromMap`/`copyWith`), `taskToFirestoreFields`, and `taskFromFirestoreDoc` in `firestore_service.dart`.
 - **Provider pattern:** 3 providers (`TaskProvider`, `AuthProvider`, `ThemeProvider`) + `SyncService` via `ProxyProvider<AuthProvider, SyncService>`. `TaskProvider._refreshCurrentList()` reloads children only, NOT `_currentParent` — see gotcha below. **Every provider method that mutates data must call `_refreshAfterMutation()`** (which reloads the task list, auxiliary data, and calls `notifyListeners` + `onMutation`). Calling only `onMutation` without `_refreshAfterMutation` causes stale UI — icons, indicators, and sort order won't update until the user navigates away and back.

docs/CODE_REVIEW.md

@@ -33,8 +33,9 @@ Reference for manual test instructions. Use these names consistently.
 
 ### All Tasks tab
 - **Task card** — grid card for each task. Shows task name, top-left indicator icons (Today's 5, worked-on, started, priority, someday, deadline, scheduled today, starred). **Tap** navigates into the task. **Long-press** opens a context-menu bottom sheet. Items: **Rename**, **Also show under...** (add another parent), **Do after...** (add/change dependency), **Schedule**, **Delete**, plus two that appear **only when drilled into a parent (non-root), never at root level**: **Move to...** (`drive_file_move_outline` — move from the current list to another) and **Remove from here** (unlink from the current parent). At root level a task has no "current list" to move from or unlink from, so both are hidden.
+  - **Known limitation — "Also show under..." on a root task removes it from the root list.** The root level shows only **parentless** tasks (root = the *absence* of a parent, not a parent itself). So the first time you "Also show under..." a task that currently sits at root, it gains a parent and therefore **disappears from the root list**, appearing under that parent instead — despite the "*also* show under" wording implying it would stay at root as well. A second "Also show under..." then correctly adds an *additional* parent (the task is already non-root). To keep a copy visible at the top level you would need a genuine root pseudo-parent, which does not exist. This is pre-existing DAG behavior, unrelated to any specific fix — noted here to avoid manual-test confusion.
 - **Task list** — hierarchical list. Shows children of the current parent. Root level shows top-level tasks + Inbox.
-- **Inbox section** — collapsible section at top showing unorganized tasks.
+- **Inbox section** — collapsible section at top showing unorganized (inbox) tasks. Tapping the section header toggles expand/collapse; a **"File all"** action files every inbox task in one pass. **Interactions on an inbox task row:** a **tap (single press)** opens the **"File under…"** (triage) dialog to file it under a parent (or keep it at top level, which dismisses it from the inbox); a **long-press** drills **into** the task (navigateInto). This is the inverse of the main list rows, where a tap navigates in — inbox tasks prioritize filing, so tap = file, long-press = open.
 - **Leaf detail view** — appears when navigating into a leaf task (a task with no children). Shows task name, "Done today" button, "Done for good!" button, "Start"/"Started" toggle, priority selector, schedule/deadline info, dependencies, parent breadcrumbs. This is NOT the same as the Today's 5 bottom sheet.
   - **"Do after..." icon (add_task)** — shown when the task has **no** dependency. **Tap** opens the "Do X after..." picker dialog to add a dependency.
   - **Dependency icon (hourglass)** — replaces the "Do after..." icon when the task has a dependency. **Tap** navigates to the blocker task. **Long-press** opens the "Do X after..." picker dialog to change or remove the dependency. Color behavior: **primary color** when actively blocked, **greyed out** when the blocker is no longer blocking (e.g. marked "Done today"). When the blocker is completed ("Done for good") or skipped, the dependency row is deleted from the DB, so the hourglass **disappears entirely**.

docs/TEST_COVERAGE.md

@@ -568,13 +568,73 @@ class DatabaseHelper {
     return insertedIds;
   }
 
+  /// Inserts a task and links it to every id in [parentIds] — task row, all
+  /// relationship edges, and their sync-queue entries — in ONE transaction, so
+  /// a crash can't leave an orphan or partially-parented task. Returns the id.
+  //
+  // CR-fix R-10: addTask() previously ran insertTask() in its own transaction
+  // and then a SEPARATE addRelationship() transaction per parent, so the task
+  // row and its edges were not one atomic unit. A brand-new task can't form a
+  // cycle, so (unlike addRelationship's callers) no hasPath check is needed.
+  Future<int> insertTaskWithParents(Task task, List<int> parentIds) async {
+    final db = await database;
+    final now = DateTime.now().millisecondsSinceEpoch;
+    return await db.transaction((txn) async {
+      final map = task.toMap();
+      map['sync_id'] ??= _uuid.v4();
+      map['updated_at'] = now;
+      map['sync_status'] = 'pending';
+      final childSyncId = map['sync_id'] as String;
+      final childId = await txn.insert('tasks', map);
+      if (parentIds.isNotEmpty) {
+        // Resolve parent sync_ids in one query for the sync-queue enqueue.
+        final placeholders = parentIds.map((_) => '?').join(',');
+        final rows = await txn.rawQuery(
+            'SELECT id, sync_id FROM tasks WHERE id IN ($placeholders)',
+            parentIds);
+        final parentSyncIds = <int, String>{};
+        for (final r in rows) {
+          final sid = r['sync_id'] as String?;
+          if (sid != null) parentSyncIds[r['id'] as int] = sid;
+        }
+        final seen = <int>{};
+        for (final parentId in parentIds) {
+          if (!seen.add(parentId)) continue; // de-dupe repeated parents
+          await txn.insert('task_relationships', {
+            'parent_id': parentId,
+            'child_id': childId,
+          }, conflictAlgorithm: ConflictAlgorithm.ignore);
+          final parentSyncId = parentSyncIds[parentId];
+          if (parentSyncId != null) {
+            await txn.insert('sync_queue', {
+              'entity_type': 'relationship',
+              'action': 'add',
+              'key1': parentSyncId,
+              'key2': childSyncId,
+              'created_at': now,
+            });
+          }
+        }
+      }
+      return childId;
+    });
+  }
+
   Future<void> addRelationship(int parentId, int childId) async {
     final db = await database;
     await db.transaction((txn) async {
-      await txn.insert('task_relationships', {
+      // CR-fix M-45: was the default 'abort' conflict algorithm — re-inserting
+      // an existing (parent_id, child_id) edge threw a UNIQUE-constraint error.
+      // Callers only run the hasPath cycle check (which is false for an
+      // already-existing edge), so a future caller re-adding an edge would
+      // crash (inconsistent with moveTask/fileTask, which pre-check). Ignore
+      // makes it a no-op, and we skip the redundant sync 'add' enqueue below
+      // when nothing was actually inserted.
+      final inserted = await txn.insert('task_relationships', {
         'parent_id': parentId,
         'child_id': childId,
-      });
+      }, conflictAlgorithm: ConflictAlgorithm.ignore);
+      if (inserted == 0) return;
       final rows = await txn.rawQuery(
         'SELECT id, sync_id FROM tasks WHERE id IN (?, ?)', [parentId, childId]);
       final syncIds = <int, String>{};
@@ -1184,31 +1244,44 @@ class DatabaseHelper {
       }
     }
 
-    // For tasks without own deadline, check ancestors
+    // For tasks without own deadline, inherit the nearest ancestor's deadline.
+    // CR-fix I-52: this was a per-task loop, each iteration running a full
+    // recursive ancestor CTE — an N+1 on the refresh hot path
+    // (_loadAuxiliaryData runs on every mutation). A root with 50 undeadlined
+    // children fired 50 recursive walks per tap. Now one batched recursive CTE
+    // per 500-id chunk, keyed by target_id (same shape as
+    // getEffectiveScheduledTodayIds). Rows come back nearest-ancestor-first per
+    // target; we keep the first hit for each, matching the old ORDER BY depth
+    // ASC LIMIT 1 semantics.
     final remaining = taskIds.where((id) => !result.containsKey(id)).toList();
     for (var i = 0; i < remaining.length; i += 500) {
       final batch = remaining.sublist(i, i + 500 > remaining.length ? remaining.length : i + 500);
-      for (final taskId in batch) {
-        final rows = await db.rawQuery('''
-          WITH RECURSIVE ancestors(id, depth) AS (
-            SELECT parent_id, 1 FROM task_relationships WHERE child_id = ?
-            UNION ALL
-            SELECT tr.parent_id, a.depth + 1
-            FROM task_relationships tr
-            INNER JOIN ancestors a ON tr.child_id = a.id
-          )
-          SELECT t.deadline, t.deadline_type FROM tasks t
-          INNER JOIN ancestors a ON t.id = a.id
-          WHERE t.deadline IS NOT NULL
-          ORDER BY a.depth ASC
-          LIMIT 1
-        ''', [taskId]);
-        if (rows.isNotEmpty) {
-          result[taskId] = (
-            deadline: rows.first['deadline'] as String,
-            type: rows.first['deadline_type'] as String? ?? 'due_by',
-          );
-        }
+      final placeholders = batch.map((_) => '?').join(',');
+      final rows = await db.rawQuery('''
+        WITH RECURSIVE target_ancestors(target_id, ancestor_id, depth) AS (
+          SELECT tr.child_id, tr.parent_id, 1
+          FROM task_relationships tr
+          WHERE tr.child_id IN ($placeholders)
+          UNION ALL
+          SELECT ta.target_id, tr.parent_id, ta.depth + 1
+          FROM target_ancestors ta
+          INNER JOIN task_relationships tr ON tr.child_id = ta.ancestor_id
+        )
+        SELECT ta.target_id AS target_id, t.deadline AS deadline,
+               t.deadline_type AS deadline_type
+        FROM target_ancestors ta
+        INNER JOIN tasks t ON t.id = ta.ancestor_id
+        WHERE t.deadline IS NOT NULL
+        ORDER BY ta.target_id ASC, ta.depth ASC
+      ''', batch);
+      for (final row in rows) {
+        final targetId = row['target_id'] as int;
+        // Nearest-first ordering → the first row per target is the shallowest.
+        if (result.containsKey(targetId)) continue;
+        result[targetId] = (
+          deadline: row['deadline'] as String,
+          type: row['deadline_type'] as String? ?? 'due_by',
+        );
       }
     }
     return result;
@@ -2487,6 +2560,49 @@ class DatabaseHelper {
     }
   }
 
+  /// Returns the sync_ids of every local task that has one.
+  // CR-fix I-49: used by the full-pull task reconciliation to find local tasks
+  // that no longer exist remotely (deleted on another device).
+  Future<List<String>> getAllTaskSyncIds() async {
+    final db = await database;
+    final rows = await db.query('tasks',
+        columns: ['sync_id'], where: 'sync_id IS NOT NULL');
+    return rows.map((r) => r['sync_id'] as String).toList();
+  }
+
+  /// Returns the sync_ids of tasks pending push (locally created/edited).
+  // CR-fix I-49: guards the delta/full-pull deletion path so a task that was
+  // just re-created locally (not yet pushed) isn't dropped by a stale tombstone
+  // or by absence from a remote set — mirrors getPendingSyncAddKeys for edges.
+  Future<Set<String>> getPendingTaskSyncIds() async {
+    final db = await database;
+    final rows = await db.query('tasks',
+        columns: ['sync_id'],
+        where: "sync_status = 'pending' AND sync_id IS NOT NULL");
+    return rows.map((r) => r['sync_id'] as String).toSet();
+  }
+
+  /// Removes a task in response to a remote deletion (tombstone / absence on
+  /// full pull). Does NOT enqueue a sync deletion — we are applying remote
+  /// state, mirroring [removeRelationshipFromRemote]. Returns true if a row was
+  /// removed. Edges/deps/schedules cascade via ON DELETE CASCADE; the today's-5
+  /// tables have no FK, so they are cleaned explicitly to avoid orphans.
+  Future<bool> deleteTaskBySyncId(String syncId) async {
+    final db = await database;
+    return await db.transaction((txn) async {
+      final rows = await txn.query('tasks',
+          columns: ['id'], where: 'sync_id = ?', whereArgs: [syncId]);
+      if (rows.isEmpty) return false;
+      final id = rows.first['id'] as int;
+      await txn.delete('todays_five_state',
+          where: 'task_id = ?', whereArgs: [id]);
+      await txn.delete('todays_five_deadline_suppressed',
+          where: 'task_id = ?', whereArgs: [id]);
+      await txn.delete('tasks', where: 'id = ?', whereArgs: [id]);
+      return true;
+    });
+  }
+
   /// Returns all relationships as (parentSyncId, childSyncId) pairs.
   Future<List<({String parentSyncId, String childSyncId})>> getAllRelationshipsWithSyncIds() async {
     final db = await database;

docs/UI_VIEWS.md

@@ -179,24 +179,29 @@ class TaskProvider extends ChangeNotifier {
         isInbox: isInbox,
         isStarred: isStarred,
         starOrder: starOrder);
-    final taskId = await _db.insertTask(task);
 
+    // CR-fix R-10: collect all parent ids and insert the task + every edge in a
+    // single transaction (insertTaskWithParents) instead of insertTask() plus
+    // one addRelationship() txn per parent — the old split wasn't atomic, so a
+    // crash mid-way left an orphan or partially-parented task.
+    //
     // [atRoot] forces a root-level insert regardless of current navigation
     // state. Used by callers in other tabs (Today's 5, Starred) so the new
     // task isn't silently nested under whatever parent the All Tasks tab last
     // drilled into. (isInbox also implies root, enforced by the invariant
     // above — so plain atRoot suffices here.)
+    final parentIds = <int>[];
     if (!atRoot && _currentParent != null) {
-      await _db.addRelationship(_currentParent!.id!, taskId);
+      parentIds.add(_currentParent!.id!);
     }
-
     if (additionalParentIds != null) {
       for (final parentId in additionalParentIds) {
         if (atRoot || parentId != _currentParent?.id) {
-          await _db.addRelationship(parentId, taskId);
+          parentIds.add(parentId);
         }
       }
     }
+    final taskId = await _db.insertTaskWithParents(task, parentIds);
 
     // Bug fix: without deferNotify, _refreshAfterMutation() fired immediately
     // after task insert, triggering notifyListeners -> refreshSnapshots() ->
@@ -304,7 +309,12 @@ class TaskProvider extends ChangeNotifier {
             orElse: () => throw StateError('Task $taskId not found in current list'));
     final removedDeps = await _db.completeTask(taskId);
     onMutation?.call();
-    await navigateBack();
+    // CR-fix M-46: navigateBack() refreshes only when _parentStack is non-empty
+    // (returns false otherwise). On the empty-stack branch the DB mutation +
+    // sync push already fired but nothing refreshed the list / notifyListeners,
+    // leaving stale UI. Fall back to a refresh. (onMutation already fired above,
+    // so refresh the list directly rather than re-firing it.)
+    if (!await navigateBack()) await _refreshCurrentList();
     return (task: task, removedDeps: removedDeps);
   }
 
@@ -317,7 +327,8 @@ class TaskProvider extends ChangeNotifier {
             orElse: () => throw StateError('Task $taskId not found in current list'));
     final removedDeps = await _db.skipTask(taskId);
     onMutation?.call();
-    await navigateBack();
+    // CR-fix M-46: refresh on the empty-stack branch too (see completeTask).
+    if (!await navigateBack()) await _refreshCurrentList();
     return (task: task, removedDeps: removedDeps);
   }
 
@@ -795,7 +806,8 @@ class TaskProvider extends ChangeNotifier {
     await _db.markWorkedOn(taskId);
     if (alsoStart) await _db.startTask(taskId);
     onMutation?.call();
-    await navigateBack(); // single _refreshCurrentList()
+    // CR-fix M-46: refresh on the empty-stack branch too (see completeTask).
+    if (!await navigateBack()) await _refreshCurrentList();
   }
 
   Future<void> unmarkWorkedOn(int taskId, {int? restoreTo}) async {