Conversation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughReplaced Font Awesome with react-icons across the frontend: swapped imports and JSX, introduced IconWrapper, changed many icon prop types from FontAwesome types to react-icons' IconType, updated utilities/constants/url mappings, adapted numerous tests/mocks, removed Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 7
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/DisplayIcon.tsx (1)
3-51: Tighten types and guard against undefined icons to avoid runtime crashesTwo related issues here:
iconstyping vs usage
DisplayIcondeclaresicons: IconwhereIconis defined as{ [key: string]: IconType }, but in this component you treaticons[item]as a data value:{typeof icons[item] === 'number' ? millify(icons[item], { precision: 1 }) : icons[item]}This is a fundamental type mismatch. The actual data passed (from
Card.tsx) is{ starsCount: 1250, forksCount: 350, ... }— a map of numbers/strings, notIconTypes. Either adjust theIcontype to reflect the real value type (e.g.,Record<string, number | string>), or narrow the prop type for this component to a dedicated metrics type instead of reusing the icon type alias.
IconWrappermay receiveundefinedand crashHere:
<IconWrapper className={iconClassName} icon={ICONS[item as IconKeys]?.icon} />If
itemis not a validIconKeys(for example, if called with an unexpected key),ICONS[item as IconKeys]isundefined, so?.iconyieldsundefinedandIconWrapperattempts to render<undefined />, which throws at runtime. To make this safe, guard before rendering:+ const iconConfig = ICONS[item as IconKeys] + if (!iconConfig) return null // or a fallback UI/log + <Tooltip - content={`${ICONS[item as IconKeys]?.label}`} + content={iconConfig.label} ... > <div className={containerClassName}> <span className="text-gray-600 dark:text-gray-300"> - {typeof icons[item] === 'number' ? millify(icons[item], { precision: 1 }) : icons[item]} + {typeof icons[item] === 'number' + ? millify(icons[item], { precision: 1 }) + : icons[item]} </span> <span> - <IconWrapper className={iconClassName} icon={ICONS[item as IconKeys]?.icon} /> + <IconWrapper className={iconClassName} icon={iconConfig.icon} /> </span> </div> </Tooltip>This also prevents rendering a tooltip with
"undefined"as content when the key is not found.(Note: if you care about showing zeros, consider changing
return icons[item] ? (...) : nullto explicitly check for truthiness to preserve0values, but that's pre-existing behavior.)
🧹 Nitpick comments (27)
frontend/src/components/CalendarButton.tsx (1)
7-7: Remove unused hover state logic.The
isHoveredstate and its event handlers are no longer used after migrating to the staticFaCalendarPlusicon. Previously, hover state switched between two different FontAwesome icons, but that behavior was removed.Apply this diff to clean up the dead code:
-import { useState } from 'react' import { FaCalendarPlus } from 'react-icons/fa6' import type { CalendarButtonProps } from 'types/calendar' import getGoogleCalendarUrl from 'utils/getGoogleCalendarUrl' export default function CalendarButton(props: Readonly<CalendarButtonProps>) { - const [isHovered, setIsHovered] = useState(false) const { event, className = '', iconClassName = 'h-4 w-4', icon, showLabel = false, label = 'Add to Google Calendar', } = props const href = getGoogleCalendarUrl(event) const safeTitle = event.title || 'event' const ariaLabel = `Add ${safeTitle} to Google Calendar` return ( <a href={href} target="_blank" rel="noopener noreferrer" aria-label={ariaLabel} title={ariaLabel} className={className} - onMouseEnter={() => setIsHovered(true)} - onMouseLeave={() => setIsHovered(false)} > {icon || <FaCalendarPlus className={iconClassName} />} {showLabel && <span>{label}</span>} </a> ) }Also applies to: 28-29
frontend/src/components/MetricsPDFButton.tsx (2)
23-23: Consider adding error handling for the async PDF fetch.The async onClick handler doesn't catch errors from
fetchMetricsPDF. If the PDF generation fails, users won't receive feedback and the error will be an unhandled promise rejection.Consider wrapping the call with try-catch:
- onClick={async () => await fetchMetricsPDF(path, fileName)} + onClick={async () => { + try { + await fetchMetricsPDF(path, fileName) + } catch (error) { + console.error('Failed to download PDF:', error) + // Optionally show user-facing error notification + } + }}
21-24: Consider adding aria-label for improved accessibility.While the Tooltip provides context on hover, screen reader users may benefit from an explicit
aria-labelon the icon button element.<FaFileArrowDown + aria-label="Download as PDF" className="ml-2 h-7 w-7 cursor-pointer text-gray-500 transition-colors duration-200 hover:text-gray-700" onClick={async () => await fetchMetricsPDF(path, fileName)} />frontend/src/components/SnapshotCard.tsx (1)
20-29: Icon JSX replacement is correct; consider decorative a11y hintThe replacements with
<FaCalendar />and<FaChevronRight />are functionally equivalent to the previous FontAwesome usage and look good. Since these icons are purely decorative (date and “View Snapshot” text already convey meaning), you might want to hide them from screen readers:- <FaCalendar className="mr-1 h-4 w-4" /> + <FaCalendar className="mr-1 h-4 w-4" aria-hidden="true" /> ... - <FaChevronRight className="ml-2 h-4 w-4 transform transition-transform group-hover:translate-x-1" /> + <FaChevronRight + className="ml-2 h-4 w-4 transform transition-transform group-hover:translate-x-1" + aria-hidden="true" + />This keeps the DOM semantics tight while preserving the visual change.
frontend/src/components/Search.tsx (1)
6-6: Migrate icons to FA6 for consistency with the rest of the codebase.The import currently uses
react-icons/fa(Font Awesome 5), but the codebase has predominantly migrated toreact-icons/fa6. WhileFaSearchandFaTimesremain in FA5 in other components (MultiSearch.tsx,Header.tsx), consider updating to FA6 to align with the migration strategy used throughout the project, assuming FA6 equivalents are available.frontend/src/utils/utility.ts (1)
38-45: Fallback social URL icon now a component – verify downstream expectationsSwitching the fallback from a Font Awesome class string to the
FaGlobecomponent is aligned with the react-icons migration. Please make sure:
urlMappings[*].iconis also a React component type (e.g.,IconType) so that the union withFaGlobeis consistent.- The consumer that renders these objects expects
iconto be a component, not a string.If everything downstream already uses react-icons components, this change should be type‑safe and cleaner. Optionally, you could centralize this default (
'Social Links'+FaGlobe) inurlMappingsor a shared constant to avoid duplicating the fallback definition.frontend/src/app/global-error.tsx (1)
43-43: Consider splitting styling improvements into a separate PR.The dark mode styling addition (
dark:bg-slate-800) and text color adjustment appear to be UI refinements separate from the primary PR objective. Per project conventions, migration PRs should remain focused on their core objective without including additional styling or UI improvements.frontend/src/components/LineChart.tsx (1)
4-17: Use type-only import forIconTypefor consistency
IconTypeis only used as a type here. To avoid an unnecessary runtime import and match the pattern used elsewhere in the PR (e.g., DonutBarChart), prefer a type-only import:-import { IconType } from 'react-icons' +import type { IconType } from 'react-icons'The updated
icon?: IconTypeprop and its usage inSecondaryCardotherwise look aligned with the react-icons migration.frontend/src/wrappers/IconWrapper.tsx (1)
1-13: IconWrapper implementation is sound; consider type-only importsThe wrapper correctly treats
iconas anIconTypeand forwards SVG props (includingsize) to the underlying react-icon, which is what you want.Since
ComponentPropsandIconTypeare used only in types, you can tighten imports and avoid runtime overhead with type-only imports:-import { ComponentProps } from 'react' -import { IconType } from 'react-icons' +import type { ComponentProps } from 'react' +import type { IconType } from 'react-icons'Otherwise this abstraction looks good as a single place to standardize icon rendering.
frontend/src/components/LoginPageContent.tsx (1)
7-8: Spinner and GitHub icon migration looks good; optional a11y tweakThe swap to
FaSpinner/FaGithubpreserves layout and behavior. Consider addingaria-hidden="true"to these icons since the adjacent text already conveys status, reducing noise for screen readers.Also applies to: 43-56, 71-77
frontend/src/components/MenteeContributorsList.tsx (1)
5-7: IconWrapper integration is correct; consider a more stable keyPassing the icon component into
IconWrapperand then intoSecondaryCardis consistent and keeps this component’s API clean. Minor nit: ifcontributorscan ever reorder, prefer a stable key such askey={item.login}instead of the array index to avoid unnecessary remounts.Also applies to: 16-19, 21-29, 45-47, 53-75
frontend/src/utils/constants.ts (1)
1-3: Typed footerIcons config looks solidUsing
IconTypefor theiconfield and wiring each entry to its matching URL/label makes the footer social config clear and type-safe. If you want additional safety, you could mark this arrayas constto keep it fully readonly, but that’s optional.Also applies to: 31-36, 37-56
frontend/src/components/Footer.tsx (1)
5-6: Footer chevron + social icon migration is cleanThe new chevron icons hook cleanly into the existing toggle logic, and mapping
footerIconstoSocialIconcomponents keeps the social section nicely data-driven. Nice touch on addingrel="noopener noreferrer"to the external links.Also applies to: 36-41, 70-87
frontend/src/utils/urlIconMappings.ts (1)
1-1: Consider hardeninggetSocialIconagainst invalid URLsThe icon mappings themselves look good and the
IconTypereturn type matches the new usage, butnew URL(url)will throw for relative or malformed inputs. If there’s any chance callers pass non‑absolute URLs, wrapping URL parsing in a small try/catch (and falling back toFaGlobe) would make this helper more robust.Also applies to: 3-22, 24-50
frontend/src/app/organizations/page.tsx (1)
4-4: Checkbutton.icontype expectations vs passing a JSX elementHere
submitButton.iconis a rendered<FaRightToBracket />element. In other parts of the PR, icons are often passed around as icon components and rendered via IconWrapper. IfUserCard’s button prop now expects an icon component (rather than a ReactNode), consider changing this to passFaRightToBracketitself and let the consumer apply sizing/styling for consistency. Otherwise, confirm thatbutton.iconis still typed as a generic ReactNode so this usage stays type‑safe.Also applies to: 31-35
frontend/src/app/projects/page.tsx (1)
4-4: Align project card button icon with shared icon conventionsThe JSX usage of
<FaRightToBracket className="h-4 w-4" />will work ifCard’sbutton.iconremains a ReactNode. If the shared button/card types in this PR have been migrated to accept icon components that are rendered via IconWrapper, consider instead passing the icon component and centralizing size/color there to keep types and patterns consistent across the app.Also applies to: 41-45
frontend/src/app/committees/page.tsx (1)
4-4: Keep committee button icon consistent with other button/icon consumersThis page also passes a rendered
<FaRightToBracket />intosubmitButton.icon. If your shared button/card props now standardize on icon components (rendered by IconWrapper), it may be cleaner to pass the component instead and let the consumer handle sizing. If those props are still ReactNode-based, this is fine as-is; just ensure the typings remain aligned.Also applies to: 32-36
frontend/src/app/members/page.tsx (1)
4-4: Verify member card button icon type vs JSX usageAs with the other listing pages,
submitButton.iconhere is a JSX element. IfUserCard/button props have been tightened to use icon components with IconWrapper elsewhere, consider passing the component instead to avoid type drift and centralize styling. If they remain ReactNode-based, this change is consistent and should be safe.Also applies to: 30-34
frontend/src/app/community/snapshots/[id]/page.tsx (1)
5-5: Snapshot details icons updated correctly; optional DRY opportunityThe switch to
FaRightToBracketfor project/chapter buttons andFaCalendarfor the header dates preserves intent and behavior. If more buttons like this appear elsewhere, you could later extract a shared “View Details” button config, but that’s purely a nicety.Also applies to: 50-51, 79-80, 120-120
frontend/src/app/projects/[projectKey]/page.tsx (1)
7-9: Unify issue icon with the rest of the codebase (FaExclamationCircle vs FaCircleExclamation)Here the issues stat uses
FaExclamationCirclefromreact-icons/fa, while other components in this PR useFaCircleExclamationfromreact-icons/fa6for issues. For consistency and to stick to fa6 everywhere, consider this change:-import { FaExclamationCircle } from 'react-icons/fa' -import { FaCodeFork, FaFolderOpen, FaStar } from 'react-icons/fa6' +import { FaCodeFork, FaFolderOpen, FaStar, FaCircleExclamation } from 'react-icons/fa6' @@ - { - icon: FaExclamationCircle, + { + icon: FaCircleExclamation, value: project.issuesCount, unit: 'Issue', },This keeps the visual language and imports consistent with
RecentIssuesandMilestones.Also applies to: 68-82
frontend/src/components/Header.tsx (1)
8-9: Consider consolidating duplicate imports.Both lines import from
'react-icons/fa'and can be combined into a single import statement for better code organization.Apply this diff to consolidate the imports:
-import { FaRegHeart, FaRegStar } from 'react-icons/fa' -import { FaHeart as FaSolidHeart, FaStar as FaSolidStar, FaBars, FaTimes } from 'react-icons/fa' +import { FaRegHeart, FaRegStar, FaHeart as FaSolidHeart, FaStar as FaSolidStar, FaBars, FaTimes } from 'react-icons/fa'frontend/src/components/NavButton.tsx (1)
16-16: Redundant type intersection.According to
frontend/src/types/button.ts,NavButtonPropsalready includesdefaultIcon: IconTypeandhoverIcon: IconType. The intersection& { defaultIcon: IconType; hoverIcon: IconType }is redundant.-}: NavButtonProps & { defaultIcon: IconType; hoverIcon: IconType }) => { +}: NavButtonProps) => {frontend/src/components/StatusBadge.tsx (1)
83-83: Consider usingIconWrapperfor consistency.Other components in this migration (e.g.,
SecondaryCard,NavButton,ProjectsDashboardDropDown) useIconWrapperfor icon rendering. While direct rendering works, usingIconWrapperhere would maintain consistency across the codebase.+import { IconWrapper } from 'wrappers/IconWrapper' ... - {showIcon && <DisplayIcon className="h-3 w-3" />} + {showIcon && <IconWrapper icon={DisplayIcon} className="h-3 w-3" />}frontend/src/components/MenteeIssues.tsx (1)
3-4: State icon mapping and IconWrapper integration look consistent
getStateIconnow returns react-icons components (FaBug/FaCheckCircle/FaClock), and all usages route throughIconWrapper, which matches the new IconType-based pattern. The empty-state icon selection usingactiveTab === 'open' ? FaBug : FaCheckCirclealso preserves the expected UX.If you want clearer visual distinction for merged vs. other non-open states in the future, you could map
state === 'merged'to a dedicated icon instead of falling through toFaClock.Also applies to: 31-40, 46-49, 61-64
frontend/src/app/page.tsx (1)
7-21: getProjectIcon typing and icon mapping are aligned with react-iconsDeclaring
getProjectIcon(projectType: string): IconTypeand returning the Fa* components directly (FaCode, FaBook, FaFileCode, FaTag) fits the react-icons pattern and works cleanly with consumers likeIconWrapper. The default fallback toFaFileCodefor unknown types is a reasonable choice.If
project.typecan ever be null/undefined from GraphQL, consider tightening its type or guarding before calling.toLowerCase()to avoid a runtime throw in that edge case.Also applies to: 98-111
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx (1)
228-233: Section icons migrated correctly; consider minor a11y tweak for the unassign icon
FaTagsfor “Labels”,HiUserGroupfor “Assignees” and “Interested Users”,FaCodeBranchas theSecondaryCardicon for “Pull Requests”, andFaPluson the “Assign” buttonare all reasonable replacements and follow the component-based icon usage.
For the icon-only unassign button (Line 296), you already provide a descriptive
aria-labelandtitleon the<button>. To avoid screen readers potentially announcing both the label and the decorative icon, you could mark the icon as hidden:- <FaXmark /> + <FaXmark aria-hidden="true" />This keeps the visual unchanged while tightening accessibility semantics.
Also applies to: 245-256, 296-297, 304-305, 378-381, 422-424
frontend/src/components/CardDetailsPage.tsx (1)
341-354: SocialLinks now correctly renders dynamic react-icons componentsMapping
urlsto:const SocialIcon = getSocialIcon(url) ... <SocialIcon className="h-5 w-5" />works well with
getSocialIcon’sIconTypereturn type, and the anchor’s color classes (text-blue-400+ hover variants) will style the icon via inheritance whileclassName="h-5 w-5"handles sizing. This is a clean replacement for the previous FontAwesomeIcon-based rendering.If there’s any chance
urlscan contain invalid URLs, you might eventually want to hardengetSocialIcon(e.g., try/catch aroundnew URL(url)) to avoid throwing from this map, but that’s pre-existing and not introduced by this migration.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (86)
frontend/__tests__/unit/global-error.test.tsx(1 hunks)frontend/src/app/about/page.tsx(9 hunks)frontend/src/app/board/[year]/candidates/page.tsx(6 hunks)frontend/src/app/chapters/page.tsx(2 hunks)frontend/src/app/committees/[committeeKey]/page.tsx(2 hunks)frontend/src/app/committees/page.tsx(2 hunks)frontend/src/app/community/snapshots/[id]/page.tsx(4 hunks)frontend/src/app/community/snapshots/page.tsx(2 hunks)frontend/src/app/contribute/page.tsx(2 hunks)frontend/src/app/global-error.tsx(1 hunks)frontend/src/app/members/[memberKey]/page.tsx(3 hunks)frontend/src/app/members/page.tsx(2 hunks)frontend/src/app/my/mentorship/page.tsx(3 hunks)frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx(7 hunks)frontend/src/app/organizations/[organizationKey]/page.tsx(2 hunks)frontend/src/app/organizations/[organizationKey]/repositories/[repositoryKey]/page.tsx(2 hunks)frontend/src/app/organizations/page.tsx(2 hunks)frontend/src/app/page.tsx(12 hunks)frontend/src/app/projects/[projectKey]/page.tsx(2 hunks)frontend/src/app/projects/dashboard/metrics/[projectKey]/page.tsx(9 hunks)frontend/src/app/projects/dashboard/metrics/page.tsx(4 hunks)frontend/src/app/projects/dashboard/page.tsx(4 hunks)frontend/src/app/projects/page.tsx(2 hunks)frontend/src/app/settings/api-keys/page.tsx(7 hunks)frontend/src/components/AnchorTitle.tsx(2 hunks)frontend/src/components/Badges.tsx(2 hunks)frontend/src/components/BarChart.tsx(2 hunks)frontend/src/components/BreadCrumbs.tsx(2 hunks)frontend/src/components/CalendarButton.tsx(2 hunks)frontend/src/components/Card.tsx(4 hunks)frontend/src/components/CardDetailsPage.tsx(9 hunks)frontend/src/components/DashboardCard.tsx(2 hunks)frontend/src/components/DisplayIcon.tsx(4 hunks)frontend/src/components/DonutBarChart.tsx(2 hunks)frontend/src/components/EntityActions.tsx(2 hunks)frontend/src/components/Footer.tsx(3 hunks)frontend/src/components/GeneralCompliantComponent.tsx(2 hunks)frontend/src/components/Header.tsx(5 hunks)frontend/src/components/HealthMetrics.tsx(5 hunks)frontend/src/components/InfoBlock.tsx(3 hunks)frontend/src/components/InfoItem.tsx(4 hunks)frontend/src/components/ItemCardList.tsx(2 hunks)frontend/src/components/Leaders.tsx(2 hunks)frontend/src/components/LineChart.tsx(2 hunks)frontend/src/components/LoginPageContent.tsx(4 hunks)frontend/src/components/MenteeContributorsList.tsx(3 hunks)frontend/src/components/MenteeIssues.tsx(6 hunks)frontend/src/components/MetricsPDFButton.tsx(2 hunks)frontend/src/components/Milestones.tsx(2 hunks)frontend/src/components/Modal.tsx(2 hunks)frontend/src/components/ModeToggle.tsx(2 hunks)frontend/src/components/ModuleCard.tsx(3 hunks)frontend/src/components/MultiSearch.tsx(6 hunks)frontend/src/components/NavButton.tsx(3 hunks)frontend/src/components/NavDropDown.tsx(3 hunks)frontend/src/components/Pagination.tsx(2 hunks)frontend/src/components/ProjectTypeDashboardCard.tsx(1 hunks)frontend/src/components/ProjectsDashboardDropDown.tsx(2 hunks)frontend/src/components/ProjectsDashboardNavBar.tsx(2 hunks)frontend/src/components/RecentIssues.tsx(2 hunks)frontend/src/components/RecentPullRequests.tsx(2 hunks)frontend/src/components/RecentReleases.tsx(2 hunks)frontend/src/components/Release.tsx(2 hunks)frontend/src/components/RepositoryCard.tsx(2 hunks)frontend/src/components/ScrollToTop.tsx(2 hunks)frontend/src/components/Search.tsx(3 hunks)frontend/src/components/SecondaryCard.tsx(2 hunks)frontend/src/components/ShowMoreButton.tsx(2 hunks)frontend/src/components/SingleModuleCard.tsx(3 hunks)frontend/src/components/SnapshotCard.tsx(3 hunks)frontend/src/components/SortBy.tsx(2 hunks)frontend/src/components/StatusBadge.tsx(5 hunks)frontend/src/components/ToggleableList.tsx(3 hunks)frontend/src/components/TopContributorsList.tsx(2 hunks)frontend/src/components/UserCard.tsx(4 hunks)frontend/src/components/UserMenu.tsx(2 hunks)frontend/src/types/button.ts(2 hunks)frontend/src/types/card.ts(3 hunks)frontend/src/types/icon.ts(1 hunks)frontend/src/types/level.ts(1 hunks)frontend/src/utils/constants.ts(2 hunks)frontend/src/utils/data.ts(2 hunks)frontend/src/utils/milestoneProgress.ts(2 hunks)frontend/src/utils/urlIconMappings.ts(1 hunks)frontend/src/utils/utility.ts(2 hunks)frontend/src/wrappers/IconWrapper.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (13)
📚 Learning: 2025-07-03T03:08:03.290Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1703
File: frontend/src/components/BarChart.tsx:33-46
Timestamp: 2025-07-03T03:08:03.290Z
Learning: In the OWASP Nest project's BarChart component (frontend/src/components/BarChart.tsx), the days and requirements arrays are guaranteed to always have the same length in their use cases, so input validation for array length matching is not needed.
Applied to files:
frontend/src/components/BarChart.tsxfrontend/src/components/ProjectsDashboardNavBar.tsxfrontend/src/components/HealthMetrics.tsxfrontend/src/components/DonutBarChart.tsxfrontend/src/app/projects/dashboard/metrics/[projectKey]/page.tsx
📚 Learning: 2025-06-21T12:21:32.372Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1633
File: frontend/src/components/GradientRadialChart.tsx:67-116
Timestamp: 2025-06-21T12:21:32.372Z
Learning: The react-apexcharts Chart component does not support ARIA attributes like aria-label and role as direct props. To add accessibility attributes to ApexCharts in React, wrap the Chart component in a container div with the appropriate ARIA attributes.
Applied to files:
frontend/src/components/BarChart.tsxfrontend/src/components/LineChart.tsxfrontend/src/components/HealthMetrics.tsxfrontend/src/components/DonutBarChart.tsxfrontend/src/app/projects/dashboard/metrics/[projectKey]/page.tsxfrontend/src/app/projects/dashboard/page.tsx
📚 Learning: 2025-09-17T02:42:41.928Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 2288
File: frontend/src/components/ActionButton.tsx:0-0
Timestamp: 2025-09-17T02:42:41.928Z
Learning: In frontend/src/components/ActionButton.tsx, the user Rajgupta36 intentionally changed text-blue-600 to text-[#1D7BD7] to align the text color with the border color (#1D7BD7) for visual consistency, prioritizing design alignment over theme tokens.
Applied to files:
frontend/src/app/members/page.tsxfrontend/src/utils/milestoneProgress.tsfrontend/src/components/EntityActions.tsxfrontend/src/app/contribute/page.tsxfrontend/src/components/SortBy.tsxfrontend/src/components/SingleModuleCard.tsxfrontend/src/components/CalendarButton.tsxfrontend/src/types/button.tsfrontend/src/components/Modal.tsxfrontend/src/components/Search.tsxfrontend/src/components/DashboardCard.tsxfrontend/src/components/ProjectsDashboardNavBar.tsxfrontend/src/app/committees/[committeeKey]/page.tsxfrontend/src/components/AnchorTitle.tsxfrontend/src/app/community/snapshots/page.tsxfrontend/src/app/chapters/page.tsxfrontend/src/components/Badges.tsxfrontend/src/app/projects/[projectKey]/page.tsxfrontend/src/components/LoginPageContent.tsxfrontend/src/utils/urlIconMappings.tsfrontend/src/components/InfoItem.tsxfrontend/src/components/Pagination.tsxfrontend/src/components/ProjectsDashboardDropDown.tsxfrontend/src/components/Footer.tsxfrontend/src/components/HealthMetrics.tsxfrontend/src/components/RepositoryCard.tsxfrontend/src/app/page.tsxfrontend/src/components/MetricsPDFButton.tsxfrontend/src/components/ModeToggle.tsxfrontend/src/components/StatusBadge.tsxfrontend/src/app/about/page.tsxfrontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsxfrontend/src/components/Header.tsxfrontend/src/components/NavButton.tsxfrontend/src/components/Leaders.tsxfrontend/src/components/UserCard.tsxfrontend/src/components/Card.tsxfrontend/src/components/CardDetailsPage.tsxfrontend/src/components/ModuleCard.tsxfrontend/src/app/organizations/page.tsxfrontend/src/components/ScrollToTop.tsxfrontend/src/app/settings/api-keys/page.tsxfrontend/src/components/GeneralCompliantComponent.tsxfrontend/src/components/MenteeIssues.tsxfrontend/src/app/organizations/[organizationKey]/page.tsxfrontend/src/app/global-error.tsxfrontend/src/components/ShowMoreButton.tsxfrontend/src/components/UserMenu.tsxfrontend/src/app/projects/dashboard/page.tsxfrontend/src/app/community/snapshots/[id]/page.tsxfrontend/src/app/projects/page.tsxfrontend/src/components/RecentIssues.tsxfrontend/src/utils/data.tsfrontend/src/app/committees/page.tsxfrontend/src/utils/constants.tsfrontend/src/app/my/mentorship/page.tsxfrontend/src/components/DisplayIcon.tsxfrontend/src/components/MultiSearch.tsxfrontend/src/app/members/[memberKey]/page.tsxfrontend/src/app/board/[year]/candidates/page.tsx
📚 Learning: 2025-07-13T11:29:25.245Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/page.tsx:59-61
Timestamp: 2025-07-13T11:29:25.245Z
Learning: In Next.js 13+ app router, components with the 'use client' directive run entirely on the client side and don't require window object existence checks or SSR hydration considerations. Direct access to window.location and other browser APIs is safe in client components.
Applied to files:
frontend/src/app/members/page.tsxfrontend/src/components/EntityActions.tsxfrontend/src/components/SingleModuleCard.tsxfrontend/src/app/committees/[committeeKey]/page.tsxfrontend/src/app/projects/[projectKey]/page.tsxfrontend/src/components/LoginPageContent.tsxfrontend/src/app/projects/dashboard/metrics/page.tsxfrontend/src/app/page.tsxfrontend/src/app/about/page.tsxfrontend/src/components/Header.tsxfrontend/src/app/organizations/page.tsxfrontend/src/app/organizations/[organizationKey]/page.tsxfrontend/src/app/community/snapshots/[id]/page.tsxfrontend/src/app/committees/page.tsxfrontend/src/app/my/mentorship/page.tsxfrontend/src/app/members/[memberKey]/page.tsx
📚 Learning: 2025-07-09T08:37:10.241Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1718
File: frontend/src/app/projects/dashboard/metrics/page.tsx:96-96
Timestamp: 2025-07-09T08:37:10.241Z
Learning: In the OWASP Nest project's MetricsPage component (frontend/src/app/projects/dashboard/metrics/page.tsx), the sorting dropdown intentionally uses selectionMode="multiple" to allow users to select multiple sorting criteria simultaneously. This enables complex sorting scenarios where users can sort by multiple fields in sequence.
Applied to files:
frontend/src/components/SortBy.tsxfrontend/src/app/projects/dashboard/metrics/page.tsx
📚 Learning: 2025-07-13T11:34:31.823Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/page.tsx:14-14
Timestamp: 2025-07-13T11:34:31.823Z
Learning: In the Next.js frontend mentorship application, there are two distinct types for authentication-related data: ExtendedSession for useSession hook (containing accessToken and user.login properties) and UserRolesData for useUserRoles hook (containing currentUserRoles.roles array). The correct access pattern for GitHub username is `(session as ExtendedSession)?.user?.login`.
Applied to files:
frontend/src/components/SingleModuleCard.tsxfrontend/src/app/projects/[projectKey]/page.tsxfrontend/src/components/LoginPageContent.tsxfrontend/src/components/CardDetailsPage.tsxfrontend/src/app/organizations/[organizationKey]/page.tsxfrontend/src/app/organizations/[organizationKey]/repositories/[repositoryKey]/page.tsxfrontend/src/components/UserMenu.tsxfrontend/src/app/my/mentorship/page.tsxfrontend/src/app/members/[memberKey]/page.tsx
📚 Learning: 2025-07-12T17:12:25.807Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/__tests__/unit/pages/ProgramDetails.test.tsx:35-0
Timestamp: 2025-07-12T17:12:25.807Z
Learning: In React applications, button elements should always have an explicit type attribute (type="button", type="submit", or type="reset") to prevent unintended form submission behavior, even when the button appears to be outside of a form context. The default type is "submit" which can cause unexpected behavior.
Applied to files:
frontend/src/types/button.tsfrontend/src/app/chapters/page.tsxfrontend/src/app/organizations/page.tsxfrontend/src/app/community/snapshots/[id]/page.tsxfrontend/src/app/projects/page.tsxfrontend/src/app/committees/page.tsx
📚 Learning: 2025-11-17T16:47:05.578Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:427-427
Timestamp: 2025-11-17T16:47:05.578Z
Learning: In the frontend test files for the OWASP/Nest repository, `expect(true).toBe(true)` no-op assertions may be intentionally added as workarounds when ESLint rule jest/expect-expect doesn't detect expectations inside helper functions or waitFor callbacks. These can be resolved by configuring the ESLint rule's assertFunctionNames option to recognize custom assertion helpers instead of flagging them as redundant.
Applied to files:
frontend/__tests__/unit/global-error.test.tsx
📚 Learning: 2025-11-17T17:30:42.139Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:169-171
Timestamp: 2025-11-17T17:30:42.139Z
Learning: In the OWASP/Nest frontend tests (PR #2671 context), wrapper functions like `expectChaptersCountEqualsThree` that simply call another helper with a fixed parameter (e.g., `expectChaptersCountEquals(3)`) are intentionally used to avoid arrow function callbacks in `waitFor` calls. This pattern prevents adding nesting depth that would trigger rule typescript:S2004. Example: `await waitFor(expectChaptersCountEqualsThree)` avoids the extra nesting from `await waitFor(() => expectChaptersCountEquals(3))`.
Applied to files:
frontend/__tests__/unit/global-error.test.tsx
📚 Learning: 2025-06-20T16:12:59.256Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 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/src/components/HealthMetrics.tsxfrontend/src/app/projects/dashboard/page.tsx
📚 Learning: 2025-06-20T16:12:59.256Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 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/src/components/HealthMetrics.tsx
📚 Learning: 2025-07-13T07:31:06.511Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/components/ModuleCard.tsx:53-55
Timestamp: 2025-07-13T07:31:06.511Z
Learning: In Next.js 13+ app router, useRouter from 'next/navigation' does not provide asPath or query properties. Use useParams to extract route parameters and usePathname to get the current pathname instead.
Applied to files:
frontend/src/app/projects/dashboard/metrics/[projectKey]/page.tsxfrontend/src/app/community/snapshots/[id]/page.tsxfrontend/src/app/members/[memberKey]/page.tsxfrontend/src/app/board/[year]/candidates/page.tsx
📚 Learning: 2025-07-11T15:46:58.233Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/components/UserMenu.tsx:56-60
Timestamp: 2025-07-11T15:46:58.233Z
Learning: In the UserMenu component's handleLogout function, both logout() and signOut() calls are intentionally kept despite the useLogout hook already calling signOut internally, as this is necessary for the specific implementation requirements.
Applied to files:
frontend/src/components/UserMenu.tsx
🧬 Code graph analysis (27)
frontend/src/utils/milestoneProgress.ts (1)
backend/apps/github/api/internal/nodes/milestone.py (1)
progress(40-45)
frontend/src/components/DashboardCard.tsx (1)
frontend/src/wrappers/IconWrapper.tsx (1)
IconWrapper(13-13)
frontend/src/components/ProjectsDashboardNavBar.tsx (1)
frontend/src/wrappers/IconWrapper.tsx (1)
IconWrapper(13-13)
frontend/src/wrappers/IconWrapper.tsx (1)
frontend/src/types/icon.ts (1)
Icon(2-4)
frontend/src/app/committees/[committeeKey]/page.tsx (1)
backend/apps/owasp/api/internal/queries/committee.py (1)
committee(14-28)
frontend/src/components/Badges.tsx (1)
frontend/src/wrappers/IconWrapper.tsx (1)
IconWrapper(13-13)
frontend/src/app/projects/[projectKey]/page.tsx (1)
backend/apps/github/models/repository.py (1)
project(162-164)
frontend/src/components/InfoItem.tsx (1)
frontend/src/wrappers/IconWrapper.tsx (1)
IconWrapper(13-13)
frontend/src/components/ProjectsDashboardDropDown.tsx (2)
frontend/src/types/button.ts (1)
Button(4-9)frontend/src/wrappers/IconWrapper.tsx (1)
IconWrapper(13-13)
frontend/src/components/Footer.tsx (2)
frontend/src/utils/constants.ts (1)
footerIcons(31-56)frontend/src/types/link.ts (1)
Link(1-7)
frontend/src/app/projects/dashboard/metrics/page.tsx (1)
frontend/src/wrappers/IconWrapper.tsx (1)
IconWrapper(13-13)
frontend/src/app/page.tsx (4)
frontend/src/components/CalendarButton.tsx (1)
CalendarButton(6-35)frontend/src/utils/dateFormatter.ts (2)
formatDateRange(22-61)formatDate(1-20)frontend/src/components/TruncatedText.tsx (1)
TruncatedText(3-45)frontend/src/wrappers/IconWrapper.tsx (1)
IconWrapper(13-13)
frontend/src/components/StatusBadge.tsx (1)
frontend/src/components/DisplayIcon.tsx (1)
DisplayIcon(7-55)
frontend/src/components/SecondaryCard.tsx (1)
frontend/src/wrappers/IconWrapper.tsx (1)
IconWrapper(13-13)
frontend/src/app/about/page.tsx (3)
backend/apps/github/api/internal/nodes/milestone.py (1)
progress(40-45)frontend/src/utils/aboutData.ts (1)
missionContent(3-8)frontend/src/wrappers/IconWrapper.tsx (1)
IconWrapper(13-13)
frontend/src/components/NavButton.tsx (2)
frontend/src/types/button.ts (1)
NavButtonProps(11-19)frontend/src/wrappers/IconWrapper.tsx (1)
IconWrapper(13-13)
frontend/src/components/Card.tsx (4)
frontend/src/wrappers/IconWrapper.tsx (1)
IconWrapper(13-13)frontend/src/utils/data.ts (1)
level(61-82)frontend/src/utils/urlIconMappings.ts (1)
getSocialIcon(24-50)frontend/src/types/link.ts (1)
Link(1-7)
frontend/src/components/CardDetailsPage.tsx (1)
frontend/src/utils/urlIconMappings.ts (1)
getSocialIcon(24-50)
frontend/src/components/ModuleCard.tsx (2)
frontend/src/components/InfoItem.tsx (1)
TextInfoItem(37-52)frontend/src/utils/dateFormatter.ts (1)
formatDate(1-20)
frontend/src/components/Milestones.tsx (1)
frontend/src/utils/dateFormatter.ts (1)
formatDate(1-20)
frontend/src/components/Release.tsx (1)
frontend/src/utils/dateFormatter.ts (1)
formatDate(1-20)
frontend/src/app/settings/api-keys/page.tsx (1)
frontend/src/types/button.ts (1)
Button(4-9)
frontend/src/components/GeneralCompliantComponent.tsx (1)
frontend/src/wrappers/IconWrapper.tsx (1)
IconWrapper(13-13)
frontend/src/components/MenteeIssues.tsx (1)
frontend/src/wrappers/IconWrapper.tsx (1)
IconWrapper(13-13)
frontend/src/components/RecentPullRequests.tsx (1)
frontend/src/utils/dateFormatter.ts (1)
formatDate(1-20)
frontend/src/components/RecentIssues.tsx (1)
frontend/src/utils/dateFormatter.ts (1)
formatDate(1-20)
frontend/src/components/DisplayIcon.tsx (2)
frontend/src/utils/data.ts (2)
ICONS(36-57)IconKeys(59-59)frontend/src/wrappers/IconWrapper.tsx (1)
IconWrapper(13-13)
🪛 GitHub Check: SonarCloud Code Analysis
frontend/src/app/projects/dashboard/metrics/page.tsx
[warning] 97-97: Extract this nested ternary operation into an independent statement.
frontend/src/components/Header.tsx
[warning] 9-9: 'react-icons/fa' imported multiple times.
[warning] 8-8: 'react-icons/fa' imported multiple times.
|
@sameersharmadev Dec 10 was 3 days ago and it's still in draft 🤷♂️ |
Yeah I'm still kinda new to tests and I dont wanna mess up 😅. Just give me a day I have ~20 test files remaining |
icons changed: fausers->hiusergroup, sun->mdlightmode, famoon->mddarkmode
changes fontawesome mocks with react-icons replacements
sameersharmadev
force-pushed
the
migrate-to-reacticons
branch
from
23169a9 to
69196c8
Compare
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
frontend/src/components/CalendarButton.tsx (1)
7-7: Remove unused hover state and handlers.The
isHoveredstate variable and theonMouseEnter/onMouseLeavehandlers are no longer used after removing the hover-based icon switching logic during the react-icons migration.Apply this diff to remove the dead code:
- const [isHovered, setIsHovered] = useState(false) const {className={className} - onMouseEnter={() => setIsHovered(true)} - onMouseLeave={() => setIsHovered(false)} >Also remove the unused import at the top:
-import { useState } from 'react' import { FaCalendarPlus } from 'react-icons/fa6'Also applies to: 28-29
frontend/__tests__/unit/components/ItemCardList.test.tsx (1)
73-94: Critical: Update SecondaryCard mock to match migrated icon prop type.The mock's icon prop type
{ iconName: string }reflects the old FontAwesome structure, but the actual component now acceptsIconTypefrom react-icons (lines 228, 310 passFaCircleExclamationcomponent). Additionally, line 87's rendering logic{icon.iconName}won't work with IconType components.Apply this diff to align the mock with the react-icons migration:
+import type { IconType } from 'react-icons' + jest.mock('components/SecondaryCard', () => ({ __esModule: true, default: ({ children, title, icon, }: { children: React.ReactNode title?: React.ReactNode - icon?: { iconName: string } + icon?: IconType }) => { + const Icon = icon return ( <div data-testid="secondary-card"> {title && ( <div data-testid="card-title"> - {icon && <span data-testid="card-icon">{icon.iconName}</span>} + {Icon && <span data-testid="card-icon"><Icon /></span>} {title} </div> )} {children} </div> - ) + )} }, }))frontend/__tests__/unit/components/LineChart.test.tsx (1)
50-57: “passes icon to SecondaryCard” doesn’t assert icon forwarding (test currently can’t fail).
SecondaryCardis mocked without accepting/observingicon, and the test only asserts the card exists. Add an assertion on the mock call (or render a marker foricon) so this test actually verifies the prop is forwarded.@@ jest.mock('components/SecondaryCard', () => { - return jest.fn().mockImplementation(({ title, children }) => ( + return jest.fn().mockImplementation(({ title, children, icon: Icon }) => ( <div data-testid="secondary-card"> <div data-testid="card-title">{title}</div> + <div data-testid="card-icon">{Icon ? 'has-icon' : 'no-icon'}</div> <div data-testid="card-content">{children}</div> </div> )) }) @@ it('passes icon to SecondaryCard when provided', () => { - const iconProp = FaChartLine + const iconProp = FaChartLine render(<LineChart {...defaultProps} icon={iconProp} />) const secondaryCard = screen.getByTestId('secondary-card') expect(secondaryCard).toBeInTheDocument() + expect(screen.getByTestId('card-icon')).toHaveTextContent('has-icon') })Also applies to: 106-112
frontend/__tests__/unit/components/StatusBadge.test.tsx (1)
244-248: Fix failing test - react-icons render SVG, not<i>tags.This test expects an
<i>tag, which was used by FontAwesome. React-icons render as<svg>elements instead. This test will fail when executed.Apply this diff to fix the test:
- it('has appropriate semantic structure with icon', () => { + it('has appropriate semantic structure with icon', () => { render(<StatusBadge status="archived" />) const badge = screen.getByText('Archived') - expect(badge).toContainHTML('i') + expect(badge.querySelector('svg')).toBeInTheDocument() })Or simply remove this test if it no longer serves a meaningful purpose, as the "Icon Display" describe block (lines 102-132) already thoroughly tests icon rendering.
frontend/__tests__/unit/pages/UserDetails.test.tsx (1)
553-570: Inconsistent test expectations fordata-css-classattribute.These tests expect
data-css-classvalues withfa-prefix ('fa-medal','fa-shield-alt'), but other tests in this file (lines 642, 672) expect values without the prefix ('medal'). This inconsistency will likely cause test failures.Either all tests should use the
fa-prefix or none should, depending on what the actualBadgescomponent now returns after the migration.Apply this diff to align with the non-prefixed convention used elsewhere in the file:
await waitFor(() => { const contributorBadge = screen.getByTestId('badge-contributor') - expect(contributorBadge).toHaveAttribute('data-css-class', 'fa-medal') + expect(contributorBadge).toHaveAttribute('data-css-class', 'medal') expect(contributorBadge).toHaveAttribute('data-show-tooltip', 'true') const securityBadge = screen.getByTestId('badge-security-expert') - expect(securityBadge).toHaveAttribute('data-css-class', 'fa-shield-alt') + expect(securityBadge).toHaveAttribute('data-css-class', 'shield-alt') expect(securityBadge).toHaveAttribute('data-show-tooltip', 'true') })frontend/__tests__/unit/components/Card.test.tsx (1)
236-239: Stale test ID reference after migration.This test checks for
font-awesome-iconwhich no longer exists after the migration to react-icons. While the test passes (becausequeryByTestIdreturnsnull), it doesn't accurately verify the intended behavior.Apply this diff to use the correct test ID:
it('does not render social section when not provided', () => { render(<Card {...baseProps} />) - expect(screen.queryByTestId('font-awesome-icon')).not.toBeInTheDocument() + expect(screen.queryByTestId('social-icon')).not.toBeInTheDocument() })frontend/__tests__/unit/components/CardDetailsPage.test.tsx (2)
257-277: Fix unusedicondestructure inSecondaryCardmock (likely lint failure).
iconis destructured but unused; prefer_iconor an eslint disable like you did elsewhere.default: ({ title, children, className, - icon, + icon: _icon, ...props }: { icon?: unknown title: React.ReactNode
298-316: Fix unusedicondestructure inToggleableListmock (likely lint failure).
Same pattern: rename to_icon(or disable) to avoidno-unused-vars.default: ({ items, - icon, + icon: _icon, label, ...props }: { items: string[] icon: unknownfrontend/__tests__/unit/components/ProjectTypeDashboardCard.test.tsx (1)
29-58: Fix icon assertions:JSON.stringify(react-icon component)does not match the actual attribute value.The mock sets
data-icon={iconName}whereiconNameis the string"FaHeartPulse", but the assertions expectJSON.stringify(FaHeartPulse)which evaluates to'{"name":"FaHeartPulse"}'. These do not match, causing the tests to fail. Update the assertions to compare against the derived string value instead:it('passes different icons to SecondaryCard correctly', () => { const { rerender } = render(<ProjectTypeDashboardCard {...baseProps} icon={FaHeartPulse} />) let secondaryCard = screen.getByTestId('secondary-card') - expect(secondaryCard).toHaveAttribute('data-icon', JSON.stringify(FaHeartPulse)) + expect(secondaryCard).toHaveAttribute('data-icon', 'FaHeartPulse') rerender(<ProjectTypeDashboardCard {...baseProps} icon={FaExclamationTriangle} />) secondaryCard = screen.getByTestId('secondary-card') - expect(secondaryCard).toHaveAttribute('data-icon', JSON.stringify(FaExclamationTriangle)) + expect(secondaryCard).toHaveAttribute('data-icon', 'FaExclamationTriangle') })Also applies to line 340:
expect(secondaryCard).toHaveAttribute('data-icon', JSON.stringify(FaHeartPulse))should beexpect(secondaryCard).toHaveAttribute('data-icon', 'FaHeartPulse').
♻️ Duplicate comments (1)
frontend/__tests__/unit/components/Search.test.tsx (1)
383-392: Same brittle selector issue (duplicate).This test uses the identical fragile CSS selector approach. Any styling changes to the search icon will cause multiple test failures across this file.
Apply one of the more stable query methods suggested in the previous comment (lines 375-381). Using a consistent approach across all tests will make them easier to maintain.
🧹 Nitpick comments (33)
frontend/__tests__/unit/components/Milestones.test.tsx (1)
54-59: MakegetIconLabelresilient tomemo/forwardReficon shapes (avoid unhelpful"object"labels).
Right now, non-string icons that aren’t plain functions collapse totypeof iconProp(often"object"), which makes failures harder to diagnose and may hide icon-shape regressions.- const getIconLabel = (iconProp: unknown): string => { - if (!iconProp) return 'no-icon' - if (typeof iconProp === 'function' && iconProp.name) return iconProp.name - if (typeof iconProp === 'string') return iconProp - return typeof iconProp - } + const getIconLabel = (iconProp: unknown): string => { + if (iconProp == null) return 'no-icon' + if (typeof iconProp === 'string') return iconProp + + // react-icons are usually functions, but wrappers (memo/forwardRef) can be objects. + const anyIcon = iconProp as any + if (typeof iconProp === 'function' && iconProp.name) return iconProp.name + if (typeof anyIcon?.displayName === 'string' && anyIcon.displayName) return anyIcon.displayName + if (typeof anyIcon?.render?.name === 'string' && anyIcon.render.name) return anyIcon.render.name + if (typeof anyIcon?.constructor?.name === 'string' && anyIcon.constructor.name) return anyIcon.constructor.name + + return typeof iconProp + }frontend/__tests__/unit/components/HealthMetrics.test.tsx (2)
1-1: Prefer importingIconTypein tests when asserting icon identity
Since the production prop moved toreact-icons’IconType, consider typing the test icon asIconType(instead of relying on inference) to keep mocks/fixtures aligned.
55-65: BarChart mock: avoidany+ make icon-name derivation more robust
icon?: anyandprops.icon?.namework, but.namecan be unstable depending on transforms. ConsiderIconType+displayName ?? nameto reduce flakes.+import type { IconType } from 'react-icons' + -jest.mock('components/BarChart', () => (props: { title: string; icon?: any }) => ( +jest.mock('components/BarChart', () => (props: { title: string; icon?: IconType }) => ( <div data-testid="BarChart" data-props={JSON.stringify({ ...props, - icon: props.icon?.name || null, + icon: (props.icon as any)?.displayName ?? props.icon?.name ?? null, })} > {props.title} </div> ))frontend/__tests__/unit/components/BarChart.test.tsx (1)
168-172: Test currently doesn’t validate theiconprop contract (truthy-only mock)Because the
components/SecondaryCardmock only checks{icon && ...}, this test will pass even ifBarChartstarts passing the wrong icon shape during the migration (e.g.,ReactNodevsIconType, or vice versa). Consider rendering the icon in the mock so a contract mismatch fails loudly.Example tweak inside the
components/SecondaryCardmock:- {icon && <div data-testid="card-icon">icon</div>} + {icon && ( + <div data-testid="card-icon"> + {typeof icon === 'function' ? React.createElement(icon as React.ElementType) : (icon as React.ReactNode)} + </div> + )}frontend/__tests__/unit/components/UserMenu.test.tsx (1)
44-50: LGTM! Mock implementation is correct.The
FaGithubmock is properly implemented with correct TypeScript typing and follows Jest best practices. The SVG element withdata-testid="github-icon"aligns perfectly with the test assertions at lines 129 and 440.Optional cleanup: Consider removing the comment on line 45 ("Add this react-icons mock for FaGithub:") as it appears to be an instructional note that isn't needed in the final code.
- -// Add this react-icons mock for FaGithub: jest.mock('react-icons/fa', () => ({ FaGithub: (props: React.SVGProps<SVGSVGElement>) => ( <svg data-testid="github-icon" {...props} /> ), }))frontend/src/components/MenteeIssues.tsx (2)
31-40:getStateColorsupportsmerged, butgetStateIconfalls back toFaClockfor it.
Ifissue.statecan be'merged', the icon will look like “pending/time” while the color is “merged”. Consider explicitly mapping'merged'to a more appropriate icon (even reusingFaCheckCircleis better than the clock).const getStateIcon = (state: string) => { switch (state.toLowerCase()) { case 'open': return FaBug case 'closed': return FaCheckCircle + case 'merged': + return FaCheckCircle default: return FaClock } }
46-50: Empty-state icon selection should key offtitle, notactiveTab(minor robustness).
Right now it works becauserenderIssueListis only called for the active tab, but usingtitlemakesrenderIssueListsafer to reuse independently ofactiveTab.- <IconWrapper - icon={activeTab === 'open' ? FaBug : FaCheckCircle} + <IconWrapper + icon={title.toLowerCase() === 'open' ? FaBug : FaCheckCircle} className="mx-auto mb-2 h-8 w-8 text-gray-400" />frontend/__tests__/unit/pages/About.test.tsx (4)
20-27: AvoidmockRoutershadowing / closure-captured router mock
Right now thejest.mock('next/navigation')factory closes over the file-scopemockRouter(Line 20-27), while the tests mostly use thedescribe-scopedmockRouter(Line 203+). It works because you overrideuseRouterinbeforeEach, but it’s fragile/confusing.Suggested simplification: mock
useRouterwithout capturing any outer variable, and drive the return value only frombeforeEach.-const mockRouter = { - push: jest.fn(), -} - jest.mock('next/navigation', () => ({ ...jest.requireActual('next/navigation'), - useRouter: jest.fn(() => mockRouter), + useRouter: jest.fn(), }))(Then keep your existing
beforeEachmockRouter = { push: jest.fn() }+mockReturnValue(mockRouter)as the single source of truth.)Also applies to: 203-224
29-49: Icon mocks all share the samedata-testid, which can cause accidental ambiguity
Every mocked icon renders<span data-testid="mock-icon" />(Line 29-49). If any test later doesgetByTestId('mock-icon'), it’ll start failing with “multiple elements found”.Consider giving each icon a distinct testid (or render
nullif you never assert on icons):+const mkIcon = (name: string) => () => <span data-testid={`mock-icon-${name}`} /> + jest.mock('react-icons/fa', () => ({ - FaMapSigns: () => <span data-testid="mock-icon" />, - FaTools: () => <span data-testid="mock-icon" />, + FaMapSigns: mkIcon('FaMapSigns'), + FaTools: mkIcon('FaTools'), })) jest.mock('react-icons/fa6', () => ({ - FaCircleCheck: () => <span data-testid="mock-icon" />, + FaCircleCheck: mkIcon('FaCircleCheck'), // ... })) jest.mock('react-icons/hi', () => ({ - HiUserGroup: () => <span data-testid="mock-icon" />, + HiUserGroup: mkIcon('HiUserGroup'), }))
132-161:UserCardmock: drop dualonClick/onclickunless production truly supports both
The mock’sbuttonprop includes bothonClickandonclickand invokes both (Line 143-155). If production only supports one, this can mask bugs (e.g., passing the wrong prop still “works” in tests).If the real prop is
onClick, tighten the mock accordingly:- button?: { label?: string; onClick?: () => void; onclick?: () => void } + button?: { label?: string; onClick?: () => void } ... - button.onClick?.() - button.onclick?.() + button.onClick?.()
163-182:ShowMoreButtonmock: use functional state update; consider honoringlabel
setExpanded(!expanded)(Line 171) is prone to stale state on rapid clicks; use a functional updater. Also,labelis unused, so the mock may drift from the real component’s accessible naming/behavior.- const handleToggle = () => { - setExpanded(!expanded) - onToggle() - } + const handleToggle = () => { + setExpanded((v: boolean) => !v) + onToggle() + } return ( - <button onClick={handleToggle} data-testid="show-more-button"> + <button onClick={handleToggle} data-testid="show-more-button" aria-label={label}> {expanded ? 'Show less' : 'Show more'} </button> )frontend/__tests__/unit/components/GeneralCompliantComponent.test.tsx (1)
17-23: Well-implemented IconWrapper mock.The mock correctly simulates the IconWrapper behavior by rendering the icon component with the provided className and accessibility attributes. The tests will correctly verify the component's behavior with this mock.
Optional refinement: The type definition could be simplified slightly since
classNameis already part ofSVGProps:- IconWrapper: ({ icon: IconComponent, className }: { icon: React.ComponentType<React.SVGProps<SVGSVGElement> & { className?: string }>, className?: string }) => { + IconWrapper: ({ icon: IconComponent, className }: { icon: React.ComponentType<React.SVGProps<SVGSVGElement>>, className?: string }) => {frontend/__tests__/unit/components/DonutBarChart.test.tsx (2)
49-58: Make thedata-iconderivation more robust (IconType vs ReactElement).If
SecondaryCardever receives an element (e.g.,<IconWrapper icon={...} />) instead of an IconType function, the currenticon.displayName/icon.namecheck can become misleading. Consider normalizingicontoicon.typewhen it’s an element and centralizing the logic:+import React from 'react' + +const getIconName = (icon: unknown) => { + const t = + React.isValidElement(icon) ? (icon as any).type : (icon as any) + return t?.displayName ?? t?.name ?? (icon as any) +} + jest.mock('components/SecondaryCard', () => { const MockSecondaryCard = ({ title, icon, children }) => ( - <div data-testid="secondary-card" data-icon={icon && icon.displayName ? icon.displayName : icon && icon.name ? icon.name : icon}> + <div data-testid="secondary-card" data-icon={getIconName(icon)}> <div data-testid="card-title">{title}</div> <div data-testid="card-content">{children}</div> </div> ) MockSecondaryCard.displayName = 'SecondaryCard' return MockSecondaryCard })
101-107: Avoid hardcoded icon-name strings in assertions; derive from the icon component.To keep the test resilient and consistent with the mock’s naming logic, prefer deriving expected
data-iconfrom the passed icon (e.g.,FaChartLine.displayName ?? FaChartLine.name) rather than hardcoding'FaChartLine', and in the icon loop preferdisplayName ?? nameovernamealone.- expect(screen.getByTestId('secondary-card')).toHaveAttribute('data-icon', 'FaChartLine') + expect(screen.getByTestId('secondary-card')).toHaveAttribute( + 'data-icon', + FaChartLine.displayName ?? FaChartLine.name + ) ... - expect(screen.getByTestId('secondary-card')).toHaveAttribute( - 'data-icon', - iconType.name - ) + expect(screen.getByTestId('secondary-card')).toHaveAttribute( + 'data-icon', + iconType.displayName ?? iconType.name + )Also applies to: 311-325
frontend/__tests__/unit/components/SortBy.test.tsx (1)
59-60: Consider a more robust test pattern for icon verification.Using
querySelector('svg')aftergetByLabelTextworks but has some drawbacks:
querySelectoris discouraged by Testing Library as it bypasses accessibility-focused queries and provides poor error messages- Only verifies an SVG exists, not that it's the correct icon for ascending/descending order
- Creates tight coupling to DOM structure (if IconWrapper changes its rendering, tests break)
Consider these alternatives:
- Add a
data-testidto the icon element in the component and usegetByTestId- If react-icons renders with specific attributes, verify those (e.g., check for specific class names, aria attributes, or SVG paths)
- Use
within()from Testing Library for scoped queries:within(iconContainer).getByRole('img', { hidden: true })Example improvement:
- const sortIcon = screen.getByLabelText(/Sort in ascending order/i).querySelector('svg') - expect(sortIcon).toBeInTheDocument() + const iconButton = screen.getByLabelText(/Sort in ascending order/i) + expect(iconButton).toBeInTheDocument() + // Verify the icon is present and rendered correctly + const sortIcon = within(iconButton).getByRole('img', { hidden: true }) + expect(sortIcon).toBeInTheDocument()Or if you add test IDs to IconWrapper:
const sortIcon = screen.getByTestId('sort-icon-asc') expect(sortIcon).toBeInTheDocument()Also applies to: 68-69
frontend/__tests__/unit/components/ModeToggle.test.tsx (1)
26-33: Mock setup looks correct.The mock structure for
react-icons/mdis well-implemented with proper TypeScript typing and props spreading. Thedata-testidattributes enable effective testing.Minor observation: The
data-iconattribute adds semantic information but may be redundant withdata-testid. If it's not used elsewhere in the codebase for styling or other logic, consider removing it to simplify the mock.frontend/__tests__/unit/components/ProjectsDashboardDropDown.test.tsx (2)
5-18: Mock structure looks good, but consider renaming test ID for clarity.The mock correctly simulates react-icons components with proper SVG props spreading. However,
data-testid="font-awesome-icon"is misleading after migrating to react-icons. Consider updating todata-testid="react-icon"for consistency with the migration.This is a minor inconsistency that can be addressed in a follow-up PR if you prefer to minimize changes now.
193-200: Test name is misleading - it actually tests default icon behavior.The test is named "renders without icon when not provided" but the assertion confirms an icon IS rendered (
data-icon="arrow-down-wide-short"). This tests the component's default icon fallback behavior, not the absence of an icon.Consider renaming for clarity:
- it('renders without icon when not provided', () => { + it('renders default ordering icon when icon prop is undefined', () => {frontend/__tests__/unit/components/Footer.test.tsx (2)
167-169: Make the SVG assertion resilient to “extra icons” regressions.
link.querySelector('svg')(Line 167) will pass even if multiple SVGs get rendered in the link later. Consider asserting a single SVG (or at least>= 1viaquerySelectorAllconsistently) to catch accidental duplicates.Example:
-const iconElement = link.querySelector('svg') -expect(iconElement).toBeInTheDocument() +const svgs = link.querySelectorAll('svg') +expect(svgs.length).toBe(1)
214-220: Chevron test doesn’t validate the “correct icon per state” claim.
This test only checks “some svg exists” before/after click (Line 214-220), which doesn’t confirm the chevron actually changes. Either tighten it (e.g., assert exactly one SVG and that the SVG markup changes) or drop the test sincearia-expandedis already validated elsewhere.frontend/__tests__/unit/components/NavButton.test.tsx (1)
52-57: Minor: Redundant type intersection.The type
NavButtonProps & { defaultIcon: typeof FaHome; hoverIcon: typeof FaUser }is redundant sinceNavButtonPropsalready definesdefaultIconandhoverIconasIconType. This works but adds unnecessary complexity.describe('<NavButton />', () => { - const defaultProps: NavButtonProps & { defaultIcon: typeof FaHome; hoverIcon: typeof FaUser } = { + const defaultProps: NavButtonProps = { href: '/test-path', defaultIcon: FaHome, hoverIcon: FaUser, text: 'Test Button', }frontend/src/components/Leaders.tsx (1)
33-33: Consider a more semantically appropriate icon for "View Profile".
FaRightToBrackettypically represents "login" or "sign in" actions, but here it's used for a "View Profile" button. This semantic mismatch may confuse users.Consider alternatives that better convey profile viewing or navigation:
FaUserorFaUserCirclefrom react-icons/fa6 (profile-related)FaArrowRight(navigation/forward action)FaEye(viewing action)Example with FaUser:
-import { FaPersonWalkingArrowRight, FaRightToBracket } from 'react-icons/fa6' +import { FaPersonWalkingArrowRight, FaUser } from 'react-icons/fa6'- icon: <IconWrapper icon={FaRightToBracket} />, + icon: <IconWrapper icon={FaUser} />,frontend/src/app/projects/dashboard/page.tsx (1)
76-80: Consider removing redundant type annotation.The explicit type annotation for
dashboardCardsItemsis redundant—TypeScript will infer the correct type from the array literal.- const dashboardCardsItems: { - title: string - icon: IconType - stats?: string - }[] = [ + const dashboardCardsItems = [frontend/__tests__/unit/components/InfoItem.test.tsx (1)
37-37: Consider using more specific selectors for icon queries.While
document.querySelector('svg')works in these isolated tests, it could be fragile if additional SVG elements are added to InfoItem or its dependencies. Consider using a more specific selector or adding adata-testidto the IconWrapper for more robust queries.Example improvement:
-expect(document.querySelector('svg')).toBeInTheDocument() +expect(screen.getByTestId('info-item-icon')).toBeInTheDocument()Also applies to: 82-84
frontend/__tests__/unit/components/ProgramCard.test.tsx (2)
21-29: Make theIconWrappermock accept/pass through props beyondclassName.
If the real wrapper forwardssize,color,title,aria-*, etc., this mock may hide regressions or diverge from runtime behavior.A small tweak keeps it flexible:
-jest.mock('wrappers/IconWrapper', () => ({ - IconWrapper: ({ icon: IconComponent, className }: { - icon: React.ComponentType<{ className?: string }>, - className?: string - }) => { - return IconComponent ? <IconComponent className={className} /> : null - }, -})) +jest.mock('wrappers/IconWrapper', () => ({ + IconWrapper: ({ + icon: IconComponent, + ...rest + }: { + icon: React.ComponentType<any> + [key: string]: any + }) => (IconComponent ? <IconComponent {...rest} /> : null), +}))
12-19: Consider using a partial mock forreact-icons/fa6to avoid potential breakage if other icons are added.The current full-module mock replaces the entire
react-icons/fa6module. WhileProgramCardandEntityActions(its child component) don't currently import other fa6 icons, this pattern is fragile. If additional fa6 icons are imported later, they'll becomeundefinedand break the test.Consider a partial mock instead:
-jest.mock('react-icons/fa6', () => ({ - FaEye: (props: React.SVGProps<SVGSVGElement>) => ( - <svg data-testid="icon-eye" className={props.className} {...props} /> - ), - FaPencilAlt: (props: React.SVGProps<SVGSVGElement>) => ( - <svg data-testid="icon-edit" className={props.className} {...props} /> - ), -})) +jest.mock('react-icons/fa6', () => { + const actual = jest.requireActual('react-icons/fa6') + return { + ...actual, + FaEye: (props: React.SVGProps<SVGSVGElement>) => ( + <svg data-testid="icon-eye" {...props} /> + ), + FaPencilAlt: (props: React.SVGProps<SVGSVGElement>) => ( + <svg data-testid="icon-edit" {...props} /> + ), + } +})Also note: The
IconWrappermock (lines 21-29) is not used byProgramCardand can be removed.frontend/__tests__/unit/components/Badges.test.tsx (1)
85-104: Test comment mentions transformation that mock doesn't perform.The comment at line 89 states "transforms snake_case to camelCase" but the mock only performs
.replace('fa', ''). The actual transformation logic resides in theBadgescomponent, not the mock. Consider updating the comment to clarify that the transformation is tested via the component's behavior, not the mock's logic.frontend/src/components/DisplayIcon.tsx (1)
36-54: Potential undefined icon whenitemis not inICONS.The component guards with
icons[item]but doesn't verify thatitemexists inICONS. Ificonscontains a key thatICONSdoesn't have, the tooltip will showundefinedand the icon won't render.Consider adding a combined guard:
- return icons[item] ? ( + return icons[item] && ICONS[item as IconKeys] ? (Alternatively, ensure the
Icontype andICONSkeys are always synchronized.frontend/src/components/Card.tsx (1)
82-82: Inconsistent icon rendering pattern.
FaCalendaris rendered directly here, whilelevel.iconusesIconWrapperon line 47. Consider usingIconWrapperconsistently for all icons:- <FaCalendar className="mr-2 h-4 w-4" /> + <IconWrapper icon={FaCalendar} className="mr-2 h-4 w-4" />This is a minor consistency nit; the current code works correctly.
frontend/__tests__/unit/components/InfoBlock.test.tsx (1)
15-26: Make theIconWrappermock closer to production (forwardclassName, avoid extra wrapper).
Right now (Line 17-24)classNamelands on the<span>, while the icon itself gets no props. This can make DOM/className assertions pass even if the real icon element styling breaks.Suggested tweak (keeps your
data-testidbehavior, but applies props to the icon element):jest.mock('wrappers/IconWrapper', () => ({ - IconWrapper: ({ icon: IconComponent, className }: { icon: React.ComponentType<{ className?: string }>, className?: string }) => { + IconWrapper: ({ icon: IconComponent, className }: { icon: React.ComponentType<{ className?: string }>, className?: string }) => { // Extract icon name for test id from component name - const iconName = IconComponent?.name?.replace('Fa', '').toLowerCase() || 'icon' - return IconComponent ? ( - <span data-testid={`icon-${iconName}`} className={className}> - <IconComponent /> - </span> - ) : null + const iconName = + (IconComponent?.displayName ?? IconComponent?.name ?? 'icon') + .replace(/^(Fa|Md|Hi)/, '') + .toLowerCase() + return IconComponent ? ( + <IconComponent data-testid={`icon-${iconName}`} className={className} /> + ) : null }, }))frontend/src/app/page.tsx (2)
7-21: Use a type-only import forIconType+ confirmreact-iconstyping/API for your version.
Line 7 should typically beimport type { IconType } from 'react-icons'to avoid emitting a runtime import.-import { IconType } from 'react-icons' +import type { IconType } from 'react-icons'Also,
getProjectIcon()returning the component (Line 98-110) looks correct forreact-icons, but please confirm this matches the expectedIconWrapperprop type in your codebase.Also applies to: 98-111
8-20: Icon pack mixing (favsfa6): sanity-check visual consistency and intended choices.
You’re importingFaCalendarAlt/FaMapMarkerAltfromreact-icons/fa(Line 8) while most others come fromreact-icons/fa6(Line 9-19). If this isn’t deliberate, consider standardizing on one pack per page/area to avoid subtle style mismatches.No functional issues spotted in the updated call sites (SecondaryCard icons,
IconWrapperusage on project type,HiUserGroupfor groups).Also applies to: 157-219, 260-296, 298-319
frontend/__tests__/unit/components/DisplayIcon.test.tsx (1)
17-20: Remove unused interface with incorrect type definition.The
IconWrapperPropsinterface definesiconas astring, but the actualIconWrappermock (line 47) expectsiconasReact.ComponentType. This interface appears to be leftover from the FontAwesome implementation and is not used by the current mock.Apply this diff to remove the unused interface:
-interface IconWrapperProps { - className?: string - icon: string -} - jest.mock('@heroui/tooltip', () => ({
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (107)
frontend/__tests__/unit/components/AnchorTitle.test.tsx(2 hunks)frontend/__tests__/unit/components/Badges.test.tsx(1 hunks)frontend/__tests__/unit/components/BarChart.test.tsx(2 hunks)frontend/__tests__/unit/components/CalendarButton.test.tsx(5 hunks)frontend/__tests__/unit/components/Card.test.tsx(9 hunks)frontend/__tests__/unit/components/CardDetailsPage.test.tsx(7 hunks)frontend/__tests__/unit/components/DashboardCard.test.tsx(5 hunks)frontend/__tests__/unit/components/DisplayIcon.test.tsx(2 hunks)frontend/__tests__/unit/components/DonutBarChart.test.tsx(28 hunks)frontend/__tests__/unit/components/Footer.test.tsx(5 hunks)frontend/__tests__/unit/components/GeneralCompliantComponent.test.tsx(3 hunks)frontend/__tests__/unit/components/HealthMetrics.test.tsx(3 hunks)frontend/__tests__/unit/components/InfoBlock.test.tsx(39 hunks)frontend/__tests__/unit/components/InfoItem.test.tsx(8 hunks)frontend/__tests__/unit/components/ItemCardList.test.tsx(3 hunks)frontend/__tests__/unit/components/Leaders.test.tsx(1 hunks)frontend/__tests__/unit/components/LineChart.test.tsx(3 hunks)frontend/__tests__/unit/components/LoginPageContent.test.tsx(5 hunks)frontend/__tests__/unit/components/Milestones.test.tsx(1 hunks)frontend/__tests__/unit/components/Modal.test.tsx(3 hunks)frontend/__tests__/unit/components/ModeToggle.test.tsx(3 hunks)frontend/__tests__/unit/components/MultiSearch.test.tsx(8 hunks)frontend/__tests__/unit/components/NavButton.test.tsx(10 hunks)frontend/__tests__/unit/components/NavDropDown.test.tsx(1 hunks)frontend/__tests__/unit/components/ProgramCard.test.tsx(2 hunks)frontend/__tests__/unit/components/ProjectTypeDashboardCard.test.tsx(13 hunks)frontend/__tests__/unit/components/ProjectsDashboardDropDown.test.tsx(9 hunks)frontend/__tests__/unit/components/Search.test.tsx(1 hunks)frontend/__tests__/unit/components/SecondaryCard.test.tsx(3 hunks)frontend/__tests__/unit/components/SingleModuleCard.test.tsx(1 hunks)frontend/__tests__/unit/components/SortBy.test.tsx(1 hunks)frontend/__tests__/unit/components/StatusBadge.test.tsx(1 hunks)frontend/__tests__/unit/components/ToggleableList.test.tsx(3 hunks)frontend/__tests__/unit/components/TopContributorsList.test.tsx(4 hunks)frontend/__tests__/unit/components/UserCard.test.tsx(1 hunks)frontend/__tests__/unit/components/UserMenu.test.tsx(1 hunks)frontend/__tests__/unit/pages/About.test.tsx(2 hunks)frontend/__tests__/unit/pages/ApiKeysPage.test.tsx(0 hunks)frontend/__tests__/unit/pages/ChapterDetails.test.tsx(0 hunks)frontend/__tests__/unit/pages/CommitteeDetails.test.tsx(0 hunks)frontend/__tests__/unit/pages/Header.test.tsx(1 hunks)frontend/__tests__/unit/pages/Home.test.tsx(0 hunks)frontend/__tests__/unit/pages/OrganizationDetails.test.tsx(0 hunks)frontend/__tests__/unit/pages/ProjectDetails.test.tsx(0 hunks)frontend/__tests__/unit/pages/ProjectHealthDashboardMetricsDetails.test.tsx(0 hunks)frontend/__tests__/unit/pages/Projects.test.tsx(0 hunks)frontend/__tests__/unit/pages/ProjectsHealthDashboardMetrics.test.tsx(0 hunks)frontend/__tests__/unit/pages/ProjectsHealthDashboardOverview.test.tsx(0 hunks)frontend/__tests__/unit/pages/RepositoryDetails.test.tsx(0 hunks)frontend/__tests__/unit/pages/UserDetails.test.tsx(3 hunks)frontend/__tests__/unit/pages/Users.test.tsx(0 hunks)frontend/__tests__/unit/utils/milestoneProgress.test.ts(2 hunks)frontend/package.json(0 hunks)frontend/src/app/about/page.tsx(9 hunks)frontend/src/app/board/[year]/candidates/page.tsx(6 hunks)frontend/src/app/chapters/page.tsx(2 hunks)frontend/src/app/committees/[committeeKey]/page.tsx(2 hunks)frontend/src/app/committees/page.tsx(2 hunks)frontend/src/app/community/snapshots/[id]/page.tsx(4 hunks)frontend/src/app/community/snapshots/page.tsx(2 hunks)frontend/src/app/contribute/page.tsx(2 hunks)frontend/src/app/members/[memberKey]/page.tsx(3 hunks)frontend/src/app/members/page.tsx(2 hunks)frontend/src/app/my/mentorship/page.tsx(3 hunks)frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx(7 hunks)frontend/src/app/organizations/[organizationKey]/page.tsx(2 hunks)frontend/src/app/organizations/[organizationKey]/repositories/[repositoryKey]/page.tsx(2 hunks)frontend/src/app/organizations/page.tsx(2 hunks)frontend/src/app/page.tsx(12 hunks)frontend/src/app/projects/[projectKey]/page.tsx(2 hunks)frontend/src/app/projects/dashboard/metrics/[projectKey]/page.tsx(9 hunks)frontend/src/app/projects/dashboard/metrics/page.tsx(4 hunks)frontend/src/app/projects/dashboard/page.tsx(4 hunks)frontend/src/app/projects/page.tsx(2 hunks)frontend/src/app/settings/api-keys/page.tsx(7 hunks)frontend/src/components/AnchorTitle.tsx(2 hunks)frontend/src/components/Badges.tsx(2 hunks)frontend/src/components/BarChart.tsx(2 hunks)frontend/src/components/BreadCrumbs.tsx(2 hunks)frontend/src/components/BreadCrumbsWrapper.tsx(2 hunks)frontend/src/components/CalendarButton.tsx(2 hunks)frontend/src/components/Card.tsx(4 hunks)frontend/src/components/CardDetailsPage.tsx(9 hunks)frontend/src/components/DashboardCard.tsx(2 hunks)frontend/src/components/DisplayIcon.tsx(4 hunks)frontend/src/components/DonutBarChart.tsx(2 hunks)frontend/src/components/EntityActions.tsx(2 hunks)frontend/src/components/Footer.tsx(3 hunks)frontend/src/components/GeneralCompliantComponent.tsx(2 hunks)frontend/src/components/Header.tsx(5 hunks)frontend/src/components/HealthMetrics.tsx(5 hunks)frontend/src/components/InfoBlock.tsx(3 hunks)frontend/src/components/InfoItem.tsx(4 hunks)frontend/src/components/ItemCardList.tsx(2 hunks)frontend/src/components/Leaders.tsx(2 hunks)frontend/src/components/LineChart.tsx(2 hunks)frontend/src/components/LoginPageContent.tsx(4 hunks)frontend/src/components/MenteeContributorsList.tsx(2 hunks)frontend/src/components/MenteeIssues.tsx(6 hunks)frontend/src/components/MetricsPDFButton.tsx(2 hunks)frontend/src/components/Milestones.tsx(2 hunks)frontend/src/components/Modal.tsx(2 hunks)frontend/src/components/ModeToggle.tsx(2 hunks)frontend/src/components/ModuleCard.tsx(3 hunks)frontend/src/components/MultiSearch.tsx(6 hunks)frontend/src/components/NavButton.tsx(3 hunks)frontend/src/components/NavDropDown.tsx(3 hunks)
⛔ Files not processed due to max files limit (33)
- frontend/src/components/Pagination.tsx
- frontend/src/components/ProjectTypeDashboardCard.tsx
- frontend/src/components/ProjectsDashboardDropDown.tsx
- frontend/src/components/ProjectsDashboardNavBar.tsx
- frontend/src/components/RecentIssues.tsx
- frontend/src/components/RecentPullRequests.tsx
- frontend/src/components/RecentReleases.tsx
- frontend/src/components/Release.tsx
- frontend/src/components/RepositoryCard.tsx
- frontend/src/components/ScrollToTop.tsx
- frontend/src/components/Search.tsx
- frontend/src/components/SecondaryCard.tsx
- frontend/src/components/ShowMoreButton.tsx
- frontend/src/components/SingleModuleCard.tsx
- frontend/src/components/SnapshotCard.tsx
- frontend/src/components/SortBy.tsx
- frontend/src/components/StatusBadge.tsx
- frontend/src/components/ToggleableList.tsx
- frontend/src/components/TopContributorsList.tsx
- frontend/src/components/UserCard.tsx
- frontend/src/components/UserMenu.tsx
- frontend/src/types/button.ts
- frontend/src/types/card.ts
- frontend/src/types/icon.ts
- frontend/src/types/level.ts
- frontend/src/utils/constants.ts
- frontend/src/utils/data.ts
- frontend/src/utils/milestoneProgress.ts
- frontend/src/utils/urlIconMappings.ts
- frontend/src/utils/utility.ts
- frontend/src/wrappers/FontAwesomeIconWrapper.tsx
- frontend/src/wrappers/IconWrapper.tsx
- frontend/tsconfig.json
💤 Files with no reviewable changes (13)
- frontend/tests/unit/pages/ApiKeysPage.test.tsx
- frontend/tests/unit/pages/CommitteeDetails.test.tsx
- frontend/tests/unit/pages/Projects.test.tsx
- frontend/tests/unit/pages/OrganizationDetails.test.tsx
- frontend/tests/unit/pages/ProjectsHealthDashboardMetrics.test.tsx
- frontend/tests/unit/pages/RepositoryDetails.test.tsx
- frontend/tests/unit/pages/Users.test.tsx
- frontend/tests/unit/pages/ProjectDetails.test.tsx
- frontend/tests/unit/pages/Home.test.tsx
- frontend/tests/unit/pages/ProjectsHealthDashboardOverview.test.tsx
- frontend/package.json
- frontend/tests/unit/pages/ProjectHealthDashboardMetricsDetails.test.tsx
- frontend/tests/unit/pages/ChapterDetails.test.tsx
🚧 Files skipped from review as they are similar to previous changes (33)
- frontend/src/app/projects/dashboard/metrics/page.tsx
- frontend/src/components/EntityActions.tsx
- frontend/src/components/MetricsPDFButton.tsx
- frontend/src/app/organizations/[organizationKey]/repositories/[repositoryKey]/page.tsx
- frontend/src/components/MultiSearch.tsx
- frontend/src/components/Milestones.tsx
- frontend/src/components/Header.tsx
- frontend/src/components/HealthMetrics.tsx
- frontend/src/components/ModuleCard.tsx
- frontend/src/components/DonutBarChart.tsx
- frontend/src/components/Footer.tsx
- frontend/src/components/NavButton.tsx
- frontend/src/app/community/snapshots/page.tsx
- frontend/src/app/projects/dashboard/metrics/[projectKey]/page.tsx
- frontend/src/components/ModeToggle.tsx
- frontend/src/app/organizations/[organizationKey]/page.tsx
- frontend/src/app/organizations/page.tsx
- frontend/src/components/ItemCardList.tsx
- frontend/src/components/NavDropDown.tsx
- frontend/src/components/CardDetailsPage.tsx
- frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx
- frontend/src/app/projects/[projectKey]/page.tsx
- frontend/src/app/settings/api-keys/page.tsx
- frontend/src/components/AnchorTitle.tsx
- frontend/src/app/contribute/page.tsx
- frontend/src/components/LoginPageContent.tsx
- frontend/src/app/committees/page.tsx
- frontend/src/app/members/page.tsx
- frontend/src/components/DashboardCard.tsx
- frontend/src/app/community/snapshots/[id]/page.tsx
- frontend/src/components/BreadCrumbs.tsx
- frontend/src/components/MenteeContributorsList.tsx
- frontend/src/app/about/page.tsx
🧰 Additional context used
🧠 Learnings (12)
📚 Learning: 2025-11-17T16:47:05.578Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:427-427
Timestamp: 2025-11-17T16:47:05.578Z
Learning: In the frontend test files for the OWASP/Nest repository, `expect(true).toBe(true)` no-op assertions may be intentionally added as workarounds when ESLint rule jest/expect-expect doesn't detect expectations inside helper functions or waitFor callbacks. These can be resolved by configuring the ESLint rule's assertFunctionNames option to recognize custom assertion helpers instead of flagging them as redundant.
Applied to files:
frontend/__tests__/unit/components/Search.test.tsxfrontend/__tests__/unit/components/LineChart.test.tsxfrontend/__tests__/unit/components/LoginPageContent.test.tsxfrontend/__tests__/unit/components/Footer.test.tsxfrontend/__tests__/unit/components/UserCard.test.tsxfrontend/__tests__/unit/components/NavButton.test.tsxfrontend/__tests__/unit/utils/milestoneProgress.test.tsfrontend/__tests__/unit/components/MultiSearch.test.tsxfrontend/__tests__/unit/components/DisplayIcon.test.tsxfrontend/__tests__/unit/pages/UserDetails.test.tsxfrontend/__tests__/unit/components/NavDropDown.test.tsxfrontend/__tests__/unit/components/Card.test.tsxfrontend/__tests__/unit/components/TopContributorsList.test.tsxfrontend/__tests__/unit/pages/About.test.tsxfrontend/__tests__/unit/components/ProjectsDashboardDropDown.test.tsxfrontend/__tests__/unit/components/HealthMetrics.test.tsxfrontend/__tests__/unit/components/ProjectTypeDashboardCard.test.tsxfrontend/__tests__/unit/components/InfoBlock.test.tsxfrontend/__tests__/unit/components/Badges.test.tsxfrontend/__tests__/unit/components/CardDetailsPage.test.tsx
📚 Learning: 2025-07-03T03:08:03.290Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1703
File: frontend/src/components/BarChart.tsx:33-46
Timestamp: 2025-07-03T03:08:03.290Z
Learning: In the OWASP Nest project's BarChart component (frontend/src/components/BarChart.tsx), the days and requirements arrays are guaranteed to always have the same length in their use cases, so input validation for array length matching is not needed.
Applied to files:
frontend/__tests__/unit/components/LineChart.test.tsxfrontend/__tests__/unit/components/BarChart.test.tsxfrontend/src/components/BarChart.tsxfrontend/__tests__/unit/components/DonutBarChart.test.tsxfrontend/__tests__/unit/components/HealthMetrics.test.tsx
📚 Learning: 2025-06-21T12:21:32.372Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1633
File: frontend/src/components/GradientRadialChart.tsx:67-116
Timestamp: 2025-06-21T12:21:32.372Z
Learning: The react-apexcharts Chart component does not support ARIA attributes like aria-label and role as direct props. To add accessibility attributes to ApexCharts in React, wrap the Chart component in a container div with the appropriate ARIA attributes.
Applied to files:
frontend/__tests__/unit/components/LineChart.test.tsxfrontend/__tests__/unit/components/BarChart.test.tsxfrontend/src/components/BarChart.tsxfrontend/__tests__/unit/components/DonutBarChart.test.tsxfrontend/__tests__/unit/components/Card.test.tsxfrontend/src/app/projects/dashboard/page.tsxfrontend/src/components/LineChart.tsxfrontend/__tests__/unit/components/HealthMetrics.test.tsx
📚 Learning: 2025-09-17T02:42:41.928Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 2288
File: frontend/src/components/ActionButton.tsx:0-0
Timestamp: 2025-09-17T02:42:41.928Z
Learning: In frontend/src/components/ActionButton.tsx, the user Rajgupta36 intentionally changed text-blue-600 to text-[#1D7BD7] to align the text color with the border color (#1D7BD7) for visual consistency, prioritizing design alignment over theme tokens.
Applied to files:
frontend/__tests__/unit/components/SecondaryCard.test.tsxfrontend/__tests__/unit/components/BarChart.test.tsxfrontend/src/components/Modal.tsxfrontend/src/app/projects/page.tsxfrontend/__tests__/unit/components/ModeToggle.test.tsxfrontend/src/app/my/mentorship/page.tsxfrontend/src/app/members/[memberKey]/page.tsxfrontend/__tests__/unit/components/InfoItem.test.tsxfrontend/src/components/DisplayIcon.tsxfrontend/src/app/board/[year]/candidates/page.tsxfrontend/src/components/CalendarButton.tsxfrontend/src/components/Leaders.tsxfrontend/__tests__/unit/components/Modal.test.tsxfrontend/src/app/committees/[committeeKey]/page.tsxfrontend/src/components/Card.tsxfrontend/__tests__/unit/components/NavButton.test.tsxfrontend/__tests__/unit/components/CalendarButton.test.tsxfrontend/__tests__/unit/components/AnchorTitle.test.tsxfrontend/__tests__/unit/components/DashboardCard.test.tsxfrontend/__tests__/unit/pages/UserDetails.test.tsxfrontend/src/app/chapters/page.tsxfrontend/__tests__/unit/components/StatusBadge.test.tsxfrontend/__tests__/unit/components/Card.test.tsxfrontend/src/components/Badges.tsxfrontend/src/components/BreadCrumbsWrapper.tsxfrontend/__tests__/unit/components/UserMenu.test.tsxfrontend/__tests__/unit/components/TopContributorsList.test.tsxfrontend/src/components/InfoItem.tsxfrontend/src/app/projects/dashboard/page.tsxfrontend/src/app/page.tsxfrontend/__tests__/unit/components/ItemCardList.test.tsxfrontend/__tests__/unit/components/ProjectTypeDashboardCard.test.tsxfrontend/__tests__/unit/components/InfoBlock.test.tsxfrontend/__tests__/unit/components/Badges.test.tsx
📚 Learning: 2025-07-12T17:36:57.255Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.255Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.
Applied to files:
frontend/__tests__/unit/components/SecondaryCard.test.tsxfrontend/__tests__/unit/components/LoginPageContent.test.tsxfrontend/__tests__/unit/components/Footer.test.tsxfrontend/__tests__/unit/components/UserCard.test.tsxfrontend/__tests__/unit/components/Modal.test.tsxfrontend/__tests__/unit/components/NavButton.test.tsxfrontend/__tests__/unit/components/DashboardCard.test.tsxfrontend/__tests__/unit/pages/UserDetails.test.tsxfrontend/__tests__/unit/components/DonutBarChart.test.tsxfrontend/__tests__/unit/components/StatusBadge.test.tsxfrontend/__tests__/unit/components/Card.test.tsxfrontend/__tests__/unit/components/ToggleableList.test.tsxfrontend/__tests__/unit/pages/Header.test.tsxfrontend/__tests__/unit/components/TopContributorsList.test.tsxfrontend/__tests__/unit/pages/About.test.tsxfrontend/__tests__/unit/components/ProjectsDashboardDropDown.test.tsxfrontend/__tests__/unit/components/ProgramCard.test.tsxfrontend/__tests__/unit/components/ProjectTypeDashboardCard.test.tsxfrontend/__tests__/unit/components/InfoBlock.test.tsxfrontend/__tests__/unit/components/Badges.test.tsxfrontend/__tests__/unit/components/CardDetailsPage.test.tsx
📚 Learning: 2025-07-12T17:12:25.807Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/__tests__/unit/pages/ProgramDetails.test.tsx:35-0
Timestamp: 2025-07-12T17:12:25.807Z
Learning: In React applications, button elements should always have an explicit type attribute (type="button", type="submit", or type="reset") to prevent unintended form submission behavior, even when the button appears to be outside of a form context. The default type is "submit" which can cause unexpected behavior.
Applied to files:
frontend/src/app/projects/page.tsxfrontend/__tests__/unit/components/Modal.test.tsxfrontend/src/app/chapters/page.tsx
📚 Learning: 2025-07-13T11:34:31.823Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/page.tsx:14-14
Timestamp: 2025-07-13T11:34:31.823Z
Learning: In the Next.js frontend mentorship application, there are two distinct types for authentication-related data: ExtendedSession for useSession hook (containing accessToken and user.login properties) and UserRolesData for useUserRoles hook (containing currentUserRoles.roles array). The correct access pattern for GitHub username is `(session as ExtendedSession)?.user?.login`.
Applied to files:
frontend/src/app/my/mentorship/page.tsxfrontend/src/app/members/[memberKey]/page.tsxfrontend/__tests__/unit/components/SingleModuleCard.test.tsx
📚 Learning: 2025-07-13T11:29:25.245Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/page.tsx:59-61
Timestamp: 2025-07-13T11:29:25.245Z
Learning: In Next.js 13+ app router, components with the 'use client' directive run entirely on the client side and don't require window object existence checks or SSR hydration considerations. Direct access to window.location and other browser APIs is safe in client components.
Applied to files:
frontend/src/app/my/mentorship/page.tsxfrontend/src/app/members/[memberKey]/page.tsxfrontend/src/app/committees/[committeeKey]/page.tsxfrontend/src/components/BreadCrumbsWrapper.tsxfrontend/src/app/page.tsx
📚 Learning: 2025-07-13T07:31:06.511Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/components/ModuleCard.tsx:53-55
Timestamp: 2025-07-13T07:31:06.511Z
Learning: In Next.js 13+ app router, useRouter from 'next/navigation' does not provide asPath or query properties. Use useParams to extract route parameters and usePathname to get the current pathname instead.
Applied to files:
frontend/src/app/members/[memberKey]/page.tsxfrontend/src/app/board/[year]/candidates/page.tsxfrontend/__tests__/unit/components/SingleModuleCard.test.tsx
📚 Learning: 2025-11-17T17:30:42.139Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:169-171
Timestamp: 2025-11-17T17:30:42.139Z
Learning: In the OWASP/Nest frontend tests (PR #2671 context), wrapper functions like `expectChaptersCountEqualsThree` that simply call another helper with a fixed parameter (e.g., `expectChaptersCountEquals(3)`) are intentionally used to avoid arrow function callbacks in `waitFor` calls. This pattern prevents adding nesting depth that would trigger rule typescript:S2004. Example: `await waitFor(expectChaptersCountEqualsThree)` avoids the extra nesting from `await waitFor(() => expectChaptersCountEquals(3))`.
Applied to files:
frontend/__tests__/unit/components/Footer.test.tsxfrontend/__tests__/unit/components/MultiSearch.test.tsxfrontend/__tests__/unit/components/ProjectsDashboardDropDown.test.tsx
📚 Learning: 2025-06-20T16:12:59.256Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 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/DonutBarChart.test.tsxfrontend/__tests__/unit/components/HealthMetrics.test.tsxfrontend/__tests__/unit/components/ProjectTypeDashboardCard.test.tsx
📚 Learning: 2025-06-20T16:12:59.256Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 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/src/app/projects/dashboard/page.tsxfrontend/__tests__/unit/components/HealthMetrics.test.tsxfrontend/__tests__/unit/components/ProjectTypeDashboardCard.test.tsx
🧬 Code graph analysis (12)
frontend/src/components/DisplayIcon.tsx (2)
frontend/src/utils/data.ts (2)
ICONS(36-57)IconKeys(59-59)frontend/src/wrappers/IconWrapper.tsx (1)
IconWrapper(13-13)
frontend/src/components/GeneralCompliantComponent.tsx (1)
frontend/src/wrappers/IconWrapper.tsx (1)
IconWrapper(13-13)
frontend/__tests__/unit/components/Footer.test.tsx (1)
frontend/src/utils/constants.ts (1)
footerIcons(31-56)
frontend/src/components/Leaders.tsx (1)
frontend/src/wrappers/IconWrapper.tsx (1)
IconWrapper(13-13)
frontend/src/components/Card.tsx (3)
frontend/src/wrappers/IconWrapper.tsx (1)
IconWrapper(13-13)frontend/src/utils/data.ts (1)
level(61-82)frontend/src/utils/urlIconMappings.ts (1)
getSocialIcon(24-50)
frontend/__tests__/unit/components/NavButton.test.tsx (1)
frontend/src/types/button.ts (1)
NavButtonProps(11-19)
frontend/__tests__/unit/components/CalendarButton.test.tsx (1)
frontend/src/components/CalendarButton.tsx (1)
CalendarButton(6-35)
frontend/__tests__/unit/utils/milestoneProgress.test.ts (1)
frontend/src/utils/milestoneProgress.ts (1)
getMilestoneProgressIcon(14-22)
frontend/src/components/Badges.tsx (1)
frontend/src/wrappers/IconWrapper.tsx (1)
IconWrapper(13-13)
frontend/__tests__/unit/components/TopContributorsList.test.tsx (1)
frontend/src/types/icon.ts (1)
Icon(1-3)
frontend/src/components/InfoItem.tsx (1)
frontend/src/wrappers/IconWrapper.tsx (1)
IconWrapper(13-13)
frontend/src/components/InfoBlock.tsx (1)
frontend/src/wrappers/IconWrapper.tsx (1)
IconWrapper(13-13)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/__tests__/unit/components/Leaders.test.tsx (1)
15-19: IconWrapper mock now mirrors real prop-forwarding (fixes prior false-positive risk).
This matches the earlier recommendation to forwardclassNameand remaining props into the rendered icon component.
🧹 Nitpick comments (3)
frontend/__tests__/unit/components/DonutBarChart.test.tsx (1)
51-51: Simplify icon name extraction logic.The nested ternary chain is verbose and could be more concise using optional chaining and nullish coalescing.
Apply this diff to simplify the expression:
- <div data-testid="secondary-card" data-icon={icon && icon.displayName ? icon.displayName : icon && icon.name ? icon.name : icon}> + <div data-testid="secondary-card" data-icon={icon?.displayName ?? icon?.name ?? String(icon)}>frontend/__tests__/unit/components/CardDetailsPage.test.tsx (1)
118-139: Consider more specific icon typing (optional improvement).The mock correctly accepts the
iconprop (typed asunknown), but you could improve type safety by usingReact.ComponentTypeorIconTypefromreact-iconsfor better alignment with the actual component signature.Example:
+import type { IconType } from 'react-icons' + jest.mock('components/InfoBlock', () => ({ __esModule: true, default: ({ icon: _icon, pluralizedName, unit, value, className, ...props }: { - icon: unknown + icon: IconType pluralizedName?: string unit?: string value: number className?: string [key: string]: unknown }) => (frontend/__tests__/unit/components/NavButton.test.tsx (1)
52-57: Consider simplifying the type annotation.The type intersection
NavButtonProps & { defaultIcon: typeof FaHome; hoverIcon: typeof FaUser }is verbose. SinceNavButtonPropsalready defines both icons asIconType, you can simplify to justNavButtonProps:- const defaultProps: NavButtonProps & { defaultIcon: typeof FaHome; hoverIcon: typeof FaUser } = { + const defaultProps: NavButtonProps = {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
frontend/__tests__/unit/components/AnchorTitle.test.tsx(1 hunks)frontend/__tests__/unit/components/CardDetailsPage.test.tsx(7 hunks)frontend/__tests__/unit/components/DisplayIcon.test.tsx(2 hunks)frontend/__tests__/unit/components/DonutBarChart.test.tsx(27 hunks)frontend/__tests__/unit/components/Leaders.test.tsx(1 hunks)frontend/__tests__/unit/components/NavButton.test.tsx(10 hunks)frontend/__tests__/unit/components/ProgramCard.test.tsx(2 hunks)frontend/__tests__/unit/components/Search.test.tsx(1 hunks)frontend/__tests__/unit/components/SecondaryCard.test.tsx(3 hunks)frontend/__tests__/unit/components/StatusBadge.test.tsx(1 hunks)frontend/__tests__/unit/components/ToggleableList.test.tsx(3 hunks)frontend/src/components/LineChart.tsx(2 hunks)frontend/src/components/Search.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- frontend/tests/unit/components/AnchorTitle.test.tsx
- frontend/src/components/Search.tsx
- frontend/tests/unit/components/Search.test.tsx
- frontend/tests/unit/components/DisplayIcon.test.tsx
- frontend/tests/unit/components/ProgramCard.test.tsx
- frontend/src/components/LineChart.tsx
- frontend/tests/unit/components/SecondaryCard.test.tsx
- frontend/tests/unit/components/ToggleableList.test.tsx
- frontend/tests/unit/components/StatusBadge.test.tsx
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-07-03T03:08:03.290Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1703
File: frontend/src/components/BarChart.tsx:33-46
Timestamp: 2025-07-03T03:08:03.290Z
Learning: In the OWASP Nest project's BarChart component (frontend/src/components/BarChart.tsx), the days and requirements arrays are guaranteed to always have the same length in their use cases, so input validation for array length matching is not needed.
Applied to files:
frontend/__tests__/unit/components/DonutBarChart.test.tsx
📚 Learning: 2025-07-12T17:36:57.255Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.255Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.
Applied to files:
frontend/__tests__/unit/components/DonutBarChart.test.tsxfrontend/__tests__/unit/components/NavButton.test.tsxfrontend/__tests__/unit/components/CardDetailsPage.test.tsx
📚 Learning: 2025-06-21T12:21:32.372Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1633
File: frontend/src/components/GradientRadialChart.tsx:67-116
Timestamp: 2025-06-21T12:21:32.372Z
Learning: The react-apexcharts Chart component does not support ARIA attributes like aria-label and role as direct props. To add accessibility attributes to ApexCharts in React, wrap the Chart component in a container div with the appropriate ARIA attributes.
Applied to files:
frontend/__tests__/unit/components/DonutBarChart.test.tsx
📚 Learning: 2025-06-20T16:12:59.256Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 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/DonutBarChart.test.tsx
📚 Learning: 2025-09-17T02:42:41.928Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 2288
File: frontend/src/components/ActionButton.tsx:0-0
Timestamp: 2025-09-17T02:42:41.928Z
Learning: In frontend/src/components/ActionButton.tsx, the user Rajgupta36 intentionally changed text-blue-600 to text-[#1D7BD7] to align the text color with the border color (#1D7BD7) for visual consistency, prioritizing design alignment over theme tokens.
Applied to files:
frontend/__tests__/unit/components/NavButton.test.tsx
📚 Learning: 2025-11-17T16:47:05.578Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:427-427
Timestamp: 2025-11-17T16:47:05.578Z
Learning: In the frontend test files for the OWASP/Nest repository, `expect(true).toBe(true)` no-op assertions may be intentionally added as workarounds when ESLint rule jest/expect-expect doesn't detect expectations inside helper functions or waitFor callbacks. These can be resolved by configuring the ESLint rule's assertFunctionNames option to recognize custom assertion helpers instead of flagging them as redundant.
Applied to files:
frontend/__tests__/unit/components/NavButton.test.tsxfrontend/__tests__/unit/components/Leaders.test.tsxfrontend/__tests__/unit/components/CardDetailsPage.test.tsx
📚 Learning: 2025-07-12T17:12:25.807Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/__tests__/unit/pages/ProgramDetails.test.tsx:35-0
Timestamp: 2025-07-12T17:12:25.807Z
Learning: In React applications, button elements should always have an explicit type attribute (type="button", type="submit", or type="reset") to prevent unintended form submission behavior, even when the button appears to be outside of a form context. The default type is "submit" which can cause unexpected behavior.
Applied to files:
frontend/__tests__/unit/components/NavButton.test.tsx
🧬 Code graph analysis (1)
frontend/__tests__/unit/components/NavButton.test.tsx (1)
frontend/src/types/button.ts (1)
NavButtonProps(11-19)
🔇 Additional comments (8)
frontend/__tests__/unit/components/DonutBarChart.test.tsx (2)
5-5: LGTM! Icon imports migrated correctly.The migration from FontAwesome to react-icons is implemented correctly with appropriate icon imports from
react-icons/fa.
74-456: Excellent test coverage and migration approach.The test suite is comprehensive, covering all aspects of the DonutBarChart component including rendering, data handling, configuration, theming, structure, prop validation, performance, and integration. The migration from string-based icons to component-based icons is implemented correctly and consistently throughout all tests.
frontend/__tests__/unit/components/CardDetailsPage.test.tsx (3)
6-6: LGTM! Icon imports align with react-icons migration.The import of Font Awesome icons from
react-icons/fa6is correct and consistent with the broader PR objective to migrate from@fortawesomepackages toreact-icons.
62-69: Previous review feedback addressed correctly.The mock now encodes the URL using
encodeURIComponentto create CSS-safe test IDs, resolving the brittle selector concern from the previous review.
400-413: LGTM! Test data correctly uses react-icons components.The migration from FontAwesome icon definitions to react-icons component references (
FaCode,FaTags) is correct and aligns with the new icon prop pattern where icons are renderable components.frontend/__tests__/unit/components/NavButton.test.tsx (3)
11-38: Mock setup looks good.The mocks are properly structured: react-icons components are mocked before imports, IconWrapper correctly passes through the icon component, and data-testid attributes enable reliable test queries.
364-368: Previous tagName case issue resolved.The assertion now correctly expects
'SVG'(uppercase), which matches the DOM'stagNamereturn value. The previous review concern has been addressed.
78-391: Test logic is comprehensive and correct.The test suite properly verifies:
- Icon rendering with data-testid queries
- State changes on hover/leave events
- Color and transform class application
- DOM structure and accessibility
- Edge cases and rapid state changes
The migration from FontAwesome to react-icons is complete and all assertions align with the new icon implementation.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/__tests__/unit/components/NavButton.test.tsx (1)
359-368: Simplify for consistency: .toUpperCase() is redundant.Line 366 uses
.toUpperCase()which addresses the past review comment, and the test will pass correctly. However, sincetagNamealways returns uppercase in the DOM, the normalization is redundant. Line 367 correctly compares directly to'SPAN'without normalization.Apply this diff for consistency:
// Should have 2 children: react-icons icon and span expect(children).toHaveLength(2) - expect(children[0].tagName.toUpperCase()).toBe('SVG') // React-icons icon + expect(children[0].tagName).toBe('SVG') // React-icons icon expect(children[1].tagName).toBe('SPAN') // Text span
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
frontend/__tests__/unit/components/NavButton.test.tsx(10 hunks)frontend/src/components/ChapterMap.tsx(2 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-09-17T02:42:41.928Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 2288
File: frontend/src/components/ActionButton.tsx:0-0
Timestamp: 2025-09-17T02:42:41.928Z
Learning: In frontend/src/components/ActionButton.tsx, the user Rajgupta36 intentionally changed text-blue-600 to text-[#1D7BD7] to align the text color with the border color (#1D7BD7) for visual consistency, prioritizing design alignment over theme tokens.
Applied to files:
frontend/src/components/ChapterMap.tsxfrontend/__tests__/unit/components/NavButton.test.tsx
📚 Learning: 2025-07-13T11:29:25.245Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/page.tsx:59-61
Timestamp: 2025-07-13T11:29:25.245Z
Learning: In Next.js 13+ app router, components with the 'use client' directive run entirely on the client side and don't require window object existence checks or SSR hydration considerations. Direct access to window.location and other browser APIs is safe in client components.
Applied to files:
frontend/src/components/ChapterMap.tsx
📚 Learning: 2025-07-12T17:36:57.255Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.255Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.
Applied to files:
frontend/__tests__/unit/components/NavButton.test.tsx
📚 Learning: 2025-11-17T16:47:05.578Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:427-427
Timestamp: 2025-11-17T16:47:05.578Z
Learning: In the frontend test files for the OWASP/Nest repository, `expect(true).toBe(true)` no-op assertions may be intentionally added as workarounds when ESLint rule jest/expect-expect doesn't detect expectations inside helper functions or waitFor callbacks. These can be resolved by configuring the ESLint rule's assertFunctionNames option to recognize custom assertion helpers instead of flagging them as redundant.
Applied to files:
frontend/__tests__/unit/components/NavButton.test.tsx
📚 Learning: 2025-07-12T17:12:25.807Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/__tests__/unit/pages/ProgramDetails.test.tsx:35-0
Timestamp: 2025-07-12T17:12:25.807Z
Learning: In React applications, button elements should always have an explicit type attribute (type="button", type="submit", or type="reset") to prevent unintended form submission behavior, even when the button appears to be outside of a form context. The default type is "submit" which can cause unexpected behavior.
Applied to files:
frontend/__tests__/unit/components/NavButton.test.tsx
🧬 Code graph analysis (1)
frontend/__tests__/unit/components/NavButton.test.tsx (1)
frontend/src/types/button.ts (1)
NavButtonProps(11-19)
🔇 Additional comments (8)
frontend/src/components/ChapterMap.tsx (1)
6-6: Icon import migration looks correct.FaLocationDotfromreact-icons/fa6matches the “Font Awesome 6” set and aligns with the PR goal.frontend/__tests__/unit/components/NavButton.test.tsx (7)
11-38: LGTM! Mock setup is correct and well-structured.The mock setup properly configures react-icons components with data-testid attributes and implements a reasonable IconWrapper pass-through mock. The ordering (mocks before imports) is correct for Jest.
52-62: LGTM! Test setup correctly migrated to react-icons.The defaultProps and helper function properly use IconType components (FaHome, FaUser) instead of Font Awesome icon objects, aligning with the react-icons migration.
78-82: LGTM! Icon rendering tests properly migrated.Tests correctly use data-testid attributes to locate react-icons components and verify both default and hover icon rendering with color styles.
Also applies to: 106-118
130-187: LGTM! Comprehensive hover state testing.The tests thoroughly verify icon switching, hover transformations, and state management using data-testid queries. All hover state transitions are properly covered.
266-284: LGTM! Default values and edge cases properly tested.Tests verify the component handles missing optional props gracefully and maintains correct behavior with default values.
371-391: LGTM! Performance and state management tests are comprehensive.Tests properly verify memory leak prevention and correct handling of rapid hover state changes with appropriate final state assertions.
1-393: Excellent test migration to react-icons!The test file has been comprehensively updated to work with react-icons instead of Font Awesome. The mock setup is correct, test coverage is extensive (rendering, props, hover effects, styling, edge cases, accessibility, composition, and performance), and all assertions properly use data-testid attributes to locate icon components. The migration successfully maintains test quality while adapting to the new icon library.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
arkid15r
left a comment
arkid15r
left a comment
There was a problem hiding this comment.
Yes, it's a lot of work -- I bumped points for this task.
However, you should be careful with other work merging. Please make sure you're not losing any.
github-actions
bot
added
docs
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
frontend/__tests__/e2e/helpers/mockDashboardCookies.ts (1)
17-37: LGTM — Clean refactor, though unrelated to icon migration.The switch-to-if-else refactor correctly preserves the original behavior and simplifies the logic for two branches. The 'SyncDjangoSession' operation continues to return the expected payload, and all other operations return
mockDashboardData.Note: This change appears unrelated to the PR's icon migration objectives. While bundling small cleanups is common, ensure it doesn't inadvertently introduce risk or confuse reviewers tracking icon-specific changes.
Optional: Add defensive null check.
Consider adding a null-safe check for
postData.operationNameto guard against unexpected inputs:- if (postData.operationName === 'SyncDjangoSession') { + if (postData?.operationName === 'SyncDjangoSession') {This is purely defensive — in test mocks where input is controlled, the risk is negligible.
frontend/src/app/about/page.tsx (1)
294-294: Verify millify output format for project statistics.The rounding logic
Math.floor(stat.value / 10 || 0) * 10intentionally rounds down to the nearest 10 before applyingmillify. While this produces approximate values (e.g., 1234 → "1.2K+"), ensure that:
- The default
millifyoptions produce the desired format (e.g., precision, units).- The rounding behavior is tested for edge cases (values < 10, exactly 0, very large numbers).
Consider explicitly specifying millify options for consistent formatting:
-{millify(Math.floor(stat.value / 10 || 0) * 10)}+ +{millify(Math.floor(stat.value / 10 || 0) * 10, { precision: 1 })}+Or add a test to verify the output format matches expectations.
frontend/src/app/page.tsx (1)
252-289: Minor styling inconsistency in HiUserGroup icons.The
HiUserGroupicon appears twice in similar contexts (chapter leaders and project leaders), but with inconsistent margin styling:
- Line 252 (New Chapters):
className="mr-2 h-4 w-4 shrink-0"- Line 289 (New Projects):
className="h-4 w-4 shrink-0"(missingmr-2)Consider applying consistent spacing for visual uniformity:
<div className="mt-1 flex items-center gap-x-2 text-sm text-gray-600 dark:text-gray-400"> - <HiUserGroup className="h-4 w-4 shrink-0" /> + <HiUserGroup className="mr-2 h-4 w-4 shrink-0" /> <LeadersList leaders={String(project.leaders)} /> </div>
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
backend/data/nest.json.gzis excluded by!**/*.gzbackend/poetry.lockis excluded by!**/*.lockcspell/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamldocs/poetry.lockis excluded by!**/*.lockfrontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (21)
.github/PULL_REQUEST_TEMPLATE.md(1 hunks).github/workflows/run-code-ql.yaml(2 hunks)backend/apps/slack/common/handlers/chapters.py(2 hunks)backend/apps/slack/common/handlers/users.py(0 hunks)backend/tests/apps/github/management/commands/github_sync_user_test.py(1 hunks)backend/tests/apps/slack/common/handlers/users_test.py(1 hunks)cspell/custom-dict.txt(1 hunks)frontend/__tests__/e2e/helpers/mockDashboardCookies.ts(1 hunks)frontend/__tests__/e2e/pages/About.spec.ts(1 hunks)frontend/__tests__/unit/components/AnimatedCounter.test.tsx(0 hunks)frontend/__tests__/unit/components/HealthMetrics.test.tsx(3 hunks)frontend/__tests__/unit/pages/Home.test.tsx(1 hunks)frontend/package.json(3 hunks)frontend/src/app/about/page.tsx(10 hunks)frontend/src/app/page.tsx(13 hunks)frontend/src/components/AnimatedCounter.tsx(0 hunks)frontend/src/components/ProjectTypeDashboardCard.tsx(1 hunks)frontend/src/types/level.ts(1 hunks)frontend/src/utils/aboutData.ts(1 hunks)frontend/src/utils/milestoneProgress.ts(2 hunks)frontend/src/wrappers/IconWrapper.tsx(1 hunks)
💤 Files with no reviewable changes (3)
- frontend/src/components/AnimatedCounter.tsx
- frontend/tests/unit/components/AnimatedCounter.test.tsx
- backend/apps/slack/common/handlers/users.py
✅ Files skipped from review due to trivial changes (1)
- frontend/tests/e2e/pages/About.spec.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- frontend/tests/unit/components/HealthMetrics.test.tsx
- frontend/src/components/ProjectTypeDashboardCard.tsx
- frontend/package.json
- frontend/src/utils/milestoneProgress.ts
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-11-17T16:47:05.578Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:427-427
Timestamp: 2025-11-17T16:47:05.578Z
Learning: In the frontend test files for the OWASP/Nest repository, `expect(true).toBe(true)` no-op assertions may be intentionally added as workarounds when ESLint rule jest/expect-expect doesn't detect expectations inside helper functions or waitFor callbacks. These can be resolved by configuring the ESLint rule's assertFunctionNames option to recognize custom assertion helpers instead of flagging them as redundant.
Applied to files:
frontend/__tests__/unit/pages/Home.test.tsx
📚 Learning: 2025-11-17T17:30:42.139Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:169-171
Timestamp: 2025-11-17T17:30:42.139Z
Learning: In the OWASP/Nest frontend tests (PR #2671 context), wrapper functions like `expectChaptersCountEqualsThree` that simply call another helper with a fixed parameter (e.g., `expectChaptersCountEquals(3)`) are intentionally used to avoid arrow function callbacks in `waitFor` calls. This pattern prevents adding nesting depth that would trigger rule typescript:S2004. Example: `await waitFor(expectChaptersCountEqualsThree)` avoids the extra nesting from `await waitFor(() => expectChaptersCountEquals(3))`.
Applied to files:
frontend/__tests__/unit/pages/Home.test.tsx
📚 Learning: 2025-09-17T02:42:41.928Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 2288
File: frontend/src/components/ActionButton.tsx:0-0
Timestamp: 2025-09-17T02:42:41.928Z
Learning: In frontend/src/components/ActionButton.tsx, the user Rajgupta36 intentionally changed text-blue-600 to text-[#1D7BD7] to align the text color with the border color (#1D7BD7) for visual consistency, prioritizing design alignment over theme tokens.
Applied to files:
frontend/src/app/about/page.tsxfrontend/src/app/page.tsx
📚 Learning: 2025-07-13T11:29:25.245Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/page.tsx:59-61
Timestamp: 2025-07-13T11:29:25.245Z
Learning: In Next.js 13+ app router, components with the 'use client' directive run entirely on the client side and don't require window object existence checks or SSR hydration considerations. Direct access to window.location and other browser APIs is safe in client components.
Applied to files:
frontend/src/app/about/page.tsxfrontend/src/app/page.tsx
🧬 Code graph analysis (1)
backend/apps/slack/common/handlers/chapters.py (2)
frontend/src/utils/pluralize.ts (1)
pluralize(1-4)backend/apps/owasp/api/internal/nodes/common.py (1)
leaders(19-21)
🔇 Additional comments (14)
cspell/custom-dict.txt (1)
69-69: Dictionary entry correctly positioned and spelled.The addition of "hackathon" on line 69 is alphabetically correct (positioned between "gunicorn" and "heroui") and uses the standard spelling. Including legitimate project terms in the custom dictionary is appropriate.
backend/apps/slack/common/handlers/chapters.py (1)
6-6: Backend pluralization change is unrelated to this PR's icon migration objective.This change improves text formatting by using Django's built-in
pluralizefilter, and the implementation is correct. However, this backend pluralization logic has no connection to the stated PR objective of migrating Font Awesome icons to react-icons in the frontend.Consider moving unrelated improvements to a separate PR to keep changes focused and easier to review.
Also applies to: 77-77
frontend/__tests__/unit/pages/Home.test.tsx (1)
246-277: LGTM! Enhanced test coverage for stats rendering.The updated test now validates both the formatted stat values (using millify with '+' suffix) and the headers, which provides more thorough coverage than the previous header-only assertions. The dual-loop structure is clear and the 3000ms timeout is reasonable for validating both sets of elements.
backend/tests/apps/github/management/commands/github_sync_user_test.py (1)
17-19: ThetotalCountattribute is necessary for this mock.PyGithub's
PaginatedListclass has atotalCountattribute that returns the total number of items in the list, and the production code extensively uses it across multiple lines ingithub_sync_user.py(lines 115, 132, 146, 274, 275, 307, 321, 322, 363, 377, 378, 404). Other test files in the same codebase also implementtotalCounton their mocks for the same reason. The change is correct and improves mock completeness for accurate testing..github/workflows/run-code-ql.yaml (1)
34-34: The lines indicated in this review did not change in this commit. The.github/workflows/run-code-ql.yamlfile, including the CodeQL action references on lines 34 and 58, were already present with the same commit hash1b168cd39490f61582a9beae412bb7057a6b2c4ein the prior commit. No review action is needed.Likely an incorrect or invalid review comment.
backend/tests/apps/slack/common/handlers/users_test.py (1)
40-42: Test assertions are correct and intentional.The test's access of
blocks[0]for the header text andblocks[1]for user data is correct. The implementation file (backend/apps/slack/common/handlers/users.py) was created with the test in the same commit and consistently constructs blocks in this order: header block at index 0, followed by user data blocks. No issues found with these test assertions.frontend/src/app/about/page.tsx (3)
54-62: LGTM! Icon migration implemented correctly.The function correctly returns react-icons components based on milestone progress. The icon selection logic is appropriate and consistent with the migration strategy.
135-263: Icon usage is consistent with component interfaces.SecondaryCard and TopContributorsList both accept
IconTypeprops (verified in component definitions). SecondaryCard internally uses IconWrapper for rendering (line 19 of SecondaryCard.tsx), while IconWrapper is used explicitly in about/page.tsx only for inline rendering of dynamic icons at line 243. The choice of HiUserGroup at line 160 aligns with the PR description regarding FaUsers variants.
5-12: Verification confirms all dependencies are properly configured and secure.No direct vulnerabilities have been found for the millify package, and it is specified in package.json as
^6.1.0. The project uses React 19.2.3 with react-icons v5.5.0, which is compatible with React 19. The IconWrapper component exists at the specified path (src/wrappers/IconWrapper.tsx). All imported icons (FaMapSigns, FaTools, FaCircleCheck, FaClock, FaScroll, FaBullseye, FaUser, FaUsersGear, HiUserGroup) are standard react-icons exports available in v5.5.0 and will resolve correctly.frontend/src/types/level.ts (1)
1-5: LGTM! Type migration aligns with react-icons.The change from
stringtoIconTypecorrectly reflects the migration to react-icons and provides proper type safety for icon usage throughout the codebase..github/PULL_REQUEST_TEMPLATE.md (1)
18-27: LGTM! Clearer requirement communication.The template updates improve clarity by explicitly marking required items and removing ambiguity around issue linking. This will help contributors understand mandatory PR requirements.
frontend/src/app/page.tsx (2)
98-111: LGTM! Clean icon mapping logic.The
getProjectIconfunction correctly maps project types to their corresponding react-icons components with proper TypeScript typing. The default fallback toFaFileCodeensures all cases are handled.
371-371: LGTM! Clean numeric formatting with millify.The migration from
AnimatedCountertomillifyprovides concise formatting for large numbers (e.g., "1K+", "10M+") with simpler implementation.frontend/src/wrappers/IconWrapper.tsx (1)
4-11: Unusedsizeprop in the interface.The
sizeprop is declared inIconWrapperPropsbut is never passed to theIconcomponent, rendering it ineffective. React-icons components typically accept asizeprop to control dimensions.Apply this diff to forward the
sizeprop to the Icon:-const IconWrapper = ({ icon: Icon, className, ...props }: IconWrapperProps) => ( - <Icon className={className} {...props} /> +const IconWrapper = ({ icon: Icon, className, size, ...props }: IconWrapperProps) => ( + <Icon className={className} size={size} {...props} /> )Alternatively, if the
sizeprop is not needed, remove it from the interface:export interface IconWrapperProps extends ComponentProps<'svg'> { icon: IconType - size?: string | number }Likely an incorrect or invalid review comment.
kasya
force-pushed
the
migrate-to-reacticons
branch
from
4b39bb7 to
5030680
Compare
github-actions
bot
removed
docs
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/src/app/page.tsx (1)
7-21: Consider consolidating icon imports to fa6 only.The file imports icons from both
react-icons/fa(FaCalendarAlt, FaMapMarkerAlt) andreact-icons/fa6. This creates a dependency on two icon packages. Consider using fa6 equivalents:
- FaCalendarAlt (line 8, used at line 158) should use the calendar days equivalent from fa6 (not FaCalendar, which is a different icon)
- FaMapMarkerAlt (line 8) can be replaced with FaLocationDot from fa6
This would improve consistency and reduce the number of icon package dependencies.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
frontend/__tests__/unit/components/Badges.test.tsx(1 hunks)frontend/__tests__/unit/components/ModeToggle.test.tsx(3 hunks)frontend/src/app/page.tsx(12 hunks)frontend/src/components/ModeToggle.tsx(2 hunks)frontend/src/components/UserMenu.tsx(2 hunks)frontend/src/utils/data.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/components/UserMenu.tsx
- frontend/tests/unit/components/ModeToggle.test.tsx
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-07-12T17:36:57.255Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.255Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.
Applied to files:
frontend/__tests__/unit/components/Badges.test.tsx
📚 Learning: 2025-11-17T16:47:05.578Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:427-427
Timestamp: 2025-11-17T16:47:05.578Z
Learning: In the frontend test files for the OWASP/Nest repository, `expect(true).toBe(true)` no-op assertions may be intentionally added as workarounds when ESLint rule jest/expect-expect doesn't detect expectations inside helper functions or waitFor callbacks. These can be resolved by configuring the ESLint rule's assertFunctionNames option to recognize custom assertion helpers instead of flagging them as redundant.
Applied to files:
frontend/__tests__/unit/components/Badges.test.tsx
📚 Learning: 2025-09-17T02:42:41.928Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 2288
File: frontend/src/components/ActionButton.tsx:0-0
Timestamp: 2025-09-17T02:42:41.928Z
Learning: In frontend/src/components/ActionButton.tsx, the user Rajgupta36 intentionally changed text-blue-600 to text-[#1D7BD7] to align the text color with the border color (#1D7BD7) for visual consistency, prioritizing design alignment over theme tokens.
Applied to files:
frontend/src/app/page.tsxfrontend/src/components/ModeToggle.tsxfrontend/src/utils/data.ts
📚 Learning: 2025-07-13T11:29:25.245Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/page.tsx:59-61
Timestamp: 2025-07-13T11:29:25.245Z
Learning: In Next.js 13+ app router, components with the 'use client' directive run entirely on the client side and don't require window object existence checks or SSR hydration considerations. Direct access to window.location and other browser APIs is safe in client components.
Applied to files:
frontend/src/app/page.tsx
🧬 Code graph analysis (1)
frontend/src/app/page.tsx (1)
frontend/src/wrappers/IconWrapper.tsx (1)
IconWrapper(13-13)
🔇 Additional comments (6)
frontend/__tests__/unit/components/Badges.test.tsx (2)
5-24: Mock implementation looks good for testing the react-icons migration.The mock correctly simulates IconWrapper behavior by extracting the icon name from the component's
nameproperty and applying appropriate transformations. The fallback to'medal'handles edge cases well.Minor observation: The type
React.ComponentType<{ className?: string }>doesn't include thenameproperty in TypeScript's type definition, though all JavaScript functions have anameproperty at runtime. This works because of optional chaining on line 14. If TypeScript ever raises an error here, you could extend the type:icon: React.ComponentType<{ className?: string }> & { name?: string }
88-115: Good parameterized test coverage for backend icon mappings.The test suite comprehensively covers the icon mapping scenarios including snake_case inputs from the backend and direct camelCase inputs. The use of a
for...ofloop for parameterized testing is clean and maintainable.frontend/src/components/ModeToggle.tsx (2)
34-38: LGTM! Correct toggle semantics preserved.The icon usage correctly implements toggle UX by displaying the target state icon (light icon in dark mode, dark icon in light mode). The styling classes and transition effects are properly maintained.
5-5: Clean migration to react-icons Material Design icons.The import correctly switches from FontAwesome to react-icons with the proper icon names (
MdOutlineLightMode,MdOutlineDarkMode). These outline variants are standard Material Design icons available in react-icons v5.5.0.frontend/src/app/page.tsx (1)
98-111: LGTM! Function correctly migrated to react-icons.The
getProjectIconfunction has been properly updated to returnIconTypecomponents from react-icons/fa6. The implementation is clean, type-safe, and consistent with the IconWrapper usage at line 283.frontend/src/utils/data.ts (1)
1-93: LGTM! Excellent migration to react-icons.The file has been thoroughly and consistently migrated from FontAwesome to react-icons:
- All icons consistently imported from
react-icons/fa6(no mixing of packages)- Type definitions correctly updated from
IconDefinitiontoIconType- All data structures (BADGE_CLASS_MAP, ICONS, level, urlMappings) properly updated to use IconType components
- Clean and well-organized structure
This file demonstrates a model migration pattern compared to other files in the PR.
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
@sameersharmadev Great work! 👍🏼 This looks good to me!
I pushed changes to update some icons (outline instead of solid, smaller size, etc) and resolved new conflicts.
LGTM!
Thanks for handling the changes 👍 |
arkid15r
dismissed stale reviews from kasya and coderabbitai[bot]
via
c93779c
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/src/app/page.tsx (2)
8-22: Consider using Font Awesome 6 equivalents for visual consistency.The imports mix Font Awesome 5 (
FaCalendarAlt,FaMapMarkerAltfromreact-icons/fa) with Font Awesome 6 icons. Since the majority of icons are from FA6, consider migrating the remaining FA5 icons to their FA6 equivalents for consistent visual weight and styling:
FaCalendarAlt→FaCalendarDays(or continue usingFaCalendarwhich is already imported)FaMapMarkerAlt→FaLocationDotApply this diff to standardize on FA6 icons:
-import { FaCalendarAlt, FaMapMarkerAlt } from 'react-icons/fa' import { FaBook, FaCalendar, + FaCalendarDays, FaCode, FaFileCode, FaFolder, FaGlobe, + FaLocationDot, FaNewspaper, FaTag, FaUser, } from 'react-icons/fa6'Then update the icon usages accordingly (lines 158, 198, 219, 244).
158-158: Use consistent calendar icon throughout the file.Line 158 uses
FaCalendarAlt(FA5), while lines 240, 279, and 355 useFaCalendar(FA6) for similar date-display purposes. Standardizing onFaCalendar(orFaCalendarDaysfrom FA6) would improve visual consistency.Apply this diff to use the same calendar icon:
- icon={FaCalendarAlt} + icon={FaCalendar}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
frontend/src/app/page.tsx(12 hunks)frontend/src/components/ProjectTypeDashboardCard.tsx(1 hunks)frontend/src/types/level.ts(1 hunks)frontend/src/utils/milestoneProgress.ts(2 hunks)frontend/src/wrappers/IconWrapper.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/src/components/ProjectTypeDashboardCard.tsx
- frontend/src/types/level.ts
- frontend/src/wrappers/IconWrapper.tsx
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 2288
File: frontend/src/components/ActionButton.tsx:0-0
Timestamp: 2025-09-17T02:42:41.928Z
Learning: In frontend/src/components/ActionButton.tsx, the user Rajgupta36 intentionally changed text-blue-600 to text-[#1D7BD7] to align the text color with the border color (#1D7BD7) for visual consistency, prioritizing design alignment over theme tokens.
📚 Learning: 2025-09-17T02:42:41.928Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 2288
File: frontend/src/components/ActionButton.tsx:0-0
Timestamp: 2025-09-17T02:42:41.928Z
Learning: In frontend/src/components/ActionButton.tsx, the user Rajgupta36 intentionally changed text-blue-600 to text-[#1D7BD7] to align the text color with the border color (#1D7BD7) for visual consistency, prioritizing design alignment over theme tokens.
Applied to files:
frontend/src/utils/milestoneProgress.tsfrontend/src/app/page.tsx
📚 Learning: 2025-09-21T17:04:48.154Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2178
File: frontend/src/app/snapshots/[id]/page.tsx:0-0
Timestamp: 2025-09-21T17:04:48.154Z
Learning: User rudransh-shrivastava confirmed that suggested type safety improvements during Apollo Client migration were no longer relevant, reinforcing their preference to keep migration PRs focused on core migration changes rather than additional improvements.
Applied to files:
frontend/src/utils/milestoneProgress.ts
📚 Learning: 2025-07-13T11:29:25.245Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/page.tsx:59-61
Timestamp: 2025-07-13T11:29:25.245Z
Learning: In Next.js 13+ app router, components with the 'use client' directive run entirely on the client side and don't require window object existence checks or SSR hydration considerations. Direct access to window.location and other browser APIs is safe in client components.
Applied to files:
frontend/src/app/page.tsx
🧬 Code graph analysis (2)
frontend/src/utils/milestoneProgress.ts (1)
backend/apps/github/api/internal/nodes/milestone.py (1)
progress(40-45)
frontend/src/app/page.tsx (2)
frontend/src/components/TruncatedText.tsx (1)
TruncatedText(3-45)frontend/src/wrappers/IconWrapper.tsx (1)
IconWrapper(13-13)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: Run spell check
- GitHub Check: Run frontend checks
- GitHub Check: Run pre-commit checks
- GitHub Check: CodeQL (python)
🔇 Additional comments (3)
frontend/src/utils/milestoneProgress.ts (1)
1-22: LGTM! Migration is correct and return type annotation properly added.The migration from Font Awesome to react-icons is technically sound:
- Correct imports from
react-icons/fa6- Proper
IconTypereturn type annotation added (addressing previous concern)- Icon components returned correctly (not instances), following standard React patterns
The icon choice for "In Progress" (FaUserGear) was previously confirmed as an intentional design decision.
frontend/src/app/page.tsx (2)
98-111: LGTM!The
getProjectIconfunction is well-implemented with proper typing (IconType), handles all project type cases, and provides a sensible default fallback. All returned icons are consistently from FA6.
283-283: LGTM!The
IconWrapperusage correctly renders the dynamic project icon returned bygetProjectIcon. The type compatibility (IconType) and styling are properly applied.
3 participants
Proposed change
Resolves #2800
This PR migrates all FontAwesome icons to react-icons. All existing FontAwesome imports were replaced with their react-icons equivalents (
react-icons/fa,react-icons/fa6).Icon changes:
FaUsersfrom bothfaandfa6were different from the one we use for some reason, so I usedHiUserGroup(react-icons/hi) instead.faMoon and the custom sun icon were replaced with
MdDarkModeandMdLightMode.Tests for these changes still remain, just wanted to draft a PR first if any changes are needed in the implementation.
Checklist
make check-testlocally and all tests passed