Fix for Keyboard Shortcuts Not Respecting Keyboard Layout [Issue #1071]

[Mikesteam1234]

…ffects keybind related actions. Actions such as Escape and Performance overlay remain as physical key presses.

Description:

This PR fixes #1071 in which the physical key code was used over the key itself. The previous behavior resulted in issues when using keyboard layouts outside of the standard such as AZERTY.

The ideal fix would utilize a command pattern and some central manager that would route key presses. This PR is a half step towards that methodology as a quick fix for the issue described.

This approach should also respect user keybinds already in production -- converting their 'physical keys' to 'digital.'

All tests pass:

Manually tested behavior on local build, behaves as expected using QWERTY and AZERTY.

Please complete the following:

• I have added screenshots for all UI updates
• I process any text displayed to the user through translateText() and I've added it to the en.json file
• I have added relevant tests to the test directory
• I confirm I have thoroughly tested these changes and take full responsibility for any bugs introduced

Please put your Discord username so you can be contacted if a bug or regression is found:

Mikesteam1234

[coderabbitai]

Walkthrough

Reworked keyboard input handling to use event.key values, introduced a centralized Action enum and default keybind map, wired settings UI to those defaults, and added user override merging from localStorage. Updated keybind capture to store/display keys, and adjusted modifier handling and logging accordingly.

Changes

Cohort / File(s)	Summary
Input actions and defaults src/client/utilities/InputMap.ts	Added Action enum and ActionKeybindMapDefaults map centralizing default bindings.
Input handling logic src/client/InputHandler.ts	Switched detection to event.key, normalized overrides from localStorage, merged with defaults, added camelCase action-name helper, simplified modifier checks to Control/Shift, and updated debug logs.
Settings UI defaults src/client/UserSettingModal.ts	Replaced hard-coded defaultKey strings with ActionKeybindMapDefaults lookups for all listed actions.
Keybind capture component src/client/components/baseComponents/setting/SettingKeybind.ts	Now captures and emits KeyboardEvent.key instead of code for bound value.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor User
  participant DOM as Browser
  participant SettingKeybind
  participant UserSettingModal
  participant LocalStorage as localStorage
  participant InputHandler
  participant Game as GameEvents

  rect rgb(245,248,255)
    note over SettingKeybind,UserSettingModal: Bind/Change key
    User->>DOM: Press a key
    DOM-->>SettingKeybind: keydown (key)
    SettingKeybind->>SettingKeybind: set value = event.key
    SettingKeybind-->>UserSettingModal: dispatch "change" { value: key }
    UserSettingModal->>LocalStorage: save settings.keybinds
  end

  rect rgb(244,255,245)
    note over InputHandler: Initialize active keybinds
    InputHandler->>InputHandler: build defaults from ActionKeybindMapDefaults
    InputHandler->>LocalStorage: read settings.keybinds
    LocalStorage-->>InputHandler: overrides (keys normalized)
    InputHandler->>InputHandler: merge overrides into active map
  end

  rect rgb(255,249,240)
    note over InputHandler,Game: Runtime key handling (uses event.key)
    User->>DOM: Press key(s)
    DOM-->>InputHandler: keydown (key)
    InputHandler->>InputHandler: match key against active map<br/>(Control/Shift as modifiers)
    alt matched action
      InputHandler-->>Game: emit Action event
    else no match
      InputHandler-->>InputHandler: ignore
    end
  end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Assessment against linked issues

Objective	Addressed	Explanation
Use code for detection and key for display [#1071]	❌	Detection now uses event.key; UI capture and storage also use key, not code. Physical key consistency across layouts is not preserved.

Assessment against linked issues: Out-of-scope changes

Code Change	Explanation
Add Action enum and ActionKeybindMapDefaults (src/client/utilities/InputMap.ts)	Centralizing actions/defaults is not requested by #1071, which focuses on code vs key usage behavior.
Merge user overrides from localStorage and normalize values (src/client/InputHandler.ts)	Persistence/normalization logic extends beyond the code-vs-key requirement.
Introduce convertToCamelCase for action-name mapping (src/client/InputHandler.ts)	Name-shaping helper is unrelated to the issue’s objective.
Replace hard-coded defaultKey strings with dynamic lookups (src/client/UserSettingModal.ts)	Refactoring defaults source is not part of the specified behavior change for #1071.

Poem

New keys dance in the night, tap-tap they say,
A map of actions leads the play.
Defaults gather, user wills blend,
Yet codes are lost around the bend—
Someday, code to steer, key to display.
For now, the letters have their way. 🎹✨

Tip

🔌 Remote MCP (Model Context Protocol) integration is now available!

Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>, please review it.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

• Add @coderabbitai ignore or @coderabbit ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, Documentation and Community

• Visit our Status Page to check the current availability of CodeRabbit.
• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[coderabbitai]

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 (4)

src/client/components/baseComponents/setting/SettingKeybind.ts (1)

73-91: Stop propagation and normalize keys for consistency.

• Without stopPropagation, Escape while capturing can close the modal.
• Normalize single-letter keys to lowercase to match InputHandler expectations and avoid accidental Shift+letter producing uppercase bindings.
• Ignore "Dead"/"Unidentified" keys to avoid unusable bindings.

   private handleKeydown(e: KeyboardEvent) {
     if (!this.listening) return;
-    e.preventDefault();
+    e.preventDefault();
+    e.stopPropagation();
 
-    const { key } = e;
+    const { key } = e;
+    if (key === "Dead" || key === "Unidentified") return;
-    this.value = key;
+    const normalized =
+      key.length === 1 ? key.toLowerCase() : (key === "Spacebar" ? " " : key);
+    this.value = normalized;
 
     this.dispatchEvent(
       new CustomEvent("change", {
-        detail: { action: this.action, value: key },
+        detail: { action: this.action, value: normalized },
         bubbles: true,
         composed: true,
       }),
     );

src/client/InputHandler.ts (3)

210-217: Shift suppression check is broken (uses ShiftLeft/Right but you add 'Shift').

You add e.key ("Shift") to activeKeys, but here you check for "ShiftLeft"/"ShiftRight". Movement won’t pause while holding Shift.

-      if (
-        this.activeKeys.has("ShiftLeft") ||
-        this.activeKeys.has("ShiftRight")
-      ) {
+      if (this.activeKeys.has("Shift")) {
         return;
       }

---

246-257: Minus/Equal mismatches with e.key prevent zoom hotkeys from working.

• e.key is "-" and "="; "Minus"/"Equal" are e.code values. You’re storing e.key in activeKeys, so these never match.
• Also ensure you add "-" and "=" to activeKeys on keydown, and delete them on keyup.

-      if (
-        this.activeKeys.has(this.keybinds.zoomOut) ||
-        this.activeKeys.has("Minus")
-      ) {
+      if (
+        this.activeKeys.has(this.keybinds.zoomOut) ||
+        this.activeKeys.has("-")
+      ) {
         this.eventBus.emit(new ZoomEvent(cx, cy, this.ZOOM_SPEED));
       }
-      if (
-        this.activeKeys.has(this.keybinds.zoomIn) ||
-        this.activeKeys.has("Equal")
-      ) {
+      if (
+        this.activeKeys.has(this.keybinds.zoomIn) ||
+        this.activeKeys.has("=")
+      ) {
         this.eventBus.emit(new ZoomEvent(cx, cy, -this.ZOOM_SPEED));
       }

-          "Minus",
-          "Equal",
+          "-",
+          "=",

-      this.activeKeys.delete(e.key);
+      this.activeKeys.delete(e.key === " " ? " " : e.key);

Also applies to: 274-296, 343-344

---

434-449: 'realCtrl' detection is inconsistent with e.key usage.

You never add "ControlLeft/Right" to activeKeys anymore, so realCtrl is always false. Use "Control" or just event.ctrlKey.

-      const realCtrl =
-        this.activeKeys.has("ControlLeft") ||
-        this.activeKeys.has("ControlRight");
+      const realCtrl = this.activeKeys.has("Control");

🧹 Nitpick comments (8)

src/client/UserSettingModal.ts (2)

426-426: Guard against undefined defaults in templates.

ActionKeybindMapDefaults.get(...) can return undefined; Lit will stringify it as "undefined". Add a fallback.

- defaultKey="${ActionKeybindMapDefaults.get(Action.TOGGLE_VIEW)}"
+ defaultKey="${ActionKeybindMapDefaults.get(Action.TOGGLE_VIEW) ?? ""}"

Apply the same ?? "" to all defaultKey bindings listed in this comment.

Also applies to: 439-439, 448-448, 461-461, 470-470, 483-483, 492-492, 505-505, 514-514, 523-523, 532-532, 541-541

---

55-71: Prevent modal-close while capturing keybinds.

Pressing Escape during a keybind capture will bubble to the modal’s Escape handler. Consider stopping propagation in the key-capture component (see SettingKeybind.ts note) or gating this handler when a capture is active.

src/client/components/baseComponents/setting/SettingKeybind.ts (1)

58-66: displayKey: unreachable 'KeyX' path after switching to e.key.

Now that we store/display e.key, strings like "KeyW" shouldn’t appear. You can remove the Key* branch or keep it only as a legacy fallback.

src/client/InputHandler.ts (3)

260-296: Key-list maintenance: include Alt/Meta if you rely on activeKeys for combos.

You add "Control" and "Shift". If future checks rely on Alt/Meta presence (similar to Shift/Control), include them here for symmetry.

         [
           this.keybinds.moveUp,
           this.keybinds.moveDown,
           this.keybinds.moveLeft,
           this.keybinds.moveRight,
           this.keybinds.zoomOut,
           this.keybinds.zoomIn,
           "ArrowUp",
           "ArrowLeft",
           "ArrowDown",
           "ArrowRight",
-          "-",
-          "=",
+          "-",
+          "=",
           this.keybinds.attackRatioDown,
           this.keybinds.attackRatioUp,
           this.keybinds.centerCamera,
           "Control",
           "Shift",
+          "Alt",
+          "Meta",
         ].includes(e.key)

---

335-341: Debug logging left in keyup handler.

Remove or guard behind a debug flag; this is noisy every keyup.

-      console.log(e.key, e.shiftKey, e.ctrlKey, e.altKey, e.metaKey);

---

147-168: Direction check: confirm “use key for detection” vs original issue’s “use code for detection.”

The linked issue #1071 requested physical detection (event.code) and display using key. This PR flips to key detection for most actions (Escape/perf overlay still use code). This affects migration: converting "KeyW" → "w" will not yield "z" on AZERTY, so existing users may lose expected movement until they rebind.

If the product decision is to follow the issue literally (code for detection, key for display), consider:

• Persist codes; detect with e.code.
• For display: try navigator.keyboard.getLayoutMap() when available; fallback to simple KeyX/DigitN → x/n mapping.
• Provide a one-time migration screen prompting users to confirm/rebind keys after this change, rather than silent conversion.

I can draft a migration helper and a display mapper if you want.

Also applies to: 260-273, 299-341, 594-610

src/client/utilities/InputMap.ts (2)

27-42: Keep map value semantics consistent.

Most entries are e.key strings (e.g., "w"), but MODIFIER_KEY/ALT_KEY use code-like strings ("ControlLeft", "AltLeft"). This is easy to misread later. Either:

• Store canonical e.key values ("Control", "Alt") and translate where you require side-specific checks, or
• Clearly mark these two as code-based in the type.

Example typed approach:

type KeyBindValue =
  | string // e.key
  | "ControlLeft" | "AltLeft" | "ShiftLeft" | "MetaLeft"; // explicit code-based

---

1-25: Prefer explicit enum values.

Give Action members explicit string values to stabilize serialization and devtools logging.

-export enum Action {
+export enum Action {
   // View and camera controls
-  TOGGLE_VIEW,
+  TOGGLE_VIEW = "TOGGLE_VIEW",
   // ...
}

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 1ae7ebc and 7466642.

📒 Files selected for processing (4)

• src/client/InputHandler.ts (5 hunks)
• src/client/UserSettingModal.ts (13 hunks)
• src/client/components/baseComponents/setting/SettingKeybind.ts (1 hunks)
• src/client/utilities/InputMap.ts (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (2)

src/client/UserSettingModal.ts (1)

src/client/utilities/InputMap.ts (1)

• ActionKeybindMapDefaults (27-42)

src/client/InputHandler.ts (1)

src/client/utilities/InputMap.ts (1)

• ActionKeybindMapDefaults (27-42)

[coderabbitai]

⚠️ Potential issue

Harden parsing and normalization of saved keybinds.

• JSON.parse can throw; this will break input init.
• Normalize legacy and mixed inputs: KeyX → x, DigitN → N, Space/Spacebar → " ", single letters → lowercase.

-    const userKeybinds = JSON.parse(localStorage.getItem("settings.keybinds") ?? "{}") ?? {};
-
-    for (const [action, key] of Object.entries(userKeybinds) as [string, string][]) {
-
-      let keyToWrite = key;
-
-      if (/^Key[A-Z]$/.test(key)) {
-        keyToWrite = key.slice(3).toLowerCase();
-      } else if (/^Digit[0-9]$/.test(key)) {
-        keyToWrite = key.slice(5);
-      }
-
-      this.keybinds[action] = keyToWrite;
-    }
+    let userKeybinds: Record<string, string> = {};
+    try {
+      const raw = localStorage.getItem("settings.keybinds");
+      const parsed = raw ? JSON.parse(raw) : {};
+      if (parsed && typeof parsed === "object") {
+        userKeybinds = parsed as Record<string, string>;
+      }
+    } catch {
+      // ignore corrupt storage; keep defaults
+    }
+
+    for (const [action, key] of Object.entries(userKeybinds) as [string, string][]) {
+      let keyToWrite = key;
+      if (/^Key[A-Z]$/.test(key)) {
+        keyToWrite = key.slice(3).toLowerCase();
+      } else if (/^Digit[0-9]$/.test(key)) {
+        keyToWrite = key.slice(5);
+      } else if (key === "Space" || key === "Spacebar") {
+        keyToWrite = " ";
+      } else if (key.length === 1) {
+        keyToWrite = key.toLowerCase();
+      }
+      this.keybinds[action] = keyToWrite;
+    }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	initialize() {
	this.keybinds = {
	toggleView: "Space",
	centerCamera: "KeyC",
	moveUp: "KeyW",
	moveDown: "KeyS",
	moveLeft: "KeyA",
	moveRight: "KeyD",
	zoomOut: "KeyQ",
	zoomIn: "KeyE",
	attackRatioDown: "Digit1",
	attackRatioUp: "Digit2",
	boatAttack: "KeyB",
	groundAttack: "KeyG",
	modifierKey: "ControlLeft",
	altKey: "AltLeft",
	...(JSON.parse(localStorage.getItem("settings.keybinds") ?? "{}") ?? {}),
	};
	this.keybinds = {};
	for (const [action, key] of ActionKeybindMapDefaults.entries()) {
	const actionKey = this.convertToCamelCase(Action[action]);
	this.keybinds[actionKey] = key;
	}
	const userKeybinds = JSON.parse(localStorage.getItem("settings.keybinds") ?? "{}") ?? {};
	for (const [action, key] of Object.entries(userKeybinds) as [string, string][]) {
	let keyToWrite = key;
	if (/^Key[A-Z]$/.test(key)) {
	keyToWrite = key.slice(3).toLowerCase();
	} else if (/^Digit[0-9]$/.test(key)) {
	keyToWrite = key.slice(5);
	}
	this.keybinds[action] = keyToWrite;
	}
	initialize() {
	this.keybinds = {};
	for (const [action, key] of ActionKeybindMapDefaults.entries()) {
	const actionKey = this.convertToCamelCase(Action[action]);
	this.keybinds[actionKey] = key;
	}
	let userKeybinds: Record<string, string> = {};
	try {
	const raw = localStorage.getItem("settings.keybinds");
	const parsed = raw ? JSON.parse(raw) : {};
	if (parsed && typeof parsed === "object") {
	userKeybinds = parsed as Record<string, string>;
	}
	} catch {
	// ignore corrupt storage; keep defaults
	}
	for (const [action, key] of Object.entries(userKeybinds) as [string, string][]) {
	let keyToWrite = key;
	if (/^Key[A-Z]$/.test(key)) {
	keyToWrite = key.slice(3).toLowerCase();
	} else if (/^Digit[0-9]$/.test(key)) {
	keyToWrite = key.slice(5);
	} else if (key === "Space" || key === "Spacebar") {
	keyToWrite = " ";
	} else if (key.length === 1) {
	keyToWrite = key.toLowerCase();
	}
	this.keybinds[action] = keyToWrite;
	}

🤖 Prompt for AI Agents

In src/client/InputHandler.ts around lines 147 to 168, the JSON.parse of
localStorage and the key normalization are brittle: wrap the parse in try/catch
and fall back to an empty object on error; then normalize each saved key before
assigning by handling legacy prefixes and variants—if key matches /^Key[A-Z]$/
set to the single lowercase letter, if /^Digit[0-9]$/ set to the digit, if key
equals "Space" or "Spacebar" map to " ", if key is a single alphabetic character
convert to lowercase, otherwise keep the original key; finally assign the
normalized value into this.keybinds[action] (ensuring action keys match the
camelCased action names used earlier).

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
7 out of 25 committers have signed the CLA.

✅ Baguette8ot
✅ DevelopingTom
✅ evanpelle
✅ TheGiraffe3
✅ Boostry123
✅ abdallahbahrawi1
✅ woodydrn
❌ Cameron Clark
❌ djmrFunnyMan
❌ pierrebrtr
❌ dependabot[bot]
❌ TylerHanavan
❌ scottanderson
❌ TLS15
❌ VariableVince
❌ Aotumuri
❌ Kipstz
❌ aaa4xu
❌ Killersoren
❌ fantom845
❌ Bornasm
❌ Mikesteam1234
❌ ElMelchizedek
❌ PilkeySEK
❌ mokizzz

---

Cameron Clark seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}