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 shortcut registry, tracks the configured Alt key in ChangesKeyboard registry and Alt key configuration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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): okay |
|
Thanks for the work on this — the refactor approach is sound and worth having. There are two confirmed gameplay regressions that need fixing before this can merge, plus two smaller items worth addressing. Blocking
Non-blocking but worth fixing
Fix the two blocking items and this should be good to go. |
|
@TKTK123456 (via Discord): <@107073252286824448> the reason I didn't fix the selectAllWarship issue is so that there is no conflict because you can rebind shift f to something else as well |
|
@TKTK123456 (via Discord): so it would be two things using the shift f keybind |
|
@Celant (via Discord): As long as <@1334886860538904679> is happy with that caveat, then that's fine. Just sort number 2, and optionally 3 and 4, and we can get it merged :) (For Lewis, this change basically means that Shift+F no longer 'accidentally' works and selects all warships. You have to explicitly let go of shift, or remap it to Shift+F intentionally. The current original behaviour is that it'll select all warships if you hit F, whether or not you're holding shift too) |
|
@TKTK123456 (via Discord): <@107073252286824448> fixed 2. 3 & 4 |
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
|
Almost there — just one small ask before merge. The fix for #4 added a second copy-pasted block for if (
(action === "emojiMenuModifier" && activeKeybinds["altKey"] === value) ||
(action === "altKey" && activeKeybinds["emojiMenuModifier"] === value)
) {
isEmojiMenuModAndAltKeyConflict = true;
}This replaces both the |
|
@TKTK123456 (via Discord): <@107073252286824448> the problem with that is that we only want the "AltLeft" and "AltRight" code to be bypassed |
|
@TKTK123456 (via Discord): so I am going to add |
|
@Celant (via Discord): Ah sorry, I see what you mean |
|
@Celant (via Discord): Yeah that works. Ping once that's pushed and I'll merge |
|
@TKTK123456 (via Discord): <@107073252286824448> pushed |
|
@Celant (via Discord): Sweet, just waiting for CI then I'll merge |
|
@TKTK123456 (via Discord): okay |
|
@TKTK123456 (via Discord): the CI is done |
github-project-automation
Bot
moved this from Development
to Final Review
in OpenFront Release Management
github-project-automation
Bot
moved this from Final Review
to Complete
in OpenFront Release Management
5 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