0.15.0 polish

[pedramamini]

• Add daily backup system with 7-day rotation and auto-restore on corruption
• Fix tab bar scroll to keep active tab fully visible after rename

[claude]

Code Review: PR #305 - 0.15.0 Polish

This PR adds a robust daily backup system with automatic corruption recovery and fixes a tab scrolling issue. Overall, the implementation is solid with comprehensive test coverage. Here are my observations:

---

🟢 Strengths

Daily Backup System

• Smart auto-recovery: The corruption recovery logic now attempts to restore from backups before falling back to a fresh database, which is excellent for data preservation
• 7-day rotation: Automatic cleanup prevents disk bloat
• Legacy format support: The getAvailableBackups() method handles both the new daily.YYYY-MM-DD format and legacy timestamp-based backups
• Comprehensive testing: 179 lines of new tests cover edge cases well

TabBar Fix

• Correct approach: Using activeTabName as a dependency is the right way to trigger scroll when the tab width changes after rename
• Well-documented: The comment explains the multiple trigger conditions clearly

---

🟡 Areas for Improvement

1. Resource Leak Risk in recoverFromCorruption()

Location: src/main/stats/stats-db.ts:552-554

const testDb = new Database(backup.path, { readonly: true });
const result = testDb.pragma('integrity_check') as Array<{ integrity_check: string }>;
testDb.close();

Issue: If pragma('integrity_check') throws an exception, testDb is never closed, leaving the SQLite connection open.

Recommendation: Use try-finally to ensure cleanup:

let testDb: Database.Database | null = null;
try {
  testDb = new Database(backup.path, { readonly: true });
  const result = testDb.pragma('integrity_check') as Array<{ integrity_check: string }>;
  
  if (result.length === 1 && result[0].integrity_check === 'ok') {
    // Backup is valid, restore it
    if (this.restoreFromBackup(backup.path)) {
      logger.info(`Successfully restored database from backup: ${backup.date}`, LOG_CONTEXT);
      return {
        recovered: true,
        backupPath: backup.path,
        restoredFromBackup: true,
      };
    }
  } else {
    logger.warn(`Backup ${backup.date} failed integrity check, trying next...`, LOG_CONTEXT);
  }
} catch (error) {
  logger.warn(`Backup ${backup.date} is unreadable: ${error}, trying next...`, LOG_CONTEXT);
} finally {
  if (testDb) {
    try {
      testDb.close();
    } catch {
      // Ignore errors closing test database
    }
  }
}

---

2. Race Condition in restoreFromBackup()

Location: src/main/stats/stats-db.ts:484-489

// Remove WAL and SHM files if they exist
const walPath = `${this.dbPath}-wal`;
const shmPath = `${this.dbPath}-shm`;
if (fs.existsSync(walPath)) fs.unlinkSync(walPath);
if (fs.existsSync(shmPath)) fs.unlinkSync(shmPath);

Issue: There's a TOCTOU (Time-of-Check-Time-of-Use) race between existsSync() and unlinkSync(). If the file is deleted between the check and the unlink, unlinkSync() will throw.

Recommendation: Wrap in try-catch since these are cleanup operations:

// Remove WAL and SHM files if they exist
const walPath = `${this.dbPath}-wal`;
const shmPath = `${this.dbPath}-shm`;
try {
  if (fs.existsSync(walPath)) fs.unlinkSync(walPath);
} catch {
  // Ignore errors - WAL file may have been removed
}
try {
  if (fs.existsSync(shmPath)) fs.unlinkSync(shmPath);
} catch {
  // Ignore errors - SHM file may have been removed
}

The same pattern appears in recoverFromCorruption() at lines 542-543 and should be similarly protected.

---

3. Missing Error Handling in createDailyBackupIfNeeded()

Location: src/main/stats/stats-db.ts:370

// Create today's backup
fs.copyFileSync(this.dbPath, dailyBackupPath);

Issue: If the disk is full or permissions are insufficient, this will throw and the outer catch will only log a warning. The backup creation should be atomic to avoid partial files.

Recommendation: Consider copying to a temp location first, then renaming:

// Create today's backup
const tempPath = `${dailyBackupPath}.tmp`;
try {
  fs.copyFileSync(this.dbPath, tempPath);
  fs.renameSync(tempPath, dailyBackupPath);
  logger.info(`Created daily backup: ${dailyBackupPath}`, LOG_CONTEXT);
} catch (error) {
  // Clean up temp file if it exists
  try {
    if (fs.existsSync(tempPath)) fs.unlinkSync(tempPath);
  } catch {
    // Ignore cleanup errors
  }
  throw error; // Re-throw to be caught by outer handler
}

---

4. Test Coverage Gap

Location: src/__tests__/main/stats/stats-db.test.ts

The tests verify that backup creation is attempted but don't verify:

• What happens if backup creation fails (disk full, permissions)
• That rotateOldBackups() correctly parses dates and removes old backups
• That the 7-day rotation actually works with real date strings

Recommendation: Add tests for backup failure handling and rotation verification.

---

5. TabBar Performance Consideration

Location: src/renderer/components/TabBar.tsx:1603-1604

const activeTab = tabs.find((t) => t.id === activeTabId);
const activeTabName = activeTab?.name ?? null;

Issue: This find() operation runs on every render. For large tab lists, this could be optimized with useMemo.

However, this is likely premature optimization since tab lists are typically small (<100 tabs). Consider only if performance issues are observed.

---

🔒 Security & Best Practices

✅ No SQL injection risks: All file operations use safe path joining
✅ No command injection: No shell commands executed
✅ Proper error logging: All errors are logged with context
✅ Sentry integration: Following the project's error handling guidelines

---

📊 Test Coverage

Strong coverage for the backup system:

• Daily backup creation and skipping if exists
• Legacy format support
• Backup listing and sorting
• Restoration logic

Missing edge cases (as noted above):

• Failure scenarios (disk full, permissions)
• Date-based rotation verification
• Resource cleanup on errors

---

✅ Verdict

This is a solid PR that significantly improves data durability. The corruption recovery with automatic restore is a major improvement over the previous "create fresh DB" approach.

The issues identified are mostly defensive improvements rather than critical bugs. The resource leak in recoverFromCorruption() should be fixed before merge, but the others can be addressed in follow-up PRs if preferred.

Recommendation: Approve with suggested fixes for the resource leak and TOCTOU issues.