initial commit #2125
initial commit #2125
Conversation
WalkthroughAdds a WebAssembly-backed pathfinder, converts many tick/execute flows to async/await, updates execution classes to await pathfinding, removes several legacy A* adapters, and adjusts Jest/test setup to mock WASM loading for tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor T as Test/UI
participant GR as GameRunner
participant G as Game
participant EX as Executions (Warship/Trade/Transport)
participant PF as WasmPathFinder
participant WASM as wasm.find_path
T->>GR: executeNextTick()
GR->>G: await executeNextTick()
G->>EX: for each e.tick(t)
activate EX
EX->>PF: await nextTile(curr, dst)
PF-->>PF: ensure wasm loaded
PF->>WASM: find_path(grid, start, end)
WASM-->>PF: path | none
PF-->>EX: NextTile | Completed | PathNotFound
deactivate EX
G-->>GR: GameUpdates (Promise)
GR-->>T: GameUpdates
sequenceDiagram
autonumber
participant RN as RailNetworkImpl.connect(a,b)
participant PF as WasmPathFinder
participant WASM as wasm.find_path
participant RR as Railroad
RN->>PF: await nextTile(...) / path request
PF->>WASM: find_path(...)
WASM-->>PF: path | none
PF-->>RN: result
alt path found
RN->>RR: build Railroad from path
RR-->>RN: instance
else path not found
RN-->>RN: abort connection
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
tests/Disconnected.test.ts (1)
85-85: Await async executeTicks calls in tests
The helperexecuteTicksis declared asexport async function executeTicks(...)returning a Promise; all calls intests/Disconnected.test.ts(lines 85, 96, 105, 119, 128, 134, 157) must be prefixed withawaitto ensure ticks finish before assertions.tests/core/game/RailNetwork.test.ts (1)
133-138: Fix RailNetworkImpl test setup.We now expect an empty path, but the suite still builds
RailNetworkImplwith a thirdpathServiceargument. The implementation only accepts(game, stationManager)now, so the call at Line 76 triggersTS2554: Expected 2 arguments, but got 3and CI stops. Please drop the injectedpathServiceand mock the WASM pathfinder entry point instead (for example stubPathFinder.Wasmto hand back a fake). Until the test matches the new constructor, the build will not compile.tests/AllianceRequestExecution.test.ts (1)
91-96: Await the asynchronous helper.
constructionExecutionnow returns a Promise. The test moves on without waiting, so Jest can finish the spec before those internal ticks complete, which makes the assertions racey. Please addawaitwhen calling it. Based on Jest async guidance.(jestjs.io)- constructionExecution(game, player1, 0, 0, UnitType.MissileSilo); + await constructionExecution(game, player1, 0, 0, UnitType.MissileSilo);src/core/execution/WarshipExecution.ts (1)
19-30: Fix pathfinder type mismatch
PathFinder.Wasm()returns aWasmPathFinder, which does not share the field signature ofPathFinder. Assigning it toprivate pathfinder: PathFinderfails type checking and blocks compilation. Please store the instance with the actual return type (for example,ReturnType<typeof PathFinder.Wasm>orWasmPathFinder) so the async API lines up with the concrete implementation.- private pathfinder: PathFinder; + private pathfinder: ReturnType<typeof PathFinder.Wasm>;src/core/game/RailNetworkImpl.ts (1)
121-128: Await the async connect call
connect()returnsPromise<boolean>. Using it directly in anifmeans the branch always runs (a promise is always truthy) and TypeScript already flags TS2801. Please await the promise and makeconnectToNearbyStations(and its callers) async so the rail-building logic only runs after the pathfinder finishes.- if (this.connect(station, neighborStation)) { + if (await this.connect(station, neighborStation)) {Remember to mark
connectToNearbyStations(andconnectStation, plus any interface signatures) asasyncso this compiles cleanly.
🧹 Nitpick comments (2)
tests/util/Setup.ts (1)
55-59: Consider using typed wrappers instead of runtime property assignment.Assigning
widthandheightfunctions directly togameMapandminiGameMapbypasses TypeScript's type checking. If the Terrain type lacks these methods, consider:
- Adding
width()andheight()methods to the Terrain class itself, or- Creating a typed wrapper/adapter that implements the expected interface.
This approach improves type safety and makes the contract explicit rather than relying on runtime monkey-patching in test setup.
wasm/pathfinding/Cargo.toml (1)
1-14: LGTM with minor cleanup suggestions.The dependency versions are valid and well-chosen. The separation of
js-sysas an explicit dependency (line 13) and removal of thejs-sysfeature fromweb-sys(line 14) is the correct approach.Consider these optional cleanups:
- Remove the inline comments on lines 13-14 explaining the change (keep them in the PR description/commit message instead).
- Verify that
console_error_panic_hookis actually used in your WASM initialization code. If not, remove the optional dependency. If it is used, callconsole_error_panic_hook::set_once()early in your module initialization for better debugging.[dependencies] wasm-bindgen = "0.2.92" pathfinding = "4.0.0" console_error_panic_hook = { version = "0.1.7", optional = true } -js-sys = "0.3.69" # Added js-sys as a separate dependency -web-sys = { version = "0.3.69", features = ["console"] } # Removed js-sys feature +js-sys = "0.3.69" +web-sys = { version = "0.3.69", features = ["console"] }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
wasm/pathfinding/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (40)
.gitignore(1 hunks).husky/pre-commit(1 hunks)jest.config.ts(1 hunks)src/core/GameRunner.ts(2 hunks)src/core/execution/TradeShipExecution.ts(3 hunks)src/core/execution/TransportShipExecution.ts(4 hunks)src/core/execution/WarshipExecution.ts(6 hunks)src/core/game/Game.ts(2 hunks)src/core/game/GameImpl.ts(1 hunks)src/core/game/RailNetworkImpl.ts(5 hunks)src/core/game/TrainStation.ts(0 hunks)src/core/game/TransportShipUtils.ts(1 hunks)src/core/pathfinding/AStar.ts(0 hunks)src/core/pathfinding/PathFinding.ts(2 hunks)src/core/pathfinding/SerialAStar.ts(0 hunks)src/core/pathfinding/WasmPathfinding.ts(1 hunks)tests/AllianceExtensionExecution.test.ts(4 hunks)tests/AllianceRequestExecution.test.ts(3 hunks)tests/Attack.test.ts(8 hunks)tests/AttackStats.test.ts(3 hunks)tests/BotBehavior.test.ts(5 hunks)tests/DeleteUnitExecution.test.ts(1 hunks)tests/Disconnected.test.ts(1 hunks)tests/Donate.test.ts(9 hunks)tests/MissileSilo.test.ts(4 hunks)tests/PlayerImpl.test.ts(1 hunks)tests/ShellRandom.test.ts(7 hunks)tests/Stats.test.ts(1 hunks)tests/TerritoryCapture.test.ts(1 hunks)tests/Warship.test.ts(7 hunks)tests/core/executions/NukeExecution.test.ts(4 hunks)tests/core/executions/SAMLauncherExecution.test.ts(8 hunks)tests/core/game/GameImpl.test.ts(4 hunks)tests/core/game/RailNetwork.test.ts(1 hunks)tests/setupJest.ts(1 hunks)tests/util/Setup.ts(1 hunks)tests/util/utils.ts(2 hunks)tests/wasm_test.test.ts(1 hunks)wasm/pathfinding/Cargo.toml(1 hunks)wasm/pathfinding/src/lib.rs(1 hunks)
💤 Files with no reviewable changes (3)
- src/core/pathfinding/AStar.ts
- src/core/game/TrainStation.ts
- src/core/pathfinding/SerialAStar.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-24T11:13:08.495Z
Learnt from: DevelopingTom
PR: openfrontio/OpenFrontIO#1900
File: src/core/execution/SAMLauncherExecution.ts:103-111
Timestamp: 2025-08-24T11:13:08.495Z
Learning: In SAMLauncherExecution.ts, the cached target bug can only occur if: 1) SAM is not on cooldown when nuke is in range, 2) SAM goes on cooldown right after computing trajectory, 3) SAM becomes available again before nuke explodes. This is not possible with current cooldown values but the fix is still valuable for robustness.
Applied to files:
tests/core/executions/SAMLauncherExecution.test.ts
🧬 Code graph analysis (19)
tests/AllianceExtensionExecution.test.ts (3)
src/core/execution/alliance/AllianceRequestExecution.ts (1)
AllianceRequestExecution(9-72)src/core/execution/alliance/AllianceRequestReplyExecution.ts (1)
AllianceRequestReplyExecution(3-56)src/core/execution/alliance/AllianceExtensionExecution.ts (1)
AllianceExtensionExecution(9-73)
tests/core/game/GameImpl.test.ts (2)
src/core/execution/alliance/AllianceRequestReplyExecution.ts (1)
AllianceRequestReplyExecution(3-56)src/core/execution/AttackExecution.ts (1)
AttackExecution(18-376)
tests/Attack.test.ts (1)
tests/util/utils.ts (1)
constructionExecution(10-30)
src/core/game/Game.ts (2)
src/core/game/GameImpl.ts (1)
ticks(326-328)src/core/game/GameView.ts (1)
ticks(616-619)
src/core/pathfinding/PathFinding.ts (2)
src/core/game/Game.ts (1)
Game(639-723)src/core/pathfinding/WasmPathfinding.ts (1)
WasmPathFinder(43-116)
src/core/game/GameImpl.ts (1)
src/core/game/Game.ts (1)
GameUpdates(25-27)
tests/util/utils.ts (1)
src/core/game/Game.ts (2)
Game(639-723)Player(503-637)
tests/AttackStats.test.ts (2)
src/core/game/Game.ts (2)
Game(639-723)Player(503-637)src/core/execution/AttackExecution.ts (1)
AttackExecution(18-376)
tests/core/executions/NukeExecution.test.ts (1)
tests/util/utils.ts (1)
executeTicks(32-39)
tests/Warship.test.ts (1)
tests/util/utils.ts (1)
executeTicks(32-39)
tests/MissileSilo.test.ts (1)
tests/util/utils.ts (2)
constructionExecution(10-30)executeTicks(32-39)
src/core/execution/TradeShipExecution.ts (1)
src/core/pathfinding/PathFinding.ts (1)
PathFinder(106-211)
wasm/pathfinding/src/lib.rs (1)
src/client/graphics/NameBoxCalculator.ts (1)
Point(4-7)
src/core/pathfinding/WasmPathfinding.ts (4)
src/core/pathfinding/AStar.ts (1)
AStarResult(12-26)src/core/game/GameMap.ts (1)
TileRef(3-3)src/core/game/Game.ts (1)
Game(639-723)src/core/game/GameImpl.ts (2)
y(785-787)x(782-784)
src/core/execution/WarshipExecution.ts (2)
src/core/pathfinding/PathFinding.ts (1)
PathFinder(106-211)src/core/game/GameImpl.ts (1)
ticks(326-328)
tests/core/executions/SAMLauncherExecution.test.ts (1)
tests/util/utils.ts (2)
constructionExecution(10-30)executeTicks(32-39)
src/core/game/RailNetworkImpl.ts (2)
src/core/game/TrainStation.ts (1)
TrainStation(73-156)src/core/pathfinding/WasmPathfinding.ts (1)
WasmPathFinder(43-116)
tests/setupJest.ts (1)
src/core/game/FetchGameMapLoader.ts (1)
url(41-49)
src/core/execution/TransportShipExecution.ts (1)
src/core/pathfinding/PathFinding.ts (1)
PathFinder(106-211)
🪛 GitHub Actions: 🧪 CI
tests/core/game/RailNetwork.test.ts
[error] 76-76: TS2554: Expected 2 arguments, but got 3.
src/core/pathfinding/WasmPathfinding.ts
[error] 32-32: Could not locate module '../../wasm/pathfinding/pkg' mapped to '/static/js/pathfinding'. Ensure WASM pathfinding package is built and available at the expected path, and that moduleNameMapper is configured correctly.
src/core/game/RailNetworkImpl.ts
[error] 124-124: TS2801: This condition will always return true since this 'Promise' is always defined.
[error] 137-137: TS2345: Argument of type 'TrainStation[]' is not assignable to parameter of type 'Set'.
[error] 166-166: TS2341: Property 'path' is private and only accessible within class 'WasmPathFinder'.
tests/wasm_test.test.ts
[error] 4-4: Cannot find module '../wasm/pathfinding/pkg' or its corresponding type declarations.
tests/setupJest.ts
[error] 11-11: Conversion of type '{ arrayBuffer: () => Promise; headers: Headers; ok: true; redirected: false; status: number; statusText: string; type: "basic"; url: string; clone: () => Promise<...>; }' to type 'Response' may be a mistake because neither type sufficiently overlaps with the other.
[error] 29-29: Type 'Mock<Promise<WebAssemblyInstantiatedSource | Instance>, [source: Response | PromiseLike | Module, importObject: Imports | undefined], any>' is not assignable to type '(source: Response | PromiseLike, importObject?: Imports | undefined) => Promise'.
🔇 Additional comments (39)
.gitignore (1)
13-13: LGTM!Correctly ignoring the Rust build artifacts directory for the WASM pathfinding crate. The leading slash properly scopes it to the repository root.
tests/Stats.test.ts (1)
26-28: LGTM!Correctly updated to
awaitthe now-asynchronousexecuteNextTick()call. The change aligns with the broader async tick migration across the codebase, and thebeforeEachis already declaredasyncto support this.tests/Disconnected.test.ts (1)
40-42: LGTM – spawn loop correctly awaits tick execution.The conversion to
await game.executeNextTick()aligns with the async migration.tests/PlayerImpl.test.ts (1)
27-29: LGTM – spawn loop correctly awaits tick execution.The async conversion is consistent with the broader migration to async tick handling.
tests/DeleteUnitExecution.test.ts (1)
51-53: LGTM – spawn loop correctly awaits tick execution.The async conversion follows the established pattern across the test suite.
tests/TerritoryCapture.test.ts (1)
16-18: LGTM – both tick executions correctly awaited.The sequential awaits ensure proper execution order (init, then execute). Clean async conversion.
src/core/game/TransportShipUtils.ts (1)
140-148: Disabled stub is handled gracefully
Call sites in Worker.worker.ts forward thefalseresult unchanged, and ClientGameRunner mapsfalsetonullfor the spawn parameter; no caller assumes a validTileRefor throws onfalse. No tests reference this stubbed function, so disabling pathfinding won’t crash the game—only suspends transport-ship deployment until the WASM implementation is ready.src/core/game/Game.ts (1)
331-331: LGTM! Union type allows gradual async migration.The
void | Promise<void>return type lets execution implementations migrate to async incrementally. Synchronous executions can still returnvoidwhile pathfinding-dependent executions (ships, etc.) can returnPromise<void>.tests/Donate.test.ts (4)
42-44: Consistent async spawn phase handling.All spawn phase loops correctly await tick progression. Pattern is uniform across all four test cases.
Also applies to: 105-107, 169-171, 229-231
117-117: Good! State progression ensured before donation.Adding
await game.executeNextTick()after alliance acceptance ensures the alliance is fully processed before donation execution begins.
61-63: Donation execution loops properly awaited.Both gold and troops donation tests correctly await ticks during execution processing.
Also applies to: 125-127
186-186: Single tick awaits for rejected alliance scenarios.Both negative test cases (non-ally donation attempts) correctly await one tick to process the rejected donation execution.
Also applies to: 246-246
tests/Warship.test.ts (4)
32-34: Spawn phase properly awaited in setup.The
beforeEachhook correctly awaits spawn phase completion before tests run.
57-57: Healing test correctly awaits each tick.The test verifies warship healing behavior with proper async tick progression between health checks.
Also applies to: 62-62, 67-67
92-94: Trade ship capture tests allow sufficient pathfinding time.Both tests iterate 10 ticks awaiting each, giving A* pathfinding adequate time to resolve before assertions.
Also applies to: 117-119
147-147: Good use of executeTicks helper.Three tests use the
executeTicks(game, N)helper fromtests/util/utils.tsfor cleaner multi-tick progression. This is more readable than manual loops.Also applies to: 169-169, 197-197
src/core/GameRunner.ts (2)
118-118: Async conversion preserves error handling.The method is correctly marked
asyncand awaitsthis.game.executeNextTick()within the existing try-catch block. Error handling behavior is preserved.Also applies to: 135-135
119-131: Concurrency guard and sequencing preserved.The
isExecutingflag still prevents concurrent tick execution, andcurrTurnis incremented before the await. Sequencing is correct.tests/BotBehavior.test.ts (3)
238-290: Setup helper correctly migrated to async.The
setupTestEnvironmenthelper is properly markedasyncand awaits spawn phase completion at line 277. The function returns the test environment for use inbeforeEach.
316-338: Attack behavior tests properly await tick progression.Both alliance-related attack tests correctly await 5 ticks after scheduling attacks, allowing execution to complete before asserting that allied players cannot attack each other.
Also applies to: 340-384
94-229: Alliance request tests don't require async.The first two describe blocks test synchronous alliance logic without tick progression, so they correctly remain synchronous. Only the attack behavior tests need async.
tests/wasm_test.test.ts (1)
19-31: Test logic is sound—awaiting wasm build fix.The test correctly awaits
loadWasm()before invoking wasm functions and validatesget_vec_lenwith both non-empty and empty arrays. The assertions are appropriate for verifying Vec transfer.Once the wasm package build is fixed (see previous comment), this test should pass.
tests/core/game/GameImpl.test.ts (3)
22-57: LGTM—async setup is correct.The
beforeEachcorrectly awaitssetup()and allexecuteNextTick()calls in the spawn phase loop. The async conversion is clean and maintains the original test setup logic.
59-96: LGTM—async conversion preserves test intent.All
executeNextTick()calls are properly awaited, including in the do-while attack completion loop. The test logic for verifying non-traitor status when betraying a disconnected player remains intact.
98-136: LGTM—async conversion is consistent.The test properly awaits all tick executions and maintains the original logic for verifying traitor status when betraying an active player. The async conversion is clean and correct.
tests/MissileSilo.test.ts (3)
18-30: LGTM—helper function async conversion is correct.The
attackerBuildsNukehelper is properly converted to async and awaits the tick executions wheninitializeis true. The function signature and logic remain consistent with the async tick API.
33-54: LGTM—beforeEach async setup is clean.The setup correctly awaits
setup(), the spawn phase loop, andconstructionExecution(). All async operations are properly sequenced.
56-105: LGTM—test cases properly converted to async.All three test cases correctly await
attackerBuildsNuke(),executeNextTick(), andexecuteTicks(). The async conversion maintains the original test logic for nuke launching, cooldown verification, and structure upgrades.tests/Attack.test.ts (4)
29-74: LGTM—setup and initial attack flow properly awaited.The
beforeEachcorrectly awaitssetup(), spawn phase ticks, and the initial attack completion loop. The async conversion is clean and maintains the original test setup logic.
76-114: LGTM—nuke test async conversions are correct.Both nuke tests properly await
constructionExecution()andexecuteNextTick(). The test logic for verifying nuke effects on attacking troops and transport ships remains intact.
129-156: LGTM—alliance test setup awaits correctly.The
beforeEachfor alliance race condition tests properly awaitssetup()and the spawn phase loop. Player setup logic is clean.
158-296: LGTM—alliance race condition tests properly converted.All three test cases correctly await tick execution loops. The tests for traitor status, alliance-based attack prevention, and alliance request cancellation maintain their original logic with proper async control flow.
tests/AttackStats.test.ts (3)
12-27: LGTM—setup properly awaits async operations.The
beforeEachcorrectly awaitssetup()and the spawn phase loop. The player setup for attack stats testing is clean and properly sequenced.
76-85: LGTM—performAttack helper async conversion is correct.The helper function properly awaits tick execution in the attack completion loop. The async conversion maintains the original attack execution logic.
29-53: LGTM—test cases properly await attack completion.Both test cases correctly await
performAttack()before asserting war gold stats. The async conversion ensures stats are verified after all attack operations complete. The territory annexation scenario setup is clear.src/core/execution/TradeShipExecution.ts (1)
30-30: LGTM! Clean conversion to async WASM pathfinding.The pathfinder initialization switches to
PathFinder.Wasm, the tick method is correctly marked async, and the pathfinding call is properly awaited. All existing error paths and result handling remain intact.Also applies to: 33-33, 104-107
src/core/game/GameImpl.ts (1)
330-341: LGTM! Correct async tick execution pattern.The method signature correctly returns
Promise<GameUpdates>, collects all executor tick results (which may be void or Promise), and usesPromise.allto await completion before continuing with tick progression. This properly sequences the asynchronous pathfinding operations.src/core/execution/TransportShipExecution.ts (1)
66-66: LGTM! Consistent async WASM pathfinding conversion.Mirrors the pattern from TradeShipExecution: switches to
PathFinder.Wasm, marks tick async, and awaits the pathfinding call. All existing control flow and error handling remain intact.Also applies to: 158-158, 183-183
tests/core/executions/SAMLauncherExecution.test.ts (1)
70-70: LGTM! Comprehensive async test conversion.All tick execution calls are properly awaited throughout the test suite. The beforeEach setup, utility function calls (constructionExecution, executeTicks), and individual tick advances all correctly handle the new async execution model. Test logic and assertions remain unchanged.
Also applies to: 78-78, 94-94, 120-120, 139-148, 167-167, 190-190, 225-225, 240-240
| ticks(): Tick; | ||
| inSpawnPhase(): boolean; | ||
| executeNextTick(): GameUpdates; | ||
| executeNextTick(): Promise<GameUpdates>; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify all callers handle the promise.
The method now returns Promise<GameUpdates>, requiring all callers to await. Based on the test files and GameRunner changes, this appears consistently applied.
Run this script to confirm all callers properly await:
🏁 Script executed:
#!/bin/bash
# Find all executeNextTick calls and verify they're awaited
rg -nP --type=ts -C3 '\bexecuteNextTick\s*\(\)' | rg -v '(await|\.then|async)'Length of output: 27675
🏁 Script executed:
#!/bin/bash
# Show actual lines with executeNextTick calls in tests
echo "--- Tests ---"
rg -nP --type=ts '\bexecuteNextTick\s*\(\)' -n tests
# Show calls in GameRunner
echo "--- GameRunner ---"
rg -nP --type=ts '\bexecuteNextTick\s*\(' -n src/core/GameRunner.ts
# Show calls in Worker
echo "--- Worker ---"
rg -nP --type=ts '\bexecuteNextTick\s*\(' -n src/core/worker/Worker.worker.tsLength of output: 6629
Add missing await for executeNextTick in Worker.worker.ts
- At src/core/worker/Worker.worker.ts:40, the call
(await gameRunner)?.executeNextTick();isn’t awaited—change it to:
-(await gameRunner)?.executeNextTick();
+await (await gameRunner)?.executeNextTick();🤖 Prompt for AI Agents
In src/core/worker/Worker.worker.ts around line 40, the call currently uses
(await gameRunner)?.executeNextTick(); which awaits gameRunner but does not
await the executeNextTick Promise; change it to await the executeNextTick
invocation (e.g., await (await gameRunner)?.executeNextTick() or split into
const runner = await gameRunner; await runner?.executeNextTick();) so that
executeNextTick is properly awaited.
| // If no cluster owns the station, creates a new one for it | ||
| const newCluster = new Cluster(); | ||
| newCluster.addStation(station); | ||
| newCluster.addStations([station]); | ||
| } |
There was a problem hiding this comment.
Call addStation for a single station
Cluster.addStations expects a Set<TrainStation>, so passing an array triggers TS2345. Use the existing single-station helper instead.
- const newCluster = new Cluster();
- newCluster.addStations([station]);
+ const newCluster = new Cluster();
+ newCluster.addStation(station);📝 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.
| // If no cluster owns the station, creates a new one for it | |
| const newCluster = new Cluster(); | |
| newCluster.addStation(station); | |
| newCluster.addStations([station]); | |
| } | |
| // If no cluster owns the station, creates a new one | |
| const newCluster = new Cluster(); | |
| newCluster.addStation(station); | |
| } |
🧰 Tools
🪛 GitHub Actions: 🧪 CI
[error] 137-137: TS2345: Argument of type 'TrainStation[]' is not assignable to parameter of type 'Set'.
🤖 Prompt for AI Agents
In src/core/game/RailNetworkImpl.ts around lines 135 to 138, replace the call
that passes an array to Cluster.addStations (which expects a Set<TrainStation>)
by calling the single-station helper: invoke Cluster.addStation(station) instead
of newCluster.addStations([station]); ensure you use the existing addStation
method so the type matches and no Set conversion is required.
| if ( | ||
| pathResult.type === PathFindResultType.Completed && | ||
| wasmPathFinder.path | ||
| ) { | ||
| const path = wasmPathFinder.path; | ||
| if ( | ||
| path.length > 0 && | ||
| path.length < this.game.config().railroadMaxSize() | ||
| ) { | ||
| const railRoad = new Railroad(from, to, path); | ||
| this.game.addExecution(new RailroadExecution(railRoad)); | ||
| from.addRailroad(railRoad); | ||
| to.addRailroad(railRoad); | ||
| return true; |
There was a problem hiding this comment.
Handle the wasm result correctly
WasmPathFinder.nextTile returns PathFindResultType.NextTile when it successfully produces a path step; it almost never returns Completed (that only happens when the ships are already adjacent). As written, this guard never fires, so connect always falls through and no railroads ever get built. On top of that, wasmPathFinder.path is a private field, so this code cannot even read the path array.
Please update the contract so rail-building receives the full path through a public API—either expose a getter or return the path alongside the result—and branch on the success state that the wasm code actually emits. For example, add a dedicated findPath() method that returns TileRef[] and use that array here instead of peeking at a private member.
🧰 Tools
🪛 GitHub Actions: 🧪 CI
[error] 166-166: TS2341: Property 'path' is private and only accessible within class 'WasmPathFinder'.
| const fixedWidth = 16; // Use fixed width | ||
| const fixedHeight = 16; // Use fixed height | ||
|
|
||
| const gridData = new Array(fixedWidth * fixedHeight); | ||
| for (let y = 0; y < fixedHeight; y++) { | ||
| for (let x = 0; x < fixedWidth; x++) { | ||
| const tile = this.game.map().ref(x, y); | ||
| gridData[y * fixedWidth + x] = this.game.map().isWater(tile) ? 1 : 0; | ||
| } | ||
| } | ||
|
|
||
| console.log( | ||
| `WasmPathFinder.nextTile: fixedWidth=${fixedWidth} fixedHeight=${fixedHeight} gridData.length=${gridData.length}`, | ||
| ); | ||
|
|
||
| const path = wasm.find_path( | ||
| this.game.map().x(curr), | ||
| this.game.map().y(curr), | ||
| this.game.map().x(dst), | ||
| this.game.map().y(dst), | ||
| fixedWidth, | ||
| fixedHeight, | ||
| gridData, | ||
| ); | ||
|
|
||
| if (path) { |
There was a problem hiding this comment.
Use map dimensions instead of 16×16
Hard-coding the grid to 16×16 makes find_path useless on real maps. Any tile with x/y ≥ 16 sits outside this grid, so the wasm routine reads wrong cells and paths are never constructed. Please derive the grid from the game map dimensions (or at least cover the bounding box that includes curr and dst), and pass those dimensions to the wasm call.
- const fixedWidth = 16; // Use fixed width
- const fixedHeight = 16; // Use fixed height
-
- const gridData = new Array(fixedWidth * fixedHeight);
- for (let y = 0; y < fixedHeight; y++) {
- for (let x = 0; x < fixedWidth; x++) {
- const tile = this.game.map().ref(x, y);
- gridData[y * fixedWidth + x] = this.game.map().isWater(tile) ? 1 : 0;
- }
- }
-
- const path = wasm.find_path(
+ const width = this.game.map().width();
+ const height = this.game.map().height();
+
+ const gridData = new Array(width * height);
+ for (let y = 0; y < height; y++) {
+ for (let x = 0; x < width; x++) {
+ const tile = this.game.map().ref(x, y);
+ gridData[y * width + x] = this.game.map().isWater(tile) ? 1 : 0;
+ }
+ }
+
+ const path = wasm.find_path(
this.game.map().x(curr),
this.game.map().y(curr),
this.game.map().x(dst),
this.game.map().y(dst),
- fixedWidth,
- fixedHeight,
+ width,
+ height,
gridData,
);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/core/pathfinding/WasmPathfinding.ts around lines 79–104, the code
hard-codes fixedWidth/fixedHeight = 16 which corrupts lookups for tiles with x/y
≥ 16; replace the fixed 16×16 with the actual map dimensions (e.g., mapWidth =
this.game.map().width(), mapHeight = this.game.map().height()) or at minimum
compute a bounding box that contains curr and dst (minX..maxX, minY..maxY),
allocate gridData = new Array(width*height), fill gridData using the chosen
coordinate range with index = (y - originY)*width + (x - originX) and use
this.game.map().ref/isWater for each cell, and then pass the computed width,
height and appropriately offset coordinates (subtract originX/originY if using a
bounding box) into wasm.find_path so the wasm routine receives correct
dimensions and coordinates.
| global.fetch = jest.fn((url: RequestInfo | URL) => { | ||
| const urlString = typeof url === "string" ? url : url.toString(); | ||
| if (urlString.endsWith("pathfinding_bg.wasm")) { | ||
| const wasmBuffer = readFileSync( | ||
| join(__dirname, "../static/js/pathfinding_bg.wasm"), | ||
| ); | ||
| return Promise.resolve({ | ||
| arrayBuffer: () => Promise.resolve(wasmBuffer.buffer), | ||
| headers: new Headers(), | ||
| ok: true, | ||
| redirected: false, | ||
| status: 200, | ||
| statusText: "OK", | ||
| type: "basic", | ||
| url: urlString, | ||
| clone: () => global.fetch(url), | ||
| } as Response); | ||
| } | ||
| return Promise.reject(new Error(`Unhandled fetch request for: ${urlString}`)); | ||
| }); |
There was a problem hiding this comment.
Fix TypeScript type error in fetch mock.
The pipeline reports a type conversion error on line 11. The mock Response object is missing some required Response properties. Cast to unknown first, then to Response to satisfy TypeScript while maintaining the necessary mock functionality.
Apply this diff:
- return Promise.resolve({
+ return Promise.resolve({
arrayBuffer: () => Promise.resolve(wasmBuffer.buffer),
headers: new Headers(),
ok: true,
redirected: false,
status: 200,
statusText: "OK",
type: "basic",
url: urlString,
clone: () => global.fetch(url),
- } as Response);
+ } as unknown as Response);📝 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.
| global.fetch = jest.fn((url: RequestInfo | URL) => { | |
| const urlString = typeof url === "string" ? url : url.toString(); | |
| if (urlString.endsWith("pathfinding_bg.wasm")) { | |
| const wasmBuffer = readFileSync( | |
| join(__dirname, "../static/js/pathfinding_bg.wasm"), | |
| ); | |
| return Promise.resolve({ | |
| arrayBuffer: () => Promise.resolve(wasmBuffer.buffer), | |
| headers: new Headers(), | |
| ok: true, | |
| redirected: false, | |
| status: 200, | |
| statusText: "OK", | |
| type: "basic", | |
| url: urlString, | |
| clone: () => global.fetch(url), | |
| } as Response); | |
| } | |
| return Promise.reject(new Error(`Unhandled fetch request for: ${urlString}`)); | |
| }); | |
| global.fetch = jest.fn((url: RequestInfo | URL) => { | |
| const urlString = typeof url === "string" ? url : url.toString(); | |
| if (urlString.endsWith("pathfinding_bg.wasm")) { | |
| const wasmBuffer = readFileSync( | |
| join(__dirname, "../static/js/pathfinding_bg.wasm"), | |
| ); | |
| return Promise.resolve({ | |
| arrayBuffer: () => Promise.resolve(wasmBuffer.buffer), | |
| headers: new Headers(), | |
| ok: true, | |
| redirected: false, | |
| status: 200, | |
| statusText: "OK", | |
| type: "basic", | |
| url: urlString, | |
| clone: () => global.fetch(url), | |
| } as unknown as Response); | |
| } | |
| return Promise.reject(new Error(`Unhandled fetch request for: ${urlString}`)); | |
| }); |
🧰 Tools
🪛 GitHub Actions: 🧪 CI
[error] 11-11: Conversion of type '{ arrayBuffer: () => Promise; headers: Headers; ok: true; redirected: false; status: number; statusText: string; type: "basic"; url: string; clone: () => Promise<...>; }' to type 'Response' may be a mistake because neither type sufficiently overlaps with the other.
🤖 Prompt for AI Agents
In tests/setupJest.ts around lines 5 to 24, the mocked Response object lacks
some required Response properties causing a TypeScript type error; to fix it,
change the final type assertion to cast the mock to unknown first and then to
Response (e.g., replace the current "as Response" with "as unknown as Response")
so TypeScript accepts the partial mock while preserving its runtime behavior.
| let wasm: typeof import("../wasm/pathfinding/pkg"); | ||
|
|
||
| async function loadWasm() { | ||
| if (!wasm) { | ||
| const pkg = await import("../wasm/pathfinding/pkg"); | ||
|
|
||
| // Manually load the wasm file | ||
| const wasmPath = join(__dirname, "../static/js/pathfinding_bg.wasm"); | ||
| const wasmBuffer = readFileSync(wasmPath); | ||
| const wasmModule = await WebAssembly.compile(wasmBuffer); | ||
|
|
||
| wasm = await pkg.default({ data: wasmModule }); // Pass the compiled module to init | ||
| } |
There was a problem hiding this comment.
Critical: WASM package not found—verify build pipeline.
The pipeline error confirms that ../wasm/pathfinding/pkg doesn't exist at test time. This is a blocker.
Ensure the WASM build step (likely wasm-pack build) runs before tests and outputs to the expected location. The test assumes the package is prebuilt and the compiled .wasm binary is available at ../static/js/pathfinding_bg.wasm.
Verify the build order and package path:
#!/bin/bash
# Description: Check if wasm-pack build outputs exist and verify paths
# Check for the pkg directory
fd -t d "pkg" wasm/pathfinding
# Check for the wasm binary in static
fd -t f "pathfinding_bg.wasm" static
# Check for wasm-pack configuration
fd -t f "Cargo.toml" wasm/pathfinding
# Look for build scripts or CI steps that should run wasm-pack
rg -n "wasm-pack" -g "*.{json,yml,yaml,sh}"🧰 Tools
🪛 GitHub Actions: 🧪 CI
[error] 4-4: Cannot find module '../wasm/pathfinding/pkg' or its corresponding type declarations.
🤖 Prompt for AI Agents
In tests/wasm_test.test.ts around lines 4-16, the test imports
../wasm/pathfinding/pkg which isn't present at test time because the WASM
package/build step didn't run; ensure the WASM is produced and paths match by
adding a pretest/build step (e.g., run wasm-pack build with the correct out-dir
or your repository's build command) so ../wasm/pathfinding/pkg exists and the
compiled .wasm is emitted to ../static/js/pathfinding_bg.wasm, or alternatively
adjust the test to import the actual output path or mock the module if you
cannot build during tests.
| if x < width - 1 { | ||
| successors.push(Point { x: x + 1, y }); | ||
| } | ||
| if y > 0 { | ||
| successors.push(Point { x, y: y - 1 }); | ||
| } | ||
| if y < height - 1 { | ||
| successors.push(Point { x, y: y + 1 }); |
There was a problem hiding this comment.
Protect against zero-width/height grids.
When width or height is 0, the checks width - 1 / height - 1 underflow and panic on debug builds (wrap in release). Add an early return or use checked subtraction before building neighbors.(doc.rust-lang.org)
- if x < width - 1 {
+ if let Some(limit) = width.checked_sub(1) {
+ if x < limit {
+ successors.push(Point { x: x + 1, y });
+ }
+ } else {
+ return Vec::new();
+ }
- successors.push(Point { x: x + 1, y });
- }
- if y < height - 1 {
+ if let Some(limit) = height.checked_sub(1) {
+ if y < limit {
+ successors.push(Point { x, y: y + 1 });
+ }
+ } else {
+ return Vec::new();
+ }
- successors.push(Point { x, y: y + 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.
| if x < width - 1 { | |
| successors.push(Point { x: x + 1, y }); | |
| } | |
| if y > 0 { | |
| successors.push(Point { x, y: y - 1 }); | |
| } | |
| if y < height - 1 { | |
| successors.push(Point { x, y: y + 1 }); | |
| // Protect against width == 0 underflow | |
| if let Some(limit) = width.checked_sub(1) { | |
| if x < limit { | |
| successors.push(Point { x: x + 1, y }); | |
| } | |
| } else { | |
| return Vec::new(); | |
| } | |
| if y > 0 { | |
| successors.push(Point { x, y: y - 1 }); | |
| } | |
| // Protect against height == 0 underflow | |
| if let Some(limit) = height.checked_sub(1) { | |
| if y < limit { | |
| successors.push(Point { x, y: y + 1 }); | |
| } | |
| } else { | |
| return Vec::new(); | |
| } |
🤖 Prompt for AI Agents
In wasm/pathfinding/src/lib.rs around lines 63 to 70, the neighbor-generation
branch uses expressions like width - 1 and height - 1 which underflow when width
or height is 0; add a guard before building successors to return early (or skip
neighbor generation) when width == 0 || height == 0, or use checked subtraction
(e.g., width.checked_sub(1) / height.checked_sub(1)) in the conditional checks
so no underflow occurs, then proceed to push neighbors only when the checks
succeed.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/setupJest.ts (1)
11-21: Apply the double-cast to fix TypeScript type error.The pipeline error persists because the mock Response object is missing required properties. Cast to
unknownfirst, then toResponseto satisfy TypeScript while keeping the mock functional.Apply this diff:
return Promise.resolve({ arrayBuffer: () => Promise.resolve(wasmBuffer.buffer), headers: new Headers(), ok: true, redirected: false, status: 200, statusText: "OK", type: "basic", url: urlString, clone: () => global.fetch(url), - } as Response); + } as unknown as Response);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/setupJest.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/setupJest.ts (1)
src/core/game/FetchGameMapLoader.ts (1)
url(41-49)
🪛 GitHub Actions: 🧪 CI
tests/setupJest.ts
[error] 11-11: TS2352: Conversion of type 'Response' to type 'Response' may be a mistake while mocking fetch; adjust mock typings
[error] 33-33: TS2345: Argument type mismatch for WebAssembly.instantiate; adjust types or mocks
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/setupJest.ts (1)
5-24: Fix TypeScript type error in fetch mock (duplicate).The pipeline still reports a type conversion error on line 11. Apply the fix from the previous review comment by casting to
unknownfirst, then toResponse.Apply this diff:
return Promise.resolve({ arrayBuffer: () => Promise.resolve(wasmBuffer.buffer), headers: new Headers(), ok: true, redirected: false, status: 200, statusText: "OK", type: "basic", url: urlString, clone: () => global.fetch(url), - } as Response); + } as unknown as Response);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/setupJest.ts(1 hunks)
🧰 Additional context used
🪛 Biome (2.1.2)
tests/setupJest.ts
[error] 41-41: expected , but instead found const
Remove const
(parse)
[error] 43-43: Illegal return statement outside of a function
(parse)
[error] 44-45: Expected a statement but instead found '},
)'.
Expected a statement here.
(parse)
🪛 GitHub Actions: 🧪 CI
tests/setupJest.ts
[error] 41-41: TS1005: ',' expected.
[error] 44-44: TS1128: Declaration or statement expected.
[error] 44-44: TS1128: Declaration or statement expected.
[error] 45-45: TS1128: Declaration or statement expected.
[error] 11-11: TS2352: Conversion of type '{ arrayBuffer: () => Promise; headers: Headers; ok: true; redirected: false; status: number; statusText: string; type: "basic"; url: string; clone: () => Promise<...>; }' to type 'Response' may be a mistake because neither type sufficiently overlaps with the other.
🪛 GitHub Check: 🔍 ESLint
tests/setupJest.ts
[failure] 41-41:
Parsing error: ',' expected.
| } | ||
| const response = await source; | ||
| const buffer = await response.arrayBuffer(); | ||
| return WebAssembly.instantiate(buffer, importObject); |
There was a problem hiding this comment.
Remove duplicate code causing syntax errors.
Lines 41-43 duplicate lines 37-39 and appear after the async function has already closed at line 40. This causes all the parser and TypeScript errors reported by the pipeline. Remove the duplicate code block.
Apply this diff:
const response = await source;
const buffer = await response.arrayBuffer();
return WebAssembly.instantiate(buffer, importObject);
},
- }
- const response = await source;
- const buffer = await response.arrayBuffer();
- return WebAssembly.instantiate(buffer, importObject);
);📝 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 response = await source; | |
| const buffer = await response.arrayBuffer(); | |
| return WebAssembly.instantiate(buffer, importObject); | |
| const response = await source; | |
| const buffer = await response.arrayBuffer(); | |
| return WebAssembly.instantiate(buffer, importObject); | |
| }, | |
| ); |
🧰 Tools
🪛 Biome (2.1.2)
[error] 41-41: expected , but instead found const
Remove const
(parse)
[error] 43-43: Illegal return statement outside of a function
(parse)
🪛 GitHub Actions: 🧪 CI
[error] 41-41: TS1005: ',' expected.
🪛 GitHub Check: 🔍 ESLint
[failure] 41-41:
Parsing error: ',' expected.
🤖 Prompt for AI Agents
In tests/setupJest.ts around lines 40 to 43, there is a duplicated block that
repeats the async fetch/instantiate sequence already present at lines ~37-39 and
sits after the async function has closed, causing parser/TypeScript errors;
remove the duplicate lines 41-43 so only the original async function body
remains and the file has no stray/duplicated closing or instantiation code.
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