Problem
src/hooks/useKeyboardShortcut.ts, in full:
export function useKeyboardShortcut(key: string, callback: () => void, { modifier, enabled = true }: Options = {}) {
useEffect(() => {
if (!enabled) return
const handler = (e: KeyboardEvent) => {
const modOk = !modifier ||
(modifier === "ctrl" && e.ctrlKey) ||
(modifier === "meta" && e.metaKey) ||
(modifier === "alt" && e.altKey) ||
(modifier === "shift" && e.shiftKey)
if (modOk && e.key.toLowerCase() === key.toLowerCase()) {
e.preventDefault(); callback()
}
}
window.addEventListener("keydown", handler)
return () => window.removeEventListener("keydown", handler)
}, [key, callback, modifier, enabled])
}
Two independent, concrete correctness problems:
- No check for
e.target being a form control. The listener is attached to window and fires regardless of what element has focus. There is no check for document.activeElement/e.target being an <input>, <textarea>, or contenteditable element. Any component using this hook for a single-letter shortcut (e.g. "/" for search-focus, "n" for "new", patterns very common in apps with a contact list / batch-recipient list / memo fields — all present in this codebase) will hijack that keystroke while the user is typing it into a text field elsewhere on the same page.
modOk only checks that the specified modifier is held — it never checks that no other, unspecified modifier is also held. (modifier === "ctrl" && e.ctrlKey) is satisfied by Ctrl+K and by Ctrl+Shift+K and by Ctrl+Alt+K — e.shiftKey/e.altKey are never inspected when modifier === "ctrl". So a shortcut registered as useKeyboardShortcut("k", cb, { modifier: "ctrl" }) also fires for Ctrl+Shift+K, which browsers and OSes very commonly reserve for something else (e.g. many browsers use Ctrl+Shift+K/Cmd+Shift+K for their own devtools/console shortcuts) — this hook's e.preventDefault() will fire and swallow the keystroke regardless, stealing it from the browser or OS shortcut the user actually intended, in addition to also incorrectly triggering this app's own callback for a chord the developer never intended to bind.
Why it matters
- Point 1 is a direct accessibility/usability regression for any keyboard-driven power-user workflow this hook is used for — exactly the users a keyboard-shortcut feature is built to serve are the ones most likely to be interrupted by it firing while they're mid-typing in an adjacent field.
- Point 2 means every modifier-gated shortcut registered via this hook is silently broader than its author intended, with no test or type system to catch it —
{ modifier: "ctrl" } reads like "only Ctrl" but behaves like "Ctrl, optionally with Shift and/or Alt too."
Reproduction
- Register
useKeyboardShortcut("k", cb, { modifier: "ctrl" }), then dispatch a KeyboardEvent with key: "k", ctrlKey: true, shiftKey: true — cb fires, even though only Ctrl+K (no Shift) was intended to be bound.
- Render any component using this hook for an un-modified single-key shortcut (e.g.
useKeyboardShortcut("n", cb)), focus a text <input> elsewhere on the page, and type the letter "n" — the shortcut's callback fires and e.preventDefault() blocks the character from appearing in the input.
Suggested fix
- Add a guard at the top of
handler: skip (return early) when e.target is an HTMLInputElement, HTMLTextAreaElement, HTMLSelectElement, or an element with isContentEditable, unless the shortcut explicitly opts into firing during text entry (e.g. an allowInInputs option for the rare shortcut that should still work, like Escape-to-close).
- Fix
modOk to check that all four modifier flags match an explicit expected combination rather than just the one named modifier — e.g. build an expected { ctrl, meta, alt, shift } tuple from Options (defaulting unset ones to false) and compare all four against the event's flags exactly.
Edge cases
- Some shortcuts legitimately want to allow Shift as a "don't care" (e.g.
Ctrl+K and Ctrl+Shift+K are sometimes intentionally aliased to the same action) — the fix should keep that possible via an explicit opt-in (e.g. allow specifying modifier: ["ctrl"] as "these must be held" separately from others being "don't care"), not hardcode strict exact-match as the only mode.
Testing strategy
- Unit test: register with
modifier: "ctrl", dispatch Ctrl+Shift+<key>, assert the callback does not fire (regression test for point 2).
- Unit test: register a shortcut, render a focused
<input>, simulate the bound key being typed into the input, assert the callback does not fire and the character is not prevented from being entered (regression test for point 1).
Problem
src/hooks/useKeyboardShortcut.ts, in full:Two independent, concrete correctness problems:
e.targetbeing a form control. The listener is attached towindowand fires regardless of what element has focus. There is no check fordocument.activeElement/e.targetbeing an<input>,<textarea>, orcontenteditableelement. Any component using this hook for a single-letter shortcut (e.g."/"for search-focus,"n"for "new", patterns very common in apps with a contact list / batch-recipient list / memo fields — all present in this codebase) will hijack that keystroke while the user is typing it into a text field elsewhere on the same page.modOkonly checks that the specified modifier is held — it never checks that no other, unspecified modifier is also held.(modifier === "ctrl" && e.ctrlKey)is satisfied byCtrl+Kand byCtrl+Shift+Kand byCtrl+Alt+K—e.shiftKey/e.altKeyare never inspected whenmodifier === "ctrl". So a shortcut registered asuseKeyboardShortcut("k", cb, { modifier: "ctrl" })also fires forCtrl+Shift+K, which browsers and OSes very commonly reserve for something else (e.g. many browsers useCtrl+Shift+K/Cmd+Shift+Kfor their own devtools/console shortcuts) — this hook'se.preventDefault()will fire and swallow the keystroke regardless, stealing it from the browser or OS shortcut the user actually intended, in addition to also incorrectly triggering this app's own callback for a chord the developer never intended to bind.Why it matters
{ modifier: "ctrl" }reads like "only Ctrl" but behaves like "Ctrl, optionally with Shift and/or Alt too."Reproduction
useKeyboardShortcut("k", cb, { modifier: "ctrl" }), then dispatch aKeyboardEventwithkey: "k", ctrlKey: true, shiftKey: true—cbfires, even though onlyCtrl+K(no Shift) was intended to be bound.useKeyboardShortcut("n", cb)), focus a text<input>elsewhere on the page, and type the letter "n" — the shortcut'scallbackfires ande.preventDefault()blocks the character from appearing in the input.Suggested fix
handler: skip (return early) whene.targetis anHTMLInputElement,HTMLTextAreaElement,HTMLSelectElement, or an element withisContentEditable, unless the shortcut explicitly opts into firing during text entry (e.g. anallowInInputsoption for the rare shortcut that should still work, like Escape-to-close).modOkto check that all four modifier flags match an explicit expected combination rather than just the one named modifier — e.g. build an expected{ ctrl, meta, alt, shift }tuple fromOptions(defaulting unset ones tofalse) and compare all four against the event's flags exactly.Edge cases
Ctrl+KandCtrl+Shift+Kare sometimes intentionally aliased to the same action) — the fix should keep that possible via an explicit opt-in (e.g. allow specifyingmodifier: ["ctrl"]as "these must be held" separately from others being "don't care"), not hardcode strict exact-match as the only mode.Testing strategy
modifier: "ctrl", dispatchCtrl+Shift+<key>, assert the callback does not fire (regression test for point 2).<input>, simulate the bound key being typed into the input, assert the callback does not fire and the character is not prevented from being entered (regression test for point 1).