Removed Nested componentes #3906
Conversation
WalkthroughPer-candidate UI and data orchestration were moved into a new top-level CandidateCard component; a reusable UserSummary component and SentryErrorFallback were added as top-level components; CardSkeleton rendering was simplified. Error capture was moved into effect hooks and per-card snapshot fetching was introduced. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 4 files
Confidence score: 4/5
- Overall risk feels low-to-moderate; issues are mostly quality/behavior concerns rather than likely breakage.
frontend/src/app/global-error.tsxtriggersSentry.captureExceptionduring render, which can cause repeated reporting on re-renders despite ErrorBoundary already handling it.frontend/src/components/SkeletonsBase.tsxduplicatesCardSkeletonProps, which can drift over time and add maintenance risk.- Pay close attention to
frontend/src/app/global-error.tsxandfrontend/src/components/SkeletonsBase.tsx- avoid repeated Sentry calls and type divergence.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="frontend/src/components/SkeletonsBase.tsx">
<violation number="1" location="frontend/src/components/SkeletonsBase.tsx:11">
P2: Duplicated and incomplete type: `CardSkeletonComponentProps` is a subset of the existing `CardSkeletonProps` from `types/skeleton`. Use the existing type to avoid divergence and remove duplication.
(Based on your team's feedback about using existing shared components and utilities.) [FEEDBACK_USED]</violation>
</file>
<file name="frontend/src/app/global-error.tsx">
<violation number="1" location="frontend/src/app/global-error.tsx:105">
P2: `Sentry.captureException` is called as a side effect during render, which means it fires on every re-render and is also redundant — Sentry's `ErrorBoundary` already captures the exception automatically via `componentDidCatch`. This results in duplicate error reports. Either remove this call entirely (relying on the boundary's built-in reporting), or wrap it in a `useEffect` to ensure it only fires once.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@frontend/src/app/board/`[year]/candidates/page.tsx:
- Around line 113-114: Update the misleading comment above the renderChannelLink
function: replace or reword "// Render a single repository link item" to
correctly describe that the function renders a channel link (e.g., "// Render a
single channel link item" or similar) so the comment matches the function name
renderChannelLink and its parameters channelName/messageCount.
- Around line 224-279: The sequential client.query calls in fetchChapters and
fetchProjects cause an N+1 latency issue; update both functions (fetchChapters
and fetchProjects in page.tsx) to run the per-key queries in parallel (use
Promise.all over the mapped client.query() promises and then collect
data.chapter / data.project results) or, if backend supports it, replace with a
single batched GraphQL query that accepts multiple keys; ensure you still filter
out missing results and call setLedChapters(sortByName(...)) and
setLedProjects(sortByName(...)) with the aggregated arrays.
- Around line 667-686: Remove the derived local state for candidates and the
effect that copies query results into it: in BoardCandidatesPage delete the
useState<CandidateWithSnapshot[]>([]) declaration and the useEffect that checks
graphQLData?.boardOfDirectors to call setCandidates; instead derive candidates
directly from the query with something like const candidates =
graphQLData?.boardOfDirectors?.candidates || []; keep a small useEffect (or
inline check) only to call handleAppError(graphQLRequestError) when
graphQLRequestError is present; references: BoardCandidatesPage,
useQuery(GetBoardCandidatesDocument), graphQLData.boardOfDirectors.candidates,
graphQLRequestError, handleAppError.
- Around line 210-221: The component is copying query results into local state
causing extra renders; remove the snapshot useState and the useEffect that sets
it from useQuery(GetMemberSnapshotDocument) and instead derive snapshot directly
from snapshotData?.memberSnapshot wherever snapshot is used; likewise in
BoardCandidatesPage remove the local candidates state and any effects that
populate it and reference graphQLData.boardOfDirectors.candidates directly
(update usages of snapshot, setSnapshot, candidates and their setters
accordingly).
- Around line 488-492: The contribution count logic uses || which treats 0 as
falsy; update the chapter contribution calculation (variables bareKey,
directKey, withPrefix, contributionCount, accessing
snapshot.chapterContributions) to use the nullish coalescing operator (??) so
that a legitimate 0 is preserved (i.e., choose directKey ?? withPrefix ?? 0).
Apply the same change to the project contributions block (the analogous
variables that read snapshot.projectContributions) so project counts also use ??
instead of ||.
In `@frontend/src/app/global-error.tsx`:
- Around line 101-107: Sentry.captureException is being called during render in
SentryErrorFallback which can cause duplicate reports; move the call into a
React effect so it runs once per mounted error: inside the SentryErrorFallback
component use useEffect to call Sentry.captureException(error instanceof Error ?
error : new Error(String(error))) with error in the dependency array, leaving
the return of <ErrorDisplay {...errorConfig} /> unchanged; reference
functions/values SentryErrorFallback, Sentry.captureException, useEffect,
ErrorDisplay, and ERROR_CONFIGS when making the change.
🧹 Nitpick comments (8)
frontend/src/app/members/[memberKey]/page.tsx (1)
19-80: ExtractedUserSummarylooks good — achieves the PR objective.The component is cleanly extracted with well-defined props. A couple of minor observations:
Line 52 — unnecessary
.slice():user.badges.slice().map(...)creates a shallow copy before.map(), but since no mutation (e.g.,.sort()) follows, the.slice()is a no-op and can be removed.
userprop typed asUser | null: At the call site (Line 210),useris guaranteed non-null due to the guard on Line 167. Consider narrowing the prop type toUserso the component doesn't need optional chaining everywhere.Suggested tightening
interface UserSummaryProps { - user: User | null + user: User contributionData: Record<string, number> dateRange: DateRange hasContributionData: boolean formattedBio: React.ReactNode }- {user.badges.slice().map((badge: Badge) => ( + {user.badges.map((badge: Badge) => (frontend/src/components/SkeletonsBase.tsx (3)
20-22:CardSkeletonComponentis a no-op wrapper — consider usingCardSkeletondirectly.This component simply forwards all props to
CardSkeletonwithout adding any logic. Unless there's a specific reason (e.g., CardSkeleton's prop types don't align), you can assignCardSkeletondirectly toComponentand drop this wrapper entirely.
54-76: RedundantComponent = CardSkeletonComponentin every case.Line 52 already initializes
ComponenttoCardSkeletonComponent. The re-assignments on Lines 56, 60, 70, and 74 are all identical to the default and can be removed — onlycomponentPropsneeds to differ per case.Simplified switch
switch (indexName) { case 'chapters': - Component = CardSkeletonComponent componentProps = { showLevel: false, showIcons: false, showLink: false } break case 'issues': - Component = CardSkeletonComponent componentProps = { showLevel: false, showIcons: true, numIcons: 2, showContributors: false, showSocial: false, } break case 'projects': - Component = CardSkeletonComponent componentProps = { showLink: false, showSocial: false, showIcons: true, numIcons: 3 } break case 'committees': - Component = CardSkeletonComponent componentProps = { showLink: false, showLevel: false, showIcons: true, numIcons: 1 } break
95-95: Use strict equality (===) instead of==.
indexName == 'chapters'should beindexName === 'chapters'to follow TypeScript best practices and avoid unintended coercion.- {indexName == 'chapters' ? ( + {indexName === 'chapters' ? (frontend/src/app/board/[year]/candidates/page.tsx (4)
99-111: Pure utility functions and a duplicate defined inside the component body.
sortByName,sortByContributionCount, andsortChannelsByMessageCountare pure functions that don't reference any props or state. Defining them inside the component means they're recreated on every render.Additionally,
sortChannelsByMessageCount(Lines 107–111) is functionally identical tosortByContributionCount(Lines 103–105).Move them outside the component and deduplicate.
Proposed extraction
+const sortByName = <T extends { name: string }>(items: T[]): T[] => + [...items].sort((a, b) => a.name.localeCompare(b.name)) + +const sortByCount = (entries: Array<[string, number]>): Array<[string, number]> => + [...entries].sort(([, a], [, b]) => b - a) + const CandidateCard = ({ candidate, year }: CandidateCardProps) => { const client = useApolloClient() - const [snapshot, setSnapshot] = useState<MemberSnapshot | null>(null) ... - const sortByName = ... - const sortByContributionCount = ... - const sortChannelsByMessageCount = ...Then replace all uses of
sortByContributionCountandsortChannelsByMessageCountwithsortByCount.
93-665:CandidateCardis ~570 lines — consider decomposing into smaller sub-components.This single component handles data fetching, state management, multiple render helpers, and a large JSX tree. Sections like the contribution stats grid (Lines 407–444), the leadership block (Lines 456–561), the repository list (Lines 562–601), and the additional information section (Lines 622–662) are good candidates for extraction into focused child components — which would also align with this PR's own objective of avoiding large nested render logic.
150-208:renderTopActiveChannelsis a render function defined inside the component — effectively a nested component.This is exactly the pattern the PR aims to eliminate.
renderTopActiveChannels(and similarlyrenderChannelLink,renderRepositoryLink) are component-like functions defined insideCandidateCard. Consider extracting them as standalone components with explicit props.
562-601: IIFE inside JSX is hard to read — extract to a named helper or component.The
(() => { ... })()pattern on Lines 564–601 makes the JSX harder to follow. A named component (e.g.,<TopRepositories />) or at least a named function call would improve readability.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@frontend/src/app/board/`[year]/candidates/page.tsx:
- Line 475: Replace the incorrect phrase "No chapters or projects are lead by
this candidate" with the correct past-tense "No chapters or projects are led by
this candidate" where it appears in the candidates page (look for that exact
string in frontend/src/app/board/[year]/candidates/page.tsx); update the text
node/JSX literal so the UI shows "led" instead of "lead".
- Around line 286-290: The card currently uses a <Button> wrapper which renders
a <button> and contains nested <a>/<Link> elements (violates HTML
accessibility); replace the <Button> wrapper with a non-button container (e.g.,
a <div> with role="button", tabIndex={0}) and keep using the existing
handleCardClick for onClick, and add an onKeyDown handler that triggers
handleCardClick on Enter/Space so keyboard users can activate it; alternatively,
make the card a plain non-interactive container and move the primary action into
a separate clickable element, ensuring you update any references to the Button
component around the card rendering (look for the JSX that calls Button and the
handleCardClick symbol).
- Around line 588-590: The "Top 5" lists currently call sortedRepos.slice(1)
which returns all items after the first; change to sortedRepos.slice(1, 5) so
you render only the next four items (combined with the highlighted top item to
make five) and apply the same fix for the channels list that uses a similar
slice; update the calls that map to renderRepositoryLink and the channels
rendering function (the places where sortedRepos.slice(1) and the channels
equivalent are used) to use .slice(1, 5).
In `@frontend/src/components/SkeletonsBase.tsx`:
- Around line 85-96: ResolvedComponent is defined inside the render which
violates the no-component-definitions-inside-components rule and is unnecessary
because Component is always set; remove the ResolvedComponent alias and replace
all uses of <ResolvedComponent {...componentProps} /> with <Component
{...componentProps} />, eliminating the "?? CardSkeletonComponent" fallback
(dead code) and keeping the existing indexName === 'chapters' Skeleton branch
and componentProps passthrough unchanged.
🧹 Nitpick comments (7)
frontend/src/app/global-error.tsx (1)
91-98: Consider movingSentryErrorFallbackaboveErrorWrapperfor readability.
ErrorWrapper(line 91) referencesSentryErrorFallback(line 101) before it's defined. This works at runtime because the fallback callback is only invoked lazily, but placing the dependency above its consumer is the more conventional top-down reading order.frontend/src/app/board/[year]/candidates/page.tsx (3)
98-110: Move pure helpers to module scope;sortByContributionCountandsortChannelsByMessageCountare identical.These three sort functions are pure (no dependency on props/state) and are recreated every render. Given that the PR's objective is to avoid unnecessary re-creation, move them to module scope. Additionally,
sortByContributionCountandsortChannelsByMessageCounthave the exact same implementation — consolidate into one.Proposed fix
Move above
CandidateCard, at module scope:+// Module-level pure helpers +const sortByName = <T extends { name: string }>(items: T[]): T[] => + [...items].sort((a, b) => a.name.localeCompare(b.name)) + +const sortByCount = (entries: Array<[string, number]>): Array<[string, number]> => + [...entries].sort(([, a], [, b]) => b - a) + const CandidateCard = ({ candidate, year }: CandidateCardProps) => { const client = useApolloClient() const [ledChapters, setLedChapters] = useState<Chapter[]>([]) const [ledProjects, setLedProjects] = useState<Project[]>([]) - - const sortByName = <T extends { name: string }>(items: T[]): T[] => { - return [...items].sort((a, b) => a.name.localeCompare(b.name)) - } - - const sortByContributionCount = (entries: Array<[string, number]>): Array<[string, number]> => { - return [...entries].sort(([, a], [, b]) => b - a) - } - - const sortChannelsByMessageCount = ( - entries: Array<[string, number]> - ): Array<[string, number]> => { - return [...entries].sort(([, a], [, b]) => b - a) - }Then replace all
sortByContributionCountandsortChannelsByMessageCountcalls withsortByCount.
559-596: Avoid IIFE inside JSX — extract to a helper or early-compute.The immediately-invoked function expression
(() => { ... })()embedded in JSX hurts readability and is a code smell. Consider extracting this into arenderTopRepositorieshelper (similar to the existingrenderTopActiveChannels) or computingsortedReposbefore the return.
244-245: Object reference in dependency array may cause redundant fetches.
snapshot?.chapterContributionsis an object — React compares by reference. If Apollo returns a new object reference on re-render (e.g., cache eviction/re-fetch), this effect will re-trigger and re-fetch all chapters sequentially. Consider usingJSON.stringifyof the keys or memoizing the keys array to stabilize the dependency. Same applies to Line 274 forprojectContributions.frontend/src/components/SkeletonsBase.tsx (3)
2-3: Unused import:Reactdefault import is not needed.With the modern JSX transform (which Next.js / this project uses),
import React from 'react'is unnecessary. It was added here only to typeReact.FC, but if you dropCardSkeletonComponent(as suggested above) and useComponentdirectly, theReactimport becomes unused. Even if you keep the current shape, you can replaceReact.FC<...>with just the function signature or importFCas a named import to avoid importing the entire default.
44-45: Consider initializingComponenttoCardSkeletonto eliminate the fallback and improve clarity.Currently
Componentis declared without an initializer, which requires the?? CardSkeletonComponentguard on line 85. Since every switch branch that doesn't assignComponentreturns early, the guard is dead code. InitializingComponentupfront makes the intent clearer and satisfies stricter TypeScript configurations:♻️ Suggested change
- let Component: React.FC<CardSkeletonProps> - let componentProps: CardSkeletonProps = {} + let Component: React.FC<CardSkeletonProps> = CardSkeleton + let componentProps: CardSkeletonProps = {}
12-14: Remove theCardSkeletonComponentwrapper—useCardSkeletondirectly instead.
CardSkeletonComponentis a trivial pass-through wrapper that forwards all props unchanged toCardSkeletonwith no added logic, styling, or transformation. This unnecessary indirection adds complexity with no benefit.Since
CardSkeletonComponentis not used anywhere outside this file, you can safely eliminate it by:
- Removing lines 12–14
- Assigning
CardSkeletondirectly in the switch cases (lines 48, 52, 62, 66) instead ofCardSkeletonComponent- Replacing
ResolvedComponentusage with directComponentreferences at lines 92, 94–96, and removing the unnecessary alias at line 85
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@frontend/src/app/board/`[year]/candidates/page.tsx:
- Around line 98-110: These three pure helpers (sortByName,
sortByContributionCount, sortChannelsByMessageCount) should be moved out of the
React component into module scope so they aren’t re-created on every render;
also merge the two identical functions by creating a single generic sorter
(e.g., sortByCount) to replace both sortByContributionCount and
sortChannelsByMessageCount, update all call sites to use sortByCount, and keep
sortByName as a top-level exported/internal utility to preserve behavior.
In `@frontend/src/components/SkeletonsBase.tsx`:
- Around line 12-14: Remove the redundant CardSkeletonComponent wrapper and
simplify SkeletonBase: delete the CardSkeletonComponent export (which merely
returns <CardSkeleton {...props} />) and remove the dynamic Component variable
in SkeletonBase; where you currently set Component = CardSkeletonComponent for
the 'chapters', 'issues', 'projects', 'committees' cases and later render
<Component {...componentProps} />, instead render <CardSkeleton
{...componentProps} /> directly (keeping the existing componentProps handling)
and remove the unused Component identifier and import/export for
CardSkeletonComponent.
🧹 Nitpick comments (2)
frontend/src/components/SkeletonsBase.tsx (1)
85-96: Nit: prefer strict equality on Line 87.
indexName == 'chapters'should use===for consistency with TypeScript best practices.- {indexName == 'chapters' ? ( + {indexName === 'chapters' ? (frontend/src/app/board/[year]/candidates/page.tsx (1)
559-596: Extract IIFE into a named render helper for consistency.The repository section uses an immediately-invoked function expression inside JSX, while the equivalent channels section uses a dedicated
renderTopActiveChannelshelper. Consider extracting arenderTopActiveRepositoriesmethod for symmetry and readability.
|
1 participant
Proposed change
This refactor moves React component definitions out of their parent components and moved them to the top level, passing the needed data through props instead.
Resolves #2541
Checklist
make check-testlocally: all warnings addressed, tests passed