Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ final class AppSettings: ObservableObject {
}

private let defaults: UserDefaults
private let loginItemService: LoginItemService

@Published var launchAtStartup: Bool
@Published var showMenuBarIcon: Bool
Expand All @@ -49,8 +50,9 @@ final class AppSettings: ObservableObject {
private var cancellables: Set<AnyCancellable> = []
private var isApplyingExternalUpdate = false

init(defaults: UserDefaults = .standard) {
init(defaults: UserDefaults = .standard, loginItemService: LoginItemService = SMAppService.mainApp) {
self.defaults = defaults
self.loginItemService = loginItemService

Self.migrateLegacyKeys(in: defaults)

Expand All @@ -60,7 +62,7 @@ final class AppSettings: ObservableObject {
self.quitOnCommandQ = (defaults.object(forKey: Keys.quitOnCommandQ) as? Int ?? 0) != 0

let stored = (defaults.object(forKey: Keys.launchAtStartup) as? Int ?? 0) != 0
let serviceEnabled = SMAppService.mainApp.status == .enabled
let serviceEnabled = loginItemService.status == .enabled
self.launchAtStartup = stored || serviceEnabled

self.keyMappings = Self.loadKeyMappings(from: defaults)
Expand All @@ -74,13 +76,13 @@ final class AppSettings: ObservableObject {
/// Call once on app launch to reconcile any drift between SMAppService
/// (the OS-side login item registry) and our stored toggle.
func bootstrap() {
let serviceEnabled = SMAppService.mainApp.status == .enabled
let serviceEnabled = loginItemService.status == .enabled
if launchAtStartup && !serviceEnabled {
// Stored intent is "on" but the OS is not enabled. Which way to
// resolve this depends on *why* the service isn't enabled, so
// switch on the actual status instead of treating every
// non-enabled state the same:
switch SMAppService.mainApp.status {
switch loginItemService.status {
case .requiresApproval:
// The item IS registered with SMAppService, but the user
// disabled it (or hasn't approved it yet) in System
Expand All @@ -105,7 +107,7 @@ final class AppSettings: ObservableObject {
// SMAppService registration. Honor the stored intent and
// register now — this restores the pre-fix behavior for
// this specific case only.
setLaunchAtStartup(true)
setLaunchAtStartup(true, service: loginItemService)
}
} else if !launchAtStartup && serviceEnabled {
// OS already registered us (e.g. enabled in System Settings) but
Expand Down Expand Up @@ -163,7 +165,7 @@ final class AppSettings: ObservableObject {
.sink { [weak self] newValue in
guard let self = self, !self.isApplyingExternalUpdate else { return }
self.defaults.set(newValue ? 1 : 0, forKey: Keys.launchAtStartup)
if !setLaunchAtStartup(newValue) {
if !setLaunchAtStartup(newValue, service: self.loginItemService) {
self.isApplyingExternalUpdate = true
self.launchAtStartup = !newValue
self.defaults.set(!newValue ? 1 : 0, forKey: Keys.launchAtStartup)
Expand Down
15 changes: 13 additions & 2 deletions apps/cmd-ime-swift/Sources/CmdIMESwift/toggleLaunchAtStartup.swift
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,20 @@
import Cocoa
import ServiceManagement

/// Seam over `SMAppService.mainApp` so tests can inject a fake instead of
/// hitting the real OS-level login item registry. `SMAppService` already has
/// matching `status`/`register()`/`unregister()` members, so it conforms for
/// free — see the extension below.
protocol LoginItemService {
var status: SMAppService.Status { get }
func register() throws
func unregister() throws
}

extension SMAppService: LoginItemService {}

@discardableResult
func setLaunchAtStartup(_ enabled: Bool) -> Bool {
let service = SMAppService.mainApp
func setLaunchAtStartup(_ enabled: Bool, service: LoginItemService = SMAppService.mainApp) -> Bool {
do {
if enabled {
guard service.status != .enabled else { return true }
Expand Down
34 changes: 25 additions & 9 deletions apps/cmd-ime-swift/Tests/CmdIMESwiftTests/AppSettingsTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -156,22 +156,38 @@ final class AppSettingsTests: XCTestCase {
}

func testBootstrapWithStoredTrueAndServiceNotFoundRegistersInstead() {
// SMAppService.Status isn't injectable here, so this only exercises
// whatever status the real API reports for the `swift test` process
// (an unsigned, unregistered binary — not a proper .app bundle).
// That is reliably `.notFound` (confirmed empirically), which is the
// "never registered" branch: bootstrap() must honor the stored
// intent and attempt to register, NOT flip the toggle off. The
// `.requiresApproval` branch (registered but user-disabled) isn't
// reachable from this test process and has no automated coverage.
// Never-registered case (.notFound, the real status a bare
// `swift test` process reported before this test used a fake — see
// FakeLoginItemService): bootstrap() must honor the stored intent
// and attempt to register, NOT flip the toggle off.
let fakeService = FakeLoginItemService(status: .notFound)
defaults.set(1, forKey: "launchAtStartup")
let settings = AppSettings(defaults: defaults)
let settings = AppSettings(defaults: defaults, loginItemService: fakeService)
XCTAssertTrue(settings.launchAtStartup, "precondition: stored value loaded as on")

settings.bootstrap()

XCTAssertTrue(settings.launchAtStartup, "never-registered case must honor stored intent, not follow OS off")
XCTAssertEqual(defaults.object(forKey: "launchAtStartup") as? Int, 1)
XCTAssertEqual(fakeService.registerCallCount, 1, "bootstrap() must register the never-registered case")
}

func testBootstrapWithStoredTrueAndRequiresApprovalFollowsOSStateInstead() {
// Registered but user-disabled (or not yet approved) in System
// Settings -> Login Items: bootstrap() must defer to the OS state,
// not force re-registration behind the user's back. This branch was
// previously unreachable from a real `swift test` process (see
// git history) and had no automated coverage.
let fakeService = FakeLoginItemService(status: .requiresApproval)
defaults.set(1, forKey: "launchAtStartup")
let settings = AppSettings(defaults: defaults, loginItemService: fakeService)
XCTAssertTrue(settings.launchAtStartup, "precondition: stored value loaded as on")

settings.bootstrap()

XCTAssertFalse(settings.launchAtStartup, "requiresApproval case must follow the OS state, not stored intent")
XCTAssertEqual(defaults.object(forKey: "launchAtStartup") as? Int, 0)
XCTAssertEqual(fakeService.registerCallCount, 0, "must not force re-registration behind the user's back")
}

func testExclusionMutationsPropagateToGlobals() {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
import ServiceManagement
@testable import CmdIMESwift

/// In-memory stand-in for `SMAppService.mainApp` (see LoginItemService in
/// toggleLaunchAtStartup.swift). Tests must never touch the real OS-level
/// login item registry: a prior bug had a test call the real API, which
/// registered the `xctest` test-runner binary itself as a login item on the
/// developer's Mac (see CLAUDE.md and AppSettingsTests.swift for the
/// incident this class exists to prevent).
final class FakeLoginItemService: LoginItemService {
var status: SMAppService.Status
private(set) var registerCallCount = 0
private(set) var unregisterCallCount = 0
var registerError: Error?
var unregisterError: Error?

init(status: SMAppService.Status = .notRegistered) {
self.status = status
}

func register() throws {
registerCallCount += 1
if let registerError { throw registerError }
status = .enabled
}

func unregister() throws {
unregisterCallCount += 1
if let unregisterError { throw unregisterError }
status = .notRegistered
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
import XCTest

/// Regression guard for the incident where a test called the real
/// `SMAppService.mainApp` API and registered the `xctest` test-runner binary
/// itself as a login item on the developer's Mac (bootstrap() ->
/// setLaunchAtStartup(true) -> SMAppService.mainApp.register()). Every test
/// must go through the injectable `LoginItemService` seam (see
/// toggleLaunchAtStartup.swift / FakeLoginItemService.swift) instead.
final class SMAppServiceTestIsolationTests: XCTestCase {
func testNoTestFileReferencesTheRealSMAppServiceMainApp() throws {
let testsDirectory = URL(fileURLWithPath: #filePath).deletingLastPathComponent()
let contents = try FileManager.default.contentsOfDirectory(
at: testsDirectory, includingPropertiesForKeys: nil
)
let swiftFiles = contents.filter { $0.pathExtension == "swift" }
XCTAssertFalse(swiftFiles.isEmpty, "sanity check: the test directory listing must not be empty")

for file in swiftFiles {
// FakeLoginItemService itself is a legitimate SMAppService.Status
// consumer (it stands in for the real type's status enum), and
// this file's own doc comment names the hazard string it scans
// for — neither is an actual live call.
guard file.lastPathComponent != "FakeLoginItemService.swift",
file.lastPathComponent != "SMAppServiceTestIsolationTests.swift" else { continue }
let contents = try String(contentsOf: file, encoding: .utf8)
XCTAssertFalse(contents.contains("SMAppService.mainApp"),
"\(file.lastPathComponent) references the real SMAppService.mainApp — " +
"inject FakeLoginItemService instead (see AppSettingsTests.swift)")
}
}
}
Loading