From 3da3d3ce5e72e9a00c7db9cfa0a93c672bbb67b5 Mon Sep 17 00:00:00 2001 From: Alan Wizemann Date: Sat, 25 Apr 2026 07:50:52 +0200 Subject: [PATCH] fix(ios-rootmodel): surface store failures (A.3 + A.4 bundled) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bundled because the fixes are coherent — they all add the same mechanism (`lastError` + `os.Logger`) to the same model. A.3 — Distinguish "no servers" from "Keychain unreachable": - `RootModel.connect(to:)` previously used `try?` on `keyStore.load(for:)`. A biometric cancel or device-locked Keychain read returned nil → the app dropped the user into fresh onboarding, destroying the existing server's host/user/port. Now we catch the throw, log via os.Logger, set `lastError`, and stay on `.serverList`. The user sees a banner + Dismiss button instead of being kicked back to onboarding. - `RootModel.load()` now logs the corrupted-blob path via os.Logger and sets `lastError` before falling through to onboarding (recovery is necessary, but the user gets context now). A.4 — Surface delete failures in `forget()` and `disconnect()`: - Both used `try?` on every store delete. On partial failure the in-memory dict was wiped while orphan Keychain entries lingered. Now each delete is `do/catch` with logging, failures collected into `lastError`. The in-memory state is reloaded from disk so it tracks what's actually persisted (covers the partial-failure case). ServerListView gains an inline error banner above the list that reads `model.lastError`, with a Dismiss button calling `clearLastError()`. Verified: iOS build succeeds. Co-Authored-By: Claude Opus 4.7 (1M context) --- scarf/Scarf iOS/App/ScarfIOSApp.swift | 113 ++++++++++++++++--- scarf/Scarf iOS/Servers/ServerListView.swift | 20 ++++ 2 files changed, 117 insertions(+), 16 deletions(-) diff --git a/scarf/Scarf iOS/App/ScarfIOSApp.swift b/scarf/Scarf iOS/App/ScarfIOSApp.swift index 9b3a974..bbb371e 100644 --- a/scarf/Scarf iOS/App/ScarfIOSApp.swift +++ b/scarf/Scarf iOS/App/ScarfIOSApp.swift @@ -1,6 +1,7 @@ import SwiftUI import ScarfCore import ScarfIOS +import os /// App entry point. Renders a single `WindowGroup` whose root decides /// between onboarding and the connected-app surface based on whether @@ -116,19 +117,37 @@ final class RootModel { /// having to re-query stores on every re-render. private(set) var servers: [ServerID: IOSServerConfig] = [:] + /// Most recent non-fatal failure surfaced from RootModel operations + /// (load, connect, forget). The ServerListView renders a banner above + /// the list when this is non-nil with a Retry/Dismiss affordance. + /// `nil` after a successful op so stale errors don't linger. + var lastError: String? + private let keyStore: any SSHKeyStore private let configStore: any IOSServerConfigStore + private static let logger = Logger( + subsystem: "com.scarf.ios", + category: "RootModel" + ) + init(keyStore: any SSHKeyStore, configStore: any IOSServerConfigStore) { self.keyStore = keyStore self.configStore = configStore } + /// Clear the surfaced error. Called by the ServerListView banner's + /// Dismiss button. + func clearLastError() { + lastError = nil + } + /// Load configured servers from disk and pick an initial state. func load() async { do { let all = try await configStore.listAll() servers = all + lastError = nil if all.isEmpty { // Fresh install or user forgot every server → go // straight to onboarding with a new ID reserved so @@ -138,7 +157,14 @@ final class RootModel { state = .serverList } } catch { + // configStore is UserDefaults-backed; failures here are + // exceptional (corrupted v2 blob, JSONDecoder error). Surface + // the error to the user but recover into onboarding so they + // aren't permanently locked out of the app — the state is + // unsalvageable, the user needs to re-onboard anyway. + Self.logger.error("RootModel.load failed: \(error.localizedDescription, privacy: .public)") servers = [:] + lastError = "Couldn't load saved servers (\(error.localizedDescription)). Starting fresh." state = .onboarding(forNewServer: ServerID()) } } @@ -179,18 +205,33 @@ final class RootModel { /// Tap a server row → connect. Loads fresh from disk to catch any /// edits made through the Mac app (or future multi-device scenarios). func connect(to id: ServerID) async { - var diskConfig: IOSServerConfig? = servers[id] - if diskConfig == nil { - diskConfig = try? await configStore.load(id: id) + do { + var diskConfig: IOSServerConfig? = servers[id] + if diskConfig == nil { + diskConfig = try await configStore.load(id: id) + } + let diskKey: SSHKeyBundle? = try await keyStore.load(for: id) + guard let config = diskConfig, let key = diskKey else { + // Genuine "no row" / "no key" — preserve the pre-A.3 + // behaviour: re-onboard under this ID so the user keeps + // host/user/port and just regenerates the key. + state = .onboarding(forNewServer: id) + return + } + lastError = nil + state = .connected(id, config, key) + } catch { + // Transient Keychain errors (biometric cancel, device + // locked, OS-level Keychain corruption) used to drop the + // user into fresh onboarding — destroying useful state. + // Now we keep them on the server list with a banner so + // they can retry once the Keychain is reachable again. + Self.logger.error( + "RootModel.connect failed for \(id, privacy: .public): \(error.localizedDescription, privacy: .public)" + ) + lastError = "Couldn't unlock server credentials: \(error.localizedDescription)" + state = .serverList } - let diskKey: SSHKeyBundle? = try? await keyStore.load(for: id) - guard let config = diskConfig, let key = diskKey else { - // Missing key → force re-onboarding under this ID so the - // user can regenerate without losing host/user/port. - state = .onboarding(forNewServer: id) - return - } - state = .connected(id, config, key) } /// Soft disconnect: return to the server list without wiping @@ -211,20 +252,60 @@ final class RootModel { /// Hard forget: wipe the specified server's key + config, refresh /// the list, transition to serverList (or onboarding if empty). + /// Per-store failures are captured in `lastError` so a partial + /// forget surfaces a banner instead of silently leaving orphans. func forget(id: ServerID) async { - try? await keyStore.delete(for: id) - try? await configStore.delete(id: id) + var failures: [String] = [] + do { + try await keyStore.delete(for: id) + } catch { + Self.logger.error( + "RootModel.forget keyStore.delete failed for \(id, privacy: .public): \(error.localizedDescription, privacy: .public)" + ) + failures.append("Keychain: \(error.localizedDescription)") + } + do { + try await configStore.delete(id: id) + } catch { + Self.logger.error( + "RootModel.forget configStore.delete failed for \(id, privacy: .public): \(error.localizedDescription, privacy: .public)" + ) + failures.append("Config: \(error.localizedDescription)") + } + // Reload from disk so in-memory state reflects what's actually + // persisted — covers the partial-failure case where Keychain + // succeeded but config didn't (or vice versa). servers = (try? await configStore.listAll()) ?? [:] + if failures.isEmpty { + lastError = nil + } else { + lastError = "Couldn't fully forget server: " + failures.joined(separator: "; ") + } state = servers.isEmpty ? .onboarding(forNewServer: ServerID()) : .serverList } /// Legacy v1 "Disconnect" that wipes EVERYTHING. Kept for back-compat /// with any caller that still hits the no-arg path (there shouldn't /// be any after 3.5 lands, but the protocol still supports it). + /// Same partial-failure semantics as `forget(id:)`. func disconnect() async { - try? await keyStore.delete() - try? await configStore.delete() - servers = [:] + var failures: [String] = [] + do { + try await keyStore.delete() + } catch { + Self.logger.error("RootModel.disconnect keyStore.delete failed: \(error.localizedDescription, privacy: .public)") + failures.append("Keychain: \(error.localizedDescription)") + } + do { + try await configStore.delete() + } catch { + Self.logger.error("RootModel.disconnect configStore.delete failed: \(error.localizedDescription, privacy: .public)") + failures.append("Config: \(error.localizedDescription)") + } + servers = (try? await configStore.listAll()) ?? [:] + if !failures.isEmpty { + lastError = "Couldn't fully sign out: " + failures.joined(separator: "; ") + } state = .onboarding(forNewServer: ServerID()) } } diff --git a/scarf/Scarf iOS/Servers/ServerListView.swift b/scarf/Scarf iOS/Servers/ServerListView.swift index 851ccaa..115356e 100644 --- a/scarf/Scarf iOS/Servers/ServerListView.swift +++ b/scarf/Scarf iOS/Servers/ServerListView.swift @@ -19,6 +19,26 @@ struct ServerListView: View { var body: some View { NavigationStack { List { + if let err = model.lastError { + Section { + VStack(alignment: .leading, spacing: 8) { + Label("Something went wrong", systemImage: "exclamationmark.triangle.fill") + .foregroundStyle(.orange) + .font(.headline) + Text(err) + .font(.callout) + .foregroundStyle(.secondary) + .fixedSize(horizontal: false, vertical: true) + HStack(spacing: 12) { + Button("Dismiss") { model.clearLastError() } + .buttonStyle(.bordered) + .controlSize(.small) + } + } + .padding(.vertical, 4) + } + } + Section { ForEach(sortedServers, id: \.id) { row in ServerListRow(row: row) {