Conversation
Contributor
WalkthroughThis pull request consolidates in-game ad components, refactors the lobby lifecycle API from callbacks to promises, updates UI styling for consistency, reworks rate-limiting mechanisms, adjusts game-spawn logic, and modifies annexation behavior with supporting tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
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}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/server/ClientMsgRateLimiter.test.ts (1)
9-31:⚠️ Potential issue | 🟡 MinorMissing test for oversized intent kick.
The implementation kicks when an intent exceeds
MAX_INTENT_SIZE(500 bytes), but there is no test for this behavior. Consider adding a test case.🧪 Suggested test case
describe("intent messages", () => { it("allows intents within limits", () => { const limiter = new ClientMsgRateLimiter(); expect(limiter.check(CLIENT_A, "intent", SMALL)).toBe("ok"); }); + it("kicks when intent exceeds max size", () => { + const limiter = new ClientMsgRateLimiter(); + expect(limiter.check(CLIENT_A, "intent", 501)).toBe("kick"); + }); + it("limits when per-second count exceeded", () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/server/ClientMsgRateLimiter.test.ts` around lines 9 - 31, Add a test to cover the oversized-intent branch: create a ClientMsgRateLimiter instance and call its check method with message type "intent" and a payload whose size exceeds MAX_INTENT_SIZE (the constant used by the implementation, e.g., >500 bytes) for CLIENT_A, then assert the result is the expected "kick" outcome; place this new it-block alongside the other intent tests to ensure oversized intents trigger the kick path in ClientMsgRateLimiter.check.
🧹 Nitpick comments (9)
src/server/Worker.ts (1)
618-635: Simple cleanup approach is acceptable, but note the trade-off.Clearing all IP limiters every 10 minutes is straightforward and prevents memory growth. However, active rate-limited IPs get a fresh token budget immediately after cleanup.
If stricter enforcement is needed later, consider tracking last-access time and only removing stale entries. For now, this simple approach is fine since authenticated clients get per-client rate limiting in
ClientMsgRateLimiteranyway.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/Worker.ts` around lines 618 - 635, The current cleanup clears the entire wsIpLimiters map every 10 minutes which resets token budgets for active IPs; update the implementation to track last access per IP and only remove stale limiters instead of clearing all: augment the wsIpLimiters Map entries (or create a parallel Map) to store a timestamp updated inside getWsIpLimiter(ip) whenever a limiter is retrieved/created, and replace the setInterval(() => wsIpLimiters.clear(), ...) task with a sweeper that iterates wsIpLimiters and deletes entries whose last-access timestamp is older than a configurable stale threshold (e.g., 10 minutes); keep the RateLimiter creation logic in getWsIpLimiter and ensure timestamp updates occur on both creation and retrieval.src/core/execution/PlayerExecution.ts (1)
101-103: Replace hard-coded tile threshold with a named constant.The
100threshold would be easier to tune and reason about if named once nearticksPerClusterCalc.♻️ Suggested refactor
export class PlayerExecution implements Execution { private readonly ticksPerClusterCalc = 20; + private readonly smallPlayerClusterRecalcTileThreshold = 100; @@ if ( ticks - this.lastCalc > this.ticksPerClusterCalc || - this.player.numTilesOwned() < 100 + this.player.numTilesOwned() < this.smallPlayerClusterRecalcTileThreshold ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/execution/PlayerExecution.ts` around lines 101 - 103, Introduce a named constant for the hard-coded tile threshold used in the PlayerExecution cluster recalculation condition: instead of comparing this.player.numTilesOwned() < 100 inline, declare a constant (e.g. minTilesForClusterCalc or TILE_THRESHOLD_FOR_CLUSTER) alongside ticksPerClusterCalc and use that constant in the condition; update the condition that references this.lastCalc and this.ticksPerClusterCalc to use the new constant so the threshold is centralized and easier to tune (reference: class PlayerExecution, properties ticksPerClusterCalc and lastCalc, and method/usage player.numTilesOwned()).tests/core/executions/NoInverseAnnexation.test.ts (2)
58-60: Drop the no-op assertion at Line 59.
expect(largePlayer.numTilesOwned()).toBe(initialLargeTiles)immediately after assignment does not add signal.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/executions/NoInverseAnnexation.test.ts` around lines 58 - 60, Remove the redundant assertion that re-checks the same value just assigned: delete the expect(largePlayer.numTilesOwned()).toBe(initialLargeTiles) line (the no-op using initialLargeTiles and largePlayer.numTilesOwned()). Keep the meaningful assertion about smallPlayer.numTilesOwned() or replace the removed line with a real behavior check if needed; reference the variables initialLargeTiles, largePlayer.numTilesOwned(), and smallPlayer.numTilesOwned() to locate and edit the test.
63-70: Make tick advancement less brittle to internal timing changes.This test depends on fixed values (
20,50) that are tied to cluster recalculation cadence. Consider advancing until the expected state (with a max tick cap) so the test is stable if cadence changes.♻️ Suggested refactor
- // Keep ticksPerClusterCalc and lastTileChange in mind - executeTicks(game, 20); + // Warm up simulation before the trigger actions + executeTicks(game, 25); largePlayer.conquer(game.ref(49, 49)); smallPlayer.conquer(game.ref(50, 50)); - // Annexation happens here - executeTicks(game, 50); + // Annexation should happen within a bounded window + let steps = 0; + const maxSteps = 120; + while (steps < maxSteps && smallPlayer.numTilesOwned() > 0) { + game.executeNextTick(); + steps++; + } + expect(steps).toBeLessThan(maxSteps); expect(largePlayer.numTilesOwned()).toBeGreaterThan(initialLargeTiles); expect(smallPlayer.numTilesOwned()).toBe(0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/executions/NoInverseAnnexation.test.ts` around lines 63 - 70, Replace the hardcoded tick advances with a loop that calls executeTicks until the expected post-annexation state is observed or a max tick cap is reached: after calling largePlayer.conquer(game.ref(49, 49)) and smallPlayer.conquer(game.ref(50, 50)), repeatedly call executeTicks(game, 1) (or a small step) and check the conditions using largePlayer.numTilesOwned() > initialLargeTiles and smallPlayer.numTilesOwned() === 0; break when both are true or throw/assert failure after the cap to avoid flakiness. Use the existing functions executeTicks, largePlayer.conquer, smallPlayer.conquer, largePlayer.numTilesOwned, smallPlayer.numTilesOwned and preserve initialLargeTiles in the check. Ensure the loop enforces a reasonable maxTicks to prevent infinite loops in CI.src/core/execution/utils/AiAttackBehavior.ts (1)
850-853: Recommended: centralize the public-game donation policy.
This new guard is good, but keeping the rule only inAiAttackBehavior.donateTroops()can cause policy drift. Prefer one shared eligibility check (e.g., config-level or execution-level) so all troop-donation paths follow the same rule.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/execution/utils/AiAttackBehavior.ts` around lines 850 - 853, The guard in AiAttackBehavior.donateTroops() that checks this.game.config().gameConfig().gameType === GameType.Public should be centralized: add a single shared eligibility method (e.g., GameConfig.canDonateTroops() or ExecutionContext.isDonationAllowed()) that encapsulates the public-game rule (and any future rules), replace the inline check in AiAttackBehavior.donateTroops() with a call to that new method, and update all other troop-donation entry points to call the same method so all donation paths follow the same policy.src/core/configuration/DefaultConfig.ts (1)
548-554: Optional cleanup: extract spawn turn literals into named constants.
Using named constants for100,150, and300would make future balance tuning easier and reduce magic numbers in config logic.♻️ Suggested refactor
+const SINGLEPLAYER_SPAWN_PHASE_TURNS = 100; +const RANDOM_SPAWN_PHASE_TURNS = 150; +const DEFAULT_SPAWN_PHASE_TURNS = 300; ... numSpawnPhaseTurns(): number { if (this._gameConfig.gameType === GameType.Singleplayer) { - return 100; + return SINGLEPLAYER_SPAWN_PHASE_TURNS; } if (this.isRandomSpawn()) { - return 150; + return RANDOM_SPAWN_PHASE_TURNS; } - return 300; + return DEFAULT_SPAWN_PHASE_TURNS; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/configuration/DefaultConfig.ts` around lines 548 - 554, Replace the magic literals 100, 150, and 300 in DefaultConfig with named constants to make tuning clearer: add constants (e.g., SINGLEPLAYER_SPAWN_TURN = 100, RANDOM_SPAWN_TURN = 150, DEFAULT_SPAWN_TURN = 300) near the top of the DefaultConfig module/class and update the method using this._gameConfig.gameType and isRandomSpawn() to return those constants instead of raw numbers; ensure the constant names are descriptive and used in the return statements so future balance changes only require editing the constants.src/client/ClientGameRunner.ts (1)
80-83: Consider adding promise rejection for error cases.The
prestartPromiseandjoinPromisenever reject. If the connection fails or an error occurs before these lifecycle events, the promises remain pending forever. This could leave callers in a stuck state.One approach: Add
rejectPrestartandrejectJoinhandlers, then call them in the"error"message handler.♻️ Possible enhancement for error handling
let resolvePrestart: () => void; let resolveJoin: () => void; + let rejectPrestart: (err: Error) => void; + let rejectJoin: (err: Error) => void; - const prestartPromise = new Promise<void>((r) => (resolvePrestart = r)); - const joinPromise = new Promise<void>((r) => (resolveJoin = r)); + const prestartPromise = new Promise<void>((resolve, reject) => { + resolvePrestart = resolve; + rejectPrestart = reject; + }); + const joinPromise = new Promise<void>((resolve, reject) => { + resolveJoin = resolve; + rejectJoin = reject; + });Then in the error handler around line 165:
if (message.type === "error") { + const err = new Error(message.error); + rejectPrestart(err); + rejectJoin(err); if (message.error === "full-lobby") {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/ClientGameRunner.ts` around lines 80 - 83, prestartPromise and joinPromise are never rejected so they can hang forever; add paired reject handlers (e.g., rejectPrestart and rejectJoin) when creating the promises (alongside resolvePrestart/resolveJoin) and store them, then invoke those reject functions from the connection's "error" handler (and any connection close/failure handler) so callers receive a rejection on connection failure or other errors; update any cleanup logic to avoid calling both resolve and reject for the same promise.src/client/Main.ts (1)
764-818: Consider extracting the prestart handler to a named method.This
.then()callback spans ~50 lines handling modal closures, UI cleanup, and loading state. Extracting it to a method likeonLobbyPrestart()would improve readability and make the flow easier to follow.♻️ Example extraction
+ private onLobbyPrestart(): void { + console.log("Closing modals"); + document.getElementById("settings-button")?.classList.add("hidden"); + // ... rest of the prestart logic + } this.lobbyHandle.prestart.then(() => { - console.log("Closing modals"); - document.getElementById("settings-button")?.classList.add("hidden"); - // ... 50+ lines + this.onLobbyPrestart(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/Main.ts` around lines 764 - 818, Extract the long anonymous prestart handler passed to this.lobbyHandle.prestart into a named instance method (suggested name: onLobbyPrestart) and replace the .then(...) callback with this.onLobbyPrestart.bind(this) or this.onLobbyPrestart(); move all logic that manipulates UI elements (settings-button hiding, usernameInput.validationError reset, username-validation-error hiding, joinModal.closeWithoutLeaving, the array-forEach that closes modals, gameModeSelector.stop, ad hiding, crazyGamesSDK.loadingStart and the startingModal lookup/show) into that new onLobbyPrestart method so the Main class keeps the same behavior but with clearer structure and a concise prestart hookup.src/client/graphics/layers/InGamePromo.ts (1)
138-144: Consider using rem or px instead of cm for positioning.
style="bottom: -0.7cm"uses centimeters which can render differently across screens and DPI settings. Usingrem(relative to root font size) orpxprovides more predictable behavior.♻️ Suggested change
<div id="${AD_CONTAINER_ID}" class="fixed left-0 z-[100] pointer-events-auto" - style="bottom: -0.7cm" + style="bottom: -0.5rem" ></div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/InGamePromo.ts` around lines 138 - 144, The inline style on the ad container uses physical units ("bottom: -0.7cm") which can vary across devices; update the InGamePromo component to use a screen-relative unit (e.g., replace the "bottom" value on the element with a rem or px value) by changing the style on the element that renders AD_CONTAINER_ID from "bottom: -0.7cm" to a consistent unit such as "bottom: -0.7rem" or an equivalent "bottom: -12px" so positioning is predictable across DPIs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@tests/server/ClientMsgRateLimiter.test.ts`:
- Around line 9-31: Add a test to cover the oversized-intent branch: create a
ClientMsgRateLimiter instance and call its check method with message type
"intent" and a payload whose size exceeds MAX_INTENT_SIZE (the constant used by
the implementation, e.g., >500 bytes) for CLIENT_A, then assert the result is
the expected "kick" outcome; place this new it-block alongside the other intent
tests to ensure oversized intents trigger the kick path in
ClientMsgRateLimiter.check.
---
Nitpick comments:
In `@src/client/ClientGameRunner.ts`:
- Around line 80-83: prestartPromise and joinPromise are never rejected so they
can hang forever; add paired reject handlers (e.g., rejectPrestart and
rejectJoin) when creating the promises (alongside resolvePrestart/resolveJoin)
and store them, then invoke those reject functions from the connection's "error"
handler (and any connection close/failure handler) so callers receive a
rejection on connection failure or other errors; update any cleanup logic to
avoid calling both resolve and reject for the same promise.
In `@src/client/graphics/layers/InGamePromo.ts`:
- Around line 138-144: The inline style on the ad container uses physical units
("bottom: -0.7cm") which can vary across devices; update the InGamePromo
component to use a screen-relative unit (e.g., replace the "bottom" value on the
element with a rem or px value) by changing the style on the element that
renders AD_CONTAINER_ID from "bottom: -0.7cm" to a consistent unit such as
"bottom: -0.7rem" or an equivalent "bottom: -12px" so positioning is predictable
across DPIs.
In `@src/client/Main.ts`:
- Around line 764-818: Extract the long anonymous prestart handler passed to
this.lobbyHandle.prestart into a named instance method (suggested name:
onLobbyPrestart) and replace the .then(...) callback with
this.onLobbyPrestart.bind(this) or this.onLobbyPrestart(); move all logic that
manipulates UI elements (settings-button hiding, usernameInput.validationError
reset, username-validation-error hiding, joinModal.closeWithoutLeaving, the
array-forEach that closes modals, gameModeSelector.stop, ad hiding,
crazyGamesSDK.loadingStart and the startingModal lookup/show) into that new
onLobbyPrestart method so the Main class keeps the same behavior but with
clearer structure and a concise prestart hookup.
In `@src/core/configuration/DefaultConfig.ts`:
- Around line 548-554: Replace the magic literals 100, 150, and 300 in
DefaultConfig with named constants to make tuning clearer: add constants (e.g.,
SINGLEPLAYER_SPAWN_TURN = 100, RANDOM_SPAWN_TURN = 150, DEFAULT_SPAWN_TURN =
300) near the top of the DefaultConfig module/class and update the method using
this._gameConfig.gameType and isRandomSpawn() to return those constants instead
of raw numbers; ensure the constant names are descriptive and used in the return
statements so future balance changes only require editing the constants.
In `@src/core/execution/PlayerExecution.ts`:
- Around line 101-103: Introduce a named constant for the hard-coded tile
threshold used in the PlayerExecution cluster recalculation condition: instead
of comparing this.player.numTilesOwned() < 100 inline, declare a constant (e.g.
minTilesForClusterCalc or TILE_THRESHOLD_FOR_CLUSTER) alongside
ticksPerClusterCalc and use that constant in the condition; update the condition
that references this.lastCalc and this.ticksPerClusterCalc to use the new
constant so the threshold is centralized and easier to tune (reference: class
PlayerExecution, properties ticksPerClusterCalc and lastCalc, and method/usage
player.numTilesOwned()).
In `@src/core/execution/utils/AiAttackBehavior.ts`:
- Around line 850-853: The guard in AiAttackBehavior.donateTroops() that checks
this.game.config().gameConfig().gameType === GameType.Public should be
centralized: add a single shared eligibility method (e.g.,
GameConfig.canDonateTroops() or ExecutionContext.isDonationAllowed()) that
encapsulates the public-game rule (and any future rules), replace the inline
check in AiAttackBehavior.donateTroops() with a call to that new method, and
update all other troop-donation entry points to call the same method so all
donation paths follow the same policy.
In `@src/server/Worker.ts`:
- Around line 618-635: The current cleanup clears the entire wsIpLimiters map
every 10 minutes which resets token budgets for active IPs; update the
implementation to track last access per IP and only remove stale limiters
instead of clearing all: augment the wsIpLimiters Map entries (or create a
parallel Map) to store a timestamp updated inside getWsIpLimiter(ip) whenever a
limiter is retrieved/created, and replace the setInterval(() =>
wsIpLimiters.clear(), ...) task with a sweeper that iterates wsIpLimiters and
deletes entries whose last-access timestamp is older than a configurable stale
threshold (e.g., 10 minutes); keep the RateLimiter creation logic in
getWsIpLimiter and ensure timestamp updates occur on both creation and
retrieval.
In `@tests/core/executions/NoInverseAnnexation.test.ts`:
- Around line 58-60: Remove the redundant assertion that re-checks the same
value just assigned: delete the
expect(largePlayer.numTilesOwned()).toBe(initialLargeTiles) line (the no-op
using initialLargeTiles and largePlayer.numTilesOwned()). Keep the meaningful
assertion about smallPlayer.numTilesOwned() or replace the removed line with a
real behavior check if needed; reference the variables initialLargeTiles,
largePlayer.numTilesOwned(), and smallPlayer.numTilesOwned() to locate and edit
the test.
- Around line 63-70: Replace the hardcoded tick advances with a loop that calls
executeTicks until the expected post-annexation state is observed or a max tick
cap is reached: after calling largePlayer.conquer(game.ref(49, 49)) and
smallPlayer.conquer(game.ref(50, 50)), repeatedly call executeTicks(game, 1) (or
a small step) and check the conditions using largePlayer.numTilesOwned() >
initialLargeTiles and smallPlayer.numTilesOwned() === 0; break when both are
true or throw/assert failure after the cap to avoid flakiness. Use the existing
functions executeTicks, largePlayer.conquer, smallPlayer.conquer,
largePlayer.numTilesOwned, smallPlayer.numTilesOwned and preserve
initialLargeTiles in the check. Ensure the loop enforces a reasonable maxTicks
to prevent infinite loops in CI.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 36f3b237-cf6e-4711-85ad-0bc17b9d2d81
⛔ Files ignored due to path filters (8)
map-generator/assets/maps/niledelta/image.pngis excluded by!**/*.pngmap-generator/assets/maps/straitofhormuz/image.pngis excluded by!**/*.pngresources/maps/niledelta/map.binis excluded by!**/*.binresources/maps/niledelta/map16x.binis excluded by!**/*.binresources/maps/niledelta/map4x.binis excluded by!**/*.binresources/maps/straitofhormuz/map.binis excluded by!**/*.binresources/maps/straitofhormuz/map16x.binis excluded by!**/*.binresources/maps/straitofhormuz/map4x.binis excluded by!**/*.bin
📒 Files selected for processing (25)
index.htmlresources/maps/niledelta/thumbnail.webpresources/maps/straitofhormuz/manifest.jsonresources/maps/straitofhormuz/thumbnail.webpsrc/client/ClientGameRunner.tssrc/client/Main.tssrc/client/components/VideoPromo.tssrc/client/graphics/GameRenderer.tssrc/client/graphics/layers/AttacksDisplay.tssrc/client/graphics/layers/EventsDisplay.tssrc/client/graphics/layers/GameLeftSidebar.tssrc/client/graphics/layers/GameRightSidebar.tssrc/client/graphics/layers/InGameHeaderAd.tssrc/client/graphics/layers/InGamePromo.tssrc/client/graphics/layers/PlayerInfoOverlay.tssrc/client/graphics/layers/ReplayPanel.tssrc/client/graphics/layers/SpawnVideoReward.tssrc/core/configuration/DefaultConfig.tssrc/core/execution/PlayerExecution.tssrc/core/execution/utils/AiAttackBehavior.tssrc/server/ClientMsgRateLimiter.tssrc/server/MapPlaylist.tssrc/server/Worker.tstests/core/executions/NoInverseAnnexation.test.tstests/server/ClientMsgRateLimiter.test.ts
💤 Files with no reviewable changes (3)
- src/client/graphics/layers/SpawnVideoReward.ts
- src/client/graphics/layers/InGameHeaderAd.ts
- src/client/components/VideoPromo.ts
github-project-automation
bot
moved this from Triage
to Final Review
in OpenFront Release Management
github-project-automation
bot
moved this from Final Review
to Complete
in OpenFront Release Management
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description:
Show the "footer_ad" ad type during the spawn phase.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
evan