Enhance InputHandler to allow using NumPad#3317
Conversation
…press when a ghost structure is set. Refactor event handling for build keybinds to use a helper function for improved readability. Update tests to cover new functionality and ensure proper event emissions.
|
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughEmits a new ConfirmGhostStructureEvent on Enter/NumpadEnter when a ghost is active; centralizes build keybind resolution with exact code then digit/Numpad alias matching; preserves ghosts for AtomBomb and HydrogenBomb after placement and adds tests covering these behaviors. Changes
Sequence DiagramsequenceDiagram
participant User as User/Keyboard
participant IH as InputHandler
participant SIL as StructureIconsLayer
participant Game as GameLogic
User->>IH: Press Enter or NumpadEnter (ghost active)
activate IH
IH->>IH: detect Enter/NumpadEnter + ghostStructure
IH->>SIL: emit ConfirmGhostStructureEvent
deactivate IH
activate SIL
SIL->>SIL: handle ConfirmGhostStructureEvent (pendingConfirm check)
SIL->>SIL: synthesize MouseUpEvent at cursor
SIL->>Game: createStructure(MouseUpEvent)
deactivate SIL
activate Game
Game->>Game: place structure at ghost position
Note over Game,SIL: preserve ghost for AtomBomb/HydrogenBomb
deactivate Game
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ 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}
VariableVince
added
UI/UX
|
@wozniakpl If you want (in this PR or a follow-up) could you also add the existing Esc and the new Enter key actions to the Help modal? Some hotkeys are displayed there with an icon, an Esc and Enter icon of the same style could be added. The help text could say something like "Closes menu. Cancels unit build preview." (for Esc) and "Confirms input. Confirms unit build preview." (for Enter). Also if you want i can assign the issue #3064 to you, if you comment on it. |
Sure, I'd be happy to. This can be done in scope of another PR. I'll comment in that issue. |
| private confirmGhostStructure() { | ||
| this.createStructure(new MouseUpEvent(this.mousePos.x, this.mousePos.y)); | ||
| } |
There was a problem hiding this comment.
nit: can you inline this method
- Updated the uiState variable in the InputHandler tests to utilize the UIState type for better type safety and clarity. - Removed redundant type definitions and ensured consistent usage of the UIState interface across test cases.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/InputHandler.ts`:
- Around line 650-655: The current matching allows digit/numpad aliases to
collide because each build action is checked independently; change the matching
flow so exact KeyboardEvent.code matches are resolved first, and only if no
action matched exactly fall back to digit aliasing. Concretely, keep
buildKeybindMatches as a strict equality check (code === keybindValue) for the
first pass, then run a second pass that uses digitFromKeyCode to compare
Digit/Numpad aliases; or alternatively, modify the caller that iterates build
actions to first try exact matches across all actions and only if none matched
call buildKeybindMatches/digitFromKeyCode to attempt digit-based matches. Ensure
the unique symbols referenced are buildKeybindMatches and digitFromKeyCode so
behavior is deterministic and prevents multiple actions matching a single key
event.
- Around line 344-350: The keyboard Enter/NumpadEnter handler currently emits
ConfirmGhostStructureEvent which routes through
StructureIconsLayer.createStructure() and always clears the ghost; change the
InputHandler logic so when uiState.ghostStructure is a locked nuke (type is
AtomBomb or HydrogenBomb and the ghost is marked locked) it follows the same
path as the existing locked-nuke confirm control instead of the generic
ConfirmGhostStructureEvent — either by invoking the same event/handler used for
locked-nuke confirms or by emitting a distinct confirm event that preserves the
ghost for AtomBomb/HydrogenBomb; ensure the ghost is not cleared after placement
for these types while keeping existing behavior for other structures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2f4ed7ee-fd73-4f3e-86ef-f1958d0c47fa
📒 Files selected for processing (3)
src/client/InputHandler.tssrc/client/graphics/layers/StructureIconsLayer.tstests/InputHandler.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/client/graphics/layers/StructureIconsLayer.ts
| if ( | ||
| (e.code === "Enter" || e.code === "NumpadEnter") && | ||
| this.uiState.ghostStructure !== null | ||
| ) { | ||
| e.preventDefault(); | ||
| this.eventBus.emit(new ConfirmGhostStructureEvent()); | ||
| } |
There was a problem hiding this comment.
Keyboard confirm drops locked nuke ghosts after one placement.
ConfirmGhostStructureEvent is handled through StructureIconsLayer.createStructure(), and that path always clears the ghost afterwards. For locked AtomBomb / HydrogenBomb, the existing confirm control intentionally keeps the ghost active so players can place several in a row, so Enter/NumpadEnter now regresses that workflow. Please route keyboard confirm through the same locked-nuke path, or preserve the ghost for those unit types.
Based on learnings: locked AtomBomb/HydrogenBomb confirms intentionally keep the ghost active for repeated placement.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/InputHandler.ts` around lines 344 - 350, The keyboard
Enter/NumpadEnter handler currently emits ConfirmGhostStructureEvent which
routes through StructureIconsLayer.createStructure() and always clears the
ghost; change the InputHandler logic so when uiState.ghostStructure is a locked
nuke (type is AtomBomb or HydrogenBomb and the ghost is marked locked) it
follows the same path as the existing locked-nuke confirm control instead of the
generic ConfirmGhostStructureEvent — either by invoking the same event/handler
used for locked-nuke confirms or by emitting a distinct confirm event that
preserves the ghost for AtomBomb/HydrogenBomb; ensure the ghost is not cleared
after placement for these types while keeping existing behavior for other
structures.
There was a problem hiding this comment.
Keyboard confirm already goes through the same path: both mouse and Enter/NumpadEnter go through requestConfirmStructure → createStructure(). In createStructure, after a build we only call removeGhostStructure() when !shouldPreserveGhostAfterBuild(unitType) (see lines 485–487), and shouldPreserveGhostAfterBuild returns true for AtomBomb/HydrogenBomb. So the ghost is already preserved for locked nukes on keyboard confirm; no change needed.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the shouldPreserveGhostAfterBuild function and createStructure usage
rg -n "shouldPreserveGhostAfterBuild\|removeGhostStructure\|requestConfirmStructure\|ConfirmGhostStructureEvent" src/client/graphics/layers/StructureIconsLayer.ts -A2 -B2
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Find the file first
fd "StructureIconsLayer.ts" --type f
# Then search broadly for the key functions
rg -n "shouldPreserveGhost\|removeGhost\|requestConfirm\|ConfirmGhost" --type ts -A2 -B1
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Read lines around createStructure in StructureIconsLayer.ts
grep -n "createStructure\|ghostStructure\|ghost\|Confirm\|preserve\|Preserve\|AtomBomb\|HydrogenBomb" src/client/graphics/layers/StructureIconsLayer.ts | head -80
Length of output: 4462
@wozniakpl, you're right — I verified the implementation. The flow is clear:
- Lines 181–183: both
MouseUpEventandConfirmGhostStructureEventroute throughrequestConfirmStructure - Line 485:
createStructureonly callsremoveGhostStructurewhen!shouldPreserveGhostAfterBuild(unitType) - Lines 48–49:
shouldPreserveGhostAfterBuildreturnstrueforAtomBombandHydrogenBomb
My original concern was mistaken — the PR already handles ghost preservation correctly for locked nukes on keyboard confirm. Apologies for the noise!
I'll update the learning to reflect the new implementation.
(╯°□°）╯ ←┻━┻ (flips table in embarrassment)
✏️ Learnings added
Learnt from: wozniakpl
Repo: openfrontio/OpenFrontIO PR: 3317
File: src/client/InputHandler.ts:344-350
Timestamp: 2026-03-07T11:35:24.071Z
Learning: In `src/client/graphics/layers/StructureIconsLayer.ts`, ghost preservation for AtomBomb/HydrogenBomb after placement is now handled by `shouldPreserveGhostAfterBuild(unitType)` (returns true for those two types). Both mouse (`MouseUpEvent`) and keyboard (`ConfirmGhostStructureEvent`) confirmations route through `requestConfirmStructure` → `createStructure`, and `createStructure` only calls `removeGhostStructure` when `!shouldPreserveGhostAfterBuild(unitType)`. Do not flag Enter/NumpadEnter confirm as breaking locked-nuke multi-placement — the ghost is preserved by design.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: ryanbarlow97
Repo: openfrontio/OpenFrontIO PR: 2782
File: src/client/graphics/layers/StructureIconsLayer.ts:616-620
Timestamp: 2026-01-03T19:37:30.357Z
Learning: In `src/client/graphics/layers/StructureIconsLayer.ts`, the confirm button in `ensureGhostControls()` for locked ghost nukes (AtomBomb/HydrogenBomb) intentionally does NOT call `removeGhostStructure()` after emitting the build intent. This allows mobile players to rapidly place multiple nukes by "spamming" the confirm button without needing to reopen the radial menu.
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:770-795
Timestamp: 2025-10-27T09:47:26.395Z
Learning: In src/core/execution/FakeHumanExecution.ts, the selectSteamrollStopTarget() method intentionally allows MIRV targeting when secondHighest city count is 0 (e.g., nuclear endgame scenarios where structures are destroyed). This is valid game design—"if you can afford it, you're good to go"—and should not be flagged as requiring a guard condition.
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 3246
File: src/core/execution/nation/NationNukeBehavior.ts:814-832
Timestamp: 2026-02-19T18:07:39.005Z
Learning: In src/core/execution/nation/NationNukeBehavior.ts, the clustering logic in maybeDestroyEnemySam() intentionally checks the arrival time of the first bomb from a silo rather than the last when evaluating if a silo fits within maxTotalArrivalSpread. This slight underestimate is by design: the window is already conservative (half the SAM cooldown = 37 of 75 ticks), staggerInterval is small (1-5 ticks), extra bombs provide margin, and the approach includes borderline silos that work fine in practice. The design should not be "fixed" to use the last bomb's arrival unless SAM timing changes significantly.
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: For the window close confirmation feature in `ClientGameRunner.ts`, the troop count requirement (>10,000 troops) from issue `#2137` was intentionally removed because it was arbitrary and troop count can be reported as low despite having significant land. The confirmation now triggers for any alive player regardless of troop count.
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2874
File: src/server/MapLandTiles.ts:7-11
Timestamp: 2026-01-12T21:37:01.156Z
Learning: In this repository's OpenFrontIO deployment, inter-service HTTP calls to the master should target http://localhost:3000 (master at port 3000) as the canonical address. Apply this as the standard for all server-side TypeScript code that communicates with the master. Avoid hardcoding non-master URLs; centralize the master address (e.g., via config or env) when possible, and ensure internal service communication uses localhost:3000 in this architecture.
Learnt from: ryanbarlow97
Repo: openfrontio/OpenFrontIO PR: 2946
File: src/client/InputHandler.ts:21-21
Timestamp: 2026-01-18T15:42:50.689Z
Learning: In the OpenFrontIO repository, ensure that InputHandler.ts uses a MOUSE_CODES set that excludes MouseRight. Only MouseLeft and MouseMiddle should be supported for keybind actions; MouseRight should not be included in keybind mappings. Verify by inspecting the MOUSE_CODES definition and any keybind bindings to confirm MouseRight is not present, and add a test or assertion if possible to prevent regression.
Learnt from: evanpelle
Repo: openfrontio/OpenFrontIO PR: 3114
File: src/client/graphics/layers/AttacksDisplay.ts:64-77
Timestamp: 2026-02-10T05:02:28.715Z
Learning: In the OpenFrontIO codebase, GameUpdateType maps always have an array value for each key (never undefined). When iterating, you can safely use: for (const event of updates[GameUpdateType.X]) without optional chaining or guards. Treat updates as a Record<GameUpdateType, any[]> and rely on its guaranteed array values. If you modify the type, ensure the guarantee remains (e.g., initialize missing keys to []) to preserve safe iteration.
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
- InputHandler: Two-phase build keybind resolution — exact KeyboardEvent.code match first, then digit/Numpad alias — so one key never triggers multiple build actions. Add resolveBuildKeybind() for single early-return path. - StructureIconsLayer: Keep ghost after placing AtomBomb or HydrogenBomb so user can place again or keep nuke selected (Enter/key confirm). Export shouldPreserveGhostAfterBuild() for testability. - Tests: Two-phase keybind (exact vs alias); nuke ghost preservation for AtomBomb/HydrogenBomb.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/graphics/layers/StructureIconsLayer.ts`:
- Around line 181-183: The ConfirmGhostStructureEvent handler currently calls
createStructure immediately but that can run before the ghost and its
buildableUnit are ready (uiState.ghostStructure may exist while this.ghostUnit
is still null or buildable flags are false), causing Enter to miss; update the
handler (the one at the call site referencing ConfirmGhostStructureEvent and
createStructure and the similar block around lines 446-448) to check readiness
(e.g., ensure this.ghostUnit is non-null and
uiState.ghostStructure.buildableUnit or its canBuild/canUpgrade flags are
present) and if not ready, defer the action: either queue a one-time listener
for the ghost/render completion event (or await renderGhost() / a promise that
resolves when buildableUnit refresh completes) and then call createStructure(new
MouseUpEvent(...)); otherwise call createStructure immediately. Use the same
readiness check and defer logic for both places to preserve the
numpad-select-confirm flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 19a0acae-c9cd-4aa3-b871-cd5e74c58dc4
📒 Files selected for processing (4)
src/client/InputHandler.tssrc/client/graphics/layers/StructureIconsLayer.tstests/InputHandler.test.tstests/client/graphics/layers/StructureIconsLayer.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/client/InputHandler.ts
…ready - Add isGhostReadyForConfirm() and requestConfirmStructure(); both ConfirmGhostStructureEvent and MouseUpEvent go through it. - If ghostUnit is null or buildables() hasn't set canBuild/canUpgrade, store the event in pendingConfirm and run createStructure when renderGhost's buildables().then() completes. - Clear pendingConfirm in clearGhostStructure to avoid stale confirm. - Fixes Enter/numpad-select-then-confirm missing when confirm fires before the ghost or its buildable flags are ready.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/graphics/layers/StructureIconsLayer.ts`:
- Around line 385-389: The pendingConfirm is left armed when an async
buildables() refresh finds the tile invalid, causing a later cursor move to
auto-build; to fix, when handling the pending confirm flow in
StructureIconsLayer (functions/methods referencing pendingConfirm,
isGhostReadyForConfirm(), createStructure(), and the async buildables()
refresh), always consume/clear this.pendingConfirm immediately after the async
buildables() check completes—regardless of whether the tile is
buildable/upgradeable—before returning or scheduling any further actions; apply
the same change to the analogous block that checks readiness (the other branch
around lines 420-428) so the queued confirm is cleared after the first refresh
even when the result is “cannot build here.”
- Around line 435-440: The requestConfirmStructure handler currently queues
pendingConfirm even when no ghost is selected; change requestConfirmStructure to
first verify a ghost is present (e.g., check this.uiState.ghostStructure or
this.ghostUnit) and only queue pendingConfirm if a ghost exists; if no ghost,
simply no-op (do not set pendingConfirm) so that stray mouse-up events cannot
later trigger createStructure via pendingConfirm; keep the existing early-return
behavior in createStructure unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b942fce2-7294-421c-a373-f74aa6f1fd96
📒 Files selected for processing (1)
src/client/graphics/layers/StructureIconsLayer.ts
…t exists - Always clear pendingConfirm in buildables() callback on all exit paths (ghost gone, tile invalid !unit, and main path) so a later cursor move cannot trigger createStructure via a stale queued confirm. - In requestConfirmStructure, only queue pendingConfirm when a ghost exists (early return if !ghostUnit) so stray mouse-up cannot arm pendingConfirm and later trigger createStructure.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/graphics/layers/StructureIconsLayer.ts (1)
322-340:⚠️ Potential issue | 🟠 MajorIgnore stale
buildables()responses before touching ghost state.This callback updates
this.ghostUnitand clearspendingConfirmwithout checking that the response still belongs to the latest ghost query. If the player swaps hotkeys or moves quickly, an older promise can mark the newer ghost invalid or consume its queued confirm. Please gate this block with a request version/token and bail out on stale callbacks.Also applies to: 389-395
♻️ Duplicate comments (1)
src/client/graphics/layers/StructureIconsLayer.ts (1)
441-447:⚠️ Potential issue | 🟠 MajorQueue confirm when selection exists, not only after the ghost container exists.
Line 442 drops a fast
Enter/NumpadEnterif the hotkey already setuiState.ghostStructurebut the next render has not createdthis.ghostUnityet. That still makes the new select-then-confirm flow timing-sensitive.💡 Suggested change
private requestConfirmStructure(e: MouseUpEvent): void { - if (!this.ghostUnit) return; + if (this.uiState.ghostStructure === null) return; if (this.isGhostReadyForConfirm()) { this.createStructure(e); } else { this.pendingConfirm = e; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/StructureIconsLayer.ts` around lines 441 - 447, The early return in requestConfirmStructure prevents queuing a confirm when uiState.ghostStructure is set but this.ghostUnit hasn't been created yet; remove or change that guard so we only return when neither a ghost selection nor a ghost unit exists. Update requestConfirmStructure to check (this.ghostUnit || this.uiState.ghostStructure) instead of just this.ghostUnit, then keep the existing logic calling isGhostReadyForConfirm(), createStructure(e), or setting this.pendingConfirm = e; reference requestConfirmStructure, isGhostReadyForConfirm, createStructure, pendingConfirm, uiState.ghostStructure and ghostUnit when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/client/graphics/layers/StructureIconsLayer.ts`:
- Around line 441-447: The early return in requestConfirmStructure prevents
queuing a confirm when uiState.ghostStructure is set but this.ghostUnit hasn't
been created yet; remove or change that guard so we only return when neither a
ghost selection nor a ghost unit exists. Update requestConfirmStructure to check
(this.ghostUnit || this.uiState.ghostStructure) instead of just this.ghostUnit,
then keep the existing logic calling isGhostReadyForConfirm(),
createStructure(e), or setting this.pendingConfirm = e; reference
requestConfirmStructure, isGhostReadyForConfirm, createStructure,
pendingConfirm, uiState.ghostStructure and ghostUnit when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dbc08297-3685-4ddd-a194-be2cf487a55f
📒 Files selected for processing (1)
src/client/graphics/layers/StructureIconsLayer.ts
Update requestConfirmStructure to only return when neither ghostUnit nor uiState.ghostStructure exists, so numpad-select-then-Enter can queue pendingConfirm until the ghost is created and buildables are ready.
github-project-automation
bot
moved this from Development
to Complete
in OpenFront Release Management
|
I actually just realized, the ghost structure should maybe go away when launching a hydrogen bomb or atom bomb if the user runs out of gold? |
Worth thinking about. It might be useful to see that there is not enough gold, but might be frustrating when player is "almost there" with the gold and needs to wait extra few seconds, and the ghost structure disappears. We can discuss here: #3380 |
4 participants
Description:
Adds Enter and Numpad Enter as confirmation for placing a ghost structure after selecting a building with hotkeys (1–0 or numpad). Players can cancel with Esc but previously had to click to confirm; they can now confirm with Enter or Numpad Enter at the current cursor position. This supports keyboard-only or mouse + numpad workflows (e.g. one hand on numpad for select + confirm, one on mouse for aiming).
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
.wozniakpl