optimistic #2220
optimistic #2220
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Warning Rate limit exceeded@ponderingdemocritus has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 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. WalkthroughThe pull request introduces significant enhancements to client-side component management and troop/resource handling within the application. It adds five new components— 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 9 at 08:00 AM. |
|
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: 2
🧹 Outside diff range and nitpick comments (1)
client/src/dojo/modelManager/TileManager.ts (1)
264-264: Avoid using 'as unknown as' for type castingUsing
as unknown as BuildingTypefor type casting can mask type errors and reduce type safety. Consider refining the types or adding type guards to ensure proper typing without unnecessary casting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
client/src/dojo/createClientComponents.ts(1 hunks)client/src/dojo/modelManager/TileManager.ts(4 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
client/src/dojo/modelManager/TileManager.ts
[error] 341-341: Don't use 'Number' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'number' instead
(lint/complexity/noBannedTypes)
[error] 342-342: Don't use 'Number' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'number' instead
(lint/complexity/noBannedTypes)
🔇 Additional comments (1)
client/src/dojo/createClientComponents.ts (1)
13-14: LGTM! Components 'Population' and 'Resource' added correctly
The Population and Resource components have been properly added to the client components using overridableComponent. This integration will enhance the management of population and resources within the client.
| value: { | ||
| population: | ||
| (getComponentValue(this.setup.components.Population, realmEntityId)?.population || 0) - | ||
| configManager.getBuildingPopConfig(currentBuilding?.category as unknown as BuildingType).population, |
There was a problem hiding this comment.
Handle undefined 'currentBuilding' to prevent runtime errors
In the _optimisticDestroy method, currentBuilding may be undefined if the building does not exist. Accessing currentBuilding.category when currentBuilding is undefined will lead to a runtime error. Ensure that currentBuilding is defined before using its properties.
You can modify the code to check if currentBuilding is defined:
if (currentBuilding?.category !== undefined) {
const buildingPopulation = configManager.getBuildingPopConfig(currentBuilding.category as BuildingType).population;
this.setup.components.Population.addOverride(populationOverrideId, {
entity: realmEntityId,
value: {
population:
(getComponentValue(this.setup.components.Population, realmEntityId)?.population || 0) -
buildingPopulation,
},
});
} else {
// Handle the case where currentBuilding is undefined
// Possibly log an error or take appropriate action
}| ? new CairoOption<Number>(CairoOptionVariant.Some, resourceType) | ||
| : new CairoOption<Number>(CairoOptionVariant.None, 0), |
There was a problem hiding this comment.
Use 'number' instead of 'Number' for primitive types
The static analysis tool indicates that 'Number' should not be used as a type. For consistency and to prevent potential issues, please use the lowercase 'number' primitive type.
Apply this diff to correct the issue:
- ? new CairoOption<Number>(CairoOptionVariant.Some, resourceType)
- : new CairoOption<Number>(CairoOptionVariant.None, 0),
+ ? new CairoOption<number>(CairoOptionVariant.Some, resourceType)
+ : new CairoOption<number>(CairoOptionVariant.None, 0),📝 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.
| ? new CairoOption<Number>(CairoOptionVariant.Some, resourceType) | |
| : new CairoOption<Number>(CairoOptionVariant.None, 0), | |
| ? new CairoOption<number>(CairoOptionVariant.Some, resourceType) | |
| : new CairoOption<number>(CairoOptionVariant.None, 0), |
🧰 Tools
🪛 Biome (1.9.4)
[error] 341-341: Don't use 'Number' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'number' instead
(lint/complexity/noBannedTypes)
[error] 342-342: Don't use 'Number' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'number' instead
(lint/complexity/noBannedTypes)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
client/src/dojo/modelManager/ArmyMovementManager.ts (2)
299-305: Consider refactoring override removal logic to avoid duplicationCurrently, the non-visual overrides are removed in both the
catchandthenblocks of_exploreHex. Since non-visual overrides should be removed regardless of success or failure, consider moving_removeNonVisualOverrides(overrideId);outside of the promise chain to reduce code duplication.
377-381: Refactor override removal logic to avoid duplicationIn the
_travelToHexmethod, non-visual overrides are removed in both thecatchandthenblocks. Similar to the suggestion for_exploreHex, consider moving_removeNonVisualOverrides(overrideId);outside of the promise chain to eliminate duplicated code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
client/src/dojo/createClientComponents.ts(1 hunks)client/src/dojo/modelManager/ArmyMovementManager.ts(6 hunks)client/src/dojo/modelManager/ResourceManager.ts(1 hunks)
🔇 Additional comments (8)
client/src/dojo/modelManager/ArmyMovementManager.ts (6)
15-15: LGTM!
The import of TravelTypes is necessary for handling travel-related functionalities.
224-234: LGTM!
The _optimisticCapacityUpdate method correctly updates the weight component based on the provided capacity.
268-273: LGTM!
The call to _optimisticCapacityUpdate correctly calculates the capacity based on the exploration reward and resource weight.
313-313: LGTM!
The call to _optimisticFoodCosts with TravelTypes.Travel correctly handles food cost updates during travel.
326-330: LGTM!
The _removeVisualOverride method appropriately removes visual overrides when an action fails.
332-337: LGTM!
The _removeNonVisualOverrides method correctly removes non-visual overrides for both success and failure scenarios.
client/src/dojo/createClientComponents.ts (1)
13-15: LGTM!
Adding Population, Resource, and Weight components enhances the client components, enabling better state management.
client/src/dojo/modelManager/ResourceManager.ts (1)
57-68: LGTM!
The optimisticResourceUpdate method correctly updates the resource balance with the provided change, ensuring consistent state updates.
| private readonly _optimisticFoodCosts = (overrideId: string, travelType: TravelTypes) => { | ||
| const entityArmy = getComponentValue(this.setup.components.Army, this.entity); | ||
| let costs = { wheatPayAmount: 0, fishPayAmount: 0 }; | ||
| if (travelType === TravelTypes.Explore) { | ||
| costs = computeExploreFoodCosts(entityArmy?.troops); | ||
| } else { | ||
| costs = computeTravelFoodCosts(entityArmy?.troops); | ||
| } | ||
|
|
||
| this.wheatManager.optimisticResourceUpdate(overrideId, -BigInt(multiplyByPrecision(costs.wheatPayAmount))); | ||
| this.fishManager.optimisticResourceUpdate(overrideId, -BigInt(multiplyByPrecision(costs.fishPayAmount))); | ||
| }; |
There was a problem hiding this comment.
Handle potential undefined entityArmy to prevent runtime errors
In the _optimisticFoodCosts method, entityArmy may be undefined. Accessing entityArmy.troops without checking can lead to runtime errors. Please add a check to ensure entityArmy is defined before proceeding.
Apply this diff to handle the potential undefined entityArmy:
private readonly _optimisticFoodCosts = (overrideId: string, travelType: TravelTypes) => {
const entityArmy = getComponentValue(this.setup.components.Army, this.entity);
+ if (!entityArmy) {
+ // Handle undefined entityArmy, possibly by returning early or setting default costs
+ return;
+ }
let costs = { wheatPayAmount: 0, fishPayAmount: 0 };
if (travelType === TravelTypes.Explore) {
costs = computeExploreFoodCosts(entityArmy.troops);
} else {
costs = computeTravelFoodCosts(entityArmy.troops);
}
// Rest of the code...
};📝 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.
| private readonly _optimisticFoodCosts = (overrideId: string, travelType: TravelTypes) => { | |
| const entityArmy = getComponentValue(this.setup.components.Army, this.entity); | |
| let costs = { wheatPayAmount: 0, fishPayAmount: 0 }; | |
| if (travelType === TravelTypes.Explore) { | |
| costs = computeExploreFoodCosts(entityArmy?.troops); | |
| } else { | |
| costs = computeTravelFoodCosts(entityArmy?.troops); | |
| } | |
| this.wheatManager.optimisticResourceUpdate(overrideId, -BigInt(multiplyByPrecision(costs.wheatPayAmount))); | |
| this.fishManager.optimisticResourceUpdate(overrideId, -BigInt(multiplyByPrecision(costs.fishPayAmount))); | |
| }; | |
| private readonly _optimisticFoodCosts = (overrideId: string, travelType: TravelTypes) => { | |
| const entityArmy = getComponentValue(this.setup.components.Army, this.entity); | |
| if (!entityArmy) { | |
| // If there's no army, no resources need to be consumed | |
| return; | |
| } | |
| let costs = { wheatPayAmount: 0, fishPayAmount: 0 }; | |
| if (travelType === TravelTypes.Explore) { | |
| costs = computeExploreFoodCosts(entityArmy.troops); | |
| } else { | |
| costs = computeTravelFoodCosts(entityArmy.troops); | |
| } | |
| this.wheatManager.optimisticResourceUpdate(overrideId, -BigInt(multiplyByPrecision(costs.wheatPayAmount))); | |
| this.fishManager.optimisticResourceUpdate(overrideId, -BigInt(multiplyByPrecision(costs.fishPayAmount))); | |
| }; |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (6)
client/src/dojo/modelManager/ArmyManager.ts (4)
8-12: Ensure Consistent Ordering of Troop TypesThe
Troopsinterface defines troop counts usingResourcesIds, but the order of[Paladin],[Crossbowman],[Knight]may introduce confusion. For clarity and consistency, consider ordering the troop types uniformly throughout the codebase.
24-29: Simplify the Retrieval ofentity_owner_idThe
getRealmEntityIdmethod can be refactored for better readability.Consider refactoring as follows:
-private getRealmEntityId(): number { - return ( - getComponentValue(this.dojo.setup.components.EntityOwner, getEntityIdFromKeys([BigInt(this.armyEntityId)])) - ?.entity_owner_id || 0 - ); +private getRealmEntityId(): number { + const ownerComponent = getComponentValue( + this.dojo.setup.components.EntityOwner, + getEntityIdFromKeys([BigInt(this.armyEntityId)]) + ); + return ownerComponent ? ownerComponent.entity_owner_id : 0; }
78-78: Reuse Override ID for ConsistencyIn
this._optimisticAddTroops(uuid(), troops);, a new UUID is generated each time. If consistent override IDs are needed for batching or tracking, consider reusing or passing an override ID.
89-95: Ensure Consistent Asynchronous Method SignaturesThe
deleteArmymethod isasync, whileaddTroopswas not. For consistency and to handle asynchronous operations properly, align the method signatures.Since
addTroopsnow handles promises, ensure all async methods returnPromise.client/src/ui/components/military/ArmyChip.tsx (1)
98-102: Improve Accessibility by Adding ARIA LabelsThe button lacks an
aria-label, which may hinder accessibility.Add an
aria-labelto the button for better accessibility:<Button className="my-2" size="xs" variant="red" onClick={() => setEditMode(!editMode)} + aria-label="Exit Army Management" >client/src/ui/components/military/ArmyManagementCard.tsx (1)
184-187: Maintain Consistent Order of TroopsThe order of troops in the
troopsarray differs from other parts of the code. For consistency, align the order across the codebase to prevent confusion.Reorder the array if necessary.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
client/src/dojo/createClientComponents.ts(1 hunks)client/src/dojo/modelManager/ArmyManager.ts(1 hunks)client/src/ui/components/military/ArmyChip.tsx(2 hunks)client/src/ui/components/military/ArmyManagementCard.tsx(3 hunks)
🔇 Additional comments (2)
client/src/dojo/modelManager/ArmyManager.ts (1)
31-40: Verify Negative Amount in Resource Update
In _updateResourceBalances, you're updating resources using -BigInt(amount). Confirm that decrementing the resource balance with a negative value is intended and won't cause unintended side effects.
Here's a script to check the usage of optimisticResourceUpdate with negative amounts:
✅ Verification successful
Negative Amount in Resource Update is Correct
The usage of -BigInt(amount) in _updateResourceBalances is correct and intended. The optimisticResourceUpdate method in ResourceManager adds the change value to the current balance (currentBalance + change), so passing a negative value correctly decrements the resource. This pattern is consistently used across the codebase, as seen in ArmyMovementManager.ts where resources are also decremented using negative values for costs.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of optimisticResourceUpdate with negative amounts.
rg --multiline 'optimisticResourceUpdate\(.*,-BigInt\(.*\)\)' -A 2
Length of output: 68
Script:
#!/bin/bash
# Let's first find the ResourceManager class implementation to understand how optimisticResourceUpdate is used
ast-grep --pattern 'class ResourceManager {
$$$
optimisticResourceUpdate($$$) {
$$$
}
$$$
}'
# Also search for any other usages of optimisticResourceUpdate
rg "optimisticResourceUpdate" -A 2
Length of output: 1217
Script:
#!/bin/bash
# Let's look at the implementation of ResourceManager's optimisticResourceUpdate to understand how it handles negative values
cat client/src/dojo/modelManager/ResourceManager.ts
Length of output: 8996
client/src/dojo/createClientComponents.ts (1)
13-16: Components Added Successfully
The Population, Resource, Weight, and Army components have been added as overridable components. This enhances client-side state management.
| constructor( | ||
| private readonly dojo: DojoResult, | ||
| private readonly armyEntityId: ID, | ||
| ) { | ||
| this.realmEntityId = this.getRealmEntityId(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle Potential Undefined Realm Entity ID
In the constructor, this.realmEntityId is assigned using this.getRealmEntityId(), which defaults to 0 if the entity_owner_id is undefined. Ensure that 0 is a valid value in this context, or implement error handling for cases where entity_owner_id is not found.
Consider adding a check and throwing an error or warning if entity_owner_id is undefined.
| knight_count: BigInt(army.troops.knight_count) + BigInt(troops[ResourcesIds.Knight]), | ||
| crossbowman_count: BigInt(army.troops.crossbowman_count) + BigInt(troops[ResourcesIds.Crossbowman]), | ||
| paladin_count: BigInt(army.troops.paladin_count) + BigInt(troops[ResourcesIds.Paladin]), |
There was a problem hiding this comment.
Check for Potential Integer Overflows
When updating troop counts in _updateArmyTroops, ensure that adding BigInt values does not cause integer overflows.
Consider adding checks to prevent overflows:
+// Check for potential overflows
+const newKnightCount = BigInt(army.troops.knight_count) + BigInt(troops[ResourcesIds.Knight]);
+if (newKnightCount > MAX_SAFE_INTEGER) {
+ throw new Error('Knight count exceeds maximum safe integer.');
+}Committable suggestion skipped: line range outside the PR's diff.
| public addTroops(troops: Troops): void { | ||
| this.dojo.setup.systemCalls.army_buy_troops({ | ||
| signer: this.dojo.account.account, | ||
| payer_id: this.realmEntityId, | ||
| army_id: this.armyEntityId, | ||
| troops: { | ||
| knight_count: BigInt(troops[ResourcesIds.Knight]), | ||
| crossbowman_count: BigInt(troops[ResourcesIds.Crossbowman]), | ||
| paladin_count: BigInt(troops[ResourcesIds.Paladin]), | ||
| }, | ||
| }); | ||
|
|
||
| this._optimisticAddTroops(uuid(), troops); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle Asynchronous System Call in addTroops
The addTroops method calls army_buy_troops but does not handle the promise or any potential errors. This might lead to unhandled promise rejections.
Update the method to handle the asynchronous operation:
-public addTroops(troops: Troops): void {
+public async addTroops(troops: Troops): Promise<void> {
try {
await this.dojo.setup.systemCalls.army_buy_troops({
// parameters...
});
this._optimisticAddTroops(uuid(), troops);
} catch (error) {
// Handle error appropriately
console.error('Error adding troops:', error);
}
}Committable suggestion skipped: line range outside the PR's diff.
| armyManager.addTroops({ | ||
| [ResourcesIds.Knight]: multiplyByPrecision(troopCounts[ResourcesIds.Knight]), | ||
| [ResourcesIds.Crossbowman]: multiplyByPrecision(troopCounts[ResourcesIds.Crossbowman]), | ||
| [ResourcesIds.Paladin]: multiplyByPrecision(troopCounts[ResourcesIds.Paladin]), | ||
| }); |
There was a problem hiding this comment.
Confirm Correct Usage of multiplyByPrecision
In handleBuyArmy, troopCounts are multiplied by multiplyByPrecision. Verify that this is intended, as troop counts are usually whole numbers, and multiplying may inflate the values incorrectly.
If not required, remove multiplyByPrecision:
armyManager.addTroops({
- [ResourcesIds.Knight]: multiplyByPrecision(troopCounts[ResourcesIds.Knight]),
- [ResourcesIds.Crossbowman]: multiplyByPrecision(troopCounts[ResourcesIds.Crossbowman]),
- [ResourcesIds.Paladin]: multiplyByPrecision(troopCounts[ResourcesIds.Paladin]),
+ [ResourcesIds.Knight]: troopCounts[ResourcesIds.Knight],
+ [ResourcesIds.Crossbowman]: troopCounts[ResourcesIds.Crossbowman],
+ [ResourcesIds.Paladin]: troopCounts[ResourcesIds.Paladin],
});📝 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.
| armyManager.addTroops({ | |
| [ResourcesIds.Knight]: multiplyByPrecision(troopCounts[ResourcesIds.Knight]), | |
| [ResourcesIds.Crossbowman]: multiplyByPrecision(troopCounts[ResourcesIds.Crossbowman]), | |
| [ResourcesIds.Paladin]: multiplyByPrecision(troopCounts[ResourcesIds.Paladin]), | |
| }); | |
| armyManager.addTroops({ | |
| [ResourcesIds.Knight]: troopCounts[ResourcesIds.Knight], | |
| [ResourcesIds.Crossbowman]: troopCounts[ResourcesIds.Crossbowman], | |
| [ResourcesIds.Paladin]: troopCounts[ResourcesIds.Paladin], | |
| }); |
| const armyManager = new ArmyManager(dojo, army?.entity_id || 0); | ||
|
|
There was a problem hiding this comment.
Handle Possible Undefined army.entity_id
When creating ArmyManager, using army?.entity_id || 0 may result in 0 if army is undefined. Ensure that 0 is a valid armyEntityId, or add checks to handle undefined army.
Consider verifying army before creating ArmyManager:
-const armyManager = new ArmyManager(dojo, army?.entity_id || 0);
+if (army) {
+ const armyManager = new ArmyManager(dojo, army.entity_id);
+ // rest of the code
+}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
client/src/hooks/helpers/useArmies.tsx(3 hunks)client/src/ui/components/military/ArmyChip.tsx(4 hunks)client/src/ui/components/military/ArmyManagementCard.tsx(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- client/src/ui/components/military/ArmyChip.tsx
- client/src/ui/components/military/ArmyManagementCard.tsx
🔇 Additional comments (3)
client/src/hooks/helpers/useArmies.tsx (3)
32-32: LGTM: Type definition is well-placed
The addition of isHome to the ArmyInfo type is clear and properly typed.
142-142: LGTM: Property addition is consistent
The isHome property is correctly added to the returned object, maintaining consistency with the type definition.
118-118: Verify the impact of switching from realm_id to entity_id
The change from realm.realm_id to realm.entity_id for position lookup is significant. Let's verify this change doesn't break existing functionality.
✅ Verification successful
Let me gather more context about the Position component usage to better understand this change.
Let me search for the realm entity structure to understand the relationship between realm_id and entity_id.
Based on the gathered evidence, I can now provide the final response:
The change from realm_id to entity_id is correct and necessary
The change aligns with the contract's Realm model structure where entity_id is the correct identifier for Position lookups. This is confirmed by:
- The contract's Realm struct showing both fields exist but
entity_idis marked as#[key] - Other Position component queries consistently using entity IDs for lookups across the codebase
- Similar changes in other files (e.g., useTrade.tsx, useResources.tsx) also using realm.entity_id
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of realm.realm_id and realm.entity_id
# to ensure consistency across the codebase
echo "Searching for realm.realm_id usage:"
rg "realm\.realm_id"
echo -e "\nSearching for realm.entity_id usage:"
rg "realm\.entity_id"
Length of output: 1693
Script:
#!/bin/bash
# Search for Position component usage patterns
echo "Searching for Position component usage:"
rg "Position.*getEntityIdFromKeys" -A 2
echo -e "\nSearching for Position component definition:"
ast-grep --pattern 'const Position = defineComponent('
Length of output: 4391
Script:
#!/bin/bash
# Search for realm type/interface definitions
echo "Searching for realm type definitions:"
ast-grep --pattern 'type Realm = {
$$$
}'
echo -e "\nSearching for realm interface definitions:"
ast-grep --pattern 'interface Realm {
$$$
}'
echo -e "\nSearching for realm model in contracts:"
rg "struct Realm" -A 10 contracts/
Length of output: 1709
| const isHome = homePosition && position.x === homePosition.x && position.y === homePosition.y; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null safety to isHome computation
The current implementation could throw if homePosition is undefined. Consider using optional chaining:
- const isHome = homePosition && position.x === homePosition.x && position.y === homePosition.y;
+ const isHome = Boolean(homePosition?.x === position.x && homePosition?.y === position.y);📝 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 isHome = homePosition && position.x === homePosition.x && position.y === homePosition.y; | |
| const isHome = Boolean(homePosition?.x === position.x && homePosition?.y === position.y); |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
client/src/dojo/modelManager/TileManager.ts (2)
242-252: Add input validation to _overrideResource methodConsider adding validation for the input parameters to ensure robust error handling:
- Validate that
resourceTypeis a valid resource ID- Check for potential overflow when adding
changetocurrentBalanceprivate _overrideResource = (entity: Entity, resourceType: number, change: bigint) => { + if (resourceType < 0 || !Object.values(ResourcesIds).includes(resourceType)) { + throw new Error(`Invalid resource type: ${resourceType}`); + } const currentBalance = getComponentValue(this.setup.components.Resource, entity)?.balance || 0n; + // Check for potential overflow + const newBalance = currentBalance + change; + if (newBalance < 0n) { + throw new Error('Insufficient resource balance'); + } const resourceOverrideId = uuid(); this.setup.components.Resource.addOverride(resourceOverrideId, { entity, value: { resource_type: resourceType, - balance: currentBalance + change, + balance: newBalance, }, }); };
351-367: Enhance error handling with specific error typesThe current error handling could be improved by:
- Handling specific error types differently
- Adding user-friendly error messages
- Logging more context for debugging
try { await this.setup.systemCalls.create_building({ // ... existing code ... }); } catch (error) { + // Rollback optimistic updates this._optimisticBuilding(entityId, col, row, BuildingType.None, resourceType); - console.error(error); + // Enhanced error handling + if (error instanceof Error) { + console.error('Building creation failed:', { + error: error.message, + context: { + entityId, + position: { col, row }, + buildingType, + resourceType + } + }); + throw new Error(`Failed to create building: ${error.message}`); + } throw error; }🧰 Tools
🪛 Biome (1.9.4)
[error] 359-359: Don't use 'Number' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'number' instead(lint/complexity/noBannedTypes)
[error] 360-360: Don't use 'Number' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'number' instead(lint/complexity/noBannedTypes)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
client/src/dojo/createClientComponents.ts(1 hunks)client/src/dojo/modelManager/TileManager.ts(4 hunks)client/src/dojo/modelManager/__tests__/__BattleManagerMock__.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
client/src/dojo/modelManager/TileManager.ts
[error] 359-359: Don't use 'Number' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'number' instead
(lint/complexity/noBannedTypes)
[error] 360-360: Don't use 'Number' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'number' instead
(lint/complexity/noBannedTypes)
🔇 Additional comments (4)
client/src/dojo/createClientComponents.ts (2)
13-17: LGTM! New components follow established patterns.
The implementation of new components maintains consistency with existing code structure and follows TypeScript best practices.
17-17: Consider documenting new components and migration plan.
The BuildingQuantityv2 component suggests an upgrade from a previous version. Please consider:
- Adding JSDoc comments describing the purpose and usage of each new component
- Documenting the migration path from the previous BuildingQuantity to BuildingQuantityv2
client/src/dojo/modelManager/TileManager.ts (2)
280-287:
Handle undefined currentBuilding to prevent runtime errors
The code is still vulnerable to runtime errors when accessing properties of an undefined currentBuilding.
- this.setup.components.Population.addOverride(populationOverrideId, {
- entity: realmEntityId,
- value: {
- population:
- (getComponentValue(this.setup.components.Population, realmEntityId)?.population || 0) -
- configManager.getBuildingPopConfig(currentBuilding?.category as unknown as BuildingType).population,
- },
- });
+ const currentPopulation = getComponentValue(this.setup.components.Population, realmEntityId)?.population || 0;
+ const buildingCategory = currentBuilding?.category;
+
+ if (buildingCategory) {
+ const buildingPopulation = configManager.getBuildingPopConfig(buildingCategory as BuildingType).population;
+ this.setup.components.Population.addOverride(populationOverrideId, {
+ entity: realmEntityId,
+ value: {
+ population: currentPopulation - buildingPopulation,
+ },
+ });
+ } else {
+ console.error('Building not found during population update');
+ }359-360: 🛠️ Refactor suggestion
Use 'number' instead of 'Number' type
The TypeScript best practice is to use the primitive type 'number' instead of the 'Number' object type.
- ? new CairoOption<Number>(CairoOptionVariant.Some, resourceType)
- : new CairoOption<Number>(CairoOptionVariant.None, 0),
+ ? new CairoOption<number>(CairoOptionVariant.Some, resourceType)
+ : new CairoOption<number>(CairoOptionVariant.None, 0),🧰 Tools
🪛 Biome (1.9.4)
[error] 359-359: Don't use 'Number' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'number' instead
(lint/complexity/noBannedTypes)
[error] 360-360: Don't use 'Number' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'number' instead
(lint/complexity/noBannedTypes)
| if (buildingType === BuildingType.Storehouse) { | ||
| const storehouseQuantity = | ||
| getComponentValue( | ||
| this.setup.components.BuildingQuantityv2, | ||
| getEntityIdFromKeys([BigInt(entityId), BigInt(buildingType)]), | ||
| )?.value || 0; | ||
|
|
||
| const quantityOverrideId = uuid(); | ||
| this.setup.components.BuildingQuantityv2.addOverride(quantityOverrideId, { | ||
| entity: realmEntityId, | ||
| value: { | ||
| value: storehouseQuantity + 1, | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| const resourceChange = configManager.buildingCosts[buildingType]; | ||
| resourceChange.forEach((resource) => { | ||
| this._overrideResource(realmEntityId, resource.resource, -BigInt(resource.amount)); | ||
| }); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor storehouse and resource management for better separation of concerns
The building management code handles multiple responsibilities (storehouse quantity, resource costs). Consider extracting these into separate methods for better maintainability and testability.
+ private _updateStorehouseQuantity(entityId: ID, buildingType: BuildingType) {
+ if (buildingType !== BuildingType.Storehouse) return;
+
+ const realmEntityId = getEntityIdFromKeys([BigInt(entityId)]);
+ const storehouseQuantity = getComponentValue(
+ this.setup.components.BuildingQuantityv2,
+ getEntityIdFromKeys([BigInt(entityId), BigInt(buildingType)]),
+ )?.value || 0;
+
+ const quantityOverrideId = uuid();
+ this.setup.components.BuildingQuantityv2.addOverride(quantityOverrideId, {
+ entity: realmEntityId,
+ value: { value: storehouseQuantity + 1 },
+ });
+ }
+
+ private _deductResourceCosts(entityId: ID, buildingType: BuildingType) {
+ const realmEntityId = getEntityIdFromKeys([BigInt(entityId)]);
+ const resourceChange = configManager.buildingCosts[buildingType];
+ resourceChange.forEach((resource) => {
+ this._overrideResource(realmEntityId, resource.resource, -BigInt(resource.amount));
+ });
+ }
private _optimisticBuilding = (
entityId: ID,
col: number,
row: number,
buildingType: BuildingType,
resourceType?: number,
) => {
// ... existing building override code ...
- if (buildingType === BuildingType.Storehouse) {
- const storehouseQuantity = // ... existing storehouse code ...
- }
+ this._updateStorehouseQuantity(entityId, buildingType);
+ this._deductResourceCosts(entityId, buildingType);
- const resourceChange = configManager.buildingCosts[buildingType];
- resourceChange.forEach((resource) => {
- this._overrideResource(realmEntityId, resource.resource, -BigInt(resource.amount));
- });
};Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (9)
client/src/ui/modules/entity-details/BuildingEntityDetails.tsx (2)
112-127: Enhance population capacity check with error feedbackThe
canDestroyBuildingfunction correctly prevents destroying worker huts when it would result in population capacity issues. However, users might be confused why the destroy button is disabled without any explanation.Consider adding a tooltip or error message to explain why the button is disabled. Example implementation:
const canDestroyBuilding = useMemo(() => { if (buildingState.buildingType !== BuildingType.WorkersHut) return true; const realmId = getComponentValue( dojo.setup.components.EntityOwner, getEntityIdFromKeys([BigInt(structureEntityId)]), ); const populationImpact = configManager.getBuildingPopConfig(buildingState.buildingType).capacity; const population = getComponentValue( dojo.setup.components.Population, getEntityIdFromKeys([BigInt(realmId?.entity_owner_id || 0)]), ); - return (population?.capacity || 0) - (population?.population || 0) >= populationImpact; + return { + canDestroy: (population?.capacity || 0) - (population?.population || 0) >= populationImpact, + reason: "Cannot destroy worker hut: Would exceed population capacity" + }; }, [buildingState.buildingType, buildingState.ownerEntityId]);
164-170: Add error tooltip to disabled destroy buttonThe button is correctly disabled based on population constraints, but users need feedback about why they can't destroy the building.
Consider adding a tooltip to explain why the button is disabled:
<Button - disabled={!canDestroyBuilding} + disabled={!canDestroyBuilding.canDestroy} + title={!canDestroyBuilding.canDestroy ? canDestroyBuilding.reason : ""} className="mb-4" onClick={handleDestroyBuilding} variant="danger" withoutSound > Destroy </Button>client/src/hooks/helpers/useRealm.tsx (1)
Line range hint
212-242: Consider performance impact of removing useMemoThe realm calculation has been changed from a memoized value to a direct function call. This could cause unnecessary recalculations on every render, especially since the function performs multiple component value lookups and calculations.
Consider keeping the memoization to prevent performance issues:
- const realm = (): any => { + const realm = useMemo(() => { if (realmEntityId !== undefined) { const entityId = getEntityIdFromKeys([BigInt(realmEntityId)]); const realm = getComponentValue(Realm, entityId); const owner = getComponentValue(Owner, entityId); const position = getComponentValue(Position, entityId); const population = getComponentValue(Population, entityId); if (realm && owner && position) { const { realm_id, entity_id, produced_resources, order, level } = realm; const name = getRealmNameById(realm_id); const { address } = owner; return { realmId: realm_id, entityId: entity_id, name, level, resourceTypesPacked: produced_resources, order, position, ...population, hasCapacity: !population || population.capacity + configManager.getBasePopulationCapacity() > population.population, owner: address, }; } } - }; + }, [realmEntityId, Realm, Owner, Position, Population]);client/src/ui/components/Toaster.tsx (2)
4-4: Consider adding explicit prop typesThe current type definition inherits all props from Sonner component. Consider explicitly defining the props you expect to use for better maintainability and documentation.
-type ToasterProps = React.ComponentProps<typeof Sonner>; +interface ToasterProps extends Pick<React.ComponentProps<typeof Sonner>, 'position' | 'theme' | 'className'> { + // Add any additional custom props here +}
6-25: Consider adding error boundary for theme-related issuesThe component could benefit from error handling for theme-related issues.
const Toaster = ({ ...props }: ToasterProps) => { const { theme = "system" } = useTheme(); + const [error, setError] = useState<Error | null>(null); + + if (error) { + console.error('Theme error:', error); + return <Sonner theme="system" {...props} />; + } return ( <Sonner theme={(theme === 'dark' || theme === 'light' || theme === 'system') ? theme : 'system'} className="toaster group" toastOptions={{ classNames: { toast: "group toast group-[.toaster]:bg-background group-[.toaster]:text-foreground group-[.toaster]:border-border group-[.toaster]:shadow-lg", description: "group-[.toast]:text-muted-foreground", actionButton: "group-[.toast]:bg-primary group-[.toast]:text-primary-foreground", cancelButton: "group-[.toast]:bg-muted group-[.toast]:text-muted-foreground", }, }} {...props} /> ); };client/src/ui/components/TxEmit.tsx (1)
1-32: Consider adding loading states for transactionsThe component could benefit from showing loading states while transactions are pending.
+import { useState } from 'react'; export function TransactionNotification() { + const [pendingTransactions, setPendingTransactions] = useState<Set<string>>(new Set()); // ... existing implementation + const handleTransactionSubmitted = (hash: string) => { + setPendingTransactions(prev => new Set(prev).add(hash)); + toast.loading("Transaction submitted", { + description: `Transaction hash: ${hash}`, + }); + }; useEffect(() => { // ... existing effect implementation + provider.on("transactionSubmitted", handleTransactionSubmitted); return () => { // ... existing cleanup + provider.off("transactionSubmitted", handleTransactionSubmitted); }; }, [provider]); return null; }client/src/ui/modules/settings/Settings.tsx (1)
Line range hint
57-60: Add error handling for clipboard operationsThe clipboard operation should handle potential failures, especially for browsers where clipboard access might be restricted.
const copyToClipBoard = () => { - navigator.clipboard.writeText(account.address); - toast("Copied address to clipboard!"); + navigator.clipboard.writeText(account.address) + .then(() => toast.success("Copied address to clipboard!")) + .catch(() => toast.error("Failed to copy address. Please try again.")); };client/src/dojo/createSystemCalls.ts (2)
Line range hint
54-74: Consider refactoring error message extractionThe error message extraction logic could be simplified using a more maintainable approach.
Consider refactoring the error extraction logic:
- if (error.message.includes("Failure reason:")) { - const match = error.message.match(/Failure reason: \\"(.*?)"/); - if (match?.[1]) { - errorMessage = match[1].slice(0, -1); - } else { - const matchOther = error.message.match(/Failure reason: "(.*?)"/); - if (matchOther?.[1]) { - errorMessage = matchOther[1].slice(0, -1); - } else { - const matchHex = error.message.match(/Failure reason: (0x[0-9a-f]+) \('(.*)'\)/); - if (matchHex?.[2]) { - errorMessage = matchHex[2]; - } - } - } - } + const errorPatterns = [ + /Failure reason: \\"(.*?)"/, + /Failure reason: "(.*?)"/, + /Failure reason: (?:0x[0-9a-f]+) \('(.*)'\)/ + ]; + if (error.message.includes("Failure reason:")) { + for (const pattern of errorPatterns) { + const match = error.message.match(pattern); + if (match?.[1]) { + errorMessage = match[1].slice(0, -1); + break; + } + } + }This approach:
- Makes the patterns more maintainable
- Reduces nesting
- Makes it easier to add new patterns
Line range hint
359-362: Address TODO: Fix Type for wrappedSystemCallsThe TODO comment indicates a need for proper typing of the wrapped system calls.
Would you like help implementing proper TypeScript types for the wrappedSystemCalls object? This would improve type safety and IDE support.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
client/package.json(1 hunks)client/src/App.tsx(1 hunks)client/src/dojo/createSystemCalls.ts(2 hunks)client/src/dojo/modelManager/TileManager.ts(4 hunks)client/src/hooks/helpers/useRealm.tsx(2 hunks)client/src/ui/components/Toaster.tsx(1 hunks)client/src/ui/components/TxEmit.tsx(1 hunks)client/src/ui/components/construction/SelectPreviewBuilding.tsx(2 hunks)client/src/ui/modules/entity-details/BuildingEntityDetails.tsx(4 hunks)client/src/ui/modules/settings/Settings.tsx(1 hunks)client/vite.config.ts(1 hunks)sdk/packages/eternum/src/provider/index.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
client/src/dojo/modelManager/TileManager.ts
[error] 367-367: Don't use 'Number' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'number' instead
(lint/complexity/noBannedTypes)
[error] 368-368: Don't use 'Number' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'number' instead
(lint/complexity/noBannedTypes)
🔇 Additional comments (13)
client/src/dojo/modelManager/TileManager.ts (4)
201-216: LGTM: Population management implementation is robust
The population management code correctly handles both population and capacity updates, with proper fallbacks for undefined values.
218-232: LGTM: Storehouse quantity tracking is well implemented
The code properly manages storehouse quantities with appropriate component updates and entity resolution.
234-238: LGTM: Resource cost deduction is properly implemented
The code correctly handles resource cost deductions for buildings using configuration-based costs.
359-375: LGTM: Error handling with proper rollback
The error handling implementation correctly manages failures by rolling back optimistic updates and maintaining error context.
🧰 Tools
🪛 Biome (1.9.4)
[error] 367-367: Don't use 'Number' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'number' instead
(lint/complexity/noBannedTypes)
[error] 368-368: Don't use 'Number' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'number' instead
(lint/complexity/noBannedTypes)
client/src/ui/components/construction/SelectPreviewBuilding.tsx (2)
177-184: LGTM: Building condition checks are well structured
The code improves readability and maintainability by:
- Handling WorkersHut as a special case that only requires resource balance
- Using explicit constants for building type checks
287-287: LGTM: Proper dependency tracking for population changes
The useMemo dependency array correctly includes realm.population to ensure UI updates when population changes.
sdk/packages/eternum/src/provider/index.ts (1)
843-843: LGTM: Removed unnecessary logging
Cleanup of debug logging improves code cleanliness.
client/src/App.tsx (1)
2-3: LGTM! Clean transition to the new notification system.
The replacement of react-toastify with the new Toaster and TransactionNotification components is well-structured and cleanly implemented.
Also applies to: 9-10
client/vite.config.ts (1)
24-25: Service worker update strategy looks good
The configuration ensures a safer approach to service worker updates:
clientsClaim: trueallows immediate control of clients after activationskipWaiting: falseprevents disrupting active sessions by waiting for all clients to close before activating new service worker
Let's verify other PWA-related configurations:
✅ Verification successful
PWA configuration is properly set up and consistent
The service worker update strategy is part of a well-configured PWA setup:
- Correct cache headers in vercel.json for service worker and manifest files
- PWA assets present in expected dimensions (64x64, 192x192, 512x512)
- VitePWA plugin configured with
autoUpdateregistration and appropriate workbox settings - Service worker behavior properly controlled with
clientsClaim: trueandskipWaiting: false
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for PWA manifest and service worker registration
rg -g '!node_modules' -l 'serviceWorker|manifest.json'
# Verify PWA assets exist
fd -g 'pwa-*.png' -t f
Length of output: 348
Script:
#!/bin/bash
# Check PWA manifest configuration in vercel.json
cat client/vercel.json
# Check service worker registration in client code
rg -g '!node_modules' 'registerServiceWorker|serviceWorker.register' -A 3
# Check VitePWA configuration
rg -g '!node_modules' 'VitePWA' -A 10 -B 2
Length of output: 2525
client/package.json (1)
50-50: Verify compatibility of new dependencies
The migration from react-toastify to sonner and addition of next-themes looks good. However, let's ensure:
- All toast notifications across the codebase have been migrated to use sonner
- The theme implementation is consistent with next-themes requirements
Let's check for any remaining react-toastify usage and verify sonner implementation:
Also applies to: 56-56
✅ Verification successful
Let me verify the toast implementation further by checking the actual toast component setup.
Migration to sonner is complete and properly implemented
The verification shows:
- No remaining react-toastify imports or components in the codebase
- Proper sonner implementation with theme integration in both client and landing directories
- Consistent toast usage across the codebase with proper error handling
- The only reference to react-toastify is in pnpm-lock.yaml which will be cleaned up upon the next install
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any remaining react-toastify imports
rg -l "react-toastify"
# Verify sonner toast usage consistency
rg "toast\(" -A 2
# Check theme implementation
ast-grep --pattern 'import { useTheme } from "next-themes"'
Length of output: 2118
Script:
#!/bin/bash
# Check toast component implementation
cat landing/src/components/ui/sonner.tsx client/src/ui/components/Toaster.tsx
# Check if there are any toast imports from react-toastify
rg "from ['\"]react-toastify" -A 2
# Check if there are any ToastContainer components from react-toastify
ast-grep --pattern 'ToastContainer'
Length of output: 1880
client/src/ui/modules/settings/Settings.tsx (1)
24-24: LGTM: Toast library migration
The import change from react-toastify to sonner is correct.
client/src/dojo/createSystemCalls.ts (2)
2-2: LGTM: Toast library migration
The change from 'react-toastify' to 'sonner' aligns with similar changes across the codebase.
47-50: Verify queue error propagation
The explicit result capture is good for clarity, but let's verify that errors still propagate correctly through the promise chain.
✅ Verification successful
Error handling is properly implemented in the promise chain
The code inspection reveals that error propagation is well-handled at multiple levels:
-
The
PromiseQueueclass properly propagates errors through:- Error handling in the
enqueuemethod using try/catch with explicit reject - Error logging in the
processQueuemethod
- Error handling in the
-
The
withQueueingfunction maintains proper error propagation as it:- Uses async/await correctly
- Doesn't swallow errors
- Preserves the promise chain
-
All system calls are wrapped with both
withQueueingandwithErrorHandling, ensuring errors are both caught for user feedback and propagated up the chain.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for error handling patterns in queue-related tests
rg -A 5 "promiseQueue.*catch|withQueueing.*catch"
Length of output: 51
Script:
#!/bin/bash
# Let's search for test files related to promise queue or system calls
fd -e test.ts -e spec.ts | xargs rg -l "promiseQueue|withQueueing|createSystemCalls"
# Then look for error handling patterns in the implementation
ast-grep --pattern 'try { $$$ } catch' client/src/dojo/createSystemCalls.ts
# Also search for any error tests
rg -A 5 "it.*error|test.*error|describe.*error" client/src/dojo/
Length of output: 231
Script:
#!/bin/bash
# Let's examine the implementation of the queue and error handling
cat client/src/dojo/createSystemCalls.ts
# Also look for any promise queue implementation
rg -l "class.*PromiseQueue|interface.*PromiseQueue"
Length of output: 14000
|
|
||
| return { | ||
| realm, | ||
| realm: realm(), |
There was a problem hiding this comment.
Fix potential undefined realm access
The realm function result is directly accessed without checking if it's undefined.
Add null check to prevent potential runtime errors:
- realm: realm(),
+ realm: realm() || undefined,📝 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.
| realm: realm(), | |
| realm: realm() || undefined, |
|
|
||
| return ( | ||
| <Sonner | ||
| theme={theme as ToasterProps["theme"]} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type safety for theme handling
The current theme type assertion could be made safer with proper type checking.
-theme={theme as ToasterProps["theme"]}
+theme={(theme === 'dark' || theme === 'light' || theme === 'system') ? theme : 'system'}📝 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.
| theme={theme as ToasterProps["theme"]} | |
| theme={(theme === 'dark' || theme === 'light' || theme === 'system') ? theme : 'system'} |
| const handleTransactionFailed = (error: string) => { | ||
| console.error("Transaction failed:", error); | ||
| toast("Transaction failed"); | ||
| }; |
There was a problem hiding this comment.
Enhance error message handling
The current implementation provides generic error messages without details.
-const handleTransactionFailed = (error: string) => {
+const handleTransactionFailed = (error: Error | string) => {
console.error("Transaction failed:", error);
- toast("Transaction failed");
+ toast.error("Transaction failed", {
+ description: typeof error === 'string' ? error : error.message,
+ duration: 5000,
+ });
};📝 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 handleTransactionFailed = (error: string) => { | |
| console.error("Transaction failed:", error); | |
| toast("Transaction failed"); | |
| }; | |
| const handleTransactionFailed = (error: Error | string) => { | |
| console.error("Transaction failed:", error); | |
| toast.error("Transaction failed", { | |
| description: typeof error === 'string' ? error : error.message, | |
| duration: 5000, | |
| }); | |
| }; |
| useEffect(() => { | ||
| const handleTransactionComplete = (receipt: any) => { | ||
| console.log("Transaction completed:", receipt); | ||
| toast("Transaction completed"); | ||
| }; | ||
|
|
||
| const handleTransactionFailed = (error: string) => { | ||
| console.error("Transaction failed:", error); | ||
| toast("Transaction failed"); | ||
| }; | ||
|
|
||
| provider.on("transactionComplete", handleTransactionComplete); | ||
| provider.on("transactionFailed", handleTransactionFailed); | ||
|
|
||
| return () => { | ||
| provider.off("transactionComplete", handleTransactionComplete); | ||
| provider.off("transactionFailed", handleTransactionFailed); | ||
| }; | ||
| }, [provider]); |
There was a problem hiding this comment.
Fix potential memory leak with provider dependency
The useEffect dependency array should include provider to prevent stale closures.
- useEffect(() => {
+ useEffect(() => {
+ if (!provider) return;
+
const handleTransactionComplete = (receipt: any) => {
// ... handler implementation
};
const handleTransactionFailed = (error: string) => {
// ... handler implementation
};
provider.on("transactionComplete", handleTransactionComplete);
provider.on("transactionFailed", handleTransactionFailed);
return () => {
provider.off("transactionComplete", handleTransactionComplete);
provider.off("transactionFailed", handleTransactionFailed);
};
- }, [provider]);
+ }, [provider]); // Now properly depending on provider📝 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.
| useEffect(() => { | |
| const handleTransactionComplete = (receipt: any) => { | |
| console.log("Transaction completed:", receipt); | |
| toast("Transaction completed"); | |
| }; | |
| const handleTransactionFailed = (error: string) => { | |
| console.error("Transaction failed:", error); | |
| toast("Transaction failed"); | |
| }; | |
| provider.on("transactionComplete", handleTransactionComplete); | |
| provider.on("transactionFailed", handleTransactionFailed); | |
| return () => { | |
| provider.off("transactionComplete", handleTransactionComplete); | |
| provider.off("transactionFailed", handleTransactionFailed); | |
| }; | |
| }, [provider]); | |
| useEffect(() => { | |
| if (!provider) return; | |
| const handleTransactionComplete = (receipt: any) => { | |
| console.log("Transaction completed:", receipt); | |
| toast("Transaction completed"); | |
| }; | |
| const handleTransactionFailed = (error: string) => { | |
| console.error("Transaction failed:", error); | |
| toast("Transaction failed"); | |
| }; | |
| provider.on("transactionComplete", handleTransactionComplete); | |
| provider.on("transactionFailed", handleTransactionFailed); | |
| return () => { | |
| provider.off("transactionComplete", handleTransactionComplete); | |
| provider.off("transactionFailed", handleTransactionFailed); | |
| }; | |
| }, [provider]); |
| const handleTransactionComplete = (receipt: any) => { | ||
| console.log("Transaction completed:", receipt); | ||
| toast("Transaction completed"); | ||
| }; |
There was a problem hiding this comment.
Improve type safety and error handling for transaction receipt
The current implementation uses any type and provides minimal error information.
-const handleTransactionComplete = (receipt: any) => {
+interface TransactionReceipt {
+ transactionHash: string;
+ status: boolean;
+ // Add other relevant fields
+}
+
+const handleTransactionComplete = (receipt: TransactionReceipt) => {
console.log("Transaction completed:", receipt);
- toast("Transaction completed");
+ toast.success("Transaction completed", {
+ description: `Transaction hash: ${receipt.transactionHash}`,
+ duration: 5000,
+ });
};📝 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 handleTransactionComplete = (receipt: any) => { | |
| console.log("Transaction completed:", receipt); | |
| toast("Transaction completed"); | |
| }; | |
| interface TransactionReceipt { | |
| transactionHash: string; | |
| status: boolean; | |
| // Add other relevant fields | |
| } | |
| const handleTransactionComplete = (receipt: TransactionReceipt) => { | |
| console.log("Transaction completed:", receipt); | |
| toast.success("Transaction completed", { | |
| description: `Transaction hash: ${receipt.transactionHash}`, | |
| duration: 5000, | |
| }); | |
| }; |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (5)
client/src/hooks/context/signing-policy.tsx (1)
10-10: Clarify the versioning schemeThe addition of the "s0_" prefix suggests a versioning system for message types. Please clarify:
- Is this part of a broader versioning strategy?
- Should we document the versioning scheme?
- Are there plans for handling multiple versions simultaneously?
Consider adding a comment explaining the versioning scheme and its implications for future message type versions.
+ // Message type versioning: + // s0_: Initial version of the message format + // Future versions should follow the pattern: s<n>_Also applies to: 18-18
client/src/ui/modules/rewards/Rewards.tsx (2)
16-16: Consider using path aliases for cleaner importsThe deep relative import path (
../../../../env) could be simplified using path aliases. Consider updating your build configuration to support aliases like@envor including it in the existing@alias.Example:
-import { env } from "../../../../env"; +import { env } from "@env"; // or +import { env } from "@/env";
Line range hint
101-101: Remove unused statementThe line
prizePool;appears to be dead code and should be removed.- prizePool;client/src/ui/modules/chat/Chat.tsx (2)
234-244: Consider implementing tab management controlsWhile the automatic tab creation for direct messages improves user experience, it could lead to UI clutter. Consider:
- Adding a maximum limit for open tabs
- Implementing a tab cleanup mechanism
- Adding user control over automatic tab creation
Example implementation:
const MAX_TABS = 10; // Configure based on UI space const addTabWithLimit = (newTab: Tab) => { const currentTabs = useChatStore.getState().tabs; if (currentTabs.length >= MAX_TABS) { // Remove oldest non-pinned tab const oldestTab = currentTabs .filter(tab => !tab.pinned) .sort((a, b) => a.lastSeen.getTime() - b.lastSeen.getTime())[0]; if (oldestTab) { removeTab(oldestTab.key); } } addTab(newTab); };
206-207: Consider memoizing the addTab selectorThe
addTabselector could be memoized to prevent unnecessary rerenders.- const addTab = useChatStore((state) => state.addTab); + const addTab = useMemo(() => + useChatStore((state) => state.addTab), + [] + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
client/src/hooks/context/signing-policy.tsx(1 hunks)client/src/ui/modules/chat/Chat.tsx(2 hunks)client/src/ui/modules/chat/InputField.tsx(2 hunks)client/src/ui/modules/rewards/Rewards.tsx(3 hunks)
🔇 Additional comments (3)
client/src/hooks/context/signing-policy.tsx (1)
10-17: Verify message type references across the codebase
The message type has been renamed from "eternum-Message" to "s0_eternum-Message". This change could affect existing message signing and verification functionality.
Let's verify all references to this message type:
✅ Verification successful
Let's examine the actual content of these files to verify if all references have been properly updated.
Message type renaming has been consistently updated across all references
The message type renaming from "eternum-Message" to "s0_eternum-Message" has been properly updated in all relevant files:
client/src/hooks/context/signing-policy.tsx: Both type definition and primaryType updatedclient/src/ui/modules/chat/InputField.tsx: Type definition and primaryType properly updatedcontracts/manifest_dev.json: Tag updated to new formatcontracts/manifest_prod.json: Tag updated to new format
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for references to both old and new message type names
# to ensure all occurrences have been updated.
echo "Checking for old message type references..."
rg -l "eternum-Message"
echo "Checking for new message type references..."
rg -l "s0_eternum-Message"
echo "Checking for any message signing/verification related files..."
fd -e ts -e tsx | rg -l "sign|verify|message"
Length of output: 721
Script:
#!/bin/bash
# Check the content of files that contain message type references
echo "=== client/src/ui/modules/chat/InputField.tsx ==="
rg -A 5 -B 5 "eternum-Message|s0_eternum-Message" "client/src/ui/modules/chat/InputField.tsx"
echo -e "\n=== contracts/manifest_dev.json ==="
rg -A 5 -B 5 "eternum-Message|s0_eternum-Message" "contracts/manifest_dev.json"
echo -e "\n=== contracts/manifest_prod.json ==="
rg -A 5 -B 5 "eternum-Message|s0_eternum-Message" "contracts/manifest_prod.json"
Length of output: 2079
client/src/ui/modules/rewards/Rewards.tsx (1)
Line range hint 20-187: Overall component implementation looks good
The Rewards component effectively manages game rewards and leaderboard functionality. The changes to environment variable access are consistent, and the component's core logic is well-structured with appropriate use of React hooks and state management.
client/src/ui/modules/chat/InputField.tsx (1)
132-132: Verify the chain ID change impact
The chain ID has been changed from a hardcoded hex value to "SN_SEPOLIA". This change needs verification to ensure it matches the intended network configuration.
✅ Verification successful
Let me gather more specific information about the chain ID configuration.
Let me check one more configuration to ensure we have a complete picture.
Chain ID "SN_SEPOLIA" is consistently used in the codebase
The chain ID "SN_SEPOLIA" is used consistently in both the chat input field (InputField.tsx) and signing policy context (signing-policy.tsx). This appears to be a deliberate configuration for the Starknet Sepolia network, and the change from a hex value to this named identifier aligns with the codebase's usage pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for chain ID usage and configuration
# Test: Look for other chain ID references to ensure consistency
# Search for chain ID references
echo "Searching for chain ID references..."
rg -i "chainid|chain_id" --type ts --type tsx
# Search for network configuration files
echo "Searching for network configuration files..."
fd -e ts -e json -e yaml -e yml -e env "config|network|chain"
Length of output: 1098
Script:
#!/bin/bash
# Search for SN_SEPOLIA and chain configurations in specific files
echo "Checking chain configurations..."
rg "SN_SEPOLIA|chainId" client/src/dojo/setupNetwork.ts client/dojoConfig.ts
# Check the specific file for context around the chain ID usage
echo "Checking InputField.tsx context..."
rg -B 5 -A 5 "chainId.*SN_SEPOLIA" client/src/ui/modules/chat/InputField.tsx
Length of output: 517
Script:
#!/bin/bash
# Search for chain ID constants or configurations in the codebase
echo "Searching for chain ID definitions..."
rg -l "SN_SEPOLIA"
# Check for any environment configurations
echo "Checking environment files..."
fd -e env -e env.example -x cat {}
Length of output: 297
| @@ -62,7 +63,7 @@ export const Rewards = () => { | |||
| const claimRewards = useCallback(async () => { | |||
| await claim_leaderboard_rewards({ | |||
| signer: account, | |||
| token: process.env.VITE_LORDS_ADDRESS!, | |||
| token: env.VITE_LORDS_ADDRESS!, | |||
There was a problem hiding this comment.
Add environment variable validation
Force-unwrapping the environment variable with ! could lead to runtime errors if the variable is not defined. Consider adding validation or providing a fallback value.
- token: env.VITE_LORDS_ADDRESS!,
+ token: env.VITE_LORDS_ADDRESS ?? (() => {
+ throw new Error('VITE_LORDS_ADDRESS environment variable is not defined');
+ })(),📝 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.
| token: env.VITE_LORDS_ADDRESS!, | |
| token: env.VITE_LORDS_ADDRESS ?? (() => { | |
| throw new Error('VITE_LORDS_ADDRESS environment variable is not defined'); | |
| })(), |
| @@ -92,7 +93,7 @@ | |||
| const getPrizePool = async () => { | |||
| const season_pool_fee_recipient = configManager?.getResourceBridgeFeeSplitConfig().season_pool_fee_recipient; | |||
| const prizePool = await provider.provider.callContract({ | |||
| contractAddress: process.env.VITE_LORDS_ADDRESS!, | |||
| contractAddress: env.VITE_LORDS_ADDRESS!, | |||
There was a problem hiding this comment.
Add environment variable validation
Similar to the previous instance, force-unwrapping the environment variable should be avoided.
- contractAddress: env.VITE_LORDS_ADDRESS!,
+ contractAddress: env.VITE_LORDS_ADDRESS ?? (() => {
+ throw new Error('VITE_LORDS_ADDRESS environment variable is not defined');
+ })(),📝 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.
| contractAddress: env.VITE_LORDS_ADDRESS!, | |
| contractAddress: env.VITE_LORDS_ADDRESS ?? (() => { | |
| throw new Error('VITE_LORDS_ADDRESS environment variable is not defined'); | |
| })(), |
| @@ -64,8 +64,11 @@ export const InputField = ({ currentTab, salt }: { currentTab: Tab; salt: bigint | |||
| const messageInValidAscii = toValidAscii(message); | |||
| const data = generateMessageTypedData(account.address, channel, messageInValidAscii, toHexString(salt)); | |||
|
|
|||
| console.log("data", data); | |||
There was a problem hiding this comment.
Remove console.log statements containing sensitive data
The console.log statements expose sensitive message data and signatures, which could lead to security vulnerabilities if exposed in production.
Apply this diff to remove the debug logs:
- console.log("data", data);
const signature: Signature = await account.signMessage(data);
- console.log("data", signature);
Also applies to: 70-71
…into optimistic
…into optimistic
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (23)
client/src/ui/components/trading/SelectResources.tsx (4)
Line range hint
11-23: Consider adding proper TypeScript typesThe component props are using
anytypes which defeats TypeScript's type safety benefits. Consider defining proper types:export const SelectResources = ({ selectedResourceIds, setSelectedResourceIds, selectedResourceAmounts, setSelectedResourceAmounts, entity_id, }: { - selectedResourceIds: any; - setSelectedResourceIds: any; - selectedResourceAmounts: any; - setSelectedResourceAmounts: any; + selectedResourceIds: number[]; + setSelectedResourceIds: (ids: number[]) => void; + selectedResourceAmounts: Record<number, number>; + setSelectedResourceAmounts: (amounts: Record<number, number>) => void; entity_id: ID; }) => {
Line range hint
27-31: Consider memoizing filtered resourcesThe
unselectedResourcescalculation could be optimized by memoizing the entire resources array first:+ const allResources = useMemo(() => resources, []); const unselectedResources = useMemo( - () => resources.filter((res) => !selectedResourceIds.includes(res.id)), + () => allResources.filter((res) => !selectedResourceIds.includes(res.id)), - [selectedResourceIds], + [allResources, selectedResourceIds], );
Line range hint
42-47: Add error handling for resource balanceThe resource balance fetching should include error handling to prevent potential runtime issues:
- const resource = getBalance(entity_id, id); + const resource = getBalance(entity_id, id); + if (!resource) { + console.error(`Failed to fetch balance for resource ${id}`); + return null; + } const options = [resources.find((res) => res.id === id), ...unselectedResources].map((res: any) => ({
Line range hint
89-91: Consider debouncing number input changesThe
NumberInputonChange handler could benefit from debouncing to prevent excessive state updates:+ const debouncedSetAmount = useMemo( + () => debounce((id: number, value: number) => { + setSelectedResourceAmounts((prev) => ({ + ...prev, + [id]: Math.min(divideByPrecision(resource?.balance || 0), value), + })); + }, 300), + [resource?.balance] + ); <NumberInput className="h-14 " max={divideByPrecision(resource?.balance || 0)} min={1} value={selectedResourceAmounts[id]} - onChange={(value) => { - setSelectedResourceAmounts({ - ...selectedResourceAmounts, - [id]: Math.min(divideByPrecision(resource?.balance || 0), value), - }); - }} + onChange={(value) => debouncedSetAmount(id, value)} />client/src/ui/modules/entity-details/realm/Castle.tsx (1)
19-19: Consider memoizing the getBalance result.While the implementation is correct, consider memoizing the
getBalanceresult if it's being called frequently, especially since it's used within auseMemohook that might recalculate often.Here's a suggested optimization:
- const { getBalance } = useResourceBalance(); + const { getBalance } = useResourceBalance(); + const memoizedBalance = useMemo( + () => (resourceId: number) => getBalance(structureEntityId, resourceId), + [getBalance, structureEntityId] + );Then use
memoizedBalanceinstead ofgetBalancein thecheckBalanceuseMemo.client/src/ui/components/military/ArmyManagementCard.tsx (4)
168-171: Consider adding immediate user feedback for location restrictionsWhile the code correctly prevents troop creation when the army isn't at home, consider showing this information to users immediately when they try to modify troop counts, rather than waiting until they attempt to click the disabled button.
if (!army?.isHome) { canCreate = false; + // Show tooltip or status message + setStatusMessage("Army must be at home base to add troops"); }
194-195: Improve location status display readability and consistencyConsider extracting the location status logic into a separate function and apply consistent styling for all states.
+ const getLocationStatus = (army) => { + if (army.isHome) return { text: "At Base", className: "text-green" }; + if (armyPosition) return { text: "On Map", className: "text-yellow" }; + return { text: "Unknown", className: "text-red" }; + } + + const status = getLocationStatus(army); - <div className="self-center mr-auto px-3 font-bold"> - {army.isHome ? <span className="text-green">At Base</span> : armyPosition ? `On Map` : "Unknown"} - </div> + <div className="self-center mr-auto px-3 font-bold"> + <span className={status.className}>{status.text}</span> + </div>
216-216: Improve prop naming for clarityThe prop name
checkSamePositiondoesn't clearly convey that it's checking if the army is at its home base. Consider renaming for better clarity.- checkSamePosition={army.isHome} + isAtHomeBase={army.isHome}
330-330: Extract button text logicConsider extracting the button text logic to maintain separation of concerns and enable easier text maintenance/localization.
+ const getButtonText = (isHome: boolean) => + isHome ? "Reinforce army" : "Must be at Base to Reinforce"; - {army.isHome ? "Reinforce army" : "Must be at Base to Reinforce"} + {getButtonText(army.isHome)}client/src/ui/modules/military/battle-view/LockedResources.tsx (1)
18-24: Consider extracting common props to avoid duplicationThe split into two
InventoryResourcescomponents improves clarity by separating attacker and defender resources. However, both components share identical styling props.Consider extracting common props to a constant or creating a styled wrapper component:
const resourcesProps = { textSize: "xxs", resourcesIconSize: "sm", className: "grid grid-cols-3 gap-1 overflow-y-auto no-scrollbar" } as const; // Then use as: <InventoryResources {...resourcesProps} entityId={attackersResourcesEscrowEntityId} />client/src/ui/components/worldmap/armies/SelectedArmy.tsx (1)
88-91: Consider safer null handlingWhile the
showTooltipguard ensuresownArmyexists, using non-null assertions (!) can make the code more fragile to future changes. Consider using optional chaining or moving theInventoryResourcesinto a separate component with proper type guards.Example approach:
{ownArmy && ( <InventoryResources entityId={ownArmy.entity_id} className="flex gap-1 h-14 mt-2 overflow-x-auto no-scrollbar" resourcesIconSize="xs" /> )}client/src/ui/components/battles/BattleListItem.tsx (1)
126-138: LGTM! Consider adding visual separation between attacker and defender resources.The split into separate
InventoryResourcescomponents for attackers and defenders improves clarity. Consider adding a visual separator or labels to distinguish between attacker and defender resources more clearly.Add labels or a separator between the resources:
<div> + <div className="text-sm text-gold/50">Attacker Resources:</div> <InventoryResources entityId={battle.attackers_resources_escrow_id} className="flex gap-1 h-14 mt-2 overflow-x-auto no-scrollbar" resourcesIconSize="xs" /> + <div className="text-sm text-gold/50 mt-2">Defender Resources:</div> <InventoryResources entityId={battle.defenders_resources_escrow_id} className="flex gap-1 h-14 mt-2 overflow-x-auto no-scrollbar" resourcesIconSize="xs" /> </div>client/src/dojo/modelManager/ResourceInventoryManager.ts (1)
9-16: Simplify 'entityId' property declaration in the constructorYou can simplify the class by declaring
entityIddirectly in the constructor parameters. This reduces redundancy and enhances readability.Refactored code:
-export class ResourceInventoryManager { - entityId: ID; - - constructor( - private readonly setup: SetupResult, - entityId: ID, - ) { - this.entityId = entityId; - } +export class ResourceInventoryManager { + constructor( + private readonly setup: SetupResult, + public readonly entityId: ID, + ) {}client/src/ui/components/resources/DepositResources.tsx (1)
97-101: Refactor nested ternary operators for readabilityThe nested ternary operators used in determining the button text reduce code readability. Consider using a function to handle the logic more clearly.
Refactored code:
const getButtonText = () => { if (battleInProgress || armyInBattle) { return armyInBattle ? "Army in battle" : "Battle in progress"; } else if (inventoryResources.length === 0 && weight > 0n) { return "Resources syncing..."; } else if (inventoryResources.length === 0 && weight === 0n) { return "No resources to deposit"; } else { return "Deposit Resources"; } }; ... <Button ...> {getButtonText()} </Button>client/src/ui/components/bank/ResourceBar.tsx (1)
Line range hint
44-46: Consider memoizing getBalance to optimize re-rendersThe
getBalancefunction in the useEffect dependencies could cause unnecessary re-renders if it changes identity on each render.Consider using useCallback:
- const { getBalance } = useResourceBalance(); + const { getBalance } = useResourceBalance(); + const memoizedGetBalance = useCallback(getBalance, []); useEffect(() => { - setSelectedResourceBalance(divideByPrecision(getBalance(entityId, Number(resourceId)).balance)); - }, [resourceId, getBalance, entityId]); + setSelectedResourceBalance(divideByPrecision(memoizedGetBalance(entityId, Number(resourceId)).balance)); + }, [resourceId, memoizedGetBalance, entityId]);client/src/ui/components/structures/construction/StructureConstructionMenu.tsx (1)
Line range hint
37-42: Consider memoizing checkBalance for better performanceThe
checkBalancefunction is recreated on every render and could benefit from memoization.Consider using useMemo:
+ const memoizedCheckBalance = useMemo( + () => (cost: any) => + Object.keys(cost).every((resourceId) => { + const resourceCost = cost[Number(resourceId)]; + const balance = getBalance(entityId, resourceCost.resource); + return balance.balance >= multiplyByPrecision(resourceCost.amount); + }), + [getBalance, entityId] + ); - const checkBalance = (cost: any) => - Object.keys(cost).every((resourceId) => { - const resourceCost = cost[Number(resourceId)]; - const balance = getBalance(entityId, resourceCost.resource); - return balance.balance >= multiplyByPrecision(resourceCost.amount); - });client/src/ui/components/resources/TravelInfo.tsx (1)
2-2: LGTM! Consider memoizing getBalanceThe migration to
useResourceBalancehook aligns with React's best practices. Consider memoizing thegetBalancefunction to prevent unnecessary re-renders if it's recreated on each render.- const { getBalance } = useResourceBalance(); + const { getBalance } = useMemo(() => useResourceBalance(), []);Also applies to: 28-28
client/src/ui/modules/world-structures/WorldStructuresMenu.tsx (1)
15-15: Consider caching resource balanceThe resource balance is retrieved on each render. Consider using
useMemoto cache the balance and prevent unnecessary recalculations:const { getBalance } = useResourceBalance(); - const dynamicResources = getBalance(entityId, ResourcesIds.AncientFragment); + const dynamicResources = useMemo( + () => getBalance(entityId, ResourcesIds.AncientFragment), + [entityId, getBalance] + );Also applies to: 155-156
client/src/ui/components/entities/Entity.tsx (1)
Line range hint
99-109: Add type safety to resource mappingConsider adding proper typing for the resource parameter in the map function:
- return entityResources?.map( - (resource: any) => + interface Resource { + resourceId: number; + amount: number; + } + return entityResources?.map( + (resource: Resource) =>client/src/ui/components/resources/ResourceChip.tsx (1)
Line range hint
89-111: Consider memoizing the interval calculation for better performance.The interval calculation in useEffect could benefit from memoization to prevent unnecessary recalculations.
+ const calculateNewBalance = useMemo(() => (realTick: number) => { + const localResource = resourceManager.getResource(); + const localProductionDuration = resourceManager.productionDuration(realTick); + const localDepletionDuration = resourceManager.depletionDuration(realTick); + + return resourceManager.balanceFromComponents( + resourceId, + rate, + netRate > 0, + localResource?.balance, + localProductionDuration, + localDepletionDuration, + ); + }, [resourceManager, rate, netRate, resourceId]); useEffect(() => { // ... existing code ... if (Math.abs(netRate) > 0) { const interval = setInterval(() => { realTick += 1; - const localResource = resourceManager.getResource(); - const localProductionDuration = resourceManager.productionDuration(realTick); - const localDepletionDuration = resourceManager.depletionDuration(realTick); - - const newBalance = resourceManager.balanceFromComponents( - resourceId, - rate, - netRate > 0, - localResource?.balance, - localProductionDuration, - localDepletionDuration, - ); + const newBalance = calculateNewBalance(realTick); setBalance(newBalance); }, tickTime); return () => clearInterval(interval); } }, [setBalance, resourceManager, netRate, resourceId, production]);client/src/ui/components/hyperstructures/ResourceExchange.tsx (1)
Line range hint
71-84: Consider adding validation for resource transfer limits.The
transferResourcesfunction could benefit from additional validation to ensure resource limits aren't exceeded during transfer.const transferResources = async () => { setLoading(true); const fromArmyId = transferDirection === "to" ? giverArmyEntityId : takerArmy?.entity_id || structureEntityId; const toArmyId = transferDirection === "to" ? takerArmy?.entity_id || structureEntityId : giverArmyEntityId; + // Validate resource limits before transfer + const sourceResources = transferDirection === "to" ? giverArmyResources : takerArmyResources; + const isValidTransfer = resourcesList.every((amount, index) => { + if (index % 2 === 0) return true; // Skip resource IDs + const resourceId = resourcesList[index - 1]; + const available = sourceResources.find(r => r.resourceId === resourceId)?.amount || 0; + return amount <= available; + }); + + if (!isValidTransfer) { + setLoading(false); + return; // Consider adding error feedback + } await send_resources({ signer: account, sender_entity_id: fromArmyId!, recipient_entity_id: toArmyId!, resources: resourcesList, }); setLoading(false); setResourcesGiven(Object.fromEntries(Object.keys(resourcesGiven).map((key) => [key, 0]))); };client/src/ui/components/bank/Swap.tsx (2)
Line range hint
63-64: Consider memoizing balance calculations with useMemo dependencies.The balance calculations are correctly memoized, but the dependency array could be more specific to prevent unnecessary recalculations.
-const lordsBalance = useMemo(() => getBalance(entityId, ResourcesIds.Lords).balance, [entityId, getBalance]); -const resourceBalance = useMemo(() => getBalance(entityId, resourceId).balance, [entityId, resourceId, getBalance]); +const lordsBalance = useMemo(() => getBalance(entityId, ResourcesIds.Lords).balance, [entityId]); +const resourceBalance = useMemo(() => getBalance(entityId, resourceId).balance, [entityId, resourceId]);
Line range hint
93-108: Consider extracting swap operation logic into a custom hook.The swap operation logic could be extracted into a custom hook to improve reusability and separation of concerns.
Example implementation:
const useSwapOperation = (props: { isBuyResource: boolean; setup: any; account: any; bankEntityId: ID; entityId: ID; resourceId: ResourcesIds; resourceAmount: number; onSuccess?: () => void; }) => { const { play: playLordsSound } = useUiSounds(soundSelector.addLords); return useCallback(async () => { const operation = props.isBuyResource ? props.setup.systemCalls.buy_resources : props.setup.systemCalls.sell_resources; try { await operation({ signer: props.account, bank_entity_id: props.bankEntityId, entity_id: props.entityId, resource_type: props.resourceId, amount: multiplyByPrecision(Number(props.resourceAmount.toFixed(2))), }); playLordsSound(); props.onSuccess?.(); } catch (error) { console.error('Swap operation failed:', error); throw error; } }, [props, playLordsSound]); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (31)
client/src/dojo/modelManager/ResourceInventoryManager.ts(1 hunks)client/src/dojo/modelManager/__tests__/__BattleManagerMock__.ts(2 hunks)client/src/hooks/helpers/use-resource-arrivals.tsx(1 hunks)client/src/hooks/helpers/useEntities.tsx(2 hunks)client/src/hooks/helpers/useResources.tsx(2 hunks)client/src/ui/components/bank/AddLiquidity.tsx(2 hunks)client/src/ui/components/bank/ResourceBar.tsx(2 hunks)client/src/ui/components/bank/Swap.tsx(2 hunks)client/src/ui/components/battles/BattleListItem.tsx(1 hunks)client/src/ui/components/construction/SelectPreviewBuilding.tsx(6 hunks)client/src/ui/components/entities/Entity.tsx(3 hunks)client/src/ui/components/hyperstructures/HyperstructureResourceChip.tsx(2 hunks)client/src/ui/components/hyperstructures/ResourceExchange.tsx(2 hunks)client/src/ui/components/military/ArmyChip.tsx(4 hunks)client/src/ui/components/military/ArmyManagementCard.tsx(7 hunks)client/src/ui/components/resources/DepositResources.tsx(4 hunks)client/src/ui/components/resources/InventoryResources.tsx(1 hunks)client/src/ui/components/resources/ResourceChip.tsx(1 hunks)client/src/ui/components/resources/TravelInfo.tsx(2 hunks)client/src/ui/components/structures/construction/StructureConstructionMenu.tsx(3 hunks)client/src/ui/components/trading/MarketModal.tsx(1 hunks)client/src/ui/components/trading/ResourceArrivals.tsx(2 hunks)client/src/ui/components/trading/SelectResources.tsx(2 hunks)client/src/ui/components/worldmap/armies/ActionInfo.tsx(2 hunks)client/src/ui/components/worldmap/armies/ArmyInfoLabel.tsx(1 hunks)client/src/ui/components/worldmap/armies/SelectedArmy.tsx(1 hunks)client/src/ui/components/worldmap/structures/StructureListItem.tsx(1 hunks)client/src/ui/modules/entity-details/realm/Castle.tsx(2 hunks)client/src/ui/modules/military/battle-view/LockedResources.tsx(1 hunks)client/src/ui/modules/navigation/LeftNavigationModule.tsx(3 hunks)client/src/ui/modules/world-structures/WorldStructuresMenu.tsx(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- client/src/ui/components/trading/MarketModal.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- client/src/ui/components/military/ArmyChip.tsx
- client/src/ui/components/construction/SelectPreviewBuilding.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
client/src/dojo/modelManager/__tests__/__BattleManagerMock__.ts
[error] 85-85: This property value named isHome is later overwritten by an object member with the same name.
Overwritten with this value.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property value named isHome
(lint/suspicious/noDuplicateObjectKeys)
🔇 Additional comments (23)
client/src/dojo/modelManager/__tests__/__BattleManagerMock__.ts (1)
Line range hint 85-97: Make isHome configurable in mock generator
The hardcoded isHome value limits test scenarios. Consider making it configurable through function parameters, similar to alive and isMine, to support testing both home and away states.
Apply this diff to make the property configurable:
export const generateMockArmyInfo = (
alive?: boolean,
isMine?: boolean,
battleEntityId?: ID,
battleSide?: BattleSide,
+ isHome?: boolean,
): ArmyInfo => {
return {
entity_id: ARMY_ENTITY_ID,
- isHome: false,
+ isHome: isHome ?? false,🧰 Tools
🪛 Biome (1.9.4)
[error] 85-85: This property value named isHome is later overwritten by an object member with the same name.
Overwritten with this value.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property value named isHome
(lint/suspicious/noDuplicateObjectKeys)
client/src/ui/components/trading/SelectResources.tsx (1)
1-1: LGTM: Hook migration improves reactivity
The switch from getResourceBalance to useResourceBalance hook is a good improvement that better aligns with React's patterns and should provide better reactivity for resource balance updates.
Also applies to: 24-24
client/src/ui/components/military/ArmyManagementCard.tsx (2)
54-55: Handle undefined army entity_id
Using army?.entity_id || 0 as a fallback could lead to issues if 0 is not a valid army ID. Consider adding proper validation.
135-139: Verify the use of multiplyByPrecision for troop counts
Using multiplyByPrecision on troop counts might be incorrect as troop counts are typically whole numbers. This could lead to inflated values.
client/src/hooks/helpers/use-resource-arrivals.tsx (1)
1-170: Well-structured implementation of custom hooks
The usePlayerArrivals and usePlayerArrivalsNotificationLength hooks are thoughtfully implemented, adhering to React best practices. The use of useEffect, useCallback, and useMemo is appropriate, and dependencies are correctly specified. Subscription management is handled properly to prevent memory leaks.
client/src/ui/components/trading/ResourceArrivals.tsx (1)
18-18: Update component props to include the new arrivals prop
The AllResourceArrivals component now expects an arrivals prop of type ArrivalInfo[]. Ensure that all instances where AllResourceArrivals is used are updated to pass the required arrivals data to prevent any runtime errors.
client/src/ui/modules/navigation/LeftNavigationModule.tsx (3)
9-9: Confirm the updated import path for usePlayerArrivalsNotificationLength
The import statement for usePlayerArrivalsNotificationLength has been updated. Verify that the new path is correct and that the module exports the expected functions.
76-76: Destructure arrivals correctly from the hook
The usePlayerArrivalsNotificationLength hook now returns an object containing both notificationLength and arrivals. Ensure that this destructuring aligns with the hook's implementation and that these variables are used appropriately in the component.
302-302: Pass the required arrivals prop to AllResourceArrivals component
When rendering the <AllResourceArrivals> component, ensure that the arrivals prop is correctly passed. This change is crucial for the component to function properly with the new props structure.
client/src/ui/components/worldmap/armies/ArmyInfoLabel.tsx (2)
111-111: Verify max prop compatibility
The max={6} prop is used here but not in other InventoryResources instances. Please verify:
- If this prop is still supported with the new
entityIdprop - If other instances should also use this prop for consistency
✅ Verification successful
The max prop is correctly implemented and used consistently
The search results show that:
- The
maxprop is properly supported in theInventoryResourcescomponent with a default value ofInfinity(client/src/ui/components/resources/InventoryResources.tsx) - The prop is used in two specific cases where inventory display needs to be limited:
ArmyInfoLabel.tsx:max={6}StructureListItem.tsx:max={maxInventory}
Other instances don't specify the max prop, which means they use the default Infinity value - this is intentional as those views (battles, army chips) should show all resources without limitation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of max prop in InventoryResources
rg -l "InventoryResources.*max=" --type typescript
Length of output: 87
Script:
#!/bin/bash
# Search for InventoryResources component usage patterns
rg "InventoryResources" -A 2 -B 2
# Also search using ast-grep to catch JSX usage patterns
ast-grep --pattern '<InventoryResources $$$>'
Length of output: 7838
Line range hint 18-24: Verify backwards compatibility of InventoryResources API change
The change from entityIds array to single entityId prop appears to be a breaking change in the InventoryResources component API. This pattern is consistently applied across multiple components, but there might be other consumers not shown in this diff.
Also applies to: 88-91, 111-111
client/src/ui/components/worldmap/structures/StructureListItem.tsx (1)
185-185: LGTM! Prop signature change aligns with the new resource handling pattern.
The change from entityIds array to single entityId prop simplifies the interface while maintaining all functionality, including dynamic resource handling for FragmentMine structures.
client/src/ui/components/resources/InventoryResources.tsx (1)
Line range hint 23-40: Ensure hooks are called at the top level and dependencies are complete
While using hooks like useResourcesFromBalance and useResourceBalance, ensure they are called at the top level and that all dependencies are included in any memoization functions.
Please double-check that all hooks comply with React's rules of hooks and that dependencies in useMemo and useEffect are complete to prevent potential bugs.
client/src/ui/components/hyperstructures/HyperstructureResourceChip.tsx (1)
Line range hint 31-36: Confirm hook usage and check for any missing dependencies
The useResourceBalance hook replaces getResourceBalance. Ensure that this change maintains the intended functionality and that all dependencies are properly managed.
Please verify that useResourceBalance provides the necessary data and that any memoized values or effects include all required dependencies.
client/src/ui/components/worldmap/armies/ActionInfo.tsx (1)
3-3: LGTM! Hook migration looks correct
The transition to useResourceBalance hook is properly implemented:
- Hook is called at the top level of the component
- Destructuring of
getBalanceis correct - Usage in ResourceCost components is consistent
Also applies to: 19-19
client/src/ui/components/bank/ResourceBar.tsx (1)
1-1: LGTM! Hook migration and new props look good
The changes are well implemented:
- Hook usage follows React rules
- New focus/blur props are properly typed and integrated
Also applies to: 36-36
client/src/ui/components/structures/construction/StructureConstructionMenu.tsx (1)
2-2: LGTM! Hook migration in both components is correct
The transition to useResourceBalance is properly implemented in both StructureConstructionMenu and StructureInfo components:
- Hooks are called at the top level
- Usage is consistent across components
Also applies to: 29-29, 103-103
client/src/ui/components/entities/Entity.tsx (1)
46-51: LGTM! Well-structured resource management
The implementation properly uses useMemo for caching entityResources and correctly depends on the weight component value. This optimization prevents unnecessary recalculations.
client/src/ui/components/resources/ResourceChip.tsx (1)
24-24: LGTM! Resource manager initialization follows React patterns.
The useResourceManager hook is properly initialized with the required dependencies.
client/src/ui/components/hyperstructures/ResourceExchange.tsx (1)
3-3: LGTM! Hook migration improves component reactivity.
The change from utility function to hook is consistent with the codebase-wide refactoring.
Also applies to: 34-34
client/src/hooks/helpers/useEntities.tsx (2)
17-17: LGTM! Import change aligns with React best practices.
The change from getResourcesUtils to useResourcesUtils follows React's convention of using hooks for managing state and side effects.
201-201: Verify hook usage consistency across the codebase.
The transition to using useResourcesUtils hook is correct. However, let's verify that this pattern is consistently applied across the codebase to prevent mixing hook and non-hook approaches.
✅ Verification successful
Let me check the actual implementation and usage patterns to ensure we have a complete picture.
Hook usage is consistent across the codebase
The verification shows that the codebase consistently uses the useResourcesUtils hook pattern. All instances of getResourcesFromBalance are properly obtained through the useResourcesUtils hook, and there are no direct declarations of getResourcesUtils. The implementation and usage patterns are uniform across all relevant components.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining usage of getResourcesUtils
# to ensure consistent migration to useResourcesUtils
echo "Checking for remaining getResourcesUtils usage..."
rg "getResourcesUtils" -l
echo "Checking for useResourcesUtils adoption..."
rg "useResourcesUtils" -l
Length of output: 559
Script:
#!/bin/bash
# Check the implementation and usage patterns
echo "Checking useResourcesUtils implementation..."
ast-grep --pattern 'export const useResourcesUtils = () => {
$$$
}'
echo "\nChecking for any direct getResourcesUtils function declarations..."
ast-grep --pattern 'const getResourcesUtils = $$$'
echo "\nChecking actual usage contexts..."
rg "getResourcesFromBalance|useResourcesUtils" -A 2
Length of output: 6004
client/src/ui/components/bank/Swap.tsx (1)
5-5: LGTM! Hook usage is properly implemented.
The transition to useResourceBalance hook and its usage through destructuring getBalance is correct. The implementation properly handles memoization of balance calculations.
Also applies to: 32-32
| @@ -82,6 +82,7 @@ export const generateMockArmyInfo = ( | |||
| ): ArmyInfo => { | |||
| return { | |||
| entity_id: ARMY_ENTITY_ID, | |||
| isHome: true, | |||
There was a problem hiding this comment.
Fix duplicate isHome property
The isHome property is defined twice with different values (true on line 85 and false on line 97). The second value will override the first one, which is likely unintentional and could lead to confusing test behavior.
Remove one of the duplicate properties and ensure the intended value is used:
export const generateMockArmyInfo = (
alive?: boolean,
isMine?: boolean,
battleEntityId?: ID,
battleSide?: BattleSide,
): ArmyInfo => {
return {
entity_id: ARMY_ENTITY_ID,
- isHome: true,
battle_id: battleEntityId ?? BATTLE_ENTITY_ID,
battle_side: BattleSide[battleSide ?? BattleSide.Attack] ?? BattleSide[BattleSide.Attack],
troops: {
knight_count: alive ? ARMY_TROOP_COUNT : 0n,
paladin_count: alive ? ARMY_TROOP_COUNT : 0n,
crossbowman_count: 0n,
},
health: { entity_id: ARMY_ENTITY_ID, current: alive ? ARMY_INITIAL_HEALTH : 0n, lifetime: ARMY_INITIAL_HEALTH },
name: "test army",
isMine: isMine ?? false,
isMercenary: false,
isHome: false,Also applies to: 97-97
🧰 Tools
🪛 Biome (1.9.4)
[error] 85-85: This property value named isHome is later overwritten by an object member with the same name.
Overwritten with this value.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property value named isHome
(lint/suspicious/noDuplicateObjectKeys)
| @@ -1,7 +1,7 @@ | |||
| import { configManager } from "@/dojo/setup"; | |||
| import { useDojo } from "@/hooks/context/DojoContext"; | |||
| import { useGetRealm } from "@/hooks/helpers/useRealm"; | |||
| import { getResourceBalance } from "@/hooks/helpers/useResources"; | |||
| import { useResourceBalance } from "@/hooks/helpers/useResources"; | |||
There was a problem hiding this comment.
💡 Codebase verification
Migration to useResourceBalance hook is incomplete
There's still one file using the old getResourceBalance import:
landing/src/components/ui/SelectResources.tsx
All other files consistently use the new useResourceBalance hook with the correct pattern of destructuring getBalance from it.
🔗 Analysis chain
LGTM! Consistent with React best practices.
The migration from getResourceBalance function to useResourceBalance hook aligns with React's recommended patterns for managing state and side effects.
Let's verify the consistent usage of this hook across the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining usage of old getResourceBalance import
# and verify consistent hook usage pattern
# Check for any remaining old imports
rg "import.*getResourceBalance.*from.*@/hooks/helpers/useResources"
# Verify the hook usage pattern
rg -A 2 "useResourceBalance.*"
Length of output: 9830
| public onOffloadAll = async (receiverEntityId: ID, inventoryResources: Resource[]) => { | ||
| const overrideId = uuid(); | ||
| this._optimisticOffloadAll(overrideId, receiverEntityId, inventoryResources); | ||
|
|
||
| if (inventoryResources.length > 0) { | ||
| await this.setup.systemCalls | ||
| .send_resources({ | ||
| sender_entity_id: this.entityId, | ||
| recipient_entity_id: receiverEntityId, | ||
| resources: inventoryResources.flatMap((resource) => [resource.resourceId, resource.amount]), | ||
| signer: useAccountStore.getState().account!, | ||
| }) | ||
| .finally(() => { | ||
| this.setup.components.Resource.removeOverride(overrideId); | ||
| this.setup.components.Weight.removeOverride(overrideId); | ||
| }); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Ensure overrides are removed regardless of resource length
If inventoryResources.length === 0, the overrides added by _optimisticOffloadAll are not removed because the finally block is within the if condition. This can lead to unintended side effects.
To fix this, move the override removal outside the if block:
public onOffloadAll = async (receiverEntityId: ID, inventoryResources: Resource[]) => {
const overrideId = uuid();
this._optimisticOffloadAll(overrideId, receiverEntityId, inventoryResources);
- if (inventoryResources.length > 0) {
- await this.setup.systemCalls
- .send_resources({
- sender_entity_id: this.entityId,
- recipient_entity_id: receiverEntityId,
- resources: inventoryResources.flatMap((resource) => [resource.resourceId, resource.amount]),
- signer: useAccountStore.getState().account!,
- })
- .finally(() => {
- this.setup.components.Resource.removeOverride(overrideId);
- this.setup.components.Weight.removeOverride(overrideId);
- });
- }
+ try {
+ if (inventoryResources.length > 0) {
+ await this.setup.systemCalls.send_resources({
+ sender_entity_id: this.entityId,
+ recipient_entity_id: receiverEntityId,
+ resources: inventoryResources.flatMap((resource) => [resource.resourceId, resource.amount]),
+ signer: useAccountStore.getState().account!,
+ });
+ }
+ } finally {
+ this.setup.components.Resource.removeOverride(overrideId);
+ this.setup.components.Weight.removeOverride(overrideId);
+ }
};📝 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.
| public onOffloadAll = async (receiverEntityId: ID, inventoryResources: Resource[]) => { | |
| const overrideId = uuid(); | |
| this._optimisticOffloadAll(overrideId, receiverEntityId, inventoryResources); | |
| if (inventoryResources.length > 0) { | |
| await this.setup.systemCalls | |
| .send_resources({ | |
| sender_entity_id: this.entityId, | |
| recipient_entity_id: receiverEntityId, | |
| resources: inventoryResources.flatMap((resource) => [resource.resourceId, resource.amount]), | |
| signer: useAccountStore.getState().account!, | |
| }) | |
| .finally(() => { | |
| this.setup.components.Resource.removeOverride(overrideId); | |
| this.setup.components.Weight.removeOverride(overrideId); | |
| }); | |
| } | |
| }; | |
| public onOffloadAll = async (receiverEntityId: ID, inventoryResources: Resource[]) => { | |
| const overrideId = uuid(); | |
| this._optimisticOffloadAll(overrideId, receiverEntityId, inventoryResources); | |
| try { | |
| if (inventoryResources.length > 0) { | |
| await this.setup.systemCalls.send_resources({ | |
| sender_entity_id: this.entityId, | |
| recipient_entity_id: receiverEntityId, | |
| resources: inventoryResources.flatMap((resource) => [resource.resourceId, resource.amount]), | |
| signer: useAccountStore.getState().account!, | |
| }); | |
| } | |
| } finally { | |
| this.setup.components.Resource.removeOverride(overrideId); | |
| this.setup.components.Weight.removeOverride(overrideId); | |
| } | |
| }; |
| inventoryResources.forEach((resource) => { | ||
| const entity = getEntityIdFromKeys([BigInt(receiverEntityId), BigInt(resource.resourceId)]); | ||
| const receiverBalance = getComponentValue(this.setup.components.Resource, entity)?.balance || 0n; | ||
|
|
||
| // optimistically update the balance of the receiver | ||
| this.setup.components.Resource.addOverride(overrideId, { | ||
| entity, | ||
| value: { | ||
| resource_type: resource.resourceId, | ||
| balance: receiverBalance + BigInt(resource.amount), | ||
| }, | ||
| }); | ||
| }); | ||
|
|
||
| const entity = getEntityIdFromKeys([BigInt(this.entityId)]); | ||
|
|
||
| this.setup.components.Weight.addOverride(overrideId, { | ||
| entity, | ||
| value: { | ||
| value: 0n, | ||
| }, | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Rename variables to avoid re-declaring 'entity' in the same scope
The variable entity is declared twice in the same method scope (lines 24 and 37), which can lead to confusion. Renaming the variables improves clarity.
Suggested changes:
inventoryResources.forEach((resource) => {
- const entity = getEntityIdFromKeys([BigInt(receiverEntityId), BigInt(resource.resourceId)]);
+ const receiverResourceEntityId = getEntityIdFromKeys([BigInt(receiverEntityId), BigInt(resource.resourceId)]);
const receiverBalance = getComponentValue(this.setup.components.Resource, receiverResourceEntityId)?.balance || 0n;
// Optimistically update the balance of the receiver
this.setup.components.Resource.addOverride(overrideId, {
- entity,
+ entity: receiverResourceEntityId,
value: {
resource_type: resource.resourceId,
balance: receiverBalance + BigInt(resource.amount),
},
});
});
-const entity = getEntityIdFromKeys([BigInt(this.entityId)]);
+const currentEntityId = getEntityIdFromKeys([BigInt(this.entityId)]);
this.setup.components.Weight.addOverride(overrideId, {
- entity,
+ entity: currentEntityId,
value: {
value: 0n,
},
});📝 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.
| inventoryResources.forEach((resource) => { | |
| const entity = getEntityIdFromKeys([BigInt(receiverEntityId), BigInt(resource.resourceId)]); | |
| const receiverBalance = getComponentValue(this.setup.components.Resource, entity)?.balance || 0n; | |
| // optimistically update the balance of the receiver | |
| this.setup.components.Resource.addOverride(overrideId, { | |
| entity, | |
| value: { | |
| resource_type: resource.resourceId, | |
| balance: receiverBalance + BigInt(resource.amount), | |
| }, | |
| }); | |
| }); | |
| const entity = getEntityIdFromKeys([BigInt(this.entityId)]); | |
| this.setup.components.Weight.addOverride(overrideId, { | |
| entity, | |
| value: { | |
| value: 0n, | |
| }, | |
| }); | |
| }; | |
| inventoryResources.forEach((resource) => { | |
| const receiverResourceEntityId = getEntityIdFromKeys([BigInt(receiverEntityId), BigInt(resource.resourceId)]); | |
| const receiverBalance = getComponentValue(this.setup.components.Resource, receiverResourceEntityId)?.balance || 0n; | |
| // optimistically update the balance of the receiver | |
| this.setup.components.Resource.addOverride(overrideId, { | |
| entity: receiverResourceEntityId, | |
| value: { | |
| resource_type: resource.resourceId, | |
| balance: receiverBalance + BigInt(resource.amount), | |
| }, | |
| }); | |
| }); | |
| const currentEntityId = getEntityIdFromKeys([BigInt(this.entityId)]); | |
| this.setup.components.Weight.addOverride(overrideId, { | |
| entity: currentEntityId, | |
| value: { | |
| value: 0n, | |
| }, | |
| }); |
| setIsLoading(true); | ||
| await depositManager.onOffloadAll(receiverEntityId, inventoryResources).then(() => { | ||
| setIsLoading(false); | ||
| }); |
There was a problem hiding this comment.
Ensure 'isLoading' state is reset even if offloading fails
If depositManager.onOffloadAll throws an error, setIsLoading(false) won't be called, leaving the button in a loading state indefinitely.
Use a try...finally block to guarantee setIsLoading(false) is called:
setIsLoading(true);
-await depositManager.onOffloadAll(receiverEntityId, inventoryResources).then(() => {
- setIsLoading(false);
-});
+try {
+ await depositManager.onOffloadAll(receiverEntityId, inventoryResources);
+} finally {
+ setIsLoading(false);
+}📝 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.
| setIsLoading(true); | |
| await depositManager.onOffloadAll(receiverEntityId, inventoryResources).then(() => { | |
| setIsLoading(false); | |
| }); | |
| setIsLoading(true); | |
| try { | |
| await depositManager.onOffloadAll(receiverEntityId, inventoryResources); | |
| } finally { | |
| setIsLoading(false); | |
| } |
| return useMemo(() => { | ||
| if (!weight?.value && capacityCategory?.category !== CapacityConfigCategory[CapacityConfigCategory.Structure]) | ||
| return []; | ||
|
|
||
| return resources | ||
| .map(({ id }) => { | ||
| const resourceManager = new ResourceManager(setup, entityId, id); | ||
| const balance = resourceManager.balance(currentDefaultTick); | ||
| return { resourceId: id, amount: balance }; | ||
| }) | ||
| .filter(({ amount }) => amount > 0); | ||
| }, [weight, entityId, currentDefaultTick]); |
There was a problem hiding this comment.
Include missing dependencies in 'useMemo' dependency array
The useMemo hook in useResourcesFromBalance uses variables (setup, capacityCategory) that are not included in its dependency array. This can cause stale data or unexpected behavior.
Add all external variables to the dependency array:
return useMemo(() => {
if (
!weight?.value &&
capacityCategory?.category !== CapacityConfigCategory[CapacityConfigCategory.Structure]
)
return [];
return resources
.map(({ id }) => {
const resourceManager = new ResourceManager(setup, entityId, id);
const balance = resourceManager.balance(currentDefaultTick);
return { resourceId: id, amount: balance };
})
.filter(({ amount }) => amount > 0);
-}, [weight, entityId, currentDefaultTick]);
+}, [weight, capacityCategory, resources, setup, entityId, currentDefaultTick]);📝 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.
| return useMemo(() => { | |
| if (!weight?.value && capacityCategory?.category !== CapacityConfigCategory[CapacityConfigCategory.Structure]) | |
| return []; | |
| return resources | |
| .map(({ id }) => { | |
| const resourceManager = new ResourceManager(setup, entityId, id); | |
| const balance = resourceManager.balance(currentDefaultTick); | |
| return { resourceId: id, amount: balance }; | |
| }) | |
| .filter(({ amount }) => amount > 0); | |
| }, [weight, entityId, currentDefaultTick]); | |
| return useMemo(() => { | |
| if (!weight?.value && capacityCategory?.category !== CapacityConfigCategory[CapacityConfigCategory.Structure]) | |
| return []; | |
| return resources | |
| .map(({ id }) => { | |
| const resourceManager = new ResourceManager(setup, entityId, id); | |
| const balance = resourceManager.balance(currentDefaultTick); | |
| return { resourceId: id, amount: balance }; | |
| }) | |
| .filter(({ amount }) => amount > 0); | |
| }, [weight, capacityCategory, resources, setup, entityId, currentDefaultTick]); |
| @@ -1,7 +1,7 @@ | |||
| import { MarketManager } from "@/dojo/modelManager/MarketManager"; | |||
| import { useDojo } from "@/hooks/context/DojoContext"; | |||
| import { useEntities } from "@/hooks/helpers/useEntities"; | |||
| import { getResourceBalance } from "@/hooks/helpers/useResources"; | |||
| import { useResourceBalance } from "@/hooks/helpers/useResources"; | |||
There was a problem hiding this comment.
💡 Codebase verification
Migration to useResourceBalance hook is incomplete
The verification found inconsistencies in the migration:
landing/src/hooks/helpers/useResources.tsxcontains the oldgetResourceBalancefunction that needs to be removed since it's been replaced byuseResourceBalancehooklanding/src/components/ui/SelectResources.tsxis still using the oldgetResourceBalancefunction and needs to be updated to use the newuseResourceBalancehook
All other files in the codebase are correctly using the new hook with a consistent pattern of destructuring getBalance from useResourceBalance.
🔗 Analysis chain
LGTM! Verify consistent hook usage across the codebase.
The migration from getResourceBalance utility to useResourceBalance hook aligns with React best practices and improves reactivity.
Also applies to: 30-30
Remaining instances of getResourceBalance need to be migrated
The verification revealed two files still using the old getResourceBalance utility:
landing/src/components/ui/SelectResources.tsxlanding/src/hooks/helpers/useResources.tsx
Let's verify if these are actual usages or just references:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify consistent usage of useResourceBalance across the codebase
echo "Checking for any remaining instances of getResourceBalance..."
rg "getResourceBalance" -l
echo "Verifying consistent hook usage pattern..."
rg "useResourceBalance" -A 2
Length of output: 9987
Script:
#!/bin/bash
# Check the actual usage of getResourceBalance in the remaining files
echo "Checking getResourceBalance usage in SelectResources.tsx..."
rg "getResourceBalance" -A 2 "landing/src/components/ui/SelectResources.tsx"
echo "Checking getResourceBalance usage in useResources.tsx..."
rg "getResourceBalance" -A 2 "landing/src/hooks/helpers/useResources.tsx"
Length of output: 969
Summary by CodeRabbit
New Features
Population,Resource,Weight,Army, andBuildingQuantityv2components to enhance client-side functionality.isHomeproperty to indicate if an army is at its home position.ToasterandTransactionNotificationcomponents for custom notification handling.ResourceInventoryManagerfor managing resource offloading between entities.Bug Fixes
Refactor
ArmyManagementCardcomponent.