build: migrate build system to Vite and test runner to Vitest & Remove depracated husky usage #2703
Conversation
WalkthroughMigrate build/test toolchain from Webpack+Jest to Vite+Vitest, normalize asset imports to Vite (raw/url/JSON), add a runtime-safe Env helper, switch map/binary loading to fetch-based loaders, make workers module-based, and update tests/types for Vitest. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer / CI
participant Vite as Vite (dev/build)
participant Browser as Browser App
participant CDN as Static assets / server
participant Worker as Web Worker (module)
rect rgb(240,248,255)
Note right of Dev: Run dev / build (npm scripts)
Dev->>Vite: start dev / build
Vite-->>Dev: serve ESM modules & asset URLs / emit static files
end
rect rgb(255,250,240)
Browser->>Browser: load /index.html → import /src/client/Main.ts (type: module)
Browser->>CDN: GET /manifest.json, /images/*, /sprites/*
CDN-->>Browser: assets (JSON, images, sprites)
end
rect rgb(240,255,240)
Browser->>CDN: GET /maps/<map>/map.bin?v=<cacheBuster>
CDN-->>Browser: ArrayBuffer
Browser->>Browser: ArrayBuffer → Uint8Array (loadBinary) → parse map manifest
end
rect rgb(255,245,255)
Browser->>Worker: new Worker("/workers/Worker.worker.js",{type:"module"})
Worker->>CDN: fetch("/assets/data/version.txt?raw")
CDN-->>Worker: raw text
Worker->>Browser: postMessage(processed result)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (1)
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/AutoUpgrade.test.ts (1)
1-3: Remove outdated Jest environment comment.The
@jest-environment jsdomcomment is a Jest-specific directive. In Vitest, the environment is configured invite.config.ts(line 18:environment: "jsdom"). This comment is now dead code.Suggested fix
-/** - * @jest-environment jsdom - */ import { AutoUpgradeEvent } from "../src/client/InputHandler";
🧹 Nitpick comments (11)
tests/client/components/FluentSlider.test.ts (1)
1-3: Stale JSDoc comment references Jest.The
@jest-environment jsdomcomment is outdated after migrating to Vitest. While this may not cause issues, consider removing it or updating to Vitest-specific syntax if needed.Optional cleanup
-/** - * @jest-environment jsdom - */ import { FluentSlider } from "../../../src/client/components/FluentSlider";tests/client/graphics/ProgressBar.test.ts (1)
1-3: Stale JSDoc comment references Jest.Same as FluentSlider.test.ts - the
@jest-environment jsdomcomment is outdated.src/client/styles.css (1)
5-9: Vendor prefixes still present but could be simplified.Modern browsers support the standard
box-sizingproperty without vendor prefixes. The-webkit-and-moz-prefixes are no longer necessary.Optional simplification
* { - -webkit-box-sizing: border-box; - -moz-box-sizing: border-box; box-sizing: border-box; }src/core/game/BinaryLoaderGameMapLoader.ts (1)
35-41: Consider movingloadBinaryto class level.This helper is recreated on every
getMapDatacall. While harmless due to the lazy-loading cache, moving it to a private method or module-level function is cleaner.Suggested refactor
export class BinaryLoaderGameMapLoader implements GameMapLoader { private maps: Map<GameMapType, MapData>; constructor() { this.maps = new Map<GameMapType, MapData>(); } + private loadBinary(url: string): Promise<Uint8Array> { + return fetch(url) + .then((res) => { + if (!res.ok) throw new Error(`Failed to load ${url}`); + return res.arrayBuffer(); + }) + .then((buf) => new Uint8Array(buf)); + } + private createLazyLoader<T>(importFn: () => Promise<T>): () => Promise<T> {Then update usages to
this.loadBinary(m.default).src/client/graphics/layers/PlayerPanel.ts (1)
3-3: Verify ifwith { type: "json" }is required for Vite.The import path update is correct, but the
with { type: "json" }syntax may be unnecessary. Based on learnings, the TypeScript configuration supportsresolveJsonModuleand the codebase previously imported JSON without import attributes successfully.While this is the modern ES module syntax, Vite might handle JSON imports automatically through TypeScript's configuration. Consider testing if the import works without the attribute:
-import Countries from "../../../../resources/countries.json" with { type: "json" }; +import Countries from "../../../../resources/countries.json";If Vite requires the attribute, that's fine—just confirming the necessity.
Based on learnings, the codebase previously used standard JSON imports without attributes.
src/core/Schemas.ts (1)
2-2: Verify ifwith { type: "json" }is required for Vite.The resource path update is correct, but the
with { type: "json" }attribute may be unnecessary (same concern as in PlayerPanel.ts). The codebase previously imported JSON successfully without import attributes because TypeScript'sresolveJsonModuleis enabled.Consider testing if the import works without the attribute:
-import countries from "../../resources/countries.json" with { type: "json" }; +import countries from "../../resources/countries.json";If Vite requires explicit type attributes for JSON imports, that's valid—just confirming it's needed.
Based on learnings, the codebase previously used standard JSON imports without attributes.
src/core/configuration/DefaultConfig.ts (1)
226-228: Duplicate stripePublishableKey() in DefaultConfig class.This mirrors the same method in
DefaultServerConfig(line 97-99). Both useEnv.STRIPE_PUBLISHABLE_KEY. Consider whetherDefaultConfigshould delegate tothis._serverConfig.stripePublishableKey()instead of re-reading from Env directly to avoid duplication.🔎 Suggested refactor to reduce duplication
stripePublishableKey(): string { - return Env.STRIPE_PUBLISHABLE_KEY ?? ""; + return this._serverConfig.stripePublishableKey(); }src/client/vite-env.d.ts (1)
23-26: Verify .webp import behavior.By default, Vite treats image imports as URLs (returning a path string), not raw content. This declaration says it returns a string, which is technically correct for URL strings, but the variable name
webpContentsuggests raw content.Consider renaming for clarity or adding a comment:
🔎 Suggested clarification
declare module "*.webp" { - const webpContent: string; - export default webpContent; + /** URL path to the webp asset */ + const webpUrl: string; + export default webpUrl; }tests/InputHandler.test.ts (1)
1-3: Stale Jest environment comment.The
@jest-environment jsdomcomment is Jest-specific and has no effect in Vitest. Vitest configures the environment invitest.config.tsorvite.config.ts.🔎 Remove stale comment
-/** - * @jest-environment jsdom - */ import { AutoUpgradeEvent, InputHandler } from "../src/client/InputHandler";package.json (2)
4-6: Scripts migration looks correct.The build scripts properly run type checking (
tsc --noEmit) and Vite build in parallel. Thestart:clientscript correctly delegates to Vite's dev server.One small note: if you want the build to fail fast when either command errors, consider adding
--kill-others-on-failtoconcurrently. By default, concurrently continues running other commands even if one fails.- "build-dev": "concurrently \"tsc --noEmit\" \"vite build --mode development\"", - "build-prod": "concurrently \"tsc --noEmit\" \"vite build\"", + "build-dev": "concurrently --kill-others-on-fail \"tsc --noEmit\" \"vite build --mode development\"", + "build-prod": "concurrently --kill-others-on-fail \"tsc --noEmit\" \"vite build\"",
32-34: Remove unused Babel dependencies.These packages are not needed with Vite's esbuild transpiler and have no configuration files or references in the codebase:
@babel/core@babel/preset-env@babel/preset-typescriptRemoving them will reduce install size and maintenance burden.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (48)
.husky/pre-commitindex.htmljest.config.tspackage.jsonresources/countries.jsonsrc/client/FlagInputModal.tssrc/client/Main.tssrc/client/NewsModal.tssrc/client/TerrainMapFileLoader.tssrc/client/graphics/layers/ChatModal.tssrc/client/graphics/layers/PlayerPanel.tssrc/client/graphics/layers/StructureIconsLayer.tssrc/client/styles.csssrc/client/vite-env.d.tssrc/core/Schemas.tssrc/core/configuration/ConfigLoader.tssrc/core/configuration/DefaultConfig.tssrc/core/configuration/Env.tssrc/core/game/BinaryLoaderGameMapLoader.tssrc/core/game/FetchGameMapLoader.tssrc/core/worker/Worker.worker.tssrc/core/worker/WorkerClient.tssrc/global.d.tstailwind.config.jstests/AllianceExtensionExecution.test.tstests/AutoUpgrade.test.tstests/Censor.test.tstests/DeleteUnitExecution.test.tstests/InputHandler.test.tstests/MessageTypeClasses.test.tstests/NationAllianceBehavior.test.tstests/client/components/FluentSlider.test.tstests/client/graphics/ProgressBar.test.tstests/client/graphics/RadialMenuElements.test.tstests/client/graphics/UILayer.test.tstests/core/executions/NukeExecution.test.tstests/core/executions/TradeShipExecution.test.tstests/core/executions/WinCheckExecution.test.tstests/core/game/Cluster.test.tstests/core/game/GameImpl.test.tstests/core/game/RailNetwork.test.tstests/core/game/TrainStation.test.tstests/global.d.tstests/radialMenuElements.spec.tstests/setup.tstsconfig.jsonvite.config.tswebpack.config.js
💤 Files with no reviewable changes (4)
- src/global.d.ts
- jest.config.ts
- tests/global.d.ts
- webpack.config.js
🧰 Additional context used
🧠 Learnings (24)
📚 Learning: 2025-10-20T20:15:28.858Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:51-51
Timestamp: 2025-10-20T20:15:28.858Z
Learning: In src/core/execution/FakeHumanExecution.ts, game balance constants like MIRV_COOLDOWN_TICKS, MIRV_HESITATION_ODDS, VICTORY_DENIAL_TEAM_THRESHOLD, VICTORY_DENIAL_INDIVIDUAL_THRESHOLD, and STEAMROLL_CITY_GAP_MULTIPLIER are experimental tuning parameters subject to frequent change during balance testing. Do not flag changes to these values as issues or compare them against previous values.
Applied to files:
tests/core/executions/NukeExecution.test.tstests/NationAllianceBehavior.test.tstests/core/executions/WinCheckExecution.test.ts
📚 Learning: 2025-10-18T17:54:01.311Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:172-173
Timestamp: 2025-10-18T17:54:01.311Z
Learning: In src/core/execution/FakeHumanExecution.ts, MIRVs and ground attacks should not be mutually exclusive. The considerMIRV() method should not short-circuit maybeAttack() - both actions can occur in the same tick.
Applied to files:
tests/core/executions/NukeExecution.test.tstests/DeleteUnitExecution.test.tstests/core/game/GameImpl.test.tstests/AllianceExtensionExecution.test.tstests/core/executions/WinCheckExecution.test.tstests/core/executions/TradeShipExecution.test.tstests/client/graphics/RadialMenuElements.test.ts
📚 Learning: 2025-10-20T11:02:16.969Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:57-57
Timestamp: 2025-10-20T11:02:16.969Z
Learning: In src/core/execution/FakeHumanExecution.ts, the correct MIRV victory denial thresholds are VICTORY_DENIAL_TEAM_THRESHOLD = 0.8 (80% for team games) and VICTORY_DENIAL_INDIVIDUAL_THRESHOLD = 0.65 (65% for individual players), not 0.85 and 0.7 as might be mentioned in some documentation.
Applied to files:
tests/core/executions/NukeExecution.test.tstests/NationAllianceBehavior.test.tstests/core/executions/WinCheckExecution.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:
tests/core/executions/NukeExecution.test.ts
📚 Learning: 2025-08-24T11:13:08.495Z
Learnt from: DevelopingTom
Repo: openfrontio/OpenFrontIO PR: 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/NukeExecution.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/core/game/RailNetwork.test.tstests/core/game/Cluster.test.tstests/core/game/TrainStation.test.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:
tests/DeleteUnitExecution.test.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:
tests/DeleteUnitExecution.test.tstests/core/game/GameImpl.test.tstests/client/graphics/RadialMenuElements.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:
tests/DeleteUnitExecution.test.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:
tests/NationAllianceBehavior.test.ts
📚 Learning: 2025-08-23T08:03:44.914Z
Learnt from: ElMelchizedek
Repo: openfrontio/OpenFrontIO PR: 1876
File: src/core/execution/FakeHumanExecution.ts:547-592
Timestamp: 2025-08-23T08:03:44.914Z
Learning: In OpenFrontIO, FakeHumanExecution is used for AI-controlled nations that simulate human behavior, which is distinct from PlayerType.Bot. FakeHumanExecution players are not PlayerType.Bot - they represent more sophisticated AI that mimics human nation behavior, while PlayerType.Bot represents basic AI players.
Applied to files:
tests/NationAllianceBehavior.test.ts
📚 Learning: 2025-06-07T13:15:55.439Z
Learnt from: Aotumuri
Repo: openfrontio/OpenFrontIO PR: 786
File: src/core/Util.ts:4-4
Timestamp: 2025-06-07T13:15:55.439Z
Learning: In the OpenFrontIO codebase, JSON files should be imported using standard import syntax without import attributes, as the TypeScript configuration supports resolveJsonModule and the codebase already uses this pattern successfully in files like src/client/Cosmetic.ts.
Applied to files:
src/client/Main.tssrc/client/vite-env.d.tssrc/client/graphics/layers/ChatModal.tssrc/core/Schemas.tssrc/client/FlagInputModal.tssrc/client/graphics/layers/PlayerPanel.tssrc/client/NewsModal.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/Main.tsindex.html
📚 Learning: 2025-07-26T05:35:03.686Z
Learnt from: aaa4xu
Repo: openfrontio/OpenFrontIO PR: 1581
File: webpack.config.js:139-141
Timestamp: 2025-07-26T05:35:03.686Z
Learning: In OpenFrontIO's webpack configuration, binary-loader processes files from resources/maps at build-time and inlines the binary payload directly into the compiled JS bundle. This means the original .bin assets are no longer needed at runtime and can be safely excluded from the final Docker image using webpack's globOptions.ignore pattern to reduce image size.
Applied to files:
src/core/game/BinaryLoaderGameMapLoader.tssrc/client/vite-env.d.ts
📚 Learning: 2025-11-12T23:11:34.445Z
Learnt from: MaxHT0x
Repo: openfrontio/OpenFrontIO PR: 2262
File: src/core/configuration/DefaultConfig.ts:806-832
Timestamp: 2025-11-12T23:11:34.445Z
Learning: In src/core/configuration/DefaultConfig.ts, JSDoc documentation for configuration methods should not be added inline, as it was requested that documentation be placed elsewhere in the codebase.
Applied to files:
src/core/configuration/DefaultConfig.ts
📚 Learning: 2025-05-18T23:36:12.847Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 784
File: src/core/game/StatsImpl.ts:143-159
Timestamp: 2025-05-18T23:36:12.847Z
Learning: In this codebase, NukeType is a union type derived from UnitType values (specifically bomb-related values like AtomBomb, HydrogenBomb, MIRV, and MIRVWarhead) rather than a separate enum. This means comparing NukeType values against UnitType values in switch statements is valid and intentional.
Applied to files:
src/core/configuration/DefaultConfig.ts
📚 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:
src/core/Schemas.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/Schemas.tssrc/client/graphics/layers/PlayerPanel.ts
📚 Learning: 2025-08-09T05:03:18.745Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1761
File: src/core/ApiSchemas.ts:2-2
Timestamp: 2025-08-09T05:03:18.745Z
Learning: After migrating to Zod v4 in the OpenFrontIO project, the standard import statement `import { z } from "zod"` should be used across the entire codebase, not `import { z } from "zod/v4"`.
Applied to files:
src/core/Schemas.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:
tests/client/graphics/RadialMenuElements.test.tstests/radialMenuElements.spec.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:
tests/client/graphics/RadialMenuElements.test.ts
📚 Learning: 2025-05-26T09:52:52.465Z
Learnt from: tnhnblgl
Repo: openfrontio/OpenFrontIO PR: 875
File: src/client/index.html:396-402
Timestamp: 2025-05-26T09:52:52.465Z
Learning: In the OpenFrontIO project, the correct path for icons from src/client/index.html is ../../resources/icons/, not /static/icons/ as the build process handles the path mapping differently.
Applied to files:
index.html
📚 Learning: 2025-07-19T18:12:24.553Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1417
File: src/client/index.html:335-350
Timestamp: 2025-07-19T18:12:24.553Z
Learning: For the OpenFrontIO project: rel="noopener" is not required for internal links (like /privacy-policy.html and /terms-of-service.html) since the project controls the destination pages. The security concern only applies to external, untrusted links.
Applied to files:
index.html
📚 Learning: 2025-08-09T05:14:19.147Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1761
File: src/client/LocalPersistantStats.ts:24-31
Timestamp: 2025-08-09T05:14:19.147Z
Learning: Files in the src/client directory contain browser-only code in the OpenFrontIO project, so browser APIs like localStorage are guaranteed to be available and don't need undefined guards.
Applied to files:
index.html
🧬 Code graph analysis (12)
tests/core/executions/NukeExecution.test.ts (1)
tests/util/TestConfig.ts (1)
TestConfig(12-83)
tests/core/game/Cluster.test.ts (1)
src/core/game/TrainStation.ts (2)
TrainStation(74-172)Cluster(200-249)
tests/DeleteUnitExecution.test.ts (1)
src/server/GameManager.ts (1)
game(25-27)
tests/core/game/GameImpl.test.ts (1)
src/core/game/AttackImpl.ts (1)
attacker(29-31)
tests/NationAllianceBehavior.test.ts (1)
src/core/game/Game.ts (2)
Tick(23-23)AllianceRequest(384-391)
vite.config.ts (2)
src/core/configuration/DefaultConfig.ts (1)
gitCommit(142-144)eslint.config.js (1)
__dirname(10-10)
tests/core/executions/WinCheckExecution.test.ts (1)
src/core/execution/WinCheckExecution.ts (1)
WinCheckExecution(15-107)
tests/core/executions/TradeShipExecution.test.ts (1)
src/core/execution/TradeShipExecution.ts (2)
dstPort(186-188)TradeShipExecution(15-189)
src/core/game/BinaryLoaderGameMapLoader.ts (1)
src/core/game/FetchGameMapLoader.ts (1)
url(41-51)
src/core/configuration/DefaultConfig.ts (1)
src/core/configuration/Env.ts (1)
Env(47-92)
tests/client/graphics/RadialMenuElements.test.ts (2)
src/core/game/GameView.ts (2)
PlayerView(185-578)GameView(580-906)src/client/graphics/layers/RadialMenuElements.ts (2)
attackMenuElement(413-424)buildMenuElement(499-510)
src/core/configuration/ConfigLoader.ts (1)
src/core/configuration/Env.ts (1)
Env(47-92)
🔇 Additional comments (66)
tests/Censor.test.ts (2)
2-26: Migration to Vitest looks correct.The change from
jest.mocktovi.mockis appropriate for the Jest-to-Vitest migration. The mock implementation remains unchanged and correctly provides the obscenity library API.
29-32: Migration tovi.mockis correct and the module path resolves properly.The change from
jest.mocktovi.mockuses the correct Vitest syntax. The Utils module exists atsrc/client/Utils.tsand exports thetranslateTextfunction with a compatible signature. The mock provides a clean, predictable implementation for testing purposes.tests/client/graphics/UILayer.test.ts (1)
31-31: LGTM! Clean Jest to Vitest migration.Mock function replacements are correct.
Also applies to: 53-53
tests/MessageTypeClasses.test.ts (1)
1-1: LGTM! Vitest migration done correctly.Type and API changes match Vitest conventions.
Also applies to: 7-7, 10-10
src/core/worker/WorkerClient.ts (1)
26-28: LGTM! ES module worker setup is correct.Adding
type: "module"enables standard ESM syntax in the worker, which pairs correctly with the?rawimport in Worker.worker.ts.tests/radialMenuElements.spec.ts (1)
1-13: LGTM! Complete Jest to Vitest migration.Mock setup and function replacements follow Vitest conventions correctly.
Also applies to: 58-71
src/core/worker/Worker.worker.ts (1)
1-1: Raw text import is correctly configured for worker context.The
version.txtfile exists at the expected location (resources/version.txt). The path../../../resources/version.txtfromWorker.worker.tsresolves correctly. Vite's?rawquery parameter works in ES module workers and will import the file as a string..husky/pre-commit (1)
2-3: Fix typo in deprecation comment: "Depracated" → "Deprecated".The hook is correctly configured for Husky v9. Per v9 release notes, the
husky.shbootstrap sourcing is deprecated (removed in hooks), and direct command execution—as shown here withnpx lint-staged—is the expected pattern. The lint-staged configuration inpackage.jsonis present and properly set up.# Deprecated with husky v9 #. "$(dirname -- "$0")/_/husky.sh"tests/core/game/RailNetwork.test.ts (2)
10-26: Clean migration from Jest to Vitest mocks.The mock creation function properly updates all Jest mocks to Vitest equivalents. The test logic remains unchanged.
55-77: Vitest mocks correctly configured.All mock setup for game, stationManager, and pathService properly migrated to
vi.fn(). Configuration structure maintained.tests/client/components/FluentSlider.test.ts (1)
7-9: Vitest mock migration looks good.The mock setup correctly uses
vi.mockandvi.fninstead of Jest equivalents.tsconfig.json (2)
10-13: Path mapping configured for Vite.The baseUrl and paths configuration enables cleaner resource imports and aligns with Vite's expectations.
26-27: Vitest types properly configured.Adding
vitest/globalsandnodeto the types array ensures test globals and Node APIs are properly typed throughout the test suite.tests/client/graphics/ProgressBar.test.ts (1)
18-20: Vitest spies correctly implemented.The migration from
jest.spyOntovi.spyOnis properly done. Test behavior remains unchanged.tests/core/executions/TradeShipExecution.test.ts (1)
23-88: Comprehensive Vitest mock migration.All mock objects and functions properly migrated from Jest to Vitest. The test setup maintains identical structure and behavior with updated API calls.
tests/core/executions/WinCheckExecution.test.ts (3)
16-18: Vitest mock properly initialized.The
mg.setWinnermock correctly usesvi.fn()instead of Jest's API.
22-38: Spy and mock configuration migrated correctly.Both
vi.spyOncalls and the nested mock structure usingvi.fn()properly replace Jest equivalents while maintaining test behavior.
42-73: Player and game mocks comprehensive.All mock functions throughout the test scenario use
vi.fn()consistently. The test logic for win conditions remains intact.src/client/styles.css (1)
1-9: No action needed—all CSS imports are properly centralized in Main.ts.CSS imports for variables, typography, layout, components, and modal styles are already present in Main.ts (lines 47–58) and imported correctly. No styles are missing.
Likely an incorrect or invalid review comment.
src/client/graphics/layers/ChatModal.ts (1)
7-7: No changes needed—this import is consistent with the project pattern.The codebase uses import attributes
with { type: "json" }for JSON files across multiple files (Schemas.ts, FlagInputModal.ts, PlayerPanel.ts, and this file). This is the established pattern in the current codebase and works correctly with the Vite build configuration. The earlier learning about removing attributes contradicts the actual codebase state.Likely an incorrect or invalid review comment.
tests/core/game/TrainStation.test.ts (2)
1-8: Clean Vitest migration.The import and mock setup correctly uses
vi.mockandMocked<T>from Vitest. The mock paths match the original Jest setup.
16-46: Mock functions correctly converted.All
jest.fn()calls are properly replaced withvi.fn(). The mock return values and structure remain intact.src/core/game/BinaryLoaderGameMapLoader.ts (1)
44-58: Vite URL-based binary loading approach looks correct.The
?urlimport suffix tells Vite to return the asset URL rather than inlining. The subsequent fetch + arrayBuffer conversion is the standard pattern for loading binary data in Vite.tests/core/game/Cluster.test.ts (1)
1-16: Vitest migration is correct.Mock types and functions properly converted. The
createMockStationhelper usesMocked<TrainStation>andvi.fn()as expected.vite.config.ts (2)
57-68: Environment variable injection looks correct.The
defineblock properly stringifies values withJSON.stringify. This replacesprocess.env.*references at build time.
83-119: Dev server proxy configuration is well structured.WebSocket proxies for
/socketand worker paths (/w0,/w1,/w2) plus API proxy for/apiare properly configured. The path rewrites for worker proxies correctly strip the prefix.tests/client/graphics/RadialMenuElements.test.ts (3)
1-1: Correct Vitest imports.Using
Mocktype for function mock typing is appropriate for themockClearcalls later in the file.
19-22: Async mock withvi.importActualis the right pattern.This correctly imports the real
UnitTypeenum while mocking the rest of the BuildMenu module.
478-491: Usingvi.importMockfor mock access.This is the Vitest pattern for accessing mocked modules to call methods like
mockClear. The async pattern withawaitis correct.src/client/Main.ts (2)
2-2: Correct use of?rawfor text file import.The
?rawsuffix tells Vite to import the file contents as a string, which is the right approach forversion.txt.
49-58: CSS imports are well organized.The new stylesheet imports follow a clear structure: components, core, layout, and modal. Side-effect imports like these are bundled by Vite.
src/client/FlagInputModal.ts (1)
3-3: JSON import with type attribute is correct.Using
with { type: "json" }is the modern ESM standard for JSON imports. This aligns with the codebase pattern mentioned in the learnings.tests/AutoUpgrade.test.ts (2)
22-22:vi.spyOncorrectly replacesjest.spyOn.The spy setup and assertions remain functionally identical.
Also applies to: 39-39
73-73:vi.fnmock functions correctly converted.All mock function creations properly use the Vitest API.
Also applies to: 83-84, 96-96
tests/DeleteUnitExecution.test.ts (1)
115-115: LGTM! Clean migration to Vitest.The
jest.spyOn→vi.spyOnchanges are straightforward and correctly preserve the mock behavior. All test assertions remain unchanged.Also applies to: 125-125, 134-134, 147-147
src/core/game/FetchGameMapLoader.ts (1)
45-47: Good defensive improvement for URL construction.Encoding and trimming the cacheBuster value prevents potential issues with special characters and whitespace in query parameters. This follows best practices for URL construction.
src/client/graphics/layers/StructureIconsLayer.ts (1)
5-5: LGTM! Correct use of Vite's raw import.The
?rawsuffix correctly imports the XML file as a string literal, which is then passed toPIXI.Assets.load()at line 112. This is the proper pattern for loading text-based assets with Vite.tests/core/executions/NukeExecution.test.ts (1)
31-35: LGTM! Clean Vitest migration.All
jest.fn()→vi.fn()replacements correctly preserve the mock behavior. The test logic and assertions remain unchanged.Also applies to: 54-54, 61-61
src/client/TerrainMapFileLoader.ts (1)
1-1: LGTM! Correct use of Vite's raw import.The
?rawsuffix correctly imports the version text file as a string, which is then passed toFetchGameMapLoader. This aligns with the Vite migration pattern seen throughout the PR.tests/AllianceExtensionExecution.test.ts (1)
38-40: LGTM! Comprehensive Vitest migration.All
jest.spyOn→vi.spyOnreplacements are correct. The mock lifecycle (includingmockReturnValueandmockRestore) is properly maintained. Test logic and assertions remain unchanged.Also applies to: 56-56, 85-87, 103-103, 123-125, 142-142
tests/NationAllianceBehavior.test.ts (2)
145-166: Mock object setup migrated correctly to Vitest.The mock objects for
mockGame,mockHuman,mockAlliance,mockRandom, andmockPlayerall usevi.fn()properly. The test logic is preserved.
74-84: Vitest migration is correctly configured and implemented.The project properly enables
globals: truein vite.config.ts, makingviavailable globally. The migration from Jest (jest.spyOn,jest.fn()) to Vitest (vi.spyOn,vi.fn()) in this mock setup is correct and follows Vitest conventions.src/client/NewsModal.ts (1)
4-7: Vite-compatible asset imports look good.
?urlforchangelog.mdreturns a URL string, which works correctly withfetch()on line 109.?rawforversion.txtreturns the file content as a string, used directly for version comparison on lines 146 and 154.Both patterns follow Vite conventions for asset handling.
src/core/configuration/DefaultConfig.ts (4)
30-30: Env import added for centralized environment access.This aligns with the project-wide migration to use the
Envutility instead of directprocess.envaccess.
90-104: Environment variable access migrated to Env utility.The switch from
process.env.XtoEnv.Xwith nullish coalescing for defaults is clean and consistent. This pattern works for both Vite (browser) and Node.js contexts.
136-148: OTEL and API key access updated correctly.All server-side environment variables now use the Env utility consistently.
153-159: Admin token access preserved correct error behavior.The
adminToken()method still throws whenADMIN_TOKENis not set, which is the expected behavior for a required secret.src/core/configuration/ConfigLoader.ts (3)
6-6: Env import added for environment access.Consistent with the project-wide migration pattern.
25-27: Error message now uses Env.GAME_ENV.This will show the resolved environment value in the error, which is helpful for debugging.
47-50: Server config retrieval simplified.
Env.GAME_ENVhandles the fallback to"dev"internally, so no nullish coalescing needed here. Clean and readable.index.html (5)
10-11: Root-relative paths for manifest and favicon.These paths align with Vite's asset handling. Vite dev server serves from root, and production builds will resolve these correctly.
114-122: Minor formatting change in keyframes.Added blank line between
fromandtoblocks. No functional impact.
337-341: Settings icon path updated to root-relative.Consistent with other asset path changes for Vite compatibility.
415-421: GitHub icon path updated to root-relative.Follows the same pattern as other asset updates.
511-511: Entry point script correctly loads Main.ts through Vite.Vite's dev server will transpile TypeScript on-the-fly via esbuild, with type checking handled separately by
tsc --noEmitduring builds. The vite.config.ts properly configures the entry point and asset bundling for both development and production environments.src/client/vite-env.d.ts (2)
1-1: Vite client types reference added.This enables TypeScript to understand Vite-specific features like
import.meta.env.
3-11: Type declarations for .bin and .md files.These declarations assume raw string content as default export. This works when using
?rawsuffix or if Vite is configured to treat these as raw by default.tests/InputHandler.test.ts (4)
21-21: MockPointerEvent uses vi.fn() correctly.The mock function for
preventDefaultis properly migrated to Vitest.
46-65: vi.spyOn usage for EventBus.emit is correct.The spy setup and assertions follow Vitest patterns correctly.
336-402: Event listener tests migrated correctly.Mock listeners using
vi.fn()and the subscribe/unsubscribe flow tests are properly migrated.
446-456: Console.warn spy setup is correct.Using
vi.spyOn(console, "warn").mockImplementation(() => {})andspy.mockRestore()follows Vitest best practices.src/core/configuration/Env.ts (4)
1-14: Environment utility with global type declarations.The ambient declarations for
ImportMetaEnvandImportMetaextend the global scope for TypeScript to understandimport.meta.env. This is a clean approach that works alongside Vite's built-in types.
16-45: getEnv helper handles both Vite and Node.js environments.The try-catch blocks prevent runtime errors when
import.metaorprocessare undefined. The fallback chain (viteKey → key → process.env) is well thought out.One small note: the
viteKeyparameter is not used by any caller in this file. If it's for future use, consider adding a comment.
47-63: GAME_ENV getter with Vite MODE priority.The special handling for
import.meta.env.MODEmakes sense since Vite sets this automatically based on build mode. Falling back togetEnv("GAME_ENV")or"dev"provides good defaults.
65-92: Environment variable getters are clean and consistent.Each getter delegates to
getEnv()with the appropriate key. The return typestring | undefinedis correct for optional environment variables.package.json (1)
14-16: Test scripts migration looks good.The switch from Jest to Vitest is straightforward. Both
vitest runfor single runs andvitest run --coveragefor coverage are correct.
wraith4081
force-pushed
the
build/migrate-webpack-to-vite
branch
from
e73517a to
2d824c2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/client/graphics/UILayer.test.ts (1)
2-2: Update stale Jest environment comment to Vitest syntax.The comment still references
@jest-environmentfrom the old test framework. Vitest uses@vitest-environmentinstead.🔎 Proposed fix
-/** - * @jest-environment jsdom - */ +/** + * @vitest-environment jsdom + */tests/InputHandler.test.ts (1)
2-2: Remove outdated Jest environment comment.The
@jest-environment jsdomcomment is no longer used by Vitest. Remove this line since Vitest configures the test environment throughvitest.config.tsor per-file directives with different syntax.🔎 Proposed fix
-/** - * @jest-environment jsdom - */ import { AutoUpgradeEvent, InputHandler } from "../src/client/InputHandler"; import { EventBus } from "../src/core/EventBus";
♻️ Duplicate comments (1)
package.json (1)
54-55: Known issue: vite version 7.3.0 doesn't exist.As previously flagged, vite 7.3.0 is not published on npm. The latest stable versions are in the v5-v6 range, with v8 beta releases available. Please update to a valid version.
Also applies to: 68-68, 87-91
🧹 Nitpick comments (1)
src/core/game/FetchGameMapLoader.ts (1)
45-47: Good improvement: encoding and trimming the cache buster.Using
encodeURIComponent()prevents URL issues with special characters, andtrim()removes unwanted whitespace. This is the correct approach for constructing query parameters.One minor refinement you could consider: if
cacheBustercontains only whitespace, it passes the truthiness check on line 44 but becomes an empty string after trimming, resulting inv=being appended. You could handle this by checking the trimmed value first:🔎 Optional refinement to handle whitespace-only cache buster
private url(map: string, path: string) { let url = `${this.prefix}/${map}/${path}`; - if (this.cacheBuster) { + const trimmedBuster = this.cacheBuster?.trim(); + if (trimmedBuster) { url += `${url.includes("?") ? "&" : "?"}v=${encodeURIComponent( - this.cacheBuster.trim(), + trimmedBuster, )}`; } return url; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (48)
.husky/pre-commitindex.htmljest.config.tspackage.jsonresources/countries.jsonsrc/client/FlagInputModal.tssrc/client/Main.tssrc/client/NewsModal.tssrc/client/TerrainMapFileLoader.tssrc/client/graphics/layers/ChatModal.tssrc/client/graphics/layers/PlayerPanel.tssrc/client/graphics/layers/StructureIconsLayer.tssrc/client/styles.csssrc/client/vite-env.d.tssrc/core/Schemas.tssrc/core/configuration/ConfigLoader.tssrc/core/configuration/DefaultConfig.tssrc/core/configuration/Env.tssrc/core/game/BinaryLoaderGameMapLoader.tssrc/core/game/FetchGameMapLoader.tssrc/core/worker/Worker.worker.tssrc/core/worker/WorkerClient.tssrc/global.d.tstailwind.config.jstests/AllianceExtensionExecution.test.tstests/AutoUpgrade.test.tstests/Censor.test.tstests/DeleteUnitExecution.test.tstests/InputHandler.test.tstests/MessageTypeClasses.test.tstests/NationAllianceBehavior.test.tstests/client/components/FluentSlider.test.tstests/client/graphics/ProgressBar.test.tstests/client/graphics/RadialMenuElements.test.tstests/client/graphics/UILayer.test.tstests/core/executions/NukeExecution.test.tstests/core/executions/TradeShipExecution.test.tstests/core/executions/WinCheckExecution.test.tstests/core/game/Cluster.test.tstests/core/game/GameImpl.test.tstests/core/game/RailNetwork.test.tstests/core/game/TrainStation.test.tstests/global.d.tstests/radialMenuElements.spec.tstests/setup.tstsconfig.jsonvite.config.tswebpack.config.js
💤 Files with no reviewable changes (4)
- jest.config.ts
- tests/global.d.ts
- src/global.d.ts
- webpack.config.js
✅ Files skipped from review due to trivial changes (1)
- src/core/Schemas.ts
🚧 Files skipped from review as they are similar to previous changes (24)
- tests/setup.ts
- src/client/TerrainMapFileLoader.ts
- tests/client/graphics/ProgressBar.test.ts
- src/client/graphics/layers/StructureIconsLayer.ts
- tests/core/executions/WinCheckExecution.test.ts
- src/client/FlagInputModal.ts
- src/core/worker/Worker.worker.ts
- tests/client/components/FluentSlider.test.ts
- src/client/graphics/layers/ChatModal.ts
- src/client/styles.css
- tests/NationAllianceBehavior.test.ts
- tests/core/executions/NukeExecution.test.ts
- tests/AutoUpgrade.test.ts
- tests/core/executions/TradeShipExecution.test.ts
- src/client/NewsModal.ts
- tests/MessageTypeClasses.test.ts
- tests/core/game/RailNetwork.test.ts
- tsconfig.json
- tests/core/game/TrainStation.test.ts
- index.html
- tests/Censor.test.ts
- tests/core/game/GameImpl.test.ts
- tailwind.config.js
- vite.config.ts
🧰 Additional context used
🧠 Learnings (14)
📚 Learning: 2025-06-07T13:15:55.439Z
Learnt from: Aotumuri
Repo: openfrontio/OpenFrontIO PR: 786
File: src/core/Util.ts:4-4
Timestamp: 2025-06-07T13:15:55.439Z
Learning: In the OpenFrontIO codebase, JSON files should be imported using standard import syntax without import attributes, as the TypeScript configuration supports resolveJsonModule and the codebase already uses this pattern successfully in files like src/client/Cosmetic.ts.
Applied to files:
src/client/Main.tssrc/client/graphics/layers/PlayerPanel.tssrc/client/vite-env.d.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/Main.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:
tests/DeleteUnitExecution.test.ts
📚 Learning: 2025-10-18T17:54:01.311Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:172-173
Timestamp: 2025-10-18T17:54:01.311Z
Learning: In src/core/execution/FakeHumanExecution.ts, MIRVs and ground attacks should not be mutually exclusive. The considerMIRV() method should not short-circuit maybeAttack() - both actions can occur in the same tick.
Applied to files:
tests/DeleteUnitExecution.test.tstests/AllianceExtensionExecution.test.tstests/client/graphics/RadialMenuElements.test.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:
tests/DeleteUnitExecution.test.tstests/client/graphics/RadialMenuElements.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:
tests/DeleteUnitExecution.test.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/client/graphics/layers/PlayerPanel.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/PlayerPanel.ts
📚 Learning: 2025-11-12T23:11:34.445Z
Learnt from: MaxHT0x
Repo: openfrontio/OpenFrontIO PR: 2262
File: src/core/configuration/DefaultConfig.ts:806-832
Timestamp: 2025-11-12T23:11:34.445Z
Learning: In src/core/configuration/DefaultConfig.ts, JSDoc documentation for configuration methods should not be added inline, as it was requested that documentation be placed elsewhere in the codebase.
Applied to files:
src/core/configuration/DefaultConfig.ts
📚 Learning: 2025-05-18T23:36:12.847Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 784
File: src/core/game/StatsImpl.ts:143-159
Timestamp: 2025-05-18T23:36:12.847Z
Learning: In this codebase, NukeType is a union type derived from UnitType values (specifically bomb-related values like AtomBomb, HydrogenBomb, MIRV, and MIRVWarhead) rather than a separate enum. This means comparing NukeType values against UnitType values in switch statements is valid and intentional.
Applied to files:
src/core/configuration/DefaultConfig.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:
tests/client/graphics/RadialMenuElements.test.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:
tests/client/graphics/RadialMenuElements.test.ts
📚 Learning: 2025-07-26T05:35:03.686Z
Learnt from: aaa4xu
Repo: openfrontio/OpenFrontIO PR: 1581
File: webpack.config.js:139-141
Timestamp: 2025-07-26T05:35:03.686Z
Learning: In OpenFrontIO's webpack configuration, binary-loader processes files from resources/maps at build-time and inlines the binary payload directly into the compiled JS bundle. This means the original .bin assets are no longer needed at runtime and can be safely excluded from the final Docker image using webpack's globOptions.ignore pattern to reduce image size.
Applied to files:
src/core/game/BinaryLoaderGameMapLoader.tssrc/client/vite-env.d.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/core/game/Cluster.test.ts
🧬 Code graph analysis (5)
src/core/configuration/DefaultConfig.ts (1)
src/core/configuration/Env.ts (1)
Env(47-92)
tests/client/graphics/RadialMenuElements.test.ts (2)
src/core/game/GameView.ts (2)
PlayerView(185-578)GameView(580-906)src/client/graphics/layers/RadialMenuElements.ts (2)
attackMenuElement(413-424)buildMenuElement(499-510)
src/core/game/BinaryLoaderGameMapLoader.ts (1)
src/core/game/FetchGameMapLoader.ts (1)
url(41-51)
src/core/configuration/ConfigLoader.ts (1)
src/core/configuration/Env.ts (1)
Env(47-92)
tests/core/game/Cluster.test.ts (1)
src/core/game/TrainStation.ts (2)
TrainStation(74-172)Cluster(200-249)
🔇 Additional comments (22)
tests/client/graphics/UILayer.test.ts (1)
31-31: LGTM! Mock functions correctly migrated to Vitest.The migration from
jest.fn()tovi.fn()is correct and follows Vitest conventions.Also applies to: 53-53
tests/core/game/Cluster.test.ts (1)
1-1: LGTM! Vitest migration correctly applied.The test framework migration is properly executed:
vi.fn()replacesjest.fn(), andMocked<TrainStation>replacesjest.Mocked<TrainStation>. Test logic and coverage remain unchanged.Also applies to: 4-4, 7-8, 14-16
tests/InputHandler.test.ts (1)
21-21: Clean migration from Jest to Vitest APIs.All Jest mocking APIs have been correctly replaced with Vitest equivalents:
jest.fn()→vi.fn()jest.spyOn()→vi.spyOn()The test logic remains unchanged and the migration is complete.
Also applies to: 48-48, 68-68, 92-92, 112-112, 148-148, 168-168, 188-188, 210-210, 232-232, 256-256, 276-276, 296-296, 316-316, 338-338, 359-360, 388-388, 447-447
src/core/worker/WorkerClient.ts (1)
26-28: LGTM! Clean ESM worker pattern for Vite.The
new URL("./Worker.worker.ts", import.meta.url)with{ type: "module" }is the idiomatic approach for ES module workers in Vite. This properly resolves the worker path relative to the current module and enables ESM syntax inside the worker.src/core/game/BinaryLoaderGameMapLoader.ts (2)
35-41: Good runtime binary loading approach for Vite migration.The
loadBinaryhelper is clean: fetch, validate response, convert toUint8Array. This replaces the old webpack binary-loader that inlined payloads at build time.Based on learnings, the old approach inlined binary data into the JS bundle. The new runtime fetch approach trades a smaller initial bundle for additional network requests. This is a reasonable trade-off for map data that may not always be needed immediately.
44-58: LGTM! Consistent Vite?urlimport pattern.Using
?urlsuffix for.binfiles is correct since Vite does not recognize them as standard assets. The pattern is applied consistently across all three binary map files. The lazy loading viacreateLazyLoaderensures each binary is fetched only once and cached.tests/radialMenuElements.spec.ts (1)
1-101: Clean Jest to Vitest migration.The migration correctly updates all Jest APIs to Vitest equivalents:
jest.mock→vi.mockjest.fn()→vi.fn()- Import
vifrom vitestTest logic remains unchanged, which is appropriate for a tooling migration.
tests/client/graphics/RadialMenuElements.test.ts (4)
1-80: Correct async mock pattern for ESM modules.The migration properly handles ESM-specific mocking:
- BuildMenu mock uses
asyncwithvi.importActualto access the realUnitTypeenum while mocking the modulenanoidmock correctly returns a function that returns a function (matchescustomAlphabetAPI)dompurifymock uses__esModule: truefor default export compatibilityAll Jest APIs correctly replaced with Vitest equivalents.
82-153: Test setup correctly migrated.All
jest.fn()calls replaced withvi.fn()in thebeforeEachsetup. Mock structure and test data unchanged.
155-476: Test suites correctly updated to Vitest.All mock function creations updated to
vi.fn()throughout the test suites. Test expectations and logic remain unchanged.
477-515: Async mock import pattern correctly implemented.The translation tests now use
await vi.importMockto access mocked functions, with explicitMocktype casting before callingmockClear(). This pattern is correct because:
vi.importMockis async and returns the mocked module- TypeScript doesn't automatically infer the
Mocktype from the imported function- The cast enables access to mock-specific methods like
mockClear()src/client/graphics/layers/PlayerPanel.ts (1)
3-3: No action needed; import is consistent with codebase pattern.The
with { type: "json" }syntax follows the established pattern in this codebase. All imports ofcountries.json(inFlagInputModal.tsandSchemas.ts) andQuickChat.jsonuse this same syntax. The import inPlayerPanel.tsis correct.Likely an incorrect or invalid review comment.
src/client/Main.ts (2)
2-2: LGTM! Raw text import for version string.The
?rawsuffix correctly imports the version file content as a plain string, which works perfectly for displaying at line 141.
49-58: LGTM! CSS imports expanded for Vite migration.The additional stylesheet imports follow standard side-effect import patterns and align with the Vite-based asset handling introduced in this PR.
tests/DeleteUnitExecution.test.ts (1)
115-115: LGTM! Clean Jest to Vitest migration.All
jest.spyOncalls correctly replaced withvi.spyOnwhile preserving identical mocking behavior and test semantics.Also applies to: 125-125, 134-134, 147-147
tests/AllianceExtensionExecution.test.ts (1)
38-40: LGTM! Vitest migration complete.All Jest spy/mock calls successfully migrated to Vitest equivalents with identical behavior.
Also applies to: 56-56, 85-87, 103-103, 123-125, 142-142, 165-165
package.json (1)
4-6: LGTM! Build and test scripts updated for Vite/Vitest.The script changes correctly reflect the migration from Webpack to Vite for builds and Jest to Vitest for testing.
Also applies to: 14-14, 16-16
src/core/configuration/ConfigLoader.ts (1)
6-6: LGTM! Clean migration to centralized Env utility.The switch from direct
process.envaccess to the newEnvutility provides consistent environment variable access across Node.js and Vite contexts.Also applies to: 26-26, 48-48
src/core/configuration/DefaultConfig.ts (1)
30-30: LGTM! Consistent Env utility adoption.All environment variable access correctly migrated to use the new
Envutility while preserving existing fallback behavior and error handling.Also applies to: 91-91, 98-98, 101-101, 104-104, 137-137, 140-140, 143-143, 147-147, 154-154, 227-227
src/core/configuration/Env.ts (2)
16-45: LGTM! Solid hybrid environment variable access.The
getEnvhelper correctly handles both Vite (import.meta.env) and Node.js (process.env) contexts with appropriate error handling for environments where either might not exist.
47-92: LGTM! Clean environment variable getters.The
Envobject provides a consistent interface for accessing environment variables across the codebase. The specialGAME_ENVhandling for Vite'sMODEis appropriate and the "dev" fallback ensures safe defaults.src/client/vite-env.d.ts (1)
3-26: The module declarations don't match how these files are actually imported.Your actual imports use query parameters that the declarations don't account for:
.binfiles use?url(e.g.,import(...map.bin?url)) which returns a URL string, not raw file content.mdfiles use?url(e.g.,import changelog from "...changelog.md?url") which returns a URL.xmlfiles use?raw(e.g.,import bitmapFont from "...round_6x6_modified.xml?raw") to get raw content.txtfiles use?rawconsistently throughout (e.g.,import version from "...version.txt?raw") but have no declaration in this fileSince your code already uses the correct query parameters everywhere, the type declarations here are misleading—they suggest imports work without these suffixes, but they won't. Either remove unused declarations or align them with your actual import patterns by adding the query parameters.
Likely an incorrect or invalid review comment.
depracated husky usage - Replace Webpack with Vite for faster client bundling and HMR. - Migrate tests from Jest to Vitest and update configuration. - Update Web Worker instantiation to standard ESM syntax. - Implement Env utility in `src/core` for safe, hybrid environment variable access (Vite vs Node). - Refactor configuration loaders to remove direct `process.env` dependencies in shared code. - Update TypeScript environment definitions and project scripts for the new toolchain. - Remove the [depracated usage of the husky](https://github.com/typicode/husky/releases/tag/v9.0.1). build(vite): implement robust git commit resolution with fallback
wraith4081
force-pushed
the
build/migrate-webpack-to-vite
branch
from
2d824c2 to
c5f6d91
Compare
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)
Dockerfile (1)
15-15: Update Dockerfile to copyvite.config.tsinstead ofwebpack.config.js.The build system migration from Webpack to Vite is complete (
vite.config.tsexists and webpack is removed from package.json). Line 15 still copies the oldwebpack.config.jsfile, which is no longer needed. Replace it with:COPY vite.config.ts ./
🧹 Nitpick comments (3)
src/core/game/BinaryLoaderGameMapLoader.ts (1)
35-41: Consider extractingloadBinaryas a private class method.The
loadBinaryhelper is currently defined insidegetMapData, which means it's recreated for each map type. Extracting it as a private class method would improve reusability and make the code easier to test.🔎 Proposed refactoring
+ private async loadBinary(url: string): Promise<Uint8Array> { + const res = await fetch(url); + if (!res.ok) throw new Error(`Failed to load ${url}`); + const buf = await res.arrayBuffer(); + return new Uint8Array(buf); + } + getMapData(map: GameMapType): MapData { const cachedMap = this.maps.get(map); if (cachedMap) { return cachedMap; } const key = Object.keys(GameMapType).find( (k) => GameMapType[k as keyof typeof GameMapType] === map, ); const fileName = key?.toLowerCase(); - const loadBinary = (url: string) => - fetch(url) - .then((res) => { - if (!res.ok) throw new Error(`Failed to load ${url}`); - return res.arrayBuffer(); - }) - .then((buf) => new Uint8Array(buf)); - const mapData = { mapBin: this.createLazyLoader(() => import(`../../../resources/maps/${fileName}/map.bin?url`).then((m) => - loadBinary(m.default), + this.loadBinary(m.default), ), ),tsconfig.jest.json (1)
8-8: Consider renaming this file to reflect Vitest usage.The file content has been correctly updated to use Vitest types, but the filename
tsconfig.jest.jsonis now misleading. Consider renaming it totsconfig.test.jsonortsconfig.vitest.jsonfor clarity.🔍 Verification: Check references to this config file
#!/bin/bash # Find references to tsconfig.jest.json in the codebase rg -n "tsconfig\.jest\.json" --type-add 'config:*.{json,js,ts}' -t configvite.config.ts (1)
72-83: Consider migrating fromprocess.envdefines toimport.meta.env.The configuration uses
defineto replaceprocess.env.*at build time, which works but isn't the Vite-native approach. The inline comment correctly notes this.For a cleaner Vite integration, consider migrating code to use
import.meta.env.*instead ofprocess.env.*. This would align with Vite's conventions and theEnvutility (which already checksimport.meta.env.MODE).This is not blocking for the current migration but worth planning as a follow-up refactor.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (51)
.husky/pre-commitDockerfileeslint.config.jsindex.htmljest.config.tspackage.jsonresources/countries.jsonsrc/client/FlagInputModal.tssrc/client/Main.tssrc/client/NewsModal.tssrc/client/TerrainMapFileLoader.tssrc/client/graphics/layers/ChatModal.tssrc/client/graphics/layers/PlayerPanel.tssrc/client/graphics/layers/StructureIconsLayer.tssrc/client/styles.csssrc/client/vite-env.d.tssrc/core/Schemas.tssrc/core/configuration/ConfigLoader.tssrc/core/configuration/DefaultConfig.tssrc/core/configuration/Env.tssrc/core/game/BinaryLoaderGameMapLoader.tssrc/core/game/FetchGameMapLoader.tssrc/core/worker/Worker.worker.tssrc/core/worker/WorkerClient.tssrc/global.d.tstailwind.config.jstests/AllianceExtensionExecution.test.tstests/AutoUpgrade.test.tstests/Censor.test.tstests/DeleteUnitExecution.test.tstests/InputHandler.test.tstests/MessageTypeClasses.test.tstests/NationAllianceBehavior.test.tstests/client/components/FluentSlider.test.tstests/client/graphics/ProgressBar.test.tstests/client/graphics/RadialMenuElements.test.tstests/client/graphics/UILayer.test.tstests/core/executions/NukeExecution.test.tstests/core/executions/TradeShipExecution.test.tstests/core/executions/WinCheckExecution.test.tstests/core/game/Cluster.test.tstests/core/game/GameImpl.test.tstests/core/game/RailNetwork.test.tstests/core/game/TrainStation.test.tstests/global.d.tstests/radialMenuElements.spec.tstests/setup.tstsconfig.jest.jsontsconfig.jsonvite.config.tswebpack.config.js
💤 Files with no reviewable changes (5)
- jest.config.ts
- webpack.config.js
- eslint.config.js
- tests/global.d.ts
- src/global.d.ts
🚧 Files skipped from review as they are similar to previous changes (27)
- tests/core/game/GameImpl.test.ts
- src/client/styles.css
- src/core/worker/Worker.worker.ts
- tests/core/executions/WinCheckExecution.test.ts
- tests/setup.ts
- src/client/Main.ts
- tests/Censor.test.ts
- tests/NationAllianceBehavior.test.ts
- src/client/FlagInputModal.ts
- src/core/game/FetchGameMapLoader.ts
- tests/core/game/RailNetwork.test.ts
- tests/InputHandler.test.ts
- tests/core/executions/TradeShipExecution.test.ts
- src/core/Schemas.ts
- src/client/graphics/layers/PlayerPanel.ts
- src/core/configuration/Env.ts
- src/core/worker/WorkerClient.ts
- .husky/pre-commit
- src/client/graphics/layers/StructureIconsLayer.ts
- src/client/TerrainMapFileLoader.ts
- src/client/vite-env.d.ts
- src/client/graphics/layers/ChatModal.ts
- tsconfig.json
- tests/MessageTypeClasses.test.ts
- tests/client/graphics/UILayer.test.ts
- src/client/NewsModal.ts
- tests/client/graphics/RadialMenuElements.test.ts
🧰 Additional context used
🧠 Learnings (16)
📚 Learning: 2025-10-18T17:54:01.311Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:172-173
Timestamp: 2025-10-18T17:54:01.311Z
Learning: In src/core/execution/FakeHumanExecution.ts, MIRVs and ground attacks should not be mutually exclusive. The considerMIRV() method should not short-circuit maybeAttack() - both actions can occur in the same tick.
Applied to files:
tests/AllianceExtensionExecution.test.tstests/DeleteUnitExecution.test.tstests/core/executions/NukeExecution.test.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:
tests/DeleteUnitExecution.test.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:
tests/DeleteUnitExecution.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:
tests/DeleteUnitExecution.test.ts
📚 Learning: 2025-07-26T05:35:03.686Z
Learnt from: aaa4xu
Repo: openfrontio/OpenFrontIO PR: 1581
File: webpack.config.js:139-141
Timestamp: 2025-07-26T05:35:03.686Z
Learning: In OpenFrontIO's webpack configuration, binary-loader processes files from resources/maps at build-time and inlines the binary payload directly into the compiled JS bundle. This means the original .bin assets are no longer needed at runtime and can be safely excluded from the final Docker image using webpack's globOptions.ignore pattern to reduce image size.
Applied to files:
src/core/game/BinaryLoaderGameMapLoader.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:
tests/core/executions/NukeExecution.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:
tests/core/executions/NukeExecution.test.ts
📚 Learning: 2025-10-20T11:02:16.969Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:57-57
Timestamp: 2025-10-20T11:02:16.969Z
Learning: In src/core/execution/FakeHumanExecution.ts, the correct MIRV victory denial thresholds are VICTORY_DENIAL_TEAM_THRESHOLD = 0.8 (80% for team games) and VICTORY_DENIAL_INDIVIDUAL_THRESHOLD = 0.65 (65% for individual players), not 0.85 and 0.7 as might be mentioned in some documentation.
Applied to files:
tests/core/executions/NukeExecution.test.ts
📚 Learning: 2025-08-24T11:13:08.495Z
Learnt from: DevelopingTom
Repo: openfrontio/OpenFrontIO PR: 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/NukeExecution.test.ts
📚 Learning: 2025-05-18T23:36:12.847Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 784
File: src/core/game/StatsImpl.ts:143-159
Timestamp: 2025-05-18T23:36:12.847Z
Learning: In this codebase, NukeType is a union type derived from UnitType values (specifically bomb-related values like AtomBomb, HydrogenBomb, MIRV, and MIRVWarhead) rather than a separate enum. This means comparing NukeType values against UnitType values in switch statements is valid and intentional.
Applied to files:
tests/core/executions/NukeExecution.test.tssrc/core/configuration/DefaultConfig.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/core/game/Cluster.test.tstests/core/game/TrainStation.test.ts
📚 Learning: 2025-11-12T23:11:34.445Z
Learnt from: MaxHT0x
Repo: openfrontio/OpenFrontIO PR: 2262
File: src/core/configuration/DefaultConfig.ts:806-832
Timestamp: 2025-11-12T23:11:34.445Z
Learning: In src/core/configuration/DefaultConfig.ts, JSDoc documentation for configuration methods should not be added inline, as it was requested that documentation be placed elsewhere in the codebase.
Applied to files:
src/core/configuration/DefaultConfig.ts
📚 Learning: 2025-05-26T09:52:52.465Z
Learnt from: tnhnblgl
Repo: openfrontio/OpenFrontIO PR: 875
File: src/client/index.html:396-402
Timestamp: 2025-05-26T09:52:52.465Z
Learning: In the OpenFrontIO project, the correct path for icons from src/client/index.html is ../../resources/icons/, not /static/icons/ as the build process handles the path mapping differently.
Applied to files:
index.html
📚 Learning: 2025-07-19T18:12:24.553Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1417
File: src/client/index.html:335-350
Timestamp: 2025-07-19T18:12:24.553Z
Learning: For the OpenFrontIO project: rel="noopener" is not required for internal links (like /privacy-policy.html and /terms-of-service.html) since the project controls the destination pages. The security concern only applies to external, untrusted links.
Applied to files:
index.html
📚 Learning: 2025-08-09T05:14:19.147Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1761
File: src/client/LocalPersistantStats.ts:24-31
Timestamp: 2025-08-09T05:14:19.147Z
Learning: Files in the src/client directory contain browser-only code in the OpenFrontIO project, so browser APIs like localStorage are guaranteed to be available and don't need undefined guards.
Applied to files:
index.html
📚 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:
index.html
🧬 Code graph analysis (7)
tests/DeleteUnitExecution.test.ts (1)
src/server/GameManager.ts (1)
game(25-27)
src/core/game/BinaryLoaderGameMapLoader.ts (1)
src/core/game/FetchGameMapLoader.ts (1)
url(41-51)
tests/core/executions/NukeExecution.test.ts (1)
tests/util/TestConfig.ts (1)
TestConfig(12-83)
src/core/configuration/ConfigLoader.ts (1)
src/core/configuration/Env.ts (1)
Env(47-92)
src/core/configuration/DefaultConfig.ts (1)
src/core/configuration/Env.ts (1)
Env(47-92)
vite.config.ts (2)
eslint.config.js (2)
__filename(9-9)__dirname(10-10)src/core/configuration/DefaultConfig.ts (1)
gitCommit(142-144)
tests/core/game/TrainStation.test.ts (1)
src/core/game/Game.ts (3)
Game(669-763)Unit(436-515)Player(530-667)
🔇 Additional comments (25)
tests/radialMenuElements.spec.ts (1)
1-1: LGTM! Clean Vitest migration.The test file migration from Jest to Vitest is complete and correct. All necessary changes are in place:
viimported from Vitestjest.mockreplaced withvi.mockjest.fn()replaced withvi.fn()The mock structure and test logic remain unchanged, which is exactly what we want in a framework migration.
Also applies to: 4-10, 58-71
tests/client/graphics/ProgressBar.test.ts (2)
28-28: Same missing import issue applies here.This line has the same missing
viimport problem flagged above.
15-17: No changes needed. The Vitest configuration hasglobals: trueenabled, which automatically makesviavailable without requiring an explicit import. The spy setup on lines 15-17 is correct and will work as intended.tests/core/executions/NukeExecution.test.ts (2)
31-61: Jest to Vitest migration looks correct.The mock function replacements from
jest.fn()tovi.fn()are syntactically correct and maintain the same test behavior.
1-11: No action needed. Vitest globals are explicitly configured invite.config.tswithglobals: true, allowing the test file to usedescribe,test,expect,beforeEach, andviwithout imports. The code is correct as written.Likely an incorrect or invalid review comment.
src/core/game/BinaryLoaderGameMapLoader.ts (1)
45-57: Switch from build-time binary inlining to runtime fetching requires careful performance testing.With Vite, the
?urlimports return file URLs instead of inlined binary data. ThepublicDir: "resources"setting in vite.config.ts automatically copies all.binfiles to the production build, but the browser now fetches them at runtime instead of having them available immediately in the JavaScript bundle.This trade-off is intentional with the Vite migration:
- Pros: Smaller JS bundle size, binary assets loaded on demand
- Cons: Each binary requires a separate HTTP request, adding initial load latency
Ensure load times remain acceptable for your game startup sequence. If performance degrades significantly, consider whether eager-loading critical binaries in the main bundle would be worthwhile.
tests/DeleteUnitExecution.test.ts (1)
115-147: LGTM! Clean migration to Vitest.The test logic remains unchanged; only the mocking API has been updated from Jest to Vitest (
jest.spyOn→vi.spyOn). The migration is consistent across all test cases.tests/AutoUpgrade.test.ts (1)
19-93: LGTM! Consistent migration to Vitest mocks.All Jest mocking functions (
jest.fn,jest.spyOn) have been properly replaced with Vitest equivalents (vi.fn,vi.spyOn). Test logic and assertions remain unchanged.tailwind.config.js (1)
4-4: LGTM! Appropriate for Vite project structure.Adding
./index.htmlto Tailwind's content scanning ensures that classes used in the root HTML file are detected. This aligns well with Vite's convention of having index.html at the project root.tests/core/game/Cluster.test.ts (1)
1-16: LGTM! Complete migration to Vitest types and mocks.The file has been properly updated to use Vitest's type utilities (
Mockedfromvitest) and mocking functions (vi.fn()). The migration is clean and consistent.tests/client/components/FluentSlider.test.ts (2)
4-6: LGTM! Module mock correctly migrated to Vitest.The module mock for
Utils.tshas been properly updated to usevi.mockwithvi.fn(), maintaining the same behavior as the Jest version.
84-154: LGTM! Event spies consistently use Vitest API.All event spy functions have been properly migrated from
jest.fn()tovi.fn(). The test logic and expectations remain unchanged.tests/AllianceExtensionExecution.test.ts (1)
38-165: LGTM! Comprehensive migration to Vitest spies.All
jest.spyOncalls have been properly replaced withvi.spyOnthroughout the test file. ThemockRestore()call remains compatible with Vitest. Test logic and assertions are unchanged.index.html (3)
10-11: LGTM! Root-relative paths align with Vite's publicDir configuration.The migration from
../../resources/to root-relative paths (/manifest.json,/images/Favicon.svg) is correct. Vite serves thepublicDir: "resources"directory at the root, so/manifest.jsonresolves toresources/manifest.json.
338-338: LGTM! Image paths correctly updated for Vite.The paths for
SettingIconWhite.svgandgithub-mark-white.svgare now root-relative (/images/,/icons/), which is correct given Vite'spublicDir: "resources"configuration. The retrieved learning about../../resources/icons/was specific to the Webpack build setup.Also applies to: 416-416
511-511: LGTM! Vite module entry point added.This script tag loads the Vite entry point directly as an ES module, replacing the Webpack bundled output. It correctly matches the
entryconfigured invite.config.ts.tests/core/game/TrainStation.test.ts (1)
1-130: LGTM! Clean Jest to Vitest migration.The test file has been correctly migrated from Jest to Vitest:
- Import changed to
viandMockedfrom Vitest- All
jest.mock→vi.mock- All
jest.fn()→vi.fn()- Mock types updated to Vitest's
Mocked<T>Test logic and assertions remain unchanged. The migration is mechanical and correct.
src/core/configuration/DefaultConfig.ts (1)
30-30: LGTM! Centralized environment variable access via Env utility.All
process.envaccesses have been consistently replaced with theEnvutility. This provides a single source of truth for environment variables and handles the differences between Vite'simport.meta.envand Node'sprocess.env. The public API signatures remain unchanged, so this is purely an internal refactor.Also applies to: 91-91, 98-98, 101-101, 104-104, 137-137, 140-140, 143-143, 147-147, 154-154, 227-227
package.json (2)
4-6: LGTM! Scripts correctly updated for Vite and Vitest.The build and test scripts have been properly migrated:
- Build scripts use
concurrentlyto run TypeScript checks alongside Vite buildsstart:clientnow launches Vite's dev server- Test scripts invoke Vitest instead of Jest
Also applies to: 14-14, 16-16
51-52: All package versions specified are valid and available on npm registry. No action needed.Likely an incorrect or invalid review comment.
src/core/configuration/ConfigLoader.ts (1)
6-6: LGTM! Consistent Env utility integration.The file correctly adopts the
Envutility for environment variable access:
- Error messages now reference
Env.GAME_ENV- Server config retrieval uses
Env.GAME_ENVinstead ofprocess.env.GAME_ENVThis aligns with the migration in
DefaultConfig.tsand provides consistent cross-environment variable access.Also applies to: 26-26, 48-48
vite.config.ts (4)
10-11: LGTM! Correct __dirname definition for ESM.The file correctly defines
__dirnamefor ESM modules using the standard pattern. This is necessary for thepath.resolvecalls later in the configuration.
13-24: LGTM! Git commit resolution with proper error handling.The configuration correctly resolves the Git commit with:
- Primary:
process.env.GIT_COMMIT- Fallback:
git rev-parse HEAD- Error handling: warns in non-production, falls back to "unknown"
This ensures the build doesn't fail when Git is unavailable.
31-35: LGTM! Vitest configuration for browser-like testing.The test configuration is correct:
globals: trueenables global test functions (describe,it,expect)environment: "jsdom"provides browser APIs for client-side testssetupFilespoints to shared test setup
85-134: LGTM! Build and dev server configuration.The configuration correctly sets up:
Build:
- Output directory
staticmatches the previous Webpack setup- Manual vendor chunk for large dependencies (pixi.js, howler, zod, protobufjs)
Dev Server:
- Port 9000 for the client dev server
- WebSocket proxies for
/socketand worker paths (/w0,/w1,/w2)- API proxy for
/api→localhost:3000All proxies are necessary for local development to work correctly.
|
How long did this even take 😭 |
evanpelle
left a comment
evanpelle
left a comment
There was a problem hiding this comment.
had to make the following changes to get it to work on my machine, if you can incorporate them into this PR:
https://github.com/openfrontio/OpenFrontIO/pull/2717/changes
also seeing a lot of these errors in when doing a hot-reload:
[0]
[0] WARN Assets in public directory cannot be imported from JavaScript.
[0] If you intend to import that asset, put the file in the src directory, and use /src/images/Snowflake.webp instead of /resources/images/Snowflake.webp.
[0] If you intend to use the URL of that asset, use /images/Snowflake.webp?url.
[0]
[0]
[0] WARN Assets in public directory cannot be imported from JavaScript.
[0] If you intend to import that asset, put the file in the src directory, and use /src/version.txt?raw instead of /resources/version.txt?raw.
[0] If you intend to use the URL of that asset, use /version.txt?url&raw.
[0]
[0]
[0] WARN Assets in public directory cannot be imported from JavaScript.
[0] If you intend to import that asset, put the file in the src directory, and use /src/countries.json instead of /resources/countries.json.
[0] If you intend to use the URL of that asset, use /countries.json?url.
[0]
[0]
[0] WARN Assets in public directory cannot be imported from JavaScript.
[0] If you intend to import that asset, put the file in the src directory, and use /src/QuickChat.json instead of /resources/QuickChat.json.
[0] If you intend to use the URL of that asset, use /QuickChat.json?url.
[0]
[0]
[0] WARN Assets in public directory cannot be imported from JavaScript.
[0] If you intend to import that asset, put the file in the src directory, and use /src/images/RandomMap.webp instead of /resources/images/RandomMap.webp.
[0] If you intend to use the URL of that asset, use /images/RandomMap.webp?url.
[0]
[0]
[0] WARN Assets in public directory cannot be imported from JavaScript.
[0] If you intend to import that asset, put the file in the src directory, and use /src/lang/bg.json instead of /resources/lang/bg.json.
[0] If you intend to use the URL of that asset, use /lang/bg.json?url.
[0]
[0]
[0] WARN Assets in public directory cannot be imported from JavaScript.
[0] If you intend to import that asset, put the file in the src directory, and use /src/lang/bn.json instead of /resources/lang/bn.json.
[0] If you intend to use the URL of that asset, use /lang/bn.json?url.
[0]
[0]
[0] WARN Assets in public directory cannot be imported from JavaScript.
[0] If you intend to import that asset, put the file in the src directory, and use /src/lang/ar.json instead of /resources/lang/ar.json.
[0] If you intend to use the URL of that asset, use /lang/ar.json?url.
[0]
[0]
[0] WARN Assets in public directory cannot be imported from JavaScript.
[0] If you intend to import that asset, put the file in the src directory, and use /src/lang/cs.json instead of /resources/lang/cs.json.
[0] If you intend to use the URL of that asset, use /lang/cs.json?url.
[0]
[0]
[0] WARN Assets in public directory cannot be imported from JavaScript.
[0] If you intend to import that asset, put the file in the src directory, and use /src/lang/da.json instead of /resources/lang/da.json.
[0] If you intend to use the URL of that asset, use /lang/da.json?url.
[0]
[0]
[0] WARN Assets in public directory cannot be imported from JavaScript.
[0] If you intend to import that asset, put the file in the src directory, and use /src/lang/el.json instead of /resources/lang/el.json.
[0] If you intend to use the URL of that asset, use /lang/el.json?url.
[0]
[0]
[0] WARN Assets in public directory cannot be imported from JavaScript.
[0] If you intend to import that asset, put the file in the src directory, and use /src/lang/de.json instead of /resources/lang/de.json.
[0] If you intend to use the URL of that asset, use /lang/de.json?url.
[0]
[0]
[0] WARN Assets in public directory cannot be imported from JavaScript.
[0] If you intend to import that asset, put the file in the src directory, and use /src/lang/eo.json instead of /resources/lang/eo.json.
[0] If you intend to use the URL of that asset, use /lang/eo.json?url.
[0]
[0]
[0] WARN Assets in public directory cannot be imported from JavaScript.
[0] If you intend to import that asset, put the file in the src directory, and use /src/lang/es.json instead of /resources/lang/es.json.
[0] If you intend to use the URL of that asset, use /lang/es.json?url.
[0]
[0]
[0] WARN Assets in public directory cannot be imported from JavaScript.
[0] If you intend to import that asset, put the file in the src directory, and use /src/lang/en.json instead of /resources/lang/en.json.
[0] If you intend to use the URL of that asset, use /lang/en.json?url.
[0]
[0]
[0] WARN Assets in public directory cannot be imported from JavaScript.
[0] If you intend to import that asset, put the file in the src directory, and use /src/lang/it.json instead of /resources/lang/it.json.
[0] If you intend to use the URL of that asset, use /lang/it.json?url.
[0]
[0]
[0] WARN Assets in public directory cannot be imported from JavaScript.
[0] If you intend to import that asset, put the file in the src directory, and use /src/lang/fr.json instead of /resources/lang/fr.json.
[0] If you intend to use the URL of that asset, use /lang/fr.json?url.
[0]
[0]
[0] WARN Assets in public directory cannot be imported from JavaScript.
[0] If you intend to import that asset, put the file in the src directory, and use /src/lang/he.json instead of /resources/lang/he.json.
[0] If you intend to use the URL of that asset, use /lang/he.json?url.
[0]
[0]
[0] WARN Assets in public directory cannot be imported from JavaScript.
[0] If you intend to import that asset, put the file in the src directory, and use /src/lang/fa.json instead of /resources/lang/fa.json.
[0] If you intend to use the URL of that asset, use /lang/fa.json?url.
[0]
[0]
[0] WARN Assets in public directory cannot be imported from JavaScript.
[0] If you intend to import that asset, put the file in the src directory, and use /src/lang/fi.json instead of /resources/lang/fi.json.
[0] If you intend to use the URL of that asset, use /lang/fi.json?url.
[0]
[0]
[0] WARN Assets in public directory cannot be imported from JavaScript.
[0] If you intend to import that asset, put the file in the src directory, and use /src/lang/hi.json instead of /resources/lang/hi.json.
[0] If you intend to use the URL of that asset, use /lang/hi.json?url.
[0]
I will look into it asap |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
package.json (1)
84-84: Invalid vite version already flagged.This issue has been flagged in previous reviews and acknowledged by the author.
🧹 Nitpick comments (1)
src/client/graphics/layers/StructureIconsLayer.ts (1)
78-79: Simplify renderer initialization by removing redundant flag.The
rendererInitializedflag and localrenderervariable add unnecessary complexity. Since the renderer is nullable, checkingthis.renderer !== nullalready indicates initialization status.🔎 Proposed simplification
Remove the redundant flag:
- private renderer: PIXI.Renderer | null = null; - private rendererInitialized: boolean = false; + private renderer: PIXI.Renderer | null = null;Simplify setupRenderer:
async setupRenderer() { try { await PIXI.Assets.load(bitmapFont); } catch (error) { console.error("Failed to load bitmap font:", error); } - const renderer = new PIXI.WebGLRenderer(); + this.renderer = new PIXI.WebGLRenderer(); this.pixicanvas = document.createElement("canvas"); // ... stage setup ... - await renderer.init({ + await this.renderer.init({ canvas: this.pixicanvas, // ... config ... }); - - this.renderer = renderer; - this.rendererInitialized = true; }Simplify guard condition:
renderLayer(mainContext: CanvasRenderingContext2D) { - if (!this.renderer || !this.rendererInitialized) { + if (!this.renderer) { return; }Also applies to: 117-117, 147-159, 209-209
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
package.jsonsrc/client/graphics/layers/StructureIconsLayer.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: aaa4xu
Repo: openfrontio/OpenFrontIO PR: 1581
File: webpack.config.js:139-141
Timestamp: 2025-07-26T05:35:03.686Z
Learning: In OpenFrontIO's webpack configuration, binary-loader processes files from resources/maps at build-time and inlines the binary payload directly into the compiled JS bundle. This means the original .bin assets are no longer needed at runtime and can be safely excluded from the final Docker image using webpack's globOptions.ignore pattern to reduce image size.
📚 Learning: 2025-10-18T11:00:57.142Z
Learnt from: NewYearNewPhil
Repo: openfrontio/OpenFrontIO PR: 2230
File: src/client/graphics/GameRenderer.ts:269-277
Timestamp: 2025-10-18T11:00:57.142Z
Learning: In src/client/graphics/GameRenderer.ts, the GameRecapCapture implementation does not use setCaptureRenderEnabled on layers. Instead, it uses RecapCaptureSurface.capture() to render capture layers (TerrainLayer, TerritoryLayer, RailroadLayer, StructureIconsLayer, UnitLayer) directly to an off-screen canvas without requiring layer-level capture mode methods.
Applied to files:
src/client/graphics/layers/StructureIconsLayer.ts
🔇 Additional comments (4)
src/client/graphics/layers/StructureIconsLayer.ts (1)
5-5: LGTM: Correct Vite asset import pattern.The
?urlsuffix correctly tells Vite to import the asset's URL as a string, which is whatPIXI.Assets.load()expects. This follows Vite's recommended pattern for loading external assets.package.json (3)
4-6: LGTM! Script migration to Vite and Vitest is clean.The script changes follow standard Vite and Vitest patterns. Running type checking in parallel with builds using
concurrentlyis a good approach.Also applies to: 14-14, 16-16
1-125: Address Vite asset import warnings mentioned in PR comments.The PR author noted numerous runtime warnings about assets in the public directory being imported incorrectly. These need to be resolved by:
- Moving assets that need their content imported into
src/(and using?rawsuffix if needed)- Using
?urlsuffix for public assets when only the URL is neededThe author stated they will investigate and update the PR. Ensure these warnings are resolved before merging.
51-52: All Vite plugin and Vitest package versions are compatible.The Vitest packages are at matching versions (4.0.16), which is correct. Vite 7.3.0 satisfies all plugin peer dependencies: vite-plugin-html requires vite >=2.0.0, vite-plugin-static-copy requires vite ^5.0.0 || ^6.0.0 || ^7.0.0, and vite-tsconfig-paths accepts any version. jsdom 27.4.0 is compatible with Vitest 4.0.16. No compatibility issues detected.
|
@evanpelle although I'm not proud of it, i solved the warnings (I did my best to avoid structural or complex changes, but this was the only solution I could come up with) |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/graphics/layers/NameLayer.ts (1)
54-57: Remove duplicate initialization.Lines 54-57 initialize
shieldIconImagetwice with identical code—likely a copy-paste error.🔎 Proposed fix
constructor( private game: GameView, private transformHandler: TransformHandler, private eventBus: EventBus, ) { this.shieldIconImage = new Image(); this.shieldIconImage.src = shieldIcon; - this.shieldIconImage = new Image(); - this.shieldIconImage.src = shieldIcon; }
🧹 Nitpick comments (4)
src/client/graphics/layers/NameLayer.ts (1)
256-280: Consider removing dead code now.The shield icon logic is wrapped in
if (false)with a TODO to remove it. Since this PR already touches asset handling, consider deleting this dead code rather than deferring cleanup.src/client/graphics/layers/GameRightSidebar.ts (1)
13-17: Minor naming inconsistency in constant.The constant
FastForwardIconSolidon line 14 doesn't include "White" in its name, while the actual path isFastForwardIconSolidWhite.svg. Consider renaming tofastForwardIconfor consistency with the other icon constants, or useFastForwardIconSolidWhiteto match the filename exactly.Optional naming improvement
-const FastForwardIconSolid = "/images/FastForwardIconSolidWhite.svg"; +const fastForwardIcon = "/images/FastForwardIconSolidWhite.svg";Then update usage at line 174:
- src=${FastForwardIconSolid} + src=${fastForwardIcon}src/client/sound/SoundManager.ts (1)
1-5: Add a comment explaining the different audio loading approaches.Lines 2–4 bundle background music as ES6 modules (for reliable bundling), while line 5 uses a path string for the sound effect (served from the
resources/directory via Vite'spublicDirsetting). This mixed approach is intentional and working correctly, but the reason should be documented. Add a brief comment above line 5 explaining why music and effects use different loading strategies.Example:
// Sound effects are served as static assets from resources/, // unlike bundled background music const kaChingSound = "/sounds/effects/ka-ching.mp3";src/client/graphics/layers/StructureIconsLayer.ts (1)
78-79: Consider simplifying the renderer initialization guard.You've added a
rendererInitializedflag in addition to the existing null check onrenderer. Sincerendereris only assigned after initialization completes (line 158), the null check alone should be sufficient.Optional simplification
The current pattern:
private renderer: PIXI.Renderer | null = null; private rendererInitialized: boolean = false; // Later... if (!this.renderer || !this.rendererInitialized) { return; }Could be simplified to:
private renderer: PIXI.Renderer | null = null; // Later... if (!this.renderer) { return; }Since
this.rendereris only assigned afterawait renderer.init()completes, checkingthis.renderer !== nullalready guarantees initialization is complete. The separate flag adds redundancy without clear benefit.However, if you anticipate future scenarios where the renderer might exist but not be ready, the explicit flag provides clarity.
Also applies to: 117-159, 209-209
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (34)
.gitignoreeslint.config.jspackage.jsonscripts/sync-assets.mjssrc/client/FlagInputModal.tssrc/client/HostLobbyModal.tssrc/client/LangSelector.tssrc/client/Main.tssrc/client/NewsModal.tssrc/client/SinglePlayerModal.tssrc/client/TerrainMapFileLoader.tssrc/client/graphics/AnimatedSpriteLoader.tssrc/client/graphics/PlayerIcons.tssrc/client/graphics/SpriteLoader.tssrc/client/graphics/layers/BuildMenu.tssrc/client/graphics/layers/ChatModal.tssrc/client/graphics/layers/EventsDisplay.tssrc/client/graphics/layers/GameLeftSidebar.tssrc/client/graphics/layers/GameRightSidebar.tssrc/client/graphics/layers/MainRadialMenu.tssrc/client/graphics/layers/NameLayer.tssrc/client/graphics/layers/PlayerInfoOverlay.tssrc/client/graphics/layers/PlayerPanel.tssrc/client/graphics/layers/RadialMenu.tssrc/client/graphics/layers/RadialMenuElements.tssrc/client/graphics/layers/SettingsModal.tssrc/client/graphics/layers/StructureDrawingUtils.tssrc/client/graphics/layers/StructureIconsLayer.tssrc/client/graphics/layers/StructureLayer.tssrc/client/graphics/layers/UnitDisplay.tssrc/client/sound/SoundManager.tssrc/core/Schemas.tssrc/core/game/BinaryLoaderGameMapLoader.tssrc/core/worker/Worker.worker.ts
✅ Files skipped from review due to trivial changes (4)
- .gitignore
- src/client/graphics/PlayerIcons.ts
- src/client/graphics/layers/MainRadialMenu.ts
- src/client/graphics/layers/EventsDisplay.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/core/worker/Worker.worker.ts
- src/client/graphics/layers/PlayerPanel.ts
- src/client/graphics/layers/ChatModal.ts
- src/client/FlagInputModal.ts
- eslint.config.js
🧰 Additional context used
🧠 Learnings (18)
📓 Common learnings
Learnt from: aaa4xu
Repo: openfrontio/OpenFrontIO PR: 1581
File: webpack.config.js:139-141
Timestamp: 2025-07-26T05:35:03.686Z
Learning: In OpenFrontIO's webpack configuration, binary-loader processes files from resources/maps at build-time and inlines the binary payload directly into the compiled JS bundle. This means the original .bin assets are no longer needed at runtime and can be safely excluded from the final Docker image using webpack's globOptions.ignore pattern to reduce image size.
📚 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/SinglePlayerModal.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/SinglePlayerModal.tssrc/client/HostLobbyModal.tssrc/client/graphics/layers/GameLeftSidebar.tssrc/client/graphics/layers/UnitDisplay.tssrc/client/graphics/layers/SettingsModal.tssrc/client/Main.ts
📚 Learning: 2025-10-18T11:00:57.142Z
Learnt from: NewYearNewPhil
Repo: openfrontio/OpenFrontIO PR: 2230
File: src/client/graphics/GameRenderer.ts:269-277
Timestamp: 2025-10-18T11:00:57.142Z
Learning: In src/client/graphics/GameRenderer.ts, the GameRecapCapture implementation does not use setCaptureRenderEnabled on layers. Instead, it uses RecapCaptureSurface.capture() to render capture layers (TerrainLayer, TerritoryLayer, RailroadLayer, StructureIconsLayer, UnitLayer) directly to an off-screen canvas without requiring layer-level capture mode methods.
Applied to files:
src/client/graphics/layers/StructureIconsLayer.ts
📚 Learning: 2025-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/PlayerInfoOverlay.tssrc/client/graphics/layers/BuildMenu.tssrc/core/Schemas.ts
📚 Learning: 2025-10-20T20:15:28.858Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:51-51
Timestamp: 2025-10-20T20:15:28.858Z
Learning: In src/core/execution/FakeHumanExecution.ts, game balance constants like MIRV_COOLDOWN_TICKS, MIRV_HESITATION_ODDS, VICTORY_DENIAL_TEAM_THRESHOLD, VICTORY_DENIAL_INDIVIDUAL_THRESHOLD, and STEAMROLL_CITY_GAP_MULTIPLIER are experimental tuning parameters subject to frequent change during balance testing. Do not flag changes to these values as issues or compare them against previous values.
Applied to files:
src/client/graphics/layers/PlayerInfoOverlay.tssrc/client/graphics/layers/BuildMenu.ts
📚 Learning: 2025-07-31T12:03:08.052Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1655
File: resources/maps/giantworldmap/manifest.json:165-174
Timestamp: 2025-07-31T12:03:08.052Z
Learning: In OpenFrontIO draft PRs, flag names in map manifest files may be placeholders that will be updated to match the actual SVG flag file names when the assets are received. The final naming depends on the actual flag SVG files provided, not code naming conventions.
Applied to files:
src/client/graphics/layers/PlayerInfoOverlay.ts
📚 Learning: 2025-06-10T09:56:44.473Z
Learnt from: Ble4Ch
Repo: openfrontio/OpenFrontIO PR: 1063
File: src/core/configuration/PastelThemeDark.ts:53-53
Timestamp: 2025-06-10T09:56:44.473Z
Learning: In ColorAllocator class in src/core/configuration/Colors.ts, the correct method names are assignColor(id: string): Colord for general color assignment and assignTeamColor(team: Team): Colord for team-specific colors. There are no assignPlayerColor() or assignBotColor() methods.
Applied to files:
src/client/graphics/layers/PlayerInfoOverlay.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/client/HostLobbyModal.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/RadialMenu.tssrc/client/graphics/layers/RadialMenuElements.ts
📚 Learning: 2025-05-26T09:52:52.465Z
Learnt from: tnhnblgl
Repo: openfrontio/OpenFrontIO PR: 875
File: src/client/index.html:396-402
Timestamp: 2025-05-26T09:52:52.465Z
Learning: In the OpenFrontIO project, the correct path for icons from src/client/index.html is ../../resources/icons/, not /static/icons/ as the build process handles the path mapping differently.
Applied to files:
src/client/graphics/layers/BuildMenu.ts
📚 Learning: 2025-07-23T12:36:35.354Z
Learnt from: Aotumuri
Repo: openfrontio/OpenFrontIO PR: 1534
File: src/client/LangSelector.ts:97-106
Timestamp: 2025-07-23T12:36:35.354Z
Learning: In OpenFrontIO's LangSelector.ts, the getClosestSupportedLang method always joins language code parts with underscores ("_") because all keys in the languageMap use underscore format (e.g., pt_BR, sv_SE, zh_CN). This normalization ensures consistency regardless of whether the input language code uses hyphens or underscores as delimiters.
Applied to files:
src/client/LangSelector.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:
src/client/LangSelector.ts
📚 Learning: 2025-08-16T10:52:08.292Z
Learnt from: TheGiraffe3
Repo: openfrontio/OpenFrontIO PR: 884
File: resources/lang/en.json:456-461
Timestamp: 2025-08-16T10:52:08.292Z
Learning: In OpenFrontIO, translation files in resources/lang/*.json (except en.json) should not be updated in regular PRs. Only dedicated translation PRs titled "mls" and made by Aotumori should update non-English locale files. Regular PRs should only update en.json when adding or modifying translation keys.
Applied to files:
src/client/LangSelector.ts
📚 Learning: 2025-06-07T13:15:55.439Z
Learnt from: Aotumuri
Repo: openfrontio/OpenFrontIO PR: 786
File: src/core/Util.ts:4-4
Timestamp: 2025-06-07T13:15:55.439Z
Learning: In the OpenFrontIO codebase, JSON files should be imported using standard import syntax without import attributes, as the TypeScript configuration supports resolveJsonModule and the codebase already uses this pattern successfully in files like src/client/Cosmetic.ts.
Applied to files:
src/client/LangSelector.tssrc/core/Schemas.tssrc/client/TerrainMapFileLoader.ts
📚 Learning: 2025-05-30T03:53:52.231Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 949
File: resources/lang/en.json:8-10
Timestamp: 2025-05-30T03:53:52.231Z
Learning: For the OpenFrontIO project, do not suggest updating translation files in resources/lang/*.json except for en.json. The project has a dedicated translation team that handles all other locale files.
Applied to files:
src/client/LangSelector.ts
📚 Learning: 2025-08-09T05:03:18.745Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1761
File: src/core/ApiSchemas.ts:2-2
Timestamp: 2025-08-09T05:03:18.745Z
Learning: After migrating to Zod v4 in the OpenFrontIO project, the standard import statement `import { z } from "zod"` should be used across the entire codebase, not `import { z } from "zod/v4"`.
Applied to files:
src/core/Schemas.ts
📚 Learning: 2025-07-26T05:35:03.686Z
Learnt from: aaa4xu
Repo: openfrontio/OpenFrontIO PR: 1581
File: webpack.config.js:139-141
Timestamp: 2025-07-26T05:35:03.686Z
Learning: In OpenFrontIO's webpack configuration, binary-loader processes files from resources/maps at build-time and inlines the binary payload directly into the compiled JS bundle. This means the original .bin assets are no longer needed at runtime and can be safely excluded from the final Docker image using webpack's globOptions.ignore pattern to reduce image size.
Applied to files:
src/core/game/BinaryLoaderGameMapLoader.ts
🧬 Code graph analysis (2)
scripts/sync-assets.mjs (1)
eslint.config.js (2)
__filename(9-9)__dirname(10-10)
src/core/game/BinaryLoaderGameMapLoader.ts (2)
src/core/game/FetchGameMapLoader.ts (1)
url(41-51)src/core/game/TerrainMapLoader.ts (1)
MapManifest(19-25)
🪛 GitHub Actions: 🧪 CI
src/core/Schemas.ts
[error] 3-3: Failed to resolve import "../assets/data/countries.json" from "src/core/Schemas.ts". Does the file exist?
🔇 Additional comments (23)
src/client/graphics/layers/NameLayer.ts (1)
19-19: Asset reference migration looks good.The shift from SVG module import to a string path constant aligns with the Vite migration pattern used across the PR.
src/client/SinglePlayerModal.ts (1)
30-30: Part of asset path migration pattern.The random map image path follows the same migration pattern as other files. Ensure this asset exists at
/images/RandomMap.webpin your public directory.src/client/graphics/layers/RadialMenu.ts (1)
12-12: Asset path migration looks good.The back icon path follows the standard migration pattern.
src/client/graphics/layers/GameLeftSidebar.ts (1)
8-11: Good naming convention for icon variants.The Regular/Solid naming pattern clearly indicates the toggle states for the leaderboard and team icons. This makes the code easier to understand.
src/client/graphics/layers/StructureDrawingUtils.ts (1)
6-11: Document the purpose of dual icon systems in structures rendering.Both StructureDrawingUtils.ts and StructureLayer.ts exist in the codebase and use completely different icon sets:
- StructureDrawingUtils.ts:
/images/AnchorIcon.png,/images/CityIcon.png,/images/FactoryUnit.png, etc.- StructureLayer.ts:
/images/buildings/cityAlt1.png,/images/buildings/factoryAlt1.png,/images/buildings/port1.png, etc.All asset files are present and correctly mapped. However, the reason for maintaining two separate rendering systems with different icon assets is unclear. Add a comment explaining:
- Why these two rendering approaches coexist
- Which use case each system serves
- Whether consolidation is planned
src/client/graphics/layers/StructureLayer.ts (1)
11-16: Building sprite paths are correct and all required assets exist.All six PNG files are correctly placed in
resources/images/buildings/and will resolve to/images/buildings/at runtime due to Vite'spublicDir: "resources"configuration. The naming convention is consistent:cityAlt1.png,factoryAlt1.png,fortAlt3.png,port1.png,silo1.png, andsilo4.png. Paths resolve correctly in both development and production builds. No action needed.src/client/graphics/layers/UnitDisplay.ts (1)
13-22: Asset paths are correctly configured and will resolve at runtime.The hardcoded icon paths like
/images/BattleshipIconWhite.svgare correct. Vite is configured withpublicDir: "resources"in vite.config.ts, which makes the resources directory the public root. All ten referenced SVG files exist inresources/images/and will be accessible at the/images/path during development and production builds. No changes needed.src/client/graphics/layers/SettingsModal.ts (1)
10-21: LGTM! Asset reference migration looks good.The icon imports have been cleanly replaced with public URL path constants, aligning with Vite's static asset handling. The usage in the render method remains unchanged.
package.json (1)
4-28: Scripts migration to Vite and Vitest looks good.The script definitions correctly:
- Use
concurrentlyto run TypeScript checks alongside Vite builds- Replace Jest with Vitest for test commands
- Add
sync-assetspre-hooks to ensure assets are synchronized before dev/build operations- Maintain existing script semantics (dev, tunnel, format, lint, etc.)
The pre-hooks (
predev,prebuild-dev, etc.) ensure the asset sync script runs automatically, reducing the risk of stale assets.src/client/graphics/layers/RadialMenuElements.ts (1)
16-27: LGTM! Icon asset migration is consistent.The icon references have been updated to public URL path constants, matching the pattern used throughout the codebase for the Vite migration. Menu element definitions continue to use these constants without any functional changes.
src/client/TerrainMapFileLoader.ts (1)
1-4: LGTM! Version data import updated correctly.The import path has been updated to the new asset location (
src/assets/data/) with the correct Vite?rawsuffix for importing text content. TheFetchGameMapLoaderusage remains unchanged.src/client/LangSelector.ts (1)
5-86: LGTM! Language file imports migrated correctly.All language JSON imports have been updated to the new
../assets/lang/location while preserving:
- Language code mappings (including special cases like
pt-BR,sv-SE,zh-CN)- Import structure and languageMap consistency
- Existing translation loading and application logic
The change is purely a path update with no modification to translation content, which is the correct approach per project conventions.
src/client/graphics/SpriteLoader.ts (1)
5-36: LGTM! Sprite asset migration is well-structured.The sprite imports have been replaced with public URL path constants, and the
SPRITE_CONFIGmapping has been updated accordingly. The runtime loading logic properly:
- Uses
Image()withcrossOrigin="anonymous"for CORS compatibility- Converts to
ImageBitmapfor efficient rendering- Includes error handling for load failures
- Preserves all existing sprite colorization and caching logic
src/core/game/BinaryLoaderGameMapLoader.ts (1)
31-58: Verify deployment configuration includes binary map files.This change shifts from build-time binary inlining (via Webpack's binary-loader) to runtime fetch-based loading. The code now fetches
.binfiles from/maps/directory, requiring production deployments to serve these files at runtime.Ensure:
- Build pipeline copies binary files from
resources/maps/topublic/maps/ordist/maps/- Docker image includes map assets in the
/maps/directory- Server configuration serves
/maps/as a static asset directory- CI/CD deployment steps handle binary file artifacts correctly
Previous setup excluded
.binfiles from deployment since they were bundled at build-time. This is no longer the case.scripts/sync-assets.mjs (1)
1-59: The sync-assets script is necessary and cannot be eliminated without refactoring imports.The script copies files from
resources/tosrc/assets/because the codebase uses ES module imports for these files. Multiple files import directly from../assets/data/and../assets/lang/(e.g.,import countries from "../assets/data/countries.json",import ar from "../assets/lang/ar.json"). These imports require the files to exist at the specified paths at build and runtime.While Vite is already configured with
publicDir: "resources"and aresourcesalias, these serve different purposes:
publicDirserves static assets via HTTP (for images, fonts in HTML)- ES module imports require actual files at the specified path
To eliminate duplication, refactor imports to use dynamic imports or fetch, then serve assets through
publicDiror theresourcesalias. However, this changes the application's asset loading strategy and is beyond this script's scope.Likely an incorrect or invalid review comment.
src/client/Main.ts (3)
58-58: Asset path migration looks good.The Snowflake image reference has been correctly migrated from a module import to a string path constant. This aligns with the Vite migration pattern where assets in the public directory are referenced by their runtime paths.
48-57: All CSS stylesheet paths exist and are correctly imported.All 10 CSS files are present at their specified paths in the src/client/styles/ directory.
1-1: Fix the version.txt import path—the file is not at the new location.The import uses
../assets/data/version.txt?raw, but this file does not exist. The actual version file is atresources/version.txtin the repository root. Fromsrc/client/Main.ts, the correct import path is:import version from "../../resources/version.txt?raw";Update the import to point to the actual file location.
⛔ Skipped due to learnings
Learnt from: Aotumuri Repo: openfrontio/OpenFrontIO PR: 786 File: src/core/Util.ts:4-4 Timestamp: 2025-06-07T13:15:55.439Z Learning: In the OpenFrontIO codebase, JSON files should be imported using standard import syntax without import attributes, as the TypeScript configuration supports resolveJsonModule and the codebase already uses this pattern successfully in files like src/client/Cosmetic.ts.src/client/HostLobbyModal.ts (1)
34-34: Asset reference migration is consistent.The Random Map image reference follows the same pattern as other asset migrations in this PR. The string path will be correctly resolved by Vite at runtime.
src/client/graphics/AnimatedSpriteLoader.ts (1)
6-22: Sprite asset migration looks good.All sprite imports have been consistently migrated to string path constants. The runtime loading via
Image.srcwill correctly resolve these paths under Vite.src/client/graphics/layers/BuildMenu.ts (1)
26-36: Icon asset migration is consistent.All icon references have been migrated to string path constants, which will be correctly resolved by Vite when used in Lit template
imgelements. This follows the same pattern as other UI components in the PR.src/client/NewsModal.ts (1)
4-4: Asset path migrations look good.The version, changelog, and image asset references have been correctly migrated to their runtime paths. The
?rawsuffix onversion.txtensures the content is imported as a string for version comparison logic.Note: This relies on the same
version.txtverification from Main.ts review.Also applies to: 8-10
src/client/graphics/layers/StructureIconsLayer.ts (1)
42-42: Bitmap font asset path is correct.The font files exist at
resources/fonts/round_6x6_modified.xmlandresources/fonts/round_6x6_modified.png. Vite's configuration serves theresourcesdirectory as the public root (publicDir: "resources"), so the path/fonts/round_6x6_modified.xmlin the code correctly resolves to the bitmap font at runtime.
chore(assets): use ?url imports for public resources
wraith4081
force-pushed
the
build/migrate-webpack-to-vite
branch
from
42e81af to
8e23427
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
package.json (1)
89-89: vite ^7.3.0 does not exist on npm.This was already flagged in a previous review. The version needs to be corrected to an actual published version (e.g., stable v6.x or v5.x series).
🧹 Nitpick comments (5)
src/client/graphics/layers/GameRightSidebar.ts (1)
13-17: Inconsistent naming: use camelCase for all icon constants.
FastForwardIconSoliduses PascalCase whileexitIcon,pauseIcon,playIcon, andsettingsIconuse camelCase. Since these are all module-level constants with the same purpose, they should follow a consistent naming pattern.🔎 Suggested naming fix
-import FastForwardIconSolid from "/images/FastForwardIconSolidWhite.svg?url"; +import fastForwardIcon from "/images/FastForwardIconSolidWhite.svg?url";Then update the usage at line 174:
- src=${FastForwardIconSolid} + src=${fastForwardIcon}src/client/graphics/layers/StructureIconsLayer.ts (1)
78-79: Consider removing the redundantrendererInitializedflag.The
rendererInitializedboolean duplicates information already captured by therendererproperty. Sincerenderer.init()is awaited before assignment (line 147-158), ifthis.rendereris non-null, it's guaranteed to be initialized. The null check alone provides the same safety.Simplify to single null check
Remove the boolean flag:
private readonly theme: Theme; private renderer: PIXI.Renderer | null = null; - private rendererInitialized: boolean = false; private renders: StructureRenderInfo[] = [];Remove the flag assignment:
this.renderer = renderer; - this.rendererInitialized = true; }Simplify the guard:
renderLayer(mainContext: CanvasRenderingContext2D) { - if (!this.renderer || !this.rendererInitialized) { + if (!this.renderer) { return; }Also applies to: 158-159, 209-209
src/core/game/BinaryLoaderGameMapLoader.ts (3)
31-37: ExtractloadBinaryas a private method and consider adding fetch timeout.The
loadBinaryhelper is recreated on everygetMapDatacall. Extracting it as a private class method would avoid repeated function allocation and improve readability. Additionally, the fetch has no timeout, which could cause indefinite hangs on network issues.🔎 Proposed refactor
+ private async loadBinary(url: string): Promise<Uint8Array> { + const res = await fetch(url); + if (!res.ok) throw new Error(`Failed to load ${url}`); + const buf = await res.arrayBuffer(); + return new Uint8Array(buf); + } + getMapData(map: GameMapType): MapData { const cachedMap = this.maps.get(map); if (cachedMap) { return cachedMap; } const key = Object.keys(GameMapType).find( (k) => GameMapType[k as keyof typeof GameMapType] === map, ); const fileName = key?.toLowerCase(); - const loadBinary = (url: string) => - fetch(url) - .then((res) => { - if (!res.ok) throw new Error(`Failed to load ${url}`); - return res.arrayBuffer(); - }) - .then((buf) => new Uint8Array(buf)); - const mapBasePath = `/maps/${fileName}`; const mapData = { - mapBin: this.createLazyLoader(() => loadBinary(`${mapBasePath}/map.bin`)), + mapBin: this.createLazyLoader(() => this.loadBinary(`${mapBasePath}/map.bin`)), map4xBin: this.createLazyLoader(() => - loadBinary(`${mapBasePath}/map4x.bin`), + this.loadBinary(`${mapBasePath}/map4x.bin`), ), map16xBin: this.createLazyLoader(() => - loadBinary(`${mapBasePath}/map16x.bin`), + this.loadBinary(`${mapBasePath}/map16x.bin`), ),For timeout handling, consider using
AbortSignal.timeout()if targeting modern browsers, or a custom timeout wrapper.
12-17: Consider retry logic for transient fetch failures.The lazy loader caches the promise, including failures. If a map asset fetch fails due to a transient network issue, all subsequent calls return the same rejected promise without retrying. For improved reliability, consider implementing retry logic or clearing the cache on failure to allow retries.
💡 Example with retry on failure
private createLazyLoader<T>(importFn: () => Promise<T>): () => Promise<T> { let cache: Promise<T> | null = null; return () => { if (!cache) { cache = importFn().catch((err) => { cache = null; // Clear cache on failure to allow retry throw err; }); } return cache; }; }
49-56: Optional: Add runtime validation for manifest data.The manifest is fetched and type-asserted to
MapManifestwithout runtime validation. If the JSON structure doesn't match expectations, it could cause runtime errors downstream. Consider adding validation or at least checking for required fields to provide clearer error messages.💡 Example validation
manifest: this.createLazyLoader(() => fetch(`${mapBasePath}/manifest.json`).then(async (res) => { if (!res.ok) { throw new Error(`Failed to load ${mapBasePath}/manifest.json`); } const data = await res.json(); // Basic validation if (!data.name || !data.map || !data.nations) { throw new Error(`Invalid manifest format in ${mapBasePath}/manifest.json`); } return data as MapManifest; }), ),
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (34)
.gitignoreeslint.config.jspackage.jsonscripts/sync-assets.mjssrc/client/FlagInputModal.tssrc/client/HostLobbyModal.tssrc/client/LangSelector.tssrc/client/Main.tssrc/client/NewsModal.tssrc/client/SinglePlayerModal.tssrc/client/TerrainMapFileLoader.tssrc/client/graphics/AnimatedSpriteLoader.tssrc/client/graphics/PlayerIcons.tssrc/client/graphics/SpriteLoader.tssrc/client/graphics/layers/BuildMenu.tssrc/client/graphics/layers/ChatModal.tssrc/client/graphics/layers/EventsDisplay.tssrc/client/graphics/layers/GameLeftSidebar.tssrc/client/graphics/layers/GameRightSidebar.tssrc/client/graphics/layers/MainRadialMenu.tssrc/client/graphics/layers/NameLayer.tssrc/client/graphics/layers/PlayerInfoOverlay.tssrc/client/graphics/layers/PlayerPanel.tssrc/client/graphics/layers/RadialMenu.tssrc/client/graphics/layers/RadialMenuElements.tssrc/client/graphics/layers/SettingsModal.tssrc/client/graphics/layers/StructureDrawingUtils.tssrc/client/graphics/layers/StructureIconsLayer.tssrc/client/graphics/layers/StructureLayer.tssrc/client/graphics/layers/UnitDisplay.tssrc/client/sound/SoundManager.tssrc/core/Schemas.tssrc/core/game/BinaryLoaderGameMapLoader.tssrc/core/worker/Worker.worker.ts
🚧 Files skipped from review as they are similar to previous changes (20)
- src/client/TerrainMapFileLoader.ts
- src/client/graphics/layers/SettingsModal.ts
- src/client/Main.ts
- src/client/graphics/layers/ChatModal.ts
- src/client/LangSelector.ts
- src/client/NewsModal.ts
- src/client/graphics/layers/StructureDrawingUtils.ts
- src/client/graphics/SpriteLoader.ts
- src/client/graphics/layers/PlayerPanel.ts
- scripts/sync-assets.mjs
- src/client/graphics/layers/UnitDisplay.ts
- src/client/graphics/layers/MainRadialMenu.ts
- src/client/graphics/layers/RadialMenuElements.ts
- src/client/graphics/layers/EventsDisplay.ts
- src/core/Schemas.ts
- .gitignore
- src/client/graphics/layers/PlayerInfoOverlay.ts
- src/client/graphics/layers/NameLayer.ts
- eslint.config.js
- src/client/graphics/layers/StructureLayer.ts
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: aaa4xu
Repo: openfrontio/OpenFrontIO PR: 1581
File: webpack.config.js:139-141
Timestamp: 2025-07-26T05:35:03.686Z
Learning: In OpenFrontIO's webpack configuration, binary-loader processes files from resources/maps at build-time and inlines the binary payload directly into the compiled JS bundle. This means the original .bin assets are no longer needed at runtime and can be safely excluded from the final Docker image using webpack's globOptions.ignore pattern to reduce image size.
📚 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/GameLeftSidebar.tssrc/client/graphics/layers/GameRightSidebar.tssrc/client/SinglePlayerModal.tssrc/client/HostLobbyModal.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/RadialMenu.ts
📚 Learning: 2025-06-07T13:15:55.439Z
Learnt from: Aotumuri
Repo: openfrontio/OpenFrontIO PR: 786
File: src/core/Util.ts:4-4
Timestamp: 2025-06-07T13:15:55.439Z
Learning: In the OpenFrontIO codebase, JSON files should be imported using standard import syntax without import attributes, as the TypeScript configuration supports resolveJsonModule and the codebase already uses this pattern successfully in files like src/client/Cosmetic.ts.
Applied to files:
src/client/FlagInputModal.tssrc/client/graphics/AnimatedSpriteLoader.tssrc/client/SinglePlayerModal.ts
📚 Learning: 2025-10-18T11:00:57.142Z
Learnt from: NewYearNewPhil
Repo: openfrontio/OpenFrontIO PR: 2230
File: src/client/graphics/GameRenderer.ts:269-277
Timestamp: 2025-10-18T11:00:57.142Z
Learning: In src/client/graphics/GameRenderer.ts, the GameRecapCapture implementation does not use setCaptureRenderEnabled on layers. Instead, it uses RecapCaptureSurface.capture() to render capture layers (TerrainLayer, TerritoryLayer, RailroadLayer, StructureIconsLayer, UnitLayer) directly to an off-screen canvas without requiring layer-level capture mode methods.
Applied to files:
src/client/graphics/layers/StructureIconsLayer.ts
📚 Learning: 2025-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/client/HostLobbyModal.ts
📚 Learning: 2025-07-26T05:35:03.686Z
Learnt from: aaa4xu
Repo: openfrontio/OpenFrontIO PR: 1581
File: webpack.config.js:139-141
Timestamp: 2025-07-26T05:35:03.686Z
Learning: In OpenFrontIO's webpack configuration, binary-loader processes files from resources/maps at build-time and inlines the binary payload directly into the compiled JS bundle. This means the original .bin assets are no longer needed at runtime and can be safely excluded from the final Docker image using webpack's globOptions.ignore pattern to reduce image size.
Applied to files:
src/core/game/BinaryLoaderGameMapLoader.ts
📚 Learning: 2025-05-26T09:52:52.465Z
Learnt from: tnhnblgl
Repo: openfrontio/OpenFrontIO PR: 875
File: src/client/index.html:396-402
Timestamp: 2025-05-26T09:52:52.465Z
Learning: In the OpenFrontIO project, the correct path for icons from src/client/index.html is ../../resources/icons/, not /static/icons/ as the build process handles the path mapping differently.
Applied to files:
src/client/graphics/layers/BuildMenu.ts
🧬 Code graph analysis (1)
src/core/game/BinaryLoaderGameMapLoader.ts (2)
src/core/game/FetchGameMapLoader.ts (1)
url(41-51)src/core/game/TerrainMapLoader.ts (1)
MapManifest(19-25)
🔇 Additional comments (14)
src/client/graphics/layers/GameLeftSidebar.ts (1)
8-11: Asset imports migrated correctly to Vite pattern.The SVG icon imports now use absolute public paths with
?urlsuffix, which is the standard Vite approach for asset URL resolution. These changes align with the broader migration from Webpack to Vite.src/client/graphics/layers/BuildMenu.ts (1)
26-36: Icon imports updated correctly for Vite.All icon assets now use the
/images/...?urlpattern for URL-based resolution. The imports are properly consumed in thebuildTableconfiguration and render logic.src/client/graphics/layers/StructureIconsLayer.ts (1)
42-42: Font import migrated correctly to Vite.The bitmap font now uses the
/fonts/...?urlpattern for public asset URL resolution, consistent with the Vite migration.src/client/sound/SoundManager.ts (1)
2-5: No changes needed. The import patterns are correct for their respective asset types: proprietary music is bundled from source (no?urlneeded), while the sound effect is served from the public directory (correctly using absolute path with?url). This design is appropriate for the Webpack→Vite migration.Likely an incorrect or invalid review comment.
src/client/graphics/layers/RadialMenu.ts (1)
12-12: Asset location is correct and TypeScript types are properly configured.The Vite
?urlimport pattern is correct. The asset exists atresources/images/BackIconWhite.svgand TypeScript recognizes the syntax viavite-env.d.tswhich referencesvite/clienttypes. This import pattern is used consistently throughout the codebase (PlayerIcons.ts, EventsDisplay.ts, RadialMenuElements.ts, etc.), confirming the migration approach is sound.src/client/graphics/AnimatedSpriteLoader.ts (2)
196-225: Loading logic correctly handles URL-based imports.The sprite loading implementation correctly uses the URL strings from the imports. The
Image.srcassignment at line 207 works seamlessly with Vite's URL imports.
6-22: All sprite assets are properly available at their new paths.All 17 sprite files exist in
resources/sprites/and the Vite configuration (withpublicDir: "resources") correctly maps/sprites/URLs to the asset directory. The christmas subdirectory structure is intact with all 4 files in place.src/client/SinglePlayerModal.ts (1)
30-30: The import statement and asset path are correct. The fileresources/images/RandomMap.webpexists and is properly accessible at/images/RandomMap.webpbecausevite.config.tsconfigurespublicDir: "resources", making all files in the resources directory available at the root path during build and development. No action needed.Likely an incorrect or invalid review comment.
src/client/graphics/PlayerIcons.ts (1)
3-15: The Vite URL import pattern is correctly implemented.All 13 SVG assets exist in
resources/images/and are properly accessible via/images/paths due to Vite'spublicDir: "resources"configuration. TypeScript type support for?urlimports is provided by the/// <reference types="vite/client" />reference invite-env.d.ts, which gives you full IDE support and type safety for these asset imports. No action needed.src/client/HostLobbyModal.ts (1)
34-34: LGTM! Correct Vite asset import pattern.The
?urlsuffix tells Vite to return the resolved public URL as a string, which is exactly what the<img src=...>on line 224 expects. This aligns with the broader migration to Vite-style asset handling.package.json (4)
4-8: LGTM! Clean build and start script migration.Running
tsc --noEmitin parallel withvite buildis a solid pattern—type-check without emitting, let Vite handle the bundling. The--kill-others-on-failflag on prod build ensures fast failure in CI.
24-28: LGTM! Pre-hooks ensure assets are ready.Running
sync-assetsbefore dev, build, and client start prevents missing asset errors at runtime. Clean approach for the Vite migration.
90-92: Vite plugin versions look reasonable.These plugins should work with a stable vite version. Once the vite version is fixed, verify these plugins load correctly during dev.
56-57: No action needed. Vitest 4.0.16 is compatible with Vite 7.3.0 (Vitest 4.x supports Vite ^6.0.0 || ^7.0.0).
| const mapBasePath = `/maps/${fileName}`; | ||
|
|
||
| const mapData = { | ||
| mapBin: this.createLazyLoader(() => | ||
| ( | ||
| import( | ||
| `!!binary-loader!../../../resources/maps/${fileName}/map.bin` | ||
| ) as Promise<BinModule> | ||
| ).then((m) => this.toUInt8Array(m.default)), | ||
| ), | ||
| mapBin: this.createLazyLoader(() => loadBinary(`${mapBasePath}/map.bin`)), | ||
| map4xBin: this.createLazyLoader(() => | ||
| ( | ||
| import( | ||
| `!!binary-loader!../../../resources/maps/${fileName}/map4x.bin` | ||
| ) as Promise<BinModule> | ||
| ).then((m) => this.toUInt8Array(m.default)), | ||
| loadBinary(`${mapBasePath}/map4x.bin`), | ||
| ), | ||
| map16xBin: this.createLazyLoader(() => | ||
| ( | ||
| import( | ||
| `!!binary-loader!../../../resources/maps/${fileName}/map16x.bin` | ||
| ) as Promise<BinModule> | ||
| ).then((m) => this.toUInt8Array(m.default)), | ||
| loadBinary(`${mapBasePath}/map16x.bin`), | ||
| ), | ||
| manifest: this.createLazyLoader(() => | ||
| ( | ||
| import( | ||
| `../../../resources/maps/${fileName}/manifest.json` | ||
| ) as Promise<NationMapModule> | ||
| ).then((m) => m.default), | ||
| fetch(`${mapBasePath}/manifest.json`).then((res) => { | ||
| if (!res.ok) { | ||
| throw new Error(`Failed to load ${mapBasePath}/manifest.json`); | ||
| } | ||
| return res.json() as Promise<MapManifest>; | ||
| }), | ||
| ), | ||
| webpPath: this.createLazyLoader(() => | ||
| ( | ||
| import( | ||
| `../../../resources/maps/${fileName}/thumbnail.webp` | ||
| ) as Promise<{ default: string }> | ||
| ).then((m) => m.default), | ||
| Promise.resolve(`${mapBasePath}/thumbnail.webp`), | ||
| ), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find Vite configuration and build setup
find . -maxdepth 2 -type f \( -name "vite.config.*" -o -name "tsconfig.json" -o -name "package.json" \) | head -20Repository: openfrontio/OpenFrontIO
Length of output: 114
🏁 Script executed:
# Look for any binary loader or map asset handling in the codebase
rg "binary-loader|\.bin|loadBinary|map\.bin" --type ts --type js -l | head -20Repository: openfrontio/OpenFrontIO
Length of output: 468
🏁 Script executed:
# Check the structure of the public or assets directory
ls -la public/ 2>/dev/null || echo "No public dir" && find . -maxdepth 2 -type d -name "maps" -o -name "public" -o -name "assets"Repository: openfrontio/OpenFrontIO
Length of output: 120
🏁 Script executed:
# Look at the BinaryLoaderGameMapLoader to understand loadBinary implementation
fd "BinaryLoaderGameMapLoader" --type fRepository: openfrontio/OpenFrontIO
Length of output: 109
🏁 Script executed:
# Read Vite configuration
cat vite.config.tsRepository: openfrontio/OpenFrontIO
Length of output: 3750
🏁 Script executed:
# Read the BinaryLoaderGameMapLoader implementation
cat src/core/game/BinaryLoaderGameMapLoader.tsRepository: openfrontio/OpenFrontIO
Length of output: 1972
🏁 Script executed:
# Check if FetchGameMapLoader exists and how it differs
cat src/core/game/FetchGameMapLoader.tsRepository: openfrontio/OpenFrontIO
Length of output: 2056
🏁 Script executed:
# Check build scripts in package.json
rg "scripts|build|vite" package.json -A 10 -B 2Repository: openfrontio/OpenFrontIO
Length of output: 2803
🏁 Script executed:
# Check resources/maps directory structure
ls -la resources/maps/ | head -30Repository: openfrontio/OpenFrontIO
Length of output: 1793
🏁 Script executed:
# Check the sync-assets script
cat scripts/sync-assets.mjsRepository: openfrontio/OpenFrontIO
Length of output: 1646
🏁 Script executed:
# Check if resources/maps is actually copied to build output
ls -la resources/maps/europe/ | head -20Repository: openfrontio/OpenFrontIO
Length of output: 511
🏁 Script executed:
# Look for Docker-related files
find . -maxdepth 2 -type f \( -name "Dockerfile*" -o -name "docker-compose*" -o -name ".dockerignore" \)Repository: openfrontio/OpenFrontIO
Length of output: 95
🏁 Script executed:
# Check if there are any webpack or build config references
rg "webpack|binary-loader|\.bin" vite.config.ts package.json -B 2 -A 2Repository: openfrontio/OpenFrontIO
Length of output: 49
🏁 Script executed:
# Check Dockerfile
cat DockerfileRepository: openfrontio/OpenFrontIO
Length of output: 2645
🏁 Script executed:
# Check .dockerignore
cat .dockerignoreRepository: openfrontio/OpenFrontIO
Length of output: 227
🏁 Script executed:
# Check how Vite handles the publicDir during build - look for build output handling
rg "publicDir|resources|viteStaticCopy" vite.config.ts -A 3 -B 1Repository: openfrontio/OpenFrontIO
Length of output: 624
Critical: Map assets are missing from Docker production image.
The migration from Webpack to Vite changes how binary map assets are handled. Previously with Webpack's binary-loader, .bin files were inlined into the bundle at build time and didn't need deployment. Now with Vite, all map assets (map.bin, map4x.bin, map16x.bin, manifest.json, thumbnail.webp) are loaded at runtime via fetch from the /maps/${fileName}/ path.
The Dockerfile explicitly removes the maps directory with RUN rm -rf ./resources/maps, but BinaryLoaderGameMapLoader expects to fetch these files at runtime. When deployed, requests for /maps/europe/map.bin and similar assets will fail with 404 errors.
Update the Dockerfile to preserve the resources/maps directory in the final production image, or implement an alternative deployment strategy to serve map assets at the /maps/ endpoint.
🤖 Prompt for AI Agents
In src/core/game/BinaryLoaderGameMapLoader.ts around lines 39 to 59, the loader
fetches map assets at runtime from /maps/${fileName}/ (map.bin, map4x.bin,
map16x.bin, manifest.json, thumbnail.webp) but the Dockerfile currently deletes
./resources/maps so those requests 404; fix by adjusting the deployment so the
maps directory is present and served: update the Dockerfile to copy/preserve
resources/maps into the final image (or copy them into the build/static/public
directory that Vite/Nginx serves) and ensure the container’s static server is
configured to serve that folder at the /maps/ path, or alternatively change the
build to embed these assets into the app bundle or configure an alternative
asset host and update the loader base path accordingly.
evanpelle
left a comment
evanpelle
left a comment
There was a problem hiding this comment.
Looks like the CI is failing
evanpelle
left a comment
evanpelle
left a comment
There was a problem hiding this comment.
Thanks! I bet this was a lot of work!
4 participants
src/corefor safe, hybrid environment variable access (Vite vs Node).process.envdependencies in shared code.Description:
migrate build system to Vite and test runner to Vitest & Remove depracated husky usage
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
wraith4081