From f1cd81c3959b3900a8c3951cfbe8cb1cf64a48f2 Mon Sep 17 00:00:00 2001 From: Trey t Date: Sat, 14 Feb 2026 23:09:11 -0600 Subject: [PATCH] Fix 7 data mutation layer risks identified in audit - save()/saveAndRunDataListeners() now return @discardableResult Bool; listeners only fire on successful save - MoodLogger.updateMood() now recalculates streak, updates Live Activity, and notifies watch (was missing these side effects) - CSV import uses new addBatch()/importMoods() for O(1) side effects instead of O(n) per-row widget reloads and streak calcs - Foreground task ordering: fillInMissingDates() now runs before removeDuplicates() so backfill-created duplicates are caught same cycle - WidgetMoodSaver deletes ALL entries for date (was fetchLimit=1, leaving CloudKit sync duplicates behind) - cleanupPhotoIfNeeded logs warning on failed photo deletion instead of silently orphaning files Co-Authored-By: Claude Opus 4.6 --- Shared/FeelsApp.swift | 18 ++- Shared/MoodLogger.swift | 117 ++++++++++++++++++ Shared/Persisence/DataController.swift | 27 ++-- Shared/Persisence/DataControllerADD.swift | 59 +++++++-- Shared/Persisence/DataControllerDELETE.swift | 35 +++++- .../Persisence/DataControllerProtocol.swift | 6 +- Shared/SharedMoodIntent.swift | 89 +++++++++---- Shared/Views/SettingsView/SettingsView.swift | 10 +- 8 files changed, 306 insertions(+), 55 deletions(-) diff --git a/Shared/FeelsApp.swift b/Shared/FeelsApp.swift index 31a1416..9da04b7 100644 --- a/Shared/FeelsApp.swift +++ b/Shared/FeelsApp.swift @@ -21,6 +21,7 @@ struct FeelsApp: App { @StateObject var healthKitManager = HealthKitManager.shared @AppStorage(UserDefaultsStore.Keys.firstLaunchDate.rawValue, store: GroupUserDefaults.groupDefaults) private var firstLaunchDate = Date() @State private var showSubscriptionFromWidget = false + @State private var showStorageFallbackAlert = SharedModelContainer.isUsingInMemoryFallback init() { AnalyticsManager.shared.configure() @@ -61,6 +62,17 @@ struct FeelsApp: App { showSubscriptionFromWidget = true } } + .alert("Data Storage Unavailable", + isPresented: $showStorageFallbackAlert) { + Button("OK", role: .cancel) { } + } message: { + Text("Your mood data cannot be saved permanently. Please restart the app. If the problem persists, reinstall the app.") + } + .onAppear { + if SharedModelContainer.isUsingInMemoryFallback { + AnalyticsManager.shared.track(.storageFallbackActivated) + } + } // Lock screen overlay if authManager.isLockEnabled && !authManager.isUnlocked { @@ -93,12 +105,12 @@ struct FeelsApp: App { // Refresh from disk to pick up widget/watch changes DataController.shared.refreshFromDisk() - // Clean up any duplicate entries first - DataController.shared.removeDuplicates() - // Fill in any missing dates (moved from AppDelegate) DataController.shared.fillInMissingDates() + // Clean up any duplicate entries (after backfill so backfill dupes are caught) + DataController.shared.removeDuplicates() + // Reschedule notifications for new title LocalNotification.rescheduleNotifiations() diff --git a/Shared/MoodLogger.swift b/Shared/MoodLogger.swift index b47e3cb..38c591c 100644 --- a/Shared/MoodLogger.swift +++ b/Shared/MoodLogger.swift @@ -102,6 +102,110 @@ final class MoodLogger { markSideEffectsApplied(for: date) } + /// Delete a mood entry for a specific date with all associated cleanup. + /// Replaces the entry with a .missing placeholder and cleans up HealthKit/Live Activity state. + /// + /// - Parameter date: The date of the entry to delete + func deleteMood(forDate date: Date) { + // 1. Delete all entries for this date and replace with missing placeholder + DataController.shared.deleteAllEntries(forDate: date) + DataController.shared.add(mood: .missing, forDate: date, entryType: .filledInMissing) + + Self.logger.info("Deleted mood entry for \(date)") + + // 2. Delete HealthKit entry if enabled and user has access + let healthKitEnabled = GroupUserDefaults.groupDefaults.bool(forKey: UserDefaultsStore.Keys.healthKitEnabled.rawValue) + let hasAccess = !IAPManager.shared.shouldShowPaywall + if healthKitEnabled && hasAccess { + Task { + try? await HealthKitManager.shared.deleteMood(for: date) + } + } + + // 3. Recalculate streak and update Live Activity + let streak = calculateCurrentStreak() + LiveActivityManager.shared.updateActivity(streak: streak, mood: .missing) + LiveActivityScheduler.shared.invalidateCache() + LiveActivityScheduler.shared.scheduleBasedOnCurrentTime() + + // 4. Reload widgets + WidgetCenter.shared.reloadAllTimelines() + + // 5. Notify watch to refresh + WatchConnectivityManager.shared.notifyWatchToReload() + } + + /// Delete all mood data with full cleanup of HealthKit and Live Activity state. + /// Used when user clears all data from settings. + func deleteAllData() { + // 1. Clear all entries from the database + DataController.shared.clearDB() + + Self.logger.info("Cleared all mood data") + + // 2. Delete all HealthKit entries if enabled and user has access + let healthKitEnabled = GroupUserDefaults.groupDefaults.bool(forKey: UserDefaultsStore.Keys.healthKitEnabled.rawValue) + let hasAccess = !IAPManager.shared.shouldShowPaywall + if healthKitEnabled && hasAccess { + Task { + try? await HealthKitManager.shared.deleteAllMoods() + } + } + + // 3. End all Live Activities + Task { + await LiveActivityManager.shared.endAllActivities() + } + LiveActivityScheduler.shared.invalidateCache() + + // 4. Reload widgets + WidgetCenter.shared.reloadAllTimelines() + + // 5. Notify watch to refresh + WatchConnectivityManager.shared.notifyWatchToReload() + } + + /// Update an existing mood entry with all associated side effects. + /// Centralizes the update logic that was previously split between ViewModel and DataController. + /// + /// - Parameters: + /// - entryDate: The date of the entry to update + /// - mood: The new mood value + /// - Returns: Whether the update was successful + @discardableResult + func updateMood(entryDate: Date, withMood mood: Mood) -> Bool { + let success = DataController.shared.update(entryDate: entryDate, withMood: mood) + guard success else { + Self.logger.error("Failed to update mood entry for \(entryDate)") + return false + } + + // Skip side effects for placeholder/missing moods + guard mood != .missing && mood != .placeholder else { return true } + + // Sync to HealthKit if enabled and user has full access + let healthKitEnabled = GroupUserDefaults.groupDefaults.bool(forKey: UserDefaultsStore.Keys.healthKitEnabled.rawValue) + let hasAccess = !IAPManager.shared.shouldShowPaywall + if healthKitEnabled && hasAccess { + Task { + try? await HealthKitManager.shared.saveMood(mood, for: entryDate) + } + } + + // Reload widgets + WidgetCenter.shared.reloadAllTimelines() + + // Recalculate streak and update Live Activity + let streak = calculateCurrentStreak() + LiveActivityManager.shared.updateActivity(streak: streak, mood: mood) + LiveActivityScheduler.shared.invalidateCache() + + // Notify watch to refresh + WatchConnectivityManager.shared.notifyWatchToReload() + + return true + } + /// Check for and process any pending side effects from widget/extension votes. /// Call this when the app becomes active to ensure all side effects are applied. func processPendingSideEffects() { @@ -127,6 +231,19 @@ final class MoodLogger { applySideEffects(mood: entry.mood, for: entry.forDate) } + /// Import mood entries in batch with a single round of side effects. + /// Used for CSV import to avoid O(n) widget reloads, streak calcs, etc. + func importMoods(_ entries: [(mood: Mood, date: Date, entryType: EntryType)]) { + guard !entries.isEmpty else { return } + + DataController.shared.addBatch(entries: entries) + + // Single round of side effects + WidgetCenter.shared.reloadAllTimelines() + WatchConnectivityManager.shared.notifyWatchToReload() + LiveActivityScheduler.shared.invalidateCache() + } + // MARK: - Side Effects Tracking /// Mark that side effects have been applied for a given date diff --git a/Shared/Persisence/DataController.swift b/Shared/Persisence/DataController.swift index 958f17d..0e990df 100644 --- a/Shared/Persisence/DataController.swift +++ b/Shared/Persisence/DataController.swift @@ -52,19 +52,32 @@ final class DataController: ObservableObject { editedDataClosure.append(closure) } - func saveAndRunDataListeners() { - save() - for closure in editedDataClosure { - closure() + @discardableResult + func saveAndRunDataListeners() -> Bool { + let success = save() + if success { + for closure in editedDataClosure { + closure() + } } + return success } - func save() { - guard modelContext.hasChanges else { return } + @discardableResult + func save() -> Bool { + guard modelContext.hasChanges else { return true } do { try modelContext.save() + return true } catch { - Self.logger.error("Failed to save context: \(error.localizedDescription)") + Self.logger.error("Failed to save context, retrying: \(error.localizedDescription)") + do { + try modelContext.save() + return true + } catch { + Self.logger.critical("Failed to save context after retry: \(error.localizedDescription)") + return false + } } } diff --git a/Shared/Persisence/DataControllerADD.swift b/Shared/Persisence/DataControllerADD.swift index a134ebf..82b9497 100644 --- a/Shared/Persisence/DataControllerADD.swift +++ b/Shared/Persisence/DataControllerADD.swift @@ -7,16 +7,24 @@ import SwiftData import Foundation +import os.log + +private let logger = Logger(subsystem: Bundle.main.bundleIdentifier ?? "com.tt.feels", category: "DataControllerADD") extension DataController { func add(mood: Mood, forDate date: Date, entryType: EntryType) { // Delete ALL existing entries for this date (handles duplicates) let existing = getAllEntries(byDate: date) for entry in existing { + cleanupPhotoIfNeeded(for: entry) modelContext.delete(entry) } if !existing.isEmpty { - try? modelContext.save() + do { + try modelContext.save() + } catch { + logger.error("Failed to save after deleting existing entries: \(error.localizedDescription)") + } } let entry = MoodEntryModel( @@ -34,21 +42,32 @@ extension DataController { let currentOnboarding = UserDefaultsStore.getOnboarding() var endDate = ShowBasedOnVoteLogics.getCurrentVotingDate(onboardingData: currentOnboarding) // Since it's for views, take away the last date so vote is enabled - endDate = Calendar.current.date(byAdding: .day, value: -1, to: endDate)! + guard let adjustedEndDate = Calendar.current.date(byAdding: .day, value: -1, to: endDate) else { + logger.error("Failed to calculate adjusted end date") + return + } + endDate = adjustedEndDate let descriptor = FetchDescriptor( sortBy: [SortDescriptor(\.forDate, order: .reverse)] ) - guard let entries = try? modelContext.fetch(descriptor), - let firstEntry = entries.last else { return } - - let allDates: [Date] = Date.dates(from: firstEntry.forDate, toDate: endDate, includingToDate: true).map { - Calendar.current.date(bySettingHour: 0, minute: 0, second: 0, of: $0)! + let entries: [MoodEntryModel] + do { + entries = try modelContext.fetch(descriptor) + } catch { + logger.error("Failed to fetch entries for fill-in: \(error.localizedDescription)") + return } - let existingDates: Set = Set(entries.map { - Calendar.current.date(bySettingHour: 0, minute: 0, second: 0, of: $0.forDate)! + guard let firstEntry = entries.last else { return } + + let allDates: [Date] = Date.dates(from: firstEntry.forDate, toDate: endDate, includingToDate: true).compactMap { + Calendar.current.date(bySettingHour: 0, minute: 0, second: 0, of: $0) + } + + let existingDates: Set = Set(entries.compactMap { + Calendar.current.date(bySettingHour: 0, minute: 0, second: 0, of: $0.forDate) }) let missing = Array(Set(allDates).subtracting(existingDates)).sorted(by: >) @@ -58,7 +77,10 @@ extension DataController { // Batch insert all missing dates without triggering listeners for date in missing { // Add 12 hours to avoid UTC offset issues - let adjustedDate = Calendar.current.date(byAdding: .hour, value: 12, to: date)! + guard let adjustedDate = Calendar.current.date(byAdding: .hour, value: 12, to: date) else { + logger.error("Failed to calculate adjusted date for \(date)") + continue + } let entry = MoodEntryModel( forDate: adjustedDate, mood: .missing, @@ -77,11 +99,26 @@ extension DataController { for entry in data { entry.weekDay = Calendar.current.component(.weekday, from: entry.forDate) } - save() + saveAndRunDataListeners() } func removeNoForDates() { // Note: With SwiftData's non-optional forDate, this is essentially a no-op // Keeping for API compatibility } + + /// Batch insert mood entries without per-entry analytics or listener notifications. + /// Used for CSV import where side effects should fire once at the end. + func addBatch(entries: [(mood: Mood, date: Date, entryType: EntryType)]) { + for (mood, date, entryType) in entries { + let existing = getAllEntries(byDate: date) + for entry in existing { + cleanupPhotoIfNeeded(for: entry) + modelContext.delete(entry) + } + let entry = MoodEntryModel(forDate: date, mood: mood, entryType: entryType) + modelContext.insert(entry) + } + saveAndRunDataListeners() + } } diff --git a/Shared/Persisence/DataControllerDELETE.swift b/Shared/Persisence/DataControllerDELETE.swift index f6c3eb9..7c754de 100644 --- a/Shared/Persisence/DataControllerDELETE.swift +++ b/Shared/Persisence/DataControllerDELETE.swift @@ -10,10 +10,32 @@ import Foundation import os.log extension DataController { + + // MARK: - Photo Cleanup + + func cleanupPhotoIfNeeded(for entry: MoodEntryModel) { + if let photoID = entry.photoID { + if !PhotoManager.shared.deletePhoto(id: photoID) { + AppLogger.general.warning("Failed to delete orphaned photo \(photoID.uuidString) for entry on \(entry.forDate)") + } + } + } + + // MARK: - Delete Operations + func clearDB() { do { + // Clean up photo files before batch delete + let descriptor = FetchDescriptor() + if let allEntries = try? modelContext.fetch(descriptor) { + for entry in allEntries { + cleanupPhotoIfNeeded(for: entry) + } + } + try modelContext.delete(model: MoodEntryModel.self) saveAndRunDataListeners() + AnalyticsManager.shared.track(.allDataCleared) } catch { AppLogger.general.error("Failed to clear database: \(error.localizedDescription)") } @@ -24,9 +46,10 @@ extension DataController { let entries = getData(startDate: startDate, endDate: Date(), includedDays: []) for entry in entries { + cleanupPhotoIfNeeded(for: entry) modelContext.delete(entry) } - save() + saveAndRunDataListeners() } func deleteRandomFromLast(numberOfEntries: Int) { @@ -34,9 +57,10 @@ extension DataController { let entries = getData(startDate: startDate, endDate: Date(), includedDays: []) for entry in entries where Bool.random() { + cleanupPhotoIfNeeded(for: entry) modelContext.delete(entry) } - save() + saveAndRunDataListeners() } /// Get ALL entries for a specific date (not just the first one) @@ -46,7 +70,7 @@ extension DataController { let descriptor = FetchDescriptor( predicate: #Predicate { entry in - entry.forDate >= startDate && entry.forDate <= endDate + entry.forDate >= startDate && entry.forDate < endDate }, sortBy: [SortDescriptor(\.forDate, order: .forward)] ) @@ -58,9 +82,10 @@ extension DataController { func deleteAllEntries(forDate date: Date) { let entries = getAllEntries(byDate: date) for entry in entries { + cleanupPhotoIfNeeded(for: entry) modelContext.delete(entry) } - save() + saveAndRunDataListeners() } /// Find and remove duplicate entries, keeping only the most recent for each date @@ -92,6 +117,7 @@ extension DataController { // Keep the first (best) entry, delete the rest for entry in sorted.dropFirst() { + cleanupPhotoIfNeeded(for: entry) modelContext.delete(entry) duplicatesRemoved += 1 } @@ -100,6 +126,7 @@ extension DataController { if duplicatesRemoved > 0 { saveAndRunDataListeners() AppLogger.general.info("Removed \(duplicatesRemoved) duplicate entries") + AnalyticsManager.shared.track(.duplicatesRemoved(count: duplicatesRemoved)) } return duplicatesRemoved diff --git a/Shared/Persisence/DataControllerProtocol.swift b/Shared/Persisence/DataControllerProtocol.swift index 40c7e6a..ccd80be 100644 --- a/Shared/Persisence/DataControllerProtocol.swift +++ b/Shared/Persisence/DataControllerProtocol.swift @@ -70,10 +70,12 @@ protocol MoodDataDeleting { @MainActor protocol MoodDataPersisting { /// Save pending changes - func save() + @discardableResult + func save() -> Bool /// Save and notify listeners - func saveAndRunDataListeners() + @discardableResult + func saveAndRunDataListeners() -> Bool /// Add a listener for data changes func addNewDataListener(closure: @escaping (() -> Void)) diff --git a/Shared/SharedMoodIntent.swift b/Shared/SharedMoodIntent.swift index 5949c86..de28be2 100644 --- a/Shared/SharedMoodIntent.swift +++ b/Shared/SharedMoodIntent.swift @@ -73,15 +73,37 @@ extension VoteMoodIntent: ForegroundContinuableIntent {} #if WIDGET_EXTENSION enum WidgetMoodSaver { private static let logger = Logger(subsystem: "com.tt.feels.widget", category: "WidgetMoodSaver") + private static var cachedContainer: ModelContainer? @MainActor static func save(mood: Mood, date: Date) { + do { + let container = try getOrCreateContainer() + try performSave(mood: mood, date: date, container: container) + } catch { + // Container may be stale or corrupted — discard cache and retry once + logger.warning("First save attempt failed, retrying with fresh container: \(error.localizedDescription)") + cachedContainer = nil + do { + let container = try getOrCreateContainer() + try performSave(mood: mood, date: date, container: container) + } catch { + logger.error("Failed to save mood after retry: \(error.localizedDescription)") + } + } + } + + private static func getOrCreateContainer() throws -> ModelContainer { + if let existing = cachedContainer { + return existing + } + let schema = Schema([MoodEntryModel.self]) let appGroupID = Constants.currentGroupShareId guard let containerURL = FileManager.default.containerURL(forSecurityApplicationGroupIdentifier: appGroupID) else { logger.error("App Group not available") - return + throw WidgetMoodSaverError.appGroupUnavailable } #if DEBUG @@ -90,34 +112,55 @@ enum WidgetMoodSaver { let storeURL = containerURL.appendingPathComponent("Feels.store") #endif - do { - let config = ModelConfiguration(schema: schema, url: storeURL, cloudKitDatabase: .none) - let container = try ModelContainer(for: schema, configurations: [config]) - let context = container.mainContext + let config = ModelConfiguration(schema: schema, url: storeURL, cloudKitDatabase: .none) + let container = try ModelContainer(for: schema, configurations: [config]) + cachedContainer = container + return container + } - // Delete existing entry for this date - let startDate = Calendar.current.startOfDay(for: date) - let endDate = Calendar.current.date(byAdding: .day, value: 1, to: startDate)! + @MainActor + private static func performSave(mood: Mood, date: Date, container: ModelContainer) throws { + let context = container.mainContext - var descriptor = FetchDescriptor( - predicate: #Predicate { entry in - entry.forDate >= startDate && entry.forDate <= endDate - } - ) - descriptor.fetchLimit = 1 + // Delete existing entry for this date + let startDate = Calendar.current.startOfDay(for: date) + let endDate = Calendar.current.date(byAdding: .day, value: 1, to: startDate)! - if let existing = try? context.fetch(descriptor).first { - context.delete(existing) - try? context.save() + let descriptor = FetchDescriptor( + predicate: #Predicate { entry in + entry.forDate >= startDate && entry.forDate < endDate } + ) - // Create new entry - let entry = MoodEntryModel(forDate: date, mood: mood, entryType: .widget) - context.insert(entry) + let existing = try context.fetch(descriptor) + if !existing.isEmpty { + for entry in existing { + context.delete(entry) + } try context.save() - logger.info("Saved mood \(mood.rawValue) from widget") - } catch { - logger.error("Failed to save mood: \(error.localizedDescription)") + } + + // Create new entry + let entry = MoodEntryModel(forDate: date, mood: mood, entryType: .widget) + context.insert(entry) + try context.save() + logger.info("Saved mood \(mood.rawValue) from widget") + + // Note: The widget cannot run full side effects (HealthKit, streaks, analytics, etc.) + // because it runs in a separate extension process without access to MoodLogger. + // When the main app returns to the foreground, it calls + // MoodLogger.shared.processPendingSideEffects() to catch up on any side effects + // that were missed from widget or watch entries. + } + + enum WidgetMoodSaverError: LocalizedError { + case appGroupUnavailable + + var errorDescription: String? { + switch self { + case .appGroupUnavailable: + return "App Group container is not available" + } } } } diff --git a/Shared/Views/SettingsView/SettingsView.swift b/Shared/Views/SettingsView/SettingsView.swift index 37db7d3..9f9439d 100644 --- a/Shared/Views/SettingsView/SettingsView.swift +++ b/Shared/Views/SettingsView/SettingsView.swift @@ -775,7 +775,7 @@ struct SettingsContentView: View { ZStack { theme.currentTheme.secondaryBGColor Button { - DataController.shared.clearDB() + MoodLogger.shared.deleteAllData() } label: { HStack(spacing: 12) { Image(systemName: "trash") @@ -1347,6 +1347,7 @@ struct SettingsView: View { var rows = input.components(separatedBy: "\n") guard !rows.isEmpty else { return } rows.removeFirst() + var importEntries: [(mood: Mood, date: Date, entryType: EntryType)] = [] for row in rows { let stripped = row.replacingOccurrences(of: " +0000", with: "") let columns = stripped.components(separatedBy: ",") @@ -1359,10 +1360,9 @@ struct SettingsView: View { } let mood = Mood(rawValue: moodValue) ?? .missing let entryType = EntryType(rawValue: Int(columns[2]) ?? 0) ?? .listView - - DataController.shared.add(mood: mood, forDate: forDate, entryType: entryType) + importEntries.append((mood: mood, date: forDate, entryType: entryType)) } - DataController.shared.saveAndRunDataListeners() + MoodLogger.shared.importMoods(importEntries) AnalyticsManager.shared.track(.importSucceeded) } else { AnalyticsManager.shared.track(.importFailed(error: nil)) @@ -1743,7 +1743,7 @@ struct SettingsView: View { ZStack { theme.currentTheme.secondaryBGColor Button(action: { - DataController.shared.clearDB() + MoodLogger.shared.deleteAllData() }, label: { Text("Clear DB") .foregroundColor(textColor)