feat: support "None" option for keyboard shortcuts#86
feat: support "None" option for keyboard shortcuts#86peaske7 wants to merge 7 commits intoamicalhq:mainfrom
Conversation
📝 WalkthroughWalkthroughAdds clear/restore behavior to shortcut UI, persists previous shortcuts via a new usePreviousShortcut hook (backed by useLocalStorage), treats empty shortcuts as valid (disabled), and requires a new Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant UI as ShortcutInput (UI)
participant Validator as validateShortcutComprehensive
participant Hook as usePreviousShortcut
participant LS as localStorage
User->>UI: Click "Clear" (X)
UI->>Hook: savePrevious(currentKeys)
Hook->>LS: setItem("previous-shortcut-<id>", JSON)
Note right of LS: previous keys stored
User->>UI: Start recording / submit candidate
UI->>Validator: validateShortcutComprehensive(candidate, prevActiveKeys)
alt candidate is empty
Validator-->>UI: { valid: true }
UI->>UI: commit disabled (empty) shortcut
UI->>Hook: clearPrevious()
Hook->>LS: removeItem("previous-shortcut-<id>")
else valid non-empty
Validator-->>UI: { valid: true }
UI->>UI: commit shortcut
UI->>Hook: clearPrevious()
Hook->>LS: removeItem("previous-shortcut-<id>")
else invalid
Validator-->>UI: { valid: false, error }
UI-->>User: show validation error
end
User->>UI: Click "Undo" (Restore)
UI->>Hook: restorePrevious()
Hook->>LS: getItem("previous-shortcut-<id>")
Hook-->>UI: previousKeys (and clear storage)
UI-->>User: restored shortcut applied
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/desktop/src/hooks/useLocalStorage.ts`:
- Around line 7-10: The initialization of storedValue uses JSON.parse without
error handling, which will throw if localStorage has malformed JSON; update the
initializer in useLocalStorage to wrap the localStorage.getItem(key) and
JSON.parse call in a try/catch, return initialValue on parse errors (optionally
remove the bad entry with localStorage.removeItem(key) and/or log the error),
and ensure setStoredValue remains unchanged; reference storedValue,
setStoredValue, useLocalStorage, key, and initialValue to locate and modify the
initializer.
🧹 Nitpick comments (3)
apps/desktop/src/hooks/useLocalStorage.ts (1)
12-18: Consider guarding localStorage writes against quota/access errors.While less common,
localStorage.setItemcan throw (e.g., quota exceeded, private browsing mode). A silent catch would prevent runtime errors.🛡️ Optional defensive fix
useEffect(() => { - if (JSON.stringify(storedValue) === JSON.stringify(initialValue)) { - localStorage.removeItem(key); - } else { - localStorage.setItem(key, JSON.stringify(storedValue)); + try { + if (JSON.stringify(storedValue) === JSON.stringify(initialValue)) { + localStorage.removeItem(key); + } else { + localStorage.setItem(key, JSON.stringify(storedValue)); + } + } catch { + // Silently ignore storage errors (quota, private mode, etc.) } }, [key, storedValue, initialValue]);apps/desktop/src/hooks/usePreviousShortcut.ts (1)
4-10: Note: Single storage key limits undo to the most recently cleared shortcut.Using a single
"previous-shortcut"key means only one shortcut can be restored at a time. If a user clears "Push to talk" then clears "Hands-free mode", only the latter can be undone. This may be intentional given the PR describes the undo button as "optional UX flair," but consider whether per-shortcut undo keys would provide a better experience.apps/desktop/src/components/shortcut-input.tsx (1)
229-231: Variable shadowing:previousKeysshadows the hook'spreviousKeys.Line 230 declares
const previousKeys = activeKeys, which shadows thepreviousKeysfrom theusePreviousShortcuthook (line 192). While the current logic is correct, this naming collision is confusing and could lead to bugs during future maintenance.♻️ Proposed rename
onData: (keys: number[]) => { - const previousKeys = activeKeys; + const prevActiveKeys = activeKeys; setActiveKeys(keys); // When any key is released, validate the combination - if (previousKeys.length > 0 && keys.length < previousKeys.length) { - const result = validateShortcutFormat(previousKeys); + if (prevActiveKeys.length > 0 && keys.length < prevActiveKeys.length) { + const result = validateShortcutFormat(prevActiveKeys);
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/desktop/src/hooks/useLocalStorage.ts`:
- Around line 17-27: The hook useLocalStorage currently only initializes
storedValue once and doesn't rehydrate when key changes, causing stale in-memory
state to be written to a new key; move the initialization/rehydration into a
useEffect that runs on [key, initialValue] and inside it attempt to read
localStorage.getItem(key) and call setStoredValue(parsedValue || initialValue)
(fall back to initialValue on error) so the hook loads the correct value
whenever key or initialValue changes, while keeping the existing effect that
writes storedValue back to localStorage.
- Around line 8-13: The catch block in useLocalStorage currently calls
localStorage.removeItem(key) which can itself throw and crash; wrap that removal
in its own try/catch (or check availability) so failures during cleanup are
swallowed — specifically update the try/catch around localStorage.getItem in the
useLocalStorage hook and change the catch to attempt removal inside a nested
try/catch (or conditional) so removeItem failures do not propagate.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/components/shortcut-input.tsx (1)
219-243:⚠️ Potential issue | 🟠 MajorStale closure bug in subscription callback—
activeKeyswill not update.The code comment claims the closure is "fresh on each render," but tRPC's
useSubscriptiondoes not recreate callbacks on re-renders—it caches them. This meansactiveKeysat line 222 will always reference its initial value, breaking the key-release detection logic. The condition at line 226 will fail to trigger when keys are actually released.The misleading comment should be removed, and the code should use a ref to access the current state:
Fix: Use ref to access current activeKeys
+ import { useRef } from "react"; + + // Inside ShortcutInput component + const activeKeysRef = useRef<number[]>([]); + + useEffect(() => { + activeKeysRef.current = activeKeys; + }, [activeKeys]); + - // Subscribe to key events when recording - // Note: activeKeys closure is fresh on each render because useSubscription - // updates its callback reference, so previousKeys correctly captures the - // previous state value when onData fires. api.settings.activeKeysUpdates.useSubscription(undefined, { enabled: isRecordingShortcut, onData: (keys: number[]) => { - const prevActiveKeys = activeKeys; + const prevActiveKeys = activeKeysRef.current; setActiveKeys(keys);
🧹 Nitpick comments (2)
apps/desktop/src/components/shortcut-input.tsx (2)
135-142: Add aria-label to icon-only button for accessibility.The clear button uses only an icon without accessible text. Screen reader users won't know the button's purpose.
♿ Proposed fix
<Button variant="ghost" size="sm" className="h-6 w-6 p-0" onClick={onClear} + aria-label="Clear shortcut" > <X className="h-3 w-3" /> </Button>
162-169: Add aria-label to the undo button.Same accessibility concern as the clear button—the icon-only restore button needs an accessible label.
♿ Proposed fix
<Button variant="ghost" size="sm" className="h-6 w-6 p-0" onClick={onRestore} + aria-label="Restore previous shortcut" > <Undo2 className="h-3 w-3" /> </Button>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/components/shortcut-input.tsx (1)
194-203:⚠️ Potential issue | 🟠 MajorReset
activeKeysRefsynchronously inhandleStartRecordingto prevent stale key validation.
activeKeysRefis only synced via a dependency effect (line 220), not synchronously. WhenhandleStartRecording()triggersisRecordingShortcut = true, the subscription (line 225) can be enabled and fire before the reset effect (line 252) clears the state. If a user presses a combo in session A but holds a modifier, then immediately starts session B, the subscription callback can read stale keys from the previous session before they are cleared, causing the validation logic (line 232) to incorrectly finalize the old combo.Clear both
activeKeysstate andactiveKeysRefref synchronously inhandleStartRecording()before re-enabling recording, and also inhandleCancelRecording()(which currently only resets state, not the ref).🔧 Suggested fix
const handleStartRecording = () => { + setActiveKeys([]); + activeKeysRef.current = []; onRecordingShortcutChange(true); setRecordingStateMutation.mutate(true); }; const handleCancelRecording = () => { onRecordingShortcutChange(false); setActiveKeys([]); + activeKeysRef.current = []; setRecordingStateMutation.mutate(false); };
🤖 Fix all issues with AI agents
In `@apps/desktop/src/components/shortcut-input.tsx`:
- Around line 258-260: ShortcutInput currently returns null when prop value is
undefined, causing blank UI while ShortcutsSettingsPage loads async data; change
the behavior so the UI shows a placeholder instead of nothing or ensure the
parent provides a default empty array. Either (A) update ShortcutsSettingsPage
to initialize its shortcuts state to [] (match OnboardingShortcutInput) so
ShortcutInput always receives an array, or (B) modify ShortcutInput (component
ShortcutInput in shortcut-input.tsx) to render a lightweight loading
skeleton/placeholder when value === undefined rather than returning null,
keeping existing behavior for real empty arrays.
| if (value === undefined) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all ShortcutInput call sites
rg -n --type=tsx -C2 '<ShortcutInput'Repository: amicalhq/amical
Length of output: 86
🏁 Script executed:
#!/bin/bash
# Find all ShortcutInput call sites - search in all files
rg -n '<ShortcutInput' -A 2 -B 2Repository: amicalhq/amical
Length of output: 2450
🏁 Script executed:
#!/bin/bash
# First, check the ShortcutInput component definition in the target file
cat -n apps/desktop/src/components/shortcut-input.tsx | head -100Repository: amicalhq/amical
Length of output: 3348
🏁 Script executed:
#!/bin/bash
# Check lines around 258-260
sed -n '250,270p' apps/desktop/src/components/shortcut-input.tsx | cat -nRepository: amicalhq/amical
Length of output: 679
🏁 Script executed:
#!/bin/bash
# Check the settings file for state initialization
sed -n '1,150p' apps/desktop/src/renderer/main/pages/settings/shortcuts/index.tsx | cat -nRepository: amicalhq/amical
Length of output: 6660
🏁 Script executed:
#!/bin/bash
# Check the onboarding component
sed -n '1,80p' apps/desktop/src/renderer/onboarding/components/shared/OnboardingShortcutInput.tsx | cat -nRepository: amicalhq/amical
Length of output: 2441
Components render nothing during shortcut data load.
ShortcutsSettingsPage initializes shortcuts as undefined and relies on async data loading, causing ShortcutInput to return null until the query completes. This leaves blank space in the UI during load. Consider:
- Adding a default initial state (e.g.,
[]) instead ofundefined - Showing a loading skeleton or placeholder during the async phase
Note: OnboardingShortcutInput already defaults to [], so this is primarily an issue in the settings page.
🤖 Prompt for AI Agents
In `@apps/desktop/src/components/shortcut-input.tsx` around lines 258 - 260,
ShortcutInput currently returns null when prop value is undefined, causing blank
UI while ShortcutsSettingsPage loads async data; change the behavior so the UI
shows a placeholder instead of nothing or ensure the parent provides a default
empty array. Either (A) update ShortcutsSettingsPage to initialize its shortcuts
state to [] (match OnboardingShortcutInput) so ShortcutInput always receives an
array, or (B) modify ShortcutInput (component ShortcutInput in
shortcut-input.tsx) to render a lightweight loading skeleton/placeholder when
value === undefined rather than returning null, keeping existing behavior for
real empty arrays.
1 participant
Allow users to disable keyboard shortcuts by setting them to "None" instead of requiring a key assignment (directly addresses #83)
Changes
The "undo" button is a UX flair (I noticed I was fat-fingering and clearing shortcuts to "None", so I figured having an easy way to restore was less frustrating when it happens), so I'll happily take it out if you don't think its necessary.
Summary by CodeRabbit
New Features
Improvements
Accessibility
✏️ Tip: You can customize this high-level summary in your review settings.