Add army limit warning indicator for team games#3743
Open
luctrate wants to merge 1 commit intoopenfrontio:mainfrom
Open
Add army limit warning indicator for team games#3743luctrate wants to merge 1 commit intoopenfrontio:mainfrom
luctrate wants to merge 1 commit intoopenfrontio:mainfrom
Conversation
Show an orange warning banner above the troop bar when a player's troops exceed 80% of their max in team games with troop donation enabled, encouraging them to send troops to teammates. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
WalkthroughAn army limit warning feature is added across the codebase. A new configuration threshold is introduced, localization strings are added, and the ControlPanel component displays a warning banner when troop count approaches the maximum in Team mode with donations enabled. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Author
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/client/graphics/layers/ControlPanel.ts (1)
93-109: Small readability nit: extract the warning check into a tiny helper.The
tick()body now mixes state caching, stat updates, and warning logic. A small private method (e.g.computeArmyLimitWarning(player, config)) returning a boolean would keeptick()easy to scan and make the Team + donateTroops + ratio rule one obvious unit. Purely optional.♻️ Suggested refactor
- const config = this.game.config(); - this._maxTroops = config.maxTroops(player); + const config = this.game.config(); + this._maxTroops = config.maxTroops(player); this._gold = player.gold(); this._troops = player.troops(); this._attackingTroops = player .outgoingAttacks() .map((a) => a.troops) .reduce((a, b) => a + b, 0); - this.troopRate = config.troopIncreaseRate(player) * 10; - const isTeamGame = config.gameConfig().gameMode === GameMode.Team; - const canDonateTroops = config.donateTroops(); - if (isTeamGame && canDonateTroops) { - const ratio = this._troops / Math.max(this._maxTroops, 1); - this._showArmyLimitWarning = ratio >= config.armyLimitWarningThreshold(); - } else { - this._showArmyLimitWarning = false; - } + this.troopRate = config.troopIncreaseRate(player) * 10; + this._showArmyLimitWarning = this.shouldShowArmyLimitWarning(config); + } + + private shouldShowArmyLimitWarning(config = this.game.config()): boolean { + if (config.gameConfig().gameMode !== GameMode.Team) return false; + if (!config.donateTroops()) return false; + const ratio = this._troops / Math.max(this._maxTroops, 1); + return ratio >= config.armyLimitWarningThreshold(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/ControlPanel.ts` around lines 93 - 109, Extract the army-limit warning logic from tick() into a small private helper (e.g., computeArmyLimitWarning(player, config)) so tick() only updates state and delegates the Team+donateTroops+ratio rule; implement computeArmyLimitWarning to check config.gameConfig().gameMode === GameMode.Team && config.donateTroops() and then compute ratio = player.troops() / Math.max(config.maxTroops(player), 1) and return ratio >= config.armyLimitWarningThreshold(); replace the inline block that sets this._showArmyLimitWarning in tick() with this._showArmyLimitWarning = this.computeArmyLimitWarning(player, config).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/graphics/layers/ControlPanel.ts`:
- Around line 104-109: The army-limit warning currently uses this._troops which
excludes troops in outgoingAttacks and thus can flicker; change the ratio
calculation inside the isTeamGame && canDonateTroops branch to use effective
troops (this._troops + this._attackingTroops) divided by
Math.max(this._maxTroops, 1) and keep assigning the result to
this._showArmyLimitWarning using config.armyLimitWarningThreshold(); ensure you
reference the existing symbols (_troops, _attackingTroops, _maxTroops,
_showArmyLimitWarning, config.armyLimitWarningThreshold()) and preserve the
Math.max protection against zero.
---
Nitpick comments:
In `@src/client/graphics/layers/ControlPanel.ts`:
- Around line 93-109: Extract the army-limit warning logic from tick() into a
small private helper (e.g., computeArmyLimitWarning(player, config)) so tick()
only updates state and delegates the Team+donateTroops+ratio rule; implement
computeArmyLimitWarning to check config.gameConfig().gameMode === GameMode.Team
&& config.donateTroops() and then compute ratio = player.troops() /
Math.max(config.maxTroops(player), 1) and return ratio >=
config.armyLimitWarningThreshold(); replace the inline block that sets
this._showArmyLimitWarning in tick() with this._showArmyLimitWarning =
this.computeArmyLimitWarning(player, config).
🪄 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: e317eb81-6ba0-4e1a-a6f4-3312474fdd13
📒 Files selected for processing (4)
resources/lang/en.jsonsrc/client/graphics/layers/ControlPanel.tssrc/core/configuration/Config.tssrc/core/configuration/DefaultConfig.ts
Comment on lines
+104
to
+109
| if (isTeamGame && canDonateTroops) { | ||
| const ratio = this._troops / Math.max(this._maxTroops, 1); | ||
| this._showArmyLimitWarning = ratio >= config.armyLimitWarningThreshold(); | ||
| } else { | ||
| this._showArmyLimitWarning = false; | ||
| } |
Contributor
There was a problem hiding this comment.
Minor: troops() excludes outgoing attacks, so the warning can flicker off during big attacks.
player.troops() does not include troops already committed to outgoing attacks — those live in outgoingAttacks() (you already compute _attackingTroops just above). So a player who is truly near their cap but has launched a large attack will drop below the 80% threshold and the banner will disappear until troops refill. If you want the banner to reflect "effective capacity used", consider using this._troops + this._attackingTroops in the ratio.
- const ratio = this._troops / Math.max(this._maxTroops, 1);
- this._showArmyLimitWarning = ratio >= config.armyLimitWarningThreshold();
+ const used = this._troops + this._attackingTroops;
+ const ratio = used / Math.max(this._maxTroops, 1);
+ this._showArmyLimitWarning = ratio >= config.armyLimitWarningThreshold();Not a bug per se — just noting the behavior so you can decide what feels right for players.
📝 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.
Suggested change
| if (isTeamGame && canDonateTroops) { | |
| const ratio = this._troops / Math.max(this._maxTroops, 1); | |
| this._showArmyLimitWarning = ratio >= config.armyLimitWarningThreshold(); | |
| } else { | |
| this._showArmyLimitWarning = false; | |
| } | |
| if (isTeamGame && canDonateTroops) { | |
| const used = this._troops + this._attackingTroops; | |
| const ratio = used / Math.max(this._maxTroops, 1); | |
| this._showArmyLimitWarning = ratio >= config.armyLimitWarningThreshold(); | |
| } else { | |
| this._showArmyLimitWarning = false; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/graphics/layers/ControlPanel.ts` around lines 104 - 109, The
army-limit warning currently uses this._troops which excludes troops in
outgoingAttacks and thus can flicker; change the ratio calculation inside the
isTeamGame && canDonateTroops branch to use effective troops (this._troops +
this._attackingTroops) divided by Math.max(this._maxTroops, 1) and keep
assigning the result to this._showArmyLimitWarning using
config.armyLimitWarningThreshold(); ensure you reference the existing symbols
(_troops, _attackingTroops, _maxTroops, _showArmyLimitWarning,
config.armyLimitWarningThreshold()) and preserve the Math.max protection against
zero.
Author
There was a problem hiding this comment.
Intentional — the warning targets players with idle troops near cap who should be donating to teammates on the front line. Attacking troops are already being used productively, so including them in the ratio would trigger the warning even when the player is actively engaged in combat, which is the opposite of the behavior we want.
Contributor
There was a problem hiding this comment.
@luctrate, that makes total sense — the warning is specifically for players sitting idle near cap, not for those actively fighting. Including attacking troops would penalize exactly the players who are already doing their part, which would be counterproductive. Thanks for the clear explanation!
(づ｡◕‿‿◕｡)づ
✏️ Learnings added
Learnt from: luctrate
Repo: openfrontio/OpenFrontIO PR: 3743
File: src/client/graphics/layers/ControlPanel.ts:104-109
Timestamp: 2026-04-23T07:19:30.992Z
Learning: In `src/client/graphics/layers/ControlPanel.ts` (OpenFrontIO, PR `#3743`), the army-limit warning ratio intentionally uses only `player.troops()` (idle troops) and excludes `_attackingTroops` (outgoing attack troops). The warning targets players with idle troops near cap who should donate to teammates. Troops already committed to attacks are considered productively used, so including them would incorrectly trigger the warning during active combat. Do not suggest adding `_attackingTroops` to this ratio calculation.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
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.
Learnt from: 1brucben
Repo: openfrontio/OpenFrontIO PR: 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: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 3349
File: src/client/HostLobbyModal.ts:649-660
Timestamp: 2026-03-04T20:14:53.404Z
Learning: In `src/client/HostLobbyModal.ts`, `putGameConfig()` dispatches the entire game config object as a full replacement via `CustomEvent("update-game-config")` on every call (not a partial merge). Fields set to `undefined` in the config (e.g. `startingGold`, `goldMultiplier`, `maxTimerValue`) correctly signal "not set / cleared" to the server. There is no stale-value risk when these fields are `undefined`.
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").
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.
Learnt from: ryanbarlow97
Repo: openfrontio/OpenFrontIO PR: 2782
File: src/client/graphics/layers/StructureIconsLayer.ts:616-620
Timestamp: 2026-01-03T19:37:30.357Z
Learning: In `src/client/graphics/layers/StructureIconsLayer.ts`, the confirm button in `ensureGhostControls()` for locked ghost nukes (AtomBomb/HydrogenBomb) intentionally does NOT call `removeGhostStructure()` after emitting the build intent. This allows mobile players to rapidly place multiple nukes by "spamming" the confirm button without needing to reopen the radial menu.
Learnt from: bijx
Repo: openfrontio/OpenFrontIO PR: 2773
File: src/client/graphics/layers/PlayerInfoOverlay.ts:443-478
Timestamp: 2026-01-03T00:01:09.721Z
Learning: In OpenFrontIO, when an attack starts, the attacker's troops are removed from their pool before the `Attack` object is created. This means `player.troops()` does NOT include troops in outgoing attacks—those troops are tracked separately via `player.outgoingAttacks()`. The troop count and attacking troop count represent distinct, non-overlapping groups.
Learnt from: andybellenie
Repo: openfrontio/OpenFrontIO PR: 3509
File: src/server/GameServer.ts:667-680
Timestamp: 2026-03-24T23:21:38.781Z
Learning: In openfrontio/OpenFrontIO, the clan-tag privacy feature in `src/server/GameServer.ts` (`gameInfoForClient()`, PR `#3509`) is intentionally a partial mitigation against tag sniping, not a complete fix. A larger clan system rework is in development. The bypass via rejoin with a different clanTag (identityUpdate path in Worker.ts) is a known, accepted limitation. Do not flag this as a critical issue in future reviews of this file.
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 3246
File: src/core/execution/nation/NationNukeBehavior.ts:814-832
Timestamp: 2026-02-19T18:07:39.005Z
Learning: In src/core/execution/nation/NationNukeBehavior.ts, the clustering logic in maybeDestroyEnemySam() intentionally checks the arrival time of the first bomb from a silo rather than the last when evaluating if a silo fits within maxTotalArrivalSpread. This slight underestimate is by design: the window is already conservative (half the SAM cooldown = 37 of 75 ticks), staggerInterval is small (1-5 ticks), extra bombs provide margin, and the approach includes borderline silos that work fine in practice. The design should not be "fixed" to use the last bomb's arrival unless SAM timing changes significantly.
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:770-795
Timestamp: 2025-10-27T09:47:26.395Z
Learning: In src/core/execution/FakeHumanExecution.ts, the selectSteamrollStopTarget() method intentionally allows MIRV targeting when secondHighest city count is 0 (e.g., nuclear endgame scenarios where structures are destroyed). This is valid game design—"if you can afford it, you're good to go"—and should not be flagged as requiring a guard condition.
Learnt from: BrewedCoffee
Repo: openfrontio/OpenFrontIO PR: 1957
File: src/core/execution/PlayerExecution.ts:40-52
Timestamp: 2025-08-29T16:16:11.309Z
Learning: In OpenFrontIO PlayerExecution.ts, when Defense Posts are captured due to tile ownership changes, the intended behavior is to first call u.decreaseLevel() to downgrade them, then still transfer them to the capturing player via captureUnit(). This is not a bug - Defense Posts should be both downgraded and captured, not destroyed outright.
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 3540
File: src/client/graphics/layers/NameLayer.ts:126-129
Timestamp: 2026-03-30T19:14:20.158Z
Learning: In `src/client/graphics/layers/NameLayer.ts` (OpenFrontIO), the `emojiTemplate` div in `NameLayer` intentionally uses `position: absolute`, `top: 50%`, and `transform: translateY(-50%)`. Emoji badges are designed to appear behind the name/troops overlay, not as inline items in the `iconsDiv` flex row. Do not flag this absolute positioning as a layout bug in future reviews.
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.
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 0
File: :0-0
Timestamp: 2026-02-01T16:36:52.217Z
Learning: In this TypeScript game codebase, UI/layer updates should be wall-clock throttled (independent of simulation speed). Do not use tick-modulus gating like `if (game.ticks()%N===0)` inside `layer.tick()` methods. Instead, layers should expose a `getTickIntervalMs()` method and let the renderer schedule ticks based on wall-clock time.
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.
Learnt from: evanpelle
Repo: openfrontio/OpenFrontIO PR: 3114
File: src/client/graphics/layers/AttacksDisplay.ts:64-77
Timestamp: 2026-02-10T05:02:28.715Z
Learning: In the OpenFrontIO codebase, GameUpdateType maps always have an array value for each key (never undefined). When iterating, you can safely use: for (const event of updates[GameUpdateType.X]) without optional chaining or guards. Treat updates as a Record<GameUpdateType, any[]> and rely on its guaranteed array values. If you modify the type, ensure the guarantee remains (e.g., initialize missing keys to []) to preserve safe iteration.
Learnt from: wozniakpl
Repo: openfrontio/OpenFrontIO PR: 3317
File: src/client/InputHandler.ts:344-350
Timestamp: 2026-03-07T11:35:24.071Z
Learning: Ensure ghost preservation logic for special units (AtomBomb and HydrogenBomb) is implemented via shouldPreserveGhostAfterBuild(unitType) and that ghost removal is conditional on !shouldPreserveGhostAfterBuild(unitType) within createStructure. Confirm that both mouse (MouseUpEvent) and keyboard (ConfirmGhostStructureEvent) confirmation paths go through requestConfirmStructure → createStructure, and that Enter/NumpadEnter confirmations do not break the locked-nuke multi-placement since the ghost should be preserved by design. Add tests to cover shouldPreserveGhostAfterBuild returning true for AtomBomb/HydrogenBomb and verify that ghost removal only occurs for other units. Applies to files in the repository OpenFrontIO (src/client/InputHandler.ts and src/client/graphics/layers/StructureIconsLayer.ts).
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 3540
File: src/client/graphics/layers/NameLayer.ts:84-86
Timestamp: 2026-03-30T18:41:35.016Z
Learning: In graphics layers, `redraw()` should be intentionally a no-op (or have an empty/limited body) when the layer is DOM-based and does not depend on Canvas/WebGL context (e.g., it does not require reinitializing GPU resources after context loss). The `redraw()` hook should only contain reinitialization logic for Canvas/WebGL layers that must restore GPU resources on context restoration.
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 3549
File: tests/pathfinding/playground/server.ts:246-250
Timestamp: 2026-03-31T17:02:41.798Z
Learning: When migrating/implementing Express 5+ code, use the Express 5 `app.listen(PORT, (error) => { ... })` callback signature: Express 5 passes listen errors (e.g., `EADDRINUSE`) as the first argument to the callback. Handle the error inside that callback instead of relying on Node.js `http.Server`’s `'error'` event semantics.
Learnt from: baculinivan-web
Repo: openfrontio/OpenFrontIO PR: 3688
File: src/client/graphics/layers/GameRightSidebar.ts:235-248
Timestamp: 2026-04-22T08:54:50.345Z
Learning: In the OpenFrontIO/OpenFrontIO repository, do not raise review findings for standard web accessibility (a11y) keyboard/semantic issues that would normally require button semantics (e.g., using clickable `<div>` elements instead of `<button>`, or similar keyboard accessibility/ARIA patterns). This repository intentionally does not enforce those semantic/keyboard-accessibility conventions because gameplay requires touch or mouse input and is not operable via keyboard alone.
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
luctrate
changed the title
evanpelle
reviewed
Apr 23, 2026
Collaborator
evanpelle
left a comment
evanpelle
left a comment
There was a problem hiding this comment.
Please add screenshots
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.
Resolves #3445
Description:
Adds a persistent orange warning banner above the troop bar when a player's troops exceed 80% of max capacity in team games with troop donation enabled, encouraging players to send excess troops to teammates instead of wasting capacity (Option B from #3445).
Changes
armyLimitWarningThreshold()(80% default)tick(), newrenderArmyLimitWarning()method, placed above troop bar in both desktop and mobile layoutscontrol_panel.army_limit_warningtranslation keyEdge cases handled
Math.max(maxTroops, 1)Please complete the following:
UI Change only, pls comment if tests are still required.
Please put your Discord username so you can be contacted if a bug or regression is found:
luctrate_98321