Skip to content

Fix double-modifier bug in hotkey modifier sorting#146

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

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

Conversation

@iansan5653

Copy link
Copy Markdown
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.

Comment thread src/hotkey.ts
Alt: 1,
Meta: 2,
Shift: 3
}

Copy link
Copy Markdown
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)$.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread test/test-normalize-hotkey.js Outdated
Comment thread src/hotkey.ts
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@iansan5653

Copy link
Copy Markdown
Member Author

@copilot fix the eslint errors in the changed files

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

Copilot AI commented Mar 16, 2026

Copy link
Copy Markdown
Contributor

@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

4 participants