From 6bda0cc1a23b62cf7ad9d2c5e99d13b3ff3e226f Mon Sep 17 00:00:00 2001 From: Adron Hall Date: Sun, 2 Aug 2026 14:27:50 -0700 Subject: [PATCH] feat(errors): friendly loading-error messages + rotating debug log MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Loading failures surfaced technical text straight from APIError (e.g. "Decoding ListRowDTO failed: …"). APIError.errorDescription now returns a new userFacingMessage — friendly copy for the client-only cases (decoding/transport/bare status), with server-written 4xx messages preserved verbatim. The technical form stays in .description for logs, so every existing error banner improves with no view edits. Adds AppLog (facade over os.Logger) + a rotating FileLog that writes to Library/Logs/InterlinedList/interlinedlist.log inside the app container. APIClient logs the full technical cause — request path plus the complete DecodingError coding path — at each decode/transport/ non-2xx failure; the user only ever sees the friendly banner. Tests: APIError user-facing vs. technical split, FileLog write + rotation, and a test-isolation guard so unit runs never touch the real ~/Library. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../InterlinedKit/APIClient/APIClient.swift | 27 +-- .../InterlinedKit/Errors/APIError.swift | 40 ++++- .../InterlinedKit/Logging/AppLog.swift | 161 ++++++++++++++++++ .../InterlinedKitTests/APIErrorTests.swift | 43 +++++ .../InterlinedKitTests/FileLogTests.swift | 77 +++++++++ 5 files changed, 334 insertions(+), 14 deletions(-) create mode 100644 Packages/InterlinedKit/Sources/InterlinedKit/Logging/AppLog.swift create mode 100644 Packages/InterlinedKit/Tests/InterlinedKitTests/FileLogTests.swift diff --git a/Packages/InterlinedKit/Sources/InterlinedKit/APIClient/APIClient.swift b/Packages/InterlinedKit/Sources/InterlinedKit/APIClient/APIClient.swift index 22484f6..053d4d3 100644 --- a/Packages/InterlinedKit/Sources/InterlinedKit/APIClient/APIClient.swift +++ b/Packages/InterlinedKit/Sources/InterlinedKit/APIClient/APIClient.swift @@ -1,5 +1,4 @@ import Foundation -import os // MARK: - APIClientProtocol @@ -83,7 +82,7 @@ public final class APIClient: APIClientProtocol { private let decoder: JSONDecoder private let encoder: JSONEncoder private let retryPolicy: RetryPolicy - private let logger: Logger + private let appLog: AppLog public init( baseURL: URL = URL(string: "https://interlinedlist.com")!, @@ -99,10 +98,7 @@ public final class APIClient: APIClientProtocol { self.decoder = decoder self.encoder = encoder self.retryPolicy = retryPolicy - self.logger = Logger( - subsystem: Bundle.main.bundleIdentifier ?? "com.interlinedlist.kit", - category: "APIClient" - ) + self.appLog = AppLog(category: "APIClient") } // MARK: APIClientProtocol @@ -114,9 +110,13 @@ public final class APIClient: APIClientProtocol { do { return try decoder.decode(Response.self, from: data) } catch { + // Log the full decoder detail (coding path / key) with the request + // path — the user only ever sees `APIError.userFacingMessage`. + let detail = String(reflecting: error) + appLog.error("Decode failed [\(request.path)] type=\(String(describing: Response.self)): \(detail)") throw APIError.decoding( type: String(describing: Response.self), - message: error.localizedDescription + message: detail ) } } @@ -141,9 +141,13 @@ public final class APIClient: APIClientProtocol { // that is the correct "no limit on this route" signal. return (decoded, RateLimitInfo.parse(from: response)) } catch { + // Log the full decoder detail (coding path / key) with the request + // path — the user only ever sees `APIError.userFacingMessage`. + let detail = String(reflecting: error) + appLog.error("Decode failed [\(request.path)] type=\(String(describing: Response.self)): \(detail)") throw APIError.decoding( type: String(describing: Response.self), - message: error.localizedDescription + message: detail ) } } @@ -166,7 +170,7 @@ public final class APIClient: APIClientProtocol { // transparently try once via the session transport before we // give up. This catches future API drift in either direction. if case .unauthorized = error, request.auth == .bearer { - logger.warning("Bearer request returned 401 — retrying via session transport") + appLog.warning("Bearer request returned 401 [\(request.path)] — retrying via session transport") return try await performWithRetry(request, forceSession: true) } throw error @@ -220,13 +224,16 @@ public final class APIClient: APIClientProtocol { // and be indistinguishable from a genuine transport failure). throw CancellationError() } catch { + appLog.error("Transport failed [\(request.path)]: \(String(reflecting: error))") throw APIError.transport(message: error.localizedDescription) } guard (200..<300).contains(response.statusCode) else { + let serverMessage = decodeServerMessage(from: data) + appLog.notice("HTTP \(response.statusCode) [\(request.path)]: \(serverMessage ?? "no server message")") throw APIError.from( statusCode: response.statusCode, - serverMessage: decodeServerMessage(from: data), + serverMessage: serverMessage, retryAfter: parseRetryAfter(response.value(forHTTPHeaderField: "Retry-After")) ) } diff --git a/Packages/InterlinedKit/Sources/InterlinedKit/Errors/APIError.swift b/Packages/InterlinedKit/Sources/InterlinedKit/Errors/APIError.swift index d149985..967ee37 100644 --- a/Packages/InterlinedKit/Sources/InterlinedKit/Errors/APIError.swift +++ b/Packages/InterlinedKit/Sources/InterlinedKit/Errors/APIError.swift @@ -81,11 +81,43 @@ extension APIError: Equatable { } extension APIError: LocalizedError, CustomStringConvertible { - /// The server-supplied human message if one is available, otherwise a - /// concise developer-facing description of the case. Suitable for both - /// `NSAlert` body text and `os.Logger` output. - public var errorDescription: String? { description } + /// The message shown to the user. Points at `userFacingMessage` so that + /// anything rendering `error.localizedDescription` (the whole app does) + /// gets friendly, non-technical copy. The technical form lives in + /// `description` and is what goes to the debug log. + public var errorDescription: String? { userFacingMessage } + /// A friendly, non-technical message safe to show in a loading/error UI. + /// + /// Server-supplied messages (400/403/404/429) are already human-written on + /// InterlinedList and are preserved verbatim. The client-only cases + /// (`decoding`, `transport`) and bare status codes — whose `description` + /// is developer jargon like "Decoding ListRowDTO failed: …" — get a + /// generic, reassuring message instead. The real cause is captured in the + /// debug log via `description`. + public var userFacingMessage: String { + switch self { + case .transport: + return "Can’t reach InterlinedList. Check your internet connection and try again." + case .decoding: + return "InterlinedList sent back something we couldn’t read. Please try again in a moment." + case .unauthorized(let message): + return message ?? "Your session has expired. Please sign in again." + case .forbidden(let message): + return message ?? "You don’t have permission to do that." + case .notFound(let message): + return message ?? "We couldn’t find what you were looking for." + case .badRequest(let message): + return message ?? "That request couldn’t be completed. Please check your input and try again." + case .rateLimited(let message, _): + return message ?? "You’re doing that a little too quickly. Please wait a moment and try again." + case .httpStatus(_, let message): + return message ?? "Something went wrong. Please try again." + } + } + + /// The technical description — developer jargon, request/decoder detail — + /// used for `os.Logger` and the debug-log file. Never shown to the user. public var description: String { switch self { case .transport(let message): diff --git a/Packages/InterlinedKit/Sources/InterlinedKit/Logging/AppLog.swift b/Packages/InterlinedKit/Sources/InterlinedKit/Logging/AppLog.swift new file mode 100644 index 0000000..300f17f --- /dev/null +++ b/Packages/InterlinedKit/Sources/InterlinedKit/Logging/AppLog.swift @@ -0,0 +1,161 @@ +import Foundation +import os + +/// App-wide logging facade. +/// +/// Every call mirrors to Apple's unified logging (visible live in Console.app +/// and `log stream`) **and** appends to a rotating file under the app's +/// container so a user can send it for debugging later. Under the sandbox the +/// file lives at: +/// +/// ~/Library/Containers//Data/Library/Logs/InterlinedList/interlinedlist.log +/// +/// The unified-logging side keeps the existing on-device behaviour; the file +/// side is the new, retrievable artifact. User-facing UI text is produced +/// separately (see `APIError.userFacingMessage`) — the log holds the full +/// technical detail, the UI shows the friendly message. +public struct AppLog: Sendable { + + /// Log severity. Mirrors the `os.Logger` levels we actually use. + public enum Level: String, Sendable { + case debug, info, notice, warning, error, fault + } + + /// The unified-logging subsystem shared across the app and its packages. + public static let subsystem = "com.interlinedlist.macos" + + private let category: String + private let osLogger: Logger + private let file: FileLog + + /// - Parameters: + /// - category: groups related messages (e.g. `"APIClient"`). + /// - file: the file sink. Defaults to the process-wide `.shared` log so + /// every category writes to the same file; injectable for tests. + public init(category: String, file: FileLog = .shared) { + self.category = category + self.osLogger = Logger(subsystem: AppLog.subsystem, category: category) + self.file = file + } + + public func error(_ message: @autoclosure () -> String) { log(.error, message()) } + public func warning(_ message: @autoclosure () -> String) { log(.warning, message()) } + public func notice(_ message: @autoclosure () -> String) { log(.notice, message()) } + public func info(_ message: @autoclosure () -> String) { log(.info, message()) } + public func debug(_ message: @autoclosure () -> String) { log(.debug, message()) } + + public func log(_ level: Level, _ message: String) { + // `.public` privacy: these strings are already scrubbed of user data + // by the callers (we log error *structure*, not response bodies). + switch level { + case .debug: osLogger.debug("\(message, privacy: .public)") + case .info: osLogger.info("\(message, privacy: .public)") + case .notice: osLogger.notice("\(message, privacy: .public)") + case .warning: osLogger.warning("\(message, privacy: .public)") + case .error: osLogger.error("\(message, privacy: .public)") + case .fault: osLogger.fault("\(message, privacy: .public)") + } + file.append(level: level, category: category, message: message) + } +} + +/// The rotating file sink behind `AppLog`. +/// +/// Thread-safety is provided by a private serial queue; writes are +/// fire-and-forget so logging never blocks the caller. `@unchecked Sendable` +/// is sound because every stored property is immutable and the only mutable +/// state (the file on disk, the shared date formatter) is touched solely on +/// `queue`. +public final class FileLog: @unchecked Sendable { + + /// The process-wide log used by `AppLog` when no sink is injected. + public static let shared = FileLog() + + private let queue = DispatchQueue(label: "com.interlinedlist.filelog") + private let fileURL: URL? + private let maxBytes: Int + private let formatter: ISO8601DateFormatter + + /// - Parameters: + /// - directory: where the log file is written. `nil` disables file + /// logging entirely (every `append` becomes a no-op) — used under + /// XCTest so unit runs never touch the real `~/Library`. + /// - fileName: the active log file's name. + /// - maxBytes: rotate once the active file reaches this size. One + /// previous generation is kept as `.1`. + public init( + directory: URL? = FileLog.defaultDirectory(), + fileName: String = "interlinedlist.log", + maxBytes: Int = 5 * 1024 * 1024 + ) { + self.maxBytes = maxBytes + let formatter = ISO8601DateFormatter() + formatter.formatOptions = [.withInternetDateTime, .withFractionalSeconds] + self.formatter = formatter + + if let directory { + try? FileManager.default.createDirectory( + at: directory, + withIntermediateDirectories: true + ) + self.fileURL = directory.appendingPathComponent(fileName) + } else { + self.fileURL = nil + } + } + + /// The app's log directory inside the (sandbox) container's `Library/Logs`, + /// or `nil` when running under XCTest so tests don't write to real Library. + public static func defaultDirectory() -> URL? { + if isRunningUnderTests { return nil } + guard let library = FileManager.default + .urls(for: .libraryDirectory, in: .userDomainMask).first else { return nil } + return library.appendingPathComponent("Logs/InterlinedList", isDirectory: true) + } + + /// True inside a unit-test host. `XCTestConfigurationFilePath` covers Xcode; + /// SwiftPM's `swift test` doesn't set it, so also sniff the loaded XCTest + /// runtime (never linked into the shipping app). + private static var isRunningUnderTests: Bool { + ProcessInfo.processInfo.environment["XCTestConfigurationFilePath"] != nil + || NSClassFromString("XCTestCase") != nil + } + + /// The active log file on disk, if file logging is enabled. Useful for a + /// future "Export Logs…" affordance. + public var currentFileURL: URL? { fileURL } + + /// Appends one line: ` [LEVEL] : `. + public func append(level: AppLog.Level, category: String, message: String) { + guard let fileURL else { return } + let now = Date() + queue.async { + self.rotateIfNeeded(fileURL: fileURL) + let line = "\(self.formatter.string(from: now)) " + + "[\(level.rawValue.uppercased())] \(category): \(message)\n" + guard let data = line.data(using: .utf8) else { return } + if let handle = try? FileHandle(forWritingTo: fileURL) { + defer { try? handle.close() } + _ = try? handle.seekToEnd() + try? handle.write(contentsOf: data) + } else { + // File doesn't exist yet (first write, or just rotated). + try? data.write(to: fileURL, options: .atomic) + } + } + } + + /// Rotates `interlinedlist.log` → `interlinedlist.log.1` once it grows past + /// `maxBytes`, discarding any older generation. Must run on `queue`. + private func rotateIfNeeded(fileURL: URL) { + let fm = FileManager.default + guard + let attributes = try? fm.attributesOfItem(atPath: fileURL.path), + let size = attributes[.size] as? Int, + size >= maxBytes + else { return } + let backup = fileURL.appendingPathExtension("1") + try? fm.removeItem(at: backup) + try? fm.moveItem(at: fileURL, to: backup) + } +} diff --git a/Packages/InterlinedKit/Tests/InterlinedKitTests/APIErrorTests.swift b/Packages/InterlinedKit/Tests/InterlinedKitTests/APIErrorTests.swift index c0bf380..93fb7e1 100644 --- a/Packages/InterlinedKit/Tests/InterlinedKitTests/APIErrorTests.swift +++ b/Packages/InterlinedKit/Tests/InterlinedKitTests/APIErrorTests.swift @@ -90,6 +90,49 @@ final class APIErrorTests: XCTestCase { XCTAssertEqual(error.description, "Unauthorized") } + // MARK: - User-facing vs. technical messages + + func test_givenDecodingError_whenUserFacing_thenHidesTechnicalDetail() { + let error = APIError.decoding(type: "ListRowDTO", message: "keyNotFound(CodingKeys(stringValue: \"id\"))") + + // The UI (localizedDescription → errorDescription → userFacingMessage) + // must not leak the decoder jargon… + XCTAssertFalse(error.userFacingMessage.contains("ListRowDTO")) + XCTAssertFalse(error.userFacingMessage.lowercased().contains("decoding")) + XCTAssertFalse(error.userFacingMessage.lowercased().contains("keynotfound")) + XCTAssertEqual(error.errorDescription, error.userFacingMessage) + + // …but the technical form (for the debug log) still carries it. + XCTAssertTrue(error.description.contains("ListRowDTO")) + XCTAssertTrue(error.description.contains("keyNotFound")) + } + + func test_givenTransportError_whenUserFacing_thenIsFriendlyConnectionMessage() { + let error = APIError.transport(message: "The request timed out.") + XCTAssertFalse(error.userFacingMessage.lowercased().contains("network error")) + XCTAssertTrue(error.userFacingMessage.contains("connection")) + // Technical detail preserved for the log. + XCTAssertEqual(error.description, "Network error: The request timed out.") + } + + func test_givenServerMessage_whenUserFacing_thenPreservesItVerbatim() { + // 4xx server messages are already human-written — keep them. + XCTAssertEqual( + APIError.forbidden(serverMessage: "Email not verified").userFacingMessage, + "Email not verified" + ) + XCTAssertEqual( + APIError.badRequest(serverMessage: "Name is required").userFacingMessage, + "Name is required" + ) + } + + func test_givenStatusWithoutServerMessage_whenUserFacing_thenFriendlyFallback() { + let error = APIError.httpStatus(code: 500, serverMessage: nil) + XCTAssertFalse(error.userFacingMessage.contains("500")) + XCTAssertEqual(error.description, "HTTP 500") + } + // MARK: - APIErrorBody decoding func test_givenErrorBodyJSON_whenDecoded_thenExtractsMessage() throws { diff --git a/Packages/InterlinedKit/Tests/InterlinedKitTests/FileLogTests.swift b/Packages/InterlinedKit/Tests/InterlinedKitTests/FileLogTests.swift new file mode 100644 index 0000000..d1c3262 --- /dev/null +++ b/Packages/InterlinedKit/Tests/InterlinedKitTests/FileLogTests.swift @@ -0,0 +1,77 @@ +import XCTest +@testable import InterlinedKit + +final class FileLogTests: XCTestCase { + + private var tempDir: URL! + + override func setUpWithError() throws { + tempDir = FileManager.default.temporaryDirectory + .appendingPathComponent("FileLogTests-\(UUID().uuidString)", isDirectory: true) + } + + override func tearDownWithError() throws { + try? FileManager.default.removeItem(at: tempDir) + } + + func test_givenAppend_whenFlushed_thenLineIsWrittenToFile() throws { + let log = FileLog(directory: tempDir, fileName: "test.log") + let url = try XCTUnwrap(log.currentFileURL) + + log.append(level: .error, category: "APIClient", message: "Decode failed [/api/lists]") + + let contents = try waitForFileContents(at: url, containing: "Decode failed") + XCTAssertTrue(contents.contains("[ERROR]")) + XCTAssertTrue(contents.contains("APIClient:")) + XCTAssertTrue(contents.contains("Decode failed [/api/lists]")) + } + + func test_givenDisabledDirectory_whenAppend_thenNoOp() throws { + // nil directory (the XCTest / no-Library case) must never throw or write. + let log = FileLog(directory: nil) + XCTAssertNil(log.currentFileURL) + log.append(level: .error, category: "APIClient", message: "should be dropped") + // Nothing to assert beyond "did not crash"; the log is a no-op sink. + } + + func test_givenFileOverMaxBytes_whenAppend_thenRotatesToBackup() throws { + // Tiny cap so a couple of writes trip rotation deterministically. + let log = FileLog(directory: tempDir, fileName: "test.log", maxBytes: 64) + let url = try XCTUnwrap(log.currentFileURL) + let backup = url.appendingPathExtension("1") + + // First write creates the active file and pushes it past the cap. + log.append(level: .info, category: "T", message: String(repeating: "x", count: 128)) + _ = try waitForFileContents(at: url, containing: "xxxx") + + // Second write sees the oversized active file and rotates it to `.1` + // before writing the fresh line. + log.append(level: .info, category: "T", message: "after-rotation") + let active = try waitForFileContents(at: url, containing: "after-rotation") + + XCTAssertTrue(FileManager.default.fileExists(atPath: backup.path), + "previous generation should be preserved as test.log.1") + XCTAssertFalse(active.contains(String(repeating: "x", count: 128)), + "rotated content should no longer be in the active file") + } + + // MARK: - Helpers + + /// Writes are dispatched asynchronously onto the logger's serial queue, so + /// poll briefly until the file contains the expected marker. + private func waitForFileContents( + at url: URL, + containing marker: String, + timeout: TimeInterval = 2.0 + ) throws -> String { + let deadline = Date().addingTimeInterval(timeout) + while Date() < deadline { + if let data = try? Data(contentsOf: url) { + let text = String(decoding: data, as: UTF8.self) + if text.contains(marker) { return text } + } + Thread.sleep(forTimeInterval: 0.02) + } + return try String(contentsOf: url, encoding: .utf8) + } +}