Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions .changeset/carousel-circular-transition.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
---
"flowbite-react": patch
---

fix(carousel): seamless circular transition between last and first slides

### Changes

- [x] implement circular carousel using clone-based technique for smooth wrap-around
- [x] extract animation duration to named constant
- [x] prevent memory leak with timeout cleanup on unmount
- [x] guard initialization to prevent reset on dynamic children updates
50 changes: 30 additions & 20 deletions packages/ui/src/components/Carousel/Carousel.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@ import { Carousel } from "./Carousel";
beforeEach(() => {
vi.useFakeTimers();
vi.spyOn(global, "setTimeout");
// Mock scrollTo since jsdom does not implement it
Element.prototype.scrollTo = vi.fn();
// Mock clientWidth so scroll-based index calculations work correctly in jsdom
Object.defineProperty(HTMLElement.prototype, "clientWidth", { configurable: true, value: 100 });
});

afterEach(() => {
Expand All @@ -19,8 +23,9 @@ describe("Components / Carousel", () => {
it("should render and show first item", () => {
render(<TestCarousel />);

expect(carouselItems()[0]).toHaveAttribute("data-active", "true");
expect(carouselItems()[1]).toHaveAttribute("data-active", "false");
// With cloned slides the real items start at index 1
expect(realCarouselItems()[0]).toHaveAttribute("data-active", "true");
expect(realCarouselItems()[1]).toHaveAttribute("data-active", "false");
expect(carouselIndicators()).toHaveLength(5);
expect(carouselIndicators()[0]).toHaveClass(activeIndicatorClasses);
expect(carouselIndicators()[1]).toHaveClass(nonActiveIndicatorClasses);
Expand All @@ -38,14 +43,14 @@ describe("Components / Carousel", () => {
render(<TestCarousel />);

expect(carouselIndicators()[0]).toHaveClass(activeIndicatorClasses);
expect(carouselItems()[0]).toHaveAttribute("data-active", "true");
expect(realCarouselItems()[0]).toHaveAttribute("data-active", "true");

act(() => {
fireEvent.click(carouselIndicators()[3]);
});

expect(carouselItems()[0]).toHaveAttribute("data-active", "false");
expect(carouselItems()[3]).toHaveAttribute("data-active", "true");
expect(realCarouselItems()[0]).toHaveAttribute("data-active", "false");
expect(realCarouselItems()[3]).toHaveAttribute("data-active", "true");
expect(carouselIndicators()[0]).not.toHaveClass(activeIndicatorClasses);
expect(carouselIndicators()[3]).toHaveClass(activeIndicatorClasses);
});
Expand All @@ -61,68 +66,68 @@ describe("Components / Carousel", () => {
render(<TestCarousel />);

expect(carouselIndicators()[0]).toHaveClass(activeIndicatorClasses);
expect(carouselItems()[0]).toHaveAttribute("data-active", "true");
expect(realCarouselItems()[0]).toHaveAttribute("data-active", "true");

act(() => {
fireEvent.click(carouselRightControl());
});

expect(carouselItems()[0]).toHaveAttribute("data-active", "false");
expect(carouselItems()[1]).toHaveAttribute("data-active", "true");
expect(realCarouselItems()[0]).toHaveAttribute("data-active", "false");
expect(realCarouselItems()[1]).toHaveAttribute("data-active", "true");
expect(carouselIndicators()[0]).not.toHaveClass(activeIndicatorClasses);
expect(carouselIndicators()[1]).toHaveClass(activeIndicatorClasses);
});

it("should transition to the next item after about 3 s by default", () => {
render(<TestCarousel />);

expect(carouselItems()[0]).toHaveAttribute("data-active", "true");
expect(carouselItems()[1]).toHaveAttribute("data-active", "false");
expect(realCarouselItems()[0]).toHaveAttribute("data-active", "true");
expect(realCarouselItems()[1]).toHaveAttribute("data-active", "false");
expect(carouselIndicators()[0]).toHaveClass(activeIndicatorClasses);
expect(carouselIndicators()[1]).toHaveClass(nonActiveIndicatorClasses);

act(() => {
vi.advanceTimersByTime(3000);
});

expect(carouselItems()[0]).toHaveAttribute("data-active", "false");
expect(carouselItems()[1]).toHaveAttribute("data-active", "true");
expect(realCarouselItems()[0]).toHaveAttribute("data-active", "false");
expect(realCarouselItems()[1]).toHaveAttribute("data-active", "true");
expect(carouselIndicators()[0]).toHaveClass(nonActiveIndicatorClasses);
expect(carouselIndicators()[1]).toHaveClass(activeIndicatorClasses);
});

it("should transition to the next item after `slideInterval` when it is provided", () => {
render(<TestCarousel slideInterval={9000} />);

expect(carouselItems()[0]).toHaveAttribute("data-active", "true");
expect(carouselItems()[1]).toHaveAttribute("data-active", "false");
expect(realCarouselItems()[0]).toHaveAttribute("data-active", "true");
expect(realCarouselItems()[1]).toHaveAttribute("data-active", "false");
expect(carouselIndicators()[0]).toHaveClass(activeIndicatorClasses);
expect(carouselIndicators()[1]).toHaveClass(nonActiveIndicatorClasses);

act(() => {
vi.advanceTimersByTime(9000);
});

expect(carouselItems()[0]).toHaveAttribute("data-active", "false");
expect(carouselItems()[1]).toHaveAttribute("data-active", "true");
expect(realCarouselItems()[0]).toHaveAttribute("data-active", "false");
expect(realCarouselItems()[1]).toHaveAttribute("data-active", "true");
expect(carouselIndicators()[0]).toHaveClass(nonActiveIndicatorClasses);
expect(carouselIndicators()[1]).toHaveClass(activeIndicatorClasses);
});

it("should not automatically transition to the next item when `slide={false}`", () => {
render(<TestCarousel slide={false} />);

expect(carouselItems()[0]).toHaveAttribute("data-active", "true");
expect(carouselItems()[1]).toHaveAttribute("data-active", "false");
expect(realCarouselItems()[0]).toHaveAttribute("data-active", "true");
expect(realCarouselItems()[1]).toHaveAttribute("data-active", "false");
expect(carouselIndicators()[0]).toHaveClass(activeIndicatorClasses);
expect(carouselIndicators()[1]).toHaveClass(nonActiveIndicatorClasses);

act(() => {
vi.advanceTimersByTime(3000);
});

expect(carouselItems()[0]).toHaveAttribute("data-active", "true");
expect(carouselItems()[1]).toHaveAttribute("data-active", "false");
expect(realCarouselItems()[0]).toHaveAttribute("data-active", "true");
expect(realCarouselItems()[1]).toHaveAttribute("data-active", "false");
expect(carouselIndicators()[0]).toHaveClass(activeIndicatorClasses);
expect(carouselIndicators()[1]).toHaveClass(nonActiveIndicatorClasses);
});
Expand All @@ -142,6 +147,11 @@ const TestCarousel = (props: CarouselProps) => (
);

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);
};
Comment on lines +151 to +154
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

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).

const carouselIndicators = () => screen.getAllByTestId("carousel-indicator");
const carouselLeftControl = () => screen.getByTestId("carousel-left-control");
const carouselRightControl = () => screen.getByTestId("carousel-right-control");
148 changes: 134 additions & 14 deletions packages/ui/src/components/Carousel/Carousel.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ export interface CarouselProps extends ComponentProps<"div">, ThemingProps<Carou

export interface DefaultLeftRightControlProps extends ComponentProps<"div">, ThemingProps<CarouselTheme> {}

const SCROLL_ANIMATION_DURATION_MS = 500;

export const Carousel = forwardRef<HTMLDivElement, CarouselProps>((props, ref) => {
const provider = useThemeProvider();
const theme = useResolveTheme(
Expand Down Expand Up @@ -89,8 +91,11 @@ export const Carousel = forwardRef<HTMLDivElement, CarouselProps>((props, ref) =
const [activeItem, setActiveItem] = useState(0);
const [isDragging, setIsDragging] = useState(false);
const [isHovering, setIsHovering] = useState(false);
const [isAnimating, setIsAnimating] = useState(false);

const didMountRef = useRef(false);
const initializedRef = useRef(false);
const transitionTimeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null);

const items = useMemo(
() =>
Expand All @@ -102,31 +107,102 @@ export const Carousel = forwardRef<HTMLDivElement, CarouselProps>((props, ref) =
[children, theme.item.base],
);

const itemCount = items?.length ?? 0;

const navigateTo = useCallback(
(item: number) => () => {
if (!items) return;
item = (item + items.length) % items.length;
if (carouselContainer.current) {
carouselContainer.current.scrollLeft = carouselContainer.current.clientWidth * item;
if (!items || isAnimating) return;

const container = carouselContainer.current;
if (!container) return;

const totalItems = items.length;
const targetItem = ((item % totalItems) + totalItems) % totalItems;

const isWrappingForward = activeItem === totalItems - 1 && item >= totalItems;
const isWrappingBackward = activeItem === 0 && item < 0;

if (isWrappingForward || isWrappingBackward) {
setIsAnimating(true);

if (transitionTimeoutRef.current) {
clearTimeout(transitionTimeoutRef.current);
}

// Scroll to the clone (last element for backward, first-after-last for forward)
if (isWrappingForward) {
container.scrollTo({
left: container.clientWidth * (totalItems + 1),
behavior: "smooth",
});
} else {
container.scrollTo({
left: 0,
behavior: "smooth",
});
}
Comment on lines +134 to +143
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

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.


// After the smooth scroll animation, jump instantly to the real slide
const onTransitionDone = () => {
container.style.scrollBehavior = "auto";
if (isWrappingForward) {
container.scrollLeft = container.clientWidth * 1;
} else {
container.scrollLeft = container.clientWidth * totalItems;
}
container.style.scrollBehavior = "";
setIsAnimating(false);
transitionTimeoutRef.current = null;
};

transitionTimeoutRef.current = setTimeout(onTransitionDone, SCROLL_ANIMATION_DURATION_MS);
setActiveItem(targetItem);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
} else {
// Normal navigation - account for the prepended clone
container.scrollTo({
left: container.clientWidth * (targetItem + 1),
behavior: "smooth",
});
setActiveItem(targetItem);
}
setActiveItem(item);
},
[items],
[items, activeItem, isAnimating],
);

// Initialize scroll position to first real slide (past the prepended clone)
useEffect(() => {
const container = carouselContainer.current;
if (container && items && items.length > 0 && !initializedRef.current) {
initializedRef.current = true;
container.style.scrollBehavior = "auto";
container.scrollLeft = container.clientWidth * 1;
container.style.scrollBehavior = "";
}
}, [items]);

useEffect(() => {
if (carouselContainer.current && !isDragging && carouselContainer.current.scrollLeft !== 0) {
setActiveItem(Math.round(carouselContainer.current.scrollLeft / carouselContainer.current.clientWidth));
if (carouselContainer.current && !isDragging && !isAnimating) {
const container = carouselContainer.current;
const rawIndex = Math.round(container.scrollLeft / container.clientWidth);
// Account for the prepended clone: real items start at index 1
const totalItems = items?.length ?? 0;
if (totalItems > 0) {
const realIndex = (((rawIndex - 1) % totalItems) + totalItems) % totalItems;
setActiveItem(realIndex);
}
}
}, [isDragging]);
}, [isDragging, isAnimating, items]);

useEffect(() => {
if (slide && !(pauseOnHover && isHovering)) {
const intervalId = setInterval(() => !isDragging && navigateTo(activeItem + 1)(), slideInterval ?? 3000);
const intervalId = setInterval(
() => !isDragging && !isAnimating && navigateTo(activeItem + 1)(),
slideInterval ?? 3000,
);

return () => clearInterval(intervalId);
}
}, [activeItem, isDragging, navigateTo, slide, slideInterval, pauseOnHover, isHovering]);
}, [activeItem, isDragging, isAnimating, navigateTo, slide, slideInterval, pauseOnHover, isHovering]);

useEffect(() => {
if (didMountRef.current) {
Expand All @@ -136,11 +212,55 @@ export const Carousel = forwardRef<HTMLDivElement, CarouselProps>((props, ref) =
}
}, [onSlideChange, activeItem]);

// Cleanup transition timeout on unmount
useEffect(() => {
return () => {
if (transitionTimeoutRef.current) {
clearTimeout(transitionTimeoutRef.current);
}
};
}, []);

const handleDragging = (dragging: boolean) => () => setIsDragging(dragging);

// Handle drag end: snap to nearest real slide and fix wrap-around
const handleDragEnd = useCallback(() => {
setIsDragging(false);
const container = carouselContainer.current;
if (!container || !items) return;

const totalItems = items.length;
const rawIndex = Math.round(container.scrollLeft / container.clientWidth);

// If scrolled to the prepended clone (index 0), jump to real last slide
if (rawIndex <= 0) {
container.style.scrollBehavior = "auto";
container.scrollLeft = container.clientWidth * totalItems;
container.style.scrollBehavior = "";
setActiveItem(totalItems - 1);
}
// If scrolled to the appended clone (index totalItems + 1), jump to real first slide
else if (rawIndex >= totalItems + 1) {
container.style.scrollBehavior = "auto";
container.scrollLeft = container.clientWidth * 1;
container.style.scrollBehavior = "";
setActiveItem(0);
} else {
setActiveItem(rawIndex - 1);
}
}, [items]);

const setHoveringTrue = useCallback(() => setIsHovering(true), []);
const setHoveringFalse = useCallback(() => setIsHovering(false), []);

// Build the extended items array: [cloneLast, ...items, cloneFirst]
const extendedItems = useMemo(() => {
if (!items || items.length === 0) return items;
const lastClone = cloneElement(items[items.length - 1], { key: "clone-last" });
const firstClone = cloneElement(items[0], { key: "clone-first" });
return [lastClone, ...items, firstClone];
}, [items]);

return (
<div
ref={ref}
Expand All @@ -156,16 +276,16 @@ export const Carousel = forwardRef<HTMLDivElement, CarouselProps>((props, ref) =
className={twMerge(theme.scrollContainer.base, (isDeviceMobile || !isDragging) && theme.scrollContainer.snap)}
draggingClassName="cursor-grab"
innerRef={carouselContainer}
onEndScroll={handleDragging(false)}
onEndScroll={handleDragEnd}
onStartScroll={handleDragging(draggable)}
vertical={false}
horizontal={draggable}
>
{items?.map((item, index) => (
{extendedItems?.map((item, index) => (
<div
key={index}
className={theme.item.wrapper[draggable ? "on" : "off"]}
data-active={activeItem === index}
data-active={activeItem === (index - 1 + itemCount) % itemCount}
data-testid="carousel-item"
>
{item}
Expand Down
Loading