Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: change battle UI #1121

Closed
wants to merge 5 commits into from
Closed

feat: change battle UI #1121

wants to merge 5 commits into from

Conversation

edisontim
Copy link
Collaborator

@edisontim edisontim commented Jul 17, 2024

User description

  • beginning of changes to UI to adapt to contract changes:

    • Armies can't join battle once it's done:
      • UI needs to allow multiple battles on the same hex
      • UI needs to restrict players from joining a finished battle
      • UI needs to be able to force the defending army of a structure back into a battle if the battle is finished
    • Show that you can only create 5 armies per Realm
    • Allow players to delete their armies
    • Don't filter out "finished" battles, only filter out battle which have nobody left inside
  • Army panel and entity details design changes


PR Type

Enhancement, Tests


Description

  • Added new utility class .no-scrollbar to hide scrollbars across different browsers.
  • Refactored army-related hooks and types for improved type safety and functionality.
  • Refactored battle-related hooks and added utility functions.
  • Updated various components to use new army and battle data structures.
  • Added new components for displaying entity details.
  • Added tests for useBattlesUtils functions.

Changes walkthrough 📝

Relevant files
Enhancement
14 files
tailwind.config.js
Add utility class to hide scrollbars                                         

client/tailwind.config.js

  • Added a new utility class .no-scrollbar to hide scrollbars across
    different browsers.
  • +12/-0   
    useArmies.tsx
    Refactor army-related hooks and types                                       

    client/src/hooks/helpers/useArmies.tsx

  • Refactored ArmyInfo type to use ComponentValue.
  • Updated formatArmies function to handle undefined values and
    structured cloning.
  • Renamed useArmies to useMovableArmies.
  • Updated various functions to use new utility functions and improved
    type safety.
  • +135/-118
    useBattles.tsx
    Refactor battle-related hooks and add utility functions   

    client/src/hooks/helpers/battles/useBattles.tsx

  • Added getBattle function to fetch and process battle data.
  • Refactored getExtraBattleInformation to use getBattle.
  • Renamed useBattles to useAllBattles.
  • Updated various hooks to use new utility functions and improved type
    safety.
  • +74/-77 
    ArmyManagementCard.tsx
    Update ArmyManagementCard component and add map navigation icon

    client/src/ui/components/military/ArmyManagementCard.tsx

  • Renamed entity prop to army.
  • Updated various references to use army instead of entity.
  • Added ViewOnMapIcon component for map navigation.
  • +68/-27 
    BattleManager.ts
    Refactor BattleManager for improved type safety and functionality

    client/src/dojo/modelManager/BattleManager.ts

  • Refactored BattleManager class to improve type safety.
  • Added methods to update army and battle health.
  • Improved handling of battle and army updates.
  • +122/-45
    useStructures.tsx
    Refactor structure-related hooks for improved type safety

    client/src/hooks/helpers/useStructures.tsx

  • Refactored structure-related hooks to improve type safety.
  • Updated useStructuresPosition and getStructureAtPosition to handle
    undefined values.
  • +47/-60 
    EntityDetails.tsx
    Add components for displaying entity details                         

    client/src/ui/modules/entity-details/EntityDetails.tsx

  • Added new components to display entity details.
  • Implemented logic to determine what information to show based on the
    selected entity.
  • +207/-4 
    useEntities.tsx
    Refactor entity-related hooks and add utility functions   

    client/src/hooks/helpers/useEntities.tsx

  • Refactored useEntities and useEntitiesUtils hooks.
  • Improved type safety and added utility functions.
  • +67/-71 
    ArmyChip.tsx
    Update ArmyChip component with new army data structure and buttons

    client/src/ui/components/military/ArmyChip.tsx

  • Updated ArmyChip component to use new army data structure.
  • Added buttons for map navigation and inventory display.
  • +78/-29 
    Battle.tsx
    Update EnemyArmies component with new army data structure

    client/src/ui/components/military/Battle.tsx

  • Updated EnemyArmies component to use new army data structure.
  • Added logic to handle different battle scenarios.
  • +37/-32 
    Entity.tsx
    Update Entity component with new hooks and utility functions

    client/src/ui/components/entities/Entity.tsx

  • Updated Entity component to use new hooks and utility functions.
  • Improved handling of entity state and battle information.
  • +23/-15 
    BattleActions.tsx
    Update BattleActions component with new data structures   

    client/src/ui/modules/military/battle-view/BattleActions.tsx

  • Updated BattleActions component to use new army and battle data
    structures.
  • Improved handling of battle actions and state.
  • +16/-11 
    ArmyInfoLabel.tsx
    Update ArmyInfoLabel component with new army data structure

    client/src/ui/components/worldmap/armies/ArmyInfoLabel.tsx

  • Updated ArmyInfoLabel component to use new army data structure.
  • Improved display of army information.
  • +13/-15 
    Army.tsx
    Update Army and ArmySelectionOverlay components with new data
    structure

    client/src/ui/components/worldmap/armies/Army.tsx

  • Updated Army and ArmySelectionOverlay components to use new army data
    structure.
  • Improved handling of army selection and battle labels.
  • +20/-15 
    Tests
    1 files
    useBattlesUtils.test.tsx
    Add tests for useBattlesUtils functions                                   

    client/src/hooks/helpers/battles/test/useBattlesUtils.test.tsx

    • Added tests for useBattlesUtils functions.
    +161/-0 
    Additional files (token-limit)
    71 files
    __mock__.tsx
    ...                                                                                                           

    client/src/hooks/helpers/battles/test/mock.tsx

    ...

    +54/-0   
    Military.tsx
    ...                                                                                                           

    client/src/ui/modules/military/Military.tsx

    ...

    +2/-46   
    BattleProgressBar.tsx
    ...                                                                                                           

    client/src/ui/modules/military/battle-view/BattleProgressBar.tsx

    ...

    +7/-6     
    BattleView.tsx
    ...                                                                                                           

    client/src/ui/modules/military/battle-view/BattleView.tsx

    ...

    +11/-9   
    Battle.tsx
    ...                                                                                                           

    client/src/ui/modules/military/battle-view/Battle.tsx

    ...

    +8/-7     
    useHyperstructures.tsx
    ...                                                                                                           

    client/src/hooks/helpers/useHyperstructures.tsx

    ...

    +9/-11   
    TroopChip.tsx
    ...                                                                                                           

    client/src/ui/components/military/TroopChip.tsx

    ...

    +5/-5     
    useGuilds.tsx
    ...                                                                                                           

    client/src/hooks/helpers/useGuilds.tsx

    ...

    +6/-6     
    TopMiddleNavigation.tsx
    ...                                                                                                           

    client/src/ui/modules/navigation/TopMiddleNavigation.tsx

    ...

    +6/-6     
    useTrade.tsx
    ...                                                                                                           

    client/src/hooks/helpers/useTrade.tsx

    ...

    +9/-9     
    useStamina.tsx
    ...                                                                                                           

    client/src/hooks/helpers/useStamina.tsx

    ...

    +4/-6     
    Battles.tsx
    ...                                                                                                           

    client/src/ui/components/models/buildings/worldmap/Battles.tsx

    ...

    +4/-5     
    StructureCard.tsx
    ...                                                                                                           

    client/src/ui/components/hyperstructures/StructureCard.tsx

    ...

    +3/-11   
    GroundGrid.tsx
    ...                                                                                                           

    client/src/ui/components/construction/GroundGrid.tsx

    ...

    +9/-9     
    ShardsMines.tsx
    ...                                                                                                           

    client/src/ui/components/models/buildings/worldmap/ShardsMines.tsx

    ...

    +8/-5     
    useHexPosition.tsx
    ...                                                                                                           

    client/src/hooks/helpers/useHexPosition.tsx

    ...

    +4/-4     
    useTravel.tsx
    ...                                                                                                           

    client/src/hooks/helpers/useTravel.tsx

    ...

    +7/-3     
    ArmyViewCard.tsx
    ...                                                                                                           

    client/src/ui/components/military/ArmyViewCard.tsx

    ...

    +5/-3     
    MarketManager.ts
    ...                                                                                                           

    client/src/dojo/modelManager/MarketManager.ts

    ...

    +13/-5   
    RealmListItem.tsx
    ...                                                                                                           

    client/src/ui/components/worldmap/realms/RealmListItem.tsx

    ...

    +2/-11   
    InstancedCastles.tsx
    ...                                                                                                           

    client/src/ui/components/models/buildings/worldmap/InstancedCastles.tsx

    ...

    +6/-6     
    TradeHistoryEvent.tsx
    ...                                                                                                           

    client/src/ui/components/trading/TradeHistoryEvent.tsx

    ...

    +3/-3     
    Armies.tsx
    ...                                                                                                           

    client/src/ui/components/worldmap/armies/Armies.tsx

    ...

    +5/-13   
    LeftNavigationModule.tsx
    ...                                                                                                           

    client/src/ui/modules/navigation/LeftNavigationModule.tsx

    ...

    +4/-4     
    useResources.tsx
    ...                                                                                                           

    client/src/hooks/helpers/useResources.tsx

    ...

    +4/-2     
    HexceptionViewScene.tsx
    ...                                                                                                           

    client/src/ui/modules/scenes/HexceptionViewScene.tsx

    ...

    +8/-8     
    useBuildings.tsx
    ...                                                                                                           

    client/src/hooks/helpers/useBuildings.tsx

    ...

    +5/-5     
    ArmyHitBox.tsx
    ...                                                                                                           

    client/src/ui/components/worldmap/armies/ArmyHitBox.tsx

    ...

    +14/-2   
    useUISound.tsx
    ...                                                                                                           

    client/src/hooks/useUISound.tsx

    ...

    +0/-2     
    utils.tsx
    ...                                                                                                           

    client/src/ui/components/worldmap/armies/utils.tsx

    ...

    +6/-3     
    useRoads.tsx
    ...                                                                                                           

    client/src/hooks/helpers/useRoads.tsx

    ...

    +5/-5     
    useBanks.tsx
    ...                                                                                                           

    client/src/hooks/helpers/useBanks.tsx

    ...

    +3/-3     
    RightNavigationModule.tsx
    ...                                                                                                           

    client/src/ui/modules/navigation/RightNavigationModule.tsx

    ...

    +2/-2     
    WorldStructuresMenu.tsx
    ...                                                                                                           

    client/src/ui/modules/world-structures/WorldStructuresMenu.tsx

    ...

    +4/-4     
    BattleDetails.tsx
    ...                                                                                                           

    client/src/ui/modules/military/battle-view/BattleDetails.tsx

    ...

    +2/-2     
    StructureListItem.tsx
    ...                                                                                                           

    client/src/ui/components/worldmap/structures/StructureListItem.tsx

    ...

    +1/-1     
    useBlockchainStore.tsx
    ...                                                                                                           

    client/src/hooks/store/useBlockchainStore.tsx

    ...

    +2/-2     
    HexGrid.tsx
    ...                                                                                                           

    client/src/ui/components/models/biomes/HexGrid.tsx

    ...

    +2/-3     
    GrasslandBiome.tsx
    ...                                                                                                           

    client/src/ui/components/models/biomes/GrasslandBiome.tsx

    ...

    +2/-3     
    DeepOceanBiome.tsx
    ...                                                                                                           

    client/src/ui/components/models/biomes/DeepOceanBiome.tsx

    ...

    +2/-3     
    DesertBiome.tsx
    ...                                                                                                           

    client/src/ui/components/models/biomes/DesertBiome.tsx

    ...

    +2/-3     
    OceanBiome.tsx
    ...                                                                                                           

    client/src/ui/components/models/biomes/OceanBiome.tsx

    ...

    +2/-3     
    useRealm.tsx
    ...                                                                                                           

    client/src/hooks/helpers/useRealm.tsx

    ...

    +1/-1     
    useQuestStore.tsx
    ...                                                                                                           

    client/src/hooks/store/useQuestStore.tsx

    ...

    +1/-1     
    vitest.config.ts
    ...                                                                                                           

    client/vitest.config.ts

    ...

    +18/-0   
    DeciduousForestBiome.tsx
    ...                                                                                                           

    client/src/ui/components/models/biomes/DeciduousForestBiome.tsx

    ...

    +2/-3     
    SubtropicalDesertBiome.tsx
    ...                                                                                                           

    client/src/ui/components/models/biomes/SubtropicalDesertBiome.tsx

    ...

    +2/-3     
    TemperateDesertBiome.tsx
    ...                                                                                                           

    client/src/ui/components/models/biomes/TemperateDesertBiome.tsx

    ...

    +2/-3     
    TemperateRainforestBiome.tsx
    ...                                                                                                           

    client/src/ui/components/models/biomes/TemperateRainforestBiome.tsx

    ...

    +2/-3     
    TropicalSeasonalForestBiome.tsx
    ...                                                                                                           

    client/src/ui/components/models/biomes/TropicalSeasonalForestBiome.tsx

    ...

    +2/-3     
    _mapStore.tsx
    ...                                                                                                           

    client/src/hooks/store/_mapStore.tsx

    ...

    +2/-2     
    ScorchedBiome.tsx
    ...                                                                                                           

    client/src/ui/components/models/biomes/ScorchedBiome.tsx

    ...

    +2/-3     
    SnowBiome.tsx
    ...                                                                                                           

    client/src/ui/components/models/biomes/SnowBiome.tsx

    ...

    +2/-3     
    TropicalRainforestBiome.tsx
    ...                                                                                                           

    client/src/ui/components/models/biomes/TropicalRainforestBiome.tsx

    ...

    +2/-3     
    BeachBiome.tsx
    ...                                                                                                           

    client/src/ui/components/models/biomes/BeachBiome.tsx

    ...

    +2/-3     
    ShrublandBiome.tsx
    ...                                                                                                           

    client/src/ui/components/models/biomes/ShrublandBiome.tsx

    ...

    +2/-3     
    TaigaBiome.tsx
    ...                                                                                                           

    client/src/ui/components/models/biomes/TaigaBiome.tsx

    ...

    +2/-3     
    TundraBiome.tsx
    ...                                                                                                           

    client/src/ui/components/models/biomes/TundraBiome.tsx

    ...

    +2/-3     
    ExistingBuildings.tsx
    ...                                                                                                           

    client/src/ui/components/construction/ExistingBuildings.tsx

    ...

    +0/-1     
    BattleLabel.tsx
    ...                                                                                                           

    client/src/ui/components/worldmap/armies/BattleLabel.tsx

    ...

    +1/-1     
    useCaravans.tsx
    ...                                                                                                           

    client/src/hooks/helpers/useCaravans.tsx

    ...

    +1/-2     
    global.ts
    ...                                                                                                           

    sdk/packages/eternum/src/constants/global.ts

    ...

    +1/-1     
    ArmyPanel.tsx
    ...                                                                                                           

    client/src/ui/components/military/ArmyPanel.tsx

    ...

    +2/-1     
    BattlesArmyTable.tsx
    ...                                                                                                           

    client/src/ui/components/military/BattlesArmyTable.tsx

    ...

    +1/-1     
    DepositResources.tsx
    ...                                                                                                           

    client/src/ui/components/resources/DepositResources.tsx

    ...

    +1/-1     
    createClientComponents.ts
    ...                                                                                                           

    client/src/dojo/createClientComponents.ts

    ...

    +0/-1     
    pnpm-lock.yaml
    ...                                                                                                           

    pnpm-lock.yaml

    ...

    +569/-39
    tests.cairo
    ...                                                                                                           

    contracts/src/systems/combat/tests.cairo

    ...

    +103/-1 
    package.json
    ...                                                                                                           

    client/package.json

    ...

    +16/-6   
    test-contracts.yml
    ...                                                                                                           

    .github/workflows/test-contracts.yml

    ...

    +63/-1   
    test-client.yml
    ...                                                                                                           

    .github/workflows/test-client.yml

    ...

    +36/-0   

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link

    vercel bot commented Jul 17, 2024

    The latest updates on your projects. Learn more about Vercel for Git ↗︎

    Name Status Preview Comments Updated (UTC)
    eternum ❌ Failed (Inspect) Jul 25, 2024 5:26pm

    @edisontim edisontim marked this pull request as draft July 17, 2024 13:49
    Copy link

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Refactoring Complexity
    The refactoring in useArmies.tsx significantly increases the complexity of the code. Consider simplifying the logic or breaking down the functions further to improve maintainability.

    New Functionality
    New functions and types such as getBattle and BattleInfo have been added. Ensure that these changes are well-documented and covered by unit tests to maintain code quality and understandability.

    UI Component Changes
    Changes in ArmyManagementCard.tsx involve significant modifications to the UI logic and state management. It's crucial to ensure that these changes do not introduce regressions or affect the user experience negatively.

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Prevent division by zero by validating precision values before division

    Avoid potential division by zero errors by checking if
    EternumGlobalConfig.resources.resourcePrecision and
    EternumGlobalConfig.troop.healthPrecision are not zero before performing the
    division.

    client/src/hooks/helpers/useArmies.tsx [81-85]

    -BigInt(EternumGlobalConfig.resources.resourcePrecision) * EternumGlobalConfig.troop.healthPrecision
    +const precisionProduct = BigInt(EternumGlobalConfig.resources.resourcePrecision) * EternumGlobalConfig.troop.healthPrecision;
    +if (precisionProduct === 0n) throw new Error("Precision values should not be zero.");
    +healthClone.current = BigInt(healthClone.current) / precisionProduct;
    +healthClone.lifetime = BigInt(healthClone.lifetime) / precisionProduct;
     
    Suggestion importance[1-10]: 10

    Why: This suggestion prevents a critical error (division by zero) by validating precision values, which is essential for ensuring the correctness and stability of the code.

    10
    Add a check to prevent division by zero in troop count calculations

    Ensure that the getUpdatedArmy function handles the case where
    battle_army_lifetime.troops.knight_count is zero to avoid division by zero errors.

    client/src/dojo/modelManager/BattleManager.ts [92-102]

     cloneArmy.troops.knight_count =
    -  cloneArmy.troops.knight_count === 0n
    +  cloneArmy.troops.knight_count === 0n || battle_army_lifetime.troops.knight_count === 0
         ? 0n
         : BigInt(
             Math.floor(
               Number(
                 (cloneArmy.troops.knight_count * battle_army.troops.knight_count) /
                   battle_army_lifetime.troops.knight_count,
               ),
             ),
           );
     
    Suggestion importance[1-10]: 9

    Why: This suggestion prevents a possible division by zero error, which is a significant bug that could cause runtime exceptions.

    9
    Add a check for undefined army object to prevent runtime errors

    Consider checking for undefined values in the army object before accessing its
    properties in the checkIfArmyLostAFinishedBattle function. This will prevent
    potential runtime errors if an undefined army object is passed to the function.

    client/src/hooks/helpers/useArmies.tsx [618]

    -const battle = getBattle(getEntityIdFromKeys([BigInt(army?.battle_id || 0n)]), Battle);
    +if (!army) return false;
    +const battle = getBattle(getEntityIdFromKeys([BigInt(army.battle_id || 0n)]), Battle);
     
    Suggestion importance[1-10]: 9

    Why: This suggestion addresses a potential runtime error by adding a check for an undefined army object, which is a crucial improvement for code robustness.

    9
    Add null checks to prevent runtime errors when accessing potentially null objects

    Consider adding null checks or optional chaining when accessing properties of
    objects that might potentially be null or undefined. This is particularly important
    for the realm and structure objects in the playerStructures function, as they are
    being accessed directly which could lead to runtime errors if they are null.

    client/src/hooks/helpers/useEntities.tsx [48-50]

     const realm = getComponentValue(Realm, id);
     const structure = getComponentValue(Structure, id);
     const position = getComponentValue(Position, id);
    +if (!realm || !structure || !position) return null;
     
    Suggestion importance[1-10]: 9

    Why: Adding null checks is crucial to prevent potential runtime errors when accessing properties of objects that might be null or undefined. This suggestion addresses a possible bug and improves the robustness of the code.

    9
    Improve the safety of BigInt to number conversion to prevent potential data loss

    Replace the direct usage of Number for converting BigInt to number with a safer
    method that checks for potential overflow or precision issues. This is crucial
    because using Number directly can lead to precision loss for very large BigInt
    values.

    client/src/dojo/modelManager/BattleManager.ts [39]

    -const duractionSinceLastUpdate = currentTimestamp - Number(battle.last_updated);
    +const duractionSinceLastUpdate = currentTimestamp - safeBigIntToNumber(battle.last_updated);
     
    Suggestion importance[1-10]: 8

    Why: This suggestion addresses a potential bug by ensuring safer conversion from BigInt to number, which is crucial for preventing precision loss in large values.

    8
    Add null checks to function inputs to prevent runtime errors

    Ensure that the protectorStillInBattle function handles cases where the input
    parameters are null or undefined to prevent runtime errors.

    client/src/hooks/helpers/battles/test/useBattlesUtils.test.tsx [21]

    -const isInBattle = testedModule.protectorStillInBattle({} as any, {} as any, {} as any);
    +const isInBattle = testedModule.protectorStillInBattle({} as any ?? {}, {} as any ?? {}, {} as any ?? {});
     
    Suggestion importance[1-10]: 5

    Why: Adding null checks can prevent potential runtime errors, but the suggested implementation using the nullish coalescing operator is not the most appropriate for this context.

    5
    Enhancement
    Add error handling for undefined return values in getBattle to enhance robustness

    Consider adding error handling or a fallback mechanism in getBattle method when
    getComponentValue returns undefined, to ensure the method's robustness.

    client/src/dojo/modelManager/BattleManager.ts [70]

    -return getComponentValue(this.battleModel, getEntityIdFromKeys([this.battleId]));
    +const battleComponent = getComponentValue(this.battleModel, getEntityIdFromKeys([this.battleId]));
    +if (!battleComponent) {
    +  // Handle the undefined case or throw an error
    +  throw new Error("Battle component not found for the given battle ID.");
    +}
    +return battleComponent;
     
    Suggestion importance[1-10]: 8

    Why: Adding error handling increases the robustness of the method, preventing potential issues when getComponentValue returns undefined.

    8
    Ensure consistent army positioning by replacing random offset calculation with a deterministic approach

    Replace the usage of Math.random() for offset calculation with a deterministic
    function based on armyEntityId to ensure consistent positioning across different
    renders or sessions.

    client/src/hooks/helpers/useArmies.tsx [107]

    -const offsetToAvoidOverlapping = Math.random() * 1 - 0.5;
    +const offsetToAvoidOverlapping = (Number(armyEntityId) % 10) / 20 - 0.5;
     
    Suggestion importance[1-10]: 8

    Why: Replacing Math.random() with a deterministic function ensures consistent positioning, which is important for predictable behavior across different renders or sessions.

    8
    Use destructuring to enhance code readability and maintainability

    To improve code readability and maintainability, consider using destructuring for
    the account object in the useEntitiesUtils function. This approach can make the code
    cleaner and easier to understand.

    client/src/hooks/helpers/useEntities.tsx [68]

    -account: { account },
    +const { account } = account;
     
    Suggestion importance[1-10]: 5

    Why: While using destructuring can enhance code readability, the existing code is already clear. This suggestion is a minor enhancement and does not address any critical issues.

    5
    Best practice
    Improve type safety by using a more robust type checking mechanism

    Replace the direct type assertion with a more robust type checking mechanism to
    ensure the correct type is being passed to the Set constructor. This will prevent
    potential runtime errors if the type of the entity is not as expected.

    client/src/hooks/helpers/battles/test/useBattlesUtils.test.tsx [18]

    -const protectors = new Set(["0x0" as Entity]);
    +const protectors = new Set<Entity>(["0x0"].map(e => e as Entity));
     
    Suggestion importance[1-10]: 8

    Why: This suggestion improves type safety by ensuring that the elements being added to the Set are of the correct type, which can prevent potential runtime errors.

    8
    Add error handling for undefined entityName to enhance function robustness

    It's a good practice to handle cases where entityName might be undefined in the
    getEntityName function. This can prevent potential runtime errors and make the
    function more robust.

    client/src/hooks/helpers/useEntities.tsx [119-120]

     const entityName = getComponentValue(EntityName, getEntityIdFromKeys([entityId]));
    -return entityName ? shortString.decodeShortString(entityName.name.toString()) : entityId.toString();
    +if (!entityName) return `Entity ID: ${entityId}`;
    +return shortString.decodeShortString(entityName.name.toString());
     
    Suggestion importance[1-10]: 8

    Why: Handling cases where entityName might be undefined is important for preventing runtime errors and improving the robustness of the function. This suggestion addresses a best practice.

    8
    Enhance type safety by using a more specific type guard in the filter method

    Use a more specific type guard in the .filter method to ensure only valid ArmyInfo
    objects are processed, enhancing type safety and clarity.

    client/src/hooks/helpers/useArmies.tsx [133]

    -.filter((army): army is ArmyInfo => army !== undefined);
    +.filter((army): army is ArmyInfo => army !== undefined && army.hasOwnProperty('name') && army.hasOwnProperty('health'));
     
    Suggestion importance[1-10]: 7

    Why: Using a more specific type guard improves type safety and clarity, which is beneficial for maintainability and reducing potential bugs.

    7
    Improve type safety by using explicit type casting or type guards

    Use explicit type casting or type guards instead of using as any to ensure type
    safety and maintainability of the code.

    client/src/hooks/helpers/battles/test/useBattlesUtils.test.tsx [21]

    -const isInBattle = testedModule.protectorStillInBattle({} as any, {} as any, {} as any);
    +const isInBattle = testedModule.protectorStillInBattle({} as SpecificType, {} as SpecificType, {} as SpecificType);
     
    Suggestion importance[1-10]: 6

    Why: Using explicit type casting or type guards enhances type safety and maintainability, but the suggestion lacks specificity regarding the exact types to be used.

    6
    Maintainability
    Refactor the method to calculate total troop health to be more scalable and maintainable

    Refactor getTroopFullHealth to use a loop or a more scalable method to handle an
    arbitrary number of troop types, which will improve maintainability and scalability
    of the code.

    client/src/dojo/modelManager/BattleManager.ts [131-140]

    -let total_knight_health = health * Number(troops.knight_count);
    -let total_paladin_health = health * Number(troops.paladin_count);
    -let total_crossbowman_health = health * Number(troops.crossbowman_count);
    +let totalHealth = 0;
    +for (const troopType in troops) {
    +  totalHealth += health * Number(troops[troopType]);
    +}
     return BigInt(
       Math.floor(
    -    (total_knight_health + total_paladin_health + total_crossbowman_health) /
    +    totalHealth /
           (EternumGlobalConfig.resources.resourceMultiplier * Number(EternumGlobalConfig.troop.healthPrecision)),
       ),
     );
     
    Suggestion importance[1-10]: 7

    Why: This refactor improves code maintainability and scalability, which is beneficial for future extensions but not immediately critical.

    7
    Reduce redundancy and improve maintainability by refactoring repeated mocking

    Refactor the repeated mocking of runQuery to a utility function or before each test
    setup to reduce redundancy and improve test maintainability.

    client/src/hooks/helpers/battles/test/useBattlesUtils.test.tsx [19]

    -vi.mocked(runQuery).mockReturnValue(protectors);
    +beforeEach(() => {
    +  vi.mocked(runQuery).mockReturnValue(protectors);
    +});
     
    Suggestion importance[1-10]: 7

    Why: Refactoring the repeated mocking to a utility function or setup step reduces redundancy and improves the maintainability of the tests, making the code cleaner and easier to manage.

    7
    Possible issue
    Validate address before usage to ensure data integrity and prevent bugs

    To avoid potential bugs and ensure the data integrity, consider validating the
    address before using it to fetch addressName in the getAddressNameFromEntity
    function.

    client/src/hooks/helpers/useEntities.tsx [124-127]

     const address = getPlayerAddressFromEntity(entityId);
    -if (!address) return;
    -const addressName = getComponentValue(AddressName, getEntityIdFromKeys([BigInt(address)]));
    +if (!address || typeof address !== 'bigint') return;
    +const addressName = getComponentValue(AddressName, getEntityIdFromKeys([address]));
     
    Suggestion importance[1-10]: 7

    Why: Validating the address before usage is a good practice to ensure data integrity and prevent potential bugs. This suggestion addresses a possible issue but is not critical.

    7

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant