Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses two UX/robustness areas in KeyScribe: (1) improving behavior around macOS sleep/wake permission gating, and (2) adding user-selectable visual themes for the floating assistant orb HUD.
Changes:
- Add an
NSWorkspace.didWakeNotificationhandler to re-runupdatePermissionGateafter wake with a short delay. - Stop active recordings when reconfiguring hotkeys to avoid losing stop callbacks/state during hotkey manager teardown.
- Add an orb “Theme” context menu and plumb the selected theme through
AssistantOrbHUDViewintoOrbSphererendering.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| Sources/KeyScribe/Assistant/AssistantOrbHUD.swift | Adds orb theme selection via context menu + persists theme in UserDefaults/@AppStorage, and updates orb rendering accordingly. |
| Sources/KeyScribe/App.swift | Adds wake observer to re-evaluate permissions post-wake and adjusts hotkey reconfiguration to stop active recordings first. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| case standard = "Standard" | ||
| case solarEclipse = "Solar Eclipse" | ||
| case bloodMoon = "Blood Moon" |
There was a problem hiding this comment.
OrbTheme uses user-facing display strings as raw values (e.g., "Solar Eclipse"), and those raw values are also persisted via UserDefaults/@AppStorage. This makes the stored preference brittle (renames/localization would break existing settings). Consider persisting a stable identifier (e.g., case name) and exposing a separate displayName for menu/UI text.
| case standard = "Standard" | |
| case solarEclipse = "Solar Eclipse" | |
| case bloodMoon = "Blood Moon" | |
| case standard | |
| case solarEclipse | |
| case bloodMoon | |
| /// User-facing name for display in menus/UI. Raw value is a stable identifier for persistence. | |
| var displayName: String { | |
| switch self { | |
| case .standard: | |
| return "Standard" | |
| case .solarEclipse: | |
| return "Solar Eclipse" | |
| case .bloodMoon: | |
| return "Blood Moon" | |
| } | |
| } |
| let themeItem = NSMenuItem(title: "Theme", action: nil, keyEquivalent: "") | ||
| let themeMenu = NSMenu() | ||
| let currentThemeStr = UserDefaults.standard.string(forKey: "AssistantOrbTheme") ?? OrbTheme.standard.rawValue | ||
|
|
There was a problem hiding this comment.
The UserDefaults key "AssistantOrbTheme" is duplicated across the NSMenu code and @AppStorage. To reduce the chance of typos/drift, define a single constant for this key (or route through an existing settings/keys namespace if the codebase has one) and reuse it everywhere.
| NSWorkspace.shared.notificationCenter.addObserver( | ||
| self, | ||
| selector: #selector(systemDidWake(_:)), | ||
| name: NSWorkspace.didWakeNotification, | ||
| object: nil | ||
| ) | ||
|
|
There was a problem hiding this comment.
NSWorkspace.didWakeNotification is registered using the selector-based API but isn't removed during shutdown (applicationShouldTerminate currently removes other workspace observers). This can leave a dangling observer pattern and is inconsistent with the token-based observers used elsewhere in this method; store the returned token (using addObserver(forName:queue:using:)) and remove it on termination, or explicitly removeObserver(self,name:object:).
| NSWorkspace.shared.notificationCenter.addObserver( | |
| self, | |
| selector: #selector(systemDidWake(_:)), | |
| name: NSWorkspace.didWakeNotification, | |
| object: nil | |
| ) | |
| let wakeObserver = NSWorkspace.shared.notificationCenter.addObserver( | |
| forName: NSWorkspace.didWakeNotification, | |
| object: nil, | |
| queue: .main | |
| ) { [weak self] notification in | |
| self?.systemDidWake(notification) | |
| } | |
| NotificationCenter.default.addObserver( | |
| forName: NSApplication.willTerminateNotification, | |
| object: nil, | |
| queue: .main | |
| ) { _ in | |
| NSWorkspace.shared.notificationCenter.removeObserver(wakeObserver) | |
| } |
| // If a recording is active, stop it cleanly before destroying | ||
| // the manager that would otherwise deliver the onStop callback. | ||
| if isDictating { | ||
| stopRecording() | ||
| dictationInputMode = .idle | ||
| } |
There was a problem hiding this comment.
The new comment says the recording is stopped "before destroying the manager" that would deliver the onStop callback, but the hotkey managers are already stopped/nilled a few lines above. Either move this stopRecording block before tearing down the managers (if the intent is to preserve callback ordering) or update the comment to reflect the actual rationale (stopping an in-flight recording during hotkey reconfiguration).
No description provided.