Bomb Confirmation - mobile (better version) #2783
Conversation
ryanbarlow97
added
UI/UX
WalkthroughAdds a locked-ghost flow for nuke placement: UIState gains Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant RadialMenu as RadialMenuElements
participant StructureLayer as StructureIconsLayer
participant Trajectory as NukeTrajectoryPreviewLayer
participant UIState
participant EventBus
User->>RadialMenu: Select AtomBomb / HydrogenBomb
RadialMenu->>UIState: Set ghostStructure, set lockedGhostTile
RadialMenu->>EventBus: Emit GhostStructureChangedEvent
RadialMenu-->>User: Close menu
User->>StructureLayer: Move mouse / hover
StructureLayer->>Trajectory: Request preview update
Trajectory->>UIState: Check lockedGhostTile
alt lockedGhostTile present
Trajectory->>Trajectory: Use locked tile as target (calc path)
else lockedGhostTile absent
Trajectory->>Trajectory: Convert mouse->world, validate coords
end
Trajectory->>StructureLayer: Return points or clear preview
User->>StructureLayer: Click tile to lock
StructureLayer->>UIState: Set lockedGhostTile
StructureLayer->>StructureLayer: Render ghost controls (OK/Flip/X)
alt Confirm
User->>StructureLayer: Click OK
StructureLayer->>EventBus: Emit build intent / commit
StructureLayer->>UIState: Clear lockedGhostTile & ghostStructure
StructureLayer->>StructureLayer: Destroy controls
else Flip
User->>StructureLayer: Click Flip
StructureLayer->>EventBus: Emit SwapRocketDirectionEvent
StructureLayer->>StructureLayer: Update visuals
else Cancel
User->>StructureLayer: Click X
StructureLayer->>UIState: Clear lockedGhostTile & ghostStructure
StructureLayer->>StructureLayer: Destroy controls
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 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: 0
🧹 Nitpick comments (2)
src/client/graphics/layers/NukeTrajectoryPreviewLayer.ts (1)
207-238: Duplicated target resolution logic.The target resolution code in
updateTrajectoryPath()(lines 207-238) is nearly identical to the code inupdateTrajectoryPreview()(lines 107-136). Both checklockedGhostTile, fall back to mouse-derived coords, validate, and get a tile ref.Consider extracting this into a small helper method to reduce duplication.
🔎 Suggested helper extraction
+ private resolveTargetTile(): TileRef | null { + if (this.uiState.lockedGhostTile) { + return this.uiState.lockedGhostTile; + } + const rect = this.transformHandler.boundingRect(); + if (!rect) return null; + + const localX = this.mousePos.x - rect.left; + const localY = this.mousePos.y - rect.top; + const worldCoords = this.transformHandler.screenToWorldCoordinates( + localX, + localY, + ); + + if (!this.game.isValidCoord(worldCoords.x, worldCoords.y)) { + return null; + } + return this.game.ref(worldCoords.x, worldCoords.y); + }Then replace both duplicated blocks with:
const targetTile = this.resolveTargetTile(); if (!targetTile) { this.trajectoryPoints = []; // ...clear state... return; }src/client/graphics/layers/StructureIconsLayer.ts (1)
637-645:hideGhostControlsis just an alias fordestroyGhostControls.Currently these two methods are identical. If they stay that way, consider removing one to reduce confusion. If you plan to add "hide without destroy" behavior later (e.g.,
display: none), then keeping them separate makes sense.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/client/graphics/GameRenderer.tssrc/client/graphics/UIState.tssrc/client/graphics/layers/NukeTrajectoryPreviewLayer.tssrc/client/graphics/layers/RadialMenuElements.tssrc/client/graphics/layers/StructureIconsLayer.tstests/InputHandler.test.ts
🧰 Additional context used
🧠 Learnings (12)
📓 Common learnings
Learnt from: ryanbarlow97
Repo: openfrontio/OpenFrontIO PR: 2782
File: src/client/graphics/layers/StructureIconsLayer.ts:616-620
Timestamp: 2026-01-03T19:37:22.111Z
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.
📚 Learning: 2025-08-24T11:13:08.495Z
Learnt from: DevelopingTom
Repo: openfrontio/OpenFrontIO PR: 1900
File: src/core/execution/SAMLauncherExecution.ts:103-111
Timestamp: 2025-08-24T11:13:08.495Z
Learning: In SAMLauncherExecution.ts, the cached target bug can only occur if: 1) SAM is not on cooldown when nuke is in range, 2) SAM goes on cooldown right after computing trajectory, 3) SAM becomes available again before nuke explodes. This is not possible with current cooldown values but the fix is still valuable for robustness.
Applied to files:
src/client/graphics/layers/NukeTrajectoryPreviewLayer.ts
📚 Learning: 2025-06-22T21:51:14.990Z
Learnt from: devalnor
Repo: openfrontio/OpenFrontIO PR: 1248
File: src/client/graphics/layers/TerritoryInfoLayer.ts:20-20
Timestamp: 2025-06-22T21:51:14.990Z
Learning: In TerritoryInfoLayer.ts, the highlightedTerritory field uses both null and undefined intentionally: undefined represents initial state or inactive layer (Ctrl released), while null represents active layer with no territory being highlighted at cursor position. This distinction is important for proper state change detection.
Applied to files:
src/client/graphics/layers/NukeTrajectoryPreviewLayer.tssrc/client/graphics/GameRenderer.tssrc/client/graphics/UIState.ts
📚 Learning: 2025-10-18T11:00:57.142Z
Learnt from: NewYearNewPhil
Repo: openfrontio/OpenFrontIO PR: 2230
File: src/client/graphics/GameRenderer.ts:269-277
Timestamp: 2025-10-18T11:00:57.142Z
Learning: In src/client/graphics/GameRenderer.ts, the GameRecapCapture implementation does not use setCaptureRenderEnabled on layers. Instead, it uses RecapCaptureSurface.capture() to render capture layers (TerrainLayer, TerritoryLayer, RailroadLayer, StructureIconsLayer, UnitLayer) directly to an off-screen canvas without requiring layer-level capture mode methods.
Applied to files:
src/client/graphics/layers/NukeTrajectoryPreviewLayer.tssrc/client/graphics/layers/StructureIconsLayer.ts
📚 Learning: 2026-01-03T19:37:22.111Z
Learnt from: ryanbarlow97
Repo: openfrontio/OpenFrontIO PR: 2782
File: src/client/graphics/layers/StructureIconsLayer.ts:616-620
Timestamp: 2026-01-03T19:37:22.111Z
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.
Applied to files:
src/client/graphics/layers/NukeTrajectoryPreviewLayer.tssrc/client/graphics/layers/RadialMenuElements.tssrc/client/graphics/GameRenderer.tssrc/client/graphics/UIState.ts
📚 Learning: 2025-06-16T03:03:59.778Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1192
File: src/client/graphics/layers/RadialMenuElements.ts:312-314
Timestamp: 2025-06-16T03:03:59.778Z
Learning: In RadialMenuElements.ts, when fixing undefined params errors in subMenu functions, use explicit checks like `if (params === undefined || params.selected === null)` rather than optional chaining, as it makes the intent clearer and matches the specific error scenarios where params can be undefined during spawn phase operations.
Applied to files:
src/client/graphics/layers/RadialMenuElements.ts
📚 Learning: 2025-05-18T23:36:12.847Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 784
File: src/core/game/StatsImpl.ts:143-159
Timestamp: 2025-05-18T23:36:12.847Z
Learning: In this codebase, NukeType is a union type derived from UnitType values (specifically bomb-related values like AtomBomb, HydrogenBomb, MIRV, and MIRVWarhead) rather than a separate enum. This means comparing NukeType values against UnitType values in switch statements is valid and intentional.
Applied to files:
src/client/graphics/layers/RadialMenuElements.ts
📚 Learning: 2025-10-27T09:47:26.395Z
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.
Applied to files:
src/client/graphics/layers/RadialMenuElements.ts
📚 Learning: 2025-05-18T23:36:12.847Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 784
File: src/core/game/StatsImpl.ts:143-159
Timestamp: 2025-05-18T23:36:12.847Z
Learning: In this codebase, NukeType is a union type derived from specific UnitType enum values (AtomBomb, HydrogenBomb, MIRV, MIRVWarhead). This means that comparisons in switch statements between a NukeType parameter and UnitType enum values are valid and will work correctly at runtime.
Applied to files:
src/client/graphics/layers/RadialMenuElements.ts
📚 Learning: 2025-11-26T20:49:29.140Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2519
File: src/core/game/GameView.ts:516-525
Timestamp: 2025-11-26T20:49:29.140Z
Learning: In GameView.ts, when usesSharedTileState is true (SAB mode), packedTileUpdates contains unpacked tile references as BigInt(tileRef) only, because all tile state lives in the shared Uint16Array. In non-SAB mode, packedTileUpdates contains packed TileUpdate bigints in the format (tileRef << 16n | state), which must be decoded via updateTile(tu). Therefore, Number(tu) is correct in SAB mode and shifting right by 16 bits would be wrong.
Applied to files:
src/client/graphics/layers/RadialMenuElements.tssrc/client/graphics/GameRenderer.tssrc/client/graphics/UIState.ts
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/client/graphics/GameRenderer.tssrc/client/graphics/UIState.ts
📚 Learning: 2026-01-03T19:37:22.111Z
Learnt from: ryanbarlow97
Repo: openfrontio/OpenFrontIO PR: 2782
File: src/client/graphics/layers/StructureIconsLayer.ts:616-620
Timestamp: 2026-01-03T19:37:22.111Z
Learning: In StructureIconsLayer.ts, within ensureGhostControls(), when handling locked ghost nukes (AtomBomb/HydrogenBomb), do not call removeGhostStructure() after emitting the build intent. This enables rapid repeated placement by mobile players. Reviewers should verify that the confirm button path intentionally bypasses ghost removal in this scenario and that this design aligns with UX goals and does not introduce inconsistent state.
Applied to files:
src/client/graphics/layers/StructureIconsLayer.ts
🧬 Code graph analysis (3)
src/client/graphics/layers/NukeTrajectoryPreviewLayer.ts (2)
src/core/game/GameView.ts (1)
targetTile(134-136)src/core/game/GameMap.ts (1)
TileRef(3-3)
src/client/graphics/layers/RadialMenuElements.ts (3)
src/core/game/PlayerImpl.ts (1)
canBuild(964-1010)src/client/graphics/layers/UnitDisplay.ts (1)
canBuild(80-99)src/client/InputHandler.ts (1)
GhostStructureChangedEvent(88-90)
src/client/graphics/UIState.ts (1)
src/core/game/GameMap.ts (1)
TileRef(3-3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Deploy to openfront.dev
🔇 Additional comments (11)
src/client/graphics/GameRenderer.ts (1)
53-58: LGTM!Clean initialization of the new
lockedGhostTileproperty. The initialnullvalue matches the type definition and signals no locked ghost on startup.src/client/graphics/UIState.ts (1)
1-9: LGTM!Simple interface extension with proper typing. Using
TileRef | nullis clear:nullmeans unlocked, a validTileRefmeans the ghost is locked to that tile.tests/InputHandler.test.ts (1)
36-45: LGTM!Test correctly updated to include the new
lockedGhostTilefield. This keeps the test in sync with theUIStateinterface changes.src/client/graphics/layers/RadialMenuElements.ts (1)
416-436: Good addition for the nuke ghost-lock flow.The logic correctly identifies AtomBomb/HydrogenBomb selections and redirects them through the ghost placement flow instead of the immediate build path. The
params.uiStatecheck on line 423 provides proper null safety.One small note: the
uiStateproperty inMenuElementParams(line 42) is optional (uiState?: UIState). If a caller ever omits it for a nuke selection, the condition silently falls through to the standard path. This seems acceptable since the nuke flow is only available whenuiStateis provided.src/client/graphics/layers/NukeTrajectoryPreviewLayer.ts (1)
107-136: Target resolution logic looks correct.Good validation: when deriving target from mouse, invalid coords clear state and return early. When locked, the locked tile is used directly.
src/client/graphics/layers/StructureIconsLayer.ts (6)
88-93: Good state structure for ghost controls.Clean typed object for the DOM-based control buttons. Using separate named fields (
confirm,cancel,flip) is clearer than an array.
257-284: Smooth locked ghost repositioning.Converting world coordinates to screen coordinates every frame keeps the ghost visually stable during panning. The
updateGhostControlscall ensures buttons follow the ghost.
409-436: Click re-targeting logic for locked ghost.When a locked ghost is active, clicks now update the target location instead of committing. This lets players adjust placement before confirming. The
GhostStructureChangedEventemission forces trajectory recalculation.
581-635: Ghost control buttons implementation.The DOM-based approach with inline styles works, though it couples presentation tightly to logic. A few observations:
- z-index "5" (line 590) is low. If other UI elements have higher z-index, controls may be hidden.
- Attaching to
document.body(line 632) works but means controls exist outside the game's normal DOM hierarchy.These are fine for now but worth noting if z-index conflicts arise later.
Could you confirm the z-index value of "5" is sufficient relative to other game UI elements? If there are overlays or modals, they may need to be checked.
616-620: Confirm button intentionally keeps ghost active.Per the retrieved learning, the confirm button does NOT call
removeGhostStructure()afteremitBuildIntent(). This enables mobile players to rapidly place multiple nukes without reopening the radial menu. The design is intentional.
523-541:emitBuildIntentcorrectly handles rocket direction.For AtomBomb and HydrogenBomb, the
rocketDirectionUpvalue from UI state is passed to the build event. For other unit types, it'sundefined. Clean conditional logic.
ryanbarlow97
changed the title
ryanbarlow97
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/client/graphics/layers/StructureIconsLayer.ts (2)
88-93: Consider extracting the ghost controls type.The inline object type for
ghostControlsworks but could be clearer as a named type alias. This would improve documentation and maintainability.🔎 Proposed refactor
Above the class definition, add:
type GhostControls = { container: HTMLDivElement; confirm: HTMLButtonElement; cancel: HTMLButtonElement; flip: HTMLButtonElement; };Then simplify the field:
- private ghostControls: { - container: HTMLDivElement; - confirm: HTMLButtonElement; - cancel: HTMLButtonElement; - flip: HTMLButtonElement; - } | null = null; + private ghostControls: GhostControls | null = null;
643-645: hideGhostControls name is misleading.The method name suggests temporary hiding, but it actually destroys the controls. Consider either renaming to match the behavior or making it a simple inline call.
🔎 Proposed refactor
Option 1: Remove the wrapper and call
destroyGhostControls()directly at line 276 and 653.Option 2: Rename to clarify:
- private hideGhostControls() { - this.destroyGhostControls(); - } + private ensureGhostControlsDestroyed() { + this.destroyGhostControls(); + }Then update call sites.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/client/graphics/layers/StructureIconsLayer.ts
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: ryanbarlow97
Repo: openfrontio/OpenFrontIO PR: 2782
File: src/client/graphics/layers/StructureIconsLayer.ts:616-620
Timestamp: 2026-01-03T19:37:22.111Z
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.
📚 Learning: 2026-01-03T19:37:22.111Z
Learnt from: ryanbarlow97
Repo: openfrontio/OpenFrontIO PR: 2782
File: src/client/graphics/layers/StructureIconsLayer.ts:616-620
Timestamp: 2026-01-03T19:37:22.111Z
Learning: In StructureIconsLayer.ts, within ensureGhostControls(), when handling locked ghost nukes (AtomBomb/HydrogenBomb), do not call removeGhostStructure() after emitting the build intent. This enables rapid repeated placement by mobile players. Reviewers should verify that the confirm button path intentionally bypasses ghost removal in this scenario and that this design aligns with UX goals and does not introduce inconsistent state.
Applied to files:
src/client/graphics/layers/StructureIconsLayer.ts
📚 Learning: 2025-10-18T11:00:57.142Z
Learnt from: NewYearNewPhil
Repo: openfrontio/OpenFrontIO PR: 2230
File: src/client/graphics/GameRenderer.ts:269-277
Timestamp: 2025-10-18T11:00:57.142Z
Learning: In src/client/graphics/GameRenderer.ts, the GameRecapCapture implementation does not use setCaptureRenderEnabled on layers. Instead, it uses RecapCaptureSurface.capture() to render capture layers (TerrainLayer, TerritoryLayer, RailroadLayer, StructureIconsLayer, UnitLayer) directly to an off-screen canvas without requiring layer-level capture mode methods.
Applied to files:
src/client/graphics/layers/StructureIconsLayer.ts
🧬 Code graph analysis (1)
src/client/graphics/layers/StructureIconsLayer.ts (5)
src/core/game/GameMap.ts (4)
TileRef(3-3)x(127-129)y(131-133)magnitude(172-174)src/core/game/UnitImpl.ts (2)
tile(178-180)type(148-150)src/client/InputHandler.ts (2)
GhostStructureChangedEvent(88-90)SwapRocketDirectionEvent(92-94)src/client/Transport.ts (2)
SendUpgradeStructureIntentEvent(51-56)BuildUnitIntentEvent(91-97)src/core/game/GameImpl.ts (3)
x(837-839)y(840-842)magnitude(870-872)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Deploy to openfront.dev
🔇 Additional comments (4)
src/client/graphics/layers/StructureIconsLayer.ts (4)
261-284: Locked ghost repositioning looks correct.The logic correctly branches between locked and unlocked ghost states. Locked ghosts are repositioned every frame for smooth panning, which aligns with the PR objective for mobile UX. The throttling below at line 287-291 prevents expensive queries from running too frequently.
409-437: Re-targeting logic for locked ghosts is well-designed.When a locked ghost is active, mouse clicks update the target location instead of building. This matches the PR objective for mobile-friendly tap-to-reposition behavior. The helper method decomposition improves readability.
523-579: Helper methods improve code clarity.The extracted helper methods make the code more readable and maintainable.
isLockableGhosthardcodes the two nuke types, which is acceptable given the limited set of lockable units.
647-672: Control positioning logic is well-designed.The method correctly positions controls just below the nuke radius and scales them appropriately based on zoom level. The guard conditions ensure controls only exist when a locked ghost is active.
| const confirm = makeButton("OK", "#2e7d32", () => { | ||
| if (this.uiState.lockedGhostTile) { | ||
| this.emitBuildIntent(this.uiState.lockedGhostTile); | ||
| } | ||
| }); | ||
|
|
||
| const flip = makeButton("Flip", "#1565c0", () => { | ||
| const next = !this.uiState.rocketDirectionUp; | ||
| this.eventBus.emit(new SwapRocketDirectionEvent(next)); | ||
| }); | ||
|
|
||
| const cancel = makeButton("X", "#b71c1c", () => | ||
| this.removeGhostStructure(), | ||
| ); |
There was a problem hiding this comment.
Text labels must use translateText().
The button labels "OK", "Flip", and "X" are hardcoded, but the PR description states "All UI text routed through translateText() and added to en.json." This breaks internationalization.
🔎 Proposed fix
Assuming translateText is available (import may be needed):
- const confirm = makeButton("OK", "#2e7d32", () => {
+ const confirm = makeButton(translateText("ghost_controls_confirm"), "#2e7d32", () => {
if (this.uiState.lockedGhostTile) {
this.emitBuildIntent(this.uiState.lockedGhostTile);
}
});
- const flip = makeButton("Flip", "#1565c0", () => {
+ const flip = makeButton(translateText("ghost_controls_flip"), "#1565c0", () => {
const next = !this.uiState.rocketDirectionUp;
this.eventBus.emit(new SwapRocketDirectionEvent(next));
});
- const cancel = makeButton("X", "#b71c1c", () =>
+ const cancel = makeButton(translateText("ghost_controls_cancel"), "#b71c1c", () =>
this.removeGhostStructure(),
);Also verify these keys exist in en.json:
ghost_controls_confirmghost_controls_flipghost_controls_cancel
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/client/graphics/layers/StructureIconsLayer.ts around lines 616 to 629,
the button labels are hardcoded ("OK", "Flip", "X") which bypasses localization;
replace those string literals with calls to translateText() using keys like
ghost_controls_confirm, ghost_controls_flip, and ghost_controls_cancel (and add
an import for translateText if missing), update en.json to include those keys
with appropriate values, and ensure the makeButton calls pass the translated
text instead of the hardcoded strings.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/client/graphics/layers/StructureIconsLayer.ts (1)
638-651: Button labels need translateText() for i18n.The strings "OK", "Flip", and "X" are hardcoded. The PR description states all UI text should go through
translateText().This was already flagged in a previous review. Please ensure keys like
ghost_controls_confirm,ghost_controls_flip, andghost_controls_cancelare added toen.jsonand used here.
🧹 Nitpick comments (3)
src/client/graphics/layers/StructureIconsLayer.ts (3)
561-579: Consider extracting shared tile resolution logic.
getTileFromContextMenuEventandgetTileFromMouseEventare nearly identical. You could unify them with a single helper that accepts a{x: number, y: number}object, reducing duplication.🔎 Proposed refactor
- private getTileFromContextMenuEvent(e: ContextMenuEvent): TileRef | null { - const rect = this.transformHandler.boundingRect(); - if (!rect) return null; - const x = e.x - rect.left; - const y = e.y - rect.top; - const tile = this.transformHandler.screenToWorldCoordinates(x, y); - if (!this.game.isValidCoord(tile.x, tile.y)) return null; - return this.game.ref(tile.x, tile.y); - } - - private getTileFromMouseEvent(e: MouseUpEvent): TileRef | null { - const rect = this.transformHandler.boundingRect(); - if (!rect) return null; - const x = e.x - rect.left; - const y = e.y - rect.top; - const tile = this.transformHandler.screenToWorldCoordinates(x, y); - if (!this.game.isValidCoord(tile.x, tile.y)) return null; - return this.game.ref(tile.x, tile.y); - } + private getTileFromScreenPosition(pos: { x: number; y: number }): TileRef | null { + const rect = this.transformHandler.boundingRect(); + if (!rect) return null; + const x = pos.x - rect.left; + const y = pos.y - rect.top; + const tile = this.transformHandler.screenToWorldCoordinates(x, y); + if (!this.game.isValidCoord(tile.x, tile.y)) return null; + return this.game.ref(tile.x, tile.y); + }Then update callers to pass
edirectly (both event types have{x, y}properties).
665-667:hideGhostControlsis just an alias fordestroyGhostControls.Currently both do the same thing. If you intended to support hiding without destroying (e.g.,
display: none), this could be extended later. Otherwise, you could inline the call todestroyGhostControlsdirectly wherehideGhostControlsis used.
687-693: Consider naming the scale constants.The magic numbers
0.75,1.4, and the divisor2in the scale calculation would be clearer as named constants (e.g.,MIN_CONTROLS_SCALE,MAX_CONTROLS_SCALE).🔎 Example
private static readonly MIN_CONTROLS_SCALE = 0.75; private static readonly MAX_CONTROLS_SCALE = 1.4; private static readonly CONTROLS_SCALE_DIVISOR = 2; // Then in updateGhostControls: const scale = Math.max( StructureIconsLayer.MIN_CONTROLS_SCALE, Math.min( StructureIconsLayer.MAX_CONTROLS_SCALE, this.transformHandler.scale / StructureIconsLayer.CONTROLS_SCALE_DIVISOR, ), );
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/client/graphics/layers/StructureIconsLayer.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ryanbarlow97
Repo: openfrontio/OpenFrontIO PR: 2782
File: src/client/graphics/layers/StructureIconsLayer.ts:616-620
Timestamp: 2026-01-03T19:37:22.111Z
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.
📚 Learning: 2026-01-03T19:37:22.111Z
Learnt from: ryanbarlow97
Repo: openfrontio/OpenFrontIO PR: 2782
File: src/client/graphics/layers/StructureIconsLayer.ts:616-620
Timestamp: 2026-01-03T19:37:22.111Z
Learning: In StructureIconsLayer.ts, within ensureGhostControls(), when handling locked ghost nukes (AtomBomb/HydrogenBomb), do not call removeGhostStructure() after emitting the build intent. This enables rapid repeated placement by mobile players. Reviewers should verify that the confirm button path intentionally bypasses ghost removal in this scenario and that this design aligns with UX goals and does not introduce inconsistent state.
Applied to files:
src/client/graphics/layers/StructureIconsLayer.ts
🧬 Code graph analysis (1)
src/client/graphics/layers/StructureIconsLayer.ts (2)
src/client/InputHandler.ts (5)
ContextMenuEvent(52-57)GhostStructureChangedEvent(88-90)MouseMoveEvent(45-50)MouseUpEvent(8-13)SwapRocketDirectionEvent(92-94)src/client/Transport.ts (2)
SendUpgradeStructureIntentEvent(51-56)BuildUnitIntentEvent(91-97)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Deploy to openfront.dev
🔇 Additional comments (10)
src/client/graphics/layers/StructureIconsLayer.ts (10)
19-25: LGTM!Imports for
ContextMenuEventandSwapRocketDirectionEventare correctly added to support the new locked-ghost interaction flow.
89-94: LGTM!Clean typed definition for the ghost controls UI state. Good use of composition to group related DOM elements.
182-183: LGTM!Event handler for
ContextMenuEventfollows the existing pattern and correctly wires up the locked bomb target update.
263-293: LGTM!Good separation of concerns: locked ghost repositioning runs every frame for smooth panning, while expensive action queries remain throttled. The control flow is clear.
411-431: LGTM!Clean refactor with early returns. Correctly ignores left clicks when a locked bomb is active, directing mobile users to use the OK button instead.
433-449: LGTM!Right-click target repositioning is correctly guarded and triggers trajectory recalculation via event emission.
451-457: LGTM!Correctly prevents mouse movement from affecting ghost position when locked. Ghost follows the locked tile instead.
477-492: LGTM!Properly resets lock state for non-lockable structures and initializes ghost position from locked tile when applicable.
514-533: LGTM!Ghost controls and lock state are properly cleaned up in the respective cleanup methods.
535-554: LGTM!Good extraction of build intent logic. Enables the confirm button to emit intents without removing the ghost, supporting rapid nuke placement as intended. Based on learnings, this is the desired behavior.
Description:
Improves the bomb radial attack menu (does not change hotkeys) by using a fixed position that can be repositioned with a tap. This changes the interaction to a click-to-select approach rather than continuously moving the cursor for live updates, making it better suited for mobile use.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
w.o.n