Skip to content

Conversation

@Tr1sma
Copy link
Owner

@Tr1sma Tr1sma commented Jan 6, 2026

No description provided.

Copilot AI review requested due to automatic review settings January 6, 2026 00:00
@Tr1sma Tr1sma merged commit 8c68739 into main Jan 6, 2026
1 check failed
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds cloud backup functionality to the BookLogger application, enabling users to create ZIP-based backups of their database and book cover images, and restore from these backups. The implementation introduces platform-specific file sharing and file picker services to facilitate cloud storage integration.

Key changes:

  • Refactored backup/restore to use ZIP format instead of single database files, now including cover images
  • Added ShareService and FilePickerService abstractions for cross-platform file operations
  • Enhanced UI with backup/restore buttons in the Settings page

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 21 comments.

Show a summary per file
File Description
BookLoggerApp/Services/ShareService.cs New MAUI-specific service implementing file sharing functionality
BookLoggerApp/Services/FilePickerService.cs New MAUI-specific service implementing cross-platform file picker
BookLoggerApp/MauiProgram.cs Registers new ShareService and FilePickerService as singletons
BookLoggerApp/Components/Pages/Settings.razor Adds Backup & Restore UI section with buttons for cloud backup and restore operations
BookLoggerApp.Tests/Services/LookupServiceSecurityTests.cs Removed security test file (unrelated to backup feature)
BookLoggerApp.Tests/Integration/ImportExportServiceZipIntegrationTests.cs New integration tests validating backup/restore ZIP functionality
BookLoggerApp.Infrastructure/Services/ImportExportService.cs Refactored CreateBackupAsync and RestoreFromBackupAsync to use ZIP format with cover images
BookLoggerApp.Infrastructure/Services/FileSystemAdapter.cs Added DirectoryExists method to support new backup functionality
BookLoggerApp.Core/ViewModels/SettingsViewModel.cs Added BackupToCloudAsync and RestoreFromBackupAsync commands
BookLoggerApp.Core/Services/Abstractions/IShareService.cs New interface for platform-specific file sharing
BookLoggerApp.Core/Services/Abstractions/IFileSystem.cs Added DirectoryExists method (with duplicate documentation)
BookLoggerApp.Core/Services/Abstractions/IFilePickerService.cs New interface for platform-specific file picker

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +439 to +440
// Wait a bit to ensure locks are released (SQLite can be sticky)
await Task.Delay(100, ct);
Copy link

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using a fixed 100ms delay is a fragile solution for ensuring SQLite locks are released. This can lead to race conditions on slower systems or fail unnecessarily on faster ones. Consider implementing a proper retry mechanism with exponential backoff, or better yet, use SQLite's built-in checkpoint functionality to ensure the WAL is properly flushed before copying the database file.

Copilot uses AI. Check for mistakes.
Comment on lines +424 to +425
var extractedDbPath = Path.Combine(tempExtractDir, "booklogger.db");
if (!File.Exists(extractedDbPath))
Copy link

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Direct use of Path.Combine and File methods instead of the injected IFileSystem abstraction breaks the established pattern. This code should use _fileSystem.CombinePath and _fileSystem.FileExists for consistency and testability.

Copilot uses AI. Check for mistakes.
await Task.Delay(100, ct);

_logger?.LogInformation("Backup restored successfully");
File.Copy(extractedDbPath, currentDbPath, true);
Copy link

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Direct use of File.Copy instead of _fileSystem.CopyFile breaks the abstraction pattern and makes the code less testable.

Suggested change
File.Copy(extractedDbPath, currentDbPath, true);
_fileSystem.CopyFile(extractedDbPath, currentDbPath, overwrite: true);

Copilot uses AI. Check for mistakes.
Comment on lines +477 to +479
if (Directory.Exists(tempExtractDir))
{
Directory.Delete(tempExtractDir, true);
Copy link

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Direct use of Directory methods in cleanup logic instead of the IFileSystem abstraction creates inconsistency and reduces testability.

Suggested change
if (Directory.Exists(tempExtractDir))
{
Directory.Delete(tempExtractDir, true);
if (_fileSystem.DirectoryExists(tempExtractDir))
{
_fileSystem.DeleteDirectory(tempExtractDir, true);

Copilot uses AI. Check for mistakes.
{
if (Directory.Exists(tempExtractDir))
{
Directory.Delete(tempExtractDir, true);
Copy link

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cleanup code in the finally block swallows all exceptions silently. If Directory.Delete fails, this could leave orphaned temporary directories on disk over time. Consider logging the exception or at least checking if the directory deletion succeeded to avoid accumulating temporary files.

Suggested change
Directory.Delete(tempExtractDir, true);
try
{
Directory.Delete(tempExtractDir, true);
}
catch (Exception cleanupEx)
{
_logger?.LogWarning(cleanupEx, "Failed to delete temporary restore directory: {Path}", tempExtractDir);
}

Copilot uses AI. Check for mistakes.
var dbPath = context.Database.GetConnectionString()?.Replace("Data Source=", "");
// Temp directory for extraction
var tempExtractDir = _fileSystem.CombinePath(_backupDirectory, $"restore_{Guid.NewGuid()}");
Directory.CreateDirectory(tempExtractDir);
Copy link

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Direct use of Directory methods (Directory.CreateDirectory, Directory.Exists, Directory.Delete) instead of the injected IFileSystem abstraction creates inconsistency. The code should use _fileSystem.CreateDirectory and _fileSystem.DirectoryExists for better testability and to maintain the abstraction pattern.

Suggested change
Directory.CreateDirectory(tempExtractDir);
_fileSystem.CreateDirectory(tempExtractDir);

Copilot uses AI. Check for mistakes.
Comment on lines +383 to +401
private void CopyDirectory(string sourceDir, string destinationDir)
{
var dir = new DirectoryInfo(sourceDir);
if (!dir.Exists) return; // Should have been checked

Directory.CreateDirectory(destinationDir);

foreach (FileInfo file in dir.GetFiles())
{
string targetFilePath = Path.Combine(destinationDir, file.Name);
file.CopyTo(targetFilePath, true);
}

foreach (DirectoryInfo subDir in dir.GetDirectories())
{
string newDestinationDir = Path.Combine(destinationDir, subDir.Name);
CopyDirectory(subDir.FullName, newDestinationDir);
}
}
Copy link

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CopyDirectory method uses direct System.IO methods (Directory.CreateDirectory, DirectoryInfo) instead of the IFileSystem abstraction. This creates inconsistency with the rest of the class and makes the code harder to test. Consider adding a recursive copy method to IFileSystem or using the existing abstraction methods.

Copilot uses AI. Check for mistakes.
Comment on lines +326 to +341
// Manually copy files since IFileSystem might not have recursive copy
// Assuming flat structure for covers as per ImageService
// Validating ImageService implementation: it puts files directly in 'covers' dir.
// We need to check if IFileSystem exposes GetFiles, if not we might need to rely on System.IO or concrete implementation.
// Checking ImportExportService dependencies: it uses IFileSystem.

// NOTE: Since IFileSystem abstraction might be limited, and we are in Infrastructure which has access to System.IO,
// we can use standard DirectoryInfo if IFileSystem is too restrictive, BUT better to stick to injection if possible.
// However, standard System.IO.Compression.ZipFile.CreateFromDirectory works on file system paths.

// Let's use the actual file system for the directory copy to be safe and simple
// since ZipFile.CreateFromDirectory is a static system method anyway.

// We can't rely solely on _fileSystem interface for the ZipFile helper which requires string paths.
// So we will assume standard IO access is permitted for these paths.

Copy link

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is unnecessarily verbose and contains implementation notes that should be cleaned up before merging. The comment spans lines 313-343 and includes phrases like "NOTE: Since IFileSystem abstraction might be limited" and internal deliberation about implementation choices. These should be condensed to a brief explanation of why the approach was chosen, or removed entirely if the code is self-explanatory.

Suggested change
// Manually copy files since IFileSystem might not have recursive copy
// Assuming flat structure for covers as per ImageService
// Validating ImageService implementation: it puts files directly in 'covers' dir.
// We need to check if IFileSystem exposes GetFiles, if not we might need to rely on System.IO or concrete implementation.
// Checking ImportExportService dependencies: it uses IFileSystem.
// NOTE: Since IFileSystem abstraction might be limited, and we are in Infrastructure which has access to System.IO,
// we can use standard DirectoryInfo if IFileSystem is too restrictive, BUT better to stick to injection if possible.
// However, standard System.IO.Compression.ZipFile.CreateFromDirectory works on file system paths.
// Let's use the actual file system for the directory copy to be safe and simple
// since ZipFile.CreateFromDirectory is a static system method anyway.
// We can't rely solely on _fileSystem interface for the ZipFile helper which requires string paths.
// So we will assume standard IO access is permitted for these paths.
// Explicitly copy cover files into the backup directory.

Copilot uses AI. Check for mistakes.
Comment on lines +420 to 422
// 1. Extract ZIP
ZipFile.ExtractToDirectory(backupPath, tempExtractDir);

Copy link

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code extracts a ZIP file without validating its contents or checking for path traversal attacks (zip slip vulnerability). A malicious backup file could contain entries with paths like "../../../etc/passwd" that would extract files outside the intended directory. Consider validating that all extracted file paths are within the target directory before extraction, or use ZipArchive with manual extraction and path validation.

Suggested change
// 1. Extract ZIP
ZipFile.ExtractToDirectory(backupPath, tempExtractDir);
// 1. Extract ZIP with path validation to prevent zip slip
var extractionRoot = Path.GetFullPath(tempExtractDir);
if (!extractionRoot.EndsWith(Path.DirectorySeparatorChar))
{
extractionRoot += Path.DirectorySeparatorChar;
}
using (var archive = ZipFile.OpenRead(backupPath))
{
foreach (var entry in archive.Entries)
{
if (string.IsNullOrEmpty(entry.FullName))
{
continue;
}
var destinationPath = Path.GetFullPath(
Path.Combine(extractionRoot, entry.FullName));
// Ensure the destination path is within the intended extraction directory
if (!destinationPath.StartsWith(extractionRoot, StringComparison.OrdinalIgnoreCase))
{
throw new InvalidOperationException(
$"Invalid entry path in backup archive: {entry.FullName}");
}
// Handle directory entries
if (entry.FullName.EndsWith("/", StringComparison.Ordinal) ||
string.IsNullOrEmpty(Path.GetFileName(entry.FullName)))
{
Directory.CreateDirectory(destinationPath);
continue;
}
var destinationDir = Path.GetDirectoryName(destinationPath);
if (!string.IsNullOrEmpty(destinationDir))
{
Directory.CreateDirectory(destinationDir);
}
entry.ExtractToFile(destinationPath, overwrite: true);
}
}

Copilot uses AI. Check for mistakes.
Comment on lines +449 to +464

// Clean target covers dir to remove orphaned images?
// User requested "Restore", usually enabling a clean slate or overwrite.
// Let's clear target directory to ensure exact match with backup.
if (Directory.Exists(targetCoversDir))
{
var dirInfo = new DirectoryInfo(targetCoversDir);
foreach (var file in dirInfo.GetFiles()) file.Delete();
foreach (var dir in dirInfo.GetDirectories()) dir.Delete(true);
}
else
{
Directory.CreateDirectory(targetCoversDir);
}

CopyDirectory(extractedCoversDir, targetCoversDir);
Copy link

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The restore operation deletes all existing cover images before restoring the backup. However, if the restoration fails after clearing the covers directory (for example, during the CopyDirectory call), the user will lose their existing cover images without successfully restoring the backup. Consider keeping the old covers directory as a temporary backup or implementing atomic replacement to avoid data loss in failure scenarios.

Suggested change
// Clean target covers dir to remove orphaned images?
// User requested "Restore", usually enabling a clean slate or overwrite.
// Let's clear target directory to ensure exact match with backup.
if (Directory.Exists(targetCoversDir))
{
var dirInfo = new DirectoryInfo(targetCoversDir);
foreach (var file in dirInfo.GetFiles()) file.Delete();
foreach (var dir in dirInfo.GetDirectories()) dir.Delete(true);
}
else
{
Directory.CreateDirectory(targetCoversDir);
}
CopyDirectory(extractedCoversDir, targetCoversDir);
string? backupCoversDir = null;
try
{
// To avoid data loss if restore fails, move the existing covers directory
// to a temporary backup, then restore from backup into a fresh directory.
if (Directory.Exists(targetCoversDir))
{
backupCoversDir = targetCoversDir + "_backup_" + Guid.NewGuid().ToString("N");
Directory.Move(targetCoversDir, backupCoversDir);
}
// Ensure target directory exists before copying
if (!Directory.Exists(targetCoversDir))
{
Directory.CreateDirectory(targetCoversDir);
}
CopyDirectory(extractedCoversDir, targetCoversDir);
// Copy succeeded; old covers are no longer needed
if (backupCoversDir != null && Directory.Exists(backupCoversDir))
{
Directory.Delete(backupCoversDir, true);
}
}
catch
{
// Attempt to restore original covers if something went wrong
try
{
if (backupCoversDir != null && Directory.Exists(backupCoversDir))
{
// If the target directory exists (partially restored), remove it
if (Directory.Exists(targetCoversDir))
{
Directory.Delete(targetCoversDir, true);
}
Directory.Move(backupCoversDir, targetCoversDir);
}
}
catch
{
// Swallow secondary failures during rollback; original exception is rethrown.
}
throw;
}

Copilot uses AI. Check for mistakes.
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