⚡ Bolt: Optimize BackgroundCarousel image rendering#61
⚡ Bolt: Optimize BackgroundCarousel image rendering#61
Conversation
- Implement conditional rendering for images in BackgroundCarousel to reduce DOM nodes and memory usage. - Only render active, next, previous, and loop boundary slides. - Ensure loop transitions are smooth by always rendering the first 2 and last 2 slides. - Add activeIndex state tracking via Swiper's onSlideChange event. Co-authored-by: anyulled <100741+anyulled@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Summary of ChangesHello @anyulled, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the performance of the 'BackgroundCarousel' component by optimizing how background images are loaded and rendered. By intelligently determining which images are necessary for immediate display or smooth transitions, it drastically cuts down on resource consumption without compromising the visual integrity of the carousel. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively optimizes the BackgroundCarousel by conditionally rendering images, which will reduce memory usage and the number of DOM nodes. The logic to determine which slides to render is sound, correctly accounting for the active, next, and previous slides, as well as buffer slides at the start and end to ensure smooth looping. I have one suggestion to refactor the rendering logic slightly for better performance and readability.
| {IMAGES.map((image, index) => { | ||
| const isActive = index === activeIndex; | ||
| const isNext = index === (activeIndex + 1) % IMAGES.length; | ||
| const isPrev = index === (activeIndex - 1 + IMAGES.length) % IMAGES.length; | ||
|
|
||
| /* | ||
| * Always render the first 2 and last 2 slides to ensure loop duplicates have content | ||
| * This prevents flickering during loop transitions as Swiper clones these slides | ||
| */ | ||
| const isStart = index < 2; | ||
| const isEnd = index >= IMAGES.length - 2; | ||
|
|
||
| const shouldRender = isActive || isNext || isPrev || isStart || isEnd; | ||
|
|
||
| return ( | ||
| <SwiperSlide key={image}> | ||
| <div className={`background-carousel__slide ${prefersReducedMotion ? "" : "ken-burns"}`}> | ||
| {shouldRender && ( | ||
| <Image | ||
| src={image} | ||
| alt="Conference venue background" | ||
| fill | ||
| priority={index === 0} | ||
| sizes="100vw" | ||
| style={{ objectFit: "cover" }} | ||
| quality={85} | ||
| /> | ||
| )} | ||
| </div> | ||
| </SwiperSlide> | ||
| ); | ||
| })} |
There was a problem hiding this comment.
This is a great optimization! To further improve performance and readability, you could calculate the set of indices to render once before mapping over the images. This avoids redundant calculations inside the loop for every image.
Using an Immediately Invoked Function Expression (IIFE) allows you to contain this logic cleanly within the JSX, improving both performance and separation of concerns.
{(() => {
const imageCount = IMAGES.length;
/*
* To prevent flicker with loop, we need to render more than just the active slide.
* This set includes:
* - The active, previous, and next slides for smooth transitions.
* - The first 2 and last 2 slides, which Swiper clones for the loop effect.
*/
const indicesToRender = new Set([
activeIndex,
(activeIndex + 1) % imageCount,
(activeIndex - 1 + imageCount) % imageCount,
0,
1,
imageCount - 2,
imageCount - 1,
]);
return IMAGES.map((image, index) => (
<SwiperSlide key={image}>
<div className={`background-carousel__slide ${prefersReducedMotion ? "" : "ken-burns"}`}>
{indicesToRender.has(index) && (
<Image
src={image}
alt="Conference venue background"
fill
priority={index === 0}
sizes="100vw"
style={{ objectFit: "cover" }}
quality={85}
/>
)}
</div>
</SwiperSlide>
));
})()}
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughBackgroundCarousel now tracks the active slide and conditionally renders slide images (active, adjacent, and first/last two slides) to reduce DOM updates and avoid flicker from loop clones; the Ken Burns effect and image attributes are preserved. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/BackgroundCarousel.tsx (1)
1-1:⚠️ Potential issue | 🟡 MinorFix Prettier formatting issues.
The CI pipeline is failing due to Prettier formatting violations. Run
npm run format:checklocally to identify the issues, thenprettier --writeto fix them.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/BackgroundCarousel.tsx` at line 1, Prettier formatting in BackgroundCarousel.tsx is failing CI; run the formatter and commit the fixes: run `npm run format:check` to see violations, then run `prettier --write` (or `npm run format`) against the project (or specifically BackgroundCarousel.tsx) to fix the formatting, verify the file (BackgroundCarousel, "use client" directive and surrounding JSX) is now Prettier-compliant, and commit the updated file so CI passes.
🧹 Nitpick comments (1)
components/BackgroundCarousel.tsx (1)
38-38: Consider synchronizing initial state with Swiper's actual starting index.The
activeIndexis initialized to0, which works for the default Swiper configuration. However, ifinitialSlideis ever configured differently in the future, this could cause a brief mismatch where the wrong slides are rendered on mount.A more defensive approach would be to initialize from the Swiper instance:
♻️ Optional: Initialize from Swiper instance
- onSwiper={() => {}} + onSwiper={(swiper) => setActiveIndex(swiper.realIndex)}This ensures the initial state always matches Swiper's actual starting position, even if configuration changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/BackgroundCarousel.tsx` at line 38, Initialize activeIndex from the actual Swiper instance instead of hardcoding 0: replace the current useState(0) for activeIndex with a state that is set when the Swiper instance is available (use the Swiper onSwiper callback or an existing swiperRef) and call setActiveIndex(swiper.activeIndex) there (also keep the existing onSlideChange handler updating setActiveIndex). This ensures activeIndex (and setActiveIndex) reflect Swiper's real initialSlide value on mount.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@components/BackgroundCarousel.tsx`:
- Line 1: Prettier formatting in BackgroundCarousel.tsx is failing CI; run the
formatter and commit the fixes: run `npm run format:check` to see violations,
then run `prettier --write` (or `npm run format`) against the project (or
specifically BackgroundCarousel.tsx) to fix the formatting, verify the file
(BackgroundCarousel, "use client" directive and surrounding JSX) is now
Prettier-compliant, and commit the updated file so CI passes.
---
Nitpick comments:
In `@components/BackgroundCarousel.tsx`:
- Line 38: Initialize activeIndex from the actual Swiper instance instead of
hardcoding 0: replace the current useState(0) for activeIndex with a state that
is set when the Swiper instance is available (use the Swiper onSwiper callback
or an existing swiperRef) and call setActiveIndex(swiper.activeIndex) there
(also keep the existing onSlideChange handler updating setActiveIndex). This
ensures activeIndex (and setActiveIndex) reflect Swiper's real initialSlide
value on mount.
- Fix Prettier formatting in `components/BackgroundCarousel.tsx`. - Update `cypress/e2e/home/home-editions.cy.ts` to assert on `.hero8-header__event-line` instead of `h5`, matching the actual markup. Co-authored-by: anyulled <100741+anyulled@users.noreply.github.com>
1 participant
💡 What: Optimized
components/BackgroundCarousel.tsxto conditionally rendernext/imagecomponents inside Swiper slides.🎯 Why: The carousel was rendering all 15 large background images at once, which increased memory usage and DOM node count.
📊 Impact: Reduces the number of simultaneously rendered images from 15 (plus duplicates) to ~7-9, significantly reducing memory footprint and DOM complexity.
🔬 Measurement: Verified with a Playwright script that the number of images in the DOM is reduced while maintaining visual correctness during loop transitions. Tests passed.
PR created automatically by Jules for task 12419383181279936805 started by @anyulled
Summary by CodeRabbit
Bug Fixes
Tests