Conversation
![constructor-claude-bedrock[bot]](https://avatars.githubusercontent.com/u/9828194?s=60&v=4){kind=small}
| import type { Callbacks } from '../types'; | ||
|
|
||
| export interface ShopifyDefaults { | ||
| selector: string; |
There was a problem hiding this comment.
Important Issue: Inconsistent indentation — the selector property inside the ShopifyDefaults interface uses 4-space indentation while callbacks uses 2-space indentation (the project standard). Align both to 2 spaces.
| }, | ||
| onProductClick(event: CustomEvent<ProductCardEventDetail>) { | ||
| if (typeof window !== 'undefined') { | ||
| const itemId = event.detail.product.id |
There was a problem hiding this comment.
Important Issue: Missing semicolon at the end of this line (const itemId = event.detail.product.id). The rest of the codebase uses semicolons consistently. This will also trigger the ESLint semi rule.
| } | ||
| }, | ||
| }, | ||
| }; No newline at end of file |
There was a problem hiding this comment.
Important Issue: The file is missing a trailing newline at the end (\ No newline at end of file). All other source files in this repo end with a newline. Add one to stay consistent with the project's .editorconfig / Prettier settings.
| onAddToCart(event: CustomEvent<ProductCardEventDetail>) { | ||
| const shopifyId = event.detail.product.id; | ||
|
|
||
| if (typeof window !== 'undefined') { |
There was a problem hiding this comment.
Important Issue: The typeof window !== 'undefined' guard inside onAddToCart is checked after shopifyId has already been extracted from event.detail.product.id. If this code ever runs in an SSR context the extraction itself is fine, but the guard should wrap the entire body consistently — or be removed from onAddToCart altogether since fetch is also a browser/Node built-in. More importantly, the guard in onProductClick is the same: window.location.href assignment is inside the guard which is correct, but the const itemId assignment outside the guard is redundant work when window is undefined. Move both const declarations inside their respective window guards for clarity.
| callbacks: Pick<Callbacks, 'onProductClick' | 'onAddToCart'>; | ||
| } | ||
|
|
||
| // eslint-disable-next-line import/prefer-default-export |
There was a problem hiding this comment.
Suggestion: The ESLint disable comment import/prefer-default-export is a workaround. The ShopifyDefaults interface is also exported from this file, so the rule should not fire (two exports exist). Verify whether the disable is actually needed; if not, remove it.
| parameters, | ||
| itemFieldGetters: { ...defaultGetters, ...itemFieldGetters }, | ||
| callbacks, | ||
| callbacks: { ...(useShopifyDefaults && shopifyDefaults.callbacks), ...callbacks }, |
There was a problem hiding this comment.
Important Issue: When useShopifyDefaults is false (or undefined), useShopifyDefaults && shopifyDefaults.callbacks evaluates to false, so the spread becomes ...false. While this is technically valid JavaScript (spreading false is a no-op), it is unintuitive and may confuse readers or future type-checkers. Use a ternary or explicit conditional instead:
callbacks: {
...(useShopifyDefaults ? shopifyDefaults.callbacks : {}),
...callbacks,
},| }, | ||
| }); | ||
|
|
||
| // Verify the URL is constructed correctly without triggering navigation |
There was a problem hiding this comment.
Important Issue: The onProductClick test does not assert that window.location.href was actually set to the expected URL — it only checks that the callback doesn't throw and independently verifies URL construction logic outside the callback. This means the core behavior (navigation) is untested. Mock window.location (e.g. Object.defineProperty(window, 'location', { value: { href: '' }, writable: true })) and assert that window.location.href equals the expected URL after calling the callback.
There was a problem hiding this comment.
Pull request overview
This PR adds Shopify integration defaults to the Constructor.io Recommendations component. It provides default callbacks for adding items to cart and handling product clicks, along with a default selector for the recommendations container. The feature is opt-in via a useShopifyDefaults boolean prop that merges Shopify default callbacks with any custom callbacks provided by the user.
Changes:
- Created a new
shopifyDefaults.tsutility that exports default callbacks for Shopify cart operations and product navigation, plus a default container selector - Added
useShopifyDefaultsboolean property to theRecommendationsContextValueandCioRecommendationsProviderPropstypes - Implemented callback merging logic in
CioRecommendationsProviderto merge Shopify defaults with custom callbacks - Updated the bundled component to use the Shopify default selector when the flag is enabled
- Added comprehensive test coverage for the new functionality
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/shopifyDefaults.ts | New utility providing Shopify default callbacks and selector |
| src/types.ts | Added useShopifyDefaults property to context and provider prop types |
| src/components/CioRecommendations/CioRecommendationsProvider.tsx | Implements callback merging logic with Shopify defaults |
| src/bundled.jsx | Uses Shopify default selector when useShopifyDefaults is enabled |
| spec/utils/shopifyDefaults.test.ts | New unit tests for Shopify defaults functionality |
| spec/components/CioRecommendations/CioRecommendationsProvider.test.jsx | Tests for callback merging with Shopify defaults |
| spec/components/CioRecommendations/CioRecommendations.test.tsx | Integration tests for Shopify defaults feature |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| parameters, | ||
| itemFieldGetters: { ...defaultGetters, ...itemFieldGetters }, | ||
| callbacks, | ||
| callbacks: { ...(useShopifyDefaults && shopifyDefaults.callbacks), ...callbacks }, |
There was a problem hiding this comment.
Using the logical AND operator (&&) with the spread operator will cause a runtime error when useShopifyDefaults is false or undefined. When useShopifyDefaults is falsy, the expression evaluates to the falsy value itself (e.g., false) instead of an empty object. Attempting to spread a non-iterable boolean will throw a TypeError. This should use a ternary operator instead: useShopifyDefaults ? shopifyDefaults.callbacks : {}
| }, | ||
| onProductClick(event: CustomEvent<ProductCardEventDetail>) { | ||
| if (typeof window !== 'undefined') { | ||
| const itemId = event.detail.product.id |
There was a problem hiding this comment.
Missing semicolon at the end of the statement
| const itemId = event.detail.product.id | |
| const itemId = event.detail.product.id; |
| import type { Callbacks } from '../types'; | ||
|
|
||
| export interface ShopifyDefaults { | ||
| selector: string; |
There was a problem hiding this comment.
Inconsistent indentation in interface property. The selector property has extra indentation compared to the callbacks property below it and compared to the standard formatting used throughout the codebase (see src/types.ts for reference).
| selector: string; | |
| selector: string; |
Pull Request Checklist
Before you submit a pull request, please make sure you have to following:
PR Type
What kind of change does this PR introduce?