BRD-1043-Surface-variant-URL-and-price-in-Shopify-search-index

[PauliusInvertus]

Per-variant productUrl_ with ?variant= deep-linking and the
locale prefix, plus price/basePrice/priceTaxExcluded fields. Variant
imageUrl is intentionally omitted so the parent's curated featuredImage
stays the search-row hero.

Indexes Shopify variants with their own locale-prefixed productUrl (using ?variant=) and price fields, so a search like "gloves xl" can deep-link to the matching variant via brad-search's variant enrichment. Variant imageUrl is intentionally left off — merchants want the curated featuredImage to remain the row hero regardless of which variant matched. (Greengoing case featuredImage)

BRD-1043

Screen.Recording.2026-05-07.at.14.40.10.mov

[gemini-code-assist]

Code Review

This pull request enhances the ShopifyAdapter to include localized product URLs and pricing information (price, basePrice) for product variants, ensuring variant-specific deep-linking by appending the variant ID to the URL. It also adds comprehensive tests for these changes, including logic for preserving existing query parameters like preview_key. Review feedback suggests using array_merge instead of the + operator for query parameters to ensure the variant ID correctly overrides existing values and recommends pre-calculating handle translations outside the variant loop to improve performance.

[PauliusInvertus]

@claude review

[claude]

Claude finished @PauliusInvertus's task in 2m 13s —— View job

---

Code Review: BRD-1043 — Surface variant URL and price in Shopify search index

Tasks

• Gather context and understand the request
• Read changed files (ShopifyAdapter.php, ShopifyAdapterTest.php)
• Analyze implementation correctness and edge cases
• Review test coverage
• Post comprehensive review feedback

---

The overall approach is solid — the design decision to omit imageUrl from variants is well-reasoned and documented, the http_build_query usage is correct, and the test suite is meaningfully expanded. Two bugs and one missing test case need attention before this merges.

---

🔴 Bug: variant query param can be silently ignored

src/Adapters/ShopifyAdapter.php:499

// Existing query (e.g. preview_key) wins on key collision.
$mergedQuery = $existingQuery + $extraQuery;

The PHP + operator gives priority to left-hand keys. If a product's onlineStoreUrl already contains ?variant=111 (some Shopify themes set a default variant in the canonical URL), the variant deep-link (?variant=12345) in $extraQuery is silently dropped. Every indexed variant for that product would then point to variant 111, regardless of which variant is actually being indexed.

The comment "Existing query (e.g. preview_key) wins on key collision" documents exactly the wrong rule. preview_key is preserved because it's not present in $extraQuery at all — not because of key-collision priority. The fix is:

// $extraQuery (variant ID) must win on collision; $existingQuery fills in other params (e.g. preview_key).
$mergedQuery = array_merge($existingQuery, $extraQuery);

The existing test testVariantUrlPreservesPreviewKeyAlongsideVariantParam doesn't catch this because it only uses a URL with ?preview_key=abc123 — no variant collision exists there, so both + and array_merge produce identical output. A test with a base URL of ?variant=old_id would expose the bug.

Fix this →

---

🟡 Performance: Handle translation re-resolved on every variant × locale

src/Adapters/ShopifyAdapter.php:729-735

foreach ($locales as $locale) {
    $localeTranslations = $this->resolveTranslationsForLocale($locale, $primaryLocale, $translations);
    $url = $this->buildLocaleProductUrl(
        ...
        $this->translated($localeTranslations, 'handle'),
        ...
    );
}

$translations is product-level data — the translated handle for locale lt is the same for every variant of the product. With N variants and L locales, resolveTranslationsForLocale + translated(..., 'handle') run N×L times doing redundant work. For a product with 50 variants and 5 locales that's 250 calls instead of 5.

Pre-calculate the locale→translated-handle map once before the array_map closure:

// Pre-calculate once per product, not once per variant×locale
$localeHandles = [];
foreach ($locales as $locale) {
    $lt = $this->resolveTranslationsForLocale($locale, $primaryLocale, $translations);
    $localeHandles[$locale] = $this->translated($lt, 'handle');
}

return array_map(function (array $edge) use ($locales, $productUrl, $defaultHandle, $primaryLocale, $localeHandles): array {
    ...
    foreach ($locales as $locale) {
        $url = $this->buildLocaleProductUrl(
            $productUrl, $defaultHandle, $localeHandles[$locale], $locale, $primaryLocale, ['variant' => $variantId],
        );
        ...
    }
}, $variantsData['edges']);

Fix this →

---

🟡 Missing test: compareAtPrice = "0.00" should fall back to price

src/Adapters/ShopifyAdapter.php:711

$basePrice = ($compareAt !== '' && bccomp($compareAt, '0.00', 2) > 0) ? $compareAt : $price;

The bccomp guard is intentionally excluding compareAtPrice: "0.00" (Shopify sets this when no compare-at is configured), but there's no test covering it. A test with compareAtPrice: "0.00" should assert that basePrice equals price, not "0.00". Without the test, a future refactor of this logic has no safety net.

---

🟡 is_numeric guard missing before bccomp

src/Adapters/ShopifyAdapter.php:707-711

$compareAt = isset($variant['compareAtPrice']) && is_scalar($variant['compareAtPrice'])
    ? (string) $variant['compareAtPrice']
    : '';
$basePrice = ($compareAt !== '' && bccomp($compareAt, '0.00', 2) > 0) ? $compareAt : $price;

is_scalar accepts strings like "N/A" or "$99.95". In PHP 8.x, bccomp throws a ValueError on non-numeric strings. If Shopify returns a malformed compareAtPrice, this would crash variant transformation entirely. Change is_scalar to is_numeric (which also handles numeric strings and floats):

$compareAt = isset($variant['compareAtPrice']) && is_numeric($variant['compareAtPrice'])
    ? (string) $variant['compareAtPrice']
    : '';

---

✅ What's working well

• The intentional omission of imageUrl from variants is correctly documented in both the code docblock and the PR description — clear rationale for future maintainers.
• Six new test cases covering the main paths: localized URL, no-locale URL, preview_key preservation, price/basePrice, compareAtPrice: null fallback, and no imageUrl on variant.
• http_build_query instead of manual string concatenation for query params is the right call.
• The sku defensive guard (is_string($variant['sku'] ?? null)) is a nice improvement over the old ?? '' that would silently accept non-string SKUs.
• ext-bcmath is already declared in composer.json — no missing dependency.

---

[PauliusInvertus]

@claude review

[claude]

Claude finished @PauliusInvertus's task in 2m 23s —— View job

---

Code Review: BRD-1043 — Surface variant URL and price in Shopify search index

Tasks

• Gather context and understand the request
• Read changed files (ShopifyAdapter.php, ShopifyAdapterTest.php)
• Analyze implementation correctness and edge cases
• Review test coverage
• Post comprehensive review feedback

---

The implementation is solid and all issues raised in the previous review round have been addressed in commit e72c816. Two minor observations remain, but neither is a blocker.

---

✅ Previous feedback — all addressed

Issue	Status
+ operator silently dropped variant ID on key collision	✅ Fixed — array_merge now used (line 501)
Locale handles re-resolved on every variant × locale	✅ Fixed — $localeHandles map pre-calculated before array_map (lines 695-701)
is_scalar would pass "N/A" into bccomp	✅ Fixed — is_numeric guard used (lines 717-720)
No test for compareAtPrice: "0.00" fallback	✅ Fixed — testVariantBasePriceFallsBackToPriceWhenCompareAtIsZero added
No test for variant ID overriding an existing ?variant= in base URL	✅ Fixed — testVariantUrlOverridesDefaultVariantParamFromBaseUrl added

---

🟡 Minor: basePriceTaxExcluded set even when basePrice === price

src/Adapters/ShopifyAdapter.php:732-735

if ($basePrice !== '') {
    $result['basePrice'] = $basePrice;
    $result['basePriceTaxExcluded'] = $basePrice;
}

When no compareAtPrice is set, $basePrice falls back to $price, so price, priceTaxExcluded, basePrice, and basePriceTaxExcluded all hold the same value. This may be intentional (the downstream API ignores duplicates), but if the search engine uses basePrice > price to decide whether to render a strikethrough, a basePrice === price scenario could trigger a false "sale" badge.

Worth a clarifying comment, or a guard like if ($basePrice !== '' && $basePrice !== $price) — whichever matches the intended API semantics.

---

🟡 Minor: fallback test cases omit priceTaxExcluded / basePriceTaxExcluded

testVariantBasePriceFallsBackToPriceWhenCompareAtIsZero and testVariantBasePriceFallsBackToPriceWhenCompareAtMissing assert price and basePrice but not the ...TaxExcluded sibling fields. If a future change accidentally decouples those, the tests won't catch it.

---

✅ What's working well

• All four issues from the prior review have been cleanly addressed with no over-engineering.
• Eight new test cases cover the main paths thoroughly: localized URL, no-locale URL, preview_key preservation, variant ID override, price/basePrice, compareAtPrice: null fallback, compareAtPrice: "0.00" fallback, and the deliberate imageUrl omission.
• The $localeHandles pre-calculation comment explains the why, not just the what — good for future maintainers.
• No-locale path (lines 754-760) correctly passes $primaryLocale for both $locale and $primaryLocale to ensure no locale prefix is injected — subtle but correct.
• Intentional omission of imageUrl from variants is documented in both the method docblock and the test — the decision is clearly motivated and won't be accidentally "fixed" later.

---