From 5a065d71c5637d14335bd6499ebde00928d66616 Mon Sep 17 00:00:00 2001 From: Achim Marius <67611764+plazmaezio@users.noreply.github.com> Date: Sun, 28 Dec 2025 23:35:05 +0200 Subject: [PATCH] Fix alliance renewal popup not being removed when alliance is broken (#2722) Resolves #2464 ## Description This PR fixes a bug where the alliance renewal popup remained visible after an alliance was broken or betrayed. The issue occurred because renewal UI events were tied to player identifiers instead of the unique allianceId. When a player had multiple alliances, breaking one alliance did not correctly remove the associated renewal popup. This change ensures that renewal popups are correctly removed only for the specific alliance that was broken. ### What was wrong Alliance renewal UI events were previously associated implicitly with players. This caused incorrect behavior when a player had **multiple alliances**, because **players are not unique identifiers of an alliance**. As a result, breaking one alliance could leave stale renewal popups visible. ### What was changed - Alliance break logic now relies on **allianceId**, not player IDs - Renewal popups are removed **only for the specific broken alliance** - Alliances involving the same player but different allianceIds are unaffected - Added tests to ensure this bug cannot reappear ### Result - Renewal popup disappears immediately when an alliance is betrayed/broken - No unintended removal of other alliance renewal popups - Correct behavior even when a player is involved in multiple alliances ## Please complete the following: - [x] I have added screenshots for all UI updates - [x] I process any text displayed to the user through translateText() and I've added it to the en.json file - [x] I have added relevant tests to the test directory - [x] I confirm I have thoroughly tested these changes and take full responsibility for any bugs introduced ## Please put your Discord username so you can be contacted if a bug or regression is found: assessin. --- src/client/graphics/layers/EventsDisplay.ts | 15 ++ src/core/game/GameImpl.ts | 13 +- src/core/game/GameUpdates.ts | 1 + src/core/game/PlayerImpl.ts | 2 +- .../layers/EventDisplayAlliance.test.ts | 143 ++++++++++++++++++ 5 files changed, 164 insertions(+), 10 deletions(-) create mode 100644 tests/client/graphics/layers/EventDisplayAlliance.test.ts diff --git a/src/client/graphics/layers/EventsDisplay.ts b/src/client/graphics/layers/EventsDisplay.ts index 826a3948d..abdf29887 100644 --- a/src/client/graphics/layers/EventsDisplay.ts +++ b/src/client/graphics/layers/EventsDisplay.ts @@ -70,6 +70,7 @@ interface GameEvent { focusID?: number; unitView?: UnitView; shouldDelete?: (game: GameView) => boolean; + allianceID?: number; } @customElement("events-display") @@ -336,6 +337,7 @@ export class EventsDisplay extends LitElement implements Layer { highlight: true, createdAt: this.game.ticks(), focusID: other.smallID(), + allianceID: alliance.id, }); } } @@ -361,6 +363,16 @@ export class EventsDisplay extends LitElement implements Layer { renderLayer(): void {} + private removeAllianceRenewalEvents(allianceID: number) { + this.events = this.events.filter( + (event) => + !( + event.type === MessageType.RENEW_ALLIANCE && + event.allianceID === allianceID + ), + ); + } + onDisplayMessageEvent(event: DisplayMessageUpdate) { const myPlayer = this.game.myPlayer(); if ( @@ -552,6 +564,9 @@ export class EventsDisplay extends LitElement implements Layer { const myPlayer = this.game.myPlayer(); if (!myPlayer) return; + this.removeAllianceRenewalEvents(update.allianceID); + this.requestUpdate(); + const betrayed = this.game.playerBySmallID(update.betrayedID) as PlayerView; const traitor = this.game.playerBySmallID(update.traitorID) as PlayerView; diff --git a/src/core/game/GameImpl.ts b/src/core/game/GameImpl.ts index c575e7c71..ee0a7783a 100644 --- a/src/core/game/GameImpl.ts +++ b/src/core/game/GameImpl.ts @@ -632,7 +632,7 @@ export class GameImpl implements Game { }); } - public breakAlliance(breaker: Player, alliance: Alliance) { + public breakAlliance(breaker: Player, alliance: MutableAlliance) { let other: Player; if (alliance.requestor() === breaker) { other = alliance.recipient(); @@ -648,18 +648,13 @@ export class GameImpl implements Game { breaker.markTraitor(); } - const breakerSet = new Set(breaker.alliances()); - const alliances = other.alliances().filter((a) => breakerSet.has(a)); - if (alliances.length !== 1) { - throw new Error( - `must have exactly one alliance, have ${alliances.length}`, - ); - } - this.alliances_ = this.alliances_.filter((a) => a !== alliances[0]); + this.alliances_ = this.alliances_.filter((a) => a !== alliance); + this.addUpdate({ type: GameUpdateType.BrokeAlliance, traitorID: breaker.smallID(), betrayedID: other.smallID(), + allianceID: alliance.id(), }); } diff --git a/src/core/game/GameUpdates.ts b/src/core/game/GameUpdates.ts index 6cba024bc..a8c6c53f6 100644 --- a/src/core/game/GameUpdates.ts +++ b/src/core/game/GameUpdates.ts @@ -202,6 +202,7 @@ export interface BrokeAllianceUpdate { type: GameUpdateType.BrokeAlliance; traitorID: number; betrayedID: number; + allianceID: number; } export interface AllianceExpiredUpdate { diff --git a/src/core/game/PlayerImpl.ts b/src/core/game/PlayerImpl.ts index e771012ec..4ac72fd74 100644 --- a/src/core/game/PlayerImpl.ts +++ b/src/core/game/PlayerImpl.ts @@ -436,7 +436,7 @@ export class PlayerImpl implements Player { return delta >= this.mg.config().allianceRequestCooldown(); } - breakAlliance(alliance: Alliance): void { + breakAlliance(alliance: MutableAlliance): void { this.mg.breakAlliance(this, alliance); } diff --git a/tests/client/graphics/layers/EventDisplayAlliance.test.ts b/tests/client/graphics/layers/EventDisplayAlliance.test.ts new file mode 100644 index 000000000..d01569ef7 --- /dev/null +++ b/tests/client/graphics/layers/EventDisplayAlliance.test.ts @@ -0,0 +1,143 @@ +jest.mock("lit", () => ({ + html: () => {}, + LitElement: class {}, +})); + +jest.mock("lit/decorators.js", () => ({ + customElement: () => (clazz: any) => clazz, + query: () => () => {}, + state: () => () => {}, + property: () => () => {}, +})); + +jest.mock("lit/directive.js", () => ({ + DirectiveResult: class {}, +})); + +jest.mock("lit/directives/unsafe-html.js", () => ({ + unsafeHTML: () => {}, + UnsafeHTMLDirective: class {}, +})); + +import { EventsDisplay } from "../../../../src/client/graphics/layers/EventsDisplay"; +import { MessageType } from "../../../../src/core/game/Game"; + +describe("EventsDisplay - alliance renewal cleanup (allianceID based)", () => { + function makeRenewal( + allianceID: number, + focusID: number, + description = "Alliance about to expire", + ) { + return { + description, + type: MessageType.RENEW_ALLIANCE, + allianceID, + focusID, + createdAt: 0, + }; + } + + test("removes ONLY renewal events for the broken alliance", () => { + const display = new EventsDisplay(); + + const allianceAB = 1; + const allianceAC = 2; + const allianceBC = 3; + + (display as any).events = [ + makeRenewal(allianceAB, 1), // A–B + makeRenewal(allianceAC, 1), // A–C + makeRenewal(allianceBC, 2), // B–C + ]; + + // Break alliance A–B + (display as any).removeAllianceRenewalEvents(allianceAB); + + const remaining = (display as any).events; + + // A–B renewal removed + expect(remaining.some((e: any) => e.allianceID === allianceAB)).toBe(false); + + // Other alliances untouched + expect(remaining.some((e: any) => e.allianceID === allianceAC)).toBe(true); + + expect(remaining.some((e: any) => e.allianceID === allianceBC)).toBe(true); + }); + + test("does NOT remove renewals just because the same player is involved", () => { + const display = new EventsDisplay(); + + const allianceAB = 10; + const allianceAC = 11; + + (display as any).events = [ + makeRenewal(allianceAB, 1), // Player 1 involved + makeRenewal(allianceAC, 1), // Same player, different alliance + ]; + + (display as any).removeAllianceRenewalEvents(allianceAB); + + const remaining = (display as any).events; + + expect(remaining.length).toBe(1); + expect(remaining[0].allianceID).toBe(allianceAC); + }); + + test("breaking one alliance does not affect renewals between other players", () => { + const display = new EventsDisplay(); + + const allianceAB = 100; + const allianceCD = 200; + + (display as any).events = [ + makeRenewal(allianceAB, 1), // A–B + makeRenewal(allianceCD, 3), // C–D + ]; + + (display as any).removeAllianceRenewalEvents(allianceAB); + + const remaining = (display as any).events; + + expect(remaining.length).toBe(1); + expect(remaining[0].allianceID).toBe(allianceCD); + }); + + test("does not affect non-RENEW_ALLIANCE events", () => { + const display = new EventsDisplay(); + + (display as any).events = [ + { + description: "Alliance broken", + type: MessageType.ALLIANCE_BROKEN, + createdAt: 0, + }, + { + description: "Alliance accepted", + type: MessageType.ALLIANCE_ACCEPTED, + createdAt: 0, + }, + { + description: "Renewal", + type: MessageType.RENEW_ALLIANCE, + allianceID: 999, + createdAt: 0, + }, + ]; + + (display as any).removeAllianceRenewalEvents(999); + + const remaining = (display as any).events; + + expect( + remaining.some((e: any) => e.type === MessageType.ALLIANCE_BROKEN), + ).toBe(true); + + expect( + remaining.some((e: any) => e.type === MessageType.ALLIANCE_ACCEPTED), + ).toBe(true); + + expect( + remaining.some((e: any) => e.type === MessageType.RENEW_ALLIANCE), + ).toBe(false); + }); +});