Skip to content

Fix double-modifier bug in hotkey modifier sorting#146

Merged
iansan5653 merged 4 commits intomainfrom
fix-sort-modifiers
Mar 16, 2026
Merged

Fix double-modifier bug in hotkey modifier sorting#146
iansan5653 merged 4 commits intomainfrom
fix-sort-modifiers

Conversation

@iansan5653
Copy link
Member

The sortModifers method duplicates a modifier if there is no non-modifier key. For example, sortModifiers("Alt") returns "Alt+Alt".

This PR updates the method to preserve the input key names, limiting modifications to just sorting. Also updates tests.

Alt: 1,
Meta: 2,
Shift: 3
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be a little simpler to just write an ordered array here and use indexOf for the numbers, but it would be slightly slower since the lookup would be $O(n)$, whereas the object lookup is $O(1)$.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates hotkey normalization to better handle modifier-only shortcuts and adds test coverage for those cases.

Changes:

  • Refactors modifier sorting in normalizeHotkey to use a general token sort with an explicit modifier order.
  • Adds normalizeHotkey tests for “only modifiers” and “modifiers + Mod” edge cases.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
test/test-normalize-hotkey.js Adds new normalizeHotkey test cases for modifier-only inputs
src/hotkey.ts Refactors modifier sorting logic used by normalizeHotkey
Comments suppressed due to low confidence (1)

src/hotkey.ts:104

  • modifierKeyNames already defines the canonical modifier ordering, but orderedModifiers duplicates that list in a second place. To avoid these drifting apart in the future, consider generating the ordering map from modifierKeyNames (single source of truth) instead of hardcoding both.
const modifierKeyNames: string[] = ['Control', 'Alt', 'Meta', 'Shift']

/**
 * Normalizes a hotkey string before comparing it to the serialized event
 * string produced by `eventToHotkeyString`.
 * - Replaces the `Mod` modifier with `Meta` on mac, `Control` on other
 *   platforms.
 * - Ensures modifiers are sorted in a consistent order
 * @param hotkey a hotkey string
 * @param platform NOTE: this param is only intended to be used to mock `navigator.platform` in tests. `window.navigator.platform` is used by default.
 * @returns {string} normalized representation of the given hotkey string
 */
export function normalizeHotkey(hotkey: string, platform?: string | undefined): NormalizedHotkeyString {
  let result: string
  result = localizeMod(hotkey, platform)
  result = sortModifiers(result)
  return result as NormalizedHotkeyString
}

const matchApplePlatform = /Mac|iPod|iPhone|iPad/i

function localizeMod(hotkey: string, platform?: string | undefined): string {
  const ssrSafeWindow = typeof window === 'undefined' ? undefined : window
  const safePlatform = platform ?? ssrSafeWindow?.navigator.platform ?? ''

  const localModifier = matchApplePlatform.test(safePlatform) ? 'Meta' : 'Control'
  return hotkey.replace('Mod', localModifier)
}

const orderedModifiers: Partial<Record<string, number>> = {
  Control: 0,
  Alt: 1,
  Meta: 2,
  Shift: 3
}

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@iansan5653
Copy link
Member Author

@copilot fix the eslint errors in the changed files

Co-authored-by: iansan5653 <2294248+iansan5653@users.noreply.github.com>
Copy link
Contributor

Copilot AI commented Mar 16, 2026

@copilot fix the eslint errors in the changed files

Fixed in dd025b5. There was a prettier formatting error in sortModifiers — the method chain needed a newline before .split('+'). Running eslint --fix resolved it.

@iansan5653 iansan5653 merged commit 4ff070b into main Mar 16, 2026
4 checks passed
@iansan5653 iansan5653 deleted the fix-sort-modifiers branch March 16, 2026 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants