Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Jan 8, 2026

Addressed 9 code review comments on the initial log rotation PR to improve robustness, prevent data loss, and fix race conditions.

String handling

  • Replace replaceAll('.log', '') with endsWith() + substring() to handle edge cases like my.log.log

Race condition fix

  • Made initLogger synchronous to ensure rotation completes before FileLogPrinter opens file

Data loss prevention

  • Modified _compressFile to return bool indicating success
  • Delete backup only if compression succeeds AND zip file exists with non-zero size
  • Log warning if zip is empty

Code quality

  • Replaced print with debugPrint for consistency
  • Added comprehensive documentation for LogRotation class and methods
  • Clarified backup sorting logic (oldest-first, delete oldest beyond limit)
  • Removed unused currentPath parameter from _cleanupOldBackups

Dependencies

  • Moved archive from dev_dependencies to dependencies (used in production code)
// Before: unsafe deletion
_compressFile(backupPath, zipPath);
File(backupPath).deleteSync();

// After: verified deletion
final compressionSucceeded = _compressFile(backupPath, zipPath);
if (compressionSucceeded && File(zipPath).existsSync()) {
  if (File(zipPath).lengthSync() > 0) {
    File(backupPath).deleteSync();
  }
}

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Copilot AI and others added 2 commits January 8, 2026 10:59
Co-authored-by: jigar-f <132374182+jigar-f@users.noreply.github.com>
Co-authored-by: jigar-f <132374182+jigar-f@users.noreply.github.com>
Copilot AI changed the title [WIP] Add log rotation and compression system to logging service Address code review feedback for log rotation implementation Jan 8, 2026
Copilot AI requested a review from jigar-f January 8, 2026 11:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants