refactor: frontend functions to reduce deep nesting violations #2491
Conversation
Fix TypeScript rule S2004 violations by reducing functions with more than 4 levels of nesting: - Extract test utility functions in MultiSearch.test.tsx (11 violations) - Simplify nested patterns in ProjectTypeDashboardCard.test.tsx (1 violation) - Refactor setTimeout/waitFor nesting in Home.test.tsx (1 violation) - Extract MockDropdownSection component in ProjectsHealthDashboardMetrics.test.tsx (1 violation) - Add extractBadgeTestIds utility in UserDetails.test.tsx (1 violation) - Create utility functions and badge components in board candidates page (6 violations) Improves code readability, maintainability, and reduces cognitive load for developers. All functionality preserved with zero regression.
Summary by CodeRabbit
WalkthroughIntroduces test utility helpers and component/component mocks across multiple test suites and refactors the board candidates page to extract data-fetching helpers and presentational Badge components, consolidating rendering and sorting logic. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/__tests__/unit/pages/Home.test.tsx (1)
297-316: Fix unreliable test pattern with setTimeout inside waitFor.The
setTimeoutat line 311 creates assertions that won't be properly awaited by the test framework. The assertions inside thesetTimeoutcallback will execute after 2 seconds, butwaitForwon't wait for them, potentially causing false positives or flaky tests.If the intent is to wait for AnimatedCounter animations, consider one of these approaches:
Option 1: Remove setTimeout and verify counter values immediately
- // Check that AnimatedCounter components are rendered (they should show some numeric values) await waitFor(() => { - // Look for any numeric content that indicates the counters are working const statsSection = screen.getByText('Active Projects').closest('section') || document.body - expect(statsSection).toBeInTheDocument(); - - setTimeout(() => { - Object.values(stats).forEach((value) => - expect(screen.getByText(`${millify(value)}+`)).toBeInTheDocument() - ) - }, 2000) + expect(statsSection).toBeInTheDocument() + + // Verify the formatted counter values are rendered + expect(screen.getByText('90')).toBeInTheDocument() // activeProjectsStats + expect(screen.getByText('250')).toBeInTheDocument() // activeChaptersStats + expect(screen.getByText('9.6k')).toBeInTheDocument() // contributorsStats })Option 2: If animation delay is required, use nested waitFor
await waitFor(() => { const statsSection = screen.getByText('Active Projects').closest('section') || document.body - expect(statsSection).toBeInTheDocument(); - - setTimeout(() => { - Object.values(stats).forEach((value) => - expect(screen.getByText(`${millify(value)}+`)).toBeInTheDocument() - ) - }, 2000) + expect(statsSection).toBeInTheDocument() }) + + // Wait for animated values to appear + await waitFor(() => { + Object.values(stats).forEach((value) => + expect(screen.getByText(`${millify(value)}+`)).toBeInTheDocument() + ) + }, { timeout: 3000 })frontend/src/app/board/[year]/candidates/page.tsx (1)
586-589: Copy nit: “lead by” → “led by”.- No chapters or projects are lead by this candidate + No chapters or projects are led by this candidate
🧹 Nitpick comments (6)
frontend/__tests__/unit/components/ProgramCard.test.tsx (1)
44-55: Optimize timestamp comparisons by computing once.The mock creates new
Dateobjects on every call to compare timestamps, which is inefficient when the mock is invoked multiple times during test execution.Consider computing the target timestamps once outside the mock implementation:
beforeEach(() => { jest.clearAllMocks() + const timestamp1 = new Date('2024-01-01T00:00:00Z').getTime() + const timestamp2 = new Date('2024-12-31T23:59:59Z').getTime() + // Mock toLocaleDateString to ensure consistent formatting jest.spyOn(Date.prototype, 'toLocaleDateString').mockImplementation(function(this: Date, locale, options) { - if (this.getTime() === new Date('2024-01-01T00:00:00Z').getTime()) { + if (this.getTime() === timestamp1) { return 'Jan 1, 2024' } - if (this.getTime() === new Date('2024-12-31T23:59:59Z').getTime()) { + if (this.getTime() === timestamp2) { return 'Dec 31, 2024' } // Call the original implementation for other dates return new Intl.DateTimeFormat(locale, options).format(this) }) })frontend/src/app/board/[year]/candidates/page.tsx (5)
2-4: Unify Apollo imports and use type-only imports to reduce bundle size.Import hooks from '@apollo/client' and import ApolloClient/React as types only.
-import { useQuery, useApolloClient } from '@apollo/client/react' -import { ApolloClient } from '@apollo/client'; +import { useQuery, useApolloClient } from '@apollo/client' +import type { ApolloClient } from '@apollo/client'; -import React from 'react' +import type React from 'react'Also applies to: 19-19
345-353: Parallelize chapter fetches, dedupe keys, and guard against race on unmount.- const chapterKeys = Object.keys(snapshot.chapterContributions) - const chapters: Chapter[] = [] - - for (const key of chapterKeys) { - const chapter = await fetchChapterData(client, key) - if (chapter) { - chapters.push(chapter) - } - } - - setLedChapters(sortItemsByName(chapters)) + const rawKeys = Object.keys(snapshot.chapterContributions) + const bareKeys = Array.from( + new Set(rawKeys.map(k => k.startsWith('www-chapter-') ? k.replace('www-chapter-', '') : k)), + ) + let cancelled = false + const results = await Promise.all(bareKeys.map(k => fetchChapterData(client, k))) + if (cancelled) return + setLedChapters(sortItemsByName(results.filter(Boolean) as Chapter[]))Also add a cleanup to the effect:
- fetchChapters() + let cancelled = false + fetchChapters() + return () => { cancelled = true }
366-374: Do the same for projects: parallelize, dedupe, and cancel on unmount.- const projectKeys = Object.keys(snapshot.projectContributions) - const projects: Project[] = [] - - for (const key of projectKeys) { - const project = await fetchProjectData(client, key) - if (project) { - projects.push(project) - } - } - - setLedProjects(sortItemsByName(projects)) + const rawKeys = Object.keys(snapshot.projectContributions) + const bareKeys = Array.from( + new Set(rawKeys.map(k => k.startsWith('www-project-') ? k.replace('www-project-', '') : k)), + ) + let cancelled = false + const results = await Promise.all(bareKeys.map(k => fetchProjectData(client, k))) + if (cancelled) return + setLedProjects(sortItemsByName(results.filter(Boolean) as Project[]))And add the cleanup:
- fetchProjects() + let cancelled = false + fetchProjects() + return () => { cancelled = true }
100-101: Remove redundantkeyon inner Links inside badge components.Keys belong on the array elements where badges are mapped, not inside the badge internals.
- <Link - key={chapter.id} + <Link- <Link - key={project.id} + <LinkAlso applies to: 139-140
413-421: Simplify LinkedIn anchor: stop propagation only; let the anchor open.
preventDefault+window.openduplicates default behavior withtarget="_blank".- onClick={(e) => { - e.stopPropagation() - e.preventDefault() - window.open( - `https://linkedin.com/in/${candidate.member.linkedinPageId}`, - '_blank', - 'noopener,noreferrer' - ) - }} + onClick={(e) => e.stopPropagation()}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
frontend/__tests__/unit/components/MultiSearch.test.tsx(5 hunks)frontend/__tests__/unit/components/ProgramCard.test.tsx(2 hunks)frontend/__tests__/unit/components/ProjectTypeDashboardCard.test.tsx(1 hunks)frontend/__tests__/unit/data/mockProjectDetailsData.ts(1 hunks)frontend/__tests__/unit/pages/Home.test.tsx(4 hunks)frontend/__tests__/unit/pages/ProjectDetails.test.tsx(1 hunks)frontend/__tests__/unit/pages/ProjectsHealthDashboardMetrics.test.tsx(1 hunks)frontend/__tests__/unit/pages/UserDetails.test.tsx(2 hunks)frontend/src/app/board/[year]/candidates/page.tsx(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
frontend/__tests__/unit/pages/Home.test.tsx (1)
frontend/__tests__/unit/data/mockHomeData.ts (1)
mockGraphQLData(1-158)
frontend/src/app/board/[year]/candidates/page.tsx (4)
frontend/src/types/__generated__/boardQueries.generated.ts (2)
GetChapterByKeyDocument(35-35)GetProjectByKeyDocument(36-36)backend/apps/owasp/api/internal/queries/snapshot.py (1)
snapshot(14-22)backend/apps/owasp/models/member_snapshot.py (1)
MemberSnapshot(11-132)backend/apps/owasp/api/internal/queries/chapter.py (1)
chapter(14-19)
🔇 Additional comments (15)
frontend/__tests__/unit/components/ProjectTypeDashboardCard.test.tsx (1)
356-368: Effective nesting reduction via extracted callback function.The refactoring at line 364 successfully reduces nesting by extracting the inline forEach callback into a named
testTypeValuefunction. The.not.toThrow()assertion correctly captures the intent that rendering with valid types must not throw, improving both readability and compliance with the 4-level nesting limit.frontend/__tests__/unit/components/ProgramCard.test.tsx (3)
57-59: LGTM!Properly restores mocks after each test to ensure test isolation.
237-237: LGTM!Comment appropriately clarifies that the exact string match is possible due to the mocked date formatting.
44-55: Clarify how this change addresses the PR objective.The PR objective states: "Fix TypeScript rule S2004 violations by reducing functions with more than 4 levels of nesting." However, the changes in this file introduce date mocking infrastructure rather than reducing existing deep nesting. Additionally,
ProgramCard.test.tsxis not mentioned in the PR summary's list of files with violations addressed.Could you clarify:
- Does this file have a deep nesting violation that this change addresses?
- Or is this change included as a related test infrastructure improvement?
frontend/__tests__/unit/data/mockProjectDetailsData.ts (1)
28-28: LGTM!The addition of the
createdAtfield to the health metrics mock data is appropriate and supports deterministic testing of health-related UI components.frontend/__tests__/unit/pages/ProjectDetails.test.tsx (1)
30-50: LGTM!The addition of mocks for LineChart, BarChart, and environment configuration appropriately isolates the ProjectDetailsPage component during testing, enabling tests to verify health metrics UI without depending on real chart implementations.
frontend/__tests__/unit/pages/ProjectsHealthDashboardMetrics.test.tsx (1)
19-29: Well-structured extraction!The
MockDropdownSectiontest utility component effectively reduces nesting depth by encapsulating the section rendering logic. The component is focused, reusable, and maintains the original test behavior.frontend/__tests__/unit/pages/UserDetails.test.tsx (1)
82-84: LGTM!The
extractBadgeTestIdsutility function appropriately reduces nesting depth and improves test readability by extracting the badge test ID mapping logic.frontend/__tests__/unit/pages/Home.test.tsx (2)
38-54: LGTM!The AnimatedCounter mock appropriately handles edge cases (undefined/null counts) and provides simple number formatting for test scenarios. The logic is clear and suitable for test isolation.
87-93: LGTM!The ChapterMap and ContributionHeatmap mocks appropriately isolate these components during testing with simple placeholder implementations.
frontend/__tests__/unit/components/MultiSearch.test.tsx (4)
86-123: Excellent test utility extraction!The test helper functions effectively reduce nesting depth and significantly improve test readability:
createUserWithSetup,renderMultiSearchWithDefaults, andgetInputElementstandardize setup and element accessexpectSuggestionsVisible,expectListItemHighlighted,expectNoSuggestions, andexpectSuggestionsForEachprovide focused assertion helpersEach utility is well-named, type-safe, and serves a single purpose. This is exemplary refactoring that directly addresses the PR objectives.
346-359: LGTM!The refactored test effectively uses the new utility functions (
createUserWithSetup,renderMultiSearchWithDefaults,getInputElement,expectSuggestionsForEach) to reduce nesting depth while maintaining clear test logic.
378-437: LGTM!The keyboard navigation tests are well-refactored using the utility functions. The use of
createUserWithSetup,renderMultiSearchWithDefaults,getInputElement,expectListItemHighlighted,expectSuggestionsVisible, andexpectNoSuggestionsmakes the tests more readable and reduces nesting depth effectively.
439-453: LGTM!The Enter key test is cleanly refactored using the utility functions, reducing nesting while maintaining clear test intent.
frontend/src/app/board/[year]/candidates/page.tsx (1)
80-80: Verify Slack URL expects channel ID, not name.Slack “archives/…” links typically require channel IDs (e.g., C123…), not human-readable names. If
channelCommunicationsuses names, links may 404. Consider storing/deriving channel IDs or mapping names→IDs before rendering.Would you confirm what
snapshot.channelCommunicationskeys contain (ID vs name)? If names, I can propose a safe mapping strategy.Also applies to: 223-235
| {sortedChannels.slice(1).map(([channelName, messageCount]) => ( | ||
| <a | ||
| <ChannelBadge | ||
| 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 | ||
| </span> | ||
| </a> | ||
| channelName={channelName} | ||
| messageCount={Number(messageCount)} | ||
| onStopPropagation={(e) => e.stopPropagation()} | ||
| /> |
There was a problem hiding this comment.
Limit to 5 channels to match the section title.
- <ChannelBadge
+ <ChannelBadge
key={channelName}
channelName={channelName}
messageCount={Number(messageCount)}
onStopPropagation={(e) => e.stopPropagation()}
/>And slice only the next 4:
- {sortedChannels.slice(1).map(([channelName, messageCount]) => (
+ {sortedChannels.slice(1, 5).map(([channelName, messageCount]) => (📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {sortedChannels.slice(1).map(([channelName, messageCount]) => ( | |
| <a | |
| <ChannelBadge | |
| 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 | |
| </span> | |
| </a> | |
| channelName={channelName} | |
| messageCount={Number(messageCount)} | |
| onStopPropagation={(e) => e.stopPropagation()} | |
| /> | |
| {sortedChannels.slice(1, 5).map(([channelName, messageCount]) => ( | |
| <ChannelBadge | |
| key={channelName} | |
| channelName={channelName} | |
| messageCount={Number(messageCount)} | |
| onStopPropagation={(e) => e.stopPropagation()} | |
| /> |
🤖 Prompt for AI Agents
In frontend/src/app/board/[year]/candidates/page.tsx around lines 714 to 720,
the code maps over sortedChannels.slice(1) which renders all channels after the
first; change the slice to only take the next four channels (slice(1, 5)) so the
UI displays a total of five channels to match the section title.
|
@coderabbitai generate docstrings |
✅ Actions performedInitiated docstring generation; will generate only if new commits exist. |
|
Caution Docstrings generation - FAILED No docstrings were generated. |
|
|
@coderabbitai generate docstrings |
✅ Actions performedInitiated docstring generation; will generate only if new commits exist. |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/app/board/[year]/candidates/page.tsx (1)
586-589: Minor copy: “led by”, not “lead by”.- No chapters or projects are lead by this candidate + No chapters or projects are led by this candidate
♻️ Duplicate comments (1)
frontend/src/app/board/[year]/candidates/page.tsx (1)
704-720: Limit channels to 5 to match the header.Same rationale as repos; cap to next 4 after the top.
- {sortedChannels.slice(1).map(([channelName, messageCount]) => ( + {sortedChannels.slice(1, 5).map(([channelName, messageCount]) => (
🧹 Nitpick comments (7)
frontend/src/app/board/[year]/candidates/page.tsx (7)
2-4: Unify Apollo imports; use type-only for types to avoid extra bundle.Avoid mixing
@apollo/client/reactand@apollo/client. Import hooks from the root and import types as type-only.-import { useQuery, useApolloClient } from '@apollo/client/react' -import { ApolloClient } from '@apollo/client'; +import { useQuery, useApolloClient } from '@apollo/client' +import type { ApolloClient, NormalizedCacheObject } from '@apollo/client'Update function signatures below to use
ApolloClient<NormalizedCacheObject>.
19-19: Make React a type-only import (new JSX transform).No runtime
Reactusage; keep types only.-import React from 'react' +import type React from 'react'
36-46: Tighten client typing; avoidany.Use the cache type to improve safety.
-const fetchChapterData = async (client: ApolloClient<any>, key: string) => { +const fetchChapterData = async (client: ApolloClient<NormalizedCacheObject>, key: string) => {
48-58: Same here: precise typing for project fetch.-const fetchProjectData = async (client: ApolloClient<any>, key: string) => { +const fetchProjectData = async (client: ApolloClient<NormalizedCacheObject>, key: string) => {
304-304: Specify radix forparseInt(or useNumber).Prevents accidental octal parsing.
- variables: { year: Number.parseInt(year) }, + variables: { year: Number.parseInt(year, 10) },
343-353: Parallelize chapter fetches and de‑dup by id (perf + correctness).Current loop fires sequential requests and can duplicate when both bare/prefixed keys appear. Fetch in parallel and de‑dup before sorting.
- const chapterKeys = Object.keys(snapshot.chapterContributions) - const chapters: Chapter[] = [] - - for (const key of chapterKeys) { - const chapter = await fetchChapterData(client, key) - if (chapter) { - chapters.push(chapter) - } - } - - setLedChapters(sortItemsByName(chapters)) + const chapterKeys = Object.keys(snapshot.chapterContributions) + const results = await Promise.all( + chapterKeys.map((key) => fetchChapterData(client, key)) + ) + const deduped = [ + ...new Map(results.filter(Boolean).map((c) => [(c as Chapter).id, c as Chapter])).values(), + ] + setLedChapters(sortItemsByName(deduped))
363-374: Do the same for projects.Parallelize and de‑dup.
- const projectKeys = Object.keys(snapshot.projectContributions) - const projects: Project[] = [] - - for (const key of projectKeys) { - const project = await fetchProjectData(client, key) - if (project) { - projects.push(project) - } - } - - setLedProjects(sortItemsByName(projects)) + const projectKeys = Object.keys(snapshot.projectContributions) + const results = await Promise.all( + projectKeys.map((key) => fetchProjectData(client, key)) + ) + const deduped = [ + ...new Map(results.filter(Boolean).map((p) => [(p as Project).id, p as Project])).values(), + ] + setLedProjects(sortItemsByName(deduped))
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/app/board/[year]/candidates/page.tsx(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/app/board/[year]/candidates/page.tsx (3)
frontend/src/types/__generated__/boardQueries.generated.ts (2)
GetChapterByKeyDocument(35-35)GetProjectByKeyDocument(36-36)backend/apps/owasp/models/member_snapshot.py (1)
MemberSnapshot(11-132)backend/apps/owasp/api/internal/queries/chapter.py (1)
chapter(14-19)
🔇 Additional comments (2)
frontend/src/app/board/[year]/candidates/page.tsx (2)
64-78: Good fix: preserve 0 counts with nullish coalescing.The
??logic prevents truthiness bugs for zero-valued contributions. LGTM.
625-631: Repos section matches “Top 5” now.Using
slice(1, 5)aligns with the title and avoids overflow. Nice.Also applies to: 636-644
1 participant
Proposed change
Fix TypeScript rule S2004 violations by reducing functions with more than 4 levels of nesting:
Resolves #2487
Add the PR description here.
Improves code readability, maintainability, and reduces cognitive load for developers. All functionality preserved with zero regression.
Checklist
make check-testlocally; all checks and tests passed.