Fix: Missing binds in settings#3963
Open
sardidefcon wants to merge 4 commits into
Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughAdds configurable keybinds for graphics reset, select-all warships, and box-select hold; implements Alt+ parsing/display/capture, updates InputHandler behavior and defaults, adds UI controls, localization strings, and tests. ChangesKeybind customization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with 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.
Inline comments:
In `@src/client/InputHandler.ts`:
- Around line 877-885: The new keybindMatchesEvent supports modifiers but
several runtime checks (notably the selectAllWarships handler and any places
using a raw shiftKey comparison) still compare only e.code, so saved combos with
Alt+/Shift+ never fire; update those checks to use keybindMatchesEvent (or
parseKeybind + compare shift/alt) against the configured keybind string instead
of raw e.code—specifically replace any direct e.code or e.shiftKey/e.altKey
comparisons in the selectAllWarships logic and the shiftKey check with calls to
keybindMatchesEvent(event, configuredKeybind) so modifier-aware bindings are
honored.
In `@src/client/UserSettingModal.ts`:
- Around line 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.
In `@src/core/game/UserSettings.ts`:
- Line 36: Change introduces a default keybind change for the resetGfx key
(symbol: resetGfx) on the UserSettings default map; add a unit test that asserts
the new default (e.g., "Alt+KeyR") is present in the UserSettings defaults and
that any consumer logic reading UserSettings.defaultKeybinds (or the
UserSettings class/constructor) returns the updated value. Place the test
alongside existing core tests for UserSettings (or create one in the same test
suite) and include an assertion that deserializing/loading UserSettings
preserves the new default keybind so CI catches regressions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 27f5d38e-6764-452e-b3c9-ab9e01c6936f
📒 Files selected for processing (6)
resources/lang/en.jsonsrc/client/InputHandler.tssrc/client/UserSettingModal.tssrc/client/Utils.tssrc/client/components/baseComponents/setting/SettingKeybind.tssrc/core/game/UserSettings.ts
Comment on lines
+651
to
+669
| <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> |
Contributor
There was a problem hiding this comment.
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.
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
Contributor
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)
449-455:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix
shiftKeykeybind default to prevent activation failure.
keybindMatchesEventcheckse.shiftKey === parsed.shift. The default"ShiftLeft"parses as{shift: false, alt: false, code: "ShiftLeft"}. When pressing Shift, the event hase.shiftKey = true, butparsed.shift = false, so the condition fails and box-select mode never activates.Change the default to
"Shift+ShiftLeft"or use a different key binding that works with the current logic, such as"KeyB"(with a Shift+ prefix if shift-modification is needed).🤖 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 449 - 455, The shift keybind default is incorrect: update the default keybind used by InputHandler so parsed.shift is true (e.g., change the default from "ShiftLeft" to "Shift+ShiftLeft" or pick a different binding like "KeyB"/"Shift+KeyB") so keybindMatchesEvent(e, ...) will succeed; then verify behavior in methods keybindMatchesEvent, the shift handling block that sets shiftKeybindHeld, the uiState.ghostStructure check that calls setGhostStructure(null), and the canvas.style.cursor change to "crosshair" still work as expected.
♻️ Duplicate comments (1)
src/client/InputHandler.ts (1)
524-527:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
keybindMatchesEventforselectAllWarshipsto support modifiers.This still uses raw
e.codecomparison. If the user configuresselectAllWarshipsasAlt+KeyForShift+KeyF, it will never trigger because modifiers aren't checked.Fix with modifier-aware matching
- if (e.code === this.keybinds.selectAllWarships) { + if (this.keybindMatchesEvent(e, this.keybinds.selectAllWarships)) { e.preventDefault(); this.eventBus.emit(new SelectAllWarshipsEvent()); }🤖 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 524 - 527, Replace the raw e.code comparison for the selectAllWarships keybind with the modifier-aware helper: use keybindMatchesEvent(this.keybinds.selectAllWarships, e) inside InputHandler (the block that currently checks e.code === this.keybinds.selectAllWarships), and keep the existing e.preventDefault() and this.eventBus.emit(new SelectAllWarshipsEvent()) when it returns true so configured modifiers (Alt/Shift/Ctrl) are respected.
🧹 Nitpick comments (1)
src/client/components/baseComponents/setting/SettingKeybind.ts (1)
122-133: ⚖️ Poor tradeoffConsider consistent display formatting for Alt+ bindings.
Shift+ displays
Shift+B(usinge.key), but Alt+ displaysAlt+KeyR(usinge.code). This inconsistency is intentional to avoid special characters, but might confuse users who see key names for Shift+ and code names for Alt+.If
formatKeyForDisplay()in Utils handles this, ignore this comment. Otherwise, consider extracting the letter from codes likeKeyR→Rfor consistent display.🤖 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/components/baseComponents/setting/SettingKeybind.ts` around lines 122 - 133, In SettingKeybind (the block building code/displayKey using e.code and e.key) the Alt+ branch uses e.code (e.g. "KeyR") while Shift+ uses e.key (e.g. "B"), causing inconsistent UX; update the Alt+ formatting to produce a consistent human-readable display by normalizing e.code (strip common prefixes like "Key"/"Digit"/"Numpad" to the actual character) or call the shared helper formatKeyForDisplay() if available, so displayKey for Alt+ bindings becomes "Alt+R" instead of "Alt+KeyR" while keeping code as `Alt+${e.code}` and preserving existing behavior for other branches.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@src/client/InputHandler.ts`:
- Around line 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).
- Around line 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.
---
Outside diff comments:
In `@src/client/InputHandler.ts`:
- Around line 449-455: The shift keybind default is incorrect: update the
default keybind used by InputHandler so parsed.shift is true (e.g., change the
default from "ShiftLeft" to "Shift+ShiftLeft" or pick a different binding like
"KeyB"/"Shift+KeyB") so keybindMatchesEvent(e, ...) will succeed; then verify
behavior in methods keybindMatchesEvent, the shift handling block that sets
shiftKeybindHeld, the uiState.ghostStructure check that calls
setGhostStructure(null), and the canvas.style.cursor change to "crosshair" still
work as expected.
---
Duplicate comments:
In `@src/client/InputHandler.ts`:
- Around line 524-527: Replace the raw e.code comparison for the
selectAllWarships keybind with the modifier-aware helper: use
keybindMatchesEvent(this.keybinds.selectAllWarships, e) inside InputHandler (the
block that currently checks e.code === this.keybinds.selectAllWarships), and
keep the existing e.preventDefault() and this.eventBus.emit(new
SelectAllWarshipsEvent()) when it returns true so configured modifiers
(Alt/Shift/Ctrl) are respected.
---
Nitpick comments:
In `@src/client/components/baseComponents/setting/SettingKeybind.ts`:
- Around line 122-133: In SettingKeybind (the block building code/displayKey
using e.code and e.key) the Alt+ branch uses e.code (e.g. "KeyR") while Shift+
uses e.key (e.g. "B"), causing inconsistent UX; update the Alt+ formatting to
produce a consistent human-readable display by normalizing e.code (strip common
prefixes like "Key"/"Digit"/"Numpad" to the actual character) or call the shared
helper formatKeyForDisplay() if available, so displayKey for Alt+ bindings
becomes "Alt+R" instead of "Alt+KeyR" while keeping code as `Alt+${e.code}` and
preserving existing behavior for other branches.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 07c23345-6aae-4037-a48f-857cd2fd436e
📒 Files selected for processing (3)
src/client/InputHandler.tssrc/client/Utils.tssrc/client/components/baseComponents/setting/SettingKeybind.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/client/Utils.ts
Comment on lines
+895
to
+899
| private static readonly MOUSE_BUTTON_NAMES: Record<number, string> = { | ||
| 0: "MouseLeft", | ||
| 1: "MouseMiddle", | ||
| 2: "MouseRight", | ||
| }; |
Contributor
There was a problem hiding this comment.
🧩 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.tsRepository: 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 -iRepository: 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 3Repository: 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.tsRepository: 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.tsRepository: 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.
Comment on lines
+915
to
973
| 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; | ||
| } |
Contributor
There was a problem hiding this comment.
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).
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/core/game/UserSettingsKeybinds.test.ts (2)
14-16: 💤 Low valueConsider testing both platform modes for consistency.
Same observation as the
selectAllWarshipstest above—testing both platform flags would improve consistency with theresetGfxandmodifierKeytests.📝 Suggested addition
test("shiftKey (warship box select) defaults to ShiftLeft", () => { expect(getDefaultKeybinds(false).shiftKey).toBe("ShiftLeft"); + expect(getDefaultKeybinds(true).shiftKey).toBe("ShiftLeft"); });🤖 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 `@tests/core/game/UserSettingsKeybinds.test.ts` around lines 14 - 16, The test only asserts getDefaultKeybinds(false).shiftKey; add the same assertion for the other platform mode to ensure consistency: call getDefaultKeybinds(true).shiftKey and expect it toBe("ShiftLeft") (referencing the test in UserSettingsKeybinds.test.ts and the getDefaultKeybinds function) so both platform flags are covered like the other tests (resetGfx, modifierKey).
10-12: 💤 Low valueConsider testing both platform modes for consistency.
The
resetGfxtest above checks bothgetDefaultKeybinds(false)andgetDefaultKeybinds(true), but this test only checksfalse. WhileselectAllWarshipslikely doesn't vary by platform, testing both would make the test suite more consistent and future-proof against platform-specific changes.📝 Suggested addition
test("selectAllWarships defaults to KeyF", () => { expect(getDefaultKeybinds(false).selectAllWarships).toBe("KeyF"); + expect(getDefaultKeybinds(true).selectAllWarships).toBe("KeyF"); });🤖 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 `@tests/core/game/UserSettingsKeybinds.test.ts` around lines 10 - 12, The test only asserts getDefaultKeybinds(false).selectAllWarships; update the "selectAllWarships defaults to KeyF" test to assert both platform modes by also calling getDefaultKeybinds(true).selectAllWarships and expecting "KeyF" so the spec checks both platform variants (referencing getDefaultKeybinds and the selectAllWarships property).
🤖 Prompt for all review comments with 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.
Inline comments:
In `@src/client/InputHandler.ts`:
- Around line 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.
- Around line 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.
---
Nitpick comments:
In `@tests/core/game/UserSettingsKeybinds.test.ts`:
- Around line 14-16: The test only asserts getDefaultKeybinds(false).shiftKey;
add the same assertion for the other platform mode to ensure consistency: call
getDefaultKeybinds(true).shiftKey and expect it toBe("ShiftLeft") (referencing
the test in UserSettingsKeybinds.test.ts and the getDefaultKeybinds function) so
both platform flags are covered like the other tests (resetGfx, modifierKey).
- Around line 10-12: The test only asserts
getDefaultKeybinds(false).selectAllWarships; update the "selectAllWarships
defaults to KeyF" test to assert both platform modes by also calling
getDefaultKeybinds(true).selectAllWarships and expecting "KeyF" so the spec
checks both platform variants (referencing getDefaultKeybinds and the
selectAllWarships property).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b432229c-810d-4c0d-baf7-c656279e5f30
📒 Files selected for processing (8)
resources/lang/en.jsonsrc/client/InputHandler.tssrc/client/UserSettingModal.tssrc/client/Utils.tssrc/client/components/baseComponents/setting/SettingKeybind.tssrc/core/game/UserSettings.tstests/InputHandler.test.tstests/core/game/UserSettingsKeybinds.test.ts
Comment on lines
+217
to
+218
| // True while the configured shiftKey (box-select hold key) is held down | ||
| private shiftKeybindHeld: boolean = false; |
Contributor
There was a problem hiding this comment.
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.
Comment on lines
+579
to
586
| // 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 = ""; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
If this PR fixes an issue, link it below. If not, delete these two lines.
Resolves #3962
Description:
Settings have been added to allow binding keys that could not be customized.
The following actions are now configurable from the keybinds settings menu:
I've encountered several errors when trying to bind key combinations with
Alt+To support
Alt+key combinations, the keybind system was extended:Alt+prefixes alongside the existingShift+supportAlt+Keycombinations correctlyWhen I was testing it, I realized that using
Alt+Keytyped special characters, so:Alt+Keyon certain keyboards and OS layouts (e.g.Alt+R→®) are prevented by using the physical key code instead of the character valueMouse click binding
Additionally, I noticed that it wasn't possible to bind mouse clicks (Left Click or Right Click). I've fixed this by making mouse clicks bindable, but limiting it to only being bound with a specific key combination
(e.g.
Shift+LeftorAlt+Right)The behavior is a bit erratic; I've tried to correct several aspects so that it works. Clicking on the "bind box" resets the function to bind the key, forcing you to click outside of it. I've fixed this, and now the only way to bind a
Key+Clickis by clicking directly on this "bind box."When it came to binding the right click, the classic browser options menu would open. I've added a flag to prevent this from happening so that the right click can be bound correctly.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
sardidefcon