[CDX-272] Shopify Feature Toggle

[esezen]

Definition of Done

• Create a feature flag that customers can pass as a prop to opt into
• Absorb the configurations declared in the Shopify connector
• Ensure that the bundled version of the library can take advantage of this feature

Don't forget tests!

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:

[Copilot]

Pull Request Overview

This PR introduces a useShopifyDefaults feature flag that enables Shopify-specific configurations for the CioPlp component. When enabled, it provides default implementations for callbacks and URL helpers tailored to Shopify's cart API and routing conventions.

Key Changes:

• Added a new getShopifyDefaults() utility function that returns Shopify-specific implementations for onAddToCart, onProductCardClick, and setUrl
• Introduced the useShopifyDefaults boolean prop to CioPlpProvider that applies these defaults when enabled
• Custom callbacks and URL helpers can still override the Shopify defaults through the normal prop mechanism

Reviewed Changes

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

Show a summary per file

File	Description
src/utils/shopifyDefaults.ts	New utility function providing Shopify-specific default implementations for cart operations, product navigation, and URL handling
src/utils/index.ts	Exports the new shopifyDefaults utility
src/types.ts	Adds useShopifyDefaults boolean prop to CioPlpProviderProps interface
src/stories/components/CioPlp/CioPlpProps.md	Documentation for the new useShopifyDefaults prop with usage examples
src/stories/components/CioPlp/CioPlp.stories.tsx	Storybook control definition for the useShopifyDefaults prop
src/hooks/useCioPlpProvider.ts	Integrates Shopify defaults into the provider, applying them before custom callbacks/urlHelpers in the merge order
spec/utils/shopifyDefaults.test.ts	Comprehensive test coverage for the Shopify defaults utility functions
spec/hooks/useCioPlpProvider/useCioPlpProvider.test.js	Tests verifying the integration of Shopify defaults with the provider hook
spec/components/CioPlp/CioPlp.test.jsx	Component-level tests ensuring the feature flag works correctly through the full component tree

---

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

[Mudaafi]

LGTM, just some docs lints. We may want to also include SPA-rendering since this is a toggle, but that can be a future story

[Mudaafi]

Let's not specify which configurations. In the near future, we may want to also have itemFieldGetter defaults

[Mudaafi]

Unnecessary imo, it doesn't add enough value to justify the space it takes in the docs

[constructor-claude-bedrock]

Code Review Results

✅ Strengths

Well-structured feature with comprehensive tests covering integration, unit, and override scenarios.

🚨 Critical Issues

[File: spec/components/CioPlp/CioPlp.test.jsx Line: L143-L144]
Missing closing brace and semicolon for the test case "should allow custom callbacks to override Shopify defaults". Line 143 has the expect statement but is missing }); before the next test starts on line 144.

// Current (broken):
expect(getByText("custom-callback")).toBeInTheDocument();
it("renders CioPlp with hideGroups set to true on the client", async () => {

// Should be:
expect(getByText("custom-callback")).toBeInTheDocument();
    });

  it("renders CioPlp with hideGroups set to true on the client", async () => {

[File: src/utils/shopifyDefaults.ts Line: L11]
The setUrl function uses String.replace() which only replaces the first occurrence. If a URL contains multiple /group_id segments, only the first will be replaced. Use a global regex or replaceAll() instead:

// Current:
const modifiedUrl = url.replace("/group_id", "/collections");

// Should be:
const modifiedUrl = url.replace(/\/group_id/g, "/collections");

⚠️ Important Issues

[File: src/utils/shopifyDefaults.ts Line: L24-L34]
The onAddToCart callback does not provide user feedback for success or failure. The fetch call silently fails with only a console.error. Consider:

• Returning a promise so consumers can handle success/failure
• Adding user-facing error feedback (toast notification, error state)
• Documenting that this implementation does not provide feedback

[File: src/utils/shopifyDefaults.ts Line: L37-L43]
The onProductCardClick callback does not validate item.itemId. If itemId is undefined or contains special characters that are not URL-safe, this could create malformed URLs. Add validation:

onProductCardClick(_event: React.MouseEvent, item: Item) {
  if (typeof window \!== "undefined" && item.itemId) {
    const url = new URL(`/products/${encodeURIComponent(item.itemId)}`, window.location.origin);
    window.location.href = url.href;
  }
}

[File: src/types.ts Line: L296]
Missing JSDoc comment for the new useShopifyDefaults prop. While the Storybook description exists, the type definition should include JSDoc for better IDE documentation:

/**
 * When enabled, applies Shopify-specific default configurations for callbacks and URL helpers.
 * Custom configurations provided will override these defaults.
 * @default false
 */
useShopifyDefaults?: boolean;

[File: spec/utils/shopifyDefaults.test.ts Line: L37-L53]
The setUrl tests do not verify that String.replace() handles edge cases like:

• Multiple occurrences: /group_id/foo/group_id/bar
• URL with group_id in query params: /collections?filter=group_id
• Partial matches: /group_ids/foo

Add test cases for these scenarios.

[File: spec/utils/shopifyDefaults.test.ts Line: L56-L159]
The onAddToCart tests mock fetch but do not verify the actual response handling or test what happens with different response statuses (200, 400, 500). Consider adding tests for:

• Successful cart add response
• Rate limiting (429)
• Out of stock (422 or similar)

💡 Suggestions

[File: src/utils/shopifyDefaults.ts Line: L19-L21]
The fallback chain for shopifyId could be more explicit with comments explaining when each fallback is used:

// Priority: 1) Shopify product variant ID, 2) Constructor variation ID, 3) Constructor item ID
const shopifyId = item.data?.__shopify_id || selectedVariation?.variationId || item.itemId;

[File: src/hooks/useCioPlpProvider.ts Line: L28-L31]
The empty object fallback { callbacks: {}, urlHelpers: {} } is created on every render when useShopifyDefaults is false. Consider extracting it as a constant to avoid unnecessary object creation:

const EMPTY_SHOPIFY_DEFAULTS = { callbacks: {}, urlHelpers: {} };

const shopifyDefaults = useMemo(
  () => (useShopifyDefaults ? getShopifyDefaults() : EMPTY_SHOPIFY_DEFAULTS),
  [useShopifyDefaults],
);

[File: src/utils/shopifyDefaults.ts Line: L4-L7]
Consider defining a named interface/type for the return value instead of using inline type for better reusability and documentation:

export interface ShopifyDefaults {
  urlHelpers: Pick<UrlHelpers, "setUrl">;
  callbacks: Pick<Callbacks, "onAddToCart" | "onProductCardClick">;
}

export function getShopifyDefaults(): ShopifyDefaults {

[File: src/stories/components/CioPlp/CioPlpProps.md Line: L1-L213]
The new documentation file is comprehensive but appears to be a duplicate of existing prop documentation. Verify this file is necessary and does not create documentation fragmentation. If it is meant to be the single source of truth, ensure it is linked properly from the main documentation.

Overall Assessment: ❌ Critical Issues

The implementation is well-tested and follows good patterns, but has a critical syntax error that will break tests and a logic bug in the URL replacement function. These must be fixed before merge.

[constructor-claude-bedrock]

Code Review Results

✅ Strengths

Well-tested feature toggle implementation with comprehensive test coverage including edge cases for callback overrides and TypeScript type safety.

🚨 Critical Issues

None

⚠️ Important Issues

[File: src/utils/shopifyDefaults.ts Line: 11]
The string replacement in setUrl is too simplistic and could cause unintended replacements. The current implementation uses url.replace('/group_id', '/collections') which will only replace the first occurrence and won't handle edge cases correctly.

Issue: This could fail or produce incorrect results for:

• URLs with multiple /group_id occurrences
• URLs where /group_id appears in query parameters or hash fragments
• Partial matches like /group_id_something

Suggestion:

setUrl(url: string) {
  const modifiedUrl = url.replace(/\/group_id\//g, '/collections/');
  
  if (typeof window \!== 'undefined') {
    window.location.href = modifiedUrl;
  }
}

This uses a regex with global flag and ensures we're only replacing the path segment pattern /group_id/ (with trailing slash).

---

[File: src/utils/shopifyDefaults.ts Line: 24-35]
The fetch call in onAddToCart lacks proper error handling for non-network errors and doesn't provide user feedback on success or failure.

Issue:

• Users won't know if the item was successfully added to cart
• The error is only logged to console, which users won't see
• No handling of HTTP error responses (4xx, 5xx status codes)

Suggestion: Consider returning a Promise or using a callback pattern to allow consumers to handle success/error states for UI feedback:

onAddToCart(_event: React.MouseEvent, item: Item, selectedVariation?: Variation) {
  const shopifyId = item.data?.__shopify_id || selectedVariation?.variationId || item.itemId;

  if (typeof window \!== 'undefined') {
    return fetch('/cart/add.js', {
      method: 'POST',
      headers: { 'Content-Type': 'application/json' },
      body: JSON.stringify({
        id: shopifyId,
        quantity: 1,
      }),
    })
      .then((response) => {
        if (\!response.ok) {
          throw new Error(`HTTP error\! status: ${response.status}`);
        }
        return response.json();
      })
      .catch((error) => {
        console.error('Failed to add item to cart:', error);
        throw error; // Re-throw to allow consumers to handle
      });
  }
  return Promise.resolve();
}

---

[File: spec/utils/shopifyDefaults.test.ts Line: 40-47]
Test doesn't verify the actual regex replacement behavior for edge cases mentioned above.

Suggestion: Add tests for edge cases:

it('should handle multiple /group_id occurrences', () => {
  const defaults = getShopifyDefaults();
  const testUrl = 'https://example.com/group_id/test/group_id/nested';
  
  defaults.urlHelpers.setUrl(testUrl);
  
  expect(window.location.href).toBe('https://example.com/collections/test/collections/nested');
});

it('should not replace /group_id in query parameters', () => {
  const defaults = getShopifyDefaults();
  const testUrl = 'https://example.com/collections/test?redirect=/group_id/something';
  
  defaults.urlHelpers.setUrl(testUrl);
  
  // Should not modify query params
  expect(window.location.href).toBe(testUrl);
});

---

[File: spec/utils/shopifyDefaults.test.ts Line: 144-158]
The test "should handle fetch errors gracefully" doesn't actually verify graceful error handling - it only checks that no exception is thrown synchronously.

Issue: The test passes even though the promise rejection is unhandled. It doesn't verify that errors are properly caught and logged.

Suggestion:

it('should handle fetch errors gracefully', async () => {
  const consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation();
  const defaults = getShopifyDefaults();
  const mockItem = {
    itemId: 'item-123',
    itemName: 'Test Product',
    data: {},
  } as Item;
  const mockEvent = {} as React.MouseEvent;

  fetchMock.mockRejectedValueOnce(new Error('Network error'));

  await defaults.callbacks.onAddToCart?.(mockEvent, mockItem);

  expect(consoleErrorSpy).toHaveBeenCalledWith(
    'Failed to add item to cart:',
    expect.any(Error)
  );
  
  consoleErrorSpy.mockRestore();
});

💡 Suggestions

[File: src/hooks/useCioPlpProvider.ts Line: 28-31]
The empty objects returned when useShopifyDefaults is false could be extracted as a constant to avoid creating new objects on every render (though this is minor since it's memoized).

Suggestion:

const EMPTY_SHOPIFY_DEFAULTS = { callbacks: {}, urlHelpers: {} };

const shopifyDefaults = useMemo(
  () => (useShopifyDefaults ? getShopifyDefaults() : EMPTY_SHOPIFY_DEFAULTS),
  [useShopifyDefaults],
);

---

[File: src/utils/shopifyDefaults.ts Line: 4-7]
Consider adding JSDoc documentation to explain the feature toggle and what defaults are provided, especially since this is a public API feature.

Suggestion:

/**
 * Returns Shopify-specific default configurations for callbacks and URL helpers.
 * 
 * @returns An object containing:
 * - urlHelpers.setUrl: Converts /group_id URLs to /collections for Shopify compatibility
 * - callbacks.onAddToCart: Adds items to Shopify cart via /cart/add.js API
 * - callbacks.onProductCardClick: Navigates to /products/{itemId}
 * 
 * @example
 * const defaults = getShopifyDefaults();
 * // Use with CioPlp component via useShopifyDefaults prop
 */
export function getShopifyDefaults(): {
  urlHelpers: Pick<UrlHelpers, 'setUrl'>;
  callbacks: Pick<Callbacks, 'onAddToCart' | 'onProductCardClick'>;
}

---

[File: spec/components/CioPlp/CioPlp.test.jsx Line: 75-76]
The test structure nesting "Shopify Defaults" as a describe block at the same level as individual test cases is inconsistent. The two hideGroups tests are now nested inside the Shopify Defaults describe block but aren't actually related to Shopify defaults.

Suggestion: Move the hideGroups tests outside the "Shopify Defaults" describe block:

describe('CioPlp React Client-Side Rendering', () => {
  // ... other tests
  
  describe('Shopify Defaults', () => {
    // Only Shopify-related tests
  });
  
  it('renders CioPlp with hideGroups set to true on the client', async () => {
    // test implementation
  });
  
  it('renders CioPlp with hideGroups set to false on the client', async () => {
    // test implementation  
  });
});

Overall Assessment: ⚠️ Needs Work

The implementation is solid with good test coverage, but there are important issues with the URL replacement logic and error handling that should be addressed before merging. The string replacement approach is too simplistic and could cause bugs in production. Additionally, the fetch error handling needs improvement to provide better user feedback.

[constructor-claude-bedrock]

Code Review Results

✅ Strengths

Well-structured implementation with comprehensive test coverage, clear separation of concerns, and proper TypeScript typing throughout the new feature.

🚨 Critical Issues

[File: src/utils/shopifyDefaults.ts Line: L12] The setUrl function uses a simple string replacement that could incorrectly replace /group_id in other parts of the URL (e.g., query params, fragments). Use a more precise regex pattern or path-based replacement:

setUrl(url: string) {
  const modifiedUrl = url.replace(/\/group_id\//g, '/collections/');
  if (typeof window !== 'undefined') {
    window.location.href = modifiedUrl;
  }
}

[File: src/hooks/useCioPlpProvider.ts Line: L30] The shopifyDefaultsValue is computed outside of useMemo, but it's used inside the useMemo dependency array at line 53. This creates a stable reference issue - the object reference changes on every render when useShopifyDefaults is true, causing unnecessary re-renders. Move the conditional logic inside useMemo:

const contextValue = useMemo(
  (): PlpContextValue => {
    const shopifyDefaultsValue = useShopifyDefaults ? shopifyDefaults : EMPTY_SHOPIFY_DEFAULTS;
    return {
      cioClient,
      cioClientOptions,
      setCioClientOptions,
      staticRequestConfigs,
      itemFieldGetters: { ...defaultGetters, ...itemFieldGetters },
      formatters: { ...defaultFormatters, ...formatters },
      callbacks: { ...shopifyDefaultsValue.callbacks, ...callbacks },
      urlHelpers: { ...defaultUrlHelpers, ...shopifyDefaultsValue.urlHelpers, ...urlHelpers },
      renderOverrides: { ...renderOverrides },
    };
  },
  [
    cioClient,
    cioClientOptions,
    itemFieldGetters,
    formatters,
    callbacks,
    urlHelpers,
    renderOverrides,
    staticRequestConfigs,
    useShopifyDefaults, // Replace shopifyDefaultsValue with useShopifyDefaults
  ],
);

⚠️ Important Issues

[File: src/utils/shopifyDefaults.ts Line: L25-36] The onAddToCart callback doesn't provide user feedback on success or failure. Consider adding a success notification or returning a promise so consumers can handle the result:

onAddToCart(_event: React.MouseEvent, item: Item, selectedVariation?: Variation) {
  const shopifyId = item.data?.__shopify_id || selectedVariation?.variationId || item.itemId;

  if (typeof window !== 'undefined') {
    return fetch('/cart/add.js', {
      method: 'POST',
      headers: { 'Content-Type': 'application/json' },
      body: JSON.stringify({
        id: shopifyId,
        quantity: 1,
      }),
    }).then(() => {
      // Consider adding success notification here
    }).catch((error) => {
      console.error('Failed to add item to cart:', error);
      // Consider adding error notification here
    });
  }
}

[File: spec/utils/shopifyDefaults.test.ts Line: L144] The error handling test doesn't actually verify that errors are handled gracefully - it only checks that the function doesn't throw. Consider testing that the error is logged or that the promise rejects properly.

[File: src/utils/shopifyDefaults.ts Line: L12] The URL replacement only handles /group_id but the tests suggest there might be other patterns like /collection_id. Ensure consistency with what patterns are supported based on getUrlFromState in urlHelpers (line 162 in CioPlpProps.md shows both group_id and collection_id patterns).

💡 Suggestions

[File: src/hooks/useCioPlpProvider.ts Line: L9] Consider freezing the EMPTY_SHOPIFY_DEFAULTS constant to prevent accidental mutations:

const EMPTY_SHOPIFY_DEFAULTS = Object.freeze({ callbacks: {}, urlHelpers: {} });

[File: spec/utils/shopifyDefaults.test.ts Line: L64-65] Consider adding more edge case tests:

• Test with malformed URLs
• Test when window.location.href assignment fails
• Test with special characters in itemId
• Test network timeout scenarios for the fetch call

[File: src/stories/components/CioPlp/CioPlpProps.md Line: L1-213] The new documentation file appears to duplicate existing documentation. Consider whether this file is necessary or if the useShopifyDefaults documentation should be integrated into existing documentation files to avoid maintenance burden.

[File: src/utils/shopifyDefaults.ts Line: L38-44] The onProductCardClick creates a new URL object but could be simplified since you're constructing from scratch:

onProductCardClick(_event: React.MouseEvent, item: Item) {
  if (typeof window !== 'undefined') {
    window.location.href = `${window.location.origin}/products/${item.itemId}`;
  }
}

Overall Assessment: ⚠️ Needs Work

The feature implementation is solid with good test coverage, but there are critical performance issues with the memoization setup and potential bugs in the URL replacement logic that should be addressed before merging.

[constructor-claude-bedrock]

Code Review Results

✅ Strengths

Comprehensive test coverage with unit tests for all Shopify default callbacks and URL helpers, clear API design with proper TypeScript types, and well-structured separation of concerns.

🚨 Critical Issues

[File: src/utils/shopifyDefaults.ts Line: L12] The URL replacement logic uses .replace('/group_id', '/collections') which only replaces the first occurrence. This will fail for URLs like https://example.com/group_id/foo/group_id/bar. Use a global replace with regex: url.replace(/\/group_id\//g, '/collections/') or url.replace(/\/group_id(?=\/)/g, '/collections') to ensure all occurrences in the path are replaced correctly. Additionally, the current implementation will incorrectly replace /group_id even if it appears in query parameters or fragments.

[File: src/utils/shopifyDefaults.ts Line: L25-36] The onAddToCart callback lacks proper error handling for failed fetch responses (e.g., 404, 500 status codes). While the catch block handles network errors, successful responses with error status codes are not handled. The Shopify cart API may return specific error messages that should be surfaced to users. Consider checking response.ok and handling error responses appropriately, or at minimum add a comment explaining why this is intentionally fire-and-forget.

⚠️ Important Issues

[File: src/stories/components/CioPlp/CioPlpProps.md Line: L26, L49] Typo in documentation: "funciton" should be "function" (appears in lines for formatPrice and getPrice descriptions).

[File: src/stories/components/CioPlp/CioPlpProps.md Line: L1-213] The documentation file CioPlpProps.md is created as a new file but appears to duplicate content that may already exist elsewhere in the documentation. This creates a maintenance burden - if this content exists in another location, consider updating that file instead or ensure there is a clear reason for the duplication. If this is the first comprehensive props documentation, consider whether this should live in a more discoverable location like a main README or docs folder.

[File: src/utils/shopifyDefaults.ts Line: L11-12] The setUrl function performs a full page navigation with window.location.href, which will cause a complete page reload. This may not be the expected behavior in modern SPAs. Consider adding a JSDoc comment warning developers that this causes a full page reload, or consider using a more SPA-friendly approach if the library supports it.

💡 Suggestions

[File: src/utils/shopifyDefaults.ts Line: L22] The ID fallback chain (__shopify_id || variationId || itemId) is well-designed, but consider extracting this logic into a separate helper function like getShopifyProductId(item, variation) for better testability and potential reuse. This would also make it easier to document the ID resolution strategy.

[File: src/hooks/useCioPlpProvider.ts Line: L9] The constant EMPTY_SHOPIFY_DEFAULTS could be defined with as const for better type inference: const EMPTY_SHOPIFY_DEFAULTS = { callbacks: {}, urlHelpers: {} } as const;. This provides stronger immutability guarantees.

[File: spec/utils/shopifyDefaults.test.ts Line: L35-50] Consider adding edge case tests for setUrl:

• URLs with multiple /group_id occurrences
• URLs with /group_id in query parameters (e.g., ?redirect=/group_id/test)
• URLs with /group_id in hash fragments
• URLs ending with /group_id without a trailing path segment

[File: src/utils/shopifyDefaults.ts Line: L20-37] Consider using async/await syntax instead of promise chaining for better readability and error handling in onAddToCart. This would make the error flow clearer and allow for more sophisticated error handling in the future.

Overall Assessment: ⚠️ Needs Work

The feature implementation is well-structured with excellent test coverage, but the critical URL replacement bug needs to be fixed before merge, and the fetch error handling should be improved or explicitly documented as intentional.

[HHHindawy]

How about we spread it like this:

Suggested change

	callbacks: { ...shopifyDefaultsValue.callbacks, ...callbacks },
	callbacks: { ...(useShopifyDefaults && shopifyDefaults.callbacks), ...callbacks },

This way, we will not have to define shopifyDefaultsValue or maintain the EMPTY_SHOPIFY_DEFAULTS object

[constructor-claude-bedrock]

Code Review Results

✅ Strengths

Comprehensive test coverage with unit tests for all Shopify default functions, proper integration tests verifying prop propagation, and well-structured code with clear separation of concerns.

🚨 Critical Issues

[File: src/utils/shopifyDefaults.ts Line: L12]
The setUrl function only replaces the first occurrence of /group_id due to using String.replace() without a global flag. This could fail if /group_id appears multiple times in the URL (e.g., https://example.com/group_id/foo/group_id/bar).

Fix: Use a global regex replacement:

const modifiedUrl = url.replace(/\/group_id/g, '/collections');

---

[File: src/utils/shopifyDefaults.ts Line: L24-36]
The onAddToCart callback doesn't await or return the fetch promise, making the error handling ineffective. The function returns immediately and any fetch errors are silently caught without proper user feedback.

Fix: Consider returning the promise or providing user feedback:

return fetch('/cart/add.js', {
  method: 'POST',
  headers: { 'Content-Type': 'application/json' },
  body: JSON.stringify({
    id: shopifyId,
    quantity: 1,
  }),
}).catch((error) => {
  console.error('Failed to add item to cart:', error);
  // Consider: throw error or provide user notification
});

⚠️ Important Issues

[File: spec/utils/shopifyDefaults.test.ts Line: L139-153]
The test "should handle fetch errors gracefully" doesn't actually wait for the async operation to complete. The test passes immediately without verifying the error handling behavior since the callback doesn't await the fetch.

Fix: Either make this test async and await the callback, or add a comment explaining this is testing synchronous behavior only:

it('should handle fetch errors gracefully', async () => {
  const mockItem = { /* ... */ } as Item;
  const mockEvent = {} as React.MouseEvent;
  
  fetchMock.mockRejectedValueOnce(new Error('Network error'));
  const consoleSpy = jest.spyOn(console, 'error').mockImplementation();

  shopifyDefaults.callbacks.onAddToCart?.(mockEvent, mockItem);
  
  // Wait for promise to be rejected
  await new Promise(resolve => setTimeout(resolve, 0));
  
  expect(consoleSpy).toHaveBeenCalledWith('Failed to add item to cart:', expect.any(Error));
  consoleSpy.mockRestore();
});

---

[File: src/utils/shopifyDefaults.ts Line: L1-46]
Missing JSDoc comments. The PR checklist indicates "I have added JSDoc comments to my TypeScript definitions" but the shopifyDefaults export and its methods lack documentation explaining their purpose, parameters, and behavior.

Fix: Add JSDoc comments:

/**
 * Shopify-specific default configurations for callbacks and URL helpers.
 * These defaults enable seamless integration with Shopify stores.
 */
export const shopifyDefaults: ShopifyDefaults = {
  urlHelpers: {
    /**
     * Sets the browser URL, converting group_id paths to Shopify collections format.
     * @param url - The URL to navigate to
     */
    setUrl(url: string) { /* ... */ }
  },
  callbacks: {
    /**
     * Adds a product to the Shopify cart via the cart API.
     * @param _event - The click event (unused)
     * @param item - The product item to add
     * @param selectedVariation - Optional product variation
     */
    onAddToCart(_event: React.MouseEvent, item: Item, selectedVariation?: Variation) { /* ... */ }
  }
};

---

[File: src/utils/shopifyDefaults.ts Line: L22]
The fallback logic item.data?.__shopify_id || selectedVariation?.variationId || item.itemId may not work correctly for all Shopify stores. If selectedVariation.variationId is present, it should take precedence over item.data.__shopify_id since the user explicitly selected a variation.

Recommendation: Reorder the fallback logic:

const shopifyId = selectedVariation?.variationId || item.data?.__shopify_id || item.itemId;

💡 Suggestions

[File: src/utils/shopifyDefaults.ts Line: L24-36]
Consider extracting the cart API endpoint as a constant for easier configuration and testing:

const SHOPIFY_CART_ADD_ENDPOINT = '/cart/add.js';

// In the callback:
fetch(SHOPIFY_CART_ADD_ENDPOINT, { /* ... */ })

---

[File: spec/utils/shopifyDefaults.test.ts Line: L85-88]
The test assertions could be more specific by checking the fetch call was made exactly once:

expect(fetchMock).toHaveBeenCalledTimes(1);

---

[File: src/stories/components/CioPlp/CioPlpProps.md Line: L1-213]
This appears to be newly created documentation (213 lines added), but most of it duplicates existing interface documentation. Consider whether this documentation file is necessary or if it should only document the new useShopifyDefaults prop (lines 1-15 are sufficient).

---

[File: src/hooks/useCioPlpProvider.ts Line: L36,39]
The spread operator with conditional object creates an empty object when useShopifyDefaults is false. While this works, it's slightly inefficient. Consider using a ternary for clarity:

callbacks: {
  ...(useShopifyDefaults ? shopifyDefaults.callbacks : {}),
  ...callbacks
},

Though the current implementation is acceptable.

Overall Assessment: ⚠️ Needs Work

The implementation is well-structured with excellent test coverage, but has critical issues with the URL replacement logic and async error handling that should be addressed before merging.

[constructor-claude-bedrock]

Code Review Results

✅ Strengths

Well-structured feature implementation with comprehensive test coverage and clear documentation.

🚨 Critical Issues

[File: src/utils/shopifyDefaults.ts Line: L14]
The setUrl function uses .replace('/group_id', '/collections'), which only replaces the first occurrence of the substring. This could cause issues if /group_id appears multiple times in the URL or if it appears in query parameters.

Recommendation: Use a more robust replacement approach or use .replaceAll():

const modifiedUrl = url.replaceAll('/group_id', '/collections');

Alternatively, if you only want to replace it in the pathname (not in query strings), use a regex with proper constraints:

const urlObj = new URL(url);
urlObj.pathname = urlObj.pathname.replace(/\/group_id(\/|$)/g, '/collections$1');
window.location.href = urlObj.href;

---

[File: src/bundled.jsx Line: L13]
The logic containerSelector ? document.querySelector(containerSelector) : null means that when useShopifyDefaults is true but no selector is provided, it correctly gets the default selector. However, when useShopifyDefaults is false and no selector is provided, containerSelector becomes undefined, which leads to containerElement = null, and an error is logged. This is inconsistent with the behavior before this PR.

Issue: Before this PR, if no selector was provided, the code would attempt document.querySelector(undefined), which would throw an error earlier. Now it's caught by the if (!containerElement) check, but the error message doesn't clarify that a selector is required when useShopifyDefaults is false.

Recommendation: Improve the error message to be more helpful:

if (!containerElement) {
  console.error(
    `CioPlp: No elements found. ${
      !containerSelector
        ? 'Please provide a selector or enable useShopifyDefaults to use the default selector.'
        : `No elements found for selector: ${containerSelector}`
    }`
  );
  return;
}

⚠️ Important Issues

[File: src/utils/shopifyDefaults.ts Lines: L27-37]
The onAddToCart callback makes a fetch request but doesn't provide any user feedback on success or failure. While the error is logged to the console (L36), users won't know if the item was added to their cart.

Recommendation: Consider returning the promise or providing a way for consumers to handle success/error states:

onAddToCart(_event: React.MouseEvent, item: Item, selectedVariation?: Variation) {
  const shopifyId = item.data?.__shopify_id || selectedVariation?.variationId || item.itemId;

  if (typeof window !== 'undefined') {
    return fetch('/cart/add.js', {
      method: 'POST',
      headers: { 'Content-Type': 'application/json' },
      body: JSON.stringify({
        id: shopifyId,
        quantity: 1,
      }),
    })
      .then((response) => {
        if (!response.ok) {
          throw new Error(`HTTP error! status: ${response.status}`);
        }
        return response.json();
      })
      .catch((error) => {
        console.error('Failed to add item to cart:', error);
        throw error; // Re-throw so consumers can handle it
      });
  }
  return Promise.resolve();
}

---

[File: spec/utils/shopifyDefaults.test.ts Line: L42]
The test "should replace /group_id with /collections in URL" passes, but it doesn't test edge cases where /group_id might appear multiple times or in different contexts.

Recommendation: Add test cases for:

• URLs with multiple /group_id occurrences
• URLs with /group_id in query parameters
• URLs with /group_id not as a path segment (e.g., /my-group_id/)

💡 Suggestions

[File: src/utils/shopifyDefaults.ts Line: L22]
The parameter prefix _event indicates the event parameter is unused. Consider if there are any event-related actions that should be performed (like preventDefault() or stopPropagation()). If not, this is acceptable, but worth documenting why the event is included in the signature.

---

[File: spec/utils/shopifyDefaults.test.ts Line: L141-155]
The test "should handle fetch errors gracefully" verifies that the function doesn't throw, but it doesn't verify that the error was logged. Consider adding:

const consoleSpy = jest.spyOn(console, 'error').mockImplementation();
// ... existing test code ...
await new Promise(resolve => setTimeout(resolve, 0)); // Wait for promise to reject
expect(consoleSpy).toHaveBeenCalledWith('Failed to add item to cart:', expect.any(Error));
consoleSpy.mockRestore();

---

[File: README.md Line: L56]
Minor documentation issue: useShopifyDefaults=true should be useShopifyDefaults={true} for JSX boolean props. While useShopifyDefaults alone would work (as a boolean attribute), the example should show the explicit boolean syntax for clarity.

---

[General]
Consider exporting the ShopifyDefaults type from the main types file or the utils index for consumers who might want to reference it or create custom configurations that match this shape.

---

[File: src/hooks/useCioPlpProvider.ts Lines: L36, L39]
The spread operator conditionally includes Shopify defaults. While this works, the order matters for proper overriding. Consider adding a code comment to clarify the precedence:

callbacks: {
  // Order: Shopify defaults (if enabled) < custom callbacks
  ...(useShopifyDefaults && shopifyDefaults.callbacks),
  ...callbacks,
},

Overall Assessment: ⚠️ Needs Work

The implementation is well-tested and follows good patterns, but the URL replacement logic in shopifyDefaults.ts:14 needs to be fixed to handle edge cases properly. The error message improvement in bundled.jsx would also significantly enhance the developer experience. Once these issues are addressed, this will be a solid feature addition.

[HHHindawy]

LGTM!

[constructor-claude-bedrock]

Code Review Results

✅ Strengths

Well-structured feature implementation with comprehensive test coverage, clear documentation, and proper TypeScript type definitions.

🚨 Critical Issues

None

⚠️ Important Issues

[File: src/utils/shopifyDefaults.ts Line: 14]
The setUrl function uses a simple string replacement that could cause unintended replacements if "/group_id" appears elsewhere in the URL (e.g., in query parameters or as part of another path segment). Consider using a more robust replacement:

const modifiedUrl = url.replace(/\/group_id\//, '/collections/');

Or even better, parse the URL and replace the specific path segment:

const urlObj = new URL(url);
urlObj.pathname = urlObj.pathname.replace(/\/group_id(\/|$)/, '/collections$1');
window.location.href = urlObj.href;

[File: src/utils/shopifyDefaults.ts Lines: 27-38]
The onAddToCart callback lacks proper error handling feedback to the user. While the error is logged to the console, there's no visual feedback or retry mechanism. Consider:

• Showing a user-facing error message (toast notification, alert, etc.)
• Implementing retry logic for transient network failures
• Returning a promise or status indicator so consumers can handle failures

[File: src/bundled.jsx Line: 17]
The error message "There were no elements found for the provided selector" is misleading when useShopifyDefaults is true but no selector is explicitly provided. It should indicate that the default selector #cio-plp-ui-container was used:

console.error(`CioPlp: There were no elements found for the selector: ${containerSelector}`);

💡 Suggestions

[File: src/utils/shopifyDefaults.ts Lines: 22-39]
Consider making the fetch error handling more robust by distinguishing between network errors and API errors (4xx/5xx responses). You could check the response status and provide more specific error messages.

[File: spec/utils/shopifyDefaults.test.ts Lines: 140-152]
The test "should handle fetch errors gracefully" doesn't verify that the error was actually caught and logged. Consider using jest.spyOn(console, 'error') to verify the error logging behavior.

[File: src/hooks/useCioPlpProvider.ts Lines: 36-40]
The object spread pattern { ...(useShopifyDefaults && shopifyDefaults.callbacks), ...callbacks } is clever but could be more explicit for readability. Consider extracting this logic into a helper function or adding a comment explaining the precedence order.

[File: README.md Lines: 66-77]
Consider adding a troubleshooting section for common Shopify integration issues, such as:

• What to do if the cart API returns errors
• How to customize the cart behavior
• Debugging tips for URL transformation issues

[General]
The feature implementation follows Shopify's conventions well. Consider documenting whether this has been tested with Shopify's theme editor and preview mode, as those environments sometimes have special behaviors.

Overall Assessment: ⚠️ Needs Work

The implementation is solid with good test coverage and documentation. However, the URL replacement logic could cause edge case bugs, and error handling for the cart API should provide user feedback. These issues should be addressed before merging.