Add wishlist support - #184
Conversation
|
@Abdul-Haseeb-Kamboh is attempting to deploy a commit to the Spree Commerce Team on Vercel. A member of the Team first needs to authorize it. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds wishlist persistence, client state, storefront controls, account pages, navigation links, localized messages, unit tests, and end-to-end coverage for adding, viewing, and removing wishlist items. ChangesWishlist feature
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Shopper
participant ProductDetails
participant WishlistButton
participant WishlistContext
participant wishlistData
Shopper->>ProductDetails: Select product variant
ProductDetails->>WishlistButton: Render selected variantId
Shopper->>WishlistButton: Click add to wishlist
WishlistButton->>WishlistContext: addItem(variantId, 1)
WishlistContext->>wishlistData: addWishlistItem()
wishlistData-->>WishlistContext: Updated wishlist
WishlistContext-->>WishlistButton: Updated membership and count
WishlistButton-->>Shopper: Show active wishlist control
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
src/app/[country]/[locale]/(storefront)/account/wishlist/page.tsx (2)
1-5: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the new dynamic routes’ rendering contracts explicit.
As per coding guidelines:
src/app/[country]/[locale]/(storefront)/account/wishlist/page.tsx#L1-L5: addgenerateMetadataand explicitly choosedynamic = "force-dynamic"orgenerateStaticParams.src/app/[country]/[locale]/(storefront)/wishlist/page.tsx#L3-L14: explicitly choosedynamic = "force-dynamic"orgenerateStaticParamsfor the redirect route.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/`[country]/[locale]/(storefront)/account/wishlist/page.tsx around lines 1 - 5, Update src/app/[country]/[locale]/(storefront)/account/wishlist/page.tsx lines 1-5 to add generateMetadata and explicitly configure either dynamic = "force-dynamic" or generateStaticParams. Update src/app/[country]/[locale]/(storefront)/wishlist/page.tsx lines 3-14 to explicitly choose dynamic = "force-dynamic" or generateStaticParams for the redirect route.Source: Coding guidelines
3-5: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd explicit return types to the new functions.
As per coding guidelines, these new functions should not rely on inferred return types.
src/app/[country]/[locale]/(storefront)/account/wishlist/page.tsx#L3-L5: annotateAccountWishlistPage.src/app/[country]/[locale]/(storefront)/wishlist/page.tsx#L10-L15: annotate the async redirect page asPromise<never>.src/components/layout/WishlistNavButton.tsx#L11-L29: annotate the component return type.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/`[country]/[locale]/(storefront)/account/wishlist/page.tsx around lines 3 - 5, Annotate the return types of the three new functions: set AccountWishlistPage in src/app/[country]/[locale]/(storefront)/account/wishlist/page.tsx to its explicit component return type, set the async redirect page in src/app/[country]/[locale]/(storefront)/wishlist/page.tsx to Promise<never>, and annotate the component return type in src/components/layout/WishlistNavButton.tsx.Source: Coding guidelines
src/components/wishlist/__tests__/WishlistButton.test.tsx (1)
59-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for the authenticated remove click path.
This test only asserts the "Remove from wishlist" label renders; it never clicks the button to verify
removeItemByVariantis actually invoked, unlike the parallel add-path test at lines 78-84.✅ Suggested addition
it("shows remove state for variants already in wishlist", () => { mockHasVariant.mockReturnValue(true); render(<WishlistButton variantId="var_1" />); expect( screen.getByRole("button", { name: "Remove from wishlist" }), ).toBeInTheDocument(); }); + + it("removes variant when authenticated and already in wishlist", async () => { + mockHasVariant.mockReturnValue(true); + render(<WishlistButton variantId="var_1" />); + + fireEvent.click(screen.getByRole("button", { name: "Remove from wishlist" })); + + expect(mockRemoveItemByVariant).toHaveBeenCalledWith("var_1"); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/wishlist/__tests__/WishlistButton.test.tsx` around lines 59 - 65, Add click coverage to the existing “shows remove state for variants already in wishlist” test by clicking the rendered Remove from wishlist button and asserting the authenticated remove handler, removeItemByVariant, is called with the expected variant identifier, matching the parallel add-path test.src/app/[country]/[locale]/(storefront)/products/[slug]/ProductDetails.tsx (1)
99-117: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate variant-id fallback logic.
selectedVariantId(114-117) recomputes the exact same fallback chain (selectedVariant?.id || product.default_variant?.id || product.default_variant_id) already inlined inhandleAddToCart(100-103). Hoist to a single derived value and reuse it in both places to avoid the two copies drifting apart.♻️ Suggested change
+ const selectedVariantId = + selectedVariant?.id || + product.default_variant?.id || + product.default_variant_id; + const handleAddToCart = async () => { - const variantId = - selectedVariant?.id || - product.default_variant?.id || - product.default_variant_id; + const variantId = selectedVariantId; if (!variantId) { throw new Error("No variant selected"); } ... }; - - const selectedVariantId = - selectedVariant?.id || - product.default_variant?.id || - product.default_variant_id;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/`[country]/[locale]/(storefront)/products/[slug]/ProductDetails.tsx around lines 99 - 117, Hoist the shared variant ID fallback chain into a single derived value near handleAddToCart, then reuse that value for the addItem call and selectedVariantId-dependent logic. Remove the duplicated inline computation while preserving the existing no-variant error behavior.src/contexts/WishlistContext.tsx (1)
101-103: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
router.refresh()on every wishlist mutation may be unnecessary overhead.
mutateWishlistalready sets localwishliststate from the mutation response, and all wishlist consumers (WishlistButton,ProductCard,WishlistPageContent) read from this context, not from server-rendered data. Callingrouter.refresh()after every add/remove forces a full RSC data refetch for the current route on every toggle, which is likely redundant unless some server component elsewhere depends on wishlist state.♻️ Suggested change
setWishlist(result.wishlist ?? null); - router.refresh(); return true;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/contexts/WishlistContext.tsx` around lines 101 - 103, Remove the router.refresh() call from mutateWishlist after updating local wishlist state with result.wishlist, while preserving the existing state update and successful return behavior. Remove any now-unused router dependency associated with this call.src/lib/data/wishlist.ts (1)
73-73: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueRemove the no-op wishlist invalidation
updateTag("wishlist")doesn’t invalidate anything today because the wishlist reads in this file aren’t cached or tagged. Either addcacheTag("wishlist")to the read path or drop these calls.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/data/wishlist.ts` at line 73, Remove the no-op updateTag("wishlist") calls from the wishlist mutation flow, since the reads in this file are not cached or tagged. Do not add cache tagging unless you also update the corresponding read path to use cacheTag("wishlist").
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@e2e/wishlist.spec.ts`:
- Around line 12-14: Add explicit Promise<void> return types to the async test
callbacks in the wishlist.spec.ts tests, including “guest is redirected to
account when clicking wishlist on PDP” and the other flagged tests at lines
32–34 and 70, while preserving their existing behavior.
In `@src/app/`[country]/[locale]/(storefront)/account/wishlist/page.tsx:
- Around line 3-5: Update the account route protection around
AccountWishlistPage so unauthenticated users can reach WishlistPageContent and
its guest sign-in state, preserving the wishlist path and redirect query for
sign-in; exempt this wishlist route from the generic account-layout redirect
rather than sending guests to /account.
In `@src/components/layout/WishlistNavButton.tsx`:
- Around line 18-24: Update the Link aria-label in WishlistNavButton to expose
itemCount to assistive technology using a localized label that includes the
count, or associate a localized screen-reader-only count with the link while
preserving the visible badge.
In `@src/components/wishlist/WishlistPageContent.tsx`:
- Around line 174-202: Remove the unrelated updating flag from the disabled
condition of the Add to Cart Button in WishlistPageContent. Keep the button
disabled only while another variant is being added or when
item.variant.purchasable is false, preserving the existing addingVariantId
behavior.
In `@src/contexts/__tests__/WishlistContext.test.tsx`:
- Around line 38-53: The wishlist fixtures are incorrectly cast as never,
causing downstream object spreads to fail type checking. In
src/contexts/__tests__/WishlistContext.test.tsx at lines 38-53, cast
wishlistFixture and updatedWishlist around line 81 as unknown as Wishlist; in
src/lib/data/__tests__/wishlist.test.ts at lines 34-67, cast wishlistFixture as
unknown as Wishlist, preserving the existing fixture data.
---
Nitpick comments:
In `@src/app/`[country]/[locale]/(storefront)/account/wishlist/page.tsx:
- Around line 1-5: Update
src/app/[country]/[locale]/(storefront)/account/wishlist/page.tsx lines 1-5 to
add generateMetadata and explicitly configure either dynamic = "force-dynamic"
or generateStaticParams. Update
src/app/[country]/[locale]/(storefront)/wishlist/page.tsx lines 3-14 to
explicitly choose dynamic = "force-dynamic" or generateStaticParams for the
redirect route.
- Around line 3-5: Annotate the return types of the three new functions: set
AccountWishlistPage in
src/app/[country]/[locale]/(storefront)/account/wishlist/page.tsx to its
explicit component return type, set the async redirect page in
src/app/[country]/[locale]/(storefront)/wishlist/page.tsx to
Promise<never>, and annotate the component return type in
src/components/layout/WishlistNavButton.tsx.
In `@src/app/`[country]/[locale]/(storefront)/products/[slug]/ProductDetails.tsx:
- Around line 99-117: Hoist the shared variant ID fallback chain into a single
derived value near handleAddToCart, then reuse that value for the addItem call
and selectedVariantId-dependent logic. Remove the duplicated inline computation
while preserving the existing no-variant error behavior.
In `@src/components/wishlist/__tests__/WishlistButton.test.tsx`:
- Around line 59-65: Add click coverage to the existing “shows remove state for
variants already in wishlist” test by clicking the rendered Remove from wishlist
button and asserting the authenticated remove handler, removeItemByVariant, is
called with the expected variant identifier, matching the parallel add-path
test.
In `@src/contexts/WishlistContext.tsx`:
- Around line 101-103: Remove the router.refresh() call from mutateWishlist
after updating local wishlist state with result.wishlist, while preserving the
existing state update and successful return behavior. Remove any now-unused
router dependency associated with this call.
In `@src/lib/data/wishlist.ts`:
- Line 73: Remove the no-op updateTag("wishlist") calls from the wishlist
mutation flow, since the reads in this file are not cached or tagged. Do not add
cache tagging unless you also update the corresponding read path to use
cacheTag("wishlist").
🪄 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: 775ce688-6fea-4c57-8ca9-c1d8093f5bec
📒 Files selected for processing (26)
e2e/wishlist.spec.tsmessages/de.jsonmessages/en.jsonmessages/es.jsonmessages/fr.jsonmessages/pl.jsonsrc/app/[country]/[locale]/(storefront)/account/layout.tsxsrc/app/[country]/[locale]/(storefront)/account/page.tsxsrc/app/[country]/[locale]/(storefront)/account/wishlist/page.tsxsrc/app/[country]/[locale]/(storefront)/products/[slug]/ProductDetails.tsxsrc/app/[country]/[locale]/(storefront)/wishlist/page.tsxsrc/app/[country]/[locale]/layout.tsxsrc/components/layout/Footer.tsxsrc/components/layout/Header.tsxsrc/components/layout/MobileMenu.tsxsrc/components/layout/WishlistNavButton.tsxsrc/components/products/ProductCard.tsxsrc/components/products/__tests__/ProductCard.test.tsxsrc/components/wishlist/WishlistButton.tsxsrc/components/wishlist/WishlistPageContent.tsxsrc/components/wishlist/__tests__/WishlistButton.test.tsxsrc/contexts/WishlistContext.tsxsrc/contexts/__tests__/WishlistContext.test.tsxsrc/lib/data/__tests__/wishlist.test.tssrc/lib/data/index.tssrc/lib/data/wishlist.ts
| test("guest is redirected to account when clicking wishlist on PDP", async ({ | ||
| page, | ||
| }) => { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add explicit return types to the async test functions.
As per coding guidelines, “Always define explicit return types for functions.”
Proposed fix
-test("guest is redirected to account when clicking wishlist on PDP", async ({
+test("guest is redirected to account when clicking wishlist on PDP", async ({
page,
-}) => {
+}): Promise<void> => {
@@
-test("authenticated user can add and remove a wishlist item", async ({
+test("authenticated user can add and remove a wishlist item", async ({
page,
-}) => {
+}): Promise<void> => {
@@
-async function registerUser(page: Page, email: string, password: string) {
+async function registerUser(
+ page: Page,
+ email: string,
+ password: string,
+): Promise<void> {Also applies to: 32-34, 70-70
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@e2e/wishlist.spec.ts` around lines 12 - 14, Add explicit Promise<void> return
types to the async test callbacks in the wishlist.spec.ts tests, including
“guest is redirected to account when clicking wishlist on PDP” and the other
flagged tests at lines 32–34 and 70, while preserving their existing behavior.
Source: Coding guidelines
| export default function AccountWishlistPage() { | ||
| return <WishlistPageContent />; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve the guest wishlist flow.
This route is nested under the protected account layout, which redirects unauthenticated /account/* paths to /account. As a result, WishlistPageContent’s guest sign-in state and its redirect query are never reached; guests clicking /wishlist, the header link, or footer link land on the generic account page and lose their destination. Exempt this route from the layout redirect or preserve the wishlist URL when redirecting to sign-in.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/`[country]/[locale]/(storefront)/account/wishlist/page.tsx around
lines 3 - 5, Update the account route protection around AccountWishlistPage so
unauthenticated users can reach WishlistPageContent and its guest sign-in state,
preserving the wishlist path and redirect query for sign-in; exempt this
wishlist route from the generic account-layout redirect rather than sending
guests to /account.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| const imageUrl = | ||
| variant.thumbnail_url || | ||
| variant.option_values?.find((value) => Boolean(value.image_url)) | ||
| ?.image_url || | ||
| variant.images?.[0]?.styles?.product || | ||
| variant.images?.[0]?.styles?.large || | ||
| variant.images?.[0]?.styles?.small || | ||
| variant.images?.[0]?.url || | ||
| variant.images?.[0]?.original_url || | ||
| wishlistItem.thumbnail_url || | ||
| itemProduct?.thumbnail_url || | ||
| itemProductAttributes?.thumbnail_url || | ||
| itemProduct?.images?.[0]?.styles?.product || | ||
| itemProduct?.images?.[0]?.styles?.large || | ||
| itemProduct?.images?.[0]?.styles?.small || | ||
| itemProduct?.images?.[0]?.url || | ||
| itemProduct?.images?.[0]?.original_url || | ||
| product?.thumbnail_url || | ||
| productAttributes?.thumbnail_url || | ||
| null; |
There was a problem hiding this comment.
- There's no
imagesproperty, onlymedia - Also you should just ust
primary_media - Also you should just check
variant.thumbnail_url || itemProduct.thumbnail_urlno need for such extended finders
| import { getClient, isAuthError, withAuthRefresh } from "@/lib/spree"; | ||
| import { actionResult } from "./utils"; | ||
|
|
||
| type WishlistItemWithVariantProductId = NonNullable< |
There was a problem hiding this comment.
@Abdul-Haseeb-Kamboh, why not just use the type from SDK https://github.com/spree/spree/blob/main/packages/sdk/src/types/generated/WishlistItem.ts ?
| thumbnail_url?: string | null; | ||
| }; | ||
|
|
||
| type ProductWithPrimaryMedia = { |
There was a problem hiding this comment.
@Abdul-Haseeb-Kamboh same here, also you don't need to fetch the image from primary_media - thumbnail_url is just fine
|
Lets wait for Spree 5.6.1, including this improvement - spree/spree@f62ec82 |
2 participants
Summary
Add wishlist functionality to the storefront.
Problem
The Spree API and SDK already support wishlists, but the storefront did not provide a user interface for managing wishlist items.
Solution
Implemented wishlist support including:
Testing
Tested locally and verified:
Fixes #136
Summary by CodeRabbit
/wishlistroute that redirects to the localized account wishlist.