Closed aymericdelab closed 3 months ago
The latest updates on your projects. Learn more about Vercel for Git βοΈ
Name | Status | Preview | Comments | Updated (UTC) |
---|---|---|---|---|
eternum | β Ready (Inspect) | Visit Preview | π¬ Add feedback | Jun 13, 2024 3:27pm |
β±οΈ Estimated effort to review [1-5] | 3 |
π§ͺ Relevant tests | No |
π Security concerns | No |
β‘ Key issues to review |
Possible Bug: The `getArmiesByBattleId` function in `useArmies.tsx` does not handle the case where `armiesEntityIds` is empty or null. This could lead to runtime errors when trying to format armies with an undefined or null input. |
Data Handling Concern: The `getArmiesByBattleId` function uses a destructuring approach to extract a large number of components and account information, which might not be efficient or necessary for all calls. Consider optimizing this to only extract needed data. | |
Consistency Issue: The `BattleView` component's logic for handling different types of defenders (Army vs. Structure) seems to be inconsistent and could lead to errors if not handled properly. |
Category | Suggestion | Score |
Possible bug |
Add optional chaining to safely access properties___ **Use optional chaining to safely access properties of potentially undefined objects toprevent runtime errors.** [client/src/hooks/helpers/useArmies.tsx [232-249]](https://github.com/BibliothecaDAO/eternum/pull/926/files#diff-6d68c14fbe3262def9a16f121115d2006c7c34bf942b3e5d8ed7c076d2b89027R232-R249) ```diff const armiesEntityIds = runQuery([HasValue(Army, { battle_id })]); return formatArmies( - Array.from(armiesEntityIds), - account.address, + Array.from(armiesEntityIds ?? []), + account?.address, Army, Protectee, EntityName, Health, Quantity, Movable, Capacity, ArrivalTime, Position, EntityOwner, Owner, Realm, Stamina, ); ``` Suggestion importance[1-10]: 9Why: Adding optional chaining is a significant improvement as it prevents potential runtime errors when accessing properties of possibly undefined objects. This enhances the robustness of the code. | 9 |
Maintainability |
Simplify object destructuring for better readability___ **Consider destructuring thecomponents object directly in the function parameter to simplify the code and improve readability.** [client/src/hooks/helpers/useArmies.tsx [210-230]](https://github.com/BibliothecaDAO/eternum/pull/926/files#diff-6d68c14fbe3262def9a16f121115d2006c7c34bf942b3e5d8ed7c076d2b89027R210-R230) ```diff const { - setup: { - components: { - Position, - EntityOwner, - Owner, - Health, - Quantity, - Movable, - Capacity, - ArrivalTime, - Realm, - Army, - Protectee, - EntityName, - Stamina, - }, - }, + setup: { components }, account: { account }, } = useDojo(); +const { + Position, + EntityOwner, + Owner, + Health, + Quantity, + Movable, + Capacity, + ArrivalTime, + Realm, + Army, + Protectee, + EntityName, + Stamina, +} = components; ``` Suggestion importance[1-10]: 7Why: This suggestion improves code readability and maintainability by simplifying the destructuring process. However, it does not address any critical issues or bugs. | 7 |
Standardize the order of union types for consistency___ **Ensure consistent ordering of the types in the union fordefenders to maintain uniformity across different parts of the codebase.** [client/src/hooks/store/useUIStore.tsx [199]](https://github.com/BibliothecaDAO/eternum/pull/926/files#diff-6be3d49574f69bf63b036da26eaba9e59f4ccca6b393ee685ca020398c8465c5R199-R199) ```diff -defenders: { type: CombatTarget; entities: FullStructure | ArmyInfo[] }; +defenders: { type: CombatTarget; entities: ArmyInfo[] | FullStructure }; ``` Suggestion importance[1-10]: 6Why: Standardizing the order of union types improves code consistency and readability. However, this change is minor and does not impact functionality or fix any bugs. | 6 |
PR Type
enhancement, bug fix
Description
getArmiesByBattleId
function to fetch armies by battle ID.BattleViewInfo
type to includeattackers
anddefenders
arrays.setBattleView
to handle multiple attackers and defenders.attackers
anddefenders
structure.Changes walkthrough π
useArmies.tsx
Add function to fetch armies by battle ID.
client/src/hooks/helpers/useArmies.tsx
getArmiesByBattleId
function to fetch armies by battle ID.function.
types.ts
Update BattleViewInfo type for multiple attackers and defenders.
client/src/hooks/store/types.ts
BattleViewInfo
type to includeattackers
anddefenders
arrays.useUIStore.tsx
Modify setBattleView for multiple attackers and defenders.
client/src/hooks/store/useUIStore.tsx
setBattleView
to handle multiple attackers and defenders.StructureCard.tsx
Update setBattleView call in StructureCard component.
client/src/ui/components/hyperstructures/StructureCard.tsx
setBattleView
call to use newattackers
anddefenders
structure.
Battle.tsx
Update setBattleView call in Battle component.
client/src/ui/components/military/Battle.tsx
setBattleView
call to use newattackers
anddefenders
structure.
Battles.tsx
Clean up and pass selectedBattle to BattleLabel.
client/src/ui/components/models/buildings/worldmap/Battles.tsx
selectedBattle
.selectedBattle
toBattleLabel
component.BattleLabel.tsx
Implement battle view logic in BattleLabel component.
client/src/ui/components/worldmap/armies/BattleLabel.tsx
getArmiesByBattleId
and other necessary hooks.defenders.
BattleView.tsx
Update getArmiesAndStructure for new battle view structure.
client/src/ui/modules/military/battle-view/BattleView.tsx
getArmiesAndStructure
to handle newattackers
anddefenders
structure.