Input handler.ts rework#4225
Conversation
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughInputHandler now uses a registry for keyboard shortcuts. It stores keybind handlers with conditions, matches keys with real or synthesized events, tracks Alt keys in activeKeys, runs handlers from the registry-based keydown flow, and exposes the Alt-key modifier as a user-configurable setting for graphics refresh. ChangesKeyboard event data-driven refactoring and Alt-key configuration
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser
participant InputHandler as InputHandler.keydown
participant Registry as keybindAndEvent Array
participant Matcher as keybindMatchesEvent
participant Active as activeKeys Set
participant Handler as registered handler
Browser->>InputHandler: keydown (KeyboardEvent)
InputHandler->>Active: add configured Alt code when pressed
InputHandler->>Registry: iterate entries
Registry->>Matcher: match entry against event or synthesized object
Matcher->>Active: consult activeKeys for modifier predicates
InputHandler->>Registry: evaluate predicates, call preventDefault()
Registry->>Handler: invoke handler when predicates pass
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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: 1
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)
1-1:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPrettier formatting issues reported by CI.
Run
npx prettier --write src/client/InputHandler.tsto fix the code style issues flagged by the pipeline.🤖 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` at line 1, This file has Prettier style violations; run the formatter and commit the changes by executing npx prettier --write src/client/InputHandler.ts (or run the project's format script) to update formatting for the InputHandler module; after formatting, verify that imports like EventBus and GameEvent and any exported class/function in InputHandler.ts still compile and then re-run CI.Source: Pipeline failures
🧹 Nitpick comments (1)
src/client/InputHandler.ts (1)
929-941: ⚡ Quick winTypo in JSDoc and consider a typed structure.
Line 932:
"exectued"→"executed"The array-based convention
[action, ...conditions]works but requires knowing the index contract. A typed object would be clearer and catch misuse at compile time:interface KeybindEntry { action: (e: KeyboardEvent) => void; conditions?: Array<(e: KeyboardEvent) => boolean>; } private keybindAndEvent = new Map<string, KeybindEntry>();This makes the structure explicit and gives proper types for actions (void) vs conditions (boolean).
🔤 Minimal fix for typo
- * `@param` event The code to be exectued when this keybind is pressed + * `@param` event The code to be executed when this keybind is pressed🤖 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 929 - 941, Fix the JSDoc typo ("exectued" → "executed") and replace the fragile array convention used by keybindAndEvent with a typed object: define a KeybindEntry (with action: (e: KeyboardEvent) => void and optional conditions: Array<(e: KeyboardEvent) => boolean>), change the Map type to Map<string, KeybindEntry>, and update addKeybindAndEvent to accept an action and conditions with those explicit types (use the symbol addKeybindAndEvent and keybindAndEvent to locate the code); also update any code that reads from keybindAndEvent to use entry.action and entry.conditions and adjust return/usage expectations accordingly.
🤖 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 580-592: Rename the misspelled local variable
allConditionsFullfiled to allConditionsFulfilled in the loop inside the
InputHandler logic that iterates over this.keybindAndEvent (the block that uses
keybindMatchesEvent and item[1].slice(1)), updating every reference (assignment,
checks, and the continue branch) to the new name to keep readability consistent;
ensure the same corrected name is used throughout that function scope and no
other behavior changes are introduced.
---
Outside diff comments:
In `@src/client/InputHandler.ts`:
- Line 1: This file has Prettier style violations; run the formatter and commit
the changes by executing npx prettier --write src/client/InputHandler.ts (or run
the project's format script) to update formatting for the InputHandler module;
after formatting, verify that imports like EventBus and GameEvent and any
exported class/function in InputHandler.ts still compile and then re-run CI.
---
Nitpick comments:
In `@src/client/InputHandler.ts`:
- Around line 929-941: Fix the JSDoc typo ("exectued" → "executed") and replace
the fragile array convention used by keybindAndEvent with a typed object: define
a KeybindEntry (with action: (e: KeyboardEvent) => void and optional conditions:
Array<(e: KeyboardEvent) => boolean>), change the Map type to Map<string,
KeybindEntry>, and update addKeybindAndEvent to accept an action and conditions
with those explicit types (use the symbol addKeybindAndEvent and keybindAndEvent
to locate the code); also update any code that reads from keybindAndEvent to use
entry.action and entry.conditions and adjust return/usage expectations
accordingly.
🪄 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: 09de9720-a581-40ca-9829-091e215d1987
📒 Files selected for processing (1)
src/client/InputHandler.ts
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
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 317-333: Prettier formatting errors are caused by the block that
defines and transforms buildKeybinds in InputHandler.ts; reformat this section
(variable buildKeybinds, the .map using this.keybinds, the push of numpad keys,
and the for-loop) to match the project's Prettier rules by running the formatter
(run `prettier --write src/client/InputHandler.ts`) or apply equivalent
formatting changes so the file passes CI.
- Around line 317-333: The build keybind registration only adds Numpad0..Numpad9
extras so pressing Digit1 when the keybind is set to Numpad1 bypasses the
fallback; update the registration logic in InputHandler (the buildKeybinds array
mapping and the subsequent push of extras) to also include the Digit0..Digit9
counterparts for each Numpad key (or add a small helper that, for any "NumpadX"
adds both "NumpadX" and "DigitX") so resolveBuildKeybind() will be reached on
both Numpad and Digit keyups; apply the same dual-registration change to the
analogous block around lines 335-347 to cover all build-related keybinds.
- Around line 299-301: Remove the debug console.log calls from the input hot
paths: delete the console.log("TogglePerformanceOverlayEvent") inside the
addKeybindAndEvent handler that emits TogglePerformanceOverlayEvent (and any
other console.log added in the same file during init, e.g., the one around line
334-335). Locate the usages of addKeybindAndEvent and the init/startup code in
InputHandler (search for the string "TogglePerformanceOverlayEvent" and
"console.log") and simply remove the console.log statements so the handler only
calls this.eventBus.emit(new TogglePerformanceOverlayEvent()) and initialization
code has no noisy logs.
🪄 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: d0ddfca9-2f39-45ab-94f4-fd62e41f7d9b
📒 Files selected for processing (1)
src/client/InputHandler.ts
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
|
@TKTK123456 (via Discord): I do want this to be implemented by v32 or early v33 |
|
@TKTK123456 (via Discord): And think it should be |
|
@TKTK123456 (via Discord): <@516828441065160714> |
Celant
left a comment
Celant
left a comment
There was a problem hiding this comment.
Re-reviewed against the previous round — this is a big step up, nice work. Both of the critical issues are genuinely fixed:
- Same-key collisions no longer drop handlers. Switching
keybindAndEventto an array + iterating every entry (withcontinue outerLoopon a failed condition) restores the old "all matching actions fire" behavior. This was the main one. - Digit↔Numpad aliasing works both ways again now that both
Digit0–9andNumpad0–9are registered as triggers.
Also resolved: the typed KeybindEntry (no more item[1][0]/.slice(1)), early-exit on the first failed condition, the allConditionsFullfiled typo, the debug console.log, and the junk-entry cleanup via the Set. And making altKey a real rebindable setting is a nicer fix than the old isAltKeyHeld — it also fixes the latent bug where Alt+R did nothing for default users.
The one thing I'd block on: tests
This is the same ask as last round — the diff touches 4 files but adds no tests, and the checklist still says tests were added. The two bugs you just fixed are exactly the kind a couple of unit tests would lock down so they can't regress.
Here's a bare-bones starting point (mirrors the (x as any) private-poke style used in ActionableEventsAlliance.test.ts) — it guards the collision fix specifically. It's deliberately minimal; the real value is extending it:
import { InputHandler } from "../../src/client/InputHandler";
describe("InputHandler keybind registry", () => {
function makeHandler() {
return new InputHandler(
{} as any, // gameView
{} as any, // uiState
document.createElement("div"), // canvas
{} as any, // eventBus
);
}
test("two actions bound to the same key are both kept (no overwrite)", () => {
const ih = makeHandler() as any;
ih.keybindAndEvent = [];
ih.addKeybindAndEvent("KeyX", () => {});
ih.addKeybindAndEvent("KeyX", () => {});
const entries = ih.keybindAndEvent.filter(([k]: [string, unknown]) => k === "KeyX");
expect(entries.length).toBe(2); // would have been 1 with the old Map
});
});From here the cases worth adding (leaving these to you): a synthesized keydown actually dispatches the matching handler; a failing condition skips it; pressing Digit1 resolves a build bound to Numpad1 and vice-versa. Those last two are what would have caught the original bugs automatically.
A few smaller things — mostly just confirm the intent
These came out of the refactor and may well be fine, but they're behavior changes from the old code, so flagging to make sure they're deliberate rather than accidental:
selectAllWarshipsnow requires Shift to be up. The old code matched on rawe.codeand ignored Shift, so it fired even with Shift held; the new plain registration goes throughkeybindMatchesEvent, which requiresshiftKey === false. Is dropping the Shift-held case intended?- Alt+Shift+R no longer refreshes graphics, same mechanism —
resetKeyonly matches with Shift up, whereas the oldisAltKeyHeldpath ignored Shift. Intended? - Only the configured
altKey(defaultAltLeft) is tracked, soAltRightno longer works as the refresh modifier. The oldisAltKeyHeldkeyed offevent.altKey, which is true for either physical Alt. Worth a conscious decision since some players use right-Alt.
Minor nits (non-blocking)
- The null-filter doesn't catch what it looks like:
this.keybinds[i]returnsundefined(notnull) for a missing key, so.filter((v): v is string => v !== null)letsundefined/"Shift+undefined"through. Harmless (they never match), but!= nullorBoolean(v)would actually clean them. - The conflict-suppression in
UserSettingModalis hardcoded to theemojiMenuModifier/altKeypair against the literal"AltLeft". It works because both default toAltLeft, but it'll break silently if a default changes and doesn't generalize to other overlapping modifiers — a general "modifiers may overlap" rule would be sturdier. addKeybindAndEvent's parameters are still typed(type: KeyboardEvent) => anywith the param namedtype(the storedKeybindEntryis correctly typed now), and the JSDoc still says "exectued". Cosmetic.- Optional: dispatch is now a linear scan of ~75 entries calling
parseKeybindon every keydown. AMap<normalizedKey, entry[]>would be O(1) and keep the all-matches behavior. Not blocking.
Overall the core refactor looks good to me — add the tests and confirm the three Shift/Alt behavior questions above and I think this is in good shape.
The three shift/alt behaviour is intentional, for the shift behaviour it avoids conflicts with other keybinds and the alt behaviour matches the help menu |
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/InputHandler.test.ts (1)
640-653: ⚡ Quick winUse the shared
setup()helper for these new tests.These new test blocks instantiate
InputHandlerdirectly instead of usingtests/util/Setup.ts. Please migrate them tosetup()so they run through the standard game simulation wiring and stay consistent with repo test conventions.As per coding guidelines: "
tests/**/*.test.ts: Use thesetup()helper fromtests/util/Setup.tsto create test game instances and exercise core simulation directly."Also applies to: 674-680
🤖 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/InputHandler.test.ts` around lines 640 - 653, The test blocks in tests/InputHandler.test.ts are directly instantiating InputHandler instead of using the shared setup() helper from tests/util/Setup.ts. Replace the manual beforeEach block at lines 640-653 that creates a UIState object and instantiates InputHandler directly with a call to the setup() helper, which provides the standard game simulation wiring and maintains consistency with repository test conventions. The same fix applies at lines 674-680 where InputHandler is also being instantiated directly instead of through setup().Source: Coding guidelines
🤖 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 `@tests/InputHandler.test.ts`:
- Around line 640-658: The testSettings.setKeybinds() call is currently
positioned after inputHandler.initialize() in the beforeEach hook, which means
the InputHandler instance is initialized before the custom Numpad keybinds are
set. Move the testSettings.setKeybinds() call to occur before the
inputHandler.initialize() call so that the InputHandler reads the correct custom
keybinds during its initialization, ensuring the test properly validates the
Numpad-bound configuration instead of testing with default keybinds.
---
Nitpick comments:
In `@tests/InputHandler.test.ts`:
- Around line 640-653: The test blocks in tests/InputHandler.test.ts are
directly instantiating InputHandler instead of using the shared setup() helper
from tests/util/Setup.ts. Replace the manual beforeEach block at lines 640-653
that creates a UIState object and instantiates InputHandler directly with a call
to the setup() helper, which provides the standard game simulation wiring and
maintains consistency with repository test conventions. The same fix applies at
lines 674-680 where InputHandler is also being instantiated directly instead of
through setup().
🪄 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: 8d2a3df0-1ae6-4fb8-8e0f-3c288717d107
📒 Files selected for processing (3)
resources/lang/en.jsonsrc/client/InputHandler.tstests/InputHandler.test.ts
✅ Files skipped from review due to trivial changes (1)
- resources/lang/en.json
🚧 Files skipped from review as they are similar to previous changes (1)
- src/client/InputHandler.ts
4 participants
Add approved & assigned issue number here:
Resolves #4193
Description:
Use activeKeys set in places where it is checking if a key is being pressed in a different way, and it makes more sense to use the activeKeys set. Make the overall code of the InputHandler.ts file more consistent and to make it easier to add new keybinds in the future.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
tktk1234567