search

BibliothecaDAO / eternum

onchain eternal game
https://alpha-eternum.realms.world
MIT License
46 stars 34 forks source link

Feat/battle model #925

Closed aymericdelab closed 3 months ago

aymericdelab commented 3 months ago

PR Type

Enhancement, Tests

Description

Changes walkthrough 📝

Relevant files
Enhancement
useBattles.tsx
Include position data in battle entities.                               
client/src/hooks/helpers/useBattles.tsx
  • Added position data to battle entities.
  • Modified return structure to include both army and position data.
    		•  +2/-1     
    _mapStore.tsx
    Add state management for selected battles.                             
    client/src/hooks/store/_mapStore.tsx
  • Added state management for selected battles.
  • Updated clearSelection to reset selectedBattle.
    		•  +5/-0     
    Battles.tsx
    Create Battles component to display battle models.             
    client/src/ui/components/models/buildings/worldmap/Battles.tsx
  • Created Battles component to display battle models.
  • Implemented right-click functionality to select battles.
  • Added useEffect to log selected battles.
    		•  +59/-0   
    Armies.tsx
    Filter out armies in battle.                                                         
    client/src/ui/components/worldmap/armies/Armies.tsx - Filtered out armies that are currently in a battle.
    		+9/-1     
    BattleLabel.tsx
    Create BattleLabel component for battle details.                 
    client/src/ui/components/worldmap/armies/BattleLabel.tsx
  • Created BattleLabel component with a button to view battle details.
    		•  +32/-0   
    WorldHexagon.tsx
    Integrate Battles component into WorldMap.                             
    client/src/ui/components/worldmap/hexagon/WorldHexagon.tsx - Added Battles component to WorldMap.
    		+2/-0     

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

    vercel[bot] commented 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 1:04pm
    github-actions[bot] commented 3 months ago

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review [1-5] 3
    🧪 Relevant tests No
    🔒 Security concerns No
    ⚡ Key issues to review Possible Bug:
    In `Battles.tsx`, the `onClick` function in `BattleModel` is not being called correctly. It should be `onClick()` instead of just `onClick`.
    Data Integrity:
    In `Armies.tsx`, the filtering logic for armies not in battle checks if `army.battle_id === 0n`. Ensure that this condition correctly identifies armies not in battle based on the system's data structure and logic.
    Code Clarity:
    In `BattleLabel.tsx`, the `onClick` function is commented as "todo: implement this" and the actual function call in the `Button` component is incorrect (`onClick;` should be `onClick()`).
    github-actions[bot] commented 3 months ago

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Add a check to ensure position is not undefined before merging it with army ___ **Ensure that the position object is not undefined before destructuring it into the return
    object. This avoids potential runtime errors if getComponentValue(Position, entityId) does
    not return a value.** [client/src/hooks/helpers/useBattles.tsx [54]](https://github.com/BibliothecaDAO/eternum/pull/925/files#diff-ff52dac853da522ebc45bc0dd798b19076f4f3dfa27236ee94becd0c6eaee455R54-R54) ```diff -return { ...army, ...position }; +if (position) { + return { ...army, ...position }; +} else { + return army; +} ```
    Suggestion importance[1-10]: 9 Why: This suggestion addresses a potential runtime error by ensuring that `position` is not `undefined` before merging it with `army`. This is a crucial check to prevent unexpected behavior and improve code robustness.
    		9
    Best practice
    Replace any type with bigint for battle_id to enhance type safety ___ **Replace the any type for battle_id with a more specific type, such as bigint, to improve
    type safety and align with the usage of BigInt for IDs in other parts of the code.** [client/src/ui/components/models/buildings/worldmap/Battles.tsx [15]](https://github.com/BibliothecaDAO/eternum/pull/925/files#diff-5f342f04015b76da66b7e97f1c3491940764ce97259e444fa972a6db2667517cR15-R15) ```diff -const onRightClick = useCallback((battle_id: any, position: Position) => { +const onRightClick = useCallback((battle_id: bigint, position: Position) => { ```
    Suggestion importance[1-10]: 8 Why: Replacing `any` with `bigint` for `battle_id` enhances type safety and consistency across the codebase. This is a significant improvement for maintainability and reducing potential type-related errors.
    		8
    Specify a more detailed type for armies to improve type safety ___ **Use a more specific type for armies parameter in filterArmiesNotInBattle function to
    enhance type safety and clarify the expected structure of army objects.** [client/src/ui/components/worldmap/armies/Armies.tsx [15]](https://github.com/BibliothecaDAO/eternum/pull/925/files#diff-f5a037987ff3530c09bf19e74f6ea19acde71f78131f5c28765856654d77479aR15-R15) ```diff -const filterArmiesNotInBattle = (armies: any) => { +const filterArmiesNotInBattle = (armies: Array<{ battle_id: bigint; entity_id: bigint; }>) => { ```
    Suggestion importance[1-10]: 8 Why: Specifying a more detailed type for `armies` improves type safety and clarifies the expected structure of the objects, which is beneficial for code readability and maintainability.
    		8
    Initialize selectedBattle with a default value to ensure type consistency ___ **Initialize selectedBattle with a default value that matches its type to ensure consistency
    and avoid potential issues with undefined checks elsewhere in the code.** [client/src/hooks/store/_mapStore.tsx [65]](https://github.com/BibliothecaDAO/eternum/pull/925/files#diff-6d3df53ed6fbfb737106e9d7b48fa7c3fa33d11521804ea10a7f5d2fad2517beR65-R65) ```diff -selectedBattle: undefined; +selectedBattle: { id: BigInt(0), position: { x: 0, y: 0 } } | undefined; ```
    Suggestion importance[1-10]: 7 Why: Initializing `selectedBattle` with a default value ensures type consistency and can prevent potential issues with `undefined` checks. However, the improvement is more about best practices and does not address a critical issue.
    		7