fix: month label overlapping#216
Conversation
|
@abhix4 is attempting to deploy a commit to the ossdotnow Team on Vercel. A member of the Team first needs to authorize it. |
|
Caution Review failedThe pull request is closed. WalkthroughThe ContributionGraph component now handles TRPC errors, loading, and empty states; normalizes date parsing; rebuilds a 7-row, padded and width-normalized contribution grid; refines month-header generation (emit only for spans ≥2 weeks); stabilizes render keys and uses inline positioning styles. Public signature unchanged. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CG as ContributionGraph
participant TRPC as TRPC useQuery
User->>CG: Mount
CG->>TRPC: fetch contribution data
alt isLoading
TRPC-->>CG: { isLoading: true }
CG-->>User: Render Loading Card
else error
TRPC-->>CG: { error }
CG-->>User: Render Error Card
else success
TRPC-->>CG: { data: { totalContributions, days[] } }
alt no days / empty
CG-->>User: Render Empty Data Card
else has days
CG->>CG: Normalize dates to local midnight, build 7-row grid, pad weeks, normalize row widths
CG->>CG: Compute month headers, emit only if colspan ≥ 2
CG-->>User: Render Table with month headers, cells (titles via toLocaleDateString), legend
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
apps/web/components/user/contribution-graph.tsx (7)
21-26: UseisErrorinstead of truthyerrorchecks; render error details when present.React Query exposes a boolean
isError. Relying onerrortruthiness is fragile and may miss states. Also, consider surfacing a short message for debugging.Apply this diff:
- const { data, isLoading, error } = useQuery( + const { data, isLoading, isError, error } = useQuery( trpc.profile.getUserContributions.queryOptions({ username, provider, }), );- if (error) { + if (isError) { return ( <Card className="rounded-none border-neutral-800 bg-neutral-900/50 p-0"> <CardContent className="p-4"> <div className="flex items-center justify-center py-8"> - <div className="text-red-400">Failed to load contribution data</div> + <div className="text-red-400"> + Failed to load contribution data{error instanceof Error ? `: ${error.message}` : ''} + </div> </div> </CardContent> </Card> ); }Also applies to: 176-186
54-58: Grid construction works, but ensure deterministic ordering and remove unnecessary optional chaining.
- If the API ever returns days out of order, week grouping can skew. Sort defensively.
grid[dayOfWeek]is always defined (0–6), so?.is unnecessary and hides out-of-range bugs.Apply this diff to simplify the push:
- grid[i]?.push(null); + grid[i].push(null);- grid[dayOfWeek]?.push(day); + grid[dayOfWeek].push(day);And consider sorting contributions before using them (outside these lines):
// just before building the grid const contributionsSorted = [...contributions].sort( (a, b) => a.date.getTime() - b.date.getTime(), ); // then iterate contributionsSortedIf you’d like, I can provide a minimal patch wiring this into getContributionGrid with useMemo to avoid recomputation.
Also applies to: 64-67, 69-73, 75-81
107-151: Month header logic reduces overlap, but labels can lag when a month starts mid-week.You pick the month for a week from the first non-null day in that column. If the 1st falls on Thu, the week’s first non-null is Sun/Mon of the previous month, so the “new month” label shifts to the following week. GitHub-style calendars label the column that contains the 1st of the month.
Consider this refinement:
- For each week, scan all 7 days; if any day has date-of-month === 1, set
monthForWeekto that day’s month; otherwise fall back to the earliest day in that week (your current behavior).- Keep the
colspan >= 2rule to avoid tiny headers.I can provide a precise diff for
getMonthColumns()if you want to align labeling with GitHub while keeping the overlap fix. Please verify visually for months starting on Tue–Fri.
223-239: Use semantic table headers for accessibility (th+scope).Header cells are rendered as
td. For assistive tech, usethwith appropriatescope. Keep the absolute-positioned month label as-is.Apply this diff:
- <tr style={{ height: '13px' }}> - <td style={{ width: '28px' }}> + <tr style={{ height: '13px' }}> + <th scope="col" style={{ width: '28px' }}> <span className="sr-only">Day of Week</span> - </td> + </th> {getMonthColumns().map((col) => ( - <td + <th key={`month-${col.index}`} className="relative text-xs text-neutral-400" colSpan={col.colspan} > <span className="sr-only">{col.month}</span> <span aria-hidden="true" style={{ position: 'absolute', top: 0, left: '3px' }}> {col.month} </span> - </td> + </th> ))} </tr>Row headers for weekdays are fine as data cells with sr-only labels, but you could also promote them to
th scope="row"if you want full semantics.Also applies to: 243-255
229-237: Stable keys for month columns are fine but can be strengthened.
indexworks, but a structural key avoids remounts if the month grouping changes. Consider using a composite like${month}-${startWeek}if you storestartWeek, or${month}-${col.colspan}-${col.index}.No change required for this PR; just a future-proofing note.
102-105: Minor readability: simplifyweeksderivation.Current code is correct; you can simplify to use optional chaining and nullish coalescing.
Suggested tweak (outside this hunk):
const weeks = grid[0]?.length ?? 0;
50-85: Memoize derived structures to avoid recomputation on re-renders.
getContributionGrid()andgetMonthColumns()are O(N) over ~365 items. Cheap, but memoization prevents churn on parent re-renders.Outside this hunk:
import { useMemo } from 'react'; const grid = useMemo(getContributionGrid, [contributions]); const monthCols = useMemo(getMonthColumns, [grid]); // then use `monthCols` in render
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/web/components/user/contribution-graph.tsx(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/components/user/contribution-graph.tsx (2)
packages/api/src/driver/types.ts (1)
ContributionDay(192-202)packages/ui/src/lib/utils.ts (1)
cn(4-6)
🔇 Additional comments (3)
apps/web/components/user/contribution-graph.tsx (3)
188-201: Empty state UX is clear and consistent with the loading/error cards.Good fallback when there’s no data. Title and structure match the other states.
260-265: Cell keys and styles look good; classes map 1:1 with level indices.Assuming
rounded-xsexists in your Tailwind config. If not, replace withrounded-[2px]or an available radius token.If you’re unsure about
rounded-xs, please confirm your Tailwind theme defines it.
279-283: Legend rendering is simple and correct.Clear mapping from intensity to color, with stable keys.
|
Is it responsive also ? |
no bro. @aysahoo |
okay okay no issues . |
|
please check @aysahoo |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/web/components/user/contribution-graph.tsx (1)
29-31: Timezone drift fixed correctly by parsing at local midnight.This change eliminates UTC→local off-by-one issues and makes weekday/month calculations stable. Nicely done.
🧹 Nitpick comments (5)
apps/web/components/user/contribution-graph.tsx (5)
55-59: Make grid construction robust to input ordering by sorting dates first.The grid logic assumes
contributionsare in ascending date order. If the backend ever changes ordering, weeks/headers will misalign. Sort locally to harden.Apply this diff within
getContributionGrid:const getContributionGrid = () => { - const grid: (ContributionDay | null)[][] = []; - if (contributions.length === 0) return grid; + const grid: (ContributionDay | null)[][] = []; + if (contributions.length === 0) return grid; + + // Ensure deterministic week layout even if API ordering changes. + const days = [...contributions].sort( + (a, b) => a.date.getTime() - b.date.getTime(), + ); // Initialize 7 rows for days of week for (let i = 0; i < 7; i++) { grid.push([]); } - const firstDay = contributions[0]; + const firstDay = days[0]; if (!firstDay) return grid; const firstDayOfWeek = firstDay.date.getDay(); // Add null padding at the beginning for the first week for (let i = 0; i < firstDayOfWeek; i++) { grid[i]?.push(null); } // Add all contributions to the grid - contributions.forEach((day) => { + days.forEach((day) => { const dayOfWeek = day.date.getDay(); grid[dayOfWeek]?.push(day); });Also applies to: 60-67, 71-74, 76-83
106-109: Month label overlap fix looks good; consider using stable keys and anchoring keys to start week.
- The ≥2-week rule should eliminate cramped month labels. Good trade-off.
- Keys based on incremental indices can reshuffle on data refresh. Using the month segment’s starting week yields stable keys across renders.
Apply this diff (propagate the startWeek as the key):
const getMonthColumns = () => { - const monthCols: { month: string; colspan: number; index: number }[] = []; + const monthCols: { month: string; colspan: number; startWeek: number }[] = []; if (weeks === 0) return monthCols; let currentMonth = -1; let startWeek = 0; for (let week = 0; week < weeks; week++) { let monthForWeek = -1; // Find the first valid contribution in this week for (let day = 0; day < 7; day++) { const contribution = grid[day]?.[week]; if (contribution) { monthForWeek = contribution.date.getMonth(); break; } } if (monthForWeek !== -1 && monthForWeek !== currentMonth) { // Close the previous month if it has enough weeks to display if (currentMonth !== -1) { const colspan = week - startWeek; - if (colspan >= 2) { // Only show months with at least 2 weeks + if (colspan >= 2) { // Only show months with at least 2 weeks monthCols.push({ month: months[currentMonth] || '', - colspan: colspan, - index: monthCols.length, + colspan: colspan, + startWeek, }); } } currentMonth = monthForWeek; startWeek = week; } } // Close the last month if it has enough weeks if (currentMonth !== -1) { const colspan = weeks - startWeek; - if (colspan >= 2) { // Only show months with at least 2 weeks + if (colspan >= 2) { // Only show months with at least 2 weeks monthCols.push({ month: months[currentMonth] || '', - colspan: colspan, - index: monthCols.length, + colspan: colspan, + startWeek, }); } }And in the render:
- {getMonthColumns().map((col) => ( + {getMonthColumns().map((col) => ( <td - key={`month-${col.index}`} + key={`month-${col.startWeek}-${col.month}`} className="relative text-xs text-neutral-400" colSpan={col.colspan} >Also applies to: 116-123, 125-136, 142-151, 230-236
216-221: Use native table semantics for better accessibility (th/scope, remove ARIA grid roles).You’re using a semantic table; avoid overriding with grid roles and prefer
thwith properscopefor headers. This improves screen reader navigation.Apply these diffs:
- <table - className="border-separate" - style={{ borderSpacing: '3px' }} - role="grid" - aria-readonly="true" - > + <table + className="border-separate" + style={{ borderSpacing: '3px' }} + >- <tr style={{ height: '13px' }}> - <td style={{ width: '28px' }}> + <tr style={{ height: '13px' }}> + <th scope="col" style={{ width: '28px' }}> <span className="sr-only">Day of Week</span> - </td> + </th> {getMonthColumns().map((col) => ( - <td + <th key={`month-${col.index}`} className="relative text-xs text-neutral-400" colSpan={col.colspan} > <span className="sr-only">{col.month}</span> <span aria-hidden="true" style={{ position: 'absolute', top: 0, left: '3px' }}> {col.month} </span> - </td> + </th> ))} </tr>- <tr key={`day-${dayIndex}`} style={{ height: '10px' }}> - <td + <tr key={`day-${dayIndex}`} style={{ height: '10px' }}> + <th + scope="row" className="relative pr-1 text-right text-xs text-neutral-400" style={{ width: '28px' }} > <span className="sr-only">{dayName}</span> <span aria-hidden="true" className={cn('absolute right-1', dayIndex % 2 === 0 && 'opacity-0')} > {dayName.slice(0, 3)} </span> - </td> + </th>Also consider removing
role="gridcell"/aria-selectedfrom data cells; they’re redundant with table semantics:- role="gridcell" - aria-selected="false"Also applies to: 224-233, 245-256, 260-275
177-187: Add a retry action and show a safe error hint.Great to have an error path. Provide a “Retry” button using
refetch()and avoid exposing internal error details in the UI.Apply this diff:
- const { data, isLoading, error } = useQuery( + const { data, isLoading, error, refetch } = useQuery( trpc.profile.getUserContributions.queryOptions({ username, provider, }), );if (error) { return ( <Card className="rounded-none border-neutral-800 bg-neutral-900/50 p-0"> <CardContent className="p-4"> <div className="flex items-center justify-center py-8"> - <div className="text-red-400">Failed to load contribution data</div> + <div className="flex items-center gap-3"> + <div className="text-red-400">Failed to load contribution data</div> + <button + type="button" + onClick={() => refetch()} + className="rounded bg-neutral-800 px-2 py-1 text-xs text-neutral-200 hover:bg-neutral-700" + > + Retry + </button> + </div> </div> </CardContent> </Card> ); }Also applies to: 21-26
260-271: Expose accessible labels for cells (title is not read by all screen readers).Add
aria-labelthat mirrors the tooltip and drop the grid roles as suggested above.Apply this diff:
- <td + <td key={`cell-${dayIndex}-${weekIndex}`} className={cn( 'h-[10px] w-[10px] rounded-xs', day ? levelColors[day.level] : '', )} style={{ width: '10px' }} - title={ - day - ? `${day.count} contributions on ${day.date.toLocaleDateString(undefined, { weekday: 'short', year: 'numeric', month: 'short', day: 'numeric' })}` - : '' - } - role="gridcell" - aria-selected="false" + title={ + day + ? `${day.count} contributions on ${day.date.toLocaleDateString(undefined, { weekday: 'short', year: 'numeric', month: 'short', day: 'numeric' })}` + : '' + } + aria-label={ + day + ? `${day.count} contributions on ${day.date.toLocaleDateString(undefined, { weekday: 'long', year: 'numeric', month: 'long', day: 'numeric' })}` + : 'No contribution data' + } />Also applies to: 272-275
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/web/components/user/contribution-graph.tsx(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/components/user/contribution-graph.tsx (2)
packages/api/src/driver/types.ts (1)
ContributionDay(192-202)packages/ui/src/lib/utils.ts (1)
cn(4-6)
3 participants
Description
Type of Change
Testing
Checklist
Screenshots (if applicable)
before:
after:
Please ensure your PR title clearly describes the change.
Summary by CodeRabbit
New Features
Bug Fixes
Style