feat: Add alliance request panel with colored circle indicators (#2905) #2906
Conversation
…frontio#2905) - Add AllianceRequestPanel component showing colored circles for incoming alliance requests and renewal notifications - Circles display player color with countdown overlay showing time remaining - Hover popup shows player name, seconds remaining, accept/reject buttons - Different icons: blue ? for requests, orange hourglass for renewals, green pulsing for 'wants to renew' - Move alliance request/renewal UI from EventsDisplay chat to dedicated panel - Position panel to left of chat at bottom right of screen
|
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughA new AllianceRequestPanel UI component was added and integrated into the renderer and layout. Alliance request/renewal UI was moved from EventsDisplay to this panel. Core game models and views were extended to surface cross-player extension state and timings were shortened; tests for renewal panel logic were added. Changes
Sequence Diagram(s)sequenceDiagram
participant EB as EventBus
participant ARP as AllianceRequestPanel
participant GR as GameRenderer/UI
participant User as Player
EB->>ARP: onAllianceRequestEvent(update)
ARP->>ARP: create/update AllianceIndicator (queue if popup open)
ARP-->>GR: renderLayer() / tick() -> draw indicators
User->>GR: hover / click indicator
GR->>ARP: forward interaction (open popup / focus)
User->>ARP: accept / reject / request extension
ARP->>EB: emit SendAllianceReplyIntentEvent / SendAllianceExtensionIntentEvent
EB->>ARP: onAllianceRequestReplyEvent(update)
ARP->>ARP: remove/update indicator (flush queued changes)
ARP-->>GR: rerender without indicator
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/client/graphics/layers/AllianceRequestPanel.ts`:
- Line 4: Remove the unused import symbol MessageType from the import statement
that currently reads something like "import { MessageType, Tick } ..."; update
that import to only include Tick (e.g., "import { Tick } ...") so ESLint no
longer flags an unused import and imports remain clean.
🧹 Nitpick comments (5)
src/core/configuration/DefaultConfig.ts (1)
528-530: Redundant conditional — simplify or fix the intended values.Both branches of the ternary return
300, making the condition pointless. If the intention was to unify spawn phase duration, just return300directly. If different values were intended, the first branch needs correction.Suggested simplification
numSpawnPhaseTurns(): number { - return this._gameConfig.gameType === GameType.Singleplayer ? 300 : 300; + return 300; }src/client/graphics/layers/EventsDisplay.ts (2)
281-283: Remove the empty method or its call site.The method
checkForAllianceExpirationsis now a no-op with just a comment, but it is still called every tick at line 225. Remove either the method or the call to avoid confusion and unnecessary overhead.Option 1: Remove the method entirely
Remove line 225 (
this.checkForAllianceExpirations();) and delete lines 281-283.
407-410: The handler does nothing — consider removing from updateMap.The comment says this handler is kept for event filtering, but an empty handler does not actually filter anything. If no action is needed here, consider removing
GameUpdateType.AllianceRequestfromupdateMapto avoid confusion.src/client/graphics/layers/AllianceRequestPanel.ts (2)
327-347: Non-null assertions on optional fields — add guards for safety.The
!assertions onrequestorView,recipientView, andotherPlayerViewassume these fields are always present based on indicator type. While the current logic ensures this, a future refactor could break these assumptions silently. Consider adding explicit guards.Safer approach
private handleAccept(indicator: AllianceIndicator) { if (indicator.type === "request") { + if (!indicator.requestorView || !indicator.recipientView) return; this.eventBus.emit( new SendAllianceReplyIntentEvent( - indicator.requestorView!, - indicator.recipientView!, + indicator.requestorView, + indicator.recipientView, true, ), ); } else if (indicator.type === "renewal") { + if (!indicator.otherPlayerView) return; this.eventBus.emit( - new SendAllianceExtensionIntentEvent(indicator.otherPlayerView!), + new SendAllianceExtensionIntentEvent(indicator.otherPlayerView), ); }
621-656: Run Prettier to fix formatting.The pipeline flags formatting issues. The nested HTML structure is correct but indentation is inconsistent. Run
npx prettier --write src/client/graphics/layers/AllianceRequestPanel.tsto fix.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
index.htmlsrc/client/graphics/GameRenderer.tssrc/client/graphics/layers/AllianceRequestPanel.tssrc/client/graphics/layers/EventsDisplay.tssrc/core/configuration/DefaultConfig.ts
🧰 Additional context used
🧠 Learnings (12)
📚 Learning: 2025-10-18T11:00:57.142Z
Learnt from: NewYearNewPhil
Repo: openfrontio/OpenFrontIO PR: 2230
File: src/client/graphics/GameRenderer.ts:269-277
Timestamp: 2025-10-18T11:00:57.142Z
Learning: In src/client/graphics/GameRenderer.ts, the GameRecapCapture implementation does not use setCaptureRenderEnabled on layers. Instead, it uses RecapCaptureSurface.capture() to render capture layers (TerrainLayer, TerritoryLayer, RailroadLayer, StructureIconsLayer, UnitLayer) directly to an off-screen canvas without requiring layer-level capture mode methods.
Applied to files:
src/client/graphics/GameRenderer.ts
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/client/graphics/GameRenderer.tssrc/client/graphics/layers/AllianceRequestPanel.tsindex.html
📚 Learning: 2025-08-12T00:31:50.144Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1752
File: src/core/game/Game.ts:750-752
Timestamp: 2025-08-12T00:31:50.144Z
Learning: In the OpenFrontIO codebase, changes to the PlayerInteraction interface (like adding canDonateGold and canDonateTroops flags) do not require corresponding updates to src/core/Schemas.ts or server serialization code.
Applied to files:
src/client/graphics/GameRenderer.tssrc/client/graphics/layers/EventsDisplay.ts
📚 Learning: 2026-01-12T21:37:01.156Z
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2874
File: src/server/MapLandTiles.ts:7-11
Timestamp: 2026-01-12T21:37:01.156Z
Learning: In this repository's OpenFrontIO deployment, inter-service HTTP calls to the master should target http://localhost:3000 (master at port 3000) as the canonical address. Apply this as the standard for all server-side TypeScript code that communicates with the master. Avoid hardcoding non-master URLs; centralize the master address (e.g., via config or env) when possible, and ensure internal service communication uses localhost:3000 in this architecture.
Applied to files:
src/client/graphics/GameRenderer.tssrc/client/graphics/layers/AllianceRequestPanel.tssrc/core/configuration/DefaultConfig.tssrc/client/graphics/layers/EventsDisplay.ts
📚 Learning: 2026-01-13T20:16:05.535Z
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2887
File: src/core/execution/NukeExecution.ts:118-122
Timestamp: 2026-01-13T20:16:05.535Z
Learning: In code paths that return a Player-like object, prefer returning a union type (Player | TerraNullius) instead of undefined. When a function may fail to find a player, return TerraNullius for the 'not found' case and a Player for valid IDs, and check .isPlayer() (or equivalent) directly on the result instead of guarding with undefined or optional chaining. This should be enforced in Game, GameImpl, and GameView (and similar accessors) to avoid undefined checks and simplify null-safety handling.
Applied to files:
src/client/graphics/GameRenderer.ts
📚 Learning: 2025-12-29T23:33:17.920Z
Learnt from: wraith4081
Repo: openfrontio/OpenFrontIO PR: 2735
File: index.html:390-391
Timestamp: 2025-12-29T23:33:17.920Z
Learning: In Tailwind CSS v4, update blur-related utilities in HTML templates. The mappings are: backdrop-blur-sm (v3) -> backdrop-blur-xs (v4); backdrop-blur (bare) -> backdrop-blur-sm; blur-sm -> blur-xs. Apply these changes to all HTML files (e.g., index.html and other templates) to reflect the v4 naming. Consider updating a project-wide search/replace or using a codemod to ensure consistency across the codebase.
Applied to files:
index.html
📚 Learning: 2025-10-20T20:15:28.858Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:51-51
Timestamp: 2025-10-20T20:15:28.858Z
Learning: In src/core/execution/FakeHumanExecution.ts, game balance constants like MIRV_COOLDOWN_TICKS, MIRV_HESITATION_ODDS, VICTORY_DENIAL_TEAM_THRESHOLD, VICTORY_DENIAL_INDIVIDUAL_THRESHOLD, and STEAMROLL_CITY_GAP_MULTIPLIER are experimental tuning parameters subject to frequent change during balance testing. Do not flag changes to these values as issues or compare them against previous values.
Applied to files:
src/core/configuration/DefaultConfig.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: For the window close confirmation feature in `ClientGameRunner.ts`, the troop count requirement (>10,000 troops) from issue `#2137` was intentionally removed because it was arbitrary and troop count can be reported as low despite having significant land. The confirmation now triggers for any alive player regardless of troop count.
Applied to files:
src/core/configuration/DefaultConfig.ts
📚 Learning: 2025-10-20T11:02:16.969Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:57-57
Timestamp: 2025-10-20T11:02:16.969Z
Learning: In src/core/execution/FakeHumanExecution.ts, the correct MIRV victory denial thresholds are VICTORY_DENIAL_TEAM_THRESHOLD = 0.8 (80% for team games) and VICTORY_DENIAL_INDIVIDUAL_THRESHOLD = 0.65 (65% for individual players), not 0.85 and 0.7 as might be mentioned in some documentation.
Applied to files:
src/core/configuration/DefaultConfig.ts
📚 Learning: 2025-11-12T23:11:34.445Z
Learnt from: MaxHT0x
Repo: openfrontio/OpenFrontIO PR: 2262
File: src/core/configuration/DefaultConfig.ts:806-832
Timestamp: 2025-11-12T23:11:34.445Z
Learning: In src/core/configuration/DefaultConfig.ts, JSDoc documentation for configuration methods should not be added inline, as it was requested that documentation be placed elsewhere in the codebase.
Applied to files:
src/core/configuration/DefaultConfig.ts
📚 Learning: 2025-12-26T22:21:21.904Z
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2689
File: src/client/PublicLobby.ts:245-245
Timestamp: 2025-12-26T22:21:21.904Z
Learning: In public lobbies with HumansVsNations mode in src/client/PublicLobby.ts, maxPlayers represents only human player slots (already halved in DefaultConfig.ts). The nation NPCs are added automatically server-side and don't count toward maxPlayers. Therefore, getTeamSize correctly returns maxPlayers directly for HumansVsNations to display the proper team size (e.g., maxPlayers=5 yields "5 Humans vs 5 Nations").
Applied to files:
src/core/configuration/DefaultConfig.ts
📚 Learning: 2025-10-26T15:37:07.732Z
Learnt from: GlacialDrift
Repo: openfrontio/OpenFrontIO PR: 2298
File: src/client/graphics/layers/TerritoryLayer.ts:200-210
Timestamp: 2025-10-26T15:37:07.732Z
Learning: In GameImpl.ts lines 124-139, team assignment logic varies by number of teams: when numPlayerTeams < 8, teams are assigned ColoredTeams values (Red, Blue, Yellow, Green, Purple, Orange, Teal); when numPlayerTeams >= 8, teams are assigned generic string identifiers like "Team 1", "Team 2", etc., which are not members of ColoredTeams.
Applied to files:
src/core/configuration/DefaultConfig.ts
🧬 Code graph analysis (1)
src/client/graphics/layers/AllianceRequestPanel.ts (6)
src/core/game/GameView.ts (2)
PlayerView(185-582)GameView(584-942)src/core/EventBus.ts (1)
EventBus(7-44)src/core/game/AllianceImpl.ts (1)
other(19-24)src/core/game/GameUpdates.ts (2)
AllianceRequestUpdate(188-193)AllianceRequestReplyUpdate(195-199)src/client/Transport.ts (2)
SendAllianceReplyIntentEvent(58-65)SendAllianceExtensionIntentEvent(67-69)src/client/graphics/layers/Leaderboard.ts (1)
GoToPlayerEvent(21-23)
🪛 GitHub Actions: 🧪 CI
src/client/graphics/layers/AllianceRequestPanel.ts
[warning] 1-1: Prettier formatting issue detected. The check failed (exit code 1). Run 'npx prettier --write src/client/graphics/layers/AllianceRequestPanel.ts' or 'npx prettier --write .' to fix all formatting issues.
🪛 GitHub Check: 🔍 ESLint
src/client/graphics/layers/AllianceRequestPanel.ts
[failure] 4-4:
'MessageType' is defined but never used.
🔇 Additional comments (5)
src/client/graphics/layers/AllianceRequestPanel.ts (2)
19-34: Good use of typed union for indicator types.The
AllianceIndicatorinterface uses a cleantype: "request" | "renewal"discriminator with optional fields for each variant. This is idiomatic TypeScript — no class hierarchies needed.
69-131: Good pattern: queue changes while popup is open.The tick method correctly defers indicator updates when the popup is being hovered. This prevents jarring UI changes while the user is interacting. The forced close when the hovered indicator expires is a sensible edge case handler.
src/client/graphics/GameRenderer.ts (2)
126-134: Integration follows existing patterns — looks good.The
allianceRequestPanelis wired up identically to other UI layers: query the element, validate type, assigneventBusandgame, and log error if not found. Consistent and predictable.
273-275: Layer ordering is sensible.Placing
allianceRequestPanelbetweeneventsDisplayandchatDisplaygroups related UI layers together. All returnshouldTransform() = false, avoiding unnecessary context save/restore calls.index.html (1)
249-257: Clean layout for the new panel.The new structure creates a horizontal flex container at bottom-right.
alliance-request-panelsits to the left of the chat/events column. Thepointer-events-noneon the container withpointer-events-autoon child panels (set in their styles) is a good pattern for click-through.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
- Add events_display.seconds_remaining for countdown text - Add events_display.wants_to_renew for renewal indicator
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/client/graphics/layers/AllianceRequestPanel.ts`:
- Around line 609-611: In the AllianceRequestPanel template there's an extra
closing brace in the conditional expression (the fragment using indicator.type
=== "renewal" and indicator.otherPlayerWantsRenewal) causing a parse error; edit
the template inside AllianceRequestPanel (the renewal indicator block) to remove
the surplus '}' so the tertiary expression ends with ''}' -> '}' (i.e., ensure
the false branch returns an empty string without the extra brace).
- Around line 368-373: handleFocus currently casts the result of playerBySmallID
to PlayerView after a truthy check, but playerBySmallID can return TerraNullius
(which is truthy); update handleFocus to explicitly check that the returned
value is not TerraNullius (e.g. compare against the TerraNullius sentinel or use
the project’s isTerraNullius/type-guard helper) before casting and emitting
GoToPlayerEvent so only real PlayerView instances are passed to new
GoToPlayerEvent.
♻️ Duplicate comments (1)
src/client/graphics/layers/AllianceRequestPanel.ts (1)
4-4: Remove unused importMessageType.ESLint flags
MessageTypeas imported but never used.Fix
-import { MessageType, Tick } from "../../../core/game/Game"; +import { Tick } from "../../../core/game/Game";
🧹 Nitpick comments (3)
src/client/graphics/layers/AllianceRequestPanel.ts (3)
62-65: Minor: Redundant initialization.
this.indicatorsis already initialized to[]at line 42. The constructor assignment is not needed.Optional cleanup
constructor() { super(); - this.indicators = []; }
196-196: Consider extracting magic number to a named constant.The value
3 * 10(ticks for 3-second buffer) is used here and similarly20at line 302. A named constant would make the tick-to-seconds conversion clearer.Example
// At class level or module scope const TICKS_PER_SECOND = 10; const RENEWAL_BUFFER_SECONDS = 3; const REQUEST_BUFFER_SECONDS = 2; // Usage duration: this.game.config().allianceExtensionPromptOffset() - RENEWAL_BUFFER_SECONDS * TICKS_PER_SECOND,
1-669: Note: Run Prettier to fix formatting.Pipeline reports code style issues. Run
npx prettier --write src/client/graphics/layers/AllianceRequestPanel.tsbefore merging.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
resources/lang/en.jsonsrc/client/graphics/layers/AllianceRequestPanel.ts
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/client/graphics/layers/AllianceRequestPanel.ts
📚 Learning: 2025-08-12T00:31:50.144Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1752
File: src/core/game/Game.ts:750-752
Timestamp: 2025-08-12T00:31:50.144Z
Learning: In the OpenFrontIO codebase, changes to the PlayerInteraction interface (like adding canDonateGold and canDonateTroops flags) do not require corresponding updates to src/core/Schemas.ts or server serialization code.
Applied to files:
src/client/graphics/layers/AllianceRequestPanel.ts
📚 Learning: 2025-06-02T14:27:37.609Z
Learnt from: andrewNiziolek
Repo: openfrontio/OpenFrontIO PR: 1007
File: resources/lang/de.json:115-115
Timestamp: 2025-06-02T14:27:37.609Z
Learning: For OpenFrontIO project: When localization keys are renamed in language JSON files, the maintainers separate technical changes from translation content updates. They wait for community translators to update the actual translation values rather than attempting to translate in the same PR. This allows technical changes to proceed while ensuring accurate translations from native speakers.
Applied to files:
src/client/graphics/layers/AllianceRequestPanel.tsresources/lang/en.json
📚 Learning: 2025-08-27T08:12:19.610Z
Learnt from: mokizzz
Repo: openfrontio/OpenFrontIO PR: 1940
File: resources/lang/en.json:763-766
Timestamp: 2025-08-27T08:12:19.610Z
Learning: In OpenFrontIO, some country entries in src/client/data/countries.json may have similar names but different codes (e.g., "Empire of Japan" vs "Empire of Japan1"). Each unique code requires its own translation key in resources/lang/en.json after normalization. Always verify against countries.json before suggesting removal of translation keys.
Applied to files:
src/client/graphics/layers/AllianceRequestPanel.ts
📚 Learning: 2026-01-12T21:37:01.156Z
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2874
File: src/server/MapLandTiles.ts:7-11
Timestamp: 2026-01-12T21:37:01.156Z
Learning: In this repository's OpenFrontIO deployment, inter-service HTTP calls to the master should target http://localhost:3000 (master at port 3000) as the canonical address. Apply this as the standard for all server-side TypeScript code that communicates with the master. Avoid hardcoding non-master URLs; centralize the master address (e.g., via config or env) when possible, and ensure internal service communication uses localhost:3000 in this architecture.
Applied to files:
src/client/graphics/layers/AllianceRequestPanel.ts
🧬 Code graph analysis (1)
src/client/graphics/layers/AllianceRequestPanel.ts (7)
src/core/game/Game.ts (1)
Tick(25-25)src/core/game/GameView.ts (3)
PlayerView(185-582)GameView(584-942)myPlayer(753-755)src/core/EventBus.ts (1)
EventBus(7-44)src/core/game/AllianceImpl.ts (1)
other(19-24)src/core/game/GameUpdates.ts (2)
AllianceRequestUpdate(188-193)AllianceRequestReplyUpdate(195-199)src/client/Transport.ts (2)
SendAllianceReplyIntentEvent(58-65)SendAllianceExtensionIntentEvent(67-69)src/client/graphics/layers/Leaderboard.ts (1)
GoToPlayerEvent(21-23)
🪛 GitHub Actions: 🧪 CI
src/client/graphics/layers/AllianceRequestPanel.ts
[warning] 1-1: Code style issues found in this file. Run 'npx prettier --write' to fix.
🪛 GitHub Check: 🔍 ESLint
src/client/graphics/layers/AllianceRequestPanel.ts
[failure] 4-4:
'MessageType' is defined but never used.
🔇 Additional comments (8)
resources/lang/en.json (1)
718-719: LGTM!New localization keys are correctly structured and follow the existing
events_display.*naming convention. The{seconds}placeholder uses the proper format for this project.src/client/graphics/layers/AllianceRequestPanel.ts (7)
19-34: LGTM!Clean use of typed union (
"request" | "renewal") instead of class hierarchy. This follows good TypeScript composition patterns.
69-131: LGTM!The tick lifecycle correctly handles the popup hover state by queuing changes, and properly forces popup closure when the hovered indicator expires. Good defensive check for player existence and alive state.
133-160: LGTM!Good null safety checks before accessing optional properties. The deletion logic correctly handles both request and renewal indicator types.
327-340: LGTM, but consider type narrowing for optional fields.The non-null assertions (
!) are justified by the type discriminator check (indicator.type === "request"), but TypeScript doesn't automatically narrow optional fields. Current code works correctly.
388-569: LGTM!The inline CSS is well-organized and appropriately scoped to this component. Transitions and animations are smooth. Colors follow common UI conventions.
666-668: LGTM!Returning
thisfromcreateRenderRoot()disables Shadow DOM, allowing the component to use global Tailwind classes. This matches patterns used elsewhere in the codebase.
286-291: Remove theas PlayerViewcasts—they are intentional and correct here.
playerBySmallID()returnsPlayerView | TerraNullius. The casts are intentional and widely used throughout the codebase at trusted call sites (like event handlers receiving data from the server). The maintainers prefer loud failures over silent masking when handling data from trusted sources. In alliance request events,requestorIDandrecipientIDcome directly from the server and are guaranteed to be valid player IDs (never 0/TerraNullius), so the casts are safe. No additional guards are needed.Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
How does this look for mobile? |
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In `@src/client/graphics/layers/AllianceRequestPanel.ts`:
- Around line 458-481: Prettier formatting is failing in the describeArc method;
run a code formatter to reflow long template strings and fix line length/spacing
issues in describeArc (the returned array of path segments)—e.g., run npx
prettier --write . (or your project's pre-commit formatter) and commit the
updated file so the long template literal/array lines in describeArc are wrapped
to match the project's Prettier rules.
In `@src/core/configuration/DefaultConfig.ts`:
- Around line 528-531: The change hardcodes the spawn phase length by returning
100 in DefaultConfig.numSpawnPhaseTurns(), removing any per-game-type behavior;
restore configurability by either reading a config value or delegating to the
previous game-type logic: update DefaultConfig.numSpawnPhaseTurns() to check the
existing configuration or game-mode accessor (the same place other mode-specific
settings are derived) and return the mode-specific tick value, or expose a new
config property (e.g., spawnPhaseTurns) that defaults to 100 and use that in
numSpawnPhaseTurns() so non-default game types can still override the spawn
duration.
In `@src/core/execution/nation/NationAllianceBehavior.ts`:
- Around line 43-55: The tests are failing because the test double for alliance
lacks the expiresAt() method used in NationAllianceBehavior (the code calls
alliance.expiresAt() to compare with this.game.ticks() +
this.game.config().allianceExtensionPromptOffset()); either add an expiresAt()
method to the alliance mock/stub that returns a numeric tick (e.g., based on the
test game tick) or, if the real runtime shape exposes a property, change the
code to read alliance.expiresAt (property) instead; update any test
factories/mocks that build alliance instances (those used in tests exercising
onlyOneAgreedToExtend and bothAgreedToExtend branches) so they provide the
correct expiresAt value consistent with the test scenarios.
- Around line 76-81: The change to this.random.chance(2) in
NationAllianceBehavior greatly increases alliance request frequency and can spam
players; revert to a configurable value and gate aggressive testing behavior
behind a config/difficulty flag: replace the hardcoded 2 with a config-driven
variable (e.g., gameConfig.allianceRequestChance or
difficulty.allianceRequestMultiplier) used by the call to this.random.chance,
keep the previous default (30) for normal play, and expose a test/dev override
(e.g., gameConfig.debugHighAllianceRate) so the aggressive rate is only enabled
when that flag is true; ensure references to this.random.chance,
player.canSendAllianceRequest, and getAllianceDecision remain unchanged.
- Around line 47-51: The comment says "90% chance" but the code uses
this.random.chance(2) (which is ~50% in our RNG API); update the call to match
the comment by changing this.random.chance(2) to this.random.chance(9) so the
proactive renewal request probability aligns with the "90% chance" comment;
adjust only the numeric argument in the block that checks isExpiringSoon and the
alliance methods (alliance.onlyOneAgreedToExtend, alliance.bothAgreedToExtend)
and leave the rest (getAllianceDecision, this.game.addExecution) intact.
In `@tests/AllianceRenewalPanel.test.ts`:
- Line 4: The import list includes an unused symbol Tick; update the import
statement in AllianceRenewalPanel.test.ts by removing Tick from the named
imports (leaving Game, Player, PlayerType) so the unused import is eliminated
and lint/static-analysis warnings are resolved.
- Line 56: The variable ticksBefore (assigned from game.ticks()) is unused;
either remove the declaration of ticksBefore or add assertions that actually use
it (e.g., compare game.ticks() before/after action). Locate the assignment to
ticksBefore in the test (the call to game.ticks()) and either delete that line
or replace it with appropriate assertion logic that references ticksBefore to
validate tick changes.
- Around line 407-429: The test assigns promptOffset but never asserts
duplicate-prompt behavior; complete it by using promptOffset with
game.executeNextTick() to verify prompts are suppressed within the window and
allowed after it. Specifically: after creating the alliance via
AllianceRequestExecution and AllianceRequestReplyExecution and confirming
player1.allianceWith(player2), spy on the method that emits the extension prompt
(e.g., AllianceRenewalPanel.promptForAllianceExtension or the notification/send
method used) then advance ticks by less than promptOffset and assert the spy was
not called, then advance ticks past promptOffset and assert the spy was called
(or remove the test if you prefer not to test this behavior). Ensure you
reference promptOffset, game.executeNextTick(), AllianceRequestExecution,
AllianceRequestReplyExecution, and player1.allianceWith(...) in the updated
test.
🧹 Nitpick comments (4)
src/core/game/GameView.ts (1)
616-673: Keep test nation patterns behind a dev flag.Lines 616-673 add hardcoded test patterns and apply them to every other nation. Please gate this to dev/test builds or a config toggle so production visuals aren’t changed by “test” data.
src/client/graphics/layers/AllianceRequestPanel.ts (1)
85-101: Prefer a discriminated union forAllianceIndicator.Optional fields force non‑null asserts later. A union makes request/renewal fields required and safer.
♻️ Suggested type shape
-type AllianceIndicator = { - id: string; - type: "request" | "renewal"; - playerSmallID: number; - playerName: string; - playerColor: string; - playerPattern?: PlayerPattern; - createdAt: Tick; - duration: Tick; - requestorView?: PlayerView; - recipientView?: PlayerView; - allianceID?: number; - otherPlayerView?: PlayerView; - otherPlayerWantsRenewal?: boolean; -}; +type BaseIndicator = { + id: string; + playerSmallID: number; + playerName: string; + playerColor: string; + playerPattern?: PlayerPattern; + createdAt: Tick; + duration: Tick; +}; +type RequestIndicator = BaseIndicator & { + type: "request"; + requestorView: PlayerView; + recipientView: PlayerView; +}; +type RenewalIndicator = BaseIndicator & { + type: "renewal"; + allianceID: number; + otherPlayerView: PlayerView; + otherPlayerWantsRenewal: boolean; +}; +type AllianceIndicator = RequestIndicator | RenewalIndicator;tests/AllianceRenewalPanel.test.ts (2)
309-327: Tests rely on hardcoded magic numbers from UI implementation.These tests check specific buffer values (20 and 30 ticks) that are implementation details of
AllianceRequestPanel, not config values. If the panel changes these buffers, these tests will break but won't catch real bugs.Consider either:
- Moving these buffer constants to a shared location (config or constants file) so both the panel and tests reference the same source
- Testing the behavior (indicator expires before the actual request/prompt window ends) rather than specific tick values
397-405: Test assertion is too weak.This test only checks that the offset is greater than zero, which is a very low bar. The comment says the test is about how offset "affects when renewal shows" but there's no assertion about that behavior.
Consider either adding meaningful assertions about how the offset value affects prompt timing, or removing the test if it's redundant with other tests in this file.
| numSpawnPhaseTurns(): number { | ||
| return this._gameConfig.gameType === GameType.Singleplayer ? 100 : 300; | ||
| // Always use 10 seconds (100 ticks) for spawn phase | ||
| return 100; | ||
| } |
There was a problem hiding this comment.
Confirm global 10s spawn phase is intended.
Line 528-531 now fixes spawn phase to 100 ticks for all modes. If the previous game-type behavior mattered, please gate this via config or restore it.
🤖 Prompt for AI Agents
In `@src/core/configuration/DefaultConfig.ts` around lines 528 - 531, The change
hardcodes the spawn phase length by returning 100 in
DefaultConfig.numSpawnPhaseTurns(), removing any per-game-type behavior; restore
configurability by either reading a config value or delegating to the previous
game-type logic: update DefaultConfig.numSpawnPhaseTurns() to check the existing
configuration or game-mode accessor (the same place other mode-specific settings
are derived) and return the mode-specific tick value, or expose a new config
property (e.g., spawnPhaseTurns) that defaults to 100 and use that in
numSpawnPhaseTurns() so non-default game types can still override the spawn
duration.
| // This makes them very likely to request BEFORE the human does (for testing) | ||
| if (isExpiringSoon && !alliance.onlyOneAgreedToExtend() && !alliance.bothAgreedToExtend()) { | ||
| // 90% chance each tick to proactively request renewal | ||
| if (this.random.chance(2) && this.getAllianceDecision(human, true)) { | ||
| this.game.addExecution( |
There was a problem hiding this comment.
Chance value does not match the comment.
Lines 47-51 say “90% chance” but chance(2) is ~50% per tick based on current usage. Update the comment or adjust the chance value.
🤖 Prompt for AI Agents
In `@src/core/execution/nation/NationAllianceBehavior.ts` around lines 47 - 51,
The comment says "90% chance" but the code uses this.random.chance(2) (which is
~50% in our RNG API); update the call to match the comment by changing
this.random.chance(2) to this.random.chance(9) so the proactive renewal request
probability aligns with the "90% chance" comment; adjust only the numeric
argument in the block that checks isExpiringSoon and the alliance methods
(alliance.onlyOneAgreedToExtend, alliance.bothAgreedToExtend) and leave the rest
(getAllianceDecision, this.game.addExecution) intact.
| vi.spyOn(player2, "isAlive").mockReturnValue(true); | ||
| vi.spyOn(player1, "isAlive").mockReturnValue(true); | ||
|
|
||
| const ticksBefore = game.ticks(); |
There was a problem hiding this comment.
Remove unused variable.
ticksBefore is assigned but never used. If you planned to use it in assertions, add that logic. Otherwise, remove it.
- const ticksBefore = game.ticks();📝 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.
| const ticksBefore = game.ticks(); |
🧰 Tools
🪛 GitHub Check: 🔍 ESLint
[failure] 56-56:
'ticksBefore' is assigned a value but never used.
🤖 Prompt for AI Agents
In `@tests/AllianceRenewalPanel.test.ts` at line 56, The variable ticksBefore
(assigned from game.ticks()) is unused; either remove the declaration of
ticksBefore or add assertions that actually use it (e.g., compare game.ticks()
before/after action). Locate the assignment to ticksBefore in the test (the call
to game.ticks()) and either delete that line or replace it with appropriate
assertion logic that references ticksBefore to validate tick changes.
| test("once prompted for an alliance, should not prompt again within offset period", () => { | ||
| vi.spyOn(player1, "canSendAllianceRequest").mockReturnValue(true); | ||
| vi.spyOn(player2, "isAlive").mockReturnValue(true); | ||
| vi.spyOn(player1, "isAlive").mockReturnValue(true); | ||
|
|
||
| // The panel tracks alliancesCheckedAt to avoid duplicate prompts | ||
| // Logic: if checkedAt >= ticks - promptOffset, skip | ||
| const promptOffset = game.config().allianceExtensionPromptOffset(); | ||
|
|
||
| // Create alliance | ||
| game.addExecution(new AllianceRequestExecution(player1, player2.id())); | ||
| game.executeNextTick(); | ||
| game.addExecution( | ||
| new AllianceRequestReplyExecution(player1.id(), player2, true), | ||
| ); | ||
| game.executeNextTick(); | ||
|
|
||
| const alliance = player1.allianceWith(player2); | ||
| expect(alliance).toBeTruthy(); | ||
|
|
||
| // The map would store when we last checked, preventing duplicate notifications | ||
| // within the prompt offset window | ||
| }); |
There was a problem hiding this comment.
Incomplete test: promptOffset is assigned but never used.
This test sets up an alliance but ends with a comment about what "would" happen without actually asserting the behavior. The test does not verify that duplicate prompts are prevented.
Either complete the test with real assertions or remove it. As-is, it passes but validates nothing about the duplicate prompt logic.
// The map would store when we last checked, preventing duplicate notifications
// within the prompt offset window
+ // TODO: Add assertions for duplicate prompt prevention
});🧰 Tools
🪛 GitHub Check: 🔍 ESLint
[failure] 414-414:
'promptOffset' is assigned a value but never used.
🤖 Prompt for AI Agents
In `@tests/AllianceRenewalPanel.test.ts` around lines 407 - 429, The test assigns
promptOffset but never asserts duplicate-prompt behavior; complete it by using
promptOffset with game.executeNextTick() to verify prompts are suppressed within
the window and allowed after it. Specifically: after creating the alliance via
AllianceRequestExecution and AllianceRequestReplyExecution and confirming
player1.allianceWith(player2), spy on the method that emits the extension prompt
(e.g., AllianceRenewalPanel.promptForAllianceExtension or the notification/send
method used) then advance ticks by less than promptOffset and assert the spy was
not called, then advance ticks past promptOffset and assert the spy was called
(or remove the test if you prefer not to test this behavior). Ensure you
reference promptOffset, game.executeNextTick(), AllianceRequestExecution,
AllianceRequestReplyExecution, and player1.allianceWith(...) in the updated
test.
Looks the same, seems to work fine |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@src/client/graphics/layers/AllianceRequestPanel.ts`:
- Around line 313-322: The renewal indicator’s duration is computed from
this.game.config().allianceExtensionPromptOffset() - 3*10 and can be <= 0 when
the prompt offset is small; update the addIndicator call that sets duration (the
object created for id `renewal-${alliance.id}`) to clamp the computed value to a
minimum of 1 tick (e.g., compute duration = Math.max(1,
this.game.config().allianceExtensionPromptOffset() - 30) or equivalent) so the
indicator never vanishes immediately.
- Around line 458-474: In onAllianceRequestReplyEvent avoid directly mutating
this.indicators while this.popupHovered is true: instead, collect matching
removals into a new queue (e.g., this.pendingIndicatorRemovals) when
popupHovered is true and only filter them out of this.indicators and call
this.requestUpdate() when popupHovered becomes false (flush the queue); when
popupHovered is false behave as today (immediate filter and requestUpdate).
Ensure you add the pendingIndicatorRemovals storage to the class, use
update.request.requestorID to identify queued items, and flush the queue in
whatever method toggles popupHovered (or on hover-leave) so the “freeze”
behavior is preserved.
In `@src/core/configuration/DefaultConfig.ts`:
- Around line 508-509: The allianceDuration() method in DefaultConfig.ts
currently returns a hardcoded 45*10 (45s) “for testing”; change it to return the
proper production default (5 minutes) unless a test flag/config is set. Modify
allianceDuration() to read a configuration or environment flag (e.g. a
isTestMode/isFastAlliances boolean from the config object or
process.env.FAST_ALLIANCES) and return 45*10 when that flag is true, otherwise
return 5*60 (or equivalent Tick value) for normal runs so production games keep
the 5‑minute duration.
In `@src/core/game/GameView.ts`:
- Around line 615-675: The code injects hardcoded test skins via testPatterns
into this._cosmetics for every other nation (loop over this._mapData.nations and
the this._cosmetics.set call), which must not ship to production; either remove
the testPatterns block and the alternating assignment entirely or gate it behind
a debug/config flag (e.g., GAME_DEBUG or a GameView option) and only apply the
pattern when that flag is true; ensure the assigned object still satisfies
PlayerCosmetics and that nation.flag is preserved when the flag is off.
In `@src/core/game/NationCreation.ts`:
- Around line 52-69: Remove the TEST/debug block that forces a 30-nation fill:
delete the entire conditional that checks targetNationCount and the loop that
calls generateUniqueNationName, creates new Nation(...) with undefined spawnCell
and pushes onto baseNations; instead return baseNations as produced by
effectiveNations.map(toNation) so compact map and spawn cell logic (lines using
toNation/effectiveNations) remain authoritative; ensure no references to
usedNames, targetNationCount, or random.nextID() remain in NationCreation.ts.
♻️ Duplicate comments (6)
src/core/configuration/DefaultConfig.ts (1)
527-529: Confirm spawn phase is intentionally fixed to 10 seconds.This still removes game‑type differences. If it’s test‑only, please gate it like the alliance duration.
src/core/execution/nation/NationAllianceBehavior.ts (3)
43-46: Tests need an expiresAt() stub.New use of
alliance.expiresAt()breaks existing test doubles (CI already fails). Update alliance mocks to provideexpiresAt()(or align with the runtime shape).
55-57: Chance value and comment disagree.The comment says 90%, but
chance(2)is ~50%. Update the comment or the chance value to match.
82-85: Alliance request rate looks test‑only.
chance(2)is extremely frequent and can spam players. If it’s for testing, gate it by config/difficulty and keep the normal rate for production.tests/AllianceRenewalPanel.test.ts (2)
56-56: Remove unusedticksBefore.This variable is unused and breaks lint.
🔧 Suggested fix
- const ticksBefore = game.ticks();
407-429: Finish the duplicate‑prompt test or remove it.
promptOffsetis unused and the test ends without assertions. Add real checks or drop the test.
🧹 Nitpick comments (2)
src/client/graphics/layers/AllianceRequestPanel.ts (2)
23-78: Cap the pattern preview cache.
patternPreviewCachegrows with every unique pattern/size and never evicts. Consider a small size cap or simple LRU to avoid unbounded growth in long sessions.
103-103: Make the indicator cap configurable.It’s set to 5 with a TEST note. If production should show 20, keep 20 by default and make the test value a config override.
| this.addIndicator({ | ||
| id: `renewal-${alliance.id}`, | ||
| type: "renewal", | ||
| playerSmallID: other.smallID(), | ||
| playerName: other.name(), | ||
| playerColor: color, | ||
| playerPattern: other.cosmetics.pattern, | ||
| createdAt: this.game.ticks(), | ||
| duration: this.game.config().allianceExtensionPromptOffset() - 3 * 10, // 3 second buffer | ||
| allianceID: alliance.id, |
There was a problem hiding this comment.
Guard renewal duration against small prompt offsets.
If allianceExtensionPromptOffset() is ≤ 30 ticks, duration becomes zero/negative and the indicator will vanish immediately. Clamp to at least 1 tick.
🔧 Suggested fix
- duration: this.game.config().allianceExtensionPromptOffset() - 3 * 10, // 3 second buffer
+ duration: Math.max(
+ 1,
+ this.game.config().allianceExtensionPromptOffset() - 3 * 10,
+ ), // 3 second buffer🤖 Prompt for AI Agents
In `@src/client/graphics/layers/AllianceRequestPanel.ts` around lines 313 - 322,
The renewal indicator’s duration is computed from
this.game.config().allianceExtensionPromptOffset() - 3*10 and can be <= 0 when
the prompt offset is small; update the addIndicator call that sets duration (the
object created for id `renewal-${alliance.id}`) to clamp the computed value to a
minimum of 1 tick (e.g., compute duration = Math.max(1,
this.game.config().allianceExtensionPromptOffset() - 30) or equivalent) so the
indicator never vanishes immediately.
| onAllianceRequestReplyEvent(update: AllianceRequestReplyUpdate) { | ||
| const myPlayer = this.game.myPlayer(); | ||
| if (!myPlayer) { | ||
| return; | ||
| } | ||
| // myPlayer can deny alliances without clicking on the button | ||
| if (update.request.recipientID === myPlayer.smallID()) { | ||
| // Remove alliance requests whose requestors are the same as the reply's requestor | ||
| this.indicators = this.indicators.filter( | ||
| (indicator) => | ||
| !( | ||
| indicator.type === "request" && | ||
| indicator.playerSmallID === update.request.requestorID | ||
| ), | ||
| ); | ||
| this.requestUpdate(); | ||
| } |
There was a problem hiding this comment.
Do not mutate indicators while popup is open.
onAllianceRequestReplyEvent removes indicators immediately even when popupHovered, which breaks the “freeze” behavior and can make the popup disappear while hovering. Queue removals instead.
🔧 Suggested fix
if (update.request.recipientID === myPlayer.smallID()) {
- // Remove alliance requests whose requestors are the same as the reply's requestor
- this.indicators = this.indicators.filter(
- (indicator) =>
- !(
- indicator.type === "request" &&
- indicator.playerSmallID === update.request.requestorID
- ),
- );
- this.requestUpdate();
+ const toRemove = this.indicators.filter(
+ (indicator) =>
+ indicator.type === "request" &&
+ indicator.playerSmallID === update.request.requestorID,
+ );
+ if (this.popupHovered) {
+ toRemove.forEach((indicator) => this.pendingRemovals.add(indicator.id));
+ return;
+ }
+ // Remove alliance requests whose requestors are the same as the reply's requestor
+ this.indicators = this.indicators.filter(
+ (indicator) =>
+ !(
+ indicator.type === "request" &&
+ indicator.playerSmallID === update.request.requestorID
+ ),
+ );
+ this.requestUpdate();
}🤖 Prompt for AI Agents
In `@src/client/graphics/layers/AllianceRequestPanel.ts` around lines 458 - 474,
In onAllianceRequestReplyEvent avoid directly mutating this.indicators while
this.popupHovered is true: instead, collect matching removals into a new queue
(e.g., this.pendingIndicatorRemovals) when popupHovered is true and only filter
them out of this.indicators and call this.requestUpdate() when popupHovered
becomes false (flush the queue); when popupHovered is false behave as today
(immediate filter and requestUpdate). Ensure you add the
pendingIndicatorRemovals storage to the class, use update.request.requestorID to
identify queued items, and flush the queue in whatever method toggles
popupHovered (or on hover-leave) so the “freeze” behavior is preserved.
| allianceDuration(): Tick { | ||
| return 300 * 10; // 5 minutes. | ||
| return 45 * 10; // 45 seconds for testing |
There was a problem hiding this comment.
Gate the 45‑second alliance duration for prod.
This is marked “for testing” and shortens alliances for every mode. If fast tests are needed, guard by env/config and keep the 5‑minute default for real games.
🔧 Suggested fix
- allianceDuration(): Tick {
- return 45 * 10; // 45 seconds for testing
- }
+ allianceDuration(): Tick {
+ // Use shorter duration only in dev/test
+ return this._serverConfig.env() === GameEnv.Dev ? 45 * 10 : 300 * 10;
+ }🤖 Prompt for AI Agents
In `@src/core/configuration/DefaultConfig.ts` around lines 508 - 509, The
allianceDuration() method in DefaultConfig.ts currently returns a hardcoded
45*10 (45s) “for testing”; change it to return the proper production default (5
minutes) unless a test flag/config is set. Modify allianceDuration() to read a
configuration or environment flag (e.g. a isTestMode/isFastAlliances boolean
from the config object or process.env.FAST_ALLIANCES) and return 45*10 when that
flag is true, otherwise return 5*60 (or equivalent Tick value) for normal runs
so production games keep the 5‑minute duration.
4 participants
Resolves #2905
Description:
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
3_lambda