fix(settings): honor empty key-mapping list, survive duplicate exclusion IDs, follow OS login-item state, inert new mapping rows#124
Merged
Conversation
…ion IDs, follow OS login-item state, inert new mapping rows - Bug 1: loadKeyMappings(from:) now treats any stored array as authoritative, including an empty one, instead of falling back to factory defaults when the user removes every mapping. - Bug 2: loadExclusionApps(from:) de-dups by id (keep first occurrence), and both Dictionary(uniqueKeysWithValues:) call sites (the $exclusionApps sink and publishGlobalsFromState()) now use uniquingKeysWith so a duplicate id in persisted data can never trap. - Bug 3: bootstrap() now follows the OS state symmetrically, but distinguishes *why* the service isn't enabled via SMAppService.mainApp.status instead of treating every non-enabled state the same: `.requiresApproval` (registered, but the user disabled/hasn't approved it in System Settings → Login Items) syncs the stored toggle to "off" (via the isApplyingExternalUpdate re-entrancy guard); `.notRegistered` / `.notFound` / any unknown future status (never registered at all — e.g. a legacy-migration user whose old `lunchAtStartup=true` was carried over but who never went through SMAppService registration) honors the stored intent and registers instead, restoring the pre-fix behavior for that specific case only. - Bug 4: addKeyMapping() appends a disabled row (KeyMapping()'s default shortcut is keyCode 0 / "A"), and updateKeyMapping(at:) only re-enables it once both input and output have been explicitly set through the preset menus (added KeyboardShortcut.isUnset), so a half-configured row can never act as a live A -> A remap. - Bug 5: removing an excluded app now calls reloadRecent() so it reappears in "Recently active" immediately. - Bug 6: ShortcutsSettingsView.isShadowed(_:at:) flags a later enabled row with an inline warning icon when an earlier enabled row already has the identical input shortcut, since findMapping is first-match-wins. Extends AppSettingsTests.swift and adds ShortcutsSettingsViewTests.swift covering all six fixes; swift test and swiftlint pass with no new violations. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Six fixes in the settings/persistence layer:
loadKeyMappingstreated a stored empty array like "never written" and fell back to factory defaults, so removing every mapping didn't survive a relaunch. A stored array is now authoritative, including empty (matchingloadExclusionAppssemantics).Dictionary(uniqueKeysWithValues:)traps on duplicate keys and ran unconditionally ininit, so one duplicate entry in persisted data crashed every launch with no recovery path.loadExclusionAppsnow de-dups by id (first wins) and both dictionary sites useuniquingKeysWith:.bootstrap()force re-registered whenever the stored toggle was on but the service wasn't enabled, reverting a user who disabled ⌘IME in System Settings → Login Items. It now switches onSMAppService.mainApp.status:.requiresApproval(registered, user disabled/unapproved) follows the OS and syncs the toggle off;.notRegistered/.notFound(never registered — e.g. legacylunchAtStartupmigration) still honors stored intent and registers.KeyboardShortcut()'s default keyCode 0 is the real "A" key and new rows were enabled, so a half-configured row (input set to Right ⌘, action forgotten) made Right Command type a literal "A". New rows start disabled and auto-enable only once both input and output have been explicitly chosen (newKeyboardShortcut.isUnset; the view has no per-row enable toggle).reloadRecent()so it reappears immediately.findMappingis first-match-wins; a later enabled row with an identical input now shows an inline warning icon with a tooltip instead of silently never firing.Tests
swift test: 42 executed, 0 failures (1 pre-existing unrelated skip). New coverage: empty-vs-missing key-mapping persistence, duplicate exclusion de-dup, bootstrap in the (only reachable in-process).notFoundstate,isUnset/auto-enable transitions, andisShadowedlogic in a newShortcutsSettingsViewTests.swift. The.requiresApprovalbootstrap branch has no automated coverage —SMAppServicehas no injection seam and the test process reports.notFound; noted in a test comment.🤖 Generated with Claude Code