-
Notifications
You must be signed in to change notification settings - Fork 51
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
min troops #2424
min troops #2424
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
WalkthroughThe changes in this pull request involve modifications to the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
You are out of MentatBot reviews. Your usage will refresh December 16 at 08:00 AM. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
client/src/dojo/modelManager/BattleManager.ts (2)
317-323
: Consider extracting the troop calculation logicThe total troops calculation is duplicated in both
isRaidable
andisAttackable
methods.Consider extracting this into a private method:
+ private calculateTotalTroops(army: ArmyInfo | undefined): number { + return army?.troops + ? Object.values(army.troops).reduce((sum, count) => sum + Number(count), 0) + : 0; + } public isRaidable(...): RaidStatus { // ... - const totalTroops = selectedArmy.troops - ? Object.values(selectedArmy.troops).reduce((sum, count) => sum + Number(count), 0) - : 0; + const totalTroops = this.calculateTotalTroops(selectedArmy); if (totalTroops < MIN_TROOPS_BATTLE) return RaidStatus.MinTroops; // ... }
335-343
: Use extracted method for consistencySimilar to the previous comment, use the extracted method here as well.
Apply the same refactoring:
public isAttackable(...): BattleStartStatus { if (!selectedArmy) return BattleStartStatus.NothingToAttack; - const totalTroops = selectedArmy.troops - ? Object.values(selectedArmy.troops).reduce((sum, count) => sum + Number(count), 0) - : 0; + const totalTroops = this.calculateTotalTroops(selectedArmy); if (totalTroops < MIN_TROOPS_BATTLE) return BattleStartStatus.MinTroops; // ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
client/src/dojo/modelManager/BattleManager.ts
(5 hunks)client/src/ui/modules/military/battle-view/BattleActions.tsx
(1 hunks)sdk/packages/eternum/src/constants/global.ts
(1 hunks)
🔇 Additional comments (4)
sdk/packages/eternum/src/constants/global.ts (1)
17-17
: Verify the minimum troop requirement value
The MIN_TROOPS_BATTLE
constant is set to 100,000 troops, which seems quite high and could significantly impact gameplay. Please verify if this value aligns with the game design requirements.
Consider making this value configurable per environment (development/staging/production) to allow for easier testing and balancing.
client/src/ui/modules/military/battle-view/BattleActions.tsx (2)
251-253
: LGTM: Improved tooltip text alignment
The centered tooltip text provides better visual feedback.
255-260
: LGTM: Clear warning about army losses
Good addition of a warning message about the 25% army loss during raids. This helps users make informed decisions.
client/src/dojo/modelManager/BattleManager.ts (1)
41-41
: LGTM: Clear status messages for minimum troop requirement
The addition of MinTroops
status to both enums with consistent messaging is good for user feedback.
Also applies to: 52-52
Failed to generate code suggestions for PR |
Summary by CodeRabbit
New Features
Bug Fixes
Documentation