Feat: Quick donate troops via radial menu #2708
Conversation
…r attack vs donate
WalkthroughRadial menu detects friendly targets and switches the center button to a donate appearance and size; clicking donates troops using Changes
Sequence DiagramsequenceDiagram
autonumber
actor Player
participant RadialMenu
participant RadialMenuElements
participant PlayerActionHandler
Note over Player,RadialMenuElements: hover / open radial menu
Player->>RadialMenu: updateActions(recipient, uiState)
RadialMenu->>RadialMenuElements: updateActions(recipient, uiState)
RadialMenuElements->>RadialMenuElements: isFriendlyTarget(params)
alt Friendly target
RadialMenuElements->>RadialMenu: setCenterButtonAppearance(donateTroopIcon, "#34D399", 0.75)
else Enemy target
RadialMenuElements->>RadialMenu: setCenterButtonAppearance(swordIcon, "#2c3e50")
end
Note over Player: click center button
Player->>RadialMenuElements: center button action
alt Donate (friendly)
RadialMenuElements->>PlayerActionHandler: handleDonateTroops(recipient, computedAmount)
PlayerActionHandler->>PlayerActionHandler: validate amount
PlayerActionHandler->>PlayerActionHandler: emit SendDonateTroopsIntentEvent(amount|null)
else Attack (enemy)
RadialMenuElements->>PlayerActionHandler: performAttack(recipient)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/client/graphics/layers/MainRadialMenu.ts (1)
1-1: Fix code formatting.The pipeline reports a Prettier formatting issue. Run the suggested command to fix:
npx prettier --write src/client/graphics/layers/MainRadialMenu.tssrc/client/graphics/layers/RadialMenuElements.ts (1)
1-1: Fix code formatting.The pipeline reports a Prettier formatting issue. Run the suggested command to fix:
npx prettier --write src/client/graphics/layers/RadialMenuElements.ts
🧹 Nitpick comments (2)
src/client/graphics/layers/MainRadialMenu.ts (1)
136-144: Refactor: Extract duplicated friendly-target logic.The
isFriendlyTargetlogic here duplicates the helper function defined inRadialMenuElements.ts(lines 112-119). Consider importing and reusing the helper fromRadialMenuElements.tsto maintain a single source of truth.🔎 Suggested refactor
Import the helper from RadialMenuElements:
import { centerButtonElement, COLORS, + isFriendlyTarget, MenuElementParams, rootMenuElement, } from "./RadialMenuElements";Then replace the local computation:
- const isFriendlyTarget = - recipient !== null && - recipient.id() !== myPlayer.id() && - recipient.isFriendly(myPlayer); + const friendly = isFriendlyTarget(params); this.radialMenu.setCenterButtonAppearance( - isFriendlyTarget ? donateTroopIcon : swordIcon, - isFriendlyTarget ? "#34D399" : "#2c3e50", + friendly ? donateTroopIcon : swordIcon, + friendly ? "#34D399" : "#2c3e50", );Note: You'll need to export
isFriendlyTargetfromRadialMenuElements.tsfirst.src/client/graphics/layers/RadialMenuElements.ts (1)
112-119: Consider exporting this helper for reuse.The
isFriendlyTargethelper encapsulates the friendly-player detection logic cleanly. Consider exporting it soMainRadialMenu.ts(lines 136-139) can reuse it instead of duplicating the same logic.🔎 Suggested change
-function isFriendlyTarget(params: MenuElementParams): boolean { +export function isFriendlyTarget(params: MenuElementParams): boolean { const selectedPlayer = params.selected; return ( selectedPlayer !== null && selectedPlayer.id() !== params.myPlayer.id() && selectedPlayer.isFriendly(params.myPlayer) ); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/client/graphics/layers/MainRadialMenu.tssrc/client/graphics/layers/PlayerActionHandler.tssrc/client/graphics/layers/RadialMenu.tssrc/client/graphics/layers/RadialMenuElements.ts
🧰 Additional context used
🧠 Learnings (6)
📚 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/PlayerActionHandler.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/PlayerActionHandler.tssrc/client/graphics/layers/MainRadialMenu.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/client/graphics/layers/PlayerActionHandler.ts
📚 Learning: 2025-06-16T03:03:59.778Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1192
File: src/client/graphics/layers/RadialMenuElements.ts:312-314
Timestamp: 2025-06-16T03:03:59.778Z
Learning: In RadialMenuElements.ts, when fixing undefined params errors in subMenu functions, use explicit checks like `if (params === undefined || params.selected === null)` rather than optional chaining, as it makes the intent clearer and matches the specific error scenarios where params can be undefined during spawn phase operations.
Applied to files:
src/client/graphics/layers/MainRadialMenu.tssrc/client/graphics/layers/RadialMenuElements.tssrc/client/graphics/layers/RadialMenu.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/RadialMenuElements.ts
📚 Learning: 2025-06-10T09:56:44.473Z
Learnt from: Ble4Ch
Repo: openfrontio/OpenFrontIO PR: 1063
File: src/core/configuration/PastelThemeDark.ts:53-53
Timestamp: 2025-06-10T09:56:44.473Z
Learning: In ColorAllocator class in src/core/configuration/Colors.ts, the correct method names are assignColor(id: string): Colord for general color assignment and assignTeamColor(team: Team): Colord for team-specific colors. There are no assignPlayerColor() or assignBotColor() methods.
Applied to files:
src/client/graphics/layers/RadialMenu.ts
🧬 Code graph analysis (3)
src/client/graphics/layers/PlayerActionHandler.ts (3)
src/core/game/GameView.ts (3)
PlayerView(185-578)troops(101-103)troops(493-495)src/core/game/PlayerImpl.ts (1)
troops(832-834)src/client/Transport.ts (1)
SendDonateTroopsIntentEvent(115-120)
src/client/graphics/layers/MainRadialMenu.ts (1)
src/core/game/GameView.ts (1)
myPlayer(724-726)
src/client/graphics/layers/RadialMenuElements.ts (2)
src/client/graphics/UIState.ts (1)
UIState(3-6)src/core/game/GameView.ts (1)
PlayerView(185-578)
🪛 GitHub Actions: 🧪 CI
src/client/graphics/layers/MainRadialMenu.ts
[warning] 1-1: Code style issues found in file. Run 'npx prettier --write src/client/graphics/layers/MainRadialMenu.ts' to fix.
src/client/graphics/layers/RadialMenuElements.ts
[warning] 1-1: Code style issues found in file. Run 'npx prettier --write src/client/graphics/layers/RadialMenuElements.ts' to fix.
🔇 Additional comments (4)
src/client/graphics/layers/PlayerActionHandler.ts (1)
87-93: LGTM! Clean parameter-driven donation logic.The updated signature accepts an optional
troopsparameter, properly guards against non-positive amounts, and emits the event with the computed value. The guard on line 89 correctly prevents donating zero or negative troops.src/client/graphics/layers/RadialMenuElements.ts (1)
574-589: LGTM! Proper donation flow with good safeguards.The center button action correctly:
- Detects friendly targets using the helper
- Uses
uiState?.attackRatio ?? 1with proper fallback- Applies
Math.floorto ensure whole troop counts- Guards against zero-troop donations before calling the handler
The logic integrates well with the existing attack flow.
src/client/graphics/layers/RadialMenu.ts (2)
81-82: Good color state management.The introduction of
defaultCenterButtonColoras a readonly constant andcenterButtonColoras mutable state provides clean separation between the default and current appearance. This pattern works well with the new dynamic appearance API.
959-981: LGTM! Clean API for dynamic center button appearance.The
setCenterButtonAppearancemethod properly:
- Updates both icon and color state
- Provides sensible fallback to
defaultCenterButtonColor- Guards against missing
menuElement- Applies changes immediately to the visible button via
updateCenterButtonStateThe implementation integrates well with the existing center button state management.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/client/graphics/layers/MainRadialMenu.ts (1)
136-142: Extract hardcoded colors to the COLORS constant for consistency.The file already uses
COLORS.tooltip.costandCOLORS.tooltip.count. Consider addingCOLORS.centerButton.donateandCOLORS.centerButton.attackto keep all colors in one place, then update thesetCenterButtonAppearancecall to use them instead of"#34D399"and"#2c3e50".
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/client/graphics/layers/MainRadialMenu.tssrc/client/graphics/layers/RadialMenuElements.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/client/graphics/layers/RadialMenuElements.ts
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-06-16T03:03:59.778Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1192
File: src/client/graphics/layers/RadialMenuElements.ts:312-314
Timestamp: 2025-06-16T03:03:59.778Z
Learning: In RadialMenuElements.ts, when fixing undefined params errors in subMenu functions, use explicit checks like `if (params === undefined || params.selected === null)` rather than optional chaining, as it makes the intent clearer and matches the specific error scenarios where params can be undefined during spawn phase operations.
Applied to files:
src/client/graphics/layers/MainRadialMenu.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/MainRadialMenu.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: In `ClientGameRunner.ts`, the `myPlayer` field is always set when `shouldPreventWindowClose()` is called, so the null check in that method is sufficient without needing to fetch it again from `gameView.playerByClientID()`.
Applied to files:
src/client/graphics/layers/MainRadialMenu.ts
📚 Learning: 2025-12-13T14:58:29.645Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2607
File: src/core/execution/PlayerExecution.ts:271-295
Timestamp: 2025-12-13T14:58:29.645Z
Learning: In src/core/execution/PlayerExecution.ts surroundedBySamePlayer(), the `as Player` cast on `mg.playerBySmallID(scan.enemyId)` is intentional. Since scan.enemyId comes from ownerID() on an owned tile and playerBySmallID() only returns Player or undefined, the cast expresses a known invariant. The maintainers prefer loud failures (runtime errors) over silent masking (early returns with guards) for corrupted game state scenarios at trusted call sites.
Applied to files:
src/client/graphics/layers/MainRadialMenu.ts
🧬 Code graph analysis (1)
src/client/graphics/layers/MainRadialMenu.ts (1)
src/core/game/GameView.ts (1)
myPlayer(728-730)
🔇 Additional comments (2)
src/client/graphics/layers/MainRadialMenu.ts (2)
23-23: LGTM!The import follows the same pattern as the existing
swordIconimport.
131-131: LGTM!Correctly passes
uiStateto the params, enabling the radial menu to access UI state for the donate feature.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/client/graphics/layers/PlayerPanel.ts (1)
155-160: Consider resettingsuppressNextHidein thehide()method.Currently, if the panel is closed via the X button or
CloseViewEventbefore theMouseUpEventfires,suppressNextHideremains true. While not breaking (sincehide()is idempotent), this creates stale state that could suppress an unrelated future mouse event.Suggested state cleanup
public hide() { this.isVisible = false; this.sendMode = "none"; this.sendTarget = null; + this.suppressNextHide = false; this.requestUpdate(); }Also applies to: 188-188, 194-194
src/client/graphics/layers/RadialMenuElements.ts (1)
433-449: Use a COLORS constant for the donate gold color.Line 440 uses a hardcoded color
"#EAB308". For consistency with the rest of the codebase, define this in theCOLORSobject.🔎 Proposed refactor
Add to the COLORS object around line 85:
trade: "#008080", embargo: "#6600cc", + donateGold: "#EAB308", tooltip: {Then update line 440:
- color: "#EAB308", + color: COLORS.donateGold,Note: The
id: Slot.Attackon line 434 matchesattackMenuElement, but this is intentional since they occupy the same radial position conditionally. The menu composition logic on lines 633-635 ensures only one renders based on whether the target is friendly.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/client/graphics/layers/PlayerPanel.tssrc/client/graphics/layers/RadialMenuElements.ts
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-06-16T03:03:59.778Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1192
File: src/client/graphics/layers/RadialMenuElements.ts:312-314
Timestamp: 2025-06-16T03:03:59.778Z
Learning: In RadialMenuElements.ts, when fixing undefined params errors in subMenu functions, use explicit checks like `if (params === undefined || params.selected === null)` rather than optional chaining, as it makes the intent clearer and matches the specific error scenarios where params can be undefined during spawn phase operations.
Applied to files:
src/client/graphics/layers/RadialMenuElements.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/RadialMenuElements.tssrc/client/graphics/layers/PlayerPanel.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: In `ClientGameRunner.ts`, the `myPlayer` field is always set when `shouldPreventWindowClose()` is called, so the null check in that method is sufficient without needing to fetch it again from `gameView.playerByClientID()`.
Applied to files:
src/client/graphics/layers/RadialMenuElements.ts
📚 Learning: 2025-12-13T14:58:29.645Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2607
File: src/core/execution/PlayerExecution.ts:271-295
Timestamp: 2025-12-13T14:58:29.645Z
Learning: In src/core/execution/PlayerExecution.ts surroundedBySamePlayer(), the `as Player` cast on `mg.playerBySmallID(scan.enemyId)` is intentional. Since scan.enemyId comes from ownerID() on an owned tile and playerBySmallID() only returns Player or undefined, the cast expresses a known invariant. The maintainers prefer loud failures (runtime errors) over silent masking (early returns with guards) for corrupted game state scenarios at trusted call sites.
Applied to files:
src/client/graphics/layers/RadialMenuElements.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/RadialMenuElements.tssrc/client/graphics/layers/PlayerPanel.ts
🧬 Code graph analysis (2)
src/client/graphics/layers/RadialMenuElements.ts (2)
src/client/graphics/UIState.ts (1)
UIState(3-6)src/core/game/GameView.ts (1)
PlayerView(185-582)
src/client/graphics/layers/PlayerPanel.ts (4)
src/client/InputHandler.ts (1)
MouseUpEvent(8-13)src/core/game/GameView.ts (2)
actions(406-412)tile(110-112)src/core/game/Game.ts (1)
PlayerActions(772-778)src/core/game/GameMap.ts (1)
TileRef(3-3)
🪛 GitHub Actions: 🧪 CI
src/client/graphics/layers/RadialMenuElements.ts
[error] 114-114: TypeError: selectedPlayer.isFriendly is not a function. isFriendlyTarget calls selectedPlayer.isFriendly but the selected player object does not implement that method.
🔇 Additional comments (9)
src/client/graphics/layers/PlayerPanel.ts (4)
65-65: State flag added to control panel hiding behavior.The
suppressNextHideflag is properly declared with clean initialization. This prevents the panel from closing immediately after opening a modal when the user releases the mouse button.
82-88: MouseUpEvent handler correctly implements suppression logic.The conditional check properly consumes the
suppressNextHideflag on the next mouse-up event, preventing premature panel closure when modals are opened. Clean early return pattern.
141-153: New public API for opening gold donation modal directly.The
openSendGoldModalmethod properly sets up all necessary state for direct modal invocation, likely from the radial menu. SettingsuppressNextHide = truecorrectly prevents the release-click from immediately closing the panel.
141-153: API is properly integrated with the radial menu.The
openSendGoldModalmethod is called fromRadialMenuElements.ts(line 443) with the correct parameters when a player selects the donation option from the radial menu.src/client/graphics/layers/RadialMenuElements.ts (5)
7-7: LGTM! UIState integration is clean.Adding the optional
uiStateparameter toMenuElementParamsallows menu elements to accessattackRatiofor donation calculations. The optional field is appropriate since not all menu contexts require UI state.Also applies to: 41-41
577-580: Conditional gating logic looks correct.The center button correctly disables for friendly targets when troop donation is not permitted, and for non-friendly targets when attack is not permitted. This provides appropriate access control for the dual-purpose button.
Note: This depends on fixing the
isFriendlyTargetfunction (lines 112-115) which currently has a pipeline failure.
630-636: Clean conditional menu composition.The ternary operator clearly switches between
donateGoldRadialElementandattackMenuElementbased on target type, making the dual-purpose slot behavior explicit and maintainable.Note: This depends on fixing the
isFriendlyTargetfunction (lines 112-115) which currently has a pipeline failure.
112-115: TheisFriendlymethod exists and is correctly implemented onPlayerView(line 520 insrc/core/game/GameView.ts). The code inRadialMenuElements.ts:112-115is correct and requires no changes.Likely an incorrect or invalid review comment.
588-603: No issues found. The code correctly prevents donations with zero or negative troop amounts through the guard clause at line 592 (if (troopsToDonate > 0)). The handler is never called with an invalid amount, and doing nothing when the calculated donation rounds to zero is the expected behavior—not a silent failure. The menu item is already disabled at the UI level if donations are not permitted. No changes needed.
…cted isnt a PlayerView
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/client/graphics/layers/RadialMenuElements.ts (1)
587-610: Consider adding user feedback when troop donation cannot proceed.The code uses
params.uiState?.attackRatio ?? 1as fallback, butuiStateis always initialized inGameRendererand passed throughMainRadialMenu, so this default should not occur in practice. The actual concern is the silent no-op: whentroopsToDonateis 0 (from low troop count or small ratio), the menu closes without any feedback to the player. Add a notification or disable the action more explicitly to guide players.
🧹 Nitpick comments (2)
src/client/graphics/layers/RadialMenuElements.ts (2)
112-118: Simplify method invocation.The defensive checks are good (aligns with commit message about test safety), but the
.call()usage is unnecessarily verbose.🔎 Simpler equivalent
function isFriendlyTarget(params: MenuElementParams): boolean { const selectedPlayer = params.selected; if (selectedPlayer === null) return false; - const isFriendly = (selectedPlayer as PlayerView).isFriendly; - if (typeof isFriendly !== "function") return false; - return isFriendly.call(selectedPlayer, params.myPlayer); + const playerView = selectedPlayer as PlayerView; + if (typeof playerView.isFriendly !== "function") return false; + return playerView.isFriendly(params.myPlayer); }
436-452: Use COLORS constant for consistency.The hardcoded color
"#EAB308"(line 443) breaks the pattern used elsewhere in the file. Consider adding a named constant to theCOLORSobject for better maintainability.🔎 Consistent with existing patterns
At the COLORS definition (around line 73), add:
export const COLORS = { build: "#ebe250", building: "#2c2c2c", boat: "#3f6ab1", ally: "#53ac75", + donate: "#EAB308", breakAlly: "#c74848", // ...Then use it:
icon: donateGoldIcon, - color: "#EAB308", + color: COLORS.donate, action: (params: MenuElementParams) => {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/client/graphics/layers/RadialMenuElements.ts
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-06-16T03:03:59.778Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1192
File: src/client/graphics/layers/RadialMenuElements.ts:312-314
Timestamp: 2025-06-16T03:03:59.778Z
Learning: In RadialMenuElements.ts, when fixing undefined params errors in subMenu functions, use explicit checks like `if (params === undefined || params.selected === null)` rather than optional chaining, as it makes the intent clearer and matches the specific error scenarios where params can be undefined during spawn phase operations.
Applied to files:
src/client/graphics/layers/RadialMenuElements.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/RadialMenuElements.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: In `ClientGameRunner.ts`, the `myPlayer` field is always set when `shouldPreventWindowClose()` is called, so the null check in that method is sufficient without needing to fetch it again from `gameView.playerByClientID()`.
Applied to files:
src/client/graphics/layers/RadialMenuElements.ts
📚 Learning: 2025-12-13T14:58:29.645Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2607
File: src/core/execution/PlayerExecution.ts:271-295
Timestamp: 2025-12-13T14:58:29.645Z
Learning: In src/core/execution/PlayerExecution.ts surroundedBySamePlayer(), the `as Player` cast on `mg.playerBySmallID(scan.enemyId)` is intentional. Since scan.enemyId comes from ownerID() on an owned tile and playerBySmallID() only returns Player or undefined, the cast expresses a known invariant. The maintainers prefer loud failures (runtime errors) over silent masking (early returns with guards) for corrupted game state scenarios at trusted call sites.
Applied to files:
src/client/graphics/layers/RadialMenuElements.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/RadialMenuElements.ts
🧬 Code graph analysis (1)
src/client/graphics/layers/RadialMenuElements.ts (3)
src/client/graphics/UIState.ts (1)
UIState(3-6)src/core/game/GameView.ts (2)
isFriendly(520-522)PlayerView(185-582)src/core/game/PlayerImpl.ts (1)
isFriendly(805-810)
🔇 Additional comments (3)
src/client/graphics/layers/RadialMenuElements.ts (3)
41-41: LGTM! Optional field maintains backward compatibility.The optional
uiStatefield is a clean addition to the interface that allows existing code to continue working while new donation features can access the UI state.
568-586: LGTM! Correct gating logic.The center button correctly disables based on whether the target is friendly and whether troop donation is allowed.
633-639: LGTM! Correct conditional rendering.The radial menu appropriately swaps between donation and attack elements based on target friendliness.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/client/graphics/layers/MainRadialMenu.ts (1)
136-145: LGTM! Clean logic for switching center button appearance.The friendly target detection is null-safe, and the conditional appearance update correctly switches between donate and attack modes. The 0.75 size multiplier aligns with your commit message about visual appearance next to the donate gold icon.
Optional: Extract colors to COLORS constant for consistency
The file already uses a
COLORSobject for tooltip styling. For consistency, consider extracting the hardcoded hex values:const menuConfig: RadialMenuConfig = { centerButtonIcon: swordIcon, tooltipStyle: ` .radial-tooltip .cost { margin-top: 4px; color: ${COLORS.tooltip.cost}; } .radial-tooltip .count { color: ${COLORS.tooltip.count}; } `, };In
RadialMenuElements.ts, you could add:centerButton: { friendly: "#34D399", // green for donate attack: "#2c3e50", // gray for attack }Then use
COLORS.centerButton.friendlyandCOLORS.centerButton.attackhere.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/client/graphics/layers/MainRadialMenu.tssrc/client/graphics/layers/RadialMenu.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/client/graphics/layers/RadialMenu.ts
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-06-16T03:03:59.778Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1192
File: src/client/graphics/layers/RadialMenuElements.ts:312-314
Timestamp: 2025-06-16T03:03:59.778Z
Learning: In RadialMenuElements.ts, when fixing undefined params errors in subMenu functions, use explicit checks like `if (params === undefined || params.selected === null)` rather than optional chaining, as it makes the intent clearer and matches the specific error scenarios where params can be undefined during spawn phase operations.
Applied to files:
src/client/graphics/layers/MainRadialMenu.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/MainRadialMenu.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: In `ClientGameRunner.ts`, the `myPlayer` field is always set when `shouldPreventWindowClose()` is called, so the null check in that method is sufficient without needing to fetch it again from `gameView.playerByClientID()`.
Applied to files:
src/client/graphics/layers/MainRadialMenu.ts
📚 Learning: 2025-12-13T14:58:29.645Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2607
File: src/core/execution/PlayerExecution.ts:271-295
Timestamp: 2025-12-13T14:58:29.645Z
Learning: In src/core/execution/PlayerExecution.ts surroundedBySamePlayer(), the `as Player` cast on `mg.playerBySmallID(scan.enemyId)` is intentional. Since scan.enemyId comes from ownerID() on an owned tile and playerBySmallID() only returns Player or undefined, the cast expresses a known invariant. The maintainers prefer loud failures (runtime errors) over silent masking (early returns with guards) for corrupted game state scenarios at trusted call sites.
Applied to files:
src/client/graphics/layers/MainRadialMenu.ts
🧬 Code graph analysis (1)
src/client/graphics/layers/MainRadialMenu.ts (1)
src/core/game/GameView.ts (1)
myPlayer(728-730)
🔇 Additional comments (2)
src/client/graphics/layers/MainRadialMenu.ts (2)
23-23: LGTM!The donate troop icon import is correctly placed and used in the center button appearance logic below.
131-131: LGTM!Adding
uiStateto params enables radial menu elements to access the attack ratio for troop donations, which is necessary for the quick donate feature.
## Description: I believe this bug came after the introduction of the new donation button in the radial menu (#2708) causing the rectangular SVGs to get squished/skewed into square aspect ratios. This fix adds a little check to fallback onto original image dimensions when an SVG's aspect ratio isn't available. I tried a hardcoded fix earlier but if we ever decide to add different scaled icons into the radial menu again, this will automatically ensure the correct aspect ratio is used and the icon is centered properly. ### Before Fix <img width="1133" height="473" alt="image" src="https://github.com/user-attachments/assets/5f06b19c-7072-4650-a1b7-4cb2bf1200dc" /> ### After Fix <img width="840" height="421" alt="image" src="https://github.com/user-attachments/assets/176f1e7b-84bf-4b06-9ad6-4031e516d4ff" /> ## Please complete the following: - [x] I have added screenshots for all UI updates - [x] I process any text displayed to the user through translateText() and I've added it to the en.json file - [x] I have added relevant tests to the test directory - [x] I confirm I have thoroughly tested these changes and take full responsibility for any bugs introduced ## Please put your Discord username so you can be contacted if a bug or regression is found: bijx
…2769) ## Description: Updates the instructions Help Menu to update the ally radial menu screenshot to show the new gold and troop donation icons, as well as what they do. Related to #2708 <img width="1656" height="974" alt="image" src="https://github.com/user-attachments/assets/365e0fe5-6854-4cac-8288-039a05cf4905" /> ## Please complete the following: - [x] I have added screenshots for all UI updates - [x] I process any text displayed to the user through translateText() and I've added it to the en.json file - [x] I have added relevant tests to the test directory - [x] I confirm I have thoroughly tested these changes and take full responsibility for any bugs introduced ## Please put your Discord username so you can be contacted if a bug or regression is found: bijx
2 participants
If this PR fixes an issue, link it below. If not, delete these two lines.
Resolves #2705
Description:
Introduces a quick donation feature in games where the
canDonateTroopsoption is enabled. It works by converting the center button in the radial menu from a disabled attack button to a troop donate button when the player right clicked on is friendly (teammate or ally).Also adds donate gold button to Attack slot on radial menu when right clicking friendly troops.
Video Example
Recording.2025-12-26.214041.mp4
New Icon
Donate Gold
Recording.2025-12-27.000313.mp4
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
bijx