Complete migration to @spree/next package - #7
Conversation
Migrate all server-side data layer from direct @spree/sdk usage to @spree/next, which provides Server Actions for authentication, cart, checkout, and customer operations with built-in cookie-based auth and token refresh. Changes: - Add @spree/next@0.1.1 as dependency and configure transpilePackages - Delete src/lib/spree.ts (client initialization now internal to @spree/next) - Delete src/lib/data/auth-request.ts (token refresh now internal to @spree/next) - Simplify src/lib/data/cookies.ts to only isAuthenticated() check - Rewrite all data layer files to delegate to @spree/next Server Actions - Fix React 19 hook ordering in CheckoutPage (use() before other hooks) - Fix nested form issue in AddressStep component Result: Reduced data layer from 867 to 252 lines while maintaining all functionality. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughCentralizes data access by replacing local Spree client and auth utilities with Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/checkout/AddressStep.tsx (1)
155-161:⚠️ Potential issue | 🟠 Major
ship_address_idis never sent when a saved address is selected.The
onSubmitprop acceptsship_address_id?: string(line 24), buthandleSubmitalways sends the fullship_addressobject. When an authenticated user picks an existing saved address viaAddressSelector, it would be more correct (and efficient) to passship_address_idinstead. Currently, the server receives a copy of the address fields rather than a reference to the saved address, which may create a duplicate address or bypass address-book linkage on the backend.Proposed fix
You'd need to track whether a saved address was selected (e.g., store
selectedSavedAddressId) and branch inhandleSubmit:const handleSubmit = (e: React.FormEvent) => { e.preventDefault(); + // If a saved address is selected, send its ID instead of duplicating the data + if (selectedSavedAddressId && selectedSavedAddressId !== "new") { + onSubmit({ + email, + ship_address_id: selectedSavedAddressId, + }); + return; + } onSubmit({ email, ship_address: formDataToAddress(shipAddress), }); };This requires lifting or sharing the
selectedAddressIdstate fromAddressSelector, or adding a callback prop.src/lib/data/gift-cards.ts (1)
5-19:⚠️ Potential issue | 🟡 MinorAdd explicit return types to both exported functions.
The coding guidelines require explicit return types for all functions. Both
getGiftCardsandgetGiftCardlack them, which violates the strict TypeScript requirement.For
getGiftCards, the return type should bePromise<{ data: GiftCard[] }>based on the SDK and usage in page.tsx (line 207-208). ForgetGiftCard, it should bePromise<GiftCard | null>to match the error handling (returningnullon failure).Define these types explicitly, or import concrete types from
@spree/nextif available:-export async function getGiftCards() { +export async function getGiftCards(): Promise<{ data: unknown[] }> { try { return await listGiftCards(); } catch { return { data: [] }; } } -export async function getGiftCard(id: string) { +export async function getGiftCard(id: string): Promise<unknown | null> { try { return await _getGiftCard(id); } catch { return null; } }As per coding guidelines: "Use strict TypeScript type checking; define explicit return types for functions and avoid 'any' type."
🤖 Fix all issues with AI agents
In `@package.json`:
- Line 14: Remove the invalid dependency entry "@spree/next": "^0.1.1" from
package.json (or replace it with the correct published package name if you
intended a different Spree package such as "@spree/dashboard" or
"@spree/storefront-api-v2-sdk"); verify there are no imports of "@spree/next" in
the codebase and then run npm install to ensure the install succeeds after
removing or correcting the package name.
In `@src/lib/data/cart.ts`:
- Around line 25-38: The three mutation helpers addToCart, updateCartItem, and
removeCartItem are inconsistent with associateCartWithUser: they throw on errors
instead of returning a structured { success, error } result and also risk
surfacing a fetch error after a successful mutation; wrap each function
(addToCart, updateCartItem, removeCartItem) in a try/catch that calls the
underlying mutation (addItem/updateItem/removeItem) and then attempts _getCart —
on success return { success: true, cart }, on mutation failure return { success:
false, error }, and if the mutation succeeds but _getCart fails return {
success: true, cart: null, error: fetchError } (or include both mutation and
fetch error fields) so callers get a consistent structured result like
associateCartWithUser.
In `@src/lib/data/credit-cards.ts`:
- Around line 8-14: Add an explicit return type to getCreditCards to match the
shape returned by listCreditCards (or a custom interface that includes at least
the data array) so callers get proper typing; update the function signature of
getCreditCards to declare that return type, import the appropriate type from the
Spree SDK if available (or define a local interface), and ensure the error
fallback ({ data: [] }) conforms to that type so the implementation and
signature remain consistent with listCreditCards.
🧹 Nitpick comments (13)
src/lib/data/customer.ts (2)
11-29: Missing explicit return types on exported functions.Per coding guidelines, functions should have explicit return types. These Server Actions are part of the public API surface and would benefit from declared return types for type safety and documentation.
♻️ Add explicit return types
-export async function getCustomer() { +export async function getCustomer(): ReturnType<typeof _getCustomer> { return _getCustomer(); } -export async function login(email: string, password: string) { +export async function login(email: string, password: string): ReturnType<typeof _login> { return _login(email, password); } -export async function register( +export async function register( email: string, password: string, passwordConfirmation: string, -) { +): ReturnType<typeof _register> { return _register(email, password, passwordConfirmation); } -export async function logout() { +export async function logout(): ReturnType<typeof _logout> { return _logout(); }Alternatively, import and use the concrete types from
@spree/nextif available.As per coding guidelines: "Use strict TypeScript type checking; define explicit return types for functions and avoid 'any' type".
31-44: Inconsistent error handling across functions.
updateCustomerwraps the call in try/catch and returns a{ success, customer | error }result object, whilegetCustomer,login,register, andlogoutlet exceptions propagate. If this is intentional (e.g., the others rely on error boundaries or@spree/nextreturning error states), a brief comment would help future maintainers understand the distinction.src/components/checkout/AddressStep.tsx (2)
133-153: useEffect for state fetching triggered by state change — consider moving to event handler.This
useEffectfetches states whenevershipAddress.country_isochanges. The coding guideline says to avoiduseEffectfor data fetching triggered by state changes, preferring event handlers instead. The fetch could be initiated directly insideupdateShipAddresswhenfield === "country_iso".Sketch
const updateShipAddress = (field: keyof AddressFormData, value: string) => { setShipAddress((prev) => { const updated = { ...prev, [field]: value }; if (field === "country_iso") { updated.state_abbr = ""; updated.state_name = ""; } return updated; }); + if (field === "country_iso") { + if (!value) { + setShipStates([]); + return; + } + startTransitionShip(() => { + fetchStates(value).then(setShipStates); + }); + } };Then remove the
useEffectat lines 133-153. Note: you'd still need to handle the initial load fororder.ship_address.country_iso— a one-time fetch on mount (or computing initial states server-side) would cover that.As per coding guidelines: "Avoid using useEffect for data fetching triggered by state changes; use event handlers or Server Actions instead".
633-841: Significant duplication betweenAddressFormandAddressSelector's inline form.The local
AddressFormcomponent (lines 633-841) renders nearly identical fields to the form embedded insideAddressSelector(see relevant snippet, lines ~160-380). Both render the same first name, last name, company, address lines, city, country, state, zip, and phone fields with the same markup. Consider havingAddressSelectorconsumeAddressForminternally to eliminate the duplication.src/lib/data/countries.ts (1)
5-10: Add explicit return types to exported functions.
getCountries()andgetCountry()should declare their return types explicitly. Withstrict: truein tsconfig, this aligns with the project's TypeScript guidelines and guards against unintended type changes from@spree/nextupdates. Seesrc/lib/data/cookies.tsfor the pattern in use (e.g.,Promise<boolean>).export async function getCountries(options?: { locale?: string; currency?: string; }): Promise</* return type from `@spree/next` */> { return listCountries(options); } export async function getCountry( iso: string, options?: { locale?: string; currency?: string }, ): Promise</* return type from `@spree/next` */> { return _getCountry(iso, options); }src/lib/data/checkout.ts (2)
15-21: Consider adding explicit return types to exported Server Action functions.Per coding guidelines, functions should have explicit return types. The inferred types work but explicit annotations improve discoverability and catch accidental return shape changes.
For example:
export async function getCheckoutOrder(orderId: string): Promise<StoreOrder | null> {This applies to all 8 exported functions in this file.
As per coding guidelines: "Use strict TypeScript type checking; define explicit return types for functions and avoid 'any' type"
67-84:selectShippingRatedoesn't return the updated order, unlike other mutation functions.Other mutation wrappers (
applyCouponCode,removeCouponCode,advanceCheckout,completeOrder) return the updatedorderin the success response, butselectShippingRateonly returns{ success: true }. The caller inpage.tsx(Line 289) works around this by re-fetching viagetCheckoutOrder, but returning the order here would be more consistent and could eliminate that extra network call.This is minor since the current approach works correctly.
src/lib/data/store.ts (1)
5-10: Missing error handling — inconsistent with other data layer wrappers.Other data layer functions in this PR (e.g.,
getCheckoutOrder,getOrders,getOrder) wrap calls in try/catch and return safe fallbacks (null,{ data: [] }).getStorelets exceptions propagate, which could cause unhandled errors in consuming components/pages.Consider adding a try/catch with a sensible fallback, or at minimum document that callers must handle the thrown error.
♻️ Proposed fix
export async function getStore(options?: { locale?: string; currency?: string; }) { - return _getStore(options); + try { + return await _getStore(options); + } catch { + return null; + } }src/lib/data/orders.ts (1)
5-11: Add explicit type for params to align with SDK type safety guidelines.The
Record<string, unknown>parameter type is too loose. Per the coding guidelines, Spree SDK types should be used for type safety. If@spree/next'slistOrdersaccepts specific parameters (likeinclude,filter,per_page), use a typed interface or the SDK's own param type instead ofRecord<string, unknown>. Additionally, define an explicit return type for the function.src/lib/data/taxonomies.ts (2)
10-38: Missing explicit return types on all exported functions.The coding guidelines require explicit return types for functions. All four functions here rely on inferred return types, which makes the public API contract implicit and fragile — any change in
@spree/nextinternals silently propagates to consumers.Additionally,
Record<string, unknown>is a significant type-safety regression from the previously structured parameter interfaces. Consider re-exporting or defining typed param interfaces that match the@spree/nextexpected shapes, so callers get autocomplete and compile-time validation.As per coding guidelines: "Use strict TypeScript type checking; define explicit return types for functions and avoid 'any' type" and "Import and use Spree SDK types (StoreProduct, StoreVariant, StoreOrder, StoreLineItem, PaginatedResponse) for type safety".
10-38: Extract the duplicated options type.The
{ locale?: string; currency?: string }type is repeated verbatim four times in this file (and likely across other data-layer files likeproducts.ts). Extract it into a shared type alias.♻️ Suggested refactor
+type SpreeRequestOptions = { locale?: string; currency?: string }; + export async function getTaxonomies( params?: Record<string, unknown>, - options?: { locale?: string; currency?: string }, + options?: SpreeRequestOptions, ) {Apply similarly to all four functions (and consider placing the type in a shared module if used across files).
src/lib/data/products.ts (1)
10-38: Same type-safety concerns astaxonomies.ts.Missing explicit return types and
Record<string, unknown>params apply here as well. Notably,getProducton Line 19 retains a more specific{ includes?: string }param type — consider applying similar specificity to the other three functions for consistency and caller guidance.As per coding guidelines: "Use strict TypeScript type checking; define explicit return types for functions and avoid 'any' type".
src/lib/data/addresses.ts (1)
30-57: Add explicit discriminated-union return types for mutation functions.
createAddressandupdateAddressreturn either{ success: true, address }or{ success: false, error }, but without an explicit return type annotation, consumers lose the benefit of TypeScript narrowing onsuccess. This matters more here than on the read-only wrappers because callers will branch on the result.♻️ Example for createAddress
+type MutationResult<T = unknown> = + | { success: true; address: T } + | { success: false; error: string }; + -export async function createAddress(address: AddressParams) { +export async function createAddress(address: AddressParams): Promise<MutationResult> {As per coding guidelines: "Use strict TypeScript type checking; define explicit return types for functions and avoid 'any' type".
…ditCards
Wrap addToCart, updateCartItem, removeCartItem in try/catch with structured
{ success, cart, error } returns for consistency with associateCartWithUser.
Update CartContext to handle the new structured return type. Add explicit
return type annotation to getCreditCards.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/lib/data/cart.ts`:
- Around line 13-23: clearCart currently re-throws errors instead of matching
the structured { success, error } response used by other mutation helpers;
change clearCart to call _clearCart() inside a try/catch, return { success: true
} on success, and on failure catch the error and return { success: false, error:
String(err) } (or include err.message) so its signature and error-handling
mirror addToCart, updateCartItem, removeCartItem, and associateCartWithUser
while still delegating work to _clearCart.
🧹 Nitpick comments (4)
src/lib/data/credit-cards.ts (1)
17-28: Add an explicit return type todeleteCreditCard.Per coding guidelines, all functions should have explicit return types. The two return branches have different shapes (
{ success: true }vs{ success: false, error: string }), so a discriminated union would also improve caller ergonomics.Suggested fix
-export async function deleteCreditCard(id: string) { +export async function deleteCreditCard( + id: string +): Promise<{ success: true } | { success: false; error: string }> {As per coding guidelines,
**/*.ts{,x}: "Use strict TypeScript type checking; define explicit return types for functions and avoid 'any' type".src/lib/data/cart.ts (2)
13-15: Missing explicit return type annotations on all exported functions.The coding guideline requires explicit return types for functions. Every public function here relies on inference. Adding return types improves documentation and prevents accidental signature drift — especially important for Server Actions consumed across client boundaries.
For example:
-export async function addToCart(variantId: string, quantity: number) { +export async function addToCart(variantId: string, quantity: number): Promise< + { success: true; cart: StoreOrder } | { success: false; error: string } +> {As per coding guidelines: "Use strict TypeScript type checking; define explicit return types for functions and avoid 'any' type."
Also applies to: 17-19, 25-37, 39-51, 53-65, 67-78
25-37: Mutation-then-fetch can return a false failure when only the re-fetch fails.If
addItem/updateItem/removeItemsucceeds but the subsequent_getCart()throws, the catch returns{ success: false }even though the server-side mutation was applied. The caller (CartContext) won't update its state, leaving the UI stale until the next refresh.A lightweight mitigation is to catch the re-fetch separately:
await addItem(variantId, quantity); try { const cart = await _getCart(); return { success: true as const, cart }; } catch { return { success: true as const, cart: null }; }This way callers know the mutation succeeded and can trigger a refresh. Not blocking, but worth considering.
Also applies to: 39-51, 53-65
src/contexts/CartContext.tsx (1)
57-74: ConsideruseOptimisticfor instant cart UI updates.All three mutation handlers (
addItem,updateItem,removeItem) setupdating=true, wait for the full server round-trip, then update state. This means the user sees a loading state until the Server Action completes and returns the refreshed cart.React 19's
useOptimistichook would let you apply the expected state change immediately (e.g., update quantity or remove item from the local cart) and reconcile when the server responds, providing a snappier UX. This is a good-to-have improvement and can be deferred.As per coding guidelines: "Use useOptimistic hook for instant UI updates when performing async operations like cart updates."
Also applies to: 76-92, 94-110
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Set up Vitest with React Testing Library and jsdom for unit/integration testing. Add 58 tests across 5 test files covering the data layer (cart, checkout, customer), CartContext, and ProductCard component. Add GitHub Actions CI workflow with parallel lint, typecheck, and test jobs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Use node: protocol for path import, fix import ordering, and auto-format. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2 participants
Summary
Migrate all server-side data layer from direct
@spree/sdkusage to@spree/next, which provides Server Actions for authentication, cart, checkout, and customer operations with built-in cookie-based auth and token refresh.Changes
@spree/next@0.1.1as dependency and configuretranspilePackagessrc/lib/spree.ts(client initialization now internal)src/lib/data/auth-request.ts(token refresh now internal)@spree/nextServer ActionsTest plan
npx tsc --noEmit)npm run check)npm run build)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Refactor