Refine desktop shortcuts and dashboard widgets#6107
Conversation
Greptile SummaryThis PR refines the desktop app's Settings UI by splitting keyboard shortcuts into a dedicated Shortcuts section, moving floating-bar appearance controls (background style, draggable bar) into the existing Floating Bar section, and simplifying the dashboard by centering the tasks widget and removing the Daily task creation button and AI goal-generation header controls. It also adds proper enable/disable states for the Ask Omi and Push-to-Talk shortcuts backed by Key changes:
Confidence Score: 5/5Safe to merge — all remaining findings are P2 style/UX suggestions with no correctness or data-integrity impact. The enable/disable logic is correct end-to-end: settings are persisted, GlobalShortcutManager re-reads them on each registration call, and PushToTalkManager gates events properly. The three findings are limited to a misleading button label, a redundant double-notification on preset selection, and an idempotent re-fire when Disable is clicked while already disabled — none affect data integrity or the primary user path. desktop/Desktop/Sources/MainWindow/Pages/ShortcutsSettingsSection.swift — "Save" label regression and double-notification pattern are worth a quick follow-up. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A([User selects shortcut option]) --> B{Which option?}
B -->|Preset key button| C[stopShortcutCapture\naskOmiEnabled = true\naskOmiShortcut = preset]
B -->|Custom button| D[askOmiEnabled = true\nstartShortcutCapture]
B -->|Disable button| E[stopShortcutCapture\naskOmiEnabled = false]
C --> F[didSet: post askOmiShortcutChanged x2]
D --> G[Local key monitor active\nUser presses key combo]
E --> H[didSet: post askOmiShortcutChanged]
G --> I{Valid shortcut?}
I -->|Yes| J[askOmiEnabled = true\naskOmiShortcut = captured\nstopShortcutCapture]
I -->|No| K[captureError shown]
J --> F
F --> L[GlobalShortcutManager\n.registerAskOmi]
H --> L
L --> M{askOmiEnabled?}
M -->|false| N[UnregisterEventHotKey\nreturn early]
M -->|true| O[UnregisterEventHotKey\nRegisterEventHotKey with new key]
P([PTT key event]) --> Q{pttEnabled?}
Q -->|false| R[return — event ignored]
Q -->|true| S[Process push-to-talk logic]
Reviews (1): Last reviewed commit: "Refine desktop shortcuts and dashboard w..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| Button(action: action) { | ||
| Text(recordingTarget != nil ? "Listening..." : "Change") | ||
| Text(isRecording ? "Listening..." : "Save") |
There was a problem hiding this comment.
When the shortcut recorder card is shown because askOmiUsesCustomShortcut (or pttUsesCustomShortcut) is true and no recording is in progress, isRecording is false and the button renders "Save". However, clicking "Save" does not commit the currently-displayed shortcut — it calls startShortcutCapture(target), which begins a new recording session. The old label "Change" accurately described this intent.
| Text(isRecording ? "Listening..." : "Save") | |
| Text(isRecording ? "Listening..." : "Change") |
This applies equally to the PTT shortcutRecorderCard call at line 109.
| return Button { | ||
| stopShortcutCapture() | ||
| settings.askOmiEnabled = true | ||
| settings.askOmiShortcut = shortcut |
There was a problem hiding this comment.
Both askOmiKeyButton and pttKeyButton now set two @Published properties sequentially:
settings.askOmiEnabled = true // didSet → posts askOmiShortcutChanged → GlobalShortcutManager.registerAskOmi() with OLD shortcut
settings.askOmiShortcut = shortcut // didSet → posts askOmiShortcutChanged → GlobalShortcutManager.registerAskOmi() with NEW shortcutregisterAskOmi() first calls UnregisterEventHotKey, then re-registers, so it runs twice: once binding the old shortcut key to the new handler, and once more with the correct new key. In practice this is harmless because the second call immediately corrects it, but the same double-notification pattern also occurs in handleShortcutCapture. Consider batching the two mutations to avoid the redundant registration call.
| private func disableShortcutButton(isDisabled: Bool, action: @escaping () -> Void) -> some View { | ||
| Button(action: action) { | ||
| Text("Disable") | ||
| .scaledFont(size: 13, weight: .medium) | ||
| .foregroundColor(OmiColors.textPrimary) | ||
| .padding(.horizontal, 14) | ||
| .padding(.vertical, 10) | ||
| .background( | ||
| RoundedRectangle(cornerRadius: 10) | ||
| .fill(isDisabled | ||
| ? OmiColors.purplePrimary.opacity(0.3) | ||
| : OmiColors.backgroundTertiary.opacity(0.5)) | ||
| ) | ||
| .overlay( | ||
| RoundedRectangle(cornerRadius: 10) | ||
| .stroke(isDisabled ? OmiColors.purplePrimary : Color.clear, lineWidth: 1.5) | ||
| ) | ||
| } | ||
| .buttonStyle(.plain) |
There was a problem hiding this comment.
The disableShortcutButton action unconditionally calls settings.askOmiEnabled = false (or pttEnabled = false). When the shortcut is already disabled (isDisabled == true), pressing the button again triggers didSet, which posts askOmiShortcutChanged and writes to UserDefaults unnecessarily. A simple guard prevents the redundant side-effects:
disableShortcutButton(isDisabled: !settings.askOmiEnabled) {
guard settings.askOmiEnabled else { return } // already disabled, no-op
stopShortcutCapture()
settings.askOmiEnabled = false
}The same applies to the PTT disable button.
1 participant
Summary
Verification
/Applications/onboarding-v14.appfrom this branchagent-swiftand verified the newShortcutssettings section is presentAI ModelandTranscription Modesettings are no longer exposed in the shortcuts UI