Refactor: Replace negated conditions with positive checks#3274
Refactor: Replace negated conditions with positive checks#3274arkid15r merged 4 commits intoOWASP:mainfrom
Conversation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughRewrote several inverted conditionals in four frontend components to use positive checks: pluralization labels in candidates page and ModuleCard, a null-check-to-string expression in CardDetailsPage, and the loaded/unloaded rendering branch in SearchPageLayout. No exported signatures were changed. (49 words) Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (2)📚 Learning: 2025-06-20T16:12:59.256ZApplied to files:
📚 Learning: 2025-06-20T16:12:59.256ZApplied to files:
⏰ 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). (3)
🔇 Additional comments (2)
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: 0
🧹 Nitpick comments (2)
frontend/src/components/SearchPageLayout.tsx (1)
52-70: Conditional logic correctly inverted, but fix formatting.The change successfully replaces the negated condition with a positive check. However, there's a minor formatting inconsistency on line 68.
✨ Formatting improvement
- ):( + ) : ( <SkeletonBase indexName={indexName} loadingImageUrl={loadingImageUrl} />Add a space before the colon and after the closing parenthesis for consistency with standard formatting conventions.
frontend/src/components/CardDetailsPage.tsx (1)
162-162: Conditional logic correctly inverted, but fix spacing.The change successfully inverts the condition to check for null/undefined first. However, there's a minor spacing issue in the ternary operator.
✨ Formatting improvement
- <LeadersList leaders={detail?.value == null ? 'Unknown': String(detail.value)} /> + <LeadersList leaders={detail?.value == null ? 'Unknown' : String(detail.value)} />Add a space before the colon in the ternary operator for consistency with standard formatting conventions.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/src/app/board/[year]/candidates/page.tsxfrontend/src/components/CardDetailsPage.tsxfrontend/src/components/ModuleCard.tsxfrontend/src/components/SearchPageLayout.tsx
🧰 Additional context used
🧠 Learnings (2)
📚 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
🔇 Additional comments (2)
frontend/src/components/ModuleCard.tsx (1)
110-110: LGTM! Pluralization logic improved with positive condition.The change correctly inverts the condition to check for the singular case first (
weeks === 1), making the logic more readable and aligning with the PR objective of replacing negated conditions.frontend/src/app/board/[year]/candidates/page.tsx (1)
480-480: LGTM! Pluralization logic improved with positive conditions.Both changes correctly invert the conditions to check for the singular case first (
=== 1), making the pluralization logic more readable and consistent with the PR objective.Also applies to: 486-486
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/src/components/CardDetailsPage.tsx (1)
162-162: Logic is correct; minor formatting and optional modern syntax suggestion.The refactoring successfully replaces the negated condition with a positive check, meeting the PR objectives. The logic is functionally equivalent and uses the "error-first" pattern which some find more readable.
Minor formatting issue: Missing space after
'Unknown'before the colon in the ternary operator. Standard formatting would be:detail?.value == null ? 'Unknown' : String(detail.value)This will likely be caught by your formatter (Prettier, ESLint, etc.) when running
make check-test.Optional modern alternative: Consider using the nullish coalescing operator for more idiomatic TypeScript:
String(detail?.value ?? 'Unknown')This is more concise and eliminates the ternary entirely.
♻️ Optional: Modern nullish coalescing syntax
- <LeadersList leaders={detail?.value == null ? 'Unknown': String(detail.value)} /> + <LeadersList leaders={String(detail?.value ?? 'Unknown')} />
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/components/CardDetailsPage.tsx
🧰 Additional context used
🧠 Learnings (2)
📚 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
arkid15r
left a comment
arkid15r
left a comment
There was a problem hiding this comment.
You didn't run the required code quality checks. Your future issue assignment requests will be de-prioritized.
|
* Refactor: Replace negated conditions with positive checks * Update code --------- Co-authored-by: Arkadii Yakovets <arkadii.yakovets@owasp.org> Co-authored-by: Arkadii Yakovets <2201626+arkid15r@users.noreply.github.com>
2 participants
Proposed change
Resolves #3231
Changed the negated condition checks in the files to positive conditions.
It includes:
frontend/src/components/ModuleCard.tsx
frontend/src/components/SearchPageLayout.tsx
frontend/src/app/board/[year]/candidates/page.tsx
frontend/src/components/CardDetailsPage.tsx
Checklist
make check-testlocally and all tests passed