Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix guild names UI #950

Merged
merged 1 commit into from
Jun 17, 2024
Merged

Fix guild names UI #950

merged 1 commit into from
Jun 17, 2024

Conversation

bob0005
Copy link
Collaborator

@bob0005 bob0005 commented Jun 17, 2024

User description

Closes #947


PR Type

Bug fix, Enhancement


Description

  • Updated the method for decoding entity names in useEntities.tsx to use shortString.decodeShortString instead of hexToAscii.
  • Simplified the getEntityName function call in useGuilds.tsx by removing unnecessary non-null assertion.
  • Improved code readability in the formatGuilds function in useGuilds.tsx.

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

    Copy link

    vercel bot commented Jun 17, 2024

    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

    Copy link

    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.

    Copy link
    Contributor

    @mentatbot mentatbot bot left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    The pull request introduces changes that improve code readability and simplify logic. However, careful attention should be given to the new decoding method in useEntities.tsx and the handling of potentially undefined values in useGuilds.tsx to ensure robustness.

    Butler is in closed beta. Reply with feedback or to ask Butler to review other parts of the PR. Please give feedback with emoji reacts.

    @@ -37,7 +37,7 @@ export const useEntities = () => {

    const getEntityName = (entityId: bigint) => {
    const entityName = getComponentValue(EntityName, getEntityIdFromKeys([entityId]));
    return entityName ? hexToAscii(numberToHex(Number(entityName.name))) : entityId.toString();
    return entityName ? shortString.decodeShortString(entityName.name.toString()) : entityId.toString();
    Copy link
    Contributor

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Ensure that shortString.decodeShortString correctly handles all expected input formats and edge cases. This change in decoding logic could potentially introduce bugs if not all scenarios are covered.

    @@ -80,7 +80,7 @@ export const useGuilds = () => {
    (id) => getComponentValue(GuildMember, id),
    )[0]?.guild_entity_id;

    const guildName = userGuildEntityId ? getEntityName(BigInt(userGuildEntityId!)) : undefined;
    const guildName = userGuildEntityId ? getEntityName(BigInt(userGuildEntityId)) : undefined;
    Copy link
    Contributor

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Ensure that userGuildEntityId is always defined where used, or handle potential undefined cases. The removal of the non-null assertion simplifies the code but could lead to undefined behavior if not managed properly.

    Copy link

    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]

    -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]

    -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]

    +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

    @ponderingdemocritus ponderingdemocritus merged commit fa86766 into main Jun 17, 2024
    9 of 17 checks passed
    @ponderingdemocritus ponderingdemocritus deleted the fix/guild-names branch June 17, 2024 21:34
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    [client] Guild names not displaying correctly
    2 participants