From 0372d863b05ccf9b6b6c8b716536d81f9e0fa563 Mon Sep 17 00:00:00 2001 From: Akhil <11626756+buggerman@users.noreply.github.com> Date: Sun, 26 Apr 2026 23:07:45 +0200 Subject: [PATCH] Make ServerStore instance-based so tests can't poison production servers.json --- Sources/Brygga/BryggaApp.swift | 2 +- Sources/BryggaCore/Models/AppState.swift | 15 +++- .../BryggaCore/Persistence/ServerStore.swift | 79 ++++++++++++------- Tests/AppStateNavigationTests.swift | 40 +++++++++- 4 files changed, 101 insertions(+), 35 deletions(-) diff --git a/Sources/Brygga/BryggaApp.swift b/Sources/Brygga/BryggaApp.swift index 232a2f2..4d44e68 100644 --- a/Sources/Brygga/BryggaApp.swift +++ b/Sources/Brygga/BryggaApp.swift @@ -30,7 +30,7 @@ final class BryggaAppDelegate: NSObject, NSApplicationDelegate { @main struct BryggaApp: App { @NSApplicationDelegateAdaptor(BryggaAppDelegate.self) private var appDelegate - @State private var appState = AppState() + @State private var appState = AppState(store: .shared) var body: some Scene { Window("Brygga", id: "main") { diff --git a/Sources/BryggaCore/Models/AppState.swift b/Sources/BryggaCore/Models/AppState.swift index 2f13f58..0dd1759 100644 --- a/Sources/BryggaCore/Models/AppState.swift +++ b/Sources/BryggaCore/Models/AppState.swift @@ -61,7 +61,16 @@ public final class AppState { /// is gated at the call site via `PreferencesKeys.linkPreviewsEnabled`. public let linkPreviews = LinkPreviewStore() - public init() { + /// Backing store for `servers.json`. Production passes + /// `ServerStore.shared`; tests pass a `ServerStore(root: tempDir)` so + /// they never overwrite the user's real config. There is **no** + /// `AppState()` default — every caller must be explicit, which makes + /// "I forgot to use a temp store" a compile error rather than a silent + /// production data wipe. + private let store: ServerStore + + public init(store: ServerStore) { + self.store = store restoreFromStore() requestNotificationPermission() } @@ -120,7 +129,7 @@ public final class AppState { private var isRestoring: Bool = false private func restoreFromStore() { - let snapshot = ServerStore.load() + let snapshot = store.load() guard !snapshot.servers.isEmpty else { return } isRestoring = true @@ -280,7 +289,7 @@ public final class AppState { private func persist() { guard !isRestoring else { return } - ServerStore.save(snapshot()) + store.save(snapshot()) } /// Close a private-message tab. Removes the query channel from its diff --git a/Sources/BryggaCore/Persistence/ServerStore.swift b/Sources/BryggaCore/Persistence/ServerStore.swift index faf45e1..ca7619a 100644 --- a/Sources/BryggaCore/Persistence/ServerStore.swift +++ b/Sources/BryggaCore/Persistence/ServerStore.swift @@ -3,10 +3,55 @@ import Foundation -/// Loads and saves the list of configured servers to -/// `~/Library/Application Support/Brygga/servers.json`. -public enum ServerStore { - public struct ServerConfig: Codable, Equatable { +/// Loads and saves the list of configured servers as JSON. The default +/// instance reads/writes `~/Library/Application Support/Brygga/servers.json`; +/// tests construct their own `ServerStore(root: tempDir)` so they never touch +/// the user's real config. `AppState` requires a store to be passed in +/// explicitly — there is no `AppState()` default — so a test that forgets +/// to inject a temp store is a compile error rather than a silent prod +/// overwrite. +public final class ServerStore: Sendable { + /// Default singleton, anchored at the production path. Used by the app + /// executable; tests must construct their own instance. + public static let shared = ServerStore() + + private let url: URL + + /// `root` is the directory that holds `servers.json`. `nil` resolves to + /// the standard `~/Library/Application Support/Brygga/` location. + public init(root: URL? = nil) { + let dir: URL = if let root { + root + } else { + (FileManager.default.urls(for: .applicationSupportDirectory, in: .userDomainMask).first + ?? URL(fileURLWithPath: NSHomeDirectory()).appendingPathComponent("Library/Application Support")) + .appendingPathComponent("Brygga", isDirectory: true) + } + try? FileManager.default.createDirectory(at: dir, withIntermediateDirectories: true) + url = dir.appendingPathComponent("servers.json") + } + + /// Absolute path of the backing JSON file. Useful for diagnostics and + /// for the recovery utility. + public var fileURL: URL { + url + } + + public func load() -> Snapshot { + guard let data = try? Data(contentsOf: url) else { + return Snapshot(servers: []) + } + return (try? JSONDecoder().decode(Snapshot.self, from: data)) ?? Snapshot(servers: []) + } + + public func save(_ snapshot: Snapshot) { + let encoder = JSONEncoder() + encoder.outputFormatting = [.prettyPrinted, .sortedKeys] + guard let data = try? encoder.encode(snapshot) else { return } + try? data.write(to: url, options: .atomic) + } + + public struct ServerConfig: Codable, Equatable, Sendable { /// Stable identifier for this server across launches. Keys the /// scrollback directory on disk (`scrollback//.log`) /// so message history survives relaunches. `nil` when migrating @@ -152,31 +197,7 @@ public enum ServerStore { } } - public struct Snapshot: Codable, Equatable { + public struct Snapshot: Codable, Equatable, Sendable { public var servers: [ServerConfig] } - - public static func fileURL() -> URL { - let base = FileManager.default.urls(for: .applicationSupportDirectory, in: .userDomainMask).first - ?? URL(fileURLWithPath: NSHomeDirectory()).appendingPathComponent("Library/Application Support") - let dir = base.appendingPathComponent("Brygga", isDirectory: true) - try? FileManager.default.createDirectory(at: dir, withIntermediateDirectories: true) - return dir.appendingPathComponent("servers.json") - } - - public static func load() -> Snapshot { - let url = fileURL() - guard let data = try? Data(contentsOf: url) else { - return Snapshot(servers: []) - } - return (try? JSONDecoder().decode(Snapshot.self, from: data)) ?? Snapshot(servers: []) - } - - public static func save(_ snapshot: Snapshot) { - let url = fileURL() - let encoder = JSONEncoder() - encoder.outputFormatting = [.prettyPrinted, .sortedKeys] - guard let data = try? encoder.encode(snapshot) else { return } - try? data.write(to: url, options: .atomic) - } } diff --git a/Tests/AppStateNavigationTests.swift b/Tests/AppStateNavigationTests.swift index 4f0d338..34a455d 100644 --- a/Tests/AppStateNavigationTests.swift +++ b/Tests/AppStateNavigationTests.swift @@ -6,8 +6,19 @@ import XCTest @MainActor final class AppStateNavigationTests: XCTestCase { + /// Build a `ServerStore` rooted at a unique temp directory. Critical + /// for isolation: the production singleton would share the user's real + /// `~/Library/Application Support/Brygga/servers.json`, and any test + /// mutation that triggers `persist()` (e.g. `closePrivateMessage`) + /// would overwrite that file with the test fixture data. + private func makeStore() -> ServerStore { + let dir = FileManager.default.temporaryDirectory + .appendingPathComponent("BryggaTests-\(UUID().uuidString)", isDirectory: true) + return ServerStore(root: dir) + } + private func makeFixture() -> AppState { - let state = AppState() + let state = AppState(store: makeStore()) state.servers.removeAll() state.selection = nil let s1 = Server(name: "ServerA", host: "a.example.org", nickname: "me") @@ -84,10 +95,35 @@ final class AppStateNavigationTests: XCTestCase { } func testEmptyStateIsNoOp() { - let state = AppState() + let state = AppState(store: makeStore()) state.servers.removeAll() state.selection = nil state.selectAdjacentChannel(direction: 1) XCTAssertNil(state.selection) } + + /// Regression for the test-pollution bug: a test that triggers + /// `persist()` must write to the injected store's path, not to the + /// production `~/Library/Application Support/Brygga/servers.json`. + func testPersistWritesToInjectedStoreNotProduction() { + let dir = FileManager.default.temporaryDirectory + .appendingPathComponent("BryggaTests-\(UUID().uuidString)", isDirectory: true) + let store = ServerStore(root: dir) + let state = AppState(store: store) + state.servers.removeAll() + + let pm = Channel(name: "carol") + let owner = Server(name: "Test", host: "irc.example.org", nickname: "me") + owner.channels = [pm] + state.servers = [owner] + state.selection = pm.id + state.closePrivateMessage(channelID: pm.id) + + // File exists under the injected store's path... + XCTAssertTrue(FileManager.default.fileExists(atPath: store.fileURL.path)) + // ...and contains the post-mutation snapshot. + let snapshot = store.load() + XCTAssertEqual(snapshot.servers.count, 1) + XCTAssertEqual(snapshot.servers.first?.openQueries, []) + } }