refactor: extract shared ContributorsList component#3408
Conversation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughConsolidates duplicated contributor components into a single Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@frontend/src/components/ContributorsList.tsx`:
- Around line 54-61: The avatar URL concatenation in ContributorsList.tsx uses
`${item?.avatarUrl}&s=60` which breaks when item?.avatarUrl has no query string;
update the Image src construction (where item?.avatarUrl is used) to append the
size parameter correctly by detecting if the URL already contains '?' and using
'?' or '&' accordingly (or use the URL/URLSearchParams APIs to add s=60) so the
final src is valid in both cases.
- Line 73: The ShowMoreButton is out of sync with the parent because it uses its
own internal isExpanded state; update the usage in ContributorsList to pass the
parent's showAllContributors to ShowMoreButton (i.e., add prop
isExpanded={showAllContributors} where ShowMoreButton is rendered alongside the
existing onToggle={toggleContributors}), and modify the ShowMoreButton component
to accept an isExpanded prop and derive its label/display from that prop instead
of local state (remove or fallback internal state so the label always reflects
the parent's showAllContributors controlled value).
🧹 Nitpick comments (1)
frontend/src/components/MenteeContributorsList.tsx (1)
22-23: Consider memoizinggetMenteeUrlto prevent unnecessary re-renders.The
getMenteeUrlfunction is recreated on every render. While this is unlikely to cause performance issues in practice, wrapping it withuseCallbackwould be more idiomatic and preventContributorsListfrom potentially re-rendering if it uses reference equality checks.♻️ Optional optimization
+import { useCallback } from 'react' import type { IconType } from 'react-icons' import type { Contributor } from 'types/contributor' import ContributorsList from 'components/ContributorsList' // ... interface definition ... const MenteeContributorsList = ({ contributors, label = 'Mentees', maxInitialDisplay = 12, icon, programKey, moduleKey, }: MenteeContributorsListProps) => { - const getMenteeUrl = (login: string) => - `/my/mentorship/programs/${programKey}/modules/${moduleKey}/mentees/${login}` + const getMenteeUrl = useCallback( + (login: string) => + `/my/mentorship/programs/${programKey}/modules/${moduleKey}/mentees/${login}`, + [programKey, moduleKey] + )
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/__tests__/unit/components/ContributorsList.test.tsx (1)
503-518: The test correctly validates current behavior, but the component has a bug handling emptyavatarUrl.The component at line 58 produces
src="?s=60"whenavatarUrlis empty, which is exactly what the test expects. However, this is not graceful handling—?s=60is not a valid image URL and will fail to load, resulting in a broken image rather than falling back to a placeholder. The component should either use a default avatar URL or handle empty URLs differently instead of producing invalid image URLs.
🧹 Nitpick comments (1)
frontend/src/components/CardDetailsPage.tsx (1)
334-342: Consider adding a stricter guard for URL generation.The empty string fallbacks (
programKey || '',entityKey || '') could produce malformed URLs if either key is undefined. While the current guard ensures mentees exist and contextually these keys should be present for module types, a stricter condition would be more defensive:- {mentees && mentees.length > 0 && ( + {mentees && mentees.length > 0 && programKey && entityKey && (This ensures the URL is only generated when all required parameters are available.
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
@SuyashJain17 thanks for working on this 👍🏼
I pushed a little change to just have one ContributorsList component since the wrappers would just generate urls. Moved the logic for generating url into urlFormatter for consistency - we already had other url builders there too 👌🏼
2 participants
Proposed change
Resolves #3321
This PR refactors the contributor list components by extracting the shared logic from
TopContributorsListandMenteeContributorsListinto a reusableContributorsListcomponent.The new component centralizes the grid layout, contributor card rendering, and show more / less
toggle behavior, while allowing different profile URL strategies via a configurable
getUrlprop. Both existing components have been converted into thin wrappers that preservetheir original public APIs and behavior.
This change reduces code duplication and improves maintainability without introducing any
visual or functional changes.
Checklist
make check-testlocally: all warnings addressed, tests passed