Refactor: replaced all forEach with for...of loop #2489
Conversation
Summary by CodeRabbit
Note: These changes are internal improvements with no impact on user-facing functionality or application behavior. WalkthroughReplaced many Array.prototype.forEach usages with for...of loops across ~30 frontend unit tests and one utility (githubHeatmap.ts); assertions and observable behavior were preserved, with index handling introduced where needed. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 |
|
Test: The linked issue must be assigned to the PR author. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
frontend/__tests__/unit/pages/UserDetails.test.tsx (2)
208-213: forEach → for...of: LGTMSame assertions, improved readability. Optional destructuring to reduce repetition.
- for (const milestone of milestones) { - expect(screen.getByText(milestone.title)).toBeInTheDocument() - expect(screen.getByText(milestone.repositoryName)).toBeInTheDocument() - expect(screen.getByText(`${milestone.openIssuesCount} open`)).toBeInTheDocument() - expect(screen.getByText(`${milestone.closedIssuesCount} closed`)).toBeInTheDocument() - } + for (const { title, repositoryName, openIssuesCount, closedIssuesCount } of milestones) { + expect(screen.getByText(title)).toBeInTheDocument() + expect(screen.getByText(repositoryName)).toBeInTheDocument() + expect(screen.getByText(`${openIssuesCount} open`)).toBeInTheDocument() + expect(screen.getByText(`${closedIssuesCount} closed`)).toBeInTheDocument() + }
188-191: Optional destructuring refactor confirmed valid; forEach migration completeThe codebase has successfully migrated from
forEachtofor...ofloops. The search found only one inactiveforEachin a comment, confirming active migration completion.The optional refactor suggestion to use destructuring in the loop remains valid and improves readability:
- for (const release of releases) { - expect(screen.getByText(release.name)).toBeInTheDocument() - expect(screen.getByText(release.repositoryName)).toBeInTheDocument() - } + for (const { name, repositoryName } of releases) { + expect(screen.getByText(name)).toBeInTheDocument() + expect(screen.getByText(repositoryName)).toBeInTheDocument() + }frontend/__tests__/unit/pages/OrganizationDetails.test.tsx (1)
119-124: for...of migration approvedMaintains intent; minor optional destructuring can tighten the assertions.
- for (const milestone of recentMilestones) { - expect(screen.getByText(milestone.title)).toBeInTheDocument() - expect(screen.getByText(milestone.repositoryName)).toBeInTheDocument() - expect(screen.getByText(`${milestone.openIssuesCount} open`)).toBeInTheDocument() - expect(screen.getByText(`${milestone.closedIssuesCount} closed`)).toBeInTheDocument() - } + for (const { title, repositoryName, openIssuesCount, closedIssuesCount } of recentMilestones) { + expect(screen.getByText(title)).toBeInTheDocument() + expect(screen.getByText(repositoryName)).toBeInTheDocument() + expect(screen.getByText(`${openIssuesCount} open`)).toBeInTheDocument() + expect(screen.getByText(`${closedIssuesCount} closed`)).toBeInTheDocument() + }frontend/__tests__/unit/components/ModuleList.test.tsx (1)
79-81: for...of migration approvedMinor nit: avoid shadowing the CommonJS
moduleidentifier in test contexts.- for (const module of modules) { - expect(screen.getByText(module)).toBeInTheDocument() - } + for (const mod of modules) { + expect(screen.getByText(mod)).toBeInTheDocument() + }frontend/__tests__/unit/components/TopContributorsList.test.tsx (1)
569-578: LGTM! forEach → for...of conversions are correct.All three loop refactors properly validate contributor items, avatars, and links. The explicit iteration syntax is clear and maintains identical test coverage.
Minor style note: The blank line added at line 568 before the loop is slightly unusual (other similar loops in this file don't have it), but this is purely cosmetic and doesn't affect functionality.
Also applies to: 597-601, 609-619
frontend/__tests__/unit/pages/Home.test.tsx (1)
215-218: LGTM! Multiple test iterations correctly refactored.All forEach → for...of conversions maintain proper test coverage for upcoming events, pull requests, milestones, and stats. The refactor preserves all assertions and improves code clarity.
Minor style observation: Blank lines are added inconsistently before some loops (lines 227, 246, 276) but not others (line 215). For consistency across the test suite, consider applying a uniform spacing pattern—though this is purely cosmetic.
Also applies to: 228-231, 240-245, 277-284
frontend/src/utils/helpers/githubHeatmap.ts (1)
318-346: Simplify with.entries()and remove commented code.The manual index tracking works but is less idiomatic. Using
.entries()provides cleaner intent. Additionally, the commented-out code (lines 335-346) should be removed as it serves no purpose.Apply this diff:
- let index = 0 - -for (const year of data.years) { - - const offsetY = yearHeight * index + canvasMargin + headerOffset + 10 + for (const [index, year] of data.years.entries()) { + const offsetY = yearHeight * index + canvasMargin + headerOffset + 10 const offsetX = canvasMargin drawYear(ctx, { ...opts, year, offsetX, offsetY, data, themeName, }) - - index++ -} - // data.years.forEach((year, i) => { - // const offsetY = yearHeight * i + canvasMargin + headerOffset + 10 - // const offsetX = canvasMargin - // drawYear(ctx, { - // ...opts, - // year, - // offsetX, - // offsetY, - // data, - // themeName, - // }) - // }) + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
frontend/__tests__/unit/components/AnchorTitle.test.tsx(1 hunks)frontend/__tests__/unit/components/Badges.test.tsx(1 hunks)frontend/__tests__/unit/components/BarChart.test.tsx(1 hunks)frontend/__tests__/unit/components/CardDetailsPage.test.tsx(2 hunks)frontend/__tests__/unit/components/DisplayIcon.test.tsx(1 hunks)frontend/__tests__/unit/components/DonutBarChart.test.tsx(2 hunks)frontend/__tests__/unit/components/Footer.test.tsx(5 hunks)frontend/__tests__/unit/components/LeadersList.test.tsx(1 hunks)frontend/__tests__/unit/components/LogoCarousel.test.tsx(3 hunks)frontend/__tests__/unit/components/MetricsCard.test.tsx(1 hunks)frontend/__tests__/unit/components/Modal.test.tsx(1 hunks)frontend/__tests__/unit/components/ModuleList.test.tsx(1 hunks)frontend/__tests__/unit/components/MultiSearch.test.tsx(1 hunks)frontend/__tests__/unit/components/Pagination.test.tsx(2 hunks)frontend/__tests__/unit/components/ProjectTypeDashboardCard.test.tsx(3 hunks)frontend/__tests__/unit/components/ProjectsDashboardDropDown.test.tsx(1 hunks)frontend/__tests__/unit/components/SkeletonBase.test.tsx(7 hunks)frontend/__tests__/unit/components/ToggleableList.test.tsx(2 hunks)frontend/__tests__/unit/components/TopContributorsList.test.tsx(3 hunks)frontend/__tests__/unit/pages/Chapters.test.tsx(1 hunks)frontend/__tests__/unit/pages/Header.test.tsx(2 hunks)frontend/__tests__/unit/pages/Home.test.tsx(4 hunks)frontend/__tests__/unit/pages/OrganizationDetails.test.tsx(1 hunks)frontend/__tests__/unit/pages/ProjectDetails.test.tsx(2 hunks)frontend/__tests__/unit/pages/ProjectHealthDashboardMetricsDetails.test.tsx(1 hunks)frontend/__tests__/unit/pages/Projects.test.tsx(1 hunks)frontend/__tests__/unit/pages/ProjectsHealthDashboardMetrics.test.tsx(4 hunks)frontend/__tests__/unit/pages/RepositoryDetails.test.tsx(2 hunks)frontend/__tests__/unit/pages/UserDetails.test.tsx(2 hunks)frontend/__tests__/unit/utils/middleware.test.ts(1 hunks)frontend/src/utils/helpers/githubHeatmap.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
frontend/__tests__/unit/components/LogoCarousel.test.tsx (1)
frontend/src/components/LogoCarousel.tsx (1)
MovingLogos(10-85)
frontend/__tests__/unit/utils/middleware.test.ts (1)
frontend/src/middleware.ts (1)
authenticationMiddleware(5-12)
frontend/__tests__/unit/components/DisplayIcon.test.tsx (2)
frontend/src/types/icon.ts (1)
Icon(1-3)frontend/src/components/DisplayIcon.tsx (1)
DisplayIcon(7-60)
frontend/__tests__/unit/pages/Home.test.tsx (1)
frontend/__tests__/unit/data/mockHomeData.ts (1)
mockGraphQLData(1-158)
🔇 Additional comments (38)
frontend/__tests__/unit/components/Pagination.test.tsx (1)
85-87: Consistent switch to for...of: looks goodEquivalent behavior, clearer loops. No risk of callback scoping issues.
Also applies to: 94-96, 125-127
frontend/__tests__/unit/components/LeadersList.test.tsx (1)
271-274: Loop refactor is correctfor...of over NodeList array preserves order and keeps assertions simple.
frontend/__tests__/unit/components/Modal.test.tsx (1)
213-215: LGTM! Clean forEach → for...of conversion.The refactor correctly replaces the forEach iteration with a for...of loop while preserving the assertion logic. The change aligns with TypeScript best practices and improves type narrowing within the loop scope.
frontend/__tests__/unit/components/AnchorTitle.test.tsx (1)
142-148: LGTM! Correct forEach → for...of conversion.The refactor properly converts the forEach loop to for...of while maintaining the test logic—rendering each title variant, asserting the href format, and cleaning up with unmount. The change improves readability and type safety.
frontend/__tests__/unit/pages/Header.test.tsx (2)
251-255: LGTM! Correct forEach → for...of conversion.The loop correctly iterates over logoImages and validates attributes. The conversion preserves the original test behavior while improving type narrowing.
278-280: LGTM! Correct forEach → for...of conversion.The loop correctly iterates over homeLinks and validates the href attribute. Consistent with the other conversion in this file.
frontend/__tests__/unit/components/MultiSearch.test.tsx (1)
320-323: LGTM! Correct forEach → for...of conversion.The refactor properly converts the forEach loop within the waitFor block. The additional blank line improves readability by separating the loop from the preceding code.
frontend/__tests__/unit/pages/ProjectDetails.test.tsx (2)
148-151: LGTM! Correct forEach → for...of conversion.The loop correctly iterates over issues and validates both title and repositoryName. The conversion maintains test behavior while improving type safety.
255-260: LGTM! Correct forEach → for...of conversion.The loop correctly iterates over milestones with multiple assertions per iteration. The conversion is consistent with the earlier change in this file and maintains all test expectations.
frontend/__tests__/unit/components/SkeletonBase.test.tsx (3)
129-133: LGTM! Consistent forEach → for...of conversions.All four conversions in the "Prop-based Behaviour" describe block correctly iterate over cardSkeletons arrays and validate attributes. The changes maintain test behavior while improving type safety and readability.
Also applies to: 140-146, 153-158, 165-170
241-248: LGTM! Correct forEach → for...of conversions with cleanup.These loops correctly iterate over test cases (unhandledValues, specialNames, skeletonTypes) while performing proper cleanup with container.remove(). The for...of syntax improves clarity, especially with the additional blank lines providing better visual separation.
Also applies to: 326-331, 427-434
264-267: LGTM! Clean forEach → for...of conversions.These three conversions consistently iterate over cardSkeletons arrays with simple attribute assertions. All maintain the original test behavior while following TypeScript best practices.
Also applies to: 381-383, 453-456
frontend/__tests__/unit/components/ProjectsDashboardDropDown.test.tsx (1)
550-553: LGTM! Correct forEach → for...of conversion.The loop correctly iterates over dropdown items and validates accessibility attributes. The conversion maintains test coverage while improving type narrowing for the item elements.
frontend/__tests__/unit/components/DonutBarChart.test.tsx (1)
306-318: LGTM! Correct forEach → for...of conversions.Both loops correctly iterate over test case arrays (iconTypes and titles) with proper component rendering, assertions, and cleanup. The additional blank lines improve readability by clearly delineating the loop structure from surrounding code. The conversions maintain all test behavior while following TypeScript best practices.
Also applies to: 330-337
frontend/__tests__/unit/components/CardDetailsPage.test.tsx (1)
749-752: LGTM! Clean forEach → for...of conversion.The refactor correctly replaces
forEachwithfor...ofloops when validating link attributes. Test assertions remain identical and the explicit loop syntax improves readability.Also applies to: 1033-1036
frontend/__tests__/unit/components/LogoCarousel.test.tsx (1)
409-412: LGTM! Accessibility and DOM structure tests correctly refactored.All four forEach → for...of conversions maintain proper test coverage for link attributes, semantic structure, image styles, and container classes. The refactor aligns with the PR's goal of standardizing iteration patterns.
Also applies to: 419-422, 457-460, 517-519
frontend/__tests__/unit/components/DisplayIcon.test.tsx (1)
262-267: LGTM! Test case iteration correctly refactored.The forEach → for...of conversion properly updates variable access from destructuring to
testCase.itemandtestCase.value. The test assertions for flip-container classes remain identical.frontend/__tests__/unit/pages/RepositoryDetails.test.tsx (1)
141-144: LGTM! Data rendering tests correctly refactored.Both forEach → for...of conversions properly test the rendering of issues and milestones. The explicit loop syntax clearly shows the iteration over test data while preserving all assertions.
Also applies to: 190-195
frontend/__tests__/unit/pages/ProjectHealthDashboardMetricsDetails.test.tsx (1)
92-94: LGTM! Clean header validation refactor.The forEach → for...of conversion correctly maintains the test logic for verifying metrics headers are rendered. Simple and straightforward change.
frontend/__tests__/unit/components/Badges.test.tsx (1)
103-111: LGTM! Backend icon tests correctly refactored.The forEach → for...of conversion properly updates the test to use
backendIcon.cssClassandbackendIcon.expectedIconthroughout. The test coverage for icon transformations (including snake_case to camelCase) remains intact.frontend/__tests__/unit/components/ToggleableList.test.tsx (2)
45-52: LGTM!The for...of loop conversions correctly iterate over the array items without needing indices.
146-148: LGTM!The for...of loop conversion is correct for this simple iteration.
frontend/__tests__/unit/components/MetricsCard.test.tsx (1)
68-73: LGTM!The destructuring correctly unpacks tuples from the
casesarray.frontend/__tests__/unit/pages/ProjectsHealthDashboardMetrics.test.tsx (3)
112-115: LGTM!The for...of loop correctly iterates over the headers array.
133-149: LGTM!All three for...of loops correctly iterate over their respective arrays without needing indices.
158-173: LGTM!The for...of loop correctly iterates over the metrics array.
frontend/__tests__/unit/utils/middleware.test.ts (1)
39-56: LGTM!The for...of loop correctly iterates over test cases and accesses properties appropriately.
frontend/__tests__/unit/components/Footer.test.tsx (8)
133-135: LGTM!The for...of loop correctly iterates over footer sections.
153-162: LGTM!The nested for...of loops correctly iterate over sections and their links.
168-177: LGTM!The for...of loop correctly iterates over footer icons.
286-289: LGTM!The for...of loop correctly iterates over footer sections.
297-300: LGTM!The for...of loop correctly iterates over footer sections.
305-308: LGTM!The for...of loop correctly iterates over footer icons.
339-342: LGTM!The for...of loop correctly iterates over buttons.
385-387: LGTM!The for...of loop correctly iterates over buttons.
frontend/__tests__/unit/components/ProjectTypeDashboardCard.test.tsx (3)
212-218: LGTM: for-of conversion reads clearly and keeps per-iteration cleanup.The loop is correct; unmount within each iteration is appropriate to isolate renders.
369-371: LGTM: parameterized type validation with for-of.Straightforward replacement; preserves behavior.
376-383: LGTM: icon iteration refactor.Loop body and unmount placement look good.
frontend/__tests__/unit/components/ProjectTypeDashboardCard.test.tsx
Outdated
Show resolved
Hide resolved
|
@anirudhprmar thanks for working on this! |
sure I'll make the updates and resolve those comments |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/__tests__/unit/components/Footer.test.tsx (2)
168-177: Consider removing trailing whitespace.The for...of conversion is correct. However, Line 174 contains trailing whitespace after the closing brace.
expect(link).toHaveAttribute('rel', 'noopener noreferrer') - + const iconElement = link.querySelector('[data-testid="font-awesome-icon"]')
274-280: Past critical issue resolved; minor formatting polish suggested.The previous runtime error has been correctly fixed by using
buttons.entries()to access both index and element. The test logic is now sound.Consider these optional formatting improvements for consistency:
const buttons = screen.getAllByRole('button') - - for (const [index,button] of buttons.entries()) { + for (const [index, button] of buttons.entries()) { const sectionTitle = mockFooterSections[index].title expect(button).toHaveAttribute('aria-controls', `footer-section-${sectionTitle}`) expect(button).toHaveAttribute('aria-expanded') - }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
frontend/__tests__/unit/components/BarChart.test.tsx(1 hunks)frontend/__tests__/unit/components/Footer.test.tsx(5 hunks)frontend/__tests__/unit/components/ProjectTypeDashboardCard.test.tsx(3 hunks)frontend/__tests__/unit/pages/Chapters.test.tsx(1 hunks)frontend/__tests__/unit/pages/Projects.test.tsx(1 hunks)frontend/src/utils/helpers/githubHeatmap.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/tests/unit/pages/Projects.test.tsx
- frontend/src/utils/helpers/githubHeatmap.ts
🔇 Additional comments (8)
frontend/__tests__/unit/components/BarChart.test.tsx (1)
237-246: LGTM! Previous critical issue resolved.The fix correctly implements the suggested change from the previous review. The loop now properly uses
.entries()to destructure both the index and dataPoint, and all property accesses (dataPoint.x,dataPoint.y,dataPoint.goals) are correct.frontend/__tests__/unit/pages/Chapters.test.tsx (1)
142-145: LGTM! Previous critical issue has been resolved.The for...of loop now correctly uses
.entries()and destructures as[index, link]in the proper order. This fix addresses the previous review comment—the code will no longer throw and correctly binds the index for route construction.frontend/__tests__/unit/components/Footer.test.tsx (2)
133-135: LGTM: Clean forEach → for...of conversions.These straightforward array iterations correctly replace forEach with for...of syntax while preserving the original test logic.
Also applies to: 153-158, 160-162
286-289: LGTM: All remaining forEach → for...of conversions are correct.The conversions across the Accessibility, CSS Classes, and Component Integration test sections properly replace forEach with for...of while maintaining test behavior.
Also applies to: 297-300, 305-308, 339-342, 385-387
frontend/__tests__/unit/components/ProjectTypeDashboardCard.test.tsx (4)
212-218: LGTM! Clean forEach → for...of conversion.The conversion correctly iterates over the types array and maintains the unmount cleanup inside the loop.
369-371: LGTM! Correct forEach → for...of conversion.The explicit loop syntax improves readability and aligns with TypeScript iteration best practices.
376-383: LGTM! Correct conversion with proper cleanup.The forEach → for...of conversion is correct, and the unmount cleanup is properly placed inside the loop. The blank line at 376 improves readability.
407-410: LGTM! Critical issue from previous review has been resolved.The code now correctly uses
.entries()and destructures in the proper order[index, type]. This ensurescountreceives the numeric index andtypereceives the string value, fixing the bug flagged in the previous review.
arkid15r
left a comment
arkid15r
left a comment
There was a problem hiding this comment.
Please make sure all checks are green -- https://github.com/OWASP/Nest/actions/runs/18829948852/job/53824516875?pr=2489
…dhprmar/Nest into pr/anirudhprmar/2489
| render(<ModuleList modules={modules} />) | ||
|
|
||
| modules.forEach((module) => { | ||
| for (const module of modules) { |
There was a problem hiding this comment.
Not the best idea to name a variable module :)
https://nextjs.org/docs/messages/no-assign-module-variable
Pushed changes to update this and address other issues!
There was a problem hiding this comment.
okay, module is a reserved keyword which can't be used explicitly anywhere.
|
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
@anirudhprmar Thank you for working on this! It's a big PR and a lot of changes 👍🏼
One point for any future PRs in OWASP Nest - please run make check-test locally before pushing changes and address found issues.
There were a lot of missed issues in this one, super small ones and they would have been all "fixed" automatically.
Thanks for assigning me this issue, even though i wasn't assigned. Yes, definitely I'll make sure to run "make check-test" locally before pushing changes and will fix those issues from my end making sure everything works properly. |
3 participants
Refactor: Replace all forEach with for...of loops
Resolves #2484
Replaced all instances of
forEachacross the codebase withfor...ofloopsto improve readability, performance, and consistency with the TypeScript
iteration guidelines. See the full SonarCloud issue list.
Checklist
make check-testlocally; all checks and tests passed.