BibliothecaDAO / eternum

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

Optimistic stamina #955

Closed aymericdelab closed 3 months ago

aymericdelab commented 3 months ago

PR Type

Enhancement


Description


Changes walkthrough ๐Ÿ“

Relevant files
Enhancement
useExplore.tsx
Integrate optimistic stamina update in exploration logic.

client/src/hooks/helpers/useExplore.tsx
  • Imported EternumGlobalConfig and useStamina.
  • Added optimisticStaminaUpdate call in exploreHex.
  • Removed Position and Stamina overrides on error.
  • +12/-4   
    useStamina.tsx
    Add optimistic stamina update function.                                   

    client/src/hooks/helpers/useStamina.tsx
  • Refactored getStamina function to use currentArmiesTick.
  • Added optimisticStaminaUpdate function.
  • +21/-9   
    useTravel.tsx
    Integrate optimistic stamina update in travel logic.         

    client/src/hooks/helpers/useTravel.tsx
  • Imported EternumGlobalConfig and useStamina.
  • Added optimisticStaminaUpdate call in travelToHex.
  • Removed Position and Stamina overrides on error.
  • +5/-3     

    ๐Ÿ’ก 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 19, 2024 6:48am
    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:
    The optimisticStaminaUpdate function in both useExplore.tsx and useTravel.tsx is called to update stamina optimistically. However, it's crucial to ensure that the stamina does not drop below zero or any predefined minimum value. This should be checked and handled to prevent negative stamina values.
    Error Handling:
    In the error handling section where overrides are removed, ensure that the catch blocks are adequately capturing and logging the specific errors for better debugging and maintenance.
    github-actions[bot] commented 3 months ago

    PR Code Suggestions โœจ

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Prevent stamina from going negative by ensuring it does not drop below zero ___ **Ensure that the stamina amount does not go below zero when subtracting the cost in the
    optimisticStaminaUpdate function.** [client/src/hooks/helpers/useExplore.tsx [73]](https://github.com/BibliothecaDAO/eternum/pull/955/files#diff-45821874b656c15cf5d9d6a603afd6d4858df5008f6353317b5a2d75482aa9a9R73-R73) ```diff -amount: stamina.amount - cost, +amount: Math.max(0, stamina.amount - cost), ```
    Suggestion importance[1-10]: 10 Why: This suggestion addresses a potential bug where stamina could become negative, which is a critical issue. Ensuring stamina does not drop below zero is essential for maintaining game logic integrity.
    10
    Enhancement
    Add error handling around stamina updates to catch and log failures ___ **Add error handling for the optimisticStaminaUpdate function call to manage cases where the
    stamina update might fail.** [client/src/hooks/helpers/useExplore.tsx [126]](https://github.com/BibliothecaDAO/eternum/pull/955/files#diff-45821874b656c15cf5d9d6a603afd6d4858df5008f6353317b5a2d75482aa9a9R126-R126) ```diff -optimisticStaminaUpdate(overrideId, entityId, EternumGlobalConfig.stamina.exploreCost); +try { + optimisticStaminaUpdate(overrideId, entityId, EternumGlobalConfig.stamina.exploreCost); +} catch (error) { + console.error("Failed to update stamina optimistically", error); +} ```
    Suggestion importance[1-10]: 8 Why: Adding error handling improves the robustness of the code by catching and logging potential failures, which is important for debugging and maintaining application stability.
    8
    Maintainability
    Use optional chaining and nullish coalescing to simplify accessing values ___ **Refactor the getStamina function to use optional chaining and nullish coalescing to
    simplify the code and improve readability.** [client/src/hooks/helpers/useStamina.tsx [40]](https://github.com/BibliothecaDAO/eternum/pull/955/files#diff-1462ae4521465368a66e185d64e149c741cf0439b84171e3946f3fa1046b8764R40-R40) ```diff -const armyEntity = getComponentValue(Army, armiesEntityIds.values().next().value); +const armyEntity = getComponentValue(Army, armiesEntityIds.values().next()?.value); ```
    Suggestion importance[1-10]: 7 Why: This suggestion improves code readability and maintainability by using modern JavaScript features. However, it does not address a critical issue.
    7
    Possible issue
    Ensure explorerId is always a valid bigint and not undefined ___ **Consider validating the explorerId in the ExploreHexProps interface to ensure it is not
    undefined before use in functions that require a valid ID.** [client/src/hooks/helpers/useExplore.tsx [21]](https://github.com/BibliothecaDAO/eternum/pull/955/files#diff-45821874b656c15cf5d9d6a603afd6d4858df5008f6353317b5a2d75482aa9a9R21-R21) ```diff -explorerId: bigint | undefined; +explorerId: bigint; ```
    Suggestion importance[1-10]: 6 Why: Ensuring `explorerId` is always valid can prevent runtime errors, but it may require additional changes to the codebase to handle cases where `explorerId` can be `undefined`. This is a good practice but not immediately critical.
    6