fix: radial build sub-menu items stay grayed out after gaining enough gold#3415
fix: radial build sub-menu items stay grayed out after gaining enough gold#3415
Conversation
… 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.
WalkthroughThis 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
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
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 |
There was a problem hiding this comment.
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 | 🟡 MinorUpdate the inline HUD width note to match 500px.
Line 279 still says
460px, but the layout now uses500px(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:
- reach max players, then drop below before start time;
- 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
📒 Files selected for processing (7)
index.htmlsrc/client/graphics/layers/ControlPanel.tssrc/client/graphics/layers/PlayerInfoOverlay.tssrc/client/graphics/layers/RadialMenuElements.tssrc/core/configuration/Colors.tssrc/server/GameServer.tssrc/server/MasterLobbyService.ts
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:
Please put your Discord username so you can be contacted if a bug or regression is found:
evan