fix: radial build sub-menu items stay grayed out after gaining enough gold#3415
Merged
fix: radial build sub-menu items stay grayed out after gaining enough gold#3415
Conversation
Contributor
WalkthroughRefactor in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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. 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}
Contributor
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
github-project-automation
bot
moved this from Triage
to Final Review
in OpenFront Release Management
… 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.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/client/graphics/RadialMenuElements.test.ts`:
- Around line 586-596: Add an assertion that the menu item's disabled predicate
returns true so the test verifies disabled state as well as color: after
obtaining cityElement from buildMenuElement.subMenu(mockParams) and before or
after the color check, call cityElement!.disabled(mockParams) and expect it to
be true (this augments the existing mockBuildMenu.canBuildOrUpgrade = vi.fn(()
=> false) setup and ensures the disabled logic on the MenuElement is enforced
alongside the color check against COLORS.building).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 82ed16fe-d024-40ce-a122-63f2d58a4a8a
📒 Files selected for processing (2)
src/client/graphics/layers/RadialMenuElements.tstests/client/graphics/RadialMenuElements.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/client/graphics/layers/RadialMenuElements.ts
Comment on lines
+586
to
+596
| it("should use disabled color when element is disabled", () => { | ||
| mockBuildMenu.canBuildOrUpgrade = vi.fn(() => false); | ||
|
|
||
| const subMenu = buildMenuElement.subMenu!(mockParams); | ||
| const cityElement = subMenu.find((item) => item.id === "build_City"); | ||
|
|
||
| expect(cityElement!.color).toBeUndefined(); | ||
| expect( | ||
| (cityElement!.color as (params: MenuElementParams) => string)( | ||
| mockParams, | ||
| ), | ||
| ).toBe(COLORS.building); |
Contributor
There was a problem hiding this comment.
This test should also assert disabled, not only color
Right now this case is weak: build items use COLORS.building in both enabled and disabled states, so this can pass even if disable logic breaks. Please assert cityElement!.disabled(mockParams) === true too.
Suggested test hardening
it("should use disabled color when element is disabled", () => {
mockBuildMenu.canBuildOrUpgrade = vi.fn(() => false);
const subMenu = buildMenuElement.subMenu!(mockParams);
const cityElement = subMenu.find((item) => item.id === "build_City");
+ expect(cityElement!.disabled(mockParams)).toBe(true);
expect(
(cityElement!.color as (params: MenuElementParams) => string)(
mockParams,
),
).toBe(COLORS.building);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it("should use disabled color when element is disabled", () => { | |
| mockBuildMenu.canBuildOrUpgrade = vi.fn(() => false); | |
| const subMenu = buildMenuElement.subMenu!(mockParams); | |
| const cityElement = subMenu.find((item) => item.id === "build_City"); | |
| expect(cityElement!.color).toBeUndefined(); | |
| expect( | |
| (cityElement!.color as (params: MenuElementParams) => string)( | |
| mockParams, | |
| ), | |
| ).toBe(COLORS.building); | |
| it("should use disabled color when element is disabled", () => { | |
| mockBuildMenu.canBuildOrUpgrade = vi.fn(() => false); | |
| const subMenu = buildMenuElement.subMenu!(mockParams); | |
| const cityElement = subMenu.find((item) => item.id === "build_City"); | |
| expect(cityElement!.disabled(mockParams)).toBe(true); | |
| expect( | |
| (cityElement!.color as (params: MenuElementParams) => string)( | |
| mockParams, | |
| ), | |
| ).toBe(COLORS.building); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/client/graphics/RadialMenuElements.test.ts` around lines 586 - 596, Add
an assertion that the menu item's disabled predicate returns true so the test
verifies disabled state as well as color: after obtaining cityElement from
buildMenuElement.subMenu(mockParams) and before or after the color check, call
cityElement!.disabled(mockParams) and expect it to be true (this augments the
existing mockBuildMenu.canBuildOrUpgrade = vi.fn(() => false) setup and ensures
the disabled logic on the MenuElement is enforced alongside the color check
against COLORS.building).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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