🧹 [Code Health] Refactor cardRenderer.tsx to extract options and elements#127
🧹 [Code Health] Refactor cardRenderer.tsx to extract options and elements#127
Conversation
Co-authored-by: is0692vs <135803462+is0692vs@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 10 minutes and 17 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
✨ 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 |
Review Summary by QodoRefactor cardRenderer.tsx into modular options and elements files
WalkthroughsDescription• Extracted options parsing logic into dedicated cardOptions.ts file • Separated React UI components into new cardElements.tsx file • Reduced cardRenderer.tsx from 680+ lines to focused rendering logic • Maintained backward compatibility via re-exports in main file Diagramflowchart LR
CR["cardRenderer.tsx<br/>680+ lines"]
CO["cardOptions.ts<br/>types & parsing"]
CE["cardElements.tsx<br/>React components"]
CR -- "extract types<br/>& parseCardQueryParams" --> CO
CR -- "extract UI logic<br/>& themes" --> CE
CR -- "re-export from<br/>cardOptions" --> CO
CR -- "import from<br/>cardElements" --> CE
File Changes1. src/lib/cardOptions.ts
|
Code Review by Qodo🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)
Great, no issues found!Qodo reviewed your code and found no material issues that require reviewⓘ The new review experience is currently in Beta. Learn more |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the card rendering logic by decomposing the monolithic cardRenderer.tsx into specialized modules: cardElements.tsx for UI components and cardOptions.ts for configuration parsing and layout logic. The feedback focuses on further improving code maintainability and performance. Specifically, it is suggested to refactor the large createBlock function into smaller, dedicated components using a component map and to utilize a Set for more efficient uniqueness checks when parsing requested blocks.
src/lib/cardElements.tsx
Outdated
| export function createBlock( | ||
| block: CardBlockType, | ||
| data: CardData, | ||
| theme: ThemePalette, | ||
| hide: Set<string>, | ||
| ): ReactElement { | ||
| if (block === "bio") { | ||
| return ( | ||
| <div | ||
| style={{ | ||
| display: "flex", | ||
| flexDirection: "row", | ||
| gap: 14, | ||
| alignItems: "center", | ||
| }} | ||
| > | ||
| {/* eslint-disable-next-line @next/next/no-img-element */} | ||
| <img | ||
| src={data.profile.avatarUrl} | ||
| width={58} | ||
| height={58} | ||
| style={{ borderRadius: 999, border: `2px solid ${theme.border}` }} | ||
| alt="avatar" | ||
| /> | ||
| <div style={{ display: "flex", flexDirection: "column", gap: 2 }}> | ||
| <div style={{ color: theme.text, fontSize: 22, fontWeight: 700 }}> | ||
| {data.profile.name} | ||
| </div> | ||
| <div style={{ color: theme.subtext, fontSize: 14 }}> | ||
| @{data.profile.login} | ||
| </div> | ||
| {data.profile.bio ? ( | ||
| <div style={{ color: theme.subtext, fontSize: 13, maxWidth: 470 }}> | ||
| {data.profile.bio.length > 110 | ||
| ? `${data.profile.bio.slice(0, 110)}...` | ||
| : data.profile.bio} | ||
| </div> | ||
| ) : null} | ||
| </div> | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| if (block === "stats") { | ||
| return ( | ||
| <div style={{ display: "flex", flexDirection: "column", gap: 8 }}> | ||
| <div style={{ color: theme.text, fontSize: 16, fontWeight: 700 }}> | ||
| Stats | ||
| </div> | ||
| <div | ||
| style={{ | ||
| display: "flex", | ||
| flexDirection: "row", | ||
| gap: 14, | ||
| color: theme.subtext, | ||
| fontSize: 14, | ||
| }} | ||
| > | ||
| <div>Followers: {data.profile.followers.toLocaleString()}</div> | ||
| <div>Following: {data.profile.following.toLocaleString()}</div> | ||
| <div>Repos: {data.profile.publicRepos.toLocaleString()}</div> | ||
| {!hide.has("stars") ? ( | ||
| <div>Stars: {data.totalStars.toLocaleString()}</div> | ||
| ) : null} | ||
| </div> | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| if (block === "langs") { | ||
| return ( | ||
| <div style={{ display: "flex", flexDirection: "column", gap: 7 }}> | ||
| <div style={{ color: theme.text, fontSize: 16, fontWeight: 700 }}> | ||
| Top Languages | ||
| </div> | ||
| {data.languages.slice(0, 4).map((lang) => ( | ||
| <div | ||
| key={lang.name} | ||
| style={{ | ||
| display: "flex", | ||
| flexDirection: "row", | ||
| justifyContent: "space-between", | ||
| fontSize: 13, | ||
| color: theme.subtext, | ||
| }} | ||
| > | ||
| <span>{lang.name}</span> | ||
| <span>{lang.percentage.toFixed(1)}%</span> | ||
| </div> | ||
| ))} | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| if (block === "repos") { | ||
| return ( | ||
| <div style={{ display: "flex", flexDirection: "column", gap: 7 }}> | ||
| <div style={{ color: theme.text, fontSize: 16, fontWeight: 700 }}> | ||
| Top Repositories | ||
| </div> | ||
| {data.repos.slice(0, 3).map((repo) => ( | ||
| <div | ||
| key={repo.name} | ||
| style={{ | ||
| display: "flex", | ||
| flexDirection: "row", | ||
| justifyContent: "space-between", | ||
| fontSize: 13, | ||
| color: theme.subtext, | ||
| }} | ||
| > | ||
| <span>{repo.name}</span> | ||
| <span> | ||
| {!hide.has("stars") ? `★${repo.stars}` : ""} | ||
| {!hide.has("stars") && !hide.has("forks") ? " / " : ""} | ||
| {!hide.has("forks") ? `⑂${repo.forks}` : ""} | ||
| </span> | ||
| </div> | ||
| ))} | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| if (block === "streak") { | ||
| return ( | ||
| <div style={{ display: "flex", flexDirection: "column", gap: 8 }}> | ||
| <div style={{ color: theme.text, fontSize: 16, fontWeight: 700 }}> | ||
| Streak | ||
| </div> | ||
| <div | ||
| style={{ | ||
| display: "flex", | ||
| flexDirection: "row", | ||
| gap: 16, | ||
| color: theme.subtext, | ||
| fontSize: 13, | ||
| }} | ||
| > | ||
| <span>Current: {data.streak.current} days</span> | ||
| <span>Longest: {data.streak.longest} days</span> | ||
| </div> | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| const days = data.heatmap.days.slice(-42); | ||
| const columns: Array<Array<{ date: string; count: number }>> = []; | ||
| for (let i = 0; i < days.length; i += 7) { | ||
| columns.push(days.slice(i, i + 7)); | ||
| } | ||
|
|
||
| return ( | ||
| <div style={{ display: "flex", flexDirection: "column", gap: 8 }}> | ||
| <div style={{ color: theme.text, fontSize: 16, fontWeight: 700 }}> | ||
| Heatmap | ||
| </div> | ||
| <div style={{ display: "flex", flexDirection: "row", gap: 4 }}> | ||
| {columns.map((column, colIndex) => ( | ||
| <div | ||
| key={`col-${colIndex}`} | ||
| style={{ display: "flex", flexDirection: "column", gap: 4 }} | ||
| > | ||
| {column.map((day) => ( | ||
| <div | ||
| key={day.date} | ||
| style={{ | ||
| width: 10, | ||
| height: 10, | ||
| borderRadius: 2, | ||
| backgroundColor: levelColor( | ||
| day.count, | ||
| data.heatmap.maxCount, | ||
| theme, | ||
| ), | ||
| }} | ||
| /> | ||
| ))} | ||
| </div> | ||
| ))} | ||
| </div> | ||
| </div> | ||
| ); | ||
| } |
There was a problem hiding this comment.
The createBlock function is quite large (over 180 lines) and handles the rendering logic for all block types. This reduces readability and makes it harder to maintain.
A good refactoring would be to break this down into smaller, dedicated components for each block type (e.g., BioBlock, StatsBlock). You could then use a component map to render the correct block dynamically.
Additionally, the logic for the heatmap block is implicit. It's the fall-through case if no other if condition matches. It would be clearer to handle all block types explicitly, for example with a switch statement or a component map.
Here's an example of how you could use a component map:
type BlockProps = { data: CardData; theme: ThemePalette; hide: Set };
const BioBlock = ({ data, theme }: BlockProps) => { /* ... / };
const StatsBlock = ({ data, theme, hide }: BlockProps) => { / ... */ };
const blockComponents: Record<CardBlockType, React.FC> = {
bio: BioBlock,
stats: StatsBlock,
langs: LangsBlock,
repos: ReposBlock,
streak: StreakBlock,
heatmap: HeatmapBlock,
};
export function createBlock(block: CardBlockType, data: CardData, theme: ThemePalette, hide: Set): ReactElement {
const BlockComponent = blockComponents[block];
return ;
}
This approach makes the code more modular, easier to read, and easier to test.
| function parseBlocks(raw: string | null): CardBlockType[] { | ||
| const requested = toList(raw); | ||
| if (requested.length === 0) { | ||
| return DEFAULT_BLOCKS; | ||
| } | ||
|
|
||
| const unique: CardBlockType[] = []; | ||
| for (const block of requested) { | ||
| if (!VALID_BLOCKS.includes(block as CardBlockType)) { | ||
| continue; | ||
| } | ||
| if (!unique.includes(block as CardBlockType)) { | ||
| unique.push(block as CardBlockType); | ||
| } | ||
| } | ||
|
|
||
| return unique.length > 0 ? unique : DEFAULT_BLOCKS; | ||
| } |
There was a problem hiding this comment.
The current implementation for creating a unique list of blocks uses unique.includes() inside a loop. This has a time complexity of O(n^2) and can be inefficient. Using a Set is a more performant and idiomatic way to handle uniqueness in modern JavaScript.
function parseBlocks(raw: string | null): CardBlockType[] {
const requested = toList(raw);
if (requested.length === 0) {
return DEFAULT_BLOCKS;
}
const validBlocks = [...new Set(requested)].filter((block): block is CardBlockType =>
VALID_BLOCKS.includes(block as CardBlockType),
);
return validBlocks.length > 0 ? validBlocks : DEFAULT_BLOCKS;
}
|
Deployment failed with the following error: Learn More: https://vercel.com/hirokis-projects-afd618c7?upgradeToPro=build-rate-limit |
1 participant
🎯 What: Extracted code from
src/lib/cardRenderer.tsxinto two new files:src/lib/cardOptions.ts(for types and parsing URL query parameters) andsrc/lib/cardElements.tsx(for pure React UI components).💡 Why:
src/lib/cardRenderer.tsxwas a monolithic 680+ line file. By separating concerns, it is now much easier to navigate, test, and maintain. Options handling, UI logic, and Image generation (via Satori / Vercel OG) are clearly separated.✅ Verification: Verified by running the entire Vitest suite (
npm run test -- --run) which showed all tests passing (including specificallycardRenderer.test.tsandroute.test.ts), and also executednpx tsc --noEmitandnpm run lintwith no new errors.✨ Result: A more robust and readable codebase following standard single-responsibility principles without any breaking changes to consumers (thanks to
export * from "./cardOptions"in the maincardRenderer.tsxfile).PR created automatically by Jules for task 5519682949436094347 started by @is0692vs