Feature/troop breakdown visualization with missions #2386
Conversation
- Add max troops breakdown visualization showing territory vs cities contribution - Display breakdown as a progress bar that fills based on current troops percentage - Use player's territory color (lighter) for territory contribution - Use darkened territory color for cities contribution - Add translation keys for territory and cities labels - Bar dynamically adjusts width from 0-100% based on current/max troops ratio
- Remove unused translation keys (purely visual feature) - Fix bar proportions using flex-grow for smooth display - Territory and cities sections now properly proportional - Removed debug console.log
…city/_cityCapacity
Address CodeRabbit feedback by creating shared methods in Config interface: - Add baseTerritoryCapacity(player) and baseCityCapacity(player) methods - Implement in DefaultConfig.ts, refactor maxTroops() to use them - Update ControlPanel.ts to call new methods instead of duplicating formula - Eliminates code duplication and ensures single source of truth - All formulas now centralized in DefaultConfig for easier maintenance This addresses the concern about formula drift between ControlPanel and DefaultConfig. Tests: 316/317 passing (1 pre-existing failure unrelated to changes)
- Add try-catch with console.warn for capacity calculation failures - Add ARIA attributes (role, aria-valuenow, aria-valuemin, aria-valuemax, aria-label) - Add tooltip showing territory and cities capacity values - Addresses CodeRabbit nitpick comments about error handling and accessibility
When an error occurs during capacity calculation, all three state variables (_territoryCapacity, _cityCapacity, _maxTroops) should be reset to 0 to prevent inconsistent UI state where max capacity is non-zero but breakdown values are zero.
WalkthroughControlPanel now tracks territory/city capacity and troops-on-mission, uses new Config capacity & estimated-allocation APIs and Player.totalUnitLevels, and renders a segmented max-troops breakdown bar with ARIA attributes. DefaultConfig adds capacity and proportional-estimate helpers; Player gains totalUnitLevels(type). Changes
Sequence Diagram(s)sequenceDiagram
participant CP as ControlPanel
participant CFG as Config / DefaultConfig
participant P as Player
CP->>P: read outgoingAttacks(), units(type=TRANSPORT)
P-->>CP: outgoing attacks, transport units
CP->>CFG: maxTroopsTerritory(player)
CFG->>P: query owned tiles / counts
P-->>CFG: owned tile counts
CFG-->>CP: territoryCapacity
CP->>CFG: maxTroopsCity(player)
CFG->>P: totalUnitLevels(CITY)
P-->>CFG: total city unit levels
CFG-->>CP: cityCapacity
CP->>CFG: estimatedTroopsTerritory(player)
CFG-->>CP: territory troop estimate
CP->>CP: compute totals, troopsOnMission, bar widths
CP-->>CP: render segmented bar (territory / city / missions) with ARIA
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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: 1
🧹 Nitpick comments (1)
src/client/graphics/layers/ControlPanel.ts (1)
101-107: Consider the type safety trade-off when querying EventsDisplay.The DOM query with
anycast works but loses type safety. This is acceptable for sibling Lit components, though you could extract an interface if the coupling grows.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/client/graphics/layers/ControlPanel.ts(4 hunks)src/client/graphics/layers/EventsDisplay.ts(1 hunks)src/core/configuration/Config.ts(1 hunks)src/core/configuration/DefaultConfig.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
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.
📚 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/core/configuration/Config.tssrc/core/configuration/DefaultConfig.tssrc/client/graphics/layers/ControlPanel.ts
📚 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/Config.tssrc/core/configuration/DefaultConfig.tssrc/client/graphics/layers/ControlPanel.ts
📚 Learning: 2025-05-31T18:15:03.445Z
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.
Applied to files:
src/core/configuration/Config.tssrc/core/configuration/DefaultConfig.tssrc/client/graphics/layers/ControlPanel.tssrc/client/graphics/layers/EventsDisplay.ts
📚 Learning: 2025-05-19T06:00:38.007Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 784
File: src/core/game/StatsImpl.ts:125-134
Timestamp: 2025-05-19T06:00:38.007Z
Learning: In StatsImpl.ts, unused parameters in boat/stats-related methods are intentionally kept for future use and shouldn't be removed.
Applied to files:
src/core/configuration/Config.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/Config.tssrc/core/configuration/DefaultConfig.ts
📚 Learning: 2025-10-21T20:06:04.823Z
Learnt from: Saphereye
Repo: openfrontio/OpenFrontIO PR: 2233
File: src/client/HostLobbyModal.ts:891-891
Timestamp: 2025-10-21T20:06:04.823Z
Learning: For the HumansVsNations game mode in `src/client/HostLobbyModal.ts` and related files, the implementation strategy is to generate all nations and adjust their strength for balancing, rather than limiting lobby size based on the number of available nations on the map.
Applied to files:
src/core/configuration/Config.tssrc/core/configuration/DefaultConfig.tssrc/client/graphics/layers/ControlPanel.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/Config.tssrc/core/configuration/DefaultConfig.tssrc/client/graphics/layers/ControlPanel.tssrc/client/graphics/layers/EventsDisplay.ts
📚 Learning: 2025-06-22T21:51:14.990Z
Learnt from: devalnor
Repo: openfrontio/OpenFrontIO PR: 1248
File: src/client/graphics/layers/TerritoryInfoLayer.ts:20-20
Timestamp: 2025-06-22T21:51:14.990Z
Learning: In TerritoryInfoLayer.ts, the highlightedTerritory field uses both null and undefined intentionally: undefined represents initial state or inactive layer (Ctrl released), while null represents active layer with no territory being highlighted at cursor position. This distinction is important for proper state change detection.
Applied to files:
src/client/graphics/layers/ControlPanel.ts
🧬 Code graph analysis (3)
src/core/configuration/Config.ts (2)
src/core/game/GameView.ts (2)
player(596-602)PlayerView(181-446)src/core/game/Game.ts (1)
Player(525-661)
src/core/configuration/DefaultConfig.ts (2)
src/core/game/GameView.ts (2)
player(596-602)PlayerView(181-446)src/core/game/Game.ts (1)
Player(525-661)
src/client/graphics/layers/ControlPanel.ts (2)
src/core/game/GameView.ts (1)
config(647-649)src/core/game/GameImpl.ts (1)
config(336-338)
🔇 Additional comments (6)
src/client/graphics/layers/EventsDisplay.ts (1)
656-678: LGTM! Clean implementation of troops-on-mission calculation.The method correctly sums troops from all mission types using reduce, and the pattern is consistent across all three sources. Empty arrays are handled naturally by reduce's initial value.
src/core/configuration/Config.ts (1)
121-122: LGTM! Interface additions follow existing patterns.The method signatures are consistent with the Config interface conventions, using the same
Player | PlayerViewunion type as other player-related methods.src/core/configuration/DefaultConfig.ts (2)
825-836: LGTM! Capacity methods centralize the calculation logic.The extracted methods successfully centralize the territory and city capacity formulas, making them reusable by both the maxTroops calculation and the UI breakdown display. The implementation correctly handles both
PlayerandPlayerViewtypes.
838-862: LGTM! maxTroops refactored to use the new base capacity methods.The refactor maintains the original formula while improving maintainability by delegating to the extracted base capacity methods. This ensures consistency between the internal maxTroops calculation and the UI's capacity breakdown.
src/client/graphics/layers/ControlPanel.ts (2)
109-124: LGTM! Defensive error handling for capacity calculations.The try-catch with fallback ensures the UI remains functional even if capacity calculations fail. The console warning aids debugging without disrupting the user experience.
218-264: LGTM! Accessible and visually clear capacity breakdown.The progress bar implementation includes proper ARIA attributes, handles zero-capacity edge cases with conditional rendering, and provides clear visual distinction between territory, city, and mission segments using opacity and color.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/core/game/PlayerImpl.ts (1)
265-269: LGTM! Implementation is correct and handles edge cases.The method correctly sums unit levels with the initial value
0handling the empty array case. The map-then-reduce approach is readable and follows existing patterns in the codebase.For a minor efficiency gain, you could combine map and reduce into a single pass:
totalUnitLevels(type: UnitType): number { - return this.units(type) - .map((unit) => unit.level()) - .reduce((a, b) => a + b, 0); + return this.units(type).reduce((sum, unit) => sum + unit.level(), 0); }This avoids creating an intermediate array, though the performance difference is negligible for typical unit counts.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/core/configuration/DefaultConfig.ts(1 hunks)src/core/game/Game.ts(1 hunks)src/core/game/PlayerImpl.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
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.
📚 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-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-05-31T18:15:03.445Z
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.
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-10-21T20:06:04.823Z
Learnt from: Saphereye
Repo: openfrontio/OpenFrontIO PR: 2233
File: src/client/HostLobbyModal.ts:891-891
Timestamp: 2025-10-21T20:06:04.823Z
Learning: For the HumansVsNations game mode in `src/client/HostLobbyModal.ts` and related files, the implementation strategy is to generate all nations and adjust their strength for balancing, rather than limiting lobby size based on the number of available nations on the map.
Applied to files:
src/core/configuration/DefaultConfig.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/core/configuration/DefaultConfig.ts
🧬 Code graph analysis (2)
src/core/game/Game.ts (1)
src/core/game/PlayerImpl.ts (1)
type(201-203)
src/core/configuration/DefaultConfig.ts (2)
src/core/game/GameView.ts (2)
player(596-602)PlayerView(181-446)src/core/game/Game.ts (1)
Player(525-662)
🔇 Additional comments (4)
src/core/game/Game.ts (1)
571-571: LGTM! Clean interface addition.The new
totalUnitLevelsmethod is well-positioned in the Units section and follows the existing interface patterns. The signature is clear and type-safe.src/core/configuration/DefaultConfig.ts (3)
825-827: LGTM! Clear extraction of territory capacity formula.The formula correctly computes capacity from tiles owned with reasonable edge case behavior (zero tiles yields 100,000 base capacity). Extracting this into a dedicated method improves code organization and makes it reusable for UI components.
829-831: LGTM! Clean city capacity calculation.The method correctly sums city levels using the new
totalUnitLevelsmethod and scales by the city troop increase constant. Handles the no-cities case correctly by returning zero.
833-857: LGTM! Excellent refactoring that preserves game logic.The refactored
maxTroopsmethod now uses the extracted capacity helpers while preserving all existing logic paths:
- Infinite troops path unchanged for humans (1,000,000,000)
- Bot and difficulty multipliers remain intact
- The formula
baseTerritoryCapacity + baseCityCapacityis equivalent to the previous inline calculationThis improves maintainability by centralizing capacity calculations and makes them accessible for UI breakdowns (as mentioned in the PR objectives).
- Remove getTroopsOnMission() method from EventsDisplay (no longer needed) - Calculate troops on mission directly in ControlPanel from PlayerView - Add UnitType import to ControlPanel - Cap progress bar width at 100% with Math.min() to prevent overflow - Follows existing codebase pattern - ControlPanel only references GameView/PlayerView - Cleaner implementation without cross-component DOM queries
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/client/graphics/layers/ControlPanel.ts (3)
104-116: Simplify the attack troops calculation.The code filters outgoing attacks by
targetID !== 0andtargetID === 0, sums each group separately, then adds them together. Since both groups are summed and combined anyway, you can remove the filtering and use a single reduce:Apply this diff to simplify:
- const attackTroops = outgoingAttacks - .filter((a) => a.targetID !== 0) - .reduce((sum, attack) => sum + attack.troops, 0); - - const landAttackTroops = outgoingAttacks - .filter((a) => a.targetID === 0) - .reduce((sum, attack) => sum + attack.troops, 0); + const attackTroops = outgoingAttacks.reduce( + (sum, attack) => sum + attack.troops, + 0, + ); const boatTroops = player .units(UnitType.TransportShip) .reduce((sum, boat) => sum + boat.troops(), 0); - this._troopsOnMission = attackTroops + landAttackTroops + boatTroops; + this._troopsOnMission = attackTroops + boatTroops;
248-266: Extract color lookup to avoid redundant calls.The territory color is retrieved twice from
this.game?.myPlayer()?.territoryColor()with different fallback colors ("rgb(147, 51, 234)" for territory, "rgb(59, 130, 246)" for city). This is redundant and the inconsistent fallbacks seem unintentional.Consider extracting the color lookup once before the HTML template:
// In the render() method, before the return statement: const playerColor = this.game?.myPlayer()?.territoryColor()?.toRgbString() ?? "rgb(147, 51, 234)";Then use
${playerColor}in both places (lines 251-254 and 260-263).
122-125: Consider rounding_maxTroopsto match the sum of capacity segments.The independent
Math.round()calls onbaseTerritoryCapacityandbaseCityCapacitycan produce a sum that differs slightly from_maxTroops(which is not rounded). SincebaseTerritoryCapacityreturns decimals due toMath.pow(), the mismatch is real: for example,round(100.6) + round(50.6) = 152while the true sum is151.2.Although the flex-grow proportions handle this gracefully (producing <0.1% visual difference), set
_maxTroops = Math.round(config.maxTroops(player))for mathematical consistency and clearer intent—the display values and bar proportions will then align perfectly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/client/graphics/layers/ControlPanel.ts(5 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
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.
📚 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/client/graphics/layers/ControlPanel.ts
📚 Learning: 2025-05-31T18:15:03.445Z
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.
Applied to files:
src/client/graphics/layers/ControlPanel.ts
📚 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/client/graphics/layers/ControlPanel.ts
📚 Learning: 2025-06-22T21:51:14.990Z
Learnt from: devalnor
Repo: openfrontio/OpenFrontIO PR: 1248
File: src/client/graphics/layers/TerritoryInfoLayer.ts:20-20
Timestamp: 2025-06-22T21:51:14.990Z
Learning: In TerritoryInfoLayer.ts, the highlightedTerritory field uses both null and undefined intentionally: undefined represents initial state or inactive layer (Ctrl released), while null represents active layer with no territory being highlighted at cursor position. This distinction is important for proper state change detection.
Applied to files:
src/client/graphics/layers/ControlPanel.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/ControlPanel.ts
📚 Learning: 2025-10-21T20:06:04.823Z
Learnt from: Saphereye
Repo: openfrontio/OpenFrontIO PR: 2233
File: src/client/HostLobbyModal.ts:891-891
Timestamp: 2025-10-21T20:06:04.823Z
Learning: For the HumansVsNations game mode in `src/client/HostLobbyModal.ts` and related files, the implementation strategy is to generate all nations and adjust their strength for balancing, rather than limiting lobby size based on the number of available nations on the map.
Applied to files:
src/client/graphics/layers/ControlPanel.ts
📚 Learning: 2025-08-29T16:16:11.309Z
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.
Applied to files:
src/client/graphics/layers/ControlPanel.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/layers/ControlPanel.ts
🧬 Code graph analysis (1)
src/client/graphics/layers/ControlPanel.ts (2)
src/core/game/PlayerImpl.ts (1)
outgoingAttacks(1178-1180)src/core/game/GameView.ts (3)
outgoingAttacks(291-293)player(596-602)config(647-649)
🔇 Additional comments (4)
src/client/graphics/layers/ControlPanel.ts (4)
5-5: LGTM!The UnitType import and new state fields are properly initialized with safe defaults.
Also applies to: 26-30, 44-45
239-245: Width calculation correctly caps overflow.The use of
Math.min()to cap the progress bar width at 100% properly addresses the previous review concern about visual overflow when troops exceed max capacity.
269-274: LGTM!The mission troops visualization is well-implemented with appropriate conditional rendering and clear visual distinction using red coloring.
228-236: LGTM!The progress bar includes proper ARIA attributes for accessibility. The
aria-valuenow,aria-valuemin,aria-valuemax, and descriptivearia-labelprovide screen readers with complete context about troop capacity.
- Simplify attack troops calculation by removing redundant filtering - Round max troops for consistency with capacity segments - Extract player color lookup to avoid redundant calls
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/client/graphics/layers/ControlPanel.ts (2)
117-130: LGTM! Error handling is defensive and appropriate.The try-catch guards against unexpected config errors and provides a safe fallback. Resetting to zero ensures consistent state on error.
Optional: Consider preserving previous values on error instead of resetting to zero, which would keep the last valid display visible. However, the current approach (reset to zero) is safer and simpler.
162-164: LGTM! Progress bar rendering is well-implemented.The visual breakdown correctly uses flex-grow to show:
- Overall utilization (bar width)
- Available vs. mission troops (primary split)
- Territory vs. city capacity contributions (secondary split within available)
The flex-grow math works correctly: if you have 120 available troops and 30 on mission with a 150 max (100 territory + 50 city), the bar shows 100% width with available taking 80% (subdivided 2:1 for territory:city) and mission taking 20%.
ARIA attributes are appropriate for accessibility.
Optional micro-optimizations:
Cache player color in tick(): Currently computed in render() on every update. Moving to tick() would be more consistent with other computed values like
_troops.// In state declarations @state() private _playerColor: string = "rgb(147, 51, 234)"; // In tick() this._playerColor = player.territoryColor()?.toRgbString() ?? "rgb(147, 51, 234)"; // In render() const playerColor = this._playerColor;Consider adding aria-describedby: Could enhance accessibility by describing what the colored segments represent (territory capacity vs. city capacity).
Also applies to: 228-271
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/client/graphics/layers/ControlPanel.ts(6 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
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.
📚 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/client/graphics/layers/ControlPanel.ts
📚 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/client/graphics/layers/ControlPanel.ts
📚 Learning: 2025-05-31T18:15:03.445Z
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.
Applied to files:
src/client/graphics/layers/ControlPanel.ts
📚 Learning: 2025-06-22T21:51:14.990Z
Learnt from: devalnor
Repo: openfrontio/OpenFrontIO PR: 1248
File: src/client/graphics/layers/TerritoryInfoLayer.ts:20-20
Timestamp: 2025-06-22T21:51:14.990Z
Learning: In TerritoryInfoLayer.ts, the highlightedTerritory field uses both null and undefined intentionally: undefined represents initial state or inactive layer (Ctrl released), while null represents active layer with no territory being highlighted at cursor position. This distinction is important for proper state change detection.
Applied to files:
src/client/graphics/layers/ControlPanel.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/ControlPanel.ts
📚 Learning: 2025-10-21T20:06:04.823Z
Learnt from: Saphereye
Repo: openfrontio/OpenFrontIO PR: 2233
File: src/client/HostLobbyModal.ts:891-891
Timestamp: 2025-10-21T20:06:04.823Z
Learning: For the HumansVsNations game mode in `src/client/HostLobbyModal.ts` and related files, the implementation strategy is to generate all nations and adjust their strength for balancing, rather than limiting lobby size based on the number of available nations on the map.
Applied to files:
src/client/graphics/layers/ControlPanel.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/layers/ControlPanel.ts
🧬 Code graph analysis (1)
src/client/graphics/layers/ControlPanel.ts (3)
src/core/game/PlayerImpl.ts (1)
outgoingAttacks(1178-1180)src/core/game/GameView.ts (3)
outgoingAttacks(291-293)player(596-602)config(647-649)src/core/game/GameImpl.ts (2)
player(472-478)config(336-338)
🔇 Additional comments (2)
src/client/graphics/layers/ControlPanel.ts (2)
5-5: LGTM! Clean state additions.The new state fields and import are straightforward and follow existing patterns. Defaults of 0 are sensible for numeric state.
Also applies to: 26-30, 44-45
101-113: LGTM! Troops on mission calculation is correct.The logic correctly sums troops from outgoing attacks and transport ships. The reduce pattern is clean and idiomatic TypeScript.
- Cache player color in tick() instead of computing in render() - Use theme.neutralColor() as fallback instead of hardcoded value - Add aria-describedby for better screen reader accessibility - Remove unnecessary inline comments per codebase style
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/client/graphics/layers/ControlPanel.ts (1)
117-129: Consider logging more error context.The try/catch provides a safe fallback, but the warning message could include more details (e.g., the exception message or stack) to aid debugging if these config methods unexpectedly throw.
} catch (e) { - console.warn("Failed to calculate capacity breakdown:", e); + console.warn("Failed to calculate capacity breakdown:", e instanceof Error ? e.message : e); this._territoryCapacity = 0; this._cityCapacity = 0; this._maxTroops = 0; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/client/graphics/layers/ControlPanel.ts(6 hunks)
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
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.
📚 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/client/graphics/layers/ControlPanel.ts
📚 Learning: 2025-06-22T21:51:14.990Z
Learnt from: devalnor
Repo: openfrontio/OpenFrontIO PR: 1248
File: src/client/graphics/layers/TerritoryInfoLayer.ts:20-20
Timestamp: 2025-06-22T21:51:14.990Z
Learning: In TerritoryInfoLayer.ts, the highlightedTerritory field uses both null and undefined intentionally: undefined represents initial state or inactive layer (Ctrl released), while null represents active layer with no territory being highlighted at cursor position. This distinction is important for proper state change detection.
Applied to files:
src/client/graphics/layers/ControlPanel.ts
📚 Learning: 2025-05-31T18:15:03.445Z
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.
Applied to files:
src/client/graphics/layers/ControlPanel.ts
📚 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/client/graphics/layers/ControlPanel.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/ControlPanel.ts
📚 Learning: 2025-10-21T20:06:04.823Z
Learnt from: Saphereye
Repo: openfrontio/OpenFrontIO PR: 2233
File: src/client/HostLobbyModal.ts:891-891
Timestamp: 2025-10-21T20:06:04.823Z
Learning: For the HumansVsNations game mode in `src/client/HostLobbyModal.ts` and related files, the implementation strategy is to generate all nations and adjust their strength for balancing, rather than limiting lobby size based on the number of available nations on the map.
Applied to files:
src/client/graphics/layers/ControlPanel.ts
📚 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/layers/ControlPanel.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/client/graphics/layers/ControlPanel.ts
📚 Learning: 2025-08-29T16:16:11.309Z
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.
Applied to files:
src/client/graphics/layers/ControlPanel.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/layers/ControlPanel.ts
🧬 Code graph analysis (1)
src/client/graphics/layers/ControlPanel.ts (3)
src/core/game/PlayerImpl.ts (1)
outgoingAttacks(1178-1180)src/core/game/GameView.ts (3)
outgoingAttacks(291-293)player(596-602)config(647-649)src/core/game/GameImpl.ts (2)
player(472-478)config(336-338)
🔇 Additional comments (6)
src/client/graphics/layers/ControlPanel.ts (6)
5-5: LGTM: UnitType import added for transport ship filtering.The import is used correctly at line 112 to filter units by type.
26-48: LGTM: New reactive state fields properly declared.The fields track capacity breakdown and mission status, with appropriate initializers and
@state()decorators for reactive rendering.
131-133: LGTM: Color assignment with safe fallback.Optional chaining and fallback to neutral color ensure the visualization always has a valid color value.
165-165: LGTM: Local variable improves template readability.Extracting
_playerColorto a local variable avoids repetition in the template below.
229-238: Excellent accessibility implementation.Proper ARIA progressbar attributes, descriptive labels, and screen-reader-only text ensure the visualization is accessible. The aria-valuenow correctly matches the width calculation.
Also applies to: 274-278
104-115: Code correctly counts troop-carrying units. No changes needed.Verification confirms that
TransportShipis the only unit type that carries troops. The game design includes only two boat types (TransportShip and TradeShip), and TradeShip is used exclusively for trading resources, not troop transport. No land or air transport variants exist. The logic in lines 104-115 correctly sums troops from outgoing attacks and transport ships—the only two sources of troops on a mission.
- Rename baseTerritoryCapacity() to maxTroopsTerritory() - Rename baseCityCapacity() to maxTroopsCity() - Add estimatedTroopsTerritory() and estimatedTroopsCity() helpers - Update ControlPanel UI to use new estimation methods for troop breakdown visualization - Use _troopsTerritory and _troopsCity state instead of _territoryCapacity and _cityCapacity - Update Config interface and DefaultConfig implementation
binmogit
force-pushed
the
feature/troop-breakdown-visualization-with-missions
branch
from
fc88e8a to
8608dfc
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/configuration/Config.ts (1)
833-853: Fix rounding inconsistency in proportional estimation.Both estimation methods independently round their results, which can cause
estimatedTroopsTerritory + estimatedTroopsCity ≠ player.troops()due to rounding error accumulation.Example:
- maxTroops = 100 (territory: 50, city: 50)
- currentTroops = 51
- territoryEstimate = round(0.5 × 51) = 26
- cityEstimate = round(0.5 × 51) = 26
- Sum = 52 ≠ 51
This mismatch will cause visual glitches in the ControlPanel progress bar where flex-grow values don't align.
Apply this diff to ensure the estimates sum exactly to current troops:
estimatedTroopsTerritory(player: Player | PlayerView): number { const maxTroops = this.maxTroops(player); if (maxTroops === 0) return 0; const maxTroopsTerritory = this.maxTroopsTerritory(player); const currentTroops = player.troops(); // Proportionally attribute current troops based on territory's share of max capacity return Math.round((maxTroopsTerritory / maxTroops) * currentTroops); } estimatedTroopsCity(player: Player | PlayerView): number { const maxTroops = this.maxTroops(player); if (maxTroops === 0) return 0; - const maxTroopsCity = this.maxTroopsCity(player); const currentTroops = player.troops(); + const territoryTroops = this.estimatedTroopsTerritory(player); - // Proportionally attribute current troops based on cities' share of max capacity - return Math.round((maxTroopsCity / maxTroops) * currentTroops); + // Calculate as residual to ensure territory + city = current troops exactly + return Math.max(0, currentTroops - territoryTroops); }
🧹 Nitpick comments (1)
src/core/configuration/DefaultConfig.ts (1)
833-853: Consider acceptingmaxTroopsas an optional parameter to avoid redundant calculations.Both
estimatedTroopsTerritoryandestimatedTroopsCitycallthis.maxTroops(player), which recalculates the max capacity. Since callers likeControlPanel.tick()already computemaxTroopsonce (line 127), passing it as an optional parameter would eliminate redundant work:estimatedTroopsTerritory(player: Player | PlayerView, maxTroops?: number): number { const max = maxTroops ?? this.maxTroops(player); if (max === 0) return 0; // ... rest of logic }This isn't critical, but it would optimize the common path where all four methods (
maxTroopsTerritory,maxTroopsCity,maxTroops, and the two estimated methods) are called in sequence.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/client/graphics/layers/ControlPanel.ts(6 hunks)src/core/configuration/Config.ts(1 hunks)src/core/configuration/DefaultConfig.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
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.
📚 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.tssrc/core/configuration/Config.tssrc/client/graphics/layers/ControlPanel.ts
📚 Learning: 2025-05-31T18:15:03.445Z
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.
Applied to files:
src/core/configuration/DefaultConfig.tssrc/core/configuration/Config.tssrc/client/graphics/layers/ControlPanel.ts
📚 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.tssrc/core/configuration/Config.tssrc/client/graphics/layers/ControlPanel.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.tssrc/core/configuration/Config.ts
📚 Learning: 2025-10-21T20:06:04.823Z
Learnt from: Saphereye
Repo: openfrontio/OpenFrontIO PR: 2233
File: src/client/HostLobbyModal.ts:891-891
Timestamp: 2025-10-21T20:06:04.823Z
Learning: For the HumansVsNations game mode in `src/client/HostLobbyModal.ts` and related files, the implementation strategy is to generate all nations and adjust their strength for balancing, rather than limiting lobby size based on the number of available nations on the map.
Applied to files:
src/core/configuration/DefaultConfig.tssrc/core/configuration/Config.ts
📚 Learning: 2025-05-19T06:00:38.007Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 784
File: src/core/game/StatsImpl.ts:125-134
Timestamp: 2025-05-19T06:00:38.007Z
Learning: In StatsImpl.ts, unused parameters in boat/stats-related methods are intentionally kept for future use and shouldn't be removed.
Applied to files:
src/core/configuration/Config.ts
📚 Learning: 2025-06-22T21:51:14.990Z
Learnt from: devalnor
Repo: openfrontio/OpenFrontIO PR: 1248
File: src/client/graphics/layers/TerritoryInfoLayer.ts:20-20
Timestamp: 2025-06-22T21:51:14.990Z
Learning: In TerritoryInfoLayer.ts, the highlightedTerritory field uses both null and undefined intentionally: undefined represents initial state or inactive layer (Ctrl released), while null represents active layer with no territory being highlighted at cursor position. This distinction is important for proper state change detection.
Applied to files:
src/client/graphics/layers/ControlPanel.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/ControlPanel.ts
📚 Learning: 2025-06-22T05:48:19.241Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 786
File: src/client/TerritoryPatternsModal.ts:337-338
Timestamp: 2025-06-22T05:48:19.241Z
Learning: In src/client/TerritoryPatternsModal.ts, the bit shifting operators (<<) used in coordinate calculations with decoder.getScale() are intentional and should not be changed to multiplication. The user scottanderson confirmed this is functioning as intended.
Applied to files:
src/client/graphics/layers/ControlPanel.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/layers/ControlPanel.ts
🧬 Code graph analysis (3)
src/core/configuration/DefaultConfig.ts (2)
src/core/game/GameView.ts (2)
player(596-602)PlayerView(181-446)src/core/game/Game.ts (1)
Player(525-662)
src/core/configuration/Config.ts (2)
src/core/game/GameView.ts (2)
player(596-602)PlayerView(181-446)src/core/game/Game.ts (1)
Player(525-662)
src/client/graphics/layers/ControlPanel.ts (2)
src/core/game/PlayerImpl.ts (1)
outgoingAttacks(1178-1180)src/core/game/GameView.ts (3)
outgoingAttacks(291-293)player(596-602)config(647-649)
🔇 Additional comments (13)
src/core/configuration/Config.ts (8)
121-127: LGTM! Clean interface additions with clear documentation.The four new methods provide a clean breakdown of troop capacity by source (territory vs. city) and estimated allocation. The comment on lines 124-125 helpfully clarifies that these are proportional estimates, not actual tracking, which sets appropriate expectations for callers.
825-831: LGTM! Clean extraction of capacity formulas.The territory and city capacity methods correctly extract the formulas that were previously inline in
maxTroops. UsingtotalUnitLevels(UnitType.City)is the right approach for city-based capacity.
855-879: LGTM! Refactored to use composition while preserving behavior.The refactored
maxTroopsmethod now uses the extractedmaxTroopsTerritoryandmaxTroopsCitymethods, maintaining the same formula while improving modularity. The infinite troops and difficulty modifiers are correctly preserved.
5-5: LGTM! Added necessary import for unit type filtering.The
UnitTypeimport is correctly added to support filtering transport ships when calculating troops on mission.
26-54: LGTM! Well-organized state additions for capacity breakdown.The new state fields are clearly named and initialized with safe defaults:
_maxTroopsTerritoryand_maxTroopsCityfor capacity values_troopsTerritoryand_troopsCityfor estimated allocations_troopsOnMissionfor troops in attacks/transport_playerColorfor consistent stylingThe naming convention distinguishes capacity (_maxTroops*) from current allocations (_troops*), making the code easier to reason about.
109-122: LGTM! Correctly calculates troops on mission.The logic properly sums troops from two sources:
- Outgoing attacks via
player.outgoingAttacks()- Transport ships via
player.units(UnitType.TransportShip)Both use the correct APIs from PlayerView and accumulate troops with reduce operations.
123-143: LGTM! Defensive error handling with appropriate fallbacks.The try-catch block properly handles potential errors from config method calls and resets all capacity-related state to zero on failure. The warning log helps debugging without breaking the UI. Getting player color with a fallback to neutral color is also appropriate.
239-288: The review comment refers to the wrong file and wrong line numbers.The code being reviewed is in
src/client/graphics/layers/ControlPanel.ts(around lines 241-275), notsrc/core/configuration/Config.ts(which has only 216 lines). The empty code snippet confirms this mismatch.The flex-grow structure with Territory/City segments and ARIA attributes do exist and work as described, but in ControlPanel.ts. The rounding concern in DefaultConfig.ts (lines 841, 852) is valid—both
estimatedTroopsTerritoryandestimatedTroopsCityuse independentMath.round()calls, which can cause their sum to diverge slightly from_troops. However, this review comment cannot be verified against the stated file location.Likely an incorrect or invalid review comment.
src/core/configuration/DefaultConfig.ts (2)
825-831: LGTM! Clean extraction of capacity formulas.The territory and city capacity methods correctly extract the formulas that were previously inline. The territory formula uses the power-law scaling with tiles, and the city formula leverages the new
totalUnitLevelsAPI to sum city contributions.
855-879: LGTM! Refactored composition maintains original behavior.The
maxTroopsmethod now composes the newmaxTroopsTerritoryandmaxTroopsCitymethods instead of computing inline. The infinite-troops and Bot/FakeHuman scaling logic remains unchanged.src/client/graphics/layers/ControlPanel.ts (3)
26-54: LGTM! State properties align with capacity breakdown requirements.The new state fields track territory/city capacities, estimated troop allocations, troops on mission, and player color. The reactive
@state()decorators ensure the progress bar updates when values change.
109-144: LGTM! Troops-on-mission and capacity calculations are well-implemented.The tick logic correctly:
- Sums troops from outgoing attacks and transport ships (lines 110-121)
- Retrieves capacity values from config with proper rounding (lines 123-131)
- Wraps config calls in try-catch with fallback to zeros and warning (lines 123-139)
- Caches player color for rendering (lines 141-143)
The defensive error handling prevents crashes if config methods throw.
239-288: LGTM! Progress bar visualization addresses previous structural concerns.The nested flex structure now works correctly because
_troopsTerritoryand_troopsCity(lines 264, 270) are estimated allocations of current troops, not raw capacities. SinceestimatedTroopsTerritory + estimatedTroopsCity ≈ _troops, the children proportionally fill the parent.The implementation includes:
- Width capped to 100% (lines 253-257) ✓
- Comprehensive ARIA attributes for accessibility (lines 241-247) ✓
- Descriptive sr-only text (lines 284-288) ✓
- Visual distinction via opacity (0.6 vs 0.8) and red for mission troops ✓
Minor note: Due to rounding in
DefaultConfig.estimatedTroopsTerritory/City, the sum may differ from_troopsby ±1-2 units. This causes negligible visual impact but could be eliminated by computing one estimate as_troops - otherEstimateif pixel-perfect accuracy is desired.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/core/configuration/DefaultConfig.ts (1)
833-853: Proportional estimation may not reflect actual troop sources.The
estimatedTroopsTerritoryandestimatedTroopsCitymethods split current troops proportionally based on capacity ratios, not actual troop generation sources. For example, if territory capacity is 60% of max and current troops are 1000, the method returns 600—even if all troops actually came from cities.This is acceptable for UI visualization if users understand it shows capacity-weighted distribution rather than actual sourcing. The method names with "estimated" help signal this, but consider documenting the proportional nature if users expect source tracking.
If precise source tracking is needed in the future, you'd need to track territory vs. city troop generation separately throughout the game logic.
src/client/graphics/layers/ControlPanel.ts (1)
239-288: Minor visual inconsistency possible due to rounding.The flex-grow structure uses
_troopsfor the parent container and_troopsTerritory+_troopsCityfor the children. Since the estimation methods in DefaultConfig useMath.round(), the sum_troopsTerritory + _troopsCitymight not exactly equal_troops.For example:
_troops = 1000maxTroopsTerritory = 600,maxTroopsCity = 401,maxTroops = 1001_troopsTerritory = Math.round((600/1001) * 1000) = 599_troopsCity = Math.round((401/1001) * 1000) = 401- Sum = 1000 ✓ (happens to match)
But with different numbers:
_troops = 1000maxTroopsTerritory = 500,maxTroopsCity = 500,maxTroops = 1000- Both round to 500, sum = 1000 ✓
Actually, since the proportions are exact when maxTroops equals the sum of components, and we're rounding each independently, the rounding errors should be minimal (±1 troop). The flex-grow will handle this gracefully, but there might be a tiny visual mismatch where the parent width is slightly different from the sum of children widths.
This is acceptable for a visualization bar. If pixel-perfect rendering is needed, you could calculate one component as the remainder:
_troopsCity = _troops - _troopsTerritoryin the ControlPanel to guarantee the sum matches.
The ARIA attributes and conditional rendering for zero-value segments look excellent for accessibility and clean UI.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/client/graphics/layers/ControlPanel.ts(6 hunks)src/core/configuration/Config.ts(1 hunks)src/core/configuration/DefaultConfig.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/configuration/Config.ts
🧰 Additional context used
🧠 Learnings (10)
📚 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/client/graphics/layers/ControlPanel.tssrc/core/configuration/DefaultConfig.ts
📚 Learning: 2025-06-22T21:51:14.990Z
Learnt from: devalnor
Repo: openfrontio/OpenFrontIO PR: 1248
File: src/client/graphics/layers/TerritoryInfoLayer.ts:20-20
Timestamp: 2025-06-22T21:51:14.990Z
Learning: In TerritoryInfoLayer.ts, the highlightedTerritory field uses both null and undefined intentionally: undefined represents initial state or inactive layer (Ctrl released), while null represents active layer with no territory being highlighted at cursor position. This distinction is important for proper state change detection.
Applied to files:
src/client/graphics/layers/ControlPanel.ts
📚 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/client/graphics/layers/ControlPanel.tssrc/core/configuration/DefaultConfig.ts
📚 Learning: 2025-05-31T18:15:03.445Z
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.
Applied to files:
src/client/graphics/layers/ControlPanel.tssrc/core/configuration/DefaultConfig.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/ControlPanel.ts
📚 Learning: 2025-10-21T20:06:04.823Z
Learnt from: Saphereye
Repo: openfrontio/OpenFrontIO PR: 2233
File: src/client/HostLobbyModal.ts:891-891
Timestamp: 2025-10-21T20:06:04.823Z
Learning: For the HumansVsNations game mode in `src/client/HostLobbyModal.ts` and related files, the implementation strategy is to generate all nations and adjust their strength for balancing, rather than limiting lobby size based on the number of available nations on the map.
Applied to files:
src/client/graphics/layers/ControlPanel.tssrc/core/configuration/DefaultConfig.ts
📚 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/layers/ControlPanel.ts
📚 Learning: 2025-06-22T05:48:19.241Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 786
File: src/client/TerritoryPatternsModal.ts:337-338
Timestamp: 2025-06-22T05:48:19.241Z
Learning: In src/client/TerritoryPatternsModal.ts, the bit shifting operators (<<) used in coordinate calculations with decoder.getScale() are intentional and should not be changed to multiplication. The user scottanderson confirmed this is functioning as intended.
Applied to files:
src/client/graphics/layers/ControlPanel.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/layers/ControlPanel.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
🧬 Code graph analysis (1)
src/client/graphics/layers/ControlPanel.ts (2)
src/core/game/PlayerImpl.ts (1)
outgoingAttacks(1178-1180)src/core/game/GameView.ts (3)
outgoingAttacks(291-293)player(596-602)config(647-649)
🔇 Additional comments (7)
src/core/configuration/DefaultConfig.ts (2)
825-831: LGTM! Clean capacity method extraction.The territory and city capacity methods properly extract the formulas that were previously inline in
maxTroops(). The territory formula uses tile-based growth, and the city formula correctly leveragestotalUnitLevels(UnitType.City)multiplied by the city troop increase constant.
855-859: LGTM! Refactoring improves maintainability.The
maxTroops()method now delegates tomaxTroopsTerritory()andmaxTroopsCity(), making the capacity calculation reusable and the code more modular.src/client/graphics/layers/ControlPanel.ts (5)
5-5: LGTM! Import needed for TransportShip reference.The
UnitTypeimport is required for accessingUnitType.TransportShipin the troops-on-mission calculation.
26-54: LGTM! State variables properly initialized.All new state fields are correctly decorated with
@state()and initialized with sensible defaults (0 for numbers, empty string for color).
110-121: LGTM! Troops-on-mission calculation is correct.The logic correctly sums:
- Attack troops from all outgoing attacks
- Troops loaded on transport ships
This gives an accurate count of troops temporarily unavailable.
123-139: LGTM! Defensive error handling in place.The try-catch block properly handles potential errors from config methods and resets all breakdown fields to 0 on failure. The warning log aids debugging without crashing the UI.
141-143: LGTM! Color assignment with fallback.The player color correctly falls back to the theme's neutral color when territory color is unavailable.
…issions: bring feature branch up to date with latest upstream changes.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/client/graphics/layers/ControlPanel.ts(6 hunks)src/core/configuration/Config.ts(1 hunks)src/core/configuration/DefaultConfig.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (9)
📚 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/client/graphics/layers/ControlPanel.tssrc/core/configuration/DefaultConfig.tssrc/core/configuration/Config.ts
📚 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/client/graphics/layers/ControlPanel.tssrc/core/configuration/DefaultConfig.tssrc/core/configuration/Config.ts
📚 Learning: 2025-06-22T21:51:14.990Z
Learnt from: devalnor
Repo: openfrontio/OpenFrontIO PR: 1248
File: src/client/graphics/layers/TerritoryInfoLayer.ts:20-20
Timestamp: 2025-06-22T21:51:14.990Z
Learning: In TerritoryInfoLayer.ts, the highlightedTerritory field uses both null and undefined intentionally: undefined represents initial state or inactive layer (Ctrl released), while null represents active layer with no territory being highlighted at cursor position. This distinction is important for proper state change detection.
Applied to files:
src/client/graphics/layers/ControlPanel.ts
📚 Learning: 2025-05-31T18:15:03.445Z
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.
Applied to files:
src/client/graphics/layers/ControlPanel.tssrc/core/configuration/DefaultConfig.tssrc/core/configuration/Config.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/ControlPanel.tssrc/core/configuration/Config.ts
📚 Learning: 2025-10-21T20:06:04.823Z
Learnt from: Saphereye
Repo: openfrontio/OpenFrontIO PR: 2233
File: src/client/HostLobbyModal.ts:891-891
Timestamp: 2025-10-21T20:06:04.823Z
Learning: For the HumansVsNations game mode in `src/client/HostLobbyModal.ts` and related files, the implementation strategy is to generate all nations and adjust their strength for balancing, rather than limiting lobby size based on the number of available nations on the map.
Applied to files:
src/client/graphics/layers/ControlPanel.tssrc/core/configuration/DefaultConfig.tssrc/core/configuration/Config.ts
📚 Learning: 2025-06-22T05:48:19.241Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 786
File: src/client/TerritoryPatternsModal.ts:337-338
Timestamp: 2025-06-22T05:48:19.241Z
Learning: In src/client/TerritoryPatternsModal.ts, the bit shifting operators (<<) used in coordinate calculations with decoder.getScale() are intentional and should not be changed to multiplication. The user scottanderson confirmed this is functioning as intended.
Applied to files:
src/client/graphics/layers/ControlPanel.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/layers/ControlPanel.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
🧬 Code graph analysis (3)
src/client/graphics/layers/ControlPanel.ts (4)
src/core/game/PlayerImpl.ts (1)
outgoingAttacks(1178-1180)src/core/game/GameView.ts (3)
outgoingAttacks(291-293)player(596-602)config(647-649)src/core/game/GameImpl.ts (2)
player(472-478)config(336-338)src/core/game/StatsImpl.ts (1)
attack(149-158)
src/core/configuration/DefaultConfig.ts (1)
src/core/game/Game.ts (1)
Player(525-662)
src/core/configuration/Config.ts (2)
src/core/game/GameView.ts (2)
player(596-602)PlayerView(181-446)src/core/game/Game.ts (1)
Player(525-662)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/core/configuration/DefaultConfig.ts (2)
825-833: Consider extracting magic numbers to named constants.The formula uses several unexplained constants (0.6, 1000, 50000, and multiplier 2). While the documentation explains the high-level behavior, extracting these to named constants would improve maintainability and make the formula's intent clearer.
Example:
private static readonly TERRITORY_CAPACITY_EXPONENT = 0.6; // Diminishing returns private static readonly TERRITORY_CAPACITY_SCALE = 1000; private static readonly TERRITORY_CAPACITY_BASE = 50000; private static readonly TERRITORY_CAPACITY_MULTIPLIER = 2; maxTroopsTerritory(player: Player | PlayerView): number { const baseCapacity = Math.pow(player.numTilesOwned(), DefaultConfig.TERRITORY_CAPACITY_EXPONENT) * DefaultConfig.TERRITORY_CAPACITY_SCALE + DefaultConfig.TERRITORY_CAPACITY_BASE; return DefaultConfig.TERRITORY_CAPACITY_MULTIPLIER * baseCapacity; }
845-887: Previous review feedback properly addressed!The estimation methods now correctly use the unscaled base capacity (
maxTroopsTerritory + maxTroopsCity) as the denominator instead ofmaxTroops(player). This keeps the ratio consistent for all player types (bots, humans, and fake humans with difficulty modifiers), preventing the estimates from exceeding actual troop counts or dropping far below reality.The fix matches the suggested approach from the previous review.
Optional: Extract common base capacity calculation.
Both estimation methods compute
baseCapacityidentically (lines 853-857 and 875-879). Consider extracting this to a private helper method to reduce duplication:private baseCapacity(player: Player | PlayerView): number { return this.maxTroopsTerritory(player) + this.maxTroopsCity(player); } estimatedTroopsTerritory(player: Player | PlayerView): number { const maxTroopsTerritory = this.maxTroopsTerritory(player); const baseCapacity = this.baseCapacity(player); if (baseCapacity === 0) return 0; const currentTroops = player.troops(); return Math.round((maxTroopsTerritory / baseCapacity) * currentTroops); } estimatedTroopsCity(player: Player | PlayerView): number { const maxTroopsCity = this.maxTroopsCity(player); const baseCapacity = this.baseCapacity(player); if (baseCapacity === 0) return 0; const currentTroops = player.troops(); return Math.round((maxTroopsCity / baseCapacity) * currentTroops); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/core/configuration/DefaultConfig.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📚 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-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-05-31T18:15:03.445Z
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.
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-10-27T09:47:26.395Z
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.
Applied to files:
src/core/configuration/DefaultConfig.ts
📚 Learning: 2025-10-21T20:06:04.823Z
Learnt from: Saphereye
Repo: openfrontio/OpenFrontIO PR: 2233
File: src/client/HostLobbyModal.ts:891-891
Timestamp: 2025-10-21T20:06:04.823Z
Learning: For the HumansVsNations game mode in `src/client/HostLobbyModal.ts` and related files, the implementation strategy is to generate all nations and adjust their strength for balancing, rather than limiting lobby size based on the number of available nations on the map.
Applied to files:
src/core/configuration/DefaultConfig.ts
📚 Learning: 2025-10-18T17:54:01.311Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:172-173
Timestamp: 2025-10-18T17:54:01.311Z
Learning: In src/core/execution/FakeHumanExecution.ts, MIRVs and ground attacks should not be mutually exclusive. The considerMIRV() method should not short-circuit maybeAttack() - both actions can occur in the same tick.
Applied to files:
src/core/configuration/DefaultConfig.ts
🔇 Additional comments (2)
src/core/configuration/DefaultConfig.ts (2)
835-843: Clean implementation.The method is straightforward and well-documented. The logic correctly scales city capacity by the unit levels owned.
889-920: Well-structured refactoring.The method now properly delegates to the new helper methods (
maxTroopsTerritory+maxTroopsCity) while preserving all existing behavior for infinite troops, bots, and difficulty scaling. The composition approach is clean and the documentation is comprehensive.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
…itory/city to remove duplication and improve maintainability
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/core/configuration/DefaultConfig.ts (1)
853-868: Consider deriving one estimate to guarantee sum equals current troops.The independent rounding of both estimates can cause their sum to differ from
currentTroopsby ±1. For example, with equal capacity ratios andcurrentTroops = 99:
territory = Math.round(0.5 * 99) = 50city = Math.round(0.5 * 99) = 50- Sum = 100 ≠ 99
This slight mismatch affects flex-grow ratios in the UI. Consider deriving one estimate from the other to ensure exact sum:
return { territory: Math.round( (maxTroopsTerritory / baseCapacity) * currentTroops, ), - city: Math.round((maxTroopsCity / baseCapacity) * currentTroops), + city: currentTroops - Math.round((maxTroopsTerritory / baseCapacity) * currentTroops), };src/client/graphics/layers/ControlPanel.ts (2)
137-153: Enhance error logging for capacity calculation failures.The try-catch silently resets all values to 0 with only a warning. Consider logging the error details for debugging:
} catch (e) { - console.warn("Failed to calculate capacity breakdown:", e); + console.warn("Failed to calculate capacity breakdown:", e, { + playerID: player.id(), + ticks: this.game.ticks() + }); this._maxTroopsTerritory = 0;
298-302: Add internationalization for screen reader description.The descriptive text for screen readers is hardcoded in English, while other UI text uses
translateText(). Consider adding this to the translation files:<span id="troop-capacity-description" class="sr-only"> ${translateText("control_panel.capacity_description")} </span>Then add the key to
en.json:"control_panel.capacity_description": "Colored bar segments represent territory capacity (lighter shade) and city capacity (darker shade). Red segment shows troops currently on mission."
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/client/graphics/layers/ControlPanel.ts(6 hunks)src/core/configuration/Config.ts(1 hunks)src/core/configuration/DefaultConfig.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (10)
📚 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.tssrc/core/configuration/Config.tssrc/client/graphics/layers/ControlPanel.ts
📚 Learning: 2025-05-31T18:15:03.445Z
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.
Applied to files:
src/core/configuration/DefaultConfig.tssrc/core/configuration/Config.tssrc/client/graphics/layers/ControlPanel.ts
📚 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.tssrc/core/configuration/Config.tssrc/client/graphics/layers/ControlPanel.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-10-27T09:47:26.395Z
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.
Applied to files:
src/core/configuration/DefaultConfig.ts
📚 Learning: 2025-10-21T20:06:04.823Z
Learnt from: Saphereye
Repo: openfrontio/OpenFrontIO PR: 2233
File: src/client/HostLobbyModal.ts:891-891
Timestamp: 2025-10-21T20:06:04.823Z
Learning: For the HumansVsNations game mode in `src/client/HostLobbyModal.ts` and related files, the implementation strategy is to generate all nations and adjust their strength for balancing, rather than limiting lobby size based on the number of available nations on the map.
Applied to files:
src/core/configuration/DefaultConfig.tssrc/core/configuration/Config.ts
📚 Learning: 2025-06-22T21:51:14.990Z
Learnt from: devalnor
Repo: openfrontio/OpenFrontIO PR: 1248
File: src/client/graphics/layers/TerritoryInfoLayer.ts:20-20
Timestamp: 2025-06-22T21:51:14.990Z
Learning: In TerritoryInfoLayer.ts, the highlightedTerritory field uses both null and undefined intentionally: undefined represents initial state or inactive layer (Ctrl released), while null represents active layer with no territory being highlighted at cursor position. This distinction is important for proper state change detection.
Applied to files:
src/client/graphics/layers/ControlPanel.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/ControlPanel.ts
📚 Learning: 2025-06-22T05:48:19.241Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 786
File: src/client/TerritoryPatternsModal.ts:337-338
Timestamp: 2025-06-22T05:48:19.241Z
Learning: In src/client/TerritoryPatternsModal.ts, the bit shifting operators (<<) used in coordinate calculations with decoder.getScale() are intentional and should not be changed to multiplication. The user scottanderson confirmed this is functioning as intended.
Applied to files:
src/client/graphics/layers/ControlPanel.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/layers/ControlPanel.ts
🧬 Code graph analysis (3)
src/core/configuration/DefaultConfig.ts (2)
src/core/game/GameView.ts (2)
player(596-602)PlayerView(181-446)src/core/game/Game.ts (1)
Player(525-662)
src/core/configuration/Config.ts (2)
src/core/game/GameView.ts (2)
player(596-602)PlayerView(181-446)src/core/game/Game.ts (1)
Player(525-662)
src/client/graphics/layers/ControlPanel.ts (3)
src/core/game/PlayerImpl.ts (1)
outgoingAttacks(1178-1180)src/core/game/GameView.ts (3)
outgoingAttacks(291-293)player(596-602)config(647-649)src/core/game/GameImpl.ts (2)
player(472-478)config(336-338)
🔇 Additional comments (10)
src/core/configuration/Config.ts (1)
121-161: Well-documented interface additions.The new capacity and estimation methods are clearly documented with JSDoc, and the distinction between capacity limits and proportional estimates is explicit. The naming is clean and the parameter types use proper union types.
src/core/configuration/DefaultConfig.ts (4)
828-836: Clean territory capacity formula.The implementation correctly computes raw territory-based capacity using tile count, without player-type scaling. This aligns with the design where scaling is applied in
maxTroops().
838-846: Simple city capacity computation.The implementation correctly computes city-based capacity by multiplying total city levels by the per-level increase. Clean and straightforward.
870-892: Clean wrapper methods using shared helper.The two estimation methods efficiently delegate to the shared
estimateTroopSourceshelper, avoiding code duplication.
901-925: Correctly refactored to use new capacity helpers.The method properly composes territory and city capacities, then applies player-type scaling. By keeping the scaling here (not in the raw capacity methods), the estimates can use unscaled values for their denominator, fixing the ratio overflow issue from the past review.
src/client/graphics/layers/ControlPanel.ts (5)
5-5: Import correctly added for new functionality.The
UnitTypeimport is properly used for accessingTransportShipandCityunit types in the troop calculation logic.
26-68: Well-documented state properties.The new state properties are clearly documented with JSDoc explaining their purpose. The distinction between capacity values and proportional estimates is explicit.
189-189: Good use of local variable for template clarity.Extracting
_playerColorto a local variable improves readability in the template.
253-297: Progress bar structure and accessibility implemented well.The bar correctly uses capped width percentage to prevent overflow, and the flex-grow structure properly splits available vs. mission troops. ARIA attributes provide good screen reader support. The visual breakdown matches the design intent from the PR objectives.
124-135: Calculation is complete—no action needed.The
troopsOnMissionvalue correctly captures all troops away from territory:
attackTroops: troops committed to outgoing attacksboatTroops: troops in TransportShip unitsA codebase search shows only
TransportShipis queried for troop capacity in any context. Train, Warship, and other unit types do not participate in troop transport for missions. The semantic meaning of "on mission" (troops beyond territory control) is fully covered.Likely an incorrect or invalid review comment.
1 participant
Description:
Adds a visual progress bar to the Control Panel showing a breakdown for troops
The progress bar shows current maximum utilization (0-100%) with visual breakdown: territory (light) + cities (dark) + missions (red). Updates in real-time as territory, cities, and missions change.
Added
totalUnitLevels(),maxTroopsTerritory(),maxTroopsCity(),estimatedTroopsTerritory, andestimatedTroopsCityThe values for available city and territory troops are proportional allocations based on their respective troop maximums, not an exact record of each troop’s origin.
Resolves #1742
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
Binmo.