search

BibliothecaDAO / eternum

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

Changed 'waiting to offload' only when at base #946

Closed cwastche closed 3 months ago

cwastche commented 3 months ago

User description

Closes #903

PR Type

Bug fix, Enhancement

Description

Changes walkthrough 📝

Relevant files
Enhancement
useResources.tsx
Refactor and enhance entity ownership retrieval functions.
client/src/hooks/helpers/useResources.tsx
  • Refactored useGetOwnedEntityOnPosition to useOwnedEntitiesOnPosition.
  • Added new helper functions getOwnedEntitiesOnPosition and
    getOwnedEntityOnPosition.
  • Updated the return object to include the new helper functions.
    		•  +32/-18 
    Bug fix
    Entity.tsx
    Update entity state display and resource deposit logic.   
    client/src/ui/components/entities/Entity.tsx
  • Integrated getOwnedEntityOnPosition to determine deposit entity ID.
  • Modified entity state display logic for Idle and WaitingToOffload.
  • Conditional rendering for DepositResources component based on entity
    state.
    		•  +15/-9   
    DepositResources.tsx
    Use new entity ownership retrieval in resource deposit.   
    client/src/ui/components/resources/DepositResources.tsx
  • Replaced useGetOwnedEntityOnPosition with useOwnedEntitiesOnPosition.
  • Utilized getOwnedEntityOnPosition to fetch deposit entity ID.
    		•  +3/-4     

    💡 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 15, 2024 2:03pm
    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 refactoring in `useResources.tsx` changes the function `useGetOwnedEntityOnPosition` to `useOwnedEntitiesOnPosition`, which now returns a function rather than directly executing the logic. This change could potentially break existing functionality if not all usages have been updated accordingly.
    Code Clarity:
    The new functions `getOwnedEntitiesOnPosition` and `getOwnedEntityOnPosition` could benefit from more detailed comments explaining their purpose and the logic, especially given the complex query conditions and mapping.
    github-actions[bot] commented 3 months ago

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Add a guard clause to check for null or undefined position before destructuring ___ **Consider checking for the existence of position before destructuring it to avoid potential
    runtime errors if position is undefined or null.** [client/src/hooks/helpers/useResources.tsx [234]](https://github.com/BibliothecaDAO/eternum/pull/946/files#diff-da868a5a06b096d28f8b0e970a0260a8d26d34f5acfafa3dcbd0156da8f15749R234-R234) ```diff +if (!position) return; const { x, y } = position; ```
    Suggestion importance[1-10]: 9 Why: This suggestion addresses a potential runtime error by ensuring that `position` is not null or undefined before destructuring it. This is a crucial fix to prevent unexpected crashes.
    		9
    Add a check to handle empty or undefined entities before mapping and filtering ___ **To ensure that the function getOwnedEntitiesOnPosition handles cases where entities is
    empty or undefined, add a check before proceeding to map and filter operations.** [client/src/hooks/helpers/useResources.tsx [245-251]](https://github.com/BibliothecaDAO/eternum/pull/946/files#diff-da868a5a06b096d28f8b0e970a0260a8d26d34f5acfafa3dcbd0156da8f15749R245-R251) ```diff +if (!entities || entities.length === 0) return []; return Array.from(entities) .map((entityId) => { const position = getComponentValue(Position, entityId); if (!position) return; return position?.entity_id; }) .filter(Boolean) as bigint[]; ```
    Suggestion importance[1-10]: 8 Why: Adding a check for empty or undefined `entities` enhances the robustness of the function by preventing potential errors during the mapping and filtering operations.
    		8
    Maintainability
    Extract repeated logic into a separate function for better maintainability ___ **Refactor the repeated logic for getting position and checking its existence into a
    separate function to improve code reusability and maintainability.** [client/src/hooks/helpers/useResources.tsx [247-249]](https://github.com/BibliothecaDAO/eternum/pull/946/files#diff-da868a5a06b096d28f8b0e970a0260a8d26d34f5acfafa3dcbd0156da8f15749R247-R249) ```diff -const position = getComponentValue(Position, entityId); -if (!position) return; -return position?.entity_id; +function getPositionEntityId(entityId: bigint): bigint | undefined { + const position = getComponentValue(Position, entityId); + if (!position) return undefined; + return position.entity_id; +} +return getPositionEntityId(entityId); ```
    Suggestion importance[1-10]: 7 Why: This refactoring improves code maintainability and reusability by encapsulating repeated logic into a separate function, making the code cleaner and easier to manage.
    		7
    Enhancement
    Utilize optional chaining to simplify the return statement ___ **Use TypeScript's optional chaining to simplify the return statement in the mapping
    function, enhancing readability.** [client/src/hooks/helpers/useResources.tsx [249]](https://github.com/BibliothecaDAO/eternum/pull/946/files#diff-da868a5a06b096d28f8b0e970a0260a8d26d34f5acfafa3dcbd0156da8f15749R249-R249) ```diff +return position?.entity_id; - ```
    Suggestion importance[1-10]: 6 Why: Using optional chaining improves code readability and conciseness, although it is a minor enhancement compared to the other suggestions.
    		6