refactor: nested ternary operators into independent statements #2532
Conversation
Summary by CodeRabbitRelease Notes
WalkthroughRefactors nested/inline ternaries into explicit control flow across tests, hooks, and components; adds a new milestoneProgress utility (text + icon mapping) with unit tests; and updates impacted components to use the new helpers and extracted local constants. No public API signature changes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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: 1
🧹 Nitpick comments (3)
frontend/__tests__/unit/utils/milestoneProgress.test.ts (2)
11-13: Consider testing boundary values in the 1-99 range.The test only checks the middle value (50). Consider adding tests for edge cases like 1 and 99 to ensure the boundary conditions work correctly. Also, since the backend returns decimal values (e.g., 50.5), consider testing a decimal input.
Example:
test('returns "In Progress" when progress is between 1 and 99', () => { + expect(getMilestoneProgressText(1)).toBe('In Progress') expect(getMilestoneProgressText(50)).toBe('In Progress') + expect(getMilestoneProgressText(50.5)).toBe('In Progress') + expect(getMilestoneProgressText(99)).toBe('In Progress') })
25-27: Consider testing boundary values for icon selection.Similar to the text tests, consider adding tests for edge cases like 1 and 99 to verify boundary behavior.
Example:
test('returns faUserGear when progress is between 1 and 99', () => { + expect(getMilestoneProgressIcon(1)).toBe(faUserGear) expect(getMilestoneProgressIcon(50)).toBe(faUserGear) + expect(getMilestoneProgressIcon(99)).toBe(faUserGear) })frontend/src/utils/milestoneProgress.ts (1)
13-21: Consider adding an explicit return type annotation.The function successfully refactors the nested ternary into clear if-else logic. For better type safety and documentation, consider adding an explicit return type.
Example:
+import type { IconDefinition } from '@fortawesome/fontawesome-svg-core' import { faCircleCheck, faClock, faUserGear } from '@fortawesome/free-solid-svg-icons' -export const getMilestoneProgressIcon = (progress: number) => { +export const getMilestoneProgressIcon = (progress: number): IconDefinition => { if (progress === 100) { return faCircleCheck } else if (progress > 0) { return faUserGear } else { return faClock } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
frontend/__tests__/unit/components/ModuleList.test.tsx(1 hunks)frontend/__tests__/unit/components/ProgramCard.test.tsx(1 hunks)frontend/__tests__/unit/pages/ProjectsHealthDashboardMetrics.test.tsx(2 hunks)frontend/__tests__/unit/utils/milestoneProgress.test.ts(1 hunks)frontend/src/app/about/page.tsx(2 hunks)frontend/src/app/global-error.tsx(1 hunks)frontend/src/app/projects/dashboard/metrics/page.tsx(2 hunks)frontend/src/components/CardDetailsPage.tsx(2 hunks)frontend/src/components/ProgramCard.tsx(2 hunks)frontend/src/hooks/useSearchPage.ts(1 hunks)frontend/src/utils/milestoneProgress.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 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/pages/ProjectsHealthDashboardMetrics.test.tsxfrontend/__tests__/unit/components/ProgramCard.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/ProgramCard.test.tsxfrontend/src/app/projects/dashboard/metrics/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/app/projects/dashboard/metrics/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 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/CardDetailsPage.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/CardDetailsPage.tsx
🧬 Code graph analysis (5)
frontend/src/components/ProgramCard.tsx (1)
frontend/src/utils/dateFormatter.ts (1)
formatDate(1-20)
frontend/src/hooks/useSearchPage.ts (1)
frontend/src/server/fetchAlgoliaData.ts (1)
fetchAlgoliaData(6-59)
frontend/__tests__/unit/utils/milestoneProgress.test.ts (1)
frontend/src/utils/milestoneProgress.ts (2)
getMilestoneProgressText(3-11)getMilestoneProgressIcon(13-21)
frontend/src/utils/milestoneProgress.ts (1)
backend/apps/github/api/internal/nodes/milestone.py (1)
progress(40-45)
frontend/src/app/about/page.tsx (1)
frontend/src/utils/milestoneProgress.ts (2)
getMilestoneProgressText(3-11)getMilestoneProgressIcon(13-21)
🔇 Additional comments (10)
frontend/__tests__/unit/components/ProgramCard.test.tsx (1)
9-21: LGTM! Clean refactor of test mock.The refactor from inline ternary to explicit if-else branches with an intermediate
iconNamevariable improves readability and maintainability while preserving the same test behavior.frontend/src/components/ProgramCard.tsx (1)
34-42: LGTM! Excellent refactor of date display logic.The
dateInfoIIFE elegantly extracts the date rendering logic into a descriptive variable with clear conditional branches. This significantly improves readability compared to inline conditionals.frontend/src/app/projects/dashboard/metrics/page.tsx (1)
80-108: LGTM! Well-structured refactor with clear separation of concerns.The three IIFEs (
alignmentClass,textAlignClass,fontAwesomeIconType) effectively replace nested ternaries with explicit conditional logic. Each IIFE handles a single responsibility, making the code more maintainable and testable.frontend/__tests__/unit/pages/ProjectsHealthDashboardMetrics.test.tsx (1)
151-161: LGTM! Good test coverage for refactored alignment logic.This test validates that the refactored
alignmentClassandtextAlignClassIIFEs produce the correct CSS classes, helping prevent regressions.frontend/src/app/global-error.tsx (1)
66-74: LGTM! Clear refactor of error handling logic.The explicit if-else structure with the intermediate
appErrorvariable improves readability while preserving the original error handling behavior.frontend/__tests__/unit/components/ModuleList.test.tsx (1)
8-18: LGTM! Consistent test mock refactor.The refactor follows the same pattern as other test files in this PR, using explicit if-else branches with an intermediate variable to improve readability.
frontend/src/hooks/useSearchPage.ts (1)
85-95: LGTM! Clean extraction of index name computation.The
computedIndexNamevariable with the intermediatehasValidSortcheck effectively replaces inline logic, making the Algolia index name construction explicit and easier to understand.frontend/src/utils/milestoneProgress.ts (1)
3-11: Excellent refactoring from nested ternary to clear if-else chain.The function successfully eliminates nested ternary operators and uses a clear, readable if-else structure. The logic correctly handles all progress states including decimal values from the backend.
frontend/src/app/about/page.tsx (2)
36-36: Clean import addition for centralized milestone utilities.The import correctly brings in the new utility functions, successfully centralizing milestone progress logic and eliminating the need for inline nested ternaries.
221-228: Successfully refactored nested ternaries with utility function calls.The inline nested ternary operators have been cleanly replaced with calls to the centralized utility functions. This significantly improves readability and maintainability while achieving the PR's objective of eliminating nested ternaries per SonarQube rule S3358.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/src/components/CardDetailsPage.tsx (1)
78-78: Inconsistent equality operator.Line 78 uses loose equality (
==) for the module check while using strict equality (===) for the program check. For consistency and to follow TypeScript best practices, use strict equality throughout.Apply this diff:
- if (type === 'program' || type == 'module') { + if (type === 'program' || type === 'module') {
🧹 Nitpick comments (1)
frontend/src/components/CardDetailsPage.tsx (1)
77-85: Consider a more idiomatic pattern for the conditional logic.While the IIFE pattern works correctly, a simpler approach using
letwith if-else statements is more conventional in modern React/TypeScript and avoids the function invocation overhead.Apply this diff for a more straightforward implementation:
- const secondaryCardStyles = (() => { - if (type === 'program' || type === 'module') { - return 'gap-2 md:col-span-7' - } else if (type !== 'chapter') { - return 'gap-2 md:col-span-5' - } else { - return 'gap-2 md:col-span-3' - } - })() + let secondaryCardStyles: string + if (type === 'program' || type === 'module') { + secondaryCardStyles = 'gap-2 md:col-span-7' + } else if (type !== 'chapter') { + secondaryCardStyles = 'gap-2 md:col-span-5' + } else { + secondaryCardStyles = 'gap-2 md:col-span-3' + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/components/CardDetailsPage.tsx(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 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/CardDetailsPage.tsx
📚 Learning: 2025-07-12T17:14:28.536Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/[programKey]/edit/page.tsx:90-128
Timestamp: 2025-07-12T17:14:28.536Z
Learning: Both ProgramForm (programCard.tsx) and ModuleForm (mainmoduleCard.tsx) components already implement HTML validation using the `required` attribute on form fields. The browser's native validation prevents form submission and displays error messages when required fields are empty, eliminating the need for additional JavaScript validation before GraphQL mutations.
Applied to files:
frontend/src/components/CardDetailsPage.tsx
📚 Learning: 2025-09-29T06:02:35.566Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2178
File: frontend/src/components/SingleModuleCard.tsx:54-54
Timestamp: 2025-09-29T06:02:35.566Z
Learning: In the Module type from types/mentorship.ts, the experienceLevel field is required (experienceLevel: ExperienceLevelEnum), not optional, so null/undefined checks are not needed when accessing this field.
Applied to files:
frontend/src/components/CardDetailsPage.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/CardDetailsPage.tsx
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/__tests__/unit/pages/ProjectsHealthDashboardMetrics.test.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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/__tests__/unit/pages/ProjectsHealthDashboardMetrics.test.tsx
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/__tests__/unit/pages/ProjectsHealthDashboardMetrics.test.tsx (1)
151-162: Test correctly verifies alignment classes after addressing null check.The null check on line 157 addresses the primary concern from the previous review. The test now properly guards against unclear failures when
closest('div')doesn't find a match.One minor TypeScript note: Jest's
expect().not.toBeNull()doesn't narrow the type, sowrapperDivis technically stillElement | nullon lines 160-161. If you want stricter type safety, you could add a non-null assertion (wrapperDiv!) or wrap the subsequent assertions in anif (wrapperDiv)block. However, this is optional—the test will fail clearly at line 157 ifwrapperDivis null, and your assertion likely handles the type just fine.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/__tests__/unit/pages/ProjectsHealthDashboardMetrics.test.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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/__tests__/unit/pages/ProjectsHealthDashboardMetrics.test.tsx
|
@coderabbitai generate docstrings |
✅ Actions performedInitiated docstring generation; will generate only if new commits exist. |
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @ishanBahuguna. * #2532 (comment) The following files were modified: * `frontend/src/hooks/useSearchPage.ts`
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/src/components/CardDetailsPage.tsx (1)
77-86: Great refactoring—nested ternary successfully eliminated!The if-else structure is clear, readable, and uses strict equality consistently throughout. The descriptive variable name makes the intent obvious.
Optional: Consider simplifying the IIFE pattern.
While the IIFE works correctly, a simpler pattern might be more idiomatic for React components:
// Option 1: Helper function const getSecondaryCardStyles = () => { if (type === 'program' || type === 'module') { return 'gap-2 md:col-span-7' } else if (type !== 'chapter') { return 'gap-2 md:col-span-5' } else { return 'gap-2 md:col-span-3' } } const secondaryCardStyles = getSecondaryCardStyles()// Option 2: Let assignment (if reassignment isn't needed, const is fine) let secondaryCardStyles = 'gap-2 md:col-span-3' if (type === 'program' || type === 'module') { secondaryCardStyles = 'gap-2 md:col-span-7' } else if (type !== 'chapter') { secondaryCardStyles = 'gap-2 md:col-span-5' }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/src/components/CardDetailsPage.tsx(2 hunks)frontend/src/components/ProgramCard.tsx(2 hunks)frontend/src/hooks/useSearchPage.ts(1 hunks)frontend/src/utils/milestoneProgress.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/src/hooks/useSearchPage.ts
- frontend/src/components/ProgramCard.tsx
- frontend/src/utils/milestoneProgress.ts
🧰 Additional context used
🧠 Learnings (3)
📚 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/CardDetailsPage.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/CardDetailsPage.tsx
📚 Learning: 2025-07-12T17:14:28.536Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/[programKey]/edit/page.tsx:90-128
Timestamp: 2025-07-12T17:14:28.536Z
Learning: Both ProgramForm (programCard.tsx) and ModuleForm (mainmoduleCard.tsx) components already implement HTML validation using the `required` attribute on form fields. The browser's native validation prevents form submission and displays error messages when required fields are empty, eliminating the need for additional JavaScript validation before GraphQL mutations.
Applied to files:
frontend/src/components/CardDetailsPage.tsx
🔇 Additional comments (1)
frontend/src/components/CardDetailsPage.tsx (1)
137-137: LGTM!Clean and straightforward usage of the extracted constant.
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
@ishanBahuguna Thanks for working on this!
I posted one suggestion below ⬇️
Could you also run make check-test locally and address found issues? Thanks!
| const secondaryCardStyles = (() => { | ||
| if (type === 'program' || type === 'module') { | ||
| return 'gap-2 md:col-span-7' | ||
| } else if (type !== 'chapter') { | ||
| return 'gap-2 md:col-span-5' | ||
| } else { | ||
| return 'gap-2 md:col-span-3' | ||
| } | ||
| })() |
There was a problem hiding this comment.
Let's not introduce negated conditions. This is also one of the issues we happen to have quite a few of in SonarQube and something that makes the code less readable.
| const secondaryCardStyles = (() => { | |
| if (type === 'program' || type === 'module') { | |
| return 'gap-2 md:col-span-7' | |
| } else if (type !== 'chapter') { | |
| return 'gap-2 md:col-span-5' | |
| } else { | |
| return 'gap-2 md:col-span-3' | |
| } | |
| })() | |
| const secondaryCardStyles = (() => { | |
| if (type === 'program' || type === 'module') { | |
| return 'gap-2 md:col-span-7' | |
| } | |
| if (type === 'chapter') { | |
| return 'gap-2 md:col-span-3' | |
| } | |
| return 'gap-2 md:col-span-5' | |
| })() |
There was a problem hiding this comment.
@kasya I'have updated the code as you have suggested.
The issue you are talking about is the one which is failing CI/CD ?
or I have to fix something else
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/__tests__/unit/pages/ProjectsHealthDashboardMetrics.test.tsx (1)
151-158: Consider testing multiple alignment cases.The test correctly verifies that the refactored alignment logic produces the expected CSS classes for the center-aligned "Stars" column. However, if the
MetricsPageincludes columns with different alignment values (e.g.,align="left"oralign="right"), testing those cases would provide more comprehensive coverage of the refactored ternary logic.You could extend this test or add a parameterized test covering different columns:
test('SortableColumnHeader applies correct alignment classes', async () => { render(<MetricsPage />) + + // Test center alignment const sortButton = await screen.findByTitle('Sort by Stars') const wrapperDiv = sortButton.closest('div') expect(wrapperDiv).not.toBeNull() expect(wrapperDiv).toHaveClass('justify-center') expect(sortButton).toHaveClass('text-center') + + // Test left alignment (if applicable) + const leftAlignButton = await screen.findByTitle('Sort by Forks') + const leftWrapper = leftAlignButton.closest('div') + expect(leftWrapper).not.toBeNull() + expect(leftWrapper).toHaveClass('justify-start') + expect(leftAlignButton).toHaveClass('text-left') })Note: Only add additional assertions if other columns actually use different alignment values.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/__tests__/unit/pages/ProjectsHealthDashboardMetrics.test.tsx(1 hunks)frontend/src/components/CardDetailsPage.tsx(2 hunks)frontend/src/hooks/useSearchPage.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/hooks/useSearchPage.ts
- frontend/src/components/CardDetailsPage.tsx
2 participants
Proposed change
Resolves #2520
Tasks
Checklist
make check-testlocally; all checks and tests passed.