refactor: nested ternary operators into independent statements #2532
Conversation
Summary by CodeRabbit
WalkthroughThe PR refactors nested ternary operators and inline conditional logic into explicit intermediate variables, if/else statements, and IIFEs across multiple test and production files. A new milestoneProgress utility module is introduced with helper functions for determining milestone progress text and icons. The about page is updated to use this centralized utility instead of inline icon selection logic. 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.
Thanks! I don't think this was falling CI/CD check. There was a different issue with parseNumber that we now enforce with CI/CD - maybe that was failing. It's all good now 👌🏼
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
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 (3)
frontend/src/app/about/page.tsx(2 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/app/about/page.tsx
- frontend/src/components/CardDetailsPage.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/hooks/useSearchPage.ts (1)
frontend/src/server/fetchAlgoliaData.ts (1)
fetchAlgoliaData(6-59)
⏰ 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). (4)
- GitHub Check: Run frontend unit tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (1)
frontend/src/hooks/useSearchPage.ts (1)
99-99: LGTM - correct usage of computed index name.The change correctly uses
computedIndexNameinstead of the directindexName, enabling dynamic index selection based on sorting state as intended by the refactoring.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
frontend/src/hooks/useSearchPage.ts (3)
71-73: Remove redundant empty string check.The condition
sortBy !== ''is redundant becausesortByis already checked for truthiness. An empty string is falsy, so ifsortByis truthy, it cannot be an empty string.Apply this diff:
- if (sortBy && sortBy !== 'default' && sortBy !== '') { + if (sortBy && sortBy !== 'default') { params.set('sortBy', sortBy) }
75-77: Remove redundant empty string check.The condition
order !== ''is redundant becauseorderis already checked for truthiness. An empty string is falsy, so iforderis truthy, it cannot be an empty string.Apply this diff:
- if (sortBy !== 'default' && order && order !== '') { + if (sortBy !== 'default' && order) { params.set('order', order) }
87-96: Good refactoring! Minor comment formatting and redundant check to address.The refactoring from nested ternary to explicit statements successfully achieves the PR objective and improves readability. The critical bug (
sortBy[0] !== 'default') identified in the previous review has been correctly addressed and removed.Minor improvements needed:
- Line 89: Add period at end of comment:
"Check if valid sort option is selected."- Line 93: Capitalize and fix grammar:
"If sorting is active then append the sort field and order to the base index name."- Line 94: Remove redundant
order !== ''check (similar to the issues at lines 71 and 75)Apply this diff:
- // Check if valid sort option is selected + // Check if valid sort option is selected. const hasValidSort = sortBy && sortBy !== 'default' if (hasValidSort) { - // if sorting is active then appends the sort field and order to the base index name. - const orderSuffix = order && order !== '' ? `_${order}` : '' + // If sorting is active then append the sort field and order to the base index name. + const orderSuffix = order ? `_${order}` : '' computedIndexName = `${indexName}_${sortBy}${orderSuffix}` }backend/apps/owasp/models/snapshot.py (1)
42-65: Count properties align with API needs; be mindful of potential query overheadThe new
new_*_countproperties are clear and line up with the snapshot detail schema. Each call issues a.count()on the related manager, which is fine for single‑snapshot detail usage, but if you ever consume these in bulk (e.g., iterating many snapshots), consider aggregations or prefetching to avoid N extra queries.backend/tests/apps/api/rest/v0/snapshot_test.py (1)
72-262: API tests cover main branches; consider adding list/ordering and stronger resolver assertionsThe RequestFactory + MagicMock strategy exercises both 404 and success paths for each endpoint, which is solid. Two optional improvements you might consider:
- Add tests for
list_snapshots(and, if not covered elsewhere, explicitorderingvalues) to guard the main listing endpoint and ordering behavior.- The
test_list_snapshot_issues_no_repository/test_list_snapshot_releases_no_repositorynames suggest resolver behavior, but they currently only assert that the returned list contains the mock object. If you want these to cover the “no repository” semantics, you could either:
- Assert via
SnapshotIssue/SnapshotReleaseschemas (e.g., checkingorganization_login/repository_name), or- Rely solely on the dedicated resolver tests at the bottom and rename/remove the “no_repository” tests to avoid redundancy.
backend/apps/api/rest/v0/snapshot.py (3)
29-94: Snapshot schemas and resolvers look consistent with the underlying models
SnapshotBase/SnapshotDetailmirrorSnapshotModel’s fields andnew_*_countproperties, andSnapshotIssue/SnapshotReleasecorrectly deriveorganization_loginandrepository_namefrom the related repository/organization. This aligns with the dedicated resolver tests insnapshot_test.pyand keeps the API shape predictable.You could factor the repository/organization-to-login/name logic into a small helper (or mixin) shared between
SnapshotIssueandSnapshotReleaseto avoid duplication, but that’s purely a style choice here.
126-147: Clarifyget_snapshotreturn style vs annotatedSnapshotDetail | SnapshotErrorThe success branch currently returns a
SnapshotModelinstance while the type annotation and response mapping declareSnapshotDetail. Django Ninja will typically coerce ORM objects to the declared schema, but static type-checkers and readers may find this slightly misleading.If you want the code to mirror the type hint more closely, consider either:
- Explicitly constructing the schema (e.g., mapping fields or using
from_orm/from_attributesif configured), or- Adjusting the annotation/comment to make it clear that Ninja performs the model→schema conversion at the framework layer.
Functionally this is fine; this is about readability and type clarity.
150-276: Snapshot lookup pattern is clear but repeated across endpointsEach list endpoint repeats:
snapshot = SnapshotModel.objects.filter( key__iexact=snapshot_id, status=SnapshotModel.Status.COMPLETED ).first()followed by either returning related objects or an
.objects.none()queryset. This is easy to follow, but you might consider a small helper likeget_completed_snapshot(snapshot_id: str) -> SnapshotModel | Noneto centralize the lookup and make future changes (e.g., additional filters) less error-prone.Not urgent, but a modest readability/maintainability win.
📜 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 (11)
backend/apps/api/rest/v0/__init__.py(2 hunks)backend/apps/api/rest/v0/snapshot.py(1 hunks)backend/apps/owasp/models/snapshot.py(1 hunks)backend/tests/apps/api/rest/v0/snapshot_test.py(1 hunks)backend/tests/apps/api/rest/v0/urls_test.py(2 hunks)frontend/.pnpmrc(0 hunks)frontend/__tests__/unit/pages/About.test.tsx(1 hunks)frontend/next.config.ts(1 hunks)frontend/package.json(0 hunks)frontend/src/app/about/page.tsx(3 hunks)frontend/src/hooks/useSearchPage.ts(2 hunks)
💤 Files with no reviewable changes (2)
- frontend/package.json
- frontend/.pnpmrc
🧰 Additional context used
🧠 Learnings (1)
📚 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.tsx
🧬 Code graph analysis (7)
backend/apps/owasp/models/snapshot.py (1)
backend/apps/owasp/api/internal/nodes/snapshot.py (5)
new_chapters(35-37)new_issues(40-42)new_projects(45-47)new_releases(50-52)new_users(55-57)
backend/tests/apps/api/rest/v0/urls_test.py (1)
backend/apps/owasp/api/internal/queries/snapshot.py (1)
snapshot(14-22)
backend/apps/api/rest/v0/snapshot.py (3)
backend/apps/api/decorators/cache.py (1)
cache_response(19-56)backend/apps/owasp/models/snapshot.py (7)
Snapshot(7-72)new_chapters_count(43-45)new_issues_count(48-50)new_projects_count(53-55)new_releases_count(58-60)new_users_count(63-65)Status(14-18)backend/apps/owasp/api/internal/nodes/snapshot.py (5)
new_chapters(35-37)new_issues(40-42)new_users(55-57)new_projects(45-47)new_releases(50-52)
backend/tests/apps/api/rest/v0/snapshot_test.py (2)
backend/apps/api/rest/v0/snapshot.py (14)
SnapshotDetail(44-51)get_snapshot(137-147)list_snapshot_chapters(158-172)list_snapshot_issues(183-199)list_snapshot_members(210-224)list_snapshot_projects(235-249)list_snapshot_releases(260-276)Snapshot(40-41)SnapshotIssue(54-72)resolve_organization_login(61-67)resolve_organization_login(82-88)resolve_repository_name(70-72)resolve_repository_name(91-93)SnapshotRelease(75-93)backend/apps/owasp/models/snapshot.py (7)
Snapshot(7-72)new_chapters_count(43-45)new_issues_count(48-50)new_projects_count(53-55)new_releases_count(58-60)new_users_count(63-65)Status(14-18)
frontend/src/hooks/useSearchPage.ts (1)
frontend/src/server/fetchAlgoliaData.ts (1)
fetchAlgoliaData(6-59)
backend/apps/api/rest/v0/__init__.py (1)
backend/apps/owasp/api/internal/queries/snapshot.py (1)
snapshot(14-22)
frontend/src/app/about/page.tsx (1)
frontend/src/utils/milestoneProgress.ts (2)
getMilestoneProgressText(4-12)getMilestoneProgressIcon(14-22)
🔇 Additional comments (7)
frontend/next.config.ts (1)
34-34: Verify scope of this change and confirm intentionality.Verification shows that
'require-in-the-middle'is only a transitive dependency in the lockfile and is not directly imported or used anywhere in the codebase, so removing it fromserverExternalPackagesis functionally safe. However, this change remains inconsistent with the stated PR objectives of refactoring nested ternary operators.Please confirm whether this dependency cleanup is an intentional scope expansion for this PR, or if it should be deferred to a separate PR focused on dependency management.
frontend/src/app/about/page.tsx (2)
36-36: Excellent refactoring to extract milestone logic into utility functions!The extraction of milestone progress text and icon logic into dedicated utility functions (
getMilestoneProgressTextandgetMilestoneProgressIcon) successfully eliminates nested ternaries from the component, improving readability and maintainability. This aligns perfectly with the PR objectives.Also applies to: 225-225, 232-232
150-150: LGTM! Label change is properly tested.The label change to "Wall of Fame" for the top contributors section is appropriately reflected in the test expectations.
frontend/__tests__/unit/pages/About.test.tsx (1)
221-221: LGTM! Test updated to match production code.The test expectation correctly reflects the label change to "Wall of Fame" in the About page component.
backend/tests/apps/api/rest/v0/urls_test.py (1)
15-15: Snapshot router test wiring looks correctImporting
snapshot_routerand adding"/snapshots": snapshot_routertoEXPECTED_ROUTERSmatches the v0 router configuration and ensures the new snapshot router registration is covered by tests.Also applies to: 34-35
backend/apps/api/rest/v0/__init__.py (1)
19-19: Consistent registration of/snapshotsrouterIncluding
snapshot_routerinROUTERSunder the"/snapshots"prefix is consistent with other resources and aligns with the updatedurls_test.py.Also applies to: 33-35
backend/tests/apps/api/rest/v0/snapshot_test.py (1)
24-69: SnapshotDetail schema test is well-aligned with the schema/model fieldsThe parametrized coverage for timestamps and all
new_*_countfields matchesSnapshotDetail/SnapshotModeland should catch schema drift early. Looks good as-is.
ishanBahuguna
force-pushed
the
feature/refactor-nested-ternary-operators
branch
from
1c4e3f3 to
3f693ab
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/__tests__/unit/utils/milestoneProgress.test.ts (2)
11-13: Test boundary values 1 and 99.The test uses 50 as a representative value for the "In Progress" range (1–99), but explicitly testing the boundaries 1 and 99 would strengthen confidence that the
progress > 0andprogress < 100conditions work correctly.Add tests for boundary values:
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(99)).toBe('In Progress') })Apply the same pattern to
getMilestoneProgressIcontests (lines 25–27).
5-32: Consider testing invalid inputs to document expected behavior.The utility functions lack input validation, so invalid values (negative numbers, > 100,
NaN,null,undefined) will fall through to theelsebranch, returning "Not Started" /faClock. Adding tests for these cases would document this behavior and catch future regressions if validation is added.Example tests for edge cases:
describe('getMilestoneProgressText edge cases', () => { test('returns "Not Started" for negative progress', () => { expect(getMilestoneProgressText(-10)).toBe('Not Started') }) test('returns "Completed" for progress > 100', () => { // or document if this should be clamped or throw expect(getMilestoneProgressText(150)).toBe('Completed') }) })If invalid inputs should be rejected, consider adding validation to the utility functions and updating tests accordingly.
📜 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(1 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(2 hunks)frontend/src/utils/milestoneProgress.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- frontend/src/components/CardDetailsPage.tsx
- frontend/src/components/ProgramCard.tsx
- frontend/src/app/projects/dashboard/metrics/page.tsx
- frontend/src/utils/milestoneProgress.ts
- frontend/tests/unit/components/ModuleList.test.tsx
- frontend/src/app/global-error.tsx
- frontend/src/hooks/useSearchPage.ts
- frontend/tests/unit/components/ProgramCard.test.tsx
- frontend/src/app/about/page.tsx
🧰 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
🧬 Code graph analysis (1)
frontend/__tests__/unit/utils/milestoneProgress.test.ts (1)
frontend/src/utils/milestoneProgress.ts (2)
getMilestoneProgressText(4-12)getMilestoneProgressIcon(14-22)
🔇 Additional comments (1)
frontend/__tests__/unit/pages/ProjectsHealthDashboardMetrics.test.tsx (1)
151-158: Test correctly verifies alignment refactoring.The test appropriately validates that the alignment classes are applied correctly after refactoring nested ternaries. The null check addresses the previous review feedback, making failure messages clear if the DOM structure changes.
|
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
This is great! One tiny request!
| const alignmentClass = (() => { | ||
| if (align === 'center') { | ||
| return 'justify-center' | ||
| } else if (align === 'right') { | ||
| return 'justify-end' | ||
| } else { | ||
| return 'justify-start' | ||
| } | ||
| })() | ||
|
|
||
| const textAlignClass = (() => { | ||
| if (align === 'center') { | ||
| return 'text-center' | ||
| } else if (align === 'right') { | ||
| return 'text-right' | ||
| } else { | ||
| return 'text-left' | ||
| } | ||
| })() |
There was a problem hiding this comment.
@ishanBahuguna I think this would be more clean and readable if we switch to using maps here. What do you think?
There was a problem hiding this comment.
Map would be great , more readable code , avoid repeated if/else logic and easier to extend
const justifyMap = {
left: 'justify-start',
center: 'justify-center',
right: 'justify-end',
}
const alignmentClass = justifyMap[align] || justifyMap.leftThere was a problem hiding this comment.
@kasya should I go ahead and make the changes?
2 participants
Proposed change
Resolves #2520
Tasks
Checklist
make check-testlocally; all checks and tests passed.