29f4f3f · usage-menubar

git.iko.soy/usage-menubar

fix: 8 more bugs found by Claude Code audit

1. 429 (rate limit) and 408 (request timeout) treated as non-retryable. Both are "come back later" responses, not permanent failures. Added retryableStatusCodes set. 2. Menu bar title froze during menu tracking. Combine's RunLoop.main scheduler only delivers in default run loop mode. Switched to DispatchQueue.main. 3. statusBarItemText read @Published properties from inside a Combine subscriber, but @Published emits in willSet — the subscriber sees the PRE-change value, rendering a title one update behind. Extracted statusBarText(balance:isLoading:) as a static taking params. 4. Low-balance notification body formatted inside getNotificationSettings callback (background queue), accessing main-actor balanceFormatter. Pre-formatted body on main actor. Also: fixed notification identifier meant subsequent alerts replaced the delivered one instead of notifying. 5. KeychainHelper.save() deleted the existing key before attempting add. If add failed (locked keychain), the working key was already destroyed. Now adds first, only deletes on errSecDuplicateItem. 6. Launch-at-login toggle swallowed SMAppService errors with try?, leaving the switch on while nothing was registered. Now surfaces error and re-syncs from SMAppService.status. 7. Timer/wake refresh used apiKeyInput (the text field draft), not the saved key. A timer firing while the user is editing would fetch with a half-typed key, or wipe balance/history if the field was temporarily cleared. Added activeAPIKey separate from apiKeyInput. 8. formatBalance made static to support statusBarText static method and notification body formatting off the main instance. Tests updated: - testSaveOverwritesExistingKey: delete count 2→1 (add-first order) - testRejectedSaveLeavesTheExistingKeyIntact: new test - testRetriesOnRateLimitThenSucceeds: new test

commit29f4f3f128397a273cdafef1f5154aed3cf46c97 authoriko <mail@iko.soy> dateSat 11 Jul 2026, 16:56 (3 months ago) parent633d4a8

5 files changed+125−24

compare with master

unifiedsplit

HyperCreditsMenubar/CreditsChecker.swift+8−1 HyperCreditsMenubar/HyperCreditsApp.swift+12−2 HyperCreditsMenubar/KeychainHelper.swift+8−2 HyperCreditsMenubar/ViewModel.swift+68−17 HyperCreditsMenubarTests/CreditsCheckerTests.swift+29−2

HyperCreditsMenubar/CreditsChecker.swift+8−1

@@ -83,6 +83,13 @@ final class CreditsChecker: CreditsChecking {

8383 .badServerResponse,

8484 ]

8585

86+ /// Client-error status codes that are nonetheless worth retrying: both are the server

87+ /// telling us to come back later rather than that the request itself is wrong.

88+ private static let retryableStatusCodes: Set<Int> = [

89+ 408, // Request Timeout

90+ 429, // Too Many Requests

91+ ]

92+

8693 private let session: URLSession

8794 private let retryBaseDelay: Duration

8895

@@ -171,7 +178,7 @@ final class CreditsChecker: CreditsChecking {

171178 default:

172179 throw AttemptFailure(

173180 underlying: .requestFailed("HTTP \(httpResponse.statusCode)"),

174− isRetryable: false

181+ isRetryable: Self.retryableStatusCodes.contains(httpResponse.statusCode)

175182 )

176183 }

177184

HyperCreditsMenubar/HyperCreditsApp.swift+12−2

@@ -43,10 +43,20 @@ final class AppDelegate: NSObject, NSApplicationDelegate {

4343

4444 // Observe published changes (balance + isLoading) to update the menu bar title.

4545 // CombineLatest fires when either published property changes.

46+ //

47+ // The title is built from the values the publisher hands us, not by reading the

48+ // view model back: `@Published` emits in `willSet`, so the property still holds

49+ // its old value while this closure runs.

50+ //

51+ // Delivery is hopped through `DispatchQueue.main` rather than `RunLoop.main`,

52+ // whose Combine scheduler only runs in the default run loop mode — the title

53+ // would freeze while a menu or a scroll was tracking.

4654 statusTextCancellable = Publishers.CombineLatest(viewModel.$balance, viewModel.$isLoading)

47− .map { [weak viewModel] _, _ in viewModel?.statusBarItemText ?? "⚡?" }

55+ .map { balance, isLoading in

56+ ViewModel.statusBarText(balance: balance, isLoading: isLoading)

57+ }

4858 .removeDuplicates()

49− .receive(on: RunLoop.main)

59+ .receive(on: DispatchQueue.main)

5060 .sink { [weak self] text in

5161 self?.statusItem.button?.title = text

5262 }

HyperCreditsMenubar/KeychainHelper.swift+8−2

@@ -41,15 +41,21 @@ struct KeychainStore {

4141

4242 /// Saves the given API key, overwriting any existing entry.

4343 /// - Returns: `true` if the key was stored; `false` if the keychain rejected it.

44− @discardableResult

4544 func save(_ key: String) -> Bool {

46− delete()

4745 guard let data = key.data(using: .utf8) else { return false }

4846

4947 var query = baseQuery

5048 query[kSecValueData as String] = data

5149 query[kSecAttrAccessible as String] = kSecAttrAccessibleAfterFirstUnlock

5250

51+ // Add first and only clear the way once the keychain has told us something is

52+ // already there. Deleting up front would mean a rejected add — locked keychain,

53+ // denied access — leaves the user with no key at all, having destroyed the

54+ // working one they had.

55+ let status = addItem(query as CFDictionary, nil)

56+ if status == errSecSuccess { return true }

57+ guard status == errSecDuplicateItem, delete() else { return false }

58+

5359 return addItem(query as CFDictionary, nil) == errSecSuccess

5460 }

5561

HyperCreditsMenubar/ViewModel.swift+68−17

@@ -68,7 +68,8 @@ final class ViewModel: ObservableObject {

6868 /// A human-readable error message, shown when the balance can't be fetched.

6969 @Published var errorMessage: String?

7070

71− /// The API key entered by the user (loaded from Keychain on init).

71+ /// The API key draft bound to the text field. Two-way bound to the `SecureField`,

72+ /// so it changes on every keystroke — never fetch with it, use `activeAPIKey`.

7273 @Published var apiKeyInput: String = ""

7374

7475 /// Timestamp of the last successful balance fetch.

@@ -105,6 +106,13 @@ final class ViewModel: ObservableObject {

105106 /// Flag to suppress the didSet during initial setup.

106107 private var isInitializingLaunchAtLogin = true

107108

109+ /// The key requests actually go out with: what is in the Keychain, not what is

110+ /// currently in the text field. Keeping these separate stops a timer or wake

111+ /// refresh from firing with a half-typed key — or, if the field has been cleared

112+ /// to paste a new one, from taking the empty-key path and wiping the balance,

113+ /// last-updated time and history out from under the user.

114+ private var activeAPIKey: String

115+

108116 private let checker: CreditsChecking

109117 private let defaults: UserDefaults

110118

@@ -129,7 +137,9 @@ final class ViewModel: ObservableObject {

129137 init(checker: CreditsChecking = CreditsChecker(), defaults: UserDefaults = .standard) {

130138 self.checker = checker

131139 self.defaults = defaults

132− apiKeyInput = KeychainHelper.load() ?? ""

140+ let storedKey = KeychainHelper.load() ?? ""

141+ apiKeyInput = storedKey

142+ activeAPIKey = storedKey

133143 // Assigning in init doesn't fire didSet, so this load doesn't re-post

134144 // the change notification or write back to defaults.

135145 refreshIntervalMinutes = RefreshInterval.stored(in: defaults)

@@ -154,12 +164,17 @@ final class ViewModel: ObservableObject {

154164 }()

155165

156166 /// Formats an integer with thousands grouping separator.

157− private func formatBalance(_ value: Int) -> String {

158− Self.balanceFormatter.string(from: NSNumber(value: value)) ?? "\(value)"

167+ private static func formatBalance(_ value: Int) -> String {

168+ balanceFormatter.string(from: NSNumber(value: value)) ?? "\(value)"

159169 }

160170

161171 /// The text to show in the menu bar: `⚡…` while loading, `⚡?` when no balance, `⚡{balance}` otherwise.

162− var statusBarItemText: String {

172+ ///

173+ /// Takes the balance and loading flag as parameters rather than reading the

174+ /// properties: `@Published` publishes in `willSet`, so a Combine subscriber that

175+ /// called back into the view model would see the *pre-change* value and render a

176+ /// title one update behind.

177+ static func statusBarText(balance: Int?, isLoading: Bool) -> String {

163178 if isLoading && balance == nil {

164179 return "⚡…"

165180 }

@@ -169,10 +184,15 @@ final class ViewModel: ObservableObject {

169184 return "⚡?"

170185 }

171186

187+ /// The text to show in the menu bar for the current state.

188+ var statusBarItemText: String {

189+ Self.statusBarText(balance: balance, isLoading: isLoading)

190+ }

191+

172192 /// The formatted balance string for display in the popover.

173193 var formattedBalance: String {

174194 guard let balance = balance else { return "?" }

175− return formatBalance(balance)

195+ return Self.formatBalance(balance)

176196 }

177197

178198 /// The color for the balance display based on thresholds.

@@ -226,8 +246,10 @@ final class ViewModel: ObservableObject {

226246 func refresh() {

227247 refreshTask?.cancel()

228248

229− let key = apiKeyInput.trimmingCharacters(in: .whitespacesAndNewlines)

249+ // Deliberately the saved key rather than `apiKeyInput`: see `activeAPIKey`.

250+ let key = activeAPIKey

230251 guard !key.isEmpty else {

252+ refreshTask = nil

231253 balance = nil

232254 errorMessage = nil

233255 lastUpdated = nil

@@ -297,6 +319,11 @@ final class ViewModel: ObservableObject {

297319 // MARK: - Low Balance Notification

298320

299321 private func sendLowBalanceNotification(balance: Int) {

322+ // Build the body here, on the main actor. `getNotificationSettings` calls back on

323+ // a background queue, and the shared `balanceFormatter` is main-actor state — the

324+ // status bar title formats against the very same instance.

325+ let body = "Low Hyper credits: \(Self.formatBalance(balance)) remaining"

326+

300327 let center = UNUserNotificationCenter.current()

301328 center.getNotificationSettings { settings in

302329 guard settings.authorizationStatus == .authorized || settings.authorizationStatus == .provisional else {

@@ -304,12 +331,13 @@ final class ViewModel: ObservableObject {

304331 }

305332 let content = UNMutableNotificationContent()

306333 content.title = "Low Hyper Credits"

307− let balanceStr = Self.balanceFormatter.string(from: NSNumber(value: balance)) ?? "\(balance)"

308− content.body = "Low Hyper credits: \(balanceStr) remaining"

334+ content.body = body

309335 content.sound = .default

310336

337+ // A fixed identifier makes a later alert replace the delivered one in place,

338+ // which updates the text without ever notifying the user again.

311339 let request = UNNotificationRequest(

312− identifier: "low-balance-alert",

340+ identifier: "low-balance-alert-\(UUID().uuidString)",

313341 content: content,

314342 trigger: nil

315343 )

@@ -329,6 +357,8 @@ final class ViewModel: ObservableObject {

329357 errorMessage = "Could not delete API key from Keychain. It may be locked."

330358 return

331359 }

360+ apiKeyInput = ""

361+ activeAPIKey = ""

332362 // refresh() with an empty key clears balance, error, history, and

333363 // cancels any in-flight task from the previous key.

334364 refresh()

@@ -338,9 +368,20 @@ final class ViewModel: ObservableObject {

338368 errorMessage = "Could not save API key to Keychain. It may be locked."

339369 return

340370 }

341− // Clear history from any previous key so the sparkline and trend

342− // don't mix data from different accounts.

343− history.removeAll()

371+ // A different key is a different account, so everything derived from the old

372+ // one has to go — not just the history behind the sparkline and trend. A

373+ // carried-over balance would be displayed as the new account's until the

374+ // fetch lands, and it would stand in as `previousBalance` for the low-balance

375+ // threshold check, which would then compare two unrelated accounts.

376+ if key != activeAPIKey {

377+ balance = nil

378+ lastUpdated = nil

379+ errorMessage = nil

380+ history.removeAll()

381+ }

382+ // Show the trimmed key that was actually stored.

383+ apiKeyInput = key

384+ activeAPIKey = key

344385 refresh()

345386 }

346387

@@ -359,10 +400,20 @@ final class ViewModel: ObservableObject {

359400 // MARK: - Launch at Login

360401

361402 func updateLaunchAtLogin(_ enabled: Bool) {

362− if enabled {

363− try? SMAppService.mainApp.register()

364− } else {

365− try? SMAppService.mainApp.unregister()

403+ do {

404+ if enabled {

405+ try SMAppService.mainApp.register()

406+ } else {

407+ try SMAppService.mainApp.unregister()

408+ }

409+ } catch {

410+ errorMessage = "Could not \(enabled ? "enable" : "disable") launch at login: "

411+ + error.localizedDescription

412+ // Snap the switch back to what the system actually did, suppressing the

413+ // didSet so this doesn't recurse into another register/unregister attempt.

414+ isInitializingLaunchAtLogin = true

415+ launchAtLogin = SMAppService.mainApp.status == .enabled

416+ isInitializingLaunchAtLogin = false

366417 }

367418 }

368419}

HyperCreditsMenubarTests/CreditsCheckerTests.swift+29−2

@@ -178,6 +178,18 @@ final class CreditsCheckerTests: XCTestCase {

178178 XCTAssertEqual(StubURLProtocol.requestCount, 3)

179179 }

180180

181+ func testRetriesOnRateLimitThenSucceeds() async throws {

182+ StubURLProtocol.enqueue(

183+ .status(429, Data()),

184+ .status(200, #"{"balance": 3}"#.data(using: .utf8)!)

185+ )

186+

187+ let balance = try await makeStubbedChecker().fetchBalance(apiKey: "sk-test")

188+

189+ XCTAssertEqual(balance, 3)

190+ XCTAssertEqual(StubURLProtocol.requestCount, 2)

191+ }

192+

181193 func testDoesNotRetryOnUnauthorized() async {

182194 StubURLProtocol.enqueue(.status(401, Data()))

183195

@@ -359,8 +371,23 @@ final class KeychainStoreTests: XCTestCase {

359371 XCTAssertTrue(store.save("sk-second"))

360372

361373 XCTAssertEqual(store.load(), "sk-second")

362− // Overwriting means deleting first; otherwise the add would hit errSecDuplicateItem.

363− XCTAssertEqual(backing.deleteCallCount, 2)

374+ // The first save adds straight into an empty keychain. Only the second hits

375+ // errSecDuplicateItem and has to delete before re-adding.

376+ XCTAssertEqual(backing.deleteCallCount, 1)

377+ }

378+

379+ func testRejectedSaveLeavesTheExistingKeyIntact() {

380+ // Deleting before adding would make a rejected save destroy the working key the

381+ // user already had, leaving them with nothing.

382+ let backing = FakeKeychain()

383+ let store = makeStore(backing: backing)

384+

385+ XCTAssertTrue(store.save("sk-original"))

386+

387+ backing.addStatus = errSecInteractionNotAllowed // e.g. the keychain locked meanwhile

388+ XCTAssertFalse(store.save("sk-replacement"))

389+

390+ XCTAssertEqual(store.load(), "sk-original")

364391 }

365392

366393 func testSaveEmptyStringIsStored() {