Conversation
|
|
|
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:
WalkthroughAdds a local-attack mode: new keybind/UI state to hold local-attack, client-side source-tile resolution and async menu actions, transport/schema to carry optional Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant InputHandler as "InputHandler\n(rgba(100,150,240,0.5))"
participant RadialMenu as "RadialMenu\n(rgba(140,200,120,0.5))"
participant AttackSource as "AttackSource\n(rgba(200,150,100,0.5))"
participant ClientRunner as "ClientRunner\n(rgba(180,120,200,0.5))"
participant Transport as "Transport\n(rgba(120,180,180,0.5))"
participant ServerExec as "ServerExec\n(rgba(200,100,140,0.5))"
User->>InputHandler: Hold KeyL
InputHandler->>InputHandler: set uiState.localAttackHeld = true
User->>RadialMenu: Click target
RadialMenu->>RadialMenu: read uiState.localAttackHeld
alt localAttackHeld
RadialMenu->>AttackSource: resolveAttackSourceTile(game, player, target, clicked)
AttackSource-->>RadialMenu: return sourceTile/null
end
RadialMenu->>ClientRunner: handleAttack(player, target, sourceTile)
ClientRunner->>ClientRunner: build intent (include sourceTile)
ClientRunner->>Transport: Emit SendAttackIntentEvent(intent)
Transport-->>ServerExec: deliver attack intent (with sourceTile)
ServerExec->>ServerExec: resolveSourceTile() & compute attackLossMultiplier()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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
🤖 Fix all issues with AI agents
In @src/client/ClientGameRunner.ts:
- Around line 687-707: sendAttackIntent currently awaits resolveAttackSourceTile
which internally calls player.borderTiles() and can reject; wrap the await in a
try-catch so rejections are handled instead of creating an unhandled promise
rejection. Specifically, in sendAttackIntent around the call to
resolveAttackSourceTile, catch any error, log it (e.g. this.logger.error or
console.error) and set sourceTile to null as a safe fallback, then continue to
emit the SendAttackIntentEvent so behavior remains consistent. Ensure you
reference sendAttackIntent, resolveAttackSourceTile and the fallback to null for
sourceTile in your change.
🧹 Nitpick comments (1)
src/core/Schemas.ts (1)
258-258: Consider simplifying the schema modifier.The field uses both
.nullable()and.optional(), which is redundant. Since the field can be omitted entirely (.optional()), you likely only need one modifier. Consider using just.optional()with the understanding thatundefinedwill be the default when omitted, or keep.nullable().optional()if you need to distinguish between "not provided" and "explicitly null."♻️ Simplified alternative
- sourceTile: z.number().nullable().optional(), + sourceTile: z.number().optional(),Or keep as-is if you need both semantics.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
resources/lang/en.jsonsrc/client/ClientGameRunner.tssrc/client/HelpModal.tssrc/client/InputHandler.tssrc/client/KeybindsModal.tssrc/client/Transport.tssrc/client/attackSource.tssrc/client/graphics/GameRenderer.tssrc/client/graphics/UIState.tssrc/client/graphics/layers/PlayerActionHandler.tssrc/client/graphics/layers/RadialMenuElements.tssrc/core/Schemas.tssrc/core/execution/AttackExecution.tssrc/core/execution/ExecutionManager.ts
🧰 Additional context used
🧠 Learnings (15)
📚 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/PlayerActionHandler.tssrc/core/execution/ExecutionManager.tssrc/client/attackSource.tssrc/core/execution/AttackExecution.tssrc/client/ClientGameRunner.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/client/graphics/layers/PlayerActionHandler.tssrc/client/graphics/GameRenderer.ts
📚 Learning: 2025-10-18T17:54:01.311Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:172-173
Timestamp: 2025-10-18T17:54:01.311Z
Learning: In src/core/execution/FakeHumanExecution.ts, MIRVs and ground attacks should not be mutually exclusive. The considerMIRV() method should not short-circuit maybeAttack() - both actions can occur in the same tick.
Applied to files:
src/client/graphics/layers/PlayerActionHandler.tssrc/core/execution/ExecutionManager.tssrc/core/execution/AttackExecution.tssrc/client/ClientGameRunner.ts
📚 Learning: 2025-08-23T07:48:19.060Z
Learnt from: ElMelchizedek
Repo: openfrontio/OpenFrontIO PR: 1876
File: src/core/execution/FakeHumanExecution.ts:470-473
Timestamp: 2025-08-23T07:48:19.060Z
Learning: In FakeHumanExecution.ts DefensePost placement logic, returning -Infinity from structureSpawnTileValue when no sampled border tiles neighbor enemies is intentional. The logic samples up to 50 border tiles as a heuristic - if none are adjacent to enemies, it assumes DefensePost placement is unnecessary and aborts the entire placement attempt rather than continuing to evaluate individual tiles.
Applied to files:
src/client/attackSource.tssrc/core/execution/AttackExecution.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.ts
📚 Learning: 2026-01-03T19:37:30.357Z
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.
Applied to files:
src/client/graphics/GameRenderer.tssrc/client/graphics/layers/RadialMenuElements.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/client/graphics/UIState.tssrc/core/execution/AttackExecution.ts
📚 Learning: 2025-08-29T16:16:11.309Z
Learnt from: BrewedCoffee
Repo: openfrontio/OpenFrontIO PR: 1957
File: src/core/execution/PlayerExecution.ts:40-52
Timestamp: 2025-08-29T16:16:11.309Z
Learning: In OpenFrontIO PlayerExecution.ts, when Defense Posts are captured due to tile ownership changes, the intended behavior is to first call u.decreaseLevel() to downgrade them, then still transfer them to the capturing player via captureUnit(). This is not a bug - Defense Posts should be both downgraded and captured, not destroyed outright.
Applied to files:
src/core/execution/AttackExecution.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/core/execution/AttackExecution.ts
📚 Learning: 2025-10-20T11:02:16.969Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:57-57
Timestamp: 2025-10-20T11:02:16.969Z
Learning: In src/core/execution/FakeHumanExecution.ts, the correct MIRV victory denial thresholds are VICTORY_DENIAL_TEAM_THRESHOLD = 0.8 (80% for team games) and VICTORY_DENIAL_INDIVIDUAL_THRESHOLD = 0.65 (65% for individual players), not 0.85 and 0.7 as might be mentioned in some documentation.
Applied to files:
src/core/execution/AttackExecution.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/client/ClientGameRunner.ts
📚 Learning: 2025-06-02T14:27:37.609Z
Learnt from: andrewNiziolek
Repo: openfrontio/OpenFrontIO PR: 1007
File: resources/lang/de.json:115-115
Timestamp: 2025-06-02T14:27:37.609Z
Learning: For OpenFrontIO project: When localization keys are renamed in language JSON files, the maintainers separate technical changes from translation content updates. They wait for community translators to update the actual translation values rather than attempting to translate in the same PR. This allows technical changes to proceed while ensuring accurate translations from native speakers.
Applied to files:
resources/lang/en.json
📚 Learning: 2025-08-16T10:52:08.292Z
Learnt from: TheGiraffe3
Repo: openfrontio/OpenFrontIO PR: 884
File: resources/lang/en.json:456-461
Timestamp: 2025-08-16T10:52:08.292Z
Learning: In OpenFrontIO, translation files in resources/lang/*.json (except en.json) should not be updated in regular PRs. Only dedicated translation PRs titled "mls" and made by Aotumori should update non-English locale files. Regular PRs should only update en.json when adding or modifying translation keys.
Applied to files:
resources/lang/en.json
📚 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-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.ts
🧬 Code graph analysis (7)
src/client/Transport.ts (2)
src/core/game/AttackImpl.ts (1)
sourceTile(22-24)src/core/game/GameMap.ts (1)
TileRef(3-3)
src/client/graphics/layers/PlayerActionHandler.ts (3)
src/core/game/AttackImpl.ts (1)
sourceTile(22-24)src/core/game/GameMap.ts (1)
TileRef(3-3)src/client/Transport.ts (1)
SendAttackIntentEvent(75-81)
src/client/KeybindsModal.ts (2)
src/client/LangSelector.ts (1)
translateText(248-268)src/client/Utils.ts (1)
translateText(147-202)
src/client/attackSource.ts (3)
src/core/game/GameView.ts (2)
GameView(584-917)PlayerView(185-582)src/core/game/Game.ts (1)
PlayerID(23-23)src/core/game/GameMap.ts (1)
TileRef(3-3)
src/core/execution/AttackExecution.ts (1)
src/core/game/GameMap.ts (1)
TileRef(3-3)
src/client/ClientGameRunner.ts (2)
src/client/attackSource.ts (1)
resolveAttackSourceTile(5-27)src/client/Transport.ts (1)
SendAttackIntentEvent(75-81)
src/client/graphics/layers/RadialMenuElements.ts (2)
src/core/game/AttackImpl.ts (1)
sourceTile(22-24)src/client/attackSource.ts (1)
resolveAttackSourceTile(5-27)
🪛 GitHub Actions: 🧪 CI
src/core/Schemas.ts
[warning] 1-1: Module 'src/core/Schemas.ts' tried to import 'resources/countries.json' with no attributes, but it was already imported elsewhere with 'type': 'json' attributes. Please ensure import attributes for the same module are consistent.
🔇 Additional comments (22)
src/client/graphics/UIState.ts (1)
7-7: LGTM! Clean interface extension.The
localAttackHeldfield follows the same pattern as other boolean state flags in this interface.resources/lang/en.json (1)
500-501: LGTM! Clear and descriptive localization strings.The text properly explains the hold-to-activate behavior and describes what the feature does.
src/client/HelpModal.ts (1)
52-52: LGTM! Keybinding properly configured.The
localAttackkeybinding follows the same pattern as other keys and will be correctly merged with user settings.src/client/InputHandler.ts (4)
215-215: LGTM! Keybinding properly defined.The
localAttackkeybinding is consistent with other action keys in the system.
342-342: LGTM! Proper hold-to-activate implementation.The keydown handler correctly sets the
localAttackHeldstate flag, enabling the hold-to-activate behavior described in the localization strings. This differs appropriately from the event-based pattern used for instant actions likegroundAttackandboatAttack.Also applies to: 353-355
394-396: LGTM! Clean keyup handling.Properly resets the
localAttackHeldflag when the key is released, completing the hold-to-activate pattern.
644-644: LGTM! Proper cleanup.Resetting
localAttackHeldindestroy()prevents stale state if the input handler is torn down while the key is being held.src/core/execution/ExecutionManager.ts (1)
63-63: Schema and constructor changes are in place.The
sourceTilefield is correctly defined inAttackIntentSchemaasz.number().nullable().optional(), and theAttackExecutionconstructor accepts it asprivate sourceTile: TileRef | null = null. The nullish coalescing operator cleanly handles the optional value.src/client/graphics/GameRenderer.ts (1)
59-59: LGTM - Clean addition to UI state.The
localAttackHeldflag is appropriately initialized tofalseand integrates cleanly with the attack source resolution flow.src/client/graphics/layers/PlayerActionHandler.ts (1)
33-45: LGTM - Backward-compatible signature extension.The optional
sourceTileparameter with a default ofnullmaintains backward compatibility while cleanly supporting the local attack feature.src/client/Transport.ts (2)
75-81: LGTM - Event signature properly extended.The
SendAttackIntentEventconstructor cleanly accepts the optionalsourceTileparameter, maintaining backward compatibility with the defaultnullvalue.
489-497: LGTM - Payload correctly includes sourceTile.The attack intent payload now properly includes
sourceTile, aligning with the schema update inSchemas.ts.src/client/ClientGameRunner.ts (2)
50-50: LGTM - Clean import addition.The
resolveAttackSourceTileimport supports the new local attack feature.
527-527: Intentional fire-and-forget pattern for attack intents.The
voidkeyword indicates intentional fire-and-forget async calls. This is consistent with the existing pattern forsendBoatAttackIntent(line 618), which also performs async operations without awaiting at call sites.While this pattern works for non-critical UI actions, ensure errors in
sendAttackIntentwon't cause silent failures that confuse users.Also applies to: 638-638
src/client/KeybindsModal.ts (2)
24-24: LGTM!New keybind entry follows the existing pattern and uses a sensible default key.
436-444: No action needed. The translation keysuser_setting.local_attackanduser_setting.local_attack_descalready exist inresources/lang/en.json.src/core/execution/AttackExecution.ts (2)
316-329: LGTM!Clean validation logic. The method correctly checks ownership and adjacency before accepting the source tile. Good defensive server-side validation of client input.
100-101: LGTM!The init flow properly resolves the source tile before using it, and branches correctly: use
addNeighborsfrom the specific tile when valid, fall back torefreshToConquerotherwise.Also applies to: 116-120
src/client/attackSource.ts (2)
5-27: LGTM!Clean implementation finding the closest border tile that borders the target. The async pattern is correct for the
borderTiles()API.
29-40: No action needed—behavior is correct.TerraNullius.id() returns
nullas expected, and the comparison inbordersTarget()works correctly for both cases: whentargetIdisnull(attacking unclaimed land) and when it's a PlayerID string (attacking a player).src/client/graphics/layers/RadialMenuElements.ts (2)
6-6: LGTM!Import added for the new attack source resolution module.
588-621: Code change is correct.The async change and local attack flow are properly implemented. The
handleAttackmethod inPlayerActionHandleraccepts thesourceTileparameter with typeTileRef | null, matching what the code passes—either a resolved tile ornull. The logic correctly:
- Checks if local attack mode is active
- Resolves the source tile only when needed
- Passes the result (or
null) tohandleAttack
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/configuration/DefaultConfig.ts (1)
657-675: Critical: Switch statement missing cases for Lake and Ocean terrain types.The
TerrainTypeenum has 5 values:Plains,Highland,Mountain,Lake, andOcean. The switch statement only handles the first 3, causing a crash when the terrain type isLake(3) orOcean(4). The default case throws an error instead of handling these valid enum values.Add cases for
LakeandOceanwith appropriate magnitude and speed values.
🧹 Nitpick comments (1)
src/core/execution/AttackExecution.ts (1)
20-20: Consider moving to Config for easier tuning.The
localAttackLossMultiplierconstant is a game balance parameter. Consider moving it toConfig(similar to other balance constants) to make it easier to tune without code changes.Based on learnings, game balance constants are experimental tuning parameters subject to frequent change during balance testing.
♻️ Proposed refactor
Add to
Config.tsinterface:+ localAttackLossMultiplier(): number;Add to
DefaultConfig.tsimplementation:+ localAttackLossMultiplier(): number { + return 1.5; + }Update usage in
AttackExecution.ts:- return localAttackLossMultiplier; + return this.mg.config().localAttackLossMultiplier();
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/core/configuration/Config.tssrc/core/configuration/DefaultConfig.tssrc/core/execution/AttackExecution.ts
🧰 Additional context used
🧠 Learnings (8)
📚 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/configuration/Config.tssrc/core/configuration/DefaultConfig.tssrc/core/execution/AttackExecution.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/configuration/Config.tssrc/core/configuration/DefaultConfig.ts
📚 Learning: 2025-10-20T11:02:16.969Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:57-57
Timestamp: 2025-10-20T11:02:16.969Z
Learning: In src/core/execution/FakeHumanExecution.ts, the correct MIRV victory denial thresholds are VICTORY_DENIAL_TEAM_THRESHOLD = 0.8 (80% for team games) and VICTORY_DENIAL_INDIVIDUAL_THRESHOLD = 0.65 (65% for individual players), not 0.85 and 0.7 as might be mentioned in some documentation.
Applied to files:
src/core/configuration/DefaultConfig.tssrc/core/execution/AttackExecution.ts
📚 Learning: 2025-10-18T17:54:01.311Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:172-173
Timestamp: 2025-10-18T17:54:01.311Z
Learning: In src/core/execution/FakeHumanExecution.ts, MIRVs and ground attacks should not be mutually exclusive. The considerMIRV() method should not short-circuit maybeAttack() - both actions can occur in the same tick.
Applied to files:
src/core/execution/AttackExecution.ts
📚 Learning: 2025-08-23T07:48:19.060Z
Learnt from: ElMelchizedek
Repo: openfrontio/OpenFrontIO PR: 1876
File: src/core/execution/FakeHumanExecution.ts:470-473
Timestamp: 2025-08-23T07:48:19.060Z
Learning: In FakeHumanExecution.ts DefensePost placement logic, returning -Infinity from structureSpawnTileValue when no sampled border tiles neighbor enemies is intentional. The logic samples up to 50 border tiles as a heuristic - if none are adjacent to enemies, it assumes DefensePost placement is unnecessary and aborts the entire placement attempt rather than continuing to evaluate individual tiles.
Applied to files:
src/core/execution/AttackExecution.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/execution/AttackExecution.ts
📚 Learning: 2025-08-29T16:16:11.309Z
Learnt from: BrewedCoffee
Repo: openfrontio/OpenFrontIO PR: 1957
File: src/core/execution/PlayerExecution.ts:40-52
Timestamp: 2025-08-29T16:16:11.309Z
Learning: In OpenFrontIO PlayerExecution.ts, when Defense Posts are captured due to tile ownership changes, the intended behavior is to first call u.decreaseLevel() to downgrade them, then still transfer them to the capturing player via captureUnit(). This is not a bug - Defense Posts should be both downgraded and captured, not destroyed outright.
Applied to files:
src/core/execution/AttackExecution.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/core/execution/AttackExecution.ts
🧬 Code graph analysis (3)
src/core/configuration/Config.ts (1)
src/core/execution/AttackExecution.ts (1)
attackLossMultiplier(333-342)
src/core/configuration/DefaultConfig.ts (1)
src/core/execution/AttackExecution.ts (1)
attackLossMultiplier(333-342)
src/core/execution/AttackExecution.ts (1)
src/core/game/GameMap.ts (1)
TileRef(3-3)
🪛 GitHub Actions: 🧪 CI
src/core/configuration/DefaultConfig.ts
[error] 674-674: terrain type 3 not supported
⏰ 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 (8)
src/core/configuration/Config.ts (1)
100-111: LGTM!The
attackLossMultiplierparameter is cleanly added to theattackLogicsignature. The placement betweenattackTroopsandattackeris logical.src/core/configuration/DefaultConfig.ts (3)
645-656: LGTM!The method signature correctly implements the updated
Configinterface with the newattackLossMultiplierparameter.
736-752: LGTM!The
attackLossMultiplieris correctly applied toattackerTroopLossin the defender-is-player branch. The multiplier is appropriately chained with other factors like traitor debuff and large attacker/defender modifiers.
754-765: LGTM!The
attackLossMultiplieris correctly applied toattackerTroopLossin the non-player defender branch, maintaining consistency with the player-defender logic.src/core/execution/AttackExecution.ts (4)
101-121: LGTM!The source tile resolution and conditional neighbor logic correctly implements the "local attack" feature. When a valid
sourceTileexists, the attack is constrained to that region; otherwise it falls back to the original full-border behavior.
288-297: LGTM!The
attackLossMultiplier()is correctly passed toattackLogicin the right position, matching the updated method signature.
318-331: LGTM!The
resolveSourceTile()validation logic is sound:
- Ensures the attacker still owns the source tile
- Verifies at least one neighbor is owned by the target
- Returns
nullif invalid, triggering fallback to full-border modeThe method correctly handles edge cases like ownership changes between attack creation and initialization.
333-342: LGTM!The
attackLossMultiplier()logic correctly applies the 1.5x penalty only when:
- It's a local attack (sourceTile exists)
- The target is a player (not neutral territory)
- Troops are actually removed (real attack, not simulation)
This is sound game design - local attacks against players are more costly, while attacks on neutral territory aren't penalized.
This PR only allows choosing where an attack starts from, so it feels separate from anything that defines overall direction or strategy. |
Ah ok. To me it feels kind of like a directed attack but it's subtle i suppose Btw: the description says 'hold' but when the player takes that literally, they might expect holding L to start the attack. While (at least in my case) you have to also click to attack there. If that's correct i would propose to describe it more like 'hold + attack-click' |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/util/TestConfig.ts (1)
96-116: Missing parameter in delegation call.The method receives
attackLossMultiplierbut doesn't forward it toDefaultConfig.prototype.attackLogic.call(). This breaks the delegation and causes the real attack logic to receive incorrect parameter values.Fix: Forward the attackLossMultiplier parameter
return DefaultConfig.prototype.attackLogic.call( this, gm, attackTroops, + attackLossMultiplier, attacker, defender, tileToConquer, );
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/util/TestConfig.ts
🧰 Additional context used
🧠 Learnings (4)
📚 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:
tests/util/TestConfig.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:
tests/util/TestConfig.ts
📚 Learning: 2025-10-20T11:02:16.969Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:57-57
Timestamp: 2025-10-20T11:02:16.969Z
Learning: In src/core/execution/FakeHumanExecution.ts, the correct MIRV victory denial thresholds are VICTORY_DENIAL_TEAM_THRESHOLD = 0.8 (80% for team games) and VICTORY_DENIAL_INDIVIDUAL_THRESHOLD = 0.65 (65% for individual players), not 0.85 and 0.7 as might be mentioned in some documentation.
Applied to files:
tests/util/TestConfig.ts
📚 Learning: 2025-10-18T17:54:01.311Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:172-173
Timestamp: 2025-10-18T17:54:01.311Z
Learning: In src/core/execution/FakeHumanExecution.ts, MIRVs and ground attacks should not be mutually exclusive. The considerMIRV() method should not short-circuit maybeAttack() - both actions can occur in the same tick.
Applied to files:
tests/util/TestConfig.ts
⏰ 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 (1)
tests/util/TestConfig.ts (1)
70-83: LGTM - test stub correctly updated.The signature correctly includes the new
attackLossMultiplierparameter. Since this is a test stub returning hardcoded values, not using the parameter is expected behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/InputHandler.test.ts (1)
37-42: Consider adding test coverage for the newlocalAttackHeldoption.The parameter addition correctly maintains compatibility with the updated InputHandler constructor. However, since this PR introduces localized attack functionality, consider adding tests that verify behavior when
localAttackHeldistrue. This would help catch regressions and document the expected behavior.For example:
- Test that appropriate events are emitted when localAttackHeld is active
- Test interaction between localAttackHeld state and pointer events
- Test any coordinate resolution or attack source logic that depends on this flag
Do you want me to generate test cases for the
localAttackHeldfunctionality?
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
resources/lang/en.jsontests/InputHandler.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- resources/lang/en.json
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: andrewNiziolek
Repo: openfrontio/OpenFrontIO PR: 1007
File: resources/lang/de.json:115-115
Timestamp: 2025-06-02T14:27:37.609Z
Learning: For OpenFrontIO project: When localization keys are renamed in language JSON files, the maintainers separate technical changes from translation content updates. They wait for community translators to update the actual translation values rather than attempting to translate in the same PR. This allows technical changes to proceed while ensuring accurate translations from native speakers.
📚 Learning: 2026-01-09T06:10:44.499Z
Learnt from: plazmaezio
Repo: openfrontio/OpenFrontIO PR: 2716
File: tests/AllianceAcceptNukes.test.ts:85-87
Timestamp: 2026-01-09T06:10:44.499Z
Learning: In tests under tests/**, objects may stack on the same tile coordinates (e.g., MissileSilo). When writing tests that involve building/placing structures or cooldowns, assume multiple items can share coordinates; adjust assertions to account for stacking, and explicitly manage or reset the stack state where necessary.
Applied to files:
tests/InputHandler.test.ts
⏰ 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
I fixed title & desc 'w' |
|
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
|
This pr need to be approved |
github-actions
bot
removed
the
Stale
true i want a feature like that in the game |
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/RadialMenuElements.ts (1)
593-626:⚠️ Potential issue | 🟠 MajorUpdate
CenterButtonElementandMenuElementtype signatures to allow async actions — unhandled rejection risk with real async operations.
CenterButtonElement.actionis typed as(params: MenuElementParams) => void(line 71), but this implementation isasync, creating a promise that the caller neverawaits (RadialMenu.ts line 847). UnlikeboatMenuElement(which is also async but contains no actual awaits),centerButtonElementcontains a real async operation:await resolveAttackSourceTile(...). If this call throws, the promise rejection goes unhandled andparams.closeMenu()never executes, leaving the menu in an inconsistent state.Update both interfaces to accept async actions:
Type fixes
export interface CenterButtonElement { disabled: (params: MenuElementParams) => boolean; - action: (params: MenuElementParams) => void; + action: (params: MenuElementParams) => void | Promise<void>; }export interface MenuElement { id: string; name: string; displayed?: boolean | ((params: MenuElementParams) => boolean); color?: string | ((params: MenuElementParams) => string); icon?: string; - action?: (params: MenuElementParams) => void; + action?: (params: MenuElementParams) => void | Promise<void>; }Then update the caller in RadialMenu.ts to handle async actions:
- this.centerButtonElement?.action(this.params); + await this.centerButtonElement?.action(this.params);
🧹 Nitpick comments (1)
src/core/Schemas.ts (1)
296-296: Consider adding.int().nonnegative()tosourceTilevalidation.Other tile fields in this file (lines 301, 363, 386) also use bare
z.number(), so this is consistent. But sincesourceTileis client-supplied and used server-side for attack origin, tighter validation would help reject bad input early.Suggested tightening
- sourceTile: z.number().nullable().optional(), + sourceTile: z.number().int().nonnegative().nullable().optional(),
|
Hello! A consideration that needs to be taken is how would someone on mobile complete this action? Without a mobile equivilant this wouldnt be able to be added due to balancing across platforms |
Why didn't you engage about this in the countless attempts to talk about this PR in the Dev server? Feels weird to close it all of a sudden |
|
I didnt see it under discussion in the Dev server, however we are trying to make it easier for people to see in the future but people do need to think about the wider game ecosystem |
Am I correct in understanding that the idea has been rejected? |
In its current form yes, could you plan out how to implement this on mobile alongside the actual game? |
@iiamlewis I’m not attached to this specific UI idea — if there’s a better approach, I’m completely open to changing it. |
@iiamlewis |
5 participants
Description:
Add a “localized attack” hold key (
KeyL) so holding the key and clicking attack starts from the nearest border tile to the target.Add an attack loss multiplier parameter so localized attacks increase attacker losses (set to
1.2x).2026-01-10.21.29.58.mov
2026-01-10.21.25.50.mov
2026-01-10.21.23.11.discord.mp4
2026-01-10.21.19.11.discord.mp4
2026-01-10.21.18.48.mov
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
aotumuri