DRAFT Transport ship perf #2295
Conversation
|
|
WalkthroughAdds a Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant WorkerClient
participant Worker
participant GameRunner
participant Player
Client->>WorkerClient: playerInteraction(playerID, x?, y?, transportShipFilter?)
WorkerClient->>Worker: postMessage { type: "player_actions", transportShipFilter }
Worker->>GameRunner: playerActions(playerID, x?, y?, transportShipFilter)
GameRunner->>Player: buildableUnits(tile, transportShipFilter)
alt transportShipFilter == Only
Player-->>GameRunner: BuildableUnit[] (transports only)
else transportShipFilter == Exclude
Player-->>GameRunner: BuildableUnit[] (no transports)
else
Player-->>GameRunner: BuildableUnit[] (all units)
end
GameRunner-->>Worker: PlayerActions
Worker-->>WorkerClient: response
WorkerClient-->>Client: Promise<PlayerActions>
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks❌ Failed checks (2 warnings, 1 inconclusive)
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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/worker/WorkerClient.ts (1)
162-194: Rename parameter to follow TypeScript conventions.The parameter
TransportShipFiltershadows the type name. Use lowercasetransportShipFilterfor the parameter and message property.playerInteraction( playerID: PlayerID, x?: number, y?: number, - TransportShipFilter?: TransportShipFilter, + transportShipFilter?: TransportShipFilter, ): Promise<PlayerActions> { return new Promise((resolve, reject) => { if (!this.isInitialized) { reject(new Error("Worker not initialized")); return; } const messageId = generateID(); this.messageHandlers.set(messageId, (message) => { if ( message.type === "player_actions_result" && message.result !== undefined ) { resolve(message.result); } }); this.worker.postMessage({ type: "player_actions", id: messageId, playerID: playerID, x: x, y: y, - TransportShipFilter: TransportShipFilter, + transportShipFilter: transportShipFilter, }); }); }
🧹 Nitpick comments (2)
src/core/game/Game.ts (1)
863-869: Add documentation for the enum.The new
TransportShipFilterenum lacks documentation explaining when to use each variant. Consider adding a JSDoc comment describing the purpose and usage of each value.+/** + * Filter for buildable units to control transport ship inclusion. + * - Default: Include transport ships in results (standard behavior) + * - Exclude: Remove transport ships from results + * - Only: Return only transport ships + */ export enum TransportShipFilter { - Default = "default", // Include - Exclude = "exclude", // Exclude - Only = "only", // TransportShip only + Default = "default", + Exclude = "exclude", + Only = "only", }src/client/ClientGameRunner.ts (1)
416-432: Consider capturingmyPlayerin a local const for safer async callback access.The non-null assertion on line 426 assumes
myPlayerremains non-null after the asyncactions()call. While safe in practice, capturingmyPlayerin a local variable after the null check improves clarity and guards against future changes.Apply this pattern:
if (this.myPlayer === null) { const myPlayer = this.gameView.playerByClientID(this.lobby.clientID); if (myPlayer === null) return; this.myPlayer = myPlayer; } +const player = this.myPlayer; -this.myPlayer.actions(tile, TransportShipFilter.Only).then((actions) => { +player.actions(tile, TransportShipFilter.Only).then((actions) => { if (actions.canAttack) { this.eventBus.emit( new SendAttackIntentEvent( this.gameView.owner(tile).id(), - this.myPlayer!.troops() * this.renderer.uiState.attackRatio, + player.troops() * this.renderer.uiState.attackRatio, ), );The same pattern applies to
doBoatAttackUnderCursor(line 517) anddoGroundAttackUnderCursor(line 536).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/client/ClientGameRunner.ts(3 hunks)src/client/graphics/layers/StructureIconsLayer.ts(2 hunks)src/client/graphics/layers/UnitDisplay.ts(2 hunks)src/core/GameRunner.ts(2 hunks)src/core/game/Game.ts(2 hunks)src/core/game/GameView.ts(2 hunks)src/core/game/PlayerImpl.ts(2 hunks)src/core/game/TransportShipUtils.ts(3 hunks)src/core/worker/Worker.worker.ts(1 hunks)src/core/worker/WorkerClient.ts(3 hunks)src/core/worker/WorkerMessages.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
src/core/GameRunner.ts (2)
src/core/game/Game.ts (1)
PlayerActions(742-747)src/core/game/GameView.ts (5)
player(594-600)tile(103-105)x(669-671)y(672-674)actions(276-286)
src/core/game/Game.ts (1)
src/core/game/GameMap.ts (1)
TileRef(3-3)
src/client/graphics/layers/UnitDisplay.ts (1)
src/core/game/GameView.ts (2)
player(594-600)actions(276-286)
src/core/game/GameView.ts (2)
src/core/game/GameMap.ts (1)
TileRef(3-3)src/core/game/Game.ts (1)
PlayerActions(742-747)
src/core/game/PlayerImpl.ts (3)
src/core/game/GameView.ts (1)
inSpawnPhase(642-644)src/core/game/GameMap.ts (1)
TileRef(3-3)src/core/game/Game.ts (1)
BuildableUnit(749-755)
src/client/ClientGameRunner.ts (2)
src/core/game/GameView.ts (2)
tile(103-105)actions(276-286)src/client/Transport.ts (1)
SendAttackIntentEvent(72-77)
src/core/game/TransportShipUtils.ts (2)
src/core/game/GameMap.ts (3)
TileRef(3-3)andFN(459-464)manhattanDistFN(364-380)src/core/game/Game.ts (2)
Game(656-740)Player(517-654)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Deploy to openfront.dev
🔇 Additional comments (7)
src/client/graphics/layers/UnitDisplay.ts (1)
14-19: LGTM! Appropriate use of transport ship filter.The change correctly excludes transport ships from the actions query for the UI display, which makes sense for this context.
Also applies to: 105-105
src/client/graphics/layers/StructureIconsLayer.ts (1)
13-13: LGTM! Correct filter usage.The change appropriately excludes transport ships when querying actions for the ghost structure rendering logic.
Also applies to: 253-253
src/client/ClientGameRunner.ts (3)
15-19: LGTM!The import additions are appropriate for the transport ship filtering functionality.
517-521: LGTM!Correctly uses
TransportShipFilter.Onlyfor boat attack actions.
536-546: LGTM!Ground attack correctly uses default filter behavior (no transport ship filtering).
src/core/game/PlayerImpl.ts (1)
904-935: LGTM!The Default/Exclude filter path correctly handles unit filtering and upgrade logic. The explicit loop structure is clearer than a filtered map for this filtering requirement.
src/core/game/TransportShipUtils.ts (1)
75-117: Logic changes extend beyond performance optimization.The PR title mentions "perf" but lines 75-117 introduce significant behavioral changes:
- Line 76: Explicitly rejects building transport ships to your own territory
- Lines 100-117: New fallback logic for lake/shore deployment when ocean shore conditions aren't met
These are feature enhancements, not just performance optimizations. Consider updating the PR description to document these behavioral changes.
Verify the new lake/shore deployment logic with edge cases:
- When
dstis not an ocean shore but a lake shore- When player has no border tiles that are ocean shores
- When the BFS finds no suitable spawn tiles
You could add unit tests covering these scenarios.
| interface CacheEntry<T> { | ||
| tick: number; | ||
| value: T; | ||
| } | ||
|
|
||
| const transportTileCache = new Map<string, CacheEntry<TileRef | null>>(); | ||
| const buildTransportShipCache = new Map<string, CacheEntry<TileRef | false>>(); | ||
|
|
||
| function getTransportTileCacheKey(tile: TileRef, tick: number): string { | ||
| return `transport_${tile}_${tick}`; | ||
| } | ||
|
|
||
| function getBuildCacheKey( | ||
| playerId: string, | ||
| tile: TileRef, | ||
| tick: number, | ||
| ): string { | ||
| return `build_${playerId}_${tile}_${tick}`; | ||
| } | ||
|
|
||
| let lastCleanupTick = 0; | ||
|
|
||
| function cleanupCache<T>( | ||
| cache: Map<string, CacheEntry<T>>, | ||
| currentTick: number, | ||
| cacheName: string, //TODO: remove after testing | ||
| ): void { | ||
| if (currentTick < lastCleanupTick + 20) { | ||
| return; | ||
| } | ||
| lastCleanupTick = currentTick; | ||
|
|
||
| for (const [key, entry] of cache.entries()) { | ||
| if (entry.tick < currentTick) { | ||
| console.log( | ||
| `Cleaning up ${cacheName} cache for key ${key} at tick ${entry.tick}`, | ||
| ); | ||
| cache.delete(key); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove debug logging and consider game instance isolation for caches.
The TODO comments (lines 30, 39-41) indicate debug logging should be removed before merging. Additionally, module-level caches shared across all game instances could cause issues if multiple games run simultaneously (e.g., in tests or future multi-game scenarios).
- Remove the debug logging:
function cleanupCache<T>(
cache: Map<string, CacheEntry<T>>,
currentTick: number,
- cacheName: string, //TODO: remove after testing
): void {
if (currentTick < lastCleanupTick + 20) {
return;
}
lastCleanupTick = currentTick;
for (const [key, entry] of cache.entries()) {
if (entry.tick < currentTick) {
- console.log(
- `Cleaning up ${cacheName} cache for key ${key} at tick ${entry.tick}`,
- );
cache.delete(key);
}
}
}- Consider scoping caches per-game instance rather than module-level to avoid cross-contamination between games:
// Instead of module-level:
// const transportTileCache = new Map<string, CacheEntry<TileRef | null>>();
// Consider passing game-scoped cache as parameter or storing in Game instance🤖 Prompt for AI Agents
In src/core/game/TransportShipUtils.ts around lines 5 to 45, remove the
temporary console.log debug statement and the leftover TODO, and refactor
module-level caches so they are scoped to a game instance: stop using the
top-level transportTileCache and buildTransportShipCache maps and instead accept
cache objects (or store them) on the Game instance and pass them into functions
(or export a factory that creates per-game caches); update cleanupCache to
operate on the provided per-game cache and remove the debug log call and the
TODO comment.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/core/game/PlayerImpl.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/core/game/PlayerImpl.ts (2)
src/core/game/GameMap.ts (1)
TileRef(3-3)src/core/game/Game.ts (2)
Unit(427-502)BuildableUnit(749-755)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Deploy to openfront.dev
🔇 Additional comments (6)
src/core/game/PlayerImpl.ts (6)
36-36: LGTM!The import is correctly added to support the new filter parameter.
848-866: LGTM!The
skipBuildCheckparameter correctly optimizes repeated feasibility checks. The method properly delegates tocanUpgradeUnitand returns the nearest upgradable unit.
868-879: LGTM!The method correctly checks upgradability first, then conditionally validates build feasibility. The
skipBuildCheckparameter enables efficient reuse.
881-892: LGTM!Good refactoring. This private helper cleanly centralizes the unit build feasibility checks (disabled state, gold cost, alive status) in one place.
908-911: LGTM!Good performance optimization:
canBuildUnitis checked once (line 926), thenskipBuildCheck=trueis passed to avoid redundant validation incanBuildandfindUnitToUpgrade. ThevalidTilescomputation is also optimized by skipping when filter isOnly(ships don't need it).Also applies to: 923-931
944-988: LGTM!The
skipBuildCheckparameter is cleanly integrated. Whenfalse(default), it correctly validates build feasibility before proceeding. This maintains backward compatibility while enabling performance optimization for callers that pre-validate.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/core/game/TransportShipUtils.ts (1)
106-106: Use a named constant for BFS search distance.The hard-coded
300reduces readability. Consider extracting it:const LAKE_TRANSPORT_SEARCH_RADIUS = 300;Then use
manhattanDistFN(dst, LAKE_TRANSPORT_SEARCH_RADIUS)to clarify intent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/core/game/TransportShipUtils.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/core/game/TransportShipUtils.ts (2)
src/core/game/GameMap.ts (3)
TileRef(3-3)andFN(459-464)manhattanDistFN(364-380)src/core/game/Game.ts (2)
Game(656-740)Player(517-654)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Deploy to openfront.dev
| return `build_${playerId}_${tile}_${tick}`; | ||
| } | ||
|
|
||
| let lastCleanupTick = 0; |
There was a problem hiding this comment.
Module-level lastCleanupTick compounds multi-game isolation issue.
The lastCleanupTick variable is shared across all game instances. If multiple games run concurrently at different tick rates, cleanup timing will be incorrect for some games. This amplifies the module-level cache concern already flagged in earlier comments.
Consider moving lastCleanupTick into per-game cache state along with the cache Maps themselves when addressing the broader isolation issue.
Based on learnings
🤖 Prompt for AI Agents
In src/core/game/TransportShipUtils.ts around line 25, the module-level let
lastCleanupTick = 0; is shared across all game instances causing incorrect
cleanup timing for concurrent games; move lastCleanupTick into the per-game
cache state object (the same object that holds the cache Maps) so each game
tracks its own lastCleanupTick, update all references/reads/writes to use
gameCache.lastCleanupTick instead of the module variable, initialize it when
creating the per-game cache, and remove the module-level declaration.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/core/game/Game.ts (1)
863-869: Consider if "Default" member is necessary.Since
transportShipFilteris optional, callers can omit it to get default behavior. Having an explicitDefaultenum member might be redundant unless you need to distinguish between "not specified" and "explicitly default."Additionally, modern TypeScript style favors string literal unions over enums for simple value sets:
export type TransportShipFilter = "default" | "exclude" | "only";However, this enum is consistent with the existing patterns in this codebase (Difficulty, GameType, etc.), so changing it would create inconsistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/core/game/Game.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/core/game/Game.ts (1)
src/core/game/GameMap.ts (1)
TileRef(3-3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Deploy to openfront.dev
🔇 Additional comments (1)
src/core/game/Game.ts (1)
563-566: LGTM - Parameter naming issue resolved.The parameter name now follows TypeScript conventions (lowercase
transportShipFilter), addressing the concern from the previous review.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/core/game/PlayerImpl.ts (2)
901-904: Consider adding an explicit default value for the filter parameter.The
transportShipFilterparameter is optional, and the code relies onundefinedbehaving as "Default" (include all units). This works but could be more explicit.Apply this diff to make the default behavior explicit:
public buildableUnits( tile: TileRef | null, - transportShipFilter?: TransportShipFilter, + transportShipFilter: TransportShipFilter = TransportShipFilter.Default, ): BuildableUnit[] {(Note: This assumes
TransportShipFilter.Defaultexists. If not, adjust accordingly.)
905-905: Consider renaming to avoid double negative.The variable
notInSpawnPhaseuses a double negative which can hurt readability.Apply this diff:
-const notInSpawnPhase = !this.mg.inSpawnPhase(); +const inGamePhase = !this.mg.inSpawnPhase();And update line 926:
-if (tile !== null && this.canBuildUnit(u) && notInSpawnPhase) { +if (tile !== null && this.canBuildUnit(u) && inGamePhase) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/core/GameRunner.ts(2 hunks)src/core/game/Game.ts(2 hunks)src/core/game/GameView.ts(2 hunks)src/core/game/PlayerImpl.ts(3 hunks)src/core/worker/Worker.worker.ts(1 hunks)src/core/worker/WorkerClient.ts(3 hunks)src/core/worker/WorkerMessages.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/core/game/Game.ts
- src/core/worker/WorkerClient.ts
🧰 Additional context used
🧬 Code graph analysis (3)
src/core/GameRunner.ts (2)
src/core/game/Game.ts (1)
PlayerActions(742-747)src/core/game/GameView.ts (5)
player(594-600)tile(103-105)x(669-671)y(672-674)actions(276-286)
src/core/game/PlayerImpl.ts (2)
src/core/game/GameMap.ts (1)
TileRef(3-3)src/core/game/Game.ts (2)
Unit(427-502)BuildableUnit(749-755)
src/core/game/GameView.ts (2)
src/core/game/GameMap.ts (1)
TileRef(3-3)src/core/game/Game.ts (1)
PlayerActions(742-747)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Deploy to openfront.dev
🔇 Additional comments (8)
src/core/game/GameView.ts (1)
276-285: LGTM! Parameter threading is clean.The
transportShipFilterparameter is correctly named (lowercase) and properly forwarded to the worker interaction. The optional parameter pattern matches the rest of the API.src/core/game/PlayerImpl.ts (4)
848-866: LGTM! Clean addition of skipBuildCheck parameter.The
skipBuildCheckparameter with a default value offalseis a clean way to allow callers to bypass build feasibility checks when needed. The early return pattern keeps the logic clear.
868-892: Good refactor to centralize build feasibility checks.Extracting
canBuildUniteliminates duplication and makes the build logic easier to maintain. The three checks (disabled, cost, alive) properly gate unit construction.
914-921: LGTM! Filter logic is now correct.The filter correctly checks
u !== UnitType.TransportShipfor the Only case, ensuring:
- Exclude: skip TransportShip, include others
- Only: skip non-TransportShip, include only TransportShip
- Default (undefined): include all units
This fixes the critical bug from the previous review where it checked TradeShip instead.
923-931: LGTM! canUpgrade logic is correct.The code properly computes
canUpgradeby callingfindUnitToUpgradeand using the unit's ID when an upgradable unit exists. This fixes the issue where it was previously hardcoded to0.src/core/GameRunner.ts (1)
181-215: LGTM! Clean parameter threading.The
transportShipFilterparameter is correctly named (lowercase), properly typed, and correctly passed toplayer.buildableUnits. The signature matches the worker interface expectations.src/core/worker/WorkerMessages.ts (1)
61-67: LGTM! Message interface correctly extended.The
transportShipFilterproperty is correctly named (lowercase), properly typed as optional, and aligns with the other optional coordinate parameters in the message.src/core/worker/Worker.worker.ts (1)
76-96: LGTM! Worker correctly forwards the filter parameter.The worker properly extracts
message.transportShipFilter(lowercase) and passes it toplayerActions. The error handling remains appropriate.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/core/game/TransportShipUtils.ts (1)
5-15: Module-level cache isolation issue remains unresolved.Past review comments identified that
transportTileCache(line 9) andlastCleanupTick(line 15) are shared across all game instances, which can cause incorrect behavior if multiple games run concurrently. This concern still applies to the current code.
🧹 Nitpick comments (1)
src/core/game/TransportShipUtils.ts (1)
17-28: Consider cleaning stale entries more proactively.The cleanup runs every 20 ticks (line 18), meaning stale cache entries from previous ticks accumulate in memory between cleanup cycles. While the cache check on line 113 ensures stale entries are never returned, they still consume memory.
Consider triggering cleanup when the tick changes, or clearing the entire cache when
currentTickadvances:function cleanupCache(currentTick: number): void { - if (currentTick < lastCleanupTick + 20) { + if (currentTick === lastCleanupTick) { return; } - lastCleanupTick = currentTick; - - for (const [key, entry] of transportTileCache.entries()) { - if (entry.tick < currentTick) { - transportTileCache.delete(key); - } - } + // Clear entire cache when tick advances since entries are only valid for current tick + transportTileCache.clear(); + lastCleanupTick = currentTick; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/core/game/TransportShipUtils.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/core/game/TransportShipUtils.ts (2)
src/core/game/GameMap.ts (1)
TileRef(3-3)src/core/game/Game.ts (2)
Game(656-740)Player(517-654)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Deploy to openfront.dev
🔇 Additional comments (2)
src/core/game/TransportShipUtils.ts (2)
49-52: Clean refactor to use cached destination resolution.The refactored logic correctly delegates to
targetTransportTile, which provides per-tick caching. This improves performance while keeping the code readable.
108-129: Well-structured caching layer.The new
targetTransportTilefunction correctly implements per-tick caching with appropriate cache validation (line 113). CallingcleanupCacheon every cache miss (line 126) is acceptable since cleanup early-exits when not needed.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/core/game/PlayerImpl.ts (1)
898-909: Clarify method purpose: this checks a specificUnitinstance, not aUnitType.The method signature correctly expects a
Unitinstance (line 898), but it's being called with aUnitTypeat line 883. The instance-specific check at line 899 (unit.isMarkedForDeletion()) confirms this method is for checking specific units, not types in general.Consider renaming to
canUpgradeUnitInstanceor adding a separate type-level check method to make the distinction clearer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/client/graphics/layers/StructureIconsLayer.ts(2 hunks)src/core/GameRunner.ts(2 hunks)src/core/game/Game.ts(2 hunks)src/core/game/GameView.ts(2 hunks)src/core/game/PlayerImpl.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/core/GameRunner.ts
- src/core/game/Game.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/core/game/GameView.ts (2)
src/core/game/GameMap.ts (1)
TileRef(3-3)src/core/game/Game.ts (1)
PlayerActions(750-756)
src/core/game/PlayerImpl.ts (3)
src/core/game/GameMap.ts (1)
TileRef(3-3)src/core/game/Game.ts (2)
Unit(430-508)BuildableUnit(758-764)src/client/graphics/layers/UnitDisplay.ts (1)
canBuild(82-101)
🪛 GitHub Actions: 🧪 CI
src/core/game/PlayerImpl.ts
[error] 899-899: unit.isMarkedForDeletion is not a function
🪛 GitHub Actions: 🚀 Deploy
src/core/game/PlayerImpl.ts
[error] 883-883: TS2345: Argument of type 'import("/usr/src/app/src/core/game/Game").UnitType' is not assignable to parameter of type 'Unit'.
🔇 Additional comments (5)
src/core/game/GameView.ts (1)
280-289: LGTM! Parameter threading looks clean.The optional
transportShipFilterparameter is properly typed and forwarded to the worker. The previous naming concern has been addressed correctly with the lowercase parameter name.src/client/graphics/layers/StructureIconsLayer.ts (1)
253-253: LGTM! Filter usage is appropriate here.Passing
TransportShipFilter.Excludefor ghost structure placement makes sense—structure building UI doesn't need transport ship actions.src/core/game/PlayerImpl.ts (3)
911-922: Nice consolidation of build feasibility checks.Extracting the common checks (disabled, gold, alive) into a private helper method reduces duplication and makes the code easier to maintain.
931-978: Filter logic looks correct; depends on fixingfindUnitToUpgradetype mismatch.The
TransportShipFilterintegration is well-structured:
Excludefilter skipsTransportShip(lines 946-950)Onlyfilter processes onlyTransportShipthen breaks (lines 952-957)validTilesoptimization skips expensive computation when not needed (lines 939-942)However, line 965 will fail due to the type mismatch bug in
findUnitToUpgradeidentified above. Once that's fixed, this logic should work correctly.The
skipBuildCheck=truepattern at lines 963 and 965 is correct since line 962 already validatedcanBuildUnit(u).
980-1024: Clean addition ofskipBuildCheckparameter.The optional
skipBuildCheckparameter (line 984) allows callers to avoid redundant validations when they've already checkedcanBuildUnit. The conditional at line 986 correctly applies the check only when not skipping.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/core/game/PlayerImpl.ts (1)
878-900: Consider removing redundant type check in canUpgradeUnit call.When
skipUnitTypeCheckis false, the code checkscanUpgradeUnitType(type, ...)at line 883, then filters nearby units by that same type at line 889, and finally callscanUpgradeUnit(unit, ...)at line 895 which checkscanUpgradeUnitType(unit.type())again at line 909. Since line 889 already filters by type,unit.type()must equal thetypeparameter, making the second check redundant.This is defensive but adds unnecessary overhead when skip is false. Consider either:
- Always passing
truefor skip tocanUpgradeUnitsince the type was already validated- Documenting that the double-check is intentional
- if (!this.canUpgradeUnit(unit, skipUnitTypeCheck)) { + // Type already validated at line 883, only check unit-specific conditions + if (!this.canUpgradeUnit(unit, true)) { return false; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/core/game/PlayerImpl.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/core/game/PlayerImpl.ts (2)
src/core/game/GameMap.ts (1)
TileRef(3-3)src/core/game/Game.ts (2)
Unit(430-508)BuildableUnit(758-764)
🪛 GitHub Actions: 🧪 CI
src/core/game/PlayerImpl.ts
[error] 1-1: City upgrade test failure: expected city.canUpgrade to be true, but it was false in 'City can be upgraded' scenario.
[error] 1-1: City upgrade test failure: expected cityToUpgrade to be truthy in 'City can be upgraded from another city' scenario.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Deploy to openfront.dev
🔇 Additional comments (4)
src/core/game/PlayerImpl.ts (4)
928-939: Good refactor consolidating build eligibility checks.This helper cleanly extracts the unit-type-level validation (disabled check, gold check, alive check) that was previously scattered or duplicated. The separation from spawn-location validation makes the code easier to reason about.
956-960: Good optimization: compute valid tiles once and reuse.Computing
validTilesonce before the loop (lines 956-959) and passing it tocanBuild(line 980) avoids redundant BFS traversals for each unit type. When filter isOnly, the empty array is safe since TransportShip uses its own spawn logic.
997-1005: LGTM: skipUnitTypeCheck parameter addition.The
skipUnitTypeCheckparameter follows the same pattern established infindUnitToUpgradeandcanUpgradeUnit, allowing callers to bypass type validation when they've already performed it. This improves efficiency inbuildableUnitswhich pre-checks withcanBuildUnitTypebefore calling this method.
948-995: The review comment is incorrect. No issues exist in the code.The
buildableUnitsmethod correctly gates bothcanBuildandcanUpgradeonnotInSpawnPhase(line 979). This is the intended design: upgrades are not allowed during spawn phase.Evidence:
- The "City can be upgraded" test (tests/PlayerImpl.test.ts:38) explicitly exits spawn phase before testing:
while (game.inSpawnPhase()) { game.executeNextTick(); }- The UI disables build and upgrade menus during spawn phase (RadialMenuElements.ts)
- Game logic skips non-spawn actions during spawn (ClientGameRunner.ts:413-414)
Blocking upgrades during spawn phase is by design, not a bug. The test setup confirms this is working as intended.
Likely an incorrect or invalid review comment.
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/game/PlayerImpl.ts (1)
883-905: Update interface signature to match implementation.The interface at
src/core/game/Game.ts:586definesfindUnitToUpgrade(type: UnitType, targetTile: TileRef): Unit | falsewithout theskipUnitTypeCheckparameter, while the implementation atsrc/core/game/PlayerImpl.ts:883includesskipUnitTypeCheck: boolean = false.The logic at line 900 (hardcoding
truetocanUpgradeUnit) appears intentional—validate type eligibility once upfront, then skip type recheck on the specific unit instance. However, the interface and implementation contract mismatch means code using this method through theGameinterface type cannot pass theskipUnitTypeCheckargument. The call at line 990 works because it directly accesses thePlayerImplinstance.Add the optional parameter to the interface signature to expose this functionality consistently.
♻️ Duplicate comments (1)
src/core/game/TransportShipUtils.ts (1)
9-9: ** Module-level caches still cause multi-game isolation issues.**The
transportTileCachemap andlastCleanupTickvariable remain at module scope, shared across all game instances. This was flagged in previous reviews. If multiple games run concurrently (tests, future multi-game scenarios), cache entries and cleanup timing will be incorrect.Based on learnings
Also applies to: 15-15
🧹 Nitpick comments (1)
src/core/game/TransportShipUtils.ts (1)
17-28: Consider renaming parameter or making cleanup more explicit.The
cleanupCachefunction now only operates on the module-leveltransportTileCache, but doesn't accept the cache as a parameter. This makes it less clear that it's tied to a specific cache and harder to refactor when addressing the multi-game isolation issue.Minor: Once the cache is moved to per-game scope, this function should accept the cache as a parameter:
-function cleanupCache(currentTick: number): void { +function cleanupCache( + cache: Map<string, CacheEntry<TileRef | null>>, + currentTick: number, +): void { if (currentTick < lastCleanupTick + 20) { return; } lastCleanupTick = currentTick; - for (const [key, entry] of transportTileCache.entries()) { + for (const [key, entry] of cache.entries()) { if (entry.tick < currentTick) { - transportTileCache.delete(key); + cache.delete(key); } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/core/game/Game.ts(2 hunks)src/core/game/PlayerImpl.ts(4 hunks)src/core/game/TransportShipUtils.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/game/Game.ts
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2025-08-12T00:31:50.144Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1752
File: src/core/game/Game.ts:750-752
Timestamp: 2025-08-12T00:31:50.144Z
Learning: In the OpenFrontIO codebase, changes to the PlayerInteraction interface (like adding canDonateGold and canDonateTroops flags) do not require corresponding updates to src/core/Schemas.ts or server serialization code.
Applied to files:
src/core/game/PlayerImpl.ts
📚 Learning: 2025-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/game/PlayerImpl.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/game/PlayerImpl.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/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/game/PlayerImpl.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/game/PlayerImpl.ts
📚 Learning: 2025-05-19T06:00:38.007Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 784
File: src/core/game/StatsImpl.ts:125-134
Timestamp: 2025-05-19T06:00:38.007Z
Learning: In StatsImpl.ts, unused parameters in boat/stats-related methods are intentionally kept for future use and shouldn't be removed.
Applied to files:
src/core/game/TransportShipUtils.ts
📚 Learning: 2025-10-20T20:15:28.858Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:51-51
Timestamp: 2025-10-20T20:15:28.858Z
Learning: In src/core/execution/FakeHumanExecution.ts, game balance constants like MIRV_COOLDOWN_TICKS, MIRV_HESITATION_ODDS, VICTORY_DENIAL_TEAM_THRESHOLD, VICTORY_DENIAL_INDIVIDUAL_THRESHOLD, and STEAMROLL_CITY_GAP_MULTIPLIER are experimental tuning parameters subject to frequent change during balance testing. Do not flag changes to these values as issues or compare them against previous values.
Applied to files:
src/core/game/TransportShipUtils.ts
🧬 Code graph analysis (2)
src/core/game/PlayerImpl.ts (3)
src/core/game/GameMap.ts (1)
TileRef(3-3)src/core/game/Game.ts (2)
Unit(432-510)BuildableUnit(761-767)src/client/graphics/layers/UnitDisplay.ts (1)
canBuild(82-101)
src/core/game/TransportShipUtils.ts (2)
src/core/game/GameMap.ts (1)
TileRef(3-3)src/core/game/Game.ts (2)
Game(667-751)Player(525-665)
🔇 Additional comments (5)
src/core/game/TransportShipUtils.ts (2)
108-129: Cache key includes tick for proper per-tick isolation.The caching implementation correctly includes the tick in the cache key, ensuring results are only reused within the same tick. The cleanup logic appropriately removes stale entries from previous ticks.
49-68: Simplified ocean logic validation is preserved—no issue found.The opponent ocean border check has not been removed; it's validated implicitly earlier in the function flow. Here's why:
targetTransportTile(game, tile)retrieves the destination player's closest shore tile by callingclosestShoreFromPlayer()- If the destination player is landlocked (no shore access),
closestShoreFromPlayerreturnsnull- Lines 50-51 catch this:
if (dst === null) { return false; }- Only if the destination has valid shore access does the code reach line 54
The ocean path then checks: current player borders ocean + destination has shore access (already verified). This is correct logic. The refactoring removed redundant explicit checks in favor of factoring validation into
targetTransportTile, making the code cleaner without losing safety.Transport ship spawning correctly rejects attempts against landlocked players through the existing
targetTransportTilevalidation.src/core/game/PlayerImpl.ts (3)
923-947: Helper methods cleanly separate type-level and instance-level validation.The new
canUpgradeUnitTypeandcanBuildUnitTypehelpers properly separate concerns: type eligibility (upgradable flag, disabled status, cost, alive status) vs. instance validity (deletion status, ownership). This makes the validation flow clearer and reusable.
956-1003:buildableUnitscorrectly handles TransportShipFilter modes.The filter logic:
- Exclude: Skips TransportShip (line 971-975)
- Only: Processes only TransportShip, breaks early (line 977-982)
- Default: Processes all unit types
The
validTilescomputation (lines 964-967) correctly excludes computation whenTransportShipFilter.Onlyis active, since TransportShip doesn't require structure spawn tile validation. ThecanBuildcall at line 988 passesskipUnitTypeCheck=trueto avoid redundant checks aftercanBuildUnitTypevalidation.
1005-1049:canBuildsignature extended withskipUnitTypeCheckparameter.The new parameter enables callers to skip type-level validation when already verified upstream (e.g., in
buildableUnits). Line 1011 guards thecanBuildUnitTypecall with the flag. For TransportShip (line 1034), the delegation tocanBuildTransportShipis unchanged and doesn't use thevalidTilesparameter, which is correct.
Description:
Describe the PR.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
DISCORD_USERNAME