BibliothecaDAO / eternum

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

army travel after raiding + hook error #976

Closed aymericdelab closed 2 weeks ago

aymericdelab commented 2 weeks ago

User description


PR Type

Bug fix, Enhancement


Description


Changes walkthrough ๐Ÿ“

Relevant files
Enhancement
useStructures.tsx
Refactor and enhance structure position hooks                       

client/src/hooks/helpers/useStructures.tsx
  • Refactored useStructuresPosition to split logic into separate hooks.
  • Added useFormattedRealmAtPosition and useFormattedStructureAtPosition
    hooks.
  • Introduced hasStructuresAtPosition function.
  • +22/-18 
    StructureCard.tsx
    Integrate new structure position hooks in StructureCard   

    client/src/ui/components/hyperstructures/StructureCard.tsx
  • Updated StructureCard to use new hooks from useStructuresPosition.
  • +5/-1     
    HexagonInformationPanel.tsx
    Integrate new structure position hooks in HexagonInformationPanel

    client/src/ui/components/worldmap/hexagon/HexagonInformationPanel.tsx
  • Updated HexagonInformationPanel to use new hooks from
    useStructuresPosition.
  • +2/-1     
    utils.tsx
    Enhance pathfinding with stamina-based step limit               

    client/src/ui/components/worldmap/hexagon/utils.tsx
  • Added getMaxSteps function to calculate maximum steps based on
    stamina.
  • Updated findShortestPathBFS to use getMaxSteps and limit pathfinding.
  • +24/-2   
    Bug fix
    useArmyAnimation.tsx
    Add fallback path logic in army animation                               

    client/src/ui/components/worldmap/armies/useArmyAnimation.tsx
  • Added fallback path logic in useArmyAnimation for cases with no path.
  • +5/-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 2 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 21, 2024 11:06am
    github-actions[bot] commented 2 weeks ago

    PR Reviewer Guide ๐Ÿ”

    โฑ๏ธ Estimated effort to review [1-5] 4
    ๐Ÿงช Relevant tests No
    ๐Ÿ”’ Security concerns No
    โšก Key issues to review Possible Bug:
    The refactoring in useStructures.tsx introduces hooks (useFormattedRealmAtPosition, useFormattedStructureAtPosition, hasStructuresAtPosition) that are defined within another hook (useStructuresPosition). This pattern can lead to unexpected behavior as hooks should not be defined conditionally or nested within other hooks unless they are custom hooks themselves and are used as such. This needs to be addressed to avoid potential bugs and React errors.
    Performance Concern:
    The findShortestPathBFS function now includes a stamina-based step limit, which is a good enhancement. However, the implementation should ensure that this does not negatively impact the performance, especially in scenarios with large maps or numerous entities. It would be beneficial to see performance benchmarks or tests to validate the impact of this change.
    github-actions[bot] commented 2 weeks ago

    PR Code Suggestions โœจ

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Refactor hooks to be defined outside and independently from other hooks ___ **Avoid defining hooks inside other hooks or functions. This can lead to unexpected behavior
    and violates React's rules of hooks. Instead, define useFormattedRealmAtPosition and
    useFormattedStructureAtPosition outside of useStructuresPosition and pass necessary
    parameters.** [client/src/hooks/helpers/useStructures.tsx [44-74]](https://github.com/BibliothecaDAO/eternum/pull/976/files#diff-396eaa9679c616f55b4078861124f09fe1125e62921747c6e600b2d803948261R44-R74) ```diff -const useFormattedRealmAtPosition = () => { +export const useFormattedRealmAtPosition = (position) => { const realmsAtPosition = useEntityQuery([HasValue(Position, position), HasValue(Structure, { category: "Realm" })]); ... }; -const useFormattedStructureAtPosition = () => { +export const useFormattedStructureAtPosition = (position) => { const structuresAtPosition = useEntityQuery([HasValue(Position, position), Has(Structure)]); ... }; ```
    Suggestion importance[1-10]: 9 Why: This suggestion addresses a significant issue with React's rules of hooks, which can lead to unexpected behavior and bugs. Refactoring the hooks to be defined outside and independently is crucial for maintaining proper hook behavior and ensuring code stability.
    9
    Performance
    Consolidate duplicate entity queries into a single reusable object ___ **Refactor the repeated calls to useEntityQuery with the same parameters into a single call
    and reuse the result. This will improve performance by reducing the number of re-renders
    and duplicate queries.** [client/src/hooks/helpers/useStructures.tsx [44-74]](https://github.com/BibliothecaDAO/eternum/pull/976/files#diff-396eaa9679c616f55b4078861124f09fe1125e62921747c6e600b2d803948261R44-R74) ```diff -const realmsAtPosition = useEntityQuery([HasValue(Position, position), HasValue(Structure, { category: "Realm" })]); -const structuresAtPosition = useEntityQuery([HasValue(Position, position), Has(Structure)]); +const entityQueries = { + realms: useEntityQuery([HasValue(Position, position), HasValue(Structure, { category: "Realm" })]), + structures: useEntityQuery([HasValue(Position, position), Has(Structure)]) +}; +const { realmsAtPosition, structuresAtPosition } = entityQueries; ```
    Suggestion importance[1-10]: 8 Why: This suggestion improves performance by reducing redundant queries and re-renders, which is beneficial for the application's efficiency. It addresses a significant but not critical issue, hence the high score.
    8
    Cache the result of getMaxSteps to improve performance in pathfinding ___ **Optimize the calculation of maxHex by caching the result of getMaxSteps if the values in
    EternumGlobalConfig and TROOPS_STAMINAS do not change frequently. This avoids
    recalculating the maximum steps every time findShortestPathBFS is called.** [client/src/ui/components/worldmap/hexagon/utils.tsx [55]](https://github.com/BibliothecaDAO/eternum/pull/976/files#diff-770c44daa009ae4d3637fe48f885dfddf3ed664920aca4d4367177be3a83bcb0R55-R55) ```diff -const maxHex = getMaxSteps(); +let cachedMaxHex; +const getMaxHex = () => { + if (!cachedMaxHex) { + cachedMaxHex = getMaxSteps(); + } + return cachedMaxHex; +}; +const maxHex = getMaxHex(); ```
    Suggestion importance[1-10]: 6 Why: This suggestion offers a performance optimization that can be beneficial, especially if the function is called frequently. However, it is not addressing a critical issue, so it receives a moderate score.
    6
    Possible issue
    Improve error handling for cases where no path is found in army animation ___ **Consider handling the case where findShortestPathBFS might return an empty path more
    robustly. Instead of directly assigning a fallback path, it might be beneficial to handle
    this scenario explicitly, perhaps by notifying the user or logging an error.** [client/src/ui/components/worldmap/armies/useArmyAnimation.tsx [32-37]](https://github.com/BibliothecaDAO/eternum/pull/976/files#diff-ce247ef03061209e44162c6a4f2a687d3ad45f5b3843f3f16253cd738cf64335R32-R37) ```diff let uiPath = findShortestPathBFS(startPos, endPos, exploredHexes).map((pos) => getUIPositionFromColRow(pos.x, pos.y), ); if (uiPath.length === 0) { - uiPath = [getUIPositionFromColRow(startPos.x, startPos.y), getUIPositionFromColRow(endPos.x, endPos.y)]; + console.error("No path found from start to end position"); + // Optionally handle the scenario more gracefully } ```
    Suggestion importance[1-10]: 7 Why: This suggestion improves error handling, which is important for debugging and user experience. However, it is not as critical as the first suggestion since it deals with a specific edge case rather than a fundamental issue.
    7