Refactor: eliminate code duplication and extract shared utilities#13
Refactor: eliminate code duplication and extract shared utilities#13damianlegawiec merged 24 commits intomainfrom
Conversation
Consolidate 10 duplicate copies of extractBasePath and 2 copies of getPathWithoutPrefix into a single shared module at src/lib/utils/path.ts. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace 16 identical try/catch blocks across 7 data files with two shared helpers: actionResult (for mutation results) and withFallback (for fetch-or-default queries). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- buildProductQueryParams: shared filter-to-query-params conversion - useProductListing: shared state, pagination, infinite scroll, filter fetching - ProductListingLayout: shared filter sidebar, skeleton, empty state, load-more UI ProductsContent shrinks from 410 to 62 lines, CategoryProductsContent from 373 to 39 lines. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ters - Remove unused internal filter fetching state/effect (~30 lines dead code) - Remove useStore and getProductFilters imports (no longer needed) - Make filtersData and loading required props - Replace useEffect-based onFilterChange with direct calls via updateFilters helper - Eliminates an unnecessary render cycle on every filter change Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace 3 structurally identical cart mutation callbacks (addItem, updateItem, removeItem) with a shared mutateCart helper that handles the setUpdating/result.success/router.refresh pattern. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace 3 identical user-object constructions in refreshUser, login, and register with a shared toUser helper function. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Extract setStoreCookies helper to src/lib/utils/cookies.ts, replacing duplicated document.cookie calls with magic numbers - Extract resolveCountryAndCurrency as a pure function from the 68-line useEffect in StoreContext, improving readability - Add explicit eslint-disable comment for intentionally excluded pathname dependency Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Standardize CheckoutContext to use undefined (matching Cart, Auth, Store contexts) instead of null - Remove unnecessary fragment wrapper in CountrySwitcher - Remove unused currentCountry variable in CountrySwitcher - Merge two redundant guard blocks in account layout into one Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Consolidate AddressFormData, emptyAddress, addressToFormData, formDataToAddress, and addressesMatch from 4 files into src/lib/utils/address.ts, removing ~250 lines of duplicated code. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace 4 copies of address form fields (AddressStep, AddressSelector, PaymentStep BillingAddressForm, account AddressModal) with a single shared AddressFormFields component. Extract AddressEditModal to replace duplicated modal+form+state logic in AddressStep and account addresses page. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
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:
WalkthroughConsolidates path helpers and cookie utilities into shared utils, centralizes SVG icons via an icons barrel and SVGR, extracts address form UI and mapping into reusable components/utilities, introduces a product-listing hook/layout, and standardizes data-layer error/fallback handling across multiple modules. Changes
Sequence Diagram(s)sequenceDiagram
participant Page as Product Page (Client)
participant Hook as useProductListing
participant API as Product API
participant Filters as ProductFilters
participant Layout as ProductListingLayout
Page->>Hook: init(fetchFn, searchQuery, filterParams)
Hook->>API: fetchProductFilters(currency, locale)
API-->>Hook: filtersData
Hook->>API: fetchProducts(page=1, params)
API-->>Hook: products + meta
Hook-->>Page: listing state (products, totalCount, hasMore, handlers)
Page->>Layout: render(listing)
Layout->>Filters: render(filtersData)
Filters-->>Layout: onFilterChange(newFilters)
Layout->>Hook: handleFilterChange(newFilters)
Hook->>API: fetchProducts(page=1, filtered params)
API-->>Hook: filtered products
Hook-->>Layout: update products
sequenceDiagram
participant User as User
participant Modal as AddressEditModal
participant Form as AddressFormFields
participant API as Address API
participant Parent as Page (onSave handler)
User->>Modal: open(edit/new)
Modal->>Form: render(formData)
User->>Form: change country
Form->>Modal: request fetchStates(country)
Modal->>API: getStates(country)
API-->>Modal: states
Modal->>Form: update states
User->>Form: fill fields
User->>Modal: click Save
Modal->>Parent: onSave(formDataToAddress(data), maybeId)
Parent->>API: create/update address
API-->>Parent: result (address or error)
Parent-->>Modal: success (updated address)
Modal-->>User: close and return updated data
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/contexts/CartContext.tsx (1)
57-82:⚠️ Potential issue | 🟠 MajorMissing
catchblock — network errors will propagate as unhandled rejections.
mutateCarthastry/finallybut nocatch. Ifaction()throws (e.g., network failure, JSON parse error), the exception escapesmutateCartand becomes an unhandled promise rejection inaddItem/updateItem/removeItem, since no caller catches either.🐛 Proposed fix
try { const result = await action(); if (result.success) { setCart(result.cart ?? null); onSuccess?.(); router.refresh(); } else { console.error(`Failed to ${errorLabel}:`, result.error); } + } catch (err) { + console.error(`Failed to ${errorLabel}:`, err); } finally { setUpdating(false); }src/components/checkout/PaymentStep.tsx (1)
59-67:⚠️ Potential issue | 🟡 MinorMissing
.catchonfetchStatespromise.If
fetchStatesrejects (e.g., network error), the promise rejection is unhandled since.thenhas no rejection handler and there's no.catch.startTransitionBilldoes not catch async promise rejections.🛡️ Proposed fix
startTransitionBill(() => { fetchStates(billAddress.country_iso).then((states) => { if (!cancelled) { setBillStates(states); } - }); + }).catch(() => { + if (!cancelled) { + setBillStates([]); + } + }); });src/components/products/ProductFilters.tsx (1)
250-251:⚠️ Potential issue | 🟡 MinorPriceFilter local state goes stale after external reset.
localMinandlocalMaxare initialized fromminValue/maxValueprops only once (viauseStateinitial value). When the parent callsclearFiltersand resetsactiveFilters, the newundefinedvalues forminValue/maxValueare passed as props butlocalMin/localMaxretain their old values — the price inputs won't visually reset.Either derive a
keyfrom the parent to force remount, or sync local state with props.🐛 Option A: key-based reset (preferred, simpler)
In the parent's render of
<PriceFilter>(around line 149), add akeythat changes on reset:<PriceFilter + key={`${activeFilters.priceMin}-${activeFilters.priceMax}`} filter={filter as PriceRangeFilter} minValue={activeFilters.priceMin} maxValue={activeFilters.priceMax} onChange={handlePriceChange} />
🤖 Fix all issues with AI agents
In `@src/components/checkout/AddressEditModal.tsx`:
- Around line 64-70: The fetchStates call can reject and currently lacks error
handling; add a .catch handler to the promise chain started in AddressEditModal
so failures are handled and the user/state updated appropriately: call
fetchStates(formData.country_iso).then(result => { if (!cancelled)
setStates(result); }).catch(err => { if (!cancelled) { /* set an error state or
clear states and optionally surface a user message */ } }).finally(() => { if
(!cancelled) setLoadingStates(false); }); ensure you reference the existing
symbols fetchStates, setStates, setLoadingStates and cancelled and set or
dispatch a suitable error indicator (or clear states) inside the .catch so
there’s no unhandled rejection and the UI can show failure feedback.
In `@src/components/checkout/AddressStep.tsx`:
- Around line 109-119: The function handleSaveEditedAddress currently returns
silently when id is missing or onUpdateSavedAddress is undefined, causing
AddressEditModal.handleSubmit to close the modal without feedback; change
handleSaveEditedAddress to throw an Error (or reject) when !id or
!onUpdateSavedAddress instead of returning, so the await onSave(...) in
AddressEditModal.handleSubmit can surface the error and the modal can show an
error message; update references to handleSaveEditedAddress,
onUpdateSavedAddress and ensure AddressEditModal.handleSubmit awaits and handles
the thrown error (so onClose is only called on success).
In `@src/components/products/ProductListingLayout.tsx`:
- Around line 69-108: Add keyboard dismissal and basic focus trapping for the
mobile filter drawer in ProductListingLayout: when showMobileFilters is true,
attach a keydown listener (or onKeyDown on the drawer root) that closes the
drawer on Escape by calling setShowMobileFilters(false), move focus into the
drawer (e.g., focus the close button via a useRef to the close button) when it
opens, trap Tab/Shift+Tab within the drawer by detecting focusable elements
inside the drawer container and cycling focus between them, and restore focus to
the previously focused element when the drawer closes; implement these behaviors
near the drawer JSX that references showMobileFilters, setShowMobileFilters, and
the close button element so keyboard and screen-reader users can dismiss and
navigate the drawer.
In `@src/hooks/useProductListing.ts`:
- Around line 139-156: loadMore uses the latest activeFilters when the request
resolves, which can cause older results to be appended after a filter change; to
fix, snapshot the current loadIdRef.current (and optionally the activeFilters)
at the start of loadMore, pass that id through the async flow, and before
calling setProducts or updating hasMore/pageRef check that loadIdRef.current ===
capturedLoadId (and that capturedActiveFilters matches if you snapshot it) so
stale responses are ignored; update the function name references: loadMore,
loadProducts, loadIdRef, setProducts, pageRef, hasMoreRef to locate and
implement the guard.
- Around line 67-85: loadProducts can suffer from a race where an earlier fetch
resolves after a later one and overwrites state; fix by adding a
request-tracking guard (e.g., a request counter ref or AbortController) inside
loadProducts and only apply response to
setProducts/setTotalCount/setHasMore/hasMoreRef.current if the request is still
the latest. Specifically, create a mutable ref like requestIdRef and increment
it before calling fetchProducts(1, ...), capture the current id in the async
scope, and after awaiting response verify the id still matches
requestIdRef.current (or check !abortSignal.aborted) before calling
setProducts/setTotalCount/setHasMore/hasMoreRef.current and setLoading(false);
also ensure you setLoading(false) only when the request is valid.
In `@src/lib/data/orders.ts`:
- Around line 6-8: The withFallback wrapper currently swallows errors from calls
like listOrders, so modify withFallback (in src/lib/data/utils.ts) to log the
caught error (e.g., console.error or forward to your error-tracking helper)
before returning the fallback, and then add an explicit return type annotation
to getOrders (matching the pattern used by getCreditCards, e.g., Promise<{ data:
StoreOrder[] }>) so callers have a clear type; update references to
getOrders/listOrders as needed to satisfy the type name you choose.
In `@src/lib/utils/address.ts`:
- Around line 17-29: addressToFormData currently returns the shared constant
emptyAddress when called with no address, which allows callers to mutate a
global object; change addressToFormData so it returns a shallow copy of
emptyAddress (e.g., spread or Object.assign) instead of the original reference
to ensure callers receive an independent object and do not corrupt the shared
emptyAddress constant.
In `@src/lib/utils/cookies.ts`:
- Around line 6-9: The setStoreCookies function currently writes raw country and
locale into document.cookie (in setStoreCookies), which can break cookie
formatting if values contain reserved characters; update setStoreCookies to
encode the country and locale values (e.g., via encodeURIComponent) before
interpolating into the cookie strings and keep the same attributes (path and
max-age using ONE_YEAR); also ensure any corresponding cookie read logic decodes
those values (decodeURIComponent) where cookies are parsed.
🧹 Nitpick comments (12)
src/contexts/AuthContext.tsx (1)
45-57: Parameter type duplicates theUserinterface — reuse it directly.The inline type on lines 45–50 is identical to the
Userinterface defined on lines 19–24.Proposed simplification
-function toUser(customer: { - id: string; - email: string; - first_name?: string | null; - last_name?: string | null; -}): User { +function toUser(customer: User): User { return { id: customer.id, email: customer.email, first_name: customer.first_name, last_name: customer.last_name, }; }If the intent is to accept a broader type (e.g., a customer object with extra fields), consider
Pick<Customer, 'id' | 'email' | 'first_name' | 'last_name'>or a similar approach instead of re-declaring the shape inline.src/lib/utils/address.ts (1)
76-98:addressesMatchintentionally compares only a subset of fields — verify this is sufficient.The comparison omits
address2,phone,company,state_abbr, andstate_name. This could cause false positives (two different addresses matching) if they differ only in those fields. If this is intentional for "fuzzy matching," a brief doc comment would help future maintainers understand the design choice.src/lib/utils/product-query.ts (2)
16-20: Extract the magic max-price fallback into a named constant.
999999is a magic number that could silently exclude expensive products. A named constant would improve clarity and make it easy to adjust.♻️ Suggested change
+const DEFAULT_MAX_PRICE = 999999; + export function buildProductQueryParams( filters: ActiveFilters, searchQuery?: string, ): Record<string, unknown> { ... if (filters.priceMin !== undefined || filters.priceMax !== undefined) { params["q[price_between][]"] = [ filters.priceMin ?? 0, - filters.priceMax ?? 999999, + filters.priceMax ?? DEFAULT_MAX_PRICE, ]; }
1-1: MoveActiveFiltersto a shared types location to eliminate upward dependencies.
src/lib/utils/product-query.tsimportsActiveFiltersfrom the components layer, creating an upward dependency that violates typical layering conventions. Additionally,src/hooks/useProductListing.tshas the same pattern.ActiveFiltersis a domain type used across multiple layers (components, hooks, and utils) and should be moved tosrc/lib/types/filters.tsor similar. Update imports inProductFilters.tsx,ProductListingLayout.tsx,useProductListing.ts, andproduct-query.tsto import from the shared location.src/contexts/StoreContext.tsx (1)
95-130: Effect refactor is sound; verifypathnameexclusion is safe under fast-navigation scenarios.The
pathnameexclusion from the dependency array is correct for the "run once on mount" intent, and the eslint-disable comment clearly explains the rationale. However, if a user navigates rapidly (e.g., back/forward) before the async fetch completes,pathnameread on line 104 could reflect a stale or different URL by the time the redirect fires.Consider capturing
pathnameat the start of the async closure, though in practice this is a narrow race given Next.js remounting on route changes.src/components/checkout/PaymentStep.tsx (1)
75-79: Resetting billing address viauseEffectcan be moved to the event handler.This effect resets
billAddresswhenuseShippingForBillingtoggles, requiring aneslint-disableto suppress the exhaustive-deps warning. Moving this logic into the checkboxonChangehandler at Line 148 would eliminate the effect and the suppression.♻️ Proposed refactor
- // When "use shipping" changes, reset billing address - useEffect(() => { - if (useShippingForBilling) { - setBillAddress(shipAddressData); - } - }, [useShippingForBilling]); // eslint-disable-line react-hooks/exhaustive-depsThen in the checkbox handler:
onChange={(e) => { - setUseShippingForBilling(e.target.checked) + const checked = e.target.checked; + setUseShippingForBilling(checked); + if (checked) { + setBillAddress(addressToFormData(order.ship_address)); + } }}As per coding guidelines: "Avoid using useEffect to reset state when props change; use component key prop to reset state or compute initial state from props."
src/components/checkout/AddressEditModal.tsx (1)
107-157: Modal lacks accessibility attributes and keyboard handling.The modal
<div>is missingrole="dialog",aria-modal="true", andaria-labelledbypointing to the title. There's also no Escape key handler to close the modal. Consider adding these for keyboard and screen reader users.♿ Suggested attributes on the dialog container (Line 114)
- <div className="relative inline-block align-bottom bg-white rounded-lg text-left overflow-hidden shadow-xl transform transition-all sm:my-8 sm:align-middle sm:max-w-lg sm:w-full"> + <div + role="dialog" + aria-modal="true" + aria-labelledby="modal-title" + className="relative inline-block align-bottom bg-white rounded-lg text-left overflow-hidden shadow-xl transform transition-all sm:my-8 sm:align-middle sm:max-w-lg sm:w-full" + >And on the title:
- <h3 className="text-lg font-medium text-gray-900 mb-4"> + <h3 id="modal-title" className="text-lg font-medium text-gray-900 mb-4">src/components/products/ProductFilters.tsx (1)
38-44: Side effect inside state updater function.
onFilterChange(next)is called inside thesetActiveFiltersupdater callback. React state updater functions should be pure — they may be called multiple times (e.g., in Strict Mode). Move the parent notification outside the updater.♻️ Proposed refactor
- const updateFilters = (updater: (prev: ActiveFilters) => ActiveFilters) => { - setActiveFilters((prev) => { - const next = updater(prev); - onFilterChange(next); - return next; - }); - }; + const updateFilters = (updater: (prev: ActiveFilters) => ActiveFilters) => { + setActiveFilters((prev) => { + const next = updater(prev); + // Schedule notification outside the updater + queueMicrotask(() => onFilterChange(next)); + return next; + }); + };Alternatively, a cleaner approach using a functional update and separate call:
- const updateFilters = (updater: (prev: ActiveFilters) => ActiveFilters) => { - setActiveFilters((prev) => { - const next = updater(prev); - onFilterChange(next); - return next; - }); - }; + const updateFilters = (updater: (prev: ActiveFilters) => ActiveFilters) => { + let next: ActiveFilters; + setActiveFilters((prev) => { + next = updater(prev); + return next; + }); + // React 18+ batches setState, so next is assigned synchronously + onFilterChange(next!); + };src/hooks/useProductListing.ts (1)
122-126:useEffectfor data fetching onsearchQuerychange.Per coding guidelines, data fetching triggered by state/prop changes should prefer event handlers.
searchQuerycomes from URL search params (viauseSearchParamsin the consumer), so this effect reacts to an external navigation change — it's closer to "synchronizing with an external system" and is reasonable here. Just flagging for awareness.src/lib/data/utils.ts (2)
5-18: Latent key-collision risk: spreadingresultcan overwritesuccess.If a caller's
fnever returns an object containing asuccesskey,{ success: true, ...result }will silently overwrite it (or vice-versa), producing an incorrect discriminant. Current callers are safe, but nothing in the type system prevents this.Consider excluding
successfromT:Suggested type guard
-export async function actionResult<T extends Record<string, unknown>>( +export async function actionResult<T extends Record<string, unknown> & { success?: never }>(
23-32: Silent error swallowing inwithFallback— consider optional logging.All errors are discarded without any trace. For read operations this is a reasonable default, but it can make production debugging difficult. An optional
onErrorcallback or aconsole.errorin development would retain observability without changing the API contract.src/lib/data/checkout.ts (1)
61-66: Fallback message "Invalid coupon code" may be misleading for non-validation failures.The
fallbackMessageis only used when the caught error is not anErrorinstance. However, "Invalid coupon code" implies a validation failure, while the actual cause could be a network or server error. Consider a more neutral message like"Failed to apply coupon code"to match the convention used by all other actions in this file.
| {/* Mobile filter drawer */} | ||
| {showMobileFilters && ( | ||
| <div className="fixed inset-0 z-50 lg:hidden"> | ||
| <div | ||
| className="fixed inset-0 bg-black/25" | ||
| onClick={() => setShowMobileFilters(false)} | ||
| /> | ||
| <div className="fixed inset-y-0 right-0 w-full max-w-xs bg-white shadow-xl overflow-y-auto"> | ||
| <div className="flex items-center justify-between p-4 border-b"> | ||
| <h2 className="text-lg font-medium text-gray-900">Filters</h2> | ||
| <button | ||
| onClick={() => setShowMobileFilters(false)} | ||
| className="p-2 -mr-2 text-gray-400 hover:text-gray-500" | ||
| > | ||
| <svg | ||
| className="w-6 h-6" | ||
| fill="none" | ||
| stroke="currentColor" | ||
| viewBox="0 0 24 24" | ||
| > | ||
| <path | ||
| strokeLinecap="round" | ||
| strokeLinejoin="round" | ||
| strokeWidth={2} | ||
| d="M6 18L18 6M6 6l12 12" | ||
| /> | ||
| </svg> | ||
| </button> | ||
| </div> | ||
| <div className="p-4"> | ||
| <ProductFilters | ||
| taxonId={taxonId} | ||
| filtersData={filtersData} | ||
| loading={filtersLoading} | ||
| onFilterChange={onFilterChange} | ||
| /> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
Mobile filter drawer lacks Escape key dismissal and focus trapping.
The overlay closes on backdrop click but doesn't handle the Escape key or trap focus inside the drawer. Screen reader and keyboard-only users cannot dismiss it without clicking the close button. Consider adding an onKeyDown handler for Escape and basic focus management.
🤖 Prompt for AI Agents
In `@src/components/products/ProductListingLayout.tsx` around lines 69 - 108, Add
keyboard dismissal and basic focus trapping for the mobile filter drawer in
ProductListingLayout: when showMobileFilters is true, attach a keydown listener
(or onKeyDown on the drawer root) that closes the drawer on Escape by calling
setShowMobileFilters(false), move focus into the drawer (e.g., focus the close
button via a useRef to the close button) when it opens, trap Tab/Shift+Tab
within the drawer by detecting focusable elements inside the drawer container
and cycling focus between them, and restore focus to the previously focused
element when the drawer closes; implement these behaviors near the drawer JSX
that references showMobileFilters, setShowMobileFilters, and the close button
element so keyboard and screen-reader users can dismiss and navigate the drawer.
| export async function getOrders(params?: Record<string, unknown>) { | ||
| try { | ||
| return await listOrders(params); | ||
| } catch { | ||
| return { data: [] }; | ||
| } | ||
| return withFallback(() => listOrders(params), { data: [] }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/lib/data/orders.ts | head -30Repository: spree/nextjs-starter-spree
Length of output: 547
🏁 Script executed:
find . -name "utils.ts" -path "*/lib/data/*" -type fRepository: spree/nextjs-starter-spree
Length of output: 93
🏁 Script executed:
rg -nP "withFallback" -A 5 -B 2 --type=tsRepository: spree/nextjs-starter-spree
Length of output: 4486
🏁 Script executed:
rg -nP "function listOrders|const listOrders" -A 10 --type=tsRepository: spree/nextjs-starter-spree
Length of output: 52
🏁 Script executed:
cat -n src/lib/data/utils.tsRepository: spree/nextjs-starter-spree
Length of output: 1111
🏁 Script executed:
rg -nP "getOrders\(" --type=ts --type=tsx -C 5Repository: spree/nextjs-starter-spree
Length of output: 97
🏁 Script executed:
rg -nP "from.*\"@spree/next\"" --type=ts -A 2 | head -50Repository: spree/nextjs-starter-spree
Length of output: 2332
🏁 Script executed:
rg -nP "getOrders" --type=ts -C 5Repository: spree/nextjs-starter-spree
Length of output: 3011
🏁 Script executed:
rg -nP "\.meta" --type=ts -C 2 | grep -A 5 -B 5 "getOrders\|listOrders"Repository: spree/nextjs-starter-spree
Length of output: 52
🏁 Script executed:
rg -nP "PaginatedResponse|StoreProduct|type.*Order" --type=ts src/lib/data/ | head -20Repository: spree/nextjs-starter-spree
Length of output: 52
🏁 Script executed:
# Search for actual usage patterns and what gets accessed from the result
rg -nP "getOrders\(" -A 10 --type=tsRepository: spree/nextjs-starter-spree
Length of output: 1536
🏁 Script executed:
# Search for type definitions or any explicit types for getOrders or listOrders
rg -nP "type.*Orders|interface.*Orders" --type=ts src/lib/data/ -A 3Repository: spree/nextjs-starter-spree
Length of output: 52
🏁 Script executed:
# Check if there are any TypeScript errors in the build or type checking
find . -name "tsconfig.json" -o -name "tsconfig.*.json" | head -5Repository: spree/nextjs-starter-spree
Length of output: 85
🏁 Script executed:
# Look at the `@spree/next` package to understand the return type
cat -n node_modules/@spree/sdk/dist/index.d.ts 2>/dev/null | grep -A 10 "listOrders\|PaginatedResponse" | head -30Repository: spree/nextjs-starter-spree
Length of output: 52
🏁 Script executed:
# Search for more comprehensive usage of getOrders to see if .meta is ever accessed
rg -nP "response\.meta|getOrders" --type=ts --type=js -B 2 -A 15 | head -100Repository: spree/nextjs-starter-spree
Length of output: 6334
🏁 Script executed:
# Check tsconfig strictness settings
cat -n tsconfig.json | grep -E "strict|noImplicitAny|exactOptionalPropertyTypes"Repository: spree/nextjs-starter-spree
Length of output: 96
🏁 Script executed:
# Double-check for all callers of getOrders in the entire codebase
rg -nP "getOrders" --type=ts --type=jsRepository: spree/nextjs-starter-spree
Length of output: 400
🏁 Script executed:
# Check the actual type definition of listOrders from Spree SDK
rg -nP "listOrders" src/lib/data/ -B 2 -A 10Repository: spree/nextjs-starter-spree
Length of output: 766
🏁 Script executed:
# Look for any error logging or monitoring setup in the codebase
rg -nP "console\.error|logger|sentry|errorHandler" src/lib/data/ --type=ts | head -20Repository: spree/nextjs-starter-spree
Length of output: 52
🏁 Script executed:
# Check if there's a type definition file or package.json to understand the Spree SDK version
cat -n package.json | grep -A 5 -B 5 "@spree\|next"Repository: spree/nextjs-starter-spree
Length of output: 783
🏁 Script executed:
# Verify whether listOrders actual usage pattern in other data files
rg -nP "listOrders|listAddresses|listCreditCards" src/lib/data/ -B 2 -A 3 --type=tsRepository: spree/nextjs-starter-spree
Length of output: 2131
🏁 Script executed:
# Check if there's any return type annotation on getOrders that might clarify expected structure
rg -nP "getOrders.*=>|function getOrders" src/lib/data/orders.ts -A 5 --type=tsRepository: spree/nextjs-starter-spree
Length of output: 354
Add error logging to withFallback to prevent silent failures.
The withFallback utility in src/lib/data/utils.ts catches errors and returns the fallback value with no logging. Failures in listOrders become completely invisible, making production debugging difficult.
Consider adding console.error(error) or integrating with an error tracking service (Sentry, etc.) to surface failures.
Additionally, add an explicit return type annotation to getOrders for consistency and clarity, similar to other data functions like getCreditCards(): Promise<{ data: StoreCreditCard[] }>.
🤖 Prompt for AI Agents
In `@src/lib/data/orders.ts` around lines 6 - 8, The withFallback wrapper
currently swallows errors from calls like listOrders, so modify withFallback (in
src/lib/data/utils.ts) to log the caught error (e.g., console.error or forward
to your error-tracking helper) before returning the fallback, and then add an
explicit return type annotation to getOrders (matching the pattern used by
getCreditCards, e.g., Promise<{ data: StoreOrder[] }>) so callers have a clear
type; update references to getOrders/listOrders as needed to satisfy the type
name you choose.
- Add missing `meta` field to orders fallback to match PaginatedResponse type - Fix type narrowing for updateAddress result by checking `!result.success` first Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add catch block to mutateCart for unhandled network errors - Return spread copy from addressToFormData to prevent shared state mutation - Simplify toUser parameter to use User type directly - Move billing address reset from useEffect to onChange handler - Add accessibility attrs to AddressEditModal (role, aria-modal, escape key) - Fix side effect in ProductFilters state updater Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/products/ProductFilters.tsx (1)
248-249:⚠️ Potential issue | 🟠 MajorPriceFilter inputs don't reset when filters are cleared.
localMinandlocalMaxare initialized from props viauseState, which only evaluates the initializer on mount. When the parent callsclearFilters(resettingpriceMin/priceMaxtoundefined), the input fields retain their stale values.🐛 Option A — key-based remount (simplest)
In the parent JSX where
<PriceFilter>is rendered (around line 147):<PriceFilter + key={`${activeFilters.priceMin}-${activeFilters.priceMax}`} filter={filter as PriceRangeFilter} minValue={activeFilters.priceMin} maxValue={activeFilters.priceMax} onChange={handlePriceChange} />🐛 Option B — controlled local state sync
const [localMin, setLocalMin] = useState(minValue?.toString() || ""); const [localMax, setLocalMax] = useState(maxValue?.toString() || ""); + + // Sync local inputs when parent resets the values + if (minValue === undefined && localMin !== "") setLocalMin(""); + if (maxValue === undefined && localMax !== "") setLocalMax("");This uses the React pattern of adjusting state during render when props change, avoiding a
useEffect. As per coding guidelines: "Avoid using useEffect to reset state when props change; use component key prop to reset state or compute initial state from props."src/components/checkout/PaymentStep.tsx (1)
61-67:⚠️ Potential issue | 🟡 MinorMissing
.catchonfetchStates— unhandled rejection risk.Same issue as in
AddressEditModal: iffetchStatesrejects, nothing catches the error, producing an unhandled promise rejection. Add a.catchto clear states (or surface an error) on failure.🛡️ Proposed fix
startTransitionBill(() => { - fetchStates(billAddress.country_iso).then((states) => { - if (!cancelled) { - setBillStates(states); - } - }); + fetchStates(billAddress.country_iso) + .then((states) => { + if (!cancelled) setBillStates(states); + }) + .catch(() => { + if (!cancelled) setBillStates([]); + }); });
🤖 Fix all issues with AI agents
In `@src/components/checkout/AddressEditModal.tsx`:
- Around line 119-130: AddressEditModal currently sets role="dialog" but doesn't
trap focus, lock body scroll, or restore focus on close; update the component to
(a) wrap the modal content with a focus-trap (e.g., add FocusTrap from
focus-trap-react around the inner modal container in AddressEditModal) or
implement a useEffect that saves the activeElement, sets focus to the modal
container, and handles Tab/Shift+Tab to loop focus within the modal, (b) disable
body scrolling while open by adding/removing document.body.style.overflow =
'hidden' when the modal mounts/unmounts, and (c) onClose restore focus to the
previously focused element (capture it on open and call .focus() after closing
via the onClose handler). Ensure these changes target the modal container
element used inside AddressEditModal and integrate with the existing onClose
prop.
🧹 Nitpick comments (6)
src/contexts/CartContext.tsx (1)
97-115: ConsideruseOptimisticfor instant cart UI updates.Currently,
updateItemandremoveItemwait for the server response before reflecting changes. Using React 19'suseOptimistichook would let you show the updated quantity (or removal) immediately while the mutation is in flight, improving perceived responsiveness — especially for quantity adjustments.This can be addressed in a follow-up. Based on learnings: "Use useOptimistic hook for instant UI updates when performing async operations like cart updates."
src/components/products/ProductFilters.tsx (2)
38-42:updateFiltersreads a potentially staleactiveFilterssnapshot.The updater callback receives
activeFiltersfrom the enclosing closure rather than the latest state. If two updates were ever batched in the same render tick, the second would overwrite the first. A safer pattern captures the latest value fromsetActiveFilters's functional form:♻️ Proposed fix
const updateFilters = (updater: (prev: ActiveFilters) => ActiveFilters) => { - const next = updater(activeFilters); - setActiveFilters(next); - onFilterChange(next); + setActiveFilters((prev) => { + const next = updater(prev); + // Notify parent of the change — safe because next is a new object + // Note: onFilterChange is called during setState; if this causes + // issues, consider using useEffect or flushSync. + onFilterChange(next); + return next; + }); };Alternatively, keep the imperative style but use a
useRefto always hold the latest filters, or move theonFilterChangecall into auseEffectwatchingactiveFilters.
79-83:clearFiltersduplicates the set-and-notify pattern instead of reusingupdateFilters.Minor inconsistency — this could call
updateFilters(() => ({ optionValues: [] }))to keep a single notification path.src/components/checkout/AddressEditModal.tsx (1)
46-48: Consider spreadingemptyAddressto avoid sharing the reference.When
addressis null, the mutableemptyAddressobject is passed directly as initial state. While current update logic always creates new objects, a defensive spread avoids accidental mutation if the code evolves.🛡️ Suggested fix
const [formData, setFormData] = useState<AddressFormData>( - address ? addressToFormData(address) : emptyAddress, + address ? addressToFormData(address) : { ...emptyAddress }, );src/components/checkout/PaymentStep.tsx (2)
39-42:addressToFormDatais called on every render for both addresses.
shipAddressDataandbillAddressDataallocate new objects each render. Sinceorderis a prop that won't change within this component's lifecycle, consideruseMemoto avoid the repeated allocation — or at minimum forshipAddressDatawhich is referenced in closures (handleUseShippingChange,handleSubmit).♻️ Optional optimization
+import { useMemo } from "react"; ... - const shipAddressData = addressToFormData(order.ship_address); - const billAddressData = addressToFormData(order.bill_address); + const shipAddressData = useMemo(() => addressToFormData(order.ship_address), [order.ship_address]); + const billAddressData = useMemo(() => addressToFormData(order.bill_address), [order.bill_address]);
53-72: Consider moving state-fetching into theupdateBillAddresshandler.Per the coding guideline "Avoid using
useEffectfor data fetching triggered by state changes; use event handlers or Server Actions instead," thefetchStatescall could be triggered fromupdateBillAddresswhenfield === "country_iso", with an additional one-time call for the initial country. This would eliminate the effect and the cancellation boilerplate.That said, this mirrors the
AddressEditModalpattern and works correctly as-is, so this is a consistency-vs-guideline trade-off you can decide on. As per coding guidelines, "Avoid using useEffect for data fetching triggered by state changes; use event handlers or Server Actions instead."
- Fix race condition in useProductListing with loadIdRef guard for both loadProducts and loadMore to prevent stale results - Add .catch handlers to fetchStates in AddressEditModal and PaymentStep to prevent unhandled promise rejections - Simplify handleUpdateSavedAddress with early-return guard clauses - Throw instead of silently returning in handleSaveEditedAddress - Fix PriceFilter inputs not resetting on clearFilters via key prop - Add error logging to withFallback for production observability - Use neutral fallback message in applyCouponCode - Encode cookie values in setStoreCookies - Spread emptyAddress to avoid shared mutable reference Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/components/checkout/PaymentStep.tsx (1)
83-91:⚠️ Potential issue | 🟠 MajorUnhandled promise rejection from
onSubmit.
onSubmitreturnsPromise<void>but the call on line 85 is fire-and-forget — if it rejects, you get an unhandled promise rejection. The PR description mentions.catchhandlers were added toPaymentStep, but this call site appears to be missing one.Proposed fix
const handleSubmit = (e: React.FormEvent) => { e.preventDefault(); onSubmit({ bill_address: formDataToAddress( useShippingForBilling ? shipAddressData : billAddress, ), use_shipping_for_billing: useShippingForBilling, - }); + }).catch(() => { + // Error is expected to be handled by the caller; + // catch here to prevent unhandled promise rejection. + }); };src/components/checkout/AddressStep.tsx (2)
62-83:⚠️ Potential issue | 🟡 MinorMissing
.catchonfetchStates— unhandled promise rejection.Unlike
AddressEditModalwhich has a.catchhandler on the samefetchStatescall, this effect silently drops rejections. If the fetch fails, the promise rejection will be unhandled andshipStateswill remain stale.🛡️ Proposed fix
startTransitionShip(() => { - fetchStates(shipAddress.country_iso).then((states) => { - if (!cancelled) { - setShipStates(states); - } - }); + fetchStates(shipAddress.country_iso) + .then((states) => { + if (!cancelled) { + setShipStates(states); + } + }) + .catch(() => { + if (!cancelled) { + setShipStates([]); + } + }); });
85-91:⚠️ Potential issue | 🔴 CriticalMake
handleSubmitasync andawaittheonSubmitpromise.The parent component
handleAddressSubmithas proper error handling with try-catch, but sinceonSubmit()isn't awaited inhandleSubmit, any rejection will become an unhandled promise rejection. The parent's error handling won't trigger because the promise isn't monitored. Compare withAddressEditModal.tsxwhich correctly usesasyncandawaitfor its form submission.
🤖 Fix all issues with AI agents
In `@src/app/`[country]/[locale]/(checkout)/checkout/[id]/page.tsx:
- Around line 370-387: Add an explicit return type Promise<StoreAddress> to the
handleUpdateSavedAddress function signature; keep the implementation as-is (it
throws on failure and returns result.address on success). Ensure the
StoreAddress type is imported or available in the same module and that the
AddressParams parameter type remains unchanged; verify the function still calls
updateAddress and setSavedAddresses as before.
In `@src/components/checkout/AddressStep.tsx`:
- Around line 109-121: handleSaveEditedAddress silently no-ops when
onUpdateSavedAddress returns null which causes AddressEditModal.handleSubmit to
close the modal with no feedback; update handleSaveEditedAddress to detect a
falsy/ null updatedAddress after calling onUpdateSavedAddress(id, data) and
throw a descriptive Error (e.g. "Failed to update address") so the modal's error
banner can surface the failure; keep existing logic for updating state
(setSavedAddresses) and calling handleSelectSavedAddress when updatedAddress is
present.
🧹 Nitpick comments (6)
src/components/checkout/PaymentStep.tsx (1)
53-74: Consider moving state-fetch to theupdateBillAddresshandler.The
useEffecthere fetches data in response tobillAddress.country_isostate changes. Per the coding guidelines, prefer event handlers overuseEffectfor data fetching triggered by state changes. You could trigger the fetch directly insideupdateBillAddresswhenfield === "country_iso", which would also let you drop theuseShippingForBillingdependency from this effect.This is a minor structural improvement and not urgent given the existing cleanup logic works correctly.
src/components/checkout/AddressEditModal.tsx (1)
67-91:fetchStatesas a useEffect dependency may cause re-fetch loops if the caller passes an unstable reference.If the parent component passes an inline or non-memoized
fetchStates, this effect will re-run on every render, re-fetching states even whencountry_isohasn't changed. The callers should wrapfetchStatesinuseCallback, or this effect should store the reference in a ref to avoid re-triggering.Consider documenting this expectation in the prop's JSDoc, or defensively using a ref:
♻️ Defensive ref approach
+import { useCallback, useEffect, useRef, useState } from "react"; -import { useCallback, useEffect, useState } from "react"; export function AddressEditModal({ ... }: AddressEditModalProps) { + const fetchStatesRef = useRef(fetchStates); + fetchStatesRef.current = fetchStates; ... useEffect(() => { if (!formData.country_iso) { setStates([]); return; } let cancelled = false; setLoadingStates(true); - fetchStates(formData.country_iso) + fetchStatesRef.current(formData.country_iso) .then((result) => { if (!cancelled) setStates(result); }) ... return () => { cancelled = true; }; - }, [formData.country_iso, fetchStates]); + }, [formData.country_iso]);#!/bin/bash # Check if callers of AddressEditModal memoize fetchStates rg -n "fetchStates" --type=ts --type-add 'tsx:*.tsx' --type=tsx -C3 -g '!AddressEditModal.tsx'src/components/products/ProductFilters.tsx (2)
38-42:updateFiltersreadsactiveFiltersfrom closure — fragile updater pattern.The updater-function signature suggests React's functional
setStatesemantics, butupdater(activeFilters)always uses the value captured in the current render. If two calls ever occur before a re-render (unlikely today, but the API shape invites it), the second call would use stale state. Consider either documenting this constraint or making it robust:♻️ Suggested robust alternative
const updateFilters = (updater: (prev: ActiveFilters) => ActiveFilters) => { - const next = updater(activeFilters); - setActiveFilters(next); - onFilterChange(next); + setActiveFilters((prev) => { + const next = updater(prev); + onFilterChange(next); + return next; + }); };Note: Calling
onFilterChangeinside the setState updater is safe in React 19's batched rendering — it won't trigger a parent re-render mid-batch. This ensuresprevis always the latest queued state.
79-83: Nit:clearFiltersbypassesupdateFilters.This duplicates the set-then-notify pattern. If you adopt the robust
updateFiltersapproach above, this could simply becomeupdateFilters(() => ({ optionValues: [] })).src/hooks/useProductListing.ts (2)
125-130: Effect omitsloadProductsfrom deps — relies onstoreLoadingtoggling as proxy.This works because currency/locale changes cause
storeLoadingto cycle, which re-triggers the effect with the latestloadProducts. However, iffetchFnchanges independently (without a store reload), the effect would use a staleloadProducts.If this is a known constraint of the hook's contract, the eslint-disable comment could be expanded to note it explicitly alongside
filterParams.
143-161:loadMoredepends onloadingMorestate — causes unnecessary observer churn.Including
loadingMorein the dependency array (line 161) creates a newloadMoreidentity on every toggle, which cascades into the IntersectionObserver effect disconnecting and reconnecting. Using a ref for the loading guard would keeploadMorestable:♻️ Suggested optimization
+ const loadingMoreRef = useRef(false); + const loadMore = useCallback(async () => { - if (loadingMore || !hasMoreRef.current) return; + if (loadingMoreRef.current || !hasMoreRef.current) return; + loadingMoreRef.current = true; setLoadingMore(true); const currentLoadId = loadIdRef.current; const nextPage = pageRef.current + 1; const response = await fetchProducts(nextPage, activeFilters, searchQuery); if (response && loadIdRef.current === currentLoadId) { setProducts((prev) => [...prev, ...response.data]); const moreAvailable = nextPage < response.meta.pages; setHasMore(moreAvailable); hasMoreRef.current = moreAvailable; pageRef.current = nextPage; } + loadingMoreRef.current = false; setLoadingMore(false); - }, [fetchProducts, loadingMore, activeFilters, searchQuery]); + }, [fetchProducts, activeFilters, searchQuery]);This eliminates 2 extra observer teardown/setup cycles per pagination load.
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 (6)
src/app/[country]/[locale]/(storefront)/products/[slug]/ProductDetails.tsx (2)
25-25:⚠️ Potential issue | 🟡 MinorRemove unused
currencydestructuring.The pipeline reports
currencyfromuseStore()is unused. Either remove it or destructure only what's needed.Proposed fix
- const { currency } = useStore(); + const { } = useStore();Or if nothing else from
useStore()is needed here, remove the line entirely.
98-115:⚠️ Potential issue | 🟠 MajorUnhandled
throwin async event handler.The
throw new Error("No variant selected")at Line 104 executes before thetry/catchblock at Lines 108–114, so it will surface as an unhandled promise rejection in the browser. Either move the guard inside thetryblock, or return early with user-facing feedback instead of throwing.Proposed fix — return early instead of throwing
const handleAddToCart = async () => { const variantId = selectedVariant?.id || product.default_variant?.id || product.default_variant_id; if (!variantId) { - throw new Error("No variant selected"); + console.error("No variant selected"); + return; }src/app/[country]/[locale]/(storefront)/account/page.tsx (1)
22-22:⚠️ Potential issue | 🟡 MinorRemove unused
userfrom destructuring — CI is failing.The Biome lint check flags
useras unused. Since this component only needslogin,isAuthenticated, andloadingfrom the auth context, dropuserfrom the destructuring.Proposed fix
- const { user, login, isAuthenticated, loading: authLoading } = useAuth(); + const { login, isAuthenticated, loading: authLoading } = useAuth();src/app/[country]/[locale]/(storefront)/account/credit-cards/page.tsx (2)
31-31:⚠️ Potential issue | 🟡 MinorUse a template literal instead of string concatenation.
Pipeline lint failure flags this line. Also required per coding guidelines.
🔧 Proposed fix
- return "💳 " + ccType; + return `💳 ${ccType}`;
98-109:⚠️ Potential issue | 🟠 MajorFix missing
loadCardsin useEffect dependency array.The CI pipeline reports
useExhaustiveDependenciesfailure here. SinceloadCardsis recreated every render, adding it directly would cause an infinite loop. Move the function body inside the effect or wrap it inuseCallback.🔧 Proposed fix — inline the fetch into the effect
+ const loadCards = useCallback(async () => { + const response = await getCreditCards(); + setCards(response.data); + }, []); - const loadCards = async () => { - const response = await getCreditCards(); - setCards(response.data); - }; - useEffect(() => { async function loadData() { await loadCards(); setLoading(false); } loadData(); - }, []); + }, [loadCards]);src/components/cart/CartDrawer.tsx (1)
42-44:⚠️ Potential issue | 🟡 MinorFix the exhaustive-deps lint violation flagged by CI.
The pipeline reports
useExhaustiveDependencies:pathnameis listed as a dependency but isn't used inside the effect body — the linter sees it as extraneous. The intent is to triggercloseCarton route changes. A common fix is to storepathnamein a ref and compare in the effect, or use the Next.js router events. The simplest approach that satisfies the linter:Proposed fix
- // Close when navigating - useEffect(() => { - closeCart(); - }, [pathname, closeCart]); + // Close when navigating + const prevPathname = useRef(pathname); + useEffect(() => { + if (prevPathname.current !== pathname) { + prevPathname.current = pathname; + closeCart(); + } + }, [pathname, closeCart]);This way
pathnameis actually read inside the effect body, satisfying the lint rule while preserving the navigation-close behavior. Alternatively, you can suppress the rule with a comment if the team prefers the current style, but the CI is currently failing.
🤖 Fix all issues with AI agents
In `@src/app/`[country]/[locale]/(checkout)/layout.tsx:
- Around line 3-9: Reorder the import statements at the top of layout.tsx to
satisfy the project's formatter (group and sort external packages first, then
absolute/internal aliases like "@/components/...", "@/contexts/...",
"@/lib/..."), for example ensuring imports such as Link, usePathname, useState
are grouped before the alias imports and that icon and context imports
(ArrowLeftIcon, CheckoutProvider, CheckoutSummary, useStore, extractBasePath)
follow the project's sorted order; then run the project formatter (e.g.,
npm/yarn format or pnpm format) to apply and verify the final ordering.
In `@src/app/`[country]/[locale]/(storefront)/account/gift-cards/page.tsx:
- Line 226: The GiftIcon in the empty-state uses only "w-12 h-12 text-gray-400"
and may not be horizontally centered; update the GiftIcon's className (the
GiftIcon element) to include "mx-auto" alongside the existing classes so it
centers like the credit-cards icon (i.e., change the GiftIcon className to
include mx-auto).
In `@src/components/products/ProductFilters.tsx`:
- Around line 39-43: updateFilters currently computes next using the render-time
activeFilters closure which can be stale; change it to call setActiveFilters
with the functional updater form: pass a function (prev => { const next =
updater(prev); /* call onFilterChange(next) here */ return next; }) so the
updater always receives the latest state. Keep the onFilterChange call inside
that functional callback (or wrap it with flushSync if you need synchronous DOM
updates) to preserve behavior; reference updateFilters, activeFilters,
setActiveFilters, and onFilterChange when making the change.
🧹 Nitpick comments (3)
src/components/search/SearchBar.tsx (1)
135-136: Good ARIA combobox implementation.The
role="combobox"witharia-expanded,aria-controls,aria-activedescendant, andaria-autocompletefollows the WAI-ARIA combobox pattern well. The extendedshowSuggestionslogic to display the dropdown during loading is good UX.Minor note:
aria-controls="search-suggestions"is always rendered on the input, but the referenced element only exists when the dropdown is visible. Consider conditionally settingaria-controlsonly whenshowSuggestionsis true, though this is not a strict requirement.Also applies to: 155-161, 175-183
src/components/checkout/PaymentStep.tsx (1)
62-70:isPendingBillmay not track the async fetch duration.
startTransitionBillreceives a synchronous callback that fires-and-forgets the promise chain. In React 19,startTransitioncan trackasyncfunctions natively — if you pass anasynccallback andawaitthe fetch,isPendingBillwill staytrueuntil the promise settles.Optional: make the transition async-aware
startTransitionBill(() => { - fetchStates(billAddress.country_iso) - .then((states) => { - if (!cancelled) setBillStates(states); - }) - .catch(() => { - if (!cancelled) setBillStates([]); - }); + return fetchStates(billAddress.country_iso) + .then((states) => { + if (!cancelled) setBillStates(states); + }) + .catch(() => { + if (!cancelled) setBillStates([]); + }); });Returning the promise lets React 19's transition machinery track it, so
isPendingBillremainstruefor the full fetch duration.src/app/[country]/[locale]/(storefront)/account/register/page.tsx (1)
101-124: Consider extracting a reusablePasswordInputcomponent.The password visibility toggle pattern (relative wrapper, input, eye icon button) is duplicated between the two fields. Since this PR is specifically about eliminating code duplication, extracting a small
PasswordInputcomponent would align well with the PR's goals.💡 Example extraction
function PasswordInput({ id, value, onChange, placeholder = "••••••••", }: { id: string; value: string; onChange: (value: string) => void; placeholder?: string; }) { const [show, setShow] = useState(false); return ( <div className="relative"> <input type={show ? "text" : "password"} id={id} value={value} onChange={(e) => onChange(e.target.value)} required minLength={6} className="mt-1 block w-full px-4 py-2 pr-10 border border-gray-300 rounded-lg focus:ring-2 focus:ring-indigo-500 focus:border-transparent" placeholder={placeholder} /> <button type="button" onClick={() => setShow(!show)} className="absolute right-3 top-1/2 -translate-y-1/2 text-gray-500 hover:text-gray-700" aria-label={show ? "Hide password" : "Show password"} > {show ? <EyeSlashIcon className="w-5 h-5" /> : <EyeIcon className="w-5 h-5" />} </button> </div> ); }Also applies to: 134-161
src/app/[country]/[locale]/(storefront)/account/gift-cards/page.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/checkout/AddressStep.tsx (1)
62-83:⚠️ Potential issue | 🟠 MajorMissing
.catch()onfetchStates— unhandled promise rejection if the call fails.If
fetchStatesrejects, there is no.catch()handler on the promise chain insidestartTransitionShip. This will surface as an unhandled promise rejection. TheAddressEditModalhandles this correctly with.catch(() => setStates([])).🛡️ Proposed fix
startTransitionShip(() => { - fetchStates(shipAddress.country_iso).then((states) => { - if (!cancelled) { - setShipStates(states); - } - }); + fetchStates(shipAddress.country_iso) + .then((states) => { + if (!cancelled) { + setShipStates(states); + } + }) + .catch(() => { + if (!cancelled) { + setShipStates([]); + } + }); });
🧹 Nitpick comments (4)
vitest.config.ts (1)
20-20: Consider using asyncreadFileinstead ofreadFileSyncin an async function.Since the
transformhook is alreadyasync, usingfs/promises.readFilewould avoid blocking the event loop and be more consistent.♻️ Suggested change
At line 1, update the import:
-import { readFileSync } from "node:fs"; +import { readFile } from "node:fs/promises";Then at line 20:
- const svg = readFileSync(id, "utf-8"); + const svg = await readFile(id, "utf-8");src/components/checkout/AddressStep.tsx (1)
188-192: Simplify callback to a direct reference.
(address) => setEditingAddress(address)can be replaced withsetEditingAddresssince the signatures match.✏️ Suggested diff
onEditAddress={ - onUpdateSavedAddress - ? (address) => setEditingAddress(address) - : undefined + onUpdateSavedAddress ? setEditingAddress : undefined }src/app/[country]/[locale]/(checkout)/checkout/[id]/page.tsx (1)
475-509: Step indicator styling: minor simplification opportunity.Lines 480–484: both the
index < currentStepIndexandindex === currentStepIndexbranches apply the same classes (bg-indigo-600 text-white). This could be collapsed into a singleindex <= currentStepIndexcheck for clarity. Not a blocker since it's pre-existing logic.✏️ Optional simplification
- className={`w-8 h-8 rounded-full flex items-center justify-center text-sm font-medium ${ - index < currentStepIndex - ? "bg-indigo-600 text-white" - : index === currentStepIndex - ? "bg-indigo-600 text-white" - : "bg-gray-200 text-gray-500" - }`} + className={`w-8 h-8 rounded-full flex items-center justify-center text-sm font-medium ${ + index <= currentStepIndex + ? "bg-indigo-600 text-white" + : "bg-gray-200 text-gray-500" + }`}src/app/[country]/[locale]/(storefront)/account/gift-cards/page.tsx (1)
183-194: Consider converting to a Server Component in a follow-up.This page is marked
"use client"solely to fetch data viauseEffect+useState. Since this is undersrc/app/, it could be a Server Component thatawaitsgetGiftCards()directly, with aloading.tsxor<Suspense>boundary for the skeleton. This would also eliminate the silent failure ifgetGiftCards()rejects (currentlysetLoadingstaystrueforever).Not blocking since the current changes only touch icons, but worth a follow-up. As per coding guidelines: "Use Server Components by default" and "Avoid using useEffect for data fetching."
2 participants
Summary
extractBasePath,getPathWithoutPrefix) from 10+ files intosrc/lib/utils/path.tsactionResult,withFallback) replacing 16+ try/catch blocks across 7 data filesuseProductListinghook +ProductListingLayoutcomponent, shrinking two ~400-line page components to ~40-60 lines eachAddressFormFields,AddressEditModal) replacing 4 copies of address form fields and 2 copies of modal+form logicAddressFormData,addressToFormData,formDataToAddress,addressesMatch) from 4 files intosrc/lib/utils/address.tsuseEffectanti-patternmutateCartin CartContext,toUserin AuthContext,setStoreCookiescookie helper)undefinedvsnullin CheckoutContext, merged guard blocks in account layout)Impact
New shared modules
src/lib/utils/path.tsextractBasePath,getPathWithoutPrefixsrc/lib/data/utils.tsactionResult,withFallbacksrc/lib/utils/address.tsAddressFormData,emptyAddress,addressToFormData,formDataToAddress,addressesMatchsrc/lib/utils/cookies.tssetStoreCookiessrc/lib/utils/product-query.tsbuildProductQueryParamssrc/hooks/useProductListing.tssrc/components/products/ProductListingLayout.tsxsrc/components/checkout/AddressFormFields.tsxsrc/components/checkout/AddressEditModal.tsxTest plan
meta, checkout page.error)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements