Unify user summary card #2208
Unify user summary card #2208
Conversation
Summary by CodeRabbit
WalkthroughRemoved the DetailsCard heatmap section and prop, moved heatmap generation into a local Heatmap component inside UserSummary with responsive visibility, adjusted avatar/layout sizing, and updated tests to remove heatmap assertions and reflect the new layout. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (4 passed, 1 inconclusive)❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (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. 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/components/CardDetailsPage.tsx (1)
1-10: Remove unusedheatmapprop andfaSquarePollVerticalimport
- In
frontend/src/components/CardDetailsPage.tsx, deletefaSquarePollVerticalfrom the icon import and removeheatmapfrom the destructured props.- In
frontend/src/types/card.ts, remove theheatmap?: JSX.Elementproperty from theCardPropsinterface.--- frontend/src/components/CardDetailsPage.tsx @@ -import { - faCircleInfo, - faSquarePollVertical, - faChartPie, - faFolderOpen, - faCode, - faTags, - faUsers, - faRectangleList, -} from '@fortawesome/free-solid-svg-icons' +import { + faCircleInfo, + faChartPie, + faFolderOpen, + faCode, + faTags, + faUsers, + faRectangleList, +} from '@fortawesome/free-solid-svg-icons' @@ - geolocationData = null, - healthMetricsData, - heatmap, + geolocationData = null, + healthMetricsData, isActive = true,--- frontend/src/types/card.ts @@ interface CardProps { - heatmap?: JSX.Element
🧹 Nitpick comments (3)
frontend/src/app/members/[memberKey]/page.tsx (3)
38-41: Remove dead heatmap code at page scope (unused canvasRef/theme/effect).This code never attaches a canvas to DOM, so the draw never runs. Eliminating it reduces confusion and prevents API drift (it also used
themevsthemeName).Replace top-scope snippet with the version below (non-diff for out-of-range context):
// Remove: // const [imageLink, setImageLink] = useState('') // const canvasRef = useRef<HTMLCanvasElement | null>(null) // const theme = 'blue' // // useEffect(() => { // if (canvasRef.current && data && data.years && data.years.length > 0) { // drawContributions(canvasRef.current, { data, username, theme }) // const imageURL = canvasRef.current.toDataURL() // setImageLink(imageURL) // } // }, [username, data])Also applies to: 78-85
63-76: Avoid redundant heatmap fetches (trim effect deps).
fetchDatadoesn’t useuser; it will re-run wheneveruserchanges.- }, [memberKey, user]) + }, [memberKey])
197-219: UserSummary layout matches the spec; minor polish optional.
- Responsive 1/3 avatar + 2/3 summary/heatmap; heatmap hidden on small screens → compliant with #2167.
- Optional: since
user?.urlis external, consider<a>instead ofLink(Next recommends native anchors for external URLs).- <Link href={user?.url || '#'} className="text-xl font-bold text-blue-400 hover:underline"> + <a href={user?.url || '#'} target="_blank" rel="noopener noreferrer" + className="text-xl font-bold text-blue-400 hover:underline">
📜 Review details
Configuration used: Path: .coderabbit.yaml
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 (2)
frontend/src/app/members/[memberKey]/page.tsx(1 hunks)frontend/src/components/CardDetailsPage.tsx(1 hunks)
🔇 Additional comments (1)
frontend/src/components/CardDetailsPage.tsx (1)
119-123: Unified UserSummary card looks correct (no title/icon).Matches #2167: a single block with no title; leaves summary/other cards intact.
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 (2)
frontend/src/app/members/[memberKey]/page.tsx (2)
61-74: Fix: private contributor flag can get stuck true across navigationsWhen a private profile is viewed first,
isPrivateContributorstaystrueeven after navigating to a public profile because it’s never reset on success. Also,userisn’t used in this effect and causes unnecessary refetches. Add success reset and dropuserfrom deps; guard against stale async results.Apply:
-useEffect(() => { - const fetchData = async () => { - const result = await fetchHeatmapData(memberKey as string) - if (!result) { - setIsPrivateContributor(true) - return - } - if (result?.contributions) { - setUsername(memberKey as string) - setData(result as HeatmapData) - } - } - fetchData() -}, [memberKey, user]) +useEffect(() => { + let cancelled = false + const fetchData = async () => { + const result = await fetchHeatmapData(memberKey as string) + if (cancelled) return + if (result?.contributions) { + setIsPrivateContributor(false) + setUsername(memberKey as string) + setData(result as HeatmapData) + } else { + setIsPrivateContributor(true) + setUsername('') + setData({} as HeatmapData) + } + } + fetchData() + return () => { + cancelled = true + } +}, [memberKey])
41-59: Ensure error handling short-circuits before 404 renderIn frontend/src/app/members/[memberKey]/page.tsx (lines 41–59),
handleAppErroronly logs and shows a toast—it doesn’t throw or navigate—so execution falls through into theuser == nullbranch and renders a 404 UI for any GraphQL error. To prevent transient errors from appearing as “not found,” rethrow the error, return an error UI, or navigate to a proper error page immediately after callinghandleAppError.
♻️ Duplicate comments (1)
frontend/src/app/members/[memberKey]/page.tsx (1)
136-153: Heatmap now self-contained and reactive — good upgradeLocalizing
imgSrcand includingdata/username/isDarkModein deps resolves the previous stale-render/coupling issue.
🧹 Nitpick comments (3)
frontend/src/app/members/[memberKey]/page.tsx (3)
79-99: Tighten @mention regex to valid GitHub usernames (no dots)Current pattern allows
@alice.bob, which generates broken links. Restrict to[A-Za-z0-9-]and forbid leading/trailing hyphen; keep trailing punctuation capture.Apply:
-const mentionMatch = word.match(/^@([\w-]+(?:\.[\w-]+)*)([^\w@])?$/) +const mentionMatch = word.match( + /^@([A-Za-z0-9](?:[A-Za-z0-9-]{0,37}[A-Za-z0-9])?)([^\w@])?$/ +)
147-152: Reduce main-thread work: prefertoBlob+ object URL overtoDataURL
toDataURL()base64-encodes the bitmap on every redraw. UsetoBlob()and manage a blob URL; revoke the previous one to avoid leaks.Apply:
-const imageURL = canvasRef.current.toDataURL() -setImgSrc(imageURL) +canvasRef.current.toBlob((blob) => { + if (!blob) { + setImgSrc('') + return + } + const url = URL.createObjectURL(blob) + setImgSrc((prev) => { + if (prev && prev.startsWith('blob:')) URL.revokeObjectURL(prev) + return url + }) +}, 'image/png')Optionally add cleanup at the end of this effect:
+return () => { + setImgSrc((prev) => { + if (prev && prev.startsWith('blob:')) URL.revokeObjectURL(prev) + return '' + }) +}
155-183: Improve loader semantics and image layout
- Loader center classes lack
flex; they currently don’t center.- Background is decorative; use
alt=""andaria-hiddento avoid noise for AT.- Consider
Imagewithfillto match the container and avoid distortion/CLS.Apply:
- {imgSrc ? ( - <div className="h-32"> - <Image - width={100} - height={100} - src={imgSrc} - className="h-full w-full object-cover object-[54%_60%]" - alt="Contribution Heatmap" - /> - </div> - ) : ( - <div className="relative h-32 items-center justify-center"> + {imgSrc ? ( + <div className="relative h-32"> + <Image + fill + sizes="(min-width: 640px) 66vw, 100vw" + src={imgSrc} + className="object-cover object-[54%_60%]" + alt={`Contribution heatmap for @${username}`} + /> + </div> + ) : ( + <div className="relative h-32 flex items-center justify-center"> <Image - height={100} - width={100} + height={100} + width={100} src={ isDarkMode ? '/img/heatmap-background-dark.png' : '/img/heatmap-background-light.png' } - className="heatmap-background-loader h-full w-full border-none object-cover object-[54%_60%]" - alt="Heatmap Background" + className="heatmap-background-loader h-full w-full border-none object-cover object-[54%_60%]" + alt="" + aria-hidden="true" /> <div className="heatmap-loader"></div> </div> )}
📜 Review details
Configuration used: Path: .coderabbit.yaml
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 (2)
frontend/src/app/members/[memberKey]/page.tsx(2 hunks)frontend/src/components/CardDetailsPage.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/components/CardDetailsPage.tsx
🔇 Additional comments (1)
frontend/src/app/members/[memberKey]/page.tsx (1)
190-211: Unified summary + responsive heatmap meets the PR objectivesTwo-column layout with avatar left and heatmap hidden on small screens is correctly implemented.
|
Hy @kasya I have combined the user summary and the contribution heat map into a single block could you please take a look and let me know if any further changes are needed or should I start writing tests for these updates? |
@anurag2787 Looks good so far! Could we move the avatar to the left and use the rest of the space to make the map and make it a bit larger (250x250)? Would be good to have the map a bit larger... 🤔 We also need to start hiding the map earlier - if you look closely on screen size change the heat map image gets cut off. Also, can we make the avatar round? These are just some initial thoughts! Basically, I just don’t want any space to go to waste - I’d like us to use it as best as we can. |
anurag2787
force-pushed
the
enhancement/unifiedSummaryCard
branch
from
6f03ebd to
16c53e0
Compare
|
Hy @kasya I have updated as per your suggestion can you please review Thanks! 
|
|
@anurag2787 this is great! 👍🏼 And if so, you can start working on the tests 👍🏼 thanks! |
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 (3)
frontend/__tests__/unit/pages/UserDetails.test.tsx (3)
29-33: Fix params mock: route param is memberKey, not userKeyThis currently masks bugs because memberKey is read as undefined in the component.
- useRouter: jest.fn(() => mockRouter), - useParams: () => ({ userKey: 'test-user' }), + useRouter: jest.fn(() => mockRouter), + useParams: () => ({ memberKey: 'test-user' }),
52-55: Align heatmap mock shape with real APIfetchHeatmapData returns { years: Year[], contributions: HeatmapContribution[] } (not nested).
-;(fetchHeatmapData as jest.Mock).mockResolvedValue({ - contributions: { years: [{ year: '2023' }] }, -}) +;(fetchHeatmapData as jest.Mock).mockResolvedValue({ + years: [{ year: '2024' }], + contributions: [], +})
231-252: Test the rendered heatmap image and stub canvas.toDataURLWithout stubbing, HTMLCanvasElement.toDataURL may throw in JSDOM; also the current assertion looks for the fallback background instead of the generated image.
-;(fetchHeatmapData as jest.Mock).mockResolvedValue({ - years: [{ year: '2023' }], // Provide years data to satisfy condition in component -}) +;(fetchHeatmapData as jest.Mock).mockResolvedValue({ + years: [{ year: '2023' }], + contributions: [], +}) ... -// Wait for useEffect to process the fetchHeatmapData result -await waitFor(() => { - const heatmapContainer = screen - .getByAltText('Heatmap Background') - .closest('div.hidden.lg\\:block') - expect(heatmapContainer).toBeInTheDocument() - expect(heatmapContainer).toHaveClass('hidden') - expect(heatmapContainer).toHaveClass('lg:block') -}) +// Stub canvas serialization and assert the generated image is used +Object.defineProperty(HTMLCanvasElement.prototype, 'toDataURL', { + configurable: true, + value: () => 'data:image/png;base64,stub', +}) +await waitFor(() => { + const container = document.querySelector('div.hidden.lg\\:block') + expect(container).toBeInTheDocument() + expect(screen.getByAltText('Contribution Heatmap')).toBeInTheDocument() +})
♻️ Duplicate comments (1)
frontend/src/app/members/[memberKey]/page.tsx (1)
138-151: Heatmap never redraws on data/username change and may crash in tests (canvas.toDataURL)Effect only depends on isDarkMode; add data/username and guard canvas serialization.
- useEffect(() => { - if (canvasRef.current && data?.years?.length) { + useEffect(() => { + if (canvasRef.current && data?.years?.length) { drawContributions(canvasRef.current, { data, username, themeName: isDarkMode ? 'dark' : 'light', }) - const imageURL = canvasRef.current.toDataURL() - setImgSrc(imageURL) + try { + const imageURL = canvasRef.current.toDataURL() + setImgSrc(imageURL) + } catch { + setImgSrc('') + } } else { setImgSrc('') } - }, [isDarkMode]) + }, [isDarkMode, data, username])
🧹 Nitpick comments (5)
frontend/__tests__/unit/components/CardDetailsPage.test.tsx (1)
701-705: Duplicate “supportedTypes” blocksDefine once and reuse to avoid drift between lists.
- const supportedTypes = ['project', 'repository', 'committee', 'user', 'organization'] + const supportedTypes = ['project', 'repository', 'committee', 'user', 'organization'] ... - const supportedTypes = ['project', 'repository', 'committee', 'user', 'organization'] + // reuse existing supportedTypesAlso applies to: 957-961
frontend/src/components/CardDetailsPage.tsx (2)
146-168: Stats mapping keyUsing array index as key is fine here, but a stable key (e.g., pluralizedName+unit) reduces reorder churn.
- {stats.map((stat, index) => ( - <div key={index}> + {stats.map((stat) => ( + <div key={`${stat.pluralizedName ?? stat.unit}-${stat.value}`}>
306-327: Type SocialLinks props and harden URLsAdd typing and early filter to avoid rendering empty/invalid strings.
-export const SocialLinks = ({ urls }) => { +export const SocialLinks = ({ urls }: { urls: string[] }) => { - if (!urls || urls.length === 0) return null + const safe = Array.isArray(urls) ? urls.filter(Boolean) : [] + if (safe.length === 0) return null return ( <div> <strong>Social Links</strong> <div className="mt-2 flex flex-wrap gap-3"> - {urls.map((url, index) => ( + {safe.map((url, index) => (frontend/src/app/members/[memberKey]/page.tsx (2)
60-73: Unnecessary re-fetch on user changesThis effect depends on user; it will refetch when user state updates. Scope it to memberKey and reset private-contributor flag on success.
-useEffect(() => { +useEffect(() => { const fetchData = async () => { const result = await fetchHeatmapData(memberKey as string) if (!result) { setIsPrivateContributor(true) return } if (result?.contributions) { setUsername(memberKey as string) setData(result as HeatmapData) + setIsPrivateContributor(false) } } fetchData() -}, [memberKey, user]) +}, [memberKey])
75-97: GitHub mention regex is too permissive (allows dots) and misses edge rulesGitHub usernames: 1–39 chars, alphanumeric or hyphen, no leading/trailing hyphen.
-const mentionMatch = word.match(/^@([\w-]+(?:\.[\w-]+)*)([^\w@])?$/) +const mentionMatch = word.match(/^@([a-z\d](?:[a-z\d-]{0,37}[a-z\d])?)([^\w@])?$/i)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/__tests__/unit/components/CardDetailsPage.test.tsx(2 hunks)frontend/__tests__/unit/pages/UserDetails.test.tsx(3 hunks)frontend/src/app/members/[memberKey]/page.tsx(2 hunks)frontend/src/components/CardDetailsPage.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-06-20T16:12:59.256Z
Learnt from: ahmedxgouda
PR: OWASP/Nest#1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a safety check that ensures HealthMetrics component is only rendered when healthMetricsData exists and has at least one element: `healthMetricsData && healthMetricsData.length > 0`. This makes accessing data[0] safe within the HealthMetrics component.
Applied to files:
frontend/__tests__/unit/components/CardDetailsPage.test.tsx
📚 Learning: 2025-06-20T16:12:59.256Z
Learnt from: ahmedxgouda
PR: OWASP/Nest#1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a length check before rendering HealthMetrics: `healthMetricsData.length > 0`. This ensures that when HealthMetrics is rendered, the data array has at least one element, making accessing data[0] safe within the HealthMetrics component.
Applied to files:
frontend/__tests__/unit/components/CardDetailsPage.test.tsx
🧬 Code graph analysis (1)
frontend/__tests__/unit/pages/UserDetails.test.tsx (1)
frontend/src/utils/helpers/githubHeatmap.ts (1)
fetchHeatmapData(40-89)
🔇 Additional comments (5)
frontend/__tests__/unit/components/CardDetailsPage.test.tsx (1)
637-639: Good simplification of userSummary assertionAsserting for the presence of supplied content is sufficient now that the wrapper title/icon were removed.
frontend/__tests__/unit/pages/UserDetails.test.tsx (1)
382-386: Verify empty-state copy for “Recent Issues”The test expects “No recent releases.” under the “Recent Issues” section. Please confirm the intended copy or adjust the assertion/component.
Would you like me to scan the codebase for the actual empty-state strings and open a follow-up PR if needed?
frontend/src/components/CardDetailsPage.tsx (2)
117-118: User summary as content-only card looks goodWrapper matches the unified card direction; no title/icon is appropriate per spec.
186-200: Undefined guard for languages/topics (only if optional in type)If DetailsCardProps allows undefined for languages/topics, these length checks will throw. If they are required, ignore this comment.
If optional, add defaults in the props destructure: languages = [], topics = [].
frontend/src/app/members/[memberKey]/page.tsx (1)
187-211: Responsive layout matches designAvatar sizing, rounded styling, and lg-only heatmap visibility align with the unified card spec.
|
Hy, I have fixed this according to the suggestion given. |
|
* Maked Unified UserSummaryCard * Removed unused code * fixed code * updated the heatmap to bigger size * updated size of heatmap * added test * fix --------- Co-authored-by: Arkadii Yakovets <[email protected]>
Resolves #2167
Description
This PR combines the User summary block and the contribution heat map into a single block.
Key Changes
Checklist
make check-testlocally; all checks and tests passed.