feat: Progressive Building Placement (Hold-to-Build) #2914
+216
−40
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
Contributor
WalkthroughProgressive, mouse-driven structure placement was added: holding mouse initiates repeated build/upgrade attempts with acceleration timing, per-structure placement state, and ghost alignment. Weapon-type structures are excluded from repeat placement; legacy placement APIs are preserved as deprecated shims. Changes
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/client/graphics/layers/StructureIconsLayer.ts`:
- Around line 479-486: Before calling game.ref() after computing currentTile via
transformHandler.boundingRect() and
transformHandler.screenToWorldCoordinates(this.mousePos...), validate the
coordinates: call isValidCoord(currentTile.x, currentTile.y) (or the project’s
equivalent) and if it returns false, avoid calling game.ref(), exit the handler
early and stop the repeat loop (clear the interval / cancel the repeat
invocation). Ensure the guard is placed just before game.ref(...) so drifted
off‑map mouse positions won’t throw and will terminate the ongoing repeat
action.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/client/graphics/layers/StructureIconsLayer.tssrc/core/game/PlayerImpl.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: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.
📚 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
📚 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/layers/StructureIconsLayer.ts
📚 Learning: 2026-01-12T21:37:01.156Z
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.
Applied to files:
src/client/graphics/layers/StructureIconsLayer.tssrc/core/game/PlayerImpl.ts
📚 Learning: 2025-08-12T00:31:50.144Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1752
File: src/core/game/Game.ts:750-752
Timestamp: 2025-08-12T00:31:50.144Z
Learning: In the OpenFrontIO codebase, changes to the PlayerInteraction interface (like adding canDonateGold and canDonateTroops flags) do not require corresponding updates to src/core/Schemas.ts or server serialization code.
Applied to files:
src/core/game/PlayerImpl.ts
📚 Learning: 2025-12-13T14:58:29.645Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2607
File: src/core/execution/PlayerExecution.ts:271-295
Timestamp: 2025-12-13T14:58:29.645Z
Learning: In src/core/execution/PlayerExecution.ts surroundedBySamePlayer(), the `as Player` cast on `mg.playerBySmallID(scan.enemyId)` is intentional. Since scan.enemyId comes from ownerID() on an owned tile and playerBySmallID() only returns Player or undefined, the cast expresses a known invariant. The maintainers prefer loud failures (runtime errors) over silent masking (early returns with guards) for corrupted game state scenarios at trusted call sites.
Applied to files:
src/core/game/PlayerImpl.ts
📚 Learning: 2025-10-20T20:15:28.858Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:51-51
Timestamp: 2025-10-20T20:15:28.858Z
Learning: In src/core/execution/FakeHumanExecution.ts, game balance constants like MIRV_COOLDOWN_TICKS, MIRV_HESITATION_ODDS, VICTORY_DENIAL_TEAM_THRESHOLD, VICTORY_DENIAL_INDIVIDUAL_THRESHOLD, and STEAMROLL_CITY_GAP_MULTIPLIER are experimental tuning parameters subject to frequent change during balance testing. Do not flag changes to these values as issues or compare them against previous values.
Applied to files:
src/core/game/PlayerImpl.ts
📚 Learning: 2026-01-13T20:16:20.098Z
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2887
File: src/core/execution/NukeExecution.ts:118-122
Timestamp: 2026-01-13T20:16:20.098Z
Learning: In the OpenFrontIO codebase, `playerBySmallID(id: number)` in Game, GameImpl, and GameView explicitly returns `Player | TerraNullius` (or `PlayerView | TerraNullius`), never undefined. The method returns TerraNullius for ID 0 and Player otherwise. Code should check `.isPlayer()` directly without undefined guards or optional chaining.
Applied to files:
src/core/game/PlayerImpl.ts
📚 Learning: 2025-10-26T15:37:07.732Z
Learnt from: GlacialDrift
Repo: openfrontio/OpenFrontIO PR: 2298
File: src/client/graphics/layers/TerritoryLayer.ts:200-210
Timestamp: 2025-10-26T15:37:07.732Z
Learning: In GameImpl.ts lines 124-139, team assignment logic varies by number of teams: when numPlayerTeams < 8, teams are assigned ColoredTeams values (Red, Blue, Yellow, Green, Purple, Orange, Teal); when numPlayerTeams >= 8, teams are assigned generic string identifiers like "Team 1", "Team 2", etc., which are not members of ColoredTeams.
Applied to files:
src/core/game/PlayerImpl.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: In `ClientGameRunner.ts`, the `myPlayer` field is always set when `shouldPreventWindowClose()` is called, so the null check in that method is sufficient without needing to fetch it again from `gameView.playerByClientID()`.
Applied to files:
src/core/game/PlayerImpl.ts
📚 Learning: 2025-10-08T17:14:49.369Z
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.
Applied to files:
src/core/game/PlayerImpl.ts
🧬 Code graph analysis (1)
src/client/graphics/layers/StructureIconsLayer.ts (4)
src/client/InputHandler.ts (2)
MouseDownEvent(38-43)MouseUpEvent(8-13)src/core/game/GameImpl.ts (4)
x(884-886)y(887-889)ref(878-880)cell(890-892)src/core/game/GameMap.ts (4)
x(127-129)y(131-133)ref(116-121)cell(135-137)src/client/Transport.ts (2)
SendUpgradeStructureIntentEvent(51-56)BuildUnitIntentEvent(91-97)
🪛 GitHub Actions: 🧪 CI
src/client/graphics/layers/StructureIconsLayer.ts
[warning] 1-1: Code style issues found in file. Run 'npx prettier --write' to fix.
src/core/game/PlayerImpl.ts
[warning] 1-1: Code style issues found in file. Run 'npx prettier --write' to fix.
🔇 Additional comments (17)
src/core/game/PlayerImpl.ts (3)
173-176: Alliance extension prompt window logic looks good.
1165-1167: toString reformat is fine.
58-62: ** Run Prettier to verify and resolve any style formatting issues in this file.**CI reports style issues in this file. Please run
npx prettier --writeto check and resolve spacing and formatting inconsistencies.src/client/graphics/layers/StructureIconsLayer.ts (14)
19-23: MouseDownEvent import update looks good.
99-108: Repeat-placement state setup looks good.
186-188: Mouse down/up hooks are wired correctly.
393-402: Weapon exclusion helper is clear.
405-424: Start tile capture on mouse down looks good.
446-451: Timer cleanup is good.
453-466: Progressive delay scheduling looks good.
534-542: Stop-repeat reset is fine.
547-550: Legacy redirect is fine.
555-558: Legacy stub is fine.
568-577: Ghost locking to the repeat tile looks correct.
827-833: Scale calculation tweak is fine.
50-58: Run Prettier to clear the CI style warning.
CI reports style issues in this file; line 58 spacing is one example. Please runnpx prettier --write.💄 Example formatting fix
- ) { } + ) {}
429-441: Check if locked-ghost nuke confirm triggersonMouseUpremoval.The locked-ghost nuke confirm flow (AtomBomb/HydrogenBomb) intentionally skips
removeGhostStructure()to enable rapid placements by spamming the confirm button. TheonMouseUpchange here clears the ghost on every mouse-up, which could break this rapid-placement flow if the confirm button click triggersonMouseUp. Verify whether locked-ghost confirm uses a separate input path that avoidsonMouseUp, or add a gate to skip removal for locked-ghost cases.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/client/graphics/layers/StructureIconsLayer.ts`:
- Line 58: The file StructureIconsLayer.ts has formatting/style violations (see
the empty block inside the class/method signature shown as ") { }"); run the
project's formatter to fix styling by running npx prettier --write on the file
(or the repo) and re-commit the formatted changes so StructureIconsLayer (and
any constructor/method blocks such as the empty ") { }" block) conform to CI
formatting rules.
- Around line 558-566: stopRepeat() currently always clears the ghost (calls
removeGhostStructure()) and nulls repeatBuildType/repeatTileRef, which removes
the locked nuke ghost undesirably; change stopRepeat to detect a locked nuke
weapon placement (e.g. check repeatBuildType is AtomBomb/HydrogenBomb or
ghostUnit.hasLockedNuke/isLocked flag) and in that case do NOT call
removeGhostStructure() and do NOT clear repeatBuildType/repeatTileRef so the
ghost remains for rapid follow-up; otherwise preserve the existing behavior
(clear timers, null repeat fields, remove ghost).
📜 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 (8)
📓 Common learnings
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: plazmaezio
Repo: openfrontio/OpenFrontIO PR: 2716
File: tests/AllianceAcceptNukes.test.ts:85-87
Timestamp: 2026-01-09T06:10:50.371Z
Learning: In the OpenFrontIO game, multiple structures (including MissileSilo) can be built on the same tile coordinates - they "stack up" on the same location. This is intentional game behavior and commonly used in tests to overcome cooldown limitations.
📚 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
📚 Learning: 2025-11-29T22:22:37.178Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2539
File: src/core/game/GameImpl.ts:520-542
Timestamp: 2025-11-29T22:22:37.178Z
Learning: In GameImpl.ts, neighborsWithDiag() and forEachNeighborWithDiag() intentionally duplicate coordinate iteration logic. They serve different purposes: forEachNeighborWithDiag() is a zero-allocation hot-path optimization while neighborsWithDiag() is a convenience method that returns an array. Refactoring one to call the other would add callback/closure allocations and indirection overhead, defeating the performance goals.
Applied to files:
src/client/graphics/layers/StructureIconsLayer.ts
📚 Learning: 2025-12-13T14:58:29.645Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2607
File: src/core/execution/PlayerExecution.ts:271-295
Timestamp: 2025-12-13T14:58:29.645Z
Learning: In src/core/execution/PlayerExecution.ts surroundedBySamePlayer(), the `as Player` cast on `mg.playerBySmallID(scan.enemyId)` is intentional. Since scan.enemyId comes from ownerID() on an owned tile and playerBySmallID() only returns Player or undefined, the cast expresses a known invariant. The maintainers prefer loud failures (runtime errors) over silent masking (early returns with guards) for corrupted game state scenarios at trusted call sites.
Applied to files:
src/client/graphics/layers/StructureIconsLayer.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/StructureIconsLayer.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/layers/StructureIconsLayer.ts
📚 Learning: 2026-01-12T21:37:01.156Z
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.
Applied to files:
src/client/graphics/layers/StructureIconsLayer.ts
🧬 Code graph analysis (1)
src/client/graphics/layers/StructureIconsLayer.ts (6)
src/client/InputHandler.ts (2)
MouseDownEvent(38-43)MouseUpEvent(8-13)src/core/game/GameView.ts (7)
type(98-100)type(467-469)x(832-834)y(835-837)tile(110-112)ref(826-828)cell(838-840)src/core/game/UnitImpl.ts (2)
type(148-150)tile(178-180)src/core/game/GameMap.ts (4)
x(127-129)y(131-133)ref(116-121)cell(135-137)src/client/Transport.ts (2)
SendUpgradeStructureIntentEvent(51-56)BuildUnitIntentEvent(91-97)src/client/graphics/layers/StructureDrawingUtils.ts (2)
LEVEL_SCALE_FACTOR(25-25)ICON_SCALE_FACTOR_ZOOMED_IN(26-26)
🪛 GitHub Actions: 🧪 CI
src/client/graphics/layers/StructureIconsLayer.ts
[warning] 1-1: Code style issues found in this file. Run 'npx prettier --write' to fix.
🔇 Additional comments (10)
src/client/graphics/layers/StructureIconsLayer.ts (10)
99-109: Progressive placement state is clear and easy to follow.
186-188: MouseDown/MouseUp wiring looks good for hold-to-build.
393-403: Weapon filter list is explicit and safe.
405-424: Start-tile lock and immediate first placement make sense.
429-441: Mouse-up cleanup is clear and consistent.
446-471: Timer cleanup + acceleration math look solid.
473-552: Repeat placement flow is solid (bounds guard + snap update).
592-601: Ghost lock during repeat placement looks good.
855-857: Scale factor switch is clear and safe.
568-582: Delete both unused deprecated methods.These private methods have zero callers anywhere in the codebase, making them safe to remove entirely. Since both
placeStructureAtPosition()andcreateStructure()are private, they cannot be called from outside the class anyway. The deprecation comments claiming "external callers" are misleading—there are none to worry about.Simply delete these methods rather than keeping them as empty shims.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description:
Implements 'Hold to Place' functionality for Bottom Menu Buildings. Holding the mouse button places multiple buildings at an accelerating rate.
(Note: This applies to the Structure Icons Layer (bottom bar), not the right-click context menu).
Features:
Please complete the following:
Discord username:
andres024020