Refactored nested ternary expressions #3096
Conversation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughRefactors nested ternary logic into explicit conditional code on two frontend pages: moves deadline/PR/assign-button logic into helpers and derived status variables on the module issue details page; centralizes API-keys UI state into a ContentType-driven dispatch on the settings page. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-09-21T17:04:48.154ZApplied to files:
🧬 Code graph analysis (1)frontend/src/app/settings/api-keys/page.tsx (2)
🔇 Additional comments (5)
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 (2)
frontend/src/app/settings/api-keys/page.tsx (1)
73-88: Type and helper defined inside component body.The
ContentTypetype andgetContentTypefunction are defined inside the component. While this works, consider moving the type definition outside the component for clarity and potential reuse. The function itself is fine here since it depends on component state.🔎 Suggested improvement
+type ContentType = 'error' | 'loading' | 'empty' | 'table' + export default function Page() { // ... state declarations ... - type ContentType = 'error' | 'loading' | 'empty' | 'table' - const getContentType = (): ContentType => {frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx (1)
141-151:!issueIdcheck is unreachable.At line 450 where
getAssignButtonTitleis called, the component has already passed the early returns (lines 99-104). Givenskip: !issueIdon the query (line 66), ifissueIdis missing, the query result would lead to the!issueearly return. The!issueIdbranch in this helper will never execute.Consider simplifying:
🔎 Simplified version
const getAssignButtonTitle = (assigning: boolean) => { - let title: string - if (!issueId) { - title = 'Loading issue…' - } else if (assigning) { - title = 'Assigning…' - } else { - title = 'Assign to this user' - } - return title + return assigning ? 'Assigning…' : 'Assign to this user' }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsxfrontend/src/app/settings/api-keys/page.tsx
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: adithya-naik
Repo: OWASP/Nest PR: 1894
File: frontend/src/components/TopContributorsList.tsx:74-74
Timestamp: 2025-07-28T14:51:14.736Z
Learning: In the OWASP/Nest project, the maintainer adithya-naik prefers not to create separate components for code that's only used in two specific cases, following the YAGNI principle to avoid over-engineering when the duplication is limited and manageable.
🧬 Code graph analysis (2)
frontend/src/app/settings/api-keys/page.tsx (3)
frontend/src/components/skeletons/ApiKeySkelton.tsx (1)
ApiKeysSkeleton(1-96)frontend/src/types/apiKey.ts (1)
ApiKey(9-15)frontend/src/types/button.ts (1)
Button(4-9)
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx (2)
backend/tests/apps/github/models/issue_test.py (1)
issue(26-28)backend/apps/github/models/common.py (1)
title(40-44)
🔇 Additional comments (6)
frontend/src/app/settings/api-keys/page.tsx (1)
241-281: Switch-based rendering achieves the PR objective.The switch statement effectively replaces the nested ternary expressions with explicit conditional logic, improving readability and maintainability as intended by issue #2938.
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/issues/[issueId]/page.tsx (5)
39-61: Deadline formatting refactored correctly.The if-else chains for
statusTextandcolorare clearer than nested ternaries. Logic is correct.
112-123: Issue status derivation is clear and correct.The explicit if-else structure makes the status logic easy to follow and maintain.
125-139:getPRStatushelper is well-structured.Clear conditional logic for PR status. The colors (
#8657E5,#DA3633,#238636) match those used inissueStatusClassabove, maintaining visual consistency.
386-396: PR status badge rendering works correctly.The IIFE pattern to destructure
getPRStatusresult is consistent with similar usage in the component (lines 256-259).
449-451: Assign button title helper usage is clean.The refactor successfully eliminates inline conditional logic for the button title.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/src/app/settings/api-keys/page.tsx (2)
74-86: Strengthen the empty-state check to prevent potential runtime errors.The final
elsebranch returns'table'without verifying thatdata?.apiKeysactually exists. Ifdatais present butdata.apiKeysis undefined or null (e.g., due to a backend schema issue), the code would return'table'and then attempt to map over undefined at line 252, causing a runtime error.🔎 Suggested improvement to handle edge cases defensively
const getContentType = (): ContentType => { if (error) { return 'error' - } else if (loading) { + } + if (loading) { return 'loading' - } else if (data?.apiKeys && data.apiKeys.length === 0) { + } + if (!data?.apiKeys || data.apiKeys.length === 0) { return 'empty' - } else { - return 'table' } + return 'table' }This revision:
- Treats missing or undefined
apiKeysas empty rather than attempting to render a table- Removes unnecessary
elsekeywords since each branch returns
222-282: Consider extracting the IIFE into a separate render function for improved readability.While the IIFE pattern works, it's not idiomatic in React and can reduce code readability. Extracting the switch logic into a dedicated render function would align better with React conventions and make the code more maintainable.
🔎 Suggested refactor to extract content rendering
Define a helper function before the return statement:
const renderContent = () => { switch (contentType) { case 'error': return ( <div className="rounded-md bg-red-50 p-4 text-red-700 dark:bg-red-900/20 dark:text-red-400"> Error loading API keys </div> ) case 'loading': return <LoadingSpinner /> case 'empty': return ( <div className="rounded-md bg-gray-50 p-8 text-center text-gray-500 dark:bg-gray-800/50 dark:text-gray-400"> You don't have any API keys yet. </div> ) case 'table': return ( <div className="overflow-x-auto"> <table className="w-full border-collapse"> {/* table content */} </table> </div> ) default: return null } }Then in the JSX, simply call:
- {(() => { - switch (contentType) { - case 'error': - return (...) - // ... other cases - } - })()} + {renderContent()}This approach is more conventional and easier to follow for React developers.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/app/settings/api-keys/page.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/app/settings/api-keys/page.tsx (2)
frontend/src/types/apiKey.ts (1)
ApiKey(9-15)frontend/src/types/button.ts (1)
Button(4-9)
🔇 Additional comments (1)
frontend/src/app/settings/api-keys/page.tsx (1)
17-17: LGTM! Addresses previous review feedback.The
LoadingSpinnerimport is appropriate for the inline loading state and correctly addresses the past review concern about using the full-pageApiKeysSkeletoninside the card.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
frontend/src/app/settings/api-keys/page.tsx (2)
74-74: Consider moving ContentType outside the component.Defining the
ContentTypetype inside the component function is valid but moving it to module scope would improve organization and enable potential reuse across files.🔎 Proposed refactor
Move the type definition above the component:
+type ContentType = 'error' | 'loading' | 'empty' | 'table' + export default function Page() { // ... - type ContentType = 'error' | 'loading' | 'empty' | 'table'
222-282: Refactor successfully eliminates nested ternary.The switch-based approach clearly separates the four rendering states and successfully addresses the PR objective of replacing nested ternary expressions with explicit conditionals. The logic is correct and all cases are properly handled.
The IIFE wrapper (
(() => { switch... })()) is valid but adds an extra layer. Consider extracting to a named function for cleaner code:🔎 Optional refactor
+ const renderContent = () => { + switch (contentType) { + case 'error': + return ( + <div className="rounded-md bg-red-50 p-4 text-red-700 dark:bg-red-900/20 dark:text-red-400"> + Error loading API keys + </div> + ) + case 'loading': + return <LoadingSpinner /> + case 'empty': + return ( + <div className="rounded-md bg-gray-50 p-8 text-center text-gray-500 dark:bg-gray-800/50 dark:text-gray-400"> + You don't have any API keys yet. + </div> + ) + case 'table': + return ( + <div className="overflow-x-auto"> + <table className="w-full border-collapse"> + {/* ... table content ... */} + </table> + </div> + ) + default: + return null + } + } + <SecondaryCard> {/* ... */} - {(() => { - switch (contentType) { - // ... cases ... - } - })()} + {renderContent()} </SecondaryCard>
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/app/settings/api-keys/page.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/app/settings/api-keys/page.tsx (2)
frontend/src/types/apiKey.ts (1)
ApiKey(9-15)frontend/src/types/button.ts (1)
Button(4-9)
🔇 Additional comments (1)
frontend/src/app/settings/api-keys/page.tsx (1)
17-17: LGTM!The LoadingSpinner import appropriately addresses the previous feedback about using a lightweight loading indicator instead of the full-page ApiKeysSkeleton.
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
@anurag2787 thanks for working on this. Left a suggestion/question!
| {(() => { | ||
| switch (contentType) { | ||
| case 'error': | ||
| return ( | ||
| <div className="rounded-md bg-red-50 p-4 text-red-700 dark:bg-red-900/20 dark:text-red-400"> | ||
| Error loading API keys | ||
| </div> | ||
| ) | ||
| case 'loading': | ||
| return <LoadingSpinner /> | ||
| case 'empty': | ||
| return ( | ||
| <div className="rounded-md bg-gray-50 p-8 text-center text-gray-500 dark:bg-gray-800/50 dark:text-gray-400"> | ||
| You don't have any API keys yet. | ||
| </div> | ||
| ) | ||
| case 'table': | ||
| return ( | ||
| <div className="overflow-x-auto"> | ||
| <table className="w-full border-collapse"> | ||
| <thead> | ||
| <tr className="border-b-1 border-b-gray-200 dark:border-b-gray-700"> | ||
| <th className="py-3 text-left font-semibold">Name</th> | ||
| <th className="py-3 text-left font-semibold">ID</th> | ||
| <th className="py-3 text-left font-semibold">Created</th> | ||
| <th className="py-3 text-left font-semibold">Expires</th> | ||
| <th className="py-3 text-right font-semibold">Actions</th> | ||
| </tr> | ||
| </thead> | ||
| <tbody> | ||
| {(data?.apiKeys ?? []).map((key: ApiKey) => ( | ||
| <tr | ||
| key={key.uuid} | ||
| className="border-b border-b-gray-200 dark:border-b-gray-700" | ||
| > | ||
| <td className="py-3">{key.name}</td> | ||
| <td className="py-3 font-mono text-sm">{key.uuid}</td> | ||
| <td className="py-3">{format(new Date(key.createdAt), 'PP')}</td> | ||
| <td className="py-3"> | ||
| {key.expiresAt ? format(new Date(key.expiresAt), 'PP') : 'Never'} | ||
| </td> | ||
| <td className="py-3 text-right"> | ||
| <Button | ||
| variant="light" | ||
| size="sm" | ||
| onPress={() => setKeyToRevoke(key)} | ||
| className="text-red-600 hover:bg-red-50 dark:hover:bg-red-900/20" | ||
| > | ||
| <FaTrash /> | ||
| </Button> | ||
| </td> | ||
| </tr> | ||
| ))} | ||
| </tbody> | ||
| </table> | ||
| </div> | ||
| ) | ||
| default: | ||
| return null | ||
| } | ||
| })()} | ||
| </SecondaryCard> |
There was a problem hiding this comment.
@anurag2787 I'm not sure I like using case inside a template. It is a bit hard to read..
What if we put those into separate "components" in this file and use a map to match with contentState. And the in the template we'll just get the right component based on contentState?
Something like this (just an example):
// 1. Define your components (separate functions)
const ErrorState = () => (
<div className="rounded-md bg-red-50 p-4 text-red-700 dark:bg-red-900/20 dark:text-red-400">
Error loading API keys
</div>
)
const EmptyState = () => (
<div className="rounded-md bg-gray-50 p-8 text-center text-gray-500 dark:bg-gray-800/50 dark:text-gray-400">
You don't have any API keys yet.
</div>
)
const ApiKeysTable = ({ data, onRevoke }) => (
<div className="overflow-x-auto">
{/* your table JSX */}
</div>
)
// 2. Map them (inside your main component)
const contentComponents: Record<ContentType, () => JSX.Element> = {
error: () => <ErrorState />,
loading: () => <LoadingSpinner />,
empty: () => <EmptyState />,
table: () => <ApiKeysTable data={data} onRevoke={setKeyToRevoke} />
}
// 3. Render (in your JSX)
{contentComponents[contentType]()}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/src/app/settings/api-keys/page.tsx (1)
131-152: Clean content-type dispatch pattern.This approach aligns well with the suggested pattern from previous review and effectively replaces nested ternaries with a declarative component map. The logic is clear and maintainable.
One optional refinement: consider moving
ContentTypeoutside the component to avoid type redefinition on each render and improve reusability.🔎 Optional: Extract type outside component
+type ContentType = 'error' | 'loading' | 'empty' | 'table' + export default function Page() { // ... state declarations ... - type ContentType = 'error' | 'loading' | 'empty' | 'table' - const getContentType = (): ContentType => {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/app/settings/api-keys/page.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 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/app/settings/api-keys/page.tsx
🧬 Code graph analysis (1)
frontend/src/app/settings/api-keys/page.tsx (2)
frontend/src/types/apiKey.ts (1)
ApiKey(9-15)frontend/src/types/button.ts (1)
Button(4-9)
🔇 Additional comments (4)
frontend/src/app/settings/api-keys/page.tsx (4)
8-8: LGTM on imports.The
Reactimport supports theReact.ReactNodetype annotation, andLoadingSpinnerappropriately replaces the full-pageApiKeysSkeletonfor in-card loading states as suggested in previous review.Also applies to: 18-18
24-78: Well-structured extraction of UI state components.The separation into
ErrorState,EmptyState, andApiKeysTablefollows the pattern suggested in previous review and improves readability. The null-safe access at line 55 with(data?.apiKeys ?? []).map(...)properly addresses the earlier concern about potential runtime errors whenapiKeysis undefined.
286-286: LGTM!The invocation
contentComponents[contentType]()is clean and achieves the PR objective of eliminating nested ternary expressions in favor of a more readable dispatch pattern.
233-235: Correct handling of initial vs. subsequent loading states.Using
ApiKeysSkeletonfor initial page load (when!data) whileLoadingSpinnerhandles in-card refetch loading properly addresses the UI layout concern from previous review.
|
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
@anurag2787 thanks for working on this 👍🏼
* Removed nested ternary * fixed coderabbit review * fixed soanrqube warning * fixed warning * Added check * Fixed review * Fixed sonarqube warning
2 participants
Description
Refactored nested ternary expressions into explicit conditional statements to improve readability and maintainability.
Resolves #2938
Checklist
make check-testlocally and all tests passed