Make hotkeys great again #2946
Make hotkeys great again #2946
Conversation
WalkthroughCentralizes default keybinds and parsing, adds composite keyboard/mouse/wheel keybind support, validates reserved browser shortcuts, updates UI capture/rendering and input handling to use dynamic keybinds, and adds many new translation keys for the expanded keybind surface. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as Settings/Help/Keybind Capture
participant Store as LocalStorage/Settings
participant Input as InputHandler
participant Parser as Keybind Parser
participant Game as EventBus/Game
User->>UI: Define or change keybind (keyboard / mouse / wheel)
UI->>Store: Save keybinds to localStorage
Store-->>Input: settings.keybinds.changed
Input->>Parser: Load merged config (getDefaultKeybinds + saved)
Parser-->>Input: ParsedKeybinds cached
User->>Input: Keyboard / Pointer / Wheel event
Input->>Parser: Match event against parsed keybinds
alt Matching action found
Input->>Game: Emit action event (e.g., AutoUpgradeEvent, AttackRatioEvent, ToggleOverlay)
else No match
Input->>Game: Ignore or forward raw input
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/InputHandler.ts (1)
425-431: Remove debug logging.Line 426 has a
console.logthat logs all key events. This should be removed before merging.🔧 Proposed fix
if (this.matchesKeybind(e, this.keybinds.swapDirection)) { e.preventDefault(); const nextDirection = !this.uiState.rocketDirectionUp; this.eventBus.emit(new SwapRocketDirectionEvent(nextDirection)); } - // Toggle performance overlay keybind - console.log(e.code, e.shiftKey, e.ctrlKey, e.altKey, e.metaKey); if (this.matchesKeybind(e, this.keybinds.togglePerformanceOverlay)) { e.preventDefault(); - console.log("TogglePerformanceOverlayEvent"); this.eventBus.emit(new TogglePerformanceOverlayEvent()); }
🤖 Fix all issues with AI agents
In `@src/client/HelpModal.ts`:
- Line 3: The code in HelpModal that reads stored keybinds (used alongside
getDefaultKeybinds) currently drops entries when the stored value is a string[];
fix this by normalizing any array values before filtering: detect
Array.isArray(value) and convert to a single string (e.g., value.join(" ") or
another delimiter you use for display) then proceed with the existing
filter/format logic so no saved keybinds are ignored; update the read/normalize
logic in the HelpModal code path that merges stored keybinds with
getDefaultKeybinds so arrays are handled consistently.
In `@src/client/UserSettingModal.ts`:
- Around line 184-213: In showKeybindError, don't split the translated string by
displayKey (this breaks translations when displayKey is short); instead pass a
unique placeholder token into translateText (e.g., replace displayKey with
"__KEY_TOKEN__" in the translation args) and then replace that token in the
resulting message with the styled <span> for displayKey (use string replace or
split on the token) so you inject the formatted key without risk of accidental
matches; update the code paths around messageKey, displayKey, and translateText
accordingly.
🧹 Nitpick comments (3)
src/client/components/baseComponents/setting/SettingKeybind.ts (1)
274-310: Consider extracting shared modifier logic.
getPointerModifiersandgetWheelModifiershave nearly identical code. A shared helper would reduce duplication.♻️ Proposed refactor
+ private getEventModifiers(e: { ctrlKey: boolean; shiftKey: boolean; altKey: boolean; metaKey: boolean }): string[] { + const modifiers = new Set(this.activeModifiers); + if (e.ctrlKey) modifiers.add("ControlLeft"); + if (e.shiftKey) modifiers.add("ShiftLeft"); + if (e.altKey) modifiers.add("AltLeft"); + if (e.metaKey) modifiers.add("MetaLeft"); + return this.orderModifiers(Array.from(modifiers)); + } + private getPointerModifiers(e: PointerEvent): string[] { - const modifiers = new Set(this.activeModifiers); - - if (e.ctrlKey) { - modifiers.add("ControlLeft"); - } - if (e.shiftKey) { - modifiers.add("ShiftLeft"); - } - if (e.altKey) { - modifiers.add("AltLeft"); - } - if (e.metaKey) { - modifiers.add("MetaLeft"); - } - - return this.orderModifiers(Array.from(modifiers)); + return this.getEventModifiers(e); } private getWheelModifiers(e: WheelEvent): string[] { - const modifiers = new Set(this.activeModifiers); - - if (e.ctrlKey) { - modifiers.add("ControlLeft"); - } - if (e.shiftKey) { - modifiers.add("ShiftLeft"); - } - if (e.altKey) { - modifiers.add("AltLeft"); - } - if (e.metaKey) { - modifiers.add("MetaLeft"); - } - - return this.orderModifiers(Array.from(modifiers)); + return this.getEventModifiers(e); }src/client/graphics/layers/UnitDisplay.ts (1)
101-114: Good helper for handling legacy formats, but the array check is unnecessary.The
getKeybindValuemethod correctly handles both legacy string values and the newer{value, key}object format. However, the array check on line 108 is overly defensive—keybinds are always stored as strings ({ value: string, key: string }), never as arrays. The code works fine, but this array handling can be removed to simplify the method.src/client/InputHandler.ts (1)
275-276: Add guards for window access in moveInterval callback.The code accesses
window.innerWidthandwindow.innerHeightat lines 275–276 inside themoveIntervalcallback without checking ifwindowis defined. While tests pass with jsdom, this code will fail ifinitialize()is called in non-browser environments (SSR, Node.js test runners without DOM).Add a guard at the start of the interval callback:
this.moveInterval = setInterval(() => { if (typeof window === "undefined") return; let deltaX = 0; let deltaY = 0; // ... rest of callbackAlternatively, consider moving the zoom calculation outside the interval or ensuring
initialize()only runs in browser contexts.
ryanbarlow97
added
Refactor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/client/InputHandler.ts`:
- Around line 480-497: The onScroll handler currently uses deltaX when deltaY is
0, causing pure horizontal scroll to be treated as zoom/ratio; change onScroll
in InputHandler to detect horizontal-only scroll (deltaY === 0 && deltaX !== 0)
and do not run the ratio/zoom keybind logic or emit ZoomEvent (i.e., return
early or let the pan handler handle it), while preserving existing
matchesWheelKeybind checks for mixed/vertical scroll; ensure you reference
onScroll, matchesWheelKeybind, and the ZoomEvent emission when making the
change.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/client/InputHandler.ts`:
- Around line 503-518: onAttackRatioScroll currently treats horizontal-only
wheel events as valid input, causing Shift+horizontal trackpad swipes to change
attack ratio; add the same horizontal-only guard used in onScroll so the handler
returns early when deltaY === 0 && deltaX !== 0. Update the method
onAttackRatioScroll to compute scrollValue but first check if event.deltaY === 0
&& event.deltaX !== 0 and return if true, then proceed to determine direction,
lookup this.keybinds.attackRatioScrollDown/Up, call this.matchesWheelKeybind and
emit new AttackRatioEvent as before.
- Around line 328-336: The keyup handler contains unreachable checks using
matchesKeybind against keybinds.attackRatioScrollDown and attackRatioScrollUp
(which have primary keys like "ScrollDown"/"ScrollUp")—keyboard events never
produce those codes; remove the two if-blocks that call matchesKeybind and emit
AttackRatioEvent from the keyup handler (or alternatively, if you intended
keyboard alternatives, change those keybind definitions to use real keyboard
codes and keep the checks). Keep the existing wheel handling that uses
matchesWheelKeybind (used in the wheel handlers) as-is.
🧹 Nitpick comments (2)
src/client/InputHandler.ts (2)
480-501: Minor:deltaXfallback is now unreachable.After the early return on line 481-483, when
deltaY === 0at line 485,deltaXmust also be zero (otherwise we'd have returned). The ternary is now effectively dead logic.This could be simplified:
- const scrollValue = event.deltaY === 0 ? event.deltaX : event.deltaY; - if (scrollValue === 0) return; + if (event.deltaY === 0) return;
658-684: Consider extracting "Null" to a constant.Line 659 checks for the magic string
"Null"as a disabled marker. For clarity and to prevent typos elsewhere, extract to a constant:const KEYBIND_DISABLED = "Null";This is a minor readability improvement.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/client/InputHandler.ts`:
- Line 21: The MOUSE_CODES set in InputHandler.ts is missing "MouseRight",
causing matchesPointerKeybind() to reject parsed.primary === "MouseRight" even
though getPointerCode() returns "MouseRight"; update the MOUSE_CODES constant
(the Set named MOUSE_CODES) to include "MouseRight" so matchesPointerKeybind()
will find the pointer code and correctly match right-click keybinds (ensure you
keep "MouseLeft" and "MouseMiddle" as well).
🧹 Nitpick comments (2)
src/client/InputHandler.ts (2)
6-6: PreferKeybindMapfor typed keybinds.This avoids stringly-typed access and keeps key names as a typed union.
♻️ Proposed refactor
-import { getDefaultKeybinds } from "./Keybinds"; +import { getDefaultKeybinds, KeybindMap } from "./Keybinds"; - private keybinds: Record<string, string> = {}; + private keybinds: KeybindMap = getDefaultKeybinds(); private reloadKeybindsFromStorage() { - let saved: Record<string, string> = {}; + let saved: Partial<KeybindMap> = {}; try { const parsed = JSON.parse( localStorage.getItem("settings.keybinds") ?? "{}", ); @@ - ) as Record<string, string>; + ) as Partial<KeybindMap>; } catch (e) { console.warn("Invalid keybinds JSON:", e); }Also applies to: 171-172, 598-631
652-677: Reject keybinds with multiple primary keys.Right now,
"KeyA+KeyB"silently keeps only the last primary. That makes broken keybinds hard to spot. Consider treating this as invalid to avoid surprises.🔧 Suggested guard
const modifiers: string[] = []; let primary: string | null = null; for (const part of parts) { if (MODIFIER_CODES.has(part)) { modifiers.push(part); } else { - primary = part; + if (primary) return null; // multiple primaries -> invalid + primary = part; } }
Description:
I've made it so that pretty much everything is a configurable hotkey.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
w.o.n