Make hotkeys great again #2946
Make hotkeys great again #2946
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughCentralizes and exposes default keybinds, adds composite keyboard/mouse/wheel keybind support, updates UI capture/validation and help/unit displays to use dynamic keybinds, reloads keybinds at runtime, 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; } }
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/client/HelpModal.ts`:
- Around line 2-6: There are duplicate imports causing TS2300; consolidate the
duplicated import groups by merging the lit/decorators.js imports into one
statement that exports customElement, query and state, and merge the
../client/Utils imports into one statement that exports getDefaultKeybinds,
translateText and TUTORIAL_VIDEO_URL (so replace the two separate import blocks
with a single import from "lit/decorators.js" containing customElement, query,
state and a single import from "../client/Utils" containing getDefaultKeybinds,
translateText, TUTORIAL_VIDEO_URL).
- Around line 37-46: The code that normalizes stored keybind arrays in HelpModal
(the branch handling typeof v === "object" with "value" and the Array.isArray(v)
branch) joins arrays with a space, which mismatches getKeyLabel's expectation of
"+"-delimited composites; change both join calls to use "+" as the delimiter so
array-based keybinds (e.g., ["ShiftLeft","KeyR"]) become "ShiftLeft+KeyR" and
are parsed correctly by getKeyLabel.
🧹 Nitpick comments (2)
src/client/HelpModal.ts (2)
348-368: Two rows share the same action label for ratio change.Lines 348–358 (keyboard) and 359–368 (scroll wheel) both display
help_modal.action_ratio_change. Users might wonder why there are two identical-looking rows. Consider adding a note like "(Alternate)" to the scroll variant, or combining them into one row.
35-35: Redundant cast after type guard.
isKeybindObjectalready narrowsvto{ value: string }, so(v as any).valuecan be simplified tov.value.♻️ Tiny cleanup
- if (this.isKeybindObject(v)) return [k, (v as any).value]; + if (this.isKeybindObject(v)) return [k, v.value];
| import { customElement, state } from "lit/decorators.js"; | ||
| import { getDefaultKeybinds } from "../client/Keybinds"; | ||
| import { translateText } from "../client/Utils"; | ||
| import { customElement, query, state } from "lit/decorators.js"; | ||
| import { translateText, TUTORIAL_VIDEO_URL } from "../client/Utils"; |
There was a problem hiding this comment.
Duplicate imports break the build.
Lines 2–4 duplicate the imports already on lines 5–6. The CI pipeline fails with TS2300: Duplicate identifier for customElement, state, and translateText. Merge both groups into single import statements and keep query and TUTORIAL_VIDEO_URL which lines 2–4 dropped.
🐛 Fix: merge into two clean imports
-import { customElement, state } from "lit/decorators.js";
-import { getDefaultKeybinds } from "../client/Keybinds";
-import { translateText } from "../client/Utils";
-import { customElement, query, state } from "lit/decorators.js";
-import { translateText, TUTORIAL_VIDEO_URL } from "../client/Utils";
+import { customElement, query, state } from "lit/decorators.js";
+import { getDefaultKeybinds } from "../client/Keybinds";
+import { translateText, TUTORIAL_VIDEO_URL } from "../client/Utils";📝 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.
| import { customElement, state } from "lit/decorators.js"; | |
| import { getDefaultKeybinds } from "../client/Keybinds"; | |
| import { translateText } from "../client/Utils"; | |
| import { customElement, query, state } from "lit/decorators.js"; | |
| import { translateText, TUTORIAL_VIDEO_URL } from "../client/Utils"; | |
| import { customElement, query, state } from "lit/decorators.js"; | |
| import { getDefaultKeybinds } from "../client/Keybinds"; | |
| import { translateText, TUTORIAL_VIDEO_URL } from "../client/Utils"; |
🧰 Tools
🪛 Biome (2.3.13)
[error] 5-5: Shouldn't redeclare 'customElement'. Consider to delete it or rename it.
'customElement' is defined here:
(lint/suspicious/noRedeclare)
[error] 5-5: Shouldn't redeclare 'state'. Consider to delete it or rename it.
'state' is defined here:
(lint/suspicious/noRedeclare)
[error] 6-6: Shouldn't redeclare 'translateText'. Consider to delete it or rename it.
'translateText' is defined here:
(lint/suspicious/noRedeclare)
🪛 GitHub Actions: 🧪 CI
[error] 2-2: Duplicate identifier 'customElement'.
🪛 GitHub Actions: 🚀 Deploy
[error] 2-6: Duplicate identifier 'customElement'. Duplicate identifier 'state'. Duplicate identifier 'translateText'.
[error] 2-6: TS2300: Duplicate identifier errors detected during TypeScript compilation in HelpModal.ts.
🤖 Prompt for AI Agents
In `@src/client/HelpModal.ts` around lines 2 - 6, There are duplicate imports
causing TS2300; consolidate the duplicated import groups by merging the
lit/decorators.js imports into one statement that exports customElement, query
and state, and merge the ../client/Utils imports into one statement that exports
getDefaultKeybinds, translateText and TUTORIAL_VIDEO_URL (so replace the two
separate import blocks with a single import from "lit/decorators.js" containing
customElement, query, state and a single import from "../client/Utils"
containing getDefaultKeybinds, translateText, TUTORIAL_VIDEO_URL).
| if ( | ||
| typeof v === "object" && | ||
| v !== null && | ||
| "value" in (v as any) && | ||
| Array.isArray((v as any).value) | ||
| ) { | ||
| return [k, (v as any).value.join(" ")]; | ||
| } | ||
|
|
||
| if (Array.isArray(v)) return [k, v.join(" ")]; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd src/client && head -100 HelpModal.ts | tail -80Repository: openfrontio/OpenFrontIO
Length of output: 2247
🏁 Script executed:
# Find the full HelpModal.ts file and check its size
wc -l src/client/HelpModal.tsRepository: openfrontio/OpenFrontIO
Length of output: 95
🏁 Script executed:
# Look for getKeyLabel function
rg "getKeyLabel" src/client/HelpModal.ts -A 15Repository: openfrontio/OpenFrontIO
Length of output: 1194
🏁 Script executed:
# Look for getDefaultKeybinds
rg "getDefaultKeybinds\|DEFAULT_KEYBINDS" src/client/ -A 10Repository: openfrontio/OpenFrontIO
Length of output: 49
🏁 Script executed:
# Check the full getKeybinds method
rg "getKeybinds\(\)" src/client/HelpModal.ts -A 40Repository: openfrontio/OpenFrontIO
Length of output: 1938
🏁 Script executed:
# Search for getDefaultKeybinds function definition
rg "function getDefaultKeybinds|const getDefaultKeybinds|export.*getDefaultKeybinds" src/client/ -A 20Repository: openfrontio/OpenFrontIO
Length of output: 222
🏁 Script executed:
# Look in the entire src/client directory for DEFAULT_KEYBINDS or getDefaultKeybinds
rg "getDefaultKeybinds" src/ -B 2 -A 25Repository: openfrontio/OpenFrontIO
Length of output: 8863
🏁 Script executed:
# Check the Keybinds.ts file to see DEFAULT_KEYBINDS
cat -n src/client/Keybinds.ts | head -60Repository: openfrontio/OpenFrontIO
Length of output: 1751
Array values joined with space, but getKeyLabel splits on +.
When a stored keybind value is an array (e.g. ["ShiftLeft", "KeyR"]), it gets joined with " " (space). But getKeyLabel splits composite keys on "+" and all default keybinds use "+" as the composite delimiter (e.g. "ShiftLeft+KeyR"). This means array-based composite keybinds will render as an unrecognized label like "ShiftLeft KeyR" instead of "⇧ Shift + R".
Join arrays with "+" to match the default format and parsing logic.
Fix: use "+" as the join delimiter
Array.isArray((v as any).value)
) {
- return [k, (v as any).value.join(" ")];
+ return [k, (v as any).value.join("+")];
}
- if (Array.isArray(v)) return [k, v.join(" ")];
+ if (Array.isArray(v)) return [k, v.join("+")];📝 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.
| if ( | |
| typeof v === "object" && | |
| v !== null && | |
| "value" in (v as any) && | |
| Array.isArray((v as any).value) | |
| ) { | |
| return [k, (v as any).value.join(" ")]; | |
| } | |
| if (Array.isArray(v)) return [k, v.join(" ")]; | |
| if ( | |
| typeof v === "object" && | |
| v !== null && | |
| "value" in (v as any) && | |
| Array.isArray((v as any).value) | |
| ) { | |
| return [k, (v as any).value.join("+")]; | |
| } | |
| if (Array.isArray(v)) return [k, v.join("+")]; |
🤖 Prompt for AI Agents
In `@src/client/HelpModal.ts` around lines 37 - 46, The code that normalizes
stored keybind arrays in HelpModal (the branch handling typeof v === "object"
with "value" and the Array.isArray(v) branch) joins arrays with a space, which
mismatches getKeyLabel's expectation of "+"-delimited composites; change both
join calls to use "+" as the delimiter so array-based keybinds (e.g.,
["ShiftLeft","KeyR"]) become "ShiftLeft+KeyR" and are parsed correctly by
getKeyLabel.
|
What about "retaliate"? will that now have a key? because it would be really useful to default it to "r", i don't see anything about this. |
this PR is more to make it that everything is changeable - that would be a separate PR |
2 participants
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