From fc571b3faafe73b634ffe0553e5b9cc5e1c8423c Mon Sep 17 00:00:00 2001 From: Nguyen Ngo Date: Tue, 23 Jun 2026 18:03:40 -0400 Subject: [PATCH] 06/23 Fix Medium//Low-impact issues --- JanitorialQC/ContentView.swift | 22 +++++ JanitorialQC/Models/LocalIssue.swift | 52 ++++++++++- JanitorialQC/Models/LocalTemplate.swift | 17 +++- JanitorialQC/Models/SyncQueueEntry.swift | 15 +++- JanitorialQC/Sync/SyncManager.swift | 106 ++++++++++++++++------- 5 files changed, 173 insertions(+), 39 deletions(-) diff --git a/JanitorialQC/ContentView.swift b/JanitorialQC/ContentView.swift index 96dc3f3..ebf73e3 100644 --- a/JanitorialQC/ContentView.swift +++ b/JanitorialQC/ContentView.swift @@ -7,6 +7,7 @@ struct ContentView: View { @EnvironmentObject private var auth: AuthManager @EnvironmentObject private var sync: SyncManager @StateObject private var updateChecker = UpdateChecker.shared + @Environment(\.scenePhase) private var scenePhase var body: some View { Group { @@ -42,6 +43,27 @@ struct ContentView: View { // normal app use. await updateChecker.checkForUpdate() } + // Stop the 60s notification poll when the app goes to background and + // restart it when it returns to the foreground. iOS suspends Tasks + // automatically in the background anyway, but explicitly managing the + // poll task here: + // • Prevents the Task object from accumulating sleep-resume cycles + // that never fired while suspended. + // • Triggers an immediate sync+poll when the inspector returns to the + // app after being away, rather than waiting up to 60s for the next + // scheduled poll tick. + // • Makes the lifecycle intent explicit and avoids relying on implicit + // iOS suspension behaviour. + .onChange(of: scenePhase) { _, newPhase in + switch newPhase { + case .background: + SyncManager.shared.suspendPolling() + case .active: + SyncManager.shared.resumePolling() + default: + break + } + } .alert("Update Available", isPresented: $updateChecker.updateAvailable) { Button("Update") { if let url = updateChecker.appStoreURL { diff --git a/JanitorialQC/Models/LocalIssue.swift b/JanitorialQC/Models/LocalIssue.swift index 249acbe..4bf0ff5 100644 --- a/JanitorialQC/Models/LocalIssue.swift +++ b/JanitorialQC/Models/LocalIssue.swift @@ -32,16 +32,60 @@ final class LocalIssue { private static let jsonDecoder = JSONDecoder() private static let jsonEncoder = JSONEncoder() + // Lightweight decode cache — avoids re-parsing identical JSON strings. + // SwiftData may call the getter multiple times per render pass (once for + // isEmpty, once for count, once for ForEach). Caching the last-decoded + // value by JSON string identity means the JSON parse only happens when + // the underlying data actually changes. + // @Transient tells SwiftData not to persist these — they're in-memory only. + // Lightweight decode cache — split into key+value pairs because SwiftData's + // @Transient macro does not support tuple types. Two separate @Transient + // properties per cache entry achieve the same result with no schema impact. + @Transient private var _cachedLocalKey: String = "" + @Transient private var _cachedLocalValue: [String] = [] + @Transient private var _cachedServerKey: String = "" + @Transient private var _cachedServerValue: [String] = [] + /// Decoded local photo paths (up to 5) var photoLocalPaths: [String] { - get { (try? Self.jsonDecoder.decode([String].self, from: Data(photoLocalPathsJSON.utf8))) ?? [] } - set { photoLocalPathsJSON = (try? String(data: Self.jsonEncoder.encode(newValue), encoding: .utf8)) ?? "[]" } + get { + if _cachedLocalKey == photoLocalPathsJSON, !_cachedLocalKey.isEmpty { + return _cachedLocalValue + } + let decoded = (try? Self.jsonDecoder.decode([String].self, + from: Data(photoLocalPathsJSON.utf8))) ?? [] + _cachedLocalKey = photoLocalPathsJSON + _cachedLocalValue = decoded + return decoded + } + set { + let encoded = (try? String(data: Self.jsonEncoder.encode(newValue), + encoding: .utf8)) ?? "[]" + photoLocalPathsJSON = encoded + _cachedLocalKey = encoded + _cachedLocalValue = newValue + } } /// Decoded server photo paths var photoServerPaths: [String] { - get { (try? Self.jsonDecoder.decode([String].self, from: Data(photoServerPathsJSON.utf8))) ?? [] } - set { photoServerPathsJSON = (try? String(data: Self.jsonEncoder.encode(newValue), encoding: .utf8)) ?? "[]" } + get { + if _cachedServerKey == photoServerPathsJSON, !_cachedServerKey.isEmpty { + return _cachedServerValue + } + let decoded = (try? Self.jsonDecoder.decode([String].self, + from: Data(photoServerPathsJSON.utf8))) ?? [] + _cachedServerKey = photoServerPathsJSON + _cachedServerValue = decoded + return decoded + } + set { + let encoded = (try? String(data: Self.jsonEncoder.encode(newValue), + encoding: .utf8)) ?? "[]" + photoServerPathsJSON = encoded + _cachedServerKey = encoded + _cachedServerValue = newValue + } } var createdAt: Date diff --git a/JanitorialQC/Models/LocalTemplate.swift b/JanitorialQC/Models/LocalTemplate.swift index 29b9f8a..776323f 100644 --- a/JanitorialQC/Models/LocalTemplate.swift +++ b/JanitorialQC/Models/LocalTemplate.swift @@ -15,6 +15,10 @@ final class LocalTemplate { var formSchemaJSON: String var lastSyncedAt: Date var isActive: Bool = true // phase21 — false templates excluded from picker + /// Timestamp of the last successful schema fetch (GET /api/v1/templates/{id}). + /// Nil when the schema has never been fetched (e.g. template just inserted). + /// Used to skip redundant detail calls when summary fields are unchanged. + var schemaFetchedAt: Date? = nil init(from summary: APITemplateSummary) { self.serverId = summary.id @@ -26,12 +30,20 @@ final class LocalTemplate { self.isActive = summary.isActive } - func updateSummary(from summary: APITemplateSummary) { + /// Update summary fields and return whether any field changed. + /// Used by pullReferenceData to skip schema re-fetching when nothing changed. + @discardableResult + func updateSummary(from summary: APITemplateSummary) -> Bool { + let changed = name != summary.name + || templateDescription != summary.description + || frequency != summary.frequency + || isActive != summary.isActive self.name = summary.name self.templateDescription = summary.description self.frequency = summary.frequency self.isActive = summary.isActive self.lastSyncedAt = Date() + return changed } func updateSchema(from template: APITemplate) { @@ -43,7 +55,8 @@ final class LocalTemplate { let str = String(data: data, encoding: .utf8) { self.formSchemaJSON = str } - self.lastSyncedAt = Date() + self.schemaFetchedAt = Date() + self.lastSyncedAt = Date() } /// Decode stored JSON back into [[String: Any]] for the form renderer diff --git a/JanitorialQC/Models/SyncQueueEntry.swift b/JanitorialQC/Models/SyncQueueEntry.swift index a93a8d1..34f4629 100644 --- a/JanitorialQC/Models/SyncQueueEntry.swift +++ b/JanitorialQC/Models/SyncQueueEntry.swift @@ -1,8 +1,17 @@ // Models/SyncQueueEntry.swift // --------------------------- -// SwiftData model for the outbox sync queue. -// Every offline write (inspection, issue, photo) enqueues an entry here. -// SyncManager processes entries in FIFO order when connectivity is restored. +// LEGACY — This model is no longer used. The original design enqueued every +// offline write here and processed in FIFO order; the current architecture uses +// LocalInspection.syncStatus / LocalIssue.syncStatus / PendingPhoto.uploadStatus +// directly (simpler, fewer moving parts, no double-bookkeeping). +// +// The model is kept registered in the SwiftData container solely to maintain +// schema compatibility with existing installs — removing it from modelContainer +// would trigger a migration failure on devices that already have the table. +// A future dedicated migration (phase N) can drop the table explicitly using +// op.execute("DROP TABLE IF EXISTS SyncQueueEntry") once it's safe to do so. +// +// DO NOT add new code that reads or writes this model. import Foundation import SwiftData diff --git a/JanitorialQC/Sync/SyncManager.swift b/JanitorialQC/Sync/SyncManager.swift index 2f831b8..41fc34f 100644 --- a/JanitorialQC/Sync/SyncManager.swift +++ b/JanitorialQC/Sync/SyncManager.swift @@ -118,6 +118,21 @@ class SyncManager: ObservableObject { stopPollTask() } + /// Called when the app enters the background (scenePhase == .background). + /// Stops the poll loop so it doesn't accumulate suspended sleep cycles. + func suspendPolling() { + stopPollTask() + } + + /// Called when the app returns to the foreground (scenePhase == .active). + /// Restarts the poll loop and immediately syncs so stale data is refreshed + /// without waiting up to 60s for the next scheduled tick. + func resumePolling() { + guard isOnline, AuthManager.shared.isAuthenticated else { return } + startPollTask() + Task { await triggerSync() } + } + /// Call when the user opens the NotificationsView to clear the badge. func markNotificationsViewed() { unreadNotificationCount = 0 @@ -136,8 +151,13 @@ class SyncManager: ObservableObject { deliverLocalNotification(n) } - // Update in-app inbox state - recentNotifications = notifications + recentNotifications.prefix(50 - notifications.count) + // Update in-app inbox state. + // Prepend new notifications and cap at 50 — avoids allocating two + // arrays and concatenating them on every poll (the old pattern + // `notifications + recentNotifications.prefix(50 - count)` always + // created a new array even when notifications.count >= 50). + recentNotifications.insert(contentsOf: notifications, at: 0) + if recentNotifications.count > 50 { recentNotifications = Array(recentNotifications.prefix(50)) } unreadNotificationCount += notifications.count // Update the cursor to the newest notification's timestamp so the @@ -493,13 +513,18 @@ class SyncManager: ObservableObject { ) for apiSummary in templatesData.templates { + let summaryChanged: Bool if let existing = templateMap[apiSummary.id] { - existing.updateSummary(from: apiSummary) + summaryChanged = existing.updateSummary(from: apiSummary) } else { context.insert(LocalTemplate(from: apiSummary)) + summaryChanged = true // new template — must fetch schema } try await upsertTemplateSchema( - id: apiSummary.id, context: context, templateMap: templateMap + id: apiSummary.id, + context: context, + templateMap: templateMap, + summaryChanged: summaryChanged ) } @@ -543,14 +568,8 @@ class SyncManager: ObservableObject { guard last == nil || Date().timeIntervalSince(last!) >= Self.cleanupInterval else { return } UserDefaults.standard.set(Date(), forKey: Self.lastCleanupKey) - let fm = FileManager.default - guard let docsDir = fm.urls(for: .documentDirectory, in: .userDomainMask).first else { return } - let photosDir = docsDir.appendingPathComponent("JQCPhotos") - guard let diskFiles = try? fm.contentsOfDirectory( - at: photosDir, includingPropertiesForKeys: nil - ) else { return } - - // Collect all local paths that are still in use. + // ── Phase 1: collect referenced paths on @MainActor (SwiftData fetches) ── + // These are fast in-memory operations — always runs on the main actor. var referencedPaths = Set() // PendingPhoto — not yet uploaded @@ -576,17 +595,29 @@ class SyncManager: ObservableObject { } } - // Delete any disk file not in referencedPaths - var deletedCount = 0 - for fileURL in diskFiles { - let path = fileURL.path - if !referencedPaths.contains(path) { - try? fm.removeItem(at: fileURL) - deletedCount += 1 + // ── Phase 2: FileManager enumeration + deletion on a background thread ── + // Directory enumeration and file removal are I/O-bound and can stutter + // the main thread when JQCPhotos/ contains hundreds of files. Dispatching + // here is safe because `referencedPaths` is a value type (Set) + // captured by copy — no shared mutable state crosses the boundary. + 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") + guard let diskFiles = try? fm.contentsOfDirectory( + at: photosDir, includingPropertiesForKeys: nil + ) else { return } + + var deletedCount = 0 + for fileURL in diskFiles { + if !referencedPaths.contains(fileURL.path) { + try? fm.removeItem(at: fileURL) + deletedCount += 1 + } + } + if deletedCount > 0 { + print("[JQC] Sync | cleanupOrphanedPhotos | removed \(deletedCount) file(s)") } - } - if deletedCount > 0 { - print("[JQC] Sync | cleanupOrphanedPhotos | removed \(deletedCount) file(s)") } } @@ -631,18 +662,33 @@ class SyncManager: ObservableObject { private func upsertTemplateSchema( id: Int, context: ModelContext, - templateMap: [Int: LocalTemplate] + templateMap: [Int: LocalTemplate], + summaryChanged: Bool ) async throws { + let existing = templateMap[id] ?? (try? context.fetch(FetchDescriptor()))? + .first { $0.serverId == id } + + // Skip the detail API call if: + // • The schema was already fetched (schemaFetchedAt is non-nil) + // • No summary fields changed this sync pass (name, frequency, isActive) + // • The local schema is non-empty (not a first-run blank) + // + // This reduces N sequential GET /api/v1/templates/{id} calls to zero + // on a typical sync where templates haven't changed — the common case. + // The schema is always re-fetched when any summary field changes, + // when schemaFetchedAt is nil (new template or first launch), or + // when the local schema is empty ("[]"). + if let existing, + existing.schemaFetchedAt != nil, + existing.formSchemaJSON != "[]", + !summaryChanged { + return // schema is current — no network call needed + } + let detailData: TemplateDetailResponseData = try await APIClient.shared.request( "/api/v1/templates/\(id)" ) - if let existing = templateMap[id] { - existing.updateSchema(from: detailData.template) - } else { - (try? context.fetch(FetchDescriptor()))? - .first { $0.serverId == id }? - .updateSchema(from: detailData.template) - } + existing?.updateSchema(from: detailData.template) } // ── Pull server-assigned issues ───────────────────────────────────────