WIP Vassalage #2732
WIP Vassalage #2732
Conversation
Goals: - Make hierarchy/vassalage durable with enforced alliances and consistent client updates. - Keep hierarchy logic consistent across server/client and reduce magic constants. - Allow hierarchy-aware targeting/transport while preserving win conditions. Summary: - Introduced HierarchyUtils + relation helpers; aligned server/client hierarchy checks. - Added AllianceImpl.PERMANENT and isEnforced(), and used for enforced (permanent) alliances. - Refreshed hierarchy alliances quietly for rebuilds, but announce actual expirations. - Updated nuke/MIRV targeting rules to respect hierarchy directionality. - Transport ship hierarchy calculations now use shared hierarchy logic. - Win check counts vassal tiles under the root overlord (even cross-team). - Enabled building on vassal territory (via hierarchy access checks) and added related tests. - Added vassal offer/support executions, UI hooks, and tests for vassal flows.
…ests Goal: - Update merged code to ensure it's vassal aware. Summary: - Added hierarchy-aware win checks and nation MIRV threat scoring using vassal tiles and effective troops. - Implemented vassal-aware alliance decisions, vassal offer heuristics, and enforced-alliance handling. - Added build-menu failure reasons from server and new EN translation keys; localized vassal UI strings. - Prevented bot participation in vassal offers and refreshed hierarchy transport helpers. - Added test ensuring nukes can hit temp allies/descendants but not siblings or overlords. - Fixed test config to include vassal toggle support.
WalkthroughAdds a vassal/overlord system across client and server: new intents/updates, vassalage lifecycle, tribute and support mechanics, UI controls (slider, menus), hierarchy-aware AI/attack/conquest logic, and many tests and localization keys. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Transport as Transport (EventBus)
participant Server as GameServer
participant ExecMgr as ExecutionManager
participant Vex as VassalOfferExecution
Client->>Transport: SendForceVassalIntentEvent(target)
Transport->>Server: POST intent "offerVassal"
Server->>ExecMgr: enqueue intent
ExecMgr->>Vex: instantiate VassalOfferExecution(requestor, target)
Vex->>Server: init() (validate, resolve target, set expiry)
Note over Vex: Evaluate shouldAccept (strength, alliances, surrounded)
alt Target is Bot and shouldAccept
Vex->>Server: perform vassalize()
Server->>Client: broadcast VASSAL_ACCEPTED update
else Target is Human
Vex->>Server: emit VassalOfferRequest update
Server->>Client: display offer UI
Client->>Transport: SendVassalOfferReplyIntentEvent(accept)
Transport->>Server: POST intent "vassalOfferReply"
Server->>ExecMgr: enqueue reply intent
ExecMgr->>Server: VassalOfferReplyExecution processes accept/reject -> vassalize or notify
Server->>Client: broadcast VASSAL_ACCEPTED or VASSAL_REJECTED
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/game/TrainStation.ts (1)
52-78: Extract shared payout logic to eliminate duplication.PortStopHandler duplicates the exact same effective owner logic from CityStopHandler (lines 28-48). This violates DRY and makes maintenance harder.
🔎 Extract shared helper
Add a shared helper before the handler classes:
function payStationRevenue( mg: Game, station: TrainStation, trainExecution: TrainExecution, ): void { const stationOwner = station.unit.owner(); const trainOwner = trainExecution.owner(); const tileOwner = mg.owner(station.tile()); const effectiveOwner = tileOwner?.isPlayer?.() && tileOwner !== stationOwner && stationOwner.isFriendly(tileOwner) ? tileOwner : stationOwner; const goldBonus = mg.config().trainGold(rel(trainOwner, effectiveOwner)); if (trainOwner !== effectiveOwner) { effectiveOwner.addGold(goldBonus, station.tile()); } trainOwner.addGold(goldBonus, station.tile()); }Then simplify both handlers:
class CityStopHandler implements TrainStopHandler { onStop( mg: Game, station: TrainStation, trainExecution: TrainExecution, ): void { - const stationOwner = station.unit.owner(); - const trainOwner = trainExecution.owner(); - const tileOwner = mg.owner(station.tile()) as Player | null; - - const effectiveOwner = ... - const goldBonus = ... - if (trainOwner !== effectiveOwner) { ... } - trainOwner.addGold(goldBonus, station.tile()); + payStationRevenue(mg, station, trainExecution); } }Apply the same change to PortStopHandler.
🧹 Nitpick comments (27)
src/core/game/TrainStation.ts (1)
28-48: Simplify the type guards and consider extracting shared payout logic.The effective owner logic introduces complexity that could be cleaner:
Redundant type checks: Lines 34-35 check both
tileOwner.isPlayer(property) andtileOwner.isPlayer()(method). After theisPlayer()check passes, the subsequent casts on lines 37-38 are redundant.Code structure: The double payout (lines 44-48) where both
effectiveOwnerandtrainOwnerreceive gold appears intentional for vassal/hierarchy revenue sharing, but this isn't immediately obvious from the code alone.🔎 Cleaner type guard pattern
- const tileOwner = mg.owner(station.tile()) as Player | null; - - // If city is built by a friendly on someone else's land, treat the land owner - // as the effective station owner for revenue purposes so both sides get full share. - const effectiveOwner = - tileOwner && - tileOwner.isPlayer && - tileOwner.isPlayer() && - tileOwner !== stationOwner && - stationOwner.isFriendly(tileOwner as Player) - ? (tileOwner as Player) - : stationOwner; + const tileOwner = mg.owner(station.tile()); + + // If city is built by a friendly on someone else's land, treat the land owner + // as the effective station owner for revenue purposes so both sides get full share. + const effectiveOwner = + tileOwner?.isPlayer?.() && + tileOwner !== stationOwner && + stationOwner.isFriendly(tileOwner) + ? tileOwner + : stationOwner;src/client/graphics/layers/PlayerActionHandler.ts (1)
80-88: Keyboard listener may leave dangling handler on early cleanup.The
{ once: true }option removes the listener after the first keydown. However, if the user clicks a button before pressing any key,cleanup()is called but the keydown listener remains attached until some key is pressed.Consider removing the listener explicitly in
cleanup().Proposed fix
+ const onKey = (e: KeyboardEvent) => { + if (e.key === "Escape") cleanup(); + if (e.key === "Enter") { + e.preventDefault(); + cleanup(); + opts.onConfirm(); + } + }; + document.addEventListener("keydown", onKey); + - const onKey = (e: KeyboardEvent) => { - if (e.key === "Escape") cleanup(); - if (e.key === "Enter") { - e.preventDefault(); - cleanup(); - opts.onConfirm(); - } - }; - document.addEventListener("keydown", onKey, { once: true }); - const cleanup = () => overlay.remove(); + const cleanup = () => { + document.removeEventListener("keydown", onKey); + overlay.remove(); + };src/core/game/PlayerImpl.ts (2)
568-569: Consider caching vassals() result if called frequently.
isOverlordOfcallsthis.vassals().includes(other), andvassals()filtersmg.vassalages_each time. If this is called in hot paths (like rendering or per-tick logic), consider caching the result.
350-357: Intentional semantic difference between canUseTerritoryOf and hasTerritorialAccess needs clarification.
canUseTerritoryOf(line 350) permits only overlord→vassal territory access for building/structures:return p === this || this.isOverlordOf(p);
hasTerritorialAccess(line 582) permits bidirectional access for unit movement:return this.isOverlordOf(owner) || this.isVassalOf(owner);This asymmetry reflects different game rules (building restrictions vs movement permissions) but is easy to misread. Rename to clarify intent:
canBuildOnTerritoryOfvscanMoveThroughTerritory, or add clear comments explaining why semantics differ.src/client/graphics/UIState.ts (1)
4-4: Restore type safety forghostStructure.Changing
ghostStructurefromUnitType | nulltoanyremoves compile-time type checking and allows any value to be assigned without validation. This weakens type safety significantly.Consider using a proper typed union instead:
- If only
UnitType | nullis needed, keep the original type- If more flexibility is needed, define an explicit union:
UnitType | SomeOtherType | null- Reserve
anyonly for cases where typing is genuinely impossible🔎 Restore typed union
export interface UIState { attackRatio: number; vassalSupportRatio?: number; - ghostStructure: any; + ghostStructure: UnitType | null; }If additional types are needed beyond
UnitType, define an explicit union rather than usingany.tests/client/graphics/ControlPanel.test.ts (1)
41-92: Test covers the happy path well, but consider edge cases.The test validates correct emission of
SendVassalSupportIntentEventwhen vassals are enabled and localStorage has a value. Consider adding tests for:
- When
vassalsEnabled()returnsfalse(no emission expected).- When
localStoragehas nosettings.vassalSupportRatiokey (default behavior).tests/BuildingOnVassal.test.ts (1)
42-47: Assertion logic correct but relies on internal API.The
validStructureSpawnTilesaccess throughanycast suggests this method may not be part of the public interface. If this is intentional test-only access, consider adding a comment explaining why direct API access is needed.tests/nukes/NukeHierarchyTargeting.test.ts (1)
93-100: Assertions correctly validate hierarchy-aware nuke targeting.The test verifies expected behavior:
- Can target temp allies (non-permanent alliance)
- Can target descendants (nukes flow down hierarchy)
- Cannot target siblings or overlords (protected by hierarchy)
The
.not.toBe(false)pattern works becausecanBuildreturnsTileRef | false, but consider.toBeTruthy()for clearer intent.🔎 Clearer assertion style
- expect(attacker.canBuild(UnitType.AtomBomb, tempAllyTile)).not.toBe( - false, - ); - expect(attacker.canBuild(UnitType.AtomBomb, descendantTile)).not.toBe( - false, - ); + expect(attacker.canBuild(UnitType.AtomBomb, tempAllyTile)).toBeTruthy(); + expect(attacker.canBuild(UnitType.AtomBomb, descendantTile)).toBeTruthy();src/core/GameRunner.ts (1)
213-226: Vassal interaction flags have correct eligibility conditions.The conditions for
canSurrenderandcanOfferVassalproperly check:
- Player type restrictions (no bots)
- Existing hierarchy constraints
- Self-targeting prevention
- Team membership
The repeated
(other as Player)casts are verbose but safe sinceother.isPlayer()is checked first. Consider extracting to a local variable if this pattern expands.🔎 Reduce cast repetition
if (tile !== null && this.game.hasOwner(tile)) { const other = this.game.owner(tile) as Player; + const otherPlayer = other.isPlayer() ? other : null; const enforcedAlliance = player.sharesHierarchy(other); actions.interaction = { // ... existing fields ... canSurrender: - other.isPlayer() && + otherPlayer !== null && player.overlord() === null && - !player.isVassalOf(other as Player) && - !player.isOverlordOf(other as Player) && - player !== (other as Player) && - (other as Player).type() !== PlayerType.Bot, + !player.isVassalOf(otherPlayer) && + !player.isOverlordOf(otherPlayer) && + player !== otherPlayer && + otherPlayer.type() !== PlayerType.Bot, // similar for canOfferVassal... };src/core/execution/TradeShipExecution.ts (1)
143-155: Helper function for effective ownership is clear.The
effectiveOwnerhelper correctly determines who should receive gold based on tile ownership and friendliness. The logic ensures vassals' territory gold goes to the appropriate party.The
tileOwner.isPlayer && tileOwner.isPlayer()pattern (line 147-148) is defensive but redundant ifisPlayeris always a method on the owner type.🔎 Simplify isPlayer check
const effectiveOwner = (unitOwner: Player, portTile: TileRef): Player => { const tileOwner = this.mg.owner(portTile); if ( tileOwner && - tileOwner.isPlayer && tileOwner.isPlayer() && tileOwner !== unitOwner && unitOwner.isFriendly(tileOwner as Player) ) { return tileOwner as Player; } return unitOwner; };src/client/graphics/layers/PlayerInfoOverlay.ts (1)
262-277: Simplify the redundant method existence checks.Lines 273 and 277 have redundant checks (
player.overlord &&andplayer.vassals &&). Since these are interface methods on PlayerView, they should always exist. The optional call is not needed.🔎 Suggested simplification
- const vassalOf = - showVassal && player.overlord && player.overlord() - ? (player.overlord() as PlayerView) - : null; - const vassals = - showVassal && player.vassals ? (player.vassals() as PlayerView[]) : []; + const vassalOf = showVassal ? (player.overlord() as PlayerView | null) : null; + const vassals = showVassal ? (player.vassals() as PlayerView[]) : [];src/core/execution/VassalSupportExecution.ts (2)
14-20: Avoid theas anycast; use the typed interface directly.Per the relevant code snippets, the
Playerinterface already declaressetVassalSupportRatio(ratio: number): void. The runtime check + cast pattern here suggests distrust in the type system. If the method exists on Player, call it directly without casting. If it might not exist, the interface definition is wrong.🔎 Proposed fix
tick(): void { const clamped = Math.max(0, Math.min(1, this.ratio)); - if (typeof (this.player as any).setVassalSupportRatio === "function") { - (this.player as any).setVassalSupportRatio(clamped); - } + this.player.setVassalSupportRatio(clamped); this.active = false; }
1-2: Combine imports from the same module.Minor style nit: merge the two imports from
"../game/Game"into one statement.🔎 Proposed fix
-import { Execution } from "../game/Game"; -import { Player } from "../game/Game"; +import { Execution, Player } from "../game/Game";src/client/graphics/layers/EventsDisplay.ts (1)
568-615: Consider implementingshouldDeleteto remove stale vassal offers.The alliance request handler (line 518-521) uses
shouldDeleteto auto-remove events when players become allied. Here,shouldDelete: () => falsemeans vassal offer events never auto-dismiss even if the vassal relationship is already established by another means. Consider checking if the requestor is already the recipient's overlord.🔎 Proposed fix
duration: this.game.config().allianceRequestDuration() - 20, - shouldDelete: () => false, + shouldDelete: () => { + // Auto-dismiss if vassal relationship already established + return recipient.overlord()?.smallID() === requestor.smallID(); + }, focusID: update.requestorID,src/core/execution/AttackExecution.ts (1)
148-163: Redundant assignment ofstartTroopson line 163.Line 161 assigns
this.startTroops = actualTroopsinside theif (this.removeTroops)block. Line 163 assigns it again unconditionally. The second assignment is redundant whenremoveTroopsis true, but required when false. Consider restructuring for clarity.🔎 Proposed fix
actualTroops = sendFromSelf + sendFromSupport; - this.startTroops = actualTroops; } this.startTroops = actualTroops;src/core/game/GameUpdates.ts (1)
210-221: Minor: inconsistent field namingacceptvsaccepted.
AllianceRequestReplyUpdateusesaccepted: boolean(line 207), butVassalOfferReplyUpdateusesaccept: boolean. Consider usingacceptedfor consistency across reply types.🔎 Proposed fix
export interface VassalOfferReplyUpdate { type: GameUpdateType.VassalOfferReply; requestorID: number; recipientID: number; - accept: boolean; + accepted: boolean; }src/core/game/VassalageImpl.ts (1)
8-15: Remove unusedmg(Game) field unless it's needed for planned functionality.The
mgparameter is stored but never referenced in any method of this class. All functionality (overlord, vassal, createdAt, goldTributeRatio, troopTributeRatio, setGoldTributeRatio, setTroopTributeRatio) works without it. Either remove it to keep the class lean, or add a comment explaining why it's preserved for future use.Proposed fix
export class VassalageImpl implements MutableVassalage { constructor( - private readonly mg: Game, private readonly overlord_: Player, private readonly vassal_: Player, private readonly createdAt_: Tick, private goldRatio_: number, private troopRatio_: number, ) {}src/core/execution/PlayerExecution.ts (2)
5-5: Unused import.The
isClusterSurroundedByimport does not appear to be used in this file. If it's not needed, remove it to keep imports clean.Proposed fix
-import { isClusterSurroundedBy } from "./utils/surround";
303-332: DuplicaterootOfimplementation.This local
rootOffunction duplicatesrootOffromHierarchyUtils.ts. Use the shared utility to avoid drift and keep code DRY.Proposed fix
Add import at top of file:
import { rootOf } from "../game/HierarchyUtils";Then remove the local function:
- const rootOf = (p: Player): Player => { - let r: Player = p; - while (r.overlord && r.overlord()) { - const o = r.overlord(); - if (!o) break; - r = o; - } - return r; - };src/core/execution/nation/NationAllianceBehavior.ts (1)
346-353:effectiveTroopsis duplicated across multiple files.This same logic appears in
NationMIRVBehavior.tsandGameView.ts. Consider extracting to a shared utility inHierarchyUtils.tsto keep it DRY.src/core/game/GameView.ts (1)
539-559: LocalrootOfhelper could use shared utility.The
rootOfhelper insidesharesHierarchyduplicates logic fromHierarchyUtils.ts. For consistency, consider importing and using the shared version, or at minimum ensure the logic stays in sync.src/client/graphics/vassalHelpers.ts (2)
7-14: Avoidanycasts - use typed union.The function signature and body use
anycasts which lose type safety. Consider using a typed union or function overloads.Proposed approach
type ConfigSource = | Config | { config: () => Config } | null | undefined; export function vassalsEnabledFrom(source: ConfigSource): boolean { if (!source) return false; const cfg = 'config' in source && typeof source.config === 'function' ? source.config() : source as Config; return cfg.vassalsEnabled?.() ?? false; }
24-64: Heavy use ofanycasts reduces type safety.The function has many
anycasts and optional property checks that could be made safer with proper typing. Consider using the actualPlayerViewtype from the params or creating a minimal interface for the required properties.src/core/game/HierarchyUtils.ts (2)
12-18: Guard against infinite loop if hierarchy has a cycle.If the data becomes corrupted with a circular overlord chain,
rootOfwill loop forever. Consider adding cycle detection with a visited set or max iteration limit.Proposed defensive fix
export function rootOf(player: Player): Player { let curr: Player = player; + const visited = new Set<Player>(); while (curr.overlord && curr.overlord()) { + if (visited.has(curr)) { + console.error("Cycle detected in overlord chain"); + return curr; + } + visited.add(curr); curr = curr.overlord() as Player; } return curr; }
38-43: Missing self-check in hierarchyPosition.When
a === b, neitherisAncestorOfcheck returns true, so the function returns"Sibling"(same root). This may be intentional, but documenting or handling the self-case explicitly would be clearer.Make self-case explicit
export function hierarchyPosition(a: Player, b: Player): HierarchyPosition { + if (a === b) return "Sibling"; // Same player shares hierarchy with itself if (isAncestorOf(a, b)) return "Ancestor"; if (isAncestorOf(b, a)) return "Descendant"; if (rootOf(a) === rootOf(b)) return "Sibling"; return "Unrelated"; }src/core/game/GameImpl.ts (2)
262-270: Silent failure when vassal already has a different overlord.If a vassal already has overlord A, calling
vassalize(vassal, overlord_B)silently returns the existing vassalage with overlord A. The caller might not realize the operation did not change anything. Consider logging a warning or returningnullto signal failure.Make the failure explicit
const existing = this.vassalages_.find((v) => v.vassal() === vassal); if (existing) { if (existing.overlord() === overlord) { existing.setGoldTributeRatio(goldRatio); existing.setTroopTributeRatio(troopRatio); return existing; } - // Already has a different overlord; do not allow multiple. - return existing; + // Already has a different overlord; cannot change overlord without releasing first. + console.warn( + `Cannot vassalize ${vassal.name()} to ${overlord.name()}: already vassal of ${existing.overlord().name()}` + ); + return null; }
238-245: Unnecessary isPlayer() checks on Player array.The
playersvariable comes fromgroups.values(), which contains onlyPlayerobjects (see line 203). TheisPlayer()check andas Playercast are redundant here.Simplify the loop
// Clear embargoes inside the same hierarchy for (const players of groups.values()) { for (const p of players) { for (const q of players) { - if (p === q || !p.isPlayer() || !q.isPlayer()) continue; - if (p.hasEmbargoAgainst(q as Player)) p.stopEmbargo(q as Player); + if (p === q) continue; + if (p.hasEmbargoAgainst(q)) p.stopEmbargo(q); } } }
| const troopRatio = this.target.troops() / this.requestor.troops(); | ||
| const tileRatio = this.target.troops() / this.requestor.troops(); | ||
| const massivelyOutgunned = troopRatio < 0.1 && tileRatio < 0.1; |
There was a problem hiding this comment.
Bug: tileRatio uses troops instead of tiles.
Line 60 calculates tileRatio using troops() instead of numTilesOwned(). This makes tileRatio identical to troopRatio, breaking the intended check.
Proposed fix
const troopRatio = this.target.troops() / this.requestor.troops();
- const tileRatio = this.target.troops() / this.requestor.troops();
+ const tileRatio = this.target.numTilesOwned() / this.requestor.numTilesOwned();
const massivelyOutgunned = troopRatio < 0.1 && tileRatio < 0.1;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const troopRatio = this.target.troops() / this.requestor.troops(); | |
| const tileRatio = this.target.troops() / this.requestor.troops(); | |
| const massivelyOutgunned = troopRatio < 0.1 && tileRatio < 0.1; | |
| const troopRatio = this.target.troops() / this.requestor.troops(); | |
| const tileRatio = this.target.numTilesOwned() / this.requestor.numTilesOwned(); | |
| const massivelyOutgunned = troopRatio < 0.1 && tileRatio < 0.1; |
🤖 Prompt for AI Agents
In src/core/execution/VassalOfferExecution.ts around lines 59 to 61 the
tileRatio is incorrectly computed using this.target.troops() /
this.requestor.troops(), making it identical to troopRatio; change tileRatio to
use this.target.numTilesOwned() / this.requestor.numTilesOwned() instead, and
add a safe-guard for division by zero (return 0 or handle when
requestor.numTilesOwned() is 0) so massivelyOutgunned uses the correct
tile-to-tile ratio.
github-project-automation
bot
moved this to Development
in OpenFront Release Management
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/game/PlayerImpl.ts (1)
1238-1299: Eliminate duplication betweennukeSpawnandnukeSpawnWithReason.Both methods contain identical hierarchy checking and silo selection logic. Refactor
nukeSpawnto delegate:🔎 Proposed refactor
nukeSpawn(tile: TileRef): TileRef | false { - const owner = this.mg.owner(tile); - if (owner.isPlayer()) { - const other = owner as Player; - if (other !== this) { - if (this.isOnSameTeam(other)) return false; // just no - - // Allow nuking down the hierarchy (any depth), but not up or sideways. - const relation = hierarchyPosition(this, other); - if (relation === "Descendant") return false; // vassal cannot nuke overlord chain - if (relation === "Sibling") return false; // siblings under same overlord - } - - // otherwise, nuking self, temp ally, vassal, or hostile/neutral, all ok - // or non-owned tile in the implicit else - } - - // only get missilesilos that are not on cooldown and not under construction - const spawns = this.units(UnitType.MissileSilo) - .filter((silo) => { - return !silo.isInCooldown() && !silo.isUnderConstruction(); - }) - .sort(distSortUnit(this.mg, tile)); - if (spawns.length === 0) { - return false; - } - return spawns[0].tile(); + return this.nukeSpawnWithReason(tile).spawn; }
♻️ Duplicate comments (1)
src/core/execution/nation/NationMIRVBehavior.ts (1)
284-291:effectiveTroopsduplicated across multiple files.This implementation matches previous review feedback noting duplication in
NationMIRVBehavior,NationAllianceBehavior, andGameView. Extract toHierarchyUtils.tsas a shared utility.
🧹 Nitpick comments (12)
tests/BuildingOnVassal.test.ts (4)
43-46: Consider testing through public API rather than casting toany.The
(overlord as any)casts to accessvalidStructureSpawnTilessuggest the test depends on private implementation details. This makes the test fragile to refactoring. Consider either:
- Making
validStructureSpawnTilesa testable part of the public API, or- Testing the actual behavior (attempting to spawn a structure) rather than inspecting the valid tiles list.
Testing concrete behavior (spawn succeeds on vassal territory, fails on neutral) would be more robust than checking internal state.
Also applies to: 50-53
20-28: Add assertion that 2 land tiles were found.If the test map has fewer than 2 land tiles, the destructuring on line 28 will silently assign
undefined, causing confusing failures later. Add a guard:🔎 Suggested improvement
}); const [vassalTile, neutralTile] = landTiles; + expect(landTiles.length).toBeGreaterThanOrEqual(2); vassal.conquer(vassalTile);
33-35: Consider adding a safety bound to the phase advancement loop.The unbounded
whileloop could hang if the game state becomes corrupted or spawn phase logic has a bug. Adding a max iteration counter would prevent test timeouts:🔎 Suggested improvement
- while (game.inSpawnPhase()) { + let maxTicks = 1000; + while (game.inSpawnPhase() && maxTicks-- > 0) { game.executeNextTick(); } + expect(maxTicks).toBeGreaterThan(0);
31-31: TheaddGoldcall may be unnecessary withinfiniteGold: true.Since the game config on line 8 sets
infiniteGold: true, this explicit gold grant might be redundant. Consider removing it unless there's a specific reason (e.g., testing gold-related logic or working around initialization timing).src/client/graphics/layers/ControlPanel.ts (2)
318-318: Vassal support slider uses attack ratio CSS class name.Line 318 applies class
"attackRatio"to the vassal support slider, which appears to be a copy-paste from the attack ratio slider above. Consider using a more generic class name (like"troopRatioSlider") or a specific one for this slider to keep styling intentions clear.🔎 Suggested fix
- class="absolute left-0 right-0 top-2 m-0 h-4 cursor-pointer attackRatio" + class="absolute left-0 right-0 top-2 m-0 h-4 cursor-pointer vassalSupportRatio"Then add corresponding CSS for the thumb (after line 200):
+ .vassalSupportRatio::-webkit-slider-thumb { + border-color: rgb(250 204 21); + } + .vassalSupportRatio::-moz-range-thumb { + border-color: rgb(250 204 21); + }
332-341: Lifecycle hook provides additional initialization path.The
updated()hook ensures vassal support is emitted when thegameproperty changes, which complements the paths ininit()andtick(). ThesentSupportInitflag prevents duplicate emissions across all three code paths. The logic is sound, though having three initialization paths adds some complexity—consider consolidating if you refactor later.src/core/execution/VassalOfferExecution.ts (1)
62-64: Consider guards for division by zero.If
requestorhas zero troops or zero tiles,troopRatioortileRatiowill beInfinity. While unlikely in practice, adding a guard or early return would make the code more defensive.Suggested defensive guard
+ // Guard against division by zero + if (this.requestor.troops() === 0 || this.requestor.numTilesOwned() === 0) { + return false; + } + const troopRatio = this.target.troops() / this.requestor.troops(); const tileRatio = this.target.numTilesOwned() / this.requestor.numTilesOwned();src/core/game/PlayerImpl.ts (5)
107-107: Consider encapsulatinglastVassalSupportUseTickwith a getter.This field is declared public, which exposes mutable state. If external code only needs read access, add a getter and make the field private:
private _lastVassalSupportUseTick: number = -1; lastVassalSupportUseTick(): number { return this._lastVassalSupportUseTick; }Then update assignment sites to use
this._lastVassalSupportUseTick = ....
129-136: Simplify boolean expression.The IIFE can be simplified by returning early:
🔎 Proposed simplification
- const isOppositeTeamVassal = (() => { - const o = this.overlord(); - if (!o) return false; - const myTeam = this.team(); - const oTeam = o.team(); - if (myTeam === null || oTeam === null) return false; - return myTeam !== oTeam; - })(); + const o = this.overlord(); + const isOppositeTeamVassal = + o !== null && + this.team() !== null && + o.team() !== null && + this.team() !== o.team();
549-561: Reduce duplication by reusingoverlord().The
vassalTribute()method duplicates the find logic fromoverlord():🔎 Proposed refactor
vassalTribute(): | { goldRatio: number; troopRatio: number; } | null { - const rel = this.mg.vassalages_.find((v) => v.vassal() === this); - if (!rel) return null; + const o = this.overlord(); + if (!o) return null; + const rel = this.mg.vassalages_.find((v) => v.vassal() === this)!; return { goldRatio: rel.goldTributeRatio(), troopRatio: rel.troopTributeRatio(), }; }
322-323: Simplify overlord eligibility check.The condition
overlord && this.canUseTerritoryOf(overlord)is redundant. Ifoverlordexists,this.canUseTerritoryOf(overlord)will returntruebecausethis.isVassalOf(overlord)is true (line 355-356 incanUseTerritoryOf). Simplify:🔎 Proposed simplification
const partners: Player[] = []; const overlord = this.overlord(); -if (overlord && this.canUseTerritoryOf(overlord)) { +if (overlord) { partners.push(overlord); }
327-331: Type assertion bypasses encapsulation.Casting to
PlayerImplto access private_borderTilesbreaks encapsulation. Consider adding a protected or package-private getter:🔎 Proposed refactor
Add to the Player interface or create an internal interface:
// In Player interface or a separate internal interface borderTilesInternal(): ReadonlySet<TileRef>;Then in PlayerImpl:
borderTilesInternal(): ReadonlySet<TileRef> { return this._borderTiles; }Update the code:
for (const p of partners) { if ( this.sharesBorderAcrossTiles( - (p as PlayerImpl)._borderTiles, + p.borderTilesInternal(), other, ) ) {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
resources/lang/en.jsonsrc/client/Utils.tssrc/client/graphics/layers/ControlPanel.tssrc/client/graphics/layers/PlayerActionHandler.tssrc/client/graphics/layers/RadialMenuElements.tssrc/core/execution/VassalOfferExecution.tssrc/core/execution/nation/NationMIRVBehavior.tssrc/core/game/HierarchyUtils.tssrc/core/game/PlayerImpl.tstests/BuildingOnVassal.test.tstests/NationAllianceBehavior.test.tstests/client/graphics/PlayerActionHandler.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/client/graphics/layers/PlayerActionHandler.ts
- src/client/Utils.ts
- src/core/game/HierarchyUtils.ts
🧰 Additional context used
🧠 Learnings (24)
📚 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/nation/NationMIRVBehavior.tstests/BuildingOnVassal.test.tssrc/core/execution/VassalOfferExecution.tssrc/client/graphics/layers/RadialMenuElements.tstests/client/graphics/PlayerActionHandler.test.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/nation/NationMIRVBehavior.tstests/BuildingOnVassal.test.tssrc/core/execution/VassalOfferExecution.tssrc/client/graphics/layers/RadialMenuElements.tssrc/core/game/PlayerImpl.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/nation/NationMIRVBehavior.tssrc/core/execution/VassalOfferExecution.tstests/client/graphics/PlayerActionHandler.test.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/execution/nation/NationMIRVBehavior.tssrc/core/execution/VassalOfferExecution.tstests/NationAllianceBehavior.test.tssrc/core/game/PlayerImpl.ts
📚 Learning: 2025-10-17T19:16:57.699Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:711-723
Timestamp: 2025-10-17T19:16:57.699Z
Learning: In the OpenFrontIO game, MIRVs (Multiple Independently targetable Reentry Vehicles) have an area of effect with a radius of 1500 tiles around the aim point. To maximize efficiency, bots should aim at the center of the target player's territory rather than selecting random tiles. This differs from the initial understanding that MIRVs covered an entire player's territory regardless of aim point.
Applied to files:
src/core/execution/nation/NationMIRVBehavior.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/nation/NationMIRVBehavior.tstests/BuildingOnVassal.test.tssrc/core/execution/VassalOfferExecution.tssrc/client/graphics/layers/ControlPanel.tssrc/client/graphics/layers/RadialMenuElements.tstests/NationAllianceBehavior.test.tssrc/core/game/PlayerImpl.tstests/client/graphics/PlayerActionHandler.test.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/nation/NationMIRVBehavior.tstests/BuildingOnVassal.test.tssrc/core/game/PlayerImpl.ts
📚 Learning: 2025-10-21T20:06:04.823Z
Learnt from: Saphereye
Repo: openfrontio/OpenFrontIO PR: 2233
File: src/client/HostLobbyModal.ts:891-891
Timestamp: 2025-10-21T20:06:04.823Z
Learning: For the HumansVsNations game mode in `src/client/HostLobbyModal.ts` and related files, the implementation strategy is to generate all nations and adjust their strength for balancing, rather than limiting lobby size based on the number of available nations on the map.
Applied to files:
src/core/execution/nation/NationMIRVBehavior.tstests/NationAllianceBehavior.test.ts
📚 Learning: 2025-05-31T18:15:03.445Z
Learnt from: 1brucben
Repo: openfrontio/OpenFrontIO PR: 977
File: src/core/execution/AttackExecution.ts:123-125
Timestamp: 2025-05-31T18:15:03.445Z
Learning: The removeTroops function in PlayerImpl.ts already prevents negative troop counts by using minInt(this._troops, toInt(troops)) to ensure it never removes more troops than available.
Applied to files:
src/core/execution/nation/NationMIRVBehavior.tssrc/core/execution/VassalOfferExecution.tssrc/client/graphics/layers/ControlPanel.tssrc/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/execution/nation/NationMIRVBehavior.tssrc/core/execution/VassalOfferExecution.tssrc/client/graphics/layers/ControlPanel.tssrc/client/graphics/layers/RadialMenuElements.tstests/client/graphics/PlayerActionHandler.test.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/core/execution/nation/NationMIRVBehavior.tssrc/client/graphics/layers/RadialMenuElements.tssrc/core/game/PlayerImpl.ts
📚 Learning: 2025-11-01T00:24:33.860Z
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2161
File: src/core/execution/FakeHumanExecution.ts:670-678
Timestamp: 2025-11-01T00:24:33.860Z
Learning: In OpenFrontIO, PlayerType.Bot entities cannot be in teams and do not have friendliness relationships. Only PlayerType.Human and PlayerType.FakeHuman can participate in teams and alliances. Therefore, when targeting bot-owned tiles, friendliness checks like `owner.isFriendly(this.player)` are unnecessary and meaningless for bots.
Applied to files:
src/core/execution/nation/NationMIRVBehavior.tssrc/core/game/PlayerImpl.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:
tests/BuildingOnVassal.test.ts
📚 Learning: 2025-11-21T22:30:12.246Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2493
File: src/core/game/TrainStation.ts:79-80
Timestamp: 2025-11-21T22:30:12.246Z
Learning: In src/core/game/TrainStation.ts and RailNetworkImpl.ts, the railroad network maintains a system invariant of at most one railroad between any two stations. RailNetworkImpl.connect is private and only called from connectToNearbyStations during initial station connection, which iterates over unique nearby units without duplicates. The Map<TrainStation, Railroad> structure in railroadByNeighbor correctly reflects this 1-to-1 relationship and should not be flagged as potentially allowing silent overwrites.
Applied to files:
tests/BuildingOnVassal.test.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/VassalOfferExecution.ts
📚 Learning: 2025-06-22T05:48:19.241Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 786
File: src/client/TerritoryPatternsModal.ts:337-338
Timestamp: 2025-06-22T05:48:19.241Z
Learning: In src/client/TerritoryPatternsModal.ts, the bit shifting operators (<<) used in coordinate calculations with decoder.getScale() are intentional and should not be changed to multiplication. The user scottanderson confirmed this is functioning as intended.
Applied to files:
src/core/execution/VassalOfferExecution.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/core/execution/VassalOfferExecution.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/client/graphics/layers/ControlPanel.tssrc/client/graphics/layers/RadialMenuElements.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/ControlPanel.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-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/RadialMenuElements.tstests/NationAllianceBehavior.test.tssrc/core/game/PlayerImpl.tstests/client/graphics/PlayerActionHandler.test.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-27T08:12:19.610Z
Learnt from: mokizzz
Repo: openfrontio/OpenFrontIO PR: 1940
File: resources/lang/en.json:763-766
Timestamp: 2025-08-27T08:12:19.610Z
Learning: In OpenFrontIO, some country entries in src/client/data/countries.json may have similar names but different codes (e.g., "Empire of Japan" vs "Empire of Japan1"). Each unique code requires its own translation key in resources/lang/en.json after normalization. Always verify against countries.json before suggesting removal of translation keys.
Applied to files:
resources/lang/en.json
📚 Learning: 2025-06-13T20:53:16.380Z
Learnt from: its-sii
Repo: openfrontio/OpenFrontIO PR: 1168
File: src/core/execution/WinCheckExecution.ts:157-176
Timestamp: 2025-06-13T20:53:16.380Z
Learning: In the alliance system, coalitions for peace voting are formed by taking each player with allies and creating a group of [player, ...player.allies()]. This means players can be in the same coalition even if they're not all directly allied with each other, as long as they share a common ally. For example, if A.allies()=[B], B.allies()=[A,C], and C.allies()=[B], then B, A, and C should be considered one coalition for voting purposes.
Applied to files:
src/core/game/PlayerImpl.ts
🧬 Code graph analysis (5)
tests/BuildingOnVassal.test.ts (5)
src/server/GameManager.ts (1)
game(25-27)tests/util/Setup.ts (2)
setup(23-84)playerInfo(86-88)src/core/game/PlayerImpl.ts (1)
overlord(526-529)src/core/game/VassalageImpl.ts (2)
overlord(17-19)vassal(21-23)src/core/game/GameView.ts (1)
overlord(539-543)
src/core/execution/VassalOfferExecution.ts (4)
src/core/game/Game.ts (4)
Execution(358-363)Player(548-698)Game(700-804)PlayerID(22-22)src/core/Schemas.ts (1)
Player(132-132)src/core/execution/utils/surround.ts (1)
isClusterSurroundedBy(9-41)src/core/game/PlayerImpl.ts (2)
target(706-709)neighbors(359-374)
src/client/graphics/layers/ControlPanel.ts (3)
src/client/graphics/UIState.ts (1)
UIState(1-5)src/client/graphics/vassalHelpers.ts (1)
vassalsEnabledFrom(7-14)src/client/Transport.ts (1)
SendVassalSupportIntentEvent(90-92)
src/client/graphics/layers/RadialMenuElements.ts (7)
src/client/graphics/layers/RadialMenu.ts (1)
TooltipItem(18-21)src/client/graphics/vassalHelpers.ts (2)
vassalsEnabledFrom(7-14)vassalMenuVisibility(24-64)src/core/game/PlayerImpl.ts (2)
target(706-709)isPlayer(376-378)src/core/game/GameImpl.ts (1)
target(748-754)src/core/game/GameView.ts (2)
isPlayer(473-475)PlayerView(185-636)src/client/Utils.ts (1)
translateText(92-147)src/client/LangSelector.ts (1)
translateText(258-278)
tests/client/graphics/PlayerActionHandler.test.ts (2)
src/client/graphics/layers/PlayerActionHandler.ts (1)
PlayerActionHandler(23-233)src/client/Transport.ts (1)
SendSurrenderIntentEvent(50-57)
🪛 ast-grep (0.40.3)
tests/client/graphics/PlayerActionHandler.test.ts
[warning] 36-36: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: document.body.innerHTML = ""
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
🔇 Additional comments (26)
tests/NationAllianceBehavior.test.ts (1)
80-83: LGTM: Mock updated to match new alliance interface.The mock correctly implements the
isEnforced()method required by the updated Alliance interface. Returningfalseensures tests verify non-enforced alliance behavior.src/core/execution/nation/NationMIRVBehavior.ts (6)
10-16: LGTM: Imports support hierarchy-aware MIRV targeting.The new imports enable threat scoring based on hierarchy relationships and proximity calculations.
Also applies to: 21-21
152-163: LGTM: Threat-based counter-MIRV targeting is well-structured.The logic precomputes hierarchy context, prioritizes the highest-threat inbound attacker, and validates positive threat scores before selecting targets.
171-196: LGTM: Victory denial logic properly accounts for hierarchy.The updated logic uses
hierarchyTilesandteamHierarchyTilesto accurately assess victory threats, and consistently applies threat scoring to prioritize targets within teams and individuals.
237-239: LGTM: Filtering excludes same-hierarchy players.The
sharesHierarchycheck correctly prevents MIRV targeting of vassals/overlords within the same hierarchy.
293-313: LGTM: Threat scoring balances troops and proximity effectively.The composite score with diminishing returns for high troop counts (lines 299-302) and proximity-based multipliers (lines 309-311) creates sensible MIRV target prioritization.
315-334: LGTM: Distance calculation handles hierarchy boundaries correctly.The logic efficiently checks border adjacency (line 323) before falling back to shore tile distance, with appropriate
Infinityreturns for unreachable cases.tests/client/graphics/PlayerActionHandler.test.ts (2)
4-38: Test setup looks good; mock matches real constructor.The mock constructor now correctly mirrors
SendSurrenderIntentEventwith all four parameters. The test helpers provide clean fixtures.Regarding the static analysis warning on line 37: assigning
""toinnerHTMLin a test cleanup hook is safe—there's no user input or XSS risk when resetting the DOM between tests.
40-72: Comprehensive test coverage for surrender confirmation flow.Both tests verify the expected behavior:
- Confirm path: modal renders, event emits with correct properties, modal clears
- Cancel path: modal closes without emitting
The assertions on
emitted.requestor,emitted.recipient, and optional ratio properties ensure the event carries the right data.resources/lang/en.json (1)
15-16: Localization keys are well-organized and comprehensive.The new vassal-related keys cover:
- Feature toggle and UI labels
- Build menu error states for hierarchy restrictions
- Player info overlay showing vassal/overlord relationships
- Event notifications for offers and replies
- Confirmation dialog text
Key naming follows existing patterns, and the text is clear for users.
Also applies to: 564-580, 625-629, 664-669, 701-701, 812-837
src/client/graphics/layers/RadialMenuElements.ts (4)
2-7: Interface extension and imports are clean.The new
tooltipItemsFnfield onMenuElementenables dynamic tooltips when elements are disabled, providing better user feedback. The vassal helper functions centralize feature flag and visibility logic.Also applies to: 31-34, 61-61, 95-95
231-312: Surrender element implementation is thorough.The element correctly:
- Shows for any player target (relying on tooltips to explain restrictions)
- Checks
canSurrenderfrom server-provided interaction flags- Provides detailed tooltip feedback for each disabled case
The overlord checks at lines 266 and 274 handle distinct scenarios (already vassal of this target vs. already has a different overlord) in the correct order.
314-409: Offer vassalage element provides clear user feedback.The element:
- Disables when target has an overlord or server denies the action
- Shows only for valid player targets (excluding self)
- Generates detailed tooltip reasons when disabled, mirroring server-side validation rules
The layered checks ensure users understand why vassalage offers aren't available.
818-836: Vassal elements integrate cleanly into the radial menu.The root menu:
- Uses
vassalMenuVisibilityhelper to determine surrender visibility- Conditionally includes both vassal elements based on feature flag and context
- Positions them in the non-owned-territory menu branch alongside ally actions
The null-filtering pattern keeps the menu composition logic clean.
src/client/graphics/layers/ControlPanel.ts (6)
2-2: Property and import changes look good.The
@property({ attribute: false })decorator correctly prevents HTML attribute binding for the game object reference, and making it nullable with explicit initialization is the right approach. The new vassal support state and imports are properly structured.Also applies to: 12-13, 17-18, 25-28
54-62: Initialization guard correctly addresses the past review concern.The check at line 60 ensures
this.gameis non-null before callingonVassalSupportChange, which prevents the early-return issue flagged in the previous review. The logic now guaranteessentSupportInitgets set properly.
90-91: Null-safe pattern with local variable is clean.Using a local
gamevariable with an early return makes the null handling explicit and reduces repeated property access. The fallback initialization at lines 103-106 ensures vassal support is emitted once a player exists, even ifinit()ran beforegamewas set.Also applies to: 103-106
134-146: Handler guards and emission logic are sound.The method correctly guards against null game, disabled vassals, and missing player before emitting the event. Setting
sentSupportInitafter a successful emit ensures initialization is only marked complete when the event actually fires.
293-294: Verify intentional use oftroops()vseffectiveTroops().The vassal support calculation uses
troops()directly (line 293), while the attack ratio calculation above (lines 245-247) useseffectiveTroops?.()with a fallback. Confirm this difference is intentional—whether vassal support should be based on raw troops or effective troops.
245-247: No action needed. TheeffectiveTroops()method is properly defined as a public method onPlayerView(line 505, src/core/game/GameView.ts). It calculates troops available for attacks by including vassal support when the overlord's support pool is ready. The optional chaining pattern (?.()) is consistent across the codebase and appropriately handles the null case frommyPlayer(). The fallback totroops()works as expected for display values.src/core/execution/VassalOfferExecution.ts (4)
25-48: Initialization logic looks solid.The guard conditions properly validate the feature flag, player existence, and player types. The expiry calculation is straightforward.
71-101: Alliance and surrounded checks look good.The long-alliance heuristic and surrounded logic using
isClusterSurroundedByare well-structured. TheallianceTimeLeftcalculation now has correct parentheses (fixed from previous review).
121-139: Fallout pressure logic is well-implemented.The early-break optimization at line 133 and the conditional check (only runs if not already under attack) make this efficient. The threshold constant has a clear comment explaining the approximation.
142-184: Tick execution flow is correct.The decision paths are now properly structured with no unreachable code (previous issue fixed). The human request handling cleanly sends an update and deactivates, while the bot acceptance path vassalizes and displays messages appropriately.
src/core/game/PlayerImpl.ts (2)
1369-1377: Hierarchy collection logic stops after one overlord level; verify if multi-level vassalage is supported.The
collectHierarchyfunction recursively collects all vassals (downward), but only collects the immediate overlord (upward). For multi-level hierarchies—such as Player A → B → C—when C callsvalidStructureSpawnTiles, it collects:
- C and C's vassals
- B (C's overlord) and B's vassals
But NOT A (B's overlord).
The function calls
collectHierarchy(this.overlord() as Player)without recursively checking the overlord's overlord. If the game design allows multi-level vassalage chains, this will exclude ancestor territories. Clarify whether:
- The game only permits single-level vassalage (then code is correct).
- Multi-level vassalage is possible (then function needs to recursively collect ancestors).
1156-1236: All i18n keys for build reasons are properly defined in English locale.The
canBuildWithReasonmethod returns 15 reason strings (e.g.,"build_menu.not_alive","build_menu.not_enough_money"), and all are present in./resources/lang/en.json. The return pattern is intentional: thereasonproperty is included only for failure cases (whencanBuildisfalse); success cases return{ canBuild: targetTile }without a reason.
| const underAttack = this.target.incomingAttacks().some((a) => { | ||
| const attacker = a.attacker(); | ||
| if (attacker === this.requestor) return true; | ||
| // Treat attacks from the requestor's direct vassals as pressure too | ||
| // (does not recurse through deeper hierarchy). | ||
| return ( | ||
| attacker.overlord && | ||
| attacker.overlord() && | ||
| attacker.overlord()!.smallID() === this.requestor.smallID() | ||
| ); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Simplify the overlord check with optional chaining.
Lines 114-116 contain a redundant and confusing pattern:
- Line 114 checks
attacker.overlord(the method reference), which is always truthy - Lines 115-116 call
overlord()twice
This should use idiomatic optional chaining instead.
Proposed refactor
const underAttack = this.target.incomingAttacks().some((a) => {
const attacker = a.attacker();
if (attacker === this.requestor) return true;
// Treat attacks from the requestor's direct vassals as pressure too
// (does not recurse through deeper hierarchy).
- return (
- attacker.overlord &&
- attacker.overlord() &&
- attacker.overlord()!.smallID() === this.requestor.smallID()
- );
+ return attacker.overlord()?.smallID() === this.requestor.smallID();
});🤖 Prompt for AI Agents
In src/core/execution/VassalOfferExecution.ts around lines 108 to 118, the
overlord check uses a redundant pattern (checking attacker.overlord then calling
attacker.overlord() twice) — replace it with idiomatic optional chaining: call
attacker.overlord() once via optional chaining to access smallID (e.g.
attacker.overlord()?.smallID()) and compare that to this.requestor.smallID(),
removing the initial truthy reference check and the duplicated call.
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/execution/TradeShipExecution.ts (1)
183-200: Messages and stats still reference port owners, not effective owners.Gold distribution now uses
srcOwneranddstOwner, but the display messages (lines 186, 192) and stats (line 200) still referencesrcPort.owner()and_dstPort.owner(). This mismatch may confuse players when the effective owner differs from the port owner.Consider updating messages and stats to reflect the actual gold recipients.
Proposed fix
this.mg.displayMessage( - `Received ${renderNumber(gold)} gold from trade with ${this.srcPort.owner().displayName()}`, + `Received ${renderNumber(gold)} gold from trade with ${srcOwner.displayName()}`, MessageType.RECEIVED_GOLD_FROM_TRADE, - this._dstPort.owner().id(), + dstOwner.id(), gold, ); this.mg.displayMessage( - `Received ${renderNumber(gold)} gold from trade with ${this._dstPort.owner().displayName()}`, + `Received ${renderNumber(gold)} gold from trade with ${dstOwner.displayName()}`, MessageType.RECEIVED_GOLD_FROM_TRADE, - this.srcPort.owner().id(), + srcOwner.id(), gold, ); // Record stats this.mg .stats() - .boatArriveTrade(this.srcPort.owner(), this._dstPort.owner(), gold); + .boatArriveTrade(srcOwner, dstOwner, gold);
🧹 Nitpick comments (5)
src/client/graphics/layers/PlayerInfoOverlay.ts (1)
262-273: Redundantoverlord()call.
player.overlord()is called twice—once at line 262 and again at line 271. Consider reusing the first result to avoid the repeated lookup.Proposed fix
const overlord = player.overlord(); const supportTroops = overlord !== null && this.game.config().vassalsEnabled() ? Math.floor(overlord.troops() * overlord.vassalSupportRatio()) : 0; const effectiveTroops = player.effectiveTroops?.() ?? supportTroops + player.troops(); const showVassal = this.game.config().vassalsEnabled(); - const vassalOf = showVassal - ? (player.overlord() as PlayerView | null) - : null; + const vassalOf = showVassal ? (overlord as PlayerView | null) : null; const vassals = showVassal ? (player.vassals() as PlayerView[]) : [];src/core/game/TrainStation.ts (1)
52-81: Clean refactoring that consolidates revenue logic.The extraction of common payout logic into
payoutTrainStopeliminates duplication betweenCityStopHandlerandPortStopHandler. The parameters are passed correctly and the code is clearer.src/core/execution/PlayerExecution.ts (1)
87-114: Vassal tribute flow is well-structured.The tribute calculation correctly computes troop and gold ratios, ensures the overlord is alive before transferring, and records stats. One minor note:
goldGaincould theoretically go negative ifgoldTribute > goldFromWorkers, though tribute ratios should be clamped to[0, 1].Consider adding a safety clamp:
goldGain = goldFromWorkers - goldTribute; if (goldGain < 0n) goldGain = 0n;src/core/game/HierarchyUtils.ts (1)
12-24: Cycle detection inrootOfis a good safeguard.The visited set prevents infinite loops if hierarchy data becomes corrupted. The error log helps debugging.
Minor: Line 15's
curr.overlord && curr.overlord()is redundant sinceoverlord()returningnullis falsy. Could simplify towhile (curr.overlord?.()).Proposed simplification
export function rootOf(player: Player): Player { let curr: Player = player; const visited = new Set<Player>(); - while (curr.overlord && curr.overlord()) { + while (curr.overlord?.()) { if (visited.has(curr)) { console.error("Cycle detected in overlord chain"); return curr; } visited.add(curr); curr = curr.overlord() as Player; } return curr; }src/core/game/GameImpl.ts (1)
197-249:refreshHierarchyAlliancesrebuilds hierarchy bonds.The method groups players by root, ensures permanent alliances within each hierarchy, expires cross-hierarchy enforced alliances, and clears intra-hierarchy embargoes. Solid approach.
One concern: removing an existing alliance (line 215) then immediately creating a new one (lines 217-225) could cause brief inconsistency. The
announce: falsemitigates client spam, but consider checking if the existing alliance is already permanent before recreating.Avoid unnecessary recreation of permanent alliances
if (a.allianceWith(b) !== null) { - // Quietly remove the temporary alliance so we can rebuild a permanent one - // without spamming an "alliance expired" event to clients. - this.removeAlliance(a.allianceWith(b)!, false); + const existing = a.allianceWith(b)!; + if (existing.isEnforced()) { + // Already permanent, skip + continue; + } + // Quietly remove the temporary alliance + this.removeAlliance(existing, false); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/client/graphics/UIState.tssrc/client/graphics/layers/EventsDisplay.tssrc/client/graphics/layers/PlayerActionHandler.tssrc/client/graphics/layers/PlayerInfoOverlay.tssrc/core/execution/AttackExecution.tssrc/core/execution/PlayerExecution.tssrc/core/execution/SurrenderExecution.tssrc/core/execution/TradeShipExecution.tssrc/core/execution/VassalSupportExecution.tssrc/core/execution/nation/NationAllianceBehavior.tssrc/core/execution/nation/NationMIRVBehavior.tssrc/core/game/GameImpl.tssrc/core/game/HierarchyUtils.tssrc/core/game/TrainStation.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/client/graphics/UIState.ts
- src/core/execution/SurrenderExecution.ts
🧰 Additional context used
🧠 Learnings (22)
📚 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/PlayerExecution.tssrc/core/execution/TradeShipExecution.tssrc/client/graphics/layers/PlayerActionHandler.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/PlayerExecution.tssrc/client/graphics/layers/PlayerInfoOverlay.tssrc/core/execution/VassalSupportExecution.tssrc/core/game/TrainStation.tssrc/core/execution/TradeShipExecution.tssrc/core/execution/nation/NationMIRVBehavior.tssrc/core/game/HierarchyUtils.tssrc/client/graphics/layers/PlayerActionHandler.tssrc/core/execution/nation/NationAllianceBehavior.tssrc/core/game/GameImpl.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/execution/PlayerExecution.tssrc/client/graphics/layers/PlayerInfoOverlay.tssrc/core/execution/VassalSupportExecution.tssrc/core/execution/nation/NationMIRVBehavior.tssrc/core/execution/nation/NationAllianceBehavior.tssrc/core/execution/AttackExecution.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/execution/PlayerExecution.tssrc/client/graphics/layers/EventsDisplay.tssrc/client/graphics/layers/PlayerInfoOverlay.tssrc/core/execution/TradeShipExecution.tssrc/client/graphics/layers/PlayerActionHandler.tssrc/core/game/GameImpl.ts
📚 Learning: 2025-06-05T02:34:45.899Z
Learnt from: Egraveline
Repo: openfrontio/OpenFrontIO PR: 1012
File: src/core/execution/UpgradeStructureExecution.ts:49-54
Timestamp: 2025-06-05T02:34:45.899Z
Learning: In the upgrade system, gold deduction for structure upgrades is handled internally by the `upgradeUnit` method in PlayerImpl, not in the UpgradeStructureExecution class. The UpgradeStructureExecution only needs to check if the player has sufficient gold before calling `upgradeUnit`.
Applied to files:
src/core/execution/PlayerExecution.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/PlayerExecution.tssrc/core/execution/VassalSupportExecution.tssrc/core/execution/nation/NationMIRVBehavior.tssrc/client/graphics/layers/PlayerActionHandler.tssrc/core/execution/nation/NationAllianceBehavior.tssrc/core/execution/AttackExecution.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/core/execution/PlayerExecution.ts
📚 Learning: 2025-05-31T18:15:03.445Z
Learnt from: 1brucben
Repo: openfrontio/OpenFrontIO PR: 977
File: src/core/execution/AttackExecution.ts:123-125
Timestamp: 2025-05-31T18:15:03.445Z
Learning: The removeTroops function in PlayerImpl.ts already prevents negative troop counts by using minInt(this._troops, toInt(troops)) to ensure it never removes more troops than available.
Applied to files:
src/core/execution/PlayerExecution.tssrc/client/graphics/layers/PlayerInfoOverlay.tssrc/core/execution/nation/NationMIRVBehavior.tssrc/client/graphics/layers/PlayerActionHandler.tssrc/core/execution/nation/NationAllianceBehavior.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/client/graphics/layers/PlayerInfoOverlay.tssrc/core/execution/nation/NationMIRVBehavior.tssrc/client/graphics/layers/PlayerActionHandler.tssrc/core/execution/nation/NationAllianceBehavior.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/VassalSupportExecution.tssrc/core/execution/nation/NationMIRVBehavior.tssrc/client/graphics/layers/PlayerActionHandler.tssrc/core/execution/AttackExecution.ts
📚 Learning: 2025-08-28T22:47:31.406Z
Learnt from: BrewedCoffee
Repo: openfrontio/OpenFrontIO PR: 1957
File: tests/core/executions/PlayerExecution.test.ts:16-39
Timestamp: 2025-08-28T22:47:31.406Z
Learning: In test files, PlayerExecution instances must be manually registered with game.addExecution() because the setup utility doesn't automatically register SpawnExecution, which would normally handle this during the spawn phase in a real game.
Applied to files:
src/core/execution/VassalSupportExecution.ts
📚 Learning: 2025-11-21T22:30:12.246Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2493
File: src/core/game/TrainStation.ts:79-80
Timestamp: 2025-11-21T22:30:12.246Z
Learning: In src/core/game/TrainStation.ts and RailNetworkImpl.ts, the railroad network maintains a system invariant of at most one railroad between any two stations. RailNetworkImpl.connect is private and only called from connectToNearbyStations during initial station connection, which iterates over unique nearby units without duplicates. The Map<TrainStation, Railroad> structure in railroadByNeighbor correctly reflects this 1-to-1 relationship and should not be flagged as potentially allowing silent overwrites.
Applied to files:
src/core/game/TrainStation.tssrc/core/game/GameImpl.ts
📚 Learning: 2025-11-08T16:57:19.090Z
Learnt from: mepoohsta
Repo: openfrontio/OpenFrontIO PR: 2413
File: src/core/game/RailNetworkImpl.ts:298-311
Timestamp: 2025-11-08T16:57:19.090Z
Learning: In src/core/game/RailNetworkImpl.ts, the getDestinationWeight method calculates route profitability (not just length) for train destination selection. The heavy relationship bonuses (ally +50, team/other +25, self +10) compared to the base +1 per station are intentional, as profitability considers both route length and player relationships. Longer routes are usually more profitable, but shorter routes to allies can be more valuable.
Applied to files:
src/core/game/TrainStation.ts
📚 Learning: 2025-11-06T00:56:21.251Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 2396
File: src/core/execution/TransportShipExecution.ts:180-189
Timestamp: 2025-11-06T00:56:21.251Z
Learning: In OpenFrontIO, when a player conquers a disconnected player, transport ships and warships are only transferred to the conqueror if they are on the same team. If an enemy (non-teammate) conquers a disconnected player, the ships are deleted (described as "boated into a sea mine"), not captured.
Applied to files:
src/core/execution/TradeShipExecution.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/nation/NationMIRVBehavior.tssrc/client/graphics/layers/PlayerActionHandler.ts
📚 Learning: 2025-10-17T19:16:57.699Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:711-723
Timestamp: 2025-10-17T19:16:57.699Z
Learning: In the OpenFrontIO game, MIRVs (Multiple Independently targetable Reentry Vehicles) have an area of effect with a radius of 1500 tiles around the aim point. To maximize efficiency, bots should aim at the center of the target player's territory rather than selecting random tiles. This differs from the initial understanding that MIRVs covered an entire player's territory regardless of aim point.
Applied to files:
src/core/execution/nation/NationMIRVBehavior.ts
📚 Learning: 2025-10-21T20:06:04.823Z
Learnt from: Saphereye
Repo: openfrontio/OpenFrontIO PR: 2233
File: src/client/HostLobbyModal.ts:891-891
Timestamp: 2025-10-21T20:06:04.823Z
Learning: For the HumansVsNations game mode in `src/client/HostLobbyModal.ts` and related files, the implementation strategy is to generate all nations and adjust their strength for balancing, rather than limiting lobby size based on the number of available nations on the map.
Applied to files:
src/core/execution/nation/NationMIRVBehavior.tssrc/core/game/HierarchyUtils.tssrc/core/execution/nation/NationAllianceBehavior.tssrc/core/game/GameImpl.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/nation/NationMIRVBehavior.ts
📚 Learning: 2025-11-01T00:24:33.860Z
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2161
File: src/core/execution/FakeHumanExecution.ts:670-678
Timestamp: 2025-11-01T00:24:33.860Z
Learning: In OpenFrontIO, PlayerType.Bot entities cannot be in teams and do not have friendliness relationships. Only PlayerType.Human and PlayerType.FakeHuman can participate in teams and alliances. Therefore, when targeting bot-owned tiles, friendliness checks like `owner.isFriendly(this.player)` are unnecessary and meaningless for bots.
Applied to files:
src/core/execution/nation/NationMIRVBehavior.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/PlayerActionHandler.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/client/graphics/layers/PlayerActionHandler.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/GameImpl.ts
🧬 Code graph analysis (7)
src/client/graphics/layers/PlayerInfoOverlay.ts (7)
src/core/game/PlayerImpl.ts (2)
overlord(520-523)vassals(525-529)src/core/game/VassalageImpl.ts (1)
overlord(17-19)src/core/game/GameView.ts (5)
overlord(539-543)player(786-792)effectiveTroops(505-518)PlayerView(185-636)vassals(545-549)src/core/game/GameImpl.ts (1)
player(606-612)src/core/game/HierarchyUtils.ts (1)
effectiveTroops(66-73)src/core/game/GameUpdates.ts (1)
AllianceView(189-195)src/client/Utils.ts (1)
renderTroops(15-17)
src/core/game/TrainStation.ts (3)
src/core/game/Game.ts (2)
Game(700-804)Player(548-698)src/core/Schemas.ts (1)
Player(132-132)src/core/game/GameMap.ts (1)
TileRef(3-3)
src/core/execution/nation/NationMIRVBehavior.ts (3)
src/core/game/HierarchyUtils.ts (5)
hierarchyPlayers(96-98)hierarchyShoreTiles(100-108)teamHierarchyTiles(85-94)hierarchyTiles(55-63)sharesHierarchy(50-52)src/core/game/Game.ts (1)
Player(548-698)src/core/game/GameMap.ts (1)
TileRef(3-3)
src/core/game/HierarchyUtils.ts (2)
src/core/game/Game.ts (3)
Player(548-698)Game(700-804)Team(51-51)src/core/game/GameMap.ts (1)
TileRef(3-3)
src/core/execution/nation/NationAllianceBehavior.ts (2)
src/core/execution/VassalOfferExecution.ts (1)
VassalOfferExecution(15-195)src/core/game/HierarchyUtils.ts (3)
sharesHierarchy(50-52)rootOf(12-24)effectiveTroops(66-73)
src/core/execution/AttackExecution.ts (3)
src/core/game/PlayerImpl.ts (1)
overlord(520-523)src/core/game/VassalageImpl.ts (1)
overlord(17-19)src/core/game/GameView.ts (1)
overlord(539-543)
src/core/game/GameImpl.ts (4)
src/core/game/VassalageImpl.ts (3)
VassalageImpl(7-44)vassal(21-23)overlord(17-19)src/core/game/HierarchyUtils.ts (2)
rootOf(12-24)sharesHierarchy(50-52)src/core/game/AllianceImpl.ts (2)
AllianceImpl(3-89)other(25-30)src/core/game/PlayerImpl.ts (2)
sharesHierarchy(570-572)overlord(520-523)
🔇 Additional comments (37)
src/client/graphics/layers/PlayerInfoOverlay.ts (3)
286-304: LGTM!The
currentAlliancevariable and the permanent alliance check are well structured. UsingNumber.MAX_SAFE_INTEGERas the sentinel for permanent vassalage alliances is a clear pattern.
352-365: LGTM!The vassal and overlord relationship labels are properly styled and use
translateTextwith interpolation for localization.
388-408: LGTM!The overlord support and attack capacity display is well implemented with proper conditional checks and consistent styling.
src/client/graphics/layers/PlayerActionHandler.ts (5)
29-129: Well-structured confirmation modal with proper cleanup.The
showInlineConfirmmethod builds DOM elements programmatically withtextContent, which is safe from XSS. The keyboard handling and cleanup are properly implemented. Good work addressing the previous review feedback.One small note: pressing Enter always confirms, even if the cancel button is focused. This is typically fine for confirmation dialogs, but worth being aware of.
138-146: LGTM!The optional chaining for
effectiveTroopswith fallback totroops()handles backward compatibility well. Both attack handlers use the same pattern consistently.
148-163: Consistent with handleAttack.Same
effectiveTroopspattern applied correctly.
184-206: LGTM!The
handleSurrendermethod properly guards withvassalsEnabled()and shows a confirmation dialog before emitting the surrender intent. This prevents accidental surrenders and addresses the previous review feedback.
238-242: LGTM!The
handleForceVassalmethod now includes thevassalsEnabled()guard as requested in the previous review.src/client/graphics/layers/EventsDisplay.ts (4)
32-34: LGTM!New imports for vassal offer updates are correctly added.
190-191: LGTM!The update map correctly wires the new vassal offer handlers following the existing pattern.
620-642: LGTM!The
onVassalOfferReplyEventhandler correctly displays the offer reply status to the requestor. The pattern matches the existing alliance reply handler.
568-617: LGTM! TheonVassalOfferRequesthandler follows the same pattern asonAllianceRequestEvent. TheshouldDeletecallback correctly checks if the vassalage relationship already exists between the players. Good use ofdisplayName()for readable user-facing text, andMessageType.VASSAL_REQUESTis properly defined and categorized.src/core/game/TrainStation.ts (2)
34-46: Verify the economic impact of double gold generation.When
trainOwner !== effectiveOwner, both players receive the fullgoldBonusamount. This effectively doubles gold generation compared to the previous implementation. While this aligns with the vassalage support mechanics described in the PR, please confirm that the economic impact has been considered and balanced, especially since the PR notes mention that "balancing tweaks likely needed."
16-32: No changes needed—isFriendly()already includes hierarchy relationships.The
isFriendly()method correctly identifies hierarchy relationships through itssharesHierarchy()check on line 969 of PlayerImpl.ts. The code in TrainStation.ts is correct as written and requires no modifications.Likely an incorrect or invalid review comment.
src/core/execution/TradeShipExecution.ts (1)
143-153: Clean helper for hierarchy-aware ownership.The
effectiveOwnerfunction neatly determines which player should receive gold when a port sits on another player's territory. Logic is clear and safe.src/core/execution/VassalSupportExecution.ts (1)
1-25: Simple and clean single-tick execution.The implementation is minimal and correct: clamps ratio to
[0, 1], applies it once, and deactivates. Good use of composition over inheritance with theExecutioninterface.src/core/execution/AttackExecution.ts (3)
131-147: Troop calculation logic is sound.The ratio-based calculation properly re-applies the intended ratio to the currently available pool, preventing overdraw when support is on cooldown. Clamping to
availableNowensures safety.
148-162: Troop removal order is correct but relies on external invariant.The code prioritizes removing troops from overlord support before the vassal's own troops. This depends on
removeTroopsinPlayerImplclamping to available troops (per retrieved learnings). The logic is correct given that invariant.
108-114: AddlastVassalSupportUseTickto thePlayerinterface.The field is declared on
PlayerImplbut missing from thePlayerinterface, forcingAttackExecution.tsto use(overlord as any)to access it. This bypasses type safety. AddlastVassalSupportUseTick: number;to thePlayerinterface insrc/core/game/Game.tsso the property can be accessed directly without casting.⛔ Skipped due to learnings
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.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.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.Learnt from: 1brucben Repo: openfrontio/OpenFrontIO PR: 977 File: src/core/execution/AttackExecution.ts:123-125 Timestamp: 2025-05-31T18:15:03.445Z Learning: The removeTroops function in PlayerImpl.ts already prevents negative troop counts by using minInt(this._troops, toInt(troops)) to ensure it never removes more troops than available.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.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.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.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: 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.src/core/execution/PlayerExecution.ts (2)
48-53: Territorial access check prevents friendly unit capture.Clean addition that respects hierarchy-based territorial access. Units on vassal/overlord territory are no longer incorrectly captured.
303-323: Hierarchy-aware capturing logic is clean.The approach of grouping neighbors by
rootOfand deciding capture based on hierarchy membership is intuitive. Single neighbor → that neighbor captures. Multiple vassals of same hierarchy → root captures. Mixed hierarchies → fallback to original attack-based logic.src/core/execution/nation/NationAllianceBehavior.ts (4)
67-72: Proactive vassal offer before alliance request.Nations now offer vassalage to sufficiently weaker enemies before attempting alliances. This integrates well with the hierarchy system and prevents unnecessary alliance churn.
90-102: Vassals defer alliance decisions to hierarchy.A vassal won't form new alliances unless the overlord (or root) is already allied with the target. This maintains hierarchy integrity and prevents diplomatic fragmentation.
322-328: Enforced alliances cannot be betrayed.The check for
alliance?.isEnforced()correctly prevents breaking vassal-overlord bonds through betrayal. Clean guard clause.
337-371: Well-designed helper functions.
weightedHierarchyTilesapplies a discount factor to vassal territory, giving a more realistic strength estimate.shouldOfferVassalhas sensible criteria: massively outgunned players or long-allied weaker players become vassalage candidates. Both are clean and readable.src/core/execution/nation/NationMIRVBehavior.ts (4)
153-165: Threat-score-based MIRV target selection.Sorting attackers by
threatScoreand only targeting if score is positive is a sound approach. This prioritizes dangerous, nearby enemies over distant weak ones.
172-198: Victory denial uses hierarchy-aware territory counting.Using
teamHierarchyTilesprevents double-counting vassal territory when assessing team victory thresholds. Switching tohierarchyTiles(p)for individual players also ensures consistent hierarchy-aware calculations.
238-239: Exclude hierarchy members from MIRV targeting.Adding
!sharesHierarchy(this.player!, p)ensures nations won't nuke their own vassals or overlords. Clean and necessary.
285-324: Threat score and distance calculations are well-designed.The
threatScorefunction balances troop count (with diminishing returns past 500k) and proximity (border adjacency or shore distance).hierarchyDistanceToEnemycorrectly checks direct borders first, then falls back to shore tile distance. Clear logic throughout.src/core/game/HierarchyUtils.ts (3)
41-52: Hierarchy position and sharing functions are clear.
hierarchyPositioncovers all cases (ancestor, descendant, sibling, unrelated), andsharesHierarchyis a simple wrapper. Note thata === breturns"Sibling"which may be unexpected but is semantically correct (shares root with self).
55-73:hierarchyTilesandeffectiveTroopsare useful utilities.Both functions are recursive/computed correctly. The defensive
typeofcheck forvassalshandles edge cases gracefully.effectiveTroopsincludes overlord support in the calculation.
76-108: Root and team hierarchy utilities are well-structured.
rootPlayerscorrectly identifies players without overlords.teamHierarchyTilesaggregates hierarchy tiles per team via roots to avoid double-counting.hierarchyPlayersandhierarchyShoreTilesleveragesharesHierarchyfor filtering.src/core/game/GameImpl.ts (5)
77-77: Vassalages storage added.Clean addition of
vassalages_: VassalageImpl[] = []to track vassal-overlord relationships.
255-306:vassalizehandles edge cases correctly.The method prevents multiple overlords by returning existing vassalage, clears embargoes, creates permanent alliances, and triggers hierarchy refresh. Good defensive design.
325-331:addUpdatenow accepts arrays.Simple and useful extension for batching updates. Recursive handling is clean.
765-768: Guard against breaking vassal alliances.Prevents diplomatic exploits by blocking alliance breaks between vassals and overlords. The warning log aids debugging.
1119-1124: Conquest cleans up vassalages properly.Removing vassalages involving the conquered player and refreshing hierarchy alliances ensures no orphaned relationships remain. Clean cleanup logic.
2 participants
Description:
Massive feature via openai codex 5.1 locked behind game config option adding vassals to the game enforcing a permanent alliance, allowing 'overlords' to build on vassal territory, anyone in the hierarchy to send transport ships from each other's shores (probably chaos inducing in MP). WinCheck and Teams respect hierarchy, Rankings sum vassal territory under overlords, for Teams I avoided editing the code to make team-switching possible mid-game since it seemed like it's own big change and that complicates things a bit there... Primarily tested in single player but I did check that requests work as expected with multiple browser tabs. Probably needs balancing tweaks.
Honestly, there's no way this is something I can support myself even if I was truly inclined to... but it was a feature I had interest in since I mostly play SP and found myself forcing nations onto islands without silos and surrounding them with warships to try and keep them friendly at the end. Was initially just curious if it was even possible and then somehow it actually turned out to work. Spent a few weeks off and on just working unstaged so minimal commits... honestly didn't expect it to work.
Not sure of any other way to really share this in case there's any interest so I'm creating a PR/draft anyway.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
FreeER