BibliothecaDAO / eternum

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

Fix tick #957

Closed aymericdelab closed 3 weeks ago

aymericdelab commented 3 weeks ago

User description


PR Type

Bug fix, Enhancement


Description


Changes walkthrough πŸ“

Relevant files
Enhancement
useExplore.tsx
Include `currentArmiesTick` in exploration functions.       

client/src/hooks/helpers/useExplore.tsx
  • Added currentArmiesTick to ExploreHexProps interface.
  • Updated optimisticExplore and exploreHex functions to include
    currentArmiesTick parameter.
  • +5/-4     
    useStamina.tsx
    Add `currentArmiesTick` to stamina functions.                       

    client/src/hooks/helpers/useStamina.tsx
  • Added currentArmiesTick parameter to getStamina and
    optimisticStaminaUpdate functions.
  • +9/-3     
    useTravel.tsx
    Include `currentArmiesTick` in travel functions.                 

    client/src/hooks/helpers/useTravel.tsx
  • Added currentArmiesTick to TravelToHexProps interface.
  • Updated optimisticTravelHex and travelToHex functions to include
    currentArmiesTick parameter.
  • +22/-4   
    useEventHandlers.tsx
    Pass `currentArmiesTick` to event handlers.                           

    client/src/ui/components/worldmap/hexagon/useEventHandlers.tsx
  • Added currentArmiesTick from useBlockchainStore.
  • Updated handleTravelClick and handleExploreClick functions to include
    currentArmiesTick parameter.
  • +27/-6   
    LeftNavigationModule.tsx
    Include `currentArmiesTick` in navigation module.               

    client/src/ui/modules/navigation/LeftNavigationModule.tsx
  • Added currentArmiesTick from useBlockchainStore.
  • Updated stamina check in armiesWithStaminaLeft to include
    currentArmiesTick.
  • +4/-1     

    πŸ’‘ 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 weeks 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 2:16pm
    github-actions[bot] commented 3 weeks ago

    PR Reviewer Guide πŸ”

    ⏱️ Estimated effort to review [1-5] 3
    πŸ§ͺ Relevant tests No
    πŸ”’ Security concerns No
    ⚑ Key issues to review Possible Bug:
    The currentArmiesTick parameter is added to various functions and methods, but it's unclear if all necessary parts of the codebase that interact with these functions have been updated to pass this parameter. This could lead to runtime errors if the parameter is not provided.
    Code Consistency:
    The addition of currentArmiesTick across multiple files suggests a significant change in how state is managed regarding ticks. It would be beneficial to ensure that there is a consistent and error-free implementation across all affected functions.
    github-actions[bot] commented 3 weeks ago

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Handle cases where no valid entity IDs are returned in getStamina ___ **Ensure that the getStamina function handles cases where runQuery might not return any
    valid entity IDs, to avoid potential runtime errors when accessing .next().value.** [client/src/hooks/helpers/useStamina.tsx [35-43]](https://github.com/BibliothecaDAO/eternum/pull/957/files#diff-1462ae4521465368a66e185d64e149c741cf0439b84171e3946f3fa1046b8764R35-R43) ```diff const getStamina = ({ travelingEntityId, currentArmiesTick, }: { travelingEntityId: bigint; currentArmiesTick: number; }) => { const staminasEntityIds = runQuery([HasValue(Stamina, { entity_id: travelingEntityId })]); + if (!staminasEntityIds.values().next().value) { + console.error("No stamina entities found for ID:", travelingEntityId); + return null; + } let staminaEntity = getComponentValue(Stamina, staminasEntityIds.values().next().value); ```
    Suggestion importance[1-10]: 9 Why: Ensuring that `getStamina` handles cases where `runQuery` might not return any valid entity IDs is crucial to avoid potential runtime errors. This suggestion addresses a possible bug and improves the reliability of the function.
    9
    Possible issue
    Add error handling to the optimisticExplore function ___ **Consider adding error handling for the optimisticExplore function to manage exceptions
    that might occur during the execution of getEntityIdFromKeys or optimisticStaminaUpdate.** [client/src/hooks/helpers/useExplore.tsx [122-129]](https://github.com/BibliothecaDAO/eternum/pull/957/files#diff-45821874b656c15cf5d9d6a603afd6d4858df5008f6353317b5a2d75482aa9a9R122-R129) ```diff const optimisticExplore = (entityId: bigint, col: number, row: number, currentArmiesTick: number) => { let overrideId = uuid(); - const entity = getEntityIdFromKeys([entityId]); - optimisticStaminaUpdate(overrideId, entityId, EternumGlobalConfig.stamina.exploreCost, currentArmiesTick); - Position.addOverride(overrideId, { - entity, -... + try { + const entity = getEntityIdFromKeys([entityId]); + optimisticStaminaUpdate(overrideId, entityId, EternumGlobalConfig.stamina.exploreCost, currentArmiesTick); + Position.addOverride(overrideId, { + entity, + ... + } catch (error) { + console.error("Failed to execute optimisticExplore:", error); + } +} ```
    Suggestion importance[1-10]: 8 Why: Adding error handling to the `optimisticExplore` function is a good practice to manage potential exceptions and improve robustness. This suggestion addresses a possible issue that could cause runtime errors.
    8
    Include currentArmiesTickRef in the useEffect dependencies ___ **Update the useEffect dependencies to include currentArmiesTickRef to ensure the component
    correctly reacts to changes in this value.** [client/src/ui/components/worldmap/hexagon/useEventHandlers.tsx [35-41]](https://github.com/BibliothecaDAO/eternum/pull/957/files#diff-774607aebdf016d6f7b9e964ad9f0426597cd4be1a10642ee27489cc29b404a4R35-R41) ```diff useEffect(() => { selectedEntityRef.current = selectedEntity; clickedHexRef.current = clickedHex; travelPathsRef.current = travelPaths; hoveredHexRef.current = hoveredHex; currentArmiesTickRef.current = currentArmiesTick; -}, [hoveredHex, travelPaths, selectedEntity, explored, clickedHex, currentArmiesTick]); +}, [hoveredHex, travelPaths, selectedEntity, explored, clickedHex, currentArmiesTick, currentArmiesTickRef]); ```
    Suggestion importance[1-10]: 7 Why: Including `currentArmiesTickRef` in the `useEffect` dependencies ensures that the component correctly reacts to changes in this value. This suggestion addresses a possible issue and improves the component's reactivity.
    7
    Maintainability
    Refactor stamina update logic in optimisticTravelHex into a separate function ___ **Refactor the optimisticTravelHex function to separate concerns, specifically extracting
    the stamina update logic into a separate function for better code readability and
    maintenance.** [client/src/hooks/helpers/useTravel.tsx [33-50]](https://github.com/BibliothecaDAO/eternum/pull/957/files#diff-a3ae3bb7c20e0f95e121cc173b192dadd3a166fd8c2dc84cd37c28c8ed59126aR33-R50) ```diff +const updateStaminaForTravel = (entityId: bigint, pathLength: number, currentArmiesTick: number) => { + let overrideId = uuid(); + optimisticStaminaUpdate( + overrideId, + entityId, + EternumGlobalConfig.stamina.travelCost * pathLength, + currentArmiesTick, + ); + return overrideId; +} + const optimisticTravelHex = ( entityId: bigint, col: number, row: number, pathLength: number, currentArmiesTick: number, ) => { - let overrideId = uuid(); + let overrideId = updateStaminaForTravel(entityId, pathLength, currentArmiesTick); const entity = getEntityIdFromKeys([entityId]); - optimisticStaminaUpdate( - overrideId, - entityId, - EternumGlobalConfig.stamina.travelCost * pathLength, - currentArmiesTick, - ); components.Position.addOverride(overrideId, { entity, ```
    Suggestion importance[1-10]: 6 Why: Refactoring the stamina update logic into a separate function improves code readability and maintainability. However, it does not address a critical issue, so it receives a moderate score.
    6