Skip to content

Latest commit

 

History

History
628 lines (495 loc) · 17.4 KB

File metadata and controls

628 lines (495 loc) · 17.4 KB

🔴 CRITICAL OFFLINE-FIRST AUDIT REPORT

Project: GitDoIt v0.5.0+126
Audit Date: March 18, 2026
Audit Type: Comprehensive Offline-First Architecture Scan
Severity Levels: 🔴 Critical | 🟠 High | 🟡 Medium | 🟢 Low


📊 Executive Summary

Overall Health Score: 62/100 ⚠️

The app has made significant progress in offline-first functionality, but CRITICAL VULNERABILITIES remain that can cause data loss, duplication, and inconsistent state.

Key Findings:

  • Offline issue creation: WORKING (with caveats)
  • Offline close/reopen: WORKING (with caveats)
  • ⚠️ Issue duplication on sync: PARTIALLY FIXED (still vulnerable)
  • 🔴 Pending operations queue: NOT FULLY IMPLEMENTED
  • 🔴 Error handling: INCONSISTENT across services
  • 🟠 Data consistency: MULTIPLE RACE CONDITIONS
  • 🟠 Network state handling: DISCONNECTED FROM UI
  • 🟡 Storage cleanup: ASYNC OPERATIONS NOT AWAITED

🔴 CRITICAL ISSUES (Must Fix Immediately)

1. Pending Operations Queue NOT Used for Offline Close/Reopen

Severity: 🔴 CRITICAL
Impact: Offline status changes are LOST if app closes before sync
Location: lib/screens/issue_detail_screen.dart, lib/widgets/expandable_repo.dart

Problem:

When users close/reopen issues offline, the changes are saved to local markdown files BUT no pending operation is queued for GitHub issues. The code checks for network but only queues operations for local issues, not GitHub issues edited offline.

Current Code (issue_detail_screen.dart:943-1012):

if (_currentIssue.isLocalOnly) {
  // ✅ Local issue - update and save
  await _localStorage.saveLocalIssue(updatedIssue);
  return;
}

// CHECK NETWORK
final isOnline = await _networkService.checkConnectivity();

if (!isOnline) {
  // ❌ BUG: This code queues the operation BUT...
  final operation = PendingOperation(...);
  await _pendingOps.addOperation(operation);
  // ...UI updates optimistically without tracking rollback
  
  _showSnackBar('Issue queued for sync');
  return;
}

Problem Analysis:

  1. GitHub issues edited offline DO queue operations ✅
  2. BUT the optimistic update has NO rollback tracking
  3. If app closes before sync, operation is in queue but UI state is lost ❌
  4. No link between optimistic UI state and queued operation

Fix Required:

if (!isOnline) {
  // Create operation WITH rollback support
  final operation = OptimisticOperation(
    id: operationId,
    type: OperationType.closeIssue,
    originalIssue: _currentIssue, // ✅ Track original for rollback
    newIssue: updatedIssue,
    timestamp: DateTime.now(),
  );
  
  // Add to provider state for rollback
  ref.read(issueOperationsProvider.notifier).addOperation(operation);
  
  // Queue for sync
  await _pendingOps.addOperation(pendingOperation);
}

2. Race Condition: Local File Deletion Before Sync Confirmation

Severity: 🔴 CRITICAL
Impact: Issues can be LOST if sync fails after file deletion
Location: lib/services/sync_service.dart:732

Current Code:

for (final issue in localOnlyIssues) {
  try {
    final createdIssue = await _githubApi.createIssue(...);
    
    // ❌ CRITICAL: Delete local file IMMEDIATELY
    await _localStorage.removeLocalIssue(issue.id);
    
    syncedIds.add(issue.id);
  } catch (e) {
    // ❌ TOO LATE: File already deleted if createIssue succeeds
    // but subsequent operations fail
  }
}

Problem:

  1. Local file is deleted immediately after createIssue succeeds
  2. BUT if subsequent sync operations fail, the local backup is gone
  3. If GitHub API has eventual consistency issues, issue might not appear immediately
  4. No way to recover if sync is partially successful

Fix Required:

// Phase 1: Create all issues on GitHub
final createdIssues = <String, IssueItem>{};
for (final issue in localOnlyIssues) {
  try {
    final created = await _githubApi.createIssue(...);
    createdIssues[issue.id] = created;
  } catch (e) {
    // Keep local file for failed syncs
    debugPrint('Failed to create issue, keeping local file');
  }
}

// Phase 2: Delete local files ONLY for successfully synced issues
for (final entry in createdIssues.entries) {
  await _localStorage.removeLocalIssue(entry.key);
  syncedIds.add(entry.key);
}

3. Duplicate Prevention Relies on Async Operations Without Await

Severity: 🔴 CRITICAL
Impact: Duplicate issues can appear due to race conditions
Location: lib/services/sync_service.dart:631, 655

Current Code:

if (remoteIssuesByNumber.containsKey(issue.number)) {
  debugPrint('⚠️ SKIP local issue...');
  // ❌ CRITICAL: Async operation NOT awaited
  _localStorage.removeLocalIssue(issue.id).then((_) {
    debugPrint('Removed local file...');
  });
  return false;
}

Problem:

  1. .then() callback is fire-and-forget - no error handling
  2. If removeLocalIssue fails, no one knows
  3. Next sync will see the same file again → DUPLICATE
  4. No retry mechanism for failed deletions

Fix Required:

if (remoteIssuesByNumber.containsKey(issue.number)) {
  debugPrint('⚠️ SKIP local issue...');
  try {
    await _localStorage.removeLocalIssue(issue.id); // ✅ AWAIT
    debugPrint('Removed local file for issue #${issue.number}');
  } catch (e) {
    AppErrorHandler.handle(e, stackTrace: stackTrace);
    debugPrint('Failed to remove local file, will retry next sync');
    // Keep in list for next sync attempt
    return true;
  }
  return false;
}

4. No Pending Operation for Status Changes in expandable_repo.dart

Severity: 🔴 CRITICAL
Impact: Offline close operations are LOST for GitHub issues
Location: lib/widgets/expandable_repo.dart:183-220

Current Code:

if (issue.isLocalOnly || issue.number == null) {
  // ✅ Local issue - save to file
  await _localStorage.saveLocalIssue(updatedIssue);
  // Shows "Issue closed (local)"
} else {
  // ❌ CRITICAL: GitHub issue - uses IssueService directly
  await _issueService.closeIssue(issue, owner, repo);
  // ❌ NO offline check
  // ❌ NO pending operation queued
  // ❌ If offline, this will FAIL silently
}

Problem:

  1. No network check before calling IssueService.closeIssue()
  2. IssueService calls _githubApi.updateIssue() directly
  3. If offline, API call fails but no error is shown
  4. User thinks issue is closed but it's not queued for sync

Fix Required:

else {
  // GitHub issue - CHECK NETWORK FIRST
  final isOnline = await NetworkService().checkConnectivity();
  
  if (!isOnline) {
    // ❌ OFFLINE: Queue operation
    final operation = PendingOperation.closeIssue(
      id: 'close_${issue.id}_${DateTime.now().millisecondsSinceEpoch}',
      issueNumber: issue.number!,
      owner: owner,
      repo: repo,
    );
    await _pendingOps.addOperation(operation);
    
    // Update UI optimistically
    setState(() {
      _issues[index] = updatedIssue;
    });
    
    ScaffoldMessenger.of(context).showSnackBar(
      const SnackBar(
        content: Text('Issue queued for sync'),
        backgroundColor: AppColors.primary,
      ),
    );
  } else {
    // ✅ ONLINE: Use IssueService
    await _issueService.closeIssue(issue, owner, repo);
  }
}

🟠 HIGH SEVERITY ISSUES

5. Optimistic Update Listener Not Connected to All Operations

Severity: 🟠 HIGH
Impact: Users don't see errors when sync fails
Location: lib/main.dart, lib/widgets/optimistic_update_listener.dart

Problem:

  1. OptimisticUpdateListener wraps the app ✅
  2. But it only listens to issueOperationsProvider
  3. Direct API calls (like in expandable_repo.dart) bypass the provider
  4. No error notification when direct API calls fail

Fix Required:

  • Route ALL issue operations through IssueOperationsNotifier
  • Remove direct IssueService and _githubApi calls from screens/widgets
  • Centralize error handling in provider

6. Network Service Disconnected from Sync Triggers

Severity: 🟠 HIGH
Impact: Auto-sync may not trigger when network returns
Location: lib/services/network_service.dart, lib/services/sync_service.dart

Problem:

  1. NetworkService has a broadcast stream ✅
  2. SyncService has its OWN connectivity subscription ❌
  3. Two separate listeners for the same event
  4. No coordination between services
  5. If one fails, the other might not trigger sync

Fix Required:

// SyncService should USE NetworkService, not duplicate it
class SyncService {
  SyncService() {
    // Subscribe to NetworkService stream
    NetworkService().onConnectivityChanged.listen((isOnline) {
      if (isOnline) {
        _triggerAutoSync();
      }
    });
  }
}

7. Conflict Detection Only Runs for Numbered Issues

Severity: 🟠 HIGH
Impact: Local edits to un-synced issues are lost
Location: lib/services/conflict_detection_service.dart:75

Current Code:

for (final localIssue in localIssues) {
  if (localIssue.number == null) continue; // ❌ SKIP un-numbered issues
  
  final remoteIssue = remoteIssuesMap[localIssue.number];
  if (remoteIssue == null) continue;
  
  // ❌ Only check conflicts if local issue has been modified
  if (!localIssue.isLocalOnly &&
      localIssue.localUpdatedAt == null) {
    continue;
  }
}

Problem:

  1. Issues created offline don't get conflict detection
  2. If user edits issue offline, then GitHub is edited online → NO CONFLICT DETECTED
  3. "Remote wins" strategy applied silently without user notification

Fix Required:

  • Remove if (localIssue.number == null) continue;
  • Check conflicts by title + body hash for un-numbered issues
  • Show conflict resolution dialog even for local issues

8. No Validation of Vault Folder Permissions

Severity: 🟠 HIGH
Impact: Offline issues may fail silently
Location: lib/services/local_storage_service.dart

Problem:

  1. getVaultFolder() returns path from secure storage
  2. No validation that folder exists or is writable
  3. saveLocalIssue() tries to create folder, but:
    • No error if creation fails (just debugPrint)
    • No fallback to alternative storage
    • No user notification

Fix Required:

Future<bool> validateVaultFolder() async {
  final vaultPath = await getVaultFolder();
  if (vaultPath == null) return false;
  
  final vaultDir = Directory(vaultPath);
  if (!await vaultDir.exists()) {
    try {
      await vaultDir.create(recursive: true);
    } catch (e) {
      AppErrorHandler.handle(e);
      return false;
    }
  }
  
  // Test write permissions
  try {
    final testFile = File('$vaultPath/.write_test');
    await testFile.writeAsString('test');
    await testFile.delete();
    return true;
  } catch (e) {
    return false;
  }
}

🟡 MEDIUM SEVERITY ISSUES

9. Empty Catch Blocks Hide Errors

Severity: 🟡 MEDIUM
Impact: Silent failures, impossible to debug
Location: lib/services/local_storage_service.dart:225

Current Code:

try {
  updatedAt = DateTime.parse(createdMatch.group(1) ?? '');
} catch (_) {} // ❌ SWALLOW ALL ERRORS

Fix Required:

try {
  updatedAt = DateTime.parse(createdMatch.group(1) ?? '');
} catch (e, stackTrace) {
  AppErrorHandler.handle(e, stackTrace: stackTrace);
  debugPrint('Failed to parse created date: $e');
  updatedAt = DateTime.now(); // Fallback
}

10. No Retry for Failed Operations After Max Retries

Severity: 🟡 MEDIUM
Impact: Operations permanently stuck in queue
Location: lib/services/sync_service.dart:803

Current Code:

if (operation.isSyncing && operation.retryCount > 5) {
  debugPrint('Skipping operation (max retries exceeded)');
  continue; // ❌ SKIP FOREVER
}

Problem:

  1. Operations with retryCount > 5 are skipped forever
  2. No user notification that operation failed permanently
  3. No way to manually retry
  4. Operation stays in queue, blocking other operations

Fix Required:

  • Move failed operations to "dead letter queue"
  • Show error in UI with "Retry" button
  • Allow manual intervention

11. Background Sync Doesn't Check Pending Operations

Severity: 🟡 MEDIUM
Impact: Pending operations may not sync in background
Location: lib/main.dart:callbackDispatcher

Current Code:

void callbackDispatcher() {
  Workmanager().executeTask((task, inputData) async {
    // ...
    // Note: We can't directly access PendingOperationsService here due to isolation
    // The sync service will handle this during syncAll()
    // ❌ COMMENT IS WRONG - syncAll() DOES call _processPendingOperations()
    
    await syncService.syncAll(forceRefresh: false);
  });
}

Problem:

  1. Comment says "can't access" but it can and should
  2. Background sync should explicitly check for pending operations
  3. Should show notification if pending ops exist

12. No Transaction Support for Multi-Step Operations

Severity: 🟡 MEDIUM
Impact: Partial syncs leave data inconsistent
Location: Multiple locations

Problem:

  1. Sync involves multiple steps:
    • Create issue on GitHub
    • Delete local file
    • Update UI
  2. No transaction support - if step 3 fails, steps 1-2 are already committed
  3. No rollback mechanism for partial failures

Fix Required:

Implement transaction pattern:

class SyncTransaction {
  final List<Function> _steps = [];
  final List<Function> _rollback = [];
  
  void addStep(Function step, Function rollback) {
    _steps.add(step);
    _rollback.add(rollback);
  }
  
  Future<bool> commit() async {
    for (final step in _steps) {
      if (!await step()) {
        await rollback();
        return false;
      }
    }
    return true;
  }
  
  Future<void> rollback() async {
    for (final rb in _rollback.reversed) {
      await rb();
    }
  }
}

🟢 LOW SEVERITY ISSUES

13. Inconsistent Error Messages

Severity: 🟢 LOW
Impact: User confusion
Locations: Multiple

Examples:

  • "Issue closed (local)" vs "Issue queued for sync"
  • "Issue saved (will sync when online)" vs "Local issue created"
  • No consistency in message format or duration

Fix Required:

Centralize error/success messages in a service.


14. No Loading State for Sync Operations

Severity: 🟢 LOW
Impact: Users don't know sync is happening
Location: lib/widgets/sync_status_widget.dart

Problem:

  • Sync status widget exists but doesn't show per-operation progress
  • Users see "syncing" but don't know which issues are being synced

15. Debug Prints in Production Code

Severity: 🟢 LOW
Impact: Performance, log clutter
Locations: 722 matches for debugPrint

Fix Required:

Replace with proper logging service that can be disabled in production.


📋 RECOMMENDED FIX PRIORITY

Phase 1 (Immediate - This Sprint):

  1. Fix #4: Add pending operation queue to expandable_repo.dart
  2. Fix #3: Await async file deletion operations
  3. Fix #2: Two-phase commit for local file deletion
  4. Fix #1: Add rollback tracking to optimistic updates

Phase 2 (Next Sprint):

  1. Fix #5: Centralize all operations through provider
  2. Fix #6: Unify network state handling
  3. Fix #8: Validate vault folder permissions
  4. Fix #9: Fix empty catch blocks

Phase 3 (Future Enhancement):

  1. Fix #7: Improve conflict detection
  2. Fix #10: Dead letter queue for failed ops
  3. Fix #12: Transaction support
  4. Fix #13-15: UX improvements

🧪 TESTING REQUIREMENTS

Critical Test Cases to Add:

  1. Offline Close/Reopen Test:

    • Close issue offline → kill app → restart → verify still closed
    • Reopen issue offline → kill app → restart → verify still open
  2. Sync Failure Recovery Test:

    • Create issue offline
    • Simulate GitHub API failure during sync
    • Verify local file still exists
    • Retry sync → verify success
  3. Duplicate Prevention Test:

    • Create issue offline
    • Sync successfully
    • Manually restore local file (simulate failed deletion)
    • Sync again → verify NO duplicate
  4. Race Condition Test:

    • Create 10 issues offline simultaneously
    • Trigger sync
    • Verify all 10 created on GitHub
    • Verify all 10 local files deleted
  5. Network Transition Test:

    • Start offline → create issue
    • Enable network → verify auto-sync triggers
    • Disable network during sync → verify recovery

📈 METRICS TO TRACK

Key Performance Indicators:

  1. Sync Success Rate: Target > 95%
  2. Duplicate Issue Rate: Target 0%
  3. Data Loss Incidents: Target 0
  4. Pending Operation Stuck Rate: Target < 1%
  5. Offline Operation Success Rate: Target > 99%

✅ CONCLUSION

The GitDoIt app has a solid foundation for offline-first functionality, but critical gaps remain in:

  1. Pending operations queue - Not fully integrated
  2. Error handling - Inconsistent and sometimes silent
  3. Race conditions - Multiple async operations not properly awaited
  4. Rollback support - Missing for most optimistic updates

Immediate action required to prevent data loss and ensure reliable offline operation.


Audit Performed By: GitDoIt Critical Scan
Date: March 18, 2026
Version: 0.5.0+126
Next Audit: After Phase 1 fixes