BibliothecaDAO / eternum

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

Fix guild names UI #950

Closed cwastche closed 3 weeks ago

cwastche commented 3 weeks ago

User description

Closes #947


PR Type

Bug fix, Enhancement


Description


Changes walkthrough πŸ“

Relevant files
Enhancement
useEntities.tsx
Update entity name decoding method to use `shortString`. 

client/src/hooks/helpers/useEntities.tsx
  • Removed hexToAscii import.
  • Added shortString import from starknet.
  • Replaced hexToAscii with shortString.decodeShortString for decoding
    entity names.
  • +3/-3     
    Bug fix
    useGuilds.tsx
    Simplify guild name retrieval and improve readability.     

    client/src/hooks/helpers/useGuilds.tsx
  • Simplified getEntityName function call by removing unnecessary
    non-null assertion.
  • Added spacing for better readability in formatGuilds function.
  • +3/-2     

    πŸ’‘ 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 17, 2024 9:35am
    github-actions[bot] commented 3 weeks ago

    PR Reviewer Guide πŸ”

    ⏱️ Estimated effort to review [1-5] 2
    πŸ§ͺ Relevant tests No
    πŸ”’ Security concerns No
    ⚑ Key issues to review Possible Bug:
    Ensure that shortString.decodeShortString correctly handles all expected input formats and edge cases, as it replaces hexToAscii(numberToHex(Number(entityName.name))). This change in decoding logic could potentially introduce bugs if not all scenarios are covered.
    Refactoring Impact:
    The removal of the non-null assertion in useGuilds.tsx simplifies the code but ensure that userGuildEntityId is always defined where used, or handle potential undefined cases.
    github-actions[bot] commented 3 weeks ago

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Simplify the code by removing redundant BigInt conversion ___ **Ensure that userGuildEntityId is always treated as a BigInt by removing the unnecessary
    conditional check, as it is already converted to BigInt in the new code.** [client/src/hooks/helpers/useGuilds.tsx [83]](https://github.com/BibliothecaDAO/eternum/pull/950/files#diff-4a6ae30a3969851b248ce9c56e472b99b3f3ae1a4598fd3610e91aadc0899fa0R83-R83) ```diff -const guildName = userGuildEntityId ? getEntityName(BigInt(userGuildEntityId)) : undefined; +const guildName = userGuildEntityId ? getEntityName(userGuildEntityId) : undefined; ```
    Suggestion importance[1-10]: 7 Why: The suggestion correctly identifies that the redundant BigInt conversion can be removed, simplifying the code. However, this is a minor enhancement and does not address a major issue.
    7
    Maintainability
    Remove unnecessary conditional check for better code clarity ___ **Remove the unnecessary check for guild_entity_id in the conditional operator since it's
    already ensured to be non-null by the context.** [client/src/hooks/helpers/useGuilds.tsx [83]](https://github.com/BibliothecaDAO/eternum/pull/950/files#diff-4a6ae30a3969851b248ce9c56e472b99b3f3ae1a4598fd3610e91aadc0899fa0R83-R83) ```diff -const guildName = userGuildEntityId ? getEntityName(BigInt(userGuildEntityId)) : undefined; +const guildName = getEntityName(BigInt(userGuildEntityId)); ```
    Suggestion importance[1-10]: 6 Why: The suggestion improves code clarity by removing an unnecessary check. However, this is a minor maintainability improvement and does not address a critical issue.
    6
    Possible issue
    Verify the correctness of the shortString.decodeShortString method for decoding entity names ___ **Consider checking if shortString module is correctly imported and if decodeShortString
    method is appropriate for the type of data handled, as the previous implementation used
    hexToAscii.** [client/src/hooks/helpers/useEntities.tsx [40]](https://github.com/BibliothecaDAO/eternum/pull/950/files#diff-655bd94eaba19bfcec06d7b736d2e64944c31d55f2b38819201a2b0b612787e4R40-R40) ```diff +return entityName ? shortString.decodeShortString(entityName.name.toString()) : entityId.toString(); - ```
    Suggestion importance[1-10]: 5 Why: The suggestion raises a valid point about verifying the correctness of the `shortString.decodeShortString` method. However, it does not provide a concrete improvement or fix, making it more of a cautionary note.
    5