BibliothecaDAO / eternum

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

guild work #1016

Closed ponderingdemocritus closed 3 months ago

ponderingdemocritus commented 3 months ago

User description


PR Type

Enhancement, Bug fix


Description


Changes walkthrough πŸ“

Relevant files
Enhancement
useGuilds.tsx
Add function to fetch guild owner in useGuilds hook           

client/src/hooks/helpers/useGuilds.tsx
  • Added getGuildOwner function to fetch the owner of a guild.
  • Updated the return object to include getGuildOwner.
  • +11/-1   
    GuildMembers.tsx
    Integrate guild owner logic in GuildMembers component       

    client/src/ui/components/worldmap/guilds/GuildMembers.tsx
  • Added ownerAddress prop to GuildMembers component.
  • Integrated getGuildOwner to fetch and use guild owner information.
  • Modified conditional rendering for kicking out members based on guild
    owner.
  • +7/-4     
    Guilds.tsx
    Update CSS classes for guild items                                             

    client/src/ui/components/worldmap/guilds/Guilds.tsx - Adjusted CSS classes for guild items.
    +2/-2     
    MyGuild.tsx
    Improve layout and styling for MyGuild component                 

    client/src/ui/components/worldmap/guilds/MyGuild.tsx
  • Adjusted CSS classes and layout for guild creation form.
  • Improved layout for guild name display.
  • +12/-10 
    SelectBox.tsx
    Update CSS styling for SelectBox component                             

    client/src/ui/elements/SelectBox.tsx - Updated CSS classes for `SelectBox` component.
    +2/-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 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 26, 2024 1:01am
    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 getGuildOwner function in useGuilds.tsx uses a memoization with an empty dependency array. This means the function will not update if the underlying data changes, which could lead to stale data issues.
    Data Handling Concern:
    The getGuildOwner function directly accesses the first element of an array without checking if the array is empty. This could potentially lead to runtime errors if the array is empty.
    UI Logic Condition:
    In GuildMembers.tsx, the condition to display the "Kick out" button checks if guildOwner?.address == BigInt(account.address). This assumes guildOwner will always be defined, which might not be the case if the getGuildOwner function fails or returns an undefined or null value.
    github-actions[bot] commented 3 months ago

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add dependencies to useMemo for getGuildOwner to ensure it updates correctly ___ **Consider adding a dependency array to the useMemo hook for getGuildOwner to ensure that it
    recomputes only when necessary. Currently, it's set to an empty array, which means it only
    computes once and does not update if runQuery or getComponentValue functions or their
    dependencies change.** [client/src/hooks/helpers/useGuilds.tsx [77-85]](https://github.com/BibliothecaDAO/eternum/pull/1016/files#diff-4a6ae30a3969851b248ce9c56e472b99b3f3ae1a4598fd3610e91aadc0899fa0R77-R85) ```diff const getGuildOwner = useMemo( () => (guildEntityId: bigint) => { const owner = Array.from(runQuery([HasValue(Owner, { entity_id: guildEntityId })])).map((id) => getComponentValue(Owner, id), )[0]; return owner; }, - [], + [runQuery, getComponentValue], # Assuming these functions are props or could change ); ```
    Suggestion importance[1-10]: 8 Why: Adding dependencies to the `useMemo` hook ensures that the memoized function recomputes when necessary, improving the accuracy and reliability of the component. This is a significant improvement for maintaining correct state.
    8
    Maintainability
    Simplify the conditional logic in the className attribute by extracting it to a constant ___ **The condition inside the className template string is complex and can be simplified for
    better readability and maintainability. Consider extracting the condition to a constant
    above the return statement.** [client/src/ui/components/worldmap/guilds/GuildMembers.tsx [85-87]](https://github.com/BibliothecaDAO/eternum/pull/1016/files#diff-df74009fce54be014b2566aa226e13ff9ebfbc021d40359b58e4848b60fe80b7R85-R87) ```diff -className={`grid grid-cols-3 gap-4 text-sm clip-angled-sm p-1 ${ - member.playerAddress == account.address ? "bg-green/20" : "" -} `} +const isCurrentUser = member.playerAddress == account.address; +className={`grid grid-cols-3 gap-4 text-sm clip-angled-sm p-1 ${isCurrentUser ? "bg-green/20" : ""}`} ```
    Suggestion importance[1-10]: 7 Why: Extracting the condition to a constant improves readability and maintainability of the code, making it easier to understand and modify in the future.
    7
    Extract the complex className logic to a constant for better readability ___ **The className for the guild list item should be extracted to a constant before the return
    statement to improve code readability and potentially allow easier changes in the future.** [client/src/ui/components/worldmap/guilds/Guilds.tsx [110-112]](https://github.com/BibliothecaDAO/eternum/pull/1016/files#diff-35e1ce0de5d066ca31a31dee90665f2b4ff706e2473f4e0d65c4472394042cd5R110-R112) ```diff -className={`grid grid-cols-4 gap-4 text-md clip-angled-sm p-1 ${ +const guildItemClass = `grid grid-cols-4 gap-4 text-md clip-angled-sm p-1 ${ userGuildEntityId === BigInt(guild.entity_id) ? "bg-green/20" : "" -} `} +}`; +className={guildItemClass} ```
    Suggestion importance[1-10]: 7 Why: Extracting the `className` logic to a constant improves code readability and maintainability, making it easier to manage and update the styling logic.
    7
    Best practice
    Use a function in setIsPublic to toggle state based on the previous state ___ **The onClick handler for SelectBox should be updated to handle the state change more
    explicitly by using a function that receives the previous state.** [client/src/ui/components/worldmap/guilds/MyGuild.tsx [221-223]](https://github.com/BibliothecaDAO/eternum/pull/1016/files#diff-187d62a536d58004f66e023774aeb6b9d3cdda70528f8236c8bc7ee63db1aa23R221-R223) ```diff - setIsPublic(!isPublic)}> + setIsPublic(prevState => !prevState)}> Public ```
    Suggestion importance[1-10]: 6 Why: Using a function to handle state updates based on the previous state is a best practice in React, ensuring that the state is updated correctly even if multiple state updates are queued.
    6