Skip to content

Fix: Missing binds in settings #3963

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
sardidefcon wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Fix: Missing binds in settings #3963

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7b853a8
Fix: Missing binds in settings
sardidefcon May 18, 2026
f2949fb
Fix: Missing binds in settings
sardidefcon May 18, 2026
de4458e
Solved requested changes by codrabbit
sardidefcon May 18, 2026
ca67d5a
Merge branch 'main' into fix-rebind-certain-keys
sardidefcon May 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions resources/lang/en.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -742,6 +742,8 @@
"view_options": "View Options",
"toggle_view": "Toggle View",
"toggle_view_desc": "Alternate view (terrain/countries)",
"reset_gfx": "Reset Graphics",
"reset_gfx_desc": "Reset / refresh the graphics renderer.",
"build_controls": "Build Controls",
"build_city": "Build City",
"build_city_desc": "Build a City under your cursor.",
Expand All @@ -768,6 +770,8 @@
"build_menu_modifier_desc": "Hold this key while clicking to open the build menu.",
"emoji_menu_modifier": "Emoji Menu Modifier",
"emoji_menu_modifier_desc": "Hold this key while clicking to open the emoji menu.",
"shift_key": "Warship Box Select",
"shift_key_desc": "Hold this key and drag to draw a selection box that selects all warships within it.",
"pause_game": "Pause",
"pause_game_desc": "Pause or resume the game (single player and custom games for host).",
"game_speed_up": "Game Speed Up",
Expand Down Expand Up @@ -795,6 +799,8 @@
"break_alliance_desc": "Break alliance with the player whose tile is under your cursor.",
"swap_direction": "Swap Rocket Direction",
"swap_direction_desc": "Toggle rocket launch direction (up/down).",
"select_all_warships": "Select All Warships",
"select_all_warships_desc": "Select all your warships at once.",
"zoom_controls": "Zoom Controls",
"zoom_out": "Zoom Out",
"zoom_out_desc": "Zoom out the map",
Expand Down
161 changes: 138 additions & 23 deletions src/client/InputHandler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,8 @@ export class InputHandler {
private selectionBoxActive: boolean = false;
// True while warships are selected via box (waiting for move target click)
private multiSelectionActive: boolean = false;
// True while the configured shiftKey (box-select hold key) is held down
private shiftKeybindHeld: boolean = false;
Comment on lines +213 to +214
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot May 18, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Reset shiftKeybindHeld on blur to prevent stuck input mode.

If the window blurs while the hold key is down, this flag can stay true and keep pan-drag blocked after focus returns. Clear it in the existing blur cleanup block with the other transient input state.

Proposed fix 
     window.addEventListener("blur", () => {
       this.activeKeys.clear();
+      this.shiftKeybindHeld = false;
       if (this.alternateView) {
         this.alternateView = false;
         this.eventBus.emit(new AlternateViewEvent(false));
       }
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/client/InputHandler.ts` around lines 217 - 218, The flag shiftKeybindHeld
can remain true if the window blurs while the hold key is down; update the
existing blur cleanup block (the blur/visibilitychange handler that clears other
transient input state) to explicitly set this.shiftKeybindHeld = false so the
boxed-select/pan-drag mode is reset when focus returns.


// Touch long-press state
private longPressTimer: ReturnType<typeof setTimeout> | null = null;
Expand Down Expand Up @@ -307,8 +309,8 @@ export class InputHandler {
let deltaX = 0;
let deltaY = 0;

// Skip if shift is held down
if (this.activeKeys.has(this.keybinds.shiftKey)) {
// Skip if box-select key is held down
if (this.shiftKeybindHeld) {
return;
}

Expand Down Expand Up @@ -433,15 +435,15 @@ export class InputHandler {
this.keybinds.centerCamera,
"ControlLeft",
"ControlRight",
this.keybinds.shiftKey,
].includes(e.code)
) {
this.activeKeys.add(e.code);
}

// Shift = warship box selection mode.
// If a ghost structure is active, discard it first.
if (e.code === this.keybinds.shiftKey) {
if (this.keybindMatchesEvent(e, this.keybinds.shiftKey)) {
this.shiftKeybindHeld = true;
if (this.uiState.ghostStructure !== null) {
this.setGhostStructure(null);
}
Expand Down Expand Up @@ -478,8 +480,7 @@ export class InputHandler {
this.eventBus.emit(new AlternateViewEvent(false));
}

const resetKey = this.keybinds.resetGfx ?? "KeyR";
if (e.code === resetKey && this.isAltKeyHeld(e)) {
if (this.keybindMatchesEvent(e, this.keybinds.resetGfx ?? "Alt+KeyR")) {
e.preventDefault();
this.eventBus.emit(new RefreshGraphicsEvent());
}
Expand Down Expand Up @@ -516,7 +517,7 @@ export class InputHandler {
this.eventBus.emit(new CenterCameraEvent());
}

if (e.code === this.keybinds.selectAllWarships) {
if (this.keybindMatchesEvent(e, this.keybinds.selectAllWarships)) {
e.preventDefault();
this.eventBus.emit(new SelectAllWarshipsEvent());
}
Expand Down Expand Up @@ -571,13 +572,13 @@ export class InputHandler {

this.activeKeys.delete(e.code);

// Reset crosshair when Shift is released (unless selection box or multi-selection still active)
if (
e.code === this.keybinds.shiftKey &&
!this.selectionBoxActive &&
!this.multiSelectionActive
) {
this.canvas.style.cursor = "";
// Reset crosshair when box-select key is released (unless selection box or multi-selection still active)
const parsedShiftKey = this.parseKeybind(this.keybinds.shiftKey);
if (e.code === parsedShiftKey.code) {
this.shiftKeybindHeld = false;
if (!this.selectionBoxActive && !this.multiSelectionActive) {
this.canvas.style.cursor = "";
}
}
Comment on lines +575 to 582
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot May 18, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Release logic should also handle modifier-first keyup for combo binds.

This reset checks only e.code === parsedShiftKey.code. For binds like Alt+KeyR, releasing Alt first leaves shiftKeybindHeld latched until KeyR is released.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/client/InputHandler.ts` around lines 579 - 586, The keyup handler only
compares e.code to parsedShiftKey.code so combo binds like "Alt+KeyR" latch if
the modifier is released first; update the logic in the InputHandler keyup
branch (where parseKeybind(this.keybinds.shiftKey) is used and shiftKeybindHeld
is set) to also detect when the modifier is no longer held (instead of only
matching e.code). Concretely, after getting parsedShiftKey from parseKeybind,
clear shiftKeybindHeld and reset the cursor when either e.code ===
parsedShiftKey.code OR the modifier state for that bind is false (use
e.getModifierState(parsedShiftKey.modifier) or the relevant event property like
e.altKey/e.ctrlKey/e.metaKey as appropriate), and keep the existing guard that
selectionBoxActive and multiSelectionActive prevent resetting. Ensure you
reference parseKeybind, keybinds.shiftKey, shiftKeybindHeld, selectionBoxActive
and multiSelectionActive when making the change.

});
}
Expand Down Expand Up @@ -688,6 +689,11 @@ export class InputHandler {
}
}

if (this.dispatchPointerKeybindActions(event)) {
this.suppressNextTap = false;
return;
}

if (this.isModifierKeyPressed(event)) {
this.suppressNextTap = false;
this.eventBus.emit(new ShowBuildMenuEvent(event.clientX, event.clientY));
Expand Down Expand Up @@ -800,7 +806,7 @@ export class InputHandler {
// started, continue emitting selection box updates
if (
this.selectionBoxActive ||
this.activeKeys.has(this.keybinds.shiftKey) ||
this.shiftKeybindHeld ||
this.longPressActive
) {
this.selectionBoxActive = true;
Expand Down Expand Up @@ -849,24 +855,133 @@ export class InputHandler {
}

/**
* Parses a keybind value that may include a "Shift+" prefix.
* e.g. "Shift+KeyB" → { shift: true, code: "KeyB" }
* "KeyB" → { shift: false, code: "KeyB" }
* Parses a keybind value that may include a "Shift+" or "Alt+" prefix.
* e.g. "Shift+KeyB" → { shift: true, alt: false, code: "KeyB" }
* "Alt+KeyR" → { shift: false, alt: true, code: "KeyR" }
* "KeyB" → { shift: false, alt: false, code: "KeyB" }
*/
private parseKeybind(value: string): { shift: boolean; code: string } {
private parseKeybind(value: string): {
shift: boolean;
alt: boolean;
code: string;
} {
if (value?.startsWith("Shift+")) {
return { shift: true, code: value.slice(6) };
return { shift: true, alt: false, code: value.slice(6) };
}
if (value?.startsWith("Alt+")) {
return { shift: false, alt: true, code: value.slice(4) };
}
return { shift: false, code: value };
return { shift: false, alt: false, code: value };
}

private static readonly MODIFIER_KEY_CODES = new Set([
"ShiftLeft",
"ShiftRight",
"AltLeft",
"AltRight",
"ControlLeft",
"ControlRight",
"MetaLeft",
"MetaRight",
]);

/**
* Returns true if the keyboard event matches the given keybind value,
* including optional Shift+ prefix support.
* including optional Shift+ and Alt+ prefix support.
*/
private keybindMatchesEvent(e: KeyboardEvent, keybindValue: string): boolean {
const parsed = this.parseKeybind(keybindValue);
return e.code === parsed.code && e.shiftKey === parsed.shift;
if (e.code !== parsed.code) return false;
// A bare modifier-key bind (e.g. "ShiftLeft") can't be matched on flag
// state: pressing the key itself sets its own modifier flag to true.
if (
!parsed.shift &&
!parsed.alt &&
InputHandler.MODIFIER_KEY_CODES.has(parsed.code)
) {
return true;
}
return e.shiftKey === parsed.shift && e.altKey === parsed.alt;
}

private static readonly MOUSE_BUTTON_NAMES: Record<number, string> = {
0: "MouseLeft",
1: "MouseMiddle",
2: "MouseRight",
};
Comment on lines +907 to +911
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot May 18, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Search for tests covering MouseRight keybind behavior
rg -n 'MouseRight.*keybind|keybind.*MouseRight' --type=ts

# Check if MouseRight is exposed in UI defaults
rg -n 'MouseRight' src/core/game/UserSettings.ts src/client/UserSettingModal.ts

Repository: openfrontio/OpenFrontIO

Length of output: 49

🏁 Script executed:

# Find all usages of MOUSE_BUTTON_NAMES
rg -n 'MOUSE_BUTTON_NAMES' src/client/ --type=ts

# Check how keybind actions are initialized and filtered
rg -n 'keybind.*action|action.*keybind' src/client/InputHandler.ts --type=ts -A 3

# Look for context menu handling that might conflict with MouseRight
rg -n 'contextmenu|context.menu|rightclick|right.click' src/client/ --type=ts -i

Repository: openfrontio/OpenFrontIO

Length of output: 3796

🏁 Script executed:

# Find keybindMatchesPointerEvent implementation
rg -n 'keybindMatchesPointerEvent' src/client/InputHandler.ts -A 20

# Check if there's filtering of button 2 in keybind logic
rg -n 'e\.button|button.*2|MouseRight' src/client/InputHandler.ts -B 2 -A 2

# Look for keybind defaults/configuration
rg -n 'keybind.*default|default.*keybind' src/core/game/UserSettings.ts -A 3

Repository: openfrontio/OpenFrontIO

Length of output: 2031

🏁 Script executed:

# Check keybind defaults in UserSettings
rg -n 'keybinds|Keybind' src/core/game/UserSettings.ts -A 2 -B 2

# Look for any MouseRight in default keybinds
rg -n 'MouseRight' src/core/game/ --type=ts

# Check what keybinds are actually bound by default
rg -n 'boatAttack|boatMove' src/core/game/UserSettings.ts

Repository: openfrontio/OpenFrontIO

Length of output: 1566

🏁 Script executed:

# Get the full getDefaultKeybinds function
rg -n 'export function getDefaultKeybinds' src/core/game/UserSettings.ts -A 50

# Check if any default keybinds use mouse buttons
rg -n 'MouseLeft|MouseMiddle|MouseRight' src/core/game/UserSettings.ts

Repository: openfrontio/OpenFrontIO

Length of output: 1731

Remove MouseRight from keybind support to match established constraints.

The MOUSE_BUTTON_NAMES constant includes button 2 (MouseRight), and keybindMatchesPointerEvent() has no filtering, allowing MouseRight to be recorded and matched as a keybind action. This violates the established pattern: only MouseLeft and MouseMiddle should be supported for keybind actions. MouseRight must be reserved to prevent conflicts with context menu behavior.

Remove the MouseRight entry from MOUSE_BUTTON_NAMES or add filtering in keybindMatchesPointerEvent() to skip button 2.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/client/InputHandler.ts` around lines 895 - 899, The MOUSE_BUTTON_NAMES
map currently exposes button 2 ("MouseRight") and keybindMatchesPointerEvent()
does not filter it, allowing right-clicks to be recorded as keybinds; update the
code so right-click is excluded by removing the 2: "MouseRight" entry from
MOUSE_BUTTON_NAMES and/or add an explicit guard in keybindMatchesPointerEvent()
to return false when event.button === 2, ensuring only MouseLeft (0) and
MouseMiddle (1) are considered for keybind matching.


private keybindMatchesPointerEvent(
e: PointerEvent,
keybindValue: string,
): boolean {
if (!keybindValue) return false;
const parsed = this.parseKeybind(keybindValue);
const buttonName = InputHandler.MOUSE_BUTTON_NAMES[e.button];
return (
parsed.code === buttonName &&
e.shiftKey === parsed.shift &&
e.altKey === parsed.alt
);
}

private dispatchPointerKeybindActions(event: PointerEvent): boolean {
const actions: Array<[string, () => void]> = [
[
this.keybinds.boatAttack,
() => this.eventBus.emit(new DoBoatAttackEvent()),
],
[
this.keybinds.groundAttack,
() => this.eventBus.emit(new DoGroundAttackEvent()),
],
[
this.keybinds.retaliateAttack,
() => this.eventBus.emit(new DoRetaliateAttackEvent()),
],
[
this.keybinds.requestAlliance,
() => this.eventBus.emit(new DoRequestAllianceEvent()),
],
[
this.keybinds.breakAlliance,
() => this.eventBus.emit(new DoBreakAllianceEvent()),
],
[
this.keybinds.swapDirection,
() => this.eventBus.emit(new SwapRocketDirectionEvent(true)),
],
[
this.keybinds.selectAllWarships,
() => this.eventBus.emit(new SelectAllWarshipsEvent()),
],
[
this.keybinds.centerCamera,
() => this.eventBus.emit(new CenterCameraEvent()),
],
[
this.keybinds.pauseGame,
() => this.eventBus.emit(new TogglePauseIntentEvent()),
],
[
this.keybinds.gameSpeedUp,
() => this.eventBus.emit(new GameSpeedUpIntentEvent()),
],
[
this.keybinds.gameSpeedDown,
() => this.eventBus.emit(new GameSpeedDownIntentEvent()),
],
[
this.keybinds.resetGfx ?? "Alt+KeyR",
() => this.eventBus.emit(new RefreshGraphicsEvent()),
],
];
for (const [keybind, action] of actions) {
if (this.keybindMatchesPointerEvent(event, keybind)) {
action();
return true;
}
}
return false;
}
Comment on lines +927 to 985
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot May 18, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Fix swapDirection to toggle instead of forcing true.

Line 939 hardcodes true, but the keyboard handler at line 550 toggles based on !this.uiState.rocketDirectionUp. Mouse and keyboard bindings should have identical behavior.

Proposed fix 
       [
         this.keybinds.swapDirection,
-        () => this.eventBus.emit(new SwapRocketDirectionEvent(true)),
+        () => {
+          const nextDirection = !this.uiState.rocketDirectionUp;
+          this.eventBus.emit(new SwapRocketDirectionEvent(nextDirection));
+        },
       ],
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/client/InputHandler.ts` around lines 915 - 973, The swapDirection pointer
action currently emits SwapRocketDirectionEvent(true) which forces upward
direction; change the action in dispatchPointerKeybindActions for
keybinds.swapDirection to emit new
SwapRocketDirectionEvent(!this.uiState.rocketDirectionUp) so the pointer binding
toggles the rocket direction the same way the keyboard handler does (see the
keyboard toggle that reads this.uiState.rocketDirectionUp).


/**
Expand Down
31 changes: 30 additions & 1 deletion src/client/UserSettingModal.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,6 @@ export class UserSettingModal extends BaseModal {
activeKeybinds[k] = normalizedValue;
}
}

const values = Object.entries(activeKeybinds)
.filter(([k]) => k !== action)
.map(([, v]) => v);
Expand Down Expand Up @@ -401,6 +400,16 @@ export class UserSettingModal extends BaseModal {
@change=${this.handleKeybindChange}
></setting-keybind>

<setting-keybind
action="resetGfx"
label=${translateText("user_setting.reset_gfx")}
description=${translateText("user_setting.reset_gfx_desc")}
.defaultKey=${this.defaultKeybinds.resetGfx}
.value=${this.getKeyValue("resetGfx")}
.display=${this.getKeyChar("resetGfx")}
@change=${this.handleKeybindChange}
></setting-keybind>

<h2
class="text-blue-200 text-xl font-bold mt-8 mb-3 border-b border-white/10 pb-2"
>
Expand Down Expand Up @@ -639,6 +648,26 @@ export class UserSettingModal extends BaseModal {
@change=${this.handleKeybindChange}
></setting-keybind>

<setting-keybind
action="selectAllWarships"
label=${translateText("user_setting.select_all_warships")}
description=${translateText("user_setting.select_all_warships_desc")}
.defaultKey=${this.defaultKeybinds.selectAllWarships}
.value=${this.getKeyValue("selectAllWarships")}
.display=${this.getKeyChar("selectAllWarships")}
@change=${this.handleKeybindChange}
></setting-keybind>

<setting-keybind
action="shiftKey"
label=${translateText("user_setting.shift_key")}
description=${translateText("user_setting.shift_key_desc")}
.defaultKey=${this.defaultKeybinds.shiftKey}
.value=${this.getKeyValue("shiftKey")}
.display=${this.getKeyChar("shiftKey")}
@change=${this.handleKeybindChange}
></setting-keybind>
Comment on lines +651 to +669
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot May 18, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

New warship keybind settings can save combos that gameplay may not execute.

These new controls accept modifier combos, but runtime matching for these actions is still not fully modifier-aware. Users can configure valid-looking binds that won’t trigger.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/client/UserSettingModal.ts` around lines 651 - 669, The new warship
keybind controls allow modifier combos that the runtime doesn't support; update
the keybind-saving flow to validate/normalize binds for the affected actions
(e.g., action="selectAllWarships" and action="shiftKey") by changing
handleKeybindChange to detect modifier-inclusive combos and either strip
modifiers or reject them (fallback to this.defaultKeybinds.selectAllWarships)
before persisting, and ensure getKeyValue/getKeyChar are consistent with that
normalized format so saved values cannot be a modifier-only or
modifier-combination that won’t match at runtime.


<h2
class="text-blue-200 text-xl font-bold mt-8 mb-3 border-b border-white/10 pb-2"
>
Expand Down
10 changes: 10 additions & 0 deletions src/client/Utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,16 @@ export function formatKeyForDisplay(value: string): string {
return "Shift+" + formatKeyForDisplay(value.slice(6));
}

// Handle Alt+ prefix: format as "Alt+X"
if (value.startsWith("Alt+")) {
return "Alt+" + formatKeyForDisplay(value.slice(4));
}

// Handle mouse buttons
if (value === "MouseLeft") return "Left";
if (value === "MouseRight") return "Right";
if (value === "MouseMiddle") return "Middle";

// Handle space character or "Space" key
if (value === " " || value === "Space") return "Space";

Expand Down
Loading
Loading