feat(leaderboard): add tooltip explaining contribution scoring#43
feat(leaderboard): add tooltip explaining contribution scoring#43gautamsidhwani29 wants to merge 6 commits intoCircuitVerse:mainfrom
Conversation
✅ Deploy Preview for circuitverse-leaderboard ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughAdds a new PointsInfoButton component and renders it inline in the Leaderboard header (desktop: tooltip; mobile: bottom-sheet) to show points allocation. Updates LeaderboardView layout and responsive breakpoints, standardizes string quoting, refactors Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ 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 |
❌ Deploy Preview for cv-community-dashboard failed.
|
|
hey @gautamsidhwani29 that is a helpful PR, one suggestion, i have already created a reusable component named hint.tsx can you replace with our tooltip, just to make the code more readable :) thanks!! |
|
Hey @gautamsidhwani29 info tooltip is not responsive for mid-sized viewports, can you fix that as well please? |
|
@Atharva7126 will use the hint component for the infocard |
yes for sure! |
|
also one suggestion replace this with the info, it is punchy but uses higher-level vocabulary than "opened" Heading: Point Allocation
Footer: Leaderboard reflects total activity score |
![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
🧹 Nitpick comments (2)
components/hint.tsx (1)
25-26: Consider extracting z-index value to a theme constant.The hardcoded
z-50class could conflict with other layered UI elements. For maintainability and consistency, consider defining z-index values in a central theme configuration.components/PointsInfoButton.tsx (1)
17-94: Extract hardcoded color value to theme configuration.The color
#50B78Bappears multiple times throughout this component (lines 29, 42, 68). For maintainability and theming consistency, define this color in your theme configuration and reference it via a CSS variable or Tailwind theme extension.Example: Using Tailwind theme extension
In your
tailwind.config.js:+theme: { + extend: { + colors: { + 'brand-primary': '#50B78B', + }, + }, +}Then update the component:
-className="font-semibold text-[#50B78B] text-base" +className="font-semibold text-brand-primary text-base"
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
components/Leaderboard/LeaderboardView.tsxcomponents/PointsInfoButton.tsxcomponents/hint.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- components/Leaderboard/LeaderboardView.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
components/hint.tsx (1)
components/ui/tooltip.tsx (4)
TooltipProvider(61-61)Tooltip(61-61)TooltipTrigger(61-61)TooltipContent(61-61)
🔇 Additional comments (5)
components/hint.tsx (2)
28-36: Conditional rendering preserves backward compatibility.The type guard and conditional rendering correctly handle both
stringandReactNodelabels. String labels receive the original styled wrapper, while ReactNode labels are rendered directly, allowing custom styling.
11-11: Type change is backward-compatible, not breaking.The
labelprop has been widened fromstringtostring | React.ReactNode. This is an API expansion, not a breaking change. All existing callers passing strings (e.g., infooter.tsxandRelativeTime.tsx) remain compatible. The implementation correctly handles both types with conditional rendering. New callers can now pass ReactNode content (as inPointsInfoButton.tsx), but no existing code requires modification.Likely an incorrect or invalid review comment.
components/PointsInfoButton.tsx (3)
6-10: LGTM!The
POINTS_CONFIGcorrectly defines the scoring rules specified in issue #42 (PR merged: +5, PR opened: +2, Issue opened: +1). Theas constassertion provides proper type narrowing.
102-112: Desktop tooltip implementation looks good.The desktop variant correctly uses the
Hintcomponent withPointsContentas a ReactNode label. The button includes proper accessibility attributes (aria-label, focus-visible ring) and responsive behavior.
127-132: The animation utility classes (animate-in,fade-in,slide-in-from-bottom) are properly configured in this project. The codebase uses@tailwindcss/postcss, Tailwind CSS's official v4 plugin, which includes built-in animation utilities. These classes are already being used consistently throughout the codebase in multiple UI components (tooltip.tsx, popover.tsx, select.tsx, dropdown-menu.tsx) and require no additional configuration.
| {showMobileInfo && ( | ||
| <> | ||
| <div | ||
| className="fixed inset-0 bg-black/60 z-40 animate-in fade-in duration-200" | ||
| onClick={() => setShowMobileInfo(false)} | ||
| /> | ||
|
|
||
| <div | ||
| className="fixed bottom-0 left-0 right-0 z-50 bg-white dark:bg-zinc-900 rounded-t-2xl border-t border-zinc-200 dark:border-zinc-800 shadow-2xl animate-in slide-in-from-bottom duration-300" | ||
| role="dialog" | ||
| > | ||
| <PointsContent | ||
| isMobile | ||
| onClose={() => setShowMobileInfo(false)} | ||
| /> | ||
| </div> | ||
| </> | ||
| )} |
There was a problem hiding this comment.
Critical accessibility gaps in mobile modal implementation.
The mobile dialog is missing several essential accessibility features:
- Missing
aria-labelledby: The dialog should reference its title for screen readers. - No keyboard dismissal: Users cannot close the modal with the Escape key.
- No focus management: Focus should move to the dialog when opened and return to the trigger button when closed.
- Backdrop not keyboard-accessible: The backdrop uses
onClickwhich doesn't work for keyboard navigation; consider addingonKeyDownor making the backdrop non-interactive.
These issues prevent keyboard-only users from effectively using this feature.
Recommended fixes
+import { useEffect, useRef } from "react";
export default function PointsInfoButton() {
const [showMobileInfo, setShowMobileInfo] = useState(false);
+ const triggerRef = useRef<HTMLButtonElement>(null);
+ const dialogRef = useRef<HTMLDivElement>(null);
+
+ useEffect(() => {
+ if (showMobileInfo) {
+ dialogRef.current?.focus();
+ } else {
+ triggerRef.current?.focus();
+ }
+ }, [showMobileInfo]);
+
+ useEffect(() => {
+ const handleEscape = (e: KeyboardEvent) => {
+ if (e.key === 'Escape' && showMobileInfo) {
+ setShowMobileInfo(false);
+ }
+ };
+ document.addEventListener('keydown', handleEscape);
+ return () => document.removeEventListener('keydown', handleEscape);
+ }, [showMobileInfo]);
return (
<>
{/* ... */}
<div className="md:hidden">
<button
+ ref={triggerRef}
type="button"
onClick={() => setShowMobileInfo(true)}
className="..."
+ aria-label="Show points allocation"
>
ⓘ
</button>
{showMobileInfo && (
<>
<div
className="..."
onClick={() => setShowMobileInfo(false)}
+ aria-hidden="true"
/>
<div
+ ref={dialogRef}
className="..."
role="dialog"
+ aria-labelledby="points-modal-title"
+ aria-modal="true"
+ tabIndex={-1}
>
<PointsContent
isMobile
onClose={() => setShowMobileInfo(false)}
/>
</div>
</>
)}
</div>
</>
);
}Committable suggestion skipped: line range outside the PR's diff.
|
Hey @gautamsidhwani29 can you please resolve coderrabbit suggestions :)) |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
components/PointsInfoButton.tsx (1)
148-156: Previous accessibility issues addressed.The mobile button now includes the
aria-labelattribute. Good work implementing the feedback from the previous review.Consider adding
focus-visible:ringstyling to match the desktop button for consistency:- className="text-lg text-[#50B78B]/70 active:text-[#50B78B] transition-colors focus:outline-none rounded-sm" + className="text-lg text-[#50B78B]/70 active:text-[#50B78B] transition-colors focus:outline-none focus-visible:ring-2 focus-visible:ring-[#50B78B]/50 rounded-sm"
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/PointsInfoButton.tsx
🔇 Additional comments (4)
components/PointsInfoButton.tsx (4)
6-10: LGTM!The
POINTS_CONFIGconstant withas constassertion provides good type safety and makes the scoring rules easily maintainable. The values align with the documented point allocation.
12-15: LGTM!Interface is minimal and correctly typed for the component's usage.
17-95: LGTM!The
PointsContentcomponent is well-structured with proper accessibility attributes. The mobile close button has an appropriatearia-label, and theid="points-modal-title"correctly references the modal'saria-labelledby. The conditional styling for mobile vs. desktop variants is clear and maintainable.
158-180: Accessibility improvements well implemented.The modal now includes all essential accessibility features that were flagged in the previous review:
aria-labelledby="points-modal-title"correctly references the headeraria-modal="true"androle="dialog"are presenttabIndex={-1}allows programmatic focus- Backdrop uses
aria-hidden="true"- Escape key handler is implemented in the effect above
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/Leaderboard/LeaderboardView.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
components/Leaderboard/LeaderboardView.tsx (1)
components/Leaderboard/ActivityTrendChart.tsx (1)
ActivityTrendChart(13-211)
🔇 Additional comments (2)
components/Leaderboard/LeaderboardView.tsx (2)
241-246: LGTM! PointsInfoButton integration addresses PR objectives and responsive feedback.The changes successfully integrate the PointsInfoButton inline with the leaderboard title, as requested in issue #42. The responsive breakpoint change from
mdtosm(line 241) directly addresses the mid-sized viewport concern raised by naman79820.
255-268: LGTM! Improved search bar structure and styling.The search bar refactoring improves the component structure by properly positioning the search icon within the input using absolute positioning. The enhanced styling includes better focus states and dark mode support. The responsive breakpoint change (
md→sm) aligns with the overall improvements for mid-sized viewports.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
components/Leaderboard/LeaderboardView.tsx (1)
12-12: Unused import issue resolved.The previously flagged unused
Divideimport has been removed. The import statement now correctly includes only the icons used in this file.
🧹 Nitpick comments (2)
components/Leaderboard/LeaderboardView.tsx (2)
257-268: Consider extracting input styles for better readability.The search bar implementation is functionally correct and includes proper accessibility features (focus-visible ring, aria-label via Search icon). However, the Input component has a very long className string (spanning lines 264-266) that reduces readability.
Optional: Extract styles to a constant or use Tailwind's @apply
You could improve maintainability by extracting the repeated focus styles:
+ const searchInputClasses = cn( + "pl-9 h-9 w-full sm:w-64", + "bg-white dark:bg-[#07170f]", + "border border-[#50B78B]/60 dark:border-[#50B78B]/40", + "text-foreground dark:text-foreground", + "shadow-sm dark:shadow-none", + "focus-visible:ring-2 focus-visible:ring-[#50B78B] focus-visible:ring-offset-0", + "transition-colors" + ); <Input type="text" placeholder="Search contributors..." value={searchQuery} onChange={(e) => setSearchQuery(e.target.value)} - className=" - pl-9 h-9 w-full sm:w-64 bg-white dark:bg-[#07170f] border border-[#50B78B]/60 dark:border-[#50B78B]/40 text-foreground dark:text-foreground shadow-sm dark:shadow-none outline-none focus:outline-none focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-[#50B78B] focus-visible:ring-offset-0 transition-colors - " + className={searchInputClasses} />Also note: the multiple
outline-nonedeclarations (outline-none focus:outline-none focus-visible:outline-none) are redundant—focus-visible:ring-*already provides the appropriate focus indicator.
247-247: Remove unnecessary blank line.There's an empty line (247) after the closing
</h1>tag that doesn't follow the spacing pattern of the rest of the file.Minor cleanup
</h1> - <p className="text-muted-foreground">
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/Leaderboard/LeaderboardView.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
components/Leaderboard/LeaderboardView.tsx (2)
components/PointsInfoButton.tsx (1)
PointsInfoButton(97-190)components/Leaderboard/ActivityTrendChart.tsx (1)
ActivityTrendChart(13-211)
🔇 Additional comments (4)
components/Leaderboard/LeaderboardView.tsx (4)
418-418: LGTM: Consistent responsive breakpoint adjustment.The card layout breakpoint change from
mdtosmis consistent with the header changes (line 241) and improves the mobile experience by enabling horizontal layout at 640px instead of 768px.
508-517: Activity chart hidden on mobile devices.The ActivityTrendChart is wrapped in a
hidden sm:blockcontainer, meaning it will only display on screens ≥ 640px wide. This is likely intentional to save space on mobile, but ensure this design decision aligns with the mobile UX requirements.The conditional rendering logic correctly checks for the presence and non-empty state of
entry.daily_activitybefore rendering the chart.
20-20: No action needed—PointsInfoButton correctly implements all requirements.The
PointsInfoButtoncomponent already uses theHintcomponent (line 4 of PointsInfoButton.tsx) and properly implements all requested features:
- Hint component usage: Correctly imported and used on line 141 for desktop tooltip
- Tooltip content: Matches expectations exactly—"Points Allocation" header, correct point allocations (+5 merged, +2 opened, +1 issue), and footer text "Leaderboard reflects total activity score"
- Responsiveness: Properly handles mid-sized viewports with separate implementations—desktop uses Hint tooltip, mobile uses bottom sheet modal with full accessibility support
The component is fully functional and addresses all reviewer suggestions from the PR comments.
241-246: The responsive layout change frommdtosmis intentional and consistent with other components in the codebase. The title andPointsInfoButtondisplay correctly on narrow mobile screens due to proper flex containment usingmin-w-0on the parent container. ThePointsInfoButtonis a minimal-width icon (~30-40px), and period labels ("Daily", "Weekly", "Monthly", "Yearly") fit within typical mobile viewports when rendered attext-4xl. No layout adjustments needed.
|
@naman79820 you can have a look at this now |
|
Hey @gautamsidhwani29 tooltip is still not responsive |
|
also can you fix coderabbit suggestions which he gave |
|
Now after forking the latest repo, the PR merged / PR opened / Issue opened points look like this now what do you think @Atharva7126 should we make it exact on tooltip or let it be like that? @gautamsidhwani29 you can also give your thoughts in it :)) |
|
I think we should Match the colors on the hint as well but at the same time i think it going to look cluttered as 3-4 colors in a hint will look something off , also @naman79820 can you point out the exact issue in the responsiveness by a screenshot |
|
honestly @naman79820 PR merged / PR opened / Issue opened points are looking very different from the layout of the leaderboard, the old pills were perfectly matching that and for the tooltip it should be simple as user dont really observe that much |
Desktop.2026.01.03.-.20.09.58.02.mp4 |
naman79820
left a comment
naman79820
left a comment
There was a problem hiding this comment.
Can you please resolve these merge conflicts as ui is abit changed now :))
|
@coderabbitai what needs to be changed and cleaned up? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 8476 🏁 Script executed: Length of output: 18072 🏁 Script executed: Length of output: 216 @gautamsidhwani29 Great job addressing most of the suggestions! Here's a summary of what you've already fixed: ✅ Already Addressed
🎨 Optional Minor Improvements (Nitpicks)These are optional styling improvements, not critical issues:
SummaryAll critical issues have been resolved! The remaining items are minor style/formatting suggestions that don't affect functionality. The PR is in good shape from a technical perspective. The main discussion now seems to be about the visual design with the reviewers discussing color matching for the points display. |
gautamsidhwani29
force-pushed
the
feat/tooltip-for-points-info
branch
from
0da304b to
839b261
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
components/Leaderboard/LeaderboardView.tsx (1)
943-1046: Consider extracting pagination logic for testability.The pagination UI implementation is well-crafted with good accessibility attributes and smart ellipsis handling. However, the complex inline IIFE (lines 961-1031) would benefit from extraction into a separate helper function for better testability and reusability.
🔎 Suggested refactor to extract pagination logic
Create a helper function in the same file:
function calculatePageNumbers(currentPage: number, totalPages: number): number[] { const pages: number[] = []; if (totalPages <= 7) { for (let i = 1; i <= totalPages; i++) { pages.push(i); } } else { pages.push(1); if (currentPage <= 4) { for (let i = 2; i <= 5; i++) { pages.push(i); } pages.push(-1); pages.push(totalPages); } else if (currentPage >= totalPages - 3) { pages.push(-1); for (let i = totalPages - 4; i <= totalPages; i++) { pages.push(i); } } else { pages.push(-1); pages.push(currentPage - 1); pages.push(currentPage); pages.push(currentPage + 1); pages.push(-1); pages.push(totalPages); } } return pages; }Then simplify the rendering:
<div className="flex items-center gap-1"> {calculatePageNumbers(currentPage, totalPages).map((pageNum, idx) => { if (pageNum === -1) { return ( <span key={`ellipsis-${idx}`} className="px-2 text-muted-foreground"> … </span> ); } return ( <Button key={pageNum} variant={currentPage === pageNum ? "default" : "ghost"} size="sm" onClick={() => goToPage(pageNum)} className={cn( "h-9 w-9 p-0", currentPage === pageNum ? "bg-[#50B78B] text-white hover:bg-[#50B78B]/90" : "hover:bg-[#50B78B]/20 hover:text-[#50B78B]" )} aria-label={`Go to page ${pageNum}`} aria-current={currentPage === pageNum ? "page" : undefined} > {pageNum} </Button> ); })} </div>This improves testability and separates concerns between logic and presentation.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
components/Leaderboard/LeaderboardView.tsxcomponents/PointsInfoButton.tsxcomponents/hint.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- components/PointsInfoButton.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
components/Leaderboard/LeaderboardView.tsx (2)
lib/leaderboard.ts (2)
SortBy(3-3)sortEntries(9-30)lib/utils.ts (1)
cn(5-7)
🔇 Additional comments (10)
components/hint.tsx (3)
28-36: Clean conditional rendering implementation.The type-based conditional rendering correctly handles both string and ReactNode labels. String labels maintain the previous styled appearance while ReactNode labels can provide their own styling.
11-11: API change is backward compatible and properly implemented.The expansion of
labelto acceptstring | React.ReactNodepasses verification. All existing usages remain compatible: string literals in footer.tsx and RelativeTime.tsx continue to work, while the PointsInfoButton component's React.ReactNode usage is now formally supported. The implementation correctly handles both types with a typeof check (lines 28–36), rendering strings in a styled container and passing through ReactNode directly.
25-26: z-50 is appropriate and consistent with the design system.The z-index value of
z-50on TooltipContent aligns with the application's established stacking strategy. All overlay components in the codebase (dropdowns, popovers, tooltips, modals, etc.) consistently usez-50, indicating this is an intentional design pattern, not a conflict risk. No higher z-indices exist in the codebase that would cause stacking issues.components/Leaderboard/LeaderboardView.tsx (7)
38-38: LGTM!The PointsInfoButton import supports the main feature of this PR.
82-109: Improved type safety and structure.The refactored
activityStyleswith explicit type annotations enhances code clarity and maintainability. The consistent addition ofborderColorto all activity types is a good improvement.
111-120: Safer fallback handling with explicit defaults.The refactored
getActivityStylenow returns a well-structured fallback object instead of potentially undefined values, improving defensive programming.The fallback
icon: () => nullworks correctly with the conditional rendering pattern at lines 876-883 whereIconComponent &&ensures it's only rendered when the component exists.
497-507: Verify responsive behavior on mid-sized viewports.The PointsInfoButton integration looks good, and the header layout adjustments are reasonable. However, PR comments indicate ongoing concerns about responsiveness on mid-sized viewports.
Based on the PR discussion, ensure the tooltip and header layout work correctly across all viewport sizes, particularly mid-sized screens (tablet range). The
overflow-x-autowrapper on line 499 suggests potential overflow issues that should be tested.Test the following breakpoints:
- Small mobile (< 640px)
- Large mobile/small tablet (640px - 768px)
- Medium tablet (768px - 1024px)
- Desktop (1024px+)
Verify:
- The PointsInfoButton tooltip appears correctly and is fully visible
- The header title doesn't overflow or wrap awkwardly
- The search and filter controls remain accessible
152-212: Good standardization of URLSearchParams usage.The consistent use of
.get("key")throughout improves code readability and follows the correct URLSearchParams API pattern.
846-909: Enhanced activity breakdown rendering.The activity breakdown rendering is well-structured with explicit type annotations, proper icon handling, and consistent styling. The priority-based sorting ensures consistent display order.
915-923: Good defensive rendering for daily activity charts.The conditional check ensures charts only render when data exists, preventing potential rendering issues with empty datasets.
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)
components/Leaderboard/LeaderboardView.tsx (1)
372-390: Inconsistent filter clearing behavior between mobile and desktop.The
clearFiltersfunction behaves differently on mobile vs. desktop:
- Desktop: Clears roles, sort, and search query
- Mobile: Only clears search query (early return on line 377)
This inconsistency means mobile users cannot clear role filters or sorting via the "Clear" button, leading to a confusing user experience.
🔎 Proposed fix: Make clearFilters consistent across devices
const clearFilters = () => { - const isMobile = typeof window !== "undefined" && window.innerWidth < 640; const params = new URLSearchParams(searchParams.toString()); - if (isMobile) { - setSearchQuery(""); - return; - } params.delete("roles"); params.delete("sort"); params.delete("order"); // Reset to page 1 when clearing filters params.delete("page"); setCurrentPage(1); // Note: We preserve the limit param when clearing filters window.history.replaceState(null, "", `${pathname}?${params.toString()}`); setSearchQuery(""); setSortBy("points"); };
🧹 Nitpick comments (2)
components/Leaderboard/LeaderboardView.tsx (2)
283-287: Good defensive error handling.The try-catch around
sortEntriesprevents crashes and logs errors appropriately. This ensures the UI remains functional even if sorting fails.Optional improvement: Consider displaying a toast notification to inform users when sorting fails, rather than silently falling back to the unsorted list.
965-1038: Extract pagination page calculation to a helper function.The IIFE-based page number calculation (lines 967-1037) is complex and difficult to maintain inline. The logic appears correct but would benefit from extraction to a named helper function for better readability and testability.
🔎 Suggested refactor
Extract to a helper above the component:
function calculatePageNumbers(currentPage: number, totalPages: number): number[] { const pages: number[] = []; if (totalPages <= 7) { for (let i = 1; i <= totalPages; i++) { pages.push(i); } } else { pages.push(1); if (currentPage <= 4) { for (let i = 2; i <= 5; i++) { pages.push(i); } pages.push(-1); // ellipsis pages.push(totalPages); } else if (currentPage >= totalPages - 3) { pages.push(-1); for (let i = totalPages - 4; i <= totalPages; i++) { pages.push(i); } } else { pages.push(-1); pages.push(currentPage - 1); pages.push(currentPage); pages.push(currentPage + 1); pages.push(-1); pages.push(totalPages); } } return pages; }Then simplify the JSX:
<div className="flex items-center gap-1"> - {(() => { - const pages: number[] = []; - // ... complex logic ... - return pages.map((pageNum, idx) => { + {calculatePageNumbers(currentPage, totalPages).map((pageNum, idx) => { + if (pageNum === -1) { + return ( + <span key={`ellipsis-${idx}`} className="px-2 text-muted-foreground"> + … + </span> + ); + } + return ( // ... Button render ... - }); - })()} + ); + })} </div>
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/Leaderboard/LeaderboardView.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
components/Leaderboard/LeaderboardView.tsx (6)
lib/leaderboard.ts (1)
sortEntries(9-32)components/PointsInfoButton.tsx (1)
PointsInfoButton(104-201)lib/utils.ts (1)
cn(5-7)components/Leaderboard/ActivityTrendChart.tsx (1)
ActivityTrendChart(13-211)components/ui/button.tsx (1)
Button(60-60)components/ui/card.tsx (2)
Card(85-85)CardContent(91-91)
🔇 Additional comments (7)
components/Leaderboard/LeaderboardView.tsx (7)
39-39: Verify PointsInfoButton responsiveness and planned refactor to hint component.Per PR comments, responsiveness issues on mid-sized viewports have been reported repeatedly. Additionally, a reviewer requested replacing this custom component with the existing reusable
hint.tsxcomponent for consistency.Please confirm:
- The tooltip renders correctly and is responsive across all viewport sizes (especially mid-range as flagged in comments).
- Whether you still plan to refactor this to use the
hintcomponent as suggested by the reviewer.If responsiveness issues persist, please test the component at various breakpoints (sm, md, lg) and ensure the mobile bottom-sheet and desktop tooltip both function correctly.
83-91: LGTM: Improved type explicitness.The expanded type annotation makes the structure of
activityStylesmore explicit and readable. This is a good refactoring for maintainability.
246-262: Verify the ranking behavior change is intentional.The ranking calculation now computes ranks within the filtered role subset rather than globally. This means when users filter by role, they see relative ranks within that role (e.g., "1st among mentors") rather than global ranks.
While this behavior seems intuitive, please confirm:
- This change is intentional and aligns with product requirements.
- Users understand that filtered rankings are relative to the filter, not absolute.
Consider adding a visual indicator (e.g., "Rank within selected role(s)") if rankings are filtered, to avoid confusion when the same contributor shows different ranks under different filters.
500-510: LGTM: Clean integration of PointsInfoButton.The button is well-integrated into the header with appropriate responsive spacing and overflow handling. The inline-flex alignment ensures the button stays visually connected to the "Leaderboard" text.
Note: This assumes the PointsInfoButton component itself handles responsiveness correctly (as flagged in earlier comment on line 39).
921-929: LGTM: Defensive rendering of activity chart.The conditional check ensures
ActivityTrendChartonly renders when data is available, preventing empty charts and potential errors. This is a good defensive coding practice.
1064-1083: LGTM: Consistent styling across leaderboard and sidebar.The sidebar activity cards now use the same color scheme and styling as the main leaderboard activity badges, creating a cohesive visual experience. The use of
cn()with activity styles ensures maintainability.
1120-1125: LGTM: Improved activity count formatting.The singular/plural formatting ("1 activity" vs "2 activities") improves readability and polish.
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)
components/Leaderboard/LeaderboardView.tsx (1)
651-692: Remove duplicate Checkbox components.Each role renders two separate
Checkboxcomponents:
- Lines 657-661:
<Checkbox id={role} .../>- Lines 672-680:
<Checkbox id={role-${role}} .../>The label at line 663 has
htmlFor={role-${role}}, which only associates with the second checkbox. The first checkbox is orphaned and creates a confusing double-checkbox UI. Additionally, the label has a redundantkey={role}attribute when it's already inside a div with the same key.🔎 Proposed fix
Remove the first checkbox and its wrapper div, and remove the redundant
keyfrom the label:<h4 className="font-medium text-sm">Role</h4> - <div className="space-y-2 max-h-64 overflow-y-auto px-4"> + <div className="space-y-2 max-h-64 overflow-y-auto"> {availableRoles.map((role) => ( - <div - key={role} - className="flex items-center space-x-2" - > - <Checkbox - id={role} - checked={selectedRoles.has(role)} - onCheckedChange={() => toggleRole(role)} - /> <label key={role} htmlFor={`role-${role}`} className={cn( "flex items-center gap-3 px-3 py-2.5 rounded-lg cursor-pointer transition-all duration-200 group", selectedRoles.has(role) ? "bg-[#50B78B]/10 border border-[#50B78B]/30 shadow-sm" : "bg-muted/30 hover:bg-muted/60 border border-transparent" )} > <Checkbox id={`role-${role}`} checked={selectedRoles.has(role)} onCheckedChange={() => toggleRole(role)} className={cn( "data-[state=checked]:bg-[#50B78B] data-[state=checked]:border-[#50B78B] border-2", "transition-all duration-200" )} /> <span className={cn( "text-sm font-medium flex-1 transition-colors", selectedRoles.has(role) ? "text-[#50B78B]" : "text-foreground group-hover:text-[#50B78B]" )}> {role} </span> </label> - ))} - </div> + ))} </div>
🧹 Nitpick comments (2)
components/Leaderboard/LeaderboardView.tsx (2)
987-1057: Consider extracting pagination page-range logic.The IIFE calculating which page numbers to display (lines 987-1057) is correct but complex. Extracting it into a named helper function would improve testability and readability.
Example refactor
function calculateVisiblePages(currentPage: number, totalPages: number): number[] { const pages: number[] = []; if (totalPages <= 7) { for (let i = 1; i <= totalPages; i++) { pages.push(i); } } else { pages.push(1); if (currentPage <= 4) { for (let i = 2; i <= 5; i++) { pages.push(i); } pages.push(-1); pages.push(totalPages); } else if (currentPage >= totalPages - 3) { pages.push(-1); for (let i = totalPages - 4; i <= totalPages; i++) { pages.push(i); } } else { pages.push(-1); pages.push(currentPage - 1); pages.push(currentPage); pages.push(currentPage + 1); pages.push(-1); pages.push(totalPages); } } return pages; }Then use:
{calculateVisiblePages(currentPage, totalPages).map((pageNum, idx) => { // ... existing render logic })}
505-505: Consider centralizing the brand color.The color
#50B78Bappears over 20 times throughout this file. Consider defining it as a CSS variable or adding it to your Tailwind theme configuration for easier maintenance.Example approach
In
tailwind.config.js:module.exports = { theme: { extend: { colors: { 'brand-primary': '#50B78B', } } } }Then replace instances like
text-[#50B78B]withtext-brand-primary.Also applies to: 538-539, 546-546, 628-632, 748-748, 1045-1046
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/Leaderboard/LeaderboardView.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
components/Leaderboard/LeaderboardView.tsx (3)
components/PointsInfoButton.tsx (1)
PointsInfoButton(104-201)lib/leaderboard.ts (1)
SortBy(3-3)lib/utils.ts (1)
cn(5-7)
🔇 Additional comments (5)
components/Leaderboard/LeaderboardView.tsx (5)
39-39: LGTM: Clean integration of PointsInfoButton.The import and inline rendering of the PointsInfoButton component within the leaderboard header is well-placed and maintains semantic structure.
Also applies to: 508-508
83-91: Improved type safety.The explicit type annotation for
activityStylesenhances code clarity and type safety without altering functionality.
501-511: Verify responsive behavior on mid-sized viewports.The PR comments repeatedly mention that the tooltip is not responsive on mid-sized viewports. While the
overflow-x-autopattern withwhitespace-nowrapon small screens is a valid approach for long headings, please test thoroughly on tablet/mid-range breakpoints (768px–1024px) to ensure the info button and tooltip remain accessible and don't cause layout issues.Based on PR comments from reviewers.
119-127: LGTM: Formatting improvement.The explicit return wrapping is a stylistic improvement with no functional change.
598-649: Excellent refactor of Sort By UI.The explicit button-based approach with clear option definitions improves maintainability. The state management properly coordinates popover closure, URL updates, and pagination reset. Good use of
aria-pressedfor accessibility.
3 participants
Description
This PR adds a small info (ⓘ) tooltip next to the leaderboard title to clearly explain how contribution points are calculated.
The leaderboard was already correctly sorted by total contribution points, but the lack of visible scoring rules caused confusion when individual metrics (PRs opened, issues opened) appeared higher for lower-ranked contributors. This change improves clarity without altering any existing logic.
Related Issue
Fixes #42
Type of change
Checklist
Screenshots
Summary by CodeRabbit
New Features
Improvements
Accessibility
✏️ Tip: You can customize this high-level summary in your review settings.