Skip to content

fix: prevent LogoCarousel DOM cloning leak#3801

Merged
arkid15r merged 4 commits intoOWASP:mainfrom
preeeetham:fix/3754-logo-carousel-memory-leak
Feb 7, 2026
Merged

fix: prevent LogoCarousel DOM cloning leak#3801
arkid15r merged 4 commits intoOWASP:mainfrom
preeeetham:fix/3754-logo-carousel-memory-leak

Conversation

@preeeetham
Copy link
Contributor

Summary

  • Fixes memory leak in LogoCarousel by removing DOM cloning in useEffect and rendering the duplicated sponsor list declaratively.
  • Ensures stable unique keys even when sponsors contains repeated items.

Fixes #3754.

Test plan

  • make check-test-frontend (format + lint passed; graphql-codegen requires backend running locally)
  • pnpm exec jest __tests__/unit/components/LogoCarousel.test.tsx --coverage=false (pass)

Made with Cursor

Replace DOM cloning in LogoCarousel with declarative rendering to avoid node accumulation on re-renders.

Co-authored-by: Cursor <cursoragent@cursor.com>
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 6, 2026

Summary by CodeRabbit

  • Refactor
    • Optimized logo carousel component rendering and animation system. Animation duration now dynamically adjusts based on sponsor count, sponsor items render more reliably as data changes, and image handling is more robust when URLs are missing.

Walkthrough

Replaces imperative DOM cloning in the LogoCarousel with declarative JSX rendering: adds a renderSponsorCard helper, computes a stable animation duration, and renders sponsors twice (original + loop) to simulate continuous scrolling without DOM mutation or cleanup logic. No public API changes.

Changes

Cohort / File(s) Summary
LogoCarousel refactor
frontend/src/components/LogoCarousel.tsx
Removed useRef/useEffect DOM cloning and cleanup concerns; added renderSponsorCard helper, stable animation duration calculation (max(sponsors.length, 1) * 2), and duplicated render pass (original + loop keys) to achieve infinite scroll declaratively. Improved key generation and safe image/link handling.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • kasya
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main fix: preventing a DOM cloning memory leak in LogoCarousel.
Description check ✅ Passed The description is directly related to the changeset, explaining the memory leak fix and the declarative rendering approach used.
Linked Issues check ✅ Passed The PR successfully addresses all coding objectives from issue #3754: removes DOM cloning, implements declarative rendering in JSX, ensures stable unique keys, and preserves infinite-scroll behavior.
Out of Scope Changes check ✅ Passed All changes are scoped to fixing the LogoCarousel memory leak issue and are directly related to the linked issue #3754 requirements.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

cubic-dev-ai[bot]
cubic-dev-ai bot previously approved these changes Feb 6, 2026
Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 1 file

Confidence score: 5/5

  • Automated review surfaced no issues in the provided summaries.
  • No files require special attention.

coderabbitai[bot]
coderabbitai bot previously approved these changes Feb 6, 2026
@sonarqubecloud
Copy link

sonarqubecloud bot commented Feb 7, 2026

@arkid15r arkid15r enabled auto-merge February 7, 2026 00:59
@codecov
Copy link

codecov bot commented Feb 7, 2026

Codecov Report

❌ Patch coverage is 80.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.24%. Comparing base (fe8eff8) to head (e66fdbe).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
frontend/src/components/LogoCarousel.tsx 80.00% 0 Missing and 2 partials ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3801      +/-   ##
==========================================
- Coverage   90.25%   90.24%   -0.01%     
==========================================
  Files         463      463              
  Lines       14412    14410       -2     
  Branches     1934     1936       +2     
==========================================
- Hits        13008    13005       -3     
  Misses        987      987              
- Partials      417      418       +1     
Flag Coverage Δ
backend 90.97% <ø> (ø)
frontend 88.22% <80.00%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
frontend/src/components/LogoCarousel.tsx 85.71% <80.00%> (-8.04%) ⬇️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fe8eff8...e66fdbe. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@arkid15r arkid15r added this pull request to the merge queue Feb 7, 2026
Merged via the queue into OWASP:main with commit 0976876 Feb 7, 2026
33 of 35 checks passed
@preeeetham preeeetham deleted the fix/3754-logo-carousel-memory-leak branch February 8, 2026 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Memory Leak: LogoCarousel clones DOM nodes without cleanup

2 participants