fix: sync onboarding hotkey mode immediately - #133
Conversation
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
There was a problem hiding this comment.
@d1scolor Nice fix, the bug is real and centralising the sync path is the right shape. A few inline nits, nothing blocking.
|
@jatinkrmalik thanks for the feedback, all addressed. |
There was a problem hiding this comment.
@d1scolor Approved with one safety nit.
The shared syncHotKeyConfiguration() path is the right shape for keeping onboarding, settings, and startup aligned, and the test additions cover the important configuration tuple.
I left one inline comment on resetting hotkey key state during onboarding completion while a recording may still be active.
The setup wizard saved activation mode changes through @AppStorage, but the global hotkey listener could already be running with the default push-to-talk configuration. That left first-time users in a confusing state where double-tap was saved in preferences while the live event tap still behaved as press-to-talk until the app restarted. Add AppState.syncHotKeyConfiguration() as the shared path for applying persisted hotkey settings to the active HotKeyManager. Onboarding now refreshes the live listener when activation mode, hotkey, or double-tap threshold changes, and completeOnboarding() applies the current configuration again before marking setup complete. Settings now uses the same sync path, including max recording duration so the safety timeout remains aligned. Tests extend the hotkey mock to record configuration updates and cover both direct sync and onboarding completion sync. Local verification: swift test was attempted; the sandboxed run could not write SwiftPM caches, the escalated debug run was blocked by the existing SwiftUI #Preview macro/PreviewsMacros command-line toolchain issue, and the release test run was blocked by local XCTest availability. Manual app testing confirmed the setup wizard hotkey mode issue is fixed.
The PR review called out a few places where the hotkey listener sync path could drift or create avoidable noise. This follow-up keeps the live listener updates focused while preserving the immediate onboarding behavior the original fix introduced. Centralize the hotkey safety timeout calculation so startup, permission-grant startup, and explicit syncs all use the same max-recording-duration slack. Demote the sync log to debug, sync double-tap threshold sliders only when editing commits, and document that onboarding's live onChange handlers cover only the fields shown in that step. Also reset the hotkey key-state when onboarding completes, and extend AppState tests to cover the default sync tuple plus the completion reset. Caveat: the first sandboxed swift test run could not write SwiftPM/Clang caches; the escalated run completed successfully. Verification: swift test (171 tests, 0 failures).
Completing onboarding syncs the latest hotkey configuration and then clears stale key state, but resetting key state while a recording is active can cancel the push-to-talk safety timer. That creates a narrow path where a held-key release could be ignored and the safety backstop would be unavailable until the audio engine max-duration callback fires. Keep the configuration sync on completion, but only reset hotkey key state when AppState is not recording. Add onboarding coverage that locks in the active-recording behavior so the hotkey reset path cannot regress silently. Verification: swift test --filter AppStateOnboardingTests; swift test.
jatinkrmalik
left a comment
jatinkrmalik
left a comment
There was a problem hiding this comment.
Overall assessment: merge-ready from my review. I rebased the branch onto current origin/main (9b76574) and pushed the new head 9169492 with --force-with-lease. Re-fix check: I found related prior work around custom hotkeys, onboarding, and max-recording safety timers, but I did not find an earlier fix for this specific first-run onboarding live-listener sync issue. Local verification passed with swift test --filter AppStateOnboardingTests (8 tests, 0 failures) and swift test (176 tests, 1 skipped for local Accessibility permission, 0 failures). GitHub currently reports the PR as mergeable, with no checks reported on the branch and review still required.
| /// Apply persisted hotkey settings to the active listener. | ||
| /// `@AppStorage` updates save preferences immediately, but an already-running | ||
| /// event tap also needs its in-memory configuration refreshed. | ||
| func syncHotKeyConfiguration() { |
There was a problem hiding this comment.
syncHotKeyConfiguration() is the right shared entry point here. It keeps onboarding, settings, startup, and permission-grant behavior aligned without pushing more HotKeyManager details into the views.
| } | ||
|
|
||
| @MainActor | ||
| func testCompleteOnboardingSyncsHotKeyConfiguration() { |
There was a problem hiding this comment.
Good coverage for the original drift: persisted hotkey settings now have an explicit assertion that they are applied to the live listener during onboarding completion.
2 participants
The setup wizard saved activation mode changes through
@AppStorage, but the global hotkey listener could already be running with the default push-to-talk configuration. That left first-time users in a confusing state where double-tap was saved in preferences while the live event tap still behaved as press-to-talk until the app restarted.Add AppState.syncHotKeyConfiguration() as the shared path for applying persisted hotkey settings to the active HotKeyManager. Onboarding now refreshes the live listener when activation mode, hotkey, or double-tap threshold changes, and completeOnboarding() applies the current configuration again before marking setup complete. Settings now uses the same sync path, including max recording duration so the safety timeout remains aligned.
Tests extend the hotkey mock to record configuration updates and cover both direct sync and onboarding completion sync. Local verification: swift test was attempted; the sandboxed run could not write SwiftPM caches, the escalated debug run was blocked by the existing SwiftUI #Preview macro/PreviewsMacros command-line toolchain issue, and the release test run was blocked by local XCTest availability. Manual app testing confirmed the setup wizard hotkey mode issue is fixed.