-
Notifications
You must be signed in to change notification settings - Fork 52
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
refactor armies hook + prettier #2644
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. |
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/components/worldmap/structures/structure-label.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 introduces significant changes to how army data is retrieved and managed across multiple components in the game client. The primary modifications involve replacing hook-based army data fetching with utility functions, specifically Changes
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 |
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: 1
🧹 Nitpick comments (2)
client/apps/game/src/utils/army.ts (1)
121-132
: Consider adding an index for battle_id.The query conditions are comprehensive, but querying armies by battle_id could benefit from an index to improve performance, especially as the number of armies grows.
client/apps/game/src/ui/components/structures/worldmap/structure-card.tsx (1)
324-327
: Consider memoizing the troops deduction calculation.The troops deduction calculation could be memoized to prevent unnecessary recalculations on each render.
+ const deductedTroops = useMemo( + () => Object.values(troopsGiven).reduce((a, b) => a + b, 0n), + [troopsGiven] + ); <ArmyCapacity army={transferDirection === "to" ? giverArmy : protectorArmy} className="flex justify-center" - deductedTroops={Object.values(troopsGiven).reduce((a, b) => a + b, 0n)} + deductedTroops={deductedTroops} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
client/apps/game/src/hooks/helpers/use-armies.tsx
(3 hunks)client/apps/game/src/hooks/helpers/use-structures.tsx
(5 hunks)client/apps/game/src/ui/components/entities/entity.tsx
(2 hunks)client/apps/game/src/ui/components/structures/worldmap/structure-card.tsx
(4 hunks)client/apps/game/src/ui/components/trading/market-modal.tsx
(2 hunks)client/apps/game/src/ui/components/worldmap/armies/army-info-label.tsx
(1 hunks)client/apps/game/src/ui/components/worldmap/structures/structure-list-item.tsx
(2 hunks)client/apps/game/src/ui/modules/military/battle-view/battle-actions.tsx
(4 hunks)client/apps/game/src/ui/modules/military/battle-view/battle-history.tsx
(3 hunks)client/apps/game/src/ui/modules/military/battle-view/battle-side-view.tsx
(2 hunks)client/apps/game/src/ui/modules/military/battle-view/battle-view.tsx
(3 hunks)client/apps/game/src/ui/utils/leaderboard.tsx
(0 hunks)client/apps/game/src/utils/army.ts
(3 hunks)
💤 Files with no reviewable changes (1)
- client/apps/game/src/ui/utils/leaderboard.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Run pr agent on every pull request, respond to user comments
🔇 Additional comments (23)
client/apps/game/src/hooks/helpers/use-armies.tsx (4)
3-3
: ImportingContractAddress
,ID
, andPosition
The import statement correctly adds
ContractAddress
,ID
, andPosition
from@bibliothecadao/eternum
.
5-5
: Importing from@dojoengine/recs
The import of
Has
,HasValue
,Not
, andNotValue
from@dojoengine/recs
is appropriate.
23-23
: Ensureaccount.address
is defined before usingContractAddress
Using
ContractAddress(account.address)
may cause errors ifaccount.address
isundefined
. Consider verifying thataccount.address
is defined before using it.
46-46
: Ensureaccount.address
is defined before usingContractAddress
Similarly, ensure that
account.address
is defined before callingContractAddress(account.address)
to prevent potential runtime errors.client/apps/game/src/ui/components/entities/entity.tsx (2)
7-7
: ImportinggetArmy
from@/utils/army
The import statement correctly includes
getArmy
from the utility functions.
46-49
: Ensuredojo.account.account.address
is defined before usingContractAddress
Consider verifying that
dojo.account.account.address
is defined to avoid potential errors when callingContractAddress(dojo.account.account.address)
.client/apps/game/src/ui/components/worldmap/armies/army-info-label.tsx (4)
15-15
: ImportinggetArmy
from@/utils/army
The addition of
getArmy
to the imports is appropriate and aligns with the refactoring effort.
17-17
: Updating imports to includeContractAddress
Including
ContractAddress
in the import statement from@bibliothecadao/eternum
is correct.
22-25
: Destructuringaccount
andcomponents
fromuseDojo
Properly destructuring
account
andcomponents
enhances code readability.
31-33
: Ensureaccount.address
is defined before usingContractAddress
Verify that
account.address
is defined before using it inContractAddress(account.address)
to prevent unexpected errors.client/apps/game/src/utils/army.ts (2)
14-18
: LGTM! Type safety and data handling improvements.The changes improve the code by:
- Using ContractAddress type instead of string for better type safety
- Using structuredClone to prevent mutation of original data
- Simplifying the isMine check with direct comparison
Also applies to: 82-83
113-119
: LGTM! Well-designed utility function.The new
getArmy
function is concise, follows SRP, and maintains type safety while reusing the existingformatArmies
function.client/apps/game/src/ui/modules/military/battle-view/battle-view.tsx (2)
20-23
: LGTM! Proper memoization of updatedTarget.The dependencies array includes all required values: targetArmy, account.address, and components.
44-52
: LGTM! Proper memoization of armiesInBattle.The dependencies array correctly includes battleEntityId, account.address, and components.
client/apps/game/src/ui/modules/military/battle-view/battle-side-view.tsx (1)
59-62
: LGTM! Proper memoization of ownArmy.The dependencies array includes all required values: ownArmyEntityId, account.address, and components.
client/apps/game/src/ui/modules/military/battle-view/battle-history.tsx (1)
76-79
: LGTM! Proper memoization of doerArmy.The dependencies array includes all required values: doerArmyEntityId, account.address, and components.
client/apps/game/src/ui/components/worldmap/structures/structure-list-item.tsx (1)
12-12
: LGTM! The army data retrieval has been refactored correctly.The changes properly integrate the new
getArmiesInBattle
utility function with appropriate memoization and dependencies.Also applies to: 65-73
client/apps/game/src/hooks/helpers/use-structures.tsx (1)
5-5
: LGTM! The army data retrieval has been standardized.The changes consistently update all instances of
getArmy
with the required parameters (ContractAddress
andcomponents
), aligning with the broader refactoring effort.Also applies to: 42-44, 86-88, 126-128, 173-175
client/apps/game/src/ui/components/trading/market-modal.tsx (1)
27-28
: LGTM! The bank's army data retrieval has been refactored correctly.The changes properly integrate the new
getArmy
utility function with appropriate memoization and dependencies.Also applies to: 109-117
client/apps/game/src/ui/modules/military/battle-view/battle-actions.tsx (1)
16-16
: LGTM! The army data retrieval has been standardized.The changes consistently update the usage of
getArmy
with the required parameters and properly referencecomponents.TroopConfig
.Also applies to: 23-23, 64-64, 91-91, 227-227
client/apps/game/src/ui/components/structures/worldmap/structure-card.tsx (3)
24-24
: LGTM! Good architectural improvement.The shift from hook-based army data fetching to utility functions aligns with the PR objectives and improves code maintainability.
266-272
: Well-structured army data management!Good implementation with:
- Effective memoization to prevent unnecessary recalculations
- Proper type safety using ContractAddress wrapper
- Centralized army data fetching
277-278
: Add validation for undefined armies.While the direction logic is clean, consider adding validation to handle cases where either army could be undefined.
- const fromArmy = transferDirection === "to" ? giverArmy : protectorArmy; - const toArmy = transferDirection === "to" ? protectorArmy : giverArmy; + const fromArmy = transferDirection === "to" ? giverArmy : protectorArmy; + const toArmy = transferDirection === "to" ? protectorArmy : giverArmy; + if (!fromArmy || !toArmy) { + console.error('Source or destination army is undefined'); + return; + }
() => | ||
getArmy(battleView?.ownArmyEntityId || 0, ContractAddress(dojo.account.account.address), dojo.setup.components), | ||
[battleView?.ownArmyEntityId || 0], |
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.
Fix useMemo dependencies for ownArmyBattleStarter.
The dependencies array is missing account.address
and components
, which could lead to stale data.
() =>
getArmy(battleView?.ownArmyEntityId || 0, ContractAddress(dojo.account.account.address), dojo.setup.components),
- [battleView?.ownArmyEntityId || 0],
+ [battleView?.ownArmyEntityId, dojo.account.account.address, dojo.setup.components],
📝 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.
() => | |
getArmy(battleView?.ownArmyEntityId || 0, ContractAddress(dojo.account.account.address), dojo.setup.components), | |
[battleView?.ownArmyEntityId || 0], | |
() => | |
getArmy(battleView?.ownArmyEntityId || 0, ContractAddress(dojo.account.account.address), dojo.setup.components), | |
[battleView?.ownArmyEntityId, dojo.account.account.address, dojo.setup.components], |
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: 2
🧹 Nitpick comments (10)
client/apps/game/src/ui/modules/military/military.tsx (1)
20-23
: LGTM! Proper memoization with correct dependencies.The implementation:
- Correctly memoizes structure retrieval
- Includes all necessary dependencies
- Properly handles undefined entityId
Consider adding error boundaries or loading states for structure retrieval failures.
const selectedStructure = useMemo( - () => (entityId ? getStructure(entityId, ContractAddress(address), components) : undefined), + () => { + try { + return entityId ? getStructure(entityId, ContractAddress(address), components) : undefined; + } catch (error) { + console.error('Failed to retrieve structure:', error); + return undefined; + } + }, [entityId, address, components], );client/apps/game/src/ui/components/worldmap/structures/structure-label.tsx (1)
43-48
: Consider memoizing the contract address.The contract address could be memoized separately to avoid unnecessary recalculations.
+ const contractAddress = useMemo( + () => ContractAddress(dojo.account.account.address), + [dojo.account.account.address] + ); const structure = useMemo(() => { return getStructure( hoveredStructure?.entityId || 0, - ContractAddress(dojo.account.account.address), + contractAddress, dojo.setup.components, ); - }, [hoveredStructure]); + }, [hoveredStructure, contractAddress, dojo.setup.components]);client/apps/game/src/ui/components/resources/deposit-resources.tsx (1)
31-37
: Consider memoizing the contract address for consistency.Similar to other components, the contract address could be memoized separately.
+ const contractAddress = useMemo( + () => ContractAddress(dojo.account.account.address), + [dojo.account.account.address] + ); const structureAtPosition = useMemo(() => { return getStructure( arrival.recipientEntityId || 0, - ContractAddress(dojo.account.account.address), + contractAddress, dojo.setup.components, ); - }, [arrival.recipientEntityId, dojo.account.account.address, dojo.setup.components]); + }, [arrival.recipientEntityId, contractAddress, dojo.setup.components]);client/apps/game/src/utils/structure.ts (2)
104-104
: Remove redundant null check.The null check is redundant since the return statement already handles the case when
nextBlockTimestamp
is falsy.- if (!nextBlockTimestamp) return 0; return immunityEndTimestamp - nextBlockTimestamp!;
18-106
: Add JSDoc comments for better documentation.Consider adding JSDoc comments to document the parameters and return values of the exported functions.
+/** + * Retrieves a structure at a specified position for a given player. + * @param position - The position to check for a structure. + * @param playerAddress - The address of the player. + * @param components - The client components. + * @returns The structure at the position, if any. + */ export const getStructureAtPosition = ( { x, y }: Position, playerAddress: ContractAddress, components: ClientComponents, ): Structure | undefined => {client/apps/game/src/ui/modules/entity-details/realm/realm-details.tsx (1)
98-98
: Consider handling undefined owner name more explicitly.The current implementation passes an empty string when
ownerName
is undefined. Consider using nullish coalescing for better clarity.-onClick={() => copyPlayerAddressToClipboard(structure.owner.address, structure.ownerName || "")} +onClick={() => copyPlayerAddressToClipboard(structure.owner.address, structure.ownerName ?? "")}client/apps/game/src/ui/modules/entity-details/enemy-armies.tsx (1)
64-64
: Consider memoizing the immunity check.The immunity check within
getArmyChip
could benefit from memoization to prevent recalculation on each render.+const armyImmune = useMemo( + () => isStructureImmune(structure, nextBlockTimestamp!) || ownArmyIsImmune, + [structure, nextBlockTimestamp, ownArmyIsImmune] +); -const isImmune = isStructureImmune(structure, nextBlockTimestamp!) || ownArmyIsImmune; +const isImmune = armyImmune;client/apps/game/src/ui/modules/entity-details/combat-entity-details.tsx (1)
43-51
: Consider extracting position contract calculation.The
hexPosition.getContract()
call within the memoized function could be moved outside to prevent recalculation on each render.+const positionContract = useMemo( + () => hexPosition.getContract(), + [hexPosition] +); const structure = useMemo( () => getStructureAtPosition( - hexPosition.getContract(), + positionContract, ContractAddress(dojo.account.account.address), dojo.setup.components, ), - [hexPosition, dojo.account.account.address, dojo.setup.components], + [positionContract, dojo.account.account.address, dojo.setup.components], );client/apps/game/src/ui/components/hyperstructures/co-owners.tsx (1)
83-86
: Improved performance with memoized structure retrieval.The structure retrieval has been optimized by replacing the hook with a memoized
getStructure
call. The memoization dependencies are correctly set to recompute only when necessary (hyperstructureEntityId, account.address, or components change).client/apps/game/src/ui/components/worldmap/armies/army-info-label.tsx (1)
59-59
: Verify the dependency array for structure memoization.The structure retrieval is not memoized, which could lead to unnecessary recalculations.
Consider wrapping the
getStructure
call in auseMemo
hook:- return getStructure(entityOwner.entity_owner_id, ContractAddress(address), components); + return useMemo( + () => getStructure(entityOwner.entity_owner_id, ContractAddress(address), components), + [entityOwner.entity_owner_id, address, components] + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
client/apps/game/src/hooks/helpers/use-entities.tsx
(2 hunks)client/apps/game/src/hooks/helpers/use-resources.tsx
(2 hunks)client/apps/game/src/hooks/helpers/use-structures.tsx
(0 hunks)client/apps/game/src/ui/components/bank/add-liquidity.tsx
(2 hunks)client/apps/game/src/ui/components/bank/swap.tsx
(4 hunks)client/apps/game/src/ui/components/hyperstructures/co-owners.tsx
(2 hunks)client/apps/game/src/ui/components/resources/deposit-resources.tsx
(2 hunks)client/apps/game/src/ui/components/structures/worldmap/structure-card.tsx
(8 hunks)client/apps/game/src/ui/components/trading/market-modal.tsx
(3 hunks)client/apps/game/src/ui/components/trading/market-order-panel.tsx
(3 hunks)client/apps/game/src/ui/components/worldmap/armies/army-info-label.tsx
(4 hunks)client/apps/game/src/ui/components/worldmap/battles/battle-label.tsx
(1 hunks)client/apps/game/src/ui/components/worldmap/structures/structure-label.tsx
(3 hunks)client/apps/game/src/ui/components/worldmap/structures/structure-list-item.tsx
(2 hunks)client/apps/game/src/ui/modules/entity-details/combat-entity-details.tsx
(3 hunks)client/apps/game/src/ui/modules/entity-details/enemy-armies.tsx
(4 hunks)client/apps/game/src/ui/modules/entity-details/realm/castle.tsx
(2 hunks)client/apps/game/src/ui/modules/entity-details/realm/realm-details.tsx
(4 hunks)client/apps/game/src/ui/modules/military/battle-view/battle-view.tsx
(4 hunks)client/apps/game/src/ui/modules/military/military.tsx
(1 hunks)client/apps/game/src/utils/entities.ts
(0 hunks)client/apps/game/src/utils/realm.ts
(0 hunks)client/apps/game/src/utils/structure.ts
(1 hunks)client/sdk/packages/eternum/src/modelManager/types/index.ts
(1 hunks)
💤 Files with no reviewable changes (3)
- client/apps/game/src/utils/entities.ts
- client/apps/game/src/utils/realm.ts
- client/apps/game/src/hooks/helpers/use-structures.tsx
🧰 Additional context used
📓 Learnings (1)
client/apps/game/src/ui/components/worldmap/structures/structure-label.tsx (1)
Learnt from: aymericdelab
PR: BibliothecaDAO/eternum#1818
File: client/src/ui/components/worldmap/structures/StructureLabel.tsx:47-0
Timestamp: 2024-11-12T03:28:50.421Z
Learning: In `StructureLabel.tsx`, when `timer` is negative, the immunity message is not shown, so handling negative `timer` values isn't necessary.
🔇 Additional comments (37)
client/apps/game/src/hooks/helpers/use-resources.tsx (1)
23-41
: LGTM! Well-structured hook implementation with proper memoization.The implementation correctly:
- Handles undefined battle IDs with a fallback
- Uses memoization with appropriate dependencies
- Integrates with the battle manager for resource locking checks
However, consider adding error handling for the case when
nextBlockTimestamp
is undefined.✅ Verification successful
Non-null assertion is safe, no changes needed
The
nextBlockTimestamp!
usage is safe because the value is initialized immediately with the current timestamp and is continuously updated via an interval, ensuring it's always defined. This is consistent with the pattern used throughout the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for potential undefined nextBlockTimestamp usage rg -l "useNextBlockTimestamp" | xargs rg "nextBlockTimestamp!"Length of output: 1518
Script:
#!/bin/bash # Check the implementation of useNextBlockTimestamp ast-grep --pattern 'export const useNextBlockTimestamp = () => { $$$ }'Length of output: 75
Script:
#!/bin/bash # Find the file containing useNextBlockTimestamp implementation rg -l "useNextBlockTimestamp" --type ts --type tsx # Try to find the implementation with a more flexible pattern ast-grep --pattern 'useNextBlockTimestamp'Length of output: 7362
Script:
#!/bin/bash # Get the implementation of useNextBlockTimestamp cat client/apps/game/src/hooks/use-next-block-timestamp.tsxLength of output: 1372
client/apps/game/src/hooks/helpers/use-entities.tsx (1)
45-46
: Type assertion change improves type safety.The update to use
Structure
type and includeContractAddress
aligns with the broader refactoring effort.However, verify that all consumers of
usePlayerStructures
are compatible with the newStructure
type.✅ Verification successful
Type assertion change is safe and properly implemented.
The change from
PlayerStructure
toStructure
type is consistent across all consumers and properly type-guarded through the filter assertion. All components access only valid properties of theStructure
type from@bibliothecadao/eternum
.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find all usages of usePlayerStructures to verify type compatibility rg "usePlayerStructures" --type typescriptLength of output: 77
Script:
#!/bin/bash # Search for usePlayerStructures in ts/tsx files rg "usePlayerStructures" -t ts -t tsx -A 3 # Search for Structure type usage rg "Structure(\s+|[^\w])" -t ts -t tsx -A 2 # Find the original hook definition fd -e ts -e tsx | xargs ast-grep --pattern 'export function usePlayerStructures'Length of output: 227
Script:
#!/bin/bash # Find TypeScript files containing usePlayerStructures fd -e ts -e tsx | xargs grep -l "usePlayerStructures" # Search for Structure type usage in TypeScript files fd -e ts -e tsx | xargs grep -A 2 "Structure" # Look for the hook definition fd -e ts -e tsx | xargs grep -A 5 "export.*usePlayerStructures"Length of output: 66495
client/sdk/packages/eternum/src/modelManager/types/index.ts (1)
42-42
: LGTM! Position property addition enhances Structure type.The addition of the position property with proper typing from ClientComponents improves type safety and accessibility of structure location data.
Verify that existing code properly handles this new required property.
✅ Verification successful
Position property is properly handled throughout the codebase
The position property is consistently used and properly handled across all structure-related functionality including placement, combat, resource management, and UI rendering. No issues found with the property addition.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find Structure type usage to verify position property handling ast-grep --pattern 'Structure = $_' --lang typescriptLength of output: 55
Script:
#!/bin/bash # Search for Structure type/interface declarations ast-grep --pattern 'type Structure = $_' --lang typescript ast-grep --pattern 'interface Structure $_' --lang typescript # Find Structure type references in type annotations ast-grep --pattern ': Structure' --lang typescript # Look for Structure imports rg "import.*Structure.*from" --type typescriptLength of output: 2077
Script:
#!/bin/bash # Find Structure type usage and imports with correct ripgrep syntax rg "Structure" --type ts # Find variable declarations of Structure type ast-grep --pattern '$_: Structure = $_' --lang typescriptLength of output: 75051
client/apps/game/src/ui/components/worldmap/structures/structure-label.tsx (1)
Line range hint
14-33
: LGTM! Well-structured immunity timer implementation.The refactored
ImmunityTimer
component effectively encapsulates immunity-related calculations using memoization for better performance.client/apps/game/src/ui/components/resources/deposit-resources.tsx (1)
31-37
: LGTM! Well-implemented structure retrieval.The structure retrieval is correctly memoized with proper dependency tracking.
client/apps/game/src/utils/structure.ts (1)
18-38
: LGTM! Well-implemented structure retrieval functions.The structure retrieval functions are well-organized and handle edge cases properly.
client/apps/game/src/ui/modules/entity-details/realm/castle.tsx (1)
32-35
: LGTM! Effective use of memoization.The replacement of the hook with a memoized utility function call is well-implemented. The dependency array correctly includes all required dependencies:
structureEntityId
,dojo.account.account.address
, anddojo.setup.components
.client/apps/game/src/ui/modules/entity-details/realm/realm-details.tsx (2)
22-25
: LGTM! Well-structured memoization.The structure retrieval has been effectively refactored to use memoization with appropriate dependencies.
52-59
: LGTM! Efficient immunity checks.The immunity checks have been well-implemented using memoization, preventing unnecessary recalculations.
client/apps/game/src/ui/modules/entity-details/enemy-armies.tsx (2)
23-31
: LGTM! Well-structured position-based structure retrieval.The memoized structure retrieval is well-implemented with appropriate dependencies.
48-51
: LGTM! Efficient immunity check implementation.The immunity check is correctly memoized with appropriate dependencies.
client/apps/game/src/ui/components/bank/add-liquidity.tsx (2)
3-3
: Import updated to use structure-specific resource locking.The import has been updated to use
useIsStructureResourcesLocked
from the resources helper, which better reflects its structure-specific functionality.
69-70
: Resource locking checks updated to use structure-specific hook.The resource locking checks now use
useIsStructureResourcesLocked
, providing more explicit and focused functionality for checking if resources are locked for specific structures.client/apps/game/src/ui/components/bank/swap.tsx (3)
4-4
: Imports updated for better modularity.The imports have been updated to use more specific modules:
useIsStructureResourcesLocked
from resources helpergetStructure
from structure utilityAlso applies to: 14-14
53-53
: Structure retrieval optimized.Structure retrieval now uses the direct
getStructure
utility function with proper parameters.
93-94
: Resource locking checks updated to use structure-specific hook.The resource locking checks now use
useIsStructureResourcesLocked
, providing more explicit functionality for checking if resources are locked for specific structures.client/apps/game/src/ui/components/trading/market-order-panel.tsx (2)
2-2
: Import updated to use structure-specific hooks.The import has been updated to use
useIsStructureResourcesLocked
anduseResourceManager
from the resources helper, providing more focused functionality.
116-116
: Resource locking checks updated to use structure-specific hook.The resource locking checks in both MarketOrderPanel and OrderRow components now use
useIsStructureResourcesLocked
, providing more explicit functionality for checking if resources are locked for specific structures.Also applies to: 267-267
client/apps/game/src/ui/components/worldmap/armies/army-info-label.tsx (1)
13-15
: LGTM! Imports are well organized.The imports are logically grouped and follow a consistent pattern.
client/apps/game/src/ui/modules/military/battle-view/battle-view.tsx (6)
6-8
: LGTM! Imports are well organized.The imports are logically grouped and follow a consistent pattern.
19-22
: LGTM! Proper memoization of updatedTarget.The dependency array correctly includes all dependencies:
battleView?.targetArmy
,dojo.account.account.address
, anddojo.setup.components
.
43-51
: LGTM! Proper memoization of armiesInBattle.The dependency array correctly includes all dependencies:
battleManager.battleEntityId
,dojo.account.account.address
, anddojo.setup.components
.
66-68
: Fix useMemo dependencies for ownArmyBattleStarter.The dependencies array is missing
account.address
andcomponents
, which could lead to stale data.Apply this diff to fix the dependency array:
() => getArmy(battleView?.ownArmyEntityId || 0, ContractAddress(dojo.account.account.address), dojo.setup.components), - [battleView?.ownArmyEntityId || 0], + [battleView?.ownArmyEntityId, dojo.account.account.address, dojo.setup.components],
131-139
: LGTM! Proper memoization of structureFromPosition.The dependency array correctly includes all dependencies:
battlePosition.x
,battlePosition.y
,dojo.account.account.address
, anddojo.setup.components
.
141-148
: LGTM! Proper memoization of structureId.The dependency array correctly includes all dependencies:
defenderArmies
,dojo.account.account.address
, anddojo.setup.components
.client/apps/game/src/ui/components/worldmap/structures/structure-list-item.tsx (3)
11-13
: LGTM! Imports are well organized.The imports are logically grouped and follow a consistent pattern.
65-73
: LGTM! Proper memoization of armiesInBattle.The dependency array correctly includes all dependencies:
updatedBattle?.entity_id
,dojo.account.account.address
, anddojo.setup.components
.
80-80
: LGTM! Proper memoization of structure immunity.The dependency array correctly includes all dependencies:
structure
andnextBlockTimestamp
.client/apps/game/src/ui/components/trading/market-modal.tsx (3)
26-28
: LGTM! Imports are well organized.The imports are logically grouped and follow a consistent pattern.
71-79
: LGTM! Proper memoization of bankStructure.The dependency array correctly includes all dependencies:
bank?.position
,dojo.account.account.address
, anddojo.setup.components
.
116-124
: LGTM! Proper memoization of bankArmy.The dependency array correctly includes all dependencies:
bankStructure?.protector?.entity_id
,dojo.account.account.address
, anddojo.setup.components
.client/apps/game/src/ui/components/structures/worldmap/structure-card.tsx (6)
19-20
: LGTM! Imports are well organized.The imports are logically grouped and follow a consistent pattern.
49-57
: LGTM! Proper memoization of structure.The dependency array correctly includes all dependencies:
position
,dojo.account.account.address
, anddojo.setup.components
.
272-273
: LGTM! Helper function improves code reusability.The
getArmyWithAddress
helper function reduces code duplication and improves readability.
283-284
: LGTM! Clear and readable troop transfer logic.The use of descriptive variable names
fromArmy
andtoArmy
makes the troop transfer direction clear and easy to understand.
274-278
: Verify the dependency arrays for army memoization.The dependency arrays for both
protectorArmy
andgiverArmy
are missingaccount.address
andcomponents
, which could lead to stale data.Apply this diff to fix the dependency arrays:
const protectorArmy = useMemo( () => takerArmy || getArmyWithAddress(protector?.army_id || 0), - [takerArmy, protector?.army_id], + [takerArmy, protector?.army_id, account.address, components], ); const giverArmy = useMemo( () => getArmyWithAddress(giverArmyEntityId), - [giverArmyEntityId], + [giverArmyEntityId, account.address, components], );✅ Verification successful
The review comment is correct. Dependencies need to be updated.
The
getArmyWithAddress
function directly uses bothaccount.address
andcomponents
in its implementation, so these values should be included in the dependency arrays to prevent stale data. The suggested changes are necessary.🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Try to find getArmyWithAddress function definition ast-grep --pattern 'function getArmyWithAddress($_) { $$$ }' # Fallback to searching with ripgrep echo "=== Ripgrep search ===" rg "getArmyWithAddress" -A 10Length of output: 2310
59-62
: Verify the dependency array for playerGuild memoization.The dependency array only includes
structure?.owner.address
, but the memoized function also depends ongetGuildFromPlayerAddress
. Consider if this function needs to be included in the dependencies.Run the following script to check if
getGuildFromPlayerAddress
is stable:✅ Verification successful
The current dependency array is correct
The
getGuildFromPlayerAddress
function is already memoized withuseCallback
and an empty dependency array, making it stable across renders. All its internal dependencies (components, utility functions) are also stable. Including it in theuseMemo
dependency array would be redundant.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if getGuildFromPlayerAddress is defined within a hook or component # Test: Search for the function definition ast-grep --pattern 'const getGuildFromPlayerAddress = $_'Length of output: 2238
Script:
#!/bin/bash # Check where 'components' is defined and how it's used in use-guilds.tsx rg -B 5 -A 5 "components.*getEntityName" client/apps/game/src/hooks/helpers/use-guilds.tsxLength of output: 92
Script:
#!/bin/bash # Get the complete implementation of use-guilds.tsx rg -B 10 -A 5 "export function useGuilds" client/apps/game/src/hooks/helpers/use-guilds.tsxLength of output: 93
Script:
#!/bin/bash # Get all exports and hook definitions from use-guilds.tsx cat client/apps/game/src/hooks/helpers/use-guilds.tsxLength of output: 11274
const structure = getStructureAtPosition( | ||
{ x: hoveredBattlePosition?.x || 0, y: hoveredBattlePosition?.y || 0 }, | ||
ContractAddress(dojo.account.account.address), | ||
dojo.setup.components, | ||
); |
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.
🛠️ Refactor suggestion
Memoize structure retrieval for better performance.
The structure retrieval should be memoized to prevent unnecessary recalculations on each render.
+ const contractAddress = useMemo(
+ () => ContractAddress(dojo.account.account.address),
+ [dojo.account.account.address]
+ );
- const structure = getStructureAtPosition(
- { x: hoveredBattlePosition?.x || 0, y: hoveredBattlePosition?.y || 0 },
- ContractAddress(dojo.account.account.address),
- dojo.setup.components,
- );
+ const structure = useMemo(
+ () =>
+ getStructureAtPosition(
+ { x: hoveredBattlePosition?.x || 0, y: hoveredBattlePosition?.y || 0 },
+ contractAddress,
+ dojo.setup.components,
+ ),
+ [hoveredBattlePosition?.x, hoveredBattlePosition?.y, contractAddress, dojo.setup.components]
+ );
📝 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 structure = getStructureAtPosition( | |
{ x: hoveredBattlePosition?.x || 0, y: hoveredBattlePosition?.y || 0 }, | |
ContractAddress(dojo.account.account.address), | |
dojo.setup.components, | |
); | |
const contractAddress = useMemo( | |
() => ContractAddress(dojo.account.account.address), | |
[dojo.account.account.address] | |
); | |
const structure = useMemo( | |
() => | |
getStructureAtPosition( | |
{ x: hoveredBattlePosition?.x || 0, y: hoveredBattlePosition?.y || 0 }, | |
contractAddress, | |
dojo.setup.components, | |
), | |
[hoveredBattlePosition?.x, hoveredBattlePosition?.y, contractAddress, dojo.setup.components] | |
); |
const { | ||
account: { account }, | ||
setup: { components }, | ||
} = useDojo(); |
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.
💡 Codebase verification
The dependency array is indeed incomplete and should be updated.
The codebase consistently shows that getArmy
calls require all three dependencies (hoveredArmyEntityId
, account.address
, and components
) to be included in the dependency array. This pattern is well-established in multiple files, particularly in battle-side-view.tsx
, battle-view.tsx
, and battle-history.tsx
. The suggested fix is correct and necessary to prevent stale data issues.
🔗 Analysis chain
Verify the dependency array for army memoization.
The useMemo
dependency array only includes hoveredArmyEntityId
, but the memoized function also depends on account.address
and components
. This could lead to stale data if these dependencies change.
Apply this diff to fix the dependency array:
const army = useMemo(() => {
if (hoveredArmyEntityId) return getArmy(hoveredArmyEntityId, ContractAddress(account.address), components);
return undefined;
- }, [hoveredArmyEntityId]);
+ }, [hoveredArmyEntityId, account.address, components]);
Also applies to: 30-30
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Find the complete useMemo implementation
ast-grep --pattern 'const army = useMemo(() => {
$$$
}, [$$$]);'
# Find getArmy function definition
ast-grep --pattern 'function getArmy($_) {
$$$
}'
# Search for similar patterns with ripgrep for better context
rg "getArmy.*components" -A 5 -B 5
Length of output: 11350
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
🧹 Nitpick comments (1)
client/apps/game/src/ui/components/worldmap/structures/structure-label.tsx (1)
14-25
: Consider lifting state calculations to the parent component.While the memoization is well implemented, consider lifting the immunity calculations to the parent component to maintain better separation of concerns. This would make the component more reusable and testable.
Example refactor:
-export const ImmunityTimer = ({ structure, className }: { structure: Structure; className?: string }) => { +interface ImmunityTimerProps { + isImmune: boolean; + timer: number; + className?: string; +} + +export const ImmunityTimer = ({ isImmune, timer, className }: ImmunityTimerProps) => { const { nextBlockTimestamp } = useNextBlockTimestamp(); - - const isImmune = useMemo( - () => isStructureImmune(structure, nextBlockTimestamp || 0), - [structure, nextBlockTimestamp], - ); - - const timer = useMemo( - () => getStructureImmunityTimer(structure, nextBlockTimestamp || 0), - [structure, nextBlockTimestamp], - );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/apps/game/src/ui/components/worldmap/structures/structure-label.tsx
(3 hunks)
🧰 Additional context used
📓 Learnings (1)
client/apps/game/src/ui/components/worldmap/structures/structure-label.tsx (1)
Learnt from: aymericdelab
PR: BibliothecaDAO/eternum#1818
File: client/src/ui/components/worldmap/structures/StructureLabel.tsx:47-0
Timestamp: 2024-11-12T03:28:50.421Z
Learning: In `StructureLabel.tsx`, when `timer` is negative, the immunity message is not shown, so handling negative `timer` values isn't necessary.
🔇 Additional comments (3)
client/apps/game/src/ui/components/worldmap/structures/structure-label.tsx (3)
1-13
: LGTM! Clean import organization.The new imports for structure utility functions align well with the refactoring approach.
Line range hint
37-84
: LGTM! Clean implementation of structure handling.The changes demonstrate good practices:
- Type-safe usage of ContractAddress
- Appropriate memoization of structure retrieval
- Consistent use of utility functions
45-49
: Verify consistent usage of structure utility functions.Let's ensure the new utility functions are used consistently across the codebase.
✅ Verification successful
Consistent usage pattern confirmed across codebase
The
getStructure
function is used consistently throughout the codebase with the same parameter pattern:
- Entity ID
- Contract address wrapped with ContractAddress
- Components from dojo setup
No legacy function calls remain, indicating a complete migration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check usage patterns of structure utility functions # Check for any remaining uses of old patterns rg "useIsStructureImmune|useStructureImmunityTimer|getStructureByEntityId" --type typescript # Verify consistent parameter passing for new utility functions rg "getStructure\(" -A 2 --type typescriptLength of output: 209
Script:
#!/bin/bash # Search for old utility function patterns rg -g '*.{ts,tsx}' 'useIsStructureImmune|useStructureImmunityTimer|getStructureByEntityId' # Search for new getStructure function usage patterns rg -g '*.{ts,tsx}' 'getStructure\(' -A 2 # Use ast-grep to find getStructure function calls ast-grep --pattern 'getStructure($_)'Length of output: 3982
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.
lgtm
Summary by CodeRabbit
Release Notes
Army Management
Utility Updates
Refactoring
Removed Features
calculateLordsShare
function from leaderboard utilities.