Refactor radial menu #1246
Refactor radial menu #1246
Conversation
|
Warning Rate limit exceeded@evanpelle has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 35 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (5)
WalkthroughThis update removes the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant EventBus
participant MainRadialMenu
participant RadialMenu
User->>EventBus: Triggers ContextMenuEvent (right-click)
EventBus->>MainRadialMenu: Emits ContextMenuEvent
MainRadialMenu->>MainRadialMenu: Converts screen to world coords, validates
MainRadialMenu->>MainRadialMenu: Fetches player actions for tile
MainRadialMenu->>RadialMenu: Calls handlePlayerActions (with actions, screenX, screenY)
RadialMenu->>User: Displays radial menu at cursor position
Possibly related PRs
Suggested labels
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/client/graphics/layers/RadialMenuElements.ts (1)
411-424: Simplify disabled logic for better readabilityThe nested conditions make the logic harder to follow. Consider extracting to named variables:
disabled: (params: MenuElementParams): boolean => { const tileOwner = params.game.owner(params.tile); const isLand = params.game.isLand(params.tile); - if (!isLand) { - return true; - } - if (params.game.inSpawnPhase()) { - if (tileOwner.isPlayer()) { - return true; - } - return false; - } - return false; + const isNotLand = !isLand; + const isSpawnPhase = params.game.inSpawnPhase(); + const isOwnedByPlayer = tileOwner.isPlayer(); + + if (isNotLand) return true; + if (isSpawnPhase && isOwnedByPlayer) return true; + return false; },src/client/graphics/layers/RadialMenu.ts (1)
296-297: Consider required params when menu is visibleThe frequent null checks for
paramsadd complexity. Since the menu should not be visible without valid params, consider ensuring params is always set before showing the menu.Consider adding a guard in
showRadialMenu:public showRadialMenu(x: number, y: number) { if (!this.params) { console.warn("Cannot show menu without params"); return; } // ... rest of implementation }Also applies to: 370-371, 447-451
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/client/graphics/GameRenderer.ts(0 hunks)src/client/graphics/layers/MainRadialMenu.ts(6 hunks)src/client/graphics/layers/MenuEventManager.ts(0 hunks)src/client/graphics/layers/RadialMenu.ts(24 hunks)src/client/graphics/layers/RadialMenuElements.ts(5 hunks)
💤 Files with no reviewable changes (2)
- src/client/graphics/GameRenderer.ts
- src/client/graphics/layers/MenuEventManager.ts
🔇 Additional comments (9)
src/client/graphics/layers/RadialMenuElements.ts (4)
58-62: Good interface designThe
CenterButtonElementinterface follows a clean composition pattern with clear responsibilities.
236-280: Improved emoji menu UXGood refactor: placing the "more" button first makes it more discoverable, and reducing initial emojis from 15 to 8 reduces visual clutter.
286-287: Correct disabled logic for spawn phaseThe spawn phase check appropriately disables the info menu when players cannot interact.
29-41: Breaking change: Verify all usages of MenuElementParamsThe removal of
tileOwnerproperty fromMenuElementParamsis a breaking change. Ensure all code that previously usedparams.tileOwnerhas been updated.#!/bin/bash # Description: Search for any remaining references to tileOwner in MenuElementParams # Search for tileOwner references rg -A 3 "tileOwner" --type tssrc/client/graphics/layers/MainRadialMenu.ts (2)
76-100: Clean event handling implementationGood refactor: directly handling
ContextMenuEventsimplifies the architecture and removes unnecessary abstraction.
188-204: Well-structured menu cleanupThe
closeMenumethod properly handles all related UI elements with appropriate visibility checks.src/client/graphics/layers/RadialMenu.ts (3)
30-30: Clean state management refactorGood use of typed union for
CenterButtonStateinstead of multiple boolean flags. This makes state transitions clearer.Also applies to: 47-48
840-887: Well-implemented center button state managementThe unified state management with clear visual feedback improves code maintainability.
922-932: Good API design for menu configurationThe updated signature properly encapsulates both menu items and center button configuration.
| const disabled = | ||
| this.params === null || | ||
| this.params.game.inSpawnPhase() || | ||
| d.data.disabled(this.params); |
There was a problem hiding this comment.
Remove redundant spawn phase check
The spawn phase check here is redundant since individual menu elements already handle this in their disabled methods. This creates duplicate logic.
const disabled =
this.params === null ||
- this.params.game.inSpawnPhase() ||
d.data.disabled(this.params);📝 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.
| const disabled = | |
| this.params === null || | |
| this.params.game.inSpawnPhase() || | |
| d.data.disabled(this.params); | |
| const disabled = | |
| this.params === null || | |
| d.data.disabled(this.params); |
🤖 Prompt for AI Agents
In src/client/graphics/layers/RadialMenu.ts around lines 527 to 530, remove the
redundant check for spawn phase (this.params.game.inSpawnPhase()) from the
disabled variable assignment, since individual menu elements already handle this
condition in their disabled methods. Update the disabled assignment to only
check if this.params is null or if d.data.disabled(this.params) returns true.
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
evanpelle
force-pushed
the
evan-radial-disable
branch
from
66fdcdd to
877b4c3
Compare
evanpelle
force-pushed
the
evan-radial-disable
branch
from
24226c1 to
a459213
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/client/graphics/layers/RadialMenu.ts (1)
527-530: Remove redundant spawn phase check.The spawn phase check here is redundant since individual menu elements already handle this in their
disabledmethods. This creates duplicate logic.const disabled = this.params === null || - this.params.game.inSpawnPhase() || d.data.disabled(this.params);
🧹 Nitpick comments (1)
src/client/graphics/layers/RadialMenuElements.ts (1)
407-437: Center button logic is correct but could be simplified.The implementation handles spawn/attack logic correctly. Consider extracting the disabled logic into smaller, named functions for better readability:
+ private isWaterTile = (params: MenuElementParams): boolean => + !params.game.isLand(params.tile); + + private isSpawnPhaseWithPlayerTile = (params: MenuElementParams): boolean => + params.game.inSpawnPhase() && params.game.owner(params.tile).isPlayer(); disabled: (params: MenuElementParams): boolean => { - const tileOwner = params.game.owner(params.tile); - const isLand = params.game.isLand(params.tile); - if (!isLand) { - return true; - } - if (params.game.inSpawnPhase()) { - if (tileOwner.isPlayer()) { - return true; - } - return false; - } - return false; + return this.isWaterTile(params) || this.isSpawnPhaseWithPlayerTile(params); },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/client/graphics/GameRenderer.ts(0 hunks)src/client/graphics/layers/MainRadialMenu.ts(5 hunks)src/client/graphics/layers/MenuEventManager.ts(0 hunks)src/client/graphics/layers/RadialMenu.ts(24 hunks)src/client/graphics/layers/RadialMenuElements.ts(8 hunks)
💤 Files with no reviewable changes (2)
- src/client/graphics/GameRenderer.ts
- src/client/graphics/layers/MenuEventManager.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Deploy to openfront.dev
🔇 Additional comments (6)
src/client/graphics/layers/RadialMenuElements.ts (2)
58-61: Good interface design!The
CenterButtonElementinterface follows composition principles and provides a clean abstraction for center button behavior.
439-443: Menu reordering looks good.Moving
infoMenuElementto the first position improves the menu layout as intended.src/client/graphics/layers/MainRadialMenu.ts (2)
74-99: Clean event handling implementation.The direct context menu event handling is well-structured and removes unnecessary complexity from the previous
MenuEventManagerapproach. The coordinate validation and async action fetching are handled correctly.
159-175: Excellent centralized cleanup method.The
closeMenumethod provides a clean way to hide all menu-related UI components with proper visibility checks. This prevents UI state inconsistencies.src/client/graphics/layers/RadialMenu.ts (2)
30-30: Excellent state management refactor.The unified
CenterButtonStateenum andcenterButtonElementapproach is much cleaner than the previous multiple boolean flags. This follows good TypeScript practices with typed unions.Also applies to: 47-48
818-830: Clean center button click handling.The state-based approach for handling center button clicks is well-implemented and much clearer than the previous logic.
evanpelle
force-pushed
the
evan-radial-disable
branch
from
a459213 to
1d59164
Compare
github-project-automation
bot
moved this from Development
to Complete
in OpenFront Release Management
Description:
Refactor & clean up the Radial menu.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
evan