Skip to content

fix: radial build sub-menu items stay grayed out after gaining enough gold#3415

Open
evanpelle wants to merge 1 commit intov30from
fix-radial
Open

fix: radial build sub-menu items stay grayed out after gaining enough gold#3415
evanpelle wants to merge 1 commit intov30from
fix-radial

Conversation

@evanpelle
Copy link
Collaborator

@evanpelle evanpelle commented Mar 13, 2026

Description:

canBuildOrUpgrade was captured once at sub-menu open time, so disabled/color
never updated while the menu was open. Evaluate canBuildOrUpgrade dynamically
inside the disabled and color callbacks so the menu reflects current gold on
each refresh tick.

Please complete the following:

  • I have added screenshots for all UI updates
  • I process any text displayed to the user through translateText() and I've added it to the en.json file
  • I have added relevant tests to the test directory
  • 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:

evan

… gold

canBuildOrUpgrade was captured once at sub-menu open time, so disabled/color
never updated while the menu was open. Evaluate canBuildOrUpgrade dynamically
inside the disabled and color callbacks so the menu reflects current gold on
each refresh tick.
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 13, 2026

Walkthrough

This pull request consolidates multiple layout refinements and a game state tracking improvement. It tightens horizontal spacing across the HUD interface, reduces visual margins and padding in control panels and overlays, simplifies menu element build checks to use parameterized callbacks instead of cached values, adjusts team color hue distribution to a narrower range, enhances server-side lobby state tracking with a new flag, and increases the broadcast polling interval.

Changes

Cohort / File(s) Summary
UI Layout & Spacing
index.html, src/client/graphics/layers/ControlPanel.ts, src/client/graphics/layers/PlayerInfoOverlay.ts
Consistent tightening of horizontal widths (460px→500px) and vertical/horizontal spacing (mb-1.5→mb-1, gap-2→gap-1.5, padding adjustments). Overlay positioning simplified with centered alignment and fixed sizing. Slider height reduced from h-2 to h-1.5.
Menu Rendering
src/client/graphics/layers/RadialMenuElements.ts
Refactored canBuildOrUpgrade logic from precomputed constant to inline parameterized callback invocation, with color defaulting to building color when not upgradeable.
Color Generation
src/core/configuration/Colors.ts
Team color hue distribution narrowed from ±12° band (modulo 24 cycle) to ±6° band (modulo 12 cycle), reducing hue variance while maintaining lightness and chromatic variation.
Server Game State
src/server/GameServer.ts, src/server/MasterLobbyService.ts
Added hasReachedMaxPlayerCount flag to track maximum player capacity; updated join flow and phase evaluation logic to reference this flag. Increased broadcast polling interval from 250ms to 500ms.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

✨ Widths expand, spacing compresses tight,
Colors shift within a narrower light,
Callbacks inline where caches once stood,
Lobbies tracked when player counts are good. 🎮

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main fix: addressing grayed-out radial menu items that should update when the player gains enough gold.
Description check ✅ Passed The PR description accurately relates to the changes in RadialMenuElements.ts and the overall changeset, explaining the root cause and solution for the canBuildOrUpgrade capture issue.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

📝 Coding Plan
  • Generate coding plan for human review comments

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@evanpelle evanpelle marked this pull request as ready for review March 13, 2026 03:49
@evanpelle evanpelle requested a review from a team as a code owner March 13, 2026 03:49
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
index.html (1)

272-281: ⚠️ Potential issue | 🟡 Minor

Update the inline HUD width note to match 500px.

Line 279 still says 460px, but the layout now uses 500px (Line 272 and Line 281). Keeping this comment in sync will avoid confusion later.

✏️ Suggested update
-      <!-- HUD: <sm contents (children join outer flex), sm+ flex-col 460px, lg+ col-2 -->
+      <!-- HUD: <sm contents (children join outer flex), sm+ flex-col 500px, lg+ col-2 -->
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@index.html` around lines 272 - 281, Update the HUD width note so the inline
comment matches the actual layout width: change the "460px" mention in the HUD
comment above the div with class "contents sm:flex sm:flex-col ..." to "500px"
to reflect the w-[500px] used in the container classes and the other comment
references.
🧹 Nitpick comments (3)
src/core/configuration/Colors.ts (1)

34-35: Scope note: move this hue-band tweak to a separate PR.

Line 34-Line 35 changes team palette behavior, but this draft PR is scoped to radial build-menu state refresh. Splitting this into a small follow-up visual-tuning PR will make QA and rollback safer.

Based on learnings, the team prefers minimal, focused changes in bug-fix PRs to avoid scope creep.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/core/configuration/Colors.ts` around lines 34 - 35, Revert the recent
hue-band tweak so team palette behavior is unchanged in this PR: remove the new
hueShift adjustment (the line introducing "const hueShift = ((index *
goldenAngle) % 12) - 6;") from the palette-generation logic in Colors.ts (where
goldenAngle and hueShift are used) and restore the original hue calculation,
then move this visual-tuning change into a separate follow-up PR for QA. Ensure
any references to hueShift in the surrounding function (palette generation) are
removed or reverted to the prior implementation so no behavioral change lands in
this bug-fix PR.
src/server/GameServer.ts (1)

821-825: Add regression tests for this phase gate.

This condition now controls a key lobby/active transition. Please add tests for:

  1. reach max players, then drop below before start time;
  2. never reach max players before start time.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/server/GameServer.ts` around lines 821 - 825, The new phase-gate
condition in GameServer (the if using lessThanLifetime, this.hasStarted(), and
this.hasReachedMaxPlayerCount) lacks regression tests; add unit/integration
tests that exercise lobby->active transition behavior for GameServer.start
logic: (1) simulate reaching max players (set this.hasReachedMaxPlayerCount true
or add players until the server reports max), then remove a player so count
drops below max before start time and assert the transition does NOT proceed to
active; and (2) simulate never reaching max players before start time and assert
the transition proceeds (or remains allowed) according to current lifetime
logic; locate tests by referencing GameServer.hasStarted(),
GameServer.hasReachedMaxPlayerCount, and the lessThanLifetime condition and
assert expected state changes around the lobby/active boundary.
src/client/graphics/layers/RadialMenuElements.ts (1)

427-434: Add a regression test for “submenu open + gold changes” behavior.

Please add one focused test that keeps the submenu open, changes gold/buildables, and verifies disabled state and click behavior update without reopening the menu.

If you want, I can draft a minimal test case outline for this flow.

Also applies to: 459-460

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/client/graphics/layers/RadialMenuElements.ts` around lines 427 - 434, Add
a regression test that mounts the radial menu component (from
RadialMenuElements.ts) and simulates opening a submenu, then mutates the
player's gold and available buildables and asserts that
buildMenu.canBuildOrUpgrade(item) driven properties (disabled and color behavior
used in the MenuElementParams callbacks) update while the submenu remains open;
specifically verify the disabled state and click behavior change without the
menu reopening. Use the component’s public API to open the submenu, change
gold/buildables in the store/state, re-render/flush updates, then assert the
menu item’s disabled state and that clicking the item now either triggers or
blocks the build action accordingly. Ensure the test covers the same callbacks
referenced around buildMenu.canBuildOrUpgrade and the submenu path currently
handled in RadialMenuElements.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@index.html`:
- Around line 272-281: Update the HUD width note so the inline comment matches
the actual layout width: change the "460px" mention in the HUD comment above the
div with class "contents sm:flex sm:flex-col ..." to "500px" to reflect the
w-[500px] used in the container classes and the other comment references.

---

Nitpick comments:
In `@src/client/graphics/layers/RadialMenuElements.ts`:
- Around line 427-434: Add a regression test that mounts the radial menu
component (from RadialMenuElements.ts) and simulates opening a submenu, then
mutates the player's gold and available buildables and asserts that
buildMenu.canBuildOrUpgrade(item) driven properties (disabled and color behavior
used in the MenuElementParams callbacks) update while the submenu remains open;
specifically verify the disabled state and click behavior change without the
menu reopening. Use the component’s public API to open the submenu, change
gold/buildables in the store/state, re-render/flush updates, then assert the
menu item’s disabled state and that clicking the item now either triggers or
blocks the build action accordingly. Ensure the test covers the same callbacks
referenced around buildMenu.canBuildOrUpgrade and the submenu path currently
handled in RadialMenuElements.

In `@src/core/configuration/Colors.ts`:
- Around line 34-35: Revert the recent hue-band tweak so team palette behavior
is unchanged in this PR: remove the new hueShift adjustment (the line
introducing "const hueShift = ((index * goldenAngle) % 12) - 6;") from the
palette-generation logic in Colors.ts (where goldenAngle and hueShift are used)
and restore the original hue calculation, then move this visual-tuning change
into a separate follow-up PR for QA. Ensure any references to hueShift in the
surrounding function (palette generation) are removed or reverted to the prior
implementation so no behavioral change lands in this bug-fix PR.

In `@src/server/GameServer.ts`:
- Around line 821-825: The new phase-gate condition in GameServer (the if using
lessThanLifetime, this.hasStarted(), and this.hasReachedMaxPlayerCount) lacks
regression tests; add unit/integration tests that exercise lobby->active
transition behavior for GameServer.start logic: (1) simulate reaching max
players (set this.hasReachedMaxPlayerCount true or add players until the server
reports max), then remove a player so count drops below max before start time
and assert the transition does NOT proceed to active; and (2) simulate never
reaching max players before start time and assert the transition proceeds (or
remains allowed) according to current lifetime logic; locate tests by
referencing GameServer.hasStarted(), GameServer.hasReachedMaxPlayerCount, and
the lessThanLifetime condition and assert expected state changes around the
lobby/active boundary.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 266872d2-d25a-4cc6-abc5-888b9b5bce9a

📥 Commits

Reviewing files that changed from the base of the PR and between 3013133 and 50a3d45.

📒 Files selected for processing (7)
  • index.html
  • src/client/graphics/layers/ControlPanel.ts
  • src/client/graphics/layers/PlayerInfoOverlay.ts
  • src/client/graphics/layers/RadialMenuElements.ts
  • src/core/configuration/Colors.ts
  • src/server/GameServer.ts
  • src/server/MasterLobbyService.ts

@github-project-automation github-project-automation bot moved this from Triage to Final Review in OpenFront Release Management Mar 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Final Review

Development

Successfully merging this pull request may close these issues.

1 participant