Alt + Scroll Wheel for AR & Ctrl + Numb for structure filtering #2236
Conversation
…mber for structure filtering
|
|
|
Warning Rate limit exceeded@ryanbarlow97 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 29 minutes and 33 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds Alt+Scroll to adjust attack ratio and Ctrl+Number shortcuts to filter/highlight structures; updates InputHandler to handle Alt keys, new onAltScroll, Ctrl+Digit prevention of browser tab switching, and emitting ToggleStructureEvent (including null on Ctrl release). Documentation and tests added. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant InputHandler
participant EventBus
rect rgba(200,230,255,0.4)
Note over User,InputHandler: Alt + Scroll → adjust attack ratio
User->>InputHandler: wheel event (altKey=true)
InputHandler->>InputHandler: onAltScroll(event) -- compute delta
InputHandler->>EventBus: emit AttackRatioEvent(delta)
end
rect rgba(235,245,210,0.4)
Note over User,InputHandler: Ctrl + Number → toggle structure visibility
User->>InputHandler: keydown (Ctrl + DigitN)
InputHandler->>InputHandler: detect Ctrl+Digit → map to UnitType
InputHandler->>EventBus: emit ToggleStructureEvent(UnitType)
InputHandler-->>User: preventDefault(browser Ctrl+Digit)
User->>InputHandler: keyup (Ctrl released)
InputHandler->>EventBus: emit ToggleStructureEvent(null)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks✅ Passed checks (3 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 |
There was a problem hiding this comment.
Pull Request Overview
Adds alternative input bindings: Alt + mouse wheel to adjust attack ratio and Ctrl + number keys to filter visible structures. Also updates input handling to prevent browser Ctrl+Digit tab switching and documents/testing steps for these features.
- Alt + Scroll adjusts attack ratio; normal zoom suppressed when Alt is held
- Ctrl + Digit filters structures and prevents browser tab switching; releasing Ctrl restores visibility
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/client/InputHandler.ts | Implements Alt-scroll handling, Ctrl+Digit filtering, tab-switch prevention, and key tracking updates |
| TEST_NEW_FEATURES.md | Adds manual QA steps and expected outcomes for the new controls |
| CONTROL_FEATURES.md | Documents the new controls, implementation notes, and usage guidance |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/client/InputHandler.ts (3)
367-395: Refactor structureMap to a class constant.The
structureMapobject is recreated on every keyup event. Define it as a private static readonly or class constant to avoid repeated allocations.Apply this refactor by adding the constant near the top of the class:
private readonly ZOOM_SPEED = 10; + private readonly STRUCTURE_MAP: Record<string, UnitType> = { + "1": UnitType.City, + "2": UnitType.Factory, + "3": UnitType.Port, + "4": UnitType.DefensePost, + "5": UnitType.MissileSilo, + "6": UnitType.SAMLauncher, + "7": UnitType.Warship, + "8": UnitType.AtomBomb, + "9": UnitType.HydrogenBomb, + "0": UnitType.MIRV, + }; + private readonly userSettings: UserSettings = new UserSettings();Then use it in the handler:
e.preventDefault(); const digit = digitMatch[1]; - const structureMap: Record<string, UnitType> = { - "1": UnitType.City, - "2": UnitType.Factory, - "3": UnitType.Port, - "4": UnitType.DefensePost, - "5": UnitType.MissileSilo, - "6": UnitType.SAMLauncher, - "7": UnitType.Warship, - "8": UnitType.AtomBomb, - "9": UnitType.HydrogenBomb, - "0": UnitType.MIRV, - }; - const structureType = structureMap[digit]; + const structureType = this.STRUCTURE_MAP[digit]; if (structureType) {
371-371: Optional: Simplify redundant condition check.The check
realCtrl && e.ctrlKeyis redundant—ifrealCtrlis true,e.ctrlKeyshould also be true. You can simplify to justrealCtrlor juste.ctrlKey.- if (realCtrl && e.ctrlKey) { + if (realCtrl) {
548-554: Optional: Reduce duplication with onShiftScroll.The
onAltScrollmethod duplicates logic fromonShiftScroll(lines 540-546). You could extract a helper method to avoid repetition, though the current approach is clear and simple.Example refactor:
private handleScrollAttackRatio(event: WheelEvent, checkModifier: (e: WheelEvent) => boolean) { if (checkModifier(event)) { const scrollValue = event.deltaY === 0 ? event.deltaX : event.deltaY; const ratio = scrollValue > 0 ? -10 : 10; this.eventBus.emit(new AttackRatioEvent(ratio)); } } private onShiftScroll(event: WheelEvent) { this.handleScrollAttackRatio(event, (e) => e.shiftKey); } private onAltScroll(event: WheelEvent) { this.handleScrollAttackRatio(event, (e) => e.altKey); }CONTROL_FEATURES.md (1)
70-76: Optional: Add language identifiers to code blocks.The fenced code blocks at lines 70 and 79 are missing language specifiers. Add
textorplaintextfor proper rendering.Apply this diff:
-``` +```text While in game: 1. Hold Alt key 2. Scroll wheel up/down to adjust attack ratio 3. Release Alt to stop adjusting-
+text
While in game:
- Hold Ctrl key
- Press a number key (e.g., "1" for cities)
Also applies to: 79-86 </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between f161c94ff475092811fc49be79138fc37a446179 and a507424e131fe2605b7ad58ac32bdbee0ca65551. </details> <details> <summary>📒 Files selected for processing (3)</summary> * `CONTROL_FEATURES.md` (1 hunks) * `TEST_NEW_FEATURES.md` (1 hunks) * `src/client/InputHandler.ts` (7 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🪛 markdownlint-cli2 (0.18.1)</summary> <details> <summary>CONTROL_FEATURES.md</summary> 70-70: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 79-79: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> </details> <details> <summary>🔇 Additional comments (7)</summary><blockquote> <details> <summary>TEST_NEW_FEATURES.md (1)</summary><blockquote> `1-60`: **LGTM! Clear testing documentation.** The testing guide is well-structured and provides comprehensive step-by-step instructions for both features. The troubleshooting section and structure type mappings are helpful additions. </blockquote></details> <details> <summary>src/client/InputHandler.ts (5)</summary><blockquote> `208-215`: **LGTM! Proper integration of Alt scroll handler.** The `onAltScroll` call is correctly added to the wheel event listener alongside existing handlers, maintaining the established pattern. --- `280-286`: **LGTM! Browser shortcut prevention works correctly.** The implementation properly prevents browser tab switching when Ctrl+Number is pressed, ensuring the game controls take precedence. --- `323-324`: **LGTM! Consistent modifier key tracking.** Adding AltLeft and AltRight to the active keys tracking follows the same pattern as other modifier keys. --- `455-458`: **LGTM! Proper cleanup on Ctrl release.** Emitting `ToggleStructureEvent(null)` when Ctrl is released correctly restores all structure visibility. The placement before `activeKeys.delete` is appropriate. --- `530-538`: **LGTM! Correct exclusion of Alt from zoom behavior.** Adding `!event.altKey` to the condition properly prevents zoom when Alt+Scroll is used for attack ratio adjustment, avoiding conflicts between the two features. </blockquote></details> <details> <summary>CONTROL_FEATURES.md (1)</summary><blockquote> `1-103`: **LGTM! Comprehensive feature documentation.** The documentation clearly explains both features, their implementation, usage, and benefits. The technical details will help future maintainers understand the changes. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/client/InputHandler.ts (1)
367-395: Fix undefined variable and inconsistent condition.Two issues here:
Critical:
realCtrlis not declared in thekeyuphandler—it was only declared inkeydown(line 281). This will cause a ReferenceError at runtime.Major: Line 371 uses
realCtrl && e.ctrlKey, but the keydown handler (line 284) uses(realCtrl || e.ctrlKey). The AND condition is overly strict and could prevent the feature from working in edge cases.Apply this diff to declare
realCtrlin the keyup handler and fix the condition:// Ctrl + Number keys to show only specific structures - // (realCtrl already declared earlier in this handler) - - - if (realCtrl && e.ctrlKey) { + const realCtrl = + this.activeKeys.has("ControlLeft") || + this.activeKeys.has("ControlRight"); + if (realCtrl || e.ctrlKey) { const digitMatch = e.code.match(/^Digit(\d)$/);
🧹 Nitpick comments (1)
src/client/InputHandler.ts (1)
548-554: Consider extracting shared logic.The
onAltScrollmethod is correct and duplicates the proven logic fromonShiftScroll. This is acceptable, but you could optionally extract the shared scroll-to-ratio calculation into a helper method to reduce duplication.Optional refactor:
+ private calculateAttackRatioFromScroll(event: WheelEvent): number { + const scrollValue = event.deltaY === 0 ? event.deltaX : event.deltaY; + return scrollValue > 0 ? -10 : 10; + } + private onShiftScroll(event: WheelEvent) { if (event.shiftKey) { - const scrollValue = event.deltaY === 0 ? event.deltaX : event.deltaY; - const ratio = scrollValue > 0 ? -10 : 10; - this.eventBus.emit(new AttackRatioEvent(ratio)); + this.eventBus.emit(new AttackRatioEvent(this.calculateAttackRatioFromScroll(event))); } } private onAltScroll(event: WheelEvent) { if (event.altKey) { - const scrollValue = event.deltaY === 0 ? event.deltaX : event.deltaY; - const ratio = scrollValue > 0 ? -10 : 10; - this.eventBus.emit(new AttackRatioEvent(ratio)); + this.eventBus.emit(new AttackRatioEvent(this.calculateAttackRatioFromScroll(event))); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CONTROL_FEATURES.md(1 hunks)TEST_NEW_FEATURES.md(1 hunks)src/client/InputHandler.ts(7 hunks)
🧰 Additional context used
🪛 GitHub Actions: 🧪 CI
src/client/InputHandler.ts
[warning] 1-1: Code style issues found in file. Run Prettier with --write to fix.
TEST_NEW_FEATURES.md
[warning] 1-1: Code style issues found in file. Run Prettier with --write to fix.
🪛 markdownlint-cli2 (0.18.1)
CONTROL_FEATURES.md
70-70: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
79-79: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (6)
src/client/InputHandler.ts (5)
209-211: Double adjustment when Alt + Shift both held is intentional.When both Alt and Shift are held while scrolling, both
onShiftScrollandonAltScrollfire, resulting in a double AttackRatioEvent per tick (±20 instead of ±10). Based on past discussion, this appears to be the intended behavior for faster adjustments.
280-286: LGTM!The browser tab-switching prevention is correctly implemented using
realCtrl || e.ctrlKeyand properly intercepts all Ctrl+Digit combinations.
323-324: LGTM!Alt key tracking correctly added alongside other modifier keys.
455-458: LGTM!Ctrl release correctly emits
ToggleStructureEvent(null)to restore all structure visibility.
530-531: LGTM!The condition correctly excludes both Shift and Alt from normal zoom behavior, ensuring Alt+Scroll is dedicated to attack ratio control.
CONTROL_FEATURES.md (1)
100-101: LGTM!Mac compatibility note is accurate—the Ctrl+Number feature only checks
ControlLeft/ControlRight, so Mac users must use Control (not Command) for structure filtering.
|
This pull request is stale because it has been open for fourteen days with no activity. If you want to keep this pull request open, add a comment or update the branch. |
github-actions
bot
added
the
Stale
|
Could you fix the failing checks? |
github-actions
bot
removed
the
Stale
|
This pull request is stale because it has been open for fourteen days with no activity. If you want to keep this pull request open, add a comment or update the branch. |
github-actions
bot
added
the
Stale
github-actions
bot
removed
the
Stale
4 participants
Features Implemented
1. Alt + Scroll Wheel = Attack Ratio Control
Previous behaviour: Only Shift + Scroll wheel adjusted the attack ratio.
New behaviour: Now Alt + Scroll wheel also adjusts the attack ratio, giving players an alternative way to control their attack percentage.
This provides more flexibility for players who find Alt more convenient than Shift, or who want to use Alt for specific gameplay scenarios.
2. Ctrl + Number Keys = Structure Filtering
New feature: Hold Ctrl and press any number key to show only that specific structure type, similar to how hovering over structures works in the UI.
Key Mappings:
Ctrl + 1: Show only CitiesCtrl + 2: Show only FactoriesCtrl + 3: Show only PortsCtrl + 4: Show only Defence PostsCtrl + 5: Show only Missile SilosCtrl + 6: Show only SAM LaunchersCtrl + 7: Show only WarshipsCtrl + 8: Show only Atom BombsCtrl + 9: Show only Hydrogen BombsCtrl + 0: Show only MIRVsAlt + Scroll modifies attack ratio
Alt + Scroll doesn't interfere with normal zoom
Shift + Scroll still works for attack ratio (backward compatibility)
Ctrl + Number filters structures
Releasing Ctrl restores all structures
Number keys without Ctrl still build structures
Alt and Ctrl keys are tracked in activeKeys
No TypeScript compilation errors
DISCORD:
e2611f