Closed edisontim 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 26, 2024 1:09pm |
β±οΈ Estimated effort to review [1-5] | 4 |
π§ͺ Relevant tests | No |
π Security concerns | No |
β‘ Key issues to review |
Possible Bug: The implementation in BattleManager.ts for updating troops based on health might lead to incorrect troop counts if the health is zero or the calculations are not handled properly. This could result in unexpected behavior during battle simulations. |
Code Duplication: There seems to be a pattern of repeated logic, especially in methods like attackingDelta and defendingDelta in BattleManager.ts . Consider refactoring these to reduce redundancy and improve maintainability. | |
Data Handling: In useRealm.tsx , the change from hexToAscii to shortString.decodeShortString needs careful validation to ensure that the address names are decoded correctly without data loss or corruption. |
Category | Suggestion | Score |
Possible bug |
Use optional chaining when accessing properties of potentially undefined objects to enhance code safety___ **Replace the direct access of properties with optional chaining to prevent runtime errorsif the properties are undefined.** [client/src/ui/modules/military/battle-view/BattleActions.tsx [73-84]](https://github.com/BibliothecaDAO/eternum/pull/1024/files#diff-a2b38e5f9c040c3591adb945a143c3fe234beb433a77a677a8d069ea47a6a462R73-R84) ```diff -battle?.entity_id! !== 0n && battle?.entity_id === BigInt(selectedArmy!.battle_id) -selectedArmy!.entity_id +battle?.entity_id !== 0n && battle?.entity_id === BigInt(selectedArmy?.battle_id) +selectedArmy?.entity_id ``` Suggestion importance[1-10]: 10Why: Optional chaining is a robust way to prevent runtime errors when accessing properties of objects that might be undefined, significantly enhancing code safety. | 10 |
Ensure type consistency by using
___
**Use | 9 | |
Add null checks to prevent errors from accessing properties on an undefined object___ **Add null checks forselectedArmy before performing operations to ensure the object is not undefined.** [client/src/ui/modules/military/battle-view/BattleActions.tsx [73-136]](https://github.com/BibliothecaDAO/eternum/pull/1024/files#diff-a2b38e5f9c040c3591adb945a143c3fe234beb433a77a677a8d069ea47a6a462R73-R136) ```diff -if (selectedArmy!.entity_id) +if (selectedArmy && selectedArmy.entity_id) ``` Suggestion importance[1-10]: 9Why: Adding null checks ensures that operations are only performed on defined objects, preventing potential runtime errors and improving code reliability. | 9 | |
Ensure both attackers and defenders have resources before rendering___ **Correct the conditional rendering logic to ensure that the UI elements are displayed onlywhen both attackers and defenders have resources.** [client/src/ui/modules/military/battle-view/LockedResources.tsx [20]](https://github.com/BibliothecaDAO/eternum/pull/1024/files#diff-359d4a5ec2ea695cafb43989d9412232fc58a85aae799cdc84e83e4b128d9f42R20-R20) ```diff -{attackersResources.length > 0 || defendersResources.length > 0 ? ( +{attackersResources.length > 0 && defendersResources.length > 0 ? ( ``` Suggestion importance[1-10]: 9Why: Correcting the conditional rendering logic to check that both attackers and defenders have resources before rendering prevents potential UI issues and ensures accurate display of information. | 9 | |
Add a check to ensure the
___
**Consider adding a check for | 8 | |
Possible issue |
Improve handling of undefined
___
**Consider handling the case where | 8 |
Maintainability |
Refactor nested ternary operators to a function for clarity___ **Refactor the nested ternary operators in thebattleStatus calculation for better readability and maintainability. Consider using a function to handle the logic.** [client/src/ui/modules/military/battle-view/BattleProgressBar.tsx [71-81]](https://github.com/BibliothecaDAO/eternum/pull/1024/files#diff-f9a547b6a282b885d288bf085e915af12ff12ce991fc19051cff70106c89c9deR71-R81) ```diff -return ownArmySide === "Attack" - ? Number(attackingHealthPercentage) === 100 - ? "You Won" - : Number(attackingHealthPercentage) === 0 - ? "You Lost" - : undefined - : Number(defendingHealthPercentage) === 100 - ? "You Won" - : Number(defendingHealthPercentage) === 0 - ? "You Lost" - : undefined; +return getBattleStatus(ownArmySide, attackingHealthPercentage, defendingHealthPercentage); +function getBattleStatus(side, attackingPercentage, defendingPercentage) { + if (side === "Attack") { + return attackingPercentage === 100 ? "You Won" : attackingPercentage === 0 ? "You Lost" : undefined; + } else { + return defendingPercentage === 100 ? "You Won" : defendingPercentage === 0 ? "You Lost" : undefined; + } +} + ``` Suggestion importance[1-10]: 7Why: Refactoring the nested ternary operators into a function improves readability and maintainability, making the code easier to understand and modify in the future. | 7 |
Best practice |
Add condition to
___
**Ensure that | 6 |
PR Type
Enhancement, Bug fix
Description
Changes walkthrough π
7 files
BattleManager.ts
Refactor BattleManager to include troop updates and damage
calculations.
client/src/dojo/modelManager/BattleManager.ts
getUpdatedTroops
method to update troop counts based on health.getUpdatedBattle
to include damage calculations and updatetroop counts.
attackingDelta
anddefendingDelta
methods to private.Battle.tsx
Add user armies in battle prop and update button styling.
client/src/ui/modules/military/battle-view/Battle.tsx
userArmiesInBattle
prop to Battle component.BattleActions.tsx
Add army selection and update action handlers in BattleActions.
client/src/ui/modules/military/battle-view/BattleActions.tsx
BattleProgressBar.tsx
Add battle status message and simplify health display.
client/src/ui/modules/military/battle-view/BattleProgressBar.tsx
BattleSideView.tsx
Clear selection after joining a battle.
client/src/ui/modules/military/battle-view/BattleSideView.tsx - Added `clearSelection` call after joining a battle.
BattleView.tsx
Add user armies in battle prop to BattleView.
client/src/ui/modules/military/battle-view/BattleView.tsx - Added `userArmiesInBattle` prop to BattleView component.
EntityAvatar.tsx
Adjust styling for EntityAvatar component.
client/src/ui/modules/military/battle-view/EntityAvatar.tsx - Adjusted styling for EntityAvatar component.
2 files
useRealm.tsx
Update address name decoding and reorganize imports.
client/src/hooks/helpers/useRealm.tsx
hexToAscii
withshortString.decodeShortString
for addressname decoding.
LockedResources.tsx
Fix resource retrieval and add overflow handling in LockedResources.
client/src/ui/modules/military/battle-view/LockedResources.tsx