Revert "counter attack doesn't cancel out attack (#1132)"#1301
Revert "counter attack doesn't cancel out attack (#1132)"#1301
Conversation
This reverts commit a0d17ed.
WalkthroughA new logic block was added to the Changes
Sequence Diagram(s)sequenceDiagram
participant Owner as Owner Player
participant AttackExec as AttackExecution
participant Attacker as Attacker Player
Owner->>AttackExec: init()
AttackExec->>Owner: Get incoming attacks
loop For each incoming attack
AttackExec->>Attacker: Check if attacker matches target
alt Troop counts comparison
AttackExec->>AttackExec: Reduce troops, delete attack(s) as needed
AttackExec->>AttackExec: Mark execution inactive if canceled
end
end
AttackExec->>AttackExec: Continue with attack combination logic
Possibly related PRs
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-project-automation
bot
moved this from Triage
to Complete
in OpenFront Release Management
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/core/execution/AttackExecution.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: 1brucben
PR: openfrontio/OpenFrontIO#977
File: src/core/execution/AttackExecution.ts:123-125
Timestamp: 2025-05-31T18:15:03.445Z
Learning: The removeTroops function in PlayerImpl.ts already prevents negative troop counts by using minInt(this._troops, toInt(troops)) to ensure it never removes more troops than available.
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#784
File: src/core/game/StatsImpl.ts:73-84
Timestamp: 2025-05-18T20:07:40.702Z
Learning: In the StatsImpl class, methods like attackCancel don't need bounds checking because all calls to these functions come from trusted sources that guarantee valid values.
Learnt from: Egraveline
PR: openfrontio/OpenFrontIO#1012
File: src/core/execution/UpgradeStructureExecution.ts:49-54
Timestamp: 2025-06-05T02:34:45.899Z
Learning: In the upgrade system, gold deduction for structure upgrades is handled internally by the `upgradeUnit` method in PlayerImpl, not in the UpgradeStructureExecution class. The UpgradeStructureExecution only needs to check if the player has sufficient gold before calling `upgradeUnit`.
src/core/execution/AttackExecution.ts (2)
Learnt from: 1brucben
PR: openfrontio/OpenFrontIO#977
File: src/core/execution/AttackExecution.ts:123-125
Timestamp: 2025-05-31T18:15:03.445Z
Learning: The removeTroops function in PlayerImpl.ts already prevents negative troop counts by using minInt(this._troops, toInt(troops)) to ensure it never removes more troops than available.
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#784
File: src/core/game/StatsImpl.ts:73-84
Timestamp: 2025-05-18T20:07:40.702Z
Learning: In the StatsImpl class, methods like attackCancel don't need bounds checking because all calls to these functions come from trusted sources that guarantee valid values.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Deploy to openfront.dev
| for (const incoming of this._owner.incomingAttacks()) { | ||
| if (incoming.attacker() === this.target) { | ||
| // Target has opposing attack, cancel them out | ||
| if (incoming.troops() > this.attack.troops()) { | ||
| incoming.setTroops(incoming.troops() - this.attack.troops()); | ||
| this.attack.delete(); | ||
| this.active = false; | ||
| return; | ||
| } else { | ||
| this.attack.setTroops(this.attack.troops() - incoming.troops()); | ||
| incoming.delete(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Check for potential concurrent modification during iteration.
The counter-attack cancellation logic looks correct, but there's a potential issue with modifying the collection while iterating. If incomingAttacks() returns a live collection, calling incoming.delete() during iteration could cause problems.
Consider collecting attacks to delete first, then delete them after iteration:
+ const attacksToDelete: Attack[] = [];
for (const incoming of this._owner.incomingAttacks()) {
if (incoming.attacker() === this.target) {
// Target has opposing attack, cancel them out
if (incoming.troops() > this.attack.troops()) {
incoming.setTroops(incoming.troops() - this.attack.troops());
this.attack.delete();
this.active = false;
return;
} else {
this.attack.setTroops(this.attack.troops() - incoming.troops());
- incoming.delete();
+ attacksToDelete.push(incoming);
}
}
}
+ for (const attack of attacksToDelete) {
+ attack.delete();
+ }Also consider: what happens if multiple incoming attacks exist from the same target? The current logic processes them sequentially, which may not be the intended behavior.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (const incoming of this._owner.incomingAttacks()) { | |
| if (incoming.attacker() === this.target) { | |
| // Target has opposing attack, cancel them out | |
| if (incoming.troops() > this.attack.troops()) { | |
| incoming.setTroops(incoming.troops() - this.attack.troops()); | |
| this.attack.delete(); | |
| this.active = false; | |
| return; | |
| } else { | |
| this.attack.setTroops(this.attack.troops() - incoming.troops()); | |
| incoming.delete(); | |
| } | |
| } | |
| } | |
| // Collect attacks to delete after iteration to avoid concurrent modification | |
| const attacksToDelete: Attack[] = []; | |
| for (const incoming of this._owner.incomingAttacks()) { | |
| if (incoming.attacker() === this.target) { | |
| // Target has opposing attack, cancel them out | |
| if (incoming.troops() > this.attack.troops()) { | |
| incoming.setTroops(incoming.troops() - this.attack.troops()); | |
| this.attack.delete(); | |
| this.active = false; | |
| return; | |
| } else { | |
| this.attack.setTroops(this.attack.troops() - incoming.troops()); | |
| attacksToDelete.push(incoming); | |
| } | |
| } | |
| } | |
| for (const attack of attacksToDelete) { | |
| attack.delete(); | |
| } |
🤖 Prompt for AI Agents
In src/core/execution/AttackExecution.ts around lines 125 to 138, avoid
modifying the incoming attacks collection during iteration to prevent concurrent
modification issues. Refactor the code to first collect attacks to delete or
adjust in a separate list while iterating, then apply deletions and troop
adjustments after the loop completes. Additionally, review the logic to handle
multiple incoming attacks from the same target collectively rather than
sequentially, ensuring the cancellation behavior matches the intended game
mechanics.
1 participant
This reverts commit a0d17ed.
Description:
Causes some strange edge cases.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
evan