fix: Add guard to normalizeKeyName()#98
Conversation
📝 WalkthroughWalkthroughnormalizeKeyName now immediately returns an empty string for falsy inputs (e.g., undefined or empty string). A test was added asserting this guard behavior; a changeset notes the patch. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/hotkeys/src/constants.ts`:
- Around line 458-460: Fix the typo in the inline comment that currently reads
"// key an be undefined in rare cases" to "// key can be undefined in rare
cases" near the null-check for the variable 'key' (the line with "if (!key)
return ''"); update only the comment text to correct "an" to "can".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0a4dbe1a-1b12-4d8a-86c2-490f5a6f4b29
📒 Files selected for processing (2)
packages/hotkeys/src/constants.tspackages/hotkeys/tests/parse.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/hotkeys/src/constants.ts`:
- Around line 458-460: The guard in constants.ts currently returns an empty
string for an invalid/undefined key which lets invalid keys flow downstream;
change the sentinel to return null instead of '' so callers must short-circuit
and handle the absence of a key. Update the function that checks `key` (the if
(!key) return ...) to return null, and then update any callers that consume its
result—particularly code paths that add to `#heldKeysSet` and `#heldCodesMap` in
key-state-tracker.ts—to explicitly check for null and skip processing. Ensure
all uses of the function treat null as “invalid key” (not a valid key string) so
no empty-string values are inserted into heldKeys/heldCodes structures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5bcf3970-4c74-46e6-9a16-da87ccdb8da2
📒 Files selected for processing (1)
packages/hotkeys/src/constants.ts
| // key can be undefined in rare cases | ||
| // (browser extensions synthesizing key events, accessibility tools, certain OS/browser combinations). | ||
| if (!key) return '' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect direct call sites that normalize event.key.
rg -n --type=ts -C3 '\bnormalizeKeyName\s*\(\s*event\.key\s*\)' packages/hotkeys/src
# Inspect likely downstream consumers where '' may be treated as a valid key.
rg -n --type=ts -C3 '\bheldKeysSet\.add\(|\bheldCodesMap\.set\(|\bisModifierKey\s*\(\s*normalizeKeyName\s*\(\s*event\.key\s*\)\s*\)' packages/hotkeys/srcRepository: TanStack/hotkeys
Length of output: 5912
🏁 Script executed:
# First, let's see the full implementation of normalizeKeyName
sed -n '450,470p' packages/hotkeys/src/constants.ts
# Then check the isModifierKey function
rg -n --type=ts 'export function isModifierKey|export const isModifierKey' packages/hotkeys/src -A 10
# Also check what normalizeKeyName does with empty/falsy values
sed -n '440,465p' packages/hotkeys/src/constants.tsRepository: TanStack/hotkeys
Length of output: 2317
🏁 Script executed:
# Check key-state-tracker to see if there's any guard before adding to Set
sed -n '110,130p' packages/hotkeys/src/key-state-tracker.ts
# Check if normalizeKeyName can actually return empty string, and check parse.ts usage
rg -n --type=ts 'const normalizedKey = normalizeKeyName' packages/hotkeys/src -A 5Repository: TanStack/hotkeys
Length of output: 1669
🏁 Script executed:
# Check match.ts for how it handles empty eventKey
sed -n '63,75p' packages/hotkeys/src/match.ts
# Check hotkey-manager.ts for releasedKey usage
sed -n '635,650p' packages/hotkeys/src/hotkey-manager.tsRepository: TanStack/hotkeys
Length of output: 1037
Line 460: Avoid returning '' for invalid keys; use null and require callers to short-circuit.
The guard prevents a crash but lets invalid events (with undefined event.key) flow as empty-string sentinels into downstream stateful paths. For example, in key-state-tracker.ts:113–116, the empty string gets added directly to #heldKeysSet and #heldCodesMap with no validation. Returning null instead makes the invalid state explicit and forces callers to handle it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/hotkeys/src/constants.ts` around lines 458 - 460, The guard in
constants.ts currently returns an empty string for an invalid/undefined key
which lets invalid keys flow downstream; change the sentinel to return null
instead of '' so callers must short-circuit and handle the absence of a key.
Update the function that checks `key` (the if (!key) return ...) to return null,
and then update any callers that consume its result—particularly code paths that
add to `#heldKeysSet` and `#heldCodesMap` in key-state-tracker.ts—to explicitly
check for null and skip processing. Ensure all uses of the function treat null
as “invalid key” (not a valid key string) so no empty-string values are inserted
into heldKeys/heldCodes structures.
There was a problem hiding this comment.
I used '' so the return type stays string, but happy to be corrected...
There was a problem hiding this comment.
🧩 Analysis chain
Good question — let me look at the actual downstream call sites to give you a concrete recommendation.
🏁 Script executed:
#!/bin/bash
# Check the full context around key-state-tracker.ts where normalizeKeyName result is used
rg -n --type=ts 'normalizeKeyName' packages/hotkeys/src -A 5 -B 3
# Also check if there's any existing empty-string guard at call sites
rg -n --type=ts 'normalizedKey' packages/hotkeys/src -A 3 -B 3
Length of output: 15632
@ryck Keeping string is the right call here — changing to string | null would cascade through 7+ call sites across the codebase. The '' sentinel is actually fine for most of them:
| Call site | Effect with '' |
|---|---|
match.ts:66 |
eventKey.length === 1 is false → safely skipped |
hotkey-manager.ts:638 |
'' won't match any registered key → harmless |
parse.ts:267 |
Returns ParsedHotkey with key: '' → no hotkey will match it |
sequence-manager.ts:442, recorder-chord.ts:17 |
isModifierKey('') → false, minor but non-critical |
The one concrete problem is key-state-tracker.ts lines 113–116, where no guard exists and '' would be stored directly into #heldKeysSet and #heldCodesMap:
const key = normalizeKeyName(event.key)
if (!this.#heldKeysSet.has(key)) {
this.#heldKeysSet.add(key) // ← '' inserted here
this.#heldCodesMap.set(key, event.code)
}The fix is simple — a single guard at that call site, keeping your string return type intact:
// key-state-tracker.ts, keydown listener
this.#keydownListener = (event: KeyboardEvent) => {
const key = normalizeKeyName(event.key)
if (!key) return // ← guard against undefined event.key
if (!this.#heldKeysSet.has(key)) {
this.#heldKeysSet.add(key)
this.#heldCodesMap.set(key, event.code)
this.#syncState()
}
}So: your normalizeKeyName guard is correct, and '' is the right sentinel value. The remaining gap is just this one guard in key-state-tracker.ts.
@tanstack/angular-hotkeys
@tanstack/hotkeys
@tanstack/hotkeys-devtools
@tanstack/lit-hotkeys
@tanstack/preact-hotkeys
@tanstack/preact-hotkeys-devtools
@tanstack/react-hotkeys
@tanstack/react-hotkeys-devtools
@tanstack/solid-hotkeys
@tanstack/solid-hotkeys-devtools
@tanstack/svelte-hotkeys
@tanstack/vue-hotkeys
@tanstack/vue-hotkeys-devtools
commit: |
KevinVandy
changed the title
2 participants
🎯 Changes
normalizeKeyName calls key.toUpperCase() without guarding against undefined, and event.key can be undefined in rare cases (browser extensions synthesizing key events, accessibility tools, certain OS/browser combinations).
This PR adds a guard for these cases, so no more errors like:
Uncaught (in promise) TypeError: Cannot read properties of undefined (reading 'payload')Fixes #97
✅ Checklist
pnpm run test:pr.🚀 Release Impact
Summary by CodeRabbit
Bug Fixes
Tests