Fix: Retreat naval invasions when alliance is accepted (#3502)#3516
Open
AlexBesios wants to merge 3 commits intoopenfrontio:mainfrom
Open
Fix: Retreat naval invasions when alliance is accepted (#3502)#3516AlexBesios wants to merge 3 commits intoopenfrontio:mainfrom
AlexBesios wants to merge 3 commits intoopenfrontio:mainfrom
Conversation
|
|
Contributor
WalkthroughWhen an alliance is accepted, in-flight transport ships targeting the new ally are ordered to retreat; the change adds retreat logic to alliance acceptance, two i18n keys, and tests verifying retreat state and display events. Changes
Sequence Diagram(s)sequenceDiagram
participant PlayerA as Player A
participant PlayerB as Player B
participant AllianceExec as AllianceRequestExecution
participant Ships as TransportShip Units
participant Updates as GameUpdate
PlayerA->>AllianceExec: accept alliance with Player B
AllianceExec->>Ships: scan active transport ships for both players
Ships-->>AllianceExec: return ships with targets
AllianceExec->>Ships: filter ships targeting the allied player
Ships-->>AllianceExec: confirm matching ships
AllianceExec->>Ships: order retreat on each matching ship
Ships-->>AllianceExec: set retreating = true
AllianceExec->>Updates: emit outgoing retreat event (count, name)
AllianceExec->>Updates: emit incoming retreat event (count, name)
Updates-->>PlayerA: display retreat notifications
Updates-->>PlayerB: display retreat notifications
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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.
🧹 Nitpick comments (1)
tests/AllianceAcceptBoatRetreat.test.ts (1)
101-125: Make the display-event assertion stricter.This case should emit both outgoing and incoming retreat messages; checking only “some” can miss partial regressions.
Proposed test assertion update
- expect( - messages.some( - (m) => - m === "events_display.alliance_boats_retreated_outgoing" || - m === "events_display.alliance_boats_retreated_incoming", - ), - ).toBe(true); + expect(messages).toContain( + "events_display.alliance_boats_retreated_outgoing", + ); + expect(messages).toContain( + "events_display.alliance_boats_retreated_incoming", + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/AllianceAcceptBoatRetreat.test.ts` around lines 101 - 125, The current test in AllianceAcceptBoatRetreat.test.ts uses a loose assertion that only checks for the presence of either "events_display.alliance_boats_retreated_outgoing" or "events_display.alliance_boats_retreated_incoming"; tighten it to assert that both messages are emitted. Locate the messages array (derived from updates[GameUpdateType.DisplayEvent]?.map((e) => e.message) ?? []) and replace the .some(...) check with explicit checks that messages.includes("events_display.alliance_boats_retreated_outgoing") and messages.includes("events_display.alliance_boats_retreated_incoming"), asserting both are true so the test fails if one of the two retreat display events is missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/AllianceAcceptBoatRetreat.test.ts`:
- Around line 101-125: The current test in AllianceAcceptBoatRetreat.test.ts
uses a loose assertion that only checks for the presence of either
"events_display.alliance_boats_retreated_outgoing" or
"events_display.alliance_boats_retreated_incoming"; tighten it to assert that
both messages are emitted. Locate the messages array (derived from
updates[GameUpdateType.DisplayEvent]?.map((e) => e.message) ?? []) and replace
the .some(...) check with explicit checks that
messages.includes("events_display.alliance_boats_retreated_outgoing") and
messages.includes("events_display.alliance_boats_retreated_incoming"), asserting
both are true so the test fails if one of the two retreat display events is
missing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ba157898-88a3-48fc-8173-0dbb73acc66c
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
resources/lang/en.jsonsrc/core/execution/alliance/AllianceRequestExecution.tstests/AllianceAcceptBoatRetreat.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/AllianceAcceptBoatRetreat.test.ts`:
- Around line 39-41: The test contains an unbounded loop using
game.inSpawnPhase() and game.executeNextTick() that can hang; add a guarded loop
with a max tick counter (e.g., const MAX_SPAWN_TICKS = 1000 or similar) and
iterate at most that many times calling game.executeNextTick(), and if the
counter is exceeded assert/fail the test (or throw) with a descriptive message
so the test fails fast instead of hanging. Ensure you replace the current while
loop with the bounded loop using MAX_SPAWN_TICKS and check the counter after
each tick to fail when exceeded.
- Around line 87-90: The two alliance requests are queued in the same tick
making tests order-dependent; change to the two-tick pattern used elsewhere:
call game.addExecution(new AllianceRequestExecution(player1, player2.id()));
then call game.executeNextTick(); then call game.addExecution(new
AllianceRequestExecution(player2, player1.id())); then call
game.executeNextTick() so acceptance is deterministic; update the identical
block that appears later (the other AllianceRequestExecution pair) the same way.
🪄 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: 557b122f-f579-45c9-9fdc-e2368c80e60d
📒 Files selected for processing (1)
tests/AllianceAcceptBoatRetreat.test.ts
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/AllianceAcceptBoatRetreat.test.ts (2)
121-127: Strengthen the display-message assertion to verify both keys.The current check passes if only one key is emitted. This can miss regressions where one side’s message is dropped.
Suggested assertion update
- expect( - messages.some( - (m) => - m === "events_display.alliance_boats_retreated_outgoing" || - m === "events_display.alliance_boats_retreated_incoming", - ), - ).toBe(true); + expect(messages).toContain( + "events_display.alliance_boats_retreated_outgoing", + ); + expect(messages).toContain( + "events_display.alliance_boats_retreated_incoming", + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/AllianceAcceptBoatRetreat.test.ts` around lines 121 - 127, The test currently only asserts that at least one of the two display-message keys is present, which can miss regressions; update the assertion in AllianceAcceptBoatRetreat.test (the block that checks the messages array/variable) to assert that both "events_display.alliance_boats_retreated_outgoing" and "events_display.alliance_boats_retreated_incoming" are present (e.g., check messages includes/contains both keys or assert messages.some(...) for each key separately) so the test fails if either key is missing.
59-63: Consider extracting alliance-accept flow into a tiny helper.This sequence is repeated four times; a helper would reduce noise and make test intent clearer.
Refactor sketch
+function acceptAlliance(a: Player, b: Player): void { + game.addExecution(new AllianceRequestExecution(a, b.id())); + game.executeNextTick(); // creates request + game.addExecution(new AllianceRequestExecution(b, a.id())); + game.executeNextTick(); // counter-request auto-accepts +} ... - game.addExecution(new AllianceRequestExecution(player1, player2.id())); - game.executeNextTick(); // creates request - game.addExecution(new AllianceRequestExecution(player2, player1.id())); - game.executeNextTick(); // counter-request auto-accepts + acceptAlliance(player1, player2);Also applies to: 90-94, 111-114, 141-144
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/AllianceAcceptBoatRetreat.test.ts` around lines 59 - 63, Extract the repeated two-step alliance request/auto-accept sequence into a helper like sendAndAutoAcceptAlliance(initiator, counterparty): it should call game.addExecution(new AllianceRequestExecution(initiator, counterparty.id())), then game.executeNextTick(), then game.addExecution(new AllianceRequestExecution(counterparty, initiator.id())), then game.executeNextTick(); replace each repeated block (the occurrences around AllianceRequestExecution + game.executeNextTick) with calls to this helper to make the tests clearer and remove duplication.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/AllianceAcceptBoatRetreat.test.ts`:
- Around line 121-127: The test currently only asserts that at least one of the
two display-message keys is present, which can miss regressions; update the
assertion in AllianceAcceptBoatRetreat.test (the block that checks the messages
array/variable) to assert that both
"events_display.alliance_boats_retreated_outgoing" and
"events_display.alliance_boats_retreated_incoming" are present (e.g., check
messages includes/contains both keys or assert messages.some(...) for each key
separately) so the test fails if either key is missing.
- Around line 59-63: Extract the repeated two-step alliance request/auto-accept
sequence into a helper like sendAndAutoAcceptAlliance(initiator, counterparty):
it should call game.addExecution(new AllianceRequestExecution(initiator,
counterparty.id())), then game.executeNextTick(), then game.addExecution(new
AllianceRequestExecution(counterparty, initiator.id())), then
game.executeNextTick(); replace each repeated block (the occurrences around
AllianceRequestExecution + game.executeNextTick) with calls to this helper to
make the tests clearer and remove duplication.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 45a3fe4d-ab2c-46aa-8ead-d46a6630ba86
📒 Files selected for processing (1)
tests/AllianceAcceptBoatRetreat.test.ts
github-project-automation
bot
moved this from Development
to Final Review
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
2 participants
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.
If this PR fixes an issue, link it below. If not, delete these two lines.
Resolves #3502
Description:
When an alliance is accepted, any naval invasions between the two newly allied players now automatically retreat. This prevents the exploit where a player could launch a naval invasion, request an alliance, and gain a risk-free foothold if the target accepts. The retreating troops are then added back
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
deathllotus