[CDX-327] Pass callbacks to ProductCard

[esezen]

No description provided.

[Copilot]

Pull request overview

This PR adds support for passing callback functions to ProductCard components within a Carousel. The change enables parent components to handle product interactions (clicks, add to cart, add to wishlist) from carousel items.

Changes:

• Added CarouselItemCallbacks type definition with three optional callback functions
• Updated Carousel component to accept and pass through itemCallbacks to ProductCard instances
• Added comprehensive test coverage for all three callback scenarios plus the no-callbacks case

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
src/types/carouselTypes.ts	Defines new CarouselItemCallbacks type and adds itemCallbacks to CarouselOpts
src/components/carousel.tsx	Extracts callbacks from props and passes them to ProductCard components
spec/components/Carousel/Carousel.test.tsx	Adds test coverage for all callback scenarios

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[constructor-claude-bedrock]

Code Review Results

✅ Strengths

Well-designed type-safe callback system with comprehensive test coverage for all three callback scenarios.

🚨 Critical Issues

None

⚠️ Important Issues

[File: src/components/carousel.tsx Line: L229-233]
The callback handlers are creating new arrow functions on every render, which could cause unnecessary re-renders of ProductCard components. Consider wrapping these callbacks with useCallback to maintain referential equality across renders.

Suggested approach:

const handleProductClick = useCallback(
  (item: T, index: number) => onProductClick?.(item, index),
  [onProductClick]
);

const handleAddToCart = useCallback(
  (e: React.MouseEvent, item: T, index: number) => onAddToCart?.(e, item, index),
  [onAddToCart]
);

const handleAddToWishlist = useCallback(
  (e: React.MouseEvent, item: T, index: number) => onAddToWishlist?.(e, item, index),
  [onAddToWishlist]
);

Then pass stable references:

<ProductCard
  product={product}
  className='w-full h-full'
  onProductClick={onProductClick ? () => handleProductClick(item, index) : undefined}
  onAddToCart={onAddToCart ? (e) => handleAddToCart(e, item, index) : undefined}
  onAddToWishlist={onAddToWishlist ? (e) => handleAddToWishlist(e, item, index) : undefined}
/>

[File: spec/components/Carousel/Carousel.test.tsx Line: L572]
The comment "buttons won't exist" is misleading. The buttons (Add to Cart and Wishlist) should still exist and be rendered even without callbacks. The test should verify that clicking doesn't throw when callbacks are undefined, but the buttons should still be present in the ProductCard component. Consider updating the comment or the test assertion to be more accurate.

💡 Suggestions

[File: src/types/carouselTypes.ts Line: L74-78]
The CarouselItemCallbacks type is well-structured, but the naming convention could be more explicit. Consider renaming to CarouselProductCallbacks since these callbacks are specific to ProductCard interactions, or add a JSDoc comment explaining that these callbacks are forwarded to ProductCard components within the carousel.

[File: src/components/carousel.tsx Line: L185]
The destructuring could be simplified by using a default empty object inline:

const { onProductClick, onAddToCart, onAddToWishlist } = itemCallbacks ?? {};

While both approaches work, using ?? is more consistent with modern JavaScript practices.

[File: spec/components/Carousel/Carousel.test.tsx Line: L541-577]
Consider adding a test case that verifies all three callbacks can be used together simultaneously to ensure they don't interfere with each other.

Overall Assessment: ✅ Pass

---

Summary: The implementation is solid with good type safety and test coverage. The main concern is the performance optimization around callback memoization, which should be addressed to prevent unnecessary re-renders in carousel scenarios with many items. The code is otherwise production-ready.

[constructor-claude-bedrock]

Code Review Results

✅ Strengths

Well-structured implementation with comprehensive test coverage for all three callbacks and proper optional handling.

🚨 Critical Issues

None found.

⚠️ Important Issues

[File: src/components/carousel.tsx Lines: 229-233]
The callbacks are being wrapped with unnecessary conditional checks that create new functions on every render. This pattern can cause performance issues and breaks React's reconciliation optimization.

More critically, there's a signature mismatch: The ProductCard component expects different callback signatures than what you're passing. ProductCard's onProductClick (line 364) is called with no arguments, while onAddToCart (line 242) and onAddToWishlist (line 60) are called with only the event.

Your implementation adds product and index parameters. You need to update the ProductCard component's type definitions in src/types/productCardTypes.ts to accept these additional parameters, then update the ProductCard implementation to pass them through.

[File: src/components/carousel.tsx Line: 184]
The destructuring of itemCallbacks creates new wrapper functions for every product on every render. Consider using useCallback or useMemo to optimize performance and prevent unnecessary re-renders.

💡 Suggestions

[File: src/types/carouselTypes.ts Lines: 74-78]
Consider adding JSDoc comments to document the callback signatures and when they're triggered for better developer experience.

[File: spec/components/Carousel/Carousel.test.tsx Line: 572]
The comment on line 572 is slightly misleading. The ProductCard will still render - the handlers just won't be attached. Consider clarifying the comment.

Overall Assessment: ⚠️ Needs Work

The implementation has a significant architectural issue where the callback signatures don't match the ProductCard component's expectations. The ProductCard component needs to be updated to accept and pass through the additional parameters (product and index) to its callbacks.

[constructor-claude-bedrock]

Code Review Results

✅ Strengths

Well-tested implementation with comprehensive test coverage for all three callback scenarios and proper edge case handling.

🚨 Critical Issues

None

⚠️ Important Issues

[File: src/components/carousel.tsx Lines: L229-233]
The callback wrappers passed to ProductCard are creating unnecessary arrow functions that break referential equality. This causes ProductCard to re-render on every Carousel render, even when callbacks haven't changed. Consider extracting these wrappers to memoized callbacks or passing the callbacks with proper memoization.

Current implementation:

onProductClick={onProductClick ? () => onProductClick(item, index) : undefined}
onAddToCart={onAddToCart ? (e) => onAddToCart(e, item, index) : undefined}
onAddToWishlist={onAddToWishlist ? (e) => onAddToWishlist(e, item, index) : undefined}

This creates new function instances on every render. For a carousel with many items, this compounds the performance issue.

[File: src/types/carouselTypes.ts Line: L74-78]
The CarouselItemCallbacks type should be documented with JSDoc comments explaining the callback parameters, especially since the signatures differ from ProductCard's native callbacks (adding item and index parameters).

💡 Suggestions

[File: src/components/carousel.tsx Lines: L184-185]
Consider using a more descriptive destructuring pattern for better code readability:

const { onProductClick, onAddToCart, onAddToWishlist } = itemCallbacks || {};

could be:

const callbacks = itemCallbacks || {};
const { onProductClick, onAddToCart, onAddToWishlist } = callbacks;

Though the current approach is acceptable.

[File: spec/components/Carousel/Carousel.test.tsx Lines: L568-576]
The test "does not throw when itemCallbacks is not provided" could be more explicit by also checking that the ProductCard still renders correctly without callbacks, rather than just ensuring it doesn't throw.

Overall Assessment: ⚠️ Needs Work

The implementation correctly passes callbacks to ProductCard and has solid test coverage. However, the performance issue with arrow function recreation on every render for every item in the carousel should be addressed before merge, as it impacts rendering performance for carousels with many items.

[constructor-claude-bedrock]

Code Review Results

✅ Strengths

Well-tested implementation with comprehensive test coverage for all three callback scenarios and proper handling of optional callbacks.

🚨 Critical Issues

None

⚠️ Important Issues

[File: src/components/carousel.tsx Lines: 229-233]
The callback wrapper functions are recreated on every render, which can cause unnecessary re-renders of ProductCard components. Consider wrapping these with useCallback or memoizing them to maintain referential equality across renders.

// Current implementation:
onProductClick={onProductClick ? () => onProductClick(item, index) : undefined}
onAddToCart={onAddToCart ? (e) => onAddToCart(e, item, index) : undefined}
onAddToWishlist={onAddToWishlist ? (e) => onAddToWishlist(e, item, index) : undefined}

// Recommended: Create memoized callbacks or move to a separate component

[File: src/components/carousel.tsx Line: 229]
Type inconsistency: onProductClick callback passes item and index but ProductCard expects onProductClick?: () => void with no parameters. This works because the wrapper creates a parameterless function, but the Carousel's onProductClick signature is (item: T, index: number) => void. This mismatch could confuse developers - the ProductCard doesn't actually get access to item/index through its click handler.

💡 Suggestions

[File: src/types/carouselTypes.ts Lines: 76-80]
Consider adding JSDoc comments to explain the callback signatures and when each is called, especially the difference between onProductClick (for the entire card click) vs onAddToCart/onAddToWishlist (for button clicks with event object).

[File: spec/components/Carousel/Carousel.test.tsx Line: 632]
The test comment "buttons won't exist" is slightly misleading - the ProductCard will still render, just without the action buttons. Consider clarifying that clicking should work but simply not trigger any callbacks.

Overall Assessment: ⚠️ Needs Work

The implementation is functional and well-tested, but the performance issue with recreating callback wrappers on every render should be addressed before merge. The type inconsistency, while not breaking, could lead to developer confusion and should be clarified.

[Mudaafi]

Shall we move this into the types for ProductCard instead? This will be then be generically interpreted if the consumer uses the carousel for others types

[Mudaafi]

Looks good! I left a comment at https://github.com/Constructor-io/constructorio-ui-components/pull/28/changes#r2708353012 for considerations

[esezen]

Closing without merging in favor of handling this in consuming libraries