Conversation
- Conditionally render Next/Image components in BackgroundCarousel based on active index (render only active, next, prev). - Scope `ken-burns` CSS animation to active/prev slides to prevent premature execution on preloaded slides. - Reduces initial DOM node count and image loading from ~15 to 3. References: #perf-opt 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 introduces significant performance enhancements to the 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
|
📝 WalkthroughWalkthroughBackgroundCarousel now tracks the active slide and conditionally renders only the active, previous, and next slides; ken-burns animation and related styles are applied only when slide content is actually rendered. A Cypress selector in the home editions test was tightened. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
![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
The pull request effectively optimizes the BackgroundCarousel component by reducing the number of rendered Next/Image components and scoping the ken-burns animation to active slides. This significantly improves performance by reducing DOM nodes, network requests, and CPU/GPU usage. The changes are well-implemented and address the identified performance bottlenecks.
| }} | ||
| loop={true} | ||
| onSwiper={() => {}} | ||
| onSlideChange={(swiper) => setActiveIndex(swiper.realIndex)} |
There was a problem hiding this comment.
The onSwiper prop is currently an empty function. While it might be a placeholder, it's good practice to remove unused event handlers or provide a comment explaining its purpose if it's intentionally left empty for future use. An empty function still creates a new function on every render, which can have a minor performance impact.
| onSlideChange={(swiper) => setActiveIndex(swiper.realIndex)} | |
| onSwiper={(swiper) => { /* TODO: Add swiper initialization logic if needed */ }} |
| </div> | ||
| {shouldRenderSlide(index) ? ( | ||
| <div className={`background-carousel__slide ${prefersReducedMotion ? "" : "ken-burns"}`}> | ||
| <Image src={image} alt="Conference venue background" fill priority={index === 0} sizes="100vw" style={{ objectFit: "cover" }} quality={85} /> |
There was a problem hiding this comment.
The priority prop for Next/Image is set to index === 0. While this is correct for the initial load, as slides change, the priority image might not be the currently active one. This could lead to less optimal loading of subsequent active images. Consider dynamically setting priority based on activeIndex to ensure the currently visible image always has priority.
| <Image src={image} alt="Conference venue background" fill priority={index === 0} sizes="100vw" style={{ objectFit: "cover" }} quality={85} /> | |
| <Image src={image} alt="Conference venue background" fill priority={index === activeIndex} sizes="100vw" style={{ objectFit: "cover" }} quality={85} /> |
|
|
||
| .swiper-slide-active, | ||
| .swiper-slide-duplicate-active, | ||
| .swiper-slide-prev { |
There was a problem hiding this comment.
The selector .swiper-slide-prev is included here to apply the kenBurns animation. However, the shouldRenderSlide logic in BackgroundCarousel.tsx also renders the prevIndex. This means the animation will apply to the previous slide as well. If the intention is to only animate the active slide, then .swiper-slide-prev should be removed from this selector. If the intention is to animate the previous, active, and next slides, then this is correct, but it's worth clarifying the animation behavior for these slides.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
- Implements lazy rendering in BackgroundCarousel to only mount active/prev/next slides (reduces images from 15 to 3). - Scopes expensive ken-burns CSS animation to visible slides. - Fixes Cypress test `home-editions.cy.ts` by replacing missing `h5` selector with `.hero8-header__event-line`. Co-authored-by: anyulled <100741+anyulled@users.noreply.github.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cypress/e2e/home/home-editions.cy.ts (1)
14-14: LGTM — using the scoped BEM class is more resilient than the generich5tag.The tighter selector correctly reflects the component's actual DOM structure, avoiding false matches from any other
h5headings that may or may not exist on the page. With lazy rendering now capping rendered slides at 3 (active + prev + next), theat.least: 2threshold is still satisfied on initial mount since all three slots are populated synchronously.One minor note: the inner
cy.get(".hero8-header__event-line")inherits the global Cypress default assertion timeout (4 s) rather than the explicit 30 s set on the parent.hero8-headerselector. If the event-line elements are ever part of a lazily-hydrated region, that mismatch could cause intermittent flakiness. Consider propagating an explicit timeout:🔧 Optional: explicit timeout on inner assertion
- cy.get(".hero8-header__event-line").should("have.length.at.least", 2); + cy.get(".hero8-header__event-line", { timeout: 30000 }).should("have.length.at.least", 2);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cypress/e2e/home/home-editions.cy.ts` at line 14, The inner selector cy.get(".hero8-header__event-line") uses the global default timeout (4s) and may be flaky if the region is lazily hydrated; update the assertion to pass an explicit timeout (30_000 ms) matching the parent ".hero8-header" call so the inner presence check uses the same 30s window (locate the cy.get(".hero8-header__event-line") line and add the timeout option to the get/assert invocation).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cypress/e2e/home/home-editions.cy.ts`:
- Line 14: The inner selector cy.get(".hero8-header__event-line") uses the
global default timeout (4s) and may be flaky if the region is lazily hydrated;
update the assertion to pass an explicit timeout (30_000 ms) matching the parent
".hero8-header" call so the inner presence check uses the same 30s window
(locate the cy.get(".hero8-header__event-line") line and add the timeout option
to the get/assert invocation).
1 participant
💡 What: Optimized
BackgroundCarouselto only renderNext/Imagecomponents for the current, previous, and next slides, instead of rendering all 15 images at once. Also scoped the CSSken-burnsanimation to only run on active/visible slides.🎯 Why: The carousel was rendering 15 large hero images on page load, even though only one is visible. This caused unnecessary network requests, memory usage, and DOM node creation. The CSS animation was also running on hidden slides.
📊 Impact:
🔬 Measurement:
imgtags in the carousel. The count dropped from ~15 to 3.npm testoutput shows significantly fewer "unconfigured quality" warnings (proxy for image rendering count).PR created automatically by Jules for task 7937693282089218174 started by @anyulled
Summary by CodeRabbit
Refactor
Style
Tests