fix: surface SwiftData save failures instead of swallowing them

assertionFailure is compiled out under -O, so every catch around
modelContext.save() that only asserted discarded the error entirely in
Release builds. InstanceFormView.handleSave then set currentInstance and
dismissed regardless, reporting a connected account that was never
persisted and would be gone on the next launch.

Add ModelContext.saveOrRollback(), which returns a user-facing message on
failure and rolls the context back so in-memory state matches disk. Views
now show a "Save Failed" alert and skip the follow-on state changes.

Two behaviour fixes beyond error reporting:

- InstanceListView.deleteInstance saves before disconnecting and deleting
  Keychain credentials. A failed save previously left the rolled-back
  instance in the list with its credentials already gone, making it
  unauthenticatable. AuthenticationService.logout already did this.
- Both default toggles revert their @AppStorage flag when the save fails,
  so it cannot disagree with the stored isDefault flags.

ContentView's saves only carry lastUsed, so a failure costs list ordering
rather than account data and stays unsurfaced during automatic restore.

Move Mode, AuthMode and accountAlreadyExists out of InstanceFormView to
keep the file within the 400-line lint limit.

Fixes https://codeberg.org/secana/Forji/issues/92
This commit is contained in:
Stefan Hausotte 2026-08-06 18:39:58 +02:00
parent 1beeb91179
commit 3289fc9eb0
8 changed files with 242 additions and 70 deletions

View file

@ -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
}

View file

@ -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)
}
}
}

View file

@ -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.

View file

@ -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

View file

@ -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
}
}
}

View file

@ -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<MultiInstanceManager?>) {
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

View file

@ -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() {

View file

@ -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<ForgejoInstance>())
}
// 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)
}
}