Fix/double tap full send#4398
Conversation
… any client, and merges their troop consumption instead of adding them. Required to convey separately troopRatio and troopCount for these Intents
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAttack and boat intents now send ratio and troop-count fields separately. Client emission, transport serialization, execution allocation, and tests were updated to use the new intent shape. ChangesAttack ratio payload and execution flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/execution/ExecutionManager.ts (1)
95-111: 🩺 Stability & Availability | 🟡 MinorDefault
troopRatioFactorto1instead of force-unwrapping.The parameter is optional but force-unwrapped at lines 108 and 129 without checking. If
createExecis called directly with an attack or boat intent and notroopRatioFactor, the multiplication producesNaN. Defaulting to1(no scaling) is safer and clearer:Suggested fix
- createExec(intent: StampedIntent, troopRatioFactor?: number): Execution { + createExec(intent: StampedIntent, troopRatioFactor = 1): Execution { const player = this.mg.playerByClientID(intent.clientID); if (!player) { console.warn(`player with clientID ${intent.clientID} not found`); @@ -108,7 +108,7 @@ case "attack": { return new AttackExecution( Math.floor( Math.min( - troopRatioFactor! * intent.troopRatio * intent.troopCount, + troopRatioFactor * intent.troopRatio * intent.troopCount, intent.maxTroopSent ?? intent.troopCount, ), ), @@ -129,7 +129,7 @@ case "boat": return new TransportShipExecution( player, intent.dst, - Math.floor(troopRatioFactor! * intent.troopRatio * intent.troopCount), + Math.floor(troopRatioFactor * intent.troopRatio * intent.troopCount), );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/execution/ExecutionManager.ts` around lines 95 - 111, The parameter troopRatioFactor is optional but is being force-unwrapped with the ! operator in the multiplication operation at line 108 (and line 129 based on the comment). When createExec is called without providing troopRatioFactor, it will be undefined and the multiplication will result in NaN. Change the function signature of createExec to default troopRatioFactor to 1 instead of making it optional with force-unwrapping, either by using a default parameter value in the function declaration or by assigning a default value early in the function body. This ensures safe behavior when the parameter is not provided while maintaining the same calculation logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/execution/ExecutionManager.ts`:
- Around line 45-50: The computeRatio method in ExecutionManager is performing
floating-point division which violates deterministic simulation requirements for
src/core. Replace the floating-point arithmetic in the computeRatio method with
fixed-point integer arithmetic using basis points (where 10000 represents 100
percent). Additionally, audit and update the troopRatio aggregation and
troop-send computation logic at lines 106-111 and 126-130 to also use
fixed-point integer arithmetic instead of floating-point operations to ensure
deterministic behavior across all combat resolution calculations.
- Around line 45-73: The computeRatio method calculates a scaling factor based
on raw troopRatio values without considering the maxTroopSent cap that is
applied later to each intent. This causes incorrect under-sending when mixing
capped and uncapped attacks from the same client in one turn. Fix this by
modifying the calculation of totalRatioUsage_perClientID in the switch case for
"boat" and "attack" intent types to account for the effective troops that will
actually be sent after the maxTroopSent cap is applied, rather than using only
the raw intent.troopRatio value. This ensures the computed ratio properly
reflects the actual constraints that will limit troop allocation.
In `@src/core/Schemas.ts`:
- Around line 364-365: The maxTroopSent cap is being applied too late in the
process, after ratio aggregation, which allows capped retaliations to
incorrectly reduce other same-tick attacks. Apply the retaliation cap earlier by
computing the effective capped ratio using min(troopRatio, maxTroopSent /
troopCount) during the ratio aggregation phase when building per-client merge
inputs, rather than after the downstream same-tick ratio factor is computed.
Additionally, add a core test case that verifies the behavior when a capped
retaliation is mixed with another same-tick attack to ensure the cap is applied
correctly without affecting other attacks. Remember to follow the coding
guideline that all src/core/ changes must include tests.
- Line 363: The troopRatio field in Schemas.ts currently uses a floating-point
validation with z.number().gt(0).max(1), which causes precision issues when
multiplied in the core execution logic. Replace this with a fixed-point integer
validation using either basis points (0-10000) or ppm (0-1000000)
representation. Update the troopRatio validation to use
z.number().int().min(1).max(10000) for basis points or
z.number().int().min(1).max(1000000) for ppm, then ensure the core execution
code (referenced in the comment at lines 66-67, 71-72, 108-109, 129) treats
troopRatio as an integer and performs division only at the client edge when
converting back to decimal form for display purposes.
- Around line 362-365: The troopCount and maxTroopSent fields in this schema
currently accept decimal numbers (like 12.5), but troop counts must be integers.
Add the .int() constraint to the troopCount field (which has .nonnegative()) and
to the maxTroopSent field (which has .nonnegative().optional()) to tighten the
wire contract. Additionally, apply the same .int() constraint to the troops
field in DonateTroopIntentSchema at line 426 for consistency.
In `@tests/core/execution/ExecutionManager.test.ts`:
- Around line 17-28: The test has nested beforeEach hooks which causes the outer
hook to execute first (before game is initialized) and the inner hook to execute
later. This means the Executor constructor is called with an undefined game
parameter. Refactor this by consolidating into a single async beforeEach hook
where setup() is called first to initialize the game variable, then the Executor
is created with that initialized game. Additionally, remove the manual mocking
of executor.mg (the stub assignment in the test method itself) and instead rely
on the real game simulation by passing the actual initialized game object to the
Executor constructor, following the pattern used in other test files that call
setup() and then use the returned game instance directly.
---
Outside diff comments:
In `@src/core/execution/ExecutionManager.ts`:
- Around line 95-111: The parameter troopRatioFactor is optional but is being
force-unwrapped with the ! operator in the multiplication operation at line 108
(and line 129 based on the comment). When createExec is called without providing
troopRatioFactor, it will be undefined and the multiplication will result in
NaN. Change the function signature of createExec to default troopRatioFactor to
1 instead of making it optional with force-unwrapping, either by using a default
parameter value in the function declaration or by assigning a default value
early in the function body. This ensures safe behavior when the parameter is not
provided while maintaining the same calculation logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 691359b3-993e-47af-910c-31f7b98670e2
📒 Files selected for processing (8)
src/client/ClientGameRunner.tssrc/client/Transport.tssrc/client/hud/layers/AttacksDisplay.tssrc/client/hud/layers/PlayerActionHandler.tssrc/core/Schemas.tssrc/core/execution/ExecutionManager.tstests/Attack.test.tstests/core/execution/ExecutionManager.test.ts
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
…mputation (for deterministic behavior) and removing the maxTroop value for retalition, because capping the troopRatio is more versatile during aggregation
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)
src/client/ClientGameRunner.ts (1)
1240-1243: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSend boat ratio as an integer percent.
Line 1242 still sends the raw UI ratio, like
0.6, while the new boat schema and execution math expect1..100. This can make boat intents invalid or send the wrong troop count.Proposed fix
new SendBoatAttackIntentEvent( tile, - this.renderer.uiState.attackRatio, + Math.floor(100 * this.renderer.uiState.attackRatio), this.myPlayer.troops(), ),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/client/ClientGameRunner.ts` around lines 1240 - 1243, The boat attack intent is still passing the raw UI ratio from ClientGameRunner instead of an integer percent expected by the new schema and execution math. Update the SendBoatAttackIntentEvent call in ClientGameRunner to convert this.renderer.uiState.attackRatio into a 1..100 integer before sending it, and make sure any related boat intent construction or handling uses the same percent-based value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/client/ClientGameRunner.ts`:
- Around line 1240-1243: The boat attack intent is still passing the raw UI
ratio from ClientGameRunner instead of an integer percent expected by the new
schema and execution math. Update the SendBoatAttackIntentEvent call in
ClientGameRunner to convert this.renderer.uiState.attackRatio into a 1..100
integer before sending it, and make sure any related boat intent construction or
handling uses the same percent-based value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 73b8f087-814a-4353-827c-1618d87226a3
📒 Files selected for processing (7)
src/client/ClientGameRunner.tssrc/client/Transport.tssrc/client/hud/layers/AttacksDisplay.tssrc/client/hud/layers/PlayerActionHandler.tssrc/core/Schemas.tssrc/core/execution/ExecutionManager.tstests/core/execution/ExecutionManager.test.ts
💤 Files with no reviewable changes (1)
- src/client/Transport.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/client/hud/layers/PlayerActionHandler.ts
- tests/core/execution/ExecutionManager.test.ts
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/core/execution/ExecutionManager.test.ts (1)
17-26: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not stub
executor.mgorcomputeRatioin this core test.After
setup()creates a real game, Lines 24-26 and 98 replace the core objects you want to validate. That makes this test pass on a mocked path instead of exercising the real simulation and execution wiring. Keep the realGame/player fromsetup()and assert on observable troop counts from the produced executions. Based on learnings, tests undertests/**/*.test.tsshould “use thesetup()helper fromtests/util/Setup.tsto create test game instances and exercise core simulation directly.”Also applies to: 97-99
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/core/execution/ExecutionManager.test.ts` around lines 17 - 26, This core test is stubbing executor.mg and computeRatio, which bypasses the real simulation path and makes the assertions validate mocked behavior instead of ExecutionManager/Executor wiring. Remove those stubs in createExecs merges attack-ratio-based intents from same client ID (and the similar block around the later lines), keep the real Game/player returned by setup(), and assert against observable troop counts or execution results produced by the actual createExecs flow.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/client/hud/layers/AttacksDisplay.ts`:
- Around line 203-218: The retaliation payload is being rounded to an integer
percentage in AttacksDisplay before it reaches core, which can turn a valid
1-troop reply into 0. Update the SendAttackIntentEvent path so retaliation flows
preserve the exact capped attack amount/ratio instead of flooring it into a
percent, and apply the same fix in ClientGameRunner where the attack intent is
constructed. Keep the full precision needed by core and only convert at the
final execution point if required.
---
Duplicate comments:
In `@tests/core/execution/ExecutionManager.test.ts`:
- Around line 17-26: This core test is stubbing executor.mg and computeRatio,
which bypasses the real simulation path and makes the assertions validate mocked
behavior instead of ExecutionManager/Executor wiring. Remove those stubs in
createExecs merges attack-ratio-based intents from same client ID (and the
similar block around the later lines), keep the real Game/player returned by
setup(), and assert against observable troop counts or execution results
produced by the actual createExecs flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0e197039-354f-46d5-a797-12527c193dda
📒 Files selected for processing (8)
src/client/ClientGameRunner.tssrc/client/Transport.tssrc/client/hud/layers/AttacksDisplay.tssrc/client/hud/layers/PlayerActionHandler.tssrc/core/Schemas.tssrc/core/execution/ExecutionManager.tstests/Attack.test.tstests/core/execution/ExecutionManager.test.ts
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/core/execution/ExecutionManager.test.ts (1)
23-26: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftExercise the real game path in this core test.
setup()creates a real game, but Line 24 replacesexecutor.mgwith a stub and later assertions read private fields before execution init/tick. This can pass while real troop clamping/removal behaves differently. Prefer real players/client IDs fromsetup()and assert troop deltas after executing the created executions. As per coding guidelines, tests should usesetup()and exercise core simulation directly.Also applies to: 107-131
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/core/execution/ExecutionManager.test.ts` around lines 23 - 26, The test is stubbing executor.mg and reading private state too early, which bypasses the real simulation path. Update ExecutionManager.test.ts to use the real game/player/client IDs returned by setup() instead of mocking playerByClientID, then run the created executions through the normal init/tick flow and assert the resulting troop deltas after execution. Keep the checks focused on observable outcomes from the core execution path rather than private fields.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/execution/ExecutionManager.ts`:
- Around line 45-53: The ratio merging in ExecutionManager.computeRatio can
overflow into Infinity/NaN when many same-tick intents are combined, so update
this path to use integer-safe arithmetic or enforce a hard cap on per-client
ratio intents before merging. Keep the fix localized to computeRatio and the
same-tick intent aggregation logic that feeds it, and make sure attack/boat
troop counts remain finite even at high intent counts.
---
Nitpick comments:
In `@tests/core/execution/ExecutionManager.test.ts`:
- Around line 23-26: The test is stubbing executor.mg and reading private state
too early, which bypasses the real simulation path. Update
ExecutionManager.test.ts to use the real game/player/client IDs returned by
setup() instead of mocking playerByClientID, then run the created executions
through the normal init/tick flow and assert the resulting troop deltas after
execution. Keep the checks focused on observable outcomes from the core
execution path rather than private fields.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 97f43854-9440-4b1a-92d1-034aa71417bc
📒 Files selected for processing (8)
src/client/ClientGameRunner.tssrc/client/Transport.tssrc/client/hud/layers/AttacksDisplay.tssrc/client/hud/layers/PlayerActionHandler.tssrc/core/Schemas.tssrc/core/execution/ExecutionManager.tstests/Attack.test.tstests/core/execution/ExecutionManager.test.ts
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)
src/client/ClientGameRunner.ts (1)
1182-1198: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGuard against a divide-by-zero that makes the ratio
NaN.
mostRecentAttack.troops / this.myPlayer.troops()divides by the player's troop count. Whentroops()is0, this isInfinity(whenmostRecentAttack.troops > 0,Math.minthen safely picksattackRatio), but if both are0you get0 / 0 = NaN.Math.min(attackRatio, NaN)isNaN, and it flows throughMath.max(NaN, 1) → NaNandMath.ceil(NaN) → NaN, so aNaNpercentage is sent. A short guard keeps the value clean.🛡️ Proposed guard
+ const myTroops = this.myPlayer.troops(); + const fullDefenseRatio = + myTroops > 0 ? mostRecentAttack.troops / myTroops : 1; this.eventBus.emit( new SendAttackIntentEvent( attacker.id(), Math.ceil( // Ensures the attackRatio is between 1% and the required percentage to defend fully. Math.max( 100 * Math.min( this.renderer.uiState.attackRatio, - mostRecentAttack.troops / this.myPlayer.troops(), + fullDefenseRatio, ), 1, ), ), - this.myPlayer.troops(), + myTroops, ), );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/client/ClientGameRunner.ts` around lines 1182 - 1198, The attack percentage calculation in ClientGameRunner’s SendAttackIntentEvent path can become NaN when myPlayer.troops() is 0 and mostRecentAttack.troops is also 0. Add a guard around the ratio computation so the division result is never used when it would be NaN, and ensure the value passed into Math.min/Math.max/Math.ceil always falls back to a valid minimum percentage; keep the fix localized near the existing attackRatio logic in ClientGameRunner.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/client/ClientGameRunner.ts`:
- Around line 1182-1198: The attack percentage calculation in ClientGameRunner’s
SendAttackIntentEvent path can become NaN when myPlayer.troops() is 0 and
mostRecentAttack.troops is also 0. Add a guard around the ratio computation so
the division result is never used when it would be NaN, and ensure the value
passed into Math.min/Math.max/Math.ceil always falls back to a valid minimum
percentage; keep the fix localized near the existing attackRatio logic in
ClientGameRunner.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bbcf581e-7a03-4e6b-973e-f1e568db7082
📒 Files selected for processing (3)
src/client/ClientGameRunner.tssrc/core/Schemas.tstests/Attack.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/core/Schemas.ts
- tests/Attack.test.ts
evanpelle
left a comment
evanpelle
left a comment
There was a problem hiding this comment.
Instead of the merge math, could we just send troopRatio and resolve floor(ratio * owner.troops()) inside AttackExecution.init (not createExecs) — since same-tick executions already init sequentially after each troop removal, two 50% attacks compose to 75% for free, with no per-client maps or float math in core?
|
@evanpelle Solving To be fair, I do not know if this sequencing is guaranteed in some way or not. If we can guarantee each attack-intent is executed in the same order amongst all servers/clients, then your proposal works and is more elegant code-wise (but less elegant mathematically because the sequence influences the gameplay AND could differ from the sequence the player clicked). Let's compare all versions with a simple case. player A has 100k troops and attacks player B with 50% Att-ratio and, during the same tick, retaliate player C's attack which has 25k inbound troops. Current behavior: The game sends one 50k attack to player B and one 25k attack to retaliate player C (retaliation is complete). Total troops used: 75k. PR's behavior: The game consider it as two attacks with (50%, 100k) for player B and (25%, 100k) for retaliation against C. Your proposition has the same intents attacks with (50%) for player B and (25%) for retaliation against C but sends them directly to
Note on extreme-ness of the example: Conclusion: I think the merge-math is the clear winner. I still need to include some comments of coderabbitai though. Do you have a specific problem with the merge-computation in core? I think that it is using only integers, flooring the numbers only at two specific points (computing the correction factor and for the total troops of each attack) where both are ensured to be consistent amongst servers. |
…ontIO into fix/double-tap-full-send
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/core/execution/ExecutionManager.ts (1)
116-118: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSmall duplication in the troop-count formula.
The
Math.floor((troopRatioFactor * intent.troopRatio * intent.troopCount) / 10000)expression is repeated for"attack"and"boat". A tiny private helper (e.g.applyRatio(factor, intent)) would remove the duplication.♻️ Optional refactor
+ private applyRatio( + troopRatioFactor: number, + intent: { troopRatio: number; troopCount: number }, + ): number { + return Math.floor( + (troopRatioFactor * intent.troopRatio * intent.troopCount) / 10000, + ); + } + createExec(intent: StampedIntent, troopRatioFactor = 100): Execution { ... case "attack": { return new AttackExecution( - Math.floor( - (troopRatioFactor * intent.troopRatio * intent.troopCount) / 10000, - ), + this.applyRatio(troopRatioFactor, intent), player, intent.targetID, null, ); } ... case "boat": return new TransportShipExecution( player, intent.dst, - Math.floor( - (troopRatioFactor * intent.troopRatio * intent.troopCount) / 10000, - ), + this.applyRatio(troopRatioFactor, intent), );Also applies to: 133-139
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/execution/ExecutionManager.ts` around lines 116 - 118, The troop-count calculation is duplicated in ExecutionManager, where the same Math.floor((troopRatioFactor * intent.troopRatio * intent.troopCount) / 10000) logic is used for both "attack" and "boat". Extract that shared formula into a small private helper in ExecutionManager, such as applyRatio(factor, intent), and have both branches call it so the ratio logic lives in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/client/ClientGameRunner.ts`:
- Around line 984-990: Clamp the attack percentage before emitting attack
intents in ClientGameRunner so it never becomes 0 and violates
AttackIntentSchema/BoatAttackIntentSchema. Update inputEvent,
doGroundAttackUnderCursor, and sendBoatAttackIntent to use the same Math.max(1,
Math.floor(100 * this.renderer.uiState.attackRatio)) guard already used by
doRetaliateAttackMostRecent, and keep the event payloads otherwise unchanged.
---
Nitpick comments:
In `@src/core/execution/ExecutionManager.ts`:
- Around line 116-118: The troop-count calculation is duplicated in
ExecutionManager, where the same Math.floor((troopRatioFactor *
intent.troopRatio * intent.troopCount) / 10000) logic is used for both "attack"
and "boat". Extract that shared formula into a small private helper in
ExecutionManager, such as applyRatio(factor, intent), and have both branches
call it so the ratio logic lives in one place.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 808732ae-b694-4696-aa45-4149f099ead8
📒 Files selected for processing (8)
src/client/ClientGameRunner.tssrc/client/Transport.tssrc/client/hud/layers/AttacksDisplay.tssrc/client/hud/layers/PlayerActionHandler.tssrc/core/Schemas.tssrc/core/execution/ExecutionManager.tstests/Attack.test.tstests/core/execution/ExecutionManager.test.ts
|
@coderabbitai full review |
✅ Action performedFull review finished. |
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)
src/client/hud/layers/PlayerActionHandler.ts (1)
26-44: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClamp
troopRatioto a minimum of 1 here.Math.floor(100 * this.uiState.attackRatio)can become0, which does not satisfytroopRatio: z.number().int().gt(0).max(100)and drops the intent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/client/hud/layers/PlayerActionHandler.ts` around lines 26 - 44, The attack intent handlers in PlayerActionHandler currently compute troop ratio with Math.floor(100 * this.uiState.attackRatio), which can produce 0 and fail the SendAttackIntentEvent/SendBoatAttackIntentEvent validation. Update handleAttack and handleBoatAttack to clamp the computed ratio to a minimum of 1 before emitting the event, while keeping the maximum within 100.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/client/hud/layers/PlayerActionHandler.ts`:
- Around line 26-44: The attack intent handlers in PlayerActionHandler currently
compute troop ratio with Math.floor(100 * this.uiState.attackRatio), which can
produce 0 and fail the SendAttackIntentEvent/SendBoatAttackIntentEvent
validation. Update handleAttack and handleBoatAttack to clamp the computed ratio
to a minimum of 1 before emitting the event, while keeping the maximum within
100.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a24489b3-f39c-444e-be54-bcad653a61b8
📒 Files selected for processing (8)
src/client/ClientGameRunner.tssrc/client/Transport.tssrc/client/hud/layers/AttacksDisplay.tssrc/client/hud/layers/PlayerActionHandler.tssrc/core/Schemas.tssrc/core/execution/ExecutionManager.tstests/Attack.test.tstests/core/execution/ExecutionManager.test.ts
|
@coderabbitai full review |
✅ Action performedFull review finished. |
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 (1)
src/client/hud/layers/PlayerActionHandler.ts (1)
26-44: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winCritical:
Math.minshould beMath.max— attacks always send only 1% troops.
Math.min(Math.floor(100 * this.uiState.attackRatio), 1)caps the ratio at 1 whenever the floored value is ≥ 1 (true for virtually anyattackRatio), so this always resolves to1. Every equivalent call site elsewhere in this PR (ClientGameRunner.ts,AttacksDisplay.ts) correctly usesMath.max(..., 1)to guarantee a minimum of 1%, not a maximum. As written,handleAttackandhandleBoatAttackwill send only 1% of troops no matter what ratio the player selects.🐛 Proposed fix
handleAttack(player: PlayerView, targetId: string | null) { this.eventBus.emit( new SendAttackIntentEvent( targetId, - Math.min(Math.floor(100 * this.uiState.attackRatio), 1), + Math.max(Math.floor(100 * this.uiState.attackRatio), 1), player.troops(), ), ); } handleBoatAttack(player: PlayerView, targetTile: TileRef) { this.eventBus.emit( new SendBoatAttackIntentEvent( targetTile, - Math.min(Math.floor(100 * this.uiState.attackRatio), 1), + Math.max(Math.floor(100 * this.uiState.attackRatio), 1), player.troops(), ), ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/client/hud/layers/PlayerActionHandler.ts` around lines 26 - 44, The attack ratio calculation in PlayerActionHandler is using Math.min where it should enforce a minimum, which makes both handleAttack and handleBoatAttack always send only 1% of troops. Update the SendAttackIntentEvent and SendBoatAttackIntentEvent argument to use the same Math.max(Math.floor(100 * this.uiState.attackRatio), 1) pattern used elsewhere in ClientGameRunner and AttacksDisplay so the selected ratio is preserved while still clamping to at least 1%.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/client/hud/layers/AttacksDisplay.ts`:
- Around line 203-219: The retaliate attack intent in AttacksDisplay is
computing attackRatio from attack.troops / myPlayer.troops() without a zero
guard, which can produce NaN and fail SendAttackIntentEvent validation. Update
the ratio calculation in the SendAttackIntentEvent path to mirror
ClientGameRunner’s doRetaliateAttackMostRecent by handling myPlayer.troops() <=
0 safely and falling back to 1 (or another valid minimum) before applying
Math.min/Math.max, so troopRatio always remains valid.
---
Outside diff comments:
In `@src/client/hud/layers/PlayerActionHandler.ts`:
- Around line 26-44: The attack ratio calculation in PlayerActionHandler is
using Math.min where it should enforce a minimum, which makes both handleAttack
and handleBoatAttack always send only 1% of troops. Update the
SendAttackIntentEvent and SendBoatAttackIntentEvent argument to use the same
Math.max(Math.floor(100 * this.uiState.attackRatio), 1) pattern used elsewhere
in ClientGameRunner and AttacksDisplay so the selected ratio is preserved while
still clamping to at least 1%.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0ebcfb6d-7901-493b-8b94-d2e58dc552c8
📒 Files selected for processing (8)
src/client/ClientGameRunner.tssrc/client/Transport.tssrc/client/hud/layers/AttacksDisplay.tssrc/client/hud/layers/PlayerActionHandler.tssrc/core/Schemas.tssrc/core/execution/ExecutionManager.tstests/Attack.test.tstests/core/execution/ExecutionManager.test.ts
|
@evanpelle I think that this PR is ready to merge. CodeRabbitAI finally agrees. Please check my answer to your previous suggestion and either continue the conversation or merge the fix! |
3 participants
Add approved & assigned issue number here:
Resolves #4237
Improved version of #4397 that got automatically closed because I didn't respect the issue-resolution formatting and because I could not add commits to the PR anymore.
Description:
The PR changes the Intent Events structure using the attack-ratio to compute sent troop such that it do send to the Execution Manager both the attack ratio and the total troopCount to ratio from.
Then, the Execution Manager collects all those numbers for each ClientID sending many of those intents events in the same tick, and adjust the computation such that the total sent number of troop is equal (up to roundings) to the total troops that would be sent if the attacks where computed one after the other (the order is not important because of math).
More precisely, the "Remaining troops" after attacks of ratio 0.6, 0.3 and 0.2 is the multiplication of their non-attacks ratio, 0.40.70.8 = 0.224. The actual attack ratio for each of these same-tick attacks is then adapted accordingly BUT in a way that do not depend at all from their order or size, because the reduction is applied uniformingly.
Also, this PR adds a new test that tests various situations and compares them with the mocked-up still buggy version.
Please complete the following:
I also tested that the game is yes fixed by forcing the radial-menu attack to always send two attacks (easy way to reproduce the bug) and confirmed it is fixed.
Please put your Discord username so you can be contacted if a bug or regression is found:
Katokoda