fix bugs #2659
fix bugs #2659
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
|
Hi @aymericdelab! You need to be added as a user to interact with me. Please ask @ponderingdemocritus to add you on the settings page. You are receiving this comment because I am set to review all PRs. That is configurable here. |
|
Caution Review failedThe pull request is closed. Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
client/apps/game/src/ui/elements/number-input.tsxOops! Something went wrong! :( ESLint: 9.18.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by WalkthroughThis pull request encompasses multiple changes across different files in the game configuration and client-side code. The modifications include updating quest-related hooks, removing console log statements, simplifying troop management logic, and introducing a new constant for troop food consumption. The most significant change is the deletion of the Changes
Sequence DiagramsequenceDiagram
participant Client as Game Client
participant Hooks as Quest Hooks
participant Config as Configuration
Client->>Hooks: Request quest status
Hooks->>Config: Retrieve quest configuration
Config-->>Hooks: Return quest configuration
Hooks->>Hooks: Compute memoized quests
Hooks-->>Client: Return updated quests
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
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 (
|
|
Failed to generate code suggestions for PR |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
client/apps/game/src/hooks/helpers/use-quests.ts (1)
Remove duplicate
QuestStatusenum definitionThe
QuestStatusenum inuse-quests.tsis indeed unused as flagged by knip. This is because it's already defined in@bibliothecadao/eternum(packages/core/src/types/common.ts), and all other files are importing it from there. You should:
- Remove the local enum definition
- Import
QuestStatusfrom@bibliothecadao/eternuminstead🔗 Analysis chain
Line range hint
10-14: Verify ifQuestStatusexport is needed.The
QuestStatusenum is flagged as unused by the knip tool. While it's used internally in this file, please verify if the export is necessary for other components.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for QuestStatus usage in other files rg "QuestStatus" --type tsLength of output: 1633
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
client/apps/game/src/hooks/helpers/use-quests.ts(1 hunks)client/apps/game/src/hooks/helpers/use-starting-tutorial.ts(1 hunks)client/apps/game/src/ui/components/military/army-management-card.tsx(2 hunks)config/deployer/config.ts(1 hunks)config/environments/_shared_.ts(0 hunks)config/environments/index.ts(0 hunks)config/environments/utils/troop.ts(1 hunks)
💤 Files with no reviewable changes (2)
- config/environments/shared.ts
- config/environments/index.ts
✅ Files skipped from review due to trivial changes (1)
- client/apps/game/src/hooks/helpers/use-starting-tutorial.ts
🧰 Additional context used
🪛 GitHub Actions: knip
client/apps/game/src/hooks/helpers/use-quests.ts
[warning] 10-10: Unused exported type 'QuestStatus'
🔇 Additional comments (4)
client/apps/game/src/hooks/helpers/use-quests.ts (1)
36-36: LGTM! Correct dependency array update.Adding
questStatusto the dependency array is necessary since the memoized value depends onquestStatus.questClaimStatus. This fix ensures the quests array updates properly when the claim status changes.config/environments/utils/troop.ts (2)
9-10: Well-documented constant with clear purpose and example!The comment clearly explains the purpose of the constant and provides a helpful example of how the consumption rates relate to resource precision.
Line range hint
11-31: Verify food consumption rates across troop types.The consumption rates show that:
- Paladin and Knight have identical exploration costs (10 each for wheat/fish)
- Knight has higher travel costs than Paladin (5 vs 4)
- Crossbowman has significantly lower costs overall
Please verify if these relative consumption rates align with the intended game balance.
✅ Verification successful
Food consumption rates correctly implement documented troop characteristics
The consumption rates in the code accurately reflect the documented game design:
- Crossbowmen's 50% reduced rates (6/3 vs standard 10/5)
- Knights as the baseline with standard rates (10/5)
- Paladins with efficient travel (4 vs 5) matching their "extended movement" trait
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any references to troop food consumption rates in documentation or balance discussions rg -A 5 "food.*consumption|consumption.*rate" .Length of output: 10611
Script:
#!/bin/bash # Search for documentation about specific troop types and their characteristics rg -i -A 5 "paladin|knight|crossbow" ./client/apps/game-docsLength of output: 9595
client/apps/game/src/ui/components/military/army-management-card.tsx (1)
244-244: Verify the impact of simplified troop count calculation.The max troop count calculation has been simplified to only consider the balance. Please verify that removing the additional conditions doesn't impact:
- Defending army restrictions
- Remaining troop count limitations
- Army status-based restrictions
✅ Verification successful
Troop count restrictions remain properly enforced
The simplified max calculation is safe as the core troop count restrictions are enforced at the contract and system levels through:
- Global max troop count in TroopConfig
- Battle system validations
- Army management restrictions
- Troop count normalization
The balance-based limit is sufficient at the UI level since the underlying systems will enforce all necessary restrictions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for any other troop count restrictions or validations rg -A 5 "max.*troop|troop.*count|army.*restrict" .Length of output: 70963
| explore_wheat_burn_amount: costs.explore_wheat_burn_amount, | ||
| explore_fish_burn_amount: costs.explore_fish_burn_amount, | ||
| travel_wheat_burn_amount: costs.travel_wheat_burn_amount, | ||
| travel_fish_burn_amount: costs.travel_fish_burn_amount, |
There was a problem hiding this comment.
💡 Codebase verification
Food burn amounts should maintain resource precision multiplication
The food consumption values should be multiplied by resourcePrecision to maintain consistency with how other resource amounts are handled throughout the config. All other resource-related values (rewards, costs, bounds) are consistently scaled by resourcePrecision.
- Consider updating lines 775-778 to multiply each burn amount by
config.config.resources.resourcePrecision
🔗 Analysis chain
Verify removal of resource precision multiplication.
The food consumption values are now used directly without multiplying by resourcePrecision. This change needs verification:
- Does this align with how other resource amounts are handled in the config?
- Could this lead to precision loss in calculations?
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how resource precision is handled in other config settings
rg "resourcePrecision.*\*|config\.resources\.resourcePrecision" .
Length of output: 2555
Summary by CodeRabbit
Based on the comprehensive summary, here are the updated release notes:
Performance Improvements
Configuration Updates
Code Cleanup
These updates enhance the application's performance and maintainability while providing more responsive quest and troop management functionality.