diff --git a/Forji/Forji/App/ContentView.swift b/Forji/Forji/App/ContentView.swift index 94c3cd3..fcf2d95 100644 --- a/Forji/Forji/App/ContentView.swift +++ b/Forji/Forji/App/ContentView.swift @@ -81,7 +81,9 @@ struct ContentView: View { do { try await authService.restoreSession(instance: instance) instance.lastUsed = Date() - try? modelContext.save() + // Only the last-used timestamp rides on this save: a failure costs list + // ordering, not account data, and automatic restore has no error surface. + _ = modelContext.saveOrRollback() } catch { if Self.shouldLogout(after: error) { await authService.logout(modelContext: modelContext) @@ -139,7 +141,7 @@ struct ContentView: View { do { try await authService.restoreSession(instance: defaultInstance) defaultInstance.lastUsed = Date() - try? modelContext.save() + _ = modelContext.saveOrRollback() sessionFullyRestored = true } catch { // A transient/network failure leaves the bootstrapped session in place so the @@ -154,7 +156,7 @@ struct ContentView: View { do { try await authService.restoreSession(instance: defaultInstance) defaultInstance.lastUsed = Date() - try? modelContext.save() + _ = modelContext.saveOrRollback() } catch { // Auto-login failed, fall through to instance list } diff --git a/Forji/Forji/Helpers/PersistenceSave.swift b/Forji/Forji/Helpers/PersistenceSave.swift new file mode 100644 index 0000000..32710f9 --- /dev/null +++ b/Forji/Forji/Helpers/PersistenceSave.swift @@ -0,0 +1,36 @@ +import Foundation +import SwiftData + +/// Turns a SwiftData save failure into a message a view can show. +/// +/// `assertionFailure` is compiled out under `-O`, so a `catch` that only asserts +/// discards the error entirely in Release builds and lets the caller continue as +/// if the write had succeeded. These helpers hand the failure back instead. +enum PersistenceSave { + static func message(for error: Error) -> String { + "Your change could not be saved: \(error.localizedDescription)" + } +} + +extension ModelContext { + /// Saves pending changes and returns a user-facing message when the save fails. + /// + /// A failed save rolls the context back so the in-memory state matches what is + /// actually on disk. Without it a view keeps showing an inserted or edited + /// object that would be gone on the next launch. + func saveOrRollback() -> String? { + saveOrRollback { try save() } + } + + /// Testing seam: `performSave` lets tests drive the failure path, which a real + /// save on an in-memory store cannot reach. + func saveOrRollback(performSave: () throws -> Void) -> String? { + do { + try performSave() + return nil + } catch { + rollback() + return PersistenceSave.message(for: error) + } + } +} diff --git a/Forji/Forji/Services/AuthenticationService.swift b/Forji/Forji/Services/AuthenticationService.swift index 66df955..e656fb5 100644 --- a/Forji/Forji/Services/AuthenticationService.swift +++ b/Forji/Forji/Services/AuthenticationService.swift @@ -103,10 +103,7 @@ class AuthenticationService { // unauthenticatable orphan, and bail out without clearing the session. instance.isDefault = false modelContext.delete(instance) - do { - try modelContext.save() - } catch { - assertionFailure("SwiftData save failed during logout: \(error)") + if modelContext.saveOrRollback() != nil { return } // Delete credentials only after the instance is gone from SwiftData. diff --git a/Forji/Forji/Views/InstanceFormView.swift b/Forji/Forji/Views/InstanceFormView.swift index 8a72c9e..ae511da 100644 --- a/Forji/Forji/Views/InstanceFormView.swift +++ b/Forji/Forji/Views/InstanceFormView.swift @@ -4,23 +4,6 @@ import SwiftUI // swiftlint:disable:next type_body_length struct InstanceFormView: View { - enum Mode: Identifiable { - case add - case edit(ForgejoInstance) - - var id: String { - switch self { - case .add: "add" - case let .edit(instance): instance.serverURL + instance.username - } - } - } - - enum AuthMode: String, CaseIterable { - case credentials = "Password" - case token = "API Token" - } - @Environment(\.modelContext) private var modelContext @Environment(\.dismiss) private var dismiss @Query(sort: \ForgejoInstance.lastUsed, order: .reverse) private var instances: [ForgejoInstance] @@ -49,6 +32,10 @@ struct InstanceFormView: View { @State private var isLoading = false @State private var errorMessage: String? @State private var showError = false + @State private var errorTitle = Self.loginFailedTitle + + private static let loginFailedTitle = "Login Failed" + private static let saveFailedTitle = "Save Failed" init(authService: AuthenticationService, mode: Mode) { self.authService = authService @@ -175,7 +162,7 @@ struct InstanceFormView: View { Button("Cancel") { dismiss() } } } - .errorAlert("Login Failed", message: $errorMessage, isPresented: $showError) + .errorAlert(errorTitle, message: $errorMessage, isPresented: $showError) .onAppear { populateFields() } @@ -248,6 +235,7 @@ struct InstanceFormView: View { return inst.name == name && instId != editingInstanceId } if conflict != nil { + errorTitle = Self.loginFailedTitle errorMessage = "An instance named \"\(name)\" already exists. Please choose a different name." showError = true return @@ -290,10 +278,12 @@ struct InstanceFormView: View { useTokenAuth: authMode == .token, ) modelContext.insert(instance) - do { - try modelContext.save() - } catch { - assertionFailure("SwiftData save failed: \(error)") + // The rollback drops the insert, so a store that refused the write + // cannot leave the app reporting a connection to an account that + // would be missing on the next launch. + if let failure = modelContext.saveOrRollback() { + reportSaveFailure(failure) + return } authService.currentInstance = instance @@ -332,10 +322,11 @@ struct InstanceFormView: View { } instance.isDefault = isDefault - do { - try modelContext.save() - } catch { - assertionFailure("SwiftData save failed: \(error)") + // The rollback restores the instance's previous field values so the + // form and list keep showing what is actually stored. + if let failure = modelContext.saveOrRollback() { + reportSaveFailure(failure) + return } authService.currentInstance = instance } @@ -344,6 +335,7 @@ struct InstanceFormView: View { } catch AuthenticationError.otpRequired { needsOTP = true } catch { + errorTitle = Self.loginFailedTitle errorMessage = error.localizedDescription showError = true } @@ -352,6 +344,15 @@ struct InstanceFormView: View { } } + /// Surfaces a failed persist and leaves the form open so the account is never + /// presented as saved when it is not. + private func reportSaveFailure(_ message: String) { + errorTitle = Self.saveFailedTitle + errorMessage = message + showError = true + isLoading = false + } + /// Sets the duplicate-account error and returns true if another stored account /// already uses this server+username. `excluding` skips the instance being edited. private func rejectDuplicateAccount( @@ -362,25 +363,12 @@ struct InstanceFormView: View { guard Self.accountAlreadyExists( serverURL: normalizedURL, username: username, in: instances, excluding: excluding, ) else { return false } + errorTitle = Self.loginFailedTitle errorMessage = "An account for \"\(username)\" on \(normalizedURL) already exists." showError = true isLoading = false return true } - - /// True if `instances` already contains an account with this server+username, - /// ignoring `excluding` (the instance being edited). Accounts that share a - /// server+username collide on the same sourceKey, which crashes merged overviews. - static func accountAlreadyExists( - serverURL normalizedURL: String, - username: String, - in instances: [ForgejoInstance], - excluding: ForgejoInstance? = nil, - ) -> Bool { - instances.contains { inst in - inst !== excluding && inst.serverURL == normalizedURL && inst.username == username - } - } } #if DEBUG diff --git a/Forji/Forji/Views/InstanceFormViewSupport.swift b/Forji/Forji/Views/InstanceFormViewSupport.swift new file mode 100644 index 0000000..1546d4e --- /dev/null +++ b/Forji/Forji/Views/InstanceFormViewSupport.swift @@ -0,0 +1,36 @@ +import Foundation + +/// Supporting types and pure checks for `InstanceFormView`, kept out of the view file +/// so it stays within the file length limit. +extension InstanceFormView { + enum Mode: Identifiable { + case add + case edit(ForgejoInstance) + + var id: String { + switch self { + case .add: "add" + case let .edit(instance): instance.serverURL + instance.username + } + } + } + + enum AuthMode: String, CaseIterable { + case credentials = "Password" + case token = "API Token" + } + + /// True if `instances` already contains an account with this server+username, + /// ignoring `excluding` (the instance being edited). Accounts that share a + /// server+username collide on the same sourceKey, which crashes merged overviews. + static func accountAlreadyExists( + serverURL normalizedURL: String, + username: String, + in instances: [ForgejoInstance], + excluding: ForgejoInstance? = nil, + ) -> Bool { + instances.contains { inst in + inst !== excluding && inst.serverURL == normalizedURL && inst.username == username + } + } +} diff --git a/Forji/Forji/Views/InstanceListView.swift b/Forji/Forji/Views/InstanceListView.swift index 0e657ca..223a44b 100644 --- a/Forji/Forji/Views/InstanceListView.swift +++ b/Forji/Forji/Views/InstanceListView.swift @@ -14,6 +14,10 @@ struct InstanceListView: View { @State private var connectingAll = false @State private var errorMessage: String? @State private var showError = false + @State private var errorTitle = Self.connectionFailedTitle + + private static let connectionFailedTitle = "Connection Failed" + private static let saveFailedTitle = "Save Failed" init(authService: AuthenticationService, multiInstanceManager: Binding) { self.authService = authService @@ -89,7 +93,7 @@ struct InstanceListView: View { .sheet(item: $editingInstance) { instance in InstanceFormView(authService: authService, mode: .edit(instance)) } - .errorAlert("Connection Failed", message: $errorMessage, isPresented: $showError) + .errorAlert(errorTitle, message: $errorMessage, isPresented: $showError) } } @@ -196,14 +200,11 @@ struct InstanceListView: View { do { try await authService.restoreSession(instance: instance) instance.lastUsed = Date() - do { - try modelContext.save() - } catch { - assertionFailure("SwiftData save failed: \(error)") - } + persist() } catch is KeychainError { editingInstance = instance } catch { + errorTitle = Self.connectionFailedTitle errorMessage = error.localizedDescription showError = true } @@ -212,17 +213,18 @@ struct InstanceListView: View { } private func deleteInstance(_ instance: ForgejoInstance) { - // Disconnect if deleting the currently active instance - if authService.currentInstance?.id == instance.id { - authService.disconnect() - } + let wasActive = authService.currentInstance?.id == instance.id let normalizedURL = ForgejoClient.normalizeServerURL(instance.serverURL) let username = instance.username + + // Remove the instance from SwiftData first. A failed save rolls the delete + // back, so the credentials have to stay put or the restored account would be + // left unauthenticatable. modelContext.delete(instance) - do { - try modelContext.save() - } catch { - assertionFailure("SwiftData save failed: \(error)") + guard persist() else { return } + + if wasActive { + authService.disconnect() } Task { try? await KeychainManager.shared.deleteCredentials(for: normalizedURL, username: username) @@ -233,15 +235,16 @@ struct InstanceListView: View { defaultAllInstances.toggle() if defaultAllInstances { ForgejoInstance.clearDefaults(in: instances) - do { - try modelContext.save() - } catch { - assertionFailure("SwiftData save failed: \(error)") + if !persist() { + // The stored per-instance defaults survived the rollback, so the + // @AppStorage flag must go back to match them. + defaultAllInstances = false } } } private func toggleDefault(_ instance: ForgejoInstance) { + let previousDefaultAll = defaultAllInstances if instance.isDefault { instance.isDefault = false } else { @@ -249,12 +252,21 @@ struct InstanceListView: View { instance.isDefault = true defaultAllInstances = false } - do { - try modelContext.save() - } catch { - assertionFailure("SwiftData save failed: \(error)") + if !persist() { + defaultAllInstances = previousDefaultAll } } + + /// Saves pending changes, surfacing a failure to the user. Returns false when the + /// save failed and the context was rolled back. + @discardableResult + private func persist() -> Bool { + guard let failure = modelContext.saveOrRollback() else { return true } + errorTitle = Self.saveFailedTitle + errorMessage = failure + showError = true + return false + } } #if DEBUG diff --git a/Forji/Forji/Views/SettingsTabView.swift b/Forji/Forji/Views/SettingsTabView.swift index 579b89c..94270e2 100644 --- a/Forji/Forji/Views/SettingsTabView.swift +++ b/Forji/Forji/Views/SettingsTabView.swift @@ -149,6 +149,8 @@ struct MergedSettingsTabView: View { @AppStorage("defaultAllInstances") private var defaultAllInstances = false @State private var showClearCacheConfirmation = false @State private var showClearCacheSuccess = false + @State private var saveErrorMessage: String? + @State private var showSaveError = false @Environment(\.modelContext) private var modelContext @Query private var allInstances: [ForgejoInstance] let manager: MultiInstanceManager @@ -215,7 +217,13 @@ struct MergedSettingsTabView: View { .onChange(of: defaultAllInstances) { _, isOn in if isOn { ForgejoInstance.clearDefaults(in: allInstances) - try? modelContext.save() + if let failure = modelContext.saveOrRollback() { + // The stored per-instance defaults survived the rollback, + // so the toggle has to go back to match them. + saveErrorMessage = failure + showSaveError = true + defaultAllInstances = false + } } } } footer: { @@ -256,6 +264,7 @@ struct MergedSettingsTabView: View { AboutSection() } .navigationTitle("Settings") + .errorAlert("Save Failed", message: $saveErrorMessage, isPresented: $showSaveError) } private func clearCache() { diff --git a/Forji/ForjiTests/PersistenceSaveTests.swift b/Forji/ForjiTests/PersistenceSaveTests.swift new file mode 100644 index 0000000..cbc5318 --- /dev/null +++ b/Forji/ForjiTests/PersistenceSaveTests.swift @@ -0,0 +1,92 @@ +import Foundation +import SwiftData +import Testing +@testable import Forji + +@MainActor +struct PersistenceSaveTests { + private static let saveFailure = NSError( + domain: "PersistenceSaveTests", + code: 1, + userInfo: [NSLocalizedDescriptionKey: "the disk is full"], + ) + + private func makeContext() throws -> ModelContext { + let container = try ModelContainer( + for: ForgejoInstance.self, + configurations: ModelConfiguration(isStoredInMemoryOnly: true), + ) + return ModelContext(container) + } + + private func storedInstances(in context: ModelContext) throws -> [ForgejoInstance] { + try context.fetch(FetchDescriptor()) + } + + // MARK: - message + + @Test func messageIncludesTheUnderlyingError() { + let message = PersistenceSave.message(for: Self.saveFailure) + #expect(message.contains("the disk is full")) + } + + // MARK: - success path + + @Test func successfulSaveReportsNoFailureAndPersists() throws { + let context = try makeContext() + context.insert(ForgejoInstance(serverURL: "https://a.com", username: "user", name: "Work")) + + #expect(context.saveOrRollback() == nil) + #expect(try storedInstances(in: context).count == 1) + } + + // MARK: - failure path + + @Test func failedSaveReportsTheFailure() throws { + let context = try makeContext() + + let failure = context.saveOrRollback { throw Self.saveFailure } + + #expect(failure?.contains("the disk is full") == true) + } + + @Test func failedSaveDiscardsAPendingInsert() throws { + // The bug this guards: a failed save used to leave the inserted account in the + // context, so the app showed it as connected until the next launch lost it. + let context = try makeContext() + context.insert(ForgejoInstance(serverURL: "https://a.com", username: "user", name: "Work")) + + #expect(context.saveOrRollback { throw Self.saveFailure } != nil) + + #expect(try storedInstances(in: context).isEmpty) + } + + @Test func failedSaveRestoresAPendingDelete() throws { + let context = try makeContext() + context.insert(ForgejoInstance(serverURL: "https://a.com", username: "user", name: "Work")) + try context.save() + + let instance = try #require(try storedInstances(in: context).first) + context.delete(instance) + + #expect(context.saveOrRollback { throw Self.saveFailure } != nil) + + #expect(try storedInstances(in: context).count == 1) + } + + @Test func failedSaveRestoresEditedValues() throws { + let context = try makeContext() + context.insert(ForgejoInstance(serverURL: "https://a.com", username: "user", name: "Work")) + try context.save() + + let instance = try #require(try storedInstances(in: context).first) + instance.name = "Renamed" + instance.isDefault = true + + #expect(context.saveOrRollback { throw Self.saveFailure } != nil) + + let restored = try #require(try storedInstances(in: context).first) + #expect(restored.name == "Work") + #expect(!restored.isDefault) + } +}