fix(carousel): seamless circular transition between last and first slides#1669
Open
seojcarlos wants to merge 5 commits intothemesberg:mainfrom
Open
fix(carousel): seamless circular transition between last and first slides#1669seojcarlos wants to merge 5 commits intothemesberg:mainfrom
seojcarlos wants to merge 5 commits intothemesberg:mainfrom
Conversation
…lides Replace linear scroll-through-all-slides behavior with a clone-based infinite loop technique. Prepend a clone of the last slide and append a clone of the first slide, then instantly reposition after the smooth transition completes. This gives a seamless circular carousel experience matching vanilla Flowbite behavior. Also adds an isAnimating guard to prevent rapid clicks from breaking the transition state. Fixes themesberg#1103 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: bda2185 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
@seojcarlos is attempting to deploy a commit to the Bergside Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdded clone-based circular wrap to Carousel: prepends/appends cloned slides, maps extended indices back to real indices, gates navigation with Changes
Sequence DiagramsequenceDiagram
participant User
participant Carousel as Carousel\n(State & Logic)
participant Autoplay as AutoplayTimer
participant Scroll as ScrollContainer\n(DOM/Scroll API)
User->>Carousel: Click "next" on last slide
Carousel->>Carousel: Normalize target index (modulo)\nset isAnimating = true
Carousel->>Autoplay: Pause/skip advance
Carousel->>Scroll: Smooth scroll to cloned boundary slide
Scroll-->>Carousel: scrollEnd event
Carousel->>Scroll: Instant jump (scrollBehavior: 'auto') to real slide
Carousel->>Carousel: Update activeItem, set isAnimating = false
Carousel->>Autoplay: Resume scheduling
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/ui/src/components/Carousel/Carousel.tsx (2)
152-152: Hardcoded 500ms animation duration is fragile.The timeout duration should match the CSS transition duration. Consider extracting this to a named constant or, preferably, using the
scrollendortransitionendevent for more reliable synchronization with the actual animation.+const SCROLL_ANIMATION_DURATION_MS = 500; + // ... inside navigateTo - setTimeout(onTransitionDone, 500); + setTimeout(onTransitionDone, SCROLL_ANIMATION_DURATION_MS);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Carousel/Carousel.tsx` at line 152, The Carousel currently uses a hardcoded setTimeout(…, 500) to call onTransitionDone which may drift from the actual CSS animation; update the Carousel component to remove the fixed 500ms delay and instead synchronize with the real transition by either (a) listening for the element's 'transitionend' (or 'scrollend' for scrolling-based animations) event and calling onTransitionDone when the event fires, or (b) deriving the duration from a single named constant/CSS variable used by the component and using that value rather than 500. Locate usages around setTimeout and the onTransitionDone invocation in Carousel.tsx and replace the timeout-based flow with an event listener approach (add/remove the listener properly) or a shared duration constant/CSS variable so timing stays in sync.
166-174: Initialization effect re-runs on everyitemschange.When children change dynamically, this effect will reset the scroll position to the first slide. If dynamic slide updates are a supported use case, consider guarding this with a mount-only check or handling resync more gracefully.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Carousel/Carousel.tsx` around lines 166 - 174, The effect that sets initial scroll in the Carousel component currently runs on every items change and resets the viewport; guard it so it only runs on mount or only when transitioning from empty→non-empty: add an initializedRef (or prevItemsLength ref) so in the useEffect for carouselContainer.current you only perform the auto scroll when initializedRef.current is false (or when prevItemsLength === 0 and items.length > 0), then set initializedRef.current = true; reference the existing carouselContainer ref and the items array in your check so dynamic updates to items won't re-reset the scroll position.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/ui/src/components/Carousel/Carousel.tsx`:
- Around line 140-153: The setTimeout in navigateTo that calls onTransitionDone
can leak if the component unmounts or navigateTo is re-invoked; introduce a
transitionTimeoutRef (e.g., const transitionTimeoutRef =
useRef<ReturnType<typeof setTimeout> | null>(null)) and replace raw setTimeout
with transitionTimeoutRef.current = setTimeout(...), clear any existing timeout
before setting a new one, and clear it in a useEffect cleanup so
onTransitionDone (which calls setIsAnimating(false) and manipulates
container.scrollBehavior/scrollLeft) never runs after unmount or during a newer
navigation.
---
Nitpick comments:
In `@packages/ui/src/components/Carousel/Carousel.tsx`:
- Line 152: The Carousel currently uses a hardcoded setTimeout(…, 500) to call
onTransitionDone which may drift from the actual CSS animation; update the
Carousel component to remove the fixed 500ms delay and instead synchronize with
the real transition by either (a) listening for the element's 'transitionend'
(or 'scrollend' for scrolling-based animations) event and calling
onTransitionDone when the event fires, or (b) deriving the duration from a
single named constant/CSS variable used by the component and using that value
rather than 500. Locate usages around setTimeout and the onTransitionDone
invocation in Carousel.tsx and replace the timeout-based flow with an event
listener approach (add/remove the listener properly) or a shared duration
constant/CSS variable so timing stays in sync.
- Around line 166-174: The effect that sets initial scroll in the Carousel
component currently runs on every items change and resets the viewport; guard it
so it only runs on mount or only when transitioning from empty→non-empty: add an
initializedRef (or prevItemsLength ref) so in the useEffect for
carouselContainer.current you only perform the auto scroll when
initializedRef.current is false (or when prevItemsLength === 0 and items.length
> 0), then set initializedRef.current = true; reference the existing
carouselContainer ref and the items array in your check so dynamic updates to
items won't re-reset the scroll position.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0dd2f249-27d9-4e85-b3ff-408b5dbb18e0
📒 Files selected for processing (1)
packages/ui/src/components/Carousel/Carousel.tsx
- Extract hardcoded 500ms to SCROLL_ANIMATION_DURATION_MS constant - Add transitionTimeoutRef to prevent memory leak on unmount or rapid navigation (clear pending timeout before setting a new one) - Add cleanup effect to clear timeout on component unmount - Guard initialization effect with initializedRef to prevent scroll reset on dynamic children changes
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/ui/src/components/Carousel/Carousel.tsx`:
- Around line 134-143: Tests fail because JSDOM doesn't implement
element.scrollTo; update the test setup (or top of the Carousel tests) to mock
scrollTo so calls like container.scrollTo(...) in Carousel (the logic that sets
left to container.clientWidth * (totalItems + 1) or 0) won't throw; add a
beforeAll or beforeEach that sets Element.prototype.scrollTo = vi.fn() and
optionally implement the mock to set this.scrollLeft when options.left is
provided so assertions about scroll position still work.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3c3dcda3-2a58-4b26-a1bc-98201f157a77
📒 Files selected for processing (2)
.changeset/carousel-circular-transition.mdpackages/ui/src/components/Carousel/Carousel.tsx
✅ Files skipped from review due to trivial changes (1)
- .changeset/carousel-circular-transition.md
Comment on lines
+134
to
+143
| container.scrollTo({ | ||
| left: container.clientWidth * (totalItems + 1), | ||
| behavior: "smooth", | ||
| }); | ||
| } else { | ||
| container.scrollTo({ | ||
| left: 0, | ||
| behavior: "smooth", | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
Tests fail because JSDOM lacks scrollTo implementation.
The static analysis shows multiple test failures with TypeError: container.scrollTo is not a function. JSDOM (used by Vitest/Jest) doesn't implement scrollTo on elements by default.
Add a mock in your test setup or at the top of the test file:
🧪 Proposed fix: Mock scrollTo in tests
Add to your test setup file or before tests:
beforeAll(() => {
Element.prototype.scrollTo = vi.fn();
});Or if you need to verify scroll positions:
beforeEach(() => {
Element.prototype.scrollTo = vi.fn(function(this: Element, options?: ScrollToOptions) {
if (options?.left !== undefined) {
(this as any).scrollLeft = options.left;
}
});
});Also applies to: 160-166
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ui/src/components/Carousel/Carousel.tsx` around lines 134 - 143,
Tests fail because JSDOM doesn't implement element.scrollTo; update the test
setup (or top of the Carousel tests) to mock scrollTo so calls like
container.scrollTo(...) in Carousel (the logic that sets left to
container.clientWidth * (totalItems + 1) or 0) won't throw; add a beforeAll or
beforeEach that sets Element.prototype.scrollTo = vi.fn() and optionally
implement the mock to set this.scrollLeft when options.left is provided so
assertions about scroll position still work.
- Mock Element.prototype.scrollTo for jsdom compatibility - Use realCarouselItems() helper to skip clone wrappers (extended items: [cloneLast, ...items, cloneFirst])
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/ui/src/components/Carousel/Carousel.test.tsx (1)
7-12: RestoreElement.prototype.scrollToafter each test.Direct prototype assignment in
beforeEachmutates global state and can leak into other test files. Capture and restore the original descriptor/value inafterEach.🧪 Safer mock lifecycle
+let originalScrollTo: typeof Element.prototype.scrollTo | undefined; + beforeEach(() => { vi.useFakeTimers(); vi.spyOn(global, "setTimeout"); // Mock scrollTo since jsdom does not implement it - Element.prototype.scrollTo = vi.fn(); + originalScrollTo = Element.prototype.scrollTo; + Object.defineProperty(Element.prototype, "scrollTo", { + configurable: true, + writable: true, + value: vi.fn(), + }); }); afterEach(() => { + if (originalScrollTo) { + Object.defineProperty(Element.prototype, "scrollTo", { + configurable: true, + writable: true, + value: originalScrollTo, + }); + } else { + delete (Element.prototype as Partial<Element>).scrollTo; + } vi.clearAllMocks(); vi.runOnlyPendingTimers(); vi.useRealTimers(); });Also applies to: 14-18
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Carousel/Carousel.test.tsx` around lines 7 - 12, Store the original Element.prototype.scrollTo (or its property descriptor via Object.getOwnPropertyDescriptor) at the start of the test setup (inside beforeEach), replace it with the vi.fn() mock for tests, and then restore the original value/descriptor inside afterEach; also undo test timer/spy changes (vi.useRealTimers() and restore spies/mocks) so Element.prototype.scrollTo and global timer behavior are returned to their original state after each test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/ui/src/components/Carousel/Carousel.test.tsx`:
- Around line 149-152: realCarouselItems() is brittle because it unconditionally
strips the first and last element from carouselItems(), breaking tests when
cloned slides aren't present; update realCarouselItems to return the full array
from carouselItems() (or only remove clones by detecting a clone marker/class if
your markup uses one) instead of always slicing off first/last so tests that
expect edge items (referenced in failing assertions) see the real items; modify
the realCarouselItems helper (and adjust any tests that relied on the old
behavior) to use carouselItems() directly or filter by a clone identifier rather
than items.slice(1, items.length - 1).
---
Nitpick comments:
In `@packages/ui/src/components/Carousel/Carousel.test.tsx`:
- Around line 7-12: Store the original Element.prototype.scrollTo (or its
property descriptor via Object.getOwnPropertyDescriptor) at the start of the
test setup (inside beforeEach), replace it with the vi.fn() mock for tests, and
then restore the original value/descriptor inside afterEach; also undo test
timer/spy changes (vi.useRealTimers() and restore spies/mocks) so
Element.prototype.scrollTo and global timer behavior are returned to their
original state after each test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ff6fa9c0-7b90-4bc9-98bc-9fa652d76741
📒 Files selected for processing (1)
packages/ui/src/components/Carousel/Carousel.test.tsx
Comment on lines
+149
to
+152
| const realCarouselItems = () => { | ||
| const items = carouselItems(); | ||
| return items.slice(1, items.length - 1); | ||
| }; |
Contributor
There was a problem hiding this comment.
realCarouselItems() is brittle and is likely causing the current failing assertions.
The helper always strips the first/last nodes, which breaks when clones are not present (or not represented in queried items for that render state). That matches the failing expectations on Line 25, Line 82, Line 100, and Line 118.
🔧 Proposed fix
const carouselItems = () => screen.getAllByTestId("carousel-item");
// Real items exclude the prepended and appended clones
const realCarouselItems = () => {
const items = carouselItems();
- return items.slice(1, items.length - 1);
+ const indicators = screen.queryAllByTestId("carousel-indicator");
+ const hasBoundaryClones = indicators.length > 0 && items.length === indicators.length + 2;
+ return hasBoundaryClones ? items.slice(1, items.length - 1) : items;
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ui/src/components/Carousel/Carousel.test.tsx` around lines 149 -
152, realCarouselItems() is brittle because it unconditionally strips the first
and last element from carouselItems(), breaking tests when cloned slides aren't
present; update realCarouselItems to return the full array from carouselItems()
(or only remove clones by detecting a clone marker/class if your markup uses
one) instead of always slicing off first/last so tests that expect edge items
(referenced in failing assertions) see the real items; modify the
realCarouselItems helper (and adjust any tests that relied on the old behavior)
to use carouselItems() directly or filter by a clone identifier rather than
items.slice(1, items.length - 1).
jsdom does not implement clientWidth, causing scroll-based index calculations to produce incorrect results. Adding a mock ensures the carousel correctly identifies the active slide in tests.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
isAnimatingguard to prevent rapid clicks from breaking transition stateProblem
When navigating from the last slide to the first (or vice versa), the carousel scrolled backwards through ALL intermediate slides instead of performing a smooth circular transition. This is a significant UX regression compared to vanilla Flowbite.
Technical Approach
Uses the standard clone-based infinite carousel technique:
[cloneLast, slide1, slide2, ..., slideN, cloneFirst]Changes
packages/ui/src/components/Carousel/Carousel.tsx: Rewrite navigation logic with clone-based circular transitionsTest plan
Fixes #1103
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests