Add resource filter for contributions #2596
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
WalkthroughThe pull request introduces a new Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![mentatbot[bot]](https://avatars.githubusercontent.com/in/837875?s=60&v=4){kind=small}
There was a problem hiding this comment.
The PR adds a nice resource filtering feature to the contributions view. The implementation is solid but could benefit from a few improvements:
- The useMemo dependencies need to be fixed to prevent stale data
- The clipboard copy notification could be improved
- Consider adding keyboard navigation to the resource filter
- Consider persisting the filter state in URL parameters
- Adding a count of filtered vs total contributors would improve UX
Thanks for using MentatBot. Give comments a 👍 or 👎 to help me improve!
| } | ||
| return b.percentage - a.percentage; | ||
| }), | ||
| [groupedContributions, selectedResource], |
There was a problem hiding this comment.
The useMemo dependency array should include hyperstructureEntityId and resourceContributions since they are used in the memoized calculation
| [groupedContributions, selectedResource], | |
| [groupedContributions, selectedResource, hyperstructureEntityId, resourceContributions], |
| @@ -258,9 +258,9 @@ export const formatTime = ( | |||
| return parts.join(" "); | |||
| }; | |||
|
|
|||
| export const copyPlayerAddressToClipboard = (address: ContractAddress, name: string) => { | |||
| export const copyPlayerAddressToClipboard = (address: ContractAddress, name: string, hex: boolean = false) => { | |||
There was a problem hiding this comment.
Consider using a more modern notification system like toast instead of alert() for better UX
|
Failed to generate code suggestions for PR |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
client/src/ui/components/hyperstructures/ContributionSummary.tsx (1)
53-74: Efficient filtering & sorting logic.
- Filtering by
selectedResourcein lines 62-63 is well-structured, but ensure a resource ID of0isn’t inadvertently filtered out.- Sorting logic in lines 65-72 successfully prioritizes the selected resource, otherwise uses overall percentage.
Consider robust null checks for
resources[selectedResource]to avoid potential undefined references if there's data mismatch.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
client/src/ui/components/hyperstructures/ContributionSummary.tsx(3 hunks)client/src/ui/elements/SelectResource.tsx(1 hunks)client/src/ui/utils/utils.tsx(1 hunks)
🔇 Additional comments (7)
client/src/ui/elements/SelectResource.tsx (1)
79-82: Confirm string vs. numeric consistency when clearing resource selection.
The state variable selectedResource is tracked as a string in this component but is conveyed to onSelect(null) as a numeric-or-null type outside. This works fine for clearing, but ensure consumer code properly manages string vs. number states in other parts of the code as well.
client/src/ui/utils/utils.tsx (1)
261-263: Optional hex parameter adds flexibility, looks good.
Allowing the address to be copied in hexadecimal format is a valuable enhancement. Just ensure any calling site verifies if hex should indeed be true or sticks to the default false.
client/src/ui/components/hyperstructures/ContributionSummary.tsx (5)
4-4: Import of SelectResource is clear.
The import statement correctly reflects the new SelectResource component. Code style looks consistent with the rest of the imports.
5-5: Importing the modified utility function.
Invoking copyPlayerAddressToClipboard with the new hex flag is straightforward. Ensure consistent usage throughout the app if multiple address formats are required.
7-7: Memo & state usage for performance.
The introduction of useState and useMemo is appropriate for optimizing re-renders. This can help prevent unnecessary recalculations when the resource selection changes.
50-50: Initialize resource selection state to null.
Using null to denote no resource is selected aligns well with the onSelect API in SelectResource. Good consistency between parent and child.
86-117: Displaying contributor details and copying addresses.
- Adding the
SelectResourceabove the contributors list is intuitive, ensuring the user picks a resource filter before the list is shown. - The clickable address with
copyPlayerAddressToClipboard(..., true)is a neat enhancement. Verify that a hex representation is always desired in this context. - The
.filter()call in lines 105-106 elegantly checks either the selected resource or all resources.
Unit tests verifying the filter and copy functionalities would further solidify correctness.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
client/src/ui/components/hyperstructures/ContributionSummary.tsx (2)
97-102: Add error handling for address copy operationThe click handler for copying addresses should include error handling to provide feedback to users when the operation fails.
<div - onClick={() => copyPlayerAddressToClipboard(ContractAddress(playerAddress), addressName, true)} + onClick={() => { + try { + copyPlayerAddressToClipboard(ContractAddress(playerAddress), addressName, true); + } catch (error) { + console.error('Failed to copy address:', error); + // Consider adding a toast notification here + } + }} className="text-sm font-bold cursor-pointer hover:text-gold/50" > {addressName} </div>
Line range hint
10-119: Consider breaking down the component for better maintainabilityThe ContributionSummary component currently handles multiple responsibilities (filtering, sorting, and rendering). Consider splitting it into smaller, focused components:
- ContributorsList: Handle the list rendering
- ContributorCard: Handle individual contributor display
- ContributionSorter: Handle the sorting logic
This would improve maintainability and make the code more testable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/src/ui/components/hyperstructures/ContributionSummary.tsx(4 hunks)
🔇 Additional comments (3)
client/src/ui/components/hyperstructures/ContributionSummary.tsx (3)
Line range hint 4-20: LGTM: Clean import organization and state management
The new imports and state management are well-structured and follow React best practices.
43-48: LGTM: Well-implemented filtering logic
The resource filtering implementation is clean and type-safe, with proper handling of the selectedResource state.
55-75: Add missing dependencies to useMemo
The dependency array is missing hyperstructureEntityId and resourceContributions which are used in the memoized calculation.
- [groupedContributions, selectedResource],
+ [groupedContributions, selectedResource, hyperstructureEntityId, resourceContributions],
Summary by CodeRabbit
SelectResourcecomponent for selecting resource types.