Fix PDP sale prices, variant image switching, and OG image - #97
Merged
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
WalkthroughProduct media and variant display logic is refactored to use product-level media with variant-aware gallery highlighting via a new Changes
Sequence DiagramsequenceDiagram
participant User as User<br/>(selects variant)
participant PD as ProductDetails
participant MI as Memoized<br/>variantImageIndex
participant MG as MediaGallery
participant UI as Gallery UI
User->>PD: Select variant (selectedVariant changes)
PD->>MI: Compute index in product.media<br/>matching variant_ids
MI-->>PD: variantImageIndex: number | null
PD->>MG: Pass activeIndex={variantImageIndex}
MG->>MG: useEffect watches activeIndex
alt activeIndex is non-null
MG->>MG: setSelectedIndex(activeIndex)
MG->>MG: Clear mainImageErrorUrl
end
MG->>UI: Render gallery with highlighted image
UI-->>User: Display variant's media item highlighted
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/products/MediaGallery.tsx`:
- Around line 24-35: selectedIndex is always initialized to 0 and the useEffect
only updates it when activeIndex != null, which causes the gallery to show an
incorrect image or retain the previous variant image when activeIndex is null;
change the initialization to derive from activeIndex (e.g., useState(activeIndex
?? 0)) and update the effect to setSelectedIndex(activeIndex ?? 0) and reset
mainImageErrorUrl via setMainImageErrorUrl(null) so the gallery correctly
initializes and resets whenever activeIndex changes (refer to selectedIndex,
setSelectedIndex, activeIndex, useEffect, and setMainImageErrorUrl).
In `@src/lib/metadata/product.ts`:
- Around line 38-50: The ogImage object currently hardcodes width: 1200 and
height: 630 while using the /_next/image proxy (variables: firstMedia, rawOgUrl,
ogImage, storeUrl); remove the hardcoded width/height when building the proxy
URL so the metadata only includes url and alt for proxied images (i.e., when
ogImage is created from `${storeUrl}/_next/image?...` leave out width/height).
To provide a true 1200×630 image for social previews, add a dedicated
opengraph-image route (e.g., app/products/[slug]/opengraph-image.tsx) that
exports size = {width:1200,height:630}, contentType = "image/png" and a default
Image component using ImageResponse to render the exact 1200×630 image, then
point metadata to that route when available so metadata can safely include
width/height matching the served image.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c4f9bdb3-c22d-4b8e-ba67-3fdd8a4e638e
📒 Files selected for processing (5)
src/app/[country]/[locale]/(storefront)/products/[slug]/ProductDetails.tsxsrc/components/products/MediaGallery.tsxsrc/components/products/ProductCard.tsxsrc/lib/data/cached.tssrc/lib/metadata/product.ts
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/components/products/MediaGallery.tsx (1)
30-35:⚠️ Potential issue | 🟠 MajorThe
nullcase is still not handled in theuseEffect.The initialization was fixed, but the
useEffectstill skips thenullcase. When a user switches from a variant with assigned media to one without (whereactiveIndexbecomesnull), the gallery retains the previous variant's image index instead of resetting to 0.Proposed fix
useEffect(() => { - if (activeIndex != null) { - setSelectedIndex(activeIndex); - setMainImageErrorUrl(null); - } + setSelectedIndex(activeIndex ?? 0); + setMainImageErrorUrl(null); }, [activeIndex]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/products/MediaGallery.tsx` around lines 30 - 35, useEffect currently only handles activeIndex != null, so when activeIndex becomes null the gallery keeps the previous selected index; update the effect that references activeIndex (the useEffect containing setSelectedIndex and setMainImageErrorUrl) to explicitly handle the null case by calling setSelectedIndex(0) and clearing setMainImageErrorUrl(null) when activeIndex is null, while preserving the existing branch when activeIndex is a number.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/components/products/MediaGallery.tsx`:
- Around line 30-35: useEffect currently only handles activeIndex != null, so
when activeIndex becomes null the gallery keeps the previous selected index;
update the effect that references activeIndex (the useEffect containing
setSelectedIndex and setMainImageErrorUrl) to explicitly handle the null case by
calling setSelectedIndex(0) and clearing setMainImageErrorUrl(null) when
activeIndex is null, while preserving the existing branch when activeIndex is a
number.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 88c0f91a-8b35-43a7-ba08-afe6478ae8dc
📒 Files selected for processing (2)
src/components/products/MediaGallery.tsxsrc/lib/metadata/product.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/metadata/product.ts
- Fix strikethrough price not showing on PDP and ProductCard when original_price is null (compare-at price sales) - Show all product images in gallery and auto-switch to variant's image when selecting a variant with assigned media - Add variants.media to product expand for variant image data - Use single OG image proxied through Next.js image optimization Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Initialize selectedIndex from activeIndex prop instead of always 0 - Use Spree's og_image_url (pre-resized 1200x630) for OG image source - Only include width/height in metadata when og_image_url is available Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Match variant images via media.variant_ids.includes(selectedVariant.id) instead of expanding variants.media — product media already contains variant_ids for matching. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Allow next/image to load images from localhost and *.localhost for local development with Spree backend. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Cichorek
force-pushed
the
fix/pdp-sale-prices-variant-images-og
branch
from
April 1, 2026 14:48
dd96947 to
b8102ff
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
next.config.ts (1)
34-43: Remove the duplicatedlocalhostremote pattern.The entry on Lines 34-38 is identical to the existing one on Lines 19-23, so it does not change the allowlist. Keeping only the new
**.localhostrule makes the intent clearer.♻️ Proposed cleanup
{ protocol: "https", hostname: "**.spree.sh", pathname: "/rails/active_storage/**", }, - { - protocol: "http", - hostname: "localhost", - pathname: "/rails/active_storage/**", - }, { protocol: "http", hostname: "**.localhost", pathname: "/rails/active_storage/**", },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@next.config.ts` around lines 34 - 43, There are two identical remotePatterns entries using protocol: "http", hostname: "localhost", pathname: "/rails/active_storage/**"; remove the duplicate plain "localhost" object and keep the intended wildcard entry with hostname: "**.localhost" inside the remotePatterns array in next.config.ts so the allowlist only contains the wildcard rule; update the array to contain just the single /**.localhost*/ entry.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@next.config.ts`:
- Around line 34-43: There are two identical remotePatterns entries using
protocol: "http", hostname: "localhost", pathname: "/rails/active_storage/**";
remove the duplicate plain "localhost" object and keep the intended wildcard
entry with hostname: "**.localhost" inside the remotePatterns array in
next.config.ts so the allowlist only contains the wildcard rule; update the
array to contain just the single /**.localhost*/ entry.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1e8274d8-700b-409e-ada7-51c6e57d32d6
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
next.config.tssrc/app/[country]/[locale]/(storefront)/products/[slug]/ProductDetails.tsxsrc/components/products/MediaGallery.tsxsrc/components/products/ProductCard.tsxsrc/lib/metadata/product.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/components/products/ProductCard.tsx
- src/components/products/MediaGallery.tsx
- src/lib/metadata/product.ts
12 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
original_priceis null (compare-at price sales)variants.mediato product page expand for variant image data from the APITest plan
og:imagemeta tag — should be single image, proxied through/_next/imagewhenSTORE_URLis set🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Improvements