Fix backup state logic, ring colors, Already Safe invariant, and dedup
BackupJob: fix finish() — failedCount > 0 now correctly sets .failed instead of .completed in both branches. BackupEngine: replace filename-only fileExists check with a 3-layer dedup strategy. For each asset: (1) check ManifestIndex by localIdentifier O(1); (2) filename fallback for bootstrap manifests; (3) NAS fileExists for files not yet in manifest. Never overwrite an existing NAS file. Manifest index is built once after connect and held in a local let for the loop — Data+BackupManifest are released immediately after. BackupView — ring color: active (running/preparing/paused) → orange always failed → destructive needBackup == 0 && phoneTotal > 0 → green default → neutral gray Ring is no longer permanently green after .completed. BackupView — CTA button: Default color is always black (AppTheme.ink). On .completed: briefly flashes green for 2.5 s then smoothly reverts to black. ctaFlashGreen state drives the color. BackupView — Already Safe invariant: alreadySafeDisplayCount = min(base + uploadedFiles, phoneTotal) during active backup; min(base, phoneTotal) otherwise. Adding skippedFiles was removed — they are already counted in snapshot.alreadySafe from the last reconcile. notBackedUpDisplayCount is derived from alreadySafeDisplayCount to keep the invariant alreadySafe + needBackup == phoneTotal. Co-Authored-By: Kutesir <kutesir@provoc.ug> Co-Authored-By: Sentry <sentry@provoc.ug>
This commit is contained in:
@@ -72,7 +72,9 @@ final class BackupJob: ObservableObject {
|
||||
|
||||
func finish(result: BackupResult) {
|
||||
endDate = Date()
|
||||
status = result.failedCount == 0 ? .completed : .completed
|
||||
status = result.failedCount == 0
|
||||
? .completed
|
||||
: .failed("Completed with \(result.failedCount) error\(result.failedCount == 1 ? "" : "s")")
|
||||
}
|
||||
|
||||
func fail(error: BackupError) {
|
||||
|
||||
@@ -13,6 +13,7 @@ struct BackupView: View {
|
||||
@State private var showDestinationPicker = false
|
||||
@State private var pickerPath: String = "/"
|
||||
@State private var ringPage = 0
|
||||
@State private var ctaFlashGreen = false
|
||||
|
||||
private let ringPageCount = 4
|
||||
|
||||
@@ -122,11 +123,14 @@ struct BackupView: View {
|
||||
statusService.refresh(force: true)
|
||||
}
|
||||
.onChange(of: engine.job.status) { status in
|
||||
// After backup completes, the engine already calls refreshAfterBackup
|
||||
// but also do a delayed full reconcile to catch any stragglers
|
||||
if status == .completed {
|
||||
// Flash green then revert to black
|
||||
withAnimation(.spring(response: 0.3, dampingFraction: 0.8)) { ctaFlashGreen = true }
|
||||
Task {
|
||||
try? await Task.sleep(nanoseconds: 2_000_000_000)
|
||||
try? await Task.sleep(nanoseconds: 2_500_000_000)
|
||||
withAnimation(.spring(response: 0.5, dampingFraction: 0.8)) { ctaFlashGreen = false }
|
||||
// Full reconcile after flash to get accurate counts
|
||||
try? await Task.sleep(nanoseconds: 500_000_000)
|
||||
statusService.refresh(force: true)
|
||||
}
|
||||
}
|
||||
@@ -322,40 +326,42 @@ struct BackupView: View {
|
||||
}
|
||||
|
||||
private var ringColor: Color {
|
||||
switch engine.job.status {
|
||||
case .completed: return AppTheme.positive
|
||||
case .failed: return AppTheme.destructive
|
||||
case .running, .paused,
|
||||
.preparing: return Color(red: 1.0, green: 0.58, blue: 0.0)
|
||||
default: return AppTheme.interactive.opacity(0.28)
|
||||
// Active: always orange
|
||||
if engine.job.status.isActive { return Color(red: 1.0, green: 0.58, blue: 0.0) }
|
||||
// Failed: red
|
||||
if case .failed = engine.job.status { return AppTheme.destructive }
|
||||
// Green only when ALL phone photos are accounted for (needBackup == 0)
|
||||
if statusService.snapshot.phoneTotal > 0 && statusService.snapshot.needBackup == 0 {
|
||||
return AppTheme.positive
|
||||
}
|
||||
// Default: neutral
|
||||
return AppTheme.inkQuaternary.opacity(0.55)
|
||||
}
|
||||
|
||||
private var percentText: String { "\(Int(engine.job.progress * 100))%" }
|
||||
|
||||
// MARK: — Compact stats strip
|
||||
|
||||
// Live "need backup" count: optimistic during active backup, service-derived otherwise.
|
||||
// Invariant: needBackup + alreadySafe == phoneTotal always.
|
||||
private var notBackedUpDisplayCount: Int {
|
||||
switch engine.job.status {
|
||||
case .running, .paused, .preparing:
|
||||
return max(0, engine.job.totalFiles - engine.job.uploadedFiles
|
||||
- engine.job.skippedFiles - engine.job.failedFiles)
|
||||
case .completed: return 0
|
||||
default: return statusService.snapshot.needBackup
|
||||
// Invariant: alreadySafe + needBackup == phoneTotal always.
|
||||
// During active backup show optimistic count (only newly uploaded, not skipped —
|
||||
// skipped files are already counted inside snapshot.alreadySafe from the last reconcile).
|
||||
private var alreadySafeDisplayCount: Int {
|
||||
let total = statusService.snapshot.phoneTotal
|
||||
guard total > 0 else { return 0 }
|
||||
let base = statusService.snapshot.alreadySafe
|
||||
if engine.job.status.isActive {
|
||||
return min(base + engine.job.uploadedFiles, total)
|
||||
}
|
||||
return min(base, total) // enforce invariant at all times
|
||||
}
|
||||
|
||||
private var alreadySafeDisplayCount: Int {
|
||||
switch engine.job.status {
|
||||
case .running, .paused, .preparing, .completed:
|
||||
// Optimistic: base from service + newly confirmed uploads this session
|
||||
return statusService.snapshot.alreadySafe
|
||||
+ engine.job.uploadedFiles + engine.job.skippedFiles
|
||||
default:
|
||||
return statusService.snapshot.alreadySafe
|
||||
private var notBackedUpDisplayCount: Int {
|
||||
let total = statusService.snapshot.phoneTotal
|
||||
guard total > 0 else { return 0 }
|
||||
if engine.job.status.isActive {
|
||||
return max(0, total - alreadySafeDisplayCount)
|
||||
}
|
||||
return statusService.snapshot.needBackup
|
||||
}
|
||||
|
||||
private var statsStrip: some View {
|
||||
@@ -605,10 +611,10 @@ struct BackupView: View {
|
||||
switch engine.job.status {
|
||||
case .idle, .completed, .failed, .cancelled:
|
||||
Button(action: startBackup) {
|
||||
Text(engine.job.status == .completed ? "Back up again" : "Start backup")
|
||||
Text(engine.job.status == .idle ? "Start backup" : "Back up again")
|
||||
}
|
||||
.buttonStyle(PrimaryButtonStyle(
|
||||
color: engine.job.status == .completed ? AppTheme.positive : AppTheme.ink
|
||||
color: ctaFlashGreen ? AppTheme.positive : AppTheme.ink
|
||||
))
|
||||
.accessibilityHint("Starts backing up your photos to the NAS")
|
||||
|
||||
|
||||
@@ -73,7 +73,19 @@ final class BackupEngine: ObservableObject {
|
||||
activeTransfer = nil
|
||||
}
|
||||
|
||||
// 3. Start job tracking
|
||||
// 3. Load manifest → build index (manifest Data+struct released after this block)
|
||||
let manifestPath = "\(connection.remotePath)/\(BackupManifest.remoteFilename)"
|
||||
let index: ManifestIndex
|
||||
if let data = try? await transfer.downloadData(at: manifestPath),
|
||||
let manifest = try? JSONDecoder().decode(BackupManifest.self, from: data) {
|
||||
index = ManifestIndex(manifest: manifest)
|
||||
logger.info("Manifest loaded — \(index.totalCount) entries, filename-only=\(index.isFilenameOnly)")
|
||||
} else {
|
||||
index = ManifestIndex(nasListing: [])
|
||||
logger.info("No manifest — will check NAS file existence per asset")
|
||||
}
|
||||
|
||||
// 4. Start job tracking
|
||||
job.start(totalFiles: assets.count, totalBytes: 0)
|
||||
|
||||
var uploaded = 0
|
||||
@@ -83,21 +95,40 @@ final class BackupEngine: ObservableObject {
|
||||
var speedTracker = SpeedTracker()
|
||||
var manifestEntries: [ManifestEntry] = []
|
||||
|
||||
// 4. Transfer loop
|
||||
// 5. Transfer loop
|
||||
for asset in assets {
|
||||
if isCancelled { break }
|
||||
while case .paused = job.status {
|
||||
try await Task.sleep(nanoseconds: 500_000_000)
|
||||
}
|
||||
|
||||
let remotePath = "\(connection.remotePath)/\(asset.filename)"
|
||||
|
||||
if filter.newFilesOnly, (try? await transfer.fileExists(at: remotePath)) == true {
|
||||
// ── Primary dedup: manifest localIdentifier (O(1), no network call) ──
|
||||
if index.matches(localIdentifier: asset.localIdentifier) {
|
||||
job.fileCompleted(skipped: true)
|
||||
skipped += 1
|
||||
logger.debug("Skipped (manifest id): \(asset.filename, privacy: .public)")
|
||||
continue
|
||||
}
|
||||
|
||||
// ── Filename fallback: bootstrap manifest has no localIdentifiers ──
|
||||
if index.isFilenameOnly && index.matches(filename: asset.filename) {
|
||||
job.fileCompleted(skipped: true)
|
||||
skipped += 1
|
||||
logger.debug("Skipped (manifest filename): \(asset.filename, privacy: .public)")
|
||||
continue
|
||||
}
|
||||
|
||||
// ── NAS file existence check — never overwrite ──
|
||||
let baseRemotePath = "\(connection.remotePath)/\(asset.filename)"
|
||||
let fileAlreadyExists = (try? await transfer.fileExists(at: baseRemotePath)) == true
|
||||
if fileAlreadyExists {
|
||||
job.fileCompleted(skipped: true)
|
||||
skipped += 1
|
||||
logger.debug("Skipped (NAS exists, no-overwrite): \(asset.filename, privacy: .public)")
|
||||
continue
|
||||
}
|
||||
|
||||
// ── Upload ──
|
||||
do {
|
||||
let localURL = try await photoService.exportAsset(asset)
|
||||
defer { try? FileManager.default.removeItem(at: localURL) }
|
||||
@@ -105,7 +136,7 @@ final class BackupEngine: ObservableObject {
|
||||
let fileSize = (try? localURL.resourceValues(forKeys: [.fileSizeKey]).fileSize)
|
||||
.flatMap { Int64($0) } ?? 0
|
||||
|
||||
try await transfer.upload(localURL: localURL, remotePath: remotePath) { sent, total in
|
||||
try await transfer.upload(localURL: localURL, remotePath: baseRemotePath) { sent, total in
|
||||
let speed = speedTracker.update(bytesSent: sent)
|
||||
Task { @MainActor [weak self] in
|
||||
self?.job.updateProgress(
|
||||
@@ -126,7 +157,7 @@ final class BackupEngine: ObservableObject {
|
||||
filename: asset.filename,
|
||||
creationDate: asset.creationDate,
|
||||
fileSize: fileSize,
|
||||
remotePath: remotePath,
|
||||
remotePath: baseRemotePath,
|
||||
uploadedAt: Date()
|
||||
))
|
||||
} catch {
|
||||
|
||||
Reference in New Issue
Block a user