Conversation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughThis PR adds stable identifiers and namespacing for list keys: introduces Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![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
🤖 Fix all issues with AI agents
In `@frontend/eslint-rules/no-index-key.mjs`:
- Around line 144-156: The BinaryExpression branch incorrectly returns true when
only a string operand is present; change the final return so we only flag when
an index variable is present. In the node.type === 'BinaryExpression' block,
keep the compound check using hasIndexVariable(...) and hasString, but replace
"return hasIndex || hasString" with "return hasIndex" so the rule only flags
when hasIndexVariable(...) is true (use the existing hasIndex and hasString
variables and the hasIndexVariable function to locate the logic).
🧹 Nitpick comments (3)
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx (1)
277-279: Prefer a stable label identifier when possible.If label values are unique in this list, using
key={label}avoids any index-derived key and aligns better with the intent of the no-index-key rule. Otherwise, consider a stable ID from the API.♻️ Suggested tweak (only if labels are unique)
- {visibleLabels.map((label, index) => ( - <span key={`${label}-${index}`} className={labelButtonClassName}> + {visibleLabels.map((label) => ( + <span key={label} className={labelButtonClassName}> {label} </span> ))}frontend/src/components/MenteeContributorsList.tsx (1)
54-57: Use a stable contributor identifier for the key.This list is dynamic; prefer a stable field (e.g.,
loginorid) over any index-derived key.♻️ Suggested tweak (if login is unique)
- {displayContributors.map((item, index) => ( + {displayContributors.map((item) => ( <div - key={`contributor-${index}`} + key={item.login} className="overflow-hidden rounded-lg bg-gray-200 p-4 dark:bg-gray-700" >frontend/src/components/MenteeIssues.tsx (1)
76-78: Prefer label-only keys if they’re unique.If label values are unique per issue,
key={label}is more stable than including the index.♻️ Suggested tweak (only if labels are unique)
- {issue.labels.slice(0, 3).map((label, index) => ( + {issue.labels.slice(0, 3).map((label) => ( <span - key={`${label}-${index}`} + key={label} className="rounded bg-blue-100 px-2 py-1 text-xs text-blue-800" >
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/src/components/Card.tsx (1)
142-148: Inconsistent with PR goal:topContributorsstill uses array index for keys.The PR aims to eliminate array-index-based keys, yet
ContributorAvatarstill usesindexin both thekeyfallback anduniqueKeyprop. SinceContributornow has anidfield, use it for stable keying.Suggested fix
- {topContributors?.map((contributor, index) => ( + {topContributors?.map((contributor) => ( <ContributorAvatar - key={contributor.login || `contributor-${index}`} + key={contributor.id} contributor={contributor} - uniqueKey={index.toString()} + uniqueKey={contributor.id} /> ))}frontend/src/app/members/[memberKey]/page.tsx (1)
61-82: Duplicate keys when words repeat in bio.Using
${user.login}-${word}as the key will produce duplicates when the same word appears multiple times in a bio (e.g., "I am a a developer" → two elements with keyjohndoe-a). Similarly, the mention key could duplicate if the same user is mentioned twice.Consider retaining the index but combining it with a stable prefix to satisfy the lint rule while ensuring uniqueness:
Proposed fix
- const formattedBio = user?.bio?.split(' ').map((word) => { + const formattedBio = user?.bio?.split(' ').map((word, index) => { const mentionMatch = word.match(/^@([\w-]+(?:\.[\w-]+)*)([^\w@])?$/) if (mentionMatch && mentionMatch.length > 1) { const username = mentionMatch[1] const punctuation = mentionMatch[2] || '' return ( - <React.Fragment key={`mention-${user.login}-${username}`}> + <React.Fragment key={`${user.login}-bio-${index}`}> <Link href={`https://github.com/${username}`} target="_blank" rel="noopener noreferrer" className="text-blue-400 hover:underline" > @{username} </Link> {punctuation} <span> </span> </React.Fragment> ) } - return <span key={`${user.login}-${word}`}>{word} </span> + return <span key={`${user.login}-bio-${index}`}>{word} </span> })Note: If the ESLint rule disallows array indices entirely, an alternative is to generate a unique ID for each word position using a deterministic hash or by tracking word occurrence count.
🤖 Fix all issues with AI agents
In `@frontend/__tests__/unit/components/CardDetailsPage.test.tsx`:
- Around line 176-179: The prop type declaration is using the wrong property
name `_entityKey`; change the interface so it declares `entityKey: string` (not
`_entityKey`) to match the actual prop passed in and the component
destructuring; keep `leaders: string` and the index signature as-is, and ensure
any places that destructure props (where you alias `entityKey` to `_entityKey`)
continue to work with the renamed type property.
- Around line 349-354: The props type in the CardDetailsPage test mock declares
`_entityKey: string` but the component destructures `entityKey`, causing a
mismatch; update the type/interface in CardDetailsPage.test.tsx to use
`entityKey: string` (matching the destructured prop name)—mirror the same fix
applied to the LeadersList mock so other tests remain consistent and rebuild
correctly.
In `@frontend/__tests__/unit/components/MultiSearch.test.tsx`:
- Around line 326-352: The test fixtures in the "filters event data based on
query" test are missing the required Event.id field and are being force-cast
with "as Event[]"; update each event object in the eventData array to include a
unique id property (e.g., 'event-1', 'event-2', 'event-3') so the objects
conform to the Event type instead of relying on the type assertion; ensure the
test continues to use the same objectID/key values and remove or keep the "as
Event[]" cast once the objects are fully typed.
🧹 Nitpick comments (4)
frontend/src/components/NavDropDown.tsx (1)
70-73: Avoid potentially non-unique/undefined keys.
submenu.hrefis optional, sokey={${submenu.href}}can become"undefined"for multiple items, causing duplicate keys and unstable reconciliation. Consider a safer composite fallback.♻️ Proposed fix
- {link.submenu?.map((submenu) => ( + {link.submenu?.map((submenu) => ( <Link - key={`${submenu.href}`} + key={`${submenu.href ?? 'no-href'}-${submenu.text}`} href={submenu.href || '/'} className={cn(frontend/src/components/Pagination.tsx (1)
71-73: Prefer stable keys and drop the eslint disable.Index-based keys are avoidable here; use the page value as the key and only fall back to index for ellipses. This keeps the new rule meaningful.
♻️ Proposed refactor
- {pageNumbers.map((number, index) => ( - // eslint-disable-next-line react/no-array-index-key - <React.Fragment key={`pagination-${index}-${number}`}> + {pageNumbers.map((number, index) => { + const key = + number === '...' ? `pagination-ellipsis-${index}` : `pagination-${number}` + return ( + <React.Fragment key={key}> {number === '...' ? ( <div className="flex h-10 w-10 items-center justify-center text-gray-600 dark:text-gray-400"> <FaEllipsis className="h-5 w-5" aria-hidden="true" /> </div> ) : ( <Button type="button" aria-current={currentPage === number ? 'page' : undefined} aria-label={`Go to page ${number}`} className={`flex h-10 min-w-10 items-center justify-center rounded-md px-3 text-sm font-medium ${ currentPage === number ? 'bg-[`#83a6cc`] text-white dark:bg-white dark:text-black' : 'border-1 border-gray-200 bg-white text-gray-700 hover:bg-gray-50 dark:border-gray-600 dark:bg-gray-800 dark:text-gray-300 dark:hover:bg-gray-700' }`} onPress={() => onPageChange(number as number)} > {number} </Button> )} - </React.Fragment> - ))} + </React.Fragment> + ) + })}frontend/src/components/LeadersList.tsx (1)
27-38: Potential key collision with duplicate leader names.Using
${entityKey}-${leader}as the key assumes leader names are unique within the list. If the same leader name appears multiple times (e.g., from a malformed input string like"John, John"), React will encounter duplicate keys.Consider including the index as a fallback for uniqueness while still benefiting from stable identity:
Suggested fix
- <span key={`${entityKey}-${leader}`}> + <span key={`${entityKey}-${leader}-${index}`}>frontend/src/app/community/snapshots/[id]/page.tsx (1)
152-156: Consider simplifying by moving key to the Card component.The
React.Fragmentwrapper is solely used to attach thekeyprop. SincerenderChapterCardreturns a single Card element, you could simplify by passing the key directly to the Card.Suggested simplification
- {snapshot.newChapters - .filter((chapter) => chapter.isActive) - .map((chapter) => ( - <React.Fragment key={chapter.key}>{renderChapterCard(chapter)}</React.Fragment> - ))} + {snapshot.newChapters + .filter((chapter) => chapter.isActive) + .map((chapter) => renderChapterCard(chapter))}Then update
renderChapterCardto accept and use the key:const renderChapterCard = (chapter: Chapter) => { // ... existing code ... return ( <Card key={chapter.key} button={submitButton} cardKey={chapter.key} // ... rest of props /> ) }
|
2 participants
Added a
no-index-keyrule to custom eslint-rules. This will help keep codebase clean and comply with typescript:S6479 ruleChecklist
make check-testlocally: all warnings addressed, tests passed