Skip to content

Conversation

@GabLeRoux
Copy link
Member

@GabLeRoux GabLeRoux commented Dec 19, 2025

Changes

Fix OpenCollective sponsors section display issues

Problem:

  • Sponsors section displayed at fixed width instead of 100%
  • Height used hardcoded breakpoint values that didn't adapt to content changes
  • Visible scrollbar on iframe

Solution:

  • Implemented dynamic height using OpenCollective's postMessage API
  • Added responsive width with proper overflow handling
  • Disabled iframe scrolling with scrolling="no" attribute

Results:

✅ Full-width display on all screen sizes
✅ Dynamic height that adapts to sponsor count changes
✅ No scrollbars or overflow issues
✅ Future-proof - no manual updates needed when sponsors change

The iframe now automatically resizes based on actual content height (1136px desktop, 1995px mobile) instead of using hardcoded values.

Checklist

  • Read the contribution guide and accept the
    code of conduct
  • Readme (updated or not needed)
  • Tests (added, updated or not needed)

Summary by CodeRabbit

  • Bug Fixes
    • Sponsors section now displays responsively with dynamic height adjustment and improved layout for optimal viewing across different screen sizes.

✏️ Tip: You can customize this high-level summary in your review settings.

- Add `className="w-full max-w-7xl"` to FadeIntoView wrapper for proper width constraints
- Replace fixed `height` and `width` attributes with responsive Tailwind classes
- Implement breakpoint-based height scaling: 1200px (mobile), 1000px (sm), 900px (md), 750px (lg), 667px (xl)
- Set iframe to full width for better mobile experience
- Improves layout consistency across different screen sizes and devices
…tMessage

- Add useEffect hook to listen for postMessage events from OpenCollective
- Implement dynamic height state that updates based on iframe content changes
- Parse OpenCollective's "oc-" prefixed message format to extract height data
- Add 10px buffer to calculated height to prevent scrollbars
- Replace responsive Tailwind height classes with dynamic inline styles
- Wrap iframe in container div with overflow-hidden for better containment
- Add comprehensive JSDoc comment explaining iframe behavior and fallback mechanism
- Validate message origin to only accept events from https://opencollective.com
- Add error handling for JSON parsing failures with silent fallback
- Improve accessibility and user experience by eliminating layout shifts
@coderabbitai
Copy link

coderabbitai bot commented Dec 19, 2025

Walkthrough

The SponsorsSection component now dynamically adjusts the OpenCollective iframe height based on postMessage events from the embedded frame. A useState hook manages height with a 1200px default, and a useEffect listens for validated messages with an origin check, parsing payloads prefixed with "oc-", and applying the new height with a buffer offset.

Changes

Cohort / File(s) Change Summary
Dynamic iframe height handling
src/components/pages/home/section/sponsors-section.tsx
Added height state management and useEffect hook to listen for postMessage events from OpenCollective. Validates message origin, parses "oc-" prefixed payloads, and updates iframe height dynamically with buffer. Wraps iframe in responsive container with FadeIntoView, applies ref and inline styling, and includes proper cleanup on unmount.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~18 minutes

Areas requiring extra attention:

  • Origin validation logic in the postMessage handler—ensure security assumptions are correct and origin checking prevents abuse
  • Payload parsing and error handling—verify silent failures won't mask integration issues
  • Cleanup of the message listener on component unmount—confirm no memory leaks
  • Height buffer logic and default fallback behavior—test responsiveness across viewport sizes

Poem

🐰 A rabbit whispers through the window pane,
"Heights shall flow like morning rain!"
When Collective speaks, we gently hear,
And stretch our frame without a fear.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Fix home page sponsors section size' directly summarizes the main change: resolving display sizing issues in the sponsors section component.
Description check ✅ Passed The description comprehensively covers the problem, solution, and results, with all required checklist items completed. It aligns well with the description template structure.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix-home-page-sponsors-section-size

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.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (2)
src/components/pages/home/section/sponsors-section.tsx (2)

13-13: Consider removing the unused ref.

The iframeRef is created and assigned to the iframe but never accessed (e.g., iframeRef.current is not used anywhere). Unless you're planning to use it in the future, it can be removed.

🔎 Proposed change to remove unused ref
-  const iframeRef = useRef<HTMLIFrameElement>(null);

And update line 57:

-            ref={iframeRef}

34-36: Optional: Extract the buffer value to a named constant.

The 10px buffer is a magic number. While the inline comment explains its purpose, extracting it to a named constant would improve readability.

🔎 Suggested refactor

At the top of the component:

 const SponsorsSection = () => {
+  const HEIGHT_BUFFER = 10; // Buffer to prevent scrollbars
   const iframeRef = useRef<HTMLIFrameElement>(null);

Then update line 36:

-          setIframeHeight(Math.ceil(height) + 10);
+          setIframeHeight(Math.ceil(height) + HEIGHT_BUFFER);
📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1e49e41 and 9bac9cd.

📒 Files selected for processing (1)
  • src/components/pages/home/section/sponsors-section.tsx (1 hunks)
🔇 Additional comments (4)
src/components/pages/home/section/sponsors-section.tsx (4)

1-1: LGTM! Hook imports are appropriate.

The imports of useEffect, useRef, and useState are necessary for the dynamic height functionality and fully compatible with React 17.0.2.


5-11: LGTM! Clear and helpful documentation.

The JSDoc comment effectively explains the dynamic resize mechanism and fallback behavior, which will help future maintainers understand the postMessage integration.


54-65: LGTM! Clean and accessible iframe implementation.

The responsive width classes, overflow handling, dynamic height styling, and accessibility attributes (title and scrolling="no") are all correctly implemented. The wrapping structure properly manages the layout.


17-49: LGTM! Well-implemented message handler with proper security.

The origin validation, cleanup logic, and empty dependency array are all correct. The message parsing and height extraction logic appear sound.

Note: The specific postMessage format from OpenCollective (including the "oc-" prefix and height/frameHeight properties) should be confirmed against their actual implementation, as this is not publicly documented in their official API documentation.

@github-actions
Copy link

Visit the preview URL for this PR (updated for commit 9bac9cd):

https://game-ci-5559f--pr526-fix-home-page-sponso-en32fmku.web.app

(expires Fri, 26 Dec 2025 16:07:25 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 1f0574f15f83e11bfc148eae8646486a6d0e078b

@github-actions
Copy link

Cat Gif

@webbertakken
Copy link
Member

We need to also upgrade React version because there was a CVE recently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants