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

feat: player id implemented #1723

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

feat: player id implemented #1723

wants to merge 3 commits into from

Conversation

edisontim
Copy link
Collaborator

No description provided.

Copy link

vercel bot commented Sep 27, 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 Sep 27, 2024 10:16pm

Copy link

Failed to generate code suggestions for PR

Copy link

@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 implements a player ID feature and makes several improvements to the chat system. The changes look good overall, with a few minor suggestions for optimization and code organization. The new PlayerId component provides a nice overview of player information, and the chat system updates to support direct messaging are well integrated. The use of zustand for state management is consistent throughout the changes. Consider some minor optimizations in the Chat component and potentially breaking down the PlayerId component into smaller subcomponents for better maintainability.

Thanks for using MentatBot. Give comments a 👍 or 👎 to help me improve!

setTabs(newTabs);
localStorage.setItem(CHAT_STORAGE_KEY + account.address, JSON.stringify(newTabs));
};

const messages = useMemo(() => {
const messageMap = new Map<ContractAddress, ChatMessage[]>();
Copy link

Choose a reason for hiding this comment

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

Consider memoizing this value to avoid recalculating on every render.

@@ -116,7 +117,7 @@
messages.sort((a, b) => a.timestamp.getTime() - b.timestamp.getTime());
});
return messageMap;
Copy link

Choose a reason for hiding this comment

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

This dependency array includes account.address, which may cause unnecessary re-renders. Consider using a more specific dependency if possible.

const realmSettleData = getComponentValue(
SettleRealmData,
Array.from(runQuery([HasValue(SettleRealmData, { owner_address: selectedPlayer })]))[0],
);
Copy link

Choose a reason for hiding this comment

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

This line is accessing the store state directly. Consider using a selector or moving this logic to a custom hook for better performance and maintainability.

)}
</div>
);
};
Copy link

Choose a reason for hiding this comment

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

Consider moving this avatar generation logic to a separate utility function or component for better reusability and separation of concerns.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Co-ordinates and 'share point of interest' as a link
1 participant