From 7ed4cd4f40cf6f6384a21d464cd801e74c61a431 Mon Sep 17 00:00:00 2001 From: Kyle Van Essen Date: Sun, 5 Jul 2026 20:44:46 -0400 Subject: [PATCH 1/4] Redesign Foreman settings as a sidebar and add launch-at-login Rework the Foreman Settings window from a single grouped Form into a macOS System-Settings-style NavigationSplitView with three panes: General (launch at login), Repositories (scan directory), and Agent (cursor-agent path). Each editable pane keeps the settings-window "apply on commit, no Save button" behavior and the WindowVisibilityReader re-seed. Add a "Launch Foreman at login" toggle backed by a new ForemanCore LoginItemController that wraps SMAppService.mainApp behind an @_spi(Testing) backend. The OS owns the real state, so the toggle reads live status and re-syncs after each change; ForemanServices.startsAtLogin surfaces failures on issueMessage rather than swallowing them. Launching at login reuses the existing start() restore of enabled workers, so no separate restore path is needed. Co-authored-by: Cursor --- Foreman/Foreman/README.md | 20 +- Foreman/Foreman/Sources/ForemanSession.swift | 14 ++ Foreman/Foreman/Sources/MainWindowView.swift | 2 +- Foreman/Foreman/Sources/SettingsView.swift | 229 +++++++++++++----- Foreman/ForemanCore/AGENTS.md | 10 +- Foreman/ForemanCore/README.md | 11 +- .../ForemanCore/Sources/ForemanServices.swift | 30 +++ .../Sources/LoginItemController.swift | 87 +++++++ .../Tests/ForemanCoreTestSupport.swift | 34 +++ .../Tests/ForemanServicesTests.swift | 40 ++- .../Tests/LoginItemControllerTests.swift | 56 +++++ Foreman/TODOs.md | 5 + 12 files changed, 469 insertions(+), 69 deletions(-) create mode 100644 Foreman/ForemanCore/Sources/LoginItemController.swift create mode 100644 Foreman/ForemanCore/Tests/LoginItemControllerTests.swift diff --git a/Foreman/Foreman/README.md b/Foreman/Foreman/README.md index 86ba5185..d79bdc81 100644 --- a/Foreman/Foreman/README.md +++ b/Foreman/Foreman/README.md @@ -43,14 +43,26 @@ closed. Closing the window just hides it — the app keeps running until Quit. verbose startup logs. Editable while the worker is stopped — options apply on the next start. - **Toolbar** — a "Preventing sleep" badge while the sleep assertion is held, - Rescan, Settings (opens the standard settings window: the scan directory - and an explicit `cursor-agent` path, empty = auto-detect, applied as - fields commit), and Quit. + Rescan, Settings (opens the settings window — see below), and Quit. + +## Settings + +The settings window is a macOS System-Settings-style sidebar with three panes. +There's no Save button — changes apply as you make them (a field commits on +Return / focus change / a folder pick; a toggle applies immediately). + +- **General** — *Launch Foreman at login*. Registers Foreman as a login item + via `SMAppService`, so it starts (and restores your enabled workers) when + you log in. +- **Repositories** — the directory scanned for git repositories (empty = + `~/Development`), with a folder picker. +- **Agent** — an explicit `cursor-agent` executable path (empty = auto-detect). ## Lifecycle - On launch, Foreman restores the saved configuration and restarts the workers - that were enabled last time. + that were enabled last time. With *Launch Foreman at login* on, this happens + automatically after you log in. - Quitting stops every worker (stop-on-quit: the app owns its processes and never leaves orphans). - The repo list refreshes every time the window is opened or focused, and on diff --git a/Foreman/Foreman/Sources/ForemanSession.swift b/Foreman/Foreman/Sources/ForemanSession.swift index aaebcd85..bbf34952 100644 --- a/Foreman/Foreman/Sources/ForemanSession.swift +++ b/Foreman/Foreman/Sources/ForemanSession.swift @@ -37,6 +37,14 @@ final class ForemanSession { services.isAnyWorkerLive } + /// Whether Foreman launches at login. `SettingsView` binds this two-way; + /// the setter registers/unregisters the login item in Core (which logs and + /// surfaces any failure on `issueMessage`). + var startsAtLogin: Bool { + get { services.startsAtLogin } + set { services.startsAtLogin = newValue } + } + init(services: ForemanServices) { self.services = services } @@ -63,4 +71,10 @@ final class ForemanSession { func rescan() { services.rescan() } + + /// Re-reads the login-item status from the OS (it can change in System + /// Settings while Foreman runs). + func refreshLoginItemStatus() { + services.refreshLoginItemStatus() + } } diff --git a/Foreman/Foreman/Sources/MainWindowView.swift b/Foreman/Foreman/Sources/MainWindowView.swift index 148f3d05..d8f981bc 100644 --- a/Foreman/Foreman/Sources/MainWindowView.swift +++ b/Foreman/Foreman/Sources/MainWindowView.swift @@ -89,7 +89,7 @@ struct MainWindowView: View { SettingsLink { Label("Settings", systemImage: "gearshape") } - .help("Change the scan directory or the cursor-agent executable.") + .help("Open Foreman's settings.") } ToolbarItem { Button { diff --git a/Foreman/Foreman/Sources/SettingsView.swift b/Foreman/Foreman/Sources/SettingsView.swift index add945c1..c95fee08 100644 --- a/Foreman/Foreman/Sources/SettingsView.swift +++ b/Foreman/Foreman/Sources/SettingsView.swift @@ -2,33 +2,120 @@ import AppKit import ForemanCore import SwiftUI -/// Global settings: where to look for repositories and which `cursor-agent` -/// to run. Hosted by the `Settings` scene (app menu / Cmd-, / the toolbar's -/// `SettingsLink`), so it behaves like a standard macOS settings window: -/// edits apply on field commit (Return / focus change / panel selection) — -/// no Save button. Values write through the observable `AppSettings`, which -/// persists and rescans in Core. +/// Global settings, laid out like a modern macOS System Settings window: a +/// `NavigationSplitView` sidebar selects one of three panes. +/// +/// - **General** — launch Foreman at login. +/// - **Repositories** — the directory scanned for git repositories. +/// - **Agent** — which `cursor-agent` executable to run. +/// +/// Hosted by the `Settings` scene (app menu / Cmd-, / the toolbar's +/// `SettingsLink`), so it follows settings-window conventions: edits apply on +/// commit (Return / focus change / panel selection / a toggle flip) — no Save +/// button. Values write through the observable Core (`AppSettings` for the +/// paths, `LoginItemController` for the login item), which persists. struct SettingsView: View { - private enum Field { - case scanDirectory - case agentExecutable + /// The sidebar panes. `Identifiable` (via `id: \.self`) so the sidebar + /// `List` can iterate `allCases`. + private enum Pane: String, CaseIterable, Identifiable { + case general + case repositories + case agent + + var id: Self { + self + } + + var title: String { + switch self { + case .general: "General" + case .repositories: "Repositories" + case .agent: "Agent" + } + } + + var symbol: String { + switch self { + case .general: "gearshape" + case .repositories: "folder" + case .agent: "terminal" + } + } } let session: ForemanSession + @State private var selection: Pane? = .general + + var body: some View { + NavigationSplitView { + List(selection: $selection) { + ForEach(Pane.allCases) { pane in + Label(pane.title, systemImage: pane.symbol) + .tag(pane) + } + } + .navigationSplitViewColumnWidth(min: 170, ideal: 190, max: 220) + } detail: { + detail + } + .frame(minWidth: 620, minHeight: 380) + } + + @ViewBuilder + private var detail: some View { + switch selection { + // No selection falls back to General — the sidebar always has a + // meaningful pane on screen. + case .general, .none: + GeneralSettingsView(session: session) + case .repositories: + RepositoriesSettingsView(session: session) + case .agent: + AgentSettingsView(session: session) + } + } +} + +/// General preferences: whether Foreman launches at login. +private struct GeneralSettingsView: View { + @Bindable var session: ForemanSession + + @State private var isWindowVisible = true + + var body: some View { + Form { + Toggle("Launch Foreman at login", isOn: $session.startsAtLogin) + Text( + "Foreman lives in the menu bar. Turn this on to start it automatically when you log in — it will restore any enabled workers.", + ) + .font(.caption) + .foregroundStyle(.secondary) + } + .formStyle(.grouped) + .navigationTitle("General") + // The OS owns the login-item state and the user can change it in + // System Settings, so re-read it whenever this pane comes back on + // screen. The Settings window keeps its hierarchy across opens. + .onAppear { session.refreshLoginItemStatus() } + .background(WindowVisibilityReader(isVisible: $isWindowVisible)) + .onChange(of: isWindowVisible) { _, visible in + if visible { session.refreshLoginItemStatus() } + } + } +} + +/// Repository discovery: where to scan for git repositories. +private struct RepositoriesSettingsView: View { + let session: ForemanSession + @State private var scanDirectory: String - @State private var agentExecutable: String @State private var isWindowVisible = true - @FocusState private var focusedField: Field? + @FocusState private var isFocused: Bool init(session: ForemanSession) { self.session = session - _scanDirectory = State( - initialValue: session.settings.scanDirectory?.path ?? "", - ) - _agentExecutable = State( - initialValue: session.settings.agentExecutable?.path ?? "", - ) + _scanDirectory = State(initialValue: session.settings.scanDirectory?.path ?? "") } var body: some View { @@ -41,12 +128,61 @@ struct SettingsView: View { prompt: Text("~/Development"), ) .labelsHidden() - .focused($focusedField, equals: .scanDirectory) - .onSubmit { applyScanDirectory() } + .focused($isFocused) + .onSubmit { apply() } Button("Choose…") { chooseScanDirectory() } } } + Text("Foreman lists the git repositories directly inside this directory.") + .font(.caption) + .foregroundStyle(.secondary) + } + .formStyle(.grouped) + .navigationTitle("Repositories") + // Tabbing or clicking out of the field commits it, like Return does. + .onChange(of: isFocused) { _, focused in + if !focused { apply() } + } + .background(WindowVisibilityReader(isVisible: $isWindowVisible)) + // The Settings window keeps its hierarchy across opens, so init-time + // drafts go stale; re-seed whenever the window comes back. + .onChange(of: isWindowVisible) { _, visible in + if visible { scanDirectory = session.settings.scanDirectory?.path ?? "" } + } + } + private func apply() { + session.settings.scanDirectory = parseURL(from: scanDirectory) + } + + private func chooseScanDirectory() { + let panel = NSOpenPanel() + panel.canChooseDirectories = true + panel.canChooseFiles = false + panel.allowsMultipleSelection = false + panel.directoryURL = session.settings.resolvedScanDirectory + if panel.runModal() == .OK, let url = panel.url { + scanDirectory = url.path + apply() + } + } +} + +/// Agent settings: which `cursor-agent` executable to run. +private struct AgentSettingsView: View { + let session: ForemanSession + + @State private var agentExecutable: String + @State private var isWindowVisible = true + @FocusState private var isFocused: Bool + + init(session: ForemanSession) { + self.session = session + _agentExecutable = State(initialValue: session.settings.agentExecutable?.path ?? "") + } + + var body: some View { + Form { LabeledContent("cursor-agent") { VStack(alignment: .leading, spacing: 4) { TextField( @@ -55,8 +191,8 @@ struct SettingsView: View { prompt: Text("Auto-detect"), ) .labelsHidden() - .focused($focusedField, equals: .agentExecutable) - .onSubmit { applyAgentExecutable() } + .focused($isFocused) + .onSubmit { apply() } Text( "Leave empty to search: \(CursorAgentLocator.defaultSearchPaths.joined(separator: ", "))", ) @@ -66,55 +202,26 @@ struct SettingsView: View { } } .formStyle(.grouped) - .frame(width: 480) - // Tabbing or clicking out of a field commits it, like Return does. - .onChange(of: focusedField) { previous, _ in - switch previous { - case .scanDirectory: applyScanDirectory() - case .agentExecutable: applyAgentExecutable() - case nil: break - } + .navigationTitle("Agent") + .onChange(of: isFocused) { _, focused in + if !focused { apply() } } .background(WindowVisibilityReader(isVisible: $isWindowVisible)) - // The Settings window keeps its hierarchy across opens, so init-time - // drafts go stale; re-seed them whenever the window comes back. .onChange(of: isWindowVisible) { _, visible in - if visible { reseedDrafts() } + if visible { agentExecutable = session.settings.agentExecutable?.path ?? "" } } } - private func reseedDrafts() { - scanDirectory = session.settings.scanDirectory?.path ?? "" - agentExecutable = session.settings.agentExecutable?.path ?? "" - } - - private func applyScanDirectory() { - session.settings.scanDirectory = parseURL(from: scanDirectory) - } - - private func applyAgentExecutable() { + private func apply() { session.settings.agentExecutable = parseURL(from: agentExecutable) } +} - /// Empty means "use the default" (`nil`); otherwise a tilde-expanded - /// file URL. - private func parseURL(from text: String) -> URL? { - let trimmed = text.trimmingCharacters(in: .whitespaces) - guard !trimmed.isEmpty else { return nil } - return URL(fileURLWithPath: (trimmed as NSString).expandingTildeInPath) - } - - private func chooseScanDirectory() { - let panel = NSOpenPanel() - panel.canChooseDirectories = true - panel.canChooseFiles = false - panel.allowsMultipleSelection = false - panel.directoryURL = session.settings.resolvedScanDirectory - if panel.runModal() == .OK, let url = panel.url { - scanDirectory = url.path - applyScanDirectory() - } - } +/// Empty means "use the default" (`nil`); otherwise a tilde-expanded file URL. +private func parseURL(from text: String) -> URL? { + let trimmed = text.trimmingCharacters(in: .whitespaces) + guard !trimmed.isEmpty else { return nil } + return URL(fileURLWithPath: (trimmed as NSString).expandingTildeInPath) } #if DEBUG diff --git a/Foreman/ForemanCore/AGENTS.md b/Foreman/ForemanCore/AGENTS.md index 784da552..91a04dcc 100644 --- a/Foreman/ForemanCore/AGENTS.md +++ b/Foreman/ForemanCore/AGENTS.md @@ -11,7 +11,8 @@ and per-type detail. ForemanServices ── AppSettings ├────────── RepoDiscovery ── [Repo] ── Worker ├────────── WorkerConfigStore (ForemanConfiguration JSON) - └────────── SleepInhibitor + ├────────── SleepInhibitor + └────────── LoginItemController (SMAppService) ``` This file complements the root [`AGENTS.md`](../../AGENTS.md), which owns the @@ -49,6 +50,13 @@ build system, formatting, and global conventions. Read that first. start. - **The sleep assertion tracks liveness, not toggles** — recomputed on every worker transition, held while any worker (draining included) is live. +- **The login item is OS-owned, not persisted config.** `LoginItemController` + reads/writes `SMAppService.mainApp` (behind an `@_spi(Testing)` backend); + `ForemanServices.startsAtLogin` is a live read of that status, and its setter + surfaces failures on `issueMessage` while keeping the observed value honest. + Never mirror it into `ForemanConfiguration` — the system is the source of + truth. Launch-at-login just relaunches the app, which reuses the existing + `start()` restore of enabled workers. - **Absence vs failure.** Missing options read as `WorkerOptions.standard` and a missing config file is `.initial`; an unreadable/undecodable file throws. **CLI defaults are deferred to, not duplicated** — omit a flag diff --git a/Foreman/ForemanCore/README.md b/Foreman/ForemanCore/README.md index 088f6c79..ae212dc8 100644 --- a/Foreman/ForemanCore/README.md +++ b/Foreman/ForemanCore/README.md @@ -53,7 +53,9 @@ let argv = repo.options.arguments(workerDirectory: repo.rootURL) path. Funnels every persisted mutation (repo toggles/options, settings) into the saved JSON, recomputes the sleep assertion on every worker transition, and surfaces tree-level problems (unreadable config, failed - scan, failed save) on the observable `issueMessage`. + scan, failed save) on the observable `issueMessage`. `startsAtLogin` is a + two-way property over the login item (see `LoginItemController`); + `refreshLoginItemStatus()` re-reads it from the OS. - **`Repo` / `RepoID`** — one discovered repository as an `@Observable` object: identity (`name`, `rootURL`, typed `RepoID` = canonical absolute path), the persisted intent (`isEnabled`, `options` — mutations start/stop @@ -107,6 +109,13 @@ let argv = repo.options.arguments(workerDirectory: repo.rootURL) equivalent), so the machine won't doze off mid-agent-run. Display sleep and an explicit lid close are unaffected. The observable `isActive` backs the app's "preventing sleep" indicator. +- **`LoginItemController`** — reflects and toggles whether Foreman launches at + login, wrapping `SMAppService.mainApp` (no helper bundle or entitlement + needed for the main app). The OS owns the real state, so the observable + `isEnabled` is read from the service and `refresh()` re-reads it (the user + can change it in System Settings); `setEnabled(_:)` registers/unregisters + and re-syncs so a failed attempt never reads as falsely on. The real service + sits behind an `@_spi(Testing)` `LoginItemBackend` so tests inject a double. - **`CursorAgentLocator`** — resolves the `cursor-agent` executable. GUI apps don't inherit the shell `PATH`, so it checks the CLI's known install locations (`~/.local/bin`, `/usr/local/bin`, `/opt/homebrew/bin`); an diff --git a/Foreman/ForemanCore/Sources/ForemanServices.swift b/Foreman/ForemanCore/Sources/ForemanServices.swift index b3233541..c8ec578d 100644 --- a/Foreman/ForemanCore/Sources/ForemanServices.swift +++ b/Foreman/ForemanCore/Sources/ForemanServices.swift @@ -55,6 +55,32 @@ public final class ForemanServices { sleepInhibitor.isActive } + /// Whether Foreman is registered to launch at login. Backed by + /// `SMAppService` (the OS owns the real state), so this is a live read of + /// the login-item status. The setter registers/unregisters and, on + /// failure, logs *and* surfaces `issueMessage` while leaving the observed + /// value honest — a failed toggle stays off rather than falsely on. + public var startsAtLogin: Bool { + get { loginItem.isEnabled } + set { + do { + try loginItem.setEnabled(newValue) + } catch { + Self.logger.error("Couldn't update the login item: \(error)") + issueMessage = newValue + ? "Couldn't turn on Start at Login: \(error.localizedDescription)" + : "Couldn't turn off Start at Login: \(error.localizedDescription)" + } + } + } + + /// Re-reads the login-item status from the OS; it can change outside the + /// app (System Settings › General › Login Items), so the UI refreshes it + /// when the settings window reappears. + public func refreshLoginItemStatus() { + loginItem.refresh() + } + /// Where worker logs land: `~/Library/Logs/Foreman`. public static var defaultLogDirectory: URL { FileManager.default.homeDirectoryForCurrentUser @@ -70,6 +96,7 @@ public final class ForemanServices { private let configStore: WorkerConfigStore private let logDirectory: URL private let sleepInhibitor: SleepInhibitor + private let loginItem: LoginItemController private let locator = CursorAgentLocator() public convenience init(configStore: WorkerConfigStore, logDirectory: URL) { @@ -77,6 +104,7 @@ public final class ForemanServices { configStore: configStore, logDirectory: logDirectory, sleepInhibitor: SleepInhibitor(), + loginItem: LoginItemController(), ) } @@ -89,10 +117,12 @@ public final class ForemanServices { configStore: WorkerConfigStore, logDirectory: URL, sleepInhibitor: SleepInhibitor, + loginItem: LoginItemController, ) { self.configStore = configStore self.logDirectory = logDirectory self.sleepInhibitor = sleepInhibitor + self.loginItem = loginItem do { configuration = try configStore.load() configLoadFailure = nil diff --git a/Foreman/ForemanCore/Sources/LoginItemController.swift b/Foreman/ForemanCore/Sources/LoginItemController.swift new file mode 100644 index 00000000..bfd6297d --- /dev/null +++ b/Foreman/ForemanCore/Sources/LoginItemController.swift @@ -0,0 +1,87 @@ +import Foundation +import Observation +import ServiceManagement + +/// Registers, unregisters, and reports the app's login-item state behind +/// ``LoginItemController``. Production uses the `SMAppService.mainApp`-backed +/// implementation; tests conform a fake so no real login item is touched. +@MainActor +@_spi(Testing) +public protocol LoginItemBackend: AnyObject { + /// Whether the app is currently registered to launch at login. + var isRegistered: Bool { get } + func register() throws + func unregister() throws +} + +/// The real backend: `SMAppService.mainApp`. Registering the *main app* as a +/// login item needs no helper bundle and no special entitlement. +@MainActor +final class MainAppLoginItemBackend: LoginItemBackend { + var isRegistered: Bool { + switch SMAppService.mainApp.status { + case .enabled: true + // `.requiresApproval` means the user has to flip it on in System + // Settings before it actually launches, so it isn't "enabled" yet. + case .notRegistered, .requiresApproval, .notFound: false + @unknown default: false + } + } + + func register() throws { + try SMAppService.mainApp.register() + } + + func unregister() throws { + try SMAppService.mainApp.unregister() + } +} + +/// Reflects and controls whether Foreman launches at login, wrapping +/// `SMAppService.mainApp`. +/// +/// The OS owns the real state, so ``isEnabled`` is read from the backend (and +/// re-read via ``refresh()``, since the user can change it in System Settings +/// while the app runs). ``setEnabled(_:)`` registers or unregisters and then +/// re-syncs ``isEnabled`` from the backend so the observed value never lies — +/// a failed registration leaves the toggle honestly off, not falsely on. +@MainActor +@Observable +public final class LoginItemController { + /// Whether the app is registered to launch at login. Observable so the + /// settings toggle reflects it. + public private(set) var isEnabled: Bool + + @ObservationIgnored private let backend: any LoginItemBackend + + public init() { + backend = MainAppLoginItemBackend() + isEnabled = backend.isRegistered + } + + /// Swaps the real login-item backend for a test double. + @_spi(Testing) + public init(backend: any LoginItemBackend) { + self.backend = backend + isEnabled = backend.isRegistered + } + + /// Re-reads the OS status; it can change outside the app (System Settings + /// › General › Login Items). + public func refresh() { + isEnabled = backend.isRegistered + } + + /// Registers or unregisters the login item, then re-syncs ``isEnabled`` + /// from the backend. Rethrows the underlying `SMAppService` error; the + /// observed value stays honest whether it succeeds or throws. + public func setEnabled(_ enabled: Bool) throws { + defer { isEnabled = backend.isRegistered } + guard enabled != isEnabled else { return } + if enabled { + try backend.register() + } else { + try backend.unregister() + } + } +} diff --git a/Foreman/ForemanCore/Tests/ForemanCoreTestSupport.swift b/Foreman/ForemanCore/Tests/ForemanCoreTestSupport.swift index 6896be88..75d42643 100644 --- a/Foreman/ForemanCore/Tests/ForemanCoreTestSupport.swift +++ b/Foreman/ForemanCore/Tests/ForemanCoreTestSupport.swift @@ -18,6 +18,40 @@ final class SleepAssertionRecorder: SleepAssertionBackend { } } +/// A `LoginItemBackend` that records register/unregister calls in memory and +/// can be told to fail, so login-item wiring is testable without touching the +/// real `SMAppService`. +@MainActor +final class LoginItemRecorder: LoginItemBackend { + private(set) var isRegistered: Bool + private(set) var registerCount = 0 + private(set) var unregisterCount = 0 + + /// When set, both `register()` and `unregister()` throw it (and leave + /// `isRegistered` unchanged), simulating an `SMAppService` failure. + var failure: (any Error)? + + init(isRegistered: Bool = false, failure: (any Error)? = nil) { + self.isRegistered = isRegistered + self.failure = failure + } + + func register() throws { + registerCount += 1 + if let failure { throw failure } + isRegistered = true + } + + func unregister() throws { + unregisterCount += 1 + if let failure { throw failure } + isRegistered = false + } +} + +/// A stand-in error for login-item failure injection. +struct LoginItemTestError: Error {} + /// Builds a live `Repo` over a fixed executable for tree-level tests: /// disabled, standard options, no-op persistence and state-change hooks. @MainActor diff --git a/Foreman/ForemanCore/Tests/ForemanServicesTests.swift b/Foreman/ForemanCore/Tests/ForemanServicesTests.swift index 837cd2f9..00c6bd31 100644 --- a/Foreman/ForemanCore/Tests/ForemanServicesTests.swift +++ b/Foreman/ForemanCore/Tests/ForemanServicesTests.swift @@ -11,14 +11,17 @@ struct ForemanServicesTests { let scanDirectory: URL let executable: URL let sleep: SleepAssertionRecorder + let login: LoginItemRecorder } /// A config-store + scan-directory sandbox. `repoNames` become git repos /// in the scan directory; `configure` edits the saved configuration /// (which already points `agentExecutable` at a long-running stub) and - /// receives the scan directory for building `RepoID`s. + /// receives the scan directory for building `RepoID`s. `loginBackend` + /// injects a login-item double (defaults to a fresh one that succeeds). private func makeFixture( repoNames: [String], + loginBackend: LoginItemRecorder? = nil, configure: (inout ForemanConfiguration, _ scanDirectory: URL) -> Void = { _, _ in }, ) throws -> Fixture { let base = try makeTemporaryDirectory() @@ -46,10 +49,12 @@ struct ForemanServicesTests { try store.save(configuration) let recorder = SleepAssertionRecorder() + let login = loginBackend ?? LoginItemRecorder() let services = ForemanServices( configStore: store, logDirectory: base.appendingPathComponent("logs"), sleepInhibitor: SleepInhibitor(backend: recorder), + loginItem: LoginItemController(backend: login), ) return Fixture( services: services, @@ -58,6 +63,7 @@ struct ForemanServicesTests { scanDirectory: scanDirectory, executable: executable, sleep: recorder, + login: login, ) } @@ -114,6 +120,7 @@ struct ForemanServicesTests { configStore: WorkerConfigStore(directory: configDirectory), logDirectory: base.appendingPathComponent("logs"), sleepInhibitor: SleepInhibitor(backend: SleepAssertionRecorder()), + loginItem: LoginItemController(backend: LoginItemRecorder()), ) services.start() @@ -179,6 +186,37 @@ struct ForemanServicesTests { #expect(fixture.services.repos.map(\.name) == ["New"]) } + // MARK: - Launch at login + + @Test func startsAtLoginWritesThroughToTheLoginItem() throws { + let fixture = try makeFixture(repoNames: []) + #expect(!fixture.services.startsAtLogin) + + fixture.services.startsAtLogin = true + #expect(fixture.services.startsAtLogin) + #expect(fixture.login.registerCount == 1) + + fixture.services.startsAtLogin = false + #expect(!fixture.services.startsAtLogin) + #expect(fixture.login.unregisterCount == 1) + } + + @Test func aFailedLoginItemToggleSurfacesTheIssueAndStaysHonest() throws { + let fixture = try makeFixture( + repoNames: [], + loginBackend: LoginItemRecorder(failure: LoginItemTestError()), + ) + fixture.services.start() + #expect(fixture.services.issueMessage == nil) + + fixture.services.startsAtLogin = true + + // The registration failed, so the toggle stays off and the failure is + // observable rather than silently swallowed. + #expect(!fixture.services.startsAtLogin) + #expect(fixture.services.issueMessage?.contains("Start at Login") == true) + } + // MARK: - Rescan @Test func rescanPrunesStaleEntriesButKeepsForeignScanDirectoryOnes() throws { diff --git a/Foreman/ForemanCore/Tests/LoginItemControllerTests.swift b/Foreman/ForemanCore/Tests/LoginItemControllerTests.swift new file mode 100644 index 00000000..f1997d40 --- /dev/null +++ b/Foreman/ForemanCore/Tests/LoginItemControllerTests.swift @@ -0,0 +1,56 @@ +@_spi(Testing) import ForemanCore +import Testing + +@MainActor +struct LoginItemControllerTests { + @Test func seedsEnabledStateFromTheBackend() { + let off = LoginItemController(backend: LoginItemRecorder(isRegistered: false)) + #expect(!off.isEnabled) + + let on = LoginItemController(backend: LoginItemRecorder(isRegistered: true)) + #expect(on.isEnabled) + } + + @Test func enablingRegistersAndDisablingUnregisters() throws { + let recorder = LoginItemRecorder() + let controller = LoginItemController(backend: recorder) + + try controller.setEnabled(true) + #expect(controller.isEnabled) + #expect(recorder.registerCount == 1) + + // Re-enabling is a no-op: the backend isn't touched again. + try controller.setEnabled(true) + #expect(recorder.registerCount == 1) + + try controller.setEnabled(false) + #expect(!controller.isEnabled) + #expect(recorder.unregisterCount == 1) + } + + @Test func aFailedRegistrationLeavesTheStateHonestlyOff() { + let recorder = LoginItemRecorder(failure: LoginItemTestError()) + let controller = LoginItemController(backend: recorder) + + #expect(throws: LoginItemTestError.self) { + try controller.setEnabled(true) + } + // The register attempt happened, but it failed — the observed value + // must not read as "on". + #expect(recorder.registerCount == 1) + #expect(!controller.isEnabled) + } + + @Test func refreshPicksUpAnOutOfBandChange() { + let recorder = LoginItemRecorder(isRegistered: false) + let controller = LoginItemController(backend: recorder) + #expect(!controller.isEnabled) + + // Simulate the user enabling the login item in System Settings. + try? recorder.register() + #expect(!controller.isEnabled) + + controller.refresh() + #expect(controller.isEnabled) + } +} diff --git a/Foreman/TODOs.md b/Foreman/TODOs.md index d5fd94dd..ab194ea5 100644 --- a/Foreman/TODOs.md +++ b/Foreman/TODOs.md @@ -29,3 +29,8 @@ ## P2s (Nice to have) +- feat: Redesign the Settings window as a macOS System-Settings-style sidebar + (`NavigationSplitView`) with General / Repositories / Agent panes. +- feat: Add "Launch Foreman at login" (`SMAppService` via `LoginItemController`) + to General settings; launching at login reuses the existing `start()` restore + of enabled workers. From b851f11f3bc02d87eb1cd1d2c82a5125cc6000f9 Mon Sep 17 00:00:00 2001 From: Kyle Van Essen Date: Sun, 5 Jul 2026 20:53:34 -0400 Subject: [PATCH 2/4] Keep the settings sidebar always open and narrow the window Pin the NavigationSplitView column visibility to .all and remove the default sidebar-toggle button so the settings panes can't be collapsed. Reduce the window's minimum width from 620 to 420pt. Co-authored-by: Cursor --- Foreman/Foreman/Sources/SettingsView.swift | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/Foreman/Foreman/Sources/SettingsView.swift b/Foreman/Foreman/Sources/SettingsView.swift index c95fee08..d5782b60 100644 --- a/Foreman/Foreman/Sources/SettingsView.swift +++ b/Foreman/Foreman/Sources/SettingsView.swift @@ -48,7 +48,9 @@ struct SettingsView: View { @State private var selection: Pane? = .general var body: some View { - NavigationSplitView { + // The sidebar is always open: pin the visibility and drop the default + // sidebar-toggle button so the panes can't be collapsed away. + NavigationSplitView(columnVisibility: .constant(.all)) { List(selection: $selection) { ForEach(Pane.allCases) { pane in Label(pane.title, systemImage: pane.symbol) @@ -56,10 +58,11 @@ struct SettingsView: View { } } .navigationSplitViewColumnWidth(min: 170, ideal: 190, max: 220) + .toolbar(removing: .sidebarToggle) } detail: { detail } - .frame(minWidth: 620, minHeight: 380) + .frame(minWidth: 420, minHeight: 380) } @ViewBuilder From e71ab4d89cd6699f6da37d9e91161e2a7aca65e7 Mon Sep 17 00:00:00 2001 From: Kyle Van Essen Date: Sun, 5 Jul 2026 21:18:55 -0400 Subject: [PATCH 3/4] Surface login-item failures in settings, handle pending approval, and edit paths via sheets MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses code-review findings on the settings redesign: - Login-toggle failures now surface on a dedicated ForemanServices loginItemError (shown inline in the General settings pane) instead of the shared issueMessage, which the main-window rescan could clear before the user saw it. A successful toggle clears it. - LoginItemController now reads a typed LoginItemStatus; a registered-but- pending item (requiresApproval) reads as on with a needsApproval flag, and the General pane offers an "Open Login Items…" button (SMAppService.openSystemSettingsLoginItems) so it's no longer misreported as off. - Repositories and Agent panes show their value read-only with a Change… button that opens an explicit Save/Cancel sheet, so switching sidebar panes can't drop a half-typed path (removes the commit-on-blur reseed machinery). Co-authored-by: Cursor --- Foreman/Foreman/AGENTS.md | 6 +- Foreman/Foreman/README.md | 12 +- Foreman/Foreman/Sources/ForemanSession.swift | 18 +- Foreman/Foreman/Sources/SettingsView.swift | 211 ++++++++++++------ Foreman/ForemanCore/AGENTS.md | 15 +- Foreman/ForemanCore/README.md | 19 +- .../ForemanCore/Sources/ForemanServices.swift | 29 ++- .../Sources/LoginItemController.swift | 81 +++++-- .../Tests/ForemanCoreTestSupport.swift | 23 +- .../Tests/ForemanServicesTests.swift | 34 ++- .../Tests/LoginItemControllerTests.swift | 36 ++- 11 files changed, 347 insertions(+), 137 deletions(-) diff --git a/Foreman/Foreman/AGENTS.md b/Foreman/Foreman/AGENTS.md index adebb0e5..cc895ff1 100644 --- a/Foreman/Foreman/AGENTS.md +++ b/Foreman/Foreman/AGENTS.md @@ -40,8 +40,10 @@ formatting, global conventions) and ForemanCore's - **Hiding the window does not cancel `.task`** — an ordered-out `NSWindow` keeps its SwiftUI hierarchy alive and loops keep ticking (verified empirically). Any periodic work in this window must gate on - `WindowVisibilityReader` (see `WorkerLogView`), and view drafts re-seed on - visibility (see `SettingsView`). + `WindowVisibilityReader` (see `WorkerLogView`), and the settings General + pane re-reads the login-item status on visibility (see `SettingsView`). The + settings path fields are edited in an explicit Save/Cancel sheet, so there + is no commit-on-blur to lose when switching panes. - The target is an `LSUIElement` with a hand-written Info.plist in [`Project.swift`](../../Project.swift) — don't switch to `.extendingDefault`, which injects `NSMainStoryboardFile` on macOS. diff --git a/Foreman/Foreman/README.md b/Foreman/Foreman/README.md index d79bdc81..1fad07f5 100644 --- a/Foreman/Foreman/README.md +++ b/Foreman/Foreman/README.md @@ -48,15 +48,17 @@ closed. Closing the window just hides it — the app keeps running until Quit. ## Settings The settings window is a macOS System-Settings-style sidebar with three panes. -There's no Save button — changes apply as you make them (a field commits on -Return / focus change / a folder pick; a toggle applies immediately). +The *Launch at login* toggle applies immediately; the path settings open a +small editor sheet that commits only on **Save** (Cancel or Escape discards). - **General** — *Launch Foreman at login*. Registers Foreman as a login item via `SMAppService`, so it starts (and restores your enabled workers) when - you log in. + you log in. If macOS needs you to approve the item first, the pane says so + and links straight to System Settings; a failed toggle shows its error here. - **Repositories** — the directory scanned for git repositories (empty = - `~/Development`), with a folder picker. -- **Agent** — an explicit `cursor-agent` executable path (empty = auto-detect). + `~/Development`), edited via a sheet with a folder picker. +- **Agent** — an explicit `cursor-agent` executable path (empty = auto-detect), + edited via a sheet. ## Lifecycle diff --git a/Foreman/Foreman/Sources/ForemanSession.swift b/Foreman/Foreman/Sources/ForemanSession.swift index bbf34952..f2e069ce 100644 --- a/Foreman/Foreman/Sources/ForemanSession.swift +++ b/Foreman/Foreman/Sources/ForemanSession.swift @@ -39,12 +39,22 @@ final class ForemanSession { /// Whether Foreman launches at login. `SettingsView` binds this two-way; /// the setter registers/unregisters the login item in Core (which logs and - /// surfaces any failure on `issueMessage`). + /// surfaces any failure on `loginItemError`). var startsAtLogin: Bool { get { services.startsAtLogin } set { services.startsAtLogin = newValue } } + /// The login item is registered but awaiting approval in System Settings. + var loginItemNeedsApproval: Bool { + services.loginItemNeedsApproval + } + + /// The most recent login-item failure (shown in the General settings pane). + var loginItemError: String? { + services.loginItemError + } + init(services: ForemanServices) { self.services = services } @@ -77,4 +87,10 @@ final class ForemanSession { func refreshLoginItemStatus() { services.refreshLoginItemStatus() } + + /// Opens System Settings › General › Login Items (to approve a pending + /// login item). + func openSystemSettingsLoginItems() { + services.openSystemSettingsLoginItems() + } } diff --git a/Foreman/Foreman/Sources/SettingsView.swift b/Foreman/Foreman/Sources/SettingsView.swift index d5782b60..885abba0 100644 --- a/Foreman/Foreman/Sources/SettingsView.swift +++ b/Foreman/Foreman/Sources/SettingsView.swift @@ -94,6 +94,29 @@ private struct GeneralSettingsView: View { ) .font(.caption) .foregroundStyle(.secondary) + + // Registered but macOS is waiting for the user to approve it. + if session.loginItemNeedsApproval { + LabeledContent { + Button("Open Login Items…") { + session.openSystemSettingsLoginItems() + } + } label: { + Label( + "Approve Foreman in System Settings to finish enabling this.", + systemImage: "exclamationmark.triangle.fill", + ) + .foregroundStyle(.orange) + } + } + + // A failed register/unregister — shown here (not the main-window + // banner) because the toggle lives in this window. + if let error = session.loginItemError { + Label(error, systemImage: "xmark.octagon.fill") + .font(.callout) + .foregroundStyle(.red) + } } .formStyle(.grouped) .navigationTitle("General") @@ -108,115 +131,155 @@ private struct GeneralSettingsView: View { } } -/// Repository discovery: where to scan for git repositories. +/// Repository discovery: where to scan for git repositories. The value is +/// read-only here; editing happens in an explicit-commit sheet so switching +/// panes can't drop a half-typed path. private struct RepositoriesSettingsView: View { let session: ForemanSession - @State private var scanDirectory: String - @State private var isWindowVisible = true - @FocusState private var isFocused: Bool - - init(session: ForemanSession) { - self.session = session - _scanDirectory = State(initialValue: session.settings.scanDirectory?.path ?? "") - } + @State private var isEditing = false var body: some View { Form { LabeledContent("Scan directory") { - HStack(spacing: 4) { - TextField( - "Scan directory", - text: $scanDirectory, - prompt: Text("~/Development"), - ) - .labelsHidden() - .focused($isFocused) - .onSubmit { apply() } - Button("Choose…") { chooseScanDirectory() } - } + Text(session.settings.scanDirectory?.path ?? "~/Development (default)") + .foregroundStyle(.secondary) + .textSelection(.enabled) } + Button("Change…") { isEditing = true } Text("Foreman lists the git repositories directly inside this directory.") .font(.caption) .foregroundStyle(.secondary) } .formStyle(.grouped) .navigationTitle("Repositories") - // Tabbing or clicking out of the field commits it, like Return does. - .onChange(of: isFocused) { _, focused in - if !focused { apply() } - } - .background(WindowVisibilityReader(isVisible: $isWindowVisible)) - // The Settings window keeps its hierarchy across opens, so init-time - // drafts go stale; re-seed whenever the window comes back. - .onChange(of: isWindowVisible) { _, visible in - if visible { scanDirectory = session.settings.scanDirectory?.path ?? "" } + .sheet(isPresented: $isEditing) { + PathEditorSheet( + title: "Scan directory", + prompt: "~/Development", + caption: "Leave empty to use the default (~/Development).", + directoryPickerStart: session.settings.resolvedScanDirectory, + initialValue: session.settings.scanDirectory?.path ?? "", + onSave: { session.settings.scanDirectory = $0 }, + ) } } +} - private func apply() { - session.settings.scanDirectory = parseURL(from: scanDirectory) - } +/// Agent settings: which `cursor-agent` executable to run. Read-only here; +/// edited via an explicit-commit sheet (see `RepositoriesSettingsView`). +private struct AgentSettingsView: View { + let session: ForemanSession - private func chooseScanDirectory() { - let panel = NSOpenPanel() - panel.canChooseDirectories = true - panel.canChooseFiles = false - panel.allowsMultipleSelection = false - panel.directoryURL = session.settings.resolvedScanDirectory - if panel.runModal() == .OK, let url = panel.url { - scanDirectory = url.path - apply() + @State private var isEditing = false + + var body: some View { + Form { + LabeledContent("cursor-agent") { + Text(session.settings.agentExecutable?.path ?? "Auto-detect") + .foregroundStyle(.secondary) + .textSelection(.enabled) + } + Button("Change…") { isEditing = true } + Text( + "Leave empty to search: \(CursorAgentLocator.defaultSearchPaths.joined(separator: ", "))", + ) + .font(.caption2) + .foregroundStyle(.tertiary) + } + .formStyle(.grouped) + .navigationTitle("Agent") + .sheet(isPresented: $isEditing) { + PathEditorSheet( + title: "cursor-agent executable", + prompt: "Auto-detect", + caption: "Leave empty to auto-detect from the known install locations.", + directoryPickerStart: nil, + initialValue: session.settings.agentExecutable?.path ?? "", + onSave: { session.settings.agentExecutable = $0 }, + ) } } } -/// Agent settings: which `cursor-agent` executable to run. -private struct AgentSettingsView: View { - let session: ForemanSession +/// A modal editor for a single optional path setting. The draft only commits +/// when the user taps Save (Cancel/Escape discards it), so there is no +/// commit-on-blur and no way to lose a half-typed value by navigating away. +/// Empty commits as `nil` (the setting's default). +private struct PathEditorSheet: View { + let title: String + let prompt: String + let caption: String + /// When non-nil, shows a "Choose…" folder picker starting at this URL. + let directoryPickerStart: URL? + let initialValue: String + let onSave: (URL?) -> Void - @State private var agentExecutable: String - @State private var isWindowVisible = true + @Environment(\.dismiss) private var dismiss + @State private var draft: String @FocusState private var isFocused: Bool - init(session: ForemanSession) { - self.session = session - _agentExecutable = State(initialValue: session.settings.agentExecutable?.path ?? "") + init( + title: String, + prompt: String, + caption: String, + directoryPickerStart: URL?, + initialValue: String, + onSave: @escaping (URL?) -> Void, + ) { + self.title = title + self.prompt = prompt + self.caption = caption + self.directoryPickerStart = directoryPickerStart + self.initialValue = initialValue + self.onSave = onSave + _draft = State(initialValue: initialValue) } var body: some View { - Form { - LabeledContent("cursor-agent") { - VStack(alignment: .leading, spacing: 4) { - TextField( - "cursor-agent executable", - text: $agentExecutable, - prompt: Text("Auto-detect"), - ) + VStack(alignment: .leading, spacing: 16) { + Text(title) + .font(.headline) + HStack(spacing: 8) { + TextField(title, text: $draft, prompt: Text(prompt)) .labelsHidden() .focused($isFocused) - .onSubmit { apply() } - Text( - "Leave empty to search: \(CursorAgentLocator.defaultSearchPaths.joined(separator: ", "))", - ) - .font(.caption2) - .foregroundStyle(.tertiary) + .onSubmit { save() } + if directoryPickerStart != nil { + Button("Choose…") { choose() } } } + Text(caption) + .font(.caption) + .foregroundStyle(.secondary) + HStack { + Spacer() + Button("Cancel", role: .cancel) { dismiss() } + .keyboardShortcut(.cancelAction) + Button("Save") { save() } + .keyboardShortcut(.defaultAction) + } } - .formStyle(.grouped) - .navigationTitle("Agent") - .onChange(of: isFocused) { _, focused in - if !focused { apply() } - } - .background(WindowVisibilityReader(isVisible: $isWindowVisible)) - .onChange(of: isWindowVisible) { _, visible in - if visible { agentExecutable = session.settings.agentExecutable?.path ?? "" } - } + .padding(20) + .frame(width: 460) + .onAppear { isFocused = true } } - private func apply() { - session.settings.agentExecutable = parseURL(from: agentExecutable) + private func save() { + onSave(parseURL(from: draft)) + dismiss() + } + + private func choose() { + guard let directoryPickerStart else { return } + let panel = NSOpenPanel() + panel.canChooseDirectories = true + panel.canChooseFiles = false + panel.allowsMultipleSelection = false + panel.directoryURL = directoryPickerStart + if panel.runModal() == .OK, let url = panel.url { + draft = url.path + } } } diff --git a/Foreman/ForemanCore/AGENTS.md b/Foreman/ForemanCore/AGENTS.md index 91a04dcc..85de2789 100644 --- a/Foreman/ForemanCore/AGENTS.md +++ b/Foreman/ForemanCore/AGENTS.md @@ -51,12 +51,15 @@ build system, formatting, and global conventions. Read that first. - **The sleep assertion tracks liveness, not toggles** — recomputed on every worker transition, held while any worker (draining included) is live. - **The login item is OS-owned, not persisted config.** `LoginItemController` - reads/writes `SMAppService.mainApp` (behind an `@_spi(Testing)` backend); - `ForemanServices.startsAtLogin` is a live read of that status, and its setter - surfaces failures on `issueMessage` while keeping the observed value honest. - Never mirror it into `ForemanConfiguration` — the system is the source of - truth. Launch-at-login just relaunches the app, which reuses the existing - `start()` restore of enabled workers. + reads/writes `SMAppService.mainApp` (behind an `@_spi(Testing)` backend) as a + typed `LoginItemStatus`; `requiresApproval` counts as *on* (registered but + pending), so it never reads as off. `ForemanServices.startsAtLogin` is a live + read of that status, and its setter surfaces failures on the dedicated + `loginItemError` (the toggle lives in the settings window, not the main + banner) while keeping the observed value honest. Never mirror it into + `ForemanConfiguration` — the system is the source of truth. Launch-at-login + just relaunches the app, which reuses the existing `start()` restore of + enabled workers. - **Absence vs failure.** Missing options read as `WorkerOptions.standard` and a missing config file is `.initial`; an unreadable/undecodable file throws. **CLI defaults are deferred to, not duplicated** — omit a flag diff --git a/Foreman/ForemanCore/README.md b/Foreman/ForemanCore/README.md index ae212dc8..e85ff9a5 100644 --- a/Foreman/ForemanCore/README.md +++ b/Foreman/ForemanCore/README.md @@ -54,8 +54,11 @@ let argv = repo.options.arguments(workerDirectory: repo.rootURL) into the saved JSON, recomputes the sleep assertion on every worker transition, and surfaces tree-level problems (unreadable config, failed scan, failed save) on the observable `issueMessage`. `startsAtLogin` is a - two-way property over the login item (see `LoginItemController`); - `refreshLoginItemStatus()` re-reads it from the OS. + two-way property over the login item (see `LoginItemController`), with + `loginItemNeedsApproval` and a dedicated `loginItemError` (login failures + surface here, not on the shared `issueMessage`); `refreshLoginItemStatus()` + re-reads it from the OS and `openSystemSettingsLoginItems()` jumps to the + approval UI. - **`Repo` / `RepoID`** — one discovered repository as an `@Observable` object: identity (`name`, `rootURL`, typed `RepoID` = canonical absolute path), the persisted intent (`isEnabled`, `options` — mutations start/stop @@ -112,10 +115,14 @@ let argv = repo.options.arguments(workerDirectory: repo.rootURL) - **`LoginItemController`** — reflects and toggles whether Foreman launches at login, wrapping `SMAppService.mainApp` (no helper bundle or entitlement needed for the main app). The OS owns the real state, so the observable - `isEnabled` is read from the service and `refresh()` re-reads it (the user - can change it in System Settings); `setEnabled(_:)` registers/unregisters - and re-syncs so a failed attempt never reads as falsely on. The real service - sits behind an `@_spi(Testing)` `LoginItemBackend` so tests inject a double. + `status` (a `LoginItemStatus`: `enabled` / `requiresApproval` / + `notRegistered`) is read from the service and `refresh()` re-reads it (the + user can change it in System Settings). `isEnabled` treats a pending + (`requiresApproval`) item as on — not off — with `needsApproval` flagging + that the user still has to confirm it (`openSystemSettingsLoginItems()` + jumps there). `setEnabled(_:)` registers/unregisters and re-syncs so a + failed attempt never reads as falsely on. The real service sits behind an + `@_spi(Testing)` `LoginItemBackend` so tests inject a double. - **`CursorAgentLocator`** — resolves the `cursor-agent` executable. GUI apps don't inherit the shell `PATH`, so it checks the CLI's known install locations (`~/.local/bin`, `/usr/local/bin`, `/opt/homebrew/bin`); an diff --git a/Foreman/ForemanCore/Sources/ForemanServices.swift b/Foreman/ForemanCore/Sources/ForemanServices.swift index c8ec578d..87436154 100644 --- a/Foreman/ForemanCore/Sources/ForemanServices.swift +++ b/Foreman/ForemanCore/Sources/ForemanServices.swift @@ -58,22 +58,35 @@ public final class ForemanServices { /// Whether Foreman is registered to launch at login. Backed by /// `SMAppService` (the OS owns the real state), so this is a live read of /// the login-item status. The setter registers/unregisters and, on - /// failure, logs *and* surfaces `issueMessage` while leaving the observed - /// value honest — a failed toggle stays off rather than falsely on. + /// failure, logs *and* surfaces ``loginItemError`` while leaving the + /// observed value honest — a failed toggle stays off rather than falsely + /// on. A successful toggle clears the error. public var startsAtLogin: Bool { get { loginItem.isEnabled } set { do { try loginItem.setEnabled(newValue) + loginItemError = nil } catch { Self.logger.error("Couldn't update the login item: \(error)") - issueMessage = newValue - ? "Couldn't turn on Start at Login: \(error.localizedDescription)" - : "Couldn't turn off Start at Login: \(error.localizedDescription)" + loginItemError = newValue + ? "Couldn't turn on “Launch at login”: \(error.localizedDescription)" + : "Couldn't turn off “Launch at login”: \(error.localizedDescription)" } } } + /// The login item is registered but macOS is waiting for the user to + /// approve it in System Settings before it will launch. + public var loginItemNeedsApproval: Bool { + loginItem.needsApproval + } + + /// The most recent login-item failure, surfaced in the settings window's + /// General pane (not the main-window banner — the toggle lives here). + /// Cleared by the next successful toggle. + public private(set) var loginItemError: String? + /// Re-reads the login-item status from the OS; it can change outside the /// app (System Settings › General › Login Items), so the UI refreshes it /// when the settings window reappears. @@ -81,6 +94,12 @@ public final class ForemanServices { loginItem.refresh() } + /// Opens System Settings › General › Login Items so the user can approve a + /// pending login item. + public func openSystemSettingsLoginItems() { + loginItem.openSystemSettingsLoginItems() + } + /// Where worker logs land: `~/Library/Logs/Foreman`. public static var defaultLogDirectory: URL { FileManager.default.homeDirectoryForCurrentUser diff --git a/Foreman/ForemanCore/Sources/LoginItemController.swift b/Foreman/ForemanCore/Sources/LoginItemController.swift index bfd6297d..eb604415 100644 --- a/Foreman/ForemanCore/Sources/LoginItemController.swift +++ b/Foreman/ForemanCore/Sources/LoginItemController.swift @@ -2,29 +2,45 @@ import Foundation import Observation import ServiceManagement +/// The login-item state Foreman cares about, distilled from +/// `SMAppService.Status`. +/// +/// `requiresApproval` is distinct from `notRegistered`: the app *is* registered +/// but macOS is waiting for the user to approve it in System Settings before it +/// will actually launch. Collapsing it into "off" would misreport an enabled +/// (pending) item as disabled. +public enum LoginItemStatus: Sendable, Equatable { + case enabled + case requiresApproval + case notRegistered +} + /// Registers, unregisters, and reports the app's login-item state behind /// ``LoginItemController``. Production uses the `SMAppService.mainApp`-backed /// implementation; tests conform a fake so no real login item is touched. @MainActor @_spi(Testing) public protocol LoginItemBackend: AnyObject { - /// Whether the app is currently registered to launch at login. - var isRegistered: Bool { get } + var status: LoginItemStatus { get } func register() throws func unregister() throws + /// Opens System Settings › General › Login Items so the user can approve + /// or inspect the item. + func openSystemSettingsLoginItems() } /// The real backend: `SMAppService.mainApp`. Registering the *main app* as a /// login item needs no helper bundle and no special entitlement. @MainActor final class MainAppLoginItemBackend: LoginItemBackend { - var isRegistered: Bool { + var status: LoginItemStatus { switch SMAppService.mainApp.status { - case .enabled: true - // `.requiresApproval` means the user has to flip it on in System - // Settings before it actually launches, so it isn't "enabled" yet. - case .notRegistered, .requiresApproval, .notFound: false - @unknown default: false + case .enabled: .enabled + case .requiresApproval: .requiresApproval + // `.notFound` means the service isn't registered from this bundle; + // for the toggle that reads the same as "not registered". + case .notRegistered, .notFound: .notRegistered + @unknown default: .notRegistered } } @@ -35,48 +51,65 @@ final class MainAppLoginItemBackend: LoginItemBackend { func unregister() throws { try SMAppService.mainApp.unregister() } + + func openSystemSettingsLoginItems() { + SMAppService.openSystemSettingsLoginItems() + } } /// Reflects and controls whether Foreman launches at login, wrapping /// `SMAppService.mainApp`. /// -/// The OS owns the real state, so ``isEnabled`` is read from the backend (and +/// The OS owns the real state, so ``status`` is read from the backend (and /// re-read via ``refresh()``, since the user can change it in System Settings /// while the app runs). ``setEnabled(_:)`` registers or unregisters and then -/// re-syncs ``isEnabled`` from the backend so the observed value never lies — -/// a failed registration leaves the toggle honestly off, not falsely on. +/// re-syncs ``status`` from the backend so the observed value never lies — a +/// failed registration leaves the toggle honestly off, not falsely on. @MainActor @Observable public final class LoginItemController { - /// Whether the app is registered to launch at login. Observable so the - /// settings toggle reflects it. - public private(set) var isEnabled: Bool + /// The current login-item status. Observable so the settings UI reflects + /// it (including the pending-approval state). + public private(set) var status: LoginItemStatus @ObservationIgnored private let backend: any LoginItemBackend + /// Whether the login item is on. Includes ``LoginItemStatus/requiresApproval``: + /// the user asked for it and it's registered — it just needs a nod in + /// System Settings — so the toggle should read on, not off. + public var isEnabled: Bool { + status != .notRegistered + } + + /// The login item is registered but macOS needs the user to approve it in + /// System Settings before it will actually launch. + public var needsApproval: Bool { + status == .requiresApproval + } + public init() { backend = MainAppLoginItemBackend() - isEnabled = backend.isRegistered + status = backend.status } /// Swaps the real login-item backend for a test double. @_spi(Testing) public init(backend: any LoginItemBackend) { self.backend = backend - isEnabled = backend.isRegistered + status = backend.status } /// Re-reads the OS status; it can change outside the app (System Settings /// › General › Login Items). public func refresh() { - isEnabled = backend.isRegistered + status = backend.status } - /// Registers or unregisters the login item, then re-syncs ``isEnabled`` - /// from the backend. Rethrows the underlying `SMAppService` error; the - /// observed value stays honest whether it succeeds or throws. + /// Registers or unregisters the login item, then re-syncs ``status`` from + /// the backend. Rethrows the underlying `SMAppService` error; the observed + /// value stays honest whether it succeeds or throws. public func setEnabled(_ enabled: Bool) throws { - defer { isEnabled = backend.isRegistered } + defer { status = backend.status } guard enabled != isEnabled else { return } if enabled { try backend.register() @@ -84,4 +117,10 @@ public final class LoginItemController { try backend.unregister() } } + + /// Opens System Settings › General › Login Items (used to approve a + /// pending item). + public func openSystemSettingsLoginItems() { + backend.openSystemSettingsLoginItems() + } } diff --git a/Foreman/ForemanCore/Tests/ForemanCoreTestSupport.swift b/Foreman/ForemanCore/Tests/ForemanCoreTestSupport.swift index 75d42643..96d21d59 100644 --- a/Foreman/ForemanCore/Tests/ForemanCoreTestSupport.swift +++ b/Foreman/ForemanCore/Tests/ForemanCoreTestSupport.swift @@ -18,34 +18,39 @@ final class SleepAssertionRecorder: SleepAssertionBackend { } } -/// A `LoginItemBackend` that records register/unregister calls in memory and -/// can be told to fail, so login-item wiring is testable without touching the -/// real `SMAppService`. +/// A `LoginItemBackend` that records register/unregister/open calls in memory +/// and can be told to fail, so login-item wiring is testable without touching +/// the real `SMAppService`. @MainActor final class LoginItemRecorder: LoginItemBackend { - private(set) var isRegistered: Bool + private(set) var status: LoginItemStatus private(set) var registerCount = 0 private(set) var unregisterCount = 0 + private(set) var openCount = 0 /// When set, both `register()` and `unregister()` throw it (and leave - /// `isRegistered` unchanged), simulating an `SMAppService` failure. + /// `status` unchanged), simulating an `SMAppService` failure. var failure: (any Error)? - init(isRegistered: Bool = false, failure: (any Error)? = nil) { - self.isRegistered = isRegistered + init(status: LoginItemStatus = .notRegistered, failure: (any Error)? = nil) { + self.status = status self.failure = failure } func register() throws { registerCount += 1 if let failure { throw failure } - isRegistered = true + status = .enabled } func unregister() throws { unregisterCount += 1 if let failure { throw failure } - isRegistered = false + status = .notRegistered + } + + func openSystemSettingsLoginItems() { + openCount += 1 } } diff --git a/Foreman/ForemanCore/Tests/ForemanServicesTests.swift b/Foreman/ForemanCore/Tests/ForemanServicesTests.swift index 00c6bd31..77c9d3f5 100644 --- a/Foreman/ForemanCore/Tests/ForemanServicesTests.swift +++ b/Foreman/ForemanCore/Tests/ForemanServicesTests.swift @@ -201,20 +201,46 @@ struct ForemanServicesTests { #expect(fixture.login.unregisterCount == 1) } - @Test func aFailedLoginItemToggleSurfacesTheIssueAndStaysHonest() throws { + @Test func aFailedLoginItemToggleSurfacesTheErrorAndStaysHonest() throws { let fixture = try makeFixture( repoNames: [], loginBackend: LoginItemRecorder(failure: LoginItemTestError()), ) fixture.services.start() - #expect(fixture.services.issueMessage == nil) + #expect(fixture.services.loginItemError == nil) fixture.services.startsAtLogin = true // The registration failed, so the toggle stays off and the failure is - // observable rather than silently swallowed. + // observable on the login-item channel (not the tree-level banner) + // rather than silently swallowed. #expect(!fixture.services.startsAtLogin) - #expect(fixture.services.issueMessage?.contains("Start at Login") == true) + #expect(fixture.services.loginItemError != nil) + #expect(fixture.services.issueMessage == nil) + } + + @Test func aSuccessfulToggleClearsAPriorLoginItemError() throws { + let recorder = LoginItemRecorder(failure: LoginItemTestError()) + let fixture = try makeFixture(repoNames: [], loginBackend: recorder) + + fixture.services.startsAtLogin = true + #expect(fixture.services.loginItemError != nil) + + // Recover: the next toggle succeeds and the stale error clears. + recorder.failure = nil + fixture.services.startsAtLogin = true + #expect(fixture.services.startsAtLogin) + #expect(fixture.services.loginItemError == nil) + } + + @Test func pendingApprovalReadsAsEnabledAndNeedsApproval() throws { + let fixture = try makeFixture( + repoNames: [], + loginBackend: LoginItemRecorder(status: .requiresApproval), + ) + + #expect(fixture.services.startsAtLogin) + #expect(fixture.services.loginItemNeedsApproval) } // MARK: - Rescan diff --git a/Foreman/ForemanCore/Tests/LoginItemControllerTests.swift b/Foreman/ForemanCore/Tests/LoginItemControllerTests.swift index f1997d40..516043ae 100644 --- a/Foreman/ForemanCore/Tests/LoginItemControllerTests.swift +++ b/Foreman/ForemanCore/Tests/LoginItemControllerTests.swift @@ -3,12 +3,23 @@ import Testing @MainActor struct LoginItemControllerTests { - @Test func seedsEnabledStateFromTheBackend() { - let off = LoginItemController(backend: LoginItemRecorder(isRegistered: false)) + @Test func seedsStateFromTheBackend() { + let off = LoginItemController(backend: LoginItemRecorder(status: .notRegistered)) #expect(!off.isEnabled) + #expect(!off.needsApproval) - let on = LoginItemController(backend: LoginItemRecorder(isRegistered: true)) + let on = LoginItemController(backend: LoginItemRecorder(status: .enabled)) #expect(on.isEnabled) + #expect(!on.needsApproval) + } + + @Test func requiresApprovalReadsAsEnabledButPending() { + let controller = LoginItemController(backend: LoginItemRecorder(status: .requiresApproval)) + + // Registered-but-pending is "on" for the toggle — not off — with a + // flag the UI can use to nudge the user toward System Settings. + #expect(controller.isEnabled) + #expect(controller.needsApproval) } @Test func enablingRegistersAndDisablingUnregisters() throws { @@ -28,6 +39,15 @@ struct LoginItemControllerTests { #expect(recorder.unregisterCount == 1) } + @Test func disablingAPendingItemStillUnregisters() throws { + let recorder = LoginItemRecorder(status: .requiresApproval) + let controller = LoginItemController(backend: recorder) + + try controller.setEnabled(false) + #expect(recorder.unregisterCount == 1) + #expect(!controller.isEnabled) + } + @Test func aFailedRegistrationLeavesTheStateHonestlyOff() { let recorder = LoginItemRecorder(failure: LoginItemTestError()) let controller = LoginItemController(backend: recorder) @@ -42,7 +62,7 @@ struct LoginItemControllerTests { } @Test func refreshPicksUpAnOutOfBandChange() { - let recorder = LoginItemRecorder(isRegistered: false) + let recorder = LoginItemRecorder(status: .notRegistered) let controller = LoginItemController(backend: recorder) #expect(!controller.isEnabled) @@ -53,4 +73,12 @@ struct LoginItemControllerTests { controller.refresh() #expect(controller.isEnabled) } + + @Test func openSystemSettingsForwardsToTheBackend() { + let recorder = LoginItemRecorder() + let controller = LoginItemController(backend: recorder) + + controller.openSystemSettingsLoginItems() + #expect(recorder.openCount == 1) + } } From b3d7a0fed54ca977b4b932896da4a535a16ee07b Mon Sep 17 00:00:00 2001 From: Kyle Van Essen Date: Sun, 5 Jul 2026 21:24:13 -0400 Subject: [PATCH 4/4] Model settings panes with a SettingsPane protocol instead of an enum MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the parallel `Pane` enum (title/symbol properties plus a `detail` switch) with a `SettingsPane` protocol: each pane is a `View` that provides a static `title`, static `icon`, and an `init(session:)` builder. A small type-erased `SettingsPaneItem` (keyed by the pane type's `ObjectIdentifier`, so selection stays a typed token) drives the sidebar list and detail, so adding a pane is just conforming a new view and listing it — no central enum or switch to keep in lockstep. Addresses PR review feedback on SettingsView structure. Co-authored-by: Cursor --- Foreman/Foreman/Sources/SettingsView.swift | 142 ++++++++++++--------- 1 file changed, 80 insertions(+), 62 deletions(-) diff --git a/Foreman/Foreman/Sources/SettingsView.swift b/Foreman/Foreman/Sources/SettingsView.swift index 885abba0..dc87d64b 100644 --- a/Foreman/Foreman/Sources/SettingsView.swift +++ b/Foreman/Foreman/Sources/SettingsView.swift @@ -2,60 +2,61 @@ import AppKit import ForemanCore import SwiftUI -/// Global settings, laid out like a modern macOS System Settings window: a -/// `NavigationSplitView` sidebar selects one of three panes. -/// -/// - **General** — launch Foreman at login. -/// - **Repositories** — the directory scanned for git repositories. -/// - **Agent** — which `cursor-agent` executable to run. -/// -/// Hosted by the `Settings` scene (app menu / Cmd-, / the toolbar's -/// `SettingsLink`), so it follows settings-window conventions: edits apply on -/// commit (Return / focus change / panel selection / a toggle flip) — no Save -/// button. Values write through the observable Core (`AppSettings` for the -/// paths, `LoginItemController` for the login item), which persists. -struct SettingsView: View { - /// The sidebar panes. `Identifiable` (via `id: \.self`) so the sidebar - /// `List` can iterate `allCases`. - private enum Pane: String, CaseIterable, Identifiable { - case general - case repositories - case agent - - var id: Self { - self - } +/// A pane in the settings sidebar: static identity/metadata plus its own +/// content view, built from the session. Conformers are ordinary `View`s; +/// `SettingsView` discovers their title/icon and builds them without a central +/// enum or switch. +private protocol SettingsPane: View { + /// Sidebar label and navigation title. + static var title: String { get } + /// Sidebar SF Symbol. + static var icon: String { get } - var title: String { - switch self { - case .general: "General" - case .repositories: "Repositories" - case .agent: "Agent" - } - } + init(session: ForemanSession) +} - var symbol: String { - switch self { - case .general: "gearshape" - case .repositories: "folder" - case .agent: "terminal" - } - } +/// Type-erased sidebar entry: a pane's metadata plus a builder for its content. +/// Keyed by the pane type's `ObjectIdentifier`, so the sidebar selection stays +/// a typed token rather than a stringly value. +private struct SettingsPaneItem: Identifiable { + let id: ObjectIdentifier + let title: String + let icon: String + let makeView: (ForemanSession) -> AnyView + + init(_ pane: (some SettingsPane).Type) { + id = ObjectIdentifier(pane) + title = pane.title + icon = pane.icon + makeView = { AnyView(pane.init(session: $0)) } } +} +/// Global settings, laid out like a modern macOS System Settings window: a +/// `NavigationSplitView` sidebar over the registered ``SettingsPaneItem``s. +/// +/// Hosted by the `Settings` scene (app menu / Cmd-, / the toolbar's +/// `SettingsLink`), so it follows settings-window conventions: the toggle in +/// General applies immediately, and the path panes commit through an explicit +/// Save/Cancel sheet. Values write through the observable Core (`AppSettings` +/// for the paths, `LoginItemController` for the login item), which persists. +struct SettingsView: View { let session: ForemanSession - @State private var selection: Pane? = .general + private let panes: [SettingsPaneItem] = [ + SettingsPaneItem(GeneralSettingsPane.self), + SettingsPaneItem(RepositoriesSettingsPane.self), + SettingsPaneItem(AgentSettingsPane.self), + ] + + @State private var selection: ObjectIdentifier? var body: some View { // The sidebar is always open: pin the visibility and drop the default // sidebar-toggle button so the panes can't be collapsed away. NavigationSplitView(columnVisibility: .constant(.all)) { - List(selection: $selection) { - ForEach(Pane.allCases) { pane in - Label(pane.title, systemImage: pane.symbol) - .tag(pane) - } + List(panes, selection: $selection) { pane in + Label(pane.title, systemImage: pane.icon) } .navigationSplitViewColumnWidth(min: 170, ideal: 190, max: 220) .toolbar(removing: .sidebarToggle) @@ -63,29 +64,32 @@ struct SettingsView: View { detail } .frame(minWidth: 420, minHeight: 380) + .onAppear { + if selection == nil { selection = panes.first?.id } + } } - @ViewBuilder - private var detail: some View { - switch selection { - // No selection falls back to General — the sidebar always has a - // meaningful pane on screen. - case .general, .none: - GeneralSettingsView(session: session) - case .repositories: - RepositoriesSettingsView(session: session) - case .agent: - AgentSettingsView(session: session) - } + /// The selected pane's content, falling back to the first pane so the + /// detail always shows something meaningful. + private var detail: AnyView { + let pane = panes.first { $0.id == selection } ?? panes.first + return pane?.makeView(session) ?? AnyView(EmptyView()) } } /// General preferences: whether Foreman launches at login. -private struct GeneralSettingsView: View { +private struct GeneralSettingsPane: SettingsPane { + static let title = "General" + static let icon = "gearshape" + @Bindable var session: ForemanSession @State private var isWindowVisible = true + init(session: ForemanSession) { + _session = Bindable(session) + } + var body: some View { Form { Toggle("Launch Foreman at login", isOn: $session.startsAtLogin) @@ -119,7 +123,7 @@ private struct GeneralSettingsView: View { } } .formStyle(.grouped) - .navigationTitle("General") + .navigationTitle(Self.title) // The OS owns the login-item state and the user can change it in // System Settings, so re-read it whenever this pane comes back on // screen. The Settings window keeps its hierarchy across opens. @@ -134,11 +138,18 @@ private struct GeneralSettingsView: View { /// Repository discovery: where to scan for git repositories. The value is /// read-only here; editing happens in an explicit-commit sheet so switching /// panes can't drop a half-typed path. -private struct RepositoriesSettingsView: View { +private struct RepositoriesSettingsPane: SettingsPane { + static let title = "Repositories" + static let icon = "folder" + let session: ForemanSession @State private var isEditing = false + init(session: ForemanSession) { + self.session = session + } + var body: some View { Form { LabeledContent("Scan directory") { @@ -152,7 +163,7 @@ private struct RepositoriesSettingsView: View { .foregroundStyle(.secondary) } .formStyle(.grouped) - .navigationTitle("Repositories") + .navigationTitle(Self.title) .sheet(isPresented: $isEditing) { PathEditorSheet( title: "Scan directory", @@ -167,12 +178,19 @@ private struct RepositoriesSettingsView: View { } /// Agent settings: which `cursor-agent` executable to run. Read-only here; -/// edited via an explicit-commit sheet (see `RepositoriesSettingsView`). -private struct AgentSettingsView: View { +/// edited via an explicit-commit sheet (see `RepositoriesSettingsPane`). +private struct AgentSettingsPane: SettingsPane { + static let title = "Agent" + static let icon = "terminal" + let session: ForemanSession @State private var isEditing = false + init(session: ForemanSession) { + self.session = session + } + var body: some View { Form { LabeledContent("cursor-agent") { @@ -188,7 +206,7 @@ private struct AgentSettingsView: View { .foregroundStyle(.tertiary) } .formStyle(.grouped) - .navigationTitle("Agent") + .navigationTitle(Self.title) .sheet(isPresented: $isEditing) { PathEditorSheet( title: "cursor-agent executable",