nngo/JQC_iOS_App
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Aug 19 - Fixed photo-loss issue

Browse Source
This commit is contained in:
Nguyen Ngo
2026-08-19 10:31:20 -04:00
parent 7cbc514c39
commit e31cd7e1ff
13 changed files with 856 additions and 149 deletions
Download Patch File Download Diff File Expand all files Collapse all files
JanitorialQC/Sync/SyncManager.swift
+329 -65
View File
@@ -299,35 +299,43 @@ class SyncManager: ObservableObject {
// ── Outbox: Photos ────────────────────────────────────────────────────
/// Upload attempts before a PendingPhoto is given up on.
///
/// Until this is reached the row stays "pending", which does two things:
/// the next sync retries it, AND processInspectionQueue keeps waiting
/// rather than submitting the inspection with a blank photo field.
static let maxPhotoUploadAttempts = 5
private func processPhotoQueue(context: ModelContext) async {
// Fetch all then filter in Swift — #Predicate cannot reference
// string literals against PendingPhoto.uploadStatus reliably
// when the predicate type is inferred across model boundaries.
guard let allPhotos = try? context.fetch(FetchDescriptor<PendingPhoto>()) else { return }
var pending = allPhotos
let pending = allPhotos
.filter { $0.uploadStatus == "pending" }
.sorted { $0.createdAt < $1.createdAt }
// Defense-in-depth: if two PendingPhoto rows somehow reference the
// exact same local file (e.g. a future call site re-submitting the
// same photo array), only upload it once. The primary fix for photo
// duplication is the re-entrancy guard in triggerSync(), but this
// keeps processPhotoQueue itself safe even if it's ever invoked
// outside that guard.
var seenPaths = Set<String>()
// Two rows can legitimately point at the same file — the same image
// attached to two form fields, or a call site re-submitting a photo
// array. Upload it once, then give EVERY row that references it the
// same server path.
//
// The previous version marked the duplicates "uploaded" up front and
// never set their serverPath, so the second field was submitted blank.
// Settling them from the upload result instead keeps every field
// pointing at real evidence. (Uploading once still matters: two uploads
// of one file produce two different server filenames and duplicate the
// photo in the issue's evidence and its PDF.)
var firstByPath: [String: PendingPhoto] = [:]
var toUpload: [PendingPhoto] = []
var duplicates: [PendingPhoto] = []
pending = pending.filter { photo in
if seenPaths.contains(photo.localFilePath) {
for photo in pending {
if firstByPath[photo.localFilePath] == nil {
firstByPath[photo.localFilePath] = photo
toUpload.append(photo)
} else {
duplicates.append(photo)
return false
}
seenPaths.insert(photo.localFilePath)
return true
}
for dup in duplicates {
// Mark the duplicate row as uploaded without re-uploading — the
// first row for this file will populate serverPath/photoServerPaths.
dup.uploadStatus = "uploaded"
}
// Pre-fetch parent records ONCE before the loop.
@@ -340,7 +348,9 @@ class SyncManager: ObservableObject {
let allInspections = (try? context.fetch(FetchDescriptor<LocalInspection>())) ?? []
let allIssues = (try? context.fetch(FetchDescriptor<LocalIssue>())) ?? []
for photo in pending {
var uploadedPaths: [String: String] = [:] // localFilePath -> serverPath
for photo in toUpload {
do {
// Capture metadata was recorded at the shutter, not now — the
// sync may run hours after an offline capture, and the server
@@ -352,35 +362,122 @@ class SyncManager: ObservableObject {
latitude: photo.captureLatitude,
longitude: photo.captureLongitude
)
photo.serverPath = serverPath
photo.uploadStatus = "uploaded"
// Update parent inspection form field value.
// Pre-fetched before the loop — not repeated per photo.
if photo.entityType == "inspection", let fieldId = photo.fieldId {
let entityId = photo.entityLocalId
allInspections.first(where: { $0.localId == entityId })?
.setValue(serverPath, forFieldId: fieldId)
}
// Update parent issue photo paths array.
// Pre-fetched before the loop — not repeated per photo.
if photo.entityType == "issue" {
let entityId = photo.entityLocalId
if let issue = allIssues.first(where: { $0.localId == entityId }) {
var paths = issue.photoServerPaths
if !paths.contains(serverPath) { paths.append(serverPath) }
issue.photoServerPaths = paths
}
}
photo.serverPath = serverPath
photo.uploadStatus = "uploaded"
photo.uploadRetryCount = 0
uploadedPaths[photo.localFilePath] = serverPath
attachServerPath(serverPath, for: photo,
inspections: allInspections, issues: allIssues)
await pushLateIssuePhotoIfNeeded(serverPath, for: photo,
issues: allIssues, context: context)
try? context.save()
} catch {
photo.uploadStatus = "failed"
// Treat the error as TRANSIENT by default. Leaving the row
// "pending" means the next sync retries it and — critically —
// processInspectionQueue keeps waiting instead of submitting
// the inspection with this field blanked to "" by
// APIClient.submitInspection.
//
// The old code marked "failed" on the very first error, and
// nothing anywhere ever moved a row back off "failed". One
// dropped connection therefore cost the inspection its evidence
// photo permanently, silently, with the inspection still
// reported as successfully synced.
photo.uploadRetryCount += 1
if photo.uploadRetryCount >= Self.maxPhotoUploadAttempts {
photo.uploadStatus = "failed"
syncError = "A photo failed to upload after "
+ "\(Self.maxPhotoUploadAttempts) attempts. "
+ "Open Pending Sync → Retry Failed Items to try again."
}
try? context.save()
}
}
// Settle rows that shared a file with one just uploaded. A row whose
// twin failed is deliberately left "pending" so both retry together.
for dup in duplicates {
guard let serverPath = uploadedPaths[dup.localFilePath] else { continue }
dup.serverPath = serverPath
dup.uploadStatus = "uploaded"
dup.uploadRetryCount = 0
attachServerPath(serverPath, for: dup,
inspections: allInspections, issues: allIssues)
await pushLateIssuePhotoIfNeeded(serverPath, for: dup,
issues: allIssues, context: context)
}
try? context.save()
}
/// Carry a photo across to an issue that has ALREADY been created on the
/// server, which the create request therefore could not have included.
///
/// Only reachable via recovery: the photo exhausted
/// `maxPhotoUploadAttempts`, the issue was submitted without it (rule 83
/// deliberately lets that happen rather than blocking forever), and the
/// inspector later hit Pending Sync → Retry Failed Items and the upload
/// succeeded. In the normal path processPhotoQueue runs BEFORE
/// processIssueQueue, so `issue.serverId` is still nil here and this does
/// nothing — which is the discriminator, and why this is not a rule 85
/// violation.
///
/// Must happen now, in this same pass: `pullAssignedIssues` later overwrites
/// `photoServerPaths` with the server's copy, so a photo left only in local
/// state would be erased before anything else could notice it.
///
/// Best-effort. A failure here leaves the photo attached locally but not
/// server-side until the next pull overwrites it — the residual limitation
/// noted in rule 83. Reaching this at all takes five consecutive upload
/// failures, and the PATCH merges idempotently, so a repeat is harmless.
private func pushLateIssuePhotoIfNeeded(
_ serverPath: String,
for photo: PendingPhoto,
issues: [LocalIssue],
context: ModelContext
) async {
guard photo.entityType == "issue" else { return }
let entityId = photo.entityLocalId
guard let issue = issues.first(where: { $0.localId == entityId }),
let issueServerId = issue.serverId
else { return }
do {
try await APIClient.shared.updateIssuePhotos(
issueId: issueServerId, resultPhotos: [serverPath]
)
issue.syncErrorMessage = nil
} catch {
issue.syncErrorMessage =
"A recovered photo could not be attached: \(error.localizedDescription)"
}
try? context.save()
}
/// Write a freshly uploaded server path onto whichever record owns the photo.
/// Both collections are pre-fetched by the caller — see processPhotoQueue.
private func attachServerPath(
_ serverPath: String,
for photo: PendingPhoto,
inspections: [LocalInspection],
issues: [LocalIssue]
) {
let entityId = photo.entityLocalId
// Inspection form image field.
if photo.entityType == "inspection", let fieldId = photo.fieldId {
inspections.first(where: { $0.localId == entityId })?
.setValue(serverPath, forFieldId: fieldId)
}
// Issue evidence photo array.
if photo.entityType == "issue",
let issue = issues.first(where: { $0.localId == entityId }) {
var paths = issue.photoServerPaths
if !paths.contains(serverPath) { paths.append(serverPath) }
issue.photoServerPaths = paths
}
}
// ── Outbox: Inspections ───────────────────────────────────────────────
@@ -392,6 +489,13 @@ class SyncManager: ObservableObject {
.sorted { $0.createdAt < $1.createdAt }
for inspection in pending {
// "failed" is only reachable after maxPhotoUploadAttempts, so this
// now means "uploaded, or genuinely unrecoverable" rather than
// "uploaded, or hit one network error". A still-retrying photo
// keeps its row "pending" and holds the inspection back — which is
// the point: submitting first is what blanked the field for good,
// since APIClient.submitInspection rewrites a surviving local://
// value to "" and the inspection is then marked synced forever.
let photosReady = inspection.pendingPhotos.allSatisfy {
$0.uploadStatus == "uploaded" || $0.uploadStatus == "failed"
}
@@ -443,11 +547,30 @@ class SyncManager: ObservableObject {
// processInspectionQueue wrote the serverId back, leaving it nil even
// when the parent inspection already synced successfully this same pass.
let allInspections = (try? context.fetch(FetchDescriptor<LocalInspection>())) ?? []
// Issue photos are inserted standalone (FlagIssueView / StandaloneIssueView
// do not append them to any relationship), so they have to be matched by
// entityType + entityLocalId rather than navigated to.
let allPhotos = (try? context.fetch(FetchDescriptor<PendingPhoto>())) ?? []
for issue in pending {
let parentLocalId = issue.inspectionLocalId
let parent = allInspections.first(where: { $0.localId == parentLocalId })
// ── Photo readiness ───────────────────────────────────────────
// Wait for this issue's photos exactly as processInspectionQueue
// waits for an inspection's. There was no guard here at all: the
// issue submitted with photo_path = photoServerPaths.first (nil
// while uploads were still in flight or being retried), and the
// unconditional clear below then dropped the only local reference
// to the files.
let issuePhotos = allPhotos.filter {
$0.entityType == "issue" && $0.entityLocalId == issue.localId
}
let photosSettled = issuePhotos.allSatisfy {
$0.uploadStatus == "uploaded" || $0.uploadStatus == "failed"
}
guard photosSettled else { continue }
// ── Parent inspection status guards ───────────────────────────
if let parent {
switch parent.syncStatus {
@@ -486,23 +609,25 @@ class SyncManager: ObservableObject {
let issueId = try await APIClient.shared.submitIssue(issue, inspectionServerId: inspectionServerId)
issue.serverId = issueId
issue.syncStatus = "synced"
// Photos are now represented by photoServerPaths on the server.
// Clear the local file paths so IssueDetailView doesn't render
// a duplicate "local photos" section alongside the server section.
issue.photoLocalPaths = []
try? context.save()
// If there are additional photos beyond the first (which was sent
// as photo_path on create), PATCH them to result_photos now.
// The server create endpoint only stores photo_path; result_photos
// must be set via a separate PATCH call.
let extras = Array(issue.photoServerPaths.dropFirst())
if !extras.isEmpty {
try? await APIClient.shared.updateIssuePhotos(
issueId: issueId, resultPhotos: extras
)
// Clear the local file paths ONLY when every photo actually
// reached the server. Clearing unconditionally is what made a
// partial upload unrecoverable: the files stayed on disk but
// nothing referenced them any more, so neither the UI nor
// PhotoDiagnosticView could find them again. Keeping them costs
// a duplicate photo section in IssueDetailView at worst; losing
// them costs the evidence itself.
let allUploaded = issuePhotos.allSatisfy { $0.uploadStatus == "uploaded" }
if allUploaded {
issue.photoLocalPaths = []
}
// No follow-up call: submitIssue() sends every photo in the
// create request (photo_path + result_photos), so attachment is
// atomic with creation — there is no window in which the issue
// is `synced` but its photos are not attached. See rule 85.
try? context.save()
} catch {
issue.syncRetryCount += 1
issue.syncErrorMessage = error.localizedDescription
@@ -657,6 +782,14 @@ class SyncManager: ObservableObject {
private static let lastCleanupKey = "jqc.photoCleanup.lastRunAt"
private static let cleanupInterval: TimeInterval = 3600 // 1 hour
/// A file must be at least this old before cleanup will consider it.
///
/// Resolution photos staged in IssueDetailView live purely in `@State`
/// until they upload — no database row references them — so for that flow
/// age is the only available evidence that a file is not in active use.
/// The floor also covers the window between writing a JPEG and saving the
/// record that points at it.
private static let cleanupMinFileAge: TimeInterval = 7 * 24 * 3600 // 7 days
private func cleanupOrphanedPhotos(context: ModelContext) {
let last = UserDefaults.standard.object(forKey: Self.lastCleanupKey) as? Date
@@ -673,9 +806,17 @@ class SyncManager: ObservableObject {
referencedPaths.insert(p.localFilePath)
}
}
// LocalInspection — draft photos (formData values starting with "local://")
// LocalInspection — EVERY field still holding the local:// sentinel,
// whatever the inspection's status.
//
// Deliberately not limited to drafts. A sentinel surviving on a
// submitted inspection means that photo never reached the server, so
// the file on disk is the only copy in existence — it is exactly the
// material PhotoDiagnosticView reports as "recoverable". Restricting
// this to drafts would have made correcting the directory below destroy
// the evidence this whole fix exists to preserve.
if let inspections = try? context.fetch(FetchDescriptor<LocalInspection>()) {
for insp in inspections where insp.status == "draft" {
for insp in inspections {
for val in insp.formData.values {
if let s = val as? String, s.hasPrefix("local://") {
referencedPaths.insert(String(s.dropFirst("local://".count)))
@@ -683,9 +824,12 @@ class SyncManager: ObservableObject {
}
}
}
// LocalIssue — unsync'd issue photos
// LocalIssue — every retained local path, synced or not, for the same
// reason. processIssueQueue now clears these only once all photos have
// actually uploaded, so a path still present means evidence the server
// does not have.
if let issues = try? context.fetch(FetchDescriptor<LocalIssue>()) {
for issue in issues where issue.syncStatus != "synced" {
for issue in issues {
for path in issue.photoLocalPaths { referencedPaths.insert(path) }
}
}
@@ -695,20 +839,39 @@ class SyncManager: ObservableObject {
// the main thread when JQCPhotos/ contains hundreds of files. Dispatching
// here is safe because `referencedPaths` is a value type (Set<String>)
// captured by copy — no shared mutable state crosses the boundary.
let minAge = Self.cleanupMinFileAge
Task.detached(priority: .utility) {
let fm = FileManager.default
guard let docsDir = fm.urls(for: .documentDirectory, in: .userDomainMask).first else { return }
let photosDir = docsDir.appendingPathComponent("JQCPhotos")
// "JQC/Photos" — the directory every writer actually uses
// (FlagIssueView, StandaloneIssueView, CompactImageFieldView,
// ImageFieldView). This previously read "JQCPhotos", which no code
// path has ever written to: contentsOfDirectory failed, the guard
// returned, and this function silently deleted nothing for its
// entire life while photos accumulated indefinitely.
//
// "JQC/ResultPhotos" is deliberately NOT swept. Those files are
// staged in IssueDetailView's @State with no database row, so
// nothing here can prove one is unused; they are cleaned up by
// uploadAndAttachResultPhotos() and removeResultPhoto() instead.
let photosDir = docsDir.appendingPathComponent("JQC/Photos", isDirectory: true)
guard let diskFiles = try? fm.contentsOfDirectory(
at: photosDir, includingPropertiesForKeys: nil
at: photosDir, includingPropertiesForKeys: [.contentModificationDateKey]
) else { return }
let cutoff = Date().addingTimeInterval(-minAge)
var deletedCount = 0
for fileURL in diskFiles {
if !referencedPaths.contains(fileURL.path) {
try? fm.removeItem(at: fileURL)
deletedCount += 1
}
if referencedPaths.contains(fileURL.path) { continue }
// Age floor — never touch a file young enough to belong to a
// capture flow that has not yet written its record.
let modified = (try? fileURL.resourceValues(
forKeys: [.contentModificationDateKey]
))?.contentModificationDate
guard let modified, modified < cutoff else { continue }
try? fm.removeItem(at: fileURL)
deletedCount += 1
}
if deletedCount > 0 {
print("[JQC] Sync | cleanupOrphanedPhotos | removed \(deletedCount) file(s)")
@@ -1025,6 +1188,107 @@ class SyncManager: ObservableObject {
}
}
// ── Session-scope purge ───────────────────────────────────────────────
/// Delete local data belonging to a previous session.
///
/// Called when the `(server, user)` pair the database is scoped to changes
/// — a different inspector signing in on this iPad, or a server switch. See
/// `SessionScope` for why that combination is the boundary, and rule 88.
///
/// - Parameters:
/// - keepingUserId: the incoming user. Their own unsent inspections are
/// preserved when `sameServer` is true; pass nil to keep nothing.
/// - sameServer: false when the server changed, in which case NOTHING can
/// be kept — every `serverId`, `facilityServerId` and `templateServerId`
/// names a row in a different database.
///
/// Deliberately NOT called on a plain logout: the common case is the same
/// inspector signing back into the same server, and wiping the cache there
/// would leave them with an empty app until a full sync succeeds, breaking
/// offline use. That was the correct instinct in the original code — the
/// bug was only that nothing ever checked whether the next login was
/// actually the same person.
func purgeSessionScopedData(keepingUserId: Int?, sameServer: Bool) {
guard let context = modelContext else { return }
// ── Reference caches ──────────────────────────────────────────────
// Pure server-scoped copies with no local authorship. Cheap to re-pull,
// so there is never a reason to keep one across an identity change.
deleteAll(LocalFacility.self, from: context) // cascades areas
deleteAll(LocalArea.self, from: context)
deleteAll(LocalTemplate.self, from: context)
deleteAll(LocalScheduledInspection.self, from: context)
deleteAll(LocalFollowUpRequest.self, from: context)
// ── Issues ────────────────────────────────────────────────────────
// All of them, unconditionally. LocalIssue carries no author field, so
// an unsent one cannot be attributed — and submitting the previous
// inspector's issue under the new inspector's credentials would put a
// false name on a QC record. Server-pulled and already-synced rows are
// the previous user's assignments and stale serverIds respectively.
deleteAll(LocalIssue.self, from: context)
// ── Inspections ───────────────────────────────────────────────────
// These DO carry an author (`inspectorUserId`), so the incoming user's
// own unsent work can be handed back to them intact — but only when the
// server is unchanged, since otherwise its facility/template ids point
// into the wrong database.
let inspections = (try? context.fetch(FetchDescriptor<LocalInspection>())) ?? []
var keptInspectionIds = Set<String>()
for insp in inspections {
let ownedByIncomingUser = keepingUserId.map { $0 == insp.inspectorUserId } ?? false
if sameServer && ownedByIncomingUser {
keptInspectionIds.insert(insp.localId)
} else {
// Remove the JPEGs too — cleanupOrphanedPhotos would otherwise
// wait out its 7-day age floor holding another user's evidence.
for photo in insp.pendingPhotos {
try? FileManager.default.removeItem(atPath: photo.localFilePath)
}
context.delete(insp) // cascades pendingPhotos + localIssues
}
}
// ── Photos orphaned by the above ──────────────────────────────────
// Issue photos are inserted standalone rather than through a
// relationship, so no cascade reaches them — and every LocalIssue has
// just been deleted, which is why only kept inspections can retain one.
let photos = (try? context.fetch(FetchDescriptor<PendingPhoto>())) ?? []
for photo in photos {
let stillOwned = photo.entityType == "inspection"
&& keptInspectionIds.contains(photo.entityLocalId)
if !stillOwned {
try? FileManager.default.removeItem(atPath: photo.localFilePath)
context.delete(photo)
}
}
try? context.save()
// ── In-memory state from the old session ──────────────────────────
// The poll TASK is left running; only its data is dropped. Stopping it
// here would leave polling dead until the next connectivity change or
// foreground, because nothing on the login path restarts it.
dashboardStats = nil
lastNotificationFetch = nil
unreadNotificationCount = 0
recentNotifications = []
syncError = nil
lastSyncAt = nil
updatePendingCount(context: context)
print("[JQC] Sync | purgeSessionScopedData | kept \(keptInspectionIds.count) "
+ "inspection(s) for user \(keepingUserId.map(String.init) ?? "-")")
}
/// Fetch-all + delete. No #Predicate (CLAUDE.md rule 3), and `try?`
/// parenthesised before `??` (rule 25).
private func deleteAll<T: PersistentModel>(_ type: T.Type, from context: ModelContext) {
let rows = (try? context.fetch(FetchDescriptor<T>())) ?? []
for row in rows { context.delete(row) }
}
// ── Dashboard Stats ───────────────────────────────────────────────────
// Best-effort fetch — a network failure silently leaves dashboardStats nil
// so the UI falls back to a placeholder card. Never blocks the sync pipeline.