fix: resource prod cap #1834
fix: resource prod cap #1834
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe pull request introduces several modifications across multiple files, primarily focusing on the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
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 |
![mentatbot[bot]](https://avatars.githubusercontent.com/in/837875?s=60&v=4){kind=small}
There was a problem hiding this comment.
The changes in this PR appear to address the issue of fixing the resource production cap. The modifications in ResourceChip.tsx improve the logic for updating the display balance and showing production rates, considering cases where inputs are depleted or production has stopped. The optimization in useStructures.tsx by moving getEntityName outside the function is good for performance. The minor correction in the console log message in index.ts improves clarity. Overall, these changes seem appropriate and aligned with the PR's goal. Consider adding comments to explain the new conditions in ResourceChip.tsx for better code readability, and ensure that moving getEntityName in useStructures.tsx doesn't affect its functionality. It might also be beneficial to add unit tests to verify the new behavior of ResourceChip, especially the conditions for displaying production rates and updating balances.
Thanks for using MentatBot. Give comments a 👍 or 👎 to help me improve!
| @@ -80,7 +80,7 @@ export const ResourceChip = ({ | |||
|
|
|||
| const interval = setInterval(() => { | |||
| setDisplayBalance((prevDisplayBalance) => { | |||
| if (Math.abs(netRate) > 0) { | |||
| if (Math.abs(netRate) > 0 && !isConsumingInputsWithoutOutput) { | |||
There was a problem hiding this comment.
Good addition of the !isConsumingInputsWithoutOutput check. Consider adding a comment explaining the purpose of this condition for better code readability.
| @@ -137,7 +137,7 @@ | |||
| : ""} | |||
| </div> | |||
|
|
|||
| {netRate && !reachedMaxCap ? ( | |||
| {netRate && !reachedMaxCap && !isConsumingInputsWithoutOutput && displayBalance > 0 ? ( | |||
There was a problem hiding this comment.
The additional checks !isConsumingInputsWithoutOutput && displayBalance > 0 are good improvements. Consider adding a comment explaining why these conditions are necessary for displaying the production rate.
| }, | ||
| } = useDojo(); | ||
|
|
||
| const { getAliveArmy } = getArmyByEntityId(); | ||
| const { getEntityName } = useEntitiesUtils(); |
There was a problem hiding this comment.
Good optimization moving getEntityName outside of getStructureByEntityId. Ensure this doesn't affect its functionality if it was relying on any local variables.
![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 and nitpick comments (2)
client/src/ui/components/resources/ResourceChip.tsx (2)
Line range hint
140-173: Approval: Improved display logic and user feedbackThese changes significantly improve the user interface by:
- Ensuring the net rate is only displayed when production is active and there's a positive balance.
- Adding a warning icon with informative tooltips when production has stopped.
These enhancements align well with the PR objective by providing clearer feedback on resource production status, particularly when resources are depleted or max capacity is reached.
For consistency, consider using template literals for the className in the warning icon div:
-className="self-center px-2 col-span-3 mx-auto" +className={`self-center px-2 col-span-3 mx-auto ${isConsumingInputsWithoutOutput || reachedMaxCap ? 'cursor-help' : ''}`}This change would make the cursor change to a help icon when hovering over the warning, consistent with the presence of a tooltip.
Line range hint
1-173: Summary: Effective solution to infinite resource production issueThe changes in this file successfully address the core issue described in the PR objectives:
- The balance update logic now prevents infinite production when inputs are depleted.
- The UI provides clear feedback on production status, including warnings when production has stopped due to depleted inputs or reached max capacity.
These modifications not only fix the bug but also improve the overall user experience by providing more accurate and informative resource production information.
To further improve the robustness of the solution, consider implementing server-side checks to ensure that resource production is always contingent on available inputs. This would provide an additional layer of protection against similar issues in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- client/src/hooks/helpers/useStructures.tsx (1 hunks)
- client/src/ui/components/resources/ResourceChip.tsx (2 hunks)
- sdk/packages/eternum/src/config/index.ts (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- sdk/packages/eternum/src/config/index.ts
🧰 Additional context used
🔇 Additional comments (2)
client/src/ui/components/resources/ResourceChip.tsx (1)
83-86: Approval: Prevents updating balance when inputs are depletedThis change effectively addresses the issue of infinite resource production by preventing balance updates when inputs are being consumed without producing output. It aligns well with the PR objective of fixing the bug that allowed Alchemical Silver to be produced without the necessary resources.
client/src/hooks/helpers/useStructures.tsx (1)
Line range hint
1-280: Clarify the relationship between these changes and the PR objectives.While the changes in this file improve code clarity and optimize imports, it's not immediately clear how they address the issue of infinite production of Alchemical Silver without the required resources (Twilight Quartz and Adamantine) as mentioned in the PR objectives.
Could you please clarify:
- How do these changes in the
useStructureshook contribute to fixing the production cap issue?- Are there other files or components that handle the actual resource production logic?
- Is this part of a larger refactoring effort that indirectly contributes to fixing the production cap issue?
This information would help reviewers better understand the context and impact of these changes in relation to the stated objectives of the PR.
| @@ -162,11 +162,12 @@ export const useStructures = () => { | |||
| const { | |||
| account: { account }, | |||
| setup: { | |||
| components: { Structure, EntityOwner, Owner, Protector, EntityName, Realm, Position, AddressName }, | |||
| components: { Structure, EntityOwner, Owner, Protector, Position, AddressName }, | |||
There was a problem hiding this comment.
💡 Codebase verification
EntityName and Realm Still Present After Intended Removal
The useStructures.tsx file still includes EntityName and Realm in the destructured components. Please remove these to simplify dependencies as intended.
- Line 165:
components: { Structure, EntityOwner, Owner, Protector, EntityName, Realm },
- Additional Instances:
import { getRealm } from "@/ui/utils/realms"; const { getEntityName } = useEntitiesUtils(); const name = getEntityName(structure.entity_id);
🔗 Analysis chain
LGTM! Verify impact on other parts of the codebase.
The removal of EntityName and Realm from the destructured components simplifies the dependencies in the useStructures hook, which is a good practice. This change, along with the direct import of getEntityName, aligns with the goal of optimizing code clarity.
However, these changes don't seem to directly address the issue of infinite production of Alchemical Silver mentioned in the PR objectives.
To ensure this change doesn't have unintended consequences, please run the following script to check for any remaining usages of EntityName and Realm in this file:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining usages of EntityName and Realm in the file
# Test: Search for EntityName and Realm usage
rg --type typescript 'EntityName|Realm' client/src/hooks/helpers/useStructures.tsx
Length of output: 119
Script:
#!/bin/bash
# Description: Check for any remaining usages of EntityName and Realm in the file
# Test: Search for EntityName and Realm usage
rg --type ts 'EntityName|Realm' client/src/hooks/helpers/useStructures.tsx
Length of output: 964
Closes #1819
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
useStructureshook by optimizing component dependencies and imports for improved code clarity.