[CDX-376] Emit events on user interaction

[esezen]

Pull Request Checklist

Before you submit a pull request, please make sure you have to following:

• I have added or updated TypeScript types for my changes, ensuring they are compatible with the existing codebase.
• I have added JSDoc comments to my TypeScript definitions for improved documentation.
• I have added tests that prove my fix is effective or that my feature works.
• I have added any necessary documentation (if appropriate).
• I have made sure my PR is up-to-date with the main branch.

PR Type

What kind of change does this PR introduce?

• Bugfix
• Feature
• Code style update (formatting, local variables)
• Refactoring (no functional changes, no API changes)
• Documentation content changes
• TypeScript type definitions update
• Other... Please describe:

---

Events to Emit:

Product Card is Clicked - cio.components.productCard.click

Product is converted - cio.components.productCard.conversion

Image is hovered - cio.components.productCard.imageEnter / cio.components.productCard.imageLeave

Carousel next button is clicked - cio.components.carousel.next

Carousel prev button is clicked - cio.components.carousel.previous

Continued here

[Mudaafi]

Liked what you did here, but I have some concerns over where we're emitting the events from and what we're expecting to happen. Might just be me misunderstanding the requirements but lmk what you think.

[Mudaafi]

should this just be canScroll or do we need both indicators?

[esezen]

I think it needs both, no?

[Mudaafi]

Why would it need both? Isn't the event fired a nav event in a particular direction so shouldn't only the canScroll in that direction be relevant?

[esezen]

Maybe I'm missing something but let's say I scroll to a side, I was thinking the event would tell the listener if there is still more room to scroll to next or prev. However, I just noticed the implementation is wrong and the event emits the previous state rather than current. I will just remove these

[Copilot]

Pull request overview

Copilot reviewed 19 out of 20 changed files in this pull request and generated no new comments.

---

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

[Mudaafi]

lgtm, thanks for making the changes. I like the event log in the stories

[constructor-claude-bedrock]

Code Review Results

✅ Strengths

The pub-sub event system is well-architected: a frozen CIO_EVENTS constants object, a fully-typed CioEventDetailMap, a single dispatchCioEvent utility that safely handles SSR, and excellent test coverage across unit, component, and integration levels that covers bubbling, cross-pollination prevention, and the SSR no-op path.

🚨 Critical Issues

• [File: src/components/product-card.tsx Lines: ~310-325] The handleProductClick guard uses target.closest('[data-cnstrc-btn]') to suppress the productCard.click event when a conversion button is clicked. However, e.target is the innermost element clicked (e.g. the <img> inside the wishlist button), while e.currentTarget is the <Card> root. The guard walks up from e.target, which works, but the dispatchCioEvent call passes e.currentTarget as the dispatch target. If the click propagates naturally before the guard is evaluated there is a subtle ordering dependency. More critically, the guard relies on data-cnstrc-btn being present on conversion buttons — but this attribute is set by the <Button> component's conversionType prop. If a consumer replaces AddToCartButton or WishlistButton via componentOverrides with a plain element that lacks data-cnstrc-btn, the productCard.click event will fire on top of the custom conversion action, breaking the intended mutual exclusion. This contract is undocumented and fragile. Consider documenting the requirement, or switching to e.stopPropagation() inside the conversion handlers instead of relying on a DOM attribute sentinel.

• [File: src/utils/events.ts Lines: ~56-68] dispatchCioEvent is typed to accept EventTarget | null for target, but the function body only guards against typeof window === 'undefined' (SSR). If target is a detached DOM node (not connected to any document), dispatchEvent will still be called and the event will not bubble to any listening ancestor — silently producing no-ops for consumers who listen on parent elements. This is particularly relevant given the e.currentTarget usage in handlers. While not a crash, it is a silent failure mode worth guarding or documenting.

⚠️ Important Issues

• [File: src/components/product-card.tsx Lines: ~330-348] The ProductCard component memoizes contextValue with React.useMemo, but the dependency array includes the entire props object (spread as ...props into renderProps). Because props is a new object reference on every render, useMemo will recompute on every render, defeating its purpose and causing all context consumers to re-render unnecessarily. The dependency array should list individual stable primitive/reference props instead of relying on the spread object.

• [File: src/components/carousel.tsx Lines: ~194-212] CarouselItem renders with className='shrink-0 grow-0 basis-full flex items-stretch justify-center items-center' — items-center appears twice (Tailwind class duplication). The second items-center is redundant and may cause confusion; one should be removed.

• [File: src/components/carousel.tsx Lines: ~159-175] CarouselNavButton's handleClick callback has [isPrevious, direction, scrollFn] as dependencies but does not include dispatchCioEvent in the list (it is a module-level stable import so this is technically fine), however direction is always in sync with isPrevious — they are derived from the same direction prop. Including both is not wrong but is redundant; prefer depending only on direction and deriving isPrevious inside the callback.

• [File: src/utils/events.ts Lines: ~6-17] CIO_EVENTS.productCard.wishlist (cio.components.productCard.wishlist) is defined in the constants map and its detail interface is declared in CioEventDetailMap, but it is not listed in the PR description as one of the intended events to emit. If this is intentional (undocumented extension), it should be called out explicitly in the PR. If it was forgotten in the description, the description should be updated.

• [File: spec/components/product-card/product-card.test.tsx Line: 14] The type annotation '79.99' as string | undefined is unnecessary — a string literal is already assignable to string | undefined without a cast. Remove the type assertion.

• [File: spec/components/product-card/product-card.test.tsx Lines: 399-402] The 'Pub-Sub Events' describe block has its own nested afterEach(() => { cleanup(); }) while the outer describe('ProductCard component') already calls cleanup() in its afterEach (line 33). The nested afterEach is redundant and can be removed.

💡 Suggestions

• [File: src/utils/events.ts Lines: ~56-68] Consider adding a composed: true option to the CustomEvent constructor so events can cross shadow DOM boundaries if consumers use Web Components. This is a forward-compatibility concern.

• [File: src/components/product-card.tsx Lines: ~358-363] getProductCardDataAttributes is exported as a static method on ProductCard (ProductCard.getProductCardDataAttributes), which exposes an implementation detail as part of the public API. If consumers are not expected to call it directly, consider removing it from the compound component namespace or moving it to the src/utils barrel.

• [File: src/index.ts Lines: ~9-13] The public API now exports CIO_EVENTS, dispatchCioEvent, and the event detail types. Consider whether dispatchCioEvent should be part of the public surface — consumers listening to events don't need to call it, and exposing it could lead to misuse (e.g. emitting fake Constructor.io events from external code). If the intent is purely for advanced consumers, a JSDoc @internal marker or a separate advanced export path would clarify the intent.

• [File: spec/utils/events.test.ts Lines: ~40-55] The SSR test manually deletes globalThis.window and restores it in a finally block. Consider using Vitest's vi.stubGlobal / vi.unstubAllGlobals pattern for cleaner isolation that is automatically reversed even when tests are skipped or fail mid-execution.

Overall Assessment: ⚠️ Needs Work