Refactor Limit frontend function nesting to maximum 4 levels #2671
Conversation
Summary by CodeRabbit
WalkthroughCentralized repeated DOM/assertion logic into test helper functions across multiple unit tests, reorganized ProjectsHealthDashboardMetrics tests into focused helpers, added a ProjectHealthType alias, and extracted sorting/rendering helpers plus minor styling and safety tweaks in a candidates page component. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (5 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: 3
🧹 Nitpick comments (5)
frontend/__tests__/unit/components/ProjectTypeDashboardCard.test.tsx (1)
372-373: Remove the no-op assertion.The assertion
expect(true).toBe(true)adds no value and should be removed. If this was added to satisfy a linting rule about tests needing assertions, the helper functionexpectValidTypeRendersWithoutErroralready contains a meaningful assertion viaexpect().not.toThrow().Apply this diff:
- - expect(true).toBe(true)frontend/__tests__/unit/components/MultiSearch.test.tsx (2)
419-424: Remove no-op assertions after helper calls.Multiple tests follow the pattern of calling a helper within
waitFor, then addingexpect(true).toBe(true). These no-op assertions add no value since the helpers already contain meaningful assertions. This pattern appears throughout the file (lines 423, 437, 453, 467, 576, 595, 616, 633).Apply this diff to remove the no-op assertions:
await waitFor(() => expectSuggestionsToExist()) await user.keyboard('{ArrowDown}') await waitFor(() => expectFirstListItemHighlighted()) - - expect(true).toBe(true)Repeat for all similar occurrences throughout the file.
730-735: Simplify the nested waitFor pattern.Wrapping a single
expectcall insidewaitForand then calling that from anotherwaitForcreates unnecessary complexity. The inner helperexpectListItemsNotHighlightedcould be called directly within the outerwaitFor.Consider simplifying this pattern:
- await waitFor(() => - expect(mockFetchAlgoliaData).toHaveBeenCalledWith('chapters', 'new query', 1, 3) - ) - - await waitFor(() => expectListItemsNotHighlighted()) + await waitFor(() => { + expect(mockFetchAlgoliaData).toHaveBeenCalledWith('chapters', 'new query', 1, 3) + expectListItemsNotHighlighted() + })frontend/__tests__/unit/pages/ProjectsHealthDashboardMetrics.test.tsx (2)
107-124: Test behavior is unclear and may not validate meaningful functionality.The
testFilterOptionshelper clicks each filter button but only verifies the button exists afterward. This doesn't validate that clicking the button produced the intended filtering behavior or state change. Consider either:
- Removing the click action if only testing presence
- Adding assertions to verify the filtering behavior after each click
If testing button presence only:
const testFilterOptions = async () => { const filterOptions = [ 'Incubator', 'Lab', 'Production', 'Flagship', 'Healthy', 'Need Attention', 'Unhealthy', 'Reset All Filters', ] for (const option of filterOptions) { expect(screen.getAllByText(option).length).toBeGreaterThan(0) const button = screen.getByRole('button', { name: option }) - fireEvent.click(button) expect(button).toBeInTheDocument() } }
150-151: Remove no-op assertions.Multiple tests include
expect(true).toBe(true)after calling helpers. These assertions add no value since the helpers already contain meaningful assertions. Remove these no-op assertions at lines 150, 162, 169, 176, and 187.Apply this diff pattern to all occurrences:
await expectLoadingSpinnerExists() - - expect(true).toBe(true)Also applies to: 162-163, 169-170, 176-177, 187-188
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
frontend/__tests__/unit/components/MultiSearch.test.tsx(14 hunks)frontend/__tests__/unit/components/ProjectTypeDashboardCard.test.tsx(2 hunks)frontend/__tests__/unit/pages/ProjectsHealthDashboardMetrics.test.tsx(4 hunks)frontend/__tests__/unit/pages/UserDetails.test.tsx(3 hunks)frontend/src/app/board/[year]/candidates/page.tsx(8 hunks)
🧰 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/MultiSearch.test.tsxfrontend/__tests__/unit/components/ProjectTypeDashboardCard.test.tsxfrontend/__tests__/unit/pages/ProjectsHealthDashboardMetrics.test.tsxfrontend/__tests__/unit/pages/UserDetails.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/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 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/ProjectTypeDashboardCard.test.tsxfrontend/__tests__/unit/pages/ProjectsHealthDashboardMetrics.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/__tests__/unit/pages/ProjectsHealthDashboardMetrics.test.tsx
🧬 Code graph analysis (2)
frontend/src/app/board/[year]/candidates/page.tsx (2)
backend/apps/github/models/user.py (2)
chapters(116-125)projects(128-137)backend/apps/owasp/api/internal/queries/snapshot.py (1)
snapshot(14-22)
frontend/__tests__/unit/pages/ProjectsHealthDashboardMetrics.test.tsx (1)
frontend/__tests__/unit/data/mockProjectsHealthMetricsData.ts (1)
mockHealthMetricsData(1-25)
🔇 Additional comments (6)
frontend/__tests__/unit/pages/UserDetails.test.tsx (1)
99-112: Well-designed helper functions.The badge helper functions (
getBadgeElements,getBadgeTestIds,expectBadgesInCorrectOrder) follow good separation of concerns and improve test maintainability by centralizing badge-related assertions. This refactor successfully reduces nesting while maintaining clarity.frontend/__tests__/unit/pages/ProjectsHealthDashboardMetrics.test.tsx (2)
19-30: Well-structured mock helper functions.The
createDropDownMockItemandcreateDropDownMockSectionhelpers effectively reduce complexity in the mock implementation and improve readability.
239-257: Excellent refactor that improves maintainability.The
testMetricsDataDisplayhelper effectively consolidates repetitive metric assertions into a reusable function, reducing nesting and improving test clarity.frontend/src/app/board/[year]/candidates/page.tsx (3)
134-168: Good extraction of rendering logic.The helper functions effectively reduce nesting and improve readability by centralizing the rendering logic for channel and repository links. The consistent styling and clear parameter passing make the code more maintainable.
206-206: Effective use of sorting helpers to reduce nesting.The integration of the sorting helper functions successfully reduces nesting depth and improves code readability, aligning with the PR objective. The mutation concerns are already addressed in the earlier comments on the helper functions themselves.
Also applies to: 235-235, 534-536, 611-612
258-258: Clean refactoring of rendering logic and UI utilities.The changes effectively reduce nesting:
- Line 258: The
shrink-0utility is the modern Tailwind equivalent offlex-shrink-0and works correctly- Lines 562-566 and 642-646: The rendering helpers cleanly replace inline JSX, making the code more maintainable
These changes align well with the PR objective of reducing function nesting depth.
Also applies to: 562-566, 642-646
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
frontend/__tests__/unit/components/MultiSearch.test.tsx (1)
106-171: Consider defensive queries in highlight-checking helpers.The helpers
expectFirstListItemHighlighted,expectSecondListItemHighlighted, andexpectListItemsNotHighlightedusegetAllByRole('listitem'), which throws immediately if no list items exist. While this works insidewaitFor(which catches and retries), it differs from helpers likeexpectNoListItemsthat usequeryAllByRole. For consistency and clearer intent, consider either:
- Adding an explicit length check before accessing array indices
- Using
queryAllByRolewith existence assertions firstAlso,
expectChaptersCountEqualsThree(lines 169-171) is a single-use wrapper aroundexpectChaptersCountEquals(3). Consider removing it and callingexpectChaptersCountEquals(3)directly at line 500, or using an inline arrow function.Example refactor for defensive queries:
const expectFirstListItemHighlighted = () => { const listItems = screen.getAllByRole('listitem') + expect(listItems.length).toBeGreaterThanOrEqual(1) expect(listItems[0]).toHaveClass('bg-gray-100') }For the wrapper function:
-const expectChaptersCountEqualsThree = () => { - expectChaptersCountEquals(3) -}Then at line 500:
-await waitFor(expectChaptersCountEqualsThree) +await waitFor(() => expectChaptersCountEquals(3))
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/__tests__/unit/components/MultiSearch.test.tsx(14 hunks)frontend/__tests__/unit/components/ProjectTypeDashboardCard.test.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/tests/unit/components/ProjectTypeDashboardCard.test.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 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/MultiSearch.test.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
frontend/src/app/board/[year]/candidates/page.tsx (3)
133-148: Tighten the type signature for consistency.The
messageCountparameter is typed asstring | number, butchannelCommunicationsisRecord<string, number>, so the value will always benumber. The defensiveNumber()coercion at line 145 is unnecessary with the correct type.Apply this diff to tighten the type:
- const renderChannelLink = (channelName: string, messageCount: string | number) => ( + const renderChannelLink = (channelName: string, messageCount: number) => ( <a key={channelName} href={`https://owasp.slack.com/archives/${channelName}`} target="_blank" rel="noopener noreferrer" className="inline-flex items-center gap-1.5 rounded-md bg-blue-50 px-2 py-1 text-xs font-medium text-blue-700 ring-1 ring-blue-700/10 ring-inset hover:bg-blue-100 dark:bg-blue-900/20 dark:text-blue-400 dark:ring-blue-400/30 dark:hover:bg-blue-900/30" onClick={(e) => e.stopPropagation()} > <span>#{channelName}</span> <span className="rounded bg-blue-100 px-1.5 py-0.5 text-[10px] font-semibold text-blue-800 dark:bg-blue-800/40 dark:text-blue-300"> - {Number(messageCount)} messages + {messageCount} messages </span> </a> )
150-168: Remove redundant Number conversion.The
countparameter is already typed asnumber, so theNumber(count)conversion at line 152 is redundant. You can usecountdirectly.Apply this diff to simplify:
const renderRepositoryLink = (repoName: string, count: number) => { - const commitCount = Number(count) return ( <a key={repoName} href={`https://github.com/${repoName}/commits?author=${candidate.member?.login}`} target="_blank" rel="noopener noreferrer" className="inline-flex items-center gap-1.5 rounded-md bg-blue-50 px-2 py-1 text-xs font-medium text-blue-700 ring-1 ring-blue-700/10 ring-inset hover:bg-blue-100 dark:bg-blue-900/20 dark:text-blue-400 dark:ring-blue-400/30 dark:hover:bg-blue-900/30" onClick={(e) => e.stopPropagation()} > <span>{repoName}</span> <span className="rounded bg-blue-100 px-1.5 py-0.5 text-[10px] font-semibold text-blue-800 dark:bg-blue-800/40 dark:text-blue-300"> - {commitCount} commits + {count} commits </span> </a> ) }
170-230: Remove redundant empty check.The check at line 193 (
if (sortedChannels.length === 0) return null) is redundant. IfhasChannelsis false, the function already returns at line 177. SincehasChannelsverifies thatsnapshot.channelCommunicationshas keys,sortedChannelswill always have at least one entry when execution reaches line 193.Apply this diff to remove the unreachable check:
const sortedChannels = sortChannelsByMessageCount( Object.entries(snapshot.channelCommunications) ) - if (sortedChannels.length === 0) return null - const topChannel = sortedChannels[0] const [topChannelName, topChannelCount] = topChannelfrontend/__tests__/unit/components/MultiSearch.test.tsx (1)
489-506: Use index-specific configuration or existence check for navigation test.This test verifies chapter navigation but expects exactly 3 chapters because
defaultPropssearches across 3 indexes. This couples the assertion to the default configuration rather than the test's intent. Other navigation tests (lines 532-533, 552, 570) overrideindexesto be specific.Apply this diff for consistency with other navigation tests:
it('navigates to chapter page when chapter is clicked', async () => { mockFetchAlgoliaData.mockResolvedValue({ hits: [mockChapter], totalPages: 1, }) const user = userEvent.setup() - render(<MultiSearchBar {...defaultProps} />) + render(<MultiSearchBar {...defaultProps} indexes={['chapters']} />) const input = screen.getByPlaceholderText('Search...') await user.type(input, 'test') - await waitFor(expectChaptersCountEqualsThree) + await waitFor(expectTestChaptersExist) const chapterElements = screen.getAllByText('Test Chapter') await user.click(chapterElements[0]) expect(mockPush).toHaveBeenCalledWith('/chapters/test-chapter') })This makes the test's intent clearer and decouples it from the default multi-index configuration.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/__tests__/unit/components/MultiSearch.test.tsx(14 hunks)frontend/__tests__/unit/components/ProjectTypeDashboardCard.test.tsx(4 hunks)frontend/src/app/board/[year]/candidates/page.tsx(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/tests/unit/components/ProjectTypeDashboardCard.test.tsx
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-17T16:47:05.560Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:427-427
Timestamp: 2025-11-17T16:47:05.560Z
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/MultiSearch.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/MultiSearch.test.tsx
🧬 Code graph analysis (1)
frontend/src/app/board/[year]/candidates/page.tsx (2)
backend/apps/owasp/api/internal/queries/snapshot.py (1)
snapshot(14-22)backend/apps/github/models/user.py (2)
chapters(116-125)projects(128-137)
🔇 Additional comments (5)
frontend/src/app/board/[year]/candidates/page.tsx (5)
119-121: Past mutation issue resolved.The sorting function now correctly creates a shallow copy before sorting, addressing the previous review concern about in-place mutation.
123-125: Past mutation issue resolved.The function now sorts immutably by creating a shallow copy first, addressing the previous review concern.
127-131: Past issues resolved.Both the mutation and type signature concerns from the previous review have been addressed. The function now sorts immutably and uses the correct type signature matching
channelCommunications: Record<string, number>.
320-320: LGTM: Modern Tailwind utility class.The change from
flex-shrink-0toshrink-0uses the modern Tailwind CSS shorthand notation, which is equivalent and preferred.
119-230: Excellent refactoring to reduce nesting.The extraction of helper functions successfully addresses the PR objective of reducing function nesting (typescript:S2004 rule). The code is now more maintainable and readable, with clear separation of concerns. All previously flagged mutation issues have been properly resolved with immutable sorting operations.
ahmedxgouda
left a comment
ahmedxgouda
left a comment
There was a problem hiding this comment.
Good job. Left a couple of comments.
frontend/__tests__/unit/components/ProjectTypeDashboardCard.test.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
frontend/src/app/board/[year]/candidates/page.tsx (3)
133-148: Consider tightening the type signature.The
messageCountparameter is typed asstring | number, but actual usage (line 223) always passes anumberfromObject.entries(snapshot.channelCommunications)wherechannelCommunicationsisRecord<string, number>. TheNumber()coercion on line 145 would be unnecessary if the parameter type was justnumber.Apply this diff to align the type with actual usage:
- const renderChannelLink = (channelName: string, messageCount: string | number) => ( + const renderChannelLink = (channelName: string, messageCount: number) => ( <a key={channelName} href={`https://owasp.slack.com/archives/${channelName}`} target="_blank" rel="noopener noreferrer" className="inline-flex items-center gap-1.5 rounded-md bg-blue-50 px-2 py-1 text-xs font-medium text-blue-700 ring-1 ring-blue-700/10 ring-inset hover:bg-blue-100 dark:bg-blue-900/20 dark:text-blue-400 dark:ring-blue-400/30 dark:hover:bg-blue-900/30" onClick={(e) => e.stopPropagation()} > <span>#{channelName}</span> <span className="rounded bg-blue-100 px-1.5 py-0.5 text-[10px] font-semibold text-blue-800 dark:bg-blue-800/40 dark:text-blue-300"> - {Number(messageCount)} messages + {messageCount} messages </span> </a> )
150-168: Remove redundant type coercion and one-time variable.Line 152 uses
Number(count)whencountis already typed asnumber, and the resultingcommitCountvariable is only used once on line 164.Apply this diff to simplify:
const renderRepositoryLink = (repoName: string, count: number) => { - const commitCount = Number(count) return ( <a key={repoName} href={`https://github.com/${repoName}/commits?author=${candidate.member?.login}`} target="_blank" rel="noopener noreferrer" className="inline-flex items-center gap-1.5 rounded-md bg-blue-50 px-2 py-1 text-xs font-medium text-blue-700 ring-1 ring-blue-700/10 ring-inset hover:bg-blue-100 dark:bg-blue-900/20 dark:text-blue-400 dark:ring-blue-400/30 dark:hover:bg-blue-900/30" onClick={(e) => e.stopPropagation()} > <span>{repoName}</span> <span className="rounded bg-blue-100 px-1.5 py-0.5 text-[10px] font-semibold text-blue-800 dark:bg-blue-800/40 dark:text-blue-300"> - {commitCount} commits + {count} commits </span> </a> ) }
170-230: Remove redundant type coercion.On line 213,
Number(topChannelCount)is redundant sincetopChannelCountis destructured fromsortedChannels[0], which comes fromObject.entries(snapshot.channelCommunications)wherechannelCommunicationsis typed asRecord<string, number>. The value is already anumber.Apply this diff:
<span>#{topChannelName}</span> <span className="rounded bg-green-100 px-1.5 py-0.5 text-[10px] font-semibold text-green-800 dark:bg-green-800/40 dark:text-green-300"> - {Number(topChannelCount)} messages + {topChannelCount} messages </span>Otherwise, the function structure is excellent with proper early returns and clear separation of concerns.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/__tests__/unit/components/ProjectTypeDashboardCard.test.tsx(4 hunks)frontend/src/app/board/[year]/candidates/page.tsx(7 hunks)frontend/src/types/project.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:169-171
Timestamp: 2025-11-17T17:30:42.119Z
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))`.
📚 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/__tests__/unit/components/ProjectTypeDashboardCard.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/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 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/ProjectTypeDashboardCard.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/ProjectTypeDashboardCard.test.tsx
📚 Learning: 2025-11-17T16:47:05.560Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:427-427
Timestamp: 2025-11-17T16:47:05.560Z
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/ProjectTypeDashboardCard.test.tsx
📚 Learning: 2025-11-17T17:30:42.119Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:169-171
Timestamp: 2025-11-17T17:30:42.119Z
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/ProjectTypeDashboardCard.test.tsx
🧬 Code graph analysis (2)
frontend/src/app/board/[year]/candidates/page.tsx (2)
backend/apps/owasp/api/internal/queries/snapshot.py (1)
snapshot(14-22)backend/apps/github/models/user.py (2)
chapters(116-125)projects(128-137)
frontend/__tests__/unit/components/ProjectTypeDashboardCard.test.tsx (1)
frontend/src/types/project.ts (1)
ProjectHealthType(10-10)
🔇 Additional comments (7)
frontend/src/app/board/[year]/candidates/page.tsx (4)
119-131: LGTM! Sorting helpers now use immutable operations.The sorting helper functions now properly create shallow copies before sorting, addressing the previous review comments. The type signatures are also correct and match the actual data structures being sorted.
320-320: LGTM! Modern Tailwind CSS shorthand.Using
shrink-0instead offlex-shrink-0is the modern Tailwind CSS shorthand notation. Both are functionally equivalent.
268-268: LGTM! Helpers properly integrated throughout the component.The extracted helper functions are consistently used throughout the component, successfully reducing nesting depth and improving readability. The integration at lines 268, 297, 596-598, and 624-626 is clean and maintainable.
Also applies to: 297-297, 596-626
650-650: LGTM! Successful nesting reduction.The call to
renderTopActiveChannels()successfully replaces previously inline nested rendering logic, achieving the PR objective of reducing nesting depth while maintaining functionality.frontend/src/types/project.ts (1)
10-11: LGTM! Proper type centralization.The new
ProjectHealthTypeexport provides type safety for health status values and centralizes the type definition in the appropriatetypes/directory, addressing the previous review feedback.frontend/__tests__/unit/components/ProjectTypeDashboardCard.test.tsx (2)
5-5: LGTM! Test helper successfully reduces nesting.The import of
ProjectHealthTypeprovides proper type safety, and theexpectValidTypeRendersWithoutErrorhelper function successfully reduces nesting depth in test assertions while maintaining clarity.Also applies to: 68-72
363-370: Test assertions properly typed.The use of
Array<ProjectHealthType>ensures type-safe iteration over valid health type values. Theexpect(true).toBe(true)on line 369 is a known workaround when ESLint'sjest/expect-expectrule doesn't detect expectations inside helper functions—this pattern is documented in the retrieved learnings.Based on learnings
There was a problem hiding this comment.
What is this file for?
There was a problem hiding this comment.
Actually another PRs file got pushed to this Sorry for inconvenience
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
@anurag2787 looks good. Can you clean up the file that was clearly pushed by accident?
|
4 participants
Resolves #2487
Description
Refactored frontend code to eliminate excessive nesting typescript:S2004 across tests and components extracted reusable helper functions replaced deeply nested callbacks,
Checklist
make check-testlocally; all checks and tests passed.