search

BibliothecaDAO / eternum

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

fix: battle component changing with getComponentValue #1035

Closed edisontim closed 5 days ago

edisontim commented 5 days ago

PR Type

Bug fix, Enhancement

Description

Changes walkthrough 📝

Relevant files
Bug fix
contractComponents.ts
Change `battle_side` type from `String` to `Number`           
client/src/dojo/contractComponents.ts
  • Changed battle_side type from String to Number in multiple components.
  • Updated custom types to reflect changes in battle_side.
    		•  +8/-6     
    TroopSelect.tsx
    Remove console.log from button click handlers                       
    client/src/ui/components/military/TroopSelect.tsx - Removed `console.log` statements from button click handlers.
    		+2/-2     
    Onboarding.tsx
    Remove unnecessary console.log statement                                 
    client/src/ui/layouts/Onboarding.tsx - Removed unnecessary `console.log` statement.
    		+0/-2     
    Steps.tsx
    Remove unnecessary console.log statement                                 
    client/src/ui/modules/onboarding/Steps.tsx - Removed unnecessary `console.log` statement.
    		+0/-2     
    Enhancement
    BattleManager.ts
    Clone battle object and update health calculations             
    client/src/dojo/modelManager/BattleManager.ts
  • Cloned battle object to battleClone for immutability.
  • Updated health and troops calculations to use battleClone.
  • Made getBattle method public.
    		•  +20/-12 
    LockedResources.tsx
    Adjust layout and styling for battle chest                             
    client/src/ui/modules/military/battle-view/LockedResources.tsx - Adjusted layout and styling for battle chest display.
    		+3/-3     
    Troops.tsx
    Simplify TroopRow and adjust TroopCard styling                     
    client/src/ui/modules/military/battle-view/Troops.tsx
  • Simplified TroopRow component by removing commented-out code.
  • Adjusted TroopCard component height and image size.
    		•  +21/-31 

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

    vercel[bot] commented 5 days 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 28, 2024 8:14am
    github-actions[bot] commented 5 days ago

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review [1-5] 3
    🧪 Relevant tests No
    🔒 Security concerns No
    ⚡ Key issues to review Type Consistency:
    The change from RecsType.String to RecsType.Number for battle_side in contractComponents.ts is significant. Ensure that all related components and data handling logic are updated to handle battle_side as a number.
    Immutability:
    The use of structuredClone in BattleManager.ts to clone battle into battleClone is a good practice for immutability. Reviewers should ensure that all operations thereafter are using battleClone and not mutating the original battle object.
    Data Type Conversion:
    In BattleManager.ts, ensure that the conversion to BigInt during health calculations is handled correctly across all platforms and doesn't lead to precision issues or bugs.
    github-actions[bot] commented 5 days ago

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Add onClick handlers to the buttons to ensure they are functional ___ **Restore the onClick handlers for the buttons to ensure that they perform actions when
    clicked. Removing these handlers might have been unintentional and could lead to
    non-functional UI elements.** [client/src/ui/components/military/TroopSelect.tsx [131-134]](https://github.com/BibliothecaDAO/eternum/pull/1035/files#diff-d48cb754711d156d71eb306469bed2491bb0e2249e298521427cbecba37d9a77R131-R134) ```diff - - + + ```
    Suggestion importance[1-10]: 9 Why: Removing the onClick handlers makes the buttons non-functional, which is a significant issue. This suggestion addresses a major bug by restoring functionality.
    		9
    Possible issue
    Revert the type of battle_side to ensure compatibility with the rest of the codebase ___ **Consider reverting the type of battle_side from RecsType.Number back to RecsType.String or
    ensure that the consuming code correctly handles the type change. Changing the type might
    lead to unexpected behavior if the rest of the codebase expects a string.** [client/src/dojo/contractComponents.ts [218]](https://github.com/BibliothecaDAO/eternum/pull/1035/files#diff-5bab736e704fb6f184e27c05d5a774e9e90c45f019bf5733c9e6ea9efb249101R218-R218) ```diff -battle_side: RecsType.Number, +battle_side: RecsType.String, ```
    Suggestion importance[1-10]: 8 Why: Changing the type of `battle_side` from `RecsType.String` to `RecsType.Number` could lead to compatibility issues if the rest of the codebase expects a string. This suggestion addresses a potential major bug.
    		8
    Best practice
    Keep the getBattle method private to maintain encapsulation ___ **Refactor the method getBattle to remain private if it does not need to be accessed outside
    of BattleManager. Changing the access modifier without necessity can lead to unintended
    usage and makes the API less clean.** [client/src/dojo/modelManager/BattleManager.ts [82]](https://github.com/BibliothecaDAO/eternum/pull/1035/files#diff-bdcffae87f6ec69e4c6a7ad71f18a884bf300084e529d7a9cbc8fc55642dc5acR82-R82) ```diff -public getBattle() { +private getBattle() { ```
    Suggestion importance[1-10]: 7 Why: Making `getBattle` public without necessity can lead to unintended usage and breaks encapsulation. This suggestion improves code maintainability and follows best practices.
    		7
    Enhancement
    Restore previous styling for visual consistency in the UI ___ **Consider maintaining the previous styling for the Battle Chest div to ensure consistent UI
    experience. The changes in class names and structure might affect the visual consistency
    and layout.** [client/src/ui/modules/military/battle-view/LockedResources.tsx [18-19]](https://github.com/BibliothecaDAO/eternum/pull/1035/files#diff-359d4a5ec2ea695cafb43989d9412232fc58a85aae799cdc84e83e4b128d9f42R18-R19) ```diff -
    -
    Battle Chest
    +
    +
    Battle Chest
    ```
    Suggestion importance[1-10]: 6 Why: The suggested change ensures visual consistency in the UI, which is important for user experience but is a minor issue compared to functionality bugs.
    		6